From 41c388f44ada4e4dfda5449ef57e0e9961619635 Mon Sep 17 00:00:00 2001 From: Neil Date: Thu, 10 Sep 2026 01:38:22 -0700 Subject: [PATCH] fix(agent-hooks): only redirect the Codex hook layout when the home really moved `installRemote` treats an explicit `codexHomeDir` as a redirected runtime home: it moves the hook script under that home, switches the command wrapper to the readable form and prepends rather than appends the managed hook group. Passing it `~/.codex` for every host applied that WSL-shaped contract to plain SSH hosts too, relocating the script from `~/.orca/agent-hooks/codex-hook.sh` to `~/.codex/.orca/agent-hooks/codex-hook.sh` for users who have no wrapper at all. `resolveRelayRedirectedCodexHome` now answers `null` for the ordinary home, and the installer only receives `codexHomeDir` when Codex really did name somewhere else. Verified against the Linux SSH host: an unwrapped host reproduces the `origin/main` layout exactly, and a wrapped one installs under the reported home. --- .../agent-hooks/managed-hook-runtime.test.ts | 66 ++++++++++++++----- src/main/agent-hooks/managed-hook-runtime.ts | 27 ++++---- 2 files changed, 67 insertions(+), 26 deletions(-) diff --git a/src/main/agent-hooks/managed-hook-runtime.test.ts b/src/main/agent-hooks/managed-hook-runtime.test.ts index 9de011e1ecb..bb4ec809936 100644 --- a/src/main/agent-hooks/managed-hook-runtime.test.ts +++ b/src/main/agent-hooks/managed-hook-runtime.test.ts @@ -17,7 +17,7 @@ const { execFile } = await import('node:child_process') const execFileMock = vi.mocked(execFile) const { execFile: actualExecFile } = await vi.importActual('node:child_process') -const { installManagedHooks, resolveRelayCodexHome, resolveRelayGrokHome } = +const { installManagedHooks, resolveRelayGrokHome, resolveRelayRedirectedCodexHome } = await import('./managed-hook-runtime') type ExecFileCallback = (error: Error | null, result?: { stdout: string; stderr: string }) => void @@ -160,21 +160,40 @@ describe.runIf(process.platform !== 'win32')('installManagedHooks', () => { }) }) -describe.runIf(process.platform !== 'win32')('resolveRelayCodexHome', () => { +describe.runIf(process.platform !== 'win32')('resolveRelayRedirectedCodexHome', () => { const CODEX_ONLY = ['codex'] as const - it('uses the CODEX_HOME the host s own Codex reports', async () => { + it('reports the CODEX_HOME the host s own Codex names', async () => { // #19598: a launcher wrapper exports CODEX_HOME inside the script, so only // Codex itself knows. The login-shell environment never sees it. await expect( - resolveRelayCodexHome('/home/orca', CODEX_ONLY, undefined, async () => '/home/orca/.codex-x/') + resolveRelayRedirectedCodexHome( + '/home/orca', + CODEX_ONLY, + undefined, + async () => '/home/orca/.codex-x/' + ) ).resolves.toBe('/home/orca/.codex-x') }) - it('falls back to ~/.codex when Codex does not answer', async () => { + // Why null rather than the path: an explicit codexHomeDir switches + // installRemote to its redirected-runtime contract (script location, command + // wrapper, hook order). An unwrapped host must keep the incumbent layout. + it('reports no redirect when Codex names the ordinary home', async () => { await expect( - resolveRelayCodexHome('/home/orca', CODEX_ONLY, undefined, async () => null) - ).resolves.toBe('/home/orca/.codex') + resolveRelayRedirectedCodexHome( + '/home/orca/', + CODEX_ONLY, + undefined, + async () => '/home/orca/.codex' + ) + ).resolves.toBeNull() + }) + + it('reports no redirect when Codex does not answer', async () => { + await expect( + resolveRelayRedirectedCodexHome('/home/orca', CODEX_ONLY, undefined, async () => null) + ).resolves.toBeNull() }) it.each([ @@ -182,18 +201,18 @@ describe.runIf(process.platform !== 'win32')('resolveRelayCodexHome', () => { ['Windows', 'C:\\Users\\bob\\.codex'], ['control-character', '/home/orca/.codex\u0007'], ['empty', ' '] - ])('falls back when the reported home is %s', async (_label, reported) => { + ])('reports no redirect when the reported home is %s', async (_label, reported) => { await expect( - resolveRelayCodexHome('/home/orca', CODEX_ONLY, undefined, async () => reported) - ).resolves.toBe('/home/orca/.codex') + resolveRelayRedirectedCodexHome('/home/orca', CODEX_ONLY, undefined, async () => reported) + ).resolves.toBeNull() }) it('does not probe for a host with no detected Codex', async () => { const probe = vi.fn() - await expect(resolveRelayCodexHome('/home/orca', ['claude'], undefined, probe)).resolves.toBe( - '/home/orca/.codex' - ) + await expect( + resolveRelayRedirectedCodexHome('/home/orca', ['claude'], undefined, probe) + ).resolves.toBeNull() expect(probe).not.toHaveBeenCalled() }) }) @@ -235,7 +254,9 @@ describe.runIf(process.platform !== 'win32')( }) }) - it('still installs into ~/.codex when Codex reports the default home', async () => { + // The redirected-home contract moves the hook script under CODEX_HOME and + // rewrites the command; an unwrapped host must keep the incumbent layout. + it('leaves an unwrapped host on its incumbent ~/.codex layout', async () => { const home = await createTempHome() await stubWrappedCodex(home, join(home, '.codex')) @@ -245,8 +266,23 @@ describe.runIf(process.platform !== 'win32')( }) await expect(readFile(join(home, '.codex', 'hooks.json'), 'utf8')).resolves.toContain( - 'codex-hook.sh' + join(home, '.orca', 'agent-hooks', 'codex-hook.sh') ) + await expect( + readFile(join(home, '.codex', '.orca', 'agent-hooks', 'codex-hook.sh'), 'utf8') + ).rejects.toMatchObject({ code: 'ENOENT' }) + }) + + it('moves the hook script under a redirected home, as the runtime installer does', async () => { + const home = await createTempHome() + const codexHome = join(home, '.codex-openai') + await stubWrappedCodex(home, codexHome) + + await installManagedHooks({ agents: ['codex'] }) + + await expect( + readFile(join(codexHome, '.orca', 'agent-hooks', 'codex-hook.sh'), 'utf8') + ).resolves.toContain('#!/bin/sh') }) } ) diff --git a/src/main/agent-hooks/managed-hook-runtime.ts b/src/main/agent-hooks/managed-hook-runtime.ts index 94f90305316..64880d24cc6 100644 --- a/src/main/agent-hooks/managed-hook-runtime.ts +++ b/src/main/agent-hooks/managed-hook-runtime.ts @@ -80,26 +80,30 @@ export async function resolveRelayGrokHome(home: string, signal?: AbortSignal): } /** - * Where this host's Codex actually keeps its config. + * The Codex home this host redirects to, or `null` for the ordinary `~/.codex`. * * `printenv CODEX_HOME` is not enough: a launcher wrapper exports it inside the * script and `exec`s the real binary, so it never reaches the parent shell * (#19598). Codex's own app-server handshake reports the value the wrapper set, - * which is the only authority on the question. Anything else — no Codex on - * PATH, a CLI too old for the handshake, a timeout, a non-POSIX answer — falls - * back to `~/.codex`, the incumbent behaviour. + * which is the only authority on the question. No Codex on PATH, a CLI too old + * for the handshake, a timeout, an abort, or a non-POSIX answer all read as + * "not redirected". + * + * Why `null` rather than the default path: `installRemote` treats an explicit + * `codexHomeDir` as a redirected runtime home and moves the hook script under + * it, switches the command wrapper and reorders the hook groups. Handing it + * `~/.codex` would apply that contract to every unwrapped host as well. */ -export async function resolveRelayCodexHome( +export async function resolveRelayRedirectedCodexHome( home: string, agents: readonly AgentHookTarget[], signal?: AbortSignal, probe: typeof probeCodexHomeViaAppServer = probeCodexHomeViaAppServer -): Promise { - const fallback = defaultAgentHome(home, '.codex') +): Promise { // Why: only a positively detected Codex pays for the probe; other hosts must // not start an app-server for a CLI the user never installed. if (!agents.includes('codex')) { - return fallback + return null } const { shell, flag } = loginShellInvocation() const reported = await probe({ @@ -107,7 +111,8 @@ export async function resolveRelayCodexHome( loginShellFlag: flag, ...(signal ? { signal } : {}) }) - return (reported === null ? null : normalizePosixAgentHome(reported.trim())) ?? fallback + const codexHome = reported === null ? null : normalizePosixAgentHome(reported.trim()) + return !codexHome || codexHome === defaultAgentHome(home, '.codex') ? null : codexHome } export async function installManagedHooks(options?: { @@ -124,7 +129,7 @@ export async function installManagedHooks(options?: { const home = homedir() const grokHomeDir = await resolveRelayGrokHome(home, options?.signal) options?.signal?.throwIfAborted() - const codexHomeDir = await resolveRelayCodexHome(home, agents, options?.signal) + const codexHomeDir = await resolveRelayRedirectedCodexHome(home, agents, options?.signal) options?.signal?.throwIfAborted() const hostIdentity = scopeManagedHookHostIdentity( await readManagedHookHostIdentity(), @@ -138,7 +143,7 @@ export async function installManagedHooks(options?: { createManagedHookLocalFilesystem(), home, { - codexHomeDir, + ...(codexHomeDir ? { codexHomeDir } : {}), grokHomeDir, signal: options?.signal, agents