Skip to content

fix(cli): make the Windows CLI test gate runnable and green - #11

Draft
andromarces wants to merge 2 commits into
rjx18:mainfrom
andromarces:fix/windows-test-portability
Draft

fix(cli): make the Windows CLI test gate runnable and green#11
andromarces wants to merge 2 commits into
rjx18:mainfrom
andromarces:fix/windows-test-portability

Conversation

@andromarces

@andromarces andromarces commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Problem

On Windows, the CLI package's own documented test command could not run at all, and even a direct Vitest invocation showed five failing tests unrelated to any of the behavioural changes proposed elsewhere. Contributors on Windows had no way to distinguish their own regressions from this pre-existing baseline. See docs/issues/codor-install-issues.md (Issue I, parts 1 and 2) in this fork for the full investigation this PR implements.

What was wrong, and the fix

pnpm --filter @codor/cli test aborted with spawn vitest ENOENT. The wrapper at scripts/test-env.mjs spawned the given command name directly (spawn(command, args, ...)). On Windows, vitest in node_modules/.bin is a .CMD shim; child_process.spawn cannot execute a bare command name that only resolves to a .CMD/.bat file without shell: true, and enabling shell: true would put every test argument through cmd.exe's own quoting rules on a wrapper that exists to pass arguments through untouched.

Fix: resolve a bare package bin name (e.g. vitest) to its real JS entrypoint via the package's own package.json bin field, and run that under node directly — bypassing the platform-specific shim entirely, no shell involved. A bare command that names no such package (e.g. a system executable like node or git) falls back to being spawned directly, exactly as before this change. A command that already looks like a path (contains a separator, or is absolute — e.g. process.execPath, which the wrapper's own spec passes directly) is spawned unchanged too, since that case already worked and isn't the reported defect. (A first pass made bin-name resolution mandatory instead of a fallback, which regressed the bare-executable case — caught in review and corrected in the second commit, with regression coverage added for both paths.)

Five CLI-suite tests then still failed, each for its own reason (verified by direct reproduction, not assumed):

  • runtime-install.spec.ts and setup-tailscale.spec.ts: both fixtures build a POSIX-shaped absolute path as a raw string literal (/home/u, /usr/bin/tailscale) and assert exact equality against it. The code under test (packageRuntimePaths, resolveTailscale) normalizes through path.resolve(), which on Windows drive-qualifies a rooted-but-driveless path (/home/uC:\home\u). Fix: build the fixtures with resolve(sep, ...) too, so the expectation is constructed the same way the implementation constructs its result, on every platform — rather than special-casing the assertion for Windows.
  • index.spec.ts ("parses documented Claude hooks..."): a real temp-file path (containing backslashes on Windows) was substituted unescaped into a JSON fixture string, which JSON.parse then rejected (Bad escaped character in JSON). Separately, the actual file write used a JSON-escaped (double-backslashed) form of that same path — harmless on Windows only because NTFS collapses repeated separators, but the reverse of what each call site needed. Fix: use the raw path for the real filesystem write, and the JSON-escaped form for the JSON-embedded copy.
  • index.spec.ts ("bootstraps one home-rooted Desk..."): a real two-cycle server bootstrap (native module load, socket, HTTP) genuinely takes ~5.3-5.8s on this Windows machine, just over Vitest's 5s default — not a hang (confirmed by re-running with a raised timeout, where it consistently passes). Fix: extend this one test's timeout to 15s.
  • artifact.spec.ts: asserted the built wrapper's file mode carries a POSIX executable permission bit. Windows/NTFS has no such concept — writeFileSync(..., { mode: 0o755 }) verifiably produces 0o666 on Windows regardless (checked directly on this machine) — so the assertion cannot pass there and isn't testing anything meaningful on that platform. Fix: skip that one assertion on win32, keep it as-is elsewhere.

Scope

This is Issue I, parts (1) and (2), from the linked investigation. It does not make pnpm -r test green — the recursive gate still stops in packages/adapters/antigravity before ever reaching packages/cli (Issue I, part 3: an interrupted vs failed adapter-event mismatch). That failure is unrelated to test infrastructure and was not diagnosed as part of this change; it may need its own issue.

Harn

CONTRIBUTING.md notes a PR may be opened without Harn review, with the hard rule of never editing .harn/assumptions/ directly — this PR does not. The four spec-file changes touch no behavioural assumption; they only make existing assertions match what the implementation actually does, on every platform. The scripts/test-env.mjs change sits inside the cli-tests-delete-inherited-codor-environment block (ref=cli-test-environment-wrapper). The assumption's substance is unaffected: it changes how the wrapper spawns its child process, not which CODOR_* variables it strips or when. Opened as a draft since this touches process-spawning behaviour.

Testing

Windows 11, Node 26.2.0, pnpm 10.9.0 (the pinned packageManager). Commands and output below are the exact, current (post-review-fix) results.

pnpm install --frozen-lockfile   # Lockfile is up to date, resolution step is skipped
pnpm -r build                    # Scope: 17 of 18 workspace projects; succeeds
cd packages/cli
pnpm test                        # now runs (previously: spawn vitest ENOENT)
# Test Files  16 passed | 2 skipped (18)
#      Tests  182 passed | 18 skipped (200)

Before this change, a direct Vitest invocation (pnpm exec vitest run, since pnpm test itself could not spawn Vitest) showed 5 failed / 175 passed / 18 skipped (198). After this change, pnpm test itself runs to completion and is fully green: the same 175 plus the previously-failing 5, plus 2 new regression tests added in the review-fix commit (bare npm-bin resolution and bare-executable fallback) — 182 passed / 18 skipped / 0 failed (200 total).

Also re-ran pnpm -r test after this change to confirm scope: it still aborts in packages/adapters/antigravity (ERR_PNPM_RECURSIVE_RUN_FIRST_FAIL) and packages/cli still never runs under the recursive gate — confirming that gap is Issue I part (3), untouched by this PR.

Not tested: macOS and Linux. The scripts/test-env.mjs change is written to be a no-op in effect on POSIX (a bare vitest there already resolves and spawns correctly without a shim; this fix reroutes it through the same resolve-and-spawn-under-node path, which was manually reasoned through but not executed on that platform). If a POSIX reviewer can confirm pnpm --filter @codor/cli test still passes there, that closes the last gap.

pnpm --filter @codor/cli test could not spawn vitest on Windows: the
wrapper (scripts/test-env.mjs) spawned bare command names directly,
but child_process.spawn cannot execute a node_modules/.bin .CMD shim
without shell:true. Resolve a bare bin name to its real entrypoint and
run it under node directly instead; a caller-supplied path (e.g.
process.execPath, used by the wrapper's own spec) is spawned unchanged.

That unblocked the CLI suite's five pre-existing Windows failures, each
with its own root cause:
- runtime-install.spec.ts / setup-tailscale.spec.ts: POSIX-literal
  fixture paths didn't match what packageRuntimePaths()/resolveTailscale()
  actually produce after resolve() drive-qualifies them on Windows;
  build the fixtures the same way the code under test does.
- index.spec.ts: a real Windows path was substituted unescaped into a
  JSON fixture, producing invalid JSON; JSON-escape it, and use the
  raw (unescaped) path for the actual file write.
- index.spec.ts: a two-cycle real server bootstrap test routinely
  exceeds vitest's 5s default on Windows; extend its timeout.
- artifact.spec.ts: Windows has no POSIX executable permission bit, so
  skip that one assertion there instead of asserting a property NTFS
  doesn't track.
Codex review of PR 11 caught a compatibility regression: resolving a
bare command through its package.json bin entry was made mandatory,
so a bare non-package executable (node, git, etc.) now failed instead
of spawning directly as it did before. Only the codebase's one caller
(bare `vitest`) happened to still work, masking the narrowed contract.

Make resolution a preference: fall back to spawning the bare command
unchanged when it names no resolvable package bin entry. Add regression
coverage for both the bin-name-resolution path (bare `vitest`) and the
generic-executable fallback path (bare `node`).
@andromarces

andromarces commented Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Addressed both points from the review:

Compatibility regression (fixed, ee7f6a6). resolveBinEntry previously threw when a bare command named no npm package, so any bare non-package executable (node, git, ...) would now fail where it worked before — the PR only appeared correct because the codebase's one caller happens to be vitest. Made resolution a preference instead of a requirement: resolveBinEntry now returns undefined when the command isn't a resolvable package bin name, and the wrapper falls back to spawning the bare command directly, exactly as it did before this change. Added two regression tests to artifact.spec.ts's existing wrapper-isolation block: one exercising the bin-resolution path with bare vitest, one exercising the restored fallback with bare node. Full suite: 182 passed / 18 skipped / 0 failed (up from 180 — the two new tests).

pnpm -r build scope precision. Confirmed: it reports Scope: 17 of 18 workspace projects, not all 18. I don't have a verified explanation for why one project is excluded from that count, so I'm not going to guess at one here — the PR body now states the observed number only. The success claim itself was accurate; I'd misquoted the count in the PR body, which is now corrected there too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant