From 8cee50e49681d9ce6cbeebf249513c5aba0db572 Mon Sep 17 00:00:00 2001 From: Kris Wong Date: Fri, 26 Jun 2026 10:58:23 -0500 Subject: [PATCH 1/4] refactor(code-review): excise inert Premise category layer; SCHEMA_VERSION 2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The premise reviewer was removed earlier, so nothing emits `category: "Premise"` — the entire category-processing layer was dead code. This removes it and bumps the finding/result schema to v2. Removed: - `Premise` from the CATEGORIES schema enum. - The two verdict rules that gated on Premise findings (priority-0 -> CHANGES_REQUESTED; cumulative-Premise -> NEEDS_ATTENTION) plus `_count_gateable_premise_medium`, the `premise_cumulative_medium` threshold, and its `justification_rate_alert` companion. - Premise-scoped `stats` telemetry (`by_subcategory`, `justification`, `premise_cumulative_medium_count`) and the helpers that built them. - The verifier "always verify Premise" eligibility branch and the Premise extra-strictness blocks in verifier_prompt.txt. - The four `premise_*.md` fix templates + their /fix dispatch rows, and the reserved `golden_premise_*` fixtures. - Premise category-layer refs across shared/impact prompts, present-local and github-review rendering, SCHEMA.md, README.md, and start.md. Kept (general, not premise-specific): - The "PREMISE:" reasoning step every reviewer uses. - The author-justification machinery: `justified[]`, JUSTIFIED-VALID/ INVALID, the verifier J1/J2 audit, and the justification-audit learning stream (its `premise-justifications.jsonl` filename is kept stable for the self-learning consumer). SCHEMA_VERSION 1 -> 2 (folded into the prompt/cache hash, so BHA + verification caches invalidate once on rollout). verdict-thresholds.json now exposes only `impact_cumulative`; `stats` keeps `impact_cumulative_count`. Tests: 1209 pass, ruff + pyright clean. Deleted the dead premise test classes, cleaned stale fixtures to valid vocabulary, and regenerated the golden + run_plan snapshots (schema_version 1->2). Version 3.0.0 (MAJOR: breaking change to the review_result.json contract). Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 8 + .../code-review/.claude-plugin/plugin.json | 2 +- plugins/code-review/README.md | 13 +- plugins/code-review/SCHEMA.md | 17 +- plugins/code-review/commands/start.md | 10 +- plugins/code-review/prompts/github-review.md | 18 +- plugins/code-review/skills/fix/SKILL.md | 11 +- .../skills/fix/templates/premise_cohesion.md | 21 - .../fix/templates/premise_complexity.md | 22 - .../skills/fix/templates/premise_necessity.md | 15 - .../fix/templates/premise_workaround.md | 21 - .../code-review/skills/present-local/SKILL.md | 11 +- .../tools/prompts/impact_analyzer_prompt.txt | 6 +- .../tools/prompts/shared_prompt.txt | 12 +- .../tools/prompts/verifier_prompt.txt | 51 +- .../tools/python/code_review_helpers.py | 236 +------- .../tools/python/code_review_schema.py | 8 +- .../expected/review_result.json | 18 +- .../expected/review_result.json | 18 +- .../expected/review_result.json | 18 +- .../golden_premise_justified/README.md | 11 - .../golden_premise_rejected/README.md | 11 - .../expected/review_result.json | 18 +- .../github_pr42_all_flags.json | 4 +- .../local_no_pr_empty_flags.json | 4 +- .../tools/python/test_code_review_helpers.py | 507 +++--------------- .../tools/python/test_code_review_schema.py | 13 +- .../tools/python/test_golden_fixtures.py | 4 +- 28 files changed, 173 insertions(+), 935 deletions(-) delete mode 100644 plugins/code-review/skills/fix/templates/premise_cohesion.md delete mode 100644 plugins/code-review/skills/fix/templates/premise_complexity.md delete mode 100644 plugins/code-review/skills/fix/templates/premise_necessity.md delete mode 100644 plugins/code-review/skills/fix/templates/premise_workaround.md delete mode 100644 plugins/code-review/tools/python/fixtures/golden_premise_justified/README.md delete mode 100644 plugins/code-review/tools/python/fixtures/golden_premise_rejected/README.md diff --git a/CHANGELOG.md b/CHANGELOG.md index d7b461c1..d48a7e40 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,14 @@ 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.0.0 + +#### Removed +- Retired the `Premise` finding category and its entire processing layer, now inert since the premise reviewer was removed (no producer emits `category: "Premise"`). Dropped `Premise` from the `CATEGORIES` schema enum; removed the two verdict-precedence rules that gated on Premise findings (the Premise priority-0 → `CHANGES_REQUESTED` rule and the cumulative-Premise → `NEEDS_ATTENTION` rule) along with the `_count_gateable_premise_medium` helper, the `premise_cumulative_medium` operator threshold, and its `justification_rate_alert` companion; removed the Premise-scoped `stats` telemetry sub-blocks (`by_subcategory`, `justification`, `premise_cumulative_medium_count`); removed the verifier "always verify Premise" eligibility branch and the Premise extra-strictness blocks from `verifier_prompt.txt`; and deleted the four `premise_*.md` fix templates with their `/fix` dispatch rows. The shared/BHA reviewer prompts keep the generic "PREMISE:" reasoning step (what the code is supposed to do), and the general author-justification machinery — the `justified[]` bucket, `JUSTIFIED-VALID`/`JUSTIFIED-INVALID` verdicts, the verifier's J1/J2 audit, and the justification-audit learning stream — is retained; only the Premise-specific pieces were removed. + +#### Changed +- Bumped `SCHEMA_VERSION` 1 → 2 for the `Finding` + `ResultEnvelope` contract to reflect the removed `Premise` category and telemetry keys. Because the schema version is folded into the prompt/cache hash, this invalidates the Bug Hunter A and verification caches once on rollout. `verdict-thresholds.json` now exposes only `impact_cumulative` (the FEA-1401 Rule 6 Impact gate), and `stats` retains `impact_cumulative_count` in place of the removed premise count. + ### code-review v2.37.0 #### Changed diff --git a/plugins/code-review/.claude-plugin/plugin.json b/plugins/code-review/.claude-plugin/plugin.json index b59c2fa9..ecebd253 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": "2.37.0", + "version": "3.0.0", "author": { "name": "ClosedLoop", "email": "support@closedloop.ai" diff --git a/plugins/code-review/README.md b/plugins/code-review/README.md index edec5a8b..0664e84b 100644 --- a/plugins/code-review/README.md +++ b/plugins/code-review/README.md @@ -260,21 +260,19 @@ Each finding includes: file path, line number, severity, category, issue title, Operator-tunable knobs live under `.closedloop-ai/settings/`. All files are optional; absent or malformed entries fall back to built-in defaults. -### `verdict-thresholds.json` (PLN-721, PLN-773) +### `verdict-thresholds.json` (FEA-1401) -Tunes the verdict-precedence gates and the operator-facing telemetry alerts: +Tunes the verdict-precedence gates: ```json { - "premise_cumulative_medium": 3, - "justification_rate_alert": 0.30 + "impact_cumulative": 2 } ``` | Key | Default | Effect | |---|---|---| -| `premise_cumulative_medium` | `3` | Trigger `NEEDS_ATTENTION` when at least N MEDIUM Premise findings survive verification on the same PR, even if no individual finding is HIGH. Set to a very large number (e.g. `999`) to disable. Values below 1 are ignored. | -| `justification_rate_alert` | `0.30` | Threshold above which `stats.justification.threshold_alert` flips to `true` and the Verifier Stats footer flags the run. PLN-721 §Telemetry: "if > ~30%, authors likely gaming the hatch." Footer-only alert (does not modify the verdict). Set to `1.0` to disable. Values outside `[0.0, 1.0]` are ignored. | +| `impact_cumulative` | `2` | Trigger `NEEDS_ATTENTION` when at least N BLOCKING/HIGH `ImpactAnalysis` findings survive verification on the same PR (Rule 6), even if no single finding would gate on its own. Set to a very large number (e.g. `999`) to disable. Values below 1 are ignored. | ### `verification-gates.json` (PLN-722) @@ -307,8 +305,7 @@ The presenter (local mode `start.md`, GitHub mode `code-review-verifier-stats.md - Per-reviewer FP rate (`stats.verification.by_reviewer[*].fp_rate`) - Override count per reviewer (`stats.verification.by_reviewer[*].re_asserted`) -- Premise justification rate + rejection rate (`stats.justification.*`) -- Premise findings partitioned by subcategory (`stats.by_subcategory`) +- Justified-finding counts (`stats.verification.justified_valid_count` / `justified_invalid_count`) `pending-learnings/premise-justifications.jsonl` and `pending-learnings/verifier-overrides.jsonl` feed `self-learning:process-learnings` so the verifier's J2 (responsiveness) threshold and the per-reviewer FP-rate gate can tune over time. Both jsonl writers serialize via `fcntl.flock` so concurrent runs each get exactly one well-formed line per event. diff --git a/plugins/code-review/SCHEMA.md b/plugins/code-review/SCHEMA.md index fd740bd9..f38fd85f 100644 --- a/plugins/code-review/SCHEMA.md +++ b/plugins/code-review/SCHEMA.md @@ -37,7 +37,7 @@ shape. Producers may emit dicts directly; the Python convenience type lives in "system_marker": "", // ── Classification ──────────────────────────────────────── - "category": "Correctness | Code Quality | Documentation | Hygiene | Repo Hygiene | Premise | ImpactAnalysis | TestQuality | Coverage | InjectionAttempt | CompanionChange | Security", + "category": "Correctness | Code Quality | Documentation | Hygiene | Repo Hygiene | ImpactAnalysis | TestQuality | Coverage | InjectionAttempt | CompanionChange | Security", "subcategory": "", // ── Severity ────────────────────────────────────────────── @@ -181,7 +181,7 @@ The terminal artifact of every review run. // ── Stats ───────────────────────────────────────────────── "stats": { "by_severity": {"BLOCKING": , "HIGH": , "MEDIUM": }, - "by_category": {"Correctness": , "Premise": , ...}, + "by_category": {"Correctness": , ...}, "by_reviewer": {"": {"verified": , "rejected": , "tentative": , "justified": }}, "by_finding_scope": {"diff": , "system": , "pr_metadata": }, "verification": { @@ -194,7 +194,7 @@ The terminal artifact of every review run. "skipped_count": , "false_positive_rate": }, - "premise_cumulative_medium_count": , + "impact_cumulative_count": , "agent_failures": [{"agent_id": "", "reason": ""}] }, @@ -270,10 +270,9 @@ The `verdict` subcommand applies these rules in order; the first match wins. 1. Any coverage gap with `required: true` (foundation) → **CHANGES_REQUESTED** 2. Any BLOCKING finding (verified or system-scoped) → **CHANGES_REQUESTED** 3. Any HIGH finding (verified or system-scoped) → **NEEDS_ATTENTION** -4. ≥ N MEDIUM Premise findings (plan 02; default N=3) → **NEEDS_ATTENTION** -5. ≥ M BLOCKING/HIGH Impact Analysis findings (plan 06; default M=2) → **NEEDS_ATTENTION** -6. Any TENTATIVE finding (plan 03) → **NEEDS_ATTENTION** -7. Otherwise → **APPROVED** +4. ≥ M BLOCKING/HIGH Impact Analysis findings (plan 06; default M=2) → **NEEDS_ATTENTION** +5. Any TENTATIVE finding (plan 03) → **NEEDS_ATTENTION** +6. Otherwise → **APPROVED** `verdict_reason` cites the specific finding(s) that produced the verdict. @@ -651,8 +650,8 @@ commit diff, not auto-merged. **Phase 8 ships 3 fixtures end-to-end** (`golden_minimal_correctness`, `golden_all_categories`, `golden_schema_v1_round_trip`) plus a byte-identical determinism test -for `prepare-run`. The 6 fixtures requiring plans 01/02/03/05/06 -(`golden_premise_*`, `golden_impact_with_callsites`, +for `prepare-run`. The 4 fixtures requiring plans 03/05/06 +(`golden_impact_with_callsites`, `golden_coverage_gap`, `golden_injection_quarantine`, `golden_budget_exceeded`) have reserved directories with READMEs and are skipped via a `_DEFERRED_FIXTURES` map in the test module until diff --git a/plugins/code-review/commands/start.md b/plugins/code-review/commands/start.md index 3423ba32..6be59303 100644 --- a/plugins/code-review/commands/start.md +++ b/plugins/code-review/commands/start.md @@ -319,19 +319,19 @@ These conditions mirror the cache-update gate (Gate C) and the pre-Phase-4b "Rev Two optional operator-authored config files live under `.closedloop-ai/settings/`. Both are absent by default; the pipeline uses built-in defaults until they exist. -### `.closedloop-ai/settings/verdict-thresholds.json` (PLN-721) +### `.closedloop-ai/settings/verdict-thresholds.json` (FEA-1401) Override the verdict-precedence thresholds without forking the plugin: ```json { - "premise_cumulative_medium": 3 + "impact_cumulative": 2 } ``` | Key | Default | Effect | |-----|---------|--------| -| `premise_cumulative_medium` | `3` | Number of MEDIUM Premise findings on a single PR that flip the verdict to `NEEDS_ATTENTION` (Rule 4 in `_compute_canonical_verdict`). JUSTIFIED-VALID findings (routed to `justified[]`) and JUSTIFIED-INVALID findings (kept in `verified[]` with the verifier flag) are excluded from the count. Set higher to relax the gate; raise above any realistic finding count to disable. | +| `impact_cumulative` | `2` | Number of BLOCKING/HIGH `ImpactAnalysis` findings on a single PR that flip the verdict to `NEEDS_ATTENTION` (Rule 6 in `_compute_canonical_verdict`). Set higher to relax the gate; raise above any realistic finding count to disable. | Unknown keys are ignored. Non-integer or `< 1` values fall back to the default — the file is operator-authored and should not crash the pipeline on a typo. @@ -364,10 +364,10 @@ These notes annotate the run-plan stages with anything not obvious from the plan - **stage_20_spawn_reviewers**: agent_fleet stage. Invoke the `code-review:spawn-reviewers` skill. The skill reads `/spawn.json` (`spec` section) first and dispatches one Task per agent descriptor (using the `agent_id`, `reviewer`, `model`, and `patches_file` from the spec). If `spawn.json` is missing, its `spec` section is absent, or it marks `arbitrate_status: "fallback"`, the skill walks its static reviewer table fallback instead — a derive failure must never block review. In `MODE=github`, the walker must follow the skill's synchronous standard-flow branch: do not use `TaskOutput`, watcher files, sleep loops, polling loops, or turn-ending waits as replacements for synchronous reviewer completion. `stage_20` must complete every GitHub synchronous reviewer and retry, leaving no reviewer task still running, or fail before `stage_21_collect_findings`. - **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; `category: "Premise"` always (strict adversarial framing). 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_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_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 `premise_cumulative_medium=3`; absent/malformed → built-in default) so `_compute_canonical_verdict` Rule 4 can fire (≥ 3 MEDIUM Premise 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_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` Rule 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**. - **stage_27_review_state_write**: gated by **Gate D**. - **stage_29_present**: present stage. Invoke the `code-review:present-local` skill (MODE=local) or follow Steps 6 and 8 in `github-review.md` (MODE=github). The mode-agnostic Gate A hygiene-only early-exit fires before this stage and uses its own format section above. diff --git a/plugins/code-review/prompts/github-review.md b/plugins/code-review/prompts/github-review.md index 62a66cc8..14654b2c 100644 --- a/plugins/code-review/prompts/github-review.md +++ b/plugins/code-review/prompts/github-review.md @@ -255,7 +255,6 @@ Also write `.closedloop-ai/code-review-justified-summary.md` (skip when `justifi **Finding ID:** `{ID}` **Original reviewer:** {REVIEWER} -**Subcategory:** `{SUBCATEGORY}` (Premise findings only) **Verifier verdict:** JUSTIFIED-VALID **Verifier confidence:** {VERIFIER_CONFIDENCE} @@ -277,13 +276,13 @@ Mark todo as `completed`. ### 6e: Write Verifier Stats (PLN-773) -Read `$CR_DIR/review_result.json` → `stats.verification` and `stats.justification`. If neither block is present (very old envelope), skip this step. Otherwise write `.closedloop-ai/code-review-verifier-stats.md` (the workflow posts this as a collapsible `
` block in a single comment so the metrics are visible to PR reviewers without polluting inline review comments): +Read `$CR_DIR/review_result.json` → `stats.verification`. If the block is not present (very old envelope), skip this step. Otherwise write `.closedloop-ai/code-review-verifier-stats.md` (the workflow posts this as a collapsible `
` block in a single comment so the metrics are visible to PR reviewers without polluting inline review comments): ```markdown ## Verifier Stats
-{verified_count} verified · {rejected_count} dismissed · {justified_emitted} justified +{verified_count} verified · {rejected_count} dismissed · {justified_valid_count + justified_invalid_count} justified **Verifier outcomes** - CONFIRMED + DOWNGRADE: {verified_count - tentative_count - re_asserted} @@ -291,11 +290,10 @@ Read `$CR_DIR/review_result.json` → `stats.verification` and `stats.justificat - RE_ASSERTED: {sum over by_reviewer[].re_asserted} - REJECTED: {rejected_count} -**Justification (PLN-721 escape hatch)** -- Justified emitted: {justified_emitted} ({rate:.0%} of Premise total) -- JUSTIFIED-VALID: {justified_valid} -- JUSTIFIED-INVALID: {justified_invalid} (rejection rate {rejection_rate:.0%}) -- Threshold alert: {threshold_alert} (alerts when rate > {justification_rate_alert:.0%}) +**Justification (PLN-721)** — read from `stats.verification` +- Justified emitted: {justified_valid_count + justified_invalid_count} +- JUSTIFIED-VALID: {justified_valid_count} +- JUSTIFIED-INVALID: {justified_invalid_count} **Per-reviewer FP rate** (rejected / audited) | Reviewer | Verified | Rejected | FP rate | Re-asserts | @@ -304,10 +302,6 @@ Read `$CR_DIR/review_result.json` → `stats.verification` and `stats.justificat The Reviewer column keys off the `reviewer` field, which `cmd_collect_findings` derives from the agent filename (`agent_bha_p0.json` → `reviewer='bha_p0'`). Under partitioned mode the table shows one BHA row per partition (`bha_p0`, `bha_p1`, …); under unified mode it shows a single `bha_p0` row because only one partition exists. -**Premise MEDIUM cumulative gate** -- Current count: {premise_cumulative_medium_count} -- Gate threshold: {premise_cumulative_medium} - **Partition mode** ({verify_manifest.partition_mode}, {verify_manifest.partition_count} partitions) — read from `/verify_manifest.json`. Omit this line when the manifest file is absent (hygiene-only run or pre-PLN-774 cache).
diff --git a/plugins/code-review/skills/fix/SKILL.md b/plugins/code-review/skills/fix/SKILL.md index 3be9a988..2f619d58 100644 --- a/plugins/code-review/skills/fix/SKILL.md +++ b/plugins/code-review/skills/fix/SKILL.md @@ -6,7 +6,7 @@ argument-hint: " [--include-medium] [--include-tentative] [--include-jus # Fix Code Review Findings -Category-dispatch fix flow for findings from a prior code review. Each finding is routed to one of four buckets based on its category/subcategory: **auto-fix**, **callsite-fix**, **specialized-fix**, or **manual-surface**. Premise findings, injection attempts, sensitive-file changes, and coverage gaps never auto-apply — they surface in a manual-action report instead. +Category-dispatch fix flow for findings from a prior code review. Each finding is routed to one of four buckets based on its category/subcategory: **auto-fix**, **callsite-fix**, **specialized-fix**, or **manual-surface**. Injection attempts, sensitive-file changes, and coverage gaps never auto-apply — they surface in a manual-action report instead. The caller controls re-review cycles. @@ -105,11 +105,6 @@ For each surviving finding, look up its dispatch bucket using the table below. S | `Hygiene` / `Repo Hygiene` | `gitignore_drift` | **auto-fix** | Add gitignore entry. Only auto-fix when the producer emits this exact subcategory — see `_check_gitignore_drift`. | | `Hygiene` / `Repo Hygiene` | `sensitive_files` | **manual-surface** | Never auto-modify (.env, credentials, .pem). Producer is `_check_sensitive_files`. | | `Hygiene` / `Repo Hygiene` | (other / unset / unrecognized) | **manual-surface** | **Fail-safe default**: any Hygiene finding without a recognized subcategory routes to manual-surface. Prevents a hygiene producer that forgets to set `subcategory` (or a future producer that emits a subcategory we haven't routed yet) from silently falling into auto-fix on a sensitive file. | -| `Premise` | `necessity` | **manual-surface** | Auto-revert unsafe | -| `Premise` | `cohesion` | **manual-surface** | Surface cited 5+ examples | -| `Premise` | `workaround` | **manual-surface** | Surface root cause location | -| `Premise` | `complexity` | **manual-surface** | Both over- and under-complexity surfaced for author input (the auto-fix path for over-complexity / single-use abstraction is deferred to a follow-up phase) | -| `Premise` | (any other) | **manual-surface** | Fail safe | | `TestQuality` | `missing-coverage` | **manual-surface** | Specialized-fix flow (test-engineer) is **deferred to PLN-723 ship** | | `TestQuality` | `weak-assertion` | **manual-surface** | Deferred to PLN-723 | | `TestQuality` | `mock-faithfulness` | **manual-surface** | Deferred to PLN-723 | @@ -200,10 +195,6 @@ For each finding in the manual-surface bucket, look up the template per the rout | Routing | Template file | |---|---| -| `Premise/necessity` | `templates/premise_necessity.md` | -| `Premise/cohesion` | `templates/premise_cohesion.md` | -| `Premise/workaround` | `templates/premise_workaround.md` | -| `Premise/complexity` | `templates/premise_complexity.md` | | `TestQuality/bug-locking` | `templates/testquality_bug_locking.md` | | `TestQuality/test-deletion` | `templates/testquality_test_deletion.md` | | `TestQuality/*` (other, pre-PLN-723) | `templates/testquality_specialized.md` | diff --git a/plugins/code-review/skills/fix/templates/premise_cohesion.md b/plugins/code-review/skills/fix/templates/premise_cohesion.md deleted file mode 100644 index d690f22b..00000000 --- a/plugins/code-review/skills/fix/templates/premise_cohesion.md +++ /dev/null @@ -1,21 +0,0 @@ -### ⚠️ Premise / Cohesion — `{file}:{line}` ({severity}) - -**Issue:** {issue} - -**Prevailing pattern the codebase already follows:** -{reasoning_certificate.prevailing_pattern.description} - -**Cited examples** (≥ 5 sibling sites when `is_duplicate_abstraction = false`, ≥ 1 when `true`): -{reasoning_certificate.prevailing_pattern.examples} - -**Duplicate abstraction?** {reasoning_certificate.is_duplicate_abstraction} - -**Why this is surfaced manually:** Auto-refactoring to match the prevailing pattern would be a structural change to working code. The author may have a deliberate reason to diverge. - -**Your options:** - -1. **Refactor** `{file}:{line}` to match the prevailing pattern shown above. When `is_duplicate_abstraction` is `true`, the canonical fix is usually to delete the new abstraction and call the existing one from the cited example. -2. **Justify the divergence** with a code comment within 5 lines of `{file}:{line}` explaining the reason (e.g., performance hot path, deliberate isolation, in-flight migration). -3. **Re-assert** via `python3 /tools/python/code_review_helpers.py re-assert --cr-dir --cache-dir --finding-ids {id}` if the reviewer's prevailing-pattern analysis is wrong. (Resolve `` from `/cache_config.json:cache_dir`.) - -**Original recommendation:** {recommendation} diff --git a/plugins/code-review/skills/fix/templates/premise_complexity.md b/plugins/code-review/skills/fix/templates/premise_complexity.md deleted file mode 100644 index 3ed3d5b5..00000000 --- a/plugins/code-review/skills/fix/templates/premise_complexity.md +++ /dev/null @@ -1,22 +0,0 @@ -### ⚠️ Premise / Complexity — `{file}:{line}` ({severity}) - -**Issue:** {issue} - -**Use-site count vs justification threshold:** -- **Observed use sites:** {reasoning_certificate.use_site_count} -- **Threshold that would have justified this complexity:** {reasoning_certificate.justification_threshold} -- **Grep pattern the reviewer used to count:** `{reasoning_certificate.grep_pattern_used}` - -**Sites observed:** -{reasoning_certificate.sites} - -**Why this is surfaced manually:** Complexity findings say the machinery introduced here (caching, batching, memoization, configuration surface, generic abstraction, …) cannot be justified by how many places actually use it. The auto-fix path for single-use abstractions is planned but not yet shipped — until then, every Premise/Complexity finding is surfaced for author decision. - -**Your options:** - -1. **Simplify** — remove the abstraction layers / parameters / config surface the observed use-site count does not warrant. The reviewer's `grep_pattern_used` is the right starting point to re-check the call sites listed above. -2. **Add the missing callers** — if you know of imminent use sites that bring the count above `{reasoning_certificate.justification_threshold}`, add them in this PR (the count is then provable, not promised). -3. **Justify** with a code comment within 5 lines of `{file}:{line}` naming the second call site or near-future use case. -4. **Re-assert** via `python3 /tools/python/code_review_helpers.py re-assert --cr-dir --cache-dir --finding-ids {id} --reason ''` if you believe the reviewer's count is wrong. (Resolve `` from `/cache_config.json:cache_dir`.) - -**Original recommendation:** {recommendation} diff --git a/plugins/code-review/skills/fix/templates/premise_necessity.md b/plugins/code-review/skills/fix/templates/premise_necessity.md deleted file mode 100644 index c18e57bf..00000000 --- a/plugins/code-review/skills/fix/templates/premise_necessity.md +++ /dev/null @@ -1,15 +0,0 @@ -### ⚠️ Premise / Necessity — `{file}:{line}` ({severity}) - -**Issue:** {issue} - -**Counter-evidence from the codebase:** -{reasoning_certificate.counter_evidence} - -**Why this is surfaced manually:** The reviewer determined the stated motivation for this change is contradicted by codebase evidence. Auto-reverting is unsafe — the original author may have context the reviewer lacks. - -**Your options:** -1. **Revert** the change if the counter-evidence above is correct. -2. **Justify** the change by adding a code comment within 5 lines of `{file}:{line}` explaining why the counter-evidence does not apply here. -3. **Re-assert** the original intent via `python3 /tools/python/code_review_helpers.py re-assert --cr-dir --cache-dir --finding-ids {id} --reason ''` if you believe the reviewer is wrong. (Resolve `` from `/cache_config.json:cache_dir`.) - -**Original recommendation:** {recommendation} diff --git a/plugins/code-review/skills/fix/templates/premise_workaround.md b/plugins/code-review/skills/fix/templates/premise_workaround.md deleted file mode 100644 index d85547a1..00000000 --- a/plugins/code-review/skills/fix/templates/premise_workaround.md +++ /dev/null @@ -1,21 +0,0 @@ -### ⚠️ Premise / Workaround — `{file}:{line}` ({severity}) - -**Issue:** {issue} - -**Root cause** (per the reviewer's `workaround` certificate): -- **Location:** `{reasoning_certificate.root_cause_location.file}:{reasoning_certificate.root_cause_location.line}` -- **Ownership:** {reasoning_certificate.root_cause_ownership} -- **Why not fixed at the source:** {reasoning_certificate.why_not_fixed_at_source} - -**Why this is surfaced manually:** The reviewer believes this change works around a problem instead of addressing the root cause. Auto-fixing the root cause is out of scope for `/fix` — that change usually belongs in a separate PR. - -**Your options:** - -1. **Address the root cause** at `{reasoning_certificate.root_cause_location.file}:{reasoning_certificate.root_cause_location.line}` — preferred when ownership is `in-repo` (the certificate's `why_not_fixed_at_source` should be empty in that case; any non-empty value is the workaround justification). -2. **Document the workaround** in the PR description with a link to a tracking issue / follow-up PR that addresses the root cause. -3. **Justify** with a code comment within 5 lines of `{file}:{line}` if the workaround is the intentional final state. -4. **Re-assert** via `python3 /tools/python/code_review_helpers.py re-assert --cr-dir --cache-dir --finding-ids {id}` if the analysis is wrong. (Resolve `` from `/cache_config.json:cache_dir`.) - -> Note: the Premise prompt only emits actionable workaround findings when `root_cause_ownership == "in-repo"`. If you see `upstream-dep` or `external-service` here, the reviewer should have discarded the finding — flag this as a Premise prompt regression. - -**Original recommendation:** {recommendation} diff --git a/plugins/code-review/skills/present-local/SKILL.md b/plugins/code-review/skills/present-local/SKILL.md index c31c5894..2a2bbac9 100644 --- a/plugins/code-review/skills/present-local/SKILL.md +++ b/plugins/code-review/skills/present-local/SKILL.md @@ -124,7 +124,6 @@ For each justified finding: ### [{ORIGINAL_SEVERITY} justified] {FILE}:{LINE} — {ISSUE_HEAD} **Finding ID:** `{ID}` **Original reviewer:** {REVIEWER} -**Subcategory:** `{SUBCATEGORY}` (Premise findings only) **Verifier verdict:** JUSTIFIED-VALID **Verifier confidence:** {VERIFIER_CONFIDENCE} @@ -186,7 +185,7 @@ If `review_result.json.pending_verification[]` is non-empty, append a one-line n ## Verifier Stats (PLN-773) -Read `/review_result.json` → `stats.verification` and `stats.justification`. Render the footer below verbose-by-design — operators read the per-reviewer FP rate and the justification rate to detect over-rejection (reviewer hallucinating) and escape-hatch abuse (authors gaming the gate) respectively. +Read `/review_result.json` → `stats.verification`. Render the footer below verbose-by-design — operators read the per-reviewer FP rate to detect over-rejection (a reviewer hallucinating findings the verifier then discards). ``` === Verifier Stats === @@ -195,13 +194,11 @@ Findings verified: {stats.verification.verified_count} - TENTATIVE: {tentative_count} - RE_ASSERTED: sum over by_reviewer[].re_asserted Findings dismissed: {stats.verification.rejected_count} -Findings justified: {stats.justification.justified_emitted} - - JUSTIFIED-VALID: {stats.justification.justified_valid} - - JUSTIFIED-INVALID: {stats.justification.justified_invalid} +Findings justified: {stats.verification.justified_valid_count + stats.verification.justified_invalid_count} + - JUSTIFIED-VALID: {stats.verification.justified_valid_count} + - JUSTIFIED-INVALID: {stats.verification.justified_invalid_count} Reviewers (FP rate / overrides): {reviewer}: {fp_rate:.2f} / {re_asserted}{ " ⚠ override" if re_asserted > 0 else "" } -Justification rate: {stats.justification.rate:.2f} (threshold {threshold} — {ALERT|OK}) -Premise MEDIUM cumulative: {stats.premise_cumulative_medium_count} (gate threshold {premise_cumulative_medium}) Impact gateable count: {stats.impact_cumulative_count} (gate threshold {impact_cumulative}) Partition mode: {verify_manifest.partition_mode} ({verify_manifest.partition_count} partitions) ``` diff --git a/plugins/code-review/tools/prompts/impact_analyzer_prompt.txt b/plugins/code-review/tools/prompts/impact_analyzer_prompt.txt index 2e01b6c1..bfbf7bad 100644 --- a/plugins/code-review/tools/prompts/impact_analyzer_prompt.txt +++ b/plugins/code-review/tools/prompts/impact_analyzer_prompt.txt @@ -35,8 +35,7 @@ usage is still compatible with the change. No other reviewer does this. BHA reviews the diff itself for bugs; BHB looks for sibling-pattern drift; the unified auditor handles style -and small correctness issues; premise questions whether the change -should have happened at all. None of them read code that isn't in the +and small correctness issues. None of them read code that isn't in the diff. **You do.** That is the entire point of this reviewer — and the only thing you should be doing. @@ -349,7 +348,8 @@ analyze 1–5 symbols, run 5–15 greps, and finish in under a minute. breaks. The break is in the `external_impact[]` entry. - You may NOT emit findings about the symbol's internal logic — that is BHA's job. You may NOT emit findings about whether the change - should have happened — that is Premise's job. + should have happened at all — that subjective judgment is out of + scope for this reviewer. - You may NOT use Bash. Use Read, Grep, Glob — plus the read-only `mcp__codebase-memory-mcp__*` graph tools when available (see Inputs). - If a callsite_snippet contains whitespace or quotes, preserve them diff --git a/plugins/code-review/tools/prompts/shared_prompt.txt b/plugins/code-review/tools/prompts/shared_prompt.txt index 853f8e0a..6873ef39 100644 --- a/plugins/code-review/tools/prompts/shared_prompt.txt +++ b/plugins/code-review/tools/prompts/shared_prompt.txt @@ -204,7 +204,7 @@ Why it's wrong: You found this while grepping for context, but session-manager.t Before outputting your findings, reason through each one in tags: 1. Is this finding about code AFFECTED by the diff — either a file in , or an unchanged file/line the diff demonstrably broke? (If it's about entirely unrelated unchanged code, discard.) 2. PREMISE: What is this code supposed to do? [one sentence citing function/context] -3. EVIDENCE: What concrete evidence proves the issue? For correctness bugs, trace the execution path. For DRY, convention, or premise findings, cite the structural or rule-based contradiction. [cite file:line for each evidence point] +3. EVIDENCE: What concrete evidence proves the issue? For correctness bugs, trace the execution path. For DRY or convention findings, cite the structural or rule-based contradiction. [cite file:line for each evidence point] 4. CAUSATION (when the finding line is NOT in the diff): Cite the specific diff change that caused this finding. Without a clear causal link, downgrade or discard — the finding may belong in a separate PR. 5. GUARD CHECK: Is there error handling, validation, or upstream logic that prevents this? [cite search result or "verified none exists at file:line"] 6. SEVERITY CHECK: Does the evidence support the assigned severity? [cite the triggering input/state] @@ -221,7 +221,6 @@ The "category" field MUST be one of these canonical values (the schema validator - Documentation — README/docstring/comment accuracy or completeness - Hygiene — generic source-tree hygiene (trailing whitespace, etc.) - Repo Hygiene — repo-level hygiene findings emitted by the hygiene helper - - Premise — change premise/justification (Premise pass only) - ImpactAnalysis — cross-file impact / callsite breakage analysis - TestQuality — tests that don't actually test, weak assertions, flaky setup - Coverage — files/scopes not covered by review (coverage pass only) @@ -251,12 +250,9 @@ The JSON format: OPTIONAL canonical fields (the validator preserves these through the pipeline; you may emit them when relevant, otherwise omit): - - "subcategory": string — only the Premise reviewer uses this today - (necessity | cohesion | workaround | complexity). Other reviewers - may set it for finer-grained labels. - - "reasoning_certificate": object — Premise findings MUST emit this - with a shape matching their subcategory (premise_prompt.txt - documents the required fields). The Impact Analyzer MUST emit + - "subcategory": string — optional finer-grained label a reviewer may + set on a finding. + - "reasoning_certificate": object — the Impact Analyzer MUST emit `{kind: "impact", fields: {...}}` per impact_analyzer_prompt.txt. Other reviewers (BHA, BHB, auditor, test_quality, sibling_pattern) may emit one too but it is not required. diff --git a/plugins/code-review/tools/prompts/verifier_prompt.txt b/plugins/code-review/tools/prompts/verifier_prompt.txt index 8bb1bfd9..4da3b7e3 100644 --- a/plugins/code-review/tools/prompts/verifier_prompt.txt +++ b/plugins/code-review/tools/prompts/verifier_prompt.txt @@ -107,37 +107,15 @@ or is it a generic disclaimer? verdict will be whatever the six checks produce — see "After the justification audit" below for the exact emission rule. -### Premise-specific justification audit (extra strictness) - -For findings where `category == "Premise"`, also confirm that the -justification rebuts the specific subcategory: - - - `subcategory == "necessity"`: the justification must explain why the - "phantom bug" is, in fact, a real bug — citing the trigger path. - - `subcategory == "cohesion"`: the justification must explain why the - prevailing pattern doesn't apply here (e.g., the new code lives in - a layer the convention doesn't cover). - - `subcategory == "workaround"`: the justification must explain why - fixing at the root-cause location is not feasible (e.g., wire-format - freeze, external dep). - - `subcategory == "complexity"`: the justification must explain the - expected near-term use-site growth (e.g., "three more callers in - PR #4321 land next week"), and that follow-up must be cited. - -Premise justifications that fail this subcategory-specific check are -treated as J2 failures even when J2 looked plausible at first read — -fall through to the six-check protocol below on the same emission -contract as J1/J2 failures. - ### After the justification audit — verdict emission rules There are exactly two outcomes you may emit from the audit alone: - - **Audit passes** (J1 OK AND J2 OK AND any Premise subcategory check - OK) → emit `verifier_verdict: "JUSTIFIED-VALID"`. Skip the six-check + - **Audit passes** (J1 OK AND J2 OK) → emit + `verifier_verdict: "JUSTIFIED-VALID"`. Skip the six-check protocol; write the verdict directly using the Output shape below. - - **Audit fails** (J1 fails, OR J2 fails, OR a Premise subcategory - check fails) → **continue into the six-check protocol below.** Your + - **Audit fails** (J1 fails OR J2 fails) → **continue into the + six-check protocol below.** Your final emitted verdict is whatever the six checks produce (CONFIRMED, DOWNGRADE, TENTATIVE, or REJECTED). **Do NOT set `verifier_verdict: "JUSTIFIED-INVALID"`** on the fall-through path — @@ -248,24 +226,6 @@ distinction: - "I searched for the issue and could not find supporting evidence" → that is absence of evidence; TENTATIVE. -## Premise findings (extra strictness) - -For findings where `category == "Premise"`, also re-execute the embedded -reasoning_certificate's claim chain independently: - - - **Necessity**: re-check the "AUTHOR'S CLAIM → COUNTER-EVIDENCE → - ALTERNATIVE CHECK" walk. The counter-evidence citation must exist. - - **Cohesion**: confirm the certificate's `prevailing_pattern` cites - ≥ 5 examples in the codebase (≥ 1 for duplicate-abstraction). Grep - each cited file:line. - - **Workaround**: confirm `root_cause_location` exists and - `root_cause_ownership` matches reality. - - **Complexity**: re-count `use_site_count` for over-complexity claims. - -Premise findings whose certificate can't be re-validated → REJECTED -with `rejection_class: "evidence_not_found"` (the underlying claim -chain failed). - ## Impact Analyzer findings (FEA-1401 / PLN-726) For findings where `category == "ImpactAnalysis"` AND `external_impact[]` @@ -438,8 +398,7 @@ Rules: the four enumerated classes. At least one `evidence_checks[]` entry must have `verified: false` with a concrete `actual_read`. - **JUSTIFIED-VALID** (PLN-721): the author's justification passed both - J1 (existence) and J2 (responsiveness), including any subcategory- - specific check for Premise findings. `verifier_severity` is null. + J1 (existence) and J2 (responsiveness). `verifier_severity` is null. `rejection_class` is null. `evidence_checks[]` should include the audit checks (claim = "justification exists at ", claim = "justification addresses the specific concern"). diff --git a/plugins/code-review/tools/python/code_review_helpers.py b/plugins/code-review/tools/python/code_review_helpers.py index 3a7be3a5..e2875d29 100644 --- a/plugins/code-review/tools/python/code_review_helpers.py +++ b/plugins/code-review/tools/python/code_review_helpers.py @@ -989,10 +989,9 @@ def _check_tier_mismatch_nudge( finding's marker (``tier_mismatch_nudge``) makes it easy to auto-dismiss or filter out at the operator's discretion. Severity is MEDIUM — the lowest tier that survives validate's - SEVERITY_NORMALIZE map (which DISCARDs "low"). Category "Coverage" - rather than "Premise" keeps it out of the cumulative Premise - MEDIUM gate (Rule 4 of _compute_canonical_verdict) so it cannot - escalate the verdict on its own. + SEVERITY_NORMALIZE map (which DISCARDs "low"). It carries category + "Coverage" — a MEDIUM coverage nudge that surfaces to the operator + without escalating the verdict on its own. """ if depth != "shallow": return [] @@ -1028,9 +1027,7 @@ def _check_tier_mismatch_nudge( # MEDIUM severity (not LOW) so the finding survives validate's # SEVERITY_NORMALIZE map (which DISCARDs "low"). MEDIUM is the # lowest severity that reaches the operator. Category "Coverage" - # rather than "Premise" so it does not contribute to the cumulative - # Premise MEDIUM gate (Rule 4 of _compute_canonical_verdict) and - # cannot accidentally escalate the verdict on its own. + # keeps it a non-escalating coverage nudge. return [{ "reviewer": "hygiene", "source": "hygiene", @@ -2098,7 +2095,6 @@ def cmd_validate(args: argparse.Namespace) -> int: # | LOW (P3) | No | # | category=Hygiene | No (deterministic producer) | # | source=injection-detector | No (deterministic producer) | -# | category=Premise | Always (strict adversarial framing) | # # MAX_VERIFICATIONS = 50 (PLN-722: 50 × Sonnet ≈ $2/PR at current pricing). # Overflow ranks by (severity_weight × confidence) and tags the bottom of @@ -2159,6 +2155,9 @@ def _verifications_cache_path(cache_dir: Path, key: str) -> Path: # --------------------------------------------------------------------------- _PENDING_LEARNINGS_DIR = Path(".closedloop-ai/pending-learnings") +# General justification-audit learning stream — every JUSTIFIED-INVALID +# verdict (any category), feeding the verifier's J2 tuning. The historical +# "premise-" filename is kept stable for the self-learning consumer. _PENDING_LEARNINGS_PREMISE = "premise-justifications.jsonl" _PENDING_LEARNINGS_OVERRIDES = "verifier-overrides.jsonl" @@ -2583,13 +2582,6 @@ def _needs_verification(finding: dict[str, Any]) -> bool: if finding.get("out_of_hunk_kept") is True: return True - # Premise: always verified with the strict adversarial framing in - # verifier_prompt.txt — the verdict precedence already gives Premise - # a high blast radius (cumulative MEDIUM gate in PLN-721), so every - # Premise finding needs an independent second opinion. - if category == "Premise": - return True - if severity in ("BLOCKING", "HIGH"): return True @@ -2831,21 +2823,11 @@ def cmd_verify_prepare(args: argparse.Namespace) -> int: # = no escalation; bootstrap does NOT auto-generate per `00-discovery.md`. _VERIFICATION_GATES_DEFAULT_PATH = Path(".closedloop-ai/settings/verification-gates.json") -# PLN-721 Phase 4: Premise cumulative-MEDIUM verdict gate. Operator-overridable -# via ``.closedloop-ai/settings/verdict-thresholds.json``; absent/malformed → -# the default fires at 3 (matches the plan's design intent: "a single MEDIUM -# does not block; three MEDIUM Premise findings on the same PR signal the -# patch is structurally wrong even when no individual line is dangerous"). +# Operator-overridable verdict thresholds via +# ``.closedloop-ai/settings/verdict-thresholds.json``; absent/malformed → +# the built-in defaults. _VERDICT_THRESHOLDS_DEFAULT_PATH = Path(".closedloop-ai/settings/verdict-thresholds.json") -_VERDICT_PREMISE_MEDIUM_THRESHOLD_DEFAULT = 3 -# PLN-773 Premise justification rate alert. Fires when the share of -# Premise findings carrying author justification crosses the threshold -# (PLN-721 §Telemetry: "if > ~30%, authors likely gaming the hatch"). -# Operator-tunable via the same verdict-thresholds.json config. -_VERDICT_JUSTIFICATION_RATE_ALERT_DEFAULT = 0.30 _VERDICT_THRESHOLD_KEYS: tuple[str, ...] = ( - "premise_cumulative_medium", - "justification_rate_alert", "impact_cumulative", ) @@ -2886,37 +2868,21 @@ def _load_verdict_thresholds(path: Path | None) -> dict[str, Any]: Returns a dict with the canonical keys present: - - ``premise_cumulative_medium`` (int, ≥ 1): MEDIUM Premise count - gate. Default 3. - - ``justification_rate_alert`` (float, [0.0, 1.0]): threshold above - which the justification-rate footer flips to ALERT. Default 0.30. - ``impact_cumulative`` (int, ≥ 1): BLOCKING/HIGH ImpactAnalysis cumulative gate (FEA-1401 Rule 6). Default 2. Unknown keys are ignored. Invalid entries (wrong type, out of range) fall back to the default — the file is operator-authored and should not crash the pipeline on a typo or a "0" that would disable a gate - entirely (use a very large number / 1.0 for that respectively). + entirely (use a very large number for that). """ defaults: dict[str, Any] = { - "premise_cumulative_medium": _VERDICT_PREMISE_MEDIUM_THRESHOLD_DEFAULT, - "justification_rate_alert": _VERDICT_JUSTIFICATION_RATE_ALERT_DEFAULT, "impact_cumulative": _VERDICT_IMPACT_THRESHOLD_DEFAULT, } data, out = _load_optional_settings_dict(path, defaults) if data is None: return out # Per-key validation (each threshold has its own range constraints). - raw_pm = data.get("premise_cumulative_medium") - if isinstance(raw_pm, int) and not isinstance(raw_pm, bool) and raw_pm >= 1: - out["premise_cumulative_medium"] = raw_pm - raw_jr = data.get("justification_rate_alert") - if ( - isinstance(raw_jr, (int, float)) - and not isinstance(raw_jr, bool) - and 0.0 <= float(raw_jr) <= 1.0 - ): - out["justification_rate_alert"] = float(raw_jr) raw_ic = data.get("impact_cumulative") if isinstance(raw_ic, int) and not isinstance(raw_ic, bool) and raw_ic >= 1: out["impact_cumulative"] = raw_ic @@ -9453,7 +9419,7 @@ def _count_gateable_impact(verified: list[dict[str, Any]]) -> int: the verdict escalates to NEEDS_ATTENTION even if no single finding would have triggered Rule 2 or Rule 3 on its own. - Counting policy mirrors ``_count_gateable_premise_medium``: + Counting policy: - Only ``verified[]`` findings (justified/rejected/coverage_gap buckets are bucketed elsewhere; Impact findings flow through @@ -9497,45 +9463,6 @@ def _count_gateable_impact(verified: list[dict[str, Any]]) -> int: _VERDICT_IMPACT_THRESHOLD_DEFAULT = 2 -def _count_gateable_premise_medium(verified: list[dict[str, Any]]) -> int: - """Return the count Rule 4's Premise-MEDIUM gate fires on. - - Shared between ``_compute_canonical_verdict`` (Rule 4) and - ``_stats_from_findings`` (telemetry's - ``premise_cumulative_medium_count``) so the value the gate triggers - on always matches the value the operator-facing telemetry reports. - Counting policy: - - - Only ``verified[]`` findings (``justified[]`` is bucketed - elsewhere; ``rejected[]`` is dropped from the verdict; and - ``coverage_gaps`` never carry ``category=Premise``). - - JUSTIFIED-VALID vs JUSTIFIED-INVALID are **asymmetric**: - * ``JUSTIFIED-VALID`` = author defense was audited and - accepted; the finding is dismissed and lives in - ``justified[]``, NOT ``verified[]``. Excluded defensively - in case a cached entry leaks into ``verified[]`` — its - concern was waived. - * ``JUSTIFIED-INVALID`` = author defense was audited and - REFUSED; the original concern survived. It belongs in the - count the same way a plain CONFIRMED MEDIUM does. The - reserved-but-unemitted enum value also lands here if a - future code path produces one in ``verified[]``. - - Severity is read post-DOWNGRADE — ``_merge_verifier_fields`` - rewrites ``severity`` from ``verifier_severity`` for valid - downgrades, so a DOWNGRADE from HIGH → MEDIUM correctly counts. - """ - count = 0 - for finding in verified: - if str(finding.get("category", "")) != "Premise": - continue - if str(finding.get("severity", "")) != "MEDIUM": - continue - if finding.get("verifier_verdict") == "JUSTIFIED-VALID": - continue - count += 1 - return count - - def _compute_canonical_verdict( verified: list[dict[str, Any]], coverage_gaps: list[dict[str, Any]], @@ -9548,19 +9475,15 @@ def _compute_canonical_verdict( Returns (canonical_verdict, reason). PLN-722 added two rules: the ``force_human_review`` short-circuit (rule 2.5 — mandatory_human_review_ paths) and the TENTATIVE → NEEDS_ATTENTION fall-through (rule 3.5). - PLN-721 fills in Rule 4: cumulative Premise MEDIUM gate. PLN-726 - (FEA-1401) fills in Rule 6: cumulative Impact gate (≥2 BLOCKING/HIGH - ImpactAnalysis findings → NEEDS_ATTENTION). + PLN-726 (FEA-1401) fills in Rule 6: cumulative Impact gate (≥2 + BLOCKING/HIGH ImpactAnalysis findings → NEEDS_ATTENTION). - ``thresholds`` (PLN-721): optional dict from ``_load_verdict_thresholds``; + ``thresholds``: optional dict from ``_load_verdict_thresholds``; callers that do not pass it get the built-in defaults - (``premise_cumulative_medium`` = 3, ``impact_cumulative`` = 2; see - ``_VERDICT_PREMISE_MEDIUM_THRESHOLD_DEFAULT`` and - ``_VERDICT_IMPACT_THRESHOLD_DEFAULT``) so existing test fixtures - and back-compat callers keep working. + (``impact_cumulative`` = 2; see ``_VERDICT_IMPACT_THRESHOLD_DEFAULT``) + so existing test fixtures and back-compat callers keep working. """ thresholds = thresholds or { - "premise_cumulative_medium": _VERDICT_PREMISE_MEDIUM_THRESHOLD_DEFAULT, "impact_cumulative": _VERDICT_IMPACT_THRESHOLD_DEFAULT, } @@ -9580,12 +9503,9 @@ def _short(text: str) -> str: # APPROVED. all_findings = verified + coverage_gaps - # Rule 2: BLOCKING (any scope) or Premise P0 → CHANGES_REQUESTED. + # Rule 2: BLOCKING (any scope) → CHANGES_REQUESTED. for finding in all_findings: - sev = str(finding.get("severity", "")) - if sev == "BLOCKING": - return "CHANGES_REQUESTED", _short(str(finding.get("issue", ""))) - if str(finding.get("category", "")) == "Premise" and finding.get("priority") == 0: + if str(finding.get("severity", "")) == "BLOCKING": return "CHANGES_REQUESTED", _short(str(finding.get("issue", ""))) # Rule 2.5 (PLN-722): mandatory_human_review_paths force NEEDS_ATTENTION. @@ -9614,23 +9534,6 @@ def _short(text: str) -> str: f"verifier uncertain: {finding.get('issue', '')}", ) - # Rule 4 (PLN-721): cumulative Premise MEDIUM gate. The counting - # policy is documented on ``_count_gateable_premise_medium`` — this - # site MUST use that helper so the value the gate fires on matches - # the value telemetry reports in ``premise_cumulative_medium_count``. - premise_medium_threshold = int( - thresholds.get( - "premise_cumulative_medium", - _VERDICT_PREMISE_MEDIUM_THRESHOLD_DEFAULT, - ), - ) - premise_medium_count = _count_gateable_premise_medium(verified) - if premise_medium_count >= premise_medium_threshold: - return "NEEDS_ATTENTION", _short( - f"{premise_medium_count} MEDIUM Premise findings " - f"(threshold {premise_medium_threshold})", - ) - # Rule 6 (FEA-1401 / PLN-726 OQ#6): cumulative Impact gate. ≥2 # BLOCKING/HIGH ImpactAnalysis findings → NEEDS_ATTENTION. Under # current precedence this is unreachable: Rule 2 (any BLOCKING → @@ -12499,69 +12402,6 @@ def _empty_coverage_plan() -> dict[str, Any]: } -_PLN773_PREMISE_SUBCATEGORIES: tuple[str, ...] = ( - "necessity", "cohesion", "workaround", "complexity", -) - - -def _justification_stats( - verified: list[dict[str, Any]], - justified: list[dict[str, Any]], - *, - rate_alert_threshold: float, -) -> dict[str, Any]: - """PLN-773 Phase 2 — Premise justification telemetry sub-block. - - The denominator is total Premise findings across ``verified[]`` AND - ``justified[]`` (the JUSTIFIED-VALID bucket lives in ``justified[]`` - after cmd_verify_consolidate routes; JUSTIFIED-INVALID stays in - ``verified[]``). NaN-safe: empty inputs return zeros, not divisions. - """ - valid_count = sum( - 1 for f in justified - if str(f.get("category", "")) == "Premise" - ) - invalid_count = sum( - 1 for f in verified - if str(f.get("category", "")) == "Premise" - and f.get("verifier_verdict") == "JUSTIFIED-INVALID" - ) - premise_in_verified = sum( - 1 for f in verified if str(f.get("category", "")) == "Premise" - ) - total_premise = premise_in_verified + valid_count - emitted = valid_count + invalid_count - rate = (emitted / total_premise) if total_premise > 0 else 0.0 - rejection_rate = (invalid_count / emitted) if emitted > 0 else 0.0 - return { - "rate": rate, - "rejection_rate": rejection_rate, - "total_premise": total_premise, - "justified_emitted": emitted, - "justified_valid": valid_count, - "justified_invalid": invalid_count, - "threshold_alert": rate > rate_alert_threshold, - } - - -def _by_subcategory_stats(verified: list[dict[str, Any]]) -> dict[str, int]: - """PLN-773 Phase 2 — Premise findings partitioned by subcategory. - - Counts only ``category=Premise`` findings in ``verified[]``. Subcategories - are pinned to the canonical four (PLN-721) so a typo in a reviewer - output doesn't introduce spurious buckets; non-canonical subcategories - are silently ignored. - """ - counts: dict[str, int] = {k: 0 for k in _PLN773_PREMISE_SUBCATEGORIES} - for f in verified: - if str(f.get("category", "")) != "Premise": - continue - sub = str(f.get("subcategory", "")) - if sub in counts: - counts[sub] += 1 - return counts - - def _verification_by_reviewer( verified: list[dict[str, Any]], rejected: list[dict[str, Any]], @@ -12624,15 +12464,11 @@ def _stats_from_findings( ) -> dict[str, Any]: """Compute the ``stats`` block of the result envelope. - ``thresholds`` (PLN-773): optional dict from ``_load_verdict_thresholds``. - Callers that omit it get the built-in default for the - ``justification_rate_alert`` toggle (0.30). All existing call sites - keep working through the optional kwarg. + ``thresholds`` is accepted for signature stability with the finalize + caller but is not currently read — the only threshold-driven stat was + the retired Premise justification telemetry. """ - thresholds = thresholds or { - "premise_cumulative_medium": _VERDICT_PREMISE_MEDIUM_THRESHOLD_DEFAULT, - "justification_rate_alert": _VERDICT_JUSTIFICATION_RATE_ALERT_DEFAULT, - } + del thresholds # reserved; see docstring by_severity: dict[str, int] = {"BLOCKING": 0, "HIGH": 0, "MEDIUM": 0} by_category: dict[str, int] = {} @@ -12669,18 +12505,6 @@ def _bump(finding: dict[str, Any], bucket: str) -> None: "by_category": by_category, "by_reviewer": by_reviewer, "by_finding_scope": by_scope, - # PLN-773 Phase 2 — Premise telemetry sub-blocks (additive; the - # envelope schema accepts arbitrary stats keys). - "by_subcategory": _by_subcategory_stats(verified), - "justification": _justification_stats( - verified, justified, - rate_alert_threshold=float( - thresholds.get( - "justification_rate_alert", - _VERDICT_JUSTIFICATION_RATE_ALERT_DEFAULT, - ), - ), - ), "verification": { "verified_count": len(verified), "rejected_count": len(rejected), @@ -12700,15 +12524,11 @@ def _bump(finding: dict[str, Any], bucket: str) -> None: # PLN-773 Phase 2 — per-reviewer FP rate + override counter. "by_reviewer": _verification_by_reviewer(verified, rejected), }, - # Must match the count Rule 4 actually fires on — - # _count_gateable_premise_medium is the single source of truth - # for that policy (excludes JUSTIFIED-VALID / JUSTIFIED-INVALID). - "premise_cumulative_medium_count": _count_gateable_premise_medium(verified), - # FEA-1401 Rule 6: Impact Analyzer gate count. Mirrors - # premise_cumulative_medium_count — single source of truth is - # _count_gateable_impact. Present-local SKILL.md's Verifier Stats - # block reads `stats.impact_cumulative_count` directly from this - # dict; without this key the footer line renders as None. + # FEA-1401 Rule 6: Impact Analyzer gate count. Single source of + # truth is _count_gateable_impact. Present-local SKILL.md's + # Verifier Stats block reads `stats.impact_cumulative_count` + # directly from this dict; without this key the footer line + # renders as None. "impact_cumulative_count": _count_gateable_impact(verified), "agent_failures": [], } diff --git a/plugins/code-review/tools/python/code_review_schema.py b/plugins/code-review/tools/python/code_review_schema.py index 333a624c..fbedc94e 100644 --- a/plugins/code-review/tools/python/code_review_schema.py +++ b/plugins/code-review/tools/python/code_review_schema.py @@ -28,9 +28,12 @@ # Version # --------------------------------------------------------------------------- -SCHEMA_VERSION = 1 +SCHEMA_VERSION = 2 """Integer schema version for Finding + ResultEnvelope. Bumped on breaking -changes per section 12 of the foundation plan.""" +changes per section 12 of the foundation plan. v2 retired the ``Premise`` +finding category (and its verdict gates, telemetry sub-blocks, and verifier +extra-strictness) when the premise reviewer was removed — no producer emits +``category == "Premise"`` anymore, so it is no longer a valid category.""" # --------------------------------------------------------------------------- @@ -65,7 +68,6 @@ "Documentation", "Hygiene", "Repo Hygiene", - "Premise", "ImpactAnalysis", "TestQuality", "Coverage", diff --git a/plugins/code-review/tools/python/fixtures/golden_all_categories/expected/review_result.json b/plugins/code-review/tools/python/fixtures/golden_all_categories/expected/review_result.json index 37ee194a..d5873aec 100644 --- a/plugins/code-review/tools/python/fixtures/golden_all_categories/expected/review_result.json +++ b/plugins/code-review/tools/python/fixtures/golden_all_categories/expected/review_result.json @@ -24,7 +24,7 @@ "rejected": [], "review_branch": "feature/x", "review_id": "00000000-0000-4000-8000-000000000000", - "schema_version": 1, + "schema_version": 2, "stats": { "agent_failures": [], "by_category": { @@ -51,23 +51,7 @@ "HIGH": 2, "MEDIUM": 2 }, - "by_subcategory": { - "cohesion": 0, - "complexity": 0, - "necessity": 0, - "workaround": 0 - }, "impact_cumulative_count": 0, - "justification": { - "justified_emitted": 0, - "justified_invalid": 0, - "justified_valid": 0, - "rate": 0.0, - "rejection_rate": 0.0, - "threshold_alert": false, - "total_premise": 0 - }, - "premise_cumulative_medium_count": 0, "verification": { "by_reviewer": { "bha_p0": { diff --git a/plugins/code-review/tools/python/fixtures/golden_injection_quarantine/expected/review_result.json b/plugins/code-review/tools/python/fixtures/golden_injection_quarantine/expected/review_result.json index db1b897c..6761a5ef 100644 --- a/plugins/code-review/tools/python/fixtures/golden_injection_quarantine/expected/review_result.json +++ b/plugins/code-review/tools/python/fixtures/golden_injection_quarantine/expected/review_result.json @@ -24,7 +24,7 @@ "rejected": [], "review_branch": "feature/x", "review_id": "00000000-0000-4000-8000-000000000000", - "schema_version": 1, + "schema_version": 2, "stats": { "agent_failures": [], "by_category": { @@ -48,23 +48,7 @@ "HIGH": 0, "MEDIUM": 0 }, - "by_subcategory": { - "cohesion": 0, - "complexity": 0, - "necessity": 0, - "workaround": 0 - }, "impact_cumulative_count": 0, - "justification": { - "justified_emitted": 0, - "justified_invalid": 0, - "justified_valid": 0, - "rate": 0.0, - "rejection_rate": 0.0, - "threshold_alert": false, - "total_premise": 0 - }, - "premise_cumulative_medium_count": 0, "verification": { "by_reviewer": { "injection-detector": { diff --git a/plugins/code-review/tools/python/fixtures/golden_minimal_correctness/expected/review_result.json b/plugins/code-review/tools/python/fixtures/golden_minimal_correctness/expected/review_result.json index dc322ef0..d5733f75 100644 --- a/plugins/code-review/tools/python/fixtures/golden_minimal_correctness/expected/review_result.json +++ b/plugins/code-review/tools/python/fixtures/golden_minimal_correctness/expected/review_result.json @@ -24,7 +24,7 @@ "rejected": [], "review_branch": "feature/x", "review_id": "00000000-0000-4000-8000-000000000000", - "schema_version": 1, + "schema_version": 2, "stats": { "agent_failures": [], "by_category": { @@ -48,23 +48,7 @@ "HIGH": 1, "MEDIUM": 0 }, - "by_subcategory": { - "cohesion": 0, - "complexity": 0, - "necessity": 0, - "workaround": 0 - }, "impact_cumulative_count": 0, - "justification": { - "justified_emitted": 0, - "justified_invalid": 0, - "justified_valid": 0, - "rate": 0.0, - "rejection_rate": 0.0, - "threshold_alert": false, - "total_premise": 0 - }, - "premise_cumulative_medium_count": 0, "verification": { "by_reviewer": { "bha_p0": { diff --git a/plugins/code-review/tools/python/fixtures/golden_premise_justified/README.md b/plugins/code-review/tools/python/fixtures/golden_premise_justified/README.md deleted file mode 100644 index 3c133dba..00000000 --- a/plugins/code-review/tools/python/fixtures/golden_premise_justified/README.md +++ /dev/null @@ -1,11 +0,0 @@ -# golden_premise_justified - -**Status:** deferred — pending PLN-721 (justification / plan 02). - -PLN-719 Section 10 listed this fixture but its expected envelope depends -on the `justification` field semantics shipped by plan 02. The fixture -directory is reserved here so the future PR just drops `config.yaml`, -`inputs/`, and `expected/` alongside the existing siblings. - -The test parametrization in `test_golden_fixtures.py` skips this fixture -via the `_DEFERRED_FIXTURES` map until inputs land. diff --git a/plugins/code-review/tools/python/fixtures/golden_premise_rejected/README.md b/plugins/code-review/tools/python/fixtures/golden_premise_rejected/README.md deleted file mode 100644 index 8563af70..00000000 --- a/plugins/code-review/tools/python/fixtures/golden_premise_rejected/README.md +++ /dev/null @@ -1,11 +0,0 @@ -# golden_premise_rejected - -**Status:** deferred — pending plan 02 (justification). - -PLN-719 Section 10 listed this fixture but its expected envelope depends -on outputs from plan 02 (justification), which has not shipped yet. The directory is -reserved so the future PR just drops `config.yaml`, `inputs/`, and -`expected/` alongside the existing siblings. - -The test parametrization in `test_golden_fixtures.py` skips this -fixture via the `_DEFERRED_FIXTURES` map until inputs land. diff --git a/plugins/code-review/tools/python/fixtures/golden_schema_v1_round_trip/expected/review_result.json b/plugins/code-review/tools/python/fixtures/golden_schema_v1_round_trip/expected/review_result.json index 8c3aa1d9..2ab9d8ba 100644 --- a/plugins/code-review/tools/python/fixtures/golden_schema_v1_round_trip/expected/review_result.json +++ b/plugins/code-review/tools/python/fixtures/golden_schema_v1_round_trip/expected/review_result.json @@ -24,7 +24,7 @@ "rejected": [], "review_branch": "feature/x", "review_id": "00000000-0000-4000-8000-000000000000", - "schema_version": 1, + "schema_version": 2, "stats": { "agent_failures": [], "by_category": { @@ -48,23 +48,7 @@ "HIGH": 1, "MEDIUM": 0 }, - "by_subcategory": { - "cohesion": 0, - "complexity": 0, - "necessity": 0, - "workaround": 0 - }, "impact_cumulative_count": 0, - "justification": { - "justified_emitted": 0, - "justified_invalid": 0, - "justified_valid": 0, - "rate": 0.0, - "rejection_rate": 0.0, - "threshold_alert": false, - "total_premise": 0 - }, - "premise_cumulative_medium_count": 0, "verification": { "by_reviewer": { "bha_p0": { 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 75979563..13fc0dbb 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 @@ -518,7 +518,7 @@ "--model-id", "", "--schema-version", - "1", + "2", "--output-dir", "/tmp/cr_dir", "--global-cache", @@ -770,7 +770,7 @@ "--model-id", "", "--schema-version", - "1", + "2", "--partitions-file", "/tmp/cr_dir/partitions.json", "--global-cache", 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 fed2fa81..752b3aaf 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 @@ -512,7 +512,7 @@ "--model-id", "", "--schema-version", - "1", + "2", "--output-dir", "/tmp/cr_dir", "--global-cache", @@ -762,7 +762,7 @@ "--model-id", "", "--schema-version", - "1", + "2", "--partitions-file", "/tmp/cr_dir/partitions.json", "--global-cache", 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 2c3bf1fa..73b679f5 100644 --- a/plugins/code-review/tools/python/test_code_review_helpers.py +++ b/plugins/code-review/tools/python/test_code_review_helpers.py @@ -2218,12 +2218,6 @@ def test_verdict_decline_blocking(self) -> None: assert result["verdict"] == "decline" assert "Missing null check" in result["reason"] - def test_verdict_decline_premise_p0(self) -> None: - result = self._run_verdict([ - {"severity": "HIGH", "issue": "[P0] Unnecessary change", "priority": 0, "category": "Premise"}, - ]) - assert result["verdict"] == "decline" - def test_verdict_needs_attention_high(self) -> None: result = self._run_verdict([ {"severity": "HIGH", "issue": "[P1] Race condition", "priority": 1, "category": "Correctness"}, @@ -7004,7 +6998,7 @@ def test_hygiene_emits_canonical_fields(self, tmp_path: Path) -> None: if result["findings"]: f0 = result["findings"][0] # Canonical schema fields - assert f0["schema_version"] == 1 + assert f0["schema_version"] == 2 assert f0["finding_scope"] == "diff" assert f0["system_marker"] is None assert f0["source"] == "hygiene" @@ -7028,9 +7022,9 @@ def test_collect_findings_assigns_deterministic_ids(self, tmp_path: Path) -> Non "issue": "y", "priority": 2, "confidence": 0.8}, ], })) - (tmp_path / "agent_premise.json").write_text(json.dumps({ + (tmp_path / "agent_auditor.json").write_text(json.dumps({ "findings": [ - {"file": "b.ts", "severity": "MEDIUM", "line": 1, "category": "Premise", + {"file": "b.ts", "severity": "MEDIUM", "line": 1, "category": "Code Quality", "issue": "z", "priority": 2, "confidence": 0.7}, ], })) @@ -7047,10 +7041,10 @@ def test_collect_findings_assigns_deterministic_ids(self, tmp_path: Path) -> Non ids = [f["id"] for f in merged] assert "bha_p0_f0" in ids assert "bha_p0_f1" in ids - assert "premise_f0" in ids + assert "auditor_f0" in ids # All findings must have schema_version + finding_scope for f in merged: - assert f["schema_version"] == 1 + assert f["schema_version"] == 2 assert f["finding_scope"] == "diff" def test_collect_findings_survives_bad_reviewer_string(self, tmp_path: Path) -> None: @@ -7099,9 +7093,9 @@ def test_collect_findings_preserves_existing_ids(self, tmp_path: Path) -> None: (tmp_path / "agent_bha_p0.json").write_text(json.dumps({ "findings": [ - {"id": "premise_f99", "file": "a.ts", "severity": "HIGH", "line": 1, + {"id": "custom_f99", "file": "a.ts", "severity": "HIGH", "line": 1, "category": "Correctness", "issue": "x", "priority": 1, "confidence": 0.9, - "reviewer": "premise"}, + "reviewer": "unified_auditor"}, ], })) @@ -7114,8 +7108,8 @@ def test_collect_findings_preserves_existing_ids(self, tmp_path: Path) -> None: _sys.stdout = old_stdout merged = json.loads((tmp_path / "findings.json").read_text()) - assert merged[0]["id"] == "premise_f99" - assert merged[0]["reviewer"] == "premise" + assert merged[0]["id"] == "custom_f99" + assert merged[0]["reviewer"] == "unified_auditor" def test_validate_passes_through_system_scoped_finding(self, tmp_path: Path) -> None: """A system-scoped finding bypasses file/line checks.""" @@ -7282,7 +7276,7 @@ def test_writes_envelope_with_no_findings(self, tmp_path: Path) -> None: result = self._run_finalize(tmp_path, []) assert result["verdict"] == "APPROVED" envelope = json.loads((tmp_path / "review_result.json").read_text()) - assert envelope["schema_version"] == 1 + assert envelope["schema_version"] == 2 assert envelope["verdict"] == "APPROVED" assert envelope["verified"] == [] assert envelope["coverage_gaps"] == [] @@ -7678,7 +7672,7 @@ def _run( def test_simple_fits_under_cap(self, tmp_path: Path) -> None: diff = _make_diff_data(files=["src/app.ts"]) plan_in = { - "required": [{"reviewer": "premise", "priority": 0}], + "required": [{"reviewer": "bug_hunter_a", "priority": 0}], "best_effort": [ {"reviewer": "test_quality", "priority": 1}, {"reviewer": "impact", "priority": 2}, @@ -7737,7 +7731,7 @@ def test_docs_only_waives_bha_floor(self, tmp_path: Path) -> None: def test_required_with_bha_floor(self, tmp_path: Path) -> None: diff = _make_diff_data(files=["src/app.ts"]) plan_in = { - "required": [{"reviewer": "premise", "priority": 0}], + "required": [{"reviewer": "bug_hunter_a", "priority": 0}], "best_effort": [], } summary, plan, gaps = self._run(tmp_path, plan_in, diff, cap=20) @@ -9199,19 +9193,15 @@ def test_evidence_array_preserved(self, tmp_path: Path) -> None: def test_reasoning_certificate_preserved(self, tmp_path: Path) -> None: cert = { - "kind": "necessity", + "kind": "impact", "fields": { - "authors_claim": "fixes the data-loss bug", - "counter_evidence": "auth.ts:47 already guards this", - "alternative_check": { - "searched_for": "callers of save()", - "found": "no callers pass null", - }, - "conclusion": "PREMISE REFUTED", + "changed_symbol": "save", + "external_callsites": ["api.ts:47"], + "break_kind": "signature", }, } finding = _make_validated_finding( - "premise_f0", category="Premise", reasoning_certificate=cert, + "impact_f0", category="ImpactAnalysis", reasoning_certificate=cert, ) result = self._run([finding], tmp_path) assert result["validated"][0]["reasoning_certificate"] == cert @@ -9279,13 +9269,6 @@ def test_injection_detector_never_verified(self) -> None: "category": "InjectionAttempt", "source": "injection-detector"} assert _needs_verification(f) is False - def test_premise_always_verified(self) -> None: - # Even at MEDIUM with high confidence, Premise gets the strict - # adversarial re-check. - f = {"severity": "MEDIUM", "confidence": 0.95, - "category": "Premise", "source": "agent"} - assert _needs_verification(f) is True - def test_out_of_hunk_kept_always_verified_even_at_high_confidence( self, ) -> None: @@ -9379,10 +9362,6 @@ def test_tier_routing_splits_eligible_and_skipped( "injection_f0", source="injection-detector", category="InjectionAttempt", severity="BLOCKING", ), - _make_validated_finding( - "premise_f0", category="Premise", severity="MEDIUM", - confidence=0.99, - ), _make_validated_finding("low_f0", severity="LOW"), _make_validated_finding( "medium_hi_f0", severity="MEDIUM", confidence=0.95, @@ -9391,7 +9370,7 @@ def test_tier_routing_splits_eligible_and_skipped( _, manifest = _run_verify_prepare(tmp_path, findings) to_verify_ids = {e["finding_id"] for e in manifest["to_verify"]} skipped = set(manifest["skipped_no_verification"]) - assert to_verify_ids == {"bha_p0_f0", "bhb_f0", "premise_f0"} + assert to_verify_ids == {"bha_p0_f0", "bhb_f0"} assert skipped == {"hygiene_f0", "injection_f0", "low_f0", "medium_hi_f0"} def test_max_verifications_cap_keeps_highest_priority( @@ -9725,33 +9704,33 @@ def test_consolidate_writes_justification_invalid_event( self, tmp_path: Path, ) -> None: """End-to-end: JUSTIFIED-INVALID through cmd_verify_consolidate - appends one event to premise-justifications.jsonl (the autouse - fixture redirects the base dir to tmp_path).""" + appends one event to the justification-audit learning stream (the + autouse fixture redirects the base dir to tmp_path).""" from code_review_helpers import ( _PENDING_LEARNINGS_DIR, _PENDING_LEARNINGS_PREMISE, ) - finding = _make_validated_finding("premise_f0", category="Premise") - finding["subcategory"] = "cohesion" + finding = _make_validated_finding("cq_f0", category="Code Quality") + finding["subcategory"] = "duplication" finding["justification"] = { "text": "// intentional", "source": "code_comment:src/x.py:1", "addresses_specific_concern": True, - "claimed_by_reviewer": "premise", + "claimed_by_reviewer": "unified_auditor", } verifier_output = { - "finding_id": "premise_f0", + "finding_id": "cq_f0", "verifier_verdict": "JUSTIFIED-INVALID", "verifier_reasoning": "generic disclaimer; does not address concern", } rc, _ = _run_verify_consolidate( tmp_path, [finding], manifest={ - "to_verify": [{"finding_id": "premise_f0", "model": "sonnet"}], + "to_verify": [{"finding_id": "cq_f0", "model": "sonnet"}], "skipped_no_verification": [], "deferred_budget": [], "cache_hits": [], }, - verifier_outputs={"premise_f0": verifier_output}, + verifier_outputs={"cq_f0": verifier_output}, ) assert rc == 0 jsonl = _PENDING_LEARNINGS_DIR / _PENDING_LEARNINGS_PREMISE @@ -9759,8 +9738,8 @@ def test_consolidate_writes_justification_invalid_event( lines = jsonl.read_text().splitlines() assert len(lines) == 1 event = json.loads(lines[0]) - assert event["finding_id"] == "premise_f0" - assert event["subcategory"] == "cohesion" + assert event["finding_id"] == "cq_f0" + assert event["subcategory"] == "duplication" assert event["justification_text"] == "// intentional" @@ -10400,13 +10379,13 @@ def test_justified_valid_routes_to_justified_bucket( """PLN-721: JUSTIFIED-VALID verdicts land in the new justified[] bucket, NOT verified[] or rejected[]. They are the author's defense holding up under independent audit.""" - findings = [_make_validated_finding("premise_f0", severity="MEDIUM")] + findings = [_make_validated_finding("med_f0", severity="MEDIUM")] manifest = { - "to_verify": [{"finding_id": "premise_f0", "model": "sonnet"}], + "to_verify": [{"finding_id": "med_f0", "model": "sonnet"}], "skipped_no_verification": [], "deferred_budget": [], "cache_hits": [], } verdicts = { - "premise_f0": { + "med_f0": { "verifier_verdict": "JUSTIFIED-VALID", "verifier_confidence": 0.9, "verifier_reasoning": "justification addresses the concern", @@ -10429,13 +10408,13 @@ def test_justified_invalid_routes_to_verified_bucket( """PLN-721: JUSTIFIED-INVALID verdicts land in verified[] — the justification audit failed, so the original concern stands and downstream verdict rules treat it like any other verified MEDIUM.""" - findings = [_make_validated_finding("premise_f0", severity="MEDIUM")] + findings = [_make_validated_finding("med_f0", severity="MEDIUM")] manifest = { - "to_verify": [{"finding_id": "premise_f0", "model": "sonnet"}], + "to_verify": [{"finding_id": "med_f0", "model": "sonnet"}], "skipped_no_verification": [], "deferred_budget": [], "cache_hits": [], } verdicts = { - "premise_f0": { + "med_f0": { "verifier_verdict": "JUSTIFIED-INVALID", "verifier_confidence": 0.85, "verifier_reasoning": "justification is generic, does not address concern", @@ -10460,15 +10439,15 @@ def test_tentative_on_paths_lifts_justified_valid( so Rule 3.5 fires and the verdict becomes NEEDS_ATTENTION.""" findings = [ _make_validated_finding( - "premise_f0", severity="MEDIUM", file="lib/auth/handler.ts", + "med_f0", severity="MEDIUM", file="lib/auth/handler.ts", ), ] manifest = { - "to_verify": [{"finding_id": "premise_f0", "model": "sonnet"}], + "to_verify": [{"finding_id": "med_f0", "model": "sonnet"}], "skipped_no_verification": [], "deferred_budget": [], "cache_hits": [], } verdicts = { - "premise_f0": { + "med_f0": { "verifier_verdict": "JUSTIFIED-VALID", "verifier_confidence": 0.9, "verifier_reasoning": "looks fine", @@ -10616,57 +10595,51 @@ def test_confirmed_medium_alone_is_approved(self) -> None: class TestLoadVerdictThresholds: - """PLN-721: operator-overridable verdict thresholds.""" + """Operator-overridable verdict thresholds. After the Premise gate was + retired, ``impact_cumulative`` (FEA-1401 Rule 6) is the sole tunable.""" def test_none_path_returns_default(self) -> None: from code_review_helpers import _load_verdict_thresholds out = _load_verdict_thresholds(None) - # PLN-773 added justification_rate_alert (default 0.30) alongside - # the original premise_cumulative_medium (default 3). FEA-1401 - # added impact_cumulative (default 2) — Rule 6's gate threshold. - assert out == { - "premise_cumulative_medium": 3, - "justification_rate_alert": 0.30, - "impact_cumulative": 2, - } + assert out == {"impact_cumulative": 2} def test_missing_file_returns_default(self, tmp_path: Path) -> None: from code_review_helpers import _load_verdict_thresholds out = _load_verdict_thresholds(tmp_path / "missing.json") - assert out["premise_cumulative_medium"] == 3 + assert out["impact_cumulative"] == 2 def test_valid_override(self, tmp_path: Path) -> None: from code_review_helpers import _load_verdict_thresholds p = tmp_path / "verdict-thresholds.json" - p.write_text(json.dumps({"premise_cumulative_medium": 5})) + p.write_text(json.dumps({"impact_cumulative": 5})) out = _load_verdict_thresholds(p) - assert out["premise_cumulative_medium"] == 5 + assert out["impact_cumulative"] == 5 def test_malformed_json_falls_back_to_default(self, tmp_path: Path) -> None: from code_review_helpers import _load_verdict_thresholds p = tmp_path / "verdict-thresholds.json" p.write_text("not json {") out = _load_verdict_thresholds(p) - assert out["premise_cumulative_medium"] == 3 + assert out["impact_cumulative"] == 2 def test_non_int_value_falls_back_to_default(self, tmp_path: Path) -> None: from code_review_helpers import _load_verdict_thresholds p = tmp_path / "verdict-thresholds.json" - p.write_text(json.dumps({"premise_cumulative_medium": "three"})) + p.write_text(json.dumps({"impact_cumulative": "two"})) out = _load_verdict_thresholds(p) - assert out["premise_cumulative_medium"] == 3 + assert out["impact_cumulative"] == 2 def test_zero_or_negative_falls_back_to_default(self, tmp_path: Path) -> None: """A 0 or negative threshold would silently disable the gate. The operator must use a very large number (e.g. 9999) to disable, not 0/-1 — the loader rejects values < 1 and falls back to the default - so a typo doesn't silently switch off Rule 4.""" + so a typo doesn't silently switch off Rule 6.""" from code_review_helpers import _load_verdict_thresholds p = tmp_path / "verdict-thresholds.json" - p.write_text(json.dumps({"premise_cumulative_medium": 0})) - assert _load_verdict_thresholds(p)["premise_cumulative_medium"] == 3 - p.write_text(json.dumps({"premise_cumulative_medium": -1})) - assert _load_verdict_thresholds(p)["premise_cumulative_medium"] == 3 + p.write_text(json.dumps({"impact_cumulative": 0})) + assert _load_verdict_thresholds(p)["impact_cumulative"] == 2 + p.write_text(json.dumps({"impact_cumulative": -1})) + assert _load_verdict_thresholds(p)["impact_cumulative"] == 2 def test_bool_rejected_as_threshold(self, tmp_path: Path) -> None: """Python's `True` is `int(True) == 1` — the loader must explicitly @@ -10674,343 +10647,9 @@ def test_bool_rejected_as_threshold(self, tmp_path: Path) -> None: """ from code_review_helpers import _load_verdict_thresholds p = tmp_path / "verdict-thresholds.json" - p.write_text(json.dumps({"premise_cumulative_medium": True})) - out = _load_verdict_thresholds(p) - assert out["premise_cumulative_medium"] == 3 - - -class TestPremiseTelemetryStats: - """PLN-773 Phase 2 — Premise justification + by_subcategory telemetry.""" - - @staticmethod - def _premise(severity: str = "MEDIUM", subcategory: str = "cohesion", - verdict: str | None = "CONFIRMED") -> dict[str, Any]: - return { - "category": "Premise", - "subcategory": subcategory, - "severity": severity, - "verifier_verdict": verdict, - "issue": "premise", - "reviewer": "premise", - } - - def test_justification_stats_empty_inputs_nan_safe(self) -> None: - from code_review_helpers import _justification_stats - out = _justification_stats([], [], rate_alert_threshold=0.30) - assert out["rate"] == 0.0 - assert out["rejection_rate"] == 0.0 - assert out["total_premise"] == 0 - assert out["threshold_alert"] is False - - def test_justification_stats_no_justified_findings(self) -> None: - from code_review_helpers import _justification_stats - # 3 Premise CONFIRMED, no justified — rate is 0 - verified = [self._premise() for _ in range(3)] - out = _justification_stats(verified, [], rate_alert_threshold=0.30) - assert out["total_premise"] == 3 - assert out["justified_emitted"] == 0 - assert out["rate"] == 0.0 - assert out["rejection_rate"] == 0.0 - assert out["threshold_alert"] is False - - def test_justification_rate_crosses_threshold(self) -> None: - from code_review_helpers import _justification_stats - # 2 Premise CONFIRMED in verified, 1 Premise JUSTIFIED-VALID in justified - # → rate = 1/3 = 0.33 > 0.30 → alert fires - verified = [self._premise(), self._premise()] - justified = [self._premise(verdict="JUSTIFIED-VALID")] - out = _justification_stats( - verified, justified, rate_alert_threshold=0.30, - ) - assert out["total_premise"] == 3 - assert out["justified_emitted"] == 1 - assert out["justified_valid"] == 1 - assert out["justified_invalid"] == 0 - assert out["rate"] == pytest.approx(1 / 3) - assert out["threshold_alert"] is True - - def test_justified_invalid_in_verified_counts_for_emitted(self) -> None: - from code_review_helpers import _justification_stats - verified = [ - self._premise(), # CONFIRMED - self._premise(verdict="JUSTIFIED-INVALID"), - ] - justified = [self._premise(verdict="JUSTIFIED-VALID")] - out = _justification_stats( - verified, justified, rate_alert_threshold=0.30, - ) - # total_premise = 2 (verified) + 1 (justified) = 3 - # emitted = 1 (invalid) + 1 (valid) = 2 - # rejection_rate = 1 / 2 = 0.5 - assert out["total_premise"] == 3 - assert out["justified_emitted"] == 2 - assert out["rejection_rate"] == 0.5 - - def test_by_subcategory_partitions_only_premise(self) -> None: - from code_review_helpers import _by_subcategory_stats - verified = [ - self._premise(subcategory="necessity"), - self._premise(subcategory="cohesion"), - self._premise(subcategory="cohesion"), - # Non-Premise — must not appear in any bucket - {"category": "Correctness", "severity": "HIGH", - "subcategory": "cohesion", "issue": "x"}, - ] - out = _by_subcategory_stats(verified) - assert out == { - "necessity": 1, "cohesion": 2, "workaround": 0, "complexity": 0, - } - - def test_by_subcategory_drops_non_canonical_keys(self) -> None: - """A reviewer typo (e.g. 'duplicaiton') does NOT create a new bucket.""" - from code_review_helpers import _by_subcategory_stats - verified = [ - self._premise(subcategory="cohesion"), - self._premise(subcategory="duplicaiton"), # typo - ] - out = _by_subcategory_stats(verified) - assert "duplicaiton" not in out - assert out["cohesion"] == 1 - - def test_verification_by_reviewer_fp_rate(self) -> None: - from code_review_helpers import _verification_by_reviewer - verified = [ - {"reviewer": "bug_hunter_a", "verifier_verdict": "CONFIRMED"}, - {"reviewer": "bug_hunter_a", "verifier_verdict": "CONFIRMED"}, - {"reviewer": "premise", "verifier_verdict": "CONFIRMED"}, - ] - rejected = [ - {"reviewer": "bug_hunter_a", "verifier_verdict": "REJECTED"}, - ] - out = _verification_by_reviewer(verified, rejected) - # bug_hunter_a: 2 verified + 1 rejected → 1/3 FP rate - assert out["bug_hunter_a"]["verified"] == 2 - assert out["bug_hunter_a"]["rejected"] == 1 - assert out["bug_hunter_a"]["fp_rate"] == pytest.approx(1 / 3) - # premise: only verified → 0.0 FP rate (NaN-safe) - assert out["premise"]["fp_rate"] == 0.0 - - def test_verification_by_reviewer_counts_re_asserted(self) -> None: - from code_review_helpers import _verification_by_reviewer - verified = [ - {"reviewer": "premise", "verifier_verdict": "RE_ASSERTED"}, - {"reviewer": "premise", "verifier_verdict": "CONFIRMED"}, - ] - out = _verification_by_reviewer(verified, []) - assert out["premise"]["re_asserted"] == 1 - assert out["premise"]["verified"] == 2 # both still in verified[] - - def test_stats_block_includes_pln773_sub_blocks(self) -> None: - """End-to-end: _stats_from_findings produces all PLN-773 keys.""" - from code_review_helpers import _stats_from_findings - verified = [self._premise(subcategory="necessity")] - justified = [self._premise( - subcategory="cohesion", verdict="JUSTIFIED-VALID", - )] - stats = _stats_from_findings(verified, [], justified, []) - assert "by_subcategory" in stats - assert "justification" in stats - assert "by_reviewer" in stats["verification"] - - -class TestLoadVerdictThresholdsJustificationRate: - """PLN-773: justification_rate_alert key in verdict-thresholds.json.""" - - def test_default_is_point_three(self) -> None: - from code_review_helpers import _load_verdict_thresholds - out = _load_verdict_thresholds(None) - assert out["justification_rate_alert"] == 0.30 - - def test_valid_float_override(self, tmp_path: Path) -> None: - from code_review_helpers import _load_verdict_thresholds - p = tmp_path / "vt.json" - p.write_text(json.dumps({"justification_rate_alert": 0.5})) + p.write_text(json.dumps({"impact_cumulative": True})) out = _load_verdict_thresholds(p) - assert out["justification_rate_alert"] == 0.5 - - def test_out_of_range_falls_back_to_default(self, tmp_path: Path) -> None: - """1.5 is outside [0.0, 1.0] — fall back to default.""" - from code_review_helpers import _load_verdict_thresholds - p = tmp_path / "vt.json" - p.write_text(json.dumps({"justification_rate_alert": 1.5})) - out = _load_verdict_thresholds(p) - assert out["justification_rate_alert"] == 0.30 - - def test_negative_falls_back_to_default(self, tmp_path: Path) -> None: - from code_review_helpers import _load_verdict_thresholds - p = tmp_path / "vt.json" - p.write_text(json.dumps({"justification_rate_alert": -0.1})) - out = _load_verdict_thresholds(p) - assert out["justification_rate_alert"] == 0.30 - - def test_bool_rejected(self, tmp_path: Path) -> None: - """A bool sneaks through int isinstance() — explicit reject.""" - from code_review_helpers import _load_verdict_thresholds - p = tmp_path / "vt.json" - p.write_text(json.dumps({"justification_rate_alert": True})) - out = _load_verdict_thresholds(p) - assert out["justification_rate_alert"] == 0.30 - - -class TestCumulativePremiseMediumGate: - """PLN-721 Rule 4: cumulative Premise MEDIUM gate.""" - - @staticmethod - def _premise_med(verifier_verdict: str | None = "CONFIRMED") -> dict[str, Any]: - return { - "category": "Premise", - "severity": "MEDIUM", - "verifier_verdict": verifier_verdict, - "issue": "premise med", - } - - def test_two_medium_premise_approved(self) -> None: - v, _ = _compute_canonical_verdict( - [self._premise_med(), self._premise_med()], [], - ) - assert v == "APPROVED" - - def test_three_medium_premise_triggers_needs_attention(self) -> None: - v, r = _compute_canonical_verdict( - [self._premise_med(), self._premise_med(), self._premise_med()], [], - ) - assert v == "NEEDS_ATTENTION" - assert "3 MEDIUM Premise" in r - assert "threshold 3" in r - - def test_four_medium_premise_still_needs_attention(self) -> None: - v, _ = _compute_canonical_verdict( - [self._premise_med()] * 4, [], - ) - assert v == "NEEDS_ATTENTION" - - def test_custom_threshold_raises_bar(self) -> None: - # Operator override: premise_cumulative_medium = 5 ⇒ 3 is no longer enough - v, _ = _compute_canonical_verdict( - [self._premise_med()] * 3, [], - thresholds={"premise_cumulative_medium": 5}, - ) - assert v == "APPROVED" - # but 5 fires the gate - v, _ = _compute_canonical_verdict( - [self._premise_med()] * 5, [], - thresholds={"premise_cumulative_medium": 5}, - ) - assert v == "NEEDS_ATTENTION" - - def test_non_premise_medium_does_not_count(self) -> None: - # A pile of MEDIUM CodeQuality findings doesn't trigger Rule 4. - v, _ = _compute_canonical_verdict( - [{"category": "Code Quality", "severity": "MEDIUM", - "verifier_verdict": "CONFIRMED", "issue": "dry"}] * 5, - [], - ) - assert v == "APPROVED" - - def test_high_blocking_premise_does_not_count_toward_rule_4(self) -> None: - # Rule 3 (HIGH) short-circuits before Rule 4 ever runs. - v, _ = _compute_canonical_verdict( - [{"category": "Premise", "severity": "HIGH", - "verifier_verdict": "CONFIRMED", "issue": "high prem"}, - self._premise_med(), self._premise_med()], - [], - ) - assert v == "NEEDS_ATTENTION" # caused by HIGH, not the cumulative gate - - def test_justified_valid_excluded_from_count(self) -> None: - """Defensive: if a JUSTIFIED-VALID finding leaks into verified[] - (it shouldn't — cmd_verify_consolidate routes it to justified[]), - the gate must still ignore it.""" - v, _ = _compute_canonical_verdict( - [self._premise_med(), - self._premise_med(), - self._premise_med(verifier_verdict="JUSTIFIED-VALID")], - [], - ) - assert v == "APPROVED" - - def test_justified_invalid_counts_concern_survived(self) -> None: - """PR #113 review (thadeusb): JUSTIFIED-INVALID is the verifier - REFUSING the author's defense — the original concern survives, so - it must count toward the cumulative gate the same way a plain - CONFIRMED MEDIUM does. Excluding it (v2.9.0/v2.9.1 behavior) was - backwards: the author's failed wave-off shouldn't be the thing - that prevents the gate from firing.""" - v, r = _compute_canonical_verdict( - [self._premise_med(), - self._premise_med(), - self._premise_med(verifier_verdict="JUSTIFIED-INVALID")], - [], - ) - assert v == "NEEDS_ATTENTION" - assert "3 MEDIUM Premise" in r - - def test_valid_vs_invalid_are_asymmetric(self) -> None: - """Pin the asymmetry directly: same shape, only the JUSTIFIED-* - verdict differs, opposite gate outcomes.""" - from code_review_helpers import _count_gateable_premise_medium - with_valid = [self._premise_med()] * 2 + [ - self._premise_med(verifier_verdict="JUSTIFIED-VALID") - ] - with_invalid = [self._premise_med()] * 2 + [ - self._premise_med(verifier_verdict="JUSTIFIED-INVALID") - ] - assert _count_gateable_premise_medium(with_valid) == 2 - assert _count_gateable_premise_medium(with_invalid) == 3 - - def test_downgrade_to_medium_counts(self) -> None: - """A DOWNGRADE from HIGH → MEDIUM (severity already rewritten by - _merge_verifier_fields) counts toward Rule 4.""" - v, _ = _compute_canonical_verdict( - [self._premise_med(verifier_verdict="DOWNGRADE")] * 3, [], - ) - assert v == "NEEDS_ATTENTION" - - def test_tentative_rule_35_wins_over_rule_4_counting(self) -> None: - """If any Premise finding is TENTATIVE, Rule 3.5 short-circuits - first and Rule 4 never runs. The verdict is still NEEDS_ATTENTION - but for the verifier-uncertainty reason.""" - v, r = _compute_canonical_verdict( - [self._premise_med(verifier_verdict="TENTATIVE"), - self._premise_med(), self._premise_med()], - [], - ) - assert v == "NEEDS_ATTENTION" - assert "uncertain" in r.lower() - - @pytest.mark.parametrize( - "verdicts", - [ - ["CONFIRMED", "CONFIRMED", "CONFIRMED"], - ["CONFIRMED", "CONFIRMED", "JUSTIFIED-VALID"], - ["CONFIRMED", "JUSTIFIED-INVALID", "DOWNGRADE"], - ["DOWNGRADE", "DOWNGRADE", "DOWNGRADE", "CONFIRMED"], - ["JUSTIFIED-VALID", "JUSTIFIED-INVALID", "JUSTIFIED-VALID"], - ], - ) - def test_telemetry_count_matches_rule_4_count( - self, verdicts: list[str], - ) -> None: - """PLN-721 v2.9.1: the count Rule 4 fires on MUST match the value - telemetry surfaces as `premise_cumulative_medium_count`. The v2.9.0 - review caught these counts diverging because JUSTIFIED-* findings - were excluded from the gate but not the telemetry. Both sites now - delegate to `_count_gateable_premise_medium`; this test pins that - they stay aligned across the JUSTIFIED-VALID / JUSTIFIED-INVALID / - DOWNGRADE shapes that triggered the divergence. - """ - from code_review_helpers import ( - _count_gateable_premise_medium, - _stats_from_findings, - ) - verified = [self._premise_med(verifier_verdict=v) for v in verdicts] - gate_count = _count_gateable_premise_medium(verified) - stats = _stats_from_findings(verified, [], [], []) - assert stats["premise_cumulative_medium_count"] == gate_count, ( - f"telemetry/gate divergence for verdicts {verdicts}: " - f"stat={stats['premise_cumulative_medium_count']}, " - f"gate={gate_count}" - ) + assert out["impact_cumulative"] == 2 class TestFinalizeResultPrefersVerified: @@ -11113,7 +10752,7 @@ def test_justified_bucket_flows_to_envelope(self, tmp_path: Path) -> None: The verdict stays APPROVED — JUSTIFIED-VALID findings do not trigger any of the precedence rules.""" validated = [ - _make_validated_finding("premise_f0", severity="MEDIUM"), + _make_validated_finding("med_f0", severity="MEDIUM"), ] verified_doc = { "verified": [], @@ -11292,8 +10931,8 @@ def test_already_dismissed_when_finding_is_justified( envelope = { "verified": [], "rejected": [], "pending_verification": [], "justified": [ - {"id": "premise_f0", "file": "src/x.py", "line": 5, - "category": "Premise", + {"id": "cq_f0", "file": "src/x.py", "line": 5, + "category": "Code Quality", "verifier_verdict": "JUSTIFIED-VALID"}, ], } @@ -11304,7 +10943,7 @@ def test_already_dismissed_when_finding_is_justified( ns = argparse.Namespace( cr_dir=str(cr), cache_dir=str(cache), - finding_ids="premise_f0", + finding_ids="cq_f0", prior_result=str(prior_path), reason="", asserted_by="ops", @@ -11320,11 +10959,11 @@ def test_already_dismissed_when_finding_is_justified( finally: _sys.stdout = old assert rc == 0 - assert summary["already_dismissed"] == ["premise_f0"] + assert summary["already_dismissed"] == ["cq_f0"] assert summary["re_asserted"] == [] # And critically: no override file written. Re-asserting a # justified finding must not silently promote it on the next run. - assert _load_override(cache, "premise_f0") is None + assert _load_override(cache, "cq_f0") is None def test_system_scoped_re_assert_writes_sentinel_and_is_honored( self, tmp_path: Path, @@ -11793,12 +11432,12 @@ def test_non_bha_reviewers_unchanged(self) -> None: from code_review_helpers import _verification_by_reviewer verified = [ self._finding("bhb_f0", "bhb", "CONFIRMED"), - self._finding("premise_f0", "premise", "CONFIRMED"), + self._finding("test_quality_f0", "test_quality", "CONFIRMED"), self._finding("auditor_f0", "auditor", "CONFIRMED"), self._finding("bha_p0_f0", "bha_p0", "CONFIRMED"), ] out = _verification_by_reviewer(verified, []) - assert set(out.keys()) == {"bhb", "premise", "auditor", "bha_p0"} + assert set(out.keys()) == {"bhb", "test_quality", "auditor", "bha_p0"} def test_re_asserted_counter_attributes_to_correct_partition(self) -> None: from code_review_helpers import _verification_by_reviewer @@ -12570,8 +12209,8 @@ class TestPR121TaxonomyCommentNoDanglingReference: """MED #3: signal_taxonomy.json comment must not reference a bootstrap mirror that does not yet exist. - Reviewer: Bug Hunter B + Premise both flagged the same doc-accuracy - gap. A developer adding a signal would look for the bootstrap mirror, + A doc-accuracy gap: a developer adding a signal would look for the + bootstrap mirror, find nothing, and either skip the step (breaking the documented invariant) or be confused. Fix: comment now defers the mirror to Phase 9 explicitly, rather than implying a co-located file exists. @@ -17347,7 +16986,7 @@ class TestPLN725Phase8DeriveSpawnSpecBlockingSanitization: so review still ran, but actioning a verifier-rejected plan can spawn agents the closed_vocabulary / shape / evidence checks flagged. Sanitization keeps the canonical static fleet running - (BHB, Auditor, Premise, BHA per partition) while suppressing + (BHB, Auditor, BHA per partition) while suppressing every rule/critic-source reviewer; the BLOCKING gap finding already lives in agent_coverage-verify-blocking.json so the operator sees the rejection. @@ -20108,7 +19747,7 @@ def test_read_returns_empty_when_cached_tier_too_weak( self, tmp_path: Path, capsys: pytest.CaptureFixture[str], ) -> None: """shallow cached + standard invocation → cache miss. - Forces the standard upgrade to actually run premise + critics.""" + Forces the standard upgrade to actually run signal extraction + critics.""" cmd_review_state_write(self._ns_write(tmp_path, "abc", depth="shallow")) capsys.readouterr() rc = cmd_review_state_read(self._ns_read(tmp_path, depth="standard")) @@ -20767,8 +20406,8 @@ def test_annotate_defer_reason_preserves_original_fields(self) -> None: class TestPLN807Phase5TierMismatchNudge: """PLN-807 Phase 5: hygiene-stage tier-mismatch nudge. - When shallow runs on a PR that would have benefited from premise - or critic-gates entries, a single MEDIUM system-scoped finding + When shallow runs on a PR that would have benefited from signal + extraction or critic-gates entries, a single MEDIUM system-scoped finding (category ``Coverage``) is emitted so the user can see what was skipped. MEDIUM rather than LOW because validate's SEVERITY_NORMALIZE map DISCARDs ``"low"`` — a LOW nudge would @@ -21676,10 +21315,7 @@ def test_thresholds_dict_flows_through_compute_canonical_verdict( # category coverage. verified = [self._impact_finding("HIGH")] verdict, reason = _compute_canonical_verdict( - verified, [], thresholds={ - "premise_cumulative_medium": 3, - "impact_cumulative": 1, - }, + verified, [], thresholds={"impact_cumulative": 1}, ) assert verdict == "NEEDS_ATTENTION" assert reason # non-empty reason from whichever rule fired @@ -22016,7 +21652,7 @@ def test_stats_includes_impact_cumulative_count(self) -> None: # The key MUST exist; absence would render the SKILL.md # presenter footer line as None and break the Rule 6 docstring's # contract. The value matches _count_gateable_impact exactly - # (single source of truth; mirrors premise_cumulative_medium_count). + # (single source of truth). assert "impact_cumulative_count" in stats assert stats["impact_cumulative_count"] == 2 # MEDIUM excluded @@ -22033,9 +21669,8 @@ def test_stats_impact_cumulative_count_zero_with_no_impact_findings( class TestFEA1401VerdictThresholds: - """Verify ``_load_verdict_thresholds`` parses the new - ``impact_cumulative`` key with the same validation contract as - ``premise_cumulative_medium`` (int, ≥ 1, not bool).""" + """Verify ``_load_verdict_thresholds`` parses the + ``impact_cumulative`` key with int, ≥ 1, not-bool validation.""" def test_default_impact_threshold_when_no_config(self) -> None: from code_review_helpers import ( diff --git a/plugins/code-review/tools/python/test_code_review_schema.py b/plugins/code-review/tools/python/test_code_review_schema.py index 59eda3b7..73d71589 100644 --- a/plugins/code-review/tools/python/test_code_review_schema.py +++ b/plugins/code-review/tools/python/test_code_review_schema.py @@ -40,8 +40,10 @@ # --------------------------------------------------------------------------- -def test_schema_version_is_one(): - assert SCHEMA_VERSION == 1 +def test_schema_version_is_two(): + # v2 retired the Premise finding category when the premise reviewer + # was removed. + assert SCHEMA_VERSION == 2 def test_finding_scopes(): @@ -55,10 +57,15 @@ def test_severities(): def test_categories_include_canonical_and_legacy_alias(): assert "Hygiene" in CATEGORIES assert "Repo Hygiene" in CATEGORIES # legacy alias retained - assert "Premise" in CATEGORIES assert "Coverage" in CATEGORIES +def test_premise_category_retired(): + # v2: the premise reviewer was removed; no producer emits Premise, so + # it is no longer a valid category (the validator rejects it). + assert "Premise" not in CATEGORIES + + def test_categories_include_code_quality(): """Code Quality is the canonical category for DRY/maintainability findings. diff --git a/plugins/code-review/tools/python/test_golden_fixtures.py b/plugins/code-review/tools/python/test_golden_fixtures.py index 565bffce..9ac3d238 100644 --- a/plugins/code-review/tools/python/test_golden_fixtures.py +++ b/plugins/code-review/tools/python/test_golden_fixtures.py @@ -26,11 +26,9 @@ # Phase 8 originally shipped 3 fixtures end-to-end + 6 deferred skeletons. # PLN-720 (plan 01) added a 4th end-to-end fixture: golden_injection_quarantine. -# The remaining 5 fixtures depend on plans 02/03/05/06 outputs and stay +# The remaining 3 fixtures depend on plans 03/05/06 outputs and stay # deferred until those plans land. _DEFERRED_FIXTURES: dict[str, str] = { - "golden_premise_justified": "plan 02 (justification) not shipped", - "golden_premise_rejected": "plan 02 (justification) not shipped", "golden_impact_with_callsites": "plan 06 (external impact) not shipped", "golden_coverage_gap": "plans 03 + 05 (verifier + coverage) not shipped", "golden_budget_exceeded": "arbitrate-budget integration fixture pending", From 204fa7bc5de7d0be79af2dbafd30754468772f1a Mon Sep 17 00:00:00 2001 From: Kris Wong Date: Fri, 26 Jun 2026 12:14:06 -0500 Subject: [PATCH 2/4] fix(code-review): pin effort:high on review workers so a lowered session effort can't starve reviewers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reasoning effort, unlike model, has no per-Task override — a spawned subagent inherits the session effort unless its own frontmatter pins one. The two worker shells (code-review-worker, code-review-worker-graph) carried no effort, so running /code-review from a lowered-effort session would silently drop every spawned reviewer's reasoning depth to that lower level, degrading review quality even though per-spawn model routing keeps their model identity intact. Pin effort: high on both worker defs (valid on both Opus and Sonnet, the two route-assigned reviewer models) so reviewer reasoning depth is fixed regardless of session effort. This is the effort-axis analogue of the per-spawn model insulation and makes the "run /code-review from a cheap Sonnet session" cost path safe to combine with a lowered session effort. Document the effort axis in start.md and README alongside the existing session-model cost guidance. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 1 + plugins/code-review/README.md | 2 +- plugins/code-review/agents/code-review-worker-graph.md | 1 + plugins/code-review/agents/code-review-worker.md | 1 + plugins/code-review/commands/start.md | 2 +- 5 files changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d7336ef..23d4e4c9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). #### Changed - Bumped `SCHEMA_VERSION` 1 → 2 for the `Finding` + `ResultEnvelope` contract to reflect the removed `Premise` category and telemetry keys. Because the schema version is folded into the prompt/cache hash, this invalidates the Bug Hunter A and verification caches once on rollout. `verdict-thresholds.json` now exposes only `impact_cumulative` (the FEA-1401 Rule 6 Impact gate), and `stats` retains `impact_cumulative_count` in place of the removed premise count. +- Pinned `effort: high` in the `code-review-worker` and `code-review-worker-graph` subagent definitions so the reviewer fleet's reasoning depth no longer drops when `/code-review` is run from a lower session effort level. Reasoning effort, unlike `model`, has no per-invocation override — a spawned reviewer otherwise inherits the session effort — so pinning it in frontmatter holds every reviewer at `high` regardless of the session level (`high` is valid on both Opus and Sonnet, the route-assigned reviewer models). `start.md` and `README.md` now document the effort axis alongside the existing session-model cost guidance. ### code-review v2.37.1 diff --git a/plugins/code-review/README.md b/plugins/code-review/README.md index 526644d5..5f0d4012 100644 --- a/plugins/code-review/README.md +++ b/plugins/code-review/README.md @@ -7,7 +7,7 @@ A multi-agent code review plugin for Claude Code that performs deep, partitioned - **Multi-agent parallel review**: Splits changed files into partitions and spawns concurrent reviewer agents (Bug Hunter A, plus domain specialists) to review each partition independently - **Deterministic hygiene checks**: Pattern-based checks for CI artifacts, sensitive file exposure, and path leakage — zero LLM tokens required - **Risk-based model routing**: Scores each file partition by risk (size, file type, LOC) and routes high-risk partitions to more capable models -- **Cost-optimized orchestration**: The orchestrator walk is mechanical (run helper, read JSON, honor gates) and carries no diff or large artifacts in its own context, so it is cheap to run on a lower-cost session model while spawned reviewer and verifier subagents keep their own route-assigned models — run `/code-review` from a standard-context Sonnet session for the cheapest orchestrator without changing review quality +- **Cost-optimized orchestration**: The orchestrator walk is mechanical (run helper, read JSON, honor gates) and carries no diff or large artifacts in its own context, so it is cheap to run on a lower-cost session model while spawned reviewer and verifier subagents keep their own route-assigned models — run `/code-review` from a standard-context Sonnet session for the cheapest orchestrator without changing review quality (the worker subagents pin `effort: high`, so lowering the session effort too won't reduce reviewer reasoning depth) - **Finding validation and deduplication**: Normalizes severity, filters low-confidence findings, deduplicates near-duplicate issues via Jaccard similarity, and validates line numbers against the actual diff - **Incremental reviews**: Tracks prior review state to diff only new commits since the last successful review (auto-incremental mode) - **Caching**: Content-addressed cache keyed on prompt hash and diff tip to skip re-reviewing unchanged partitions diff --git a/plugins/code-review/agents/code-review-worker-graph.md b/plugins/code-review/agents/code-review-worker-graph.md index bf239047..06c9f327 100644 --- a/plugins/code-review/agents/code-review-worker-graph.md +++ b/plugins/code-review/agents/code-review-worker-graph.md @@ -2,6 +2,7 @@ name: code-review-worker-graph description: Graph-aware code review worker for the cross-file reviewers (Impact Analyzer, Bug Hunter B, fast-path). Identical to code-review-worker but adds read-only codebase-memory-mcp tools for precise cross-file usage discovery. Use only for reviewers whose role prompt loads the codebase knowledge graph protocol. tools: Read, Write, Grep, Glob, mcp__codebase-memory-mcp__search_graph, mcp__codebase-memory-mcp__trace_path, mcp__codebase-memory-mcp__get_code_snippet, mcp__codebase-memory-mcp__search_code +effort: high # pinned so a lowered session effort can't cut reviewer reasoning depth (no per-Task override; frontmatter is the only lever). Not redundant with the default — do not remove. Rationale: start.md "Orchestrator model (cost)". --- # Code Review Worker (graph-aware) diff --git a/plugins/code-review/agents/code-review-worker.md b/plugins/code-review/agents/code-review-worker.md index 9257ab9d..d378987c 100644 --- a/plugins/code-review/agents/code-review-worker.md +++ b/plugins/code-review/agents/code-review-worker.md @@ -2,6 +2,7 @@ name: code-review-worker description: Worker agent for partitioned code review tasks. Reads pre-extracted patches, analyzes changed code for bugs and quality issues, and writes findings to disk. Use when spawning background review agents that need file access. tools: Read, Write, Grep, Glob +effort: high # pinned so a lowered session effort can't cut reviewer reasoning depth (no per-Task override; frontmatter is the only lever). Not redundant with the default — do not remove. Rationale: start.md "Orchestrator model (cost)". --- # Code Review Worker diff --git a/plugins/code-review/commands/start.md b/plugins/code-review/commands/start.md index 28632458..2905321e 100644 --- a/plugins/code-review/commands/start.md +++ b/plugins/code-review/commands/start.md @@ -56,7 +56,7 @@ The walk is hybrid: - **Agent fleet stages** — spawn parallel sub-agent Tasks. `stage_20_spawn_reviewers` invokes the `code-review:spawn-reviewers` skill; `stage_23_verify_findings` invokes the `code-review:verify-findings` skill. - **Present stage** (`stage_29_present`) — invoke the `code-review:present-local` skill (MODE=local) or follow `github-review.md` (MODE=github). -**Orchestrator model (cost).** The orchestrator runs on the **session model** — there is intentionally no `model:` frontmatter override. The walk is mechanical (run helper, read JSON, honor gates) with no Opus-grade reasoning, so it is safe on a cheaper model; the judgment lives in the **subagents**, which keep their own route-assigned models regardless (BHA Opus / Sonnet on test-only partitions, domain critics Sonnet, Impact Analyzer Opus, verifiers per the verify skill). The spine is ~65% of historical review cost (~180 turns/deep-review, cache-read dominated), so for the cheapest run invoke `/code-review` from a standard-context **Sonnet** session (`/model sonnet`) — the reviewers stay on their assigned models either way. A `model: sonnet` frontmatter override is **deliberately avoided**: a per-command model override inherits the session's context-window tier, so on a **1M-context** session it resolves to Sonnet-with-1M and bills as extra pay-as-you-go API usage outside a Pro/Max subscription (a known Claude Code limitation with no per-command opt-out) — which would invert the saving. +**Orchestrator model (cost).** The orchestrator runs on the **session model** — there is intentionally no `model:` frontmatter override. The walk is mechanical (run helper, read JSON, honor gates) with no Opus-grade reasoning, so it is safe on a cheaper model; the judgment lives in the **subagents**, which keep their own route-assigned models regardless (BHA Opus / Sonnet on test-only partitions, domain critics Sonnet, Impact Analyzer Opus, verifiers per the verify skill). The spine is ~65% of historical review cost (~180 turns/deep-review, cache-read dominated), so for the cheapest run invoke `/code-review` from a standard-context **Sonnet** session (`/model sonnet`) — the reviewers stay on their assigned models either way. A `model: sonnet` frontmatter override is **deliberately avoided**: a per-command model override inherits the session's context-window tier, so on a **1M-context** session it resolves to Sonnet-with-1M and bills as extra pay-as-you-go API usage outside a Pro/Max subscription (a known Claude Code limitation with no per-command opt-out) — which would invert the saving. **Effort is a separate axis** and, unlike `model`, has no per-Task override — a subagent inherits the session effort unless its own frontmatter pins one. So the two worker agent defs pin `effort: high`, fixing every reviewer's reasoning depth regardless of the session level. That lets you drop the session **model** *and* **effort** for the cheapest orchestrator without starving reviewer thinking (`high` is valid on both Opus and Sonnet, so it holds across the route-assigned reviewer models). **Turn & context discipline (cost).** Cache cost scales with carried context × turn count, so keep both small: - **Never read large artifacts into the orchestrator's context.** `diff_data.json`, `patches_*.txt`, and per-file diffs are passed to helpers and reviewers as **file-path arguments**, never `cat`/`Read` into the walk. Reviewers read patches themselves (see the spawn skill's anti-inline rule). The only large file the orchestrator reads is `review_result.json` at the present stage, once, with the per-section display caps the present skill already applies. From a189ed1e282f67e0b2e49be1353c077c47120c8b Mon Sep 17 00:00:00 2001 From: Kris Wong Date: Fri, 26 Jun 2026 13:07:57 -0500 Subject: [PATCH 3/4] fix(code-review): clean up residual Premise references after the category excision Follow-up cleanup for the v3.0.0 Premise excision, addressing stale artifacts left behind: - Remove the four now-unreachable Premise reasoning-certificate kinds (necessity, cohesion, workaround, complexity) from REASONING_CERTIFICATE_KINDS; no producer emits them and the constant was never referenced. - Retarget the orphaned PLN-773 stats test from the removed stats.justification / stats.by_subcategory premise sub-blocks to the live stats.verification sub-block (justified_valid_count / justified_invalid_count / by_reviewer). - Disambiguate the cumulative Impact verdict gate's cross-references: start.md and README.md now name it (FEA-1401 / PLN-726 OQ#6) instead of a bare "Rule 6" ordinal that collided with SCHEMA.md's renumbered sequential precedence list (Impact is item 4 there; Rule 6 = APPROVED). Add a numbering note to SCHEMA.md recording that _compute_canonical_verdict's plan-derived rule labels (2.5, 3.5, "Rule 6") do not map 1:1 to the table's sequential numbers. The code and test labels are left intact -- internally consistent and OQ#6-tagged. - Fix the SCHEMA.md deferred-fixture count to match _DEFERRED_FIXTURES (3 deferred; golden_injection_quarantine was promoted to a shipped end-to-end fixture by PLN-720). No behavior change. 1209 tests pass; ruff + pyright clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 6 ++++- plugins/code-review/README.md | 2 +- plugins/code-review/SCHEMA.md | 10 +++++--- plugins/code-review/commands/start.md | 4 +-- .../tools/python/code_review_schema.py | 4 --- .../tools/python/test_code_review_schema.py | 25 +++++++------------ 6 files changed, 23 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 23d4e4c9..91f2a1b0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,11 +8,15 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). #### Removed - Retired the `Premise` finding category and its entire processing layer, now inert since the premise reviewer was removed (no producer emits `category: "Premise"`). Dropped `Premise` from the `CATEGORIES` schema enum; removed the two verdict-precedence rules that gated on Premise findings (the Premise priority-0 → `CHANGES_REQUESTED` rule and the cumulative-Premise → `NEEDS_ATTENTION` rule) along with the `_count_gateable_premise_medium` helper, the `premise_cumulative_medium` operator threshold, and its `justification_rate_alert` companion; removed the Premise-scoped `stats` telemetry sub-blocks (`by_subcategory`, `justification`, `premise_cumulative_medium_count`); removed the verifier "always verify Premise" eligibility branch and the Premise extra-strictness blocks from `verifier_prompt.txt`; and deleted the four `premise_*.md` fix templates with their `/fix` dispatch rows. The shared/BHA reviewer prompts keep the generic "PREMISE:" reasoning step (what the code is supposed to do), and the general author-justification machinery — the `justified[]` bucket, `JUSTIFIED-VALID`/`JUSTIFIED-INVALID` verdicts, the verifier's J1/J2 audit, and the justification-audit learning stream — is retained; only the Premise-specific pieces were removed. +- Removed the four now-unreachable Premise reasoning-certificate kinds (`necessity`, `cohesion`, `workaround`, `complexity`) from `REASONING_CERTIFICATE_KINDS`; only the active reviewer/reasoning-step kinds (`impact`, `test_quality`, `sibling_pattern`, `bha`, `bhb`, `auditor`) remain. #### Changed -- Bumped `SCHEMA_VERSION` 1 → 2 for the `Finding` + `ResultEnvelope` contract to reflect the removed `Premise` category and telemetry keys. Because the schema version is folded into the prompt/cache hash, this invalidates the Bug Hunter A and verification caches once on rollout. `verdict-thresholds.json` now exposes only `impact_cumulative` (the FEA-1401 Rule 6 Impact gate), and `stats` retains `impact_cumulative_count` in place of the removed premise count. +- Bumped `SCHEMA_VERSION` 1 → 2 for the `Finding` + `ResultEnvelope` contract to reflect the removed `Premise` category and telemetry keys. Because the schema version is folded into the prompt/cache hash, this invalidates the Bug Hunter A and verification caches once on rollout. `verdict-thresholds.json` now exposes only `impact_cumulative` (the FEA-1401 cumulative Impact gate, PLN-726 OQ#6), and `stats` retains `impact_cumulative_count` in place of the removed premise count. - Pinned `effort: high` in the `code-review-worker` and `code-review-worker-graph` subagent definitions so the reviewer fleet's reasoning depth no longer drops when `/code-review` is run from a lower session effort level. Reasoning effort, unlike `model`, has no per-invocation override — a spawned reviewer otherwise inherits the session effort — so pinning it in frontmatter holds every reviewer at `high` regardless of the session level (`high` is valid on both Opus and Sonnet, the route-assigned reviewer models). `start.md` and `README.md` now document the effort axis alongside the existing session-model cost guidance. +#### Fixed +- Corrected documentation and a test left stale by the `Premise` removal and the renumbered verdict-precedence list. The cumulative Impact verdict gate is now referenced by name (FEA-1401 / PLN-726 OQ#6) in `start.md` and `README.md` rather than a bare ordinal that collided with `SCHEMA.md`'s sequential rules, and a numbering note in `SCHEMA.md` records that `_compute_canonical_verdict`'s plan-derived rule labels do not map 1:1 to that list. The `SCHEMA.md` deferred-fixture count now matches `_DEFERRED_FIXTURES` (3 deferred; `golden_injection_quarantine` is listed as a shipped fixture). The schema test that exercised the removed `stats.justification` / `stats.by_subcategory` telemetry sub-blocks now targets the live `stats.verification` sub-block (`justified_valid_count` / `justified_invalid_count` / `by_reviewer`). + ### code-review v2.37.1 #### Fixed diff --git a/plugins/code-review/README.md b/plugins/code-review/README.md index 5f0d4012..2582258a 100644 --- a/plugins/code-review/README.md +++ b/plugins/code-review/README.md @@ -272,7 +272,7 @@ Tunes the verdict-precedence gates: | Key | Default | Effect | |---|---|---| -| `impact_cumulative` | `2` | Trigger `NEEDS_ATTENTION` when at least N BLOCKING/HIGH `ImpactAnalysis` findings survive verification on the same PR (Rule 6), even if no single finding would gate on its own. Set to a very large number (e.g. `999`) to disable. Values below 1 are ignored. | +| `impact_cumulative` | `2` | Trigger `NEEDS_ATTENTION` when at least N BLOCKING/HIGH `ImpactAnalysis` findings survive verification on the same PR (the cumulative Impact gate — SCHEMA.md §5 verdict-precedence Rule 4 / FEA-1401 OQ#6), even if no single finding would gate on its own. Set to a very large number (e.g. `999`) to disable. Values below 1 are ignored. | ### `verification-gates.json` (PLN-722) diff --git a/plugins/code-review/SCHEMA.md b/plugins/code-review/SCHEMA.md index f38fd85f..823ba39b 100644 --- a/plugins/code-review/SCHEMA.md +++ b/plugins/code-review/SCHEMA.md @@ -276,6 +276,8 @@ The `verdict` subcommand applies these rules in order; the first match wins. `verdict_reason` cites the specific finding(s) that produced the verdict. +**Numbering note.** This list is a clean sequential summary. The implementation (`_compute_canonical_verdict`) carries plan-derived rule labels — including the `2.5` (mandatory-human-review short-circuit) and `3.5` (TENTATIVE fall-through) sub-rules, and the cumulative Impact gate labeled **Rule 6** (PLN-726 OQ#6) — so the code's labels do not map 1:1 to the numbers above (e.g. the Impact gate is item 4 here but "Rule 6" in code). + The verdict subcommand writes `/verdict.json` with both the canonical verdict and a `verdict` string compatible with `run-loop.sh` (which keys on the legacy form): - APPROVED → approve - NEEDS_ATTENTION → needs_attention @@ -647,12 +649,12 @@ the same normalization path the assertion uses, so a subsequent no-flag run sees byte-identical output. Updates are reviewed in the commit diff, not auto-merged. -**Phase 8 ships 3 fixtures end-to-end** +**Phase 8 shipped 3 fixtures end-to-end** (`golden_minimal_correctness`, `golden_all_categories`, `golden_schema_v1_round_trip`) plus a byte-identical determinism test -for `prepare-run`. The 4 fixtures requiring plans 03/05/06 -(`golden_impact_with_callsites`, -`golden_coverage_gap`, `golden_injection_quarantine`, +for `prepare-run`; PLN-720 promoted a 4th, `golden_injection_quarantine`. +The remaining 3 fixtures requiring plans 03/05/06 +(`golden_impact_with_callsites`, `golden_coverage_gap`, `golden_budget_exceeded`) have reserved directories with READMEs and are skipped via a `_DEFERRED_FIXTURES` map in the test module until their dependent plans land. Phase 4b will extend the harness to walk diff --git a/plugins/code-review/commands/start.md b/plugins/code-review/commands/start.md index 2905321e..0b93b097 100644 --- a/plugins/code-review/commands/start.md +++ b/plugins/code-review/commands/start.md @@ -330,7 +330,7 @@ Override the verdict-precedence thresholds without forking the plugin: | Key | Default | Effect | |-----|---------|--------| -| `impact_cumulative` | `2` | Number of BLOCKING/HIGH `ImpactAnalysis` findings on a single PR that flip the verdict to `NEEDS_ATTENTION` (Rule 6 in `_compute_canonical_verdict`). Set higher to relax the gate; raise above any realistic finding count to disable. | +| `impact_cumulative` | `2` | Number of BLOCKING/HIGH `ImpactAnalysis` findings on a single PR that flip the verdict to `NEEDS_ATTENTION` (the cumulative Impact gate in `_compute_canonical_verdict`; FEA-1401 / PLN-726 OQ#6). Set higher to relax the gate; raise above any realistic finding count to disable. | Unknown keys are ignored. Non-integer or `< 1` values fall back to the default — the file is operator-authored and should not crash the pipeline on a typo. @@ -366,7 +366,7 @@ These notes annotate the run-plan stages with anything not obvious from the plan - **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_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` Rule 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_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**. - **stage_27_review_state_write**: gated by **Gate D**. - **stage_29_present**: present stage. Invoke the `code-review:present-local` skill (MODE=local) or follow Steps 6 and 8 in `github-review.md` (MODE=github). The mode-agnostic Gate A hygiene-only early-exit fires before this stage and uses its own format section above. diff --git a/plugins/code-review/tools/python/code_review_schema.py b/plugins/code-review/tools/python/code_review_schema.py index fbedc94e..fdff1905 100644 --- a/plugins/code-review/tools/python/code_review_schema.py +++ b/plugins/code-review/tools/python/code_review_schema.py @@ -332,10 +332,6 @@ }) REASONING_CERTIFICATE_KINDS: frozenset[str] = frozenset({ - "necessity", - "cohesion", - "workaround", - "complexity", "impact", "test_quality", "sibling_pattern", diff --git a/plugins/code-review/tools/python/test_code_review_schema.py b/plugins/code-review/tools/python/test_code_review_schema.py index 73d71589..35e060f8 100644 --- a/plugins/code-review/tools/python/test_code_review_schema.py +++ b/plugins/code-review/tools/python/test_code_review_schema.py @@ -567,25 +567,18 @@ def test_envelope_accepts_re_asserted_verifier_verdict(): assert validate_result_envelope(env) == [] -def test_envelope_accepts_pln773_telemetry_sub_blocks(): - """PLN-773: stats gains optional sub-blocks for justification metrics, - by_subcategory partitioning, and per-reviewer FP rate. The envelope - `stats` field accepts arbitrary keys so these are additive.""" +def test_envelope_accepts_verification_stats_sub_block(): + """PLN-773: the envelope `stats` field accepts the verification telemetry + sub-block additively — the `justified_valid_count` / `justified_invalid_count` + tallies plus per-reviewer FP rate (`by_reviewer`), as emitted by + `_stats_from_findings`. (The earlier premise-scoped `justification` and + `by_subcategory` sub-blocks were removed with the Premise category, so no + pipeline stage emits them anymore.)""" env = _minimal_envelope() env["stats"] = { - "justification": { - "rate": 0.18, - "rejection_rate": 0.40, - "total_premise": 11, - "justified_emitted": 2, - "justified_valid": 1, - "justified_invalid": 1, - "threshold_alert": False, - }, - "by_subcategory": { - "necessity": 2, "cohesion": 1, "workaround": 0, "complexity": 1, - }, "verification": { + "justified_valid_count": 2, + "justified_invalid_count": 1, "by_reviewer": { "bug_hunter_a": { "verified": 12, "rejected": 3, From dc02b06f3b6d6a8278928a59ff699d340655cb48 Mon Sep 17 00:00:00 2001 From: Kris Wong Date: Fri, 26 Jun 2026 13:37:10 -0500 Subject: [PATCH 4/4] fix(code-review): restore GitHub-mode Impact gate count line; clarify v1 round-trip fixture Two more residual-Premise-excision follow-ups: - github-review.md's Verifier Stats block was missing the Impact gateable count line that present-local/SKILL.md already shows. The Premise cumulative-gate display removed from both presenters was replaced only in local mode, leaving GitHub-mode operators with no line for stats.impact_cumulative_count (the envelope's sole operator-tunable verdict-gate count). Mirror the local-mode line before the Partition mode line. - Clarify the golden_schema_v1_round_trip fixture's config description. The SCHEMA_VERSION 1->2 bump restamps the envelope to 2 while each finding's own stamp is preserved, so the fixture round-trips a schema_version: 1 finding through a schema_version: 2 envelope. The finding is genuinely v1 (same as every other golden fixture's findings), so the name/"v1" are accurate -- not renamed; the description now states the v1-finding / v2-envelope split explicitly so it reads as the backward-compat probe it is. No behavior change. 1209 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 1 + plugins/code-review/prompts/github-review.md | 2 ++ .../fixtures/golden_schema_v1_round_trip/config.yaml | 10 +++++++--- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 91f2a1b0..672b63f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). #### Fixed - Corrected documentation and a test left stale by the `Premise` removal and the renumbered verdict-precedence list. The cumulative Impact verdict gate is now referenced by name (FEA-1401 / PLN-726 OQ#6) in `start.md` and `README.md` rather than a bare ordinal that collided with `SCHEMA.md`'s sequential rules, and a numbering note in `SCHEMA.md` records that `_compute_canonical_verdict`'s plan-derived rule labels do not map 1:1 to that list. The `SCHEMA.md` deferred-fixture count now matches `_DEFERRED_FIXTURES` (3 deferred; `golden_injection_quarantine` is listed as a shipped fixture). The schema test that exercised the removed `stats.justification` / `stats.by_subcategory` telemetry sub-blocks now targets the live `stats.verification` sub-block (`justified_valid_count` / `justified_invalid_count` / `by_reviewer`). +- Restored the operator-facing Impact gate count in GitHub-mode output: the `github-review.md` Verifier Stats block now shows `Impact gateable count` (`stats.impact_cumulative_count`, gate threshold `impact_cumulative`) to match the local-mode presenter — the removed Premise cumulative-gate display had left GitHub mode with no line for the envelope's sole operator-tunable verdict-gate count. Also clarified the `golden_schema_v1_round_trip` fixture description to reflect that it round-trips a `schema_version: 1` finding through the current `schema_version: 2` envelope (a v1-finding backward-compat probe), instead of implying a stale v1-only test. ### code-review v2.37.1 diff --git a/plugins/code-review/prompts/github-review.md b/plugins/code-review/prompts/github-review.md index 14654b2c..d782eaa8 100644 --- a/plugins/code-review/prompts/github-review.md +++ b/plugins/code-review/prompts/github-review.md @@ -302,6 +302,8 @@ Read `$CR_DIR/review_result.json` → `stats.verification`. If the block is not The Reviewer column keys off the `reviewer` field, which `cmd_collect_findings` derives from the agent filename (`agent_bha_p0.json` → `reviewer='bha_p0'`). Under partitioned mode the table shows one BHA row per partition (`bha_p0`, `bha_p1`, …); under unified mode it shows a single `bha_p0` row because only one partition exists. +**Impact gateable count**: {stats.impact_cumulative_count} (gate threshold {impact_cumulative}) — read from `stats`. The envelope's sole operator-tunable verdict-gate count (the cumulative Impact gate; FEA-1401 / PLN-726 OQ#6). + **Partition mode** ({verify_manifest.partition_mode}, {verify_manifest.partition_count} partitions) — read from `/verify_manifest.json`. Omit this line when the manifest file is absent (hygiene-only run or pre-PLN-774 cache).
diff --git a/plugins/code-review/tools/python/fixtures/golden_schema_v1_round_trip/config.yaml b/plugins/code-review/tools/python/fixtures/golden_schema_v1_round_trip/config.yaml index 754905c2..daa93bad 100644 --- a/plugins/code-review/tools/python/fixtures/golden_schema_v1_round_trip/config.yaml +++ b/plugins/code-review/tools/python/fixtures/golden_schema_v1_round_trip/config.yaml @@ -1,9 +1,13 @@ # PLN-719 Section 10 fixture: schema round-trip exemplar. # A single finding with every optional schema field populated # (evidence[], reasoning_certificate, other_locations) so the produced -# envelope exercises the maximal v1 shape. The harness's always-on -# validate_result_envelope call pins the round-trip contract. -description: "Maximal v1 finding shape — every optional field populated for round-trip." +# envelope exercises the maximal finding shape. The input finding declares +# schema_version 1; the SCHEMA_VERSION 1->2 bump restamps the envelope to the +# current version (2) while each finding's own stamp is preserved — so this +# fixture doubles as a v1-finding backward-compat probe: the current validator +# still round-trips a finding that declares schema_version 1. The harness's +# always-on validate_result_envelope call pins the round-trip contract. +description: "Maximal finding shape (every optional field populated) declaring schema_version 1, round-tripped through the current schema_version-2 envelope — also pins v1-finding backward-compat." mode: github diff_tip: "deadbeef" expected_verdict: NEEDS_ATTENTION