Add reviewer_code_holistic CRITICAL role alongside fan-out (#2126)

[jwbron]

Summary

• Adds a new reviewer_code_holistic BRC role that always single-passes the full diff in the implement phase, in parallel with reviewer_code's fan-out + slice subagents and the lens reviewers. Both feed consensus as distinct CRITICAL roles.
• Restores the holistic generalist scope that #2061 traded away: end-to-end primary-use-case walk, doc↔code symmetry, synthetic-key / sentinel cross-module audit, and silent-fallback hunt — exactly the failure shapes postmortem #2125 flagged on PR #2105 (the canonical __checkout__ dead-end).

Closes #2126.

What changed

• shared/egg_contracts/agent_roles.py — new REVIEWER_CODE_HOLISTIC enum value, REVIEWER_CODE_HOLISTIC_ROLE definition, AGENT_ROLES registry entry, Role.REVIEWER contract mapping, and roster slot in _PHASE_REVIEWERS["implement"] (applies to every repo, not egg-only).
• orchestrator/review_graph.py — two CRITICAL edges (reviewer_code_holistic → coder and reviewer_code_holistic → tester) added to get_default_implement_graph(). NACKs are tracked per (reviewer, producer) edge by the existing ApprovalMatrix, so distinct roles → no averaging falls out for free.
• orchestrator/routes/pipelines.py — new code-holistic branches in _get_reviewer_scope_preamble, _get_review_criteria_for_type (loads new criteria file), _build_reviewer_preparation (proactive prep text for both BABYSIT and standard implement), and a roster description. Fan-out remains gated to reviewer_type == "code" only, with an explicit guard + comment so future drift can't accidentally enable fan-out for the holistic role. Procedural-step / convention / NACK-vs-ACK blocks now apply to both code reviewers.
• shared/prompts/code-review-holistic-criteria.md — new criteria file. Inherits from code-review-criteria.md and adds the four holistic passes (end-to-end use case, doc↔code symmetry, synthetic-key audit, silent-fallback hunt) with explicit "don't verify every line; the fan-out reviewer covers that" framing.
• Tests:
  • orchestrator/tests/test_reviewer_code_holistic.py (new) — graph wiring, role registration, prompt-criteria differentiation, and an explicit guard that the fan-out block never appears for code-holistic.
  • orchestrator/tests/test_reviewer_code_fan_out_prompt.py — code-holistic added to the fan-out-block-absence parametrization.
  • tests/shared/egg_contracts/test_agent_roles.py — review-roles roster + count assertions updated.

Out of scope

Per the issue itself: this PR does not promote reviewer_security / reviewer_concurrency to CRITICAL (#1997 / #2062), fan out the lens reviewers (postmortem #2125 option A), or add a doc-correctness lens.

Test plan

• pytest orchestrator/tests/test_reviewer_code_holistic.py orchestrator/tests/test_reviewer_code_fan_out_prompt.py orchestrator/tests/test_review_graph_advisory_reviewers.py — 57 passed locally
• ruff check . and ruff format --check clean across the worktree
• CI runs full make test + make lint
• In-pipeline BRC consensus on a future implement-phase PR shows both reviewer_code and reviewer_code_holistic ACKing independently before convergence

🤖 Generated with Claude Code

[james-in-a-box]

Autofix tracking

{"Test/Unit Tests": 1, "Lint/Python": 1}

[egg-reviewer]

No agent-mode design concerns.

This PR adds a specialized reviewer (reviewer_code_holistic) with a focused four-pass lens — the guide explicitly endorses focused guidelines for specialized review bots ("A security review bot should have specific things to check—that's its purpose"). The criteria are the role's specialization, not over-specification of a generalist.

Quick checks against the anti-pattern list:

• No pre-fetching: the agent runs git diff <base>...HEAD itself; the prompt orients via PR/issue metadata only.
• No structured-output-for-humans: ACK/NACK with natural-language findings, no JSON schema.
• No post-processing pipeline: BRC consensus consumes the ACK/NACK directly.
• No prompt-level security or direct LLM/API calls.

The preparation text and "Verdict shape" guidance are orienting (telling the agent what passes to run and what to name in a NACK), not constraining what it can see or do. The explicit reviewer_type == "code" guard around the fan-out block is also good — it prevents holistic from accidentally inheriting the slice strategy.

— Authored by egg

[egg-reviewer]

Review of PR #2130 — reviewer_code_holistic CRITICAL role

I traced the new role end-to-end through spawn, prompt assembly, BRC consensus, and the registry/pattern surface. The plumbing is mostly correct — the dynamic role_value.replace("reviewer_", "", 1).replace("_", "-") mapping handles reviewer_code_holistic → code-holistic automatically, the _PHASE_REVIEWERS["implement"] entry covers babysit-pr (since has_contract=False only filters out reviewer_contract), and the explicit reviewer_type == "code" guards on the fan-out block prevent drift. Tests and registry updates are thorough.

I found one blocking integration gap and three non-blocking issues.

---

Blocking

1. REVIEWER_CODE_HOLISTIC is missing from _ROLES_WITHOUT_WORKTREE

File: orchestrator/kubernetes_spawner.py:187-199

Every other reviewer role — including REVIEWER_CODE, the original lens reviewers (REVIEWER_SECURITY, REVIEWER_CONCURRENCY added in #2061), and the contract/refine/plan/agent-design reviewers — is in this frozenset. REVIEWER_CODE_HOLISTIC is not, even though it is exactly the same kind of role as REVIEWER_CODE (BRC consensus reviewer that operates on a diff).

Consequences:

• A spawn with repos=[] will raise KubernetesSpawnError("Cannot spawn reviewer_code_holistic … no repos provided") — the failure mode _ROLES_WITHOUT_WORKTREE exists to prevent.
• A spawn with repos provided will provision a per-agent worktree, wasting infrastructure (the same waste the existing test test_lens_reviewers_in_roles_without_worktree was added to prevent for #2061).
• The fact that the conftest _skip_worktree_disk_check autouse fixture stubs _role_needs_worktree to False means this gap is invisible to the existing test suite — the only test that catches a missing entry is the lens-reviewer-specific assertion at orchestrator/tests/test_kubernetes_spawner.py:244, which doesn't cover the new role.

Fix:

_ROLES_WITHOUT_WORKTREE: frozenset[AgentRole] = frozenset(
    {
        AgentRole.REVIEWER_CODE,
        AgentRole.REVIEWER_CODE_HOLISTIC,  # add
        AgentRole.REVIEWER_CONTRACT,
        AgentRole.REVIEWER_AGENT_DESIGN,
        AgentRole.REVIEWER_REFINE,
        AgentRole.REVIEWER_PLAN,
        AgentRole.REVIEWER_SECURITY,
        AgentRole.REVIEWER_CONCURRENCY,
        AgentRole.OVERSEER,
        AgentRole.INSPECTOR,
    }
)

And add an assertion in orchestrator/tests/test_reviewer_code_holistic.py mirroring test_lens_reviewers_in_roles_without_worktree:

def test_in_roles_without_worktree(self) -> None:
    """Holistic reviewer must spawn without a per-agent worktree, like reviewer_code."""
    from kubernetes_spawner import _ROLES_WITHOUT_WORKTREE

    assert AgentRole.REVIEWER_CODE_HOLISTIC in _ROLES_WITHOUT_WORKTREE

---

Non-blocking

2. Procedural-step prompt contradicts the holistic mission

File: orchestrator/routes/pipelines.py:4695-4728

The PR widens the procedural-steps gate from reviewer_type == "code" to reviewer_type in ("code", "code-holistic"), which means code-holistic now receives this line:

"2. Get the full diff and review every changed file systematically"

That directly contradicts the scope preamble (_get_reviewer_scope_preamble) which says "Don't verify every line; the fan-out reviewer covers that." and the new criteria file's prominent "Don't verify every line. The fan-out reviewer reads each file carefully. Re-doing that is waste — and it pulls your attention away from the cross-module questions only you are asked to answer."

The model will see both messages in the same prompt. Either the procedural steps should be tailored for code-holistic (drop step 2's "review every changed file systematically" wording, replace step 8 "Evaluate against the criteria below" with explicit references to the four passes), or the holistic reviewer should skip the entire procedural-steps block and let the criteria file structure the work. The current bundling defeats the lens distinction the PR is built around.

3. No attestation model for reviewer_code_holistic

File: orchestrator/attestation_schemas.py:98-102

REVIEWER_ATTESTATION_MODELS doesn't have an entry for reviewer_code_holistic. This matches the pattern for reviewer_security / reviewer_concurrency (both intentionally absent per test_pipeline_role_to_reviewer_type_mapping.py:142-163), and validate_attestation only fires when review.attestation is truthy — so the default empty-attestation path works.

But: reviewer_code does have a ReviewerCodeAttestation model with files_reviewed, issues_found, issues_resolved. If an agent following the holistic prompt mirrors reviewer_code's ACK shape and includes an attestation dict, validation will raise ValueError: No attestation schema for role 'reviewer_code_holistic'. Worth either (a) adding a parallel ReviewerCodeHolisticAttestation (e.g., passes_run, findings_per_pass) and registering it, or (b) adding an explicit guard test like the lens reviewers have so future contributors know the absence is intentional.

4. COMPOSITE_REVIEWER_ROLES set is incomplete

File: shared/egg_contracts/checkpoint_cli.py:74-82

reviewer_code_holistic isn't in the COMPOSITE_REVIEWER_ROLES frozenset that drives egg-checkpoint --agent-type choices. Same status as reviewer_security / reviewer_concurrency (also missing), so this is consistent with existing drift rather than a regression — but it means operators can't filter checkpoints by the new role. Worth backfilling all three composite roles in a follow-up.

---

What I checked and found correct

• Review-graph wiring (two CRITICAL edges, distinct from reviewer_code so NACKs don't average via ApprovalMatrix).
• Role registration in AgentRole enum, AGENT_ROLES, _PHASE_REVIEWERS["implement"], and Role.REVIEWER contract mapping. Applies to non-egg repos (no EGG_ONLY_REVIEWERS membership).
• File-access patterns in shared/egg_restrictions/patterns.py — same allow/block list as REVIEWER_CODE_PATTERNS, which is correct (no contracts write access).
• reviewer_type mapping is the dynamic one-liner — no redundant if/elif chain.
• Fan-out block explicitly guarded if reviewer_type == "code" with an in-line comment about why (clean defense against future drift).
• BRC roster preamble (_build_brc_preamble) and roster-description dict include the new role.
• Babysit-pr roster picks it up automatically through get_roles_for_phase("implement", has_contract=False) since has_contract=False only filters reviewer_contract.
• Both PR-aware (_is_pr_diff_aware) and non-PR implement-phase preparation branches have explicit reviewer_code_holistic text with the "skim once, defer line-by-line" framing.
• Criteria file loader (_get_code_review_holistic_criteria) uses the same _read_shared_criteria pattern as the other criteria functions, with a sane inline fallback.
• Test count assertions updated consistently (18 → 19) in three test files.

Once issue #1 is fixed (and ideally #2 addressed), this is good to merge.

— Authored by egg

[james-in-a-box]

Thanks for the thorough review. Addressed all four issues in 8ad8c49:

1. (Blocking) REVIEWER_CODE_HOLISTIC added to _ROLES_WITHOUT_WORKTREE (orchestrator/kubernetes_spawner.py:190). Added a mirror of test_lens_reviewers_in_roles_without_worktree in test_reviewer_code_holistic.py::TestRoleRegistration::test_in_roles_without_worktree.

2. Procedural-step prompt tailored by lens (orchestrator/routes/pipelines.py:4695-4738). Step 2 and step 8 now branch on reviewer_type:

• code-holistic step 2: "Skim the full diff once to build a mental map … do not re-verify every line; that is the fan-out reviewer's job"
• code-holistic step 8: names the four mandatory passes (end-to-end primary use case, doc ↔ code symmetry, synthetic-key / sentinel coordination, silent-fallback hunt)
• code keeps the original "review every changed file systematically" / "Evaluate against the criteria below" wording.

I kept the rest of the procedural scaffold (steps 3-7, 9, plus the "Find ALL issues on the first pass" line) shared because they apply to both lenses without contradiction. Added two regression tests pinning the holistic prompt does not regress to the slice-style step-2 wording and does name the four passes.

3. Guard test added for reviewer_code_holistic having no attestation model (orchestrator/tests/test_pipeline_role_to_reviewer_type_mapping.py::TestNoAttestationModelsForNewLensReviewers::test_reviewer_code_holistic_has_no_attestation_model). Matches the existing security/concurrency guard pattern and explicitly notes that ReviewerCodeAttestation is the wrong shape for holistic — if a schema is wanted later, it should be designed around the four passes rather than copying reviewer_code's slice fields.

4. reviewer_code_holistic added to COMPOSITE_REVIEWER_ROLES (shared/egg_contracts/checkpoint_cli.py:78). I left reviewer_security / reviewer_concurrency for the follow-up you mentioned — backfilling those is a larger sweep across the same set and not strictly tied to the holistic role.

Local: 148 targeted tests pass; ruff check + format clean.

— Authored by egg

[egg-reviewer]

No agent-mode design concerns on the delta since 5f4d79c.

The single new commit (8ad8c49) addresses the four general-review issues without changing the agent-mode shape:

• Spawn-time worktree (kubernetes_spawner.py) and checkpoint-CLI filter (checkpoint_cli.py) — infrastructure plumbing, no prompt impact.
• Procedural-step lens differentiation (pipelines.py:4688-4737) — step 2 now tells the holistic reviewer to "skim the full diff once" instead of "review every changed file systematically", and step 8 names the four mandatory passes. This stays on the right side of the orient-vs-constrain line: it specializes the prompt for a specialized role (which the guide explicitly endorses for review bots) and removes the contradiction between the procedural scaffold and the criteria file. No pre-fetching, no structured-output-for-humans, no post-processing pipeline.
• Attestation guard test — pins intentional absence; no behavior change.

Re-checked the anti-pattern list against the new diff: clean on all eight items. Approving.

— Authored by egg

[egg-reviewer]

Re-review of PR #2130 — delta 5f4d79c..8ad8c49

All four issues from the previous review are addressed correctly. Verified against the current branch state.

Issue 1 (Blocking) — _ROLES_WITHOUT_WORKTREE membership ✅

AgentRole.REVIEWER_CODE_HOLISTIC is now in the frozenset at orchestrator/kubernetes_spawner.py:190, alphabetically slotted next to REVIEWER_CODE. The mirror regression test TestRoleRegistration::test_in_roles_without_worktree at orchestrator/tests/test_reviewer_code_holistic.py:115-133 follows the exact pattern of test_lens_reviewers_in_roles_without_worktree. The KubernetesSpawnError("Cannot spawn reviewer_code_holistic …") failure mode is now blocked.

Issue 2 — Procedural-step / criteria contradiction ✅

orchestrator/routes/pipelines.py:4690-4734 now branches both step 2 and step 8 on reviewer_type:

• Step 2 / holistic: "Skim the full diff once … do not re-verify every line; that is the fan-out reviewer's job" — directly aligned with the criteria's "don't verify every line".
• Step 2 / code: original "review every changed file systematically" wording preserved.
• Step 8 / holistic: names the four mandatory passes inline (end-to-end primary use case, doc ↔ code symmetry, synthetic-key / sentinel coordination, silent-fallback hunt).
• Step 8 / code: original "Evaluate against the criteria below" wording preserved.

The shared steps 3-7 (read context, trace data flow, verify end-to-end, research, edge cases) apply cleanly to both lenses without contradicting either criteria block. Two regression tests pin the holistic prompt against drift back to the slice-style wording (test_procedural_step_does_not_demand_every_file_review, test_procedural_step_references_holistic_passes).

Issue 3 — Attestation-model guard ✅

test_reviewer_code_holistic_has_no_attestation_model at orchestrator/tests/test_pipeline_role_to_reviewer_type_mapping.py:165-188 mirrors the existing security/concurrency guards and explicitly explains why registering ReviewerCodeAttestation for the holistic role would be wrong (the slice schema's files_reviewed/issues_found fields are the wrong shape for the four-pass holistic ACK). The docstring also points future contributors at the right design (ReviewerCodeHolisticAttestation with passes_run/findings_per_pass) if a schema is ever needed.

Issue 4 — COMPOSITE_REVIEWER_ROLES backfill ✅

reviewer_code_holistic added at shared/egg_contracts/checkpoint_cli.py:77. Operators can now filter egg-checkpoint --agent-type reviewer_code_holistic. Author's deferral of reviewer_security/reviewer_concurrency to a follow-up is reasonable — those are pre-existing drift, not regressions from this PR.

Verification

Ran the full holistic + spawner + mapping suites locally:

• orchestrator/tests/test_reviewer_code_holistic.py: 18/18 passed (3 new tests included)
• orchestrator/tests/test_pipeline_role_to_reviewer_type_mapping.py + test_kubernetes_spawner.py: 88/88 passed
• The new procedural-step branching is structurally sound; the four-pass terminology in step 8 matches the criteria-file headers exactly.

No new issues found in the delta.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

8 previous review(s) hidden.