fix(utils): require a project marker before ordinary-word dirs exclude - #1763
fix(utils): require a project marker before ordinary-word dirs exclude#1763TalexDreamSoul wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe file scanner now passes sibling entry names to directory filtering. Context-dependent blacklist names are excluded when project markers are present, while unconditional exclusions remain active. Tests cover marker variants, nested paths, case-insensitivity, context-free callers, and empty sibling context. ChangesProject-context directory filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change narrows ordinary-word directory exclusions to project-marked locations while preserving system, cache, and dependency exclusions; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant scanDirectoryInto
participant FileSystem
participant FileFilterService
scanDirectoryInto->>FileSystem: Read directory entries
FileSystem-->>scanDirectoryInto: Return sibling names
scanDirectoryInto->>FileFilterService: Evaluate path with siblingNames
FileFilterService-->>scanDirectoryInto: Return exclusion reason
scanDirectoryInto->>scanDirectoryInto: Recurse with sibling context
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying tuff with
|
| Latest commit: |
2ecd318
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://72d11124.tuff-dso.pages.dev |
| Branch Preview URL: | https://fix-1727-dev-dir-policy.tuff-dso.pages.dev |
`build`, `dist`, `out`, `bin`, `target`, `coverage`, `logs`, `tmp`, `temp` and `cache` were matched on the leaf name at any depth, so a folder a user created under an index root was skipped for being named an ordinary English word. The check runs before `readdir`, so nothing errored, nothing incremented `errorCount`, and the file was simply absent from search -- indistinguishable from a file that does not exist. They now exclude only where the caller can show a project marker (`package.json`, `Cargo.toml`, `.git`, ...) sits beside the directory: the same signal ripgrep, fd and VS Code use for this question, and free here because the traversal has already read the parent before it descends. `node_modules` is nobody's document folder and keeps excluding everywhere. `PATH_PATTERNS.DEV_PATHS` / `CACHE_PATHS` restate those names as unanchored substrings that match an *ancestor* segment, so relaxing the leaf name alone would have fixed `~/Documents/build` and still lost `~/Documents/build/2026` -- half a fix, and the half that looks right in a shallow test. They are skipped for callers walking top-down, which have already had the per-level rule applied at every ancestor. `SYSTEM_PATHS` stays unconditional: it is what keeps `/usr/bin` excluded once `bin` is relaxed. Callers that pass no sibling list keep the pre-#1727 answer, so index upserts, manual adds and the file-level containing-path check are unchanged. Fixes #1727
96d09be to
2ecd318
Compare
Wiring verificationI said in review that this PR had no end-to-end verification, and that the fixture experiment I tried did not run — the control file came back unindexed, which proved the experiment was broken rather than the filter aggressive. Here is what I did instead, and what it does and does not establish. The risk worth checking is not "is the rule right" — the unit tests cover that. It is "is the new context actually supplied by the real scanner, at the right level, in the shape the filter reads". A context that no production caller populates would leave The capability is derived, not passed as a flag: The recursion supplies it from entries it already has — this is the line that makes the PR live, and the one absent on master: so every directory below a scan root is judged against its own sibling list, and a scan root — which has no parent that was read — passes And the production index path is that same code, not a parallel implementation: consumed by both Worked through the fixture case by hand against that chain: What this does not establish. No live scan was run and no rows were read out of |
|
Disclosure against #318, which scopes the remaining indexing-memory risk to exactly the function this PR changes. That issue's residual is that const entryNames = entries.map((entry) => entry.name);retained for the duration of each directory's recursion, alongside
The strings are shared with the Dirent objects, so this is the pointer array only. A Dirent stand-in was used, so the ratio is the reliable part, not the absolute MiB. Not proposing a change here. The comment at that line already says the map "costs one map, no syscall", which is true about syscalls and silent about memory — worth saying out loud on an issue that is specifically about this function's retention. It is also compatible with the Also noted on #318: that issue's tree-level numbers were measured on the assumption the dev/temp filter half stays as-is, which is what this PR changes. They should be re-measured after this lands rather than carried forward. |
1 participant
Closes the last two acceptance criteria of #1727. The first two — root-anchoring
SYSTEM_BLACKLISTED_DIRS, and keeping~/Library/%APPDATA%excluded — landed in #1728 and #1731.The decision #1727 asked for
Not by name alone.
node_modulesis nobody's document folder and keeps excluding wherever it appears.build,dist,out,bin,target,coverage,logs,tmp,tempandcacheare ordinary English words, and they now exclude only where a project marker (package.json,Cargo.toml,go.mod,Makefile,.git,*.csproj, …) sits beside the directory.That is the signal ripgrep, fd and VS Code use for the same question, and it is free here:
scanDirectoryIntohas already calledreaddiron the parent before it descends, so the sibling list is in hand. Onemap, no extra syscall.~/Documents/build~/Documents/build/2026~/Movies/tmp,~/Pictures/cache,~/Documents/logs~/Projects/app/dist(next topackage.json)~/Documents/x/node_modules/usr/bin,/var/tmpThe half a leaf-name-only fix would have missed
PATH_PATTERNS.DEV_PATHScarries/build\//andCACHE_PATHScarries/\/tmp\//— unanchored, so they match an ancestor segment. Relaxing the leaf name alone would have made~/Documents/buildindexable while everything under it stayed excluded: a fix that passes a shallow test and still loses the user's files.Those two pattern sets are therefore skipped for callers that supply a sibling list. Such a caller is walking top-down and has already had the per-level rule applied at every ancestor, and every name the patterns carry is either in
DEV_BLACKLISTED_DIRS/TEMP_BLACKLISTED_DIRSor starts with a dot, so nothing is lost.SYSTEM_PATHSstays unconditional — it is the one of the three that catches what the per-level name rule cannot,/usr/binbeing exactly that case oncebinis relaxed.Blast radius
TraversalContextis optional and onlyscanDirectoryIntopasses it. Index upserts, manual adds and the file-level containing-path check read as "cannot tell" and keep the pre-#1727 answer, so they are byte-for-byte unchanged.undefinedand[]are deliberately distinct: never-looked vs. read-and-found-nothing.One reason string changes:
/usr/binwas reported asdevelopment-pathand is nowsystem-path. No caller branches on the value — every one of them tests=== nullor truthiness.Verification
packages/utilsfull suite: 189 files, 1378 passed, 1 skippedfile-filter-project-context.test.ts, both directionspnpm lint(the utils CI gate) cleanFixes #1727
Summary by CodeRabbit
Bug Fixes
Tests