Skip to content
Closed
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
69 changes: 69 additions & 0 deletions packages/loopover-engine/src/github/linked-references.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
// GitHub cross-reference extraction — the closing-keyword ("Closes #123") and PR-mention parsers that decide
// whether a pull request has a linked issue at all.
//
// This is pure, side-effect-free regex logic that was stranded inside `src/db/repositories.ts` (#4882) — a ~386KB,
// D1-query-heavy repository-access file — even though the four host modules that consume it (`src/github/backfill.ts`,
// `src/review/enrichment-wire.ts`, `src/review/linked-issue-hard-rules.ts`, `src/signals/engine.ts`) want the parsing
// and nothing from the database layer. The engine could not reach into `src/` at all, so
// `signals/predicted-gate-engine.ts` carried a hand-written second copy that had silently diverged from the live
// gate's — it was missing the inline-code-span guard documented below, so the miner's predicted gate credited a
// linked issue for a body that the live gate reads as having none.
//
// Living in the engine, this is the single source of truth both gates resolve, so they can no longer disagree.

/** Hard cap on how many linked issues one body may declare, so a pathological body can't fan out unbounded work. */
export const MAX_LINKED_ISSUE_NUMBERS = 50;

export type LinkedIssueExtractionResult = {
numbers: number[];
/** True when the body declared MORE distinct issues than `limit` — the caller decides what an overflow means. */
overflow: boolean;
};

export function extractLinkedIssueNumbersWithOverflow(
text: string,
repoFullName: string,
limit = MAX_LINKED_ISSUE_NUMBERS,
): LinkedIssueExtractionResult {
const normalizedLimit = Math.max(0, Math.floor(limit));
const target = repoFullName.toLowerCase();

// GitHub's native closing-keyword linker does not treat backtick-wrapped text as a real
// "Closes #N" directive, and this repo's own PR template contains "(e.g. `Closes #123`)".
// Keep the original text while rejecting regex hits that occur inside inline code spans; replacing
// spans with whitespace would let text on either side combine into a fake closing reference.
const inlineCodeSpanRanges = [...text.matchAll(/`[^`\n]*`/g)].map((match) => ({
start: match.index!,
end: match.index! + match[0].length,
}));

const linkedIssues: number[] = [];
const seen = new Set<number>();
// Matches both GitHub's bare `KEYWORD #N` and fully-qualified `KEYWORD owner/repo#N` closing syntax (#3862) --
// the qualified form only counts when owner/repo case-insensitively matches THIS repo; a reference to a
// different repo closes an issue there, not here, and must not spoof a same-repo linked-issue match.
for (const match of text.matchAll(/\b(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)\s+(?:([\w.-]+\/[\w.-]+)#|#)(\d+)\b/gi)) {
const matchStart = match.index!;
const matchEnd = matchStart + match[0].length;
if (inlineCodeSpanRanges.some((range) => matchStart < range.end && matchEnd > range.start)) continue;
const owner = match[1];
if (owner && owner.toLowerCase() !== target) continue;
const value = Number(match[2]);
if (!Number.isInteger(value) || value <= 0 || seen.has(value)) continue;
seen.add(value);
if (linkedIssues.length >= normalizedLimit) return { numbers: linkedIssues, overflow: true };
linkedIssues.push(value);
}
return { numbers: linkedIssues, overflow: false };
}

/** {@link extractLinkedIssueNumbersWithOverflow} for the callers that only need the numbers. */
export function extractLinkedIssueNumbers(text: string, repoFullName: string, limit = MAX_LINKED_ISSUE_NUMBERS): number[] {
return extractLinkedIssueNumbersWithOverflow(text, repoFullName, limit).numbers;
}

/** Extract the PR numbers an issue body mentions in prose (`PR #12`, `pull request #12`). Deduped, positive only. */
export function extractLinkedPrNumbers(text: string): number[] {
const matches = [...text.matchAll(/\b(?:PR|pull request)\s+#(\d+)\b/gi)];
return [...new Set(matches.map((match) => Number(match[1])).filter((value) => Number.isInteger(value) && value > 0))];
}
16 changes: 4 additions & 12 deletions packages/loopover-engine/src/signals/predicted-gate-engine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ import type {
SignalFinding,
} from "../types/predicted-gate-types.js";
import { nowIso } from "../utils/json.js";
// The predicted gate resolves the SAME closing-keyword parser the live gate does (#4882). It used to keep a
// hand-written copy, which had drifted: no inline-code-span guard, so an unedited PR template — whose checklist
// line reads "(e.g. `Closes #123`)" — predicted a linked issue where the live gate correctly sees none.
import { extractLinkedIssueNumbers } from "../github/linked-references.js";
import { PREFLIGHT_LIMITS } from "./preflight-limits.js";
import { hasValidationNote, isTestPath } from "./test-evidence.js";
import { diffFilePriority } from "../review/diff-file-priority.js";
Expand Down Expand Up @@ -917,18 +921,6 @@ export function tokenize(value: string): string[] {
.filter((term) => term.length > 2 && !STOPWORDS.has(term));
}

function extractLinkedIssueNumbers(text: string, repoFullName: string): number[] {
const numbers = [...text.matchAll(/\b(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)\s+#(\d+)\b/gi)].map((match) => Number(match[1]));
// GitHub also auto-closes via the fully-qualified `KEYWORD owner/repo#N` form (e.g. Renovate/Dependabot bodies).
// Count it only when owner/repo case-insensitively equals THIS repo — a reference to a different repo closes an
// issue elsewhere, not here, so it must not spoof a same-repo link. Same `\b`-anchored keywords as above (#1988).
const target = repoFullName.toLowerCase();
for (const match of text.matchAll(/\b(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)\s+([\w.-]+\/[\w.-]+)#(\d+)\b/gi)) {
if (match[1]!.toLowerCase() === target) numbers.push(Number(match[2]));
}
return [...new Set(numbers.filter((value) => Number.isInteger(value) && value > 0))];
}

function isMaintainerAssociation(value: string | null | undefined): boolean {
return value === "OWNER" || value === "MEMBER" || value === "COLLABORATOR";
}
Expand Down
63 changes: 15 additions & 48 deletions src/db/repositories.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import { parsePullRequestTargetKey } from "@loopover/engine";
import { and, asc, desc, eq, gte, inArray, not, or, sql, type SQL } from "drizzle-orm";
import {
extractLinkedIssueNumbers,
extractLinkedPrNumbers,
} from "../../packages/loopover-engine/src/github/linked-references";
import { getDb } from "./client";
import {
activeReviewTracking,
Expand Down Expand Up @@ -7829,51 +7833,14 @@ function loginMatches(column: unknown, login: string) {
return sql`lower(${column}) = ${login.toLowerCase()}`;
}

export const MAX_LINKED_ISSUE_NUMBERS = 50;

export type LinkedIssueExtractionResult = {
numbers: number[];
overflow: boolean;
};

export function extractLinkedIssueNumbersWithOverflow(text: string, repoFullName: string, limit = MAX_LINKED_ISSUE_NUMBERS): LinkedIssueExtractionResult {
const normalizedLimit = Math.max(0, Math.floor(limit));
const target = repoFullName.toLowerCase();

// GitHub's native closing-keyword linker does not treat backtick-wrapped text as a real
// "Closes #N" directive, and this repo's own PR template contains "(e.g. `Closes #123`)".
// Keep the original text while rejecting regex hits that occur inside inline code spans; replacing
// spans with whitespace would let text on either side combine into a fake closing reference.
const inlineCodeSpanRanges = [...text.matchAll(/`[^`\n]*`/g)].map((match) => ({
start: match.index!,
end: match.index! + match[0].length,
}));

const linkedIssues: number[] = [];
const seen = new Set<number>();
// Matches both GitHub's bare `KEYWORD #N` and fully-qualified `KEYWORD owner/repo#N` closing syntax (#3862) --
// the qualified form only counts when owner/repo case-insensitively matches THIS repo; a reference to a
// different repo closes an issue there, not here, and must not spoof a same-repo linked-issue match.
for (const match of text.matchAll(/\b(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)\s+(?:([\w.-]+\/[\w.-]+)#|#)(\d+)\b/gi)) {
const matchStart = match.index!;
const matchEnd = matchStart + match[0].length;
if (inlineCodeSpanRanges.some((range) => matchStart < range.end && matchEnd > range.start)) continue;
const owner = match[1];
if (owner && owner.toLowerCase() !== target) continue;
const value = Number(match[2]);
if (!Number.isInteger(value) || value <= 0 || seen.has(value)) continue;
seen.add(value);
if (linkedIssues.length >= normalizedLimit) return { numbers: linkedIssues, overflow: true };
linkedIssues.push(value);
}
return { numbers: linkedIssues, overflow: false };
}

export function extractLinkedIssueNumbers(text: string, repoFullName: string, limit = MAX_LINKED_ISSUE_NUMBERS): number[] {
return extractLinkedIssueNumbersWithOverflow(text, repoFullName, limit).numbers;
}

function extractLinkedPrNumbers(text: string): number[] {
const matches = [...text.matchAll(/\b(?:PR|pull request)\s+#(\d+)\b/gi)];
return [...new Set(matches.map((match) => Number(match[1])).filter((value) => Number.isInteger(value) && value > 0))];
}
// The closing-keyword / PR-mention parsers moved to `@loopover/engine` (#4882): they are pure regex logic with no
// D1 or `Env` dependency, and the engine's predicted gate needs the exact same answer the live gate computes here.
// Re-exported so the host modules that already import them from this file keep working unchanged. Imported via
// relative source path, not the published package, to match this repo's existing engine-consumption convention
// (see e.g. src/signals/check-summary.ts).
export {
extractLinkedIssueNumbers,
extractLinkedIssueNumbersWithOverflow,
type LinkedIssueExtractionResult,
MAX_LINKED_ISSUE_NUMBERS,
} from "../../packages/loopover-engine/src/github/linked-references";
122 changes: 122 additions & 0 deletions test/unit/linked-references.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
import { describe, expect, it } from "vitest";

import {
extractLinkedIssueNumbers,
extractLinkedIssueNumbersWithOverflow,
extractLinkedPrNumbers,
MAX_LINKED_ISSUE_NUMBERS,
} from "../../packages/loopover-engine/src/github/linked-references";
import { predictedGateEngineInternals } from "../../packages/loopover-engine/src/signals/predicted-gate-engine";
import { extractLinkedIssueNumbers as extractViaRepositoriesShim } from "../../src/db/repositories";

const REPO = "acme/widgets";

/** The repo's own `.github/pull_request_template.md` checklist line, verbatim. */
const PR_TEMPLATE_LINE =
"- [ ] I linked a currently open issue this PR resolves (e.g. `Closes #123`) — a linked open issue is required for every contributor PR.";

describe("extractLinkedIssueNumbersWithOverflow() (#4882)", () => {
it("extracts the bare `KEYWORD #N` closing form for every supported keyword", () => {
for (const keyword of ["close", "closes", "closed", "fix", "fixes", "fixed", "resolve", "resolves", "resolved"]) {
expect(extractLinkedIssueNumbers(`${keyword} #7`, REPO)).toEqual([7]);
}
expect(extractLinkedIssueNumbers("CLOSES #7", REPO)).toEqual([7]);
});

it("extracts the qualified `KEYWORD owner/repo#N` form only when owner/repo is THIS repo", () => {
expect(extractLinkedIssueNumbers(`closes ${REPO}#9`, REPO)).toEqual([9]);
expect(extractLinkedIssueNumbers("closes ACME/Widgets#9", REPO)).toEqual([9]);
// A reference to a DIFFERENT repo closes an issue there, not here — it must not spoof a same-repo link.
expect(extractLinkedIssueNumbers("closes other/repo#9", REPO)).toEqual([]);
});

it("REGRESSION: rejects a closing keyword wrapped in an inline code span, so the unedited PR template links nothing", () => {
expect(extractLinkedIssueNumbers(PR_TEMPLATE_LINE, REPO)).toEqual([]);
expect(extractLinkedIssueNumbers("`Closes #123`", REPO)).toEqual([]);
expect(extractLinkedIssueNumbers(`\`closes ${REPO}#123\``, REPO)).toEqual([]);
});

it("still counts a real closing keyword that merely sits near an unrelated code span", () => {
// Span AFTER the match, and span BEFORE the match — neither overlaps, so neither suppresses it.
expect(extractLinkedIssueNumbers("closes #5 in `src/a.ts`", REPO)).toEqual([5]);
expect(extractLinkedIssueNumbers("`src/a.ts` — closes #5", REPO)).toEqual([5]);
// A span cannot be blanked out first: that would let the text on either side combine into a fake reference.
expect(extractLinkedIssueNumbers("closes `nothing` #5", REPO)).toEqual([]);
});

it("dedupes repeats and drops non-positive / non-finite issue numbers", () => {
expect(extractLinkedIssueNumbers("closes #4\nfixes #4\nresolves #6", REPO)).toEqual([4, 6]);
expect(extractLinkedIssueNumbers("closes #0", REPO)).toEqual([]);
// 400 digits overflows to Infinity, which is not an integer.
expect(extractLinkedIssueNumbers(`closes #${"9".repeat(400)}`, REPO)).toEqual([]);
});

it("reports overflow once the body declares more distinct issues than the limit", () => {
expect(extractLinkedIssueNumbersWithOverflow("closes #1 closes #2 closes #3", REPO, 2)).toEqual({
numbers: [1, 2],
overflow: true,
});
expect(extractLinkedIssueNumbersWithOverflow("closes #1 closes #2", REPO, 2)).toEqual({
numbers: [1, 2],
overflow: false,
});
expect(extractLinkedIssueNumbersWithOverflow("", REPO)).toEqual({ numbers: [], overflow: false });
});

it("normalizes a fractional or negative limit to a non-negative integer", () => {
expect(extractLinkedIssueNumbersWithOverflow("closes #1 closes #2 closes #3", REPO, 2.9).numbers).toEqual([1, 2]);
// A negative limit floors to 0: the very first hit already exceeds it.
expect(extractLinkedIssueNumbersWithOverflow("closes #1", REPO, -5)).toEqual({ numbers: [], overflow: true });
});

it("defaults to MAX_LINKED_ISSUE_NUMBERS, and honours an explicit limit", () => {
expect(MAX_LINKED_ISSUE_NUMBERS).toBe(50);
const body = Array.from({ length: 51 }, (_, index) => `closes #${index + 1}`).join(" ");
const result = extractLinkedIssueNumbersWithOverflow(body, REPO);
expect(result.numbers).toHaveLength(MAX_LINKED_ISSUE_NUMBERS);
expect(result.overflow).toBe(true);
expect(extractLinkedIssueNumbers("closes #1 closes #2", REPO, 1)).toEqual([1]);
});
});

describe("extractLinkedPrNumbers() (#4882)", () => {
it("extracts, dedupes, and filters prose PR mentions", () => {
expect(extractLinkedPrNumbers("see PR #12 and pull request #13, plus PR #12 again")).toEqual([12, 13]);
expect(extractLinkedPrNumbers("PR #0")).toEqual([]);
expect(extractLinkedPrNumbers(`PR #${"9".repeat(400)}`)).toEqual([]);
expect(extractLinkedPrNumbers("no references here")).toEqual([]);
});
});

describe("linked-reference parser convergence (#4882)", () => {
it("the src/db/repositories shim resolves the engine implementation", () => {
expect(extractViaRepositoriesShim(PR_TEMPLATE_LINE, REPO)).toEqual([]);
expect(extractViaRepositoriesShim(`closes ${REPO}#42`, REPO)).toEqual([42]);
});

it("REGRESSION: the predicted gate now agrees with the live gate on an unedited PR template", () => {
// Before the convergence, the engine's own copy had no inline-code-span guard, so it read the template's
// "(e.g. `Closes #123`)" as a real link and predicted a PASS on a PR the live gate closes for having none.
expect(predictedGateEngineInternals.extractLinkedIssueNumbers(PR_TEMPLATE_LINE, REPO)).toEqual([]);
expect(predictedGateEngineInternals.extractLinkedIssueNumbers(PR_TEMPLATE_LINE, REPO)).toEqual(
extractViaRepositoriesShim(PR_TEMPLATE_LINE, REPO),
);
});

it("the two gates agree across the whole closing-keyword grammar", () => {
const bodies = [
"closes #1",
`fixes ${REPO}#2`,
"resolves other/repo#3",
"`closes #4`",
"closes #5 in `src/a.ts`",
"closes #0",
"nothing to see",
];
for (const body of bodies) {
expect(predictedGateEngineInternals.extractLinkedIssueNumbers(body, REPO)).toEqual(
extractViaRepositoriesShim(body, REPO),
);
}
});
});