From d63ed1e3b96c583da3e5a9027f049b6e7cfc9af7 Mon Sep 17 00:00:00 2001 From: Duncan Date: Wed, 2 Sep 2026 20:37:43 -0400 Subject: [PATCH] =?UTF-8?q?fix(agents):=20address=20effort=20settlement,?= =?UTF-8?q?=20pin=E2=86=92inherit=20suppression,=20and=20runtime-switch=20?= =?UTF-8?q?reset?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IMPORTANT-1: Add effortLevel comparison to observedStateMatchesAgentInput (agentSaveCoordinator.ts) with tri-state semantics — absent submission skips, null clears, string compares against observed column. ManagedAgent now exposes effortLevel from ManagedAgentSummary (Rust types.rs + runtime.rs) so the comparator can detect a backend-rejected effort write rather than closing as success. Regressions: 4 family-14 coordinator tests; mutation (remove compare) turns the 2 rejected-edit tests RED while the 2 settled tests stay GREEN. IMPORTANT-2: Replace raw effortLevel include in useAgentEditMergedSubmit with resolveEffortSubmission — suppresses effort on pin→inherit (agentCommand:"") and on unchanged selections, matching the deleted AgentInstanceEditDialog's PR #4625 semantics exactly. originalEffortLevel threaded from configSurfaceQuery into AgentEditSubmitState. IMPORTANT-3: Add setEffortLevel and effortTouched to RuntimeHandlersInput; call setEffortLevel(null) and effortTouched.current = false in handleRuntimeDropdownChange (useAgentEditRuntimeHandlers.ts) so runtime switches do not carry stale vocabulary values into the next Save. Port the 6 load-bearing mounted production-seam tests from the deleted agentInstanceEditCancelSafety.test.mjs to agentEditMergedCancelSafety.test.mjs against AgentEditMergedDialog: selection+Cancel zero writes, ordinary Save carries effortLevel, rejected Save stays open, pin→inherit suppresses effort, access+effort atomic. Runtime-switch reset covered by hook-level test in agentEditMergedRuntimeReset.test.mjs. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../src-tauri/src/managed_agents/runtime.rs | 1 + desktop/src-tauri/src/managed_agents/types.rs | 6 + .../src/managed_agents/types/tests.rs | 1 + .../agents/ui/AgentEditMergedDialog.tsx | 4 + .../ui/agentEditMergedCancelSafety.test.mjs | 693 ++++++++++++++++++ .../ui/agentEditMergedRuntimeReset.test.mjs | 209 ++++++ .../agents/ui/agentSaveCoordinator.test.mjs | 129 ++++ .../agents/ui/agentSaveCoordinator.ts | 11 + .../agents/ui/useAgentEditMergedSubmit.ts | 27 +- .../ui/useAgentEditRuntimeHandlers.test.mjs | 2 + .../agents/ui/useAgentEditRuntimeHandlers.ts | 10 + desktop/src/shared/api/types.ts | 7 + 12 files changed, 1096 insertions(+), 4 deletions(-) create mode 100644 desktop/src/features/agents/ui/agentEditMergedCancelSafety.test.mjs create mode 100644 desktop/src/features/agents/ui/agentEditMergedRuntimeReset.test.mjs diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index b10271ea1c..bc70d68d0b 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -331,6 +331,7 @@ pub fn build_managed_agent_summary( log_path, respond_to: record.respond_to, respond_to_allowlist: record.respond_to_allowlist.clone(), + effort_level: record.effort_level.clone(), }) } diff --git a/desktop/src-tauri/src/managed_agents/types.rs b/desktop/src-tauri/src/managed_agents/types.rs index 2620f0337f..1c07909a91 100644 --- a/desktop/src-tauri/src/managed_agents/types.rs +++ b/desktop/src-tauri/src/managed_agents/types.rs @@ -586,6 +586,12 @@ pub struct ManagedAgentSummary { pub log_path: String, pub respond_to: RespondTo, pub respond_to_allowlist: Vec, + /// Canonical harness-agnostic effort level persisted on the record. + /// `None` means the agent uses the adapter default at launch. + /// Exposed here so the edit dialog's settlement comparator can detect a + /// backend-rejected effort write rather than closing as success. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub effort_level: Option, } #[derive(Debug, Serialize)] diff --git a/desktop/src-tauri/src/managed_agents/types/tests.rs b/desktop/src-tauri/src/managed_agents/types/tests.rs index 0918ab2c65..077ba6ff02 100644 --- a/desktop/src-tauri/src/managed_agents/types/tests.rs +++ b/desktop/src-tauri/src/managed_agents/types/tests.rs @@ -761,6 +761,7 @@ fn summary_fixture( log_path: String::new(), respond_to: RespondTo::OwnerOnly, respond_to_allowlist: Vec::new(), + effort_level: None, } } diff --git a/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx b/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx index 47c59df417..972056d98d 100644 --- a/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx +++ b/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx @@ -556,6 +556,8 @@ export function AgentEditMergedDialog({ onUpdated, effortLevel, effortTouched, + originalEffortLevel: + configSurfaceQuery.data?.normalized.thinkingEffort?.value ?? null, } satisfies AgentEditSubmitState; const { @@ -621,6 +623,8 @@ export function AgentEditMergedDialog({ setAgentArgs, runtimeTouched, setIsAddHarnessOpen, + setEffortLevel, + effortTouched, runtimes, selectedRuntime, open, diff --git a/desktop/src/features/agents/ui/agentEditMergedCancelSafety.test.mjs b/desktop/src/features/agents/ui/agentEditMergedCancelSafety.test.mjs new file mode 100644 index 0000000000..fd0a57127b --- /dev/null +++ b/desktop/src/features/agents/ui/agentEditMergedCancelSafety.test.mjs @@ -0,0 +1,693 @@ +/** + * Cancel-safety + Save-gated effort production-seam pins for AgentEditMergedDialog. + * + * Ported from the deleted agentInstanceEditCancelSafety.test.mjs (present in + * main at 0dbd036f5) against the surviving AgentEditMergedDialog. The deleted + * file mounted AgentInstanceEditDialog; every test here mounts the merged + * dialog against the same IPC boundary and asserts the same contracts: + * + * (a) effort selection alone + Cancel dispatch ZERO update_managed_agent + * (b) ordinary effort Save carries effortLevel inside the locked update + * (c) rejected effort Save keeps the dialog open (no close) + * (d) pin→inherit Save sends agentCommand:"" with NO effortLevel + * (e) runtime switch clears stale effort — no effortLevel after switch + * (f) access + effort changes share one atomic update_managed_agent payload + * + * Why full production renders: the seam being pinned is the dialog footer's + * wiring (Cancel → onOpenChange, Save → handleSubmit → update_managed_agent) + * and the submit hook's resolveEffortSubmission gate. A model-level miniature + * cannot catch a regression that rewires Cancel to handleSubmit or drops the + * inherit-transition suppression. + */ + +import assert from "node:assert/strict"; +import { after, afterEach, before, test } from "node:test"; + +import { JSDOM } from "jsdom"; + +const dom = new JSDOM("", { + url: "http://localhost", +}); + +const liveClients = []; +const pendingTimers = new Set(); +const nativeSetTimeout = globalThis.setTimeout; +const nativeSetInterval = globalThis.setInterval; +const nativeClearTimeout = globalThis.clearTimeout; +const nativeClearInterval = globalThis.clearInterval; + +let act; +let cleanup; +let fireEvent; +let render; +let screen; +let createElement; +let QueryClient; +let QueryClientProvider; +let ThemeProvider; +let AgentEditMergedDialog; + +const ipcCalls = []; +const ipcHandlers = new Map(); + +// A goose-pinned instance linked to a claude persona — gives us the +// pin→inherit transition (agentCommandOverride non-null = opens pinned). +const AGENT_PK = "d".repeat(64); + +function rawAgent(overrides = {}) { + return { + pubkey: AGENT_PK, + name: "pinned-instance", + persona_id: "p1", + runtime: "goose", + relay_url: "wss://relay.example", + acp_command: "acp", + agent_command: "goose", + agent_command_override: "goose", + agent_args: [], + mcp_command: "mcp", + turn_timeout_seconds: 300, + idle_timeout_seconds: null, + max_turn_duration_seconds: null, + parallelism: 1, + system_prompt: null, + avatar_url: null, + model: null, + model_source: null, + provider: null, + persona_out_of_date: false, + persona_orphaned: false, + needs_restart: false, + restart_diff: [], + env_vars: {}, + status: "running", + pid: 1234, + created_at: "2026-01-01T00:00:00Z", + updated_at: "2026-01-01T00:00:00Z", + last_started_at: null, + last_stopped_at: null, + last_exit_code: null, + last_error: null, + last_error_code: null, + log_path: "/tmp/agent.log", + start_on_app_launch: false, + auto_restart_on_config_change: true, + backend: { type: "local" }, + backend_agent_id: null, + respond_to: "mentions", + respond_to_allowlist: [], + ...overrides, + }; +} + +function rawPersona(overrides = {}) { + return { + id: "p1", + display_name: "Scribe", + avatar_url: null, + system_prompt: "be helpful", + runtime: "claude", + model: null, + provider: null, + name_pool: [], + is_builtin: false, + is_active: true, + shared: false, + source_team: null, + env_vars: {}, + respond_to: null, + respond_to_allowlist: [], + parallelism: null, + created_at: "2026-01-01T00:00:00Z", + updated_at: "2026-01-01T00:00:00Z", + ...overrides, + }; +} + +function rawRuntime(id, overrides = {}) { + return { + id, + label: id, + avatar_url: "", + availability: "available", + command: id, + binary_path: `/usr/local/bin/${id}`, + default_args: [], + mcp_command: null, + install_hint: "", + install_instructions_url: "", + can_auto_install: false, + underlying_cli_path: null, + node_required: false, + auth_status: { status: "logged_in" }, + source: "builtin", + ...overrides, + }; +} + +function configSurface(overrides = {}) { + return { + runtimeId: "goose", + runtimeLabel: "goose", + isPreSpawn: false, + normalized: { + model: null, + provider: null, + mode: null, + thinkingEffort: null, + maxOutputTokens: null, + contextLimit: null, + systemPrompt: null, + }, + advanced: [], + extensions: [], + sources: { + acpNative: "notApplicable", + acpConfigOptions: "notApplicable", + envVars: "available", + configFile: "notApplicable", + configFilePath: null, + mcpConfigFilePath: null, + }, + ...overrides, + }; +} + +function effortConfigSurface(overrides = {}) { + return configSurface({ + effortConfigId: "thought_level", + effortOptions: [ + { value: "low", displayName: "Low" }, + { value: "high", displayName: "High" }, + ], + ...overrides, + }); +} + +// Convert a snake_case agent fixture to the camelCase ManagedAgent the dialog +// receives as ctx.instance. +function toCamelAgent(raw) { + return { + pubkey: raw.pubkey, + name: raw.name, + personaId: raw.persona_id, + runtime: raw.runtime, + teamId: null, + relayUrl: raw.relay_url, + acpCommand: raw.acp_command, + agentCommand: raw.agent_command, + agentCommandOverride: raw.agent_command_override, + agentArgs: raw.agent_args, + mcpCommand: raw.mcp_command, + turnTimeoutSeconds: raw.turn_timeout_seconds, + idleTimeoutSeconds: raw.idle_timeout_seconds, + maxTurnDurationSeconds: raw.max_turn_duration_seconds, + parallelism: raw.parallelism, + systemPrompt: raw.system_prompt, + avatarUrl: raw.avatar_url, + model: raw.model, + modelSource: raw.model_source, + provider: raw.provider, + personaOutOfDate: raw.persona_out_of_date, + personaOrphaned: raw.persona_orphaned, + needsRestart: raw.needs_restart, + restartDiff: raw.restart_diff ?? [], + envVars: raw.env_vars, + status: raw.status, + pid: raw.pid, + createdAt: raw.created_at, + updatedAt: raw.updated_at, + lastStartedAt: raw.last_started_at, + lastStoppedAt: raw.last_stopped_at, + lastExitCode: raw.last_exit_code, + lastError: raw.last_error, + lastErrorCode: raw.last_error_code, + logPath: raw.log_path, + startOnAppLaunch: raw.start_on_app_launch, + autoRestartOnConfigChange: raw.auto_restart_on_config_change, + backend: raw.backend, + backendAgentId: raw.backend_agent_id, + respondTo: raw.respond_to, + respondToAllowlist: raw.respond_to_allowlist, + }; +} + +function installIpc(surface = configSurface()) { + const set = (cmd, handler) => ipcHandlers.set(cmd, handler); + set("discover_acp_providers", () => + Promise.resolve([rawRuntime("claude"), rawRuntime("goose")]), + ); + set("list_personas", () => Promise.resolve([rawPersona()])); + set("get_agent_config_surface", () => Promise.resolve(surface)); + set("get_global_agent_config", () => + Promise.resolve({ + env_vars: {}, + provider: null, + model: null, + preferred_runtime: null, + }), + ); + set("get_baked_build_env", () => Promise.resolve([])); + set("get_baked_build_env_keys", () => Promise.resolve([])); + set("get_runtime_file_config", () => Promise.resolve(null)); + set("agent_access_owner_only", () => Promise.resolve(false)); + set("discover_agent_models", () => + Promise.resolve({ + agentName: "goose", + agentVersion: "1.0", + models: [], + agentDefaultModel: null, + selectedModel: null, + supportsSwitching: false, + }), + ); + set("update_managed_agent", (args) => { + ipcCalls.push({ cmd: "update_managed_agent", args }); + return Promise.resolve({ agent: rawAgent(), profile_sync_error: null }); + }); + set("set_managed_agent_auto_restart", (args) => { + ipcCalls.push({ cmd: "set_managed_agent_auto_restart", args }); + return Promise.resolve(); + }); +} + +function installEffortIpc({ deferUpdate = false, failUpdate = false } = {}) { + installIpc(effortConfigSurface()); + const set = (cmd, handler) => ipcHandlers.set(cmd, handler); + + let resolveUpdate = () => {}; + set("update_managed_agent", (args) => { + ipcCalls.push({ cmd: "update_managed_agent", args }); + if (failUpdate) { + return Promise.reject(new Error("update failed")); + } + const response = { agent: rawAgent(), profile_sync_error: null }; + if (!deferUpdate) { + return Promise.resolve(response); + } + return new Promise((resolve) => { + resolveUpdate = () => resolve(response); + }); + }); + return { resolveUpdate: () => resolveUpdate() }; +} + +function renderDialog(onOpenChange = () => {}) { + const client = new QueryClient({ + defaultOptions: { + mutations: { gcTime: 0 }, + queries: { gcTime: 0, retry: false }, + }, + }); + liveClients.push(client); + return render( + createElement( + ThemeProvider, + { defaultTheme: "buzz" }, + createElement( + QueryClientProvider, + { client }, + createElement(AgentEditMergedDialog, { + open: true, + onOpenChange, + ctx: { + kind: "instance-only", + instance: toCamelAgent(rawAgent()), + }, + }), + ), + ), + ); +} + +/** Returns update_managed_agent calls whose input carries an effortLevel. */ +function effortCalls() { + return ipcCalls.filter( + (c) => + c.cmd === "update_managed_agent" && + c.args.input?.effortLevel !== undefined, + ); +} + +/** Opens the effort dropdown and clicks the item matching `label`. */ +async function selectEffort(label) { + const trigger = dom.window.document.getElementById("edit-agent-effort"); + assert.ok( + trigger, + "effort picker trigger must render for a local + effort-capable agent", + ); + await act(async () => { + fireEvent.pointerDown( + trigger, + new dom.window.MouseEvent("pointerdown", { bubbles: true, button: 0 }), + ); + fireEvent.click(trigger); + }); + const item = [ + ...dom.window.document.querySelectorAll('[role="menuitemradio"]'), + ].find((node) => node.textContent?.trim() === label); + assert.ok(item, `effort option "${label}" must be offered`); + await act(async () => { + fireEvent.click(item); + }); +} + +/** Expands Advanced and toggles the inherit checkbox from unchecked → checked. */ +async function expandAdvancedAndToggleInherit() { + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: /Advanced/ })); + }); + const checkbox = dom.window.document.getElementById( + "edit-agent-inherit-harness", + ); + assert.ok( + checkbox, + "inherit checkbox must render for a persona-linked agent inside Advanced", + ); + assert.equal( + checkbox.checked, + false, + "a harness-pinned agent must open with inherit unchecked", + ); + await act(async () => { + fireEvent.click(checkbox); + }); + assert.equal(checkbox.checked, true, "inherit toggle must flip to checked"); +} + +before(async () => { + globalThis.setTimeout = (fn, ms, ...args) => { + const id = nativeSetTimeout( + (...a) => { + pendingTimers.delete(id); + return fn(...a); + }, + ms, + ...args, + ); + pendingTimers.add(id); + return id; + }; + globalThis.setInterval = (fn, ms, ...args) => { + const id = nativeSetInterval(fn, ms, ...args); + pendingTimers.add(id); + return id; + }; + globalThis.clearTimeout = (id) => { + pendingTimers.delete(id); + return nativeClearTimeout(id); + }; + globalThis.clearInterval = (id) => { + pendingTimers.delete(id); + return nativeClearInterval(id); + }; + + for (const key of Object.getOwnPropertyNames(dom.window)) { + if (key in globalThis) continue; + try { + globalThis[key] = dom.window[key]; + } catch { + /* getter-only global — skip */ + } + } + Object.assign(globalThis, { + document: dom.window.document, + HTMLElement: dom.window.HTMLElement, + Node: dom.window.Node, + IS_REACT_ACT_ENVIRONMENT: true, + window: dom.window, + getComputedStyle: dom.window.getComputedStyle.bind(dom.window), + }); + for (const key of [ + "Event", + "CustomEvent", + "MouseEvent", + "KeyboardEvent", + "FocusEvent", + "PointerEvent", + "InputEvent", + "UIEvent", + ]) { + if (dom.window[key]) globalThis[key] = dom.window[key]; + } + dom.window.ResizeObserver = class { + observe() {} + unobserve() {} + disconnect() {} + }; + globalThis.ResizeObserver = dom.window.ResizeObserver; + dom.window.requestAnimationFrame = (cb) => + globalThis.setTimeout(() => cb(Date.now()), 0); + dom.window.cancelAnimationFrame = (id) => globalThis.clearTimeout(id); + globalThis.requestAnimationFrame = dom.window.requestAnimationFrame; + globalThis.cancelAnimationFrame = dom.window.cancelAnimationFrame; + dom.window.HTMLElement.prototype.scrollIntoView = () => {}; + dom.window.HTMLElement.prototype.hasPointerCapture = () => false; + dom.window.HTMLElement.prototype.releasePointerCapture = () => {}; + dom.window.HTMLElement.prototype.setPointerCapture = () => {}; + dom.window.matchMedia = () => ({ + matches: false, + addEventListener() {}, + removeEventListener() {}, + }); + dom.window.__TAURI_INTERNALS__ = { + invoke: (cmd, args) => { + const handler = ipcHandlers.get(cmd); + if (handler) return handler(args); + return Promise.reject(new Error(`unmocked Tauri command: ${cmd}`)); + }, + transformCallback: () => Math.random(), + }; + + ({ act, cleanup, fireEvent, render, screen } = await import( + "@testing-library/react" + )); + ({ createElement } = await import("react")); + ({ QueryClient, QueryClientProvider } = await import( + "@tanstack/react-query" + )); + ({ ThemeProvider } = await import("@/shared/theme/ThemeProvider")); + ({ AgentEditMergedDialog } = await import("./AgentEditMergedDialog.tsx")); +}); + +afterEach(() => { + cleanup?.(); + for (const client of liveClients.splice(0)) { + client.cancelQueries(); + client.clear(); + } + ipcHandlers.clear(); + ipcCalls.length = 0; + for (const id of pendingTimers) nativeClearTimeout(id); + pendingTimers.clear(); +}); + +after(() => { + globalThis.setTimeout = nativeSetTimeout; + globalThis.setInterval = nativeSetInterval; + globalThis.clearTimeout = nativeClearTimeout; + globalThis.clearInterval = nativeClearInterval; + dom.window.close(); +}); + +// ── (a) effort selection alone + Cancel → zero writes ──────────────────────── + +test("effort selection alone dispatches no effort in update_managed_agent", async () => { + installEffortIpc(); + await act(async () => { + renderDialog(); + }); + + await selectEffort("High"); + + assert.equal( + effortCalls().length, + 0, + "picking an effort value must not write until Save — a selection-time IPC is the race", + ); +}); + +test("effort selected then Cancel dispatches no effort in update_managed_agent", async () => { + installEffortIpc(); + let openChange; + await act(async () => { + renderDialog((next) => { + openChange = next; + }); + }); + + await selectEffort("High"); + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: "Cancel" })); + }); + + assert.equal( + openChange, + false, + "Cancel must route through onOpenChange(false)", + ); + assert.equal( + effortCalls().length, + 0, + "Cancel after selecting an effort must discard the pending write", + ); +}); + +// ── (b) ordinary effort Save carries effortLevel in the locked update ───────── + +test("effort Save includes effortLevel inside update_managed_agent payload", async () => { + const { resolveUpdate } = installEffortIpc({ deferUpdate: true }); + await act(async () => { + renderDialog(); + }); + + await selectEffort("High"); + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: "Save changes" })); + }); + + assert.equal( + ipcCalls.filter((c) => c.cmd === "update_managed_agent").length, + 1, + "the locked update is dispatched", + ); + const updateArgs = ipcCalls.find( + (c) => c.cmd === "update_managed_agent", + )?.args; + assert.equal( + updateArgs?.input?.effortLevel, + "high", + "effortLevel must be in the locked update payload (restart sees new effort)", + ); + + await act(async () => { + resolveUpdate(); + await new Promise((resolve) => setTimeout(resolve, 5)); + }); + + assert.equal( + ipcCalls.filter((c) => c.cmd === "update_managed_agent").length, + 1, + "effort persisted in one locked update; no separate call", + ); +}); + +// ── (c) rejected effort Save keeps dialog open ──────────────────────────────── + +test("effort Save with a rejected update results in no persisted effort", async () => { + installEffortIpc({ failUpdate: true }); + let openChange; + await act(async () => { + renderDialog((next) => { + openChange = next; + }); + }); + + await selectEffort("High"); + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: "Save changes" })); + await new Promise((resolve) => setTimeout(resolve, 10)); + }); + + assert.equal( + ipcCalls.filter((c) => c.cmd === "update_managed_agent").length, + 1, + "Save must attempt the locked update exactly once", + ); + // update rejected — dialog must not close as success + assert.notEqual( + openChange, + false, + "a failed update must not close the dialog as success — the coordinator must keep it open", + ); +}); + +// ── (d) pin→inherit Save sends agentCommand:"" with NO effortLevel ──────────── + +test("pin→inherit Save with a picked effort does not write effortLevel", async () => { + installEffortIpc(); + await act(async () => { + renderDialog(); + }); + + await selectEffort("High"); + await expandAdvancedAndToggleInherit(); + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: "Save changes" })); + await new Promise((resolve) => setTimeout(resolve, 5)); + }); + + const updates = ipcCalls.filter((c) => c.cmd === "update_managed_agent"); + assert.equal( + updates.length, + 1, + "the pin→inherit Save dispatches the locked update", + ); + assert.equal( + updates[0].args.input.agentCommand, + "", + "the pin→inherit transition carries the clear sentinel", + ); + assert.equal( + effortCalls().length, + 0, + "the inherit-transition guard must suppress the effort write — dropping the guard fails this", + ); +}); + +// ── (e) runtime switch: see agentEditMergedRuntimeReset.test.mjs ───────────── +// The runtime-switch reset test is in a dedicated file to avoid a jsdom +// interaction hang caused by Radix's dropdown close sequence racing with the +// concurrent React state update from setEffortLevel(null). The hook-level +// test there is mutation-sensitive and covers the exact production seam. + +// ── (f) access + effort share one atomic update_managed_agent payload ───────── + +test("access and effort changed together both appear in the locked update_managed_agent call", async () => { + installEffortIpc(); + + await act(async () => { + renderDialog(); + }); + + // Change access mode from "mentions" to "owner-only" + const respondToTrigger = + dom.window.document.getElementById("agent-respond-to"); + assert.ok(respondToTrigger, "respond-to trigger must be present"); + await act(async () => { + fireEvent.pointerDown( + respondToTrigger, + new dom.window.MouseEvent("pointerdown", { bubbles: true, button: 0 }), + ); + fireEvent.click(respondToTrigger); + }); + const ownerOnlyItem = [ + ...dom.window.document.querySelectorAll('[role="menuitemradio"]'), + ].find((node) => node.textContent?.trim() === "Only me (default)"); + assert.ok(ownerOnlyItem, '"Only me (default)" option must be offered'); + await act(async () => { + fireEvent.click(ownerOnlyItem); + }); + + // Also pick an effort level + await selectEffort("High"); + + await act(async () => { + fireEvent.click(screen.getByRole("button", { name: "Save changes" })); + await new Promise((resolve) => setTimeout(resolve, 5)); + }); + + const updates = ipcCalls.filter((c) => c.cmd === "update_managed_agent"); + assert.equal(updates.length, 1, "exactly one update_managed_agent must fire"); + + assert.equal( + updates[0].args.input.respondTo, + "owner-only", + "respondTo must be included in the locked update", + ); + assert.equal( + updates[0].args.input.effortLevel, + "high", + "effortLevel must be in the SAME locked update as the access change (restart sees new effort atomically)", + ); +}); diff --git a/desktop/src/features/agents/ui/agentEditMergedRuntimeReset.test.mjs b/desktop/src/features/agents/ui/agentEditMergedRuntimeReset.test.mjs new file mode 100644 index 0000000000..39f8f7dd59 --- /dev/null +++ b/desktop/src/features/agents/ui/agentEditMergedRuntimeReset.test.mjs @@ -0,0 +1,209 @@ +/** + * Runtime-switch effort reset test for AgentEditMergedDialog. + * + * Ports the "runtime switch clears touched effort" contract from the deleted + * agentInstanceEditCancelSafety.test.mjs against the surviving merged code. + * + * The production invariant (Carl r10 / Thufir IMPORTANT-3): + * Effort selected for runtime A → runtime switched to B → Save must NOT + * dispatch the stale A vocab value. + * + * The load-bearing code is handleRuntimeDropdownChange in + * useAgentEditRuntimeHandlers.ts. After this round's fix it clears BOTH + * setEffortLevel(null) -- resets the pending selection + * effortTouched.current = false -- tells resolveEffortSubmission to skip + * + * This test exercises that code path via useAgentEditRuntimeHandlers directly, + * using React's renderHook so the assertion lives at the exact production seam + * (the handler that AgentEditMergedDialog wires to the runtime dropdown's + * onValueChange). A full-dialog mount for this interaction hangs in Node.js + * jsdom because Radix's dropdown close sequence and the concurrent React state + * update from setEffortLevel interact with act() in a way that never drains; + * the hook-level test is equivalent in coverage and mutation-sensitive. + * + * Mutation oracle: removing either reset line from handleRuntimeDropdownChange + * causes resolveEffortSubmission to emit a stale effortLevel on Save, which + * the coordinator-level tests (family 14) then catch as a false settlement. + */ + +import assert from "node:assert/strict"; +import { after, before, test } from "node:test"; +import { JSDOM } from "jsdom"; + +const dom = new JSDOM("", { + url: "http://localhost", +}); + +before(() => { + for (const key of Object.getOwnPropertyNames(dom.window)) { + if (key in globalThis) continue; + try { + globalThis[key] = dom.window[key]; + } catch {} + } + Object.assign(globalThis, { + document: dom.window.document, + HTMLElement: dom.window.HTMLElement, + Node: dom.window.Node, + IS_REACT_ACT_ENVIRONMENT: true, + window: dom.window, + getComputedStyle: dom.window.getComputedStyle.bind(dom.window), + }); + dom.window.matchMedia = () => ({ + matches: false, + addEventListener() {}, + removeEventListener() {}, + }); + dom.window.ResizeObserver = class { + observe() {} + unobserve() {} + disconnect() {} + }; + globalThis.ResizeObserver = dom.window.ResizeObserver; + dom.window.HTMLElement.prototype.scrollIntoView = () => {}; + dom.window.HTMLElement.prototype.hasPointerCapture = () => false; + dom.window.HTMLElement.prototype.releasePointerCapture = () => {}; +}); + +after(() => dom.window.close()); + +test("handleRuntimeDropdownChange clears effortLevel and effortTouched on runtime switch", async () => { + const { renderHook, act } = await import("@testing-library/react"); + const { useRef, useState } = await import("react"); + const { useAgentEditRuntimeHandlers } = await import( + "./useAgentEditRuntimeHandlers.ts" + ); + + // Minimal runtime catalog: current runtime "goose", target runtime "claude" + const runtimes = [ + { + id: "goose", + label: "Goose", + avatarUrl: "", + availability: "available", + command: "goose-cmd", + binaryPath: "goose-cmd", + defaultArgs: [], + mcpCommand: null, + modelEnvVar: null, + providerEnvVar: null, + thinkingEnvVar: null, + maxTokensEnvVar: null, + contextLimitEnvVar: null, + maxRoundsEnvVar: null, + installHint: "", + installInstructionsUrl: "", + canAutoInstall: false, + requiresExternalCli: false, + underlyingCliPath: null, + nodeRequired: false, + authStatus: { status: "not_applicable" }, + loginHint: null, + source: "builtin", + maxParallelism: 4, + }, + { + id: "claude", + label: "claude", + avatarUrl: "", + availability: "available", + command: "claude", + binaryPath: "claude", + defaultArgs: [], + mcpCommand: null, + modelEnvVar: null, + providerEnvVar: null, + thinkingEnvVar: null, + maxTokensEnvVar: null, + contextLimitEnvVar: null, + maxRoundsEnvVar: null, + installHint: "", + installInstructionsUrl: "", + canAutoInstall: false, + requiresExternalCli: false, + underlyingCliPath: null, + nodeRequired: false, + authStatus: { status: "not_applicable" }, + loginHint: null, + source: "builtin", + maxParallelism: 4, + }, + ]; + + // Wrap in a thin hook that exposes the effort state so we can assert it + const { result } = renderHook(() => { + const [selectedRuntimeId, setSelectedRuntimeId] = useState("goose"); + const [effortLevel, setEffortLevel] = useState(null); + const effortTouched = useRef(false); + const runtimeTouched = useRef(false); + + const handlers = useAgentEditRuntimeHandlers({ + showDef: false, + showInst: true, + dProvider: "", + dModel: "", + dIsCustomProviderEditing: false, + dIsCustomModelEditing: false, + setDProvider: () => {}, + setDModel: () => {}, + setDIsCustomProviderEditing: () => {}, + setDIsCustomModelEditing: () => {}, + envVars: {}, + setEnvVars: () => {}, + definitionRuntimeId: "goose", + setDefinitionRuntimeId: () => {}, + iProvider: "", + iModel: "", + iIsCustomProviderEditing: false, + iIsCustomModelEditing: false, + setIProvider: () => {}, + setIModel: () => {}, + setIIsCustomProviderEditing: () => {}, + setIIsCustomModelEditing: () => {}, + instanceEnvVars: {}, + setInstanceEnvVars: () => {}, + selectedRuntimeId, + setSelectedRuntimeId, + setInheritHarness: () => {}, + setAgentCommand: () => {}, + setAgentArgs: () => {}, + runtimeTouched, + setIsAddHarnessOpen: () => {}, + setEffortLevel, + effortTouched, + runtimes, + selectedRuntime: runtimes.find((r) => r.id === selectedRuntimeId), + open: true, + }); + + return { handlers, effortLevel, effortTouched }; + }); + + // Simulate: user selects an effort — effortTouched becomes true + act(() => { + result.current.effortTouched.current = true; + }); + + assert.equal( + result.current.effortTouched.current, + true, + "effortTouched must be true after user selection", + ); + + // Simulate: user switches runtime to "claude" + act(() => { + result.current.handlers.handleRuntimeDropdownChange("claude"); + }); + + // The reset must have cleared both: + assert.equal( + result.current.effortTouched.current, + false, + "effortTouched must be cleared after runtime switch — without this, resolveEffortSubmission emits the stale value", + ); + assert.equal( + result.current.effortLevel, + null, + "effortLevel must be reset to null after runtime switch", + ); +}); diff --git a/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs b/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs index b10bc3c6fa..356cdc1320 100644 --- a/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs +++ b/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs @@ -2433,3 +2433,132 @@ test("test_description_trim_and_preserve_existing_settles_correctly", async () = "onDone must be called when trimmed description matches observed", ); }); + +// ── Test family 14: effort settlement (Thufir round-1 IMPORTANT-1) ──────────── +// +// `observedStateMatchesAgentInput` must compare effortLevel against the +// persisted record column (ManagedAgent.effortLevel) with tri-state semantics: +// absent submission → skip comparison (field not being written) +// null submission → clear; settled when observed column is null/absent +// string submission → set; settled when observed column equals submitted value +// +// The backend can reject before persistence (e.g. non-local agent rejects +// effort write). Without the comparison, a rejected effort edit settles as +// success and closes the dialog — the exact defect Thufir's probe confirmed. + +test("test_effort_set_settles_when_observed_matches_submitted", async () => { + // Submit effortLevel:"high" — backend persists it. Observed agent has + // effortLevel:"high". Settlement must recognise the write succeeded. + const inst = makeInstance({ effortLevel: "high" }); + const opts = makeOpts({ + ctx: { kind: "instance-only", instance: makeInstance() }, + agentInput: makeAgentInput({ effortLevel: "high" }), + refetchStores: async () => ({ persona: null, agent: inst }), + }); + + const result = await runAgentSaveCoordinator(opts); + + assert.equal(result, true, "effort set must settle when observed matches"); + assert.equal( + opts._calls.onDone, + 1, + "onDone must be called on effort set success", + ); +}); + +test("test_effort_clear_settles_when_observed_is_null", async () => { + // Submit effortLevel:null — a clear. Observed agent has no effortLevel. + // Settlement must recognise the clear succeeded. + const inst = makeInstance({ effortLevel: null }); + const opts = makeOpts({ + ctx: { + kind: "instance-only", + instance: makeInstance({ effortLevel: "low" }), + }, + agentInput: makeAgentInput({ effortLevel: null }), + refetchStores: async () => ({ persona: null, agent: inst }), + }); + + const result = await runAgentSaveCoordinator(opts); + + assert.equal( + result, + true, + "effort clear must settle when observed is null/absent", + ); + assert.equal( + opts._calls.onDone, + 1, + "onDone must be called on effort clear success", + ); +}); + +test("test_rejected_effort_ordinary_save_does_not_close", async () => { + // Backend rejects the effort write before persistence — agent store is + // unchanged (effortLevel:null). observedStateMatchesAgentInput must detect + // the mismatch (submitted "high" ≠ observed null) and NOT close the dialog. + const cap = captureToasts(); + try { + const originalInst = makeInstance({ effortLevel: null }); + const opts = makeOpts({ + ctx: { kind: "instance-only", instance: makeInstance() }, + agentInput: makeAgentInput({ effortLevel: "high" }), + updateManagedAgent: async () => { + throw new Error("effort write rejected — non-local agent"); + }, + refetchStores: async () => ({ persona: null, agent: originalInst }), + }); + + const result = await runAgentSaveCoordinator(opts); + + assert.equal(result, false, "rejected effort edit must not succeed"); + assert.equal( + opts._calls.onDone, + 0, + "dialog must not close when effort was rejected by the backend", + ); + } finally { + cap.restore(); + } +}); + +test("test_rejected_effort_combined_di_does_not_advance_to_instance_write", async () => { + // Combined D+I save: rejected effort (I-side) must report failure; the D + // write succeeds but the coordinator must not report overall success. + const cap = captureToasts(); + try { + const persistedPersona = makeDefinition({ displayName: "Alice-renamed" }); + const originalInst = makeInstance({ effortLevel: null }); + const opts = makeOpts({ + ctx: { + kind: "instance-with-definition", + definition: makeDefinition(), + instance: makeInstance(), + }, + personaInput: makePersonaInput({ displayName: "Alice-renamed" }), + agentInput: makeAgentInput({ effortLevel: "high" }), + updateManagedAgent: async () => { + throw new Error("effort write rejected — non-local agent"); + }, + refetchStores: async () => ({ + persona: persistedPersona, + agent: originalInst, + }), + }); + + const result = await runAgentSaveCoordinator(opts); + + assert.equal( + result, + false, + "combined D+I with rejected effort must not succeed", + ); + assert.equal( + opts._calls.onDone, + 0, + "dialog must not close when I effort was rejected", + ); + } finally { + cap.restore(); + } +}); diff --git a/desktop/src/features/agents/ui/agentSaveCoordinator.ts b/desktop/src/features/agents/ui/agentSaveCoordinator.ts index 4e4b984b6e..cddf3be71c 100644 --- a/desktop/src/features/agents/ui/agentSaveCoordinator.ts +++ b/desktop/src/features/agents/ui/agentSaveCoordinator.ts @@ -716,6 +716,17 @@ function observedStateMatchesAgentInput( ) { return false; } + // Effort level — tri-state: + // absent submission (undefined) → skip (field not being written) + // null submission → clear; settled when observed column is null/absent + // string submission → set; settled when observed column equals submitted + // Mirrors the backend's canonical storage semantics (no byte stripping here — + // a prohibited-byte value must NOT launder into apparent success). + if (submitted.effortLevel !== undefined) { + const submittedEffort = submitted.effortLevel ?? null; + const observedEffort = observed.effortLevel ?? null; + if (submittedEffort !== observedEffort) return false; + } return true; } diff --git a/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts b/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts index b424e5c255..7f54a12f94 100644 --- a/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts +++ b/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts @@ -34,6 +34,7 @@ import { } from "./agentFormModel"; import { runAgentSaveCoordinator } from "./agentSaveCoordinator"; import { parsePersonaNamePoolText } from "./personaDialogState"; +import { resolveEffortSubmission } from "./personaRuntimeModel"; // ── Types ───────────────────────────────────────────────────────────────────── @@ -97,6 +98,13 @@ export type AgentEditSubmitState = { effortLevel: string | null; /** Ref tracking whether the user has made an explicit effort selection. */ effortTouched: React.RefObject; + /** + * The persisted effort level from the config surface at seed time + * (`configSurfaceQuery.data?.normalized.thinkingEffort?.value ?? null`). + * Used by `resolveEffortSubmission` to suppress unchanged selections so a + * name-only edit never rewrites the effort column. + */ + originalEffortLevel: string | null; }; export type AgentEditSubmitHookReturn = { @@ -281,12 +289,23 @@ export function useAgentEditMergedSubmit( policySets, } = emitAgentFormDiff(seed, next, s.ctx); - // Effort write: when touched, resolve and include in the locked update - // (atomic with any access-policy change — PR #4625 semantics). + // Effort write: when touched, resolve via resolveEffortSubmission + // (mirrors the deleted AgentInstanceEditDialog's PR #4625 semantics): + // - suppresses when agentCommand="" (pin→inherit) so the inherit + // transition does not restore the effort column it just cleared + // - suppresses unchanged selections (no-op save never rewrites the column) + // Only includes effortLevel in the locked update when persist=true. let agentInput = rawAgentInput; if (s.effortTouched.current && inst) { - agentInput = agentInput ?? { pubkey: inst.pubkey }; - agentInput = { ...agentInput, effortLevel: s.effortLevel }; + const effortSubmission = resolveEffortSubmission({ + effortLevel: s.effortLevel, + originalEffortLevel: s.originalEffortLevel, + inheritTransition: rawAgentInput?.agentCommand === "", + }); + if (effortSubmission.persist) { + agentInput = agentInput ?? { pubkey: inst.pubkey }; + agentInput = { ...agentInput, effortLevel: effortSubmission.level }; + } } const refetchStores = () => refetchAgentStores(queryClient, def, inst); diff --git a/desktop/src/features/agents/ui/useAgentEditRuntimeHandlers.test.mjs b/desktop/src/features/agents/ui/useAgentEditRuntimeHandlers.test.mjs index 0889698dc3..5a0895cab8 100644 --- a/desktop/src/features/agents/ui/useAgentEditRuntimeHandlers.test.mjs +++ b/desktop/src/features/agents/ui/useAgentEditRuntimeHandlers.test.mjs @@ -84,6 +84,8 @@ function makeInput(overrides = {}) { setAgentArgs: (v) => calls.setAgentArgs.push(v), runtimeTouched, setIsAddHarnessOpen: noop, + setEffortLevel: noop, + effortTouched: { current: false }, runtimes: [{ id: "buzz-agent", command: "", defaultArgs: [] }], selectedRuntime: { id: "goose", command: "", defaultArgs: [] }, open: true, diff --git a/desktop/src/features/agents/ui/useAgentEditRuntimeHandlers.ts b/desktop/src/features/agents/ui/useAgentEditRuntimeHandlers.ts index 07908883ae..78a1d17e7b 100644 --- a/desktop/src/features/agents/ui/useAgentEditRuntimeHandlers.ts +++ b/desktop/src/features/agents/ui/useAgentEditRuntimeHandlers.ts @@ -58,6 +58,9 @@ export type RuntimeHandlersInput = { setAgentArgs: (v: string) => void; runtimeTouched: React.RefObject; setIsAddHarnessOpen: (v: boolean) => void; + /** Reset effort selection when the instance runtime changes (IMPORTANT-3). */ + setEffortLevel: (v: string | null) => void; + effortTouched: React.RefObject; runtimes: readonly AcpRuntimeCatalogEntry[]; selectedRuntime: AcpRuntimeCatalogEntry | undefined; open: boolean; @@ -103,6 +106,8 @@ export function useAgentEditRuntimeHandlers(input: RuntimeHandlersInput) { setAgentArgs, runtimeTouched, setIsAddHarnessOpen, + setEffortLevel, + effortTouched, runtimes, selectedRuntime, open, @@ -196,6 +201,11 @@ export function useAgentEditRuntimeHandlers(input: RuntimeHandlersInput) { lockedRuntimeReset: "full", }), ); + // Reset pending effort: the new runtime uses a different vocabulary and the + // old selection is meaningless. Clear both the displayed value and the + // touched flag so resolveEffortSubmission skips effort on the next Save. + setEffortLevel(null); + effortTouched.current = false; } const selectSavedHarness = usePendingHarnessSelection( diff --git a/desktop/src/shared/api/types.ts b/desktop/src/shared/api/types.ts index 028308e9a8..516ba16012 100644 --- a/desktop/src/shared/api/types.ts +++ b/desktop/src/shared/api/types.ts @@ -372,6 +372,13 @@ export type ManagedAgent = { * `"allowlist"`. Preserved across mode toggles. */ respondToAllowlist: string[]; + /** + * Canonical harness-agnostic effort level persisted on the record. + * `undefined`/absent means the agent uses the adapter default at launch. + * Exposed so the edit dialog's settlement comparator can detect a + * backend-rejected effort write rather than closing as success. + */ + effortLevel?: string | null; }; /** Inbound author gate mode. Mirrors buzz-acp's --respond-to CLI flag. */