diff --git a/config/packaged-runtime-node-modules.cjs b/config/packaged-runtime-node-modules.cjs index c88d70350eb0..0cdcb4faf1ac 100644 --- a/config/packaged-runtime-node-modules.cjs +++ b/config/packaged-runtime-node-modules.cjs @@ -28,6 +28,8 @@ const PACKAGED_RUNTIME_PACKAGE_ROOTS = [ // serve-sim (for CLI JS entry + closure + state/middleware + to make packaged require('serve-sim') + its internal relatives work; mirrors other runtime JS like ws/yaml/zod. Natives/dylibs still via extraResources + the node_modules/serve-sim copy in resources from builder. Client if added too. 'serve-sim', 'qrcode', + // smol-toml: Codex config.toml reads/validation in main and the CLI. + 'smol-toml', 'ssh2', 'tweetnacl', 'ws', diff --git a/config/scripts/relay-codex-config-toml-bundle.test.mjs b/config/scripts/relay-codex-config-toml-bundle.test.mjs new file mode 100644 index 000000000000..6da3d65e03e9 --- /dev/null +++ b/config/scripts/relay-codex-config-toml-bundle.test.mjs @@ -0,0 +1,89 @@ +import { build } from 'esbuild' +import { mkdirSync, mkdtempSync, readFileSync, realpathSync, rmSync, writeFileSync } from 'node:fs' +import { createRequire } from 'node:module' +import { tmpdir } from 'node:os' +import { join, resolve } from 'node:path' +import { parse } from 'smol-toml' +import { afterAll, beforeAll, describe, expect, it } from 'vitest' + +// Why: relay hosts run the esbuild bundle with no node_modules, so the Codex config +// parser must be inlined; a runtime-resolved require only fails on a real SSH host. +let root +let applyRelayAgentWorkspaceTrust + +beforeAll(async () => { + root = realpathSync.native(mkdtempSync(join(tmpdir(), 'orca-relay-codex-toml-'))) + const outfile = join(root, 'bundle', 'agent-workspace-trust-spawn.js') + // Same options as build-relay.mjs uses for relay.js. + await build({ + entryPoints: [resolve('src/relay/agent-workspace-trust-spawn.ts')], + outfile, + bundle: true, + platform: 'node', + target: 'node18', + format: 'cjs', + external: ['node-pty', '@parcel/watcher', 'electron'], + logLevel: 'silent' + }) + ;({ applyRelayAgentWorkspaceTrust } = createRequire(outfile)(outfile)) +}) + +afterAll(() => { + rmSync(root, { recursive: true, force: true }) +}) + +describe('relay bundle Codex trust write', () => { + it('loads outside any node_modules and edits a quoted ["projects"."…"] table in place', async () => { + const codexHome = join(root, 'codex-home') + const workspace = join(root, 'wt') + mkdirSync(codexHome) + mkdirSync(workspace) + const configPath = join(codexHome, 'config.toml') + writeFileSync(configPath, `["projects"."${workspace}"]\ntrust_level = "untrusted"\n`) + + await applyRelayAgentWorkspaceTrust( + { workspacePath: workspace }, + 'codex', + { HOME: root, CODEX_HOME: codexHome }, + { wslShell: false } + ) + + expect(readFileSync(configPath, 'utf-8')).toBe( + `["projects"."${workspace}"]\ntrust_level = "trusted"\n` + ) + }) + + it('repairs a duplicate an older Orca wrote using only Node 18 array methods', async () => { + const codexHome = join(root, 'codex-home-node18') + const workspace = join(root, 'wt-node18') + mkdirSync(codexHome) + mkdirSync(workspace) + const configPath = join(codexHome, 'config.toml') + writeFileSync( + configPath, + `["projects"."${workspace}"]\ntrust_level = "trusted"\n\n[projects."${workspace}"]\ntrust_level = "trusted"\n` + ) + // Why: SSH hosts may run the relay on Node 18, which lacks the Node 20 copy-array methods. + const node20Methods = ['toReversed', 'toSorted', 'toSpliced', 'with'] + const saved = node20Methods.map((name) => [name, Array.prototype[name]]) + for (const name of node20Methods) { + delete Array.prototype[name] + } + try { + await applyRelayAgentWorkspaceTrust( + { workspacePath: workspace }, + 'codex', + { HOME: root, CODEX_HOME: codexHome }, + { wslShell: false } + ) + } finally { + for (const [name, method] of saved) { + Array.prototype[name] = method + } + } + + const repaired = readFileSync(configPath, 'utf-8') + expect(parse(repaired).projects[workspace].trust_level).toBe('trusted') + expect(repaired.match(/trust_level/g)).toHaveLength(1) + }) +}) diff --git a/config/tsconfig.cli.json b/config/tsconfig.cli.json index b2cad728fd9e..cd9aa3e465c7 100644 --- a/config/tsconfig.cli.json +++ b/config/tsconfig.cli.json @@ -117,7 +117,13 @@ "../src/main/codex/codex-app-server-record-reader.ts", "../src/main/codex/codex-app-server-session.ts", "../src/main/codex/codex-config-mirror.ts", + "../src/main/codex/codex-config-legacy-shared-home-mirror.ts", "../src/main/codex/codex-config-mirror-merge.ts", + "../src/main/codex/codex-config-mirror-source.ts", + "../src/main/codex/codex-config-toml-checked-edit.ts", + "../src/main/codex/codex-config-toml-document.ts", + "../src/main/codex/codex-config-toml-repair.ts", + "../src/main/codex/codex-config-toml-structure.ts", "../src/main/codex/codex-config-path-reference-rewrite.ts", "../src/main/codex/codex-config-settings-preservation.ts", "../src/main/codex/codex-config-settings-removal.ts", @@ -153,6 +159,7 @@ "../src/main/codex/codex-real-home-hook-sweep.ts", "../src/main/codex/codex-real-home-hook-withdrawal.ts", "../src/main/codex/codex-real-home-hooks-json.ts", + "../src/main/codex/codex-retired-hook-sweep-trust.ts", "../src/main/codex/codex-state-db.ts", "../src/main/codex/codex-trust-identity.ts", "../src/main/codex/codex-trust-config-mutation-queue.ts", diff --git a/package.json b/package.json index 0b41ed3980ac..1eea5f8797a1 100644 --- a/package.json +++ b/package.json @@ -193,6 +193,7 @@ "react-i18next": "17.0.15", "serve-sim": "0.1.40", "sherpa-onnx": "1.12.37", + "smol-toml": "1.8.0", "ssh2": "^1.17.0", "tldts": "7.4.14", "tweetnacl": "^1.0.3", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 8b5c4b890d44..e07623f97394 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -181,6 +181,9 @@ importers: sherpa-onnx: specifier: 1.12.37 version: 1.12.37 + smol-toml: + specifier: 1.8.0 + version: 1.8.0 ssh2: specifier: ^1.17.0 version: 1.17.0 diff --git a/src/main/agent-trust-presets.test.ts b/src/main/agent-trust-presets.test.ts index e956494fc763..ce790785aced 100644 --- a/src/main/agent-trust-presets.test.ts +++ b/src/main/agent-trust-presets.test.ts @@ -392,6 +392,38 @@ describe('markCodexProjectTrusted', () => { } }) + // Why both orders: the lock order writes the managed home first, so each home must survive the other refusing. + it.each([ + ['the managed home', '~/.codex', 'system'], + ['~/.codex', 'the managed home', 'runtime'] + ] as const)('still trusts %s when %s cannot be parsed', async (_trusted, _broken, brokenHome) => { + const workspace = mkdtempSync(join(tmpdir(), 'orca-codex-ws-')) + try { + const homes = { + system: join(testState.fakeHomeDir, '.codex'), + runtime: join(testState.userDataDir, 'codex-runtime-home', 'home') + } + const trustedHome = brokenHome === 'system' ? homes.runtime : homes.system + mkdirSync(homes[brokenHome], { recursive: true }) + const brokenConfig = 'model = "o3"\nmodel_reasoning_effort = \n' + writeFileSync(join(homes[brokenHome], 'config.toml'), brokenConfig, 'utf-8') + + await expect( + markCodexProjectTrusted(workspace, getLocalCodexTrustConfigFiles(testState.fakeHomeDir)) + ).rejects.toMatchObject({ + name: 'CodexConfigTomlEditRefusedError', + reason: 'input-invalid' + }) + + expect(readFileSync(join(homes[brokenHome], 'config.toml'), 'utf-8')).toBe(brokenConfig) + expect(readFileSync(join(trustedHome, 'config.toml'), 'utf-8')).toContain( + `[projects."${escapeTomlBasicString(realpathSync.native(workspace))}"]` + ) + } finally { + rmSync(workspace, { recursive: true, force: true }) + } + }) + it('preserves existing config keys and updates an existing project block', async () => { const workspace = mkdtempSync(join(tmpdir(), 'orca-codex-ws-')) const realpath = realpathSync.native(workspace) diff --git a/src/main/agent-trust-presets.ts b/src/main/agent-trust-presets.ts index 2d52ee005e4a..de6d4d231fb1 100644 --- a/src/main/agent-trust-presets.ts +++ b/src/main/agent-trust-presets.ts @@ -176,8 +176,23 @@ export function markCodexProjectTrusted( const write = configFiles.reduceRight<() => Promise>( (inner, configFile) => () => runExclusivelyForCodexTrustConfig(configFile, inner), async () => { + // Why: a config.toml Orca refuses to edit must not also leave Codex's other homes untrusted. + const failures: unknown[] = [] for (const configFile of configFiles) { - upsertProjectTrustLevel(configFile, absPath, 'trusted') + try { + upsertProjectTrustLevel(configFile, absPath, 'trusted') + } catch (error) { + failures.push(error) + } + } + if (failures.length === 1) { + throw failures[0] + } + if (failures.length > 1) { + throw new AggregateError( + failures, + 'Orca could not mark the project trusted in several Codex homes' + ) } } ) diff --git a/src/main/codex/codex-config-legacy-shared-home-mirror.ts b/src/main/codex/codex-config-legacy-shared-home-mirror.ts new file mode 100644 index 000000000000..b6200fafd44e --- /dev/null +++ b/src/main/codex/codex-config-legacy-shared-home-mirror.ts @@ -0,0 +1,80 @@ +import { join } from 'node:path' +import { + recoverInterruptedGuardedFileOperation, + writeFileAtomicallyIfUnchanged +} from '../codex-accounts/fs-utils' +import { observeAgentStateFile } from './codex-path-observation' +import { getOrcaManagedCodexHomePath, getSystemCodexHomePath } from './codex-home-paths' +import type { CodexSettingsPromotionHomes } from './config-settings-promotion' +import { applyCodexDaemonSocketGuard } from './codex-daemon-socket-path-guard' +import { refuseUnreadableCodexConfigResult } from './codex-config-toml-checked-edit' +import { mergeSystemCodexConfigIntoRuntime } from './codex-config-mirror-merge' +import { + prepareSystemConfigForFreshRuntimeMirror, + prepareSystemConfigForRuntimeMirror, + resolveCodexConfigMirrorSourceDirectory +} from './codex-config-mirror-source' + +/** + * Refreshes the retired shared home for PTYs that survived real-home rollout. + * + * This is deliberately one-way: a retained PTY may hold pre-rollout settings, + * so treating that home as a promotion source could overwrite the live config. + */ +export function syncSystemConfigIntoLegacySharedCodexHome( + homes: CodexSettingsPromotionHomes = { + runtimeHomePath: getOrcaManagedCodexHomePath(), + systemHomePath: getSystemCodexHomePath() + } +): void { + const systemConfigPath = join(homes.systemHomePath, 'config.toml') + const runtimeConfigPath = join(homes.runtimeHomePath, 'config.toml') + recoverInterruptedGuardedFileOperation(runtimeConfigPath) + const systemConfigObservation = observeAgentStateFile(systemConfigPath) + if (systemConfigObservation.kind === 'indeterminate') { + throw systemConfigObservation.error + } + const rawSystemConfig = + systemConfigObservation.kind === 'present' ? systemConfigObservation.value : '' + const runtimeConfigObservation = observeAgentStateFile(runtimeConfigPath) + if (runtimeConfigObservation.kind === 'indeterminate') { + throw runtimeConfigObservation.error + } + const runtimeConfigBeforeMirror = + runtimeConfigObservation.kind === 'present' ? runtimeConfigObservation.value : null + // Why: a missing cloud-synced source is not proof the user cleared config. + let mirroredRuntimeConfig = runtimeConfigBeforeMirror ?? '' + if (rawSystemConfig.trim() !== '') { + const sourceConfigDir = resolveCodexConfigMirrorSourceDirectory(homes.systemHomePath) + // The retired home has no ownership baseline; its entire MCP root stays canonical. + mirroredRuntimeConfig = + runtimeConfigBeforeMirror !== null + ? mergeSystemCodexConfigIntoRuntime( + runtimeConfigBeforeMirror, + prepareSystemConfigForRuntimeMirror(rawSystemConfig, sourceConfigDir), + sourceConfigDir, + new Set(), + true + ) + : prepareSystemConfigForFreshRuntimeMirror(rawSystemConfig, sourceConfigDir) + } + // Why: retained pre-rollout panes still use this home, so a refresh must keep the daemon guard. + const nextRuntimeConfig = applyCodexDaemonSocketGuard( + mirroredRuntimeConfig, + homes.runtimeHomePath + ) + if ( + (runtimeConfigBeforeMirror ?? '') === nextRuntimeConfig || + refuseUnreadableCodexConfigResult({ + configPath: runtimeConfigPath, + result: nextRuntimeConfig, + inputs: [rawSystemConfig, runtimeConfigBeforeMirror], + context: 'Skipped mirroring the Codex config into a managed home' + }) + ) { + return + } + // Why: stage first, then compare immediately before replace so a retained + // Codex trust write during mirror preparation wins. + writeFileAtomicallyIfUnchanged(runtimeConfigPath, runtimeConfigBeforeMirror, nextRuntimeConfig) +} diff --git a/src/main/codex/codex-config-mirror-duplicate-repair.test.ts b/src/main/codex/codex-config-mirror-duplicate-repair.test.ts new file mode 100644 index 000000000000..97606a13e45c --- /dev/null +++ b/src/main/codex/codex-config-mirror-duplicate-repair.test.ts @@ -0,0 +1,356 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { mkdtempSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import type * as NodeOs from 'node:os' +import { join } from 'node:path' +import { parse } from 'smol-toml' + +// Why: temp homes exceed sun_path on macOS but not on Linux; keep asserted config bytes host-independent. +vi.mock('./codex-daemon-socket-path-guard', async (importOriginal) => ({ + ...(await importOriginal()), + applyCodexDaemonSocketGuard: (config: string) => config +})) + +const { getPathMock, homedirMock } = vi.hoisted(() => ({ + getPathMock: vi.fn<(name: string) => string>(), + homedirMock: vi.fn<() => string>() +})) + +vi.mock('electron', () => ({ + app: { + getPath: getPathMock + } +})) + +vi.mock('node:os', async () => { + const actual = await vi.importActual('node:os') + return { + ...actual, + homedir: homedirMock + } +}) + +import { + syncSystemConfigIntoLegacySharedCodexHome, + syncSystemConfigIntoManagedCodexHome +} from './codex-config-mirror' + +let fakeHomeDir: string +let userDataDir: string +let previousUserDataPath: string | undefined + +function getSystemCodexHomePath(): string { + return join(fakeHomeDir, '.codex') +} + +function getSystemConfigPath(): string { + return join(getSystemCodexHomePath(), 'config.toml') +} + +function getRuntimeConfigPath(): string { + return join(userDataDir, 'codex-runtime-home', 'home', 'config.toml') +} + +beforeEach(() => { + fakeHomeDir = mkdtempSync(join(tmpdir(), 'orca-codex-config-home-')) + userDataDir = mkdtempSync(join(tmpdir(), 'orca-codex-config-user-data-')) + previousUserDataPath = process.env.ORCA_USER_DATA_PATH + process.env.ORCA_USER_DATA_PATH = userDataDir + homedirMock.mockReturnValue(fakeHomeDir) + getPathMock.mockImplementation((name: string) => { + if (name === 'userData') { + return userDataDir + } + throw new Error(`unexpected app.getPath(${name})`) + }) + mkdirSync(getSystemCodexHomePath(), { recursive: true }) +}) + +afterEach(() => { + rmSync(fakeHomeDir, { recursive: true, force: true }) + rmSync(userDataDir, { recursive: true, force: true }) + if (previousUserDataPath === undefined) { + delete process.env.ORCA_USER_DATA_PATH + } else { + process.env.ORCA_USER_DATA_PATH = previousUserDataPath + } + vi.clearAllMocks() +}) + +describe('managed-home mirror never writes a config Codex cannot read (#22592)', () => { + it('collapses both project spellings and a doubled [hooks.state] in an existing managed home (#22592)', () => { + writeFileSync( + getSystemConfigPath(), + 'model = "m"\n\n["projects"."/repo"]\ntrust_level = "trusted"\n', + 'utf-8' + ) + mkdirSync(join(userDataDir, 'codex-runtime-home', 'home'), { recursive: true }) + writeFileSync( + getRuntimeConfigPath(), + [ + 'model = "m"', + '', + '["projects"."/repo"]', + 'trust_level = "trusted"', + '', + '[projects."/repo"]', + 'trust_level = "trusted"', + '', + '[hooks.state]', + '', + '[hooks.state]', + '', + '[hooks.state."/rt/hooks.json:stop:0:0"]', + 'enabled = true', + 'trusted_hash = "sha256:x"', + '' + ].join('\n'), + 'utf-8' + ) + + syncSystemConfigIntoManagedCodexHome() + + const runtimeConfig = readFileSync(getRuntimeConfigPath(), 'utf-8') + expect(parse(runtimeConfig)).toEqual({ + model: 'm', + projects: { '/repo': { trust_level: 'trusted' } }, + hooks: { state: { '/rt/hooks.json:stop:0:0': { enabled: true, trusted_hash: 'sha256:x' } } } + }) + }) + + it('drops a runtime project table that ~/.codex defines inline', () => { + writeFileSync( + getSystemConfigPath(), + '[projects]\n"/repo" = { trust_level = "untrusted" }\n', + 'utf-8' + ) + mkdirSync(join(userDataDir, 'codex-runtime-home', 'home'), { recursive: true }) + writeFileSync(getRuntimeConfigPath(), '[projects."/repo"]\ntrust_level = "trusted"\n', 'utf-8') + + syncSystemConfigIntoManagedCodexHome() + + expect(parse(readFileSync(getRuntimeConfigPath(), 'utf-8'))).toEqual({ + projects: { '/repo': { trust_level: 'untrusted' } } + }) + }) + + it('keeps managed trust for a project ~/.codex defines only through a sub-table', () => { + writeFileSync( + getSystemConfigPath(), + 'model = "m"\n\n[projects."/repo".extra]\nx = 1\n', + 'utf-8' + ) + mkdirSync(join(userDataDir, 'codex-runtime-home', 'home'), { recursive: true }) + writeFileSync( + getRuntimeConfigPath(), + 'model = "m"\n\n[projects."/repo".extra]\nx = 1\n\n[projects."/repo"]\ntrust_level = "trusted"\n', + 'utf-8' + ) + + syncSystemConfigIntoManagedCodexHome() + + expect(parse(readFileSync(getRuntimeConfigPath(), 'utf-8'))).toEqual({ + model: 'm', + projects: { '/repo': { extra: { x: 1 }, trust_level: 'trusted' } } + }) + }) + + it.each([ + ['dotted keys under [projects]', '[projects]\n"/repo".trust_level = "untrusted"\n'], + ['root dotted keys', 'projects."/repo".trust_level = "untrusted"\n'] + ])( + 'drops managed trust for a project ~/.codex defines through a sub-table and %s', + (_placement, dotted) => { + const systemConfig = `model = "m"\n${dotted}\n[projects."/repo".extra]\nx = 1\n` + writeFileSync(getSystemConfigPath(), systemConfig, 'utf-8') + mkdirSync(join(userDataDir, 'codex-runtime-home', 'home'), { recursive: true }) + writeFileSync( + getRuntimeConfigPath(), + 'model = "m"\n\n[projects."/repo"]\ntrust_level = "trusted"\n', + 'utf-8' + ) + + syncSystemConfigIntoManagedCodexHome() + + expect(parse(readFileSync(getRuntimeConfigPath(), 'utf-8'))).toEqual({ + model: 'm', + projects: { '/repo': { trust_level: 'untrusted', extra: { x: 1 } } } + }) + } + ) + + it.each([ + ['[projects]', '[projects]\n"/repo".extra.q = 1\n'], + ['the root', 'projects."/repo".extra.q = 1\n'] + ])( + 'drops managed trust when a dotted key under %s creates the parent of a deeper sub-table', + (_placement, dotted) => { + writeFileSync( + getSystemConfigPath(), + `model = "m"\n${dotted}\n[projects."/repo".extra.deep]\nz = 1\n`, + 'utf-8' + ) + mkdirSync(join(userDataDir, 'codex-runtime-home', 'home'), { recursive: true }) + writeFileSync( + getRuntimeConfigPath(), + 'model = "m"\n\n[projects."/repo"]\ntrust_level = "trusted"\n', + 'utf-8' + ) + + syncSystemConfigIntoManagedCodexHome() + + expect(parse(readFileSync(getRuntimeConfigPath(), 'utf-8'))).toEqual({ + model: 'm', + projects: { '/repo': { extra: { q: 1, deep: { z: 1 } } } } + }) + } + ) + + it('keeps managed trust for a project ~/.codex defines only through a deeper sub-table', () => { + writeFileSync( + getSystemConfigPath(), + 'model = "m"\n\n[projects."/repo".extra.deep]\nz = 1\n', + 'utf-8' + ) + mkdirSync(join(userDataDir, 'codex-runtime-home', 'home'), { recursive: true }) + writeFileSync( + getRuntimeConfigPath(), + 'model = "m"\n\n[projects."/repo"]\ntrust_level = "trusted"\n', + 'utf-8' + ) + + syncSystemConfigIntoManagedCodexHome() + + expect(parse(readFileSync(getRuntimeConfigPath(), 'utf-8'))).toEqual({ + model: 'm', + projects: { '/repo': { extra: { deep: { z: 1 } }, trust_level: 'trusted' } } + }) + }) + + it('collapses Orca\u2019s own duplicate hook tables, so managed-only trust survives', () => { + writeFileSync(getSystemConfigPath(), 'model = "m"\n', 'utf-8') + mkdirSync(join(userDataDir, 'codex-runtime-home', 'home'), { recursive: true }) + const key = '/rt/hooks.json:stop:0:0' + writeFileSync( + getRuntimeConfigPath(), + [ + 'model = "m"', + '', + '[projects."/answered-in-orca"]', + 'trust_level = "trusted"', + '', + `[hooks.state."${key}"]`, + 'trusted_hash = "sha256:a"', + '', + `[hooks.state."${key}"]`, + 'trusted_hash = "sha256:b"', + '' + ].join('\n'), + 'utf-8' + ) + + syncSystemConfigIntoManagedCodexHome() + + expect(parse(readFileSync(getRuntimeConfigPath(), 'utf-8'))).toEqual({ + model: 'm', + projects: { '/answered-in-orca': { trust_level: 'trusted' } }, + hooks: { state: { [key]: { trusted_hash: 'sha256:a' } } } + }) + }) +}) + +// Why: handling a ~/.codex the user broke by hand is a separate change; until +// then the mirrors must do exactly what they did before the checked writer. +describe('a ~/.codex the user broke by hand is mirrored as before', () => { + const brokenSource = 'model = "A"\n\n[mcp_servers.keep]\ncommand = "k"\nbroken = \n' + const managedOnly = + '\n[mcp_servers.mine]\ncommand = "m"\n\n[projects."/sub"]\ntrust_level = "trusted"\n' + const managed = `model = "A"\n\n[mcp_servers.keep]\ncommand = "k"\n${managedOnly}` + + function makeHomes(): { systemHomePath: string; runtimeHomePath: string } { + const homes = { + systemHomePath: join(fakeHomeDir, 'sys'), + runtimeHomePath: join(fakeHomeDir, 'rt') + } + mkdirSync(homes.systemHomePath, { recursive: true }) + mkdirSync(homes.runtimeHomePath, { recursive: true }) + writeFileSync(join(homes.systemHomePath, 'config.toml'), brokenSource) + return homes + } + + it.each([ + [ + 'managed home', + syncSystemConfigIntoManagedCodexHome, + `model = "A"\n\n[mcp_servers.keep]\ncommand = "k"\nbroken =\n${managedOnly}` + ], + [ + 'legacy shared home', + syncSystemConfigIntoLegacySharedCodexHome, + 'model = "A"\n\n[mcp_servers.keep]\ncommand = "k"\nbroken =\n\n[projects."/sub"]\ntrust_level = "trusted"\n' + ] + ])('merges it into a %s holding managed-only state', (_label, sync, expected) => { + const homes = makeHomes() + const rtPath = join(homes.runtimeHomePath, 'config.toml') + writeFileSync(rtPath, managed) + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + sync(homes) + sync(homes) + + expect(readFileSync(rtPath, 'utf8')).toBe(expected) + expect(warn).not.toHaveBeenCalled() + warn.mockRestore() + }) + + it.each([ + ['managed home', syncSystemConfigIntoManagedCodexHome], + ['legacy shared home', syncSystemConfigIntoLegacySharedCodexHome] + ])('seeds a fresh %s from it', (_label, sync) => { + const homes = makeHomes() + + sync(homes) + + expect(readFileSync(join(homes.runtimeHomePath, 'config.toml'), 'utf8')).toBe( + 'model = "A"\n\n[mcp_servers.keep]\ncommand = "k"\nbroken =\n' + ) + }) + + it('holds an in-Codex change back until ~/.codex parses, then promotes it', () => { + const homes = makeHomes() + const sysPath = join(homes.systemHomePath, 'config.toml') + const rtPath = join(homes.runtimeHomePath, 'config.toml') + const good = 'model = "A"\n\n[mcp_servers.keep]\ncommand = "k"\n' + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + writeFileSync(sysPath, good) + syncSystemConfigIntoManagedCodexHome(homes) + // Codex inside Orca: /model B and a trust answer; then the user breaks ~/.codex. + writeFileSync( + rtPath, + `${readFileSync(rtPath, 'utf8').replace('model = "A"', 'model = "B"')}${managedOnly}` + ) + const runtimeBeforeBreak = readFileSync(rtPath, 'utf8') + writeFileSync(sysPath, brokenSource) + syncSystemConfigIntoManagedCodexHome(homes) + syncSystemConfigIntoManagedCodexHome(homes) + + expect(readFileSync(sysPath, 'utf8')).toBe(brokenSource) + expect(readFileSync(rtPath, 'utf8')).toBe(runtimeBeforeBreak) + expect( + warn.mock.calls.flat().filter((line) => String(line).includes('Skipped promoting')) + ).toHaveLength(1) + + writeFileSync(sysPath, good) + syncSystemConfigIntoManagedCodexHome(homes) + expect(parse(readFileSync(sysPath, 'utf8')).model).toBe('B') + expect(parse(readFileSync(rtPath, 'utf8'))).toMatchObject({ + model: 'B', + mcp_servers: { mine: { command: 'm' } }, + projects: { '/sub': { trust_level: 'trusted' } } + }) + } finally { + warn.mockRestore() + } + }) +}) diff --git a/src/main/codex/codex-config-mirror-hook-trust-carry.test.ts b/src/main/codex/codex-config-mirror-hook-trust-carry.test.ts index 3ffd963a9669..4300debb2a8a 100644 --- a/src/main/codex/codex-config-mirror-hook-trust-carry.test.ts +++ b/src/main/codex/codex-config-mirror-hook-trust-carry.test.ts @@ -255,7 +255,8 @@ describe('carrying plain-Codex hook trust into the managed Codex home', () => { const runtimeConfig = mirror() expect(countHookTrustTables(runtimeConfig, PLUGIN_KEY)).toBe(1) - expect(runtimeConfig).toContain('sha256:first') + // Why: the bare copy has the shape older Orca wrote, so the repair keeps Codex's spelling. + expect(runtimeConfig).toContain('sha256:second') expectNoDuplicateTables(runtimeConfig) }) diff --git a/src/main/codex/codex-config-mirror-merge.ts b/src/main/codex/codex-config-mirror-merge.ts index ffdaa6b712d5..902812144cd1 100644 --- a/src/main/codex/codex-config-mirror-merge.ts +++ b/src/main/codex/codex-config-mirror-merge.ts @@ -1,4 +1,7 @@ import { readMcpServerTomlOwnership } from './config-toml-mcp-servers' +import { getTomlTable, parseCodexConfigToml } from './codex-config-toml-document' +import { repairUnparseableCodexConfig } from './codex-config-toml-repair' +import { normalizeCodexProjectPathForLookup } from './config-toml-trust' import { deduplicateProjectTomlSections, getHookTrustTomlSectionKeys, @@ -12,6 +15,8 @@ import { joinTomlBlocks, stripRuntimeOwnedTomlSections } from './config-toml-runtime-owned-sections' +import { parseTomlTableHeaderPath } from './config-toml-key-path' +import { mayNameTomlKeys, parseProjectTomlHeaderPath } from './config-toml-syntax' /** Ordinary settings and shared hook trust from ~/.codex plus the tables the managed home owns. */ export function mergeSystemCodexConfigIntoRuntime( @@ -21,7 +26,16 @@ export function mergeSystemCodexConfigIntoRuntime( mirroredMcpServerNames: ReadonlySet = new Set(), mirroredMcpServerRoot = false ): string { - const runtimeSections = deduplicateProjectTomlSections(getTomlSections(runtimeConfig)) + const systemInlineProjects = getProjectsDefinedOutsideTables(systemConfig) + const runtimeSections = deduplicateProjectTomlSections( + getTomlSections(repairUnparseableCodexConfig(runtimeConfig)) + ).filter( + // Why: ~/.codex defines this project inline or with dotted keys, so a + // runtime `[projects."…"]` table beside it would redefine it. + (section) => + !isRuntimeProjectTomlSection(section.header) || + !systemInlineProjects.has(getTomlSectionHeaderKey(section.header)) + ) const runtimeProjectHeaders = new Set( runtimeSections .filter((section) => isRuntimeProjectTomlSection(section.header)) @@ -76,3 +90,60 @@ export function mergeSystemCodexConfigIntoRuntime( .map((section) => section.block) ]) } + +/** Project keys ~/.codex defines inline or with dotted keys rather than as `[projects."…"]` tables. */ +function getProjectsDefinedOutsideTables(config: string): ReadonlySet { + const parsed = parseCodexConfigToml(config) + const projects = parsed.ok ? getTomlTable(parsed.table.projects) : null + if (!projects) { + return new Set() + } + // Header paths below each project, JSON-encoded; `[]` is the project's own header. + const headerPaths = new Map>() + for (const { header } of getTomlSections(config)) { + if (!mayNameTomlKeys(header, ['projects'])) { + continue + } + const projectPath = parseProjectTomlHeaderPath(header) + const [root, subProjectPath, ...rest] = + projectPath === null + ? (parseTomlTableHeaderPath(header)?.segments ?? []) + : ['projects', projectPath] + if (root === 'projects' && subProjectPath !== undefined) { + const key = projectKey(subProjectPath) + headerPaths.set(key, (headerPaths.get(key) ?? new Set()).add(JSON.stringify(rest))) + } + } + return new Set( + Object.entries(projects) + .filter( + ([projectPath, value]) => + !isDefinedByHeaders(value, [], headerPaths.get(projectKey(projectPath)) ?? new Set()) + ) + .map(([projectPath]) => projectKey(projectPath)) + ) +} + +// Why: `[projects."/a".extra]` defines `/a` as a table only when no dotted or +// inline key sets a value anywhere under `/a` outside a header. +function isDefinedByHeaders( + value: unknown, + path: readonly string[], + headerPaths: ReadonlySet +): boolean { + if (headerPaths.has(JSON.stringify(path))) { + return true + } + const table = getTomlTable(value) + return ( + table !== null && + Object.keys(table).length > 0 && + Object.entries(table).every(([name, child]) => + isDefinedByHeaders(child, [...path, name], headerPaths) + ) + ) +} + +function projectKey(projectPath: string): string { + return `project:${normalizeCodexProjectPathForLookup(projectPath)}` +} diff --git a/src/main/codex/codex-config-mirror-source.ts b/src/main/codex/codex-config-mirror-source.ts new file mode 100644 index 000000000000..932cd5df3a1a --- /dev/null +++ b/src/main/codex/codex-config-mirror-source.ts @@ -0,0 +1,43 @@ +import { dirname, join } from 'node:path' +import { parseWslUncPath } from '../../shared/wsl-paths' +import { rewriteRelativePathConfigValues } from './codex-config-path-reference-rewrite' +import { normalizeDeprecatedCodexHookFeatureFlag } from './config-toml-deprecated-hook-flag' +import { repairUnparseableCodexConfig } from './codex-config-toml-repair' +import { stripRuntimeOwnedTomlSections } from './config-toml-runtime-owned-sections' + +// Why: how ~/.codex is prepared before it is mirrored into an Orca-owned Codex home. + +export function resolveCodexConfigMirrorSourceDirectory( + systemHomePath: string, + systemConfigDir?: string +): string { + return ( + systemConfigDir ?? + parseWslUncPath(systemHomePath)?.linuxPath ?? + dirname(join(systemHomePath, 'config.toml')) + ) +} + +export function prepareSystemConfigForRuntimeMirror( + config: string, + systemConfigDir: string +): string { + return rewriteRelativePathConfigValues( + normalizeDeprecatedCodexHookFeatureFlag(repairUnparseableCodexConfig(config)), + systemConfigDir + ) +} + +// Why: install re-keys ~/.codex user-hook trust; plugin and project hook trust carries as-is. +// Also seeds WSL runtime homes, where systemConfigDir must be the Linux-side +// ~/.codex the config resolves against inside the distro. +export function prepareSystemConfigForFreshRuntimeMirror( + config: string, + systemConfigDir: string +): string { + return stripRuntimeOwnedTomlSections( + prepareSystemConfigForRuntimeMirror(config, systemConfigDir), + new Set(), + { systemHomeDir: systemConfigDir, runtimeHookTrustKeys: new Set() } + ) +} diff --git a/src/main/codex/codex-config-mirror.test.ts b/src/main/codex/codex-config-mirror.test.ts index 0de545e43e10..51fe6001f768 100644 --- a/src/main/codex/codex-config-mirror.test.ts +++ b/src/main/codex/codex-config-mirror.test.ts @@ -699,7 +699,8 @@ describe('syncSystemConfigIntoManagedCodexHome', () => { const runtimeConfig = readFileSync(getRuntimeConfigPath(), 'utf-8') expect(runtimeConfig.match(/\[projects\./g)).toHaveLength(1) - expect(runtimeConfig).toContain('[projects."c:\\\\gemini_etl"]') + // Why (#22592): the basic-string copy has the exact shape Orca appended, so the other one stays. + expect(runtimeConfig).toContain("[projects.'c:\\gemini_etl']") }) it('keeps untrusted when self-healing duplicate project tables', () => { diff --git a/src/main/codex/codex-config-mirror.ts b/src/main/codex/codex-config-mirror.ts index a97a8b9644c8..4d7e3748a97b 100644 --- a/src/main/codex/codex-config-mirror.ts +++ b/src/main/codex/codex-config-mirror.ts @@ -1,15 +1,8 @@ import { readMcpServerTomlOwnership } from './config-toml-mcp-servers' -import { dirname, join } from 'node:path' +import { join } from 'node:path' import { observeAgentStateFile } from './codex-path-observation' -import { - recoverInterruptedGuardedFileOperation, - writeFileAtomically, - writeFileAtomicallyIfUnchanged -} from '../codex-accounts/fs-utils' +import { writeFileAtomically, writeFileAtomicallyIfUnchanged } from '../codex-accounts/fs-utils' import { getOrcaManagedCodexHomePath, getSystemCodexHomePath } from './codex-home-paths' -import { rewriteRelativePathConfigValues } from './codex-config-path-reference-rewrite' -import { normalizeDeprecatedCodexHookFeatureFlag } from './config-toml-deprecated-hook-flag' -import { parseWslUncPath } from '../../shared/wsl-paths' import { promoteCodexRuntimeSettingsToSystem, snapshotCodexRuntimeSettingsBaseline, @@ -20,8 +13,19 @@ import { readCodexSettingsBaseline } from './config-settings-baseline' import { getCodexConfigSyncStatus, reportCodexConfigSyncOutcome } from './config-sync-stall' import { preserveRuntimeConflictValues } from './codex-config-settings-preservation' import { applyCodexDaemonSocketGuard } from './codex-daemon-socket-path-guard' -import { stripRuntimeOwnedTomlSections } from './config-toml-runtime-owned-sections' +import { refuseUnreadableCodexConfigResult } from './codex-config-toml-checked-edit' import { mergeSystemCodexConfigIntoRuntime } from './codex-config-mirror-merge' +import { + prepareSystemConfigForFreshRuntimeMirror, + prepareSystemConfigForRuntimeMirror, + resolveCodexConfigMirrorSourceDirectory +} from './codex-config-mirror-source' + +export { syncSystemConfigIntoLegacySharedCodexHome } from './codex-config-legacy-shared-home-mirror' +export { + prepareSystemConfigForFreshRuntimeMirror, + resolveCodexConfigMirrorSourceDirectory +} from './codex-config-mirror-source' export function syncSystemConfigIntoManagedCodexHome( homes: CodexSettingsPromotionHomes = { @@ -68,6 +72,11 @@ function mirrorSystemConfigIntoManagedCodexHome(homes: CodexSettingsPromotionHom reportCodexConfigSyncOutcome(homes.runtimeHomePath, getCodexConfigSyncStatus(homes), error) return false } + if (mirrorResult.status === 'refused-invalid') { + // Why: writing a config Codex cannot parse breaks every managed-home launch; + // keep the last good copy and advance nothing, like an unreadable source. + return false + } if (mirrorResult.status === 'refused-indeterminate') { // Why: no mirror ran, so this must behave exactly like the throwing path // above — surface the reason and advance nothing. Advancing the baseline @@ -126,69 +135,25 @@ export function ensureCodexDaemonSocketGuard(runtimeHomePath: string): void { function writeCodexDaemonSocketGuard(runtimeHomePath: string, runtimeConfig: string | null): void { const guarded = applyCodexDaemonSocketGuard(runtimeConfig ?? '', runtimeHomePath) if (guarded !== (runtimeConfig ?? '')) { - writeFileAtomicallyIfUnchanged(join(runtimeHomePath, 'config.toml'), runtimeConfig, guarded) - } -} - -/** - * Refreshes the retired shared home for PTYs that survived real-home rollout. - * - * This is deliberately one-way: a retained PTY may hold pre-rollout settings, - * so treating that home as a promotion source could overwrite the live config. - */ -export function syncSystemConfigIntoLegacySharedCodexHome( - homes: CodexSettingsPromotionHomes = { - runtimeHomePath: getOrcaManagedCodexHomePath(), - systemHomePath: getSystemCodexHomePath() - } -): void { - const systemConfigPath = join(homes.systemHomePath, 'config.toml') - const runtimeConfigPath = join(homes.runtimeHomePath, 'config.toml') - recoverInterruptedGuardedFileOperation(runtimeConfigPath) - const systemConfigObservation = observeAgentStateFile(systemConfigPath) - if (systemConfigObservation.kind === 'indeterminate') { - throw systemConfigObservation.error - } - const rawSystemConfig = - systemConfigObservation.kind === 'present' ? systemConfigObservation.value : '' - const runtimeConfigObservation = observeAgentStateFile(runtimeConfigPath) - if (runtimeConfigObservation.kind === 'indeterminate') { - throw runtimeConfigObservation.error - } - const runtimeConfigBeforeMirror = - runtimeConfigObservation.kind === 'present' ? runtimeConfigObservation.value : null - // Why: a missing cloud-synced source is not proof the user cleared config. - let mirroredRuntimeConfig = runtimeConfigBeforeMirror ?? '' - if (rawSystemConfig.trim() !== '') { - const sourceConfigDir = resolveCodexConfigMirrorSourceDirectory(homes.systemHomePath) - // The retired home has no ownership baseline; its entire MCP root stays canonical. - mirroredRuntimeConfig = - runtimeConfigBeforeMirror !== null - ? mergeSystemCodexConfigIntoRuntime( - runtimeConfigBeforeMirror, - prepareSystemConfigForRuntimeMirror(rawSystemConfig, sourceConfigDir), - sourceConfigDir, - new Set(), - true - ) - : prepareSystemConfigForFreshRuntimeMirror(rawSystemConfig, sourceConfigDir) - } - // Why: retained pre-rollout panes still use this home, so a refresh must keep the daemon guard. - const nextRuntimeConfig = applyCodexDaemonSocketGuard( - mirroredRuntimeConfig, - homes.runtimeHomePath - ) - if ((runtimeConfigBeforeMirror ?? '') === nextRuntimeConfig) { - return + const runtimeConfigPath = join(runtimeHomePath, 'config.toml') + if ( + refuseUnreadableCodexConfigResult({ + configPath: runtimeConfigPath, + result: guarded, + inputs: [runtimeConfig], + context: 'Skipped writing a managed Codex config' + }) + ) { + return + } + writeFileAtomicallyIfUnchanged(runtimeConfigPath, runtimeConfig, guarded) } - // Why: stage first, then compare immediately before replace so a retained - // Codex trust write during mirror preparation wins. - writeFileAtomicallyIfUnchanged(runtimeConfigPath, runtimeConfigBeforeMirror, nextRuntimeConfig) } type CodexConfigMirrorResult = | { status: 'skipped-missing-source' } | { status: 'refused-indeterminate'; error: unknown } + | { status: 'refused-invalid' } | { status: 'mirrored' preservedConflictKeys: ReadonlySet @@ -242,6 +207,9 @@ function syncSystemConfigIntoManagedCodexHomeUnsafe( prepareSystemConfigForFreshRuntimeMirror(rawSystemConfig, sourceConfigDir), runtimeHomePath ) + if (refuseUnreadableMirrorResult(runtimeConfigPath, freshRuntimeConfig, [rawSystemConfig])) { + return { status: 'refused-invalid' } + } const ownership = readMcpServerTomlOwnership(freshRuntimeConfig) writeFileAtomically(runtimeConfigPath, freshRuntimeConfig) return { @@ -270,6 +238,14 @@ function syncSystemConfigIntoManagedCodexHomeUnsafe( ) const nextRuntimeConfig = applyCodexDaemonSocketGuard(preserved.content, runtimeHomePath) if (nextRuntimeConfig !== runtimeConfig) { + if ( + refuseUnreadableMirrorResult(runtimeConfigPath, nextRuntimeConfig, [ + rawSystemConfig, + runtimeConfig + ]) + ) { + return { status: 'refused-invalid' } + } writeFileAtomically(runtimeConfigPath, nextRuntimeConfig) } return { @@ -280,34 +256,15 @@ function syncSystemConfigIntoManagedCodexHomeUnsafe( } } -export function resolveCodexConfigMirrorSourceDirectory( - systemHomePath: string, - systemConfigDir?: string -): string { - return ( - systemConfigDir ?? - parseWslUncPath(systemHomePath)?.linuxPath ?? - dirname(join(systemHomePath, 'config.toml')) - ) -} - -function prepareSystemConfigForRuntimeMirror(config: string, systemConfigDir: string): string { - return rewriteRelativePathConfigValues( - normalizeDeprecatedCodexHookFeatureFlag(config), - systemConfigDir - ) -} - -// Why: install re-keys ~/.codex user-hook trust; plugin and project hook trust carries as-is. -// Also seeds WSL runtime homes, where systemConfigDir must be the Linux-side -// ~/.codex the config resolves against inside the distro. -export function prepareSystemConfigForFreshRuntimeMirror( - config: string, - systemConfigDir: string -): string { - return stripRuntimeOwnedTomlSections( - prepareSystemConfigForRuntimeMirror(config, systemConfigDir), - new Set(), - { systemHomeDir: systemConfigDir, runtimeHookTrustKeys: new Set() } - ) +function refuseUnreadableMirrorResult( + runtimeConfigPath: string, + result: string, + inputs: readonly (string | null)[] +): boolean { + return refuseUnreadableCodexConfigResult({ + configPath: runtimeConfigPath, + result, + inputs, + context: 'Skipped mirroring the Codex config into a managed home' + }) } diff --git a/src/main/codex/codex-config-toml-checked-edit.test.ts b/src/main/codex/codex-config-toml-checked-edit.test.ts new file mode 100644 index 000000000000..c4d7230c4eaf --- /dev/null +++ b/src/main/codex/codex-config-toml-checked-edit.test.ts @@ -0,0 +1,405 @@ +import { describe, expect, it, vi } from 'vitest' +import { parse } from 'smol-toml' +import { + readHookTrustEntriesFromContent, + removeHookTrustEntriesFromContent, + upsertHookTrustEntriesInContent, + upsertProjectTrustLevelInContent, + type CodexTrustEntry +} from './config-toml-trust' +import { + CodexConfigTomlEditRefusedError, + refuseUnreadableCodexConfigResult, + reportCodexConfigTomlEditRefusal, + reportCodexTrustWriteRefusals +} from './codex-config-toml-checked-edit' +import { getTomlTable, readTomlValueAtPath } from './codex-config-toml-document' +import { setHookTrustEnabledContent } from './config-toml-hook-trust-edit' +import { repairOrcaCodexConfigDuplicates } from './codex-config-toml-repair' +import { prepareSystemConfigForFreshRuntimeMirror } from './codex-config-mirror' +import { getProjectTrustLevel, getTomlSections } from './config-toml-runtime-owned-sections' + +const trust = (content: string, path = '/work/repo', level: 'trusted' | 'untrusted' = 'trusted') => + upsertProjectTrustLevelInContent(content, path, level, { alreadyCanonical: true }) + +function projectsOf(content: string): Record { + const parsed = parse(content, { integersAsBigInt: 'asNeeded' }) + return getTomlTable(parsed.projects) ?? {} +} + +describe('project trust edits find the existing table in any spelling (#22592)', () => { + it.each([ + ['bare projects, basic path', '[projects."/work/repo"]'], + ['quoted projects (Codex desktop form)', '["projects"."/work/repo"]'], + ['literal-quoted segments', "['projects'.'/work/repo']"], + ['spaced dots', '[ projects . "/work/repo" ]'], + ['trailing comment', '[projects."/work/repo"] # added by codex'] + ])('%s', (_name, header) => { + const existing = `model = "gpt-5"\n\n${header}\ntrust_level = "untrusted"\nsandbox = "workspace-write"\n` + const updated = trust(existing) + + expect(updated).toBe(existing.replace('"untrusted"', '"trusted"')) + expect(projectsOf(updated)).toEqual({ + '/work/repo': { trust_level: 'trusted', sandbox: 'workspace-write' } + }) + }) + + it('rewrites a quoted "trust_level" key in place instead of adding a second one', () => { + const existing = '[projects."/work/repo"]\n"trust_level" = "untrusted" # mine\n' + expect(trust(existing)).toBe('[projects."/work/repo"]\ntrust_level = "trusted" # mine\n') + }) + + it('finds an escaped-quote path', () => { + const existing = '["projects"."/work/a \\"b\\""]\ntrust_level = "untrusted"\n' + const updated = trust(existing, '/work/a "b"') + expect(Object.keys(projectsOf(updated))).toEqual(['/work/a "b"']) + }) + + it('leaves an inline projects table that already trusts the folder untouched', () => { + const existing = '[projects]\n"/work/repo" = { trust_level = "trusted" }\n' + expect(trust(existing)).toBe(existing) + }) + + it('refuses to append a second definition beside an inline project table', () => { + const existing = '[projects]\n"/work/repo" = { trust_level = "untrusted" }\n' + expect(() => trust(existing)).toThrow(CodexConfigTomlEditRefusedError) + }) + + it('edits a dotted trust_level key where it is', () => { + const existing = '# top\nprojects."/work/repo".trust_level = "untrusted"\nmodel = "o3"\n' + expect(trust(existing)).toBe( + '# top\nprojects."/work/repo".trust_level = "trusted"\nmodel = "o3"\n' + ) + }) + + it('adds trust as one more dotted key to a project defined by dotted keys', () => { + const existing = '[projects]\n"/work/repo".sandbox = "read-only"\n' + const updated = trust(existing) + expect(updated).toBe( + '[projects]\n"/work/repo".sandbox = "read-only"\n"/work/repo".trust_level = "trusted"\n' + ) + }) + + it('keeps CRLF, drops only the BOM, and preserves comments', () => { + const existing = '# keep me\r\n["projects"."/work/repo"]\r\n# and me\r\nmodel = "o3"\r\n' + const updated = trust(existing) + expect(updated).toBe( + '# keep me\r\n["projects"."/work/repo"]\r\ntrust_level = "trusted"\r\n# and me\r\nmodel = "o3"\r\n' + ) + }) + + it('refuses to edit a file that is invalid for reasons Orca did not cause', () => { + const existing = '[mcp_servers.a]\ncommand = "x"\n[mcp_servers.a]\ncommand = "y"\n' + const error = (() => { + try { + trust(existing) + } catch (caught) { + return caught + } + return null + })() + expect(error).toBeInstanceOf(CodexConfigTomlEditRefusedError) + expect(error).toMatchObject({ reason: 'input-invalid', line: 3 }) + }) + + it('refuses an edit whose result would not parse', () => { + // An inline root `projects` table cannot be extended with a header. + const existing = 'projects = { "/other" = { trust_level = "trusted" } }\n' + expect(() => trust(existing)).toThrow(/would have produced invalid TOML/) + }) +}) + +describe('repair of duplicates earlier Orca builds wrote (#22592)', () => { + it('drops Orca’s appended bare table after Codex’s quoted one', () => { + const broken = [ + '["projects"."/work/repo"]', + 'trust_level = "trusted"', + '', + '[projects."/work/repo"]', + 'trust_level = "trusted"', + '' + ].join('\n') + expect(repairOrcaCodexConfigDuplicates(broken)).toBe( + '["projects"."/work/repo"]\ntrust_level = "trusted"\n' + ) + expect(trust(broken)).toBe('["projects"."/work/repo"]\ntrust_level = "trusted"\n') + }) + + it('keeps the user’s first answer when Orca appended a different one', () => { + const broken = + '["projects"."/w"]\ntrust_level = "untrusted"\n[projects."/w"]\ntrust_level = "trusted"\n' + expect(projectsOf(repairOrcaCodexConfigDuplicates(broken))).toEqual({ + '/w': { trust_level: 'untrusted' } + }) + }) + + it('keeps the user’s table even when Orca’s copy held a trust line it lacks', () => { + const broken = '["projects"."/w"]\nmodel = "o3"\n\n[projects."/w"]\ntrust_level = "trusted"\n' + expect(projectsOf(repairOrcaCodexConfigDuplicates(broken))).toEqual({ '/w': { model: 'o3' } }) + expect(projectsOf(trust(broken, '/w'))).toEqual({ + '/w': { trust_level: 'trusted', model: 'o3' } + }) + }) + + it('drops the bare trust_level Orca inserted beside a quoted one', () => { + const broken = '[projects."/w"]\ntrust_level = "trusted"\n"trust_level" = "untrusted"\n' + expect(repairOrcaCodexConfigDuplicates(broken)).toBe( + '[projects."/w"]\n"trust_level" = "untrusted"\n' + ) + }) + + it('drops a second, empty [hooks.state] header but keeps its comment', () => { + const broken = + '[hooks.state]\n\n[hooks.state."k:stop:0:0"]\nenabled = true\n[hooks.state]\n# note\n' + const repaired = repairOrcaCodexConfigDuplicates(broken) + expect(repaired).toBe('[hooks.state]\n\n[hooks.state."k:stop:0:0"]\nenabled = true\n# note\n') + expect(() => parse(repaired)).not.toThrow() + }) + + it('handles the wrong-order sed workaround (two identical bare headers)', () => { + const broken = + '[projects."/w"]\ntrust_level = "trusted"\nmodel = "o3"\n\n[projects."/w"]\ntrust_level = "trusted"\n' + expect(projectsOf(repairOrcaCodexConfigDuplicates(broken))).toEqual({ + '/w': { trust_level: 'trusted', model: 'o3' } + }) + }) + + it('never deletes a duplicate table that carries the user’s own values', () => { + const broken = '[projects."/w"]\ntrust_level = "trusted"\n[projects."/w"]\nmodel = "o3"\n' + expect(repairOrcaCodexConfigDuplicates(broken)).toBe(broken) + expect(() => trust(broken, '/w')).toThrow(CodexConfigTomlEditRefusedError) + }) +}) + +const hookEntry = (overrides: Partial = {}): CodexTrustEntry => ({ + sourcePath: '/home/u/.codex/hooks.json', + eventLabel: 'stop', + groupIndex: 0, + handlerIndex: 0, + command: '/bin/sh /home/u/.orca/agent-hooks/codex-hook.sh', + trustedHash: 'sha256:new', + ...overrides +}) + +describe('hooks.state edits and reads by decoded key (#22592)', () => { + const key = '/home/u/.codex/hooks.json:stop:0:0' + + it('replaces a quoted-segment hooks.state table instead of appending a duplicate', () => { + const existing = `model = "o3"\n\n["hooks"."state"."${key}"]\ntrusted_hash = "sha256:old"\n` + const updated = upsertHookTrustEntriesInContent(existing, [hookEntry()]) + const state = readTomlValueAtPath(parse(updated), ['hooks', 'state']) + expect(state).toEqual({ [key]: { enabled: true, trusted_hash: 'sha256:new' } }) + expect(updated.startsWith('model = "o3"\n')).toBe(true) + }) + + it('reads a literal-quoted enabled key and an inline hooks.state entry', () => { + const content = `[hooks.state]\n"${key}" = { trusted_hash = "sha256:h", 'enabled' = false }\n` + expect(readHookTrustEntriesFromContent(content).get(key)).toEqual({ + trustedHash: 'sha256:h', + enabled: false + }) + }) + + it('removes a quoted-segment hooks.state table', () => { + const existing = `["hooks"."state"."${key}"]\ntrusted_hash = "sha256:old"\n\n[tui]\ntheme = "dark"\n` + expect(parse(removeHookTrustEntriesFromContent(existing, [key]))).toEqual({ + tui: { theme: 'dark' } + }) + }) +}) + +describe('the managed-home mirror sees one identity per table (#22592 layer 3)', () => { + it('keeps one project table when ~/.codex has both spellings', () => { + const system = [ + '["projects"."/w"]', + 'trust_level = "trusted"', + '', + '[projects."/w"]', + 'trust_level = "trusted"', + '', + '[hooks.state]', + '[hooks.state]', + '' + ].join('\n') + const mirrored = prepareSystemConfigForFreshRuntimeMirror(system, '/home/u/.codex') + expect(() => parse(mirrored)).not.toThrow() + }) + + it('reads a quoted trust_level from a section block', () => { + const [section] = getTomlSections('["projects"."/w"]\n"trust_level" = \'untrusted\'\n') + expect(getProjectTrustLevel(section!.block)).toBe('untrusted') + }) +}) + +describe('review follow-ups for the checked writer', () => { + it('never treats the same sub-table under two array-of-tables elements as a duplicate', () => { + const content = [ + '["projects"."/w"]', + 'trust_level = "trusted"', + '[projects."/w"]', + 'trust_level = "trusted"', + '[[s]]', + '[s.o]', + 'x = 1', + '[[s]]', + '[s.o]', + 'x = 1', + '' + ].join('\n') + const repaired = repairOrcaCodexConfigDuplicates(content) + expect(parse(repaired)).toEqual({ + projects: { '/w': { trust_level: 'trusted' } }, + s: [{ o: { x: 1 } }, { o: { x: 1 } }] + }) + }) + + it('keeps the user’s empty table over Orca’s copy directly below it', () => { + const broken = '["projects"."/w"]\n[projects."/w"]\ntrust_level = "trusted"\n' + expect(repairOrcaCodexConfigDuplicates(broken)).toBe('["projects"."/w"]\n') + expect(trust(broken, '/w')).toBe('["projects"."/w"]\ntrust_level = "trusted"\n') + }) + + it('edits a file holding integers past 2^53, which Codex reads as 64-bit', () => { + const existing = 'max_bytes = 9007199254740993\n' + const updated = trust(existing) + expect(updated.startsWith(existing)).toBe(true) + expect(projectsOf(updated)).toEqual({ '/work/repo': { trust_level: 'trusted' } }) + }) + + it('keeps a comment that sits above the next table when replacing or removing a hook block', () => { + const key = '/home/u/.codex/hooks.json:stop:0:0' + const existing = `[hooks.state."${key}"]\ntrusted_hash = "sha256:old"\n\n# my servers\n[mcp_servers.z]\ncommand = "z"\n` + expect(upsertHookTrustEntriesInContent(existing, [hookEntry()])).toContain( + '# my servers\n[mcp_servers.z]' + ) + expect(removeHookTrustEntriesFromContent(existing, [key])).toBe( + '# my servers\n[mcp_servers.z]\ncommand = "z"\n' + ) + }) + + it('inserts `enabled = false` with the file\u2019s own CRLF line ending', () => { + const key = '/home/u/.codex/hooks.json:stop:0:0' + const existing = `model = "o3"\r\n[hooks.state."${key}"]\r\ntrusted_hash = "h"\r\n` + expect(setHookTrustEnabledContent(existing, [{ key, enabled: false }])).toBe( + `model = "o3"\r\n[hooks.state."${key}"]\r\nenabled = false\r\ntrusted_hash = "h"\r\n` + ) + }) + + it('checks repair removals against the file without Orca\u2019s blocks when the repair alone does not parse', () => { + const key = '/home/u/.codex/hooks.json:stop:0:0' + const block = (hash: string) => `[hooks.state."${key}"]\ntrusted_hash = "${hash}"\n` + const broken = `model = "o3"\n${block('a')}${block('a')}${block('b')}` + expect(parse(upsertHookTrustEntriesInContent(broken, [hookEntry()]))).toEqual({ + model: 'o3', + hooks: { state: { [key]: { enabled: true, trusted_hash: 'sha256:new' } } } + }) + }) + + it('refuses to collapse owned duplicates when something else in the file is also broken', () => { + const key = '/home/u/.codex/hooks.json:stop:0:0' + const broken = [ + `[hooks.state."${key}"]`, + 'trusted_hash = "a"', + `[hooks.state."${key}"]`, + 'trusted_hash = "b"', + '[mcp_servers.a]', + 'command = "x"', + '[mcp_servers.a]', + 'command = "y"', + '' + ].join('\n') + expect(() => upsertHookTrustEntriesInContent(broken, [hookEntry()])).toThrow( + CodexConfigTomlEditRefusedError + ) + const ownedOnly = broken.split('[mcp_servers.a]')[0]! + expect(() => parse(upsertHookTrustEntriesInContent(ownedOnly, [hookEntry()]))).not.toThrow() + }) +}) + +describe('refused Codex config writes', () => { + it('logs each refusal once, including inside an AggregateError, and returns other failures', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + const refusal = new CodexConfigTomlEditRefusedError({ + reason: 'input-invalid', + detail: 'the file is not valid TOML', + configPath: '/tmp/report-once-probe/config.toml' + }) + const other = new Error('EACCES') + expect( + reportCodexTrustWriteRefusals(new AggregateError([refusal, other], 'trust write failed')) + ).toEqual([other]) + expect(reportCodexTrustWriteRefusals(refusal)).toEqual([]) + expect(warn).toHaveBeenCalledTimes(1) + expect(String(warn.mock.calls[0]?.[0])).toContain('/tmp/report-once-probe/config.toml') + } finally { + warn.mockRestore() + } + }) + + it('logs each distinct refusal once across launches and a new message again', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + const systemPath = '/tmp/report-once-launches/system.toml' + const runtimePath = '/tmp/report-once-launches/runtime.toml' + const refusal = (configPath: string, line = 2) => + new CodexConfigTomlEditRefusedError({ + reason: 'input-invalid', + detail: 'the file is not valid TOML', + line, + configPath + }) + for (let launch = 0; launch < 4; launch++) { + reportCodexTrustWriteRefusals( + new AggregateError([refusal(systemPath), refusal(runtimePath)], 'trust write failed') + ) + reportCodexConfigTomlEditRefusal(refusal(systemPath), 'Skipped promoting Codex settings') + } + expect(warn).toHaveBeenCalledTimes(3) + + reportCodexTrustWriteRefusals(refusal(systemPath, 7)) + expect(warn).toHaveBeenCalledTimes(4) + expect(String(warn.mock.calls[3]?.[0])).toContain('(line 7)') + } finally { + warn.mockRestore() + } + }) +}) + +describe('a hook trust edit with nothing to write', () => { + const handBroken = 'model = "a"\nbroken = \n' + + it('leaves a config Codex cannot parse as it is instead of refusing', () => { + expect(upsertHookTrustEntriesInContent(handBroken, [])).toBe(handBroken) + expect( + setHookTrustEnabledContent(handBroken, [{ key: '/rt/hooks.json:stop:0:0', enabled: false }]) + ).toBe(handBroken) + }) +}) + +describe('a whole-document mirror result', () => { + const unreadable = 'model = "a"\nmodel = "b"\n' + const refuse = (inputs: (string | null)[]) => + refuseUnreadableCodexConfigResult({ + configPath: '/tmp/mirror-result-probe/config.toml', + result: unreadable, + inputs, + context: 'Skipped mirroring the Codex config into a managed home' + }) + + it('is refused when every input parses, counting Orca-repairable duplicates as parsing', () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + try { + const orcaDuplicates = + '["projects"."/r"]\ntrust_level = "trusted"\n\n[projects."/r"]\ntrust_level = "trusted"\n' + expect(refuse(['model = "a"\n', null, orcaDuplicates])).toBe(true) + expect(warn).toHaveBeenCalledTimes(1) + } finally { + warn.mockRestore() + } + }) + + it('is written as before when an input was broken by hand', () => { + expect(refuse(['model = "a"\n', 'broken = \n'])).toBe(false) + }) +}) diff --git a/src/main/codex/codex-config-toml-checked-edit.ts b/src/main/codex/codex-config-toml-checked-edit.ts new file mode 100644 index 000000000000..a98591f67ec2 --- /dev/null +++ b/src/main/codex/codex-config-toml-checked-edit.ts @@ -0,0 +1,257 @@ +import { + omitTomlPaths, + parseCodexConfigToml, + readTomlValueAtPath, + tomlValuesEqual, + type TomlKeyPath, + type TomlTable +} from './codex-config-toml-document' +import { + removedValuesSurviveIn, + repairOrcaCodexConfigDuplicatesWithRemovals, + repairUnparseableCodexConfig +} from './codex-config-toml-repair' +import type { TomlAssignmentLine } from './codex-config-toml-structure' + +export type CodexConfigTomlEditRefusalReason = + | 'input-invalid' + | 'result-invalid' + | 'unrelated-change' + | 'intended-value-missing' + | 'unsupported-form' + +/** Orca declined to write a Codex config.toml because the edit could not be proven safe. */ +export class CodexConfigTomlEditRefusedError extends Error { + readonly reason: CodexConfigTomlEditRefusalReason + readonly detail: string + readonly line: number | null + readonly configPath: string | null + + constructor(args: { + reason: CodexConfigTomlEditRefusalReason + detail: string + line?: number | null + configPath?: string | null + }) { + const where = args.configPath ?? 'Codex config.toml' + const at = args.line ? ` (line ${args.line})` : '' + super(`Orca left ${where} unchanged${at}: ${args.detail}`) + this.name = 'CodexConfigTomlEditRefusedError' + this.reason = args.reason + this.detail = args.detail + this.line = args.line ?? null + this.configPath = args.configPath ?? null + } + + forConfigPath(configPath: string): CodexConfigTomlEditRefusedError { + return new CodexConfigTomlEditRefusedError({ + reason: this.reason, + detail: this.detail, + line: this.line, + configPath + }) + } +} + +export type CodexConfigTomlEditResult = { + content: string + /** Paths the edit owns; every other parsed value must be unchanged. */ + ownedPaths: readonly TomlKeyPath[] + /** Values the result must hold afterwards; `undefined` means the path must be absent. */ + expected?: readonly { path: TomlKeyPath; value: unknown }[] +} + +export type CodexConfigTomlEdit = ( + content: string, + table: TomlTable | null +) => CodexConfigTomlEditResult + +export type CheckedCodexConfigTomlEditOptions = { + /** + * The edit replaces every definition of its owned paths, so it may run on a + * file that is invalid only because of duplicates it collapses. Returns the + * content with those definitions removed, which must parse and is what the + * "changes nothing else" check compares against. + */ + collapsesOwnedDuplicates?: (content: string) => string +} + +/** + * Runs a text-level edit and returns its result only if Codex could read it: + * the result parses, holds the intended values, and leaves every other value + * as it was. A file Orca broke earlier is repaired first; anything else is refused. + */ +export function applyCheckedCodexConfigTomlEdit( + existingContent: string, + edit: CodexConfigTomlEdit, + options: CheckedCodexConfigTomlEditOptions = {} +): string { + const before = parseCodexConfigToml(existingContent) + let input = existingContent + let inputTable: TomlTable | null = before.ok ? before.table : null + let comparisonTable = inputTable + let unverifiedRemovals: readonly TomlAssignmentLine[] = [] + if (!before.ok) { + const repair = repairOrcaCodexConfigDuplicatesWithRemovals(existingContent) + input = repair.content + const repaired = parseCodexConfigToml(input) + const withoutOwned = + !repaired.ok && options.collapsesOwnedDuplicates + ? parseCodexConfigToml(options.collapsesOwnedDuplicates(input)) + : null + if (repaired.ok) { + inputTable = repaired.table + comparisonTable = repaired.table + } else if (withoutOwned?.ok) { + comparisonTable = withoutOwned.table + unverifiedRemovals = repair.unverifiedRemovals + } else { + throw new CodexConfigTomlEditRefusedError({ + reason: 'input-invalid', + detail: `the file is not valid TOML (${before.message}). Fix it so Codex can read it; Orca never edits a config Codex cannot parse.`, + line: before.line + }) + } + } + const result = edit(input, inputTable) + if (result.content === existingContent) { + return existingContent + } + const after = parseCodexConfigToml(result.content) + if (!after.ok) { + throw new CodexConfigTomlEditRefusedError({ + reason: 'result-invalid', + detail: `the edit would have produced invalid TOML (${after.message}).`, + line: after.line + }) + } + for (const { path, value } of result.expected ?? []) { + if (!tomlValuesEqual(readTomlValueAtPath(after.table, path), value)) { + throw new CodexConfigTomlEditRefusedError({ + reason: 'intended-value-missing', + detail: `the edit would not have set ${path.join('.')} as intended.` + }) + } + } + if ( + comparisonTable && + !removedValuesSurviveIn(comparisonTable, unverifiedRemovals, result.ownedPaths) + ) { + throw new CodexConfigTomlEditRefusedError({ + reason: 'input-invalid', + detail: 'repairing duplicate tables would have removed a value that is defined nowhere else.' + }) + } + if ( + comparisonTable && + !tomlValuesEqual( + omitTomlPaths(comparisonTable, result.ownedPaths), + omitTomlPaths(after.table, result.ownedPaths) + ) + ) { + throw new CodexConfigTomlEditRefusedError({ + reason: 'unrelated-change', + detail: 'the edit would have changed settings it does not own.' + }) + } + return result.content +} + +/** For whole-document writers: the result must at least parse. */ +export function assertCodexConfigTomlParses(content: string): void { + const parsed = parseCodexConfigToml(content) + if (!parsed.ok) { + throw new CodexConfigTomlEditRefusedError({ + reason: 'result-invalid', + detail: `the result would not be valid TOML (${parsed.message}).`, + line: parsed.line + }) + } +} + +/** + * For whole-document producers (the managed-home mirrors): true, and reported + * once, when `result` does not parse although every input does after Orca's own + * repair. An input the user broke by hand is mirrored as it is. + */ +export function refuseUnreadableCodexConfigResult(args: { + configPath: string + result: string + inputs: readonly (string | null)[] + context: string +}): boolean { + const parsed = parseCodexConfigToml(args.result) + if ( + parsed.ok || + args.inputs.some( + (input) => input !== null && !parseCodexConfigToml(repairUnparseableCodexConfig(input)).ok + ) + ) { + return false + } + reportCodexConfigTomlEditRefusal( + new CodexConfigTomlEditRefusedError({ + reason: 'result-invalid', + detail: `the result would not be valid TOML (${parsed.message}).`, + line: parsed.line, + configPath: args.configPath + }), + args.context + ) + return true +} + +const TRUST_WRITE_CONTEXT = 'Skipped marking a workspace trusted for Codex' +const MAX_REPORTED_WARNINGS = 500 +const reportedWarnings = new Set() + +/** + * Why: a refused write recurs on every launch and mirror pass until the user + * edits the file, so each distinct message about a file logs once per session. + */ +export function warnCodexConfigOnce(configPath: string, message: string): void { + const key = `${configPath}\n${message}` + if (reportedWarnings.has(key)) { + return + } + if (reportedWarnings.size >= MAX_REPORTED_WARNINGS) { + const oldest = reportedWarnings.values().next() + if (!oldest.done) { + reportedWarnings.delete(oldest.value) + } + } + reportedWarnings.add(key) + console.warn(message) +} + +export function reportCodexConfigTomlEditRefusal( + error: CodexConfigTomlEditRefusedError, + context: string +): void { + warnCodexConfigOnce(error.configPath ?? context, `[codex-config] ${context}: ${error.message}`) +} + +/** Status detail for a failed hook trust write; `/hooks` cannot help while Codex cannot read the file. */ +export function describeHookTrustWriteFailure( + summary: string, + error: unknown, + approveAdvice = 'Run /hooks in Codex to approve.' +): string { + if (error instanceof CodexConfigTomlEditRefusedError && error.reason === 'input-invalid') { + return `${summary}: ${error.message}` + } + const message = (error instanceof Error ? error.message : String(error)).replace(/\.+$/, '') + return `${summary}: ${message}. ${approveAdvice}` +} + +/** Logs each refused trust write once and returns the other failures for the caller. */ +export function reportCodexTrustWriteRefusals(error: unknown): unknown[] { + const failures = error instanceof AggregateError ? error.errors : [error] + return failures.filter((failure) => { + if (!(failure instanceof CodexConfigTomlEditRefusedError)) { + return true + } + reportCodexConfigTomlEditRefusal(failure, TRUST_WRITE_CONTEXT) + return false + }) +} diff --git a/src/main/codex/codex-config-toml-document.ts b/src/main/codex/codex-config-toml-document.ts new file mode 100644 index 000000000000..bc665c2c818a --- /dev/null +++ b/src/main/codex/codex-config-toml-document.ts @@ -0,0 +1,131 @@ +export type TomlTable = Record + +// Why: smol-toml's only declarations are ESM, which the CommonJS CLI build cannot +// import statically, so type just the two members used. A literal require() (not +// createRequire) lets the SSH relay's esbuild bundle inline it: relay hosts have no node_modules. +type SmolToml = { + parse: (source: string, options: { integersAsBigInt: 'asNeeded' }) => TomlTable + TomlError: abstract new (...args: never[]) => Error & { line: number; column: number } +} +const { parse, TomlError }: SmolToml = require('smol-toml') +export type TomlKeyPath = readonly string[] + +export type CodexConfigTomlParse = + | { ok: true; table: TomlTable } + | { ok: false; message: string; line: number | null; column: number | null } + +/** Parses Codex config.toml exactly as TOML 1.0 does, so duplicates in any spelling are errors. */ +export function parseCodexConfigToml(content: string): CodexConfigTomlParse { + const source = content.charCodeAt(0) === 0xfeff ? content.slice(1) : content + try { + // Why: Codex reads integers as 64-bit, so values past 2^53 are valid config, not a parse error. + return { ok: true, table: parse(source, { integersAsBigInt: 'asNeeded' }) } + } catch (error) { + if (error instanceof TomlError) { + return { + ok: false, + message: error.message.split('\n')[0] ?? error.message, + line: error.line, + column: error.column + } + } + return { + ok: false, + message: error instanceof Error ? error.message : String(error), + line: null, + column: null + } + } +} + +export function isTomlTable(value: unknown): value is TomlTable { + return ( + typeof value === 'object' && + value !== null && + !Array.isArray(value) && + // Why: smol-toml's TomlDate extends Date, so this also excludes TOML datetimes. + !(value instanceof Date) + ) +} + +export function getTomlTable(value: unknown): TomlTable | null { + return isTomlTable(value) ? value : null +} + +export function readTomlValueAtPath(table: TomlTable, path: TomlKeyPath): unknown { + let current: unknown = table + for (const segment of path) { + const next = getTomlTable(current) + if (!next || !Object.hasOwn(next, segment)) { + return undefined + } + current = next[segment] + } + return current +} + +export function tomlValuesEqual(left: unknown, right: unknown): boolean { + if (Object.is(left, right)) { + return true + } + if (left instanceof Date || right instanceof Date) { + return left instanceof Date && right instanceof Date && String(left) === String(right) + } + if (Array.isArray(left) || Array.isArray(right)) { + return ( + Array.isArray(left) && + Array.isArray(right) && + left.length === right.length && + left.every((value, index) => tomlValuesEqual(value, right[index])) + ) + } + const leftTable = getTomlTable(left) + const rightTable = getTomlTable(right) + if (!leftTable || !rightTable) { + return false + } + const leftKeys = Object.keys(leftTable) + return ( + leftKeys.length === Object.keys(rightTable).length && + leftKeys.every( + (key) => Object.hasOwn(rightTable, key) && tomlValuesEqual(leftTable[key], rightTable[key]) + ) + ) +} + +/** Returns a copy without `paths`, dropping tables those removals left empty so a new parent table is not a change. */ +export function omitTomlPaths(table: TomlTable, paths: readonly TomlKeyPath[]): TomlTable { + const copy = cloneTomlTable(table) + for (const path of paths) { + omitTomlPath(copy, path) + } + return copy +} + +function omitTomlPath(table: TomlTable, path: TomlKeyPath): void { + const [head, ...rest] = path + if (head === undefined || !Object.hasOwn(table, head)) { + return + } + if (rest.length === 0) { + delete table[head] + return + } + const child = getTomlTable(table[head]) + if (!child) { + return + } + omitTomlPath(child, rest) + if (Object.keys(child).length === 0) { + delete table[head] + } +} + +function cloneTomlTable(table: TomlTable): TomlTable { + const copy: TomlTable = {} + for (const [key, value] of Object.entries(table)) { + const child = getTomlTable(value) + copy[key] = child ? cloneTomlTable(child) : value + } + return copy +} diff --git a/src/main/codex/codex-config-toml-repair.ts b/src/main/codex/codex-config-toml-repair.ts new file mode 100644 index 000000000000..9a3b2361f256 --- /dev/null +++ b/src/main/codex/codex-config-toml-repair.ts @@ -0,0 +1,160 @@ +import { + getAssignmentKeyPath, + getTomlTableBlocks, + readTomlAssignmentValue, + scanTomlStructure, + tomlKeyPathStartsWith, + tomlKeyPathsEqual, + type TomlAssignmentLine, + type TomlTableBlock +} from './codex-config-toml-structure' +import { + parseCodexConfigToml, + readTomlValueAtPath, + tomlValuesEqual, + type TomlTable +} from './codex-config-toml-document' +import { repairOrcaDuplicateTrustTables } from './config-toml-project-duplicate-repair' + +// Why (#22592): earlier Orca builds appended `[projects."

"]` beside Codex's +// `["projects"."

"]`, inserted a bare `trust_level` beside `"trust_level"`, +// and mirrored a second `[hooks.state]`. TOML rejects each, so every `codex` +// run failed. Orca's exact trust copies go (the user's table wins), then any +// duplicate that carries no value of its own; nothing else is removed. + +type TextEdit = { + start: number + end: number + replacement: string + /** Assignments this edit deletes whose value must survive elsewhere at the same path. */ + removes?: readonly TomlAssignmentLine[] +} + +export function repairOrcaCodexConfigDuplicates(content: string): string { + return repairOrcaCodexConfigDuplicatesWithRemovals(content).content +} + +// Why (#22592): a config Orca broke earlier must not carry its duplicates into +// every managed home; launch prep repairs the file itself on the next trust write. +export function repairUnparseableCodexConfig(config: string): string { + return parseCodexConfigToml(config).ok ? config : repairOrcaCodexConfigDuplicates(config) +} + +export type CodexConfigRepair = { + content: string + /** Removed assignments whose values a caller must still find, when the repaired file does not parse on its own. */ + unverifiedRemovals: readonly TomlAssignmentLine[] +} + +export function repairOrcaCodexConfigDuplicatesWithRemovals(content: string): CodexConfigRepair { + // Why: returns the content unchanged unless every Orca-shaped trust duplicate could go. + const trustRepaired = repairOrcaDuplicateTrustTables(content) + const edits: TextEdit[] = [] + collectDuplicateTableEdits(getTomlTableBlocks(scanTomlStructure(trustRepaired)), edits) + const repaired = applyTextEdits(trustRepaired, edits) + const removed = edits.flatMap((edit) => edit.removes ?? []) + const parsed = parseCodexConfigToml(repaired) + if (!parsed.ok) { + return { content: repaired, unverifiedRemovals: removed } + } + return removedValuesSurviveIn(parsed.table, removed) + ? { content: repaired, unverifiedRemovals: [] } + : { content: trustRepaired, unverifiedRemovals: [] } +} + +/** A repair may not lose a value: each removed assignment must still read the same at its path. */ +export function removedValuesSurviveIn( + table: TomlTable, + removed: readonly TomlAssignmentLine[], + exemptPaths: readonly (readonly string[])[] = [] +): boolean { + return removed.every((line) => { + const path = getAssignmentKeyPath(line) + if (path !== null && exemptPaths.some((exempt) => tomlKeyPathStartsWith(path, exempt))) { + return true + } + return ( + path !== null && + tomlValuesEqual(readTomlValueAtPath(table, path), readTomlAssignmentValue(line)) + ) + }) +} + +function assignments(block: TomlTableBlock): TomlAssignmentLine[] { + return block.body.filter((line): line is TomlAssignmentLine => line.kind === 'assignment') +} + +function collectDuplicateTableEdits(blocks: readonly TomlTableBlock[], edits: TextEdit[]): void { + const firstByIdentity = new Map() + const arrayTablePaths: (readonly string[])[] = [] + for (const block of blocks) { + if (block.header.isArray) { + arrayTablePaths.push(block.header.segments) + continue + } + // Why: `[s.o]` under two `[[s]]` elements is two tables, not a duplicate. + if (arrayTablePaths.some((path) => tomlKeyPathStartsWith(block.header.segments, path))) { + continue + } + const identity = JSON.stringify(block.header.segments) + const first = firstByIdentity.get(identity) + if (!first) { + firstByIdentity.set(identity, block) + continue + } + const duplicateAssignments = assignments(block) + if (duplicateAssignments.length === 0) { + // Why: a header with no values of its own defines nothing; dropping only + // the header line keeps any comments under it. + edits.push(removeLine(block.header)) + continue + } + if (duplicateAssignments.every((line) => isValueAlreadyDefined(first, line))) { + edits.push(removeLine(block.header)) + for (const line of duplicateAssignments) { + edits.push({ ...removeLine(line), removes: [line] }) + } + } + } +} + +function isValueAlreadyDefined(first: TomlTableBlock, line: TomlAssignmentLine): boolean { + const value = readTomlAssignmentValue(line) + return ( + value !== undefined && + assignments(first).some( + (candidate) => + tomlKeyPathsEqual(candidate.keySegments, line.keySegments) && + tomlValuesEqual(readTomlAssignmentValue(candidate), value) + ) + ) +} + +function removeLine(line: { lineStart: number; nextLineStart: number }): TextEdit { + return { start: line.lineStart, end: line.nextLineStart, replacement: '' } +} + +function applyTextEdits(content: string, edits: readonly TextEdit[]): string { + // Why: an insert at the offset where a removal starts must survive, so zero-width edits sort first. + const ordered = [...edits].sort( + (left, right) => + left.start - right.start || + Number(left.end !== left.start) - Number(right.end !== right.start) || + right.end - left.end + ) + const disjoint: TextEdit[] = [] + for (const edit of ordered) { + // Why: a line inside a table already removed whole is removed with it. + const previous = disjoint.at(-1) + if (!previous || edit.start >= previous.end) { + disjoint.push(edit) + } + } + let result = content + // Why: no toReversed(); the SSH relay runs this on Node 18. + for (let index = disjoint.length - 1; index >= 0; index -= 1) { + const edit = disjoint[index] + result = result.slice(0, edit.start) + edit.replacement + result.slice(edit.end) + } + return result +} diff --git a/src/main/codex/codex-config-toml-structure.ts b/src/main/codex/codex-config-toml-structure.ts new file mode 100644 index 000000000000..ad45f6cbcd6b --- /dev/null +++ b/src/main/codex/codex-config-toml-structure.ts @@ -0,0 +1,160 @@ +import { + createTomlLineScanState, + getTomlTableHeader, + isTomlStructuralLine, + updateTomlLineScanState +} from './config-toml-line-scan' +import { parseTomlKeyPath, parseTomlTableHeaderPath } from './config-toml-key-path' +import { escapeTomlBasicString } from './config-toml-syntax' +import { + parseCodexConfigToml, + readTomlValueAtPath, + type TomlKeyPath +} from './codex-config-toml-document' + +// Why: every Orca edit of a Codex config.toml locates tables and keys by their +// decoded key path, so `[projects."/x"]`, `["projects"."/x"]` and +// `projects."/x".trust_level` are one identity instead of three spellings. + +export type TomlTableContext = { segments: readonly string[]; isArray: boolean } + +type TomlLineBounds = { + /** Offset of the first character of the line. */ + lineStart: number + /** Offset just past the line's content, before any `\r\n` or `\n`. */ + contentEnd: number + /** Offset of the next line (content length for the last line). */ + nextLineStart: number + /** Line text without the trailing `\r` (and without a leading BOM on line one). */ + text: string +} + +export type TomlTableLine = TomlLineBounds & { + kind: 'table' + segments: readonly string[] + isArray: boolean +} + +export type TomlAssignmentLine = TomlLineBounds & { + kind: 'assignment' + table: TomlTableContext + keySegments: readonly string[] + /** Offset within `text` of the first value character. */ + valueOffset: number +} + +export type TomlStructureLine = + | TomlTableLine + | TomlAssignmentLine + | (TomlLineBounds & { kind: 'other' }) + +const ROOT_TABLE: TomlTableContext = { segments: [], isArray: false } + +export function scanTomlStructure(content: string): TomlStructureLine[] { + const lines: TomlStructureLine[] = [] + let cursor = 0 + let scanState = createTomlLineScanState() + let table = ROOT_TABLE + while (cursor < content.length) { + const newlineIndex = content.indexOf('\n', cursor) + const lineEnd = newlineIndex === -1 ? content.length : newlineIndex + const rawLine = content.slice(cursor, lineEnd) + const withoutCr = rawLine.endsWith('\r') ? rawLine.slice(0, -1) : rawLine + const text = cursor === 0 && withoutCr.charCodeAt(0) === 0xfeff ? withoutCr.slice(1) : withoutCr + const bounds: TomlLineBounds = { + lineStart: cursor, + contentEnd: cursor + withoutCr.length, + nextLineStart: newlineIndex === -1 ? content.length : newlineIndex + 1, + text + } + const line = isTomlStructuralLine(scanState) + ? classifyStructuralLine(bounds, table) + : { ...bounds, kind: 'other' as const } + if (line.kind === 'table') { + table = { segments: line.segments, isArray: line.isArray } + } + lines.push(line) + scanState = updateTomlLineScanState(scanState, text) + cursor = bounds.nextLineStart + } + return lines +} + +function classifyStructuralLine( + bounds: TomlLineBounds, + table: TomlTableContext +): TomlStructureLine { + const header = getTomlTableHeader(bounds.text) + if (header !== null) { + const parsed = parseTomlTableHeaderPath(header) + return parsed + ? { ...bounds, kind: 'table', segments: parsed.segments, isArray: parsed.isArray } + : { ...bounds, kind: 'other' } + } + const key = parseTomlKeyPath(bounds.text) + if (!key || bounds.text[key.end] !== '=') { + return { ...bounds, kind: 'other' } + } + let valueOffset = key.end + 1 + while (bounds.text[valueOffset] === ' ' || bounds.text[valueOffset] === '\t') { + valueOffset += 1 + } + return { ...bounds, kind: 'assignment', table, keySegments: key.segments, valueOffset } +} + +/** The decoded absolute key path an assignment defines, or null inside an array of tables. */ +export function getAssignmentKeyPath(line: TomlAssignmentLine): readonly string[] | null { + return line.table.isArray ? null : [...line.table.segments, ...line.keySegments] +} + +/** Reads a single-line assignment's value; multiline values read as undefined. */ +export function readTomlAssignmentValue(line: TomlAssignmentLine): unknown { + const parsed = parseCodexConfigToml(line.text) + return parsed.ok ? readTomlValueAtPath(parsed.table, line.keySegments) : undefined +} + +export function tomlKeyPathsEqual(left: TomlKeyPath, right: TomlKeyPath): boolean { + return left.length === right.length && left.every((segment, index) => segment === right[index]) +} + +export function tomlKeyPathStartsWith(path: TomlKeyPath, prefix: TomlKeyPath): boolean { + return path.length >= prefix.length && prefix.every((segment, index) => segment === path[index]) +} + +export function formatTomlKeyPath(path: TomlKeyPath): string { + return path + .map((segment) => + /^[A-Za-z0-9_-]+$/.test(segment) ? segment : `"${escapeTomlBasicString(segment)}"` + ) + .join('.') +} + +export type TomlTableBlock = { + header: TomlTableLine + /** Lines after the header up to (not including) the next table header. */ + body: TomlStructureLine[] + /** Offset just past the block (the next header's line start, or content length). */ + end: number +} + +export function getTomlTableBlocks(lines: readonly TomlStructureLine[]): TomlTableBlock[] { + const blocks: TomlTableBlock[] = [] + let current: TomlTableBlock | null = null + for (const line of lines) { + if (line.kind === 'table') { + if (current) { + blocks.push(current) + } + current = { header: line, body: [], end: line.nextLineStart } + continue + } + if (current) { + current.body.push(line) + current.end = line.nextLineStart + } + } + if (current) { + blocks.push(current) + } + return blocks +} diff --git a/src/main/codex/codex-hook-legacy-cleanup.ts b/src/main/codex/codex-hook-legacy-cleanup.ts index 36bdff8df62e..6942d196af4b 100644 --- a/src/main/codex/codex-hook-legacy-cleanup.ts +++ b/src/main/codex/codex-hook-legacy-cleanup.ts @@ -16,12 +16,10 @@ import { } from './codex-hook-definition' import { isRetiredCodexHookCommand } from './codex-hook-retired-commands' import { getSystemCodexHomePath } from './codex-home-paths' -import { - collectManagedTrustEntries, - removeSelfComputedMatchingTrustEntries -} from './codex-hook-trust-cleanup' +import { collectManagedTrustEntries } from './codex-hook-trust-cleanup' import { runExclusivelyForCodexTrustConfig } from './codex-trust-config-mutation-queue' -import { mutateRealHomeHooksPreservingUserTrust } from './codex-user-hook-trust-moves' +import { reportCodexConfigTomlEditRefusal } from './codex-config-toml-checked-edit' +import { planRetiredHookSweepTrust } from './codex-retired-hook-sweep-trust' const LEGACY_ORCA_PROFILE_NAME = 'orca-agent-status' const LEGACY_ORCA_PROFILE_BLOCK_START = '# BEGIN ORCA AGENT STATUS HOOKS' @@ -90,28 +88,43 @@ async function sweepLegacySystemManagedHooks(): Promise { } // Why: Codex hooks moved to Orca's managed CODEX_HOME in #2350; hooks from before then would keep external Codex sessions reporting into Orca. - if (removedManagedHook) { - // Why: this is the user's system hooks file, not Orca's runtime copy. - // Remove only retired Orca hook entries and preserve other managers' metadata. - const hooksWritePath = resolveHooksJsonWritePath(legacyConfigPath) - mutateRealHomeHooksPreservingUserTrust({ - sourcePath: legacyConfigPath, - tomlPath: getSystemCodexConfigTomlPath(), - beforeHooks: config.hooks, - afterHooks: nextHooks, - writeHooks: () => { - if ( - readFileSync(legacyConfigPath, 'utf-8') !== previousRaw || - resolveHooksJsonWritePath(legacyConfigPath) !== hooksWritePath - ) { - // Why: another process may have saved since the read; never replace - // that newer dotfiles generation with this stale parse. - throw new Error('System Codex hooks changed since Orca read them') - } - writeHooksJson(hooksWritePath, { ...config, hooks: nextHooks }, { preserveMode: true }) - } - }) - removeSelfComputedMatchingTrustEntries(getSystemCodexConfigTomlPath(), trustEntries) + if (!removedManagedHook) { + return + } + const tomlPath = getSystemCodexConfigTomlPath() + const trustPlan = planRetiredHookSweepTrust({ + tomlPath, + hooksPath: legacyConfigPath, + beforeHooks: config.hooks, + afterHooks: nextHooks, + retiredTrustEntries: trustEntries + }) + if (trustPlan.kind === 'refused' && trustPlan.error.reason === 'input-invalid') { + // Why: a retired entry is inert while Codex cannot read config.toml, but removing it + // now would strand the trust it shifts; the next launch re-derives the sweep from hooks.json. + reportCodexConfigTomlEditRefusal( + trustPlan.error, + 'Waiting to remove retired Orca hook entries until config.toml can be read' + ) + return + } + // Why: this is the user's system hooks file, not Orca's runtime copy. + // Remove only retired Orca hook entries and preserve other managers' metadata. + const hooksWritePath = resolveHooksJsonWritePath(legacyConfigPath) + if ( + readFileSync(legacyConfigPath, 'utf-8') !== previousRaw || + resolveHooksJsonWritePath(legacyConfigPath) !== hooksWritePath + ) { + // Why: another process may have saved since the read; never replace + // that newer dotfiles generation with this stale parse. + throw new Error('System Codex hooks changed since Orca read them') + } + writeHooksJson(hooksWritePath, { ...config, hooks: nextHooks }, { preserveMode: true }) + if (trustPlan.kind === 'edit') { + writeConfigAtomically(tomlPath, trustPlan.next, trustPlan.previous) + } else if (trustPlan.kind === 'refused') { + // Why no rollback: Codex reads this file, so the retired entry must go; it lists the shifted hooks for review. + reportCodexConfigTomlEditRefusal(trustPlan.error, 'Skipped moving shifted user hook trust') } } @@ -163,15 +176,20 @@ function cleanupLegacyCodexProfileHooks(): void { if (next.trim().length === 0) { unlinkSync(profilePath) } else { - writeConfigAtomically(profilePath, next) + writeConfigAtomically(profilePath, next, existing) } } export async function cleanupLegacyManagedHookRepresentations(): Promise { try { await cleanupLegacySystemManagedHooks() - cleanupLegacyCodexProfileHooks() } catch (error) { console.warn('[codex-hook-service] failed to clean legacy Codex hooks', error) } + // Why separate: the profile file is unrelated to ~/.codex/hooks.json, so its cleanup never waits on that sweep. + try { + cleanupLegacyCodexProfileHooks() + } catch (error) { + console.warn('[codex-hook-service] failed to clean the legacy Codex profile', error) + } } diff --git a/src/main/codex/codex-hook-local-install.ts b/src/main/codex/codex-hook-local-install.ts index 0c76835e4153..933c88cbd6d9 100644 --- a/src/main/codex/codex-hook-local-install.ts +++ b/src/main/codex/codex-hook-local-install.ts @@ -25,6 +25,7 @@ import { } from './codex-hook-definition' import { getCodexManagedScriptFileName } from './codex-hook-identity' import { cleanupLegacyManagedHookRepresentations } from './codex-hook-legacy-cleanup' +import { describeHookTrustWriteFailure } from './codex-config-toml-checked-edit' import { getManagedScript } from './codex-hook-script' import { grantManagedCodexHookTrust, @@ -179,12 +180,17 @@ export async function installCodexHooksExclusively( } applyMirroredRuntimeUserHookTrustStates(tomlPath, mirroredUserTrustEntries) } catch (error) { + // Why: the legacy sweep edits ~/.codex, not this config; a refused trust write here must not skip it. + await cleanupLegacyManagedHookRepresentations() return { agent: 'codex', state: 'error', configPath, managedHooksPresent: true, - detail: `Hooks installed but trust entries could not be written: ${error instanceof Error ? error.message : String(error)}. Run /hooks in Codex to approve.` + detail: describeHookTrustWriteFailure( + 'Hooks installed but trust entries could not be written', + error + ) } } snapshotCodexRuntimeHookTrustProvenance(runtimeHomePath) diff --git a/src/main/codex/codex-hook-local-maintenance.ts b/src/main/codex/codex-hook-local-maintenance.ts index bb89b4e6dc84..aee9f0e78ea8 100644 --- a/src/main/codex/codex-hook-local-maintenance.ts +++ b/src/main/codex/codex-hook-local-maintenance.ts @@ -10,6 +10,7 @@ import { upsertHookTrustEntries } from './config-toml-trust' import { getCodexConfigTomlPath, getConfigPath, writeCodexHooksJson } from './codex-hook-definition' import { getCodexManagedScriptFileName } from './codex-hook-identity' import { cleanupLegacyManagedHookRepresentations } from './codex-hook-legacy-cleanup' +import { describeHookTrustWriteFailure } from './codex-config-toml-checked-edit' import { removeRealHomeCodexHookForOptOut } from './codex-real-home-hook-install' import { removeRuntimeManagedHookTrustEntries, @@ -75,12 +76,17 @@ export async function refreshCodexRuntimeUserHooksExclusively( removeStaleRuntimeHookTrustEntries(tomlPath, configPath, trustEntries) applyMirroredRuntimeUserHookTrustStates(tomlPath, hookPlan.trustEntries) } catch (error) { + // Why: the legacy sweep edits ~/.codex, not this config; a refused trust write here must not skip it. + await cleanupLegacyManagedHookRepresentations() return { agent: 'codex', state: 'error', configPath, managedHooksPresent: false, - detail: `User hooks refreshed but trust entries could not be written: ${error instanceof Error ? error.message : String(error)}. Run /hooks in Codex to approve.` + detail: describeHookTrustWriteFailure( + 'User hooks refreshed but trust entries could not be written', + error + ) } } snapshotCodexRuntimeHookTrustProvenance(runtimeHomePath) diff --git a/src/main/codex/codex-hook-remote-install.ts b/src/main/codex/codex-hook-remote-install.ts index 2b6105cdc3df..8ce87fa7e40f 100644 --- a/src/main/codex/codex-hook-remote-install.ts +++ b/src/main/codex/codex-hook-remote-install.ts @@ -22,6 +22,7 @@ import { wrapReadablePosixHookCommand } from './codex-hook-definition' import { getManagedScript } from './codex-hook-script' +import { describeHookTrustWriteFailure } from './codex-config-toml-checked-edit' export async function installCodexHooksRemote( sftp: SFTPWrapper, @@ -118,9 +119,11 @@ export async function installCodexHooksRemote( state: 'error', configPath: remoteConfigPath, managedHooksPresent: true, - detail: `Hooks installed but trust entries could not be written: ${ - error instanceof Error ? error.message : String(error) - }. Run /hooks in Codex on the remote host to approve.` + detail: describeHookTrustWriteFailure( + 'Hooks installed but trust entries could not be written', + error, + 'Run /hooks in Codex on the remote host to approve.' + ) } } diff --git a/src/main/codex/codex-hook-user-mirroring.ts b/src/main/codex/codex-hook-user-mirroring.ts index c257819cf09c..f38b67267d6e 100644 --- a/src/main/codex/codex-hook-user-mirroring.ts +++ b/src/main/codex/codex-hook-user-mirroring.ts @@ -1,4 +1,3 @@ -import { existsSync, readFileSync } from 'node:fs' import { readHooksJsonWithRaw, removeManagedCommands, @@ -6,10 +5,8 @@ import { } from '../agent-hooks/installer-utils' import { computeTrustKey, - escapeTomlString, getCodexExplicitHomeHookSourcePath, - parseTrustKey, - writeConfigAtomically, + setHookTrustEnabledStates, type CodexTrustEntry } from './config-toml-trust' import { createCodexHookTrustEntry, getCodexHookTrustSignature } from './codex-hook-identity' @@ -158,52 +155,14 @@ export function moveMirroredRuntimeUserTrustAfterManagedStatusHook( }) } -function escapeRegex(value: string): string { - return value.replaceAll(/[.*+?^${}()|[\]\\]/g, '\\$&') -} - -function buildHookTrustHeaderKeyPattern(key: string): string { - const keyVariants = [key] - const parsed = parseTrustKey(key) - if (parsed && /^[A-Za-z]:[\\/]|^\\\\/.test(parsed.sourcePath)) { - const suffix = `:${parsed.eventLabel}:${parsed.groupIndex}:${parsed.handlerIndex}` - keyVariants.push( - `${parsed.sourcePath.replace(/\\/g, '/')}${suffix}`, - `${parsed.sourcePath.replace(/\//g, '\\')}${suffix}` - ) - } - const alternatives = [...new Set(keyVariants)].flatMap((variant) => { - const quoted = [`"${escapeRegex(escapeTomlString(variant))}"`] - if (!variant.includes("'")) { - // Why: tolerate raw-backslash literal keys from Codex/manual approval while repairing mirrored runtime trust across both Windows variants. - quoted.push(`'${escapeRegex(variant)}'`) - } - return quoted - }) - return `(?:${alternatives.join('|')})` -} - export function applyMirroredRuntimeUserHookTrustStates( tomlPath: string, entries: readonly MirroredRuntimeUserHookTrustEntry[] ): void { - if (entries.length === 0 || !existsSync(tomlPath)) { - return - } - - const existing = readFileSync(tomlPath, 'utf-8') - let updated = existing - for (const { entry, enabled } of entries) { - const headerKeyPattern = buildHookTrustHeaderKeyPattern(computeTrustKey(entry)) - const pattern = new RegExp( - `(\\[hooks\\.state\\.${headerKeyPattern}\\]\\r?\\n[ \\t]*enabled[ \\t]*=[ \\t]*)(true|false)`, - 'g' - ) - updated = updated.replace(pattern, `$1${enabled}`) - } - if (updated !== existing) { - writeConfigAtomically(tomlPath, updated) - } + setHookTrustEnabledStates( + tomlPath, + entries.map(({ entry, enabled }) => ({ key: computeTrustKey(entry), enabled })) + ) } function dedupeHookDefinitions(definitions: readonly HookDefinition[]): HookDefinition[] { diff --git a/src/main/codex/codex-hook-wsl-runtime.ts b/src/main/codex/codex-hook-wsl-runtime.ts index e2318e30b96f..54c1b6c87576 100644 --- a/src/main/codex/codex-hook-wsl-runtime.ts +++ b/src/main/codex/codex-hook-wsl-runtime.ts @@ -26,6 +26,7 @@ import { } from './codex-hook-trust-grant' import { removeSelfComputedTrustBeforeGrant } from './codex-managed-trust-grant-plan' import { getManagedScript } from './codex-hook-script' +import { describeHookTrustWriteFailure } from './codex-config-toml-checked-edit' import { removeStaleWslRuntimeManagedHookTrustEntries, removeWslRuntimeManagedHookTrustEntries @@ -136,7 +137,10 @@ async function installManagedHooksIntoWslRuntimeExclusively( state: 'error', configPath: plan.configPath, managedHooksPresent: true, - detail: `Hooks installed but trust entries could not be written: ${error instanceof Error ? error.message : String(error)}. Run /hooks in Codex to approve.` + detail: describeHookTrustWriteFailure( + 'Hooks installed but trust entries could not be written', + error + ) } } diff --git a/src/main/codex/codex-retired-hook-sweep-trust.ts b/src/main/codex/codex-retired-hook-sweep-trust.ts new file mode 100644 index 000000000000..2fd3b4ae4378 --- /dev/null +++ b/src/main/codex/codex-retired-hook-sweep-trust.ts @@ -0,0 +1,57 @@ +import { existsSync, readFileSync } from 'node:fs' +import type { HookDefinition } from '../agent-hooks/installer-utils' +import { + computeTrustKey, + computeTrustedHash, + readHookTrustEntriesFromContent, + removeHookTrustEntriesFromContent, + type CodexTrustEntry +} from './config-toml-trust' +import { moveHookTrustContent } from './config-toml-hook-trust-edit' +import { CodexConfigTomlEditRefusedError } from './codex-config-toml-checked-edit' +import { getMovedCodexUserHookTrust } from './codex-user-hook-trust-moves' + +/** + * The config.toml side of sweeping retired Orca entries from ~/.codex/hooks.json: + * the user hooks the removal shifts carry their trust to the new keys, and the + * retired entries' own self-computed trust goes. + */ +export type RetiredHookSweepTrustPlan = + | { kind: 'unchanged' } + | { kind: 'edit'; previous: string; next: string } + | { kind: 'refused'; error: CodexConfigTomlEditRefusedError } + +export function planRetiredHookSweepTrust(args: { + tomlPath: string + hooksPath: string + beforeHooks: Record + afterHooks: Record + retiredTrustEntries: readonly CodexTrustEntry[] +}): RetiredHookSweepTrustPlan { + if (!existsSync(args.tomlPath)) { + return { kind: 'unchanged' } + } + const raw = readFileSync(args.tomlPath, 'utf-8') + const previous = raw.charCodeAt(0) === 0xfeff ? raw.slice(1) : raw + try { + const moves = getMovedCodexUserHookTrust(args.hooksPath, args.beforeHooks, args.afterHooks) + const moved = moves.length > 0 ? moveHookTrustContent(previous, moves) : previous + // Why after the move: a shifted user hook may now hold a retired entry's key. + const trust = readHookTrustEntriesFromContent(moved) + const ownedKeys = args.retiredTrustEntries + .filter( + (entry) => trust.get(computeTrustKey(entry))?.trustedHash === computeTrustedHash(entry) + ) + .map(computeTrustKey) + const next = ownedKeys.length > 0 ? removeHookTrustEntriesFromContent(moved, ownedKeys) : moved + return next === previous ? { kind: 'unchanged' } : { kind: 'edit', previous, next } + } catch (error) { + if (!(error instanceof CodexConfigTomlEditRefusedError)) { + throw error + } + return { + kind: 'refused', + error: error.configPath === null ? error.forConfigPath(args.tomlPath) : error + } + } +} diff --git a/src/main/codex/codex-user-hook-trust-moves.test.ts b/src/main/codex/codex-user-hook-trust-moves.test.ts index 60e86e2d1ef6..63b9880cf24b 100644 --- a/src/main/codex/codex-user-hook-trust-moves.test.ts +++ b/src/main/codex/codex-user-hook-trust-moves.test.ts @@ -9,6 +9,7 @@ import { upsertHookTrustEntries, type CodexTrustEntry } from './config-toml-trust' +import { parseCodexConfigToml } from './codex-config-toml-document' import { getMovedCodexUserHookTrust, mutateRealHomeHooksPreservingUserTrust @@ -225,4 +226,45 @@ describe('real-home user hook trust rebasing', () => { }) } ) + + it.skipIf(process.platform === 'win32')( + 'replaces a stale table at the new key in any spelling, leaving one table that parses', + () => { + const orca = command('orca-hook') + const user = command('user-hook') + writeFileSync( + configPath, + [ + `["hooks"."state"."${hooksPath}:stop:0:0"]`, + 'trusted_hash = "sha256:stale"', + '', + `["hooks"."state"."${hooksPath}:stop:1:0"]`, + 'trusted_hash = "sha256:moved"', + '' + ].join('\n') + ) + + mutate({ Stop: [{ hooks: [orca] }, { hooks: [user] }] }, { Stop: [{ hooks: [user] }] }) + + const after = readFileSync(configPath, 'utf-8') + expect(after).toBe(`[hooks.state."${hooksPath}:stop:0:0"]\ntrusted_hash = "sha256:moved"\n`) + expect(parseCodexConfigToml(after).ok).toBe(true) + } + ) + + it('leaves a config Codex cannot parse untouched and logs the refusal once', () => { + const orca = command('orca-hook') + const user = command('user-hook') + upsertHookTrustEntries(configPath, [{ ...stopEntry(1, user), trustedHash: 'sha256:user' }]) + const broken = `not toml at all\n${readFileSync(configPath, 'utf-8')}` + writeFileSync(configPath, broken) + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + + mutate({ Stop: [{ hooks: [orca] }, { hooks: [user] }] }, { Stop: [{ hooks: [user] }] }) + mutate({ Stop: [{ hooks: [orca] }, { hooks: [user] }] }, { Stop: [{ hooks: [user] }] }) + + expect(readFileSync(configPath, 'utf-8')).toBe(broken) + expect(warn).toHaveBeenCalledTimes(1) + expect(String(warn.mock.calls[0]?.[0])).toContain('[codex-config]') + }) }) diff --git a/src/main/codex/codex-user-hook-trust-moves.ts b/src/main/codex/codex-user-hook-trust-moves.ts index 6182de8a439e..d286c355fb57 100644 --- a/src/main/codex/codex-user-hook-trust-moves.ts +++ b/src/main/codex/codex-user-hook-trust-moves.ts @@ -1,6 +1,10 @@ import type { HookCommandConfig, HookDefinition } from '../agent-hooks/installer-utils' import { createCodexHookTrustEntry } from './codex-hook-identity' import { computeTrustKey, moveHookTrustEntries, type CodexTrustEntry } from './config-toml-trust' +import { + CodexConfigTomlEditRefusedError, + reportCodexConfigTomlEditRefusal +} from './codex-config-toml-checked-edit' type HooksByEvent = Record @@ -81,6 +85,10 @@ export function mutateRealHomeHooksPreservingUserTrust(args: { } catch (error) { // Why no rollback: the hooks write is what was asked for; Codex lists the // moved hooks for review, which the user can approve there. - console.warn('[codex-user-hook-trust] could not move shifted user hook trust:', error) + if (error instanceof CodexConfigTomlEditRefusedError) { + reportCodexConfigTomlEditRefusal(error, 'Skipped moving shifted user hook trust') + } else { + console.warn('[codex-user-hook-trust] could not move shifted user hook trust:', error) + } } } diff --git a/src/main/codex/config-settings-promotion.ts b/src/main/codex/config-settings-promotion.ts index 56e499a6c186..84ea9c4e3474 100644 --- a/src/main/codex/config-settings-promotion.ts +++ b/src/main/codex/config-settings-promotion.ts @@ -6,6 +6,11 @@ import { writeFileAtomically } from '../codex-accounts/fs-utils' import { parseWslUncPath } from '../../shared/wsl-paths' import { getOrcaManagedCodexHomePath, getSystemCodexHomePath } from './codex-home-paths' import { upsertPromotedSettingsInContent } from './codex-config-settings-upsert' +import { + applyCheckedCodexConfigTomlEdit, + CodexConfigTomlEditRefusedError, + reportCodexConfigTomlEditRefusal +} from './codex-config-toml-checked-edit' import { PROMOTED_STRUCTURED_KEYS, readPromotedSettingValues, @@ -117,7 +122,10 @@ export function promoteCodexRuntimeSettingsToSystem( return promoteCodexRuntimeSettingsToSystemUnsafe(homes ?? getHostPromotionHomes()) } catch (error) { // Why: promotion is best-effort launch prep; a malformed file must not block Codex launch. - console.warn('[codex-settings-promotion] failed to promote runtime settings', error) + // A refused edit was already reported. + if (!(error instanceof CodexConfigTomlEditRefusedError)) { + console.warn('[codex-settings-promotion] failed to promote runtime settings', error) + } return null } } @@ -199,18 +207,44 @@ function promoteCodexRuntimeSettingsToSystemUnsafe( writeTargetObservation.kind === 'present' ? writeTargetObservation.value : extractOrdinaryCodexSettings(runtimeTomlObservation.value) - const withPromotedSettings = upsertPromotedSettingsInContent(systemContent, updates) - // Why: plan against the content actually being edited, not a second read of the - // source — when the system config is seeded from the runtime, its registration - // tables are already present and re-appending them would duplicate the table. - const nextContent = applyCodexRegistrationPromotions( - withPromotedSettings, - planCodexRegistrationPromotion( - runtimeTomlObservation.value, + const promote = (content: string): string => { + const withPromotedSettings = upsertPromotedSettingsInContent(content, updates) + // Why: plan against the content actually being edited, not a second read of the + // source — when the system config is seeded from the runtime, its registration + // tables are already present and re-appending them would duplicate the table. + return applyCodexRegistrationPromotions( withPromotedSettings, - baseline?.registrations ?? new Map() + planCodexRegistrationPromotion( + runtimeTomlObservation.value, + withPromotedSettings, + baseline?.registrations ?? new Map() + ) ) - ) + } + // Why (#22592): this is the user's real config, so the edit must leave it readable and change only what it promotes. + let nextContent: string + try { + nextContent = applyCheckedCodexConfigTomlEdit(systemContent, (content) => ({ + content: promote(content), + ownedPaths: [ + ...[...updates.keys()].map((key) => key.split('.')), + ['marketplaces'], + ['plugins'] + ] + })) + } catch (error) { + if (!(error instanceof CodexConfigTomlEditRefusedError)) { + throw error + } + const refusal = error.forConfigPath(systemTomlPath) + if (refusal.reason === 'input-invalid' && promote(systemContent) === systemContent) { + // Why: nothing to write back, so a ~/.codex the user broke by hand is mirrored as before. + return { conflicts, runtimeValuesToPreserve, mirroredMcpServers, mirroredMcpServerRoot } + } + // Why: a write-back that cannot land stalls the mirror, which would otherwise erase the runtime change. + reportCodexConfigTomlEditRefusal(refusal, 'Skipped promoting Codex settings') + throw refusal + } if (nextContent === systemContent) { return { conflicts, runtimeValuesToPreserve, mirroredMcpServers, mirroredMcpServerRoot } } diff --git a/src/main/codex/config-sync-stall.ts b/src/main/codex/config-sync-stall.ts index 6971b8d62412..c5f7b9887a00 100644 --- a/src/main/codex/config-sync-stall.ts +++ b/src/main/codex/config-sync-stall.ts @@ -20,7 +20,8 @@ import type { * blank, which is silent by design — but the stall can persist for every launch * (a downed WSL distro, an unhydrated cloud-synced home), leaving "Orca ignores * my config edits" with nothing to diagnose. Derived on demand from the same - * predicates the mirror uses, so the two can never disagree. + * source predicates the mirror uses; a pass that stalls on content (a refused + * settings promotion or an unparseable merge result) still reads as synced here. */ export function getCodexConfigSyncStatus( homes: CodexSettingsPromotionHomes = { diff --git a/src/main/codex/config-toml-hook-state-duplicate-repair.test.ts b/src/main/codex/config-toml-hook-state-duplicate-repair.test.ts index f28912ce906f..949aaa6c867f 100644 --- a/src/main/codex/config-toml-hook-state-duplicate-repair.test.ts +++ b/src/main/codex/config-toml-hook-state-duplicate-repair.test.ts @@ -9,6 +9,7 @@ import { upsertProjectTrustLevel, type CodexTrustEntry } from './config-toml-trust' +import { CodexConfigTomlEditRefusedError } from './codex-config-toml-checked-edit' import { createTrustConfigFixture, removeTrustConfigFixture @@ -132,7 +133,7 @@ describe('repairing Orca hooks.state duplicates', () => { 'model = "gpt-5.5"', '', `["projects"."${projectPath}"]`, - 'trust_level = "trusted"', + '"trust_level" = "trusted"', '', '["hooks"."state"]', '', @@ -178,7 +179,7 @@ describe('repairing Orca hooks.state duplicates', () => { ) }) - it('leaves two user-shaped hook tables alone and logs once', () => { + it('leaves two user-shaped hook tables alone and refuses the write', () => { const broken = [ '["hooks"."state"."/two-users:stop:0:0"]', 'enabled = true', @@ -191,7 +192,8 @@ describe('repairing Orca hooks.state duplicates', () => { ].join('\n') expect(repairOrcaDuplicateTrustTables(broken)).toBe(broken) - expect(repairOrcaDuplicateTrustTables(broken)).toBe(broken) - expect(warn).toHaveBeenCalledTimes(1) + expect(() => upsertHookTrustEntriesInContent(broken, [hookEntry])).toThrow( + CodexConfigTomlEditRefusedError + ) }) }) diff --git a/src/main/codex/config-toml-hook-trust-blocks.ts b/src/main/codex/config-toml-hook-trust-blocks.ts index 31dd30a1af5f..bae95abadd4a 100644 --- a/src/main/codex/config-toml-hook-trust-blocks.ts +++ b/src/main/codex/config-toml-hook-trust-blocks.ts @@ -1,12 +1,15 @@ -import { parseTomlTableHeaderPath } from './config-toml-key-path' import { createTomlLineScanState, - getTomlTableHeader, isTomlStructuralLine, updateTomlLineScanState } from './config-toml-line-scan' import { normalizeCodexHookTrustLookupKey } from './codex-trust-identity' import { findNextTomlTableHeader, parseHookStateTomlHeaderKey } from './config-toml-syntax' +import { + scanTomlStructure, + tomlKeyPathsEqual, + type TomlTableLine +} from './codex-config-toml-structure' export type HookTrustBlockRange = { start: number @@ -40,7 +43,12 @@ export function findHookTrustBlockRanges( const headerLineEnd = rawLine.endsWith('\r') ? lineEnd - 1 : lineEnd const nextHeaderOffset = findNextTomlTableHeader(content.slice(nextCursor)) const blockEnd = nextHeaderOffset === -1 ? content.length : nextCursor + nextHeaderOffset - ranges.push({ start: cursor, headerLineEnd, contentStart: nextCursor, end: blockEnd }) + ranges.push({ + start: cursor, + headerLineEnd, + contentStart: nextCursor, + end: excludeTrailingComments(content, nextCursor, blockEnd) + }) cursor = Math.max(blockEnd, nextCursor) continue } @@ -50,6 +58,29 @@ export function findHookTrustBlockRanges( return ranges } +/** Comments directly above the next table describe that table, so an edit of this block keeps them. */ +function excludeTrailingComments(content: string, contentStart: number, blockEnd: number): number { + const lines = content.slice(contentStart, blockEnd).split('\n') + if (lines.at(-1) === '') { + lines.pop() + } + let firstKept = lines.length + for (let index = lines.length - 1; index >= 0; index -= 1) { + const line = lines[index]?.trim() ?? '' + if (line.startsWith('#')) { + firstKept = index + } else if (line !== '') { + break + } + } + if (firstKept === lines.length) { + return blockEnd + } + return ( + contentStart + lines.slice(0, firstKept).reduce((length, line) => length + line.length + 1, 0) + ) +} + export function findAllHookTrustBlocks(content: string): (HookTrustBlockRange & { key: string })[] { const blocks: (HookTrustBlockRange & { key: string })[] = [] let cursor = 0 @@ -85,14 +116,21 @@ export function findAllHookTrustBlocks(content: string): (HookTrustBlockRange & } export function ensureHooksStateParentTable(content: string): string { - const { hasParent, firstChildOffset } = findHooksStateHeaders(content) - if (hasParent) { + const tables = scanTomlStructure(content).filter( + (line): line is TomlTableLine => line.kind === 'table' && !line.isArray + ) + if (tables.some((line) => tomlKeyPathsEqual(line.segments, ['hooks', 'state']))) { return content } const eol = content.includes('\r\n') ? '\r\n' : '\n' const parent = `[hooks.state]${eol}` - if (firstChildOffset !== null) { - return `${content.slice(0, firstChildOffset)}${parent}${eol}${content.slice(firstChildOffset)}` + // Why (#22592): any `hooks.state.*` spelling is a child; the parent must precede it. + const firstHookHeader = tables.find( + (line) => + line.segments.length > 2 && line.segments[0] === 'hooks' && line.segments[1] === 'state' + ) + if (firstHookHeader) { + return `${content.slice(0, firstHookHeader.lineStart)}${parent}${eol}${content.slice(firstHookHeader.lineStart)}` } if (content.length === 0) { return parent @@ -100,39 +138,3 @@ export function ensureHooksStateParentTable(content: string): string { const separator = content.endsWith(`${eol}${eol}`) ? '' : content.endsWith(eol) ? eol : eol + eol return `${content}${separator}${parent}` } - -// Why (#22592): `["hooks"."state"]` is the same table; a bare-only match appended a duplicate. -function findHooksStateHeaders(content: string): { - hasParent: boolean - firstChildOffset: number | null -} { - let firstChildOffset: number | null = null - let cursor = 0 - let scanState = createTomlLineScanState() - while (cursor < content.length) { - const newlineIndex = content.indexOf('\n', cursor) - const lineEnd = newlineIndex === -1 ? content.length : newlineIndex - const line = content.slice(cursor, lineEnd).replace(/\r$/, '') - if (isTomlStructuralLine(scanState)) { - const header = getTomlTableHeader(line) - const table = header === null ? null : parseTomlTableHeaderPath(header) - if ( - table && - !table.isArray && - table.segments[0] === 'hooks' && - table.segments[1] === 'state' - ) { - if (table.segments.length === 2) { - return { hasParent: true, firstChildOffset } - } - firstChildOffset ??= cursor - } - } - scanState = updateTomlLineScanState(scanState, line) - if (newlineIndex === -1) { - break - } - cursor = newlineIndex + 1 - } - return { hasParent: false, firstChildOffset } -} diff --git a/src/main/codex/config-toml-hook-trust-edit.ts b/src/main/codex/config-toml-hook-trust-edit.ts index e7858dd694cb..6f3373207207 100644 --- a/src/main/codex/config-toml-hook-trust-edit.ts +++ b/src/main/codex/config-toml-hook-trust-edit.ts @@ -13,43 +13,152 @@ import { findHookTrustBlockRanges, type HookTrustBlockRange } from './config-toml-hook-trust-blocks' -import { CODEX_HOOK_TRUST_KEY, escapeTomlBasicString } from './config-toml-syntax' -import { repairOrcaDuplicateTrustTables } from './config-toml-project-duplicate-repair' +import { + CODEX_HOOK_TRUST_KEY, + escapeTomlBasicString, + parseHookStateTomlHeaderKey +} from './config-toml-syntax' +import { applyCheckedCodexConfigTomlEdit } from './codex-config-toml-checked-edit' +import { readTomlValueAtPath } from './codex-config-toml-document' +import { + readTomlAssignmentValue, + scanTomlStructure, + tomlKeyPathsEqual +} from './codex-config-toml-structure' export function upsertHookTrustContent( existingContent: string, entries: readonly CodexTrustEntry[] ): string { - const existing = repairOrcaDuplicateTrustTables(stripLeadingBom(existingContent)) - let updated = entries.some((entry) => + // Why: nothing to write, so a config Codex cannot parse must not turn into a refusal. + if (entries.length === 0) { + return existingContent + } + const existing = stripLeadingBom(existingContent) + const writes = entries.map((entry) => ({ + keys: getTrustKeyWriteVariants(computeCodexTrustKey(entry)), + hash: entry.trustedHash ?? computeCodexTrustedHash(entry), + explicitEnabled: entry.enabled + })) + const needsParentTable = entries.some((entry) => usesWindowsCodexPathSeparators(normalizeCodexTrustSourcePath(entry.sourcePath)) ) - ? ensureHooksStateParentTable(existing) - : existing - for (const entry of entries) { - updated = upsertTrustBlocks( - updated, - getTrustKeyWriteVariants(computeCodexTrustKey(entry)), - entry.trustedHash ?? computeCodexTrustedHash(entry), - entry.enabled - ) + const updated = applyCheckedCodexConfigTomlEdit( + existing, + (content) => { + let next = needsParentTable ? ensureHooksStateParentTable(content) : content + for (const write of writes) { + next = upsertTrustBlocks(next, write.keys, write.hash, write.explicitEnabled) + } + return { + content: next, + ownedPaths: getOwnedHookStatePaths( + content, + writes.flatMap((write) => write.keys) + ), + expected: writes.flatMap((write) => + write.keys.map((key) => ({ + path: ['hooks', 'state', key, 'trusted_hash'], + value: write.hash + })) + ) + } + }, + { + collapsesOwnedDuplicates: (content) => + stripHookTrustBlocks( + content, + new Set(writes.flatMap((write) => write.keys).map(normalizeCodexHookTrustLookupKey)) + ) + } + ) + return updated === existing ? existingContent : updated +} + +function stripHookTrustBlocks(content: string, normalizedKeys: ReadonlySet): string { + let cursor = 0 + let stripped = '' + for (const range of findHookTrustBlockRanges(content, normalizedKeys)) { + stripped += content.slice(cursor, range.start) + cursor = range.end } - return updated + return stripped + content.slice(cursor) } export function removeHookTrustContent(content: string, keys: readonly string[]): string { const normalizedKeys = new Set(keys.map(normalizeCodexHookTrustLookupKey)) - const ranges = findHookTrustBlockRanges(content, normalizedKeys) - if (ranges.length === 0) { + if (findHookTrustBlockRanges(content, normalizedKeys).length === 0) { return content } - let cursor = 0 - let updated = '' - for (const range of ranges) { - updated += content.slice(cursor, range.start) - cursor = range.end + return applyCheckedCodexConfigTomlEdit( + content, + (input) => ({ + content: stripHookTrustBlocks(input, normalizedKeys), + ownedPaths: getOwnedHookStatePaths(input, keys) + }), + { collapsesOwnedDuplicates: (input) => stripHookTrustBlocks(input, normalizedKeys) } + ) +} + +/** Sets `enabled` on existing hooks.state tables only; a missing table stays missing. */ +export function setHookTrustEnabledContent( + existingContent: string, + states: readonly { key: string; enabled: boolean }[] +): string { + const existing = stripLeadingBom(existingContent) + const keys = new Set(states.map((state) => normalizeCodexHookTrustLookupKey(state.key))) + if (findHookTrustBlockRanges(existing, keys).length === 0) { + return existingContent } - return updated + content.slice(cursor) + const updated = applyCheckedCodexConfigTomlEdit(existing, (content) => { + let next = content + for (const { key, enabled } of states) { + next = setEnabledInTrustBlocks(next, key, enabled) + } + return { + content: next, + ownedPaths: getOwnedHookStatePaths( + content, + states.map((state) => state.key) + ).map((path) => [...path, 'enabled']) + } + }) + return updated === existing ? existingContent : updated +} + +function setEnabledInTrustBlocks(content: string, key: string, enabled: boolean): string { + const ranges = findHookTrustBlockRanges(content, new Set([normalizeCodexHookTrustLookupKey(key)])) + let next = content + // Why: no toReversed(); the SSH relay runs this on Node 18. + for (let index = ranges.length - 1; index >= 0; index -= 1) { + const range = ranges[index] + const enabledLine = scanTomlStructure(next.slice(range.contentStart, range.end)).find( + (line) => line.kind === 'assignment' && tomlKeyPathsEqual(line.keySegments, ['enabled']) + ) + if (enabledLine?.kind === 'assignment') { + const valueStart = range.contentStart + enabledLine.lineStart + enabledLine.valueOffset + const token = /^(?:true|false)/.exec(next.slice(valueStart)) + if (token) { + next = `${next.slice(0, valueStart)}${enabled}${next.slice(valueStart + token[0].length)}` + } + } else if (!enabled) { + const eol = next.includes('\r\n') ? '\r\n' : '\n' + next = `${next.slice(0, range.contentStart)}enabled = false${eol}${next.slice(range.contentStart)}` + } + } + return next +} + +/** The written keys plus every spelling of them the edit replaces. */ +function getOwnedHookStatePaths(content: string, keys: readonly string[]): string[][] { + const normalized = new Set(keys.map(normalizeCodexHookTrustLookupKey)) + const owned = new Set(keys) + for (const block of findAllHookTrustBlocks(content)) { + if (normalized.has(normalizeCodexHookTrustLookupKey(block.key))) { + owned.add(block.key) + } + } + return [...owned].map((key) => ['hooks', 'state', key]) } /** @@ -63,39 +172,75 @@ export function moveHookTrustContent( existingContent: string, moves: readonly { oldKey: string; newKey: string }[] ): string { - const content = stripLeadingBom(existingContent) - if (findAllHookTrustBlocks(content).some(({ key }) => !CODEX_HOOK_TRUST_KEY.test(key))) { - return existingContent - } - const bodies = moves.flatMap(({ oldKey, newKey }) => { - const [range] = findHookTrustBlockRanges( - content, - new Set([normalizeCodexHookTrustLookupKey(oldKey)]) - ) - return range ? [{ newKey, body: content.slice(range.contentStart, range.end).trimEnd() }] : [] - }) - let updated = removeHookTrustContent(content, [ - ...moves.map(({ oldKey }) => oldKey), - ...moves.map(({ newKey }) => newKey) - ]) - if (bodies.length === 0) { - return updated - } + const existing = stripLeadingBom(existingContent) + const touchedKeys = new Set( + moves.flatMap(({ oldKey, newKey }) => [oldKey, newKey]).map(normalizeCodexHookTrustLookupKey) + ) + // Why: nothing to move, so a config Codex cannot parse must not turn into a refusal. if ( - bodies.some(({ newKey }) => - usesWindowsCodexPathSeparators(parseCodexTrustKey(newKey)?.sourcePath ?? '') - ) + findHookTrustBlockRanges(existing, touchedKeys).length === 0 || + findAllHookTrustBlocks(existing).some(({ key }) => !CODEX_HOOK_TRUST_KEY.test(key)) ) { - updated = ensureHooksStateParentTable(updated) + return existingContent } - const blocks = bodies.flatMap(({ newKey, body }) => - getTrustKeyWriteVariants(newKey).map( - (key) => `[hooks.state.${formatHookStateTableKey(key)}]${body ? `\n${body}` : ''}` - ) + const updated = applyCheckedCodexConfigTomlEdit( + existing, + (content, table) => { + const moved = moves.flatMap(({ oldKey, newKey }) => { + const [range] = findHookTrustBlockRanges( + content, + new Set([normalizeCodexHookTrustLookupKey(oldKey)]) + ) + const sourceKey = range + ? parseHookStateTomlHeaderKey(content.slice(range.start, range.headerLineEnd)) + : null + return range && sourceKey !== null + ? [ + { + sourceKey, + newKey, + newKeys: getTrustKeyWriteVariants(newKey), + body: content.slice(range.contentStart, range.end).trimEnd() + } + ] + : [] + }) + let next = stripHookTrustBlocks(content, touchedKeys) + if (moved.length > 0) { + if ( + moved.some(({ newKey }) => + usesWindowsCodexPathSeparators(parseCodexTrustKey(newKey)?.sourcePath ?? '') + ) + ) { + next = ensureHooksStateParentTable(next) + } + const blocks = moved.flatMap(({ newKeys, body }) => + newKeys.map( + (key) => `[hooks.state.${formatHookStateTableKey(key)}]${body ? `\n${body}` : ''}` + ) + ) + next = appendTomlBlock(next, blocks.join('\n\n')) + } + return { + content: next, + ownedPaths: getOwnedHookStatePaths(content, [ + ...moves.flatMap(({ oldKey, newKey }) => [oldKey, newKey]), + ...moved.flatMap(({ newKeys }) => newKeys) + ]), + // Why: a move carries the block's values unchanged, so each new key must hold them. + expected: table + ? moved.flatMap(({ sourceKey, newKeys }) => + newKeys.map((key) => ({ + path: ['hooks', 'state', key], + value: readTomlValueAtPath(table, ['hooks', 'state', sourceKey]) + })) + ) + : [] + } + }, + { collapsesOwnedDuplicates: (content) => stripHookTrustBlocks(content, touchedKeys) } ) - const separator = - updated.length === 0 || updated.endsWith('\n\n') ? '' : updated.endsWith('\n') ? '\n' : '\n\n' - return `${updated}${separator}${blocks.join('\n\n')}\n` + return updated === existing ? existingContent : updated } function upsertTrustBlocks( @@ -109,7 +254,7 @@ function upsertTrustBlocks( new Set(keys.map(normalizeCodexHookTrustLookupKey)) ) if (ranges.length === 0) { - return appendTrustBlocks(content, keys, hash, explicitEnabled ?? true) + return appendTomlBlock(content, buildTrustBlocks(keys, hash, explicitEnabled ?? true)) } const enabled = explicitEnabled ?? !ranges.some((range) => isBlockDisabled(content, range)) const block = buildTrustBlocks(keys, hash, enabled) @@ -126,22 +271,17 @@ function upsertTrustBlocks( } function isBlockDisabled(content: string, range: HookTrustBlockRange): boolean { - const block = content.slice(range.headerLineEnd, range.end) - const enabledMatch = /^[ \t]*enabled[ \t]*=[ \t]*(true|false)[ \t\r]*(?:#.*)?$/m.exec(block) - return enabledMatch?.[1] === 'false' + return scanTomlStructure(content.slice(range.contentStart, range.end)).some( + (line) => + line.kind === 'assignment' && + tomlKeyPathsEqual(line.keySegments, ['enabled']) && + readTomlAssignmentValue(line) === false + ) } -function appendTrustBlocks( - content: string, - keys: readonly string[], - hash: string, - enabled: boolean -): string { - const block = buildTrustBlocks(keys, hash, enabled) - if (content.length === 0) { - return `${block}\n` - } - const separator = content.endsWith('\n\n') ? '' : content.endsWith('\n') ? '\n' : '\n\n' +function appendTomlBlock(content: string, block: string): string { + const separator = + content.length === 0 || content.endsWith('\n\n') ? '' : content.endsWith('\n') ? '\n' : '\n\n' return `${content}${separator}${block}\n` } diff --git a/src/main/codex/config-toml-hook-trust-read.test.ts b/src/main/codex/config-toml-hook-trust-read.test.ts new file mode 100644 index 000000000000..3a391db66dfe --- /dev/null +++ b/src/main/codex/config-toml-hook-trust-read.test.ts @@ -0,0 +1,24 @@ +import { describe, expect, it } from 'vitest' +import { readHookTrustContent } from './config-toml-hook-trust-read' + +const KEY = '/h/hooks.json:stop:0:0' + +// Why: Codex may quote field names too; a quoted `"enabled" = false` must still read as disabled. +describe('readHookTrustContent quoted field names', () => { + const block = `["hooks"."state"."${KEY}"]\n"trusted_hash" = "sha256:abc"\n"enabled" = false\n` + + it('reads them from a file that parses', () => { + expect(readHookTrustContent(block).get(KEY)).toEqual({ + trustedHash: 'sha256:abc', + enabled: false + }) + }) + + it('reads them from a file that does not parse (#22592 duplicate key)', () => { + const broken = `model = "a"\nmodel = "b"\n\n${block}` + expect(readHookTrustContent(broken).get(KEY)).toEqual({ + trustedHash: 'sha256:abc', + enabled: false + }) + }) +}) diff --git a/src/main/codex/config-toml-hook-trust-read.ts b/src/main/codex/config-toml-hook-trust-read.ts index 690a2a8538a4..a5b0937f7202 100644 --- a/src/main/codex/config-toml-hook-trust-read.ts +++ b/src/main/codex/config-toml-hook-trust-read.ts @@ -1,12 +1,12 @@ import type { CodexHookTrustState } from './config-toml-trust' import { normalizeCodexHookTrustLookupKey } from './codex-trust-identity' import { findAllHookTrustBlocks } from './config-toml-hook-trust-blocks' +import { getTomlTable, parseCodexConfigToml } from './codex-config-toml-document' import { - createTomlLineScanState, - isTomlStructuralLine, - updateTomlLineScanState -} from './config-toml-line-scan' -import { unescapeTomlBasicString } from './config-toml-syntax' + readTomlAssignmentValue, + scanTomlStructure, + tomlKeyPathsEqual +} from './codex-config-toml-structure' export class CodexHookTrustEntryMap extends Map { override get(key: string): CodexHookTrustState | undefined { @@ -26,12 +26,13 @@ export class CodexHookTrustEntryMap extends Map { } } +type HookTrustStateSource = { key: string; trustedHashes: Set; enabled?: boolean } + export function readHookTrustContent(content: string): Map { const result = new CodexHookTrustEntryMap() const conflictingTrustedHashKeys = new Set() - for (const block of findAllHookTrustBlocks(content)) { - const state = readHookTrustBlockState(content.slice(block.contentStart, block.end)) - const normalizedKey = normalizeCodexHookTrustLookupKey(block.key) + for (const state of readHookTrustStateSources(content)) { + const normalizedKey = normalizeCodexHookTrustLookupKey(state.key) const existingState = result.get(normalizedKey) const trustedHash = state.trustedHashes.size === 1 ? state.trustedHashes.values().next().value : undefined @@ -56,32 +57,52 @@ export function readHookTrustContent(content: string): Map { + const state = getTomlTable(value) + if (!state) { + return [] + } + const trustedHashes = new Set() + if (typeof state.trusted_hash === 'string') { + trustedHashes.add(state.trusted_hash) + } + return [ + { + key, + trustedHashes, + enabled: typeof state.enabled === 'boolean' ? state.enabled : undefined + } + ] + }) + } + return findAllHookTrustBlocks(content).map((block) => ({ + key: block.key, + ...readHookTrustBlockState(content.slice(block.contentStart, block.end)) + })) +} + function readHookTrustBlockState(block: string): { trustedHashes: Set enabled?: boolean } { const trustedHashes = new Set() let enabled: boolean | undefined - let cursor = 0 - let scanState = createTomlLineScanState() - while (cursor < block.length) { - const newlineIndex = block.indexOf('\n', cursor) - const lineEnd = newlineIndex === -1 ? block.length : newlineIndex - const line = block.slice(cursor, lineEnd).replace(/\r$/, '') - if (isTomlStructuralLine(scanState)) { - const hashMatch = /^[ \t]*trusted_hash[ \t]*=[ \t]*"((?:[^"\\]|\\.)*)"[ \t]*(?:#.*)?$/.exec( - line - ) - if (hashMatch) { - trustedHashes.add(unescapeTomlBasicString(hashMatch[1]!)) - } - const enabledMatch = /^[ \t]*enabled[ \t]*=[ \t]*(true|false)[ \t]*(?:#.*)?$/.exec(line) - if (enabledMatch) { - enabled = enabled !== false && enabledMatch[1] === 'true' - } + for (const line of scanTomlStructure(block)) { + if (line.kind !== 'assignment' || line.table.segments.length > 0) { + continue + } + const value = readTomlAssignmentValue(line) + if (tomlKeyPathsEqual(line.keySegments, ['trusted_hash']) && typeof value === 'string') { + trustedHashes.add(value) + } + if (tomlKeyPathsEqual(line.keySegments, ['enabled']) && typeof value === 'boolean') { + enabled = enabled !== false && value } - scanState = updateTomlLineScanState(scanState, line) - cursor = newlineIndex === -1 ? block.length : newlineIndex + 1 } return { trustedHashes, enabled } } diff --git a/src/main/codex/config-toml-line-scan.ts b/src/main/codex/config-toml-line-scan.ts index 0e983aac2dde..71c05245d4da 100644 --- a/src/main/codex/config-toml-line-scan.ts +++ b/src/main/codex/config-toml-line-scan.ts @@ -1,4 +1,4 @@ -// Why: Orca edits Codex config.toml byte-preservingly (no TOML dependency), so +// Why: Orca edits Codex config.toml as text to preserve the user's bytes, so // every editor must agree on which lines sit inside multiline strings or // arrays vs real TOML structure. Keep the line-scanner in one place to avoid // drift. diff --git a/src/main/codex/config-toml-project-duplicate-repair.test.ts b/src/main/codex/config-toml-project-duplicate-repair.test.ts index 2c3df27d49d8..c67093fefcb4 100644 --- a/src/main/codex/config-toml-project-duplicate-repair.test.ts +++ b/src/main/codex/config-toml-project-duplicate-repair.test.ts @@ -3,12 +3,16 @@ import { readFileSync, writeFileSync } from 'node:fs' import { findDuplicateTomlDeclarations } from './config-toml-duplicate-declarations-test-fixture' import { repairOrcaDuplicateTrustTables } from './config-toml-project-duplicate-repair' import { upsertProjectTrustLevel, upsertProjectTrustLevelInContent } from './config-toml-trust' +import { + CodexConfigTomlEditRefusedError, + reportCodexTrustWriteRefusals +} from './codex-config-toml-checked-edit' import { createTrustConfigFixture, removeTrustConfigFixture } from './config-toml-trust-test-fixtures' -// Why: each case uses its own path because refusals are logged once per process. +// Why: each case uses its own path because refusals are logged once per file and message. let tmpDir: string let configPath: string let warn: ReturnType @@ -45,12 +49,13 @@ describe('repairing configs broken by #22592', () => { upsertProjectTrustLevel(configPath, projectPath, 'trusted') const repaired = readFileSync(configPath, 'utf-8') + // Why: Codex's own line already says trusted, so it is left as Codex wrote it. expect(repaired).toBe( [ 'model = "gpt-5.5"', '', `["projects"."${projectPath}"]`, - 'trust_level = "trusted"', + '"trust_level" = "trusted"', '' ].join('\n') ) @@ -142,15 +147,23 @@ describe('repairing configs broken by #22592', () => { '' ].join('\n') - const first = upsertProjectTrustLevelInContent(broken, '/repo-d', 'trusted', { - alreadyCanonical: true - }) - const second = upsertProjectTrustLevelInContent(broken, '/repo-d', 'trusted', { - alreadyCanonical: true - }) - - expect(first).toBe(broken) - expect(second).toBe(broken) + const attempt = (): unknown => { + try { + return upsertProjectTrustLevelInContent(broken, '/repo-d', 'trusted', { + alreadyCanonical: true + }) + } catch (error) { + return error + } + } + const first = attempt() + const second = attempt() + + // Why: the checked writer refuses instead of writing; its callers log each refusal once. + expect(first).toBeInstanceOf(CodexConfigTomlEditRefusedError) + expect(second).toBeInstanceOf(CodexConfigTomlEditRefusedError) + expect(reportCodexTrustWriteRefusals(first)).toEqual([]) + expect(reportCodexTrustWriteRefusals(second)).toEqual([]) expect(warn).toHaveBeenCalledTimes(1) }) @@ -171,7 +184,9 @@ describe('repairing configs broken by #22592', () => { ].join('\n') expect(repairOrcaDuplicateTrustTables(broken)).toBe(broken) - expect(warn).toHaveBeenCalledTimes(1) + expect(() => + upsertProjectTrustLevelInContent(broken, '/repo-e', 'trusted', { alreadyCanonical: true }) + ).toThrow(CodexConfigTomlEditRefusedError) }) it('does not touch a valid config', () => { diff --git a/src/main/codex/config-toml-project-duplicate-repair.ts b/src/main/codex/config-toml-project-duplicate-repair.ts index 92e7a506e6ce..a6748d9d1331 100644 --- a/src/main/codex/config-toml-project-duplicate-repair.ts +++ b/src/main/codex/config-toml-project-duplicate-repair.ts @@ -18,7 +18,6 @@ type TomlTable = { } const ORCA_TRUST_LINE = 'trust_level = "trusted"' -const loggedRefusals = new Set() /** * Removes the duplicates older Orca builds wrote beside Codex-spelled trust @@ -26,7 +25,7 @@ const loggedRefusals = new Set() * empty `[hooks.state]` that repeats a table the user (or Codex) wrote in * another spelling, and the bare `trust_level` line Orca inserted under a * quoted `"trust_level"`. The user's table always wins; anything else leaves - * the content untouched. + * the content untouched, and the checked writer that called this reports it. */ export function repairOrcaDuplicateTrustTables(content: string): string { const lines = content.split('\n') @@ -48,7 +47,7 @@ export function repairOrcaDuplicateTrustTables(content: string): string { (userTables.length !== 1 || group.some((table) => table !== survivor && table.header === survivor.header))) ) { - return refuseRepair(content, `duplicate table ${group[0]?.header}`) + return content } for (const table of group) { if (table !== survivor) { @@ -60,7 +59,7 @@ export function repairOrcaDuplicateTrustTables(content: string): string { if (getTrustTableKind(survivor) === 'project') { const insertedLine = findOrcaInsertedTrustLine(lines, survivor) if (insertedLine === 'unsafe') { - return refuseRepair(content, `duplicate trust_level in ${survivor.header}`) + return content } if (insertedLine !== null) { removedLines.add(insertedLine) @@ -73,10 +72,7 @@ export function repairOrcaDuplicateTrustTables(content: string): string { const kept = lines.filter((_, index) => !removedLines.has(index)).join('\n') const repaired = content.endsWith('\n') && !kept.endsWith('\n') ? `${kept}\n` : kept // Why: a partial repair would still fail Codex's parse, so only write a file it can load. - const remainingDuplicate = findDuplicateTable(repaired) - return remainingDuplicate === null - ? repaired - : refuseRepair(content, `duplicate table ${remainingDuplicate} remains`) + return findDuplicateTable(repaired) === null ? repaired : content } function readTomlTables(lines: string[]): TomlTable[] { @@ -187,14 +183,6 @@ function findDuplicateTable(content: string): string | null { return null } -function refuseRepair(content: string, reason: string): string { - if (!loggedRefusals.has(reason)) { - loggedRefusals.add(reason) - console.warn(`[codex-config] Left a duplicate in config.toml unrepaired: ${reason}`) - } - return content -} - function stripCr(line: string | undefined): string { return (line ?? '').replace(/\r$/, '') } diff --git a/src/main/codex/config-toml-project-trust.ts b/src/main/codex/config-toml-project-trust.ts index 9335605cb15b..fcc13b858c4c 100644 --- a/src/main/codex/config-toml-project-trust.ts +++ b/src/main/codex/config-toml-project-trust.ts @@ -1,18 +1,22 @@ import { realpathSync } from 'node:fs' import type { CodexProjectTrustLevel } from './config-toml-trust' import { normalizeCodexTrustProjectPath } from './codex-trust-identity' +import { escapeTomlBasicString } from './config-toml-syntax' +import { getTomlTable, type TomlTable } from './codex-config-toml-document' import { - createTomlLineScanState, - isTomlStructuralLine, - updateTomlLineScanState -} from './config-toml-line-scan' + applyCheckedCodexConfigTomlEdit, + CodexConfigTomlEditRefusedError, + type CodexConfigTomlEditResult +} from './codex-config-toml-checked-edit' import { - escapeTomlBasicString, - findNextTomlTableHeader, - parseProjectTomlHeaderPath -} from './config-toml-syntax' -import { findProjectTrustLevelEntries } from './config-toml-project-trust-level' -import { repairOrcaDuplicateTrustTables } from './config-toml-project-duplicate-repair' + formatTomlKeyPath, + getAssignmentKeyPath, + scanTomlStructure, + tomlKeyPathsEqual, + tomlKeyPathStartsWith, + type TomlAssignmentLine, + type TomlStructureLine +} from './codex-config-toml-structure' export function upsertProjectTrustContent( existingContent: string, @@ -20,28 +24,139 @@ export function upsertProjectTrustContent( trustLevel: CodexProjectTrustLevel, options?: { alreadyCanonical?: boolean } ): string { - const existing = repairOrcaDuplicateTrustTables(stripLeadingBom(existingContent)) const trustedProjectPath = options?.alreadyCanonical ? projectPath : canonicalizeLocalProjectPath(projectPath) - const headerLineEnd = findProjectHeaderLineEnd(existing, trustedProjectPath) - const eol = existing.includes('\r\n') ? '\r\n' : '\n' - const trustLine = `trust_level = "${trustLevel}"` - if (headerLineEnd === null) { - return appendProjectTrustBlock(existing, trustedProjectPath, trustLine, eol) + const existing = stripLeadingBom(existingContent) + const updated = applyCheckedCodexConfigTomlEdit(existing, (content, table) => + editProjectTrust(content, table, trustedProjectPath, trustLevel) + ) + return updated === existing ? existingContent : updated +} + +/** Decides from the parsed document, so any spelling Codex accepts is found before Orca appends. */ +function editProjectTrust( + content: string, + table: TomlTable | null, + projectPath: string, + trustLevel: CodexProjectTrustLevel +): CodexConfigTomlEditResult { + const projectKey = findExistingProjectKey(table, projectPath) ?? projectPath + const trustPath = ['projects', projectKey, 'trust_level'] + const result = (next: string): CodexConfigTomlEditResult => ({ + content: next, + ownedPaths: [trustPath], + expected: [{ path: trustPath, value: trustLevel }] + }) + const project = getTomlTable(getTomlTable(table?.projects)?.[projectKey]) + if (project?.trust_level === trustLevel) { + return result(content) + } + const eol = content.includes('\r\n') ? '\r\n' : '\n' + const trustValue = `"${trustLevel}"` + const lines = scanTomlStructure(content) + const trustLine = lines.find( + (line): line is TomlAssignmentLine => + line.kind === 'assignment' && pathEquals(getAssignmentKeyPath(line), trustPath) + ) + if (trustLine) { + // Why (#22592): a key Codex quoted is rewritten bare, the one spelling Orca writes. + const bareKey = trustLine.keySegments.length === 1 ? 'trust_level' : null + return result(replaceAssignmentValue(content, trustLine, trustValue, bareKey)) + } + const header = lines.find( + (line) => + line.kind === 'table' && + !line.isArray && + tomlKeyPathsEqual(line.segments, trustPath.slice(0, 2)) + ) + if (header) { + return result(insertLineAt(content, header, `trust_level = ${trustValue}`, eol)) + } + const dottedProjectLine = findLastDottedProjectAssignment(lines, trustPath.slice(0, 2)) + if (dottedProjectLine) { + const relativeKey = formatTomlKeyPath(trustPath.slice(dottedProjectLine.table.segments.length)) + return result(insertLineAt(content, dottedProjectLine, `${relativeKey} = ${trustValue}`, eol)) + } + const inlineProject = lines.some( + (line) => + line.kind === 'assignment' && pathEquals(getAssignmentKeyPath(line), trustPath.slice(0, 2)) + ) + if (inlineProject) { + throw new CodexConfigTomlEditRefusedError({ + reason: 'unsupported-form', + detail: `projects."${projectKey}" is an inline table; set trust_level there or trust the folder in Codex.` + }) + } + return result(appendProjectTrustBlock(content, projectKey, `trust_level = ${trustValue}`, eol)) +} + +function findExistingProjectKey(table: TomlTable | null, projectPath: string): string | null { + const projects = getTomlTable(table?.projects) + if (!projects) { + return null + } + if (Object.hasOwn(projects, projectPath)) { + return projectPath } - const nextHeaderOffset = findNextTomlTableHeader(existing.slice(headerLineEnd)) - const blockEnd = nextHeaderOffset === -1 ? existing.length : headerLineEnd + nextHeaderOffset - // Why: rewrite any existing trust_level key, whatever its spelling or value, so none is duplicated. - const existingEntry = findProjectTrustLevelEntries(existing.slice(headerLineEnd, blockEnd))[0] - if (existingEntry) { - return ( - existing.slice(0, headerLineEnd + existingEntry.start) + - trustLine + - existing.slice(headerLineEnd + existingEntry.end) - ) + const lookupPath = normalizeCodexTrustProjectPath(projectPath) + return ( + Object.keys(projects).find((key) => normalizeCodexTrustProjectPath(key) === lookupPath) ?? null + ) +} + +function pathEquals(path: readonly string[] | null, expected: readonly string[]): boolean { + return path !== null && tomlKeyPathsEqual(path, expected) +} + +/** A project defined only by dotted keys (`"/x".model = …` under `[projects]`) takes its trust as one more dotted key. */ +function findLastDottedProjectAssignment( + lines: readonly TomlStructureLine[], + projectTablePath: readonly string[] +): TomlAssignmentLine | undefined { + // Why: no findLast(); the SSH relay bundles this for older Node hosts. + for (let index = lines.length - 1; index >= 0; index -= 1) { + const line = lines[index] + if (line?.kind !== 'assignment') { + continue + } + const path = getAssignmentKeyPath(line) + if ( + path !== null && + path.length > projectTablePath.length && + tomlKeyPathStartsWith(path, projectTablePath) && + line.table.segments.length < projectTablePath.length + ) { + return line + } } - return `${existing.slice(0, headerLineEnd)}${eol}${trustLine}${existing.slice(headerLineEnd)}` + return undefined +} + +function replaceAssignmentValue( + content: string, + line: TomlAssignmentLine, + renderedValue: string, + renderedKey: string | null +): string { + const valueStart = line.lineStart + line.valueOffset + const rest = content.slice(valueStart, line.contentEnd) + const stringValue = /^(?:"(?:[^"\\]|\\.)*"|'[^']*')/.exec(rest) + const valueEnd = stringValue ? valueStart + stringValue[0].length : line.contentEnd + const keyStart = line.lineStart + line.text.length - line.text.trimStart().length + const assignment = renderedKey === null ? renderedValue : `${renderedKey} = ${renderedValue}` + const replaceFrom = renderedKey === null ? valueStart : keyStart + return content.slice(0, replaceFrom) + assignment + content.slice(valueEnd) +} + +function insertLineAt( + content: string, + after: TomlStructureLine, + text: string, + eol: string +): string { + const lineEnd = after.contentEnd + return `${content.slice(0, lineEnd)}${eol}${text}${content.slice(lineEnd)}` } function canonicalizeLocalProjectPath(projectPath: string): string { @@ -70,28 +185,6 @@ function appendProjectTrustBlock( return `${existing}${separator}${block}${eol}` } -function findProjectHeaderLineEnd(content: string, projectPath: string): number | null { - const lookupPath = normalizeCodexTrustProjectPath(projectPath) - let cursor = 0 - let scanState = createTomlLineScanState() - while (cursor < content.length) { - const newlineIndex = content.indexOf('\n', cursor) - const lineEnd = newlineIndex === -1 ? content.length : newlineIndex - const rawLine = content.slice(cursor, lineEnd) - const line = rawLine.replace(/\r$/, '') - const existingPath = isTomlStructuralLine(scanState) ? parseProjectTomlHeaderPath(line) : null - if (existingPath !== null && normalizeCodexTrustProjectPath(existingPath) === lookupPath) { - return rawLine.endsWith('\r') ? lineEnd - 1 : lineEnd - } - scanState = updateTomlLineScanState(scanState, line) - if (newlineIndex === -1) { - return null - } - cursor = newlineIndex + 1 - } - return null -} - function stripLeadingBom(content: string): string { return content.charCodeAt(0) === 0xfeff ? content.slice(1) : content } diff --git a/src/main/codex/config-toml-runtime-owned-sections.ts b/src/main/codex/config-toml-runtime-owned-sections.ts index cf60c3fe95be..c385b84d2462 100644 --- a/src/main/codex/config-toml-runtime-owned-sections.ts +++ b/src/main/codex/config-toml-runtime-owned-sections.ts @@ -6,8 +6,13 @@ import { updateTomlLineScanState } from './config-toml-line-scan' import { parseTomlTableHeaderPath } from './config-toml-key-path' +import { + CODEX_HOOK_TRUST_KEY, + mayNameTomlKeys, + parseHookStateTomlHeaderKey, + parseStandardTableHeaderSegments +} from './config-toml-syntax' import { findProjectTrustLevelEntries } from './config-toml-project-trust-level' -import { CODEX_HOOK_TRUST_KEY, parseHookStateTomlHeaderKey } from './config-toml-syntax' import { codexHookSourcePathsEqual, getCodexExplicitHomeHookSourcePath, @@ -99,11 +104,14 @@ export function isRuntimePreservedTomlSection(header: string): boolean { } export function isRuntimeHookTrustTomlSection(header: string): boolean { - const table = parseTomlTableHeaderPath(header) + if (!mayNameTomlKeys(header, ['hooks', 'state'])) { + return false + } + const segments = parseStandardTableHeaderSegments(header) // Why: Codex's config writer materializes the parent table on Windows. It is // part of runtime-owned trust and must survive the next config mirror too. // Its `["hooks"."state"]` spelling is the same table (#22592). - return !!table && !table.isArray && table.segments[0] === 'hooks' && table.segments[1] === 'state' + return segments !== null && segments[0] === 'hooks' && segments[1] === 'state' } // Why: user-layer keys name the home's own hooks.json/config.toml; plugin/project keys don't. @@ -169,16 +177,24 @@ export function getMcpServerTomlSectionName(header: string): string | null { export function getTomlSectionHeaderKey(header: string): string { const projectPath = parseCodexProjectHeaderPath(header) return projectPath === null - ? header.trim() + ? getDecodedTomlSectionIdentity(header) : `project:${normalizeCodexProjectPathForLookup(projectPath)}` } +// Why (#22592): spellings of one table share one identity, as they do for Codex. +function getDecodedTomlSectionIdentity(header: string): string { + const table = parseTomlTableHeaderPath(header) + return table + ? `${table.isArray ? 'array' : 'table'}:${JSON.stringify(table.segments)}` + : header.trim() +} + // Why: configs written before WSL tails compared case-sensitively can hold a // revocation under drifted casing; match it loosely so trust is not resurrected. export function getRevocationTomlSectionHeaderKey(header: string): string { const projectPath = parseCodexProjectHeaderPath(header) return projectPath === null - ? header.trim() + ? getDecodedTomlSectionIdentity(header) : `project:${normalizeCodexProjectPathForRevocationLookup(projectPath)}` } @@ -187,7 +203,17 @@ export function getRevocationTomlSectionHeaderKey(header: string): string { export function deduplicateProjectTomlSections(sections: TomlSection[]): TomlSection[] { const deduplicated: TomlSection[] = [] const projectIndexes = new Map() + const hookTrustIdentities = new Set() for (const section of sections) { + if (isRuntimeHookTrustTomlSection(section.header)) { + // Why: two definitions of one hooks.state table make the whole file unreadable to Codex. + const identity = getDecodedTomlSectionIdentity(section.header) + if (!hookTrustIdentities.has(identity)) { + hookTrustIdentities.add(identity) + deduplicated.push(section) + } + continue + } if (!isRuntimeProjectTomlSection(section.header)) { deduplicated.push(section) continue diff --git a/src/main/codex/config-toml-syntax.test.ts b/src/main/codex/config-toml-syntax.test.ts new file mode 100644 index 000000000000..08528d371a54 --- /dev/null +++ b/src/main/codex/config-toml-syntax.test.ts @@ -0,0 +1,43 @@ +import { describe, expect, it } from 'vitest' +import { + parseHookStateTomlHeaderKey, + parseProjectTomlHeaderPath, + parseStandardTableHeaderSegments +} from './config-toml-syntax' +import { isRuntimeHookTrustTomlSection } from './config-toml-runtime-owned-sections' + +function fullProjectParse(line: string): string | null { + const segments = parseStandardTableHeaderSegments(line) + return segments?.length === 2 && segments[0] === 'projects' ? (segments[1] ?? null) : null +} + +describe('header fast paths agree with the full key-path parse', () => { + it.each([ + ['[projects."/work/repo"]', '/work/repo'], + [' [ projects . "/work/repo" ] # note "quoted" ]', '/work/repo'], + ['[projects."/work/a#b]c"]\r', '/work/a#b]c'], + ['[projects."C:\\\\Users\\\\me"]', 'C:\\Users\\me'], + ['["\\u0070rojects"."/work/repo"]', '/work/repo'], + ["['projects'.'/work/repo']", '/work/repo'], + ['["projects"."/work/\\u00e9"]', '/work/é'], + ['[projects."/work/tab\there"]', '/work/tab\there'], + ['[projects."/work/😀"]', '/work/😀'], + ['[[projects."/work/repo"]]', null], + ['[projects."/work/repo".child]', null], + ['[projects."/work/repo"] trailing', null], + ['[model_providers."x"]', null], + ['model = "projects"', null] + ])('parses %j as %j', (line, expected) => { + expect(parseProjectTomlHeaderPath(line)).toBe(expected) + expect(fullProjectParse(line)).toBe(expected) + }) + + it('still recognizes hooks.state tables spelled with escapes', () => { + const escaped = '["hoo\\u006bs"."\\u0073tate"."/h.json:stop:0:0"]' + expect(parseHookStateTomlHeaderKey(escaped)).toBe('/h.json:stop:0:0') + expect(isRuntimeHookTrustTomlSection(escaped)).toBe(true) + expect(isRuntimeHookTrustTomlSection('[hooks.state]')).toBe(true) + expect(isRuntimeHookTrustTomlSection('[projects."/hooks/state"]')).toBe(false) + expect(isRuntimeHookTrustTomlSection('[mcp_servers.x]')).toBe(false) + }) +}) diff --git a/src/main/codex/config-toml-syntax.ts b/src/main/codex/config-toml-syntax.ts index 61c23b0c4e0a..e2d637ddf44a 100644 --- a/src/main/codex/config-toml-syntax.ts +++ b/src/main/codex/config-toml-syntax.ts @@ -20,27 +20,48 @@ export function escapeTomlBasicString(value: string): string { // Why: Codex's hook_key is `{source}:{event}:{group}:{handler}` for any label (unchanged 0.141-0.158). export const CODEX_HOOK_TRUST_KEY = /^(.+):[a-z_]+:(?:0|[1-9]\d*):(?:0|[1-9]\d*)$/ +// Why (#22592): identity is the decoded key path, so `["hooks"."state"."k"]` +// and `[hooks.state.'k']` are the same table Codex sees. export function parseHookStateTomlHeaderKey(line: string): string | null { - return parseTomlTableHeaderLeafKey(line, ['hooks', 'state']) + if (!mayNameTomlKeys(line, ['hooks', 'state'])) { + return null + } + const segments = parseStandardTableHeaderSegments(line) + return segments?.length === 3 && segments[0] === 'hooks' && segments[1] === 'state' + ? (segments[2] ?? null) + : null } +// Only the canonical `[projects."path"]` whose path is printable with no `"` or `\`; else the full parse. +const PLAIN_PROJECT_HEADER = + /^[ \t]*\[[ \t]*projects[ \t]*\.[ \t]*"([ !#-[\]-~\u0080-\uffff]*)"[ \t]*\][ \t]*(?:#.*)?$/ + export function parseProjectTomlHeaderPath(line: string): string | null { - return parseTomlTableHeaderLeafKey(line, ['projects']) + if (!mayNameTomlKeys(line, ['projects'])) { + return null + } + const withoutCr = line.replace(/\r$/, '') + const plain = PLAIN_PROJECT_HEADER.exec(withoutCr) + if (plain) { + return plain[1] ?? null + } + const segments = parseStandardTableHeaderSegments(withoutCr) + return segments?.length === 2 && segments[0] === 'projects' ? (segments[1] ?? null) : null +} + +/** + * Why: header parsing runs per table on every mirror pass. A key spells its + * name literally unless a basic-string escape is used, so a line without the + * names and without a backslash cannot name that table. + */ +export function mayNameTomlKeys(line: string, keys: readonly string[]): boolean { + return line.includes('\\') || keys.every((key) => line.includes(key)) } -// Why (#22592): Codex writes `["projects"."/p"]`; every spelling of the key path must match. -function parseTomlTableHeaderLeafKey(line: string, parent: readonly string[]): string | null { +export function parseStandardTableHeaderSegments(line: string): readonly string[] | null { const header = getTomlTableHeader(line.replace(/\r$/, '')) - const table = header === null ? null : parseTomlTableHeaderPath(header) - if ( - !table || - table.isArray || - table.segments.length !== parent.length + 1 || - parent.some((segment, index) => table.segments[index] !== segment) - ) { - return null - } - return table.segments.at(-1) ?? null + const parsed = header === null ? null : parseTomlTableHeaderPath(header) + return parsed && !parsed.isArray ? parsed.segments : null } export function findNextTomlTableHeader(text: string): number { @@ -99,32 +120,3 @@ function isCompleteTomlTableHeader(line: string): boolean { } return false } - -function unescapeTomlBasicStringEscape(next: string): string { - const escaped: Record = { - n: '\n', - r: '\r', - t: '\t', - b: '\b', - f: '\f', - '"': '"', - '\\': '\\' - } - return escaped[next] ?? `\\${next}` -} - -export function unescapeTomlBasicString(escaped: string): string { - let result = '' - let index = 0 - while (index < escaped.length) { - const char = escaped[index] - if (char === '\\' && index + 1 < escaped.length) { - result += unescapeTomlBasicStringEscape(escaped[index + 1]!) - index += 2 - continue - } - result += char - index += 1 - } - return result -} diff --git a/src/main/codex/config-toml-trust-stale-writes.test.ts b/src/main/codex/config-toml-trust-stale-writes.test.ts index 6a8fb8398dec..e9ccf01aeabc 100644 --- a/src/main/codex/config-toml-trust-stale-writes.test.ts +++ b/src/main/codex/config-toml-trust-stale-writes.test.ts @@ -1,6 +1,7 @@ import { afterEach, describe, expect, it } from 'vitest' import { chmodSync, readFileSync, readdirSync, statSync } from 'node:fs' import * as trustApi from './config-toml-trust' +import { CodexConfigTomlEditRefusedError } from './codex-config-toml-checked-edit' import { createTrustConfigFixture, removeTrustConfigFixture @@ -15,19 +16,15 @@ afterEach(() => { }) describe('config.toml partial and stale writes', () => { - it('preserves a truncated malformed prefix while appending trust', () => { + it('refuses to append trust to a truncated malformed file instead of editing it further', () => { const partial = ['model = "gpt-5"', '[mcp_servers.partial', 'command = "still-user-data'].join( '\n' ) - const updated = trustApi.upsertProjectTrustLevelInContent( - partial, - 'C:/Remote/Repo', - 'trusted', - { alreadyCanonical: true } - ) - - expect(updated.startsWith(partial)).toBe(true) - expect(updated).toContain('[projects."C:/Remote/Repo"]') + expect(() => + trustApi.upsertProjectTrustLevelInContent(partial, 'C:/Remote/Repo', 'trusted', { + alreadyCanonical: true + }) + ).toThrow(CodexConfigTomlEditRefusedError) }) it('pins the no-lock stale-writer policy to one complete last snapshot', () => { diff --git a/src/main/codex/config-toml-trust.ts b/src/main/codex/config-toml-trust.ts index d106a1920f2a..011b4c66470c 100644 --- a/src/main/codex/config-toml-trust.ts +++ b/src/main/codex/config-toml-trust.ts @@ -14,12 +14,18 @@ import { writeTomlConfigAtomically } from './config-toml-atomic-write' import { moveHookTrustContent, removeHookTrustContent, + setHookTrustEnabledContent, upsertHookTrustContent } from './config-toml-hook-trust-edit' import { CodexHookTrustEntryMap, readHookTrustContent } from './config-toml-hook-trust-read' import { upsertProjectTrustContent } from './config-toml-project-trust' import { escapeTomlBasicString, parseProjectTomlHeaderPath } from './config-toml-syntax' import { observe } from './codex-path-observation' +import { parseCodexConfigToml } from './codex-config-toml-document' +import { + assertCodexConfigTomlParses, + CodexConfigTomlEditRefusedError +} from './codex-config-toml-checked-edit' export type CodexEventLabel = | 'pre_tool_use' @@ -108,9 +114,11 @@ export function upsertHookTrustEntries( entries: readonly CodexTrustEntry[] ): void { const existing = readTomlForMutation(configPath) - const updated = upsertHookTrustEntriesInContent(existing, entries) + const updated = withConfigPath(configPath, () => + upsertHookTrustEntriesInContent(existing, entries) + ) if (updated !== existing) { - writeConfigAtomically(configPath, updated) + writeTomlConfigAtomically(configPath, updated) } } @@ -123,9 +131,9 @@ export function moveHookTrustEntries( return } const existing = readTomlForMutation(configPath) - const updated = moveHookTrustContent(existing, moves) + const updated = withConfigPath(configPath, () => moveHookTrustContent(existing, moves)) if (updated !== existing) { - writeConfigAtomically(configPath, updated) + writeTomlConfigAtomically(configPath, updated) } } @@ -136,15 +144,32 @@ export function upsertHookTrustEntriesInContent( return upsertHookTrustContent(existingContent, entries) } +/** Mirrors the user's own enabled/disabled choice onto hooks.state tables that already exist. */ +export function setHookTrustEnabledStates( + configPath: string, + states: readonly { key: string; enabled: boolean }[] +): void { + if (states.length === 0 || !existsSync(configPath)) { + return + } + const existing = readTomlFile(configPath) + const updated = withConfigPath(configPath, () => setHookTrustEnabledContent(existing, states)) + if (updated !== existing) { + writeTomlConfigAtomically(configPath, updated) + } +} + export function upsertProjectTrustLevel( configPath: string, projectPath: string, trustLevel: CodexProjectTrustLevel ): void { const existing = readTomlForMutation(configPath) - const updated = upsertProjectTrustLevelInContent(existing, projectPath, trustLevel) + const updated = withConfigPath(configPath, () => + upsertProjectTrustLevelInContent(existing, projectPath, trustLevel) + ) if (updated !== existing) { - writeConfigAtomically(configPath, updated) + writeTomlConfigAtomically(configPath, updated) } } @@ -169,18 +194,38 @@ export function parseCodexProjectHeaderPath(line: string): string | null { return parseProjectTomlHeaderPath(line) } -export function writeConfigAtomically(configPath: string, contents: string): void { +/** Writes a whole-file edit, refusing one that would turn a config Codex can read into one it cannot. */ +export function writeConfigAtomically( + configPath: string, + contents: string, + previousContents?: string +): void { + if (previousContents === undefined || parseCodexConfigToml(previousContents).ok) { + withConfigPath(configPath, () => assertCodexConfigTomlParses(contents)) + } writeTomlConfigAtomically(configPath, contents) } +function withConfigPath(configPath: string, run: () => T): T { + try { + return run() + } catch (error) { + throw error instanceof CodexConfigTomlEditRefusedError && error.configPath === null + ? error.forConfigPath(configPath) + : error + } +} + export function removeHookTrustEntries(configPath: string, keys: readonly string[]): void { if (!existsSync(configPath)) { return } const existing = readTomlFile(configPath) - const updated = removeHookTrustEntriesFromContent(existing, keys) + const updated = withConfigPath(configPath, () => + removeHookTrustEntriesFromContent(existing, keys) + ) if (updated !== existing) { - writeConfigAtomically(configPath, updated) + writeTomlConfigAtomically(configPath, updated) } } diff --git a/src/main/codex/hook-service-refused-trust-legacy-sweep.test.ts b/src/main/codex/hook-service-refused-trust-legacy-sweep.test.ts new file mode 100644 index 000000000000..0284a4dc9301 --- /dev/null +++ b/src/main/codex/hook-service-refused-trust-legacy-sweep.test.ts @@ -0,0 +1,161 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { mkdirSync, readFileSync, writeFileSync } from 'node:fs' +import type * as Os from 'node:os' +import { join } from 'node:path' +import { + computeTrustKey, + computeTrustedHash, + readHookTrustEntriesFromContent, + type CodexTrustEntry +} from './config-toml-trust' +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) => { + const actual = await importOriginal() + return { + ...actual, + homedir: homedirMock + } +}) + +import { CodexHookService } from './hook-service' + +const homes = setupCodexHookHomes(homedirMock, getPathMock) + +// Why a retired form: the sweep removes only commands no current build writes. +function retiredManagedHookCommand(): string { + if (process.platform === 'win32') { + return join(homes.userDataDir, 'agent-hooks', 'codex-hook.cmd') + } + const quoted = `'${join(homes.tmpHome, '.orca', 'agent-hooks', 'codex-hook.sh')}'` + return `if [ -x ${quoted} ]; then /bin/sh ${quoted}; fi` +} + +type Seeded = { + hooksPath: string + tomlPath: string + hooks: string + toml: string + userAtOne: CodexTrustEntry + userAtZero: CodexTrustEntry + retired: CodexTrustEntry +} + +function trustBlock(entry: CodexTrustEntry): string { + return [ + `[hooks.state."${computeTrustKey(entry).replaceAll('\\', '\\\\')}"]`, + `trusted_hash = "${computeTrustedHash(entry)}"` + ].join('\n') +} + +/** A retired Orca entry ahead of a user hook, both trusted, in a config.toml headed by `firstLine`. */ +function seedRetiredEntryAheadOfUserHook(firstLine: string): Seeded { + const systemCodexHome = join(homes.tmpHome, '.codex') + const hooksPath = join(systemCodexHome, 'hooks.json') + const tomlPath = join(systemCodexHome, 'config.toml') + mkdirSync(systemCodexHome, { recursive: true }) + const userHook = { type: 'command', command: 'user-stop-hook', timeout: 30 } + const hooks = `${JSON.stringify( + { + hooks: { + Stop: [ + { hooks: [{ type: 'command', command: retiredManagedHookCommand() }] }, + { hooks: [userHook] } + ] + } + }, + null, + 2 + )}\n` + writeFileSync(hooksPath, hooks, 'utf-8') + const base = { sourcePath: hooksPath, eventLabel: 'stop' as const, handlerIndex: 0 } + const retired = { ...base, groupIndex: 0, command: retiredManagedHookCommand() } + const userAtOne = { ...base, groupIndex: 1, command: userHook.command, timeoutSec: 30 } + const toml = [firstLine, '', trustBlock(retired), '', trustBlock(userAtOne), ''].join('\n') + writeFileSync(tomlPath, toml, 'utf-8') + return { + hooksPath, + tomlPath, + hooks, + toml, + userAtOne, + userAtZero: { ...userAtOne, groupIndex: 0 }, + retired + } +} + +function warnings(warn: ReturnType, text: string): unknown[][] { + return warn.mock.calls.filter( + ([message]) => typeof message === 'string' && message.includes(text) + ) +} + +const HOOK_SETTINGS = [ + ['hooks on', true], + ['hooks off', false] +] as const + +// Why: the managed mirror copies a hand-broken ~/.codex/config.toml, so its trust +// write is refused first; the ~/.codex sweep must still run, and while the trust it +// shifts cannot move, it must wait rather than strand the user's approval. +describe('CodexHookService retired-entry sweep while config.toml is unreadable', () => { + let warn: ReturnType + beforeEach(() => { + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + }) + afterEach(() => { + warn.mockRestore() + }) + + it.each(HOOK_SETTINGS)('with %s, waits and logs once', async (_label, hooksEnabled) => { + // Why `model =`: a hand-broken line no repair may touch. + const seeded = seedRetiredEntryAheadOfUserHook('model =') + const service = new CodexHookService() + + for (let launch = 0; launch < 2; launch++) { + const status = await service.prepareRuntimeHomeForLaunch(undefined, undefined, hooksEnabled) + expect(status.state).toBe('error') + expect(status.detail).not.toContain('..') + expect(status.detail).not.toContain('Run /hooks') + } + + expect(readFileSync(seeded.hooksPath, 'utf-8')).toBe(seeded.hooks) + expect(readFileSync(seeded.tomlPath, 'utf-8')).toBe(seeded.toml) + expect(warnings(warn, 'Waiting to remove retired Orca hook entries')).toHaveLength(1) + expect(warnings(warn, 'failed to clean legacy Codex hooks')).toHaveLength(0) + }) + + it.each(HOOK_SETTINGS)( + 'with %s, sweeps and moves the approval once the file is fixed', + async (_label, hooksEnabled) => { + const seeded = seedRetiredEntryAheadOfUserHook('model =') + const service = new CodexHookService() + await service.prepareRuntimeHomeForLaunch(undefined, undefined, hooksEnabled) + + writeFileSync(seeded.tomlPath, seeded.toml.replace('model =', 'model = "gpt-5"'), 'utf-8') + await service.prepareRuntimeHomeForLaunch(undefined, undefined, hooksEnabled) + + expect(JSON.parse(readFileSync(seeded.hooksPath, 'utf-8'))).toMatchObject({ + hooks: { Stop: [{ hooks: [{ type: 'command', command: 'user-stop-hook', timeout: 30 }] }] } + }) + const toml = readFileSync(seeded.tomlPath, 'utf-8') + const trust = readHookTrustEntriesFromContent(toml) + expect(trust.get(computeTrustKey(seeded.userAtZero))?.trustedHash).toBe( + computeTrustedHash(seeded.userAtOne) + ) + expect(trust.has(computeTrustKey(seeded.userAtOne))).toBe(false) + expect(toml).not.toContain(computeTrustedHash(seeded.retired)) + } + ) +}) diff --git a/src/main/codex/hook-service-user-hook-mirroring.test.ts b/src/main/codex/hook-service-user-hook-mirroring.test.ts index f13953a60514..2f0d8c1df1a7 100644 --- a/src/main/codex/hook-service-user-hook-mirroring.test.ts +++ b/src/main/codex/hook-service-user-hook-mirroring.test.ts @@ -579,3 +579,29 @@ describe('CodexHookService', () => { expect(runtimeToml).not.toContain(':permission_request:0:0') }) }) + +describe('hook trust against a ~/.codex config the user broke by hand', () => { + function seedHandBrokenSystemConfig(): void { + const systemCodexHome = join(homes.tmpHome, '.codex') + mkdirSync(systemCodexHome, { recursive: true }) + writeFileSync( + join(systemCodexHome, 'hooks.json'), + `${JSON.stringify({ hooks: { Stop: [{ hooks: [{ type: 'command', command: 'user-hook' }] }] } })}\n`, + 'utf-8' + ) + writeFileSync(join(systemCodexHome, 'config.toml'), 'model = "a"\nbroken = \n', 'utf-8') + } + + it('refreshes user hooks with no trust to write without reporting an error', async () => { + seedHandBrokenSystemConfig() + const status = await new CodexHookService().refreshRuntimeUserHooks() + expect(status.state).not.toBe('error') + }) + + it('reports a refused managed trust write in the install status', async () => { + seedHandBrokenSystemConfig() + const status = await new CodexHookService().install() + expect(status.state).toBe('error') + expect(status.detail).toContain('config.toml unchanged') + }) +}) diff --git a/src/main/execution-host-workspace-trust.ts b/src/main/execution-host-workspace-trust.ts index 52562e68064c..a85a74ccd34a 100644 --- a/src/main/execution-host-workspace-trust.ts +++ b/src/main/execution-host-workspace-trust.ts @@ -15,6 +15,7 @@ import { grantClaudeWorkspaceTrust } from './claude/claude-folder-trust-file' import { isTooBroadToPreTrust } from '../shared/home-or-filesystem-root' +import { reportCodexTrustWriteRefusals } from './codex/codex-config-toml-checked-edit' /** * What the host that runs the agent knows about where that agent reads trust. Relay-safe: @@ -137,9 +138,13 @@ export async function applyWorkspaceTrustOnThisHost( deadlineMs: host.deadlineMs }) } catch (error) { - console.warn( - `[agent-trust] ${preset} trust for ${workspacePath} failed; the agent will ask`, - error - ) + // Why: a refused Codex config edit recurs every launch, so it logs once per file and message. + const unreported = reportCodexTrustWriteRefusals(error) + if (unreported.length > 0) { + console.warn( + `[agent-trust] ${preset} trust for ${workspacePath} failed; the agent will ask`, + ...unreported + ) + } } } diff --git a/src/main/startup/codex-launch-hook-status-warning.test.ts b/src/main/startup/codex-launch-hook-status-warning.test.ts new file mode 100644 index 000000000000..18a3a96a56fa --- /dev/null +++ b/src/main/startup/codex-launch-hook-status-warning.test.ts @@ -0,0 +1,64 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => ({ + prepareRuntimeHomeForLaunch: vi.fn( + async (): Promise<{ state: string; configPath?: string; detail?: string }> => ({ state: 'ok' }) + ) +})) + +vi.mock('electron', () => ({ app: { getPath: vi.fn(() => '/tmp/orca-user-data') } })) +vi.mock('../codex/hook-service', () => ({ + codexHookService: { prepareRuntimeHomeForLaunch: mocks.prepareRuntimeHomeForLaunch } +})) +vi.mock('../codex/codex-real-home-hook-install', () => ({ + ensureRealHomeCodexHookState: vi.fn(async () => {}) +})) +vi.mock('../agent-hooks/managed-agent-hook-controls', () => ({ + isAgentStatusHooksEnabledForAgent: () => true +})) +vi.mock('../wsl', () => ({ getDefaultWslDistro: () => 'Ubuntu' })) +vi.mock('./main-process-state', () => ({ + mainProcessState: { + codexRuntimeHome: { + prepareForCodexLaunchAsync: vi.fn(async () => '/managed/.codex'), + isHostSystemDefaultRealHomeSelected: () => false + }, + store: { getSettings: () => ({}) } + } +})) + +import { prepareCodexRuntimeHomeForLaunch } from './codex-launch-preparation' + +describe('Codex launch-prep hook status warning', () => { + let warn: ReturnType + beforeEach(() => { + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + }) + afterEach(() => { + warn.mockRestore() + }) + + it('logs a repeated hook status error once and a new one again', async () => { + const detail = + 'Hooks installed but trust entries could not be written: Orca left /tmp/launch-hook-trust-probe/config.toml unchanged (line 2): the file is not valid TOML.' + const refused = { + state: 'error', + configPath: '/tmp/launch-hook-trust-probe/hooks.json', + detail + } + mocks.prepareRuntimeHomeForLaunch.mockResolvedValue(refused) + for (let launch = 0; launch < 3; launch++) { + await prepareCodexRuntimeHomeForLaunch() + } + expect(warn).toHaveBeenCalledTimes(1) + expect(String(warn.mock.calls[0]?.[0])).toContain('[codex-hook-service]') + expect(String(warn.mock.calls[0]?.[0])).toContain('(line 2)') + + mocks.prepareRuntimeHomeForLaunch.mockResolvedValue({ + ...refused, + detail: detail.replace('line 2', 'line 5') + }) + await prepareCodexRuntimeHomeForLaunch() + expect(warn).toHaveBeenCalledTimes(2) + }) +}) diff --git a/src/main/startup/codex-launch-preparation.ts b/src/main/startup/codex-launch-preparation.ts index b70d61396840..1e8ae4ef753e 100644 --- a/src/main/startup/codex-launch-preparation.ts +++ b/src/main/startup/codex-launch-preparation.ts @@ -1,6 +1,7 @@ import { app } from 'electron' import type { CodexHomeLaunchContext } from '../ipc/pty' import type { CodexAccountSelectionTarget } from '../codex-accounts/runtime-selection' +import { warnCodexConfigOnce } from '../codex/codex-config-toml-checked-edit' import { codexHookService } from '../codex/hook-service' import { getDefaultWslDistro } from '../wsl' import { isAgentStatusHooksEnabledForAgent } from '../agent-hooks/managed-agent-hook-controls' @@ -68,9 +69,10 @@ export async function prepareCodexRuntimeHomeForLaunch( hooksEnabled ) if (status.state === 'error') { - console.warn( - `[codex-hook-service] failed to ${hooksEnabled ? 'refresh' : 'refresh user'} runtime hooks before launch`, - status.detail + // Why: a refused trust write returns the same detail on every launch until the user fixes the file. + warnCodexConfigOnce( + status.configPath, + `[codex-hook-service] failed to ${hooksEnabled ? 'refresh' : 'refresh user'} runtime hooks before launch: ${status.detail}` ) } } catch (error) { diff --git a/src/relay/agent-workspace-trust-spawn.test.ts b/src/relay/agent-workspace-trust-spawn.test.ts index d7f20d2961c4..2299181d754e 100644 --- a/src/relay/agent-workspace-trust-spawn.test.ts +++ b/src/relay/agent-workspace-trust-spawn.test.ts @@ -231,6 +231,55 @@ describe('applyRelayAgentWorkspaceTrust', () => { expect(warn).toHaveBeenCalled() warn.mockRestore() }) + + it('edits the host’s quoted ["projects"."…"] table instead of appending a duplicate (#22592)', async () => { + const configPath = join(home, '.codex', 'config.toml') + mkdirSync(join(home, '.codex')) + writeFileSync(configPath, `["projects"."${workspace}"]\ntrust_level = "untrusted"\n`) + await applyRelayAgentWorkspaceTrust( + { workspacePath: workspace }, + 'codex', + { HOME: home }, + HOST_SHELL + ) + expect(readFileSync(configPath, 'utf-8')).toBe( + `["projects"."${workspace}"]\ntrust_level = "trusted"\n` + ) + }) + + describe('with a config.toml Codex cannot parse', () => { + const unreadable = '[a]\nx = 1\n[a]\ny = 2\n' + let configPath: string + let warn: ReturnType + beforeEach(() => { + configPath = join(home, '.codex', 'config.toml') + mkdirSync(join(home, '.codex')) + writeFileSync(configPath, unreadable) + warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + }) + afterEach(() => { + warn.mockRestore() + }) + const attempt = (): Promise => + applyRelayAgentWorkspaceTrust( + { workspacePath: workspace }, + 'codex', + { HOME: home }, + HOST_SHELL + ) + + it('never writes it and names the file in the log', async () => { + await expect(attempt()).resolves.toBeUndefined() + expect(readFileSync(configPath, 'utf-8')).toBe(unreadable) + expect(String(warn.mock.calls[0]?.[0])).toContain(configPath) + }) + + it('logs the recurring refusal once', async () => { + await attempt() + await attempt() + expect(warn).toHaveBeenCalledTimes(1) + }) + }) }) describe('buildSshPtySpawnRequest', () => {