From 7afc4e7c6994bb8e0b71f103d80f2ded85f465b6 Mon Sep 17 00:00:00 2001 From: Peter Ulsteen Date: Wed, 15 Jul 2026 11:37:18 -0500 Subject: [PATCH 1/2] feat(code-review): synchronous GitHub-mode verifier dispatch + loud missing-verifier signal (FEA-3154) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Applies the v3.6.0 reviewer hardening to the stage_23 verifier fleet. Bumps code-review 3.6.0 -> 3.7.0. stage_23 (skill code-review:verify-findings) spawned every verifier with run_in_background: true + blocking TaskOutput in BOTH modes — the pattern v3.6.0 proved unsafe under headless claude -p (the turn can end before the blocking collection is issued, so the process exits and verdicts never land). Worse for verifiers: stage_23 is on_failure: continue, so a missing output degrades SILENTLY to pending_verification[], which _compute_canonical_verdict does not read — a BLOCKING finding whose verifier died could ship APPROVED. - verify-findings/SKILL.md: MODE branch — github synchronous one-at-a-time, local parallel background + blocking TaskOutput; headless warning. - commands/start.md: stage_23 note + Verifier Fleet walker guard mirroring stage_20 (ban watcher/sleep/polling/turn-end substitutes). - cmd_verify_consolidate: in github mode, emit ONE aggregate coverage:verifier-missing-output gap (HIGH, required: false -> NEEDS_ATTENTION) when a BLOCKING/HIGH finding has no verifier output. New optional --mode on verify-consolidate, wired at stage_24a. - Tests: TestFEA3154VerifierDispatchContracts prose-contract class + behavioral cmd_verify_consolidate/cmd_finalize_result cases; regenerated the 3 declarative-config snapshots (only the --mode addition). Full code-review suite green (1175 passed), ruff + pyright clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 5 + .../code-review/.claude-plugin/plugin.json | 2 +- plugins/code-review/commands/start.md | 6 +- .../skills/verify-findings/SKILL.md | 42 +- .../tools/python/code_review_helpers.py | 100 +++++ .../code-review/tools/python/config/cli.json | 10 + .../tools/python/config/stages.json | 4 +- .../cli_parser_resolved.json | 11 + .../github_pr42_all_flags.json | 4 +- .../local_no_pr_empty_flags.json | 4 +- .../tools/python/test_code_review_helpers.py | 370 ++++++++++++++++++ 11 files changed, 534 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ddd0f67..5175e8d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,11 @@ All notable changes to the claude-plugins project will be documented in this fil The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). Entries are listed newest-first; each plugin section is treated as released when merged to `main`. +### code-review v3.7.0 + +#### Changed +- **GitHub-mode verifier fleet (`stage_23_verify_findings`) now dispatches synchronously, and a missing BLOCKING/HIGH verifier output raises a loud coverage-gap signal (FEA-3154).** Applies the v3.6.0 reviewer hardening to the verifier fleet. Previously `stage_23` (skill `code-review:verify-findings`) spawned every verifier with `run_in_background: true` + blocking `TaskOutput` in **both** modes — the same pattern v3.6.0 proved unsafe for the reviewer fleet, where a headless `claude -p` turn can end before the blocking collection is issued, so the process exits and verifier verdicts never land. That failure is worse for verifiers than for reviewers: `stage_23` is `on_failure: continue`, so a missing output degrades silently to `pending_verification[]`, which `_compute_canonical_verdict` does not read — a BLOCKING finding whose verifier died could ship `APPROVED`. GitHub mode now dispatches verifiers **synchronously** one-at-a-time (`run_in_background: false`, wait for each Task response before the next), which keeps the headless turn alive by construction; local mode keeps parallel background dispatch + blocking `TaskOutput`. A matching walker-level guard in `commands/start.md` bans watcher files / sleep loops / polling loops / turn-ending waits at `stage_23`. As defense-in-depth (a genuine verifier crash still degrades silently), `cmd_verify_consolidate` now emits — in GitHub mode only, when one or more BLOCKING/HIGH findings had no verifier output — one aggregate `coverage:verifier-missing-output` coverage-gap finding (`severity: HIGH`, `required: false`) that escalates the canonical verdict to `NEEDS_ATTENTION` (mirroring the existing "verifier uncertain → human triages, not silent approval" semantics), so an unverified high-severity finding cannot pass silently. Threaded via a new optional `--mode` on the `verify-consolidate` subcommand (wired at `stage_24a_verify_consolidate`, the same `{mode}` token the adjacent `finalize-result` already uses). Net-new github-synchronous verifier behavior — not a revert (pre-v3.5.0 verifiers were background in both modes). Adds the `TestFEA3154VerifierDispatchContracts` prose-contract class plus behavioral `cmd_verify_consolidate`/`cmd_finalize_result` tests. + ### code-review v3.6.0 #### Fixed diff --git a/plugins/code-review/.claude-plugin/plugin.json b/plugins/code-review/.claude-plugin/plugin.json index 7d2c66e..684332f 100644 --- a/plugins/code-review/.claude-plugin/plugin.json +++ b/plugins/code-review/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "code-review", "description": "Code review plugin", - "version": "3.6.0", + "version": "3.7.0", "author": { "name": "ClosedLoop", "email": "support@closedloop.ai" diff --git a/plugins/code-review/commands/start.md b/plugins/code-review/commands/start.md index fc27177..ca5de90 100644 --- a/plugins/code-review/commands/start.md +++ b/plugins/code-review/commands/start.md @@ -394,7 +394,7 @@ These notes annotate the run-plan stages with anything not obvious from the plan - **stage_20b_verify_spawn** (PLN-725): runs `verify-spawn`. Reads `/spawn.json` (`spec` section) and globs `/agent_*.json`; for every descriptor with `bucket: "required"` that has no on-disk output, appends a coverage-gap finding to `/coverage_gaps.json` (reason `spawn_missing_required_agent`) and records the omission in `/spawn.json` (`verification` section). Missing best-effort descriptors are recorded for telemetry but emit no finding — best-effort omissions are budget-driven, not coverage gaps. No-ops cleanly when the spec is missing (`spec_missing`), marks fallback (`spec_fallback`), or contains no agents (`spec_empty`). `on_failure: continue` — a verification bug must never block review; worst case is missing telemetry, not a halted pipeline. Wired before `stage_21_collect_findings` so the gap findings land in `coverage_gaps.json` in time for `cmd_finalize_result` to merge them into the canonical envelope. - **stage_22_validate**: writes `/findings_validated.json` via `> /findings_validated.json` redirection. Validates finding scope and applies the out-of-hunk confidence gate. P2+ findings whose `line` falls outside the file's changed range survive when `confidence > out_of_hunk_confidence_floor` (default `0.80`, operator-tunable via `.closedloop-ai/settings/code-review.json:out_of_hunk_confidence_floor`, range `[0.0, 1.0]`) — this admits legitimate companion-change findings (e.g. a signature change in the diff window leaving stale sibling call sites just outside it) while still filtering low-confidence noise. Survivors get tagged `out_of_hunk_kept: true` so presenters can label them as companion-change without re-deriving hunk membership; the validate-stats block exposes `kept_out_of_hunk` and `discarded_out_of_hunk_low_confidence`. The comparison is strict `>`, so setting the floor to `1.0` is a kill switch (nothing can clear); setting it to `0.0` lets every out-of-hunk P2+ through (lean on the PLN-722 verifier downstream). Per-finding verification (stage_23) still applies on top, so noise that surfaces here gets a second-pass CONFIRMED/REJECTED verdict. - **stage_22b_verify_prepare** (PLN-722): tier-selects findings for verification per the canonical table — BLOCKING/HIGH always; MEDIUM with confidence < 0.85 yes; MEDIUM with confidence ≥ 0.85 no; LOW (P3) no; `category: "Hygiene"` no; `source: "injection-detector"` no. Ranks the eligible set by `severity_weight × confidence`, caps at `VERIFY_MAX_VERIFICATIONS = 50`, and writes (a) `/verify_manifest.json` with `to_verify[]` + `skipped_no_verification[]` + `deferred_budget[]` + `cache_hits[]`, and (b) `/verifier_inputs/.json` per eligible finding. When `--cache-dir` is set, fresh verifier outputs from a prior run for the same `(finding_id, code_snippet_hash, model, prompt_hash)` tuple are pre-materialized at `agent_verifier_.json` and skipped from `to_verify[]` (logged under `cache_hits[]`). `on_failure: continue` is intentional — verify-prepare failure degrades to "no verifier this run", not a pipeline abort. -- **stage_23_verify_findings** (PLN-722): agent_fleet stage. Invoke the `code-review:verify-findings` skill. Each spawned agent reads its `verifier_inputs/.json` (containing the finding + the `verifier_prompt_path` + the canonical `output_path`) and emits one verdict file at `/agent_verifier_.json`. `on_failure: continue` so a single agent crash never aborts review. +- **stage_23_verify_findings** (PLN-722): agent_fleet stage. Invoke the `code-review:verify-findings` skill. Each spawned agent reads its `verifier_inputs/.json` (containing the finding + the `verifier_prompt_path` + the canonical `output_path`) and emits one verdict file at `/agent_verifier_.json`. `on_failure: continue` so a single agent crash never aborts review. In `MODE=github`, the walker must follow the skill's synchronous verifier branch: do not use `TaskOutput`, watcher files, sleep loops, polling loops, or turn-ending waits as replacements for synchronous verifier completion. `stage_23` must complete every GitHub synchronous verifier, leaving no verifier task still running, or fail before `stage_24a_verify_consolidate`. - **stage_24a_verify_consolidate** (PLN-722, extended in PLN-721): merges all `agent_verifier_*.json` outputs back into the validated set, applies sensitive-path escalation from `.closedloop-ai/settings/verification-gates.json` (rules: REJECTED on `sensitive_paths` + BLOCKING/HIGH → TENTATIVE with severity capped at HIGH; any finding on `tentative_on_paths` → TENTATIVE; any finding on `mandatory_human_review_paths` → TENTATIVE + `force_human_review: true`), routes JUSTIFIED-VALID verdicts to a new `justified[]` bucket and JUSTIFIED-INVALID verdicts back into `verified[]` (the audited justification was refuted; the original concern stands), and writes `/findings_verified.json` with the bucket-split shape `{verified[], rejected[], pending_verification[], justified[], force_human_review}`. `tentative_on_paths` lifts JUSTIFIED-VALID/INVALID to TENTATIVE on the same operator-policy contract as the other verdicts. When `--cache-dir` is set, fresh verifier outputs are written back to the `verifications/` namespace (30-day TTL) for re-use on subsequent runs. Missing fleet outputs degrade to `pending_verification[]`; `on_failure: continue`. - **stage_25_finalize_result** (PLN-722 + PLN-721): writes `/review_result.json` (the canonical envelope) BEFORE running schema validation. PLN-722: prefers `/findings_verified.json` (verify-consolidate output) when present and honors its `force_human_review` flag in the verdict computation; falls back to `findings_validated.json` (everything to `verified[]`) when verify-consolidate didn't run. PLN-721: pipes the consolidate `justified[]` bucket into the envelope, and loads operator-overridable thresholds from `.closedloop-ai/settings/verdict-thresholds.json` (defaults to `impact_cumulative=2`; absent/malformed → built-in default) so `_compute_canonical_verdict`'s cumulative Impact gate (FEA-1401 / PLN-726 OQ#6) can fire (≥ 2 BLOCKING/HIGH `ImpactAnalysis` findings in `verified[]` → NEEDS_ATTENTION). A non-zero exit signals reviewer-emitted category/field drift (e.g. a category not in the canonical enum) but does not block the pipeline — `on_failure: continue` lets `stage_28_verdict` read the structurally complete envelope. Surface the stderr text in the present step so operators can correct prompts/schema; do not abort. - **stage_26_cache_update**: gated by **Gate C**. @@ -421,6 +421,10 @@ Decomposition rationale: ~470 lines of reviewer-fleet dispatch content was extra When the walker reaches `stage_23_verify_findings`, invoke the `code-review:verify-findings` skill. The skill owns the full finding-verifier dispatch: reading `verify_manifest.json`, spawning one falsify-oriented verifier Task per `to_verify[]` entry (skipping `cache_hits[]`), the no-retry collection contract, and the `pending_verification[]` degradation when a verifier output is missing. +The skill is invoked for both `MODE=local` and `MODE=github`, but Task scheduling is mode-specific: GitHub mode dispatches verifiers synchronously, while local mode preserves parallel background dispatch plus blocking collection. In `MODE=github` a missing verifier output for a BLOCKING/HIGH finding also raises a durable coverage-gap signal at `stage_24a_verify_consolidate` so an unverified high-severity finding cannot pass silently to an approved verdict. + +GitHub headless mode has a walker-level guard in addition to the skill contract: verifiers and retries must be dispatched synchronously, and the walker must not proceed to `stage_24a_verify_consolidate`, emit a final summary, mark the review complete, or end the assistant turn while any GitHub verifier remains outstanding. Watcher files, sleep loops, polling loops, background `TaskOutput` waits, and "I'll continue when notified" turns are forbidden substitutes for completing the synchronous verifier response. + --- diff --git a/plugins/code-review/skills/verify-findings/SKILL.md b/plugins/code-review/skills/verify-findings/SKILL.md index 100dd88..7d3dd75 100644 --- a/plugins/code-review/skills/verify-findings/SKILL.md +++ b/plugins/code-review/skills/verify-findings/SKILL.md @@ -1,11 +1,11 @@ --- name: verify-findings -description: Dispatch and collect the finding-verifier fleet at stage_23_verify_findings (PLN-722). Reads verify_manifest.json (written by stage_22b_verify_prepare), spawns one falsify-oriented verifier Task per to_verify[] entry, skips cache_hits[], and collects outputs without retry — missing outputs degrade to pending_verification[]. Invoke when the walker reaches stage_23_verify_findings. Do NOT use for the reviewer fleet (stage_20 — see the spawn-reviewers skill) or the PLN-725 singletons (stage_11/stage_15 — see the singleton-dispatch skill). +description: Dispatch and collect the finding-verifier fleet at stage_23_verify_findings (PLN-722). Reads verify_manifest.json (written by stage_22b_verify_prepare), spawns one falsify-oriented verifier Task per to_verify[] entry with mode-specific Task scheduling (GitHub mode dispatches verifiers synchronously; local mode uses parallel background + blocking TaskOutput), skips cache_hits[], and collects outputs without retry — missing outputs degrade to pending_verification[] (and, in GitHub mode, a missing BLOCKING/HIGH verifier raises a coverage-gap signal). Invoke when the walker reaches stage_23_verify_findings. Do NOT use for the reviewer fleet (stage_20 — see the spawn-reviewers skill) or the PLN-725 singletons (stage_11/stage_15 — see the singleton-dispatch skill). --- # Finding-Verifier Fleet Dispatch (stage_23_verify_findings) -This skill is the canonical finding-verifier dispatcher for `/code-review` at `stage_23_verify_findings`. It is split out of `commands/start.md` so the orchestration spine stays lean; the orchestrator invokes it when the walker reaches `stage_23_verify_findings`. The content below is relocated verbatim from `start.md`. +This skill is the canonical finding-verifier dispatcher for `/code-review` at `stage_23_verify_findings`. It is split out of `commands/start.md` so the orchestration spine stays lean; the orchestrator invokes it when the walker reaches `stage_23_verify_findings`. The content below is authoritative for both `MODE=local` and `MODE=github`, with mode-specific Task scheduling: GitHub mode dispatches verifiers synchronously, while local mode preserves parallel background dispatch plus blocking `TaskOutput` collection. --- @@ -30,29 +30,31 @@ This stage runs when the walker reaches `stage_23`. It implements PLN-722's find ### Spawn contract -For each entry in `verify_manifest.json.to_verify[]`: +**First branch on `MODE`.** GitHub and local runs intentionally use different Task scheduling because GitHub headless mode cannot survive outstanding background verifiers after the assistant turn ends. -1. Spawn one background `Task` with `subagent_type: "code-review:code-review-worker"`. The agent's tool allowlist (`Read`, `Write`, `Grep`, `Glob`) is identical to the Reviewer Fleet's — no permission changes needed. -2. Prompt template: - ``` - You are the FINDING VERIFIER. Read your prompt at: - {VERIFIER_PROMPT_PATH} +**GitHub mode (`MODE=github`): dispatch synchronously.** Spawn exactly one verifier at a time and wait for its Task response before spawning the next `to_verify[]` entry. Omit `run_in_background` or set `run_in_background: false`; never set it to `true` for GitHub verifiers. Do not use `TaskOutput`, watcher files, sleep loops, polling loops, or "wait for background task" turns in GitHub verifier dispatch. Each verifier must finish, write its verdict JSON to the manifest entry's `output_path` (`/agent_verifier_.json`), and return before the walker dispatches the next `to_verify[]` entry. When `stage_23` completes in GitHub mode there must be no verifier task still running. - Your input file is at: - {INPUT_PATH} +**Local mode (`MODE=local`): spawn ALL verifiers at once.** Use `run_in_background: true` on every verifier `Task`. You can spawn all agents in a single message or across a few messages. - Read it for the finding to verify, the canonical output path, and the - per-output JSON shape. Write your verdict JSON to the output path the - input file specifies. Do not write anywhere else. - ``` - Substitute the resolved paths from the manifest entry (the verifier prompt is at `/verifier_prompt.txt`, copied by `stage_02_prep_assets`). Each input file also carries a `review_root` field (written by `stage_22b_verify_prepare` from `scope.json`); the verifier prompt tells the agent to read source under that root when it is non-empty (local PR-head worktree isolation) — no extra wiring is needed here. -3. Set `model` to the entry's `model` field (currently uniform `sonnet`; future revisions may split by original-reviewer model for cross-model independence). +Each spawned verifier — in either mode — uses `subagent_type: "code-review:code-review-worker"` (tool allowlist `Read`, `Write`, `Grep`, `Glob` — identical to the Reviewer Fleet's, so no permission changes are needed) and this prompt template: +``` +You are the FINDING VERIFIER. Read your prompt at: + {VERIFIER_PROMPT_PATH} + +Your input file is at: + {INPUT_PATH} + +Read it for the finding to verify, the canonical output path, and the +per-output JSON shape. Write your verdict JSON to the output path the +input file specifies. Do not write anywhere else. +``` +Substitute the resolved paths from the manifest entry (the verifier prompt is at `/verifier_prompt.txt`, copied by `stage_02_prep_assets`). Each input file also carries a `review_root` field (written by `stage_22b_verify_prepare` from `scope.json`); the verifier prompt tells the agent to read source under that root when it is non-empty (local PR-head worktree isolation) — no extra wiring is needed here. Set `model` to the entry's `model` field (currently uniform `sonnet`; future revisions may split by original-reviewer model for cross-model independence). ### Collection contract -- Call `TaskOutput` (block: true) for every spawned verifier agent before letting the walker proceed past `stage_23`. -- A missing `agent_verifier_.json` is NOT a fatal error — `cmd_verify_consolidate` tags it as `pending_verification[]` so operators see what didn't get verified. -- Do NOT retry verifier agents in the walker. If a verifier fails, the finding's downstream handling already covers the gap (pending) — and verifier retries would burn tokens on a finding already flagged for human review. +- **Local collection (MANDATORY for `MODE=local`):** Call `TaskOutput` (block: true) for every spawned local background verifier before letting the walker proceed past `stage_23`. You MUST collect ALL verifiers before consolidation. In headless GitHub mode there is no asynchronous completion notification, so GitHub verifier dispatch uses the synchronous branch above instead of backgrounding and collecting with `TaskOutput`. +- A missing `agent_verifier_.json` is NOT a fatal error — `cmd_verify_consolidate` tags it as `pending_verification[]` so operators see what didn't get verified. In `MODE=github`, a missing verifier output for a **BLOCKING/HIGH** finding additionally raises a durable coverage-gap signal (see `cmd_verify_consolidate` / `stage_24a_verify_consolidate`) so an unverified high-severity finding cannot pass silently to an approved verdict. +- Do NOT retry verifier agents in the walker. If a verifier fails, the finding's downstream handling already covers the gap (pending) — and verifier retries would burn tokens on a finding already flagged for human review. If a verifier is re-dispatched at all, it uses the same mode branch — GitHub verifier retries are synchronous; local retries may use the local background-plus-`TaskOutput` collection contract. - `stage_23.on_failure == "continue"`: a fleet-wide failure does NOT abort the pipeline; `verify-consolidate` and `finalize-result` produce a usable envelope even when zero verifier outputs land on disk. ### Cache hits (skip spawn) @@ -64,3 +66,5 @@ Entries in `verify_manifest.json.cache_hits[]` are already on disk at `agent_ver - Do not read finding source files in the orchestrator (verifier agents read files via Read/Grep themselves). - Do not parse `agent_verifier_*.json` in the orchestrator — `cmd_verify_consolidate` (stage_24a) reads them. - Do not regenerate `verify_manifest.json` in the walker — `cmd_verify_prepare` (stage_22b) is the only writer. + +**Headless mode warning.** In GitHub mode the review runs under headless `claude -p`, where there is NO asynchronous subagent-completion notification: when the orchestrator's assistant turn ends with no pending synchronous tool call, the process terminates immediately (`terminal_reason: "completed"`). If you background a verifier and then end your turn to "wait" for it, the run dies before `stage_24a_verify_consolidate` through `stage_30_footer` execute, so the verifier verdicts never land and any BLOCKING/HIGH finding whose verifier was outstanding ships unverified. The GitHub verifier synchronous Task calls and local-mode blocking `TaskOutput` collection are the only supported ways to keep the turn alive until verifiers finish; never substitute either with watcher files, sleep loops, polling loops, or "I'll continue when notified." diff --git a/plugins/code-review/tools/python/code_review_helpers.py b/plugins/code-review/tools/python/code_review_helpers.py index 47bdba6..9dbcbcf 100644 --- a/plugins/code-review/tools/python/code_review_helpers.py +++ b/plugins/code-review/tools/python/code_review_helpers.py @@ -3113,6 +3113,12 @@ def cmd_verify_consolidate(args: argparse.Namespace) -> int: ) cache_dir = Path(args.cache_dir) if getattr(args, "cache_dir", None) else None prompt_hash = str(getattr(args, "prompt_hash", "") or "") + # FEA-3154: in GitHub/headless mode a verifier whose output never lands + # (e.g. the orchestrator ended its turn before collection) must not let a + # BLOCKING/HIGH finding ship silently — those degrade to pending_verification[], + # which _compute_canonical_verdict does not read. We count such misses and, + # in github mode, raise one durable coverage-gap signal below. + mode = str(getattr(args, "mode", None) or "") validated_data = _read_optional_json(validated_path, {}) if isinstance(validated_data, dict): @@ -3172,6 +3178,8 @@ def cmd_verify_consolidate(args: argparse.Namespace) -> int: escalated_sensitive = 0 escalated_mandatory = 0 force_human_review = False + # FEA-3154: count BLOCKING/HIGH findings whose verifier produced no output. + missing_high_or_blocking = 0 for raw in validated: if not isinstance(raw, dict): @@ -3198,6 +3206,12 @@ def cmd_verify_consolidate(args: argparse.Namespace) -> int: "Verifier agent did not produce an output file; finding " "deferred for re-verification." ) + # FEA-3154: a BLOCKING/HIGH finding left unverified is the + # silent-headless-death risk — count it so github mode can + # raise a loud coverage-gap signal below. (Deferred-budget + # findings take the elif branch and are intentionally excluded.) + if str(finding.get("severity", "")) in ("BLOCKING", "HIGH"): + missing_high_or_blocking += 1 pending.append(finding) continue _merge_verifier_fields(finding, verdict_data) @@ -3320,6 +3334,34 @@ def cmd_verify_consolidate(args: argparse.Namespace) -> int: }, ) + # FEA-3154: loud degradation. In github mode, if any BLOCKING/HIGH finding + # was left unverified (verifier produced no output), emit ONE aggregate + # coverage-gap finding so cmd_finalize_result escalates the verdict to at + # least NEEDS_ATTENTION instead of silently approving. pending_verification[] + # alone is not read by _compute_canonical_verdict, so without this a + # BLOCKING finding whose verifier died could ship APPROVED. + # Scoped to github deliberately: local mode renders pending_verification[] + # to a human at stage_29_present (human-in-the-loop), whereas github mode + # auto-posts a verdict with no human gate — so the loud signal matters there. + if mode == "github" and missing_high_or_blocking > 0: + gaps_path = cr_dir / "coverage_gaps.json" + existing_gaps = _read_optional_json(gaps_path, None) + gap_index = ( + len(existing_gaps.get("findings", []) or []) + if isinstance(existing_gaps, dict) + else 0 + ) + _append_to_coverage_gaps( + gaps_path, + [ + _make_unverified_findings_gap( + missing_high_or_blocking, + index=gap_index, + emitted_at=datetime.now(timezone.utc).isoformat(), + ), + ], + ) + output = { "verified": verified, "rejected": rejected, @@ -11294,6 +11336,64 @@ def _make_coverage_gap_finding( ) +def _make_unverified_findings_gap( + count: int, + *, + index: int, + emitted_at: str, +) -> dict[str, Any]: + """Build a canonical system-scoped coverage-gap for unverified BLOCKING/HIGH findings. + + FEA-3154: emitted by ``cmd_verify_consolidate`` in github mode when one or + more BLOCKING/HIGH findings had no verifier output. Unlike + ``_make_coverage_gap_finding`` (a dropped *reviewer*, ``required: True`` → + CHANGES_REQUESTED), this is ``required: False`` → the finding surfaces as a + ``HIGH`` coverage gap that ``_compute_canonical_verdict`` escalates to + NEEDS_ATTENTION (Rule 3), mirroring the existing "verifier uncertain → + human triages, not silent approval" TENTATIVE semantics. It reuses the + ``coverage:`` marker template (no schema change) and sets every + field ``validate_finding`` requires, because ``normalize_legacy_finding`` + does not synthesize priority/severity/confidence/category/issue. + """ + return normalize_legacy_finding( + { + "id": make_finding_id("coverage-verifier", index), + "reviewer": "coverage-verifier", + "source": "coverage-verifier", + "schema_version": SCHEMA_VERSION, + "finding_scope": "system", + "file": None, + "line": None, + "system_marker": "coverage:verifier-missing-output", + "category": "Coverage", + "severity": "HIGH", + "priority": 1, + "confidence": 1.0, + "issue": ( + f"{count} BLOCKING/HIGH finding(s) shipped unverified — verifier " + "output missing in GitHub mode" + ), + "explanation": ( + f"{count} BLOCKING/HIGH finding(s) had no verifier output file in " + "this GitHub-mode run (a verifier agent may have crashed, or the " + "headless turn ended before collection). Those findings remain in " + "pending_verification[], which the canonical verdict does not read, " + "so they would otherwise pass silently." + ), + "recommendation": ( + "Re-run the review so the verifier fleet completes, or verify the " + "flagged BLOCKING/HIGH finding(s) manually before merging." + ), + "code_snippet": "", + "required": False, + }, + reviewer="coverage-verifier", + source="coverage-verifier", + index=index, + emitted_at=emitted_at, + ) + + def _normalize_coverage_verify_doc(doc: Any) -> tuple[str | None, list[dict[str, str]]]: """Coerce a parsed coverage-verify section/file into ``(verdict, violations)``. diff --git a/plugins/code-review/tools/python/config/cli.json b/plugins/code-review/tools/python/config/cli.json index bea9e93..db7bf3e 100644 --- a/plugins/code-review/tools/python/config/cli.json +++ b/plugins/code-review/tools/python/config/cli.json @@ -247,6 +247,16 @@ ], "default": "", "help": "Verifier prompt hash for cache write-back keys." + }, + { + "flags": [ + "--mode" + ], + "choices": [ + "local", + "github" + ], + "help": "Review mode; github raises a coverage-gap signal when a BLOCKING/HIGH finding has no verifier output (FEA-3154). Optional; omitted defaults to no signal." } ] }, diff --git a/plugins/code-review/tools/python/config/stages.json b/plugins/code-review/tools/python/config/stages.json index 68d5d19..46b0aa1 100644 --- a/plugins/code-review/tools/python/config/stages.json +++ b/plugins/code-review/tools/python/config/stages.json @@ -735,7 +735,9 @@ "--cache-dir", "", "--prompt-hash", - "" + "", + "--mode", + "{mode}" ], "stdout": null, "expected_outputs": [ diff --git a/plugins/code-review/tools/python/fixtures/run_plan_snapshots/cli_parser_resolved.json b/plugins/code-review/tools/python/fixtures/run_plan_snapshots/cli_parser_resolved.json index 99a800f..a3c5669 100644 --- a/plugins/code-review/tools/python/fixtures/run_plan_snapshots/cli_parser_resolved.json +++ b/plugins/code-review/tools/python/fixtures/run_plan_snapshots/cli_parser_resolved.json @@ -263,6 +263,17 @@ ], "dest": "prompt_hash", "default": "" + }, + { + "flags": [ + "--mode" + ], + "dest": "mode", + "default": null, + "choices": [ + "local", + "github" + ] } ] }, diff --git a/plugins/code-review/tools/python/fixtures/run_plan_snapshots/github_pr42_all_flags.json b/plugins/code-review/tools/python/fixtures/run_plan_snapshots/github_pr42_all_flags.json index 13fc0db..aa92d1b 100644 --- a/plugins/code-review/tools/python/fixtures/run_plan_snapshots/github_pr42_all_flags.json +++ b/plugins/code-review/tools/python/fixtures/run_plan_snapshots/github_pr42_all_flags.json @@ -714,7 +714,9 @@ "--cache-dir", "", "--prompt-hash", - "" + "", + "--mode", + "github" ], "stdout": null, "expected_outputs": [ diff --git a/plugins/code-review/tools/python/fixtures/run_plan_snapshots/local_no_pr_empty_flags.json b/plugins/code-review/tools/python/fixtures/run_plan_snapshots/local_no_pr_empty_flags.json index 752b3aa..9b18185 100644 --- a/plugins/code-review/tools/python/fixtures/run_plan_snapshots/local_no_pr_empty_flags.json +++ b/plugins/code-review/tools/python/fixtures/run_plan_snapshots/local_no_pr_empty_flags.json @@ -708,7 +708,9 @@ "--cache-dir", "", "--prompt-hash", - "" + "", + "--mode", + "local" ], "stdout": null, "expected_outputs": [ diff --git a/plugins/code-review/tools/python/test_code_review_helpers.py b/plugins/code-review/tools/python/test_code_review_helpers.py index 7e99d39..5b90771 100644 --- a/plugins/code-review/tools/python/test_code_review_helpers.py +++ b/plugins/code-review/tools/python/test_code_review_helpers.py @@ -8906,6 +8906,7 @@ def _run_verify_consolidate( cache_dir: Path | None = None, prompt_hash: str = "", cr_dir: Path | None = None, + mode: str | None = None, ) -> tuple[int, dict[str, Any]]: """Invoke ``cmd_verify_consolidate`` with stdout captured into a dict.""" import io @@ -8941,6 +8942,7 @@ def _run_verify_consolidate( gates=gates_path, cache_dir=str(cache_dir) if cache_dir else None, prompt_hash=prompt_hash, + mode=mode, ) rc = cmd_verify_consolidate(ns) _sys.stdout.seek(0) @@ -10438,6 +10440,177 @@ def test_deferred_budget_findings_go_to_pending( assert len(out["pending_verification"]) == 1 assert "MAX_VERIFICATIONS" in out["pending_verification"][0]["verifier_reasoning"] + def test_missing_github_verifier_high_emits_coverage_gap( + self, tmp_path: Path, + ) -> None: + """FEA-3154: a missing verifier output for a HIGH finding in github mode + raises one durable coverage-gap that escalates the verdict. + """ + import argparse as _argparse + import io + import sys as _sys + + from code_review_helpers import cmd_finalize_result + + cr_dir = tmp_path / "cr" + findings = [_make_validated_finding("bha_p0_f0", severity="HIGH")] + manifest = { + "to_verify": [{"finding_id": "bha_p0_f0", "model": "sonnet"}], + "skipped_no_verification": [], "deferred_budget": [], "cache_hits": [], + } + _, out = _run_verify_consolidate( + tmp_path, findings, manifest=manifest, verifier_outputs={}, + cr_dir=cr_dir, mode="github", + ) + # (a) the finding still degrades to pending — the loud signal is additive + assert len(out["pending_verification"]) == 1 + # (b) exactly one schema-valid coverage gap with the FEA-3154 shape + gaps = json.loads((cr_dir / "coverage_gaps.json").read_text())["findings"] + assert len(gaps) == 1 + gap = gaps[0] + assert gap["system_marker"] == "coverage:verifier-missing-output" + assert gap["severity"] == "HIGH" + assert gap["priority"] == 1 + assert gap["required"] is False + assert gap["category"] == "Coverage" + assert gap["finding_scope"] == "system" + assert gap["file"] is None + # (c) end-to-end finalize: the gap escalates the verdict to + # NEEDS_ATTENTION AND the envelope validates. Asserting rc == 0 and + # validation_errors == [] is required because cmd_finalize_result writes + # the envelope and computes the verdict *before* returning nonzero on a + # validation error — a verdict-only assertion would pass even with a + # schema-invalid gap. + (cr_dir / "setup.json").write_text(json.dumps( + {"head_sha": "abc", "current_branch": "feat/x"}, + )) + validated_path = cr_dir / "findings_validated.json" + validated_path.write_text(json.dumps({"validated": findings})) + old_stdout = _sys.stdout + _sys.stdout = io.StringIO() + try: + ns = _argparse.Namespace( + cr_dir=str(cr_dir), + findings_validated=str(validated_path), + mode="github", + diff_tip="abc", + pr_number=None, + ) + rc2 = cmd_finalize_result(ns) + _sys.stdout.seek(0) + summary = json.load(_sys.stdout) + finally: + _sys.stdout = old_stdout + assert rc2 == 0 + assert summary["validation_errors"] == [] + assert summary["verdict"] == "NEEDS_ATTENTION" + + def test_missing_local_verifier_does_not_emit_coverage_gap( + self, tmp_path: Path, + ) -> None: + """FEA-3154: local mode keeps the pending-only degradation — no gap.""" + cr_dir = tmp_path / "cr" + findings = [_make_validated_finding("bha_p0_f0", severity="HIGH")] + manifest = { + "to_verify": [{"finding_id": "bha_p0_f0", "model": "sonnet"}], + "skipped_no_verification": [], "deferred_budget": [], "cache_hits": [], + } + _, out = _run_verify_consolidate( + tmp_path, findings, manifest=manifest, verifier_outputs={}, + cr_dir=cr_dir, mode="local", + ) + assert len(out["pending_verification"]) == 1 + assert not (cr_dir / "coverage_gaps.json").exists() + + def test_missing_github_verifier_medium_does_not_emit_gap( + self, tmp_path: Path, + ) -> None: + """FEA-3154: only BLOCKING/HIGH misses are loud; MEDIUM stays pending.""" + cr_dir = tmp_path / "cr" + findings = [_make_validated_finding("bhb_f0", severity="MEDIUM")] + manifest = { + "to_verify": [{"finding_id": "bhb_f0", "model": "sonnet"}], + "skipped_no_verification": [], "deferred_budget": [], "cache_hits": [], + } + _, out = _run_verify_consolidate( + tmp_path, findings, manifest=manifest, verifier_outputs={}, + cr_dir=cr_dir, mode="github", + ) + assert len(out["pending_verification"]) == 1 + assert not (cr_dir / "coverage_gaps.json").exists() + + def test_deferred_budget_does_not_emit_github_gap( + self, tmp_path: Path, + ) -> None: + """FEA-3154: budget-deferred findings are not a headless death — no gap.""" + cr_dir = tmp_path / "cr" + findings = [_make_validated_finding("bha_p0_f0", severity="HIGH")] + manifest = { + "to_verify": [], + "skipped_no_verification": [], + "deferred_budget": ["bha_p0_f0"], + "cache_hits": [], + } + _, out = _run_verify_consolidate( + tmp_path, findings, manifest=manifest, verifier_outputs={}, + cr_dir=cr_dir, mode="github", + ) + assert len(out["pending_verification"]) == 1 + assert not (cr_dir / "coverage_gaps.json").exists() + + def test_missing_github_verifiers_aggregate_into_one_gap_with_count( + self, tmp_path: Path, + ) -> None: + """FEA-3154: multiple missing BLOCKING/HIGH verifiers collapse into ONE + aggregate gap whose count reflects the number unverified.""" + cr_dir = tmp_path / "cr" + findings = [ + _make_validated_finding("bha_p0_f0", severity="HIGH"), + _make_validated_finding("bha_p0_f1", severity="BLOCKING", file="src/b.ts"), + ] + manifest = { + "to_verify": [ + {"finding_id": "bha_p0_f0", "model": "sonnet"}, + {"finding_id": "bha_p0_f1", "model": "sonnet"}, + ], + "skipped_no_verification": [], "deferred_budget": [], "cache_hits": [], + } + _, out = _run_verify_consolidate( + tmp_path, findings, manifest=manifest, verifier_outputs={}, + cr_dir=cr_dir, mode="github", + ) + assert len(out["pending_verification"]) == 2 + gaps = json.loads((cr_dir / "coverage_gaps.json").read_text())["findings"] + # ONE aggregate gap (not one per finding), count == 2 + assert len(gaps) == 1 + assert "2 BLOCKING/HIGH finding" in gaps[0]["issue"] + + def test_github_gap_index_continues_past_existing_coverage_gaps( + self, tmp_path: Path, + ) -> None: + """FEA-3154: the aggregate gap's id continues past any pre-existing + coverage gaps (e.g. stage_20b's coverage-verifier_f0), so no id collides.""" + cr_dir = tmp_path / "cr" + cr_dir.mkdir(parents=True, exist_ok=True) + # Simulate a stage_20b dropped-reviewer gap already on disk. + (cr_dir / "coverage_gaps.json").write_text(json.dumps( + {"findings": [{"id": "coverage-verifier_f0", "severity": "HIGH"}]}, + )) + findings = [_make_validated_finding("bha_p0_f0", severity="HIGH")] + manifest = { + "to_verify": [{"finding_id": "bha_p0_f0", "model": "sonnet"}], + "skipped_no_verification": [], "deferred_budget": [], "cache_hits": [], + } + _run_verify_consolidate( + tmp_path, findings, manifest=manifest, verifier_outputs={}, + cr_dir=cr_dir, mode="github", + ) + gaps = json.loads((cr_dir / "coverage_gaps.json").read_text())["findings"] + assert len(gaps) == 2 # pre-existing + the new aggregate + new_gap = gaps[-1] + assert new_gap["id"] == "coverage-verifier_f1" # index = len(existing) → no collision + assert new_gap["system_marker"] == "coverage:verifier-missing-output" + def test_sensitive_path_escalates_rejected_blocking_to_tentative( self, tmp_path: Path, ) -> None: @@ -17986,6 +18159,203 @@ def _assert_headless_warning_mode_specific(section: str) -> None: assert "blocking `taskoutput` collection (standard flow)" not in lowered +def _verify_findings_skill_text() -> str: + """Read the finding-verifier dispatch prompt contract under test (FEA-3154).""" + return ( + Path(__file__).parents[2] + / "skills" + / "verify-findings" + / "SKILL.md" + ).read_text() + + +def _assert_github_verifier_flow_sync(section: str) -> None: + """Assert the GitHub-mode verifier dispatch source contract is sync-only.""" + assert "one verifier at a time" in section + assert "wait for its Task response before spawning the next" in section + assert "run_in_background: false" in section + assert "never set it to `true`" in section + assert "Do not use `TaskOutput`, watcher files, sleep loops, polling loops" in section + assert "there must be no verifier task still running" in section + + +def _assert_start_md_github_stage_23_guard(section: str) -> None: + """Assert start.md pins the GitHub-mode walker guard at stage 23.""" + lowered = section.lower() + assert "MODE=github" in section + assert "synchronous" in section + assert "`taskoutput`" in lowered + assert "watcher" in lowered + assert "sleep loop" in lowered + assert "polling loop" in lowered + assert "turn-ending waits" in section or "end the assistant turn" in section + assert "no verifier task" in section or "verifier remains outstanding" in section + assert ( + "fail before `stage_24a_verify_consolidate`" in section + or "must not proceed to `stage_24a_verify_consolidate`" in section + ) + + +def _assert_headless_warning_verifier_mode_specific(section: str) -> None: + """Assert the verifier headless warning keeps GitHub flow synchronous.""" + lowered = section.lower() + assert "github verifier synchronous task calls" in lowered + assert "local-mode blocking `taskoutput` collection" in lowered + assert "watcher files" in lowered + assert "sleep loops" in lowered + assert "polling loops" in lowered + + +class TestFEA3154VerifierDispatchContracts: + """FEA-3154 hardens GitHub-mode verifier dispatch (stage_23) the same way + FEA-2162 hardened the reviewer fleet, while preserving local parallelism. + """ + + def test_github_verifier_flow_requires_synchronous_dispatch(self) -> None: + """GitHub headless verifier dispatch must not leave background Tasks + outstanding when stage_23 completes. + """ + github_section = _extract_section( + _verify_findings_skill_text(), + "**GitHub mode (`MODE=github`): dispatch synchronously.**", + "**Local mode (`MODE=local`): spawn ALL verifiers at once.**", + ) + _assert_github_verifier_flow_sync(github_section) + + mutated = github_section.replace("never set it to `true`", "it may be true") + with pytest.raises(AssertionError): + _assert_github_verifier_flow_sync(mutated) + + def test_github_verifier_flow_bans_watchers_sleep_and_taskoutput_waiting(self) -> None: + """Background waiting alternatives are equally unsafe in headless + GitHub mode and should fail the source contract. + """ + github_section = _extract_section( + _verify_findings_skill_text(), + "**GitHub mode (`MODE=github`): dispatch synchronously.**", + "**Local mode (`MODE=local`): spawn ALL verifiers at once.**", + ) + _assert_github_verifier_flow_sync(github_section) + + for forbidden_replacement in ( + "Use `TaskOutput` to wait for background verifiers.", + "Use watcher files to wait for background verifiers.", + "Use sleep loops to wait for background verifiers.", + "Use polling loops to wait for background verifiers.", + ): + mutated = re.sub( + r"Do not use `TaskOutput`, watcher files, sleep loops, polling loops,[^\n]+", + forbidden_replacement, + github_section, + ) + with pytest.raises(AssertionError): + _assert_github_verifier_flow_sync(mutated) + + def test_local_verifier_flow_preserves_background_collection(self) -> None: + """Local mode keeps parallel verifier dispatch and blocking + TaskOutput collection. + """ + skill = _verify_findings_skill_text() + local_section = _extract_section( + skill, + "**Local mode (`MODE=local`): spawn ALL verifiers at once.**", + "Each spawned verifier", + ) + local_collection = _extract_section( + skill, + "**Local collection (MANDATORY for `MODE=local`):**", + "A missing `agent_verifier_", + ) + assert "Use `run_in_background: true` on every verifier `Task`" in local_section + assert ( + "Call `TaskOutput` (block: true) for every spawned local background verifier" + in local_collection + ) + + mutated = local_section.replace("run_in_background: true", "run_in_background: false") + with pytest.raises(AssertionError): + assert "run_in_background: true" in mutated + + def test_skill_intro_describes_mode_specific_task_scheduling(self) -> None: + """The skill introduction must not call verifier spawning + mode-agnostic after FEA-3154 split scheduling by mode. + """ + intro = _extract_section( + _verify_findings_skill_text(), + "# Finding-Verifier Fleet Dispatch (stage_23_verify_findings)", + "## Verifier Fleet (stage_23_verify_findings)", + ) + _assert_mode_specific_prompt_contract(intro) + + mutated = intro.replace( + "with mode-specific Task scheduling", + "verifier spawning is mode-agnostic", + ) + with pytest.raises(AssertionError): + _assert_mode_specific_prompt_contract(mutated) + + def test_headless_warning_ties_github_verifier_to_synchronous_task_calls(self) -> None: + """The headless warning must keep GitHub verifier flow tied to + synchronous Task calls, not blocking TaskOutput collection. + """ + warning = _extract_from( + _verify_findings_skill_text(), + "**Headless mode warning", + ) + _assert_headless_warning_verifier_mode_specific(warning) + + mutated = warning.replace( + "GitHub verifier synchronous Task calls", + "the blocking `TaskOutput` collection", + ) + with pytest.raises(AssertionError): + _assert_headless_warning_verifier_mode_specific(mutated) + + def test_start_md_verifier_fleet_pins_stage_23_guard(self) -> None: + """start.md must not leave room for headless wait-loop substitutes at + the verifier fleet. + """ + verifier_fleet = _extract_section( + _start_command_text(), + "## Verifier Fleet (stage_23_verify_findings)", + "## PLN-725 Single-Agent Dispatch", + ) + _assert_start_md_github_stage_23_guard(verifier_fleet) + _assert_mode_specific_prompt_contract(verifier_fleet) + + for forbidden_replacement in ( + "Watcher files may replace synchronous verifier completion.", + "Sleep loops may replace synchronous verifier completion.", + "End the assistant turn and continue when notified.", + "Use background `TaskOutput` waits in GitHub mode.", + ): + mutated = re.sub( + r"Watcher files, sleep loops, polling loops, background `TaskOutput` waits,[^\n]+", + forbidden_replacement, + verifier_fleet, + ) + with pytest.raises(AssertionError): + _assert_start_md_github_stage_23_guard(mutated) + + def test_start_md_stage_23_note_pins_github_sync_completion(self) -> None: + """The orchestration spine's stage_23 note must pin the GitHub-mode + synchronous-completion clause. + """ + stage_note = _extract_section( + _start_command_text(), + "- **stage_23_verify_findings**", + "- **stage_24a_verify_consolidate**", + ) + _assert_start_md_github_stage_23_guard(stage_note) + + mutated = stage_note.replace( + "or fail before `stage_24a_verify_consolidate`", + "or continue later", + ) + with pytest.raises(AssertionError): + _assert_start_md_github_stage_23_guard(mutated) + + def _seed_phase9_inputs( tmp_path: Path, *, From 51053cd632e23827fecd857abe0b9a7f4e3025ff Mon Sep 17 00:00:00 2001 From: Peter Ulsteen Date: Thu, 16 Jul 2026 10:08:32 -0500 Subject: [PATCH 2/2] docs(code-review): note verify-consolidate as 3rd coverage_gaps.json producer (FEA-3154 review) Address review: _append_to_coverage_gaps docstring listed only arbitrate-budget and derive-spawn-spec; verify-consolidate (stage_24a) is now a third writer. Doc-only. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../tools/python/code_review_helpers.py | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/plugins/code-review/tools/python/code_review_helpers.py b/plugins/code-review/tools/python/code_review_helpers.py index f7551ed..84e9431 100644 --- a/plugins/code-review/tools/python/code_review_helpers.py +++ b/plugins/code-review/tools/python/code_review_helpers.py @@ -12595,14 +12595,17 @@ def _append_to_coverage_gaps( ) -> None: """Append findings to ``coverage_gaps.json`` preserving existing entries. - ``arbitrate-budget`` is the original producer of this file - (writing the ``budget_exceeded`` findings); ``derive-spawn-spec`` - is the second producer (writing the ``spawn_*`` reason findings). - Both run before ``stage_21_collect_findings`` which globs - ``agent_*.json`` and ``cmd_finalize_result`` which reads - ``coverage_gaps.json`` directly. The append-not-overwrite - contract keeps both producers' findings visible in the final - envelope. + Three producers write this file. ``arbitrate-budget`` is the + original (writing the ``budget_exceeded`` findings); ``derive-spawn-spec`` + is the second (writing the ``spawn_*`` reason findings); both run before + ``stage_21_collect_findings`` which globs ``agent_*.json``. + ``verify-consolidate`` (``stage_24a``, FEA-3154) is the third, appending a + single ``coverage:verifier-missing-output`` finding in github mode when a + BLOCKING/HIGH finding had no verifier output — it runs after + collect-findings but before ``stage_25_finalize_result``. All three are + picked up by ``cmd_finalize_result``, which reads ``coverage_gaps.json`` + directly. The append-not-overwrite contract keeps every producer's + findings visible in the final envelope. """ existing = _read_optional_json(gaps_path, None) if isinstance(existing, dict):