Skip to content

Commit 072ab7f

Browse files
os-helpclaude
andauthored
fix(cli): os test walks the tree lazily and prunes, instead of listing the whole repository first (#7363) (#7489)
`os test` documents `**` in `resolveGlob`'s own header, and a `**` pattern was the one thing it could not survive. ## The repro, measured before and after `node packages/cli/bin/run.js test '**/*.test.json'`, from the repository root of a `pnpm install`-ed worktree of this monorepo: - **Before** (`main` @ `afdc6ea`, built `dist`): **exit 134** — `FATAL ERROR: Ineffective mark-compacts near heap limit`, after **440 s** (7 m 20 s) of GC thrash at an 8 GB heap. Not one suite was loaded before it died. - **After**: **completes in 2–3 s**, exit 1. It finds the 3 `*.test.json` files that are actually in the tree — `packages/{client,metadata-core,spec}/ tsconfig.test.json` — and refuses each at load time as not-a-suite (the #6247 `TestSuiteSchema` gate, working as designed: they are tsconfigs with comments, not Quality Protocol suites). Exit 1 is the correct verdict for "3 files matched, 3 were not suites"; the point of the measurement is that the command now *reaches* a verdict. Isolated confirmation of the mechanism, same worktree: `fs.readdirSync('.', { recursive: true })` alone OOMs at 479 s. ## Why it died `resolveGlob` split the pattern at the first wildcard to get a static base dir, so a leading `**` left the base at `.` — and then called `fs.readdirSync(baseDir, { recursive: true })`, which **materialises every path under the base as one array before any filtering runs**. The filter that would have discarded almost all of them never got to run. The array was worse than one-entry-per-file, and this is the part the card did not have: **`readdirSync(recursive)` follows symlinked directories** (verified directly — a symlinked dir's contents appear in the result under the link's name). A pnpm `node_modules` is a symlink graph in which every package links to its dependencies' real directories, so the walkable path set is combinatorial in dependency depth, not linear in file count. That is how a tree `find` reports as **97 048 real entries** (10 432 outside `node_modules`) exhausted 8 GB. ## What changed `packages/cli/src/commands/test.ts` — `resolveGlob` only. The walk is now lazy and segment-directed: pattern segments are compiled to per-segment matchers, and the walk reads one directory at a time, descending only where the remaining pattern can still be satisfied. Nothing is ever accumulated in order to be discarded, and symlinked directories are not descended into — which removes the combinatorial blow-up along with any cycle risk. **Prune list: `node_modules`, `.git`, `dist`, `build`** (the four the card named). `node_modules` is the one that made the command unusable; all four are the same claim — a wildcard is a search of *your* sources, and none of these holds one. `node_modules` and `.git` are foreign trees. `dist` and `build` are generated, so a suite found there is a stale copy of one that also lives in source, and `os test` does not merely list it: it **loads and runs it against a live server**. Without an ignore list that was true of a vendored suite in `node_modules` too. The prune applies only to directories a *wildcard* reached. A pattern that spells the name out still walks it, because naming a directory is asking for it and a list of defaults must not overrule the argument you typed. **The second `statSync` pass is gone.** `Dirent` (`withFileTypes: true`) already answers file-vs-directory during the walk. Symlinks are the only entries that still cost a `statSync`, because `Dirent` reports them as neither — and the old pass did count symlinks-to-files as matches, so that is preserved rather than quietly dropped. ## Decisions the dispatch did not specify Three of these are behaviour changes beyond "walk lazily". Each is a case where the old code was wrong rather than merely slow, and each is pinned by a test. 1. **Absolute patterns now resolve absolutely.** A leading `/` was folded through `path.join` as an ordinary segment, so `/tmp/x/*.test.json` resolved against the cwd as `tmp/x` and silently matched nothing. Fixing this was also what let the new tests point at a `mkdtemp` fixture instead of `process.chdir`-ing the vitest worker. 2. **Only `*` and `**` are wildcards.** The old translation escaped dots and nothing else, so every other regex metacharacter in a filename reached the `RegExp` as an operator: `a+b.test.json` did not match itself, and `a?.json` meant "optional `a`" and matched `.json`. All metacharacters but `*` are now escaped. 3. **Results are sorted and de-duplicated**, so suites run in the same order on every filesystem. Adjacent `**` segments are also collapsed at compile time, so `**/**/x` does not reach the same directory down two pattern paths. Dropped, deliberately: the `fs.existsSync(baseDir)` pre-check. The walk's own `readdirSync` is what has to survive a base that is missing, unreadable, or a plain file — and it does, inside a `try`. The pre-check was a second answer to the same question, and the one that goes stale between the check and the read. Verified by mutation: removing the `try`/`catch` turns the missing-base test red, removing the `existsSync` alone changes nothing. Docs: `content/docs/deployment/cli.mdx` `#### os test` now states the glob semantics and the prune list, since a silently-skipped suite in `build/` is exactly the surprise that has to be written down. (Nothing under `content/docs/releases/`.) ## The tests, and proof they can fail `packages/cli/test/resolve-glob-lazy-walk.test.ts` — 15 cases. As the dispatch warned, this repo has **no** Quality Protocol suites, so the tests build their own fixture tree under `mkdtemp`: source suites at three depths, decoy suites inside `node_modules`, a nested `packages/a/node_modules`, `dist`, `build` and `.git`, a directory *named* `decoy.test.json`, a filename with a regex metacharacter, and a symlinked directory pointing back up the tree. A test that passed by finding nothing would prove nothing, so every assertion names the files it expects. The laziness pins do not test the result, they test the walk: `vi.spyOn(fs, 'readdirSync')` asserts no pruned directory is ever *read* (the prune is a walk decision, not a post-filter) and that `recursive` is never requested; `vi.spyOn(fs, 'statSync')` asserts the second pass is gone. Every one of the 15 was driven red by mutating the source and green by reverting. The mutations run, each in isolation: empty prune list (2 red) · descend into symlinked dirs (4 red) · drop sort+dedup (1 red) · escape-dots-only (1 red) · stat every survivor (1 red) · `recursive: true` restored (7 red) · absolute base folded as relative (11 red) · no-wildcard fast path removed (1 red) · prune overrules an explicit literal segment (1 red) · readdir error uncaught (1 red) · collapse+dedup both removed (1 red). Two mutations initially **survived**, and both were real slack that this found: the "walks a pruned dir when you spell it out" case only exercised names sitting in the *static base* (peeled off before the walk, so never offered to the prune at all), and the missing-base case was covered by the `try`/`catch` rather than by the `existsSync` it appeared to test. The test was rewritten to reach the pruned name through a literal segment the walk itself matches, and the redundant `existsSync` was removed. ## One thing worth passing on The first full-suite run failed `format-zod-union.test.ts`'s `--json` purity pin with a JSON parse error — caused by **this commit's own doc comment**. It contained the example `packages/*/dist/…`, whose `*/` terminated the block comment early; the rest of the prose became code, and the residue happened to parse (a division, then a template literal opened by a backtick in the next comment). `tsc --noEmit` and `eslint` were both green on it. It surfaced only as `ReferenceError: dist is not defined` from oclif's command discovery, printed to stdout, corrupting the `--json` payload of an *unrelated* command. The pin caught it; the comment is rephrased. Worth knowing that a stray `*/` in a doc example is invisible to the type and lint gates and lands as output pollution. ## Gates - `pnpm lint` (repo-wide `eslint . --no-inline-config`) — clean - `pnpm typecheck` (turbo, 126 tasks) — all successful - `pnpm --filter @objectstack/cli test` — 109 files, **1182 passed, 0 failed** (the pre-fix run of the same suite was 1147 tests with 4 failed files; the difference is the comment bug above, which aborted three files at import) - `check:empty-changeset`, `check:adr-0087-registration`, `check:nul-bytes`, `check:doc-authoring`, `check:docs-audit-scope` — all green - Changeset: `.changeset/os-test-glob-lazy-walk.md` (`@objectstack/cli`: patch) Fixes #7363 Claude-Session: https://claude.ai/code/session_01G4J3CVg3cRVnZ9QCL2KsQY Co-authored-by: Claude <noreply@anthropic.com>
1 parent cf7c694 commit 072ab7f

4 files changed

Lines changed: 424 additions & 29 deletions

File tree

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
---
2+
"@objectstack/cli": patch
3+
---
4+
5+
fix(cli): `os test` walks the tree lazily and prunes, instead of listing the whole repository first (#7363)
6+
7+
`os test` documents `**` in `resolveGlob`'s own header, and a `**` pattern was the
8+
one thing it could not survive. Run from a repository root:
9+
10+
```
11+
$ node packages/cli/bin/run.js test '**/*.test.json'
12+
FATAL ERROR: Ineffective mark-compacts near heap limit — JavaScript heap out of memory
13+
```
14+
15+
Exit 134, after ~7 minutes of GC thrash, before a single suite loaded.
16+
17+
**Why.** `resolveGlob` split the pattern at the first wildcard to get a static base
18+
directory, so a leading `**` left the base at `.` — and then called
19+
`fs.readdirSync(baseDir, { recursive: true })`, which **materialises every path
20+
under the base as one array before any filtering runs**. The filter that would have
21+
thrown almost all of them away never got to run.
22+
23+
The array was worse than "one entry per file", too. `readdirSync(…, { recursive:
24+
true })` *follows symlinked directories*, and a pnpm `node_modules` is a symlink
25+
graph in which every package links to its dependencies' real directories — so the
26+
set of walkable paths is combinatorial in dependency depth, not linear in file
27+
count. That is how a tree `find` reports as ~97k real entries exhausted an 8 GB heap.
28+
29+
**Now.** The walk is lazy and segment-directed: it reads one directory at a time and
30+
descends only into directories that can still satisfy the rest of the pattern, so
31+
nothing is ever accumulated in order to be discarded. It does not follow symlinked
32+
directories, which removes the combinatorial blow-up along with any cycle risk.
33+
34+
Same command, same repository, after: **completes in 3 seconds**, having found the
35+
three `*.test.json` files that are actually in the tree.
36+
37+
**A wildcard no longer descends into `node_modules`, `.git`, `dist` or `build`.**
38+
These are the same defect at a smaller scale: with no ignore list, a suite vendored
39+
in `node_modules` — or a stale copy of your own suite left in `dist` — was a match
40+
`os test` would load and **run against a live server**. A wildcard is a search of
41+
your sources, and none of those four holds one. The prune applies only to
42+
directories a *wildcard* reached: a pattern that spells the name out
43+
(`packages/*/dist/*.test.json`) still walks it, because naming a directory is asking
44+
for it and a list of defaults must not overrule the argument you typed.
45+
46+
Three smaller corrections that fell out of the rewrite:
47+
48+
- **No second `statSync` pass.** The old code stat'ed every surviving match to
49+
confirm it was a file; `Dirent` already answers that during the walk. Symlinks are
50+
the only entries that still cost a syscall, and they are still counted as matches
51+
exactly as before.
52+
- **Only `*` and `**` are wildcards now.** The old translation escaped dots and
53+
nothing else, so every other regex metacharacter in a filename reached the `RegExp`
54+
as an operator: `a+b.test.json` did not match itself, and `a?.json` meant "optional
55+
`a`" and matched `.json`.
56+
- **Absolute patterns resolve absolutely.** A leading `/` was folded through
57+
`path.join` as an ordinary segment, so `/tmp/x/*.test.json` was resolved against
58+
the current working directory and silently matched nothing.
59+
60+
Matching is otherwise unchanged: the default `qa/*.test.json` resolves as it always
61+
did, `**` still matches zero segments as well as many, and a pattern with no wildcard
62+
is still a direct file path. Results are now sorted, so suites run in the same order
63+
on every filesystem.

content/docs/deployment/cli.mdx

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1017,8 +1017,18 @@ os test # Default: qa/*.test.json
10171017
os test qa/my-test.json # Specific test file
10181018
os test --url http://localhost:4000 # Custom server URL
10191019
os test --token my-api-key # With authentication
1020+
os test 'qa/**/*.test.json' # Recursive — quote it, or the shell expands it first
10201021
```
10211022

1023+
The pattern accepts `*` (one path segment) and `**` (any number of segments);
1024+
every other character is matched literally. A **wildcard** never descends into
1025+
`node_modules`, `.git`, `dist` or `build`: a wildcard is a search of your own
1026+
sources, and a suite found in a dependency or in build output is one `os test`
1027+
would otherwise load and **run against your server**. Naming such a directory
1028+
still reaches it — `packages/*/dist/*.test.json` walks `dist` because you asked
1029+
for `dist`. Matches run in sorted order, so a suite runs in the same position on
1030+
every machine.
1031+
10221032
Each file is validated against `TestSuiteSchema` **before it runs**. A suite that
10231033
does not match is refused at load time, naming the file and every offending path,
10241034
and counts as one failed suite — the rest of the glob still runs. This is what

packages/cli/src/commands/test.ts

Lines changed: 154 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -8,49 +8,174 @@ import { QA as CoreQA } from '@objectstack/core';
88
import * as QA from '@objectstack/spec/qa';
99
import type { ZodError } from 'zod';
1010

11+
/**
12+
* Directory names a wildcard never descends into (#7363).
13+
*
14+
* `node_modules` is the one that made the command unusable, but all four are
15+
* the same claim: a wildcard is a search of *your* sources, and none of these
16+
* holds one. `node_modules` and `.git` are foreign trees; `dist` and `build`
17+
* are generated, so a suite found there is a stale copy of one that also lives
18+
* in source — discovering both runs it twice, and reports the build output's
19+
* version of a file the author is editing.
20+
*
21+
* The prune applies only to directories a *wildcard* reached. A pattern that
22+
* spells the name out still walks it — whether the name sits in the static
23+
* base (`node_modules/pkg/qa/…`, never offered to the prune at all) or in a
24+
* literal segment the walk itself matched (`packages/<any>/dist/…`). Naming a
25+
* directory is asking for it, and a list of defaults must not overrule the
26+
* argument you typed.
27+
*/
28+
const PRUNED_DIRS = new Set(['node_modules', '.git', 'dist', 'build']);
29+
30+
/** `**` — matches zero or more path segments. */
31+
const GLOBSTAR = Symbol('globstar');
32+
33+
type SegmentMatcher = typeof GLOBSTAR | { readonly literal: string } | { readonly re: RegExp };
34+
35+
/**
36+
* Compile one pattern segment.
37+
*
38+
* Only `*` and `**` are wildcards — the two the command documents. Every other
39+
* regex metacharacter is escaped, including `?`, which the previous
40+
* escape-the-dots-only translation leaked through as a quantifier: `a?.json`
41+
* used to mean "optional `a`" and matched `.json`.
42+
*/
43+
function compileSegment(segment: string): SegmentMatcher {
44+
if (segment === '**') return GLOBSTAR;
45+
if (!segment.includes('*')) return { literal: segment };
46+
const source = segment
47+
.replace(/[.+?^${}()|[\]\\]/g, '\\$&')
48+
.replace(/\*/g, '[^/]*');
49+
return { re: new RegExp(`^${source}$`) };
50+
}
51+
52+
function segmentMatches(matcher: SegmentMatcher, name: string): boolean {
53+
if (matcher === GLOBSTAR) return true;
54+
return 'literal' in matcher ? matcher.literal === name : matcher.re.test(name);
55+
}
56+
57+
/**
58+
* Is this entry a file? `Dirent` already answers for the common cases, so the
59+
* only entries that still cost a syscall are symlinks — which `Dirent` reports
60+
* as neither file nor directory, and which the old `statSync` pass did count as
61+
* matches. That behaviour is preserved; what is gone is stat'ing every survivor
62+
* a second time.
63+
*/
64+
function isFileEntry(entry: fs.Dirent, fullPath: string): boolean {
65+
if (entry.isFile()) return true;
66+
if (!entry.isSymbolicLink()) return false;
67+
try {
68+
return fs.statSync(fullPath).isFile();
69+
} catch {
70+
return false; // dangling link — not a suite anyone can load
71+
}
72+
}
73+
1174
/**
1275
* Resolve a glob-like pattern to matching file paths.
1376
* Supports `*` (single segment wildcard) and `**` (recursive wildcard).
1477
* Falls back to direct file path if no glob characters are present.
78+
*
79+
* The walk is lazy and segment-directed: it descends only into directories that
80+
* can still satisfy the rest of the pattern, and never builds a list of
81+
* candidates to filter afterwards. The previous implementation materialised
82+
* `readdirSync(baseDir, { recursive: true })` — every path under the base, as
83+
* one array, before any filtering ran — which for a leading `**` (base dir `.`)
84+
* meant the whole repository. Measured on this monorepo, `os test` with a
85+
* `**` pattern died at exit 134 after ~7 minutes of GC thrash, before a single
86+
* suite loaded (#7363).
87+
*
88+
* The walk also never descends into a symlinked directory. That is not only
89+
* cycle safety, it is the other half of why the old array was unsurvivable:
90+
* `readdirSync(…, { recursive: true })` DOES follow them, and a pnpm
91+
* `node_modules` is a symlink graph in which every package links to its
92+
* dependencies' real directories — so the set of walkable paths is not merely
93+
* large, it is combinatorial in dependency depth. That is where the "millions
94+
* of entries" came from, and how a tree `find` reports as ~97k real entries
95+
* exhausted an 8 GB heap.
1596
*/
16-
function resolveGlob(pattern: string): string[] {
97+
export function resolveGlob(pattern: string): string[] {
1798
// Direct file path — no wildcards
1899
if (!pattern.includes('*')) {
19100
return fs.existsSync(pattern) ? [pattern] : [];
20101
}
21102

22-
// Split pattern into the static base directory and the glob portion
23-
const parts = pattern.split(path.sep.replace('\\', '/'));
24-
// Also handle forward-slash on Windows
25-
const segments = pattern.includes('/') ? pattern.split('/') : parts;
103+
const segments = pattern.replace(/\\/g, '/').split('/');
26104

27-
let baseDir = '.';
105+
// Peel off the leading static segments: they are a plain directory path, and
106+
// walking them is a lookup rather than a search.
28107
let globStart = 0;
29-
for (let i = 0; i < segments.length; i++) {
30-
if (segments[i].includes('*')) {
31-
globStart = i;
32-
break;
33-
}
34-
baseDir = i === 0 ? segments[i] : path.join(baseDir, segments[i]);
108+
while (globStart < segments.length && !segments[globStart].includes('*')) globStart++;
109+
const baseParts = segments.slice(0, globStart);
110+
const baseDir = baseParts.length === 0
111+
? '.'
112+
: baseParts[0] === ''
113+
// A leading '' means an absolute pattern. Folding it through `path.join`
114+
// as an ordinary segment turned `/tmp/x/*.json` into a cwd-relative
115+
// `tmp/x`, which then silently matched nothing.
116+
? path.join('/', ...baseParts)
117+
: path.join(...baseParts);
118+
119+
// No `existsSync(baseDir)` guard: the walk's own `readdirSync` is the thing
120+
// that has to survive a base that is missing, unreadable, or a plain file,
121+
// and it does. A pre-check would only be a second answer to the same
122+
// question — and the one that goes stale between the check and the read.
123+
124+
// Adjacent `**` segments are one `**`; collapsing them keeps the walk from
125+
// reaching the same directory down two different pattern paths.
126+
const matchers: SegmentMatcher[] = [];
127+
for (const segment of segments.slice(globStart)) {
128+
const matcher = compileSegment(segment);
129+
if (matcher === GLOBSTAR && matchers[matchers.length - 1] === GLOBSTAR) continue;
130+
matchers.push(matcher);
35131
}
36132

37-
if (!fs.existsSync(baseDir)) return [];
38-
39-
// Convert the glob portion into a RegExp
40-
const globPortion = segments.slice(globStart).join('/');
41-
const regexStr = globPortion
42-
.replace(/\./g, '\\.') // escape dots
43-
.replace(/\*\*\//g, '(.+/)?') // ** matches any directory depth
44-
.replace(/\*\*/g, '.*') // trailing ** without slash
45-
.replace(/\*/g, '[^/]*'); // * matches within a single segment
46-
const regex = new RegExp(`^${regexStr}$`);
47-
48-
// Recursively read all files under baseDir
49-
const entries = fs.readdirSync(baseDir, { recursive: true, encoding: 'utf-8' }) as string[];
50-
return entries
51-
.filter(entry => regex.test(entry.replace(/\\/g, '/')))
52-
.map(entry => path.join(baseDir, entry))
53-
.filter(fullPath => fs.statSync(fullPath).isFile());
133+
const results: string[] = [];
134+
135+
const walk = (dir: string, index: number): void => {
136+
const matcher = matchers[index];
137+
const isLast = index === matchers.length - 1;
138+
139+
// `**` also matches *zero* segments, so the rest of the pattern gets a turn
140+
// at this same directory before we descend.
141+
if (matcher === GLOBSTAR && !isLast) walk(dir, index + 1);
142+
143+
let entries: fs.Dirent[];
144+
try {
145+
entries = fs.readdirSync(dir, { withFileTypes: true });
146+
} catch {
147+
return; // unreadable directory — nothing in it can be a match
148+
}
149+
150+
for (const entry of entries) {
151+
const isDir = entry.isDirectory();
152+
const fullPath = path.join(dir, entry.name);
153+
154+
if (matcher === GLOBSTAR) {
155+
// A trailing `**` matches every file below this point.
156+
if (isLast && isFileEntry(entry, fullPath)) results.push(fullPath);
157+
// `**` is a wildcard, so the prune list applies to it unconditionally.
158+
if (isDir && !PRUNED_DIRS.has(entry.name)) walk(fullPath, index);
159+
continue;
160+
}
161+
162+
if (!segmentMatches(matcher, entry.name)) continue;
163+
// Pruned only when a wildcard segment is what reached it.
164+
if (isDir && PRUNED_DIRS.has(entry.name) && !('literal' in matcher)) continue;
165+
166+
if (isLast) {
167+
if (isFileEntry(entry, fullPath)) results.push(fullPath);
168+
} else if (isDir) {
169+
walk(fullPath, index + 1);
170+
}
171+
}
172+
};
173+
174+
walk(baseDir, 0);
175+
176+
// `readdir` order is filesystem-dependent; suites should run in the same
177+
// order on every machine.
178+
return [...new Set(results)].sort();
54179
}
55180

56181
/** The suite shape, quoted back at an author whose file did not match it. */

0 commit comments

Comments
 (0)