From 7508bd164e8815ea5b2f7e9b2da691c0fd73f08d Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 10 Jul 2026 02:33:02 -0700 Subject: [PATCH] fix(review): make engine-parity's directory scan recursive discoverEngineParityPairs only listed the immediate children of each src/{review,settings,signals} area directory, so a hand-duplicated pair nested one directory deeper on both sides was invisible to the scan by construction -- even though the existing shim/stub filters would have handled it correctly once discovered. #4605's own Finding 1 (the Cartfile.resolved regex typo) was found by manual audit rather than this tripwire precisely because that class of duplicate lives in files the directory-only scan can't see. collectTsFilesRecursive walks the tree via the same pluggable listDir(root, relativePath) shape the function already accepted, treating a `.ts`-suffixed entry as a leaf and any other entry as a directory only if listDir on it returns at least one entry -- reusing defaultListDir's existing resolve-to-[] convention for non-directories rather than requiring a stat call. Pairing is now by identical relative sub-path on both sides, not bare filename, so a depth MISMATCH (e.g. safe-url.ts nested under content-lane/ on the host but flat on the engine) still correctly falls through to its NAMED_TWIN_PAIRS entry rather than silently going undiscovered OR getting falsely paired with an unrelated same-named file elsewhere in the tree. Completes #4605's third acceptance criterion (PR #4638 shipped the other two: the regex fix itself and named-pair coverage for the specific already-known nested/renamed duplicates). Part of #4605 --- scripts/check-engine-parity.ts | 45 ++++++++++++---- test/unit/check-engine-parity-script.test.ts | 56 ++++++++++++++++++++ 2 files changed, 92 insertions(+), 9 deletions(-) diff --git a/scripts/check-engine-parity.ts b/scripts/check-engine-parity.ts index 08ee70bcd4..f4b75f764c 100644 --- a/scripts/check-engine-parity.ts +++ b/scripts/check-engine-parity.ts @@ -173,9 +173,36 @@ export function isEngineStubPair(srcText: string, engineText: string): boolean { return engineCompact > 0 && srcCompact > engineCompact * 3 && engineCompact < 250; } +/** Recursively collect `.ts` file paths under `dirRelative` (relative to `root`), reusing the same + * pluggable `listDir(root, relativePath)` shape `discoverEngineParityPairs` already accepted (#4605 + * Finding 2: the old scan only listed the immediate children of each area directory, so a duplicate + * nested one level deeper -- e.g. `content-lane/` -- was invisible by construction even though the + * filter/shim/stub checks below it would have handled it fine). An entry ending in `.ts` is treated as a + * leaf file; anything else is probed with another `listDir` call and treated as a subdirectory only if + * that call returns at least one entry -- `defaultListDir` already resolves a non-directory path (or a + * missing one) to `[]` via its catch-all, so this reuses that existing convention rather than requiring + * callers to distinguish files from directories themselves (a plain `readdirSync` result can't tell them + * apart without an extra stat call per entry). An empty real subdirectory is indistinguishable from "not a + * directory" under this convention, which is harmless -- either way it contributes zero `.ts` files. */ +function collectTsFilesRecursive(root: string, dirRelative: string, listDir: EngineParityListDir): string[] { + const results: string[] = []; + for (const entry of listDir(root, dirRelative)) { + if (entry.endsWith(".ts")) { + results.push(join(dirRelative, entry)); + continue; + } + const subRelative = join(dirRelative, entry); + const subEntries = listDir(root, subRelative); + if (subEntries.length > 0) results.push(...collectTsFilesRecursive(root, subRelative, listDir)); + } + return results; +} + /** - * Discover in-scope hand-duplicated twins under src/{review,settings,signals} that also exist in the engine tree - * and are neither host shims nor engine stubs. + * Discover in-scope hand-duplicated twins under src/{review,settings,signals} (at any nesting depth, matched + * by identical sub-path on both sides -- a depth MISMATCH, like `content-lane/safe-url.ts` on the host vs a + * flat `safe-url.ts` on the engine, still needs its own `NAMED_TWIN_PAIRS` entry, same as before) that also + * exist in the engine tree and are neither host shims nor engine stubs. */ export function discoverEngineParityPairs({ root, @@ -190,17 +217,17 @@ export function discoverEngineParityPairs({ for (const area of ENGINE_PARITY_AREAS) { const hostDir = join(HOST_SRC_ROOT, area); const engineDir = join(ENGINE_SRC_ROOT, area); - const hostFiles = listDir(root, hostDir).filter((name) => name.endsWith(".ts")); - const engineFiles = new Set(listDir(root, engineDir).filter((name) => name.endsWith(".ts"))); - for (const fileName of hostFiles.sort()) { - if (!engineFiles.has(fileName)) continue; - const hostRelative = join(hostDir, fileName); - const engineRelative = join(engineDir, fileName); + const hostFiles = collectTsFilesRecursive(root, hostDir, listDir); + const engineFiles = new Set(collectTsFilesRecursive(root, engineDir, listDir)); + for (const hostRelative of hostFiles.sort()) { + const subPath = hostRelative.slice(hostDir.length + 1); + const engineRelative = join(engineDir, subPath); + if (!engineFiles.has(engineRelative)) continue; const hostText = readFile(root, hostRelative); const engineText = readFile(root, engineRelative); if (isThinEngineReExportShim(hostText)) continue; if (isEngineStubPair(hostText, engineText)) continue; - pairs.push({ area, fileName, hostRelative, engineRelative, hostText, engineText }); + pairs.push({ area, fileName: subPath, hostRelative, engineRelative, hostText, engineText }); } } return pairs; diff --git a/test/unit/check-engine-parity-script.test.ts b/test/unit/check-engine-parity-script.test.ts index c408864061..e14742c353 100644 --- a/test/unit/check-engine-parity-script.test.ts +++ b/test/unit/check-engine-parity-script.test.ts @@ -97,6 +97,62 @@ describe("check-engine-parity script", () => { expect(pairs.some((pair: EngineParityPair) => pair.fileName === "check-names.ts")).toBe(false); }); + describe("recursive nested-directory discovery (#4605)", () => { + it("discovers a pair nested one directory deeper on BOTH sides, invisible to a top-level-only scan", () => { + const body = "export const NESTED = 1;\n"; + const readFile = (_root: string, relativePath: string) => { + if (relativePath === "src/review/sub/nested.ts") return body; + if (relativePath === "packages/gittensory-engine/src/review/sub/nested.ts") return body; + throw new Error(`unexpected read: ${relativePath}`); + }; + const listDir = (_root: string, relativePath: string) => { + if (relativePath === "src/review") return ["sub"]; + if (relativePath === "src/review/sub") return ["nested.ts"]; + if (relativePath === "packages/gittensory-engine/src/review") return ["sub"]; + if (relativePath === "packages/gittensory-engine/src/review/sub") return ["nested.ts"]; + return []; + }; + const pairs = discoverEngineParityPairs({ root: "/fake", readFile, listDir }); + expect(pairs).toHaveLength(1); + expect(pairs[0]!.fileName).toBe("sub/nested.ts"); + expect(pairs[0]!.hostRelative).toBe("src/review/sub/nested.ts"); + expect(pairs[0]!.engineRelative).toBe("packages/gittensory-engine/src/review/sub/nested.ts"); + }); + + it("still requires an identical sub-path on both sides — a depth MISMATCH stays invisible to the scan (needs its own NAMED_TWIN_PAIRS entry)", () => { + const body = "export const MISMATCHED = 1;\n"; + const readFile = (_root: string, relativePath: string) => { + if (relativePath === "src/review/sub/mismatch.ts") return body; + if (relativePath === "packages/gittensory-engine/src/review/mismatch.ts") return body; + throw new Error(`unexpected read: ${relativePath}`); + }; + const listDir = (_root: string, relativePath: string) => { + if (relativePath === "src/review") return ["sub"]; + if (relativePath === "src/review/sub") return ["mismatch.ts"]; + if (relativePath === "packages/gittensory-engine/src/review") return ["mismatch.ts"]; + return []; + }; + const pairs = discoverEngineParityPairs({ root: "/fake", readFile, listDir }); + expect(pairs).toHaveLength(0); + }); + + it("treats an empty listDir result for a non-.ts entry as a leaf (not a directory) rather than throwing", () => { + // A plain file with no extension (or an empty real subdirectory) both resolve to listDir(...) === []; + // collectTsFilesRecursive must not recurse into it or blow up either way — just contribute zero files. + const listDir = (_root: string, relativePath: string) => { + if (relativePath === "src/review") return ["not-a-directory-or-ts-file"]; + return []; + }; + const pairs = discoverEngineParityPairs({ root: "/fake", readFile: () => "", listDir }); + expect(pairs).toEqual([]); + }); + + it("does not re-discover the already-invisible content-lane/safe-url.ts pair via recursion (depth mismatch, unchanged from before #4605's recursive fix)", () => { + const scanned = discoverEngineParityPairs({ root: process.cwd() }); + expect(scanned.some((discovered) => discovered.fileName.endsWith("safe-url.ts"))).toBe(false); + }); + }); + it("the real repo's hand-duplicated pairs agree after normalization (regression guard)", () => { const result = checkEngineParityDrift({ root: process.cwd() }); expect(result.failures).toEqual([]);