Fix #2064: reject wait_loop on CONSENSUS_CONFIRMED for non-confirmed producers

[jwbron]

Summary

A producer's own consensus_confirmed is part of what generates the global CONSENSUS_CONFIRMED signal — waiting on it before the producer has reached CONFIRMED state is a circular dependency that deadlocks the pipeline until the overseer's heartbeat-stall band-aid eventually intervenes.

This adds a server-side guard in the orchestrator's /messages/wait endpoint that returns HTTP 400 with an actionable error when the caller is a producer in WORKING/PROPOSED state and CONSENSUS_CONFIRMED appears in for_types. The error tells the agent to call mcp__brc__confirm and, if it returns pending_acks, to wait on the prerequisite events instead (CONSENSUS_PROPOSE, CONSENSUS_ACK, CONSENSUS_RE_REVIEW) before retrying confirm.

The fix is structural — matching the project's preference for infrastructure constraints over prompt-based rules (docs/design/capability-removal.md) — and applies generically to every producer role (coder, tester, documenter, and any future producer), not just the documenter case that surfaced the bug.

What actually happened in pipeline issue-1965

Reading .egg-state/brc-history/1965-implement.md:

• 18:00:52 — documenter PROPOSES (version 1)
• 18:04:22 — reviewer_code ACKs documenter (only reviewer; ADVISORY edge); orchestrator emits a directed STATUS nudge "ready to confirm"
• 18:04:35 — documenter calls consensus confirmed → returns status='pending_acks' because global_zero_proposal fires (coder/tester haven't proposed yet)
• 18:04:56 — documenter enters wait_loop --for CONSENSUS_CONFIRMED,CONSENSUS_RE_REVIEW,OVERSEER_ALERT (the post-confirm STAY ALIVE pattern, invoked prematurely as if confirm had succeeded)
• documenter wakes briefly on each peer's CONSENSUS_CONFIRMED event but never re-attempts its own confirm
• 18:40:57 — agent-heartbeat-stall overseer alert finally fires (~3.5 min after the last other-agent confirm); ~36 min of total wedged time
• 18:41:03 — documenter confirms; consensus reached

The issue title's framing ("before calling their own confirm") is slightly off: documenter did call confirm — but it failed with pending_acks, and the agent had no event-driven recovery path from the post-confirm STAY ALIVE wait.

Why a server-side guard

I considered four shapes of fix:

1. Tighten the producer-lifecycle prompt — fragile; the prompt is correct in step ordering, the agent just couldn't recover from pending_acks.
2. Auto-retry inside the confirm handler — too much new state machinery.
3. Promote the existing check_brc_progress health check — it already detects "fully ACKed but not confirmed" and escalates after 180s, but didn't fire on this pipeline (presumably a state-condition mismatch worth investigating separately). Reactive, not preventative.
4. Server-side wait_loop guard — surgical; turns the deadlock from "silent for 36 minutes" into "loud 400 the moment the agent calls wait." Defense in depth that prevents the entire class of bug.

(4) matches docs/design/capability-removal.md's thesis that infrastructure-level constraints beat prompt-based rules. It catches every producer role, every reason pending_acks might fire (global zero-proposal, missing critical ACKs, stale ACKs, unresolved NACKs), and any future producer that gets added.

Behavior

GET /api/v1/pipelines/<pid>/messages/wait?for=CONSENSUS_CONFIRMED&role=documenter&...

Returns 400 when peer_consensus.get_peer_consensus_tracker(pid).is_producer_pending_confirm(role) is True (producer not yet in CONFIRMED). Error body:

Producer 'documenter' cannot wait on ['CONSENSUS_CONFIRMED'] before its own consensus_confirmed has succeeded — its own confirm is part of what generates that signal, so the wait would deadlock (#2064). Call mcp__brc__confirm first; if it returns status='pending_acks' (e.g. another producer hasn't proposed yet, or your reviewers haven't ACKed), wait on the prerequisite events instead — CONSENSUS_PROPOSE from missing producers, CONSENSUS_ACK from your reviewers, or CONSENSUS_RE_REVIEW — then retry confirm.

The sandbox message_wait_loop handler already maps 4xx (non-408) to permanent errors per sandbox/egg_agent_tools/handlers/message.py:319-323, surfaced as exit code 3 by the egg-orch CLI shim (per docs/reference/agent-wait-patterns.md §3 contract).

The guard short-circuits to allow when:

• no role query param (broadcast snapshots)
• no consensus tracker registered (non-BRC pipelines, post-orchestrator-restart before reconstruction)
• caller is reviewer-only (their producer phase doesn't exist; they may legitimately wait on global confirms)
• caller is a producer in CONFIRMED (the post-confirm STAY ALIVE pattern is exactly what this is for)
• for_types doesn't include CONSENSUS_CONFIRMED (other event types are unaffected)

Files

• orchestrator/peer_consensus.py — is_producer_pending_confirm(role) helper on PeerConsensusTracker
• orchestrator/routes/messages.py — guard wired into the wait endpoint
• orchestrator/tests/test_messages.py — 7 endpoint tests (documenter scenario, dual-role, reviewer-only, missing tracker, missing role, other for_types passthrough, confirmed producer passthrough)
• orchestrator/tests/test_peer_consensus_integration.py — 5 helper unit tests across the producer state machine
• docs/reference/agent-wait-patterns.md — anti-pattern Phase 3: Container extraction #5 documenting the deadlock and the recovery idiom

Test plan

• pytest orchestrator/tests/test_messages.py::TestProducerPendingConfirmGuard — 7/7 pass
• pytest orchestrator/tests/test_peer_consensus_integration.py::TestIsProducerPendingConfirm — 5/5 pass
• pytest orchestrator/tests/test_messages.py orchestrator/tests/test_peer_consensus_integration.py — 172/172 pass (no existing-test regressions)
• ruff check clean
• Live BRC pipeline run to confirm the error path surfaces sensibly to a real agent (CI / next pipeline)

Follow-ups (not in this PR)

• Investigate why check_brc_progress (orchestrator/health_monitor.py:731) didn't escalate at 180s — its escalation might be the right place to also emit a directed STATUS nudge to the producer (cleaner than the current heartbeat-stall band-aid).
• Confirm response shape: mcp__brc__confirm returns {ok: True, status: "pending_acks", ...} which is misleading. A future change could rename ok to accepted or surface pending_acks more prominently — orthogonal to this fix.

🤖 Generated with Claude Code

[egg-reviewer]

Reviewed end-to-end: the new server-side guard, the is_producer_pending_confirm helper, the route wiring, both test classes, and the docs change. I also walked the BRC state machine (peer_consensus.handle_propose / handle_ack / handle_nack / handle_confirmed) and the wait-loop client (sandbox/egg_agent_tools/handlers/message.py) to confirm the 400 → exit-code-3 contract. The fix is correct and well-targeted: it surfaces the deadlock as an actionable, immediate error instead of a 36-minute silent stall.

No blocking issues. A few non-blocking notes below.

Non-blocking

1. Misleading comment in test_peer_consensus_integration.py

orchestrator/tests/test_peer_consensus_integration.py:3071 reads:

# Reviewer must confirm before producers can confirm cleanly
result = t.handle_confirmed("reviewer_code")

That isn't true. check_confirm_guard (orchestrator/action_guards.py:283) gates a producer's confirm only on global_zero_proposal + producer_not_fully_acked — there is no dependency on reviewer confirms. The test happens to confirm the reviewer first but doesn't need to. Either drop the reviewer confirm (and the comment) or rewrite the comment to "Confirming reviewer first to keep the assertion order tidy" so future readers don't take it as a state-machine rule.

2. Dual-role tester is fixtured but never asserted on

The PR description lists "dual-role" as one of the seven endpoint test cases, and implement_tracker does include tester in the graph — but no test exercises role=tester. The dual-role behaviour is well-defined by the existing implementation (both producer and reviewer phases transition together inside the same handle_confirmed call), and the helper covers it implicitly via the producer-phase check. Still, a one-line test like test_dual_role_tester_in_proposed_blocked would lock the contract in for the tester-specific case and match what the PR description promises. Not a blocker.

3. Import-shim style drift inside routes/messages.py

The existing peer_consensus import in _apply_delphi_filter (orchestrator/routes/messages.py:319-322) is a one-tier try / except → None pattern. The new helper at L387-395 introduces a two-tier from peer_consensus … / from ..peer_consensus … fallback. Both work, but the inconsistency invites future drift. Suggest matching the established one-tier shape — the package-relative fallback is unnecessary in this file (every other peer_consensus import here is single-tier and this code only runs in-process where peer_consensus resolves at the top-level). Tightening this also drops the # pragma: no cover line.

4. Guard ignores from_role filter

_check_producer_pending_confirm_guard rejects any wait that includes CONSENSUS_CONFIRMED, regardless of whether the caller scoped it to a peer (e.g. from=reviewer_code). Today, no documented producer pattern waits on a peer's per-agent CONSENSUS_CONFIRMED while still in WORKING/PROPOSED — every prompt only does so post-confirm — so the over-rejection is harmless. Worth a code comment noting the intentional broadness, in case a future use case (e.g. cross-producer sync) tries this and gets surprised.

5. Doc nit — exit-code wording

docs/reference/agent-wait-patterns.md: "the wrapper surfaces this as exit code 3 (permanent error)." Verified against sandbox/egg_agent_tools/handlers/message.py:319-323 (the wait_loop raises GatewayError on 4xx-non-408) and the rc=3 contract in §3 of the same doc. Accurate. No change needed — flagging only because it's the kind of cross-file claim that rots silently.

Verified correct

• Reviewer-only roles short-circuit on is_producer(role) == False (peer_consensus.py:1259).
• Dual-role agents (tester) transition both _producer_phases and _reviewer_phases in the same handle_confirmed call (L600-604), so a successfully-confirmed tester returns False from the helper.
• NACK regresses producer to WORKING (peer_consensus.py:384); a previously-CONFIRMED producer correctly re-enters the guard's reject path until it re-proposes and re-confirms.
• The guard fires before any expensive work: route enters _check_producer_pending_confirm_guard before get_message_store() and the long-poll bookkeeping (messages.py:466-472).
• 4xx surface: sandbox/egg_agent_tools/handlers/message.py:319-323 correctly maps 400 → GatewayError → permanent for both the CLI shim and mcp__brc__wait_loop.
• Test class names and fixture imports match the existing test_messages.py style; patch("peer_consensus.get_peer_consensus_tracker", …) correctly intercepts the route's local from peer_consensus import ….

— Authored by egg

[egg-reviewer]

Re-reviewed the delta between eeb10f4 and 31d32db. All five suggestions from the previous review were addressed substantively:

1. Misleading comment — test_producer_in_confirmed_is_not_pending no longer claims reviewer must confirm first; the redundant handle_confirmed("reviewer_code") call is gone and the new comment correctly cites check_confirm_guard's actual gates.
2. Dual-role tester — test_dual_role_tester_in_proposed_blocked added at orchestrator/tests/test_messages.py:1709 and asserts the 400 path with the right message fragments.
3. Import-shim drift — _check_producer_pending_confirm_guard now matches the one-tier shape used by _apply_delphi_filter in the same file (orchestrator/routes/messages.py:394-399); the # pragma: no cover and unused package-relative fallback are gone.
4. from_role broadness — Inline comment added at orchestrator/routes/messages.py:387-393 documenting the intentional over-rejection.
5. Exit-code wording — No change requested.

Verified: pytest orchestrator/tests/test_messages.py orchestrator/tests/test_peer_consensus_integration.py → 173 passed (was 172, +1 for the new dual-role test). Ruff clean.

No blocking issues. A few minor non-blocking observations on the new code:

Non-blocking

1. Dead weight in test_producer_in_confirmed_is_not_pending

Now that the reviewer's confirm is dropped, the immediately-prior t.handle_ack("reviewer_code", "documenter", …) (orchestrator/tests/test_peer_consensus_integration.py:3079) and its comment ("Advisory ACK still required for reviewer to satisfy 'must have reviewed' guard, even though it doesn't gate is_fully_acked") no longer serve any purpose:

• ApprovalMatrix.is_fully_acked (orchestrator/approval_matrix.py:188) only iterates critical_reviewers_for(producer) — documenter has zero critical reviewers in this fixture, so it's already fully ACKed without that line.
• The "must have reviewed" guard the comment references only applies when the reviewer tries to confirm, which the test no longer does.

The test still passes; the leftover line and comment just refer to a guard that's no longer exercised. Either drop both, or trim the comment to "Advisory ACK is informational here — kept for symmetry with real pipelines" so future readers don't chase a guard that isn't being tested.

2. test_dual_role_tester_in_proposed_blocked doesn't exercise dual-role behavior

The implement_tracker fixture (orchestrator/tests/test_messages.py:1574-1581) does NOT include the ReviewEdge("tester", "coder", CRITICAL) that the default get_default_implement_graph ships with — so in this fixture tester is purely a producer, not dual-role. The docstring acknowledges this with "(producer + implicit reviewer surface in some graphs)", but the test name still implies dual-role-specific coverage.

Functionally the test does what the previous review asked for (locks tester's producer-phase contract), and the dual-role transition logic is exercised indirectly via the is_producer short-circuit in peer_consensus.py:1259. But if the intent was to lock the tester-as-reviewer-too case, the fixture would need the tester→coder edge. Otherwise consider renaming to test_tester_producer_in_proposed_blocked to match what's actually tested.

3. Comment placement on the from_role broadness

The new comment at orchestrator/routes/messages.py:387-393 sits between the if not blocking: return None early-return and the import shim, which visually ties it to the import rather than to the guard's filtering policy. Moving it up to the function docstring (or to where role is read at L382) would put it closer to where future readers will look when they wonder why from=… is ignored. Minor.

Verified correct

• The simplified import-shim mirrors _apply_delphi_filter (orchestrator/routes/messages.py:319-325) exactly — same try/except → None → if-not-truthy guard. Drift-resolved.
• test_dual_role_tester_in_proposed_blocked correctly drives the tester through handle_propose (PROPOSED phase) and asserts is_producer_pending_confirm("tester") returns True via the route's 400 path.
• The # type: ignore[assignment] annotation on the new get_peer_consensus_tracker = None matches the existing _apply_delphi_filter annotation.
• The not get_peer_consensus_tracker truthy check correctly handles both None (import failed) and any future falsy sentinel without behavior change.
• Ruff still clean across all three modified files.

— Authored by egg

[egg-reviewer]

Re-reviewed the delta between 31d32db and c78a4be (one commit, three localized changes). All three non-blocking suggestions from the previous re-review were addressed substantively.

Suggestions addressed

1. Dead weight in test_producer_in_confirmed_is_not_pending ✓

orchestrator/tests/test_peer_consensus_integration.py:3076 — the redundant t.handle_ack("reviewer_code", "documenter", …) call and its misleading "must have reviewed" comment are gone. The remaining setup (one critical ACK for coder, no ACK for documenter) still satisfies ApprovalMatrix.is_fully_acked("documenter") (zero critical reviewers in the fixture), so handle_confirmed("documenter") returns confirmed and the assertion that is_producer_pending_confirm("documenter") returns False still holds.

2. Dual-role tester fixture ✓

orchestrator/tests/test_messages.py:1580-1584 — ReviewEdge("tester", "coder", CRITICAL) added to the implement_tracker graph so tester is genuinely dual-role (producer of test artifacts + reviewer of coder), matching get_default_implement_graph. The docstring on test_dual_role_tester_in_proposed_blocked is updated to drop the "in some graphs" hedge and now claims dual-role coverage truthfully.

Worth noting (informational, not a critique): the assertion path itself (is_producer_pending_confirm("tester") → True via the producer-phase check at peer_consensus.py:1259-1261) is identical with or without the new edge. The improvement is fixture-realism — the test now operates on a graph that mirrors production, so a future regression that mishandles dual-role roles would be caught here rather than slipping past a producer-only fixture.

The new edge does not break test_producer_in_confirmed_passes because documenter's check_confirm_guard only gates on global_zero_proposal + producer_not_fully_acked for documenter itself, not for coder — coder's incomplete reviewer set under the new edge is irrelevant.

3. from_role-broadness comment moved to docstring ✓

orchestrator/routes/messages.py:379-385 — the policy note now lives in the function docstring where readers will find it when they look at the contract, rather than dangling above the import shim. The body of the function is correspondingly cleaner.

Verified correct

• pytest orchestrator/tests/test_messages.py orchestrator/tests/test_peer_consensus_integration.py → 173 passed (same as before this commit; the new edge doesn't regress any existing assertions).
• pytest …::TestProducerPendingConfirmGuard …::TestIsProducerPendingConfirm → 13/13 pass.
• The docstring for _check_producer_pending_confirm_guard still ends with the Returns ``None`` when … line, so the contract documentation flows correctly after the inserted policy paragraph.
• No changes to orchestrator/peer_consensus.py or docs/reference/agent-wait-patterns.md in this delta — both already in their final shape from the prior pass.

No blocking issues. No new non-blocking issues.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

8 previous review(s) hidden.