Skip to content

fix: compare whole path segments when detecting recursive symlinks - #170

Open
xyrolle wants to merge 1 commit into
thecodrr:masterfrom
xyrolle:fix/symlink-prefix-siblings
Open

xyrolle wants to merge 1 commit into
thecodrr:masterfrom
xyrolle:fix/symlink-prefix-siblings

Conversation

@xyrolle

@xyrolle xyrolle commented Oct 22, 2026

Copy link
Copy Markdown

Fixes #169

With withSymlinks({ resolvePaths: false }), isRecursive treated one symlink target as inside another when its path merely started with the other's as a string, so a link to /x/shared-other inside a link to /x/shared was skipped as recursive. It now compares whole path segments (isInside), after normalizing the target, since a custom fs can return / on Windows. This keeps the existing rule that equal, ancestor and descendant targets count as recursive; only siblings whose names share a prefix change.

Test: a symlinks.test.ts case with root/src -> shared and shared/alias -> shared-other, for both sync and withPromise. It fails without the change.

On master (790c182), 19 tests already fail without this change, and the same 19 fail with it. Applied on v6.5.0, all 107 pass. tsc --noEmit, npm run build and Prettier pass on the branch. #162 moves these tests off mock-fs; if it lands first, I'll move this case to its fixtures.

@thecodrr

Copy link
Copy Markdown
Owner

@xyrolle any idea why all the tests are failing?

@xyrolle

xyrolle commented Oct 26, 2026 •

Copy link
Copy Markdown
Author

@thecodrr These 19 fail on master too, without this change; on v6.5.0 all 107 pass. They start with two commits after it: eb6136b adds .withErrors() to the tests' crawls, so errors they used to skip now fail them (the symlink fixtures' broken links, ENOENT … /some/dir/fileSymlink2, and scandir 'node_modules\' in the separator tests), and 4a2a6b3 adds "do not add a separator if path already ends with one", which fails on master as well. This PR only changes isInside and adds one test, red before and green after. I can send a separate PR that fixes master's tests if that helps.

@xyrolle

xyrolle commented Oct 26, 2026 •

Copy link
Copy Markdown
Author

@thecodrr The failing tests are fixed in #172, which brings master's suite back to green (112 passing): a line the errors commit dropped, a missing normalizePath import, and a broken link in the shared symlink fixtures that .withErrors() now reports, with tests of its own for broken links. Once it lands, this PR's CI should pass as well.

#172 marks one test as a known failure on POSIX, "crawl all files and invert path separator", because it fails for a real reason: the walker reads the disk through the output path, so a \ separator reaches readdir. A separate PR fixes the walker to keep native paths for the file system, and removes that marker. I'll link it here when it's up.

@xyrolle

xyrolle commented Oct 26, 2026

Copy link
Copy Markdown
Author

@thecodrr The walker fix is up in #173: it crawls through native paths and gives only the output the custom separator, so a \ separator on Linux and macOS finds files instead of failing on readdir. It removes the known-failure marker #172 adds, with tests for the output forms, names containing \, and symlinks under either separator. It's stacked on #172 and meets this PR only at the path import in resolve-symlink.ts; together they pass all 160 tests.

So the order is #172 (master's tests), then this PR, then #173.

@xyrolle

xyrolle commented Oct 29, 2026

Copy link
Copy Markdown
Author

@thecodrr A friendly ping in case this got buried: #172 is green on all six jobs and brings master's tests back to passing, which is what made this PR's CI fail. Once #172 is merged, this PR should go green as it is, and #173 follows it. Happy to change anything in any of them.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

withSymlinks({ resolvePaths: false }) skips a nested link whose target's name starts with the outer target's

2 participants