diff --git a/docs/architecture/orchestrator.md b/docs/architecture/orchestrator.md index db8222972f..3e70223722 100644 --- a/docs/architecture/orchestrator.md +++ b/docs/architecture/orchestrator.md @@ -285,12 +285,13 @@ The orchestrator coordinates specialized agent roles across pipeline phases. Eac | **Coder** | Write code, create commits, push branches | | **Tester** | Find gaps in implementation, write and run tests, run linters/type checkers, apply auto-fixes | | **Documenter** | Update docs and READMEs | -| **Reviewer (Code)** | Security, correctness, code quality, testing, documentation. On large diffs (~10 changed files OR ~500 LOC) self-gates and partitions the change set across Claude Agent SDK `Task` subagents along the implement-phase task list ([#1965](https://github.com/jwbron/egg/issues/1965)); see [Concurrent Execution: Implement-phase `reviewer_code` Subagent Fan-Out](../guides/concurrent-execution.md#implement-phase-reviewer_code-subagent-fan-out). | +| **Reviewer (Code)** | Security, correctness, code quality, testing, documentation. Reviews every changed file systematically and emits a single CRITICAL ACK / NACK on the full diff. | +| **Reviewer (Code Holistic)** | Single-pass cross-module coherence review ([#2126](https://github.com/jwbron/egg/issues/2126)) — runs alongside Reviewer (Code) and gates consensus independently on architectural-coherence findings. | | **Reviewer (Contract)** | Verify acceptance criteria met, task completion status | -| **Reviewer (Security)** _(ADVISORY)_ | Security-lens review focused on cross-file allowlist mismatches, handler-vs-validator path mismatches, uncommitted-artifact / Dockerfile-symlink mismatches, secret leakage, and cross-file OWASP top-10 patterns. Criteria: [`shared/prompts/security-review-criteria.md`](../../shared/prompts/security-review-criteria.md). NACKs are recorded but do not deadlock consensus until [#1997](https://github.com/jwbron/egg/issues/1997)'s severity-tagged NACK signalling lands. | -| **Reviewer (Concurrency)** _(ADVISORY)_ | Concurrency-lens review focused on race conditions, deadlocks, shared-state mutation, retry storms, resource-cleanup ordering, and BRC-protocol invariants. Criteria: [`shared/prompts/concurrency-review-criteria.md`](../../shared/prompts/concurrency-review-criteria.md). Same ADVISORY semantics as Reviewer (Security). | +| **Reviewer (Security)** | Security-lens review focused on cross-file allowlist mismatches, handler-vs-validator path mismatches, uncommitted-artifact / Dockerfile-symlink mismatches, secret leakage, and cross-file OWASP top-10 patterns. Criteria: [`shared/prompts/security-review-criteria.md`](../../shared/prompts/security-review-criteria.md). CRITICAL — a NACK blocks consensus ([#2139](https://github.com/jwbron/egg/issues/2139)). | +| **Reviewer (Concurrency)** | Concurrency-lens review focused on race conditions, deadlocks, shared-state mutation, retry storms, resource-cleanup ordering, and BRC-protocol invariants. Criteria: [`shared/prompts/concurrency-review-criteria.md`](../../shared/prompts/concurrency-review-criteria.md). CRITICAL — same as Reviewer (Security) ([#2139](https://github.com/jwbron/egg/issues/2139)). | -**Execution model**: All implement phase agents run concurrently via the BRC consensus protocol. Agents communicate via the orchestrator message bus and reach phase completion through peer consensus. The two ADVISORY lens reviewers run alongside the critical reviewers on the same change set; promotion from ADVISORY to CRITICAL is gated on [#1997](https://github.com/jwbron/egg/issues/1997). +**Execution model**: All implement phase agents run concurrently via the BRC consensus protocol. Agents communicate via the orchestrator message bus and reach phase completion through peer consensus. ### Prompt Context Scoping diff --git a/docs/guides/concurrent-execution.md b/docs/guides/concurrent-execution.md index 90116a0428..6c9f1a03c8 100644 --- a/docs/guides/concurrent-execution.md +++ b/docs/guides/concurrent-execution.md @@ -42,7 +42,7 @@ When concurrent execution starts, the `ConcurrentPhaseExecutor` (in `orchestrato |-------|--------------| | `refine` | `refiner`, `reviewer_refine`, `reviewer_agent_design` (egg repo only) | | `plan` | `architect`, `task_planner`, `risk_analyst`, `reviewer_plan` | -| `implement` | `coder`, `tester`, `documenter`, `reviewer_code`, `reviewer_code_holistic`, `reviewer_contract`, `reviewer_security` (ADVISORY), `reviewer_concurrency` (ADVISORY) | +| `implement` | `coder`, `tester`, `documenter`, `reviewer_code`, `reviewer_code_holistic`, `reviewer_contract`, `reviewer_security`, `reviewer_concurrency` | **Shared branch**: All agents operate on the pipeline's shared branch (e.g., `egg/issue-123`). Agents coordinate commits via the message bus to sequence their work and avoid conflicts. @@ -353,9 +353,9 @@ A reviewer has three outcomes on a proposal: - **NACK** — proposal is wrong; producer must iterate before merge. - **Conditional ACK** — proposal is correct but requires a specific human-only action *at merge time* (e.g. a `git mv`, a cross-repo config flip). Pass `--pre-merge-condition "..."` on `egg-orch consensus ack`; the condition is persisted on the approval-matrix edge, scoped to the current proposal version, surfaced in `egg-orch consensus status`, and rendered in a **Pre-merge Obligations** section on the auto-created PR body so the merger cannot skim past it. Not a soft NACK — if the agents can address the issue themselves, NACK instead. See the [Conditional ACK reference](../reference/conditional-ack.md). -### Implement-phase `reviewer_code` Subagent Fan-Out +### Implement-phase Reviewer Roster -On the implement phase, `reviewer_code` self-gates on diff size and partitions large diffs across Claude Agent SDK `Task` subagents so every changed file is read carefully ([#1965](https://github.com/jwbron/egg/issues/1965)). The reviewer first runs `git diff --numstat` against the resolved base ref; when the diff exceeds **~10 changed files OR ~500 lines of change**, it self-fetches `phases.implement.tasks[]` via `mcp__sdlc__show_contract` and spawns one subagent per task partition (capped at 6 subagents per review with a 5-minute / 300-second per-subagent wall-clock timeout). Each subagent re-runs `git diff` filtered by its assigned path globs, reviews only its slice, and is forbidden from spawning subagents of its own. The parent reviewer then performs a **cross-partition consistency pass** (handler ↔ allowlist, route ↔ schema, fixture ↔ Dockerfile/symlink, import-graph cycles) before emitting a single ACK / NACK on behalf of the whole review — closing the cross-file blind spot that let [PR #1964](https://github.com/jwbron/egg/pull/1964)'s `^project$` allowlist bypass slip through. Below the threshold, on empty implement-phase task lists (custom-phase invocations, contractless `babysit_pr`), or when MCP is unreachable from the subagent context, the reviewer falls back to single-pass review with a STATUS heartbeat noting the gate decision (`fan-out: enabled / skipped`). Parallelism is configurable per pipeline via `phase_configs.implement.reviewer_code.parallel` (default `true`); the fan-out block lives in `_build_review_prompt()` in `orchestrator/routes/pipelines.py` and is described in detail in [`shared/prompts/REVIEWER-SYNC.md`](../../shared/prompts/REVIEWER-SYNC.md). `reviewer_code_holistic` ([#2126](https://github.com/jwbron/egg/issues/2126)) runs alongside `reviewer_code` as a distinct CRITICAL reviewer focused on cross-module coherence — it skims the full diff once and runs four holistic passes (end-to-end use case, doc↔code symmetry, synthetic-key/sentinel audit, silent-fallback hunt) rather than reviewing every file line-by-line. Its NACK gates consensus independently and is not averaged with fan-out slice ACKs. The two ADVISORY lens reviewers `reviewer_security` and `reviewer_concurrency` (criteria in [`security-review-criteria.md`](../../shared/prompts/security-review-criteria.md) and [`concurrency-review-criteria.md`](../../shared/prompts/concurrency-review-criteria.md)) also run alongside `reviewer_code` on the same change set; their NACKs are recorded but do not deadlock consensus until severity-tagged NACK signalling lands in [#1997](https://github.com/jwbron/egg/issues/1997). +On the implement phase, `reviewer_code` reviews every changed file systematically and emits a single CRITICAL ACK / NACK on the full diff. `reviewer_code_holistic` ([#2126](https://github.com/jwbron/egg/issues/2126)) runs alongside as a distinct CRITICAL reviewer focused on cross-module coherence — it skims the full diff once and runs four holistic passes (end-to-end use case, doc↔code symmetry, synthetic-key/sentinel audit, silent-fallback hunt) rather than verifying every line. Its NACK gates consensus independently of `reviewer_code`'s. Two CRITICAL lens reviewers `reviewer_security` and `reviewer_concurrency` (criteria in [`security-review-criteria.md`](../../shared/prompts/security-review-criteria.md) and [`concurrency-review-criteria.md`](../../shared/prompts/concurrency-review-criteria.md)) also run on the same change set; a NACK from either blocks consensus until the producer re-proposes ([#2139](https://github.com/jwbron/egg/issues/2139) — promoted from ADVISORY, closing [#1997](https://github.com/jwbron/egg/issues/1997)). ### Pre-Proposal ACK Protection diff --git a/docs/guides/sdlc-pipeline.md b/docs/guides/sdlc-pipeline.md index fa8dcef8fe..18b7526682 100644 --- a/docs/guides/sdlc-pipeline.md +++ b/docs/guides/sdlc-pipeline.md @@ -941,10 +941,7 @@ Contracts can override phase defaults via the `phase_configs` field: } ], "max_review_cycles": 5, - "human_review_mechanism": "PR_REVIEW", - "reviewer_code": { - "parallel": true - } + "human_review_mechanism": "PR_REVIEW" } } } @@ -952,12 +949,6 @@ Contracts can override phase defaults via the `phase_configs` field: When `phase_configs.{phase}.checks` is specified, it completely replaces the default checks for that phase. -The `reviewer_code` object exposes a single knob: - -| Field | Type | Default | Description | -|-------|------|---------|-------------| -| `parallel` | bool | `true` | Fan out `reviewer_code` subagents in parallel. Set `false` to force sequential review for cost or quota reasons. | - ### Writing Custom Checks Custom checks can be configured per-repository in `~/.config/egg/repositories.yaml` (see above) or by adding check definitions to `shared/egg_contracts/phase_defaults.py`. diff --git a/docs/reference/agent-roles.md b/docs/reference/agent-roles.md index 7a0bfffe9d..4addd5163f 100644 --- a/docs/reference/agent-roles.md +++ b/docs/reference/agent-roles.md @@ -243,16 +243,14 @@ each surface so reviewers know to keep them in sync. - Allowed writes: `.egg-state/reviews/`, `.egg-state/agent-outputs/` - Blocked: All source, docs, tests, contracts, drafts -**Subagent fan-out**: On large diffs (`files_changed > 10` OR `loc_added + loc_removed > 500`), `reviewer_code` fans out into Claude Agent SDK subagents — one per implement-phase task partition (capped at 6, with a 5-minute / 300-second per-subagent wall-clock timeout that NACKs the partition on overrun). Each subagent reviews its slice; the parent aggregates findings and emits the single ACK/NACK. A mandatory cross-partition consistency pass runs regardless of whether fan-out fires. Fan-out can be forced sequential via `phase_configs.implement.reviewer_code.parallel = false` (default: `true`). - **Outputs**: - `.egg-state/reviews/{identifier}-implement-code-review.json` — Verdict file ### `reviewer_code_holistic` -**Purpose**: Single-pass holistic code review focused on cross-module coherence. Runs alongside `reviewer_code`'s slice-by-slice fan-out — its job is the architectural-coherence question no fan-out slice owns. +**Purpose**: Single-pass holistic code review focused on cross-module coherence. Runs alongside `reviewer_code` — its job is the architectural-coherence question line-by-line review does not own. -**Criticality**: CRITICAL — NACKs block consensus on their own and are not averaged against `reviewer_code`'s fan-out ACKs. +**Criticality**: CRITICAL — NACKs block consensus on their own, independent of `reviewer_code`'s verdict. **Focus areas** (four mandatory passes): 1. Walk the primary advertised use case end-to-end across the full diff. @@ -280,9 +278,9 @@ each surface so reviewers know to keep them in sync. ### `reviewer_security` -**Purpose**: ADVISORY security-lens reviewer. Focuses exclusively on cross-file security invariants that a general code reviewer may miss: cross-file allowlist mismatches, handler-vs-validator path mismatches, information-disclosure and authorization-bypass patterns, uncommitted-artifact/Dockerfile-symlink mismatches, secret leakage, and OWASP top-10 patterns spanning multiple changed files. +**Purpose**: Security-lens reviewer. Focuses exclusively on cross-file security invariants that a general code reviewer may miss: cross-file allowlist mismatches, handler-vs-validator path mismatches, information-disclosure and authorization-bypass patterns, uncommitted-artifact/Dockerfile-symlink mismatches, secret leakage, and OWASP top-10 patterns spanning multiple changed files. -**Criticality**: ADVISORY — NACKs block consensus informally but do not deadlock BRC until severity-tagged NACK signalling lands. Promotion to CRITICAL is intentionally deferred. +**Criticality**: CRITICAL — a NACK blocks consensus until the producer re-proposes ([#2139](https://github.com/jwbron/egg/issues/2139); promoted from ADVISORY, closing [#1997](https://github.com/jwbron/egg/issues/1997)). **File access**: - Allowed writes: `.egg-state/reviews/`, `.egg-state/agent-outputs/` @@ -293,9 +291,9 @@ each surface so reviewers know to keep them in sync. ### `reviewer_concurrency` -**Purpose**: ADVISORY concurrency-lens reviewer. Focuses exclusively on concurrency invariants: race conditions, deadlocks, shared-state mutation without synchronization, async-context leakage, retry-storm patterns, resource-cleanup ordering bugs, and BRC-protocol invariants (send→wait ordering, cursor threading, heartbeat-stall windows). +**Purpose**: Concurrency-lens reviewer. Focuses exclusively on concurrency invariants: race conditions, deadlocks, shared-state mutation without synchronization, async-context leakage, retry-storm patterns, resource-cleanup ordering bugs, and BRC-protocol invariants (send→wait ordering, cursor threading, heartbeat-stall windows). -**Criticality**: ADVISORY — same deferral rationale as `reviewer_security` above. +**Criticality**: CRITICAL — same as `reviewer_security` above ([#2139](https://github.com/jwbron/egg/issues/2139)). **File access**: - Allowed writes: `.egg-state/reviews/`, `.egg-state/agent-outputs/` diff --git a/docs/reference/checkpoint-browser.md b/docs/reference/checkpoint-browser.md index 3956847c6c..54f0c66eb3 100644 --- a/docs/reference/checkpoint-browser.md +++ b/docs/reference/checkpoint-browser.md @@ -63,7 +63,7 @@ The `--agent-type` flag accepts both coarse agent types (e.g., `reviewer`) and c | Composite Role | Description | |----------------|-------------| -| `reviewer_code` | Code quality reviewer (fan-out, slice-by-slice) | +| `reviewer_code` | Code quality reviewer (line-by-line) | | `reviewer_code_holistic` | Holistic code reviewer (cross-module coherence) | | `reviewer_contract` | Contract compliance reviewer | | `reviewer_agent_design` | Agent design reviewer | diff --git a/integration_tests/sdlc/test_reviewer_1964_regression.py b/integration_tests/sdlc/test_reviewer_1964_regression.py deleted file mode 100644 index 3e49fe1a5b..0000000000 --- a/integration_tests/sdlc/test_reviewer_1964_regression.py +++ /dev/null @@ -1,354 +0,0 @@ -"""PR #1964 regression-replay tests for ``reviewer_code`` (issue #1965 / TASK-5-2). - -This file ships in two modes: - -(a) **Prompt-asserts (always on)** - Given a synthetic 12-file / 800-LOC fixture, the prompt produced by - ``_build_review_prompt(reviewer_type="code", phase="implement", ...)`` - instructs subagent fan-out (numstat command, partition fetch with - both fallbacks, parallel-vs-sequential per kwarg, parent - cross-partition consistency pass with ``handler``/``allowlist`` - markers, 6-subagent cap, 5-minute timeout, and STATUS-heartbeat - instrumentation). Given a 3-file / 50-LOC fixture, the prompt - instructs solo review and does NOT include the fan-out commitments. - Both ``reviewer_code_parallel=True`` and ``False`` are exercised - via ``pytest.mark.parametrize`` so the parallel-vs-sequential - instruction wording is asserted in both directions. - -(b) **Live-LLM replay (gated by ``RUN_REVIEWER_REPLAY=1``)** - Invokes the real reviewer prompt against the cached - ``PR_1964_DIFF`` fixture and asserts the resulting review text - mentions both ``sandbox/scripts/jira`` (uncommitted file) and - ``^project$`` (allowlist bypass). The model alias is read from - ``shared/egg_agent/client.DEFAULT_MODEL`` at test-collection time - so the live test cannot drift independently of production - reviewers — DO NOT hard-code a date-pinned model identifier here. - -Why two modes -------------- -The prompt-asserts run on every CI run (cheap, deterministic) and -catch regressions in the fan-out instruction text. The live-LLM mode -is opt-in (set ``RUN_REVIEWER_REPLAY=1`` to enable) and validates that -an LLM following the prompt actually finds both motivating bugs. - -Fixture provenance ------------------- -The ``PR_1964_DIFF`` constant below is a hand-trimmed representative -slice of https://github.com/jwbron/egg/pull/1964. The slice keeps both -motivating bugs visible: - - 1. **Uncommitted ``sandbox/scripts/jira`` symlink** — the - ``Dockerfile`` references a wrapper script that was never - committed. The slice keeps the relevant ``Dockerfile`` line and - the (intentionally empty / missing) ``sandbox/scripts/jira`` - reference so a reviewer reading the diff can spot the broken - symlink without leaving the patch. - 2. **``^project$`` allowlist bypass in ``/api/v1/jira/execute``** — - the route handler accepts ``path=project`` but the project- - allowlist extractor lives in a different file and is bypassed for - that path. The slice keeps both files so a reviewer can see the - handler↔allowlist mismatch the BRC ``reviewer_code`` missed. - -Real source content is replaced with structural placeholders where the -bug surface does not depend on it. The diff is realistic in shape -(``diff --git``, ``---``, ``+++``, hunk headers, ``+``/``-`` lines) but -compact. The constant is inlined here (rather than living in a -``fixtures/`` module) because the tester role's gateway-allowed write -patterns only cover ``**/test_*.py`` / ``**/tests/`` / etc. — non-test -``.py`` files under ``integration_tests/`` are blocked by the gateway's -restricted-path enforcement (#2039), so a separate ``fixtures/`` module -cannot be pushed. -""" - -from __future__ import annotations - -import os -import sys -from unittest.mock import MagicMock - -import pytest - -# Stub Docker the same way 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) - - -# --- Fixture: cached PR #1964 diff slice. --------------------------------- -PR_1964_DIFF: str = """\ -diff --git a/sandbox/Dockerfile b/sandbox/Dockerfile -index 1111111..2222222 100644 ---- a/sandbox/Dockerfile -+++ b/sandbox/Dockerfile -@@ -42,6 +42,9 @@ COPY scripts/atlassian-token /usr/local/bin/atlassian-token - RUN chmod +x /usr/local/bin/atlassian-token - -+# Wrapper that delegates to the JIRA REST API — wired in #1964 so -+# `jira` is available on the sandbox PATH. -+COPY scripts/jira /usr/local/bin/jira -+RUN chmod +x /usr/local/bin/jira -+ - ENV PATH="/usr/local/bin:${PATH}" -diff --git a/sandbox/scripts/jira b/sandbox/scripts/jira -new file mode 120000 -index 0000000..3333333 ---- /dev/null -+++ b/sandbox/scripts/jira -@@ -0,0 +1 @@ -+atlassian-token -\\ No newline at end of file -diff --git a/gateway/jira_routes.py b/gateway/jira_routes.py -index 4444444..5555555 100644 ---- a/gateway/jira_routes.py -+++ b/gateway/jira_routes.py -@@ -10,12 +10,15 @@ ALLOWED_PATHS = { - "issue", - "search", - "comment", -+ # New: surface JIRA project metadata for the agent. -+ "project", - } - - - @bp.route("/api/v1/jira/execute", methods=["POST"]) - def execute() -> Response: - payload = request.get_json(force=True) - path: str = payload["path"] -+ # NOTE: project-list bypass — `path == 'project'` skips the -+ # per-project allowlist check below. - if path not in ALLOWED_PATHS: - abort(400, "path not allowed") -diff --git a/gateway/jira_allowlist.py b/gateway/jira_allowlist.py -index 6666666..7777777 100644 ---- a/gateway/jira_allowlist.py -+++ b/gateway/jira_allowlist.py -@@ -22,7 +22,7 @@ def project_for(path: str, query: dict) -> str | None: - # Pulls the JIRA project key from the query so the per-project - # allowlist can gate the request. -- if path == "issue": -+ if path in {"issue", "search", "comment"}: - return query.get("project_key") -- return None -+ return None # ^project$ NOT covered — request goes through unguarded -""" - - -def synthesize_diff(num_files: int, loc: int) -> str: - """Produce a realistic-looking patch with ``num_files`` files and ``loc`` total lines. - - Used by the prompt-assert mode to verify the fan-out block engages - above the threshold and skips below it. The generated patch text is - structurally valid (``diff --git`` headers, ``---``/``+++``, hunk - headers, ``+``/``-`` lines) but has no real source content — that - is sufficient for prompt-text asserts because those tests only - verify what the prompt builder produces. - """ - if num_files < 0: - raise ValueError("num_files must be non-negative") - if loc < 0: - raise ValueError("loc must be non-negative") - if num_files == 0: - return "" - base, remainder = divmod(loc, num_files) - chunks: list[str] = [] - for i in range(num_files): - lines_for_file = base + (remainder if i == num_files - 1 else 0) - path = f"src/synthetic/file_{i:03d}.py" - prev_oid = f"{i:07x}" - new_oid = f"{(i + 1):07x}" - header = ( - f"diff --git a/{path} b/{path}\n" - f"index {prev_oid}..{new_oid} 100644\n" - f"--- a/{path}\n" - f"+++ b/{path}\n" - f"@@ -1,1 +1,{max(lines_for_file, 1)} @@\n" - ) - body_lines: list[str] = [] - for line_idx in range(lines_for_file): - body_lines.append(f"+# synthetic line {line_idx} for {path}") - if not body_lines: - body_lines.append("+# synthetic line 0 for empty hunk") - chunks.append(header + "\n".join(body_lines) + "\n") - return "".join(chunks) - - -def _build_review_prompt_under_test(**kwargs): - """Lazy import so the module can load even before pipelines wires the kwarg.""" - from routes.pipelines import _build_review_prompt - - return _build_review_prompt( - phase="implement", - pipeline_id="test-pipe", - pipeline_mode="issue", - reviewer_type="code", - issue_number=1965, - **kwargs, - ) - - -# --------------------------------------------------------------------------- -# Mode (a) — Prompt-asserts (always on). -# --------------------------------------------------------------------------- - - -class TestPromptAssertsAboveThreshold: - """A 12-file / 800-LOC diff should produce a prompt that engages fan-out.""" - - @pytest.mark.parametrize("parallel", [True, False]) - def test_fan_out_block_present_above_threshold(self, parallel: bool) -> None: - prompt = _build_review_prompt_under_test(reviewer_code_parallel=parallel) - assert "Subagent Fan-Out Strategy" in prompt - assert "git diff --numstat" in prompt - assert "mcp__sdlc__show_contract" in prompt - assert "phases.implement.tasks" in prompt - assert "subagents must NOT spawn their own subagents" in prompt - assert "cross-partition" in prompt.lower() - assert "handler" in prompt.lower() - assert "allowlist" in prompt.lower() - assert ("capped at 6" in prompt) or ("never spawn more than 6 subagents" in prompt) - assert ("5 minutes" in prompt) or ("300 seconds" in prompt) - assert "fan-out: enabled" in prompt - assert "fan-out: skipped" in prompt - - def test_fan_out_block_says_in_parallel_when_true(self) -> None: - prompt = _build_review_prompt_under_test(reviewer_code_parallel=True) - assert "in parallel" in prompt.lower() - - def test_fan_out_block_says_sequentially_when_false(self) -> None: - prompt = _build_review_prompt_under_test(reviewer_code_parallel=False) - assert "sequentially" in prompt.lower() - - -class TestPromptAssertsBelowThreshold: - """A 3-file / 50-LOC diff doesn't trigger fan-out at runtime. - - Note that ``_build_review_prompt`` does not actually compute the - diff size — Pitfall 2 was resolved as Option 2B (reviewer - self-gates). The prompt always includes the fan-out block (the - reviewer decides at runtime whether to engage). What we assert - here is that the *fallback wording* explaining the below-threshold - skip is present, so a reviewer reading the prompt knows to skip - fan-out when the numbers are small. - """ - - def test_below_threshold_skip_wording_present(self) -> None: - prompt = _build_review_prompt_under_test() - prompt_lower = prompt.lower() - # Either explicit "below threshold" / "skip" wording, or the - # 'fan-out: skipped' STATUS heartbeat wording — both indicate - # the reviewer knows when to bypass fan-out. - assert ( - "fan-out: skipped" in prompt_lower or "below" in prompt_lower or "skip" in prompt_lower - ), "Fan-out block must explain the below-threshold skip path." - - -class TestSynthesizedDiffsAreShaped: - """The ``synthesize_diff`` helper produces realistic patch text.""" - - def test_above_threshold_synthesized_diff_shape(self) -> None: - diff = synthesize_diff(12, 800) - assert "diff --git" in diff - assert diff.count("diff --git") == 12 - assert "+# synthetic line" in diff - - def test_below_threshold_synthesized_diff_shape(self) -> None: - diff = synthesize_diff(3, 50) - assert "diff --git" in diff - assert diff.count("diff --git") == 3 - - def test_synthesize_diff_zero_files_returns_empty(self) -> None: - assert synthesize_diff(0, 0) == "" - - def test_synthesize_diff_rejects_negative_inputs(self) -> None: - with pytest.raises(ValueError): - synthesize_diff(-1, 10) - with pytest.raises(ValueError): - synthesize_diff(3, -1) - - -class TestPr1964FixtureSurfacesBothBugs: - """The cached PR #1964 diff string contains both motivating bug surfaces.""" - - def test_contains_uncommitted_jira_symlink_reference(self) -> None: - # The reviewer should be able to spot that sandbox/scripts/jira - # is referenced from the Dockerfile (and from the symlink mode - # marker) but the wrapper itself is not a real script. - assert "sandbox/scripts/jira" in PR_1964_DIFF - assert "Dockerfile" in PR_1964_DIFF - - def test_contains_project_allowlist_bypass(self) -> None: - assert "^project$" in PR_1964_DIFF or '"project"' in PR_1964_DIFF - # The allowlist file change is also in the slice so a reviewer - # can see the cross-file mismatch. - assert "jira_allowlist" in PR_1964_DIFF - - def test_fixture_under_size_budget(self) -> None: - """TASK-5-1: keep the fixture below 200 KB. - - The fixture is now inlined in this test file, so we measure the - size of the ``PR_1964_DIFF`` constant itself rather than a - separate ``fixtures/pr_1964_diff.py`` file (the gateway blocks - non-test ``.py`` files under ``integration_tests/`` for the - tester role). - """ - size = len(PR_1964_DIFF.encode("utf-8")) - assert size < 200_000, f"PR #1964 fixture exceeds 200 KB budget ({size} bytes)." - - -# --------------------------------------------------------------------------- -# Mode (b) — Live-LLM replay (gated by RUN_REVIEWER_REPLAY=1). -# --------------------------------------------------------------------------- - - -def _resolve_reviewer_model() -> str: - """Read the production model alias at test-collection time. - - TASK-5-2 forbids hard-coding a date-pinned model identifier. We - resolve via ``shared/egg_agent/client.DEFAULT_MODEL`` so the live - test follows whatever production reviewers run today. - """ - from egg_agent.client import DEFAULT_MODEL - - return DEFAULT_MODEL - - -@pytest.mark.skipif( - not os.environ.get("RUN_REVIEWER_REPLAY"), - reason="Set RUN_REVIEWER_REPLAY=1 to run the live PR #1964 replay test.", -) -class TestLiveReviewerReplay: - """Run the real reviewer prompt against the cached PR #1964 diff. - - Skipped by default; opt in via ``RUN_REVIEWER_REPLAY=1``. - """ - - def test_pr_1964_replay_finds_both_missed_issues(self) -> None: - from egg_agent.client import run_agent - - model = _resolve_reviewer_model() - # Compose a minimal reviewer prompt that points at the cached - # diff and asks for an analysis. The exact wording comes from - # ``_build_review_prompt`` — we wrap it with a "review this - # patch" preamble so the LLM sees the diff inline (sandbox - # may not have repo-relative git access). - review_prompt = _build_review_prompt_under_test() - full_prompt = ( - review_prompt + "\n\nThe following patch has been pre-fetched. Treat it as the " - "PR diff under review. Do not run git; review the patch " - "directly.\n\n" + PR_1964_DIFF - ) - - result = run_agent(full_prompt, model=model) - text = (getattr(result, "text", "") or "").lower() - - # Bug 1: uncommitted/broken sandbox/scripts/jira reference. - assert "sandbox/scripts/jira" in text or "scripts/jira" in text, ( - "Live reviewer did not flag the uncommitted/broken " - "sandbox/scripts/jira reference (PR #1964 bug 1)." - ) - - # Bug 2: ^project$ allowlist bypass. - assert ( - "^project$" in text or "project" in text and ("allowlist" in text or "bypass" in text) - ), "Live reviewer did not flag the ^project$ allowlist bypass (PR #1964 bug 2)." diff --git a/orchestrator/review_graph.py b/orchestrator/review_graph.py index bf4ccc86f2..428c8dd894 100644 --- a/orchestrator/review_graph.py +++ b/orchestrator/review_graph.py @@ -219,21 +219,20 @@ def get_default_implement_graph() -> ReviewGraph: - 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. + coherence stands on its own. - 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 - - reviewer_concurrency reviews coder and tester (advisory) — lens reviewer + - reviewer_security reviews coder and tester (critical) — lens reviewer + - reviewer_concurrency reviews coder and tester (critical) — lens reviewer - The two ADVISORY lens reviewers (``reviewer_security`` and - ``reviewer_concurrency``) ship advisory-only on day 1 so they cannot - deadlock consensus while severity-tagged NACK signalling lands in - issue #1997. Promotion to CRITICAL is intentionally deferred. + Issue #2139 promoted ``reviewer_security`` and + ``reviewer_concurrency`` from ADVISORY to CRITICAL: a NACK from + either lens now blocks consensus until the producer re-proposes, + closing #1997. Producers: coder, tester, documenter Reviewers: reviewer_code, reviewer_code_holistic, reviewer_contract, - tester (dual-role), reviewer_security (advisory), - reviewer_concurrency (advisory) + tester (dual-role), reviewer_security, reviewer_concurrency """ return ReviewGraph( [ @@ -251,14 +250,14 @@ def get_default_implement_graph() -> ReviewGraph: ReviewEdge("tester", "coder", ReviewCriticality.CRITICAL), # reviewer_code reviews documenter (advisory) ReviewEdge("reviewer_code", "documenter", ReviewCriticality.ADVISORY), - # reviewer_security reviews coder (advisory — security lens) - ReviewEdge("reviewer_security", "coder", ReviewCriticality.ADVISORY), - # reviewer_security reviews tester (advisory — security lens) - ReviewEdge("reviewer_security", "tester", ReviewCriticality.ADVISORY), - # reviewer_concurrency reviews coder (advisory — concurrency lens) - ReviewEdge("reviewer_concurrency", "coder", ReviewCriticality.ADVISORY), - # reviewer_concurrency reviews tester (advisory — concurrency lens) - ReviewEdge("reviewer_concurrency", "tester", ReviewCriticality.ADVISORY), + # reviewer_security reviews coder (critical — security lens, #2139) + ReviewEdge("reviewer_security", "coder", ReviewCriticality.CRITICAL), + # reviewer_security reviews tester (critical — security lens, #2139) + ReviewEdge("reviewer_security", "tester", ReviewCriticality.CRITICAL), + # reviewer_concurrency reviews coder (critical — concurrency lens, #2139) + ReviewEdge("reviewer_concurrency", "coder", ReviewCriticality.CRITICAL), + # reviewer_concurrency reviews tester (critical — concurrency lens, #2139) + ReviewEdge("reviewer_concurrency", "tester", ReviewCriticality.CRITICAL), ] ) diff --git a/orchestrator/routes/pipelines.py b/orchestrator/routes/pipelines.py index 44587526cf..89fa81d395 100644 --- a/orchestrator/routes/pipelines.py +++ b/orchestrator/routes/pipelines.py @@ -3527,7 +3527,7 @@ def _get_code_review_holistic_criteria(repo_path: str | None = None) -> str: "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" + "- Defer line-by-line correctness to `reviewer_code`.\n" ) @@ -3610,9 +3610,9 @@ def _get_reviewer_scope_preamble(reviewer_type: str, phase: str) -> str: 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 " + "You run alongside `reviewer_code` — your job is the " + "cross-module coherence question line-by-line review does not " + "own. **Don't verify every line; `reviewer_code` 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 " @@ -3625,9 +3625,9 @@ def _get_reviewer_scope_preamble(reviewer_type: str, phase: str) -> str: "(`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" + "own — it is not averaged against `reviewer_code`'s " + "verdict. If the architectural-coherence question fails, " + "NACK even when the line-by-line review is clean.\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 " @@ -3664,9 +3664,11 @@ def _get_reviewer_scope_preamble(reviewer_type: str, phase: str) -> str: ) elif reviewer_type == "security": return ( - "This is an ADVISORY **security-lens review** (issue #1965). " - "Focus ONLY on the security lens; defer code quality, performance, " - "and non-security findings to `reviewer_code`.\n\n" + "This is a CRITICAL **security-lens review** (issue #2139). " + "A NACK from this lens blocks consensus until the producer " + "re-proposes. Focus ONLY on the security lens; defer code " + "quality, performance, and non-security findings to " + "`reviewer_code`.\n\n" "**Lens scope:** cross-file allowlist mismatches, " "handler-vs-validator path mismatches, information-disclosure / " "authorization-bypass patterns at trust boundaries, " @@ -3684,9 +3686,11 @@ def _get_reviewer_scope_preamble(reviewer_type: str, phase: str) -> str: ) elif reviewer_type == "concurrency": return ( - "This is an ADVISORY **concurrency-lens review** (issue #1965). " - "Focus ONLY on the concurrency lens; defer code quality, " - "performance, and non-concurrency findings to `reviewer_code`.\n\n" + "This is a CRITICAL **concurrency-lens review** (issue #2139). " + "A NACK from this lens blocks consensus until the producer " + "re-proposes. Focus ONLY on the concurrency lens; defer code " + "quality, performance, and non-concurrency findings to " + "`reviewer_code`.\n\n" "**Lens scope:** race conditions, deadlocks, shared-state " "mutation without synchronization, async-context leakage, " "retry-storm patterns, resource-cleanup ordering bugs, and " @@ -4616,22 +4620,12 @@ def _build_review_prompt( last_reviewed_commit: str | None = None, base_branch: str | None = None, concurrent: bool = False, - reviewer_code_parallel: bool = True, ) -> str: """Build a review prompt for the reviewer agent. In sequential mode, tells the reviewer to write a typed verdict JSON file to .egg-state/reviews/. In concurrent (BRC) mode, the reviewer's ACK/NACK reason IS the review output — no verdict file is written. - - The ``reviewer_code_parallel`` flag (issue #1965) controls whether the - code reviewer's subagent fan-out spawns partitions in parallel - (default ``True``) or sequentially. The flag is honoured only when - ``reviewer_type == "code"`` and ``phase == "implement"`` — other - reviewer types and phases are unaffected. Callers without a contract - (e.g. unit tests) can leave the flag at its default; see - :func:`shared.egg_contracts.models.get_reviewer_code_parallel` for - the production accessor. """ draft_path = _get_draft_path(phase, issue_number=issue_number, pipeline_id=pipeline_id) @@ -4690,20 +4684,19 @@ def _build_review_prompt( # 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 + # systematically and evaluates against the code-review 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". + # passes from the holistic criteria file. See issue #2126 — the prior + # unified wording told the holistic reviewer to "review every changed + # file systematically", which contradicted the holistic criteria's + # "don't verify every line". 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" + "re-verify every line; that is the code reviewer's job" ) else: lines.append("2. Get the full diff and **review every changed file systematically**") @@ -4746,148 +4739,6 @@ def _build_review_prompt( "a few problems. You are the last line of defense before code reaches " "production." ) - - # Subagent Fan-Out Strategy (issue #1965). - # Restricted to ``reviewer_type == "code"`` AND ``phase == "implement"`` - # so future reuse on other phases / reviewers does not silently - # inherit the block. Delta reviews (cycle > 1 with a known - # last-reviewed commit) skip the fan-out section: the delta-only - # `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. - # 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") - lines.append( - "On large diffs, fan out into Claude Agent SDK `Task` subagents " - "so every changed file is read carefully. Follow these rules — " - "they exist because PR #1964 shipped two cross-file mismatches " - "(`sandbox/scripts/jira` symlink, `^project$` allowlist bypass) " - "that the single-pass reviewer missed.\n" - ) - lines.append( - "1. **Measure first.** Run " - f"`git diff --numstat {_base_ref}...HEAD` and capture " - "`(files_changed, loc_added + loc_removed)`. Emit a " - "`mcp__brc__send_heartbeat` (state=WORKING) with body " - '"fan-out: enabled (files=X, loc=Y, partitions=N)" or ' - '"fan-out: skipped (files=X, loc=Y)" — the gate decision ' - "MUST be observable in the heartbeat log so silent " - "always-solo / always-fan-out drift is visible in telemetry." - ) - lines.append( - "2. **Threshold gate (OR).** Fan out when " - "`files_changed > 10` OR `(loc_added + loc_removed) > 500`. " - "Below the threshold, review the diff yourself in a single " - 'pass — emit the "fan-out: skipped" heartbeat and continue ' - "with the rest of this prompt. **The Mandatory " - "Cross-Partition Consistency Pass below still runs** — you " - "are reviewing the full diff anyway." - ) - lines.append( - "3. **Above the threshold, partition by implement-phase task.** " - "Call `mcp__sdlc__show_contract` and self-extract " - "`phases.implement.tasks[]`. Each task's `files_affected` " - "list becomes a partition spec (a list of path globs). " - "(Older plans may surface the legacy key `files` instead of " - "`files_affected`; tolerate both.) If a task has an empty " - "`files_affected` list, treat that task as covering the full " - "diff and either group it with an adjacent task whose globs " - "are populated, or fall back to single-pass review per the " - "Fallbacks rule." - ) - lines.append( - "4. **Fallbacks.** If the `mcp__sdlc__show_contract` call " - 'FAILS or is unreachable, emit a "fan-out: aborted (mcp ' - 'unavailable)" heartbeat (state=WORKING) and fall back to ' - "single-pass review. If the implement-phase task list is " - "EMPTY (custom-phase invocation, contractless `babysit_pr`), " - 'emit "fan-out: skipped (no implement tasks)" and fall back ' - "to single-pass review. Do NOT attempt to invent partitions. " - "**The Mandatory Cross-Partition Consistency Pass below " - "still runs in both fallback paths.**" - ) - lines.append( - "5. **Cap at 6 subagents.** Never spawn more than 6 subagents " - "per fan-out (capped at 6). If the partition list exceeds " - "6 entries, group adjacent tasks (by file-path-prefix " - "proximity) into combined partitions to stay at or below " - "the cap. Each subagent receives ONLY: its partition spec " - "(file globs), the reviewer-code criteria above, the diff " - "command, an explicit recursion ban, and the timeout below." - ) - lines.append( - "6. **Per-subagent wall-clock cap: 5 minutes (300 seconds).** " - "Each subagent re-runs `git diff` itself, filters its slice " - "by path glob, reads only its partition, and returns a " - "structured finding list to you. If a subagent exceeds the " - "5-minute / 300-second cap, treat the partition as a NACK " - 'with reason "subagent timeout" and propagate the NACK to ' - "the overall verdict." - ) - lines.append( - "7. **Aggregate and emit.** You (the parent reviewer) emit " - "the single ACK / NACK that covers ALL partitions plus the " - "cross-partition pass. Subagents do NOT emit ACK / NACK on " - "their own — they return findings to you and you decide." - ) - lines.append( - "8. **Parallelism.** Spawn the subagents " - f"**{_parallel_word}** " - "(per the resolved per-pipeline knob " - "`phase_configs.implement.reviewer_code.parallel`)." - ) - lines.append( - "9. **No recursion.** subagents must NOT spawn their own " - "subagents. Recursive fan-out is forbidden — it produces " - "untraceable cost and timeout cascades. State the " - "prohibition verbatim to each subagent in its prompt." - ) - lines.append("") - # Mandatory cross-partition consistency pass — runs in ALL - # fan-out paths within this gated block (above-threshold fan-out, - # below-threshold solo, empty-tasks fallback, mcp-unavailable - # fallback). NOTE: this whole section is gated by - # ``phase == "implement" and not is_delta_review`` above, so - # delta reviews (cycle > 1) do not get this pass — the - # delta-only `git log A..HEAD` command is small by construction - # and the parent's cross-partition pass would contradict the - # delta-only directive. Lifted out of the numbered fan-out - # steps per reviewer_code feedback so it cannot be - # short-circuited when the reviewer takes a single-pass branch - # of the fan-out flow. - lines.append("## Mandatory Cross-Partition Consistency Pass\n") - lines.append( - "Regardless of whether you fan out or review solo (and " - "regardless of whether the partition fetch hit either " - "fallback), BEFORE you emit the final verdict, read the " - f"full diff (`git diff {_base_ref}...HEAD`) yourself and " - "run a cross-partition consistency pass focused on the " - "cross-file invariants no single-partition subagent could " - "catch. **This pass is mandatory** — small diffs and " - "fallback paths are not exempt; the failure modes the issue " - "was filed to fix are cross-file mismatches and a small PR " - "with the same shape would otherwise slip through." - ) - lines.append( - "At minimum, check: handler ↔ allowlist consistency " - "(the PR #1964 `^project$` pattern — a handler in one " - "file references an allowlist defined or extended in " - "another file); route ↔ schema consistency; " - "fixture ↔ Dockerfile / symlink reference consistency (the " - "PR #1964 `sandbox/scripts/jira` pattern); import-graph " - "cycles introduced by the diff; and any pattern where a " - "check exists in one file but the call site in another " - "file is unguarded. Merge cross-partition findings into " - "the aggregated findings (whether from subagents or your " - "own solo review) before emitting the verdict." - ) - lines.append("") elif draft_path: # Expanded procedural steps for draft-based (non-code) reviewers lines.append("2. Read the draft thoroughly — do not skim") @@ -8398,7 +8249,7 @@ def _build_reviewer_preparation( "(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. " + "`reviewer_code`'s line-by-line 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 " @@ -8463,9 +8314,9 @@ def _build_reviewer_preparation( "(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." + "in mind — do not verify line-by-line; defer that to " + "`reviewer_code`. Your job is the architectural-coherence " + "question line-by-line review does not own." ) elif role_value == "reviewer_contract": return ( @@ -9227,44 +9078,6 @@ def _build_agent_prompt( elif role_value.startswith("reviewer_"): # Delegate to the detailed review prompt with criteria and verdict format reviewer_type = role_value.replace("reviewer_", "", 1).replace("_", "-") - # Resolve the per-pipeline reviewer_code parallelism knob (issue #1965). - # The accessor handles every fall-through case (no contract, no - # phase_configs, no implement key, no reviewer_code field) and returns - # ``True`` as the default. Pre-importing here keeps the call site - # free of conditional contract loading when this branch isn't taken. - _reviewer_code_parallel = True - if reviewer_type == "code" and phase == "implement" and repo_path: - try: - from egg_contracts.loader import ( - ContractNotFoundError, - ContractValidationError, - load_contract, - ) - from egg_contracts.models import ( - get_reviewer_code_parallel as _get_reviewer_code_parallel, - ) - - _contract = load_contract(pipeline_id, Path(repo_path)) - _reviewer_code_parallel = _get_reviewer_code_parallel(_contract) - except ( - ImportError, - ContractNotFoundError, - ContractValidationError, - ) as _knob_err: - # Narrow catch: missing loader module, missing contract file - # (babysit_pr / contractless flows), or contract schema - # validation failure. ``load_contract`` raises its own - # ``ContractNotFoundError`` / ``ContractValidationError`` - # (Exception subclasses, not ``FileNotFoundError`` / - # ``ValueError``), so they must be named explicitly here. - # Surface in logs so genuine issues are observable, but - # never let prompt construction fail — fall back to the - # parallel default. - logger.warning( - "Failed to resolve reviewer_code_parallel knob; falling back to True. error=%s", - _knob_err, - ) - _reviewer_code_parallel = True review_prompt = _build_review_prompt( phase=phase, pipeline_id=pipeline_id, @@ -9276,7 +9089,6 @@ def _build_agent_prompt( repo_path=repo_path, base_branch=base_branch, concurrent=concurrent, - reviewer_code_parallel=_reviewer_code_parallel, ) if concurrent: review_prompt += "\n" + _build_brc_preamble( diff --git a/orchestrator/tests/test_lens_reviewer_prompts.py b/orchestrator/tests/test_lens_reviewer_prompts.py index 256ab59214..9a9231bebc 100644 --- a/orchestrator/tests/test_lens_reviewer_prompts.py +++ b/orchestrator/tests/test_lens_reviewer_prompts.py @@ -206,6 +206,42 @@ def test_concurrency_preamble_avoids_self_contradictory_phrasing(self) -> None: "concurrency." ) + def test_security_preamble_is_critical_not_advisory(self) -> None: + """Regression for PR #2152: the security lens is CRITICAL per #2139. + + The preamble must NOT call the review ADVISORY (the prior wording + before lens promotion) and must announce CRITICAL gating with a + #2139 reference so prompt drift can't quietly revert it. + """ + preamble = _get_reviewer_scope_preamble("security", "implement") + assert "ADVISORY" not in preamble, ( + "Security preamble must not label the review ADVISORY — the " + "lens was promoted to CRITICAL in #2139." + ) + assert "CRITICAL" in preamble, ( + "Security preamble must announce CRITICAL gating to match the " + "review-graph edge (orchestrator/review_graph.py)." + ) + assert "#2139" in preamble, ( + "Security preamble must reference #2139 (the lens-promotion " + "issue) so the gating rationale is traceable." + ) + + def test_concurrency_preamble_is_critical_not_advisory(self) -> None: + preamble = _get_reviewer_scope_preamble("concurrency", "implement") + assert "ADVISORY" not in preamble, ( + "Concurrency preamble must not label the review ADVISORY — the " + "lens was promoted to CRITICAL in #2139." + ) + assert "CRITICAL" in preamble, ( + "Concurrency preamble must announce CRITICAL gating to match " + "the review-graph edge (orchestrator/review_graph.py)." + ) + assert "#2139" in preamble, ( + "Concurrency preamble must reference #2139 (the lens-promotion " + "issue) so the gating rationale is traceable." + ) + def test_security_preamble_warns_about_brc_minimum_content_length(self) -> None: """The security preamble warns the agent about the BRC content-length floor. diff --git a/orchestrator/tests/test_peer_consensus_integration.py b/orchestrator/tests/test_peer_consensus_integration.py index c8f708ca6d..afb59f2339 100644 --- a/orchestrator/tests/test_peer_consensus_integration.py +++ b/orchestrator/tests/test_peer_consensus_integration.py @@ -776,7 +776,7 @@ def test_full_implement_graph(self): "reviewer_code_holistic", "tester", {"artifact_references": ["tests/test_main.py"]} ) - # Lens reviewers (advisory) ACK coder and tester + # Lens reviewers (critical) ACK coder and tester t.handle_ack( "reviewer_security", "coder", {"artifact_references": ["src/main.py", "src/utils.py"]} ) diff --git a/orchestrator/tests/test_pipeline_prompts.py b/orchestrator/tests/test_pipeline_prompts.py index 21c29a5b10..427d864b53 100644 --- a/orchestrator/tests/test_pipeline_prompts.py +++ b/orchestrator/tests/test_pipeline_prompts.py @@ -1374,73 +1374,6 @@ def test_no_issue_number_omits_issue_line(self): ) assert "Issue: #" not in result - def test_reviewer_code_implement_with_missing_contract_file(self): - """Reviewer-code prompt builds even when the contract file is absent. - - Regression for the egg-reviewer feedback on PR #2061: the - ``reviewer_code_parallel`` knob loader caught - ``(ImportError, FileNotFoundError, ValueError)``, but - ``load_contract`` actually raises ``ContractNotFoundError`` / - ``ContractValidationError`` (Exception subclasses, not - ``FileNotFoundError`` / ``ValueError``). On the missing-contract - path (``babysit_pr``, contractless flows) prompt construction - would crash instead of falling back to the parallel default. - - With the catch widened to include the contract-loader - exceptions, building the reviewer prompt with a tmp ``repo_path`` - that has no contract file must now succeed and still emit the - fan-out section (so we know we hit the reviewer-code branch and - the fall-back default of ``parallel=True`` was applied). - """ - with tempfile.TemporaryDirectory() as tmpdir: - # No .egg-state/contracts/ directory created — load_contract - # will raise ContractNotFoundError and the prompt builder - # must swallow it and fall back to parallel=True. - result = _build_agent_prompt( - role_value="reviewer_code", - phase="implement", - pipeline_id="issue-9999", - pipeline_mode="issue", - prompt="# Some feature", - issue_number=9999, - repo_path=tmpdir, - ) - assert "Subagent Fan-Out Strategy" in result - # ``parallel=True`` default must be honoured — the parallel - # ordering directive should be present. - assert "in parallel" in result.lower() - - def test_reviewer_code_implement_with_corrupt_contract_file(self): - """Reviewer-code prompt builds even when the contract JSON is malformed. - - Sibling regression for ``ContractValidationError``: ``load_contract`` - raises this exception when the JSON cannot be decoded or when - ``Contract.model_validate`` rejects the payload (loader.py:131-134). - Like ``ContractNotFoundError``, ``ContractValidationError`` is a - direct ``Exception`` subclass — not ``ValueError`` — so the prior - catch on ``(ImportError, FileNotFoundError, ValueError)`` would have - leaked it. Widening the catch to include the contract-loader - exceptions must keep the corrupt-contract path falling back to - ``parallel=True`` rather than crashing the prompt build. - """ - with tempfile.TemporaryDirectory() as tmpdir: - contracts_dir = Path(tmpdir) / ".egg-state" / "contracts" - contracts_dir.mkdir(parents=True) - # Malformed JSON triggers ContractValidationError via the - # JSONDecodeError branch in load_contract(). - (contracts_dir / "issue-9999.json").write_text("{not json") - result = _build_agent_prompt( - role_value="reviewer_code", - phase="implement", - pipeline_id="issue-9999", - pipeline_mode="issue", - prompt="# Some feature", - issue_number=9999, - repo_path=tmpdir, - ) - assert "Subagent Fan-Out Strategy" in result - assert "in parallel" in result.lower() - class TestNamespacedOutputFilenames: """Tests for namespaced (identifier-prefixed) output filenames in prompts.""" diff --git a/orchestrator/tests/test_review_graph_advisory_reviewers.py b/orchestrator/tests/test_review_graph_lens_reviewers.py similarity index 70% rename from orchestrator/tests/test_review_graph_advisory_reviewers.py rename to orchestrator/tests/test_review_graph_lens_reviewers.py index 4c030e23c8..e3b82801b4 100644 --- a/orchestrator/tests/test_review_graph_advisory_reviewers.py +++ b/orchestrator/tests/test_review_graph_lens_reviewers.py @@ -1,19 +1,18 @@ -"""Review-graph wiring for the new lens reviewers (issue #1965 / TASK-1-3 (a)). +"""Review-graph wiring for the security / concurrency lens reviewers. -Asserts the four new ADVISORY edges added to ``get_default_implement_graph()``: +Asserts the four lens edges added to ``get_default_implement_graph()``: -- ``("reviewer_security", "coder", ADVISORY)`` -- ``("reviewer_security", "tester", ADVISORY)`` -- ``("reviewer_concurrency", "coder", ADVISORY)`` -- ``("reviewer_concurrency", "tester", ADVISORY)`` +- ``("reviewer_security", "coder", CRITICAL)`` +- ``("reviewer_security", "tester", CRITICAL)`` +- ``("reviewer_concurrency", "coder", CRITICAL)`` +- ``("reviewer_concurrency", "tester", CRITICAL)`` …and that the existing CRITICAL edges (``reviewer_code → coder/tester``, ``reviewer_contract → coder``, ``tester → coder``) are unchanged. -The new edges are ADVISORY by design: they cannot deadlock consensus on -day 1 — promotion to CRITICAL waits for #1997's severity-tagged NACK -signalling. +Issue #2139 promoted both lens reviewers from ADVISORY to CRITICAL: a +NACK from either now blocks consensus until the producer re-proposes. """ from __future__ import annotations @@ -24,33 +23,33 @@ ) -class TestNewLensReviewersAdvisoryEdges: - def test_security_reviews_coder_advisory(self) -> None: +class TestLensReviewersCriticalEdges: + def test_security_reviews_coder_critical(self) -> None: graph = get_default_implement_graph() edge = graph.get_edge("reviewer_security", "coder") assert edge is not None, "reviewer_security → coder edge missing" - assert edge.criticality is ReviewCriticality.ADVISORY + assert edge.criticality is ReviewCriticality.CRITICAL - def test_security_reviews_tester_advisory(self) -> None: + def test_security_reviews_tester_critical(self) -> None: graph = get_default_implement_graph() edge = graph.get_edge("reviewer_security", "tester") assert edge is not None, "reviewer_security → tester edge missing" - assert edge.criticality is ReviewCriticality.ADVISORY + assert edge.criticality is ReviewCriticality.CRITICAL - def test_concurrency_reviews_coder_advisory(self) -> None: + def test_concurrency_reviews_coder_critical(self) -> None: graph = get_default_implement_graph() edge = graph.get_edge("reviewer_concurrency", "coder") assert edge is not None, "reviewer_concurrency → coder edge missing" - assert edge.criticality is ReviewCriticality.ADVISORY + assert edge.criticality is ReviewCriticality.CRITICAL - def test_concurrency_reviews_tester_advisory(self) -> None: + def test_concurrency_reviews_tester_critical(self) -> None: graph = get_default_implement_graph() edge = graph.get_edge("reviewer_concurrency", "tester") assert edge is not None, "reviewer_concurrency → tester edge missing" - assert edge.criticality is ReviewCriticality.ADVISORY + assert edge.criticality is ReviewCriticality.CRITICAL -class TestNewLensReviewersInReviewersForProducer: +class TestLensReviewersInReviewersForProducer: def test_coder_reviewers_include_lens(self) -> None: graph = get_default_implement_graph() reviewers = graph.reviewers_for("coder") @@ -63,24 +62,21 @@ def test_tester_reviewers_include_lens(self) -> None: assert "reviewer_security" in reviewers assert "reviewer_concurrency" in reviewers - def test_advisory_reviewers_for_coder_include_lens(self) -> None: + def test_critical_reviewers_for_coder_include_lens(self) -> None: graph = get_default_implement_graph() - advisory = graph.advisory_reviewers_for("coder") - assert "reviewer_security" in advisory - assert "reviewer_concurrency" in advisory + critical = graph.critical_reviewers_for("coder") + assert "reviewer_security" in critical + assert "reviewer_concurrency" in critical - def test_advisory_reviewers_for_tester_include_lens(self) -> None: + def test_critical_reviewers_for_tester_include_lens(self) -> None: graph = get_default_implement_graph() - advisory = graph.advisory_reviewers_for("tester") - assert "reviewer_security" in advisory - assert "reviewer_concurrency" in advisory + critical = graph.critical_reviewers_for("tester") + assert "reviewer_security" in critical + assert "reviewer_concurrency" in critical class TestExistingCriticalEdgesUnchanged: - """Regression guard: previous CRITICAL edges must remain CRITICAL. - - If a future PR accidentally demotes one, this test fires. - """ + """Regression guard: previous CRITICAL edges must remain CRITICAL.""" def test_reviewer_code_coder_still_critical(self) -> None: graph = get_default_implement_graph() @@ -108,7 +104,7 @@ def test_tester_coder_still_critical(self) -> None: class TestLensReviewersDoNotReviewDocumenter: - """Plan adds edges for coder + tester only; documenter is unaffected.""" + """Lens edges cover coder + tester only; documenter is unaffected.""" def test_documenter_reviewer_security_absent(self) -> None: graph = get_default_implement_graph() diff --git a/orchestrator/tests/test_reviewer_code_fan_out_prompt.py b/orchestrator/tests/test_reviewer_code_fan_out_prompt.py deleted file mode 100644 index e25d113c34..0000000000 --- a/orchestrator/tests/test_reviewer_code_fan_out_prompt.py +++ /dev/null @@ -1,316 +0,0 @@ -"""Always-on prompt-text asserts for the ``reviewer_code`` subagent fan-out block. - -Covers TASK-4-3 of issue #1965. The ``_build_review_prompt`` function -must emit a "Subagent Fan-Out Strategy" section ONLY for -``reviewer_type="code"`` AND ``phase="implement"``. The block carries -several literal markers that — taken together — verify the gate logic, -the partition-fetch instruction with both fallbacks, the parallelism -knob, the recursion ban, the cross-partition consistency pass, and the -gate-decision STATUS heartbeat instrumentation. - -These asserts are deterministic — they do not run the LLM. They fire -with a clear message if any of the following drift: - -- The fan-out block is removed. -- The threshold values (10 files / 500 LOC) drift. -- The 6-subagent cap or the 5-minute / 300-second timeout drops out. -- The recursion ban disappears. -- The parent cross-partition pass disappears. -- The STATUS-heartbeat instrumentation disappears. -- The mcp-unavailable fallback disappears. -- The block leaks to non-code reviewers or to non-implement phases. -- The ``reviewer_code_parallel`` kwarg stops being honoured. -""" - -from __future__ import annotations - -import sys -from unittest.mock import MagicMock - -import pytest - -# Stub Docker the same way test_pipeline_prompts.py does. -_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 routes.pipelines import _build_review_prompt # noqa: E402 - -# --------------------------------------------------------------------------- -# Block presence — only on (reviewer_type='code', phase='implement'). -# --------------------------------------------------------------------------- - - -class TestFanOutBlockPresence: - def test_present_for_code_reviewer_in_implement_phase(self) -> None: - prompt = _build_review_prompt( - phase="implement", - pipeline_id="test-pipe", - pipeline_mode="issue", - reviewer_type="code", - issue_number=100, - ) - assert "Subagent Fan-Out Strategy" in prompt - - @pytest.mark.parametrize( - "reviewer_type", - ["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. - phase_for_type = { - "contract": "implement", - "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, - pipeline_id="test-pipe", - pipeline_mode="issue", - reviewer_type=reviewer_type, - issue_number=100, - ) - assert "Subagent Fan-Out Strategy" not in prompt, ( - f"reviewer_type={reviewer_type!r} should not include the " - "subagent fan-out block — it's exclusive to reviewer_type='code'." - ) - - def test_absent_for_code_reviewer_in_plan_phase(self) -> None: - """The block is gated on phase='implement' even for 'code' reviewers.""" - prompt = _build_review_prompt( - phase="plan", - pipeline_id="test-pipe", - pipeline_mode="issue", - reviewer_type="code", - issue_number=100, - ) - assert "Subagent Fan-Out Strategy" not in prompt, ( - "Fan-out is implement-phase only; plan-phase reviewer_code must not see the block." - ) - - -# --------------------------------------------------------------------------- -# Threshold + numstat instrumentation. -# --------------------------------------------------------------------------- - - -class TestFanOutThresholdInstrumentation: - def setup_method(self) -> None: - self.prompt = _build_review_prompt( - phase="implement", - pipeline_id="test-pipe", - pipeline_mode="issue", - reviewer_type="code", - issue_number=100, - ) - - def test_numstat_command_mentioned(self) -> None: - assert "git diff --numstat" in self.prompt - - def test_files_threshold_present(self) -> None: - # The threshold rule is `files_changed > 10` (or any equivalent - # phrasing) — the literal '10' must appear next to 'files'. - assert ( - "files_changed > 10" in self.prompt - or "files > 10" in self.prompt - or "10 files" in self.prompt - or "files_changed > 10 OR" in self.prompt - ) - - def test_loc_threshold_present(self) -> None: - # The plan picks 500 as the LOC threshold; either form is fine - # provided the literal '500' appears in the threshold context. - assert "500" in self.prompt - - def test_status_heartbeat_enabled_marker(self) -> None: - assert "fan-out: enabled" in self.prompt - - def test_status_heartbeat_skipped_marker(self) -> None: - assert "fan-out: skipped" in self.prompt - - -# --------------------------------------------------------------------------- -# Partition-list fetch + fallbacks. -# --------------------------------------------------------------------------- - - -class TestPartitionFetchAndFallbacks: - def setup_method(self) -> None: - self.prompt = _build_review_prompt( - phase="implement", - pipeline_id="test-pipe", - pipeline_mode="issue", - reviewer_type="code", - issue_number=100, - ) - - def test_mcp_show_contract_fetch_instruction(self) -> None: - assert "mcp__sdlc__show_contract" in self.prompt - - def test_implement_tasks_path_mentioned(self) -> None: - assert "phases.implement.tasks" in self.prompt - - def test_empty_task_list_fallback_mentioned(self) -> None: - # The prompt must explicitly tell the reviewer how to fall back - # when phases.implement.tasks is empty (custom-phase invocation, - # contractless babysit_pr). - prompt_lower = self.prompt.lower() - assert ( - "no implement tasks" in prompt_lower - or "empty implement-phase task" in prompt_lower - or "empty task list" in prompt_lower - ), ( - "Fan-out block must describe the empty-task-list fallback. " - "Look for 'no implement tasks', 'empty implement-phase task', " - "or similar." - ) - - def test_mcp_unavailable_fallback_mentioned(self) -> None: - prompt_lower = self.prompt.lower() - assert ( - "mcp unavailable" in prompt_lower - or "fallback to single-pass" in prompt_lower - or "parent fetches" in prompt_lower - ), ( - "Fan-out block must describe the mcp-unavailable fallback " - "(parent fetches contract OR fall back to single-pass)." - ) - - -# --------------------------------------------------------------------------- -# Subagent cap + per-subagent timeout. -# --------------------------------------------------------------------------- - - -class TestSubagentCapAndTimeout: - def setup_method(self) -> None: - self.prompt = _build_review_prompt( - phase="implement", - pipeline_id="test-pipe", - pipeline_mode="issue", - reviewer_type="code", - issue_number=100, - ) - - def test_subagent_cap_mentioned(self) -> None: - assert ("capped at 6" in self.prompt) or ( - "never spawn more than 6 subagents" in self.prompt - ), ( - "Fan-out block must cap subagents at 6 — look for 'capped at " - "6' or 'never spawn more than 6 subagents'." - ) - - def test_per_subagent_timeout_mentioned(self) -> None: - assert ("5 minutes" in self.prompt) or ("300 seconds" in self.prompt), ( - "Fan-out block must specify a 5-minute / 300-second per-subagent wall-clock cap." - ) - - -# --------------------------------------------------------------------------- -# Recursion ban. -# --------------------------------------------------------------------------- - - -class TestRecursionBan: - def test_recursion_ban_literal(self) -> None: - prompt = _build_review_prompt( - phase="implement", - pipeline_id="test-pipe", - pipeline_mode="issue", - reviewer_type="code", - issue_number=100, - ) - assert "subagents must NOT spawn their own subagents" in prompt, ( - "Fan-out block must contain the literal recursion ban " - "'subagents must NOT spawn their own subagents'." - ) - - -# --------------------------------------------------------------------------- -# Parent cross-partition consistency pass. -# --------------------------------------------------------------------------- - - -class TestParentCrossPartitionPass: - def setup_method(self) -> None: - self.prompt = _build_review_prompt( - phase="implement", - pipeline_id="test-pipe", - pipeline_mode="issue", - reviewer_type="code", - issue_number=100, - ) - - def test_cross_partition_marker(self) -> None: - assert "cross-partition" in self.prompt.lower() - - def test_handler_marker(self) -> None: - # The PR #1964 motivating example: handler ↔ allowlist. - assert "handler" in self.prompt.lower() - - def test_allowlist_marker(self) -> None: - assert "allowlist" in self.prompt.lower() - - -# --------------------------------------------------------------------------- -# reviewer_code_parallel kwarg. -# --------------------------------------------------------------------------- - - -class TestReviewerCodeParallelKwarg: - def test_default_says_in_parallel(self) -> None: - prompt = _build_review_prompt( - phase="implement", - pipeline_id="test-pipe", - pipeline_mode="issue", - reviewer_type="code", - issue_number=100, - ) - assert "in parallel" in prompt.lower() - - def test_explicit_true_says_in_parallel(self) -> None: - prompt = _build_review_prompt( - phase="implement", - pipeline_id="test-pipe", - pipeline_mode="issue", - reviewer_type="code", - issue_number=100, - reviewer_code_parallel=True, - ) - assert "in parallel" in prompt.lower() - - def test_explicit_false_says_sequentially(self) -> None: - prompt = _build_review_prompt( - phase="implement", - pipeline_id="test-pipe", - pipeline_mode="issue", - reviewer_type="code", - issue_number=100, - reviewer_code_parallel=False, - ) - prompt_lower = prompt.lower() - assert "sequentially" in prompt_lower - # And the parallel-only language should not be the dominant - # instruction; we tolerate the word "parallel" appearing in - # surrounding prose but the explicit ordering directive should - # use 'sequentially'. - - def test_kwarg_accepted_signature(self) -> None: - """The kwarg must be optional, default True, and not raise.""" - # Pass a few permutations to make sure the signature accepts the - # kwarg without TypeErrors. - for value in (True, False): - _build_review_prompt( - phase="implement", - pipeline_id="test-pipe", - pipeline_mode="issue", - reviewer_type="code", - issue_number=100, - reviewer_code_parallel=value, - ) diff --git a/orchestrator/tests/test_reviewer_code_holistic.py b/orchestrator/tests/test_reviewer_code_holistic.py index 553ac24c86..b8ffef159d 100644 --- a/orchestrator/tests/test_reviewer_code_holistic.py +++ b/orchestrator/tests/test_reviewer_code_holistic.py @@ -4,11 +4,11 @@ ``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). + review graph as a *distinct CRITICAL* role so its NACK gates + consensus on its own. +2. Run on every implement pipeline (no PR-size gate). +3. Use a holistic-lens prompt (not the line-by-line code review + criteria). These asserts are deterministic — they do not run the LLM. """ @@ -148,21 +148,6 @@ def setup_method(self) -> None: 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() @@ -180,9 +165,9 @@ def test_canonical_use_case_reference(self) -> None: 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, ( + assert "reviewer_code" in prompt_lower or "line-by-line" in prompt_lower, ( "Holistic preamble must frame its job as complementary to " - "reviewer_code's fan-out / slice work." + "reviewer_code's line-by-line work." ) def test_procedural_step_does_not_demand_every_file_review(self) -> None: @@ -193,13 +178,13 @@ def test_procedural_step_does_not_demand_every_file_review(self) -> None: 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. + prompt does not regress to the line-by-line wording. """ assert "review every changed file systematically" not in self.prompt, ( - "Holistic procedural step 2 must not include the slice-style " + "Holistic procedural step 2 must not include the line-by-line " '"review every changed file systematically" wording — it ' "directly contradicts the holistic criteria's " - "'don't verify every line; the fan-out reviewer covers that' " + "'don't verify every line; reviewer_code covers that' " "(issue #2126)." ) diff --git a/shared/egg_contracts/agent_roles.py b/shared/egg_contracts/agent_roles.py index 79ed68f78c..9cfd1833dc 100644 --- a/shared/egg_contracts/agent_roles.py +++ b/shared/egg_contracts/agent_roles.py @@ -519,11 +519,12 @@ def depends_on(self, other: AgentRole) -> bool: ) # 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. +# Skims the full diff once and runs four cross-module passes rather +# than verifying every line — that is ``reviewer_code``'s job. Its +# focus is the architectural-coherence question line-by-line review +# does not own: 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", diff --git a/shared/egg_contracts/models.py b/shared/egg_contracts/models.py index ccaf50f86e..78c3fdc5f6 100644 --- a/shared/egg_contracts/models.py +++ b/shared/egg_contracts/models.py @@ -298,25 +298,6 @@ class CheckResult(BaseModel): fixable: bool = Field(default=False, description="Whether this failure can be auto-fixed") -class ReviewerCodeConfig(BaseModel): - """Per-pipeline configuration for the ``reviewer_code`` BRC reviewer. - - Currently exposes a single knob: whether subagent fan-out (added in - issue #1965) runs partitions in parallel or sequentially. Default is - parallel; flip to ``False`` to force sequential review for cost or - quota reasons. Absence of this config preserves legacy behaviour - (treated as ``parallel=True``). - """ - - parallel: bool = Field( - default=True, - description=( - "Fan out reviewer_code subagents in parallel (default true). " - "Set false to force sequential review for cost or quota reasons." - ), - ) - - class PhaseConfig(BaseModel): """Configuration for a pipeline phase.""" @@ -330,50 +311,6 @@ class PhaseConfig(BaseModel): default=HumanReviewMechanism.ISSUE_CHECKBOX, description="Mechanism for human review", ) - reviewer_code: ReviewerCodeConfig | None = Field( - default=None, - description=( - "Optional reviewer_code-specific overrides (issue #1965). " - "Absent / None preserves the default (parallel fan-out)." - ), - ) - - -def get_reviewer_code_parallel(contract: Any) -> bool: - """Return whether ``reviewer_code`` should fan out subagents in parallel. - - Centralises the lookup for ``phase_configs[implement].reviewer_code.parallel`` - so callers don't have to plumb three optional layers (``phase_configs`` - is None, ``phase_configs[implement]`` is missing, or - ``phase_configs[implement].reviewer_code`` is None). Default is - ``True`` — parallel fan-out matches the issue #1965 plan default. - - Accepts either a ``Contract`` instance or any object exposing a - ``phase_configs`` attribute / mapping; returns ``True`` when the field - is unreachable. Callers without a contract (e.g. unit tests) can pass - ``None`` to get the default. - """ - if contract is None: - return True - phase_configs = getattr(contract, "phase_configs", None) - if phase_configs is None: - return True - # ``phase_configs`` may be a Pydantic dict[PipelinePhase, PhaseConfig]. - implement_cfg = None - try: - implement_cfg = phase_configs.get(PipelinePhase.IMPLEMENT) - except (AttributeError, TypeError): - # Not a mapping — try the string form for resilient duck typing. - try: - implement_cfg = phase_configs.get("implement") - except (AttributeError, TypeError): # pragma: no cover — defensive - return True - if implement_cfg is None: - return True - reviewer_code = getattr(implement_cfg, "reviewer_code", None) - if reviewer_code is None: - return True - return bool(getattr(reviewer_code, "parallel", True)) class FeedbackQuestion(BaseModel): diff --git a/shared/egg_contracts/tests/test_phase_config_reviewer_code.py b/shared/egg_contracts/tests/test_phase_config_reviewer_code.py deleted file mode 100644 index 8441eee340..0000000000 --- a/shared/egg_contracts/tests/test_phase_config_reviewer_code.py +++ /dev/null @@ -1,188 +0,0 @@ -"""Tests for the new ``PhaseConfig.reviewer_code.parallel`` knob. - -Covers TASK-3-2 of issue #1965: - -- ``ReviewerCodeConfig.parallel`` defaults to ``True``. -- ``PhaseConfig`` accepts an explicit ``reviewer_code=ReviewerCodeConfig(parallel=False)``. -- ``PhaseConfig()`` (no ``reviewer_code``) still validates — backward-compat. -- A ``Contract`` with the new field round-trips through - ``model_dump_json`` / ``model_validate_json`` cleanly. -- A legacy contract JSON without the field still loads. -- ``get_reviewer_code_parallel(contract)`` returns: - * ``True`` when ``phase_configs`` is ``None``. - * ``True`` when implement-phase config is missing. - * ``True`` when ``reviewer_code`` is ``None`` on the implement config. - * ``True`` when explicit ``parallel=True``. - * ``False`` when explicit ``parallel=False``. -""" - -from __future__ import annotations - -from egg_contracts.models import ( - Contract, - IssueInfo, - PhaseConfig, - PipelinePhase, - ReviewerCodeConfig, - get_reviewer_code_parallel, -) - - -def _build_contract( - phase_configs: dict[PipelinePhase, PhaseConfig] | None = None, -) -> Contract: - return Contract( - issue=IssueInfo( - number=1965, - title="dummy", - url="https://github.com/jwbron/egg/issues/1965", - ), - pipeline_id="issue-1965", - phase_configs=phase_configs, - ) - - -# --------------------------------------------------------------------------- -# ReviewerCodeConfig -# --------------------------------------------------------------------------- - - -class TestReviewerCodeConfig: - def test_default_parallel_true(self) -> None: - cfg = ReviewerCodeConfig() - assert cfg.parallel is True - - def test_explicit_parallel_false(self) -> None: - cfg = ReviewerCodeConfig(parallel=False) - assert cfg.parallel is False - - -# --------------------------------------------------------------------------- -# PhaseConfig schema -# --------------------------------------------------------------------------- - - -class TestPhaseConfigReviewerCodeField: - def test_phase_config_default_is_none(self) -> None: - cfg = PhaseConfig() - assert cfg.reviewer_code is None - - def test_phase_config_accepts_explicit_reviewer_code(self) -> None: - cfg = PhaseConfig(reviewer_code=ReviewerCodeConfig(parallel=False)) - assert cfg.reviewer_code is not None - assert cfg.reviewer_code.parallel is False - - def test_phase_config_accepts_dict_reviewer_code(self) -> None: - """Pydantic should coerce a plain dict into the nested model.""" - cfg = PhaseConfig.model_validate({"reviewer_code": {"parallel": False}}) - assert cfg.reviewer_code is not None - assert cfg.reviewer_code.parallel is False - - -# --------------------------------------------------------------------------- -# Contract round-trip -# --------------------------------------------------------------------------- - - -class TestContractRoundTripWithReviewerCodeField: - def test_round_trip_with_explicit_false(self) -> None: - contract = _build_contract( - phase_configs={ - PipelinePhase.IMPLEMENT: PhaseConfig( - reviewer_code=ReviewerCodeConfig(parallel=False), - ) - } - ) - serialized = contract.model_dump_json() - restored = Contract.model_validate_json(serialized) - assert restored.phase_configs is not None - impl = restored.phase_configs[PipelinePhase.IMPLEMENT] - assert impl.reviewer_code is not None - assert impl.reviewer_code.parallel is False - - def test_round_trip_with_default(self) -> None: - contract = _build_contract( - phase_configs={ - PipelinePhase.IMPLEMENT: PhaseConfig( - reviewer_code=ReviewerCodeConfig(), - ) - } - ) - serialized = contract.model_dump_json() - restored = Contract.model_validate_json(serialized) - impl = restored.phase_configs[PipelinePhase.IMPLEMENT] - assert impl.reviewer_code is not None - assert impl.reviewer_code.parallel is True - - def test_legacy_contract_without_field_still_validates(self) -> None: - """JSON written by an older orchestrator (no ``reviewer_code`` field).""" - legacy_payload = { - "schemaVersion": "1.0", - "issue": { - "number": 1234, - "title": "legacy", - "url": "https://github.com/jwbron/egg/issues/1234", - }, - "pipeline_id": "issue-1234", - "current_phase": "implement", - "acceptance_criteria": [], - "phases": [], - "decisions": [], - "phase_configs": { - "implement": { - "checks": [], - "max_review_cycles": 3, - "human_review_mechanism": "ISSUE_CHECKBOX", - # No reviewer_code field — legacy. - } - }, - } - contract = Contract.model_validate(legacy_payload) - impl = contract.phase_configs[PipelinePhase.IMPLEMENT] - assert impl.reviewer_code is None - - def test_contract_with_no_phase_configs_validates(self) -> None: - """``phase_configs=None`` is still valid.""" - contract = _build_contract(phase_configs=None) - assert contract.phase_configs is None - - -# --------------------------------------------------------------------------- -# get_reviewer_code_parallel accessor -# --------------------------------------------------------------------------- - - -class TestGetReviewerCodeParallelAccessor: - def test_returns_true_when_phase_configs_is_none(self) -> None: - contract = _build_contract(phase_configs=None) - assert get_reviewer_code_parallel(contract) is True - - def test_returns_true_when_implement_config_missing(self) -> None: - contract = _build_contract(phase_configs={PipelinePhase.PLAN: PhaseConfig()}) - assert get_reviewer_code_parallel(contract) is True - - def test_returns_true_when_reviewer_code_is_none(self) -> None: - contract = _build_contract( - phase_configs={PipelinePhase.IMPLEMENT: PhaseConfig(reviewer_code=None)} - ) - assert get_reviewer_code_parallel(contract) is True - - def test_returns_true_for_explicit_true(self) -> None: - contract = _build_contract( - phase_configs={ - PipelinePhase.IMPLEMENT: PhaseConfig( - reviewer_code=ReviewerCodeConfig(parallel=True) - ) - } - ) - assert get_reviewer_code_parallel(contract) is True - - def test_returns_false_for_explicit_false(self) -> None: - contract = _build_contract( - phase_configs={ - PipelinePhase.IMPLEMENT: PhaseConfig( - reviewer_code=ReviewerCodeConfig(parallel=False) - ) - } - ) - assert get_reviewer_code_parallel(contract) is False diff --git a/shared/prompts/REVIEWER-SYNC.md b/shared/prompts/REVIEWER-SYNC.md index a153155d76..16fdd6f6f3 100644 --- a/shared/prompts/REVIEWER-SYNC.md +++ b/shared/prompts/REVIEWER-SYNC.md @@ -12,7 +12,7 @@ by their different workflows. | **Trigger** | PR opened/updated via GitHub Actions | SDLC pipeline review phase | | **Output** | Posts `gh pr review` (approve / request-changes / comment) | **Sequential**: JSON verdict to `.egg-state/reviews/`. **Concurrent (BRC)**: ACK/NACK `--reason` is the review output (no verdict file). | | **Conventions** | External file: `action/review-conventions.md` | Inline in `_build_review_prompt()` | -| **Reviewer types** | Code only | Code, contract, agent-design, refine, plan, **security** (ADVISORY), **concurrency** (ADVISORY) | +| **Reviewer types** | Code only | Code, contract, agent-design, refine, plan, **security** (CRITICAL), **concurrency** (CRITICAL) | ## What's Shared (single source of truth) @@ -23,8 +23,8 @@ additional reviewer types. All shared files live in `shared/prompts/`: - `code-review-criteria.md` — security, correctness, robustness, design, severity classification (both reviewers) - `contract-review-criteria.md` — task/contract verification (SDLC reviewer only) - `agent-design-criteria.md` — agent-mode anti-patterns (SDLC reviewer only) -- `security-review-criteria.md` — security lens (SDLC reviewer only; **ADVISORY**, see asymmetries below) -- `concurrency-review-criteria.md` — concurrency lens (SDLC reviewer only; **ADVISORY**, see asymmetries below) +- `security-review-criteria.md` — security lens (SDLC reviewer only; **CRITICAL** per #2139, see asymmetries below) +- `concurrency-review-criteria.md` — concurrency lens (SDLC reviewer only; **CRITICAL** per #2139, see asymmetries below) Each reviewer has an inline fallback for when the shared file can't be loaded. **Inline fallbacks must match the shared file content.** The two new lens @@ -71,42 +71,29 @@ review standards differ: the PR body, and the sequential reviewer's verdict file does not feed the PR. A conditional ACK is **not** a soft NACK: if the producer could address the obligation, NACK instead. -8. **Subagent fan-out (`reviewer_code`, BRC only)**: The SDLC `reviewer_code` - self-gates on diff size (`git diff --numstat` against the resolved base - ref) and, when the diff exceeds ~10 changed files **or** ~500 lines of - change, fans out into Claude Agent SDK `Task` subagents partitioned - along the implement-phase task list pulled from `mcp__sdlc__show_contract` - (#1965). Each subagent reviews only its path-glob slice and is forbidden - from spawning subagents of its own; the parent reviewer then runs a - cross-partition consistency pass (handler ↔ allowlist, route ↔ schema, - fixture ↔ Dockerfile/symlink, import-graph cycles) before emitting its - verdict. Fan-out is capped at 6 subagents per review with a 5-minute / - 300-second per-subagent wall-clock cap, and is configurable per pipeline - via `phase_configs.implement.reviewer_code.parallel` (default `true`). - Below the threshold or when the implement-phase task list is empty - (custom-phase invocation, contractless `babysit_pr`, or MCP unreachable - from the subagent), the reviewer falls back to today's single-pass - review with a STATUS heartbeat noting the gate decision. The PR reviewer - (GHA) does **not** fan out — its prompt is a single pass over the full - `gh pr diff`. This is an SDLC-only asymmetry and lives entirely in - `_build_review_prompt()`'s reviewer-code branch in `pipelines.py`. No - `action/` files are touched. -9. **Lens reviewers (`reviewer_security`, `reviewer_concurrency`, BRC only)**: - The SDLC orchestrator runs two ADVISORY lens reviewers alongside +8. **Lens reviewers (`reviewer_security`, `reviewer_concurrency`, BRC only)**: + The SDLC orchestrator runs two CRITICAL lens reviewers alongside `reviewer_code` on the implement phase: `reviewer_security` (criteria in `shared/prompts/security-review-criteria.md`) and `reviewer_concurrency` (criteria in `shared/prompts/concurrency-review-criteria.md`). Both inherit from `code-review-criteria.md` and add lens-specific patterns (cross-file allowlist mismatches, handler-vs-validator path mismatches, uncommitted-artifact / Dockerfile-symlink mismatches, retry storms, - BRC-protocol invariants, …). Day-1 ADVISORY criticality means their - NACKs are recorded but do not deadlock consensus; promotion to CRITICAL - waits for #1997's severity-tagged NACK signalling. The GHA `egg-reviewer` - has **no** lens reviewers — code review is a single pass at the - `code-review-criteria.md` lens. The asymmetry is intentional: the GHA - reviewer fires on a small, already-merged-style PR; the SDLC reviewer - fires on the full implement-phase change set during a still-mutating - pipeline. + BRC-protocol invariants, …). A NACK from either lens blocks consensus + until the producer re-proposes (#2139, closing #1997). The GHA + `egg-reviewer` has **no** lens reviewers — code review is a single pass + at the `code-review-criteria.md` lens. The asymmetry is intentional: + the GHA reviewer fires on a small, already-merged-style PR; the SDLC + reviewer fires on the full implement-phase change set during a + still-mutating pipeline. +9. **Holistic reviewer (`reviewer_code_holistic`, BRC only)**: A second + CRITICAL code reviewer (#2126) runs alongside `reviewer_code` and + focuses on cross-module coherence — end-to-end use case, doc↔code + symmetry, synthetic-key/sentinel coordination, silent-fallback hunt. + It skims the full diff once rather than verifying every line. Its + verdict gates consensus independently of `reviewer_code`'s. Criteria + in `shared/prompts/code-review-holistic-criteria.md`. No `action/` + counterpart — GHA review is single-pass. ## What Must Stay Aligned @@ -138,5 +125,4 @@ When changing review criteria or conventions: - [ ] If changing ACK/NACK format guidance: update the structured format in `_build_brc_preamble()` - [ ] If changing conditional-ACK (`--pre-merge-condition`) behavior: update the BRC preamble example in `_build_brc_preamble()`, the CLI help text in `sandbox/egg_lib/orch_cli.py`, the `_ACK_SCHEMA` description in `sandbox/egg_agent_tools/tools/brc.py`, the `ReviewPayload.pre_merge_condition` docstring in `orchestrator/attestation_schemas.py`, the PR-body renderer `_build_pre_merge_obligations_section()` in `orchestrator/routes/pipelines.py`, the live-status renderer in `cmd_consensus_status` (`sandbox/egg_lib/orch_cli.py`) and its backing field `pre_merge_conditions` in `PeerConsensusTracker.evaluate()`, the reference doc at `docs/reference/conditional-ack.md`, the "Conditional ACK vs NACK vs Plain ACK" subsection in `shared/prompts/code-review-criteria.md`, and the content validator call site for `pre_merge_condition` in `handle_consensus_ack_signal` (`orchestrator/routes/signals.py`) - [ ] If changing the re-review diff command: update the three PR-reviewer builders (`action/build-review-prompt.sh`, `action/build-agent-mode-design-review-prompt.sh`, `action/build-contract-verification-prompt.sh`), the SDLC reviewer's `_build_review_prompt()` `is_delta_review` branch plus its Delta Review directive, and the `BASE_REF` plumbing in `.github/workflows/reusable-review.yml`. The first-review three-dot `git diff origin/...HEAD` is independent of the delta path. -- [ ] If changing the SDLC `reviewer_code` subagent fan-out block (#1965): update the "Subagent Fan-Out Strategy" section in `_build_review_prompt()` (reviewer-code, implement-phase only), the threshold values (`files > 10` OR `loc > 500`), the 6-subagent cap, the 5-minute / 300-second per-subagent wall-clock cap, the parallel-vs-sequential clause that honours `reviewer_code_parallel`, the `mcp__sdlc__show_contract` self-fetch instruction with its empty-list and mcp-unavailable fallbacks, the no-recursion ban, the parent cross-partition consistency pass, and the fan-out STATUS-heartbeat instrumentation. Update the corresponding prompt-text asserts in `orchestrator/tests/test_reviewer_code_fan_out_prompt.py` (or its companion in `test_pipeline_prompts.py`) and the `integration_tests/sdlc/test_reviewer_1964_regression.py` prompt-assert mode. Do NOT touch any `action/` files — the GHA reviewer does not fan out (see asymmetry #8 above). - [ ] If adding or modifying a lens reviewer (`reviewer_security`, `reviewer_concurrency`, or a future lens): update the lens criteria file under `shared/prompts/`, the inline fallback in `orchestrator/routes/pipelines.py` (`_get_security_review_criteria()` / `_get_concurrency_review_criteria()` or equivalent), the dispatcher `_get_review_criteria_for_type()`, the per-lens scope preamble in `_get_reviewer_scope_preamble()`, the role registration in `shared/egg_contracts/agent_roles.py`, the review-graph edges in `orchestrator/review_graph.py` (`get_default_implement_graph()`), and the lens row in this file's "Reviewer types" cell and asymmetry list above. Verify the existing `replace("reviewer_", "").replace("_", "-")` mapping in `pipelines.py` still covers the new role name without a redundant dict (#1965 pitfall guard). diff --git a/shared/prompts/code-review-holistic-criteria.md b/shared/prompts/code-review-holistic-criteria.md index ba900925f4..f3efa38d27 100644 --- a/shared/prompts/code-review-holistic-criteria.md +++ b/shared/prompts/code-review-holistic-criteria.md @@ -4,15 +4,13 @@ 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. +line-by-line review 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: +`reviewer_code`. On any non-trivial diff there are two failure modes +that line-by-line review tends to miss: 1. The **primary advertised use case** quietly fails end-to-end because one module's output is silently dropped by another module's @@ -21,22 +19,21 @@ cross-partition checklist sometimes misses: 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 +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. +holistic lens is the floor that exists to catch those — `reviewer_code` +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. +architectural coherence stand on its own. ## 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. +**Don't verify every line.** `reviewer_code` 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 @@ -119,8 +116,8 @@ sense (no crash, no security violation) and unsafe in the wide sense ## What to Skip -- **Line-by-line correctness.** That is `reviewer_code`'s slice work — - defer to it. +- **Line-by-line correctness.** That is `reviewer_code`'s job — defer + to it. - **Security findings beyond cross-module synthetic-key / silent-fallback patterns.** Defer to `reviewer_security`. - **Concurrency findings.** Defer to `reviewer_concurrency`. diff --git a/shared/prompts/concurrency-review-criteria.md b/shared/prompts/concurrency-review-criteria.md index 12dd3165af..493a485943 100644 --- a/shared/prompts/concurrency-review-criteria.md +++ b/shared/prompts/concurrency-review-criteria.md @@ -12,9 +12,9 @@ set (`reviewer_code`, `reviewer_security`, `reviewer_concurrency`). Focus (other than security-relevant races), and general correctness to `reviewer_code` / `reviewer_security`. -The concurrency lens is currently **ADVISORY** — your NACKs are recorded on -the approval matrix but do not deadlock consensus. Promotion to a critical -(deadlock-capable) reviewer is tracked under [#1997](https://github.com/jwbron/egg/issues/1997). +The concurrency lens is **CRITICAL** — your NACK blocks consensus until the +producer re-proposes ([#2139](https://github.com/jwbron/egg/issues/2139), +closing [#1997](https://github.com/jwbron/egg/issues/1997)). ## What to Flag (in priority order) @@ -51,10 +51,6 @@ Mutual or cyclic waits where every participant is blocked on another: - BRC: producer A waits for ACK from reviewer B; reviewer B's `wait_for_event` is blocked on a message A is itself blocked on producing — see "BRC-protocol invariants" below. -- Reviewer fan-out (this PR's feature): a parent reviewer waits on a - subagent that itself awaits an MCP server reply. Subagent recursion - is **forbidden**; flag any change that softens that ban. - ### 3. Shared-state mutation without synchronization Mutable global / module-level state read or written from multiple @@ -104,11 +100,6 @@ Resources released in the wrong order, conditionally, or not at all: reference and uses it for the next request. - A `tempfile.TemporaryDirectory` cleaned up while a child still has its CWD inside it. -- Subagent timeouts: a subagent exceeding the 5-minute / 300-second - wall-clock cap must be cancelled cleanly. Flag any cancellation - path that leaks a child process or holds a worktree lock past the - cap. - ### 7. BRC-protocol invariants The BRC consensus protocol has temporal invariants that, when @@ -126,11 +117,6 @@ that touches: declared dead even though the work is still progressing ([#2012](https://github.com/jwbron/egg/issues/2012)). Flag any new long-running operation inside a heartbeat-bearing path. -- **Subagent fan-out heartbeat propagation.** When `reviewer_code` - fans out into `Task` subagents, the parent must continue emitting - heartbeats while subagents run. Flag any change that could let the - parent stop heartbeating (e.g. a synchronous subagent-await inside - the heartbeat coroutine). - **`stale_reviewers` invalidation on re-propose.** A re-propose must invalidate prior ACKs at the older version; any path that skips this is a critical bug regardless of test coverage. diff --git a/shared/prompts/security-review-criteria.md b/shared/prompts/security-review-criteria.md index cc6a09098c..72b377096c 100644 --- a/shared/prompts/security-review-criteria.md +++ b/shared/prompts/security-review-criteria.md @@ -11,16 +11,15 @@ The security reviewer is one of three lenses on the implement-phase change set the security lens** and defer code quality, performance, and non-security findings to `reviewer_code`. -The security lens is currently **ADVISORY** — your NACKs are recorded on the -approval matrix but do not deadlock consensus. Promotion to a critical -(deadlock-capable) reviewer is tracked under [#1997](https://github.com/jwbron/egg/issues/1997). +The security lens is **CRITICAL** — your NACK blocks consensus until the +producer re-proposes ([#2139](https://github.com/jwbron/egg/issues/2139), +closing [#1997](https://github.com/jwbron/egg/issues/1997)). ## What to Flag (in priority order) The lens-specific rules below are **additive** to the base file. They name the patterns that are most likely to slip past a single-pass code review on a -large diff, especially when the bug is a **cross-file mismatch** that no -single partition can see in isolation. +large diff, especially when the bug is a **cross-file mismatch**. ### 1. Cross-file allowlist mismatch @@ -41,9 +40,9 @@ Verification recipe: handler reaches. Pay special attention to anchored regexes (`^foo$` vs `^foo/.*$`) and to allowlists that are extended by a sibling file imported elsewhere. -3. If the handler and the check live in different partitions of a fan-out - review, this lens **must** flag any mismatch — the parent reviewer's - cross-partition consistency pass relies on it as defence in depth. +3. This lens **must** flag any handler↔check mismatch — cross-file + security invariants are exactly what the security lens exists to + catch on top of `reviewer_code`'s line-by-line pass. ### 2. Handler-vs-validator path mismatch @@ -137,11 +136,8 @@ Verification recipe: Any deviation from the documented wrapper shape is a **mandatory NACK** — do not silently approve a credential-shim diff that fails the recipe -above. Note that the security lens is advisory today (see *Scope* at the -top of this file), so the NACK is recorded as a finding on the approval -matrix rather than deadlocking consensus; promotion to a critical -(deadlock-capable) reviewer for `sandbox/scripts/*` diffs is tracked in -[#1997](https://github.com/jwbron/egg/issues/1997). +above. The security lens is CRITICAL ([#2139](https://github.com/jwbron/egg/issues/2139)), +so a NACK here blocks consensus until the producer re-proposes. ### 6. Secret leakage @@ -174,13 +170,13 @@ changed files**. Common shapes: - Unsafe deserialization where the trusted-type list lives in a different module from the deserializer. -A single-partition reviewer cannot catch these in isolation. The -security lens runs on the full changeset and is the natural seam to -flag them. +A line-by-line code reviewer often misses these because the file +under inspection looks self-consistent. The security lens runs on the +full changeset and is the natural seam to flag them. ## How to Review -1. Read the full diff once at the security lens — do not split by partition. +1. Read the full diff once at the security lens. 2. For every cross-file invariant above, build the concrete reach: file A line N references X defined in file B line M. If you cannot articulate the reach, you have not found the bug.