-
Notifications
You must be signed in to change notification settings - Fork 10
fix(code-review): surface discarded findings; stop treating absence as disproof (ISS-10711) #203
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the bullet I'd push on. Bullets 2 and 3 are tightly scoped, but "cited file does not exist under review_root" is also the exact signature of the most common reviewer hallucination: an agent inventing a plausible path it never read. TENTATIVE lands the finding in verified[] via the bucketing at code_review_helpers.py:3850, and Rule 3.5 turns any TENTATIVE into NEEDS_ATTENTION for the whole run. So a single fabricated path now reds the review instead of being dismissed. The PR body says a blanket downgrade would let hallucinated findings survive as TENTATIVE, but that is what this bullet does for the hallucinated-path class specifically. _require_review_root does not help here either, since it only proves the root holds the files the diff changed, and a hallucinated path is by definition not one of those. False-red is the safer direction so I'm not holding the PR, but if you see NEEDS_ATTENTION noise from this, the split you want is probably "missing path is also not in the patch at all" to REJECTED, keeping TENTATIVE for the rename/drift case. |
||
| 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. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SCHEMA.md:278 still says "verdict_reason cites the specific finding(s) that produced the verdict." After this, on APPROVED it cites the findings that specifically did not. That doc line is the contract a downstream reader parses against, so it needs a sentence for the APPROVED case in this PR. |
||
|
|
||
|
|
||
| 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. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No CHANGELOG.md in the branch. CI only enforces the version bump, so this passes green, but .githooks/pre-push blocks exactly this and CLAUDE.md says run /update-documentation before pushing. Looks like core.hooksPath isn't set locally. Worth a run before merge so the release notes aren't reconstructed later.