Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 32 additions & 10 deletions packages/loopover-engine/src/advisory/gate-advisory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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({
Expand All @@ -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)),
Expand Down
Original file line number Diff line number Diff line change
@@ -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"));
});
38 changes: 37 additions & 1 deletion src/db/repositories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<number[]>(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.
Expand Down
6 changes: 6 additions & 0 deletions src/env.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading
Loading