Skip to content
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Windows: the Claude Code executable is now resolved from `where.exe` instead of assuming the npm shim. `execFileSync` does not apply PATHEXT, and the previous hardcoded `claude.cmd` did not exist for native-installer users who have `claude.exe` - those calls raised `ENOENT`, the error was swallowed, and the CLI reported success while installing nothing. A directly launchable `claude.exe` is preferred over a batch shim regardless of PATH order.
- Windows: an npm-global `claude.cmd` shim is launched through `cmd.exe` rather than handed to `execFileSync`, which fails with `EINVAL` - Node's `src` has disallowed direct `.bat`/`.cmd` spawning since the CVE-2024-27980 fix in 18.20.2 / 20.12.2 / 21.7.3. Arguments are rejected unless they are free of whitespace and shell metacharacters, so the extra hop cannot reintroduce the injection surface #388 closed.
- `agentsys install` no longer reports `[OK] Installed ... successfully` when Claude Code rejected a plugin. It names the plugins that failed, with the errno when the shim could not be spawned at all, and how to retry. The same now applies when the `claude` CLI is not on PATH at all (`~/.claude` alone was enough to mark the platform as installed) and when a dependency id would be rejected. Such a plugin is also no longer recorded against `claude` in `installed.json` - `agentsys list` and `agentsys remove` read those platforms back - though a registration recorded by an earlier successful install is preserved, since a failed re-install is not evidence the first one never landed. The process now exits non-zero, matching the `--tool` path so `agentsys install x && ...` stops.
- Windows: `agentsys-dev test`, `agentsys-dev bump`, `runBenchmark`, and `runProfiling` all handed a `.cmd` shim straight to `spawnSync`/`execFileSync`, which fails with `EINVAL` for the same reason the Claude shim did - `resolveExecutableForPlatform` turns `npm` into `npm.cmd` and `node_modules/.bin/vitest` into `vitest.cmd`, so every benchmark, profile, and dev test run died on Windows. These now route through `cmd.exe` via a shared `planShimSpawn` in `lib/utils/command-parser.js`, which `bin/cli.js` uses as well so the two mechanisms cannot drift apart. Unlike the Claude call sites, these carry user-written commands, so arguments are quoted for `cmd.exe` rather than rejected for containing whitespace or metacharacters; a literal `"`, `%`, CR or LF is still refused, in the executable as well as the arguments, since none survives `cmd.exe` intact - `bench\ncalc` would otherwise reach the command line, which `bin/cli.js` is shielded from only by its allowlist. The rewrite is win32-only: a repo-local `build.cmd` on Linux is spawnable as it stands, and routing it through a `cmd.exe` that is not there would only turn a working command into `ENOENT`. `agentsys-dev test` also prints the error it used to swallow, since a refused argument now reaches the user from there.
- Windows: a custom source naming an npm-shipped CLI (`npx`, `pnpm`, `yarn`) was always probed as unavailable - `execFileSync` applies no PATHEXT, so the bare name raised `ENOENT`, and the `.cmd` it needs cannot be spawned directly either. `probeCLI` resolves the shim and routes it the same way.
- `lib/utils/command-parser.js` used 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. Also normalized to LF, the only CRLF-encoded source file in the repo.

## [6.0.1] - 2026-07-22
Expand Down
30 changes: 30 additions & 0 deletions __tests__/bump-version.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,36 @@ describe('bump-version', () => {
expect(consoleErrorSpy).not.toHaveBeenCalled();
});

test('spawns npm directly off Windows', () => {
const { execFileSync } = require('child_process');
expect(main(['3.7.3'])).toBe(0);
expect(execFileSync).toHaveBeenCalledWith(
'npm',
['version', '3.7.3', '--no-git-tag-version'],
expect.not.objectContaining({ windowsVerbatimArguments: true })
);
Comment on lines +84 to +99

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an auto review done by revuto.


This case is host-dependent and will fail when the suite runs on Windows — the very platform this PR is fixing.

Unlike its sibling below (which fakes win32 explicitly) and unlike __tests__/perf-benchmark-runner.test.js, which fakes linux for its "off Windows" case, this test leaves process.platform as the host's. On a Windows host resolveExecutableForPlatform('npm') returns npm.cmd (lib/utils/command-parser.js:45-47), so scripts/bump-version.js:69-71 plans the cmd.exe hop and execFileSync is called with 'cmd.exe', not 'npm' — the assertion on line 88 fails.

__tests__/command-parser.test.js:71-72 states the intended convention for exactly this reason ("every shim case names the platform rather than depending on the host running the suite"). Pinning the platform to 'linux' around this main(['3.7.3']) call, the same way the perf test does, makes it deterministic.

(Note tests/sources/custom-handler.test.js's "should probe a plain executable directly off Windows" is fine as written, since gh is not in WINDOWS_CMD_SHIMS and so resolves unchanged on win32 too.)

});

test('routes the npm.cmd shim through cmd.exe on Windows', () => {
// A direct .cmd spawn fails with EINVAL since the CVE-2024-27980 fix, so
// `agentsys-dev bump` needs the same hop as the rest of the CLI.
const { execFileSync } = require('child_process');
const platform = process.platform;
Object.defineProperty(process, 'platform', { value: 'win32', configurable: true });

try {
expect(main(['3.7.3'])).toBe(0);
} finally {
Object.defineProperty(process, 'platform', { value: platform, configurable: true });
}

expect(execFileSync).toHaveBeenCalledWith(
'cmd.exe',
['/d', '/s', '/c', '""npm.cmd" "version" "3.7.3" "--no-git-tag-version""'],
expect.objectContaining({ windowsVerbatimArguments: true })
);
});

test('rejects invalid version format - missing patch', () => {
const code = main(['3.7']);
expect(code).toBe(1);
Expand Down
31 changes: 21 additions & 10 deletions __tests__/cli-args.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -229,34 +229,45 @@ describe('claudeSpawnPlan', () => {
const args = ['plugin', 'install', 'agentsys-core@agentsys'];

test('spawns a posix or native executable directly', () => {
expect(claudeSpawnPlan('claude', args)).toEqual({ file: 'claude', args });
expect(claudeSpawnPlan('C:\\bin\\claude.exe', args)).toEqual({ file: 'C:\\bin\\claude.exe', args });
expect(claudeSpawnPlan('claude', args)).toEqual({ file: 'claude', args, verbatim: false });
expect(claudeSpawnPlan('C:\\bin\\claude.exe', args)).toEqual({ file: 'C:\\bin\\claude.exe', args, verbatim: false });
});

test('routes a batch shim through cmd.exe, which execFileSync cannot spawn', () => {
// Node's src disallows direct .bat/.cmd spawning since the CVE-2024-27980
// fix, so a shim handed to execFileSync fails with EINVAL.
const shim = 'C:\\npm\\claude.cmd';
expect(claudeSpawnPlan(shim, args)).toEqual({
expect(claudeSpawnPlan(shim, args, undefined, 'win32')).toEqual({
file: 'cmd.exe',
args: ['/d', '/s', '/c', '""C:\\npm\\claude.cmd" plugin install agentsys-core@agentsys"'],
args: ['/d', '/s', '/c', '""C:\\npm\\claude.cmd" "plugin" "install" "agentsys-core@agentsys""'],
verbatim: true
});
expect(claudeSpawnPlan('C:\\npm\\claude.bat', args).file).toBe('cmd.exe');
expect(claudeSpawnPlan('C:\\npm\\claude.bat', args, undefined, 'win32').file).toBe('cmd.exe');
});

test('spawns a .cmd directly off Windows, where cmd.exe does not exist', () => {
// The extension alone is not evidence a file needs a shell: routing a
// repo-local build.cmd through a missing cmd.exe would only cause ENOENT.
expect(claudeSpawnPlan('./claude.cmd', args, undefined, 'linux'))
.toEqual({ file: './claude.cmd', args, verbatim: false });
});

test('honours COMSPEC when routing through a shell', () => {
expect(claudeSpawnPlan('claude.cmd', args, 'C:\\Windows\\system32\\cmd.exe').file)
expect(claudeSpawnPlan('claude.cmd', args, 'C:\\Windows\\system32\\cmd.exe', 'win32').file)
.toBe('C:\\Windows\\system32\\cmd.exe');
});

test('refuses arguments cmd.exe would reparse', () => {
// cmd.exe re-splits its command line, so an unquoted metacharacter would be
// a command injection - the exact hazard behind CVE-2024-27980.
expect(() => claudeSpawnPlan('claude.cmd', ['plugin', 'install', 'x&calc'])).toThrow(/Refusing to pass/);
expect(() => claudeSpawnPlan('claude.cmd', ['plugin', 'install', 'a|b'])).toThrow(/Refusing to pass/);
expect(() => claudeSpawnPlan('claude.cmd', ['plugin', 'install', 'a b'])).toThrow(/Refusing to pass/);
expect(() => claudeSpawnPlan('claude.cmd', ['plugin', 'install', 'a"b'])).toThrow(/Refusing to pass/);
const reject = arg => () => claudeSpawnPlan('claude.cmd', ['plugin', 'install', arg], undefined, 'win32');
expect(reject('x&calc')).toThrow(/Refusing to pass/);
expect(reject('a|b')).toThrow(/Refusing to pass/);
expect(reject('a b')).toThrow(/Refusing to pass/);
expect(reject('a"b')).toThrow(/Refusing to pass/);
// CR and LF are refused wherever they appear, trailing included.
expect(reject('agentsys-core@agentsys\n')).toThrow(/Refusing to pass/);
expect(reject('a\n&calc')).toThrow(/Refusing to pass/);
});

test('passes the same arguments through unchecked when no shell is involved', () => {
Expand Down
97 changes: 96 additions & 1 deletion __tests__/command-parser.test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const fs = require('fs');
const path = require('path');
const { parseCommand, resolveExecutableForPlatform } = require('../lib/utils/command-parser');
const { parseCommand, resolveExecutableForPlatform, planShimSpawn, shimSpawnOptions } = require('../lib/utils/command-parser');

describe('command parser', () => {
test('parses simple command into executable and args', () => {
Expand Down Expand Up @@ -70,6 +70,101 @@ describe('resolveExecutableForPlatform', () => {
});
});

describe('planShimSpawn', () => {
// The .bat/.cmd rewrite is win32-only, so every shim case names the platform
// rather than depending on the host running the suite.
const plan = (executable, args, options = {}) =>
planShimSpawn(executable, args, { platform: 'win32', ...options });

test('leaves a directly spawnable executable alone', () => {
const args = ['run', 'bench'];
expect(plan('npm', args)).toEqual({ file: 'npm', args, verbatim: false });
expect(plan('node.exe', args)).toEqual({ file: 'node.exe', args, verbatim: false });
expect(plan('/usr/bin/node', args)).toEqual({ file: '/usr/bin/node', args, verbatim: false });
});

test('leaves a .cmd alone off Windows, where cmd.exe does not exist', () => {
// A repo-local build.cmd on Linux spawns as it stands; rewriting it to a
// cmd.exe that is not there would turn a working command into ENOENT.
const args = ['run', 'bench'];
for (const platform of ['linux', 'darwin']) {
expect(planShimSpawn('./build.cmd', args, { platform }))
.toEqual({ file: './build.cmd', args, verbatim: false });
}
});

test('routes a batch shim through cmd.exe, which spawn cannot launch', () => {
// Node's src disallows direct .bat/.cmd spawning since the CVE-2024-27980
// fix, so spawnSync/execFileSync fail with EINVAL on a shim.
expect(plan('npm.cmd', ['run', 'bench'])).toEqual({
file: 'cmd.exe',
args: ['/d', '/s', '/c', '""npm.cmd" "run" "bench""'],
verbatim: true
});
expect(plan('yarn.bat', []).args).toEqual(['/d', '/s', '/c', '""yarn.bat""']);
});

test('quotes arguments so cmd.exe cannot reinterpret them', () => {
// Inside double quotes cmd.exe leaves these alone, so a benchmark command
// carrying them runs instead of being split into extra commands.
const [, , , payload] = plan('npm.cmd', ['run', 'a && calc', 'x|y', 'a>b']).args;
expect(payload).toBe('""npm.cmd" "run" "a && calc" "x|y" "a>b""');
});

test('preserves an empty argument', () => {
const [, , , payload] = plan('npm.cmd', ['run', '']).args;
expect(payload).toBe('""npm.cmd" "run" """');
});

test('doubles trailing backslashes so the closing quote survives', () => {
// "C:\dir\" would read as an escaped quote when the child parses argv back.
const [, , , payload] = plan('npm.cmd', ['--cwd', 'C:\\dir\\']).args;
expect(payload).toBe('""npm.cmd" "--cwd" "C:\\dir\\\\""');
});

test('doubles a long run of backslashes without backtracking', () => {
const [, , , payload] = plan('npm.cmd', ['\\'.repeat(5000)]).args;
expect(payload).toBe(`""npm.cmd" "${'\\'.repeat(10000)}""`);
});

test('refuses arguments cmd.exe cannot carry faithfully', () => {
// % is expanded even inside quotes, and a literal " ends the quoting.
expect(() => plan('npm.cmd', ['run', '%PATH%'])).toThrow(/not representable/);
expect(() => plan('npm.cmd', ['run', 'say "hi"'])).toThrow(/not representable/);
// A newline ends the command line, so what follows is dropped or run alone.
expect(() => plan('npm.cmd', ['run', 'bench\ncalc'])).toThrow(/not representable/);
expect(() => plan('npm.cmd', ['run', 'bench\r'])).toThrow(/not representable/);
expect(() => plan('npm.cmd', ['run', 'a\0b'])).toThrow(/null byte/);
expect(() => plan('npm.cmd', ['run', 42])).toThrow(/must be a string/);
});

test('refuses a shim path cmd.exe would rewrite before resolving it', () => {
// The executable lands on the same command line as the arguments, and
// cmd.exe expands %TEMP% there too - the path it opens is then not the one
// the caller named.
expect(() => plan('C:\\build%TEMP%\\.bin\\vitest.cmd', ['run']))
.toThrow(/executable path.*not representable/s);
});

test('leaves those arguments alone when no shell is involved', () => {
// Nothing reparses an execFileSync argv, so the restriction is shim-only.
expect(plan('npm', ['run', '%PATH%', 'say "hi"']).args)
.toEqual(['run', '%PATH%', 'say "hi"']);
});

test('honours an explicit comspec', () => {
expect(plan('npm.cmd', [], { comspec: 'C:\\Windows\\system32\\cmd.exe' }).file)
.toBe('C:\\Windows\\system32\\cmd.exe');
});

test('shimSpawnOptions adds windowsVerbatimArguments only for a shim', () => {
expect(shimSpawnOptions(plan('npm.cmd', ['run']), { cwd: '/tmp' }))
.toEqual({ cwd: '/tmp', windowsVerbatimArguments: true });
expect(shimSpawnOptions(plan('npm', ['run']), { cwd: '/tmp' }))
.toEqual({ cwd: '/tmp' });
});
});

describe('command-parser source hygiene', () => {
test('rejects a null byte in an argument', () => {
expect(() => parseCommand('node --eval a\0b')).toThrow(/null byte/);
Expand Down
9 changes: 8 additions & 1 deletion __tests__/dev-cli.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -245,10 +245,17 @@ describe('dev-cli module', () => {

test('test command resolves npm executable without shell interpolation', () => {
expect(cliSource).toContain('resolveExecutableForPlatform');
expect(cliSource).toContain('spawnSync(npmExecutable');
expect(cliSource).toContain('shell: false');
});

test('test command routes the npm.cmd shim through a spawn plan', () => {
// npm resolves to npm.cmd on Windows, and a direct spawn of it fails with
// EINVAL since the CVE-2024-27980 fix, so the plan must not be bypassed.
expect(cliSource).toContain('planShimSpawn(npmExecutable');
expect(cliSource).toContain('spawnSync(plan.file, plan.args, shimSpawnOptions(plan');
expect(cliSource).not.toContain('spawnSync(npmExecutable');
});

test('exports parseArgs, COMMANDS, VALIDATE_SUBCOMMANDS, NEW_SUBCOMMANDS, route', () => {
expect(cliSource).toContain('module.exports');
expect(typeof parseArgs).toBe('function');
Expand Down
39 changes: 39 additions & 0 deletions __tests__/perf-benchmark-runner.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,45 @@ describe('runBenchmark', () => {
}));
});

it('runs a batch shim through cmd.exe instead of spawning it directly', () => {
// Node fails with EINVAL on a direct .cmd spawn since the CVE-2024-27980
// fix, so an npm-based benchmark command needs the cmd.exe hop. The hop is
// win32-only, so the platform is faked rather than skipping off Windows.
execFileSync.mockImplementation(() => 'output');
const platform = process.platform;
Object.defineProperty(process, 'platform', { value: 'win32', configurable: true });

try {
runBenchmark('npm.cmd run bench', { allowShort: true });
} finally {
Object.defineProperty(process, 'platform', { value: platform, configurable: true });
}

expect(execFileSync).toHaveBeenCalledWith(
'cmd.exe',
['/d', '/s', '/c', '""npm.cmd" "run" "bench""'],
expect.objectContaining({ windowsVerbatimArguments: true })
);
});

it('spawns a .cmd directly off Windows, where cmd.exe does not exist', () => {
execFileSync.mockImplementation(() => 'output');
const platform = process.platform;
Object.defineProperty(process, 'platform', { value: 'linux', configurable: true });

try {
runBenchmark('./build.cmd bench', { allowShort: true });
} finally {
Object.defineProperty(process, 'platform', { value: platform, configurable: true });
}

expect(execFileSync).toHaveBeenCalledWith(
'./build.cmd',
['bench'],
expect.not.objectContaining({ windowsVerbatimArguments: true })
);
});

it('sets PERF_RUN_DURATION env variable by default', () => {
execFileSync.mockImplementation(() => 'output');

Expand Down
35 changes: 35 additions & 0 deletions __tests__/perf-profiling-runner.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,41 @@ describe('perf profiling runner', () => {
});
});

it('runs a batch shim profiler command through cmd.exe', () => {
// A direct .cmd spawn fails with EINVAL since the CVE-2024-27980 fix.
jest.resetModules();
const execFileSync = jest.fn();
jest.doMock('child_process', () => ({ execFileSync }));
jest.doMock('../lib/perf/profilers', () => ({
selectProfiler: () => ({
id: 'fake',
buildCommand: () => 'npx.cmd clinic doctor',
parseOutput: () => ({ tool: 'fake', hotspots: [], artifacts: [] })
})
}));

const runner = require('../lib/perf/profiling-runner');
// The cmd.exe hop is win32-only, so fake the platform instead of skipping.
const platform = process.platform;
Object.defineProperty(process, 'platform', { value: 'win32', configurable: true });

try {
expect(runner.runProfiling().ok).toBe(true);
} finally {
Object.defineProperty(process, 'platform', { value: platform, configurable: true });
}

expect(execFileSync).toHaveBeenCalledWith(
'cmd.exe',
['/d', '/s', '/c', '""npx.cmd" "clinic" "doctor""'],
expect.objectContaining({ windowsVerbatimArguments: true })
);

jest.dontMock('child_process');
jest.dontMock('../lib/perf/profilers');
jest.resetModules();
});

it('does not enforce timeout when timeoutMs is not provided', () => {
jest.resetModules();
const execFileSync = jest.fn();
Expand Down
Loading
Loading