From add1eccae384ad55ef42d70a1656a95decc4a728 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Mon, 19 Oct 2026 00:31:11 -0700 Subject: [PATCH] fix(codex): a Codex chat keeps its Orca identity when the user's Codex config strips command environment variables (#30406) * fix(codex): a Codex chat's commands keep its Orca identity under a restricted shell environment policy Codex rebuilds each command's environment from the user's shell_environment_policy, so inherit = "core" dropped the chat id, marker, launcher and instance pin Orca gives the app-server. orca orchestration then could not tell which chat was calling. Every thread start and resume now sends those values as shell_environment_policy.set overrides, which Codex applies after its filter and merges per key. * fix(codex): accept the launch's read-only delete list * test(codex): a chat without visuals gets no visuals config, identity overrides aside * refactor(codex): a chat's command env is what the seal wrote, not a second key list The seal now returns the session values it set (identity, a registered worker's own handle, the CLI launcher, the instance pin and the blanked remote selectors) as commandEnv, and the composed launch carries it. The Codex acquire sends that plus the chat's scratch folder from its own visuals. This deletes the hand-kept key list, the handle prefix test and the delete-list re-check, so a key later added to the seal reaches each command too, and an inherited worker handle the seal never named no longer does. * test(codex): wait for the shell command's turn to complete before the cold resume --- ...ex-structured-real-cli-command-env.test.ts | 84 +++++++++++++++++++ .../codex/codex-structured-session-acquire.ts | 11 ++- .../codex-structured-session-adapter.test.ts | 14 +++- .../codex/codex-structured-session-state.ts | 2 + .../codex/codex-structured-thread-open.ts | 16 +++- .../codex/codex-structured-visuals.test.ts | 11 ++- ...native-chat-inline-visuals-setting.test.ts | 9 +- .../structured-agent-launch-composition.ts | 17 +++- .../runtime/structured-session-child-env.ts | 44 +++++----- 9 files changed, 178 insertions(+), 30 deletions(-) create mode 100644 src/main/codex/codex-structured-real-cli-command-env.test.ts diff --git a/src/main/codex/codex-structured-real-cli-command-env.test.ts b/src/main/codex/codex-structured-real-cli-command-env.test.ts new file mode 100644 index 00000000000..401684247e2 --- /dev/null +++ b/src/main/codex/codex-structured-real-cli-command-env.test.ts @@ -0,0 +1,84 @@ +// The installed Codex under a user policy that keeps only core env vars: a user shell command +// (`thread/shellCommand`) must still see the chat's identity, on a new thread and on a cold resume. +// The model's exec tool reads the same `shell_environment_policy.set` in Codex's source. Opt-in +// (ORCA_REAL_CODEX_CLI_TEST=1); no account is used, nothing reaches a model. +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { describe, expect, it } from 'vitest' +import { resolveCodexCommand } from '../codex-cli/command' +import { openCodexAppServerConnection } from './codex-app-server-connection' +import { openCodexThread } from './codex-structured-thread-open' +import { codexStructuredPermissionPolicy } from './codex-structured-permission-policy' + +// The probe is a POSIX shell command. +const enabled = process.env.ORCA_REAL_CODEX_CLI_TEST === '1' && process.platform !== 'win32' +const SESSION_ID = 'codex_real_command_env' +const PROBE = 'printf "probe[%s]" "$ORCA_AGENT_SESSION_ID"' + +async function probeIdentity( + base: string, + thread: { resumeThreadId: string | null; commandEnv?: Record } +): Promise<{ threadId: string; seen: string }> { + let output = '' + let turnCompleted = false + const cwd = join(base, 'workspace') + const connection = await openCodexAppServerConnection( + { + command: resolveCodexCommand(), + args: ['app-server'], + cwd, + env: { HOME: base, CODEX_HOME: join(base, 'codex-home'), ORCA_AGENT_SESSION_ID: SESSION_ID } + }, + { + onNotification: (method, params) => { + output += JSON.stringify(params) + turnCompleted ||= method === 'turn/completed' + } + } + ) + try { + const opened = await openCodexThread( + connection, + { cwd, permissionPolicy: codexStructuredPermissionPolicy('ask'), ...thread }, + 20_000 + ) + await connection.request( + 'thread/shellCommand', + { threadId: opened.threadId, command: PROBE }, + { timeoutMs: 10_000 } + ) + // Until the turn completes Codex may not have saved the rollout the cold resume reads. + const deadline = Date.now() + 15_000 + while (!turnCompleted && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 100)) + } + const seen = output.match(/probe\[([^\]%]*)\]/) + return { threadId: opened.threadId, seen: seen?.[1] ?? '' } + } finally { + await connection.close() + } +} + +describe.skipIf(!enabled)('Codex real app-server command env', () => { + it('keeps the chat identity in commands under inherit = "core", on start and resume', async () => { + const base = mkdtempSync(join(tmpdir(), 'orca-real-codex-env-')) + try { + mkdirSync(join(base, 'workspace'), { recursive: true }) + mkdirSync(join(base, 'codex-home'), { recursive: true }) + writeFileSync( + join(base, 'codex-home', 'config.toml'), + '[shell_environment_policy]\ninherit = "core"\n' + ) + const commandEnv = { ORCA_AGENT_SESSION_ID: SESSION_ID } + // The policy really strips it: the app-server holds the id, its command does not. + expect((await probeIdentity(base, { resumeThreadId: null })).seen).toBe('') + const started = await probeIdentity(base, { resumeThreadId: null, commandEnv }) + expect(started.seen).toBe(SESSION_ID) + const resumed = await probeIdentity(base, { resumeThreadId: started.threadId, commandEnv }) + expect(resumed).toEqual({ threadId: started.threadId, seen: SESSION_ID }) + } finally { + rmSync(base, { recursive: true, force: true }) + } + }, 120_000) +}) diff --git a/src/main/codex/codex-structured-session-acquire.ts b/src/main/codex/codex-structured-session-acquire.ts index 7d400dab145..88e30f4d162 100644 --- a/src/main/codex/codex-structured-session-acquire.ts +++ b/src/main/codex/codex-structured-session-acquire.ts @@ -24,6 +24,7 @@ import { } from './codex-structured-owner-identity' import { openCodexThread } from './codex-structured-thread-open' import { withCodexVisualsThreadConfig } from './codex-structured-visuals' +import { NATIVE_CHAT_SCRATCH_DIR_ENV } from '../native-chat/native-chat-visuals-delivery' import { closeCodexPublishedSession, handleCodexSessionExit @@ -193,7 +194,15 @@ export async function acquireCodexStructuredSession(input: { ...(deps.logger ? { logger: deps.logger } : {}) }) acquisitions.assertCurrent(sessionId, attempt) - const opened = await openCodexThread(connection, threadLaunch, deps.requestTimeoutMs) + const commandEnv = { + ...launch.commandEnv, + ...(launch.visuals ? { [NATIVE_CHAT_SCRATCH_DIR_ENV]: launch.visuals.folder } : {}) + } + const opened = await openCodexThread( + connection, + { ...threadLaunch, commandEnv }, + deps.requestTimeoutMs + ) acquisitions.assertCurrent(sessionId, attempt) primaryThreadId = opened.threadId const restoreAdmission = translator?.restoreThread(opened.threadId, opened.thread ?? {}) diff --git a/src/main/codex/codex-structured-session-adapter.test.ts b/src/main/codex/codex-structured-session-adapter.test.ts index 6ed9144c6e9..9ed195bfd24 100644 --- a/src/main/codex/codex-structured-session-adapter.test.ts +++ b/src/main/codex/codex-structured-session-adapter.test.ts @@ -171,7 +171,19 @@ describe('CodexStructuredSessionAdapter.acquire', () => { cwd: '/work/repo', approvalPolicy: 'on-request', sandbox: 'workspace-write', - approvalsReviewer: 'user' + approvalsReviewer: 'user', + // A user's `inherit = "core"` policy would strip these from every command the chat runs. + config: { + 'shell_environment_policy.set.ORCA_AGENT_SESSION_ID': 'session-1', + 'shell_environment_policy.set.ORCA_STRUCTURED_SESSION': '1', + 'shell_environment_policy.set.ORCA_CLI_COMMAND': expect.stringMatching( + /^[^:;]*[\\/]cli[\\/]bin[\\/]orca-dev$/ + ), + 'shell_environment_policy.set.ORCA_USER_DATA_PATH': expect.any(String), + 'shell_environment_policy.set.ORCA_ENVIRONMENT': '', + 'shell_environment_policy.set.ORCA_PAIRING_CODE': '', + 'shell_environment_policy.set.ORCA_REMOTE_PAIRING': '' + } } }) expect(acquisition.process).toEqual({ diff --git a/src/main/codex/codex-structured-session-state.ts b/src/main/codex/codex-structured-session-state.ts index 7a253003bd9..ae04e846c00 100644 --- a/src/main/codex/codex-structured-session-state.ts +++ b/src/main/codex/codex-structured-session-state.ts @@ -48,6 +48,8 @@ export type CodexStructuredLaunch = { /** The child's sealed environment, and the inherited keys it must not keep. */ env?: Record envToDelete?: readonly string[] + /** The session values each command must see, whatever the user's shell environment policy. */ + commandEnv?: Record /** This chat's scratch folder and visuals skill; absent when the chat has no visuals. */ visuals?: NativeChatVisualsLaunch } diff --git a/src/main/codex/codex-structured-thread-open.ts b/src/main/codex/codex-structured-thread-open.ts index c6e686869b7..ce53af7fed9 100644 --- a/src/main/codex/codex-structured-thread-open.ts +++ b/src/main/codex/codex-structured-thread-open.ts @@ -45,6 +45,17 @@ function isCodexNoRolloutError(error: unknown, threadId: string): boolean { ) } +/** + * Codex rebuilds each command's env from `shell_environment_policy`, so `inherit = "core"` drops + * every Orca key the app-server holds. Codex adds `set` keys after that filter and merges these + * per key, so the user's own policy stays as written. + */ +function shellEnvironmentSetConfig(env: Record | undefined): Record { + return Object.fromEntries( + Object.entries(env ?? {}).map(([key, value]) => [`shell_environment_policy.set.${key}`, value]) + ) +} + export async function openCodexThread( connection: Pick, launch: { @@ -58,14 +69,17 @@ export async function openCodexThread( model?: string /** Config overrides for this thread, on start and resume alike. */ threadConfig?: Record + /** Env every command the agent runs must see, whatever the user's shell environment policy. */ + commandEnv?: Record }, timeoutMs: number | undefined ): Promise { const resumeThreadId = launch.resumeThreadId + const config = { ...launch.threadConfig, ...shellEnvironmentSetConfig(launch.commandEnv) } const threadSettings = { cwd: launch.cwd, ...launch.permissionPolicy, - ...(launch.threadConfig ? { config: launch.threadConfig } : {}) + ...(Object.keys(config).length > 0 ? { config } : {}) } const startThread = (): Promise => connection.request( diff --git a/src/main/codex/codex-structured-visuals.test.ts b/src/main/codex/codex-structured-visuals.test.ts index 46b381c4ac7..b4be2830211 100644 --- a/src/main/codex/codex-structured-visuals.test.ts +++ b/src/main/codex/codex-structured-visuals.test.ts @@ -207,7 +207,10 @@ describe('a Codex chat acquisition with visuals', () => { expect(methods.indexOf('config/read')).toBeLessThan(methods.indexOf(open)) expect(connection.calls.find((call) => call.method === open)?.params).toMatchObject({ ...MANUAL, - config: { 'sandbox_workspace_write.writable_roots': ['/home/me/scratch', VISUALS.folder] } + config: { + 'sandbox_workspace_write.writable_roots': ['/home/me/scratch', VISUALS.folder], + [`shell_environment_policy.set.${NATIVE_CHAT_SCRATCH_DIR_ENV}`]: VISUALS.folder + } }) expect(connection.launch.env?.[NATIVE_CHAT_SCRATCH_DIR_ENV]).toBe(VISUALS.folder) } @@ -220,7 +223,11 @@ describe('a Codex chat acquisition with visuals', () => { const methods = connection.calls.map((call) => call.method) expect(methods[0]).toBe('thread/start') expect(methods).not.toContain('skills/extraRoots/set') - expect(connection.calls[0]?.params).not.toHaveProperty('config') + // Only the chat's identity overrides: no visuals root, and no inherited folder handed on. + const config = connection.calls[0]?.params?.config + const scratchDirOverride = `shell_environment_policy.set.${NATIVE_CHAT_SCRATCH_DIR_ENV}` + expect(config).not.toHaveProperty(['sandbox_workspace_write.writable_roots']) + expect(config).not.toHaveProperty([scratchDirOverride]) expect(connection.launch.env).not.toHaveProperty(NATIVE_CHAT_SCRATCH_DIR_ENV) }) }) diff --git a/src/main/native-chat/native-chat-inline-visuals-setting.test.ts b/src/main/native-chat/native-chat-inline-visuals-setting.test.ts index 38962a1602f..1a7f5e75416 100644 --- a/src/main/native-chat/native-chat-inline-visuals-setting.test.ts +++ b/src/main/native-chat/native-chat-inline-visuals-setting.test.ts @@ -129,9 +129,12 @@ describe('inline visuals launch preference', () => { expect(disabled.calls.slice(0, openIndex).map(({ method }) => method)).not.toContain( 'config/read' ) - expect( - disabled.calls.find(({ method }) => method === 'thread/start')?.params - ).not.toHaveProperty('config') + const disabledOpen = disabled.calls.find(({ method }) => method === 'thread/start') + const scratchDirOverride = `shell_environment_policy.set.${NATIVE_CHAT_SCRATCH_DIR_ENV}` + expect(disabledOpen?.params?.config).not.toHaveProperty([ + 'sandbox_workspace_write.writable_roots' + ]) + expect(disabledOpen?.params?.config).not.toHaveProperty([scratchDirOverride]) expect(existsSync(nativeChatScratchFolderFor(stateDirectory, 'disabled'))).toBe(false) expect(first.closed).toBe(false) expect(first.calls).toEqual(firstCalls) diff --git a/src/main/runtime/structured-agent-launch-composition.ts b/src/main/runtime/structured-agent-launch-composition.ts index e868f0f1033..cf69fe4eba0 100644 --- a/src/main/runtime/structured-agent-launch-composition.ts +++ b/src/main/runtime/structured-agent-launch-composition.ts @@ -65,8 +65,11 @@ export type StructuredLaunchRequest = { permissionMode?: AgentChatPermissionMode } +/** What the composition sets on a launch; an agent's part never does. */ +type ComposedLaunchFields = 'cwd' | 'env' | 'envToDelete' | 'commandEnv' + export type StructuredAgentLaunchPart = { - launch: Omit + launch: Omit /** The agent's environment, before the seal names the child. */ env: Record /** Inherited keys this agent's child must not keep; required, so an agent states even none. */ @@ -81,10 +84,12 @@ export type StructuredAgentLaunchPartResolver = ( identity: AgentSessionJournalIdentity ) => Promise> -export type ComposedStructuredLaunch = Omit & { +export type ComposedStructuredLaunch = Omit & { cwd: string env: Record envToDelete: string[] + /** See `SealedStructuredSessionChild`; absent from a read, which seals no child. */ + commandEnv?: Record } /** The child to spawn, sealed with the reservation's token. */ @@ -189,7 +194,13 @@ export function composeStructuredLaunch( inheritedEnvToDelete, ...(cliRuntimeCommand ? { cliRuntimeCommand } : {}) }) - return { ...launch, cwd, env: child.env, envToDelete: child.envToDelete } + return { + ...launch, + cwd, + env: child.env, + envToDelete: child.envToDelete, + commandEnv: child.commandEnv + } } } diff --git a/src/main/runtime/structured-session-child-env.ts b/src/main/runtime/structured-session-child-env.ts index f565a47886f..7ba6ebd5a71 100644 --- a/src/main/runtime/structured-session-child-env.ts +++ b/src/main/runtime/structured-session-child-env.ts @@ -34,7 +34,7 @@ * remote selectors a shell profile may export are set empty, which the CLI reads as unset: empty * rather than deleted, because the child is spawned over Orca's own env, which may carry them. * Explicit `--environment` / `--pairing-code` flags still work, and a person's terminal keeps - * its selectors. A login shell the agent itself starts can export them again. + * its selectors. A login shell the agent starts can export them again, unless its agent sets them. * * Deliberately NOT `ORCA_PANE_KEY`. Claude structured sessions run hooks, and a pane key in their * environment starts flowing into hook-emitted agent-status payloads and the hook-attestation, @@ -84,6 +84,13 @@ export type SealedStructuredSessionChild = { env: Record /** Removed from what the child inherits, after `env` is laid over it. */ envToDelete: string[] + /** + * The session values the seal wrote, for an agent that rebuilds each command's env from its own + * policy (Codex `shell_environment_policy`), which drops them otherwise. Never PATH: Codex keeps + * it under `inherit = "core"`, and a set PATH would freeze it over a login shell's PATH restore. + * Never the spawn token, which is the child's alone. + */ + commandEnv: Record } /** @@ -100,34 +107,29 @@ export function sealStructuredSessionChild(input: { cliRuntimeCommand?: string }): SealedStructuredSessionChild { const identity = childIdentity(input.sessionId, input.env) - const named = new Set([ - ORCA_AGENT_SESSION_ID_ENV, - ORCA_STRUCTURED_SESSION_ENV, - ORCA_AGENT_SESSION_SPAWN_TOKEN_ENV, - ...(identity.workerHandle ? ['ORCA_TERMINAL_HANDLE'] : []) - ]) + const named = new Set([...Object.keys(identity.commandEnv), ORCA_AGENT_SESSION_SPAWN_TOKEN_ENV]) const env = input.cliRuntimeCommand ? withCliRuntimeOnPath(input.cliRuntimeCommand, identity.env, { platform: process.platform }) : identity.env return { env: { ...env, [ORCA_AGENT_SESSION_SPAWN_TOKEN_ENV]: input.spawnToken }, - envToDelete: input.inheritedEnvToDelete.filter((key) => !named.has(key)) + envToDelete: input.inheritedEnvToDelete.filter((key) => !named.has(key)), + commandEnv: identity.commandEnv } } function childIdentity( sessionId: string, childEnv: Record -): { env: Record; workerHandle: string | null } { +): { env: Record; commandEnv: Record } { const identity = resolveStructuredWorkerIdentityForSession(sessionId, null) - const env: Record = { - ...childEnv, + const named: Record = { ...(identity ? { ORCA_TERMINAL_HANDLE: identity.handle } : {}), [ORCA_AGENT_SESSION_ID_ENV]: sessionId, [ORCA_STRUCTURED_SESSION_ENV]: '1' } - applyThisAppCli(env) - return { env, workerHandle: identity?.handle ?? null } + const env = { ...childEnv, ...named } + return { env, commandEnv: { ...named, ...applyThisAppCli(env) } } } const REMOTE_RUNTIME_SELECTOR_ENV_KEYS = [ @@ -137,31 +139,35 @@ const REMOTE_RUNTIME_SELECTOR_ENV_KEYS = [ ] as const /** - * A host with no app environment installed — a plain-Node fork, or a unit test — has no userData - * root to resolve, and inventing one would write a shim into the wrong directory. + * Returns the values it set, beside the PATH it prepends. A host with no app environment + * installed — a plain-Node fork, or a unit test — has no userData root to resolve, and inventing + * one would write a shim into the wrong directory. */ -function applyThisAppCli(env: Record): void { +function applyThisAppCli(env: Record): Record { delete env.ORCA_CLI_COMMAND if (!hasAppEnvironment()) { - return + return {} } const app = getAppEnvironment() const userDataPath = app.getPath('userData') const isPackaged = app.isPackaged() - env.ORCA_USER_DATA_PATH = userDataPath + const set: Record = { ORCA_USER_DATA_PATH: userDataPath } for (const key of REMOTE_RUNTIME_SELECTOR_ENV_KEYS) { - env[key] = '' + set[key] = '' } + Object.assign(env, set) const launcher = prependOrcaCliDirToChildPath(env, { isPackaged, userDataPath, resourcesPath: process.resourcesPath ?? null }) if (launcher) { + set.ORCA_CLI_COMMAND = launcher env.ORCA_CLI_COMMAND = launcher } else { console.warn( "[structured-session] This app's CLI launcher did not resolve; the session's child has no ORCA_CLI_COMMAND." ) } + return set }