diff --git a/packages/loopover-engine/src/review/diff-file-priority.ts b/packages/loopover-engine/src/review/diff-file-priority.ts index 37c4f9bba8..c96ab0a1c8 100644 --- a/packages/loopover-engine/src/review/diff-file-priority.ts +++ b/packages/loopover-engine/src/review/diff-file-priority.ts @@ -1,7 +1,10 @@ +import { isLockfile } from "../signals/path-matchers.js"; import { isTestPath } from "../signals/test-evidence.js"; export function diffFilePriority(path: string): number { - if (/(^|\/)(package-lock\.json|npm-shrinkwrap\.json|pnpm-lock\.yaml|yarn\.lock|bun\.lock|bun\.lockb|cargo\.lock|poetry\.lock|pipfile\.lock|composer\.lock|gemfile\.lock|go\.sum|go\.work\.sum|uv\.lock|packages\.lock\.json|flake\.lock|deno\.lock|pubspec\.lock|podfile\.lock|mix\.lock|package\.resolved|gradle\.lockfile|pdm\.lock|conan\.lock|pixi\.lock|cartfile\.resolved|gopkg\.lock|shard\.lock|rebar\.lock|renv\.lock|chart\.lock)$|\.(min\.(js|css)|map|snap)$/i.test(path)) return 4; + // Lockfile-NAME matching delegates to the canonical isLockfile/LOCKFILE_NAMES so no copy of this + // function can drift from the shared set (the #4605 Finding 1 class); suffix patterns stay inline. + if (isLockfile(path) || /\.(min\.(js|css)|map|snap)$/i.test(path)) return 4; // Must stay in sync with signals/path-matchers.ts's isVendoredFileFrom -- the two already had this // obligation implicitly (bower_components/jspm_packages were added there in #2777 with no corresponding // update here, #7526) and now match the same directory-name set exactly. diff --git a/scripts/check-engine-parity.ts b/scripts/check-engine-parity.ts index 37af8db0ad..b5fcaf95d5 100644 --- a/scripts/check-engine-parity.ts +++ b/scripts/check-engine-parity.ts @@ -57,9 +57,12 @@ export const SAFE_URL_MARKERS = Object.freeze([ /** `diffFilePriority` is duplicated by FUNCTION, not by file: two byte-identical host copies * (review-diff.ts, review-grounding.ts) and a differently-named engine copy (diff-file-priority.ts) — - * none share a filename, so the directory scan never pairs them. The `cartfile\.resolved` marker directly - * regression-guards #4605 Finding 1: the engine copy's Carthage-lockfile regex had silently drifted to - * `cartfile\.lock`, which is not a real filename (Carthage's lockfile is `Cartfile.resolved`). */ + * none share a filename, so the directory scan never pairs them. The `isLockfile(path)` marker + * regression-guards #4605 Finding 1 at its root: that bug was the engine copy's hand-rolled + * Carthage-lockfile regex silently drifting to `cartfile\.lock` (not a real filename — Carthage's is + * `Cartfile.resolved`). Since #8357 every copy delegates lockfile-NAME matching to the canonical + * `isLockfile`/`LOCKFILE_NAMES`, so no copy owns a name list that CAN drift; asserting the delegation is + * present is therefore a strictly stronger guard than asserting one literal name inside a private regex. */ export const DIFF_FILE_PRIORITY_TWIN_PAIR: NamedTwinPair = Object.freeze({ area: "diff-file-priority", hostRelative: "src/review/review-diff.ts", @@ -70,7 +73,7 @@ export const DIFF_FILE_PRIORITY_TWIN_PAIR: NamedTwinPair = Object.freeze({ export const DIFF_FILE_PRIORITY_MARKERS = Object.freeze([ "export function diffFilePriority(path: string): number {", - "cartfile\\.resolved", + "isLockfile(path)", ] as const); /** `sharesMeaningfulFile` is a near-duplicate helper (the host folds its guard clause into one `if`; the diff --git a/src/review/review-diff.ts b/src/review/review-diff.ts index 73b443fbc4..a442b1ae80 100644 --- a/src/review/review-diff.ts +++ b/src/review/review-diff.ts @@ -6,6 +6,7 @@ // which survived even with full-file grounding on). This builder orders source-first, reduces oversized // patches hunk-aware instead of dropping them, and always lists patch-less/over-budget files. (#accuracy-gap-1) +import { isLockfile } from "../signals/path-matchers"; import { isTestPath } from "../signals/test-evidence"; import type { listPullRequestFiles } from "../db/repositories"; @@ -19,9 +20,13 @@ export const DEFAULT_DIFF_BUDGET = 80_000; * Test detection delegates to the canonical `isTestPath` so this matcher can't drift from it — the * previous inline regex missed real conventions (pytest `test_*.py`, Go `*_test.go`, Ruby `*_spec.rb`, * Cypress/Playwright `.cy`/`.e2e`, a bare `spec/` dir), so those tests were ranked as SOURCE(0) and - * could displace real source under a tight budget — the exact opposite of this function's job. */ + * could displace real source under a tight budget — the exact opposite of this function's job. + * Lockfile detection delegates to the canonical `isLockfile`/`LOCKFILE_NAMES` for the same reason: a + * hand-duplicated name list silently misses every future ecosystem addition to the shared set (which has + * been extended in batches before), ranking a new lockfile format as SOURCE(0) instead of GENERATED(4). */ export function diffFilePriority(path: string): number { - if (/(^|\/)(package-lock\.json|npm-shrinkwrap\.json|pnpm-lock\.yaml|yarn\.lock|bun\.lock|bun\.lockb|cargo\.lock|poetry\.lock|pipfile\.lock|composer\.lock|gemfile\.lock|go\.sum|go\.work\.sum|uv\.lock|packages\.lock\.json|flake\.lock|deno\.lock|pubspec\.lock|podfile\.lock|mix\.lock|package\.resolved|gradle\.lockfile|pdm\.lock|conan\.lock|pixi\.lock|cartfile\.resolved|gopkg\.lock|shard\.lock|rebar\.lock|renv\.lock|chart\.lock)$|\.(min\.(js|css)|map|snap)$/i.test(path)) return 4; + // Only the lockfile-NAME portion is delegated; the suffix-based generated-file patterns stay inline. + if (isLockfile(path) || /\.(min\.(js|css)|map|snap)$/i.test(path)) return 4; if (/(^|\/)(dist|build|out|coverage|vendor|node_modules)\//i.test(path)) return 4; if (/\.(md|mdx|markdown|rst|adoc|asciidoc|txt)$/i.test(path)) return 2; if (isTestPath(path)) return 1; diff --git a/src/review/review-grounding.ts b/src/review/review-grounding.ts index acaefae44e..576f76d847 100644 --- a/src/review/review-grounding.ts +++ b/src/review/review-grounding.ts @@ -22,6 +22,7 @@ // review-grounding.ts -- if reviewbot's copy is ever resynced from this file, carry this piece forward too, // or reviewbot will regress to the old all-or-nothing truncation this was written to eliminate. +import { isLockfile } from "../signals/path-matchers"; import { isTestPath } from "../signals/test-evidence"; import { neutralizePromptInjection } from "./prompt-injection"; @@ -189,7 +190,9 @@ export function buildGrounding(f: GroundingFlags, checks?: CheckAggregate, fileC * copy missed pytest `test_*.py`, Go `*_test.go`, Ruby `*_spec.rb`, Cypress/Playwright `.cy`/`.e2e`, and a * bare `spec/` dir — so those tests ranked as SOURCE(0) and were inlined ahead of real source). */ export function diffFilePriority(path: string): number { - if (/(^|\/)(package-lock\.json|npm-shrinkwrap\.json|pnpm-lock\.yaml|yarn\.lock|bun\.lock|bun\.lockb|cargo\.lock|poetry\.lock|pipfile\.lock|composer\.lock|gemfile\.lock|go\.sum|go\.work\.sum|uv\.lock|packages\.lock\.json|flake\.lock|deno\.lock|pubspec\.lock|podfile\.lock|mix\.lock|package\.resolved|gradle\.lockfile|pdm\.lock|conan\.lock|pixi\.lock|cartfile\.resolved|gopkg\.lock|shard\.lock|rebar\.lock|renv\.lock|chart\.lock)$|\.(min\.(js|css)|map|snap)$/i.test(path)) return 4; + // Lockfile-NAME matching delegates to the canonical isLockfile/LOCKFILE_NAMES so no copy of this + // function can drift from the shared set (the #4605 Finding 1 class); suffix patterns stay inline. + if (isLockfile(path) || /\.(min\.(js|css)|map|snap)$/i.test(path)) return 4; if (/(^|\/)(dist|build|out|coverage|vendor|node_modules)\//i.test(path)) return 4; if (/\.(md|mdx|markdown|rst|adoc|asciidoc|txt)$/i.test(path)) return 2; if (isTestPath(path)) return 1; diff --git a/test/unit/check-engine-parity-script.test.ts b/test/unit/check-engine-parity-script.test.ts index 9a81f3e6f3..6831e4951a 100644 --- a/test/unit/check-engine-parity-script.test.ts +++ b/test/unit/check-engine-parity-script.test.ts @@ -376,10 +376,11 @@ describe("check-engine-parity script", () => { } }); - it("diffFilePriority markers cover the exact Cartfile.resolved regression (#4605 Finding 1)", () => { - // Reproduces the actual bug: the engine copy's regex previously matched `cartfile\.lock` (not a - // real Carthage filename) instead of `cartfile\.resolved`. A body missing the resolved-lockfile - // marker fails presence — exactly what the old, un-checked engine copy would have done. + it("diffFilePriority markers cover the Cartfile.resolved regression class at its root (#4605 Finding 1, #8357)", () => { + // Reproduces the actual bug's ROOT CAUSE: a copy that hand-rolls its own lockfile-name regex can + // silently drift (the engine copy once matched `cartfile\.lock`, not a real Carthage filename). + // Since #8357 every copy delegates to the canonical isLockfile, so a body that still owns a private + // name list fails presence — exactly what the old, un-checked engine copy would have done. const buggyEngineBody = "export function diffFilePriority(path: string): number {\n" + " if (/cartfile\\.lock$/i.test(path)) return 4;\n" + @@ -387,7 +388,7 @@ describe("check-engine-parity script", () => { "}\n"; const fixedHostBody = "export function diffFilePriority(path: string): number {\n" + - " if (/cartfile\\.resolved$/i.test(path)) return 4;\n" + + " if (isLockfile(path)) return 4;\n" + " return 0;\n" + "}\n"; const result = checkGateDecisionTwinPresence({