diff --git a/src/main/claude/claude-structured-fork-launch.test.ts b/src/main/claude/claude-structured-fork-launch.test.ts index f74da659a96..5146104f73a 100644 --- a/src/main/claude/claude-structured-fork-launch.test.ts +++ b/src/main/claude/claude-structured-fork-launch.test.ts @@ -1,6 +1,10 @@ import { describe, expect, it } from 'vitest' import { applyClaudeStructuredForkLaunch } from './claude-structured-fork-launch' -import type { ClaudeStructuredLaunch } from './claude-structured-launch-resolution' +import { + claudeSessionIdForOrcaSession, + type ClaudeStructuredLaunch +} from './claude-structured-launch-resolution' +import { isAgentSessionPreSpawnError } from '../native-chat/agent-session-wire/structured-agent-session-adapter' const launch: ClaudeStructuredLaunch = { pathToClaudeCodeExecutable: 'claude', @@ -47,4 +51,30 @@ describe('Claude structured fork launch', () => { ) ).toThrow('agent_session_identity_required') }) + + it('classifies BOTH refusals as pre-spawn, so a failed fork settles instead of stranding', () => { + // Nothing here spawns: it rewrites launch arguments and its one caller runs it before the child + // is opened. Unclassified, these two throws leave the child record at `attempted` forever — + // every later attach throws `agent_session_operation_unknown` and the turn becomes permanently + // unforkable, while the user is told to "retry the same turn", which that path cannot honour. + const collides = claudeSessionIdForOrcaSession('claude_new_session') + const cases: [ClaudeStructuredLaunch, string, string][] = [ + [launch, 'foreign', 'agent_session_identity_required'], + [{ ...launch, resumed: false }, collides, 'agent_session_provider_handle_invalid'] + ] + for (const [input, sessionId, message] of cases) { + let thrown: unknown + try { + applyClaudeStructuredForkLaunch( + input, + { source: { provider: 'claude', sessionId, leafUuid: null }, throughId: 'selected' }, + 'claude_new_session' + ) + } catch (error) { + thrown = error + } + expect((thrown as Error).message).toBe(message) + expect(isAgentSessionPreSpawnError(thrown)).toBe(true) + } + }) }) diff --git a/src/main/claude/claude-structured-fork-launch.ts b/src/main/claude/claude-structured-fork-launch.ts index f58412844bf..256bb65ffcb 100644 --- a/src/main/claude/claude-structured-fork-launch.ts +++ b/src/main/claude/claude-structured-fork-launch.ts @@ -1,9 +1,13 @@ import type { AgentSessionForkTarget } from '../../shared/agent-session-fork' +import { AgentSessionPreSpawnError } from '../native-chat/agent-session-wire/structured-agent-session-adapter' import { claudeSessionIdForOrcaSession, type ClaudeStructuredLaunch } from './claude-structured-launch-resolution' +/** Rejections here are PRE-SPAWN by construction: this only rewrites launch arguments, and its one + * caller runs it before the child is opened. Saying so lets the wire settle the fork attempt + * instead of stranding the child record at `attempted`, which wedges the turn until app reload. */ export function applyClaudeStructuredForkLaunch( launch: ClaudeStructuredLaunch, fork: AgentSessionForkTarget, @@ -13,11 +17,11 @@ export function applyClaudeStructuredForkLaunch( fork.source.provider !== 'claude' || (launch.resumed && launch.providerSessionId !== fork.source.sessionId) ) { - throw new Error('agent_session_identity_required') + throw new AgentSessionPreSpawnError(new Error('agent_session_identity_required')) } const providerSessionId = claudeSessionIdForOrcaSession(sessionId) if (providerSessionId === fork.source.sessionId) { - throw new Error('agent_session_provider_handle_invalid') + throw new AgentSessionPreSpawnError(new Error('agent_session_provider_handle_invalid')) } return { ...launch, diff --git a/src/main/codex/codex-structured-fork-eligibility.test.ts b/src/main/codex/codex-structured-fork-eligibility.test.ts index e3d216af222..54c2a036497 100644 --- a/src/main/codex/codex-structured-fork-eligibility.test.ts +++ b/src/main/codex/codex-structured-fork-eligibility.test.ts @@ -7,7 +7,8 @@ import type { } from '../../shared/agent-session-journal-types' import { selectAgentSessionPrefix, - structuredForkEligibleItems + structuredForkEligibleItems, + structuredForkTurnAnchors } from '../../shared/agent-session-prefix' import type { StructuredAgentSessionEventSink } from '../native-chat/agent-session-wire/structured-agent-session-event-sink' import { createCodexJournalTranslator } from './codex-structured-journal-translation' @@ -95,7 +96,7 @@ describe('forking a turn the real Codex producer settled', () => { it('offers the settled turn as forkable and retains it inclusively', () => { const items = journalAfterTurn(true) const itemId = assistantId(items) - expect(structuredForkEligibleItems(items).has(itemId)).toBe(true) + expect(structuredForkEligibleItems(structuredForkTurnAnchors(items)).has(itemId)).toBe(true) const selected = selectAgentSessionPrefix({ items, itemId, @@ -116,7 +117,7 @@ describe('forking a turn the real Codex producer settled', () => { (item) => item.body.kind === 'status' && item.body.turnLifecycle?.state === 'running' ) ).toBe(true) - expect(structuredForkEligibleItems(items).has(itemId)).toBe(false) + expect(structuredForkEligibleItems(structuredForkTurnAnchors(items)).has(itemId)).toBe(false) expect( selectAgentSessionPrefix({ items, itemId, handle: HANDLE, boundary: 'through' }) ).toMatchObject({ ok: false, reason: 'busy' }) @@ -125,6 +126,8 @@ describe('forking a turn the real Codex producer settled', () => { it('keeps a settled earlier turn forkable while a later turn runs', () => { const settled = journalAfterTurn(true) const items = [...settled, ...journalAfterTurn(false, 'turn-2')] - expect(structuredForkEligibleItems(items)).toEqual(new Set([assistantId(settled)])) + expect(structuredForkEligibleItems(structuredForkTurnAnchors(items))).toEqual( + new Set([assistantId(settled)]) + ) }) }) diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-acquisition.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-acquisition.ts index a29c01c3e5f..6ce88df24da 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-acquisition.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-acquisition.ts @@ -8,6 +8,7 @@ import { claudeRewindAcquisitionProofs } from './structured-rewind-claude-proof' import type { AgentSessionRecord } from '../../../shared/agent-session-record' import { AgentSessionPreSpawnError, + isAgentSessionAcquisitionExitAmbiguous, isAgentSessionPreSpawnError, rethrowAfterAgentSessionAcquisitionCleanup } from './structured-agent-session-adapter' @@ -81,16 +82,42 @@ export async function acquireOwner( } } catch (error) { if (isAgentSessionPreSpawnError(error)) { - // The launch never resolved, so no provider session can exist: settle the attempt rather - // than strand it. A failure past this point stays ambiguous and keeps the guard. - await refuseStructuredForkAttempt(input.store, record, describePreSpawnRefusal(error)) + // Nothing spawned, so no provider session can exist: settle the attempt rather than strand it. + await refuseStructuredForkAttempt(input.store, record, describeRefusal(error)) throw error } - return rethrowAfterAgentSessionAcquisitionCleanup(input.adapter, record.sessionId, error) + try { + return await rethrowAfterAgentSessionAcquisitionCleanup( + input.adapter, + record.sessionId, + error + ) + } catch (settled) { + // Cleanup that PROVED a clean release leaves no provider session behind, so a fork attempt + // that died past the spawn — a fork-history verification timeout, a restore refusal — can + // settle too instead of wedging the turn forever. Anything ambiguous keeps `attempted` and + // goes on refusing, because a second attempt could then mint a second child. + const ambiguous = isAgentSessionAcquisitionExitAmbiguous(settled) + if (!ambiguous) { + await refuseStructuredForkAttempt(input.store, record, describeRefusal(settled)) + } + if (record.fork) { + // Which branch ran is the difference between a retryable fork and a wedged one, and it is + // invisible from the client, which sees one sentence either way. + console.warn( + `[agent-session] fork acquisition failed for ${record.sessionId}: ` + + `${ambiguous ? 'exit unproven, left attempted' : 'released cleanly, settled refused'}`, + settled + ) + } + throw settled + } } } -function describePreSpawnRefusal(error: unknown): string { +function describeRefusal(error: unknown): string { const cause = error instanceof Error ? (error.cause ?? error) : error - return cause instanceof Error && cause.message ? cause.message : 'agent_session_pre_spawn' + return cause instanceof Error && cause.message + ? cause.message + : 'agent_session_acquisition_failed' } diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-adapter.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-adapter.ts index 0cdaa3d7b31..15849190111 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-adapter.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-adapter.ts @@ -92,6 +92,22 @@ export function isAgentSessionPreSpawnError(error: unknown): error is AgentSessi return error instanceof Error && error.name === 'AgentSessionPreSpawnError' } +/** + * Whether a failure leaves the provider's fate UNSETTLED once acquisition cleanup has run. + * + * Both markers say the same thing from different distances: something may still be running that + * nobody proved dead. A caller may only settle an attempt terminally when this is false — for a + * fork that means the difference between retiring the attempt and risking a second child for a + * turn that already has one. + */ +export function isAgentSessionAcquisitionExitAmbiguous(error: unknown): boolean { + return ( + error instanceof Error && + (error.name === 'AgentSessionAcquisitionExitUnprovenError' || + error.name === 'AgentSessionAcquisitionRootExitObservedError') + ) +} + export type AgentSessionDispatchOutcome = /** The provider owns the turn now, under this identity. */ | { state: 'accepted'; providerIdentity: AgentJournalItemIdentity } diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-fork.test.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-fork.test.ts index 7c02f907c51..28fe9f926fd 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-fork.test.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-fork.test.ts @@ -20,6 +20,7 @@ import { attachFingerprintFields, type AgentSessionAttachParams } from './structured-agent-session-attach' +import { structuredSessionForkState } from '../../../renderer/src/components/native-chat/structured-agent-session-fork-state' import { HOST_TEST_NOW as NOW, hostTestAttachParams, @@ -106,11 +107,17 @@ async function setup(provider: 'claude' | 'codex' = 'codex') { }) return { state: 'accepted', providerIdentity: identity('turn-3', 0) } }) + // Whether acquisition cleanup can PROVE the provider child is gone is the whole question for a + // failed fork, so it is a knob here rather than a constant. + const release = vi.fn>( + async () => true + ) const adapter: StructuredAgentSessionAdapter = { acquire, forkSupport: () => ({ supported: true }), dispatch, - releaseAcquisition: async () => true, + releaseAcquisition: release, + readOptions: async () => ({ models: [], current: { model: 'model' } }), closeSession: async () => true, cancelTurn: async () => ({ cancelled: false }), answerPrompt: async () => {}, @@ -152,7 +159,7 @@ async function setup(provider: 'claude' | 'codex' = 'codex') { sessionId: 'child-session', fields: attachFingerprintFields(child) }) - return { host, store, params, child, source, acquire, inputs, identity } + return { host, store, params, child, source, acquire, release, inputs, identity } } describe('fork from a structured turn', () => { @@ -263,8 +270,15 @@ describe('fork from a structured turn', () => { const { host, store, child, source, inputs, acquire, identity } = await setup() inputs .get(source.sessionId)! + // The namespace the real translator keys a lifecycle row in; no consumer reads it today, but + // a fixture that models a row nothing emits is how this feature shipped inert once already. .events!.appendItem( - { provider: 'orca', clientMessageId: 'running' }, + { + provider: 'legacy', + agent: 'codex', + sessionId: source.sessionId, + recordId: 'turn-lifecycle:turn-2' + }, { kind: 'status', text: 'Working', turnLifecycle: { turnId: 'turn-2', state: 'running' } } ) const result = await host.fork(caller, child, { @@ -288,17 +302,48 @@ describe('fork from a structured turn', () => { }) it('does not publish a visible empty session when the fork outcome is unknown', async () => { - const { host, store, child, source, acquire } = await setup() + const { host, store, child, source, acquire, release } = await setup() acquire.mockImplementationOnce(async () => { throw new Error('provider response lost') }) - await expect(host.fork(caller, child, source)).rejects.toThrow('provider response lost') + // Cleanup could NOT prove the child is gone, which is what makes this outcome ambiguous. + release.mockResolvedValueOnce(false) + await expect(host.fork(caller, child, source)).rejects.toThrow() expect(store.getRecord('child-session')?.fork?.phase).toBe('attempted') expect(store.getVisibleSessionTabIndex().sessionIds).not.toContain('child-session') expect(host.hasSession('child-session')).toBe(false) // The ambiguity guard: the provider may hold a child, so the retry must never make a second. expect(await host.fork(caller, child, source)).toMatchObject({ ok: false }) expect(acquire).toHaveBeenCalledTimes(2) + expect(acquire.mock.calls.filter(([input]) => input.fork)).toHaveLength(1) + }) + + it('recovers a fork that died PAST the spawn once cleanup proved the child was released', async () => { + const { host, store, child, source, acquire, release } = await setup() + // A Codex fork reaches this after `thread/fork` succeeds: a forked-history verification timeout, + // or a restore refusal on a thread longer than the bounded restore queue. On a plain resume the + // same failures are simply retryable; stranding them here made the TURN unforkable until reload. + acquire.mockImplementationOnce(async () => { + throw new Error('codex app-server timed out verifying forked history') + }) + await expect(host.fork(caller, child, source)).rejects.toThrow() + expect(release).toHaveBeenCalled() + expect(store.getRecord('child-session')?.fork).toMatchObject({ phase: 'refused', retained: [] }) + expect(await host.fork(caller, child, source)).toMatchObject({ ok: true }) + expect(host.journalSnapshot('child-session').items).not.toHaveLength(0) + expect(acquire.mock.calls.filter(([input]) => input.fork)).toHaveLength(2) + }) + + it('keeps refusing when cleanup itself could not settle, however it failed', async () => { + const { host, store, child, source, acquire, release } = await setup() + acquire.mockImplementationOnce(async () => { + throw new Error('codex app-server timed out verifying forked history') + }) + release.mockRejectedValueOnce(new Error('provider child could not be reaped')) + await expect(host.fork(caller, child, source)).rejects.toThrow() + expect(store.getRecord('child-session')?.fork?.phase).toBe('attempted') + expect(await host.fork(caller, child, source)).toMatchObject({ ok: false }) + expect(acquire.mock.calls.filter(([input]) => input.fork)).toHaveLength(1) }) it('recovers a fork whose launch failed before any provider session existed', async () => { @@ -318,6 +363,46 @@ describe('fork from a structured turn', () => { expect(acquire.mock.calls.filter(([input]) => input.fork)).toHaveLength(2) }) + it('carries fork lineage from the proven phase through the wire to the controller field', async () => { + const { host, store, child, source, params } = await setup() + expect(await host.fork(caller, child, source)).toMatchObject({ ok: true }) + // Host gate -> wire field. Lineage is claimed only once a provider child is proven; an + // `attempted` fork has proven nothing, so a parent link there would name a chat that may + // never exist. The key is OMITTED rather than nulled, which is what keeps every ordinary + // session's payload fingerprint unmoved. + const forked = await host.readOptions('child-session') + expect(forked.forkedFrom).toEqual({ sessionId: source.sessionId }) + const parent = await host.readOptions(params.envelope.sessionId) + expect(parent).not.toHaveProperty('forkedFrom') + // The same live session, pinned back to a phase that has proven nothing: the durable record + // still names a source, and the gate is the only thing that stops it being published. + await store.transitionHandoff('child-session', (current) => ({ + ...current, + fork: { ...current.fork!, phase: 'attempted' as const } + })) + expect(await host.readOptions('child-session')).not.toHaveProperty('forkedFrom') + // Wire field -> controller field, the hop the renderer half actually reads. + const state = { items: [], fence: 1, cursor: { epoch: 'epoch' } } as unknown as Parameters< + typeof structuredSessionForkState + >[0] + expect( + structuredSessionForkState(state, 'child-session', { + sessionId: 'child-session', + commands: [], + forkSupported: true, + forkedFromSessionId: forked.forkedFrom?.sessionId + }).forkedFromSessionId + ).toBe(source.sessionId) + expect( + structuredSessionForkState(state, params.envelope.sessionId, { + sessionId: params.envelope.sessionId, + commands: [], + forkSupported: true, + forkedFromSessionId: parent.forkedFrom?.sessionId + }).forkedFromSessionId + ).toBeUndefined() + }) + it('resumes the proved child when journal publication fails instead of forking again', async () => { const { host, child, source, acquire, store } = await setup() const replace = vi diff --git a/src/main/native-chat/agent-session-wire/structured-agent-session-host-fork.ts b/src/main/native-chat/agent-session-wire/structured-agent-session-host-fork.ts index a3c44511a6d..69dc6cf9b1e 100644 --- a/src/main/native-chat/agent-session-wire/structured-agent-session-host-fork.ts +++ b/src/main/native-chat/agent-session-wire/structured-agent-session-host-fork.ts @@ -1,4 +1,8 @@ import type { AgentSessionForkSource } from '../../../shared/agent-session-fork' +import type { + AgentSessionAttachResult, + AgentSessionMutationResult +} from '../../../shared/agent-session-wire' import type { AgentSessionAttachParams } from './structured-agent-session-attach' import type { StructuredAgentSessionAttachContext } from './structured-agent-session-attach-context' import { forkStructuredAgentSession } from './structured-agent-session-fork' @@ -7,12 +11,28 @@ import type { StructuredAgentSessionCaller } from './structured-agent-session-ho export function createStructuredAgentSessionFork( attachContext: () => StructuredAgentSessionAttachContext ) { - return ( + return async ( caller: StructuredAgentSessionCaller, params: AgentSessionAttachParams, source: AgentSessionForkSource - ) => { + ): Promise> => { const context = attachContext() - return forkStructuredAgentSession(context, context, caller, params, source) + // The one choke point that sees every fork outcome with both session ids. Refusals raised past + // the fork's own vocabulary carry no `forkReason`, and the client can only render them as one + // generic sentence — without this line a failed fork left NO main-process trace to debug from. + const ids = `source=${source.sessionId} child=${params.envelope.sessionId}` + try { + const result = await forkStructuredAgentSession(context, context, caller, params, source) + if (!result.ok) { + console.warn( + `[agent-session] fork refused: ${ids} code=${result.refusal.code} ` + + `reason=${result.refusal.forkReason ?? 'none'} — ${result.refusal.message}` + ) + } + return result + } catch (error) { + console.warn(`[agent-session] fork failed before it could refuse: ${ids}`, error) + throw error + } } } diff --git a/src/renderer/src/components/native-chat/NativeChatForkedFromLine.test.tsx b/src/renderer/src/components/native-chat/NativeChatForkedFromLine.test.tsx index eb3dfa65f25..95d4d506327 100644 --- a/src/renderer/src/components/native-chat/NativeChatForkedFromLine.test.tsx +++ b/src/renderer/src/components/native-chat/NativeChatForkedFromLine.test.tsx @@ -18,6 +18,7 @@ vi.mock('@/store', () => ({ })) import { NativeChatForkedFromLine } from './NativeChatForkedFromLine' +import { structuredSessionForkState } from './structured-agent-session-fork-state' afterEach(cleanup) @@ -33,6 +34,43 @@ function parentTab(overrides: Partial = {}): Partial { } describe('forked-from lineage line', () => { + it('renders the parent the CONTROLLER derived, not one handed straight to the prop', () => { + // Closes the last hop of the lineage chain: wire field -> controller field -> this component. + // Rendering with a literal prop proves the component, and nothing that feeds it. + tabs.current = [parentTab()] + const state = { items: [], fence: 1, cursor: { epoch: 'epoch' } } as unknown as Parameters< + typeof structuredSessionForkState + >[0] + const derived = structuredSessionForkState(state, 'child-session', { + sessionId: 'child-session', + commands: [], + forkSupported: true, + forkedFromSessionId: 'parent-session' + }) + render( + + ) + expect(screen.getByRole('button', { name: 'Rewrite the parser' })).toBeInTheDocument() + // A host that predates the field sends nothing, and the line must simply not appear. + cleanup() + render( + + ) + expect(screen.queryByText('Forked from')).not.toBeInTheDocument() + }) + it('names the parent chat and opens its tab', () => { activate.mockReset() tabs.current = [parentTab()] diff --git a/src/renderer/src/components/native-chat/NativeChatMessageList.fork-row-cost.test.tsx b/src/renderer/src/components/native-chat/NativeChatMessageList.fork-row-cost.test.tsx new file mode 100644 index 00000000000..bcd180c5000 --- /dev/null +++ b/src/renderer/src/components/native-chat/NativeChatMessageList.fork-row-cost.test.tsx @@ -0,0 +1,84 @@ +// @vitest-environment happy-dom +import { cleanup, render } from '@testing-library/react' +import { afterEach, expect, it, vi } from 'vitest' +import type * as MessageRowModule from './NativeChatMessageRow' +import type { NativeChatMessage } from '../../../../shared/native-chat-types' +import type { NativeChatLiveSession } from './use-native-chat-live-session' + +// Counting REAL row renders rather than props: the transcript is not windowed, so a prop that +// changes on every row re-renders the entire chat — the cost the row memo exists to prevent. +const renders = vi.hoisted(() => ({ byId: new Map() })) +vi.mock('./NativeChatMessageRow', async (importOriginal) => { + const actual = await importOriginal() + const { createElement, memo } = await import('react') + const Wrapped = (props: React.ComponentProps) => { + renders.byId.set(props.message.id, (renders.byId.get(props.message.id) ?? 0) + 1) + return createElement(actual.MessageRow, props) + } + // memo() so the parent's re-render alone does not re-run it; only a changed prop does. + return { ...actual, MessageRow: memo(Wrapped) } +}) + +const { NativeChatMessageList } = await import('./NativeChatMessageList') +afterEach(cleanup) + +const TURNS = 12 +const ANCHOR = `assistant-${TURNS - 1}` + +function messages(): NativeChatMessage[] { + return Array.from({ length: TURNS }, (_, index) => index).flatMap((index) => [ + { + id: `user-${index}`, + role: 'user' as const, + blocks: [{ type: 'text' as const, text: `ask ${index}` }], + timestamp: index * 2 + 1, + source: 'transcript' as const + }, + { + id: `assistant-${index}`, + role: 'assistant' as const, + blocks: [{ type: 'text' as const, text: `answer ${index}` }], + timestamp: index * 2 + 2, + source: 'transcript' as const + } + ]) +} + +// Hoisted: a transcript rebuilt per render hands every row a new `message` and would bust the memo +// for reasons that have nothing to do with the prop under test. +const MESSAGES = messages() +const onFork = () => {} +const loadEarlier = () => {} + +function Transcript({ pending }: { pending: boolean }) { + const session: NativeChatLiveSession = { + messages: MESSAGES, + status: 'ready', + sessionId: 'session', + agent: 'codex', + hasMore: false, + loadingEarlier: false, + loadEarlier, + readPhase: 'ready' + } + return ( + + ) +} + +it('re-renders only the anchor row when a fork goes pending', () => { + const { rerender } = render() + expect(renders.byId.get(ANCHOR)).toBe(1) + renders.byId.clear() + rerender() + // The anchor is the one row that reads `pending`, so it is the only one allowed to re-render. + expect(renders.byId.get(ANCHOR)).toBe(1) + expect([...renders.byId].filter(([id]) => id !== ANCHOR)).toEqual([]) +}) diff --git a/src/renderer/src/components/native-chat/NativeChatMessageList.tsx b/src/renderer/src/components/native-chat/NativeChatMessageList.tsx index 00612f6eef8..7b6a365e637 100644 --- a/src/renderer/src/components/native-chat/NativeChatMessageList.tsx +++ b/src/renderer/src/components/native-chat/NativeChatMessageList.tsx @@ -279,8 +279,8 @@ export function NativeChatMessageList({ ? turnStatuses.completedByTurn[turnKey] : undefined const receipt = receipts.get(message.id) - // Only the eligible row gets the handler: handing it to every row would make the - // whole transcript re-render whenever the fork action is rebuilt. + // Only the eligible row gets the handler AND the pending flag: handing either to every + // row would re-render the whole (unwindowed) transcript on every fork click. const forkEligible = forkAction?.eligibleIds.has(message.id) === true const turnDiff = turnKey && turnKeys[index + 1] !== turnKey ? turnDiffs.get(turnKey) : undefined @@ -313,7 +313,7 @@ export function NativeChatMessageList({ activityExpandOverride={turnKey ? expandedTurnIds.has(turnKey) : undefined} runtimeContext={runtimeContext} forkEligible={forkEligible} - forkPending={forkAction?.pending} + forkPending={forkEligible ? forkAction?.pending : undefined} onFork={forkEligible ? forkAction?.onFork : undefined} /> )} diff --git a/src/renderer/src/components/native-chat/NativeChatMessageRow.tsx b/src/renderer/src/components/native-chat/NativeChatMessageRow.tsx index 915761363b4..61ee29529a1 100644 --- a/src/renderer/src/components/native-chat/NativeChatMessageRow.tsx +++ b/src/renderer/src/components/native-chat/NativeChatMessageRow.tsx @@ -4,16 +4,11 @@ import CommentMarkdown, { } from '@/components/sidebar/CommentMarkdown' import { cn } from '@/lib/utils' import { translate } from '@/i18n/i18n' -import { - isSubagentGroupFallbackText, - subagentGroupBlocks -} from '../../../../shared/native-chat-subagent-summary' -import { - isSubagentGroupBlock, - type NativeChatMessage, - type NativeChatToolCallBlock +import { nativeChatRowContent } from '../../../../shared/native-chat-row-content' +import type { + NativeChatMessage, + NativeChatToolCallBlock } from '../../../../shared/native-chat-types' -import { splitNativeChatBlocks } from './native-chat-tool-fold' import { NativeChatToolRun } from './NativeChatToolRun' import { NativeChatNoticeRow } from './NativeChatNoticeRow' import { NativeChatMessageTimestamp } from './NativeChatMessageTimestamp' @@ -72,27 +67,11 @@ export const MessageRow = memo(function MessageRow({ // One pass per block set: a streaming turn re-renders this row on every frame, and these // derivations used to re-run each time even though `message.blocks` had not changed. const { hasImages, markdown, prose, subagentGroups, tools } = useMemo(() => { - const split = splitNativeChatBlocks(message.blocks) - const groups = subagentGroupBlocks(split.prose) - // A spawn-group row carries a plain-text twin so a client without the block - // type still reads the roster. This one draws the block, so the twin is - // dropped rather than printed beside it — only the twin, never the prose - // beside it: the block is provider-agnostic, so a lane that folds a roster - // into a message with real text must not lose that text here. - const prose = - groups.length === 0 - ? split.prose - : split.prose.filter( - (block) => - !isSubagentGroupBlock(block) && - !(block.type === 'text' && isSubagentGroupFallbackText(block.text)) - ) + const content = nativeChatRowContent(message.blocks) return { - tools: split.tools, - prose, - subagentGroups: groups, - markdown: nativeChatProseToMarkdown(prose), - hasImages: prose.some((block) => block.type === 'image-ref') + ...content, + markdown: nativeChatProseToMarkdown(content.prose), + hasImages: content.prose.some((block) => block.type === 'image-ref') } }, [message.blocks]) const isUser = message.role === 'user' diff --git a/src/renderer/src/components/native-chat/native-chat-prose.ts b/src/renderer/src/components/native-chat/native-chat-prose.ts index 68242e48e5b..b159a7ab273 100644 --- a/src/renderer/src/components/native-chat/native-chat-prose.ts +++ b/src/renderer/src/components/native-chat/native-chat-prose.ts @@ -1,8 +1 @@ -import { isTextBlock, type NativeChatBlock } from '../../../../shared/native-chat-types' - -export function nativeChatProseToMarkdown(blocks: NativeChatBlock[]): string { - return blocks - .map((block) => (isTextBlock(block) ? block.text : '')) - .filter((part) => part.length > 0) - .join('\n\n') -} +export { nativeChatProseToMarkdown } from '../../../../shared/native-chat-row-content' diff --git a/src/renderer/src/components/native-chat/structured-agent-session-fork-command.ts b/src/renderer/src/components/native-chat/structured-agent-session-fork-command.ts index 4737513712d..1d489cde101 100644 --- a/src/renderer/src/components/native-chat/structured-agent-session-fork-command.ts +++ b/src/renderer/src/components/native-chat/structured-agent-session-fork-command.ts @@ -6,7 +6,8 @@ import { import type { AgentSessionForkSource } from '../../../../shared/agent-session-fork' import type { AgentSessionAttachResult, - AgentSessionMutationResult + AgentSessionMutationResult, + AgentSessionWireRefusal } from '../../../../shared/agent-session-wire' import type { RuntimeClientTarget } from '@/runtime/runtime-rpc-client' import { @@ -15,7 +16,12 @@ import { } from '@/runtime/structured-agent-session-client' import { translate } from '@/i18n/i18n' -type ForkAttempt = { params: StructuredAgentSessionCreateParams; running?: Promise } +type ForkAttempt = { + params: StructuredAgentSessionCreateParams + running?: Promise + /** The last outcome left this child's fate unknown, so its ids are the only way to adjudicate it. */ + unconfirmed?: boolean +} const attempts = new Map() const MAX_TRACKED_ATTEMPTS = 128 @@ -41,7 +47,12 @@ export function forkStructuredSessionFromTurn(input: { if (attempt?.running) { return attempt.running } - if (!attempt) { + if (attempt) { + // Re-insert: the map's insertion order IS the eviction order, so reuse has to refresh recency + // or the turn a user keeps retrying is evicted before one they touched once and abandoned. + attempts.delete(key) + attempts.set(key, attempt) + } else { attempt = { params: structuredAgentSessionCreateParams({ sessionId: createStructuredAgentSessionId(input.agent, () => crypto.randomUUID()), @@ -60,7 +71,7 @@ export function forkStructuredSessionFromTurn(input: { .then( (result) => { if (!result.ok) { - throw refusalError(key, result.refusal.forkReason) + throw refusalError(current, key, result.refusal) } attempts.delete(key) return result.value.sessionId @@ -72,6 +83,7 @@ export function forkStructuredSessionFromTurn(input: { attempts.delete(key) throw error } + current.unconfirmed = true throw new Error(unconfirmed()) } ) @@ -81,29 +93,54 @@ export function forkStructuredSessionFromTurn(input: { return current.running } -/** Bound the table by EVICTING the oldest idle entry. Refusing at the cap instead wedged forking - * app-wide — every session, tab and worktree — until a restart, reported as an unconfirmed fork. */ +/** Bound the table by EVICTING the least recently used entry with nothing left to adjudicate. + * Refusing at the cap instead wedged forking app-wide — every session, tab and worktree — until a + * restart, reported as an unconfirmed fork. + * + * An UNCONFIRMED entry is idle but is retained precisely so a retry can adjudicate the child that + * may already exist, so it is evicted only once nothing else can be: dropping it makes the next + * fork of that turn mint a SECOND provider session, the one thing this ledger exists to prevent. */ function track(key: string, attempt: ForkAttempt): void { while (attempts.size >= MAX_TRACKED_ATTEMPTS) { - let evicted = false - for (const [candidate, entry] of attempts) { - if (!entry.running) { - attempts.delete(candidate) - evicted = true - break - } - } - if (!evicted) { + if ( + !evictOldest((entry) => !entry.running && !entry.unconfirmed) && + !evictOldest((entry) => !entry.running) + ) { break } } attempts.set(key, attempt) } +function evictOldest(admissible: (entry: ForkAttempt) => boolean): boolean { + for (const [candidate, entry] of attempts) { + if (admissible(entry)) { + attempts.delete(candidate) + return true + } + } + return false +} + /** A settled refusal proves the host minted no provider session, so the child id is retired and a * retry starts clean. An unknown or mismatched outcome must reuse it to adjudicate the original. */ -function refusalError(key: string, reason: string | undefined): Error { - if (reason === undefined || reason === 'outcome-unknown' || reason === 'proof-mismatch') { +function refusalError(attempt: ForkAttempt, key: string, refusal: AgentSessionWireRefusal): Error { + const reason = refusal.forkReason + if (reason === undefined) { + // No `forkReason` means the host refused somewhere with no fork vocabulary at all — a provider + // that never finished starting, a stale checkpoint, an unsupported workspace. Every refusal + // carries a `code` and a `message`; reporting them all as "could not be confirmed" threw away + // the only diagnostic anyone had. The attempt is still RETAINED, because a refusal raised after + // acquisition began may have left a child behind. + attempt.unconfirmed = true + return new Error( + translate('components.native-chat.forkRefused', 'Could not fork this turn: {{reason}}', { + reason: refusal.message + }) + ) + } + if (reason === 'outcome-unknown' || reason === 'proof-mismatch') { + attempt.unconfirmed = true return new Error(unconfirmed()) } attempts.delete(key) diff --git a/src/renderer/src/components/native-chat/structured-conversation-command-send.ts b/src/renderer/src/components/native-chat/structured-conversation-command-send.ts index a273572ea0c..77be7d336e7 100644 --- a/src/renderer/src/components/native-chat/structured-conversation-command-send.ts +++ b/src/renderer/src/components/native-chat/structured-conversation-command-send.ts @@ -46,26 +46,3 @@ export function isUnconfirmedConversationCommand(method: string, value: unknown) (value as AgentSessionConversationCommandResult).state === 'unknown' ) } - -export function structuredConversationCommandRunner( - pending: { current: boolean }, - blocked: boolean, - mutate: ( - method: string, - fingerprintMethod: string, - fields: Record - ) => Promise -) { - return (command: AgentSessionConversationCommand) => - sendStructuredConversationCommand({ - command, - pending, - blocked, - send: (command) => - mutate( - 'agentSession.conversationCommand', - 'agentSession.conversationCommand', - { command } - ) - }) -} diff --git a/src/renderer/src/components/native-chat/use-structured-fork-action.test.tsx b/src/renderer/src/components/native-chat/use-structured-fork-action.test.tsx index 60c933a42e8..12f567556a9 100644 --- a/src/renderer/src/components/native-chat/use-structured-fork-action.test.tsx +++ b/src/renderer/src/components/native-chat/use-structured-fork-action.test.tsx @@ -33,7 +33,10 @@ vi.mock('@/runtime/runtime-worktree-selector', () => ({ vi.mock('@/lib/structured-agent-session-tab-activation', () => ({ activateStructuredAgentSessionById: activate })) -vi.mock('@/i18n/i18n', () => ({ translate: (_key: string, fallback: string) => fallback })) +vi.mock('@/i18n/i18n', () => ({ + translate: (_key: string, fallback: string, options?: Record) => + fallback.replace(/{{(\w+)}}/g, (_match, name: string) => options?.[name] ?? '') +})) vi.mock('sonner', () => ({ toast: { success: toastSuccess } })) import { useStructuredForkAction } from './use-structured-fork-action' @@ -43,17 +46,13 @@ type Controller = Parameters[1] const props = { agent: 'codex', target: { kind: 'local' } } as unknown as Props -/** text -> tool call -> text, the shape of a normal turn. Only the last row is drawn as an - * assistant row, so only it can carry the control. */ +/** text -> tool call -> text, the shape of a normal turn. Tool activity is a `tool-call` BODY, + * which is what the live translators emit; only the last row draws a control cluster. */ function turn(): AgentJournalRenderItem[] { const bodies: AgentJournalRenderItem['body'][] = [ { kind: 'message', role: 'user', blocks: [{ type: 'text', text: 'Ask' }] }, { kind: 'message', role: 'assistant', blocks: [{ type: 'text', text: 'Looking' }] }, - { - kind: 'message', - role: 'assistant', - blocks: [{ type: 'tool-call', name: 'read', input: {} }] - }, + { kind: 'tool-call', name: 'read', input: {}, state: 'completed' }, { kind: 'message', role: 'assistant', blocks: [{ type: 'text', text: 'Answered' }] } ] return bodies.map((body, index) => ({ @@ -65,9 +64,44 @@ function turn(): AgentJournalRenderItem[] { })) } -function controller(overrides: { isWorking?: boolean; items?: AgentJournalRenderItem[] } = {}) { +/** The settled turn above, followed by a prompt whose turn is still running. */ +function turnThenLiveTurn(): AgentJournalRenderItem[] { + return [ + ...turn(), + { + itemId: 'codex:parent:b:0', + revision: 1, + body: { + kind: 'message' as const, + role: 'user' as const, + blocks: [{ type: 'text' as const, text: 'Next' }] + }, + sequence: 4, + observedAt: 1 + }, + { + itemId: 'legacy:codex:parent:turn-lifecycle%3Ab', + revision: 1, + body: { + kind: 'status' as const, + text: 'Working', + turnLifecycle: { turnId: 'b', state: 'running' as const } + }, + sequence: 5, + observedAt: 1 + } + ] +} + +function controller( + overrides: { + isWorking?: boolean + items?: AgentJournalRenderItem[] + forkSupported?: boolean + } = {} +) { return { - forkSupported: true, + forkSupported: overrides.forkSupported ?? true, // A distinct parent per test: the command's replay table lives for the module's lifetime. forkSource: { sessionId: `parent-${Math.random()}`, @@ -80,13 +114,57 @@ function controller(overrides: { isWorking?: boolean; items?: AgentJournalRender } describe('fork action eligibility work', () => { - it('does no eligibility scan while a turn is live', () => { - scanned.mockClear() - const items = turn() - // A live turn emits a journal delta per frame and the memo runs before the early return, so an - // ungated memo rescans the whole transcript for a result nothing can use. + it('still offers SETTLED turns while another turn is streaming', () => { + // The host is per-turn: `selectAgentSessionPrefix(boundary:'through')` serves a settled turn and + // refuses only the live one as `busy`. Gating the hook on session-level `isWorking` stripped the + // action off EVERY turn the moment anything streamed — the case where branching is most useful. + const { result } = renderHook(() => + useStructuredForkAction( + props, + controller({ isWorking: true, items: turnThenLiveTurn() }), + 'worktree', + () => {} + ) + ) + expect([...(result.current?.eligibleIds ?? [])]).toEqual(['codex:parent:a:3']) + }) + + it('withholds the LIVE turn even so', () => { + const { result } = renderHook(() => + useStructuredForkAction( + props, + controller({ isWorking: true, items: turnThenLiveTurn() }), + 'worktree', + () => {} + ) + ) + expect(result.current?.eligibleIds.has('codex:parent:b:0')).toBe(false) + }) + + it('keeps its click handler stable across journal deltas so anchor rows do not re-render', () => { + // A live turn republishes the journal every frame. A handler rebuilt per frame would hand every + // anchor row a new prop and defeat the row memo the transcript depends on. Everything except the + // journal is held fixed, so only the per-frame republish is under test. + const stable = controller({ isWorking: true }) + const onError = () => {} + let items = turnThenLiveTurn() const { result, rerender } = renderHook(() => - useStructuredForkAction(props, controller({ isWorking: true, items }), 'worktree', () => {}) + useStructuredForkAction(props, { ...stable, journalItems: items }, 'worktree', onError) + ) + const first = result.current?.onFork + for (let index = 0; index < 5; index += 1) { + items = [...items] + rerender() + } + expect(result.current?.onFork).toBe(first) + // Still resolves against the newest journal, not the one the handler closed over. + expect(result.current?.eligibleIds.has('codex:parent:a:3')).toBe(true) + }) + + it('does no eligibility scan when forking is unavailable', () => { + scanned.mockClear() + const { result, rerender } = renderHook(() => + useStructuredForkAction(props, controller({ forkSupported: false }), 'worktree', () => {}) ) for (let index = 0; index < 5; index += 1) { rerender() @@ -115,7 +193,6 @@ describe('two rows of one turn cannot mint two forks', () => { // replay table sees one key and the second click joins the first attempt. act(() => { result.current?.onFork('codex:parent:a:1') - result.current?.onFork('codex:parent:a:2') result.current?.onFork('codex:parent:a:3') }) expect(call).toHaveBeenCalledTimes(1) @@ -128,6 +205,52 @@ describe('two rows of one turn cannot mint two forks', () => { }) }) + it('reports the host reason for a refusal that carries no fork reason', async () => { + // Rendered QA hit exactly this and could not debug it: a refusal raised past the fork's own + // vocabulary — a provider that never finished starting, a stale checkpoint — has no + // `forkReason`, and every one of them reached the user as "could not be confirmed". + call.mockReset() + const errors: string[] = [] + call.mockResolvedValue({ + ok: false, + refusal: { + code: 'agent_session_operation_invalid', + message: 'Claude did not finish starting session child within 10 seconds.' + } + }) + const { result } = renderHook(() => + useStructuredForkAction(props, controller(), 'worktree', (message) => errors.push(message)) + ) + await act(async () => { + result.current?.onFork('codex:parent:a:3') + }) + expect(errors).toEqual([ + 'Could not fork this turn: Claude did not finish starting session child within 10 seconds.' + ]) + }) + + it('still says UNCONFIRMED when the host could not adjudicate the child', async () => { + call.mockReset() + const errors: string[] = [] + call.mockResolvedValue({ + ok: false, + refusal: { + code: 'agent_session_operation_invalid', + message: 'agent_session_fork:outcome-unknown', + forkReason: 'outcome-unknown' + } + }) + const { result } = renderHook(() => + useStructuredForkAction(props, controller(), 'worktree', (message) => errors.push(message)) + ) + await act(async () => { + result.current?.onFork('codex:parent:a:3') + }) + expect(errors).toEqual([ + 'A fork could not be confirmed. Retry the same turn to check its outcome.' + ]) + }) + it('offers a way into the child instead of stealing the surface', async () => { call.mockReset() toastSuccess.mockReset() diff --git a/src/renderer/src/components/native-chat/use-structured-fork-action.ts b/src/renderer/src/components/native-chat/use-structured-fork-action.ts index 9a96acaac49..b10f25fd6d9 100644 --- a/src/renderer/src/components/native-chat/use-structured-fork-action.ts +++ b/src/renderer/src/components/native-chat/use-structured-fork-action.ts @@ -1,6 +1,9 @@ -import { useCallback, useMemo, useState } from 'react' +import { useCallback, useEffect, useMemo, useRef, useState } from 'react' import { toast } from 'sonner' -import { structuredForkTurnAnchors } from '../../../../shared/agent-session-prefix' +import { + structuredForkEligibleItems, + structuredForkTurnAnchors +} from '../../../../shared/agent-session-prefix' import type { NativeChatStructuredViewProps } from './native-chat-view-types' import type { useStructuredAgentSession } from './use-structured-agent-session' import { forkStructuredSessionFromTurn } from './structured-agent-session-fork-command' @@ -18,20 +21,25 @@ export function useStructuredForkAction( ) { const [pending, setPending] = useState(false) const agent = props.agent === 'claude' ? 'claude' : props.agent === 'codex' ? 'codex' : undefined - const enabled = Boolean( - controller.forkSupported && - controller.forkSource && - worktreeId && - !controller.isWorking && - agent - ) - // Hooks cannot be skipped, so the unavailable case is gated inside the memo instead: a live turn - // emits a journal delta per frame and every one of them would rescan for a discarded result. + // Deliberately NOT gated on `controller.isWorking`. The host is per-turn — it refuses only the + // LIVE turn as `busy` and serves every settled one — so gating the whole hook on session-level + // work stripped the action off every turn in the chat the moment any turn started streaming, + // which is exactly when branching off an earlier answer is most useful. `structuredForkTurnAnchors` + // already withholds the running turn. + const enabled = Boolean(controller.forkSupported && controller.forkSource && worktreeId && agent) + // Hooks cannot be skipped, so the unavailable case is gated inside the memo instead. const anchors = useMemo( () => (enabled ? structuredForkTurnAnchors(controller.journalItems ?? []) : NO_ANCHORS), [enabled, controller.journalItems] ) - const eligibleIds = useMemo(() => new Set(anchors.values()), [anchors]) + const eligibleIds = useMemo(() => structuredForkEligibleItems(anchors), [anchors]) + // A live turn republishes the journal every frame, so the anchor map is a new object every frame. + // Reading it through a ref keeps `onFork` stable, or each frame would hand every anchor row a new + // handler and re-render it — the cost the row memo exists to avoid. + const anchorsRef = useRef(anchors) + useEffect(() => { + anchorsRef.current = anchors + }, [anchors]) // Read through the fields, not the object: `forkSource` is rebuilt every render, and depending on // it would hand every eligible row a new handler and defeat the row memo. const sourceSessionId = controller.forkSource?.sessionId @@ -42,7 +50,7 @@ export function useStructuredForkAction( (itemId: string) => { // The clicked row resolves to its TURN's anchor, so the command's replay key names the turn: // two rows of one turn join a single attempt instead of minting two identical children. - const anchor = anchors.get(itemId) + const anchor = anchorsRef.current.get(itemId) if ( !anchor || !worktreeId || @@ -78,16 +86,7 @@ export function useStructuredForkAction( .catch((error: unknown) => onError(error instanceof Error ? error.message : String(error))) .finally(() => setPending(false)) }, - [ - agent, - anchors, - expectedEpoch, - expectedRuntimeFence, - onError, - sourceSessionId, - target, - worktreeId - ] + [agent, expectedEpoch, expectedRuntimeFence, onError, sourceSessionId, target, worktreeId] ) if (!enabled || !controller.forkSource || !worktreeId || !agent) { return undefined diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 4c707ea1105..3b6e0fbfe34 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -17253,8 +17253,10 @@ "unconfirmed": "Conversation operation was not confirmed." }, "forkServerUpdateRequired": "Forking requires a newer Orca server. Update the server and try again.", + "rewindServerUpdateRequired": "Rewinding requires a newer Orca server. Update the server and try again.", "forkFromTurn": "Fork from this turn", "forkFailed": "Could not fork this turn.", + "forkRefused": "Could not fork this turn: {{reason}}", "forkBusy": "Wait for this conversation to finish, then fork the turn.", "forkUnsupported": "This conversation cannot be forked.", "forkHistoryLimit": "This turn carries too much history to fork.", diff --git a/src/renderer/src/runtime/structured-agent-session-client.ts b/src/renderer/src/runtime/structured-agent-session-client.ts index 5ed8769337b..d2f424ff270 100644 --- a/src/renderer/src/runtime/structured-agent-session-client.ts +++ b/src/renderer/src/runtime/structured-agent-session-client.ts @@ -33,7 +33,10 @@ export async function callStructuredAgentSession( )) ) { throw new StructuredAgentSessionCapabilityError( - 'Rewinding requires a newer Orca server. Update the server and try again.' + translate( + 'components.native-chat.rewindServerUpdateRequired', + 'Rewinding requires a newer Orca server. Update the server and try again.' + ) ) } if ( diff --git a/src/shared/agent-session-prefix.test.ts b/src/shared/agent-session-prefix.test.ts index 7219266bfb8..b47e2e9d710 100644 --- a/src/shared/agent-session-prefix.test.ts +++ b/src/shared/agent-session-prefix.test.ts @@ -20,7 +20,7 @@ function items(provider: 'claude' | 'codex', running?: string): AgentJournalRend itemId: provider === 'codex' ? `codex:parent:${turnId}:0` : `claude:parent:${turnId}-prompt`, revision: 1, - body: { kind: 'message', role: 'user', blocks: [] }, + body: { kind: 'message', role: 'user', blocks: [{ type: 'text', text: `${turnId} ask` }] }, sequence: turn * 3, observedAt: 1 } @@ -37,7 +37,11 @@ function items(provider: 'claude' | 'codex', running?: string): AgentJournalRend rows.push({ itemId: provider === 'codex' ? `codex:parent:${turnId}:1` : `claude:parent:${turnId}-answer`, revision: 1, - body: { kind: 'message', role: 'assistant', blocks: [] }, + body: { + kind: 'message', + role: 'assistant', + blocks: [{ type: 'text', text: `${turnId} answer` }] + }, sequence: turn * 3 + 2, observedAt: 1 }) @@ -66,7 +70,7 @@ describe('bounded conversation prefix', () => { history.slice(0, 2).map((item) => item.itemId) ) } - expect(structuredForkEligibleItems(history)).toEqual( + expect(structuredForkEligibleItems(structuredForkTurnAnchors(history))).toEqual( new Set([history[1]!.itemId, history[3]!.itemId]) ) } @@ -112,7 +116,9 @@ describe('bounded conversation prefix', () => { expect( selectAgentSessionPrefix({ ...args, items: live, itemId: live[4]!.itemId }) ).toMatchObject({ ok: false, reason: 'busy' }) - expect(structuredForkEligibleItems(live)).toEqual(new Set([live[1]!.itemId])) + expect(structuredForkEligibleItems(structuredForkTurnAnchors(live))).toEqual( + new Set([live[1]!.itemId]) + ) }) it('inherits the retained entry and UTF-8 byte bounds', () => { @@ -125,33 +131,9 @@ describe('bounded conversation prefix', () => { }) }) -/** A turn whose assistant side is text -> tool call -> text: the shape a normal turn actually has, - * and the one that used to expose an action on every row. */ -function multiRowTurn(): AgentJournalRenderItem[] { - const rows: [string, AgentJournalRenderItem['body']][] = [ - [ - 'codex:parent:a:0', - { kind: 'message', role: 'user', blocks: [{ type: 'text', text: 'Ask' }] } - ], - [ - 'codex:parent:a:1', - { kind: 'message', role: 'assistant', blocks: [{ type: 'text', text: 'Looking' }] } - ], - [ - 'codex:parent:a:2', - { - kind: 'message', - role: 'assistant', - blocks: [{ type: 'tool-call', name: 'read', input: {} }] - } - ], - [ - 'codex:parent:a:3', - { kind: 'message', role: 'assistant', blocks: [{ type: 'text', text: 'Answered' }] } - ] - ] - return rows.map(([itemId, body], index) => ({ - itemId, +function journal(bodies: AgentJournalRenderItem['body'][]): AgentJournalRenderItem[] { + return bodies.map((body, index) => ({ + itemId: `codex:parent:a:${index}`, revision: 1, body, sequence: index, @@ -159,10 +141,39 @@ function multiRowTurn(): AgentJournalRenderItem[] { })) } +/** A turn whose assistant side is text -> tool call -> text: the shape a normal turn actually has. + * + * Tool activity is a `tool-call` BODY, which is what both live translators emit — never an + * assistant message whose blocks happen to all be tool blocks. Only the bridge-era importer + * produces that, so a fixture built from it tests a journal Orca cannot make. */ +function multiRowTurn(): AgentJournalRenderItem[] { + return journal([ + { kind: 'message', role: 'user', blocks: [{ type: 'text', text: 'Ask' }] }, + { kind: 'message', role: 'assistant', blocks: [{ type: 'text', text: 'Looking' }] }, + { kind: 'tool-call', name: 'read', input: {}, state: 'completed' }, + { kind: 'message', role: 'assistant', blocks: [{ type: 'text', text: 'Answered' }] } + ]) +} + +function withRunningTurn(items: AgentJournalRenderItem[], turnId = 'a'): AgentJournalRenderItem[] { + return [ + ...items, + { + itemId: `legacy:codex:parent:turn-lifecycle%3A${turnId}`, + revision: 1, + body: { kind: 'status', text: 'Working', turnLifecycle: { turnId, state: 'running' } }, + sequence: items.length, + observedAt: 1 + } + ] +} + describe('one fork action per turn', () => { it('exposes a single action for a turn that spans several assistant rows', () => { const history = multiRowTurn() - expect(structuredForkEligibleItems(history)).toEqual(new Set(['codex:parent:a:3'])) + expect(structuredForkEligibleItems(structuredForkTurnAnchors(history))).toEqual( + new Set(['codex:parent:a:3']) + ) }) it('resolves every row of that turn to the SAME target, so two clicks cannot mint two forks', () => { @@ -170,29 +181,96 @@ describe('one fork action per turn', () => { // The dedupe that matters is here, not in eligibility: the fork command keys its replay table // by the resolved target, so sibling rows join one attempt however they were surfaced. expect(anchors.get('codex:parent:a:1')).toBe('codex:parent:a:3') - expect(anchors.get('codex:parent:a:2')).toBe('codex:parent:a:3') + expect(anchors.get('codex:parent:a:3')).toBe('codex:parent:a:3') expect(new Set(anchors.values()).size).toBe(1) }) it('never anchors on a trailing tool-only row, which the transcript folds away', () => { const history = multiRowTurn() - // Drop the closing prose: the last assistant row is now pure tool activity, which renders no - // row of its own and so could carry no control. + // Drop the closing prose: the turn now ends in tool activity, which the transcript folds into + // the row above and draws no row of its own, so it could carry no control. const folded = history.slice(0, 3) - expect(structuredForkEligibleItems(folded)).toEqual(new Set(['codex:parent:a:1'])) + expect(structuredForkEligibleItems(structuredForkTurnAnchors(folded))).toEqual( + new Set(['codex:parent:a:1']) + ) + }) + + it('never anchors on a row that draws no control, even though it draws SOMETHING', () => { + // An assistant row of pure images renders its attachments but no control cluster, so anchoring + // there costs the turn its only fork affordance in silence. Reachable via Claude: Codex always + // prepends a text block. `isToolOnlyBlockSet` calls this row "drawn" and picks it. + const withTrailingImage = journal([ + { kind: 'message', role: 'user', blocks: [{ type: 'text', text: 'Ask' }] }, + { kind: 'message', role: 'assistant', blocks: [{ type: 'text', text: 'Answered' }] }, + { + kind: 'message', + role: 'assistant', + blocks: [{ type: 'image-ref', path: '/tmp/shot.png', alt: 'screenshot' }] + } + ]) + const anchors = structuredForkTurnAnchors(withTrailingImage) + expect(structuredForkEligibleItems(anchors)).toEqual(new Set(['codex:parent:a:1'])) + expect(anchors.get('codex:parent:a:2')).toBe('codex:parent:a:1') + }) + + it('gives a turn with nothing drawable NO anchor rather than a phantom one', () => { + // The `?? turn.at(-1)` fallback anchored here anyway, and the transcript then drew nothing. + const imageOnlyTurn = journal([ + { kind: 'message', role: 'user', blocks: [{ type: 'text', text: 'Ask' }] }, + { + kind: 'message', + role: 'assistant', + blocks: [{ type: 'image-ref', path: '/tmp/shot.png', alt: 'screenshot' }] + }, + { kind: 'message', role: 'user', blocks: [{ type: 'text', text: 'Again' }] }, + { kind: 'message', role: 'assistant', blocks: [{ type: 'text', text: 'Answered' }] } + ]) + const anchors = structuredForkTurnAnchors(imageOnlyTurn) + expect(anchors.get('codex:parent:a:1')).toBeUndefined() + expect(structuredForkEligibleItems(anchors)).toEqual(new Set(['codex:parent:a:3'])) + }) + + it('still refuses an all-tool-block assistant message, which only a legacy import can produce', () => { + const imported = journal([ + { kind: 'message', role: 'user', blocks: [{ type: 'text', text: 'Ask' }] }, + { kind: 'message', role: 'assistant', blocks: [{ type: 'text', text: 'Answered' }] }, + { + kind: 'message', + role: 'assistant', + blocks: [{ type: 'tool-call', name: 'read', input: {} }] + } + ]) + expect(structuredForkEligibleItems(structuredForkTurnAnchors(imported))).toEqual( + new Set(['codex:parent:a:1']) + ) }) it('keeps the whole turn out while it is still running', () => { - const live: AgentJournalRenderItem[] = [ - ...multiRowTurn(), - { - itemId: 'legacy:codex:parent:turn-lifecycle%3Aa', - revision: 1, - body: { kind: 'status', text: 'Working', turnLifecycle: { turnId: 'a', state: 'running' } }, - sequence: 4, - observedAt: 1 - } - ] - expect(structuredForkTurnAnchors(live).size).toBe(0) + expect(structuredForkTurnAnchors(withRunningTurn(multiRowTurn())).size).toBe(0) + }) + + it('keeps offering SETTLED turns while a LATER turn runs, exactly as the host does', () => { + // The host refuses only the live turn as `busy` and serves every settled one, so nothing may + // withhold a settled turn's action merely because the session is working. + const live = withRunningTurn( + [ + ...multiRowTurn(), + { + itemId: 'codex:parent:b:0', + revision: 1, + body: { + kind: 'message' as const, + role: 'user' as const, + blocks: [{ type: 'text' as const, text: 'Next' }] + }, + sequence: 10, + observedAt: 1 + } + ], + 'b' + ) + expect(structuredForkEligibleItems(structuredForkTurnAnchors(live))).toEqual( + new Set(['codex:parent:a:3']) + ) }) }) diff --git a/src/shared/agent-session-prefix.ts b/src/shared/agent-session-prefix.ts index b5f4881e982..2539c55b13a 100644 --- a/src/shared/agent-session-prefix.ts +++ b/src/shared/agent-session-prefix.ts @@ -10,8 +10,11 @@ import type { import type { AgentSessionProviderHandle } from './agent-session-provider-handle' import type { AgentSessionRewindReason, AgentSessionRewindRecord } from './agent-session-rewind' import { agentSessionPrefixWithinBounds } from './agent-session-prefix-bounds' -import { activeStructuredAgentSessionTurnId } from './structured-agent-session-projection' -import { isToolOnlyBlockSet } from './native-chat-tool-fold' +import { + activeStructuredAgentSessionTurnId, + projectStructuredItemToNativeChat +} from './structured-agent-session-projection' +import { nativeChatMessageDrawsAgentControls } from './native-chat-row-content' type PrefixSelection = | { ok: false; reason: AgentSessionRewindReason } @@ -59,15 +62,8 @@ export function selectAgentSessionPrefix( if (key.threadId !== head.threadId) { return { ok: false, reason: 'invalid-target' } } - boundary = snapshot.items.findIndex((item) => { - const identity = parseAgentJournalItemKey(providerKey(item.itemId)) - return ( - (identity?.provider === 'codex' && - identity.threadId === key.threadId && - identity.turnId === key.turnId) || - (item.body.kind === 'status' && item.body.turnLifecycle?.turnId === key.turnId) - ) - }) + // Only a `before` boundary keeps this: `through` overwrites it below, and the scan is O(n). + boundary = input.boundary === 'before' ? turnStart(snapshot.items, key, providerKey) : boundary } else if (key.provider === 'claude' && head.provider === 'claude') { if (key.sessionId !== head.sessionId) { return { ok: false, reason: 'invalid-target' } @@ -131,6 +127,23 @@ export function selectAgentSessionPrefix( } } +/** First row of the Codex turn `key` names, including the lifecycle row the turn opened with. */ +function turnStart( + items: readonly AgentJournalRenderItem[], + key: AgentJournalItemIdentity & { provider: 'codex' }, + providerKey: (id: string) => string +): number { + return items.findIndex((item) => { + const identity = parseAgentJournalItemKey(providerKey(item.itemId)) + return ( + (identity?.provider === 'codex' && + identity.threadId === key.threadId && + identity.turnId === key.turnId) || + (item.body.kind === 'status' && item.body.turnLifecycle?.turnId === key.turnId) + ) + }) +} + /** End of the turn containing `selected`, or null while that turn is still live. * * Settlement TOMBSTONES a turn's lifecycle row rather than rewriting it to `completed`, so a @@ -172,8 +185,10 @@ function liveTurn( * is named — so per-row targets let two clicks on one turn mint two identical children. Mapping * the siblings onto a shared anchor makes that impossible rather than merely unlikely. * - * The anchor is the turn's last assistant row that the transcript still DRAWS: a trailing - * tool-only row is folded into the row above it and renders nothing, so it can carry no control. */ + * The anchor is the turn's last row the transcript DRAWS A CONTROL CLUSTER ON, decided by the + * renderer's own predicate rather than an approximation of it. A row folded into the one above, + * or one carrying only images, draws no cluster and so can carry no fork action; a turn with no + * such row gets NO anchor, because a phantom one costs that turn its only affordance in silence. */ export function structuredForkTurnAnchors( items: readonly AgentJournalRenderItem[] ): Map { @@ -184,7 +199,7 @@ export function structuredForkTurnAnchors( const anchors = new Map() let turn: { itemId: string; index: number; drawn: boolean }[] = [] const settle = (): void => { - const anchor = turn.findLast((row) => row.drawn) ?? turn.at(-1) + const anchor = turn.findLast((row) => row.drawn) // Parsing every key is wasted work on the idle journal a fork is actually taken from. if ( anchor && @@ -203,8 +218,12 @@ export function structuredForkTurnAnchors( } if (item.body.role === 'user') { settle() - } else if (item.body.role === 'assistant') { - turn.push({ itemId: item.itemId, index, drawn: !isToolOnlyBlockSet(item.body.blocks) }) + } else { + turn.push({ + itemId: item.itemId, + index, + drawn: nativeChatMessageDrawsAgentControls(projectStructuredItemToNativeChat(item)) + }) } }) settle() @@ -212,6 +231,6 @@ export function structuredForkTurnAnchors( } /** The rows that show a fork action: one per settled turn, derived from the anchors. */ -export function structuredForkEligibleItems(items: readonly AgentJournalRenderItem[]): Set { - return new Set(structuredForkTurnAnchors(items).values()) +export function structuredForkEligibleItems(anchors: ReadonlyMap): Set { + return new Set(anchors.values()) } diff --git a/src/shared/native-chat-row-content.ts b/src/shared/native-chat-row-content.ts new file mode 100644 index 00000000000..7cb3c669048 --- /dev/null +++ b/src/shared/native-chat-row-content.ts @@ -0,0 +1,60 @@ +import { + isSubagentGroupBlock, + isTextBlock, + type NativeChatBlock, + type NativeChatMessage, + type NativeChatSubagentGroupBlock +} from './native-chat-types' +import { isSubagentGroupFallbackText, subagentGroupBlocks } from './native-chat-subagent-summary' +import { splitNativeChatBlocks } from './native-chat-tool-fold' + +export function nativeChatProseToMarkdown(blocks: readonly NativeChatBlock[]): string { + return blocks + .map((block) => (isTextBlock(block) ? block.text : '')) + .filter((part) => part.length > 0) + .join('\n\n') +} + +/** What a transcript row actually draws, split the way the row draws it. + * + * A spawn-group row carries a plain-text twin so a client without the block type still reads the + * roster; a client that draws the block drops the twin rather than printing both. */ +export function nativeChatRowContent(blocks: readonly NativeChatBlock[]): { + prose: NativeChatBlock[] + tools: NativeChatBlock[] + subagentGroups: NativeChatSubagentGroupBlock[] +} { + const split = splitNativeChatBlocks(blocks) + const groups = subagentGroupBlocks(split.prose) + return { + tools: split.tools, + subagentGroups: groups, + prose: + groups.length === 0 + ? split.prose + : split.prose.filter( + (block) => + !isSubagentGroupBlock(block) && + !(block.type === 'text' && isSubagentGroupFallbackText(block.text)) + ) + } +} + +/** + * Whether the transcript draws its hover control cluster on this row. + * + * The fork action lives in that cluster, so this is also the only place a fork control can appear: + * anything anchored elsewhere silently renders nothing. Kept beside the row's own derivation rather + * than approximated, because an approximation is exactly how a turn loses its only affordance — + * a row whose blocks are all tool activity, all images, or a provider frame draws no prose and so + * draws no controls. + */ +export function nativeChatMessageDrawsAgentControls(message: NativeChatMessage | null): boolean { + if (message?.role !== 'assistant') { + return false + } + if (message.blocks.some((block) => block.type === 'text' && block.providerFrame)) { + return false + } + return nativeChatProseToMarkdown(nativeChatRowContent(message.blocks).prose).length > 0 +}