feat(desktop): merge agent edit surface into a single dialog - #5328
wpfleger96 wants to merge 53 commits into
Conversation
|
I do not think the failing E2E tests should be updated around this head. They are catching real lost behavior.
I restored the existing production routes while leaving the new model aside. The 33 affected smoke tests, the catalog publication integration, typecheck and all 4,540 unit tests pass. The flat model still needs an ownership fix too: linked respond-to, allowlist and parallelism are emitted to the instance, while definition-only changes to those fields are not emitted. I would split this work. Keep the existing routes, then land the field model once it represents shared template state and per-agent state separately. Route one edit path at a time with parity tests. The product can still say “Edit agent” and “Edit its template”; the storage boundary does not need to become user-facing. |
|
Thanks for turning this around quickly. At Three behavior blockers remain:
Please add route-level regressions for the linked-template task and the agent-requested |
|
At the current head, two concrete problems from my last pass are fixed: respond-to requests are carried through, and linked profiles can edit definition fields again. The 57 focused tests and typecheck pass here. The remaining blocker is now clearer. In an instance-with-definition context, the form deliberately renders definition, instance and local fields together, then one Save writes all three boundaries. Changing the name, prompt or definition runtime changes the shared definition. Changing access, the harness pin or launch settings changes only this agent. The user is never asked which object they mean to edit, and linkedInstanceCount appears only in the delete copy. Field ownership in code does not solve that user problem. These are two actions:
The components can be shared, but a linked definition change and an instance change should not be combined silently into one edit transaction. Please add a route-level E2E test proving the user chooses the object and sees the impact before a definition save. |
wolfyy970
left a comment
There was a problem hiding this comment.
Round 8 improves field ownership, but the product blocker from my last review is unchanged. A linked agent still opens one form with definition and instance controls and one Save. There is no choice between Edit this agent and Edit its template, and the affected count and names still appear only in delete copy. The new E2E checks control visibility, not that flow.
I also found three correctness problems in definition-only editing:
- An unset respond-to policy renders as Anyone, although minting resolves unset to Owner only.
- Parallelism accepts 0, negatives, or values over 32. Save stays enabled; invalid low values are silently replaced with the old value, while high values fail in the backend.
- Returning a manually changed runtime to the auto-seeded default can drop that deliberate choice because the auto-seed marker never clears.
Please split agent and template actions, show the impacted agents before a template save, and add route-level tests for these defaults and validation.
8c6dc28 to
8096c37
Compare
wolfyy970
left a comment
There was a problem hiding this comment.
At 8096c37a2, the access default and runtime reselection bugs are fixed, and the R6 test now reaches the real dialog. Thanks for closing those.
Three blockers remain:
- A linked agent still opens one form where one Save can change both the agent and its shared definition. There is no choice between Edit this agent and Edit its template, and affected agents are not shown before the template write. The new E2E now locks in that combined transaction.
- Definition parallelism has no maximum or visible validation. Blank, zero and negative input is silently replaced with the saved value, while values above 32 reach backend rejection.
- If the same definition changes while this dialog is open, the form stays stale. Submit then seeds from the newer definition, combines it with stale form values and can overwrite the concurrent update. Please detect version drift or rebase the edit, with a regression test.
I am keeping changes requested at this head.
wolfyy970
left a comment
There was a problem hiding this comment.
At 8cc6e5a11, blank definition parallelism now reaches the wire as a clear, and the policy settlement change looks right. That closes the clearing defect.
Three blockers remain:
- The linked route still combines agent and template edits behind one Save. There is no object choice or affected-agent disclosure, and the E2E still requires one save to change both layers.
- Definition-only parallelism still bypasses validation. The input has no maximum, and nonblank zero or negative values silently revert to the saved value. Please cover
0, negatives,32and33through the route. - A same-definition update arriving while the dialog is open can still be overwritten. The form stays stale, submit combines it with the latest definition seed, and the backend replaces the definition without revision checking.
I am keeping changes requested at this head.
2cd05be to
b25ac47
Compare
wolfyy970
left a comment
There was a problem hiding this comment.
I re-reviewed b25ac47cf8. The observed-state save coordinator is a real improvement, but the three blockers from the last head remain.
- The linked route still puts shared definition fields and instance fields behind one Save. The E2E now explicitly requires one save to change both layers. The linked-agent count appears only in delete copy, so the user never chooses Edit this agent or Edit its template and never sees who a template edit affects.
- Definition-only parallelism still bypasses the save gate because validation runs only when an instance is present. Zero and negative values silently fall back to the saved value; 33 reaches backend rejection. Please enforce 1 through 32 in the form and cover 0, negative, 32 and 33 through the route.
- The coordinator refetches only after writing. If the definition changes while the dialog is open, it submits the stale form without an expected revision and can overwrite the newer definition. Please reject or rebase that edit before the first write.
I am keeping changes requested at this head.
d1e55f4 to
b23b706
Compare
Merges the two agent edit dialogs (AgentInstanceEditDialog, AgentDefinitionDialog) into a single AgentEditMergedDialog that every edit entry point — the agents library, the profile panel, owner review, and the requestOpenEditAgent event path — reaches through AgentEditDialog. AgentInstanceEditDialog is deleted, AgentDialog's instance-edit arm is removed, and the editPersonaDialogState export is gone. One canonical form model: seedAgentFormModel builds an AgentFormModel from the edit context and owns every editable field; emitAgentFormDiff routes each changed field to its owning layer via the single FIELD_OWNERS map, which also drives per-field editability and the catalog-publish dirty signal. Save coordinator settles from observed state: definition → instance → policy writes, each followed by a store refetch — a write that threw after landing on disk counts as persisted, and a submitted behavior group (respondTo / respondToAllowlist / parallelism) settles as the full-replacement unit the backend writes, so a clear the backend failed to apply cannot false-succeed. Definition and instance runtime state are independent; definition behavior fields (access, allowlist, parallelism) are first-class with an unset respondTo shown as owner-only; team-managed definitions are read-only with a notice while instance-owned fields stay editable. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…o merged dialog Every field reachable in main's AgentDefinitionDialog edit mode was not carried into the merged dialog's definition (D) section: the numeric tuning knobs and buzz-agent effort knob, the provider API-key field, the parallelism cap hint, and the full EnvVarsEditor prop set (required-key highlighting, file-satisfied indicators, mint-key annotation, hidden structured keys). A definition-backed or definition-only edit therefore lost controls it had on main. These are additive-only restorations to reach field parity. The instance-side credential state was gated on open && showInst, starving the D-section in definition-only context. useAgentEditRuntimeState now derives a DSectionAdvancedState bundle from the definition runtime, provider, and env — independent of the instance harness pin — mirroring AgentDefinitionDialog. Bundling it as one prop keeps AgentEditMergedDialog under the desktop line-size gate. Tuning and API-key edits write the definition env layer, so emitAgentFormDiff routes them to personaInput and never the instance overlay. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…credential gate follow the definition layer The parity restoration exposed four behavioral gaps in the merged agent-edit dialog's definition (D) section, all rooted in the runtime hook multiplexing D- and I-layer selection on `showInst`: - Provider options, model discovery, and model-scope clearing switched on `showInst`, so in linked context they followed the instance harness pin instead of the definition runtime whose picker is rendered. Collapsed to a single `activeRuntimeId`/`activeRuntime` layer choice (definition whenever `showDef`, instance pin only for unlinked instance-only). - The D inherited-env layer reused the persona-inclusive overlay, so clearing a definition tuning value echoed its own just-deleted value as the inherited placeholder. Derive a D-only layer from global/build defaults with no persona env, matching main's AgentDefinitionDialog. - The definition's missing-required-credential signal was computed but dropped, so Save was never gated and the collapsed Advanced badge never showed. Carry it, gate Save whenever `showDef && !defReadOnly`, and render the shared AdvancedRequiredBadge; team-managed definitions never block instance edits. Adds linked-runtime, clear/reopen, save-gate, and seam-interaction regression tests that fail when any restored write callback is disconnected. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
A config-nudge deep link opens the merged agent-edit dialog with an initialFocus target, but Radix's default open-autofocus lands on the first focusable control and, running after the dialog's rAF-scheduled targeted focus, steals it back — so the deep link never lands on the requested field. Prevent the default open-autofocus whenever an initialFocus is supplied and let the targeted effect own initial focus. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
IMPORTANT: refresh_for_persona_at read only personas.json, which is retired (renamed .bak) by the Phase 1A.2 fold migration. On a post-fold install this resolved zero definitions, causing resolve_team_members to fail and resolve_and_refresh_or_retract_at to retract (purge+tombstone) every affected shared 30178 head. A successful persona retry/share-toggle would silently destroy the team catalog and report success. Fix: replace the single-store load in refresh_for_persona_at with the same dual-store rule as boot reconciliation: try personas.json first; if empty, read keyless definition records from managed-agents.json. Reuses crate::event_sync::read_persona_definitions (promoted to pub(crate)). Test: split the regression into pre-fold and post-fold variants. Post-fold variant: teams.json + keyless managed-agents.json record, personas.json explicitly absent. Verified mutations: - Mutation A (single-store revert): pre-fold GREEN, post-fold RED 0/1. - Mutation B (delete refresh call): both RED 0/2; restored: both GREEN 2/2. MINOR 1: remove stale coordinator comment claiming the flush loop retries and reopen/save forces retry — both proven false by Thufir's trace. MINOR 2: add per-team eprintln logging in refresh_for_persona_at for RemovalQueued and Err outcomes, matching the logging in the AppHandle path (refresh_shared_team_catalog_heads_for_persona). Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…ze cap tests.rs was at 1044 lines (split-count), over the 1000-line ceiling. Move the new command-path regression tests into a separate sub-module file pending/tests/retry_refresh.rs, following the existing cross_device/gate pattern. tests.rs drops to 830 lines (split-count 831). No behavior change: both tests pass and both mutations still hold. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested on exact head 46053a730aac1746b3265b2488d215cd32b97275, against base 00e61eafa917d296104006576b7a2ddbfd58bb5a.
The instance-name validation/emission and scoped labels/Advanced accessibility findings from the previous review are resolved. The merged-dialog product direction is unchanged. Publication recovery still has the following defects:
P1 — Combined saves bypass publication recovery
desktop/src/features/agents/ui/agentSaveCoordinator.ts:428–430 only handles publishFailed when there is no instance/policy remainder. Reproduce with a shared linked definition: change its prompt and an instance setting (or launch policy), then make strict publication preparation fail after save_personas commits but before the new event is retained (personas/update.rs:207–212; personas/pending.rs:173–200). The coordinator records publishFailed and firstError (288–294), skips instance/policy writes (299,328), and therefore bypasses the new publishRetry call. The partial warning says the profile saved and to reopen/retry (514–519), without preserving the unpublished outcome. Reopening seeds the already-saved definition; retrying only the remaining instance/policy changes emits no personaInput, so publication is never retried and this new head has no pending row to flush.
Settle publication independently of the other unsaved parts. Attempt the publish-only recovery for a persisted definition even when I/L writes remain, and report any unrecovered publication explicitly alongside those remaining changes. Add post-persist strict-preparation-failure cases with both an instance change and a policy change, including recovery success/failure. The new tests currently exercise the post-persist publication retry only in definition-only context.
P1 — New unlocked team refresh can undo an explicit unshare
desktop/src-tauri/src/commands/personas/sharing.rs:119–122 runs refresh_for_persona_at after releasing the managed-agent store lock. That helper reads the current shared team head, builds a shared=true replacement, and retains it without a transaction or a fresh visibility check (teams/pending.rs:325–365). A concrete interleaving: after a saved member prompt changes, this retry refresh reads shared head T; concurrent set_team_shared(false) acquires the store lock and retains unshared T+1; the refresh then retains its shared replacement at T+1. retain_event accepts equal timestamps (managed_agents/retention.rs:213–222), replacing the pending unshare. If the toggle's flush starts afterward, it publishes the shared replacement instead of the requested unshare. This can leave the team discoverable after the owner removed it.
Serialize the refresh's authoritative store/head reads and retain/retract operation with the same store lock as team edits/unshare/delete, without holding it across the network await. Add a controlled concurrent unshare regression at the production retry boundary. The filesystem seam's sequential pre-/post-fold tests do not cover this interleaving.
P2 — Successful publish-only retry hides skipped linked identity updates
The new success path at agentSaveCoordinator.ts:444–451,474–510 treats a successful setPersonaShared retry as complete recovery. For a shared definition name/avatar edit, strict retention failure occurs after the definition save but before linked instance name/avatar propagation and relay profile sync (desktop/src-tauri/src/commands/personas/update.rs:209–217,227–289,304–324). If the transient failure clears, the publish-only retry publishes the definition and refreshes teams (personas/sharing.rs:110–122), but never performs those skipped effects. The dialog then reports success and closes while linked identities retain the old name/avatar. A no-op reopen/save cannot recover the original name transition.
Keep local dependent effects independent of a failed enqueue, or preserve an explicit partial outcome until they are completed; do not claim full recovery from the catalog-only retry. Cover a definition-only name/avatar edit with a linked instance, a post-save strict-retain failure, and a successful publication retry, asserting the dependent identity state as well as the catalog event.
Validation: exact-head source/diff and regression-test inspection only; no checkout, build, tests, or PR-code execution. All three delegated lanes returned and were integrated. This is a bounded re-review of the agreed merged edit/save contract and changed recovery paths, not a request to restore separate dialogs or expand create-flow scope.
…h lock, linked identity effects Three correctness fixes from review 5070974257: P1-1 (combined saves bypass publication recovery): settle publication independently at the definition boundary before advancing to I/L writes. When the publish command throws after persona persist and I/L writes remain, attempt the publish-only retry immediately — not at final settlement where the !observedRemainder gate suppresses it. On retry success, clear publishFailed and continue the same save. On retry failure (or no seam), set firstError to block I/L advancement with a clear publication-failure reason; the partial-failure toast names both D (saved) and the I/L remainder (not saved). Definition-only saves retain the existing final !observedRemainder && publishFailed path unchanged. New family-12 TS tests cover D+I and D+L combined save with retry success, retry failure, and no-seam cases; mutation (restoring the original !observedRemainder gate) turns all four RED. P1-2 (unlocked retry refresh can undo explicit unshare): serialize refresh_for_persona_at inside a managed_agents_store_lock scope in publish_and_refresh_teams_at, acquired after the network await and held for the synchronous refresh. Without the lock a concurrent set_team_shared(false) can retain an unshared head that the unlocked refresh overwrites at the same monotonic timestamp. Adds REFRESH_LOCK_OBSERVER (cfg(test)) matching the PRE_GUARD_OBSERVER pattern; new test asserts try_lock() fails while the refresh runs — removing the lock scope causes try_lock() to succeed, turning the test RED. P2 (successful publish-only retry hides skipped linked identity effects): capture retain_result without ? in update_persona_with, complete linked managed-agent name/avatar propagation and save_managed_agents, then propagate the retain error. Before the fix, a strict publication failure after save_personas skipped propagate_persona_name_rename and save_managed_agents entirely; a subsequent publish-only retry published the persona but linked instances retained stale names/avatars. New Rust regression seeds a persona + linked instance, induces a retain failure, and asserts the linked agent's name was updated before the error propagated; restoring retain()? before the propagation block turns the test RED. Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz> Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Resolve conflicts in personaDialogState.ts, personaDialogState.test.mjs, and types.ts by taking origin/main's version (our branch made no changes to those files). sharing.rs and update.rs auto-merged cleanly — our P1-2 lock scope and P2 retain-result changes combined with main's #7126 description field additions without conflict. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…er main merge After merging origin/main (cb31449, #7126 public descriptions), two struct initializers need updating for the new description field introduced by that PR: - concurrent_edit_tests.rs: add description: None to AgentDefinition, UpdatePersonaRequest (×2), and ManagedAgentRecord initializers - personaTypes.ts: add expectedUpdatedAt to UpdatePersonaInput (moved from types.ts by #7126 but the field was not carried across) Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…py::type_complexity Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Three async tests in concurrent_edit_tests.rs used std::env::set_var("HOME")
for store isolation but held lock_path_mutex() only in a setup block, releasing
it before the actual I/O. When the tokio test runner executed them concurrently,
tests raced on the process-global HOME env var and read from each other's temp
directories.
Fix: convert all three from #[tokio::test] async fn to #[test] fn with an
explicit single-threaded tokio runtime (block_on). lock_path_mutex() is now
held at fn scope for the full test duration — no MutexGuard across await, no
clippy flag, and the mutex serializes the three tests so HOME cannot be
clobbered mid-run.
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
* origin/main: Add voice notes to desktop messages (#6978) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
* origin/main: ci: run PostgreSQL tests in isolated lane (#6730) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz>
…e, D+L failure Three correctness fixes from Thufir pass-1 review at 08467d5: P2 (retain error at wrong boundary): remove ? from retain_result inside the blocking phase's Ok(...) return, completing linked managed-agent name/avatar propagation (save_managed_agents) and relay kind:0 profile sync (phase 2) before propagating the error. Add regression test in sharing.rs using update_persona_with with a prepare_persona_publication_at closure (EISDIR failure) and counter server; mutation restoring retain_result? before phase 2 makes count == 0 → RED. Complementary counter-server test in concurrent_edit_tests.rs with injected Err closure also turns RED. P1-2 (lock-scope regression has no actual race): replace the stub try_lock test with a real concurrency regression using std::sync::Barrier(2). The barrier fires inside the REFRESH_LOCK_OBSERVER while the lock is held, releasing a racing OS thread that blocks on managed_agents_store_lock.lock() until refresh completes, then retains an unshared persona head at T+1. Final assertion verifies the retained event is both later than the seed AND unshared. Mutation (remove/move lock acquisition) causes try_lock() to succeed → first assert panics → RED. D+L retry-failure (family-12 gap): add test_combined_dl_save_publish_retry_failure_names_both_catalog_and_remainder symmetric to the D+I case: publish throws, retry also fails, policy write must be skipped (firstError blocks step 3), warning names profile as saved + auto-restart policy as unsaved remainder + catalog publication as failure. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested on exact head 08467d5824272f10055d82df42c3f1f67f4c4e68, against base bd73490418266f267d9bb3bdf13e64582adc8e80.
The prior combined-save publication recovery and team-refresh/unshare locking defects are fixed in the inspected production paths. Linked local name/avatar persistence also now runs before a strict-retain error is returned. The merged-dialog product direction remains unchanged. Two blockers remain:
P1 — Unrelated definition edits silently erase the authored description
At agentFormModel.ts:496–505, the merged form emits a replacement definition without description. The form does not seed or expose that field either. Both update commands pass this payload through tauriPersonas.ts:113–133, where an omitted description becomes null. Rust assigns that null to the definition and saves it (personas/update.rs:218,264,283). Settlement never compares description, so the deletion is reported as a successful save.
Reproduce with an editable definition that already has a nonempty public description: open the merged editor, change only its prompt, and save. The description is cleared locally and can be published as cleared to the catalog and linked profiles. Both definition-only and linked-definition saves are affected; I/L-only saves do not issue this definition write. This is a concrete base-to-head regression: at the exact base, personaDialogState.ts:121–126 preserves the description and AgentDefinitionDialog.tsx:209–211,361–384,767–773 seeds, submits, and exposes it. This PR reroutes those edits to the merged form.
Carry description through the merged form's seed, definition ownership/diff, editing control, replacement payload, and settlement. Cover preservation on an unrelated definition edit through the real adapter for both plain save and save-and-publish, plus explicit description edit/clear behavior. Restoring a separate dialog is not requested.
P2 — Strict-retain failure still skips linked relay-profile sync
personas/update.rs:372–376 propagates retain_result? before the phase-2 sync_managed_agent_profile loop at lines 383–401. The corrective change now persists linked local records, but discards the collected kind:0 sync work on this failure path.
Reproduce with a shared definition and a linked instance with valid agent keys: edit the definition's name/avatar and make strict publication preparation fail after the definition save; let the immediate catalog-only retry succeed. set_persona_shared republishes the persona and refreshes team catalog heads, not linked kind:0 profiles (personas/sharing.rs:44–81,122–160). The coordinator then reports success and closes (agentSaveCoordinator.ts:484–550), while other clients still see the old name/avatar. Boot or a later successful start can reconcile this from the corrected local records (commands/agents.rs:1008–1032, managed_agents/restore.rs:431–485); this is stale relay metadata until reconciliation, not permanent identity loss. That later lifecycle repair does not complete the current save's skipped work.
Keep the captured retain result intact across the blocking return, release the store lock, run the existing best-effort phase-2 sync, and only then propagate the retain error. Preserve the original strict publication failure for the coordinator. Add a production-path regression with valid agent keys and a relay observer that asserts kind:0 sync is attempted even when retention fails. The new linked-instance test only checks the local name and seeds an empty private key (update/concurrent_edit_tests.rs:446,535–548), so it cannot witness this phase.
Non-blocking test notes: the new refresh observer checks the lock at its probe, not an actual concurrent unshare; and the combined-save failure test overrides updateManagedAgent without incrementing the counter it asserts (agentSaveCoordinator.test.mjs:2025–2043). Neither is an additional production blocker.
Validation: all three delegated lanes returned and were independently integrated. Exact-base/head/prior source, diffs, and regression-test inspection only. No checkout, builds, tests, or PR-code execution; no fresh CI-pass claim. This is a bounded re-review of the three agreed recovery fixes plus the concrete description merge regression, not a fresh review of unrelated incoming-main features.
…server fixtures Two new test-only axum counter servers added in the Thufir pass-1 fix commit introduce /events route registrations that the inventory scan did not know about: - sharing.rs: bump from 1 → 2 (existing spawn_relay route + new P2 counter server in test_update_and_publish_relay_profile_syncs_despite_preparation_failure) - concurrent_edit_tests.rs: add row at 1 (P2 counter server in linked_instance_relay_profile_syncs_despite_retain_failure) Both are #[cfg(test)] fixtures with no production egress; production publishes go through the guarded boundary-1 funnel (submit_signed_event_at_with_keys). Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Item 1 — P1-2 race regression (shared-team lock scope): Seed a real kind:30178 head T in the retention DB before the test, write teams.json with the real shared team so refresh_for_persona_at finds it, and write a modified persona so the rebuild content differs from T (avoiding the idempotency skip). Move REFRESH_READ_OBSERVER from sharing.rs into commands/teams/pending.rs and fire it inside refresh_or_retract_shared_head_at after the shared-T read, before retain. The observer asserts try_lock().is_err() (lock held), then signals a Barrier(2) so the race thread unblocks. Race thread acquires managed_agents_store_lock and retains an unshared T+1; since it runs last, the final retained head is UNSHARED. Mutation (move refresh outside lock): observer fires with lock released, try_lock() succeeds, assertion panics — RED. Item 2 — P2 real-seam acceptance (update_persona_and_publish): Extract update_persona_and_publish_inner<R: tauri::Runtime> as the generic core; the #[tauri::command] wrapper becomes a one-line delegate. Test calls update_persona_and_publish_inner through a MockRuntime AppHandle, resolves the active retention scope (so the production prepare_persona_publication resolver is exercised), then sabotages the DB path by replacing it with a directory. The production command path hits prepare_persona_publication, which returns EISDIR; phase-2 relay sync must still fire before the error propagates. Mutation (retain_result? before phase 2): counter = 0 → RED. Item 3 — Description field regression (built-in persona edits persist): Add description: string to AgentFormModel (D-owned), seed it from def.description, carry it through buildNextAgentFormModel, emit it in emitAgentFormDiff's dChanged check and personaInput. Wire description state through AgentEditMergedDialog and AgentEditMergedDSection. Replace the D-section's hand-rolled displayName block with AgentIdentityFields (from AgentDescriptionField.tsx) which renders both Agent name and Description with the #7126 clamp/counter semantics. The agents.spec.ts 'built-in persona edits persist' test can now find getByLabel('Description') in the merged dialog. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…agent-edit * origin/main: feat(buzz-agent): add DatabricksAuthCoordinator single-flight OAuth (#5545) fix(desktop): preserve keyring identity during recovery (#7203) feat(mobile): prepare `buzz-push-gateway` for deployment (#7158) ci: relax file-size ceilings by surface (#6485) fix(mobile): isolate extension linker flags; complete iOS build in CI (#7187) chore(ci): lower Codex security review effort (#7179) fix(dev-mcp): extend shell timeout cap to 20 minutes and align outer budgets (#7185) fix(dev): keep the canonical profile when launching from desktop/ (#7143) feat(buzz-auth): add production NIP-FI federated assertion runtime (#7109) Hide download action on voice notes (#7182) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz> # Conflicts: # desktop/src/features/agents/AGENTS.md
…ssertions P1-2 race test: replace barrier+try_lock lock-presence oracle with a deterministic two-phase mpsc channel harness. Observer pauses refresh after its shared-T read; unshare thread announces readiness, receives explicit go-signal, completes its unshared retain, then signals done. Lock-move mutation: observer probes lock free, waits for unshare to finish, then releases refresh; refresh writes shared T+1 last → final head is SHARED → test RED at the final '!event_is_shared' assertion. No lock-presence assertion, no sleeps. P2 command test: load contract from shared test-fixtures/update-persona-publish-partial-outcome.json; reload both durable stores after the strict preparation error and assert the persona rename and linked ManagedAgentRecord persisted; assert error text, relay-profile request count, and retry status from contract. Early-? mutation → kind:0 count 0 != 1 → RED. Coordinator recovery test: consume same contract; gate retry completion with a promise and assert onDone === 0 while retry is in flight, then release and assert onDone === 1 after completion. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: one P2 remains
Reviewed head d739ba791332a9bb7a628593dbce64b290ecb66f against base 5aed49b505a7e27f3b0e34dafa53d6c4e8cdcd64. This is a bounded corrective re-review; the single merged editor and D → I → L save contract remain the product direction.
[P2] Include description in observed save settlement. observedStateMatchesPersonaInput never compares description. Change only an editable definition’s description to text containing U+200B. The input/adapter preserve it, and Rust rejects it before writing (personas/update.rs:218; definition_validation.rs:50–60,89–107,159–178). The unchanged definition nevertheless passes the coordinator’s per-boundary and final comparisons (agentSaveCoordinator.ts:279–288,414–423), so ordinary Save reports success and closes, discarding the draft. Save-and-publish instead treats that pre-write rejection as a publication failure, can republish the old record, and then closes successfully; combined edits can advance to I/L despite the unsaved description.
Compare description using the backend’s accepted canonical storage semantics, including absent/empty as explicit clear, without normalizing prohibited bytes into apparent success. Add production-seam regressions for rejected/nonpersisted description-only edits and clears on both ordinary and publish paths: no close, no old-record publication retry, and no I/L advancement. Also cover successful normalization and preservation on unrelated definition edits. The model, submit, and coordinator test files currently contain no description cases.
Closed in source: the prior unrelated-edit description-erasure mechanism is fixed by seed/control/emission wiring. The prior linked kind:0 sync omission is fixed by carrying the retention Result through the blocking phase, attempting profile sync outside the store lock, and only then propagating the retention error. The new native regression exercises the production publication command with valid keys and a request counter; the team-refresh test now checks retained state under an interleaving. These are inspected test oracles, not executed mutation results.
Validation: exact-tree/blob-verified source and test-text review with independent frontend/native lanes; no checkout, builds, tests, or PR-code execution. Previously closed runtime, ownership, CAS, and refetch findings were not reopened; unrelated incoming-main features and create-flow unification are excluded.
| return false; | ||
| if (observed.systemPrompt.trim() !== (submitted.systemPrompt ?? "").trim()) | ||
| return false; | ||
| // Optional fields — only compare when submitted |
There was a problem hiding this comment.
[P2] Observe description before accepting this definition as persisted
The correction emits description, but this equality helper still ignores it. For a description-only edit containing U+200B, the control/adapter preserve the text and normalize_description rejects it before any write. All fields checked here still equal the old record, so both settlement passes report persistence: ordinary Save closes as success; Save-and-publish may republish the old record and then close. Compare description with backend-compatible accepted storage semantics, including undefined-as-clear, and cover rejected edits/clears through the coordinator so they keep the dialog open and stop I/L advancement. Do not trim prohibited bytes into a false match.
…ttlement The comparator covers displayName, systemPrompt, avatarUrl, runtime/model/ provider, namePool, envVars, and the behavior group — but description was absent. A rejected description-only edit (e.g. U+200B via normalize_description) would return persisted=true, close the dialog as success, and discard the draft. On the publish path the old record could be republished via the retry seam; on combined D+I or D+L saves the I/L writes would advance over the unsaved field. Add description to the comparator using the same canonical semantics as the Rust backend (normalize_description): trim, then blank/absent collapses to null. Do NOT strip prohibited bytes — a submitted U+200B must not be laundered into matching the unchanged observed null. Add 5 regressions to test family 13: rejected description on ordinary save (onDone 0), rejected description on publish path (no retry, no close), rejected description in D+I combined save (I write blocked), blank clear settles as null, and trimmed description matches observed stored value. Mutation: removing the new description compare turns the rejected-edit tests RED. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
…agent-edit * origin/main: feat(desktop): add persistent Bestie experience (#7223) fix(desktop): harden profile batch and thread-reply fetches against relay slowness (#7188) docs(nip-fi): adopt deny-until-TTL and extend enforcement to HTTP ingress (#7254) fix(composer): align wrapped inline chip fragments (#7242) Add operation-aware database pool acquisition metrics (#7195) fix(desktop): keep explicit agent profiles bound to their exact key (#7131) fix(desktop): discover authenticated owned relay agents (#7122) feat(agents): harness-agnostic effort write path and spawn bridge (#4625) chore(db): drop Phase-A NIP-FI relay-side authority ledger (#7221) fix(acp): replace real user name in base prompt mention example (#7250) ci: split CI into reusable workflows (#7168) fix(desktop): retain automatic mentions only in threads (#7144) feat: add databricks fable 5.1 model capabilities (#7213) docs(nip-fi): rewrite NIP-FI as stateless OSS Buzz spec v2 (#7214) feat(relay): add detailed readiness metrics (#7149) feat(desktop): add Pi agent preset (#7208) Signed-off-by: Duncan <dcfd242e557282d7a1e2cf2e6877522682f1e5c6156dc92ca7d90eaedd3b0f95@buzz.block.builderlab.xyz> # Conflicts: # desktop/src/features/agents/ui/AgentInstanceEditDialog.tsx # desktop/src/features/agents/ui/agentInstanceEditPinning.test.mjs
EffortPickerField gained required disabled/value/onChange props in #4625. Wire them through the merged dialog using the same Save-gated pattern as the now-deleted AgentInstanceEditDialog: effortLevel state + effortTouched ref in AgentEditMergedDialog, passed as effortValue/onEffortChange to the instance section, which forwards to EffortPickerField. In useAgentEditMergedSubmit, add effortLevel/effortTouched to AgentEditSubmitState and include effortLevel in the locked update_managed_agent call when touched. When no other I-fields changed (agentInput would be null), synthesise a minimal input from inst.pubkey so the effort write is still atomic. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested
Reviewed head a950da2a3ca2d8c669c1dc830b21e806f759f5c0 against base ac5a18697c8294e9237f505a2e01ec6fc374849a. The single edit surface is the right direction, but six user-visible regressions remain at the rendered-form/save boundaries.
1. [P2] Bind the unlinked prompt input to its editable draft
AgentEditMergedDialogInstanceSection.tsx:523–535
Open an unlinked instance, expand Advanced, and type in System prompt override. The controlled textarea receives inst.systemPrompt, but its change handler writes the dialog’s systemPrompt state. Every edit is therefore rendered back to the stored value, while buildNextAgentFormModel submits the changed state the user cannot see. Bind the control to that same draft state. Add a rendered multi-keystroke edit/Save test that checks both the visible text and submitted prompt.
2. [P2] Do not discard owner-review requests while personas are loading
AgentManagementDialogs.tsx:16–24
Delay list_personas while managed agents and channels have initialized, then deliver a valid owned-agent update request. useAgentManagement accepts it, computes matches from personasQuery.data ?? [] (119–129), and returns a not-found editError (228–238). This new effect immediately dismisses the request and retains its ID in seenRequestIds, so completion of the persona query cannot recover the draft. Distinguish loading/query failure from an authoritative missing target; do not dismiss before successful resolution. Test the actual management host with a deferred persona query.
3. [P2] Keep owner-review identity stable across a rename save
AgentManagementDialogs.tsx:52–57
In an owner-reviewed update, rename the target. matchingPersonas continually resolves the request’s original agentName against the current display name (useAgentManagement.ts:119–129). The awaited mutation invalidation and coordinator readbacks refresh that name after persistence. The match then disappears, this render guard unmounts the editor, and the dismissal effect reports “can only update … by its current name.” This can happen before save/publication settlement finishes, removing the draft/error host even when the coordinator returns a recoverable failure. Resolve the name once, retain the persona ID for the request lifetime, and test rename with delayed or failed post-write settlement. This is a source-traced interleaving, not a live reproduction.
4. [P2] Validate instance credentials against the instance overlay
useAgentEditRuntimeState.ts:332–340
Open a linked instance of a team-managed, provider-backed definition whose required API key is supplied only by the instance overlay, with no global/build/file credential. showDef selects the definition env map here; the instance required-key gate consumes that map at 363–372. The instance API-key control instead writes instanceEnvVars (AgentEditMergedDialog.tsx:889–891). Thus even a populated instance credential remains invisible to validation and Save stays disabled (575–582); the read-only definition cannot be edited to work around it. Derive the instance effective credential map from its overlay with the intended inheritance precedence, independently of D-section validation. Cover both a valid local key and an explicit-empty override.
5. [P2] Do not settle an unverified effort write as successful
agentSaveCoordinator.ts:349–358
Changing only effort produces {pubkey, effortLevel} (useAgentEditMergedSubmit.ts:287–290). If update_managed_agent fails at save_managed_agents (agent_models_update.rs:314–323) but list readback succeeds, observedStateMatchesAgentInput returns true: it checks no effort field. Both this boundary and final settlement discard the failure and close as “saved.” ManagedAgent does not expose effort, and refetchAgentStores never reads the separate config surface. Verify canonical effort before claiming success, or retain an explicitly unverified outcome. Add a production-submit regression with a rejected effort-only write and unchanged successful readback.
6. [P2] Preserve pin-to-inherit effort suppression
useAgentEditMergedSubmit.ts:284–290
For a local pinned agent with a discovered effort picker, choose a different non-null effort, select “Inherit runtime from template,” then Save. The form emits agentCommand: "" to clear the pin, but this block unconditionally appends the touched effort. The existing backend clears effort for inheritance and then applies the explicit effort field (agent_models_update.rs:93–106,197–215), restoring the override that the transition should remove. The deleted base dialog explicitly prevented this with resolveEffortSubmission({ inheritTransition: agentCommandUpdate === "" }). Restore that suppression in merged submit and test the combined picker→inherit→Save transition; a backend rewrite is unnecessary.
Scope and validation
Source-only review of immutable blobs; no PR checkout, build, test execution, or live reproduction. Carl integrated complementary UI/runtime, form/settlement, and Rust persistence/publication reviews. Coverage included library/profile/deep-link/owner-review routing, linked/unlinked/definition-only/team-managed ownership, exact-key identity, save/cancel/reopen, runtime inheritance, partial writes, CAS, and catalog publication/retry. Create-flow unification and vocabulary changes remain excluded; no independent mobile/web runtime validation was performed.
The CAS recovery candidate was rejected after tracing the awaited mutation onSettled cache invalidations; a model-clear setter mismatch was also excluded because no concrete user-impacting trigger was established. The existing effort test checks mount/hide rather than either save transition, and the R6 test mounts the merged dialog directly rather than the management host responsible for findings 2–3. Exit criteria are the six bounded fixes above with production-seam regressions, not another architectural expansion.
…runtime-switch reset IMPORTANT-1: Add effortLevel comparison to observedStateMatchesAgentInput (agentSaveCoordinator.ts) with tri-state semantics — absent submission skips, null clears, string compares against observed column. ManagedAgent now exposes effortLevel from ManagedAgentSummary (Rust types.rs + runtime.rs) so the comparator can detect a backend-rejected effort write rather than closing as success. Regressions: 4 family-14 coordinator tests; mutation (remove compare) turns the 2 rejected-edit tests RED while the 2 settled tests stay GREEN. IMPORTANT-2: Replace raw effortLevel include in useAgentEditMergedSubmit with resolveEffortSubmission — suppresses effort on pin→inherit (agentCommand:"") and on unchanged selections, matching the deleted AgentInstanceEditDialog's PR #4625 semantics exactly. originalEffortLevel threaded from configSurfaceQuery into AgentEditSubmitState. IMPORTANT-3: Add setEffortLevel and effortTouched to RuntimeHandlersInput; call setEffortLevel(null) and effortTouched.current = false in handleRuntimeDropdownChange (useAgentEditRuntimeHandlers.ts) so runtime switches do not carry stale vocabulary values into the next Save. Port the 6 load-bearing mounted production-seam tests from the deleted agentInstanceEditCancelSafety.test.mjs to agentEditMergedCancelSafety.test.mjs against AgentEditMergedDialog: selection+Cancel zero writes, ordinary Save carries effortLevel, rejected Save stays open, pin→inherit suppresses effort, access+effort atomic. Runtime-switch reset covered by hook-level test in agentEditMergedRuntimeReset.test.mjs. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested: five prior contracts remain unresolved
Reviewed head d63ed1e3b96c583da3e5a9027f049b6e7cfc9af7 against base ac5a18697c8294e9237f505a2e01ec6fc374849a, bounded to the six findings from a950da2a3ca2d8c669c1dc830b21e806f759f5c0 and the new corrective delta. The single merged edit surface remains accepted product direction. Prior #6 (pin-to-inherit effort suppression) is fixed in source; #1–4 are unchanged, and #5 is incomplete at the IPC conversion boundary.
1. [P2] Bind the unlinked prompt input to its editable draft
AgentEditMergedDialogInstanceSection.tsx:523–535
Open an unlinked instance, expand Advanced, and type in System prompt override. The controlled textarea receives inst.systemPrompt, but its change handler writes the dialog’s systemPrompt state. Every edit is therefore rendered back to the stored value, while buildNextAgentFormModel submits the changed state the user cannot see. Bind the control to that same draft state. Add a rendered multi-keystroke edit/Save test that checks both the visible text and submitted prompt.
2. [P2] Do not discard owner-review requests while personas are loading
AgentManagementDialogs.tsx:16–24
Delay list_personas while managed agents and channels have initialized, then deliver a valid owned-agent update request. useAgentManagement accepts it, computes matches from personasQuery.data ?? [] (119–129), and returns a not-found editError (228–238). This new effect immediately dismisses the request and retains its ID in seenRequestIds, so completion of the persona query cannot recover the draft. Distinguish loading/query failure from an authoritative missing target; do not dismiss before successful resolution. Test the actual management host with a deferred persona query.
3. [P2] Keep owner-review identity stable across a rename save
AgentManagementDialogs.tsx:52–57
In an owner-reviewed update, rename the target. matchingPersonas continually resolves the request’s original agentName against the current display name (useAgentManagement.ts:119–129). The awaited mutation invalidation and coordinator readbacks refresh that name after persistence. The match then disappears, this render guard unmounts the editor, and the dismissal effect reports “can only update … by its current name.” This can happen before save/publication settlement finishes, removing the draft/error host even when the coordinator returns a recoverable failure. Resolve the name once, retain the persona ID for the request lifetime, and test rename with delayed or failed post-write settlement. This is a source-traced interleaving, not a live reproduction.
4. [P2] Validate instance credentials against the instance overlay
useAgentEditRuntimeState.ts:332–340
Open a linked instance of a team-managed buzz-agent/Anthropic definition whose required API key is supplied only by the instance overlay, with no global/build/file credential. showDef selects the definition env map here; the instance required-key gate consumes that map at 363–372. The instance API-key control instead writes instanceEnvVars (AgentEditMergedDialog.tsx:893–895). Thus even a populated instance credential remains invisible to validation and Save stays disabled (576–584); the read-only definition cannot be edited to work around it. Derive the instance effective credential map from its overlay with the intended inheritance precedence, independently of D-section validation. Cover both a valid local key and an explicit-empty override.
5. [P2] Carry canonical effort through the IPC mapper before comparing it
agentSaveCoordinator.ts:725–728, with the missing conversion at tauri.ts:628–671.
The Rust summary now supplies effort_level, but RawManagedAgent has no corresponding member and fromRawManagedAgent drops it. Both listManagedAgents (tauri.ts:778–781) and updateManagedAgent (1021–1030) use that mapper, so production readback still has effortLevel === undefined regardless of the persisted column. Selecting a non-null effort and successfully saving therefore compares the submitted string to null and falsely reports failure. Conversely, clearing an existing effort with a rejected write compares null to null and can close as saved while the override remains on disk. This is the still-open observed-settlement finding, with a new false-failure consequence introduced by the comparator.
Add the raw field and map it into ManagedAgent.effortLevel; test successful set and rejected clear through raw IPC responses and the real conversion/list/refetch path. The added coordinator tests construct normalized agents directly, while the rendered successful-Save test asserts the outgoing payload/count but not successful settlement, so those assertions do not catch this dropped field.
Closed finding and validation scope
Prior #6 is closed in source: useAgentEditMergedSubmit.ts:299–308 calls resolveEffortSubmission with rawAgentInput?.agentCommand === "", suppressing the explicit effort write when inheritance clears it. The runtime handler also restores the base pending-effort reset. This does not require a backend rewrite or clearing the persisted canonical effort on every runtime switch.
Source-only review of exact immutable blobs and test text. No PR checkout, build, test execution, mutation experiment, or live reproduction was performed. Re-review covered the actual unlinked prompt, linked/team-managed credential gate, owner-request loading/rename host, and effort picker → submit → native persistence → summary → IPC conversion → list/refetch settlement. Previously closed routing/ownership/CAS/catalog contracts were not reopened without new evidence. Create-flow unification, vocabulary changes, unrelated incoming-main features, and independent mobile/web runtime validation remain excluded.
Non-blocking test correction: agentEditMergedRuntimeReset.test.mjs:136,182–208 starts effort at null and changes only the touched flag before switching runtimes. Its final null assertion cannot establish that setEffortLevel(null) reset a non-null draft. Seed a real non-null selection; do not claim a mutation result from that assertion alone.
Exit criteria are the five bounded fixes above with regressions at their production boundaries, not another architectural expansion.
Merges the two agent edit dialogs (
AgentInstanceEditDialog,AgentDefinitionDialog) into a singleAgentEditMergedDialogthat every edit entry point — the agents library, the profile panel, owner review, and therequestOpenEditAgentevent path — reaches throughAgentEditDialog.AgentInstanceEditDialogis deleted,AgentDialog'sinstance-editarm is removed, and theeditPersonaDialogStateexport is gone.Design
seedAgentFormModelbuilds anAgentFormModelfrom the edit context (linked instance, unlinked instance, or definition-only). The model owns every editable field, including the harness pin (harnessInherit/harnessCommand/harnessArgs/acpCommand).emitAgentFormDiffdiffs the seed against the submitted model and routes each changed field to its owning layer, returning{ personaInput, agentInput, policySets }. There is no parallel React state model and no post-emit merge.FIELD_OWNERSmap drives emit routing (emitAgentFormDiff), per-field editability (fieldEditable), and the catalog-publish dirty signal (definitionFieldsDirty). No control decides ownership locally.runAgentSaveCoordinatorapplies definition → instance → local-policy writes in order. After every boundary — success or throw — it re-fetches the store and lets observed persistence decide the outcome: a write that threw after landing on disk counts as persisted and the sequence continues; a write that returned but did not persist counts as failed. A submitted behavior group (respondTo/respondToAllowlist/parallelism) settles as the full-replacement unit the backend writes, so a clear the backend failed to apply cannot false-succeed.dModel/dProvider) and instance-owned (iModel/iProvider) runtime state are separate, with per-owner selection handlers. An instance pin change cannot clear the definition's model or provider, and a definition runtime change cannot touch the instance overlay.respondTodisplays as owner-only (the effective default) and is never coerced to "anyone". Blanking parallelism in a definition-only edit emits an explicit clear rather than silently restoring the stored value.fieldEditabledisables every definition-owned control and the dialog shows a "Managed by team «name»" notice; instance-owned fields, including access, stay editable. A runtime-less definition with a saved model but no provider exposes the provider picker and keeps Save disabled until a provider is chosen.EffortPickerrenders in the merged dialog's instance section, so the reasoning-effort control lives on the same surface as every other editable field rather than in a separate dialog.useAgentEditDeepLinkFocusresolves arequestOpenEditAgentfocus target after the dialog mounts: it expands the collapsed env-key section when the target is a credential field, suppresses the dialog's open-autofocus so it cannot steal focus from a deep-link target, and retries once the target control is present. Focus reachesmodelandenv_keytargets that the pre-merge path left unfocused. TheEnvVarsEditorfocus effect keys on the required-key set's content rather than its array identity, so a content-equal re-render while model discovery and file-config queries settle no longer cancels the pending focus frame and strands theenv_keytarget.Out of scope (follow-up work)
src-taurimodule names)