fix(simplifier): de-role the producer + move companion gate into the verdict rubric (#3381)

[jwbron]

Fixes #3381.

The simplifier's *-analysis-human.md / *-plan-human.md companion kept coming out as a review/verification memo (verdict framing, file:line detail, plan-phase directives) even after #3376/#3377. Per the issue there are two independent failures; this PR fixes each at its root. No automated keyword guard — the reviewer remains the content gate, as requested.

Problem A — production side: the doc was the wrong format from the start

Even post-#3376 the simplifier was still structurally a reviewer: both prompt paths (_build_producer_orientation and the _build_agent_prompt "## Reviewer role" section) required it to emit an ADVISORY ACK/NACK verdict on the upstream draft in the same pass. So the agent entered every invocation wearing a reviewer hat and its output came out review-shaped. #3376 fenced the companion section ("not a review here") but left the role identity intact — fencing can't beat "you are the reviewer of this draft."

• Strip all reviewer/verdict semantics from the three simplifier prompt blocks (the dual-role banner, the dual-role-agents note, _build_producer_orientation, and the _build_agent_prompt reviewer-role section). The simplifier is now a producer only: the upstream's CONSENSUS_PROPOSE is purely its cue to read-and-summarize; it issues no ACK/NACK.
• The advisory review edge stays in review_graph.py as the pure event-pump wake-wire (it's what re-invokes the simplifier on the upstream's propose; removing it reintroduces the "v|" first-propose dedupe deadlock).
• No consensus-engine change needed. is_fully_acked counts only CRITICAL reviewers, and the Post-BRC event-pump deadlock: confirmed slice never closes (containers won't exit); no self-heal; advance_phase(force=true) trips #2806 + re-entry thrash #3043 confirm-guard exclusion already drops a never-voted advisory edge once the upstream producer confirms — so the simplifier confirms normally without ever casting a verdict.

Problem B — gate side: reviewer_refine approved it anyway

The companion criteria #3376 added lived only in _build_reviewer_preparation — the "while waiting, prepare by…" prep text — not in the rubric the reviewer runs at verdict time. _get_refine_review_criteria / _get_plan_review_criteria were entirely about the refiner/planner draft (sections 1–N) with zero companion criteria, and _get_review_criteria_for_type is keyed by reviewer type, not by which producer is under review. So when reviewer_refine rendered its verdict on the simplifier's companion it applied the refiner-analysis rubric, which gave it no grounds to NACK — and it ACKed by default.

• Add a forcing _human_companion_review_criteria checklist into both verdict rubrics. The reviewer must walk it before ACKing the simplifier — summary-not-review, no file:line/symbol detail, materially lighter than the parent, readable by a non-engineer, faithful — and NACK the simplifier (not the producer it summarises) on any defect.

The two are complementary: A removes the source of the bad format; B is the defense-in-depth gate that should have caught it and didn't.

Tests

• Regression guards for the de-roled simplifier: producer-only banner, no verdict/ACK-NACK instruction, no ## Reviewer role section.
• Verdict-rubric companion-checklist guards for both refine and plan.
• Targeted suite (test_pipeline_prompts.py + test_concurrent_executor.py): 535 pass; ruff clean. Full make test/make test-all not run.

[egg-reviewer]

Review — request changes

I focused on the one part of this PR that changes runtime BRC behavior (Problem A: de-roling the simplifier so it casts no ACK/NACK). The prompt edits themselves are clean and Problem B is well-scoped. But the "No consensus-engine change needed" claim is incomplete: it analyzes is_fully_acked and the #3043 confirm-guard, but not the event-loop's next-action derivation, which still treats the advisory edge as requiring a vote. The result is a behavioral regression.

Blocking — the advisory ACK was load-bearing for terminating the wake-arm; removing it turns a quiet wait into repeated ack spawns

The PR keeps the simplifier → refiner advisory edge "as the pure event-pump wake-wire" but removes the only thing that ever acknowledged it. Pre-PR the wake-wire was self-terminating; post-PR it is self-perpetuating. Trace:

• _has_pending_peer_proposals(reviewer) (routes/consensus.py:158) returns a producer as "pending review" whenever the producer is PROPOSED and the reviewer has no current-version verdict entry (entry is None or entry.version < current_version, line 193). It does not special-case advisory edges — producers_for (review_graph.py:69) includes them.
• For the de-roled simplifier, the (simplifier, refiner) matrix entry now never gets created (it casts no verdict — confirmed against the wrapper: the ack arm runs the agent, which under the new prompt issues nothing; the wrapper does not auto-ACK, consensus_wrapper.py:484-489). So _has_pending_peer_proposals(simplifier) returns refiner for the entire window refiner is PROPOSED.
• In _derive_next_action (routes/consensus.py:296), the dual-role simplifier in PROPOSED fails check_confirm_guard (reviewer Guard 1 must_have_reviewed, action_guards.py:456-463, because has_reviewed(simplifier, refiner) is now permanently False), falls through to the reviewer block, and returns "ack" (line 404-410).
• "ack" is a SPAWN action (event_loop.py:94). Terminal (EXITED) Jobs do not suppress re-spawn (kubernetes_spawner/_events.py:22-66 and the outcome_for design note: "an event whose pod failed without advancing the tracker would be silently swallowed … instead of respawned"). The driver re-derives every role on a fixed 5s poll (event_loop.py:1116-1125).

Net effect: for the whole span [simplifier proposes companion → refiner confirms] (which includes reviewer_agent_design and reviewer_refine reviewing refiner), the simplifier's derived action is ack and it gets re-spawned — invoking a full agent each time that can no longer satisfy the event. Pre-PR this same span was a quiet wait (line 414): the agent's advisory ACK had advanced the matrix entry so _has_pending_peer_proposals returned False.

Two concrete harms:

1. Resource/robustness regression — repeated agent (pod) spawns for the duration the upstream is under review, where before there were none.
2. Correctness risk — the re-spawned ack invocation still hands the agent the producer prompt ("write and PROPOSE the companion"). If the agent re-proposes the companion, it bumps the simplifier's own proposal version and invalidates reviewer_refine's ACK on it (stale-version), churning the companion's own consensus.

This is not a permanent deadlock — confirm is agent-free (AGENT_FREE_ACTIONS, event_loop.py:95) and once refiner confirms, the #3043 filter (action_guards.py:434-454) drops the never-voted edge and the loop auto-confirms the simplifier. But "converges eventually after churn" is not the same as "no consensus-engine change needed."

What's needed:

• Make the wake-wire self-terminating again without re-introducing a verdict. Options: (a) skip advisory edges for a producer-only-configured reviewer in _has_pending_peer_proposals / _derive_next_action so a de-roled advisory reviewer is never assigned ack; or (b) auto-record the advisory edge orchestrator-side when the simplifier is woken, so has_reviewed/pending clears exactly as the agent's ACK used to. Either keeps the proven wake-up while stopping the re-spawn.
• Add a behavioral test (not just prompt-text assertions) over _derive_next_action / the event loop: refiner PROPOSED + simplifier PROPOSED (no verdict) must not keep yielding a spawn-able ack, and the phase must converge. The current tests (test_pipeline_prompts.py) only assert the prompt strings — they would not catch this.

Non-blocking observations

• Problem B scoping is correct. The companion checklist is added only to _get_refine_review_criteria / _get_plan_review_criteria, which dispatch from reviewer types refine / plan (_get_review_criteria_for_type, pipelines.py:5789). reviewer_agent_design (type agent-design) does not get it — good, since it doesn't review the simplifier. The explicit "NACK the simplifier, never the {producer}" guard inside the shared verdict is the right call given the reviewer renders separate per-edge verdicts.
• _human_companion_review_criteria is module-level and forward-referenced by _get_refine_review_criteria; resolved at call time, so the definition-after-use ordering is fine.
• The prompt de-roling text itself reads cleanly and consistently across all four blocks.

— Authored by egg

[james-in-a-box]

Thanks for the trace — the blocking finding was exactly right, and the fix is now pushed.

Disposition

1. Blocking — advisory ACK was load-bearing; removing it turns a quiet wait into repeated ack spawns — fixed-in-PR (commit 99a3405)

You were correct: the wake-wire became self-perpetuating because nothing acknowledged the simplifier → upstream edge anymore, so _has_pending_peer_proposals returned the upstream as "pending" for the whole [upstream proposes → upstream confirms] window and _derive_next_action kept yielding a spawn-able ack.

I took option (a) — make the wake-wire self-terminating without reintroducing a verdict — via an explicit, principled marker rather than a role-name special-case:

• Added a wake_only: bool flag on ReviewEdge (serialized through to_dict/from_dict) and marked both simplifier advisory edges (simplifier → refiner in refine, simplifier → task_planner in plan). A wake_only edge drives the event-pump wake-wire but carries no review obligation. I did not blanket-skip all advisory edges precisely because genuine advisory reviewers (reviewer_plan → risk_analyst, reviewer_code → documenter) still vote and must keep being routed to ack.
• _has_pending_peer_proposals (routes/consensus.py) now skips wake_only producers → the de-roled simplifier derives a quiet wait, never a spawn-able ack.
• check_confirm_guard (action_guards.py) now excludes wake_only producers from the reviewer guards (alongside the existing no-op / already-confirmed exclusions) → the simplifier confirms its companion as soon as its CRITICAL reviewer (reviewer_refine/reviewer_plan) ACKs it, without waiting for the upstream to confirm. This restores the pre-PR convergence timing rather than relying on the Post-BRC event-pump deadlock: confirmed slice never closes (containers won't exit); no self-heal; advance_phase(force=true) trips #2806 + re-entry thrash #3043 "converges eventually after churn" path.

Behavioral test (as requested) — fixed-in-PR (commit 99a3405): added two tests in test_consensus_next_action.py that drive a real get_default_refine_graph() tracker through _derive_next_action (not prompt-string assertions):

• test_wake_only_simplifier_never_routed_to_ack — with refiner PROPOSED + simplifier PROPOSED and no verdict, repeated derivations stay wait (never ack), including across the full span the refiner is fully-ACKed-but-not-confirmed.
• test_wake_only_simplifier_confirms_without_verdict — the phase converges to global consensus and the simplifier never records a verdict on the refiner.

2. Non-blocking — Problem B scoping / forward-ref / prompt de-roling — acknowledged, no change

Agreed on all three observations; no change needed. The companion checklist stays scoped to _get_refine_review_criteria/_get_plan_review_criteria (not reviewer_agent_design), the module-level forward reference resolves at call time, and the four de-roling blocks read consistently.

make lint is clean for the changed files (the 3 pre-existing mypy errors are in shared/egg_agent/client.py, untouched by this PR). Per workflow policy I did not re-run the full suite; CI covers HEAD.

— Authored by egg

[egg-reviewer]

Re-review — approve (prior blocking issue resolved); non-blocking concerns on the rationale

The blocking finding from my last review is fixed and verified. The de-roled simplifier no longer churns a spawn-able ack for the window the upstream is PROPOSED: _has_pending_peer_proposals (routes/consensus.py:172,180-191) and check_confirm_guard (action_guards.py:459-463) both now exclude wake_only producers, so the simplifier derives a quiet wait post-propose and confirms its companion once reviewer_refine ACKs it. The two new derivation tests (test_consensus_next_action.py) exercise the real get_default_refine_graph() tracker and assert the post-propose wait/confirm/converge path — which is exactly the segment that was regressing. Good.

I also confirmed the fix is complete across the graph: every other reviewers_for/producers_for consumer safely no-ops on the wake-only edge, because the de-roled simplifier never creates a matrix entry over the refiner —

• is_fully_acked(refiner) is critical-only (approval_matrix.py:294), so the refiner's convergence is unaffected by the never-cast advisory edge;
• _un_confirm_stale_reviewers / _invalidate_pre_proposal_acks (_state.py:206,260) and the _confirm.py invalidation loops all guard on entry is None / invalidate_ack no-op;
• the check_invariants reviewer loop (action_guards.py:743-755) continues on entry is None, so a confirmed simplifier with no refiner verdict trips no invariant.

So I'm approving. The remaining items below are non-blocking — the feature works end-to-end — but they matter for this being critical, long-lived infra.

Non-blocking 1 — the justifying comments (and commit message) describe the wrong wake mechanism, and contradict themselves

The new comments assert the ack arm is the simplifier's wake-up:

"the BRC ack arm re-invokes it when the refiner proposes (the proven wake-up the spawn-dedupe key relies on; a pure producer's first-propose key is constant and would never re-spawn)" — review_graph.py refine/plan graphs

This is self-contradictory: the same wake_only flag that the comment introduces is precisely what removes the edge from pending-review derivation, so the ack arm can never fire for it. More importantly, the underlying claim is false in the post-#3164 one-shot model:

• The simplifier is a producer; in WORKING it derives propose first (R11a, routes/consensus.py:357-372) — not ack. The ack arm only ever applied after it proposed (PROPOSED → reviewer fall-through). So the ack/advisory edge was never the wake-to-propose; it was the post-propose advisory re-invocation (the thing de-roling removed, and the churn this PR fixes).
• A WORKING producer whose derived action stays propose does re-spawn every poll: a clean orient-and-exit is classified success/legitimate, which frees the dedupe key, and the next poll re-derives the same propose key and spawns again. This is exactly what your own test_event_loop.py::test_stale_exit_is_a_non_trigger_through_loop asserts ("Legitimate outcomes free the key, so it keeps getting respawned"). So the propose arm is the simplifier's wake-to-propose, and has been all along.

Net: the behavior is correct, but the documented reason for the wake_only edge's existence is wrong. That's a maintenance trap in consensus code — it tells the next person the ack arm is the load-bearing wake-wire when it is inert. Please correct the comments to state that (a) the simplifier is woken to propose by the propose-arm re-spawn, and (b) wake_only exists only to neutralize the residual advisory edge (no ack, no confirm-block), not to "drive" a wake.

Non-blocking 2 — given (1), the simplifier → refiner edge looks unnecessary; deleting it is simpler and removes a prompt contradiction

If the propose arm is the real wake, the advisory edge buys nothing functional — dropping ReviewEdge("simplifier", "refiner", …) / ("simplifier", "task_planner", …) entirely yields identical behavior (simplifier woken via propose arm; not a reviewer, so no ack churn and nothing to skip). It would also fix a concrete inconsistency this PR's de-roling leaves behind: because the edge keeps graph.is_reviewer("simplifier") == True, the BRC preamble still renders Your role type: **PRODUCER and REVIEWER (dual role)** and Your assigned producers: refiner (pipelines.py:13238-13241) — directly above the new "You are a producer only" banner. That mixed signal is the kind of thing #3381 was trying to kill. If there's a real reason to retain the edge (e.g. the health-monitor idle-suppression in _queries.py:179), call it out in the comment instead of the false "ack wake-wire" story.

Non-blocking 3 — the new tests don't exercise the actual wake path

Both new tests call _propose(t, "simplifier"), which drives the tracker FSM straight to PROPOSED, bypassing the event loop's spawn/dedupe entirely. They validate "given the simplifier has proposed, derivation converges without a verdict" — valuable for the churn fix — but they do not cover "the WORKING simplifier actually gets woken to PROPOSED." A regression that prevented the simplifier from ever proposing would not break them. The real wake is covered generically by the test_event_loop.py re-spawn tests, so this is acceptable; but since the PR's stated mechanism is both wrong and untested, a small loop-level test (or just fixing the comments to point at the mechanism that is tested) would close the gap.

— Authored by egg

[james-in-a-box]

Thanks for the approval and the careful follow-up. The blocking finding was fixed in 99a3405; this commit addresses all three non-blocking items.

Disposition

Non-blocking 1 — justifying comments describe the wrong wake mechanism / contradict themselves — fixed-in-PR (commit 1712801)

You're right: the ack arm is inert for a wake_only edge (it's excluded from pending-review derivation), and a WORKING producer is re-spawned to propose by the propose-arm — a legitimate orient-and-exit frees the spawn-dedupe key, so it keeps getting respawned (your own test_event_loop.py::test_stale_exit_is_a_non_trigger_through_loop). I corrected the comments to state (a) the simplifier is woken to propose by the propose-arm, self-gating on the upstream draft existing, and (b) wake_only exists only to neutralize the residual advisory edge (no ack, no confirm-block), not to drive a wake. Fixed in three places in review_graph.py (the ReviewEdge.wake_only docstring + both graph builders) and — since the same false story appeared there too — the code comment in _build_brc_preamble (pipelines.py).

Non-blocking 2 — edge looks unnecessary; preamble contradiction — fixed-in-PR (commit 1712801) for the contradiction; disagree on deleting the edge

• Preamble contradiction (fixed): a role whose only reviewed producers are reached via wake_only edges casts no verdict on anyone, so _build_brc_preamble now renders it PRODUCER (not dual-role) and drops the Your assigned producers: refiner line. The graph-level is_dual_role flag is kept intact for the banner dispatch. Verified: the simplifier preamble now reads Your role type: **PRODUCER** with no contradictory producers line, directly above the producer-only execution banner.
• Deleting the edge (disagree): deletion is not behavior-neutral, so I kept it. The Problem-B prompt de-roling is gated on the simplifier being graph-dual-role (if is_dual_role and role_value == "simplifier" → the producer-only ### Execution Order banner). Dropping the edge would force re-gating that banner and rewriting the agent-facing wake prompt to rely solely on the propose-arm — which is exactly the wake path you flagged as untested in Non-blocking 3. That's a real behavioral change to the de-roling, out of proportion to a polish item, and not safely validatable under this workflow's "no full suite" constraint. So the edge is retained as a fully-neutralized wake_only marker, with the corrected comments now documenting why it's there (the real reason you asked me to surface) instead of the false ack-wire story.

Non-blocking 3 — tests don't exercise the actual wake path — fixed-in-PR (commit 1712801)

Added test_wake_only_simplifier_woken_to_propose_by_propose_arm to test_consensus_next_action.py: it drives a WORKING simplifier (not yet PROPOSED) through _derive_next_action and asserts it derives propose — both before and after the refiner proposes — demonstrating the propose-arm is the wake-to-propose and that the simplifier is never routed to ack for the advisory edge. The comments now also point at test_event_loop.py's re-spawn coverage for the loop-level mechanism.

make lint is clean for the changed files (ruff check + format pass; no new mypy errors in the edited regions). Targeted tests pass: test_consensus_next_action.py (17), test_pipeline_prompts.py (464), test_action_guards.py + test_review_graph_lens_reviewers.py (88). Per workflow policy I did not re-run the full suite; CI covers HEAD.

— Authored by egg

[egg-reviewer]

Re-review — request changes: Problem A's de-roling is incomplete; the de-roled simplifier still receives the full reviewer playbook

The delta since my last approval (commit 1712801) correctly addresses the three non-blocking items I raised:

• Wake-mechanism comments (review_graph.py, _build_brc_preamble) now accurately describe the propose-arm as the wake and wake_only as a pure neutralizer. Good.
• Role-type label now renders Your role type: **PRODUCER** and drops the Your assigned producers: refiner line via the new real_producers / casts_real_verdicts computation (pipelines.py:13241-13242). Verified by rendering.
• New behavioral tests in test_consensus_next_action.py exercise the real get_default_refine_graph() tracker and cover the post-propose wait/confirm/converge path and the propose-arm wake. Sound, and they close the gap I flagged.

The Problem B (gate-side) work is also correct: _human_companion_review_criteria is now in both verdict rubrics (_get_refine_review_criteria / _get_plan_review_criteria), scoped to NACK the simplifier, and the forward reference resolves at call time. Verified both render.

But there is a blocking gap that this PR's own thesis makes blocking, and that my prior non-blocking #2 pointed straight at. I fixed the symptom I named (the role-type label) but the root cause I named (graph.is_reviewer("simplifier") == True) drives two far larger reviewer-instruction blocks in the same preamble, and neither was de-roled.

Blocking — the simplifier preamble still hands the de-roled agent a complete ACK/NACK reviewer playbook

Both of these blocks in _build_brc_preamble are gated on raw is_reviewer, which remains True for the simplifier because the wake_only edge keeps it in _reviewer_roles (review_graph.py:95-97):

1. ### Reviewer Lifecycle (pipelines.py:13499) — emitted in full to the simplifier:
   • 4. **REVIEW**: ... form independent judgment from the referenced code artifacts
   • 5. **ACK/NACK**: Your --reason IS your review ... with the literal egg-orch consensus nack <role> --files-reviewed ... --reason command template
   • 6. **CONFIRM**: When all assigned producers reviewed ...
   • 7. **HANDLE RE-REVIEW** ... This is adversarial re-review ... NACK without hesitance
2. The **As a reviewer** directed-coordination block (pipelines.py:13698) — when you need clarification before ACK/NACKing, put the question in your NACK --reason block.

Rendered output for the simplifier (refine and plan, confirmed by running _build_brc_preamble) is self-contradictory: the new banner says

You do not review, critique, score, or vote on that draft — you never issue an ACK or a NACK.

and roughly five lines later the same preamble says

5. **ACK/NACK**: … egg-orch consensus nack <role> …

plus a full "adversarial re-review … NACK without hesitance" mandate.

This is exactly the failure mode the PR's Problem A is built to eliminate. Your own diagnosis is that the agent "entered every invocation wearing a reviewer hat and its output came out review-shaped," and the fix is to "strip all reviewer/verdict semantics from the … simplifier prompt blocks." You stripped four blocks (the two banners, _build_producer_orientation, the _build_agent_prompt reviewer section) but left the most operationally detailed reviewer instruction of all — a complete, actionable ACK/NACK + adversarial-re-review lifecycle — rendered verbatim to the de-roled simplifier. A single inserted producer-only sentence in step 2 (pipelines.py:13517-13525) does not neutralize the five surrounding steps that instruct it to REVIEW, ACK/NACK, CONFIRM, and adversarially re-review.

The codebase already encodes the correct invariant: test_producer_only_no_sync_step (test_pipeline_prompts.py:5320-5324) asserts that a producer-only role (coder) gets no ### Reviewer Lifecycle at all. This PR explicitly redefines the simplifier as "a producer only" — so by the project's own standard it must satisfy that same invariant, and it does not.

Per the review rules this is blocking: the feature's core purpose (de-role the simplifier so it stops receiving reviewer framing) does not work end-to-end in the very function the PR modifies to achieve it — a single-module break in _build_brc_preamble.

Suggested fix. You already compute casts_real_verdicts (pipelines.py:13242). Gate both reviewer blocks on it instead of raw is_reviewer:

if is_reviewer and casts_real_verdicts:   # 13499 — Reviewer Lifecycle
    ...
if is_reviewer and casts_real_verdicts:   # 13698 — "As a reviewer" coordination
    ...

A pure reviewer (reviewer_refine) and the tester both have casts_real_verdicts == True (their reviewed producers are non-wake-only), so they keep the Reviewer Lifecycle; the simplifier (real_producers == []) drops it. Once gated out, the simplifier-specific producer-only note inside step 2 becomes dead code and should be removed with it.

Why the new guards didn't catch this

test_simplifier_banner_issues_no_verdict asserts only the absence of the specific phrases you removed ("issue your advisory verdict", "advisory ack/nack") and the presence of "you never issue an ack or a nack". None of those match the leaked ### Reviewer Lifecycle / **As a reviewer** text, so the guard passes while the contradiction stands. After the fix, add a positive regression guard mirroring the existing producer-only invariant, e.g.:

assert "### Reviewer Lifecycle" not in preamble
assert "egg-orch consensus nack <role>" not in preamble
assert "**As a reviewer**" not in preamble

— Authored by egg

[james-in-a-box]

Thanks — the blocking finding was exactly right: gating those two blocks on raw is_reviewer leaked the full reviewer playbook to a role the rest of the PR redefines as producer-only. Fixed and pushed.

Disposition

Blocking — the de-roled simplifier still receives the full ACK/NACK + adversarial-re-review reviewer playbook — fixed-in-PR (commit 3a198b0)

You diagnosed the root cause precisely: graph.is_reviewer("simplifier") == True (the wake_only edge keeps it in _reviewer_roles), so both reviewer-instruction blocks in _build_brc_preamble — ### Reviewer Lifecycle and the **As a reviewer** directed-coordination block — rendered verbatim to the simplifier, directly contradicting its producer-only banner. I took your suggested fix:

• Gated both blocks on casts_real_verdicts instead of raw is_reviewer. A role whose only reviewed producers are reached via wake_only edges (real_producers == []) now drops the entire Reviewer Lifecycle and the As a reviewer coordination block. The dual-role tester and the pure reviewer_refine both have casts_real_verdicts == True, so they keep both blocks unchanged.
• Removed the now-dead simplifier-specific producer-only note inside Reviewer Lifecycle step 2 — it could only ever render when casts_real_verdicts is true, which the simplifier never is, so it was unreachable once the block is gated out.

This brings the simplifier into compliance with the project's own producer-only invariant (test_producer_only_no_sync_step for the coder): a producer-only role gets no ### Reviewer Lifecycle at all.

Regression guard (as requested) — fixed-in-PR (commit 3a198b0): added test_simplifier_preamble_has_no_reviewer_lifecycle to test_pipeline_prompts.py, mirroring the existing producer-only invariant. For both refine and plan it asserts:

assert "### Reviewer Lifecycle" not in preamble
assert "egg-orch consensus nack <role>" not in preamble
assert "**As a reviewer**" not in preamble
assert "NACK without hesitance" not in preamble

Verified by rendering: the simplifier preamble (refine + plan) now reads Your role type: **PRODUCER** with no Reviewer Lifecycle, no As a reviewer block, and no NACK template; tester (dual-role) and reviewer_refine (pure reviewer) retain both blocks.

make lint is clean for the changed files (ruff check + format pass). Targeted tests pass: test_pipeline_prompts.py (465) + test_consensus_next_action.py (17) = 482. Per workflow policy I did not re-run the full suite; CI covers HEAD.

— Authored by egg

[egg-reviewer]

Re-review — request changes: the de-roling is still incomplete; producer-lifecycle step 6 leaks dual-role reviewer framing (and a now-dangling cross-reference) to the simplifier

The delta since my last review (1712801..3a198b0) correctly applies the fix I suggested for the two blocks I named:

• ### Reviewer Lifecycle (pipelines.py:13509) and the **As a reviewer** coordination block (pipelines.py:13702) are now gated on casts_real_verdicts (pipelines.py:13242) instead of raw is_reviewer. Verified by rendering: the simplifier (refine + plan) reads Your role type: **PRODUCER**, gets no Reviewer Lifecycle, no As a reviewer block, no NACK template; tester (dual-role) and reviewer_refine (pure reviewer) retain both blocks unchanged.
• The dead if role_value == "simplifier" branch inside Reviewer-Lifecycle step 2 is correctly removed — it was unreachable once the block is gated on casts_real_verdicts (the simplifier never satisfies it).
• The new test test_simplifier_preamble_has_no_reviewer_lifecycle exercises the real _build_brc_preamble for both phases. Good. And test_preparation_in_reviewer_lifecycle (reviewer_code) guards against over-aggressive gating that would strip a genuine reviewer.

casts_real_verdicts = bool([p for p in producers if p not in wake_only_producers]) is the right discriminator: the two wake_only=True edges (review_graph.py:253,304) are the only wake-only edges, both with the simplifier as reviewer, so the simplifier is the only role for which casts_real_verdicts == False. The gate is precise.

But the same root cause I flagged last round — graph.is_reviewer("simplifier") == True, and now also is_dual_role == True — drives a third reviewer-framing site that this commit did not de-role. The fix is incomplete in the very same way, in the same function.

Blocking — Producer-Lifecycle step 6 still hands the de-roled simplifier dual-role re-review/ACK-NACK framing, and points it at a Reviewer Lifecycle section this PR deleted from its preamble

### Producer Lifecycle is gated on is_producer (correct — the simplifier is a producer and its companion is reviewed). But step 6 HANDLE RE-REVIEW (pipelines.py:13456) contains an is_dual_role-gated parenthetical (pipelines.py:13458-13466):

"(or a CONSENSUS_PROPOSE for a re-propose — version > 1, after you NACKed a prior version; dual-role agents handle both — see Reviewer Lifecycle step 8 for the adversarial re-review framing)"

and the following sentence (pipelines.py:13468-13472):

"If you are a reviewer of the re-proposing producer, re-review and ACK/NACK the new proposal (dual-role agents: see Reviewer Lifecycle step 8 below for the adversarial re-review framing that applies to this case)."

is_dual_role is still True for the simplifier (the PR deliberately keeps the graph-level flag for banner dispatch, pipelines.py:13237), so the parenthetical renders. I confirmed by rendering _build_brc_preamble("simplifier", "refine"/"plan"): the simplifier preamble contains dual-role agents handle both, re-review and ACK/NACK the new proposal, and Reviewer Lifecycle step 8 — all True.

Two concrete harms, both newly introduced/left by this PR:

1. Self-contradiction — the exact failure mode Problem A exists to kill. The producer-only banner this PR adds says (pipelines.py:13300-13304):

   "You do not review, critique, score, or vote on that draft — you never issue an ACK or a NACK."

   Step 6, a few lines down in the same preamble, tells the same agent "dual-role agents handle both [re-reviews]" and "re-review and ACK/NACK the new proposal." This is the identical self-contradiction my prior review (and the round before it) called blocking — just relocated from the two big blocks into producer step 6. Your own diagnosis was to "strip all reviewer/verdict semantics from the simplifier prompt blocks"; this is residual reviewer/verdict semantics.

2. Dangling cross-reference introduced by this PR. Both clauses say "see Reviewer Lifecycle step 8 below." This PR removes the entire Reviewer Lifecycle from the simplifier's preamble — so for the simplifier the reference now points at a section that is not there at all. Pre-PR the simplifier got the full Reviewer Lifecycle, so the reference resolved; this PR breaks it. The next reader of the simplifier prompt is told to consult a section the same change deleted.

This is a single-module break in _build_brc_preamble — the feature's stated purpose (de-role the simplifier so it receives no reviewer framing) does not fully hold end-to-end in the function the PR modifies to achieve it.

Why the new guard misses it: test_simplifier_preamble_has_no_reviewer_lifecycle only asserts the absence of "### Reviewer Lifecycle", "egg-orch consensus nack <role>", "**As a reviewer**", and "NACK without hesitance" — none of which match the leaked step-6 strings. The guard passes while the contradiction stands, exactly as the previous guard did before this round.

Suggested fix. Gate the dual-role reviewer references in producer step 6 on casts_real_verdicts, mirroring the two blocks you already fixed:

# pipelines.py:13458 — the parenthetical
+ (
    " (or a `CONSENSUS_PROPOSE` for a re-propose — ... dual-role "
    "agents handle both — see Reviewer Lifecycle step 7 for the "
    "adversarial re-review framing)"
    if is_dual_role and casts_real_verdicts   # was: if is_dual_role
    else ""
),

and make the "(dual-role agents: see Reviewer Lifecycle step 8 below ...)" aside in 13468-13472 conditional on the same casts_real_verdicts (today it is ungated, so it also renders for pure producers — see Non-blocking below). The tester keeps it (casts_real_verdicts == True); the simplifier drops it. Then extend the regression guard to assert the simplifier preamble contains none of "re-review and ACK/NACK the new proposal", "dual-role agents handle both", or "Reviewer Lifecycle step".

Non-blocking 1 — "Reviewer Lifecycle step 8" is itself a stale/off-by-one reference (pre-existing, affects the tester too)

The Reviewer Lifecycle renders only 7 steps (PREPARE … HANDLE RE-REVIEW); the adversarial re-review framing lives in step 7 (pipelines.py:13643), not step 8. So even for the tester, "see Reviewer Lifecycle step 8" points one step past the end. This predates the PR, but since you'll be editing these two references to fix the blocking leak anyway, correct step 8 → step 7 while you're there.

Non-blocking 2 — the ungated dual-role aside renders for pure producers with no Reviewer Lifecycle

"(dual-role agents: see Reviewer Lifecycle step 8 below ...)" (13468-13472) is ungated, so it renders in the coder/refiner preambles too — neither of which has a Reviewer Lifecycle. It is addressed to "dual-role agents," so a pure producer reasonably skips it, but it's a reference to a non-existent "step 8 below" for every pure producer. Gating it on casts_real_verdicts per the suggested fix above resolves this at the same time.

— Authored by egg

[james-in-a-box]

Thanks — the blocking finding was exactly right, and the two non-blocking items rode along in the same edit. Fixed and pushed in b44a5ed.

Disposition

Blocking — Producer-Lifecycle step 6 still leaks dual-role reviewer framing (and a dangling cross-reference) to the de-roled simplifier — fixed-in-PR (commit b44a5ed)

You diagnosed it precisely: the same root cause (is_dual_role == True for the simplifier, kept for banner dispatch) drove a third reviewer-framing site I hadn't gated — step 6 HANDLE RE-REVIEW. The is_dual_role-gated parenthetical and the ungated "If you are a reviewer of the re-proposing producer, re-review and ACK/NACK the new proposal" sentence both rendered for the simplifier, contradicting its producer-only banner. I applied your suggested fix, gating on the existing casts_real_verdicts discriminator:

• The parenthetical (pipelines.py:13458) is now if is_dual_role and casts_real_verdicts (was if is_dual_role).
• The "If you are a reviewer of the re-proposing producer, re-review and ACK/NACK …" sentence (13468) is now if casts_real_verdicts, with an else that emits just "Re-confirm via \egg-orch consensus confirmed`."` for producer-only roles.

Verified by rendering _build_brc_preamble: the simplifier (refine + plan) step 6 now reads only "… act on it — failure to respond stalls the pipeline. Re-confirm via \egg-orch consensus confirmed`."— nore-review and ACK/NACK the new proposal, no dual-role agents handle both, no Reviewer Lifecycle step. The tester(dual-role) keeps the full framing;reviewer_refine` (pure reviewer) is unaffected.

Regression guard (as requested) — fixed-in-PR (commit b44a5ed): extended test_simplifier_preamble_has_no_reviewer_lifecycle to also assert the simplifier preamble contains none of re-review and ACK/NACK the new proposal, dual-role agents handle both, or Reviewer Lifecycle step, for both refine and plan.

Non-blocking 1 — "Reviewer Lifecycle step 8" is a stale/off-by-one reference — fixed-in-PR (commit b44a5ed)

Correct — the Reviewer Lifecycle renders 7 steps and the adversarial re-review framing is step 7 (pipelines.py:13643), not step 8. Both references in step 6 now say "step 7." Since the de-roling drops the entire Reviewer Lifecycle for the simplifier, the corrected reference now renders only for the tester, where step 7 actually exists.

Non-blocking 2 — the ungated dual-role aside renders for pure producers with no Reviewer Lifecycle — fixed-in-PR (commit b44a5ed)

Resolved by the same casts_real_verdicts gate: the "If you are a reviewer of the re-proposing producer …" sentence (and its embedded step reference) no longer renders for coder/refiner/simplifier. Verified the coder step 6 now reads the producer-only "Re-confirm via \egg-orch consensus confirmed`."` form.

make lint is clean for the changed files (ruff check + format pass). Targeted tests pass: test_pipeline_prompts.py selection (108: simplifier/tester/producer-only/reviewer-lifecycle/preparation invariants). Per workflow policy I did not re-run the full suite; CI covers HEAD.

— Authored by egg

[egg-reviewer]

Re-review — approve (prior blocking issue resolved); one non-blocking note on the degraded fallback path

The delta since my last review (3a198b0..b44a5ed) correctly resolves the blocking finding I raised and both non-blocking items that rode along.

Prior blocking issue — Producer-Lifecycle step 6 leaked dual-role reviewer framing to the de-roled simplifier — fixed and verified

I confirmed by rendering _build_brc_preamble("simplifier", "refine"/"plan"):

• Role label is Your role type: **PRODUCER** with no Your assigned producers: line.
• No ### Reviewer Lifecycle, no **As a reviewer** coordination block, no egg-orch consensus nack <role> template, no NACK without hesitance.
• Step 6 (HANDLE RE-REVIEW) now renders only Re-confirm via \egg-orch consensus confirmed`.— thedual-role agents handle bothparenthetical and there-review and ACK/NACK the new proposalsentence are gone. Both are gated oncasts_real_verdicts (pipelines.py:13509parenthetical viais_dual_role and casts_real_verdicts; the following sentence via casts_real_verdictswith a producer-onlyelse`).
• The only remaining ACK/NACK strings in the simplifier preamble are legitimate: the agent roster's descriptions of other reviewers, the producer-only banner that negates it ("you never issue an ACK or a NACK"), and the producer's own RESPOND-TO-REVIEWS / CONFIRM steps for its companion (which reviewer_refine does gate CRITICAL).

Non-blocking 1 (step 8 → 7 off-by-one) — fixed. The two step-6 cross-references now say "Reviewer Lifecycle step 7", and I verified the rendered Reviewer Lifecycle has exactly 7 steps with step 7 = HANDLE RE-REVIEW (where the adversarial re-review framing lives). No step 8 string remains anywhere.

Non-blocking 2 (ungated aside rendering for pure producers) — fixed. coder/refiner/task_planner step 6 now renders the producer-only Re-confirm via … form (no dangling step reference).

Gate precision — verified

I rendered the full role matrix to confirm the casts_real_verdicts gate neither over- nor under-strips:

role	phase	role label	Reviewer Lifecycle
simplifier	refine/plan	PRODUCER	absent ✓
tester	implement	PRODUCER and REVIEWER	present ✓
reviewer_refine	refine	REVIEWER	present ✓
reviewer_plan	plan	REVIEWER	present ✓
reviewer_code	implement	REVIEWER	present ✓
risk_analyst	plan	REVIEWER	present ✓
coder / refiner / task_planner	—	PRODUCER	absent ✓

casts_real_verdicts = bool([p for p in producers if p not in wake_only_producers]) is the right discriminator: the two wake_only=True edges (review_graph.py:253,304) are the only wake-only edges in the graph, both with the simplifier as reviewer, so the simplifier is the only role for which it is False. to_dict/from_dict round-trips the flag (verified). Any is imported (review_graph.py:12).

Tests — sound, exercise the production path

The new test_simplifier_preamble_has_no_reviewer_lifecycle / test_simplifier_banner_issues_no_verdict / test_simplifier_agent_prompt_has_no_reviewer_section and the two verdict-rubric tests drive the real _build_brc_preamble / _build_agent_prompt / _get_*_review_criteria helpers (no golden fixtures, no hand-built bypass), and the names match their assertions. test_preparation_in_reviewer_lifecycle (reviewer_code) guards against over-aggressive gating. The three test_consensus_next_action.py wake-only tests drive a real get_default_refine_graph() tracker through _derive_next_action. I ran the targeted selection locally: test_consensus_next_action.py -k wake_only (3) + TestSimplifierHumanCompanionPrompt (19) all pass.

Non-blocking — the casts_real_verdicts gate degrades all reviewers to PARTICIPANT in the graph-load fallback path

Changing the gate from raw is_reviewer to is_reviewer and casts_real_verdicts has an interaction with the except Exception: fallback in _build_brc_preamble (pipelines.py:13193) that this PR does not account for. In that branch producers = [] is set unconditionally (and wake_only_producers = set()), so real_producers == [] and casts_real_verdicts == False for every role — not just the simplifier.

Consequence in the fallback path:

• A genuine pure reviewer (reviewer_code, reviewer_refine, reviewer_plan) now renders Your role type: **PARTICIPANT** (it hits the final else at pipelines.py:13251, since it is neither is_producer nor casts_real_verdicts) and gets no Reviewer Lifecycle and no As a reviewer block.
• Pre-PR, the same fallback gated on raw is_reviewer, so those roles rendered REVIEWER with the full reviewer playbook.

So this PR silently strips the reviewer playbook from real reviewers whenever the graph load fails. The realistic impact is low: get_review_graph_for_phase is a pure dict lookup with no I/O (review_graph.py:388), so the except only fires on catastrophic review_graph import failure — a state where BRC is broken regardless. That's why I'm classifying this non-blocking, not as a feature break. But it is a latent inconsistency introduced in code the PR modifies, and it's cheap to close: in the except block, either set casts_real_verdicts = is_reviewer explicitly (the fallback can't distinguish wake-only edges, and the only wake-only role — the simplifier — is independently rendered producer-only by the is_dual_role and role_value == "simplifier" banner dispatch, which still fires in the fallback), or seed a non-empty producers sentinel for the known reviewer roles. A one-line assertion that reviewer_code's fallback preamble still contains ### Reviewer Lifecycle would lock it.

Net: Problem A (de-role the simplifier) and Problem B (companion checklist in the verdict rubric) both work end-to-end in the normal path, the three rounds of blocking findings are all resolved, and the fix is precise. Approving.

— Authored by egg

[james-in-a-box]

Thanks for the approval and the careful note on the degraded fallback path. I took it in-PR rather than leaving the latent inconsistency in code this PR modifies.

Disposition

Non-blocking — the casts_real_verdicts gate degrades all reviewers to PARTICIPANT in the graph-load fallback path — fixed-in-PR (commit e145e62)

You diagnosed it precisely: casts_real_verdicts was computed unconditionally from real_producers = producers - wake_only_producers, and the except Exception: fallback (pipelines.py:13193) sets producers = [] / wake_only_producers = set() for every role. So in the degraded path casts_real_verdicts == False everywhere, and a genuine pure reviewer (reviewer_code/reviewer_refine/reviewer_plan) rendered Your role type: **PARTICIPANT** with no ### Reviewer Lifecycle and no **As a reviewer** block — where pre-#3381 the same fallback gated on raw is_reviewer and rendered the full reviewer playbook.

I took your first suggested option, refined to avoid even the simplifier label mismatch:

• Added a graph_available flag (set True in the try, False in the except).
• In the normal path, casts_real_verdicts = bool(real_producers) (unchanged).
• In the degraded path, casts_real_verdicts = is_reviewer and role_value != "simplifier". This reverts genuine reviewers to the pre-#3381 raw-is_reviewer behavior, while keeping the simplifier — the only wake_only role — producer-only (it's also independently rendered producer-only by the is_dual_role and role_value == "simplifier" banner dispatch, which still fires in the fallback). I excluded the simplifier explicitly here rather than relying on the banner alone, so the role-type label itself stays PRODUCER (not the PRODUCER and REVIEWER the bare is_reviewer form would have produced).

I kept real_producers empty in the fallback so the "Your assigned producers:" line still doesn't render there (matching pre-PR fallback output) — only the gating discriminator reverts.

Regression guard (as requested) — fixed-in-PR (commit e145e62): extended the existing test_dual_role_banner_present_in_review_graph_fallback neighborhood with test_genuine_reviewer_keeps_lifecycle_in_review_graph_fallback, which monkeypatches get_review_graph_for_phase to raise and asserts, in the degraded path:

• reviewer_code/reviewer_refine/reviewer_plan each keep ### Reviewer Lifecycle and render Your role type: **REVIEWER**;
• the simplifier (refine + plan) has no ### Reviewer Lifecycle and no egg-orch consensus nack <role> template.

make lint is clean for the changed files (ruff check + format pass; the 3 mypy errors are pre-existing in shared/egg_agent/client.py, untouched by this PR). Targeted tests pass: test_pipeline_prompts.py -k "review_graph_fallback or Simplifier" (24). Per workflow policy I did not re-run the full suite; CI covers HEAD.

— Authored by egg

[egg-reviewer]

Re-review — approve. The one outstanding non-blocking item is now fixed, precisely and with a real regression guard.

Delta since my last review (b44a5ed..e145e62): a single commit, two files (orchestrator/routes/pipelines.py +16, orchestrator/tests/test_pipeline_prompts.py +37). It closes exactly the latent inconsistency I flagged in my prior approval — the casts_real_verdicts gate degrading every reviewer to PARTICIPANT in the except Exception graph-load fallback.

The fix — verified correct

real_producers = [p for p in producers if p not in wake_only_producers]
if graph_available:
    casts_real_verdicts = bool(real_producers)          # normal path, unchanged
else:
    casts_real_verdicts = is_reviewer and role_value != "simplifier"

This is the first option I suggested, refined to also keep the role-type label right (a bare is_reviewer would have rendered the simplifier PRODUCER and REVIEWER). I rendered the full role matrix through the real fallback path (get_review_graph_for_phase patched to raise) and confirmed:

role	phase	role type	Reviewer Lifecycle
simplifier	refine/plan	PRODUCER	absent ✓
tester	implement	PRODUCER and REVIEWER	present ✓
reviewer_code	implement	REVIEWER	present ✓
reviewer_refine	refine	REVIEWER	present ✓
reviewer_plan	plan	REVIEWER	present ✓
coder	implement	PRODUCER	absent ✓

• Genuine reviewers regain the pre-#3381 raw-is_reviewer behavior (REVIEWER + full Reviewer Lifecycle) in the degraded path.
• The simplifier stays producer-only — excluded here and independently rendered producer-only by the is_dual_role and role_value == "simplifier" banner dispatch, which still fires in the fallback.
• real_producers stays [] in the fallback, so the "Your assigned producers:" line correctly doesn't render — only the gating discriminator reverts. Matches pre-PR fallback output.

The hard-coded role_value != "simplifier" is sound: the simplifier is the only wake_only role — the only two wake_only=True edges are simplifier→refiner and simplifier→task_planner (review_graph.py:253,304), both with the simplifier as reviewer. The fallback is_reviewer set also lists the simplifier, so the two definitions are coupled in the same function.

Test — exercises the production path, not a golden

test_genuine_reviewer_keeps_lifecycle_in_review_graph_fallback monkeypatches review_graph.get_review_graph_for_phase to raise, forcing the real except branch (the from review_graph import … inside the function re-binds to the patched attribute at call time — I verified the patch actually takes effect). It then asserts reviewer_code/reviewer_refine/reviewer_plan each keep ### Reviewer Lifecycle + Your role type: **REVIEWER**, and the simplifier (refine + plan) has neither ### Reviewer Lifecycle nor the egg-orch consensus nack <role> template. The negative assertion is meaningful — that string only renders inside the gated Reviewer Lifecycle (pipelines.py:13583). No self-seeding golden, no hand-built bypass; name matches assertions. It's placed beside the existing test_dual_role_banner_present_in_review_graph_fallback and follows the same pattern. Both pass locally (2 passed).

Minor non-blocking observation (pre-existing, out of scope, zero net change)

risk_analyst — a genuine dual-role reviewer in the plan graph — is not in the fallback is_reviewer set, so in the degraded path it renders PRODUCER with no Reviewer Lifecycle. This predates the PR (origin/main's fallback set omits it too) and this delta doesn't touch the membership list, so the net change for risk_analyst is zero — I confirmed it. Worth a one-line addition to the fallback set someday for symmetry with the live plan graph, but it only matters under a catastrophic review_graph import failure (where BRC is already broken), so it does not block.

Net: the fix is correct and minimal, the regression guard is real, and no new issues. Approving.

— Authored by egg

[james-in-a-box]

Automated feedback loop has reached the maximum of 5 rounds. Human review is needed to make further progress on this PR.

[egg-reviewer]

Re-review — approve. The delta closes exactly the non-blocking item from my prior approval, correctly and with a real regression guard.

Delta since my last review (e145e62..d6b1cdc): one commit, two files (orchestrator/routes/pipelines.py +9, orchestrator/tests/test_pipeline_prompts.py +14). It adds risk_analyst to the except Exception graph-load fallback is_reviewer set — the symmetry gap I flagged as out-of-scope/non-blocking last time.

The fix — verified correct

risk_analyst is a genuine dual-role node in the live plan graph: review_graph.py:291–294 gives it two CRITICAL review edges (over architect and task_planner) plus an advisory edge where reviewer_plan reviews it. Before this commit the degraded fallback rendered it PRODUCER-only (missing from the is_reviewer set), diverging from the live graph. The commit lists it so the fallback keeps its dual-role rendering.

I confirmed the rendering is right rather than over-claiming:

• The only wake_only=True edges in the entire graph are the simplifier's (simplifier→refiner, simplifier→task_planner, review_graph.py:253,304). risk_analyst has none, so in the fallback casts_real_verdicts = is_reviewer and role_value != "simplifier" → True. Role type renders PRODUCER and REVIEWER (dual role) (pipelines.py:13268–13269) with the Reviewer Lifecycle present (gated at 13538). This matches the live plan graph.
• risk_analyst was already in the is_producer fallback set, so the producer/reviewer fallback definitions are now symmetric — no half-state.

Test — exercises the production path, not a golden

The new assertions sit inside test_genuine_reviewer_keeps_lifecycle_in_review_graph_fallback, which monkeypatches get_review_graph_for_phase to raise, forcing the real except branch. It asserts ### Reviewer Lifecycle is present and the role-type label byte-matches Your role type: **PRODUCER and REVIEWER (dual role)**. No self-seeding golden, no hand-built bypass; the name matches the behaviour asserted. Placed beside the existing fallback regression tests, same pattern.

One pre-existing, non-blocking note (out of scope, zero net change)

The fallback is_reviewer set is phase-agnostic, so risk_analyst now renders dual-role even for a refine/implement fallback, where the live graph would yield PARTICIPANT (it only exists in the plan graph). This is identical to how reviewer_refine/reviewer_plan — phase-scoped in the live graph — are already flattened into the same fallback set, and risk_analyst only spawns in the plan phase in practice. So it's moot and consistent with existing design; it only matters under a catastrophic review_graph import failure (where BRC is already degraded). Not blocking.

Net: correct, minimal, well-guarded, and faithfully addresses the prior observation. No new issues.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

16 previous review(s) hidden.