Skip to content

Commit f4e69f4

Browse files
committed
fix(review): verify linked issue closure source
1 parent 7bbf529 commit f4e69f4

3 files changed

Lines changed: 139 additions & 13 deletions

File tree

src/github/backfill.ts

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3230,6 +3230,33 @@ export async function fetchLivePullRequestMergedAt(
32303230
return result === undefined ? undefined : (result.data.merged_at ?? null);
32313231
}
32323232

3233+
export type LinkedIssueClosureByPullRequestResult = "closed_by_pull_request" | "not_closed_by_pull_request" | "fetch_error";
3234+
3235+
function timelineEventClosesIssueFromPullRequest(
3236+
event: { event?: string | null; source?: { issue?: { number?: number | null; pull_request?: unknown } | null } | null },
3237+
prNumber: number,
3238+
): boolean {
3239+
return event.event === "closed" && event.source?.issue?.number === prNumber && event.source.issue.pull_request !== undefined;
3240+
}
3241+
3242+
/** Verifies whether GitHub's issue timeline attributes this issue close to the specific PR. Timestamp ordering
3243+
* alone only proves the issue closed after the PR merged; the timeline's closing-reference source binds the
3244+
* closure to THIS PR and prevents borrowing labels from an unrelated issue that happened to close later. */
3245+
export async function fetchLinkedIssueClosedByPullRequest(
3246+
env: Env,
3247+
repoFullName: string,
3248+
issueNumber: number,
3249+
prNumber: number,
3250+
token: string | undefined,
3251+
admissionKey?: GitHubRateLimitAdmissionKey,
3252+
): Promise<LinkedIssueClosureByPullRequestResult> {
3253+
const result = await githubJsonWithHeaders<
3254+
Array<{ event?: string | null; source?: { issue?: { number?: number | null; pull_request?: unknown } | null } | null }>
3255+
>(env, repoFullName, `/issues/${issueNumber}/timeline?per_page=100`, token, githubRateLimitOptions(admissionKey)).catch(() => undefined);
3256+
if (result === undefined) return "fetch_error";
3257+
return result.data.some((event) => timelineEventClosesIssueFromPullRequest(event, prNumber)) ? "closed_by_pull_request" : "not_closed_by_pull_request";
3258+
}
3259+
32333260
/** The issue's LIVE state ("open" / "closed") via REST `GET /issues/{n}`. Mirrors {@link fetchLivePullRequestState}
32343261
* for issues: the stored open-issue cache lags GitHub, so a sibling closed on GitHub (or elsewhere) can still
32353262
* read `open` locally. The per-contributor open-issue cap (#2479 gate finding) confirms each counted sibling's

src/review/linked-issue-label-propagation-fetch.ts

Lines changed: 39 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,10 @@
1-
import { fetchLinkedIssueFacts, fetchLivePullRequestMergedAt, type LinkedIssueFactsFetch, type LinkedIssueFactsResult } from "../github/backfill";
1+
import {
2+
fetchLinkedIssueClosedByPullRequest,
3+
fetchLinkedIssueFacts,
4+
fetchLivePullRequestMergedAt,
5+
type LinkedIssueFactsFetch,
6+
type LinkedIssueFactsResult,
7+
} from "../github/backfill";
28
import { createInstallationToken, getRepositoryCollaboratorPermission } from "../github/app";
39
import { githubRateLimitAdmissionKeyForToken, type GitHubRateLimitAdmissionKey } from "../github/client";
410
import { parseGitHubLoginList } from "../auth/security";
@@ -68,17 +74,13 @@ async function isRepoMaintainerLogin(env: Env, installationId: number, repoFullN
6874
return permission != null && new Set(["admin", "maintain", "write"]).has(permission) ? "maintainer" : "not_maintainer";
6975
}
7076

71-
/** True when the linked issue's authority for propagation can be trusted (#4528): it's still OPEN, or it
72-
* was closed no earlier than THIS PR's own merge. Merging a PR whose body says "Closes #N" auto-closes
73-
* issue #N as an immediate side effect of that same merge -- so `closedAt >= prMergedAt` is exactly the
74-
* signature of "this merge is what closed it," the single most authoritative moment for propagation to
75-
* fire, not a weaker one. An issue closed BEFORE this PR ever merged (`closedAt < prMergedAt`) is the
76-
* gaming case the OPEN-only check originally existed to block -- a PR opportunistically referencing some
77-
* unrelated, already-resolved issue to borrow its label -- and stays blocked, unchanged. `prMergedAt`
78-
* absent (PR not yet merged) never trusts a closed issue, also unchanged. */
79-
function isLinkedIssueTrustworthy(facts: LinkedIssueFactsResult, prMergedAt: string | null): boolean {
77+
function linkedIssueNeedsClosureVerification(facts: LinkedIssueFactsResult, prMergedAt: string | null): boolean {
78+
return facts.state !== "open" && prMergedAt !== null && facts.closedAt !== null && facts.closedAt >= prMergedAt;
79+
}
80+
81+
function isLinkedIssueTrustworthy(facts: LinkedIssueFactsResult, prMergedAt: string | null, closedByThisPr: boolean): boolean {
8082
if (facts.state === "open") return true;
81-
return prMergedAt !== null && facts.closedAt !== null && facts.closedAt >= prMergedAt;
83+
return linkedIssueNeedsClosureVerification(facts, prMergedAt) && closedByThisPr;
8284
}
8385

8486
/** {@link resolveIssueLabelsForPropagation}'s and {@link fetchLinkedIssueLabelsForPropagation}'s return shape
@@ -172,7 +174,31 @@ async function resolveIssueLabelsForPropagation(
172174
}
173175
trustedMergedAt = liveMergedAt;
174176
}
175-
if (!isLinkedIssueTrustworthy(result.facts, trustedMergedAt)) return { labels: [], inconclusive: false };
177+
let closedByThisPr = false;
178+
if (linkedIssueNeedsClosureVerification(result.facts, trustedMergedAt)) {
179+
if (args.prNumber === undefined) return { labels: [], inconclusive: false };
180+
const closure = await fetchLinkedIssueClosedByPullRequest(
181+
args.env,
182+
args.repoFullName,
183+
result.facts.number,
184+
args.prNumber,
185+
args.token,
186+
args.admissionKey,
187+
);
188+
if (closure === "fetch_error") {
189+
console.log(
190+
JSON.stringify({
191+
event: "linked_issue_label_propagation_inconclusive",
192+
repoFullName: args.repoFullName,
193+
issueNumber: result.facts.number,
194+
reason: "issue_closure_timeline_check_failed",
195+
}),
196+
);
197+
return { labels: [], inconclusive: true };
198+
}
199+
closedByThisPr = closure === "closed_by_pull_request";
200+
}
201+
if (!isLinkedIssueTrustworthy(result.facts, trustedMergedAt, closedByThisPr)) return { labels: [], inconclusive: false };
176202
const allLabels = result.facts.labels;
177203
const issueAuthorLogin = result.facts.authorLogin?.toLowerCase();
178204
const assignees = result.facts.assignees.map((login) => login.toLowerCase());
@@ -212,7 +238,7 @@ async function resolveIssueLabelsForPropagation(
212238

213239
/** FETCH every linked issue's labels (fail-open) and flatten into one label list for
214240
* `resolvePrTypeLabel` (`src/settings/pr-type-label.ts`) to match against. Only an OPEN issue, or one
215-
* closed no earlier than THIS PR's own merge (#4528, {@link isLinkedIssueTrustworthy}), can contribute
241+
* closed by THIS PR as verified from GitHub's timeline (#4528, {@link isLinkedIssueTrustworthy}), can contribute
216242
* labels; closing-keyword text in a PR body is author-controlled and is not authority by itself. Mirrors
217243
* `resolveLinkedIssueHardRule`'s own fetch idiom (`src/review/linked-issue-hard-rules.ts`): a per-issue
218244
* fetch failure contributes no labels rather than throwing, so if EVERY linked issue fails, `labels` is

test/unit/linked-issue-label-propagation-fetch.test.ts

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -300,6 +300,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", (
300300
user: { login: "contrib" },
301301
labels: ["gittensor:feature", "gittensor:priority"],
302302
});
303+
if (url.includes("/issues/4279/timeline")) return Response.json([{ event: "closed", source: { issue: { number: 4494, pull_request: {} } } }]);
303304
return new Response("not found", { status: 404 });
304305
});
305306
const env = createTestEnv({});
@@ -310,10 +311,81 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", (
310311
installationId: 123,
311312
prAuthorLogin: "contrib",
312313
prMergedAt: "2026-07-09T22:15:13Z",
314+
prNumber: 4494,
313315
});
314316
expectPropagation(result, ["gittensor:feature", "gittensor:priority"]);
315317
});
316318

319+
it("REGRESSION (#closed-issue-timestamp-spoof): does NOT propagate when an unrelated issue closed after this PR merged", async () => {
320+
stubFetch((url) => {
321+
if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
322+
if (url.endsWith("/issues/9001"))
323+
return Response.json({
324+
number: 9001,
325+
state: "closed",
326+
closed_at: "2026-07-09T22:15:14Z",
327+
user: { login: "contrib" },
328+
labels: ["gittensor:feature", "gittensor:priority"],
329+
});
330+
if (url.includes("/issues/9001/timeline")) return Response.json([{ event: "closed", source: { issue: { number: 123, pull_request: {} } } }]);
331+
return new Response("not found", { status: 404 });
332+
});
333+
const env = createTestEnv({});
334+
const result = await fetchLinkedIssueLabelsForPropagation({
335+
env,
336+
repoFullName: "owner/repo",
337+
linkedIssues: [9001],
338+
installationId: 123,
339+
prAuthorLogin: "contrib",
340+
prMergedAt: "2026-07-09T22:15:13Z",
341+
prNumber: 4494,
342+
});
343+
expectPropagation(result, []);
344+
});
345+
346+
it("does not propagate a timestamp-eligible closed issue when the caller cannot identify this PR number", async () => {
347+
const fetchSpy = vi.fn(async (input: RequestInfo | URL) => {
348+
const url = input.toString();
349+
if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
350+
if (url.endsWith("/issues/9003"))
351+
return Response.json({ number: 9003, state: "closed", closed_at: "2026-07-09T22:15:14Z", user: { login: "contrib" }, labels: ["gittensor:priority"] });
352+
return new Response("not found", { status: 404 });
353+
});
354+
vi.stubGlobal("fetch", fetchSpy);
355+
const env = createTestEnv({});
356+
const result = await fetchLinkedIssueLabelsForPropagation({
357+
env,
358+
repoFullName: "owner/repo",
359+
linkedIssues: [9003],
360+
installationId: 123,
361+
prAuthorLogin: "contrib",
362+
prMergedAt: "2026-07-09T22:15:13Z",
363+
});
364+
expectPropagation(result, []);
365+
expect(fetchSpy.mock.calls.some(([input]) => input.toString().includes("/timeline"))).toBe(false);
366+
});
367+
368+
it("flags closed issue propagation inconclusive when the timeline closure check fails", async () => {
369+
stubFetch((url) => {
370+
if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
371+
if (url.endsWith("/issues/9002"))
372+
return Response.json({ number: 9002, state: "closed", closed_at: "2026-07-09T22:15:14Z", user: { login: "contrib" }, labels: ["gittensor:priority"] });
373+
if (url.includes("/issues/9002/timeline")) return new Response("server error", { status: 500 });
374+
return new Response("not found", { status: 404 });
375+
});
376+
const env = createTestEnv({});
377+
const result = await fetchLinkedIssueLabelsForPropagation({
378+
env,
379+
repoFullName: "owner/repo",
380+
linkedIssues: [9002],
381+
installationId: 123,
382+
prAuthorLogin: "contrib",
383+
prMergedAt: "2026-07-09T22:15:13Z",
384+
prNumber: 4494,
385+
});
386+
expectPropagation(result, [], true);
387+
});
388+
317389
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 () => {
318390
stubFetch((url) => {
319391
if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" });
@@ -372,6 +444,7 @@ describe("fetchLinkedIssueLabelsForPropagation (#priority-linked-issue-gate)", (
372444
labels: ["gittensor:feature"],
373445
});
374446
if (url.endsWith("/pulls/4818")) return Response.json({ merged_at: "2026-07-11T02:26:24Z" });
447+
if (url.includes("/issues/2192/timeline")) return Response.json([{ event: "closed", source: { issue: { number: 4818, pull_request: {} } } }]);
375448
return new Response("not found", { status: 404 });
376449
});
377450
const env = createTestEnv({});

0 commit comments

Comments
 (0)