fix(scripts): split cross-OS launcher into POSIX + Windows files#159
Merged
Conversation
The polyglot launcher (PR #145, PR #157) cannot satisfy both POSIX direct exec and zero Windows stdout pollution at byte 0: - POSIX kernel needs `#!` shebang for direct exec via posix_spawn (libuv 1.51.0 does NOT fall back to /bin/sh on ENOEXEC). - cmd.exe needs `@`/`:` to keep line 1 off stdout, which corrupts MCP stdio JSON-RPC framing. These are mutually exclusive. Replace the single-file polyglot with two files dispatched by extension — the same pattern npm cmd-shim, yarn, pnpm, and the MCP reference servers all use: scripts/run POSIX, byte 0 = "#!/usr/bin/env bash", +x scripts/run.cmd Windows, byte 0 = "@echo off" All plugin/hook/MCP configs reference the extensionless `scripts/run`. POSIX kernel finds it directly. Windows cmd.exe and cross-spawn (the package Claude Code's MCP SDK uses) resolve `.cmd` via PATHEXT for absolute extensionless paths — verified against ReactOS where.c and npm/node-which sources, not guessed. New regression guards close the CI gap that masked PR #145 in production: - scripts/test_run_spawn.js — spawns the launcher via child_process.spawn(..., { shell: false }) and parallel cross-spawn variant. This is the EXACT transport Claude Code uses; previous CI invoked /bin/sh as a wrapper, which is why the macOS posix_spawn ENOEXEC failure went undetected. - scripts/test_run_dispatch_windows.bat — strict byte-equality on stdout proves zero echoed lines from the launcher itself. - scripts/test_run_windows.ps1 — first-install MCP handshake test, retargeted at the renamed run.cmd. Validated end-to-end in real Claude Code on macOS: SessionStart hook emits clean JSON via /bin/sh -c (shell:true spawn), MCP stdio server emits byte-exact JSON-RPC frames via posix_spawn (shell:false spawn). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR migrates the Lumen launcher from Windows-centric delegation stubs to a unified cross-platform approach. The extensionless ChangesLauncher Migration to Cross-Platform Unified Entry Point
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
The first revision of test_run_spawn.js asserted child_process.spawn
with shell:false on Windows, which failed with ENOENT because libuv's
CreateProcess does not PATHEXT-resolve absolute paths. That is not a
production code path Claude Code uses on Windows, so asserting it was
testing the wrong invariant.
Production transports per OS:
- macOS / Linux MCP: cross-spawn (~= raw spawn shell:false) →
libuv posix_spawn → kernel direct-execs the shebang launcher.
- Windows MCP: cross-spawn → PATHEXT + cmd.exe /d /s /c wrap.
- Hooks (all OSes): child_process.spawn with shell:true →
/bin/sh -c (POSIX) or cmd.exe /d /s /c (Windows).
The test now mirrors those exactly:
- POSIX only: shell:false (the macOS posix_spawn bug guard).
- All OSes: shell:true (the hook dispatch primitive).
- All OSes: cross-spawn (the MCP SDK transport).
Also simplifies the Windows mock launcher to a fixed JSON literal so
strict byte-equality works without fighting cmd.exe quote escaping.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
scripts/test_run_spawn.js (1)
106-109: ⚡ Quick winConsider validating argument values, not just count.
The current check verifies that
parsed.args.length === args.length, but doesn't validate that the actual argument values match. This could miss bugs where arguments are corrupted or reordered.For a more robust test, consider:
if (!Array.isArray(parsed.args) || parsed.args.length !== args.length) { failMsg(`${label}: argv length mismatch`); console.log(` expected ${args.length} args, got ${JSON.stringify(parsed.args)}`); return resolve(); } // Validate each argument value for (let i = 0; i < args.length; i++) { if (parsed.args[i] !== args[i]) { failMsg(`${label}: argv[${i}] value mismatch`); console.log(` expected ${JSON.stringify(args[i])}, got ${JSON.stringify(parsed.args[i])}`); return resolve(); } }However, for a spawn-semantics smoke test, the current length-only check may be acceptable.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/test_run_spawn.js` around lines 106 - 109, The test currently only checks parsed.args.length against args.length which misses reordered or corrupted values; update the check in the block that references parsed.args, args, failMsg, label, and resolve to also iterate over args and compare each parsed.args[i] === args[i], and on mismatch call failMsg(`${label}: argv[${i}] value mismatch`) and log both expected and actual with JSON.stringify before returning resolve(); keep the existing length check but change its message to "argv length mismatch" if you want clearer logging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hooks/hooks-cursor.json`:
- Line 6: The command value for the "command" key uses an unquoted launcher path
(${CURSOR_PLUGIN_ROOT}/scripts/run ...) which will break if the resolved path
contains spaces; update the JSON "command" value so the launcher path is quoted
(match the quoting style used in other hooks.json entries) so the entire command
tokenizes correctly — locate the "command" entry containing
${CURSOR_PLUGIN_ROOT}/scripts/run hook session-start lumen --host cursor and
wrap the launcher path (or the full command string) in quotes.
In `@scripts/run.cmd`:
- Around line 55-95: The download logic treats any curl failure as "version not
found" and writes partial EXEs to %BINARY%; change it to (1) download to a temp
file (e.g. "%BINARY%.partial") instead of writing directly to "%BINARY%" and
only move/rename the temp file to "%BINARY%" on success, and (2) detect an
explicit 404 before falling back to the GitHub releases/latest flow by capturing
the HTTP status (use curl -w "%{http_code}" -o "<temp>" or an equivalent) and
only proceed to the fallback block when the status is 404; ensure the fallback
download uses the same temp-file pattern and only replaces %BINARY% after a
successful download. Make these changes around the curl invocations that
reference !URL!, "%BINARY%", and the fallback assembly logic that sets VERSION,
ASSET, LATEST_TAG, TMPJSON, and AUTH_HEADER.
In `@scripts/test_run_dispatch_windows.bat`:
- Line 1: Convert the batch script so it uses CRLF line endings instead of bare
LF; update the file containing the "@echo off" line to have Windows-style CRLF
endings (you can re-save the file in the editor with CRLF or run a conversion
tool) and ensure the repository preserves CRLF for .bat files (e.g., adjust
.gitattributes if needed) so the script parses reliably on Windows.
- Around line 98-117: The test never checks the run dispatch exit code or the
empty-output case, so capture %ERRORLEVEL% immediately after the cmd /c
"%~dp0run" call (e.g., set "RUN_EXIT=%ERRORLEVEL%") and if RUN_EXIT is not 0,
echo a FAIL with the exit code and contents of "%REAL_STDERR%" and increment
FAIL (set /a FAIL+=1) instead of continuing; additionally, treat an empty
FIRST_CHAR as a failure by checking if FIRST_CHAR is defined before relying on
its first character (if not defined, echo FAIL and increment FAIL), leaving the
existing STARTS_WITH_AT logic only for non-empty FIRST_CHAR.
In `@scripts/test_run_spawn.js`:
- Around line 46-56: The WINDOWS_LAUNCHER test mock builds JSON with incorrectly
escaped quotes using backslashes (the WINDOWS_LAUNCHER template), which yields
literal backslashes and invalid JSON; change the batch-style escaping so the
quoted argument is produced using doubled quotes inside the SET assignment (use
""%~1"" instead of \\"%~1\\" in the ARGS construction and any corresponding
places), ensuring the emitted echo line outputs valid JSON like
{"mock":"ok","args":["stdio"]} that can be parsed by JSON.parse().
---
Nitpick comments:
In `@scripts/test_run_spawn.js`:
- Around line 106-109: The test currently only checks parsed.args.length against
args.length which misses reordered or corrupted values; update the check in the
block that references parsed.args, args, failMsg, label, and resolve to also
iterate over args and compare each parsed.args[i] === args[i], and on mismatch
call failMsg(`${label}: argv[${i}] value mismatch`) and log both expected and
actual with JSON.stringify before returning resolve(); keep the existing length
check but change its message to "argv length mismatch" if you want clearer
logging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cce48ef6-ac50-4299-a3f0-e3adb740b967
📒 Files selected for processing (21)
.claude-plugin/plugin.json.codex/INSTALL.md.cursor-plugin/INSTALL.md.cursor/mcp.json.github/workflows/ci.yml.gitignore.opencode/INSTALL.md.opencode/plugins/lumen.jsREADME.mdhooks/hooks-cursor.jsonhooks/hooks.jsonscripts/package.jsonscripts/runscripts/run.batscripts/run.cmdscripts/test_run.shscripts/test_run_cmd.shscripts/test_run_cmd_windows.batscripts/test_run_dispatch_windows.batscripts/test_run_spawn.jsscripts/test_run_windows.ps1
💤 Files with no reviewable changes (3)
- scripts/test_run_cmd.sh
- scripts/run.bat
- scripts/test_run_cmd_windows.bat
Comment on lines
+55
to
+95
| call curl -sfL --max-time 300 --retry 3 --retry-delay 2 "!URL!" -o "%BINARY%" | ||
| if errorlevel 1 ( | ||
| :: Fallback: manifest version not released yet — resolve latest from GitHub API | ||
| echo Version !VERSION! not found, resolving latest release... >&2 | ||
|
|
||
| set "AUTH_HEADER=" | ||
| if defined GITHUB_TOKEN set "AUTH_HEADER=-H "Authorization: token %GITHUB_TOKEN%"" | ||
|
|
||
| set "TMPJSON=%TEMP%\lumen-latest.json" | ||
| call curl -sfL !AUTH_HEADER! --max-time 30 --retry 2 --retry-delay 2 ^ | ||
| "https://api.github.com/repos/!REPO!/releases/latest" -o "!TMPJSON!" | ||
|
|
||
| set "LATEST_TAG=" | ||
| for /f "tokens=2 delims=:" %%a in ('findstr /r "tag_name" "!TMPJSON!"') do ( | ||
| set "LATEST_TAG=%%~a" | ||
| set "LATEST_TAG=!LATEST_TAG: =!" | ||
| set "LATEST_TAG=!LATEST_TAG:,=!" | ||
| set "LATEST_TAG=!LATEST_TAG:"=!" | ||
| ) | ||
| del "!TMPJSON!" 2>nul | ||
|
|
||
| if "!LATEST_TAG!"=="" ( | ||
| echo Error: could not resolve latest release from GitHub API >&2 | ||
| exit /b 1 | ||
| ) | ||
| echo !LATEST_TAG! | findstr /r "^v[0-9]" >nul 2>&1 | ||
| if errorlevel 1 ( | ||
| echo Error: resolved tag "!LATEST_TAG!" does not look like a version >&2 | ||
| exit /b 1 | ||
| ) | ||
|
|
||
| echo Falling back to !LATEST_TAG!... >&2 | ||
| set "VERSION=!LATEST_TAG!" | ||
| set "ASSET=lumen-!VERSION:~1!-windows-!ARCH!.exe" | ||
| set "URL=https://github.com/!REPO!/releases/download/!VERSION!/!ASSET!" | ||
|
|
||
| call curl -sfL --max-time 300 --retry 3 --retry-delay 2 "!URL!" -o "%BINARY%" | ||
| if errorlevel 1 ( | ||
| echo Error: fallback download also failed >&2 | ||
| exit /b 1 | ||
| ) |
There was a problem hiding this comment.
Only fall back to releases/latest on an explicit “not found”, and keep failed downloads out of %BINARY%.
This block currently treats every pinned-download failure as “version not found”. A transient 5xx/network error will silently install whatever latest is, which can desync the launcher from the manifest-pinned distribution. On top of that, curl -o "%BINARY%" can leave a partial EXE behind; the next run then skips the download block entirely and tries to execute a corrupt file.
💡 Safer shape for the download path
- call curl -sfL --max-time 300 --retry 3 --retry-delay 2 "!URL!" -o "%BINARY%"
- if errorlevel 1 (
+ set "TMPBIN=%BINARY%.tmp"
+ del /f /q "!TMPBIN!" 2>nul
+ for /f %%s in ('curl -sSL -o "!TMPBIN!" -w "%%{http_code}" --max-time 300 --retry 3 --retry-delay 2 "!URL!"') do set "HTTP_STATUS=%%s"
+ if "!HTTP_STATUS!"=="200" (
+ move /y "!TMPBIN!" "%BINARY%" >nul
+ ) else (
+ del /f /q "!TMPBIN!" 2>nul
+ if not "!HTTP_STATUS!"=="404" (
+ echo Error: failed to download pinned release (!HTTP_STATUS!) >&2
+ exit /b 1
+ )
:: Fallback: manifest version not released yet — resolve latest from GitHub APIApply the same temp-file pattern to the fallback download below as well.
As per coding guidelines, "Keep .claude-plugin/, .codex/, .cursor-plugin/, .cursor/, .opencode/, hooks/, skills/, scripts/, and package.json aligned where they represent the same Lumen distribution".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/run.cmd` around lines 55 - 95, The download logic treats any curl
failure as "version not found" and writes partial EXEs to %BINARY%; change it to
(1) download to a temp file (e.g. "%BINARY%.partial") instead of writing
directly to "%BINARY%" and only move/rename the temp file to "%BINARY%" on
success, and (2) detect an explicit 404 before falling back to the GitHub
releases/latest flow by capturing the HTTP status (use curl -w "%{http_code}" -o
"<temp>" or an equivalent) and only proceed to the fallback block when the
status is 404; ensure the fallback download uses the same temp-file pattern and
only replaces %BINARY% after a successful download. Make these changes around
the curl invocations that reference !URL!, "%BINARY%", and the fallback assembly
logic that sets VERSION, ASSET, LATEST_TAG, TMPJSON, and AUTH_HEADER.
Three changes flagged on the initial commit: 1. hooks/hooks-cursor.json: quote the launcher path so it matches the already-quoted form in hooks/hooks.json. Future-proofs against plugin installs under paths with spaces. 2. scripts/test_run_dispatch_windows.bat: Test 4 captured no exit code from the real-launcher dispatch, so a failing dispatch with empty stdout would silently PASS. Capture %ERRORLEVEL% and fail loudly on non-zero before falling through to the @-echo assertion. 3. .gitattributes: pin .bat/.cmd to CRLF on checkout. Windows batch parser has documented 512-byte boundary bugs with bare-LF files; this guarantees correct line endings regardless of the user's core.autocrlf setting. The other two CodeRabbit comments (test_run_spawn.js argv escaping + length validation) are stale — already addressed in commit 19be4ee which replaced argv reflection with a fixed JSON literal. The curl download-path concern is real but out of scope; tracked as a follow-up to harden run + run.cmd symmetrically. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Test 4 invoked `lumen --version` but lumen uses `version` as a subcommand — `--version` is not a registered flag, so cobra exited 1 with its usage help on stderr. The new exit-code capture surfaced it on the first Windows CI run. Stdout was empty (proving `@echo off` works correctly and run.cmd doesn't leak source lines), but the test now fails loudly on the non-zero exit code as intended. Switching to `version` gives exit 0 + clean stdout — the assertion we actually want. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
scripts/run(POSIX, shebang) andscripts/run.cmd(Windows,@echo off). All plugin/hook/MCP configs reference the extensionlessscripts/run.scripts/test_run_spawn.js— the regression guard that catches what every prior CI run missed: it invokes the launcher with the EXACT spawn semantics Claude Code's MCP SDK uses (child_process.spawnwithshell: false, plus cross-spawn variant), not a /bin/sh wrapper.scripts/test_run_cmd_windows.batwithscripts/test_run_dispatch_windows.bat— strict byte-equality on stdout proves zero echoed lines from the launcher itself.Why two files
A single-file polyglot cannot satisfy both POSIX direct exec and zero Windows stdout pollution at byte 0:
#!shebang. macOS Node 22's libuv (1.51.0) usesposix_spawn, which does NOT fall back to/bin/shon ENOEXEC. PR fix(scripts): eliminate stdout pollution in run.cmd polyglot #145's:<<"::CMDLITERAL"polyglot lacked a shebang and exploded on macOS.@/:on line 1 to keep that line off stdout. MCP stdio JSON-RPC has zero tolerance for the resulting pollution.These two constraints are mutually exclusive at byte 0. Same pattern used by npm cmd-shim, yarn, pnpm, and the MCP reference servers.
PATHEXT resolution of absolute extensionless paths is verified against authoritative sources, not guessed:
cross-spawnwhich.sync()(Claude Code's MCP SDK transport) — npm/node-which sourcecmd.exe /c— ReactOSwhere.c:SearchForExecutable+ Microsoft docsTest plan
Local validation (macOS, completed before push):
bash scripts/test_run.sh— 31/31 passnode scripts/test_run_spawn.js— both child_process.spawn and cross-spawn variants pass with byte-exact JSONmake build-local— producesbin/lumenclaude --plugin-dir .→ SessionStart hook emits clean JSON via/bin/sh -c(shell:true spawn); MCP stdio server emits byte-exact JSON-RPC viaposix_spawn(shell:false spawn). Verified against~/.local/share/lumen/debug.log"lumen config" line and indexing-complete trail.CI matrix (ubuntu-latest, macos-latest, windows-latest):
bash scripts/test_run.sh(Unix)cmd /c scripts\test_run_dispatch_windows.bat(Windows): proves cmd.exe PATHEXT resolution + zero stdout pollutionpwsh scripts/test_run_windows.ps1(Windows): MCP handshake test against renamed run.cmdnode scripts/test_run_spawn.js(all 3 OSes): spawn-semantics regression guard — the bar that caught PR fix(scripts): eliminate stdout pollution in run.cmd polyglot #145 only in productionWhy this won't regress like PR #145 / PR #157
child_process.spawn(..., { shell: false }), which on macOS goes throughposix_spawndirectly. PR fix(scripts): eliminate stdout pollution in run.cmd polyglot #145 passed CI by being invoked through/bin/sh launcherworkarounds; that workaround is gone.🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
Tests
Documentation
CI