fix(review): reuse the live mergeable_state/CI read across a single pass

src/queue/processors.ts

@@ -613,13 +613,23 @@ interface LiveGithubFacts {
   requiredContexts: Map<string, Promise<RequiredStatusContextsLookup>>;
   ciAggregates: Map<string, Promise<LiveCiAggregate>>;
   mergeStates: Map<string, Promise<string | undefined>>;
+  // #4498: which ciAggregates/mergeStates keys were populated by a FORCED (refreshLiveCiAggregate/
+  // refreshLiveMergeState) write THIS pass, as opposed to a cached* reader's write -- the cached* variants can
+  // populate the SAME map/key from the DURABLE cross-webhook cache (potentially stale, e.g. readiness's own
+  // cachedLiveCiAggregate check), so a plain "is there anything in the map for this key" check cannot tell a
+  // genuinely-fresh forced value apart from a possibly-stale cached one. reuseOrRefreshLiveCiAggregate/
+  // reuseOrRefreshLiveMergeState only ever reuse a memoized value when its key is ALSO in these sets.
+  forcedCiAggregateKeys: Set<string>;
+  forcedMergeStateKeys: Set<string>;
 }
 
 function createLiveGithubFacts(): LiveGithubFacts {
   return {
     requiredContexts: new Map(),
     ciAggregates: new Map(),
     mergeStates: new Map(),
+    forcedCiAggregateKeys: new Set(),
+    forcedMergeStateKeys: new Set(),
   };
 }
 
@@ -873,6 +883,7 @@ function refreshLiveCiAggregate(
     ),
   );
   facts.ciAggregates.set(key, next);
+  facts.forcedCiAggregateKeys.add(key);
   return next;
 }
 
@@ -920,9 +931,53 @@ function refreshLiveMergeState(
     fetchLivePullRequestMergeState(env, repoFullName, prNumber, token, admissionKey),
   );
   facts.mergeStates.set(key, next);
+  facts.forcedMergeStateKeys.add(key);
   return next;
 }
 
+// #4498: reuses THIS PASS's own already-FORCED-live-refreshed value when an earlier refreshLiveMergeState/
+// refreshLiveCiAggregate call in the SAME webhook pass (sharing the SAME `facts` object and key -- e.g.
+// maybePublishPrPublicSurface's own post-gate-publish refresh) already populated the request-local memo,
+// instead of re-fetching the identical resource from GitHub a second time. Deliberately NOT a plain "is
+// something in facts.mergeStates/ciAggregates for this key" check: cachedLiveMergeState/cachedLiveCiAggregate
+// (the READINESS-path reader) populate the SAME map/key from the DURABLE cross-webhook cache on their own
+// request-local miss -- a durable-cache HIT there can be an OLDER webhook's snapshot, exactly what
+// refreshLiveMergeState's #4220 doc comment above prohibits for this act-boundary-adjacent disposition input.
+// So this only reuses a memoized value when its key is ALSO in forcedMergeStateKeys/forcedCiAggregateKeys --
+// i.e. it was written by a FORCED (genuinely-live-this-pass) call, never by a cached-path reader. On a genuine
+// miss (no forced call ran yet this pass, e.g. unifiedCommentAllowed was false) this falls through to a REAL
+// live refresh, so behavior can only ever improve (fewer calls) over the pre-fix code, never go staler.
+function reuseOrRefreshLiveMergeState(
+  env: Env,
+  repoFullName: string,
+  facts: LiveGithubFacts,
+  prNumber: number,
+  token: string | undefined,
+  admissionKey?: GitHubRateLimitAdmissionKey,
+): Promise<string | undefined> {
+  const key = liveFactKey(repoFullName, prNumber, liveFactTokenPart(token));
+  const cached = facts.forcedMergeStateKeys.has(key) ? facts.mergeStates.get(key) : undefined;
+  if (cached) return cached;
+  return refreshLiveMergeState(env, repoFullName, facts, prNumber, token, admissionKey);
+}
+
+function reuseOrRefreshLiveCiAggregate(
+  env: Env,
+  repoFullName: string,
+  facts: LiveGithubFacts,
+  prNumber: number,
+  headSha: string | null | undefined,
+  baseRef: string | null | undefined,
+  token: string | undefined,
+  expectedCiContexts: ReadonlyArray<string> | null | undefined,
+  admissionKey?: GitHubRateLimitAdmissionKey,
+): Promise<LiveCiAggregate> {
+  const key = liveFactKey(repoFullName, headSha, baseRef, liveFactTokenPart(token), expectedCiContextsKeyPart(expectedCiContexts));
+  const cached = facts.forcedCiAggregateKeys.has(key) ? facts.ciAggregates.get(key) : undefined;
+  if (cached) return cached;
+  return refreshLiveCiAggregate(env, repoFullName, facts, prNumber, headSha, baseRef, token, expectedCiContexts, admissionKey);
+}
+
 /**
  * Run (or dry-run) the data-retention prune across the configured log/snapshot tables, plus the
  * signal_snapshots dedup pass (#3810 -- signal_snapshots has no natural dedup, so within its own
@@ -2705,16 +2760,19 @@ async function runAgentMaintenancePlanAndExecute(
       admissionKey,
     ),
     // Live mergeable_state after the gate's own publish/review/check mutations. Readiness may have seen the PR as
-    // blocked before the bot approval/check landed, so this boundary must refresh instead of replaying the cache.
-    refreshLiveMergeState(env, repoFullName, args.liveFacts, pr.number, token, admissionKey),
+    // blocked before the bot approval/check landed, so this boundary must never replay the durable cross-webhook
+    // cache -- but maybePublishPrPublicSurface's OWN post-publish refresh (same pass, same liveFacts object) has
+    // typically already paid for this exact live read moments earlier (#4498); reuse it instead of fetching twice.
+    reuseOrRefreshLiveMergeState(env, repoFullName, args.liveFacts, pr.number, token, admissionKey),
     // RC1: live reviewDecision so the approve/request-changes dedup is accurate. The STORED reviewDecision is
     // only written by the open-PR backfill and goes stale → the planner re-posted a review every cycle (the
     // re-review loop with 14-23 stacked reviews). With the live value, an already-approved/changes-requested PR
     // is not re-reviewed for the same state.
     fetchLivePullRequestReviewDecision(env, repoFullName, pr.number, token, admissionKey),
   ]);
   const requiredContexts = requiredContextsLookup.requiredContexts;
-  const ciAggregate = await refreshLiveCiAggregate(
+  // Same reuse-this-pass-else-refresh-live rationale as reuseOrRefreshLiveMergeState above (#4498).
+  const ciAggregate = await reuseOrRefreshLiveCiAggregate(
     env,
     repoFullName,
     args.liveFacts,

test/unit/queue.test.ts

@@ -18372,6 +18372,120 @@ describe("queue processors", () => {
     }
   });
 
+  it("INVARIANT (#4498): the disposition planner reuses the public surface's own live mergeable_state/CI read instead of re-fetching a third time", async () => {
+    const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), GITTENSORY_REVIEW_UNIFIED_COMMENT: "1" });
+    await persistRegistrySnapshot(
+      env,
+      normalizeRegistryPayload(
+        { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } },
+        { kind: "raw-github", url: "https://example.test" },
+        "2026-05-23T00:00:00.000Z",
+      ),
+    );
+    await upsertRepositorySettings(env, {
+      repoFullName: "JSONbored/gittensory",
+      commentMode: "detected_contributors_only",
+      publicAudienceMode: "gittensor_only",
+      publicSignalLevel: "standard",
+      publicSurface: "comment_and_label",
+      autoLabelEnabled: false,
+      checkRunMode: "off",
+      checkRunDetailLevel: "minimal",
+      gateCheckMode: "enabled",
+      backfillEnabled: true,
+      autonomy: { update_branch: "auto" },
+    });
+    let mergeableStateReads = 0;
+    // No mockRejectedValueOnce here -- unlike the "renders the unified PR-review comment" test above, every call
+    // succeeds identically, isolating the "both refreshes succeed" case this fix targets (a prior-call failure
+    // legitimately forces a genuine second live read, which is a different, already-covered scenario).
+    const liveCiSpy = vi.spyOn(backfillModule, "fetchLiveCiAggregatePreferGraphQl").mockResolvedValue({
+      ciState: "passed",
+      hasPending: false,
+      hasVisiblePending: false,
+      hasMissingRequiredContext: false,
+      failingDetails: [],
+      nonRequiredFailingDetails: [],
+      ciCompletenessWarning: null,
+    });
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {
+      const url = input.toString();
+      const method = init?.method ?? "GET";
+      // commentMode: "detected_contributors_only" requires the author to actually resolve as a detected
+      // Gittensor contributor for the unified-comment (and its live merge-state/CI refresh) code path to
+      // engage at all -- an empty miner match here would silently skip that whole block, same as the
+      // original "renders the unified PR-review comment" test's fixture this one is adapted from.
+      if (url === "https://api.gittensor.io/miners") {
+        return Response.json([
+          { uid: 7, githubUsername: "oktofeesh1", githubId: "123", totalPrs: 4, totalMergedPrs: 3, totalOpenPrs: 1, totalClosedPrs: 0, totalOpenIssues: 0, totalClosedIssues: 0, totalSolvedIssues: 0, totalValidSolvedIssues: 0, isEligible: true, credibility: 1, eligibleRepoCount: 1, hotkey: "must-not-leak" },
+        ]);
+      }
+      if (url === "https://api.gittensor.io/miners/123") {
+        return Response.json({
+          repositories: [
+            { repositoryFullName: "JSONbored/gittensory", totalPrs: "4", totalMergedPrs: "3", totalOpenPrs: "1", totalClosedPrs: "0", totalOpenIssues: "0", totalClosedIssues: "0", isEligible: true, credibility: "1.000000" },
+          ],
+        });
+      }
+      if (url === "https://api.gittensor.io/miners/123/prs") return Response.json([]);
+      if (url === "https://mirror.gittensor.io/api/v1/miners/123/issues") return Response.json({ issues: [] });
+      if (url.endsWith("/users/oktofeesh1")) return Response.json({ login: "oktofeesh1", public_repos: 2, followers: 1 });
+      if (url.includes("/users/oktofeesh1/repos")) return Response.json([{ language: "TypeScript" }]);
+      if (url.includes("/access_tokens")) return Response.json({ token: "installation-token", expires_at: "2026-05-28T00:04:00.000Z" });
+      if (url.includes("/pulls/3/files")) return Response.json([{ filename: "src/cache.ts", additions: 5, deletions: 1, status: "modified" }]);
+      if (/\/pulls\/3(?:\?|$)/.test(url) && method === "GET") {
+        mergeableStateReads += 1;
+        return Response.json({ number: 3, mergeable_state: "clean" });
+      }
+      if (url.includes("/check-runs") && method === "GET") return Response.json({ total_count: 0, check_runs: [] });
+      if (url.includes("/check-runs") && method === "POST") return Response.json({ id: 901 }, { status: 201 });
+      if (url.includes("/check-runs/901") && method === "PATCH") return Response.json({ id: 901 });
+      if (url.includes("/issues/3/comments") && method === "GET") return Response.json([]);
+      if (url.includes("/issues/3/comments") && method === "POST") return Response.json({ id: 1, html_url: "https://github.com/comment/1" }, { status: 201 });
+      return new Response("not found", { status: 404 });
+    });
+
+    try {
+      await processJob(env, {
+        type: "github-webhook",
+        deliveryId: "pr-single-live-fetch",
+        eventName: "pull_request",
+        payload: {
+          action: "synchronize",
+          installation: {
+            id: 123,
+            account: { login: "JSONbored", id: 1, type: "User" },
+            repository_selection: "selected",
+            permissions: { metadata: "read", pull_requests: "read", issues: "write", checks: "write" },
+            events: ["issues", "issue_comment", "pull_request", "repository", "installation_repositories"],
+          },
+          repository: { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } },
+          pull_request: {
+            number: 3,
+            title: "Single live fetch per pass",
+            state: "open",
+            user: { login: "oktofeesh1" },
+            head: { sha: "singlefetch123" },
+            labels: [{ name: "bug" }],
+            body: "Fixes #1\n\nValidation: npm test",
+          },
+        },
+      });
+
+      // 2, not 3: readiness's own cachedLiveMergeState/cachedLiveCiAggregate check contributes ONE legitimate,
+      // unrelated live read each (a genuine durable-cache miss on this never-before-seen head, unaffected by
+      // this fix), and maybePublishPrPublicSurface's own forced refresh contributes the other -- reused
+      // directly by the disposition planner instead of re-fetched a third time. Verified empirically: reverting
+      // this fix on this exact fixture produces 3 of each, confirming the fix removes exactly the redundant
+      // third call, not readiness's separate, necessary one.
+      expect(mergeableStateReads).toBe(2);
+      const installationTokenCiReads = liveCiSpy.mock.calls.filter(([, , , token]) => token === "installation-token");
+      expect(installationTokenCiReads).toHaveLength(2);
+    } finally {
+      liveCiSpy.mockRestore();
+    }
+  });
+
   // #3609/#3610: same fixture as the unified-comment test above (screenshotsAllowed needs both the global flag
   // AND the repo cutover allowlist — createTestEnv already defaults GITTENSORY_REVIEW_REPOS to include this
   // repo), but the changed file is WEB-VISIBLE (isVisualPath) so the capture pipeline actually fires, proving