From 773ae91ee65dc60689c50effa38969e926ed0652 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 31 Jul 2026 04:47:12 -0700 Subject: [PATCH 1/4] feat(review): detect a pull request superseded by a merged rival MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A contributor whose linked issue is closed by a rival PR that merged first is currently told "No linked issue detected — link it explicitly in the PR body". They did link one, correctly, and the advice cannot work: re-linking a closed issue changes nothing. metagraphed#8886 linked issue #8829 at 09:22:36; rival #8881 merged at 09:30:24 and the issue closed one second later, and from then on every evaluation produced the same unactionable hold. confirmedNoOpenLinkedIssue collapses gaming (citing an already-dead issue to clear linkedIssueGateMode: block) with supersession (linking a genuinely open issue that a rival then closed). Only the first is a linking failure. The two separate on facts already in hand, with no new GitHub call: the issue's closed_at postdates the PR's created_at, and a MERGED sibling citing the same issue landed in the window ending at that close. GitHub's issue payload already carries closed_at (#4528) and the linked-issue pass already fetches every linked issue -- it discarded everything but a boolean. The rival comes from our own pull_requests rows; the duplicate machinery cannot see it, because that keys on OPEN siblings and the rival stopped being one when it merged. Every uncertain case resolves to "not superseded": a supersession verdict closes a contributor's PR, so a missing timestamp, an unparseable date, a still-open issue, or an absent rival all leave the existing disposition alone. Ordering is pinned on both axes (ascending issue number, then ascending PR number within one issue) because a result that closes a PR must not depend on database row order. Refs #10168 --- src/review/linked-issue-superseded.ts | 132 ++++++++++++++++++ test/unit/linked-issue-superseded.test.ts | 161 ++++++++++++++++++++++ 2 files changed, 293 insertions(+) create mode 100644 src/review/linked-issue-superseded.ts create mode 100644 test/unit/linked-issue-superseded.test.ts diff --git a/src/review/linked-issue-superseded.ts b/src/review/linked-issue-superseded.ts new file mode 100644 index 000000000..8ef15c06e --- /dev/null +++ b/src/review/linked-issue-superseded.ts @@ -0,0 +1,132 @@ +// Telling a superseded contributor they forgot to link an issue (#10168). +// +// metagraphed#8886 linked issue #8829 correctly at 09:22:36. At 09:30:24 a rival PR (#8881) linking the SAME +// issue merged, and one second later the issue closed. From then on #8886's every evaluation produced +// `hold | missing_linked_issue` with the message "The PR cites an issue number, but it could not be verified +// as a currently open issue" and the advice "link it explicitly in the PR body" -- advice that cannot work, +// because re-linking a closed issue changes nothing. The contributor was told to fix a mistake they did not +// make. (It is also the shape that never clears, which is what #10184's backoff exists to throttle.) +// +// `confirmedNoOpenLinkedIssue` (#unlinked-issue-guardrail-followup) collapses two different situations: +// +// GAMING -- the PR cited an already-dead issue to clear `linkedIssueGateMode: block`. Real, and the +// countermeasure this flag exists for. Unchanged by this module. +// SUPERSEDED -- the PR linked a genuinely OPEN issue, and a different PR merged first and closed it. +// Ordinary contributor collision, and not a linking failure at all. +// +// ── HOW THE TWO ARE SEPARATED ───────────────────────────────────────────────────────────────────────────── +// Two facts already in hand, no new GitHub call: +// +// 1. the issue was OPEN when this PR was created -- `closedAt` postdates `createdAt`. GitHub's issue payload +// already carries `closed_at` (LinkedIssueFactsResult, added by #4528), and the linked-issue verification +// pass already fetches every linked issue; it just discarded everything but a boolean. +// 2. a MERGED sibling PR in this repo cites the same issue, and merged into the window that ends at the +// close. Our own `pull_requests` rows answer this -- the duplicate/overlap machinery cannot, because it +// keys on OPEN siblings and the rival stopped being one the moment it merged (#10168). +// +// Deliberately NOT GitHub's issue-timeline API: it would be a second network call per linked issue on the +// hottest path, to re-derive a fact our own ledger already recorded when we merged the rival ourselves. +// +// ── WHY EVERY UNCERTAIN CASE RESOLVES TO "NOT SUPERSEDED" ───────────────────────────────────────────────── +// A supersession verdict CLOSES a contributor's pull request. That is destructive and one-shot, so a missing +// timestamp, an unparseable date, an absent rival, or a PR whose own `createdAt` was never synced must all +// resolve to null -- the PR keeps its existing disposition rather than being closed on incomplete evidence. +// This is the same discipline as #10184's backoff, pointed the other way: there, uncertainty must not +// SUPPRESS a review; here, uncertainty must not TAKE an irreversible action. + +/** One linked issue's closure state, as read from the live issue payload. */ +export type LinkedIssueClosure = { + issueNumber: number; + /** Lowercased GitHub issue state. Only a non-`open` issue can have superseded anything. */ + state: string; + /** GitHub's `closed_at`, or null while open. */ + closedAt: string | null; +}; + +/** A candidate rival: a pull request in the same repo that has MERGED, and what it cited. */ +export type MergedRivalPullRequest = { + number: number; + /** GitHub's `merged_at`. Null for a PR that closed without merging -- never a supersession. */ + mergedAt: string | null; + linkedIssues: number[]; +}; + +/** The evidence behind a supersession, carried onto the finding so the message can name the rival. */ +export type SupersededByRival = { + issueNumber: number; + rivalPullNumber: number; + rivalMergedAt: string; + issueClosedAt: string; +}; + +/** + * How long after a rival's merge the issue's own close may land and still count as caused by it. + * + * GitHub closes a linked issue as a side effect of the merge, normally within a second (#8886: merge 09:30:24, + * close 09:30:25). The window absorbs webhook/relay lag without stretching so far that an UNRELATED manual + * close minutes later gets misattributed to a merge that happened to precede it. + */ +export const SUPERSEDED_CLOSE_WINDOW_MS = 5 * 60_000; + +/** PURE. Parse a GitHub timestamp, keeping the original string beside the epoch ms so a caller that has + * already proved a timestamp parses never needs a second, unreachable null-check to use its text. Null when + * the value is absent or unparseable. */ +function parseInstant(value: string | null | undefined): { iso: string; ms: number } | null { + if (!value) return null; + const ms = Date.parse(value); + return Number.isFinite(ms) ? { iso: value, ms } : null; +} + +/** + * PURE. Decide whether this pull request was superseded, and by which rival. + * + * Returns null -- meaning "not superseded, leave the disposition alone" -- for every case that is not a fully + * evidenced collision. Determinism matters because the result closes a PR: candidates are resolved in + * ascending issue number, and within one issue the EARLIEST qualifying merge wins, so the same inputs always + * name the same rival regardless of row order. + */ +export function resolveSupersession(args: { + prNumber: number; + /** The superseded PR's own creation time. Absent (never synced) ⇒ null, since fact 1 cannot be established. */ + prCreatedAt: string | null | undefined; + closures: readonly LinkedIssueClosure[]; + mergedRivals: readonly MergedRivalPullRequest[]; +}): SupersededByRival | null { + const created = parseInstant(args.prCreatedAt); + if (created === null) return null; + const closures = [...args.closures].sort((a, b) => a.issueNumber - b.issueNumber); + for (const closure of closures) { + if (closure.state === "open") continue; + const closed = parseInstant(closure.closedAt); + if (closed === null) continue; + // Fact 1: the issue was still open when this PR was created. A PR that cited an ALREADY-closed issue is + // the gaming case the linked-issue guardrail exists for, and must keep reading as `missing_linked_issue`. + if (closed.ms <= created.ms) continue; + let earliest: { rival: MergedRivalPullRequest; mergedIso: string; mergedMs: number } | null = null; + // Ascending PR number, so that two rivals merged at the SAME instant (a batch merge lands both `merged_at` + // values in the same second often enough to matter) resolve to the lower number rather than to whichever + // row the database happened to return first. + for (const rival of [...args.mergedRivals].sort((a, b) => a.number - b.number)) { + if (rival.number === args.prNumber) continue; + if (!rival.linkedIssues.includes(closure.issueNumber)) continue; + const merged = parseInstant(rival.mergedAt); + if (merged === null) continue; + // Fact 2: the rival merged inside the window that ends at the close -- after this PR opened (a merge + // that predates it cannot have taken work this PR had not yet proposed) and not so long before the + // close that some other actor is the likelier cause. + if (merged.ms < created.ms) continue; + if (merged.ms > closed.ms + SUPERSEDED_CLOSE_WINDOW_MS) continue; + if (earliest !== null && merged.ms >= earliest.mergedMs) continue; + earliest = { rival, mergedIso: merged.iso, mergedMs: merged.ms }; + } + if (earliest !== null) { + return { + issueNumber: closure.issueNumber, + rivalPullNumber: earliest.rival.number, + rivalMergedAt: earliest.mergedIso, + issueClosedAt: closed.iso, + }; + } + } + return null; +} diff --git a/test/unit/linked-issue-superseded.test.ts b/test/unit/linked-issue-superseded.test.ts new file mode 100644 index 000000000..f1349b1f6 --- /dev/null +++ b/test/unit/linked-issue-superseded.test.ts @@ -0,0 +1,161 @@ +import { describe, expect, it } from "vitest"; +import { + SUPERSEDED_CLOSE_WINDOW_MS, + resolveSupersession, + type LinkedIssueClosure, + type MergedRivalPullRequest, +} from "../../src/review/linked-issue-superseded"; + +// The real collision this exists for (#10168), timestamps as recorded on the Orb: +// PR 8886 created 09:22:36 citing issue 8829 -- issue open at that moment +// PR 8881 (same issue) merged 09:30:24 +// issue 8829 closed 09:30:25, one second later, as a side effect of that merge +const PR_CREATED = "2026-07-31T09:22:36Z"; +const RIVAL_MERGED = "2026-07-31T09:30:24Z"; +const ISSUE_CLOSED = "2026-07-31T09:30:25Z"; + +function closure(overrides: Partial = {}): LinkedIssueClosure { + return { issueNumber: 8829, state: "closed", closedAt: ISSUE_CLOSED, ...overrides }; +} + +function rival(overrides: Partial = {}): MergedRivalPullRequest { + return { number: 8881, mergedAt: RIVAL_MERGED, linkedIssues: [8829], ...overrides }; +} + +function resolve(overrides: Partial[0]> = {}) { + return resolveSupersession({ + prNumber: 8886, + prCreatedAt: PR_CREATED, + closures: [closure()], + mergedRivals: [rival()], + ...overrides, + }); +} + +describe("resolveSupersession", () => { + it("names the merged rival for the metagraphed#8886 collision", () => { + expect(resolve()).toEqual({ + issueNumber: 8829, + rivalPullNumber: 8881, + rivalMergedAt: RIVAL_MERGED, + issueClosedAt: ISSUE_CLOSED, + }); + }); + + describe("fact 1 — the issue must have been open when the PR was created", () => { + it("declines when the issue closed BEFORE the PR opened (the gaming case the guardrail exists for)", () => { + // The contributor cited an already-dead issue: `missing_linked_issue` must keep its current meaning. + expect(resolve({ closures: [closure({ closedAt: "2026-07-31T09:00:00Z" })] })).toBeNull(); + }); + + it("declines on the exact tie — a close at the creation instant proves nothing was taken from this PR", () => { + // The rival is placed so it WOULD qualify if the tie were admitted, so this pins `<=` and not `<`. + expect( + resolve({ + closures: [closure({ closedAt: PR_CREATED })], + mergedRivals: [rival({ mergedAt: PR_CREATED })], + }), + ).toBeNull(); + }); + + it("declines while the issue is still open", () => { + expect(resolve({ closures: [closure({ state: "open", closedAt: null })] })).toBeNull(); + }); + + it("declines on a still-open issue even if a stale closedAt is present", () => { + // Without the explicit state check a reopened issue carrying an old closed_at would read as superseded, + // and this PR would be closed over an issue that is once again open. + expect(resolve({ closures: [closure({ state: "open" })] })).toBeNull(); + }); + + it("declines when a non-open issue carries no closedAt", () => { + expect(resolve({ closures: [closure({ closedAt: null })] })).toBeNull(); + }); + + it("declines when closedAt is unparseable", () => { + expect(resolve({ closures: [closure({ closedAt: "not-a-date" })] })).toBeNull(); + }); + + it("declines when the PR has no synced createdAt", () => { + expect(resolve({ prCreatedAt: null })).toBeNull(); + expect(resolve({ prCreatedAt: undefined })).toBeNull(); + expect(resolve({ prCreatedAt: "" })).toBeNull(); + }); + + it("declines when the PR's createdAt is unparseable", () => { + expect(resolve({ prCreatedAt: "whenever" })).toBeNull(); + }); + }); + + describe("fact 2 — a merged rival must be the plausible cause", () => { + it("declines when no rival cites the issue", () => { + expect(resolve({ mergedRivals: [rival({ linkedIssues: [9999] })] })).toBeNull(); + }); + + it("declines when there are no rivals at all", () => { + expect(resolve({ mergedRivals: [] })).toBeNull(); + }); + + it("declines when the rival closed without merging", () => { + expect(resolve({ mergedRivals: [rival({ mergedAt: null })] })).toBeNull(); + }); + + it("declines when the rival's mergedAt is unparseable", () => { + expect(resolve({ mergedRivals: [rival({ mergedAt: "sometime" })] })).toBeNull(); + }); + + it("never treats the PR as its own rival", () => { + expect(resolve({ mergedRivals: [rival({ number: 8886 })] })).toBeNull(); + }); + + it("declines when the rival merged before this PR was even opened", () => { + expect(resolve({ mergedRivals: [rival({ mergedAt: "2026-07-31T09:00:00Z" })] })).toBeNull(); + }); + + it("accepts a rival merged at the window's outer edge", () => { + const edge = new Date(Date.parse(ISSUE_CLOSED) + SUPERSEDED_CLOSE_WINDOW_MS).toISOString(); + expect(resolve({ mergedRivals: [rival({ mergedAt: edge })] })?.rivalPullNumber).toBe(8881); + }); + + it("declines a rival merged past the window — some other actor is the likelier cause", () => { + const past = new Date(Date.parse(ISSUE_CLOSED) + SUPERSEDED_CLOSE_WINDOW_MS + 1).toISOString(); + expect(resolve({ mergedRivals: [rival({ mergedAt: past })] })).toBeNull(); + }); + }); + + describe("determinism — the result closes a PR, so it must not depend on row order", () => { + it("picks the EARLIEST qualifying merge regardless of the order rivals arrive in", () => { + const early = rival({ number: 8881, mergedAt: "2026-07-31T09:29:00Z" }); + const late = rival({ number: 8899, mergedAt: RIVAL_MERGED }); + expect(resolve({ mergedRivals: [early, late] })?.rivalPullNumber).toBe(8881); + expect(resolve({ mergedRivals: [late, early] })?.rivalPullNumber).toBe(8881); + }); + + it("breaks a same-instant merge tie on the lower PR number, not on row order", () => { + const first = rival({ number: 8881, mergedAt: RIVAL_MERGED }); + const second = rival({ number: 8899, mergedAt: RIVAL_MERGED }); + expect(resolve({ mergedRivals: [first, second] })?.rivalPullNumber).toBe(8881); + expect(resolve({ mergedRivals: [second, first] })?.rivalPullNumber).toBe(8881); + }); + + it("resolves closures in ascending issue number regardless of input order", () => { + const low = closure({ issueNumber: 100 }); + const high = closure({ issueNumber: 900 }); + const rivals = [rival({ number: 11, linkedIssues: [100] }), rival({ number: 22, linkedIssues: [900] })]; + expect(resolve({ closures: [high, low], mergedRivals: rivals })?.issueNumber).toBe(100); + expect(resolve({ closures: [low, high], mergedRivals: rivals })?.issueNumber).toBe(100); + }); + + it("falls through a non-qualifying earlier issue to a qualifying later one", () => { + // Issue 100 is still open, so it cannot supersede; 900 did close behind a merged rival. + const stillOpen = closure({ issueNumber: 100, state: "open", closedAt: null }); + const superseded = closure({ issueNumber: 900 }); + expect( + resolve({ + closures: [stillOpen, superseded], + mergedRivals: [rival({ number: 22, linkedIssues: [900] })], + })?.issueNumber, + ).toBe(900); + }); + }); +}); From 4016c8368b031fbb2526343e02d1ee0abdd4b1bd Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 31 Jul 2026 04:51:14 -0700 Subject: [PATCH 2/4] feat(db): read merged pull requests in a time window for supersession checks The supersession resolver (#10168) needs the rivals that merged between a PR's creation and its linked issue's close. Selects only number/merged_at/linked issues, bounded by the window and capped, ordered by ascending merge time so the cap keeps the earliest merges -- the resolver elects the earliest qualifying rival, so dropping those would mis-name the cause. Reads pull_requests rather than the purpose-shaped recent_merged_pull_requests table: that table is no longer written -- its newest row on the Orb is three weeks old -- so a rival that merged minutes ago is absent from it entirely, and a check reading it would silently never fire. Refs #10168 --- src/db/repositories.ts | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 1232119e5..ac0bb7c79 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -2,7 +2,7 @@ // (dist/index.js re-exporting calibration/advisory/policy modules) measured ~420ms of cold import // under vitest — a tax paid by every test file that transitively touches repositories (#test-import-cost). import { parsePullRequestTargetKey } from "@loopover/engine/parse-pull-request-target-key"; -import { and, asc, desc, eq, gte, inArray, isNotNull, lt, not, or, sql, type SQL } from "drizzle-orm"; +import { and, asc, desc, eq, gte, inArray, isNotNull, lt, lte, not, or, sql, type SQL } from "drizzle-orm"; import { getDb } from "./client"; import { activeReviewTracking, @@ -5116,6 +5116,42 @@ export async function listOtherOpenPullRequests(env: Env, fullName: string, numb return rows.map(toPullRequestRecordFromRow); } +/** + * Merged pull requests in this repo whose merge landed inside `[sinceIso, untilIso]` (#10168) — the candidate + * rivals for a supersession check. Only the three fields that decision needs are selected. + * + * Reads `pull_requests`, deliberately NOT the purpose-shaped `recent_merged_pull_requests` table: that one is + * no longer written (its newest row on the Orb predates this by weeks), so a rival that merged minutes ago is + * simply absent from it, and a supersession check reading it would silently never fire. + * + * `merged_at` holds GitHub's Z-normalised ISO-8601, so the window comparison is a lexicographic string range + * over the stored text — the same shape every other timestamp filter in this file uses. + */ +export async function listMergedPullRequestsInWindow( + env: Env, + fullName: string, + sinceIso: string, + untilIso: string, +): Promise<{ number: number; mergedAt: string | null; linkedIssues: number[] }[]> { + const db = getDb(env.DB); + const rows = await db + .select({ number: pullRequests.number, mergedAt: pullRequests.mergedAt, linkedIssuesJson: pullRequests.linkedIssuesJson }) + .from(pullRequests) + .where( + and( + eq(pullRequests.repoFullName, fullName), + isNotNull(pullRequests.mergedAt), + gte(pullRequests.mergedAt, sinceIso), + lte(pullRequests.mergedAt, untilIso), + ), + ) + // Ascending merge time, so the cap keeps the EARLIEST merges in the window — the supersession resolver + // elects the earliest qualifying rival, so dropping those would mis-name the cause. + .orderBy(asc(pullRequests.mergedAt)) + .limit(100); + return rows.map((row) => ({ number: row.number, mergedAt: row.mergedAt, linkedIssues: parseJson(row.linkedIssuesJson, []) })); +} + // #9125: `authorGithubId` is optional and ADDITIVE -- when the caller has it, a sibling PR matches on the // immutable id OR the (renameable) login, so a contributor who renamed between two PRs still gets counted // against their own cap. Omit it and this behaves exactly as the login-only match always did. From 35e269282a029ecef9483568997f1a2271d8d5d7 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 31 Jul 2026 05:00:47 -0700 Subject: [PATCH 3/4] feat(review): report a superseded pull request as superseded, not unlinked Splits the confirmedNoOpenLinkedIssue verdict in two. A PR that cited an already-dead issue keeps reading as missing_linked_issue -- that is the gaming case the guardrail exists for. A PR whose genuinely-open issue a rival closed after it opened now gets its own code, its own message naming the rival, and an action a contributor can actually take. The two facts are proven before the split fires: the issue outlived this PR's creation, and a merged sibling citing it landed in the window ending at its close. resolveLinkedIssueHasOpenReference already fetched every linked issue and discarded all but a boolean, so the closure facts now ride out of that same pass -- no extra GitHub call. The rival comes from a bounded pull_requests read. Wired through all four gate-evaluating call sites via the shared resolveLinkedIssueAdvisoryContext, so the sweep, the webhook path, the heavy re-review and authorized PR actions cannot disagree about it. Applied to BOTH advisory twins (src/rules/advisory.ts and the engine's gate-advisory.ts), each with its own suite, since a fix to only one side is exactly the drift that pair is kept apart to expose. linked_issue_superseded rides the same linkedIssueGateMode knob it was split out of: a repo that opted into block already asked for this PR to be acted on, and splitting the message must not quietly change WHETHER it is acted on -- only what the contributor is told and which code the ledger records. It joins CONCRETE_EVIDENCE_BLOCKER_CODES on the same footing as its sibling (two recorded timestamps and a merged PR's own linked-issue set; no AI judgment) and CONFIGURED_GATE_BLOCKER_SIGNAL_CODES so its reversals record under their own id. Gated OFF by LOOPOVER_SUPERSEDED_CLOSE: recognising supersession CLOSES the pull request, so it ships dark until shadow-checked against the held backlog. Closes #10168 --- .../src/advisory/gate-advisory.ts | 42 ++++++++--- ...e-advisory-linked-issue-superseded.test.ts | 73 +++++++++++++++++++ src/env.d.ts | 6 ++ src/queue/processors.ts | 68 ++++++++++++++--- src/review/linked-issue-hard-rules.ts | 34 ++++++++- src/rules/advisory.ts | 53 +++++++++++--- src/settings/agent-actions.ts | 5 +- src/settings/superseded-close-mode.ts | 11 +++ test/unit/agent-actions.test.ts | 3 +- test/unit/linked-issue-hard-rules.test.ts | 52 ++++++++++--- test/unit/rules.test.ts | 42 +++++++++++ worker-configuration.d.ts | 4 +- wrangler.jsonc | 6 ++ 13 files changed, 354 insertions(+), 45 deletions(-) create mode 100644 packages/loopover-engine/test/gate-advisory-linked-issue-superseded.test.ts create mode 100644 src/settings/superseded-close-mode.ts diff --git a/packages/loopover-engine/src/advisory/gate-advisory.ts b/packages/loopover-engine/src/advisory/gate-advisory.ts index cb76e758c..85e46bcd9 100644 --- a/packages/loopover-engine/src/advisory/gate-advisory.ts +++ b/packages/loopover-engine/src/advisory/gate-advisory.ts @@ -195,6 +195,12 @@ export function buildPullRequestAdvisory( * — this is fail-open by construction: the caller only ever sets it true after a live check confirms * every reference is dead, never on ambiguity. */ confirmedNoOpenLinkedIssue?: boolean; + /** #10168: evidence that this PR's linked issue was closed by a rival that merged AFTER it opened. + * Present ⇒ the `confirmedNoOpenLinkedIssue` case reports as a supersession naming the rival instead of + * as `missing_linked_issue`'s unactionable "link it explicitly in the PR body". Structurally typed here + * rather than imported from the host's review/linked-issue-superseded.ts, for the same reason the rest of + * this file is a slimmed twin: @loopover/engine must not drag the host's subsystem into its graph. */ + supersededBy?: { issueNumber: number; rivalPullNumber: number } | null | undefined; } = {}, ): Advisory { const repoFullName = pr?.repoFullName ?? repo?.fullName ?? "unknown/unknown"; @@ -223,7 +229,7 @@ export function buildPullRequestAdvisory( action: "Re-deliver the webhook or wait for the next sync.", }); } else { - addPullRequestFindings(repo, pr, findings, context.otherOpenPullRequests ?? [], Boolean(context.requireLinkedIssue), Boolean(context.duplicateWinnerEnabled), context.linkedIssueAuthorLogins ?? [], Boolean(context.confirmedNoOpenLinkedIssue)); + addPullRequestFindings(repo, pr, findings, context.otherOpenPullRequests ?? [], Boolean(context.requireLinkedIssue), Boolean(context.duplicateWinnerEnabled), context.linkedIssueAuthorLogins ?? [], Boolean(context.confirmedNoOpenLinkedIssue), context.supersededBy); } return advisory("pull_request", targetKey, repoFullName, findings, "Pull request advisory generated.", pr?.number, undefined, pr?.headSha ?? undefined); } @@ -298,6 +304,8 @@ function addPullRequestFindings( duplicateWinnerEnabled: boolean, linkedIssueAuthorLogins: (string | null | undefined)[], confirmedNoOpenLinkedIssue: boolean, + // #10168: present only when the caller proved a rival merged after this PR opened and closed its issue. + supersededBy?: { issueNumber: number; rivalPullNumber: number } | null | undefined, ): void { if (pr.state !== "open") { findings.push({ @@ -314,15 +322,29 @@ function addPullRequestFindings( // which always hand in a freshly-read body and never hit the webhook race) stays byte-identical. const noLinkedIssueCited = pr.linkedIssues.length === 0 && pr.bodyObservedAt !== null; if ((noLinkedIssueCited || confirmedNoOpenLinkedIssue) && requireLinkedIssue) { - findings.push({ - code: "missing_linked_issue", - severity: "warning", - title: "No linked issue detected", - detail: noLinkedIssueCited - ? "No closing reference or linked issue number was found in the PR metadata/body." - : "The PR cites an issue number, but it could not be verified as a currently open issue.", - action: "If this PR is intended to solve an issue, link it explicitly in the PR body.", - }); + // #10168 (host-parity): a PR whose linked issue a merged rival closed did NOT fail to link an issue -- it + // linked one correctly and lost a race, and telling it to "link it explicitly" is advice that cannot + // work. See the host copy (src/rules/advisory.ts) for the full rationale and the two facts the caller + // must prove before setting this. + if (supersededBy) { + findings.push({ + code: "linked_issue_superseded", + severity: "warning", + title: "Superseded by a merged pull request", + detail: `Issue #${supersededBy.issueNumber} was closed by #${supersededBy.rivalPullNumber}, which merged after this pull request opened. The work this pull request targets is already on the default branch.`, + action: `Nothing is wrong with the issue link. If part of this pull request is still unaddressed by #${supersededBy.rivalPullNumber}, open a new issue describing what remains.`, + }); + } else { + findings.push({ + code: "missing_linked_issue", + severity: "warning", + title: "No linked issue detected", + detail: noLinkedIssueCited + ? "No closing reference or linked issue number was found in the PR metadata/body." + : "The PR cites an issue number, but it could not be verified as a currently open issue.", + action: "If this PR is intended to solve an issue, link it explicitly in the PR body.", + }); + } } else { const overlappingPrs = otherOpenPullRequests.filter((otherPr) => otherPr.linkedIssues.some((issueNumber) => pr.linkedIssues.includes(issueNumber)), diff --git a/packages/loopover-engine/test/gate-advisory-linked-issue-superseded.test.ts b/packages/loopover-engine/test/gate-advisory-linked-issue-superseded.test.ts new file mode 100644 index 000000000..aad125b93 --- /dev/null +++ b/packages/loopover-engine/test/gate-advisory-linked-issue-superseded.test.ts @@ -0,0 +1,73 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +// #10168, engine-twin half. The host copy (src/rules/advisory.ts) has the same split and its own suite; both +// are covered because gate-advisory is a deliberately-divergent twin, so a fix applied to only one side is +// exactly the drift this pair of suites exists to catch. +// +// The case: a contributor links a genuinely open issue, a rival PR citing the same issue merges first, and +// the issue closes behind it. The PR then reads as "no open linked issue" -- and telling that contributor to +// "link it explicitly in the PR body" is advice that cannot work. + +const repo = { fullName: "o/r", defaultBranch: "main" } as never; +const supersededPr = { + repoFullName: "o/r", + number: 8886, + title: "fix: a thing", + state: "open", + authorLogin: "someone", + authorAssociation: "CONTRIBUTOR", + labels: [], + linkedIssues: [8829], + bodyObservedAt: "2026-07-31T09:22:36Z", +}; +const supersededBy = { issueNumber: 8829, rivalPullNumber: 8881 }; + +test("a superseded PR is reported as superseded, naming the rival that merged", async () => { + const { buildPullRequestAdvisory } = await import("../dist/advisory/gate-advisory.js"); + const advisory = buildPullRequestAdvisory(repo, supersededPr as never, { + requireLinkedIssue: true, + confirmedNoOpenLinkedIssue: true, + supersededBy, + }); + + const finding = advisory.findings.find((f) => f.code === "linked_issue_superseded"); + assert.ok(finding, "the supersession finding is raised"); + assert.equal(finding.title, "Superseded by a merged pull request"); + assert.match(finding.detail, /#8829 was closed by #8881/); + assert.ok( + !advisory.findings.some((f) => f.code === "missing_linked_issue"), + "the unactionable missing_linked_issue reading is replaced, not doubled up", + ); + assert.ok(!/link it explicitly in the PR body/.test(finding.action ?? ""), "the advice that cannot work is gone"); + assert.match(finding.action ?? "", /#8881/, "the remedy points at the rival that actually landed"); +}); + +test("without proven supersession the anti-gaming reading is unchanged", async () => { + const { buildPullRequestAdvisory } = await import("../dist/advisory/gate-advisory.js"); + for (const value of [undefined, null]) { + const advisory = buildPullRequestAdvisory(repo, supersededPr as never, { + requireLinkedIssue: true, + confirmedNoOpenLinkedIssue: true, + supersededBy: value, + }); + assert.ok( + advisory.findings.some((f) => f.code === "missing_linked_issue"), + `missing_linked_issue still fires for supersededBy=${String(value)}`, + ); + assert.ok( + !advisory.findings.some((f) => f.code === "linked_issue_superseded"), + `no supersession is claimed for supersededBy=${String(value)}`, + ); + } +}); + +test("supersession never fires while the linked-issue requirement is off", async () => { + const { buildPullRequestAdvisory } = await import("../dist/advisory/gate-advisory.js"); + const advisory = buildPullRequestAdvisory(repo, supersededPr as never, { + requireLinkedIssue: false, + confirmedNoOpenLinkedIssue: true, + supersededBy, + }); + assert.ok(!advisory.findings.some((f) => f.code === "linked_issue_superseded")); +}); diff --git a/src/env.d.ts b/src/env.d.ts index a5bee78c2..794d2a3a3 100644 --- a/src/env.d.ts +++ b/src/env.d.ts @@ -735,6 +735,12 @@ declare global { * unchanged). Once a winner closes, the next-lowest OPEN sibling becomes the winner on re-eval. See * src/signals/duplicate-winner.ts. */ LOOPOVER_DUPLICATE_WINNER?: string; + /** Superseded-PR recognition (#10168): when truthy, a PR whose linked issue was closed by a rival that + * merged AFTER this PR opened is reported as superseded — its own finding, its own message naming the + * rival — instead of the unactionable "No linked issue detected", and closes as superseded rather than + * holding forever. OFF by default: it changes the close disposition. See + * src/review/linked-issue-superseded.ts. */ + LOOPOVER_SUPERSEDED_CLOSE?: string; /** Open-PR file-path collision (#2653): when truthy, a live PR review enriches its own and its open * siblings' `changedFiles` from the `pull_request_files` cache (a plain D1 read — no extra GitHub calls) * before building the collision report, so two independently-open PRs touching the same file are flagged diff --git a/src/queue/processors.ts b/src/queue/processors.ts index cd4c56af5..c2c1ee4b4 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -25,6 +25,7 @@ import { listIssueSignalSample, listLatestSignalSnapshotsByTarget, listOtherOpenPullRequests, + listMergedPullRequestsInWindow, listOtherOpenPullRequestsForAuthor, listOpenIssues, listOpenPullRequests, @@ -349,6 +350,8 @@ import { } from "../signals/engine"; import { isDuplicateClusterWinnerByClaim } from "../signals/duplicate-winner"; import { isDuplicateWinnerEnabledGlobally, resolveDuplicateWinnerEnabled } from "../settings/duplicate-winner-mode"; +import { isSupersededCloseEnabledGlobally } from "../settings/superseded-close-mode"; +import { SUPERSEDED_CLOSE_WINDOW_MS, resolveSupersession, type LinkedIssueClosure, type SupersededByRival } from "../review/linked-issue-superseded"; import { isOpenPrFileCollisionEnabledGlobally, resolveOpenPrFileCollisionEnabled } from "../settings/open-pr-file-collision-mode"; import { buildAiReviewDiff, buildSecretScanDiff, totalAddedLineCount } from "../review/review-diff"; // #4013 step 4 (prep): buildAiReviewDiff/buildSecretScanDiff moved to review-diff.ts (a natural existing @@ -1609,12 +1612,13 @@ export async function sweepRepoRegate( // Thread linked-issue authors + the open-reference check so the re-gate sweep applies the same // self-authored-linked-issue block AND stale-issue-link countermeasure the main webhook path applies — // without this a self-authored or stale-link-gaming PR re-gated by the sweep escapes both. (#self-authored-parity, #unlinked-issue-guardrail-followup) - const { linkedIssueAuthorLogins, confirmedNoOpenLinkedIssue } = await resolveLinkedIssueAdvisoryContext( + const { linkedIssueAuthorLogins, confirmedNoOpenLinkedIssue, supersededBy } = await resolveLinkedIssueAdvisoryContext( env, sweepInstallationId, repoFullName, pr.linkedIssues, settings, + pr, ); // #9160: see resolveScopedLinkedIssueClaimedAt's own doc comment -- scopes pr's claim time to only the // issue(s) actually contested with an open sibling instead of pr's blended linkedIssueClaimedAt column. @@ -1626,6 +1630,7 @@ export async function sweepRepoRegate( duplicateWinnerEnabled, linkedIssueAuthorLogins, confirmedNoOpenLinkedIssue, + supersededBy, copycatGateMode: settings.copycatGateMode, copycatGateMinScore: settings.copycatGateMinScore, scopedLinkedIssueClaimedAt, @@ -4364,10 +4369,10 @@ export async function reReviewStoredPullRequest( pr.number, pr.headSha, ); - const [cachedOtherOpenPullRequests, { linkedIssueAuthorLogins, confirmedNoOpenLinkedIssue }] = + const [cachedOtherOpenPullRequests, { linkedIssueAuthorLogins, confirmedNoOpenLinkedIssue, supersededBy }] = await Promise.all([ listOtherOpenPullRequests(env, repoFullName, prNumber), - resolveLinkedIssueAdvisoryContext(env, installationId, repoFullName, pr.linkedIssues, settings), + resolveLinkedIssueAdvisoryContext(env, installationId, repoFullName, pr.linkedIssues, settings, pr), ]); // #dup-winner / audit #15: drop any cached-open duplicate sibling already closed on GitHub before the advisory // (and the disposition below) elect the cluster winner, so the real lowest-OPEN PR is never demoted+auto-closed. @@ -4391,6 +4396,7 @@ export async function reReviewStoredPullRequest( requireLinkedIssue: shouldCollectLinkedIssueEvidence(settings), duplicateWinnerEnabled: duplicateWinnerEnabledForPr, confirmedNoOpenLinkedIssue, + supersededBy, linkedIssueAuthorLogins, copycatGateMode: settings.copycatGateMode, copycatGateMinScore: settings.copycatGateMinScore, @@ -7491,11 +7497,11 @@ async function handlePullRequestWebhookEvent( // column, so reading late would always see this pass's own bookkeeping write instead of the real prior // review pass's recorded visual_unrelated_issue_finding. Gated on `closed` (the only action // maybePostVisualFollowupComment ever fires for) so every other action skips this read entirely. - const [repo, cachedOtherOpenPullRequests, { linkedIssueAuthorLogins, confirmedNoOpenLinkedIssue }, priorAdvisoryForVisualFollowup] = + const [repo, cachedOtherOpenPullRequests, { linkedIssueAuthorLogins, confirmedNoOpenLinkedIssue, supersededBy }, priorAdvisoryForVisualFollowup] = await Promise.all([ getRepository(env, repoFullName), listOtherOpenPullRequests(env, repoFullName, pr.number), - resolveLinkedIssueAdvisoryContext(env, installationId, repoFullName, pr.linkedIssues, settings), + resolveLinkedIssueAdvisoryContext(env, installationId, repoFullName, pr.linkedIssues, settings, pr), payload.action === "closed" && installationId ? getLatestAdvisoryForPullRequest(env, repoFullName, pr.number) : Promise.resolve(null), ]); // #dup-winner / audit #15: drop any cached-open duplicate sibling already closed on GitHub before the @@ -7520,6 +7526,7 @@ async function handlePullRequestWebhookEvent( requireLinkedIssue: shouldCollectLinkedIssueEvidence(settings), duplicateWinnerEnabled: duplicateWinnerEnabledForPr, confirmedNoOpenLinkedIssue, + supersededBy, linkedIssueAuthorLogins, copycatGateMode: settings.copycatGateMode, copycatGateMinScore: settings.copycatGateMinScore, @@ -8313,12 +8320,51 @@ async function resolveLinkedIssueAdvisoryContext( repoFullName: string, linkedIssues: number[], settings: Pick, -): Promise<{ linkedIssueAuthorLogins: (string | null)[]; confirmedNoOpenLinkedIssue: boolean }> { - const [linkedIssueAuthorLogins, hasOpenReference] = await Promise.all([ + // #10168: needed only to distinguish a superseded PR from one that cited a dead issue -- the check is + // "was this issue still open when THIS pull request was created". + pr: Pick, +): Promise<{ linkedIssueAuthorLogins: (string | null)[]; confirmedNoOpenLinkedIssue: boolean; supersededBy: SupersededByRival | null }> { + const [linkedIssueAuthorLogins, reference] = await Promise.all([ resolveLinkedIssueAuthorLogins(env, installationId, repoFullName, linkedIssues, settings.selfAuthoredLinkedIssueGateMode === "block"), - settings.linkedIssueGateMode === "block" ? resolveLinkedIssueHasOpenReference({ env, repoFullName, linkedIssues, installationId }) : Promise.resolve(true), + settings.linkedIssueGateMode === "block" + ? resolveLinkedIssueHasOpenReference({ env, repoFullName, linkedIssues, installationId }) + : Promise.resolve({ hasOpenReference: true, closures: [] }), ]); - return { linkedIssueAuthorLogins, confirmedNoOpenLinkedIssue: !hasOpenReference }; + const confirmedNoOpenLinkedIssue = !reference.hasOpenReference; + // Only a PR that ALREADY reads as having no open linked issue can be superseded -- this splits that one + // verdict in two, it never creates a new one. Gated OFF by default (#10168): recognising supersession + // closes the PR. Best effort: a failed lookup yields null, which keeps today's `missing_linked_issue`. + const supersededBy = + confirmedNoOpenLinkedIssue && isSupersededCloseEnabledGlobally(env) + ? await resolveSupersededRival(env, repoFullName, pr, reference.closures).catch(() => null) + : null; + return { linkedIssueAuthorLogins, confirmedNoOpenLinkedIssue, supersededBy }; +} + +/** + * #10168: find the merged rival that closed this PR's linked issue, if there is one. + * + * The candidate window runs from this PR's own creation to the latest observed issue close plus the + * resolver's tolerance -- the smallest range that can still contain a qualifying merge, so the DB read stays + * bounded no matter how long the PR has been sitting. Returns null when the PR has no synced `createdAt` or + * no conclusively-read closed issue, because neither half of the evidence can be established without them. + */ +async function resolveSupersededRival( + env: Env, + repoFullName: string, + pr: Pick, + closures: LinkedIssueClosure[], +): Promise { + const createdAt = pr.createdAt; + if (!createdAt) return null; + const closedInstants = closures.flatMap((closure) => { + const parsed = closure.closedAt === null ? Number.NaN : Date.parse(closure.closedAt); + return Number.isFinite(parsed) ? [parsed] : []; + }); + if (closedInstants.length === 0) return null; + const until = new Date(Math.max(...closedInstants) + SUPERSEDED_CLOSE_WINDOW_MS).toISOString(); + const mergedRivals = await listMergedPullRequestsInWindow(env, repoFullName, createdAt, until); + return resolveSupersession({ prNumber: pr.number, prCreatedAt: createdAt, closures, mergedRivals }); } export async function shouldRefreshFilesForPreMergeChecks( @@ -15171,12 +15217,13 @@ export async function buildAuthorizedPrActionAdvisory( // Mirror the main webhook path: thread linked-issue authors + the open-reference check so an authorized PR // action (gate-override / panel retrigger) honors the same self-authored-linked-issue block AND stale- // issue-link countermeasure. installationId comes from the repo record. (#self-authored-parity, #unlinked-issue-guardrail-followup) - const { linkedIssueAuthorLogins, confirmedNoOpenLinkedIssue } = await resolveLinkedIssueAdvisoryContext( + const { linkedIssueAuthorLogins, confirmedNoOpenLinkedIssue, supersededBy } = await resolveLinkedIssueAdvisoryContext( env, repo?.installationId ?? null, repoFullName, pr.linkedIssues, settings, + pr, ); const duplicateWinnerEnabledForPr = resolveDuplicateWinnerEnabled(isDuplicateWinnerEnabledGlobally(env), settings.duplicateWinnerMode); // #9160: see resolveScopedLinkedIssueClaimedAt's own doc comment -- scopes pr's claim time to only the @@ -15190,6 +15237,7 @@ export async function buildAuthorizedPrActionAdvisory( requireLinkedIssue: shouldCollectLinkedIssueEvidence(settings), duplicateWinnerEnabled: duplicateWinnerEnabledForPr, confirmedNoOpenLinkedIssue, + supersededBy, linkedIssueAuthorLogins, copycatGateMode: settings.copycatGateMode, copycatGateMinScore: settings.copycatGateMinScore, diff --git a/src/review/linked-issue-hard-rules.ts b/src/review/linked-issue-hard-rules.ts index cd333c1f2..15d57323d 100644 --- a/src/review/linked-issue-hard-rules.ts +++ b/src/review/linked-issue-hard-rules.ts @@ -1,4 +1,5 @@ import { fetchLinkedIssueFacts, type LinkedIssueFactsFetch } from "../github/backfill"; +import type { LinkedIssueClosure } from "./linked-issue-superseded"; import { githubRateLimitAdmissionKeyForToken } from "../github/client"; import { createInstallationToken } from "../github/app"; import { extractLinkedIssueNumbersWithOverflow, MAX_LINKED_ISSUE_NUMBERS } from "../db/repositories"; @@ -296,6 +297,31 @@ export function hasVerifiableOpenLinkedIssueReference(fetchResults: LinkedIssueF return fetchResults.some((result) => result.status === "fetch_error"); } +/** + * What one linked-issue verification pass learned (#10168): the anti-gaming boolean it has always produced, + * plus the per-issue closure facts the SAME fetch already returned and used to discard. + * + * Carrying both is what lets the advisory distinguish a PR that cited a dead issue from one whose live issue + * a rival closed underneath it — without a second network call, since `closed_at` rides the issue payload + * `fetchLinkedIssueFacts` already reads (#4528). + */ +export type LinkedIssueReferenceCheck = { + hasOpenReference: boolean; + closures: LinkedIssueClosure[]; +}; + +/** The fail-open answer for a pass that never fetched anything (no citations, or more than the fan-out cap): + * no evidence gathered, so no supersession can be claimed either. */ +const NO_LINKED_ISSUE_REFERENCES: LinkedIssueReferenceCheck = { hasOpenReference: true, closures: [] }; + +/** PURE. The closure facts for every issue we conclusively READ. `not_found`/`fetch_error` contribute + * nothing — a supersession must rest on an issue we actually saw closed, never on one we failed to fetch. */ +function linkedIssueClosures(fetchResults: LinkedIssueFactsFetch[]): LinkedIssueClosure[] { + return fetchResults.flatMap((result) => + result.status === "found" ? [{ issueNumber: result.facts.number, state: result.facts.state, closedAt: result.facts.closedAt }] : [], + ); +} + /** * Orchestrate the live per-issue fetch for {@link hasVerifiableOpenLinkedIssueReference}. Mints its own * installation token (falling back to the public token, exactly like fetchLinkedIssueFacts's own @@ -308,17 +334,17 @@ export async function resolveLinkedIssueHasOpenReference(args: { repoFullName: string; linkedIssues: number[]; installationId?: number | null | undefined; -}): Promise { - if (args.linkedIssues.length === 0) return true; +}): Promise { + if (args.linkedIssues.length === 0) return NO_LINKED_ISSUE_REFERENCES; // Fail open (mirrors hasVerifiableOpenLinkedIssueReference's own ambiguity philosophy above) rather than // firing an unbounded per-issue fan-out for a body citing more references than can be safely verified in // one pass -- the same cap resolveLinkedIssueHardRule's own extractLinkedIssueNumbersWithOverflow enforces // on the sibling gate, reused here instead of a second bound so a noisy body can't create surprise API // pressure on this path. - if (args.linkedIssues.length > MAX_LINKED_ISSUE_NUMBERS) return true; + if (args.linkedIssues.length > MAX_LINKED_ISSUE_NUMBERS) return NO_LINKED_ISSUE_REFERENCES; const ciToken = args.installationId ? await createInstallationToken(args.env, args.installationId).catch(() => undefined) : undefined; const token = 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))); - return hasVerifiableOpenLinkedIssueReference(fetchResults); + return { hasOpenReference: hasVerifiableOpenLinkedIssueReference(fetchResults), closures: linkedIssueClosures(fetchResults) }; } diff --git a/src/rules/advisory.ts b/src/rules/advisory.ts index 9a165e574..618fa6c50 100644 --- a/src/rules/advisory.ts +++ b/src/rules/advisory.ts @@ -47,6 +47,7 @@ import { CONFIDENCE_WHEN_UNSTATED } from "../services/ai-review"; import { LOOPOVER_GATE_CHECK_NAME } from "../review/check-names"; import { CLA_CHECK_UNRESOLVED_CODE, CLA_CONSENT_MISSING_CODE } from "../review/cla-check"; import { REVIEW_THREAD_BLOCKER_CODE } from "../review/review-thread-findings"; +import type { SupersededByRival } from "../review/linked-issue-superseded"; import { createSignalStore } from "../review/signal-tracking-wire"; import { labelMatchesPattern } from "../scoring/preview"; import { isMaintainerAuthorAssociation } from "../github/author-association"; @@ -209,6 +210,10 @@ export const GATE_SCORE_SIGNAL_CODES: readonly string[] = Object.freeze(["slop_g export const CONFIGURED_GATE_BLOCKER_SIGNAL_CODES: readonly string[] = Object.freeze([ "missing_linked_issue", + // #10168: the superseded split-off of missing_linked_issue. Listed here for the same reason its sibling is + // -- it gates a real close, so its reversals must record under their own id rather than under the code it + // was split out of, or the per-rule precision check in downgradeCloseToHold can never see it. + "linked_issue_superseded", "duplicate_pr_risk", ...AI_JUDGMENT_BLOCKER_CODES, REVIEW_THREAD_BLOCKER_CODE, @@ -392,6 +397,12 @@ export function buildPullRequestAdvisory( * — this is fail-open by construction: the caller only ever sets it true after a live check confirms * every reference is dead, never on ambiguity. */ confirmedNoOpenLinkedIssue?: boolean; + /** #10168: the evidence that this PR's linked issue was closed by a rival that merged AFTER it opened, + * resolved by the caller (`resolveSupersession`, review/linked-issue-superseded.ts). Present ⇒ the + * `confirmedNoOpenLinkedIssue` case is reported as a supersession naming the rival, instead of as + * `missing_linked_issue`'s unactionable "link it explicitly in the PR body". Absent/null ⇒ byte-identical + * to before this existed, which is also what a fleet with the flag off always sees. */ + supersededBy?: SupersededByRival | null | undefined; /** #9033: the repo's EFFECTIVE `copycatGateMode`/`copycatGateMinScore` (already resolved by the caller via * `resolveRepositorySettings` — reward-eligible repos default this to `warn` even with no `.loopover.yml` * entry, see `settings/copycat-gate-mode.ts`). Used ONLY to decide whether `pr`'s own persisted copycat @@ -449,6 +460,7 @@ export function buildPullRequestAdvisory( context.copycatGateMode, context.copycatGateMinScore, context.scopedLinkedIssueClaimedAt, + context.supersededBy, ); } return advisory("pull_request", targetKey, repoFullName, findings, "Pull request advisory generated.", pr?.number, undefined, pr?.headSha ?? undefined); @@ -1055,6 +1067,8 @@ function addPullRequestFindings( // `undefined` (every caller that hasn't been updated, and every non-DB caller like decision-replay.ts) falls // back to pr.linkedIssueClaimedAt, byte-identical to before this existed. scopedLinkedIssueClaimedAt?: string | null | undefined, + // #10168: present only when the caller proved a rival merged after this PR opened and closed its issue. + supersededBy?: SupersededByRival | null | undefined, ): void { if (pr.state !== "open") { findings.push({ @@ -1073,15 +1087,31 @@ function addPullRequestFindings( // issues API, which presupposes a real body was already parsed. const noLinkedIssueCited = pr.linkedIssues.length === 0 && pr.bodyObservedAt !== null; if ((noLinkedIssueCited || confirmedNoOpenLinkedIssue) && requireLinkedIssue) { - findings.push({ - code: "missing_linked_issue", - severity: "warning", - title: "No linked issue detected", - detail: noLinkedIssueCited - ? "No closing reference or linked issue number was found in the PR metadata/body." - : "The PR cites an issue number, but it could not be verified as a currently open issue.", - action: "If this PR is intended to solve an issue, link it explicitly in the PR body.", - }); + // #10168: a PR whose linked issue a merged rival closed did NOT fail to link an issue -- it linked one + // correctly and lost a race. Reporting that as `missing_linked_issue` gives advice that cannot work + // (re-linking a closed issue changes nothing), so the superseded case gets its own code and message. + // `supersededBy` is only ever set once the caller has proven both halves (the issue outlived this PR's + // creation, and a rival citing it merged into the window ending at its close), so this never displaces + // the anti-gaming reading for a PR that cited an already-dead issue. + if (supersededBy) { + findings.push({ + code: "linked_issue_superseded", + severity: "warning", + title: "Superseded by a merged pull request", + detail: `Issue #${supersededBy.issueNumber} was closed by #${supersededBy.rivalPullNumber}, which merged after this pull request opened. The work this pull request targets is already on the default branch.`, + action: `Nothing is wrong with the issue link. If part of this pull request is still unaddressed by #${supersededBy.rivalPullNumber}, open a new issue describing what remains.`, + }); + } else { + findings.push({ + code: "missing_linked_issue", + severity: "warning", + title: "No linked issue detected", + detail: noLinkedIssueCited + ? "No closing reference or linked issue number was found in the PR metadata/body." + : "The PR cites an issue number, but it could not be verified as a currently open issue.", + action: "If this PR is intended to solve an issue, link it explicitly in the PR body.", + }); + } } else { const linkedIssueOverlapPrs = otherOpenPullRequests.filter((otherPr) => otherPr.linkedIssues.some((issueNumber) => pr.linkedIssues.includes(issueNumber)), @@ -1290,6 +1320,11 @@ function resolveConfiguredGateMode(finding: AdvisoryFinding, policy: GateCheckPo // Missing linked issue defaults to ADVISORY — issues aren't always available, so it only blocks when a // repo explicitly opts in with linkedIssueGateMode: "block". if (code === "missing_linked_issue") return gateMode(policy.linkedIssueGateMode ?? "advisory"); + // #10168: supersession rides the SAME knob it was split out of. A repo that opted into + // `linkedIssueGateMode: "block"` already asked for an unlinked PR to be acted on; splitting the message in + // two must not quietly change WHETHER it is acted on, only what the contributor is told and which code the + // ledger records. A repo on the "advisory" default keeps getting an advisory finding here too. + if (code === "linked_issue_superseded") return gateMode(policy.linkedIssueGateMode ?? "advisory"); // #9129: default changed from "block" to "advisory" — the input this finding is derived from (another // contributor's own PR body text) is adversary-controlled, so blocking-by-default let anyone force-close a // rival's PR for free. A maintainer who explicitly opts into "block" still gets real effect: evaluateGateCheckCore diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index ca9fad11d..0219e2000 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -209,7 +209,7 @@ export type PlannedAgentAction = { // scoring/model.ts), and spreading/reading another module's export INTO A TOP-LEVEL ARRAY LITERAL evaluates it // eagerly at module-load time, before that module has necessarily finished initializing on this cycle's first // pass -- confirmed by a real "X is not iterable" failure when that was tried. A plain literal has no such -// hazard. A source-text parity test in the test file below guards all ten against producer-side drift instead. +// hazard. A source-text parity test in the test file below guards all eleven against producer-side drift instead. const CONCRETE_EVIDENCE_BLOCKER_CODES = new Set([ "secret_leak", "duplicate_pr_risk", @@ -219,6 +219,9 @@ const CONCRETE_EVIDENCE_BLOCKER_CODES = new Set([ "pre_merge_check_required", "lockfile_tamper_risk", "missing_linked_issue", + // #10168: the superseded split-off of missing_linked_issue, and concrete on the same footing -- two + // recorded timestamps and a merged sibling's own linked-issue set, no AI judgment anywhere in it. + "linked_issue_superseded", "self_authored_linked_issue", // #content-lane-deliverable: a text/path match against the resolved RegistryLaneSpec, no AI judgment involved -- // same deterministic footing as surface_lane_reject immediately above. diff --git a/src/settings/superseded-close-mode.ts b/src/settings/superseded-close-mode.ts new file mode 100644 index 000000000..2e5e0b305 --- /dev/null +++ b/src/settings/superseded-close-mode.ts @@ -0,0 +1,11 @@ +/** Truthy convention matches the rest of this codebase's `LOOPOVER_*` flags (`/^(1|true|yes|on)$/i`, trimmed + * + case-insensitive, e.g. `isDuplicateWinnerEnabledGlobally`) -- so `1`, `on`, `TRUE`, and a `.env` value + * carrying trailing whitespace all read as truthy, not silently as OFF. + * + * Opt-in and default OFF, for the same reason the duplicate-winner flag is (#10168): recognising a PR as + * superseded CLOSES it, which is a real, irreversible change to the close disposition rather than a + * low-risk default. Until a fleet operator sets this, the supersession finding is never produced and every + * affected PR keeps exactly the disposition it has today. */ +export function isSupersededCloseEnabledGlobally(env: { LOOPOVER_SUPERSEDED_CLOSE?: string | undefined }): boolean { + return /^(1|true|yes|on)$/i.test((env.LOOPOVER_SUPERSEDED_CLOSE ?? "").trim()); +} diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index 0f3d86f45..91f288bde 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -2265,7 +2265,7 @@ describe("module-load cycle safety (#module-cycle-regression)", () => { }); }); -// #hard-blockers-not-ai-judgment parity guard (nit): CONCRETE_EVIDENCE_BLOCKER_CODES hand-types all 10 of its +// #hard-blockers-not-ai-judgment parity guard (nit): CONCRETE_EVIDENCE_BLOCKER_CODES hand-types all 11 of its // literals rather than importing any of them from their producers, even where a producer DOES export a // reusable constant (advisory.ts's DUPLICATE_ONLY_BLOCKER_CODES, pre-merge-checks.ts's // PRE_MERGE_CHECK_BLOCKING_CODE) -- see the doc comment on CONCRETE_EVIDENCE_BLOCKER_CODES for why: this module @@ -2284,6 +2284,7 @@ describe("CONCRETE_EVIDENCE_BLOCKER_CODES parity — hand-typed literals still m { code: "pre_merge_check_required", file: "packages/loopover-engine/src/review/pre-merge-checks.ts" }, { code: "lockfile_tamper_risk", file: "src/review/lockfile-tamper.ts" }, { code: "missing_linked_issue", file: "src/rules/advisory.ts" }, + { code: "linked_issue_superseded", file: "src/rules/advisory.ts" }, { code: "self_authored_linked_issue", file: "src/rules/advisory.ts" }, { code: "content_lane_deliverable_missing", file: "src/queue/processors.ts" }, ]; diff --git a/test/unit/linked-issue-hard-rules.test.ts b/test/unit/linked-issue-hard-rules.test.ts index 695fcd42a..83533aab5 100644 --- a/test/unit/linked-issue-hard-rules.test.ts +++ b/test/unit/linked-issue-hard-rules.test.ts @@ -814,7 +814,7 @@ describe("resolveLinkedIssueHasOpenReference (#unlinked-issue-guardrail-followup const fetchSpy = vi.fn(); vi.stubGlobal("fetch", fetchSpy); const result = await resolveLinkedIssueHasOpenReference({ env: createTestEnv({}), repoFullName: "owner/repo", linkedIssues: [] }); - expect(result).toBe(true); + expect(result.hasOpenReference).toBe(true); expect(fetchSpy).not.toHaveBeenCalled(); }); @@ -823,7 +823,7 @@ describe("resolveLinkedIssueHasOpenReference (#unlinked-issue-guardrail-followup vi.stubGlobal("fetch", fetchSpy); const tooMany = Array.from({ length: MAX_LINKED_ISSUE_NUMBERS + 1 }, (_, i) => i + 1); const result = await resolveLinkedIssueHasOpenReference({ env: createTestEnv({}), repoFullName: "owner/repo", linkedIssues: tooMany }); - expect(result).toBe(true); + expect(result.hasOpenReference).toBe(true); expect(fetchSpy).not.toHaveBeenCalled(); }); @@ -833,7 +833,7 @@ describe("resolveLinkedIssueHasOpenReference (#unlinked-issue-guardrail-followup ); const atCap = Array.from({ length: MAX_LINKED_ISSUE_NUMBERS }, (_, i) => i + 1); const result = await resolveLinkedIssueHasOpenReference({ env: createTestEnv({}), repoFullName: "owner/repo", linkedIssues: atCap }); - expect(result).toBe(true); + expect(result.hasOpenReference).toBe(true); }); it("returns true when the linked issue is confirmed open", async () => { @@ -841,7 +841,7 @@ describe("resolveLinkedIssueHasOpenReference (#unlinked-issue-guardrail-followup input.toString().includes("/issues/") ? Response.json({ number: 7, state: "open", labels: [], assignees: [] }) : new Response("missing", { status: 404 }), ); const result = await resolveLinkedIssueHasOpenReference({ env: createTestEnv({}), repoFullName: "owner/repo", linkedIssues: [7] }); - expect(result).toBe(true); + expect(result.hasOpenReference).toBe(true); }); it("returns false when the linked issue is confirmed CLOSED — the exact stale-link gaming case", async () => { @@ -849,13 +849,13 @@ describe("resolveLinkedIssueHasOpenReference (#unlinked-issue-guardrail-followup input.toString().includes("/issues/") ? Response.json({ number: 7, state: "closed", labels: [], assignees: [] }) : new Response("missing", { status: 404 }), ); const result = await resolveLinkedIssueHasOpenReference({ env: createTestEnv({}), repoFullName: "owner/repo", linkedIssues: [7] }); - expect(result).toBe(false); + expect(result.hasOpenReference).toBe(false); }); it("fails open (true) when the fetch errors transiently rather than confirming the issue is dead", async () => { vi.stubGlobal("fetch", async () => new Response("server error", { status: 500 })); const result = await resolveLinkedIssueHasOpenReference({ env: createTestEnv({}), repoFullName: "owner/repo", linkedIssues: [7] }); - expect(result).toBe(true); + expect(result.hasOpenReference).toBe(true); }); it("still resolves correctly (via the public-token fallback) when no installationId is supplied at all", async () => { @@ -863,7 +863,7 @@ describe("resolveLinkedIssueHasOpenReference (#unlinked-issue-guardrail-followup input.toString().includes("/issues/") ? Response.json({ number: 7, state: "closed", labels: [], assignees: [] }) : new Response("missing", { status: 404 }), ); const result = await resolveLinkedIssueHasOpenReference({ env: createTestEnv({}), repoFullName: "owner/repo", linkedIssues: [7], installationId: null }); - expect(result).toBe(false); + expect(result.hasOpenReference).toBe(false); }); it("falls back to the public token (and still resolves) when installationId is set but token minting fails", async () => { @@ -871,7 +871,7 @@ describe("resolveLinkedIssueHasOpenReference (#unlinked-issue-guardrail-followup input.toString().includes("/app/installations/") ? new Response("forbidden", { status: 403 }) : input.toString().includes("/issues/") ? Response.json({ number: 7, state: "open", labels: [], assignees: [] }) : new Response("missing", { status: 404 }), ); const result = await resolveLinkedIssueHasOpenReference({ env: createTestEnv({}), repoFullName: "owner/repo", linkedIssues: [7], installationId: 123 }); - expect(result).toBe(true); + expect(result.hasOpenReference).toBe(true); }); it("checks multiple linked issues and is true when only one of several is open", async () => { @@ -882,7 +882,41 @@ describe("resolveLinkedIssueHasOpenReference (#unlinked-issue-guardrail-followup return new Response("missing", { status: 404 }); }); const result = await resolveLinkedIssueHasOpenReference({ env: createTestEnv({}), repoFullName: "owner/repo", linkedIssues: [1, 2] }); - expect(result).toBe(true); + expect(result.hasOpenReference).toBe(true); + }); + + // #10168: the same fetch that answers the anti-gaming question also carries each issue's closure facts, so + // the supersession check costs no extra GitHub call. + it("carries the per-issue closure facts the same fetch already returned", async () => { + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.endsWith("/issues/1")) return Response.json({ number: 1, state: "closed", closed_at: "2026-07-31T09:30:25Z", labels: [], assignees: [] }); + if (url.endsWith("/issues/2")) return Response.json({ number: 2, state: "open", closed_at: null, labels: [], assignees: [] }); + return new Response("missing", { status: 404 }); + }); + const result = await resolveLinkedIssueHasOpenReference({ env: createTestEnv({}), repoFullName: "owner/repo", linkedIssues: [1, 2] }); + expect(result.closures).toEqual([ + { issueNumber: 1, state: "closed", closedAt: "2026-07-31T09:30:25Z" }, + { issueNumber: 2, state: "open", closedAt: null }, + ]); + }); + + it("reports no closures when nothing was fetched, so no supersession can rest on a skipped pass", async () => { + const empty = await resolveLinkedIssueHasOpenReference({ env: createTestEnv({}), repoFullName: "owner/repo", linkedIssues: [] }); + expect(empty.closures).toEqual([]); + const overCap = await resolveLinkedIssueHasOpenReference({ + env: createTestEnv({}), + repoFullName: "owner/repo", + linkedIssues: Array.from({ length: MAX_LINKED_ISSUE_NUMBERS + 1 }, (_, index) => index + 1), + }); + expect(overCap.closures).toEqual([]); + }); + + it("contributes nothing for an issue that could not be read — a supersession needs an issue we SAW closed", async () => { + vi.stubGlobal("fetch", async () => new Response("boom", { status: 500 })); + const result = await resolveLinkedIssueHasOpenReference({ env: createTestEnv({}), repoFullName: "owner/repo", linkedIssues: [7] }); + expect(result.closures).toEqual([]); + expect(result.hasOpenReference).toBe(true); }); }); diff --git a/test/unit/rules.test.ts b/test/unit/rules.test.ts index d6880e2d7..130c64de5 100644 --- a/test/unit/rules.test.ts +++ b/test/unit/rules.test.ts @@ -181,6 +181,48 @@ describe("advisory rules", () => { expect(finding?.detail).toBe("No closing reference or linked issue number was found in the PR metadata/body."); }); + // #10168: the same confirmedNoOpenLinkedIssue state splits in two once the caller can prove a rival merged + // after this PR opened and closed its issue. The contributor did not fail to link anything. + describe("supersession (#10168)", () => { + const supersededPr: PullRequestRecord = { + repoFullName: repo.fullName, + number: 8886, + title: "Fix a bug", + state: "open", + authorLogin: "oktofeesh1", + authorAssociation: "NONE", + headSha: "abc123", + labels: [], + linkedIssues: [8829], + }; + const supersededBy = { issueNumber: 8829, rivalPullNumber: 8881, rivalMergedAt: "2026-07-31T09:30:24Z", issueClosedAt: "2026-07-31T09:30:25Z" }; + + it("reports a supersession naming the rival, not 'No linked issue detected'", () => { + const advisory = buildPullRequestAdvisory(repo, supersededPr, { requireLinkedIssue: true, confirmedNoOpenLinkedIssue: true, supersededBy }); + + expect(advisory.findings.find((f) => f.code === "missing_linked_issue")).toBeUndefined(); + const finding = advisory.findings.find((f) => f.code === "linked_issue_superseded"); + expect(finding?.title).toBe("Superseded by a merged pull request"); + expect(finding?.detail).toContain("#8829 was closed by #8881"); + // The advice must not be the one that cannot work -- re-linking a closed issue changes nothing. + expect(finding?.action).not.toContain("link it explicitly in the PR body"); + expect(finding?.action).toContain("#8881"); + }); + + it("keeps the anti-gaming reading when no rival is proven (absent and explicit-null both)", () => { + for (const value of [undefined, null]) { + const advisory = buildPullRequestAdvisory(repo, supersededPr, { requireLinkedIssue: true, confirmedNoOpenLinkedIssue: true, supersededBy: value }); + expect(advisory.findings.find((f) => f.code === "linked_issue_superseded")).toBeUndefined(); + expect(advisory.findings.find((f) => f.code === "missing_linked_issue")).toBeDefined(); + } + }); + + it("never fires while the linked-issue requirement is off", () => { + const advisory = buildPullRequestAdvisory(repo, supersededPr, { requireLinkedIssue: false, confirmedNoOpenLinkedIssue: true, supersededBy }); + expect(advisory.findings.find((f) => f.code === "linked_issue_superseded")).toBeUndefined(); + }); + }); + it("marks unknown repositories as action required", () => { const advisory = buildRepositoryAdvisory(null, "owner/repo"); expect(advisory.conclusion).toBe("action_required"); diff --git a/worker-configuration.d.ts b/worker-configuration.d.ts index 19015af4a..91540c17c 100644 --- a/worker-configuration.d.ts +++ b/worker-configuration.d.ts @@ -1,5 +1,5 @@ /* eslint-disable */ -// Generated by Wrangler by running `wrangler types` (hash: e0e315bc488417c1ee9bcfcfbe213400) +// Generated by Wrangler by running `wrangler types` (hash: 4e4a925702f0de633d6ecd4a27822346) // Runtime types generated with workerd@1.20260722.1 2026-05-28 nodejs_compat interface __BaseEnv_Env { REVIEW_AUDIT: R2Bucket; @@ -54,6 +54,7 @@ interface __BaseEnv_Env { LOOPOVER_PUBLIC_STATS_REPOS: "JSONbored/gittensory,JSONbored/loopover,JSONbored/awesome-claude,JSONbored/metagraphed"; PUBLIC_REPO_STATS_ALLOWLIST: "JSONbored/loopover"; LOOPOVER_DUPLICATE_WINNER: "true"; + LOOPOVER_SUPERSEDED_CLOSE: "false"; LOOPOVER_OPEN_PR_FILE_COLLISION: "true"; LOOPOVER_SKIP_AUTOMATION_BOT_PRS: "true"; RATE_LIMITER: DurableObjectNamespace; @@ -116,6 +117,7 @@ declare namespace NodeJS { | "LOOPOVER_REVIEW_SELFTUNE" | "LOOPOVER_REVIEW_SURFACE_VERIFICATION" | "LOOPOVER_SKIP_AUTOMATION_BOT_PRS" + | "LOOPOVER_SUPERSEDED_CLOSE" | "LOOPOVER_SWEEP_WATCHDOG" | "PUBLIC_API_ORIGIN" | "PUBLIC_REPO_STATS_ALLOWLIST" diff --git a/wrangler.jsonc b/wrangler.jsonc index f71d473f1..7cdb6eb03 100644 --- a/wrangler.jsonc +++ b/wrangler.jsonc @@ -251,6 +251,12 @@ // #1695 both closed) — the opposite of "spare one good PR". With it ON, the earliest open PR in a cluster // is judged on its merits and only the true duplicates close. "LOOPOVER_DUPLICATE_WINNER": "true", + // Superseded-PR recognition (#10168): a contributor whose linked issue was closed by a rival that merged + // first is currently told "No linked issue detected — link it explicitly in the PR body", advice that + // cannot work. When ON, that case gets its own finding naming the rival, and closes as superseded. + // DISABLED: recognising supersession CLOSES the PR, so it ships dark until it has been shadow-checked + // against the held backlog on the Orb. + "LOOPOVER_SUPERSEDED_CLOSE": "false", // Open-PR file-path collision (#2653): enrich changedFiles on the reviewed PR and its open siblings from // the pull_request_files cache before building the collision report, so two independently-open PRs on the // same file get flagged the way two title-similar PRs already are. ENABLED: a repo-wide shadow test against From 67ae721e0bb4216b49724e71ddd94fb2ce3dee2e Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 31 Jul 2026 05:26:33 -0700 Subject: [PATCH 4/4] test(review): cover the supersession wiring end to end Drives the split through reReviewStoredPullRequest rather than against buildPullRequestAdvisory directly, so the whole seam is proven: the linked-issue verification pass carrying closure facts out, the bounded merged-rival read, the flag, and the finding the contributor actually sees. Reproduces the Orb's own collision -- PR 8886 citing issue 8829, rival 8881 merging behind it, the issue closing one second later -- and pins that the feature is byte-identical with the flag off. The candidate-window derivation moves into the pure module beside the resolver it serves, so 'can this even be superseded' is one tested unit instead of branches stranded in processors.ts that only an integration test could reach. Refs #10168 --- src/queue/processors.ts | 16 +-- src/review/linked-issue-superseded.ts | 24 ++++ test/unit/db-persistence.test.ts | 60 +++++++++ .../unit/linked-issue-superseded-wire.test.ts | 116 ++++++++++++++++++ test/unit/linked-issue-superseded.test.ts | 28 +++++ test/unit/superseded-close-mode.test.ts | 24 ++++ 6 files changed, 257 insertions(+), 11 deletions(-) create mode 100644 test/unit/linked-issue-superseded-wire.test.ts create mode 100644 test/unit/superseded-close-mode.test.ts diff --git a/src/queue/processors.ts b/src/queue/processors.ts index c2c1ee4b4..9e64f1ba4 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -351,7 +351,7 @@ import { import { isDuplicateClusterWinnerByClaim } from "../signals/duplicate-winner"; import { isDuplicateWinnerEnabledGlobally, resolveDuplicateWinnerEnabled } from "../settings/duplicate-winner-mode"; import { isSupersededCloseEnabledGlobally } from "../settings/superseded-close-mode"; -import { SUPERSEDED_CLOSE_WINDOW_MS, resolveSupersession, type LinkedIssueClosure, type SupersededByRival } from "../review/linked-issue-superseded"; +import { resolveSupersession, supersededSearchWindow, type LinkedIssueClosure, type SupersededByRival } from "../review/linked-issue-superseded"; import { isOpenPrFileCollisionEnabledGlobally, resolveOpenPrFileCollisionEnabled } from "../settings/open-pr-file-collision-mode"; import { buildAiReviewDiff, buildSecretScanDiff, totalAddedLineCount } from "../review/review-diff"; // #4013 step 4 (prep): buildAiReviewDiff/buildSecretScanDiff moved to review-diff.ts (a natural existing @@ -8355,16 +8355,10 @@ async function resolveSupersededRival( pr: Pick, closures: LinkedIssueClosure[], ): Promise { - const createdAt = pr.createdAt; - if (!createdAt) return null; - const closedInstants = closures.flatMap((closure) => { - const parsed = closure.closedAt === null ? Number.NaN : Date.parse(closure.closedAt); - return Number.isFinite(parsed) ? [parsed] : []; - }); - if (closedInstants.length === 0) return null; - const until = new Date(Math.max(...closedInstants) + SUPERSEDED_CLOSE_WINDOW_MS).toISOString(); - const mergedRivals = await listMergedPullRequestsInWindow(env, repoFullName, createdAt, until); - return resolveSupersession({ prNumber: pr.number, prCreatedAt: createdAt, closures, mergedRivals }); + const window = supersededSearchWindow(pr.createdAt, closures); + if (window === null) return null; + const mergedRivals = await listMergedPullRequestsInWindow(env, repoFullName, window.sinceIso, window.untilIso); + return resolveSupersession({ prNumber: pr.number, prCreatedAt: pr.createdAt, closures, mergedRivals }); } export async function shouldRefreshFilesForPreMergeChecks( diff --git a/src/review/linked-issue-superseded.ts b/src/review/linked-issue-superseded.ts index 8ef15c06e..6e2e1093f 100644 --- a/src/review/linked-issue-superseded.ts +++ b/src/review/linked-issue-superseded.ts @@ -68,6 +68,30 @@ export type SupersededByRival = { */ export const SUPERSEDED_CLOSE_WINDOW_MS = 5 * 60_000; +/** + * PURE. The narrowest range of merge times that can still contain a qualifying rival, or null when the + * evidence for a supersession cannot exist at all. + * + * Keeping this beside {@link resolveSupersession} rather than at the call site means the whole "can this even + * be superseded" judgement is one tested unit, and the caller reduces to a bounded read plus the resolver. + * The range runs from this PR's own creation (a merge that predates it cannot have taken work not yet + * proposed) to the latest observed close plus the tolerance, so the read stays small no matter how long the + * pull request has been sitting. + */ +export function supersededSearchWindow( + prCreatedAt: string | null | undefined, + closures: readonly LinkedIssueClosure[], +): { sinceIso: string; untilIso: string } | null { + const created = parseInstant(prCreatedAt); + if (created === null) return null; + const closedInstants = closures.flatMap((closure) => { + const parsed = parseInstant(closure.closedAt); + return parsed === null ? [] : [parsed.ms]; + }); + if (closedInstants.length === 0) return null; + return { sinceIso: created.iso, untilIso: new Date(Math.max(...closedInstants) + SUPERSEDED_CLOSE_WINDOW_MS).toISOString() }; +} + /** PURE. Parse a GitHub timestamp, keeping the original string beside the epoch ms so a caller that has * already proved a timestamp parses never needs a second, unreachable null-check to use its text. Null when * the value is absent or unparseable. */ diff --git a/test/unit/db-persistence.test.ts b/test/unit/db-persistence.test.ts index c6897a27f..49efc6cdb 100644 --- a/test/unit/db-persistence.test.ts +++ b/test/unit/db-persistence.test.ts @@ -20,6 +20,7 @@ import { persistSignalSnapshot, startActiveReviewTracking, loadOrphanRequeueContext, + listMergedPullRequestsInWindow, upsertPullRequestFromGitHub, upsertRepositoryFromGitHub, terminalizeActiveReviewsFromBeforeBoot, @@ -725,3 +726,62 @@ describe("terminalizeActiveReviewsFromBeforeBoot (#deploy-orphaned-reviews)", () expect(await terminalizeActiveReviewsFromBeforeBoot(env, boot)).toEqual([]); }); }); + +// #10168: the candidate-rival read behind the supersession check. Deliberately reads `pull_requests` and not +// `recent_merged_pull_requests` -- that table stopped being written, so a rival that merged minutes ago is +// absent from it entirely and the check would silently never fire. +describe("listMergedPullRequestsInWindow (#10168)", () => { + const seedPr = async ( + env: ReturnType, + pr: { number: number; mergedAt?: string | null; linkedIssues: number[]; state?: string }, + ) => { + await upsertPullRequestFromGitHub(env, "owner/repo", { + number: pr.number, + title: `#${pr.number}`, + state: pr.state ?? "closed", + user: { login: "c" }, + head: { sha: `h${pr.number}` }, + labels: [], + created_at: "2026-07-31T09:00:00Z", + merged_at: pr.mergedAt ?? null, + body: pr.linkedIssues.map((n) => `Closes #${n}`).join(" "), + } as never); + }; + + const seedAll = async (env: ReturnType) => { + await upsertRepositoryFromGitHub(env, { full_name: "owner/repo", name: "repo", id: 1, private: false } as never, 4242); + await seedPr(env, { number: 8881, mergedAt: "2026-07-31T09:30:24Z", linkedIssues: [8829] }); + await seedPr(env, { number: 8870, mergedAt: "2026-07-31T08:00:00Z", linkedIssues: [8829] }); // before the window + await seedPr(env, { number: 8899, mergedAt: "2026-07-31T11:00:00Z", linkedIssues: [8829] }); // after the window + await seedPr(env, { number: 8886, mergedAt: null, linkedIssues: [8829], state: "open" }); // never merged + }; + + it("returns only the merges inside the window, with their linked issues", async () => { + const env = createTestEnv(); + await seedAll(env); + const rows = await listMergedPullRequestsInWindow(env, "owner/repo", "2026-07-31T09:22:36Z", "2026-07-31T09:35:25Z"); + expect(rows).toEqual([{ number: 8881, mergedAt: "2026-07-31T09:30:24Z", linkedIssues: [8829] }]); + }); + + it("excludes an unmerged PR even when it cites the same issue", async () => { + const env = createTestEnv(); + await seedAll(env); + const rows = await listMergedPullRequestsInWindow(env, "owner/repo", "2026-07-31T00:00:00Z", "2026-07-31T23:59:59Z"); + expect(rows.map((row) => row.number)).not.toContain(8886); + }); + + it("orders by ascending merge time, so a capped result keeps the EARLIEST rivals", async () => { + const env = createTestEnv(); + await seedAll(env); + const rows = await listMergedPullRequestsInWindow(env, "owner/repo", "2026-07-31T00:00:00Z", "2026-07-31T23:59:59Z"); + expect(rows.map((row) => row.mergedAt)).toEqual([...rows.map((row) => row.mergedAt)].sort()); + expect(rows.map((row) => row.number)).toEqual([8870, 8881, 8899]); + }); + + it("is scoped to the repo and yields nothing when no merge lands in the window", async () => { + const env = createTestEnv(); + await seedAll(env); + expect(await listMergedPullRequestsInWindow(env, "other/repo", "2026-07-31T00:00:00Z", "2026-07-31T23:59:59Z")).toEqual([]); + expect(await listMergedPullRequestsInWindow(env, "owner/repo", "2026-07-30T00:00:00Z", "2026-07-30T23:59:59Z")).toEqual([]); + }); +}); diff --git a/test/unit/linked-issue-superseded-wire.test.ts b/test/unit/linked-issue-superseded-wire.test.ts new file mode 100644 index 000000000..91c513222 --- /dev/null +++ b/test/unit/linked-issue-superseded-wire.test.ts @@ -0,0 +1,116 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { + getLatestAdvisoryForPullRequest, + upsertInstallation, + upsertPullRequestFromGitHub, + upsertRepositoryFromGitHub, + upsertRepositorySettings, +} from "../../src/db/repositories"; +import { reReviewStoredPullRequest } from "../../src/queue/processors"; +import { normalizeRegistryPayload } from "../../src/registry/normalize"; +import { persistRegistrySnapshot } from "../../src/registry/sync"; +import { asCloudEnv, createTestEnv } from "../helpers/d1"; +import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; +import { generatePrivateKeyPem } from "../helpers/github-app-key"; + +// #10168 end-to-end: the supersession split, driven through a real gate-evaluating entry point rather than +// against buildPullRequestAdvisory directly, so the whole seam is proven -- the linked-issue verification pass +// carrying closure facts out, the bounded merged-rival read, the flag, and the finding the contributor sees. +// +// The collision reproduced here is the real one from the Orb: +// PR 8886 opened 09:22:36 citing issue 8829 (the issue was OPEN at that moment) +// PR 8881 merged 09:30:24 citing the same issue +// issue 8829 closed 09:30:25, one second later, as a side effect of that merge + +const REPO = "JSONbored/gittensory"; +const PR_CREATED = "2026-07-31T09:22:36Z"; +const RIVAL_MERGED = "2026-07-31T09:30:24Z"; +const ISSUE_CLOSED = "2026-07-31T09:30:25Z"; + +async function seedRepo(env: ReturnType) { + await persistRegistrySnapshot( + asCloudEnv(env), + normalizeRegistryPayload({ [REPO]: { emission_share: 0.01, issue_discovery_share: 0 } }, { kind: "raw-github", url: "https://example.test" }, "2026-05-23T00:00:00.000Z"), + ); + await upsertInstallation(env, { + action: "created", + installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "selected", permissions: {}, events: [] }, + }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: REPO, private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { + repoFullName: REPO, + autoLabelEnabled: false, + gatePack: "oss-anti-slop", + // Only `label` acts, so maintenance never attempts a live merge/approve. + autonomy: { label: "auto" }, + }); + // linkedIssueGateMode is CONFIG-AS-CODE only (loopover#6442) -- upsertRepositorySettings silently drops it, + // so it has to arrive through the manifest's `gate:` block. "block" is the only mode in which the + // open-reference check runs at all, and that is the pass the closure facts ride out of. + await upsertRepoFocusManifest(env, REPO, { + gate: { linkedIssue: "block" }, + settings: { commentMode: "all_prs", publicSurface: "comment_only", checkRunMode: "off", reviewCheckMode: "required", aiReviewMode: "off" }, + }); + // The merged rival, and the PR it superseded. + await upsertPullRequestFromGitHub(env, REPO, { + number: 8881, title: "rival", state: "closed", user: { login: "rival" }, head: { sha: "shaRival" }, + labels: [], body: "Closes #8829", created_at: "2026-07-31T09:04:07Z", merged_at: RIVAL_MERGED, + } as never); + await upsertPullRequestFromGitHub(env, REPO, { + number: 8886, title: "Fix the thing", state: "open", user: { login: "contributor" }, author_association: "CONTRIBUTOR", + head: { sha: "sha8886" }, base: { ref: "main" }, labels: [], body: "Closes #8829", created_at: PR_CREATED, + } as never); +} + +function stubGitHub() { + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" }); + if (url.includes("/pulls/8886/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.endsWith("/pulls/8886")) { + return Response.json({ number: 8886, title: "Fix the thing", state: "open", user: { login: "contributor" }, head: { sha: "sha8886" }, labels: [], body: "Closes #8829", created_at: PR_CREATED, mergeable_state: "dirty" }); + } + if (url.includes("/commits/sha8886/check-runs")) return Response.json({ total_count: 1, check_runs: [{ name: "test", status: "completed", conclusion: "success", app: { slug: "github-actions" } }] }); + if (url.includes("/commits/sha8886/status")) return Response.json({ state: "success", statuses: [] }); + // The issue the contributor correctly linked -- closed, by the rival's merge, one second after it landed. + if (url.includes("/issues/8829")) return Response.json({ number: 8829, title: "The issue", state: "closed", closed_at: ISSUE_CLOSED, labels: [], assignees: [], user: { login: "reporter" } }); + if (url.includes("/issues/8886/comments") && (method === "POST" || method === "PATCH")) return Response.json({ id: 1 }, { status: 201 }); + if (url.includes("/issues/8886/comments")) return Response.json([]); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); +} + +async function runReview(supersededCloseFlag: string | undefined) { + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + ...(supersededCloseFlag === undefined ? {} : { LOOPOVER_SUPERSEDED_CLOSE: supersededCloseFlag }), + }); + await seedRepo(env); + stubGitHub(); + await reReviewStoredPullRequest(env, "superseded-wire", 123, REPO, 8886); + const advisory = await getLatestAdvisoryForPullRequest(env, REPO, 8886); + return (advisory?.findings ?? []).map((finding) => finding.code); +} + +describe("superseded linked issue, wired end to end (#10168)", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("reports the supersession instead of 'No linked issue detected' once the flag is on", async () => { + const codes = await runReview("true"); + expect(codes).toContain("linked_issue_superseded"); + expect(codes).not.toContain("missing_linked_issue"); + }); + + it("is byte-identical to today's behaviour while the flag is off", async () => { + // The whole feature ships dark: same finding, same message, same disposition as before it existed. + for (const flag of [undefined, "false"]) { + const codes = await runReview(flag); + expect(codes, String(flag)).toContain("missing_linked_issue"); + expect(codes, String(flag)).not.toContain("linked_issue_superseded"); + } + }); +}); diff --git a/test/unit/linked-issue-superseded.test.ts b/test/unit/linked-issue-superseded.test.ts index f1349b1f6..d6f461d9c 100644 --- a/test/unit/linked-issue-superseded.test.ts +++ b/test/unit/linked-issue-superseded.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { SUPERSEDED_CLOSE_WINDOW_MS, resolveSupersession, + supersededSearchWindow, type LinkedIssueClosure, type MergedRivalPullRequest, } from "../../src/review/linked-issue-superseded"; @@ -159,3 +160,30 @@ describe("resolveSupersession", () => { }); }); }); + +describe("supersededSearchWindow", () => { + it("spans this PR's creation to the latest close plus the tolerance", () => { + expect(supersededSearchWindow(PR_CREATED, [closure()])).toEqual({ + sinceIso: PR_CREATED, + untilIso: new Date(Date.parse(ISSUE_CLOSED) + SUPERSEDED_CLOSE_WINDOW_MS).toISOString(), + }); + }); + + it("anchors the end on the LATEST close when several issues are linked", () => { + const later = "2026-07-31T10:00:00Z"; + const window = supersededSearchWindow(PR_CREATED, [closure(), closure({ issueNumber: 9000, closedAt: later })]); + expect(window?.untilIso).toBe(new Date(Date.parse(later) + SUPERSEDED_CLOSE_WINDOW_MS).toISOString()); + }); + + it("declines without a synced createdAt — the 'issue outlived this PR' half cannot be established", () => { + for (const value of [null, undefined, "", "whenever"]) { + expect(supersededSearchWindow(value, [closure()]), String(value)).toBeNull(); + } + }); + + it("declines when no linked issue was conclusively read as closed", () => { + expect(supersededSearchWindow(PR_CREATED, [])).toBeNull(); + expect(supersededSearchWindow(PR_CREATED, [closure({ closedAt: null })])).toBeNull(); + expect(supersededSearchWindow(PR_CREATED, [closure({ closedAt: "not-a-date" })])).toBeNull(); + }); +}); diff --git a/test/unit/superseded-close-mode.test.ts b/test/unit/superseded-close-mode.test.ts new file mode 100644 index 000000000..04414dc22 --- /dev/null +++ b/test/unit/superseded-close-mode.test.ts @@ -0,0 +1,24 @@ +import { describe, expect, it } from "vitest"; +import { isSupersededCloseEnabledGlobally } from "../../src/settings/superseded-close-mode"; + +describe("isSupersededCloseEnabledGlobally (#10168)", () => { + it("defaults OFF when unset — recognising supersession CLOSES a PR, so it must be opted into", () => { + expect(isSupersededCloseEnabledGlobally({})).toBe(false); + expect(isSupersededCloseEnabledGlobally({ LOOPOVER_SUPERSEDED_CLOSE: undefined })).toBe(false); + expect(isSupersededCloseEnabledGlobally({ LOOPOVER_SUPERSEDED_CLOSE: "" })).toBe(false); + }); + + it("is ON for every value the codebase truthy convention accepts", () => { + // Same trimmed, case-insensitive `/^(1|true|yes|on)$/i` as the sibling flags -- #10054 caught a flag that + // was `=== "true"` only and silently read `1` / `on` / a whitespace-padded `.env` value as OFF. + for (const value of ["1", "true", "TRUE", "yes", "on", " true "]) { + expect(isSupersededCloseEnabledGlobally({ LOOPOVER_SUPERSEDED_CLOSE: value }), value).toBe(true); + } + }); + + it("stays OFF for a falsy or unrecognised value", () => { + for (const value of ["0", "false", "off", "no", "maybe"]) { + expect(isSupersededCloseEnabledGlobally({ LOOPOVER_SUPERSEDED_CLOSE: value }), value).toBe(false); + } + }); +});