diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f2ea8e88..ed4367f3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -73,3 +73,15 @@ jobs: bun-version-file: package.json - run: bun install --frozen-lockfile --ignore-scripts - run: bun run test + + windows-command-argv: + name: Windows command argv + runs-on: windows-latest + timeout-minutes: 10 + steps: + - uses: actions/checkout@v6 + - uses: oven-sh/setup-bun@v2 + with: + bun-version-file: package.json + - run: bun install --frozen-lockfile --ignore-scripts + - run: bun test tests/unit/core/native/types.test.ts tests/unit/cli/workspace-setup-command.test.ts diff --git a/bun.lock b/bun.lock index 54b32c69..d208e73c 100644 --- a/bun.lock +++ b/bun.lock @@ -15,6 +15,7 @@ "js-yaml": "^4.1.0", "json5": "^2.2.3", "micromatch": "^4.0.8", + "read-cmd-shim": "^4.0.0", "simple-git": "^3.30.0", "zod": "^3.22.4", }, @@ -307,6 +308,8 @@ "raw-body": ["raw-body@3.0.2", "", { "dependencies": { "bytes": "~3.1.2", "http-errors": "~2.0.1", "iconv-lite": "~0.7.0", "unpipe": "~1.0.0" } }, "sha512-K5zQjDllxWkf7Z5xJdV0/B0WTNqx6vxG70zJE4N0kBs4LovmEYWJzQGxC9bS9RAKu3bgM40lrd5zoLJ12MQ5BA=="], + "read-cmd-shim": ["read-cmd-shim@4.0.0", "", {}, "sha512-yILWifhaSEEytfXI76kB9xEEiG1AiozaCJZ83A87ytjRiN+jVibXjedjCRNjoZviinhG+4UkalO3mWTd8u5O0Q=="], + "rechoir": ["rechoir@0.6.2", "", { "dependencies": { "resolve": "^1.1.6" } }, "sha512-HFM8rkZ+i3zrV+4LQjwQ0W+ez98pApMGM3HUrN04j3CqzPOzl9nmP15Y8YXNm8QHGv/eacOVEjqhmWpkRV0NAw=="], "require-from-string": ["require-from-string@2.0.2", "", {}, "sha512-Xf0nWe6RseziFMu+Ap9biiUbmplq6S9/p+7w7YXP/JBHhrUDDUhwa+vANyubuqfZWTveU//DYVGsDG7RKL/vEw=="], diff --git a/package.json b/package.json index e701af46..23642f5f 100644 --- a/package.json +++ b/package.json @@ -65,6 +65,7 @@ "js-yaml": "^4.1.0", "json5": "^2.2.3", "micromatch": "^4.0.8", + "read-cmd-shim": "^4.0.0", "simple-git": "^3.30.0", "zod": "^3.22.4" }, diff --git a/src/core/native/types.ts b/src/core/native/types.ts index ff50d47a..c1fb8628 100644 --- a/src/core/native/types.ts +++ b/src/core/native/types.ts @@ -1,4 +1,8 @@ import { spawn } from 'node:child_process'; +import { once } from 'node:events'; +import { access, open } from 'node:fs/promises'; +import { delimiter, dirname, extname, resolve } from 'node:path'; +import readCmdShim from 'read-cmd-shim'; export interface NativeCommandResult { success: boolean; @@ -50,20 +54,177 @@ export interface NativeClient { syncPlugins(plugins: string[], scope: 'user' | 'project', options?: { cwd?: string; dryRun?: boolean }): Promise; } +async function resolveWindowsBinary( + binary: string, + nativeOnly = false, +): Promise { + const pathEntries = /[\\/]/.test(binary) + ? [''] + : (process.env.PATH ?? '') + .split(delimiter) + .map((pathEntry) => { + const trimmed = pathEntry.trim(); + return trimmed.startsWith('"') && trimmed.endsWith('"') + ? trimmed.slice(1, -1) + : trimmed; + }) + // Empty Windows PATH entries mean cwd, but client binaries must come + // from an explicit PATH directory rather than the workspace. + .filter((pathEntry) => pathEntry.length > 0); + const configuredExtensions = (process.env.PATHEXT ?? '.COM;.EXE;.BAT;.CMD') + .split(delimiter) + .map((extension) => { + const normalized = extension.trim().toLowerCase(); + return normalized && !normalized.startsWith('.') + ? `.${normalized}` + : normalized; + }) + .filter((extension) => extension.length > 0); + const extensions = extname(binary) + ? [''] + : nativeOnly + ? configuredExtensions.filter( + (extension) => extension === '.com' || extension === '.exe', + ) + : configuredExtensions; + + for (const directory of pathEntries) { + for (const extension of extensions) { + const candidate = resolve(directory, `${binary}${extension}`); + try { + await access(candidate); + return candidate; + } catch { + // Continue through PATH. + } + } + } + + throw new Error(`command not found on PATH: ${binary}`); +} + +async function resolveWindowsNativeBinary(binary: string): Promise { + const extension = extname(binary).toLowerCase(); + if (extension && extension !== '.com' && extension !== '.exe') { + throw new Error(`unsafe Windows interpreter '${binary}'`); + } + return resolveWindowsBinary(binary, true); +} + +async function resolveWindowsShimInterpreter( + interpreter: string, + shimDirectory: string, +): Promise { + const normalizedInterpreter = interpreter.toLowerCase(); + if ( + normalizedInterpreter !== 'node' && + normalizedInterpreter !== 'node.exe' + ) { + throw new Error(`unsupported command shim interpreter '${interpreter}'`); + } + + const siblingNode = resolve(shimDirectory, 'node.exe'); + try { + await access(siblingNode); + return siblingNode; + } catch { + return resolveWindowsNativeBinary(interpreter); + } +} + +async function resolveWindowsCommand( + binary: string, + args: string[], +): Promise<{ binary: string; args: string[] }> { + const resolvedBinary = await resolveWindowsBinary(binary); + const extension = extname(resolvedBinary).toLowerCase(); + if (extension !== '.cmd') { + if (extension !== '.com' && extension !== '.exe') { + throw new Error( + `cannot safely execute Windows command '${resolvedBinary}'`, + ); + } + return { binary: resolvedBinary, args }; + } + + const target = resolve( + dirname(resolvedBinary), + await readCmdShim(resolvedBinary), + ); + const targetExtension = extname(target).toLowerCase(); + if ( + targetExtension === '.bat' || + targetExtension === '.cmd' || + targetExtension === '.ps1' + ) { + throw new Error(`cannot safely execute command shim target '${target}'`); + } + const file = await open(target, 'r'); + const buffer = Buffer.alloc(256); + let bytesRead = 0; + try { + ({ bytesRead } = await file.read(buffer, 0, buffer.length, 0)); + } finally { + await file.close(); + } + const [firstLine = ''] = buffer + .toString('utf8', 0, bytesRead) + .split(/\r?\n/, 1); + const shebang = firstLine.match( + /^#!\s*(?:\/usr\/bin\/env\s+(?:-S\s+)?)?([^ \t]+)\s*$/, + ); + if (!shebang) { + if (firstLine.startsWith('#!')) { + throw new Error( + `unsupported command shim shebang in '${resolvedBinary}'`, + ); + } + if (targetExtension !== '.com' && targetExtension !== '.exe') { + throw new Error(`unsupported command shim target '${target}'`); + } + return { binary: target, args }; + } + + const interpreter = shebang[1]; + if (!interpreter) { + throw new Error(`missing command shim interpreter in '${resolvedBinary}'`); + } + + return { + binary: await resolveWindowsShimInterpreter( + interpreter, + dirname(resolvedBinary), + ), + args: [target, ...args], + }; +} + /** * Execute a CLI command and capture output. * Shared helper for all native client implementations. */ -export function executeCommand( +export async function executeCommand( binary: string, args: string[], options: { cwd?: string } = {}, ): Promise { - return new Promise((resolve) => { - const proc = spawn(binary, args, { + let command = { binary, args }; + if (process.platform === 'win32') { + try { + command = await resolveWindowsCommand(binary, args); + } catch (err) { + return { + success: false, + output: '', + error: `Failed to execute ${binary} CLI: ${err instanceof Error ? err.message : String(err)}`, + }; + } + } + + try { + const proc = spawn(command.binary, command.args, { cwd: options.cwd, stdio: ['ignore', 'pipe', 'pipe'], - shell: process.platform === 'win32', env: { ...process.env }, }); @@ -77,28 +238,20 @@ export function executeCommand( stderr += data.toString(); }); - let resolved = false; - proc.on('close', (code: number | null) => { - if (resolved) return; - resolved = true; - const trimmedStderr = stderr.trim(); - resolve({ - success: code === 0, - output: stdout.trim(), - ...(trimmedStderr && { error: trimmedStderr }), - }); - }); - - proc.on('error', (err: Error) => { - if (resolved) return; - resolved = true; - resolve({ - success: false, - output: '', - error: `Failed to execute ${binary} CLI: ${err.message}`, - }); - }); - }); + const [code] = (await once(proc, 'close')) as [number | null]; + const trimmedStderr = stderr.trim(); + return { + success: code === 0, + output: stdout.trim(), + ...(trimmedStderr && { error: trimmedStderr }), + }; + } catch (err) { + return { + success: false, + output: '', + error: `Failed to execute ${binary} CLI: ${err instanceof Error ? err.message : String(err)}`, + }; + } } /** diff --git a/src/types/read-cmd-shim.d.ts b/src/types/read-cmd-shim.d.ts new file mode 100644 index 00000000..51b570e1 --- /dev/null +++ b/src/types/read-cmd-shim.d.ts @@ -0,0 +1,4 @@ +declare module 'read-cmd-shim' { + function readCmdShim(path: string): Promise; + export default readCmdShim; +} diff --git a/tests/unit/cli/workspace-setup-command.test.ts b/tests/unit/cli/workspace-setup-command.test.ts index 08dd10e0..31d4343f 100644 --- a/tests/unit/cli/workspace-setup-command.test.ts +++ b/tests/unit/cli/workspace-setup-command.test.ts @@ -1,4 +1,4 @@ -import { afterEach, beforeEach, describe, expect, test } from 'bun:test'; +import { afterAll, beforeEach, describe, expect, test } from 'bun:test'; import { existsSync, mkdirSync, @@ -71,6 +71,7 @@ function writeWorkspace(root: string, setup: SetupFixture[]): void { describe('workspace setup command', () => { let testDir: string; + const testDirs: string[] = []; beforeEach(() => { testDir = join( @@ -78,10 +79,18 @@ describe('workspace setup command', () => { `allagents-workspace-setup-${process.pid}-${Date.now()}`, ); mkdirSync(testDir, { recursive: true }); + testDirs.push(testDir); }); - afterEach(() => { - rmSync(testDir, { recursive: true, force: true }); + afterAll(() => { + for (const directory of testDirs) { + rmSync(directory, { + recursive: true, + force: true, + maxRetries: 5, + retryDelay: 100, + }); + } }); test('runs commands sequentially from the workspace root', () => { @@ -221,54 +230,57 @@ describe('workspace setup command', () => { expect(readFileSync(join(testDir, 'setup.log'), 'utf8')).toBe('first'); }); - test('preserves partial results when a command is terminated by a signal', () => { - const commands = [ - fixtureCommand( - testDir, - 'write-first', - "require('node:fs').writeFileSync('setup.log', 'first');", - ), - fixtureCommand( - testDir, - 'terminate-shell', - "process.kill(process.ppid, 'SIGTERM');", - ), - fixtureCommand( - testDir, - 'write-third', - "require('node:fs').appendFileSync('setup.log', '-third');", - ), - ]; - writeWorkspace(testDir, commands); + test.skipIf(process.platform === 'win32')( + 'preserves partial results when a command is terminated by a signal', + () => { + const commands = [ + fixtureCommand( + testDir, + 'write-first', + "require('node:fs').writeFileSync('setup.log', 'first');", + ), + fixtureCommand( + testDir, + 'terminate-shell', + "process.kill(process.ppid, 'SIGTERM');", + ), + fixtureCommand( + testDir, + 'write-third', + "require('node:fs').appendFileSync('setup.log', '-third');", + ), + ]; + writeWorkspace(testDir, commands); - const proc = runCli(testDir, ['workspace', 'setup'], testDir); + const proc = runCli(testDir, ['workspace', 'setup'], testDir); - expect(proc.exitCode).toBe(1); - expect(JSON.parse(proc.stdout.toString())).toEqual({ - success: false, - command: 'workspace setup', - data: { - commands: [ - { - command: commands[0], - status: 'succeeded', - exitCode: 0, - signal: null, - reason: null, - }, - { - command: commands[1], - status: 'failed', - exitCode: null, - signal: 'SIGTERM', - reason: null, - }, - ], - }, - error: `Setup command terminated by signal SIGTERM: ${commands[1]}`, - }); - expect(readFileSync(join(testDir, 'setup.log'), 'utf8')).toBe('first'); - }); + expect(proc.exitCode).toBe(1); + expect(JSON.parse(proc.stdout.toString())).toEqual({ + success: false, + command: 'workspace setup', + data: { + commands: [ + { + command: commands[0], + status: 'succeeded', + exitCode: 0, + signal: null, + reason: null, + }, + { + command: commands[1], + status: 'failed', + exitCode: null, + signal: 'SIGTERM', + reason: null, + }, + ], + }, + error: `Setup command terminated by signal SIGTERM: ${commands[1]}`, + }); + expect(readFileSync(join(testDir, 'setup.log'), 'utf8')).toBe('first'); + }, + ); test('runs only commands matching the current platform and architecture', () => { const otherPlatform: NodeJS.Platform = diff --git a/tests/unit/core/native/types.test.ts b/tests/unit/core/native/types.test.ts index 734e35fc..66780049 100644 --- a/tests/unit/core/native/types.test.ts +++ b/tests/unit/core/native/types.test.ts @@ -1,8 +1,210 @@ import { describe, expect, test } from 'bun:test'; -import { mergeNativeSyncResults } from '../../../../src/core/native/types.js'; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { delimiter, join } from 'node:path'; +import { executeCommand, mergeNativeSyncResults } from '../../../../src/core/native/types.js'; import type { NativeSyncResult } from '../../../../src/core/native/types.js'; describe('native/types', () => { + describe('executeCommand', () => { + test('returns process creation errors instead of rejecting', async () => { + const result = await executeCommand(process.execPath, ['invalid\0argument']); + + expect(result.success).toBe(false); + expect(result.output).toBe(''); + expect(result.error).toContain(`Failed to execute ${process.execPath} CLI`); + }); + + test.skipIf(process.platform !== 'win32')( + 'executes supported npm shims safely without DEP0190 on Windows', + async () => { + const tempDir = mkdtempSync( + join(tmpdir(), 'allagents-execute-command-'), + ); + const scriptDir = join(tempDir, 'node_modules', 'test-cli'); + const scriptPath = join(scriptDir, 'print-argv.cjs'); + const shimPath = join(tempDir, 'argv-recorder.cmd'); + const nestedTargetPath = join(tempDir, 'nested-target.cmd'); + const nestedShimPath = join(tempDir, 'nested-wrapper.cmd'); + const runnerPath = join(tempDir, 'run-execute-command.mjs'); + const args = [ + 'value with spaces', + 'literal&operator', + 'literal|pipe', + 'literal;separator', + 'literal^caret', + 'literal%PATH%', + 'literal"quote', + '', + 'trailing\\', + 'backslash\\"quote', + 'literal\r\nnewline', + ]; + + try { + mkdirSync(scriptDir, { recursive: true }); + writeFileSync( + scriptPath, + [ + '#!/usr/bin/env node', + "const runtime = typeof Bun === 'undefined' ? 'node' : 'bun';", + 'const args = process.argv.slice(2);', + 'process.stdout.write(JSON.stringify({ runtime, args }));', + ].join('\n'), + ); + writeFileSync( + shimPath, + '@ECHO off\r\nnode "%~dp0\\node_modules\\test-cli\\print-argv.cjs" %*\r\n', + ); + writeFileSync( + join(tempDir, 'node.cmd'), + '@ECHO off\r\nECHO unsafe interpreter selected\r\n', + ); + writeFileSync(nestedTargetPath, '@ECHO off\r\nECHO nested batch ran\r\n'); + writeFileSync( + nestedShimPath, + '@ECHO off\r\n"%~dp0\\nested-target.cmd" %*\r\n', + ); + + const bundle = await Bun.build({ + entrypoints: [ + join(import.meta.dir, '../../../../src/core/native/types.ts'), + ], + outdir: tempDir, + target: 'node', + format: 'esm', + }); + expect(bundle.success).toBe(true); + writeFileSync( + runnerPath, + [ + "import { executeCommand } from './types.js';", + "const command = process.env.ALLAGENTS_TEST_COMMAND ?? 'argv-recorder';", + "const args = JSON.parse(process.env.ALLAGENTS_TEST_ARGS ?? '[]');", + 'const result = await executeCommand(command, args);', + 'process.stdout.write(JSON.stringify(result));', + ].join('\n'), + ); + + const runtimes = [ + ['node', '--trace-deprecation'], + [process.execPath], + ]; + const env = { ...process.env }; + const pathKey = + Object.keys(env).find((key) => key.toLowerCase() === 'path') ?? + 'PATH'; + const pathExtKey = + Object.keys(env).find((key) => key.toLowerCase() === 'pathext') ?? + 'PATHEXT'; + const originalPath = env[pathKey] ?? ''; + env[pathKey] = `${tempDir}${delimiter}${originalPath}`; + + for (const runtime of runtimes) { + const proc = Bun.spawnSync([...runtime, runnerPath], { + cwd: tempDir, + env: { + ...env, + ALLAGENTS_TEST_ARGS: JSON.stringify(args), + }, + stdout: 'pipe', + stderr: 'pipe', + }); + const stdout = new TextDecoder().decode(proc.stdout); + const stderr = new TextDecoder().decode(proc.stderr); + + expect(proc.exitCode).toBe(0); + expect(stderr).toBe(''); + expect(JSON.parse(stdout)).toEqual({ + success: true, + output: JSON.stringify({ runtime: 'node', args }), + }); + } + + const nestedProc = Bun.spawnSync(['node', runnerPath], { + cwd: tempDir, + env: { + ...env, + ALLAGENTS_TEST_COMMAND: 'nested-wrapper', + ALLAGENTS_TEST_ARGS: JSON.stringify(args), + }, + stdout: 'pipe', + stderr: 'pipe', + }); + const nestedResult = JSON.parse( + new TextDecoder().decode(nestedProc.stdout), + ); + expect(nestedProc.exitCode).toBe(0); + expect(new TextDecoder().decode(nestedProc.stderr)).toBe(''); + expect(nestedResult).toMatchObject({ + success: false, + output: '', + }); + expect(nestedResult.error).toContain( + `cannot safely execute command shim target '${nestedTargetPath}'`, + ); + + writeFileSync( + join(tempDir, 'node.com'), + 'This must not run when PATHEXT excludes .COM', + ); + const filteredPathExtProc = Bun.spawnSync( + [process.execPath, runnerPath], + { + cwd: tempDir, + env: { + ...env, + [pathExtKey]: '.CMD;.EXE', + ALLAGENTS_TEST_ARGS: JSON.stringify(args), + }, + stdout: 'pipe', + stderr: 'pipe', + }, + ); + expect(filteredPathExtProc.exitCode).toBe(0); + expect( + new TextDecoder().decode(filteredPathExtProc.stderr), + ).toBe(''); + expect( + JSON.parse(new TextDecoder().decode(filteredPathExtProc.stdout)), + ).toEqual({ + success: true, + output: JSON.stringify({ runtime: 'node', args }), + }); + + const cwdFallbackProc = Bun.spawnSync( + [process.execPath, runnerPath], + { + cwd: tempDir, + env: { + ...env, + [pathKey]: `${originalPath}${delimiter}`, + ALLAGENTS_TEST_COMMAND: 'nested-wrapper', + }, + stdout: 'pipe', + stderr: 'pipe', + }, + ); + const cwdFallbackResult = JSON.parse( + new TextDecoder().decode(cwdFallbackProc.stdout), + ); + expect(cwdFallbackProc.exitCode).toBe(0); + expect(new TextDecoder().decode(cwdFallbackProc.stderr)).toBe(''); + expect(cwdFallbackResult).toMatchObject({ + success: false, + output: '', + }); + expect(cwdFallbackResult.error).toContain( + 'command not found on PATH: nested-wrapper', + ); + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } + }, + 15_000, + ); + }); + describe('mergeNativeSyncResults', () => { test('merges two results', () => { const a: NativeSyncResult = {