From 446de7759c98691a8c49c5e69d5fbb890ff347d1 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 11:29:56 -0700 Subject: [PATCH 1/2] fix(review): treat a confirmed-nonexistent linked issue as a hard-rule violation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fetchLinkedIssueFacts collapsed a confirmed 404 and a transient fetch failure into the same undefined result. resolveLinkedIssueHardRule then treated an all-undefined fetch batch as "no violation" (fail open) regardless of WHY every fetch came back empty — so a contributor citing a fabricated issue number (e.g. "Fixes #999999") satisfied the "has a linked issue" check and was fully exempt from the ownerAssignedClose/missingPointLabelClose/maintainerOnlyLabelClose hard rules, even on a repo with all three set to block. The codebase already closed this exact evasion class for reference overflow (too-many-refs = a violation, not a silent skip); it was never applied to the all-404 case. fetchLinkedIssueFacts now returns a tri-state result (found / not_found / fetch_error) instead of collapsing both into undefined. resolveLinkedIssueHardRule treats an ALL-not_found fetch batch the same way it already treats overflow: an unsafe-to-verify violation. A single fetch_error in the mix still fails open, since a genuine outage could be masking a real, rule-violating issue. --- src/github/backfill.ts | 56 +++++++++++++++-------- src/queue/processors.ts | 3 +- src/review/linked-issue-hard-rules.ts | 18 +++++++- test/unit/backfill.test.ts | 27 +++++++++++ test/unit/linked-issue-hard-rules.test.ts | 22 +++++++-- 5 files changed, 100 insertions(+), 26 deletions(-) diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 6d73e55a95..adcf51b17f 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -2867,12 +2867,23 @@ export function isOwnReviewThreadAuthor(login: string | null | undefined): boole /** The deterministic linked-issue facts the hard-rule evaluator needs (labels / assignees / open-state). */ export type LinkedIssueFactsResult = { number: number; labels: string[]; assignees: string[]; state: string; authorLogin: string | null }; +/** Tri-state outcome of fetching one linked issue's facts (#2136). `not_found` is a CONFIRMED 404 — GitHub told + * us this issue number does not exist. `fetch_error` is everything else that prevented a read (network, 5xx, + * rate-limit, malformed body) — a genuine outage, not evidence about the issue itself. Callers that treat an + * ALL-not_found result as significant (the linked-issue hard rule) must never extend that same treatment to + * fetch_error, or a GitHub outage would spuriously look like a fabricated reference. */ +export type LinkedIssueFactsFetch = + | { status: "found"; facts: LinkedIssueFactsResult } + | { status: "not_found" } + | { status: "fetch_error" }; + /** - * FETCH the facts for one linked issue via the REST issues endpoint. FAIL-OPEN: any fetch/parse error returns - * undefined so the caller skips that issue — a deterministic auto-close must NEVER fire (or be blocked) on a - * transient fetch failure. Uses the same authenticated REST client + public-token 404-fallback as the other - * live fetches. (Note: GitHub's issues endpoint also returns pull requests, which carry a `pull_request` field; - * a PR number passed here would simply fail the rules — we only treat real issues' labels/assignees.) + * FETCH the facts for one linked issue via the REST issues endpoint. Distinguishes a CONFIRMED-nonexistent + * issue (404) from a transient fetch failure (#2136) — a deterministic auto-close must never fire on a + * transient failure, but a fabricated issue number is real, verifiable information the hard-rule evaluator + * needs. Uses the same authenticated REST client + public-token 404-fallback as the other live fetches. (Note: + * GitHub's issues endpoint also returns pull requests, which carry a `pull_request` field; a PR number passed + * here would simply fail the rules — we only treat real issues' labels/assignees.) */ export async function fetchLinkedIssueFacts( env: Env, @@ -2880,15 +2891,19 @@ export async function fetchLinkedIssueFacts( issueNumber: number, token: string | undefined, admissionKey?: GitHubRateLimitAdmissionKey, -): Promise { - const result = await githubJsonWithHeaders<{ - number?: number; - state?: string | null; - labels?: Array<{ name?: string | null } | string | null> | null; - assignees?: Array<{ login?: string | null } | null> | null; - user?: { login?: string | null } | null; - }>(env, repoFullName, `/issues/${issueNumber}`, token, githubRateLimitOptions(admissionKey)).catch(() => undefined); - if (!result) return undefined; +): Promise { + let result; + try { + result = await githubJsonWithHeaders<{ + number?: number; + state?: string | null; + labels?: Array<{ name?: string | null } | string | null> | null; + assignees?: Array<{ login?: string | null } | null> | null; + user?: { login?: string | null } | null; + }>(env, repoFullName, `/issues/${issueNumber}`, token, githubRateLimitOptions(admissionKey)); + } catch (error) { + return error instanceof GitHubApiError && error.statusCode === 404 ? { status: "not_found" } : { status: "fetch_error" }; + } const data = result.data; const labels = (data.labels ?? []).flatMap((label) => { if (typeof label === "string") return label.length > 0 ? [label] : []; @@ -2896,11 +2911,14 @@ export async function fetchLinkedIssueFacts( }); const assignees = (data.assignees ?? []).flatMap((assignee) => (assignee?.login ? [assignee.login] : [])); return { - number: data.number ?? issueNumber, - labels, - assignees, - state: String(data.state ?? "open").toLowerCase(), - authorLogin: data.user?.login ?? null, + status: "found", + facts: { + number: data.number ?? issueNumber, + labels, + assignees, + state: String(data.state ?? "open").toLowerCase(), + authorLogin: data.user?.login ?? null, + }, }; } diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 7f32dfc58b..09df7b90b1 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -3761,8 +3761,7 @@ export async function resolveLinkedIssueAuthorLogins( login != null ? Promise.resolve(login) : fetchLinkedIssueFacts(env, repoFullName, linkedIssues[index]!, token, admissionKey) - .then((facts) => facts?.authorLogin ?? null) - .catch(() => null), + .then((result) => (result.status === "found" ? result.facts.authorLogin : null)), ), ); } diff --git a/src/review/linked-issue-hard-rules.ts b/src/review/linked-issue-hard-rules.ts index 683350c04d..e3b080a49d 100644 --- a/src/review/linked-issue-hard-rules.ts +++ b/src/review/linked-issue-hard-rules.ts @@ -173,7 +173,21 @@ export async function resolveLinkedIssueHardRule(args: { if (args.linkedIssues.length === 0) return undefined; const token = args.ciToken ?? args.env.GITHUB_PUBLIC_TOKEN; const admissionKey = githubRateLimitAdmissionKeyForToken(args.env, token, args.installationId); - const issueFacts = (await Promise.all(args.linkedIssues.map((issueNumber) => fetchLinkedIssueFacts(args.env, args.repoFullName, issueNumber, token, admissionKey)))).flatMap((facts) => (facts ? [facts] : [])); - if (issueFacts.length === 0) return undefined; + const fetchResults = await Promise.all(args.linkedIssues.map((issueNumber) => fetchLinkedIssueFacts(args.env, args.repoFullName, issueNumber, token, admissionKey))); + const issueFacts = fetchResults.flatMap((result) => (result.status === "found" ? [result.facts] : [])); + if (issueFacts.length === 0) { + // Every reference resolved to a CONFIRMED 404 — never a transient fetch_error (#2136). Mirrors the overflow + // treatment above: a contributor citing a fabricated issue number must not silently satisfy the hard rule + // the same way a genuinely-linked-but-unfetchable issue fails open. A single fetch_error in the mix still + // fails open (we cannot rule out a real, rule-violating issue behind that failure). + const allConfirmedNotFound = fetchResults.every((result) => result.status === "not_found"); + if (allConfirmedNotFound) { + return { + violated: true, + reason: "The linked issue reference could not be found — please link a real, open issue or request maintainer review.", + }; + } + return undefined; + } return evaluateLinkedIssueHardRules({ issues: issueFacts, config: args.config, repoOwner: args.repoOwner }); } diff --git a/test/unit/backfill.test.ts b/test/unit/backfill.test.ts index 3a88fecac4..8ead9843cc 100644 --- a/test/unit/backfill.test.ts +++ b/test/unit/backfill.test.ts @@ -34,6 +34,7 @@ import { enqueueRepositoryOpenDataBackfill, enrichInstallationHealth, fetchAndStorePullRequestFilesForReview, + fetchLinkedIssueFacts, fetchLiveCiAggregate, fetchLiveReviewThreadBlockers, fetchRequiredStatusContexts, @@ -4846,6 +4847,32 @@ describe("GitHub backfill", () => { }); }); + describe("fetchLinkedIssueFacts (#2136)", () => { + it("returns a found result with the extracted facts, falling back to the requested number and open state when the payload omits them", async () => { + const env = createTestEnv({}); + // Sparse payload: no `number`, no `state` — exercises the `data.number ?? issueNumber` and + // `data.state ?? "open"` defensive fallbacks. + vi.stubGlobal("fetch", async () => Response.json({ labels: [{ name: "bug" }, "manual-string-label"], assignees: [{ login: "maintainer" }], user: { login: "reporter" } })); + const result = await fetchLinkedIssueFacts(env, "JSONbored/gittensory", 42, "tok"); + expect(result).toEqual({ + status: "found", + facts: { number: 42, labels: ["bug", "manual-string-label"], assignees: ["maintainer"], state: "open", authorLogin: "reporter" }, + }); + }); + + it("returns not_found on a confirmed 404, distinct from a transient fetch error", async () => { + const env = createTestEnv({}); + vi.stubGlobal("fetch", async () => new Response("missing", { status: 404 })); + expect(await fetchLinkedIssueFacts(env, "JSONbored/gittensory", 999999, "tok")).toEqual({ status: "not_found" }); + }); + + it("returns fetch_error on a transient failure (5xx), never conflating it with not_found", async () => { + const env = createTestEnv({}); + vi.stubGlobal("fetch", async () => new Response("server error", { status: 500 })); + expect(await fetchLinkedIssueFacts(env, "JSONbored/gittensory", 42, "tok")).toEqual({ status: "fetch_error" }); + }); + }); + describe("isRateLimitedGitHubFailure", () => { it("does not treat a bare permission 403 (remaining > 0, no Retry-After, no secondary body) as a rate limit", () => { expect( diff --git a/test/unit/linked-issue-hard-rules.test.ts b/test/unit/linked-issue-hard-rules.test.ts index f5fd62f9d4..7d74a12fba 100644 --- a/test/unit/linked-issue-hard-rules.test.ts +++ b/test/unit/linked-issue-hard-rules.test.ts @@ -288,9 +288,25 @@ describe("resolveLinkedIssueHardRule (#1144 — overflow + orchestration)", () = expect(await resolveLinkedIssueHardRule(args({ config: config({ ownerAssignedClose: "block" }), body: null, linkedIssues: [] }))).toBeUndefined(); }); - it("is fail-open: undefined when every fetch fails (404), with no CI token → public-token fallback", async () => { + it("treats a confirmed-nonexistent linked issue as a violation, not a silent pass (#2136)", async () => { + // Every reference 404s (CONFIRMED not-found, not a transient error) — a contributor citing a fabricated + // issue number must not silently satisfy the hard rule the same way a genuine fetch outage fails open. vi.stubGlobal("fetch", async () => new Response("missing", { status: 404 })); - expect(await resolveLinkedIssueHardRule(args({ config: config({ ownerAssignedClose: "block" }), ciToken: undefined, linkedIssues: [1, 2] }))).toBeUndefined(); + const r = await resolveLinkedIssueHardRule(args({ config: config({ ownerAssignedClose: "block" }), ciToken: undefined, linkedIssues: [1, 2] })); + expect(r?.violated).toBe(true); + expect(r?.reason).toMatch(/could not be found/i); + }); + + it("still fails open (undefined) when a linked-issue fetch fails transiently (5xx), not confirmed-nonexistent", async () => { + vi.stubGlobal("fetch", async () => new Response("server error", { status: 500 })); + expect(await resolveLinkedIssueHardRule(args({ config: config({ ownerAssignedClose: "block" }), ciToken: "tok", linkedIssues: [1, 2] }))).toBeUndefined(); + }); + + it("fails open when the linked issues are a MIX of confirmed-not-found and a transient fetch error", async () => { + // Cannot rule out a real, rule-violating issue behind the transient failure — must not treat this the same + // as an all-confirmed-not-found set. + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => (input.toString().endsWith("/issues/1") ? new Response("missing", { status: 404 }) : new Response("server error", { status: 500 }))); + expect(await resolveLinkedIssueHardRule(args({ config: config({ ownerAssignedClose: "block" }), ciToken: "tok", linkedIssues: [1, 2] }))).toBeUndefined(); }); it("fetches with the CI token and runs the deterministic evaluator over the facts", async () => { @@ -305,7 +321,7 @@ describe("resolveLinkedIssueHardRule (#1144 — overflow + orchestration)", () = }); it("derives the installation admission key from the ci token + installation id so installation reads attribute to the installation bucket, not 'unknown' (#1951 blocker)", async () => { - const spy = vi.spyOn(backfillModule, "fetchLinkedIssueFacts").mockResolvedValue(undefined); + const spy = vi.spyOn(backfillModule, "fetchLinkedIssueFacts").mockResolvedValue({ status: "fetch_error" }); await resolveLinkedIssueHardRule( args({ config: config({ ownerAssignedClose: "block" }), ciToken: "installation-token", installationId: 143010787, linkedIssues: [7] }), ); From 24c4a645af7db09ba84e9697a7ac8dfcc93c98c2 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 17:00:47 -0700 Subject: [PATCH 2/2] fix(review): don't confirm a linked issue as missing on an unauthenticated 404 GitHub returns 404 both for a genuinely nonexistent issue and for a real-but-inaccessible one (private repo, no grant) -- it deliberately doesn't distinguish the two. fetchLinkedIssueFacts treated every 404 as confirmed absence regardless of token, so a missing/under-scoped installation token falling back to the public token could get a false-positive "not_found" on a real linked issue, tripping the linked-issue hard-rule close. Only classify a 404 as not_found when the token used is a genuine, non-public credential; otherwise treat it as fetch_error (fails open), consistent with every other unprovable-outcome path in this function. --- src/github/backfill.ts | 18 ++++++++++++++---- test/unit/backfill.test.ts | 15 +++++++++++++++ test/unit/linked-issue-hard-rules.test.ts | 17 ++++++++++++++--- 3 files changed, 43 insertions(+), 7 deletions(-) diff --git a/src/github/backfill.ts b/src/github/backfill.ts index adcf51b17f..26935968af 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -2867,9 +2867,11 @@ export function isOwnReviewThreadAuthor(login: string | null | undefined): boole /** The deterministic linked-issue facts the hard-rule evaluator needs (labels / assignees / open-state). */ export type LinkedIssueFactsResult = { number: number; labels: string[]; assignees: string[]; state: string; authorLogin: string | null }; -/** Tri-state outcome of fetching one linked issue's facts (#2136). `not_found` is a CONFIRMED 404 — GitHub told - * us this issue number does not exist. `fetch_error` is everything else that prevented a read (network, 5xx, - * rate-limit, malformed body) — a genuine outage, not evidence about the issue itself. Callers that treat an +/** Tri-state outcome of fetching one linked issue's facts (#2136). `not_found` is a CONFIRMED 404 seen with a + * genuine, repo-scoped token — GitHub told an authenticated caller this issue number does not exist. `fetch_error` + * is everything else that prevented a read (network, 5xx, rate-limit, malformed body, or a 404 seen with only + * the public/anonymous token, which GitHub also returns for a real-but-inaccessible private issue) — a genuine + * outage or an unproven access gap, not confirmed evidence about the issue itself. Callers that treat an * ALL-not_found result as significant (the linked-issue hard rule) must never extend that same treatment to * fetch_error, or a GitHub outage would spuriously look like a fabricated reference. */ export type LinkedIssueFactsFetch = @@ -2884,6 +2886,12 @@ export type LinkedIssueFactsFetch = * needs. Uses the same authenticated REST client + public-token 404-fallback as the other live fetches. (Note: * GitHub's issues endpoint also returns pull requests, which carry a `pull_request` field; a PR number passed * here would simply fail the rules — we only treat real issues' labels/assignees.) + * + * GitHub returns 404 for BOTH a genuinely nonexistent issue and a real-but-inaccessible one (private repo, no + * grant) — it deliberately doesn't distinguish the two, to avoid leaking a private repo's existence to a caller + * without access. So a 404 is only trustworthy as CONFIRMED absence when `token` is a genuine, repo-scoped + * credential; the public/anonymous fallback token proves nothing about access. Without that, treat the 404 as + * `fetch_error` (fails open) rather than risk closing a PR over a real linked issue our token just can't see. */ export async function fetchLinkedIssueFacts( env: Env, @@ -2902,7 +2910,9 @@ export async function fetchLinkedIssueFacts( user?: { login?: string | null } | null; }>(env, repoFullName, `/issues/${issueNumber}`, token, githubRateLimitOptions(admissionKey)); } catch (error) { - return error instanceof GitHubApiError && error.statusCode === 404 ? { status: "not_found" } : { status: "fetch_error" }; + if (!(error instanceof GitHubApiError) || error.statusCode !== 404) return { status: "fetch_error" }; + const hasProvenAccess = Boolean(token) && token !== env.GITHUB_PUBLIC_TOKEN; + return { status: hasProvenAccess ? "not_found" : "fetch_error" }; } const data = result.data; const labels = (data.labels ?? []).flatMap((label) => { diff --git a/test/unit/backfill.test.ts b/test/unit/backfill.test.ts index 8ead9843cc..6da9edd332 100644 --- a/test/unit/backfill.test.ts +++ b/test/unit/backfill.test.ts @@ -4866,6 +4866,21 @@ describe("GitHub backfill", () => { expect(await fetchLinkedIssueFacts(env, "JSONbored/gittensory", 999999, "tok")).toEqual({ status: "not_found" }); }); + it("REGRESSION: treats a 404 seen with the public/anonymous token as fetch_error, not not_found — GitHub also returns 404 for a real but inaccessible private issue", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-tok" }); + vi.stubGlobal("fetch", async () => new Response("missing", { status: 404 })); + // The public token proves nothing about repo access, so a 404 here could just as easily mean "this issue + // is real but private and this token can't see it" -- treating it as CONFIRMED absence risks closing a PR + // over a genuinely-linked issue. + expect(await fetchLinkedIssueFacts(env, "JSONbored/gittensory", 42, env.GITHUB_PUBLIC_TOKEN)).toEqual({ status: "fetch_error" }); + }); + + it("REGRESSION: treats a 404 seen with no token at all as fetch_error, not not_found", async () => { + const env = createTestEnv({}); + vi.stubGlobal("fetch", async () => new Response("missing", { status: 404 })); + expect(await fetchLinkedIssueFacts(env, "JSONbored/gittensory", 42, undefined)).toEqual({ status: "fetch_error" }); + }); + it("returns fetch_error on a transient failure (5xx), never conflating it with not_found", async () => { const env = createTestEnv({}); vi.stubGlobal("fetch", async () => new Response("server error", { status: 500 })); diff --git a/test/unit/linked-issue-hard-rules.test.ts b/test/unit/linked-issue-hard-rules.test.ts index 7d74a12fba..707c140e92 100644 --- a/test/unit/linked-issue-hard-rules.test.ts +++ b/test/unit/linked-issue-hard-rules.test.ts @@ -289,14 +289,25 @@ describe("resolveLinkedIssueHardRule (#1144 — overflow + orchestration)", () = }); it("treats a confirmed-nonexistent linked issue as a violation, not a silent pass (#2136)", async () => { - // Every reference 404s (CONFIRMED not-found, not a transient error) — a contributor citing a fabricated - // issue number must not silently satisfy the hard rule the same way a genuine fetch outage fails open. + // Every reference 404s with a GENUINE installation token (proven repo access) — CONFIRMED not-found, not a + // transient error — a contributor citing a fabricated issue number must not silently satisfy the hard rule + // the same way a genuine fetch outage fails open. vi.stubGlobal("fetch", async () => new Response("missing", { status: 404 })); - const r = await resolveLinkedIssueHardRule(args({ config: config({ ownerAssignedClose: "block" }), ciToken: undefined, linkedIssues: [1, 2] })); + const r = await resolveLinkedIssueHardRule(args({ config: config({ ownerAssignedClose: "block" }), ciToken: "installation-token", linkedIssues: [1, 2] })); expect(r?.violated).toBe(true); expect(r?.reason).toMatch(/could not be found/i); }); + it("REGRESSION: does NOT violate when every reference 404s but ciToken is unavailable (falls back to the public token) — a 404 without proven repo access is not confirmed absence", async () => { + // GitHub also returns 404 for a real-but-inaccessible private issue, not just a genuinely nonexistent one. + // Without a genuine ciToken, this call falls back to env.GITHUB_PUBLIC_TOKEN, which proves nothing about + // repo access — closing the PR here would risk punishing a contributor for a real linked issue our token + // just can't see. + vi.stubGlobal("fetch", async () => new Response("missing", { status: 404 })); + const r = await resolveLinkedIssueHardRule(args({ config: config({ ownerAssignedClose: "block" }), ciToken: undefined, linkedIssues: [1, 2] })); + expect(r).toBeUndefined(); + }); + it("still fails open (undefined) when a linked-issue fetch fails transiently (5xx), not confirmed-nonexistent", async () => { vi.stubGlobal("fetch", async () => new Response("server error", { status: 500 })); expect(await resolveLinkedIssueHardRule(args({ config: config({ ownerAssignedClose: "block" }), ciToken: "tok", linkedIssues: [1, 2] }))).toBeUndefined();