From b552e719be893c26fdb149ad40d9c7ff424695ed Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 15 Jul 2026 15:16:39 -0700 Subject: [PATCH] fix(codex): preserve remote env and complete real-home cleanup --- .../codex/hook-service-trust-grant.test.ts | 41 ++++++++++++++++++- src/main/codex/hook-service.ts | 31 +++++++++++++- src/main/ipc/pty.test.ts | 32 +++++++++++---- src/main/ipc/pty.ts | 28 +++++++------ 4 files changed, 110 insertions(+), 22 deletions(-) diff --git a/src/main/codex/hook-service-trust-grant.test.ts b/src/main/codex/hook-service-trust-grant.test.ts index b317f5b3b65..b7028334721 100644 --- a/src/main/codex/hook-service-trust-grant.test.ts +++ b/src/main/codex/hook-service-trust-grant.test.ts @@ -5,8 +5,10 @@ import type * as Os from 'node:os' import { join } from 'node:path' import { wrapPosixHookCommand } from '../agent-hooks/installer-utils' import { + computeTrustKey, computeTrustedHash, parseTrustKey, + readHookTrustEntries, upsertHookTrustEntries, type CodexTrustEntry } from './config-toml-trust' @@ -17,6 +19,7 @@ import { writeCodexTrustGrantLedgerHome } from './codex-trust-grant-ledger' import type { CodexHookTrustGrantRequest } from './codex-app-server-client' +import { getCodexHookTrustSignature } from './codex-hook-identity' const { getPathMock, homedirMock } = vi.hoisted(() => ({ getPathMock: vi.fn<(name: string) => string>(), @@ -29,7 +32,7 @@ vi.mock('os', async (importOriginal) => { return { ...actual, homedir: homedirMock } }) -import { CodexHookService } from './hook-service' +import { CodexHookService, getCodexManagedHookInstallMaterial } from './hook-service' let tmpHome: string let userDataDir: string @@ -136,6 +139,42 @@ describe('CodexHookService app-server trust grant lane', () => { expect(readFileSync(join(managedHome, 'config.toml'))).toEqual(firstToml) }) + it('retries ledger-proven real-home trust cleanup after the hook is already gone', () => { + prepareSystemHome() + const systemHome = join(tmpHome, '.codex') + const hooksPath = join(systemHome, 'hooks.json') + const configPath = join(systemHome, 'config.toml') + const material = getCodexManagedHookInstallMaterial() + const trustedHash = 'sha256:codex-real-home-stop' + const entry: CodexTrustEntry = { + sourcePath: hooksPath, + eventLabel: 'stop', + groupIndex: 0, + handlerIndex: 0, + command: material.command, + timeoutSec: 10, + trustedHash + } + const trustKey = computeTrustKey(entry) + writeFileSync(hooksPath, `${JSON.stringify({ hooks: {} }, null, 2)}\n`) + upsertHookTrustEntries(configPath, [entry]) + writeCodexTrustGrantLedgerHome(systemHome, { + binary: null, + entries: { + [trustKey]: { + signature: getCodexHookTrustSignature(entry), + trustedHash + } + } + }) + installCodexLikeGrantRunner() + + expect(new CodexHookService().install().state).toBe('installed') + + expect(readHookTrustEntries(configPath).has(trustKey)).toBe(false) + expect(readCodexTrustGrantLedgerHome(systemHome)).toBeNull() + }) + it('does not accept a ledger hash after the recorded Codex binary stamp changes', () => { prepareSystemHome() installCodexLikeGrantRunner() diff --git a/src/main/codex/hook-service.ts b/src/main/codex/hook-service.ts index a9f12bb38de..1668608dc80 100644 --- a/src/main/codex/hook-service.ts +++ b/src/main/codex/hook-service.ts @@ -228,7 +228,10 @@ function collectManagedTrustEntries( return entries } -function removeMatchingTrustEntries(configPath: string, entries: readonly CodexTrustEntry[]): void { +function removeSelfComputedMatchingTrustEntries( + configPath: string, + entries: readonly CodexTrustEntry[] +): void { if (entries.length === 0) { return } @@ -573,6 +576,17 @@ function dedupeHookDefinitions(definitions: readonly HookDefinition[]): HookDefi }) } +function removeSystemManagedHookTrustEntries(systemHomePath: string, hooksJsonPath: string): void { + removeCodexManagedHookTrustEntries({ + tomlPath: getSystemCodexConfigTomlPath(), + runtimeHomePath: systemHomePath, + sourcePath: hooksJsonPath, + command: getManagedCommand(getManagedScriptPath()), + managedEventLabels: CODEX_MANAGED_EVENT_LABELS, + timeoutSec: MANAGED_HOOK_TIMEOUT_SECONDS + }) +} + function cleanupLegacySystemManagedHooks(): void { if (systemCodexHomeHookSweepSuppressed()) { return @@ -583,8 +597,14 @@ function cleanupLegacySystemManagedHooks(): void { return } + const systemHomePath = getSystemCodexHomePath() + const hasRecordedRealHomeGrant = + readCodexTrustGrantLedgerHomeForReconciliation(systemHomePath) !== null const config = readHooksJson(legacyConfigPath) if (!config?.hooks) { + if (hasRecordedRealHomeGrant) { + removeSystemManagedHookTrustEntries(systemHomePath, legacyConfigPath) + } return } @@ -623,8 +643,15 @@ function cleanupLegacySystemManagedHooks(): void { // Why: this is the user's system hooks file, not Orca's runtime copy. // Remove only stale Orca hook entries and preserve other managers' metadata. writeHooksJson(legacyConfigPath, { ...config, hooks: nextHooks }) + // Why: stale dev/version entries can reference an older managed script + // path that is not represented by the current grant ledger. + removeSelfComputedMatchingTrustEntries(getSystemCodexConfigTomlPath(), trustEntries) + } + if (removedManagedHook || hasRecordedRealHomeGrant) { + // Why: the ledger recognizes Codex-computed hashes and remains a retry + // marker if a prior cleanup removed hooks.json but could not update TOML. + removeSystemManagedHookTrustEntries(systemHomePath, legacyConfigPath) } - removeMatchingTrustEntries(getSystemCodexConfigTomlPath(), trustEntries) } function stripLegacyManagedProfileBlock(content: string): string { diff --git a/src/main/ipc/pty.test.ts b/src/main/ipc/pty.test.ts index a7cb4b1c9e2..a1c2120a104 100644 --- a/src/main/ipc/pty.test.ts +++ b/src/main/ipc/pty.test.ts @@ -2383,7 +2383,12 @@ describe('registerPtyHandlers', () => { it('does NOT inject host-local env on SSH spawns (connectionId set)', async () => { const sshSpawn = vi.fn( - async (_opts: { env: Record; paneKey?: string; tabId?: string }) => ({ + async (_opts: { + env: Record + envToDelete?: string[] + paneKey?: string + tabId?: string + }) => ({ id: 'ssh-pty' }) ) @@ -2420,7 +2425,8 @@ describe('registerPtyHandlers', () => { undefined, (() => ({ httpProxyUrl: 'http://proxy.example:8080', - httpProxyBypassRules: 'localhost' + httpProxyBypassRules: 'localhost', + codexSystemDefaultRealHomeEnabled: true })) as never, undefined, store as never @@ -2459,6 +2465,10 @@ describe('registerPtyHandlers', () => { expect(env.HTTPS_PROXY).toBeUndefined() expect(env.NO_PROXY).toBeUndefined() expect(env.FOO).toBe('bar') + // Why: real-home routing is host-only. A null local-home resolver on + // SSH must not become a request to alter the remote Codex environment. + expect(spawnOptions.envToDelete ?? []).not.toContain('CODEX_HOME') + expect(spawnOptions.envToDelete ?? []).not.toContain('ORCA_CODEX_HOME') expect(spawnOptions.paneKey).toBe(makePaneKey('tab-1', leafId)) expect(spawnOptions.tabId).toBe('tab-1') expect(openCodeBuildPtyEnvMock).not.toHaveBeenCalled() @@ -5635,9 +5645,11 @@ describe('registerPtyHandlers', () => { } const savedRemoteHooks = process.env.ORCA_FEATURE_REMOTE_AGENT_HOOKS process.env.ORCA_FEATURE_REMOTE_AGENT_HOOKS = '0' - const remoteSpawn = vi.fn(async (_opts: { env?: Record }) => ({ - id: 'ssh:ssh-runtime-env@@relay-pty' - })) + const remoteSpawn = vi.fn( + async (_opts: { env?: Record; envToDelete?: string[] }) => ({ + id: 'ssh:ssh-runtime-env@@relay-pty' + }) + ) registerSshPtyProvider('ssh-runtime-env', { spawn: remoteSpawn, write: vi.fn(), @@ -5683,7 +5695,10 @@ describe('registerPtyHandlers', () => { mainWindow as never, runtime as never, undefined, - undefined, + (() => ({ + agentStatusHooksEnabled: false, + codexSystemDefaultRealHomeEnabled: true + })) as never, undefined, store as never ) @@ -5705,11 +5720,14 @@ describe('registerPtyHandlers', () => { persistHostSessionBinding: true }) - const env = remoteSpawn.mock.calls[0]?.[0].env + const spawnOptions = remoteSpawn.mock.calls[0]?.[0] + const env = spawnOptions.env expect(env).toMatchObject({ FOO: 'bar' }) expect(env?.ORCA_PANE_KEY).toBeUndefined() expect(env?.ORCA_TAB_ID).toBeUndefined() expect(env?.ORCA_WORKTREE_ID).toBeUndefined() + expect(spawnOptions.envToDelete ?? []).not.toContain('CODEX_HOME') + expect(spawnOptions.envToDelete ?? []).not.toContain('ORCA_CODEX_HOME') expect(store.upsertSshRemotePtyLease).toHaveBeenCalledWith( expect.objectContaining({ targetId: 'ssh-runtime-env', diff --git a/src/main/ipc/pty.ts b/src/main/ipc/pty.ts index 72e499001c0..c938ec53001 100644 --- a/src/main/ipc/pty.ts +++ b/src/main/ipc/pty.ts @@ -3093,12 +3093,14 @@ export function registerPtyHandlers( isDaemonHostSpawn && shouldSkipCodexHomeEnvForWindowsShell(daemonShellOverride, cwd) && !selectedCodexHomePath - const stripInheritedOrcaCodexHome = shouldStripInheritedOrcaCodexHome({ - target: codexSelectionTarget, - selectedCodexHomePath, - skipCodexHomeEnv, - settings: getSettings?.() - }) + const stripInheritedOrcaCodexHome = + isDaemonHostSpawn && + shouldStripInheritedOrcaCodexHome({ + target: codexSelectionTarget, + selectedCodexHomePath, + skipCodexHomeEnv, + settings: getSettings?.() + }) if (isDaemonHostSpawn && sessionId) { if (!isSafePtySessionId(sessionId, app.getPath('userData'))) { throw new Error('Invalid PTY session id') @@ -3974,12 +3976,14 @@ export function registerPtyHandlers( isDaemonHostSpawn && shouldSkipCodexHomeEnvForWindowsShell(effectiveShellOverride, cwd) && !selectedCodexHomePath - const stripInheritedOrcaCodexHome = shouldStripInheritedOrcaCodexHome({ - target: codexSelectionTarget, - selectedCodexHomePath, - skipCodexHomeEnv, - settings: getSettings?.() - }) + const stripInheritedOrcaCodexHome = + isDaemonHostSpawn && + shouldStripInheritedOrcaCodexHome({ + target: codexSelectionTarget, + selectedCodexHomePath, + skipCodexHomeEnv, + settings: getSettings?.() + }) if (isDaemonHostSpawn) { if (effectiveSessionId === undefined) { // Should be unreachable: the expression above returns a string when