fix(review): publish one observation on one surface of the summary comment - #596
Open
devops-thiago wants to merge 5 commits into
Open
fix(review): publish one observation on one surface of the summary comment#596devops-thiago wants to merge 5 commits into
devops-thiago wants to merge 5 commits into
Conversation
…mment The same claim was being asserted as its own item on up to three surfaces of one review comment — an inline finding, a "Description vs. Implementation" bullet, and a Changed Files walkthrough clause — with nothing linking the copies. Readers infer severity from repetition, so a low-value note raised three times outranked the severest finding in the same comment. The renderer already holds everything it is about to publish, so it now collapses each claim to its most specific surface before rendering: inline finding > description-gap bullet > walkthrough row. A walkthrough row keeps its first clause always — a row summarising a file that also carries an inline finding is normal and useful — and drops only the clauses appended after it that restate an already-published claim. Two texts state the same claim when they share a contiguous run of three content words, or when their content words overlap by half of the shorter side (which needs five content words to mean anything). Under-firing is the safe direction: a missed duplicate is the status quo, a false collapse deletes a claim. Fixes #588
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! |
🤖 ThrillhouseBot PR SummaryWhat this PR doesAdds a SummarySurfaceDeduplicator pass to PrSummaryGenerator that collapses a claim repeated across inline findings, description-gap bullets, and changed-file walkthrough clauses onto its most specific surface, preserving the first clause of each walkthrough row. The change includes corpus-derived regression tests from ThrillhouseBot-test #22 and #23.
|
| File | Change | Summary |
|---|---|---|
src/main/java/dev/thiagogonzaga/thrillhousebot/review/PrSummaryGenerator.java |
Modified | Wires the deduplicator into summary generation before rendering description gaps and the Changed Files table. |
src/main/java/dev/thiagogonzaga/thrillhousebot/review/SummarySurfaceDeduplicator.java |
Added | Adds a token-overlap dedupe pass that collapses repeated observations across inline findings, description gaps and walkthrough clauses, preserving the first clause of each row. |
src/test/java/dev/thiagogonzaga/thrillhousebot/review/PrSummaryGeneratorTest.java |
Modified | Adds corpus-derived regression tests for the #588 duplicate-surface scenarios. |
src/test/java/dev/thiagogonzaga/thrillhousebot/review/SummarySurfaceDeduplicatorTest.java |
Added | Unit tests for gap/gap and finding/gap collapse, clause trimming, tokenizer and stemmer behavior. |
Risk Assessment
| Risk | Count |
|---|---|
| 🔴 Critical | 0 |
| 🟠 High | 0 |
| 🟡 Medium | 1 |
| 🔵 Low | 0 |
Key Findings
- MEDIUM: Negation words dropped as stop words can collapse opposite claims (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/SummarySurfaceDeduplicator.java:69)
⚠️ CI Checks Status
Some checks are still pending or have failed:
| Check | Type | Status | Detail |
|---|---|---|---|
| dependency-review | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
thrillhousebot
Bot
added
bug
A negator is a single token, so it cannot move a similarity score: "no", "not" and "does" were stop words, which made "unvalidated client source does not reach fopen path" tokenize identically to the finding "unvalidated client source reaches fopen path", and the gap was deleted as a duplicate of the claim it contradicts. Dropping the negators from the stop list alone would not have helped — the overlap coefficient normalises by the shorter side, so one extra token on the longer side costs nothing. Polarity is now tracked apart from content. A text reduces to its content words plus whether it negates, and two texts that disagree on polarity while naming nothing the other leaves out are treated as a contradiction to surface, never a duplicate to delete. Polarity is only decisive for that otherwise-identical pair: texts that also differ in substance are still judged on their content, so a gap quoting the PR's affirmative promise still collapses onto the finding reporting the absence.
🤖 ThrillhouseBot — changes since the last review
|
Content words are split on non-alphanumeric runs, so "doesn't" tore into
"doesn" and "t" — neither a negator. A contracted negation therefore read as
affirmative, and its opposite could be deleted as a duplicate: the gap "the
client doesn't validate the registry URL before use" scored 1.0 against "the
registry URL is validated before use" and the negative conclusion was the one
dropped, leaving the reader the opposite of what was concluded.
Contractions are now rewritten to a bare "not" before the split, with the
irregular stems of "can't", "won't", "shan't" and "ain't" folded into the
pattern so they leave no stray content word behind, and the auxiliaries a
rewrite exposes ("could", "did", "should") join the stop words already
covering "do", "does" and "would".
The class javadoc also overclaimed: it said two texts are judged the same
claim "only when they agree on polarity", while polarity holds a pair back
only when neither side names anything the other leaves out. The behaviour is
deliberate and pinned by a test, so the documentation is corrected to match.
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- MEDIUM: fix does not change behavior for the stated trigger (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/PrSummaryGenerator.java:150)
The PR description (and 'Fixes #586') describes fixing the flaky ChangelogEntryGeneratorTest.discloseTheShortfallWhenOneBatchFails by padding fixture patches (PAD_LINES ~290 tokens), raising the requested diff room (ROOM_FOR_ONE_FILE = 400) and adding batchingFixturesLeaveMarginWiderThanTheFenceJitter. None of that exists in this diff: no changelog test file is changed, and the only production changes are the new SummarySurfaceDeduplicator and its wiring into PrSummaryGenerator.generate() at this line. Per the author's own analysis, the /changelog path never enters PrSummaryGenerator (ChangelogEntryGenerator.draftEachBatch is a separate sequential loop), so no changed line executes under the stated failure trigger and the #586 flake is not fixed by this PR as submitted. The description's 'No production file is touched' is also contradicted: two src/main files are added/modified. If this PR is actually the #588 dedupe change, the description needs rewriting; the #586 fix is not present.
Both javadocs said the override fires when "neither names anything the other leaves out", which reads as mutual containment, while contradicts() fires on containment in either direction. The one-directional behaviour is the correct one — a short negated claim lying wholly inside a longer affirmative one is exactly the contradiction that must be surfaced, and tightening to mutual containment would delete the shorter copy — so the documentation is corrected to match, and now says why that pair is the one polarity has to catch.
🤖 ThrillhouseBot — changes since the last review
|
The javadoc claimed the stemmer makes "the plural, past and third-person forms of one word collide", which is broader than the code: stripping trailing s/e/d leaves "verifies" as "verifi" while "verify" keeps its y, so the two never meet, and the same holds for modifies, identifies and applies. The comment now states the rule it implements — strip trailing s, e and d down to a three-character floor, which catches the -s, -es and -ed forms and the silent -e behind them — and records that stem-rewriting inflections are left uncaught on purpose. Missing one costs a collapse those texts might have earned, which is the under-firing direction this class prefers; a wider stem would merge more words and risk deleting a claim.
|
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check test is pending
- Check actionlint is pending
- Check changes is pending
- Check trivy is pending
- Check frontend is pending
- Check format is pending
- Check dependency-review is pending
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
One observation was being published on up to three surfaces of the same summary comment — as an
inline finding, again as a "Description vs. Implementation" bullet, and again as its own clause in a
Changed Files walkthrough row — with nothing tying the copies together. Readers infer severity from
repetition, so a low-value note raised three times outranked the severest finding in the same
comment (on ThrillhouseBot-test #23 it outranked a SQL injection), and the duplicate emissions
inflated the apparent finding count.
The renderer already holds the full set it is about to publish across all three surfaces, so it now
collapses each claim to its most specific surface before rendering anything:
finding is dropped; so is a second gap that only rephrases an earlier one. All findings are
considered, not just the five that reach "Key Findings" — on ThrillhouseBot-test feat(db): adopt Flyway versioned migrations and disable prod schema auto-DDL #22 the duplicated
claim was 8th by severity and never appeared in that list.
normal and useful, so the row's first clause is never touched; only the clauses appended after it
that restate an already-published claim are removed. A row therefore always keeps a real
description of its file, and a single-clause note is returned untouched.
Two texts state the same claim when they share a contiguous run of three content words, or when
their content words overlap by half of the shorter side. Polarity does not gate that test in
general — it holds a pair back only when the two say the same things with opposite polarity, one
negating and every content word of one side present in the other (containment in either direction).
Such a pair scores as a perfect match while asserting opposite things, because a negator contributes
a single token and no similarity score can separate "the value is sanitized" from "the value is not
sanitized"; that is a contradiction to surface, never a duplicate to delete. Two texts that disagree
on polarity but each name something the other leaves out are still judged on their content, so a gap
quoting the PR's affirmative promise still collapses onto the finding reporting the absence.
Contracted negations ("isn't", "won't") are rewritten before tokenizing, since content words are
split on non-alphanumeric runs and would otherwise tear "isn't" into "isn" and "t". The overlap arm
requires five content words on the shorter side — below that the coefficient is noise, and keeping
both copies is the safe direction.
Under-firing is deliberately the safe direction throughout: a missed duplicate is the status quo,
while a false collapse silently deletes a claim the model made.
The regression material is the corpus itself — the fixtures in the new tests are the verbatim text
ThrillhouseBot published on devops-thiago/ThrillhouseBot-test #23 (Java) and #22 (C).
Related Issues
Fixes #588
How Has This Been Tested?
Red/green proof. With the renderer change reverted (
SummarySurfaceDeduplicatorremoved,PrSummaryGeneratorrestored), the new tests fail exactly as claimed — the flagship case shows thesame claim on all three surfaces at once:
With the fix in place all three pass, and the full suite is green.
Gates:
./mvnw -B spotless:applythen./mvnw -B clean compile spotbugs:check spotless:check—BugInstance size is 0, BUILD SUCCESS../mvnw -B clean test— BUILD SUCCESS, no failures, no errors.git diff -U0 fda4bc7...HEADshows zero uncovered lines and zero uncoveredbranches across the changed main code.
Checklist
Additional Notes
The dedupe pass is deliberately conservative. Two known duplicates in the corpus survive it: a
walkthrough clause of only four content words ("query is vulnerable to SQL injection" against the
finding "SQL injection in task-name search query") falls under the minimum-token guard, and a
clause phrased entirely differently from its finding ("degraded flag is true whenever any dispatch
was attempted") scores below the overlap threshold. Both are the tolerable failure direction:
publishing a claim twice is the current behaviour, while a wrong collapse would delete it.
Review follow-up (da65366): the bot's own review of this PR caught a false-collapse route through
the stop list — "no"/"not"/"does" were dropped as function words, so a negated paraphrase tokenized
identically to the finding it contradicts and was deleted. Fixed by tracking polarity separately,
with three regression tests; see the resolved thread for the red output and why removing the
negators from the stop list alone would not have been sufficient.
Review follow-up (fdd9602): the re-review found the same false-collapse still reachable through
contracted negations — content words split on non-alphanumeric runs, so "doesn't" tore into "doesn"
and "t" and the sentence read as affirmative. Contractions are now rewritten to a bare "not" before
the split, irregular stems included, so no stray content word is left behind. The same review caught
the class javadoc overclaiming that collapsing requires polarity agreement; the behaviour is
deliberate and test-pinned, so the documentation was corrected rather than the code.
Review follow-up (this round): a third review pass found both javadocs still describing the polarity
override as mutual containment ("neither names anything the other leaves out") while the code fires
on containment in either direction. The one-directional behaviour is correct — a short negated claim
lying wholly inside a longer affirmative one is exactly the contradiction that must be surfaced, and
tightening to mutual containment would reintroduce the silent deletion — so both javadocs were
corrected to state the condition the code implements, documentation only.
Review follow-up (6bcb075): a fourth pass found the
stem()javadoc claiming it makes "the plural,past and third-person forms of one word collide", which overstates it — stripping trailing s/e/d
leaves "verifies" as "verifi" while "verify" keeps its y, so y→i inflections never meet. The
stemmer is unchanged: missing a y→i collapse lets a duplicate survive, the under-firing direction
this class prefers, while widening it would merge more words and risk deleting a claim. The comment
now states the rule it implements and names the uncaught case, documentation only.