Skip to content

Commit cc886b9

Browse files
authored
feat(review): report a superseded pull request as superseded, not unlinked (#10205)
* feat(review): detect a pull request superseded by a merged rival 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 * 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 * 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 * 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
1 parent c7645de commit cc886b9

19 files changed

Lines changed: 930 additions & 46 deletions

packages/loopover-engine/src/advisory/gate-advisory.ts

Lines changed: 32 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -195,6 +195,12 @@ export function buildPullRequestAdvisory(
195195
* — this is fail-open by construction: the caller only ever sets it true after a live check confirms
196196
* every reference is dead, never on ambiguity. */
197197
confirmedNoOpenLinkedIssue?: boolean;
198+
/** #10168: evidence that this PR's linked issue was closed by a rival that merged AFTER it opened.
199+
* Present ⇒ the `confirmedNoOpenLinkedIssue` case reports as a supersession naming the rival instead of
200+
* as `missing_linked_issue`'s unactionable "link it explicitly in the PR body". Structurally typed here
201+
* rather than imported from the host's review/linked-issue-superseded.ts, for the same reason the rest of
202+
* this file is a slimmed twin: @loopover/engine must not drag the host's subsystem into its graph. */
203+
supersededBy?: { issueNumber: number; rivalPullNumber: number } | null | undefined;
198204
} = {},
199205
): Advisory {
200206
const repoFullName = pr?.repoFullName ?? repo?.fullName ?? "unknown/unknown";
@@ -223,7 +229,7 @@ export function buildPullRequestAdvisory(
223229
action: "Re-deliver the webhook or wait for the next sync.",
224230
});
225231
} else {
226-
addPullRequestFindings(repo, pr, findings, context.otherOpenPullRequests ?? [], Boolean(context.requireLinkedIssue), Boolean(context.duplicateWinnerEnabled), context.linkedIssueAuthorLogins ?? [], Boolean(context.confirmedNoOpenLinkedIssue));
232+
addPullRequestFindings(repo, pr, findings, context.otherOpenPullRequests ?? [], Boolean(context.requireLinkedIssue), Boolean(context.duplicateWinnerEnabled), context.linkedIssueAuthorLogins ?? [], Boolean(context.confirmedNoOpenLinkedIssue), context.supersededBy);
227233
}
228234
return advisory("pull_request", targetKey, repoFullName, findings, "Pull request advisory generated.", pr?.number, undefined, pr?.headSha ?? undefined);
229235
}
@@ -298,6 +304,8 @@ function addPullRequestFindings(
298304
duplicateWinnerEnabled: boolean,
299305
linkedIssueAuthorLogins: (string | null | undefined)[],
300306
confirmedNoOpenLinkedIssue: boolean,
307+
// #10168: present only when the caller proved a rival merged after this PR opened and closed its issue.
308+
supersededBy?: { issueNumber: number; rivalPullNumber: number } | null | undefined,
301309
): void {
302310
if (pr.state !== "open") {
303311
findings.push({
@@ -314,15 +322,29 @@ function addPullRequestFindings(
314322
// which always hand in a freshly-read body and never hit the webhook race) stays byte-identical.
315323
const noLinkedIssueCited = pr.linkedIssues.length === 0 && pr.bodyObservedAt !== null;
316324
if ((noLinkedIssueCited || confirmedNoOpenLinkedIssue) && requireLinkedIssue) {
317-
findings.push({
318-
code: "missing_linked_issue",
319-
severity: "warning",
320-
title: "No linked issue detected",
321-
detail: noLinkedIssueCited
322-
? "No closing reference or linked issue number was found in the PR metadata/body."
323-
: "The PR cites an issue number, but it could not be verified as a currently open issue.",
324-
action: "If this PR is intended to solve an issue, link it explicitly in the PR body.",
325-
});
325+
// #10168 (host-parity): a PR whose linked issue a merged rival closed did NOT fail to link an issue -- it
326+
// linked one correctly and lost a race, and telling it to "link it explicitly" is advice that cannot
327+
// work. See the host copy (src/rules/advisory.ts) for the full rationale and the two facts the caller
328+
// must prove before setting this.
329+
if (supersededBy) {
330+
findings.push({
331+
code: "linked_issue_superseded",
332+
severity: "warning",
333+
title: "Superseded by a merged pull request",
334+
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.`,
335+
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.`,
336+
});
337+
} else {
338+
findings.push({
339+
code: "missing_linked_issue",
340+
severity: "warning",
341+
title: "No linked issue detected",
342+
detail: noLinkedIssueCited
343+
? "No closing reference or linked issue number was found in the PR metadata/body."
344+
: "The PR cites an issue number, but it could not be verified as a currently open issue.",
345+
action: "If this PR is intended to solve an issue, link it explicitly in the PR body.",
346+
});
347+
}
326348
} else {
327349
const overlappingPrs = otherOpenPullRequests.filter((otherPr) =>
328350
otherPr.linkedIssues.some((issueNumber) => pr.linkedIssues.includes(issueNumber)),
Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
import { test } from "node:test";
2+
import assert from "node:assert/strict";
3+
4+
// #10168, engine-twin half. The host copy (src/rules/advisory.ts) has the same split and its own suite; both
5+
// are covered because gate-advisory is a deliberately-divergent twin, so a fix applied to only one side is
6+
// exactly the drift this pair of suites exists to catch.
7+
//
8+
// The case: a contributor links a genuinely open issue, a rival PR citing the same issue merges first, and
9+
// the issue closes behind it. The PR then reads as "no open linked issue" -- and telling that contributor to
10+
// "link it explicitly in the PR body" is advice that cannot work.
11+
12+
const repo = { fullName: "o/r", defaultBranch: "main" } as never;
13+
const supersededPr = {
14+
repoFullName: "o/r",
15+
number: 8886,
16+
title: "fix: a thing",
17+
state: "open",
18+
authorLogin: "someone",
19+
authorAssociation: "CONTRIBUTOR",
20+
labels: [],
21+
linkedIssues: [8829],
22+
bodyObservedAt: "2026-07-31T09:22:36Z",
23+
};
24+
const supersededBy = { issueNumber: 8829, rivalPullNumber: 8881 };
25+
26+
test("a superseded PR is reported as superseded, naming the rival that merged", async () => {
27+
const { buildPullRequestAdvisory } = await import("../dist/advisory/gate-advisory.js");
28+
const advisory = buildPullRequestAdvisory(repo, supersededPr as never, {
29+
requireLinkedIssue: true,
30+
confirmedNoOpenLinkedIssue: true,
31+
supersededBy,
32+
});
33+
34+
const finding = advisory.findings.find((f) => f.code === "linked_issue_superseded");
35+
assert.ok(finding, "the supersession finding is raised");
36+
assert.equal(finding.title, "Superseded by a merged pull request");
37+
assert.match(finding.detail, /#8829 was closed by #8881/);
38+
assert.ok(
39+
!advisory.findings.some((f) => f.code === "missing_linked_issue"),
40+
"the unactionable missing_linked_issue reading is replaced, not doubled up",
41+
);
42+
assert.ok(!/link it explicitly in the PR body/.test(finding.action ?? ""), "the advice that cannot work is gone");
43+
assert.match(finding.action ?? "", /#8881/, "the remedy points at the rival that actually landed");
44+
});
45+
46+
test("without proven supersession the anti-gaming reading is unchanged", async () => {
47+
const { buildPullRequestAdvisory } = await import("../dist/advisory/gate-advisory.js");
48+
for (const value of [undefined, null]) {
49+
const advisory = buildPullRequestAdvisory(repo, supersededPr as never, {
50+
requireLinkedIssue: true,
51+
confirmedNoOpenLinkedIssue: true,
52+
supersededBy: value,
53+
});
54+
assert.ok(
55+
advisory.findings.some((f) => f.code === "missing_linked_issue"),
56+
`missing_linked_issue still fires for supersededBy=${String(value)}`,
57+
);
58+
assert.ok(
59+
!advisory.findings.some((f) => f.code === "linked_issue_superseded"),
60+
`no supersession is claimed for supersededBy=${String(value)}`,
61+
);
62+
}
63+
});
64+
65+
test("supersession never fires while the linked-issue requirement is off", async () => {
66+
const { buildPullRequestAdvisory } = await import("../dist/advisory/gate-advisory.js");
67+
const advisory = buildPullRequestAdvisory(repo, supersededPr as never, {
68+
requireLinkedIssue: false,
69+
confirmedNoOpenLinkedIssue: true,
70+
supersededBy,
71+
});
72+
assert.ok(!advisory.findings.some((f) => f.code === "linked_issue_superseded"));
73+
});

src/db/repositories.ts

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
// (dist/index.js re-exporting calibration/advisory/policy modules) measured ~420ms of cold import
33
// under vitest — a tax paid by every test file that transitively touches repositories (#test-import-cost).
44
import { parsePullRequestTargetKey } from "@loopover/engine/parse-pull-request-target-key";
5-
import { and, asc, desc, eq, gte, inArray, isNotNull, lt, not, or, sql, type SQL } from "drizzle-orm";
5+
import { and, asc, desc, eq, gte, inArray, isNotNull, lt, lte, not, or, sql, type SQL } from "drizzle-orm";
66
import { getDb } from "./client";
77
import {
88
activeReviewTracking,
@@ -5116,6 +5116,42 @@ export async function listOtherOpenPullRequests(env: Env, fullName: string, numb
51165116
return rows.map(toPullRequestRecordFromRow);
51175117
}
51185118

5119+
/**
5120+
* Merged pull requests in this repo whose merge landed inside `[sinceIso, untilIso]` (#10168) — the candidate
5121+
* rivals for a supersession check. Only the three fields that decision needs are selected.
5122+
*
5123+
* Reads `pull_requests`, deliberately NOT the purpose-shaped `recent_merged_pull_requests` table: that one is
5124+
* no longer written (its newest row on the Orb predates this by weeks), so a rival that merged minutes ago is
5125+
* simply absent from it, and a supersession check reading it would silently never fire.
5126+
*
5127+
* `merged_at` holds GitHub's Z-normalised ISO-8601, so the window comparison is a lexicographic string range
5128+
* over the stored text — the same shape every other timestamp filter in this file uses.
5129+
*/
5130+
export async function listMergedPullRequestsInWindow(
5131+
env: Env,
5132+
fullName: string,
5133+
sinceIso: string,
5134+
untilIso: string,
5135+
): Promise<{ number: number; mergedAt: string | null; linkedIssues: number[] }[]> {
5136+
const db = getDb(env.DB);
5137+
const rows = await db
5138+
.select({ number: pullRequests.number, mergedAt: pullRequests.mergedAt, linkedIssuesJson: pullRequests.linkedIssuesJson })
5139+
.from(pullRequests)
5140+
.where(
5141+
and(
5142+
eq(pullRequests.repoFullName, fullName),
5143+
isNotNull(pullRequests.mergedAt),
5144+
gte(pullRequests.mergedAt, sinceIso),
5145+
lte(pullRequests.mergedAt, untilIso),
5146+
),
5147+
)
5148+
// Ascending merge time, so the cap keeps the EARLIEST merges in the window — the supersession resolver
5149+
// elects the earliest qualifying rival, so dropping those would mis-name the cause.
5150+
.orderBy(asc(pullRequests.mergedAt))
5151+
.limit(100);
5152+
return rows.map((row) => ({ number: row.number, mergedAt: row.mergedAt, linkedIssues: parseJson<number[]>(row.linkedIssuesJson, []) }));
5153+
}
5154+
51195155
// #9125: `authorGithubId` is optional and ADDITIVE -- when the caller has it, a sibling PR matches on the
51205156
// immutable id OR the (renameable) login, so a contributor who renamed between two PRs still gets counted
51215157
// against their own cap. Omit it and this behaves exactly as the login-only match always did.

src/env.d.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -735,6 +735,12 @@ declare global {
735735
* unchanged). Once a winner closes, the next-lowest OPEN sibling becomes the winner on re-eval. See
736736
* src/signals/duplicate-winner.ts. */
737737
LOOPOVER_DUPLICATE_WINNER?: string;
738+
/** Superseded-PR recognition (#10168): when truthy, a PR whose linked issue was closed by a rival that
739+
* merged AFTER this PR opened is reported as superseded — its own finding, its own message naming the
740+
* rival — instead of the unactionable "No linked issue detected", and closes as superseded rather than
741+
* holding forever. OFF by default: it changes the close disposition. See
742+
* src/review/linked-issue-superseded.ts. */
743+
LOOPOVER_SUPERSEDED_CLOSE?: string;
738744
/** Open-PR file-path collision (#2653): when truthy, a live PR review enriches its own and its open
739745
* siblings' `changedFiles` from the `pull_request_files` cache (a plain D1 read — no extra GitHub calls)
740746
* before building the collision report, so two independently-open PRs touching the same file are flagged

0 commit comments

Comments
 (0)