diff --git a/plugins/code-review/.claude-plugin/plugin.json b/plugins/code-review/.claude-plugin/plugin.json index a2c3e3b..1ffce1e 100644 --- a/plugins/code-review/.claude-plugin/plugin.json +++ b/plugins/code-review/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "code-review", "description": "Code review plugin", - "version": "3.10.0", + "version": "3.11.0", "author": { "name": "ClosedLoop", "email": "support@closedloop.ai" diff --git a/plugins/code-review/tools/prompts/verifier_prompt.txt b/plugins/code-review/tools/prompts/verifier_prompt.txt index a79cf64..9a83b4d 100644 --- a/plugins/code-review/tools/prompts/verifier_prompt.txt +++ b/plugins/code-review/tools/prompts/verifier_prompt.txt @@ -160,8 +160,25 @@ verbatim and compare against the stored hash. If the finding targets a removed line (the snippet is no longer present at HEAD), look at the pre-change content via the patch hunks. - - NO match anywhere AND no snippet_hash match → REJECTED, - `rejection_class: "evidence_not_found"`. +Absence splits three ways, and only one of them disproves a finding. +"I could not locate it" and "it is not there" are different claims. + + - **The cited file does not exist under `review_root`** → TENTATIVE. + A missing file is equally well explained by a rename, a bad path, or a + tree that is not the one this diff describes. Name the missing path in + `verifier_reasoning`. + - **The snippet appears in the patch hunks as an ADDED line but is absent + from `review_root`** → TENTATIVE, and say plainly that the tree you + read disagrees with the diff under review. That is a scope or checkout + fault in the run, not evidence against the finding. Never REJECT here: + a tree holding none of the branch's new code makes EVERY finding about + it look fabricated, which is how a whole fleet once rejected seven real + findings at 0.95 confidence and shipped an APPROVED (ISS-10711). + - **The file exists, you read it, and neither the snippet nor any + `snippet_hash` matches anywhere in it, and it is not an added line in + the patch** → REJECTED, `rejection_class: "evidence_not_found"`. + This is the only branch carrying positive disconfirmation: you read the + right file and the claimed code is not in it. - Line drift only (snippet matches at a different line) → continue through the remaining checks. **Do not reject on line drift alone.** - Match found → continue. diff --git a/plugins/code-review/tools/python/code_review_helpers.py b/plugins/code-review/tools/python/code_review_helpers.py index ddd6eeb..8abc80a 100644 --- a/plugins/code-review/tools/python/code_review_helpers.py +++ b/plugins/code-review/tools/python/code_review_helpers.py @@ -10293,12 +10293,48 @@ def _count_gateable_impact(verified: list[dict[str, Any]]) -> int: _VERDICT_IMPACT_THRESHOLD_DEFAULT = 2 +def _approved_verdict_reason(rejected: list[dict[str, Any]] | None) -> str: + """Reason text for an APPROVED reached after verifiers discarded findings. + + An APPROVED carrying an empty reason is indistinguishable from a review + that found nothing, so a run whose every finding was discarded reads as + clean (ISS-10711: seven of eight verifiers rejected real findings and the + rollup said ``APPROVED`` with ``reason: ""``). Naming the count and the + dominant rejection classes leaves the gating decision untouched and makes + it auditable. + + Composed to FIT ``_VERDICT_REASON_MAX`` rather than truncated to it: a + reason chopped mid-class reads as a different class. The count is the + load-bearing half, so classes are dropped (and the elision marked) before + it is. + """ + if not rejected: + return "" + tally: dict[str, int] = {} + for finding in rejected: + name = str(finding.get("rejection_class") or "unclassified") + tally[name] = tally.get(name, 0) + 1 + ranked = sorted(tally.items(), key=lambda kv: (-kv[1], kv[0])) + head = f"{len(rejected)} finding(s) rejected by verification" + shown = list(ranked) + while shown: + body = ", ".join(f"{name} x{count}" for name, count in shown) + if len(shown) < len(ranked): + body += ", ..." + text = f"{head} ({body})" + if len(text) <= _VERDICT_REASON_MAX: + return text + shown.pop() + return head + + def _compute_canonical_verdict( verified: list[dict[str, Any]], coverage_gaps: list[dict[str, Any]], *, force_human_review: bool = False, thresholds: dict[str, int] | None = None, + rejected: list[dict[str, Any]] | None = None, ) -> tuple[str, str]: """Apply canonical verdict precedence rules (PLN-719 Section 5). @@ -10312,6 +10348,10 @@ def _compute_canonical_verdict( callers that do not pass it get the built-in defaults (``impact_cumulative`` = 2; see ``_VERDICT_IMPACT_THRESHOLD_DEFAULT``) so existing test fixtures and back-compat callers keep working. + + ``rejected``: the discarded bucket, used only to give an APPROVED a + non-empty reason (ISS-10711). It never changes which verdict is + returned — a rejected finding is, by definition, not gating. """ thresholds = thresholds or { "impact_cumulative": _VERDICT_IMPACT_THRESHOLD_DEFAULT, @@ -10388,7 +10428,7 @@ def _short(text: str) -> str: f"(threshold {impact_threshold})", ) - return "APPROVED", "" + return "APPROVED", _approved_verdict_reason(rejected) def _read_optional_json(path: Path, default: Any) -> Any: @@ -14325,6 +14365,7 @@ def _normalize_bucket( verified, coverage_gaps, force_human_review=force_human_review, thresholds=thresholds, + rejected=rejected, ) # Pull optional run-context inputs. 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 61646df..6f6773a 100644 --- a/plugins/code-review/tools/python/test_code_review_helpers.py +++ b/plugins/code-review/tools/python/test_code_review_helpers.py @@ -90,6 +90,7 @@ ) from code_review_helpers import ( VERIFY_MAX_VERIFICATIONS, + _VERDICT_REASON_MAX, _compute_canonical_verdict, _glob_to_regex, _load_verification_gates, @@ -8511,6 +8512,42 @@ def test_writes_envelope_with_no_findings(self, tmp_path: Path) -> None: assert envelope["coverage_gaps"] == [] assert envelope["mode"] == "local" + def test_approved_envelope_records_verifier_rejections( + self, tmp_path: Path, + ) -> None: + """ISS-10711 end to end: the reason must survive into the envelope. + + Unit-testing ``_compute_canonical_verdict`` cannot see a call site that + forgets to pass ``rejected``, and that call site is the whole fix. + """ + (tmp_path / "findings_verified.json").write_text(json.dumps({ + "verified": [], + "rejected": [ + minimal_diff_finding( + id=f"bha_p0_f{i}", + verifier_verdict="REJECTED", + rejection_class="evidence_not_found", + ) + for i in range(7) + ], + "pending_verification": [], + })) + result = self._run_finalize(tmp_path, []) + + assert result["validation_errors"] == [], ( + "fixture must be schema-valid, or a real envelope regression " + "hides in this test's noise" + ) + assert result["verdict"] == "APPROVED" + envelope = json.loads((tmp_path / "review_result.json").read_text()) + reason = envelope["verdict_reason"] + assert reason != "", ( + "an APPROVED that discarded 7 findings must not be " + "indistinguishable from a clean review" + ) + assert "7" in reason + assert "evidence_not_found" in reason + def test_envelope_passes_schema_validation(self, tmp_path: Path) -> None: result = self._run_finalize(tmp_path, []) assert result["validation_errors"] == [] @@ -12673,6 +12710,79 @@ def test_confirmed_medium_alone_is_approved(self) -> None: assert v == "APPROVED" +class TestApprovedVerdictReasonISS10711: + """An APPROVED must say when verification discarded every finding. + + ISS-10711: seven of eight verifiers rejected real findings (two P1s, one + writing permanently-wrong rows into an append-only ledger) and the rollup + wrote ``APPROVED`` with ``reason: ""`` — indistinguishable from a review + that found nothing. The verdict itself is deliberately unchanged; only + its auditability is. + """ + + def test_approved_with_no_rejections_keeps_empty_reason(self) -> None: + v, r = _compute_canonical_verdict([], []) + assert v == "APPROVED" + assert r == "" + + def test_approved_after_rejections_names_count_and_class(self) -> None: + rejected = [ + {"rejection_class": "evidence_not_found"} for _ in range(7) + ] + v, r = _compute_canonical_verdict([], [], rejected=rejected) + assert v == "APPROVED", "gating must not change — only the reason" + # The exact ISS-10711 shape: the reason must not be empty, and must + # carry the count a reader needs to distrust the green. + assert r != "" + assert "7" in r + assert "evidence_not_found" in r + + def test_reason_ranks_classes_by_frequency(self) -> None: + # Two short class names both fit the envelope, so ordering is visible. + rejected = ( + [{"rejection_class": "guard_exists"}] * 3 + + [{"rejection_class": "unreachable"}] * 5 + ) + _, r = _compute_canonical_verdict([], [], rejected=rejected) + assert r.index("unreachable x5") < r.index("guard_exists x3") + + def test_tail_is_elided_rather_than_truncated(self) -> None: + # Three classes cannot fit; the dropped ones must be marked, and the + # surviving text must not end mid-class-name. + rejected = ( + [{"rejection_class": "guard_exists"}] * 3 + + [{"rejection_class": "evidence_not_found"}] * 5 + + [{"rejection_class": "unreachable"}] + ) + _, r = _compute_canonical_verdict([], [], rejected=rejected) + assert r.endswith("...)") + assert "evidence_not_found x5" in r + assert len(r) <= _VERDICT_REASON_MAX + + def test_missing_rejection_class_is_not_dropped(self) -> None: + _, r = _compute_canonical_verdict( + [], [], rejected=[{}, {"rejection_class": None}], + ) + assert "2 finding(s)" in r + assert "unclassified" in r + + def test_reason_stays_within_the_envelope_cap(self) -> None: + rejected = [ + {"rejection_class": f"class_{i}_with_a_very_long_name"} + for i in range(40) + ] + _, r = _compute_canonical_verdict([], [], rejected=rejected) + assert len(r) <= _VERDICT_REASON_MAX + + def test_a_blocking_finding_still_outranks_the_rejection_note(self) -> None: + v, r = _compute_canonical_verdict( + [{"severity": "BLOCKING", "issue": "rce"}], [], + rejected=[{"rejection_class": "evidence_not_found"}], + ) + assert v == "CHANGES_REQUESTED" + assert "rce" in r + + class TestLoadVerdictThresholds: """Operator-overridable verdict thresholds. After the Premise gate was retired, ``impact_cumulative`` (FEA-1401 Rule 6) is the sole tunable."""