fix: resolve Claude Code executable from where.exe on Windows - #390
Conversation
execFileSync does not apply PATHEXT, so bin/cli.js resolved the Claude CLI via
resolveExecutableForPlatform('claude'), which hardcodes claude.cmd on win32.
That path only exists for npm global installs. Users who installed through the
native installer have claude.exe, so every `claude plugin` call raised ENOENT -
and because those calls sit inside `catch {}`, the CLI printed
"[OK] Installed ... successfully" while nothing reached Claude Code.
Resolve the concrete path from `where.exe claude` instead, keeping only
extensions CreateProcess can launch (.exe/.com/.cmd/.bat) so the extensionless
npm shell script and claude.ps1 are skipped, and fall back to the existing shim
mapping when nothing spawnable is found. POSIX behavior is unchanged.
Resolution is lazy and cached, so requiring bin/cli.js spawns no process.
Also:
- installPlugin no longer reports success when Claude Code rejected a plugin.
It names the plugins that failed and how to retry.
- lib/utils/command-parser.js carried a raw null byte where '\0' was intended.
Git and grep classified the file as binary, so changes to it could not be
reviewed as a diff. Normalized to LF, the only CRLF source file in the repo.
![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 Windows resolution change, the installPlugin failure reporting, and the command-parser.js de-binarization (confirmed with git diff --text --ignore-cr-at-eol: the only semantic delta is the raw U+0000 → '\0' literal, behavior-identical). One substantive concern on the candidate ordering in pickClaudeExecutable, inline.
Minor, non-blocking: the new message says "Claude Code rejected: …", but the same branch is reached when the executable cannot be spawned at all (ENOENT/EINVAL) — i.e. exactly the failure mode this PR is fixing. Including the caught error's code would make the next resolution gap diagnosable instead of looking like a Claude-side rejection.
|
|
||
| // Extensions CreateProcess can launch directly. .ps1 and the extensionless npm | ||
| // shell script also show up in `where.exe` output but cannot be spawned. | ||
| const WINDOWS_SPAWNABLE = /\.(exe|com|cmd|bat)$/i; |
There was a problem hiding this comment.
This is an auto review done by revuto.
.cmd/.bat in this set does not hold for execFileSync on current Node. Since 18.20.2 / 20.12.2 / 21.7.3 (the CVE-2024-27980 fix), Node refuses to spawn .bat/.cmd files unless shell: true is set — spawnSync/execFileSync fail with EINVAL. The repo requires >=18.0.0 and CI runs Node 24, so every supported runtime is on the post-fix behavior.
Combined with find(...) taking the first spawnable entry (line 88), an npm-global install — where where.exe claude lists claude (filtered), claude.cmd, claude.ps1 in PATH order — resolves to claude.cmd, and execFileSync(claudeExecutable(), ['plugin', ...]) at lines 1120/1124/1127/1199/1305/1319/1326/1330/1380 throws EINVAL instead of running. Same for the empty-output fallback, which returns resolveExecutableForPlatform('claude', 'win32') → claude.cmd (asserted by the new test at __tests__/cli-args.test.js:219). Net effect: for the npm-shim channel the swallowed ENOENT becomes a swallowed EINVAL — nothing installed, only now surfaced via the new [WARN] line rather than fixed.
Suggested fix: rank .exe/.com ahead of .cmd/.bat rather than taking PATH order, and if only a .cmd/.bat is available, spawn it explicitly through cmd.exe /c <path> … (safe here: plugin IDs are already validated against /^[a-z0-9][a-z0-9-]*$/ at line 1122, and the marketplace/plugin names elsewhere are repo-controlled) instead of handing the batch file to execFileSync directly. Worth a unit case pinning "prefers claude.exe when both are present" too — the current test at line 208 passes only because .exe happens to be listed first.
Node's src has disallowed direct .bat and .cmd spawning since the CVE-2024-27980 fix (18.20.2 / 20.12.2 / 21.7.3), so handing an npm-global claude.cmd to execFileSync fails with EINVAL instead of running it. The previous commit resolved the shim correctly but still spawned it directly, which turned a swallowed ENOENT into a swallowed EINVAL for the npm install channel. - Rank a directly launchable claude.exe/.com ahead of a batch shim regardless of the order where.exe reports, since only the shim needs the extra hop. - Route a .cmd/.bat resolution through cmd.exe /d /s /c, quoting the shim path (Node's own shell:true path concatenates instead, so a path with spaces breaks there). - Reject any argument carrying whitespace or a shell metacharacter before it reaches cmd.exe. Call sites only pass literal subcommands and plugin ids already validated against /^[a-z0-9][a-z0-9-]*$/, so the hop cannot reintroduce the injection surface #388 closed. - Report the errno alongside a plugin Claude Code did not register, so a rejected plugin and an unspawnable shim are distinguishable. Reported in review of #390.
There was a problem hiding this comment.
This is an auto review done by revuto.
Reviewed the Windows executable resolution (pickClaudeExecutable / claudeSpawnPlan / claudeExecutable), the installPlugin failure reporting, and the command-parser.js de-binarization.
Verified as correct:
pickClaudeExecutablesplits on/\r?\n/(CRLF-safe forwhere.exeoutput), trims before extension matching, and falls back toresolveExecutableForPlatform('claude', platform)so it cannot regress belowmain.claudeSpawnPlanquotes the shim path and routes it through%COMSPEC%with/d /s /c+windowsVerbatimArguments, which is the shape Node itself uses for shell mode; theCMD_SAFE_ARGallowlist accepts every argument actually passed at the 10 call sites (plugin,install/update/uninstall/marketplace,agent-sh/agentsys,<name>@agentsys,<name>@awesome-slash), so the guard is not over-tight for real inputs.process.env.comspecresolves becauseprocess.envlookups are case-insensitive on Windows.- The
command-parser.jschange is text-only aside from one real logic fix — the base blob hadif (arg.includes(' '))throwing "invalid null byte", nowarg.includes('\0')(confirmed viagit diff --text --ignore-cr-at-eol), so the null-byte guard inparseCommandactually works now.resolveExecutableForPlatformbehavior is unchanged, solib/perf/*-runner.jsandbin/dev-cli.jscallers are unaffected. - All 13
claudespawns inbin/cli.jsnow go throughclaudeSpawn; noexecFileSync(claudeBin, ...)orresolveExecutableForPlatform('claude')call site is left behind anywhere in the tree.
One point on the new failure-reporting path, inline.
|
|
||
| console.log(`\n[OK] Installed ${name} successfully.`); | ||
| if (claudeFailures.length > 0) { | ||
| console.log(`\n[WARN] Installed ${name}, but Claude Code did not register: ${claudeFailures.join(', ')}`); |
There was a problem hiding this comment.
This is an auto review done by revuto.
The new [WARN] branch fixes the console message, but the two other surfaces that assert success in this same function are still unconditional, so "the CLI reported success while installing nothing" is only half closed:
recordInstall(depName, ver, platforms)runs at lines 1193-1206 before this check, withplatformsstill containing'claude'. Soinstalled.jsonrecords a successful Claude install for exactly the plugins named inclaudeFailures. That record is load-bearing:removePluginreadsinstalled.plugins[name].platforms(line 1226) to decide whether to runclaude plugin uninstall, andlistprints it (line 711).process.exitCodeis left at 0. Theinstallsubcommand returns immediately afterawait installPlugin(...)(lines 2211-2212) and never touchesprocess.exitCode— unlike the--toolpath, which setsprocess.exitCode = 1forfailedPlatforms(line 2340). Soagentsys install <plugin> && <next step>still proceeds as if everything landed, which on a native-installer Windows box is precisely the silent-success mode this PR is closing.
Both are cheap here: drop 'claude' from the recorded platforms (or mark the entry partial) when claudeFailures contains the dep, and set process.exitCode = 1 alongside the warning.
The [WARN] line fixed the console message, but two other surfaces in installPlugin still asserted success: - recordInstall ran before the check with 'claude' still in platforms, so installed.json recorded a Claude install for the very plugins that failed. That record is load-bearing: removePlugin reads it to decide whether to run `claude plugin uninstall`, and `list` prints it back. - process.exitCode stayed 0, so `agentsys install x && next-step` proceeded as if everything landed, unlike the --tool path which sets exitCode 1 for failed platforms. Track failures as a depName -> errno map so recording can drop 'claude' per plugin, and set exitCode 1 alongside the warning. Reported in review of #390.
detectInstalledPlatforms marks 'claude' whenever ~/.claude exists, so
commandExists('claude') could be false while 'claude' stayed in the
recorded platforms - the install block was skipped and installed.json
still claimed every plugin had reached Claude Code. A dependency whose
id fails the plugin-name guard was skipped the same way.
Record both as failures so they surface in the warning, stay out of
installed.json, and set a non-zero exit code.
There was a problem hiding this comment.
This is an auto review done by revuto.
Reviewed the Windows resolution rework (pickClaudeExecutable / claudeSpawnPlan / claudeExecutable), the installPlugin failure accounting, and the command-parser.js de-binarization.
Verified as sound:
pickClaudeExecutablesplits on/\r?\n/, ranks.exe/.comahead of.cmd/.bat, and falls back toresolveExecutableForPlatform('claude', platform), so it can't do worse thanmain; POSIX short-circuits to'claude'with nowhere.exespawn at import time.claudeSpawnPlan's cmd.exe hop uses the/d /s /c "…"+windowsVerbatimArgumentsform Node itself uses, andCMD_SAFE_ARGexcludes& | < > ^ % ! "and whitespace, so the extra hop doesn't reopen the injection surface #388 closed. All 10 Claude invocations now go throughclaudeSpawn, so no call site bypasses the plan.command-parser.jsat HEAD contains'\0'(line 145) and no raw NUL, matching the new source-hygiene test.
One issue with the new installed.json accounting, inline.
| for (const depName of toFetch) { | ||
| const dep = pluginMap[depName]; | ||
| const ver = depName === name && requestedVersion ? requestedVersion : (dep ? dep.version : 'unknown'); | ||
| const recorded = recordedPlatforms(platforms, claudeFailures, depName); |
There was a problem hiding this comment.
This is an auto review done by revuto.
recordInstall replaces the entry wholesale (data.plugins[name] = entry, line 885) rather than merging, so passing the filtered list here can erase a Claude registration that really exists.
Concrete path: plugin was installed successfully earlier (platforms: ['claude', …]). A later agentsys install <plugin> re-run hits plugin install → already installed → throws → plugin update → also throws (offline, marketplace-add failed earlier, transient Claude error). claudeFailures.set(depName, …) fires at line 1195, recordedPlatforms strips 'claude', and the entry is rewritten without it.
After that, removePlugin gates the uninstall on platforms.includes('claude') (line 1266), so agentsys remove <plugin> will no longer run claude plugin uninstall and the plugin stays loaded in Claude Code — the mirror image of the bug this change is fixing (state that doesn't match reality), just in the other direction. list also stops showing claude for a plugin Claude Code still has.
Union the previously recorded platforms in rather than dropping unconditionally, e.g. keep 'claude' when loadInstalledJson().plugins[depName]?.platforms already contains it — a failed re-install is not evidence that the prior install never landed.
recordInstall replaces the installed.json entry wholesale, so filtering 'claude' out on failure could erase a registration that really exists: a re-install of an already-installed plugin throws, the update fallback throws too (offline, transient error), and the rewritten entry loses 'claude'. removePlugin then skips `claude plugin uninstall` and the plugin stays loaded in Claude Code - the same state-vs-reality mismatch in the opposite direction. Keep a platform that was already recorded, and only drop 'claude' when nothing had registered it before. Reported in review of #390.
There was a problem hiding this comment.
This is an auto review done by revuto.
Revuto completed the review and found no evidence-backed concerns.
CodeQL flagged two alerts in the new shim planner on #391. quoteForCmd matched trailing backslashes with /(\\+)$/, which retries the greedy run from every start position and so is quadratic in the length of an all-backslash argument (js/polynomial-redos, high). Count the run instead - linear, and the output is unchanged. Separately, while re-reading the guard #390 added: CMD_SAFE_ARG ended in $, and in JavaScript $ also matches before a trailing newline. So 'core@agentsys\n' passed as safe, and 'a\n&calc' with it. cmd.exe ends its command line at the newline, so the tail was dropped rather than checked - not an execution path, but the guard was not enforcing what it claimed. Anchor with (?![\s\S]), and refuse CR and LF in planShimSpawn for the same reason: neither survives the cmd.exe hop. The remaining alert, js/shell-command-constructed-from-input, is the cmd.exe command line this planner exists to build; the quoting and the "/%/newline refusals are the mitigation.
) Follow-up to #390, which fixed the EINVAL-on-.cmd defect for the Claude plugin CLI only. Four more sites had it and one had the sibling PATHEXT defect. Node's src has disallowed direct .bat/.cmd spawning since the CVE-2024-27980 fix (18.20.2 / 20.12.2 / 21.7.3), and resolveExecutableForPlatform turns `npm` into `npm.cmd`, so on Windows these all failed with EINVAL instead of running: - bin/dev-cli.js - `agentsys-dev test` - scripts/bump-version.js - `agentsys-dev bump <version>` (hardcoded npm.cmd) - lib/perf/benchmark-runner.js - runBenchmark - lib/perf/profiling-runner.js - runProfiling probeCLI in lib/sources/custom-handler.js failed the other way: execFileSync applies no PATHEXT, so a custom source naming npx/pnpm/yarn was reported unavailable even when installed. #390's cmd.exe hop is now a shared planShimSpawn/shimSpawnOptions pair in lib/utils/command-parser.js, used by all six sites including bin/cli.js so they cannot drift apart. It emits `cmd.exe /d /s /c "<quoted command>"` with windowsVerbatimArguments, on win32 only - a repo-local build.cmd on Linux is spawnable as it stands, and rewriting it would only add a bogus ENOENT. Unlike the Claude sites, these carry user-written commands, so arguments are quoted per token rather than rejected for whitespace or metacharacters: inside double quotes cmd.exe leaves & | < > ^ ( ) alone, and trailing backslashes are doubled so the closing quote survives the child's argv reparse. A literal ", %, CR or LF is refused - in the executable as well as the arguments, since both land on the same command line. bin/cli.js keeps its stricter allowlist, so #390's guarantee is unchanged. `agentsys-dev test` now prints the error it used to swallow. Also from review: quoteForCmd counts the trailing backslash run instead of matching /(\\+)$/, which backtracked per start position (CodeQL js/polynomial-redos, high). One earlier commit claimed bin/cli.js's `$` anchor admitted a trailing newline - that was wrong, JS `$` without /m matches only at end of input, and it is retracted in 7b22021. Tests: 3567 pass. Every shim case names the platform rather than depending on the host, so the suite is deterministic on Windows too. CodeQL's js/shell-command-constructed-from-input on the /c payload is dismissed as mitigated - cmd.exe takes exactly one command-line string, so constructing it is unavoidable. Caveat: ci.yml has no windows-latest leg, so none of this Windows code is exercised in CI. The .bat/.cmd gate is Windows-only native code and cannot be reproduced on Linux even with a faked process.platform; the behaviour rests on Node's changelog, its April 2024 advisory, and unit tests over the constructed command line.
…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.
Problem
#388 landed the argv-array conversion, which is right, but resolves the executable at module load with:
On win32 that always returns
claude.cmd. That file exists only for npm global installs. Users who installed Claude Code through the native installer haveclaude.exe, andclaude.cmdis simply absent.The failure is silent, not loud:
commandExists('claude')(bin/cli.js:60) shells out towhere.exe, which resolvesclaude.exefine, so the branch is entered.execFileSync('claude.cmd', ...)raisesENOENT.catch {}, so the error is discarded.installPluginreachesconsole.log('[OK] Installed ${name} successfully.')unconditionally.Net effect on Windows native installs: nothing is installed into Claude Code, no error is surfaced, the user is told it succeeded. That is the same failure mode #388 and #389 were opened to eliminate, reached through a different install channel. The pre-rename
@awesome-slashcleanup (bin/cli.js:1302) fails the same way, which reintroduces the dual-load-on-upgrade problem that code exists to prevent.Change
Resolve the concrete path instead of guessing the suffix:
pickClaudeExecutable(platform, whereOutput)- pure, so the win32 branch is testable off Windows. Takes the first entry fromwhere.exe claudewhose extensionCreateProcesscan actually launch (.exe/.com/.cmd/.bat), becausewhere.exealso lists npm's extensionless shell script andclaude.ps1, neither of which is spawnable. Falls back toresolveExecutableForPlatform('claude', platform)when nothing spawnable is found, so this cannot behave worse than currentmain.claudeExecutable()- resolves once and caches. Lazy, sorequire('bin/cli.js')spawns no process (the test suite imports it).'claude'. No behavior change off Windows.Two related fixes in the same area:
installPluginnow collects failures and prints[WARN] Installed X, but Claude Code rejected: ...plus a retry line, instead of an unconditional[OK]. Without this, the next resolution gap is invisible all over again.lib/utils/command-parser.jswas binary to git. It carried a rawU+0000byte at offset 3396 where'\0'was intended. Valid JS, but git and grep classify the file as binary - which is why harden: sanitize child_process call in cli.js... #388's change to this file rendered asBinary files ... differand could not be reviewed as a diff. Now text;git grepreturns lines instead ofBinary file matches. Also normalized to LF, the only CRLF-encoded source file in the repo (core.autocrlf=inputdoes this on commit regardless).Note:
git diffstill shows this file asBin 3643 -> 3484in this PR, because the pre-image blob is binary. Every diff after this merges as text.Verification
8 new tests: 7 covering
pickClaudeExecutable(npm shim path, nativeclaude.exe, multiple matches,.ps1/extensionless skipping, empty-output fallback, POSIX), 1 guarding the source against a raw null byte. The source-shape test #388 added assertedexecFileSync(claudeBin,, so it is updated to the new shape.Not verified
No Windows host was available, so
execFileSyncagainst a realclaude.cmd/claude.exeis not executed - the same gap #388 shipped with. The.ps1and extensionless filtering is reasoned from PATHEXT andwhere.exebehavior, not run.ci.ymlis ubuntu-only; awindows-latestleg would be the way to get real coverage on this path.Context: supersedes the resolution half of #389 (closed). Builds on #388 (merged).