From 88d5469c53e9b9b1977ca4fccadc898ecef8f849 Mon Sep 17 00:00:00 2001 From: ggbdpq Date: Fri, 2 Oct 2026 21:35:35 +0800 Subject: [PATCH] fix(codex): remove Orca-created pre-trust entries when a worktree is removed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every `worker-start --agent codex` into a worktree pre-trusts the worktree path in the managed Codex account's `config.toml`, and nothing ever removed the entry: weeks of dispatches left 95 of 110 `[projects."…"]` tables pointing at deleted paths (#24697). Worktree names are reused (`-r1`, `-r2`), so a stale `trusted` entry is a real hazard — whatever appears next at that path is born trusted. Orca now records, at grant time, which project tables its preflight created (only when the table did not already exist there — a user-written or pre-existing entry is never recorded and never touched), in a small Orca-side ledger next to the existing agent state, following the same bookkeeping shape as the managed-home resource-copy markers. When a worktree is removed, the entries recorded for its bare path are deleted whole-table, exactly where all removal paths (managed, orphan, folder, preserved-branch, interrupted) already converge; the grant keyed the bare path, so a folder session's `::workspace:` id suffix is resolved away first. Every failure mode degrades toward "do not delete": an unreadable ledger or config means nothing is removed, a failed table removal keeps its record for the next attempt, and a config file that no longer exists just forgets its records. Cleanup of orphaned entries from before this ledger exists is left as follow-up work; the ledger deliberately only ever vouches for what Orca itself wrote. Fixes #24697 Generated-by: GLM-5.3-Flash (ZCode) --- .../agent-launch/trust/agent-trust-presets.ts | 6 +- .../codex-project-trust-ownership.test.ts | 99 +++++++++++++++++++ .../codex/codex-project-trust-ownership.ts | 94 ++++++++++++++++++ .../trust/toml/config-toml-project-trust.ts | 23 +++++ .../codex/trust/toml/config-toml-trust.ts | 86 +++++++++++++++- .../workspaces/workspace-removal-target.ts | 12 ++- ...ree-removal-codex-pretrust-cleanup.test.ts | 79 +++++++++++++++ 7 files changed, 392 insertions(+), 7 deletions(-) create mode 100644 src/main/codex/codex-project-trust-ownership.test.ts create mode 100644 src/main/codex/codex-project-trust-ownership.ts create mode 100644 src/main/runtime/worktree-removal-codex-pretrust-cleanup.test.ts diff --git a/src/main/agent-launch/trust/agent-trust-presets.ts b/src/main/agent-launch/trust/agent-trust-presets.ts index 4f3b2ca299b..a4a035ac6fd 100644 --- a/src/main/agent-launch/trust/agent-trust-presets.ts +++ b/src/main/agent-launch/trust/agent-trust-presets.ts @@ -2,9 +2,9 @@ import { createHash } from 'node:crypto' import { existsSync, mkdirSync, readFileSync, realpathSync } from 'node:fs' import { dirname, join, resolve } from 'node:path' import { writeFileAtomically } from '../../codex-accounts/fs-utils' -import { upsertProjectTrustLevel } from '../../codex/trust/toml/config-toml-trust' +import { upsertProjectTrustLevel, upsertOrcaCreatedProjectTrustLevel } from '../../codex/trust/toml/config-toml-trust' import { runExclusivelyForCodexTrustConfig } from '../../codex/trust/toml/codex-trust-config-mutation-queue' -import type { TuiAgentConfig } from '../../../shared/agent-launch/catalog/tui-agent-config-types' +import type { TuiAgentConfig } from '../../../shared/agent-launch/catalog/tui-agent-config-types' export type AgentTrustPreset = NonNullable @@ -136,7 +136,7 @@ export function markCodexProjectTrusted( (inner, configFile) => () => runExclusivelyForCodexTrustConfig(configFile, inner), async () => { for (const configFile of configFiles) { - upsertProjectTrustLevel(configFile, absPath, 'trusted') + upsertOrcaCreatedProjectTrustLevel(configFile, absPath, 'trusted') } } ) diff --git a/src/main/codex/codex-project-trust-ownership.test.ts b/src/main/codex/codex-project-trust-ownership.test.ts new file mode 100644 index 00000000000..3ce3052f8a4 --- /dev/null +++ b/src/main/codex/codex-project-trust-ownership.test.ts @@ -0,0 +1,99 @@ +/** + * Ownership for the [projects.""] tables Orca's preflight trust writes: + * a removed worktree's entry must die with the worktree, while a table that + * existed at the path before Orca first wrote it stays the user's. + */ +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { + removeOrcaCreatedProjectTrustEntries, + upsertOrcaCreatedProjectTrustLevel +} from './config-toml-trust' + +const PROJECT = '/tmp/worktrees/feature-r1' +const OTHER = '/tmp/worktrees/other-r1' + +describe('Orca-created project trust lifecycle', () => { + let userDataDir: string + let managedConfigPath: string + let accountConfigPath: string + let previousUserDataPath: string | undefined + + beforeEach(() => { + userDataDir = mkdtempSync(join(tmpdir(), 'orca-pretrust-ownership-')) + previousUserDataPath = process.env.ORCA_USER_DATA_PATH + process.env.ORCA_USER_DATA_PATH = userDataDir + const managedHome = join(userDataDir, 'codex-runtime-home', 'home') + const accountHome = join(userDataDir, 'codex-accounts', 'acct-1', 'home') + mkdirSync(managedHome, { recursive: true }) + mkdirSync(accountHome, { recursive: true }) + managedConfigPath = join(managedHome, 'config.toml') + accountConfigPath = join(accountHome, 'config.toml') + }) + + afterEach(() => { + rmSync(userDataDir, { recursive: true, force: true }) + if (previousUserDataPath === undefined) { + delete process.env.ORCA_USER_DATA_PATH + } else { + process.env.ORCA_USER_DATA_PATH = previousUserDataPath + } + }) + + it('removes the tables Orca created for a removed worktree, per account, and keeps other paths', async () => { + upsertOrcaCreatedProjectTrustLevel(managedConfigPath, PROJECT, 'trusted') + upsertOrcaCreatedProjectTrustLevel(accountConfigPath, PROJECT, 'trusted') + upsertOrcaCreatedProjectTrustLevel(managedConfigPath, OTHER, 'trusted') + expect(readFileSync(managedConfigPath, 'utf-8')).toContain(`[projects."${PROJECT}"]`) + + await removeOrcaCreatedProjectTrustEntries(PROJECT) + + for (const configPath of [managedConfigPath, accountConfigPath]) { + expect(readFileSync(configPath, 'utf-8')).not.toContain(`[projects."${PROJECT}"]`) + } + expect(readFileSync(managedConfigPath, 'utf-8')).toContain(`[projects."${OTHER}"]`) + // The record was forgotten, so removing again cannot touch surviving entries. + await removeOrcaCreatedProjectTrustEntries(PROJECT) + expect(readFileSync(managedConfigPath, 'utf-8')).toContain(`[projects."${OTHER}"]`) + }) + + it('keeps a table that existed at the path before Orca wrote it', async () => { + const userTable = [`[projects."${PROJECT}"]`, 'trust_level = "untrusted"', ''].join('\n') + writeFileSync(managedConfigPath, userTable) + writeFileSync(accountConfigPath, userTable) + + // The preflight grant still rewrites the trust line it always did… + upsertOrcaCreatedProjectTrustLevel(managedConfigPath, PROJECT, 'trusted') + + // …but the worktree removal must not delete the pre-existing tables. + await removeOrcaCreatedProjectTrustEntries(PROJECT) + + expect(readFileSync(managedConfigPath, 'utf-8')).toBe( + [`[projects."${PROJECT}"]`, 'trust_level = "trusted"', ''].join('\n') + ) + expect(readFileSync(accountConfigPath, 'utf-8')).toBe( + [`[projects."${PROJECT}"]`, 'trust_level = "untrusted"', ''].join('\n') + ) + }) + + it('leaves entries with no ownership record (older Orca builds) untouched', async () => { + const legacyTable = [`[projects."${PROJECT}"]`, 'trust_level = "trusted"', ''].join('\n') + writeFileSync(managedConfigPath, legacyTable) + + await removeOrcaCreatedProjectTrustEntries(PROJECT) + + expect(readFileSync(managedConfigPath, 'utf-8')).toBe(legacyTable) + }) + + it('forgets the record once the config file itself is gone, so a rebuilt path is never purged', async () => { + upsertOrcaCreatedProjectTrustLevel(managedConfigPath, PROJECT, 'trusted') + rmSync(managedConfigPath) + + await expect(removeOrcaCreatedProjectTrustEntries(PROJECT)).resolves.toBeUndefined() + + const ledger = readFileSync(join(userDataDir, 'codex-project-trust-created.json'), 'utf-8') + expect(ledger).not.toContain(PROJECT) + }) +}) diff --git a/src/main/codex/codex-project-trust-ownership.ts b/src/main/codex/codex-project-trust-ownership.ts new file mode 100644 index 00000000000..597e3631cb1 --- /dev/null +++ b/src/main/codex/codex-project-trust-ownership.ts @@ -0,0 +1,94 @@ +import { mkdirSync } from 'node:fs' +import { dirname, join } from 'node:path' +import { readAgentStateJsonFileSync } from '../agent-state-file-reader' +import { writeFileAtomically } from '../codex-accounts/fs-utils' +import { getOrcaUserDataPath } from './codex-home-paths' +import { normalizeCodexTrustProjectPath } from './codex-trust-identity' + +/** + * Ownership records for the `[projects.""]` tables Orca's preflight trust + * created, so a removed worktree's entry can be deleted without ever touching a + * table the user (or an older Orca build) had at that path. Same shape of + * bookkeeping as the managed-home resource-copy markers: Orca-side state that + * distinguishes what Orca wrote from what it only found. + */ + +const CREATED_PROJECT_TRUST_LEDGER_FILE = 'codex-project-trust-created.json' + +type CreatedProjectTrustLedger = { + version: 1 + /** Config file path → project paths whose table Orca's grant created there. */ + created: Record +} + +function getLedgerPath(): string { + return join(getOrcaUserDataPath(), CREATED_PROJECT_TRUST_LEDGER_FILE) +} + +// Why: the ledger only decides what extra cleanup may happen, so any unreadable +// state degrades to "nothing recorded", never to deleting an unrecorded entry. +function readLedger(): CreatedProjectTrustLedger { + try { + const parsed = readAgentStateJsonFileSync(getLedgerPath()) as Partial + return parsed && typeof parsed === 'object' && typeof parsed.created === 'object' + ? { version: 1, created: parsed.created } + : { version: 1, created: {} } + } catch { + return { version: 1, created: {} } + } +} + +function writeLedger(ledger: CreatedProjectTrustLedger): void { + const ledgerPath = getLedgerPath() + mkdirSync(dirname(ledgerPath), { recursive: true }) + writeFileAtomically(ledgerPath, `${JSON.stringify(ledger, null, 2)}\n`, { mode: 0o600 }) +} + +function isRecordedProjectTrust(recorded: string, projectPath: string): boolean { + return normalizeCodexTrustProjectPath(recorded) === normalizeCodexTrustProjectPath(projectPath) +} + +function hasRecord( + ledger: CreatedProjectTrustLedger, + configPath: string, + projectPath: string +): boolean { + return (ledger.created[configPath] ?? []).some((recorded) => + isRecordedProjectTrust(recorded, projectPath) + ) +} + +/** Records that Orca's grant created the project table at `projectPath` in `configPath`. */ +export function recordOrcaCreatedProjectTrust(configPath: string, projectPath: string): void { + const ledger = readLedger() + if (hasRecord(ledger, configPath, projectPath)) { + return + } + ledger.created[configPath] = [...(ledger.created[configPath] ?? []), projectPath] + writeLedger(ledger) +} + +/** Config files whose project table at `projectPath` was created by Orca's grant. */ +export function listOrcaCreatedProjectTrustConfigFiles(projectPath: string): string[] { + const ledger = readLedger() + return Object.entries(ledger.created) + .filter(([, paths]) => paths.some((recorded) => isRecordedProjectTrust(recorded, projectPath))) + .map(([configPath]) => configPath) +} + +/** Drops the ownership record once the entry is gone (or its config file is). */ +export function forgetOrcaCreatedProjectTrust(configPath: string, projectPath: string): void { + const ledger = readLedger() + if (!hasRecord(ledger, configPath, projectPath)) { + return + } + const remaining = (ledger.created[configPath] ?? []).filter( + (recorded) => !isRecordedProjectTrust(recorded, projectPath) + ) + if (remaining.length === 0) { + delete ledger.created[configPath] + } else { + ledger.created[configPath] = remaining + } + writeLedger(ledger) +} diff --git a/src/main/codex/trust/toml/config-toml-project-trust.ts b/src/main/codex/trust/toml/config-toml-project-trust.ts index cf086351b15..95d941126be 100644 --- a/src/main/codex/trust/toml/config-toml-project-trust.ts +++ b/src/main/codex/trust/toml/config-toml-project-trust.ts @@ -44,6 +44,29 @@ export function upsertProjectTrustContent( return `${existing.slice(0, headerLineEnd)}${eol}${trustLine}${existing.slice(headerLineEnd)}` } +/** Whether any `[projects.""]` table for the path already exists, in any spelling. */ +export function hasProjectTrustEntry(content: string, projectPath: string): boolean { + return findProjectHeaderLineEnd(content, projectPath) !== null +} + +/** + * Removes every `[projects.""]` table for the path, leaving all other + * bytes untouched: only whole tables go, never a sibling key or table. + */ +export function removeProjectTrustContent(content: string, projectPath: string): string { + let updated = content + for (;;) { + const headerLineEnd = findProjectHeaderLineEnd(updated, projectPath) + if (headerLineEnd === null) { + return updated + } + const headerLineStart = updated.lastIndexOf('\n', Math.max(0, headerLineEnd - 1)) + 1 + const nextHeaderOffset = findNextTomlTableHeader(updated.slice(headerLineEnd)) + const blockEnd = nextHeaderOffset === -1 ? updated.length : headerLineEnd + nextHeaderOffset + updated = updated.slice(0, headerLineStart) + updated.slice(blockEnd) + } +} + function canonicalizeLocalProjectPath(projectPath: string): string { try { return realpathSync.native(projectPath) diff --git a/src/main/codex/trust/toml/config-toml-trust.ts b/src/main/codex/trust/toml/config-toml-trust.ts index c0b57a37104..989a7f3d9df 100644 --- a/src/main/codex/trust/toml/config-toml-trust.ts +++ b/src/main/codex/trust/toml/config-toml-trust.ts @@ -15,9 +15,20 @@ import { CodexHookTrustEntryMap, readHookTrustContent as readHookTrustEntriesFromContent } from './config-toml-hook-trust-read' -import { upsertProjectTrustContent as upsertProjectTrustLevelInContent } from './config-toml-project-trust' -import '../../configuration/toml/config-toml-syntax' +import { + hasProjectTrustEntry, + removeProjectTrustContent, + upsertProjectTrustContent as upsertProjectTrustLevelInContent +} from './config-toml-project-trust' +import { + forgetOrcaCreatedProjectTrust, + listOrcaCreatedProjectTrustConfigFiles, + recordOrcaCreatedProjectTrust +} from './codex-project-trust-ownership' +import { runExclusivelyForCodexTrustConfig } from './codex-trust-config-mutation-queue' import { observe } from '../../configuration/codex-path-observation' +import '../../configuration/toml/config-toml-syntax' +import { escapeTomlBasicString, parseProjectTomlHeaderPath } from '../../configuration/toml/config-toml-syntax' export type CodexEventLabel = | 'pre_tool_use' @@ -166,6 +177,77 @@ export function upsertProjectTrustLevel( } } +/** + * Grants project trust and records the tables this grant created, so a removed + * worktree's entry can be deleted later without touching a table that existed + * at the path before Orca first wrote it. + */ +export function upsertOrcaCreatedProjectTrustLevel( + configPath: string, + projectPath: string, + trustLevel: CodexProjectTrustLevel +): void { + const created = !hasProjectTrustEntry(readTomlForMutation(configPath), projectPath) + upsertProjectTrustLevel(configPath, projectPath, trustLevel) + if (!created) { + return + } + try { + recordOrcaCreatedProjectTrust(configPath, projectPath) + } catch (error) { + // Why: the grant already landed; an unrecorded entry only loses cleanup, never safety. + console.warn('[codex-config] Failed to record the created project trust entry:', error) + } +} + +/** + * Removes every Orca-created project trust entry for `projectPath`, wherever + * recorded. Never throws: removal rides the worktree-removal path, which must + * not fail over a config file it cannot read — the record is kept so a later + * pass retries. Resolves once every touched file has been written. + */ +export async function removeOrcaCreatedProjectTrustEntries(projectPath: string): Promise { + for (const configPath of listOrcaCreatedProjectTrustConfigFiles(projectPath)) { + await runExclusivelyForCodexTrustConfig(configPath, async () => { + try { + removeOrcaCreatedProjectTrustEntry(configPath, projectPath) + } catch (error) { + console.warn('[codex-config] Failed to remove an Orca-created project trust entry:', error) + } + }) + } +} + +function removeOrcaCreatedProjectTrustEntry(configPath: string, projectPath: string): void { + // Why: same lane as the grant writers (#16441), so a queued writer cannot rewrite the entry back. + const observation = observe(() => readTomlFile(configPath)) + if (observation.kind === 'indeterminate') { + return + } + if (observation.kind === 'absent') { + forgetOrcaCreatedProjectTrust(configPath, projectPath) + return + } + const updated = removeProjectTrustContent(observation.value, projectPath) + if (updated !== observation.value) { + writeConfigAtomically(configPath, updated) + } + forgetOrcaCreatedProjectTrust(configPath, projectPath) +} + +export function escapeTomlString(value: string): string { + return escapeTomlBasicString(value) +} + +export function normalizeHookTrustKeyForLookup(key: string): string { + return normalizeCodexHookTrustLookupKey(key) +} + +export function parseCodexProjectHeaderPath(line: string): string | null { + return parseProjectTomlHeaderPath(line) +} + + export function removeHookTrustEntries(configPath: string, keys: readonly string[]): void { if (!existsSync(configPath)) { return diff --git a/src/main/runtime/workspaces/workspace-removal-target.ts b/src/main/runtime/workspaces/workspace-removal-target.ts index a1998621818..4d03e861856 100644 --- a/src/main/runtime/workspaces/workspace-removal-target.ts +++ b/src/main/runtime/workspaces/workspace-removal-target.ts @@ -8,8 +8,8 @@ import type { import { resolveRuntimeWorktreeRemovalTarget } from './removal/runtime-worktree-removal-target' import type { LocalProjectWorktreeGitOptions } from '../../workspaces/execution-host/project-runtime-git-options' import type { RuntimeStore } from '../runtime-store-contract' -import { splitWorktreeId, worktreeIdsEqual } from '../../../shared/worktree/id' -import { hasWorktreeRemovalRepoOwnerOnOtherHost } from '../../workspaces/removal/worktree-removal-repo-owner' +import { splitWorktreeId, splitWorktreeIdForFilesystem, worktreeIdsEqual } from '../../../shared/worktree/id' +import { hasWorktreeRemovalRepoOwnerOnOtherHost } from '../../workspaces/removal/worktree-removal-repo-owner' import { dropRemovedWorktreeProcessState, pruneRemovedWorktreeSnapshots @@ -34,6 +34,7 @@ import { type ExecutionHostId } from '../../../shared/execution-host' import { invalidateAuthorizedRootsCacheForRepo } from '../../ipc/worktrees/removal/registered-worktree-roots-scoped-invalidation' +import { removeOrcaCreatedProjectTrustEntries } from '../../codex/trust/toml/config-toml-trust' import { resumeInterruptedWorktreeRemovals, retryFailedWorktreeRemoval, @@ -159,6 +160,13 @@ export abstract class OrcaRuntimeWithResolveWorktreeRemovalTarget extends OrcaRu hostId?: ExecutionHostId ): void { // Purge history/caches before path-derived ids can name a recreated workspace. + // The Codex pretrust entry Orca wrote for the path is the same hazard: drop + // it so a recreated worktree is not born pre-trusted (#24697). The grant + // keyed the bare path, so a folder session's ::workspace: suffix must not hide it. + const worktreePath = splitWorktreeIdForFilesystem(worktreeId)?.worktreePath + if (worktreePath) { + void removeOrcaCreatedProjectTrustEntries(worktreePath) + } const persistedHostId = store.getWorktreeMeta(worktreeId)?.hostId const repoId = splitWorktreeId(worktreeId)?.repoId const preservesSameIdOwner = Boolean( diff --git a/src/main/runtime/worktree-removal-codex-pretrust-cleanup.test.ts b/src/main/runtime/worktree-removal-codex-pretrust-cleanup.test.ts new file mode 100644 index 00000000000..c7e2378736d --- /dev/null +++ b/src/main/runtime/worktree-removal-codex-pretrust-cleanup.test.ts @@ -0,0 +1,79 @@ +/** + * Worktree removal drops the Codex project trust entries Orca created for the + * worktree's path: path-derived ids get reused, so a stale entry would + * pre-trust whatever occupies the path next. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { mkdirSync, mkdtempSync, readFileSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { OrcaRuntimeService } from './orca-runtime' +import { upsertOrcaCreatedProjectTrustLevel } from '../codex/config-toml-trust' + +const WORKTREE_ID = 'repo-1::/tmp/worktrees/feature-r1' +const PROJECT = '/tmp/worktrees/feature-r1' +// Why: folder-project workspace sessions carry this suffix on an otherwise path-derived id. +const FOLDER_SUFFIX = '::workspace:5e8cc6b2-1d3f-4a5b-9c2d-7f6e5a4b3c2d' + +function makeRuntimePurge() { + const store = { + getWorktreeMeta: () => ({ instanceId: 'old-instance' }), + removeWorktreeMeta: () => {}, + getRepo: () => undefined, + getSettings: () => ({}) + } + const runtime = new OrcaRuntimeService(store as never) + return (worktreeId: string) => + ( + runtime as unknown as { + removeWorktreeMetadataAndHistory: (store: unknown, worktreeId: string) => void + } + ).removeWorktreeMetadataAndHistory(store, worktreeId) +} + +describe('worktree removal cleans up the Codex pretrust Orca wrote', () => { + let userDataDir: string + let configPath: string + let previousUserDataPath: string | undefined + + beforeEach(() => { + userDataDir = mkdtempSync(join(tmpdir(), 'orca-pretrust-removal-')) + previousUserDataPath = process.env.ORCA_USER_DATA_PATH + process.env.ORCA_USER_DATA_PATH = userDataDir + const managedHome = join(userDataDir, 'codex-runtime-home', 'home') + mkdirSync(managedHome, { recursive: true }) + configPath = join(managedHome, 'config.toml') + }) + + afterEach(() => { + rmSync(userDataDir, { recursive: true, force: true }) + if (previousUserDataPath === undefined) { + delete process.env.ORCA_USER_DATA_PATH + } else { + process.env.ORCA_USER_DATA_PATH = previousUserDataPath + } + }) + + it('removes the recorded entry when the worktree metadata is purged', async () => { + upsertOrcaCreatedProjectTrustLevel(configPath, PROJECT, 'trusted') + const purge = makeRuntimePurge() + + purge(WORKTREE_ID) + + // The purge fires the cleanup through the trust-config queue, so poll for the write. + await vi.waitFor(() => { + expect(readFileSync(configPath, 'utf-8')).not.toContain(`[projects."${PROJECT}"]`) + }) + }) + + it('removes the entry for a folder-workspace session id suffixing the same path', async () => { + upsertOrcaCreatedProjectTrustLevel(configPath, PROJECT, 'trusted') + const purge = makeRuntimePurge() + + purge(`${WORKTREE_ID}${FOLDER_SUFFIX}`) + + await vi.waitFor(() => { + expect(readFileSync(configPath, 'utf-8')).not.toContain(`[projects."${PROJECT}"]`) + }) + }) +})