Repository navigation
feat(native-chat): decide a restart-stranded send against provider history - #20139
Conversation
…story `markPendingSubmissionsUnknown` flips every surviving `pending` submission to `unknown` on attach and stops there. The module written to finish the job describes the intended two-step in its own header -- "Every surviving `pending` becomes `unknown` and is then matched against provider history" -- and only the first step ever shipped. `reconcileSubmissions` has been imported by exactly one test file and nothing else. So a message stranded by a dead child or a host restart had no recourse but retyping: Retry correctly refuses to redeliver something that may already be with the model, the outbox entry drops, and a transient error line is all that remains. This wires the second step, so those are decided on evidence instead of refused. Caller placement is the design decision, because where it runs determines what a consistent history boundary can mean. It runs in `attachJournal`, immediately after the sweep: attach happens after the record store's CAS hands this host the lease and before a provider child starts, so nothing can append to provider history while it is read, and the window stays valid until the resume consumes it. The three other settlement sites can all be overtaken by a newly started child before the read is acted on. The history source is the Claude project JSONL for the handle chain's provider session id -- definitionally what a resume replays, which is what makes absence meaningful. Boundary consistency reuses `proveClaudeTranscriptBranchFromJsonl` rather than inventing a check: a fork, a compacted log and a truncated tail each already throw there, and each maps onto `boundaryConsistent: false`. A null leaf uuid is also false, because there is no anchor to prove a start from. Two guards were needed that the reconciler cannot enforce itself, because Claude echoes no client message id and only the fingerprint pass can fire: - A transcript records a pasted image as base64, and the block decoder drops it silently for want of a url or path. Such a record would enter the window advertising a text-only fingerprint, where an unrelated text-only submission with identical text could claim it. The window now inspects raw content parts before decoding and excludes any record a part would be dropped from. - A submission carrying an image-ref path can never match a transcript that keeps only base64. Without a guard it matches nothing by construction rather than by absence and falls straight through to `not_delivered`, and a Retry would then redeliver an image already sent. Only text-only bodies are handed to the reconciler. Both guards fail a named test when removed. Limits, stated rather than implied. The exact-match tier needs the provider to echo our id, which Codex does and Claude does not, so Claude resolves by fingerprint alone -- and two identical prompts deliberately reach `ambiguous_match` instead of guessing. Repeated one-word prompts therefore stay unknown by construction. This decides what it can prove and refuses the rest, which is the intended contract, not a shortfall in the wiring. Found while doing this and not fixed here: the block decoder silently dropping base64 images has a blast radius beyond reconciliation and deserves its own change.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughAdds bounded Claude transcript history windows anchored to the durable leaf. Preserves raw prompt text for matching. Adds reconciliation for unresolved text-only submissions and ambiguous duplicate fingerprints. Exposes provider history through structured session adapters. Samples history during attach before acquiring a replacement child and preserves unknown submissions when history is unavailable or inconsistent. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No concrete merge-blocking risk remains from the reviewed change. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 89cb1dd1-324b-4080-85c9-f0e15f34a8eb
📒 Files selected for processing (9)
src/main/claude/claude-structured-history-window.test.tssrc/main/claude/claude-structured-history-window.tssrc/main/claude/claude-structured-session-adapter.tssrc/main/native-chat/agent-session-journal/journal-restart-reconciliation.test.tssrc/main/native-chat/agent-session-journal/journal-restart-reconciliation.tssrc/main/native-chat/agent-session-wire/structured-agent-session-adapter-router.tssrc/main/native-chat/agent-session-wire/structured-agent-session-adapter.tssrc/main/native-chat/agent-session-wire/structured-agent-session-attach-reconciliation.test.tssrc/main/native-chat/agent-session-wire/structured-agent-session-attach.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The rejected: not_delivered half of this reconciliation never fires in the cold-restart path it was built for — the provider child is spawned before attachJournal reads history, so the window always reports turnInFlight: true and only accepted resolution actually takes effect there.
Reviewed changes
- Claude provider-history window (
claude-structured-history-window.ts) — reads the Claude project JSONL, proves branch consistency viaproveClaudeTranscriptBranchFromJsonl, walks leaf→anchor, and fingerprints each surviving text-only user prompt. - Reconciliation caller (
journal-restart-reconciliation.ts) — feedsunknownsubmissions through the pre-existingreconcileSubmissions, restricted to text-only bodies and to history items the journal has not already committed. - Adapter + router surface — adds
providerHistoryWindow?toStructuredAgentSessionAdapter, implements it on the Claude adapter using itssessionsmap for liveness, and routes it generically. - Attach integration (
structured-agent-session-attach.ts) — runs the reconcile right aftermarkPendingSubmissionsUnknownand drops settled ids fromunconfirmedClientMessageIds.
⚠️ rejected: not_delivered is unreachable in the restart path
The window's turnInFlight flag is the only thing standing between "absent from history" and not_delivered, and it is wired to the Claude adapter's live-session map — which is populated by acquireOwner before attachJournal runs. The consequence: in the host-restart scenario this PR exists for, every stranded submission resolves to accepted (when it matches) or stays unknown (turn_in_flight), and the "provably absent → reject → re-sendable via Retry" case never fires. The message the PR description calls out ("a message stranded by a dead child … had no recourse but retyping") is only fixed for the delivered half; an undelivered stranded message still cannot be retried.
The attachJournal doc comment states it runs "before onAttached starts a provider child", but the child is actually spawned inside acquireOwner → adapter.acquire → acquireClaudeSession, which completes first.
Technical details
# `turnInFlight` is always true at attach time, so `not_delivered` is unreachable
## Affected sites
- `src/main/native-chat/agent-session-wire/structured-agent-session-attach-flow.ts:143` — `acquireOwner` runs before `attachJournal` (line 214) in `performAttach`.
- `src/main/native-chat/agent-session-wire/structured-agent-session-acquisition.ts:39` — `acquireOwner` calls `adapter.acquire`, which spawns the child.
- `src/main/claude/claude-structured-session-acquisition.ts:291` — `sessions.set(sessionId, liveSession)` before `acquire` resolves.
- `src/main/claude/claude-structured-session-adapter.ts:174` — `hasLiveSession: this.sessions.has(...)`.
- `src/main/claude/claude-structured-history-window.ts:232` — `turnInFlight: hasLiveSession`.
- `src/main/native-chat/agent-session-journal/journal-submission-reconciler.ts:177` — `if (history.turnInFlight) return unknown`, so `not_delivered` requires `turnInFlight === false`.
## Required outcome
- `turnInFlight` must mean "a turn is actually in flight", not "a child exists". At attach the freshly spawned child is idle — nothing is appending — so a provably absent message should reach `rejected: not_delivered` and become re-sendable via Retry.
## Suggested approach
- Derive `turnInFlight` from an actual in-flight dispatch (a running turn / an outstanding `pending` send) rather than `this.sessions.has(...)`.
- Alternatively, run the sweep + reconcile before `acquireOwner` spawns the child, while keeping the boundary-consistency reasoning the placement is meant to buy.
## Open questions for the human
- Is the intended primary path the cold restart (where the child is already spawned by `acquireOwner`), or the `settlement-retry` path (`structured-agent-session-settlement-retry.ts:28`, where no child is yet spawned and the reconcile *does* decide)? The description reads like the former.ℹ️ Nitpicks
- The fingerprint equality the window relies on is only pinned for whitespace-free text.
claudeContentBlocksnormalizes block-form text throughextractString(.trim()), while the stored send body is fingerprinted fromstructuredAgentSessionSendBody(.trimEnd()); these diverge on leading whitespace, so a delivered message with leading whitespace stored as block content would fingerprint differently and be wronglyrejected. Worth a whitespace normalization test, given a wrongrejectedis the one outcome that costs a duplicate.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Electron CDP loaded-app evidence (PR worktree)Launched this PR worktree with {"name":"Orca: brennanb2025/wire-submission-reconciler","isDev":true,"devLabel":"wire-submission-reconciler","devBranch":"brennanb2025/wire-submission-reconciler","devWorktreeName":"wire-submission-reconciler","devRepoRoot":"/Users/brennanbenson/orca/workspaces/orca/wire-submission-reconciler","dockBadgeLabel":null}Visible state: isolated profile (no workspaces registered), full Orca chrome, and the Agents Session History surface ( Renderer console: 0 errors, 3 warnings (React DevTools hint, Radix tooltip controlled/uncontrolled, React Grab outdated). Main process:
|
Electron CDP visual verification (follow-up)Launched this PR worktree with App identity (
What was exercised
Visible UI
A collapsed Playwright inspector chip at the bottom center is CDP tooling, not product UI. Gap (not fabricated) Cleanup |
Review status: land after fixesThe review loop is clean and the finalized changes are pushed at Fixed the material restart-reconciliation issues found during review:
Validation passed locally:
Electron CDP evidence is attached in the visual verification comment. It verified the exact PR worktree identity and rendered Orca chrome. The reconciliation path is main-process/backend behavior, so it was not reproduced in the UI; no mobile-facing surface is involved. Final readiness checklist: PASS, with no proven P0/P1/P2 findings. GitHub CI for this head is still in progress. The pre-existing unrelated |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The delta since the prior Pullfrog review (ebd22d9) hardens restart reconciliation and, critically, makes the rejected: not_delivered outcome reachable in the cold-restart path it was built for.
- Provider history sampled before acquisition (
structured-agent-session-attach-flow.ts) —performAttachnow reads the window beforeacquireOwnerand passes it intoattachJournal, so the adapter's liveness map does not yet contain the new child andturnInFlightis false; a lease this process has not indexed still forcesturnInFlight: trueviaownerAlreadyAdmitted. - Account-home transcript resolution (
claude-structured-history-window.ts, adapter + router + interface) — the window resolves the Claude JSONL from the record'saccountHome.pathinstead of the process default, mirroring the existingreadTranscriptLeafresolution. - Whitespace-preserving fingerprints (
claude-structured-history-window.ts) —rawTextPartsrecovers the raw text bytes the shared decoder would trim, and bails when raw parts and decoded blocks do not line up, closing the leading-whitespace mismatch the earlier nitpick raised. - Batch fingerprint matching (
journal-submission-reconciler.ts) — identical unsettled sends sharing a fingerprint resolve as a group: a single candidate marks all of themambiguous_matchrather than assigning it to the first and rejecting the rest. - Single text-block comparability and committed-item exclusion (
journal-restart-reconciliation.ts) — only single text-block bodies are reconciled, and history items already consumed by anacceptedsubmission are excluded so an older identical message cannot stand in for a new send. - Test coverage — account-home resolution, whitespace preservation, batch ambiguity, multi-block sends, older accepted items, and the pre-acquisition sampling order (the last fails if the window is read after
acquireOwner).
Both prior concerns — the unreachable not_delivered path and the whitespace fingerprint drift — are addressed, and I found no new issues in the delta.
openrouter/deepseek/deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
CI updateThe transient Node 24 shard failure was rerun successfully after its original |
ELI5
If the agent process dies or Orca restarts while your message is in flight, Orca currently gives up and marks it "delivery unconfirmed" — forever. Retry refuses (correctly, it might duplicate), so your only option is to select your own text, copy it, and type it again. This makes Orca go and look at the agent's transcript and decide: it either landed or it didn't.
The critical change, in one sentence
Orca now asks the agent's transcript what happened, instead of assuming the worst and stopping.
journal-submission-reconciler.tsdocuments a two-step recovery in its own header:Only the first half was ever wired:
reconcileSubmissionsis imported by one test file and has no production caller. So every message in flight at a crash became permanently unconfirmed, with no attempt to find out whether it had actually landed.After:
The history window is the Claude project transcript — the file a resume actually replays — which is what makes "absent from it" mean something rather than nothing. It is sampled before a new agent child starts, because once one is running an absent message can no longer prove non-delivery.
That is the change: a step that was written, tested, and never called now runs.
What you see today vs after
The agent's process dies mid-send, or Orca restarts while a message is in flight.
The key change is that Orca stops guessing. It reads the transcript the agent will resume from and gets one of three answers: found (settle silently, no banner), provably absent (mark it as not sent, so Retry is safe and works), or genuinely can't tell (say so honestly and leave it alone).
Why
There's a module in the repo,
journal-submission-reconciler.ts, written and fully tested for exactly this. Its own header describes a two-step process: "Every survivingpendingbecomesunknownand is then matched against provider history."Only step one ever shipped. Step two has never had a production caller — the function is imported by one test file and nothing else. So Orca marks everything unknown and never does the matching, which is why a stranded message has no recourse.
This is also the gap that makes us unusual. Every comparable implementation can resolve this ambiguity by reading something authoritative. We had the machinery and never called it.
What changed in the code
proveClaudeTranscriptBranchFromJsonlrather than inventing a check: a forked branch, a compacted log and a truncated tail each already throw there, and each becomes "this window proves nothing."Limits, stated plainly
Two guards worth reviewing closely
Because Claude doesn't echo our id, the fingerprint path is the entire risk surface — and a wrong "provably absent" means a delivered message gets sent again.
Both fail a named test when removed.
Found and not fixed here: the block decoder silently dropping base64 images is a pre-existing bug with a blast radius beyond this feature. It deserves its own change.
Testing
pnpm tcclean. 23 new tests, each ablated. Two pre-existing failures, both verified:claude-structured-real-cli.test.tsfails identically on plainmain(checked out detached to prove it), andclaude-tui-resume-real-binary.integration.test.tsdies onps -axounder load, passes alone, and is skip-gated so it never runs in CI.claude-structured-session-adapter.tshit its line cap when the history method was added, so path resolution was split into the window module rather than shaved.Visual proof
N/A. No UI change — the visible effect is a delivery notice resolving itself on attach instead of persisting, which has no before/after frame that isn't simply the absence of a banner.
Notes