Skip to content

Surface agent turn failures in context for codex agents - #7549

Open
atishpatel wants to merge 7 commits into
block:mainfrom
atishpatel:workerbee/surface-agent-turn-errors
Open

Surface agent turn failures in context for codex agents#7549
atishpatel wants to merge 7 commits into
block:mainfrom
atishpatel:workerbee/surface-agent-turn-errors

Conversation

@atishpatel

@atishpatel atishpatel commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

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_error observer events. Desktop uses those fields to update one bounded failure record per conversation, with a fallback to turn_started for 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 respawnScheduled value. 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 2f06e52e with the e2e mock bridge (seeded running managed agent + turn_error observer events, viewport 1280×800):

Retrying disposition — the composer rail shows Review Bee couldn't finish · Retrying automatically · attempt 2 in place of the working indicator:

retrying

Terminal disposition — after retries are exhausted the same card reads Review Bee couldn't finish · Stopped after multiple attempts · attempt 10:

dead_lettered

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 subsequent cargo test -p buzz-acp run.

  • Manual UI validation (Fast Fizz, 2026-09-10): built desktop e2e bundle at 2f06e52e, seeded a running managed agent with turn_error observer 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 ci passed at clean merged revision f10e644a: 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

Signed-off-by: Hardworking Honey <c5c455215c2506cb8ba776518cec804af62d3a0526e32d496a22072e395042b9@buzz.block.builderlab.xyz>
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown

🔐 Codex Security Review

Status: review required for the current range.

The current range is 44c1cc7df8e0067c9d578a93ad8e4393304422b3...5752f0363561149d30c1959e2f073602f5880a83.
A new review must complete for this exact range. When manual authorization
is required, a Block organization member must comment exactly
@buzz-security-review 5752f0363561149d30c1959e2f073602f5880a83 to authorize a new review.
Any previous review applies only to its recorded range.

@atishpatel
atishpatel marked this pull request as ready for review September 10, 2026 16:05
@atishpatel
atishpatel requested a review from a team as a code owner September 10, 2026 16:05
@atishpatel atishpatel changed the title Surface agent turn failures in context Surface agent turn failures in context for codex agents Sep 10, 2026

@salman1993 salman1993 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖

Finding — Majorcrates/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 wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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.
  2. The older-runtime fallback misplaces threaded failures. The pre-PR start event has only source and triggeringEventIds, 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.
  3. 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.

Comment thread desktop/src/features/agents/recentAgentTurnFailuresStore.ts Outdated
Comment thread desktop/src/features/agents/recentAgentTurnFailuresStore.ts Outdated
Comment thread crates/buzz-acp/src/lib.rs
Brainy Bumble added 5 commits September 11, 2026 09:48
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 salman1993 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Finding — Majorcrates/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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants