Repository navigation
fix(cloud-agent): order transcripts turn by turn - #10673
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryI reviewed the full PR at Assumptions: Files Reviewed (12 files)
Previous Review Summaries (3 snapshots, latest commit 0a43686)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 0a43686)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental commit replaces the layered synthetic/confirmation ordering with turn-based ordering ( Assumptions: Files Reviewed (10 files)
Previous review (commit 3f68921)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe queue-acknowledgement fix keeps pending rows unconfirmed, but a failed follow-up is still confirmed at its submission time, so it can render inside the output of the turn that was already running when it was queued. Assumptions: Overview
Issue Details (click to expand)WARNING
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Previous review (commit c0edf6a)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryQueue-acknowledged rows lose the Assumptions: Overview
Issue Details (click to expand)WARNING
Files Reviewed (7 files)
Reviewed by deepseek-flash-latest · Input: 152.9K · Output: 41K · Cached: 6.8M Review guidance: REVIEW.md from base branch |
iscekic
left a comment
iscekic
left a comment
There was a problem hiding this comment.
bot: Automated merge-risk stamp for this pull request, requested by @eshurakov.
The merge-risk judge did not clear this change, so the code review ran instead of a stamp.
What the change does
The PR makes a cloud.message.queued event carry the server's persisted queue time (normalizer.ts parses the envelope timestamp into createdAt, session.ts forwards it on both the live and replay paths) and uses it in chat-processor.synthesizeQueuedUserMessage: a new placeholder is stamped with that time and a pre-existing optimistic row has its time.created replaced and its synthetic ordering marker cleared. The intent is that a failed prompt, which never receives a native message.updated, is ordered before a later retry both live and after replay; the comment in storage/helpers.ts is updated to match. Tests were added for the new-row and existing-row confirmation, for the normalizer fallback on an unreadable timestamp, and for live and replay ordering plus the DO page path.
What I checked
I ran the full @kilocode/cloud-agent-sdk Jest suite from an isolated copy of src (the checkout has no dependencies and is read-only): 45 suites, 2,309 tests, all passing, including the changed chat-processor, normalizer, storage/helpers and session-transport suites. I also ran a purpose-built ordering probe against the changed code (see the e2e evidence). The data flow from the DO is correct for this purpose: both producers of cloud.message.queued stamp it with message.createdAt (session-do.ts emitQueued, and the queued bootstrap snapshot in websocket/stream.ts), so the value is a real server queue time rather than a delivery time.
What to act on first
Clearing synthetic on every queue acknowledgement, rather than only on a terminal queued failure, is broader than the bug: it re-ranks a prompt that is still queued to its queue time. A v2/native turn produces one assistant message per step, so a follow-up queued mid-turn renders between the streaming turn's step messages (I reproduced this: u1 > a1 > q2 > a2) instead of after them (u1 > a1 > a2 > q2 before the change), then jumps down when its own native row arrives. The inline comment on chat-processor.ts asks for the marker to be kept while the message is merely queued and cleared when its terminal queued failure arrives; the same comment names the test to add (queued, not failed, prompt mid-turn with a later step message).
No security, data-loss or build finding: the parsed time is server-supplied, the parse sits at the existing normalizer boundary, and nothing destructive was added.
E2E: run partially.
The checkout has no node_modules and is read-only, so I copied packages/cloud-agent-sdk/src to a temp directory outside the checkout, vendored the two workspace packages it imports (app-shared, session-ingest-contracts), installed jest/swc/jotai/zod/react, and ran the package's own Jest config. Changed suites: chat-processor, normalizer and storage/helpers, 437 tests, all pass. Full SDK suite: 45 suites, 2,309 tests, all pass (one suite initially failed only because my copy lacked the package package.json next to src; it passes once that file is present). Targeted ordering probe: with user u1(created 900), assistant a1(1000), a queued prompt q2 taken from a cloud.message.queued envelope timestamp of 1500, and assistant a2(2000), orderMessageIds returns 'u1 > a1 > q2 > a2'; with the queued row left synthetic it returns 'u1 > a1 > a2 > q2', which confirms the mid-turn interleaving introduced by clearing the marker on queue acknowledgement. Not run: any browser flow, Worker/sandbox session, or production session. The PR itself reports full Worker/sandbox E2E as unverified, and local startup was not attempted here.
Every remark is attached to the changed line where GitHub accepts one.
This service never pushes, rebases, forces, closes, labels, or merges, and it posts nothing else on this pull request.
eshurakov
changed the title
Replace the layered transcript order (id order, time order with an unconfirmed tier, causal repair, confirm-on-failure) with one rule: a turn is a user row and the assistant rows that answer it, turns sort by the server time of their user row, and rows in a turn follow it by time. Queue acknowledgement records the server queue time as `queuedAt` on the client row and keeps `synthetic`, so retry, delivery, and unconfirmed handling keep their meaning. A queued or failed follow-up sorts after the turn that was running when it was sent, and a failed prompt stays above a later retry. Only a row without a server time sorts last. The shared-session page and admin trace viewer use the same order.
0a43686 to
25d61c4
Compare
|
Resolved the merge conflict with Post-rebase checks passed:
All three review threads were already addressed and resolved. The existing local-stack evidence in the description still covers the unchanged behavior. I did not restart the stack. The stated failed-follow-up and v2 stack coverage gaps remain. Final verification: all 11 required checks pass for |
Summary
Order the Cloud Agent transcript turn by turn, so a failed prompt stays above a later successful retry, and a queued or failed follow-up stays below the turn that was running when it was sent.
Before this change, the order had several layers: store id order, then
time.createdwith an "unconfirmed rows last" tier, then a causal repair. A failed queued prompt never gets a native row, so it kept thesyntheticmarker and sorted below every later row, including the retry and its answer. Replay also replaced its queue time with the device time.Now one rule applies (
orderMessageIdsinpackages/cloud-agent-sdk/src/storage/helpers.ts):parentID, followed through assistant parents).synthetic) is placed by its server queue time.cloud.message.queuedrecords it as a newqueuedAtKilo extension, live and on replay, from the persisted event timestamp.synthetickeeps its meaning ("client row, not yet echoed by the server"). Retry, delivered-send detection, and unconfirmed-row handling still read it unchanged. The shared-session page and the admin trace viewer use the same order.Both producers set
parentIDto the prompt that a step answers: Kilo v1 natively, and the v2 wrapper feed from the native message order. So a prompt that the runtime receives between steps owns the later steps, which matches what the runtime did.Verification
native-sync-snapshot.test.ts. It also fails with this change stashed, while the local dev stack loads the machine.src/lib/cloud-agent-nextandsrc/components/cloud-agent-next, 147 suites, 3,081 tests.src/components/agents, 307 files, 5,684 tests.git diff --checkpass.Local stack check (real Worker, sandbox, and Kilo v1 runtime, fake LLM)
billing_unavailable). Then I restarted the meter and sent a retry. After replay,mainshows the failed prompt below the retry and its answer, with a device-clock time. This branch shows it first, at its queue time.Not checked on the stack: a follow-up that fails while an earlier turn still streams. This needs delivery to fail during a live turn, which I could not cause locally without changing service code. The transport tests cover it. Kilo v2 was not run on the stack. Its
parentIDderivation was reviewed in code.Local setup needed two workarounds that are not in this PR: trusting the sandbox egress proxy CA in the sandbox image build, and retrying a flaky local R2 publish. No production session was accessed.
Browser evidence
Failed prompt and retry after replay, on
main:The same session on this branch:
Multi-step turn with a follow-up sent during step 2: