test(windows): bring the Windows CI suite to green (299→0), no Linux regression#278
Merged
Conversation
Temporary trigger so the Windows full-suite job (push-only) runs on this branch while we fix it. Revert before merge.
…dir honors temp HOME os.homedir() reads %USERPROFILE% (then %HOMEDRIVE%%HOMEPATH%) on Windows, not $HOME. 27 test files faked the home dir via process.env.HOME only, so on Windows production code resolved the real C:\Users\<runner>\... — causing hundreds of path-assertion + ENOENT failures. Add tests/shared/fake-home.ts (setFakeHome/clearFakeHome) setting all four vars, and adopt it across the 27 HOME-faking files. Behavior-neutral on POSIX (homedir reads $HOME), so Linux/macOS results are unchanged.
Fix the cross-platform bugs and skip genuinely-POSIX-only tests so the
Windows full suite passes; all changes behavior-neutral on Linux/macOS.
Product (real cross-platform bugs):
- notifications/{queue,state}.ts: home-containment used startsWith(home+'/')
(rejected every write on Windows where resolve() emits '\') -> path.relative;
add renameAtomic with a Windows EPERM/EBUSY/EACCES retry for the temp->dest
swap.
- cli/install-hermes.ts: isHivemindHook now normalizes '\'->'/' so re-install
dedup matches on Windows (was duplicating hooks).
- skillify/spawn-mine-local-worker.ts: findHivemindLauncher uses 'where' on
win32 (was hardcoded 'which').
- skillify/gate-runner.ts: export buildArgs for deterministic argv assertions.
- scripts/sync-versions.mjs: drop the shebang (only run via 'node'); it broke
vitest import on a CRLF checkout. Add .gitattributes eol=lf as the backstop.
Tests:
- Path-separator assertions rebuilt with path.join/basename/isAbsolute and
separator-normalized comparisons (index-marker-store, graph/*, cli-*,
skillify-*, config, pre-tool-use, plugin-cache, summary-state, ...).
- Subprocess env stubs now set USERPROFILE (+ file:// href) so os.homedir
resolves the temp home on Windows.
- skipIf(win32) for genuinely POSIX-only tests (embed-daemon unix-socket IPC,
safe-echo /bin/bash, Unix file-mode bits, 2 .mjs-direct-spawn stage-memory
tests already covered cross-platform by stage-memory-shell-spawn).
…erage
Product (real cross-platform fixes):
- Extract shared src/utils/atomic-write.ts (renameAtomic + isPathInsideHome)
from the duplicated copies in notifications/{queue,state}.ts; wire both +
hooks/summary-state.ts to it. renameAtomic retries the Windows-only
EPERM/EBUSY/EACCES rename, fixing the cross-process producer/bump/lock
tests (children no longer exit 1 on a transiently-open dest). Removes the
duplication (jscpd) too.
- notifications/state.ts: drop ':' from the claim-file id sanitizer — it's a
valid POSIX filename char but illegal on Windows, so tryClaim fric'd
fail-open every call.
- hooks/shared/autoupdate.ts: findHivemindOnPath splits PATH on
path.delimiter and probes .cmd/.exe on win32 (was ':'-split + bare name).
Tests:
- notifications.test.ts cross-process + bundle subprocesses: import via a
file:// .href URL (not /C:/… pathname), pass USERPROFILE to the child, and
shell-spawn npx on win32.
- atomic-write.test.ts: full unit coverage of renameAtomic (retry/throw/
exhaust/default seams) + isPathInsideHome edge cases.
- skillify-gate-runner.test.ts: restore runGate spawn coverage via POSIX
shebang fixtures (skipIf win32) — the buildArgs argv contract stays
cross-platform.
- Straggler fixes (Windows): autoupdate PATH probe, index-marker path
assertion, skillify-cli EBUSY teardown (chdir-out + rmSync retries),
skillify-manifest mode-bits assertion guard, plugin-cache + cli-embeddings
skipIf(win32) for fs-mode / unix-socket-only paths.
- vitest.config.ts: add atomic-write.ts threshold (90/90/90/90).
- notifications + summary-state cross-process tests: run the inline child via
a temp .mts file + 'node --import tsx' (process.execPath, absolute, no
shell) instead of 'npx tsx -e <code>'. On Windows npx needs a shell and
cmd.exe mangled the multi-line code arg ('Transform failed'); the file +
execPath form is arg-mangle-proof and shell-free on every platform.
- skillify-cli --to project: build the expected destination from
process.cwd() (what the product uses) rather than the mkdtemp dir, which
can differ from cwd on Windows (8.3 short path vs long form).
…rators)
The '--to project' destination hardcoded `${process.cwd()}/.claude/skills`,
producing mixed separators on Windows (C:\...\Temp\dir/.claude/skills) and
diverging from the 'global' branch's join(). Use join() so the path uses the
native separator. Identical output on POSIX.
All Windows tests pass, but per-file coverage thresholds were failing the job: suites that are platform-skipped on Windows (skipIf(win32) for the Unix-socket embed daemon, /bin/sh safe-echo, Unix file-mode bits, gate-runner spawn, plugin-cache fs-mode injection) can't cover their src on Windows by design. Coverage is already enforced + reported on the Linux 'test' job; the Windows job's purpose is catching Windows-only test failures. Drop --coverage there so platform skips don't read as coverage regressions.
scripts/pack-check.mjs ran execFileSync('npm', ...) which can't launch the
npm.cmd shim on Windows (ENOENT) — failing the Windows job's Pack-check step
once the test step started passing. Use the platform-correct binary name.
execFileSync can't launch the npm.cmd shim directly on modern Node (EINVAL). Route through the shell on Windows; the npm pack args are static (no injection surface). Unix stays shell-free.
Drops the branch from on.push.branches that was added to drive the push-only windows-test job during development. The job is back to running on main/dev push only. Windows green was verified on this branch at b37fcec (all jobs pass).
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (70)
Disabled knowledge base sources:
📝 WalkthroughWalkthroughAdds Windows CI support by enforcing LF line endings via ChangesWindows Compatibility
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 9 files changed
Generated for commit 2872864. |
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
1 participant
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.
Make the Windows CI suite green (without regressing Linux/macOS)
The
Typecheck and Test (Windows)job had been red on everymainpush since it was added — 299 failing tests across 53 files. This brings it to green, with the Linuxtestjob verified green at every step.Result
b37fcec7, all jobs pass).Root causes & fixes
1.
os.homedir()ignored faked$HOMEon Windows (the big one — hundreds of failures). It reads%USERPROFILE%there, not$HOME. Newtests/shared/fake-home.ts(setFakeHome/clearFakeHome) sets all four home vars; adopted across 27 test files. Behavior-neutral on POSIX.2. Real cross-platform product bugs (fixed, benefit actual Windows users):
notifications/{queue,state}.ts: home-containment usedstartsWith(home + "/")— rejected every write on Windows (resolve()emits\). Nowpath.relative-based, extracted with the atomic-rename into sharedsrc/utils/atomic-write.ts.atomic-write.ts:renameSyncretries on Windows EPERM/EBUSY/EACCES (transiently-open dest) — fixes cross-process queue/state/summary-state writes. Wired intosummary-state.tstoo. Removes prior duplication.notifications/state.ts: dropped:from the claim-file sanitizer (illegal in Windows filenames →tryClaimfailed open every call).skillify.ts:--to projectdest used a hardcoded/.claude/skills→path.join.install-hermes.ts,spawn-mine-local-worker.ts,autoupdate.ts: separator normalization /wherevswhich/path.delimiter+.cmd/.exeprobing.scripts/{sync-versions,pack-check}.mjs: CRLF-shebang import break (+.gitattributes eol=lf);npmspawn via shell on Windows (.cmdEINVAL).3. Test portability: path assertions rebuilt with
path.join/basename/isAbsolute; subprocess env carriesUSERPROFILE; inline-TS children run via a temp file +node --import tsx(process.execPath, no shell —npx tsx -emangled the code under cmd.exe);process.cwd()vs 8.3 short-path fixes.4. POSIX-only suites skipped on Windows (with comments): Unix-socket embed daemon,
/bin/shsafe-echo, Unix file-mode bits, and a couple of fs-mode-injection paths.5. CI: the
windows-testjob no longer runs--coverage— it exists to catch Windows-only failures, and the platform-skips above make per-file thresholds unsatisfiable there by design. Coverage remains fully enforced + reported on the Linux job (new code is covered:atomic-write.ts100/95/100/100,runGatecoverage restored via POSIX fixtures).Notes
atomic-write.ts;runGatespawn coverage restored underskipIf(win32).on.push.branchestrigger used to drive the push-only Windows job during development has been reverted.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Chores