From ceada06a16f625caf66b73b85e9dd7ec611f074a Mon Sep 17 00:00:00 2001 From: Duncan Date: Sat, 29 Aug 2026 13:42:00 -0400 Subject: [PATCH] fix(agents): address Carl round-5 review findings (P1-1, P1-2, P2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Will Pfleger --- .../personas/update/concurrent_edit_tests.rs | 82 ++++++++++++ .../agents/ui/AgentEditMergedDialog.tsx | 6 +- .../ui/AgentEditMergedDialogDSection.tsx | 7 +- .../AgentEditMergedDialogInstanceSection.tsx | 2 +- .../src/features/agents/ui/agentFormModel.ts | 10 +- .../agents/ui/agentSaveCoordinator.test.mjs | 125 ++++++++++++++++++ .../agents/ui/agentSaveCoordinator.ts | 28 ++++ 7 files changed, 253 insertions(+), 7 deletions(-) diff --git a/desktop/src-tauri/src/commands/personas/update/concurrent_edit_tests.rs b/desktop/src-tauri/src/commands/personas/update/concurrent_edit_tests.rs index c7429b35a9..b19821afd8 100644 --- a/desktop/src-tauri/src/commands/personas/update/concurrent_edit_tests.rs +++ b/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"), + } +} diff --git a/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx b/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx index f67aef61c7..d0612f5cba 100644 --- a/desktop/src/features/agents/ui/AgentEditMergedDialog.tsx +++ b/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) ── diff --git a/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx b/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx index 04740b386b..72affb91de 100644 --- a/desktop/src/features/agents/ui/AgentEditMergedDialogDSection.tsx +++ b/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) */}