From 9b1d217b4760c6d563185787024ff68a355c128d Mon Sep 17 00:00:00 2001 From: gitikavj Date: Mon, 26 Oct 2026 16:02:11 +0000 Subject: [PATCH 1/2] fix(cli): resolve Windows commands from explicit PATH entries Resolve bare commands through absolute PATH entries before launching them on Windows. Route process creation through the shared subprocess helpers and pass the Windows lookup setting to child processes. --- eslint.config.mjs | 18 ++ src/cli/cdk/toolkit-lib/wrapper.ts | 11 +- src/cli/cli.ts | 5 + src/cli/commands/dev/command.tsx | 2 +- .../export/__tests__/container-export.test.ts | 4 +- src/cli/commands/export/harness-action.ts | 6 +- .../update/__tests__/update-action.test.ts | 22 +- src/cli/commands/update/action.ts | 5 +- .../__tests__/detect.test.ts | 31 ++- src/cli/external-requirements/checks.ts | 6 +- src/cli/external-requirements/detect.ts | 5 +- .../dev/__tests__/codezip-dev-server.test.ts | 12 +- .../__tests__/container-dev-server.test.ts | 2 +- .../dev/__tests__/dev-server.test.ts | 3 +- .../dev/__tests__/utils-open-browser.test.ts | 2 +- src/cli/operations/dev/codezip-dev-server.ts | 13 +- .../operations/dev/container-dev-server.ts | 3 +- src/cli/operations/dev/dev-server.ts | 5 +- src/cli/operations/dev/utils.ts | 2 +- src/cli/tui/hooks/useDevServer.ts | 2 +- .../fetch-access/useFetchAccessFlow.ts | 6 +- src/lib/packaging/__tests__/container.test.ts | 5 +- src/lib/packaging/container.ts | 5 +- src/lib/packaging/helpers.ts | 6 +- .../utils/__tests__/subprocess-win32.test.ts | 64 ++++++ src/lib/utils/__tests__/subprocess.test.ts | 209 +++++++++++++++++- src/lib/utils/index.ts | 2 + src/lib/utils/subprocess.ts | 209 +++++++++++++++++- 28 files changed, 570 insertions(+), 95 deletions(-) create mode 100644 src/lib/utils/__tests__/subprocess-win32.test.ts 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.test.ts b/src/lib/utils/__tests__/subprocess.test.ts index 0326fe7f8b..e974840b32 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,182 @@ 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(); + }); +}); 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..7c73bf0f02 100644 --- a/src/lib/utils/subprocess.ts +++ b/src/lib/utils/subprocess.ts @@ -1,5 +1,16 @@ -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'; /** * Subprocess utilities for AgentCore. @@ -14,6 +25,11 @@ import crossSpawn from 'cross-spawn'; * 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. + * + * 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 +38,190 @@ 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' }; +} + +/** + * 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: SpawnOptions = {}): ChildProcess { + const resolved = resolveExecutable(command, { env: options.env }); + if (resolved === null && process.platform === 'win32') { + return notFoundChildProcess(command, args); + } + return crossSpawn(resolved ?? command, args, { ...options, 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: SpawnSyncOptionsWithStringEncoding +): SpawnSyncReturns; +export function spawnSync( + command: string, + args?: readonly string[], + options?: SpawnSyncOptionsWithBufferEncoding | SpawnSyncOptions +): SpawnSyncReturns; +export function spawnSync( + command: string, + args: readonly string[] = [], + options: SpawnSyncOptions = {} +): SpawnSyncReturns { + 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, + 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 +241,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 +261,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 +289,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 +301,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; From 5602b24a297cf25564ad2d87192a54a0fe8d29ea Mon Sep 17 00:00:00 2001 From: gitikavj Date: Mon, 26 Oct 2026 16:32:14 +0000 Subject: [PATCH 2/2] fix(cli): enforce argument-array subprocess launches Keep caller-requested shell execution disabled and add focused Windows coverage for commands and arguments containing metacharacters. --- .github/workflows/build-and-test.yml | 17 +++++ .../__tests__/subprocess-windows-real.test.ts | 72 +++++++++++++++++++ src/lib/utils/__tests__/subprocess.test.ts | 11 +++ src/lib/utils/subprocess.ts | 35 ++++++--- 4 files changed, 127 insertions(+), 8 deletions(-) create mode 100644 src/lib/utils/__tests__/subprocess-windows-real.test.ts 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/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 e974840b32..f0b395d9c0 100644 --- a/src/lib/utils/__tests__/subprocess.test.ts +++ b/src/lib/utils/__tests__/subprocess.test.ts @@ -304,3 +304,14 @@ describe('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/subprocess.ts b/src/lib/utils/subprocess.ts index 7c73bf0f02..dc5b15d048 100644 --- a/src/lib/utils/subprocess.ts +++ b/src/lib/utils/subprocess.ts @@ -12,6 +12,12 @@ 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. * @@ -22,9 +28,9 @@ import { PassThrough } from 'stream'; * 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 @@ -123,6 +129,12 @@ function withSafeEnv(env: NodeJS.ProcessEnv | undefined): NodeJS.ProcessEnv | un 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). @@ -173,12 +185,17 @@ function notFoundChildProcess(command: string, args: readonly string[]): ChildPr * 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: SpawnOptions = {}): ChildProcess { +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, env: withSafeEnv(options.env) }); + return crossSpawn(resolved ?? command, args, { + ...options, + shell: false, + env: withSafeEnv(options.env), + }); } /** @@ -188,18 +205,19 @@ export function spawn(command: string, args: readonly string[] = [], options: Sp export function spawnSync( command: string, args: readonly string[], - options: SpawnSyncOptionsWithStringEncoding + options: SafeSpawnSyncOptionsWithStringEncoding ): SpawnSyncReturns; export function spawnSync( command: string, args?: readonly string[], - options?: SpawnSyncOptionsWithBufferEncoding | SpawnSyncOptions + options?: SafeSpawnSyncOptionsWithBufferEncoding | SafeSpawnSyncOptions ): SpawnSyncReturns; export function spawnSync( command: string, args: readonly string[] = [], - options: SpawnSyncOptions = {} + 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); @@ -215,6 +233,7 @@ export function spawnSync( } return crossSpawn.sync(resolved ?? command, args, { ...options, + shell: false, env: withSafeEnv(options.env), }) as SpawnSyncReturns; }