From e9f0d6ee55b6699c7871febc2cce5ed3263fa2a8 Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Sun, 12 Jul 2026 03:02:55 -0700 Subject: [PATCH] fix(review): verify linked issue closure source The prior trust predicate accepted any closed linked issue with closed_at >= pr.merged_at -- spoofable by an unrelated issue that happens to close after the PR merges. Adds fetchLinkedIssueClosedByPullRequest, which reads the issue's GitHub timeline and requires the closing event be attributed to THIS pr number, not just timestamp-eligible. Timeline fetch failures are treated as inconclusive (fail-conservative), never silently stripping existing labels. Also stubs the new /timeline endpoint in queue-5.test.ts's shared stubPropagationFetch helper: every existing caller there exercises the legitimate same-PR-close path (never the new spoofing test, which has its own dedicated stub), so the shared helper can attribute every closure to the PR number already in scope -- without this, all of those pre-existing tests silently lost their propagated labels. --- src/github/backfill.ts | 27 +++++++ .../linked-issue-label-propagation-fetch.ts | 52 +++++++++---- ...nked-issue-label-propagation-fetch.test.ts | 73 +++++++++++++++++++ test/unit/queue-5.test.ts | 6 ++ 4 files changed, 145 insertions(+), 13 deletions(-) diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 5d3293a740..a7bf74d2f0 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -3230,6 +3230,33 @@ export async function fetchLivePullRequestMergedAt( return result === undefined ? undefined : (result.data.merged_at ?? null); } +export type LinkedIssueClosureByPullRequestResult = "closed_by_pull_request" | "not_closed_by_pull_request" | "fetch_error"; + +function timelineEventClosesIssueFromPullRequest( + event: { event?: string | null; source?: { issue?: { number?: number | null; pull_request?: unknown } | null } | null }, + prNumber: number, +): boolean { + return event.event === "closed" && event.source?.issue?.number === prNumber && event.source.issue.pull_request !== undefined; +} + +/** Verifies whether GitHub's issue timeline attributes this issue close to the specific PR. Timestamp ordering + * alone only proves the issue closed after the PR merged; the timeline's closing-reference source binds the + * closure to THIS PR and prevents borrowing labels from an unrelated issue that happened to close later. */ +export async function fetchLinkedIssueClosedByPullRequest( + env: Env, + repoFullName: string, + issueNumber: number, + prNumber: number, + token: string | undefined, + admissionKey?: GitHubRateLimitAdmissionKey, +): Promise { + const result = await githubJsonWithHeaders< + Array<{ event?: string | null; source?: { issue?: { number?: number | null; pull_request?: unknown } | null } | null }> + >(env, repoFullName, `/issues/${issueNumber}/timeline?per_page=100`, token, githubRateLimitOptions(admissionKey)).catch(() => undefined); + if (result === undefined) return "fetch_error"; + return result.data.some((event) => timelineEventClosesIssueFromPullRequest(event, prNumber)) ? "closed_by_pull_request" : "not_closed_by_pull_request"; +} + /** 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/review/linked-issue-label-propagation-fetch.ts b/src/review/linked-issue-label-propagation-fetch.ts index f88c00e282..d6b21bb58e 100644 --- a/src/review/linked-issue-label-propagation-fetch.ts +++ b/src/review/linked-issue-label-propagation-fetch.ts @@ -1,4 +1,10 @@ -import { fetchLinkedIssueFacts, fetchLivePullRequestMergedAt, type LinkedIssueFactsFetch, type LinkedIssueFactsResult } from "../github/backfill"; +import { + fetchLinkedIssueClosedByPullRequest, + fetchLinkedIssueFacts, + fetchLivePullRequestMergedAt, + type LinkedIssueFactsFetch, + type LinkedIssueFactsResult, +} from "../github/backfill"; import { createInstallationToken, getRepositoryCollaboratorPermission } from "../github/app"; import { githubRateLimitAdmissionKeyForToken, type GitHubRateLimitAdmissionKey } from "../github/client"; import { parseGitHubLoginList } from "../auth/security"; @@ -68,17 +74,13 @@ async function isRepoMaintainerLogin(env: Env, installationId: number, repoFullN return permission != null && new Set(["admin", "maintain", "write"]).has(permission) ? "maintainer" : "not_maintainer"; } -/** True when the linked issue's authority for propagation can be trusted (#4528): it's still OPEN, or it - * was closed no earlier than THIS PR's own merge. Merging a PR whose body says "Closes #N" auto-closes - * issue #N as an immediate side effect of that same merge -- so `closedAt >= prMergedAt` is exactly the - * signature of "this merge is what closed it," the single most authoritative moment for propagation to - * fire, not a weaker one. An issue closed BEFORE this PR ever merged (`closedAt < prMergedAt`) is the - * gaming case the OPEN-only check originally existed to block -- a PR opportunistically referencing some - * unrelated, already-resolved issue to borrow its label -- and stays blocked, unchanged. `prMergedAt` - * absent (PR not yet merged) never trusts a closed issue, also unchanged. */ -function isLinkedIssueTrustworthy(facts: LinkedIssueFactsResult, prMergedAt: string | null): boolean { +function linkedIssueNeedsClosureVerification(facts: LinkedIssueFactsResult, prMergedAt: string | null): boolean { + return facts.state !== "open" && prMergedAt !== null && facts.closedAt !== null && facts.closedAt >= prMergedAt; +} + +function isLinkedIssueTrustworthy(facts: LinkedIssueFactsResult, prMergedAt: string | null, closedByThisPr: boolean): boolean { if (facts.state === "open") return true; - return prMergedAt !== null && facts.closedAt !== null && facts.closedAt >= prMergedAt; + return linkedIssueNeedsClosureVerification(facts, prMergedAt) && closedByThisPr; } /** {@link resolveIssueLabelsForPropagation}'s and {@link fetchLinkedIssueLabelsForPropagation}'s return shape @@ -172,7 +174,31 @@ async function resolveIssueLabelsForPropagation( } trustedMergedAt = liveMergedAt; } - if (!isLinkedIssueTrustworthy(result.facts, trustedMergedAt)) return { labels: [], inconclusive: false }; + let closedByThisPr = false; + if (linkedIssueNeedsClosureVerification(result.facts, trustedMergedAt)) { + if (args.prNumber === undefined) return { labels: [], inconclusive: false }; + const closure = await fetchLinkedIssueClosedByPullRequest( + args.env, + args.repoFullName, + result.facts.number, + args.prNumber, + args.token, + args.admissionKey, + ); + if (closure === "fetch_error") { + console.log( + JSON.stringify({ + event: "linked_issue_label_propagation_inconclusive", + repoFullName: args.repoFullName, + issueNumber: result.facts.number, + reason: "issue_closure_timeline_check_failed", + }), + ); + return { labels: [], inconclusive: true }; + } + closedByThisPr = closure === "closed_by_pull_request"; + } + if (!isLinkedIssueTrustworthy(result.facts, trustedMergedAt, closedByThisPr)) return { labels: [], inconclusive: false }; const allLabels = result.facts.labels; const issueAuthorLogin = result.facts.authorLogin?.toLowerCase(); const assignees = result.facts.assignees.map((login) => login.toLowerCase()); @@ -212,7 +238,7 @@ async function resolveIssueLabelsForPropagation( /** FETCH every linked issue's labels (fail-open) and flatten into one label list for * `resolvePrTypeLabel` (`src/settings/pr-type-label.ts`) to match against. Only an OPEN issue, or one - * closed no earlier than THIS PR's own merge (#4528, {@link isLinkedIssueTrustworthy}), can contribute + * closed by THIS PR as verified from GitHub's timeline (#4528, {@link isLinkedIssueTrustworthy}), can contribute * labels; closing-keyword text in a PR body is author-controlled and is not authority by itself. Mirrors * `resolveLinkedIssueHardRule`'s own fetch idiom (`src/review/linked-issue-hard-rules.ts`): a per-issue * fetch failure contributes no labels rather than throwing, so if EVERY linked issue fails, `labels` is diff --git a/test/unit/linked-issue-label-propagation-fetch.test.ts b/test/unit/linked-issue-label-propagation-fetch.test.ts index eb2b0f6044..50b4ed503f 100644 --- a/test/unit/linked-issue-label-propagation-fetch.test.ts +++ b/test/unit/linked-issue-label-propagation-fetch.test.ts @@ -300,6 +300,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( user: { login: "contrib" }, labels: ["gittensor:feature", "gittensor:priority"], }); + if (url.includes("/issues/4279/timeline")) return Response.json([{ event: "closed", source: { issue: { number: 4494, pull_request: {} } } }]); return new Response("not found", { status: 404 }); }); const env = createTestEnv({}); @@ -310,10 +311,81 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( installationId: 123, prAuthorLogin: "contrib", prMergedAt: "2026-07-09T22:15:13Z", + prNumber: 4494, }); expectPropagation(result, ["gittensor:feature", "gittensor:priority"]); }); + it("REGRESSION (#closed-issue-timestamp-spoof): does NOT propagate when an unrelated issue closed after this PR merged", async () => { + stubFetch((url) => { + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/issues/9001")) + return Response.json({ + number: 9001, + state: "closed", + closed_at: "2026-07-09T22:15:14Z", + user: { login: "contrib" }, + labels: ["gittensor:feature", "gittensor:priority"], + }); + if (url.includes("/issues/9001/timeline")) return Response.json([{ event: "closed", source: { issue: { number: 123, pull_request: {} } } }]); + return new Response("not found", { status: 404 }); + }); + const env = createTestEnv({}); + const result = await fetchLinkedIssueLabelsForPropagation({ + env, + repoFullName: "owner/repo", + linkedIssues: [9001], + installationId: 123, + prAuthorLogin: "contrib", + prMergedAt: "2026-07-09T22:15:13Z", + prNumber: 4494, + }); + expectPropagation(result, []); + }); + + it("does not propagate a timestamp-eligible closed issue when the caller cannot identify this PR number", 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/9003")) + return Response.json({ number: 9003, state: "closed", closed_at: "2026-07-09T22:15:14Z", 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: [9003], + installationId: 123, + prAuthorLogin: "contrib", + prMergedAt: "2026-07-09T22:15:13Z", + }); + expectPropagation(result, []); + expect(fetchSpy.mock.calls.some(([input]) => input.toString().includes("/timeline"))).toBe(false); + }); + + it("flags closed issue propagation inconclusive when the timeline closure check fails", async () => { + stubFetch((url) => { + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.endsWith("/issues/9002")) + return Response.json({ number: 9002, state: "closed", closed_at: "2026-07-09T22:15:14Z", user: { login: "contrib" }, labels: ["gittensor:priority"] }); + if (url.includes("/issues/9002/timeline")) 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: [9002], + installationId: 123, + prAuthorLogin: "contrib", + prMergedAt: "2026-07-09T22:15:13Z", + prNumber: 4494, + }); + expectPropagation(result, [], true); + }); + it("does NOT propagate when the linked issue was already closed BEFORE this PR merged (anti-gaming: an unrelated, already-resolved issue can't be borrowed)", async () => { stubFetch((url) => { if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); @@ -372,6 +444,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", ( labels: ["gittensor:feature"], }); if (url.endsWith("/pulls/4818")) return Response.json({ merged_at: "2026-07-11T02:26:24Z" }); + if (url.includes("/issues/2192/timeline")) return Response.json([{ event: "closed", source: { issue: { number: 4818, pull_request: {} } } }]); return new Response("not found", { status: 404 }); }); const env = createTestEnv({}); diff --git a/test/unit/queue-5.test.ts b/test/unit/queue-5.test.ts index f465cbaf0b..cc2eff6834 100644 --- a/test/unit/queue-5.test.ts +++ b/test/unit/queue-5.test.ts @@ -6222,6 +6222,12 @@ describe("queue processors", () => { seen.issueFetches += 1; return linkedIssueResponse(); } + // #4528 timeline attribution (#closed-issue-timestamp-spoof): every existing caller here exercises the + // legitimate "this PR's own merge closed the linked issue" trust path, never the spoofing case (which + // gets its own dedicated stub) -- so the shared helper can safely attribute every closure to this PR. + if (url.includes(`/issues/${linkedIssueNumber}/timeline`)) { + return Response.json([{ event: "closed", source: { issue: { number: prNumber, pull_request: {} } } }]); + } if (url.includes(`/issues/${prNumber}/labels`) && method === "GET") return Response.json([]); if (url.includes(`/issues/${prNumber}/labels`) && method === "POST") { seen.posted.push(...((JSON.parse(String(init?.body ?? "{}")).labels ?? []) as string[]));