[laoyyu]

Motivation and Context

packages/codemod/test/v1-to-v2/packageJsonUpdater.test.ts creates a directory cycle with symlinkSync(dir, path.join(dir, 'src', 'loop'), 'dir').

On Windows, creating a dir symlink requires Administrator privileges or Developer Mode, so the test fails during setup with EPERM: operation not permitted, symlink for contributors who have neither. A junction needs no elevation, and lstat().isSymbolicLink() still returns true for it, so the cycle — and the test's intent — is preserved.

This addresses the symlink case of #3117 only. The CRLF case is handled separately in #3120; the POSIX signal case is untouched.

How Has This Been Tested?

Windows 11, Node 22, Developer Mode off:

• symlinkSync(..., 'dir') → EPERM: operation not permitted, symlink
• symlinkSync(..., 'junction') → succeeds, lstatSync(link).isSymbolicLink() === true

In packages/codemod:

• pnpm test → 645 passed
• reverting the change makes the symlink cycle case fail, confirming the test exercises this path
• pnpm run typecheck and eslint pass; prettier content check passes for this file (the repo-wide CRLF formatting noise from [v2] Test suite fails on Windows: 3 independent causes (CRLF checkout, symlink EPERM, POSIX signals) #3117 is unrelated to this change)

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature
• Breaking change
• Documentation update

[changeset-bot]

⚠️ No Changeset found

Latest commit: 76c6846

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[pkg-pr-new]

Open in StackBlitz

@modelcontextprotocol/client

npm i https://pkg.pr.new/@modelcontextprotocol/client@3122

@modelcontextprotocol/codemod

npm i https://pkg.pr.new/@modelcontextprotocol/codemod@3122

@modelcontextprotocol/core

npm i https://pkg.pr.new/@modelcontextprotocol/core@3122

@modelcontextprotocol/server

npm i https://pkg.pr.new/@modelcontextprotocol/server@3122

@modelcontextprotocol/server-legacy

npm i https://pkg.pr.new/@modelcontextprotocol/server-legacy@3122

@modelcontextprotocol/express

npm i https://pkg.pr.new/@modelcontextprotocol/express@3122

@modelcontextprotocol/fastify

npm i https://pkg.pr.new/@modelcontextprotocol/fastify@3122

@modelcontextprotocol/hono

npm i https://pkg.pr.new/@modelcontextprotocol/hono@3122

@modelcontextprotocol/node

npm i https://pkg.pr.new/@modelcontextprotocol/node@3122

commit: 76c6846

[arhancanli]

Read the diff. I couldn't run it here (no Node in my scratch environment), so this is from the Node docs and the test source.

The change looks right. Per the fs.symlink docs, type "is only available on Windows and ignored on other platforms", so process.platform === 'win32' ? 'junction' : 'dir' could be just 'junction'. The ternary is harmless and makes the intent explicit, so keep it if you prefer.

One thing to check is the cycle itself. A junction target must be an absolute directory (Node resolves it for you). dir in this test comes from the temp-dir setup, so it should be absolute, but it is worth confirming. Also, the codemod's cycle guard is what the test exercises. If it detects cycles with lstat().isSymbolicLink(), a junction passes. If it uses realpath on visited directories instead, it would also resolve the junction. Which of the two does the walker use? Your note that reverting the change fails the case suggests it is exercised, but it would be good to know the Windows run actually reached the cycle assertion and did not skip it.

[laoyyu]

Thanks — both are answerable from the code, and you are right to ask rather than take the PR body at face value.

On the cycle guard (your last question): it is neither lstat() nor realpath. File collection in src/runner.ts uses fast-glob with followSymbolicLinks: false — that option exists precisely because pnpm's node_modules layout contains link cycles that ELOOP a following walker. The second walk, in src/utils/projectAnalyzer.ts, uses readdirSync(..., { withFileTypes: true }) and only descends where entry.isDirectory() is true. libuv reports a junction as a symbolic link, so isDirectory() is false and fast-glob treats it like any other link. Neither path resolves the junction, so neither guard is bypassed by the substitution.

Did the Windows run reach the assertion? Yes. That test has no platform skip, and if fast-glob had followed the junction the walk would not terminate — it would hang rather than pass. It passes as one of 645 in packages/codemod, so expect(result.fileResults.map(f => f.filePath)).toHaveLength(1) did execute on Windows 11 with Developer Mode off.

Absolute target: dir comes from mkdtempSync(path.join(os.tmpdir(), ...)), so it is absolute, as you expected.

On the ternary: agreed it is cosmetic, since type is ignored off Windows. I kept it so the POSIX branch visibly reads as "unchanged" rather than relying on a platform-ignored argument. Happy to drop it if a maintainer would rather see plain 'junction'.

One clarification for anyone reading later: the pre-change Windows failure is EPERM during setup, not a failed assertion — so "reverting makes it fail" alone does not prove coverage. The hang argument above is the actual evidence that the Windows run exercises the guard.

[arhancanli]

Thanks, that settles it. isDirectory() is false on a junction because libuv reports it as a link, so neither walk descends and the substitution cannot bypass either guard. Agreed too that the hang argument is the real evidence, since the EPERM is a setup failure and not an assertion result.

One follow-up on that: if the guard ever regressed, this test would hang until the runner kills it rather than fail on toHaveLength(1). Does the vitest config for packages/codemod set a testTimeout low enough that a cycle shows up as a clear timeout failure on the Windows job, or would it sit until the CI job limit?

[laoyyu]

Verified rather than guessed, and the answer is the worse of your two options.

Neither config sets a timeout: packages/codemod/vitest.config.js is export default baseConfig verbatim, and common/vitest-config/vitest.config.js sets no testTimeout at all, so the Vitest default (5s) is what would apply. But it could not fire for this regression anyway — both walkers are synchronous (fg.sync in src/runner.ts, readdirSync in src/utils/projectAnalyzer.ts), so a genuine cycle blocks the worker's event loop and the timer never gets to run. And .github/workflows/main.yml sets no timeout-minutes, so the job would sit until GitHub's 360-minute default. So: it would ride the CI job limit, not fail cleanly.

One mitigating detail. The projectAnalyzer walk is plain recursion, so a regression there would most likely surface within seconds as RangeError: Maximum call stack size exceeded — or as a short file count failing toHaveLength(1) if the throw lands inside the readdirSync try/catch. Either way that path fails fast rather than hanging. The fast-glob path is the one that could genuinely spin, since suppressErrors: true swallows per-entry errors; that is the path that would ride to the job limit.

Worth being explicit: this gap predates the PR and is orthogonal to it — the change does not introduce or worsen it. Before the change the test did not even reach the assertion on Windows (it failed in setup with EPERM), so this PR is strictly an improvement in coverage on that platform. Fixing the bound itself would mean either timeout-minutes on the CI job or a hard-timeout worker around a synchronous walker, both outside the scope of a one-line test fix. Happy to help if maintainers want to pursue it separately.

[arhancanli]

That settles it, thanks for tracing both walkers: a sync walk can't be interrupted by a vitest timer, so the only real bound is the job limit, and I agree it is orthogonal to this PR. Of the two paths, I'd put the fast-glob one first for a follow-up since suppressErrors: true is what turns a cycle into a spin instead of a throw. Does fg.sync get followSymbolicLinks: false anywhere in runner.ts, or is it relying on the default? That would decide whether a cycle can reach it at all.

[DoroteoNavarrete]

Windows data point on the "it would hang" question, since I reported #3117 and tested the same change locally (Windows 11, Developer Mode off, Node 24.11.1).

I ran the mutation: followSymbolicLinks: false → true in src/runner.ts (it is set explicitly, line 156 on main). With the junction in place the test doesn't hang on Windows. It fails on toHaveLength(1) with 64 files instead of 1. My guess is that Windows stops resolving after a fixed number of reparse points (63) and suppressErrors: true swallows the error, but I haven't confirmed that.

So on Windows a regression in the fast-glob guard shows up as a normal assertion failure, and the mutation proves the test exercises the guard. Before the change it never got past the EPERM.

[arhancanli]

Thanks, this is the more useful result: the mutation fails on toHaveLength(1) with 64 files, so the test does exercise the guard on Windows and does not hang there. If the 63-reparse-point guess is right, the count should track the junction depth, not the file count. Does it stay at 64 if you add a second junction pointing at the first, or does it change?

[DoroteoNavarrete]

If the 63-reparse-point guess is right, the count should track the junction depth, not the file count. Does it stay at 64 if you add a second junction pointing at the first, or does it change?

@arhancanli this question got me curious, so I wrote a small script to test it. Short answer: it changes. With a second junction pointing at the first, it drops to 32.

The script works outside the codemod: it builds the cycle and runs fs.statSync on index.ts one level deeper each time until it fails (Windows 11, Node 24.11.1). I added a control with very long directory names to rule out path length:

Setup	Last level that resolves	Path length at failure
One junction (src/loop → root), like the test	63 (fails at 64)	628 chars
Same, with very long directory names	63 (fails at 64)	4084 chars
src/loop2 → src/loop → root (2 reparse points per level)	31 (fails at 32)	372 chars

So it's the reparse point count, not path length: 63 per path. Levels 0–63 are exactly the 64 files the mutation found, and suppressErrors: true hides the failure past that. Node reports it as ENOENT, not ELOOP. I measured the 63, I didn't find it in Microsoft's docs.

[arhancanli]

63 per path, and two reparse points per level giving 31 (levels 0 to 31 would be 32 files, so the drop from 64 to 32 fits) is a clean result; the long-name control rules out path length, which was the other obvious suspect. Thanks for running it.

Practical consequence for the test: if it asserts toHaveLength(1), the mutation failure message on Windows will say "64 files", and anyone who later changes the fixture to two junctions would see 32 and might think the guard changed. Would a comment next to the assertion, or an assertion that the count is greater than 1 under the mutation, be useful? Also curious whether windows-latest on GitHub Actions shows the same 63, since it runs with Developer Mode behaviour that differs from your machine.

[laoyyu]

On the count reaching 64: worth separating where that number can be seen. The mutation (followSymbolicLinks: false → true) is not part of CI, so windows-latest only shows 63 to someone who runs it by hand; on the PR as it stands, the job sees the normal collection and passes. That makes the number a note for the next person doing the mutation, not a CI-observable value.

So yes, I think the assertion is worth one line saying where 64 comes from — 63 resolved levels plus the one that fails, which is why a second junction halves it — and it costs nothing behaviorally. I have not added it to this branch yet: if you would rather see it here than in a follow-up, say so and I will push it as a separate commit; otherwise I will leave this PR as the Windows fix it is and take it up when that test is next touched.

On the second question, I cannot answer for windows-latest from here beyond that reasoning — the only Windows box I have is the one in the thread.

[laoyyu]

Pushed as 76c6846 — a short comment above the toHaveLength(1) assertion noting that a manual mutation run on Windows reports 64 entries (63 resolvable reparse-point levels plus the one that fails), and that a second nested junction halves the count to 32. Kept it comment-only so the behavior is unchanged; CI on the new commit should stay green.