From 26e1ccd3bc5c98981201be4ed5e88b88eed3d9bb Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 20 Aug 2026 18:12:54 -0700 Subject: [PATCH] refactor(native-chat): require Claude's isReplay marker, drop the shape fallback The fallback was defensive code that kept finding new ways to be wrong: it consulted a list `src/shared/harness-injected-user-turns.ts` maintains for prompt-derived UI (so a UI edit silently changed which frames could claim a send's identity), it rejected a genuine echo whose own text opened with a harness shape, its string-content branch returned before the tool-result rejection, and the latch that switched it off lived on the session object - so it reset on every reconnect and re-opened the original bug once per connection. Measured against the real CLI instead: `--replay-user-messages` is unconditional in CLAUDE_STRUCTURED_BASE_ARGS, and every echo it produces is stamped - verified for text, image-only and text+image sends. A slash command may get no user echo at all, which `acceptsResult` already settles on the command's result receipt. So the marker is the whole test. No content sniffing, no shared list, no latch, no per-reconnect window: -98 lines. The fake CLI in claude-structured-session-adapter.test.ts echoed sends WITHOUT `isReplay`, unlike the real binary. That double is what made a content-sniffing gate look necessary; it now stamps like the real thing. Found by the review loop on #15657. --- .../claude/claude-structured-dispatch.test.ts | 76 ++----------------- src/main/claude/claude-structured-dispatch.ts | 63 ++++----------- .../claude-structured-session-adapter.test.ts | 6 +- .../claude/claude-structured-session-state.ts | 3 - 4 files changed, 25 insertions(+), 123 deletions(-) diff --git a/src/main/claude/claude-structured-dispatch.test.ts b/src/main/claude/claude-structured-dispatch.test.ts index cf47b43ce44..0fa1429258b 100644 --- a/src/main/claude/claude-structured-dispatch.test.ts +++ b/src/main/claude/claude-structured-dispatch.test.ts @@ -161,73 +161,10 @@ describe('Claude structured dispatch image limits', () => { }) }) - // Once this CLI is seen stamping the marker, the injected-turn allowlist must - // stop being load-bearing: an unrecognized injected shape would otherwise still - // steal the identity. - it('stops trusting frame shape once the CLI is seen stamping isReplay', async () => { - const session = sessionFor() - const first = dispatchClaudeTurn( - session, - { clientMessageId: 'client-1', body: userMessage([{ type: 'text', text: 'one' }]) }, - ACK_BUDGET_MS - ) - await vi.waitFor(() => expect(session.dispatchWaiters).toHaveLength(1)) - resolveClaudeReplayWaiter(session, userReplayFrame('replay-one', 'one')) - await expect(first).resolves.toMatchObject({ state: 'accepted' }) - - const second = dispatchClaudeTurn( - session, - { clientMessageId: 'client-2', body: userMessage([{ type: 'text', text: 'two' }]) }, - ACK_BUDGET_MS - ) - await vi.waitFor(() => expect(session.dispatchWaiters).toHaveLength(1)) - - // An injected shape nobody has enumerated yet, so the allowlist would pass it. - resolveClaudeReplayWaiter(session, { - type: 'user', - message: { role: 'user', content: [{ type: 'text', text: 'hi' }] }, - parent_tool_use_id: null, - session_id: 'provider-session', - uuid: 'unknown-injected-uuid' - }) - expect(session.dispatchWaiters).toHaveLength(1) - - resolveClaudeReplayWaiter(session, userReplayFrame('replay-two', 'two')) - await expect(second).resolves.toMatchObject({ - state: 'accepted', - providerIdentity: { uuid: 'replay-two' } - }) - }) - - // `readClaudeMessageEnvelope` already tolerates string content, so refusing it - // here would be a new way for a send to never be acknowledged. - it('accepts a replay whose content is a plain string', async () => { - const session = sessionFor() - const dispatched = dispatchClaudeTurn( - session, - { clientMessageId: 'client-1', body: userMessage([{ type: 'text', text: 'hello' }]) }, - ACK_BUDGET_MS - ) - await vi.waitFor(() => expect(session.dispatchWaiters).toHaveLength(1)) - - resolveClaudeReplayWaiter(session, { - type: 'user', - message: { role: 'user', content: 'hello' }, - parent_tool_use_id: null, - session_id: 'provider-session', - uuid: 'string-replay-uuid' - }) - - await expect(dispatched).resolves.toMatchObject({ - state: 'accepted', - providerIdentity: { uuid: 'string-replay-uuid' } - }) - }) - - // Dispatch base64-encodes a local image path, and `claudeMessageBody` models - // only text and URL images - so gating the waiter on "the translator would - // build a body" refuses an image-only send its own replay, times out, and - // reports a delivered message as unconfirmed (retry then sends it twice). + // Measured: the CLI stamps an image-only echo like any other, and its content + // carries no text. A gate that sniffed content for a modeled body would refuse + // this send its own replay, time out, and report a delivered message as + // unconfirmed - which sends it twice on retry. it('accepts the replay of a send that carries only a local image', async () => { const directory = await mkdtemp(join(tmpdir(), 'orca-claude-replay-')) try { @@ -241,8 +178,6 @@ describe('Claude structured dispatch image limits', () => { ) await vi.waitFor(() => expect(session.dispatchWaiters).toHaveLength(1)) - // Deliberately UNSTAMPED: `isReplay` would short-circuit the gate and leave - // the content branch this test exists for untested. resolveClaudeReplayWaiter(session, { type: 'user', message: { @@ -253,7 +188,8 @@ describe('Claude structured dispatch image limits', () => { }, parent_tool_use_id: null, session_id: 'provider-session', - uuid: 'image-replay-uuid' + uuid: 'image-replay-uuid', + isReplay: true }) await expect(dispatched).resolves.toMatchObject({ diff --git a/src/main/claude/claude-structured-dispatch.ts b/src/main/claude/claude-structured-dispatch.ts index daab1be3279..80585df1323 100644 --- a/src/main/claude/claude-structured-dispatch.ts +++ b/src/main/claude/claude-structured-dispatch.ts @@ -5,8 +5,6 @@ import type { NativeChatBlock } from '../../shared/native-chat-types' import type { AgentSessionDispatchOutcome } from '../native-chat/agent-session-wire/structured-agent-session-adapter' import type { ClaudeDispatchWaiter, ClaudeSession } from './claude-structured-session-state' import { readClaudeFrameString } from './claude-structured-init-proof' -import { isKnownHarnessInjectedUserTurnText } from '../../shared/harness-injected-user-turns' -import { claudeRecord } from './claude-structured-item-translation' const MAX_IMAGE_BYTES = 5 * 1024 * 1024 const MAX_IMAGE_COUNT = 20 @@ -52,72 +50,39 @@ const IMAGE_MIME_BY_EXTENSION: Record = { '.webp': 'image/webp' } -/** Text of a top-level user frame, or null when it is a tool-result turn (or - * carries nothing). A plain-string `content` is accepted so a CLI that sends the - * simpler shape can still settle its send - note the translator models only the - * array form, so such an echo settles the dispatch without upserting a body, the - * same way an image-only echo does. An image-only echo has no text, yielding ''. */ -function claudeUserFrameText(content: unknown): string | null { - if (typeof content === 'string') { - return content - } - if (!Array.isArray(content) || content.length === 0) { - return null - } - const parts = content.map((part) => claudeRecord(part)) - if (parts.some((part) => part?.type === 'tool_result')) { - return null - } - return parts.map((part) => (typeof part?.text === 'string' ? part.text : '')).join('\n') -} - /** * The frame that carries the user's own message back. * * `type: 'user'` with a null `parent_tool_use_id` is not enough on its own: * Claude publishes tool results and harness-injected turns (interruption * notices, system reminders, local-command output) in the same shape. Adopting - * one of those leaves the real replay to land under an identity nothing claims, - * so the user's message renders twice - and an injected turn is worse than a - * tool result, because it DOES build a body and upserts over the user's own - * bubble. + * one leaves the real replay to land under an identity nothing claims, so the + * user's message renders twice - and an injected turn is worse than a tool + * result, because it DOES build a body and upserts over the user's own bubble. * - * `--replay-user-messages` stamps `isReplay` on the genuine echo, so trust that - * when it is there. The shape test is only a fallback for a CLI that omits the - * marker, and it stays off once this session has seen one: the launch args always - * request replays, so after that a frame without the marker is by construction not - * the echo, and the hand-maintained injected-turn list must not be load-bearing. - * Without any fallback, a CLI that never stamps it would leave every send timing - * out into a false "delivery unconfirmed", and retrying sends the message twice. + * `CLAUDE_STRUCTURED_BASE_ARGS` always passes `--replay-user-messages`, and the + * CLI stamps `isReplay` on every echo it publishes in reply - measured for text, + * image-only and text+image sends. So the marker is the whole test: no content + * sniffing, and nothing here depends on the injected-turn list that + * prompt-derived UI maintains for its own reasons. + * + * A slash command may get no user echo at all; `acceptsResult` settles those on + * the command's result receipt instead. * * Measured against the real CLI: a queued send's replay is published at the * first tool boundary after it is queued, 2-3 ms AFTER that boundary's tool * result. So rejecting tool results costs no time - whenever there was a frame * to adopt, the right one follows immediately. */ -function isClaudeUserMessageReplay( - session: ClaudeSession, - message: Record -): boolean { - if (message.type !== 'user' || message.parent_tool_use_id !== null) { - return false - } - if (message.isReplay === true) { - session.sawUserReplayMarker = true - return true - } - if (session.sawUserReplayMarker) { - return false - } - const text = claudeUserFrameText(claudeRecord(message.message)?.content) - return text !== null && !isKnownHarnessInjectedUserTurnText(text) +function isClaudeUserMessageReplay(message: Record): boolean { + return message.type === 'user' && message.parent_tool_use_id === null && message.isReplay === true } export function resolveClaudeReplayWaiter( session: ClaudeSession, message: Record ): void { - const isUserReplay = isClaudeUserMessageReplay(session, message) + const isUserReplay = isClaudeUserMessageReplay(message) const isCompletedCommand = message.type === 'result' if ( (!isUserReplay && !isCompletedCommand) || diff --git a/src/main/claude/claude-structured-session-adapter.test.ts b/src/main/claude/claude-structured-session-adapter.test.ts index 4bd3fc6e652..1b7126353da 100644 --- a/src/main/claude/claude-structured-session-adapter.test.ts +++ b/src/main/claude/claude-structured-session-adapter.test.ts @@ -120,9 +120,13 @@ function fakeClaude( send: async (message) => { connection.sent.push(message) if (message.type === 'user' && options.replayUuid !== null) { + // The real CLI stamps `isReplay` on every echo `--replay-user-messages` + // produces; a double that omits it invites a gate that cannot tell an + // echo from an injected turn. handlers.onMessage?.({ ...message, - uuid: options.replayUuid ?? 'user-uuid' + uuid: options.replayUuid ?? 'user-uuid', + isReplay: true }) } }, diff --git a/src/main/claude/claude-structured-session-state.ts b/src/main/claude/claude-structured-session-state.ts index f32b2c240ca..9a8462f6ac2 100644 --- a/src/main/claude/claude-structured-session-state.ts +++ b/src/main/claude/claude-structured-session-state.ts @@ -65,9 +65,6 @@ export type ClaudeSession = { fence: number prompts: ClaudePromptRegistry dispatchWaiters: ClaudeDispatchWaiter[] - /** Set once this CLI is seen stamping `isReplay` on its user echo. From then on - * a frame without it cannot be an echo, so the shape fallback is switched off. */ - sawUserReplayMarker?: boolean options: Map reportedOptions: { model?: string; effort?: string } translator: ClaudeJournalTranslator | null