From a632ea3353341ada8e7eeee98921053d6d7a7e5a Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Thu, 3 Sep 2026 14:24:48 -0700 Subject: [PATCH] Round-3 review fixes: N-1 empty-value regression, N-2 gate leak window, N-4 lost history N-1: my presence-based conflict predicate refused a terminal launch that works today. 'ANTHROPIC_API_KEY=' is how a user blanks a variable and the settings pipeline preserves that empty value; an empty override cannot beat the pinned account and the strip removes the name anyway. Back to truthiness for the value, keeping the win32 case folding. N-2: enter the live-auth gate only after the exit/close handlers that release it, so no throw in between can leave an entry nothing reconciles. N-4: the Claude transcript resolver searches config-dir-then-default and de-dupes, matching the Codex sibling in the same file, so adopting CLAUDE_CONFIG_DIR no longer hides history written before it. --- .../claude-structured-auth-policy.test.ts | 29 +++++- src/main/claude-accounts/environment.ts | 9 +- .../claude-stream-json-connection.test.ts | 43 ++++++++ .../claude/claude-stream-json-connection.ts | 11 ++- ...session-file-resolver-claude-roots.test.ts | 97 +++++++++++++++++++ src/main/native-chat/session-file-resolver.ts | 45 ++++++--- 6 files changed, 212 insertions(+), 22 deletions(-) create mode 100644 src/main/native-chat/session-file-resolver-claude-roots.test.ts diff --git a/src/main/claude-accounts/claude-structured-auth-policy.test.ts b/src/main/claude-accounts/claude-structured-auth-policy.test.ts index 110560cd573..a2d7c7d5da8 100644 --- a/src/main/claude-accounts/claude-structured-auth-policy.test.ts +++ b/src/main/claude-accounts/claude-structured-auth-policy.test.ts @@ -6,6 +6,10 @@ import { hasClaudeAuthEnvConflict, shouldStripClaudeAuthEnvForAccount } from './environment' +import { + normalizeTuiAgentEnvRecord, + resolveTuiAgentLaunchEnv +} from '../../shared/tui-agent-launch-defaults' import { claudeStructuredAuthPolicyForSettings } from './claude-structured-auth-policy' const HOST_ACCOUNT = { id: 'host-a', managedAuthRuntime: 'host' } as ClaudeManagedAccount @@ -117,8 +121,29 @@ describe('hasClaudeAuthEnvConflict matches the strip it guards', () => { } }) - it('refuses an override whose value is empty, because the strip deletes it anyway', () => { - expect(hasClaudeAuthEnvConflict({ ANTHROPIC_API_KEY: '' }, 'linux')).toBe(true) + // `ANTHROPIC_API_KEY=` in the agent env box is how a user blanks a variable, and the + // settings pipeline preserves the empty value (agent-default-env-draft.ts assigns + // everything after the `=`; normalizeTuiAgentEnvRecord drops empty KEYS only). An + // empty value cannot beat the pinned account and the strip removes the name anyway, + // so refusing it would break a terminal launch that works today for no security gain. + it('admits an override whose value is empty, the documented way to blank a variable', () => { + expect(hasClaudeAuthEnvConflict({ ANTHROPIC_API_KEY: '' }, 'linux')).toBe(false) + expect(hasClaudeAuthEnvConflict({ anthropic_api_key: '' }, 'win32')).toBe(false) + expect(hasClaudeAuthEnvConflict({ ANTHROPIC_CUSTOM_HEADERS: '' }, 'linux')).toBe(false) + }) + + it('still refuses the same names once they carry a value', () => { + expect(hasClaudeAuthEnvConflict({ ANTHROPIC_API_KEY: 'sk-ant' }, 'linux')).toBe(true) + }) + + // The end-to-end shape the regression actually took: settings text -> normalized + // record -> launch env -> the predicate the terminal preflight gates on. + it('admits a blanked variable all the way from the settings record', () => { + const configured = normalizeTuiAgentEnvRecord({ claude: { ANTHROPIC_API_KEY: '' } }) + const launchEnv = resolveTuiAgentLaunchEnv('claude', configured) + + expect(launchEnv).toEqual({ ANTHROPIC_API_KEY: '' }) + expect(hasClaudeAuthEnvConflict(launchEnv, 'linux')).toBe(false) }) it('folds case on win32, where the OS does', () => { diff --git a/src/main/claude-accounts/environment.ts b/src/main/claude-accounts/environment.ts index 58228463cb2..b85dd60a854 100644 --- a/src/main/claude-accounts/environment.ts +++ b/src/main/claude-accounts/environment.ts @@ -78,8 +78,11 @@ export function shouldStripClaudeAuthEnvForAccount( * `ANTHROPIC_API_KEY`, and case-sensitive elsewhere. A refusal narrower than the strip * lets an override through that the strip would have removed. * - * Presence, not truthiness: the strip deletes these names whatever their value, so an - * empty override is still an override of the pinned account's credential. + * A non-empty value is what makes it a conflict. `ANTHROPIC_API_KEY=` in the agent env + * box is how a user blanks a variable — the settings pipeline preserves that empty value + * (normalizeTuiAgentEnvRecord drops empty KEYS only) — and an empty override can neither + * authenticate nor beat the pinned account, while the strip removes the name regardless. + * Refusing it would break a terminal launch that works today for no security gain. */ /** * The inherited Anthropic auth a non-stripping launch has to carry forward explicitly. @@ -118,7 +121,7 @@ export function hasClaudeAuthEnvConflict( } for (const [key, value] of Object.entries(env)) { const normalized = platform === 'win32' ? key.toUpperCase() : key - if (CLAUDE_AUTH_ENV_VARS.some((authKey) => authKey === normalized)) { + if (value && CLAUDE_AUTH_ENV_VARS.some((authKey) => authKey === normalized)) { return true } if (normalized === 'ANTHROPIC_CUSTOM_HEADERS' && isAuthLikeCustomHeaders(value)) { diff --git a/src/main/claude/claude-stream-json-connection.test.ts b/src/main/claude/claude-stream-json-connection.test.ts index ef59312ee63..a0387dd2a4b 100644 --- a/src/main/claude/claude-stream-json-connection.test.ts +++ b/src/main/claude/claude-stream-json-connection.test.ts @@ -719,4 +719,47 @@ describe('the managed-auth live gate', () => { await until(() => (hasLiveClaudePtys() ? null : true), 'the auth gate to drain') expect(hasLiveClaudePtys()).toBe(false) }, 30_000) + + // The gate entry is deliberately unpersisted, so confirmSeededClaudeLivePtys can never + // reconcile a stray one: a leak here defers the managed OAuth refresh for the life of + // the process. Entering the gate only after the release handlers are attached makes + // that unreachable regardless of what the setup in between does. + it('leaks no gate entry when setup throws between spawn and handler attachment', async () => { + await until(() => (hasLiveClaudePtys() ? null : true), 'a drained auth gate') + const scenario = scriptScenario([ + { emit: { type: 'system', subtype: 'init', session_id: SESSION_ID, uuid: 'init-1' } }, + { wait: HOLD_OPEN } + ]) + let started: SpawnedProcess | null = null + + try { + await expect( + openClaudeStreamJsonConnection(launchFor(scenario), {}, (spec) => { + const child = spawnProcess(spec) + started = child + const attach = child.stderr.on.bind(child.stderr) + // Measured attach order: the SDK binds stderr 'data' from inside query(), + // before the child is even assigned. The SECOND bind is this connection's own + // armTreeOnOutput — the first statement that runs after the child exists and + // before its 'exit'/'close' release handlers. Throwing on the first is + // vacuous: it escapes before any gate entry could have happened. + let dataAttaches = 0 + child.stderr.on = ((event: string, listener: (...args: unknown[]) => void) => { + if (event === 'data') { + dataAttaches += 1 + if (dataAttaches === 2) { + throw new Error('stderr listener attach failed') + } + } + return attach(event, listener) + }) as typeof child.stderr.on + return child + }) + ).rejects.toThrow('stderr listener attach failed') + + expect(hasLiveClaudePtys()).toBe(false) + } finally { + ;(started as SpawnedProcess | null)?.kill('SIGKILL') + } + }, 30_000) }) diff --git a/src/main/claude/claude-stream-json-connection.ts b/src/main/claude/claude-stream-json-connection.ts index d3fdb4e02c7..dd6bbc8a5eb 100644 --- a/src/main/claude/claude-stream-json-connection.ts +++ b/src/main/claude/claude-stream-json-connection.ts @@ -125,9 +125,9 @@ export async function openClaudeStreamJsonConnection( } // This child owns the account's credentials for as long as it runs, exactly as a // Claude PTY does — hold the OAuth-refresh gate so a managed refresh cannot rotate - // the single-use token out from under it mid-turn. + // the single-use token out from under it mid-turn. Entered below, once a release + // path exists. const authGateKey = randomUUID() - markClaudeStructuredChildSpawned(authGateKey) const releaseAuthGate = (): void => markClaudeStructuredChildExited(authGateKey) let exited = false let exitStatus: ExitStatus | null = null @@ -226,6 +226,13 @@ export async function openClaudeStreamJsonConnection( handleUnexpectedEnd(error) } }) + // Why here and not at spawn: a structured gate entry is deliberately unpersisted, so + // confirmSeededClaudeLivePtys can never reconcile a stray one and a leak defers the + // managed OAuth refresh for the life of the process. Entering only after 'exit' and + // 'close' are attached makes that unreachable — any later throw still leaves a + // listener that releases. Nothing between spawn and here can yield, so the child + // cannot end before the gate is entered. + markClaudeStructuredChildSpawned(authGateKey) const send = (message: Record): Promise => { if (closing || exited || terminalError || child.stdin.destroyed || !child.stdin.writable) { diff --git a/src/main/native-chat/session-file-resolver-claude-roots.test.ts b/src/main/native-chat/session-file-resolver-claude-roots.test.ts new file mode 100644 index 00000000000..87ebd570280 --- /dev/null +++ b/src/main/native-chat/session-file-resolver-claude-roots.test.ts @@ -0,0 +1,97 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const scanned = vi.hoisted(() => ({ dirs: [] as string[], hits: {} as Record })) +vi.mock('../ai-vault/session-scanner-discovery', () => ({ + walkSessionFiles: async (dir: string) => { + scanned.dirs.push(dir) + const hit = scanned.hits[dir] + return hit ? [hit] : [] + } +})) + +import { homedir } from 'node:os' +import { join } from 'node:path' +import { resolveSessionFilePath } from './session-file-resolver' + +const DEFAULT_ROOT = join(homedir(), '.claude', 'projects') +const CONFIG_DIR = '/opt/claude-home' +const CONFIG_ROOT = join(CONFIG_DIR, 'projects') + +let previousConfigDir: string | undefined + +beforeEach(() => { + previousConfigDir = process.env.CLAUDE_CONFIG_DIR + scanned.dirs = [] + scanned.hits = {} +}) + +afterEach(() => { + if (previousConfigDir === undefined) { + delete process.env.CLAUDE_CONFIG_DIR + } else { + process.env.CLAUDE_CONFIG_DIR = previousConfigDir + } +}) + +/** + * Honouring CLAUDE_CONFIG_DIR fixed new sessions but would otherwise hide every + * transcript written before the user adopted the variable. The Codex resolver in this + * same file already searches managed-then-default and de-dupes; Claude does the same. + */ +describe('claude transcript roots', () => { + it('searches the config-dir root first, then the default home', async () => { + process.env.CLAUDE_CONFIG_DIR = CONFIG_DIR + + await resolveSessionFilePath('claude', 'session-1') + + expect(scanned.dirs).toEqual([CONFIG_ROOT, DEFAULT_ROOT]) + }) + + it('still finds history written before CLAUDE_CONFIG_DIR was adopted', async () => { + process.env.CLAUDE_CONFIG_DIR = CONFIG_DIR + const legacy = join(DEFAULT_ROOT, '-repos-old', 'session-1.jsonl') + scanned.hits[DEFAULT_ROOT] = legacy + + await expect(resolveSessionFilePath('claude', 'session-1')).resolves.toBe(legacy) + }) + + it('prefers the config-dir root when both hold the session', async () => { + process.env.CLAUDE_CONFIG_DIR = CONFIG_DIR + scanned.hits[CONFIG_ROOT] = join(CONFIG_ROOT, '-repos-new', 'session-1.jsonl') + scanned.hits[DEFAULT_ROOT] = join(DEFAULT_ROOT, '-repos-old', 'session-1.jsonl') + + await expect(resolveSessionFilePath('claude', 'session-1')).resolves.toBe( + scanned.hits[CONFIG_ROOT] + ) + // The default root is never reached, so the common case pays for one scan. + expect(scanned.dirs).toEqual([CONFIG_ROOT]) + }) + + it('scans one root when the variable is unset', async () => { + delete process.env.CLAUDE_CONFIG_DIR + + await resolveSessionFilePath('claude', 'session-1') + + expect(scanned.dirs).toEqual([DEFAULT_ROOT]) + }) + + it('de-dupes when CLAUDE_CONFIG_DIR names the default home', async () => { + process.env.CLAUDE_CONFIG_DIR = join(homedir(), '.claude') + + await resolveSessionFilePath('claude', 'session-1') + + expect(scanned.dirs).toEqual([DEFAULT_ROOT]) + }) + + it('honours an explicit root override without adding fallbacks', async () => { + process.env.CLAUDE_CONFIG_DIR = CONFIG_DIR + // The account-home callers (structured-claude-runtime-adapter, the host handoff) + // know the exact tree their session pinned; a fallback there could resolve a + // different account's transcript. + await resolveSessionFilePath('claude', 'session-1', { + claudeProjectsDir: '/accounts/pinned/projects' + }) + + expect(scanned.dirs).toEqual(['/accounts/pinned/projects']) + }) +}) diff --git a/src/main/native-chat/session-file-resolver.ts b/src/main/native-chat/session-file-resolver.ts index 09486f9af2c..12d2e615742 100644 --- a/src/main/native-chat/session-file-resolver.ts +++ b/src/main/native-chat/session-file-resolver.ts @@ -31,13 +31,20 @@ import { proveClaudeTranscriptBranch } from '../claude/claude-transcript-branch- // the remote main resolves its local home, so we never hardcode an absolute // user path — homedir()/CODEX_HOME resolution stays runtime-relative and is // computed per call (not at module load) so it tracks the live home. -function claudeProjectsDir(): string { - // Why CLAUDE_CONFIG_DIR and not just homedir(): a structured Claude session pins - // its account home to `CLAUDE_CONFIG_DIR || ~/.claude` (claude-accounts/runtime-paths.ts), - // and the CLI writes its transcript under whatever home it was given. Mobile native - // chat resolves with no root override, so a default that ignored the variable read a - // different tree than the CLI wrote — a silent blackout, not an error. - return join(process.env.CLAUDE_CONFIG_DIR?.trim() || join(homedir(), '.claude'), 'projects') +// Why CLAUDE_CONFIG_DIR and not just homedir(): a structured Claude session pins its +// account home to `CLAUDE_CONFIG_DIR || ~/.claude` (claude-accounts/runtime-paths.ts), +// and the CLI writes its transcript under whatever home it was given. Mobile native chat +// resolves with no root override, so a default that ignored the variable read a different +// tree than the CLI wrote — a silent blackout, not an error. +// Why both roots and not just that one: adopting the variable would otherwise hide every +// transcript written before it was set. Same managed-then-default shape as +// codexSessionsDirs below, de-duped so the usual case still scans once. +function claudeProjectsDirs(): string[] { + const candidates = [ + join(process.env.CLAUDE_CONFIG_DIR?.trim() || join(homedir(), '.claude'), 'projects'), + join(homedir(), '.claude', 'projects') + ] + return candidates.filter((dir, index) => candidates.indexOf(dir) === index) } // Why: Orca launches Codex with ORCA_CODEX_HOME pointing at its own managed @@ -178,9 +185,11 @@ async function resolveSessionFileById( } if (transcriptAgent === 'claude') { + // An explicit root is the caller naming the exact account tree its session pinned; + // adding a fallback there could resolve a different account's transcript. return resolveClaudeSessionFile( trimmedId, - options.claudeProjectsDir ?? claudeProjectsDir(), + options.claudeProjectsDir ? [options.claudeProjectsDir] : claudeProjectsDirs(), signal ) } @@ -210,16 +219,22 @@ async function resolveSessionFileById( async function resolveClaudeSessionFile( sessionId: string, - projectsDir: string, + projectsDirs: readonly string[], signal?: AbortSignal ): Promise { const targetName = `${sessionId}.jsonl` - const files = await walkSessionFiles(projectsDir, 'claude', [], { - extensions: new Set(['.jsonl']), - filePredicate: (path) => basename(path) === targetName, - signal - }) - return files[0] ?? null + for (const projectsDir of projectsDirs) { + // No existence pre-check: walkSessionFiles already yields [] for a missing root. + const files = await walkSessionFiles(projectsDir, 'claude', [], { + extensions: new Set(['.jsonl']), + filePredicate: (path) => basename(path) === targetName, + signal + }) + if (files[0]) { + return files[0] + } + } + return null } async function resolveCodexSessionFile(