Repository navigation
perf(native-chat): parse a Claude transcript on attach only when a send is in doubt - #23606
brennanb2025 wants to merge 1 commit into
Conversation
…nd is in doubt Every Claude chat attach read the whole provider transcript before checking whether any message needed deciding: once for the branch proof, once more to replay the anchored window, and a third time for the id lookup, each parsing every line. Most attaches have nothing in doubt and paid for all of it. The attach now samples only what must be fixed before a new child starts: the running-turn fact, the transcript path, and its size and file identity (one stat). Reconciliation reads the sample only after the journal shows a send in doubt, and that read is one pass over exactly the pinned bytes, parsing each line once for the branch proof, the anchored window and the id lookup alike. Bytes appended after the pin, which the new child may have written, are never read, and a file replaced or truncated since the pin is not read as evidence. Linear: STA-7434
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Minor suggestions only.
Reviewed changes
- Lazy transcript read contract —
ProviderHistorySample(journal-submission-reconciler.ts) replaces the eagerProviderHistoryWindow; the adapter method becomessampleProviderHistory, andreconcileJournalSubmissionsAgainstHistorynow callshistory.read()only aftercomparableSubmissionsis non-empty. - One-stat pin —
sampleClaudeProviderHistoryresolves the path and pins{ size, dev, ino }with a singlestat;readPinnedClaudeTranscriptre-verifies identity, streams exactly the pinned byte range once, and refuses a replaced/truncated/unlinked file. - Single shared pass —
createClaudeBranchAncestryPass+createClaudeHistoryPassfeed the branch proof, the recorded-history id lookup, and the anchored window from one parse per line; the old two-pass replay,createWindowCollector/createRecordedCollector, andreadClaudeRecordedHistoryare removed. - Conservative behavior changes — no growth-retry (bytes appended after the pin are never evidence), and
inconsistent()now carries the realturnInFlight, so a handed-over send absent from a whole file during a running turn stays unknown instead of rejected.
I traced the refactor against #23604's collectors and found the recorded-history/window semantics preserved (first-seen uuid tie-break, harness-injected filter, fingerprint map, whole flag), with the read-error and replacement paths strictly more conservative. The lazy gate is genuinely covered: structured-agent-session-acquisition-options.test.ts fails if the sample is taken after acquisition, and the new read-count test fails if the parse is eager. Focused suites pass locally (99 tests).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| state.observedStatBytes = snapshot.size | ||
| stat: async (options?: Parameters<FileHandle['stat']>[0]) => { | ||
| const snapshot = await handle.stat(options) | ||
| state.observedStatBytes = Number(snapshot.size) |
There was a problem hiding this comment.
The state.growth machinery in this mock (declared at line 16, reset in beforeEach, and consumed in the if (state.growth) block just below) is now dead — no test sets it after the growth-retry tests were removed. Dropping it keeps the mock honest about what it actually simulates.
|
Review status: recommend closing this PR. #23604 now does its job. #23604 moved while this PR was under review. Its commit 07368bb reads the Claude transcript on attach only when a send is actually in doubt, and it also postpones the transcript path lookup until then. Its commit 50b9bf2 makes a restart only ever confirm a send: a send found under its own frame id is delivered, anything else stays unconfirmed. After that change, the byte range this PR pins before starting the new Claude process no longer protects anything, because extra bytes can only add true matches. I merged the new base locally. Every conflict came down to the base's version, and the merged tree is identical to #23604, so nothing was pushed. Measured on a synthetic 52 MB transcript (8,000 records, 2,000 prompts), median of 7-9 runs, new base vs this PR's single-pass read:
The only remaining wins are for sends written before #23604, and they last only until those are gone. Getting them would mean rewriting the branch-proof code #23604 just stabilized, which isn't worth a PR. Live QA on a remote Mac (before the base moved; 42 MB transcripts):
Found during QA on the old #23604 head (not caused by this PR, and not yet re-checked on the new #23604 head):
Not done: the end-of-review readiness pass, and screenshots, since there is no change left to ship. |
|
Closing: superseded by #23604, which removes the transcript-based delivery decision after a restart entirely (a message Orca can't confirm is shown as sent with one session-level notice), so this change is no longer needed. |
1 participant
ELI5
When Orca reopens a Claude chat, it can check Claude's saved transcript to see whether a message sent just before a crash actually reached Claude. Before this change, it read and parsed the whole transcript on every reopen, even when no message needed checking, and when one did it read the file two or three times. Now it only takes the file's size up front. It reads the transcript only when a message really is in doubt, and then it reads it once.
What Changed
Before: every Claude chat attach streamed the whole project transcript before looking at whether anything needed deciding. The branch proof parsed every line, a second pass re-parsed every line to collect the prompts after the anchor, and the id lookup from #23604 opened the file a third time and parsed every line again. All of this ran on the main process during startup restore, once per restored chat.
After:
stat, with no open and no parse.read()only after the journal shows a send that is pending or unknown. That read is one pass over exactly the pinned byte range. Each line is parsed once and feeds the branch proof, the anchored window and the id lookup together. Each prompt is kept as one small fingerprinted row under the first line that carries its uuid. The window is picked out once the proof has named the chain.providerHistoryWindowis renamed tosampleProviderHistoryand now returns{ read() }. The Claude adapter, the router and the test harness follow.Measured on a synthetic 50.6 MB Claude transcript (8,000 records, 2,000 prompts), median of 7 runs on an M-series Mac:
Both runs produced the same window (2 items, boundary consistent) and the same id set (2,000 user records).
Why
Why the sample is taken before acquiring the owner: the ordering is load-bearing, so it stays.
acquireOwnerstarts the new child, the adapter's liveness signal becomes conservative, so a missing prompt can no longer prove it was never delivered.What is taken early, and what is left late:
Alternatives considered:
onAcquiring/beforeJournalOpen) around acquisition, which is riskier than deferring the parse.statidentity check gives the same safety with nothing to clean up.Behavior differences, all in the conservative direction:
SSH and remote: Claude structured sessions only run on the execution host's own local filesystem (non-WSL), so the transcript is always local to the process doing the read. Nothing new is persisted and nothing changes on the wire.
Linked Issue
Linear: STA-7434
Visual Proof
N/A. This changes when and how often a file is read on the host, and nothing on screen changes.
Testing
New
structured-agent-session-attach-transcript-reads.test.tsdrivesattachJournalwith the real Claude sampler over a real transcript and counts opens and streams of that file:Updated tests:
structured-agent-session-acquisition-options.test.tsasserts the sample fixes liveness before acquisition,readis never called for the first attach, and it is called exactly once for the attach with a crashed send.claude-transcript-branch-streaming.test.tsandclaude-history-window-source-budget.test.tsnow pin the new read contract: one stream over one descriptor, bytes appended after the pin are never read, and a file replaced, truncated or unlinked after the pin is refused.journal-restart-reconciliation.test.tsgains "reads no history when nothing is in doubt" and "leaves a send in doubt when reading history fails".Commands:
pnpm tc:node: passes.pnpm run check:code-quality:changed: 0 findings.oxlintandoxfmton the changed files: clean.src/main/native-chatandsrc/main/claude: passes, exceptclaude-structured-real-cli.test.ts, which drives the locally installed Claude CLI and fails the same way on fix(native-chat): after a crash, show an unconfirmed message as sent with one chat notice instead of guessing from the transcript #23604's code.Not done: live Electron QA of a crash-and-restore flow.
Review
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
Ensure no issues in: Security, Cross-platoform support (Linux, Windows, Mac), Remote SSH, Mobile, general backwards compatibility, performance
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)