diff --git a/coverage-8354/lcov.info b/coverage-8354/lcov.info new file mode 100644 index 000000000..817b309d4 --- /dev/null +++ b/coverage-8354/lcov.info @@ -0,0 +1,183 @@ +TN: +SF:src\review\linked-issue-hard-rules.ts +FN:34,loadLinkedIssueHardRules +FN:35,(anonymous_1) +FN:57,anyLinkedIssueHardRuleOn +FN:66,findMatchingLabel +FN:67,(anonymous_4) +FN:68,(anonymous_5) +FN:71,labelMatches +FN:75,issueIsAssignedToAuthor +FN:77,(anonymous_8) +FN:86,evaluateLinkedIssueHardRules +FN:105,(anonymous_10) +FN:171,mergeLinkedIssueHardRuleWithPersistedViolation +FN:195,resolveLinkedIssueHardRule +FN:220,(anonymous_13) +FN:221,(anonymous_14) +FN:227,(anonymous_15) +FN:258,hasVerifiableOpenLinkedIssueReference +FN:260,(anonymous_17) +FN:261,(anonymous_18) +FN:271,resolveLinkedIssueHasOpenReference +FN:284,(anonymous_20) +FN:287,(anonymous_21) +FNF:22 +FNH:22 +FNDA:4,loadLinkedIssueHardRules +FNDA:3,(anonymous_1) +FNDA:44,anyLinkedIssueHardRuleOn +FNDA:21,findMatchingLabel +FNDA:31,(anonymous_4) +FNDA:16,(anonymous_5) +FNDA:5,labelMatches +FNDA:23,issueIsAssignedToAuthor +FNDA:7,(anonymous_8) +FNDA:27,evaluateLinkedIssueHardRules +FNDA:6,(anonymous_10) +FNDA:9,mergeLinkedIssueHardRuleWithPersistedViolation +FNDA:12,resolveLinkedIssueHardRule +FNDA:13,(anonymous_13) +FNDA:13,(anonymous_14) +FNDA:7,(anonymous_15) +FNDA:17,hasVerifiableOpenLinkedIssueReference +FNDA:21,(anonymous_17) +FNDA:12,(anonymous_18) +FNDA:9,resolveLinkedIssueHasOpenReference +FNDA:1,(anonymous_20) +FNDA:57,(anonymous_21) +DA:28,1 +DA:35,4 +DA:50,1 +DA:58,44 +DA:67,31 +DA:68,21 +DA:72,5 +DA:76,23 +DA:77,23 +DA:92,27 +DA:93,27 +DA:94,27 +DA:96,22 +DA:97,24 +DA:98,23 +DA:101,23 +DA:105,6 +DA:107,5 +DA:114,18 +DA:115,2 +DA:122,16 +DA:123,16 +DA:124,4 +DA:131,12 +DA:132,1 +DA:139,10 +DA:176,9 +DA:177,7 +DA:178,5 +DA:179,3 +DA:208,12 +DA:210,11 +DA:211,12 +DA:212,1 +DA:217,10 +DA:218,9 +DA:219,12 +DA:220,13 +DA:221,13 +DA:222,9 +DA:227,7 +DA:228,5 +DA:229,1 +DA:234,4 +DA:236,4 +DA:259,17 +DA:260,21 +DA:261,12 +DA:277,9 +DA:283,8 +DA:284,7 +DA:285,9 +DA:286,9 +DA:287,57 +DA:288,7 +LF:55 +LH:55 +BRDA:35,0,0,4 +BRDA:35,0,1,3 +BRDA:59,1,0,44 +BRDA:59,1,1,24 +BRDA:59,1,2,17 +BRDA:59,1,3,11 +BRDA:68,2,0,21 +BRDA:68,2,1,12 +BRDA:77,3,0,23 +BRDA:77,3,1,6 +BRDA:94,4,0,5 +BRDA:94,4,1,22 +BRDA:97,5,0,1 +BRDA:97,5,1,23 +BRDA:101,6,0,5 +BRDA:101,6,1,18 +BRDA:102,7,0,23 +BRDA:102,7,1,10 +BRDA:102,7,2,10 +BRDA:102,7,3,9 +BRDA:114,8,0,2 +BRDA:114,8,1,16 +BRDA:114,9,0,18 +BRDA:114,9,1,5 +BRDA:114,9,2,4 +BRDA:123,10,0,4 +BRDA:123,10,1,12 +BRDA:123,11,0,16 +BRDA:123,11,1,6 +BRDA:123,11,2,5 +BRDA:131,12,0,1 +BRDA:131,12,1,11 +BRDA:131,13,0,12 +BRDA:131,13,1,6 +BRDA:131,13,2,5 +BRDA:134,14,0,1 +BRDA:134,14,1,0 +BRDA:176,15,0,2 +BRDA:176,15,1,7 +BRDA:177,16,0,2 +BRDA:177,16,1,5 +BRDA:178,17,0,2 +BRDA:178,17,1,3 +BRDA:179,18,0,3 +BRDA:179,18,1,1 +BRDA:208,19,0,1 +BRDA:208,19,1,11 +BRDA:210,20,0,11 +BRDA:210,20,1,1 +BRDA:211,21,0,1 +BRDA:211,21,1,11 +BRDA:217,22,0,1 +BRDA:217,22,1,9 +BRDA:218,23,0,9 +BRDA:218,23,1,1 +BRDA:221,24,0,4 +BRDA:221,24,1,9 +BRDA:222,25,0,5 +BRDA:222,25,1,4 +BRDA:228,26,0,1 +BRDA:228,26,1,4 +BRDA:259,27,0,1 +BRDA:259,27,1,16 +BRDA:260,28,0,7 +BRDA:260,28,1,9 +BRDA:260,29,0,21 +BRDA:260,29,1,14 +BRDA:277,30,0,1 +BRDA:277,30,1,8 +BRDA:283,31,0,1 +BRDA:283,31,1,7 +BRDA:284,32,0,1 +BRDA:284,32,1,6 +BRDA:285,33,0,9 +BRDA:285,33,1,7 +BRF:75 +BRH:74 +end_of_record diff --git a/src/queue/processors.ts b/src/queue/processors.ts index ef553fdf2..a808f6230 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2986,13 +2986,14 @@ async function runAgentMaintenancePlanAndExecute( env, repoFullName, ); + // Issue numbers are derived inside resolveLinkedIssueHardRule from a fresh body parse (#8354) — do not pass + // pr.linkedIssues here; that synced field can lag a body edit and disagree with the overflow check. const liveLinkedIssueHardRule = await resolveLinkedIssueHardRule({ env, repoFullName, repoOwner, config: linkedIssueRulesConfig, body: pr.body, - linkedIssues: pr.linkedIssues, ciToken, prAuthorLogin: pr.authorLogin, installationId, diff --git a/src/review/linked-issue-hard-rules.ts b/src/review/linked-issue-hard-rules.ts index 7e4f29436..dfe2eedcd 100644 --- a/src/review/linked-issue-hard-rules.ts +++ b/src/review/linked-issue-hard-rules.ts @@ -186,6 +186,11 @@ export function mergeLinkedIssueHardRuleWithPersistedViolation( * - no rule in "block" mode → undefined (skip entirely, no fetch). * - the PR body links MORE closing references than the cap (overflow) → a violation: too many to verify safely. * - otherwise fetch each linked issue's facts (fail-open per issue) and run the deterministic evaluator. + * + * Both the overflow check and the per-issue fetch list come from ONE + * {@link extractLinkedIssueNumbersWithOverflow} call on `args.body` (#8354). Callers must not pass a separately + * synced `pr.linkedIssues` array — that field can lag a body edit and previously let a newly-added closing + * reference slip past hard-rule evaluation until a later sync. */ export async function resolveLinkedIssueHardRule(args: { env: Env; @@ -193,7 +198,6 @@ export async function resolveLinkedIssueHardRule(args: { repoOwner: string; config: LinkedIssueHardRulesConfig; body: string | null | undefined; - linkedIssues: number[]; ciToken: string | undefined; prAuthorLogin?: string | null | undefined; // The installation id for `ciToken` (undefined for public-token reads). The admission key is DERIVED from the @@ -202,16 +206,18 @@ export async function resolveLinkedIssueHardRule(args: { installationId?: number | null | undefined; }): Promise { if (!anyLinkedIssueHardRuleOn(args.config)) return undefined; - if (extractLinkedIssueNumbersWithOverflow(args.body ?? "", args.repoFullName).overflow) { + // Single fresh parse: overflow boolean AND the issue numbers to fetch must agree (#8354). + const linked = extractLinkedIssueNumbersWithOverflow(args.body ?? "", args.repoFullName); + if (linked.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 (linked.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(linked.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/src/services/agent-approval-queue.ts b/src/services/agent-approval-queue.ts index 39c26fd94..abc338b97 100644 --- a/src/services/agent-approval-queue.ts +++ b/src/services/agent-approval-queue.ts @@ -157,7 +157,6 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de repoOwner, config: linkedIssueRulesConfig, body: pr.body, - linkedIssues: pr.linkedIssues, ciToken, prAuthorLogin: pr.authorLogin, installationId: pending.installationId, @@ -377,7 +376,6 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de repoOwner, config: linkedIssueRulesConfig, body: pr.body, - linkedIssues: pr.linkedIssues, ciToken, prAuthorLogin: pr.authorLogin, installationId: pending.installationId, diff --git a/test/unit/linked-issue-hard-rules.test.ts b/test/unit/linked-issue-hard-rules.test.ts index 3dff483a2..c1e5024cd 100644 --- a/test/unit/linked-issue-hard-rules.test.ts +++ b/test/unit/linked-issue-hard-rules.test.ts @@ -464,14 +464,14 @@ describe("evaluateLinkedIssueHardRules with explicit config", () => { describe("resolveLinkedIssueHardRule (#1144 — overflow + orchestration)", () => { afterEach(() => vi.unstubAllGlobals()); // Defaults: body=null and ciToken=undefined so the `?? ""` and `?? env.GITHUB_PUBLIC_TOKEN` fallbacks are - // exercised; tests that need the other arm pass a string body / a CI token explicitly. + // exercised; tests that need the other arm pass a string body / a CI token explicitly. Issue numbers are + // always derived from a fresh body parse (#8354) — there is no separately-supplied linkedIssues list. const args = (over: Record = {}) => ({ env: createTestEnv({}), repoFullName: "owner/repo", repoOwner: "owner", config: config(), body: null as string | null | undefined, - linkedIssues: [] as number[], ciToken: undefined as string | undefined, ...over, }); @@ -479,7 +479,7 @@ describe("resolveLinkedIssueHardRule (#1144 — overflow + orchestration)", () = it("returns undefined and fetches nothing when no rule is in block mode", async () => { const fetchSpy = vi.fn(); vi.stubGlobal("fetch", fetchSpy); - expect(await resolveLinkedIssueHardRule(args({ config: config(), body: "closes #1", linkedIssues: [1] }))).toBeUndefined(); + expect(await resolveLinkedIssueHardRule(args({ config: config(), body: "closes #1" }))).toBeUndefined(); expect(fetchSpy).not.toHaveBeenCalled(); }); @@ -487,14 +487,22 @@ describe("resolveLinkedIssueHardRule (#1144 — overflow + orchestration)", () = const fetchSpy = vi.fn(); vi.stubGlobal("fetch", fetchSpy); const body = Array.from({ length: 60 }, (_, i) => `closes #${i + 1}`).join(" "); - const r = await resolveLinkedIssueHardRule(args({ config: config({ ownerAssignedClose: "block" }), body, linkedIssues: [1] })); + const r = await resolveLinkedIssueHardRule(args({ config: config({ ownerAssignedClose: "block" }), body })); expect(r?.violated).toBe(true); expect(r?.reason).toMatch(/more issues than LoopOver can safely verify/i); expect(fetchSpy).not.toHaveBeenCalled(); }); it("returns undefined when a rule is on but the PR links no issues (null body → no overflow)", async () => { - expect(await resolveLinkedIssueHardRule(args({ config: config({ ownerAssignedClose: "block" }), body: null, linkedIssues: [] }))).toBeUndefined(); + expect(await resolveLinkedIssueHardRule(args({ config: config({ ownerAssignedClose: "block" }), body: null }))).toBeUndefined(); + }); + + // Both nullish arms of `args.body ?? ""` (#8354) must be hit for branch-counted patch coverage: null (above) + // and undefined (here). An empty string is the non-nullish "present but blank" arm of the same parse. + it("returns undefined for undefined body and for an empty-string body (fresh-parse emptiness, not a stale list)", async () => { + const cfg = config({ ownerAssignedClose: "block" }); + expect(await resolveLinkedIssueHardRule(args({ config: cfg, body: undefined }))).toBeUndefined(); + expect(await resolveLinkedIssueHardRule(args({ config: cfg, body: "" }))).toBeUndefined(); }); it("treats a confirmed-nonexistent linked issue as a violation, not a silent pass (#2136)", async () => { @@ -502,7 +510,9 @@ 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" }), + ); expect(r?.violated).toBe(true); expect(r?.reason).toMatch(/could not be found/i); }); @@ -513,20 +523,26 @@ 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" }), + ); 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" })), + ).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" })), + ).toBeUndefined(); }); it("fetches with the CI token and runs the deterministic evaluator over the facts", async () => { @@ -535,7 +551,7 @@ describe("resolveLinkedIssueHardRule (#1144 — overflow + orchestration)", () = ? Response.json({ number: 1, state: "open", labels: [], assignees: ["owner"] }) : new Response("missing", { status: 404 }), ); - const r = await resolveLinkedIssueHardRule(args({ config: config({ ownerAssignedClose: "block" }), ciToken: "tok", body: "closes #1", linkedIssues: [1] })); + const r = await resolveLinkedIssueHardRule(args({ config: config({ ownerAssignedClose: "block" }), ciToken: "tok", body: "closes #1" })); expect(r).toBeDefined(); expect(typeof r?.violated).toBe("boolean"); }); @@ -547,7 +563,7 @@ describe("resolveLinkedIssueHardRule (#1144 — overflow + orchestration)", () = : new Response("missing", { status: 404 }), ); const blocked = await resolveLinkedIssueHardRule( - args({ config: config({ assignedIssueClose: "block" }), ciToken: "tok", body: "closes #1", linkedIssues: [1], prAuthorLogin: "drive-by" }), + args({ config: config({ assignedIssueClose: "block" }), ciToken: "tok", body: "closes #1", prAuthorLogin: "drive-by" }), ); expect(blocked).toEqual({ violated: true, @@ -555,7 +571,7 @@ describe("resolveLinkedIssueHardRule (#1144 — overflow + orchestration)", () = }); const assignee = await resolveLinkedIssueHardRule( - args({ config: config({ assignedIssueClose: "block" }), ciToken: "tok", body: "closes #1", linkedIssues: [1], prAuthorLogin: "claimed-dev" }), + args({ config: config({ assignedIssueClose: "block" }), ciToken: "tok", body: "closes #1", prAuthorLogin: "claimed-dev" }), ); expect(assignee).toEqual({ violated: false, reason: null }); }); @@ -563,7 +579,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" }), ); // 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 +587,38 @@ describe("resolveLinkedIssueHardRule (#1144 — overflow + orchestration)", () = spy.mockRestore(); }); + it("REGRESSION (#8354): evaluates every issue freshly parsed from the body — a newly-added closing reference is never skipped", async () => { + // Before the fix, overflow used extractLinkedIssueNumbersWithOverflow(body) while the fact-fetch loop + // trusted a separately-supplied linkedIssues array (often a stale pr.linkedIssues sync). A body edit that + // added "Fixes #99" between sync and evaluation could pass overflow yet never fetch #99. The linkedIssues + // parameter is now removed; the fetch list is the SAME parse's `.numbers`. + const spy = vi.spyOn(backfillModule, "fetchLinkedIssueFacts").mockResolvedValue({ + status: "found", + facts: { number: 99, state: "open", labels: [], assignees: ["owner"], authorLogin: null, closedAt: null }, + }); + const r = await resolveLinkedIssueHardRule( + args({ config: config({ ownerAssignedClose: "block" }), ciToken: "tok", body: "Fixes #99" }), + ); + // Fetch list is body-derived: #99 must be requested even though no stale linkedIssues array is (or can be) supplied. + expect(spy.mock.calls.map((call) => call[2])).toEqual([99]); + expect(r).toEqual({ + violated: true, + reason: "Linked issue #99 is assigned to the maintainer (@owner) — that work is reserved for the maintainer, so this PR cannot be auto-accepted.", + }); + spy.mockRestore(); + }); + + it("REGRESSION (#8354): a multi-issue body fans out to EVERY freshly-parsed number (overflow false, numbers non-empty)", async () => { + // Pins the non-overflow / non-empty arm of the shared parse: both #41 and #42 must be fetched from ONE body + // string, proving the fact-fetch loop reads `linked.numbers` (not a separately-supplied list). + const spy = vi.spyOn(backfillModule, "fetchLinkedIssueFacts").mockResolvedValue({ status: "fetch_error" }); + await resolveLinkedIssueHardRule( + args({ config: config({ ownerAssignedClose: "block" }), ciToken: "tok", body: "Closes #41 and fixes #42" }), + ); + expect(spy.mock.calls.map((call) => call[2]).sort((a, b) => Number(a) - Number(b))).toEqual([41, 42]); + spy.mockRestore(); + }); + 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