Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 113 additions & 1 deletion __tests__/dev-install.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -163,14 +163,126 @@ describe('dev-install script', () => {
});

test('removes marketplace for Claude', () => {
expect(devInstallSource.includes('plugin marketplace remove')).toBe(true);
expect(devInstallSource.includes("'plugin', 'marketplace', 'remove', 'agent-sh/agentsys'")).toBe(true);
});

test('copies to ~/.agentsys for OpenCode/Codex', () => {
expect(devInstallSource.includes('copyToAgentSys')).toBe(true);
});
});

describe('external commands', () => {
const realPlatform = process.platform;
const realComspec = process.env.comspec;

function setPlatform(platform) {
Object.defineProperty(process, 'platform', { value: platform, configurable: true });
}

afterEach(() => {
setPlatform(realPlatform);
if (realComspec === undefined) {
delete process.env.comspec;
} else {
process.env.comspec = realComspec;
}
jest.resetModules();
jest.clearAllMocks();
});

/**
* Load dev-install with child_process mocked, on the given platform.
*
* The platform is set before the require because resolveExecutableForPlatform
* and planShimSpawn read process.platform when runCommand calls them. comspec
* is pinned for the same reason: a real Windows host has COMSPEC set to an
* absolute path, so reading it would make the expected shell differ per host.
*/
function loadWithPlatform(platform) {
setPlatform(platform);
process.env.comspec = 'cmd.exe';
jest.resetModules();
const childProcess = require('child_process');
jest.spyOn(childProcess, 'execFileSync').mockReturnValue('');
return {
devInstall: require(devInstallPath),
execFileSync: childProcess.execFileSync
};
}

test('no execSync anywhere - every command is an argv list', () => {
expect(devInstallSource.includes('execSync(')).toBe(false);
expect(devInstallSource.includes('execFileSync')).toBe(true);
});

test('routes the claude shim through cmd.exe on Windows', () => {
const { devInstall, execFileSync } = loadWithPlatform('win32');

devInstall.runCommand('claude', ['plugin', 'uninstall', 'core@agentsys'], { stdio: 'pipe' });

expect(execFileSync).toHaveBeenCalledWith(
'cmd.exe',
['/d', '/s', '/c', '""claude.cmd" "plugin" "uninstall" "core@agentsys""'],
{ stdio: 'pipe', windowsVerbatimArguments: true }
);
});

test('spawns claude directly on other platforms', () => {
const { devInstall, execFileSync } = loadWithPlatform('linux');

devInstall.runCommand('claude', ['plugin', 'uninstall', 'core@agentsys'], { stdio: 'pipe' });

expect(execFileSync).toHaveBeenCalledWith(
'claude',
['plugin', 'uninstall', 'core@agentsys'],
{ stdio: 'pipe' }
);
});

test('resolves npm to its shim and keeps the cwd', () => {
const { devInstall, execFileSync } = loadWithPlatform('win32');

devInstall.runCommand('npm', ['install', '--production'], { cwd: 'C:\\Users\\dev\\.agentsys', stdio: 'pipe' });

expect(execFileSync).toHaveBeenCalledWith(
'cmd.exe',
['/d', '/s', '/c', '""npm.cmd" "install" "--production""'],
{ cwd: 'C:\\Users\\dev\\.agentsys', stdio: 'pipe', windowsVerbatimArguments: true }
);
});

test('a plugin name holding shell metacharacters stays one argument', () => {
const { devInstall, execFileSync } = loadWithPlatform('linux');

devInstall.runCommand('claude', ['plugin', 'uninstall', 'core & calc@agentsys'], { stdio: 'pipe' });

const [, args] = execFileSync.mock.calls[0];
expect(args).toEqual(['plugin', 'uninstall', 'core & calc@agentsys']);
});

test('commandExists asks where.exe on Windows and which elsewhere', () => {
const win = loadWithPlatform('win32');
win.devInstall.commandExists('claude');
expect(win.execFileSync.mock.calls[0][0]).toBe('where.exe');
expect(win.execFileSync.mock.calls[0][1]).toEqual(['claude']);

const linux = loadWithPlatform('linux');
linux.devInstall.commandExists('claude');
expect(linux.execFileSync).toHaveBeenCalledWith('which', ['claude'], { stdio: 'pipe' });
});

test('commandExists reports false when the lookup fails', () => {
setPlatform('linux');
jest.resetModules();
const childProcess = require('child_process');
jest.spyOn(childProcess, 'execFileSync').mockImplementation(() => {
throw new Error('not found');
});

expect(require(devInstallPath).commandExists('claude')).toBe(false);
});
});

describe('output', () => {
test('logs with [dev-install] prefix', () => {
expect(devInstallSource.includes('[dev-install]')).toBe(true);
Expand Down
33 changes: 27 additions & 6 deletions scripts/dev-install.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
* - Runs synchronously for quick feedback
*/

const { execSync } = require('child_process');
const { execFileSync } = require('child_process');
const fs = require('fs');
const path = require('path');

Expand All @@ -28,6 +28,11 @@ const SOURCE_DIR = path.join(__dirname, '..');
const VERSION = require(path.join(SOURCE_DIR, 'package.json')).version;
const discovery = require(path.join(SOURCE_DIR, 'lib', 'discovery'));
const transforms = require(path.join(SOURCE_DIR, 'lib', 'adapter-transforms'));
const {
resolveExecutableForPlatform,
planShimSpawn,
shimSpawnOptions
} = require(path.join(SOURCE_DIR, 'lib', 'utils', 'command-parser'));

// Target directories
const HOME = process.env.HOME || process.env.USERPROFILE;
Expand All @@ -54,9 +59,25 @@ function log(msg) {
console.log(`[dev-install] ${msg}`);
}

/**
* Run one external command with an argv list instead of a shell string.
*
* The commands here (`claude`, `npm`) are .cmd shims on Windows, which need two
* separate fixes: resolveExecutableForPlatform supplies the extension that
* execFileSync will not look up itself, and planShimSpawn routes the shim
* through cmd.exe, since Node has refused to spawn .cmd directly since the
* CVE-2024-27980 fix. Plugin names reach this from discovery on disk, so they
* stay argv elements - a shell string would make a directory named `a & b` a
* second command.
*/
function runCommand(command, args, options = {}) {
const plan = planShimSpawn(resolveExecutableForPlatform(command), args);

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.


runCommand('claude', ...) resolves the executable purely through resolveExecutableForPlatform, which maps claudeclaude.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.

return execFileSync(plan.file, plan.args, shimSpawnOptions(plan, options));
}

function commandExists(cmd) {
try {
execSync(`${process.platform === 'win32' ? 'where' : 'which'} ${cmd}`, { stdio: 'pipe' });
runCommand(process.platform === 'win32' ? 'where.exe' : 'which', [cmd], { stdio: 'pipe' });
return true;
} catch {
return false;
Expand Down Expand Up @@ -269,7 +290,7 @@ function installClaude() {

// Remove marketplace plugins first
try {
execSync('claude plugin marketplace remove agent-sh/agentsys', { stdio: 'pipe' });
runCommand('claude', ['plugin', 'marketplace', 'remove', 'agent-sh/agentsys'], { stdio: 'pipe' });
log(' Removed marketplace');
} catch {
// May not exist
Expand All @@ -280,7 +301,7 @@ function installClaude() {
// Uninstall both current and pre-rename plugin IDs
for (const suffix of ['agentsys', 'awesome-slash']) {
try {
execSync(`claude plugin uninstall ${plugin}@${suffix}`, { stdio: 'pipe' });
runCommand('claude', ['plugin', 'uninstall', `${plugin}@${suffix}`], { stdio: 'pipe' });
} catch {
// May not be installed
}
Expand Down Expand Up @@ -570,7 +591,7 @@ function copyToAgentSys() {

// Install dependencies
log(' Installing dependencies...');
execSync('npm install --production', { cwd: AGENTSYS_DIR, stdio: 'pipe' });
runCommand('npm', ['install', '--production'], { cwd: AGENTSYS_DIR, stdio: 'pipe' });

agentSysCopied = true;
log(' [OK] ~/.agentsys');
Expand Down Expand Up @@ -635,4 +656,4 @@ if (require.main === module) {
main();
}

module.exports = { main };
module.exports = { main, runCommand, commandExists };
Loading