feat(#67): add Windows and macOS legs to CI - #75
Merged
Conversation
.github/workflows/ci.yml's shell job ran on ubuntu-latest only, despite all five hooks being pure POSIX sh and the README making no OS-specific claim. Claude Code executes a command hook under Git Bash on native Windows, a meaningfully different environment (NTFS permission semantics, no default symlink privilege) than Ubuntu's bash - this converts that unmade platform claim into a made, tested one, matching the coverage a competing plugin (adrrr/persistent-handoff) already has. Changes needed to make tests/run.sh actually pass on Windows/Git Bash: - Three permission-based assertions stage a POSIX mode bit (000 unreadable, 555 read-only directory) that NTFS's chmod emulation can't reliably enforce the same way ext4/APFS do. Guarded with a new is_windows() helper (checks MSYSTEM, which only Git Bash/MSYS2 sets) alongside the existing root-bypasses-permissions skip, reporting how many were skipped in the final summary rather than the pass count silently reading lower with no explanation. - The missing-jq test's PATH stub swapped ln -sf for a tiny exec wrapper script per tool: a symlink needs a privilege Windows doesn't grant by default, and (caught by testing this locally before pushing) a `cp` of the real binary is actively worse - a macOS Homebrew binary can resolve its shared libraries via an @executable_path-relative reference, which hangs or crashes once relocated to the stub directory. A `#!/bin/sh -c 'exec <real> "$@"'` wrapper needs no privilege beyond writing a text file and never touches the real binary's own location. - Added .gitattributes forcing LF on *.sh: GitHub's Windows runner image defaults core.autocrlf to true, and a CRLF-mangled shebang or an embedded \r mid-line breaks a shell script's execution outright. The one chmod-based test NOT guarded (444 read-only file, blocking a write) is left as-is - Windows' read-only DOS attribute plausibly does block writes the way NTFS can't block reads, so this is a real test of real Windows behavior, not a guess dressed as one. Live CI will confirm or correct that. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXL2ffVDsAp4P7387Nr18G
The Windows leg of this PR's own CI failed on first push (172 passed, 3 failed) - exactly the outcome the PR was honest about not being able to verify locally. /review-pr independently found the same two root causes by reading the live CI output, plus two hardening gaps in the fix itself: 1. session-capture.sh passed the project root to jq as `--arg root "$root"`, then string-compared it against a captured file_path delivered via stdin. On Windows, jq is a native (non-MSYS) binary, and MSYS auto-converts a POSIX-looking ARGUMENT to Windows form before a native executable sees it - so $root arrived pre-converted while file_path didn't, and the two never matched. Every Edit/Write/NotebookEdit path onboard wrote stayed unrelativized on Windows: an absolute, OS-shaped path leaking into the buffer. First attempted fix (MSYS_NO_PATHCONV=1, global) was wrong and never shipped past local testing: the review caught that it would have broken session-onboard.sh's own `jq ... plugin.json` read, which legitimately NEEDS argv conversion to open that file on Windows. The actual fix keeps $root out of argv entirely - passed as an environment variable (`root="$root" jq ...`, referenced as `env.root`), which sidesteps MSYS's argv-only conversion without an opt-out that breaks something else. 2. A new test (7b3, from the earlier commit today) built a ~300-character path to force a git-status line past the truncation threshold. Windows' MAX_PATH (260) doesn't apply to a bare mkdir under Git Bash, but DOES apply to git's own add/commit machinery, so the fixture never staged. Shortened the path to a size that still exceeds the 200-char truncation threshold with margin while staying under 260, and added `git config core.longpaths true` as a second line of defense against whatever the real runner's own temp-path depth turns out to be. Also from the same review pass: - is_windows() checked only $MSYSTEM, which is set by the Git-for-Windows wrapper bash.exe, not the MSYS runtime itself - anything invoking usr/bin/sh.exe or usr/bin/bash.exe directly would leave it unset and the permission-test skips would silently stop firing. Added `uname -s` (MINGW*/MSYS*/CYGWIN*) and $OS=Windows_NT as backstops that come from the kernel/process environment directly. - Write and NotebookEdit path relativization only asserted the relativized form was PRESENT, not that the absolute prefix was ABSENT - the same gap that let the Edit-path bug above ship unnoticed for those two tool types too (they share the identical code path and failed identically on Windows). Added the missing negative assertions. 174 -> 176 passing tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXL2ffVDsAp4P7387Nr18G
The previous commit's fix (root="$root" jq ..., referenced as env.root) looked right by MSYS documentation but didn't survive contact with the real runner: the second CI push failed identically - all three of Edit, Write, and NotebookEdit still had their absolute prefix intact. Whatever channel is actually doing the conversion on this runner, both an argv value AND an environment variable turned out to go through it. Stopped trying to find a channel MSYS won't touch and removed the channel entirely: $root is now embedded as a JSON string LITERAL inside the jq PROGRAM TEXT itself, computed via a separate `jq -Rs .` call fed over stdin (the one channel already confirmed immune, since the file_path it's compared against uses the same one). The whole filter argument - starting with `def outcome($t): ...` and hundreds of characters long - doesn't remotely resemble "a path" as a single argument, which is what MSYS's conversion heuristic actually keys on; the value never appears as its own argv item or env var for anything to convert. Verified locally against a path containing a space, then again against one with an embedded double quote and backslash (jq -Rs . correctly escapes both), so this isn't fragile the way string-concatenation-into-a-shell- command usually is. This is now the third fix attempt for the same underlying symptom, each verified locally and each pushed to find out against the one environment that actually matters. Real Windows CI is watching this push. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UXL2ffVDsAp4P7387Nr18G
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.
Summary
.github/workflows/ci.yml's shell job ran onubuntu-latestonly, despite all fivehooks being pure POSIX
shand the README making no OS-specific claim. Claude Codeexecutes a command hook under Git Bash on native Windows - a meaningfully different
environment than Ubuntu's bash.
windows-latestandmacos-latestto the shell job's OS matrixshellcheck/manifest-validation/version-agreement stay Linux-only (static analysis,the result doesn't depend on OS) -
sh tests/run.shruns on all threeis_windows()helper (checksMSYSTEM,uname -s, and$OS) skips 4permission-based test assertions that stage a POSIX mode bit NTFS can't reliably
enforce, alongside the existing root-bypasses-permissions skip - reported in the
final summary
ln -sffor a tiny exec-wrapper script in the missing-jq test's PATH stub -caught locally before pushing that a plain
cpof the real binary can hang/crash onmacOS (a Homebrew binary resolving shared libs via an
@executable_path-relativereference breaks once relocated)
.gitattributesforcing LF on*.sh(GitHub's Windows runner defaultscore.autocrlfto true, which would otherwise corrupt every shebang)session-capture.shpassed the project root to jq as--arg root "$root", thenstring-compared it against a captured
file_pathdelivered over stdin. On Windows,jq is a native binary, and MSYS auto-converts a POSIX-looking value to Windows form
before a native executable sees it - so
$rootnever matched and everyEdit/Write/NotebookEdit path stayed unrelativized (an absolute, OS-shaped path
leaking into the buffer). Two other fix attempts (a blanket
MSYS_NO_PATHCONV=1,then passing
$rootas an environment variable) both failed identically againstreal CI before landing on the actual fix: embedding
$rootas a JSON string literalinside jq's program text itself, which never crosses the boundary that was being
converted.
This genuinely was not verifiable locally (no Windows environment available) - the
Windows leg failed twice against real CI before this third attempt passed. Each
attempt was diagnosed from the actual failing CI logs, not guessed.
Test plan
sh tests/run.sh: 176/176 pass locally on macOSshellcheck -s sh hooks/*.sh tests/*.sh: cleanlocal-ci.sh --strict: all 5 checks PASSshell (windows-latest),shell (macos-latest),shell (ubuntu-latest),opencode-pluginall pass on thefinal commit
/review-pr, which caught two real gaps in the first fix attempt(the env-var approach would have broken the plugin-version-header jq call, and
two of the three new assertions couldn't have caught this bug in the first
place) before either shipped
and separately against one with an embedded double quote and backslash
Closes #67