From 08a2944878e72ead4fe2f3ad4018070a983ca0da Mon Sep 17 00:00:00 2001 From: Duncan Date: Fri, 28 Aug 2026 10:44:17 -0400 Subject: [PATCH] fix(desktop): keep Relay Mesh runtime switch on the shown layer and name the observed agent handleProviderDropdownChange forced buzz-agent through the instance handler unconditionally, so a linked edit pinned the clicked instance to buzz-agent while the definition kept its old runtime. Route through the definition handler when a definition is shown and consult that layer's runtime id. The full-success toast preferred latestAgent?.name, which only advances on a non-throwing updateManagedAgent, so a rename that committed but whose command threw after commit reported the stale pre-save name. Prefer the observed name from the final refetch. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../agents/ui/agentSaveCoordinator.test.mjs | 53 ++++++ .../agents/ui/agentSaveCoordinator.ts | 8 +- .../ui/useAgentEditRuntimeHandlers.test.mjs | 179 ++++++++++++++++++ .../agents/ui/useAgentEditRuntimeHandlers.ts | 15 +- 4 files changed, 252 insertions(+), 3 deletions(-) create mode 100644 desktop/src/features/agents/ui/useAgentEditRuntimeHandlers.test.mjs diff --git a/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs b/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs index 7a57127996..58bb1ed2b4 100644 --- a/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs +++ b/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs @@ -1202,3 +1202,56 @@ test("test_drift_guard_inert_when_no_expected_updatedAt_supplied", async () => { assert.equal(result, true, "null expected updatedAt skips the guard"); assert.equal(opts._calls.updatePersona, 1); }); + +// ── Test family 9: success toast names the observed (persisted) agent (P2) ─── +// +// `latestAgent` only advances on a NON-throwing updateManagedAgent. A rename +// that commits to disk but whose command throws afterward leaves `latestAgent` +// at the pre-save instance, so the success toast must read the name from the +// observed refetch — not from `latestAgent` — or a committed Alice→Bob rename +// falsely reports "Alice saved." + +test("test_success_toast_uses_observed_name_on_thrown_but_persisted_rename", async () => { + const cap = captureToasts(); + try { + const opts = makeOpts({ + ctx: { + kind: "instance-only", + instance: makeInstance({ name: "Alice" }), + }, + agentInput: makeAgentInput({ name: "Bob" }), + // The rename commits to disk, but the command throws after commit, so + // `latestAgent` is never reassigned and stays at the pre-save "Alice". + updateManagedAgent: async () => { + throw new Error("summary build failed after commit"); + }, + // The final refetch observes the persisted rename. + refetchStores: async () => ({ + persona: null, + agent: makeInstance({ name: "Bob" }), + }), + }); + + const result = await runAgentSaveCoordinator(opts); + + assert.equal( + result, + true, + "a thrown-but-persisted write is a full success (observed state matches)", + ); + const successes = cap.captured.filter((c) => c.kind === "success"); + assert.equal(successes.length, 1, "exactly one success toast"); + assert.match( + successes[0].message, + /^Bob saved\./, + "toast must name the observed (persisted) rename, not the stale pre-save name", + ); + assert.doesNotMatch( + successes[0].message, + /Alice/, + "toast must not report the stale pre-save name", + ); + } finally { + cap.restore(); + } +}); diff --git a/desktop/src/features/agents/ui/agentSaveCoordinator.ts b/desktop/src/features/agents/ui/agentSaveCoordinator.ts index f117ce2be3..d6f549ffa3 100644 --- a/desktop/src/features/agents/ui/agentSaveCoordinator.ts +++ b/desktop/src/features/agents/ui/agentSaveCoordinator.ts @@ -359,8 +359,14 @@ export async function runAgentSaveCoordinator( if (!observedRemainder) { // Full success — every submitted write is reflected in observed state. + // Prefer the OBSERVED name from the final refetch over `latestAgent`. + // `latestAgent` only advances on a non-throwing `updateManagedAgent`; a + // rename that persisted but whose command threw after commit leaves it at + // the pre-save `inst`, so a committed Alice→Bob would report "Alice saved." + // The observed refetch reflects what is actually on disk (the stopped-state + // path below already prefers `observedAgent` for the same reason). const agentName = - latestAgent?.name ?? observedAgent?.name ?? def?.displayName ?? "Agent"; + observedAgent?.name ?? latestAgent?.name ?? def?.displayName ?? "Agent"; if (profileSyncError) { showAgentProfileSyncWarning(agentName, profileSyncError); diff --git a/desktop/src/features/agents/ui/useAgentEditRuntimeHandlers.test.mjs b/desktop/src/features/agents/ui/useAgentEditRuntimeHandlers.test.mjs new file mode 100644 index 0000000000..0889698dc3 --- /dev/null +++ b/desktop/src/features/agents/ui/useAgentEditRuntimeHandlers.test.mjs @@ -0,0 +1,179 @@ +import assert from "node:assert/strict"; +import { after, before, test } from "node:test"; + +import { JSDOM } from "jsdom"; + +// useAgentEditRuntimeHandlers is a React hook (it calls +// usePendingHarnessSelection internally), so it must be mounted to exercise +// the real handler wiring. JSDOM provides the DOM globals renderHook needs. +const dom = new JSDOM("", { + url: "http://localhost", +}); + +before(() => { + Object.assign(globalThis, { + document: dom.window.document, + HTMLElement: dom.window.HTMLElement, + IS_REACT_ACT_ENVIRONMENT: true, + window: dom.window, + localStorage: dom.window.localStorage, + }); +}); + +after(() => dom.window.close()); + +let hookModule; +before(async () => { + hookModule = await import("./useAgentEditRuntimeHandlers.ts"); +}); + +// Build a RuntimeHandlersInput whose every setter records its calls, so a test +// can assert which layer (definition vs instance) a handler wrote to. +function makeInput(overrides = {}) { + const calls = { + setDefinitionRuntimeId: [], + setSelectedRuntimeId: [], + setInheritHarness: [], + setAgentCommand: [], + setAgentArgs: [], + setDProvider: [], + setDModel: [], + setIProvider: [], + setIModel: [], + runtimeTouchedWrites: 0, + }; + const noop = () => {}; + const runtimeTouched = { + get current() { + return this._v ?? false; + }, + set current(v) { + this._v = v; + calls.runtimeTouchedWrites++; + }, + }; + const input = { + showDef: true, + showInst: true, + dProvider: "openai", + dModel: "gpt-4o", + dIsCustomProviderEditing: false, + dIsCustomModelEditing: false, + setDProvider: (v) => calls.setDProvider.push(v), + setDModel: (v) => calls.setDModel.push(v), + setDIsCustomProviderEditing: noop, + setDIsCustomModelEditing: noop, + envVars: {}, + setEnvVars: noop, + definitionRuntimeId: "goose", + setDefinitionRuntimeId: (v) => calls.setDefinitionRuntimeId.push(v), + iProvider: "openai", + iModel: "gpt-4o", + iIsCustomProviderEditing: false, + iIsCustomModelEditing: false, + setIProvider: (v) => calls.setIProvider.push(v), + setIModel: (v) => calls.setIModel.push(v), + setIIsCustomProviderEditing: noop, + setIIsCustomModelEditing: noop, + instanceEnvVars: {}, + setInstanceEnvVars: noop, + selectedRuntimeId: "goose", + setSelectedRuntimeId: (v) => calls.setSelectedRuntimeId.push(v), + setInheritHarness: (v) => calls.setInheritHarness.push(v), + setAgentCommand: (v) => calls.setAgentCommand.push(v), + setAgentArgs: (v) => calls.setAgentArgs.push(v), + runtimeTouched, + setIsAddHarnessOpen: noop, + runtimes: [{ id: "buzz-agent", command: "", defaultArgs: [] }], + selectedRuntime: { id: "goose", command: "", defaultArgs: [] }, + open: true, + ...overrides, + }; + return { input, calls }; +} + +async function renderHandlers(input) { + const { renderHook } = await import("@testing-library/react"); + return renderHook(() => hookModule.useAgentEditRuntimeHandlers(input)); +} + +// ── P1: Relay Mesh selection must switch the runtime on the SHOWN layer ─────── +// +// Carl's finding: `handleProviderDropdownChange` forced buzz-agent through the +// INSTANCE handler unconditionally. In a linked edit (showDef + showInst) that +// pinned the clicked instance to buzz-agent while the definition kept its old +// runtime — a D/I ownership-boundary violation. Selecting Relay Mesh while a +// definition is shown must change the DEFINITION runtime and leave the instance +// pin untouched. Reverting the fix (routing back through the instance handler, +// or consulting the instance runtime id) makes both assertions below fail. + +test("test_relay_mesh_in_linked_edit_switches_definition_runtime_not_instance", async () => { + const { act } = await import("@testing-library/react"); + const { input, calls } = makeInput(); + const { result } = await renderHandlers(input); + + act(() => result.current.handleProviderDropdownChange("relay-mesh")); + + assert.deepEqual( + calls.setDefinitionRuntimeId, + ["buzz-agent"], + "definition runtime must switch to buzz-agent when Relay Mesh is chosen with the definition shown", + ); + assert.deepEqual( + calls.setSelectedRuntimeId, + [], + "the instance runtime pin must NOT be touched in a linked edit", + ); + assert.equal( + calls.runtimeTouchedWrites, + 0, + "the instance runtimeTouched ref must NOT be set in a linked edit", + ); + assert.deepEqual( + calls.setInheritHarness, + [], + "instance harness inheritance must NOT be cleared in a linked edit", + ); + // The definition provider/model still land on the relay-mesh selection. + assert.equal( + calls.setDProvider.at(-1), + "relay-mesh", + "definition provider must become relay-mesh", + ); + assert.equal( + calls.setDModel.at(-1), + "auto", + "definition model must become auto for relay-mesh", + ); +}); + +test("test_relay_mesh_in_instance_only_edit_switches_instance_runtime", async () => { + const { act } = await import("@testing-library/react"); + // Instance-only context: no definition shown, so the instance handler is the + // correct layer and buzz-agent must be pinned on the instance. + const { input, calls } = makeInput({ + showDef: false, + showInst: true, + selectedRuntimeId: "goose", + definitionRuntimeId: "goose", + }); + const { result } = await renderHandlers(input); + + act(() => result.current.handleProviderDropdownChange("relay-mesh")); + + assert.deepEqual( + calls.setSelectedRuntimeId, + ["buzz-agent"], + "instance runtime must switch to buzz-agent in an instance-only edit", + ); + assert.deepEqual( + calls.setDefinitionRuntimeId, + [], + "definition runtime must NOT be touched in an instance-only edit", + ); + assert.equal( + calls.setIProvider.at(-1), + "relay-mesh", + "instance provider must become relay-mesh", + ); +}); diff --git a/desktop/src/features/agents/ui/useAgentEditRuntimeHandlers.ts b/desktop/src/features/agents/ui/useAgentEditRuntimeHandlers.ts index f2c8d51948..07908883ae 100644 --- a/desktop/src/features/agents/ui/useAgentEditRuntimeHandlers.ts +++ b/desktop/src/features/agents/ui/useAgentEditRuntimeHandlers.ts @@ -209,8 +209,19 @@ export function useAgentEditRuntimeHandlers(input: RuntimeHandlersInput) { function handleProviderDropdownChange(nextValue: string) { const nextProvider = nextValue === AUTO_PROVIDER_DROPDOWN_VALUE ? "" : nextValue; - if (nextProvider === "relay-mesh" && selectedRuntimeId !== "buzz-agent") { - handleRuntimeDropdownChange("buzz-agent"); + // Relay Mesh is only offered by the buzz-agent runtime, so selecting it + // forces that runtime. Route through the same layer the provider dropdown + // writes: in a definition-showing context switch the DEFINITION runtime + // (handleDefinitionRuntimeChange), never the instance handler — otherwise a + // linked edit silently pins the clicked instance to buzz-agent while the + // definition keeps its old runtime. Consult that same layer's runtime id. + const currentRuntimeId = showDef ? definitionRuntimeId : selectedRuntimeId; + if (nextProvider === "relay-mesh" && currentRuntimeId !== "buzz-agent") { + if (showDef) { + handleDefinitionRuntimeChange("buzz-agent"); + } else { + handleRuntimeDropdownChange("buzz-agent"); + } } const sel = showDef ? dSelection : iSelection; const applyFn = showDef ? applyDSelection : applyISelection;