diff --git a/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx b/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx index cbe193e5ea..b14c833a95 100644 --- a/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx +++ b/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx @@ -567,9 +567,11 @@ export function AgentEditMergedDialog({ (selectedRuntimeId !== "custom" || inheritHarness || agentCommand.trim().length > 0) && - // Block submission when the instance name field controls the name and - // the user has left it blank (no name pool — pool-named instances skip). - ((def?.namePool?.length ?? 0) > 0 || instanceName.trim().length > 0) + // Block submission when the instance name field is blank — applies to + // all instances including pool-named ones. Pooled instances can be + // explicitly renamed; the blank guard prevents submitting an empty name + // regardless of pool membership. + instanceName.trim().length > 0 : true); // ── Runtime/model/provider handlers (extracted to useAgentEditRuntimeHandlers) ── diff --git a/desktop/src/features/agents/ui/agentFormModel.test.mjs b/desktop/src/features/agents/ui/agentFormModel.test.mjs index d2d03f1fab..95d58212a1 100644 --- a/desktop/src/features/agents/ui/agentFormModel.test.mjs +++ b/desktop/src/features/agents/ui/agentFormModel.test.mjs @@ -117,6 +117,112 @@ test("test_pooled_name_instance_name_not_materialized_even_when_names_differ", ( ); }); +// ── Regression test 1b: pooled instances — explicit name change emits (Carl round-5, IMPORTANT 2) ── +// +// The hasNamePool guard previously suppressed ALL instance-name diffs when the +// definition had a name pool. This made it impossible for a user to rename their +// pooled instance: the change would be silently swallowed. The guard is NOT +// load-bearing for the definition-rename safeguard — when only a D-field +// changes, instanceName stays equal to inst.name in the seed, so the diff is +// already false without it. Removing the guard allows explicit pooled-instance +// renames to reach the backend. + +test("test_pooled_explicit_rename_emits_agentInput", () => { + // A pool-named instance: user explicitly changes the visible instance name. + const definition = makeDefinition({ + displayName: "Bot", + namePool: ["Scout", "Ranger"], + }); + const instance = makeInstance({ name: "Scout" }); + + const ctx = { kind: "instance-with-definition", definition, instance }; + const saved = seedAgentFormModel(ctx); + + // User renames the pooled instance — sets instanceName to a new value. + const next = { ...saved, instanceName: "Scout-alpha" }; + const emit = emitAgentFormDiff(saved, next, ctx); + + // D-field unchanged — no definition update. + assert.equal( + emit.personaInput, + null, + "pooled rename must not update the definition", + ); + // I-field: explicit rename must emit agentInput with the new name. + assert.ok( + emit.agentInput, + "explicit pooled instance rename must emit agentInput", + ); + assert.equal( + emit.agentInput.name, + "Scout-alpha", + "agentInput.name must equal the new instance name", + ); +}); + +test("test_pooled_clear_emits_agentInput", () => { + // Clearing the pooled instance name results in nameChanged=true (the canSubmit + // gate blocks blank names, but the model emits the diff regardless — the gate + // prevents submission before we reach emit, but the model must still be + // correct about what changed if it were somehow reached). + const definition = makeDefinition({ + displayName: "Bot", + namePool: ["Scout", "Ranger"], + }); + const instance = makeInstance({ name: "Scout" }); + + const ctx = { kind: "instance-with-definition", definition, instance }; + const saved = seedAgentFormModel(ctx); + + // User clears the pooled instance name (empty string). + const next = { ...saved, instanceName: "" }; + const emit = emitAgentFormDiff(saved, next, ctx); + + // The clear emits as a name change (empty string != "Scout"). + assert.ok( + emit.agentInput, + "a cleared pooled instance name must emit agentInput (canSubmit gate blocks it before submission)", + ); + assert.equal( + emit.agentInput.name, + "", + "emitted name must be the cleared empty string", + ); +}); + +test("test_cleared_instance_name_emits_empty_not_displayName_fallback", () => { + // When instanceName is cleared (undefined in the model, from buildNextAgentFormModel + // returning undefined for a trimmed empty string), the emit must use "" NOT + // fall back to displayName.trim(). The old fallback (next.instanceName ?? + // next.displayName.trim()) silently renamed the instance to the definition name + // instead of treating the clear as an intended empty. + const definition = makeDefinition({ + displayName: "Alice", + namePool: [], + }); + const instance = makeInstance({ name: "Alice" }); + + const ctx = { kind: "instance-with-definition", definition, instance }; + const saved = seedAgentFormModel(ctx); + + // Simulate buildNextAgentFormModel with an empty instanceName field: + // " ".trim() || undefined = undefined. + const next = { ...saved, instanceName: undefined }; + const emit = emitAgentFormDiff(saved, next, ctx); + + // A cleared instanceName (undefined) should emit agentInput.name = "" (the + // intended "clear"), not fall back to displayName.trim() = "Alice". + assert.ok( + emit.agentInput, + "undefined instanceName != inst.name must emit agentInput", + ); + assert.equal( + emit.agentInput.name, + "", + "cleared instanceName must emit empty string, not the definition displayName", + ); +}); + test("test_no_name_pool_explicit_instance_name_change_emits_agentInput", () => { // Without a name pool, explicitly editing the instanceName field should // produce an agentInput with the new name. diff --git a/desktop/src/features/agents/ui/agentFormModel.ts b/desktop/src/features/agents/ui/agentFormModel.ts index b3421f8f71..0a80a031de 100644 --- a/desktop/src/features/agents/ui/agentFormModel.ts +++ b/desktop/src/features/agents/ui/agentFormModel.ts @@ -540,12 +540,13 @@ export function emitAgentFormDiff( const nameChanged = isI("instanceName") && - !hasNamePool && - // Use the raw instanceName value — do not fall back to displayName when - // instanceName is empty string. An empty instanceName means the user - // deliberately cleared the field; the canSubmit gate blocks submission - // when it is blank, so if we reach here with "" the instance is - // definition-only and has no name change to emit. + // Pool-named instances: the hasNamePool guard previously suppressed every + // instanceName diff, but it was not load-bearing for the definition-rename + // case: when only a D-field changes the seeded instanceName still equals + // inst.name, so the diff is already false without the guard. Removing the + // guard lets an explicit user rename/clear on a pooled instance emit + // correctly. The canSubmit gate blocks blank names (pool or not), so an + // accidental clear cannot reach this path with an empty string. (next.instanceName ?? "") !== inst.name; const systemPromptChanged = isI("systemPrompt") && diff --git a/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs b/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs index 9506030ad2..a11e3becda 100644 --- a/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs +++ b/desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs @@ -1631,8 +1631,8 @@ test("test_publish_throws_post_persist_does_not_settle_as_full_success", async ( assert.equal(warnings.length, 1, "exactly one warning toast"); assert.match( warnings[0].message, - /saved.*publishing.*failed|publishing.*failed|not published/i, - "warning must indicate the persona saved but publication failed", + /saved locally.*not.*published|could not be published|saved.*not published/i, + "warning must indicate the persona saved locally but publication failed", ); assert.match( warnings[0].message, @@ -1680,7 +1680,7 @@ test("test_publish_throws_post_persist_returns_false_not_partial_failure_toast", ); assert.match( warnings[0].message, - /publishing.*failed|catalog.*failed|not published/i, + /could not be published|saved locally.*not.*published|not published/i, "warning must indicate publishing was the failure, not the save itself", ); // Must NOT use the partial-failure wording "profile saved. profile failed" @@ -1693,3 +1693,159 @@ test("test_publish_throws_post_persist_returns_false_not_partial_failure_toast", cap.restore(); } }); + +// ── Test family 11: P1-1 publish retry seam (Carl round-5 pass-2) ───────────── +// +// When the initial save+publish command throws after the persona is on disk, the +// coordinator now attempts a publish-only retry via publishRetry(def.id) before +// reporting failure. This keeps the toast copy honest: +// +// - If the retry succeeds: full-success path fires — onDone is called and the +// success toast names the persona and publication status. +// - If the retry also fails: terminal warning without "reopen to retry" +// (the flush loop holds the enqueued head and will retry automatically). +// - If no publishRetry seam is provided: same terminal warning, no false +// "reopen to retry" copy. +// +// The existing family-10 tests remain unchanged: they verify the publishFailed +// flag is set and onDone is not called on the initial throw. These family-11 +// tests cover the retry layer on top. + +test("test_publish_retry_succeeds_settles_as_full_success", async () => { + // Initial updatePersonaAndPublish throws (retain step failed), but the + // persona IS observed as persisted. publishRetry succeeds → coordinator + // should call onDone and return true. + const cap = captureToasts(); + try { + const persisted = makeDefinition({ displayName: "Alice" }); + let retryCalls = 0; + const opts = makeOpts({ + ctx: { + kind: "definition-only", + definition: makeDefinition({ displayName: "Alice" }), + }, + personaInput: makePersonaInput({ displayName: "Alice" }), + publishCatalogUpdates: true, + updatePersonaAndPublish: async () => { + throw new Error("failed to open retention db"); + }, + refetchStores: async () => ({ persona: persisted, agent: null }), + publishRetry: async () => { + retryCalls++; + return { persona: persisted, publicationStatus: "published" }; + }, + }); + + const result = await runAgentSaveCoordinator(opts); + + assert.equal( + result, + true, + "coordinator must return true when the publish retry succeeds", + ); + assert.equal( + opts._calls.onDone, + 1, + "onDone must be called on retry success", + ); + assert.equal(retryCalls, 1, "publishRetry must be called exactly once"); + const warnings = cap.captured.filter((c) => c.kind === "warning"); + assert.equal(warnings.length, 0, "no warning toast when retry succeeds"); + const successes = cap.captured.filter((c) => c.kind === "success"); + assert.equal(successes.length, 1, "success toast must fire after retry"); + } finally { + cap.restore(); + } +}); + +test("test_publish_retry_failure_shows_terminal_warning_not_reopen", async () => { + // Both the initial publish and the retry fail — should report a terminal + // state without "reopen to retry publishing". + const cap = captureToasts(); + try { + const persisted = makeDefinition({ displayName: "Alice" }); + const opts = makeOpts({ + ctx: { + kind: "definition-only", + definition: makeDefinition({ displayName: "Alice" }), + }, + personaInput: makePersonaInput({ displayName: "Alice" }), + publishCatalogUpdates: true, + updatePersonaAndPublish: async () => { + throw new Error("relay unreachable: connection refused"); + }, + refetchStores: async () => ({ persona: persisted, agent: null }), + publishRetry: async () => { + throw new Error("relay still unreachable"); + }, + }); + + const result = await runAgentSaveCoordinator(opts); + + assert.equal( + result, + false, + "coordinator must return false when retry also fails", + ); + 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". + assert.match( + warnings[0].message, + /saved locally|saved.*not.*published|could not be published/i, + "warning must indicate saved locally but not published", + ); + assert.doesNotMatch( + warnings[0].message, + /reopen to retry publishing/i, + "terminal state must not tell the user to reopen — that is a lie", + ); + } finally { + cap.restore(); + } +}); + +test("test_no_publish_retry_seam_shows_terminal_warning_not_reopen", async () => { + // No publishRetry provided — coordinator must still not say "reopen to retry". + const cap = captureToasts(); + try { + const persisted = makeDefinition({ displayName: "Alice" }); + const opts = makeOpts({ + ctx: { + kind: "definition-only", + definition: makeDefinition({ displayName: "Alice" }), + }, + personaInput: makePersonaInput({ displayName: "Alice" }), + publishCatalogUpdates: true, + updatePersonaAndPublish: async () => { + throw new Error("failed to open retention db"); + }, + refetchStores: async () => ({ persona: persisted, agent: null }), + // publishRetry intentionally omitted (no seam available). + }); + + const result = await runAgentSaveCoordinator(opts); + + assert.equal( + result, + false, + "coordinator must return false with no retry seam", + ); + 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"); + assert.match( + warnings[0].message, + /saved locally|saved.*not.*published|could not be published/i, + "must indicate saved locally not published", + ); + assert.doesNotMatch( + warnings[0].message, + /reopen to retry publishing/i, + "no-seam terminal state must not say reopen to retry", + ); + } finally { + cap.restore(); + } +}); diff --git a/desktop/src/features/agents/ui/agentSaveCoordinator.ts b/desktop/src/features/agents/ui/agentSaveCoordinator.ts index b91a0a9606..19371abac0 100644 --- a/desktop/src/features/agents/ui/agentSaveCoordinator.ts +++ b/desktop/src/features/agents/ui/agentSaveCoordinator.ts @@ -79,6 +79,16 @@ export type SaveCoordinatorOptions = { updatePersonaAndPublish: ( input: UpdatePersonaInput, ) => Promise; + /** + * Publish the current on-disk definition to the catalog without writing any + * field changes. Called when the initial save+publish succeeded on disk but + * the publication step threw — the persona is persisted, so the retry only + * needs to re-attempt the relay submission. Uses the share-toggle strict + * path (`set_persona_shared`) which reads the definition from disk, so no + * field payload is needed. If omitted the coordinator falls back to a + * terminal "saved locally, not published" report. + */ + publishRetry?: (personaId: string) => Promise; updateManagedAgent: ( input: UpdateManagedAgentInput, ) => Promise<{ agent: ManagedAgent; profileSyncError: string | null }>; @@ -122,6 +132,7 @@ export async function runAgentSaveCoordinator( updateManagedAgent, setAutoRestart, setStartOnAppLaunch, + publishRetry, refetchStores, onDone, onSavedWhileStopped, @@ -418,18 +429,46 @@ export async function runAgentSaveCoordinator( if (!observedRemainder && publishFailed) { // The persona persisted but the publish command threw before returning a - // status — the relay was never reached. "Save and publish" did not deliver - // its promise: report "saved but not published" and keep the dialog open so - // the user can retry (re-submitting will re-attempt the publish path). + // status — the relay was never reached. Attempt a publish-only retry via + // set_persona_shared (reads the current on-disk definition; no field write + // needed). If the retry succeeds, fall through to full success. If it + // fails (or no retry seam is available), report a terminal "saved locally, + // not published" state — the flush loop will re-attempt on reconnect, and + // the user can re-open and save again to force a retry. const personaName = observedPersona?.displayName ?? personaInput?.displayName ?? def?.displayName ?? "Agent"; - toast.warning( - `${personaName} saved, but publishing to the catalog failed: ${firstError ?? "unknown error"} — reopen to retry publishing.`, - ); - return false; + if (publishRetry && def) { + let retryError: string | null = null; + try { + const retryResult = await publishRetry(def.id); + publicationStatus = retryResult.publicationStatus; + // Retry succeeded — the persona is published. Clear publishFailed and + // fall through to the full-success branch below. + publishFailed = false; + } catch (err) { + 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. + toast.warning( + `${personaName} saved locally, but could not be published to the catalog: ${retryError ?? firstError ?? "unknown error"} — it will be retried automatically.`, + ); + return false; + } + // publishFailed is now false — fall through to full success. + } else { + // No retry seam available — terminal state (same as retry failure). + toast.warning( + `${personaName} saved locally, but could not be published to the catalog: ${firstError ?? "unknown error"} — it will be retried automatically.`, + ); + return false; + } } if (!observedRemainder) { diff --git a/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts b/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts index 301b9840a4..593ad7f4a7 100644 --- a/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts +++ b/desktop/src/features/agents/ui/useAgentEditMergedSubmit.ts @@ -24,6 +24,7 @@ import type { UpdateManagedAgentInput, UpdatePersonaInput, } from "@/shared/api/types"; +import { setPersonaShared } from "@/shared/api/tauriPersonas"; import type { PersonaSharePublicationResult } from "@/shared/api/tauriPersonas"; import { seedAgentFormModel, @@ -286,6 +287,12 @@ export function useAgentEditMergedSubmit( runtimes: s.runtimes.length > 0 ? s.runtimes : undefined, updatePersona: s.updatePersona, updatePersonaAndPublish: s.updatePersonaAndPublish, + // Publish-only retry: when save+publish succeeds on disk but the + // publication step throws, re-publish the current on-disk head via + // set_persona_shared without writing any field changes. The persona + // is already shared=true (it was shared when the save+publish path + // was reached), so passing true re-publishes the current head. + publishRetry: (id: string) => setPersonaShared(id, true), updateManagedAgent: async (upd) => { if (!inst) throw new Error("No instance in definition-only context"); @@ -317,8 +324,13 @@ export function useAgentEditMergedSubmit( }); if (!success) { + // The coordinator already reported the specific failure via toast. + // This error banner is a secondary indicator for users who miss the + // toast — use an accurate summary that covers all false-return paths. setSaveError( - new Error("Some changes may not have persisted. Reopen to retry."), + new Error( + "Some changes may not have fully saved — see the notification above for details.", + ), ); } if (success && inst) {