Speed up local mount initialization#103
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds MountOptions.includeDefaultExcludeDirs to opt out of an expanded built-in default exclusion list, splits exclusion rules into any-depth vs root-prefix sets used for watcher globs and tree traversal, refactors the tree walk to track mount paths and return resolved directories, and updates tests and README accordingly. ChangesParameterized Default Excludes and Tree Walk Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f7260e2aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (opts.targetDirectoryReady) { | ||
| if (!isPathWithinRoot(parentPath, rootPath)) { | ||
| return null; | ||
| } | ||
| return path.join(parentPath, path.basename(candidatePath)); |
There was a problem hiding this comment.
When targetDirectoryReady is true, this branch returns a path after only a string-prefix check and skips resolving the parent directory with realpathSync. If another process replaces a previously validated mount subdirectory with a symlink between directory creation and file copy, later copyFileSync/linkSync calls can follow that symlink and write outside mountDir. The previous implementation revalidated each target parent via realpath on every file copy, so this change weakens the containment guarantee under concurrent filesystem mutation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 7b11430. I removed the targetDirectoryReady fast path, so resolveSafeCopyTarget always goes back through ensureDirectoryWithinRoot and uses the realpath-verified parent before copyFileSync writes.
| const code = (err as NodeJS.ErrnoException).code; | ||
| if (code === 'EXDEV' || code === 'EPERM') { | ||
| return false; |
There was a problem hiding this comment.
The hardlink fallback currently handles only EXDEV and EPERM; all other link failures are rethrown. On filesystems where hardlinks are unsupported or limited (for example returning ENOTSUP/EOPNOTSUPP/EMLINK), mount creation will now fail for readonly files instead of degrading to the existing copy path. Since this commit advertises hardlinking as an optimization with fallback behavior, these error codes should also trigger the copy fallback to avoid environment-specific breakage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Resolved in 1487757 by removing the readonly hardlink optimization entirely. Readonly files are back to copied 0o444 files, so this fallback path no longer exists.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const DEFAULT_EXCLUDED_DIRS = [ | ||
| '.git', | ||
| 'node_modules', | ||
| '.npm-cache', | ||
| 'target', | ||
| '.next', | ||
| 'dist', | ||
| 'build', | ||
| 'out', | ||
| '__pycache__', | ||
| '.pytest_cache', | ||
| '.mypy_cache', | ||
| '.ruff_cache', | ||
| '.venv', | ||
| 'venv', | ||
| 'env', | ||
| '.gradle', | ||
| 'coverage', | ||
| '.nyc_output', | ||
| '.turbo', | ||
| '.cache', | ||
| '.DS_Store', | ||
| ]; |
There was a problem hiding this comment.
🔴 Generic default exclude names match at any path depth, silently excluding nested source directories
The newly added default excludes (target, dist, build, out, env, venv, coverage) are matched as bare path segments at any depth by isExcludedPath (packages/local-mount/src/mount.ts:558-566), which checks excludeSet.has(segment) for every segment of a relative path. This means a project with nested source directories like src/build/, packages/env/, lib/out/, or src/target/ would have those entire subtrees silently excluded from the mount. The old defaults (.git, node_modules, .npm-cache) were unambiguous names that never appear as source directories; the new generic names are common in legitimate source paths. The tests only verify root-level exclusion, masking this issue.
Concrete example of silently excluded source files
Running the exclusion check against nested paths:
src/build/helper.ts→ excluded (segmentbuildmatches)packages/env/config.ts→ excluded (segmentenvmatches)lib/out/formatter.ts→ excluded (segmentoutmatches)src/target/arm64.rs→ excluded (segmenttargetmatches)test/coverage/reporter.ts→ excluded (segmentcoveragematches)
All of these are legitimate source paths that would be missing from the mount.
Prompt for agents
The DEFAULT_EXCLUDED_DIRS list now contains generic names like 'target', 'dist', 'build', 'out', 'env', 'venv', 'coverage' that are matched at any depth inside isExcludedPath (mount.ts:558-566) because it checks excludeSet.has(segment) for each path segment. This means src/build/helper.ts is excluded because the 'build' segment matches, even though only root-level build/ directories should be excluded.
The fix should make isExcludedPath distinguish between root-level-only and any-depth matching. One approach: entries without a '/' prefix are currently matched at any depth. You could either:
1. Change isExcludedPath to only match the first segment (root-level) for bare names, while keeping prefix-path entries (those with '/') matched as root-anchored prefixes. This would be a breaking change for the existing behavior with node_modules (which should match at any depth in monorepos).
2. Split the exclude set into two categories: names that should match at any depth (like .git, node_modules, .npm-cache, __pycache__) and names that should only match at root level (like build, dist, out, env, target, coverage, venv). The isExcludedPath function would check each category differently.
3. Use root-anchored path-style entries for the generic names in DEFAULT_EXCLUDED_DIRS. E.g. instead of 'build', use 'build/' (or some convention) that isExcludedPath treats as root-only. The buildIgnoreGlobs function in auto-sync.ts would also need updating to match.
Also update the tests to verify that nested paths like src/build/file.ts are NOT excluded.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Fixed in 7b11430. The exclude rules are now split into any-depth names and root-anchored prefixes; generic defaults like build, env, out, target, and coverage are root-only. I added regression coverage for src/build, packages/env, lib/out, src/target, and test/coverage staying mounted.
| const code = (err as NodeJS.ErrnoException).code; | ||
| if (code === 'EXDEV' || code === 'EPERM') { | ||
| return false; |
There was a problem hiding this comment.
🟡 hardlinkMountedFile re-throws on EMLINK and ENOSYS instead of falling back to copy
hardlinkMountedFile only catches EXDEV (cross-device) and EPERM (permission) errors, re-throwing on all others. If the source inode has reached the filesystem's maximum hardlink count (EMLINK — e.g. 65,000 on ext4), or if the filesystem doesn't support hardlinks at all (ENOSYS/EOPNOTSUPP — some FUSE/network mounts), the function throws instead of returning false to trigger the copy fallback. Since this is called inside copyMountedFile during walkProjectTree, an unhandled throw would abort mount creation entirely.
| const code = (err as NodeJS.ErrnoException).code; | |
| if (code === 'EXDEV' || code === 'EPERM') { | |
| return false; | |
| if (code === 'EXDEV' || code === 'EPERM' || code === 'EMLINK' || code === 'ENOSYS' || code === 'EOPNOTSUPP') { | |
| return false; | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Resolved in 1487757 by removing hardlinkMountedFile and the readonly hardlink path. Readonly files now use the original copied 0o444 behavior again.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/local-mount/src/mount.ts`:
- Around line 574-596: The fast-path that skips realpath resolution when
opts.targetDirectoryReady is true in resolveSafeCopyTarget is unsafe; revert to
always performing the realpath check by removing the early branch that returns
path.join(parentPath, path.basename(candidatePath)) and instead call
ensureDirectoryWithinRoot(rootPath, parentPath) in all cases (preserving the
existing null checks), so the function always uses the resolved realParent
before returning the final path; keep references to isPathWithinRoot,
ensureDirectoryWithinRoot, and the caller expectations around
copyFileSync/linkSync intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3d93eb13-c792-45cf-8b27-d63d4355827f
📒 Files selected for processing (6)
packages/local-mount/README.mdpackages/local-mount/src/auto-sync.test.tspackages/local-mount/src/auto-sync.tspackages/local-mount/src/launch.tspackages/local-mount/src/mount.test.tspackages/local-mount/src/mount.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/local-mount/src/mount.ts`:
- Around line 162-170: syncBack() currently ignores excludeRules and only
respects readonly/ignored/noSyncBack, causing generated dirs (dist/, build/,
node_modules/) to be copied back; update syncBack() to apply the same exclusion
logic used for autosync by reusing the excludeRules-derived helpers (the
isExcludedPath wrapper used in autoSyncContext,
excludedAnyDepthNames/excludedRootPrefixes or the isExcluded function) or accept
the autoSyncContext so syncBack() calls isExcluded(relPosix) alongside the
existing isPathMatched checks
(readonlyMatcher/ignoredMatcher/noSyncBackMatcher), ensuring files matched by
excludeRules are skipped during manual syncBack().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d4cb5c0-69f7-4aa4-9b24-2b068432d82a
📒 Files selected for processing (4)
packages/local-mount/README.mdpackages/local-mount/src/auto-sync.tspackages/local-mount/src/mount.test.tspackages/local-mount/src/mount.ts
✅ Files skipped from review due to trivial changes (2)
- packages/local-mount/README.md
- packages/local-mount/src/mount.test.ts
1 participant
Summary
ensureDirectoryWithinRootincludeDefaultExcludeDirs: falsefor callers that need those pathsbuild,env, andcoverageonly match at the project rootsyncBack().agentreadonlyfiles as copied0o444files so direct mount writes still fail instead of mutating source filesRefs #102.
Verification
npm run test --workspace=packages/local-mount(passed outside sandbox; sandboxed run cannot start macOS FSEvents)syncBack: returns a partial count when aborted mid-walk; rerun passed unchangednpm run typecheck(afternpm run build --workspace=packages/corein the fresh worktree)npm run build --workspace=packages/local-mountNo HTTP server changes, so
scripts/check-contract-surface.shwas not needed.