fix(review): count only the verdicts the audit acted on as verification coverage - #719
Merged
Merged
Conversation
…on coverage #623's disclosure reads coverage off the candidate count and the number of candidates that received a verdict, and every degradation lane records it — including the two that salvage a body cut mid-JSON (#546/#617). The count asked only whether some salvaged verdict carried the candidate's id, so a verdict the audit could not act on — no decision label, a blank one, or a word this service does not recognize — counted as screened while apply() left its finding posting exactly as the reviewer raised it. A cut whose salvage recovered such an element therefore reported FULL coverage, and every surface stayed silent about a published set that mixed verified and unverified findings and read as fully screened — the harm #623 exists to prevent, on the lane most likely to hit it. Coverage now follows what the audit acted on: apply()'s switch and the count read the decision through one normalizer, so what the review DOES with a verdict and what it CLAIMS it verified cannot drift. Undercounting is the safe direction — it costs an over-cautious clause on a finding ruled on in words this service cannot read, while the silence it replaces tells the reader nothing is outstanding. Both edges a cut can land on were measured and are pinned in the javadoc and in tests. Nothing decidable salvaged reports (N, 0) — NONE, the same disclosure the empty-body path renders, since the findings are in the same state. A cut whose salvage covers every candidate stays genuinely FULL: a body cut after the verdicts array closed (Jackson's "expected close marker for Object" against the root) carries a complete, applied verdict for every finding, so disclosing a gap there would tell the reader a fully screened set was not screened. The cut itself is logged either way.
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Contributor
🤖 ThrillhouseBot PR SummaryWhat this PR doesVerification coverage now records only candidates whose salvaged verdict carries a decision the audit applies (confirmed, downgraded, or rejected), routing both the coverage count and apply() through a shared decisionOf() normalizer. Added tests pin the parse-failure and length-cap cut lanes as well as the zero- and full-salvage edges, while fail-open review application is unchanged. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
A["Verifier response body"] --> B{"Body cut or length-capped?"}
B -- yes --> C["salvageTruncatedVerdicts()"]
B -- no --> D["apply()"]
C --> E["candidatesCovered()"]
D --> F["decisionOf(verdict)"]
E --> F
F --> G{"Label in ACTED_ON_DECISIONS?"}
G -- yes --> H["Audit acts and id counts toward verified"]
G -- no --> I["Fail-open keeps original; id not counted"]
H --> J["Post findings and VerificationCoverage outcome"]
I --> J
Changes Overview
Changed Files
Risk Assessment
No new issues found in this PR, but the review cannot be approved until required CI is confirmed green.
|
| Check | Type | Status | Detail |
|---|---|---|---|
| test | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| dependency-review | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
thrillhousebot
Bot
added
bug
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
Description
FindingVerificationServicereports how much of the candidate set the second-pass auditcovered, and #623 turns a non-FULL coverage into a banner, a coverage clause and a check-run
brief. The
verifiedhalf of that record asked only whether some salvaged verdict carriedthe candidate's id.
That is not the same question as "was this finding screened".
apply()acts on three decisions—
confirmed,downgraded,rejected— and everything else (noverdictfield, a blank one,or a label this service does not recognize) lands in its fail-open default, where the finding
posts exactly as the reviewer raised it, indistinguishable from a candidate with no verdict at
all. The coverage count called those screened anyway, so a cut whose salvage recovered such an
element reported FULL coverage and every surface stayed silent about a published set that
mixed verified and unverified findings and read as fully screened.
The fix routes both
apply()'s switch and the coverage count through one decision normalizer,so what the review does with a verdict and what it claims it verified cannot drift.
Undercounting is the safe direction: it costs an over-cautious clause on a finding the audit
ruled on in words we cannot read, while the silence it replaces is exactly the harm #623 exists
to prevent.
On the two cut edges the issue asks about — both were measured against the real code, not
assumed, and are now pinned in the javadoc and in tests:
(N, 0)→NONEon both cut lanes; unchanged. The new siblingcase — elements salvaged but none of them decidable — now reports
(N, 0)too, since thefindings are in the state the empty-body path leaves them in.
FULL, deliberately. A body cut after the verdicts arrayclosed raises Jackson's
Unexpected end-of-input: expected close marker for Objectagainstthe root object, and every verdict inside it is complete and applied; what the cut
destroyed is the body's tail. Disclosing a gap there would tell the reader a fully screened
set was not screened. The cut is still logged for the operator either way.
Measurement note for the reported symptom: a partial salvage was already reported as
PARTIALon both cut lanes before this change (
(3, 2),(2, 1)— pinned by existing tests andre-confirmed by probe). The only way the cut lanes could read
FULLwas a salvage covering everycandidate — either genuinely complete verdicts (correct, and kept), or the undecidable-verdict
drift this PR closes. The log line quoted in the issue is the root-object shape: Jackson's message
is multi-line, so
salvaged N verdict(s) covering M of K candidate finding(s)sits on a laterline of the same record, and the absence of
Finding verification: N kept, M downgraded, K rejectedonly means nothing was rejected or downgraded —apply()logs that line only when itchanged something.
Fail-open is untouched: coverage changes what the review says, never which findings it keeps.
Related Issues
Closes #710
How Has This Been Tested?
Red/green, with the new tests run against the unfixed service first:
The fifth (
reportsFullCoverageWhenACutSalvagedEveryCandidatesVerdict) is characterization, notred/green: it pins the all-salvaged edge so it cannot drift into a false alarm.
Two of the new tests drive a real cut end to end rather than the record type — one through the
parse-failure lane (
STOP, body cut after the array closed) and one through the reportedfinish_reason=lengthlane, which unwinds viaAiResponseTruncatedExceptionintosalvageTruncatedVerdicts.Downstream disclosure was verified rather than assumed:
VerdictBuilderTest(
partialVerificationCoverageDisclosesTheHonestXOfY) already proves a recorded(4, 3)rendersonly covered 3 of the 4 finding(s)in the summary andVerification covered 3 of 4 finding(s); the rest posted unverified.in the check-run brief.Gates, all green:
Patch coverage from
target/site/jacoco/jacoco.xmlintersected withgit diff -U0 origin/main --:7 instrumented changed main lines, 0 uncovered lines and 0 uncovered branches (the
decisionOfternary reports 4/4 branches, the reworkedapply()switch 3/3).Checklist
Screenshots / Logs
None.
Additional Notes
No behaviour change to the fail-open contract, to #617's salvaging of the verdicts that closed
before a cut, or to the verdict application itself — only to what the coverage record counts as
a screened candidate.