fix(prelude): resolve and report symlinks consistently in classic and SEA modes by mpotthoff · Pull Request #296 · yao-pkg/pkg

mpotthoff · 2026-08-20T23:08:54Z

Fixes #295
Closes #305

Important

@roberts_lando/vfs is pinned to a git commit, not a release. The SEA half needs
robertsLando/vfs#4: installFsPatches never routed
fs.lstat/fs.readlink to the provider, and its existsSync gate flattened a provider's ELOOP into
ENOENT. package.json currently installs that PR's exact commit so CI can run the suite against the real
dependency — and it is green. This must be swapped for a published range before merge, because a git
dependency would make @yao-pkg/pkg consumers need GitHub access to install.

The bug

lib/walker.ts records one manifest entry per symlink (symLinks[file] = realFile), so in an npm-workspace or pnpm tree node_modules/@t/lib has a key but node_modules/@t/lib/package.json does not. The traditional bootstrap already matched symlink prefixes; SEA's resolver was exact-key only, which is why the same project packaged fine in standard mode and failed with ERR_MODULE_NOT_FOUND under SEA.

The fix

One resolver, makeSymlinkResolver() in prelude/bootstrap-shared.js, used by both bootstraps. It walks parent components the way POSIX does, so a link at node_modules/@t/lib also resolves everything beneath it.

Longest prefix wins, and the scan slices only at separator offsets, so /snapshot/foo cannot match /snapshot/foobar.
An empty symlinks record yields the identity function, so a symlink-free binary pays nothing. Otherwise the resolver precomputes which path depths can host a key and memoises each key's resolved target, keyed by manifest entry rather than by caller path so the memo stays bounded by the manifest.
A manifest cycle raises ELOOP (libuv's platform errno) instead of hanging startup, and cached chains are charged the hops they stand for so the depth bound does not depend on lookup order.
ELOOP is contained at fs.existsSync and fs.internalModuleStat in both modes — neither may throw — and everywhere else it names the syscall that hit it and the caller's path rather than the internal VFS key.

Reporting symlinks, not just resolving them

Resolution was only half of it. Traditional mode reported every snapshot entry as a plain file or directory, and SEA could not report links at all. Both now answer from their own symlink record:

	Traditional	Enhanced SEA
`readdir({ withFileTypes: true })`	link reports as a link	same, from `manifest.symlinks`
`lstat`	describes the link itself, `S_IFLNK` type bits	same
`readlink`	target, `EINVAL` for a non-link	same
`realpath`	follows the chain	same, in the platform's path form

Dirent and the stat link-semantics helper live in bootstrap-shared.js alongside the resolver, so the two modes cannot drift on what a link looks like.

Three bugs were fixed in @roberts_lando/vfs rather than worked around here, because VirtualFileSystem already delegated the right methods to the provider — only the patch layer didn't call them. All in robertsLando/vfs#4:

fs.lstat went through findVFSForFsStat, which calls statSync and therefore follows the link.
fs.readlink went through findVFSForRealpath, which never reached the provider, so EINVAL was unreachable and every ordinary file looked like a link pointing at itself.
The existsSync gate in front of each real operation flattened any provider error into "not found", so a symlink cycle reported ENOENT instead of ELOOP from stat, open, readdir and realpath.

That PR also maps an absolute link target back into the mounted namespace, including for encoding: 'buffer'.

fs.realpathSync in SEA also answered in the VFS's POSIX form, so on Windows it returned /snapshot/app/x.js where __filename is C:\snapshot\app\x.js — that is #305, fixed at the same seam as the other win32 patches.

Behaviour change worth knowing about

readdir({ withFileTypes: true }) previously reported every snapshot entry as a plain file or directory: Dirent.isSymbolicLink() took an argument it is never called with, so it always returned false. It now reports links as links, which is what Node does outside a packaged binary — a hidden bug, fixed, rather than a deliberate contract change. Still, two consequences for packaged apps whose snapshot contains symlinks:

Recursive walkers that gate descent on isDirectory() and skip links by default (glob, fast-glob, readdirp, fs.cp with recursive) no longer descend into a symlinked directory unless told to follow links.
A filter like entries.filter((e) => e.isFile()) no longer matches a symlinked file — node_modules/.bin/* is the common case.

fs.readlink and fs.lstat were patched in the same change so code taking the isSymbolicLink() branch is served rather than falling through to the host filesystem.

Shared, so the modes cannot drift

The two preludes previously grew their own copies of everything symlink-related, which is the root-cause class of #295 itself. prelude/bootstrap-shared.js now owns the resolver (makeSymlinkResolver, plus resolveKey.parent for the step readlink and lstat take), Dirent, asSymlinkStat, the readlink encoding contract, the libuv errno table, and makeFsError. Each mode keeps only what is genuinely its own — traditional ENOENT still carries pkg's "recompile adding it as asset" guidance, which test-50-not-found-wording asserts.

Tests

test/unit/resolve-symlink.test.ts — table-driven against the real makeSymlinkResolver: loops, chains, longest-prefix, prefix boundaries, depth billing, memoisation, the ELOOP shape, inherited Object.prototype keys, and resolveKey.parent including its win32 separator and ELOOP path.
test/unit/dirent-shared.test.ts — the shared Dirent and stat helper.
test/test-99-#295/ — end to end in both SEA and standard mode, building its symlink fixture at test time so Windows runs it too. It asserts one contract for both modes rather than branching on isSea(): readdir of the linked directory, Dirent.parentPath being joinable, the 'buffer' encoding, the S_IFLNK type bits, the win32 realpath form, EINVAL naming the caller's path, and the callback and promise forms of readlink/lstat/readdir — including that an async readlink on a non-link reaches the callback rather than throwing.

Unrelated: `test-80` pin

node-opcua 2.184.0 (published 2026-09-14) went ESM ("type": "module", import.meta.dirname), which esbuild's CJS bundle resolves to undefined — node-opcua-nodesets then calls path.join(undefined, ...). That breaks test-80-compression-node-opcua on main as well, and is pinned to 2.181.0 here so this PR's CI can be read. Unpinning needs an esbuild config that emits ESM.

codecov · 2026-08-25T06:38:26Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.45%. Comparing base (36f18af) to head (85eb435).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #296      +/-   ##
==========================================
- Coverage   87.23%   86.45%   -0.79%     
==========================================
  Files          23       23              
  Lines        7929     7929              
  Branches     1214     1214              
==========================================
- Hits         6917     6855      -62     
- Misses       1005     1066      +61     
- Partials        7        8       +1

see 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

robertsLando

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 _resolveSymlink for 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-#295 is a host-only SEA test (runSeaHostOnly, ignores the target arg) but wasn't added to the npmTests array. It therefore falls through the **/main.js glob and builds a SEA binary in both test:22 and test:24, each matrixed over 3 OSes, while never running in test:host — the exact redundancy the comment above npmTests exists to prevent. Adding 'test-99-#295' to that list fixes it. (test-93-sea-compress has 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 advertises internalModuleStat() O(1) manifest hash lookup (no tree walk), statSync() O(1) and existsSync() 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: _resolveSymlink is now applied asymmetrically across the provider surface. statSync / existsSync / readdirSync / readFileSync / internalModuleStat follow symlink prefixes, but readlinkSync is exact-key only and lstatSync / realpathSync aren't overridden at all — they fall through to MemoryProvider's tree, which is populated only from manifest.directories. So a path that now stats fine has no corresponding realpath. Mostly pre-existing, and readlinkSync staying 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.js takes the first insertion-order match. This is only observable if the manifest ever holds both /a and /a/b as 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 _resolveSymlink counter, so this regression won't show up in the existing perf report.
FYI: dir naming, main.js helper usage, the win32 early-return placement, and the # in the path all match sibling conventions exactly. core.symlinks being 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.

robertsLando · 2026-09-04T09:11:27Z

Pushed three commits on the symlink resolver — the middle one fixes a regression I caused in the first, flagging that up front.

4fcf2cb — resolveSymlink(p, sep, symlinks, cache) becomes a makeSymlinkResolver(symlinks, sep) factory owning the no-symlink fast path and its own memo, so neither consumer needs a guard:

The memo is keyed on the manifest entry rather than the caller's path, so it stays bounded by the manifest however many paths are looked up. The old key could grow without limit for an app resolving untrusted subpaths under a symlinked directory, and it never amortized across sibling files under one link — only across repeat lookups of the same leaf.
The resolver precomputes 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. This is what recovers the cost you flagged in the PR description for projects that do use symlinks.
Entries are matched with typeof === 'string'. The record is JSON-derived and read with a bracket index, so __proto__/constructor/toString matched on inherited values. In Dirent.isSymbolicLink this was already reachable — it indexes SYMLINKS with a bare dirent name, so a snapshot file named constructor reported itself as a symlink.
test-99-#295 now runs the fixture in standard mode too, so the rewritten bootstrap.js path has end-to-end coverage.

3f8a1bc — fixes a regression from 4fcf2cb. I folded the exact-match check into the prefix walk, on the assumption stated in the unit test comment that a manifest can't 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 shallowest-first rewrote <pkg>/lib/inner.js to <pkg>/reallib/inner.js — no archive entry — and require() of a symlinked file inside a symlinked directory failed with MODULE_NOT_FOUND. Exact key is checked first again, as your original code did.

6df8da1 — a related gap the new fixture surfaced. 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 came back ENOENT from fs.realpathSync, symlinked or not. The VFS also answers fs.readlinkSync by way of realpath, so both threw on paths under a symlinked directory even though the manifest held the entry. Implemented on the provider using the shared resolver, deferring to the base class so genuinely missing paths still raise ENOENT.

test-99-#295 packages the failing shape — reallib/inner.js -> ./log.js reached through lib -> reallib — and asserts require, realpath and readlink across it. It reproduces both failures against their respective parent commits.

Verified on 6df8da1: yarn lint clean, 277/277 unit, and e2e test-99-#295, test-50-symlink, test-10-pnpm, test-11-pnpm, test-80-compression-node-opcua, test-89-sea-fs-ops, test-85-sea-enhanced, test-86-sea-assets.

Two notes. The readlink assertion is gated on sea.isSea() — standard mode doesn't patch fs.readlinkSync at all, which is pre-existing and unrelated to this PR; filed separately. And the new reallib/inner.js symlink is in .prettierignore next to the existing test-99-#108 entry, but prettier still rejects symlinks passed explicitly, so lint-staged fails on it and those commits skip the hook.

Copilot

🟡 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.js as “~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.

robertsLando · 2026-09-07T09:23:20Z

Also folded in the suppressed nit from the Copilot review summary: docs/ARCHITECTURE.md had bootstrap-shared.js at ~438 lines on line 470 and ~767 on line 627. Both now say ~763, the actual count (410e2f7).

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

robertsLando · 2026-09-07T15:14:08Z

Pushed ffd51e0 — fixes from a review pass on this branch. Sorry for landing directly on the PR branch; the changes are all against code this PR introduces, so splitting them out would have left 296 unmergeable on its own. Happy to move any of it if you'd rather.

The main one. 0c9c07f made readdir({ withFileTypes: true }) report snapshot symlinks as links — correct, and a real bug fix, since Dirent.isSymbolicLink() took an argument it is never called with and so always returned false. But the classic bootstrap patched no fs.readlink at all, and its lstat follows the final link. So the usual pairing

if (dirent.isSymbolicLink()) fs.readlinkSync(p)

went from an unreachable branch to a reachable one that falls through to the host filesystem and throws ENOENT on a /snapshot/... path — fs.cp, glob, readdirp all do this. And lstat contradicted the dirent for the same entry.

ffd51e0 patches fs.readlinkSync / fs.readlink / fs.promises.readlink from the SYMLINKS record (EINVAL for a path that exists but isn't a link, ENOENT otherwise) and gives lstat link semantics from that same record, so readdir and lstat cannot disagree. It also hoists the link check in getFileTypes above the entity lookup — a link whose target is missing from the snapshot is still a link, and was previously becoming a literal undefined in the readdir array.

That closes the first item of #299.

The readdir change is breaking, and it is now written up in docs/ARCHITECTURE.md rather than only in a test comment. Two consequences for apps whose snapshot has symlinks (pnpm and workspace trees, plus node_modules/.bin):

recursive walkers that gate descent on isDirectory() and skip links by default no longer descend into a symlinked directory
entries.filter((e) => e.isFile()) no longer matches a symlinked file

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:

eloop() now takes the caller's syscall and uses libuv's platform errno — UV__ELOOP is -4067 on Windows, -40 elsewhere (uv/errno.h), where it was hardcoded to -40 / 'stat'
SEAProvider.realpathSync and existsSync use own-property truthiness instead of in, which walks the prototype chain of a JSON-derived object; not reachable today since keys are absolute, but the siblings all guard it
the parent join in SEAProvider.readlinkSync trims a trailing separator, so it cannot build //name and silently miss
that method's comment said it returns the target "verbatim". It is not on the fs.readlinkSync path at all — the VFS polyfill answers readlink through realpathSync — and manifest targets are full realpaths, not raw link bodies. Comment corrected rather than the code; that is fs.readlinkSync on snapshot paths: missing in standard mode, realpath-shaped in SEA #299's second and fourth items, and the routing half belongs upstream in @roberts_lando/vfs
dropped _hasSymlinks; the perf counter now counts resolutions that actually moved the path, which is honest on symlink-free binaries without a separate guard
documented the longest-prefix-wins invariant: it agrees with POSIX's leftmost-first walk only because every walker-recorded target is already a full realpath, so no target component can itself be a key

Tests: unit suite 281 → 284 (ELOOP errno/syscall shape). test-99-#295 now asserts classic-mode readlink and lstat().isSymbolicLink() against readdir, and checks readdir works in SEA mode too. Both modes pass locally on node22-linux-x64.

Two things I deliberately did not do:

the EINVAL assertion is classic-mode only. In SEA a non-link returns a resolved path instead of throwing, because the VFS polyfill never consults the provider — fs.readlinkSync on snapshot paths: missing in standard mode, realpath-shaped in SEA #299's third item, upstream.
SEA's readdir still does not surface link entries, since its listing comes from manifest.directories, which holds resolved paths only. The test comment claimed this was "tracked separately" but no issue covers it; I removed the claim rather than invent a number. Worth opening one.

spotandjake · 2026-09-16T18:28:41Z

I've been using this PR locally for a project where I was running into similar issues with npm workspaces, and the changes here have been working perfectly for me so far.

ELOOP containment. resolveSymlink() throws, and it sits under fs.existsSync and fs.internalModuleStat in both modes — neither may throw (libuv swallows every errno for exists; internalModuleStat answers a negative errno). In SEA this reached further: @roberts_lando/vfs calls the provider's existsSync outside its try in findVFSForRealpath, so the throw escaped fs.realpathSync and fs.readlinkSync too. Contain it at all four boundaries. ELOOP diagnostics. Classic mode passed no syscall at any call site, so every cycle reported 'stat' whatever the caller was; the resolver also reported the raw vfs key as the path, which under DOCOMPRESS is base36. Thread the caller's syscall and path through findVirtualFileSystemEntry and the SEA provider, and mark the error pkg-originated like every sibling factory. fs.realpathSync in SEA answered in the VFS's POSIX form, so on Windows it returned /snapshot/app/x.js where __filename is C:\snapshot\app\x.js. Convert back at the same seam the other win32 patches use. Also: - readlink honours its encoding option; 'buffer' returns a Buffer - lstat's mode carries S_IFLNK, so mode-sniffing consumers (tar, archiver, fs.cp) agree with isSymbolicLink() - the SEA manifest lookups use typeof, not truthiness — an inherited key is a function, a real entry is a stat record, so this guard actually holds - DEBUG_PKG=2 traces readlink alongside realpath and lstat - tests: prefix-boundary (/snapshot/foo must not match /snapshot/foobar), the ELOOP path and pkg marker, the win32 realpath form, SEA's actual readlink contract rather than a skip, and inherited manifest keys - ARCHITECTURE.md names the SEA "cannot report symlinks" gap as one defect rather than three table rows, and the line counts are back in sync - .gitignore covers the fixture files test-99-yao-pkg#295 builds at test time test-80 pins node-opcua to 2.181.0: 2.184.0 went ESM ("type": "module", import.meta.dirname), which esbuild's CJS bundle resolves to undefined. That break is unrelated to this PR and reaches main too.

Resolution through links already worked in both modes; reporting them did not. `lstatSync(link).isSymbolicLink()` was true in a traditional binary and false in a SEA one built from the same source, `readdir({ withFileTypes })` handed back strings instead of Dirents, and `fs.readlinkSync` never raised EINVAL because it never reached the provider. The manifest already carries what was missing: `symlinks` is the unresolved key -> target map, the same record the resolver walks. No schema change. - SEAProvider.lstatSync gives the target's stat link semantics, and readdirSync honours withFileTypes, typing each entry from that record. - SEAProvider.readlinkSync answers EINVAL for a path that is present but not a link and ENOENT otherwise, matching classic mode. - @roberts_lando/vfs routes fs.lstat through findVFSForFsStat (which follows the link) and fs.readlink through findVFSForRealpath (which never reaches the provider), so sea-vfs-setup.js re-points both, plus their callback and promise forms, at the provider. VirtualFileSystem.readlinkSync returns a provider-relative path, so the mount prefix and the platform path form go back on there, next to realpathSync's own conversion. Dirent and the stat link-semantics helper move into bootstrap-shared.js, so the two modes cannot drift on what a link looks like — the same reason makeSymlinkResolver lives there. test-99-yao-pkg#295 now asserts one contract for both modes instead of branching on isSea, and the shared Dirent and asSymlinkStat get unit coverage.

`VirtualFileSystem.lstatSync` and `.readlinkSync` already delegate to the provider; `installFsPatches` just never called them. - `fs.lstat*` went through `findVFSForFsStat`, which calls `statSync` and so follows the link — `isSymbolicLink()` could never be true inside a VFS. - `fs.readlink*` went through `findVFSForRealpath`, which never reaches the provider at all. `realpathSync` returns the path for anything that exists, so every ordinary file looked like a symlink pointing at itself and the providers' own `EINVAL` was unreachable. Adds `findVFSForFsLstat` and `findVFSForReadlink` and points the sync, callback and promise forms of both calls at them. Providers that do not model links are unaffected: `Provider.lstatSync` defaults to `statSync`. `VirtualFileSystem.readlinkSync` also handed back a provider-relative path while `realpathSync` right above it mapped its result into the mounted namespace. An absolute target now gets the same treatment; a relative one is returned verbatim, since that is the link body POSIX answers with. Reported downstream as yao-pkg/pkg#296, where SEA-mode binaries disagreed with traditional ones about `lstatSync(link).isSymbolicLink()` for the same source.

Round 2 review of the parity commit. The ELOOP containment added in the previous round was not uniform. fs.open, fs.readFile, fs.readdir, fs.stat and fs.access all call their *FromSnapshot helper synchronously, and none of those wrapped the entity lookup — so a cyclic manifest escaped as a synchronous throw from a callback API that Node guarantees never throws synchronously. Each helper already funnels errors through cb2, which is rethrow for sync callers, so routing the lookup's throw there fixes async and leaves sync behaviour identical. internalModuleReadJSON gets the same miss-not-throw treatment as internalModuleStat. The SEA fs re-patch is gone: the lstat and readlink routing it worked around is fixed upstream in robertsLando/vfs#4, where it belongs — VirtualFileSystem already delegated both to the provider, installFsPatches just never called them. **This PR needs a vfs release containing that fix.** readlink's encoding handling moves into bootstrap-shared and is used by both modes, so fs.readlinkSync(link, 'buffer') answers a Buffer either way. Dirent carries parentPath (and the `path` alias Node deprecated but still ships), because path.join(d.parentPath, d.name) is the documented way to use withFileTypes and SEA returned these where it used to return strings. SEA readdir looked entries up under the *resolved* directory key, but manifest.symlinks is keyed by the unresolved path the walker walked — so a link inside a symlinked directory reported as a plain file in SEA and a link in classic. It now probes both. Provider errors and resolver ELOOPs report the caller's /snapshot path rather than the mount-relative key the VFS hands in. test-99-yao-pkg#295 covers all of it: readdir of the linked directory, parentPath being joinable, the buffer encoding, and the EINVAL error naming the caller's path.

The SEA symlink reporting in this PR needs robertsLando/vfs#4, which fixes installFsPatches to route fs.lstat and fs.readlink to the provider. Until that release exists the install fails outright, which is the intent: the previous range would quietly install a vfs whose lstat follows the link.

Round 3 review. Three more places let the resolver's ELOOP escape a callback API synchronously, the class round 2 fixed elsewhere: getFileTypes (per directory entry, and in fs.readdir it runs inside payloadFile's real I/O completion handler), fs.realpath's callback form, and the withFileTypes step in async readdir. A cycle reachable only through an entry's parents is enough — the entry itself need not be a key. readlink's parent-resolution step moves into makeSymlinkResolver as resolveKey.parent and both modes drive it. It was written twice with its own no-double-separator join rule, classic had none at all, and ARCHITECTURE.md's parity table claimed "Same" for the readlink row, which the code did not back. It does now. The SEA provider had three key policies over one record — lstatSync read only the unresolved key, _direntType both, readlinkSync retried the parent — so for a link recorded under a followed parent, readdir called it a link, readlink returned a target and lstat followed it. One _linkTarget helper now answers for all three. toCallerPath was computed on every _resolveSymlink call but is only ever read when ELOOP throws, which put a string allocation on the ~30K-call startup path that the resolver's own doc promises is allocation-free. It moves into the catch. Also: the typeof guard's comment claimed an inherited hit is always a function, which is wrong for __proto__ (typeof gives 'object'); it now says what actually makes the lookup safe, which is that keys are always '/'-prefixed. classic readlink's EINVAL check uses typeof like its siblings. DEBUG_PKG=2 traces fs.promises.readlink. Dirent's unit test covers parentPath and its `path` alias.

installFsPatches probes with existsSync before the real call, and VirtualFileSystem.existsSync catches everything to keep its own boolean contract — so a provider that fails the probe for a reason worth reporting had it flattened into "not found" and the caller saw ENOENT. A symlink cycle is the case that matters: the provider answers ELOOP, the gate read false, and stat/open/readdir/realpath all reported a missing file instead. probeSync() keeps the reason alongside the boolean; the four gates that guard a real operation re-raise it rather than synthesising ENOENT. fs.existsSync is unchanged — it still answers false, which is what libuv does. Reported downstream as yao-pkg/pkg#296.

… guard The follow-ups from round 3, taken here rather than filed. Errors: bootstrap-shared owns the libuv errno table — sea-vfs-setup's ENOENT was not Windows-aware at all — and makeFsError gives every snapshot error the same shape, including the `pkg` marker the SEA side never set. The *messages* stay each mode's own on purpose: traditional ENOENT carries pkg's "recompile adding it as asset" guidance and test-50-not-found-wording asserts it. Link lookups: "is this a link, and to what" was answered three different ways per mode. snapshotLinkTarget (classic) and _linkTarget (SEA) now answer for readdir, readlink and lstat alike, both consulting the unresolved key first and the parent-resolved one as fallback. ELOOP: the try/catch was pasted into five shims and would have been forgotten by the sixth. findEntryOr owns it, and returns a sentinel so a caller cannot report its own ENOENT on top of an error already delivered. lstat now reports a link's size as the length of its target and zero blocks, which is what POSIX lstat does — the mode bits alone still left archivers sizing a link entry from the target's stat. SEA realpathSync accepts and honours `options`; it narrowed the base signature while the VFS passes them through, so `fs.realpathSync(p, 'buffer')` answered a string next to a readlinkSync that got it right. The existsSync gate that flattened ELOOP into ENOENT for stat/open/readdir/ realpath in SEA is fixed upstream (robertsLando/vfs#4, probeSync), not worked around here. Tests: resolveKey.parent gets its own unit coverage — including the win32 separator and the ELOOP path — and test-99-yao-pkg#295 now drives the callback and promise forms of readlink, lstat and readdir, plus that async readlink on a non-link reaches the callback rather than throwing. That absence is why two ELOOP bugs survived to the third review round.

robertsLando · 2026-09-18T09:38:45Z

Review rounds — what was decided, and what was left

Four review rounds (8 specialist lanes each) over ffd51e0…205bc25. CI is green, 26/26. Every Blocker and Major is closed. This comment records the calls that aren't visible in the diff.

One thing left before merge

@roberts_lando/vfs is installed from the robertsLando/vfs#4 commit rather than a release, so the suite could run against the real dependency — that PR fixes installFsPatches, which never routed fs.lstat/fs.readlink to the provider and whose existsSync gate flattened a provider's ELOOP into ENOENT. Swap it for a published range before merge: a git dependency would make @yao-pkg/pkg consumers need GitHub access to install.

Pinning it is also what surfaced the last bug. Up to that point the dependency could not install at all, so nothing had ever been run; the first real CI pass caught resolveSymlink.parent is not a function on every binary without symlinks — the empty-manifest fast path returns a bare identity function, and the parent-resolution step had only been attached to the real resolver. Every local symlink test builds a fixture that has symlinks, so they all took the other branch.

Decided

No BREAKING CHANGE: trailer for the readdir/lstat change. The old behaviour — Dirent.isSymbolicLink() always returning false because it took an argument it is never called with — was a hidden bug, not a contract. The consequences are written up in the description and in docs/ARCHITECTURE.md regardless.
Routing fixed upstream, not shimmed here. VirtualFileSystem already delegated lstatSync/readlinkSync to the provider; only the patch layer didn't call them. An earlier revision of this PR carried a local re-patch; it was removed once the upstream fix landed.
Error messages stay per-mode. Traditional ENOENT carries pkg's "recompile adding it as asset" guidance and test-50-not-found-wording asserts it; the SEA provider uses Node's wording. Only the error shape is shared (makeFsError).

Noted, not fixed — worth separate issues

Symlink resolver performance. The memo is keyed by manifest entry rather than by caller path, so a lookup under a resolved link re-walks the prefix chain; lstat derives the vfs key twice on the non-link path; and the resolver is rebuilt per worker thread in both modes. All three want profiling on a real pnpm tree before anyone changes them — the current shape is a large win over the pre-PR O(symlink-count) scan.
test-50-corrupt-executable is a layout lottery. It pokes 0x02 at six fixed offsets from EOF and needs at least one to land in syntactically significant prelude source. It failed on Windows only at f62213a and recovered at f191bed with no code change; building win-x64 at HEAD and reading each offset confirmed all six landed in live syntax there. Any prelude size change re-rolls it.
test-80 pins node-opcua to 2.181.0. 2.184.0 went ESM ("type": "module", import.meta.dirname), which esbuild's CJS bundle resolves to undefined. Unrelated to this PR and it breaks main too. node-opcua-crypto is still on a floating range and can drag the same break in.
readlink answers a resolved realpath, not the POSIX link body. Both modes do, consistently, because that is what the walker records. A relative link therefore comes back absolute, which rewrites relative links in fs.symlinkSync(fs.readlinkSync(a), b) tree copies. Closing it means recording the raw link body at build time.

^0.3.4 does not exist yet, so the previous commit left CI unable to install at all — package.json asked for a version the registry has never had and the lockfile still carried the ^0.3.3 descriptor. Nothing could actually be run against the SEA changes. Pinning the exact commit that carries robertsLando/vfs#4 (lstat/readlink routing, Buffer-aware link-target mapping, probeSync) gets the suite running against the real dependency instead of a hand-patched node_modules, and the lockfile records the codeload tarball for that SHA. TEMPORARY. A git dependency cannot ship: `@yao-pkg/pkg` consumers would need GitHub access to install it. Swap this for the published range before release.

makeSymlinkResolver hands back a bare identity function when the manifest has no symlinks, and the parent-resolution step added for readlink and lstat was attached only to the real resolver. Every binary without symlinks — which is most of them — therefore died with "resolveSymlink.parent is not a function" the first time anything called readlink or lstat. Only CI caught this: every local symlink test builds a fixture that has symlinks, so they all take the other branch. test-50-fs-runtime-layer and test-99-vercel#1505 failed on all six platforms.

fix(sea): files inside symlinks are not resolved correctly (yao-pkg#295)

843326b

robertsLando reviewed Aug 25, 2026

View reviewed changes

mpotthoff and others added 5 commits August 25, 2026 19:42

Address review comments

deeee73

Merge branch 'main' into 295-resolve-symlinks

b5b44b7

robertsLando mentioned this pull request Sep 4, 2026

fs.readlinkSync on snapshot paths: missing in standard mode, realpath-shaped in SEA #299

Open

robertsLando requested a balanced review from Copilot September 4, 2026 10:15

Copilot started reviewing on behalf of robertsLando September 4, 2026 10:16 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread prelude/bootstrap-shared.js Outdated

Comment thread prelude/bootstrap-shared.js Outdated

Comment thread prelude/bootstrap.js Outdated

Comment thread test/test-99-#295/main.js Outdated

robertsLando added 4 commits September 7, 2026 10:13

docs: sync the two bootstrap-shared.js line counts

410e2f7

robertsLando added 2 commits September 18, 2026 10:40

robertsLando changed the title ~~fix(sea): files inside symlinks are not resolved correctly (#295)~~ fix: resolve and report symlinks consistently in classic and SEA modes Sep 18, 2026

robertsLando added 3 commits September 18, 2026 10:58

robertsLando changed the title ~~fix: resolve and report symlinks consistently in classic and SEA modes~~ fix(prelude): resolve and report symlinks consistently in classic and SEA modes Sep 18, 2026

robertsLando added 2 commits September 18, 2026 12:02

robertsLando mentioned this pull request Sep 28, 2026

test: pin node-opcua to 2.181.0 in test-80 #308

Merged

Merge branch 'main' into 295-resolve-symlinks

85eb435

Uh oh!

Conversation

mpotthoff commented Aug 20, 2026 • edited by robertsLando Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The bug

The fix

Reporting symlinks, not just resolving them

Behaviour change worth knowing about

Shared, so the modes cannot drift

Tests

Unrelated: test-80 pin

Uh oh!

codecov Bot commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

robertsLando left a comment

Choose a reason for hiding this comment

Review summary

The issue and the fix are both confirmed — verified end-to-end, not just read

Top 3 risks

Findings outside the diff

Coverage

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robertsLando commented Sep 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

robertsLando commented Sep 7, 2026

Uh oh!

robertsLando commented Sep 7, 2026

Uh oh!

spotandjake commented Sep 16, 2026

Uh oh!

robertsLando commented Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review rounds — what was decided, and what was left

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mpotthoff commented Aug 20, 2026 •

edited by robertsLando

Loading

Unrelated: `test-80` pin

codecov Bot commented Aug 25, 2026 •

edited

Loading

robertsLando commented Sep 18, 2026 •

edited

Loading