From 1097eb5e11e14e4a1b2619278b6a9c37f34cc143 Mon Sep 17 00:00:00 2001 From: joaovictor91123 Date: Sat, 25 Jul 2026 17:58:52 -0700 Subject: [PATCH] fix(orb): sync diffFilePriority vendored-directory regex across twin copies Closes #8648 --- scripts/check-engine-parity.ts | 31 ++++++++++---- src/review/review-diff.ts | 4 +- src/review/review-grounding.ts | 4 +- test/unit/check-engine-parity-script.test.ts | 43 +++++++++++++++++--- test/unit/review-diff.test.ts | 13 ++++++ test/unit/review-grounding.test.ts | 13 ++++++ 6 files changed, 93 insertions(+), 15 deletions(-) diff --git a/scripts/check-engine-parity.ts b/scripts/check-engine-parity.ts index b5fcaf95d5..20c49cb475 100644 --- a/scripts/check-engine-parity.ts +++ b/scripts/check-engine-parity.ts @@ -55,14 +55,17 @@ export const SAFE_URL_MARKERS = Object.freeze([ "export function isSafeEndpointUrl", ] as const); -/** `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 `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. */ +/** `diffFilePriority` is duplicated by FUNCTION, not by file: two 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. Both host copies are registered below. + * 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. The vendored-directory regex marker regression-guards #7526 / #8648: the engine copy was + * fixed first and both host twins silently kept the pre-fix directory set until this check covered the + * regex body itself (not just signature / `isLockfile` presence). */ export const DIFF_FILE_PRIORITY_TWIN_PAIR: NamedTwinPair = Object.freeze({ area: "diff-file-priority", hostRelative: "src/review/review-diff.ts", @@ -71,9 +74,20 @@ export const DIFF_FILE_PRIORITY_TWIN_PAIR: NamedTwinPair = Object.freeze({ engineFileName: "diff-file-priority.ts", }); +/** Second host twin of `diffFilePriority` — same markers as `DIFF_FILE_PRIORITY_TWIN_PAIR` (#8648). */ +export const DIFF_FILE_PRIORITY_GROUNDING_TWIN_PAIR: NamedTwinPair = Object.freeze({ + area: "diff-file-priority-grounding", + hostRelative: "src/review/review-grounding.ts", + engineRelative: "packages/loopover-engine/src/review/diff-file-priority.ts", + hostFileName: "review-grounding.ts", + engineFileName: "diff-file-priority.ts", +}); + export const DIFF_FILE_PRIORITY_MARKERS = Object.freeze([ "export function diffFilePriority(path: string): number {", "isLockfile(path)", + // Exact vendored-directory regex body shared by all three copies (#7526 / #8648). + "/(^|\\/)(dist|build|out|coverage|vendor|vendored|third_party|third-party|node_modules|bower_components|jspm_packages)\\//i", ] as const); /** `sharesMeaningfulFile` is a near-duplicate helper (the host folds its guard clause into one `if`; the @@ -146,6 +160,7 @@ export const NAMED_TWIN_PAIRS: ReadonlyArray<{ pair: NamedTwinPair; markers: rea { pair: GATE_DECISION_TWIN_PAIR, markers: GATE_DECISION_CORE_MARKERS }, { pair: SAFE_URL_TWIN_PAIR, markers: SAFE_URL_MARKERS }, { pair: DIFF_FILE_PRIORITY_TWIN_PAIR, markers: DIFF_FILE_PRIORITY_MARKERS }, + { pair: DIFF_FILE_PRIORITY_GROUNDING_TWIN_PAIR, markers: DIFF_FILE_PRIORITY_MARKERS }, { pair: SHARES_MEANINGFUL_FILE_TWIN_PAIR, markers: SHARES_MEANINGFUL_FILE_MARKERS }, { pair: SECRET_DETECTION_TWIN_PAIR, markers: SECRET_DETECTION_MARKERS }, ]); diff --git a/src/review/review-diff.ts b/src/review/review-diff.ts index a442b1ae80..9126cb35bc 100644 --- a/src/review/review-diff.ts +++ b/src/review/review-diff.ts @@ -27,7 +27,9 @@ export const DEFAULT_DIFF_BUDGET = 80_000; export function diffFilePriority(path: string): number { // 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; + // Must stay in sync with packages/loopover-engine/src/review/diff-file-priority.ts and + // review-grounding.ts — vendored/third_party/third-party/bower_components/jspm_packages (#7526 / #8648). + if (/(^|\/)(dist|build|out|coverage|vendor|vendored|third_party|third-party|node_modules|bower_components|jspm_packages)\//i.test(path)) return 4; if (/\.(md|mdx|markdown|rst|adoc|asciidoc|txt)$/i.test(path)) return 2; if (isTestPath(path)) return 1; return 0; // source code diff --git a/src/review/review-grounding.ts b/src/review/review-grounding.ts index 84b5e11b0f..e3c93329f4 100644 --- a/src/review/review-grounding.ts +++ b/src/review/review-grounding.ts @@ -193,7 +193,9 @@ export function diffFilePriority(path: string): number { // 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; + // Must stay in sync with packages/loopover-engine/src/review/diff-file-priority.ts and + // review-diff.ts — vendored/third_party/third-party/bower_components/jspm_packages (#7526 / #8648). + if (/(^|\/)(dist|build|out|coverage|vendor|vendored|third_party|third-party|node_modules|bower_components|jspm_packages)\//i.test(path)) return 4; if (/\.(md|mdx|markdown|rst|adoc|asciidoc|txt)$/i.test(path)) return 2; if (isTestPath(path)) return 1; return 0; // source code diff --git a/test/unit/check-engine-parity-script.test.ts b/test/unit/check-engine-parity-script.test.ts index 6831e4951a..08503212c0 100644 --- a/test/unit/check-engine-parity-script.test.ts +++ b/test/unit/check-engine-parity-script.test.ts @@ -15,6 +15,7 @@ import { describeEngineVersionSkew, DIFF_FILE_PRIORITY_MARKERS, DIFF_FILE_PRIORITY_TWIN_PAIR, + DIFF_FILE_PRIORITY_GROUNDING_TWIN_PAIR, discoverEngineParityPairs, discoverGateDecisionTwinPair, enginePackageVersionIncreased, @@ -348,12 +349,13 @@ describe("check-engine-parity script", () => { }); describe("named twin-pair coverage (#4605)", () => { - it("registers the gate-decision, safe-url, diff-file-priority, shares-meaningful-file, and secret-detection pairs", () => { + it("registers the gate-decision, content-lane, diff-file-priority twins, shares-meaningful-file, and secret-detection pairs", () => { const areas = NAMED_TWIN_PAIRS.map(({ pair }) => pair.area); expect(areas).toEqual([ "gate-decision", "content-lane", "diff-file-priority", + "diff-file-priority-grounding", "shares-meaningful-file", "secret-detection", ]); @@ -369,7 +371,7 @@ describe("check-engine-parity script", () => { expect(scanned.some((discovered) => discovered.fileName === "safe-url.ts")).toBe(false); }); - it("passes marker presence for all five named pairs against the real repo (regression guard)", () => { + it("passes marker presence for all named pairs against the real repo (regression guard)", () => { for (const { pair, markers } of NAMED_TWIN_PAIRS) { const result = checkGateDecisionTwinPresence({ root: process.cwd(), pair, markers }); expect(result.failures).toEqual([]); @@ -389,6 +391,7 @@ describe("check-engine-parity script", () => { const fixedHostBody = "export function diffFilePriority(path: string): number {\n" + " if (isLockfile(path)) return 4;\n" + + " if (/(^|\\/)(dist|build|out|coverage|vendor|vendored|third_party|third-party|node_modules|bower_components|jspm_packages)\\//i.test(path)) return 4;\n" + " return 0;\n" + "}\n"; const result = checkGateDecisionTwinPresence({ @@ -401,9 +404,39 @@ describe("check-engine-parity script", () => { pair: DIFF_FILE_PRIORITY_TWIN_PAIR, markers: DIFF_FILE_PRIORITY_MARKERS, }); - expect(result.failures).toHaveLength(1); + expect(result.failures.length).toBeGreaterThanOrEqual(1); expect(result.failures[0]).toContain(DIFF_FILE_PRIORITY_TWIN_PAIR.engineRelative); - expect(result.failures[0]).toContain(JSON.stringify(DIFF_FILE_PRIORITY_MARKERS[1])); + expect(result.failures.some((f) => f.includes(JSON.stringify(DIFF_FILE_PRIORITY_MARKERS[1])))).toBe(true); + }); + + it("diffFilePriority markers fail when a host twin keeps the pre-#7526 vendored-directory regex (#8648)", () => { + const staleHostBody = + "export function diffFilePriority(path: string): number {\n" + + " if (isLockfile(path)) return 4;\n" + + " if (/(^|\\/)(dist|build|out|coverage|vendor|node_modules)\\//i.test(path)) return 4;\n" + + " return 0;\n" + + "}\n"; + const fixedEngineBody = + "export function diffFilePriority(path: string): number {\n" + + " if (isLockfile(path)) return 4;\n" + + " if (/(^|\\/)(dist|build|out|coverage|vendor|vendored|third_party|third-party|node_modules|bower_components|jspm_packages)\\//i.test(path)) return 4;\n" + + " return 0;\n" + + "}\n"; + for (const pair of [DIFF_FILE_PRIORITY_TWIN_PAIR, DIFF_FILE_PRIORITY_GROUNDING_TWIN_PAIR]) { + const result = checkGateDecisionTwinPresence({ + root: "/fake", + readFile: (_root, relativePath) => { + if (relativePath === pair.hostRelative) return staleHostBody; + if (relativePath === pair.engineRelative) return fixedEngineBody; + throw new Error(`unexpected read: ${relativePath}`); + }, + pair, + markers: DIFF_FILE_PRIORITY_MARKERS, + }); + expect(result.failures).toHaveLength(1); + expect(result.failures[0]).toContain(pair.hostRelative); + expect(result.failures[0]).toContain(JSON.stringify(DIFF_FILE_PRIORITY_MARKERS[2])); + } }); it("safe-url and shares-meaningful-file marker sets are non-empty and pair-specific", () => { @@ -415,7 +448,7 @@ describe("check-engine-parity script", () => { ); }); - it("includes all five named pairs in runEngineParityChecks pairsChecked", () => { + it("includes all named pairs in runEngineParityChecks pairsChecked", () => { const result = runEngineParityChecks({ root: process.cwd() }); const checkedAreas = result.pairsChecked.map((pair) => pair.area); for (const { pair } of NAMED_TWIN_PAIRS) { diff --git a/test/unit/review-diff.test.ts b/test/unit/review-diff.test.ts index 625de14789..02ab5efa9f 100644 --- a/test/unit/review-diff.test.ts +++ b/test/unit/review-diff.test.ts @@ -47,6 +47,19 @@ describe("diffFilePriority — source survives, noise drops first", () => { expect(diffFilePriority("src/review/review-diff.ts")).toBe(0); expect(diffFilePriority("packages/api/handler.py")).toBe(0); }); + + it("ranks the extended vendored-directory set as noise(4), matching the engine copy (#8648)", () => { + for (const path of [ + "vendored/lib.js", + "third_party/x.js", + "third-party/x.js", + "bower_components/x.js", + "jspm_packages/x.js", + ]) { + expect(diffFilePriority(path)).toBe(4); + expect(diffFilePriority(path)).toBeGreaterThan(diffFilePriority("src/a.ts")); + } + }); }); describe("addedLineCount — counts +lines, ignores +++ header", () => { diff --git a/test/unit/review-grounding.test.ts b/test/unit/review-grounding.test.ts index 83f545547b..1287c21d37 100644 --- a/test/unit/review-grounding.test.ts +++ b/test/unit/review-grounding.test.ts @@ -261,6 +261,19 @@ describe("review-grounding: diffFilePriority (source survives the budget first)" expect(diffFilePriority("src/review/review-grounding.ts")).toBe(0); expect(diffFilePriority("packages/api/handler.py")).toBe(0); }); + + it("ranks the extended vendored-directory set as noise(4), matching the engine copy (#8648)", () => { + for (const path of [ + "vendored/lib.js", + "third_party/x.js", + "third-party/x.js", + "bower_components/x.js", + "jspm_packages/x.js", + ]) { + expect(diffFilePriority(path)).toBe(4); + expect(diffFilePriority(path)).toBeGreaterThan(diffFilePriority("src/a.ts")); + } + }); }); describe("review-grounding: fetchFullFileContents (injected FileFetcher, fail-safe + bounded)", () => {