[brennanb2025]

	Files	Added	Deleted	Net
Test	4	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​330	$\color{#cf222e}{\Huge{\mathbf{−}}}$​368	$\color{#cf222e}{\Huge{\mathbf{−}}}$​38
Prod	4	$\color{#1a7f37}{\Huge{\mathbf{+}}}$​27	$\color{#cf222e}{\Huge{\mathbf{−}}}$​120	$\color{#cf222e}{\Huge{\mathbf{−}}}$​93

Symptom

A structured native-chat pane could latch Working for N forever after the agent had finished, showing the send arrow instead of Stop. The sidebar and worktree ps read idle at the same time, so the pane disagreed with the rest of the app.

Mechanism

No frame is lost and nothing is dropped in transit. This is a self-inflicted cursor jump inside the client replica.

The replica has one position (state.cursor) and one body (state.items). Two operations keep those consistent:

• replace — position and body both taken from one host snapshot
• append — rows contiguous with the current cursor

tail-page was a third operation: it took the cursor from the journal head but the items from a bounded page (200 items, byte-capped), then merged retained client submissions over the page's. Under continuous journal writes the client is always slightly behind the head, so this branch ran on every window focus and on every pane re-activation.

When more than a page of rows had landed since a send, that send's user item fell off the page, its submission was not carried across the merge, the retained pending survived, and the cursor jumped past the dispatch-acceptance row. Nothing re-sends it: a batch carries only touched items, and that submission is never touched again.

The change

Delete the third operation rather than guard it. A live subscription becomes the only thing that moves the cursor, and subscribe({ cursor }) already replays exactly the rows the client missed.

• remove the window focus listener and the owner/transport refresh contract
• skip warm hydration — a retained owner subscribes at its applied cursor
• cold hydration keeps its history read, applied as the existing snapshot (replace) event rather than tail-page
• delete the tail-page action and its reducer branch
• delete resumeCursor and shouldAdvanceStructuredResumeCursor — two cursors with two advancement rules were how position and body drifted apart

This also removes a second cost of the same branch: a focus during a busy turn discarded paged-in older items, shrinking the transcript to a single bounded page mid-turn.

Unchanged

older-page / loadOlder, the unattached-refusal grace, the generation guards and the event coalescer are all unchanged. There is no host, wire or schema change — every touched file is renderer or shared code plus tests.

What this does NOT fix

Deliberately out of scope here; these are PR2:

• fix(native-chat): let the provider reopen a Claude turn it resumed itself #20518's provider-reopen defect — a separate, still-open issue.
• Host page symmetry for a still-unsettled send whose user item is off a cold bounded page.
• A host-dropped subscriber with no observable close — the emit()/deliver() cursor hazard.
• afterSequence gap detection.

Validation

Unit tests run on this commit:

src/shared/structured-agent-session-reducer.test.ts
src/renderer/src/components/native-chat/structured-agent-session-read-transport.test.ts
src/renderer/src/components/native-chat/use-structured-agent-session-read.test.tsx
tests/e2e/structured-agent-session-read-owner.unit.test.ts

Test Files  4 passed (4)
     Tests  29 passed (29)

tests/e2e/**/*.unit.test.ts is in the include list in config/vitest.config.ts, so the new owner test runs as a normal unit gate rather than as an e2e-only test.

Live Electron validation

Full write-up in the follow-up comment. Stated precisely:

The mechanism is measurably gone. agentSession.history calls were counted in the main process (the renderer's window.api.runtime is frozen and cannot be instrumented from the page), same script and same isolated dev instance on both arms:

arm	commit	focus events	agentSession.history calls
before	0c812843ef	20	20
after	c80fa5f52f	20	0

On the fixed build, 172 focus events during a live turn produced 0 history calls.

The latch symptom was NOT reproduced on the base commit, so no screenshot here shows the bug being fixed. 1230 focus events plus a send left pending across 271 journal rows (past the 200-item page cap) still settled correctly — most likely because the deleted branch only runs when the client is behind the journal head, and the measured write rate on that host was only ~2 rows/sec.

The symptom fix therefore rests on the unit regression test (which fails with all four production files at origin/main and passes here) plus the measured removal of the read path, not on a live before/after of the symptom itself.

[coderabbitai]

📝 Walkthrough

Walkthrough

The read owner and transport replace refresh-tail behavior with conditional hydration. The transport now subscribes with the current applied cursor and no longer exposes refresh. The reducer removes the tail-page action and resume-cursor helper. The read hook no longer refreshes on window focus. Tests cover reconnect cursor handling, focus behavior, retained snapshots, missed rows, dispatch acceptance, and final status.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to c80fa

No concrete behavior risk remains; the optional test cleanup does not block merge.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 5 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check	⚠️ Warning	The description gives a detailed, relevant explanation of the symptom, mechanism, implementation, scope, and validation. However, it omits the required linked issue, does not provide the required Visu…	Add the required issue reference after “Fixes #”. Add a Visual Proof section with N/A and the reason no visual proof applies, or attach before/after evidence. Complete the required testing, review, boundary, notes, and checklist sections,…

✅ Passed checks (3 passed)

Check name	Status	Explanation
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.
Title check	✅ Passed	The title clearly describes the main change: preventing bounded tail reads from advancing the native-chat cursor beyond unapplied rows.

Full details: Description check

Explanation

The description gives a detailed, relevant explanation of the symptom, mechanism, implementation, scope, and validation. However, it omits the required linked issue, does not provide the required Visual Proof section with an explicit N/A statement, and leaves the required checklist and review sections incomplete.

Resolution

Add the required issue reference after “Fixes #”. Add a Visual Proof section with N/A and the reason no visual proof applies, or attach before/after evidence. Complete the required testing, review, boundary, notes, and checklist sections, including the applicable checkbox selections and CI status.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (1)

tests/e2e/structured-agent-session-read-owner.unit.test.ts (1)

147-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Remove the unreachable warm-hydration scaffolding.

The owner passes hydrate only when state.epoch === null, so the re-activation at Line 210 performs no agentSession.history call. Lines 192 and 240 both pin mocks.call at 1, which proves the warm branches never run: the limit: 1 override never applies, and delayedOlder.promise is never returned and never resolved.

Delete delayedOlder, the warm flag, and the comment at Line 150. They suggest coverage of a bounded warm page and a delayed older-page fill that this test does not exercise.

♻️ Proposed simplification

-      const delayedOlder = Promise.withResolvers<AgentSessionHistoryResult>()
-      let warm = false
       mocks.call.mockImplementation((_target, _method, request: AgentSessionHistoryRequest) => {
-        // Hold the measured bounded page before its asynchronous older-page fill can mask it.
-        if (warm && missedRows === 40 && request.direction === 'before') {
-          return delayedOlder.promise
-        }
-        const result = readAgentSessionHistory(journal, {
-          ...request,
-          ...(warm && missedRows === 40 ? { limit: 1 } : {})
-        })
+        const result = readAgentSessionHistory(journal, request)

Also drop warm = true at Line 209 and the now-unused AgentSessionHistoryResult type import if nothing else references it.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: 2758c3cc-3bee-460b-8d0a-dd9b507064c8

📥 Commits

Reviewing files that changed from the base of the PR and between 0c81284 and c80fa5f.

📒 Files selected for processing (8)

• src/renderer/src/components/native-chat/structured-agent-session-read-owner.ts
• src/renderer/src/components/native-chat/structured-agent-session-read-transport.test.ts
• src/renderer/src/components/native-chat/structured-agent-session-read-transport.ts
• src/renderer/src/components/native-chat/use-structured-agent-session-read.test.tsx
• src/renderer/src/components/native-chat/use-structured-agent-session-read.ts
• src/shared/structured-agent-session-reducer.test.ts
• src/shared/structured-agent-session-reducer.ts
• tests/e2e/structured-agent-session-read-owner.unit.test.ts

💤 Files with no reviewable changes (3)

• src/shared/structured-agent-session-reducer.ts
• src/shared/structured-agent-session-reducer.test.ts
• src/renderer/src/components/native-chat/use-structured-agent-session-read.ts

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

[pullfrog]

✅ No new issues found.

Reviewed changes — the whole diff at c80fa5f52f, read end-to-end against the host subscribe/replay path:

• Deletes the tail-page operation. Removes the action, its reducer branch, and shouldAdvanceStructuredResumeCursor so snapshot/reset/batch are the only state transitions. No in-repo consumer referenced either symbol.
• Warm re-entry re-subscribes at the applied cursor. structured-agent-session-read-owner.ts:187 passes hydrate only when epoch === null; a retained owner starts a live subscription instead of a bounded history read.
• Transport reads args.getCursor() at open time. structured-agent-session-read-transport.ts:128-145 flushes the coalescer, then reads the reducer-applied cursor, replacing the separately-tracked resumeCursor; refresh is gone from the transport/owner contract.
• Drops the focus listener (use-structured-agent-session-read.ts) and updates unit tests accordingly.
• Adds an owner-level regression test (tests/e2e/structured-agent-session-read-owner.unit.test.ts) plus a transport reconnect-flush test; deletes reducer tests that only exercised the removed tail-page behavior.

I verified warm re-subscribe is equivalent to the old refresh for page-only fields: host.subscribe routes through the background-task channel which fills backgroundTasks and handoff at open, and deliver(..., emitCheckpoint=true, ...) emits a checkpoint batch carrying those plus activity/commands/fence even when no rows advanced. So dropping the warm history read does not stale background-task state. I also confirmed coalescer.flush() in open() runs before the generation bump, so queued batches from the still-current generation apply before the cursor is read — the new test pins exactly that. The four affected test files pass (29 tests), pnpm tc:web is clean, and the changed-code quality gate reports 0 new findings.

ℹ️ Focus removal also drops an incidental resync for a silently-dropped subscriber

AgentSessionSubscribers.emit catches an emit failure and drops the subscriber without sending end, so the client transport stays connected = true with no reconnect. The window focus handler used to be a client-side resync trigger for that state; it is now gone. This is the "host-dropped subscriber with no observable close" hazard you already list as PR2, so no change is requested here — flagging only that this PR removes the last client-side heal for it.

Technical details

# Silent host-side subscriber drop now has no client resync path

## Affected sites
- `src/main/native-chat/agent-session-wire/structured-agent-session-subscribers.ts:308-319` — `emit()` catches, calls `drop()`, and never notifies the client.
- `src/renderer/src/components/native-chat/use-structured-agent-session-read.ts` — the removed `focus` listener was the only path that re-read history on regaining focus.
- `src/renderer/src/components/native-chat/structured-agent-session-read-transport.ts:119-186` — `open()` only reconnects on `end`/error/close; `connected` is never reset for a silent drop.

## Required outcome
None for this PR. Confirms the PR body's claim that the `emit()`/`deliver()` cursor hazard is deferred to PR2 — and notes that PR2 must supply the recovery path, since focus no longer does.

## Suggested approach (optional)
Leave as-is; track under PR2.

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}

[brennanb2025]

Live Electron validation — before/after

Both arms ran on the same isolated dev instance (own profile, private renderer port, own out/electron-dev binary, ownership re-proved on each attach), same seeded workspace, same structured Claude session, same script. Driven only over CDP.

Structured session confirmed in both arms (textbox "Send a message…" + model picker, tab "Claude Chat") — not a PTY chat that merely looks structured.

The headline result: the mechanism is measurably gone

agentSession.history calls were counted in the main process, by wrapping the runtime:call IPC handler (the renderer's window.api.runtime is frozen and cannot be instrumented from the page). Identical script, pane visible and document.hasFocus() === true in both arms:

arm	commit	focus events dispatched	agentSession.history calls
before	0c812843ef	20	20
after	c80fa5f52f	20	0

Verbatim:

BEFORE  baseline {"hist":0,"all":0} -> 20 focus -> {"hist":20,"all":20}
        sampleMethods: ["agentSession.history", x8 ...]
AFTER   baseline {"hist":0,"all":1} -> 20 focus -> {"hist":0,"all":1}

Stronger follow-up on the fixed build: 172 focus events fired during a live turn produced 0 history calls, and the only runtime:call methods seen were session.tabs.activate, agentSession.send, orchestration.workerTerminalUserInput, agentSession.options. The served use-structured-agent-session-read.ts module also contains zero addEventListener / focus / refresh references.

Every send was verified to actually land by asserting journal row growth (submission + dispatch rows in the session's SQLite journal), not by appearance.

What I could NOT do: I did not reproduce the latch

I was unable to reproduce the "Working for N" latch on 0c812843ef, so the after-arm screenshot does not prove the symptom is fixed, and the before-arm screenshot below is not a picture of the bug — it shows the session settling correctly.

What I drove on the before build: a real structured Claude session, 1230 dispatched focus events, and a queued send (QUEUED-PROBE-ALPHA) left pending across 271 journal rows — past the AGENT_SESSION_HISTORY_MAX_LIMIT = 200 page cap. It settled correctly every time.

The likely reason is visible in the branch being deleted: its destructive replace only runs when the client is behind the journal head, and the measured journal write rate on this host was only ~2 rows/sec, so the client never lagged and the early-return guard hit instead. Reproducing the latch plausibly needs a much higher write rate or a slower/contended renderer, which I did not induce.

So: the deleted read path is proven gone; the user-visible latch itself remains unreproduced here.

Screenshots (full window, uncropped)

Before — 0c812843ef, session settled correctly (not the bug):

After — c80fa5f52f, send under load lands and settles (SETTLED, send arrow, no stale "Working"):

Still unverified

• The latch symptom itself (not reproduced, above).
• Remote/SSH execution hosts — local host only.
• Codex and other providers — Claude only.
• Cold-start hydration path and loadOlder paging were not exercised deliberately.
• The four items listed as out of scope in the PR description remain untested here.

[brennanb2025]

Electron validation (this worktree)

Hidden CDP launch of brennanb2025/nc-fix-tailread (ORCA_BACKGROUND_LAUNCH=1, isolated profile, renderer :5176, CDP :9336). window.api.app.getIdentity() matched this worktree:

• name: Orca: brennanb2025/nc-fix-tailread
• devRepoRoot: /Users/brennanbenson/orca/workspaces/orca/nc-fix-tailread

Opened a real Claude Chat structured native-chat pane (New tab → Claude). Empty composer rendered; applied read-owner snapshot was status: ready, cursor.sequence: 1, fence: 1, items: 0. Renderer focus/visibilitychange did not move the cursor.

Empty isolated app:

Structured native chat (visible surface):

Not tested live: the original latch (pending send off a 200-item tail page), a busy-turn subscribe-append, loadOlder, remote/SSH, or Grok (Grok is not a structured-session provider). Console had one worker-induced 404 from a probe import; no product native-chat errors.

[brennanb2025]

Review status — land after fixes

The delegated review loop is clean: two same-model adversarial passes found no remaining proven P0, P1, or P2 issues, and the final readiness checklist also passed.

One meaningful, in-scope compatibility issue was fixed and pushed in 29b83a8e80: cold history hydration now applies through a dedicated reducer history-page action, preserving an optional mixed-version fence as null instead of inventing fence 0. That keeps writes unavailable until a real host fence arrives and preserves the cursor/body atomicity that fixes the original bounded-tail cursor bug. A regression test covers the missing-fence case. No unrelated scope was added.

Validation:

• Targeted native-chat/reducer/owner Vitest: 4 files, 30 tests passed.
• ORCA_BACKGROUND_LAUNCH=1 pnpm tc passed.
• Changed-code quality: 0 new findings.
• Mobile typecheck passed; mobile test suite: 576 files, 4,947 passed, 6 skipped.
• git diff --check passed.
• Final readiness checklist: PASS, with no proven P0/P1/P2 findings.

Electron evidence from the exact PR worktree (hidden CDP launch, isolated profile, real Claude structured native-chat pane):

• Empty app identity
• Structured native chat

The live Electron run confirmed the applied reader state and that focus/visibility changes did not move the cursor. The original latch was not reproduced live; the regression is covered by unit tests. Remote/SSH execution, loadOlder, and the host-dropped-subscriber/no-close case remain unverified or explicitly out of scope.

[pullfrog]

✅ No new issues found.

Reviewed changes — the delta since the prior pullfrog review (c80fa5f52f → 29b83a8e80), which restores the pre-PR fence semantics on the cold history-hydration path:

• Cold hydration no longer invents a writable fence. structured-agent-session-read-owner.ts:121 applies a new history-page action instead of a snapshot event, so a mixed-version page with no fence leaves state.fence at null rather than 0, matching the deleted tail-page branch's page.fence ?? null. Because state.fence === null disables structured sends (use-structured-agent-session-mutate.ts:42) and skips the fence filter in hasUnansweredStructuredAgentSessionDispatch, the first commit's ?? 0 had briefly made a fence-less page read as writable.
• New history-page reducer action. structured-agent-session-reducer.ts:48,168 adds the action and routes it through replacePage with action.page.fence ?? null, widening replacePage's fence param to number | null — the snapshot event's fence is a required number, which is why the event path could not express an absent fence.
• Regression test. use-structured-agent-session-read.test.tsx:126 pins state.fence === null for a page without a fence; it fails at c80fa5f52f (fence 0) and passes here.

I confirmed the new branch reproduces base's field set for the cold path, reran the four affected suites (30 tests), and pnpm tc:web plus check:code-quality:changed are clean.

  ^{｜ View workflow run ｜ Using DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏}