fix(review): stop null-hostile immutable collections from failing a review - #472
Open
devops-thiago wants to merge 1 commit into
Open
fix(review): stop null-hostile immutable collections from failing a review#472devops-thiago wants to merge 1 commit into
devops-thiago wants to merge 1 commit into
Conversation
…eview Two spots in the review path fed a possibly-null value into an immutable collection that rejects null. Both run after the 200 ack on the async review thread, so the failure was not a bad review but no review, with nothing on the PR explaining why. A persisted session row whose AI response body is the JSON literal "null" is syntactically valid, so Jackson returned Java null without throwing and parseResponse never reached its parse-failure fallback. The null element then went into List.copyOf, which rejects it, permanently failing every later review of that PR until the row was edited. Unusable prior state now degrades to the empty response, which is what the catch right above it already promised. FileDiff.filename() is not validated at construction — the record deliberately tolerates nulls, since patch is null for binary files and previousFilename is null for non-renames — but the omitted/clipped lookups asked immutable sets, whose contains(null) throws instead of answering false. ReviewDiffFormatter.IgnoreGlobs.matches already guards exactly that input, so the two sites disagreed about whether a null name was possible; that disagreement was the defect. The contract now lives in one place, ReviewDiffFormatter.namesContain, and says a file with no name is simply not in the set. VerdictBuilder's walkthrough filter was a third instance of the same lookup and moves onto it too. The directory breakdown that builds the scope header runs ahead of the per-file rows and dereferenced the same unnamed file first, so its null guard is part of the same fix rather than a separate one; its comment claimed the loop below it made a null unreachable, which had the ordering backwards. Refs #471
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
🤖 ThrillhouseBot PR SummaryWhat this PR doesThis change fixes two independent bug sources where null values could cause NullPointerException in immutable collections on the async review thread after the webhook ack. It adds null guards: in FollowUpAnalyzer for a JSON-literal null response body, and in ReviewDiffFormatter with a new namesContain method to safely check set membership of file names, replacing direct Set.contains(null) calls in FindingPipeline and VerdictBuilder. The directoryOf helper in FindingPipeline is also guarded against null filename. Changes Overview
Changed Files
Risk Assessment
No new issues found in this PR, but the review cannot be approved until CI is confirmed green.
|
| Check | Type | Status | Detail |
|---|---|---|---|
| trivy | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| frontend | check-run | ⏳ Pending | - |
| changes | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| actionlint | check-run | ⏳ Pending | - |
| dependency-review | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
thrillhousebot
Bot
added
bug
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
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
Two spots in the review path fed a possibly-
nullvalue into an immutable collection that rejectsnull. Both run after the 200 ack, on the async review thread, so the failure was not a bad review — it was no review, with nothing surfaced on the PR explaining why.1. A persisted response body of the JSON literal
null.FollowUpAnalyzer.parseResponseguarded the Java null and the blank string and caughtJsonProcessingException, but"null"is syntactically valid JSON: Jackson returns Javanulland throws nothing, so the parse-failure fallback was never reached. The caller put that null straight intoList.copyOf, which rejects null elements — a stored session row containing those four characters failed every subsequent review of that PR, permanently, until the row was edited. It now degrades toEMPTY_RESPONSE, which is exactly what thecatchdirectly above it already promised for unreadable prior state.2.
Set.copyOf(...).contains(<possibly-null filename>).GitHubPullRequestClient.FileDiffis a Jackson-deserialized record with no compact constructor validatingfilename, and it deliberately tolerates nulls in its siblings (patchis null for binary files,previousFilenamefor non-renames). MeanwhileReviewDiffFormatter.IgnoreGlobs.matchesopens with an explicitfilename == nullguard. The two disagreed about whether a null filename was possible, and that disagreement was the actual defect.This takes the side the codebase already established — a null filename is tolerated and means "not matched" — rather than adding validation to
FileDiff. Validating at construction is the other reading, but it converts a silent null into a hard failure at parse time for every file the API returns, which is a much wider behavior change than the defect calls for. The contract now lives in one place,ReviewDiffFormatter.namesContain, next to the glob matcher that already stated it.Three call sites move onto it:
FindingPipeline.changedFilesOverviewomitted.contains(...)— reported in the issueFindingPipeline.changedFilesOverviewclipped.contains(...)— reported in the issueVerdictBuilder.buildSet.copyOf(truncation.omittedFileNames()).contains(...)— a third instance, not in the issue, in the same review path and equally null-hostileTwo things worth flagging that the issue does not mention:
plan.omittedFiles()/plan.clippedFiles()cannot themselves contain null.DiffBudgetPlanner.BudgetPlan's compact constructor already runsList.copyOfon both, soSet.copyOf(...)inFindingPipelinecan only ever fail on thecontains(null)argument, never on the collection's contents. No extra guard is needed there.directoryOfneeded the same guard. The directory breakdown that builds the scope header runs ahead of the per-file rows, so it dereferenced the unnamed file first and thecontainsguard alone would have been unreachable. Its comment claimed "a null path is not guarded here: the per-file loop above already dereferences the same name against an immutable set" — that had the ordering backwards. A file with no path carries no directory component either, so it lands in the existing(repository root)bucket.Related Issues
Fixes #471
How Has This Been Tested?
Four tests, each validated red/green by neutralizing only the production behavior it covers (surgically, one at a time — a blanket revert would have produced compile errors, which prove nothing) and confirming the failure, then restoring and confirming green.
1.
FollowUpAnalyzerTest.persistedResponseOfTheJsonLiteralNullDegradesToTheEmptyResponseMutation:
parseResponsereturnsmapper.readValue(...)directly again, without the null check.That is the exact
List.copyOfline named in the issue. The test also pinspreviousFindingFilesById("null"), which reads the same response and would otherwise dereference the null.2.
ReviewDiffFormatterTest.GlobMatching.shouldTreatANullFilenameAsAbsentFromAnImmutableNameSetMutation:
namesContaindrops itsfilename != null &&and delegates straight tonames.contains(filename).Covers all three arms: null, present, and absent-but-named.
3.
FindingPipelineTest.anUnnamedReviewableFileIsNeitherOmittedNorClippedInTheSummaryOverviewSame mutation as (2), driven through the real multi-call pipeline:
The same test also pins the
directoryOfguard. Mutation: restorepath.lastIndexOf('/')without the null check.Those two stack traces are the concrete evidence for the ordering claim above: the breakdown is reached first.
4.
VerdictBuilderTest.anUnnamedFileIsNotTreatedAsOmittedFromTheWalkthroughRows— the third instance.Same mutation as (2):
It asserts the named omitted file is dropped from the walkthrough rows while the unnamed one keeps its row — the null case and the ordinary case in the same assertion.
Build gates:
Checklist
Screenshots / Logs
N/A — see the red-phase stack traces above.
Additional Notes
No config keys, no schema changes, no user-visible behavior change on today's inputs: GitHub always sends a filename, and no production session row holds a literal
null. Every path this touches is the one that previously threw.Left for a follow-up rather than folded in.
DiffBudgetPlannerhas two more sites that a null filename would still break, both upstream of the code changed here, and both needing a semantic decision this defect does not license:renderAndSizesorts with.thenComparing(FileDiff::filename), which throws once two files tie onadditions + deletions. Deciding where an unnamed file sorts is a design call, not a null guard.omitted.add(s.file().filename())feedsBudgetPlan'sList.copyOf, which rejects the null. Filtering it out would silently shrinkomittedFiles().size()— the count that holds APPROVE inVerdictBuilder— so an unreviewed file could stop withholding approval. That trade needs its own issue.Neither blocks this fix: a review can reach the sites changed here without passing through either (the sort does no comparisons on a single-file batch, and a file that fits its budget is never added to
omitted). Happy to file them if you'd like.