diff --git a/__tests__/dev-install.test.js b/__tests__/dev-install.test.js index 68c0523e..8acfa7f3 100644 --- a/__tests__/dev-install.test.js +++ b/__tests__/dev-install.test.js @@ -163,7 +163,7 @@ 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', () => { @@ -171,6 +171,118 @@ describe('dev-install script', () => { }); }); + 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); diff --git a/scripts/dev-install.js b/scripts/dev-install.js index 3b7f6553..4e2ffb9d 100644 --- a/scripts/dev-install.js +++ b/scripts/dev-install.js @@ -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'); @@ -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; @@ -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); + 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; @@ -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 @@ -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 } @@ -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'); @@ -635,4 +656,4 @@ if (require.main === module) { main(); } -module.exports = { main }; +module.exports = { main, runCommand, commandExists };