mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 00:03:52 +00:00
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.
This commit is contained in:
@@ -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: '<some-new-notice>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({
|
||||
|
||||
@@ -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<string, string> = {
|
||||
'.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<string, unknown>
|
||||
): 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<string, unknown>): boolean {
|
||||
return message.type === 'user' && message.parent_tool_use_id === null && message.isReplay === true
|
||||
}
|
||||
|
||||
export function resolveClaudeReplayWaiter(
|
||||
session: ClaudeSession,
|
||||
message: Record<string, unknown>
|
||||
): void {
|
||||
const isUserReplay = isClaudeUserMessageReplay(session, message)
|
||||
const isUserReplay = isClaudeUserMessageReplay(message)
|
||||
const isCompletedCommand = message.type === 'result'
|
||||
if (
|
||||
(!isUserReplay && !isCompletedCommand) ||
|
||||
|
||||
@@ -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
|
||||
})
|
||||
}
|
||||
},
|
||||
|
||||
@@ -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<string, string>
|
||||
reportedOptions: { model?: string; effort?: string }
|
||||
translator: ClaudeJournalTranslator | null
|
||||
|
||||
Reference in New Issue
Block a user