diff --git a/packages/loopover-engine/src/advisory/gate-advisory.ts b/packages/loopover-engine/src/advisory/gate-advisory.ts index cb76e758c5..85e46bcd91 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 0000000000..aad125b93a --- /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/db/repositories.ts b/src/db/repositories.ts index 1232119e57..ac0bb7c79b 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. diff --git a/src/env.d.ts b/src/env.d.ts index a5bee78c28..794d2a3a3d 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 cd4c56af5c..9e64f1ba42 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 { 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 @@ -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,45 @@ 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 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( @@ -15171,12 +15211,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 +15231,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 cd333c1f22..15d57323da 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/review/linked-issue-superseded.ts b/src/review/linked-issue-superseded.ts new file mode 100644 index 0000000000..6e2e1093fa --- /dev/null +++ b/src/review/linked-issue-superseded.ts @@ -0,0 +1,156 @@ +// 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. 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. */ +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/src/rules/advisory.ts b/src/rules/advisory.ts index 9a165e5746..618fa6c500 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 ca9fad11d4..0219e20009 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 0000000000..2e5e0b305c --- /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 0f3d86f459..91f288bdeb 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/db-persistence.test.ts b/test/unit/db-persistence.test.ts index c6897a27f5..49efc6cdb9 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-hard-rules.test.ts b/test/unit/linked-issue-hard-rules.test.ts index 695fcd42a4..83533aab55 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/linked-issue-superseded-wire.test.ts b/test/unit/linked-issue-superseded-wire.test.ts new file mode 100644 index 0000000000..91c5132224 --- /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 new file mode 100644 index 0000000000..d6f461d9cc --- /dev/null +++ b/test/unit/linked-issue-superseded.test.ts @@ -0,0 +1,189 @@ +import { describe, expect, it } from "vitest"; +import { + SUPERSEDED_CLOSE_WINDOW_MS, + resolveSupersession, + supersededSearchWindow, + 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); + }); + }); +}); + +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/rules.test.ts b/test/unit/rules.test.ts index d6880e2d7e..130c64de52 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/test/unit/superseded-close-mode.test.ts b/test/unit/superseded-close-mode.test.ts new file mode 100644 index 0000000000..04414dc226 --- /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); + } + }); +}); diff --git a/worker-configuration.d.ts b/worker-configuration.d.ts index 19015af4aa..91540c17cf 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 f71d473f19..7cdb6eb033 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