diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index b7832ba2d9e..6b1deaa72ec 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -888,6 +888,8 @@ jobs: src/shared/child-process/windows-cmd-shim-resolution.win32.test.ts src/main/agent-hooks/windows-hook-payload-delivery.test.ts src/main/agent-hooks/windows-direct-cmd-hook-command.test.ts + src/main/codex/windows-hook-command.test.ts + src/main/codex/windows-hook-upgrade.test.ts src/main/windows/windows-pty-job.win32.test.ts src/main/windows/windows-msys-job.win32.test.ts src/main/windows/windows-host-job.win32.test.ts diff --git a/config/scripts/pr-code-change-scope.mjs b/config/scripts/pr-code-change-scope.mjs index 3b39bb34275..77c38c546ac 100644 --- a/config/scripts/pr-code-change-scope.mjs +++ b/config/scripts/pr-code-change-scope.mjs @@ -223,6 +223,8 @@ const WINDOWS_PACKAGE_TESTS = [ 'src/shared/child-process/windows-cmd-shim-resolution.win32.test.ts', 'src/main/agent-hooks/windows-hook-payload-delivery.test.ts', 'src/main/agent-hooks/windows-direct-cmd-hook-command.test.ts', + 'src/main/codex/windows-hook-command.test.ts', + 'src/main/codex/windows-hook-upgrade.test.ts', 'src/main/windows/windows-pty-job.win32.test.ts', 'src/main/windows/windows-msys-job.win32.test.ts', 'src/main/windows/windows-host-job.win32.test.ts', diff --git a/src/main/agent-hooks/installer-utils.test.ts b/src/main/agent-hooks/installer-utils.test.ts index e68e3cc3554..40966c0f725 100644 --- a/src/main/agent-hooks/installer-utils.test.ts +++ b/src/main/agent-hooks/installer-utils.test.ts @@ -707,10 +707,7 @@ describe('wrapWindowsHookCommand', () => { describe('wrapWindowsCmdHookCommand', () => { it('returns the bare, directly-spawnable path for a cmd-safe managed script', () => { - // Why: Codex/Antigravity/Devin launch the command as a program (argv[0]), - // not via cmd.exe, so the launcher must be a single spawnable token — a bare - // .cmd path. A cmd-builtin `if …` launcher has argv[0] = `if`, which is - // unspawnable and fails every hook with exit 1 (#8430 regression). + // Direct-spawn consumers need a launchable argv[0], not a cmd builtin such as `if`. const scriptPath = 'C:\\Users\\alice\\.orca\\agent-hooks\\codex-hook.cmd' const command = wrapWindowsCmdHookCommand(scriptPath) expect(command).toBe(scriptPath) @@ -721,12 +718,7 @@ describe('wrapWindowsCmdHookCommand', () => { it.skipIf(process.platform !== 'win32')( 'resolves the launcher to a real executable file, not a shell fragment', () => { - // Regression guard for #8430: Codex/Antigravity/Devin spawn the launcher as - // a program (argv[0]), so it must be an existing, launchable file. The broken - // `if exist … (call …)` form had argv[0] = `if` — a cmd builtin, not a file — - // which is unspawnable and failed every hook. The bare path is the file. - // win32-only: the real temp path is cmd-safe only with backslashes; a POSIX - // tmpDir has `/`, which routes to the encoded fallback by design. + // POSIX temp paths contain `/`, which selects the encoded fallback instead. const scriptPath = join(tmpDir, 'codex-hook.cmd') writeFileSync(scriptPath, '@echo off\r\nexit /b 0\r\n', 'utf-8') const command = wrapWindowsCmdHookCommand(scriptPath) diff --git a/src/main/agent-hooks/installer-utils.ts b/src/main/agent-hooks/installer-utils.ts index a53721d42fe..ac4abee9469 100644 --- a/src/main/agent-hooks/installer-utils.ts +++ b/src/main/agent-hooks/installer-utils.ts @@ -118,6 +118,16 @@ export { } from './windows-powershell-hook-launcher' export function wrapWindowsHookCommand( + scriptPath: string, + env: Record = {}, + options: { fallbackStdout?: string } = {} +): string { + return wrapWindowsPowerShellEncodedCommand( + buildWindowsHookPowerShellCommand(scriptPath, env, options) + ) +} + +export function buildWindowsHookPowerShellCommand( scriptPath: string, env: Record = {}, // Why: POSIX wrap already answers missing-script with stdout; Windows must match so gate events cannot drift (#15462). @@ -135,14 +145,13 @@ export function wrapWindowsHookCommand( // Why the order: answer first (a gate event reads silence as deny), then the shared // env guard, and only then own stdin — outside an Orca pane the caller may abandon the // pipe, and ReadToEnd would strand the launcher there forever (#11549). - const command = `${envPrefix}if (Test-Path -LiteralPath ${quoted} -PathType Leaf) { & ${quoted}; exit $LASTEXITCODE }; ${fallback}${WINDOWS_POWERSHELL_HOOK_ENVIRONMENT_GUARD}; [Console]::In.ReadToEnd() | Out-Null; exit 0` - return wrapWindowsPowerShellEncodedCommand(command) + return `${envPrefix}if (Test-Path -LiteralPath ${quoted} -PathType Leaf) { & ${quoted}; exit $LASTEXITCODE }; ${fallback}${WINDOWS_POWERSHELL_HOOK_ENVIRONMENT_GUARD}; [Console]::In.ReadToEnd() | Out-Null; exit 0` } export const WINDOWS_CMD_SAFE_PATH = /^[A-Za-z0-9_.:\\~-]+$/ export function wrapWindowsCmdHookCommand(scriptPath: string): string { - // Why: Codex/Antigravity/Devin spawn the hook as argv[0], not via cmd.exe, so it must be one spawnable token; a cmd `if exist` launcher isn't (#8430). + // Direct-spawn consumers need one executable token; a cmd `if exist` fragment is not one (#8430). return WINDOWS_CMD_SAFE_PATH.test(scriptPath) ? scriptPath : wrapWindowsHookCommand(scriptPath) } diff --git a/src/main/agent-hooks/managed-hook-command-contract.test.ts b/src/main/agent-hooks/managed-hook-command-contract.test.ts index 72b522e8ff5..a49b45b6df4 100644 --- a/src/main/agent-hooks/managed-hook-command-contract.test.ts +++ b/src/main/agent-hooks/managed-hook-command-contract.test.ts @@ -182,7 +182,12 @@ describe('managed hook command contract', () => { expect(commands.length).toBeGreaterThan(0) for (const command of commands) { expect(command.length).toBeGreaterThan(0) - expect(findBareHookCommandVariables(command), command).toEqual([]) + // Native Windows Codex evaluates PowerShell variables without Grok's dollar-byte scanner. + const scannedCommand = + agent === 'codex' && platform === 'win32' && command.startsWith('if (Test-Path') + ? command.replaceAll('$LASTEXITCODE', '').replaceAll('$env:', '') + : command + expect(findBareHookCommandVariables(scannedCommand), command).toEqual([]) } }) }) diff --git a/src/main/codex/codex-hook-definition.ts b/src/main/codex/codex-hook-definition.ts index 6f03bc0aabe..641a5fc1b08 100644 --- a/src/main/codex/codex-hook-definition.ts +++ b/src/main/codex/codex-hook-definition.ts @@ -1,8 +1,9 @@ import { join } from 'node:path' import { getSharedManagedScriptPath, + buildWindowsHookPowerShellCommand, wrapPosixHookCommand, - wrapWindowsCmdHookCommand, + WINDOWS_CMD_SAFE_PATH, writeHooksJson, type HookDefinition } from '../agent-hooks/installer-utils' @@ -70,9 +71,13 @@ export function getManagedScriptPath(): string { } export function getManagedCommand(scriptPath: string): string { - return process.platform === 'win32' - ? wrapWindowsCmdHookCommand(scriptPath) - : wrapPosixHookCommand(scriptPath) + if (process.platform !== 'win32') { + return wrapPosixHookCommand(scriptPath) + } + // Codex's default native Windows hook host is PowerShell; reuse it to avoid a second interpreter. + return WINDOWS_CMD_SAFE_PATH.test(scriptPath) + ? scriptPath + : buildWindowsHookPowerShellCommand(scriptPath) } export type CodexManagedHookInstallMaterial = { diff --git a/src/main/codex/hook-service-managed-install.test.ts b/src/main/codex/hook-service-managed-install.test.ts index 3e242dc2cf4..caa67752a7f 100644 --- a/src/main/codex/hook-service-managed-install.test.ts +++ b/src/main/codex/hook-service-managed-install.test.ts @@ -28,11 +28,9 @@ vi.mock('os', async (importOriginal) => { }) import { CodexHookService } from './hook-service' +import { buildWindowsHookPowerShellCommand } from '../agent-hooks/installer-utils' import { runExclusivelyForCodexTrustConfig } from './codex-trust-config-mutation-queue' -const WINDOWS_POWERSHELL_LAUNCHER = - /^[A-Za-z]:\/[^"]*\/System32\/WindowsPowerShell\/v1\.0\/powershell\.exe -NoProfile -EncodedCommand \S+$/ - const homes = setupCodexHookHomes(homedirMock, getPathMock) function localManagedCodexEvents(): string[] { @@ -184,10 +182,7 @@ describe('CodexHookService', () => { expect(Object.keys(hooksConfig)).toEqual(['hooks']) }) - // Why: #6078 — a Windows user profile path like `C:\Users\Jane Doe` used to - // be written verbatim as the hook command, so Codex split it at the space and - // the hook exited with code 1. Keep spaced paths on the encoded launcher so - // `cmd.exe /C` never sees the raw script path. + // #6078: the existing PowerShell host must still quote spaced profile paths. it.skipIf(process.platform !== 'win32')( 'wraps the managed hook command when the profile path contains a space (#6078)', async () => { @@ -208,7 +203,11 @@ describe('CodexHookService', () => { for (const eventName of localManagedCodexEvents()) { const command = hooksConfig.hooks[eventName]?.[0]?.hooks?.[0]?.command - expect(command).toMatch(WINDOWS_POWERSHELL_LAUNCHER) + expect(command).toBe( + buildWindowsHookPowerShellCommand( + join(homedir(), '.orca', 'agent-hooks', 'codex-hook.cmd') + ) + ) } } finally { rmSync(spaceHome, { recursive: true, force: true }) @@ -216,10 +215,9 @@ describe('CodexHookService', () => { } ) - // Why: cmd.exe expands `%` and treats `^` as an escape even inside otherwise - // plausible paths. Keep those rare cases on the encoded launcher from #6078. + // Preserve literal-path quoting when constructing commands for shell metacharacters. it.skipIf(process.platform !== 'win32')( - 'keeps the encoded launcher when the profile path contains cmd metacharacters', + 'quotes the script path when the profile contains cmd metacharacters', async () => { const metacharHome = join(tmpdir(), 'orca %ORCA_TEST% ^ home') mkdirSync(metacharHome, { recursive: true }) @@ -238,7 +236,11 @@ describe('CodexHookService', () => { for (const eventName of localManagedCodexEvents()) { const command = hooksConfig.hooks[eventName]?.[0]?.hooks?.[0]?.command - expect(command).toMatch(WINDOWS_POWERSHELL_LAUNCHER) + expect(command).toBe( + buildWindowsHookPowerShellCommand( + join(homedir(), '.orca', 'agent-hooks', 'codex-hook.cmd') + ) + ) } } finally { rmSync(metacharHome, { recursive: true, force: true }) @@ -268,7 +270,11 @@ describe('CodexHookService', () => { expect(command).not.toMatch(/powershell/i) expect(command).toMatch(/\\agent-hooks\\codex-hook\.cmd$/) } else { - expect(command).toMatch(WINDOWS_POWERSHELL_LAUNCHER) + expect(command).toBe( + buildWindowsHookPowerShellCommand( + join(homedir(), '.orca', 'agent-hooks', 'codex-hook.cmd') + ) + ) } } ) diff --git a/src/main/codex/windows-hook-command.test.ts b/src/main/codex/windows-hook-command.test.ts new file mode 100644 index 00000000000..d1fc2c8b098 --- /dev/null +++ b/src/main/codex/windows-hook-command.test.ts @@ -0,0 +1,146 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { existsSync, mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { delimiter, join } from 'node:path' +import { createServer } from 'node:http' +import { runProcess } from '../../shared/child-process/run-process' +import { removeTree } from '../../shared/windows-transient-lock-removal' +import { getManagedCommand, CODEX_EVENTS } from './codex-hook-definition' +import { getManagedScript } from './codex-hook-script' +import { + createManagedCommandMatcher, + wrapWindowsCmdHookCommand +} from '../agent-hooks/installer-utils' + +vi.mock('electron', () => ({ app: { getPath: () => process.cwd() } })) +afterEach(() => vi.restoreAllMocks()) + +describe('Codex Windows hook command', () => { + it.each(['测试用户', '홍길동', '日本語', 'rené', '测试 用户', "测试 O'Brien"])( + 'uses the existing PowerShell host for %s without a second interpreter', + (profile) => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') + const path = `C:\\Users\\${profile}\\.orca\\agent-hooks\\codex-hook.cmd` + const command = getManagedCommand(path) + expect(command).not.toMatch(/powershell\.exe|EncodedCommand|Set-ExecutionPolicy/) + expect(command).toContain(`-LiteralPath '${path.replaceAll("'", "''")}' -PathType Leaf`) + expect(command).toContain(`[Console]::In.ReadToEnd()`) + expect(createManagedCommandMatcher('codex-hook.cmd')(command)).toBe(true) + expect(wrapWindowsCmdHookCommand(path)).toContain('-EncodedCommand') + } + ) + + it('preserves the existing ASCII command and POSIX launcher', () => { + vi.spyOn(process, 'platform', 'get').mockReturnValue('win32') + const path = 'C:\\Users\\alice\\.orca\\agent-hooks\\codex-hook.cmd' + expect(getManagedCommand(path)).toBe(path) + vi.spyOn(process, 'platform', 'get').mockReturnValue('linux') + expect(getManagedCommand('/home/测试/.orca/agent-hooks/codex-hook.sh')).toContain( + "[ -x '/home/测试/.orca/agent-hooks/codex-hook.sh' ]" + ) + }) +}) + +const windowsPowerShell = join( + process.env.SystemRoot ?? 'C:\\Windows', + 'System32', + 'WindowsPowerShell', + 'v1.0', + 'powershell.exe' +) +const windowsPwsh = (process.env.PATH ?? '') + .split(delimiter) + .map((directory) => join(directory, 'pwsh.exe')) + .find((file) => existsSync(file)) + +describe.skipIf(process.platform !== 'win32')('Codex hook delivery through PowerShell', () => { + it.each([windowsPowerShell, ...(windowsPwsh ? [windowsPwsh] : [])])( + 'delivers all eight events exactly once from a Unicode profile through %s', + async (shell) => { + const root = mkdtempSync(join(tmpdir(), 'orca-codex-cjk-')) + const home = join(root, "测试 사용자 O'Brien") + mkdirSync(home) + const scriptPath = join(home, 'codex-hook.cmd') + writeFileSync(scriptPath, getManagedScript()) + const posts: URLSearchParams[] = [] + const tokens: unknown[] = [] + const server = createServer((req, res) => { + const chunks: Buffer[] = [] + req.on('data', (chunk) => chunks.push(chunk)) + req.on('end', () => { + tokens.push(req.headers['x-orca-agent-hook-token']) + posts.push(new URLSearchParams(Buffer.concat(chunks).toString('utf8'))) + res.writeHead(204).end() + }) + }) + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)) + const address = server.address() + if (!address || typeof address === 'string') { + throw new Error('Missing listener port') + } + const env = { + ...Object.fromEntries( + Object.entries(process.env).filter(([key]) => !key.startsWith('ORCA_')) + ), + ORCA_BACKGROUND_LAUNCH: '1', + ORCA_AGENT_HOOK_PORT: String(address.port), + ORCA_AGENT_HOOK_TOKEN: 'unicode-test-token', + ORCA_PANE_KEY: 'unicode-tab:unicode-leaf', + ORCA_WORKTREE_ID: 'C:\\folder workspace\\测试 & repo' + } + const payloads = CODEX_EVENTS.map((hook_event_name) => + JSON.stringify({ + hook_event_name, + prompt: '测试 한국어 😀 " \\ \n & %PATH% ! $HOME '.repeat(7000) + }) + ) + const invoke = (command: string, input: string) => + runProcess({ + program: shell, + args: ['-NoProfile', '-Command', command], + input, + env, + timeoutMs: 10_000, + terminationBarrier: true + }) + try { + for (let offset = 0; offset < payloads.length; offset += 4) { + const results = await Promise.all( + payloads + .slice(offset, offset + 4) + .map((payload) => invoke(getManagedCommand(scriptPath), payload)) + ) + for (const result of results) { + expect(result).toMatchObject({ code: 0, stdout: '', stderr: '', timedOut: false }) + } + } + expect(posts).toHaveLength(CODEX_EVENTS.length) + expect(tokens).toEqual(CODEX_EVENTS.map(() => 'unicode-test-token')) + expect(posts.map((post) => post.get('payload')).sort()).toEqual([...payloads].sort()) + for (const post of posts) { + expect(post.get('paneKey')).toBe(env.ORCA_PANE_KEY) + expect(post.get('worktreeId')).toBe(env.ORCA_WORKTREE_ID) + } + await new Promise((resolve) => server.close(() => resolve())) + expect(await invoke(getManagedCommand(scriptPath), payloads[0])).toMatchObject({ + code: 0, + stdout: '', + stderr: '', + timedOut: false + }) + rmSync(scriptPath) + expect(await invoke(getManagedCommand(scriptPath), payloads[0])).toMatchObject({ + code: 0, + stdout: '', + stderr: '', + timedOut: false + }) + expect(posts).toHaveLength(CODEX_EVENTS.length) + } finally { + await new Promise((resolve) => server.close(() => resolve())) + await removeTree(root) + } + }, + 30_000 + ) +}) diff --git a/src/main/codex/windows-hook-upgrade.test.ts b/src/main/codex/windows-hook-upgrade.test.ts new file mode 100644 index 00000000000..203fdfb99c2 --- /dev/null +++ b/src/main/codex/windows-hook-upgrade.test.ts @@ -0,0 +1,97 @@ +import { describe, expect, it, vi } from 'vitest' +import { mkdirSync, readFileSync, writeFileSync } from 'node:fs' +import { join } from 'node:path' +import type * as Os from 'node:os' +import { setupCodexHookHomes } from './hook-service-test-harness' + +const { getPathMock, homedirMock } = vi.hoisted(() => ({ + getPathMock: vi.fn<(name: string) => string>(), + homedirMock: vi.fn<() => string>() +})) +vi.mock('electron', () => ({ app: { getPath: getPathMock } })) +vi.mock('os', async (importOriginal) => ({ + ...(await importOriginal()), + homedir: homedirMock +})) + +import { CodexHookService } from './hook-service' +import { CODEX_EVENTS, CODEX_EVENT_LABEL, getManagedCommand } from './codex-hook-definition' +import { readHooksJson, wrapWindowsHookCommand } from '../agent-hooks/installer-utils' +import { + computeTrustedHash, + getCodexExplicitHomeHookSourcePath, + upsertHookTrustEntries +} from './config-toml-trust' + +const homes = setupCodexHookHomes(homedirMock, getPathMock) + +describe.skipIf(process.platform !== 'win32')('Unicode Windows hook upgrade', () => { + it('replaces all encoded commands and trust hashes while preserving user hooks on reinstall', async () => { + const home = join(homes.tmpHome, '测试 用户') + mkdirSync(home) + homedirMock.mockReturnValue(home) + const runtimeHome = join(homes.userDataDir, 'codex-runtime-home', 'home') + const configPath = join(runtimeHome, 'hooks.json') + const tomlPath = join(runtimeHome, 'config.toml') + const scriptPath = join(home, '.orca', 'agent-hooks', 'codex-hook.cmd') + const oldCommand = wrapWindowsHookCommand(scriptPath) + const userHome = join(home, '.codex') + mkdirSync(userHome) + const userConfig = JSON.stringify({ + hooks: { Stop: [{ hooks: [{ type: 'command', command: 'user-hook' }] }] } + }) + writeFileSync(join(userHome, 'hooks.json'), userConfig) + mkdirSync(runtimeHome, { recursive: true }) + writeFileSync( + configPath, + JSON.stringify({ + hooks: Object.fromEntries( + CODEX_EVENTS.map((event) => [ + event, + [{ hooks: [{ type: 'command', command: oldCommand, timeout: 10 }] }] + ]) + ) + }) + ) + upsertHookTrustEntries( + tomlPath, + CODEX_EVENTS.map((event) => ({ + sourcePath: getCodexExplicitHomeHookSourcePath(configPath), + eventLabel: CODEX_EVENT_LABEL[event], + groupIndex: 0, + handlerIndex: 0, + command: oldCommand, + timeoutSec: 10 + })) + ) + const service = new CodexHookService() + expect(service.getStatus().state).not.toBe('installed') + for (let pass = 0; pass < 2; pass++) { + expect((await service.install()).state).toBe('installed') + expect(service.getStatus().state).toBe('installed') + const hooks = readHooksJson(configPath)?.hooks + const trust = readFileSync(tomlPath, 'utf8') + for (const event of CODEX_EVENTS) { + const commands = hooks?.[event]?.flatMap((group) => group.hooks ?? []) ?? [] + expect( + commands.filter((hook) => hook.command === getManagedCommand(scriptPath)) + ).toHaveLength(1) + expect(commands.some((hook) => hook.command === oldCommand)).toBe(false) + const entry = { + sourcePath: getCodexExplicitHomeHookSourcePath(configPath), + eventLabel: CODEX_EVENT_LABEL[event], + groupIndex: 0, + handlerIndex: 0, + command: getManagedCommand(scriptPath), + timeoutSec: 10 + } + expect(trust).toContain(computeTrustedHash(entry)) + expect(trust).not.toContain(computeTrustedHash({ ...entry, command: oldCommand })) + } + expect( + hooks?.Stop?.some((group) => group.hooks?.some((hook) => hook.command === 'user-hook')) + ).toBe(true) + expect(readFileSync(join(userHome, 'hooks.json'), 'utf8')).toBe(userConfig) + } + }) +})