fix(scripts): stop the snippet sync walking node_modules and cloned f… - #2664
Conversation
…ixture repos findPackageSrcDirs walked packages/ with readdirSync's recursive option and filtered node_modules out of the result. The filter came too late: recursive follows symlinks, so the walk entered every packages/*/node_modules link back into the pnpm store and re-traversed the dependency graph along every path, collecting each entry into one array until it exhausted the V8 heap. It also descended into packages/codemod/batch-test/repos and returned src dirs from the cloned fixture monorepos, whose sources the script would then process. Descend explicitly and skip node_modules, batch-test and dist before entering them. pnpm-workspace.yaml already excludes batch-test from the workspace. Severity depends on the local checkout: batch-test/repos is gitignored, so a developer who has run the codemod batch tests carries whole external monorepos under packages/, and there the check aborts out of memory and takes the pre-push hook down with it.
|
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — dev-tooling fix, verified locally: the rewritten walker returns exactly the 10 workspace src dirs (client, codemod, core, core-internal, middleware/{express,fastify,hono,node}, server, server-legacy), matching what the old traversal produced minus the node_modules/batch-test noise it exists to eliminate. The inline nit about the sibling walkers is a follow-up cleanup, not a blocker.
Extended reasoning...
Overview
Single-file change to scripts/sync-snippets.ts, a repo-internal build/docs script (run via pnpm sync:snippets and the pre-push hook). It replaces readdirSync's recursive option in findPackageSrcDirs with an explicit descent that skips node_modules, dist, and batch-test before entering them, and never follows symlinks (Dirent.isDirectory() is false for links). No changeset is needed — nothing published changes.
Verification
I executed the new walker logic directly against this checkout: it returns exactly the 10 workspace src dirs (including the nested packages/middleware/*/src ones), so no package is lost relative to the old traversal. The claim in the PR description that pnpm-workspace.yaml excludes batch-test checks out (!packages/codemod/batch-test/**). The added dist skip is a strict improvement — the old code could pick up src dirs inside build output.
Security risks
None. The script only reads/writes files inside the repo checkout, and this change narrows the set of directories it touches (notably it can no longer rewrite sources inside cloned fixture repos under batch-test).
Level of scrutiny
Low. This is developer tooling, not shipped SDK code; no public API, protocol, transport, or auth surface is involved. The main risk would be silently dropping a package src dir from the sync, which I ruled out empirically.
Other factors
The bug-hunting system found one nit: findSourceFiles and findMarkdownFiles in the same file still use the recursive-readdir + late-filter pattern. That is a real consistency/robustness gap (a symlink inside a src tree or docs/ could reintroduce the unbounded walk), but those trees contain no symlinks today, so it does not block the fix this PR ships — it is posted inline for the author as follow-up material.
| * | ||
| * @param packagesDir The packages directory | ||
| * @returns Array of absolute paths to src directories | ||
| */ | ||
| function findPackageSrcDirs(packagesDir: string): string[] { | ||
| const srcDirs: string[] = []; | ||
| const entries = readdirSync(packagesDir, { | ||
| withFileTypes: true, | ||
| recursive: true, | ||
| }); | ||
|
|
||
| for (const entry of entries) { | ||
| if (!entry.isDirectory()) continue; | ||
| if (entry.name !== 'src') continue; | ||
| const descend = (dir: string): void => { |
There was a problem hiding this comment.
🟡 The new doc comment on findPackageSrcDirs explains that recursive readdirSync follows symlinks and that filtering results post-traversal is too late — yet the same pattern survives at two sibling sites in this file: findSourceFiles (line 450) and findMarkdownFiles (line 485). Since findSourceFiles runs on every src dir the new walker returns, a stray symlink inside a package src tree (or under docs/) would re-create the unbounded walk this PR fixes; consider extracting the explicit descend/SKIPPED_DIRS walk into a shared helper used by all three walkers.
Extended reasoning...
What the finding is. This PR rewrites findPackageSrcDirs to descend explicitly and skip node_modules/dist/batch-test before entering them, and its new doc comment documents why: readdirSync's recursive option "follows symlinks and collects every entry it visits into a single array before returning, so filtering unwanted directories out of the result is too late to keep the walk bounded." But that exact pattern survives at two sibling sites in the same file: findSourceFiles (scripts/sync-snippets.ts:450) still calls readdirSync(dir, { withFileTypes: true, recursive: true }) and filters /generated paths out of the collected results afterward, and findMarkdownFiles (scripts/sync-snippets.ts:485) does the same recursive readdir over docs/ with a post-hoc startsWith(generatedApiDir) filter.\n\nThe code path. main() calls findPackageSrcDirs(PACKAGES_DIR) and then invokes findSourceFiles(srcDir) on every src dir the new walker returns, plus findMarkdownFiles(DOCS_DIR) once. So while the outer packages/ scan is now bounded by SKIPPED_DIRS and the symlink-safe entry.isDirectory() check, every inner walk immediately re-enters the unbounded recursive-readdir mode.\n\nWhy the new guard doesn't cover it. SKIPPED_DIRS and the explicit descend closure are local to findPackageSrcDirs — nothing prevents the inner walkers from following a symlink. Verifiers empirically confirmed that Node's recursive readdirSync does follow directory symlinks (a self-referencing link is re-entered until the symloop limit), so one stray symlink inside a package's src/ tree or under docs/ would re-create the very unbounded symlink-graph walk (and heap exhaustion in the pre-push hook) this PR exists to fix.\n\nStep-by-step example. (1) A developer (or a tool) drops a symlink at packages/client/src/some-link pointing at a large tree — or even back up the tree. (2) findPackageSrcDirs correctly returns packages/client/src (the link itself is irrelevant to the outer walk). (3) main() calls findSourceFiles('packages/client/src'), which runs readdirSync(dir, { recursive: true }) — this traversal follows the symlink and collects everything reachable through it into one array before any filter runs. (4) The /generated filter executes only after the full traversal, exactly the "too late" failure mode the PR's own comment describes.\n\nImpact and why it's a nit. Today src/ trees and docs/ contain no symlinks and no node_modules, so nothing breaks on merge — this is a consistency/robustness cleanup, not a live bug. The concrete cost is that the fix lands at one of three call sites of the same pattern in the same file, and future readers get contradictory guidance: a comment declaring the option unsafe sitting next to two live uses of it. This matches the repository's Completeness review convention (partial migrations leaving sibling code paths with the pattern the PR replaces).\n\nHow to fix. Extract the explicit-descend walk into a shared helper — e.g. a walk(dir, { skip: SKIPPED_DIRS, onFile }) that uses withFileTypes: true per level and never enters symlinks — and have all three walkers (findPackageSrcDirs, findSourceFiles, findMarkdownFiles) use it, moving the .examples.ts/.test.ts/generated and docs/api exclusions into the descend/file callbacks so they run before traversal rather than after.
2 participants
…ixture repos
findPackageSrcDirs walked packages/ with readdirSync's recursive option and filtered node_modules out of the result. The filter came too late: recursive follows symlinks, so the walk entered every packages/*/node_modules link back into the pnpm store and re-traversed the dependency graph along every path, collecting each entry into one array until it exhausted the V8 heap. It also descended into packages/codemod/batch-test/repos and returned src dirs from the cloned fixture monorepos, whose sources the script would then process.
Descend explicitly and skip node_modules, batch-test and dist before entering them. pnpm-workspace.yaml already excludes batch-test from the workspace.
Severity depends on the local checkout: batch-test/repos is gitignored, so a developer who has run the codemod batch tests carries whole external monorepos under packages/, and there the check aborts out of memory and takes the pre-push hook down with it.
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context