fix(code-review): surface discarded findings; stop treating absence as disproof (ISS-10711)

[cchenault4]

Closes the two ISS-10711 remedies that the cause fix did not cover.

Context: the cause is already fixed on main

ISS-10711 was filed against a machine pinned to plugin 3.1.0, where review_root was populated only for scope_kind == "pr" and both agent prompts told the fleet to fall back to the current working directory when it was empty. A pre-PR branch-scope review therefore ran its whole verifier fleet against the invoking checkout: eight verifiers, seven rejecting real findings at 0.95 confidence, and an APPROVED on a change carrying two P1 defects.

None of that is true of main. review_root is now always populated (worktree_path or _git_toplevel()), _require_review_root refuses a root it cannot prove holds the diff, and the cwd fallback is gone from both prompts.

This PR is only the part that survived that fix. Two remedies from the ticket were verifier-stage and rollup-stage, independent of where source is read, and both were still reachable on main.

1. An APPROVED must name what verification discarded

_compute_canonical_verdict ended in return "APPROVED", "". 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 — which is precisely how ISS-10711 went unnoticed until a lane distrusted it and re-read the findings by hand.

The verdict now carries e.g. 7 finding(s) rejected by verification (evidence_not_found x7).

Gating is deliberately unchanged. A rejected finding is by definition not gating, and every input returns the verdict it returned before; only the reason string differs. The reason is composed to fit _VERDICT_REASON_MAX rather than truncated to it, because a reason chopped mid-class reads as a different class — the count is the load-bearing half and survives when classes have to be dropped.

2. "Could not locate it" and "is not there" are different claims

The EXISTENCE CHECK collapsed every form of absence into REJECTED / evidence_not_found. It now splits three ways:

• cited file missing under review_root → TENTATIVE. Equally well explained by a rename, a bad path, or a tree that is not the one this diff describes.
• snippet present in the patch hunks as an ADDED line but absent from review_root → TENTATIVE, reported as a scope or checkout fault in the run rather than evidence against the finding. This is ISS-10711's fingerprint, and it would have caught that run even with the old review_root bug fully intact.
• file exists, was read, and neither the snippet nor any snippet_hash matches, and it is not an added line in the patch → REJECTED. This is now the only branch carrying positive disconfirmation.

Narrow on purpose. A blanket downgrade would let every hallucinated finding survive as TENTATIVE, and TENTATIVE at MEDIUM promotes to NEEDS_ATTENTION — trading a false green for a false red across every review.

No code-side detector was added for the same condition: _require_review_root already refuses a root missing or drifted on any file the diff changes, so a mechanical check would be redundant.

Test plan

• pytest plugins/code-review/ → 1405 passed, 3 skipped, 0 failed.
• ruff check on both touched Python files → 210 findings before, 210 after; none introduced.
• 8 tests added, including an end-to-end assertion through cmd_finalize_result. The unit tests alone cannot catch a call site that forgets to pass rejected, and that wiring is the fix.
• Counterfactuals run, both directions: reverting the reason to "" kills 4 unit tests; dropping the rejected=rejected argument at the call site kills the end-to-end test. The 3 unit tests that survive the first mutation are the ones asserting unchanged behaviour, which is correct.
• The end-to-end fixture asserts validation_errors == [], so it cannot hide a real envelope regression in its own schema noise.

Editing verifier_prompt.txt changes verifier_prompt_hash, which correctly invalidates verdicts cached under the old ladder. test_verifier_prompt_changes_hash already pins that property.

Not covered

• Remedy 4 from the ticket — resolve-scope comparing against a local ref rather than origin/<base>, which silently widened one run's scope by 29 files of other people's work. Distinct trigger from the review_root cause; not verified against 3.7.0 and should not be assumed fixed alongside it.
• ISS-10638 (resolve-scope prepending origin/ to a SHA base) remains its own ticket.

🤖 Generated with Claude Code

https://claude.ai/code/session_017hHB3pezXXMQdJZUdTjJ95

[mikeangstadt]

Good fix, and the reason-composition-not-truncation detail is the right call. Ran the 8 new tests, they pass. One real question on the first prompt bullet and two contract-hygiene things inline, none blocking.

[mikeangstadt]

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.

[mikeangstadt]

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.

[mikeangstadt]

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.