Surface agent turn failures in context for codex agents - #7549
Surface agent turn failures in context for codex agents#7549atishpatel wants to merge 7 commits into
Conversation
Signed-off-by: Hardworking Honey <c5c455215c2506cb8ba776518cec804af62d3a0526e32d496a22072e395042b9@buzz.block.builderlab.xyz>
🔐 Codex Security Review
|
There was a problem hiding this comment.
🤖
Finding — Major — crates/buzz-acp/src/lib.rs:5119-5125, 5159-5167: panic recovery requeues the failed batch and starts agent respawn, but agent_panic emits neither the resulting retry/dead-letter disposition nor attempt metadata. Desktop therefore falls through to stopped (recentAgentTurnFailuresStore.ts:183) even when the request was successfully queued for retry, and it also cannot distinguish retry-budget exhaustion. The composer can tell the owner work stopped while it is actually retrying, contrary to this PR’s core status contract. Capture the result of queue.requeue, circuit/respawn outcome, and attempt before emitting the panic event, then emit the truthful disposition (retrying/dead_lettered/respawning as appropriate) plus attempt. Add tests for a recoverable panic and exhausted retry budget through the production recovery path.
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested
Reviewed head 2f06e52ed953fd575a178008c407e0923add10f6, base tip 12023a3cba60e9a6336bf790bf3565b271af929f, with the 11-file PR diff against merge-base ad9591c43bdb7e221ee93964b09d5dadf820497f.
The owner-only composer approach fits the intended feature, but three P2 correctness gaps remain:
- Recovered batched work keeps a stale retry status. A fails and is requeued. Under the default channel policy, a newer top-level B joins the retry batch
[A,B]; the new turn is anchored to B. The store clears only B’s failure key, leaving A’s “Retrying automatically” record even after the batch succeeds. Clear failures covered by the retried events, not only the newest anchor, while preserving unrelated unresolved failures. - The older-runtime fallback misplaces threaded failures. The pre-PR start event has only
sourceandtriggeringEventIds, not the new ancestry fields. Treating the last trigger ID as a known root makes a thread reply look top-level: the channel composer receives the failure while the actual thread receives none. Resolve ancestry or represent unknown correlation honestly; the compatibility fixture must match the real old producer. - Recovery metadata is incomplete on cancellation/panic paths. A Steer/Interrupt cancel-drain timeout preserves the batch for redispatch and respawns the process but emits
disposition: "stopped". The unchanged panic producer similarly requeues/respawns without emitting a disposition or triggering context. I independently confirm the existing panic finding in review 5171573418. Additionally, a panic received after its start falls outside the five-minute subscription lookback is dropped by this new store entirely. Capture and emit actual batch fate/context for these paths, distinguishing preserved work from explicit Cancel/Rotate/removed-channel drops and retry exhaustion.
Validation: read-only producer→observer→store→composer review, with complementary UI and ownership/transport review lanes. A focused Node probe executes the exact-head production store (TypeScript transpiled in a VM, source-derived comparator/normalization helpers, inert React/transport boundaries) and reproduces the stale batched retry, legacy misplacement, panic omission/default, and cancel-drain status. This is reducer evidence, not a live ACP/relay/UI exercise. No repository files changed or broad CI-equivalent suite rerun.
CI/mergeability, separately: GitHub Actions run https://github.com/block/buzz/actions/runs/34414252290 is successful for this head (attempt 2); GitHub reports MERGEABLE. No approval submitted. Exit criteria are production-seam regressions for the three contracts above, not additional architecture or unrelated hardening.
Signed-off-by: Brainy Bumble <0ed7657b57c0e8a9f5288390dd6c8d5d0a3a06abe9b01b9006814f52077d6cdf@buzz.block.builderlab.xyz>
Signed-off-by: Brainy Bumble <0ed7657b57c0e8a9f5288390dd6c8d5d0a3a06abe9b01b9006814f52077d6cdf@buzz.block.builderlab.xyz>
Signed-off-by: Brainy Bumble <0ed7657b57c0e8a9f5288390dd6c8d5d0a3a06abe9b01b9006814f52077d6cdf@buzz.block.builderlab.xyz>
Signed-off-by: Brainy Bumble <0ed7657b57c0e8a9f5288390dd6c8d5d0a3a06abe9b01b9006814f52077d6cdf@buzz.block.builderlab.xyz>
Signed-off-by: Brainy Bumble <0ed7657b57c0e8a9f5288390dd6c8d5d0a3a06abe9b01b9006814f52077d6cdf@buzz.block.builderlab.xyz>
salman1993
left a comment
There was a problem hiding this comment.
🤖 Finding — Major — crates/buzz-acp/src/lib.rs:4948-4963, 5076-5091: fatal exits/timeouts and transport errors publish turn_error before attempting the replacement process. At that point disposition is already retrying whenever the batch was requeued, but spawn_respawn_task can then refuse the restart because the crash circuit is open. If this was the last worker, the harness exits and the in-memory queued request cannot continue, while Desktop keeps telling the owner “Retrying automatically.” This is the same truthful-final-disposition contract now handled for cancellation timeouts and panics, but these two fatal branches still decide recovery after publishing status. Attempt the respawn first, include respawnScheduled, and emit stopped when no live/in-flight worker can service the queued batch. Add production-path tests for an open circuit on the last worker for both a process exit/timeout and a transport error.
Signed-off-by: Brainy Bumble <0ed7657b57c0e8a9f5288390dd6c8d5d0a3a06abe9b01b9006814f52077d6cdf@buzz.block.builderlab.xyz>
Summary
Agent failures no longer vanish when the working indicator ends. Previous behavior felt like silent failure. Buzz now keeps an owner-visible failure status in the originating channel or thread composer, shows whether it will retry or has stopped, and opens the agent activity view when selected.
The ACP harness adds the triggering conversation IDs, retry attempt, and final disposition to
turn_errorobserver events. Desktop uses those fields to update one bounded failure record per conversation, with a fallback toturn_startedfor older runtimes. Existing chat notices remain limited to terminal and action-required failures, so retries do not create message spam.Review fixes: a new retry batch clears failures only for work it fully covers. Older runtimes that omit thread ancestry show Conversation unknown and Recovery status unknown. Panic and cancellation-timeout events report their actual recovery outcome and carry context even without an earlier start event. Fatal exits, timeouts, and transport errors also decide replacement before publishing recovery status and include the actual
respawnScheduledvalue. A blocked last-worker restart reports Stopped; a live sibling or pending replacement preserves Retrying automatically. The merged model-not-found path reports Action required.Related issue
Core issue with codex was acp version install addressed by #7427
Screenshots
Validated at revision
2f06e52ewith the e2e mock bridge (seeded running managed agent +turn_errorobserver events, viewport 1280×800):Retrying disposition — the composer rail shows
Review Bee couldn't finish · Retrying automatically · attempt 2in place of the working indicator:Terminal disposition — after retries are exhausted the same card reads
Review Bee couldn't finish · Stopped after multiple attempts · attempt 10:The card is a button that opens the agent's activity view (hover shows the raw error as a tooltip).
Testing
pnpm --dir desktop test— 6,494 tests passed before the final two component checks were added; both new checks also pass when run directly.pnpm --dir desktop typecheck— passed.cargo test -p buzz-acp— 931 unit tests and 9 lifecycle tests passed.just ci— all relevant checks passed except one unrelated flaky timing test,acp::tests::keepalive_resets_idle_past_deadline; it passed in the subsequentcargo test -p buzz-acprun.Manual UI validation (Fast Fizz, 2026-09-10): built
desktope2e bundle at2f06e52e, seeded arunningmanaged agent withturn_errorobserver events (disposition: retrying/dead_lettered) via the e2e mock bridge, and verified the failure card renders in the originating channel composer rail with correct disposition copy and attempt count. Playwright assertions + screenshots above.Full
just cipassed at clean merged revisionf10e644a: 6,503 desktop tests, 936 producer unit tests and 9 integration tests, native checks/builds, and 2,098 mobile tests. One upstream real-Pi test remains ignored without its adapter prerequisite.Review-fix browser validation at merged revision
f10e644a: the E2E mock bridge verified changed-anchor retry clearing, visible legacy unknown context, standalone panic recovery status, and opening agent activity.Independent mutation checks: seven deliberately broken variants failed the expected regressions; restored source passed. Covered anchor-only clearing, partial-overlap clearing, invented legacy roots, omitted panic disposition, stopped preserved cancellation, ignored last-worker exhaustion, and a model-not-found error mislabeled as stopped. No live deployed-agent failure was induced.
F1 mutation validation at
5752f036: separately removing the fatal-exit correction, removing the transport-error correction, and hardcoding false respawn status each failed the expected production-path regression. Restoring the exact source passed. No deployed-agent failure was induced.Generated with Codex