fix(runtime-host): forward the durable steering echo to session subscribers - #3316
fix(runtime-host): forward the durable steering echo to session subscribers#3316me2seeks wants to merge 3 commits into
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
APPROVE.
The fix is at the right seam. The root cause is stated precisely and the change addresses it rather than the symptom: the pump now forwards the durable steering_message, so rendering no longer depends on a subscriber happening to observe a transient in_flight queue state that the coalesced canonical refresh is free to skip.
I checked the two things that could have made this wrong, and neither does:
- The transient classification does not affect persistence. Adding
steering_messagetoisRuntimeSessionTransientEventlooked like it might drop the message from durable storage. It cannot::2099is the predicate's only call site, and it gatescontinuity.acceptRuntimeEvent— live forwarding — while persistence happens upstream in the run's own event stream. Only the name is now misleading, which is the P3 inline. - The mixed-version story holds without an epoch bump.
SESSION_CONTINUITY_SCHEMA_VERSIONgoes 4 to 5, and the check lives insidedecodeSessionContinuitySnapshot(session-continuity.ts:521), which thesubscription.openresult decode runs — so a schema-4 client fails at open and never reaches a frame carrying the new variant. The precedent is real and recent: #3074 took 3 to 4 the same way without touching the epoch. Worth stating explicitly since #3236, #3199, #3133 and #2521 are currently all contending for epoch 28 — this PR correctly stays out of that.
The dedup is the part I looked at hardest and it holds up. #renderedSteeringMessageIds is set on all three render paths — the rejoin seed at :154, the newly-in-flight synthesis at :356, and the forwarded echo at :329 — and the tests pin the race in both orders, plus the rejoin case, plus the original bug's shape (in-flight state never observed). The protocol test covers a round trip, the toolUseId rejection and the oversize-content rejection. The full-stack test in execution-host-message.test.ts asserts against a real host with a subscribed client rather than an intermediate object.
biome format is clean on all eight files at this head.
One P3 inline, purely naming. Nothing blocking.
AI disclosure: this review was assisted by Claude (Opus) for code search and cross-checking. Everything above I re-derived myself against the source at 18e2905f9.
…class isRuntimeSessionForwardedEvent / RuntimeSessionForwardedEvent describe what the predicate decides — forward live to subscribers — now that the durable steering_message belongs to it. Rename the client projectSessionEvent to match its Host-side counterpart. Addresses review on apache#3316.
|
The #3277 ( My approval stands — the diff I reviewed is unaffected. Written with Claude Code; the CI log, #3277's diff and the current |
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed the delta from 18e2905f9 — one commit, three files, a pure rename. It is the right response to the naming concern and I have no objection to it.
RuntimeSessionTransientEvent → RuntimeSessionForwardedEvent and isRuntimeSessionTransientEvent → isRuntimeSessionForwardedEvent make the predicate say what it decides instead of implying a durability class it never controlled, and the new comment states the actual rule — forward live to subscribers via the continuity coordinator rather than letting the canonical refresh carry it, with persistence orthogonal and handled upstream in the run's own event stream. That is exactly the distinction that made steering_message's membership look wrong under the old name, and it is now settled by the name rather than by having to re-derive it. projectToolEvent → projectSessionEvent in the projector is the same correction one layer down; the function has never been tool-specific.
I checked the rename is complete rather than partial: zero occurrences of either old identifier remain in the three touched files at this head, and the two module-local projectSessionEvent functions live in different modules at different layers, so there is no shadowing.
The one blocker is unchanged and is now inside this PR's scope — see the thread below. My earlier approval was dismissed by the new commit, so this is a fresh review; the P1 is the only thing keeping it a COMMENT.
Worth flagging separately: CI has not run on this head at all. The run is sitting at action_required (id 32367571694), which is the fork-contributor workflow-approval gate, not a failure. A committer has to release it before any check result exists — so the rebase alone will not produce a green check without that.
AI disclosure: reviewed with Claude Code. I read the delta and the current connectClient signature at this head myself, confirmed the rename left no stale identifiers, and checked the workflow-run state directly. The conclusions are mine.
| test('subscribed Clients receive the durable steering echo as a session event', async () => { | ||
| await withExecutionRoot(async (fixture) => { | ||
| const host = await fixture.startHost(); | ||
| const client = await connectClient(fixture.root, 'tui'); |
There was a problem hiding this comment.
[P1] Rebase onto main and drop the 'tui' argument — this line is why the required test check cannot pass. connectClient now takes one parameter (execution-host-suite.ts:1043, connectClient(rootPath: string)); #3277 (a6f33c952, "retire client surface identity plumbing") removed the second surface parameter after this branch was cut. The build step fails at src/__tests__/execution-host-message.test.ts(101,54): error TS2554: Expected 1 arguments, but got 2. I flagged this as external to your diff on the previous head, and that was fair then; at this head the branch has moved without picking up main, so it is now this PR's to resolve, and it is the only thing standing between the change and a green check. Evidence: confirmed by reading code at this head — I compared the call site here against the fixture's current signature on main. No new regression test is needed; the existing suite covers the behaviour once it compiles.
There was a problem hiding this comment.
Done — branch rebased onto main (5d8605ab0..6397a5677) and the new e2e test updated to the single-argument connectClient from #3277. Full rebuild of the dependency chain is clean, and the affected suites pass: projector/protocol/coordinator/root-turn 134/134, execution-host message/queue e2e 14/14.
| const event = projectToolEvent(frame); | ||
| if (event) events.push(event); | ||
| const event = projectSessionEvent(frame); | ||
| if (event) { |
There was a problem hiding this comment.
[P1] Dedup the forwarded echo against the transcript bootstrap as well. The runtime persists a steering_message before the Host forwards it, so subscription.open can bootstrap message X and install the subscriber before acceptRuntimeEvent publishes the live frame for X. A fresh projector starts #renderedSteeringMessageIds empty; #transcriptIds only suppresses queue synthesis, and this branch does not consult it. The pending live frame therefore emits X again, while the TUI reducer unconditionally appends another user prompt. This is possible even when X is still in_flight: seedActive skips it because it is durable but does not mark it rendered. Please seed/check the durable steering identities (or establish a non-overlapping transcript/live high-water boundary), and add a regression where the bootstrap already contains X and the first pending frame is the forwarded echo for X.
There was a problem hiding this comment.
Fixed in 6397a56 — dedup now covers the bootstrap path too, not just the two live paths.
RuntimeHostSessionProjectionSeed gained durableSteeringMessageIds: the messageIds of transcript user messages carrying steeringEventId (the read model assigns the submitted steering messageId as the stored message id via stableMessageId → refs.providerEventId). The projector seeds #renderedSteeringMessageIds from them in the constructor, so all three render paths — bootstrap render, queue in-flight synthesis, and the forwarded echo — share one dedup set keyed by messageId, and whichever projects the message first wins.
Regression added exactly on the requested shape (session-projector.test.ts): a projector seeded with a transcript containing X, with X still in_flight — seedActive emits no synthesis for X, the first pending frame is the forwarded echo for X, and it is suppressed; a different steering message still renders once. All suites pass (134/134 unit + 14/14 e2e).
…ribers The TUI never rendered a consumed steering message: the host event pump dropped steering_message (not a transient tool/text event), leaving the transient in-flight queue observation as the only live render path, which the coalesced canonical refresh can skip entirely (queued -> consumed). Forward the durable steering_message on the session-event frame so every subscriber renders the interjection in place, keep the queue in-flight synthesis for the attach/rejoin window, and dedup both paths per message. Bumps the session continuity wire schema to 5. Fixes apache#3304
…class isRuntimeSessionForwardedEvent / RuntimeSessionForwardedEvent describe what the predicate decides — forward live to subscribers — now that the durable steering_message belongs to it. Rename the client projectSessionEvent to match its Host-side counterpart. Addresses review on apache#3316.
…ady rendered it subscription.open can bootstrap the durable steering message and install the subscriber before the Host's forwarded echo arrives; seed the render-dedup set from steering messages already durable in the transcript so the bootstrapped render stays the only one. Also rebased onto main and updated the new e2e test to the post-apache#3277 single-argument connectClient. Addresses review on apache#3316
2225aea to
6397a56
Compare
3 participants
Summary
Fixes #3304 — a steering message sent while the TUI is mid-turn was delivered to the model but never rendered in the transcript.
Root cause: the Runtime Host event pump dropped the durable
steering_message(it is not a transient tool/text event), leaving observation of the transientin_flightqueue state as the only live render path. The async, coalesced canonical refresh can skip that state entirely (queued → consumed), so subscribers saw only an emptyingqueue_updateand the interjection vanished from the UI.Changes
session-continuity.ts): newSessionSteeringEventcarried on the existingsubscription.session_eventframe (notoolUseId), strict exact-keys decode with bounded message content; wire schema 4 → 5 (mismatched clients already fail fast at subscription open).root-turn-coordinator.ts,session-continuity-coordinator.ts): the pump now forwards the durablesteering_message;projectToolEvent→projectSessionEventpasses it through verbatim to subscribers.adapter/session-projector.ts): projects the forwarded echo as a first-classsteering_messageSessionEvent, and dedups permessageIdagainst the existing queue in-flight synthesis (#renderedSteeringMessageIds) — whichever authoritative path arrives first renders exactly once. The synthesis stays for the attach/rejoin window where the durable event has not landed yet.TUI reducer and desktop live projection already render
steering_message, so no surface changes were needed there.Tests
packages/runtime-host: 1024/1024 pass;packages/clisteering/transcript/driver suites: 202/202;@maka/ui: 183/183; biome check clean; desktop typecheck clean.Compatibility
Wire schema bump 4 → 5: old clients connecting to a new host fail the handshake at subscription open instead of mis-decoding the new event variant (the established closed wire-schema convention).