fix(review): group the review body's own fallback into one delivery

src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubLostWrites.java

@@ -211,19 +211,29 @@ public <T> T recording(Target target, Supplier<T> send) {
    * finding that really is lost still announces itself — and announces itself once rather than once
    * per attempt.
    *
-   * <p>Nesting reuses the outer group rather than opening a second one, so a caller cannot lose
-   * another caller's routes by grouping its own.
+   * <p>Nesting for the <em>same</em> pull request reuses the outer group rather than opening a
+   * second one, so a caller cannot lose another caller's routes by grouping its own. Nesting for a
+   * different pull request cannot reuse it: a group speaks only for the pull request it was opened
+   * on ({@link #deliveryFor}), so handing the inner routes the outer group would leave them with no
+   * accounting at all and remember each of them separately — the per-route over-count #729 removed,
+   * reintroduced for the nested caller and silently, with no log and no exception (#748). The inner
+   * group therefore takes over the thread and hands the outer one back when it closes.
    */
   public <T> T asOneDelivery(Target target, Supplier<T> routes) {
-    if (delivery.get() != null) {
+    var outer = delivery.get();
+    if (outer != null && outer.target.equals(target)) {
       return routes.get();
     }
     var scope = new Delivery(target);
     delivery.set(scope);
     try {
       return routes.get();
     } finally {
-      delivery.remove();
+      if (outer == null) {
+        delivery.remove();
+      } else {
+        delivery.set(outer);
+      }
       if (scope.refused && !scope.landed) {
         remember(target);
       }

src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubReviewClient.java

@@ -252,18 +252,23 @@ default PullRequestCommentResponse createPullRequestComment(
   }
 
   /**
-   * Runs {@code routes} — several {@link #createPullRequestComment} calls that are alternative ways
-   * of getting the <em>same</em> content onto the pull request — as one delivery, so a throttle
-   * that costs one route its turn is only announced to the pull request if no route delivered the
-   * content at all (#729).
+   * Runs {@code routes} — several content-creating calls that are alternative ways of getting the
+   * <em>same</em> content onto the pull request — as one delivery, so a throttle that costs one
+   * route its turn is only announced to the pull request if no route delivered the content at all
+   * (#729).
    *
    * <p>Without this the accounting counts refused HTTP calls rather than lost content: #721's
    * file-level fallback lands the finding and the review body published moments later still leads
    * with "an earlier reply on this pull request was never posted … run the command again", twice
    * over for a finding whose suggestion block earned the line-anchored route a second attempt.
    *
+   * <p>The routes need not all be {@link #createPullRequestComment}: #704's fallback carries a
+   * refused review body over to {@link GitHubCommentClient#createComment}, and those two are one
+   * delivery for the same reason (#748). What has to match is the pull request they aim at — a
+   * group speaks for one pull request only.
+   *
    * <p>Lives here rather than at the call site because the notice registry is this package's, and
-   * the call it groups is {@link #createPullRequestComment} on this interface.
+   * {@link #createPullRequestComment} on this interface is the call it was built for.
    */
   static <T> T asOneComment(String owner, String repo, int pullNumber, Supplier<T> routes) {
     return GitHubLostWrites.SHARED.asOneDelivery(

src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java

@@ -926,13 +926,40 @@ void dismissPendingBotReviews(
    * ambiguous failure (timeout, connection reset, 5xx) on any attempt may have landed that
    * attempt's review, so it throws {@link ReviewPostException} instead of risking a duplicate — as
    * does a refusal whose comment fallback fails too.
+   *
+   * <p>Every route here is a route to the <em>same</em> review body, so they are one delivery
+   * ({@link GitHubReviewClient#asOneComment}) for exactly the reason a finding's routes are (#729,
+   * #741): the dropped-post notice counts content the pull request never received, and a body the
+   * comment fallback rescued was received. Ungrouped, a content-creation block — a 403, so a
+   * definite refusal — made {@code createReview} remember a loss that the fallback's own {@code
+   * carrying} then read back, printing "an earlier reply on this pull request was never posted"
+   * above the very comment delivering the body, and counting one lost body twice when neither route
+   * landed (#748).
    */
   void createReviewWithFallback(
       String auth,
       String owner,
       String repo,
       int prNumber,
       GitHubReviewClient.CreateReviewRequest req) {
+    GitHubReviewClient.asOneComment(
+        owner,
+        repo,
+        prNumber,
+        () -> {
+          createReviewRoutes(auth, owner, repo, prNumber, req);
+          // The routes report through their return or their exception, not through a value.
+          return null;
+        });
+  }
+
+  /** The routes themselves, in preference order. See {@link #createReviewWithFallback}. */
+  private void createReviewRoutes(
+      String auth,
+      String owner,
+      String repo,
+      int prNumber,
+      GitHubReviewClient.CreateReviewRequest req) {
     RuntimeException rejection;
     boolean anyAmbiguous;
     try {

src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubLostWritesTest.java

@@ -459,6 +459,80 @@ void aDeliveryNestedInsideAnotherIsOneDelivery() {
     assertEquals("", carried.getLast(), carried.getLast());
   }
 
+  /**
+   * #748. A group inside a group for a <em>different</em> pull request is a group of its own: the
+   * outer delivery speaks only for its own pull request, so reusing it would leave the inner routes
+   * with no accounting at all and announce content the inner group delivered as lost.
+   */
+  @Test
+  void aDeliveryNestedInsideOneForAnotherPullRequestIsGroupedOnItsOwn() {
+    var carried = new ArrayList<String>();
+
+    lost.asOneDelivery(
+        PR,
+        () ->
+            lost.asOneDelivery(
+                OTHER_PR,
+                () -> {
+                  assertThrows(
+                      WebApplicationException.class,
+                      () -> lost.recording(OTHER_PR, () -> throwIt(throttled())));
+                  return lost.recording(OTHER_PR, () -> "posted");
+                }));
+    post(OTHER_PR, carried, null);
+
+    assertEquals(
+        "",
+        carried.getLast(),
+        () ->
+            "the inner delivery was not grouped, so content its second route delivered was"
+                + " announced as lost: "
+                + carried.getLast());
+  }
+
+  /**
+   * The other direction of the same nesting, and the outer delivery on the far side of it: the
+   * inner group really is lost and is announced exactly once, while the outer group is handed back
+   * intact — a route it lost before the inner group ran is still only held, not remembered, and a
+   * later route of its own settles it.
+   */
+  @Test
+  void aNestedDeliveryIsAnnouncedOnceAndHandsTheOuterOneBack() {
+    var carried = new ArrayList<String>();
+
+    lost.asOneDelivery(
+        PR,
+        () -> {
+          assertThrows(
+              WebApplicationException.class, () -> lost.recording(PR, () -> throwIt(throttled())));
+          lost.asOneDelivery(
+              OTHER_PR,
+              () -> {
+                for (var route = 0; route < 2; route++) {
+                  assertThrows(
+                      WebApplicationException.class,
+                      () -> lost.recording(OTHER_PR, () -> throwIt(throttled())));
+                }
+                return "no route landed";
+              });
+          return lost.recording(PR, () -> "posted");
+        });
+    post(OTHER_PR, carried, null);
+    post(PR, carried, null);
+
+    assertTrue(carried.get(0).contains("An earlier reply"), carried.get(0));
+    assertFalse(
+        carried.get(0).contains("earlier replies"),
+        () -> "one lost nested delivery was counted once per refused route: " + carried.get(0));
+    assertEquals(
+        "",
+        carried.get(1),
+        () ->
+            "the outer delivery lost its scope to the nested one, so a route it landed itself no"
+                + " longer settled it: "
+                + carried.get(1));
+  }
+
   private static String throwIt(WebApplicationException failure) {
     throw failure;
   }

src/test/java/dev/thiagogonzaga/thrillhousebot/review/RescuedFindingLostWriteTest.java

@@ -58,10 +58,21 @@ class RescuedFindingLostWriteTest {
   private static final String FILE_LEVEL_THREAD = "the finding, filed on its file";
   private static final String REVIEW_BODY = "the review body";
 
-  /** The response measured in #722, which is what sends a route round the backoff to exhaustion. */
-  private static final String BLOCK_BODY =
-      "{\"message\":\"You have exceeded a secondary rate limit and have been temporarily blocked"
-          + " from content creation. Please retry your request again later.\"}";
+  /**
+   * A secondary rate limit, which is what sends a route round the backoff to exhaustion.
+   *
+   * <p>Not the content-creation wording measured in #722, deliberately (#749). The seam under test
+   * is the accounting, not the backoff: what these tests need from the throttle is that {@link
+   * dev.thiagogonzaga.thrillhousebot.github.GitHubApiError#isThrottled} says yes and that the route
+   * exhausts {@code GitHubWriteRetry.MAX_ATTEMPTS}, both of which a plain secondary limit does. A
+   * content-creation block additionally lifts every wait to {@code
+   * CONTENT_CREATION_BLOCK_MIN_DELAY} — 30 seconds — however the delay was derived, {@code
+   * Retry-After} included since #738. The {@code Retry-After: 0} below used to mean "no sleep" and
+   * stopped meaning it in the same release, at a cost of 461 s for this class and 94 s for a single
+   * test. Nothing here asserts on the block wording, so the cheaper body pins the same behaviour.
+   */
+  private static final String THROTTLE_BODY =
+      "{\"message\":\"You have exceeded a secondary rate limit.\"}";
 
   /**
    * The registry the client writes to is process-wide, so each test takes a pull request of its own
@@ -101,7 +112,7 @@ void setUp() {
   }
 
   /**
-   * The production sequence: GitHub is in a content-creation block, the line-anchored comment burns
+   * The production sequence: GitHub is refusing comment creation, the line-anchored comment burns
    * its whole retry budget, the file-level thread lands on the far side of the window, and the
    * review body goes out moments later. The finding has a working thread, so the body must not open
    * with "an earlier reply on this pull request was never posted … run the command again".
@@ -140,10 +151,10 @@ void aRescuedFindingWithASuggestionIsNotAnnouncedTwiceEither() {
   }
 
   /**
-   * The other direction, which the fix must not cost: when the block outlasts every route the
+   * The other direction, which the fix must not cost: when the throttle outlasts every route the
    * finding really is gone, and the maintainer does have to run the command again. Said once, for
    * one finding, rather than once per refused route — the over-count is not confined to the rescued
-   * case, and a review that lost three findings to a wide block should say three, not nine.
+   * case, and a review that lost three findings to a wide window should say three, not nine.
    */
   @Test
   void aFindingNoRouteCouldDeliverIsStillAnnouncedAsLost() {
@@ -275,10 +286,10 @@ public ReviewResponse createReviewOnce(
       return new ReviewResponse(1L, request.body(), request.event(), request.commitId(), null);
     }
 
-    /** GitHub's content-creation block, naming a deadline of "now" so the test does not sleep. */
+    /** GitHub throttling the post, naming a deadline of "now" so the test does not sleep. */
     private static WebApplicationException blocked() {
       return new WebApplicationException(
-          Response.status(403).header("Retry-After", "0").entity(BLOCK_BODY).build());
+          Response.status(403).header("Retry-After", "0").entity(THROTTLE_BODY).build());
     }
 
     @Override