diff --git a/CLAUDE-STRUCTURED-FABLE-P1-FIX-REPORT.md b/CLAUDE-STRUCTURED-FABLE-P1-FIX-REPORT.md new file mode 100644 index 00000000000..8af9c1e787f --- /dev/null +++ b/CLAUDE-STRUCTURED-FABLE-P1-FIX-REPORT.md @@ -0,0 +1,50 @@ +# Claude structured Fable P1 fix + +## Outcome + +Fixed the three release-blocking findings against parent `bf5c9604640e8462814829dfec0b35ff2b612d96`: + +1. Structured Claude leaf sampling now rejects stdout `user`/`assistant` UUIDs with a non-null `parent_tool_use_id`, and transcript branch proof marks those rows as disallowed. If a durable close or first-hand crash proof rejects the sampled cursor, the adapter re-proves from `previousLeafUuid: null` and persists the valid main-transcript leaf when available. Existing bounded ancestry, malformed-tail, sibling-branch, and resume protections remain fail-closed. +2. Structured SDK launches now carry `systemPrompt: { type: 'preset', preset: 'claude_code' }` in the base options. The connection test injects the SDK query boundary and pins the option passed to initialization, while account/config roots, setting sources, and the legacy CLI path remain unchanged. +3. `StructuredAgentSessionAdapterRouter.closeSession` retains the adapter owner when close is false/unproven and removes it only after an exact `closed === true`, allowing a later retry to reach the same provider adapter. + +## Exact files changed + +- `src/main/claude/claude-stream-json-connection.test.ts` +- `src/main/claude/claude-stream-json-connection.ts` +- `src/main/claude/claude-structured-launch-resolution.test.ts` +- `src/main/claude/claude-structured-launch-resolution.ts` +- `src/main/claude/claude-structured-session-adapter.ts` +- `src/main/claude/claude-structured-session-close.ts` +- `src/main/claude/claude-structured-session-recovery.test.ts` +- `src/main/claude/claude-transcript-branch-proof.ts` +- `src/main/claude/claude-tui-exit.test.ts` +- `src/main/claude/claude-tui-exit.ts` +- `src/main/native-chat/agent-session-wire/structured-agent-session-adapter-router.test.ts` +- `src/main/native-chat/agent-session-wire/structured-agent-session-adapter-router.ts` +- `src/main/native-chat/session-file-resolver.test.ts` + +## Red-first tests and verification + +The new tests were run before implementation and failed for each targeted behavior: parent-tool-use UUIDs were sampled/accepted, stale-cursor recovery retained the old leaf, the base system prompt was absent, and router ownership was dropped after a false close. After implementation: + +- Focused P1 suites: 6 files, 69 tests passed. +- All Claude suites plus router/session resolver: 60 files, 512 tests passed, 2 skipped. +- Node typecheck: `node_modules/.bin/tsc --noEmit -p config/tsconfig.node.json` passed. +- Web typecheck: `node_modules/.bin/tsc --noEmit -p config/tsconfig.tc.web.json` passed. +- Native changed-area Oxlint with warnings denied passed. +- Changed-code quality gate passed with zero new findings across 13 files. +- Changed-file `oxfmt --check` and `git diff --check` passed. +- `pnpm-lock.yaml` was restored byte-for-byte to the parent and has no diff. + +## Five-finding disposition + +- Sidechain/subagent UUID and stale-cursor recovery: proven P1, fixed here; graceful close and first-hand crash regressions are covered. +- Missing Claude Code system prompt: proven P1, fixed here; the SDK query boundary is pinned. +- Router close ownership loss: proven P1, fixed here; false-then-true close retry is covered. +- Grouped-answer handling: proven separately and already fixed on the parent by the existing grouped-question change/report; no regression or scope overlap was found here, so it is dismissed from this dispatch. +- Local-image and cleanup-retry leads: reviewed as existing, unrelated paths; neither is changed or regressed by this diff, so both are dismissed from this dispatch (no new release-blocking evidence). + +## Harness/environment and artifact notes + +The repository's pnpm bootstrap added an `@pnpm/exe` lockfile entry while invoking pnpm commands; it was removed and the lockfile was rechecked clean. No Electron/mobile UI flow applies to these main-process/adapter changes. No pre-existing untracked artifacts were edited or deleted; the child remains at the requested parent lineage with only the source, tests, and this report changed. diff --git a/src/main/claude/claude-stream-json-connection.test.ts b/src/main/claude/claude-stream-json-connection.test.ts index 39a822108a9..9360988c29f 100644 --- a/src/main/claude/claude-stream-json-connection.test.ts +++ b/src/main/claude/claude-stream-json-connection.test.ts @@ -4,7 +4,7 @@ import { join } from 'node:path' import { afterEach, describe, expect, it, vi } from 'vitest' import { spawnProcess, type SpawnedProcess } from '../../shared/child-process/run-process' import type { ProcessSpec } from '../../shared/child-process/process-spec' -import type { CanUseTool } from '@anthropic-ai/claude-agent-sdk' +import { query, type CanUseTool, type Options } from '@anthropic-ai/claude-agent-sdk' import { openClaudeStreamJsonConnection, type ClaudeStreamJsonConnection, @@ -85,14 +85,20 @@ const spawnedChildren: SpawnedProcess[] = [] async function open( launch: ClaudeStreamJsonLaunch, - handlers: Parameters[1] = {} + handlers: Parameters[1] = {}, + queryImpl?: typeof query ): Promise { - const connection = await openClaudeStreamJsonConnection(launch, handlers, (spec) => { - spawned.push(spec) - const child = spawnProcess(spec) - spawnedChildren.push(child) - return child - }) + const connection = await openClaudeStreamJsonConnection( + launch, + handlers, + (spec) => { + spawned.push(spec) + const child = spawnProcess(spec) + spawnedChildren.push(child) + return child + }, + queryImpl + ) openConnections.push(connection) return connection } @@ -121,6 +127,17 @@ function readReportSafely(scenario: { readReport: () => ScriptedCliReport }) { } describe('Claude stream-json connection', () => { + it('passes the Claude Code system-prompt preset through to SDK query', async () => { + const scenario = scriptScenario([HOLD_OPEN]) + let captured: Options | undefined + await open(launchFor(scenario), {}, (params) => { + captured = params.options + return query(params) + }) + + expect(captured?.systemPrompt).toEqual({ type: 'preset', preset: 'claude_code' }) + }) + it('hands the child a derived environment, the resolved CLI path, and keeps the pid', async () => { vi.stubEnv('ANTHROPIC_API_KEY', 'sk-ant-SHELL-LEAK') vi.stubEnv('CLAUDE_CODE_CHILD_SESSION', '1') diff --git a/src/main/claude/claude-stream-json-connection.ts b/src/main/claude/claude-stream-json-connection.ts index 806817284f9..8cb09e4386b 100644 --- a/src/main/claude/claude-stream-json-connection.ts +++ b/src/main/claude/claude-stream-json-connection.ts @@ -94,12 +94,13 @@ function exitError(stderrTail: string, status: ExitStatus | null, cause?: Error) export async function openClaudeStreamJsonConnection( launch: ClaudeStreamJsonLaunch, handlers: ClaudeStreamJsonConnectionHandlers = {}, - spawnImpl: typeof spawnProcess = spawnProcess + spawnImpl: typeof spawnProcess = spawnProcess, + queryImpl?: typeof ClaudeAgentSdk.query ): Promise { const { query } = await loadClaudeAgentSdk() const spawner = createClaudeCodeProcessSpawn(spawnImpl) const inbox = createClaudeUserMessageQueue() - const session = query({ + const session = (queryImpl ?? query)({ prompt: inbox.messages, options: { ...launch.options, diff --git a/src/main/claude/claude-structured-launch-resolution.test.ts b/src/main/claude/claude-structured-launch-resolution.test.ts index aa115341d7f..653c2518407 100644 --- a/src/main/claude/claude-structured-launch-resolution.test.ts +++ b/src/main/claude/claude-structured-launch-resolution.test.ts @@ -77,6 +77,7 @@ describe('claude structured launch resolution', () => { settingSources: [...CLAUDE_DEFAULT_SETTING_SOURCES], supportedDialogKinds: [], extraArgs: { 'replay-user-messages': null }, + systemPrompt: { type: 'preset', preset: 'claude_code' }, sessionId: first.providerSessionId }) expect(first.options.resume).toBeUndefined() diff --git a/src/main/claude/claude-structured-launch-resolution.ts b/src/main/claude/claude-structured-launch-resolution.ts index 2f653924405..41db7a403ed 100644 --- a/src/main/claude/claude-structured-launch-resolution.ts +++ b/src/main/claude/claude-structured-launch-resolution.ts @@ -13,6 +13,7 @@ export const CLAUDE_DEFAULT_SETTING_SOURCES = ['user', 'project', 'local'] as co export type ClaudeStructuredSdkOptions = Pick< ClaudeAgentSdkOptions, | 'includePartialMessages' + | 'systemPrompt' | 'settingSources' | 'supportedDialogKinds' | 'extraArgs' @@ -33,6 +34,8 @@ export type ClaudeStructuredSdkOptions = Pick< */ export const CLAUDE_STRUCTURED_BASE_OPTIONS: ClaudeStructuredSdkOptions = { includePartialMessages: true, + // Keep the SDK on Claude Code's own system-prompt contract. + systemPrompt: { type: 'preset', preset: 'claude_code' }, settingSources: [...CLAUDE_DEFAULT_SETTING_SOURCES], supportedDialogKinds: [], extraArgs: { 'replay-user-messages': null } diff --git a/src/main/claude/claude-structured-session-adapter.ts b/src/main/claude/claude-structured-session-adapter.ts index 8f30e0b4cdc..69d5767086c 100644 --- a/src/main/claude/claude-structured-session-adapter.ts +++ b/src/main/claude/claude-structured-session-adapter.ts @@ -27,6 +27,7 @@ import { closeClaudeSession, settleClaudeExitedSession } from './claude-structured-session-close' +import { readClaudeTranscriptLeafWithReproof } from './claude-transcript-branch-proof' export type { ClaudeStructuredLaunch } from './claude-structured-launch-resolution' export type { @@ -97,10 +98,13 @@ export class ClaudeStructuredSessionAdapter implements StructuredAgentSessionAda private async persistSessionHandle(sessionId: string, session: ClaudeSession): Promise { try { - const transcriptLeaf = await this.deps.readTranscriptLeaf?.({ - providerSessionId: session.providerSessionId, - previousLeafUuid: session.leafUuid - }) + const transcriptLeaf = this.deps.readTranscriptLeaf + ? await readClaudeTranscriptLeafWithReproof({ + readTranscriptLeaf: this.deps.readTranscriptLeaf, + providerSessionId: session.providerSessionId, + previousLeafUuid: session.leafUuid + }) + : null if (transcriptLeaf) { session.leafUuid = transcriptLeaf } diff --git a/src/main/claude/claude-structured-session-close.ts b/src/main/claude/claude-structured-session-close.ts index ddf103bb03c..47db0ad8d32 100644 --- a/src/main/claude/claude-structured-session-close.ts +++ b/src/main/claude/claude-structured-session-close.ts @@ -5,6 +5,7 @@ import type { } from './claude-structured-session-state' import { cancelClaudeAcquisitionAttempt } from './claude-structured-session-state' import { closeProcessRegistry } from '../../shared/child-process/close-process-registry' +import { readClaudeTranscriptLeafWithReproof } from './claude-transcript-branch-proof' export function settleClaudeDispatchWaiters(session: ClaudeSession): void { for (const waiter of session.dispatchWaiters.splice(0)) { @@ -50,10 +51,13 @@ export async function closeClaudePublishedSession(input: { return false } try { - const transcriptLeaf = await input.readTranscriptLeaf?.({ - providerSessionId: session.providerSessionId, - previousLeafUuid: session.leafUuid - }) + const transcriptLeaf = input.readTranscriptLeaf + ? await readClaudeTranscriptLeafWithReproof({ + readTranscriptLeaf: input.readTranscriptLeaf, + providerSessionId: session.providerSessionId, + previousLeafUuid: session.leafUuid + }) + : null if (transcriptLeaf) { session.leafUuid = transcriptLeaf } diff --git a/src/main/claude/claude-structured-session-recovery.test.ts b/src/main/claude/claude-structured-session-recovery.test.ts index a2ebc13e590..4bbdf32809d 100644 --- a/src/main/claude/claude-structured-session-recovery.test.ts +++ b/src/main/claude/claude-structured-session-recovery.test.ts @@ -72,17 +72,14 @@ describe('ClaudeStructuredSessionAdapter transcript-derived recovery', () => { expect(persistedHandles.at(-1)).toMatchObject({ leafUuid: 'durable-tail' }) }) - it('keeps the observed leaf when transcript validation rejects a stale branch', async () => { + it('re-proves from the transcript root when the observed cursor rejects a stale branch', async () => { const claude = fakeClaude() const persistedHandles: unknown[] = [] - const adapter = adapterFor( - claude, - {}, - [], - persistedHandles, - undefined, - vi.fn().mockRejectedValue(new Error('sibling branch')) - ) + const readTranscriptLeaf = vi + .fn() + .mockRejectedValueOnce(new Error('sibling branch')) + .mockResolvedValueOnce('reproved-main-leaf') + const adapter = adapterFor(claude, {}, [], persistedHandles, undefined, readTranscriptLeaf) await adapter.acquire({ identity: identityFor(), fence: 7, spawnToken: 'spawn-9' }) claude.connections[0].handlers.onMessage?.({ type: 'assistant', @@ -92,7 +89,15 @@ describe('ClaudeStructuredSessionAdapter transcript-derived recovery', () => { await adapter.closeSession('session-1') - expect(persistedHandles.at(-1)).toMatchObject({ leafUuid: 'observed-tail' }) + expect(readTranscriptLeaf).toHaveBeenNthCalledWith(1, { + providerSessionId: PROVIDER_SESSION_ID, + previousLeafUuid: 'observed-tail' + }) + expect(readTranscriptLeaf).toHaveBeenNthCalledWith(2, { + providerSessionId: PROVIDER_SESSION_ID, + previousLeafUuid: null + }) + expect(persistedHandles.at(-1)).toMatchObject({ leafUuid: 'reproved-main-leaf' }) }) it('persists the last transcript leaf before an unexpected first-hand exit', async () => { @@ -151,6 +156,34 @@ describe('ClaudeStructuredSessionAdapter transcript-derived recovery', () => { expect(persistedHandles.at(-1)).toMatchObject({ leafUuid: 'durable-crash-leaf' }) }) + it('re-proves a first-hand crash cursor from the transcript root after stale validation', async () => { + const claude = fakeClaude() + const persistedHandles: unknown[] = [] + const readTranscriptLeaf = vi + .fn() + .mockRejectedValueOnce(new Error('stale cursor')) + .mockResolvedValueOnce('reproved-crash-leaf') + const adapter = adapterFor(claude, {}, [], persistedHandles, undefined, readTranscriptLeaf) + await adapter.acquire({ identity: identityFor(), fence: 7, spawnToken: 'spawn-9' }) + claude.connections[0].handlers.onMessage?.({ + type: 'assistant', + session_id: PROVIDER_SESSION_ID, + uuid: 'stale-observed-tail' + }) + claude.connections[0].handlers.onExit?.(new Error('crashed')) + await tick() + + expect(readTranscriptLeaf).toHaveBeenNthCalledWith(1, { + providerSessionId: PROVIDER_SESSION_ID, + previousLeafUuid: 'stale-observed-tail' + }) + expect(readTranscriptLeaf).toHaveBeenNthCalledWith(2, { + providerSessionId: PROVIDER_SESSION_ID, + previousLeafUuid: null + }) + expect(persistedHandles.at(-1)).toMatchObject({ leafUuid: 'reproved-crash-leaf' }) + }) + it('publishes lifecycle recovery even when crash-cursor persistence fails', async () => { const claude = fakeClaude() const events: ClaudeStructuredSessionEvent[] = [] diff --git a/src/main/claude/claude-transcript-branch-proof.ts b/src/main/claude/claude-transcript-branch-proof.ts index 3d7aa382c3d..4baf545eb95 100644 --- a/src/main/claude/claude-transcript-branch-proof.ts +++ b/src/main/claude/claude-transcript-branch-proof.ts @@ -74,6 +74,7 @@ export function proveClaudeTranscriptBranchFromJsonl(input: { const existing = nodes.get(uuid) const disallowedLeaf = row.isSidechain === true || + row.parent_tool_use_id != null || row.type === 'result' || row.type === 'stream_event' || (row.type === 'system' && row.subtype === 'init') @@ -160,3 +161,28 @@ export async function proveClaudeTranscriptBranch(input: { previousLeafUuid: input.previousLeafUuid }) } + +/** Re-run a durable branch proof from the transcript root when a sampled cursor is stale. */ +export async function readClaudeTranscriptLeafWithReproof(input: { + readTranscriptLeaf: (input: { + providerSessionId: string + previousLeafUuid: string | null + }) => Promise + providerSessionId: string + previousLeafUuid: string | null +}): Promise { + try { + return await input.readTranscriptLeaf({ + providerSessionId: input.providerSessionId, + previousLeafUuid: input.previousLeafUuid + }) + } catch (error) { + if (input.previousLeafUuid === null) { + throw error + } + return input.readTranscriptLeaf({ + providerSessionId: input.providerSessionId, + previousLeafUuid: null + }) + } +} diff --git a/src/main/claude/claude-tui-exit.test.ts b/src/main/claude/claude-tui-exit.test.ts index 29f50a4ca07..6e1140b0f4d 100644 --- a/src/main/claude/claude-tui-exit.test.ts +++ b/src/main/claude/claude-tui-exit.test.ts @@ -2,7 +2,11 @@ import { mkdtemp, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, describe, expect, it, vi } from 'vitest' -import { completeClaudeTuiExit, readClaudeTranscriptLeafUuid } from './claude-tui-exit' +import { + completeClaudeTuiExit, + readClaudeTranscriptEntryUuid, + readClaudeTranscriptLeafUuid +} from './claude-tui-exit' const roots: string[] = [] @@ -11,6 +15,23 @@ afterEach(async () => { }) describe('Claude TUI exit', () => { + it('does not sample UUIDs from subagent stdout frames with a parent tool use', () => { + expect( + readClaudeTranscriptEntryUuid({ + type: 'assistant', + uuid: 'subagent-assistant', + parent_tool_use_id: 'parent-tool' + }) + ).toBeNull() + expect( + readClaudeTranscriptEntryUuid({ + type: 'assistant', + uuid: 'main-assistant', + parent_tool_use_id: null + }) + ).toBe('main-assistant') + }) + it('reads the authoritative last-prompt leaf from a transcript tail', async () => { const root = await mkdtemp(join(tmpdir(), 'orca-claude-tui-exit-')) roots.push(root) diff --git a/src/main/claude/claude-tui-exit.ts b/src/main/claude/claude-tui-exit.ts index 148c3075190..3772e9c5e52 100644 --- a/src/main/claude/claude-tui-exit.ts +++ b/src/main/claude/claude-tui-exit.ts @@ -19,7 +19,9 @@ function validLeafUuid(value: unknown): string | null { } export function readClaudeTranscriptEntryUuid(value: Record): string | null { - return value.isSidechain === true || (value.type !== 'user' && value.type !== 'assistant') + return value.isSidechain === true || + value.parent_tool_use_id != null || + (value.type !== 'user' && value.type !== 'assistant') ? null : validLeafUuid(value.uuid) } diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-adapter-router.test.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-adapter-router.test.ts index db842cae3b0..2379cbc1f71 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-adapter-router.test.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-adapter-router.test.ts @@ -31,3 +31,30 @@ describe('StructuredAgentSessionAdapterRouter.releaseAcquisition', () => { expect(codex.releaseAcquisition).toHaveBeenCalledTimes(1) }) }) + +describe('StructuredAgentSessionAdapterRouter.closeSession', () => { + it('retains the owner after an unproven close so a later retry reaches the same adapter', async () => { + const claude = adapterOf(vi.fn(async () => true)) + const closeSession = vi.fn().mockResolvedValueOnce(false).mockResolvedValueOnce(true) + const dispatch = vi.fn().mockResolvedValue({ state: 'unknown', reason: 'test' }) + claude.closeSession = closeSession + claude.dispatch = dispatch + const codex = adapterOf(vi.fn(async () => false)) + const router = new StructuredAgentSessionAdapterRouter({ claude, codex }, async () => {}) + const identity = { sessionId: 'session-1', agent: 'claude' } as never + await router.acquire({ identity, fence: 1, spawnToken: 'spawn-1' }) + + await expect(router.closeSession('session-1')).resolves.toBe(false) + await expect( + router.dispatch({ + sessionId: 'session-1', + clientMessageId: 'client-1', + body: {} as never, + fence: 1 + }) + ).resolves.toMatchObject({ state: 'unknown' }) + await expect(router.closeSession('session-1')).resolves.toBe(true) + expect(closeSession).toHaveBeenCalledTimes(2) + expect(dispatch).toHaveBeenCalledTimes(1) + }) +}) diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-adapter-router.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-adapter-router.ts index 88695e58e61..5b079e9ebcc 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-adapter-router.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-adapter-router.ts @@ -72,8 +72,11 @@ export class StructuredAgentSessionAdapterRouter implements StructuredAgentSessi return false } const closed = await adapter.closeSession?.(sessionId) - this.owners.delete(sessionId) - return closed === true + if (closed === true) { + this.owners.delete(sessionId) + return true + } + return false } async closeAll(): Promise { diff --git a/src/main/native-chat/session-file-resolver.test.ts b/src/main/native-chat/session-file-resolver.test.ts index 41dce644788..9e2960d70eb 100644 --- a/src/main/native-chat/session-file-resolver.test.ts +++ b/src/main/native-chat/session-file-resolver.test.ts @@ -206,6 +206,38 @@ describe('resolveSessionFilePath', () => { ) }) + it('rejects a main leaf whose ancestry crosses a parent-tool-use sidechain', async () => { + const root = await makeRoot('orca-native-chat-resolve-claude-parent-tool-ancestry-') + const transcript = join(root, 'transcript.jsonl') + await writeFile( + transcript, + [ + { type: 'user', uuid: 'main-user', parentUuid: null, sessionId: 'session-1' }, + { + type: 'assistant', + uuid: 'subagent-assistant', + parentUuid: 'main-user', + sessionId: 'session-1', + parent_tool_use_id: 'tool-use-1' + }, + { + type: 'assistant', + uuid: 'main-after-sidechain', + parentUuid: 'subagent-assistant', + sessionId: 'session-1' + }, + { type: 'last-prompt', leafUuid: 'main-after-sidechain', sessionId: 'session-1' } + ] + .map((record) => JSON.stringify(record)) + .join('\n'), + 'utf8' + ) + + await expect(readClaudeTranscriptLeafUuid(transcript, 'session-1')).rejects.toThrow( + 'not on the main transcript' + ) + }) + it('globs Claude project subdirs for .jsonl', async () => { const root = await makeRoot('orca-native-chat-resolve-claude-') const claudeProjectsDir = join(root, 'claude-projects')