diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 34dd9d200d..5d3293a740 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -3208,6 +3208,28 @@ export async function fetchLivePullRequestState( return result?.data.state ?? undefined; } +/** The PR's LIVE `merged_at` via REST `GET /pulls/{n}` (#4818): a webhook whose embedded `pull_request` snapshot + * predates an imminent merge -- e.g. a `pull_request_review`/`pull_request_review_comment`/ + * `pull_request_review_thread` fired a few ms before an "approve and merge" action -- can carry `merged_at: + * null` even though the PR is, by the time this pass actually runs, genuinely merged; + * `handlePullRequestWebhookEvent` never re-verifies the webhook-embedded snapshot it built `pr` from. Used + * ONLY to resolve that one ambiguous case in `resolveIssueLabelsForPropagation` + * (`review/linked-issue-label-propagation-fetch.ts`) -- a CLOSED linked issue whose closure can't yet be + * attributed to this PR from the triggering webhook's own (possibly stale) `merged_at` alone. Best-effort: + * returns undefined on any error, distinct from the confirmed `null` of a genuinely-still-open PR, so the + * caller treats a fetch failure as inconclusive rather than folding it into a confirmed negative (never fails + * toward silently stripping a correct label). */ +export async function fetchLivePullRequestMergedAt( + env: Env, + repoFullName: string, + prNumber: number, + token: string | undefined, + admissionKey?: GitHubRateLimitAdmissionKey, +): Promise { + const result = await githubJsonWithHeaders<{ merged_at?: string | null }>(env, repoFullName, `/pulls/${prNumber}`, token, githubRateLimitOptions(admissionKey)).catch(() => undefined); + return result === undefined ? undefined : (result.data.merged_at ?? null); +} + /** The issue's LIVE state ("open" / "closed") via REST `GET /issues/{n}`. Mirrors {@link fetchLivePullRequestState} * for issues: the stored open-issue cache lags GitHub, so a sibling closed on GitHub (or elsewhere) can still * read `open` locally. The per-contributor open-issue cap (#2479 gate finding) confirms each counted sibling's diff --git a/src/queue/processors.ts b/src/queue/processors.ts index d966e469a5..828e74ff71 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -7620,6 +7620,11 @@ async function maybePublishPrPublicSurface( // (the standard "Closes #N" auto-close), instead of losing propagation authority the instant // the merge that's supposed to earn the label also closes its evidence. prMergedAt: pr.mergedAt ?? null, + // #4818: lets the ambiguous "issue closed but THIS pass's own prMergedAt reads null" case + // (a pull_request_review/_comment/_thread webhook whose embedded snapshot predates an + // imminent merge, delayed behind other queued work) resolve via one fresh live check instead + // of silently downgrading a correct label. + prNumber: pr.number, }) : { labels: [], inconclusive: false }; // #regression-safe-propagation: an INCONCLUSIVE recheck (the linked issue's facts or the diff --git a/src/review/linked-issue-label-propagation-fetch.ts b/src/review/linked-issue-label-propagation-fetch.ts index 6167a9bdf3..f88c00e282 100644 --- a/src/review/linked-issue-label-propagation-fetch.ts +++ b/src/review/linked-issue-label-propagation-fetch.ts @@ -1,6 +1,6 @@ -import { fetchLinkedIssueFacts, type LinkedIssueFactsFetch, type LinkedIssueFactsResult } from "../github/backfill"; +import { fetchLinkedIssueFacts, fetchLivePullRequestMergedAt, type LinkedIssueFactsFetch, type LinkedIssueFactsResult } from "../github/backfill"; import { createInstallationToken, getRepositoryCollaboratorPermission } from "../github/app"; -import { githubRateLimitAdmissionKeyForToken } from "../github/client"; +import { githubRateLimitAdmissionKeyForToken, type GitHubRateLimitAdmissionKey } from "../github/client"; import { parseGitHubLoginList } from "../auth/security"; import { errorMessage } from "../utils/json"; import type { LinkedIssueLabelPropagationMapping } from "../types"; @@ -120,7 +120,14 @@ export type LinkedIssuePropagationLabels = { * confirmed `not_found` (a proven-nonexistent issue) is likewise NOT inconclusive -- that is real, * deterministic evidence, not a hiccup. */ async function resolveIssueLabelsForPropagation( - args: { env: Env; repoFullName: string; installationId: number }, + args: { + env: Env; + repoFullName: string; + installationId: number; + prNumber: number | undefined; + token: string | undefined; + admissionKey: GitHubRateLimitAdmissionKey | undefined; + }, result: LinkedIssueFactsFetch, prAuthorLogin: string | undefined, relaxableLabels: ReadonlySet, @@ -136,7 +143,36 @@ async function resolveIssueLabelsForPropagation( ); return { labels: [], inconclusive: true }; } - if (result.status !== "found" || !isLinkedIssueTrustworthy(result.facts, prMergedAt) || !prAuthorLogin) return { labels: [], inconclusive: false }; + if (result.status !== "found" || !prAuthorLogin) return { labels: [], inconclusive: false }; + let trustedMergedAt = prMergedAt; + // #4818 (#regression-safe-propagation): a null `prMergedAt` on a CLOSED linked issue is AMBIGUOUS, not a + // confirmed negative -- it means either "this PR genuinely isn't merged yet" (the real anti-gaming case + // #4528 exists to block: an unrelated, already-resolved issue opportunistically cited by a still-open PR) + // OR "this pass's own triggering webhook happened to be a pull_request_review/_comment/_thread whose + // embedded `pull_request` snapshot was taken a few ms before an imminent merge, then this pass got delayed + // in the queue long enough for the real merge (and the issue's consequent auto-close) to land first." + // Those two cases are indistinguishable from `prMergedAt` alone -- `handlePullRequestWebhookEvent` never + // re-verifies the webhook-embedded snapshot it built `pr` from (`src/queue/processors.ts`). Resolve the + // ambiguity with ONE fresh, authoritative read of THIS PR's own live merge state (never inferred from + // whichever webhook happened to trigger this particular pass) before deciding -- only when the issue is + // confirmed closed with a real `closedAt` (a genuinely still-open issue never reaches here at all, per + // {@link isLinkedIssueTrustworthy}'s own open-state short-circuit) and a PR number is available to check. + if (trustedMergedAt === null && result.facts.state !== "open" && result.facts.closedAt !== null && args.prNumber !== undefined) { + const liveMergedAt = await fetchLivePullRequestMergedAt(args.env, args.repoFullName, args.prNumber, args.token, args.admissionKey); + if (liveMergedAt === undefined) { + console.log( + JSON.stringify({ + event: "linked_issue_label_propagation_inconclusive", + repoFullName: args.repoFullName, + issueNumber: result.facts.number, + reason: "live_merge_state_check_failed", + }), + ); + return { labels: [], inconclusive: true }; + } + trustedMergedAt = liveMergedAt; + } + if (!isLinkedIssueTrustworthy(result.facts, trustedMergedAt)) return { labels: [], inconclusive: false }; const allLabels = result.facts.labels; const issueAuthorLogin = result.facts.authorLogin?.toLowerCase(); const assignees = result.facts.assignees.map((login) => login.toLowerCase()); @@ -197,14 +233,20 @@ async function resolveIssueLabelsForPropagation( * either flag) reproduces today's strict author-or-assignee-only behavior exactly. * * `prMergedAt` (#4528) is this PR's own `merged_at`, or `null` while unmerged -- the caller's `pr.mergedAt` - * straight from the DB row, no extra fetch. + * straight from the DB row (or webhook payload), no extra fetch in the common case. + * + * `prNumber` (#4818, optional) unlocks ONE extra live fetch, only in the narrow ambiguous case + * {@link resolveIssueLabelsForPropagation} documents (a CLOSED linked issue whose closure this pass's own + * `prMergedAt` reads null): omitting it reproduces the pre-#4818 behavior exactly (a confirmed negative, + * never ambiguity-checked) -- production always passes it (`src/queue/processors.ts`'s `pr.number`). * * Returns {@link LinkedIssuePropagationLabels} (#regression-safe-propagation), NOT a bare `string[]`: - * `inconclusive` is true when ANY linked issue's resolution was inconclusive (fetch failure or an errored - * maintainer-permission check), aggregated across every linked issue with a plain OR -- deliberately - * coarse. A caller only needs to distinguish "confirmed: no propagation applies" from "could not fully - * verify this pass" when `labels` came back empty; when even one linked issue resolved with real labels, - * those labels are just as trustworthy as before regardless of a sibling issue's fetch trouble. */ + * `inconclusive` is true when ANY linked issue's resolution was inconclusive (fetch failure, an errored + * maintainer-permission check, or an errored live-merge-state recheck), aggregated across every linked + * issue with a plain OR -- deliberately coarse. A caller only needs to distinguish "confirmed: no + * propagation applies" from "could not fully verify this pass" when `labels` came back empty; when even one + * linked issue resolved with real labels, those labels are just as trustworthy as before regardless of a + * sibling issue's fetch trouble. */ export async function fetchLinkedIssueLabelsForPropagation(args: { env: Env; repoFullName: string; @@ -213,6 +255,7 @@ export async function fetchLinkedIssueLabelsForPropagation(args: { prAuthorLogin: string | null | undefined; mappings?: readonly LinkedIssueLabelPropagationMapping[] | undefined; prMergedAt?: string | null | undefined; + prNumber?: number | undefined; }): Promise { if (args.linkedIssues.length === 0) return { labels: [], inconclusive: false }; const linkedIssues = args.linkedIssues.slice(0, MAX_LINKED_ISSUES_TO_FETCH); @@ -257,7 +300,7 @@ export async function fetchLinkedIssueLabelsForPropagation(args: { const perIssueResults = await Promise.all( results.map((result) => resolveIssueLabelsForPropagation( - { env: args.env, repoFullName: args.repoFullName, installationId: args.installationId }, + { env: args.env, repoFullName: args.repoFullName, installationId: args.installationId, prNumber: args.prNumber, token, admissionKey }, result, prAuthorLogin, relaxableLabels, diff --git a/test/unit/linked-issue-label-propagation-fetch.test.ts b/test/unit/linked-issue-label-propagation-fetch.test.ts index 4bbf02a121..eb2b0f6044 100644 --- a/test/unit/linked-issue-label-propagation-fetch.test.ts +++ b/test/unit/linked-issue-label-propagation-fetch.test.ts @@ -359,6 +359,121 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( }); }); + describe("webhook-race live-recheck (#4818 — a null prMergedAt from a stale webhook snapshot is ambiguous, not confirmed)", () => { + it("REGRESSION (PR #4818 shape): propagates when prMergedAt reads null (a pull_request_review webhook's stale pre-merge snapshot) but a live check confirms the PR is actually merged at/before the issue's closedAt", async () => { + stubFetch((url) => { + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/issues/2192")) + return Response.json({ + number: 2192, + state: "closed", + closed_at: "2026-07-11T02:26:25Z", + user: { login: "owner" }, + labels: ["gittensor:feature"], + }); + if (url.endsWith("/pulls/4818")) return Response.json({ merged_at: "2026-07-11T02:26:24Z" }); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({}); + const result = await fetchLinkedIssueLabelsForPropagation({ + env, + repoFullName: "owner/repo", + linkedIssues: [2192], + installationId: 123, + prAuthorLogin: "contrib", + mappings: [{ issueLabel: "gittensor:feature", prLabel: "gittensor:feature", removeOtherTypeLabels: true, trustMaintainerAuthoredIssue: true }], + prMergedAt: null, + prNumber: 4818, + }); + expectPropagation(result, ["gittensor:feature"]); + }); + + it("does not propagate when the live recheck confirms the PR is genuinely still unmerged (the real anti-gaming case #4528 protects)", async () => { + stubFetch((url) => { + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/issues/777")) + return Response.json({ number: 777, state: "closed", closed_at: "2026-07-01T00:00:00Z", user: { login: "contrib" }, labels: ["gittensor:priority"] }); + if (url.endsWith("/pulls/42")) return Response.json({ merged_at: null }); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({}); + const result = await fetchLinkedIssueLabelsForPropagation({ + env, + repoFullName: "owner/repo", + linkedIssues: [777], + installationId: 123, + prAuthorLogin: "contrib", + prMergedAt: null, + prNumber: 42, + }); + expectPropagation(result, []); + }); + + it("does not propagate when the live recheck confirms the PR merged AFTER the issue's own independent closedAt (still not this PR's own close)", async () => { + stubFetch((url) => { + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/issues/777")) + return Response.json({ number: 777, state: "closed", closed_at: "2026-07-01T00:00:00Z", user: { login: "contrib" }, labels: ["gittensor:priority"] }); + if (url.endsWith("/pulls/42")) return Response.json({ merged_at: "2026-07-05T00:00:00Z" }); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({}); + const result = await fetchLinkedIssueLabelsForPropagation({ + env, + repoFullName: "owner/repo", + linkedIssues: [777], + installationId: 123, + prAuthorLogin: "contrib", + prMergedAt: null, + prNumber: 42, + }); + expectPropagation(result, []); + }); + + it("flags the result inconclusive (never a confirmed absence) when the live merge-state recheck itself fails to fetch", async () => { + stubFetch((url) => { + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/issues/777")) + return Response.json({ number: 777, state: "closed", closed_at: "2026-07-01T00:00:00Z", user: { login: "contrib" }, labels: ["gittensor:priority"] }); + if (url.endsWith("/pulls/42")) return new Response("server error", { status: 500 }); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({}); + const result = await fetchLinkedIssueLabelsForPropagation({ + env, + repoFullName: "owner/repo", + linkedIssues: [777], + installationId: 123, + prAuthorLogin: "contrib", + prMergedAt: null, + prNumber: 42, + }); + expectPropagation(result, [], true); + }); + + it("does not attempt a live recheck (and stays a confirmed negative, byte-identical to pre-#4818 behavior) when the caller omits prNumber", async () => { + const fetchSpy = vi.fn(async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/issues/777")) + return Response.json({ number: 777, state: "closed", closed_at: "2026-07-01T00:00:00Z", user: { login: "contrib" }, labels: ["gittensor:priority"] }); + return new Response("not found", { status: 404 }); + }); + vi.stubGlobal("fetch", fetchSpy); + const env = createTestEnv({}); + const result = await fetchLinkedIssueLabelsForPropagation({ + env, + repoFullName: "owner/repo", + linkedIssues: [777], + installationId: 123, + prAuthorLogin: "contrib", + prMergedAt: null, + }); + expectPropagation(result, []); + expect(fetchSpy.mock.calls.some(([input]) => input.toString().includes("/pulls/"))).toBe(false); + }); + }); + it("does not propagate labels when the PR author is missing", async () => { stubFetch((url) => { if (url.includes("/access_tokens"))