From 0cfc3b4ed2535cfa3a136d0cb837cd00d9655026 Mon Sep 17 00:00:00 2001 From: weishu Date: Sun, 26 Apr 2026 12:04:53 +0800 Subject: [PATCH] fix(cli): preserve codex config args on Windows --- cli/src/codex/codexLocal.test.ts | 67 ++++++++++++++++++++++-- cli/src/codex/codexLocal.ts | 3 +- cli/src/codex/utils/codexVersion.test.ts | 51 ++++++++++++++---- cli/src/codex/utils/codexVersion.ts | 31 +++++++---- cli/src/utils/spawnWithAbort.test.ts | 20 +++---- cli/src/utils/spawnWithAbort.ts | 4 +- 6 files changed, 137 insertions(+), 39 deletions(-) diff --git a/cli/src/codex/codexLocal.test.ts b/cli/src/codex/codexLocal.test.ts index 3e718c9c..2d251c27 100644 --- a/cli/src/codex/codexLocal.test.ts +++ b/cli/src/codex/codexLocal.test.ts @@ -1,5 +1,20 @@ -import { describe, it, expect } from 'vitest'; -import { filterResumeSubcommand } from './codexLocal'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const { spawnWithTerminalGuardMock } = vi.hoisted(() => ({ + spawnWithTerminalGuardMock: vi.fn(async (_options: unknown) => {}) +})); + +vi.mock('@/utils/spawnWithTerminalGuard', () => ({ + spawnWithTerminalGuard: spawnWithTerminalGuardMock +})); + +vi.mock('@/ui/logger', () => ({ + logger: { + debug: vi.fn() + } +})); + +import { codexLocal, filterResumeSubcommand } from './codexLocal'; describe('filterResumeSubcommand', () => { it('returns empty array unchanged', () => { @@ -24,13 +39,57 @@ describe('filterResumeSubcommand', () => { }); it('does not filter resume when it appears as flag value', () => { - // --name resume should pass through (resume is value, not subcommand) expect(filterResumeSubcommand(['--name', 'resume'])).toEqual(['--name', 'resume']); }); it('does not filter resume in middle of args', () => { - // If resume appears after flags, it's not the subcommand position expect(filterResumeSubcommand(['--model', 'gpt-4', 'resume', '123'])) .toEqual(['--model', 'gpt-4', 'resume', '123']); }); }); + +describe('codexLocal', () => { + beforeEach(() => { + spawnWithTerminalGuardMock.mockClear(); + }); + + it('launches codex without shell so Windows keeps -c config values as argv elements', async () => { + const controller = new AbortController(); + + await codexLocal({ + abort: controller.signal, + sessionId: null, + path: 'C:\\workspace\\project', + onSessionFound: vi.fn(), + mcpServers: { + hapi: { + command: 'C:\\Users\\test\\AppData\\Local\\hapi.exe', + args: ['mcp', '--url', 'http://127.0.0.1:63995/'] + } + }, + sessionHook: { + port: 63996, + token: 'secret-token' + } + }); + + expect(spawnWithTerminalGuardMock).toHaveBeenCalledOnce(); + const spawnOptions = spawnWithTerminalGuardMock.mock.calls[0][0] as { + command: string; + cwd: string; + args: string[]; + shell?: unknown; + }; + expect(spawnOptions).toEqual(expect.objectContaining({ + command: 'codex', + cwd: 'C:\\workspace\\project' + })); + expect(spawnOptions).not.toHaveProperty('shell'); + + const args = spawnOptions.args; + const hookArg = args.find((arg) => arg.startsWith('hooks.SessionStart=')); + expect(hookArg).toBeDefined(); + expect(hookArg).toContain('{ hooks = [{ type = "command", command = "'); + expect(args).toContain("mcp_servers.hapi.args=['mcp','--url','http://127.0.0.1:63995/']"); + }); +}); diff --git a/cli/src/codex/codexLocal.ts b/cli/src/codex/codexLocal.ts index 5f00b567..60e46f0c 100644 --- a/cli/src/codex/codexLocal.ts +++ b/cli/src/codex/codexLocal.ts @@ -95,7 +95,6 @@ export async function codexLocal(opts: { spawnName: 'codex', installHint: 'Codex CLI', includeCause: true, - logExit: true, - shell: process.platform === 'win32' + logExit: true }); } diff --git a/cli/src/codex/utils/codexVersion.test.ts b/cli/src/codex/utils/codexVersion.test.ts index 83354149..4452c6a4 100644 --- a/cli/src/codex/utils/codexVersion.test.ts +++ b/cli/src/codex/utils/codexVersion.test.ts @@ -1,11 +1,13 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' -const { execFileSyncMock } = vi.hoisted(() => ({ - execFileSyncMock: vi.fn() +const { spawnSyncMock } = vi.hoisted(() => ({ + spawnSyncMock: vi.fn() })) -vi.mock('node:child_process', () => ({ - execFileSync: execFileSyncMock +vi.mock('cross-spawn', () => ({ + default: { + sync: spawnSyncMock + } })) import { @@ -17,7 +19,7 @@ import { describe('codexVersion', () => { beforeEach(() => { - execFileSyncMock.mockReset() + spawnSyncMock.mockReset() }) describe('parseCodexVersion', () => { @@ -47,16 +49,24 @@ describe('codexVersion', () => { describe('assertCodexLocalSupported', () => { it('passes when codex is new enough', () => { - execFileSyncMock.mockReturnValueOnce('codex-cli 0.124.0\n') + spawnSyncMock.mockReturnValueOnce({ + status: 0, + stdout: 'codex-cli 0.124.0\n', + stderr: '' + }) expect(() => assertCodexLocalSupported()).not.toThrow() - expect(execFileSyncMock).toHaveBeenCalledWith('codex', ['--version'], expect.objectContaining({ + expect(spawnSyncMock).toHaveBeenCalledWith('codex', ['--version'], expect.objectContaining({ encoding: 'utf8' })) }) it('fails when codex is too old', () => { - execFileSyncMock.mockReturnValueOnce('codex-cli 0.123.9\n') + spawnSyncMock.mockReturnValueOnce({ + status: 0, + stdout: 'codex-cli 0.123.9\n', + stderr: '' + }) expect(() => assertCodexLocalSupported()).toThrow( 'Codex CLI 0.124.0+ is required for hapi codex local mode because HAPI depends on stable hooks. Detected: 0.123.9. Please upgrade Codex and retry.' @@ -64,7 +74,11 @@ describe('codexVersion', () => { }) it('fails when the version output cannot be parsed', () => { - execFileSyncMock.mockReturnValueOnce('codex-cli version unknown\n') + spawnSyncMock.mockReturnValueOnce({ + status: 0, + stdout: 'codex-cli version unknown\n', + stderr: '' + }) expect(() => assertCodexLocalSupported()).toThrow( 'Could not determine Codex CLI version. Codex CLI 0.124.0+ is required for hapi codex local mode because HAPI depends on stable hooks. Please upgrade Codex and retry.' @@ -74,13 +88,28 @@ describe('codexVersion', () => { it('fails when codex is not available on PATH', () => { const error = new Error('spawnSync codex ENOENT') as NodeJS.ErrnoException error.code = 'ENOENT' - execFileSyncMock.mockImplementationOnce(() => { - throw error + spawnSyncMock.mockReturnValueOnce({ + status: null, + stdout: '', + stderr: '', + error }) expect(() => assertCodexLocalSupported()).toThrow( 'Codex CLI 0.124.0+ is required for hapi codex local mode because HAPI depends on stable hooks. Codex was not found on PATH. Please install or upgrade Codex and retry.' ) }) + + it('fails when codex version exits unsuccessfully', () => { + spawnSyncMock.mockReturnValueOnce({ + status: 1, + stdout: '', + stderr: 'codex failed' + }) + + expect(() => assertCodexLocalSupported()).toThrow( + 'Could not determine Codex CLI version. codex failed Codex CLI 0.124.0+ is required for hapi codex local mode because HAPI depends on stable hooks. Please upgrade Codex and retry.' + ) + }) }) }) diff --git a/cli/src/codex/utils/codexVersion.ts b/cli/src/codex/utils/codexVersion.ts index f91adbcc..02fc1e0c 100644 --- a/cli/src/codex/utils/codexVersion.ts +++ b/cli/src/codex/utils/codexVersion.ts @@ -1,4 +1,4 @@ -import { execFileSync } from 'node:child_process' +import spawn from 'cross-spawn' import { withBunRuntimeEnv } from '@/utils/bunRuntime' export const MIN_CODEX_HOOKS_VERSION = '0.124.0' @@ -54,30 +54,39 @@ export function isCodexVersionAtLeast(version: string, minimum: string): boolean export function assertCodexLocalSupported(): void { let output: string - try { - output = execFileSync('codex', ['--version'], { - encoding: 'utf8', - env: withBunRuntimeEnv(), - shell: process.platform === 'win32' - }).trim() - } catch (error) { - const maybeError = error as NodeJS.ErrnoException + const result = spawn.sync('codex', ['--version'], { + encoding: 'utf8', + env: withBunRuntimeEnv() + }) + + if (result.error) { + const maybeError = result.error as NodeJS.ErrnoException const message = maybeError?.message ? ` ${maybeError.message}` : '' if (maybeError?.code === 'ENOENT') { throw new Error( `${getLocalModeRequirementMessage()} Codex was not found on PATH. Please install or upgrade Codex and retry.`, - { cause: error } + { cause: result.error } ) } throw new Error( `Could not determine Codex CLI version.${message} ` + `${getLocalModeRequirementMessage()} Please upgrade Codex and retry.`, - { cause: error } + { cause: result.error } ) } + if (result.status !== 0) { + const detail = result.stderr ? ` ${result.stderr.trim()}` : '' + throw new Error( + `Could not determine Codex CLI version.${detail} ` + + `${getLocalModeRequirementMessage()} Please upgrade Codex and retry.` + ) + } + + output = result.stdout.trim() + const version = parseCodexVersion(output) if (!version) { throw new Error( diff --git a/cli/src/utils/spawnWithAbort.test.ts b/cli/src/utils/spawnWithAbort.test.ts index 4b4b16ba..d8b4603d 100644 --- a/cli/src/utils/spawnWithAbort.test.ts +++ b/cli/src/utils/spawnWithAbort.test.ts @@ -4,15 +4,17 @@ import { EventEmitter } from 'node:events'; // Create a fake child process emitter for each test let childEmitter: EventEmitter & { exitCode: number | null; killed: boolean; pid: number }; -vi.mock('node:child_process', () => ({ - spawn: vi.fn(() => { - childEmitter = Object.assign(new EventEmitter(), { - exitCode: null, - killed: false, - pid: 12345, - }); - return childEmitter; - }), +const spawnMock = vi.hoisted(() => vi.fn(() => { + childEmitter = Object.assign(new EventEmitter(), { + exitCode: null, + killed: false, + pid: 12345, + }); + return childEmitter; +})); + +vi.mock('cross-spawn', () => ({ + default: spawnMock })); vi.mock('@/ui/logger', () => ({ diff --git a/cli/src/utils/spawnWithAbort.ts b/cli/src/utils/spawnWithAbort.ts index 00b54cc0..ca2f5913 100644 --- a/cli/src/utils/spawnWithAbort.ts +++ b/cli/src/utils/spawnWithAbort.ts @@ -1,4 +1,5 @@ -import { spawn, type SpawnOptions, type StdioOptions } from 'node:child_process'; +import type { SpawnOptions, StdioOptions } from 'node:child_process'; +import spawn from 'cross-spawn'; import { logger } from '@/ui/logger'; import { killProcessByChildProcess } from '@/utils/process'; @@ -149,4 +150,3 @@ export async function spawnWithAbort(options: SpawnWithAbortOptions): Promise