Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 47 additions & 19 deletions src/github/backfill.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2867,40 +2867,68 @@ 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 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 =
| { 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.)
*
* 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,
repoFullName: string,
issueNumber: number,
token: string | undefined,
admissionKey?: GitHubRateLimitAdmissionKey,
): Promise<LinkedIssueFactsResult | undefined> {
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<LinkedIssueFactsFetch> {
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) {
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) => {
if (typeof label === "string") return label.length > 0 ? [label] : [];
return label?.name ? [label.name] : [];
});
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,
},
};
}

Expand Down
3 changes: 1 addition & 2 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)),
),
);
}
Expand Down
18 changes: 16 additions & 2 deletions src/review/linked-issue-hard-rules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
}
42 changes: 42 additions & 0 deletions test/unit/backfill.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
enqueueRepositoryOpenDataBackfill,
enrichInstallationHealth,
fetchAndStorePullRequestFilesForReview,
fetchLinkedIssueFacts,
fetchLiveCiAggregate,
fetchLiveReviewThreadBlockers,
fetchRequiredStatusContexts,
Expand Down Expand Up @@ -4846,6 +4847,47 @@ 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("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 }));
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(
Expand Down
33 changes: 30 additions & 3 deletions test/unit/linked-issue-hard-rules.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -288,9 +288,36 @@ 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 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 }));
expect(await resolveLinkedIssueHardRule(args({ config: config({ ownerAssignedClose: "block" }), ciToken: undefined, linkedIssues: [1, 2] }))).toBeUndefined();
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();
});

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 () => {
Expand All @@ -305,7 +332,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] }),
);
Expand Down
Loading