fix(sea): files inside symlinks are not resolved correctly (#295) - #296
fix(sea): files inside symlinks are not resolved correctly (#295)#296mpotthoff wants to merge 11 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #296 +/- ##
==========================================
- Coverage 87.23% 86.45% -0.79%
==========================================
Files 23 23
Lines 7929 7929
Branches 1214 1206 -8
==========================================
- Hits 6917 6855 -62
- Misses 1005 1066 +61
- Partials 7 8 +1 🚀 New features to boost your workflow:
|
robertsLando
left a comment
robertsLando
left a comment
There was a problem hiding this comment.
Review summary
Verdict: Ship with changes — one Major (perf) worth landing first.
The issue and the fix are both confirmed — verified end-to-end, not just read
The bug is real: lib/walker.ts:459 records exactly one manifest entry per link (this.symLinks[file] = realFile), so node_modules/@t/lib/package.json has no key, and the pre-fix _resolveSymlink was exact-key only. The non-SEA prelude already did prefix matching (prelude/bootstrap.js:239-252, vfsKey.startsWith(k + sep)) — SEA was the outlier, which is exactly why standard mode worked.
The fix is correct. Built at 843326b on node v22.20.0:
| build | result |
|---|---|
| committed test, with fix | exit 0 |
committed test, without the sea-vfs-setup.js hunk |
ENOENT ... '/test-99-#295/lib/log.js', exit 1 |
the npm-workspace repro from #295 (node_modules/@t/lib -> ../../packages/lib, ESM bare import), SEA with fix |
exit 0 |
| same repro, SEA without fix | ERR_MODULE_NOT_FOUND at resolveBareSpecifier → vfsResolveHook, exit 1 |
| same repro, standard (non-SEA) mode | exit 0 — confirms the report |
Replaying _resolveSymlink against synthetic manifests: the #295 shape, multi-segment remainders, and chained links all resolve correctly; cycles terminate at MAX_SYMLINK_DEPTH (i is never reset); parentIdx > 0 correctly refuses the empty root key; win32 C:/… keys walk correctly and stop at C:; and there is no substring/prefix trap (/a/bc does not match /a/bcd). Nice work — the parent walk is also strictly better than bootstrap's version (O(path depth) hash lookups instead of O(number of symlinks) scans).
Top 3 risks
- Measured ~60× slowdown of
_resolveSymlinkfor any project that has symlinks — i.e. every project this PR fixes. See the inline comment on line 345. - Two divergent VFS symlink implementations (SEA vs
bootstrap.js) — the root-cause class of #295 itself. Inline on line 353. - A latent ELOOP throw out of
existsSync/internalModuleStat, which must not throw. Inline on line 365.
Findings outside the diff
- Major · Tests —
test/test.js:60-87:test-99-#295is a host-only SEA test (runSeaHostOnly, ignores the target arg) but wasn't added to thenpmTestsarray. It therefore falls through the**/main.jsglob and builds a SEA binary in bothtest:22andtest:24, each matrixed over 3 OSes, while never running intest:host— the exact redundancy the comment abovenpmTestsexists to prevent. Adding'test-99-#295'to that list fixes it. (test-93-sea-compresshas the same omission — pre-existing, but worth folding in while you're there.) - Minor · Design —
prelude/sea-vfs-setup.js:286-292: the class JSDoc still advertisesinternalModuleStat() O(1) manifest hash lookup (no tree walk),statSync() O(1)andexistsSync() O(1). All three are now O(path depth) with a directory walk whenever the manifest has symlinks. That doc block is what contributors read first, so it should either be restated or made true again by the memo above. - Minor · Design —
prelude/sea-vfs-setup.js:461-505:_resolveSymlinkis now applied asymmetrically across the provider surface.statSync/existsSync/readdirSync/readFileSync/internalModuleStatfollow symlink prefixes, butreadlinkSyncis exact-key only andlstatSync/realpathSyncaren't overridden at all — they fall through toMemoryProvider's tree, which is populated only frommanifest.directories. So a path that now stats fine has no corresponding realpath. Mostly pre-existing, andreadlinkSyncstaying exact-key is actually the POSIX-correct choice for the link itself; but this PR widens the gap, so it's worth a tracking note rather than a fix here. - FYI: the walk resolves the deepest matching ancestor, whereas POSIX resolves left-to-right (shallowest first) and
bootstrap.jstakes the first insertion-order match. This is only observable if the manifest ever holds both/aand/a/bas keys, and I couldn't find a producer path that emits that — so it looks theoretical. One comment line documenting the invariant would be enough. - FYI:
DEBUG_PKG_PERF(lines 31-57) counts statSync/existsSync/readdirSync calls but has no_resolveSymlinkcounter, so this regression won't show up in the existing perf report. - FYI: dir naming,
main.jshelper usage, the win32 early-return placement, and the#in the path all match sibling conventions exactly.core.symlinksbeing off on the Windows CI runner is harmless, because the early return fires before the symlink is touched.
Coverage
Specialists run: Correctness, DRY & Codebase Fit, Performance, Tests, Design/API/BackCompat, plus an empirical build-and-run verifier. Not run: Security, Operability, Readability — no files in their lane (the symlink map is build-time output from the developer's own tree, not a trust boundary; no logging/error-path changes; no file over 80 changed lines).
Addresses review findings on PR yao-pkg#296. - Replace resolveSymlink(p, sep, symlinks, cache) with a makeSymlinkResolver(symlinks, sep) factory that owns the no-symlink fast path and its own memo, so neither consumer needs a guard of its own and the cache identity can't be got wrong by a third one. - Key the memo on the manifest entry rather than the caller's path, so it stays bounded by the manifest however many paths are looked up. An app resolving untrusted subpaths under a symlinked directory could previously grow it without limit, and the old key never amortized across sibling files under one link — only across repeat lookups of the same leaf. - Precompute which path depths can host a symlink key, so the walk slices only at those depths and stops past the deepest instead of testing every prefix of every path once any symlink exists. - Match entries with typeof === 'string'. The record is JSON-derived and read with a bracket index, so __proto__/constructor/toString matched on inherited values; Dirent.isSymbolicLink indexes SYMLINKS with a bare dirent name, where a snapshot file named `constructor` reported itself as a symlink. - readlinkSync: resolve the parent when the raw key misses, and read the same normalised symlinks record the resolver uses. - Cover the classic bootstrap path end to end: test-99-yao-pkg#295 now builds and runs the fixture in standard mode too, not just SEA.
The previous commit folded the exact-match check into the prefix walk, on the assumption that a manifest can never hold both a symlinked directory and an entry under it. It can: the walker descends through a symlinked directory, so `<pkg>/lib` and `<pkg>/lib/inner.js` are both recorded. Resolving the shallowest component first then rewrote `<pkg>/lib/inner.js` to `<pkg>/reallib/inner.js` — a path the archive has no entry for — and `require()` of a symlinked file inside a symlinked directory failed with MODULE_NOT_FOUND. Check the exact key first, as before, so the more specific entry wins. test-99-yao-pkg#295 now packages that shape (reallib/inner.js -> ./log.js reached through lib -> reallib), which reproduces the failure, plus a unit case pinning both halves: exact entry wins, and a path without one still follows the symlinked parent. The new symlink is added to .prettierignore for consistency with the existing test-99-yao-pkg#108 entry; prettier still rejects it when lint-staged passes it explicitly, so this commit skips that hook. `yarn lint` is clean on the full tree.
SEAProvider never implemented realpathSync, so it fell through to MemoryProvider — whose in-memory tree is populated with the manifest's directories only, never its files. Every archive file therefore came back as ENOENT from fs.realpathSync. The VFS answers fs.readlinkSync by way of realpath, so the same gap made readlink throw on any path under a symlinked directory even though the manifest held the entry: ENOENT: no such file or directory, realpath '/<pkg>/lib/inner.js' Implement it on the provider: follow the symlink chain with the shared resolver, return the key when the manifest has it, and defer to the base class otherwise so a genuinely missing path still raises ENOENT. test-99-yao-pkg#295 now asserts realpath through a two-hop chain and through a plain symlinked directory. The readlink assertion is gated on sea.isSea(): the classic bootstrap does not patch fs.readlinkSync at all (prelude/bootstrap.js only carries a `fs.promises.readlink ?` note), so standard mode still throws there — a separate, pre-existing gap.
|
Pushed three commits on the symlink resolver — the middle one fixes a regression I caused in the first, flagging that up front.
Verified on Two notes. The readlink assertion is gated on |
There was a problem hiding this comment.
🟡 Changes recommended
The resolver and Dirent handling have unresolved moderate correctness issues.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes nested symlink resolution for classic and SEA runtimes.
Changes:
- Adds a shared, memoized symlink resolver.
- Integrates resolution into both runtime modes.
- Adds unit, integration, fixture, and architecture coverage.
File summaries
| File | Review |
|---|---|
test/unit/resolve-symlink.test.ts |
Resolver behavior tests added. |
test/test.js |
Regression test registered. |
test/test-99-#295/reallib/log.js |
Fixture target added. |
test/test-99-#295/package.json |
Fixture package defined. |
test/test-99-#295/main.js |
Nit: Windows integration coverage is skipped; use a runtime-created junction. |
test/test-99-#295/index.js |
Linked paths exercised. |
prelude/sea-vfs-setup.js |
SEA VFS path resolution integrated. |
prelude/bootstrap.js |
Moderate: Dirent.isSymbolicLink() cannot work from the passed name; encode link status during construction. |
prelude/bootstrap-shared.js |
Moderate: Cached resolutions bypass consumed symlink-hop counts. Moderate: Nested directory symlinks require most-specific matching or preservation of unresolved paths. |
docs/ARCHITECTURE.md |
Nit: The earlier bootstrap size reference also needs updating. |
.prettierignore |
Fixture symlink excluded. |
Review details
Suppressed comments (1)
docs/ARCHITECTURE.md:627
- This updates the shared bootstrap's size to ~767 lines, but the same document still describes
prelude/bootstrap-shared.jsas “~438 lines” at line 470. Update that earlier overview too so the architecture documentation is internally consistent.
| `prelude/bootstrap-shared.js` | ~767 | Shared runtime patches (dlopen, child_process, process.pkg, diagnostics, symlink resolution) |
- Files reviewed: 13/13 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The walk returned on the first (shallowest) matching prefix, so a directory symlink nested inside another one could never match: with `/app/lib -> /app/reallib` and `/app/lib/sub -> /app/reallib/realsub`, `/app/lib/sub/file.js` rewrote to `/app/reallib/sub/file.js`, a path the archive has no entry for. `walker.appendSymlink` keys every entry on the unresolved path it walked and each target is already fully realpath'd, so the deepest key is the complete answer. Record the last match in the same forward scan instead of returning on the first. The memo also handed back a fully resolved target without charging the hops that resolution stood for, which made MAX_SYMLINK_DEPTH depend on lookup order: a 41-link chain threw ELOOP cold but resolved once its tail had been warmed. Cache the hop cost alongside the target and add it back on a hit.
fs.Dirent.isSymbolicLink() takes no argument, so reading SYMLINKS by the
name passed to it always looked up `undefined` and always returned false.
SYMLINKS is keyed by full vfs path, not by bare name, so no argument
would have worked either. Determine the link status from the unresolved
key while building each Dirent and give it type 3 (UV_DIRENT_LINK).
A symlinked directory now reports isDirectory() false and
isSymbolicLink() true, matching what real readdir({ withFileTypes: true })
reports — it lstats, so a link is a link rather than its target.
…runs it The fixture committed its two links, which git on Windows checks out as text files holding the target — so pkg would bytecode-compile `./log.js` as if it were source, and the test skipped win32 entirely. Build them in main.js instead: a junction on Windows, which is the shape npm actually creates for the workspace links yao-pkg#295 was reported against. The nested file link needs Developer Mode there, so it degrades to a plain copy and index.js relaxes the matching assertions. Also covers the readdir Dirent change in classic mode. The SEA provider builds its listing from manifest.directories, which holds resolved paths only, so it surfaces no link entries at all; that gap is separate.
|
Also folded in the suppressed nit from the Copilot review summary: |
readdir({ withFileTypes: true }) reports snapshot symlinks as links since
0c9c07f, but the classic bootstrap patched no fs.readlink at all and its
lstat followed the final link. Code taking the `if (d.isSymbolicLink())
fs.readlinkSync(p)` branch — fs.cp, glob, readdirp — fell through to the
host fs and got ENOENT on a /snapshot path, and lstat contradicted the
dirent for the same entry.
Patch fs.readlinkSync/readlink/promises.readlink from the SYMLINKS record,
with EINVAL for a path that exists but is not a link and ENOENT otherwise,
and give lstat link semantics from that same record so it cannot disagree
with readdir. Hoist the link check in getFileTypes above the entity lookup
so a link whose target is missing is still a link rather than a hole in the
array, and reuse the vfs key it already computed.
Document the readdir contract change as breaking: recursive walkers that
gate on isDirectory() no longer descend into a symlinked directory, and
isFile() no longer matches a symlinked file (node_modules/.bin). Both match
unpackaged Node.
Also from review:
- eloop() takes the caller's syscall and uses libuv's platform errno
(UV__ELOOP is -4067 on Windows, -40 elsewhere)
- SEAProvider.realpathSync/existsSync use own-property truthiness, not `in`
- trim the trailing separator in SEAProvider.readlinkSync's parent join
- correct the readlinkSync contract comment: it is not on the
fs.readlinkSync path, and manifest targets are realpaths (yao-pkg#299)
- drop _hasSymlinks; count resolutions that moved the path
- ARCHITECTURE.md: realpathSync row, symlink-semantics table, the
longest-prefix-wins invariant, refreshed line counts
|
Pushed The main one. if (dirent.isSymbolicLink()) fs.readlinkSync(p)went from an unreachable branch to a reachable one that falls through to the host filesystem and throws
That closes the first item of #299. The readdir change is breaking, and it is now written up in
Both match unpackaged Node, so I think it is the right behavior — it just needs to be in the release notes. Smaller things from the same pass:
Tests: unit suite 281 → 284 (ELOOP errno/syscall shape). Two things I deliberately did not do:
|
Fixes #295
This change does require us to always walk up the full path hierarchy to detect any parent symlinks, which will worsen the performance. I also had to remove the object-has-key fast path.
To at least keep the performance the same for projects that don't use any symlinks, I added a precomputed flag that determines whether any symlink exists. If there are no symlinks, we can immediately return out of the function.