diff --git a/orchestrator/kubernetes_spawner.py b/orchestrator/kubernetes_spawner.py index e4539b639c..577854d86c 100644 --- a/orchestrator/kubernetes_spawner.py +++ b/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, diff --git a/orchestrator/review_graph.py b/orchestrator/review_graph.py index cf339012e5..bf4ccc86f2 100644 --- a/orchestrator/review_graph.py +++ b/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,8 +231,9 @@ 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( [ @@ -237,6 +241,10 @@ def get_default_implement_graph() -> ReviewGraph: 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) diff --git a/orchestrator/routes/pipelines.py b/orchestrator/routes/pipelines.py index 702172c3e0..44587526cf 100644 --- a/orchestrator/routes/pipelines.py +++ b/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/ -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: " diff --git a/orchestrator/tests/test_models.py b/orchestrator/tests/test_models.py index b04db26676..61257cb569 100644 --- a/orchestrator/tests/test_models.py +++ b/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: diff --git a/orchestrator/tests/test_peer_consensus_integration.py b/orchestrator/tests/test_peer_consensus_integration.py index c5932c368a..c8f708ca6d 100644 --- a/orchestrator/tests/test_peer_consensus_integration.py +++ b/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", diff --git a/orchestrator/tests/test_pipeline_role_to_reviewer_type_mapping.py b/orchestrator/tests/test_pipeline_role_to_reviewer_type_mapping.py index e4dbd9b0d4..3413e35b4b 100644 --- a/orchestrator/tests/test_pipeline_role_to_reviewer_type_mapping.py +++ b/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. diff --git a/orchestrator/tests/test_reviewer_code_fan_out_prompt.py b/orchestrator/tests/test_reviewer_code_fan_out_prompt.py index b334d2492d..e25d113c34 100644 --- a/orchestrator/tests/test_reviewer_code_fan_out_prompt.py +++ b/orchestrator/tests/test_reviewer_code_fan_out_prompt.py @@ -55,7 +55,7 @@ def test_present_for_code_reviewer_in_implement_phase(self) -> None: @pytest.mark.parametrize( "reviewer_type", - ["contract", "agent-design", "refine", "plan"], + ["contract", "agent-design", "refine", "plan", "code-holistic"], ) def test_absent_for_non_code_reviewer_types(self, reviewer_type: str) -> None: # Each non-code type uses an appropriate phase for that reviewer. @@ -64,6 +64,10 @@ def test_absent_for_non_code_reviewer_types(self, reviewer_type: str) -> None: "agent-design": "refine", "refine": "refine", "plan": "plan", + # code-holistic runs in implement alongside reviewer_code but + # MUST NOT receive the fan-out block — it always single-passes + # the full diff (issue #2126). + "code-holistic": "implement", }[reviewer_type] prompt = _build_review_prompt( phase=phase_for_type, diff --git a/orchestrator/tests/test_reviewer_code_holistic.py b/orchestrator/tests/test_reviewer_code_holistic.py new file mode 100644 index 0000000000..553ac24c86 --- /dev/null +++ b/orchestrator/tests/test_reviewer_code_holistic.py @@ -0,0 +1,214 @@ +"""Always-on guards for the ``reviewer_code_holistic`` BRC role (issue #2126). + +The holistic reviewer is the always-on generalist counterpart to +``reviewer_code``. It must: + +1. Be registered alongside ``reviewer_code`` in the implement-phase + review graph as a *distinct CRITICAL* role so its NACKs are not + averaged with the fan-out reviewer's slice ACKs. +2. Run on every implement pipeline (no fan-out gate, no PR-size gate). +3. Use a holistic-lens prompt (not the fan-out / line-by-line code + review criteria). + +These asserts are deterministic — they do not run the LLM. +""" + +from __future__ import annotations + +import sys +from unittest.mock import MagicMock + +# Stub Docker the same way the other prompt tests do. +_docker_mock = MagicMock() +sys.modules.setdefault("docker", _docker_mock) +sys.modules.setdefault("docker.errors", _docker_mock.errors) +sys.modules.setdefault("docker.types", _docker_mock.types) + +from review_graph import ( # noqa: E402 + ReviewCriticality, + get_default_implement_graph, +) +from routes.pipelines import _build_review_prompt # noqa: E402 + +# --------------------------------------------------------------------------- +# Review-graph wiring. +# --------------------------------------------------------------------------- + + +class TestImplementGraphWiring: + """Both holistic edges must exist as CRITICAL.""" + + def setup_method(self) -> None: + self.graph = get_default_implement_graph() + + def test_reviews_coder_critical(self) -> None: + edge = self.graph.get_edge("reviewer_code_holistic", "coder") + assert edge is not None, "reviewer_code_holistic → coder edge missing" + assert edge.criticality is ReviewCriticality.CRITICAL, ( + "reviewer_code_holistic → coder must be CRITICAL so a holistic " + "NACK gates consensus on its own (issue #2126)." + ) + + def test_reviews_tester_critical(self) -> None: + edge = self.graph.get_edge("reviewer_code_holistic", "tester") + assert edge is not None, "reviewer_code_holistic → tester edge missing" + assert edge.criticality is ReviewCriticality.CRITICAL + + def test_distinct_from_reviewer_code(self) -> None: + """Both reviewers exist as separate CRITICAL edges so NACKs don't average.""" + code_coder = self.graph.get_edge("reviewer_code", "coder") + holistic_coder = self.graph.get_edge("reviewer_code_holistic", "coder") + assert code_coder is not None + assert holistic_coder is not None + assert code_coder.reviewer_role != holistic_coder.reviewer_role, ( + "reviewer_code and reviewer_code_holistic must be distinct roles " + "in the review graph — merging them defeats the issue #2126 goal." + ) + + def test_listed_in_critical_reviewers_for_coder(self) -> None: + critical = self.graph.critical_reviewers_for("coder") + assert "reviewer_code_holistic" in critical + assert "reviewer_code" in critical + + def test_listed_in_critical_reviewers_for_tester(self) -> None: + critical = self.graph.critical_reviewers_for("tester") + assert "reviewer_code_holistic" in critical + + +# --------------------------------------------------------------------------- +# Phase roster + role registration. +# --------------------------------------------------------------------------- + + +class TestRoleRegistration: + """The role must be in the canonical registry and the implement roster.""" + + def test_in_agent_role_enum(self) -> None: + from egg_contracts.agent_roles import AgentRole + + assert AgentRole("reviewer_code_holistic") is AgentRole.REVIEWER_CODE_HOLISTIC + + def test_in_agent_roles_registry(self) -> None: + from egg_contracts.agent_roles import AGENT_ROLES, AgentRole + + assert AgentRole.REVIEWER_CODE_HOLISTIC in AGENT_ROLES + + def test_in_implement_phase_roster(self) -> None: + from egg_contracts.agent_roles import AgentRole, get_roles_for_phase + + roles = get_roles_for_phase("implement") + assert AgentRole.REVIEWER_CODE_HOLISTIC in roles + + def test_applies_to_non_egg_repos(self) -> None: + """The holistic reviewer is not egg-only — every repo gets it.""" + from egg_contracts.agent_roles import AgentRole, get_roles_for_phase + + roles = get_roles_for_phase("implement", repo="some-org/some-repo") + assert AgentRole.REVIEWER_CODE_HOLISTIC in roles + + def test_maps_to_reviewer_contract_role(self) -> None: + from egg_contracts.agent_roles import get_contract_role + from egg_contracts.roles import Role + + assert get_contract_role("reviewer_code_holistic") == Role.REVIEWER + + def test_in_roles_without_worktree(self) -> None: + """Holistic reviewer must spawn without a per-agent worktree. + + Mirrors ``test_lens_reviewers_in_roles_without_worktree`` for the + new role: ``reviewer_code_holistic`` operates purely on the diff + via the BRC consensus bus and never writes code, so spawning it + with ``repos=[]`` must succeed and a spawn with a repo must not + provision a per-agent worktree. Without this membership a + ``KubernetesSpawnError("Cannot spawn reviewer_code_holistic … + no repos provided")`` would fire at spawn time. + """ + from egg_contracts.agent_roles import AgentRole + from kubernetes_spawner import _ROLES_WITHOUT_WORKTREE + + assert AgentRole.REVIEWER_CODE_HOLISTIC in _ROLES_WITHOUT_WORKTREE, ( + "AgentRole.REVIEWER_CODE_HOLISTIC must be in " + "_ROLES_WITHOUT_WORKTREE — it reviews diffs via the BRC bus " + "and does not need a per-agent git worktree." + ) + + +# --------------------------------------------------------------------------- +# Prompt assembly: criteria differentiation + always-on (no PR-size gate). +# --------------------------------------------------------------------------- + + +class TestHolisticPrompt: + def setup_method(self) -> None: + self.prompt = _build_review_prompt( + phase="implement", + pipeline_id="test-pipe", + pipeline_mode="issue", + reviewer_type="code-holistic", + issue_number=100, + ) + + def test_no_fan_out_block(self) -> None: + """Holistic always single-passes — no fan-out section, ever.""" + assert "Subagent Fan-Out Strategy" not in self.prompt, ( + "reviewer_code_holistic must not include the fan-out block — " + "it always reads the whole diff itself (issue #2126)." + ) + + def test_no_subagent_threshold_text(self) -> None: + """The 10-files / 500-LOC gate is reviewer_code's, not holistic's.""" + # Be conservative: the holistic prompt may reference review + # criteria that mention "10" or "500" for unrelated reasons, so + # only assert on the gate phrase itself. + assert "files_changed > 10" not in self.prompt + assert "(loc_added + loc_removed) > 500" not in self.prompt + + def test_carries_holistic_scope_marker(self) -> None: + """The scope preamble must identify this as the holistic lens.""" + prompt_lower = self.prompt.lower() + assert "holistic" in prompt_lower, ( + "Holistic reviewer prompt must surface its lens identity." + ) + + def test_canonical_use_case_reference(self) -> None: + """The PR #2105 ``__checkout__`` miss is the canonical example.""" + assert "__checkout__" in self.prompt or "PR #2105" in self.prompt, ( + "Holistic prompt should reference the canonical end-to-end " + "use-case-dead-end miss (PR #2105 / __checkout__)." + ) + + def test_complementary_framing(self) -> None: + """The preamble must tell the reviewer to defer line-by-line work.""" + prompt_lower = self.prompt.lower() + assert "fan-out" in prompt_lower or "slice" in prompt_lower, ( + "Holistic preamble must frame its job as complementary to " + "reviewer_code's fan-out / slice work." + ) + + def test_procedural_step_does_not_demand_every_file_review(self) -> None: + """Step 2 must not contradict the criteria's "don't verify every line". + + The unified procedural-steps block originally told every code + reviewer to "review every changed file systematically" — that + wording directly contradicted the holistic criteria file and the + scope preamble for ``reviewer_code_holistic``. The fix + differentiates step 2 by lens; this test pins that the holistic + prompt does not regress to the slice-style wording. + """ + assert "review every changed file systematically" not in self.prompt, ( + "Holistic procedural step 2 must not include the slice-style " + '"review every changed file systematically" wording — it ' + "directly contradicts the holistic criteria's " + "'don't verify every line; the fan-out reviewer covers that' " + "(issue #2126)." + ) + + def test_procedural_step_references_holistic_passes(self) -> None: + """Step 8 should orient the agent to the four holistic passes.""" + assert "four mandatory passes" in self.prompt or ( + "end-to-end primary use case" in self.prompt and "synthetic-key" in self.prompt + ), ( + "Holistic procedural step 8 must name the four mandatory " + "passes from the criteria so the model knows what shape of " + "review the criteria file is structuring." + ) diff --git a/orchestrator/tests/test_short_flow_contract_reviewer.py b/orchestrator/tests/test_short_flow_contract_reviewer.py index 88fe6fd14c..3ab5c0c195 100644 --- a/orchestrator/tests/test_short_flow_contract_reviewer.py +++ b/orchestrator/tests/test_short_flow_contract_reviewer.py @@ -34,6 +34,7 @@ AgentRole.TESTER, AgentRole.DOCUMENTER, AgentRole.REVIEWER_CODE, + AgentRole.REVIEWER_CODE_HOLISTIC, AgentRole.REVIEWER_CONTRACT, AgentRole.REVIEWER_SECURITY, AgentRole.REVIEWER_CONCURRENCY, diff --git a/shared/egg_contracts/agent_roles.py b/shared/egg_contracts/agent_roles.py index 02a6793816..79ed68f78c 100644 --- a/shared/egg_contracts/agent_roles.py +++ b/shared/egg_contracts/agent_roles.py @@ -52,8 +52,9 @@ class AgentRole(StrEnum): Execution roles: CODER, TESTER, DOCUMENTER Analysis roles: ARCHITECT, TASK_PLANNER, RISK_ANALYST, REFINER - Review roles: REVIEWER_CODE, REVIEWER_CONTRACT, - REVIEWER_AGENT_DESIGN, REVIEWER_REFINE, REVIEWER_PLAN, + Review roles: REVIEWER_CODE, REVIEWER_CODE_HOLISTIC, + REVIEWER_CONTRACT, REVIEWER_AGENT_DESIGN, + REVIEWER_REFINE, REVIEWER_PLAN, REVIEWER_SECURITY, REVIEWER_CONCURRENCY Utility roles: AUTOFIXER, CONFLICT_RESOLVER Interface roles: OVERSEER @@ -70,6 +71,7 @@ class AgentRole(StrEnum): REFINER = "refiner" # Review roles REVIEWER_CODE = "reviewer_code" + REVIEWER_CODE_HOLISTIC = "reviewer_code_holistic" REVIEWER_CONTRACT = "reviewer_contract" REVIEWER_AGENT_DESIGN = "reviewer_agent_design" REVIEWER_REFINE = "reviewer_refine" @@ -516,6 +518,35 @@ def depends_on(self, other: AgentRole) -> bool: requires_inputs=[], ) +# Holistic generalist counterpart to ``reviewer_code`` (issue #2126). +# Always single-passes the full diff regardless of size — fan-out is +# reserved for ``reviewer_code``. Its job is the architectural-coherence +# question no fan-out slice owns: does the primary advertised use case +# work end-to-end, do docs and code agree, do synthetic keys round-trip +# across modules, are silent fallbacks hiding operator-visible failures. +REVIEWER_CODE_HOLISTIC_ROLE = AgentRoleDefinition( + role=AgentRole.REVIEWER_CODE_HOLISTIC, + description="Single-pass holistic code review focused on cross-module coherence", + category=AgentCategory.REVIEW, + responsibilities=[ + "Walk the primary advertised use case end-to-end across the full diff", + "Cross-check doc-claimed behaviour against what the code actually does", + "Audit synthetic keys, sentinels, and 'magic' values for cross-module agreement", + "Surface silent fallbacks that swallow operator-visible misconfiguration", + ], + dependencies=[AgentRole.TASK_PLANNER, AgentRole.RISK_ANALYST], + file_access=FileAccessPattern( + allowed_read=[], + allowed_write=[ + ".egg-state/reviews/", + ".egg-state/agent-outputs/", + ], + blocked_write=_REVIEWER_BLOCKED_WRITE, + ), + produces_outputs=["review_verdict"], + requires_inputs=[], +) + # Contract reviewer needs write access to .egg-state/contracts/ to mark # items as done, so it uses a custom blocked_write list that excludes it. _REVIEWER_CONTRACT_BLOCKED_WRITE = [ @@ -873,6 +904,7 @@ def depends_on(self, other: AgentRole) -> bool: AgentRole.REFINER: REFINER_ROLE, # Review roles AgentRole.REVIEWER_CODE: REVIEWER_CODE_ROLE, + AgentRole.REVIEWER_CODE_HOLISTIC: REVIEWER_CODE_HOLISTIC_ROLE, AgentRole.REVIEWER_CONTRACT: REVIEWER_CONTRACT_ROLE, AgentRole.REVIEWER_AGENT_DESIGN: REVIEWER_AGENT_DESIGN_ROLE, AgentRole.REVIEWER_REFINE: REVIEWER_REFINE_ROLE, @@ -911,6 +943,7 @@ def depends_on(self, other: AgentRole) -> bool: AgentRole.REFINER: Role.IMPLEMENTER, # Review: verdicts and phase-status/current_phase mutations AgentRole.REVIEWER_CODE: Role.REVIEWER, + AgentRole.REVIEWER_CODE_HOLISTIC: Role.REVIEWER, AgentRole.REVIEWER_CONTRACT: Role.REVIEWER, AgentRole.REVIEWER_AGENT_DESIGN: Role.REVIEWER, AgentRole.REVIEWER_REFINE: Role.REVIEWER, @@ -1081,6 +1114,7 @@ def can_retry(self, max_retries: int = 2) -> bool: _PHASE_REVIEWERS: dict[str, list[AgentRole]] = { "implement": [ AgentRole.REVIEWER_CODE, + AgentRole.REVIEWER_CODE_HOLISTIC, AgentRole.REVIEWER_CONTRACT, AgentRole.REVIEWER_SECURITY, AgentRole.REVIEWER_CONCURRENCY, diff --git a/shared/egg_contracts/checkpoint_cli.py b/shared/egg_contracts/checkpoint_cli.py index d13845d108..1b71d04db3 100644 --- a/shared/egg_contracts/checkpoint_cli.py +++ b/shared/egg_contracts/checkpoint_cli.py @@ -74,6 +74,7 @@ COMPOSITE_REVIEWER_ROLES: frozenset[str] = frozenset( { "reviewer_code", + "reviewer_code_holistic", "reviewer_contract", "reviewer_agent_design", "reviewer_refine", diff --git a/shared/egg_restrictions/patterns.py b/shared/egg_restrictions/patterns.py index bb0ed47ae9..5c05e83032 100644 --- a/shared/egg_restrictions/patterns.py +++ b/shared/egg_restrictions/patterns.py @@ -421,6 +421,13 @@ def _matches_pattern(file_path: str, pattern: str) -> bool: blocked_patterns=_REVIEWER_BLOCKED, ) +REVIEWER_CODE_HOLISTIC_PATTERNS = AgentFilePattern( + role=AgentRole.REVIEWER_CODE_HOLISTIC, + description="Holistic code reviewer agent: reviews and agent-outputs only", + allowed_patterns=_REVIEWER_ALLOWED, + blocked_patterns=_REVIEWER_BLOCKED, +) + # Contract reviewer needs write access to .egg-state/contracts/ to mark # items as done, so it uses custom lists that include/exclude contracts. _REVIEWER_CONTRACT_ALLOWED = [ @@ -695,6 +702,7 @@ def _matches_pattern(file_path: str, pattern: str) -> bool: AgentRole.TASK_PLANNER: TASK_PLANNER_PATTERNS, AgentRole.RISK_ANALYST: RISK_ANALYST_PATTERNS, AgentRole.REVIEWER_CODE: REVIEWER_CODE_PATTERNS, + AgentRole.REVIEWER_CODE_HOLISTIC: REVIEWER_CODE_HOLISTIC_PATTERNS, AgentRole.REVIEWER_CONTRACT: REVIEWER_CONTRACT_PATTERNS, AgentRole.REVIEWER_AGENT_DESIGN: REVIEWER_AGENT_DESIGN_PATTERNS, AgentRole.REFINER: REFINER_PATTERNS, diff --git a/shared/prompts/code-review-holistic-criteria.md b/shared/prompts/code-review-holistic-criteria.md new file mode 100644 index 0000000000..ba900925f4 --- /dev/null +++ b/shared/prompts/code-review-holistic-criteria.md @@ -0,0 +1,147 @@ + + +Inherits from `code-review-criteria.md`; the rules below are *additive* +and tell you what to focus on so your work complements `reviewer_code`'s +slice-by-slice fan-out instead of duplicating it. + +## Holistic Lens — Scope + +The holistic reviewer is the always-on generalist counterpart to +`reviewer_code` (which fans out into per-task slice subagents on large +diffs). On any non-trivial diff there are two failure modes that no +single-partition slice can catch and that the parent's fixed +cross-partition checklist sometimes misses: + +1. The **primary advertised use case** quietly fails end-to-end because + one module's output is silently dropped by another module's + consumer. +2. **Docs and code drift** apart — the README claims behaviour the + code does not implement, or the code emits state nothing documents. + +Issue #2126 was filed because PR #2105 shipped both shapes past a clean +fan-out review: the `__checkout__` synthetic-key dead-end broke the +PR's primary advertised use case end-to-end, and the migration doc +described an `infer_*` pathway the merge layer did not call. The +holistic lens is the floor that exists to catch those — fan-out and +the security / concurrency lenses remain additive on top. + +The holistic lens is **CRITICAL** — your NACK gates consensus exactly +the same way `reviewer_code`'s does. Distinct roles let your NACK on +architectural coherence stand on its own without being averaged +against six fan-out subagent ACKs on slice-correctness. + +## How to Review + +**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. + +**Read the diff once with the whole PR in mind.** Skim every file to +build a mental map of "what does this PR add, what does it change, who +is the user, what is the user's primary path through the change?" + +### Mandatory passes + +Run all four. Skipping any of them defeats the purpose of the role. + +#### 1. End-to-end primary use case + +Take the PR's stated intent (issue, description, contract acceptance +criteria — whichever names the user-visible change most concretely) +and walk it on the merged code. The user does X, the code path is +A → B → C, does C produce what A promised? Trace the literal call +chain — do not infer from naming. The `__checkout__` dead-end on +PR #2105 is the canonical miss: a string flowed from +`shared/egg_config/repos.py` into `sandbox/egg_lib/docker.py`'s lookup, +the lookup matched nothing, and the feature silently no-opped while +every slice-level review signed off because each file was internally +consistent. NACK any path where the producer's output is silently +dropped, defaulted, or filtered out by the consumer. + +#### 2. Doc ↔ code symmetry + +For every behaviour promised in `docs/`, `README.md`, or the PR +description, grep that the code actually does it. For every code path +that emits user-facing output (CLI text, log lines operators read, +HTTP error bodies), find the doc that documents it. Specifically +suspect: + +- "the loader will infer X" / "the system auto-derives Y" claims — + follow the call graph and confirm the inference function is reached. +- Documented YAML / JSON / shell snippets — paste them into the + schema or the validator mentally and confirm they parse. +- Rollback / migration sed snippets — confirm the regex matches the + text it claims to rewrite. + +NACK on doc-claimed behaviour the code does not implement. NACK on +code paths that emit user-facing output without a doc the operator +can find. + +#### 3. Synthetic-key, sentinel, and "magic" value coordination + +Synthetic keys, sentinels, magic strings (`__checkout__`, `default`, +`__all__`, `*`, empty string), and "special" enum values are +coordination points across modules. For every such value introduced +or referenced by the diff: + +- Find every consumer in another module. +- Confirm each consumer recognises the value (string-equality, regex, + pattern-match arm, allowlist entry). +- Flag asymmetries: producer emits the value, consumer's filter + excludes it (the `__checkout__` shape); or producer's filter + excludes the value, consumer expects it. + +NACK on any synthetic-key dead-end. The class is high-impact (it +silently disables features) and impossible to catch from a +single-file vantage point. + +#### 4. Silent-fallback hunt + +Search the diff for places where the operator would expect a signal — +an error, a warning, a refused operation — but the code instead +returns silently: + +- Bare `except Exception:` (or other broad excepts) that swallow the + error and fall back to a default. +- Functions that return `None` or an empty container on a path that + could plausibly be a misconfiguration the operator should see. +- Default-everything no-op branches (`if not config: return`) that + hide a missing-required-key bug behind a "looks fine" return. +- "Defence in depth" silent symlink / file-type rejections that drop + user input the operator deliberately set. + +NACK on silent fallbacks where the safety floor masks an +operator-facing misconfiguration. The code is "safe" in the narrow +sense (no crash, no security violation) and unsafe in the wide sense +(the operator believes the config is loaded when it is not). + +## What to Skip + +- **Line-by-line correctness.** That is `reviewer_code`'s slice work — + defer to it. +- **Security findings beyond cross-module synthetic-key / + silent-fallback patterns.** Defer to `reviewer_security`. +- **Concurrency findings.** Defer to `reviewer_concurrency`. +- **Style, naming, type-annotation completeness, lint-handled issues.** + Defer to lint, `tester`, and the base file's skip list. +- **Issues already explicitly raised by another reviewer.** If + `reviewer_code` has called out the `__checkout__`-shaped bug, + acknowledge and move on rather than re-flagging. + +## Verdict shape + +Your NACK should name: + +- The pass that found the issue (use case / doc symmetry / synthetic + key / silent fallback). +- The producer module and the consumer module the asymmetry spans. +- The user-visible failure shape — "drops `repo_settings:` for repos + not in the user file", not "filter regex is wrong". + +If the diff is small and all four passes come back clean, a concise +ACK is acceptable — verbose reports without findings are not +required, 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 (not a single-word "LGTM"). diff --git a/shared/tests/test_egg_restrictions.py b/shared/tests/test_egg_restrictions.py index 30eecfef14..1aa434f52b 100644 --- a/shared/tests/test_egg_restrictions.py +++ b/shared/tests/test_egg_restrictions.py @@ -21,6 +21,7 @@ OVERSEER_PATTERNS, REFINER_PATTERNS, REVIEWER_AGENT_DESIGN_PATTERNS, + REVIEWER_CODE_HOLISTIC_PATTERNS, REVIEWER_CODE_PATTERNS, REVIEWER_CONCURRENCY_PATTERNS, REVIEWER_CONTRACT_PATTERNS, @@ -38,7 +39,7 @@ class TestAgentRole: - def test_all_18_roles_defined(self): + def test_all_19_roles_defined(self): roles = [ AgentRole.CODER, AgentRole.TESTER, @@ -48,6 +49,7 @@ def test_all_18_roles_defined(self): AgentRole.RISK_ANALYST, AgentRole.REFINER, AgentRole.REVIEWER_CODE, + AgentRole.REVIEWER_CODE_HOLISTIC, AgentRole.REVIEWER_CONTRACT, AgentRole.REVIEWER_AGENT_DESIGN, AgentRole.REVIEWER_REFINE, @@ -59,9 +61,9 @@ def test_all_18_roles_defined(self): AgentRole.OVERSEER, AgentRole.INSPECTOR, ] - assert len(roles) == 18 + assert len(roles) == 19 # All unique - assert len(set(roles)) == 18 + assert len(set(roles)) == 19 def test_role_values_are_lowercase(self): for attr in dir(AgentRole): @@ -75,8 +77,8 @@ def test_role_values_are_lowercase(self): class TestAgentPatterns: - def test_registry_has_all_18_roles(self): - assert len(AGENT_PATTERNS) == 18 + def test_registry_has_all_19_roles(self): + assert len(AGENT_PATTERNS) == 19 def test_registry_keys_match_role_constants(self): expected_roles = { @@ -88,6 +90,7 @@ def test_registry_keys_match_role_constants(self): AgentRole.RISK_ANALYST, AgentRole.REFINER, AgentRole.REVIEWER_CODE, + AgentRole.REVIEWER_CODE_HOLISTIC, AgentRole.REVIEWER_CONTRACT, AgentRole.REVIEWER_AGENT_DESIGN, AgentRole.REVIEWER_REFINE, @@ -110,6 +113,7 @@ def test_named_constants_match_registry(self): assert AGENT_PATTERNS[AgentRole.RISK_ANALYST] is RISK_ANALYST_PATTERNS assert AGENT_PATTERNS[AgentRole.REFINER] is REFINER_PATTERNS assert AGENT_PATTERNS[AgentRole.REVIEWER_CODE] is REVIEWER_CODE_PATTERNS + assert AGENT_PATTERNS[AgentRole.REVIEWER_CODE_HOLISTIC] is REVIEWER_CODE_HOLISTIC_PATTERNS assert AGENT_PATTERNS[AgentRole.REVIEWER_CONTRACT] is REVIEWER_CONTRACT_PATTERNS assert AGENT_PATTERNS[AgentRole.REVIEWER_AGENT_DESIGN] is REVIEWER_AGENT_DESIGN_PATTERNS assert AGENT_PATTERNS[AgentRole.REVIEWER_REFINE] is REVIEWER_REFINE_PATTERNS diff --git a/tests/shared/egg_contracts/test_agent_roles.py b/tests/shared/egg_contracts/test_agent_roles.py index d9758ec7a4..9c954bc72a 100644 --- a/tests/shared/egg_contracts/test_agent_roles.py +++ b/tests/shared/egg_contracts/test_agent_roles.py @@ -80,6 +80,7 @@ class TestAgentRole: "risk_analyst", "refiner", "reviewer_code", + "reviewer_code_holistic", "reviewer_contract", "reviewer_agent_design", "reviewer_refine", @@ -114,6 +115,7 @@ def test_analysis_roles(self): def test_review_roles(self): assert AgentRole.REVIEWER_CODE == "reviewer_code" + assert AgentRole.REVIEWER_CODE_HOLISTIC == "reviewer_code_holistic" assert AgentRole.REVIEWER_CONTRACT == "reviewer_contract" assert AgentRole.REVIEWER_AGENT_DESIGN == "reviewer_agent_design" assert AgentRole.REVIEWER_REFINE == "reviewer_refine" @@ -305,6 +307,7 @@ def test_review_roles_category(self): """Review roles should have REVIEW category.""" for role_name in [ "reviewer_code", + "reviewer_code_holistic", "reviewer_contract", "reviewer_agent_design", "reviewer_refine", @@ -390,13 +393,14 @@ def test_review_roles(self): roles = get_roles_by_category(AgentCategory.REVIEW) role_values = {r.value if hasattr(r, "value") else r for r in roles} assert "reviewer_code" in role_values + assert "reviewer_code_holistic" in role_values assert "reviewer_contract" in role_values assert "reviewer_agent_design" in role_values assert "reviewer_refine" in role_values assert "reviewer_plan" in role_values assert "reviewer_security" in role_values assert "reviewer_concurrency" in role_values - assert len(roles) == 7 + assert len(roles) == 8 def test_utility_roles(self): from egg_contracts.agent_roles import get_roles_by_category