Repository navigation
fix(native-chat): after a crash, show an unconfirmed message as sent with one chat notice instead of guessing from the transcript - #23604
brennanb2025 wants to merge 56 commits into
Conversation
… frame id A send left unconfirmed by a crash was judged by looking for its text in the transcript after the last completed turn. A message before that point (the point moves on with every later turn) or one starting with a harness-looking tag was missed, declared not delivered, hidden, and Retry sent it again. The hand-over now records the uuid Orca stamps on the Claude frame, which Claude keeps as the transcript record's uuid. A restart looks that id up in the whole transcript. Sends recorded by older builds carry no id and are never declared undelivered; they stay unconfirmed. Linear: STA-7416, STA-7453
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (51)
💤 Files with no reviewable changes (9)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change removes provider-history-based reconciliation of journal submissions after restart. Unacknowledged sends remain unknown, are not resent, and can produce a host-restarted status row at a crash boundary. A shared classification rule updates how mobile and renderer surfaces display unknown and legacy not-delivered submissions. Related tests and delivery notices are updated. Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Priority: ➖ Normal Estimated code review effort: Merge Risk: 🔵 Low · up to A journal write failure during recovery can leave a sent-looking message without its session-restart notice. This narrow failure does not appear to block merging, but it warrants owner awareness. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The change reduces the risk of sending a message twice after a crash. However, a failure while recording the restart notice can leave a message uncertain without restoring the explanation on a later reopen. That failure path warrants design review. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 46.84% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 79 functions across 53 files. (7 skipped: 7 unsupported.)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
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 |
There was a problem hiding this comment.
ℹ️ No critical issues — minor suggestions inline.
Reviewed changes
- Delivery decided by the frame id. At hand-over Orca mints a Claude frame uuid (
mintClaudeDispatchIdentity), persists it on thedispatch{pending}row ashandedOverItemId, and sends that exact uuid on the frame;dispatchClaudeTurnnow accepts a caller-suppliedsentUuid. - Whole-transcript lookup on restart.
resolveClaudeProviderHistoryWindowadditionally builds aProviderRecordedHistory(every user record's journal key plus keys indexed by payload fingerprint) in one streaming pass. - Reconciler gains an identity path.
reconcileSubmissionsresolves id-bearing submissions byhandedOverItemId: present → accepted; absent from a whole read with no same-text record → rejected (not_delivered); otherwiseunknown. No-id rows keep the anchored-window match, but a miss is nowunknown(no_dispatch_identity) instead ofrejected. - Journal plumbing and tests. Optional
handedOverItemIdon the pending dispatch row/reducer;providerIdentityonResolveDispatchInputand the adapter contract; regression tests for STA-7416/STA-7453 plus dispatch-identity durability.
ℹ️ The by-id absence proof rests on an unchecked append-only assumption
resolveByIdentity settles a send rejected: not_delivered from a whole-file read without consulting history.boundaryConsistent, which the legacy path still does. The absence proof (provesAbsenceOf) only checks that every line parsed and that the key names this session. This is not a demonstrated bug — but it means the safety of the new path depends on a property nothing in the code verifies.
Technical details
# Whole-file absence proof vs. transcript mutation
## Where
- `src/main/native-chat/agent-session-journal/journal-submission-reconciler.ts:188` — `provesAbsenceOf`
gates the rejection; `boundaryConsistent` is not consulted on this path.
- `src/main/claude/claude-structured-history-window.ts:244-254` — `finish()` returns
`provesAbsenceOf` true whenever `whole` (no line failed `JSON.parse`).
## Why it could matter
If a Claude client ever rewrites or truncates the session JSONL in place, a uuid that was written
would be absent, `provesAbsenceOf` would return true, and a delivered send would settle
`rejected: not_delivered` — hiding the message and inviting a duplicate via Retry, the exact cost
this PR removes elsewhere.
## Evidence (not reachable on the evidence available)
- Compaction appends a boundary/summary and keeps the original messages; `/branch`/`--fork-session`
writes a new file under a new session id, which fails the session-id equality in
`provesAbsenceOf` and stays `unknown`. Missing/unreadable file yields `null` history → no
reconcile. So the path is not demonstrably unsound for the CLI/SDK Orca drives.
- Counterexample shape to be aware of: Claude Desktop's resume path has been reported to truncate
the same `~/.claude/projects/<proj>/<id>.jsonl` files in place.
## Open question for the human
Is append-only a contract you can rely on here (worth a comment or a targeted test), or should the
by-id rejection also require the branch proof (`boundaryConsistent`)?ℹ️ Nitpicks
mintClaudeDispatchIdentityreconstructs the trailing prompt frombody.blockswhiledispatchClaudeTurndecides "is this a command" from the mapped content produced byclaudeDispatchMessageContent. Sharing that derivation would keep the mint and dispatch decisions from silently desynchronizing.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
… need it Opening a structured chat read the whole Claude transcript up to three times (branch proof, anchored replay, and the new whole-file id scan) on every attach, even when the journal had nothing to decide. The adapter now samples liveness at attach and hands back a history source; reconciliation reads the recorded history only for a stranded send handed over under an id, and the anchored window only for one without. A normal chat open reads no transcript.
…turn Claude records a frame that arrives while a turn runs in one of two ways besides its own row: folded into the turn, it gets only a queued_command attachment naming it by source_uuid; queued with others behind the turn, it is merged into one row under the last frame's uuid. The id scan read only type:user rows by uuid, so a delivered mid-turn send was judged not delivered, hidden, and offered for a Retry that sends it twice. The scan now takes a queued_command attachment's source_uuid as the frame's id, as Claude's own readers do, and indexes every text block of every recorded frame. A send whose id is absent stays unconfirmed while any copy of its text is left over once each send found by id takes its own, which covers the merged row. The scan moves out of the anchored-window module, which returns to its previous shape.
With no durable anchor yet, the anchored read reports an inconsistent window that carries no liveness. The attach's sampled liveness now decides the id path, so a send whose record Claude has not written yet stays unconfirmed instead of being called not delivered mid-turn.
The mint comment claimed Claude always records a frame under its own row uuid; a frame folded into a running turn keeps it as the attachment's source_uuid instead.
There was a problem hiding this comment.
Important
The merged-queue recording shape the new collector relies on to avoid false not delivered verdicts is unverified and has a real counterexample — see the inline note.
Reviewed changes (since the prior pullfrog review at 4f20e4b4b0)
- Provider history is now read lazily.
openClaudeProviderHistoryreturns aProviderHistorySource;readHistoryForreads the anchored window only for id-less sends and the recorded set only for id-bearing ones, and a failed read leaves every sendunknown. - Folded and merged Claude frames are indexed by id. The recorded collector also reads
queued_commandattachments (source_uuid→ the frame's id,prompt→ text) and indexes each text block of a merged row separately;unclaimedCopiessubtracts a copy for every send whose id the history holds. - The by-id path gained a liveness guard.
resolveByIdentityreturnsturn_in_flightbefore proving absence, and the attach samples liveness before a new child is acquired. - Tests and docs exercise the recorded history through a real transcript file and the restart-interruption harness.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
…s owner is accepted Restart reconciliation dropped every id the journal had committed from every copy of text in the transcript. When Claude records two queued sends in one row under the later send's id and that later send was already accepted, this also dropped the only copy of the earlier send's text, so the earlier send was called not delivered and offered for Retry, which sent it twice. A committed id now takes only one copy: the text it was committed with. A row that holds another send's text still vetoes that send's rejection, and a crashed repeat of an older delivered message is still decided not delivered.
There was a problem hiding this comment.
✅ No new issues found in the latest commits. Two earlier review threads are still open, so this isn't an approval.
Reviewed changes
- A merged send no longer collapses to
rejectedwhen its row's owner was accepted.unseenHistorynow subtracts a committed id from only the one fingerprint it was committed with (withoutOwnCopies), instead of stripping that id from every fingerprint list, so an earlier send's text surviving inside a later accepted send's Claude row stays claimable and the send remainsunknownrather thanrejected: not_delivered.unclaimedCopiesis refactored onto the same helper; the change is strictly more conservative. - Regression coverage. New reconciler cases pin a send merged into an accepted later row (same and different text) and a repeat of a text the journal already holds; a new dispatch-identity case pins the adapter stamping the recorded
handedOverItemIduuid onto the frame it sends.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
…frame id A send found under its own frame id is delivered; one that is not found stays unconfirmed. Claude records frames queued together under the last one's id, so a delivered send can leave no record of its own, and calling it not delivered hid it and offered a Retry that sent it twice. Restart reconciliation no longer decides `rejected` at all, which removes the same-content check, the per-block text index and the whole-file flag that only served that verdict.
|
Review status: code ready, on hold for one product decision (head 50b9bf2). What changed during review (new commits, no force-push):
Validation: node typecheck passes at 50b9bf2; the changed suites pass with ablations showing each new test fails without its fix; lint and the changed-lines quality gate pass. Live crash-and-restart QA against the real Claude CLI on a separate Mac is in the PR body's Visual Proof. On hold because of what live QA showed: after a crash, reopening the chat still shows a delivered message as "Message delivery is unconfirmed." with Retry (and after an earlier completed turn the message is hidden) until the user sends another message. The delivery check only runs when the chat next starts Claude, not on reopen. Main behaves identically, so this PR does not cause it, but the user does not yet see "sent" at reopen. Decisions needed: (1) run the id lookup when the chat is reopened too, which is safe now that only a found id can change a message's state; (2) confirm the trade that a message Claude truly never received shows "unconfirmed" rather than "not delivered" after a crash. |
There was a problem hiding this comment.
ℹ️ No critical issues — the new commit is a sound simplification; one description mismatch to reconcile.
Reviewed changes
Since the prior pullfrog review at 2aeb8b0, one commit landed (50b9bf2):
- Restart reconciliation no longer decides
rejected.resolveByIdentityreturnsacceptedwhen the frame id is found andunknown: not_foundotherwise; the id-lessresolveOnepath now also ends atunknown: not_foundrather thanrejected: not_delivered.SubmissionReconciliationdrops itsrejectedvariant, andreconcileJournalSubmissionsAgainstHistoryonly ever writesaccepted, so no restart path can hide a delivered message or offer a Retry. - The absence-proving machinery is gone.
ProviderRecordedHistorykeeps onlyitemIds;itemIdsByFingerprint,withoutOwnCopies/unclaimedCopies,provesAbsenceOf, the whole-file flag, and theambiguous_matchveto they fed are removed, along with the second fingerprint pass increateRecordedCollector. - Caller and tests updated.
readHistoryForstill reads only what a strand needs (window for id-less sends, recorded ids for id-bearing ones); the reconciler, attach, crash-boundary, provider-history, acquisition-options and restart-ownership suites were rewritten to expectunknownfrom a miss and acceptance from a found id.
ℹ️ The PR description still describes the by-id "not delivered" verdict the final commit removed
50b9bf2 makes restart reconciliation incapable of ever producing rejected: not_delivered: an id absent from the transcript now leaves the message unconfirmed, never not delivered. The PR body still describes the opposite — the ELI5 and "What Changed" say Orca "says 'not delivered' only when it can prove the message is missing", the mechanism spells out the "id missing ⇒ not delivered if every line parsed, nothing running, no same-text copy" rule, and the "Why" section explicitly rejects "stop ever saying 'not delivered' (#20723)" as throwing away a correct answer the product relies on. That rejected alternative is now the implemented behavior, and notDelivered has no production writer left (it survives only as read-only legacy vocabulary in structured-agent-session-dispatch-rejection.ts). Only the author can say which is intended: rewrite the description (and the #20723 supersede note) to match the conservative design, or reconsider the commit.
Technical details
# Stale PR description vs. the implemented behavior
## Affected sites
- PR body only — the code is internally consistent; the mismatch is with the PR's stated contract.
- `src/main/native-chat/agent-session-journal/journal-submission-reconciler.ts:7-11` — comment now states the new rule ("no send is ever called not delivered").
- `src/main/native-chat/agent-session-journal/journal-restart-reconciliation.ts:4-8` — "It never decides `rejected`".
- `src/shared/structured-agent-session-dispatch-rejection.ts:40-42,136` — `DISPATCH_REJECTED_NOT_DELIVERED`/`notDelivered` are now read-only (no producer).
## Required outcome
- Align the PR description's ELI5, "What Changed", mechanism, "Why / Alternatives considered", and "Testing" sections with the commit (or revert the commit if the by-id rejection was intended to stay).
- Note in the supersede line that the final behavior matches the #20723 approach for the restart path.DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
…-delivery-by-message-id
The restart decision moves to the session level; nothing reads the provider transcript to decide delivery any more.
…ding the transcript The crash boundary no longer decides delivery from the provider's saved transcript. A send an earlier process handed over and never heard back about stays in doubt, and the chat's open writes one status row saying the session did not survive the restart. The row comes from the stale settlement that already writes the exit row, so one boundary never shows two; an acquisition that is first to open the chat now settles there too, with the death evidence it read before its reserve. Deletes the anchored history window, the submission reconciler, the attach-time reconcile and its pre-acquire sampling, and the adapter hook that fed them. The attach result keeps an always-empty unconfirmedClientMessageIds for older parsers.
…field's meaning The restart fact gets a row sentence: the agent's session didn't survive the restart, send a message to continue. unconfirmedClientMessageIds is documented as always empty, and an unknown submission as drawn as sent.
…or Retry A submission the host settled unknown, or an older host's not-delivered verdict, is an ordinary sent message on desktop, paired web and mobile: the outbox drops it, nothing queues behind it, and there is no per-message Retry. The refused-redelivery path and its untranslated error go. Mobile spends the operation id on a host unknown, so typing the message again sends a new one. The unconfirmed state and its copy stay only for a send whose reply this client lost. The command refusal for an unsettled message no longer offers a retry that does not exist, in every catalog.
There was a problem hiding this comment.
Important
The final test commits break the CI typecheck: pnpm tc:web fails on NativeChatStructuredSession.transport-probe.test.tsx:179. Everything else in the reversal reads as sound.
Reviewed changes
Since the prior pullfrog review at 50b9bf2, six commits landed (plus a merge of main), reversing the by-id mechanism:
- Dropped the transcript id lookup.
resolveClaudeProviderHistoryWindow,openClaudeProviderHistory,journal-submission-reconciler.ts,journal-restart-reconciliation.ts, and thehandedOverItemId/mintDispatchIdentityplumbing are deleted; nothing reads the Claude transcript on restart any more. - Doubt is drawn as sent. New
structuredAgentSessionSubmissionDrawsAsSent(accepted | unknown | legacyrejectedwith kindnotDelivered) now drives mobile delivery, the renderer outbox/dispatch/launch-prompt, message projection, shared outbox reconciliation, and send disposition; the send-dispositionrefusedRedeliveryparking path is gone. - One boundary row per crash. The conversation open settles stale state (now also for acquisitions) and writes a single
hostRestartednotice row when it left a send in doubt or an in-progress item, instead ofproviderExitedwhen an observed exitinterruptedit;unconfirmedClientMessageIdsis always[]. - Copy and mobile bookkeeping. The restart row now reads "…'s session didn't survive the restart. Send a message to continue."; mobile spends the operation id for
unknown, andclearMobileStructuredSettledSendOperationsclears everything notpending.
ℹ️ The transcript branch-proof module now has no production caller
Deleting claude-structured-history-window.ts removes the only production consumer of claude-transcript-branch-proof.ts (and the claude-transcript-branch-graph.ts it builds on). Both modules now survive only for their own tests, so the PR can delete them and their test files too, or say why they stay.
Technical details
# Orphaned branch-proof modules
## Affected sites
- `src/main/claude/claude-transcript-branch-proof.ts` — exports `proveClaudeTranscriptBranchFromJsonl`, `replayClaudeTranscriptBranchAncestry`, `replayClaudeTranscriptBranchAncestryFromJsonl`; imported only by `claude-transcript-{rewind-proof,marker-branch-proof,branch-streaming}.test.ts`.
- `src/main/claude/claude-transcript-branch-graph.ts` — imported only by `claude-transcript-branch-proof.ts` and its own test.
## Required outcome
- Either delete the two modules and their three test files as part of removing the history window, or leave a note that they are intentionally retained.
## Suggested approach
- Confirm no non-`src` consumer first; a repo-wide search for `transcript-branch-proof` / `transcript-branch-graph` returns only these modules and their tests.ℹ️ Nitpicks
- The PR description still documents the removed id-lookup mechanism (ELI5, "What Changed", the mechanism bullets, and "Why" all describe looking the message up by a minted uuid and reading Claude's transcript on restart). The final commits delete that path entirely; the description needs the same rewrite the code got.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
…er rejection keeps its text A live unknown can still turn rejected (an agent that never proves its start rejects it). Dropping the outbox entry on it left that rejection with nothing to draw: the message vanished with no Retry. Only a recovered unknown is settled as sent now; a live one is held like a pending, which is no barrier.
|
| page: readAgentSessionHydrationPage(attached.journal, fence), | ||
| unconfirmedClientMessageIds: attached.unconfirmedClientMessageIds, | ||
| // No host decides delivery after a restart any more; kept because the wire type requires it. | ||
| unconfirmedClientMessageIds: [], |
There was a problem hiding this comment.
Restart confirmation was removed When Claude saves a message and Orca crashes before recording its reply, attach now opens the journal without checking Claude's transcript. The message remains
unknown instead of being confirmed as delivered, so the restart cases this PR aims to fix remain unresolved.
Comments Outside DiffThese findings could not be posted inline.
|
There was a problem hiding this comment.
Important
The merged tree does not compile: #23682 (merged in from main) deleted src/main/codex/codex-structured-journal-provider-verdicts.ts but left src/main/codex/codex-background-task-frames.ts importing it. Every failing check on this head — typecheck, static analysis, verify, and all six persistence shards — traces to that one unresolved import. It is inherited from main, not introduced by this PR's own changes; the settlement reconciliation itself reads as sound and its tests pass.
Reviewed changes
Since the prior pullfrog review at cd2da8e, one commit landed — the merge of origin/main (7445feaca) into the branch:
- The crash-boundary settle is reconciled with main's per-owner death rule. A new
structured-agent-session-stale-settlement-row.ts(created by the merge's conflict resolution) extractsstaleSettlementRowandcrashBoundaryExplainedProvenTurn. The settle writes the provider-exit row only when a proof names the turn's own owner (diedInProgress) or a proof revised a turn no boundary row already explains (provenUnexplained), and keeps the branch's one-row-per-boundary crash notice otherwise. Main's per-fenceverdictForandprovenUnverifiableTurnRevisionsare adopted wholesale; the branch's pre-#23456stale-turn-verdict.tshad no unique logic to lose. - The conversation open keeps the branch's acquisition semantics. The object-shaped
acquisition: { deathEvidence }option, the single settle call, and thesettleGoneGenerationhelper survive; main'sresettleOpenStructuredAgentSessionConversationis retained and all callers pass the new shape. - Tests updated for the union. The stale-turn-verdict suite gains a "crash boundary writes at most one row" block including the proof-arrives-later suppression case; the wedged-profile-migration remote case now expects the crash-boundary row.
⚠️ The merged tree imports a module #23682 deleted
codex-background-task-frames.ts:10 imports readCodexProviderVerdict from ./codex-structured-journal-provider-verdicts, a module absent from both merge parents' tips and from the merged tree. The same esbuild resolution failure stops TC_NODE, static analysis and the persistence build on the head, so nothing downstream can run. This is main's breakage (7445feaca fails the same way), not this PR's — but this head cannot merge green until the importer is reconciled with the deletion.
Technical details
# Dangling import of a deleted Codex module
## Affected sites
- `src/main/codex/codex-background-task-frames.ts:10` — `import { readCodexProviderVerdict } from './codex-structured-journal-provider-verdicts'`; used at line 54.
- `src/main/codex/codex-structured-journal-provider-verdicts.ts` — deleted by main's `#23682` (`94b5a7d256`), which made only `turn/completed` end a Codex turn.
## Required outcome
- The head must compile: the importer stops referencing the deleted module. Per `#23682` the `error`-based `turn-failed` arm of `readCodexChildTurnEnding` (lines 43-46) has no remaining source; `thread/closed` → `unverifiable` is the surviving child-turn ending.
## Notes
- Reproduced independently: a `7445feaca` worktree fails `tsc -p config/tsconfig.node.json` with the same `TS2307`; remote `main` has no such module while its importer still references it.
- The PR's own new files are clean — this is the only `tc:node` error, `tc:web` passes, and oxlint is clean on the merge-touched sources. The 250+ focused tests I ran around the settlement/restart/fold paths all pass, including main's `#23456` `crash-turn-end` suite against the merged code.DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
… client The suite's client advertised only the structured capability, so the host down-projected the crash-doubted send to accepted for it while the test expected the raw unknown. Advertise the current remote capability list and pin the recorded unknown + recovered facts on both the send reply and the journal history.
There was a problem hiding this comment.
✅ No new issues found. The delta is a test-client correction; one earlier review thread (a shared-doc wording nit) is still open, so this isn't an approval.
Reviewed changes
Since the prior pullfrog review at 7cb874af, one commit landed (c2004eb):
- The crash-restart integration test now reads the journal as a current client.
CLIENTbuilds its capability list withremoteRuntimeClientCapabilities([...])instead of a bare[STRUCTURED_AGENT_SESSION_RUNTIME_CAPABILITY], so it advertisesagent-session.crash-doubted-send.v1(and the turn-item capability) the way a current remote client does. The host therefore returns the realunknown, recovered: truesubmission rather than theaccepteddown-projection a client that predates the capability is served. - The crash case pins the settled facts, not just the send reply. The in-flight submission must carry
recovered: true, and a follow-upagentSession.historyread must return that same recovered submission, covering both the send reply and history on the current-client path. The now-redundant per-callsubscribe(client)override and theAGENT_SESSION_TURN_ITEM_CAPABILITYimport are removed, sinceremoteRuntimeClientCapabilitiesalready includes it.
Verified locally: the suite passes (18 tests) after stubbing the inherited dangling codex-structured-journal-provider-verdicts import for module resolution; tc:node still fails on that inherited import, as raised in the prior review.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
…-delivery-by-message-id
There was a problem hiding this comment.
✅ No new issues found. The delta is a merge of
origin/mainthat lands the fix for the build blocker raised in the previous review; one earlier review thread (a shared-doc wording nit) is still open, so this isn't an approval.
Reviewed changes
Since the prior pullfrog review at c2004eb, one commit landed — the merge of origin/main (643f93f) into the branch:
- The branch build blocker is resolved by main's
#23801. The merge's only production change issrc/main/codex/codex-background-task-frames.tsdropping its import of the#23682-deletedcodex-structured-journal-provider-verdictsand itserror-basedturn-failedchild-turn ending.turn-endednow fires only forthread/closed, withturnId: nullandstate: 'unverifiable'. This clears thetc:node/static analysis/verify/persistencefailures reported last run. - No PR-authored code changed. The merge touches 9 files in total, all main-authored: the Codex fix plus CI workflow,
config/scripts, and two renderer-settings files from other main commits. No conflict resolution touched a branch file, and no PR file differs from its pre-merge content. - No dangling references remain. A repo-wide search finds no remaining importer of
codex-structured-journal-provider-verdictsand noCodexChildTurnEndingtype use.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
…-delivery-by-message-id
There was a problem hiding this comment.
✅ No new issues found. One earlier review thread (a shared-doc wording nit) is still open, so this isn't an approval.
Reviewed changes
Since the prior pullfrog review at f71d13b, one merge landed (e69eee9c, second parent 5560e534):
- Merged
origin/main. Main's #23808 (25d9c57e) restores the Codex child-turn ending on awillRetry: falseerror—readCodexErrorWillRetryis reintroduced inline incodex-structured-thread-facts.tsandreadCodexChildTurnEndingagain returns aturn-endedframe withstate: 'failed', so a child turn no longer latches onworkingfor the life of the session. Main's #23802 (2d358df8) prunes the already-removedwaitForSetupBeforeAgentHelpfrom the six locale catalogs, and the README downloads badge advances. - No PR-authored file changed. The merge's first-parent diff is main's four files only. The branch's own removal of the same help key and main's prune merge to the same result: the key is absent from all six catalogs and their JSON still parses. The dangling
codex-structured-journal-provider-verdictsimport raised in the prior review is grep-clean at head, so the earlier build blocker stays resolved.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
… start that threw, and Stop A handed-over send the provider never answered counted as owed work with no exit, so the idle sweep held the agent and the chat read working until the tab closed, and Stop did not clear it. It now stops counting as owed work, settled in doubt (recovered unknown, drawn as sent, no row), at each exit: - the provider ends the turn it was running (Claude result, Codex turn/completed, including a failed turn after an error Codex will not retry): the journal settles every open send at that fence inside the terminal row's serialized write, never for a revision of an earlier turn or a host settlement; - the dispatch threw or lost its write outcome: written settled at once; - Stop: after the interrupt, and a failure to record it never fails the Stop. A late acceptance or refusal still replaces the doubt. The Codex idle report path that only released live unknowns is removed: nothing writes a live unknown any more.
… when its proof landed One Orca crash could read "stopped while this response was in progress" or "didn't survive the restart" depending on whether the probe proving the old agent dead ran before or after the chat opened. The death evidence's kind is the boundary: only a running Orca watching the agent records exit-observed, and a probe proves death only for an owner whose Orca process was already gone. So probe evidence now writes the restart row, keyed by the dead owner's fence so both orders name the same row; an observed exit keeps the stopped row.
…nst the real journal shapes A Claude provider handle carries its leaf, and a turn has no 'failed' state: a failed turn completes with a failure outcome. Cover the real terminal states, unverifiable included.
| try { | ||
| return child | ||
| ? await plan.run(ctx) | ||
| : { ok: true, value: { turnId: params.turnId, cancelled: withdrawn.length > 0 } } | ||
| } finally { | ||
| await settleSendsLeftByStop(context, ctx) | ||
| } |
There was a problem hiding this comment.
Stop settles unrelated sends When Stop names an already-finished or different turn, the adapter can return
cancelled: false while another turn still has an unanswered send. This finally block still marks every handed-over send on the fence as settled in doubt. The chat then stops showing that send as pending, and the idle sweep can stop the agent before it answers.
| if (!childAdmission.accepted) { | ||
| return childAdmission | ||
| } | ||
| const admission = | ||
| event.method === 'turn/started' | ||
| ? turnBoundaries.start(event) | ||
| : turnBoundaries.complete(event) | ||
| if (admission.accepted) { | ||
| reportPrimaryThreadStoppedRunning() | ||
| } | ||
| return admission | ||
| return event.method === 'turn/started' | ||
| ? turnBoundaries.start(event) | ||
| : turnBoundaries.complete(event) |
There was a problem hiding this comment.
Idle sends remain pending If Codex accepts a send without returning a turn ID, then reports its primary thread idle without a terminal turn row, removing the idle-status settlement leaves that send pending. The new settlement path requires a terminal row, so the chat remains working and the idle sweep keeps an agent that has stopped doing the work.
There was a problem hiding this comment.
ℹ️ No critical issues — two substantive fixes, one documentation nit inline.
Reviewed changes
Since the prior pullfrog review at e69eee9c, two commits landed:
- A doubted send now has exits instead of pinning the agent forever.
0e4263faddsjournal-turn-end-dispatch-settlement.ts, wired intoAgentSessionJournal.appendItemandappendLifecycleBatch: a terminal turn row captures the open sends at its fence inside the row's serialized write and settles themunknown, recovered: trueonce it lands, so a handed-over send the provider never answered stops reading as owed work;handOverSubmissionwritesrecovered: truefor a start that threw or lost its write outcome; Stop settles the sends it left in doubt after the interrupt; and the Codexthread/status/changedrelease path is removed because no producer writes a liveunknownany more. - The crash row is named by the boundary crossed, not by when the proof landed.
c709f012makesstaleSettlementRowwrite theproviderExited("stopped while this response was in progress") row only forexit-observedevidence, and thecrash-boundary:<sid>:<ownerFence>restart row for a probe'spid-absent/identity-mismatch, so one Orca crash reads as one restart row whether the probe lands before or after the chat reopens.
I traced both mechanisms through their callers and the serialized journal write path, and independently had each commit reviewed; neither turns up a real-path over-settle, missed settle, or duplicate-row case.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
| * The evidence kind is the boundary: only a running Orca watching the agent records | ||
| * `exit-observed`, while a probe proves death only for an owner whose transport holder, the Orca | ||
| * process that started it, was already gone. */ |
There was a problem hiding this comment.
The second clause overstates the invariant, and that wording is the only stated reason the exit-observed split is safe. A probe can also run in the same live Orca: resolveStructuredSessionRecovery (reached from structured-agent-session-agent-start.ts:106 and structured-agent-session-attach-orchestration.ts:118) resolves a lease a failed unproven acquisition latched recovering, and agent-session-acquisition-failure-settlement.ts:134-146 (and :87-96) keeps that owner's ownerProcess while the Orca that started it is still alive, so its probe can return pid-absent. I could not make that path reach a visible row — the evidence fence it names carries no running turn, so diedInProgress/provenUnexplained stay false — but the guard that actually keeps the mapping correct is that structure, not the absolute claim as written. Consider naming the real guard (the evidence fence has no unsettled work) or noting the same-process probe case.
There was a problem hiding this comment.
✅ No new issues found. This commit is test-only; one earlier review thread (a shared-doc wording nit) is still open, so this isn't an approval.
Reviewed changes
Since the prior pullfrog review at c709f012, one commit landed (58f406f):
- The new settlement fixtures are typed against the real journal shapes. In
journal-turn-end-dispatch-settlement.test.tstheturnhelper now takes an optionalAgentJournalTurnItem['outcome'], theit.eachmatrix becomes the realcompleted/success,completed/failure,interrupted/cancellationandunverifiablestates (replacing the previously mis-typedfailed), and both this file andstructured-agent-session-crash-row-boundary.test.tsaddleafUuid: nullto the ClaudeproviderHandle. Test-only; no production change.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
…not at every turn row that ends A send written during a Claude turn that the CLI runs as its NEXT request cycle was settled at the first turn's result: the result arrives without it, the next cycle's init right after, and its echo seconds later. For that gap the chat and every session list read idle and the finished announcement could fire, then flipped back to working at the echo. The journal inferred the turn-over from any terminal turn row, but a Claude result ends one request cycle, not the CLI's work. Each provider now declares its own turn-over through the event sink, queued behind the rows ahead of it: Claude at its session-state idle (at its result only for a CLI that never reports session state), Codex at the primary thread's turn/completed as before. A child's exit, a superseded cycle and a host settlement no longer settle sends on the side; the dead-generation settlement and Stop own those.
…ent sink A journal that throws before returning its promise escaped the handler and stalled the queue behind it. The settle is bookkeeping, so any failure is logged and the next exit settles the send.
…tle tolerates its absence
| run: async (bound) => { | ||
| try { | ||
| await bound.journal.markPendingSubmissionsUnknown( | ||
| bound.fence, | ||
| DISPATCH_DOUBT_TURN_SETTLED | ||
| ) |
There was a problem hiding this comment.
New sends settled by old turns When a provider ends one turn, this queues settlement of unanswered sends but does not identify which sends belong to that turn. If an earlier journal write delays the queue, the delivery loop can hand over a new message before settlement runs. Settlement then marks that live message as recovered and no longer unanswered, so the idle sweep can stop the agent before it answers.
There was a problem hiding this comment.
✅ No new issues found. Two earlier review threads (a stale-settlement-row wording nit and a shared-type doc nit) are still open and were not touched by these commits, so this isn't an approval.
Reviewed changes
Since the prior pullfrog review at 58f406f, three commits landed:
- Doubts are settled at the provider's own turn-over, not at every terminal turn row.
settleSendsAtTurnOverjoins the event sink, which queues amarkPendingSubmissionsUnknownfor the bound fence. Claude calls it at itssession_state_changed idle, and at aresultonly for a CLI that never reports session state; Codex calls it at the primary thread'sturn/completed.journal-turn-end-dispatch-settlement.tsand its plumbing (thebeforeBuildhooks on the item/batch appenders and thewhichfilter onmarkPendingSubmissionsUnknown) are deleted, so a send a Claude result does not take and the CLI runs as its next cycle is no longer settled in the gap. - A failed turn-over settle can no longer stall the sink. The queued settle awaits the journal write inside a
try/catchthat only logs, so a journal that throws before returning its promise cannot escape the handler and wedge the queue behind it. - Regression coverage.
claude-structured-turn-end-settlement.test.tsgains a real 2.1.280 capture that keeps a steer owed from its send to its echo with no completion in the gap, plus an idle-settle case; the surface-lifetime suite pins that a turn row ending without the provider's word keeps the send owed.
Verified locally: claude-structured-turn-end-settlement, structured-agent-session-surface-lifetime, structured-agent-session-idle-sweep and codex-structured-journal-translation-settlement pass (62 tests), and pnpm tc:node plus oxlint on the changed production files are clean.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
…send owed Only the primary thread's turn/completed is Codex's turn-over for a send; a child thread ending its own turn must not settle it. Fails if the settle fires for any thread.
There was a problem hiding this comment.
✅ No new issues found. The delta is a single test-only commit; two earlier review threads (both documentation nits on files this commit does not touch) remain open, so this isn't an approval.
Reviewed changes
Since the prior pullfrog review at 76c2d5b, one commit landed (814cf11, test-only, 1 file, +25):
- The Codex child-thread turn-over is pinned as not settling a user's send.
structured-agent-session-codex-turn-end-settlement.test.tsgains a case that steers a follow-upsendinto the running primary turn, then emitsturn/started/turn/completedfor a separatechild-thread. The follow-up stayspendingwithowesWorktrue through the child's turn end, and only becomesunknownonce the primary turn ends (turns.end('completed')). Verified the case passes and is falsifiable: dropping theevent.threadId === primaryThreadId()guard inCodexJournalTurnBoundaries.completemakes the mid-child assertion fail (unknownvspending), because the doubt settle ridessink.settleSendsAtTurnOver, which only fires when that guard produced aturnLifecycle. No production change.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
Conflicts resolved keeping both sides: - codex-structured-thread-facts.ts: main's version (main removed the retry reader's last caller and now uses codexThreadStoppedRunning for the Stop turn-open wait). - host-mutations: main's working-gated Stop keeps the settle-in-doubt of sends left by Stop, now inline in the finally. - codex turn-end settlement test, transcript slots test: both sides' cases. - outbox hook: main's extracted unconfirmed probe, with this branch's comment carried into it. - NativeChatStructuredSession.transport-probe.test.tsx: main's deletion accepted; its "doubted send drawn as sent, never re-sent, next goes out" case ported to the outbox hook test.
…ettlement classifier The Stop-withdrawal restore and the turn-grouping in-flight set landed on main reading a submission's dispatch state directly, which the settlement boundary test refuses. Withdrawn is a refused send whose rejection classifies as withdrawn; the in-flight set is the open sends not yet matched to a provider item.
There was a problem hiding this comment.
✅ No new issues found. Two earlier documentation-nit threads are still open on files this delta does not touch, so this isn't an approval.
Reviewed changes
Since the prior pullfrog review at 814cf11, three commits landed:
- Merged
origin/maininto the branch. The merge adopts main's Stop-withdrawal (#23026), turn grouping (#23671) and terminal-chat changes while keeping the branch's crash-boundary settle (settleSendsAtTurnOveris still called from the Claude translator and the Codex turn boundaries); the conflict files resolve by keeping both sides, and the branch's settle-in-doubt of sends left by Stop moves inline into main's working-gatedfinally. - Main's two new send-state checks now read through the settlement classifier.
dispatchWasWithdrawnis folded intostructuredAgentSessionSubmissionWasWithdrawninstructured-agent-session-submission-settlement.ts— equivalent, since a withdrawn rejection iscancelled, nevernotDelivered, so it classifiesrefused— and the turn-timing in-flight set testssettlement === 'open'rather than a rawpending, so a liveunknownawaiting its echo can anchor a turn's opener fallback. - Main's new turn-grouping fold fixture was given the field the fold now reads. A test-only commit adds
reportsFailure: falseto the#23671fixture, clearing atc:nodebreak the merge created when main's fixture (which predates the branch's newly-requiredNativeChatTurnFoldRow.reportsFailure) was combined with the branch's type.
Verified locally at the live head (2586cf9b): pnpm tc:node and tc:web clean, 187 focused tests pass (settlement classifier + its boundary ratchet, turn timing, latest-request, send disposition, withdrawn-message restore, outbox and outbox admission), and live-head CI has no failing check.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
… interrupted Main's Stop now interrupts a turnless Stop only while the session reads working; the settle of a handed-over send in doubt runs after it either way. Only turn-naming Stops were pinned.
There was a problem hiding this comment.
✅ No new issues found. The commit is test-only; two earlier review threads remain open, so this isn't an approval.
Reviewed changes
Since the prior pullfrog review at 2586cf9b, one commit landed (c9ecc80):
- Pinned that a turnless Stop settles the send it interrupted. The existing "interrupts a handed-over message before its turn opens" case now also asserts the journal submission ends
unknown, reasonstopped_before_acknowledgement,recovered: true— the factsmarkPendingSubmissionsUnknownwrites and the settlement classifier reads as drawn-as-sent, so the chat reads idle. Before this commit only the cancel result, the provider call and the status row were pinned; the settle the reopen deadlock depended on had no end-to-end assertion. Ran the suite: 10 tests pass.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
|
Review status: ready for review at c9ecc80. CI green (all five unit shards, typecheck, static analysis, cross-version wire, packaging). Not merging. Since the last status, the open decisions were carried out, each matching how comparable agent chat tools behave:
Review: the architecture was re-checked after the "is this message settled" rule kept being copied into places that disagreed; every client reader now goes through one shared classifier, and a test fails on any new copy. Four review passes this round, the last two clean. Typechecks (node, web, cli, mobile) pass. Live QA on a separate Mac against the real Claude CLI (screenshots in the description): crash then reopen shows one notice and survives reopening; crash then send works; Stop on a message in doubt returns the chat to idle and the next message is answered; killing only the agent shows one visible "stopped" notice; quitting with a message in doubt shows no notice; a message typed while Claude is answering keeps the chat and sidebar working until it is answered, with nothing reporting "finished" early. Not run live: Codex (no usable login on the QA machine) and the phone (the emulator cannot enter a pairing code). Known edges are listed in the description, including a message the agent accepted but never echoed with no turn running (needs a wedged CLI). Relation to #21062: this PR covers the problem #21062 fixes (a message whose turn ended without an acknowledgement stays "working" forever), for Claude and Codex, but not all of its mechanism; #21062 is left open. |
|
Closing this stale crash-recovery implementation while preserving the remaining problem: a message the agent may have received must not be guessed unsent and delivered twice. |

What this fixes (scope)
<system-reminder>.ELI5
If Orca or the agent crashes right after you send a message, nobody can know for sure whether the agent got it. Orca used to guess by searching the agent's saved conversation for your message, and the guess was sometimes wrong. Now Orca stops guessing: your message stays in the chat as sent, and one line in the chat says what happened and that you can send a message to continue.
What Changed
Before, after a crash with a message in flight:
After:
orca worktree psand the phone now list a chat whose only message a crash cut off, as idle, instead of showing nothing for it.How it works:
unknownstate with itsrecoveredmarker; nothing is ever re-sent). A crash or a dead agent also gets its one notice row. One function decides which row, so one event never produces two, including when proof that the old agent died arrives after the chat was already opened; reopening the chat again adds nothing. Which sentence it shows depends on what happened, never on timing: the record of how the agent's death was learned says it. An exit that a running Orca watched gets "stopped while this response was in progress"; a death found by checking the process after Orca itself went away (a crash or restart) gets "didn't survive the restart", whether that check ran before or after the chat was reopened.unknown+recoveredstate a crash uses):idlestate, which it reports only once it has nothing left to run. A Clauderesultis not enough, because the CLI runs a message sent during a turn as its next request cycle, after that turn'sresultand with noidlein between; settling at theresultmade the chat read idle, and could announce the turn finished, until the message's echo a few seconds later. A Claude CLI that never reports its state gets itsresulttreated as the end instead; a CLI that does report it saysrunningbefore anything else in its first cycle, so which rule applies is settled before the firstresult. Codex:turn/completedon the chat's main thread; a subagent's thread ending its own turn does not count. An error the agent will not retry is covered too: Codex always follows one with a failedturn/completed(every case in the captured Codex transcripts does), and Claude's errorresultis followed by itsidle. A failure to record the settlement is logged and never holds up the chat's other rows; the next Stop, agent exit or reopen settles the message.agent-session.crash-doubted-send.v1. For a client that does not, the host publishes every message the shared classifier draws as sent (a settled-in-doubt message, or an older host's "not delivered" verdict still in the journal) asaccepted, the one state a released client already draws as sent. This happens only at the RPC boundary (history, the live stream and a replayed send's reply), the same per-client downgrade the turn record already uses. The journal and every host decision keep the real facts.Why
result; and a non-retrying error is read through the failed turn end the agent always sends right after it, because Orca's Codex path deliberately takes a turn's end only fromturn/completed.accepted: a released desktop drops its local copy and its Retry on it, and a released phone clears its saved send record, which is exactly "drawn as sent". A released client reads nothing else from these fields, and the emptyunconfirmedClientMessageIdslist is read by no client. Dropping the submission instead would leave a released desktop's copy unanswered, so it would re-send it under the same id and get the doubt back.Linked Issue
Linear: STA-7416, STA-7453, STA-7434 (no GitHub issue). Supersedes #23606 (STA-7434's separate fix), and the stale #20723 and #20792.
Visual Proof
Live QA on a separate Mac with the real Claude CLI. "Crash" means Orca was killed right after Claude saved the message and before its reply finished; then Orca was relaunched and the chat reopened.
Before (main):
<system-reminder>-shaped message, crash, then a new message: the message is hidden, there is no Retry, and the new message is stuck with nothing to click.After (this branch). Every after shot was taken before this revision's three changes (dropping the "didn't respond" notice, the narrower fold rule, and the downgrade for released clients). None of them shows the dropped notice, and each notice they show is the last row of its turn, so the narrower fold rule draws them the same way.
<system-reminder>-shaped message, crash: the notice is visible and the next message is answered; the message itself is still hidden by the separate display filter (see Notes). (predates this revision)This revision (decision changes), live on the same machine:
Final head (after the latest merge of main), same machine:
Latest head (doubted messages always settle; wording by what happened), same machine:
Not run live: Codex (no usable Codex login on the QA machine); the phone (the emulator's text input cannot enter a pairing code, and the only other route is not allowed); the idle sweep keeping an agent (its 30-minute window has no test setting), and an earlier failure followed by an answer (the real CLI does not produce one on demand) — both are covered by unit tests.
Testing
Live QA on a separate Mac against the real Claude CLI, main versus this branch, with Orca killed right after Claude recorded the message (see Visual Proof).
Automated:
acceptedon history, snapshot, batch, reset and a replayed send, a current or in-process client gets the real facts, the host's own record is untouched, and a liveunknown, pending, refused or accepted message passes through unchanged. The cross-version suite pairs the current host with a client derived from the newest release's list minus the capability.Review
agent-session.crash-doubted-send.v1, so the host publishes a settled-in-doubt message to it asaccepted, and it draws the message as sent with no note, Retry or barrier. It shows the crash notice as a plain line, which a released desktop client's older fold may collapse under "Worked for" above it. That is accepted: released client code cannot change, the notice is still there when the turn is expanded, and the phone does not fold.unconfirmedClientMessageIds, always empty; no released client reads it, and older parsers still accept the reply./clearand/compact, for Claude as well as Codex. It does not include all of fix(codex): steer a mid-turn send into the running turn by name #21062: fix(codex): steer a mid-turn send into the running turn by name #21062 settles only the messages Codex tied to that exact turn and writes them in the same database transaction as the turn's end, with a retry for a Codex settlement that fails to save; this PR settles every message still unanswered when the agent says its turn is over (Claude'sidle, Codex's main-threadturn/completed), right after that turn's rows are written, and leaves a failed save to the next Stop, exit or reopen.Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
<system-reminder>is still hidden in the conversation by the separate display filter that decides by text shape (native-chat-noise.ts). That is a pre-existing issue left for a follow-up.agent-session.crash-doubted-send.v1, delete the downgrade module and the capability check.claude-transcript-branch-proof.tsandclaude-transcript-branch-graph.tsnow have no production user and are left for a deletion-only PR.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)