chore: fix the completion import cycle + upgrade vitest 2 → 4 with re-derived thresholds - #2
Merged
Merged
Conversation
…n cycle program.ts imports completion.ts to register the command, and completion.ts imported program.ts back to walk the command tree. The import was already dynamic specifically to break that cycle at runtime, and under plain Node it does. Under vitest's module runner it does not reliably: with workers running in parallel the cycle could resolve to a half-initialised namespace, and `buildProgram` came back undefined — surfacing as an intermittent "buildProgram is not a function" in handlers-batch3/completion, ~1 in 5 full runs on Windows, always passing when the file ran in isolation. Rather than retry around it, the cycle is now gone: `createCompletionCommand` takes the builder as a parameter and program.ts passes its own `buildProgram`. The dependency is one-directional, and `extractTree` no longer needs to be async. Tests pass the builder explicitly. Behaviour is unchanged — completion still walks the live tree rather than a hardcoded list; verified `completion bash` still emits the mcp verbs.
package.json had already declared ^4.1.9 while the lockfile pinned 2.1.9, so
the two had been out of sync since before 1.5.0. This makes the upgrade real,
on its own terms, with the thresholds recomputed rather than relaxed to fit.
Vitest 4's AST-aware remapping measures the TypeScript source instead of the
transpiled output, so the numbers are not comparable to the old ones. The
denominators moved in BOTH directions and the covered counts went UP:
vitest 2 vitest 4
stmts 5158/6526 79.03% 2298/3013 76.26%
branches 1266/1679 75.40% 1371/2219 61.78%
funcs 315/358 87.98% 405/508 79.72%
lines 5158/6526 79.03% 2156/2789 77.30%
Same code, same tests — vitest 2 inflated statements/lines by counting
transpiled output while missing ~32% of branches and ~42% of functions. The
old 70% branch gate was never 70% of the real branches. Thresholds are now
74/59/77/75, ~2pts under actuals (the CI legs drift ~0.3pt between platforms).
Also scopes a 30s timeout to the spawned-CLI exit-code block: under
--coverage the child process inherits NODE_V8_COVERAGE and writes its own
profile on exit, pushing a spawn past the 5s default. Scoped rather than
global so the other ~480 in-process tests keep a tight timeout.
Suite is also ~2x faster (31s vs 60s). 490 tests pass; 8 consecutive full
runs green.
GHSA-5p4m-2wfm-xmqj (high) landed against js-yaml 4.0.0-4.3.0 — quadratic CPU consumption resolving !!omap, with the CVE-2026-59870 fix not backported. It turned `npm audit --omit=dev --audit-level=high` red on every CI leg. js-yaml is a direct production dependency (config.yml parsing) and 4.3.1 sits inside the existing ^4.1.0 range, so this is a lockfile-only bump. Unrelated to the vitest work on this branch — same class as the fast-uri/tar bumps in edb7e48. Verified: audit → 0 vulnerabilities; 490 tests pass; coverage unchanged at 76.26 / 61.78 / 79.72 / 77.30.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pays off the two debts left open by #1. Two commits, deliberately separable.
1.
fix(completion)— the Windows flake, at the rootprogram.tsimportscompletion.tsto register the command;completion.tsimported
program.tsback to walk the command tree. That import was alreadydynamic to break the cycle, and under plain Node it works. Under vitest's
module runner it did not reliably: with workers in parallel the cycle could
resolve to a half-initialised namespace and
buildProgramcame backundefined —
buildProgram is not a function, ~1 in 5 full runs, alwayspassing when the file ran alone.
Fixed by removing the cycle rather than retrying around it:
createCompletionCommandnow takes the builder as a parameter andprogram.tspasses its own
buildProgram. The dependency is one-directional andextractTreeno longer needs to be async.Behaviour unchanged — completion still walks the live tree, verified
completion bashstill emits themcpverbs.2.
chore(test)— vitest 2 → 4, thresholds recomputed not relaxedpackage.jsonalready declared^4.1.9while the lockfile pinned2.1.9;the two had been out of sync since before 1.5.0. This makes the upgrade real.
Vitest 4's AST-aware remapping measures the TypeScript source instead of the
transpiled output, so its numbers are not comparable to the old ones. The
denominators moved in both directions and the covered counts went up:
Same code, same tests. Vitest 2 inflated statements/lines by counting
transpiled output while missing ~32% of branches and ~42% of functions — the
old 70% branch gate was never 70% of the real branches. Thresholds are now
74/59/77/75, ~2pts under actuals (CI legs drift ~0.3pt between platforms).
Also scopes a 30s timeout to the spawned-CLI exit-code block: under
--coveragethe child inheritsNODE_V8_COVERAGEand writes its own profileon exit, pushing a spawn past the 5s default. Scoped rather than global so the
other ~480 in-process tests keep a tight timeout.
Verification
npm audit --omit=dev --audit-level=high→ 0 vulnerabilities.A flake at this rate can't be proven gone by sampling — but the mechanism
that caused it is removed, and the cycle is verifiably no longer in the graph.
🤖 Generated with Claude Code