fix(windows): resolve dev-install's claude through where.exe, not a fixed suffix - #394
Conversation
…ixed suffix dev-install's four external commands became argv spawns in #393, and the claude ones resolved the executable with resolveExecutableForPlatform, which maps a bare `claude` to `claude.cmd` on win32. That is the assumption #390 removed from bin/cli.js: the npm global install does ship claude.cmd, but the native installer ships claude.exe, and there commandExists('claude') passes while the spawn of claude.cmd fails - with both call sites catching and discarding the error, so the marketplace removal and every plugin uninstall silently did nothing. The where.exe-based pick moves out of bin/cli.js into lib/utils/claude-executable.js so both callers get the same answer and cannot drift apart; cli.js imports it rather than keeping a copy. Its unit tests move to __tests__/claude-executable.test.js with the module, and cli.js stops exporting the two functions it no longer owns. The empty catches also become a warning. Both removals are best-effort - nothing to remove exits non-zero and that is normal - so the two cases are told apart by what execFileSync reports: a numeric status means claude ran and refused, while a spawn failure leaves status null and carries the errno code. Only the second is worth a line, and only the first one of them. Tests cover the native .exe spawning directly with no cmd.exe hop, the .cmd shim taking the hop, the posix path, where.exe failing entirely, and the warning appearing for a spawn failure but not for a non-zero exit.
![revuto-review[bot]](https://avatars.githubusercontent.com/u/55848801?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is an auto review done by revuto.
Reviewed the move of Claude executable resolution into lib/utils/claude-executable.js and the new best-effort/spawn-failure split in installClaude. The refactor itself looks consistent: bin/cli.js no longer needs resolveExecutableForPlatform (no other use remains), WINDOWS_BATCH_SHIM is exported with its CVE note intact, and the removed bin/cli.js exports had no consumers outside __tests__/cli-args.test.js, which the PR updates. One gap in the new warning logic on the exact host class this PR targets — inline.
| const reportIfNeverRan = (err) => { | ||
| // execFileSync sets a numeric status when the child ran and exited non-zero; | ||
| // a spawn failure leaves status null and carries the errno code instead. | ||
| if (typeof err.status === 'number' || spawnFailureReported) return; |
There was a problem hiding this comment.
This is an auto review done by revuto.
The typeof err.status === 'number' discriminator does not hold on the Windows/npm-shim host this PR is about.
runCommand sends claudeBin through planShimSpawn (line 76), and for a .cmd/.bat executable on win32 that rewrites the spawn to cmd.exe /d /s /c "..." (lib/utils/command-parser.js:120-150). So when claudeBin is claude.cmd, the process that is actually spawned is cmd.exe, which exists — a claude.cmd that cannot be launched (stale where.exe entry, wrong shim, missing target) makes cmd.exe exit non-zero (9009 / 1), i.e. err.status is a number and reportIfNeverRan returns early. The warning is only reachable when claudeBin is the direct-exec .exe/.com path or the posix claude, where execFileSync itself fails with status === null.
Net effect: on a claude.cmd host the "every removal became a silent no-op" case the PR is fixing still stays silent. The two installClaude tests that exercise the warning both run on linux, so nothing covers the shim path. If you want the warning to cover the shim too, the win32 .cmd case needs a cmd.exe-specific signal (e.g. treating exit 9009 as never-ran, or probing the resolved path with fs.existsSync before the removals) rather than relying on status === null.
The spawn-failure warning missed the host it was written for. On win32 a .cmd claude is launched through cmd.exe, so execFileSync sees cmd.exe start successfully and a shim it could not launch arrives as an exit code, not as a null status - which the discriminator read as "claude ran and refused" and stayed silent, exactly the silent no-op the warning exists to break. cmd.exe reports a command it cannot find or launch as 9009, so that code counts as never-ran, but only on the shim route: 9009 from a directly spawned claude.exe is the executable's own exit code and stays a normal best-effort failure. The reason string now carries the exit code when there is no errno to report. Tests cover a 9009 through the shim warning, a 1 through the shim staying quiet, and a 9009 from a .exe staying quiet.
Why
Follow-up on the review of #393. That PR turned dev-install's four external commands into argv spawns, and the two
claudeones resolved the executable withresolveExecutableForPlatform, which maps a bareclaudetoclaude.cmdon win32.That is exactly the assumption #390 removed from
bin/cli.js. The npm global install does shipclaude.cmd, but the native installer shipsclaude.exe. On such a host:commandExists('claude')passes, becausewhere.exeapplies PATHEXT and findsclaude.exe;claude.cmdthen fails, becauseexecFileSyncdoes not;scripts/dev-install.js:293and:304) caught the error and discarded it.So the marketplace removal and every plugin uninstall silently did nothing, and the run still reported success - a regression against the old
execSync, which got PATHEXT for free from cmd.exe.What changed
where.exe-based pick moves out ofbin/cli.jsintolib/utils/claude-executable.js.scripts/dev-install.jsneeds the same answer, and a second resolution that guesses the suffix drifts from the first the moment an install layout changes.cli.jsimports it instead of keeping a copy, and stops exporting the two functions it no longer owns.installClauderesolves once viaclaudeExecutable()and passes that to bothrunCommandcalls.execFileSyncreports: a numericstatusmeans claude ran and refused, while a spawn failure leavesstatusnull and carries the errnocode. Only the second is worth a line, and only the first one of them.WINDOWS_BATCH_SHIMis exported fromlib/utils/command-parser.js, carrying the CVE-2024-27980 note it had incli.js, since callers that choose between executables have to know which ones cost a cmd.exe hop.Tests
__tests__/claude-executable.test.js(new): the six resolution cases moved with the module, plus where.exe being asked exactly once, not asked at all off Windows, the fallback when where.exe itself fails, and the cache reset.__tests__/dev-install.test.js: five tests runninginstallClaudeagainst a stubbedchild_processand a scratchHOME, covering the native.exespawning directly with no cmd.exe call, the.cmdshim taking the hop, the posix path, the warning on a spawn failure, and silence on a plain non-zero exit.Local gate:
npm test89 suites / 3584 passed / 39 skipped,npm run validate,gen-docs:check,gen-adapters:check,expand-templates:checkall clean.