The CLI round-trips a client-supplied uuid verbatim. Measured:
stdin : {"type":"user","uuid":"11111111-2222-4333-8444-555555555555",...}
stdout: replay uuid = 11111111-2222-4333-8444-555555555555
So stamp one on the outgoing frame and match the echo on it. A replay now
settles the dispatch it belongs to, whatever its queue position - which closes
the limit this PR had been documenting as a redesign:
- a late replay no longer settles the NEXT dispatch after its own waiter timed
out, handing it the previous send's identity;
- the send-failure path now checks `waiter.settledUuid` - positive evidence that
THIS waiter was settled - instead of inferring adoption from queue membership,
which could return `accepted` carrying another dispatch's uuid.
The CLI also dedupes on the input uuid, so a resent frame cannot land twice.
The shape gates stay as the compat path for a CLI that mints its own uuid rather
than echoing ours; there the frame carries no correlation and the head is the
only candidate. Every pre-existing test exercises that path, since their frames
carry uuids that match no waiter.
`isReplay` is necessary but NOT sufficient, and the binary says so. Verified in
claude 2.1.237: the model-switch breadcrumb is enqueued as
`{type:'user', parent_tool_use_id:null, isReplay:true}` whose content is the
string `<local-command-stdout>Set model to ...`. Orca triggers that path itself
from the model picker (setClaudeStructuredOption sends set_model, and reconnect
replays it), so a send in flight during a model switch adopted the breadcrumb's
uuid and the real echo appended a second row.
The CLI cannot use the marker alone either - its own RemoteSessionManager
filters the same frames by content prefix:
isReplay"in e&&e.isReplay===!0&&typeof e.message?.content==="string"
&&e.message.content.startsWith(cvl) // "<local-command-stdout>Set model to "
-> "Dropped own set_model breadcrumb echo"
So: require the marker, then reject the harness shapes with the existing
isKnownHarnessInjectedUserTurnText. This is the classifier an earlier round
deleted - but as a NARROWING on top of the marker, not a fallback instead of
it, and now on primary evidence rather than speculation.
Also stop discarding an identity the replay already claimed: a write can reach
Claude and still reject on the following flush, and reporting `unknown` there
stranded a uuid nothing else could adopt.
Found by the review loop on #15657.
Measured against the real binary rather than reasoned about:
- ` /clear` comes back as a stamped replay, so this lane reads an indented
token as prose too. The no-trim rule was inherited from a comment about the
TUIs; it is now verified for `claude -p --input-format stream-json`.
- `/clear` and `/model` answer with a bare `result` and NO echo, which is the
only case acceptsResult exists for.
- A CUSTOM project command (`/probeit`) and a path-leading message
(`/tmp what is in here?`) both DO produce a stamped replay.
So shape cannot separate `/tmp` from `/clear`, and the command catalog cannot
either: getVerifiedNativeChatCommands('claude') is a curated `/` menu (clear,
compact, init, review, help) that omits `/model` and `/permissions`. Gating on
it strands those sends - tried, and the pre-existing `/permissions` test caught
it immediately. Recording the residual and why the two obvious fixes are wrong,
so the next reader does not retry them.
Found by the review loop on #15657.
Two holes left by the previous round's "no second slash" rule:
`/README.md - what does this do?` has no second slash, so it still opted into
the result-frame path where an unrelated turn's result can claim the send's
identity. A command token has no dot.
And trimming leading whitespace contradicted `classifyNativeChatSend`, which
deliberately does not trim: "the supported TUIs only treat a line-leading token
as a command, so a draft with leading whitespace is prose". ` /clear` was prose
to the composer and a command to dispatch.
Match a command token instead: `/model`, `/clear`, `/project:foo`. Deliberately
stricter than the shared classifier, and the comment says why - over-classifying
only suppresses an echo there, but claims a wrong identity here.
Found by the review loop on #15657.
`acceptsResult` lets a send settle on a bare `result` frame, which carries no
correlation to the waiting dispatch. It was set for any text starting with '/',
so `/Users/me/repo/src/foo.ts - explain this` opted in: an unrelated in-flight
turn's result claimed the send's identity, nothing upserted into the adopted key
(readClaudeMessageEnvelope returns null for `type: 'result'`), and the real echo
appended a second row - this PR's own symptom through the one path it had not
touched.
Pre-existing, but narrowing the user-frame gate made the `result` race the only
quick settle for these sends rather than one of two, so it ships here.
A slash command's first token has no second slash; that is exactly what
separates `/model sonnet` and `/project:foo` from a path.
Also corrects the image-replay test's comment: it is NOT red against the merge
base (the old predicate accepts that frame too). It pins that the gate never
sniffs content, which is a different guarantee.
Found by the review loop on #15657.
Measured on claude 2.1.237: interrupting a turn while a send is queued emits a
top-level user frame carrying `[Request interrupted by user for tool use]` with
no `isReplay` AND no `tool_use_result` - so it cleared every guard and made a
perfectly healthy CLI log "sends will report delivery unconfirmed until the CLI
stamps the marker". A false diagnosis.
Worse, the one-shot latch is process-wide, so that one healthy interrupt
permanently consumed the signal and a genuinely unmarked CLI could then never
report itself. The warning was less than useless.
Diagnosing this properly means warning only when a waiter actually times out
having seen unmarked frames - more state in the function whose extra state has
been the source of every defect in this review loop. The flag pin from the same
round is the part that carries real weight; a real-binary assertion is the
right complement and is recorded as follow-up.
Found by the review loop on #15657.
`resolveClaudeReplayWaiter` now settles a send only on the `isReplay` marker,
so `--replay-user-messages` is load-bearing - but no test asserted it was in
CLAUDE_STRUCTURED_BASE_ARGS, and both fake connections synthesize the marker
themselves, so dropping the flag would have left the suite fully green while
every real send blocked its whole ack budget and landed "delivery unconfirmed".
Pin the flag, and warn once when a top-level user frame arrives without the
marker while a send is waiting. A CLI that echoes unmarked is otherwise
indistinguishable at runtime from a turn with no tool boundary inside the
budget: both just report delivery unconfirmed.
Found by the review loop on #15657.
`src/main/runtime/claude-structured-session-integration.test.ts` has its own
fake connection echoing `{ ...message, uuid: 'user-1' }` without `isReplay`, so
the send waited out its whole ack budget and settled `unknown`. CI red; missed
locally because the previous run covered only src/main/claude and
src/main/native-chat.
Second instance of the hazard the sibling comment already names: a double that
does not stamp like the real binary. Swept the tree - these two fakes are the
only ones that echo a user frame.
Found by the review loop on #15657.
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.
`dispatchClaudeTurn`'s error path shifted `dispatchWaiters[0]`, which is only
this call's waiter when nothing else is in flight. With two sends queued, a
broken pipe on the SECOND resolved the FIRST with null - reporting a delivered
message as "delivery unconfirmed" while the send that actually failed stayed
queued for its full ack budget, and the first send's replay then settled a
waiter nobody awaited.
Same mis-adoption class this PR is hardening: settle a dispatch only on
evidence that belongs to it. `waitForReplay` now hands its waiter back so the
error path can splice that one, mirroring what the timeout path already did.
Also raises the tests' ack budget to 5s. vi.waitFor's first successful poll
lands ~50ms after the waiter is armed, so a 100ms budget left almost nothing
for the frame and could expire under parallel load.
Found by the review loop on #15657.
The frame carried `isReplay: true`, so the gate short-circuited and
`claudeUserFrameText` never ran - the content branch the test exists for was
uncovered. Verified by mutation: making claudeUserFrameText return null when no
part carries text (exactly the regression the comment warns about) left the
test green.
Drop the marker so the frame takes the fallback, and give this one case a 5s ack
budget: it does real file I/O before the timer starts, so 100ms could expire
under parallel load before the frame arrives.
The same mutation now fails the test.
Found by the review loop on #15657.
The shape fallback ran on every frame without `isReplay`, so an injected turn
whose leading text is not on the hand-maintained allowlist in
harness-injected-user-turns.ts would still steal the send identity - and an
injected turn upserts a body, so it overwrites the user's own bubble before the
real replay appends a second one.
The launch args always request replays, so once this session has seen the CLI
stamp `isReplay`, a frame without it cannot be the echo. Latch that and switch
the fallback off; it now only ever runs on a CLI that never stamps the marker,
where the alternative is every send timing out into a false
"delivery unconfirmed" and being sent twice on retry.
Also corrects the string-content comment: `readClaudeMessageEnvelope` tolerates
a plain-string `content` but builds an EMPTY envelope from it, so such an echo
settles the dispatch without upserting a body - the same as an image-only echo,
not the reconciliation the old wording implied.
Found by the review loop on #15657.
"Not a tool result" was too weak. Claude publishes harness-injected turns -
interruption notices, system reminders, local-command output - in the same
top-level `user` shape with a null parent_tool_use_id, and those are the WORSE
half of the class: unlike a tool result they do build a journal body, so
adopting one upserts harness text over the user's own bubble and still leaves
the real replay to append as a second row.
`--replay-user-messages` stamps `isReplay` on the genuine echo, so trust that
when present. Keep a shape test as the fallback for a CLI that omits the field,
now excluding harness turns via the repo's existing
`isKnownHarnessInjectedUserTurnText` rather than a second enumeration.
Also accept string `content`. `readClaudeMessageEnvelope` already tolerates it,
so rejecting it was a new way for a send to go unacknowledged - which times out
into a false "delivery unconfirmed" and sends the message twice on retry.
Found by the review loop on #15657.
Requiring `claudeMessageBody(...) !== null` inherited that function's content
model: it builds blocks only for text and URL images, while dispatch
base64-encodes local image paths. An image-only send was therefore refused its
OWN replay, ran out the 10s ack timeout, and reported a delivered message as
`unknown` - which offers retry, and retry sends it to Claude a second time. A
duplicate render traded for a duplicate send.
Reject the tool-result shape directly instead. That still keeps a tool-result
turn from stealing the identity, without making confirmation depend on which
content parts the translator happens to model.
Measured against the real CLI while checking the timeout concern: a queued
send's replay is published at the FIRST tool boundary after it is queued, 2-3ms
after that boundary's tool result (1004ms queued -> 4772ms tool_result ->
4774ms replay, across a 22s three-tool turn). So rejecting tool results costs no
time - whenever there was a frame to adopt, the right one follows immediately -
and a turn with no boundary inside the timeout times out identically either way.
Found by the review loop on #15657.
Claude publishes a top-level `user` turn for tool results as well as for the
`--replay-user-messages` echo of the prompt Orca dispatched. Both carry
`type: "user"` and a null `parent_tool_use_id`, so resolveClaudeReplayWaiter
adopted whichever arrived first.
A send issued while a turn is running is dispatched with tools in flight, so
the tool-result turn takes the waiter. Dispatch then records that uuid as the
provider identity, the journal aliases the submission onto an item nothing
ever upserts, and the real replay lands as its own user item - the user's
message renders twice.
Gate the waiter on the same body claudeMessageBody builds, so the identity
dispatch adopts is always the one the echo upserts into. An unrecognized
frame leaves the send `unknown` ("delivery unconfirmed"), never duplicated.
Both frames in the regression test are captured verbatim from
`claude -p --input-format stream-json --output-format stream-json
--replay-user-messages`; the previous double invented a `user` frame with no
message body at all, which the CLI never emits.
Brennan Benson