fix(review): collapse duplicate verdict ids the same way in both readers - #739
Merged
Conversation
#719 routed apply() and the coverage count through one decision normalizer so what the review does with a verdict and what it claims it verified cannot drift. The normalizer was shared; the duplicate-id resolution was not. apply() collapsed duplicates first — first element for an id wins — and read the label from the survivor, while candidatesCovered filtered by label first and de-duplicated the ids that were left. An id carrying an undecidable element followed by a decidable one therefore landed in apply()'s fail-open default, where the finding posts exactly as the reviewer raised it, while the count called that candidate screened. Every other candidate decided, that is FULL coverage on an unscreened finding: no banner, no coverage clause, no check-run brief, and no "N kept, M downgraded, K rejected" line either when nothing else was rejected or downgraded. That is the harm #623 exists to prevent and #710 was filed on, reached by a narrower route — on a complete, well-formed body, not only on a cut one. The drift is one-directional: the reverse ordering already agreed, so it only ever over-counted, exclusively toward the dangerous side. Both readers now resolve duplicates through one collapse, so the verdict the count reads for an id is exactly the one the audit acted on. Duplicates are not a hypothetical input shape: the collapse predates this fix precisely because the model emits them. decisionOf now strips, so the strictness genuinely matches the strictRisk and strictConfidence it documents itself against: "rejected " was unreadable as a decision while "high " was a readable rating. Both readers take that same value, so it never drove a drift — it cost an over-cautious keep and an under-count — but the asymmetry was unintended. Fixes #735
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 doesFixes #735 by making apply() and the coverage count resolve duplicate verdict ids through the same first-wins collapse (byCandidateId), so an id whose first verdict is undecidable is no longer silently counted as screened — coverage drops from FULL to PARTIAL on that path. Also strips whitespace from decision labels in decisionOf so padded verdicts like "rejected\n" are read as decisions consistently with the strictness documented for ACTED_ON_DECISIONS, and adds three tests covering the defect, a reverse-order control, and the padding case. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
A["verification response"] --> B["byCandidateId(): collapse duplicate ids, first wins"]
B --> C["apply(): act on first-wins survivor per finding id"]
B --> D["candidatesCovered(): first-wins survivor per id"]
C --> E["keep / reject / downgrade / fail-open"]
D --> F{"id within 1..candidates?"}
F -->|"yes"| G{"decisionOf(survivor) in ACTED_ON_DECISIONS?"}
G -->|"yes"| H["count id as screened"]
G -->|"no"| I["leave id unverified"]
H --> J["VerificationCoverage(candidates, verified)"]
I --> J
J --> K["FULL hides disclosure; PARTIAL discloses (#735)"]
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 | - |
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
#719 routed
apply()and the coverage count through one decision normalizer so that "what the review does with a verdict and what it claims it verified cannot drift". The normalizer was shared; the duplicate-id resolution was not.apply()collapsed duplicates first —putIfAbsent, so the first element for an id wins — and read the label off the survivor.candidatesCoveredfiltered by label first and de-duplicated the surviving ids afterwards.For an id carrying an undecidable element followed by a decidable one the two therefore read different verdicts for the same id.
apply()lands in its fail-open default and the finding posts exactly as the reviewer raised it; the count calls that candidate screened. With every other candidate decided that isVerificationCoverage(N, N)→FULL→disclosed() == false: no banner, no coverage clause, no check-run brief. And because nothing was rejected or downgraded on that path,apply()short-circuits before theN kept, M downgraded, K rejectedline, so the operator log says nothing either. A published set containing a finding the audit never ruled on, reported as fully screened — the harm #623 exists to prevent and #710 was filed on, reached by a narrower route and on a complete, well-formed body, not only a cut one.Two properties worth stating:
apply(). So this only ever over-counted — exclusively toward the dangerous side.putIfAbsentcollapse predates this fix precisely because the model emits them; the whole salvage/fail-open stack exists because this body is unreliable model output.Both readers now resolve duplicates through one collapse (
byCandidateId), so the verdict the count reads for an id is exactly the one the audit acted on. Nothing else about the count changes: the id-range filter andACTED_ON_DECISIONSare untouched, and the fail-open contract is untouched — coverage changes what the review says, never which findings it keeps.Also in this PR:
decisionOfnow strips. Its siblingsstrictRisk/strictConfidenceboth dovalue.strip().toLowerCase(...), and the javadoc onACTED_ON_DECISIONSclaims the strictness matches them — it was stricter, so"rejected "was unreadable as a decision while"high "was a readable rating. Both readers take that same value, so this never drove a drift; it cost an over-cautious keep plus an under-count. The asymmetry was unintended, so it is closed here.Related Issues
Fixes #735
How Has This Been Tested?
Three tests added to
FindingVerificationServiceTest:doesNotCountADuplicateIdWhoseFirstVerdictTheAuditCannotRead— the defect itself: a complete, parseable body with two verdicts for id 1, the first undecidable. Asserts the finding is kept and the coverage is(2, 1)/PARTIAL/ disclosed.readsADecisionLabelPaddedWithWhitespace—"rejected\n"is acted on and" CONFIRMED "is counted.countsADuplicateIdWhoseFirstVerdictTheAuditActedOn— the control, the same duplicate in the other order. It passes both before and after the change (that is its point: the collapse must not turn the pre-existing agreement into an under-count), so it is not offered as red/green proof.Red/green proof
The two behavioural tests run against the unfixed
FindingVerificationService, verbatim:The first failure is the drift exactly as claimed: the coverage counted 2 of 2 while
apply()had ruled on 1. The second is the unstripped label: the paddedrejectedwas not acted on, so both findings survived. With the fix,FindingVerificationServiceTestruns 109 tests, 0 failures.Gates
spotless:apply→clean compile spotbugs:check spotless:checkclean testgit diff -U0 aa3c556...HEAD(main)mi=0,mb=0— zero uncovered lines, zero uncovered branchesChecklist
Additional Notes
No configuration change, no behaviour change to which findings are published — only to what the review claims it screened, and in the safe direction: a candidate the audit could not rule on now reads as unverified instead of silently counting as screened.
VerificationCoverage's compact constructor already clamped the over-count tocandidates, so no invalid record was ever produced; what the over-count did was turn PARTIAL into FULL, or shrink a real gap.The suggested patch on the issue rebuilt a second collapse inside
candidatesCovered, including a null-element guard. Verified against the real code and adjusted: the collapse is factored into one private helper both readers call, rather than duplicated, and the null guard is deliberately omitted —VerificationResponse's compact constructor andTruncatedResponseSalvager.salvageArrayboth go throughList.copyOf, which rejects null elements outright, so a null verdict cannot reach either reader and a guard would be dead, untestable code.