docs: update BRC event-pump docs for slice-4 [doc-updater] - #2965
Conversation
Reflect the slice-4 changes: EGG_BRC_EVENT_PUMP flag deleted, EGG_BRC_MEMORY default flipped off→full, legacy capped-restart wrapper and MAX_CONSENSUS_RESTARTS removed. - docs/index.md: fix stale anchor links and description for the BRC Consensus Wrapper entry - docs/reference/orchestrator-cli.md: update EGG_BRC_MEMORY default from `off` to `full` - docs/architecture/brc-memory.md: update mode table default row and opening paragraph to reflect slice-4 flip - docs/development/STRUCTURE.md: update consensus_wrapper.py description to remove legacy capped-restart reference - docs/guides/sdlc-pipeline.md: replace MAX_CONSENSUS_RESTARTS restart-cap description with event-pump idle-budget model - docs/reference/agent-recovery.md: replace Restart with Backoff section and Before/After table with event-pump crash handling
![egg-reviewer[bot]](https://avatars.githubusercontent.com/u/8340608?s=60&v=4){kind=small}
There was a problem hiding this comment.
No agent-mode design concerns. Documentation-only PR updating BRC event-pump references after slice-4 (#2908) flipped defaults — no prompts, agent procedures, or pre-fetched context are being introduced or changed.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Findings
Blocking
1. docs/reference/agent-recovery.md:198 — buffer-overflow row claims a fast-fail path that does not exist in the wrapper.
The new "Crash exit-code classification" table claims:
| SDK buffer overflow (exit 255 + overflow marker) | is_buffer_overflow | Wrapper exits immediately; loop does not retry on next event |
That description matches the legacy capped-restart template, not the event-pump template that slice-4 actually deployed. In orchestrator/consensus_wrapper.py, is_buffer_overflow() is defined at L153 but never called — neither in the propose|ack|nack arm (L676–701) nor anywhere else in the script. The wrapper's own internal comment at L145–150 acknowledges this:
The event-pump's
propose|ack|nackarm does NOT yet branch on these signals (the consecutive-failure counter + idle-budget alert combo handles the operator-visible escalation today).
The actual handling for a buffer-overflow exit is identical to every other non-zero exit (L686–701): AGENT_FAIL_STREAK++, sleep 1, loop back to the next iteration — which deterministically hits the same oversized tool result. This matters because the wrapper's own L133–138 comment already flags buffer overflow as uniquely dangerous under one-shot: it's deterministic. Operators reading this row will believe the wrapper has a special fast-fail path that catches the SDK 1 MiB JSON overflow before it burns the idle budget; in reality the overflow recurs every iteration until EGG_BRC_IDLE_BUDGET_MIN (default 30 min) trips. That's a real operator-visible behavior gap that this row mis-documents.
Fix: collapse the "Current event-pump handling" cell to match the other rows — "Increments consecutive-failure counter; idle-budget escalation emits alert" — and either drop the "Classifier" column or add a note that the listed classifiers are currently inert helpers (the prose paragraph at L203 already says this; the table contradicts it).
2. docs/guides/concurrent-execution.md (not in this diff, but in scope) — same staleness problem this PR is sweeping up elsewhere.
This PR's stated goal is to clean up doc references that slice-4 (#2951) left stale. docs/guides/concurrent-execution.md was touched by #2951 itself, but the "How it works" steps and configuration table still describe the deleted legacy template, not the post-slice-4 wrapper:
- L84 step 5: "the wrapper restarts the pod with exponential backoff (initial 5 s, doubling, capped at 30 s). These signals are infrastructure-level pod failures that the wrapper still distinguishes from clean event-pump exits via the surviving
is_transient_crash/is_buffer_overflow/is_startup_failureclassifiers." — Wrong. The wrapper does not restart the pod, does not back off exponentially, and as noted above the classifiers are not invoked. - L85 step 6: "If the agent exits with a non-transient non-zero code, the wrapper exits with the same code, triggering the orchestrator's agent failure path." — Wrong. The wrapper does not propagate non-zero agent exits; it increments the streak counter and continues blocking. The idle budget is the only escalation surface.
- L97 configuration row:
TRANSIENT_RESTART_BACKOFF_INITIAL = 5. This constant does not exist in the post-slice-4 wrapper (grep -n TRANSIENT_RESTART_BACKOFF orchestrator/consensus_wrapper.pyreturns nothing). Same for the surrounding "Doubles after each crash restart, capped at 30 s" prose.
Since #2951 already modified this file and the explicit purpose of #2965 is "Update documentation to reflect changes from c03f316 (#2951)", the natural place to fix these is here. Leaving them in place means a reader cross-referencing agent-recovery.md (this PR's correct description) against concurrent-execution.md gets contradictory accounts of crash handling.
Non-blocking
3. docs/reference/agent-recovery.md:195–201 — table format implies live dispatch where none exists.
The "Classifier" column lists is_transient_crash / is_buffer_overflow / is_startup_failure per row as if each gates a distinct response. Combined with the prose at L185–186 ("the classifiers above are retained as named helpers") and the closing paragraph at L203 ("retained as named helpers but the event-pump's consecutive-failure counter + EGG_BRC_IDLE_BUDGET_MIN escalation is the primary operator-visible signal rather than per-crash branching"), a careful reader gets there — but the table by itself reads as a live dispatch matrix. Fixing finding 1 by collapsing the rightmost column to a uniform "Increments consecutive-failure counter; idle-budget escalation emits alert" would resolve this in the same edit.
4. docs/reference/agent-recovery.md:386–388 — duplicates findings 1 and 3 in the second table.
The "Behavior Matrix" near the bottom of the file also describes the wrapper-level behavior. The rows added here ("Transient crash in consensus wrapper (segfault, OOM) | Increments consecutive-failure counter; idle-budget escalation emits OVERSEER_ALERT — no restart cap" etc.) are correct in substance. But the "Startup failure in consensus wrapper (exit 1 within 30s) | Increments consecutive-failure counter; classified as transient API/network error" row reuses the "classified as ..." phrasing that implies an active classifier dispatch. Same fix as finding 3.
Verified correct
EGG_BRC_MEMORYdefault flip fromoff→full:consensus_wrapper.py:428setsEGG_BRC_MEMORY="${EGG_BRC_MEMORY:-full}". ✅MAX_CONSENSUS_RESTARTS/_RECOVERY_SYSTEM_PROMPT/EGG_BRC_EVENT_PUMPdeletions: confirmed absent fromorchestrator/consensus_wrapper.py. ✅EGG_BRC_IDLE_BUDGET_MINdefault 30 min:EVENT_PUMP_IDLE_BUDGET_MIN_DEFAULT = 30atconsensus_wrapper.py:59, default referenced at L112. ✅- Anomaly tag
stuck-phase-transition:consensus_wrapper.py:524. ✅ - Idle-budget path never exits 1 on idle alone: confirmed in
check_idle_budget(L531–549) and the main loop. ✅ - New anchor
#brc-consensus-wrapper:docs/architecture/orchestrator.md:743. ✅ - New anchor
#10-brc-consensus-wrapper-event-pump-model:docs/reference/agent-wait-patterns.md:1174. ✅ - Old anchor
#brc-event-pump-wrapper-slice-2-behind-egg_brc_event_pumpremoved from all docs (zero matches). ✅ brc-memory.mdmode table (fulldefault,write-onlyreader-rollback,offwriter-rollback) matches the slice-4 commit message atc03f316be. ✅orchestrator-cli.mdEGG_BRC_MEMORYrow consistent withbrc-memory.md. ✅
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…conflicts in docs/index.md
Conflict Resolution SummaryResolved merge conflicts with
Please review: The Verification:
— Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns on re-review. Delta since ccb2eff is a merge from main that swept in #2952's "Prose-bearing args" / egg-orch brc docs and #2963's slice-3 index row, plus a conflict resolution in docs/index.md that correctly keeps both the renamed slice-4 row and main's new slice-3 row. The new content documents agent-friendly mechanisms (--reason-file / stdin sentinels for prose round-tripping, MCP-preferred verb parity) and does not introduce pre-fetched context, post-processing pipelines, or rigid procedural prompts.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review verdict: Changes still requested
The only delta since my prior review (ccb2eff) is the merge commit
a65e4678 resolving conflicts in docs/index.md. Neither of the two
blocking findings from the previous review has been addressed. The
underlying doc text in docs/reference/agent-recovery.md and
docs/guides/concurrent-execution.md is byte-identical to the
ccb2eff state for the cited lines — confirmed via
git diff origin/main..pr-2965 -- docs/reference/agent-recovery.md
(only the unrelated ### Crash Handling in the Event-Pump Wrapper
re-layout, no edits to the table row or the wrapper-classifier prose)
and git show pr-2965:docs/guides/concurrent-execution.md | grep …
(L74/L84/L85/L97/L882 unchanged).
Carried over from prior review (still blocking)
1. docs/reference/agent-recovery.md — "Crash exit-code classification" table still claims a fast-fail path the wrapper does not implement.
Re-verified against pr-2965:orchestrator/consensus_wrapper.py:
133:# * ``is_buffer_overflow`` — the Claude Agent SDK 1 MiB JSON
139:# * ``is_transient_crash`` — signal-based exits (SIGABRT, SIGFPE,
141:# * ``is_startup_failure`` — exit 1 within the
153:is_buffer_overflow() {{
158:is_transient_crash() {{
166:is_startup_failure() {{
Six lines — three comment mentions, three function definitions. Zero dispatch call sites. The wrapper's own L145–150 comment is unambiguous: "The event-pump's propose|ack|nack arm does NOT yet branch on these signals (the consecutive-failure counter + idle-budget alert combo handles the operator-visible escalation today)."
Despite this, the PR's docs/reference/agent-recovery.md table row still reads:
| SDK buffer overflow (exit 255 + overflow marker) | is_buffer_overflow | Wrapper exits immediately; loop does not retry on next event |
That is wrong. There is no buffer-overflow fast-fail path. An SDK 1 MiB JSON overflow exits 255, AGENT_FAIL_STREAK++, sleep 1, and the loop re-runs the same agent invocation, which deterministically hits the same oversized tool result. The operator-visible escalation is EGG_BRC_IDLE_BUDGET_MIN (default 30 min), not an immediate exit.
Fix (same as prior review): Collapse this cell to Increments consecutive-failure counter; idle-budget escalation emits alert — the same content as the other four rows. The prose paragraph at L203 already explains the classifiers are inert helpers; the table currently contradicts the prose.
2. docs/guides/concurrent-execution.md — multiple stale references describe deleted code as if it still ships.
This file describes the wrapper's behaviour in the post-slice-4 world but still asserts mechanics that were deleted with slice-4. Confirmed in the PR at HEAD:
- L84 step 5: "the wrapper restarts the pod with exponential backoff (initial 5 s, doubling, capped at 30 s). These signals are infrastructure-level pod failures that the wrapper still distinguishes from clean event-pump exits via the surviving
is_transient_crash/is_buffer_overflow/is_startup_failureclassifiers." — The wrapper does not restart the pod, does not back off exponentially, and does not invoke any of the listed classifiers (see finding 1). - L85 step 6: "If the agent exits with a non-transient non-zero code, the wrapper exits with the same code, triggering the orchestrator's agent failure path." — The wrapper does not propagate non-zero agent exits in the
propose|ack|nackarm. The post-slice-4 path isAGENT_FAIL_STREAK++and continue the loop; the only escalation surface is the idle/no-progress budget. - L97 config row:
`TRANSIENT_RESTART_BACKOFF_INITIAL` | `5` | ... Doubles after each crash restart, capped at 30 s.—grep -n TRANSIENT_RESTART_BACKOFF pr-2965:orchestrator/consensus_wrapper.pyreturns zero matches. The constant does not exist. - L882 (additional, not in prior review): "Exit codes 134, 136, 137, 139, and 255 are classified as transient and trigger a restart with exponential backoff (starting at 5 s, doubling up to 30 s). If the transient-crash restart succeeds and the agent re-enters the wrapper's event-pump loop ... the failure is fully recovered at the wrapper level — the orchestrator never sees a failure event." — Same staleness in the "Consensus Wrapper Recovery" appendix. There is no transient-crash restart path; no exponential backoff.
These four spots describe a code path that was explicitly deleted in slice-4 and explicitly called out as deleted in the same file at L74 ("The legacy _CONSENSUS_WRAPPER_TEMPLATE … MAX_CONSENSUS_RESTARTS constant … were deleted in slice-4"). The file contradicts itself: the introductory paragraph at L74 says "deleted in slice-4", the step-by-step at L84–L85 and the config table at L97 and the appendix at L882 describe the deleted code as live.
This PR's stated purpose is "Update documentation to reflect changes from c03f316 (#2951)". concurrent-execution.md was the file most affected by that deletion. Sweeping up agent-recovery.md, brc-memory.md, slice-dag.md, etc. while leaving concurrent-execution.md half-done is exactly the inconsistency the PR set out to eliminate.
Fix: In the same PR, update L84/L85/L97/L882 to match the post-slice-4 wrapper. Suggested wording for step 5: "If the agent exits non-zero, the wrapper increments AGENT_FAIL_STREAK, sleeps 1 s, and resumes the loop. There is no per-crash restart or backoff in the event-pump arm; the operator-visible escalation is the idle/no-progress safety budget (EGG_BRC_IDLE_BUDGET_MIN, default 30 min)." Drop the TRANSIENT_RESTART_BACKOFF_INITIAL row from L97. Rewrite L882 to say the same thing.
Verified — merge resolution
docs/index.mdresolution correctly keeps both the PR's renamed "BRC Consensus Wrapper" row (replaces the old "BRC Event-Pump Wrapper" entry) and main's newly-added "BRC Per-Event Prompt Composer + Preamble Collapse" row from #2963. No duplication.- Anchor
#brc-consensus-wrapperresolves todocs/architecture/orchestrator.md:743. ✅ - Anchor
#10-brc-consensus-wrapper-event-pump-modelresolves todocs/reference/agent-wait-patterns.md:1239. ✅
Non-blocking observation
The "BRC Per-Event Prompt Composer + Preamble Collapse" row brought in from main points at architecture/orchestrator.md#brc-per-event-prompt-composer--preamble-collapse-slice-3 and agent-wait-patterns.md#109-brc-per-event-prompt-composer--preamble-collapse-slice-3. The actual headings are ## BRC Per-Event Prompt Composer + Preamble Collapse (no trailing -slice-3) and ### 10.9 BRC Per-Event Prompt Composer + Preamble Collapse (no trailing -slice-3), so GitHub will render these as broken anchors. This is pre-existing in main (introduced by #2963), so it is not blocking on this PR, but the merge brings it into the diff — worth a fast follow-up against main, or a one-line tweak here while the doc-updater is in the area.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…t-pump arm Reviewer flagged that several places in the docs still describe a pre-#2908 capped-restart dispatch path where the wrapper branches on `is_transient_crash` / `is_buffer_overflow` / `is_startup_failure` and restarts with exponential backoff. The post-slice-4 wrapper does not do this: `consensus_wrapper.py` L153-170 defines the classifier helpers but L686-701 (the `propose|ack|nack` arm) calls none of them. Every non-zero agent exit takes the same `AGENT_FAIL_STREAK++` + `sleep 1` + idle-budget path. Updates: docs/reference/agent-recovery.md - "Crash exit-code classification" table: collapse the buffer-overflow row from "Wrapper exits immediately" to the uniform "Increments consecutive-failure counter; idle-budget escalation emits alert" cell shared by every other row, and rename the middle column to "Named helper (currently inert)" so the table no longer reads as a live dispatch matrix. - Add a buffer-overflow note explaining the SDK 1 MiB overflow recurs each iteration until `EGG_BRC_IDLE_BUDGET_MIN` trips. - Rename the "Consensus Wrapper: Transient Crash Recovery" section to "Consensus Wrapper: Exit-Code Classifiers (Preserved Helpers)" with a header callout that the helpers are not invoked today, so the design-intent subsections that follow are framed correctly. - Behavior Matrix row at L388: replace "classified as transient API/network error" with the same idle-budget escalation phrasing as the other wrapper rows. docs/guides/concurrent-execution.md - Step 5 / step 6 of "How it works": replace the exponential-backoff-restart story with the actual `AGENT_FAIL_STREAK++` + idle-budget behaviour. Note explicitly that the classifier helpers exist but are not invoked today. - Drop the `TRANSIENT_RESTART_BACKOFF_INITIAL` configuration row (constant does not exist in the post-slice-4 wrapper). Rewrite the `STARTUP_FAILURE_WINDOW_SECONDS` row to note the helper is inert. - "Transient Crash Recovery" appendix at L880: rewrite to remove claims about per-crash restart, exponential backoff, and classifier-driven recovery. Authored-by: egg
Review feedback addressed (commit 95ec592)Both blocking findings from the prior review (carried over in the re-review at Per-item disposition1. 2.
3. 4. Additional cleanup (out of scope of the explicit findings but caught while editing)While fixing the dispatch claims, I also renamed the Non-blocking observation from the re-review (broken
|
|
Feedback contract violation detected. The response comment posted during this run violates the contract in Violations: — Authored by egg |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
No agent-mode design concerns on re-review. Delta since a65e4678 is a single documentation-correction commit (95ec592) on docs/reference/agent-recovery.md and docs/guides/concurrent-execution.md that brings the prose in line with the actually-deployed event-pump wrapper (inert classifier helpers, uniform AGENT_FAIL_STREAK++ path, no per-crash backoff). No prompts, agent procedures, pre-fetched context, or post-processing pipelines are introduced or changed.
— Authored by egg
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Re-review verdict: Approve
Delta since the prior review (a65e4678) is a single commit 95ec592 that addresses both blocking findings from my last pass. Verified each fix against the source.
Blocking finding 1 — buffer-overflow row in docs/reference/agent-recovery.md table
Fixed. L198 now reads Increments consecutive-failure counter; idle-budget escalation emits alert — the same content as every other row. Middle column renamed from Classifier to Named helper (currently inert) (L195). Header paragraph at L193 explicitly says the classifiers are not invoked. The buffer-overflow follow-up note at L205 spells out the deterministic-recurrence consequence and points the reader at the wrapper's L145–150 comment.
Cross-checked against orchestrator/consensus_wrapper.py:
- L153–176:
is_buffer_overflow/is_transient_crash/is_startup_failuredefined. - L686–701 (
propose|ack|nackarm): onagent_rc != 0, the only work isAGENT_FAIL_STREAK++andsleep 1. No classifier is invoked.AGENT_FAIL_STREAKis only ever read for a log message (L693); it never gates control flow. ✅
Blocking finding 2 — stale references in docs/guides/concurrent-execution.md
All four sites fixed:
- L84 step 5 — rewritten to describe the actual
AGENT_FAIL_STREAK+++sleep 1+ resume-loop behavior, including that the classifiers are defined but not invoked, with a precise pointer toconsensus_wrapper.pyL145–150. ✅ - L85 step 6 — now explicitly says the wrapper does not propagate non-zero exits; the idle budget is the sole escalation surface. ✅
- L97 config table —
TRANSIENT_RESTART_BACKOFF_INITIALrow dropped entirely (verified:grep -n TRANSIENT_RESTART_BACKOFF orchestrator/consensus_wrapper.pyreturns zero matches, the constant does not exist).STARTUP_FAILURE_WINDOW_SECONDSrow rewritten to make explicit the knob has no runtime effect today (verified:STARTUP_FAILURE_WINDOW_SECONDSis referenced only insideis_startup_failureitself at L172, so since the helper is never called, the constant is dead). ✅ - L881 "Transient Crash Recovery" appendix — rewritten end-to-end. Removes the exponential-backoff-restart claim and the "transient-crash restart succeeds and the agent re-enters the wrapper's event-pump loop" framing; replaces with the actual
AGENT_FAIL_STREAK+++ idle-budget behaviour and notes both theMAX_CONSENSUS_RESTARTSandTRANSIENT_RESTART_BACKOFF_INITIALconstants were deleted in slice-4. Anchor link#crash-handling-in-the-event-pump-wrapperresolves to L181 ofagent-recovery.md. ✅
Non-blocking findings 3 and 4 — also fixed
- Finding 3 (table format implies live dispatch) — addressed by the column rename + header paragraph in the finding-1 fix.
- Finding 4 (Behavior Matrix
classified as transient API/network error) — L390 rewritten toidle-budget escalation emits OVERSEER_ALERT — same handling as all non-zero exits today. ✅
Additional cleanup (caught while editing)
The author also renamed the section ### Consensus Wrapper: Transient Crash Recovery → ### Consensus Wrapper: Exit-Code Classifiers (Preserved Helpers) and reframed the subsections that follow as design-intent / preserved-helper context. Anchor-rename safety verified: zero remaining call sites for the old anchor #consensus-wrapper-transient-crash-recovery (grep -rn returns empty across the repo).
The legacy "Agent crashed on Claude Agent SDK buffer overflow… NOT restarting." log block was removed from the Buffer Overflow Detection subsection. Verified the wrapper does not emit that line today (grep -n "NOT restarting" orchestrator/consensus_wrapper.py is empty). ✅
Disposition on the non-blocking observation
The author chose to defer the broken #brc-per-event-prompt-composer--preamble-collapse-slice-3 anchors to a separate PR against main. Acceptable — my prior framing was explicitly "not blocking on this PR" and offered both options. No issue here.
Conclusion
Two blocking findings and two non-blocking findings all properly addressed with edits that match the actual wrapper code state. Documentation is now self-consistent across agent-recovery.md and concurrent-execution.md and matches orchestrator/consensus_wrapper.py HEAD. No new issues introduced.
— Authored by egg
|
egg review completed. View run logs 9 previous review(s) hidden. |
1 participant
Update documentation to reflect changes from c03f316 (#2951):
docs/index.md: fix stale anchor links (pointing to removed#brc-event-pump-wrapper-slice-2-behind-egg_brc_event_pump) and update description to reflect slice-4 is livedocs/reference/orchestrator-cli.md:EGG_BRC_MEMORYdefault corrected fromofftofulldocs/architecture/brc-memory.md: mode table and opening paragraph updated to showfullas new default;offdemoted to rollback escape hatchdocs/development/STRUCTURE.md:consensus_wrapper.pyentry no longer references legacy capped-restart path orEGG_BRC_EVENT_PUMPdocs/guides/sdlc-pipeline.md: step 6 of concurrent phase flow replaced with event-pump idle-budget description (removedMAX_CONSENSUS_RESTARTS/ recovery prompt references)docs/reference/agent-recovery.md: "Restart with Backoff" section and the Before/After table rewritten to describe event-pump crash handling;MAX_CONSENSUS_RESTARTScap references removed from the recovery tableTriggered by: #2951
Authored-by: egg