diff --git a/.github/workflows/build-and-test.yml b/.github/workflows/build-and-test.yml index 43829b665a..19e4e2742d 100644 --- a/.github/workflows/build-and-test.yml +++ b/.github/workflows/build-and-test.yml @@ -134,6 +134,23 @@ jobs: if-no-files-found: error include-hidden-files: true + windows-subprocess-test: + name: windows-subprocess-test (20.x) + runs-on: >- + ${{ fromJSON(format('["codebuild-agentcore-e2e-{0}-{1}","image:windows-1.0","e2e-windows"]', github.run_id, + github.run_attempt)) }} + timeout-minutes: 10 + steps: + - uses: actions/checkout@v7 + - name: Use Node.js 20.x + uses: actions/setup-node@v7 + with: + node-version: 20.x + cache: 'npm' + - run: npm ci + - name: Run Windows subprocess tests + run: npx vitest run --project unit src/lib/utils/__tests__/subprocess-windows-real.test.ts + merge-reports: needs: unit-test runs-on: codebuild-agentcore-e2e-${{ github.run_id }}-${{ github.run_attempt }} diff --git a/eslint.config.mjs b/eslint.config.mjs index 189c302d75..5d110531a2 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -141,6 +141,24 @@ export default tseslint.config( }, }, prettier, + // Spawn only through src/lib/utils/subprocess.ts so bare command lookup stays consistent + // across platforms and uses explicit PATH entries on Windows. + { + files: ['src/**/*.{ts,tsx}'], + ignores: ['src/lib/utils/subprocess.ts', 'src/assets/**', '**/*.test.ts', '**/*.test.tsx', '**/test-utils/**'], + rules: { + '@typescript-eslint/no-restricted-imports': [ + 'error', + { + paths: ['child_process', 'node:child_process', 'cross-spawn'].map(name => ({ + name, + allowTypeImports: true, + message: 'Use spawn/spawnSync or the run*/check* helpers from src/lib/utils/subprocess.ts.', + })), + }, + ], + }, + }, // Relaxed rules for test files { files: ['**/*.test.ts', '**/*.test.tsx', '**/test-utils/**', 'integ-tests/**', 'browser-tests/**'], diff --git a/src/cli/cdk/toolkit-lib/wrapper.ts b/src/cli/cdk/toolkit-lib/wrapper.ts index e3b9158a95..72cdec9215 100644 --- a/src/cli/cdk/toolkit-lib/wrapper.ts +++ b/src/cli/cdk/toolkit-lib/wrapper.ts @@ -43,13 +43,6 @@ async function withErrorContext(context: string, operation: () => Promise) } } -/** - * Command to execute the CDK app. - * Matches the "app" command in cdk.json. - * Requires the CDK project to be built first (tsc). - */ -const CDK_APP_COMMAND = 'node'; - /** * Thin wrapper around @aws-cdk/toolkit-lib for AgentCore CLI. * @@ -85,7 +78,9 @@ export class CdkToolkitWrapper { * Points to the compiled JS file in dist/ (requires tsc build first). */ getCdkAppCommand(): string { - return `${CDK_APP_COMMAND} ${CDK_APP_ENTRY}`; + // The toolkit runs this command through a shell in the project directory. Use the + // absolute path of the Node binary running the CLI for consistent Windows lookup. + return `"${process.execPath}" ${CDK_APP_ENTRY}`; } /** diff --git a/src/cli/cli.ts b/src/cli/cli.ts index 042188bcb7..ff9f9d44d2 100644 --- a/src/cli/cli.ts +++ b/src/cli/cli.ts @@ -1,4 +1,5 @@ import { getOrCreateInstallationId } from '../lib/schemas/io/global-config'; +import { disableCwdExecutableSearch } from '../lib/utils/subprocess'; import { registerAdd } from './commands/add'; import { registerAddSkill } from './commands/add/skill-command'; import { registerAddTool } from './commands/add/tool-command'; @@ -139,6 +140,10 @@ export function registerCommands(program: Command) { } export const main = async (argv: string[]) => { + // Keep Windows executable lookup on explicit PATH entries, including for child processes + // spawned by third-party libraries. + disableCwdExecutableSearch(); + // Register global cleanup handlers once at startup setupAltScreenCleanup(); diff --git a/src/cli/commands/dev/command.tsx b/src/cli/commands/dev/command.tsx index c2950434fb..cbe659e88c 100644 --- a/src/cli/commands/dev/command.tsx +++ b/src/cli/commands/dev/command.tsx @@ -7,6 +7,7 @@ import { getWorkingDirectory, } from '../../../lib'; import { failureResult } from '../../../lib/result.js'; +import { spawn } from '../../../lib/utils/subprocess'; import { COMMAND_DESCRIPTIONS } from '../../constants'; import { getErrorMessage } from '../../errors'; import { detectContainerRuntime } from '../../external-requirements'; @@ -38,7 +39,6 @@ import { runCliDeploy } from '../deploy/progress'; import { parseHeaderFlags } from '../shared/header-utils'; import { launchTuiDevScreenWithPicker, runBrowserMode } from './browser-mode'; import type { Command } from '@commander-js/extra-typings'; -import { spawn } from 'child_process'; import { render } from 'ink'; import path from 'node:path'; import React from 'react'; diff --git a/src/cli/commands/export/__tests__/container-export.test.ts b/src/cli/commands/export/__tests__/container-export.test.ts index 7f7ca3eb4e..b022ae2c56 100644 --- a/src/cli/commands/export/__tests__/container-export.test.ts +++ b/src/cli/commands/export/__tests__/container-export.test.ts @@ -6,9 +6,9 @@ import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -vi.mock('node:child_process', async importOriginal => ({ +vi.mock('../../../../lib/utils/subprocess', async importOriginal => ({ ...(await importOriginal>()), - execSync: vi.fn(), + runSubprocessCaptureSync: vi.fn(() => ({ stdout: '', stderr: '', code: 0, signal: null })), })); describe('exported container Dockerfiles', () => { diff --git a/src/cli/commands/export/harness-action.ts b/src/cli/commands/export/harness-action.ts index 576547c7c3..4f9c687cc2 100644 --- a/src/cli/commands/export/harness-action.ts +++ b/src/cli/commands/export/harness-action.ts @@ -1,5 +1,6 @@ import { AgentAlreadyExistsError, ConfigIO, setEnvVar } from '../../../lib'; import { ExportHarnessError, ValidationError } from '../../../lib/errors/types'; +import { runSubprocessCaptureSync } from '../../../lib/utils/subprocess'; import { AgentNameSchema } from '../../../schema'; import type { AgentEnvSpec, BuildType, Credential, HarnessSpec } from '../../../schema'; import { getErrorMessage } from '../../errors'; @@ -23,7 +24,6 @@ import { fetchHarnessSpecByArn } from './fetch-harness-spec'; import { isPathSkill, mapHarnessToExportConfig } from './harness-mapper'; import { resolveHarnessContext } from './harness-resolver'; import type { ExportHarnessOptions, ExportNote, ResolvedHarnessContext } from './types'; -import { execSync } from 'node:child_process'; import { copyFileSync, cpSync, existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { basename, isAbsolute, join, resolve, sep } from 'node:path'; @@ -208,9 +208,7 @@ export async function handleExportHarness( // 5b. Generate uv.lock for Container builds (required by the Dockerfile's uv sync step) if (renderConfig.buildType === 'Container') { log('Generating uv.lock for container build'); - try { - execSync('uv lock', { cwd: agentDir, stdio: 'pipe' }); - } catch { + if (runSubprocessCaptureSync('uv', ['lock'], { cwd: agentDir }).code !== 0) { // uv not installed or failed — add a note and continue; user can run manually context.exportNotes.push({ category: 'uv.lock missing — run `uv lock` before deploying', diff --git a/src/cli/commands/update/__tests__/update-action.test.ts b/src/cli/commands/update/__tests__/update-action.test.ts index aa31709111..e24b4a0fb0 100644 --- a/src/cli/commands/update/__tests__/update-action.test.ts +++ b/src/cli/commands/update/__tests__/update-action.test.ts @@ -1,12 +1,12 @@ import { compareVersions, fetchLatestVersion, handleUpdate } from '../action.js'; import { afterEach, describe, expect, it, vi } from 'vitest'; -const { mockExecSync } = vi.hoisted(() => ({ - mockExecSync: vi.fn(), +const { mockRunSubprocess } = vi.hoisted(() => ({ + mockRunSubprocess: vi.fn(), })); -vi.mock('child_process', () => ({ - execSync: mockExecSync, +vi.mock('../../../../lib/utils/subprocess', () => ({ + runSubprocess: mockRunSubprocess, })); vi.mock('../../../constants.js', () => ({ @@ -104,7 +104,7 @@ describe('fetchLatestVersion', () => { describe('handleUpdate', () => { afterEach(() => { vi.restoreAllMocks(); - mockExecSync.mockReset(); + mockRunSubprocess.mockReset(); }); it('returns up-to-date when versions match', async () => { @@ -140,7 +140,7 @@ describe('handleUpdate', () => { const result = await handleUpdate(true); expect(result.status).toBe('update-available'); - expect(mockExecSync).not.toHaveBeenCalled(); + expect(mockRunSubprocess).not.toHaveBeenCalled(); }); it('returns updated after successful install', async () => { @@ -148,12 +148,14 @@ describe('handleUpdate', () => { ok: true, json: () => Promise.resolve({ version: '2.0.0' }), } as Response); - mockExecSync.mockReturnValue(undefined); + mockRunSubprocess.mockResolvedValue(undefined); const result = await handleUpdate(false); expect(result.status).toBe('updated'); - expect(mockExecSync).toHaveBeenCalledWith('npm install -g @aws/agentcore@latest', { stdio: 'inherit' }); + expect(mockRunSubprocess).toHaveBeenCalledWith('npm', ['install', '-g', '@aws/agentcore@latest'], { + stdio: 'inherit', + }); }); it('returns update-failed when install throws', async () => { @@ -161,9 +163,7 @@ describe('handleUpdate', () => { ok: true, json: () => Promise.resolve({ version: '2.0.0' }), } as Response); - mockExecSync.mockImplementation(() => { - throw new Error('install failed'); - }); + mockRunSubprocess.mockRejectedValue(new Error('install failed')); const result = await handleUpdate(false); diff --git a/src/cli/commands/update/action.ts b/src/cli/commands/update/action.ts index a32f576102..f39a7b0463 100644 --- a/src/cli/commands/update/action.ts +++ b/src/cli/commands/update/action.ts @@ -1,5 +1,5 @@ +import { runSubprocess } from '../../../lib/utils/subprocess'; import { PACKAGE_VERSION, getDistroConfig } from '../../constants'; -import { execSync } from 'child_process'; const distroConfig = getDistroConfig(); @@ -88,7 +88,8 @@ export async function handleUpdate(checkOnly: boolean): Promise { } try { - execSync(distroConfig.installCommand, { stdio: 'inherit' }); + const [installBinary, ...installArgs] = distroConfig.installCommand.trim().split(/\s+/); + await runSubprocess(installBinary!, installArgs, { stdio: 'inherit' }); return { status: 'updated', currentVersion: PACKAGE_VERSION, latestVersion }; } catch { return { status: 'update-failed', currentVersion: PACKAGE_VERSION, latestVersion }; diff --git a/src/cli/external-requirements/__tests__/detect.test.ts b/src/cli/external-requirements/__tests__/detect.test.ts index 2c3634ad5d..ce460980fa 100644 --- a/src/cli/external-requirements/__tests__/detect.test.ts +++ b/src/cli/external-requirements/__tests__/detect.test.ts @@ -1,23 +1,22 @@ import { detectContainerRuntime, requireContainerRuntime } from '../detect.js'; import { afterEach, describe, expect, it, vi } from 'vitest'; -const { mockCheckSubprocess, mockRunSubprocessCapture } = vi.hoisted(() => ({ - mockCheckSubprocess: vi.fn(), +const { mockResolveExecutable, mockRunSubprocessCapture } = vi.hoisted(() => ({ + mockResolveExecutable: vi.fn(), mockRunSubprocessCapture: vi.fn(), })); vi.mock('../../../lib', () => ({ CONTAINER_RUNTIMES: ['docker', 'podman', 'finch'], - checkSubprocess: mockCheckSubprocess, + resolveExecutable: mockResolveExecutable, runSubprocessCapture: mockRunSubprocessCapture, - isWindows: false, })); afterEach(() => vi.clearAllMocks()); describe('detectContainerRuntime', () => { it('returns docker when docker is installed', async () => { - mockCheckSubprocess.mockResolvedValue(true); + mockResolveExecutable.mockImplementation((bin: string) => `/usr/bin/${bin}`); mockRunSubprocessCapture.mockImplementation((_bin: string, args: string[]) => { if (args[0] === '--version') return Promise.resolve({ code: 0, stdout: 'Docker version 24.0.0\n', stderr: '' }); if (args[0] === 'build') return Promise.resolve({ code: 0, stdout: '', stderr: '' }); @@ -29,11 +28,7 @@ describe('detectContainerRuntime', () => { }); it('falls back to podman when docker not installed', async () => { - mockCheckSubprocess.mockImplementation((_cmd: string, args: string[]) => { - if (args[0] === 'docker') return Promise.resolve(false); - if (args[0] === 'podman') return Promise.resolve(true); - return Promise.resolve(false); - }); + mockResolveExecutable.mockImplementation((bin: string) => (bin === 'podman' ? '/usr/bin/podman' : null)); mockRunSubprocessCapture.mockImplementation((bin: string, args: string[]) => { if (bin === 'podman' && args[0] === '--version') return Promise.resolve({ code: 0, stdout: 'podman version 4.5.0\n', stderr: '' }); @@ -46,14 +41,14 @@ describe('detectContainerRuntime', () => { }); it('returns null runtime when nothing is installed', async () => { - mockCheckSubprocess.mockResolvedValue(false); + mockResolveExecutable.mockReturnValue(null); const result = await detectContainerRuntime(); expect(result.runtime).toBeNull(); }); it('skips runtime when --version check fails', async () => { - mockCheckSubprocess.mockResolvedValue(true); + mockResolveExecutable.mockImplementation((bin: string) => `/usr/bin/${bin}`); mockRunSubprocessCapture.mockImplementation((bin: string, args: string[]) => { // docker --version fails, podman works if (bin === 'docker' && args[0] === '--version') return Promise.resolve({ code: 1, stdout: '', stderr: 'error' }); @@ -70,7 +65,7 @@ describe('detectContainerRuntime', () => { }); it('extracts first line of --version output as version string', async () => { - mockCheckSubprocess.mockResolvedValue(true); + mockResolveExecutable.mockImplementation((bin: string) => `/usr/bin/${bin}`); mockRunSubprocessCapture.mockImplementation((_bin: string, args: string[]) => { if (args[0] === '--version') return Promise.resolve({ code: 0, stdout: 'Docker version 24.0.0\nExtra info line\n', stderr: '' }); @@ -83,7 +78,7 @@ describe('detectContainerRuntime', () => { }); it('uses empty first line when version output is empty', async () => { - mockCheckSubprocess.mockResolvedValue(true); + mockResolveExecutable.mockImplementation((bin: string) => `/usr/bin/${bin}`); mockRunSubprocessCapture.mockImplementation((_bin: string, args: string[]) => { if (args[0] === '--version') return Promise.resolve({ code: 0, stdout: '', stderr: '' }); if (args[0] === 'build') return Promise.resolve({ code: 0, stdout: '', stderr: '' }); @@ -96,7 +91,7 @@ describe('detectContainerRuntime', () => { }); it('skips runtime when build --help check fails with non-zero exit', async () => { - mockCheckSubprocess.mockResolvedValue(true); + mockResolveExecutable.mockImplementation((bin: string) => `/usr/bin/${bin}`); mockRunSubprocessCapture.mockImplementation((_bin: string, args: string[]) => { if (args[0] === '--version') return Promise.resolve({ code: 0, stdout: '1.0.0\n', stderr: '' }); if (args[0] === 'build') return Promise.resolve({ code: 1, stdout: '', stderr: 'unknown command "build"' }); @@ -108,7 +103,7 @@ describe('detectContainerRuntime', () => { }); it('skips runtime when build --help exits 0 but stderr indicates unknown command (shim/wrapper)', async () => { - mockCheckSubprocess.mockResolvedValue(true); + mockResolveExecutable.mockImplementation((bin: string) => `/usr/bin/${bin}`); mockRunSubprocessCapture.mockImplementation((_bin: string, args: string[]) => { // Shim exits 0 for everything but prints error to stderr if (args[0] === '--version') return Promise.resolve({ code: 0, stdout: '1.0.0\n', stderr: '' }); @@ -124,7 +119,7 @@ describe('detectContainerRuntime', () => { describe('requireContainerRuntime', () => { it('returns runtime info when available', async () => { - mockCheckSubprocess.mockResolvedValue(true); + mockResolveExecutable.mockImplementation((bin: string) => `/usr/bin/${bin}`); mockRunSubprocessCapture.mockImplementation((_bin: string, args: string[]) => { if (args[0] === '--version') return Promise.resolve({ code: 0, stdout: 'Docker version 24.0.0\n', stderr: '' }); if (args[0] === 'build') return Promise.resolve({ code: 0, stdout: '', stderr: '' }); @@ -136,7 +131,7 @@ describe('requireContainerRuntime', () => { }); it('throws with install links when no runtime found', async () => { - mockCheckSubprocess.mockResolvedValue(false); + mockResolveExecutable.mockReturnValue(null); await expect(requireContainerRuntime()).rejects.toThrow('No container runtime found'); await expect(requireContainerRuntime()).rejects.toThrow('https://docker.com'); diff --git a/src/cli/external-requirements/checks.ts b/src/cli/external-requirements/checks.ts index c369bfec02..c90b449bc7 100644 --- a/src/cli/external-requirements/checks.ts +++ b/src/cli/external-requirements/checks.ts @@ -1,7 +1,7 @@ /** * External dependency version checks. */ -import { checkSubprocess, isWindows, runSubprocessCapture } from '../../lib'; +import { checkSubprocess, isWindows, resolveExecutable, runSubprocessCapture } from '../../lib'; import type { AgentCoreProjectSpec, TargetLanguage } from '../../schema'; import { detectContainerRuntime } from './detect'; import { AWS_CLI_MIN_VERSION, NODE_MIN_VERSION, formatSemVer, parseSemVer, semVerGte } from './versions'; @@ -399,8 +399,8 @@ export async function checkCreateDependencies( async function checkBinaryAvailable(binary: string): Promise { // Try multiple detection strategies const checks = [ - // Primary: use 'where' on Windows, 'which' on Unix - () => (isWindows ? checkSubprocess('where', [binary]) : checkSubprocess('which', [binary])), + // Primary: look it up on PATH (never the current directory) + () => Promise.resolve(resolveExecutable(binary) !== null), // Fallback: try running with --version () => checkSubprocess(binary, ['--version']), // Fallback: try running with -v diff --git a/src/cli/external-requirements/detect.ts b/src/cli/external-requirements/detect.ts index 7e8a4bb124..f15c80ecdd 100644 --- a/src/cli/external-requirements/detect.ts +++ b/src/cli/external-requirements/detect.ts @@ -3,7 +3,7 @@ * Detects Docker, Podman, or Finch for container operations. */ import { CONTAINER_RUNTIMES, type ContainerRuntime } from '../../lib'; -import { checkSubprocess, isWindows, runSubprocessCapture } from '../../lib'; +import { resolveExecutable, runSubprocessCapture } from '../../lib'; export type { ContainerRuntime } from '../../lib'; @@ -26,8 +26,7 @@ export interface DetectionResult { export async function detectContainerRuntime(): Promise { for (const runtime of CONTAINER_RUNTIMES) { // Check if binary exists - const exists = isWindows ? await checkSubprocess('where', [runtime]) : await checkSubprocess('which', [runtime]); - if (!exists) continue; + if (resolveExecutable(runtime) === null) continue; // Verify with --version const result = await runSubprocessCapture(runtime, ['--version']); diff --git a/src/cli/operations/dev/__tests__/codezip-dev-server.test.ts b/src/cli/operations/dev/__tests__/codezip-dev-server.test.ts index c582305dfd..4716b9baf7 100644 --- a/src/cli/operations/dev/__tests__/codezip-dev-server.test.ts +++ b/src/cli/operations/dev/__tests__/codezip-dev-server.test.ts @@ -1,7 +1,7 @@ +import { spawnSync } from '../../../../lib/utils/subprocess'; import { CodeZipDevServer } from '../codezip-dev-server'; import type { DevConfig } from '../config'; import type { DevServerCallbacks, DevServerOptions } from '../dev-server'; -import { spawnSync } from 'child_process'; import { EventEmitter } from 'events'; import { existsSync } from 'fs'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; @@ -9,11 +9,9 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; const mockSpawn = vi.fn(); const mockRunSubprocessCapture = vi.fn(); const platformState = vi.hoisted(() => ({ isWindows: false })); -vi.mock('child_process', () => ({ +vi.mock('../../../../lib/utils/subprocess', () => ({ spawn: (...args: unknown[]) => mockSpawn(...args), spawnSync: vi.fn(() => ({ status: 0, stdout: Buffer.from(''), stderr: Buffer.from('') })), -})); -vi.mock('../../../../lib/utils/subprocess', () => ({ runSubprocessCapture: (...args: unknown[]) => mockRunSubprocessCapture(...args), })); @@ -169,7 +167,7 @@ describe('CodeZipDevServer spawn config', () => { expect(env.LOCAL_DEV).toBe('1'); }); - it('TypeScript HTTP: runs npx through cmd on Windows', async () => { + it('TypeScript HTTP: spawns npx directly on Windows (no cmd.exe, no cwd lookup)', async () => { platformState.isWindows = true; const config: DevConfig = { agentName: 'TsAgent', @@ -185,8 +183,8 @@ describe('CodeZipDevServer spawn config', () => { await server.start(); expect(mockSpawn).toHaveBeenCalledWith( - 'cmd', - ['/c', 'npx tsx watch src/main.ts'], + 'npx', + ['tsx', 'watch', 'src/main.ts'], expect.objectContaining({ cwd: 'C:\\project\\app', detached: false }) ); }); diff --git a/src/cli/operations/dev/__tests__/container-dev-server.test.ts b/src/cli/operations/dev/__tests__/container-dev-server.test.ts index 57772446b8..d09b821ffa 100644 --- a/src/cli/operations/dev/__tests__/container-dev-server.test.ts +++ b/src/cli/operations/dev/__tests__/container-dev-server.test.ts @@ -12,7 +12,7 @@ const mockExistsSync = vi.fn(); const mockDetectContainerRuntime = vi.fn(); const mockWaitForServerReady = vi.fn(); -vi.mock('child_process', () => ({ +vi.mock('../../../../lib/utils/subprocess', () => ({ spawnSync: (...args: unknown[]) => mockSpawnSync(...args), spawn: (...args: unknown[]) => mockSpawn(...args), })); diff --git a/src/cli/operations/dev/__tests__/dev-server.test.ts b/src/cli/operations/dev/__tests__/dev-server.test.ts index 3e00e0c146..42e36fb9d0 100644 --- a/src/cli/operations/dev/__tests__/dev-server.test.ts +++ b/src/cli/operations/dev/__tests__/dev-server.test.ts @@ -4,8 +4,9 @@ import { EventEmitter } from 'events'; import { type MockInstance, afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; const mockSpawn = vi.fn(); -vi.mock('child_process', () => ({ +vi.mock('../../../../lib/utils/subprocess', () => ({ spawn: (...args: unknown[]) => mockSpawn(...args), + spawnSync: vi.fn(), execSync: vi.fn(), })); vi.mock('../../../../lib/utils/platform', () => ({ diff --git a/src/cli/operations/dev/__tests__/utils-open-browser.test.ts b/src/cli/operations/dev/__tests__/utils-open-browser.test.ts index dfb69359a2..d98f8a5ffc 100644 --- a/src/cli/operations/dev/__tests__/utils-open-browser.test.ts +++ b/src/cli/operations/dev/__tests__/utils-open-browser.test.ts @@ -3,7 +3,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; const mockUnref = vi.fn(); const mockSpawn = vi.fn().mockReturnValue({ unref: mockUnref }); -vi.mock('child_process', () => ({ +vi.mock('../../../../lib/utils/subprocess', () => ({ spawn: (...args: unknown[]) => mockSpawn(...args), })); diff --git a/src/cli/operations/dev/codezip-dev-server.ts b/src/cli/operations/dev/codezip-dev-server.ts index 10db6cc60c..5a3eb7ccc4 100644 --- a/src/cli/operations/dev/codezip-dev-server.ts +++ b/src/cli/operations/dev/codezip-dev-server.ts @@ -1,10 +1,9 @@ -import { getShellArgs, getShellCommand, getVenvExecutable, isWindows } from '../../../lib/utils/platform'; -import { runSubprocessCapture } from '../../../lib/utils/subprocess'; +import { getVenvExecutable } from '../../../lib/utils/platform'; +import { runSubprocessCapture, spawnSync } from '../../../lib/utils/subprocess'; import type { ProtocolMode } from '../../../schema'; import { A2A_PORT_ENV } from './constants'; import { DevServer, type LogLevel, type SpawnConfig } from './dev-server'; import { convertEntrypointToModule } from './utils'; -import { spawnSync } from 'child_process'; import { existsSync } from 'fs'; import { delimiter, join } from 'path'; @@ -156,14 +155,6 @@ export class CodeZipDevServer extends DevServer { if (!isPython) { // TS entrypoint is already a file path like "main.ts" — pass it straight to tsx. const entryFile = module.split(':')[0] ?? module; - if (isWindows) { - return { - cmd: getShellCommand(), - args: getShellArgs(`npx tsx watch ${entryFile}`), - cwd: directory, - env, - }; - } return { cmd: 'npx', args: ['tsx', 'watch', entryFile], diff --git a/src/cli/operations/dev/container-dev-server.ts b/src/cli/operations/dev/container-dev-server.ts index 647646a8e4..aea140c5de 100644 --- a/src/cli/operations/dev/container-dev-server.ts +++ b/src/cli/operations/dev/container-dev-server.ts @@ -1,11 +1,12 @@ import { CONTAINER_INTERNAL_PORT, DOCKERFILE_NAME, getDockerfilePath } from '../../../lib'; import { getCustomBuildArgs, getUvBuildArgs } from '../../../lib/packaging/build-args'; import { ensureBuildContextDockerignore } from '../../../lib/packaging/build-context-dockerignore'; +import { spawn, spawnSync } from '../../../lib/utils/subprocess'; import { detectContainerRuntime } from '../../external-requirements/detect'; import { A2A_DEFAULT_PORT, A2A_PORT_ENV, MCP_DEFAULT_PORT } from './constants'; import { DevServer, type LogLevel, type SpawnConfig } from './dev-server'; import { waitForServerReady } from './utils'; -import { type ChildProcess, spawn, spawnSync } from 'child_process'; +import type { ChildProcess } from 'child_process'; import { existsSync } from 'fs'; import { homedir } from 'os'; import { join } from 'path'; diff --git a/src/cli/operations/dev/dev-server.ts b/src/cli/operations/dev/dev-server.ts index 3d208eb510..8bfa9080e6 100644 --- a/src/cli/operations/dev/dev-server.ts +++ b/src/cli/operations/dev/dev-server.ts @@ -1,6 +1,7 @@ import { isWindows } from '../../../lib/utils/platform'; +import { spawn, spawnSync } from '../../../lib/utils/subprocess'; import type { DevConfig } from './config'; -import { type ChildProcess, execSync, spawn } from 'child_process'; +import type { ChildProcess } from 'child_process'; export type LogLevel = 'info' | 'warn' | 'error' | 'system'; @@ -128,7 +129,7 @@ export abstract class DevServer { } try { if (isWindows) { - execSync(`taskkill /pid ${pid} /T /F`, { stdio: 'ignore' }); + spawnSync('taskkill', ['/pid', String(pid), '/T', '/F'], { stdio: 'ignore' }); } else { process.kill(-pid, signal); } diff --git a/src/cli/operations/dev/utils.ts b/src/cli/operations/dev/utils.ts index 3705103161..37ef780679 100644 --- a/src/cli/operations/dev/utils.ts +++ b/src/cli/operations/dev/utils.ts @@ -1,4 +1,4 @@ -import { spawn } from 'child_process'; +import { spawn } from '../../../lib/utils/subprocess'; import { createConnection, createServer } from 'net'; /** Check if a port is available on a specific host */ diff --git a/src/cli/tui/hooks/useDevServer.ts b/src/cli/tui/hooks/useDevServer.ts index aebc518fa9..82374b9add 100644 --- a/src/cli/tui/hooks/useDevServer.ts +++ b/src/cli/tui/hooks/useDevServer.ts @@ -1,5 +1,6 @@ import { findConfigRoot } from '../../../lib'; import { DevServerConnectionError, DevServerError } from '../../../lib/errors/types'; +import { spawn } from '../../../lib/utils/subprocess'; import type { AgentCoreProjectSpec, ProtocolMode } from '../../../schema'; import { detectContainerRuntime } from '../../external-requirements'; import { DevLogger } from '../../logging/dev-logger'; @@ -26,7 +27,6 @@ import { waitForPort, } from '../../operations/dev'; import { formatMcpToolList } from '../../operations/dev/utils'; -import { spawn } from 'child_process'; import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; type ServerStatus = 'starting' | 'running' | 'error' | 'stopped'; diff --git a/src/cli/tui/screens/fetch-access/useFetchAccessFlow.ts b/src/cli/tui/screens/fetch-access/useFetchAccessFlow.ts index bec80d2b8f..da63b0b723 100644 --- a/src/cli/tui/screens/fetch-access/useFetchAccessFlow.ts +++ b/src/cli/tui/screens/fetch-access/useFetchAccessFlow.ts @@ -1,4 +1,5 @@ import { isMacOS, isWindows } from '../../../../lib/utils/platform'; +import { spawn } from '../../../../lib/utils/subprocess'; import { getErrorMessage } from '../../../errors'; import type { OAuthTokenResult, ResourceInfo, TokenFetchResult } from '../../../operations/fetch-access'; import { @@ -9,7 +10,6 @@ import { listGateways, listHarnesses, } from '../../../operations/fetch-access'; -import { spawn } from 'node:child_process'; import { useCallback, useEffect, useRef, useState } from 'react'; /** @@ -199,8 +199,8 @@ export function useFetchAccessFlow() { const child = spawn(cmd, args, { stdio: ['pipe', 'ignore', 'ignore'] }); // eslint-disable-next-line @typescript-eslint/no-empty-function child.on('error', () => {}); - child.stdin.write(token); - child.stdin.end(); + child.stdin?.write(token); + child.stdin?.end(); setCopied(true); if (copiedTimerRef.current) clearTimeout(copiedTimerRef.current); diff --git a/src/lib/packaging/__tests__/container.test.ts b/src/lib/packaging/__tests__/container.test.ts index 0151372264..b0c1e80d97 100644 --- a/src/lib/packaging/__tests__/container.test.ts +++ b/src/lib/packaging/__tests__/container.test.ts @@ -5,8 +5,11 @@ const mockSpawnSync = vi.fn(); const mockExistsSync = vi.fn(); const mockResolveCodeLocation = vi.fn(); -vi.mock('child_process', () => ({ +vi.mock('../../utils/subprocess', () => ({ spawnSync: (...args: unknown[]) => mockSpawnSync(...args), + // Detection no longer shells out to `which`; derive PATH lookup from the same mock. + resolveExecutable: (cmd: string) => + (mockSpawnSync('which', [cmd]) as { status: number }).status === 0 ? `/usr/bin/${cmd}` : null, })); vi.mock('fs', () => ({ diff --git a/src/lib/packaging/container.ts b/src/lib/packaging/container.ts index b70eab5689..8de96017ca 100644 --- a/src/lib/packaging/container.ts +++ b/src/lib/packaging/container.ts @@ -1,12 +1,12 @@ import type { AgentEnvSpec } from '../../schema'; import { CONTAINER_RUNTIMES, DOCKERFILE_NAME, ONE_GB } from '../constants'; import { PackagingError } from '../errors/types'; +import { resolveExecutable, spawnSync } from '../utils/subprocess'; import { getCustomBuildArgs, getUvBuildArgs } from './build-args'; import { resolveBuildContext } from './build-context'; import { ensureBuildContextDockerignore } from './build-context-dockerignore'; import { resolveCodeLocation } from './helpers'; import type { ArtifactResult, PackageOptions, RuntimePackager } from './types/packaging'; -import { spawnSync } from 'child_process'; import { existsSync } from 'fs'; /** @@ -15,8 +15,7 @@ import { existsSync } from 'fs'; */ function detectContainerRuntimeSync(): string | null { for (const runtime of CONTAINER_RUNTIMES) { - const result = spawnSync('which', [runtime], { stdio: 'pipe' }); - if (result.status === 0) { + if (resolveExecutable(runtime) !== null) { const versionResult = spawnSync(runtime, ['--version'], { stdio: 'pipe' }); if (versionResult.status === 0) return runtime; } diff --git a/src/lib/packaging/helpers.ts b/src/lib/packaging/helpers.ts index e45c863018..d8d79d66e4 100644 --- a/src/lib/packaging/helpers.ts +++ b/src/lib/packaging/helpers.ts @@ -2,7 +2,7 @@ import type { RuntimeVersion } from '../../schema'; import { CONFIG_DIR } from '../constants'; import { ArtifactSizeError, MissingDependencyError, MissingProjectFileError } from '../errors/types'; import { isWindows } from '../utils/platform'; -import { checkSubprocess, checkSubprocessSync, runSubprocess } from '../utils/subprocess'; +import { checkSubprocess, checkSubprocessSync, resolveExecutable, runSubprocess } from '../utils/subprocess'; import type { PackageOptions } from './types/packaging'; import type { Zippable } from 'fflate'; import { zipSync } from 'fflate'; @@ -211,7 +211,7 @@ export async function copySourceTree(srcDir: string, destination: string): Promi export async function ensureBinaryAvailable(binary: string, installHint?: string): Promise { const checks: (() => Promise)[] = [ - () => (isWindows ? checkSubprocess('where', [binary]) : checkSubprocess('which', [binary])), + () => Promise.resolve(resolveExecutable(binary) !== null), () => checkSubprocess(binary, ['--version']), () => checkSubprocess(binary, ['-v']), ]; @@ -421,7 +421,7 @@ export function copySourceTreeSync(srcDir: string, destination: string): void { export function ensureBinaryAvailableSync(binary: string, installHint?: string): void { const checks: (() => boolean)[] = [ - () => (isWindows ? checkSubprocessSync('where', [binary]) : checkSubprocessSync('which', [binary])), + () => resolveExecutable(binary) !== null, () => checkSubprocessSync(binary, ['--version']), () => checkSubprocessSync(binary, ['-v']), ]; diff --git a/src/lib/utils/__tests__/subprocess-win32.test.ts b/src/lib/utils/__tests__/subprocess-win32.test.ts new file mode 100644 index 0000000000..c3bc3a7dda --- /dev/null +++ b/src/lib/utils/__tests__/subprocess-win32.test.ts @@ -0,0 +1,64 @@ +import { NO_CWD_EXE_SEARCH_ENV, spawn, spawnSync } from '../subprocess.js'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +const { mockCrossSpawn, mockCrossSpawnSync, mockStat } = vi.hoisted(() => ({ + mockCrossSpawn: vi.fn(), + mockCrossSpawnSync: vi.fn(), + mockStat: vi.fn(), +})); + +vi.mock('cross-spawn', () => ({ + default: Object.assign(mockCrossSpawn, { sync: mockCrossSpawnSync }), +})); + +vi.mock('fs', async importOriginal => ({ + ...(await importOriginal>()), + statSync: mockStat, +})); + +// Only the npm command on PATH is eligible; C:\project\npm.cmd is not consulted. +const REAL_NPM = 'C:\\Program Files\\nodejs\\npm.cmd'; +const env = { Path: 'C:\\Program Files\\nodejs', PATHEXT: '.EXE;.CMD' }; + +describe('spawn / spawnSync on Windows', () => { + const originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform')!; + + beforeEach(() => { + Object.defineProperty(process, 'platform', { value: 'win32' }); + mockStat.mockImplementation((p: string) => { + if (p.toLowerCase() === REAL_NPM.toLowerCase()) return { isFile: () => true }; + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); + }); + }); + + afterEach(() => { + Object.defineProperty(process, 'platform', originalPlatform); + vi.clearAllMocks(); + }); + + it('spawn passes the absolute PATH match to cross-spawn and disables cwd lookup in the child', () => { + spawn('npm', ['install'], { cwd: 'C:\\project', env }); + + expect(mockCrossSpawn).toHaveBeenCalledTimes(1); + const [command, args, options] = mockCrossSpawn.mock.calls[0]! as [string, string[], { env: NodeJS.ProcessEnv }]; + expect(command.toLowerCase()).toBe(REAL_NPM.toLowerCase()); + expect(args).toEqual(['install']); + expect(options.env[NO_CWD_EXE_SEARCH_ENV]).toBe('1'); + expect(options.env.Path).toBe(env.Path); + }); + + it('spawnSync passes the absolute PATH match to cross-spawn and disables cwd lookup in the child', () => { + mockCrossSpawnSync.mockReturnValue({ status: 0, stdout: '', stderr: '', signal: null, pid: 1, output: [] }); + spawnSync('npm', ['--version'], { cwd: 'C:\\project', env, encoding: 'utf-8' }); + + const [command, , options] = mockCrossSpawnSync.mock.calls[0]! as [string, string[], { env: NodeJS.ProcessEnv }]; + expect(command.toLowerCase()).toBe(REAL_NPM.toLowerCase()); + expect(options.env[NO_CWD_EXE_SEARCH_ENV]).toBe('1'); + }); + + it('never hands a bare, unresolved name to cross-spawn', () => { + const child = spawn('where', ['npm'], { cwd: 'C:\\project', env }); + child.on('error', () => undefined); + expect(mockCrossSpawn).not.toHaveBeenCalled(); + }); +}); diff --git a/src/lib/utils/__tests__/subprocess-windows-real.test.ts b/src/lib/utils/__tests__/subprocess-windows-real.test.ts new file mode 100644 index 0000000000..93ef5b926a --- /dev/null +++ b/src/lib/utils/__tests__/subprocess-windows-real.test.ts @@ -0,0 +1,72 @@ +import { runSubprocessCapture, spawnSync } from '../subprocess.js'; +import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'fs'; +import { tmpdir } from 'os'; +import { dirname, join } from 'path'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; + +const windowsDescribe = process.platform === 'win32' ? describe : describe.skip; + +windowsDescribe('Windows command and argument handling', () => { + let rootDir: string; + let binDir: string; + let markerPath: string; + let env: NodeJS.ProcessEnv; + + const args = [ + 'plain', + 'with spaces', + 'ampersand & value', + 'pipe | value', + 'redirect > value', + 'caret ^ value', + 'percent %PATH% value', + 'bang !PATH! value', + '(parentheses)', + '"quoted"', + ]; + + beforeAll(() => { + rootDir = mkdtempSync(join(tmpdir(), 'agentcore-spawn-')); + binDir = join(rootDir, 'bin & %CD% ! ^ (meta)'); + markerPath = join(rootDir, 'unexpected-command.txt'); + mkdirSync(binDir); + + const scriptPath = join(rootDir, 'print-args.cjs'); + writeFileSync(scriptPath, 'process.stdout.write(JSON.stringify(process.argv.slice(2)));'); + + const commandPath = join(binDir, 'print-args.cmd'); + const nodePathForBatch = process.execPath.replaceAll('%', '%%'); + const scriptPathForBatch = scriptPath.replaceAll('%', '%%'); + writeFileSync(commandPath, `@echo off\r\n"${nodePathForBatch}" "${scriptPathForBatch}" %*\r\n`); + + env = Object.fromEntries( + Object.entries(process.env).filter(([key]) => !['PATH', 'PATHEXT'].includes(key.toUpperCase())) + ); + env.Path = `${binDir};${dirname(process.execPath)}`; + env.PATHEXT = '.CMD;.EXE;.BAT;.COM'; + }); + + afterAll(() => { + rmSync(rootDir, { recursive: true, force: true }); + }); + + it('preserves metacharacters through the asynchronous wrapper', async () => { + const commandArgs = [...args, `& echo unexpected > "${markerPath}"`]; + const result = await runSubprocessCapture('print-args', commandArgs, { env }); + + expect(result.code).toBe(0); + expect(result.stderr).toBe(''); + expect(JSON.parse(result.stdout)).toEqual(commandArgs); + expect(existsSync(markerPath)).toBe(false); + }); + + it('preserves metacharacters through the synchronous wrapper', () => { + const commandArgs = [...args, `& echo unexpected > "${markerPath}"`]; + const result = spawnSync('print-args', commandArgs, { env, encoding: 'utf-8' }); + + expect(result.status).toBe(0); + expect(result.stderr).toBe(''); + expect(JSON.parse(result.stdout)).toEqual(commandArgs); + expect(existsSync(markerPath)).toBe(false); + }); +}); diff --git a/src/lib/utils/__tests__/subprocess.test.ts b/src/lib/utils/__tests__/subprocess.test.ts index 0326fe7f8b..f0b395d9c0 100644 --- a/src/lib/utils/__tests__/subprocess.test.ts +++ b/src/lib/utils/__tests__/subprocess.test.ts @@ -1,11 +1,39 @@ import { + NO_CWD_EXE_SEARCH_ENV, checkSubprocess, checkSubprocessSync, + disableCwdExecutableSearch, + resolveExecutable, runSubprocess, runSubprocessCapture, runSubprocessCaptureSync, + spawn, + spawnSync, } from '../subprocess.js'; -import { describe, expect, it } from 'vitest'; +import { chmodSync, mkdtempSync, rmSync, writeFileSync } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; + +function withPlatform(platform: NodeJS.Platform, fn: () => void) { + const original = Object.getOwnPropertyDescriptor(process, 'platform')!; + Object.defineProperty(process, 'platform', { value: platform }); + try { + fn(); + } finally { + Object.defineProperty(process, 'platform', original); + } +} + +async function withPlatformAsync(platform: NodeJS.Platform, fn: () => Promise) { + const original = Object.getOwnPropertyDescriptor(process, 'platform')!; + Object.defineProperty(process, 'platform', { value: platform }); + try { + await fn(); + } finally { + Object.defineProperty(process, 'platform', original); + } +} describe('runSubprocess', () => { it('resolves on success (exit code 0)', async () => { @@ -97,3 +125,193 @@ describe('checkSubprocessSync', () => { expect(checkSubprocessSync('__nonexistent_command_xyz__', [])).toBe(false); }); }); + +describe('resolveExecutable (Windows rules)', () => { + // Simulated Windows filesystem: the project directory also contains command wrappers. + const files = new Set([ + 'C:\\project\\npm.cmd', + 'C:\\project\\node.exe', + 'C:\\project\\where.cmd', + 'C:\\Program Files\\nodejs\\npm.cmd', + 'C:\\Program Files\\nodejs\\node.exe', + 'C:\\Windows\\System32\\where.exe', + ]); + // Windows paths are case-insensitive (PATHEXT yields "npm.CMD"). + const lowerFiles = new Set([...files].map(f => f.toLowerCase())); + const isExecutableFile = (p: string) => lowerFiles.has(p.toLowerCase()); + const env = { + Path: 'C:\\Program Files\\nodejs;C:\\Windows\\System32', + PATHEXT: '.COM;.EXE;.BAT;.CMD', + }; + const resolve = (cmd: string, e: NodeJS.ProcessEnv = env) => + resolveExecutable(cmd, { env: e, platform: 'win32', isExecutableFile }); + + it('resolves from PATH, never the current directory', () => { + expect(resolve('npm')?.toLowerCase()).toBe('c:\\program files\\nodejs\\npm.cmd'); + expect(resolve('node')?.toLowerCase()).toBe('c:\\program files\\nodejs\\node.exe'); + expect(resolve('where')?.toLowerCase()).toBe('c:\\windows\\system32\\where.exe'); + }); + + it('returns null when the command only exists in the current directory', () => { + expect(resolve('npm', { Path: 'C:\\Windows\\System32' })).toBeNull(); + }); + + it('skips empty, "." and relative PATH entries', () => { + const files2 = new Set(['.\\npm.cmd', 'npm.cmd', 'bin\\npm.cmd']); + expect( + resolveExecutable('npm', { + env: { PATH: ';.;bin;', PATHEXT: '.CMD' }, + platform: 'win32', + isExecutableFile: p => files2.has(p), + }) + ).toBeNull(); + }); + + it('honors PATHEXT order and explicit extensions', () => { + expect(resolve('npm.cmd')).toBe('C:\\Program Files\\nodejs\\npm.cmd'); + expect(resolve('node.exe')).toBe('C:\\Program Files\\nodejs\\node.exe'); + }); + + it('strips quotes around PATH entries', () => { + expect(resolve('npm', { Path: '"C:\\Program Files\\nodejs"', PATHEXT: '.cmd' })).toBe( + 'C:\\Program Files\\nodejs\\npm.cmd' + ); + }); + + it('uses the lexicographically first of duplicate-case PATH keys, like child_process', () => { + // 'PATH' sorts before 'Path', so PATH wins regardless of insertion order. + expect(resolve('npm', { Path: 'C:\\Windows\\System32', PATH: 'C:\\Program Files\\nodejs' })).not.toBeNull(); + expect(resolve('npm', { Path: 'C:\\Program Files\\nodejs', PATH: 'C:\\Windows\\System32' })).toBeNull(); + }); + + it('tries a name with an unknown extension as-is before appending PATHEXT', () => { + const files3 = new Set(['c:\\tools\\run.js']); + expect( + resolveExecutable('run.js', { + env: { Path: 'C:\\tools', PATHEXT: ' .EXE ; .CMD ' }, + platform: 'win32', + isExecutableFile: p => files3.has(p.toLowerCase()), + }) + ).toBe('C:\\tools\\run.js'); + }); + + it('returns commands with a path separator unchanged', () => { + expect(resolve('.\\tool.exe')).toBe('.\\tool.exe'); + expect(resolve('C:\\tools\\x.exe')).toBe('C:\\tools\\x.exe'); + }); +}); + +describe('resolveExecutable (POSIX)', () => { + let dir: string; + let cwdDir: string; + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'agentcore-bin-')); + cwdDir = mkdtempSync(join(tmpdir(), 'agentcore-cwd-')); + for (const d of [dir, cwdDir]) { + writeFileSync(join(d, 'mytool'), '#!/bin/sh\necho hi\n'); + chmodSync(join(d, 'mytool'), 0o755); + } + writeFileSync(join(dir, 'notexec'), 'x'); + }); + + afterEach(() => { + rmSync(dir, { recursive: true, force: true }); + rmSync(cwdDir, { recursive: true, force: true }); + }); + + it('finds executables in absolute PATH entries', () => { + expect(resolveExecutable('mytool', { env: { PATH: dir }, platform: 'linux' })).toBe(join(dir, 'mytool')); + }); + + it('ignores relative PATH entries even when the cwd contains the command', () => { + const original = process.cwd(); + process.chdir(cwdDir); + try { + expect(resolveExecutable('mytool', { env: { PATH: '.::' }, platform: 'linux' })).toBeNull(); + } finally { + process.chdir(original); + } + }); + + it('skips files without the execute bit', () => { + expect(resolveExecutable('notexec', { env: { PATH: dir }, platform: 'linux' })).toBeNull(); + }); +}); + +describe('spawn / spawnSync on Windows when the command is not on PATH', () => { + it('spawn emits ENOENT then close, like child_process.spawn', async () => { + await withPlatformAsync('win32', async () => { + const child = spawn('npm', ['--version'], { env: { Path: '' }, stdio: 'pipe' }); + expect(child.stdin).not.toBeNull(); + const events: string[] = []; + let error: NodeJS.ErrnoException | undefined; + child.on('error', err => { + events.push('error'); + error = err; + }); + await new Promise(resolve => + child.on('close', code => { + events.push(`close:${code}`); + resolve(); + }) + ); + expect(error?.code).toBe('ENOENT'); + expect(error?.message).toBe('spawn npm ENOENT'); + expect(events).toEqual(['error', 'close:-4058']); + }); + }); + + it('the not-found child is safe to kill/ref/unref', async () => { + await withPlatformAsync('win32', async () => { + const child = spawn('npm', [], { env: { Path: '' } }); + child.on('error', () => undefined); + expect(child.kill()).toBe(false); + child.ref(); + child.unref(); + await new Promise(resolve => child.on('close', resolve)); + }); + }); + + it('spawnSync returns an ENOENT result, like child_process.spawnSync', () => { + withPlatform('win32', () => { + const result = spawnSync('npm', ['--version'], { env: { Path: '' }, encoding: 'utf-8' }); + expect(result.status).toBeNull(); + expect(result.stdout).toBe(''); + expect((result.error as NodeJS.ErrnoException).code).toBe('ENOENT'); + }); + }); + + it('checkSubprocess reports a cwd-only command as unavailable', async () => { + await withPlatformAsync('win32', async () => { + expect(await checkSubprocess('npm', [], { env: { Path: '' } })).toBe(false); + }); + }); +}); + +describe('disableCwdExecutableSearch', () => { + afterEach(() => { + delete process.env[NO_CWD_EXE_SEARCH_ENV]; + }); + + it('sets NoDefaultCurrentDirectoryInExePath on Windows', () => { + withPlatform('win32', () => disableCwdExecutableSearch()); + expect(process.env[NO_CWD_EXE_SEARCH_ENV]).toBe('1'); + }); + + it('is a no-op elsewhere', () => { + withPlatform('linux', () => disableCwdExecutableSearch()); + expect(process.env[NO_CWD_EXE_SEARCH_ENV]).toBeUndefined(); + }); +}); + +describe('shell option', () => { + it('rejects shell execution for asynchronous and synchronous wrappers', () => { + expect(() => Reflect.apply(spawn, undefined, ['node', [], { shell: true }])).toThrow( + 'Shell execution is not supported' + ); + expect(() => Reflect.apply(spawnSync, undefined, ['node', [], { shell: true }])).toThrow( + 'Shell execution is not supported' + ); + }); +}); diff --git a/src/lib/utils/index.ts b/src/lib/utils/index.ts index 1904935b48..8e5db79bb4 100644 --- a/src/lib/utils/index.ts +++ b/src/lib/utils/index.ts @@ -6,6 +6,8 @@ export { runSubprocess, checkSubprocess, runSubprocessCapture, + resolveExecutable, + disableCwdExecutableSearch, type SubprocessOptions, type SubprocessResult, } from './subprocess'; diff --git a/src/lib/utils/subprocess.ts b/src/lib/utils/subprocess.ts index 1c41d3f3c4..dc5b15d048 100644 --- a/src/lib/utils/subprocess.ts +++ b/src/lib/utils/subprocess.ts @@ -1,5 +1,22 @@ -import type { StdioOptions } from 'child_process'; +import { + ChildProcess, + type SpawnOptions, + type SpawnSyncOptions, + type SpawnSyncOptionsWithBufferEncoding, + type SpawnSyncOptionsWithStringEncoding, + type SpawnSyncReturns, + type StdioOptions, +} from 'child_process'; import crossSpawn from 'cross-spawn'; +import { accessSync, constants as fsConstants, statSync } from 'fs'; +import path from 'path'; +import { PassThrough } from 'stream'; + +type WithoutShell = Omit & { shell?: false }; +type SafeSpawnOptions = WithoutShell; +type SafeSpawnSyncOptions = WithoutShell; +type SafeSpawnSyncOptionsWithBufferEncoding = WithoutShell; +type SafeSpawnSyncOptionsWithStringEncoding = WithoutShell; /** * Subprocess utilities for AgentCore. @@ -11,9 +28,14 @@ import crossSpawn from 'cross-spawn'; * and are ONLY safe in CDK bundling contexts (which run in a subprocess). They are * intentionally NOT exported from the public API to prevent accidental UI freezes. * - * Nothing spawns through a shell. cross-spawn resolves Windows .cmd/.bat wrappers and - * escapes arguments so a metacharacter in an argument (an `&` in a path, say) reaches - * the child intact instead of being interpreted by cmd.exe. + * Callers pass argument arrays and cannot request shell execution. On Windows, cross-spawn + * invokes cmd.exe only when required for .cmd/.bat wrappers and escapes the command and each + * argument before doing so. + * + * All spawning in the CLI must go through {@link spawn} / {@link spawnSync} (or the helpers + * built on them). Windows searches the current directory before PATH when resolving a bare + * command name, as do cross-spawn's resolver and cmd.exe. These wrappers instead resolve + * commands against absolute PATH entries and spawn the resulting absolute path. */ export interface SubprocessOptions { @@ -22,9 +44,203 @@ export interface SubprocessOptions { stdio?: StdioOptions; } +/** Tells cmd.exe (and CreateProcess-based lookups that honor it) not to search the current directory. */ +export const NO_CWD_EXE_SEARCH_ENV = 'NoDefaultCurrentDirectoryInExePath'; + +const DEFAULT_WINDOWS_PATHEXT = '.COM;.EXE;.BAT;.CMD'; +const WINDOWS_ENOENT_ERRNO = -4058; +const POSIX_ENOENT_ERRNO = -2; + +interface ResolveExecutableOptions { + env?: NodeJS.ProcessEnv; + platform?: NodeJS.Platform; + /** Overridable for tests; defaults to a filesystem check. */ + isExecutableFile?: (filePath: string, platform: NodeJS.Platform) => boolean; +} + +function defaultIsExecutableFile(filePath: string, platform: NodeJS.Platform): boolean { + try { + if (!statSync(filePath).isFile()) return false; + if (platform !== 'win32') accessSync(filePath, fsConstants.X_OK); + return true; + } catch { + return false; + } +} + +/** Environment variable lookup that is case-insensitive on Windows (`Path` vs `PATH`). */ +function getEnvValue(env: NodeJS.ProcessEnv, name: string, platform: NodeJS.Platform): string | undefined { + if (platform !== 'win32') return env[name]; + // With duplicate-case keys, child_process uses the lexicographically first one; match it. + const key = Object.keys(env) + .filter(k => k.toUpperCase() === name.toUpperCase()) + .sort()[0]; + return key === undefined ? undefined : env[key]; +} + +/** + * Resolve a command name to an absolute executable path using only absolute PATH entries. + * + * Unlike Windows' native lookup (and the `which` package used by cross-spawn), this never + * searches the current working directory, and it skips empty, `.` and other relative PATH + * entries. Commands that already contain a path separator are returned unchanged. + * + * @returns The absolute path, or `null` if the command is not found on PATH. + */ +export function resolveExecutable(command: string, options: ResolveExecutableOptions = {}): string | null { + const platform = options.platform ?? process.platform; + const env = options.env ?? process.env; + const isExecutableFile = options.isExecutableFile ?? defaultIsExecutableFile; + const pathImpl = platform === 'win32' ? path.win32 : path.posix; + + if (command.length === 0) return null; + if (command.includes('/') || (platform === 'win32' && command.includes('\\'))) return command; + + const dirs = (getEnvValue(env, 'PATH', platform) ?? '') + .split(platform === 'win32' ? ';' : ':') + .map(dir => (platform === 'win32' ? dir.replace(/^"(.*)"$/, '$1') : dir)) + .filter(dir => dir.length > 0 && pathImpl.isAbsolute(dir)); + + let candidates = [command]; + if (platform === 'win32') { + const exts = (getEnvValue(env, 'PATHEXT', platform) ?? DEFAULT_WINDOWS_PATHEXT) + .split(';') + .map(ext => ext.trim()) + .filter(ext => ext.length > 0); + const hasKnownExt = exts.some(ext => command.toLowerCase().endsWith(ext.toLowerCase())); + // Like Windows, a name that already has an extension is tried as-is first. + candidates = hasKnownExt + ? [command] + : [...(pathImpl.extname(command) ? [command] : []), ...exts.map(ext => command + ext)]; + } + + for (const dir of dirs) { + for (const candidate of candidates) { + const fullPath = pathImpl.join(dir, candidate); + if (isExecutableFile(fullPath, platform)) return fullPath; + } + } + return null; +} + +/** Child environment with current-directory executable search disabled on Windows. */ +function withSafeEnv(env: NodeJS.ProcessEnv | undefined): NodeJS.ProcessEnv | undefined { + if (process.platform !== 'win32') return env; + return { ...(env ?? process.env), [NO_CWD_EXE_SEARCH_ENV]: '1' }; +} + +function assertShellDisabled(options: { shell?: unknown }): void { + if (options.shell !== undefined && options.shell !== false) { + throw new TypeError('Shell execution is not supported by the subprocess wrapper.'); + } +} + +/** + * Disable current-directory executable search for this process and everything it spawns + * (including third-party libraries such as the CDK toolkit, which spawn through a shell). + * Call once at CLI startup. No-op off Windows. + */ +export function disableCwdExecutableSearch(): void { + if (process.platform === 'win32') { + process.env[NO_CWD_EXE_SEARCH_ENV] = '1'; + } +} + +function makeEnoentError(syscall: 'spawn' | 'spawnSync', command: string, args: readonly string[]) { + return Object.assign(new Error(`${syscall} ${command} ENOENT`), { + code: 'ENOENT', + errno: process.platform === 'win32' ? WINDOWS_ENOENT_ERRNO : POSIX_ENOENT_ERRNO, + syscall: `${syscall} ${command}`, + path: command, + spawnargs: [...args], + }); +} + +/** A ChildProcess that fails the way Node's spawn does for a missing command: 'error' then 'close'. */ +function notFoundChildProcess(command: string, args: readonly string[]): ChildProcess { + const child = new ChildProcess(); + // Never spawned: drop the native handle so kill()/ref()/unref() are no-ops instead of + // acting on an uninitialized process handle. + (child as unknown as { _handle: unknown })._handle = null; + const stdin = new PassThrough(); + const stdout = new PassThrough(); + const stderr = new PassThrough(); + Object.defineProperties(child, { + stdin: { value: stdin }, + stdout: { value: stdout }, + stderr: { value: stderr }, + stdio: { value: [stdin, stdout, stderr] }, + }); + const error = makeEnoentError('spawn', command, args); + process.nextTick(() => { + child.emit('error', error); + stdout.end(); + stderr.end(); + child.emit('close', error.errno, null); + }); + return child; +} + +/** + * Drop-in replacement for `child_process.spawn` that never runs an executable from the + * current directory. See the module comment. + */ +export function spawn(command: string, args: readonly string[] = [], options: SafeSpawnOptions = {}): ChildProcess { + assertShellDisabled(options); + const resolved = resolveExecutable(command, { env: options.env }); + if (resolved === null && process.platform === 'win32') { + return notFoundChildProcess(command, args); + } + return crossSpawn(resolved ?? command, args, { + ...options, + shell: false, + env: withSafeEnv(options.env), + }); +} + +/** + * Drop-in replacement for `child_process.spawnSync` that never runs an executable from the + * current directory. See the module comment. + */ +export function spawnSync( + command: string, + args: readonly string[], + options: SafeSpawnSyncOptionsWithStringEncoding +): SpawnSyncReturns; +export function spawnSync( + command: string, + args?: readonly string[], + options?: SafeSpawnSyncOptionsWithBufferEncoding | SafeSpawnSyncOptions +): SpawnSyncReturns; +export function spawnSync( + command: string, + args: readonly string[] = [], + options: SafeSpawnSyncOptions = {} +): SpawnSyncReturns { + assertShellDisabled(options); + const resolved = resolveExecutable(command, { env: options.env }); + if (resolved === null && process.platform === 'win32') { + const empty = options.encoding && options.encoding !== 'buffer' ? '' : Buffer.alloc(0); + return { + pid: 0, + output: [], + stdout: empty, + stderr: empty, + status: null, + signal: null, + error: makeEnoentError('spawnSync', command, args), + }; + } + return crossSpawn.sync(resolved ?? command, args, { + ...options, + shell: false, + env: withSafeEnv(options.env), + }) as SpawnSyncReturns; +} + export async function runSubprocess(command: string, args: string[], options: SubprocessOptions = {}): Promise { return new Promise((resolve, reject) => { - const child = crossSpawn(command, args, { ...options, stdio: options.stdio ?? 'inherit' }); + const child = spawn(command, args, { ...options, stdio: options.stdio ?? 'inherit' }); child.on('error', reject); child.on('close', (code, signal) => { @@ -44,7 +260,7 @@ export async function checkSubprocess( options: SubprocessOptions = {} ): Promise { return new Promise(resolve => { - const child = crossSpawn(command, args, { ...options, stdio: options.stdio ?? 'ignore' }); + const child = spawn(command, args, { ...options, stdio: options.stdio ?? 'ignore' }); child.on('error', () => resolve(false)); child.on('close', code => resolve(code === 0)); @@ -64,7 +280,7 @@ export async function runSubprocessCapture( options: SubprocessOptions = {} ): Promise { return new Promise(resolve => { - const child = crossSpawn(command, args, { ...options, stdio: 'pipe' }); + const child = spawn(command, args, { ...options, stdio: 'pipe' }); let stdout = ''; let stderr = ''; @@ -92,7 +308,7 @@ export function runSubprocessCaptureSync( args: string[], options: SubprocessOptions = {} ): SubprocessResult { - const result = crossSpawn.sync(command, args, { ...options, stdio: 'pipe', encoding: 'utf-8' }); + const result = spawnSync(command, args, { ...options, stdio: 'pipe', encoding: 'utf-8' }); return { stdout: result.stdout ?? '', @@ -104,7 +320,7 @@ export function runSubprocessCaptureSync( export function checkSubprocessSync(command: string, args: string[], options: SubprocessOptions = {}): boolean { try { - const result = crossSpawn.sync(command, args, { ...options, stdio: options.stdio ?? 'ignore' }); + const result = spawnSync(command, args, { ...options, stdio: options.stdio ?? 'ignore' }); return result.status === 0; } catch { return false;