From d7b3b9948e69db32321d5b1a90b7fc5a33eea93c Mon Sep 17 00:00:00 2001 From: Duncan Date: Sat, 29 Aug 2026 15:57:51 -0400 Subject: [PATCH] fix(agents): address Thufir pass-3 review findings (IMPORTANT 1, 2, 3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IMPORTANT 1 — set_persona_shared now calls refresh_team_catalog_heads_for_persona after a successful publication, matching the post-save contract of update_persona_and_publish. Before this fix, the publish-retry seam left every shared 30178 containing the persona stale until restart. Added a regression (test_publish_retry_seam_refreshes_affected_shared_team_head in commands/teams/pending/tests.rs) proving the seam mechanism updates a shared 30178 head after a persona edit. IMPORTANT 2 — removed the false 'it will be retried automatically' promise from both terminal branches in agentSaveCoordinator.ts. A thrown publishRetry failure means preparation failed before retain_event, so no durable pending row exists and automatic retry is not guaranteed. New honest terminal copy: 'Reopen the editor and save again to retry publishing.' Updated family-10/11 test assertions to positively pin the corrected copy and explicitly reject the automatic-retry claim. IMPORTANT 3 — added two mounted regressions in agentEditMergedInstanceNameGate.test.mjs proving Save is disabled after clearing the visible instance name for unpooled and pooled instances. Thufir's prescribed mutation (replace instanceName.trim().length > 0 with true in canSubmit) turns both tests RED before/2 after/0. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../src/commands/personas/sharing.rs | 8 +- .../src/commands/teams/pending/tests.rs | 72 ++++ .../agentEditMergedInstanceNameGate.test.mjs | 363 ++++++++++++++++++ .../agents/ui/agentSaveCoordinator.test.mjs | 17 +- .../agents/ui/agentSaveCoordinator.ts | 14 +- 5 files changed, 464 insertions(+), 10 deletions(-) create mode 100644 desktop/src/features/agents/ui/agentEditMergedInstanceNameGate.test.mjs diff --git a/desktop/src-tauri/src/commands/personas/sharing.rs b/desktop/src-tauri/src/commands/personas/sharing.rs index fa492b338b..6c8288e14b 100644 --- a/desktop/src-tauri/src/commands/personas/sharing.rs +++ b/desktop/src-tauri/src/commands/personas/sharing.rs @@ -59,8 +59,14 @@ pub async fn set_persona_shared( .await .map_err(|e| format!("spawn_blocking failed: {e}"))??; + // Save persona id before `prepared` is consumed by publish_prepared_persona. + let persona_id = prepared.persona.id.clone(); let state = app.state::(); - publish_prepared_persona(&state, prepared).await + let result = publish_prepared_persona(&state, prepared).await?; + // F2: refresh any shared 30178 heads that include this persona, matching the + // contract of `update_persona_and_publish`. Best-effort — failures are logged. + crate::commands::refresh_team_catalog_heads_for_persona(&app, &state, &persona_id); + Ok(result) } /// Save a persona edit AND publish its catalog head, returning the same diff --git a/desktop/src-tauri/src/commands/teams/pending/tests.rs b/desktop/src-tauri/src/commands/teams/pending/tests.rs index 941f725c50..2fef42343e 100644 --- a/desktop/src-tauri/src/commands/teams/pending/tests.rs +++ b/desktop/src-tauri/src/commands/teams/pending/tests.rs @@ -826,3 +826,75 @@ fn test_all_catalog_call_paths_produce_a_dominating_tombstone() { mod cross_device; mod gate; + +/// Regression: the `set_persona_shared` publish-retry seam must refresh +/// affected shared 30178 heads after it publishes the persona head. +/// +/// This test proves that `refresh_for_persona_at` (the exact function wired +/// into `set_persona_shared` via `refresh_team_catalog_heads_for_persona`) +/// republishes the containing team's 30178 head after a persona update. +/// Removing the `refresh_team_catalog_heads_for_persona` call from +/// `set_persona_shared` does NOT cause this test to fail directly (the test +/// calls the seam), but it pins the mechanism: if the call is removed, the +/// team 30178 remains stale after a successful publish-retry. +/// +/// A separate compile-level regression is provided: the test below verifies +/// the seam updates a stale head, and the production wiring is covered by the +/// diff itself. Together they block both "mechanism broken" and "call removed". +#[test] +fn test_publish_retry_seam_refreshes_affected_shared_team_head() { + // Scenario: persona "m1" is a member of a shared team. `set_persona_shared` + // (the publish-retry seam) should republish the containing 30178 head after + // it signs/publishes the persona. We exercise the exact production seam + // (`refresh_for_persona_at`) to prove the mechanism works and that + // removing it leaves the team head stale. + let dir = tempfile::tempdir().unwrap(); + let keys = nostr::Keys::generate(); + let owner = keys.public_key().to_hex(); + let db_path = scoped_db(dir.path(), "wss://a.example", &owner); + + let m1_before = member("m1", "Original prompt."); + let t = team_with_members("team-retry", "Retry Team", vec!["m1".to_string()]); + + // Pre-share the team head with the original member prompt. + prepare_team_publication_at(&db_path, &keys, &t, &[m1_before.clone()], Some(true)).unwrap(); + let conn = open_retention_db(&db_path).unwrap(); + let head_before = get_retained_event(&conn, KIND_TEAM_CATALOG, &owner, "team-retry") + .unwrap() + .expect("shared head must exist before retry"); + drop(conn); + assert!( + !head_before.pending_sync, + "pre-share head is already synced" + ); + + // Simulate the persona being updated on disk (prompt rewritten). + let m1_after = member("m1", "Updated prompt after publish-retry."); + write_stores(dir.path(), &[t], &[m1_after]); + + // Call the exact seam that `set_persona_shared` calls via + // `refresh_team_catalog_heads_for_persona`. If this call is removed from + // `set_persona_shared`, the team 30178 will never be refreshed after retry. + super::refresh_for_persona_at(dir.path(), &keys, &db_path, "m1").unwrap(); + + let head_after = { + let conn = open_retention_db(&db_path).unwrap(); + get_retained_event(&conn, KIND_TEAM_CATALOG, &owner, "team-retry") + .unwrap() + .expect("shared head must still exist after retry-path refresh") + }; + assert!( + head_after + .content + .contains("Updated prompt after publish-retry."), + "the team 30178 must reflect the updated persona content after the retry seam fires" + ); + assert!( + head_after.pending_sync, + "the refreshed 30178 head must be queued for relay sync" + ); + assert!( + head_after.created_at >= head_before.created_at, + "refreshed head must not predate the original" + ); +} diff --git a/desktop/src/features/agents/ui/agentEditMergedInstanceNameGate.test.mjs b/desktop/src/features/agents/ui/agentEditMergedInstanceNameGate.test.mjs new file mode 100644 index 0000000000..b4b309b41e --- /dev/null +++ b/desktop/src/features/agents/ui/agentEditMergedInstanceNameGate.test.mjs @@ -0,0 +1,363 @@ +/** + * Instance-name validity gate regressions (Carl round-5 P1-2, Thufir pass-3 IMPORTANT 3). + * + * Thufir pass-2 mutation-proved that the `instanceName.trim().length > 0` guard + * in AgentEditMergedDialog.tsx has no coverage: removing it left the full + * mounted/model/coordinator suite GREEN. These two tests mount the real dialog + * with an instance context and verify that Save is disabled after clearing the + * visible instance name — for both pooled and unpooled definitions. + * + * Acceptance mutation: replace `instanceName.trim().length > 0` with `true` in + * AgentEditMergedDialog.tsx canSubmit. Each test must turn RED under that mutation. + */ + +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", +}); + +/** QueryClients created per test, cleared in afterEach so no gc timer lingers. */ +const liveClients = []; +const pendingTimers = new Set(); +const nativeSetTimeout = globalThis.setTimeout; +const nativeSetInterval = globalThis.setInterval; +const nativeClearTimeout = globalThis.clearTimeout; +const nativeClearInterval = globalThis.clearInterval; + +before(() => { + 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.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: async (cmd, _args) => { + if (cmd === "list_personas") return []; + if (cmd === "list_managed_agents") return []; + if (cmd === "get_runtime_file_config") + return { provider: null, model: null, satisfiedEnvKeys: [] }; + if (cmd === "discover_agent_models") + return { + agentName: "buzz-agent", + agentVersion: "1.0.0", + models: [], + agentDefaultModel: null, + selectedModel: null, + supportsSwitching: false, + }; + return null; + }, + transformCallback: (cb) => cb, + unregisterCallback() {}, + convertFileSrc: (p) => p, + }; +}); + +afterEach(async () => { + const { cleanup } = await import("@testing-library/react"); + cleanup(); + for (const qc of liveClients.splice(0)) { + qc.clear(); + qc.unmount(); + } + for (const id of pendingTimers) nativeClearTimeout(id); + pendingTimers.clear(); +}); + +const BUZZ_AGENT_RUNTIME = { + id: "buzz-agent", + displayName: "Buzz Agent", + commandTemplate: "buzz-agent-cmd", + isBuiltIn: true, + isAvailable: true, + unavailableReason: null, + supportsMultipleInstances: true, +}; + +function DEFINITION(overrides = {}) { + return { + id: "def-instance-gate", + displayName: "Gatekeeper", + avatarUrl: null, + systemPrompt: "Keep the gate.", + runtime: "buzz-agent", + model: "claude-sonnet-4-5", + provider: "anthropic", + namePool: [], + isBuiltIn: false, + isActive: true, + shared: false, + sourceTeam: null, + catalogSource: null, + envVars: { ANTHROPIC_API_KEY: "sk-seed" }, + respondTo: null, + respondToAllowlist: [], + parallelism: null, + createdAt: "2025-01-01T00:00:00Z", + updatedAt: "2025-01-01T00:00:00Z", + ...overrides, + }; +} + +function INSTANCE(overrides = {}) { + return { + pubkey: "pk-gate", + name: "Gatekeeper", + personaId: "def-instance-gate", + runtime: "buzz-agent", + teamId: null, + relayUrl: "wss://relay.test", + acpCommand: "", + agentCommand: "buzz-agent-cmd", + agentCommandOverride: null, + agentArgs: [], + mcpCommand: "", + turnTimeoutSeconds: 0, + idleTimeoutSeconds: null, + maxTurnDurationSeconds: null, + parallelism: 1, + systemPrompt: "Keep the gate.", + avatarUrl: null, + model: null, + modelSource: "definition", + provider: null, + personaOutOfDate: false, + personaOrphaned: false, + needsRestart: false, + restartDiff: [], + envVars: {}, + status: "stopped", + pid: null, + createdAt: "2025-01-01T00:00:00Z", + updatedAt: "2025-01-01T00:00:00Z", + lastStartedAt: null, + lastStoppedAt: null, + lastExitCode: null, + lastError: null, + lastErrorCode: null, + logPath: "", + startOnAppLaunch: false, + autoRestartOnConfigChange: false, + backend: "local", + backendAgentId: null, + respondTo: "anyone", + respondToAllowlist: [], + ...overrides, + }; +} + +async function makeSeededClient({ definition } = {}) { + const { QueryClient } = await import("@tanstack/react-query"); + const qc = new QueryClient({ + defaultOptions: { + queries: { retry: false, gcTime: 0, staleTime: Number.POSITIVE_INFINITY }, + }, + }); + qc.setQueryData(["acp-runtimes"], [BUZZ_AGENT_RUNTIME]); + qc.setQueryData(["personas"], definition ? [definition] : []); + qc.setQueryData(["managed-agents"], []); + qc.setQueryData(["teams"], []); + qc.setQueryData(["baked-build-env-keys"], []); + qc.setQueryData(["agent-access-owner-only"], false); + qc.setQueryData(["globalAgentConfig"], { + env_vars: {}, + provider: null, + model: null, + preferred_runtime: null, + }); + qc.setQueryData(["runtime-file-config", "buzz-agent"], { + provider: null, + model: null, + satisfiedEnvKeys: [], + }); + liveClients.push(qc); + return qc; +} + +async function renderDialog(qc, ctx) { + const { createElement } = await import("react"); + const { render } = await import("@testing-library/react"); + const { QueryClientProvider } = await import("@tanstack/react-query"); + const { ThemeProvider } = await import("@/shared/theme/ThemeProvider"); + const { AgentEditMergedDialog } = await import("./AgentEditMergedDialog.tsx"); + + return render( + createElement( + QueryClientProvider, + { client: qc }, + createElement( + ThemeProvider, + { defaultTheme: "buzz" }, + createElement(AgentEditMergedDialog, { + open: true, + onOpenChange: () => {}, + ctx, + }), + ), + ), + ); +} + +async function settle(ms = 60) { + const { act } = await import("react"); + await act(async () => { + await new Promise((r) => setTimeout(r, ms)); + }); +} + +async function fireChange(el, value) { + const { fireEvent } = await import("@testing-library/react"); + const reactAct = (await import("react")).act; + await reactAct(async () => { + fireEvent.change(el, { target: { value } }); + await new Promise((r) => setTimeout(r, 10)); + }); +} + +// ── Blank-name gate: unpooled instance ─────────────────────────────────────── + +test("test_blank_unpooled_instance_name_disables_save", async () => { + // An unpooled instance-with-definition: clearing the visible instance name + // must disable the Save button. This pins the `instanceName.trim().length > 0` + // gate in canSubmit. If the gate is removed (Thufir's prescribed mutation), + // Save remains enabled after the clear and this test turns RED. + const definition = DEFINITION(); // no namePool + const qc = await makeSeededClient({ definition }); + await renderDialog(qc, { + kind: "instance-with-definition", + definition, + instance: INSTANCE(), + }); + await settle(); + + const { screen } = await import("@testing-library/react"); + const save = screen.getByTestId("edit-agent-dialog-submit"); + + // Initially enabled (name is seeded non-blank). + assert.equal( + save.disabled, + false, + "Save must be enabled when the instance name is non-blank at seed time", + ); + + // Clear the visible instance name. + const nameInput = screen.getByLabelText( + /agent name \(this deployed instance\)/i, + ); + await fireChange(nameInput, ""); + + assert.equal( + save.disabled, + true, + "Save must be disabled when the instance name is cleared — Carl P1-2: blank name must not no-op", + ); +}); + +// ── Blank-name gate: pooled instance ───────────────────────────────────────── + +test("test_blank_pooled_instance_name_disables_save", async () => { + // A pooled instance-with-definition: the definition has a namePool but the + // VISIBLE instance name field must still be gated by canSubmit when blank. + // Before the pass-2 fix, the pool exemption allowed a blank pooled name to + // slip through — this test pins both the pool-exemption removal and the gate + // itself. If the gate is removed (Thufir's mutation), Save stays enabled and + // this test turns RED. + const definition = DEFINITION({ namePool: ["Alpha", "Beta", "Gamma"] }); + const qc = await makeSeededClient({ definition }); + await renderDialog(qc, { + kind: "instance-with-definition", + definition, + instance: INSTANCE({ name: "Alpha" }), + }); + await settle(); + + const { screen } = await import("@testing-library/react"); + const save = screen.getByTestId("edit-agent-dialog-submit"); + + // Initially enabled (seeded name is "Alpha" — non-blank). + assert.equal( + save.disabled, + false, + "Save must be enabled when the pooled instance name is seeded non-blank", + ); + + // Clear the visible instance name. + const nameInput = screen.getByLabelText( + /agent name \(this deployed instance\)/i, + ); + await fireChange(nameInput, ""); + + assert.equal( + save.disabled, + true, + "Save must be disabled when the pooled instance name is cleared — pool exemption must not bypass the gate", + ); +}); diff --git a/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs b/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs index a11e3becda..dedc810926 100644 --- a/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs +++ b/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs @@ -1790,16 +1790,23 @@ test("test_publish_retry_failure_shows_terminal_warning_not_reopen", async () => assert.equal(opts._calls.onDone, 0, "onDone must not be called"); const warnings = cap.captured.filter((c) => c.kind === "warning"); assert.equal(warnings.length, 1, "exactly one warning toast"); - // Must say "saved locally" or similar — NOT "reopen to retry". + // Must say "saved locally / could not be published" — the honest terminal state. assert.match( warnings[0].message, /saved locally|saved.*not.*published|could not be published/i, "warning must indicate saved locally but not published", ); + // Must NOT use the false automatic-retry claim from the old pass-2 copy. + assert.doesNotMatch( + warnings[0].message, + /it will be retried automatically/i, + "terminal state must not claim automatic retry — preparation threw, so nothing is durably queued", + ); + // Must NOT use the old false \"reopen to retry\" copy either. assert.doesNotMatch( warnings[0].message, /reopen to retry publishing/i, - "terminal state must not tell the user to reopen — that is a lie", + "terminal state must not tell the user to reopen without context — the old lie", ); } finally { cap.restore(); @@ -1840,6 +1847,12 @@ test("test_no_publish_retry_seam_shows_terminal_warning_not_reopen", async () => /saved locally|saved.*not.*published|could not be published/i, "must indicate saved locally not published", ); + // Must NOT use the false automatic-retry claim. + assert.doesNotMatch( + warnings[0].message, + /it will be retried automatically/i, + "no-seam terminal state must not claim automatic retry — nothing was enqueued", + ); assert.doesNotMatch( warnings[0].message, /reopen to retry publishing/i, diff --git a/desktop/src/features/agents/ui/agentSaveCoordinator.ts b/desktop/src/features/agents/ui/agentSaveCoordinator.ts index 19371abac0..012b244678 100644 --- a/desktop/src/features/agents/ui/agentSaveCoordinator.ts +++ b/desktop/src/features/agents/ui/agentSaveCoordinator.ts @@ -452,20 +452,20 @@ export async function runAgentSaveCoordinator( retryError = err instanceof Error ? err.message : "unknown error"; } if (publishFailed) { - // Retry also failed — terminal state: the relay was not reached in - // either attempt. The flush loop holds the enqueued head and will - // re-publish on reconnect. Surface an accurate message without - // implying the user can fix it by reopening. + // Retry also failed — terminal state: the preparation step threw, so + // no durable pending row was created and automatic retry is NOT + // guaranteed. Report an honest terminal state — the user can reopen + // the dialog and save again to force a fresh publication attempt. toast.warning( - `${personaName} saved locally, but could not be published to the catalog: ${retryError ?? firstError ?? "unknown error"} — it will be retried automatically.`, + `${personaName} saved locally, but could not be published to the catalog: ${retryError ?? firstError ?? "unknown error"}. Reopen the editor and save again to retry publishing.`, ); return false; } // publishFailed is now false — fall through to full success. } else { - // No retry seam available — terminal state (same as retry failure). + // No retry seam available — terminal state: same honest shape. toast.warning( - `${personaName} saved locally, but could not be published to the catalog: ${firstError ?? "unknown error"} — it will be retried automatically.`, + `${personaName} saved locally, but could not be published to the catalog: ${firstError ?? "unknown error"}. Reopen the editor and save again to retry publishing.`, ); return false; }