fix(review): disclose overturned declines on every body and name the findings a round closed - #720
Merged
Merged
Conversation
The reopened-decline note rendered only from the no-new-findings body, so a round that also raised a finding took the with-findings path and dropped it. That made two very different outcomes indistinguishable from the PR: the model never reading a reply as a decline and reporting the finding unresolved, versus the model accepting the decline and the deterministic re-check overturning it on diff evidence. Both showed as the bare "N previous finding(s) remain unresolved", so a maintainer who wrote a considered decline saw no acknowledgement it was read, no contradicting evidence, and no way to tell whether to argue or to fix — the same undisclosed-decision harm as #542/#598. The note now rides both with-findings branches: the un-anchored/cap-skipped fallback body and the ordinary one, placed above the partial-coverage banner so the coverage caveat stays outermost. Each note is self-contained, naming the claim, the contradicting line and the way to keep the decline, so it needs no unresolved-count lead-in above it — that sentence opens with "No new issues in this revision", which a round posting findings must not claim. The same filter applies on both surfaces: only a note carrying the RebuttalContradiction lead-in is surfaced, never the backstop's generic carry-over note or the model's own unresolved prose.
The follow-up delta comment reported three bare integers, so nothing on the PR said which finding a round closed. A maintainer who cleared one with an @thrillhousebot resolved directive saw "1" and had to diff the PR's thread state across rounds to learn whether the match took; a maintainer who named several in one comment could not tell which the reviewer matched and which it silently skipped, and a skip is a real outcome — matching is by path:line plus title and the guards reject several spellings. It is worse for a finding published with no inline thread, where the directive is the only action that can close it and there is no thread state to diff either. The delta now names each closed finding under the count it explains, one `path:line` — title line each, using the identifiers the directive itself uses. Every finding the resolved count covers is named, not only the ones a directive cleared: the list has to add up to the integer above it. The list is capped at ten with the remainder rolled up as a count, matching the bound the coverage disclosure puts on its file names, and both halves of a line route through MarkdownSafe since they are model text spliced into a list item. A cleared finding's thread also gets a reply before it resolves, so the record sits at the point of the original discussion rather than only in a count on a separate comment. It is deterministic prose stating what happened, with the directive inside a code span — the form the analyzer reads as documentation rather than a command — and a failed reply is swallowed so it can never cost the thread resolution that follows. Refreshing the round-1 summary comment, whose Key Findings block still lists a closed finding as live, is deliberately out of scope: it is the larger change, and a delta that names what it closed removes most of the ambiguity on its own.
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 two round-7 disclosure gaps: the overturned-decline explanation note now rides every review body shape (the no-issues body, the un-anchored fallback body, and the ordinary with-findings body), and the follow-up delta comment names each finding a round closed by `path:line` and title (bounded at 10 with a count rollup) while threads closed by a clear directive receive a deterministic reply before they resolve. publishFollowUpDelta and FollowUpDeltaSummary.render now take the previous round's findings, and resolveAddressedThreads gates the reply on the exact conversationClearedNote. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
A["review round completes"] --> B{"posts new findings?"}
B -- "yes" --> C["with-findings body: append reopened-decline notes"]
B -- "no" --> D["no-issues body: append reopened-decline note"]
C --> E["append partial-coverage banner last"]
D --> E
A --> F["publishFollowUpDelta with previous findings"]
F --> G{"any resolved previous finding?"}
G -- "yes" --> H["name closed findings as path:line and title, cap 10 plus rollup"]
A --> I["resolveAddressedThreads"]
I --> J{"note equals conversationClearedNote?"}
J -- "yes and thread open" --> K["reply on thread, then resolve"]
J -- "no" --> L["resolve thread only"]
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 | - |
| format | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| frontend | 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
Two follow-up round disclosure gaps found in round-7 dogfooding, both on the
publisher surfaces, so they ship together.
#713 — an overturned decline was explained only when the round found nothing new.
appendReopenedDeclineNoteshad a single call site, inside the no-new-findingsreview body. Any round that also raised a finding took the with-findings path and
dropped the note, so two very different outcomes were indistinguishable from the
PR: the model never reading a reply as a decline and reporting the finding
unresolved, versus the model marking it
justifiedand the deterministic re-checkoverturning it on diff evidence. Both rendered as the bare "N previous finding(s)
remain unresolved" — a maintainer who wrote a considered decline saw no
acknowledgement it was read, no contradicting evidence, and no way to tell whether
to argue or to fix. Same undisclosed-decision harm as #542/#598.
The note now rides both with-findings branches (the un-anchored/cap-skipped
fallback body and the ordinary one), placed above the partial-coverage banner so
the coverage caveat stays outermost. It needs no unresolved-count lead-in: each
note is self-contained, and that sentence opens with "No new issues in this
revision", which a round posting findings must not claim. The same filter applies
on both surfaces — only a note carrying the
RebuttalContradictionlead-in issurfaced, never the backstop's generic carry-over note or the model's own prose.
#714 — a cleared finding was never named. The delta comment rendered three
bare integers, so nothing said which finding closed. A maintainer who cleared
one by directive saw "1" and had to diff GitHub thread state across rounds to
learn whether the match took; one who named several in a single comment could not
tell which the reviewer matched and which it silently skipped — and a skip is a
real outcome, since matching is by
path:lineplus title and the guards rejectseveral spellings. Worse for a finding published without an inline thread (#712),
where the directive is the only action that can close it and there is no thread
state to diff either.
Both directions from the issue are implemented:
`path:line` — titleline each — the same identifiers the directive uses.Every finding the
resolvedcount covers is named, not only directive-clearedones: the list has to add up to the integer above it. Bounded at ten names with
the remainder rolled up as a count, matching
ReviewResult.nameList's bound onthe coverage disclosure's file names; both halves route through
MarkdownSafe.at the point of the original discussion. Deterministic prose, the directive
rendered inside a code span (the form
FollowUpAnalyzer.isClearDirectivereadsas documentation rather than a command), and a failed reply is swallowed so it
can never cost the thread resolution that follows.
Out of scope, per the issue: refreshing the round-1 summary comment, whose Key
Findings block still lists a closed finding as live. That is the larger change and
may not be worth it; a delta that names what it closed removes most of the
ambiguity on its own.
Related Issues
Closes #713
Closes #714
How Has This Been Tested?
Every new test was run against the unfixed code first and failed there.
#713 —
reopenedDeclineNoteAlsoReachesTheBodyOfARoundThatPostsFindingsfailedwith
Wanted but not invoked: reviewClient.createReview(...)(the round posted itsinline comment and no review body at all), and
reopenedDeclineNoteReachesTheBodyWhenNoFindingCouldBeAnchoredfailed on a bodycarrying only the un-anchored list and the partial-coverage banner. A third test
pins the negative case: a plain unresolved note and a resolved finding's note are
both left out, so that round still posts no review body.
#714 — with the naming and the reply removed (signatures kept), the five new
tests failed on delta bodies rendering
**Previous findings resolved:** Nwith nolist under it, and on
Wanted but not invoked: reviewClient.replyToReviewComment(...). Restored, all pass.Gates, in order:
Patch coverage:
target/site/jacoco/jacoco.xmlintersected withgit diff -U0 origin/main --reports zero uncovered lines and zero uncoveredbranches across the three changed main files.
Checklist
Screenshots / Logs
The delta comment gains the named sub-list under the count it explains:
Additional Notes
publishFollowUpDeltaandFollowUpDeltaSummary.rendertake the previous round'sfindings so the comment can name what it closed;
ReviewOrchestratoralready hadthat list in hand for thread resolution and passes the same one.