perf(desktop): publish mention sends before waking agents - #7154
Conversation
Mentioning a stopped or undeployed managed agent used to block the send on the full agent start/deploy round-trip, so the message publish waited seconds behind process spawn. Detach the start instead (option 2, steps 1 and 2): the publish proceeds immediately and the wake runs fire-and-forget off the critical path. - useMentionSendFlow: start/deploy for mentioned managed agents now runs detached via a startAgentDetached callback; failures surface as a post-send toast instead of failing the send. Membership attach and access-policy relay writes stay synchronous so the harness's first kind-39002 read still sees the channel. A pending background start no longer gates the composer's next send. - attachManagedAgentToChannel / createChannelManagedAgent accept an opt-in detachedStart hook; other callers keep the awaited behavior. - Replay floor: the send captures its timestamp and passes it through start_managed_agent -> spawn_agent_child as BUZZ_ACP_REPLAY_FLOOR, so the spawned harness's startup watermark replays back past the just-published message. buzz-acp clamps the floor to now-15min (and ignores future floors) before deriving startup_watermark. - E2E: the five "starts it before sending" mentions specs now pin the new ordering (publish lands while start_managed_agent is held pending behind an injected delay) and the replayFloorUnix payload; new spec covers the post-send start-failure toast; the channels DM spec now pins that a failed start no longer drops the expanded DM. Step 3 (durable catch-up via event_mentions) and the revalidation dedupe / NIP-11 caching are intentionally left for follow-ups. Verified: cargo test -p buzz-acp --lib (835 passed), clippy + fmt on buzz-acp and desktop/src-tauri, desktop tsc --noEmit, desktop unit tests (5799 passed), and the mentions + channels Playwright smoke specs against a pnpm build:e2e bundle. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
… self For the common already-running-agent send, the ~1s of latency was not the agent wake but two identical revalidateMentionPubkeys passes (~8 sequential relay round-trips), each fronted by an uncached NIP-11 HTTP GET. This lands the independent quick win from the faster-agent-sends analysis on top of the publish-first change. - useMentionSendFlow: the publish-boundary pass now reuses the pre-side-effect pass's admitted result on the immediate path, and only re-validates when a deferred wait (background media upload, link-preview settlement) separated the two — preserving the #5681 authorization boundary where revocation can actually race the publish. Inputs are already normalized/deduped, so the substitution is behaviorally identical on the fast path. - fetch_relay_self_at: NIP-11 `self` lookups are now cached per relay URL in AppState for 5 minutes. Only verified Some values are cached; non-2xx responses and missing/malformed `self` stay retryable so an outage is never pinned for the TTL. Keying by URL keeps community switches from ever serving another relay's identity. - E2E: the two specs pinning revalidate_relay_agents at +2 per send now pin +1; a new spec pins the deferred-upload path still revalidating at the publish boundary (revocation injected mid-upload strips the p tag, +2 calls). Three new Rust tests pin cache hit, non-success no-cache, and TTL-expiry refetch. Verified: cargo test -p buzz-lib --lib (3008 passed), clippy + fmt on desktop/src-tauri, desktop tsc --noEmit, biome check, desktop unit tests (5799 passed), and the full mentions Playwright smoke suite (80/81; the one failure is the pre-existing under-load flake in the publish-first provider-deploy spec, green in isolation) plus a 3x stress rerun of the dedupe-affected specs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…e after send-path side effects Address the two review warnings on the publish-first mention-send stack (f3df4be4f, f0a9915b5), keeping its design intact. 1) Provider replay-floor gap — fixed via the preferred route: thread the floor into the deploy payload. start_managed_agent's provider branch now passes replay_floor_unix through deploy_to_provider, which injects it into the payload REBUILT after the deploy lock as launch.policy_env.BUZZ_ACP_REPLAY_FLOOR — the same env var a local spawn sets — so a cold remote harness's startup watermark replays back past the already-published triggering mention instead of booting blind to it. No synchronous fallback was needed: launch.policy_env already transports per-spawn env (effort level, model, session title) to providers, so the deploy handoff carries the floor cleanly. Any same-named launch.env key is stripped when a caller floor is present (that tier later-wins remotely); the floor is invocation state, never persisted, so redeploys without one never inherit a stale floor. Other deploy callers (create flow, access reconciliation, inbound personas) pass None and are unchanged. 2) Revalidation-at-publish window: the single-pass dedupe now reuses the pre-side-effect authorization pass only when nothing separated it from the publish. completeSend tracks whether an awaited relay round-trip actually ran in between — DM expansion (onPrepareSendChannel), managed-agent access-policy or membership attach writes (reported by ensureManagedAgentMentionsReady; a matching policy that returns the same record does not count), or active-huddle enrollment (sync_agents_to_active_huddle's matched_active_huddle; with no active huddle it returns before touching the relay) — and re-validates at the publish boundary when any did. The common send (member agent, no expansion, no huddle) stays single-pass. 3) startAgentDetached now depends on startAgentMutation.mutateAsync (stable) instead of the whole mutation object (fresh each render, repo gotcha #6), keeping the downstream useCallback chain reference-stable. Tests: five Rust unit tests pin the payload injection (policy_env ride, case-insensitive launch.env shadow strip, None passthrough, tolerance for missing launch/policy_env); the in-channel provider E2E spec now also pins replayFloorUnix on the detached deploy invoke. New E2E specs pin publish-boundary revalidation for the attach path (revocation injected while a delayed membership write holds the publish open strips the p tag; +2 passes with no update_managed_agent) and for a live huddle on the channel (+2 passes, +1 huddle sync); the managed relay-agent DM-expansion spec pins +2; the two fast-path specs keep pinning +1. Verified: cargo test --lib on desktop/src-tauri (3013 passed), clippy -D warnings + fmt, desktop tsc --noEmit, biome, desktop unit tests (5799 passed), full mentions Playwright smoke (81/83 — both failures are pre-existing under-load flakes, green in isolation), a 3x stress rerun of the new specs, and the five DM-expansion channels specs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…path The send path had zero logging on either side of the bridge, so "the send still feels slow after publish-first" could only be answered by guessing which awaited step dominated the spinner. Add info-level, per-send-click timing on both halves so one test run attributes the latency. - New `sendPerfLog.ts`: `createSendPerfTimer` wraps each awaited step and emits one `[send-perf] completeSend` summary. `useMentionSendFlow`'s `completeSend` — the whole spinner window — now times revalidate1, managedAgentsLookup, prepareSendChannel, ensureAgentsReady, huddleSync, resolvePreviewTags, revalidate2 and publish, alongside the flags that decide which of them run at all: channelType, mention/addressed-agent counts, isReply, hasAttachments, hasLinkPreviews, wroteRelayState, matchedActiveHuddle, detachedStarts, and which trigger (upload / linkPreviews / relaySideEffects) fired the second revalidation. The summary emits from the `finally`, so a send that bails early still reports the steps it reached. - New `send_perf.rs`: `log` + a `Phase` stopwatch, plus a `log_send_perf` command that mirrors the frontend summary into the same stderr stream. WKWebView drops console output logged before a Web Inspector attaches, and `tracing::info!` is silent here (the desktop binary installs no subscriber), so a plain `just dev` terminal is the only place both halves of one send can be read in order. A missing `[send-perf]` line also proves the running app predates this build. - Backend phases, same `buzz-desktop: send-perf:` prefix: the directory rebuild (relay-self lookup, membership query + page count, concurrent 10100/kind-0 batches, managed-policy batch, and whether the caller was the send path or a full autocomplete rebuild); NIP-11 `self` cache hit vs miss with the fetch cost, which directly verifies that cache; the send command split into event build vs submit; the shared submit split into rate-limiter wait vs HTTP round trip — the limiter being the one hidden sleeper nothing previously exposed; and `start_managed_agent` entry with its replay floor, so the stream shows the detached wake landing after the publish. - `detachedStart` is routed through a counting wrapper so the summary reports whether the send fired an agent wake at all. - Two Rust tests pin `Phase` resolution and monotonicity; six JS tests pin value passthrough, recording on a thrown step, step-name accumulation, fact merging, idempotent finish, and the total spanning its steps. The fast-path mentions spec now also pins exactly one `log_send_perf` per send, and the E2E bridge mocks the command. Behavior is unchanged: the revalidation-trigger refactor evaluates the same three conditions in the same order, and the probe swallows its own errors so it can never fail a send. Note: the repository file-size ratchet is red on this branch for app_state.rs, agents.rs, managed_agents/runtime.rs and useMentionSendFlow.ts. All four were already over before this change; this commit keeps identity_archive.rs under the limit it would otherwise have crossed, but does not split the pre-existing four. Verified: cargo test --lib on desktop/src-tauri (3015 passed), clippy -D warnings + fmt, desktop tsc --noEmit, biome, desktop unit tests (5805 passed), and the mentions + channels Playwright smoke suites (171/172 — the one failure is the pre-existing under-load flake in the publish-first provider-deploy spec, green 3x in isolation), plus a 3x stress rerun of the spec carrying the new assertion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
`spawn_agent_child` wrote BUZZ_ACP_REPLAY_FLOOR near the top of the command build, but the fully-layered user env (`descriptor.env`) is written last — "so user-explicit values win over Buzz-set env" — and the key is not in RESERVED_ENV_KEYS. A persona/global/agent env entry named BUZZ_ACP_REPLAY_FLOOR therefore overrode this send's floor, and defeated the `env_remove` on the None path too. A saved future-dated value clamps to `now` in `startup_watermark_with_floor`, so the harness booted blind to the just-published mention that triggered the spawn — the precise failure the floor exists to prevent. This is the same shadowing that `apply_replay_floor` already strips from `launch.env` for provider deploys and pins with `caller_replay_floor_strips_user_env_shadow`. Fixed via the post-loop write rather than a reserved key, because it reproduces the provider path's semantics exactly: - New `apply_replay_floor_env` in `runtime/metadata.rs`, called after the `descriptor.env` loop alongside `apply_effort_env` (B5) — a caller floor wins over any user-supplied entry. With no caller floor the key is left as `descriptor.env` wrote it, matching the provider payload where a floorless deploy passes a user `launch.env` value through. - The pre-loop write becomes an unconditional `env_remove`, so the ambient parent-process value is stripped on both paths (previously only the None path cleared it) before user env and the caller floor are layered on. `None` can no longer inherit the floor from the environment Desktop itself was launched with. - Adding the key to RESERVED_ENV_KEYS was the alternative; it was not taken because that list is deliberately narrow and security-scoped, and it would additionally reject the key at env save time, diverging from the provider path's passthrough on a floorless deploy. Also lifts the env var name into a shared `REPLAY_FLOOR_ENV_VAR` const consumed by both the local spawn and `provider_deploy::apply_replay_floor`, matching how SESSION_TITLE_ENV_VAR and EFFORT_LEVEL_ENV_VAR are named from one place, so the two halves cannot drift. Four unit tests pin the contract: caller floor wins over a user env collision, user value survives with no caller floor, the ambient strip holds when neither supplies one, and a caller floor re-asserts after the strip. The behavior is backend-only, so the existing E2E specs pinning `replayFloorUnix` on the invoke payload are unaffected. The repository file-size ratchet stays red on this branch for app_state.rs, agents.rs, managed_agents/runtime.rs and useMentionSendFlow.ts. All four were already over before this change; runtime.rs is unchanged at +27 lines here (the new helper and its tests live in runtime/metadata.rs), so this commit adds nothing to the ratchet and does not split the pre-existing four. Verified: cargo test --lib on desktop/src-tauri (3019 passed), clippy -D warnings on --all-targets, cargo fmt --check, and the desktop file-size ratchet re-run to confirm runtime.rs holds at its inherited delta. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
The publish-first wake called `startManagedAgent` with no `expectedRelayUrl`/`expectedSignerPubkey`, so `bind_expected_relay_scope(None, …)` let the backend resolve whatever workspace relay and signing identity were current at execution time. Awaiting the start used to bound that window to the send; detaching it means the call outlives the send, the channel, and — since community switching only remounts the React subtree — the community itself. A user who mentions a stopped agent and immediately switches communities could have it spawned or deployed against the new tenant's relay, carrying the old community's replay floor. `submitProjectAgentMessage` passes both values for exactly this outlives-its-caller reason. - New `useDetachedAgentStart`: captures the active community's relay URL and the identity pubkey per render and passes them with every detached start, so `start_managed_agent` fails closed when either has moved. Both are captured because a switch mutates the relay and the signing keys under separate locks — pinning only the relay would still let the new identity act for the old tenant. Capture is per render, never re-read after the switch it guards against: a send in flight holds the callback from the render that fired it. - The relay URL is handed over verbatim rather than through the shared `normalizeRelayUrl` (which lowercases for storage keys) — the backend's `relay_http_base_url` comparison is case-sensitive past the scheme, so a stored `wss://Relay.Example` would otherwise mismatch forever and refuse every wake. The signer check is case-insensitive, so that side is canonicalized. - Scope-mismatch failures get their own toast detail. The backend's message ends in "not sent", which is wrong here: publish-first means the message did publish and only the wake was refused. - `startAgentDetached` moves out of `useMentionSendFlow.ts` into the new hook, which also drops that file by 21 lines. Two unit tests drive the real hook under the real `CommunitiesProvider`: one pins the scope (and the surviving replay floor) on a normal start, using a mixed-case relay URL to pin the verbatim handoff; the other captures the callback, switches community, and asserts the stale wake still names the community it was fired in. Both fail without the fix. The in-channel publish-first E2E spec now also pins both scope fields on the detached invoke, and a new spec moves the active community while the start is held behind an injected delay and asserts the message stays published while the wake is refused with the reworded toast. The repository file-size ratchet stays red on this branch for app_state.rs, agents.rs, managed_agents/runtime.rs and useMentionSendFlow.ts. All four were already over before this change; this commit shrinks useMentionSendFlow.ts (1098 -> 1077) and adds nothing to the other three. Verified: desktop tsc --noEmit, biome check over src + tests, desktop unit tests (5807 passed; the two new tests re-run green after the final verbatim- relay-URL edit), and the full mentions (84) and channels (89) Playwright smoke suites against a pnpm build:e2e bundle, plus a 3x rerun of the new fail-closed spec. Both new E2E assertions were confirmed to fail with the scope threading removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
`applyReusableAgentAccessPolicy` signalled "this hit the relay" by returning a different object than it was handed: a matching policy returned the caller's `agent`, a diverging one returned the fresh record from `updateManagedAgent`. `useMentionSendFlow` read that with `readyAgent !== agent` to decide whether an awaited relay round-trip separated its pre-side-effect mention-authorization pass from the publish, and therefore whether to revalidate at the publish boundary (#5681). The contract held today, and the safe failure direction (a gratuitous new object) only costs a redundant pass — but the unsafe direction is silent: a future in-place cache update that writes to the relay and returns the caller's object would skip the publish-boundary revalidation, and nothing pinned the convention. Make the signal load-bearing by construction. - `applyReusableAgentAccessPolicy` now returns `{ agent, wrote }` (`ApplyReusableAgentAccessPolicyResult`), with `wrote` set from whether `updateManagedAgent` actually ran rather than from anything about the returned record. Its doc comment names the send-path consumer so the flag is not mistaken for incidental. - The send path destructures `{ agent: readyAgent, wrote }` and sets `wroteRelayState` from `wrote`; the existing-member branch supplies `{ agent, wrote: false }`. Behaviour is identical — for the current implementation the two signals agree case for case. - `provisionChannelManagedAgent`'s two reuse branches destructure the agent out; they never consulted the identity signal. Three unit tests pin the contract in a new `channelAgents.accessPolicy.test.mjs`: a matching policy reports `wrote: false`, returns the input agent, and issues no command; a diverging policy reports `wrote: true` and invokes `update_managed_agent` with the resolved policy; and a write whose response is content-identical to the input still reports `wrote: true`, which is the case an identity or content comparison would miss. The repository file-size ratchet stays red on this branch for app_state.rs, agents.rs, managed_agents/runtime.rs and useMentionSendFlow.ts. All four were already over before this change; useMentionSendFlow.ts holds at its inherited 1077 lines here, so this commit adds nothing to the ratchet and does not split the pre-existing four. Verified: desktop tsc --noEmit, biome check over src + tests (exit 0), desktop unit tests (5810 tests, 5804 passed; the 5 failures are the pre-existing inboxReopenNavigation and useRetainedProjectGitViews loader failures, present on origin/main), and the full mentions (84) and channels + agent-access-warning (93) Playwright smoke suites against a pnpm build:e2e bundle. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
Awaiting the start used to make a duplicate wake unreachable, via three
things stacked up: the start ran inside `completeSend`; the mutation's
`isPending` fed `isPreparingMentionSend`, a hard early return in the
composer's send handler, so *no* send could begin while one was in
flight; and by the time that gate lifted `onSuccess` had written the
running/deployed record into the query cache, so the next send read
`running` and never re-fired.
Publish-first removed the first two by design, and that breaks the
third: the cache is only updated on success, so for the whole in-flight
window `getManagedAgentsByPubkey` keeps returning the stale `stopped` /
`not_deployed` record and `ensureManagedAgentMentionsReady` fires again.
The window is longest exactly where it matters — a cold local spawn or a
first remote deploy is seconds long. Reachable by two quick sends in one
composer (`@fizz do X` → `@fizz also Y`), or by two composers at once
(channel, thread panel, `NewMessageScreen` each hold their own
`useMentionSendFlow`).
Implements option A: a module-level in-flight map in
`useDetachedAgentStart`, the single callback every path funnels through
(including the `attachAgentMutation` and persona-create paths that take
it as `detachedStart`).
- Keyed by `(scoped relay URL, normalized pubkey)`, mirroring the
backend's own runtime pair key, so a wake in one community can never
suppress one in another — the same tenant boundary `expectedRelayUrl`
already enforces. Module-level rather than a ref because the
cross-composer overlap is one of the two cases worth collapsing; a ref
dies on remount and would miss it.
- No synchronisation is needed or possible: JS is run-to-completion and
the check, the call and the registration sit in one synchronous block
with no `await` between them. The real hazards are elsewhere — the
`delete` is in `finally`, not `then`, so a failed start does not latch
the agent for the session, and it is identity-guarded so an A→B→A
switch (where `resetDetachedAgentStarts` cleared the map and a newer
start re-registered the key) cannot drop the newer entry.
- Suppressing rather than queueing a re-run is correct because the wake
is per-agent, not per-message: the first start's replay floor predates
the second message and the floor is a lower bound, so one harness boot
covers both. It also collapses two failure toasts into one, whose
wording ("your message was sent, but the agent may not respond") is
accurate for both messages.
- `resetDetachedAgentStarts()` is registered in `resetCommunityState()`
per the module-singleton rule, and is semantically right: those starts
belong to the community being left and would fail closed at the
backend's scope assertion anyway.
- The callback now returns whether it fired, and `countDetachedStart`
counts only real fires — otherwise the send-perf summary this branch
added for attributing latency would report wakes that never happened.
What this does not cover: the Agents-panel Start button, the restore
path, and inbound-persona deploys all call `startManagedAgent` outside
this hook. Closing that class needs backend coalescing (a per-pubkey
deploy epoch snapshotted before the deploy lock, so an explicit redeploy
still forces) — deferred as its own change. One residual on the covered
path: if a second send updates a provider agent's access policy while
the first deploy is in flight, the suppressed deploy would have carried
the newer policy; `apply_deploy_result` only clears
`provider_policy_pending` on a matching payload and
`reconcile_on_workspace_apply` retries pending records, so it self-heals
at the next workspace apply, just later.
Five unit tests pin the contract, each confirmed to fail under the
mutation it guards: a second wake for the same agent is suppressed while
the first is held open (fails with the map removed); different agents in
one window are not collapsed (fails with a relay-only key); the same
agent in another community is not suppressed (fails with a pubkey-only
key); and a wake fires again after the previous one resolves *or*
rejects (both fail without the `finally` delete). A new E2E spec sends
twice with the same mention behind the 45s `startManagedAgentDelayMs`
and asserts both messages publish while `start_managed_agent` stays at
exactly one call — also confirmed to fail (2 vs 1) with the map removed.
The repository file-size ratchet stays red on this branch for
app_state.rs, agents.rs, managed_agents/runtime.rs and
useMentionSendFlow.ts. All four were already over before this change;
useMentionSendFlow.ts holds at its inherited 1077 lines here, so this
commit adds nothing to the ratchet and does not split the pre-existing
four.
Verified: desktop tsc --noEmit, biome check over src + tests (exit 0),
desktop unit tests (5815 passed, 0 failed), and the full mentions (85)
and channels (89) Playwright smoke suites against a pnpm build:e2e
bundle — 2 mentions specs failed under load and are green in isolation
(the inline-code-span literal and system-agent-avatar-stack specs, both
untouched here), plus a 3x stress rerun of the new spec.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
`useDetachedAgentStart` fell back to `undefined` for both halves of its
tenant scope when the active community or the identity query had not
resolved. The backend reads a missing `expectedRelayUrl` /
`expectedSignerPubkey` as "no assertion" (`assert_expected_relay_scope`
returns `Ok(())` for `None`), so in that window the wake fired fully
unscoped — silently degrading the fail-closed guarantee the hook exists
to provide, and collapsing its dedupe key to a relay-less one shared
across communities. A blank stored relay URL had the same effect: the
backend discards a whitespace-only scope exactly as it discards a
missing one, and only `""` was screened out here.
Refuse the wake instead of firing it:
- Both scope values are checked before anything else in the callback; a
missing one returns `false` (so the send-perf summary counts no wake)
and raises the toast. Emptiness is now judged on the trimmed relay
URL, matching the backend, while the value still reaches it verbatim
so the case-sensitive comparison holds.
- Deferring until the query resolves was the alternative and is wrong
here: reading the scope after the send is exactly the post-switch
re-read the per-render capture rules out. Refusing is recoverable —
the user's next send re-fires the wake.
- The refusal toast reuses the failure path's wording via a shared
`warnAgentMayNotRespond`, because publish-first means the message went
out in both cases and the user is owed the same warning; only the
trailing detail differs ("Buzz is still connecting to this community
— mention the agent again in a moment.").
Three unit tests pin it, each confirmed red without the guard: a wake
fired while `get_identity` is held open makes no `start_managed_agent`
call, reports that it fired nothing, warns the user, and still fires
fully scoped once the query lands; a wake with no active community is
refused; a whitespace-only relay URL is refused rather than passed as no
assertion.
The harness's "wait one tick for the identity query" also became
load-bearing with this change — an under-wait now reads as a phantom
suppression bug rather than passing on an unscoped call — so
`renderDetachedStart` waits on the rendered identity value instead. That
under-wait was already happening: the existing dedupe test fails against
the fix until the harness waits properly.
The repository file-size ratchet stays red on this branch for
app_state.rs, agents.rs, managed_agents/runtime.rs and
useMentionSendFlow.ts. All four were already over before this change;
this commit touches none of them.
Verified: desktop tsc --noEmit, biome check over src + tests, desktop
unit tests (5818 passed, 0 failed), and the mentions (86) and channels
(89) Playwright smoke suites against a pnpm build:e2e bundle.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Matt Toohey <contact@matttoohey.com>
…tion-2 Signed-off-by: Matt Toohey <contact@matttoohey.com>
The send-path work pushed four files past the repository file-size gate. Extract one cohesive unit from each rather than raising a ceiling: - `runtime.rs` → `runtime/setup_payload.rs`: the readiness → payload JSON → `BUZZ_ACP_SETUP_PAYLOAD` write, as `apply_setup_payload_env`. - `commands/agents.rs` → `commands/agents_create_fields.rs`: the pure create-request field validators/resolvers. - `app_state.rs` → `app_state_accessors.rs`: the inherent `AppState` lock accessors, leaving the struct, builder, and identity resolution. - `useMentionSendFlow.ts` → `useEnsureAgentMentionsReady.ts`: the mentioned agent access-policy / membership / detached-wake reconciliation. Pure moves. The one behavioral difference is that the new hook depends on the stable `mutateAsync` rather than the whole mutation object, so `ensureManagedAgentMentionsReady` no longer changes identity every render. Signed-off-by: Matt Toohey <contact@matttoohey.com>
🔐 Codex Security Review
|
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES at exact head 00de55e019fd65ccc9026981e35a79872d758e19 (base 4a9de1a3a121285ef475d630b2b5764044c02cde).
Risk: high — this changes the authorization and lifecycle ordering of relay publication, managed-agent deployment, and cross-community UI failure delivery.
Blocking findings
-
Publish-boundary mention authorization can go stale across unbounded awaited work.
desktop/src/features/messages/ui/useMentionSendFlow.ts:378-394admits mention pubkeys before managed-agent lookup; readiness/persona work andsync_agents_to_active_huddlethen suspend at:436-479. The second authorization pass at:531-543is conditional on upload/link work or a command reporting relay side effects, so a slow lookup, store lock, provider lookup, or no-active-huddle IPC can span membership revocation and still publish the previously admittedptag. The base path revalidated unconditionally.Author action: retain the latency optimization without weakening freshness: place authorization immediately before a provably suspension-free publish boundary, or revalidate whenever awaited work follows admission. Add a causal regression that holds a no-relay-write/no-active-huddle leg, revokes membership, and proves the outgoing
ptag is stripped; mutation-disable the second pass and require failure. -
The implementation can claim “your message was sent” before publication, including when publication fails.
completeSendawaits readiness atuseMentionSendFlow.ts:436-445; readiness starts the detached agent atdesktop/src/features/messages/ui/useEnsureAgentMentionsReady.ts:116-131; relay publish does not begin untiluseMentionSendFlow.ts:552-561. An immediate start rejection can therefore emit the success-worded warning fromdesktop/src/features/agents/hooks/useDetachedAgentStart.ts:50-59,144-152before relay acceptance. If publish then rejects, the composer is restored atuseMentionSendFlow.ts:627-635, but the false success claim remains and an agent may have been woken for a message the relay never accepted.Author action: queue the scoped wake and fire it only after successful relay publication, or otherwise condition both wake and warning on confirmed acceptance. Add deterministic wake-rejection + publish-rejection coverage proving no “message was sent” warning and no orphan wake.
-
A stale wake failure can leak community A’s agent context into community B. Community reset clears only the dedupe map (
desktop/src/features/community/hooks/useCommunityInit.ts:55-85); it neither cancels nor fences the detached promise. Its catch still toasts A’s agent name/backend detail fromuseDetachedAgentStart.ts:50-59,144-152, while the global toaster remains mounted across the switch (desktop/src/main.tsx:81-105). The new E2E arounddesktop/tests/e2e/mentions.spec.ts:2797expects the stale post-switch warning and mutates storage rather than exercising the real community transition.Author action: bind warning delivery to the captured community and signer, suppress UI delivery after either changes, and add a real A→B UI-transition regression proving no A agent name or startup detail appears in B.
-
Clearing tenant-keyed in-flight dedupe on every switch permits duplicate remote deployment after A→B→A. The map is already scope-keyed (
useDetachedAgentStart.ts:26,131), butuseCommunityInit.ts:55-83clears A’s still-running entry. Returning to A allows another send to queue a second deployment while the first is active. The backend per-pubkey mutex serializes rather than coalesces this work, and already-deployed agents may be deployed again (desktop/src-tauri/src/commands/provider_deploy.rs:19-23,54-65), potentially replacing the harness that just came up for the first message.Author action: preserve safely scope-keyed in-flight entries across community reset, or coalesce them behind a scope-aware backend epoch. Add A→B→A coverage with deployment #1 held: the second A send must not invoke or queue deployment #2, while success/rejection must still unlock a later retry.
Verification owner: author for the four deterministic regressions and fixes; reviewer for exact-head rerun and ordering/ownership trace.
Validation at the reviewed head
- Full Desktop JS package: 5,856/5,856 passed in both assigned lanes.
git diff --check 4a9de1a...00de55e: passed.- Full
cargo test -p buzz-acp: 903 passed / 1 timing failure (keepalive_resets_idle_past_deadlineat 111 ms); isolated rerun passed. No causal link to this PR was established. - A separate full smoke attempt hit the 600-second reviewer harness timeout after 374 displayed passes with no test failure shown.
- The full local Tauri package was not completed because the fresh worktree lacked
desktop/src-tauri/binaries/buzz-acp-aarch64-apple-darwin.
Confidence gaps: native Desktop/AX behavior and full Tauri completion remain reviewer/tooling-owned; hosted Desktop Core, one smoke shard, and integration shards were still running at the latest lane snapshots. These are not additional author defects. Source review found no concrete content/privacy leak in the new performance instrumentation.
Any new head invalidates this verdict.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
REQUEST CHANGES on 00de55e019fd65ccc9026981e35a79872d758e19 against 4a9de1a3a121285ef475d630b2b5764044c02cde.
I found four author-actionable races in the publish-first lifecycle:
-
[P1] The UI can claim “your message was sent” before relay acceptance, and even when publish fails.
completeSendawaits readiness atdesktop/src/features/messages/ui/useMentionSendFlow.ts:436-445; readiness immediately invokes detached start atuseEnsureAgentMentionsReady.ts:116-131; relay publish begins only later atuseMentionSendFlow.ts:552-561. An immediate wake rejection therefore reaches the unconditional success-worded toast atuseDetachedAgentStart.ts:50-59,144-152before relay acceptance. If publish then rejects,useMentionSendFlow.ts:627-635restores the composer, but the user has already received the false success claim, and an agent may have been woken for a message the relay never accepted. Required: queue the scoped wake and fire it only after successful publish (or equivalently gate wake and warning on confirmed acceptance), plus a deterministic wake-rejection + publish-rejection regression asserting no success warning and no orphan wake. -
[P1] A detached failure from community A can disclose A’s agent context after switching to B. Community teardown only clears dedupe state (
useCommunityInit.ts:55-85); it neither cancels the promise nor fences its UI. The catch still displays A’s agent name/backend detail (useDetachedAgentStart.ts:50-59,144-152), while the global toaster survives the switch (desktop/src/main.tsx:81-105). The added test atdesktop/tests/e2e/mentions.spec.ts:2797+expects the stale warning and mutates storage rather than exercising the real switch. Required: bind warning delivery to captured community + signer, suppress it once either changes (logging may remain), and add a real A→B UI regression proving no A identity/detail appears in B. -
[P2] The fast path reuses stale mention authorization across awaited work. The first pass admits pubkeys at
useMentionSendFlow.ts:378-394, before awaited managed-agent lookup, readiness/persona work (:436-445), andsync_agents_to_active_huddle(:469-479). The second pass runs only for upload, previews, or a command reporting relay work (:531-543). A slow cache/store/provider/no-active-huddle leg can therefore suspend while membership is revoked, yet publish with the old admitted set. The base path revalidated unconditionally. Required: ensure authorization is fresh at the publish boundary—move admission after all suspending work or revalidate whenever awaited work follows it—and add a causal delayed no-relay-write/no-active-huddle regression that revokes membership and requires the outgoingptag to be stripped. -
[P2] Clearing tenant-keyed in-flight dedupe on every switch permits duplicate A deployments on A→B→A. The map is already scope-keyed (
useDetachedAgentStart.ts:26,131), butuseCommunityInit.ts:55-83clears all entries. Returning to A while its first start is running permits another start. Backendprovider_deploy.rs:54-65serializes but does not coalesce;:19-23deliberately redeploys an already-deployed agent, so deploy #2 can replace the harness that just came up for message #1. Required: retain safely scoped in-flight entries across switches (or coalesce in a scope-aware backend epoch), with an A→B→A held-deploy regression proving no second deployment and proving later retry unlocks after completion/rejection.
Verification at this exact head
- PASS:
git diff --check 4a9de1a...00de55e; clean detached worktrees. - PASS: full Desktop JS package, 5,856/5,856.
cargo test -p buzz-acp: 903 passed, 1 failed (keepalive_resets_idle_past_deadline, 111 ms); isolated exact-head rerun passed. Treated as a non-PR flake.- Full Desktop smoke locally timed out at the 600s harness limit after 374 displayed passes with no observed failure; confidence gap, not requested author rework.
- Full Tauri local package was blocked by the absent
desktop/src-tauri/binaries/buzz-acp-aarch64-apple-darwin; confidence gap, not requested author rework. - CI at final freshness check remained in progress; completed checks included Unit Tests, relay/backend integration, macOS/Windows builds, Rust lint, security, Semgrep, and DCO.
- Native Desktop/AX observation was not run. Reviewer/tooling-owned confidence gap; the deterministic lifecycle regressions above remain author-owned.
No additional instrumentation privacy defect was found: the new perf record contains counts/flags/timings rather than content, pubkeys, channel IDs, or relay URLs (sendPerfLog.ts:24-31,54-61). NIP-11 cache scoping/TTL/failure behavior, replay-floor handling, and local/provider scope rechecks appeared coherent in the reviewed source and focused coverage.
The send-perf probes added in adc7330 existed to attribute the post-publish-first spinner latency, and that analysis is done — the run log confirmed the shipped fixes and named the residual cost. Remove the instrumentation so the send path carries no per-send probe; every behavior it measured (publish-first wake, replay floor, revalidation dedupe, NIP-11 self cache) stays. - Frontend: delete `sendPerfLog.ts` and its six tests. `completeSend` no longer wraps its awaited steps or records facts; the publish-boundary revalidation returns to the direct three-condition ternary it wrapped (same conditions, same order). The perf-only `detachedStarts` counter leaves `EnsureAgentMentionsReadyResult` — `startAgentDetached` is passed straight through again. Its boolean "did this fire" return stays: the dedupe and fail-closed unit tests pin it; only the doc wording that justified it via the send-perf summary is gone. - Backend: delete `send_perf.rs` (the `Phase` stopwatch, its two tests, and the `log_send_perf` mirror command) and strip the phase timers and log lines from the relay-directory rebuild (`query_all_relay_pages` returns just the events again), the NIP-11 `self` cache hit/miss path, `send_channel_message`'s build/submit split, the shared submit's rate-limiter/HTTP split, and `start_managed_agent` entry. - E2E: drop the `log_send_perf` bridge mock and the one-summary-per-send assertion from the fast-path mentions spec; its revalidation-count assertions remain and still pin the restored ternary. Verified: cargo fmt --check, clippy --all-targets -D warnings, and cargo test --lib on desktop/src-tauri (3054 passed — down exactly the two removed Phase tests), desktop tsc --noEmit, biome check over src + tests (exit 0), desktop unit tests (5850 passed, 0 failed — down exactly the six removed sendPerfLog tests), and the modified mentions spec green against a fresh pnpm build:e2e bundle. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
REQUEST CHANGES on exact head 7afc7eee61043e17e31b9c4811e9aa6666901d55 against base 4a9de1a3a121285ef475d630b2b5764044c02cde.
The new commit removes performance instrumentation but leaves four current-head lifecycle defects intact.
Blocking findings
-
[P1] A wake failure can claim “your message was sent” before relay acceptance, including when publish fails.
desktop/src/features/messages/ui/useMentionSendFlow.ts:418-425awaits readiness; readiness fires detached start atuseEnsureAgentMentionsReady.ts:105-120; relay publication occurs later atuseMentionSendFlow.ts:516-523. The detached rejection path unconditionally emits the success-worded warning atuseDetachedAgentStart.ts:56-59,149-151. If publication then rejects,useMentionSendFlow.ts:589-593restores the composer, but the false claim remains and an agent may have been woken for an unaccepted message.Author action: queue scoped wake intents and fire only after successful relay publication, or otherwise condition both wake and warning on confirmed acceptance. Add deterministic start-reject + publish-reject coverage proving no success warning and no orphan wake.
-
[P1] A delayed community-A wake failure can disclose A’s agent context while B is active. The detached catch still toasts the captured A agent name/error without checking active community or signer (
useDetachedAgentStart.ts:149-151). Community reset only clears the module map (:28-35;features/communities/useCommunityInit.ts:55-82), not the outstanding promise or global toast. The retained E2E atdesktop/tests/e2e/mentions.spec.ts:2791-2865mutates local storage and expects the stale warning rather than driving A→B and proving suppression.Author action: fence warning delivery to the captured community and signer, suppressing UI output after either changes, and add a real rail-driven A→B regression proving no A agent identity or startup detail appears in B.
-
[P2] Mention authorization can remain stale across awaited work before publish. Admission occurs at
useMentionSendFlow.ts:367-369, before managed-agent lookup (:378), readiness/persona work (:418-425), and huddle IPC (:445-453). The second pass remains conditional on upload, previews, or a boolean reporting relay side effects (:496-507). A no-write/no-active-huddle leg can therefore suspend while membership is revoked and still publish the oldadmittedMentionPubkeysat:516-523. The current fast-path test explicitly expects only one revalidation (desktop/tests/e2e/mentions.spec.ts:1854-1860).Author action: revalidate after any post-admission suspension or move admission to a genuinely immediate publish boundary. Add a held no-write/no-active-huddle regression that revokes membership and proves the outgoing
ptag is stripped. -
[P2] Community reset destroys tenant-keyed in-flight coalescing and permits duplicate provider deployment after A→B→A. The map is already keyed by relay URL and pubkey (
useDetachedAgentStart.ts:26,130), but reset clears every entry (:29-35;useCommunityInit.ts:55-82). Returning to A while its first start is unresolved can invoke a second start. Native provider locking only serializes (desktop/src-tauri/src/commands/agents/provider_deploy.rs:54-65); it does not coalesce, and deployed records are deliberately redeployed (:19-23).Author action: retain safely tenant-keyed in-flight entries across switches, or coalesce through a scope-aware backend epoch. Add held-deploy A→B→A coverage proving one A deployment while settlement still unlocks later retry.
Verification owner: author for all four fixes and causal regressions; reviewer for exact-head ordering/ownership re-trace.
Exact-head validation
- Full Desktop JS package: 5,850/5,850 passed in both lanes.
- Desktop TypeScript typecheck and lint: passed in the product lane.
- Focused changed-journey smoke tests: 3/3 passed (publish-first stopped agent, detached failure warning, retained pseudo-switch test).
git diff --check 4a9de1a...7afc7eee: passed.- Delta
00de55e...7afc7eee: 15 files, 39 insertions / 457 deletions; instrumentation/timer/counter removal introduced no additional concrete defect found in the inspected delta. - Live head was refreshed immediately before submission and remained
7afc7eee61043e17e31b9c4811e9aa6666901d55; GitHub userjedwards27differs from authormatt2e.
Confidence gaps: native Desktop/AX observation, full local Tauri/native package, and full local smoke completion were not rerun for this round. Several hosted jobs remain in progress. These are reviewer/tooling or CI-owned confidence gaps, not additional author rework.
Any new head invalidates this verdict.
jedwards27
left a comment
There was a problem hiding this comment.
Review of base 4a9de1a3a121285ef475d630b2b5764044c02cde → exact head 7afc7eee61043e17e31b9c4811e9aa6666901d55.
The latest commit removes send-performance instrumentation, but the current production paths still contain four author-actionable ordering/ownership defects:
-
[P1] A detached wake failure can claim the message was sent before relay publish succeeds.
useMentionSendFlow.ts:418-425awaits readiness before the relay send at:516-523; readiness starts the detached agent atuseEnsureAgentMentionsReady.ts:105-120; anduseDetachedAgentStart.ts:149-151emits the global “your message was sent” warning on rejection. An immediate start rejection can therefore display success wording before acceptance, and if publish then rejects (useMentionSendFlow.ts:589-593) the composer is restored while the false success claim remains. It also wakes an agent for a message that may never be accepted. Queue the scoped wake until successful publish (or otherwise gate wake and warning on confirmed publish), with a deterministic start-reject + publish-reject regression proving no success wording and no orphan wake. -
[P1] A delayed community-A wake failure can leak A’s agent context after switching to B. The detached promise catch globally toasts its captured agent name/error (
useDetachedAgentStart.ts:149-151) without checking the active community or signer.resetDetachedAgentStarts()(:28-35) clears bookkeeping, not the outstanding promise or globally mounted toaster. The retained test atdesktop/tests/e2e/mentions.spec.ts:2791-2865mutates localStorage and positively expects the stale warning rather than performing the real rail switch and proving suppression. Fence warning delivery to the captured community and signer, and add a real A→B regression asserting that A’s agent name/backend detail never appears in B. -
[P2] Mention authorization can be stale at publish after awaited “no side effect” work. Admission occurs at
useMentionSendFlow.ts:367-369, then managed-agent lookup, readiness/persona work, and huddle IPC can suspend execution (:378,418-425,445-453). Revalidation at:496-507is conditional on upload, previews, or a relay-side-effect boolean. A matching access policy (wrote=false) plus no active huddle leaves that boolean false, so revocation during the suspension still publishes the oldadmittedMentionPubkeysat:516-523. Revalidate after every post-admission suspension, or move admission to the immediate publish boundary; add a held no-write/no-active-huddle revocation test. -
[P2] Community reset breaks tenant-keyed in-flight coalescing, permitting duplicate A provider deploys on A→B→A. The start map is already keyed by relay URL and pubkey (
useDetachedAgentStart.ts:26,130), but global reset clears every tenant (:29-35) and community reset calls it fromuseCommunityInit.ts:55-82. Returning to A while A’s first start is unresolved can issue a second deploy. Native locking inprovider_deploy.rs:54-65serializes but does not coalesce, and deployed records are intentionally redeployed (:19-23). Retain tenant-keyed in-flight entries across switches (or coalesce with a scope-aware backend epoch), with a held A deploy → B → A test requiring one A invocation and settlement still permitting retry.
Exact-head evidence:
- Full Desktop JS package: 5,850 passed, 0 failed.
- Desktop TypeScript typecheck and lint passed (lint retained unrelated warnings/info).
- E2E build plus focused publish-first, detached-failure-toast, and current pseudo-switch smoke tests passed.
git diff --check 4a9de1a3...7afc7eeepassed.- Live GitHub head was rechecked as
7afc7eee61043e17e31b9c4811e9aa6666901d55; reviewer worktrees were clean.
Confidence gaps, not additional author actions: no native GUI/AX journey or full local smoke suite was run. Several CI jobs remained in progress at submission time. The instrumentation-removal delta introduced no separate user-visible or privacy defect found in the reviewed paths.
PR #7154 review point 1 (P1): the publish-first detached wake fired during send preparation, so a fast start rejection could toast "Could not start {agent} - your message was sent..." before the relay had accepted anything, and when the publish then failed the composer silently restored with that false claim as the only messaging on screen. The fail-closed scope refusal toasted the same wording synchronously, deterministically pre-publish. Any abort after the wake (cancel, readiness error, huddle failure, publish rejection, dismissed non-member prompt) also stranded a started harness for a message that never landed. Implement the reviewer's fix (option A of the analysis): wakes queue while the send prepares and flush fire-and-forget only after `await send(...)` resolves. - useEnsureAgentMentionsReady no longer takes startAgentDetached; it returns `agentsToWake` - the member fast path enqueues directly, the attach path hands the collector to attachManagedAgentToChannel as `detachedStart` (whose signature is unchanged). - createMentionedPersonaAgents collects the same way and carries its entries on the pending draft, which also moves persona wakes behind the non-member prompt: a dismissed prompt now drops them instead of waking an agent for a message the user chose not to send. - completeSend merges the draft and readiness queues (first entry per agent wins - it carries the earliest floor) and finishSend flushes them immediately after the publish resolves, before the post-send cancellation check, so a cancellation racing a successful publish cannot drop the wake for a message that did land. The spinner gains no awaited work; every abort path simply drops the queue. - Each wake's replay floor is captured at enqueue time, not flush time: the flush now runs post-publish, so a flush-time stamp could exceed the message's created_at and push the harness's startup watermark past the very message the floor exists to cover (buzz-acp clamps the floor to [now-15min, now]). useDetachedAgentStart takes the queued floor as an optional parameter and keeps fire-time capture for callers without one. The toast wording is untouched - "your message was sent" is now structurally true whenever it can appear. Tests, each confirmed red on the pre-fix code: - Three useEnsureAgentMentionsReady unit tests pin the queue contract: a stopped member agent lands in agentsToWake with an enqueue-time floor and no backend call; running/deployed agents are not queued; the attach seam queues instead of firing. - Two useDetachedAgentStart unit tests pin that an explicit floor reaches the start payload verbatim and that the no-argument form still captures fire time. - New deterministic E2E spec: with sendMessageErrors and startManagedAgentErrors both armed, a failed publish restores the draft with zero start_managed_agent calls, no timeline row, and no "your message was sent" toast (pre-fix: one start plus the false toast, no timing games needed). - The in-channel publish-first spec now also pins sign_event strictly before start_managed_agent. The repository file-size ratchet stays green: useMentionSendFlow.ts lands at 975 lines after the queue plumbing. Verified: desktop tsc --noEmit, biome check over src + tests (exit 0), desktop unit tests (5855 passed, 0 failed), the full mentions (86) and channels (89) Playwright smoke suites against a pnpm build:e2e bundle, and a 3x stress rerun of the new spec. The existing publish-first, dedupe, fail-closed-scope, and start-failure-toast specs all stay green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
…munity PR #7154 review point 2: <Toaster /> mounts outside the community remount boundary, and the detached wake's catch toasted its captured agent name and error detail unconditionally — so a start that settled after a community switch rendered community A's failure over community B's UI. The wake itself already fails closed at the backend; what leaked was warning delivery: a toast in B naming an agent that does not exist in B, carrying an uncontextualized backend error, reads as a bug in B. Implement the reviewer's fix as designed in the review analysis — a module-level scope mirror compared at toast time: - New detachedToastScope module mirrors the on-screen tenant scope outside React: set to {relayUrl, signerPubkey} by useCommunityInit when a community apply completes, cleared in resetCommunityState() per the module-singleton rule. - The catch in useDetachedAgentStart delivers the failure toast only when the scope captured at fire time matches the mirror — the relay URL verbatim past a trim and the signer case-insensitively, the backend's own comparison semantics. Mismatch or no mirror suppresses with a console.warn for diagnosability rather than rewording: any wording still names another community's agent over B's UI. - Scope comparison, deliberately not a reset generation: an A→B→A round-trip restores A's mirror, so a slow start fired in A still warns once the user is back in A — exactly where "mention the agent again" is actionable. A generation check would silently drop it. - The synchronous unscoped-start refusal toast stays unfenced by design: it fires at send time in the firing community. Tests: - Four unit tests drive the mirror through its module seam (standing in for useCommunityInit, its only production writer): on-scope delivery as the over-suppression control, suppression after a switch plus the console.warn, suppression mid-switch on a cleared mirror, and A→B→A re-delivery. Both suppression tests are red without the fence. - The old stale-toast E2E spec — which mutated localStorage without a real switch and positively pinned the stale toast, the outcome the fence now forbids — is replaced by a real rail-switch regression: two seeded communities, a genuine community-rail-button click driving the provider → remount → resetCommunityState path, the held start refused by the mock's scope check, and the toast asserted absent in B via non-retrying count snapshots (a retrying toHaveCount(0) waits out sonner's auto-dismiss and passes against the very toast it forbids — the first draft did exactly that and stayed green on the unfenced build). It keeps the published-message and scoped-invoke-payload assertions the unit tests cannot pin, and is red without the fence at the stale-toast snapshot. The existing no-switch start-failure spec is the E2E positive control — it now also proves the mirror is set on community apply. - The mock bridge pushes a start_managed_agent:settled marker into the command log on resolve and reject (a distinct string, so exact-match commandCount("start_managed_agent") is untouched), giving the spec a deterministic wait for the delayed rejection before its negative assertions. Verified: desktop tsc --noEmit, biome check over src + tests (exit 0, remaining diagnostics pre-exist in unrelated files), desktop unit tests (5859 passed, 0 failed), the full mentions (86) and community-rail (25) Playwright smoke suites against a pnpm build:e2e bundle, a 3x rerun of the new spec, and the repository file-size gate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
… switches PR #7154 review point 4 (P2): resetCommunityState cleared the in-flight detached-start map on every community switch, and the justification — "those starts belong to the community being left and are about to fail closed at the backend's scope assertion anyway" — is wrong for a round trip. The scope assertion is a current-state check, so a deploy still held from community A becomes valid again the moment A is re-applied. Nothing settles the held start across the switch (the mutation lives on the QueryClient above the remount boundary; apply_workspace aborts no in-flight deploys), the record still reads not_deployed for the whole window, and the map entry was the only duplicate guard on this path — so an A→B→A round trip plus a second mention deployed a provider agent twice, handing the provider the second message's replay floor (past the first message: the exact failure class this branch exists to prevent). Local spawns were never exposed; the backend coalesces those under the process lock. Implement the reviewer's fix (option (a) of the analysis): retain the entries across switches. - resetCommunityState no longer clears the map; an explicit exemption comment at the call site (the canonical singleton-reset inventory) records why, so a future contributor does not "fix" the omission. Retention is safe by construction: the key IS the tenant scope (relay URL + pubkey), so a retained A entry can never suppress or affect a wake in B, and the identity-guarded finally delete self-cleans at settlement, which the deploy op's timeout bounds. The backend's own provider_deploy_locks map is likewise never cleared on workspace apply. - resetDetachedAgentStarts stays exported as a test-only isolation seam and its doc now says so; the map header and finally-guard comments that encoded the refuted "fail closed anyway" rationale are rewritten to record the retention contract instead. - Deliberately NOT scope-filtered clearing ("only entries for the community being left"): the A-scoped entry is exactly the one that must survive the A→B switch, so that variant recreates the bug. - The detachedToastScope mirror reset stays: on-screen warning delivery is genuinely per-community state. Out of scope, unchanged: backend deploy-epoch coalescing (option (b)) remains the deferred durable fix for the wake paths that do not funnel through useDetachedAgentStart (Agents-panel Start, restore, inbound-persona deploys, reconcile_on_workspace_apply). Residual accepted: on a same-relay identity switch, a retained old-identity entry suppresses the new identity's wake until the old start fails closed and settles — the same accepted within-community residual, self-resolving on the user's next send. Tests: - New E2E regression, the load-bearing one — it drives the real rail-switch → remount → resetCommunityState path that did the clearing: a not_deployed provider agent's deploy is held open via startManagedAgentDelayMs; the mention publishes and deploys once, scoped to A; a real A→B→A rail round-trip runs under the hold; a second mention back in A publishes while start_managed_agent stays at exactly 1; the held deploy is then settled on demand and rejects, its failure warning delivers back in A, and a third mention re-fires the wake (2 calls) — retention ends at settlement rather than latching the agent. - New mock-bridge seam for that spec: __BUZZ_E2E_RELEASE_MANAGED_AGENT_STARTS__() settles starts held behind startManagedAgentDelayMs on demand, following the bridge's defer/release idiom — the hold must deterministically outlast the round-trip AND settle before the retry leg. The first settlement is armed to reject via startManagedAgentErrors because a successful mock settle writes `deployed` into the record, which would hide the retry leg behind the status check rather than the map — the seam under test. - Two unit tests pin the complementary hook contracts in the existing real-CommunitiesProvider harness: an A→B→A switch hands back a callback whose key matches the retained entry, so the wake stays suppressed; and a rejection across the round-trip re-permits. The existing cross-community test keeps pinning that a B wake during the interlude is unaffected. Verified: desktop tsc --noEmit, biome check over the touched files, desktop unit tests (5856 passed; the 5 failures are the pre-existing inboxReopenNavigation/useRetainedProjectGitViews baseline, present on origin/main), the full mentions (87) and community-rail (25) Playwright smoke suites against a fresh pnpm build:e2e bundle, a 3x stress rerun of the new spec, and the repository file-size gate. The new E2E spec was confirmed red on the pre-fix build — 2 deploys vs 1 at the second-send snapshot — with the map clearing temporarily restored. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
There was a problem hiding this comment.
:bot: Jude’s code review agent — Request changes
Reviewed base 4a9de1a3a121285ef475d630b2b5764044c02cde through exact head b4e87be877ad55a0466f2009bceeade6cab02bed.
The new head repairs the publish/wake ordering, stale-community warning disclosure, and A→B→A duplicate deployment behavior. Two author-actionable scope/freshness defects remain.
1. [P2] Mention authorization can still go stale before publication
The first mention admission at desktop/src/features/messages/ui/useMentionSendFlow.ts:379-390 precedes awaited managed-agent lookup, readiness (:430-437), and no-active-huddle IPC (:470-476). The second admission at :527-530 remains conditional on uploads, previews, or relaySideEffectsRan. A matching access policy and no active huddle can therefore leave it false despite those suspension points; membership revoked during the gap still permits the old admittedMentionPubkeys to be published at :539-546. The fast-path E2E at desktop/tests/e2e/mentions.spec.ts:1854-1860 continues to require only one revalidation rather than exercising revocation during a held no-write/no-active-huddle leg.
Author action: revalidate after every post-admission suspension, or move admission to an actually immediate publish boundary. Add a causal regression that holds a no-write/no-active-huddle operation, revokes membership, and proves the outgoing p tag is stripped; mutation-check the production trigger.
Verification owner: author for the fix/regression; reviewer for exact-head retrace.
2. [P2] Same-relay identity switches share an incomplete detached-start key
The retained in-flight key at desktop/src/features/messages/ui/useDetachedAgentStart.ts:153 contains relay URL and agent pubkey but omits the expected signer. A slow start under owner A therefore suppresses owner B's legitimate wake for the same agent pubkey on the same relay at :154-159 until A settles. Native scope is explicitly relay plus signer (desktop/src-tauri/src/commands/agents.rs:842-846), and provider deployment rechecks both after locking (desktop/src-tauri/src/commands/agents/provider_deploy.rs:27-32,90-96). The renderer must not coalesce operations belonging to distinct signer scopes. Current A/B tests retain one constant signer (desktop/src/features/messages/ui/useDetachedAgentStart.test.mjs:43,148-163).
Author action: include normalized expectedSignerPubkey in the in-flight key. Add same-relay A→B identity-switch coverage with A held: B must fire independently, the same signer must still deduplicate, and settlement must unlock retries independently per signer.
Verification owner: author for the fix/regression; reviewer for exact-head retrace.
Verified on this head
- Full Desktop JS suite: 5,861/5,861 passed.
- TypeScript typecheck passed; lint exited 0 with only existing unrelated diagnostics;
git diff --checkpassed. - E2E build and four focused smoke journeys passed, confirming publish precedes wake, failed publish does not wake, a real A→B switch suppresses A's stale warning, and a held provider deploy survives A→B→A without duplication.
- Hosted unit, Rust lint, relay/backend E2E, Windows/macOS builds, release-candidate, security, Semgrep, DCO, all four smoke shards, and both integration shards were successful in the final snapshots supplied by the review lanes.
Confidence gaps — not additional author actions
Native Desktop/AX and the full local smoke suite were not completed. One repeated local switch journey failed before the changed switch/wake assertions at mention-chip rendering, while the other repetitions and all hosted smoke shards passed; this did not establish a PR-caused defect. Desktop Core was still in progress in the latest lane snapshot. These remain reviewer/tooling or CI-owned verification, not reasons for additional author rework.
PR #7154 review round 2, point 2 (P2): the in-flight detached-start map was keyed `(relay URL, agent pubkey)`, but the scope a wake actually asserts is relay *and* signer — `start_managed_agent` runs both `assert_expected_relay_scope` and `assert_expected_signer` before any spawn or deploy, and the provider path re-asserts both against the payload rebuilt after the deploy lock. So a start held under one signing identity suppressed another identity's wake for the same agent on the same relay: the renderer coalescing across a boundary the backend deliberately distinguishes. The omission came from mirroring the wrong key. The map's doc justified the pair as matching the backend's `ManagedAgentRuntimeKey` — and it did, that key is `(pubkey, relay_url)` with no signer. But that tracks *runtimes*; this map tracks scoped start *operations*, and an operation's identity is the full scope it asserts. The doc is rewritten so the next reader does not restore the runtime-key rationale. Reachability is narrow but real: the signing key is one global per install, so two signers only arrive sequentially, via a mid-session import — chiefly the membership-denied onboarding overlay, which writes the new identity straight into the live query cache (`queryClient.setQueryData(["identity"], …)`) while the app underneath, including this module-level map, keeps running. Correcting our own record from b4e87be, which accepted this as "the retained old-identity entry suppresses the new identity's wake until the old start fails closed and settles": that holds only if the old start had not yet passed the scope checks. If it was already mid-spawn or mid-deploy it does *not* fail closed — the scope was valid when checked, the deploy runs to completion under the stale owner, and every send in that window is dropped. Adding the signer eliminates the class rather than shrinking the residual. - The key is now `(relay URL, expected signer, agent pubkey)`. Relay stays verbatim (its backend comparison is case-sensitive past the scheme); the signer is already canonicalized at capture, matching `assert_expected_signer`'s trim + case-insensitive compare, so two casings of one identity cannot split the key. `undefined` can never reach it — the fail-closed refusal returns before key construction when either scope half is missing. - Everything downstream falls out: the identity-guarded `finally` delete closes over the same key string, so settlement frees only its own signer's entry. - Nothing existing changes. Same-signer dedupe is untouched, and the A→B→A community retention is untouched — the identity is global, so a community round-trip hands back a callback carrying the same signer, which still matches the retained entry. - `resetCommunityState`'s exemption comment (the canonical singleton-reset inventory) now names the three-part key. Coverage at the unit seam — the existing harness drives the real hook under the real `CommunitiesProvider` and a real `QueryClient`, with the identity moved by the same `setQueryData(["identity"], …)` write the membership-denied import uses, plus a mutable `get_identity` mock so a refetch stays consistent: - A wake under a newly imported identity is not suppressed by one held under the old: two calls, same agent and same relay, differing only in `expectedSignerPubkey`. - Settling one signer's start leaves the other signer's suppression intact — per-signer settlement independence. - The existing same-signer suppression test is the unmodified control, as are the cross-community and A→B→A retention tests. Both new tests were confirmed red with the signer removed from the key, with the other 18 in the file green. No E2E: unlike b4e87be's retention fix, the seam here is entirely inside the hook, and every E2E spec runs under the mock bridge's single constant identity. Verified: desktop tsc --noEmit, biome check over the touched files, desktop unit tests (5863 passed, 0 failed), and the mentions Playwright smoke suite against a fresh pnpm build:e2e bundle (86 passed, 1 failed — the persona-mention spec flaking under load at the composer-chip step, before any start assertion; green 3x in isolation). The repository file-size gate stays green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Matt Toohey <contact@matttoohey.com>
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — Request changes
Reviewed base 4a9de1a3a121285ef475d630b2b5764044c02cde through exact head d7e6e3ec862ea76dc96fbdd2493b63b08015f324.
The changed head correctly repairs signer-aware detached-start coalescing, and the publish-first UX remains truthful and community-scoped. One author-actionable authorization race remains.
[P2] Mention authorization can still go stale before publication
desktop/src/features/messages/ui/useMentionSendFlow.ts:379-390 admits mention pubkeys, then suspends for managed-agent lookup; :430-437 always awaits readiness; and :470-476 awaits huddle IPC. The publish-boundary pass at :527-530 runs only when upload/preview work exists or relaySideEffectsRan is true. A matching access policy (wrote=false) plus no active huddle leaves that flag false despite those suspension points. If authorization is revoked during a held no-write/no-active-huddle leg, the old admitted p tag is still published at :539-546.
The searched publish-boundary regressions cover deferred upload, attach writes, and active-huddle writes (desktop/tests/e2e/mentions.spec.ts:1985-2252), but not revocation during a held no-write/no-active-huddle suspension. The fast-path assertion at :1854-1860 instead requires only one revalidation, preserving the unsafe optimization.
Author action: revalidate whenever any suspension follows admission, or move admission to a genuinely suspension-free publish boundary. Add a causal regression that holds a no-write/no-active-huddle operation, revokes authorization, and proves the outgoing p tag is stripped; mutation-disable the production trigger and require the regression to fail.
Verification owner: author for this fix and causal regression; reviewer for exact-head retrace; CI for pending hosted gates.
Repaired and verified contracts
- The detached-start key now contains verbatim relay URL, normalized signer, and normalized agent pubkey (
useDetachedAgentStart.ts:139-167), matching its Tauri payload (:181-185) and native relay/signer assertions (desktop/src-tauri/src/commands/agents.rs:842-846). Provider deployment reasserts both against the post-lock rebuilt payload (provider_deploy.rs:65-96). - Real-hook tests exercise live identity-cache replacement: a new signer on the same relay/agent fires independently, and settlement releases only that signer’s key (
useDetachedAgentStart.test.mjs:757-841). The focused detached-start suite passed 20/20 at this head; removing signer from the production key made both new cases fail before restoration. - Publication is accepted before queued wakes flush (
useMentionSendFlow.ts:539-555). Failed publication restores the draft and does not wake; failed wake truthfully warns that the message was sent but the agent may not respond. - Stale failure warnings are fenced by visible relay/signer scope, and failed starts release their map entry for retry (
useDetachedAgentStart.ts:139-218).
Exact-head validation
- Full Desktop JS package: 5,863/5,863 passed.
- Desktop typecheck: passed.
- Focused interaction E2E: 9/9 passed, covering publish-before-wake, duplicate suppression, truthful wake failure, failed-publish no-wake/no-false-success, A→B warning isolation, A→B→A dedupe, provider deploy, and managed-agent member/non-member flows.
- Touched-file Biome check and
git diff --check: passed. - Live PR head immediately before submission:
d7e6e3ec862ea76dc96fbdd2493b63b08015f324; authenticated reviewerjedwards27, authormatt2e.
Confidence gaps
The full local smoke-file rerun did not complete after reviewer worktree/webserver teardown; the clean focused rerun passed. Hosted Desktop Core, four smoke shards, relay E2E, and Windows Rust remained in progress at the final snapshot, with no completed required check failing. These are reviewer/tooling or CI-owned gaps, not additional author defects.
Any new head invalidates this verdict.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES at exact head d7e6e3ec862ea76dc96fbdd2493b63b08015f324 (base 4a9de1a3a121285ef475d630b2b5764044c02cde). This changed-head review supersedes the prior verdict at b4e87be8… and integrates the systems/integration and product/UI/adversarial lanes.
Risk: high — mention authorization, signed publication, detached agent lifecycle, renderer↔Tauri scope, and cross-community failure presentation.
Blocking finding
[P2] Publish-boundary authorization remains stale across post-admission suspensions. desktop/src/features/messages/ui/useMentionSendFlow.ts:379-390 admits mention pubkeys, then awaits managed-agent lookup; :430-437 always awaits readiness, and :470-476 awaits huddle IPC. But :527-530 repeats authorization only for upload/preview work or when relaySideEffectsRan is true. A matching access policy (wrote=false) plus no active huddle leaves that flag false. If authorization is revoked during any held no-write/no-active-huddle operation, the old admitted p tag is still published at :539-546. Existing publish-boundary E2E covers deferred upload and write-producing paths, while the fast-path test explicitly expects one validation; it does not cover causal revocation during these other awaited operations.
Author action: revalidate whenever any suspension follows admission, or move admission to a genuinely suspension-free point immediately before publish. Add a causal held no-write/no-active-huddle revocation regression and mutation-prove its production trigger.
Verification owner: author for fix and causal regression; reviewer for exact-head source/interaction retrace.
Repaired and verified
The prior signer-coalescing defect is repaired. The renderer key now includes verbatim relay plus normalized signer and agent (desktop/src/features/agents/useDetachedAgentStart.ts:139-167), matching the renderer payload and native relay/signer contract. Real-hook tests cover same-relay identity replacement and signer-independent settlement; mutating the production key back to relay+agent made both new signer cases fail before restoration.
The integrated product lane found no additional UI defect. Accepted publication still precedes detached wake; publish failure restores the draft and triggers no wake, while wake failure truthfully preserves the sent message and reports that the agent may not respond. Cross-community warning isolation, retry after failed start, and A→B→A coalescing remained sound in reviewed paths and focused journeys.
Exact-head validation
- Full Desktop JS package: 5,863/5,863 passed.
- Desktop typecheck passed; Desktop check exited 0 with only pre-existing diagnostics outside the delta.
- Focused detached-start suite: 20/20 passed; signer-key causal mutation failed the two intended cases, then restoration passed.
- Focused product interaction E2E: 9/9 passed, covering publish-before-wake, duplicate suppression, truthful wake failure, failed-publish no-wake/no-false-success, A→B warning isolation, A→B→A dedupe, provider deploy, and member/non-member managed-agent flows.
git diff --check b4e87be8...d7e6e3ecpassed in clean detached worktrees; both lanes rechecked local/live head equality atd7e6e3ec….
Residual risk / confidence gaps
A broad smoke-file rerun reached 31 passes before reviewer worktree/webserver teardown caused 404/missing-worker failures; the clean focused 9/9 rerun passed. At final refresh, Desktop Core, smoke shards, integration/relay/backend E2E, Unit Tests, and Windows Rust were still running; completed observed gates were green. These are reviewer/tooling or named CI-gate confidence gaps, not additional author work and not the basis for this verdict.
Summary
Cuts perceived agent-mention send latency by publishing the message first and waking the agent afterwards, instead of blocking the send on a synchronous agent start/deploy round-trip. A send that mentions a stopped or undeployed managed agent now shows the message immediately; the wake runs fire-and-forget after the relay accepts the publish. The already-running-agent send also gets faster via revalidation dedupe and NIP-11 caching.
Changes
Publish-first agent wake
await send(...)resolves. No start can fire — and no "your message was sent" toast can appear — for a message the relay never accepted; every abort path (cancel, readiness error, publish rejection, dismissed non-member prompt) simply drops the queue. Persona-create wakes ride the pending draft behind the non-member prompt for the same reason.useDetachedAgentStarthook passesexpectedRelayUrl+expectedSignerPubkeywith every start, so a wake that outlives a community switch fails closed at the backend instead of spawning against the new tenant. A wake whose scope has not resolved yet (identity query still loading, blank stored relay URL) is refused with a recoverable toast rather than fired unscoped.(relay URL, pubkey)— the same tenant pair the backend keys on — so two quick sends or two composers cannot double-spawn a cold agent during the seconds-long start window. Entries are deliberately retained across community switches (the key is the tenant scope, so a retained entry can never affect another community, and clearing it let an A→B→A round trip deploy a provider agent twice) and self-clean when the start settles.Replay floor
BUZZ_ACP_REPLAY_FLOOR, threaded through both local spawns (spawn_agent_child) and provider deploys (deploy_to_providerinjects it intolaunch.policy_env), so the harness's startup watermark replays back past the just-published triggering message no matter how long the spawn takes.buzz-acpclamps the floor to[now − 15min, now].created_atand skip the very message the floor exists to cover.launch.env. Both halves share oneREPLAY_FLOOR_ENV_VARconst.Send-path latency reductions (already-running agents)
selflookups are cached per relay URL for 5 minutes. Only verified values are cached — non-2xx and malformed responses stay retryable — and URL keying keeps community switches from serving another relay's identity.applyReusableAgentAccessPolicynow reports its relay write explicitly ({ agent, wrote }) instead of signalling through object identity, so the revalidation trigger above is load-bearing by construction.File splits
Four files crossed the repository file-size ratchet during this work; one cohesive unit was extracted from each rather than raising a ceiling —
runtime/setup_payload.rs,commands/agents_create_fields.rs,app_state_accessors.rs, anduseEnsureAgentMentionsReady.ts. The ratchet is green at the tip.Review follow-ups
The three concrete findings from the first review round are fixed at the tip: the pre-publish wake and its false "your message was sent" toast (fixed by queueing wakes behind the publish), the stale cross-community failure toast (fixed by the scope-mirror fence), and the A→B→A duplicate provider deploy (fixed by retaining the tenant-keyed in-flight entries across switches). The fast-path admission-staleness point is answered in the review thread: deferred paths already re-validate at the publish boundary, and the remaining fast-path window is milliseconds against an irreducible network-transit race.
Mid-branch send-perf instrumentation was added to attribute the residual spinner latency and reverted once that analysis concluded — it is net-zero in this diff.
Deferred follow-ups
Durable mention catch-up via
event_mentions(option 2 step 3) and backend deploy-epoch coalescing for the wake paths that do not funnel throughuseDetachedAgentStart(Agents-panel Start, restore, inbound-persona deploys) are intentionally left for separate changes.Testing
cargo test --libon desktop/src-tauri: 3054 passed; clippy-D warnings+ fmt cleaninboxReopenNavigation/useRetainedProjectGitViewsbaseline, present on origin/main);tsc --noEmitand biome cleanpnpm build:e2ebundles, with 3× stress reruns of each new spec{ agent, wrote }contract, and the replay-floor env layering on both spawn paths🤖 Generated with Claude Code