test(cli): stop the skills CLI tests reading the module cache as CLI output - #3653
Conversation
…output The two `info JSON output` tests run `cli/main.ts` as a child process and require its stderr to be empty. They were also requiring something else: that every npm dependency of the CLI already be present on the machine. `deno.json` sets `nodeModulesDir` to `auto`, so before the child reaches the command it reconciles `node_modules/` against `cli/main.ts`'s graph, and it narrates that work on stderr -- `Download https://registry.npmjs.org/yaml`. Nothing puts `yaml` there for us. CI warms its Deno cache from `src/index.ts`, and `yaml` reaches the CLI only through `cli/main.ts`, via `@opentelemetry/configuration` and `bash-tool`; it is in neither the warmed blob nor the test process's own graph. Whether a shard paid for the fetch came down to which cache entry `actions/cache` happened to restore, which is why coverage shard 7 failed here on pull requests that cannot reach this code. `--quiet` suppresses the runtime's diagnostics and nothing else. What the CLI writes still arrives -- `veryfront skills info` with no name and no `--json` still reports its usage error on stderr underneath it -- so the assertions still hold the command to a silent stderr; they just no longer hold the module cache to it as well. Verified by evicting the `yaml` packument from `DENO_DIR` before each run, which reproduces the CI diff exactly: 0/25 runs passed before, 25/25 after.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe skills CLI test helper now passes ChangesSkills CLI test execution
Estimated code review effort: 1 (Trivial) | ~2 minutes Mergeability Score: ⚪ Minimal · up to This localized test-only change prevents runtime dependency-fetch diagnostics from being mistaken for CLI stderr output, without changing CLI behavior. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Address PR review feedback on #3652: - Replace Number.parseInt with full-string digit check in parsePortEnv so PORT=3001abc is rejected rather than silently parsed as 3001 - Reject PORT values outside 1-65535 (zero, negative, overflow) with a logWarning that names the exact value and reason - Update handleDevCommand warning block to reuse parsePortEnv("PORT") instead of a second inline parseInt path - Add three tests: trailing garbage (3001abc), PORT=0, PORT=65536 — all must fall back to 3000 and emit a warning - command-help.ts: drop the dangling comma, tell users to open the URL the CLI prints on port fallback - quickstart.md: replace em dash with semicolon to fix lint:ci failure - Revert skills/handler.test.ts and attachment-csrf.test.tsx to origin/main (changes belong to separate PRs #3653 and a future CSRF fix)
The flake
cli/commands/skills/handler.test.tshas been failing on coverage shard 7/8 for pullrequests that cannot reach the code it tests — #3648 (proxy WebSocket bridge identity) and
#3651 (a release PR touching three version files), both with the same two steps under
info JSON output, both412 passed / 1 failed.Root cause
The failing assertions are
handler.test.ts:36andhandler.test.ts:52, bothassertEquals(result.stderr, "")against a childdeno run -A cli/main.ts skills info --json. The diff in the CI log (run 31642742785, jobcoverage shard 7/8) says whatlanded there:
That is the Deno runtime narrating its own work, not the CLI writing output.
deno.jsonsets
nodeModulesDirtoauto, so a child whose dependencies are not all present firstreconciles
node_modules/againstcli/main.ts's graph, and it reports each fetch thatcosts on stderr.
Nothing puts
yamlwhere the child would find it..github/actions/setup-denowarms theDeno cache from
src/index.ts, andnpm:yaml@2.9.0reaches the CLI only throughcli/main.ts— via@opentelemetry/configuration,bash-toolandjust-bash— so it isin neither the warmed blob nor the test process's own graph.
actions/cache/restorefallsback through
restore-keys, so which packages a shard actually has is decided by whichcache entry the runner happened to restore. That is the whole of the flakiness: the file
was asserting on the machine's module cache alongside the CLI's stderr.
This is a test defect, not a product one. The CLI writes nothing to stderr here; every
environment variable that any file in shard 7 mutates was probed against the command and
none of them change its stdout, stderr or exit code.
The fix
One line:
--quieton the child.It suppresses the runtime's diagnostics and nothing else. What the CLI writes still
arrives —
veryfront skills infowith no name and no--jsonstill reports✗ Usage: veryfront skills info <name>on stderr underneath--quiet— so the assertionsstill hold the command to a silent stderr. They just no longer hold the module cache to it
as well. No retry, no tolerance, no skip.
Verification
The failure reproduces deterministically by putting the machine in the state a partially
restored CI cache puts it in — evicting the
yamlpackument fromDENO_DIRbefore eachrun:
It produces the CI diff byte for byte.
--parallel, trigger armed each runDeno 2.7.7 throughout, matching CI.
Noticed while investigating, not fixed here
src/extensions/factory-loader.test.ts:201readsconst repositoryRoot = Deno.cwd()inside a test while
cli/commands/schedule/handler.test.tsmay hold the processdirectory through
withCwd. It failed once in 25 baseline shard-7 runs withModule not found "file:///.../vf-schedule-local-timeout-.../src/platform/compat/path/index.ts"— a live flake of the class
src/testing/cwd.tsexists to prevent, and one that file hasnot opted into. Deriving the root from
import.meta.urlfixes it.LOG_LEVEL=DEBUGin the environment,veryfront <cmd> --jsonwrites ~35 lines ofdebug logs to stdout ahead of the JSON envelope, which breaks
JSON.parsefor anymachine consumer.
setJsonMode()(cli/shared/json-output.ts:20) lowers the level onlyafter flag parsing, so module-load-time logs escape it. That one is a product bug and
deserves its own issue.
Summary by CodeRabbit