[issue-2908][slice-4/6] Flag flip + delete old capped-restart wrapp...

[james-in-a-box]

Context

BRC consensus today depends on the agent re-entering a blocking
egg-orch message wait-loop between every event. That re-entry is a
seam the model can fall out of by emitting a final assistant
message instead of re-entering the wait. Claude usually re-enters;
qwen3.7-max does not (#2906) — it exits success=True after one
match, the wrapper sees no CONSENSUS_CONFIRMED, the 3-restart cap
trips (#2806), and the pipeline FAILs after ~$1 and ~20 min of
churn. Prompt-only mitigations narrow the seam for one model; the
seam itself exists for every model (lineage: #2323, #2064, #2482,
#2036, #1995, #2451).

Changes

Reframe consensus-agent execution from a long-lived participant
that holds blocking waits into a deterministic wrapper-driven
event pump that invokes the agent one-shot per actionable event,
with continuity carried by a durable per-role memory file. Lands
in six linear slices behind EGG_BRC_EVENT_PUMP (default false
until slice-4):

1. slice-1 — Foundations. New egg-orch brc next-action,
   brc get-state, brc list-blocking, phase get-context CLI
   subcommands. Durable BRC memory artifact at
   .egg-state/agent-outputs/<role>/brc-memory.md with
   action-scaffolded writes into brc_ack / brc_nack handlers,
   atomic writes via promoted _persist_atomic_template,
   last_reviewed_commit_sha per producer in the schema,
   fail-closed path construction. Gated by
   EGG_BRC_MEMORY=write-only (writes accumulate; reads land
   in slice-3). Purely additive — zero behavior change.
2. slice-2 — Event-pump wrapper. Rewrite
   orchestrator/consensus_wrapper.py as a deterministic event
   pump gated by EGG_BRC_EVENT_PUMP. Drop the 3-restart cap;
   replace with idle/no-progress safety budget. Migrate the
   heartbeat (Overseer flags BRC reviewers/testers as stalled when they're correctly blocked in mcp__brc__wait_loop (implement-phase false positive) #2036) and gateway-session keep-alive (Orchestrator heartbeat-session lookup fails: container_id missing slice-N segment for non-coder roles #2451) from
   the agent-side handler into the wrapper's blocking wait.
   Heartbeat payload carries slice_id from EGG_SLICE_ID
   (regression guard). Verification is unit-test-only — no
   in-process test double can drive a deployed pod end-to-end
   (the pod-injection avenue was ruled out per Expand integration test coverage #2474); true
   E2E deferred to slice-4 via the egg_stack real-pod
   fixture. Old path retained verbatim alongside.
3. slice-3 — Delta + prompt collapse. Wire per-event
   invocation to hand the agent the memory delta plus the full
   git log {last_reviewed_commit_sha}..HEAD --not origin/{base_branch} -p
   delta per producer (NOT just orchestrator-side
   changed_artifacts — per REVIEWER-SYNC.md the re-review must
   audit the full delta as a fresh review). Strip the
   STAY-ALIVE / wait-loop / cursor-threading guidance from
   _build_brc_preamble and mission.md; replace with a lean
   event-handler contract. Sandbox image rebuilt + agent pod
   restarted BEFORE slice-4 flag flip.
4. slice-4 — Flag flip + delete old path. Flip the
   EGG_BRC_EVENT_PUMP / EGG_BRC_MEMORY defaults to on, gated on
   the slice-2 / slice-3 unit + BRC in-process regression suites
   passing on the new default; delete the capped-restart bash, the
   _RECOVERY_SYSTEM_PROMPT, the SSE machinery, and the
   agent-side wait_loop heartbeat code. Rollback plan: revert
   slices 1–3 via git revert if production traffic regresses. The
   qwen3.7-max qwen3.7-max BRC agents end their agentic loop early (success=True) without reaching CONSENSUS_CONFIRMED → consensus-wrapper restart churn #2906 k3s spike that previously validated this
   end-to-end was removed (qwen route unavailable in k3s;
   Claude-route E2E deferred to Rewrite TestCredentialIsolation as k3s-native (currently skipped under k3s) #2585).
5. slice-5 — Additive CLI prose plumbing + 2 new BRC
   subcommands. Add stdin / --reason-file / --summary-file
   / --files-reviewed-file plumbing to
   consensus propose --summary, consensus ack --reason,
   consensus nack --reason, consensus withdraw --reason
   (docs: steer agents to the structured contract tool, not Bash-composed egg-contract #2741 regression guard). Add egg-orch brc resolve-obligation
   and egg-orch brc read-peer-artifact CLI subcommands the
   slice-6 deletion depends on. Argv kept as fallback during
   transition (deprecation warning).
6. slice-6 — MCP → CLI deletion. Delete the 28 agent-facing
   MCP tools (~1,515 LOC across 7 namespace files + the 4
   infra files + server.py), the SYSTEM_PROMPT_NUDGE, and the
   MCP registration block in shared/egg_agent/client.py:299-353
   INCLUDING the EGG_MCP_TOOLS env flag at :311 (no orphan
   flag). Retire tests/tools/test_mcp_cli_drift.py. Migrate the
   MCP E2E test so the agent's first action is consensus ack/nack via stdin/file (preserves SDK-spawn exercise).
   Verify per-event wall-clock latency within 5%.

Impact

Operator-facing: the BRC consensus subsystem becomes
model-portable — any agent that exits naturally after handling
one event reaches CONFIRMED, instead of needing prompt nudges to
keep re-entering an in-process wait. Cost-per-phase drops because
restart churn disappears, replaced by short bounded per-event
invocations that hit the prefix cache (≥ 60-min TTL on both
routes per WS7 closure). Net code deletion: the capped-restart
template, the SSE machinery, the recovery system prompt, the 28
MCP tool schemas, the EGG_MCP_TOOLS flag, the cursor-threading
guidance, and the agent-side heartbeat all go away. The agent
primitive (pod / worktree / SDK / permissions / restrictions) is
untouched.

This slice

Flag flip + delete old capped-restart wrapper path

Files affected:

• orchestrator/consensus_wrapper.py
• sandbox/egg_agent_tools/handlers/message.py
• orchestrator/tests/test_consensus_wrapper.py
• tests/sandbox/egg_agent_tools/test_handlers_message.py
• docs/architecture/orchestrator.md

Tasks:

• task-4-1: Flip the EGG_BRC_EVENT_PUMP default in orchestrator/consensus_wrapper.py's build_consensus_wrapped_command from false to true. With the default flipped, the new template path is the production path; the old template path is only emitted when an operator sets EGG_BRC_EVENT_PUMP=false explicitly. Same flip for EGG_BRC_MEMORY from off to full so the delta-scoped re-analysis from slice-3 reads the memory file in production. Pre-flight: the slice-2 / slice-3 unit + BRC in-process regression suites pass on the new default (the qwen3.7-max qwen3.7-max BRC agents end their agentic loop early (success=True) without reaching CONSENSUS_CONFIRMED → consensus-wrapper restart churn #2906 k3s spike that previously gated this flip was removed — see the slice goal; Claude-route E2E is blocked on ScriptedProvider pod injection, deferred to Rewrite TestCredentialIsolation as k3s-native (currently skipped under k3s) #2585).
  • Acceptance criteria: build_consensus_wrapped_command with unset env emits the new template; with explicit EGG_BRC_EVENT_PUMP=false emits the old template (the one-release rollback path is preserved); existing snapshot tests updated to reflect the new default; BRC integration suite passes on the new default; rollback plan documented in PR body (git revert slices 1–3 if production traffic shows regression).
• task-4-2: Delete the old capped-restart bash template, the _RECOVERY_SYSTEM_PROMPT (consensus_wrapper.py:64-99), the SSE consensus.reached machinery (consensus_wrapper.py:418-449), and the MAX_CONSENSUS_RESTARTS constant + use-sites. Keep is_buffer_overflow / is_transient_crash / is_startup_failure classifiers — they're still valid signals under the new idle/no-progress safety budget. Delete the agent-side wait_loop heartbeat path from sandbox/egg_agent_tools/handlers/message.py:267-429 — the wrapper now owns heartbeating (TASK-2-2). Same for the gateway-session keep-alive in the same region (TASK-2-4 migrated; this task deletes the agent-side path).
  • Acceptance criteria: orchestrator/consensus_wrapper.py no longer contains MAX_CONSENSUS_RESTARTS, _RECOVERY_SYSTEM_PROMPT, or SSE / consensus.reached strings; rg 'consensus\.reached|sse_url|_RECOVERY_SYSTEM_PROMPT|MAX_CONSENSUS_RESTARTS' orchestrator/consensus_wrapper.py returns zero matches (defensive grep assertion against partial deletion); handlers/message.py no longer emits heartbeats or refreshes the gateway session; the three crash classifiers remain; relevant tests in orchestrator/tests/test_consensus_wrapper.py updated (or deleted, where old-path-specific tests no longer apply) — replacement coverage lands in TASK-4-3.
• task-4-3: Update tests in orchestrator/tests/test_consensus_wrapper.py and tests/sandbox/egg_agent_tools/test_handlers_message.py to reflect the deletions in TASK-4-2. Delete tests of the retired capped-restart cap, recovery prompt, SSE path, and agent-side heartbeat / keep-alive. Add coverage for the new wrapper behaviour where it replaces the old (the idle-budget test from slice-2 becomes the canonical liveness coverage).
  • Acceptance criteria: All retired tests removed; remaining tests pass under make test; coverage report does not regress for the consensus_wrapper module (replacement tests cover the equivalent semantics).
• task-4-4: Documenter: rewrite docs/architecture/orchestrator.md consensus-wrapper section to describe the post-deletion steady state — event-pump as the only path, idle budget replaces restart cap, wrapper owns heartbeat + keep-alive. Remove the "flag-off legacy path" caveats added in slice-2. Document the rollback plan (revert slices 1–3) for completeness. Cross-link to docs/architecture/brc-memory.md from slice-1.
  • Acceptance criteria: Doc reads as if the event pump has always been the only model; legacy-path caveats removed; cross-links present; rollback plan documented; rendering clean.

Test Plan

Per-slice automated coverage:

• slice-1: unit tests for next-action derivation (producer /
  reviewer / dual-role / open-NACK barrier Producer can re-propose after only one reviewer NACK in multi-reviewer BRC, wasting review cycles #2142 /
  conditional ACK / stale-version Producer agent calls 'egg-orch message wait-loop --for CONSENSUS_CONFIRMED'; validator rejects #2482), memory-write
  side-effects on brc_ack/brc_nack covering all six
  required fields (incl. last_reviewed_commit_sha per
  producer); atomic-write contract test; fail-closed path
  constructor test (raise on unset EGG_AGENT_ROLE); CLI
  round-trip with lifecycle-secret auth.
• slice-2: wrapper template snapshot for event-pump branch;
  wrapper-side heartbeat unit test asserting slice_id
  propagation from EGG_SLICE_ID; idle-budget overseer-alert
  threshold test; in-process PeerConsensusTracker regression
  (integration_tests/regression/test_brc_*.py) with flag off
  establishes zero orchestrator-side regression; E2E deferred
  to slice-4 via egg_stack (no in-process test double can
  drive a deployed pod end-to-end per Expand integration test coverage #2474).
• slice-3: compose_event_prompt unit tests; collapsed
  preamble snapshot; full git-log delta in per-event prompt
  asserted; per-event prompt envelope (excluding delta) ≤ 10 KB;
  sandbox-image rebuild verification (new mission.md reachable
  in pod) BEFORE slice-4 flag flip.
• slice-4: no new end-to-end test — the qwen3.7-max qwen3.7-max BRC agents end their agentic loop early (success=True) without reaching CONSENSUS_CONFIRMED → consensus-wrapper restart churn #2906 k3s
  spike was removed (qwen route unavailable in k3s; Claude-route
  E2E blocked on ScriptedProvider pod injection, deferred to
  Rewrite TestCredentialIsolation as k3s-native (currently skipped under k3s) #2585). The flag flip is gated on the slice-2 / slice-3 unit +
  BRC in-process regression suites passing on the new default;
  the deletions in TASK-4-2 are covered by the updated unit tests
  in TASK-4-3.
• slice-5: stdin / --reason-file / --summary-file /
  --files-reviewed-file round-trip tests for prose containing
  $VAR / backticks / ; / && / embedded newlines (docs: steer agents to the structured contract tool, not Bash-composed egg-contract #2741
  regression guard); CLI subcommand tests for brc resolve-obligation and brc read-peer-artifact; argv
  --reason deprecation-warning test.
• slice-6: BRC actions reachable via CLI with no agent MCP
  server registered; migrated E2E runs first action as
  consensus ack/nack via stdin/file (preserves SDK-spawn
  exercise); per-event wall-clock latency within 5% of
  pre-slice-6 baseline.

Manual verification:

• slice-3 pre-merge: sandbox image rebuilt + agent pod restarted
  so the new mission.md is reachable in the pod BEFORE slice-4
  flag flip.
• slice-4 pre-flag-flip: human inspects spike output
  (brc-memory content for reasoning fidelity;
  last_reviewed_commit_sha updated per producer; cost-per-phase
  delta vs restart-churn baseline) and
  consents to default-on flip via PR review.
• slice-6 pre-merge: human verifies no in-flight pipelines are
  mid-run against pre-slice agents; gate deploy on drain or
  cancel.

Manual Steps

Pre-merge:

• slice-3: sandbox image rebuilt + agent pod restarted BEFORE
  slice-4's flag flip (the new mission.md is the slice-4
  assumption; if pods are still running the old image when the
  flag flips, the event-pump path will reference STAY-ALIVE
  semantics that have been deleted from the preamble).
• slice-4: human review of spike output (memory content,
  last_reviewed_commit_sha correctness, cost delta)
  before the EGG_BRC_EVENT_PUMP default flips to true.
• slice-6: confirmation that no
  in-flight pipelines exist before deploy (MCP tools are deleted
  so an old wrapper that still injects them starts with no MCP
  server registered).

Post-merge:

• slice-4: monitor 24 h of production BRC traffic for any
  "Agent exited without BRC consensus" entries; fall back via
  EGG_BRC_EVENT_PUMP=false deployment env if seen. Rollback
  path for full spike falsification: git revert slices 1–3 (no
  production traffic touched the new path because the flag
  stayed off until slice-4).
• slice-6: monitor latency dashboards 24 h for per-event
  wall-clock regression beyond the 5% budget.

Stack

• Position: slice 4 of 6 in pipeline issue-2908-impl2
• Stacked on top of egg/issue-2908-impl2/slice-3

Slice slice-4 of pipeline issue-2908-impl2. Stacked on top of egg/issue-2908-impl2/slice-3.

[james-in-a-box]

Conflict Resolution Summary

Resolved merge conflicts with egg/issue-2908-impl2/slice-3:

File	Category	Resolution
docs/architecture/orchestrator.md	Semantic	Kept slice-4's post-deletion narrative: "event-pump wrapper is now the only consensus-wrapper path". Slice-3's text still described both wrappers ("EGG_BRC_EVENT_PUMP selects the wrapper, not the preamble"); that framing is the pre-slice-4 state and becomes obsolete the moment this PR lands per task-4-4. Folded slice-3's _build_brc_preamble-as-anchor cue into the kept paragraph.
docs/reference/agent-wait-patterns.md	Semantic	Same pattern as above — kept HEAD's post-deletion narrative. Also dropped slice-3's "Operator telemetry note (slice-3 default-off)" callout in §10.9.5: it documents the legacy-wrapper restart-rate behaviour under EGG_BRC_EVENT_PUMP=false, which the task-4-2 deletion of MAX_CONSENSUS_RESTARTS / _RECOVERY_SYSTEM_PROMPT makes inapplicable. Folded slice-3's function-name-as-anchor cue into the kept paragraph.
orchestrator/consensus_wrapper.py	Additive (semantic-adjacent)	Both sides edited the invoke_agent_for_event env prefix. Combined them: kept slice-3's EGG_REPO_PATH="${EGG_REPO_PATH:-$PWD}" re-export (reviewer_holistic v2 #2 — propagate orchestrator-set value so the script doesn't have to fall back to os.getcwd()) AND kept slice-4 task-4-1's EGG_BRC_MEMORY="${EGG_BRC_MEMORY:-full}" default flip. Comment text was already the slice-3 version on the surrounding lines (auto-merged), which describes both changes.

The remaining auto-merged files (orchestrator/routes/event_prompt.py, orchestrator/tests/test_compose_event_prompt.py, orchestrator/tests/test_consensus_wrapper.py) had no resolved conflict — git auto-merged because the slice-3 and slice-4 edits hit disjoint regions.

Please review:

• The orchestrator.md and agent-wait-patterns.md resolutions deleted slice-3 framing that described the legacy capped-restart wrapper as still alive. That framing is correct only until slice-4 lands; deleting it is the same change task-4-4 already made in this PR (the "rewrite consensus-wrapper section to describe the post-deletion steady state" acceptance criterion). If a reviewer wants the legacy framing preserved as a historical note, the rollback paragraph in orchestrator.md §"Slice-aware restart_phase hardening" already documents that history via the "revert slices 1–3" rollback plan.
• The consensus_wrapper.py resolution is a pure superset of both sides' intent. No call site outside this PR depends on the new EGG_REPO_PATH re-export (it's a defense-in-depth value-pass, not a contract change).

Local verification: ruff check orchestrator/ clean; pytest orchestrator/tests/test_consensus_wrapper.py orchestrator/tests/test_compose_event_prompt.py orchestrator/tests/test_brc_preamble_collapsed.py tests/sandbox/egg_agent_tools/test_handlers_message.py → 199 passed.

— Authored by egg

[james-in-a-box]

Autofixer report — Unit Tests on run 26862885451

Run 26862885451 had 18 unit-test failures, all in orchestrator/tests/. They split into two groups; Group A is fixed by commit 85660f47 on this branch, Group B needs a human decision because it traces to a merge regression rather than a slice-4 code-change.

Group A — 6 failures fixed by 85660f47

These are call sites in two test files that slice-4 task-4-3 ("delete tests of the retired capped-restart cap") missed because they referenced string assertions and kwargs rather than the function/class names listed in the task scope.

Test	Symptom
test_concurrent_integration.py::TestSpawnUsesConsensusWrapper::test_spawn_agent_uses_wrapped_command	asserted "RESTART_COUNT" + "BRC Consensus Recovery" in the wrapper script
test_concurrent_integration.py::TestNoImplicitReadyOnCleanExit::test_wrapper_contains_restart_logic	asserted "Restarting" + "RESTART_COUNT"
test_consensus_race_on_exit.py::TestWrapperStaleTrackerFallback::* (4 tests)	called build_consensus_wrapped_command(..., max_restarts=2, startup_failure_window_seconds=0) — both kwargs deleted by slice-4 task-4-2

The fix migrates the first two off the deleted strings onto event-pump markers (event-pump, egg-orch brc get-state, egg-orch brc next-action) and deletes TestWrapperStaleTrackerFallback entirely (plus its now-orphan os / shlex / subprocess / sys / tempfile imports). The stale-tracker fallback that class guarded is moot under the event-pump template — the wrapper reads BRC state directly each loop iteration, so there is no wrapper-side tracker to go stale.

Group B — 12 failures NOT fixed; needs operator decision

These tests all reference production code that exists on origin/main but is missing from this branch's orchestrator/routes/pipelines.py:

Tests	Missing production symbol	Origin
test_slice_4_restart_hardening.py::TestSliceAgentsAlive::* (7)	_slice_agents_alive(...) function + LABEL_SLICE_ID import	3398005b (#2916)
test_slice_4_restart_hardening.py::TestResolveSliceBaseBranch*::* (4)	parent_branch_exists kwarg on _resolve_slice_base_branch	b8573924 (#2928)
test_short_flow_contract_population.py::test_populate_contract_from_plan_preserves_slice_and_task_runtime (1)	runtime preservation merge in _populate_contract_from_plan	66459220 (#2923)

Root cause — git ancestry shows commit 06c5a6cb ("merge slice-4 base into coder branch") brought slice-3 base in but the merge resolution dropped the main-side fixes from #2914/#2928/#2923. pipelines.py blob hash transitioned efd93ec8 (had the fixes) → 7e003565 (lost them). The subsequent fd1a8b66 restored _auto_populate_contract_at_implement_start but did not restore the other three symbols.

Recovery options (operator pick):

1. Merge origin/main into egg/issue-2908-impl2/slice-4. Structurally correct — main has every missing fix. Risk: large diff balloon (≈357 insertions / 705 deletions in pipelines.py alone) plus likely conflicts to resolve, and the diff will leak into the slice-5/6 stack on rebase.
2. Surgical cherry-pick of 3398005b, b8573924, 66459220 onto this branch. Picks up just the missing symbols without main's full delta — but the three commits may themselves depend on each other / earlier main commits, so this can snowball.
3. Delete the 12 failing tests on this branch. Cheapest, but loses the regression detection — these tests guard behaviour that main does implement, so the deletions would have to be reverted as soon as main is merged back in.

Recommendation: (1) if the slice-4/5/6 stack will be rebased onto main before merge anyway (the rebase carries the same conflicts); (2) if you want a minimal slice-4 diff and plan to do a full main merge in slice-6.

— Authored by egg

[james-in-a-box]

Autofix tracking

{"Test/Unit Tests": 4}

[james-in-a-box]

Response to review feedback

Restored the cluster of bad-merge collateral-damage in pipelines.py and test_pipeline_prompts.py per egg-reviewer's analysis. Strategy: take pipelines.py and test_pipeline_prompts.py from origin/egg/issue-2908-impl2/slice-3 (which has all the post-#2936 phrasing the merge dropped), then layer the 6 audit-log attribution restorations from origin/main directly because slice-3 forked before those #2919 hunks landed.

Disposition per blocking item

1. gateway: allow orchestrator role to execute read-only gh pr list #2893 / orchestrator: flip git-route call sites in pipelines.py from agent_role="coder" to "orchestrator" #2919 audit-log attribution at 6 reconciler hops — fixed-in-PR (commit eb8c6442). Restored agent_role="orchestrator" plus the explanatory comment blocks at all six call sites: _start_stacked_pr_reconciler._list_extant_branches, _start_stacked_pr_reconciler._rebase_onto, two _run_implement_phase_slices.is_slice_branch_merged_into_parent calls (bootstrap + spawn), and _run_implement_phase_slices.create_slice_integration_branch. The list_open_prs site (slice-3 already had it correct) is unchanged. rg 'agent_role="coder"' orchestrator/routes/pipelines.py now returns zero matches.

2. Let the coder author its own tests; tester reviews-and-hardens #2936 coder-owns-tests phrasing in 5 functions — fixed-in-PR (commit eb8c6442). Restored from slice-3: _ROLE_DESCRIPTIONS tester + coder entries, the tester block in _build_reviewer_preparation, the tester banner in _build_producer_orientation, the tester branch in _build_agent_prompt, and the coder→tester HANDOFF body. The dual-role banner header continues to render (#2749) consistently across the rendered text and the surrounding comment, so the banner-vs-surface contradiction the reviewer described is gone.

3. Bad-merge incident only partially repaired — fixed-in-PR (commit eb8c6442). This is the umbrella finding; items 1 and 2 together discharge it. git diff origin/main -- orchestrator/routes/pipelines.py orchestrator/tests/test_pipeline_prompts.py now shows only slice-3-relevant hunks (compose_event_prompt re-export, _brc_preconfirm_wait_line/_brc_stay_alive_wait_line deletion, _build_brc_preamble collapse) plus their test counterparts.

4. test_pipeline_prompts.py internal contradictions + dead docstring reference — fixed-in-PR (commit eb8c6442). Restored test_tester_prep_waits_for_coder_before_writing_tests (post-Let the coder author its own tests; tester reviews-and-hardens #2936 form: "do not write test" in lower + "consensus_propose" in lower) and test_tester_orientation_directs_review_and_harden_after_propose (post-Let the coder author its own tests; tester reviews-and-hardens #2936 form: "scaffold" not in lower + "do not write test files before" in lower). The test_dual_role_banner_does_not_contradict_coder_owns_tests docstring now correctly refers to the sibling test that actually exists. All three tests pass locally (pytest orchestrator/tests/test_pipeline_prompts.py::TestReviewerPreparation::test_tester_prep_waits_for_coder_before_writing_tests …).

5. Scope creep vs PR description — fixed-in-PR (commit eb8c6442). With items 1, 2, and 4 restored, the pipelines.py and test_pipeline_prompts.py diff is no longer outside slice-4's stated scope: the remaining cross-file hunks are all from slice-3 (the parent of this stack, already reviewed), and slice-4's contribution stays inside consensus_wrapper.py, handlers/message.py, the relevant test files, and the docs.

Disposition per non-blocking item

• derived_parent inline-string refactor at :10546 — fixed-in-PR (commit eb8c6442). Reverted to the derived_parent variable, matching slice-3 and main.
• is_buffer_overflow / is_transient_crash / is_startup_failure classifiers kept — disagree (intentional; covered by task-4-2 plan). The plan explicitly says "Keep is_buffer_overflow / is_transient_crash / is_startup_failure classifiers — they're still valid signals under the new idle/no-progress safety budget." Reviewer flagged this only for future-reader visibility, not as a change request.
• Deletion of TestMessageWaitLoopHeartbeat / TestWrapperStaleTrackerFallback / TestConsensusWrapperNackFeedback / test_consensus_wrapper_anchor.py — disagree (no change requested). Reviewer noted the deletions are clean with good replacement-coverage comments.
• TestEventPumpIdleBudgetCeiling new class — disagree (no change requested). Reviewer noted it "looks right for the post-deletion world."
• test_concurrent_integration.py::test_wrapper_drives_event_pump_loop — disagree (no change requested). Reviewer noted it correctly preserves the "must not auto-signal READY" invariant.
• docs/architecture/orchestrator.md rollback plan — disagree (no change requested). Reviewer noted it's "the right shape for an operator-facing escape hatch."
• brc_memory.py fail-safe-to-off asymmetry — disagree (no change requested). Reviewer noted "the right asymmetry, and the inline comment captures the reasoning. Clean."

Verification

• ruff check orchestrator/routes/pipelines.py orchestrator/tests/test_pipeline_prompts.py → All checks passed.
• ruff format --check on both files → already formatted.
• The three restored / preserved coder-owns-tests tests pass:
  • test_tester_prep_waits_for_coder_before_writing_tests
  • test_tester_orientation_directs_review_and_harden_after_propose
  • test_dual_role_banner_does_not_contradict_coder_owns_tests
• grep 'agent_role="coder"' orchestrator/routes/pipelines.py → zero matches.
• git diff origin/main -- orchestrator/routes/pipelines.py no longer shows the audit-log-attribution or coder-owns-tests hunks; only the slice-3-relevant changes (re-export, deletion, preamble collapse) remain.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns in the delta since b2380bb1.

The new commits are corrections, not new design:

• 2d8fa0f4 — merge from slice-3 resolving the dual-role banner conflict
• eb8c6442 — restoration of accidentally-dropped tester role descriptions (coder-owns-tests framing) and audit-log attribution (agent_role="orchestrator" for orchestrator-driven gateway calls)
• 70de132b — test alignment for the restored attribution

The banner rephrasing actually reduces prompt-level micromanagement: the scaffold-first directive (which previously instructed the tester to draft test scaffolds from tasks[].files while waiting on the coder) is replaced with "orient only; review-and-harden after the coder proposes." That's a small move toward agent-mode alignment, not against it. The dual-role execution-order banner remains verbose because it has to coordinate a genuinely dual-role agent's lifecycle ordering, but it delegates the per-event body to the existing Reviewer Lifecycle steps (3 → 4 → 5) rather than re-explaining them — which is the right factoring.

The agent_role="orchestrator" changes in _start_stacked_pr_reconciler and _run_implement_phase_slices are about audit-log accuracy for synthetic-session gateway calls, not agent design. No prompt surface or LLM-call shape changes here.

— Authored by egg

[egg-reviewer]

Re-review of PR #2951 (slice-4/6)

Re-reviewed against previous review at 2d8fa0f4. Two follow-up commits since:

• eb8c6442 — "Restore collateral-damage reverts in pipelines.py from slice-3 + audit-log attribution from main"
• 70de132b — "Fix checks: align rebase_onto agent_role test with restored orchestrator attribution"

Blocking issues from previous review — all resolved

1. Audit-log attribution (#2893 / #2919) restored — All 9 agent_role="orchestrator" call-sites in _start_stacked_pr_reconciler / _run_implement_phase_slices now match origin/main verbatim (sites at lines 15624, 15627, 15640, 15650, 15682, 15972, 16360, 16424, 16685). The synthetic-session gateway calls correctly identify the orchestrator as the caller instead of impersonating a coder.

2. Coder-owns-tests framing (#2936) restored — All 5 affected surfaces are back to post-#2936 form:

   • _ROLE_DESCRIPTIONS["coder"] / ["tester"] — coder "implements code changes AND authors their tests"; tester "reviews-and-hardens the coder's tests after the coder proposes"
   • _build_reviewer_preparation tester block — "Do NOT write test files while waiting"
   • _build_producer_orientation tester block — "Orient only until the coder proposes... do NOT write test files before the coder's CONSENSUS_PROPOSE"
   • _build_agent_prompt tester branch — "The coder authors its own tests; your job is to review-and-harden them"
   • HANDOFF body — "auth.py + tests/test_auth.py are in; review and harden the tests"

3. Bad-base-merge incident fully repaired — _resolve_slice_base_branch uses the derived_parent variable again at line 10543.

4. Contradictory tests fixed —

   • test_tester_gets_test_scaffolding → replaced by test_tester_prep_waits_for_coder_before_writing_tests (line 3783) asserting "do not write test" + "consensus_propose"
   • test_tester_orientation_directs_scaffold_first → replaced by test_tester_orientation_directs_review_and_harden_after_propose (line 3851) asserting "harden", "consensus_propose", absence of "scaffold" and "tasks[].files"
   • The docstring reference in test_dual_role_banner_does_not_contradict_coder_owns_tests now resolves to an actually-existing sibling test

5. Scope no longer creeps — git diff origin/main..HEAD -- orchestrator/routes/pipelines.py now shows only slice-4-relevant deltas (the event_prompt import addition, the deletion of _brc_preconfirm_wait_line / _brc_stay_alive_wait_line, the _build_brc_preamble rewrite for the event-pump model, the event-handler-contract closing block, and the slice-4 flag flips). No more collateral reverts of unrelated main commits.

Targeted tests verified passing locally

• TestDualRoleExecutionOrdering — 11/11 passed
• test_tester_prep_waits_for_coder_before_writing_tests — passed
• test_tester_orientation_directs_review_and_harden_after_propose — passed

Non-blocking observation (suggestion)

The comment block at orchestrator/routes/pipelines.py:12321-12348 still describes the pre-slice-4 wait-loop mechanics — e.g. "the pre-PROPOSE wait-loop in step 1 of the banner below catches the coder's first CONSENSUS_PROPOSE". The banner body it precedes no longer contains a wait-loop step; the event-pump wrapper now drives re-invocation. This is documentation drift that doesn't affect runtime behaviour, but a follow-up pass to update the comment to match the event-pump model would prevent future readers from being misled. Safe to address in a subsequent slice or a small follow-up commit on this branch.

Recommendation

LGTM. All five blocking issues from the previous review have been addressed cleanly, the scope is now properly bounded to slice-4, and the relevant tests pass.

— Authored by egg

[james-in-a-box]

Response to review feedback

Addressing the latest re-review (70de132b) from egg-reviewer, which marked all five previously-blocking issues as resolved and left one non-blocking observation.

Disposition per item

1. Comment block at pipelines.py:12330-12348 still describes pre-slice-4 wait-loop mechanics — fixed-in-PR (commit 3a300e7). The reviewer noted: "the pre-PROPOSE wait-loop in step 1 of the banner below catches the coder's first CONSENSUS_PROPOSE — the banner body it precedes no longer contains a wait-loop step; the event-pump wrapper now drives re-invocation."

Rewrote the comment to match the rendered banner the event-pump path emits:

• The tester orients up-front, exits after ORIENT, and is re-invoked by the event-pump wrapper when the coder proposes (rather than blocking on a pre-PROPOSE wait-loop).
• The two reviewer rendezvous points (the coder's first CONSENSUS_PROPOSE and subsequent re-proposes / peer-producer proposals) are described as fresh wrapper invocations, not wait-loop wakes.
• The (#2749) reference is preserved; the f4c7d780 / 8b81ed32 self-block context is preserved verbatim because that history remains the load-bearing reason the banner needs to coordinate the ordering at all.

The reviewer framed this as "safe to address in a subsequent slice or a small follow-up commit on this branch" — the small follow-up commit on this branch is the cheaper option, so I took it in-PR rather than deferring (a "non-blocking" tag isn't an explicit defer request).

Verification

• Pure comment change — no runtime behaviour delta. ruff check orchestrator/routes/pipelines.py clean; ruff format --check clean.
• rg 'pre-PROPOSE wait-loop|wait-loop in step 1' . → zero matches (no stale references elsewhere).
• All five blocking items from the prior review remain resolved (no regressions in the commit).

— Authored by egg

[egg-reviewer]

No agent-mode design concerns in the delta since 70de132b.

The single new commit (3a300e7e) is a comment-only update to the dual-role
banner block in _build_brc_preamble. The rewrite reinforces the slice-4
event-pump posture: the tester is now described as "orients, exits, and is
re-invoked by the event-pump wrapper" rather than as holding an in-process
pre-PROPOSE wait-loop. Both reviewer rendezvous points are reframed as fresh
wrapper invocations rather than wait-loop wakes — exactly the model-portable,
one-shot-per-event shape this slice exists to land.

No new pre-fetching, structured-output-for-humans, post-processing,
prompt-level security, direct-LLM-API, or hardcoded-model issues introduced.

— Authored by egg

[egg-reviewer]

Re-review of PR #2951 (slice-4/6) at 3a300e7e

Re-reviewed against my previous review at 70de132b (which was a --comment LGTM with one non-blocking suggestion). Delta since: a single commit 3a300e7e touching only the comment block at orchestrator/routes/pipelines.py:12330-12348. Net: +13 / −12 lines, no test or doc changes.

Previous non-blocking suggestion — resolved

The comment block preceding the dual-role banner in _build_brc_preamble used to describe 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"). After this commit it reads:

...it orients up-front, exits after ORIENT, and is re-invoked by the event-pump wrapper when the coder proposes, at which point it hardens + proposes + ACK/NACKs in one pass... The tester therefore has TWO reviewer rendezvous points, both surfaced as fresh wrapper invocations under the event-pump model: (a) the coder's first CONSENSUS_PROPOSE re-invokes the tester so it has something to harden; (b) subsequent re-proposes and peer-producer proposals... likewise re-invoke the tester...

This is consistent with the rendered banner that follows:

• Banner step 1: "the event-pump wrapper invokes you again when the upstream producer's CONSENSUS_PROPOSE arrives, at which point you handle the review AND (if your WORK was gated on it) start producing."
• Banner step 2: "On an upstream producer's PROPOSE, the wrapper re-invokes you with the proposal in your event payload."
• Banner step 3: "Subsequent re-proposes... and CONSENSUS_RE_REVIEW events surface as new wrapper invocations."

Comment and banner are now telling the same story.

Verified no stale references remain

rg 'pre-PROPOSE wait-loop|wait-loop in step' in source returns zero matches. The lone remaining reviewer wait-loop mention at :12339 is the historical self-block citation (f4c7d780 / 8b81ed32), which is correctly preserved verbatim because that history is the load-bearing reason the dual-role banner needs to coordinate ordering at all.

Other blocking items from earlier reviews — still resolved

All five blocking items from the 2d8fa0f4 review (audit-log attribution #2893/#2919, coder-owns-tests framing #2936 across 5 surfaces, bad-base-merge _resolve_slice_base_branch repair, contradictory test renames, scope creep) remain fixed at this commit — no regressions introduced by the comment-only delta.

Recommendation

LGTM. Pure documentation alignment with the event-pump model; no runtime behaviour delta; correctly addresses the prior non-blocking observation.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Response to review feedback

Re-read all authorized feedback (egg-reviewer formal reviews + line-level comments + jwbron / james-in-a-box review-process comments) at HEAD 3a300e7e.

Latest review state

The two most recent reviews from egg-reviewer[bot] at 3a300e7e are both LGTM:

• agent-mode-design (APPROVED) — "No agent-mode design concerns in the delta since 70de132b. ... The rewrite reinforces the slice-4 event-pump posture ... No new pre-fetching, structured-output-for-humans, post-processing, prompt-level security, direct-LLM-API, or hardcoded-model issues introduced."
• review (COMMENTED → LGTM) — "Pure documentation alignment with the event-pump model; no runtime behaviour delta; correctly addresses the prior non-blocking observation. ... All five blocking items from the 2d8fa0f4 review (audit-log attribution gateway: allow orchestrator role to execute read-only gh pr list #2893/orchestrator: flip git-route call sites in pipelines.py from agent_role="coder" to "orchestrator" #2919, coder-owns-tests framing Let the coder author its own tests; tester reviews-and-hardens #2936 across 5 surfaces, bad-base-merge _resolve_slice_base_branch repair, contradictory test renames, scope creep) remain fixed at this commit — no regressions introduced by the comment-only delta."

No line-level review comments remain (gh api repos/jwbron/egg/pulls/2951/comments filtered to authorized users returns []).

Disposition per item

All blocking items from prior review rounds

1. Audit-log attribution (gateway: allow orchestrator role to execute read-only gh pr list #2893 / orchestrator: flip git-route call sites in pipelines.py from agent_role="coder" to "orchestrator" #2919) at 6 reconciler hops — fixed-in-PR (commit eb8c6442). Restored agent_role="orchestrator" plus explanatory comment blocks at all six sites. Confirmed unchanged at 3a300e7e.
2. Coder-owns-tests framing (Let the coder author its own tests; tester reviews-and-hardens #2936) in 5 surfaces — fixed-in-PR (commit eb8c6442). _ROLE_DESCRIPTIONS, _build_reviewer_preparation, _build_producer_orientation, _build_agent_prompt, HANDOFF body all restored to post-Let the coder author its own tests; tester reviews-and-hardens #2936 form. Confirmed unchanged at 3a300e7e.
3. Bad-merge collateral damage fully repaired — fixed-in-PR (commit eb8c6442). derived_parent inline-string refactor at :10546 reverted; the git diff origin/main -- orchestrator/routes/pipelines.py is now constrained to slice-4-relevant deltas.
4. test_pipeline_prompts.py contradictions + dead docstring — fixed-in-PR (commit eb8c6442). test_tester_prep_waits_for_coder_before_writing_tests and test_tester_orientation_directs_review_and_harden_after_propose restored to post-Let the coder author its own tests; tester reviews-and-hardens #2936 form; sibling-test reference in test_dual_role_banner_does_not_contradict_coder_owns_tests now resolves.
5. Scope creep vs PR description — fixed-in-PR (commit eb8c6442). With items 1–4 restored, the cross-file diff is bounded to slice-4 (event-pump wrapper, agent-side heartbeat deletion, flag flips, _brc_preconfirm_wait_line / _brc_stay_alive_wait_line deletion, _build_brc_preamble collapse, doc updates).

Non-blocking suggestion from the 70de132b review

• Comment block at pipelines.py:12330-12348 describing pre-slice-4 wait-loop mechanics — fixed-in-PR (commit 3a300e7e). Rewrote the comment to describe the event-pump re-invocation model (tester orients up-front, exits after ORIENT, re-invoked by the wrapper on coder PROPOSE). Reviewer confirmed resolved at 3a300e7e: "Comment and banner are now telling the same story... rg 'pre-PROPOSE wait-loop|wait-loop in step' in source returns zero matches."

Non-blocking observations earlier flagged but not requested as changes

• is_buffer_overflow / is_transient_crash / is_startup_failure classifiers kept — disagree (intentional per task-4-2 plan; reviewer flagged only for future-reader visibility, not as a change request).
• Deletion of TestMessageWaitLoopHeartbeat / TestWrapperStaleTrackerFallback / TestConsensusWrapperNackFeedback / test_consensus_wrapper_anchor.py — disagree (no change requested; reviewer noted "Clean" with good replacement-coverage comments).
• TestEventPumpIdleBudgetCeiling new class — disagree (no change requested; reviewer noted "looks right for the post-deletion world").
• test_concurrent_integration.py::test_wrapper_drives_event_pump_loop — disagree (no change requested; reviewer noted it correctly preserves the "must not auto-signal READY" invariant).
• docs/architecture/orchestrator.md rollback plan — disagree (no change requested; reviewer noted "the right shape for an operator-facing escape hatch").
• brc_memory.py fail-safe-to-off asymmetry — disagree (no change requested; reviewer noted "the right asymmetry, and the inline comment captures the reasoning. Clean").

Verification

No new actionable feedback to address at 3a300e7e. Both latest reviews are LGTM. No code changes pushed in this round. CI will run on HEAD; the prior round's make lint was clean and the targeted tests (TestDualRoleExecutionOrdering 11/11, test_tester_prep_waits_for_coder_before_writing_tests, test_tester_orientation_directs_review_and_harden_after_propose) were verified passing.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

24 previous review(s) hidden.

[egg-reviewer]

Re-review of PR #2951 at 136b9ad5

Delta since my LGTM at 3a300e7e: a single merge commit 136b9ad5 ("Merge remote-tracking branch 'origin/main' into egg/issue-2908-impl2/slice-4") that brings main into the PR branch and resolves conflicts in orchestrator/routes/pipelines.py and orchestrator/tests/test_slice_run_loop_integration.py. No new PR-authored commits.

Per GitHub's compare (3a300e7e...136b9ad5), the only PR-side contribution to pipelines.py since the prior review is the same +13 / −12 comment-block update I already approved (event-pump rewrite of the dual-role banner preamble); everything else in the delta is content arriving from origin/main, which is out of scope for this PR's design review.

Agent-mode design assessment

No new agent-mode concerns in this delta:

• No new pre-fetching — the merge does not introduce baked-in diffs or file contents into prompts.
• No structured-output-for-humans — no new JSON-shaped outputs targeting PR comments or reviews.
• No post-processing pipelines — no new agent-output-parsing scripts.
• No rigid step-by-step procedures added — the event-pump posture from slice-4 is preserved.
• No prompt-level security constraints — sandbox/gateway remain the enforcement boundary.
• No direct Anthropic API calls from orchestrator/gateway/shared.
• No Agent-SDK bypass introduced.
• No hardcoded model identifiers introduced.

The five previously-blocking items from the 2d8fa0f4 round (audit-log attribution #2893/#2919, coder-owns-tests framing #2936, bad-base-merge _resolve_slice_base_branch repair, contradictory test renames, scope creep) all remain resolved at 136b9ad5 — no regressions reintroduced by the merge resolution.

Recommendation

LGTM. Merge-only delta; the slice-4 event-pump posture and all prior fixes are preserved. PR is already in MERGED state; review is confirmatory.

— Authored by egg

[james-in-a-box]

egg agent-mode-design completed. View run logs

24 previous review(s) hidden.

[egg-reviewer]

Re-review of PR #2951 at 136b9ad5

PR is in MERGED state; this is a post-merge confirmatory re-review.

Delta since my prior LGTM at 3a300e7e: a single merge commit 136b9ad5 ("Merge remote-tracking branch 'origin/main' into egg/issue-2908-impl2/slice-4") resolving conflicts in orchestrator/routes/pipelines.py and orchestrator/tests/test_slice_run_loop_integration.py. No new PR-authored production code.

Conflict resolution audit

I verified each conflict resolution against both sides of the merge (slice-4 head 3a300e7e and main 0b005b93) plus the shared base 7bdb29d8:

orchestrator/routes/pipelines.py

• The slice-4 PR-only contribution is unchanged at 136b9ad5: the same +13 / -12 rewording of the _build_brc_preamble comment block describing the dual-role tester's event-pump lifecycle (lines 12087–12106). I previously approved this hunk and confirm the merge did not regress it.
• Main's overlapping changes — removal of _handle_pr_creation_failure, _format_rescue_hint, _pr_metadata_from_plan_draft, _build_github_staging_manual_step; removal of the mode=gateway_mode parameter from the list_open_prs callsite at _open_context_pr_at_implement_start; removal of agent_role from the _list_open_prs closure inside _start_stacked_pr_reconciler (now routes through the launcher-authed control-plane endpoint per #2925); and updated guidance text in _build_role_restrictions_section / _build_file_boundary_section for .github-staging/ — are all applied cleanly. None of these conflict with slice-4's intent.
• Critically, the #2919 audit-log attribution flip from agent_role="coder" → agent_role="orchestrator" that slice-4 made (and that the prior CHANGES_REQUESTED review forced restored) is preserved at every site that survives the merge: lines 15407, 15439, 15729, 16117, 16188, 16449 in HEAD. The merge of main brought in companion changes that also attribute these calls to the orchestrator (or remove the agent_role parameter entirely where the call now uses launcher auth), so the resolution is internally consistent.

orchestrator/tests/test_slice_run_loop_integration.py

• Both slice-4 and main independently flipped the agent_role assertion in TestStartStackedPrReconciler from "coder" → "orchestrator". They differ only in the surrounding comment text. The merge resolution kept main's more detailed comment (rebase + force-push + pr-edit attribution + slice-integration exemption). The assertion itself is identical to slice-4's intent.

Five previously-blocking items remain resolved at HEAD

The cluster from the 2d8fa0f4 round (#2893 / #2919 audit-log attribution, #2936 coder-owns-tests framing, _resolve_slice_base_branch bad-base-merge repair, the contradictory test_tester_* renames, and scope creep in pipelines.py) all stay clean at 136b9ad5. No regressions reintroduced by the merge resolution.

No new findings

• No security concerns introduced by the merge.
• No correctness regressions: the conflict resolution preserves both branches' intent.
• No new tests required: both sides already had test coverage for the migrated agent_role attribution.
• The PR-vs-main delta (git diff origin/main..HEAD -- orchestrator/routes/pipelines.py) is now down to 23 lines (the comment-block update), which is the smallest the surface has been across the PR's lifetime — the scope-creep concern is fully resolved.

Recommendation

LGTM (confirmatory). Merge-only delta; the slice-4 event-pump posture, the prior round of regression repairs, and main's recent changes (control-plane list_open_prs, _handle_pr_creation_failure removal, .github-staging/ guidance update) are all coherently composed at HEAD.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

24 previous review(s) hidden.