Skip to content

Commit 5c6b6ab

Browse files
peterulsteenclaude
andauthored
feat(code-review): synchronous GitHub-mode verifier dispatch + loud missing-verifier signal (FEA-3154) (#180)
* feat(code-review): synchronous GitHub-mode verifier dispatch + loud missing-verifier signal (FEA-3154) 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 9e923ed commit 5c6b6ab

11 files changed

Lines changed: 545 additions & 32 deletions

File tree

CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,11 @@ All notable changes to the claude-plugins project will be documented in this fil
44

55
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`.
66

7+
### code-review v3.7.0
8+
9+
#### Changed
10+
- **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.
11+
712
### code-review v3.6.1
813

914
#### Fixed

plugins/code-review/.claude-plugin/plugin.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"name": "code-review",
33
"description": "Code review plugin",
4-
"version": "3.6.1",
4+
"version": "3.7.0",
55
"author": {
66
"name": "ClosedLoop",
77
"email": "support@closedloop.ai"

plugins/code-review/commands/start.md

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -394,7 +394,7 @@ These notes annotate the run-plan stages with anything not obvious from the plan
394394
- **stage_20b_verify_spawn** (PLN-725): runs `verify-spawn`. Reads `<CR_DIR>/spawn.json` (`spec` section) and globs `<CR_DIR>/agent_*.json`; for every descriptor with `bucket: "required"` that has no on-disk output, appends a coverage-gap finding to `<CR_DIR>/coverage_gaps.json` (reason `spawn_missing_required_agent`) and records the omission in `<CR_DIR>/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.
395395
- **stage_22_validate**: writes `<CR_DIR>/findings_validated.json` via `> <CR_DIR>/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.
396396
- **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) `<CR_DIR>/verify_manifest.json` with `to_verify[]` + `skipped_no_verification[]` + `deferred_budget[]` + `cache_hits[]`, and (b) `<CR_DIR>/verifier_inputs/<finding_id>.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_<finding_id>.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.
397-
- **stage_23_verify_findings** (PLN-722): agent_fleet stage. Invoke the `code-review:verify-findings` skill. Each spawned agent reads its `verifier_inputs/<finding_id>.json` (containing the finding + the `verifier_prompt_path` + the canonical `output_path`) and emits one verdict file at `<CR_DIR>/agent_verifier_<finding_id>.json`. `on_failure: continue` so a single agent crash never aborts review.
397+
- **stage_23_verify_findings** (PLN-722): agent_fleet stage. Invoke the `code-review:verify-findings` skill. Each spawned agent reads its `verifier_inputs/<finding_id>.json` (containing the finding + the `verifier_prompt_path` + the canonical `output_path`) and emits one verdict file at `<CR_DIR>/agent_verifier_<finding_id>.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`.
398398
- **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 `<CR_DIR>/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`.
399399
- **stage_25_finalize_result** (PLN-722 + PLN-721): writes `<CR_DIR>/review_result.json` (the canonical envelope) BEFORE running schema validation. PLN-722: prefers `<CR_DIR>/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.
400400
- **stage_26_cache_update**: gated by **Gate C**.
@@ -421,6 +421,10 @@ Decomposition rationale: ~470 lines of reviewer-fleet dispatch content was extra
421421

422422
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.
423423

424+
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.
425+
426+
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.
427+
424428
<!-- replaced-by-skill: code-review:verify-findings — DO NOT add inline verifier-fleet dispatch content here -->
425429

426430
---

0 commit comments

Comments
 (0)