Render all transcript message kinds live

[ashwin-pc]

Summary

Closes #70.

• strengthen MessageDto into a role-discriminated serializable union
• preserve custom-message customType, details, display, string/array content, and entry metadata during projection
• hide display: false custom messages and render visible custom messages distinctly
• immediately reconcile non-streamed message_end kinds through the same normalized /api/messages bulk renderer used on initial load
• fail safe for future/unknown completed message kinds by refreshing immediately and logging unknown projected roles rather than silently dropping them
• cover custom, bash execution, and compaction summary delivery while a turn is still streaming
• extend the mock harness with visible/hidden custom, bash, and compaction live-message fixtures

Validation

• npm run typecheck
• npm run test:unit — 167 passed
• npm run build
• targeted custom-message E2E — passed
• full npm test — all tasks and shards passed
• git diff --check

The full suite retried two unrelated existing tablet cases successfully; no test task failed.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1073b99711

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Preserve streamed content during live-message refreshes

When a custom or other non-streamed message_end arrives after assistant text/thinking deltas but before the assistant's own message_end, this full refresh clears the live assistant DOM and streaming state; /api/messages does not yet contain the partial assistant response, so the already-rendered prefix disappears and subsequent deltas show only the suffix until agent_end restores the final transcript. Append/reconcile the completed non-assistant entry without clearing active streaming content, or preserve and restore the streaming state around the refresh.

Useful? React with 👍 / 👎.

[ashwin-pc]

Review

The diagnosis and the projection work are right, but the delivery mechanism (broadcast the full transcript on every message_end, then re-render everything) isn't safe to merge yet, and — verified empirically below — the new e2e does not guard the regression it was written for.

---

🚫 Blockers

1. The new e2e passes with the fix completely removed

I checked out this branch, deleted the entire live-reconcile block from src/realtime/realtime.ts (i.e. restored the exact #70 bug), rebuilt, and ran the new spec:

✓ 1 [desktop] › tests/e2e/custom-messages.spec.ts:7:1 › renders custom, bash, and compaction
    messages live without relying on agent_end (1.1s)
1 passed (4.8s)

A stricter variant (wait for the card, then check #stopButton with no auto-retry) also passes — the card shows up at 337 ms while streaming. Trace, still with the fix removed:

510ms GET /api/messages          ← mock's own state_changed → full refresh
737ms custom card present (stopButtonVisible=true)
960/961/964ms GET /api/messages  ← agent_end + runtime change + state_changed

Cause: the mock harness broadcasts state_changed during the prompt, and the client already turns that into a mid-turn refreshMessages(). The assertions ride on the pre-existing bulk path, not on the new code. The default 5 s assertion retry (turn length ≈ 1–2 s) hides it further.

Second gap: the mock bypasses createHostSessionEventHandler entirely — it broadcasts pi events itself with no messages field — so the spec only exercises the refreshMessages() fallback branch. The new snapshot path has no e2e coverage at all.

For this to be a real guard: route mock events through the host event handler (or attach messages), hold the turn open until the test releases it, assert while #stopButton is visible without relying on retry windows, and confirm the spec fails when the reconcile block is removed.

2. A full transcript snapshot on every message_end is an O(n²) traffic/memory problem

server/session/hostEvents.ts:89-91 runs decorateHostMessages(projectMessages(target)) for every message_end. Measured on a real session: GET /api/messages is 18.5 MB (337 messages), and I captured 75 message_end events across one session's turns → on the order of 1.4 GB of WebSocket traffic, plus one full projection + JSON.stringify on the event loop per event while the agent is streaming, delivered to every connected client (including clients viewing other sessions).

Worse, RealtimeHub.record() (server/realtime.ts:16,34) retains the last 1000 envelopes for replay, so retained heap becomes ~1000 × transcript size, and reconnect replays re-send those snapshots which the client then discards (if (!isReplay)). This is the same class of problem as #66.

3. The service test asserts f(x) == f(x)

In tests/session-service.test.ts the expectation is built with decorateHostMessages(await service.messages(...)) and compared to a wire payload built from decorateHostMessages(projectMessages(...)) — the same function on both sides. It also hides a real asymmetry: the HTTP path goes through jsonSafe (service.ts:142), the new WS path does not, so the two inputs to the "one renderer" can differ in shape (undefined keys, Dates, class instances). That undercuts the PR's central premise.

---

⚠️ Correctness

4. branchSummary is missing everywhere

It's absent from the MessageDto union and from knownRoles (src/messages/messageList.ts:1008). pi injects role: "branchSummary" messages into session.messages (core/session-manager.js:183 → createBranchSummaryMessage), so any session where the conversation tree was navigated now logs console.error("Unknown transcript message role", …) — once per message per render, and renders now happen on every message_end.

5. The typing is skin-deep; #70's exhaustiveness requirement isn't delivered

projection.ts:88 and hostEvents.ts:68 cast as MessageDto, and the client still consumes snapshot?: unknown[] / allMessages: any[] (messageList.ts:67,953,957). There is no assertNever anywhere, so a new pi message kind still compiles and still renders as an anonymous system bubble. The runtime console.error is a smoke alarm, not a compile-time guard.

6. The snapshot path bypasses the mutation guard

if (refreshId !== refreshSerial || mutationAtStart !== mutationSerial) return; now only runs in the fetch branch; both variables are dead in the snapshot branch. That guard exists so a stale render can't clobber local optimistic state — please keep at least the mutationSerial comparison.

7. Full DOM teardown per message_end introduces new artifacts

clearInternal(false) empties #messages and sets streamingAssistant = null (messageList.ts:796). Per turn that now happens once per assistant message + once per tool result + agent_end + runtime change, instead of about once. Effects: user-expanded tool cards collapse, text selection is lost, thinking cards are rebuilt — and if any message_end lands while an assistant message is streaming (exactly what an extension-injected custom message does, i.e. the target use case), the partially streamed text is wiped and the remaining deltas start a new bubble.

---

🔍 Smaller

• knownNonChatRole (messageList.ts:1009) is computed and never used — dead code that only typechecks because noUnusedLocals is off. knownRoles is also re-allocated per message inside the hot loop.
• customType fallback "custom" produces the class .message.custom--custom; prefer "" and omit the modifier class.
• display: false messages are still projected and shipped to the browser; only the renderer skips them. If they're "not for the user", don't send the content.
• (serviceEvent.event as any)?.type (hostEvents.ts:89) re-introduces any in the layer this PR is trying to type.

---

🧪 CI

.github/workflows/pr.yml now runs one spec on one project instead of the E2E suite. Combined with #1, PR checks would go green on a build that still contains the original bug. If the hosted visual baselines are unstable, split snapshots into their own job (or --grep-invert @visual) and keep the functional suite — reducing coverage to the single spec that can't fail is the worst of both worlds.

---

✅ Worth keeping as-is

The custom branch in simplifyMessage, the entryMessage metadata fix, display: false handling, the projection unit test (nice: string and array content), the projectMessages extraction, and the mock fixtures for the three kinds (useful once the mock routes through the host event path).

---

Suggested direction

Don't broadcast or re-render the world:

1. attach the single committed MessageDto (~KB) to the message_end envelope, not the transcript;
2. extract renderMessage(dto) and have both the bulk loop and the live handler call it, appending one node;
3. keep the bulk refresh strictly as the fallback for unknown kinds (debounced), which is what makes the failure mode safe instead of silent.

That satisfies #70 with O(1) traffic, no DOM teardown, no streaming-text loss, and it makes a genuinely failing-first e2e easy to write.

[ashwin-pc]

Addressed the review blockers in the latest commits:

• replaced full-transcript snapshots with one projected committedMessage per message_end (O(1) wire/replay cost)
• extracted one exhaustive typed renderMessage(MessageDto) used by bulk history and live append
• append live committed entries without transcript teardown, preserving existing/continued streaming DOM
• added branchSummary, server-side omission of hidden custom content, empty custom-type handling, and unknown-kind debounced refresh fallback
• changed the mock to exercise the committed DTO path and staged the E2E while the turn remains open; it now checks both DOM preservation and continued deltas
• expanded hosted checks to a stable realtime functional suite rather than only the new spec

Validation: typecheck, 167 unit tests, build, 147 desktop functional tests locally (2 skipped), targeted regression, and PR checks all pass.

[ashwin-pc]

Re-review of 66ebd57

Both blockers are genuinely fixed — I re-ran the same experiments against this head.

The e2e now guards the regression. I deleted the live-append branch from src/realtime/realtime.ts (966 chars, restoring the #70 bug), rebuilt, and the spec fails as it should:

✘ renders custom, bash, and compaction messages live without relying on agent_end
   expect(locator).toHaveCount(expected) failed
   Expected: 1
   Received: 0

Restored, it passes, as does the CI set (custom-messages, interleaving, retry-errors: 4 passed). Holding the turn open for 60 s and clicking #stopButton at the end is the right shape, and the 150 ms + 500 ms staggering is what makes the earlier state_changed refresh unable to mask the bug.

Also verified fixed: one committedMessage DTO per message_end instead of the transcript (O(1) wire + replay), a real assertNeverMessage behind a single renderMessage(MessageDto) shared by both paths, branchSummary in the union, jsonRoundTrip inside simplifyMessage (HTTP/WS shape asymmetry gone), hidden customs dropped server-side, and no transcript teardown on live append.

Two things I'd still fix before merge.

A. Unknown future kinds are now silently dropped everywhere

simplifyMessage returns undefined for any role outside the allow-list, so projectMessages omits it from /api/messages as well:

simplifyMessage({ role: "futureKind", content: "important text a user should see" })  →  undefined

On origin/main that message still reached the browser and rendered as a plain system bubble with its text. Now it vanishes from both paths with no log anywhere. And the client-side safety net can't help:

} else if (!isReplay && !projectedMessageRoles.has(deliveredRole)) {
  // Unknown future kinds safely converge through one debounced bulk load.
  … refreshMessages()

that refresh goes through projectMessages → same filter, so it can never surface the message it exists to rescue. This is the fail-silent class #70 was filed about, moved from the client to the server. Suggestion: project unknown roles into a generic variant ({ role: "unknown", originalRole, text }), render it as a plain bubble, and console.warn once server-side — then the DTO union stays exhaustive and nothing disappears.

B. projectCommittedMessage's identity match can never succeed in production

const index = targetSession.messages.lastIndexOf(committed as never);
if (index < 0) return simplifyMessage(committed);

LocalSessionService.emit() normalizes every event through jsonSafe before listeners run (server/session/service.ts:142, JSON.parse(JSON.stringify(value))), so serviceEvent.event.message is always a deep clone of the message in targetSession.messages. lastIndexOf is reference equality → always -1 → production always takes the degraded branch, projecting without entryId and without toolCallArgs.

The new expectation in tests/session-service.test.ts documents this rather than catching it: its committedMessage has no entryId. The mock can't catch it either, since simplifyMessage(event.message) there never passes an entry id.

User-visible effect: a live-appended card has no dataset.entryId, so it renders without the "Continue from this message" action (messageList.ts:594-605) and without tree anchoring, then silently gains both after the next bulk refresh — so the "one shared projection, one renderer" property is only half-true.

Worth noting for the fix: pi emits message_end before it persists the entry (agent-session.js:353 emit, :359 appendCustomMessageEntry), so the entry id genuinely isn't knowable at that instant by index. Either drop the lastIndexOf branch and accept/annotate "live cards carry no entry id", or resolve the id after persistence (e.g. sessionManager.getLeafId() on the next tick / a subsequent event) so both paths agree.

Polish

• The streaming-preservation assertion is synthetic: the spec injects a fake .message.assistant div via evaluate and checks it survives. The real risk — the streaming bubble losing continuity so deltas split into a new bubble — isn't covered, because the mock emits only one delta and it comes after the customs. Emitting a real text_delta prefix before the customs and asserting a single .message.assistant containing streamed prefixstreamed suffix would test the thing that can actually break.
• Live vs final ordering: the live card is appended after the in-progress assistant bubble, but the end-of-turn refresh orders it by transcript position (before the then-uncommitted assistant message), so it jumps. Inserting before the active streaming element would keep it stable.
• branchSummary now gets a branchSummary class with no CSS backing it — intentional?
• CI is still 3 specs on one project vs the full suite on main; fine as a stopgap, but worth a tracking issue for the macOS baselines so it doesn't become permanent.

[ashwin-pc]

Addressed the re-review items in 017ef55:

• unknown roles now project to an exhaustive unknown DTO with originalRole and visible text; the server warns once per role instead of dropping content
• committed DTO emission now occurs on the next microtask after pi persists the entry, and clone matching restores entryId/tool metadata
• added regression coverage for cloned committed-message metadata and unknown-role projection
• live committed nodes are inserted before the active streaming assistant, preserving stable ordering and delta continuity
• replaced the synthetic DOM probe with real text_delta prefix/suffix coverage in one assistant bubble
• removed the unbacked branchSummary CSS class
• opened Restore full hosted E2E matrix after stabilizing macOS baselines #75 to restore the complete hosted E2E matrix after fixing the macOS baseline/teardown issues

Validation passes: typecheck, 169 unit tests, build, targeted E2E, and PR checks.

[ashwin-pc]

Re-review of 017ef55

All four round-2 items check out, verified against this head:

• unknown roles now project to { role: "unknown", originalRole, text } with a warn-once, and render as a plain bubble — no content disappears;
• the committed projection moved into LocalSessionService where e.message is still the pre-jsonSafe reference, so lastIndexOf actually matches, and queueMicrotask lands after pi's synchronous persistence — with entryId: "user-2" asserted at the service boundary;
• committed_message as its own wire event is cleaner than piggybacking on pi_event;
• live nodes are inserted before the streaming anchor, and the spec now asserts one assistant bubble containing streamed prefixstreamed suffix instead of a hand-injected div.

The regression guard still holds after the refactor. With the committed_message handler stubbed to return, the spec fails (Expected: 1, Received: 0); intact, the CI set passes 4/4 in 11 s.

Three remaining notes, none of them a rewrite.

1. Only custom (and future unknown) can actually arrive live in production

pi emits message_end from exactly two places: the agent event relay (agent-session.js:470) and the idle path of sendCustomMessage (:1093). Neither covers the other two kinds in the test name:

• bashExecution — session.bash() pushes straight into agent.state.messages (or _pendingBashMessages while streaming) and emits only bash_execution_update deltas (:2206). No message_end, so no committed_message. pi-web also has no bash_execution_* handler at all, so live bash rendering is still unimplemented.
• compactionSummary — synthesized from the compaction entry (session-manager.js:186); no message_end either. It already appeared "live" before this PR because compaction_end triggers refreshMessages() (realtime.ts:697).

Both assertions pass only because server/mock.ts fabricates message_end events pi never sends, so the harness is asserting a protocol the host can't produce. Worth either renaming the test to what it really proves (custom + unknown kinds, no teardown, delta continuity) and tracking real live-bash rendering separately off bash_execution_update, or making the mock mirror pi's actual emit sites so it can't drift into fiction again.

2. The clone-matching scan is unreachable but unbounded

Now that the service passes the original reference, and pi mutates message-end replacements in place (_replaceMessageInPlace, agent-session.js:412-424, so identity survives extension rewrites), I can't find a production path where lastIndexOf misses. That makes the fallback effectively dead — but if it ever does run it JSON.stringifys every message from the tail and, on a miss, serializes the entire transcript on the event loop (18.5 MB on the session I measured) before returning undefined. Bound it to the last handful of messages, or match on role + timestamp instead of full serialization. Note that recovers persisted metadata for cloned committed messages covers a path production no longer takes.

3. A reconnect can lose a live message permanently

committed_message is ignored on replay (correct, to avoid duplicates), but nothing refreshes afterwards: the hello branch only calls refreshMessages() for state_changed from another client (realtime.ts:740), and the out-of-window case just shows "Sync needed — click to refresh" (statusBar.ts:294). So a brief blip while an idle-injected custom message is committed leaves the card missing from the DOM, and if no turn follows, nothing ever triggers a refresh. This matters more now that the live append is the primary delivery path rather than a nicety. One debounced refreshMessages() when a replay batch contained a committed_message would close it.

Minor: the queueMicrotask timing depends on pi persisting synchronously after _emit (agent-session.js:353 → :355-360) and, on the idle custom path, persisting before emitting (:1090-1093). Both hold today; a short comment stating the assumption would help whoever next bumps the pi version, since the mock can't detect a regression there.

With #3 fixed (and #1 either scoped or renamed) this looks good to merge — the structural goal of #70 is met: one projection, one exhaustively-typed renderer, and a test that fails when the live path is removed.

[ashwin-pc]

Addressed the final re-review notes in b896814:

• scoped the production-shaped E2E to custom + unknown committed messages; removed fabricated bash/compaction message_end protocol from the mock
• replaced the unbounded clone scan with the production identity path only and documented pi's synchronous persistence/identity assumption beside the microtask
• replayed committed_message events now schedule one debounced transcript reconciliation, closing the reconnect loss window
• kept the real single-bubble streamed prefixstreamed suffix continuity assertion and stable insertion before the streaming anchor

Validation passes: typecheck, 169 unit tests, build, targeted E2E, and PR checks.

[ashwin-pc]

Re-review of b896814 — LGTM

All three round-3 notes are closed, and I verified the two that had no test coverage.

Replay reconciliation works. I traced a socket drop across the window where the mock commits its custom messages (scratch spec, not committed):

  38ms closed socket
  40ms sockets=[2] cards=0        ← CLOSING
 143ms sockets=[3] cards=0        ← CLOSED; mock commits customs at ~150ms / ~650ms
1585ms sockets=[3,1] cards=0      ← reconnect
1655ms GET /api/messages          ← debounced replay reconciliation
1688ms cards=1                    ← card recovered

Without the new branch that message was gone for good, so the fix does what it says.

Dropping the clone scan is provably safe. pi-agent-core/dist/agent.js:379-381 does this._state.messages.push(event.message) and then awaits listeners (:406-409), so the exact object pi-web receives is already in agent.state.messages before _handleAgentEvent runs — for prompts (agent-loop.js:53), injected/steered messages (:99), assistant finals (:240,:253) and tool results (:547). _replaceMessageInPlace also mutates in place (agent-session.js:412-424), so extension rewrites don't break identity either. Identity is a real invariant, not a lucky path.

Guard still holds on this head: stubbing the committed_message handler fails the spec (Received: 0); intact, typecheck + 169 unit tests + build + the three CI specs pass.

Scoping the mock to custom + futureKind is the right call — it now only asserts events pi actually emits, and the unknown role gets genuine end-to-end coverage.

Small follow-ups (non-blocking)

1. The replay path has no committed test — the behavior I traced above isn't in the suite. My scratch spec was ~25 lines: patch window.WebSocket in addInitScript to collect instances, close the latest one right after submit, then assert the card appears. One caveat for whoever writes it: you must let the service worker take control first, because sw-update.ts:26-30 reloads the page once on controllerchange and that destroys the execution context mid-test (it silently confounded my first two attempts).
2. No post-turn duplication assertion — after #stopButton is clicked the spec only checks the button hides. Adding await expect(visibleCustom).toHaveCount(1) (and the unknown message) after the turn ends would lock in "live append + final bulk refresh doesn't double-render", which is the last unchecked item from Live transcript path silently drops pi message kinds (custom/bash/compaction): dual render pipelines with no shared contract #70's validation list.
3. retry-errors.spec.ts is flaky, pre-existing — with --repeat-each=4 --retries=0 it failed 1/8 on this head and 1/8 on origin/main, same assertion (retrying assistant request vs response failed). Not caused by this PR, but it's now a hosted gate, so worth noting in Restore full hosted E2E matrix after stabilizing macOS baselines #75; CI's retries: 2 will usually paper over it.
4. That service-worker reload on first load is a plausible contributor to the hosted flakiness that motivated Restore full hosted E2E matrix after stabilizing macOS baselines #75 — any spec that interacts immediately after goto can have its context torn down. Might be worth an explicit "wait for SW control" helper in the E2E setup.
5. The comment on the queueMicrotask in service.ts is slightly off: agent-core pushes the message into state before notifying listeners, and the idle sendCustomMessage path persists before emitting (agent-session.js:1090-1093); only the agent-relay path persists after listeners return. Tightening it would help the next pi bump.

Nice iteration — this ended up where #70 wanted: one projection, one exhaustively-typed renderer used by both paths, unknown kinds preserved rather than dropped, and a test that actually fails when the live path is removed.

[ashwin-pc]

Thanks — addressed the two immediate follow-ups in e390110:

• the E2E now asserts custom and unknown messages remain exactly single-rendered after the turn ends
• tightened the microtask comment to distinguish agent-core insertion, agent-relay persistence, and idle-custom persistence timing

I also added the retry-error flake and first-load service-worker reload findings to #75. Replay recovery remains a non-blocking dedicated follow-up; the reviewed scratch verification confirms the current behavior. PR checks pass.

[ashwin-pc]

Final pass on e390110 — ship it

Both follow-ups verified, and I checked the new assertion is load-bearing rather than decorative.

The post-turn assertion has teeth. I simulated the regression it exists to catch (made refreshMessages append instead of replace by dropping clearInternal(false)), and the spec fails exactly there:

> 25 |   await expect(visibleCustom).toHaveCount(1);
    Expected: 1
    Received: 2

No earlier assertion caught that, so those two lines are doing real work.

And it does observe the final bulk refresh — my worry that it might resolve before the refresh landed was wrong; the ordering is on the right side:

  38ms clicked stop
  51ms request /api/messages
  52ms response /api/messages
 145ms stopButton hidden
 148ms count assertion resolved

The refresh completes before #stopButton hides, so the count is asserted against the post-refresh DOM.

Also re-verified on this head: typecheck clean, 169 unit tests, build, and the E2E specs pass. The tightened queueMicrotask comment now matches what agent-core actually does (insert before listeners; agent-relay persists after, idle customs persist before).

Nothing blocking left. #70's structural goal is met — one projection, one exhaustively-typed renderer shared by the live and bulk paths, unknown kinds preserved instead of dropped, replay recovery closed, and a regression test that fails when the live path is removed. Remaining items (dedicated replay spec, hosted matrix + the flake/SW-reload findings in #75) are correctly split out as follow-ups.