fix(windows): run dev-install's external commands without a shell - #393
Conversation
![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.
One finding on the Windows path — the claude executable is now resolved to a single hardcoded suffix, which is the defect #390 removed from bin/cli.js. Everything else (argv conversion, where.exe, planShimSpawn reuse, spying on execFileSync before dev-install destructures it) looks correct.
| * second command. | ||
| */ | ||
| function runCommand(command, args, options = {}) { | ||
| const plan = planShimSpawn(resolveExecutableForPlatform(command), args); |
There was a problem hiding this comment.
This is an auto review done by revuto.
runCommand('claude', ...) resolves the executable purely through resolveExecutableForPlatform, which maps claude → claude.cmd on win32 (lib/utils/command-parser.js:9,45-47). That is exactly the resolution #390 removed from the installer: per CHANGELOG.md:18 and bin/cli.js:77-98, claude.cmd does not exist for native-installer users (they have claude.exe), and __tests__/cli-args.test.js:364-366 asserts the CLI never hardcodes one Windows suffix.
Concretely on Windows with a native Claude install: commandExists('claude') succeeds (it asks where.exe, which finds claude.exe), then runCommand('claude', ...) spawns cmd.exe /d /s /c ""claude.cmd" ...", which fails; both call sites (scripts/dev-install.js:293 and :304) swallow the error in an empty catch, so the marketplace removal and the plugin uninstalls silently no-op and a marketplace copy can stay installed alongside the dev copy. The old execSync('claude plugin marketplace remove ...') worked here because cmd.exe applied PATHEXT and found claude.exe, so this is a behaviour regression on that host, not just a lost shell.
bin/cli.js already exports pickClaudeExecutable / claudeExecutable for this; routing claude through the same where.exe-based pick (or, minimally, falling back to a direct spawn when the shim is absent) would keep the two mechanisms from drifting apart, which was the stated goal of #391.
dev-install shelled out through execSync for all four of its external commands. On Windows that hid two problems and created a third. `claude` and `npm` are .cmd shims there. execSync worked only because it routes through cmd.exe implicitly; the moment the command is anything but a shell string it needs the explicit hop planShimSpawn builds, since Node has refused direct .cmd spawns since the CVE-2024-27980 fix and execFileSync applies no PATHEXT to a bare name. The plugin uninstall line also interpolated a discovered plugin name into a shell string. The name is checked against /^[a-z0-9][a-z0-9-]*$/ first, so nothing could reach it today, but the check and the interpolation sat in different functions - the guard is the only thing that made the line safe. As an argv element it is safe by construction. Commands are now argv lists through one runCommand helper, so every call site gets shim resolution and the cmd.exe hop. Tests cover the win32 and non-win32 plans, the cwd passthrough, and that a plugin name holding shell metacharacters stays a single argument.
111b986 to
e2a6c5a
Compare
…ixed suffix (#394) * fix(windows): resolve dev-install's claude through where.exe, not a fixed 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. * fix(windows): treat cmd.exe exit 9009 as a claude that never ran 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
scripts/dev-install.jsran all four of its external commands throughexecSync, i.e. through a shell. On Windows that hid two defects and created a third:claudeandnpmare.cmdshims there.execSynconly worked because it goes throughcmd.exeimplicitly - the same call written as an argv spawn fails twice over, once becauseexecFileSyncapplies no PATHEXT to a barenpm, and once because Node has refused direct.cmdspawns since the CVE-2024-27980 fix. This is the same defect class fixed at five other call sites in fix(windows): route .cmd shims through cmd.exe in dev and perf runners #391; this script was the last shell-string holdout.commandExistsranwhere ${cmd}/which ${cmd}as a shell string.claude plugin uninstall ${plugin}@${suffix}.On (3): nothing can reach it today -
pluginis checked against/^[a-z0-9][a-z0-9-]*$/immediately before the loop, so a directory nameda & calcis skipped rather than run. But the guard and the interpolation live in different places, and the guard is the only thing making that line safe. As an argv element it is safe by construction.What
One
runCommand(command, args, options)helper:resolveExecutableForPlatformsupplies the Windows extension,planShimSpawnbuilds thecmd.exe /d /s /chop when the target is a shim,shimSpawnOptionsaddswindowsVerbatimArguments. All four sites go through it. No behaviour change on Linux or macOS beyond losing the shell.Test plan
__tests__/dev-install.test.jsgains cases for:claude(cmd.exe+ quoted payload + verbatim flag) and the direct spawn off Windowsnpm.cmdresolution with thecwdpreserved&staying one argv elementcommandExistsusingwhere.exeon Windows,whichelsewhere, and reportingfalsewhen the lookup throwsexecSync(remainsnpm test: 88 suites, 3574 passing.npm run validate: clean.Windows CI coverage arrives with #392; this branch will be rebased onto it before merge.