mirror of
https://github.com/stablyai/orca.git
synced 2026-09-29 16:02:50 +00:00
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.
This commit is contained in:
@@ -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', () => {
|
||||
|
||||
@@ -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)) {
|
||||
|
||||
@@ -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)
|
||||
})
|
||||
|
||||
@@ -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<string, unknown>): Promise<void> => {
|
||||
if (closing || exited || terminalError || child.stdin.destroyed || !child.stdin.writable) {
|
||||
|
||||
@@ -0,0 +1,97 @@
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
|
||||
const scanned = vi.hoisted(() => ({ dirs: [] as string[], hits: {} as Record<string, string> }))
|
||||
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'])
|
||||
})
|
||||
})
|
||||
@@ -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<string | null> {
|
||||
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(
|
||||
|
||||
Reference in New Issue
Block a user