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

orchestrator/kubernetes_spawner.py

@@ -187,6 +187,7 @@ def _classify_spawn_error(e: BaseException | None) -> str | None:
 _ROLES_WITHOUT_WORKTREE: frozenset[AgentRole] = frozenset(
     {
         AgentRole.REVIEWER_CODE,
+        AgentRole.REVIEWER_CODE_HOLISTIC,
         AgentRole.REVIEWER_CONTRACT,
         AgentRole.REVIEWER_AGENT_DESIGN,
         AgentRole.REVIEWER_REFINE,

orchestrator/review_graph.py

@@ -217,6 +217,9 @@ def get_default_implement_graph() -> ReviewGraph:
 
     Review adjacency per the BRC spec:
     - reviewer_code reviews coder and tester (critical)
+    - reviewer_code_holistic reviews coder and tester (critical) — issue
+      #2126: distinct CRITICAL role so a holistic NACK on architectural
+      coherence is not averaged with the fan-out reviewer's slice ACKs.
     - reviewer_contract reviews coder (critical)
     - tester reviews coder (critical, implicitly via tests and lint/type-checks)
     - reviewer_security reviews coder and tester (advisory) — lens reviewer
@@ -228,15 +231,20 @@ def get_default_implement_graph() -> ReviewGraph:
     issue #1997. Promotion to CRITICAL is intentionally deferred.
 
     Producers: coder, tester, documenter
-    Reviewers: reviewer_code, reviewer_contract, tester (dual-role),
-    reviewer_security (advisory), reviewer_concurrency (advisory)
+    Reviewers: reviewer_code, reviewer_code_holistic, reviewer_contract,
+    tester (dual-role), reviewer_security (advisory),
+    reviewer_concurrency (advisory)
     """
     return ReviewGraph(
         [
             # reviewer_code reviews coder (critical)
             ReviewEdge("reviewer_code", "coder", ReviewCriticality.CRITICAL),
             # reviewer_code reviews tester (critical)
             ReviewEdge("reviewer_code", "tester", ReviewCriticality.CRITICAL),
+            # reviewer_code_holistic reviews coder (critical — issue #2126)
+            ReviewEdge("reviewer_code_holistic", "coder", ReviewCriticality.CRITICAL),
+            # reviewer_code_holistic reviews tester (critical — issue #2126)
+            ReviewEdge("reviewer_code_holistic", "tester", ReviewCriticality.CRITICAL),
             # reviewer_contract reviews coder (critical)
             ReviewEdge("reviewer_contract", "coder", ReviewCriticality.CRITICAL),
             # tester reviews coder (critical — via writing/running tests and lint/type-checks)

orchestrator/routes/pipelines.py

@@ -3499,6 +3499,38 @@ def _get_security_review_criteria(repo_path: str | None = None) -> str:
     )
 
 
+def _get_code_review_holistic_criteria(repo_path: str | None = None) -> str:
+    """Return holistic-lens review criteria (issue #2126).
+
+    The shared file inherits from ``code-review-criteria.md`` and adds
+    holistic-lens rules (end-to-end use-case walk, doc↔code symmetry,
+    synthetic-key / sentinel cross-module audit, silent-fallback hunt).
+    """
+    content = _read_shared_criteria(
+        "code-review-holistic-criteria.md",
+        user_override="code-review-holistic-rules.md",
+        repo_path=repo_path,
+    )
+    if content is not None:
+        return content
+    logger.warning("Shared code-review-holistic-criteria.md not found, using inline fallback")
+    return (
+        "Inherits from `code-review-criteria.md`; only holistic-lens rules "
+        "below override or extend it.\n\n"
+        "### Holistic lens (focus areas)\n"
+        "- Walk the primary advertised use case end-to-end across the "
+        "full diff. NACK silent dead-ends like the `__checkout__` bug "
+        "on PR #2105.\n"
+        "- Cross-check doc-claimed behaviour against what the code does. "
+        "NACK doc-claimed inference / migration paths that do not exist.\n"
+        "- Audit synthetic keys, sentinels, and magic values for "
+        "cross-module agreement.\n"
+        "- Hunt silent fallbacks that swallow operator-visible "
+        "misconfiguration.\n"
+        "- Defer line-by-line correctness to `reviewer_code`'s fan-out.\n"
+    )
+
+
 def _get_concurrency_review_criteria(repo_path: str | None = None) -> str:
     """Return concurrency-lens review criteria (issue #1965).
 
@@ -3536,6 +3568,8 @@ def _get_review_criteria_for_type(
         return _get_agent_design_criteria()
     elif reviewer_type == "code":
         return _get_code_review_criteria(repo_path=repo_path)
+    elif reviewer_type == "code-holistic":
+        return _get_code_review_holistic_criteria(repo_path=repo_path)
     elif reviewer_type == "contract":
         return _get_contract_review_criteria(repo_path=repo_path)
     elif reviewer_type == "refine":
@@ -3573,6 +3607,34 @@ def _get_reviewer_scope_preamble(reviewer_type: str, phase: str) -> str:
             "file. For each file, note what changed, whether the change is correct, "
             "and any issues or observations."
         )
+    elif reviewer_type == "code-holistic":
+        return (
+            "This is a CRITICAL **holistic code review** (issue #2126). "
+            "You run alongside `reviewer_code`'s slice-by-slice fan-out — "
+            "your job is the cross-module coherence question no slice "
+            "owns. **Don't verify every line; the fan-out reviewer covers "
+            "that.**\n\n"
+            "**Lens scope:** read the diff once with the whole PR in mind, "
+            "then run all four passes from the criteria below: (1) walk "
+            "the primary advertised use case end-to-end (the `__checkout__` "
+            "dead-end on PR #2105 is the canonical miss); (2) check that "
+            "every doc-claimed behaviour is actually implemented and every "
+            "user-facing code path is documented; (3) confirm synthetic "
+            "keys / sentinels / magic values are recognised by every "
+            "consumer in another module; (4) hunt silent fallbacks "
+            "(`except Exception:`, swallowed `None`s, default no-op "
+            "branches) where the operator would expect a signal.\n\n"
+            "**Distinct CRITICAL role.** Your NACK gates consensus on its "
+            "own — it is not averaged against the fan-out reviewer's "
+            "slice ACKs. If the architectural-coherence question fails, "
+            "NACK even when every slice is internally consistent.\n\n"
+            "**Analysis format:** Name the pass that found the issue, the "
+            "producer / consumer modules the asymmetry spans, and the "
+            "user-visible failure shape. If all four passes come back "
+            "clean a concise ACK is acceptable, but the BRC bus enforces "
+            "a minimum content length on ACK / NACK bodies, so write at "
+            "least a sentence or two summarising what you checked."
+        )
     elif reviewer_type == "contract":
         return (
             "This is a **contract verification review**. Verify that the implementation "
@@ -4625,9 +4687,26 @@ def _build_review_prompt(
             f"1. Review the implementation using `git log --oneline -10` and `{diff_command}`"
         )
 
-    # Add procedural steps for code reviewer (matching GHA reviewer thoroughness)
-    if reviewer_type == "code" and not draft_path:
-        lines.append("2. Get the full diff and **review every changed file systematically**")
+    # Add procedural steps for code reviewers (matching GHA reviewer thoroughness).
+    # Both ``code`` and ``code-holistic`` get the same numbered procedural-step
+    # scaffold, but steps 2 and 8 differ by lens: ``code`` reviews every file
+    # systematically and evaluates against the slice criteria, while
+    # ``code-holistic`` skims the diff once and runs the four cross-module
+    # passes from the holistic criteria file. The fan-out section (further
+    # below) is gated to ``code`` only. See issue #2126 — the prior unified
+    # wording told the holistic reviewer to "review every changed file
+    # systematically", which directly contradicted the holistic criteria's
+    # "don't verify every line; the fan-out reviewer covers that".
+    if reviewer_type in ("code", "code-holistic") and not draft_path:
+        if reviewer_type == "code-holistic":
+            lines.append(
+                "2. **Skim the full diff once** to build a mental map of "
+                "what the PR adds, who the user is, and what the user's "
+                "primary path through the change looks like — do not "
+                "re-verify every line; that is the fan-out reviewer's job"
+            )
+        else:
+            lines.append("2. Get the full diff and **review every changed file systematically**")
         lines.append(
             "3. Read surrounding context — check how changed code integrates with the rest of the codebase"
         )
@@ -4645,7 +4724,14 @@ def _build_review_prompt(
             "API usage patterns, and confirm the code follows current best practices"
         )
         lines.append("7. Consider edge cases the author may not have tested")
-        lines.append("8. Evaluate against the criteria below")
+        if reviewer_type == "code-holistic":
+            lines.append(
+                "8. Run the four mandatory passes from the criteria below "
+                "(end-to-end primary use case, doc ↔ code symmetry, "
+                "synthetic-key / sentinel coordination, silent-fallback hunt)"
+            )
+        else:
+            lines.append("8. Evaluate against the criteria below")
         if concurrent:
             lines.append(
                 "9. Deliver your full review via ACK/NACK (see BRC protocol below). "
@@ -4669,7 +4755,11 @@ def _build_review_prompt(
         # `git log A..HEAD --not origin/<base> -p` command is small by
         # construction and the parent's cross-partition pass would
         # contradict the delta-only directive above.
-        if phase == "implement" and not is_delta_review:
+        # Issue #2126: ``code-holistic`` also enters the procedural-steps
+        # branch above but MUST NOT receive the fan-out block — it always
+        # single-passes the full diff. The explicit ``reviewer_type ==
+        # "code"`` guard here keeps that invariant from drifting.
+        if reviewer_type == "code" and phase == "implement" and not is_delta_review:
             _parallel_word = "in parallel" if reviewer_code_parallel else "sequentially"
             lines.append("")
             lines.append("## Subagent Fan-Out Strategy\n")
@@ -4834,7 +4924,7 @@ def _build_review_prompt(
 
     # Review conventions — quality standards aligned with PR reviewer thoroughness
     lines.append("## Review Conventions\n")
-    if reviewer_type == "code":
+    if reviewer_type in ("code", "code-holistic"):
         lines.append(
             "You are a critical part of the engineering infrastructure — the last line "
             "of defense before code reaches production. Your review must meet these "
@@ -4867,7 +4957,7 @@ def _build_review_prompt(
     # Verdict classification — only for code reviewers (aligned with review-conventions.md)
     # Non-code reviewers get appropriate guidance from their type-specific criteria
     # (e.g., _get_plan_review_criteria() already says "flag as needs_revision")
-    if reviewer_type == "code":
+    if reviewer_type in ("code", "code-holistic"):
         _nack_label = "NACK" if concurrent else "`needs_revision`"
         _ack_label = "ACK" if concurrent else "`approved`"
         lines.append(f"### When to {_nack_label} vs {_ack_label}\n")
@@ -7908,6 +7998,7 @@ def _build_brc_preamble(
         )
         is_reviewer = role_value in (
             "reviewer_code",
+            "reviewer_code_holistic",
             "reviewer_contract",
             "tester",
             "reviewer_refine",
@@ -8190,6 +8281,12 @@ def _build_brc_preamble(
         "Reviews code quality, correctness, and security",
         "ACK/NACK with file-level feedback",
     ),
+    "reviewer_code_holistic": (
+        "Holistic single-pass review for cross-module coherence "
+        "(use-case end-to-end, doc↔code symmetry, synthetic-key audit, "
+        "silent-fallback hunt)",
+        "ACK/NACK with cross-module findings",
+    ),
     "reviewer_contract": (
         "Verifies implementation matches contract/requirements",
         "ACK/NACK with task-level verification",
@@ -8292,6 +8389,26 @@ def _build_reviewer_preparation(
                 "`tests_execution_blocked`: `tests_execution_blocked: true` is a "
                 "blocking concern unless clearly documented."
             )
+        if role_value == "reviewer_code_holistic":
+            return (
+                f"You are the holistic reviewer on an existing PR ({pr_hint}). "
+                f"(0) Check out the PR head: `{pr_checkout}` (required — "
+                "without this your worktree is on the base branch and the "
+                "diff below will be empty). "
+                "(1) **Skim the full diff once** at "
+                f"`git fetch origin && git diff {base_ref}...HEAD` to build "
+                "a mental map. Do not verify line-by-line — that is "
+                "`reviewer_code`'s slice work. "
+                "(a) Note the PR's stated intent (issue / description) — "
+                "this is the use case you will walk end-to-end. "
+                "(b) Identify the producer / consumer module pairs the diff "
+                "touches; you will audit them for synthetic-key and "
+                "silent-fallback asymmetries. "
+                "(c) Pull every doc claim into a checklist; you will grep "
+                "for code that implements each. "
+                "(d) Draft your ACK/NACK around the four passes (use case, "
+                "doc symmetry, synthetic keys, silent fallbacks)."
+            )
         if role_value == "tester":
             return (
                 f"You are reviewing an existing pull request ({pr_hint}). "
@@ -8331,6 +8448,25 @@ def _build_reviewer_preparation(
                 "Also scrutinize low `tests_run` counts relative to change scope — "
                 "a multi-file change with only 1 test run warrants investigation."
             )
+        elif role_value == "reviewer_code_holistic":
+            return (
+                "Start preparing immediately — do not wait idle for proposals. "
+                "(a) Read the contract with `egg-contract show` to extract "
+                "the primary advertised use case (this is the path you will "
+                "walk end-to-end once the producer proposes). "
+                "(b) Review the issue / PR description and any doc files "
+                "the contract names — collect the doc-claimed behaviours "
+                "into a checklist for the symmetry pass. "
+                "(c) Identify the producer / consumer module pairs the plan "
+                "touches; these are where synthetic-key and silent-fallback "
+                "asymmetries hide. "
+                "(d) Once commits land "
+                f"(`git fetch origin && git log --oneline {base_ref}..origin/{branch or '$(git branch --show-current)'}`), "
+                f"skim `git diff {base_ref}...HEAD` once with the whole PR "
+                "in mind — do not verify line-by-line; defer that to the "
+                "fan-out reviewer. Your job is the architectural-coherence "
+                "question no slice owns."
+            )
         elif role_value == "reviewer_contract":
             return (
                 "While waiting for proposals, prepare by: "

orchestrator/tests/test_models.py

@@ -634,6 +634,7 @@ def test_all_roles(self):
         assert AgentRole.REFINER in roles
         assert AgentRole.INSPECTOR in roles
         assert AgentRole.REVIEWER_CODE in roles
+        assert AgentRole.REVIEWER_CODE_HOLISTIC in roles
         assert AgentRole.REVIEWER_CONTRACT in roles
         assert AgentRole.REVIEWER_AGENT_DESIGN in roles
         assert AgentRole.REVIEWER_REFINE in roles
@@ -643,7 +644,7 @@ def test_all_roles(self):
         assert AgentRole.OVERSEER in roles
         assert AgentRole.AUTOFIXER in roles
         assert AgentRole.CONFLICT_RESOLVER in roles
-        assert len(roles) == 18
+        assert len(roles) == 19
 
 
 class TestBackwardCompatibility:

orchestrator/tests/test_peer_consensus_integration.py

@@ -727,7 +727,7 @@ def test_context_change_nack_not_escalated(self):
         assert r2["needs_escalation"] is False
 
     def test_full_implement_graph(self):
-        """Use the default implement graph (7 roles) and run a full
+        """Use the default implement graph (8 roles) and run a full
         propose/review/re-propose cycle to verify no invalidation bugs."""
         graph = get_default_implement_graph()
         t = PeerConsensusTracker("test-full", graph, cooldown_seconds=0)
@@ -756,6 +756,11 @@ def test_full_implement_graph(self):
 
         # All reviewers ACK coder
         t.handle_ack("reviewer_code", "coder", {"artifact_references": ["src/main.py"]})
+        t.handle_ack(
+            "reviewer_code_holistic",
+            "coder",
+            {"artifact_references": ["src/main.py", "src/utils.py"]},
+        )
         t.handle_ack(
             "reviewer_contract", "coder", {"artifact_references": ["src/main.py", "src/utils.py"]}
         )
@@ -766,6 +771,11 @@ def test_full_implement_graph(self):
         t.handle_ack("reviewer_code", "tester", {"artifact_references": ["tests/test_main.py"]})
         t.handle_ack("reviewer_code", "documenter", {"artifact_references": ["docs/README.md"]})
 
+        # reviewer_code_holistic ACKs tester (CRITICAL edge — issue #2126)
+        t.handle_ack(
+            "reviewer_code_holistic", "tester", {"artifact_references": ["tests/test_main.py"]}
+        )
+
         # Lens reviewers (advisory) ACK coder and tester
         t.handle_ack(
             "reviewer_security", "coder", {"artifact_references": ["src/main.py", "src/utils.py"]}
@@ -819,6 +829,12 @@ def test_full_implement_graph(self):
         t.handle_ack(
             "reviewer_code", "coder", {"artifact_references": ["src/main.py", "src/utils.py"]}
         )
+        # reviewer_code_holistic ACKed utils.py — invalidated, needs to re-ACK
+        t.handle_ack(
+            "reviewer_code_holistic",
+            "coder",
+            {"artifact_references": ["src/main.py", "src/utils.py"]},
+        )
         # reviewer_contract re-reviews and ACKs (invalidated, needs to re-ACK)
         t.handle_ack(
             "reviewer_contract", "coder", {"artifact_references": ["src/main.py", "src/utils.py"]}
@@ -844,6 +860,7 @@ def test_full_implement_graph(self):
             "tester",
             "documenter",
             "reviewer_code",
+            "reviewer_code_holistic",
             "reviewer_contract",
             "reviewer_security",
             "reviewer_concurrency",

orchestrator/tests/test_pipeline_role_to_reviewer_type_mapping.py

@@ -162,6 +162,31 @@ def test_reviewer_concurrency_has_no_attestation_model(self) -> None:
             "for rationale."
         )
 
+    def test_reviewer_code_holistic_has_no_attestation_model(self) -> None:
+        """Issue #2126: ``reviewer_code_holistic`` intentionally has no model.
+
+        The holistic reviewer follows the same attestation-less pattern as
+        the lens reviewers (``reviewer_security`` / ``reviewer_concurrency``)
+        and the other generalists (``reviewer_plan``, ``reviewer_refine``,
+        ``reviewer_agent_design``). The default empty-attestation path
+        works because ``validate_attestation`` only fires when
+        ``review.attestation`` is truthy. This explicit guard prevents a
+        future contributor from adding a partial ``ReviewerCodeAttestation``
+        copy without designing a holistic-specific schema first (e.g.
+        passes_run, findings_per_pass).
+        """
+        from attestation_schemas import REVIEWER_ATTESTATION_MODELS
+
+        assert "reviewer_code_holistic" not in REVIEWER_ATTESTATION_MODELS, (
+            "Pitfall-4: reviewer_code_holistic must NOT have an attestation "
+            "model. The holistic ACK shape intentionally diverges from "
+            "reviewer_code's slice attestation (files_reviewed / "
+            "issues_found) — silently registering ReviewerCodeAttestation "
+            "for the holistic role would force the wrong schema. If a "
+            "schema is needed, design a ReviewerCodeHolisticAttestation "
+            "around the four holistic passes and update this guard."
+        )
+
     def test_existing_attestation_models_unchanged(self) -> None:
         """Sanity check that the existing models stay registered.