From cacaedc73467378b37d9241566aa3c6834971184 Mon Sep 17 00:00:00 2001 From: glorydavid03023 Date: Tue, 14 Jul 2026 08:32:11 -0500 Subject: [PATCH] refactor(engine): extract the stranded linked-reference parsers out of the D1 repository layer (#4882) `src/db/repositories.ts` is a ~386KB, D1-query-heavy repository-access file, and #4882 calls out the exact candidate stranded inside it: "a pure regex-based parser living inside the very large D1-query-heavy repository-access file". That parser is the linked-issue/PR reference extractor -- pure regex logic with no D1 or `Env` dependency, yet four core modules reach into the database layer purely to get at it. Move it to `@loopover/engine` as `github/linked-references.ts` (regexes byte-for-byte identical, so the live gate's behavior is provably unchanged), and re-export it from `src/db/repositories.ts` as a thin shim so every existing importer keeps working. This also converges a real divergence. Because the engine cannot import from `src/`, `signals/predicted-gate-engine.ts` carried a hand-written second copy that had drifted: it was missing the inline-code-span guard. This repo's own PR template contains "(e.g. `Closes #123`)" on the line contributors are told to fill out, not replace -- so the predicted gate read a linked issue where the live gate correctly sees none, telling contributors their PR was safe right before the linked-issue hard rule closed it. The engine copy also lacked the MAX_LINKED_ISSUE_NUMBERS overflow cap that linked-issue-hard-rules.ts relies on. Both gates now resolve one shared module. Tests cover the moved module at 100% statements/branches/functions/lines, including both sides of the code-span overlap predicate, the cross-repo qualified form, overflow at default/explicit/fractional/ negative limits, and regression tests pinning that the predicted gate and the live gate now agree on the unedited PR template. --- .../src/github/linked-references.ts | 69 ++++++++++ .../src/signals/predicted-gate-engine.ts | 16 +-- src/db/repositories.ts | 63 +++------ test/unit/linked-references.test.ts | 122 ++++++++++++++++++ 4 files changed, 210 insertions(+), 60 deletions(-) create mode 100644 packages/loopover-engine/src/github/linked-references.ts create mode 100644 test/unit/linked-references.test.ts diff --git a/packages/loopover-engine/src/github/linked-references.ts b/packages/loopover-engine/src/github/linked-references.ts new file mode 100644 index 0000000000..61772b12fb --- /dev/null +++ b/packages/loopover-engine/src/github/linked-references.ts @@ -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(); + // 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))]; +} diff --git a/packages/loopover-engine/src/signals/predicted-gate-engine.ts b/packages/loopover-engine/src/signals/predicted-gate-engine.ts index 558a1df9b1..704cf7026f 100644 --- a/packages/loopover-engine/src/signals/predicted-gate-engine.ts +++ b/packages/loopover-engine/src/signals/predicted-gate-engine.ts @@ -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"; @@ -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"; } diff --git a/src/db/repositories.ts b/src/db/repositories.ts index f8bfaf90bc..a3d432a9ca 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -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, @@ -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(); - // 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"; diff --git a/test/unit/linked-references.test.ts b/test/unit/linked-references.test.ts new file mode 100644 index 0000000000..0dba52d7ea --- /dev/null +++ b/test/unit/linked-references.test.ts @@ -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), + ); + } + }); +});