fix(review): make engine-parity's directory scan recursive - #4654
Conversation
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
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4654 +/- ##
=======================================
Coverage 94.11% 94.11%
=======================================
Files 433 433
Lines 38494 38494
Branches 14037 14037
=======================================
Hits 36229 36229
Misses 1604 1604
Partials 661 661 🚀 New features to boost your workflow:
|
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-10 09:47:54 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 4 non-blocking
Concerns raised — review before merging
Review context
Contributor next steps
Signal definitions
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.
|
What
Completes #4605's third acceptance criterion. PR #4638 already shipped the other two (the Carthage
cartfile\.lock→cartfile\.resolvedregex fix, and named-pair coverage for the specific already-knownnested/renamed duplicates:
safe-url.ts,diffFilePriority,sharesMeaningfulFile) — that PR mergedbefore this piece was caught as missing by independent review, so #4605 was reopened to track just this
remaining scope.
discoverEngineParityPairsonly listed the immediate children of eachsrc/{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 handle it correctly once
discovered.
Fix
collectTsFilesRecursivewalks the tree via the same pluggablelistDir(root, relativePath)shape thefunction already accepted — a
.ts-suffixed entry is a leaf, anything else is probed with anotherlistDircall and treated as a subdirectory only if that returns ≥1 entry (reusingdefaultListDir'sexisting resolve-to-
[]convention for non-directories, no new stat call needed). Pairing is now byidentical relative sub-path on both sides, not bare filename — so a depth mismatch (e.g.
safe-url.tsnested under
content-lane/on the host but flat on the engine) still correctly falls through to itsNAMED_TWIN_PAIRSentry rather than going undiscovered or getting falsely paired with an unrelatedsame-named file elsewhere in the tree.
Testing
4 new tests: nested-pair discovery on both sides, depth-mismatch still invisible (regression guard for
the
safe-url.tscase), non-.ts/non-directory entries handled without recursing, and the existingsafe-url.tsnon-discovery assertion re-confirmed against the real repo. All 34 tests incheck-engine-parity-script.test.tspass, including the 2 pre-existing regression guards (real-repo paircount, real-repo zero-drift). Coverage confirmed 100% on every new/changed line (the file's global branch
coverage number reported by vitest reflects pre-existing uncovered branches elsewhere in the file, not
this diff). Typecheck clean.
Part of #4605