Skip to content

fix(scripts): stop the snippet sync walking node_modules and cloned f… #2664

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
felixweinberger merged 1 commit into from
Aug 14, 2026
Merged

fix(scripts): stop the snippet sync walking node_modules and cloned f… #2664

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 37 additions & 14 deletions scripts/sync-snippets.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,31 +504,54 @@
return files;
}

/**
* Directory names that are never descended into when looking for package sources.
*
* `node_modules` is the critical one: pnpm fills it with symlinks pointing back
* into the store, whose packages link onward in turn. Entering it means walking
* that graph along every distinct link path, re-visiting the same directories
* over and over.
*
* `batch-test` holds the codemod's cloned fixture repositories — whole external
* monorepos that `pnpm-workspace.yaml` already excludes from the workspace, and
* whose sources this script must never rewrite.
*/
const SKIPPED_DIRS = new Set(['node_modules', 'dist', 'batch-test']);

/**
* Find all package src directories under the packages directory.
*
* Descends explicitly rather than using `readdirSync`'s `recursive` option. That
* 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 — the traversal has already happened.
*
* @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 => {

Check warning on line 535 in scripts/sync-snippets.ts

View check run for this annotation

Claude / Claude Code Review

[quality] findSourceFiles/findMarkdownFiles keep the recursive-readdir + late-filter pattern the PR replaces 

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/SKIPP
Comment on lines +528 to +535

@claude claude Bot Aug 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

for (const entry of readdirSync(dir, { withFileTypes: true })) {
// isDirectory() is false for a symlink, so pnpm's links are never entered.
if (!entry.isDirectory()) continue;
if (SKIPPED_DIRS.has(entry.name)) continue;

const fullPath = join(entry.parentPath, entry.name);
const fullPath = join(dir, entry.name);

// Only include src dirs that are direct children of a package
// (e.g., packages/core-internal/src, packages/middleware/express/src)
// Skip nested src dirs like node_modules/*/src
if (fullPath.includes('node_modules')) continue;
// A package owns a single src dir; everything below it is that package's
// own tree, which findSourceFiles walks.
if (entry.name === 'src') {
srcDirs.push(fullPath);
continue;
}

srcDirs.push(fullPath);
}
descend(fullPath);
}
};

descend(packagesDir);

return srcDirs;
}
Expand Down
Loading