From 3b6e194e8ae964ed4520774eb18677d839becc0a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 9 Jul 2026 16:25:25 -0700 Subject: [PATCH] fix(review): reuse the live mergeable_state/CI read across a single pass reReviewStoredPullRequest / the direct pull_request webhook handler both thread ONE shared LiveGithubFacts object through readiness, maybePublishPrPublicSurface, and runAgentMaintenancePlanAndExecute -- but the latter two independently force-refetched the same PR's mergeable_state and CI aggregate live from GitHub, back to back, with no mutation between the two reads. Adds reuseOrRefreshLiveMergeState/reuseOrRefreshLiveCiAggregate, which reuse a value already populated by a FORCED write earlier in the same pass instead of re-fetching. Deliberately not the existing cachedLiveMergeState/ cachedLiveCiAggregate variants: those fall through to the durable cross-webhook cache on a request-local miss, which can replay an older webhook's snapshot -- exactly what the disposition input's #4220 invariant prohibits. Tracks which keys were populated by a forced (genuinely-live- this-pass) write via new forcedMergeStateKeys/forcedCiAggregateKeys sets on LiveGithubFacts, so a value written by the READINESS path's own cache-preferring reader is never mistaken for a fresh one. Closes #4498. --- src/queue/processors.ts | 64 ++++++++++++++++++++-- test/unit/queue.test.ts | 114 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 175 insertions(+), 3 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index d2ee69c232..c0256c5448 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -613,6 +613,14 @@ interface LiveGithubFacts { requiredContexts: Map>; ciAggregates: Map>; mergeStates: Map>; + // #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; + forcedMergeStateKeys: Set; } function createLiveGithubFacts(): LiveGithubFacts { @@ -620,6 +628,8 @@ function createLiveGithubFacts(): LiveGithubFacts { 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 { + 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 | null | undefined, + admissionKey?: GitHubRateLimitAdmissionKey, +): Promise { + 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,8 +2760,10 @@ 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 @@ -2714,7 +2771,8 @@ async function runAgentMaintenancePlanAndExecute( 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, diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 8f3b7d1956..790e307359 100644 --- a/test/unit/queue.test.ts +++ b/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