diff --git a/src/review/linked-issue-hard-rules.ts b/src/review/linked-issue-hard-rules.ts index 7e4f294368..06d8f7b46f 100644 --- a/src/review/linked-issue-hard-rules.ts +++ b/src/review/linked-issue-hard-rules.ts @@ -193,6 +193,10 @@ export async function resolveLinkedIssueHardRule(args: { repoOwner: string; config: LinkedIssueHardRulesConfig; body: string | null | undefined; + // Retained for call-site compatibility, but NO LONGER the fetch source of truth (#8354): the set of issues + // whose facts are fetched is re-derived from a fresh parse of `body` (via extractLinkedIssueNumbersWithOverflow) + // so it can never disagree with the overflow check above. A caller may still pass its last-synced list; it is + // intentionally not read here. linkedIssues: number[]; ciToken: string | undefined; prAuthorLogin?: string | null | undefined; @@ -202,16 +206,22 @@ export async function resolveLinkedIssueHardRule(args: { installationId?: number | null | undefined; }): Promise { if (!anyLinkedIssueHardRuleOn(args.config)) return undefined; - if (extractLinkedIssueNumbersWithOverflow(args.body ?? "", args.repoFullName).overflow) { + // Parse the PR's CURRENT body ONCE and drive BOTH the overflow check and the per-issue fact-fetch loop from + // that single result, so the two can never disagree about which issues are "currently linked" (#8354). The + // caller still passes `args.linkedIssues` (a possibly-stale field synced by an earlier body parse), but it is + // deliberately no longer the source of truth here: a body edit that adds a new closing reference between the + // last `pr.linkedIssues` sync and this evaluation must be seen by the fact-fetch loop, not silently skipped. + const linkedIssuesFromBody = extractLinkedIssueNumbersWithOverflow(args.body ?? "", args.repoFullName); + if (linkedIssuesFromBody.overflow) { return { violated: true, reason: "PR body links more issues than LoopOver can safely verify automatically; please reduce linked closing references or request maintainer review.", }; } - if (args.linkedIssues.length === 0) return undefined; + if (linkedIssuesFromBody.numbers.length === 0) return undefined; const token = args.ciToken ?? args.env.GITHUB_PUBLIC_TOKEN; const admissionKey = githubRateLimitAdmissionKeyForToken(args.env, token, args.installationId); - const fetchResults = await Promise.all(args.linkedIssues.map((issueNumber) => fetchLinkedIssueFacts(args.env, args.repoFullName, issueNumber, token, admissionKey))); + const fetchResults = await Promise.all(linkedIssuesFromBody.numbers.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 diff --git a/test/unit/linked-issue-hard-rules.test.ts b/test/unit/linked-issue-hard-rules.test.ts index 3dff483a28..153756838f 100644 --- a/test/unit/linked-issue-hard-rules.test.ts +++ b/test/unit/linked-issue-hard-rules.test.ts @@ -502,7 +502,7 @@ describe("resolveLinkedIssueHardRule (#1144 — overflow + orchestration)", () = // 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: "installation-token", linkedIssues: [1, 2] })); + const r = await resolveLinkedIssueHardRule(args({ config: config({ ownerAssignedClose: "block" }), ciToken: "installation-token", body: "closes #1 closes #2", linkedIssues: [1, 2] })); expect(r?.violated).toBe(true); expect(r?.reason).toMatch(/could not be found/i); }); @@ -513,20 +513,20 @@ describe("resolveLinkedIssueHardRule (#1144 — overflow + orchestration)", () = // 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] })); + const r = await resolveLinkedIssueHardRule(args({ config: config({ ownerAssignedClose: "block" }), ciToken: undefined, body: "closes #1 closes #2", 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(); + expect(await resolveLinkedIssueHardRule(args({ config: config({ ownerAssignedClose: "block" }), ciToken: "tok", body: "closes #1 closes #2", 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(); + expect(await resolveLinkedIssueHardRule(args({ config: config({ ownerAssignedClose: "block" }), ciToken: "tok", body: "closes #1 closes #2", linkedIssues: [1, 2] }))).toBeUndefined(); }); it("fetches with the CI token and runs the deterministic evaluator over the facts", async () => { @@ -563,7 +563,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({ status: "fetch_error" }); await resolveLinkedIssueHardRule( - args({ config: config({ ownerAssignedClose: "block" }), ciToken: "installation-token", installationId: 143010787, linkedIssues: [7] }), + args({ config: config({ ownerAssignedClose: "block" }), ciToken: "installation-token", installationId: 143010787, body: "closes #7", linkedIssues: [7] }), ); // The key is DERIVED from the token it will actually read with (so it can never drift): a non-public token + // finite installation id ⇒ the installation bucket, NOT undefined (which the metrics record as "unknown"). @@ -571,6 +571,25 @@ describe("resolveLinkedIssueHardRule (#1144 — overflow + orchestration)", () = spy.mockRestore(); }); + it("REGRESSION (#8354): evaluates an issue linked in the FRESH body even when it is absent from a stale linkedIssues array — the overflow check and fact-fetch can no longer disagree", async () => { + // The body links #9 (e.g. a just-edited description), but the caller's last-synced `linkedIssues` still says + // [1]. Before the fix, the fact-fetch loop trusted the stale [1] and never evaluated #9 (it would have fetched + // #1, 404'd, and reported a "could not be found" violation for the WRONG issue); now the fetch list is derived + // from the same fresh body parse the overflow check uses, so #9's real ineligibility IS detected. + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => + input.toString().includes("/issues/9") + ? Response.json({ number: 9, state: "open", labels: [], assignees: [{ login: "claimed-dev" }] }) + : new Response("missing", { status: 404 }), + ); + const result = await resolveLinkedIssueHardRule( + args({ config: config({ assignedIssueClose: "block" }), ciToken: "tok", body: "closes #9", linkedIssues: [1], prAuthorLogin: "drive-by" }), + ); + expect(result).toEqual({ + violated: true, + reason: "Linked issue #9 is already assigned to @claimed-dev — only the assignee or a maintainer can submit that work.", + }); + }); + it("REGRESSION: an ineligible (owner-assigned) linked issue still violates the hard rule regardless of linkedIssueGateMode -- the two are fully independent (#selfhost-linked-issue-gate-drift)", () => { // evaluateLinkedIssueHardRules's own input type (`{ issues, config, repoOwner }`) has no linkedIssueGateMode // field at all -- it structurally cannot read it. This test pins the END-TO-END behavior: fixing