Skip to content

fix(native-chat): every journal append reaches the chats that are open - #22811

Merged
brennanb2025 merged 22 commits into
mainfrom
brennanb2025/chat-journal-append-delivery
Sep 26, 2026
Merged

brennanb2025 merged 22 commits into
mainfrom
brennanb2025/chat-journal-append-delivery

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 30 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1705 $\color{#cf222e}{\Huge{\mathbf{−}}}$​113 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​1592
Prod 70 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​938 $\color{#cf222e}{\Huge{\mathbf{−}}}$​470 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​468

Scope: which chat this affects

  • Structured chat only. This PR changes the experimental structured native chat, which runs the agent through its SDK. It is behind the experimentalStructuredNativeChat setting, which is off by default (src/shared/default-global-settings.ts:137).
  • Default chat view: no change. The default chat view, backed by a terminal and reading the agent's transcript, does not use this code, and nothing about it changes.
  • Terminals: no change. Plain terminals and terminal-backed chats behave exactly as before.

Stacked on #22783.

ELI5

When Orca writes a new row into a chat's history (an error message, a status note, a settled message), it also has to tell the open chat windows about it. Those were two separate steps, and a few places did only the first. The row was saved but the open chat never showed it until you reopened the chat. Now saving a row is what tells the open windows, so no code path can forget.

What Changed

Before:

  • A resumed agent that dies before it finishes starting writes "stopped before it finished starting: …" and rejects the waiting message. If the host then fails to hand the session's lease back, the open chat never shows that row. The user sees nothing and their message looks stuck.
  • The same thing happened in two other places:
    • a provider revision written with no frame after it (the earlier context-usage bug fixed one caller of this);
    • the settlement written when a chat is evicted.
  • In each case the row was on disk, and a fresh subscribe would show it. An open chat never did.

After: every change that commits to an open chat's journal reaches that chat's open readers, including:

  • appends;
  • revisions;
  • epoch replacements.

The row shows up without the writer doing anything more.

Mechanism:

  • The journal store now reports every durable change through one listener, observeCommits. It fires after a row commits and after an epoch is replaced.
  • The host's session map is now StructuredAgentSessionConversations. When a journal handle is set into the map as a session's journal, the map binds that listener to the session's existing publish. That publish delivers new rows to subscribers and updates the status feed, the send-settlement waiters and the turn-completion feed.
  • Delivery runs one microtask after the commit. The writer's await resumes after the delivery, so ordering is the same as the explicit publish it replaces. A reader that throws is reported through onEventSinkError and cannot fail a write that is already durable.
  • A handle that the map has since replaced or dropped delivers nothing, so a closed or superseded journal is never read.
  • The map is the only way in. A session's journal field is now readonly, so a new handle can only reach a session through set, and set binds delivery. That holds even for collaborators that hold the map as a plain Map. The one test that swapped a handle in place now goes through set.
  • Each row is delivered exactly once. A provider frame that wrote a row still publishes afterwards, so both the commit report and the frame's publish run for the same row. Delivery reads forward from each subscriber's cursor and then moves the cursor past what it sent (structured-agent-session-subscribers.ts, the advanced check and the cursor update after each batch). So the second call finds nothing new and sends nothing. A subscriber already at the journal's cursor is answered without reading any rows, so that second call costs no journal read or timeline rebuild.
  • Reply ordering is unchanged. The delivery microtask is queued inside the commit, before the write's promise resolves. So it runs before the writer's await returns: before any RPC reply to a send, prompt answer, setOption, goal, compaction or rewind, and before the restart continuation continues. Code that reads the journal right after a mutation reads the handle's in-memory state, which the commit has already updated.

Deleted: every host-side publish that followed an append:

  • send, send refusal and send doubt (6 in turns.ts);
  • prompt answers (2);
  • thread goal (2);
  • compaction (4);
  • the failed-restart row;
  • late dispatch settlement;
  • releasing unanswered dispatches;
  • the restart-continuation note, plus that note's publish surface.

Kept on purpose, because these republish state held outside the journal:

  • the turn context's publish, used when setOption changes the record's options and when rewind changes the record's rewind phase (both feed the status projection);
  • publishFence, a snapshot when the fence moves;
  • attach's adoption publish, reset or snapshot for a newly opened handle.

Why

The bug class is a writer that forgets the second call. It has already shipped twice:

  • the context-usage revision;
  • the startup-failure row, which the exit path publishes only if (released).

The next planned change adds six more append sites. Adding the missing publish at each known site would leave the class open.

The alternatives:

  • A publish at each writer. This is what the code did before, and it is how both bugs happened.
  • Bind delivery at open time. It would thread a callback through three journal-open paths, and the journal opened during attach is provisional until it is adopted. Binding at the point the handle becomes the session's journal covers every open path, and it covers any future one without new plumbing.
  • A macrotask timer to batch more rows into one frame. Rejected for three reasons:
    • It would let a delivery land after the journal closes.
    • It would stall under the fake timers that 39 structured-session test files use.
    • The writer's await would no longer see delivery done.

Differences from the common pattern:

  • The common shape notifies subscribers from inside the append primitive, after the transaction commits. That part matches here.
  • It usually passes the notifier as an argument on every append call. Here it is bound once per journal handle, when the handle becomes a session's journal. That keeps it off every writer's signature.
  • Delivery pushes a cursor batch to each subscriber, as the existing subscriber set already does, rather than sending a "changed" signal that clients follow with a re-fetch.
  • It is not coalesced across a whole provider frame, which departs from the original plan for this change. A frame that writes three rows now sends three small batches instead of one (measured through a live subscriber). Streamed text is already folded into one row per 60 ms before it reaches the journal, so the high-volume path is unchanged.

Linked Issue

Native-chat redesign, step E: every journal append reaches open readers. It must land before the delivery-loop change, which adds the new append sites.

Visual Proof

N/A: this is host-side delivery. The fix is proven at the subscriber in unit tests (below). A before/after screen would need a lease-release failure injected into a running app.

Testing

New tests. Each asserts what a live subscriber receives, not what the journal holds.

  • structured-agent-session-append-delivery.test.ts, through the real host:
    • A failed start whose lease release is refused still shows its "stopped before it finished starting" row, and the message is rejected. This is the if (released) path.
    • A provider revision queued with no publish behind it is shown.
    • A row appended straight to the journal handle is shown.
  • structured-agent-session-conversations.test.ts:
    • An unpublished append is delivered.
    • An epoch replacement is delivered.
    • A replaced handle delivers nothing.
    • A dropped conversation delivers nothing.
    • A reader failure is reported and the write still lands.
  • Exactly once, in structured-agent-session-append-delivery.test.ts:
    • A provider-streamed row whose frame also publishes arrives once, and the journal's rows are read once for it, not twice.
    • A host-appended row followed by an explicit publish arrives once, and the second publish sends no batch at all.
  • In structured-agent-session-conversations.test.ts: a handle set through a plain Map reference is still bound.
  • structured-agent-session-surface-lifetime.test.ts: a compatibility send-wait on a chat that is evicted now answers with the unknown settlement that eviction journaled, instead of nothing. Before this change that settlement never reached anyone.

Ablations. Each was run by deleting lines from the committed file, then restoring the file:

  • Deleting the map's observeCommits binding turns all 7 delivery tests red, with an empty subscriber or an uncalled deliver. The two "delivers nothing" tests are negative controls and stay green.

  • Deleting the row-commit notification turns the 6 row tests red.

  • Deleting the epoch-adopt notification turns the epoch test red.

  • Deleting the replaced/dropped handle check turns those 2 tests red, because deliver is called.

  • Deleting the subscriber's cursor update after a batch turns both exactly-once tests red: each row arrives twice.

  • Deleting the caught-up check turns the provider-frame test red: the rows are read twice.

  • The readonly guard is enforced by the type checker. The old in-place swap in structured-agent-session-journal-handles.test.ts fails tsc with TS2540.

  • Base arm: with every source file restored to refactor(native-chat): remove the unused terminal handoff #22783's head, 4 tests are red:

    • the failed-start test;
    • the revision test;
    • the direct-append test;
    • the eviction-wait test.

    With the lease-release failure removed, the failed-start test is green at the base. So its red is exactly the missing delivery when the release fails.

Suites:

  • src/main/native-chat: all 176 files and 1478 tests pass after rebasing onto refactor(native-chat): remove the unused terminal handoff #22783's latest head (713b126feb). Two earlier full runs each hit one timeout under load (lease-renewer, then refusal-retry); each file passes alone.
  • src/main/claude, src/main/codex, src/main/runtime and src/main/ipc: pass. The only failures are the known local ones:
    • claude-structured-real-cli;
    • the shell-export case, which fails only when CODEX_LB_API_KEY is set.
  • tests/e2e/cross-version-wire: 64 of 65 pass. release-checkout.unit.test.ts times out while checking out the baseline release. That is disk and git time, unrelated to this change, and it passed on one of three runs.

Checks: pnpm tc:node, pnpm tc:web, oxlint on the changed files and check:code-quality:changed are clean.

Platforms: macOS locally.

  • The change is platform-neutral: in-process delivery on the execution host.
  • Over SSH, remote hosts run the same host code, so remote chats get the same fix.

AI Disclosure

Review

Agent skill upstream boundary

  • Not applicable, or this change follows docs/reference/agent-skill-sharing-upstream-boundary.md and copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

  • Mixed versions: host-internal. There is no new field or wire frame. Two things old clients can now observe:
    • They receive rows that were previously dropped, all using existing item and submission shapes.
    • An old client's held send reply, on a chat evicted mid-send, now carries the journaled unknown settlement instead of the pending result. The client already handles that state, and would have read it on its next subscribe.
  • Performance: one delivery per committed row. Counted with three open readers:
    • A streamed row plus its frame's publish rebuilds the timeline 4 times, the same as before this change. Without the caught-up check it was 7, because the frame's publish re-read the journal for every reader; at 5,000 rows that re-read cost about 0.9 ms per streamed chunk.
    • A turn-activity update now rebuilds nothing (it was 3).
    • A frame that writes three rows rebuilds 12 times instead of 4. These frames are much rarer than streamed chunks, and streamed text is folded into one row per 60 ms before it reaches the journal.
  • Follow-up, not in this PR: the Claude and Codex translators still call the sink's publish / tryPublish after their appends, and the resolved-append …AndPublish variants still exist.
    • For rows these are now redundant.
    • They still carry the slash-command catalog, which is not a journal row, and the frame's backpressure admission.
    • Removing them changes sink admission across both providers, so it belongs in its own change.

Author: @BrennanKB5

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

brennanb2025 and others added 19 commits September 24, 2026 23:36
No client ever called agentSession.requestHandoff or mounted the handoff
chrome. Delete the handoff coordinator, the terminal-owner runtime, the
proof write path and the unmounted UI. Keep agentSession.handoffStatus,
which released desktop clients read for worktree activation, and let
records an older build left mid handoff reconcile through the ordinary
restart and recovery paths.
Eviction now drains delivered events before quit's resume-offer snapshot. An
unbounded wait there sits ahead of the provider stop, so a sink whose journal
write stalls kept the child running until the step deadline aborted the
eviction. The offer is advisory: bound the drain and stop the child regardless.

Co-Authored-By: Claude <noreply@anthropic.com>
`claudeAuthEnvCarriedForward`, `isPathWithinDirectory` and
`queryWindowsProcessRowsFresh` lost their last caller with the handoff. The
fresh-scan tests now go through `queryWindowsProcessDescendants({ fresh: true })`,
the teardown path that still depends on that contract.

Co-Authored-By: Claude <noreply@anthropic.com>
Six comments still named the handoff coordinator, a handoff suspend, or a
terminal-owned session as live participants in the flows they describe.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
…ement

The removed restart handoff test pinned this branch; nothing else did.

Co-Authored-By: Claude <noreply@anthropic.com>
The handoff removal dropped the per-session queue from `handoffStatus`, so a
read landing mid-start reported the reservation (no owner) instead of the
settled chat owner, and shipped desktop clients blocked worktree activation on
it. The read is queued again, as it was before the removal.

Co-Authored-By: Claude <noreply@anthropic.com>
The gate only refused a write when a PTY had been bound to a chat session, and the
only code that ever bound one was the terminal handoff this branch removes. With it
gone, every admit/readmit returned "admitted" unconditionally, so the checks on the
renderer write path, the runtime controller backstop, terminal.send, agent prompts,
preview input and orchestration pointers, the refusal fields on terminal.send and
worker-start receipts, the plugin and CLI refusal copy, and the adopted-pane
orchestration routing could no longer run. Ordinary writes take the same path in
the same order as before.

Co-Authored-By: Claude <noreply@anthropic.com>
…alled

appendLegacyTranscriptMessages fed the terminal transcript catch-up and
proveClaudeTranscriptBranch backed the terminal owner's exit proof. Both lost
their last caller with the handoff. Their tests now go through the live entry
points instead: the roster bounds through the legacy import, the pinned-read and
growth tests through the ancestry replay the history window uses, and the marker
rules through the string proof in their own file rather than the session-file
resolver's.

Co-Authored-By: Claude <noreply@anthropic.com>
A send refused because the chat's owner is not settled showed "The session is
mid-handoff (<stage>)." in the composer. With the handoff gone, the stages that
reach it are a chat that is still starting, or one whose previous agent process
has not yet been confirmed stopped. The message now says which of the two it is.
The refusal code is unchanged.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
With the terminal handoff gone, the module named codex-tui-rollout-proof holds
only the pinned rollout lookup that structured Codex launches use to resume a
thread, so the name described code that no longer exists. Rename the module and
its options type. Also drop a mobile allowlist assertion that pinned the
removed agentSession.requestHandoff method, which no longer exists to allow.
The handoffStatus reply type still listed the terminal handoff's fields and
states (terminal placement, host label, proof retry, queued and waiting phases,
the to-terminal direction). No host writes them any more and the only client
reader parses the reply as unknown, so they described nothing. The reply on the
wire is unchanged.
…t decode

Nothing in this build writes a terminal owner (`runtimeKind: 'tui'`) or the
handoff's `preparing` / `old-owner-stopped` stages, but the in-memory types
still admitted them, so readers across the host kept branches for values no
path produces and the compiler could not point at them.

The store now validates the on-disk shape, which still accepts those values so
an older record is not quarantined, and maps them once while parsing:

- `preparing` and `old-owner-stopped` become `recovering`
- a `tui` lease becomes `native`; when it records a process it also becomes
  `conflicted`, the claim every build probes but never stops. A plain native
  owner would be stopped by restart recovery, here and in older builds.

Revisions are taken over the normalized state on both sides of every compare,
and the mapped record reaches disk with the store's first transaction, the
same way the tab-id backfill does.

The in-memory types narrow to what this build writes, and the branches that
existed only for the removed values go. Structured-worker identity keeps its
verdict for a former terminal owner by refusing a conflicted claim rather
than a non-native kind.
…ation

A reservation only ever names a native owner now, so the request no longer
carries a kind and the reserved lease records `native` directly. The attach
params keep `runtimeKind`: agentSession.ensure and create accept it, and the
operation fingerprint stored in the ledger covers it.
…at changes nothing else

Hiding a tab also committed the visibility index, so the no-op transaction
wrote the file even when its open-time revision was wrong. Committing the index
first leaves the pending rewrite as the only reason to write.
A journal write and its delivery to open readers were two calls, and some
writers made only the first. A failed start whose lease could not be handed
back, a provider revision with no frame behind it, and eviction's settlement
were all journaled without reaching an open chat.

A journal handle now reports every durable change, and the host's session map
binds that report to the session's readers when the handle is set. Writers no
longer publish what they append; the per-writer publish calls are deleted.
@brennanb2025
brennanb2025 force-pushed the brennanb2025/chat-journal-append-delivery branch from 59dd098 to 427dc8d Compare September 25, 2026 09:19

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No blocking issues — one minor hardening suggestion inline.

Reviewed changes — Reviewed the full 16-file diff at 59dd098 (2 commits) plus the surrounding journal store, host lifetime, subscriber, and send-settlement code.

  • One commit listener for every durable change — AgentSessionJournal.observeCommits fires from the row commit callback and the epoch adopt callback, so appends, revisions, and epoch replacements share one notification edge.
  • Delivery bound when a handle becomes a session's journal — the new StructuredAgentSessionConversations Map subclass binds that listener in set, coalesces a synchronous burst with a queued flag, and delivers one microtask later only if the map still holds that exact handle.
  • Explicit publishes after appends removed — 17 publish/ctx.publish() calls across turns, turns-prompt, restart-continuation, send-preparation, host-mutations, thread-goal, and conversation-command. Each was verified to follow a commit on the in-map handle, so the new delivery covers it. Publishes that republish non-journal state (options, rewind phase, fence, attach adoption) are deliberately kept.
  • Tests — host-level append-delivery tests (failed start with refused lease release, unpublished provider revision, direct append), conversation-delivery unit tests with replaced/dropped negative controls and a reader-failure case, and a retargeted surface-lifetime assertion for the eviction unknown settlement.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — incremental delta since the prior pullfrog review at 59dd098 (the branch was rebased; 4484e4c/59dd098 became 6dad39a/97bf3bb with identical content, then 427dc8d landed).

  • readonly journal invariant — StructuredAgentSessionHostSession.journal is now readonly with a comment, so a handle can only enter a session through the map's set, which is the only place delivery is bound. The one test that swapped a handle in place now builds a new session and calls sessions.set (structured-agent-session-journal-handles.test.ts:245). This closes the hardening gap from the prior review.
  • Exactly-once coverage — liveReader now counts batches, and two tests assert a provider-streamed row whose frame also publishes arrives exactly once, and a host append followed by an explicit publish emits a single batch (the second publish sends nothing).
  • Plain-Map binding test — a handle set through a Map-typed reference still binds delivery, pinning the fact that collaborators holding the map as Map keep the behavior.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Each commit now delivers itself, so the publish a provider frame still sends
afterwards found every reader caught up but still read rows and rebuilt the
timeline for each one. A caught-up reader now skips the read.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — incremental delta since the prior pullfrog review at 427dc8d (2 commits: 50d2e33, bd72b13).

  • Caught-up fast path — deliver now returns through a new emitCaughtUp helper without reading rows when the subscriber's cursor already equals the journal's. It guards on !journal.isReadOnly, so a read-only journal still takes its schema_unreadable reset; the other resets (epoch_changed, cursor_ahead, cursor_compacted) all imply cursors differ, so none is masked.
  • In-loop caught-up branch unified — the existing !advanced branch calls the same helper. Its batch cursor changes from page.window.nextCursor to subscriber.cursor, which are equal in that branch, so the checkpoint/activity/commands behavior is unchanged.
  • Coverage — the provider-frame exactly-once test now spies journal.readSince and asserts a single read, pinning that the frame's publish behind the commit's own delivery reads nothing. bd72b13 adds only an oxlint-disable SAFETY comment to the teardown test's fake-journal cast.

The prior review's one inline suggestion (make journal readonly so a handle can only enter through set) is addressed in 427dc8d; that thread is resolved. The delivery and append-delivery suites pass locally (11 tests).

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@brennanb2025

brennanb2025 commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor Author

Live Electron check of this PR. After: bd72b13662683c3e0bce0830705119a32bd31127 (current head; the full run was first done on 427dc8de833b772fd0795585ad7adeddb54f0593 with the same results). Before: f79eb9b88f97040d6260dd2ec8b5c382267b3500 (head of brennanb2025/remove-tui-handoff).

Setup: a hidden Orca dev build from a fresh profile, driven over the Chrome DevTools Protocol, with screenshots taken from that hidden window.
Claude was a stand-in command that fails at startup with claude: not signed in (rig) while a flag file exists, and runs the real Claude CLI once the flag is removed.

A. Failure row reaches the open chat: PASS in After. The Before build did not reproduce the gap. With the flag present, opening a Claude chat showed one "The provider stopped before it finished starting: … not signed in (rig)." row. Sending Reply with exactly: DEAD with the chat left open added exactly one more row within about 6 s, plus "Message was not sent." with Retry and the reason in red under the composer. Nothing had to be reopened. Each count matched the rows written to the chat's history on disk (read from a copy of the database).

Row counts, shown / written to disk:

  • After: 2 / 2 while the chat stayed open; 3 / 3 after switching workspace and back; 4 / 4 after reloading the window.
  • Before: exactly the same: 2 / 2, 3 / 3, 4 / 4.

Each reopen adds one row because reopening tries to start Claude again, and that start writes its own failure row. No row was ever missing or shown twice.

The Before build already showed these rows live. This setup's failure happens on a fresh chat whose session hand-back succeeds, and that path already notified the open chat before this PR. The cases this PR fixes (the session hand-back itself failing, a revision written with no frame after it, the settlement written on eviction) are not reached by this setup. So A shows no regression and that each row arrives exactly once, even though a row now has two notifiers (the new commit listener and the frame publish). It does not show the before/after difference.

After, while open:
A1 after

After, reopened:
A2 after

Before, while open:
A1 before

Before, reopened:
A2 before

B. Normal chat: PASS. With the flag removed, Reply with exactly: ONE and then List three fruits, one per line each showed the prompt once and the reply once, in order. I sampled the transcript every 100 ms and it never dropped back to empty. It stayed scrolled to the bottom, and the working indicator cleared when each reply finished.

B1

C. Compaction status row: PASS for delivery. This PR removed the explicit notifications that followed compaction, so this row now reaches the open chat only through the new commit listener. /compact wrote "Compacting conversation…" and then "Conversation compacted." to the chat's history, and the open chat received "Conversation compacted." exactly once without being reopened. It is shown inside the previous reply's collapsed "Worked for 1s" section, so the transcript looks unchanged until you expand it. The Before build shows it the same way, so this PR did not introduce that.

C1 collapsed

C1 expanded

D. Quit and relaunch with the chat open: PASS. I quit the app while the chat was open and relaunched it on the same profile. The chat came back as the active tab, and the full transcript appeared about 0.9 s after I attached. Watched every 150 ms after that, it never blanked, reset or duplicated. Claude resumed the same session, and Reply with exactly: AGAIN got AGAIN in about 2 s.

D1

Separately, an untouched "Terminal 1" tab disappears after reloading the window. This happens on both the Before (f79eb9b) and After (bd72b13) builds, from fresh profiles, with no chat open, so it is not from this PR.

@brennanb2025

brennanb2025 commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor Author

Review status: ready for review at bd72b13662

Two review rounds ran against this PR, with a pre-release checklist at the start and at the end, and a live Electron check. The first round found and fixed two problems. The second round found nothing. Both checklist passes are clean.

Fixed during review

  • 50d2e33099 (performance): each provider frame still publishes after the row it wrote, and that row has already been delivered by the new commit listener. That second publish re-read the history and rebuilt the timeline for every open view: 7 rebuilds per streamed chunk with three open views, against 4 before this PR, about 1–2 ms per chunk on a 5,000–10,000-row chat. A view that is already up to date now skips the read. Streaming is back to 4 rebuilds per chunk, and the extra publish now costs 0.02–0.09 ms. A test asserts one history read per row, and it fails with the check removed.
  • bd72b13662 (CI): a moved type cast in a test had no safety note, which failed check:code-quality:changed. The note is added and the check passes.

Functional correctness: correct. Every change to a chat's history goes through one commit step, and every commit now reaches the open views: new rows, revisions, deletions, send settlements and epoch replacements. Each row arrives once. It also arrives before the reply to the request that wrote it, but that ordering follows from the code; no test pins it. The review checked the new "already up to date" shortcut against every kind of write, each of which moves the history's position, so it cannot skip a change.

Architectural fit: the bug came from saving a row and telling open views being two separate steps at every writer. The PR removes that class of bug rather than adding another guard: the history's commit step reports every change, and the host's session map is the only way a history handle becomes a session's. Both review rounds considered binding at open time or in a single adoption function and found neither materially better.

Deviations from the usual shape (already in the body):

  • Delivery is bound once per history handle, not to one process-wide stream.
  • Views receive a batch read forward from their own position, not the written event itself.
  • Delivery happens once per row, not once per provider frame. Keeping one delivery per frame would need a timer, which lets replies overtake rows, or a second delivery mechanism.
  • Rows written during reattach before the handle enters the session map still rely on the reattach's own publish. That was true before this PR too.

Accepted minor effects:

  • Reopening a chat whose rewind was cut off by a crash now sends one extra reset before the reattach's own snapshot. Clients handle both the same way, and no rows are lost.
  • A frame that writes three rows sends three small batches.

Live check (details and screenshots in the comment above): normal replies, /compact, a quit and relaunch with the chat open, and the startup-failure row all pass on bd72b13662, with every row shown exactly once. The live rig could not reproduce the original missing row on the base build. Its failure takes a path that already published before this PR, and the three paths this PR fixes need a failure injected inside the host. Those paths are covered by the tests that read from a live subscriber, and those tests fail on the base.

Seen during the live check, not caused by this PR: each also happens on the base build.

  • After /compact, "Conversation compacted." is tucked inside the previous reply's collapsed work section.
  • Revisiting a chat whose agent fails at startup retries the start and adds another identical failure row. The planned "send starts the agent" change removes this.
  • A chat whose only message was not sent shows a green "done" check.
  • An untouched terminal tab disappeared after a window reload. In this check that was triggered by the QA rig's stand-in shell, which exits at startup. The underlying bug is real and already on main: a new terminal whose shell exits before anything is typed loses its tab on reload. It is being fixed separately.

Validation:

  • pnpm tc:node is clean.
  • src/main/native-chat: 176 of 176 files and 1478 of 1478 tests pass.
  • oxlint on the changed files is clean, and check:code-quality:changed passes.
  • The Claude, Codex, runtime and IPC suites show only failures that need a local signed-in CLI.
  • No mobile-facing surface or wire format changed. Remote hosts run the same host code.
  • Remaining gap: the live check ran only on macOS. The change is platform-neutral in-process delivery.

The live check ran on a Claude agent, because the planned Grok seat was out of quota.

…rnal-append-delivery

# Conflicts:
#	src/main/native-chat/agent-session-wire/structured-agent-session-subscribers.ts
#	src/main/runtime/agent-session-record-store-legacy-handoff-lease.test.ts
#	src/renderer/src/lib/worktree-agent-activation-gate.test.ts
@brennanb2025
brennanb2025 changed the base branch from brennanb2025/remove-tui-handoff to main September 25, 2026 18:30
@coderabbitai

coderabbitai Bot commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

📝 Walkthrough

Walkthrough

The host now observes journal commits and schedules delivery to session subscribers. Subscriber delivery skips catch-up reads when its cursor matches the writable journal cursor. Journal-writing paths no longer explicitly publish after appends; the publish contract now covers state held outside the journal.

Priority: ⬇️ Low

Merge Risk: 🟡 Moderate · up to 98ee7

A reader failure could produce an uncaught process error if error reporting also fails. Contain reporting failures before merging.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 98ee7

The change is limited to an experimental chat mode that is off by default. Session ownership checks and replay during journal replacement limit the apparent exposure, but some failure and authorization paths remain unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The directly affected delivery path is the host-managed structured-chat session, not a journal-wide broadcast: the host publishes by session ID, and the mode is off by default.

Trust Boundaries and Controls

  • observed — Before notifying subscribers, the observer checks that the session still owns the journal handle; this prevents a replaced handle’s queued notification from publishing through that session.

Resilience and Maintainability Implications

  • observed — Attach closes the previous journal before replacing its map entry, aborts replacement if that close fails, and replays or resets subscribers after a successful replacement.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 41.18% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 15 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: ensuring every native-chat journal append reaches open chats.
Description check ✅ Passed The description is detailed and covers scope, user impact, mechanism, rationale, testing, platform considerations, performance, and checklist status. The Linked Issue section does not provide the requ…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 84e9de58-37bc-41b0-b251-b91b27b08449

📥 Commits

Reviewing files that changed from the base of the PR and between 8009939 and 98ee74e.

📒 Files selected for processing (19)
  • src/main/native-chat/agent-session-journal/journal-store.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-append-delivery.test.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-conversations.test.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-conversations.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-host-mutations.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-host-types.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-host.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-journal-handles.test.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-restart-continuation.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-host.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-wiring.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-send-preparation.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-subscribers.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-surface-lifetime.test.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-thread-goal.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-turns-prompt.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-turns.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-unanswered-dispatch-release.test.ts
  • src/main/native-chat/agent-session-wire/structured-conversation-command.ts
💤 Files with no reviewable changes (4)
  • src/main/native-chat/agent-session-wire/structured-agent-session-restart-resume-host.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-turns-prompt.ts
  • src/main/native-chat/agent-session-wire/structured-agent-session-thread-goal.ts
  • src/main/native-chat/agent-session-wire/structured-conversation-command.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.

try {
this.delivery.deliver(sessionId, journal)
} catch (error) {
this.delivery.onDeliveryError(sessionId, error)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Contain failures from onDeliveryError.

If deliver throws and onDeliveryError also throws, the exception escapes the delivery microtask. A reader failure can then become an uncaught process error despite the durable write succeeding. Catch failures from the error handler within this microtask and report them through a non-throwing fallback.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Ready to merge. This is my view as coordinator of the native-chat redesign PRs.

  • Scope: matches the plan. Every journal append now reaches the chats that are open, exactly once. There is one binding point, per journal handle, and about 20 per-writer publishes are deleted. The provider-side sink publishes stay as a listed follow-up.
  • Review: it converged in two rounds. The first round fixed a streaming performance regression and a CI failure; the second came back clean. Readiness passed at start and end. Live QA passed, with every row shown exactly once across normal replies, /compact, quit and relaunch, and the startup-failure row.
  • CI: everything is green, including cross-version wire compatibility and all 8 test shards, except mobile web app bundle. That job fails on every branch that includes fix(mobile): the page's terminal shows its last rows above the command dock (OTA phase C follow-up) #22806 (c9d421f), and passes on every branch without it. Test: mobile-web-app-terminal-render.test.mjs › "takes back the frames it is owed, not only the timers". This PR changes only src/main/native-chat, so it's a regression on main that needs a separate fix, not a blocker for this PR.
  • Mergeable against main. It's based on main after refactor(native-chat): remove the unused terminal handoff #22783, and GitHub shows only this PR's 19 files.

@brennanb2025
brennanb2025 merged commit 600adba into main Sep 26, 2026
78 of 80 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant