fix(agents): address Carl round-5 review findings (P1-1, P1-2, P2)

P1-1 — Preserve publish failure through settlement: introduce `publishFailed`
flag in agentSaveCoordinator. When `publishCatalogUpdates` is true, the command
throws after save_personas completes, and settlement observes the persona as
persisted, set publishFailed = true and preserve firstError. A new
`!observedRemainder && publishFailed` guard fires before the full-success branch
— shows a warning toast ('saved, but publishing failed'), keeps the dialog open
(returns false) so the user can retry. Previously the publish failure was silently
discarded, onDone was called, and the toast claimed full success.

Required regressions (non-negotiable per Carl):
- Rust: retain_failure_after_persist_is_returned_not_swallowed — exercises
  update_persona_with with a failing retain callback; confirms the error
  propagates (command returns Err) even though save_personas already ran.
- Coordinator (TS): test_publish_throws_post_persist_does_not_settle_as_full_success
  and test_publish_throws_post_persist_returns_false_not_partial_failure_toast —
  verify updatePersonaAndPublish throwing post-persist produces warning toast,
  returns false, and does not call onDone.

P1-2 — Instance name validation: add instanceName.trim().length > 0 guard to
canSubmit when showInst is true and the definition has no name pool (pool-named
instances don't submit the visible name). Stop the agentFormModel fallback:
nameChanged and agentInput.name no longer fall back to displayName.trim() when
instanceName is empty; use the raw value so a cleared field is treated as a
no-op, not a silent definition-name copy.

P2 — Label copy and a11y: rename 'Agent name' labels to 'Agent name (shared
definition)' in DSection and 'Agent name (this deployed instance)' in
InstanceSection. Add aria-expanded + aria-controls='d-advanced-fields' to the
DSection Advanced disclosure button and id='d-advanced-fields' to its controlled
region (motion.div), mirroring the InstanceSection pattern.

Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>

desktop/src-tauri/src/commands/personas/update/concurrent_edit_tests.rs

@@ -300,3 +300,85 @@ async fn command_path_and_lock_scope_regressions() {
         None => std::env::remove_var("XDG_DATA_HOME"),
     }
 }
+
+/// P1-1 regression: the retain callback (the "publish" step inside
+/// `update_persona_with`) runs AFTER `save_personas`, so a retain failure
+/// occurs when the persona is already durable on disk.
+///
+/// The command-path contract: `update_persona_with` must return `Err(_)` when
+/// the retain callback returns `Err(_)`, even though the persona write already
+/// persisted. The error must reach the coordinator so it can set `publishFailed`
+/// and refuse to close the dialog as full success. If the retain error were
+/// swallowed — e.g. the callback's `?` were removed and it returned `Ok(())` —
+/// this test would become GREEN on a broken code path and catch it.
+#[tokio::test]
+async fn retain_failure_after_persist_is_returned_not_swallowed() {
+    let temp = tempfile::tempdir().unwrap();
+    let home = temp.path().join("home");
+    std::fs::create_dir_all(&home).unwrap();
+
+    let old_home = std::env::var_os("HOME");
+    let old_xdg = std::env::var_os("XDG_DATA_HOME");
+    {
+        let _path_guard = crate::managed_agents::lock_path_mutex();
+        std::env::set_var("HOME", &home);
+        std::env::set_var("XDG_DATA_HOME", &home);
+    }
+
+    let app = mock_app();
+
+    // Seed one persona at R1 in the persisted store.
+    save_personas(app.handle(), &[persona("p1", "Alice", R1)]).expect("seed write must succeed");
+
+    // Submit via update_persona_with with a retain callback that always fails.
+    // The persona save runs first (line ~210 in update.rs: `save_personas`),
+    // then the retain callback is called. If the retain error propagates, we
+    // get Err; if it is silently discarded, we get Ok — the test catches both.
+    let result = update_persona_with(
+        update_request("p1", "Alice retain-fail", Some(R1)),
+        app.handle().clone(),
+        |_app, _state, _persona| {
+            Err("simulated retain / publish failure after persona persisted".to_string())
+        },
+    )
+    .await;
+
+    assert!(
+        result.is_err(),
+        "update_persona_with must return Err when the retain callback fails — \
+         the save coordinator must see this error, not a silent success; \
+         got: {:?}",
+        result.ok()
+    );
+
+    let err = result.unwrap_err();
+    assert!(
+        err.contains("simulated retain"),
+        "the error text must propagate from the retain callback; got: {err}"
+    );
+
+    // Verify the persona DID persist (save_personas ran before retain) so we
+    // confirm the test scenario actually exercises the post-persist failure path.
+    let persisted =
+        crate::managed_agents::load_personas(app.handle()).expect("reload must succeed");
+    let stored = persisted
+        .iter()
+        .find(|p| p.id == "p1")
+        .expect("persona must exist");
+    assert_eq!(
+        stored.display_name, "Alice retain-fail",
+        "persona fields must have persisted before retain was called"
+    );
+
+    // Cleanup
+    std::env::remove_var("HOME");
+    std::env::remove_var("XDG_DATA_HOME");
+    match old_home {
+        Some(v) => std::env::set_var("HOME", v),
+        None => std::env::remove_var("HOME"),
+    }
+    match old_xdg {
+        Some(v) => std::env::set_var("XDG_DATA_HOME", v),
+        None => std::env::remove_var("XDG_DATA_HOME"),
+    }
+}

desktop/src/features/agents/ui/AgentEditMergedDialog.tsx

@@ -569,7 +569,11 @@ export function AgentEditMergedDialog({
         (parsedParallelism > 0 || parallelism === "") &&
         (selectedRuntimeId !== "custom" ||
           inheritHarness ||
-          agentCommand.trim().length > 0)
+          agentCommand.trim().length > 0) &&
+        // Instance name must be non-blank when the name field controls submission
+        // (no name pool — pool-named instances get their names from the definition
+        // pool and do not submit the visible name field).
+        ((def?.namePool?.length ?? 0) > 0 || instanceName.trim().length > 0)
       : true);
 
   // ── Runtime/model/provider handlers (extracted to useAgentEditRuntimeHandlers) ──

desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx

@@ -229,13 +229,13 @@ export function AgentEditMergedDSection({
 
   return (
     <>
-      {/* Agent name */}
+      {/* Agent name — definition (shared across all linked instances) */}
       <div className="space-y-1.5">
         <label
           className="text-sm font-medium text-foreground"
           htmlFor="edit-agent-display-name"
         >
-          Agent name
+          Agent name (shared definition)
         </label>
         <div
           className={cn(
@@ -455,6 +455,8 @@ export function AgentEditMergedDSection({
       {/* Advanced section toggle (name pool + D-env vars) */}
       <div className="space-y-2">
         <Button
+          aria-controls="d-advanced-fields"
+          aria-expanded={showAdvanced}
           className="flex items-center gap-1 px-0 text-sm text-muted-foreground hover:text-foreground"
           data-testid="persona-advanced-toggle"
           disabled={isSaving || !fieldEditable("namePool")}
@@ -474,6 +476,7 @@ export function AgentEditMergedDSection({
               animate={{ height: "auto", opacity: 1, scale: 1 }}
               className="origin-top overflow-hidden space-y-4"
               exit={{ height: 0, opacity: 0, scale: 0.98 }}
+              id="d-advanced-fields"
               initial={{ height: 0, opacity: 0, scale: 0.98 }}
               key="d-advanced-fields"
               transition={advancedFieldsTransition}

desktop/src/features/agents/ui/AgentEditMergedDialogInstanceSection.tsx

@@ -222,7 +222,7 @@ export function AgentEditMergedInstanceSection({
           className="text-sm font-medium text-foreground"
           htmlFor="edit-agent-name"
         >
-          Agent name
+          Agent name (this deployed instance)
         </label>
         <div
           className={cn(

desktop/src/features/agents/ui/agentFormModel.ts

@@ -541,7 +541,12 @@ export function emitAgentFormDiff(
     const nameChanged =
       isI("instanceName") &&
       !hasNamePool &&
-      (next.instanceName ?? next.displayName.trim()) !== inst.name;
+      // 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.
+      (next.instanceName ?? "") !== inst.name;
     const systemPromptChanged =
       isI("systemPrompt") &&
       (next.systemPrompt.trim() || null) !== (inst.systemPrompt ?? null);
@@ -615,8 +620,7 @@ export function emitAgentFormDiff(
 
     if (iChanged) {
       agentInput = { pubkey: inst.pubkey };
-      if (nameChanged)
-        agentInput.name = next.instanceName ?? next.displayName.trim();
+      if (nameChanged) agentInput.name = next.instanceName ?? "";
       if (systemPromptChanged)
         agentInput.systemPrompt = next.systemPrompt.trim() || null;
       if (modelChanged) agentInput.model = next.model ?? null;

desktop/src/features/agents/ui/agentSaveCoordinator.test.mjs

@@ -1568,3 +1568,128 @@ test("test_published_definition_rename_notice_uses_observed_persona_name", async
     cap.restore();
   }
 });
+
+// ── Test family 10: P1-1 regression — publish failure after persona persists (Carl round-5) ─
+//
+// `update_persona_and_publish` saves the persona first then calls the retain
+// callback (prepare_persona_publication). If the retain call throws, the command
+// propagates the error — but the persona fields are already on disk. The
+// coordinator catches the throw, sets caughtError, then runs a settlement refetch
+// that sees the persona fields match the submission → `persisted = true`. Before
+// this fix, `publishFailed` was never set and the coordinator entered the full-
+// success branch, called `onDone`, and showed the publish success toast despite
+// the relay never having been reached.
+//
+// After the fix: when `publishCatalogUpdates` is true AND the command threw AND
+// the persona persisted, `publishFailed` is set. The `!observedRemainder &&
+// publishFailed` guard fires BEFORE the full-success branch — `onDone` is NOT
+// called, a warning toast names the failure, and the coordinator returns false so
+// the dialog stays open for a retry.
+
+test("test_publish_throws_post_persist_does_not_settle_as_full_success", async () => {
+  // The critical scenario: updatePersonaAndPublish throws (the retain/enqueue
+  // step failed) but the persona fields ARE observed as persisted in the
+  // settlement refetch. Before the fix this reached `onDone` and returned true;
+  // after the fix it must return false and show a warning, NOT a success toast.
+  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 () => {
+        // Simulates prepare_persona_publication throwing after save_personas ran.
+        throw new Error("failed to open retention db");
+      },
+      // Settlement: persona IS on disk (save_personas ran before the throw).
+      refetchStores: async () => ({ persona: persisted, agent: null }),
+    });
+
+    const result = await runAgentSaveCoordinator(opts);
+
+    assert.equal(
+      result,
+      false,
+      "a publish-path throw must not settle as full success even when the persona persisted",
+    );
+    assert.equal(
+      opts._calls.onDone,
+      0,
+      "onDone must NOT be called when publish failed — dialog must stay open for retry",
+    );
+    const successes = cap.captured.filter((c) => c.kind === "success");
+    assert.equal(
+      successes.length,
+      0,
+      "a publish failure must NOT show a success toast",
+    );
+    const warnings = cap.captured.filter((c) => c.kind === "warning");
+    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",
+    );
+    assert.match(
+      warnings[0].message,
+      /retention db|failed to open|failed/i,
+      "warning must include the underlying error text",
+    );
+  } finally {
+    cap.restore();
+  }
+});
+
+test("test_publish_throws_post_persist_returns_false_not_partial_failure_toast", async () => {
+  // Distinguishes publishFailed from ordinary partial failure: the persona IS
+  // in persistedParts (observed match), failedParts is empty (no other writes
+  // failed), so `observedRemainder` is false. Only the publishFailed guard fires.
+  // The warning must NOT say "profile saved. … failed: …" (the partial-failure
+  // wording) — it must say the persona saved but publishing failed.
+  const cap = captureToasts();
+  try {
+    const persisted = makeDefinition({ displayName: "Alice-renamed" });
+    const opts = makeOpts({
+      ctx: {
+        kind: "definition-only",
+        definition: makeDefinition({ displayName: "Alice" }),
+      },
+      personaInput: makePersonaInput({ displayName: "Alice-renamed" }),
+      publishCatalogUpdates: true,
+      updatePersonaAndPublish: async () => {
+        throw new Error("relay unreachable");
+      },
+      refetchStores: async () => ({ persona: persisted, agent: null }),
+    });
+
+    const result = await runAgentSaveCoordinator(opts);
+
+    assert.equal(result, false, "publish failure returns false");
+    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");
+    // Must mention "saved" AND "publishing failed" or similar.
+    assert.match(
+      warnings[0].message,
+      /Alice-renamed.*saved|saved.*Alice-renamed/i,
+      "warning must name the saved persona",
+    );
+    assert.match(
+      warnings[0].message,
+      /publishing.*failed|catalog.*failed|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"
+    assert.doesNotMatch(
+      warnings[0].message,
+      /profile failed/i,
+      "must not use the partial D/I-failure wording for a publish-only failure",
+    );
+  } finally {
+    cap.restore();
+  }
+});

desktop/src/features/agents/ui/agentSaveCoordinator.ts

@@ -222,6 +222,11 @@ export async function runAgentSaveCoordinator(
   let publicationStatus:
     | PersonaSharePublicationResult["publicationStatus"]
     | null = null;
+  // True when the persona fields persisted but the catalog publication step
+  // threw before it could return a status (publish path only). The profile is
+  // on disk, but the relay was never reached or enqueued, so settlement must
+  // not close as full success — it must report "saved but not published".
+  let publishFailed = false;
 
   // Step 1: Definition write — settle immediately, stop if not persisted.
   if (personaInput) {
@@ -269,6 +274,13 @@ export async function runAgentSaveCoordinator(
     if (!persisted) {
       firstError =
         caughtError ?? "Agent profile did not persist — reopen to retry.";
+    } else if (publishCatalogUpdates && caughtError !== null) {
+      // The persona fields made it to disk (persisted = true) but the publish
+      // command threw before it could return a status — the catalog was never
+      // reached. Mark this so final settlement does not close as full success:
+      // "save and publish" promised a relay outcome that was not delivered.
+      publishFailed = true;
+      firstError = caughtError;
     }
   }
 
@@ -404,6 +416,22 @@ export async function runAgentSaveCoordinator(
 
   const observedRemainder = failedParts.length > 0;
 
+  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).
+    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 (!observedRemainder) {
     // Full success — every submitted write is reflected in observed state.
     // Prefer the OBSERVED name from the final refetch over `latestAgent`.