Let the coder author its own tests; tester reviews-and-hardens by jwbron · Pull Request #2936 · jwbron/egg

jwbron · 2026-06-02T05:10:53Z

Problem

The coder naturally writes tests alongside its source, but the gateway blocked the push (403 restricted_path_modified) because test paths were carved out of the coder's write scope. That work was then either thrown away or smuggled to the tester as a patch — wasted cycles and a coordination gap (the original issue-1707 symptom).

Change

Stop fighting it. Test files are now coder-writable — an intentional overlap with the tester. This retires the #1901 "coder == strict complement of tester" invariant for the test scope only; docs/markdown (documenter) and .egg-state/ stay carved out.

The tester shifts from from-scratch test author to review-and-harden:

Orients from the plan but writes no tests until the coder's CONSENSUS_PROPOSE (inverts the old Tester 'scaffold-first while waiting' prompt instruction not followed in practice — needs observability + enforcement #2249 scaffold-first directive, which existed when the tester authored the first tests).
Then reads the coder's tests, adds the missing regression + adversarial coverage itself (it shares the test scope), runs them, and ACK/NACKs the coder — the NACK naming the failing test is the bug report.

Because the tester waits for the coder's propose, the shared-scope writes are serialized (coder's tests land first; git pull --rebase brings them in before the tester hardens), so concurrent edits to the same test file don't collide.

No review-graph change was needed — the tester → coder CRITICAL edge already is the ACK/NACK channel.

What's in the diff

shared/egg_restrictions/patterns.py — drop tests_globs from the coder's blocklist; document the overlap on both coder + tester patterns. (This is the single source of truth the gateway enforces.)
orchestrator/routes/pipelines.py — rewrite the tester producer-orientation, the dual-role execution-order banner (#2749), the reviewer-preparation block, and the role-task mandate to "orient → wait for coder propose → review-and-harden → propose + ACK/NACK"; update the roster + HANDOFF examples.
docs — reference/agent-roles.md (implement-phase preamble, coder, tester sections), guides/sdlc-pipeline.md, guides/concurrent-execution.md (the worked role-boundary example flips to the still-real direction: tester → coder for a .github/ CI fix), development/STRUCTURE.md.
Retire scripts/scaffold_first_telemetry.py (+ its test) — it measured scaffold-before-propose, which is now the anti-pattern.
Tests — flip the gateway/sandbox/plan-time restriction tests to the new boundaries: coder→tests is allowed; docs/ is the canonical coder-blocked example. The plan-time validate_task_role_alignment tests now treat coder→test assignments as valid and use .github//docs/ for the reject paths.

Testing

gateway/tests/ + shared/tests/ + shared/egg_restrictions/ + the alignment suite: 3796 passed.
orchestrator/tests/test_pipeline_prompts.py: 457 passed.
Pre-existing unrelated failures in shared/egg_contracts/tests/test_orchestrator_phase_id.py / test_composite_execution.py (7) confirmed red on origin/main before this branch.
ruff check + ruff format --check clean on all changed files.

Note: make test selects a very large set here (touching routes/pipelines.py pulls in most of the orchestrator import graph) and exceeds the local wall-clock cap; CI make test-all is the ground truth.

…s-and-hardens The coder naturally writes tests alongside its source, but the gateway 403'd the push (`restricted_path_modified`), so that work was either thrown away or smuggled to the tester as a patch. Stop fighting it: make test files coder-writable (an intentional overlap with the tester, retiring the #1901 strict-complement invariant for the test scope only — docs/markdown and `.egg-state/` stay carved out). The tester's role shifts from from-scratch test author to review-and-harden: it orients from the plan but writes no tests until the coder's CONSENSUS_PROPOSE, then reads the coder's tests, adds the missing regression + adversarial coverage itself (it shares the test scope), runs them, and ACK/NACKs. This inverts the old #2249 scaffold-first directive (which existed when the tester authored the first tests) and serializes the shared-scope writes so concurrent test edits don't collide. Changes: - shared/egg_restrictions/patterns.py: drop tests_globs from the coder's blocklist; document the overlap on coder + tester patterns. - orchestrator/routes/pipelines.py: rewrite the tester producer-orientation, dual-role execution-order banner (#2749), reviewer-preparation, and role-task mandate to wait-for-propose + review-and-harden; update roster + HANDOFF examples. - docs: agent-roles (implement-phase preamble, coder, tester), sdlc-pipeline, concurrent-execution (worked example now tester->coder for CI), STRUCTURE. - Retire scripts/scaffold_first_telemetry.py (+ test) — it measured the now anti-pattern signal. - Update the plan-time role-alignment validator's tests + gateway/sandbox restriction tests to the new boundaries (coder->tests allowed; docs is the canonical coder-blocked example).

james-in-a-box · 2026-06-02T05:28:27Z

Autofix tracking

{"Test/Unit Tests": 2}

The fire thread's time.sleep(0.1) raced the daemon's from_tip snapshot on Python 3.14 CI. If the message landed before the daemon entered get_messages_with_meta, start_idx skipped it and cv.wait blocked until timeout, yielding changed=False. Replace the sleep with a poll on store._cond[pipeline_id] so the message is added only after the daemon has snapshotted the empty store and registered its condition variable.

egg-reviewer

No agent-mode design concerns.

The core change — dropping tests_globs from the coder's blocklist so the coder authors its own tests — aligns well with the "sandbox is the constraint, let the agent operate freely within it" principle. The previous design fought the natural workflow at the gateway (coder wrote tests, gateway 403'd, work was discarded or smuggled). The new shape lets the coder push its own tests and shifts the tester to review-and-harden, which is more faithful to how the agent already wants to work.

Specifics that look good from an agent-mode lens:

No prompt-level pre-fetching introduced; orientation context stays "Plan document, summarized background" / "coder handoff data, task list".
scripts/scaffold_first_telemetry.py (378 lines) and its test (472 lines) are retired — a real reduction in post-processing.
The new tester ordering instruction in _build_brc_preamble / _build_producer_orientation is procedural, but it coordinates BRC timing (which the sandbox cannot enforce) and is explicitly framed as "a directive, not a programmatic gate." The why is provided (nothing to harden until coder proposes; racing in-flight commits churns against a moving target), which gives the agent context for judgment rather than rigid micromanagement.

— Authored by egg

egg-reviewer

Verdict: Request changes

The PR cleanly retires the #1901 "coder == strict complement of tester" invariant for the test scope, updates _build_coder_pattern / _build_tester_pattern, fans the change out across tests consistently, and rewrites the Dual-Role Execution Order banner to describe the new "coder proposes first → tester hardens" flow. That part is good. There is one blocking issue, plus one minor stale doc reference.

Blocking

1. The Reviewer Lifecycle POLL step still carries the scaffold-first guidance and now directly contradicts the new Dual-Role Execution Order banner (orchestrator/routes/pipelines.py, lines 12634–12648).

The new banner (lines 12402–12417) explicitly tells the dual-role tester to do a wait-loop for the coder's CONSENSUS_PROPOSE before producing/proposing:

"1. ORIENT first, then WAIT for the coder's PROPOSE. … Block on egg-orch message wait-loop --for CONSENSUS_PROPOSE for the coder. When the coder proposes, SYNC the worktree, then do your Producer WORK … and PROPOSE your hardening."

But the Reviewer Lifecycle POLL step's is_dual_role branch — which is assembled into the same prompt — still says the opposite:

"For dual-role agents (you), this step folds into Producer Lifecycle step 4 / step 6 — those waits already include --for CONSENSUS_PROPOSE per #2749. Do NOT issue a separate wait-loop --for CONSENSUS_PROPOSE before your own PROPOSE; that would self-block the BRC round (see the Dual-Role Execution Order banner above)."

That second paragraph was correct for the scaffold-first design this PR retires (tester proposed first, then the augmented step-4 / step-6 waits caught the coder's later CONSENSUS_PROPOSE). Under the new design the tester's producer WORK is hardening the coder's tests, so the pre-PROPOSE wait-loop is required, not forbidden — and the "see the Dual-Role Execution Order banner above" cross-reference now points to the opposite instruction.

An agent reading the assembled prompt sees two contradictory directives in the same _build_brc_preamble output. The likely failure modes are (a) the tester follows the POLL guidance and skips the pre-PROPOSE wait → proposes empty/no hardening → the round closes with no hardening pass, or (b) the tester picks the banner and an operator chasing a future bug finds stale guidance still in the prompt and burns time on a phantom self-block.

Fix: rewrite the is_dual_role POLL branch to match the new flow. Two reviewer rendezvous points exist now, and the prompt should name both:

Pre-PROPOSE rendezvous with the coder's first CONSENSUS_PROPOSE — handled by step 1 of the Dual-Role Execution Order banner (the explicit wait-loop --for CONSENSUS_PROPOSE); this is the wait the older guidance forbade and the new design requires.
Post-PROPOSE rendezvous with re-proposes / peer-producer proposals — folded into Producer Lifecycle step 4 / step 6 via the dual-role augmentation, as before.

The "would self-block the BRC round" warning is no longer load-bearing — the coder no longer waits on the tester for anything — so it should be deleted, not relocated. Same for the comment block at lines 12372–12386: line 12385 still says "the tester does not need a second wait-loop for its reviewer POLL after it has proposed", which is true but easy to misread as "no separate wait-loop at all"; tighten it so the pre-PROPOSE wait is unambiguous.

Non-blocking

2. Stale #2530 includes_tests follow-up reference in shared/egg_contracts/plan_parser.py::_check_role_files docstring. The docstring still describes the obsolete includes_tests carve-out as a forward-looking follow-up; with this PR the carve-out collapses into the structural overlap, so the wording is wrong rather than just dated. Trim it to match what the function actually enforces now.

— Authored by egg

Reviewer flagged the Reviewer Lifecycle POLL step's is_dual_role branch as directly contradicting the new Dual-Role Execution Order banner introduced by coder-owns-tests. The banner now requires the tester to issue an explicit `wait-loop --for CONSENSUS_PROPOSE` BEFORE producing (so it has the coder's tests to harden), but the POLL step still carried the scaffold-first guidance forbidding that pre-PROPOSE wait and warning it would self-block the BRC round. An agent reading the assembled prompt would see two opposite directives, with likely failure mode (a) the tester follows POLL, skips the wait, and proposes empty hardening, or (b) an operator chasing a future bug burns time on a phantom self-block. The POLL step now names BOTH rendezvous points: (a) Pre-PROPOSE rendezvous — the explicit wait-loop in banner step 1 that catches the coder's first CONSENSUS_PROPOSE. Required, not forbidden. (b) Post-PROPOSE rendezvous — re-proposes and peer-producer proposals that fold into Producer Lifecycle step 4 / step 6 via the augmented filter. The misleading 'tester does not need a second wait-loop ... after it has proposed' comment block above the banner is rewritten to surface that the tester has TWO reviewer rendezvous points rather than zero, making the pre-PROPOSE wait unambiguous. `test_dual_role_reviewer_poll_redirects_to_producer_wait` is renamed and rewritten to assert the new two-rendezvous-point shape and explicitly check that the pre-#2936 'self-block the BRC round' forbid is gone. Also addresses the non-blocking #2 finding: the stale '#2530 includes_tests follow-up' framing in `_check_role_files` / `validate_task_role_alignment` docstrings is obsolete now that tests_globs is removed from the coder's blocklist — the carve-out collapses into the structural overlap and there is no longer a pending opt-in flag. The docstrings now describe what the function actually enforces. Authored-by: egg

james-in-a-box · 2026-06-02T06:31:08Z

Review feedback dispositions

1. Reviewer Lifecycle POLL contradicts the Dual-Role Execution Order banner (blocking) — fixed-in-PR (commit f7ffc6f)

Rewrote the is_dual_role branch of the Reviewer Lifecycle POLL step (orchestrator/routes/pipelines.py) to name BOTH rendezvous points the coder-owns-tests flow introduces:

(a) Pre-PROPOSE rendezvous — the explicit wait-loop --for CONSENSUS_PROPOSE in banner step 1 that catches the coder's first CONSENSUS_PROPOSE. Now framed as required: the tester's producer WORK is hardening the coder's tests, so there is nothing to propose until the coder has proposed.
(b) Post-PROPOSE rendezvous — re-proposes (version > 1) and peer-producer proposals that fold into Producer Lifecycle step 4 / step 6 via the augmented filter, as before.

The contradictory "Do NOT issue a separate wait-loop --for CONSENSUS_PROPOSE before your own PROPOSE; that would self-block the BRC round" warning is deleted, not relocated — under the new design the coder no longer waits on the tester for anything, so the warning was load-bearing for the retired scaffold-first design only.

Also tightened the comment block above the banner (the "tester does not need a second wait-loop ... after it has proposed" wording at the pre-#2936 lines 12372–12386) to surface that the tester has TWO reviewer rendezvous points rather than zero, making the pre-PROPOSE wait unambiguous.

test_dual_role_reviewer_poll_redirects_to_producer_wait was rewritten and renamed to test_dual_role_reviewer_poll_names_both_rendezvous_points. It now asserts both rendezvous-point labels appear, the explicit wait-loop --for CONSENSUS_PROPOSE reference is present, the Producer Lifecycle step 4 cross-reference still appears, and the pre-#2936 "self-block the BRC round" forbid is absent.

2. Stale #2530 includes_tests follow-up reference in _check_role_files docstring (non-blocking) — fixed-in-PR (commit f7ffc6f)

Trimmed both _check_role_files and validate_task_role_alignment docstrings in shared/egg_contracts/plan_parser.py. The "future includes_tests: true opt-in hook" framing was correct when coder→test-file pairs were the dominant false-positive blocked at the gateway; with tests_globs removed from the coder's blocklist in this PR, the carve-out collapses into the structural overlap and there is no longer a pending opt-in flag. The new wording describes what the function actually enforces (delegation to _is_file_blocked_for_role, mirroring the gateway's push-time check) and keeps the "future role-vs-files exceptions" hint without naming a specific obsolete follow-up.

— Authored by egg

egg-reviewer

No agent-mode design concerns in the delta since 27942cf.

Re-reviewed commit f7ffc6f. It does two things:

Rewrites the Reviewer Lifecycle POLL step's is_dual_role branch to name the two rendezvous points coder-owns-tests introduces (pre-PROPOSE wait-loop for the coder's first CONSENSUS_PROPOSE, post-PROPOSE waits folded into Producer Lifecycle step 4 / step 6) and deletes the now-contradictory "self-block the BRC round" forbid. This resolves the in-prompt contradiction the general code-review bot flagged without expanding the procedural footprint in any agent-mode-relevant direction.
Trims the stale #2530 includes_tests follow-up framing in _check_role_files / validate_task_role_alignment docstrings.

Specifics that look good from an agent-mode lens:

The expanded POLL block is more verbose than the pre-#2936 version, but it stays a directive that coordinates BRC timing the sandbox cannot enforce (which the previous review already approved as appropriate for this surface). It provides the why for both rendezvous points — "your producer WORK is hardening the coder's tests, so there is nothing to propose until the coder has proposed" — so the agent has the rationale needed to judge edge cases rather than rigidly following a script.
The HANDOFF worked-example flip in docs/guides/concurrent-execution.md (coder→tester test handoff replaced with tester→coder .github/ handoff) is healthy from an agent-mode lens: it eliminates a coordination message that existed only because the gateway 403'd a natural workflow, and preserves the pattern for the case that genuinely still needs it.
No new pre-fetching introduced (the orientation context stays at "plan, contract, existing test suite" — no large diffs or full file contents baked into the prompt).
No structured-output-for-humans, no post-processing pipelines, no prompt-level security, no direct API calls bypassing the Agent SDK, no hardcoded model identifiers.

— Authored by egg

egg-reviewer

Verdict: Approve

The two issues from my previous review are fully addressed at f7ffc6f.

Blocking issue 1 — Reviewer POLL contradiction (FIXED)

The is_dual_role branch of the Reviewer Lifecycle POLL step (orchestrator/routes/pipelines.py:12638–12664) is rewritten to name both rendezvous points the coder-owns-tests flow introduces:

(a) Pre-PROPOSE rendezvous — explicitly references the banner step 1 wait-loop --for CONSENSUS_PROPOSE and frames it as required ("You MUST issue it — your producer WORK is hardening the coder's tests, so there is nothing to propose until the coder has proposed").
(b) Post-PROPOSE rendezvous — preserves the augmented producer step 4 / step 6 filter for re-proposes and peer-producer proposals, and correctly scopes the "Do NOT issue a second wait-loop" prohibition to after your own PROPOSE, not before.

The contradictory "Do NOT issue a separate wait-loop … self-block the BRC round" warning is deleted from the production prompt — confirmed by the new test_dual_role_reviewer_poll_names_both_rendezvous_points assertion "self-block the BRC round" not in poll_block, and by rg "self-block the BRC round" returning only the test file. The pre-banner comment block (pipelines.py:12383–12389) is also tightened to surface the two-rendezvous-point structure, so the "tester does not need a second wait-loop after it has proposed" framing can no longer be misread as "no separate wait-loop at all."

Cross-checked against the other surfaces that reference the dual-role wait pattern:

Banner step 2 (pipelines.py:12421–12432) — still says "after your PROPOSE … you do NOT issue a second wait-loop for the reviewer POLL step." Consistent with new POLL block (b).
_brc_preconfirm_wait_line (pipelines.py:12238–12248) — "it folds your reviewer POLL into this wait so you do not issue a second wait-loop" is reached only at step 4 (post-PROPOSE), so the wording is correct in context.
test_dual_role_banner_fall_through_mentions_step_4_review still passes — the new POLL block's post-PROPOSE rendezvous renders "step 4 (REVIEW)" with parens, satisfying that assertion.

Non-blocking issue 2 — Stale `#2530 includes_tests follow-up` (FIXED)

shared/egg_contracts/plan_parser.py docstrings for both _check_role_files and validate_task_role_alignment are rewritten. The "future includes_tests: true opt-in hook" framing is gone; new wording describes what the function actually enforces (delegation to _is_file_blocked_for_role, mirroring the gateway's push-time check) and keeps the "future role-vs-files exceptions" hint without naming an obsolete follow-up. The only remaining #2530 reference repo-wide is in a test comment at shared/egg_contracts/tests/test_validate_task_role_alignment.py:124 that documents why test_coder_with_test_py_is_allowed exists ("used to be the dominant misassignment"), which is appropriate audit-trail context, not a stale forward-looking pointer.

Test exercises the production code path

test_dual_role_reviewer_poll_names_both_rendezvous_points calls _build_brc_preamble("tester", "implement") directly (tester is is_producer and is_reviewer, so is_dual_role = True) and string-slices the actual POLL block from the rendered preamble. No fixture, no parallel implementation — a regression in the prompt template would break the test.

Non-blocking observation

Minor stylistic inconsistency inside the new POLL block: the pre-PROPOSE branch reads "step 3 SYNC → step 4 REVIEW → step 5 ACK/NACK" (no parens, pipelines.py:12650) while the post-PROPOSE branch reads "step 3 (SYNC) → step 4 (REVIEW) → step 5 (ACK/NACK)" (with parens, pipelines.py:12660–12661). Existing tests pin the parenthesized form via the post-PROPOSE branch, so both are present. Worth aligning for readability but not blocking.

CI is green on f7ffc6f (Unit Tests, Integration Tests, all lint passes).

— Authored by egg

Pre-PROPOSE branch rendered step labels without parens ("step 3 SYNC"); post-PROPOSE branch used parens ("step 3 (SYNC)"). Align the pre-PROPOSE branch to match for readability — the bot reviewer flagged this as a non-blocking stylistic inconsistency on f7ffc6f. Existing tests only assert label presence, not paren form, so no test changes are needed. Authored-by: egg

james-in-a-box · 2026-06-02T06:51:32Z

Review feedback dispositions

Latest review (commit f7ffc6f) was Approve — the two previously-blocking issues are fixed. One non-blocking observation remained:

1. Stylistic inconsistency: step-label parens in dual-role POLL block (non-blocking) — fixed-in-PR (commit 7f989a8)

The pre-PROPOSE branch rendered the fall-through path without parens around the step labels ("step 3 SYNC → step 4 REVIEW → step 5 ACK/NACK", orchestrator/routes/pipelines.py:12650-12651) while the post-PROPOSE branch used parens ("step 3 (SYNC) → step 4 (REVIEW) → step 5 (ACK/NACK)", lines 12660-12661). Aligned the pre-PROPOSE branch to the parenthesized form — the post-PROPOSE form is the one already pinned by the existing test_dual_role_banner_fall_through_mentions_step_4_review assertion, so converging there avoids a second pinned form.

Verified test_dual_role_reviewer_poll_names_both_rendezvous_points, test_pure_reviewer_poll_unchanged, and test_dual_role_banner_fall_through_mentions_step_4_review still pass (3 passed). ruff check / ruff format --check clean. No production logic change — pure prompt-text alignment.

— Authored by egg

egg-reviewer

No agent-mode design concerns. The delta since f7ffc6f is a 2-line stylistic fix to the dual-role POLL pre-PROPOSE branch — aligning step labels to use parens (step 3 (SYNC)) so they match the post-PROPOSE branch. Pure formatting; no impact on agent-mode design.

— Authored by egg

egg-reviewer

Verdict: Approve

The delta since f7ffc6f is a 2-line cosmetic alignment in orchestrator/routes/pipelines.py — adding parens around the step labels in the dual-role POLL pre-PROPOSE branch (step 3 (SYNC) → step 4 (REVIEW) → step 5 (ACK/NACK), lines 12650–12651). This addresses the non-blocking stylistic inconsistency I flagged on f7ffc6f.

Verification

Form converged to the pinned variant. The post-PROPOSE branch (lines 12660–12661) already used (SYNC) → (REVIEW) → (ACK/NACK), and test_dual_role_banner_fall_through_mentions_step_4_review asserts "step 4 (REVIEW)" in poll_block. Aligning the pre-PROPOSE branch to the same form means a single, test-pinned shape across both branches instead of two divergent forms — exactly the right choice.
No logic touched. git diff f7ffc6f..7f989a8 --stat shows orchestrator/routes/pipelines.py | 4 ++-- and no other file. Diff inspection confirms the edits are inside the prose string literal of the is_dual_role POLL branch; no control flow, no other branches, no test updates needed.
Repo-wide consistency check. rg "step \d+ \((SYNC|REVIEW|ACK)" now returns four call sites — the banner block (12427–12429), the POLL pre-PROPOSE branch (12650–12651, this change), the POLL post-PROPOSE branch (12660–12662), and the test assertions at lines 5499/5509/5510/5517/5518. All five sites use the parenthesized form; there are no remaining un-parenthesized occurrences.
Targeted tests pass. test_dual_role_reviewer_poll_names_both_rendezvous_points, test_pure_reviewer_poll_unchanged, and test_dual_role_banner_fall_through_mentions_step_4_review all pass locally (3/3). CI is green on 7f989a8 (Unit Tests, Integration Tests, all lint passes).
No new tests required. The disposition comment's claim — "Existing tests only assert label presence, not paren form" — is accurate for the pre-PROPOSE branch; the only assertion that pins the parenthesized form is on the post-PROPOSE branch / banner (which already had it), so the converged form remains pinned and there is no gap.

No further concerns.

— Authored by egg

james-in-a-box · 2026-06-02T07:11:26Z

egg review completed. View run logs

13 previous review(s) hidden.

…r] (#2943) * docs: update tester role description for coder-owns-tests model Correct stale descriptions of the tester's responsibilities in README.md and docs/guides/agent-teams.md to reflect that the coder now authors its own tests (#2936). The tester no longer writes regression tests from scratch; it reviews-and-hardens the coder's tests, adds missing coverage, and adversarially probes the implementation. Authored-by: egg * docs: update Agent Roles table in sdlc-pipeline.md for coder-owns-tests The Agent Roles table in docs/guides/sdlc-pipeline.md still described the pre-f8d320a50 model. Update both stale rows: - Coder File Access: remove "tests" from the except-list (tests are coder-writable per shared/egg_restrictions/patterns.py); note the intentional overlap with the tester. - Tester Purpose: replace the from-scratch-author framing with the reviews-and-hardens model, mirroring the README.md update. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>

* feat(restrictions): let the coder author its own tests; tester reviews-and-hardens The coder naturally writes tests alongside its source, but the gateway 403'd the push (`restricted_path_modified`), so that work was either thrown away or smuggled to the tester as a patch. Stop fighting it: make test files coder-writable (an intentional overlap with the tester, retiring the #1901 strict-complement invariant for the test scope only — docs/markdown and `.egg-state/` stay carved out). The tester's role shifts from from-scratch test author to review-and-harden: it orients from the plan but writes no tests until the coder's CONSENSUS_PROPOSE, then reads the coder's tests, adds the missing regression + adversarial coverage itself (it shares the test scope), runs them, and ACK/NACKs. This inverts the old #2249 scaffold-first directive (which existed when the tester authored the first tests) and serializes the shared-scope writes so concurrent test edits don't collide. Changes: - shared/egg_restrictions/patterns.py: drop tests_globs from the coder's blocklist; document the overlap on coder + tester patterns. - orchestrator/routes/pipelines.py: rewrite the tester producer-orientation, dual-role execution-order banner (#2749), reviewer-preparation, and role-task mandate to wait-for-propose + review-and-harden; update roster + HANDOFF examples. - docs: agent-roles (implement-phase preamble, coder, tester), sdlc-pipeline, concurrent-execution (worked example now tester->coder for CI), STRUCTURE. - Retire scripts/scaffold_first_telemetry.py (+ test) — it measured the now anti-pattern signal. - Update the plan-time role-alignment validator's tests + gateway/sandbox restriction tests to the new boundaries (coder->tests allowed; docs is the canonical coder-blocked example). * Fix checks: update interceptor pipeline-repo tests for coder-owns-tests * Deterministic wait in test_overseer_alert_wakes_route The fire thread's time.sleep(0.1) raced the daemon's from_tip snapshot on Python 3.14 CI. If the message landed before the daemon entered get_messages_with_meta, start_idx skipped it and cv.wait blocked until timeout, yielding changed=False. Replace the sleep with a poll on store._cond[pipeline_id] so the message is added only after the daemon has snapshotted the empty store and registered its condition variable. * Fix dual-role POLL contradiction; clear stale #2530 docstring Reviewer flagged the Reviewer Lifecycle POLL step's is_dual_role branch as directly contradicting the new Dual-Role Execution Order banner introduced by coder-owns-tests. The banner now requires the tester to issue an explicit `wait-loop --for CONSENSUS_PROPOSE` BEFORE producing (so it has the coder's tests to harden), but the POLL step still carried the scaffold-first guidance forbidding that pre-PROPOSE wait and warning it would self-block the BRC round. An agent reading the assembled prompt would see two opposite directives, with likely failure mode (a) the tester follows POLL, skips the wait, and proposes empty hardening, or (b) an operator chasing a future bug burns time on a phantom self-block. The POLL step now names BOTH rendezvous points: (a) Pre-PROPOSE rendezvous — the explicit wait-loop in banner step 1 that catches the coder's first CONSENSUS_PROPOSE. Required, not forbidden. (b) Post-PROPOSE rendezvous — re-proposes and peer-producer proposals that fold into Producer Lifecycle step 4 / step 6 via the augmented filter. The misleading 'tester does not need a second wait-loop ... after it has proposed' comment block above the banner is rewritten to surface that the tester has TWO reviewer rendezvous points rather than zero, making the pre-PROPOSE wait unambiguous. `test_dual_role_reviewer_poll_redirects_to_producer_wait` is renamed and rewritten to assert the new two-rendezvous-point shape and explicitly check that the pre-#2936 'self-block the BRC round' forbid is gone. Also addresses the non-blocking #2 finding: the stale '#2530 includes_tests follow-up' framing in `_check_role_files` / `validate_task_role_alignment` docstrings is obsolete now that tests_globs is removed from the coder's blocklist — the carve-out collapses into the structural overlap and there is no longer a pending opt-in flag. The docstrings now describe what the function actually enforces. Authored-by: egg * Align step-label parens in dual-role POLL pre-PROPOSE branch Pre-PROPOSE branch rendered step labels without parens ("step 3 SYNC"); post-PROPOSE branch used parens ("step 3 (SYNC)"). Align the pre-PROPOSE branch to match for readability — the bot reviewer flagged this as a non-blocking stylistic inconsistency on f7ffc6f. Existing tests only assert label presence, not paren form, so no test changes are needed. Authored-by: egg --------- Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>

…r] (#2943) * docs: update tester role description for coder-owns-tests model Correct stale descriptions of the tester's responsibilities in README.md and docs/guides/agent-teams.md to reflect that the coder now authors its own tests (#2936). The tester no longer writes regression tests from scratch; it reviews-and-hardens the coder's tests, adds missing coverage, and adversarially probes the implementation. Authored-by: egg * docs: update Agent Roles table in sdlc-pipeline.md for coder-owns-tests The Agent Roles table in docs/guides/sdlc-pipeline.md still described the pre-f8d320a50 model. Update both stale rows: - Coder File Access: remove "tests" from the except-list (tests are coder-writable per shared/egg_restrictions/patterns.py); note the intentional overlap with the tester. - Tester Purpose: replace the from-scratch-author framing with the reviews-and-hardens model, mirroring the README.md update. --------- Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>

…ts, update legacy preamble tests The coder-owns-tests policy (#2936) means the slice-3 coder authors the initial test scaffold for TASK-3-6 (compose_event_prompt) and TASK-3-7 (collapsed preamble snapshot); the tester reviews-and-hardens in their own pass. New tests: * ``orchestrator/tests/test_compose_event_prompt.py`` (TASK-3-6) — prompt-shape tests per role variant (producer / reviewer / dual-role), memory-excerpt truncation at the 2 KB cap, open-NACK rendering across 0 / 1 / 2+ reviewers (the #2142 aggregated barrier case), verbatim ``git log {sha}..HEAD --not origin/{base_branch} -p`` command emission (with a regression guard against future ``changed_artifacts``-only shortcut per REVIEWER-SYNC.md + risk_analyst R6), envelope-budget assertion (≤ 10 KB excluding the rendered delta, which scales with the change), and defensive shape (None inputs, empty role, empty base branch). * ``orchestrator/tests/test_brc_preamble_collapsed.py`` (TASK-3-7) — three role-shape snapshots (coder / reviewer_code / tester); absent-strings (STAY-ALIVE, positive wait-loop instructions, cursor-threading, ready_to_confirm STATUS-nudge); kept-strings ("Both must pass to ACK", dual-mandate banner, agent roster, producer/reviewer lifecycle skeleton); event-handler contract framing replaces the legacy "you have FAILED your role" warning; byte-size drop ≥ 25% per role variant. Legacy preamble tests updated to match the slice-3 collapsed shape in ``orchestrator/tests/test_pipeline_prompts.py`` and ``orchestrator/tests/test_concurrent_integration.py``: * Updated (retargeted assertions, preserved purpose): - ``TestBrcPreambleSyncStep::test_reviewer_sync_step_after_poll`` (POLL → INVOKED PER EVENT) - ``TestBrcPreambleSyncStep::test_reviewer_lifecycle_renumbered`` (steps 1-7, STAY ALIVE deleted) - ``TestAdversarialReReviewPriming::test_reviewer_lifecycle_step8_carries_adversarial_framing`` (banner moved from step 8 to step 7; substantive content preserved) - ``TestAdversarialReReviewPriming::test_producer_respond_to_reviews_legitimizes_new_findings`` (NACK pushback paragraph condensed but preserved) - ``TestDirectedCoordinationGuidance::test_directed_coordination_before_exit_warning`` (precedes the Event-handler contract instead of the deleted "you have FAILED" warning) - Three ``TestDualRoleExecutionOrdering`` tests (banner-presence + REVIEW token; wait-loop allowlist references dropped) - ``TestConcurrentPromptLifecycle::test_concurrent_prompt_includes_lifecycle_preamble`` (asserts the slice-3 Event-handler-contract framing instead of the legacy STAY-ALIVE / "FAILED your role" framing) * Deleted (entirely about deleted functionality): - ``TestReviewerPollUsesWaitLoop`` (POLL is gone) - ``TestReviewerWaitLoopMentionsAutoCursor`` (cursor-threading is gone) - ``TestProducerRespondToReviewsWaitLoop::test_step4_lists_pre_confirm_allowlist`` and ``test_step4_explains_status_ready_to_confirm_nudge`` (the wait-loop allowlist is gone; the orchestrator's ``brc next-action`` route drives the producer pre-confirm waits now). The ``test_step4_excludes_consensus_confirmed`` regression guard is preserved. - Seven ``TestDualRoleExecutionOrdering`` tests that pinned the wait-loop allowlist content of the banner. - ``TestConcurrentPromptLifecycle::test_reviewer_stay_alive_uses_canonical_for_list`` (STAY ALIVE is gone). The deletions preserve issue cross-references in adjacent comments so the audit trail (#1943, #2064, #2323, #2482, #2531, #2749) is not lost. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

The slice-3 task-3-3 patch was authored against a base that included the #2936 coder-owns-tests rewrite; rebasing onto origin/slice-3 (which precedes #2936) required two small reconciliations: * Dual-role banner preface — restored ``propose right after`` and ``self-block`` phrasings (required by ``test_dual_role_banner_states_propose_first``) while keeping the event-pump framing from the slice-3 collapse. * ``test_no_positive_wait_loop_instructions`` — broadened the negative-qualifier allowlist to accept ``Do NOT call`` / ``do NOT call`` so the producer-orientation copy (``Do NOT call `wait-loop`...``) and the new banner preface (``Do NOT block on a reviewer wait...``) both satisfy the regression guard. The intent is unchanged: any line mentioning ``wait-loop`` must qualify it with a negative directive. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

…ble collapse Adversarial coverage added on top of the coder-authored test scaffolds for TASK-3-6 (compose_event_prompt) and TASK-3-7 (collapsed preamble). Per #2936 the coder authors its tests; the tester reviews-and-hardens in their own pass. Probes the boundaries the coder-authored happy-path tests do not: * ``_truncate`` exact-boundary + just-over-boundary semantics (the off-by-one at the cap was not pinned). * Multi-byte UTF-8 memory excerpt — truncation respects the code-point cap not the byte cap; envelope still respected. * Whitespace-only memory excerpt → section omitted (strip() guard). * Producer-delta iteration order preserves caller order (no implicit sort — sorting belongs to ``_build_delta_entries``'s memory-enum fallback, not the renderer). * ``(no commits in range — re-review is a no-op)`` sentinel for an empty delta string (the post-confirm re-confirm shape). * Producer-delta entries with missing keys render defensive defaults (``(unknown)`` producer label, ``<no prior review>`` SHA sentinel). * Non-string delta is coerced via ``str()`` rather than crashing. * NACK with missing ``reviewer`` / ``reason`` / non-list ``artifact_refs`` renders sentinels rather than crashing. * ``_parse_per_producer_sha`` edge cases — ``-`` sentinel skip, first-match-wins per heading, orphan bullet ignored, backtick-wrapped heading canonicalised. * ``_extract_nacks`` third-priority ``aggregated_nacks`` fallback (the coder pins ``nacks`` > ``unresolved_nacks``; this pins the third tier). * ``_extract_nacks`` drops non-dict entries silently. * ``_extract_changed_artifacts`` filters ``None`` / whitespace; defensive against non-dict payloads. * ``_extract_current_producers`` robust against mixed-shape ``pending_reviews`` entries; defensive against non-dict payloads. * ``_extract_artifacts_for_producer`` priority: ``pending_reviews`` > top-level ``changed_artifacts``; empty / whitespace producer returns empty list. * ``_run_git_log`` subprocess error paths — timeout sentinel, non-zero rc sentinel with stderr, 256 KiB truncation marker. * CLI: ``--event-payload-file`` reads from disk; missing file returns rc=2 with explicit error; malformed-JSON stdin falls back to surfacing the raw payload under ``raw``; empty stdin falls back to ``{"action": <argv>}`` action-only payload. Hardens the preamble-collapse contract beyond the line-by-line ``wait-loop`` qualifier check: * Positive ``egg-orch message wait-loop`` invocations forbidden (each occurrence must sit inside a ±200-char window with a ``do NOT`` / ``Do NOT`` / ``not block on`` / ``not call`` / ``not issue`` negation anchor — catches multi-line code blocks). * ``--for CONSENSUS_*`` filter-flag patterns absent (legacy wait-loop plumbing). * ``never exit`` warning gone (TASK-3-3 explicit collapse target; pairs with the coder's ``"you have FAILED your role"`` check). * ``/tmp/egg-wait-cursor-`` plumbing path absent (full-text scan, complements the substring check). * Agent roster names the producer + reviewer roles, not just the heading. * No ``Ready to confirm — all confirm preconditions satisfied`` STATUS-nudge anchors (#2531 plumbing collapsed). * Tester preamble fits inside a 20 KB ceiling (absolute bound complements the ≥ 25% relative-drop assertion against the hardcoded baseline — a runaway re-expansion fails on both anchors). * Unknown role doesn't crash; the agent-roster section still renders. * Event-handler contract surfaces the wrapper-owned-wait framing (wrapper / drives your lifecycle / one-shot) — pins the central intent of the collapse beyond merely the contract heading. All 115 tests pass under PYTHONPATH=. pytest; ruff lint + format clean on both files. Configured ``make test`` / ``make lint`` / ``make security`` cannot run end-to-end in this pod because the venv-sync step fails to fetch dev wheels (no PyPI egress) — see the propose attestation for the ``tests_execution_blocked`` rationale and the slice-3-scoped check counts that DID execute directly.

Five blocking findings + three non-blockers from reviewer_code's v1 NACK on PR-proposal v1. Address all five blockers; defer the non-blockers (env-var coordination is gated on coder task-4-1 / task-4-2 landing first). Blocker 1+2+3 — restored post-#2936 coder-owns-tests content in docs/guides/concurrent-execution.md (the v1 proposal overwrote the post-#2936 wording when I copied the slice-4 base, which predates the #2936 merge): - §"HANDOFF" table row example: "Coder can't push test files → HANDOFF to tester" → "Tester can't push a .github/ CI fix → HANDOFF to coder with the required end-state" (matches docs/reference/agent-roles.md). - §"Worked Example: Role-Boundary Handoff" rewrite: drop the reinstated pre-#2936 coder→tester test-handoff example, restore the post-#2936 tester→coder .github/-staging example, and keep the explicit lead sentence "the coder→tester test handoff that used to live here is gone: the coder now authors and pushes its own tests". - §"Rebase rarely conflicts" paragraph: "Rebase cannot conflict because agents have mutually exclusive file write permissions" / "role restrictions guarantee non-overlapping file sets" was a doc lie after #2936 — restore the pre-rewrite "rarely conflicts" wording and the follow-up paragraph that names the shared test scope and the serialize-by-time-not-concurrent invariant. Blocker 4+5 — dead anchors and stale §10 / §10.9 framing in docs/reference/agent-wait-patterns.md and docs/architecture/brc-memory.md: - agent-wait-patterns.md §10 retitled from "BRC Event-Pump Wrapper (slice-2, behind EGG_BRC_EVENT_PUMP)" to "BRC Consensus Wrapper (event-pump model)"; intro blockquote rewritten to drop the slice-2 "OFF by default" framing and instead describe the post-deletion steady state + rollback path; §10.8 retitled from "Flag-off as the temporary default — when slice-4 flips it" to "Rollout completed in slice-4" with body rewritten accordingly; §10.9 retitled to drop the (slice-3) suffix and the "Flag mapping" blockquote rewritten to "What's gated by what" since EGG_BRC_EVENT_PUMP no longer gates anything. - All five dead inbound anchor references repointed to the new anchors: agent-wait-patterns.md lines 1178, 1411, 1653, 1654 and brc-memory.md lines 235, 237. - Reverse direction — repointed orchestrator.md, README.md, and concurrent-execution.md cross-links from #10-brc-event-pump-wrapper-slice-2-behind-egg_brc_event_pump to #10-brc-consensus-wrapper-event-pump-model (3 occurrences in orchestrator.md, 1 each in README.md and concurrent-execution.md); same for #109-brc-per-event-prompt-composer--preamble-collapse-slice-3 → #109-brc-per-event-prompt-composer--preamble-collapse. - Verified via grep across docs/ that no remaining link points at the old anchors and no remaining body text carries the "(slice-2, behind EGG_BRC_EVENT_PUMP)" framing. Non-blockers deferred: - Rollback-plan example precision (compose_event_prompt slice-3 vs slice-2 wrapper) — useful tightening but doesn't change the correctness of the doc. - "schema is unchanged" past-tense alignment — minor. - EGG_BRC_EVENT_PUMP "no-op" vs "removed" wording — gated on coder's task-4-1 / task-4-2 final state. Will re-pass once the coder's proposal lands so the doc and code agree.

…r branch The slice-4 coder branch was created based on main but the slice-4 parent at origin/egg/issue-2908-impl2/slice-4 has the slice-1, slice-2, and slice-3 implementation work that slice-4 builds on. Merge it in before doing task-4-1 / task-4-2. Conflicts resolved in orchestrator/routes/pipelines.py and orchestrator/tests/test_pipeline_prompts.py — both in the BRC preamble dual-role banner. The coder-owns-tests refinement (#2936) that landed on main and the event-pump banner collapse from slice-3 task-3-3 of #2908 both touch the dual-role banner text. Resolution: keep slice-3's event-pump structure (no wait-loop filter allowlists in the preamble — the wrapper drives invocation) and layer in the coder-owns-tests semantics on top: the tester's first invocation does ORIENT/PREPARE only; the wrapper re-invokes it on the coder's CONSENSUS_PROPOSE; it does its producer WORK (review + harden the coder's tests) + PROPOSE + ACK/NACK in that single invocation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

…r branch Documenter landed task-4-4 (post-deletion consensus-wrapper docs) on the slice-4 branch in parallel with the coder's task-4-1 + task-4-2 work. Their merge resolved the dual-role banner conflict by taking the slice-3 base text (general event-pump description). This commit keeps the coder's resolution (coder-owns-tests refined: tester's first invocation does ORIENT/PREPARE only, wrapper re-invokes on coder's CONSENSUS_PROPOSE carrying proposal in event_payload, tester does producer WORK + PROPOSE + ACK/NACK in that single invocation) since the main-branch coder-owns-tests semantics (#2936) need to be preserved under the event-pump model. The other documenter changes (orchestrator.md, brc-memory.md cross-link, etc.) flow through unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

…ocker #2 (test_pipeline_prompts fixture) Follow-up to v3 (e093f67 pushed) that addressed the reviewer_code v2 blocker (missing import sys). This commit addresses the reviewer_code_holistic v2 blocker (2): two pre-existing test failures in orchestrator/tests/test_pipeline_prompts.py. Root cause: the slice-4 base-merge in 06c5a6c resolved the conflict on test_pipeline_prompts.py by keeping slice-3's _PLAN_WITH_MISASSIGNED_TASK fixture (``role: coder`` + ``files: integration_tests/conftest.py``). But main's #2936 ("coder authors its own tests; tester reviews-and- hardens") explicitly excluded coder→test-files from the role↔files alignment validator. The fixture no longer trips the reject path, breaking TestPlannerRoleAlignmentValidation::test_rejects_misassigned_plan_at_propose_time and ::test_rejected_proposal_does_not_mutate_tracker. Fix: cherry-pick main's fixture update — switch the misassignment fixture from a test-file path to a docs path (docs/fixtures.md), which IS still a misassignment, since docs remain the documenter's scope. Added an explanatory comment above the fixture citing #2936 and the slice-3 merge-resolution context so future readers do not re-revert under a conflict resolution that "looks like" the slice-3 text. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

…s in dual-role banner The conflicts in orchestrator/routes/pipelines.py and orchestrator/tests/test_pipeline_prompts.py both touched the BRC dual-role banner. main updated it for 'coder-owns-tests' (#2749 follow-up #2936) using wait-loop semantics; slice-3 of #2908 is collapsing exactly those wait-loop / STAY-ALIVE semantics in favor of the event-pump wrapper that invokes the agent one-shot per event. Accepted HEAD's event-pump version for both files — main's wait-loop guidance is replaced by the wrapper-driven re-invocation framing that already encodes the coder-tester rendezvous ("the wrapper re-invokes you when that proposal arrives; you propose right after").

…role banner conflicts slice-3's main-merge resolution settled the BRC dual-role banner on the generic event-pump version (dropping main's #2936 wait-loop banner). slice-4 had already reconciled both concerns: its banner carries the event-pump-wrapper framing AND the coder-owns-tests (#2936) tester specialization ('harden the coder's tests', re-invoke on the coder's CONSENSUS_PROPOSE). slice-4's banner is the strict superset, so accepted HEAD for both pipelines.py and test_pipeline_prompts.py — it preserves the coder-owns-tests language while keeping the event-pump framing #2908 requires.

The merge resolution in 39a08bb kept HEAD's banner verbatim while main's #2936 (coder-owns-tests) auto-merged into surrounding sections. The banner directed the tester to "execute Producer steps 1-3 FIRST" and "draft scaffolding" while the surrounding producer orientation tells the tester "Orient only until the coder proposes" and "do NOT write test files before the coder's CONSENSUS_PROPOSE". Two opposite directives in the same prompt. Fix (banner-only edit; surrounding sections already correct): - Remove "draft scaffolding" from the opportunistic-prepare list (it's the scaffold-first directive in disguise). - Reword "Producer steps 1-3 come FIRST" to "Producer ORIENT (step 1) comes FIRST" + an explicit pivot on the role-specific orientation for whether WORK (step 2) runs immediately or is gated on the upstream CONSENSUS_PROPOSE. - Name the tester case explicitly (the dual-role agent whose WORK is gated per #2936) so the agent reading the banner can reconcile it with its role-specific orientation. - Keep "the wrapper re-invokes you when that proposal arrives" and "propose right after" — slice-3's event-pump framing remains correct. Add ``test_dual_role_banner_does_not_contradict_coder_owns_tests`` that scopes assertions to the banner SUBSTRING (between ``### Dual-Role Execution Order`` and ``### Producer Lifecycle``) so a future banner-only edit can't sneak the scaffold-first prose back in. Pairs with the existing ``test_tester_orientation_directs_review_and_harden_after_propose`` which covers ``_build_producer_orientation`` — together they pin both halves of the prompt the merge resolution desynced.

…t-log attribution from main Per egg-reviewer feedback on PR #2951, the slice-3 -> slice-4 merge (commit 06c5a6c) accidentally dropped fixes that landed on main and slice-3 between the slice-4 branch fork and the merge. The slice-4 v4 commit restored _auto_populate_contract_at_implement_start but missed several other surfaces in the same incident. Restored from slice-3 (post-#2936 coder-owns-tests framing): - _ROLE_DESCRIPTIONS tester + coder entries - _build_reviewer_preparation tester block - _build_producer_orientation tester banner - _build_agent_prompt tester branch - coder->tester HANDOFF body - _resolve_slice_base_branch derived_parent variable usage - test_tester_prep_waits_for_coder_before_writing_tests - test_tester_orientation_directs_review_and_harden_after_propose Restored from main (#2893 / #2919 audit-log attribution): - _start_stacked_pr_reconciler._list_extant_branches: orchestrator role + explanatory comment - _start_stacked_pr_reconciler._rebase_onto: orchestrator role + explanatory comment - _run_implement_phase_slices bootstrap is_slice_branch_merged_into_parent: orchestrator role + explanatory comment - _run_implement_phase_slices spawn is_slice_branch_merged_into_parent: orchestrator role + explanatory comment - _run_implement_phase_slices create_slice_integration_branch: orchestrator role + explanatory comment The slice-3 restoration brings 1 of the 6 audit-log fixes (the list_open_prs call at line 15627 was already correct on slice-3); the remaining 5 are restored directly from main since slice-3 forked before those #2919 hunks landed. After this commit, all 6 stacked-PR-reconciler hops attribute their synthetic-session gateway calls to agent_role="orchestrator" so the audit log identifies the actual caller instead of impersonating a coder. Authored-by: egg

…2949) * docs(#2908 slice-3 task-3-4..3-5): event-handler mission.md + slice-3 doc surface Documenter tasks for slice-3 of #2908: task-3-4: Rewrite the Concurrent Execution Mode section of sandbox/agent-config/rules/mission.md to the event-handler contract: the agent is invoked one-shot per actionable event by the BRC event-pump wrapper; act on the event, update BRC memory, exit naturally. Removes "never exit before the orchestrator stops you", the producer/reviewer STAY-ALIVE wording, and the explicit wait-loop / cursor plumbing — under the post-#2908 model the wrapper bash owns the lifecycle. Preserves Anti-Sycophancy, Structured Progress Reporting, HITL vs OVERSEER_ALERT, and Handling Agent Failures sections verbatim. A legacy-path note preserves the pre-flip contract until slice-4 flips the default. sandbox/claude-rules/mission.md is a symlink to sandbox/agent-config/rules/, so both paths reflect the change and the diff-empty acceptance assertion holds trivially. task-3-5: Extend docs/architecture/orchestrator.md and docs/reference/agent-wait-patterns.md with the slice-3 surface: the compose_event_prompt composer (10 KB envelope, 2 KB memory truncation, NACK payload from peer_consensus.py:949-1024, verbatim per-producer git-log delta scaled by change size), the full-delta adversarial re-review rationale (REVIEWER-SYNC.md contract + risk_analyst R6), tail-position memory delivery (architect od-6 Option B, sidesteps the non-existent --append-context flag), composer interplay with EGG_BRC_MEMORY across off / write-only / full modes, the _build_brc_preamble collapse (kept/removed table + the three caller sites unchanged), the sandbox-image rebuild trigger gating slice-4's flag flip, and the slice-3 unit/snapshot verification stance. Documents the architect's open-decision resolutions od-1 / od-2 / od-3 / od-4 / od-6 with their slice-1 / slice-2 / slice-3 implementation cites and explicitly marks od-5 as deferred to slice-6's MCP→CLI latency baseline. Cross-links between brc-memory.md (slice-1 writer), orchestrator.md (slice-3 architecture), and agent-wait-patterns.md §10.9 (slice-3 wait-side companion); brc-memory.md "How slice-3 reads it" now points to the landed reader-side sections. Refs #2908 tasks 3-4, 3-5 * docs(#2908 slice-3): address reviewer_code v1 non-blocking observations Three substantive corrections from reviewer_code's v1 ACK observations: 1. **Preamble collapse is unconditional, not gated by EGG_BRC_EVENT_PUMP.** The legacy-path note in mission.md previously implied the legacy wrapper still carried "the full event-blocking / cursor / persistent-session plumbing" in the preamble — but task-3-3 collapses _build_brc_preamble unconditionally, so both wrapper paths see the collapsed preamble. The legacy wrapper re-supplies wait / restart instructions through its own recovery system prompt baked into _CONSENSUS_WRAPPER_TEMPLATE, not through the preamble. Reword to clarify what the EGG_BRC_EVENT_PUMP flag actually selects (the wrapper, not the preamble), and propagate the same clarification through orchestrator.md and §10.9 of agent-wait-patterns.md. 2. **Heading "(behind EGG_BRC_MEMORY=full)" was misleading.** The composer itself runs whenever the event-pump branch runs (EGG_BRC_EVENT_PUMP), and only the memory-excerpt *content* is gated by EGG_BRC_MEMORY. The matrix table got this right but the section heading suggested the whole composer was behind the memory flag. Drop the "behind ..." qualifier from the H2/H3 headings and add a "Flag mapping (read this first)" callout at the top of both the orchestrator.md and agent-wait-patterns.md sections. Update all four cross-link anchors (orchestrator.md, agent-wait-patterns.md, brc-memory.md) to the new slug. 3. **Line-number references in a 24.5k-line file are doc-drift prone.** Add an explicit caveat noting the numbers come from the slice-3 contract spec and reflect post-collapse positions, and lean on function- and banner-name references (_build_brc_preamble, "the dual-mandate banner") for reading the live file. Apply the same softening in agent-wait-patterns.md §10.9.5. The reviewer also flagged that the sandbox image rebuild is not exercised by this slice — that's a slice-4 coordination obligation sitting outside the BRC review surface (documenter cannot exercise make build / make k3s-import / make deploy from the pod); the docs record the rebuild-trigger as required by task-3-4's acceptance criterion so slice-4 can verify the new image deployed before flipping EGG_BRC_EVENT_PUMP. No content change needed for that one. Refs #2908 tasks 3-4, 3-5 (re-propose v2 after reviewer_code ACK) * docs(#2908 slice-3): address reviewer_code v2 non-blocking observations Three substantive corrections from reviewer_code's v2 ACK observations. None are blocking; each addresses a factual / drift risk the reviewer surfaced inline. 1. **mission.md doc cross-references now use $EGG_REPO_PATH/docs/... convention.** The three new links at mission.md:153 (BRC memory artifact) and :165 (BRC Event-Pump Wrapper + agent-wait-patterns §10) previously used 2-dot relative paths (../../docs/...). The sibling rule files in the same directory (checkpoint.md, contract.md, orchestrator.md) use the $EGG_REPO_PATH/docs/... convention, and the same mission.md file's other doc references already follow that pattern. The 2-dot paths resolve to sandbox/docs/... from the source tree and /docs/... from the runtime mount at /opt/claude-rules/, neither of which exists. Switch to the dominant convention so the links are followable from any context. 2. **"exits 0 once role_complete flips" replaced with the actual flag name.** The role_complete flag does not exist anywhere in the code. The brc next-action route returns {"action": "complete"} (orchestrator/routes/consensus.py _VALID_ACTIONS + role-complete short-circuit) and the wrapper checks is_complete from the consensus status payload. Reword the third event-handler step to "exits 0 once `brc next-action` returns the `complete` action (i.e. the role is marked complete in `consensus status`)" so the doc no longer references a non-existent flag. 3. **peer_consensus.py line-number caveat propagated.** The slice-3 prompt-composer tables in both orchestrator.md (§"Per-event prompt composer") and agent-wait-patterns.md §10.9.1 cited peer_consensus.py:949-1024 for _open_nacks_barrier_response. The actual function spans 949–1046 in this branch — the same doc-drift risk the v2 commit already softened for the 24.5k-line pipelines.py. Extend the same caveat to the peer_consensus.py citation so future drift on either file is contained by the function-name reference rather than caught only by the line number. The reviewer also surfaced a coordination note: task-3-4's acceptance criterion is "rebuild produces a new image tag", which the documenter cannot exercise from the BRC pod (no sudo / k3s access). That half is structurally deferred to slice-4's pre-flag-flip kubectl-exec assertion (task-4-1 per the contract). The docs already record the rebuild trigger (make build / make k3s-import / make deploy) as required by the acceptance criterion. No content change needed — the slice-3 PR body should surface the deferral explicitly so slice-4 doesn't have to rediscover it. Refs #2908 tasks 3-4, 3-5 (re-propose v3 after reviewer_code v2 ACK with non-blocking observations) * feat(#2908 slice-3 task-3-1): compose_event_prompt per-event prompt composer Adds ``orchestrator/routes/event_prompt.py::compose_event_prompt`` — the slice-3 replacement for the slice-2 wrapper's minimal stub prompt at ``orchestrator/consensus_wrapper.py::invoke_agent_for_event``. Per the slice-3 plan TASK-3-1: - Positional signature ``(role, event_payload, memory_excerpt, nacks, git_log_delta, base_branch) -> str`` matches the contract verbatim so the wrapper bash's ``python3 -c`` call site is stable across refactors. - Memory excerpt rendered at the user-prompt TAIL position (architect od-6 Option B); the illustrative ``--append-context`` flag from the analysis pseudocode does not exist on ``build_agent_command`` (verified at ``shared/egg_agent/command.py:11-46``). - Per-producer ``git log {sha}..HEAD --not origin/{base_branch} -p`` delta rendered verbatim alongside the executed command (NOT a ``changed_artifacts``-only shortcut — per ``docs/architecture/REVIEWER-SYNC.md`` the re-review must audit the full delta as a fresh review). - Open-NACK payload rendered per-reviewer with reason and artifact_refs (#2142 aggregated-NACK barrier shape from ``peer_consensus.py:_open_nacks_barrier_response``). - Envelope bound at ``PROMPT_ENVELOPE_MAX_BYTES = 10 KB`` (excluding the delta, which scales with the change); memory excerpt capped at ``MEMORY_EXCERPT_MAX_CHARS = 2 KB``. Placed in a sibling module rather than at the bottom of ``orchestrator/routes/pipelines.py`` (the plan explicitly allows the sibling-module form when the host file is over the decomposition cap, which ``pipelines.py`` at ~24 800 lines already is). ``pipelines.py`` adds a one-line re-export so callers importing via ``orchestrator.routes.pipelines.compose_event_prompt`` continue to work — the contract assigns task-3-1 to ``pipelines.py`` so the re-export keeps the public surface aligned with that. Wiring (TASK-3-2), the BRC preamble collapse (TASK-3-3), and tests land in follow-up commits. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(#2908 slice-3 task-3-2): wire event-pump wrapper to compose_event_prompt Slice-2's ``invoke_agent_for_event`` shipped a minimal stub prompt ("BRC event-pump handler / Role / Slice / Action / Event payload"); this commit wires it to the slice-3 ``compose_event_prompt`` via a CLI entry-point on ``orchestrator/routes/event_prompt.py``. The CLI is the wrapper-bash injection seam — calling ``compose_event_prompt`` directly from a Python heredoc would force the ``orchestrator.routes`` package ``__init__.py`` (which imports Flask) to load on the agent pod, and Flask is not in the sandbox runtime. The CLI bypasses that by being the script's ``if __name__ == "__main__":`` entry-point: the wrapper bash invokes ``python3 /opt/egg-runtime/orchestrator/routes/event_prompt.py <action>`` with the event_payload JSON on stdin, and the script imports its own neighbouring helpers without traversing the package init. CLI responsibilities (matches plan TASK-3-2): * Read ``EGG_AGENT_ROLE`` / ``EGG_BASE_BRANCH`` / ``EGG_REPO_PATH`` / ``EGG_BRC_MEMORY`` from env (set on every agent pod per ``orchestrator/kubernetes_spawner.py:818-823``). * Read the per-role memory file at ``.egg-state/agent-outputs/<role>/brc-memory.md`` iff ``EGG_BRC_MEMORY=full`` (slice-1 writer's read gate; slice-4 flips the default to ``full``). * Parse per-producer ``last_reviewed_commit_sha`` from the memory file's structured ``### <role>`` blocks (slice-1 writer schema) even in ``write-only`` mode — the architect plan splits the memory-excerpt gate (full only) from the SHA-lookup gate (always) so the wrapper still renders the per-producer delta against the fallback baseline. * For each ``last_reviewed_commit_sha``, run ``git log {sha}..HEAD --not origin/{base_branch} -p`` via subprocess inside the worktree. The gateway allows ``--not`` and ``-p`` on ``git log`` per #2905. * Extract the open-NACK list (``event_payload['nacks']`` / ``aggregated_nacks``) and forward it through. * Call ``compose_event_prompt`` and write the rendered prompt to stdout. The wrapper bash captures stdout into ``$prompt`` and passes it as the positional argument to ``python3 -m egg_agent``. On any failure (script missing, schema drift, git log subprocess crash) the wrapper falls back to the slice-2 stub so the event-pump keeps running rather than failing the agent invocation — the idle-budget safety net catches a wedged event-pump even under a degraded composer. The composer call obeys the env-flag split: * ``EGG_BRC_MEMORY=full`` — memory excerpt included; per-producer delta rendered. * ``EGG_BRC_MEMORY=write-only`` (slice-1 default) — memory excerpt omitted from prompt; per-producer delta still rendered against the memory file's stored SHAs as a fallback baseline. Matches the plan TASK-3-2 wording verbatim. * ``EGG_BRC_MEMORY=off`` — both omitted. Defensive safety rails added: * ``_GIT_LOG_TIMEOUT_SECS = 60`` so a hung gateway doesn't deadlock the event-pump. * ``_GIT_LOG_DELTA_MAX_BYTES = 256 KiB`` per producer with explicit truncation sentinel so a pathologically large refactor doesn't blow past Claude's context budget silently. * Stdin-based event_payload (#2741 prose-arg discipline) instead of argv. The TASK-3-3 preamble collapse and slice-3 tests land in follow-up commits. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(#2908 slice-3 task-3-3): collapse _build_brc_preamble for event-pump model Strips wait-loop / STAY-ALIVE / cursor-threading guidance from the BRC consensus preamble emitted to every concurrent-mode agent prompt. The slice-2 event-pump wrapper owns lifecycle now; the agent's contract is one-shot per actionable event, not "stay alive in a wait-loop until SIGTERM". What changed in _build_brc_preamble: * Producer Lifecycle step 4 (RESPOND TO REVIEWS) — removed the pre-confirm wait-loop invocation (``_brc_preconfirm_wait_line``) and the "Do not include CONSENSUS_CONFIRMED in this pre-confirm wait" foot-gun guidance + the directed STATUS-nudge prose (#2531). Kept the #2142 aggregation rule (still relevant) and the NACK pushback paragraph in condensed form. * Producer step 6 (STAY ALIVE) — deleted entirely. * Producer step 7 (HANDLE RE-REVIEW) — re-numbered to step 6 and rewritten in event-pump terms ("when you are re-invoked with a CONSENSUS_RE_REVIEW event"). Step 8 (RESOLVE OBLIGATIONS) becomes step 7. * Reviewer step 2 (POLL) — replaced with INVOKED PER EVENT framing. The wrapper invokes the reviewer when the producer's CONSENSUS_PROPOSE lands; the reviewer no longer self-drives a wait-loop. * Reviewer step 7 (STAY ALIVE) — deleted. * Reviewer step 8 — re-numbered to step 7, simplified the recovery-trigger language. The adversarial-re-review dual-mandate banner (the "TWO equal-weight mandates" + "Both must pass to ACK" paragraph) is preserved per plan acceptance. * Dual-Role Execution Order banner — kept structurally (per plan acceptance) but updated to describe the event-pump invocation pattern: the coder's PROPOSE re-invokes the tester rather than triggering an in-process wait-loop return. * Pre-seeded empty-producer shortcut (#2581) — references to step 6 STAY ALIVE replaced with "exit; the wrapper re-invokes you with the next event". The detailed STATUS-nudge / wait-loop filter set guidance collapses to "the wrapper will re-invoke you on the next event". * Trailing "If you exit before the orchestrator stops you, you have FAILED your role" warning — replaced with the event-handler contract: "the wrapper drives lifecycle, exit naturally after acting". * Reviewer ACK/NACK section — condensed the #1998 conditional-ACK example (kept the rule + flag, dropped the verbatim command example), folded the #2336 alternative-obligation block into one sentence at the tail of the #2338 drop-obligation block, and trimmed the stale-version paragraph. * Helpers ``_brc_preconfirm_wait_line`` and ``_brc_stay_alive_wait_line`` — deleted (zero callers post-collapse). Byte-size delta (per the plan acceptance ≥ 25% target softened from the original ≥ 40%): * coder 9664 -> 7238 bytes (25.1% drop) * reviewer_code 12606 -> 9346 bytes (25.9% drop) * tester 24139 -> 17635 bytes (26.9% drop) The "Both must pass to ACK" phrase from the dual-mandate banner is preserved (verified across all three role variants); the byte-size drop is across all three. Coverage caveat per the plan: slice-3 ships the collapsed preamble against the LEGACY wrapper path by default (the EGG_BRC_EVENT_PUMP flag stays off until slice-4). The intermediate state (slice-3 merged but slice-4 not yet) is intentionally inert because the legacy wrapper's capped-restart safety net catches the resulting "agent exits after one pass" behavior — slice-4's flag flip plus deletion of the legacy template close that window. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(#2908 slice-3): add compose_event_prompt + preamble-collapse tests, update legacy preamble tests The coder-owns-tests policy (#2936) means the slice-3 coder authors the initial test scaffold for TASK-3-6 (compose_event_prompt) and TASK-3-7 (collapsed preamble snapshot); the tester reviews-and-hardens in their own pass. New tests: * ``orchestrator/tests/test_compose_event_prompt.py`` (TASK-3-6) — prompt-shape tests per role variant (producer / reviewer / dual-role), memory-excerpt truncation at the 2 KB cap, open-NACK rendering across 0 / 1 / 2+ reviewers (the #2142 aggregated barrier case), verbatim ``git log {sha}..HEAD --not origin/{base_branch} -p`` command emission (with a regression guard against future ``changed_artifacts``-only shortcut per REVIEWER-SYNC.md + risk_analyst R6), envelope-budget assertion (≤ 10 KB excluding the rendered delta, which scales with the change), and defensive shape (None inputs, empty role, empty base branch). * ``orchestrator/tests/test_brc_preamble_collapsed.py`` (TASK-3-7) — three role-shape snapshots (coder / reviewer_code / tester); absent-strings (STAY-ALIVE, positive wait-loop instructions, cursor-threading, ready_to_confirm STATUS-nudge); kept-strings ("Both must pass to ACK", dual-mandate banner, agent roster, producer/reviewer lifecycle skeleton); event-handler contract framing replaces the legacy "you have FAILED your role" warning; byte-size drop ≥ 25% per role variant. Legacy preamble tests updated to match the slice-3 collapsed shape in ``orchestrator/tests/test_pipeline_prompts.py`` and ``orchestrator/tests/test_concurrent_integration.py``: * Updated (retargeted assertions, preserved purpose): - ``TestBrcPreambleSyncStep::test_reviewer_sync_step_after_poll`` (POLL → INVOKED PER EVENT) - ``TestBrcPreambleSyncStep::test_reviewer_lifecycle_renumbered`` (steps 1-7, STAY ALIVE deleted) - ``TestAdversarialReReviewPriming::test_reviewer_lifecycle_step8_carries_adversarial_framing`` (banner moved from step 8 to step 7; substantive content preserved) - ``TestAdversarialReReviewPriming::test_producer_respond_to_reviews_legitimizes_new_findings`` (NACK pushback paragraph condensed but preserved) - ``TestDirectedCoordinationGuidance::test_directed_coordination_before_exit_warning`` (precedes the Event-handler contract instead of the deleted "you have FAILED" warning) - Three ``TestDualRoleExecutionOrdering`` tests (banner-presence + REVIEW token; wait-loop allowlist references dropped) - ``TestConcurrentPromptLifecycle::test_concurrent_prompt_includes_lifecycle_preamble`` (asserts the slice-3 Event-handler-contract framing instead of the legacy STAY-ALIVE / "FAILED your role" framing) * Deleted (entirely about deleted functionality): - ``TestReviewerPollUsesWaitLoop`` (POLL is gone) - ``TestReviewerWaitLoopMentionsAutoCursor`` (cursor-threading is gone) - ``TestProducerRespondToReviewsWaitLoop::test_step4_lists_pre_confirm_allowlist`` and ``test_step4_explains_status_ready_to_confirm_nudge`` (the wait-loop allowlist is gone; the orchestrator's ``brc next-action`` route drives the producer pre-confirm waits now). The ``test_step4_excludes_consensus_confirmed`` regression guard is preserved. - Seven ``TestDualRoleExecutionOrdering`` tests that pinned the wait-loop allowlist content of the banner. - ``TestConcurrentPromptLifecycle::test_reviewer_stay_alive_uses_canonical_for_list`` (STAY ALIVE is gone). The deletions preserve issue cross-references in adjacent comments so the audit trail (#1943, #2064, #2323, #2482, #2531, #2749) is not lost. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * style(#2908 slice-3): ruff format on slice-3 source files Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(#2908 slice-3): rebase reconciliation for older slice-3 base The slice-3 task-3-3 patch was authored against a base that included the #2936 coder-owns-tests rewrite; rebasing onto origin/slice-3 (which precedes #2936) required two small reconciliations: * Dual-role banner preface — restored ``propose right after`` and ``self-block`` phrasings (required by ``test_dual_role_banner_states_propose_first``) while keeping the event-pump framing from the slice-3 collapse. * ``test_no_positive_wait_loop_instructions`` — broadened the negative-qualifier allowlist to accept ``Do NOT call`` / ``do NOT call`` so the producer-orientation copy (``Do NOT call `wait-loop`...``) and the new banner preface (``Do NOT block on a reviewer wait...``) both satisfy the regression guard. The intent is unchanged: any line mentioning ``wait-loop`` must qualify it with a negative directive. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(#2908 slice-3): address reviewer_contract v1 NACKs Three blocking findings addressed; one additional non-blocker drop: 1. **Env-var prefix bug in wrapper bash** (NACK #1). The form ``EGG_AGENT_ROLE=... printf '%s' "$event_payload" | python3 ...`` attached env-vars only to ``printf``, not to ``python3``. The agent pod's parent shell currently exports the needed vars so this works in production, but the comment claimed the re-export protected against a parent shell that hadn't propagated them — which the construct didn't actually do. Moved the env prefix to the RHS of the pipe so ``python3`` actually gets the named vars. Also captured stderr to a temp file and surfaced the first line in the fallback ``cw_log`` so the operator can tell script-not-found / schema-drift / subprocess crash apart (non-blocking observation from reviewer_concurrency). 2. **CLI tests for the memory-mode handling** (NACK #2 — plan TASK-3-2 acceptance "snapshot test verifies both branches"). Added three subprocess-level tests in ``test_compose_event_prompt.py`` driving the ``_cli`` entry point against a real tmp-repo + populated memory file: * ``test_cli_full_mode_emits_memory_and_delta`` * ``test_cli_write_only_mode_omits_memory_keeps_delta`` * ``test_cli_off_mode_omits_memory_and_uses_changed_artifacts_fallback`` Each one verifies the ``EGG_BRC_MEMORY``-mode-gated behaviour of the slice-1 default (``write-only``) and the slice-4 target (``full``). 3. **``changed_artifacts`` fallback implemented in ``_build_delta_entries``** (NACK #3 — plan TASK-3-2 acceptance + documenter's docs at ``docs/architecture/orchestrator.md`` / ``docs/reference/agent-wait-patterns.md`` describe the same degraded-baseline fallback). When no per-producer SHA is recorded yet (off mode, first-ever ACK before any memory write, parse failure, file missing) and the event payload carries a ``changed_artifacts`` list, render a single fallback entry naming the producer and the artifact list — explicitly labelled as a degraded baseline so the agent does NOT mistake it for an adversarial-re-review-grade diff. Adds ``_extract_changed_artifacts`` and ``_extract_producer_role`` helpers and threads ``event_payload`` through the ``_cli`` call. Four new unit tests cover (a) the fallback fires when no SHA + non-empty ``changed_artifacts``, (b) the producer role is pulled from ``event_payload.producer`` / ``event_payload.producer_role``, (c) no fallback when neither SHA nor ``changed_artifacts``, (d) a real SHA always wins over the fallback. 4. **Drop dead ``type`` fallback in ``_render_event_section``** (non-blocking observation). The ``next-action`` route emits ``action`` only (``consensus.py::_VALID_ACTIONS``); the ``type`` fallback was hedging against a schema that doesn't exist in this codebase. All 629 tests in the directly-affected suites pass; ruff lint + format clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(#2908 slice-3): address tester + reviewer_contract v1 NACKs Two follow-on findings from the aggregated v1 NACK barrier: * **tester NACK** — ``ruff format --check`` failed on ``orchestrator/routes/pipelines.py`` (quote-style on the ``#2338`` drop-obligation paragraph). Ran ``ruff format`` on the file; ``make lint`` now passes literally. * **reviewer_contract NACK #2** — ``test_consensus_wrapper.py`` had no test that pinned the wrapper template's ``invoke_agent_for_event`` invocation shape. The ``TestEventPumpInvokesComposer`` class adds six snapshot tests that fail if a future refactor: * drops the ``invoke_agent_for_event`` function definition; * changes the script path reference or removes the ``EGG_EVENT_PROMPT_SCRIPT`` env-var indirection; * breaks the ``EGG_AGENT_ROLE`` / ``EGG_BASE_BRANCH`` / ``EGG_BRC_MEMORY`` env-var re-export contract to the CLI; * reverts the v1-NACK fix (env-var prefix landing on ``printf`` instead of ``python3``); the ordering test asserts the textual order ``printf '%s' ... | EGG_AGENT_ROLE=... python3`` so a re-introduction of the bug trips the test; * changes the ``python3 "$script_path" "$action"`` call shape; * accidentally references ``event_prompt.py`` from the legacy flag-off template (the composer is event-pump-only). All 635 tests in the directly-affected suites pass; ruff lint + format clean across all slice-3 files. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(#2908 slice-3): documenter consensus participation marker Empty commit recording documenter participation in slice-3 BRC consensus restart (#2908). The slice-3 documenter work for task-3-4 (mission.md event-handler rewrite) and task-3-5 (orchestrator.md + agent-wait-patterns.md slice-3 doc surface) is already on the slice-3 integration branch from prior cycles (commits 6137694, e3ab9e9, 90b2cb5, plus reconciliation commits dfad765, 63c6bfe, fc2e82e, 66b7e73); this commit anchors a fresh BRC propose by the slice-3 documenter at restart. Verified on the slice-3 branch HEAD prior to this commit: - diff sandbox/agent-config/rules/mission.md sandbox/claude-rules/mission.md returns empty (byte-identical via symlink). - rg 'STAY-ALIVE\b|wait-loop|never exit' inside the rewritten Concurrent Execution Mode section of mission.md returns zero matches. - The "BRC Per-Event Prompt Composer + Preamble Collapse (slice-3, #2908)" section in docs/architecture/orchestrator.md is present and reflects the reviewer_code v1/v2 corrections (heading does not say "behind EGG_BRC_MEMORY"; preamble collapse documented as unconditional; byte-size drop softened to ≥ 25%; line-number references carry the "drift-prone — prefer function-name" caveat). - The "Per-Event Prompt Shape and Memory Consumption (slice-3, #2908)" section in docs/reference/agent-wait-patterns.md is present at §10.9 as a subsection of slice-2's §10 (BRC Event-Pump Wrapper), wiring the $EGG_REPO_PATH/docs/... convention. - EGG_BRC_MEMORY env var entry is in the orchestrator.md Environment Variables table. - Cross-refs to brc-memory.md (slice-1) and the BRC Event-Pump Wrapper section (slice-2) all resolve on this branch (slice-1/slice-2 docs are in the slice-3 branch's history). Refs #2908 tasks 3-4, 3-5 * feat(#2908 slice-3): coder consensus participation marker Empty commit recording coder participation in slice-3 BRC consensus restart (#2908). The slice-3 coder work for tasks 3-1 (compose_event_prompt), 3-2 (wire event-pump wrapper to composer), and 3-3 (collapse _build_brc_preamble) is already on the slice-3 integration branch from prior cycles (commits ed8a4c5, 27ed9d0, 2261d5c, plus the test-author commit 7cff8d1, lint-fix 63c6bfe, reconciliation dfad765, and the NACK-address commits fc2e82e, 66b7e73); this commit anchors a fresh BRC propose by the slice-3 coder at restart. Verified on slice-3 branch HEAD prior to this commit: - orchestrator/tests/test_compose_event_prompt.py: 23/23 pass — covers prompt shape per role (producer / reviewer / dual-role), memory-excerpt truncation at the 2 KB cap, NACK rendering for 0/1/2+ reviewers, full 'git log SHA..HEAD --not origin/BASE -p' delta command emitted verbatim with per-producer last_reviewed_commit_sha substituted (no changed_artifacts-only shortcut), envelope bounded ≤ 10 KB. - orchestrator/tests/test_brc_preamble_collapsed.py: 26/26 pass — covers STAY-ALIVE / wait-loop / cursor strings absent (per task-3-3), dual-mandate 'Both must pass to ACK' phrase preserved, agent roster preserved, producer/reviewer lifecycle skeletons preserved, preamble byte size drops ≥ 25% vs the pre-collapse snapshot baseline. - orchestrator/tests/test_pipeline_prompts.py: 431/431 pass — full pipeline-prompts regression suite (event-handler contract present, dual-role banner integration, etc.). - orchestrator/tests/test_consensus_wrapper.py: passes — including the TestEventPumpInvokesComposer suite added in 66b7e73 that pins the wrapper template's invoke_agent_for_event invocation shape. - ruff check + ruff format on the four touched source files passes literally (orchestrator/routes/event_prompt.py, orchestrator/consensus_wrapper.py, orchestrator/routes/pipelines.py, the two new test files). * fix(#2908 slice-3): address reviewer_code v2 + reviewer_code_holistic v2 NACKs Addresses four blocking findings from the slice-3 v2 NACK barrier (reviewer_code v2 finding #1 + reviewer_code_holistic v2 findings #1/#2/#3). All four are cross-module asymmetries between the slice-3 composer (orchestrator/routes/event_prompt.py) and the next-action route (orchestrator/routes/consensus.py). ### 1. reviewer_code v2 finding #1 — REVIEWER-SYNC.md path The composer's module docstring (event_prompt.py:16) and the "Per-producer re-review delta" prompt section (event_prompt.py:153) both cited 'docs/architecture/REVIEWER-SYNC.md' — a file that does not exist at that path. The real location is 'shared/prompts/REVIEWER-SYNC.md' (verified with git show origin/egg/issue-2908-impl2/slice-3:shared/prompts/REVIEWER-SYNC.md; every other reference in the codebase uses the correct shared/ path). Reviewers following the cited link would 404; this is the exact 'documented snippet that doesn't work as a copy-paster reads it' regression the review criteria flag. Fix: replace both occurrences with 'shared/prompts/REVIEWER-SYNC.md'. Regression guards: test_per_producer_delta_section_cites_correct_reviewer_sync_path and test_module_docstring_cites_correct_reviewer_sync_path now assert the rendered prompt contains 'shared/prompts/REVIEWER-SYNC.md' AND does not contain 'docs/architecture/REVIEWER-SYNC.md' (so a future move/rename can't silently re-break this). ### 2. reviewer_code_holistic v2 finding #2 — unresolved_nacks _extract_nacks (event_prompt.py:602-630) accepted only 'nacks' and 'aggregated_nacks' keys, but next-action's _derive_next_action (consensus.py lines 329/346) emits 'unresolved_nacks' for the single-reviewer NACK propose path — the COMMON case. The open-NACK barrier shape ('nacks' key) requires 2+ distinct reviewers; a single-reviewer NACK uses 'unresolved_nacks' and was silently dropped from the per-event prompt. Fix: add 'unresolved_nacks' as a third accepted key (priority: nacks → unresolved_nacks → aggregated_nacks). Producer re-invoked to address a single-reviewer NACK now sees the structured Open-NACKs section with reviewer 'reason' + 'artifact_refs' inline (the round-trip-per-NACK signal #2142 was built to enforce). Regression guards: - test_extract_nacks_accepts_unresolved_nacks_key_from_next_action pins the new key against the real _derive_next_action payload shape. - test_compose_event_prompt_renders_unresolved_nacks_section asserts end-to-end that the structured Open NACKs section renders with the reviewer's identity / reason / artifact_refs. - test_extract_nacks_priority_order_nacks_over_unresolved_nacks pins barrier-shape priority over the convenience key. ### 3. reviewer_code_holistic v2 finding #3 — scoping to current producer _build_delta_entries iterated EVERY producer in the reviewer's memory file (for producer in sorted(per_producer.keys())) and emitted a delta for each, irrespective of which producer the CURRENT event named. The user-visible failure: reviewer-A ACKed coder at v1 → memory stores coder's SHA. Tester then proposes for the first time. Reviewer-A is re-invoked with event_payload = {pending_reviews: [{producer: tester, ...}]}. The renderer emitted ONLY coder's stale delta — tester's first review had no delta at all, and the section title 'Per-producer re-review delta' implied the rendered delta WAS the producer being reviewed (but it was the wrong one). Fix: scope delta enumeration to producers named in event_payload.pending_reviews (or top-level producer/producer_role on the producer side). Treat memory's per-producer SHA as a per-producer LOOKUP keyed by the current producer, not as an ENUMERATION source. Legacy / synthetic-test paths with no pending_reviews key fall back to enumerating all stored SHAs (backward compat). New helpers: - _extract_current_producers(event_payload) walks pending_reviews / top-level producer keys, de-dupes in first-seen order. - _extract_artifacts_for_producer(event_payload, producer) pulls artifact_refs from the matching pending_reviews entry first (production path), falls back to top-level changed_artifacts only when the top-level producer matches (prevents cross-producer artifact leak). Regression guards: - test_build_delta_entries_scopes_to_pending_reviews_producer pins the scoping invariant (memory has X SHA, event names Y → render Y, not X). - test_build_delta_entries_pending_reviews_with_sha_renders_real_delta asserts _run_git_log invoked only for the current producer's SHA (not the stale one). - test_build_delta_entries_multiple_pending_reviews_renders_each covers the multi-pending-review case. - test_build_delta_entries_no_pending_reviews_falls_back_to_memory_enum pins the backward-compat path. - test_extract_current_producers_* / _extract_artifacts_for_producer_* pin the new helper behaviour. ### 4. reviewer_code_holistic v2 finding #1 — changed_artifacts fallback wired through The documented changed_artifacts fallback (docs/architecture/orchestrator.md + docs/reference/agent-wait-patterns.md) was dead code in production: next-action's _derive_next_action never emits a top-level changed_artifacts key, but the fallback path in _build_delta_entries looked for one. First-time reviewers of any producer (no stored SHA, no top-level changed_artifacts in the real payload) silently saw an empty 'Per-producer re-review delta' section. Fix: enrich next-action's reviewer-side pending_reviews entries with artifact_refs sourced from PeerConsensusTracker.get_current_proposal_snapshot(producer). The lock is reentrant (threading.RLock at peer_consensus.py:101) so the call inside _derive_next_action's locked block is safe. The composer's _extract_artifacts_for_producer prefers pending_reviews[i].artifact_refs over the legacy top-level changed_artifacts key, restoring the documented fallback. Regression guard: - test_next_action_reviewer_pending_reviews_includes_artifact_refs asserts a pending_reviews entry from the production next-action route carries artifact_refs mirroring the producer's current proposal artifacts (the seeded a.py from _propose). ### Tests 660 tests pass under the slice-3 regression scope (compose + preamble + pipeline_prompts + consensus_wrapper + consensus_next_action + concurrent_integration). Ruff lint + format clean on the four touched files. * test(#2908 slice-3): tester hardening on compose_event_prompt + preamble collapse Adversarial coverage added on top of the coder-authored test scaffolds for TASK-3-6 (compose_event_prompt) and TASK-3-7 (collapsed preamble). Per #2936 the coder authors its tests; the tester reviews-and-hardens in their own pass. Probes the boundaries the coder-authored happy-path tests do not: * ``_truncate`` exact-boundary + just-over-boundary semantics (the off-by-one at the cap was not pinned). * Multi-byte UTF-8 memory excerpt — truncation respects the code-point cap not the byte cap; envelope still respected. * Whitespace-only memory excerpt → section omitted (strip() guard). * Producer-delta iteration order preserves caller order (no implicit sort — sorting belongs to ``_build_delta_entries``'s memory-enum fallback, not the renderer). * ``(no commits in range — re-review is a no-op)`` sentinel for an empty delta string (the post-confirm re-confirm shape). * Producer-delta entries with missing keys render defensive defaults (``(unknown)`` producer label, ``<no prior review>`` SHA sentinel). * Non-string delta is coerced via ``str()`` rather than crashing. * NACK with missing ``reviewer`` / ``reason`` / non-list ``artifact_refs`` renders sentinels rather than crashing. * ``_parse_per_producer_sha`` edge cases — ``-`` sentinel skip, first-match-wins per heading, orphan bullet ignored, backtick-wrapped heading canonicalised. * ``_extract_nacks`` third-priority ``aggregated_nacks`` fallback (the coder pins ``nacks`` > ``unresolved_nacks``; this pins the third tier). * ``_extract_nacks`` drops non-dict entries silently. * ``_extract_changed_artifacts`` filters ``None`` / whitespace; defensive against non-dict payloads. * ``_extract_current_producers`` robust against mixed-shape ``pending_reviews`` entries; defensive against non-dict payloads. * ``_extract_artifacts_for_producer`` priority: ``pending_reviews`` > top-level ``changed_artifacts``; empty / whitespace producer returns empty list. * ``_run_git_log`` subprocess error paths — timeout sentinel, non-zero rc sentinel with stderr, 256 KiB truncation marker. * CLI: ``--event-payload-file`` reads from disk; missing file returns rc=2 with explicit error; malformed-JSON stdin falls back to surfacing the raw payload under ``raw``; empty stdin falls back to ``{"action": <argv>}`` action-only payload. Hardens the preamble-collapse contract beyond the line-by-line ``wait-loop`` qualifier check: * Positive ``egg-orch message wait-loop`` invocations forbidden (each occurrence must sit inside a ±200-char window with a ``do NOT`` / ``Do NOT`` / ``not block on`` / ``not call`` / ``not issue`` negation anchor — catches multi-line code blocks). * ``--for CONSENSUS_*`` filter-flag patterns absent (legacy wait-loop plumbing). * ``never exit`` warning gone (TASK-3-3 explicit collapse target; pairs with the coder's ``"you have FAILED your role"`` check). * ``/tmp/egg-wait-cursor-`` plumbing path absent (full-text scan, complements the substring check). * Agent roster names the producer + reviewer roles, not just the heading. * No ``Ready to confirm — all confirm preconditions satisfied`` STATUS-nudge anchors (#2531 plumbing collapsed). * Tester preamble fits inside a 20 KB ceiling (absolute bound complements the ≥ 25% relative-drop assertion against the hardcoded baseline — a runaway re-expansion fails on both anchors). * Unknown role doesn't crash; the agent-roster section still renders. * Event-handler contract surfaces the wrapper-owned-wait framing (wrapper / drives your lifecycle / one-shot) — pins the central intent of the collapse beyond merely the contract heading. All 115 tests pass under PYTHONPATH=. pytest; ruff lint + format clean on both files. Configured ``make test`` / ``make lint`` / ``make security`` cannot run end-to-end in this pod because the venv-sync step fails to fetch dev wheels (no PyPI egress) — see the propose attestation for the ``tests_execution_blocked`` rationale and the slice-3-scoped check counts that DID execute directly. * Persist BRC history for slice-3 (#2548) * Fix checks: apply automated formatting fixes * Address reviewer_holistic v2 feedback - Enforce PROMPT_ENVELOPE_MAX_BYTES in compose_event_prompt by byte-truncating the NACKs section (the variable-size driver) with an explicit sentinel when the envelope would otherwise overflow (reviewer_holistic minor #1). Memory tail-position contract and delta-exclusion are preserved. - Re-export EGG_REPO_PATH on the python3 invocation in the event-pump wrapper so the env-prefix matches the in-source comment listing all four wrapper-supplied vars (reviewer_holistic minor #2). Test updated to assert all four env-var re-exports. - Update stale pipelines.py line anchors in docs/architecture/ orchestrator.md and docs/reference/agent-wait-patterns.md to the actual post-collapse positions (12180 for _build_brc_preamble, 13366/13399/13427 for callers, 12561-12573 for the dual-mandate banner, 12567 for the 'Both must pass to ACK' anchor); add a reminder that the function/banner-name anchor is the drift-resistant reference (reviewer_holistic minor #4). - Add an operator-telemetry note to agent-wait-patterns.md §10.9.5 calling out the slice-3 default-off restart-budget consumption (one restart per agent per phase) so operator SLOs aren't surprised by the slice-3-vs-slice-4 metric step (reviewer_holistic minor #3). * Strip NACK payload from event_section JSON to honour envelope cap Reviewer re-review of commit aaaa17f found that Minor #1 (envelope-cap enforcement) was only partially fixed: the NACK list rendered into nacks_section was truncated under the cap, but the same payload was ALSO baked into event_section via json.dumps(event_payload, ...). Production payloads from _producer_has_open_barrier (consensus.py:248) and _producer_has_unresolved_nacks_on_current_version (:340-358) put the full nacks/unresolved_nacks list directly inside event_payload, so the reviewer's worked example (6 reviewers x ~2.8 KB reason) still sailed past PROMPT_ENVELOPE_MAX_BYTES because the JSON copy was untouched. Fix: strip the NACK keys (nacks / unresolved_nacks / aggregated_nacks) from event_payload before json.dumps in _render_event_section, replacing each with a cross-reference marker pointing the agent at the dedicated nacks_section. The single source of truth for the rendered NACK bytes is now nacks_section, which the envelope-cap truncation pass already governs. Existing tests masked the bug because they passed event_payload={"action": "propose"} (no nacks in payload) with the nacks list separately, which never happens in production. Two new regression tests pin the production-shaped payload: - test_production_shaped_open_barrier_payload_honours_envelope_cap (multi-reviewer 'nacks' barrier shape from _producer_has_open_barrier) - test_production_shaped_unresolved_nacks_payload_honours_envelope_cap (single-reviewer 'unresolved_nacks' shape from _derive_next_action) Both assert the envelope stays under the cap AND that the NACK reason text does NOT appear in the JSON block. * Fix dual-role banner contradiction with coder-owns-tests orientation The merge resolution in 39a08bb kept HEAD's banner verbatim while main's #2936 (coder-owns-tests) auto-merged into surrounding sections. The banner directed the tester to "execute Producer steps 1-3 FIRST" and "draft scaffolding" while the surrounding producer orientation tells the tester "Orient only until the coder proposes" and "do NOT write test files before the coder's CONSENSUS_PROPOSE". Two opposite directives in the same prompt. Fix (banner-only edit; surrounding sections already correct): - Remove "draft scaffolding" from the opportunistic-prepare list (it's the scaffold-first directive in disguise). - Reword "Producer steps 1-3 come FIRST" to "Producer ORIENT (step 1) comes FIRST" + an explicit pivot on the role-specific orientation for whether WORK (step 2) runs immediately or is gated on the upstream CONSENSUS_PROPOSE. - Name the tester case explicitly (the dual-role agent whose WORK is gated per #2936) so the agent reading the banner can reconcile it with its role-specific orientation. - Keep "the wrapper re-invokes you when that proposal arrives" and "propose right after" — slice-3's event-pump framing remains correct. Add ``test_dual_role_banner_does_not_contradict_coder_owns_tests`` that scopes assertions to the banner SUBSTRING (between ``### Dual-Role Execution Order`` and ``### Producer Lifecycle``) so a future banner-only edit can't sneak the scaffold-first prose back in. Pairs with the existing ``test_tester_orientation_directs_review_and_harden_after_propose`` which covers ``_build_producer_orientation`` — together they pin both halves of the prompt the merge resolution desynced. --------- Co-authored-by: egg <egg@example.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: egg-orchestrator <egg@localhost> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com> Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com>

#2951) * feat(#2908 slice-4 task-4-1): flip EGG_BRC_EVENT_PUMP and EGG_BRC_MEMORY defaults Slice-4 task-4-1 makes the event-pump wrapper the production default by flipping two env-flag defaults: * ``EGG_BRC_EVENT_PUMP`` flips from unset→OFF (legacy) to unset→ON (event-pump). Setting ``EGG_BRC_EVENT_PUMP=false`` (or ``0`` / ``no`` / ``off``, case-insensitive) keeps the legacy capped-restart template available for a one-release rollback window. Unrecognised tokens fall through to event-pump so a typo cannot silently downgrade the production path. Slice-4 task-4-2 will delete the legacy template entirely and the env flag with it. * ``EGG_BRC_MEMORY`` flips from unset→``off`` (slice-1 inert) to unset→``full`` (event-pump composer reads memory by default). Setting ``EGG_BRC_MEMORY=off`` is the one-release rollback escape hatch. Unknown values still fail-safe to ``off`` (the fallback target stays restrictive — an undocumented value is a misconfiguration signal, NOT a write-bearing default to mask). Files touched: * ``orchestrator/consensus_wrapper.py``: - ``_event_pump_enabled()`` default flipped; falsy-token allowlist captures rollback path; docstring + module-level reframe updated. - Wrapper template's inline ``EGG_BRC_MEMORY:-off`` → ``...:-full`` so the wrapper's invocation of ``event_prompt.py`` inherits the new default even on shells that don't export the var explicitly. * ``sandbox/egg_agent_tools/handlers/brc_memory.py``: - ``get_memory_mode()`` defaults to ``MODE_FULL``; new ``MODE_DEFAULT`` constant pins the contract. * ``orchestrator/routes/event_prompt.py``: - CLI ``memory_mode`` default flipped from ``"off"`` to ``"full"``. * Tests updated to match the new defaults: - ``orchestrator/tests/test_consensus_wrapper.py``: ``TestEventPumpTemplateSelection`` rewritten — unset-env now pins event-pump, ``EGG_BRC_EVENT_PUMP=false`` pins legacy. ``TestBuildConsensusWrappedCommand``, ``TestConsensusWrapperBehavior``, ``TestBufferOverflowDetection``, ``TestEventDrivenWait``, ``TestSSESigtermGrace`` gain an autouse ``_force_legacy_template`` fixture that engages the rollback escape hatch so they continue to drive the legacy template. Slice-4 task-4-2 deletes the entire fixture + these classes alongside the legacy template. - ``tests/sandbox/egg_agent_tools/test_handlers_brc.py``: renamed ``test_unset_defaults_to_off`` → ``test_unset_defaults_to_full``; the unset-env pin now asserts the memory file is written. - ``orchestrator/tests/test_compose_event_prompt.py``: docstring note that ``write-only`` is the rollback target, not the default. Verified manually with Python smoke tests that the flag-flip works for unset, truthy, and the full falsy-token allowlist (``false`` / ``0`` / ``no`` / ``off`` / case variants), and that ``EGG_BRC_MEMORY=writeonly`` (typo) still fails safe to ``off`` with a warning while ``EGG_BRC_MEMORY=full`` and unset both enable writes + reads. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(#2908 slice-4 task-4-4): post-deletion consensus wrapper docs Rewrite docs/architecture/orchestrator.md "BRC Consensus Wrapper" section (renamed from "BRC Event-Pump Wrapper (slice-2, behind EGG_BRC_EVENT_PUMP)") to describe the post-deletion steady state. Event-pump is now the only consensus-wrapper path; the legacy capped-restart template and the agent-side heartbeat / keep-alive path were removed in slice-4 task-4-2. Changes: - docs/architecture/orchestrator.md - Renamed section to "BRC Consensus Wrapper"; updated anchor link from #brc-event-pump-wrapper-slice-2-behind-egg_brc_event_pump. - Replaced the slice-2 caveat blockquote with a four-slice rollout summary that names the deleted symbols (_CONSENSUS_WRAPPER_TEMPLATE, _RECOVERY_SYSTEM_PROMPT, SSE consensus.reached, MAX_CONSENSUS_RESTARTS). - Reframed "Why a new wrapper template" → "Why the wrapper drives the loop"; rewrote in past tense so the doc reads as if the event-pump has always been the only model. - Rewrote "Wrapper-side heartbeat (#2036 migration)" and "Wrapper-side gateway-session keep-alive (#2451 migration)" with "completed in slice-4" qualifier; described agent-side deletion. - Rewrote "Idle / no-progress safety budget" to drop the comparison table with the legacy 3-restart cap; replaced with a single behaviour table for EGG_BRC_IDLE_BUDGET_MIN. - Added new "Rollback plan" subsection documenting git revert of slice-4 → slice-3 → slice-2 → slice-1 in reverse-merge order, the integration check operators must run, and the partial-revert interaction (reverting only slice-4 restores the dual-emission state). - Renamed "Slice-2 verification stance — unit-test-only" to "Verification stance — unit-test-only"; explained that the snapshot tests pinning the byte-for-byte legacy template emission were retired in slice-4 task-4-3. - Renamed "BRC Per-Event Prompt Composer + Preamble Collapse (slice-3)" to drop the slice marker; reframed "Flag mapping" to "What's gated by what" since EGG_BRC_EVENT_PUMP no longer gates anything. - Updated EGG_BRC_MEMORY table: full is now the slice-4 default; write-only is the opt-in regression path. - Updated env vars table: EGG_BRC_EVENT_PUMP is a deprecated no-op pointing at the rollback plan; EGG_BRC_IDLE_BUDGET_MIN is no longer gated on EGG_BRC_EVENT_PUMP=true. - docs/guides/concurrent-execution.md - Replaced the slice-2 "two emission paths" caveat with a single post-deletion summary linking to the new orchestrator.md section. - Rewrote the "Consensus Wrapper" body to describe the deterministic event-pump loop (steps 1–6) as the only path; removed MAX_CONSENSUS_RESTARTS-based restart cap, the recovery system prompt, and the final-consensus-check restart cycle. - Updated the configuration table: dropped `max_restarts` row; added EGG_BRC_IDLE_BUDGET_MIN; updated transient-crash recovery paragraph to reference the idle/no-progress budget instead of the deleted MAX_CONSENSUS_RESTARTS hard cap. - docs/architecture/README.md - Updated the cross-link card to point at the renamed section and summarise the slice-4 deletion + rollback plan. Cross-links to docs/architecture/brc-memory.md (slice-1) retained throughout. The wait-side companion at agent-wait-patterns §10 is referenced from each cross-link card. Satisfies contract task-4-4. Acceptance: doc reads as if the event pump has always been the only model; legacy-path caveats removed; cross-links present; rollback plan documented; markdown renders clean (no conflict markers; section anchors resolve). * feat(#2908 slice-4 task-4-2): delete legacy capped-restart template and agent-side heartbeat Slice-4 task-4-2 collapses ``consensus_wrapper.py`` onto the event-pump template that slice-2 introduced and slice-3 wired the per-event composer into. The event-pump is now the only production path; rollback under a regression is a ``git revert`` of slices 1-3 per the PR body, not an env-flag flip. Deleted from ``orchestrator/consensus_wrapper.py``: * ``_CONSENSUS_WRAPPER_TEMPLATE`` (the ~600-line legacy capped-restart bash template). * ``_RECOVERY_SYSTEM_PROMPT`` and ``_RECOVERY_USER_PROMPT`` — the restart-time recovery prompts. * The SSE ``consensus.reached`` curl path (issue #1897) that lived inside the legacy template — the event-pump uses ``egg-orch message wait-loop`` instead. * ``MAX_CONSENSUS_RESTARTS`` (issue #2806) and its companion constants ``MAX_READY_POLL_CYCLES``, ``TRANSIENT_RESTART_BACKOFF_INITIAL``, ``STARTUP_FAILURE_WINDOW_SECONDS``. The idle/no-progress safety budget (env ``EGG_BRC_IDLE_BUDGET_MIN``, default 30 min) is the replacement liveness ceiling. * ``_event_pump_enabled()`` — the ``EGG_BRC_EVENT_PUMP`` env-flag read. The flag is now silently inert; operators with it lingering in k8s manifests can leave it set to either truthy or falsy and still get the event-pump template. * The legacy-template branch in ``build_consensus_wrapped_command``, which is now a thin alias for ``build_event_pump_wrapped_command``. Preserved by relocating into ``_EVENT_PUMP_WRAPPER_TEMPLATE`` (per task-4-2 acceptance, "Keep ``is_buffer_overflow`` / ``is_transient_crash`` / ``is_startup_failure`` classifiers"): * ``is_buffer_overflow()`` — Claude Agent SDK 1 MiB JSON reader overflow detector (#2804). * ``is_transient_crash()`` — signal-based exits (134, 136, 137, 139, 255). * ``is_startup_failure()`` — exit 1 within a 30 s startup window. * ``STARTUP_FAILURE_WINDOW_SECONDS`` — kept as a bash-scope shell variable inside the template (was a Python module constant). The classifiers are not yet wired into the event-pump's ``propose|ack|nack`` agent-invocation failure path (which uses ``AGENT_FAIL_STREAK`` + idle-budget escalation today); they live as named helpers for future revisions. Deleted from ``sandbox/egg_agent_tools/handlers/message.py``: * ``_WAIT_LOOP_HEARTBEAT_INTERVAL_SECS`` — the 60-s cadence constant. * ``_default_emit_wait_loop_heartbeat`` — the agent-side ``WAITING_FOR_EVENT`` / ``WORKING`` heartbeat emitter (#2036). * ``_start_wait_loop_heartbeat`` — the threaded periodic-tick helper. * The per-iteration ``emit_hb`` / ``stop_hb`` calls inside ``message_wait_loop``, including the ``try/finally`` block that drove the final ``WORKING`` beat on wait exit. The event-pump wrapper now owns both heartbeat liveness (#2036) and slice-scoped gateway-session keep-alive (#2451) via the wrapper- owned ``start_background_heartbeat`` subshell. ``message_heartbeat`` (the explicit handler invoked by ``egg-orch message heartbeat``) is unchanged — the wrapper calls it. Test updates: * ``orchestrator/tests/test_consensus_wrapper.py``: deleted the ``TestBuildConsensusWrappedCommand`` / ``TestConsensusWrapperBehavior`` / ``TestBufferOverflowDetection`` / ``TestEventDrivenWait`` / ``TestSSESigtermGrace`` classes (and the ``_force_legacy_template`` fixture that fed them). The buffer-overflow / SSE / capped-restart surfaces they covered no longer exist. The event-pump classes (``TestEventPumpTemplateSelection`` and siblings) cover the new production path; ``TestEventPumpTemplateSelection`` is reworked to pin the post-task-4-2 invariant that ``EGG_BRC_EVENT_PUMP`` is silently inert (any value, including ``false`` / ``0`` / ``no`` / ``off``, emits the event-pump template). * ``orchestrator/tests/test_consensus_wrapper_anchor.py``: deleted in full — every test pinned ``_RECOVERY_SYSTEM_PROMPT`` / ``_CONSENSUS_WRAPPER_TEMPLATE`` symbols that no longer exist. * ``orchestrator/tests/test_brc_nack_iteration.py``: removed ``TestConsensusWrapperNackFeedback`` (4 tests) that pinned the legacy recovery prompt's NACK-feedback placeholder + helper. The equivalent event-pump assertion lives in ``orchestrator/tests/test_compose_event_prompt.py``. * ``tests/sandbox/egg_agent_tools/test_handlers_message.py``: removed ``TestMessageWaitLoopHeartbeat`` (16 tests) that pinned the agent-side heartbeat path. ``TestMessageHeartbeat`` (the explicit handler tests) is unchanged. * ``integration_tests/regression/test_brc_concurrency.py``: updated the slice-2 verification-stance docstring to reflect the slice-4 post-deletion steady state (E2E deferred to #2585 via ``egg_stack``; in-process tracker coverage unchanged). Defensive grep assertions all return zero matches on ``orchestrator/consensus_wrapper.py``: rg 'consensus\.reached|sse_url|_RECOVERY_SYSTEM_PROMPT|MAX_CONSENSUS_RESTARTS' \ orchestrator/consensus_wrapper.py # → 0 hits Smoke-verified that the event-pump template is emitted regardless of ``EGG_BRC_EVENT_PUMP`` value, that the three classifiers survive in the event-pump template, and that the deleted agent-side heartbeat helpers are no longer importable from ``handlers.message``. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(#2908 slice-4 task-4-4 v2): address reviewer_code v1 NACK Five blocking findings + three non-blockers from reviewer_code's v1 NACK on PR-proposal v1. Address all five blockers; defer the non-blockers (env-var coordination is gated on coder task-4-1 / task-4-2 landing first). Blocker 1+2+3 — restored post-#2936 coder-owns-tests content in docs/guides/concurrent-execution.md (the v1 proposal overwrote the post-#2936 wording when I copied the slice-4 base, which predates the #2936 merge): - §"HANDOFF" table row example: "Coder can't push test files → HANDOFF to tester" → "Tester can't push a .github/ CI fix → HANDOFF to coder with the required end-state" (matches docs/reference/agent-roles.md). - §"Worked Example: Role-Boundary Handoff" rewrite: drop the reinstated pre-#2936 coder→tester test-handoff example, restore the post-#2936 tester→coder .github/-staging example, and keep the explicit lead sentence "the coder→tester test handoff that used to live here is gone: the coder now authors and pushes its own tests". - §"Rebase rarely conflicts" paragraph: "Rebase cannot conflict because agents have mutually exclusive file write permissions" / "role restrictions guarantee non-overlapping file sets" was a doc lie after #2936 — restore the pre-rewrite "rarely conflicts" wording and the follow-up paragraph that names the shared test scope and the serialize-by-time-not-concurrent invariant. Blocker 4+5 — dead anchors and stale §10 / §10.9 framing in docs/reference/agent-wait-patterns.md and docs/architecture/brc-memory.md: - agent-wait-patterns.md §10 retitled from "BRC Event-Pump Wrapper (slice-2, behind EGG_BRC_EVENT_PUMP)" to "BRC Consensus Wrapper (event-pump model)"; intro blockquote rewritten to drop the slice-2 "OFF by default" framing and instead describe the post-deletion steady state + rollback path; §10.8 retitled from "Flag-off as the temporary default — when slice-4 flips it" to "Rollout completed in slice-4" with body rewritten accordingly; §10.9 retitled to drop the (slice-3) suffix and the "Flag mapping" blockquote rewritten to "What's gated by what" since EGG_BRC_EVENT_PUMP no longer gates anything. - All five dead inbound anchor references repointed to the new anchors: agent-wait-patterns.md lines 1178, 1411, 1653, 1654 and brc-memory.md lines 235, 237. - Reverse direction — repointed orchestrator.md, README.md, and concurrent-execution.md cross-links from #10-brc-event-pump-wrapper-slice-2-behind-egg_brc_event_pump to #10-brc-consensus-wrapper-event-pump-model (3 occurrences in orchestrator.md, 1 each in README.md and concurrent-execution.md); same for #109-brc-per-event-prompt-composer--preamble-collapse-slice-3 → #109-brc-per-event-prompt-composer--preamble-collapse. - Verified via grep across docs/ that no remaining link points at the old anchors and no remaining body text carries the "(slice-2, behind EGG_BRC_EVENT_PUMP)" framing. Non-blockers deferred: - Rollback-plan example precision (compose_event_prompt slice-3 vs slice-2 wrapper) — useful tightening but doesn't change the correctness of the doc. - "schema is unchanged" past-tense alignment — minor. - EGG_BRC_EVENT_PUMP "no-op" vs "removed" wording — gated on coder's task-4-1 / task-4-2 final state. Will re-pass once the coder's proposal lands so the doc and code agree. * docs(#2908 slice-4 task-4-4 v3): address reviewer_code v2 NACK v2 cleared mandate 1 (all v1 blockers fixed) but mandate 2 found four new blocking findings in agent-wait-patterns.md §10.3 / §10.4 / §10.5 / §10.7 — subsection bodies still described the flag-off vs flag-on dual-emission world in present tense as if both paths still shipped, contradicting the §10 intro blockquote rewritten in v2. Address all four: - §10.3 (Heartbeat ownership) — dropped the two-row flag-off vs flag-on table; rewrote in past-tense post-migration framing mirroring orchestrator.md §"Wrapper-side heartbeat (#2036 migration completed in slice-4)" — the wrapper owns heartbeating now and the pre-#2908 agent-side path in `message_wait_loop` was deleted in slice-4 task-4-2. - §10.4 (Gateway-session keep-alive) — struck the closing "With the flag off the agent-side keep-alive still runs" sentence and replaced it with the slice-4-deletion qualifier matching §10.3 / orchestrator.md. - §10.5 (Idle / no-progress safety budget) — dropped the parenthetical "(replaces the 3-restart FAIL cap)" from the heading; dropped the two-row flag-off vs flag-on table; replaced with a single-row `EGG_BRC_IDLE_BUDGET_MIN` table mirroring orchestrator.md's steady-state version; rewrote the present-tense "MAX_CONSENSUS_RESTARTS = 3 cap" framing in past tense. - §10.7 (Verification stance) — dropped "Slice-2" from the heading; rewrote the body in past tense matching orchestrator.md's §"Verification stance — unit-test-only"; removed the "snapshot equality for the flag-off path" and "deferred to slice-4" framing (slice-4 is this work; flag-off snapshot tests were retired in slice-4 task-4-3); flipped the integration-tests bullet from "runs with EGG_BRC_EVENT_PUMP=false" to "runs against the event-pump wrapper". Adjacent cleanups for body/header coherence: - §10.1 ASCII diagram: relabelled "LEGACY (flag off, today's default)" → "PRE-#2908 (deleted in slice-4 task-4-2 — kept here for git-blame readers)" and "EVENT-PUMP (flag on)" → "STEADY STATE (event-pump, the only path after slice-4)". - §10.9.4 EGG_BRC_MEMORY mode table: marked `full` as the slice-4 default (mirrors orchestrator.md); dropped the slice-3-rollout "operators opt into full just as they opt into EGG_BRC_EVENT_PUMP=true" paragraph since EGG_BRC_EVENT_PUMP is no longer consulted. - §10.9.5 `_build_brc_preamble` collapse: rewrote the closing paragraph in past tense — the collapse runs unconditionally now because the event-pump wrapper is the only path; flipped "Slice-4 flips the wrapper default" → "Slice-4 flipped the wrapper default" so the doc reads as steady state. - §10.9.6 `mission.md` sandbox-rebuild paragraph: flipped "Slice-4's flag-flip is gated" → past-tense "The slice-4 default flip was gated". - §10.9.7 Composer / preamble verification stance: dropped "Slice-3" from the heading; rewrote in past tense matching the §10.7 rewrite; removed "deferred to slice-4" since slice-4 is this work. - §10.9.8 Architect open-decision resolutions: "resolved across slices 1–3" → "resolved across slices 1–4". - §11 Related Documentation cross-link: updated the Concurrent Execution Wrapper card from "how the wrapper uses SSE + wait-loop" (SSE machinery was deleted in slice-4 task-4-2) to "the deterministic event-pump bash loop driver". The two §10.7 non-blockers (slice-2 contract back-reference at §10.7 tail, architect-corrected-pseudocode parenthetical) survive as audit history — the reviewer marked them non-blocking and the context is still useful for future maintainers tracing the slice-2 design review. * docs(#2908 slice-4 task-4-4 v3 follow-up): EGG_BRC_EVENT_PUMP removed not no-op Reviewer_code v2 non-blocker #3 was deferred awaiting coder task-4-1 / task-4-2 final state. The coder's task-4-2 commit (15664e8) has now landed and the docstring at orchestrator/consensus_wrapper.py:35 confirms the env var itself was deleted ("the EGG_BRC_EVENT_PUMP env flag itself"), not just left as a dead branch. Update the docs to match: - docs/architecture/orchestrator.md env-vars table EGG_BRC_EVENT_PUMP row: "Deprecated no-op after slice-4" → "Removed in slice-4 task-4-2"; default "unset (no-op)" → "n/a (removed)"; added the helm-values / pod-spec drop-row note for operators that referenced it explicitly. - docs/architecture/orchestrator.md §"Operator-facing env vars (cross-link)": "the EGG_BRC_EVENT_PUMP selector is no longer consulted — setting it has no effect because the legacy template it selected to is gone" → "was removed in slice-4 task-4-2 — the env var is no longer read by the orchestrator, so setting it has no effect on a post-slice-4 codebase." - docs/architecture/orchestrator.md §"Rollback plan" partial-revert paragraph: tightened the post-slice-4-revert narrative to say the env var itself comes back when slice-4 is reverted (because task-4-2 is what deleted it), and operators wanting event-pump back set EGG_BRC_EVENT_PUMP=true (not =false — the defaults flip back to off). Also tightened the example of why reverse-merge order matters (slice-2 wrapper template references a composer slice-3 added, not "a composer that no longer exists"). - docs/reference/agent-wait-patterns.md §10.8: same shift — env var was deleted alongside the legacy template, so setting it has no effect; rollback path is reverse-merge order. * fix(#2908 slice-4 v2): address reviewer_code_holistic NACK on v1 Fix the six broken tests and four stale docstrings the holistic reviewer surfaced on v1 (the gateway-blocked test execution missed them; the structural issues are all visible from grep alone). Tests (orchestrator/tests/test_consensus_wrapper.py + orchestrator/tests/test_brc_nack_iteration.py): * Restored ``import os`` / ``import shlex`` / ``import subprocess`` — the surviving event-pump test classes still need them (``TestEventPumpConfirmFailureRaisesIdleAlert`` uses ``shlex.quote`` for stubbed PATH binaries; ``TestEventPumpHeartbeatSubshellLifecycle`` and the brc_snapshot tests use ``os.environ``). * Deleted ``TestEventPumpHeartbeatCadence::test_flag_off_heartbeat_path_unchanged`` — its invariant ("legacy template does not emit ``egg-orch message heartbeat``") no longer applies; the legacy template is gone. Replaced with an inline comment cross-linking to the post-deletion positive invariant. * Deleted ``TestEventPumpKeepAliveCadence::test_flag_off_keep_alive_remains_agent_side`` — same reason. * Deleted ``TestEventPumpIdleBudgetAlert::test_flag_off_idle_budget_not_used`` — same reason. * Deleted ``TestEventPumpRoleCompleteConfirm::test_flag_off_legacy_path_does_not_auto_call_consensus_confirmed`` — the legacy template is gone; the event-pump's confirm invocation is strictly orchestrator-driven via the ``case "$ACTION"`` arms, not auto-invoked on agent exit, so the symmetry guard is structurally satisfied. * Renamed ``TestEventPumpFlagIsolation::test_flag_on_does_not_inherit_legacy_max_restarts`` to ``test_event_pump_relies_on_idle_budget_not_legacy_restart_cap`` and dropped the ``max_restarts=7`` kwarg (the legacy kwarg was deleted from ``build_consensus_wrapped_command`` by task-4-2). The remaining assertion — ``EGG_BRC_IDLE_BUDGET_MIN`` is in the script — is the salient invariant. * Deleted ``TestEventPumpInvokesComposer::test_flag_off_legacy_template_does_not_reference_event_prompt`` — same legacy-path-only invariant. * Removed the orphaned ``assert "unresolved_nacks" in _CONSENSUS_WRAPPER_TEMPLATE`` line at the bottom of ``test_brc_nack_iteration.py`` (was left outside any function by the original ``TestConsensusWrapperNackFeedback`` deletion; this is a pure cleanup of slice-4 v1 commit 15664e8). Docstrings: * ``sandbox/egg_agent_tools/handlers/brc_memory.py:546`` — ``record_review_event`` docstring updated to reflect the slice-4 task-4-1 default flip (``EGG_BRC_MEMORY`` defaults to ``full`` now, not ``off``). * ``orchestrator/routes/event_prompt.py:787`` — CLI docstring updated to ``default full``; documents that ``off`` is the one-release rollback escape hatch and ``write-only`` keeps the writer warm without consuming the excerpt. * ``orchestrator/consensus_wrapper.py:81`` — module-level template comment rewritten: the env-flag predicate is gone, the event-pump template is the only template path post-task-4-2. * ``orchestrator/consensus_wrapper.py:723`` — ``build_event_pump_wrapped_command`` docstring rewritten to describe the post-task-4-2 reality (no env-flag gate; legacy template deleted; ``compose_event_prompt`` already wired). Defensive (addresses the non-blocking observation #1): * ``tests/sandbox/egg_agent_tools/test_handlers_message.py:TestMessageHeartbeat`` gains an autouse ``_isolate_slice_id_env`` fixture that clears ``EGG_SLICE_ID``. ``message_heartbeat`` auto-attaches ``slice_id`` from that env via ``_maybe_attach_slice_id``, so a developer-machine ``EGG_SLICE_ID`` (e.g. inside the egg sandbox) would otherwise add an unexpected key to the request body and fail the strict-equality assertions. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(#2908 slice-4 v3): address reviewer_code v1 NACK on coder v2 Blocking finding: * test_consensus_wrapper.py top-level imports missed ``import sys``; ``test_persistent_confirm_failure_fires_overseer_alert`` (the §1 + §6.2 lock-in test, the most operator-critical assertion in the file) uses ``sys.executable`` at line ~1092 and would raise NameError on execution, silently disabling the regression guard. The reviewer caught it via grep — same shape as the reviewer_code_holistic v1 NACK that surfaced the missing os/shlex/subprocess imports. Fix: add ``import sys`` alongside os/shlex/subprocess. Non-blocking findings (all addressed in this v3 since they're cheap): * TestEventPumpIdleBudgetAlert class docstring rewritten — ``The old template keeps MAX_CONSENSUS_RESTARTS verbatim`` was present-tense framing for the legacy template that task-4-2 deleted. Now reads ``The legacy template that owned the historical restart cap was deleted in slice-4 task-4-2; the idle budget is now the only liveness ceiling in the wrapper.`` * TestEventPumpFlagIsolation class renamed to TestEventPumpIdleBudgetCeiling with docstring rewritten — after task-4-2 there is no flag-on / flag-off partition to police, so the original name and ``cross-cutting guards`` framing no longer apply. The class retains its single surviving test (``test_event_pump_relies_on_idle_budget_not_legacy_restart_cap``) which is correct against the post-deletion state. * test_persistent_confirm_failure_fires_overseer_alert inline comment rewritten — ``_event_pump_enabled`` was deleted by task-4-2; the ``monkeypatch.setenv("EGG_BRC_EVENT_PUMP", "true")`` is harmlessly retained as a defensive guard against a future regression that re-introduces a flag-gated branch. Comment now reads as such. The reviewer flagged the docstring drift as non-blocking but I'm folding it into the same commit because the cost is one edit each and the docstring↔code mismatch the holistic v1 NACK called out is the same class of issue. Keeping the surface honest. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(#2908 slice-4 v3 follow-up): address reviewer_code_holistic v2 blocker #2 (test_pipeline_prompts fixture) Follow-up to v3 (e093f67 pushed) that addressed the reviewer_code v2 blocker (missing import sys). This commit addresses the reviewer_code_holistic v2 blocker (2): two pre-existing test failures in orchestrator/tests/test_pipeline_prompts.py. Root cause: the slice-4 base-merge in 06c5a6c resolved the conflict on test_pipeline_prompts.py by keeping slice-3's _PLAN_WITH_MISASSIGNED_TASK fixture (``role: coder`` + ``files: integration_tests/conftest.py``). But main's #2936 ("coder authors its own tests; tester reviews-and- hardens") explicitly excluded coder→test-files from the role↔files alignment validator. The fixture no longer trips the reject path, breaking TestPlannerRoleAlignmentValidation::test_rejects_misassigned_plan_at_propose_time and ::test_rejected_proposal_does_not_mutate_tracker. Fix: cherry-pick main's fixture update — switch the misassignment fixture from a test-file path to a docs path (docs/fixtures.md), which IS still a misassignment, since docs remain the documenter's scope. Added an explanatory comment above the fixture citing #2936 and the slice-3 merge-resolution context so future readers do not re-revert under a conflict resolution that "looks like" the slice-3 text. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(#2908 slice-4 v4): restore _auto_populate_contract + ruff I001 fix (tester v3 NACK) Tester v3 NACK had two blockers: 1. ``_auto_populate_contract_at_implement_start`` was deleted from ``orchestrator/routes/pipelines.py`` during the slice-4 base merge (commit 06c5a6c). The orphan import in ``orchestrator/tests/test_auto_populate_contract.py`` broke ``pytest --collect-only`` and blocked ``make test`` from running any tests at all (collection aborts on the first ImportError). Verified by the tester via ``git diff origin/main..origin/egg/issue-2908-impl2/slice-4`` that the function was dropped, not renamed. Fix: restored the function body verbatim from ``origin/main`` (the #2915 production implementation) and re-added the call site inside the slice-loop-mode gate where it lived on main. The function: * lives between ``_check_origin_has_plan_draft`` and ``_populate_contract_from_plan_safe`` (matches main's ordering). * is called from the ``_use_slice_loop`` check in ``_run_pipeline`` when ``_slice_count == 0``, exactly as on main. * uses ``_populate_contract_from_plan``, ``PopulateOutcome``, ``ForestValidationError``, ``_commit_statefiles_to_worktree``, and ``_pipeline_identifier`` — all present in the current file (no further imports needed). The function has a slice-4 v4 banner in its docstring explaining the restore so future merge resolutions don't re-drop it. 2. ``orchestrator/consensus_wrapper.py:50`` had a ruff I001 unsorted imports failure — an extra blank line between ``import shlex`` and the next module-level constant. Fix: removed the extra blank line (one-line deletion). Verified locally: * ``pytest --collect-only`` no longer aborts on ``ImportError: cannot import name '_auto_populate_contract_at_implement_start'``. * ``orchestrator/tests/test_auto_populate_contract.py`` imports clean. * ``orchestrator.routes.pipelines`` module imports clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(#2908 slice-4 v7): address reviewer_code NACK — 4 ruff failures Reviewer_code re-reviewed coder v6 and NACKed with 4 blocking ruff failures + 1 ruff-format failure that would block ``make lint`` in CI: 1. ``orchestrator/tests/test_consensus_wrapper.py:13-23`` — I001 unsorted-import-block (resolved as a side-effect of fixes 2 and 3 reducing the import block to a single from-import). 2. ``orchestrator/tests/test_consensus_wrapper.py:18`` — F401 ``pytest`` imported but unused. The two surviving call sites inside function bodies use ``import pytest as _pytest`` so the top-level name was dead after the v2 test deletions. Fix: remove the top-level ``import pytest``. 3. ``orchestrator/tests/test_consensus_wrapper.py:22`` — F401 ``consensus_wrapper.build_event_pump_wrapped_command`` imported but unused (zero references in the file after the test-deletion sweep). Fix: drop the second name from the from-import. 4. ``tests/sandbox/egg_agent_tools/test_handlers_message.py:10`` — F401 ``threading`` imported but unused. Slice-4 task-4-2 (15664e8) deleted the threaded ``message_wait_loop`` heartbeat machinery; the test cases that exercised it were also removed but the top-level ``import threading`` was left behind. Fix: remove the now-dead import. 5. ``orchestrator/tests/test_pipeline_prompts.py:5129-5131`` — ruff format-check failure on a multi-line assertion message. Pre- existing from the slice-3 tester commit 7cff8d1 but surfaced only now that the file is in lint scope. Fix: ``ruff format`` collapses the two-string concatenation into a single line. Verified locally: * ``ruff check .`` → ``All checks passed!`` * ``ruff format --check .`` → ``872 files already formatted`` * ``pytest orchestrator/tests/test_consensus_wrapper.py`` → 33 passed. * ``pytest tests/sandbox/egg_agent_tools/test_handlers_message.py`` → 24 passed. * ``pytest orchestrator/tests/test_pipeline_prompts.py`` → 431 passed. Non-blocking observations from reviewer_code v6 (the _auto_populate_contract restore in routes/pipelines.py and the v4 consensus_wrapper.py I001 deletion) were already verified-clean in the prior review and remain unchanged in v7. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Persist BRC history for slice-4 (#2548) * fix(#2908 slice-4): migrate test assertions off deleted capped-restart wrapper The CI Unit Tests failure on PR #2951 surfaced 18 broken tests; this commit fixes the 6 caused by Group A — call sites in two test files that the slice-4 task-4-3 sweep ("delete tests of the retired capped- restart cap") missed because they referenced ``RESTART_COUNT`` / "Restarting" / "BRC Consensus Recovery" / ``max_restarts`` / ``startup_failure_window_seconds`` rather than the symbol names listed in the original task. orchestrator/tests/test_concurrent_integration.py * ``test_spawn_agent_uses_wrapped_command``: assert event-pump markers (``event-pump``, ``egg-orch brc get-state``, ``egg-orch brc next-action``) instead of the deleted ``RESTART_COUNT`` / "BRC Consensus Recovery" strings. * Rename ``test_wrapper_contains_restart_logic`` → ``test_wrapper_drives_event_pump_loop`` and re-assert against the event-pump template. The original invariant ("orchestrator must not fake consensus on behalf of agents") is preserved — the event-pump never auto-signals READY either. orchestrator/tests/test_consensus_race_on_exit.py * Delete ``TestWrapperStaleTrackerFallback`` (4 tests) plus its unused ``os`` / ``shlex`` / ``subprocess`` / ``sys`` / ``tempfile`` imports. The class exercised ``build_consensus_wrapped_command(max_restarts=..., startup_failure_window_seconds=...)`` which slice-4 deleted in favour of the event-pump template; the event-pump reads BRC state directly via ``egg-orch brc get-state`` every loop iteration, so the wrapper no longer has a "stale tracker" of its own to fall back from. Module-docstring updated to point future readers at that history. Remaining 12 Group B failures (test_short_flow_contract_population, test_slice_4_restart_hardening) reference orchestrator production code (``_slice_agents_alive``, ``_resolve_slice_base_branch``'s ``parent_branch_exists`` kwarg, contract-runtime preservation in ``_populate_contract_from_plan``) that exists on ``origin/main`` but is missing from this branch — see PR-thread comment for the merge- regression analysis and proposed recovery paths (decision required). * fix(#2908 slice-4): restore _slice_agents_alive, parent-branch probe, runtime preservation The slice-3 → slice-4 merge resolution accidentally reverted three fixes that landed on main after slice-3 forked. This re-applies them verbatim from origin/main so the unit tests pass: * _slice_agents_alive (#2914): k8s alive guard called from the Layer-C bootstrap resume branch. Without it, a restart_phase that tore down agents but left the contract IN_PROGRESS wedges with no agents. * _resolve_slice_base_branch parent_branch_exists callback (#2928): fresh non-root slices now probe whether the derived parent branch exists on origin via ls_remote_branch_strict, replacing the pre-#2928 merge-base probe that mis-routed every fresh non-root slice onto work whenever work had advanced ahead of the parent. * _merge_preserved_slice_runtime (#2908): _populate_contract_from_plan re-parses the plan into fresh PENDING slices on every restart; the safety-net populator outside the contract_synced guard would otherwise reset COMPLETE slices and strand the pipeline on slice-1. Authored-by: egg * Restore collateral-damage reverts in pipelines.py from slice-3 + audit-log attribution from main Per egg-reviewer feedback on PR #2951, the slice-3 -> slice-4 merge (commit 06c5a6c) accidentally dropped fixes that landed on main and slice-3 between the slice-4 branch fork and the merge. The slice-4 v4 commit restored _auto_populate_contract_at_implement_start but missed several other surfaces in the same incident. Restored from slice-3 (post-#2936 coder-owns-tests framing): - _ROLE_DESCRIPTIONS tester + coder entries - _build_reviewer_preparation tester block - _build_producer_orientation tester banner - _build_agent_prompt tester branch - coder->tester HANDOFF body - _resolve_slice_base_branch derived_parent variable usage - test_tester_prep_waits_for_coder_before_writing_tests - test_tester_orientation_directs_review_and_harden_after_propose Restored from main (#2893 / #2919 audit-log attribution): - _start_stacked_pr_reconciler._list_extant_branches: orchestrator role + explanatory comment - _start_stacked_pr_reconciler._rebase_onto: orchestrator role + explanatory comment - _run_implement_phase_slices bootstrap is_slice_branch_merged_into_parent: orchestrator role + explanatory comment - _run_implement_phase_slices spawn is_slice_branch_merged_into_parent: orchestrator role + explanatory comment - _run_implement_phase_slices create_slice_integration_branch: orchestrator role + explanatory comment The slice-3 restoration brings 1 of the 6 audit-log fixes (the list_open_prs call at line 15627 was already correct on slice-3); the remaining 5 are restored directly from main since slice-3 forked before those #2919 hunks landed. After this commit, all 6 stacked-PR-reconciler hops attribute their synthetic-session gateway calls to agent_role="orchestrator" so the audit log identifies the actual caller instead of impersonating a coder. Authored-by: egg * Fix checks: align rebase_onto agent_role test with restored orchestrator attribution The previous commit (eb8c644) restored agent_role="orchestrator" in the _rebase_onto callable per #2919 audit-log attribution but did not update the test that still asserted "coder". * Update dual-role banner comment to describe event-pump mechanics The comment block at _build_brc_preamble still described the pre-slice-4 wait-loop mechanics ("the pre-PROPOSE wait-loop in step 1 of the banner below catches the coder's first CONSENSUS_PROPOSE"). Under the slice-4 event-pump model the banner body it precedes no longer contains a wait-loop step — the wrapper re-invokes the agent on each upstream CONSENSUS_PROPOSE instead. Rewrite the comment to match the rendered banner: the tester orients, exits, and is re-invoked by the event-pump wrapper when the coder proposes; subsequent re-proposes and peer-producer proposals likewise surface as fresh wrapper invocations rather than wait-loop wakes. Pure documentation; no runtime behaviour change. --------- Co-authored-by: egg <egg@example.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: egg-orchestrator <egg@localhost> Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com> Co-authored-by: James Wiesebron <jameswiesebron@khanacademy.org> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>

#2952) * feat(#2908 slice-4 task-4-1): flip EGG_BRC_EVENT_PUMP and EGG_BRC_MEMORY defaults Slice-4 task-4-1 makes the event-pump wrapper the production default by flipping two env-flag defaults: * ``EGG_BRC_EVENT_PUMP`` flips from unset→OFF (legacy) to unset→ON (event-pump). Setting ``EGG_BRC_EVENT_PUMP=false`` (or ``0`` / ``no`` / ``off``, case-insensitive) keeps the legacy capped-restart template available for a one-release rollback window. Unrecognised tokens fall through to event-pump so a typo cannot silently downgrade the production path. Slice-4 task-4-2 will delete the legacy template entirely and the env flag with it. * ``EGG_BRC_MEMORY`` flips from unset→``off`` (slice-1 inert) to unset→``full`` (event-pump composer reads memory by default). Setting ``EGG_BRC_MEMORY=off`` is the one-release rollback escape hatch. Unknown values still fail-safe to ``off`` (the fallback target stays restrictive — an undocumented value is a misconfiguration signal, NOT a write-bearing default to mask). Files touched: * ``orchestrator/consensus_wrapper.py``: - ``_event_pump_enabled()`` default flipped; falsy-token allowlist captures rollback path; docstring + module-level reframe updated. - Wrapper template's inline ``EGG_BRC_MEMORY:-off`` → ``...:-full`` so the wrapper's invocation of ``event_prompt.py`` inherits the new default even on shells that don't export the var explicitly. * ``sandbox/egg_agent_tools/handlers/brc_memory.py``: - ``get_memory_mode()`` defaults to ``MODE_FULL``; new ``MODE_DEFAULT`` constant pins the contract. * ``orchestrator/routes/event_prompt.py``: - CLI ``memory_mode`` default flipped from ``"off"`` to ``"full"``. * Tests updated to match the new defaults: - ``orchestrator/tests/test_consensus_wrapper.py``: ``TestEventPumpTemplateSelection`` rewritten — unset-env now pins event-pump, ``EGG_BRC_EVENT_PUMP=false`` pins legacy. ``TestBuildConsensusWrappedCommand``, ``TestConsensusWrapperBehavior``, ``TestBufferOverflowDetection``, ``TestEventDrivenWait``, ``TestSSESigtermGrace`` gain an autouse ``_force_legacy_template`` fixture that engages the rollback escape hatch so they continue to drive the legacy template. Slice-4 task-4-2 deletes the entire fixture + these classes alongside the legacy template. - ``tests/sandbox/egg_agent_tools/test_handlers_brc.py``: renamed ``test_unset_defaults_to_off`` → ``test_unset_defaults_to_full``; the unset-env pin now asserts the memory file is written. - ``orchestrator/tests/test_compose_event_prompt.py``: docstring note that ``write-only`` is the rollback target, not the default. Verified manually with Python smoke tests that the flag-flip works for unset, truthy, and the full falsy-token allowlist (``false`` / ``0`` / ``no`` / ``off`` / case variants), and that ``EGG_BRC_MEMORY=writeonly`` (typo) still fails safe to ``off`` with a warning while ``EGG_BRC_MEMORY=full`` and unset both enable writes + reads. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(#2908 slice-4 task-4-4): post-deletion consensus wrapper docs Rewrite docs/architecture/orchestrator.md "BRC Consensus Wrapper" section (renamed from "BRC Event-Pump Wrapper (slice-2, behind EGG_BRC_EVENT_PUMP)") to describe the post-deletion steady state. Event-pump is now the only consensus-wrapper path; the legacy capped-restart template and the agent-side heartbeat / keep-alive path were removed in slice-4 task-4-2. Changes: - docs/architecture/orchestrator.md - Renamed section to "BRC Consensus Wrapper"; updated anchor link from #brc-event-pump-wrapper-slice-2-behind-egg_brc_event_pump. - Replaced the slice-2 caveat blockquote with a four-slice rollout summary that names the deleted symbols (_CONSENSUS_WRAPPER_TEMPLATE, _RECOVERY_SYSTEM_PROMPT, SSE consensus.reached, MAX_CONSENSUS_RESTARTS). - Reframed "Why a new wrapper template" → "Why the wrapper drives the loop"; rewrote in past tense so the doc reads as if the event-pump has always been the only model. - Rewrote "Wrapper-side heartbeat (#2036 migration)" and "Wrapper-side gateway-session keep-alive (#2451 migration)" with "completed in slice-4" qualifier; described agent-side deletion. - Rewrote "Idle / no-progress safety budget" to drop the comparison table with the legacy 3-restart cap; replaced with a single behaviour table for EGG_BRC_IDLE_BUDGET_MIN. - Added new "Rollback plan" subsection documenting git revert of slice-4 → slice-3 → slice-2 → slice-1 in reverse-merge order, the integration check operators must run, and the partial-revert interaction (reverting only slice-4 restores the dual-emission state). - Renamed "Slice-2 verification stance — unit-test-only" to "Verification stance — unit-test-only"; explained that the snapshot tests pinning the byte-for-byte legacy template emission were retired in slice-4 task-4-3. - Renamed "BRC Per-Event Prompt Composer + Preamble Collapse (slice-3)" to drop the slice marker; reframed "Flag mapping" to "What's gated by what" since EGG_BRC_EVENT_PUMP no longer gates anything. - Updated EGG_BRC_MEMORY table: full is now the slice-4 default; write-only is the opt-in regression path. - Updated env vars table: EGG_BRC_EVENT_PUMP is a deprecated no-op pointing at the rollback plan; EGG_BRC_IDLE_BUDGET_MIN is no longer gated on EGG_BRC_EVENT_PUMP=true. - docs/guides/concurrent-execution.md - Replaced the slice-2 "two emission paths" caveat with a single post-deletion summary linking to the new orchestrator.md section. - Rewrote the "Consensus Wrapper" body to describe the deterministic event-pump loop (steps 1–6) as the only path; removed MAX_CONSENSUS_RESTARTS-based restart cap, the recovery system prompt, and the final-consensus-check restart cycle. - Updated the configuration table: dropped `max_restarts` row; added EGG_BRC_IDLE_BUDGET_MIN; updated transient-crash recovery paragraph to reference the idle/no-progress budget instead of the deleted MAX_CONSENSUS_RESTARTS hard cap. - docs/architecture/README.md - Updated the cross-link card to point at the renamed section and summarise the slice-4 deletion + rollback plan. Cross-links to docs/architecture/brc-memory.md (slice-1) retained throughout. The wait-side companion at agent-wait-patterns §10 is referenced from each cross-link card. Satisfies contract task-4-4. Acceptance: doc reads as if the event pump has always been the only model; legacy-path caveats removed; cross-links present; rollback plan documented; markdown renders clean (no conflict markers; section anchors resolve). * feat(#2908 slice-4 task-4-2): delete legacy capped-restart template and agent-side heartbeat Slice-4 task-4-2 collapses ``consensus_wrapper.py`` onto the event-pump template that slice-2 introduced and slice-3 wired the per-event composer into. The event-pump is now the only production path; rollback under a regression is a ``git revert`` of slices 1-3 per the PR body, not an env-flag flip. Deleted from ``orchestrator/consensus_wrapper.py``: * ``_CONSENSUS_WRAPPER_TEMPLATE`` (the ~600-line legacy capped-restart bash template). * ``_RECOVERY_SYSTEM_PROMPT`` and ``_RECOVERY_USER_PROMPT`` — the restart-time recovery prompts. * The SSE ``consensus.reached`` curl path (issue #1897) that lived inside the legacy template — the event-pump uses ``egg-orch message wait-loop`` instead. * ``MAX_CONSENSUS_RESTARTS`` (issue #2806) and its companion constants ``MAX_READY_POLL_CYCLES``, ``TRANSIENT_RESTART_BACKOFF_INITIAL``, ``STARTUP_FAILURE_WINDOW_SECONDS``. The idle/no-progress safety budget (env ``EGG_BRC_IDLE_BUDGET_MIN``, default 30 min) is the replacement liveness ceiling. * ``_event_pump_enabled()`` — the ``EGG_BRC_EVENT_PUMP`` env-flag read. The flag is now silently inert; operators with it lingering in k8s manifests can leave it set to either truthy or falsy and still get the event-pump template. * The legacy-template branch in ``build_consensus_wrapped_command``, which is now a thin alias for ``build_event_pump_wrapped_command``. Preserved by relocating into ``_EVENT_PUMP_WRAPPER_TEMPLATE`` (per task-4-2 acceptance, "Keep ``is_buffer_overflow`` / ``is_transient_crash`` / ``is_startup_failure`` classifiers"): * ``is_buffer_overflow()`` — Claude Agent SDK 1 MiB JSON reader overflow detector (#2804). * ``is_transient_crash()`` — signal-based exits (134, 136, 137, 139, 255). * ``is_startup_failure()`` — exit 1 within a 30 s startup window. * ``STARTUP_FAILURE_WINDOW_SECONDS`` — kept as a bash-scope shell variable inside the template (was a Python module constant). The classifiers are not yet wired into the event-pump's ``propose|ack|nack`` agent-invocation failure path (which uses ``AGENT_FAIL_STREAK`` + idle-budget escalation today); they live as named helpers for future revisions. Deleted from ``sandbox/egg_agent_tools/handlers/message.py``: * ``_WAIT_LOOP_HEARTBEAT_INTERVAL_SECS`` — the 60-s cadence constant. * ``_default_emit_wait_loop_heartbeat`` — the agent-side ``WAITING_FOR_EVENT`` / ``WORKING`` heartbeat emitter (#2036). * ``_start_wait_loop_heartbeat`` — the threaded periodic-tick helper. * The per-iteration ``emit_hb`` / ``stop_hb`` calls inside ``message_wait_loop``, including the ``try/finally`` block that drove the final ``WORKING`` beat on wait exit. The event-pump wrapper now owns both heartbeat liveness (#2036) and slice-scoped gateway-session keep-alive (#2451) via the wrapper- owned ``start_background_heartbeat`` subshell. ``message_heartbeat`` (the explicit handler invoked by ``egg-orch message heartbeat``) is unchanged — the wrapper calls it. Test updates: * ``orchestrator/tests/test_consensus_wrapper.py``: deleted the ``TestBuildConsensusWrappedCommand`` / ``TestConsensusWrapperBehavior`` / ``TestBufferOverflowDetection`` / ``TestEventDrivenWait`` / ``TestSSESigtermGrace`` classes (and the ``_force_legacy_template`` fixture that fed them). The buffer-overflow / SSE / capped-restart surfaces they covered no longer exist. The event-pump classes (``TestEventPumpTemplateSelection`` and siblings) cover the new production path; ``TestEventPumpTemplateSelection`` is reworked to pin the post-task-4-2 invariant that ``EGG_BRC_EVENT_PUMP`` is silently inert (any value, including ``false`` / ``0`` / ``no`` / ``off``, emits the event-pump template). * ``orchestrator/tests/test_consensus_wrapper_anchor.py``: deleted in full — every test pinned ``_RECOVERY_SYSTEM_PROMPT`` / ``_CONSENSUS_WRAPPER_TEMPLATE`` symbols that no longer exist. * ``orchestrator/tests/test_brc_nack_iteration.py``: removed ``TestConsensusWrapperNackFeedback`` (4 tests) that pinned the legacy recovery prompt's NACK-feedback placeholder + helper. The equivalent event-pump assertion lives in ``orchestrator/tests/test_compose_event_prompt.py``. * ``tests/sandbox/egg_agent_tools/test_handlers_message.py``: removed ``TestMessageWaitLoopHeartbeat`` (16 tests) that pinned the agent-side heartbeat path. ``TestMessageHeartbeat`` (the explicit handler tests) is unchanged. * ``integration_tests/regression/test_brc_concurrency.py``: updated the slice-2 verification-stance docstring to reflect the slice-4 post-deletion steady state (E2E deferred to #2585 via ``egg_stack``; in-process tracker coverage unchanged). Defensive grep assertions all return zero matches on ``orchestrator/consensus_wrapper.py``: rg 'consensus\.reached|sse_url|_RECOVERY_SYSTEM_PROMPT|MAX_CONSENSUS_RESTARTS' \ orchestrator/consensus_wrapper.py # → 0 hits Smoke-verified that the event-pump template is emitted regardless of ``EGG_BRC_EVENT_PUMP`` value, that the three classifiers survive in the event-pump template, and that the deleted agent-side heartbeat helpers are no longer importable from ``handlers.message``. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(#2908 slice-4 task-4-4 v2): address reviewer_code v1 NACK Five blocking findings + three non-blockers from reviewer_code's v1 NACK on PR-proposal v1. Address all five blockers; defer the non-blockers (env-var coordination is gated on coder task-4-1 / task-4-2 landing first). Blocker 1+2+3 — restored post-#2936 coder-owns-tests content in docs/guides/concurrent-execution.md (the v1 proposal overwrote the post-#2936 wording when I copied the slice-4 base, which predates the #2936 merge): - §"HANDOFF" table row example: "Coder can't push test files → HANDOFF to tester" → "Tester can't push a .github/ CI fix → HANDOFF to coder with the required end-state" (matches docs/reference/agent-roles.md). - §"Worked Example: Role-Boundary Handoff" rewrite: drop the reinstated pre-#2936 coder→tester test-handoff example, restore the post-#2936 tester→coder .github/-staging example, and keep the explicit lead sentence "the coder→tester test handoff that used to live here is gone: the coder now authors and pushes its own tests". - §"Rebase rarely conflicts" paragraph: "Rebase cannot conflict because agents have mutually exclusive file write permissions" / "role restrictions guarantee non-overlapping file sets" was a doc lie after #2936 — restore the pre-rewrite "rarely conflicts" wording and the follow-up paragraph that names the shared test scope and the serialize-by-time-not-concurrent invariant. Blocker 4+5 — dead anchors and stale §10 / §10.9 framing in docs/reference/agent-wait-patterns.md and docs/architecture/brc-memory.md: - agent-wait-patterns.md §10 retitled from "BRC Event-Pump Wrapper (slice-2, behind EGG_BRC_EVENT_PUMP)" to "BRC Consensus Wrapper (event-pump model)"; intro blockquote rewritten to drop the slice-2 "OFF by default" framing and instead describe the post-deletion steady state + rollback path; §10.8 retitled from "Flag-off as the temporary default — when slice-4 flips it" to "Rollout completed in slice-4" with body rewritten accordingly; §10.9 retitled to drop the (slice-3) suffix and the "Flag mapping" blockquote rewritten to "What's gated by what" since EGG_BRC_EVENT_PUMP no longer gates anything. - All five dead inbound anchor references repointed to the new anchors: agent-wait-patterns.md lines 1178, 1411, 1653, 1654 and brc-memory.md lines 235, 237. - Reverse direction — repointed orchestrator.md, README.md, and concurrent-execution.md cross-links from #10-brc-event-pump-wrapper-slice-2-behind-egg_brc_event_pump to #10-brc-consensus-wrapper-event-pump-model (3 occurrences in orchestrator.md, 1 each in README.md and concurrent-execution.md); same for #109-brc-per-event-prompt-composer--preamble-collapse-slice-3 → #109-brc-per-event-prompt-composer--preamble-collapse. - Verified via grep across docs/ that no remaining link points at the old anchors and no remaining body text carries the "(slice-2, behind EGG_BRC_EVENT_PUMP)" framing. Non-blockers deferred: - Rollback-plan example precision (compose_event_prompt slice-3 vs slice-2 wrapper) — useful tightening but doesn't change the correctness of the doc. - "schema is unchanged" past-tense alignment — minor. - EGG_BRC_EVENT_PUMP "no-op" vs "removed" wording — gated on coder's task-4-1 / task-4-2 final state. Will re-pass once the coder's proposal lands so the doc and code agree. * docs(#2908 slice-4 task-4-4 v3): address reviewer_code v2 NACK v2 cleared mandate 1 (all v1 blockers fixed) but mandate 2 found four new blocking findings in agent-wait-patterns.md §10.3 / §10.4 / §10.5 / §10.7 — subsection bodies still described the flag-off vs flag-on dual-emission world in present tense as if both paths still shipped, contradicting the §10 intro blockquote rewritten in v2. Address all four: - §10.3 (Heartbeat ownership) — dropped the two-row flag-off vs flag-on table; rewrote in past-tense post-migration framing mirroring orchestrator.md §"Wrapper-side heartbeat (#2036 migration completed in slice-4)" — the wrapper owns heartbeating now and the pre-#2908 agent-side path in `message_wait_loop` was deleted in slice-4 task-4-2. - §10.4 (Gateway-session keep-alive) — struck the closing "With the flag off the agent-side keep-alive still runs" sentence and replaced it with the slice-4-deletion qualifier matching §10.3 / orchestrator.md. - §10.5 (Idle / no-progress safety budget) — dropped the parenthetical "(replaces the 3-restart FAIL cap)" from the heading; dropped the two-row flag-off vs flag-on table; replaced with a single-row `EGG_BRC_IDLE_BUDGET_MIN` table mirroring orchestrator.md's steady-state version; rewrote the present-tense "MAX_CONSENSUS_RESTARTS = 3 cap" framing in past tense. - §10.7 (Verification stance) — dropped "Slice-2" from the heading; rewrote the body in past tense matching orchestrator.md's §"Verification stance — unit-test-only"; removed the "snapshot equality for the flag-off path" and "deferred to slice-4" framing (slice-4 is this work; flag-off snapshot tests were retired in slice-4 task-4-3); flipped the integration-tests bullet from "runs with EGG_BRC_EVENT_PUMP=false" to "runs against the event-pump wrapper". Adjacent cleanups for body/header coherence: - §10.1 ASCII diagram: relabelled "LEGACY (flag off, today's default)" → "PRE-#2908 (deleted in slice-4 task-4-2 — kept here for git-blame readers)" and "EVENT-PUMP (flag on)" → "STEADY STATE (event-pump, the only path after slice-4)". - §10.9.4 EGG_BRC_MEMORY mode table: marked `full` as the slice-4 default (mirrors orchestrator.md); dropped the slice-3-rollout "operators opt into full just as they opt into EGG_BRC_EVENT_PUMP=true" paragraph since EGG_BRC_EVENT_PUMP is no longer consulted. - §10.9.5 `_build_brc_preamble` collapse: rewrote the closing paragraph in past tense — the collapse runs unconditionally now because the event-pump wrapper is the only path; flipped "Slice-4 flips the wrapper default" → "Slice-4 flipped the wrapper default" so the doc reads as steady state. - §10.9.6 `mission.md` sandbox-rebuild paragraph: flipped "Slice-4's flag-flip is gated" → past-tense "The slice-4 default flip was gated". - §10.9.7 Composer / preamble verification stance: dropped "Slice-3" from the heading; rewrote in past tense matching the §10.7 rewrite; removed "deferred to slice-4" since slice-4 is this work. - §10.9.8 Architect open-decision resolutions: "resolved across slices 1–3" → "resolved across slices 1–4". - §11 Related Documentation cross-link: updated the Concurrent Execution Wrapper card from "how the wrapper uses SSE + wait-loop" (SSE machinery was deleted in slice-4 task-4-2) to "the deterministic event-pump bash loop driver". The two §10.7 non-blockers (slice-2 contract back-reference at §10.7 tail, architect-corrected-pseudocode parenthetical) survive as audit history — the reviewer marked them non-blocking and the context is still useful for future maintainers tracing the slice-2 design review. * docs(#2908 slice-4 task-4-4 v3 follow-up): EGG_BRC_EVENT_PUMP removed not no-op Reviewer_code v2 non-blocker #3 was deferred awaiting coder task-4-1 / task-4-2 final state. The coder's task-4-2 commit (15664e8) has now landed and the docstring at orchestrator/consensus_wrapper.py:35 confirms the env var itself was deleted ("the EGG_BRC_EVENT_PUMP env flag itself"), not just left as a dead branch. Update the docs to match: - docs/architecture/orchestrator.md env-vars table EGG_BRC_EVENT_PUMP row: "Deprecated no-op after slice-4" → "Removed in slice-4 task-4-2"; default "unset (no-op)" → "n/a (removed)"; added the helm-values / pod-spec drop-row note for operators that referenced it explicitly. - docs/architecture/orchestrator.md §"Operator-facing env vars (cross-link)": "the EGG_BRC_EVENT_PUMP selector is no longer consulted — setting it has no effect because the legacy template it selected to is gone" → "was removed in slice-4 task-4-2 — the env var is no longer read by the orchestrator, so setting it has no effect on a post-slice-4 codebase." - docs/architecture/orchestrator.md §"Rollback plan" partial-revert paragraph: tightened the post-slice-4-revert narrative to say the env var itself comes back when slice-4 is reverted (because task-4-2 is what deleted it), and operators wanting event-pump back set EGG_BRC_EVENT_PUMP=true (not =false — the defaults flip back to off). Also tightened the example of why reverse-merge order matters (slice-2 wrapper template references a composer slice-3 added, not "a composer that no longer exists"). - docs/reference/agent-wait-patterns.md §10.8: same shift — env var was deleted alongside the legacy template, so setting it has no effect; rollback path is reverse-merge order. * fix(#2908 slice-4 v2): address reviewer_code_holistic NACK on v1 Fix the six broken tests and four stale docstrings the holistic reviewer surfaced on v1 (the gateway-blocked test execution missed them; the structural issues are all visible from grep alone). Tests (orchestrator/tests/test_consensus_wrapper.py + orchestrator/tests/test_brc_nack_iteration.py): * Restored ``import os`` / ``import shlex`` / ``import subprocess`` — the surviving event-pump test classes still need them (``TestEventPumpConfirmFailureRaisesIdleAlert`` uses ``shlex.quote`` for stubbed PATH binaries; ``TestEventPumpHeartbeatSubshellLifecycle`` and the brc_snapshot tests use ``os.environ``). * Deleted ``TestEventPumpHeartbeatCadence::test_flag_off_heartbeat_path_unchanged`` — its invariant ("legacy template does not emit ``egg-orch message heartbeat``") no longer applies; the legacy template is gone. Replaced with an inline comment cross-linking to the post-deletion positive invariant. * Deleted ``TestEventPumpKeepAliveCadence::test_flag_off_keep_alive_remains_agent_side`` — same reason. * Deleted ``TestEventPumpIdleBudgetAlert::test_flag_off_idle_budget_not_used`` — same reason. * Deleted ``TestEventPumpRoleCompleteConfirm::test_flag_off_legacy_path_does_not_auto_call_consensus_confirmed`` — the legacy template is gone; the event-pump's confirm invocation is strictly orchestrator-driven via the ``case "$ACTION"`` arms, not auto-invoked on agent exit, so the symmetry guard is structurally satisfied. * Renamed ``TestEventPumpFlagIsolation::test_flag_on_does_not_inherit_legacy_max_restarts`` to ``test_event_pump_relies_on_idle_budget_not_legacy_restart_cap`` and dropped the ``max_restarts=7`` kwarg (the legacy kwarg was deleted from ``build_consensus_wrapped_command`` by task-4-2). The remaining assertion — ``EGG_BRC_IDLE_BUDGET_MIN`` is in the script — is the salient invariant. * Deleted ``TestEventPumpInvokesComposer::test_flag_off_legacy_template_does_not_reference_event_prompt`` — same legacy-path-only invariant. * Removed the orphaned ``assert "unresolved_nacks" in _CONSENSUS_WRAPPER_TEMPLATE`` line at the bottom of ``test_brc_nack_iteration.py`` (was left outside any function by the original ``TestConsensusWrapperNackFeedback`` deletion; this is a pure cleanup of slice-4 v1 commit 15664e8). Docstrings: * ``sandbox/egg_agent_tools/handlers/brc_memory.py:546`` — ``record_review_event`` docstring updated to reflect the slice-4 task-4-1 default flip (``EGG_BRC_MEMORY`` defaults to ``full`` now, not ``off``). * ``orchestrator/routes/event_prompt.py:787`` — CLI docstring updated to ``default full``; documents that ``off`` is the one-release rollback escape hatch and ``write-only`` keeps the writer warm without consuming the excerpt. * ``orchestrator/consensus_wrapper.py:81`` — module-level template comment rewritten: the env-flag predicate is gone, the event-pump template is the only template path post-task-4-2. * ``orchestrator/consensus_wrapper.py:723`` — ``build_event_pump_wrapped_command`` docstring rewritten to describe the post-task-4-2 reality (no env-flag gate; legacy template deleted; ``compose_event_prompt`` already wired). Defensive (addresses the non-blocking observation #1): * ``tests/sandbox/egg_agent_tools/test_handlers_message.py:TestMessageHeartbeat`` gains an autouse ``_isolate_slice_id_env`` fixture that clears ``EGG_SLICE_ID``. ``message_heartbeat`` auto-attaches ``slice_id`` from that env via ``_maybe_attach_slice_id``, so a developer-machine ``EGG_SLICE_ID`` (e.g. inside the egg sandbox) would otherwise add an unexpected key to the request body and fail the strict-equality assertions. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(#2908 slice-4 v3): address reviewer_code v1 NACK on coder v2 Blocking finding: * test_consensus_wrapper.py top-level imports missed ``import sys``; ``test_persistent_confirm_failure_fires_overseer_alert`` (the §1 + §6.2 lock-in test, the most operator-critical assertion in the file) uses ``sys.executable`` at line ~1092 and would raise NameError on execution, silently disabling the regression guard. The reviewer caught it via grep — same shape as the reviewer_code_holistic v1 NACK that surfaced the missing os/shlex/subprocess imports. Fix: add ``import sys`` alongside os/shlex/subprocess. Non-blocking findings (all addressed in this v3 since they're cheap): * TestEventPumpIdleBudgetAlert class docstring rewritten — ``The old template keeps MAX_CONSENSUS_RESTARTS verbatim`` was present-tense framing for the legacy template that task-4-2 deleted. Now reads ``The legacy template that owned the historical restart cap was deleted in slice-4 task-4-2; the idle budget is now the only liveness ceiling in the wrapper.`` * TestEventPumpFlagIsolation class renamed to TestEventPumpIdleBudgetCeiling with docstring rewritten — after task-4-2 there is no flag-on / flag-off partition to police, so the original name and ``cross-cutting guards`` framing no longer apply. The class retains its single surviving test (``test_event_pump_relies_on_idle_budget_not_legacy_restart_cap``) which is correct against the post-deletion state. * test_persistent_confirm_failure_fires_overseer_alert inline comment rewritten — ``_event_pump_enabled`` was deleted by task-4-2; the ``monkeypatch.setenv("EGG_BRC_EVENT_PUMP", "true")`` is harmlessly retained as a defensive guard against a future regression that re-introduces a flag-gated branch. Comment now reads as such. The reviewer flagged the docstring drift as non-blocking but I'm folding it into the same commit because the cost is one edit each and the docstring↔code mismatch the holistic v1 NACK called out is the same class of issue. Keeping the surface honest. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(#2908 slice-4 v3 follow-up): address reviewer_code_holistic v2 blocker #2 (test_pipeline_prompts fixture) Follow-up to v3 (e093f67 pushed) that addressed the reviewer_code v2 blocker (missing import sys). This commit addresses the reviewer_code_holistic v2 blocker (2): two pre-existing test failures in orchestrator/tests/test_pipeline_prompts.py. Root cause: the slice-4 base-merge in 06c5a6c resolved the conflict on test_pipeline_prompts.py by keeping slice-3's _PLAN_WITH_MISASSIGNED_TASK fixture (``role: coder`` + ``files: integration_tests/conftest.py``). But main's #2936 ("coder authors its own tests; tester reviews-and- hardens") explicitly excluded coder→test-files from the role↔files alignment validator. The fixture no longer trips the reject path, breaking TestPlannerRoleAlignmentValidation::test_rejects_misassigned_plan_at_propose_time and ::test_rejected_proposal_does_not_mutate_tracker. Fix: cherry-pick main's fixture update — switch the misassignment fixture from a test-file path to a docs path (docs/fixtures.md), which IS still a misassignment, since docs remain the documenter's scope. Added an explanatory comment above the fixture citing #2936 and the slice-3 merge-resolution context so future readers do not re-revert under a conflict resolution that "looks like" the slice-3 text. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(#2908 slice-4 v4): restore _auto_populate_contract + ruff I001 fix (tester v3 NACK) Tester v3 NACK had two blockers: 1. ``_auto_populate_contract_at_implement_start`` was deleted from ``orchestrator/routes/pipelines.py`` during the slice-4 base merge (commit 06c5a6c). The orphan import in ``orchestrator/tests/test_auto_populate_contract.py`` broke ``pytest --collect-only`` and blocked ``make test`` from running any tests at all (collection aborts on the first ImportError). Verified by the tester via ``git diff origin/main..origin/egg/issue-2908-impl2/slice-4`` that the function was dropped, not renamed. Fix: restored the function body verbatim from ``origin/main`` (the #2915 production implementation) and re-added the call site inside the slice-loop-mode gate where it lived on main. The function: * lives between ``_check_origin_has_plan_draft`` and ``_populate_contract_from_plan_safe`` (matches main's ordering). * is called from the ``_use_slice_loop`` check in ``_run_pipeline`` when ``_slice_count == 0``, exactly as on main. * uses ``_populate_contract_from_plan``, ``PopulateOutcome``, ``ForestValidationError``, ``_commit_statefiles_to_worktree``, and ``_pipeline_identifier`` — all present in the current file (no further imports needed). The function has a slice-4 v4 banner in its docstring explaining the restore so future merge resolutions don't re-drop it. 2. ``orchestrator/consensus_wrapper.py:50`` had a ruff I001 unsorted imports failure — an extra blank line between ``import shlex`` and the next module-level constant. Fix: removed the extra blank line (one-line deletion). Verified locally: * ``pytest --collect-only`` no longer aborts on ``ImportError: cannot import name '_auto_populate_contract_at_implement_start'``. * ``orchestrator/tests/test_auto_populate_contract.py`` imports clean. * ``orchestrator.routes.pipelines`` module imports clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(#2908 slice-4 v7): address reviewer_code NACK — 4 ruff failures Reviewer_code re-reviewed coder v6 and NACKed with 4 blocking ruff failures + 1 ruff-format failure that would block ``make lint`` in CI: 1. ``orchestrator/tests/test_consensus_wrapper.py:13-23`` — I001 unsorted-import-block (resolved as a side-effect of fixes 2 and 3 reducing the import block to a single from-import). 2. ``orchestrator/tests/test_consensus_wrapper.py:18`` — F401 ``pytest`` imported but unused. The two surviving call sites inside function bodies use ``import pytest as _pytest`` so the top-level name was dead after the v2 test deletions. Fix: remove the top-level ``import pytest``. 3. ``orchestrator/tests/test_consensus_wrapper.py:22`` — F401 ``consensus_wrapper.build_event_pump_wrapped_command`` imported but unused (zero references in the file after the test-deletion sweep). Fix: drop the second name from the from-import. 4. ``tests/sandbox/egg_agent_tools/test_handlers_message.py:10`` — F401 ``threading`` imported but unused. Slice-4 task-4-2 (15664e8) deleted the threaded ``message_wait_loop`` heartbeat machinery; the test cases that exercised it were also removed but the top-level ``import threading`` was left behind. Fix: remove the now-dead import. 5. ``orchestrator/tests/test_pipeline_prompts.py:5129-5131`` — ruff format-check failure on a multi-line assertion message. Pre- existing from the slice-3 tester commit 7cff8d1 but surfaced only now that the file is in lint scope. Fix: ``ruff format`` collapses the two-string concatenation into a single line. Verified locally: * ``ruff check .`` → ``All checks passed!`` * ``ruff format --check .`` → ``872 files already formatted`` * ``pytest orchestrator/tests/test_consensus_wrapper.py`` → 33 passed. * ``pytest tests/sandbox/egg_agent_tools/test_handlers_message.py`` → 24 passed. * ``pytest orchestrator/tests/test_pipeline_prompts.py`` → 431 passed. Non-blocking observations from reviewer_code v6 (the _auto_populate_contract restore in routes/pipelines.py and the v4 consensus_wrapper.py I001 deletion) were already verified-clean in the prior review and remain unchanged in v7. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Persist BRC history for slice-4 (#2548) * docs(#2908 slice-5 task-5-4): prose-arg channels + brc verb-level CLI Document the slice-5 additive CLI surface across the four docs that already carry consensus-protocol prose: - docs/reference/orchestrator-cli.md * New "Prose-bearing args: stdin and --*-file channels (#2741)" subsection under ## BRC Consensus Protocol covering --summary-file, --reason-file, --files-reviewed-file, and the stdin sentinel `-`. * New "## BRC verb-level operations (egg-orch brc)" section documenting the next-action / get-state / list-blocking / resolve-obligation / read-peer-artifact subcommands. * Deprecation-warning note on the argv --summary / --reason path. - docs/reference/agent-tools.md * MCP↔CLI table: mcp__brc__get_state, mcp__brc__list_blocking (slice-1), and mcp__brc__read_peer_artifact, mcp__brc__resolve_obligation (slice-5) flipped from "no CLI" to their new egg-orch brc subcommands. * cli_command=None rationale list: drop the four promoted verbs and add a callout summarizing the slice-5 promotion. * Schema-derivation paragraph: shrink the "tools with no CLI" list accordingly. - docs/reference/agent-wait-patterns.md * Update re-propose / stale-version examples to use --summary-file / --reason-file (the canonical idiom for any wrapper-composed CLI). * New "Prose-bearing args use stdin / --*-file, not argv (#2741)" subsection under §1 with channel table, examples, and rationale. * Related Documentation: cross-link to the new orchestrator-cli.md BRC verb-level operations section and to #2741. - docs/guides/concurrent-execution.md * Refresh the worked Consensus Protocol example to use --summary-file for propose, --reason-file for ack, and the stdin sentinel for nack / withdraw. Add a brc resolve-obligation example. * New "egg-orch brc — verb-level read/derive surface" subsection cross-linking the canonical reference in orchestrator-cli.md. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * feat(#2908 slice-5 task-5-1/5-2/5-3): prose-arg channels + brc CLI surface Slice-5 of the BRC event-pump rollout extends the CLI surface for the in-bash wrapper that ships in slice-2. Three coder tasks land here: task-5-1 — Prose-arg channels for consensus propose/ack/nack/withdraw. The wrapper composes CLI invocations via ``bash -c``, so argv-only prose (``--summary "$RAW"``, ``--reason "$RAW"``) gets corrupted by shell metacharacters (``$VAR`` / backticks / ``$()`` / ``;`` / ``&&`` / embedded newlines) — the #2741 failure mode. This slice generalises the mitigation: every prose-bearing arg now offers a paired ``--FOO-file PATH`` flag and accepts ``-`` as the argv sentinel for stdin. Argv prose still works for humans and during transition but emits ``DeprecationWarning`` so a regression to argv-only inside the wrapper surfaces. ``--files-reviewed-file PATH`` carries an array with one path per line (blank lines and ``#`` comments stripped), per architect v2 §verification_strategy.slice_5. Two helpers in orch_cli.py — ``_resolve_prose_arg`` and ``_resolve_files_reviewed_arg`` — handle channel selection (file → stdin → argv), enforce mutual exclusion, and emit the deprecation. task-5-2 — ``egg-orch brc resolve-obligation`` CLI. Verb-level wrapper around ``mcp__brc__resolve_obligation`` (#2338). Slice-6 deletes the agent-side MCP server, so the wrapper bash needs this verb reachable without an MCP round-trip. Args mirror the handler: ``--reviewer-role`` and ``--producer-role`` are required; ``--commit-sha`` and ``--note`` are optional. The ``--note`` flag uses the same prose-arg plumbing as the other reason / summary args. task-5-3 — ``egg-orch brc read-peer-artifact`` CLI. Verb-level wrapper around ``mcp__brc__read_peer_artifact``. Stdout JSON; pagination via ``--limit`` + opaque ``--cursor`` round-trip; ``--message-type`` is ``action="append"`` for repeated use; ``--no-include-unattributed`` opts out of the slice-scoped + cross-cutting merge (default on, per the handler's per-slice-partition contract from #2548). Tests authored by the coder (tester reviews-and-hardens): * ``tests/sandbox/egg_lib/test_orch_cli_prose_args.py`` — #2741 regression-guard. Parametrises seven representative prose payloads (``$VAR`` / backticks / ``$()`` / shell-control / newline+tab / UTF-8 / quotes+escapes) across each delivery channel (file, stdin sentinel, argv) for ``consensus propose``, ``ack``, ``nack``, and ``withdraw``. Asserts byte-equality between the on-disk / stdin input and the request body received by the orchestrator fake. Argv-path tests assert the ``DeprecationWarning`` fires. Mutual- exclusion paths return exit 2 with helpful stderr. ``--files- reviewed-file`` one-path-per-line semantics covered (blank lines + ``#`` comments stripped). The ``consensus propose --file`` JSON payload path (from issue #1738) is explicitly tested to NOT emit the deprecation warning — only the per-arg argv channels are deprecated. * ``tests/sandbox/egg_lib/test_orch_cli_brc.py`` — extends slice-1's test file with ``TestBrcResolveObligation`` (happy path / commit SHA / note via file / note via stdin / help / parser registration) and ``TestBrcReadPeerArtifact`` (happy path / peer-role filter / message-type list / limit+cursor pagination round-trip / no-include-unattributed default flip / phase choices restricted / help / parser registration). All 306 tests pass on the changed paths; existing consensus-push, slice-1 BRC, and CLI parity tests continue to pass unchanged. Files: sandbox/egg_lib/orch_cli.py; tests/sandbox/egg_lib/test_orch_cli_brc.py; tests/sandbox/egg_lib/test_orch_cli_prose_args.py. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * docs(#2908 slice-5 task-5-4 v2): address reviewer_code v1 NACK Address three blocking findings plus the in-scope non-blocking ones raised by reviewer_code on the v1 documenter proposal. Blocking 1 — `--files-reviewed-file` is one-path-per-line (NOT JSON): The `_resolve_files_reviewed_arg` handler in orch_cli.py:884-922 reads the file as newline-delimited paths, strips blank lines and `#`-prefixed comments, and never calls `json.loads`. Update the example comment in orchestrator-cli.md to say "one path per line; blank lines and `#` comments stripped" and rewrite the heredoc example to demonstrate the comment-stripping behavior. Mirror the clarification in agent-wait-patterns.md. Blocking 2 — schema-derivation claim was wrong: The four BRC tool registrations in sandbox/egg_agent_tools/tools/brc.py (get_state / list_blocking / read_peer_artifact / resolve_obligation) ALL still declare `cli_command=None`. Slice-1 / slice-5 added thin CLI wrappers (`egg-orch brc <verb>`) over the same handlers but deliberately did NOT flip the registrations. The MCP-side schemas continue to be hand-authored in `schemas.py`; the `derive_schema_from_argparse` path is skipped. Restore the four BRC verbs to the `cli_command=None` bullet list with the additional context that a thin CLI wrapper exists; revise the "promoted to CLI" callout to "CLI surface added (registration unchanged)"; revise the schema-derivation paragraph; tag the CLI- counterpart cells with "thin wrapper, registration still cli_command=None — see callout below". Blocking 3 — `brc read-peer-artifact` does NOT use the gateway: The handler reads `.egg-state/brc-history/<identifier>-<phase>.json` files from local disk (verified: no `orchestrator_request(...)` call in `brc_read_peer_artifact`). EGG_ORCHESTRATOR_URL / EGG_LIFECYCLE_SECRET do not apply. Rewrite the "all five subcommands" sentence in orchestrator-cli.md to scope the auth claim to the other four and explain the local-disk semantics so operators don't misdiagnose missing-secret failures. Non-blocking (in-scope to the row I touched): - agent-tools.md: fix the pre-existing handler typo `handlers.brc.read_peer_artifact` → `handlers.brc.brc_read_peer_artifact` (every sibling row uses the brc_ prefix). - agent-tools.md: tighten the read_peer_artifact description to mention `<identifier>-<phase>.json` (not `<pipeline_id>`), the per-slice `<identifier>-implement-<slice_id>.json` partition, the unattributed sibling merge + `include_unattributed=False` toggle, and the `message_type` filter (single value or list). - concurrent-execution.md: author a distinct `reviewer-code-cond-ack.md` for the conditional ACK example so the prose narrative matches the obligation case (instead of re-using the unconditional ACK file). - concurrent-execution.md: show the `cat > /tmp/obligation-resolved.md` heredoc step on the `brc resolve-obligation` example (every other prose-arg example in the same section creates the file inline). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(#2908 slice-5 v2): address tester v1 NACK — catch UnicodeDecodeError, return rc=2 Tester v1 NACK on coder v1 (commit 0a8a7f6): `_resolve_prose_arg` caught `OSError` but not `UnicodeDecodeError`, so a binary or non-UTF-8 file passed to `--reason-file` / `--summary-file` / `--note-file` / `--files-reviewed-file` raised a raw traceback to the wrapper bash instead of the actionable `Error: failed to read ...` message. The tester's one-line fix recommendation was to add `UnicodeDecodeError` to the `except` clauses. That's done. While there, also aligned the helpers to the established orch_cli pattern of `return 2` from `cmd_*` (the same pattern `cmd_consensus_ack` already uses for its `--pre-merge-condition-resolved-in-diff` guard): * Added `_ProseArgError` sentinel exception. `_resolve_prose_arg` and `_resolve_files_reviewed_arg` now `raise _ProseArgError` on any CLI-level validation failure (mutual exclusion, missing required arg, file-read failure incl. `UnicodeDecodeError`); the cmd_* functions catch it and return rc=2. The stderr error message is emitted by the helper before the raise — cmd_* only translates the exception to the exit code. * No more `sys.exit(2)` inside the helpers — `sys.exit` from within a cmd_* call raises `SystemExit`, which fails pytest tests that expect a clean returned rc (the tester's bug-finding test, `test_non_utf8_file_surfaces_clean_error_not_traceback`, makes this explicit by checking `assert rc == 2` after the call). Tester's adversarial test file pulled in (tester committed it as `tests/sandbox/egg_lib/test_orch_cli_prose_args_adversarial.py` at 847985f3d8). Two of the tester's tests that used `pytest.raises(SystemExit)` updated to the new `return rc=2` contract: * `TestProseFileReadErrors::test_missing_reason_file_path_surfaces_clean_error` * `TestProseArgEmptyEdges::test_empty_string_argv_treated_as_missing` Both now assert `rc == 2` returned. The `test_invalid_phase_rejected_at_parse_time` test continues to use `pytest.raises(SystemExit)` because argparse's `choices=` rejection is genuinely a parse-time SystemExit, not a cmd_* validation path. Mirror change in `tests/sandbox/egg_lib/test_orch_cli_prose_args.py`: the coder-authored `test_reason_and_reason_file_mutually_exclusive`, `test_missing_reason_fails_cleanly`, and `test_files_reviewed_and_file_mutually_exclusive` likewise switch from `pytest.raises(SystemExit)` to `assert rc == 2` + `capsys` stderr inspection. Verification: * All 142 tests in `tests/sandbox/egg_lib/` pass. * Broader regression: 193 tests across `egg_agent_tools/test_handlers_brc.py`, `test_cli_parity.py`, `test_orch_cli_consensus_push.py`, and `test_orch_cli_slice_id.py` pass unchanged. * ruff check + ruff format clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * test(#2908 slice-5 task-5-7): MCP-surface latency baseline capture Adds the kubectl-gated integration test ``integration_tests/test_mcp_baseline_capture.py`` that drives a real-LLM 5-role consensus through the still-live MCP surface (slice-5 is additive only) and writes per-event wall-clock samples to ``.egg-state/agent-outputs/latency-mcp-baseline.json``. The schema (``samples: [{role, event_type, start_ts, end_ts, duration_seconds, exit_code}]`` plus aggregate p50/p95/max/sum) is the input slice-6 TASK-6-6 will compare against the CLI-only baseline it captures after the MCP→CLI collapse lands. The test skips cleanly when ``_kubectl_available()`` returns False via the session-scoped ``egg_stack`` fixture, and also skips when the gateway is unhealthy (dummy GH creds in CI) — mirroring the guard used by ``test_orchestrator_mcp_contract.py``. Also commits a synthetic placeholder baseline JSON marked ``_meta.synthetic: true`` so slice-6 has a file to read while the capture test waits on a real-LLM run; ``_meta.synthetic_reason`` explains exactly how to regenerate. No ``ScriptedProvider`` import / reference (per the slice-5 plan re-scope: real LLM route, not in-process provider swap). * Persist BRC history for slice-5 (#2548) * Fix mypy errors: assert file_path non-None before open() in orch_cli * fix(#2908 slice-4): migrate test assertions off deleted capped-restart wrapper The CI Unit Tests failure on PR #2951 surfaced 18 broken tests; this commit fixes the 6 caused by Group A — call sites in two test files that the slice-4 task-4-3 sweep ("delete tests of the retired capped- restart cap") missed because they referenced ``RESTART_COUNT`` / "Restarting" / "BRC Consensus Recovery" / ``max_restarts`` / ``startup_failure_window_seconds`` rather than the symbol names listed in the original task. orchestrator/tests/test_concurrent_integration.py * ``test_spawn_agent_uses_wrapped_command``: assert event-pump markers (``event-pump``, ``egg-orch brc get-state``, ``egg-orch brc next-action``) instead of the deleted ``RESTART_COUNT`` / "BRC Consensus Recovery" strings. * Rename ``test_wrapper_contains_restart_logic`` → ``test_wrapper_drives_event_pump_loop`` and re-assert against the event-pump template. The original invariant ("orchestrator must not fake consensus on behalf of agents") is preserved — the event-pump never auto-signals READY either. orchestrator/tests/test_consensus_race_on_exit.py * Delete ``TestWrapperStaleTrackerFallback`` (4 tests) plus its unused ``os`` / ``shlex`` / ``subprocess`` / ``sys`` / ``tempfile`` imports. The class exercised ``build_consensus_wrapped_command(max_restarts=..., startup_failure_window_seconds=...)`` which slice-4 deleted in favour of the event-pump template; the event-pump reads BRC state directly via ``egg-orch brc get-state`` every loop iteration, so the wrapper no longer has a "stale tracker" of its own to fall back from. Module-docstring updated to point future readers at that history. Remaining 12 Group B failures (test_short_flow_contract_population, test_slice_4_restart_hardening) reference orchestrator production code (``_slice_agents_alive``, ``_resolve_slice_base_branch``'s ``parent_branch_exists`` kwarg, contract-runtime preservation in ``_populate_contract_from_plan``) that exists on ``origin/main`` but is missing from this branch — see PR-thread comment for the merge- regression analysis and proposed recovery paths (decision required). * fix(#2908 slice-4): restore _slice_agents_alive, parent-branch probe, runtime preservation The slice-3 → slice-4 merge resolution accidentally reverted three fixes that landed on main after slice-3 forked. This re-applies them verbatim from origin/main so the unit tests pass: * _slice_agents_alive (#2914): k8s alive guard called from the Layer-C bootstrap resume branch. Without it, a restart_phase that tore down agents but left the contract IN_PROGRESS wedges with no agents. * _resolve_slice_base_branch parent_branch_exists callback (#2928): fresh non-root slices now probe whether the derived parent branch exists on origin via ls_remote_branch_strict, replacing the pre-#2928 merge-base probe that mis-routed every fresh non-root slice onto work whenever work had advanced ahead of the parent. * _merge_preserved_slice_runtime (#2908): _populate_contract_from_plan re-parses the plan into fresh PENDING slices on every restart; the safety-net populator outside the contract_synced guard would otherwise reset COMPLETE slices and strand the pipeline on slice-1. Authored-by: egg * Address PR #2952 review feedback (egg-reviewer) Finding 1: convert three Python-2-looking ``except E1, E2:`` clauses in integration_tests/test_mcp_baseline_capture.py (lines 172, 210, 369) to parenthesized tuple form. Ruff format actively strips parens off bare ``except (E1, E2):`` (no binding) — pin with ``# fmt: skip`` so the clearer form survives the formatter. Finding 2: extend the slice-5 prose-arg plumbing to the two remaining prose-bearing flags the reviewer flagged. ``consensus propose --risk`` gains ``--risk-file PATH`` and ``--risk -`` stdin sentinel; ``consensus ack --pre-merge-condition`` likewise gains ``--pre-merge-condition-file PATH`` and stdin sentinel. Argv path still works but emits the same DeprecationWarning as ``--summary`` / ``--reason``. Docs and prose-arg test surface updated; ``--pre-merge-condition-resolved-in-diff`` deliberately not exposed (it carries a commit SHA, not prose). Finding 3: add a TODO(slice-6 TASK-6-6) block to the test_mcp_baseline_capture.py module docstring naming the synthetic- baseline trip-wire — slice-6's TASK-6-6 must hard-gate on ``_meta.synthetic`` so the 5% latency budget cannot pass by coincidence against placeholder p50/p95 numbers. --------- Co-authored-by: egg <egg@example.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com> Co-authored-by: egg-orchestrator <egg@localhost> Co-authored-by: james-in-a-box[bot] <246424927+james-in-a-box[bot]@users.noreply.github.com> Co-authored-by: jwbron <8340608+jwbron@users.noreply.github.com> Co-authored-by: James Wiesebron <jameswiesebron@khanacademy.org> Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>

* fix(sandbox): make check_file_restriction phase-aware (#2968) check_file_restriction reported can_write from the role layer (shared/egg_restrictions/patterns.py) only, ignoring the gateway's phase-layer push gate (gateway/phase_filter.py). It therefore returned can_write:true for paths the phase gate rejects -- e.g. the refiner writing .egg-state/drafts/*-plan.md during refine, which is reserved to the plan phase. On pipeline-8cf1f000 a reviewer trusted the tool and NACKed the producer for a "false gateway claim" that was in fact a true phase-gate block, burning a v1->v4 BRC cycle. can_write is now the conjunction of BOTH gateway push gates, so it predicts push acceptance. Split verdicts (role_can_write, phase_allows, blocked_by, phase) show which gate fires; a phase block carries no alternative_role and tells the agent to defer to the owning phase. - New shared/egg_restrictions/phase_patterns.py mirrors the gateway's phase data + matching logic for phase-blind callers (the MCP tool runs in the sandbox, nowhere near the gateway). A gateway-side parity test pins the mirror to the live PhaseFilter so it cannot drift. Consolidating the gateway onto this constant (parallel to #1903 for the role layer) is left as a follow-up to keep this change off the security-critical push path. - Optional `phase` tool arg; defaults to EGG_PHASE. An unset phase makes the phase layer a no-op (role-only, pre-#2968 behavior). Also repoints two test_restrictions_handlers.py cases that went stale in #2936 (the coder now authors its own tests, so it is no longer blocked from tests/) onto docs/guide.md, and adds the previously-missing inverse gateway test that refine blocks *-plan.md. * review: address PR #2975 feedback - Fail-closed mirror on unknown/off-canonical phases (matches gateway). - Reject non-string `phase` arg with HandlerError. - Schema description now mentions both gateway push gates explicitly. - Reviewer UX nudge added to tool description + agent-tools.md row. - New parity test: shared mirror fails closed for off-canonical phases. - New handler test: reviewer impersonates producer via explicit args. - Drop dead "pr" entries from .egg/phase-permissions.json. * review: address PR #2975 nits — module-level import + empty-string doc - Pull PipelinePhase import from inside phase_file_verdict to module scope. Peer module shared/egg_restrictions/patterns.py already imports egg_contracts.agent_roles at module scope without circular-import drama, and the parity test imports PipelinePhase at module scope too, so the defensive lazy import was solving a hypothetical problem and paying a sys.modules lookup on every batch _check_one call. - Tighten phase_file_verdict docstring to acknowledge the small, unreachable-in-practice divergence vs. the gateway on explicit phase="" (gateway fails closed via PipelinePhase(""); mirror returns (True, None)). Documents how to close the divergence if a future caller exposes phase_file_verdict to a path that doesn't pre-normalise "" → None. --------- Co-authored-by: egg-reviewer[bot] <261018737+egg-reviewer[bot]@users.noreply.github.com>