fix(review): recover lost panel-retrigger clicks instead of silently erasing them (#9000) (#9584)

JSONbored · web-flow · commit 643383cdbee7 · 2026-07-28T19:37:25.000Z
The lost-click half of #9000, completing the issue (the skip-auditing half landed earlier; its enforcement went in as #9583). THE DEFECT. Three checkbox states looked identical to a maintainer: processed (panel republished, box reset), deferred for CI (box stays ticked, honored later via the #7626 pending marker), and DELIVERY LOST (box stays ticked, nothing recorded, nothing will ever happen -- three such losses observed live on #8972, 2026-07-26). Only the third is broken, and it is unrecoverable from our own state alone: the tick exists only in the live comment body, because the webhook that would have told us about it never became a job. Worse, the next panel republish -- whose renderer emits its checkboxes unticked unconditionally -- would OVERWRITE the ticked box, erasing the only evidence of the click on the exact pass best placed to honor it. THE FIX, at zero additional API cost. createOrUpdateIssueCommentWithMarker already fetches the existing comment for its marker search; it now returns that pre-overwrite body. Every panel republish (webhook pass, re-gate sweep, backlog convergence) doubles as the detector: a ticked rerun box in the body being replaced, with no recent processing and no pending marker, IS a lost click. Recovery then: - records github_app.pr_panel_retrigger_recovered (the named reason #9003 demands, and the loop guard against re-detection), - persists the #7626 pending marker, so the next review pass consumes it as forceAiReview exactly like a deferred click, and - enqueues an agent-regate-pr job immediately (with the #9499 prCreatedAt sort key) -- a PR with a lost click is precisely the PR that cannot count on a natural pass. No new sweep, no new persistence: the issue's "within one sweep interval" acceptance is bounded by the republish cadence that already exists. FALSE-POSITIVE PROOFING, the part that took the design care: - A pass that IS the retrigger (forceAiReview) skips detection -- overwriting its own tick is the receipt acknowledgement, not a loss. - A recently-processed retrigger (actor-agnostic audit lookup, new countAuditEventsForTargetSince -- hasAuditEventForDelivery is deliberately actor+delivery-keyed, the wrong shape when the delivery never existed) means the delivery raced the pass rather than being lost. - A DEFERRED click is guarded by ordering, not luck: the retrigger handler now marks the pending marker BEFORE its readiness check (consuming it right back on the immediate path), so the CI-wait placeholder inside that very pass sees the marker and stands down. A crash between mark and consume degrades to one extra forced review -- the safe direction for an explicit user click. The #7626 sibling test's "never creates a pending marker" assertion is updated to the invariant it documents ("no pending INTENT survives the immediate path") rather than its old mechanism (key absence), since consumption can leave the one-shot consumed sentinel behind on adapters without a real delete. TESTS. Eight in the new suite: the #8972 regression (natural pass recovers the tick: event + marker + job with the sort key), full end-to-end (the recovery job's pass consumes the marker, spends exactly one fresh AI review, and does not re-diagnose its own republish), the raced-delivery guard, the deferred-click guard (CI genuinely pending, marker untouched afterward), the no-head ghost PR (marker guard skipped, job omits the optional sort key), unticked and first-publish no-ops, and the empty event-type-list guard. Plus the three github-comments assertions extended to pin previousBody, including that it is the POSTED body, not the re-render, on the byte-identical skip path. Changed lines: 0 uncovered statements, 0 uncovered branches.
diff --git a/src/db/repositories.ts b/src/db/repositories.ts
@@ -3564,6 +3564,28 @@ export async function hasAuditEventForDelivery(env: Env, actor: string, eventTyp
   return (row?.count ?? 0) > 0;
 }
 
+/** #9000: how many of ANY of `eventTypes` exist for `targetKey` since `sinceIso`, actor-agnostic. The
+ *  lost-click recovery needs "was this retrigger processed recently, by anyone or by the recovery itself" --
+ *  hasAuditEventForDelivery above is deliberately actor+delivery-keyed for redelivery guarding, which is the
+ *  wrong shape here: a lost delivery has no deliveryId to match, and the processing pass may have run under a
+ *  different actor than whoever ticked the box. */
+export async function countAuditEventsForTargetSince(env: Env, eventTypes: readonly string[], targetKey: string, sinceIso: string): Promise<number> {
+  if (eventTypes.length === 0) return 0;
+  const db = getDb(env.DB);
+  const [row] = await db
+    .select({ count: sql<number>`count(*)` })
+    .from(auditEvents)
+    .where(
+      and(
+        inArray(auditEvents.eventType, [...eventTypes]),
+        eq(auditEvents.targetKey, targetKey),
+        gte(auditEvents.createdAt, sinceIso),
+      ),
+    );
+  /* v8 ignore next -- count(*) always returns exactly one row; the empty-array guard only satisfies the destructure type. */
+  return row?.count ?? 0;
+}
+
 /** Whether `eventType` has ALREADY been recorded for this `targetKey` at this EXACT `headSha` -- unlike
  *  `hasAuditEventForDelivery` above (which guards a single redelivered webhook within a short window), this has
  *  no time bound: a head SHA is a stable, permanent identity, so a match at any point in the past is still a
diff --git a/src/github/comments.ts b/src/github/comments.ts
@@ -67,7 +67,7 @@ export async function createOrUpdatePrIntelligenceComment(
   pullNumber: number,
   body: string,
   options: { createIfMissing?: boolean | undefined; mode?: AgentActionMode } = {},
-): Promise<{ id: number; html_url?: string; changed: boolean } | null> {
+): Promise<{ id: number; html_url?: string; changed: boolean; previousBody?: string } | null> {
   return createOrUpdateIssueCommentWithMarker(env, installationId, repoFullName, pullNumber, body, PR_INTELLIGENCE_COMMENT_MARKER, options);
 }
 
@@ -130,7 +130,7 @@ async function createOrUpdateIssueCommentWithMarker(
   body: string,
   marker: string,
   options: { createIfMissing?: boolean | undefined; mode?: AgentActionMode } = {},
-): Promise<{ id: number; html_url?: string; changed: boolean } | null> {
+): Promise<{ id: number; html_url?: string; changed: boolean; previousBody?: string } | null> {
   const parts = repoFullName.split("/");
   const owner = parts[0];
   const repo = parts[1];
@@ -168,12 +168,19 @@ async function createOrUpdateIssueCommentWithMarker(
       // marker — also collapses a duplicate webhook delivery for the same commit.
       // #9069: compared through comparableCommentBody so the panel's per-pass "Review updated" timestamp — which
       // changes on every render and made this check unreachable for the panel — no longer counts as a change.
+      // #9000: the body we are ABOUT to overwrite, surfaced to the caller. The panel renderer always emits
+      // its interactive checkboxes unticked, so a tick lives only in the live comment -- and this PATCH is
+      // the exact moment a lost-delivery click (a box the user ticked whose webhook never became a job) would
+      // otherwise be silently erased, leaving the maintainer certain ORB ignored them. Returning the previous
+      // body costs nothing (it was already fetched for the marker search) and lets the publish path notice
+      // and honor the intent instead of destroying the only evidence it existed.
       /* v8 ignore next -- `?? ""` is a type-level guard only: the marker filter above requires
        * `comment.body?.includes(candidate)`, so any comment that becomes `canonical` provably has a non-empty
        * string body. Kept because IssueComment types `body` as `string | null | undefined`. */
-      if (comparableCommentBody(canonical.body ?? "") === comparableCommentBody(body)) {
+      const previousBody = canonical.body ?? "";
+      if (comparableCommentBody(previousBody) === comparableCommentBody(body)) {
         await deleteDuplicateMarkerComments(octokit, owner, repo, existing, canonical.id);
-        return { id: canonical.id, ...(canonical.html_url !== undefined ? { html_url: canonical.html_url } : {}), changed: false };
+        return { id: canonical.id, ...(canonical.html_url !== undefined ? { html_url: canonical.html_url } : {}), changed: false, previousBody };
       }
       const response = await octokit.request("PATCH /repos/{owner}/{repo}/issues/comments/{comment_id}", {
         owner,
@@ -182,7 +189,7 @@ async function createOrUpdateIssueCommentWithMarker(
         body,
       });
       await deleteDuplicateMarkerComments(octokit, owner, repo, existing, canonical.id);
-      return { ...(response.data as { id: number; html_url?: string }), changed: true };
+      return { ...(response.data as { id: number; html_url?: string }), changed: true, previousBody };
     }
     if (options.createIfMissing === false) return null;
     const response = await octokit.request("POST /repos/{owner}/{repo}/issues/{issue_number}/comments", {
diff --git a/src/queue/processors.ts b/src/queue/processors.ts
@@ -69,6 +69,7 @@ import {
   countRecentAuditEventsForActorInRepo,
   countRecentAuditEventsForActorInRepoWithTargetSuffix,
   recentStaleRecheckDeniedPullNumbers,
+  countAuditEventsForTargetSince,
   hasAuditEventForDelivery,
   hasAuditEventForHeadSha,
   recordGateBlockOutcome,
@@ -4536,7 +4537,29 @@ async function prReadyForReview(
           repoFullName,
           pr.number,
           `${PR_PANEL_COMMENT_MARKER}\n\n${renderWaitingForCiPlaceholder({ reason: waitingReason })}`,
-        ).catch(() => undefined);
+        )
+          .then((placeholderResult) =>
+            // #9000: this placeholder is the FIRST panel overwrite on a deferring pass, so it is where a
+            // lost click would otherwise be erased with the pass never reaching the final publish at all.
+            // A legitimately-deferring retrigger pass is protected by ordering, not luck: its handler now
+            // marks the pending marker BEFORE calling prReadyForReview, so the marker guard inside skips it.
+            maybeRecoverLostPanelRetrigger(env, {
+              previousBody: placeholderResult?.previousBody,
+              forceAiReview: undefined,
+              installationId,
+              repoFullName,
+              prNumber: pr.number,
+              /* v8 ignore next -- the null arm is structurally unreachable here: a falsy headSha makes
+               * prReadyForReview return true unconditionally (its own documented design), so this CI-wait
+               * placeholder never renders for a no-head PR. Type-level guard only. */
+              headSha: pr.headSha ?? null,
+              prCreatedAt: pr.createdAt ?? null,
+              deliveryId,
+            }),
+          ).catch(
+            /* v8 ignore next -- fail-safe: recovery is additive; its failure must never turn a CI-wait deferral into a thrown error. */
+            () => undefined,
+          );
       }
       return false;
     }
@@ -4759,6 +4782,87 @@ function pendingPrPanelRetriggerKey(repoFullName: string, prNumber: number, head
   return `pr-panel-retrigger-pending:${repoFullName.toLowerCase()}#${prNumber}:${headSha}`;
 }
 
+// #9000, the lost-click half. Three checkbox states used to look identical to a maintainer: processed
+// (panel republished, box reset), deferred for CI (box stays ticked, intent persisted by the pending marker
+// above), and DELIVERY LOST (box stays ticked, nothing recorded, nothing will ever happen). Only the third is
+// broken, and it is unrecoverable from our own state alone -- the tick exists only in the live comment body,
+// because the webhook that would have told us about it never became a job (three such losses observed live on
+// #8972, 2026-07-26).
+//
+// The interception point is the panel republish: createOrUpdatePrIntelligenceComment already fetches the
+// existing comment for its marker search, and the publish was about to OVERWRITE the ticked box with the
+// renderer's unconditional `- [ ]` -- erasing the only evidence of the click, on the exact pass best placed
+// to honor it. So recovery costs zero extra API calls and needs no new sweep: any pass that republishes the
+// panel (webhook, re-gate sweep, backlog convergence) doubles as the detector, which is what bounds the
+// issue's "within one sweep interval" acceptance.
+const PR_PANEL_RETRIGGER_RECOVERED_EVENT_TYPE = "github_app.pr_panel_retrigger_recovered";
+
+async function maybeRecoverLostPanelRetrigger(
+  env: Env,
+  args: {
+    previousBody: string | undefined;
+    forceAiReview: boolean | undefined;
+    installationId: number;
+    repoFullName: string;
+    prNumber: number;
+    headSha: string | null;
+    prCreatedAt: string | null;
+    deliveryId: string;
+  },
+): Promise<void> {
+  // The box in the body we just replaced was not ticked -- nothing to recover. This is the overwhelmingly
+  // common case and the only read it costs is a string scan of a body we already held.
+  if (!isCheckedPrPanelRetrigger(args.previousBody)) return;
+  // This pass IS the retrigger being processed (or a recovery pass consuming the marker below): overwriting
+  // its own tick is the normal receipt acknowledgement, not a loss.
+  if (args.forceAiReview === true) return;
+  const targetKey = `${args.repoFullName}#${args.prNumber}`;
+  const sinceIso = new Date(Date.now() - COMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS).toISOString();
+  // A recent processed retrigger means the tick we replaced was ALREADY honored -- the delivery raced this
+  // pass rather than being lost. Actor-agnostic on purpose: any recent processing settles the question, and
+  // the audit query helpers are actor-keyed, so this reads the raw ledger.
+  const recent = await countAuditEventsForTargetSince(
+    env,
+    [ "github_app.pr_panel_retriggered", PR_PANEL_RETRIGGER_RECOVERED_EVENT_TYPE ],
+    targetKey,
+    sinceIso,
+  );
+  if (recent > 0) return;
+  // The deferred case (#7626): the click WAS processed and is waiting for CI behind the pending marker. Peek,
+  // never consume -- consumption belongs to the review pass that will honor it.
+  if (args.headSha) {
+    const pending = await getTransientKey(env, pendingPrPanelRetriggerKey(args.repoFullName, args.prNumber, args.headSha));
+    if (pending === PENDING_PR_PANEL_RETRIGGER_MARKER) return;
+  }
+
+  // A genuinely lost click. Honor the intent exactly the way the deferred path does -- persist the pending
+  // marker so the next review pass consumes it as forceAiReview -- and enqueue that pass NOW rather than
+  // waiting for the next natural touch, since "the next natural touch" is precisely what a PR with a lost
+  // click cannot count on. The audit event doubles as the loop guard above and as the named reason #9003's
+  // invariant demands.
+  await recordAuditEvent(env, {
+    eventType: PR_PANEL_RETRIGGER_RECOVERED_EVENT_TYPE,
+    actor: null,
+    targetKey,
+    outcome: "queued",
+    detail: "panel rerun checkbox found ticked with no matching processing -- the delivery was lost; recovering the click as a forced re-review",
+    metadata: { deliveryId: args.deliveryId, repoFullName: args.repoFullName, headSha: args.headSha },
+  }).catch(
+    /* v8 ignore next -- fail-safe: an audit write failure must not abort the recovery it is narrating. */
+    () => undefined,
+  );
+  await markPendingPrPanelRetrigger(env, args.repoFullName, args.prNumber, args.headSha);
+  const job: JobMessage = {
+    type: "agent-regate-pr",
+    deliveryId: `panel-retrigger-recovery:${args.repoFullName}#${args.prNumber}`,
+    repoFullName: args.repoFullName,
+    prNumber: args.prNumber,
+    installationId: args.installationId,
+    ...(args.prCreatedAt ? { prCreatedAt: args.prCreatedAt } : {}),
+  };
+  await env.JOBS.send(job);
+}
+
 /** Persists the pending forceAiReview intent for this exact (repo, PR, headSha) -- best-effort: a storage
  *  hiccup here just means the eventual natural re-evaluation reviews without the forced fresh call, the SAME
  *  degraded-but-safe outcome as before this marker existed, never a thrown error or a blocked PR. */
@@ -11166,14 +11270,29 @@ async function maybePublishPrPublicSurface(
       if (shouldShowPlaceholderNow) {
         const placeholderBody = `${PR_PANEL_COMMENT_MARKER}\n\n${renderReviewingPlaceholder()}`;
         try {
-          await createOrUpdatePrIntelligenceComment(
+          const placeholderResult = await createOrUpdatePrIntelligenceComment(
             env,
             installationId,
             repoFullName,
             pr.number,
             placeholderBody,
             { mode },
           );
+          // #9000: on a pass that runs the AI, THIS overwrite (not the final publish) is the one that
+          // replaces whatever the maintainer last saw -- including a ticked box whose delivery was lost.
+          await maybeRecoverLostPanelRetrigger(env, {
+            previousBody: placeholderResult?.previousBody,
+            forceAiReview: webhook.forceAiReview,
+            installationId,
+            repoFullName,
+            prNumber: pr.number,
+            headSha: pr.headSha ?? null,
+            prCreatedAt: pr.createdAt ?? null,
+            deliveryId: webhook.deliveryId,
+          }).catch(
+            /* v8 ignore next -- fail-safe: same additive-surface rule as the CI-wait hook; a recovery failure must not fail the placeholder publish. */
+            () => undefined,
+          );
         } catch (error) {
           /* v8 ignore next -- placeholder rate-limit propagation is covered by final-comment rate-limit tests. */
           if (isGitHubRateLimitedError(error)) throw error;
@@ -12855,6 +12974,23 @@ async function maybePublishPrPublicSurface(
       // idempotency check) sets this false -- a real create/update, or the createIfMissing:false null case
       // (not reachable on this call site, which never sets that option), both default true.
       commentContentChanged = commentResult?.changed ?? true;
+      // #9000: the body this publish just replaced may carry a ticked rerun checkbox whose webhook delivery
+      // was lost -- the overwrite above would otherwise be the moment that intent is silently erased. Fire-and-
+      // forget semantics are wrong here (the recovery enqueues a job), but a recovery failure must not fail
+      // the publish that hosted it, hence the terminal catch.
+      await maybeRecoverLostPanelRetrigger(env, {
+        previousBody: commentResult?.previousBody,
+        forceAiReview: webhook.forceAiReview,
+        installationId,
+        repoFullName,
+        prNumber: pr.number,
+        headSha: pr.headSha ?? null,
+        prCreatedAt: pr.createdAt ?? null,
+        deliveryId: webhook.deliveryId,
+      }).catch(
+        /* v8 ignore next -- fail-safe: a recovery failure must not fail the publish that hosted it. */
+        () => undefined,
+      );
       publishedOutputs.push("comment");
       incr("loopover_reviews_published_total", { repo: repoFullName });
       // Real end-to-end review latency (#review-latency-metric): pr.headShaObservedAt is the moment THIS
@@ -14304,6 +14440,13 @@ async function maybeProcessPrPanelRetrigger(
     await refreshPullRequestDetails(env, repoFullName, pr.number, { force: true });
   }
   const liveFacts = createLiveGithubFacts();
+  // #7626 intent persistence, moved BEFORE the readiness check (#9000): prReadyForReview's own CI-wait
+  // placeholder republishes the panel, and the lost-click recovery hooked into every panel republish treats
+  // "ticked box, no recent processing, no pending marker" as a lost delivery. During THIS pass the click is
+  // very much being processed -- marking first is what tells the recovery so. The immediate-readiness path
+  // consumes the marker right back (it threads forceAiReview itself); a crash between mark and consume
+  // degrades to one extra forced review on the next pass, the safe direction for an explicit user click.
+  await markPendingPrPanelRetrigger(env, repoFullName, pr.number, pr.headSha);
   if (
     !(await prReadyForReview(
       env,
@@ -14315,10 +14458,9 @@ async function maybeProcessPrPanelRetrigger(
       liveFacts,
     ))
   ) {
-    // #7626: the explicit forceAiReview intent below is otherwise lost the instant this defers -- persist it
-    // so the next natural re-evaluation of this exact (repo, PR, headSha), whichever entry point reaches it
-    // first once CI settles, can still honor the user's click instead of silently replaying stale content.
-    await markPendingPrPanelRetrigger(env, repoFullName, pr.number, pr.headSha);
+    // The marker set above persists the forceAiReview intent across the deferral -- the next natural
+    // re-evaluation of this exact (repo, PR, headSha), whichever entry point reaches it first once CI
+    // settles, consumes it and forces the fresh review the user asked for.
     await recordAuditEvent(env, {
       eventType: "github_app.pr_panel_retrigger_deferred",
       actor,
@@ -14329,6 +14471,9 @@ async function maybeProcessPrPanelRetrigger(
     }).catch(() => undefined);
     return true;
   }
+  // Immediate-readiness path: this pass threads forceAiReview directly, so the marker set above must not
+  // survive to force a SECOND review on some later, unrelated pass. One-shot consume; result discarded.
+  await consumePendingPrPanelRetrigger(env, repoFullName, pr.number, pr.headSha);
   await maybePublishPrPublicSurface(
     env,
     installationId,
diff --git a/test/unit/github-comments.test.ts b/test/unit/github-comments.test.ts
@@ -396,7 +396,9 @@ describe("GitHub PR intelligence comments", () => {
 
     const result = await createOrUpdatePrIntelligenceComment(createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }), 123, "JSONbored/gittensory", 12, body);
 
-    expect(result).toEqual({ id: 101, html_url: "https://github.com/comment/101", changed: false }); // html_url-present branch of the early return; changed:false is the #6724 no-op signal
+    // #9000: previousBody rides every canonical-comment return (identical or PATCHed) — it is how the panel
+    // publish detects a ticked rerun checkbox it is about to overwrite (the lost-click recovery).
+    expect(result).toEqual({ id: 101, html_url: "https://github.com/comment/101", changed: false, previousBody: body }); // html_url-present branch of the early return; changed:false is the #6724 no-op signal
     expect(calls.some((call) => call.startsWith("PATCH "))).toBe(false); // identical body → NO GitHub write
   });
 
@@ -416,7 +418,7 @@ describe("GitHub PR intelligence comments", () => {
 
     const result = await createOrUpdatePrIntelligenceComment(createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }), 123, "JSONbored/gittensory", 12, body);
 
-    expect(result).toEqual({ id: 202, changed: false }); // html_url-absent branch → no html_url key; changed:false is the #6724 no-op signal
+    expect(result).toEqual({ id: 202, changed: false, previousBody: body }); // html_url-absent branch → no html_url key; changed:false is the #6724 no-op signal
     expect(calls.some((call) => call.startsWith("PATCH "))).toBe(false);
   });
 
@@ -439,7 +441,9 @@ describe("GitHub PR intelligence comments", () => {
 
     // The whole point of #9069: a clock-only delta is NOT a content change, so no GitHub write and no
     // self-inflicted issue_comment.edited delivery. changed:false also keeps the #6724 no-op accounting honest.
-    expect(result).toEqual({ id: 303, html_url: "https://github.com/comment/303", changed: false });
+    // previousBody is the POSTED body (what a reader saw), not the re-render — the distinction the
+    // lost-click recovery depends on, since a ticked checkbox only ever exists in the posted copy.
+    expect(result).toEqual({ id: 303, html_url: "https://github.com/comment/303", changed: false, previousBody: posted });
     expect(calls.some((call) => call.startsWith("PATCH "))).toBe(false);
   });
 
diff --git a/test/unit/pr-panel-retrigger-lost-click-recovery.test.ts b/test/unit/pr-panel-retrigger-lost-click-recovery.test.ts
diff --git a/test/unit/pr-panel-retrigger-pending-force-review.test.ts b/test/unit/pr-panel-retrigger-pending-force-review.test.ts
