fix(github): stop announcing a finding the file-level fallback rescued as lost - #741
Merged
Conversation
…d as lost #721 gave a finding up to three routes to the pull request, but the dropped-post accounting still counted refused HTTP calls. A line-anchored comment refused by a content-creation block remembered a loss, the file-level thread then landed, and the review body published moments later still led with "an earlier reply on this pull request was never posted ... run the command again" — twice over when a suggestion block earned the line-anchored route a second attempt. GitHubLostWrites.asOneDelivery groups a finding's routes: a throttled route is held rather than remembered, a route that lands settles the group, and only a delivery that ends refused and undelivered is remembered — once, however many routes were refused. GitHubReviewClient.asOneComment exposes it beside the createPullRequestComment it groups, and ReviewPublisher wraps one finding's routes in it. A finding no route could deliver still announces itself, and now says "an earlier reply" for the one finding it lost rather than one count per refused route. RescuedFindingLostWriteTest drives the real client's default methods rather than a mock of them, which is what hid this: ReviewOrchestratorTest mocks GitHubReviewClient, so Mockito stubs away the default method the accounting lives in and no publisher test had ever executed it. Fixes #729
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 doesThis PR changes GitHubLostWrites' lost-post accounting so the several HTTP routes used to post one finding (line-anchored comment with and without its suggestion block, plus the file-level fallback thread) are grouped into a single delivery: a throttled route is held as refused instead of remembered, a route that lands marks the delivery delivered, and only a delivery that ends refused with no route landed is remembered — once. ReviewPublisher wraps each finding's routes in the new GitHubReviewClient.asOneComment, so a finding rescued by the #721 file-level fallback no longer triggers the false "an earlier reply was never posted" notice on the following review body, a suggestion-bearing rescued finding is not double-counted, and a genuinely lost finding is still announced once per finding. New tests drive the real client default methods so the recording/carrying accounting actually executes. Description vs. ImplementationNo mismatch found between the PR description and the change. Control-Flow Diagram🔀 Show diagramflowchart TD
A["postFindingComment(finding)"] --> B["asOneComment(owner, repo, PR, routes)"]
B --> C["asOneDelivery(PR)"]
C --> D{"delivery already open on thread?"}
D -- "yes: reuse outer delivery" --> G
D -- "no: open a Delivery" --> G
G["try routes: line-anchored with and without suggestion, file-level thread"]
G --> H{"route outcome"}
H -- "throttled" --> I["scope.refused = true, try next route"]
I --> G
H -- "landed" --> J["scope.landed = true"]
H -- "no route left" --> K["close Delivery"]
J --> K
K --> L{"refused and not landed?"}
L -- "yes" --> M["remember(PR) once"]
L -- "no" --> O["rescued: review body carries no notice"]
M --> O
Changes Overview
Changed Files
Risk Assessment
Everything's coming up Thrillhouse! 🎉 No issues found in this PR. Automated review by ThrillhouseBot. Reply with |
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
thrillhousebot
Bot
added
bug
This was referenced Aug 16, 2026
devops-thiago
added a commit
that referenced
this pull request
Aug 16, 2026
…751) ## What type of PR is this? - [x] 🐛 Bug fix ## Description #741 grouped a *finding's* routes into one delivery, but the review body has the identical shape — one piece of content with more than one route to the pull request — and was left ungrouped. `ReviewPublisher.createReviewWithFallback` tries `reviewClient.createReview` (wrapped in `GitHubLostWrites.carrying`) and, on a definite refusal, preserves the same body as an issue comment via `commentClient.createComment` (#704, also wrapped in `carrying`). GitHub's content-creation block is a 403, so it counts as a refusal: route 1 `remember()`s a loss and route 2's `carrying` reads that snapshot back, **prepending "an earlier reply on this pull request was never posted" to the very comment that is delivering the rescued body** — and remembering one lost body twice when neither route lands. That is #729's headline symptom and its double count, live in production on this route pair. Two changes, which have to land together: 1. **`ReviewPublisher.createReviewWithFallback`** now runs its routes inside `GitHubReviewClient.asOneComment`. The routes themselves moved verbatim into a private `createReviewRoutes`; no route, ordering or refusal/ambiguity rule changed. With a scope open, a throttled `createReview` only marks the delivery refused, so the comment fallback carries an empty notice; the fallback landing marks it delivered, so nothing is remembered; and when no route delivers, the body is remembered exactly once. 2. **`GitHubLostWrites.asOneDelivery`** no longer no-ops on *any* open scope. It did so unconditionally, including for a scope opened on a **different** pull request — and a group speaks only for the pull request it was opened on (`deliveryFor` compares the target), so the inner group got no accounting at all and each of its routes was remembered separately: exactly the per-route over-count #729 removed, reintroduced silently for the nested caller. It was unreachable while `asOneComment` had one caller; change 1 adds the second, so it is fixed here. A group whose target differs from the open one now takes over the thread and hands the outer one back in the `finally`. Same-target nesting still reuses the outer group, unchanged. Behaviour that must not regress and does not: a review body no route could deliver is still announced, once; the review body still carries notices left by earlier lost writes; a non-throttle refusal still announces nothing. ## Related Issues Fixes #748 Proof and traces: audit report AUDIT7-A, findings A1 and A2. ## How Has This Been Tested? - [x] Unit tests New `RescuedReviewBodyLostWriteTest` (2 tests) drives the **real `default` methods of both clients** — only the `*Once` HTTP attempts are faked — because a Mockito mock of the client stubs those defaults away, which is what hid this accounting from every publisher test in the first place. Its throttle is a plain secondary-rate-limit 403 rather than a content-creation block, so #738's floor does not apply and the class does not sleep. Two tests added to `GitHubLostWritesTest` for the nesting half. ### Red output on unfixed code (`fc54d93`) ``` [ERROR] RescuedReviewBodyLostWriteTest.aReviewBodyTheCommentFallbackRescuedIsNotAnnouncedAsLost org.opentest4j.AssertionFailedError: the review body was delivered by its comment fallback, and the very comment that delivered it opens by telling the maintainer it was never posted: > [!WARNING] > **An earlier reply on this pull request was never posted.** GitHub was rate-limiting the bot and the retries ran out, so work it had already finished was thrown away. If you were waiting on an answer, run the command again. (warning sign) GitHub refused the review post, so ThrillhouseBot is posting the review as a regular comment instead. ThrillhouseBot requested changes — see inline. ==> expected: [false] but was: [true] [ERROR] RescuedReviewBodyLostWriteTest.aReviewBodyNoRouteCouldDeliverIsAnnouncedExactlyOnce org.opentest4j.AssertionFailedError: the genuinely lost review body must still be announced: > [!WARNING] > **2 earlier replies on this pull request were never posted.** GitHub was rate-limiting the bot and the retries ran out, so work it had already finished was thrown away. If you were waiting on an answer, run the command again. the next thing the bot posts ==> expected: [true] but was: [false] [ERROR] GitHubLostWritesTest.aDeliveryNestedInsideOneForAnotherPullRequestIsGroupedOnItsOwn org.opentest4j.AssertionFailedError: the inner delivery was not grouped, so content its second route delivered was announced as lost: > [!WARNING] > **An earlier reply on this pull request was never posted.** ... ==> expected: [] but was: [> [!WARNING] > **An earlier reply on this pull request was never posted.** ...] [ERROR] GitHubLostWritesTest.aNestedDeliveryIsAnnouncedOnceAndHandsTheOuterOneBack org.opentest4j.AssertionFailedError: > [!WARNING] > **2 earlier replies on this pull request were never posted.** ... ==> expected: [true] but was: [false] [ERROR] Tests run: 26, Failures: 4, Errors: 0, Skipped: 0 ``` (Angle brackets around the JUnit expected/actual values replaced with square brackets so they survive rendering; everything else is verbatim.) The first failure is the false notice printed directly above the content it is denying; the second and fourth are the double count. All four pass on this branch. ### Gates - `spotless:apply` → `clean compile spotbugs:check spotless:check`: **BugInstance size is 0**, BUILD SUCCESS - `clean test`: **Tests run: 3308, Failures: 0, Errors: 0, Skipped: 0** — BUILD SUCCESS - jacoco ∩ `git diff -U0 fc54d93...HEAD` on changed main code: **zero uncovered lines, zero uncovered branches** ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] I have updated the documentation accordingly - [x] My changes generate no new warnings or errors ## Additional Notes The javadoc on `asOneComment` and `asOneDelivery` was corrected too: the former was written as if it only ever groups `createPullRequestComment` calls, and the latter justified the unconditional nesting no-op with an argument that only holds when the targets match.
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
A finding that #721's file-level fallback rescued still told the pull request it had been thrown
away.
GitHubReviewClient.createPullRequestCommentwraps every attempt inGitHubLostWrites.recording,which remembers a loss when a throttle exhausts the write budget. Before #721 that was honest — the
finding really was gone. #721 changed the outcome without changing the accounting: the line-anchored
comment is refused,
ReviewPublisherfalls back to a thread on the file, the thread lands, thefinding is counted as posted, and nothing retracts the remembered loss. The very next content the
review publishes is the review body, which goes out through
GitHubLostWrites.carryingand leadswith
while the finding sits on the PR directly above it, and re-running the command duplicates the
review. It was also double-counted: each of the two line-anchored routes (with the suggestion block,
and without) is its own
recordingcall, so one rescued finding rendered "2 earlier replies…".The fix
The accounting counted refused HTTP calls. It has to count content the pull request never received,
because since #721 a finding is one piece of content with up to three routes to the PR.
GitHubLostWrites.asOneDelivery(target, routes)groups those routes. Inside it a throttled route isheld rather than remembered; a route that lands marks the delivery delivered; and only a delivery
that ends refused and undelivered is remembered — once, however many routes were refused.
GitHubReviewClient.asOneCommentexposes it next to thecreatePullRequestCommentit groups (theregistry is that package's), and
ReviewPublisher.postFindingCommentwraps one finding's routes init.
Three consequences, all intended:
route is the same delivery, not a second loss.
for the one finding it lost rather than one count per refused route.
Nothing else changes. A route refused for anything but a throttle stays as invisible as it was (a
422 about the payload is a defect to fix, not a post to re-run), and a call outside a delivery — a
thread reply, a review body — records exactly as before.
The delivery is thread-confined, which is the confinement the routes already have: one finding's
attempts run one after another on the thread publishing that review. That avoids threading a handle
through the REST client interface that sits between
asOneDeliveryandrecording, and it keepsdeliveries for different pull requests, and different registries, from seeing each other. A group
nested inside another reuses the outer one, and a call for a different pull request made inside a
delivery is accounted to that pull request rather than to the group.
Related Issues
Fixes #729
How Has This Been Tested?
RescuedFindingLostWriteTestis the proof, and it deliberately does not mockGitHubReviewClient.ReviewOrchestratorTestdeclares that client a Mockito@Mock, which stubsthe
defaultmethods away, so therecording/carryingaccounting had never executed in anypublisher test — which is exactly why a suite of 3290 green tests could not see this. The new test
stubs only the six
*OnceHTTP attempts and lets the realcreatePullRequestComment,createReview, retry, backoff and pacer run, drivingReviewPublisher.postInlineCommentsand thencreateReviewWithFallbackagainst GitHub's measured content-creation-block response.Red — the three tests against unfixed code (
aa3c556), verbatimThe first two are the audit's
P5andP5breproduced through the production call path rather thanagainst the registry directly. The third fails because the notice reads "2 earlier replies" for the
one finding it lost — the over-count is not confined to the rescued case — and it is the test that
stops this change from buying a quiet PR by making a real loss silent.
Green
All three pass with the fix.
GitHubLostWritesTestadds five tests pinning the grouping directly: arescued delivery, a delivery no route landed, a delivery no throttle touched, a call for another
pull request made inside one, and a nested group.
Gates
./mvnw -B spotless:apply→BUILD SUCCESS./mvnw -B clean compile spotbugs:check spotless:check→BugInstance size is 0,Error size is 0,BUILD SUCCESS./mvnw -B clean test→Tests run: 3290, Failures: 0, Errors: 0, Skipped: 0,BUILD SUCCESSgit diff -U0 aa3c556...HEAD: 28 executable changed lines and 14 branches in thechanged main code, zero missed on both
Additional Notes
RescuedFindingLostWriteTesttakes about 25 seconds, and that isGitHubWritePacer, not theassertions: driving the real write path means honouring GitHub's one-content-creating-request-per-
second envelope, and proving a route is refused to exhaustion costs the full four attempts. The
shapes were chosen for the fewest attempts that still make each claim, and the pacer is left alone
on purpose —
GitHubWritePacingTestasserts against that same shared envelope, so turning it offfor the test JVM would trade this proof for that one.
Checklist