Skip to content

Commit a189ed1

Browse files
wongkclaude
andcommitted
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) <noreply@anthropic.com>
1 parent 204fa7b commit a189ed1

6 files changed

Lines changed: 23 additions & 28 deletions

File tree

CHANGELOG.md

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,15 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
88

99
#### Removed
1010
- 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.
11+
- 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.
1112

1213
#### Changed
13-
- 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.
14+
- 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.
1415
- 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.
1516

17+
#### Fixed
18+
- 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`).
19+
1620
### code-review v2.37.1
1721

1822
#### Fixed

plugins/code-review/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -272,7 +272,7 @@ Tunes the verdict-precedence gates:
272272

273273
| Key | Default | Effect |
274274
|---|---|---|
275-
| `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. |
275+
| `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. |
276276

277277
### `verification-gates.json` (PLN-722)
278278

plugins/code-review/SCHEMA.md

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -276,6 +276,8 @@ The `verdict` subcommand applies these rules in order; the first match wins.
276276

277277
`verdict_reason` cites the specific finding(s) that produced the verdict.
278278

279+
**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).
280+
279281
The verdict subcommand writes `<CR_DIR>/verdict.json` with both the canonical verdict and a `verdict` string compatible with `run-loop.sh` (which keys on the legacy form):
280282
- APPROVED → approve
281283
- NEEDS_ATTENTION → needs_attention
@@ -647,12 +649,12 @@ the same normalization path the assertion uses, so a subsequent
647649
no-flag run sees byte-identical output. Updates are reviewed in the
648650
commit diff, not auto-merged.
649651

650-
**Phase 8 ships 3 fixtures end-to-end**
652+
**Phase 8 shipped 3 fixtures end-to-end**
651653
(`golden_minimal_correctness`, `golden_all_categories`,
652654
`golden_schema_v1_round_trip`) plus a byte-identical determinism test
653-
for `prepare-run`. The 4 fixtures requiring plans 03/05/06
654-
(`golden_impact_with_callsites`,
655-
`golden_coverage_gap`, `golden_injection_quarantine`,
655+
for `prepare-run`; PLN-720 promoted a 4th, `golden_injection_quarantine`.
656+
The remaining 3 fixtures requiring plans 03/05/06
657+
(`golden_impact_with_callsites`, `golden_coverage_gap`,
656658
`golden_budget_exceeded`) have reserved directories with READMEs and
657659
are skipped via a `_DEFERRED_FIXTURES` map in the test module until
658660
their dependent plans land. Phase 4b will extend the harness to walk

plugins/code-review/commands/start.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -330,7 +330,7 @@ Override the verdict-precedence thresholds without forking the plugin:
330330

331331
| Key | Default | Effect |
332332
|-----|---------|--------|
333-
| `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. |
333+
| `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. |
334334

335335
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.
336336

@@ -366,7 +366,7 @@ These notes annotate the run-plan stages with anything not obvious from the plan
366366
- **stage_22b_verify_prepare** (PLN-722): tier-selects findings for verification per the canonical table — BLOCKING/HIGH always; MEDIUM with confidence < 0.85 yes; MEDIUM with confidence ≥ 0.85 no; LOW (P3) no; `category: "Hygiene"` no; `source: "injection-detector"` no. Ranks the eligible set by `severity_weight × confidence`, caps at `VERIFY_MAX_VERIFICATIONS = 50`, and writes (a) `<CR_DIR>/verify_manifest.json` with `to_verify[]` + `skipped_no_verification[]` + `deferred_budget[]` + `cache_hits[]`, and (b) `<CR_DIR>/verifier_inputs/<finding_id>.json` per eligible finding. When `--cache-dir` is set, fresh verifier outputs from a prior run for the same `(finding_id, code_snippet_hash, model, prompt_hash)` tuple are pre-materialized at `agent_verifier_<finding_id>.json` and skipped from `to_verify[]` (logged under `cache_hits[]`). `on_failure: continue` is intentional — verify-prepare failure degrades to "no verifier this run", not a pipeline abort.
367367
- **stage_23_verify_findings** (PLN-722): agent_fleet stage. Invoke the `code-review:verify-findings` skill. Each spawned agent reads its `verifier_inputs/<finding_id>.json` (containing the finding + the `verifier_prompt_path` + the canonical `output_path`) and emits one verdict file at `<CR_DIR>/agent_verifier_<finding_id>.json`. `on_failure: continue` so a single agent crash never aborts review.
368368
- **stage_24a_verify_consolidate** (PLN-722, extended in PLN-721): merges all `agent_verifier_*.json` outputs back into the validated set, applies sensitive-path escalation from `.closedloop-ai/settings/verification-gates.json` (rules: REJECTED on `sensitive_paths` + BLOCKING/HIGH → TENTATIVE with severity capped at HIGH; any finding on `tentative_on_paths` → TENTATIVE; any finding on `mandatory_human_review_paths` → TENTATIVE + `force_human_review: true`), routes JUSTIFIED-VALID verdicts to a new `justified[]` bucket and JUSTIFIED-INVALID verdicts back into `verified[]` (the audited justification was refuted; the original concern stands), and writes `<CR_DIR>/findings_verified.json` with the bucket-split shape `{verified[], rejected[], pending_verification[], justified[], force_human_review}`. `tentative_on_paths` lifts JUSTIFIED-VALID/INVALID to TENTATIVE on the same operator-policy contract as the other verdicts. When `--cache-dir` is set, fresh verifier outputs are written back to the `verifications/` namespace (30-day TTL) for re-use on subsequent runs. Missing fleet outputs degrade to `pending_verification[]`; `on_failure: continue`.
369-
- **stage_25_finalize_result** (PLN-722 + PLN-721): writes `<CR_DIR>/review_result.json` (the canonical envelope) BEFORE running schema validation. PLN-722: prefers `<CR_DIR>/findings_verified.json` (verify-consolidate output) when present and honors its `force_human_review` flag in the verdict computation; falls back to `findings_validated.json` (everything to `verified[]`) when verify-consolidate didn't run. PLN-721: pipes the consolidate `justified[]` bucket into the envelope, and loads operator-overridable thresholds from `.closedloop-ai/settings/verdict-thresholds.json` (defaults to `impact_cumulative=2`; absent/malformed → built-in default) so `_compute_canonical_verdict` 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.
369+
- **stage_25_finalize_result** (PLN-722 + PLN-721): writes `<CR_DIR>/review_result.json` (the canonical envelope) BEFORE running schema validation. PLN-722: prefers `<CR_DIR>/findings_verified.json` (verify-consolidate output) when present and honors its `force_human_review` flag in the verdict computation; falls back to `findings_validated.json` (everything to `verified[]`) when verify-consolidate didn't run. PLN-721: pipes the consolidate `justified[]` bucket into the envelope, and loads operator-overridable thresholds from `.closedloop-ai/settings/verdict-thresholds.json` (defaults to `impact_cumulative=2`; absent/malformed → built-in default) so `_compute_canonical_verdict`'s cumulative Impact gate (FEA-1401 / PLN-726 OQ#6) can fire (≥ 2 BLOCKING/HIGH `ImpactAnalysis` findings in `verified[]` → NEEDS_ATTENTION). A non-zero exit signals reviewer-emitted category/field drift (e.g. a category not in the canonical enum) but does not block the pipeline — `on_failure: continue` lets `stage_28_verdict` read the structurally complete envelope. Surface the stderr text in the present step so operators can correct prompts/schema; do not abort.
370370
- **stage_26_cache_update**: gated by **Gate C**.
371371
- **stage_27_review_state_write**: gated by **Gate D**.
372372
- **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.

plugins/code-review/tools/python/code_review_schema.py

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -332,10 +332,6 @@
332332
})
333333

334334
REASONING_CERTIFICATE_KINDS: frozenset[str] = frozenset({
335-
"necessity",
336-
"cohesion",
337-
"workaround",
338-
"complexity",
339335
"impact",
340336
"test_quality",
341337
"sibling_pattern",

plugins/code-review/tools/python/test_code_review_schema.py

Lines changed: 9 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -567,25 +567,18 @@ def test_envelope_accepts_re_asserted_verifier_verdict():
567567
assert validate_result_envelope(env) == []
568568

569569

570-
def test_envelope_accepts_pln773_telemetry_sub_blocks():
571-
"""PLN-773: stats gains optional sub-blocks for justification metrics,
572-
by_subcategory partitioning, and per-reviewer FP rate. The envelope
573-
`stats` field accepts arbitrary keys so these are additive."""
570+
def test_envelope_accepts_verification_stats_sub_block():
571+
"""PLN-773: the envelope `stats` field accepts the verification telemetry
572+
sub-block additively — the `justified_valid_count` / `justified_invalid_count`
573+
tallies plus per-reviewer FP rate (`by_reviewer`), as emitted by
574+
`_stats_from_findings`. (The earlier premise-scoped `justification` and
575+
`by_subcategory` sub-blocks were removed with the Premise category, so no
576+
pipeline stage emits them anymore.)"""
574577
env = _minimal_envelope()
575578
env["stats"] = {
576-
"justification": {
577-
"rate": 0.18,
578-
"rejection_rate": 0.40,
579-
"total_premise": 11,
580-
"justified_emitted": 2,
581-
"justified_valid": 1,
582-
"justified_invalid": 1,
583-
"threshold_alert": False,
584-
},
585-
"by_subcategory": {
586-
"necessity": 2, "cohesion": 1, "workaround": 0, "complexity": 1,
587-
},
588579
"verification": {
580+
"justified_valid_count": 2,
581+
"justified_invalid_count": 1,
589582
"by_reviewer": {
590583
"bug_hunter_a": {
591584
"verified": 12, "rejected": 3,

0 commit comments

Comments
 (0)