fix(desktop): exclude archived agents from nest, order regeneration - #5905
Conversation
themiguelamador
left a comment
There was a problem hiding this comment.
I found two correctness/reliability issues that should be addressed before merge:
archive_identityandunarchive_identitysubmit the state-changing event but never schedule nest regeneration. The new archived-agent filtering therefore does not updateAGENTS.mdafter the actual archive/unarchive actions; it can remain stale until an unrelated agent/team edit or application restart. The repair branch schedules the existing best-effort regeneration after each successful submission.NestRegenGate::commitintroduces a productionexpect()on the mutex. Besides violating this repository's no-new-expect()production rule, a poisoned best-effort regeneration lock would panic the desktop process rather than report a regeneration error. The repair branch converts poisoning intoio::Errorand adds a regression test.
Verified repair: 47 nest tests; all 7 identity-archive tests (including the relay-switch race); strict Tauri Clippy with -D warnings; Rust formatting; desktop file-size gate.
Fix commit: https://github.com/Complear/buzz/commit/3d644ae00
Branch: https://github.com/Complear/buzz/tree/review/pr-5905-fix
2dba60e to
5bedb05
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
The archive snapshot trust checks, one-relay capture, archive/unarchive trigger wiring, and poison handling look disciplined, but the new regeneration gate still permits the stale rollback it claims to prevent.
regenerate_nest_context snapshots personas and managed agents before awaiting the relay (desktop/src-tauri/src/managed_agents/nest.rs:727-753). NestRegenGate::commit, however, compares an old task only against last_written, which advances only after a newer generation successfully writes (nest.rs:683-720). This schedule therefore remains possible:
- gen1 snapshots pre-edit state;
- an edit requests gen2, which snapshots post-edit state;
- gen2 fails during relay work or file publication;
- gen1 finishes, sees
last_written == 0, and publishes the obsolete pre-edit roster.
The current regressions cover only the easier case where gen2 successfully commits before gen1 (nest/render_tests.rs:513-579). A newer failed request must still permanently supersede older snapshots for publication purposes.
Please serialize request claiming and publication against one highest-requested watermark (or use an equivalent protocol), so once gen2 is requested gen1 can never publish, even if gen2 fails. A separate atomic watermark check is not sufficient by itself: a new claim can race between the old task’s eligibility check and its synchronous file write. Add deterministic coverage for both a newer-request failure and a claim arriving at the old commit cutover.
I verified this against exact head 5bedb05f57f0b14af99f6a1241a712da1985c595; git diff --check is clean. GitHub had 15 successful checks, 7 skipped, and Desktop Smoke E2E (3) still running at my snapshot. I did not duplicate CI-equivalent suites locally. A focused Tauri test attempt could not execute because the checkout lacks the required sidecar desktop/src-tauri/binaries/buzz-acp-aarch64-apple-darwin; CI's Desktop Core coverage is the broad test evidence here.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Consolidated Royal Court follow-up on the same exact head 5bedb05f57f0b14af99f6a1241a712da1985c595: a second blocker arrived after the initial review.
Blocking: rapid archive state changes can leave kind 13535 stale.
publish_nipia_archival_list builds the relay-signed snapshot with the default whole-second timestamp and calls replace_addressable_event (crates/buzz-relay/src/handlers/side_effects.rs:3119-3145). That replacement path rejects a same-second event unless its random event ID sorts lower than the current one (crates/buzz-db/src/lib.rs:4821-4827, 4874-4888). Therefore a rapid archive then unarchive can update canonical archive state successfully but reject the second snapshot, leaving the archived identity in the authoritative 13535 event until some later mutation happens to publish a winning/newer snapshot. The neighboring DM-visibility snapshot already avoids this exact failure by forcing created_at strictly beyond the prior snapshot (side_effects.rs:3196-3225).
A read-previous / previous + 1 timestamp alone is not concurrency-safe: two publishers can read the same predecessor, build different snapshots, and the transaction's random-ID tie-break can preserve the older canonical view. Please make publication converge from canonical archive state under concurrent writers, for example with a bounded retry that rebuilds after a rejected replacement, or move snapshot construction/publication under an atomic serialization boundary. Add deterministic tests for:
- archive then unarchive within one second; and
- concurrent snapshot publishers where an older canonical view wins the first same-timestamp replacement race, proving the final stored snapshot is rebuilt from current archive state.
This is independent of the NestRegenGate stale-publication blocker in my initial review. Both need resolution.
|
Additional blocker confirmed on exact head
There is also a concurrency wrinkle: only adding
Validation at |
68a8187 to
628f27f
Compare
The managed "Active Agents" table in ~/.buzz/AGENTS.md was rendered from every managed-agent record with no filtering, so archived duplicate instances and records pinned to defunct relays kept appearing under the active relay's header. Two independent, separately-tested predicates now gate each record: - Relay scope: a local check that the record's relay_url equals the active workspace relay (normalized: trim, drop trailing slash, lowercase). Unconditional — there is no fetch to fail open on. - Identity archive: skip pubkeys present in the relay's kind:13535 snapshot. Local records can't tell — they all carry is_active: true (archived *definition*, not identity-archived), so archive truth lives only relay-side. Fails open: an unreachable relay yields an empty set and hides no one. The archive read is async, so try_regenerate_nest spawns the regen as a fire-and-forget task (matching its existing contract) rather than changing its 12 sync call sites. A just-archived agent may linger one regen cycle. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The nest renderer had three defects surfaced in review: Relay-scope filter contradicted the agents-everywhere model. render_dynamic_section dropped records whose stored relay_url differed from the active workspace, but effective_agent_relay_url() deliberately ignores that legacy creation-era field — every agent is eligible on every community, and snapshot-imported records store relay_url empty by design. The filter silently hid valid, runnable agents after a workspace switch or import. Removed the relay-pin predicate; the archive filter is now the only roster gate. The foreign-relay relic records leave the table via record deletion, not code. Detached regenerations could publish stale whole-file state out of order. Each try_regenerate_nest spawned an unconstrained task that snapshotted state, awaited two relay requests, then wrote — so a slow pre-edit generation could overwrite a newer one, deterministically at boot where the fallback-relay regen races the apply_workspace regen. Added NestRegenCoalescer: a monotonic generation is claimed synchronously at request time (encoding call order) and gates the write under one lock spanning the compare-and-write, so a lower-generation task drops its result instead of rolling the file back. nest/tests.rs blew the 1000-line file-size ratchet. Split the renderer, upsert, and new coalescer tests into nest/render_tests.rs. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The archive read re-resolved the workspace relay override twice — once in fetch_relay_self for the NIP-11 signer and again in query_relay for the snapshot — while the rendered footer captured it a third time. A workspace switch between those reads could pair one relay's advertised signer with another relay's snapshot, fail open, and render archived agents as active. Capture the effective relay target (ws + api base) once, before any network work, via capture_relay_target, and thread it through fetch_archived_pubkeys_at (fetch_relay_self_at + query_relay_at) and the rendered footer, so signer, snapshot, and footer all belong to one relay. A deterministic seam test mutates the override between capture and fetch and proves the fetch never crosses relays. Also rename NestRegenCoalescer to NestRegenGate: it is an ordered, latest-write-wins gate, not a work coalescer — superseded generations still perform their relay reads and drop the result at commit. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
Archive/unarchive commands submitted the relay event and returned without refreshing AGENTS.md, so a just-archived agent lingered on the roster until an unrelated edit or app restart. Wire try_regenerate_nest into both commands after a successful submit, matching the ~20 other mutation sites; the regen is fire-and-forget and races the relay's kind:13535 snapshot, so a stale render self-heals on the next cycle. Replace the production expect() on the NestRegenGate commit mutex with a poison-to-io::Error mapping so a poisoned lock degrades to the same warn-and-continue path as any other commit failure instead of panicking the desktop process (root AGENTS.md: no new expect() in production paths). Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The archive/unarchive → AGENTS.md-regeneration wiring was unguarded: deleting either try_regenerate_nest call left the suite green. The commands take AppHandle, which has no test harness in this crate, so extract an AppHandle-free submit_then_regenerate core that fires on_success iff the relay accepts, and have both commands pass the real try_regenerate_nest as that closure. A loopback-relay test drives the seam with a counting hook: accepted archive and unarchive each fire once; a rejected submit propagates the error and never fires. RED-on-revert holds — dropping on_success fails the must-fire assertions; moving it before the ? fails the must-not-fire assertion. Production behavior is intended-identical: submit_event errors still propagate, regeneration stays fire-and-forget, and both commands still return the SubmitEventResponse. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The prior test pinned only the shared submit_then_regenerate seam, so replacing either command's forwarded try_regenerate_nest closure with a no-op left the suite green — either command could silently lose roster regeneration. And the loopback relay assembled its route with ["/ev","ents"].concat() to dodge the events-URL egress inventory scan, teaching source code the exact trick a production site could use to evade the fail-closed NIP-49 guard.
Extract per-command AppHandle-free cores (archive_identity_core, unarchive_identity_core) that build the real 9035/9036 request and forward on_success; the Tauri wrappers pass try_regenerate_nest. Each core has its own loopback-relay regen test, so replacing one command's callback with || {} turns only that test RED. Use a literal /events route and add identity_archive.rs to EVENTS_INVENTORY as a test-only fixture (one occurrence, zero guard calls) so no source evades the tripwire.
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
NIP-IA snapshots used whole-second timestamps, so rapid archive state changes could lose to NIP-16's random same-second event-id tie-break and strand stale state. Advance each snapshot past the current head and retry from canonical archive state when a concurrent replacement wins or changes the set before dispatch. Add a focused archive-to-unarchive regression that verifies the final stored snapshot advances and carries the canonical empty set. Co-authored-by: Mongo <5c25403eab7271f9f94ddd4f2b270e8cac2c92e2c830c51877cca6ec974ffb3f@buzz.block.builderlab.xyz> Signed-off-by: Wes <wesbillman@users.noreply.github.com> (cherry picked from commit 1ad28a4)
Two review findings on the archive/unarchive nest-regen wiring. Wrapper callback was unprotected. The Tauri command wrappers each constructed the regeneration closure themselves (`|| try_regenerate_nest`) before delegating, so the core tests proved a core forwards its callback but never that either production command supplies one — no-oping either wrapper's closure left the whole suite green. Bind regeneration to a `NestRegenTrigger` type the cores invoke instead: the wrapper hands the core its `AppHandle` as the trigger with no closure to construct, so the 'regenerate on success' selection lives inside the cores where the tests traverse it. Each core's `|| regen.trigger()` is now RED-on-revert. Regen gate permitted stale rollback. `NestRegenGate::commit` gated on the highest *written* generation, which advances only when a newer generation successfully writes. A newer request that failed during relay work therefore left an older, stale render free to publish its obsolete roster. Gate on the highest *requested* generation instead, advanced by `claim` under the same lock `commit` reads — so once a newer generation is requested no older one can publish, even if the newer one fails, and a claim cannot race between an older task's eligibility check and its write. Semantic delta: after a newer request fails, nothing publishes until the next trigger. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The adopted monotonic-snapshot fix carried a focused sub-second archive→unarchive regression. Carl's review also required a second deterministic test: concurrent publishers where an older canonical view races the same-timestamp replacement, proving the final stored snapshot is rebuilt from current archive state rather than stranding the stale view. This exercises the post-insert drift check and the rebuild retry — reverting either leaves an unarchived identity in the authoritative 13535. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The cutover regression only claimed a second generation before the older
commit ran; a flawed separate-watermark/separate-write-lock gate passed it.
Add an under-lock hook so the test starts a competing claim while the older
commit holds the shared lock, proving the eligibility compare is atomic with
the write. commit() now delegates to commit_hooked(.., || {}) — identical
behavior, the no-op monomorphizes away.
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The 16-iteration loop only amplified the probability that a spawned stale
publisher read {target} before the unarchive; scheduling could miss the
required ordering entirely. Add a cfg(test) barrier in
publish_nipia_archival_list that holds one publisher after it reads canonical
state and before it replaces the head. The test now deterministically holds
the stale {target} publisher, runs the unarchive and compliant {} publish,
then releases it — proving the post-insert drift check converges. One
orchestrated case replaces the loop.
Co-authored-by: Will Pfleger <pfleger.will@gmail.com>
Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The cfg(test) publish_test_hooks gate was a single process-global slot, so any other publisher-calling test in the same Rust test process could consume the gate armed by the concurrent-publisher regression before the intended stale publisher reached it — under the parallel runner that could hang or exercise the wrong ordering. Scope the gate by CommunityId: after_list_archived only takes a gate whose armed community matches the publisher's tenant. Every test seeds a unique community, so the rapid and owner-archive publishers now pass straight through. Test-only; inert in production. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The cutover regression released the competing claimer, slept 200ms, then asserted the claim had not completed — but a claimer that was never scheduled satisfies that assertion under both the correct single-lock gate and the flawed separate-watermark design, so a starved thread could let the mutant pass. Add an acknowledgement the claimer sends immediately before it calls claim(); the under-lock hook blocks until it arrives, so the negative assertion is a real statement about lock contention rather than an artifact of an unscheduled thread. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
The cutover test used a thread that signalled before calling claim() then slept 200ms; a scheduler pause between the signal and claim() let the separate-watermark design pass, so the regression was probabilistic. Add a cfg(test) try_claim() that non-blockingly takes the exact lock claim() uses and assert from the under-lock hook that it reports the lock held. The correct single-lock design necessarily returns None and the separate-watermark design necessarily returns Some, so no thread, channel, or elapsed time gates the proof. Co-authored-by: Will Pfleger <pfleger.will@gmail.com> Signed-off-by: Will Pfleger <pfleger.will@gmail.com>
628f27f to
cfa1e6c
Compare
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Consolidated Royal Court re-review at exact head cfa1e6c77cd662f82ab8959107bb016e8b98e91f: no remaining blocking findings.
The current head resolves both prior correctness blockers:
NestRegenGateadvances the highest-requested generation synchronously and holds one mutex across eligibility checking and atomic file replacement. An older snapshot therefore cannot publish after a newer request, even if that newer request fails, and a new claim cannot slip into the compare/write cutover (desktop/src-tauri/src/managed_agents/nest.rs:666-777). Deterministic regressions cover both schedules (nest/render_tests.rs:583-668).- Kind
13535publication advances beyond the current head, rebuilds from canonical archive state after rejected replacements, and verifies post-insert state before dispatch. This closes both rapid archive→unarchive and concurrent stale-publisher races (crates/buzz-relay/src/handlers/side_effects.rs:3174-3270).
The surrounding contracts also hold: roster filtering trusts only verified snapshots from the NIP-11-advertised relay signer; signer, query, and rendered footer share one captured relay target; read failures fail open rather than hiding agents; and archive/unarchive trigger regeneration only after relay acceptance (desktop/src-tauri/src/commands/identity_archive.rs:175-285,393-447; desktop/src-tauri/src/managed_agents/nest.rs:783-835).
All four Court reviews converged on clear/ship. I independently fetched the PR ref and confirmed the GitHub API, remote ref, and local review ref all equal the SHA above. git diff --check origin/main...refs/review/pr-5905-head is clean. All current applicable GitHub checks are successful; skipped checks are path/publish jobs. I did not duplicate CI-equivalent suites locally.
The outstanding CHANGES_REQUESTED state comes from reviews against superseded heads; I am leaving a non-approving comment because approval was not explicitly requested.
* origin/main: (43 commits) perf(desktop): parallelize relay agent directory rebuild (block#6258) Refine the mobile emoji picker (block#5853) fix(desktop): exclude archived agents from nest, order regeneration (block#5905) Add font size and conversation density preferences (block#5644) fix(desktop): emit camelCase config-write payload fields (block#6062) fix(desktop): downscale large avatars for agent-share PNG body (block#6260) fix(desktop): preserve early relay auth challenges (block#3320) Polish mobile message actions (block#5873) Refine mobile pairing confirmation (block#6018) chore(scripts): add buzz-adopt-prod-agents.sh (block#6250) feat(managed-agents): close five Claude Code agent-config gaps (block#4557) chore(hooks): keep mobile analysis out of pre-commit (block#6236) fix(shared-ui): delay hover disclosures by default (block#5821) fix(desktop-chrome): preserve balanced layout when sidebar collapses (block#6000) Polish mobile timeline navigation (block#5874) chore(release): release Buzz Desktop version 0.5.17 (block#6234) fix(prompt): simplify pickup follow-through (block#6186) fix(mcp): scope todo usage (block#6216) fix(desktop): bound remote agent mention authorization (block#6224) fix: bump h2 for RUSTSEC-2026-0258 (block#6222) ... Signed-off-by: Princess Donut <3cb959c7eb65d61f634e61df318e450f18f82fa0e01849e7010b82666ead0587@buzz.block.builderlab.xyz> # Conflicts: # desktop/src/main.tsx # mobile/ios/Podfile.lock
Resolves seven conflicts, all in the desktop managed-agent surface, where upstream's "close five Claude Code agent-config gaps" (block#4557) and "exclude archived agents from nest" (block#5905) landed on code the fork had relocated to stay under the file-size ratchet: - types.rs / types.ts / nest/tests.rs: fork had moved the conflicting blocks to record_views.rs, managedAgent.ts, and (upstream) render_tests.rs. Kept the relocated homes and ported upstream's new `effort_level` field into record_views.rs plus the fork's community_relay_url / residual_deployments / waker_enabled fields into render_tests.rs. - readiness.rs / discovery/tests.rs: fork builds fixtures from JSON so new optional record fields don't churn them; kept that over upstream's struct literals. - mod.rs / tauriManagedAgents.ts: both sides added declarations at the same spot; kept both. The merge also pushed two files past the desktop file-size ratchet through accumulated growth on both sides. Split them along the module's existing seams rather than raising the limit: - commands/agents_deploy.rs (1037 -> 536): tests moved to agents_deploy_tests.rs, matching the agent_config.rs idiom. - managed_agents/runtime.rs (1003 -> 981): git credential-helper env moved to runtime/git_credentials.rs. Verified: workspace clippy + fmt, Tauri clippy/fmt/tests (2650 passed), desktop tsc/biome/tests (5169 passed)/build, desktop file-size + px-text guards, mobile format/analyze/tests (1523 passed) + file-size guard. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Junchao Yan <yjc801@gmail.com>
The managed "Active Agents" table in
~/.buzz/AGENTS.mdwas rendered from every managed-agent record with no filtering, so archived duplicate instances kept appearing under the active relay's header. This scopes the roster to identity-active agents and makes regeneration safe under concurrency.Roster filter: identity-archive only
render_dynamic_sectionnow drops only records whose pubkey is present in the relay'skind:13535archive snapshot. Local records can't tell they're archived — they all carryis_active: true(that flag is a definition-archive, not an identity-archive), so archive truth lives only relay-side. The read fails open: an unreachable relay yields an empty set and hides no one.There is deliberately no relay-scope filter.
relay_urlis a legacy creation-era field thateffective_agent_relay_url()ignores — every agent is eligible on every community, and snapshot-imported records storerelay_url: ""by design. Filtering on it would hide valid, runnable agents after a workspace switch or import. Foreign-relay relic records leave the table via record deletion, not code.Regeneration on archive / unarchive
archive_identityandunarchive_identitysubmitted the relay event and returned without refreshingAGENTS.md, unlike the ~20 other mutation sites that calltry_regenerate_nest. A just-archived agent therefore lingered on the roster until an unrelated edit or the next launch. Both commands now trigger a regeneration after a successful submit.Regeneration is bound through a
NestRegenTriggertrait rather than constructing thetry_regenerate_nestcallback at the Tauri-command delegation site. The command cores takeregen: &impl NestRegenTriggerand own the|| regen.trigger()binding; the thin wrappers only pass&app(whoseimplcallstry_regenerate_nest). This puts the regen wiring inside the unit-tested core — aCountingRegendouble proves each core fires exactly one regeneration — instead of an untestable seam where a wrapper could silently lose the refresh while the suite stayed green. The regen races the relay'skind:13535snapshot update, so it's best-effort and fail-open — a stale render self-heals on the next cycle.Ordered regeneration
try_regenerate_nestpreviously spawned unconstrained tasks that each snapshotted state, awaited two relay requests, then wrote — so a slow pre-edit generation could overwrite a newer one. Boot made this deterministic: the boot regen races theapply_workspaceregen, and the fallback-relay render could finish last.NestRegenGatefixes this with a singlehighest_requestedwatermark. A monotonic generation is claimed synchronously at request time (encoding call order) and advances the watermark under one lock; the spawned task carries its generation and, at commit, reads the watermark under that same lock — the compare-and-write is atomic with no await held across it. A generation whose number is below the current watermark drops its result instead of rolling the file back.Gating on highest-requested rather than highest-written is the load-bearing choice: if a newer generation is requested but then fails its relay reads, an older in-flight generation must not publish its now-obsolete roster. Behavior delta: once a newer regeneration has been requested, no older generation will ever write; if that newer generation fails, the file is left as-is and self-heals on the next trigger rather than regressing to a stale snapshot. This is an ordered, latest-request-wins gate — not a work coalescer: superseded generations still perform their relay reads and drop the result at commit time.
The gate's commit lock is acquired with a poison-to-
io::Errormapping rather thanexpect(), so a poisoned lock degrades to the same warn-and-continue path as any other commit failure instead of panicking the desktop process (a best-effort housekeeping write must never take down the app).One relay target per regeneration
A regeneration read the workspace relay override three times — the NIP-11 signer in
fetch_relay_self, the snapshot query inquery_relay, and the rendered footer — so a workspace switch mid-flight could pair one relay's advertised signer with another relay's snapshot, fail open, and render archived agents as active.capture_relay_targetnow resolves the effective relay (WebSocket + HTTP API base) once, before any network work, andfetch_archived_pubkeys_atthreads that single target through both the NIP-11 fetch and the/query; the footer renders the same target. Signer, snapshot, and footer always belong to one relay.Monotonic archival snapshot publishing
publish_nipia_archival_liststamped itskind:13535snapshot with a whole-secondcreated_at. A rapid archive→unarchive within the same second produced two events whose NIP-16 replaceable-event tie-break (higher event id wins) could strand the older, stale archive state as canonical. Publishing now uses a bounded retry that re-reads current archive state and rebuilds the snapshot each attempt, so the published list reflects the latest intended state rather than a fixed same-second race loser. After the attempt budget (8) is exhausted the publisherbail!s; the sole caller treats that as awarn!side effect and continues, matching the surrounding best-effort submit path.Test-file split
The renderer,
upsert_managed_section, and the regeneration-gate tests moved tonest/render_tests.rsso each test file stays under the repository's 1000-line ratchet.