Skip to content

Commit 67ae721

Browse files
committed
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 35e2692 commit 67ae721

6 files changed

Lines changed: 257 additions & 11 deletions

src/queue/processors.ts

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -351,7 +351,7 @@ import {
351351
import { isDuplicateClusterWinnerByClaim } from "../signals/duplicate-winner";
352352
import { isDuplicateWinnerEnabledGlobally, resolveDuplicateWinnerEnabled } from "../settings/duplicate-winner-mode";
353353
import { isSupersededCloseEnabledGlobally } from "../settings/superseded-close-mode";
354-
import { SUPERSEDED_CLOSE_WINDOW_MS, resolveSupersession, type LinkedIssueClosure, type SupersededByRival } from "../review/linked-issue-superseded";
354+
import { resolveSupersession, supersededSearchWindow, type LinkedIssueClosure, type SupersededByRival } from "../review/linked-issue-superseded";
355355
import { isOpenPrFileCollisionEnabledGlobally, resolveOpenPrFileCollisionEnabled } from "../settings/open-pr-file-collision-mode";
356356
import { buildAiReviewDiff, buildSecretScanDiff, totalAddedLineCount } from "../review/review-diff";
357357
// #4013 step 4 (prep): buildAiReviewDiff/buildSecretScanDiff moved to review-diff.ts (a natural existing
@@ -8355,16 +8355,10 @@ async function resolveSupersededRival(
83558355
pr: Pick<PullRequestRecord, "number" | "createdAt">,
83568356
closures: LinkedIssueClosure[],
83578357
): Promise<SupersededByRival | null> {
8358-
const createdAt = pr.createdAt;
8359-
if (!createdAt) return null;
8360-
const closedInstants = closures.flatMap((closure) => {
8361-
const parsed = closure.closedAt === null ? Number.NaN : Date.parse(closure.closedAt);
8362-
return Number.isFinite(parsed) ? [parsed] : [];
8363-
});
8364-
if (closedInstants.length === 0) return null;
8365-
const until = new Date(Math.max(...closedInstants) + SUPERSEDED_CLOSE_WINDOW_MS).toISOString();
8366-
const mergedRivals = await listMergedPullRequestsInWindow(env, repoFullName, createdAt, until);
8367-
return resolveSupersession({ prNumber: pr.number, prCreatedAt: createdAt, closures, mergedRivals });
8358+
const window = supersededSearchWindow(pr.createdAt, closures);
8359+
if (window === null) return null;
8360+
const mergedRivals = await listMergedPullRequestsInWindow(env, repoFullName, window.sinceIso, window.untilIso);
8361+
return resolveSupersession({ prNumber: pr.number, prCreatedAt: pr.createdAt, closures, mergedRivals });
83688362
}
83698363

83708364
export async function shouldRefreshFilesForPreMergeChecks(

src/review/linked-issue-superseded.ts

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,30 @@ export type SupersededByRival = {
6868
*/
6969
export const SUPERSEDED_CLOSE_WINDOW_MS = 5 * 60_000;
7070

71+
/**
72+
* PURE. The narrowest range of merge times that can still contain a qualifying rival, or null when the
73+
* evidence for a supersession cannot exist at all.
74+
*
75+
* Keeping this beside {@link resolveSupersession} rather than at the call site means the whole "can this even
76+
* be superseded" judgement is one tested unit, and the caller reduces to a bounded read plus the resolver.
77+
* The range runs from this PR's own creation (a merge that predates it cannot have taken work not yet
78+
* proposed) to the latest observed close plus the tolerance, so the read stays small no matter how long the
79+
* pull request has been sitting.
80+
*/
81+
export function supersededSearchWindow(
82+
prCreatedAt: string | null | undefined,
83+
closures: readonly LinkedIssueClosure[],
84+
): { sinceIso: string; untilIso: string } | null {
85+
const created = parseInstant(prCreatedAt);
86+
if (created === null) return null;
87+
const closedInstants = closures.flatMap((closure) => {
88+
const parsed = parseInstant(closure.closedAt);
89+
return parsed === null ? [] : [parsed.ms];
90+
});
91+
if (closedInstants.length === 0) return null;
92+
return { sinceIso: created.iso, untilIso: new Date(Math.max(...closedInstants) + SUPERSEDED_CLOSE_WINDOW_MS).toISOString() };
93+
}
94+
7195
/** PURE. Parse a GitHub timestamp, keeping the original string beside the epoch ms so a caller that has
7296
* already proved a timestamp parses never needs a second, unreachable null-check to use its text. Null when
7397
* the value is absent or unparseable. */

test/unit/db-persistence.test.ts

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
persistSignalSnapshot,
2121
startActiveReviewTracking,
2222
loadOrphanRequeueContext,
23+
listMergedPullRequestsInWindow,
2324
upsertPullRequestFromGitHub,
2425
upsertRepositoryFromGitHub,
2526
terminalizeActiveReviewsFromBeforeBoot,
@@ -725,3 +726,62 @@ describe("terminalizeActiveReviewsFromBeforeBoot (#deploy-orphaned-reviews)", ()
725726
expect(await terminalizeActiveReviewsFromBeforeBoot(env, boot)).toEqual([]);
726727
});
727728
});
729+
730+
// #10168: the candidate-rival read behind the supersession check. Deliberately reads `pull_requests` and not
731+
// `recent_merged_pull_requests` -- that table stopped being written, so a rival that merged minutes ago is
732+
// absent from it entirely and the check would silently never fire.
733+
describe("listMergedPullRequestsInWindow (#10168)", () => {
734+
const seedPr = async (
735+
env: ReturnType<typeof createTestEnv>,
736+
pr: { number: number; mergedAt?: string | null; linkedIssues: number[]; state?: string },
737+
) => {
738+
await upsertPullRequestFromGitHub(env, "owner/repo", {
739+
number: pr.number,
740+
title: `#${pr.number}`,
741+
state: pr.state ?? "closed",
742+
user: { login: "c" },
743+
head: { sha: `h${pr.number}` },
744+
labels: [],
745+
created_at: "2026-07-31T09:00:00Z",
746+
merged_at: pr.mergedAt ?? null,
747+
body: pr.linkedIssues.map((n) => `Closes #${n}`).join(" "),
748+
} as never);
749+
};
750+
751+
const seedAll = async (env: ReturnType<typeof createTestEnv>) => {
752+
await upsertRepositoryFromGitHub(env, { full_name: "owner/repo", name: "repo", id: 1, private: false } as never, 4242);
753+
await seedPr(env, { number: 8881, mergedAt: "2026-07-31T09:30:24Z", linkedIssues: [8829] });
754+
await seedPr(env, { number: 8870, mergedAt: "2026-07-31T08:00:00Z", linkedIssues: [8829] }); // before the window
755+
await seedPr(env, { number: 8899, mergedAt: "2026-07-31T11:00:00Z", linkedIssues: [8829] }); // after the window
756+
await seedPr(env, { number: 8886, mergedAt: null, linkedIssues: [8829], state: "open" }); // never merged
757+
};
758+
759+
it("returns only the merges inside the window, with their linked issues", async () => {
760+
const env = createTestEnv();
761+
await seedAll(env);
762+
const rows = await listMergedPullRequestsInWindow(env, "owner/repo", "2026-07-31T09:22:36Z", "2026-07-31T09:35:25Z");
763+
expect(rows).toEqual([{ number: 8881, mergedAt: "2026-07-31T09:30:24Z", linkedIssues: [8829] }]);
764+
});
765+
766+
it("excludes an unmerged PR even when it cites the same issue", async () => {
767+
const env = createTestEnv();
768+
await seedAll(env);
769+
const rows = await listMergedPullRequestsInWindow(env, "owner/repo", "2026-07-31T00:00:00Z", "2026-07-31T23:59:59Z");
770+
expect(rows.map((row) => row.number)).not.toContain(8886);
771+
});
772+
773+
it("orders by ascending merge time, so a capped result keeps the EARLIEST rivals", async () => {
774+
const env = createTestEnv();
775+
await seedAll(env);
776+
const rows = await listMergedPullRequestsInWindow(env, "owner/repo", "2026-07-31T00:00:00Z", "2026-07-31T23:59:59Z");
777+
expect(rows.map((row) => row.mergedAt)).toEqual([...rows.map((row) => row.mergedAt)].sort());
778+
expect(rows.map((row) => row.number)).toEqual([8870, 8881, 8899]);
779+
});
780+
781+
it("is scoped to the repo and yields nothing when no merge lands in the window", async () => {
782+
const env = createTestEnv();
783+
await seedAll(env);
784+
expect(await listMergedPullRequestsInWindow(env, "other/repo", "2026-07-31T00:00:00Z", "2026-07-31T23:59:59Z")).toEqual([]);
785+
expect(await listMergedPullRequestsInWindow(env, "owner/repo", "2026-07-30T00:00:00Z", "2026-07-30T23:59:59Z")).toEqual([]);
786+
});
787+
});
Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
import { afterEach, describe, expect, it, vi } from "vitest";
2+
import {
3+
getLatestAdvisoryForPullRequest,
4+
upsertInstallation,
5+
upsertPullRequestFromGitHub,
6+
upsertRepositoryFromGitHub,
7+
upsertRepositorySettings,
8+
} from "../../src/db/repositories";
9+
import { reReviewStoredPullRequest } from "../../src/queue/processors";
10+
import { normalizeRegistryPayload } from "../../src/registry/normalize";
11+
import { persistRegistrySnapshot } from "../../src/registry/sync";
12+
import { asCloudEnv, createTestEnv } from "../helpers/d1";
13+
import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader";
14+
import { generatePrivateKeyPem } from "../helpers/github-app-key";
15+
16+
// #10168 end-to-end: the supersession split, driven through a real gate-evaluating entry point rather than
17+
// against buildPullRequestAdvisory directly, so the whole seam is proven -- the linked-issue verification pass
18+
// carrying closure facts out, the bounded merged-rival read, the flag, and the finding the contributor sees.
19+
//
20+
// The collision reproduced here is the real one from the Orb:
21+
// PR 8886 opened 09:22:36 citing issue 8829 (the issue was OPEN at that moment)
22+
// PR 8881 merged 09:30:24 citing the same issue
23+
// issue 8829 closed 09:30:25, one second later, as a side effect of that merge
24+
25+
const REPO = "JSONbored/gittensory";
26+
const PR_CREATED = "2026-07-31T09:22:36Z";
27+
const RIVAL_MERGED = "2026-07-31T09:30:24Z";
28+
const ISSUE_CLOSED = "2026-07-31T09:30:25Z";
29+
30+
async function seedRepo(env: ReturnType<typeof createTestEnv>) {
31+
await persistRegistrySnapshot(
32+
asCloudEnv(env),
33+
normalizeRegistryPayload({ [REPO]: { emission_share: 0.01, issue_discovery_share: 0 } }, { kind: "raw-github", url: "https://example.test" }, "2026-05-23T00:00:00.000Z"),
34+
);
35+
await upsertInstallation(env, {
36+
action: "created",
37+
installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "selected", permissions: {}, events: [] },
38+
});
39+
await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: REPO, private: false, owner: { login: "JSONbored" } }, 123);
40+
await upsertRepositorySettings(env, {
41+
repoFullName: REPO,
42+
autoLabelEnabled: false,
43+
gatePack: "oss-anti-slop",
44+
// Only `label` acts, so maintenance never attempts a live merge/approve.
45+
autonomy: { label: "auto" },
46+
});
47+
// linkedIssueGateMode is CONFIG-AS-CODE only (loopover#6442) -- upsertRepositorySettings silently drops it,
48+
// so it has to arrive through the manifest's `gate:` block. "block" is the only mode in which the
49+
// open-reference check runs at all, and that is the pass the closure facts ride out of.
50+
await upsertRepoFocusManifest(env, REPO, {
51+
gate: { linkedIssue: "block" },
52+
settings: { commentMode: "all_prs", publicSurface: "comment_only", checkRunMode: "off", reviewCheckMode: "required", aiReviewMode: "off" },
53+
});
54+
// The merged rival, and the PR it superseded.
55+
await upsertPullRequestFromGitHub(env, REPO, {
56+
number: 8881, title: "rival", state: "closed", user: { login: "rival" }, head: { sha: "shaRival" },
57+
labels: [], body: "Closes #8829", created_at: "2026-07-31T09:04:07Z", merged_at: RIVAL_MERGED,
58+
} as never);
59+
await upsertPullRequestFromGitHub(env, REPO, {
60+
number: 8886, title: "Fix the thing", state: "open", user: { login: "contributor" }, author_association: "CONTRIBUTOR",
61+
head: { sha: "sha8886" }, base: { ref: "main" }, labels: [], body: "Closes #8829", created_at: PR_CREATED,
62+
} as never);
63+
}
64+
65+
function stubGitHub() {
66+
vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => {
67+
const url = input.toString();
68+
const method = init?.method ?? "GET";
69+
if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" });
70+
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;" }]);
71+
if (url.endsWith("/pulls/8886")) {
72+
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" });
73+
}
74+
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" } }] });
75+
if (url.includes("/commits/sha8886/status")) return Response.json({ state: "success", statuses: [] });
76+
// The issue the contributor correctly linked -- closed, by the rival's merge, one second after it landed.
77+
if (url.includes("/issues/8829")) return Response.json({ number: 8829, title: "The issue", state: "closed", closed_at: ISSUE_CLOSED, labels: [], assignees: [], user: { login: "reporter" } });
78+
if (url.includes("/issues/8886/comments") && (method === "POST" || method === "PATCH")) return Response.json({ id: 1 }, { status: 201 });
79+
if (url.includes("/issues/8886/comments")) return Response.json([]);
80+
if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } });
81+
return Response.json({});
82+
});
83+
}
84+
85+
async function runReview(supersededCloseFlag: string | undefined) {
86+
const env = createTestEnv({
87+
GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(),
88+
...(supersededCloseFlag === undefined ? {} : { LOOPOVER_SUPERSEDED_CLOSE: supersededCloseFlag }),
89+
});
90+
await seedRepo(env);
91+
stubGitHub();
92+
await reReviewStoredPullRequest(env, "superseded-wire", 123, REPO, 8886);
93+
const advisory = await getLatestAdvisoryForPullRequest(env, REPO, 8886);
94+
return (advisory?.findings ?? []).map((finding) => finding.code);
95+
}
96+
97+
describe("superseded linked issue, wired end to end (#10168)", () => {
98+
afterEach(() => {
99+
vi.unstubAllGlobals();
100+
});
101+
102+
it("reports the supersession instead of 'No linked issue detected' once the flag is on", async () => {
103+
const codes = await runReview("true");
104+
expect(codes).toContain("linked_issue_superseded");
105+
expect(codes).not.toContain("missing_linked_issue");
106+
});
107+
108+
it("is byte-identical to today's behaviour while the flag is off", async () => {
109+
// The whole feature ships dark: same finding, same message, same disposition as before it existed.
110+
for (const flag of [undefined, "false"]) {
111+
const codes = await runReview(flag);
112+
expect(codes, String(flag)).toContain("missing_linked_issue");
113+
expect(codes, String(flag)).not.toContain("linked_issue_superseded");
114+
}
115+
});
116+
});

test/unit/linked-issue-superseded.test.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest";
22
import {
33
SUPERSEDED_CLOSE_WINDOW_MS,
44
resolveSupersession,
5+
supersededSearchWindow,
56
type LinkedIssueClosure,
67
type MergedRivalPullRequest,
78
} from "../../src/review/linked-issue-superseded";
@@ -159,3 +160,30 @@ describe("resolveSupersession", () => {
159160
});
160161
});
161162
});
163+
164+
describe("supersededSearchWindow", () => {
165+
it("spans this PR's creation to the latest close plus the tolerance", () => {
166+
expect(supersededSearchWindow(PR_CREATED, [closure()])).toEqual({
167+
sinceIso: PR_CREATED,
168+
untilIso: new Date(Date.parse(ISSUE_CLOSED) + SUPERSEDED_CLOSE_WINDOW_MS).toISOString(),
169+
});
170+
});
171+
172+
it("anchors the end on the LATEST close when several issues are linked", () => {
173+
const later = "2026-07-31T10:00:00Z";
174+
const window = supersededSearchWindow(PR_CREATED, [closure(), closure({ issueNumber: 9000, closedAt: later })]);
175+
expect(window?.untilIso).toBe(new Date(Date.parse(later) + SUPERSEDED_CLOSE_WINDOW_MS).toISOString());
176+
});
177+
178+
it("declines without a synced createdAt — the 'issue outlived this PR' half cannot be established", () => {
179+
for (const value of [null, undefined, "", "whenever"]) {
180+
expect(supersededSearchWindow(value, [closure()]), String(value)).toBeNull();
181+
}
182+
});
183+
184+
it("declines when no linked issue was conclusively read as closed", () => {
185+
expect(supersededSearchWindow(PR_CREATED, [])).toBeNull();
186+
expect(supersededSearchWindow(PR_CREATED, [closure({ closedAt: null })])).toBeNull();
187+
expect(supersededSearchWindow(PR_CREATED, [closure({ closedAt: "not-a-date" })])).toBeNull();
188+
});
189+
});
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
import { describe, expect, it } from "vitest";
2+
import { isSupersededCloseEnabledGlobally } from "../../src/settings/superseded-close-mode";
3+
4+
describe("isSupersededCloseEnabledGlobally (#10168)", () => {
5+
it("defaults OFF when unset — recognising supersession CLOSES a PR, so it must be opted into", () => {
6+
expect(isSupersededCloseEnabledGlobally({})).toBe(false);
7+
expect(isSupersededCloseEnabledGlobally({ LOOPOVER_SUPERSEDED_CLOSE: undefined })).toBe(false);
8+
expect(isSupersededCloseEnabledGlobally({ LOOPOVER_SUPERSEDED_CLOSE: "" })).toBe(false);
9+
});
10+
11+
it("is ON for every value the codebase truthy convention accepts", () => {
12+
// Same trimmed, case-insensitive `/^(1|true|yes|on)$/i` as the sibling flags -- #10054 caught a flag that
13+
// was `=== "true"` only and silently read `1` / `on` / a whitespace-padded `.env` value as OFF.
14+
for (const value of ["1", "true", "TRUE", "yes", "on", " true "]) {
15+
expect(isSupersededCloseEnabledGlobally({ LOOPOVER_SUPERSEDED_CLOSE: value }), value).toBe(true);
16+
}
17+
});
18+
19+
it("stays OFF for a falsy or unrecognised value", () => {
20+
for (const value of ["0", "false", "off", "no", "maybe"]) {
21+
expect(isSupersededCloseEnabledGlobally({ LOOPOVER_SUPERSEDED_CLOSE: value }), value).toBe(false);
22+
}
23+
});
24+
});

0 commit comments

Comments
 (0)