Skip to content

refactor(runtime): derive Session transcripts from RuntimeEvents - #4879

Merged
Astro-Han merged 35 commits into
mainfrom
refactor/4791-single-transcript-authority
Sep 7, 2026
Merged

refactor(runtime): derive Session transcripts from RuntimeEvents#4879
Astro-Han merged 35 commits into
mainfrom
refactor/4791-single-transcript-authority

Conversation

@Astro-Han

@Astro-Han Astro-Han commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Closes #4791.

The problem

Every execution fact was written twice: once as a RuntimeEvent, once as a session_messages row. Two authorities for one fact means every writer has to keep them in step, every reader has to choose one, and a crash between the two writes leaves a Session that disagrees with itself.

What this does

Makes the RuntimeEvent ledger the only durable transcript authority and deletes the second write. session_messages survives in exactly two roles: input to the one-way importer that converts a pre-ledger transcript on first read, and the WorkHub Coordination Session's own store, whose write path is out of scope for this issue — see The one Session the ledger cannot hold below.

The commits are the staging of one cutover — notes onto the ledger, a whole and resumable conversion, the read model, then the cut — and are meant to land together. Losing a legacy tool card in conversion is acceptable; losing a Session or a conversation is not, and the importer is a total function over every legacy row type.

The cut, site by site

  • markMessagesHandedOff no longer projects admitted Messages into transcript rows. It validates the admission and retires it. The proof those rows carried already lives in the agent-run admission's sourceMessages and in the RuntimeEvent steering proof.
  • Catalog projection (lastMessagePreview, lastMessageAt, connectionLocked) used to fall out of a transcript insert. AgentRun now commits it explicitly through commitMessageCatalogProjection: fail-closed for a user message, because that write also takes the Session's one-way connection lock, and fail-open for the assistant preview, which costs a stale sidebar line at worst.
  • The read marker no longer needs an ordered index of visible transcript rows. lastReadMessageId has no client consumer, so hasUnread is the only decision left, and it clears when the client has caught up with the ledger's newest visible message — read off a bounded tail of the last run rather than a whole-Session scan.
  • Startup recovery writes a crashed Turn's admitted prompt into the invocation that had already opened for it. A Root folded from several queued Messages carries no single admitted Message identity, so the prompt is durable under a derived ${runId}-admitted-prompt; recovering the same crash twice writes the same event and the store dedupes it. A sealed Run takes nothing — it is immutable, and a Run that reached its terminal fact has a prompt the crash did not eat.
  • WorkHub target linkage (fix(workhub): resolve WorkHub delegation linkage on demand #4699) enumerated a delegated Message's identity from three lifecycle tables, one of which was the transcript row this PR stops writing. Its handed-off arm now reads core_root_source_message_proofs — the Root admission that consumed the Message, in the same database and as durable as the Session.

Deleted with their last caller: markSessionReadThroughMessage, SessionReadMarkerMessageNotFoundError, readMessagesForRecovery (byte-identical to readMessages), listForRecovery's separate query, the transcript-ordering privates in the SQLite store, and buildTurnStateMessage with its lineage types.

One definition of a read, not two

An earlier revision of this PR answered four transcript questions twice: SQL expressions in runtime-transcript-query.ts restated message identity, output shape, terminal status and thinking↔text attachment, then fed their answers back into the TypeScript projector through an options.context escape hatch. Four of those expressions were baked into schema v17 expression indexes, so a semantic change had to be made in the projector, in SQL, and in a migration, together.

That layer is gone. projectRuntimeEventsToStoredMessages is the only place that says what a transcript row is; SQL keeps only what it is uniquely good at — the terminal predicate, the invocation joins, and the ordinal seek. The projector now returns sourceEventIds so a page can attribute each message to the event it came from, and page sequences are ordinal * EVENT_SEQUENCE_STRIDE + offset.

The cost is real and accepted: a page costs one Turn's events rather than exactly the rows it serves. The bound is ACTIVE_TRANSCRIPT_SOURCE_MAX_EVENTS (8192 events) and ACTIVE_TRANSCRIPT_OVERLAY_MAX_BYTES (16 MiB) per Turn — the same bound the active overlay already enforced. Sampling a real workspace, the largest Turn was 420 events / 843 KB, twenty times inside it.

session-transcript-reader.test.ts's decode budget follows that shape: the head page keeps a 512 KB assertion, which is the one that discriminates paging from materialization, and the tail page, lookup miss, and turn-index pages get a one-Turn budget.

Migration

transcriptLedgerVersion distinguishes the three states: absent means pre-ledger and converted on read, 0 means an imported transcript staged for conversion, 1 means ledger-authoritative. Event ids are derived from the run and the position within it, so an interrupted import is resumable — re-running it writes the same events and the store dedupes them.

A conversion a released build left part-written can hold events under ids this build cannot rederive. The converter resumes on the legacy row each event names in refs.storedMessageId: it derives the turn whole as a fresh conversion would, then skips only the events whose row already has that many on the run. A row half-converted by a crash between two of its events keeps the rest.

What this cutover permanently loses

transcriptLedgerVersion: 1 says a conversion ran, not that every legacy fact reached the ledger. A released build set the marker on the first send and went on writing turn-scoped context notes to session_messages alone. Re-running the converter cannot reach them: they belong to turns a real run already sealed, and a sealed run refuses the append.

Seven note kinds are affected — context_compacted, context_compaction_failed_open, context_provider_dropping, context_window_suggestion, context_window_overrun, context_reported_window_exceeded, context_overflow_after_compaction — on Sessions those builds touched. They are hidden from the model and describe context, not conversation, so this is accepted rather than repaired. No user message, assistant message, tool call, or tool result is affected.

Compatibility

Four epoch steps, all hard handshake gates:

  • 126 — durable transcript cursors seek Session event ordinals instead of run indexes.
  • 127 — Session Turn contributions carry only the Turn's recorded state; the five derived shape booleans are no longer sent, because the projector answers those questions.
  • 128 — transcript bootstraps drop durableCoverage. The field claimed that consecutive durable sequences differ by one, and the client enforced it. A durable sequence is now ordinal * EVENT_SEQUENCE_STRIDE + offset, so the gaps are the representation and no projection can make that claim.
  • 129 — Turn states and Turn records drop partialOutputRetained, a fact derived twice and read by nothing.

An epoch rejects an incompatible peer, not an incompatible row already on disk. Dropping partialOutputRetained from TurnStateMessage also dropped it from the shape the decoder accepts, and hasExactShape is exact on both sides — so every turn_state row a released build wrote stopped decoding at all, and the Sessions made of them could not be converted or displayed. What a shape emits may shrink freely; what it accepts may only grow. defineObjectShape now takes the retired keys as a third argument: accepted on read, dropped by pickShape. That names a second contract the helper had been deriving from the first, and it absorbs the three hand-built copies of the same rule that already existed (withoutRetiredSubagentRuntimeKeys, LAST_REQUEST_ANCHOR_DECODE_SHAPE, CONTEXT_BUDGET_SHAPE).

Paging is ordered by sequence, not by opening

A page resumes from one record's sequence and drops everything on the other side of it, so a scalar cursor is only sound over a stream totally ordered by that sequence. The durable walk emitted Turn by Turn in opening order, which agrees with sequence order only while Turns occupy disjoint ordinal ranges — and this PR withdrew exactly that invariant from the store, because a conversation-copy fixture interleaves two visible root runs on purpose. A nested run therefore lost the outer Turn's later rows: one sweep returned them, page-size-1 did not.

Enforcing disjointness was tried first and reverted: three fixtures falsified it deliberately, and the alternative to reverting was trading "a transcript slightly out of order" for "a Session permanently wedged after one failed recovery". So emission is made monotone instead of assuming disjointness — Turns whose ordinal ranges overlap are drained as one cluster, sorted. A Session without overlap yields a cluster of exactly one Turn, and the lookahead read that ends a cluster becomes the next cluster's first Turn.

Why isRuntimeHostedRootAuthority is not narrowed here

The hosted and embedded paths each answer "is this the root authority?" for themselves, and a reviewer asked whether the transcript work should collapse them. It should not, in this PR. The duplication is a write-side question — who may admit a Turn — and this change owns the read side; narrowing the predicate would move a policy decision into a refactor whose acceptance does not test it, and the shared-Session paths that would have to change are the same ones #3492 already has queued for the WorkHub. The judgement is recorded here rather than fixed so the next reader does not re-derive it.

The one Session the ledger cannot hold

A ledger row hangs on an invocation. The WorkHub Coordination Session has almost none: workhub.coordination.answer is the only path that admits a root Turn and nothing in the renderer calls it, while act, prepareStop, prepareReplacement and record all append rows under a Turn id no admission ever minted. record goes further and refuses to write unless no admission exists, then persists a renderer-composed status string as an assistant message with modelId: 'maka-workhub-coordination' and a self-authored turn_state.

Nothing converts those rows, so moving the read path to the ledger emptied this Session's timeline — caught by the desktop E2E, which seeds and drives the real WorkHub.

This PR gives that one Session a reader over its own rows, and readMessagesAfter a backward bound so the reader can page towards older rows without the full replay #4647 removed. The reader is written to be deleted: the WorkHub's own ADR already requires the Coordination Session to reuse the existing Turn infrastructure and forbids the renderer-appended summary. Reported with a concrete fix at #3492 (comment); once every Coordination action admits a Turn, this Session reads like any other and the reader goes.

Ablations kept out

  • An existing.some(role !== 'system') guard in recovery layered on top of the terminal-event check: the terminal check alone is exact, so the extra read was removed.
  • Removing the singular appendMessage: pure test churn with no production gain, so it stayed.
  • A durable per-Session conversion cursor: the conversion is already correct and idempotent under interruption, and a cursor would add a protocol-visible header field and its invalidation obligation to make a crash-retry of a one-time migration cheaper. Ablation says drop it.

Verification

All twelve workspace suites pass with zero failures on the merged head, plus npm run format and npm run lint. Desktop E2E: 32/32.

Two regression tests came out of the defects above rather than out of the design: a released-shape decode fixture (session-retired-fields.test.ts), and a paging property — page size 1 returns exactly what one sweep returns, in both directions, over a nested Turn. Both are properties at a boundary; neither defect was reachable by an example test, which is why an earlier round missed them.

The E2E suite was the only thing that caught three defects in the read path, because its fixtures seed legacy session_messages and drive the real converter and reader: chunked records were not reassembled by the paged scan, durableCoverage demanded contiguity that stride sequencing cannot give, and hasOlder assumed a transcript starts at sequence zero.

Not covered by automated tests and worth a human pass before merge: opening a Session created by an older build and confirming its whole history renders.

@github-actions github-actions Bot added the effort/XXL Over 2500 readable lines label Sep 5, 2026

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed at exact head a6fca96a. One [P1] and three [P2]s. The P1 is the failure mode this PR exists to remove, reintroduced on the recovery path. The direction is right and most of the cut checks out; details below.

[P1] One executed prompt can become two transcript users after a restart

The write path and the recovery path derive the user event's id by different rules:

  • A Root folded from several queued Messages has no single Message identity, so admission.userMessageId is null and begin() writes its user event under a fresh newId().
  • Recovery looks for the derived ${runId}-admitted-prompt.

The window named in the description — invocation open, no user event, process dies — is genuinely repaired: no terminal, the admission is still in core_root_turn_admissions, and recovery writes the admitted-prompt id.

The adjacent window is not. If the user event has already landed under newId() and the Host dies before the terminal, recovery filters on the admitted-prompt id, does not see the live event, and appends a second one. One prompt the model executed once, two user messages in the transcript. That is a Session disagreeing with itself, which is precisely what making the ledger the single authority was meant to end.

Single-source Roots are unaffected: there the admitted id and the begin() id are the same Message id.

On reachability, since that is what should decide the grade: folding happens when someone queues a few messages before a turn runs, and losing the Host mid-turn is an ordinary crash. Neither leg is exotic, and the result survives the restart rather than being repaired by it.

The fix that fits this PR's own design is to make the derivation one rule on both sides — have begin() use ${runId}-admitted-prompt when the admission carries no userMessageId, so the write and the recovery agree and the store's exact-duplicate dedupe absorbs the second attempt. Filtering recovery by run rather than by derived id would also work; the first is smaller and matches how the rest of the cutover derives ids.

[P2] An in-process start failure seals the hole that crash recovery would fill

runAgentTurn's catch runs finalizeFailedRunStartfailStartfinalize. If opening already committed, the second openInvocation is a no-op and a terminal is written with no user event. The sealed-run skip then refuses to backfill it, permanently.

The asymmetry is the point: a process crash on the same state recovers, and this exception path does not. The user saw the turn fail and can resend, so the cost is a permanent empty sealed Run rather than lost work — but it is the more common of the two failures and it is the one that cannot be repaired.

[P2] Turns with no remaining user after the steering filter are dropped silently

materializeTranscriptLedger converts a turn only if some message is still type === 'user' after user rows carrying steeringEventId are removed. A turn that is only notes, only tools, or only steering users never becomes a run, with no diagnostic. Production context_compacted carries the live turnId and does convert, and I did not find a tagged-release writer that produces a user-less conversation turn — so this is a live silent filter rather than a demonstrated loss.

[P2] The bounded unread tail can fail to clear

session.read_marker.set reads the newest 64 messages / 256 KiB instead of scanning every visible row, and clears hasUnread only when that window's newest user|assistant id matches. It never clears falsely — a newer visible message would be nearer the tail and inside the window — but it can fail to clear when the newest records are all hidden tool or system rows. Badge only; the request itself is still there. lastReadMessageId has no in-tree client reader, so hasUnread is the live bit.

What holds

The dual-write window is genuinely gone from the live path. The base wrote session_messages via appendUserMessageOnce and then the user RuntimeEvent, markMessagesHandedOff inserted user rows, and finalize appended session_resume. At this head agent-run.ts has no appendUserMessageOnce and no appendMessage, and handoff only deletes admissions. There is no longer a pair of authorities for a crash to split — which is why the P1 above is worth fixing rather than accepting: the design achieves its goal everywhere except that one id mismatch.

The importer is total over the legacy row types, enumerated from the tagged writers (v0.1.0v0.1.11, then v0.2.0-dev.9+) rather than from what remains in the tree. Each type converts, or is deliberately skipped with the fact owned elsewhere. An unknown type throws StoredSessionMessageIncompatibleError and fails the whole readMessages, so conversion never starts — a fail-closed Session instead of a quietly truncated history, which is the right way for this to break.

Import staging is reentrant and cannot strand a Session. transcriptLedgerVersion === 0 is hidden from the catalog and blocked from every execution kind, conversion runs in admitTurn before begin, and externalSessions.recover() retries on every Host start. Re-running an interrupted import writes the same derived ids and the production SQLite store dedupes them — verified on the real store, not assumed.

The five deletions carry their proofs elsewhere. Handoff's content lives in the admission's sourceMessages plus the user RuntimeEvent; the catalog projection's user path is fail-closed and its assistant path fail-open, and the lock and preview share one SQLite transaction so "lock taken, projection failed" cannot happen; readMessagesForRecovery was byte-identical to readMessages; buildTurnStateMessage's lineage is rebuilt from invocation.opening.lineage.

Scope note

The WorkHub linkage lane — the second item the description flags as needing a human pass, and the one that matters because #4699's target linkage enumerated a lifecycle table this PR stops writing — is still running. I will post it separately rather than amend this.

One disclosure: the importer, second-write, and crash/race lanes were all carried out by the same reviewer, so they are not independent cross-checks of each other.

简体中文

在 exact head a6fca96a 上评审。一条 [P1],三条 [P2]。而这条 P1 恰恰是本 PR 立意要消灭的那种失败,在恢复路径上又出现了。 方向是对的,大部分切除也经得起核。

[P1] 一次已执行的 prompt,重启后可能变成两条 transcript user

写入路径与恢复路径用两套规则派生 user 事件的 id:由多条排队 Message 折叠而成的 Root 没有单一 Message 身份,admission.userMessageIdnull,于是 begin() 用新的 newId() 写下 user 事件;而恢复侧寻找的是派生的 ${runId}-admitted-prompt

描述中点名的那个窗口(invocation 已开、无 user 事件、进程死亡)确实被修好了但紧邻的那个没有:若 user 事件已以 newId() 落盘、而 Host 在写 terminal 之前死亡,恢复会按派生 id 过滤、看不见那条已存在的事件,于是再追加一条模型只执行过一次的 prompt,在 transcript 里成了两条 user 消息 —— 这正是「让账本成为唯一权威」本要终结的「会话自相矛盾」。

单源 Root 不受影响:那时 admitted id 与 begin() 的 id 是同一个 Message id。

关于可及性(既然定级应由它决定):折叠发生在有人在一轮执行前排入几条消息时,而 Turn 执行中失去 Host 是普通崩溃。两条腿都不罕见,而且结果会熬过重启,而不是被重启修好。

与本 PR 自身设计相符的修法,是让派生在两侧成为同一条规则 —— 当 admission 不带 userMessageId 时,让 begin() 也用 ${runId}-admitted-prompt,使写入与恢复一致,并由 store 的精确去重吸收第二次写入。让恢复按 run 而非派生 id 过滤同样可行;前者更小,且与这次切换其余部分派生 id 的方式一致。

[P2] 同进程内的启动失败,会把崩溃恢复本可填上的洞封死

runAgentTurn 的 catch 走 finalizeFailedRunStartfailStartfinalize。若 opening 已提交,第二次 openInvocation 是 no-op,随后写下一个没有 user 事件的 terminal;已封存跳过规则此后永久拒绝补写。

不对称才是要点:同样的状态下,进程崩溃能被恢复,而这条异常路径不能。 用户看到那一轮失败、可以重发,所以代价是一个永久的空封存 Run 而非丢失工作 —— 但它是两者中更常见的那一个,也是唯一无法修复的那一个。

[P2] steering 过滤后不再剩 user 的 turn 被静默丢弃

materializeTranscriptLedger 仅在去掉带 steeringEventIduser 行之后仍有 type === 'user' 时才转换该 turn。只有 note、只有工具、或只有 steering user 的 turn 永远不会成为 run,且无任何诊断。生产的 context_compacted 带着实时 turnId,会被转换;我也没有找到会产出「无 user 的会话 turn」的已发布写入方 —— 所以这是一个仍然存活的静默过滤,而不是已被证实的丢失。

[P2] 有界的未读尾部可能清不掉

session.read_marker.set 改为读最新 64 条 / 256 KiB 而不再扫描全部可见行,仅当该窗口内最新的 user|assistant id 匹配时才清除 hasUnread它永远不会误清 —— 更新的可见消息必然更靠近尾部、落在窗口内 —— 但当最新记录全是隐藏的工具/系统行时,它可能清不掉。 只影响角标,请求本身仍在。lastReadMessageId 在本仓库没有任何客户端读取方,真正起作用的是 hasUnread

成立的部分

双写窗口在活路径上确实消失了。 基线上 appendUserMessageOnce 先写 session_messages、再写 user RuntimeEvent,markMessagesHandedOff 还会插入 user 行,finalize 追加 session_resume。在此 head 上,agent-run.ts 既无 appendUserMessageOnce 也无 appendMessage,handoff 只删除 admission。同一事实不再有两个权威可供崩溃劈开 —— 这也正是上面那条 P1 值得修而不是被接受的原因:这个设计在除那一处 id 不一致之外的每一处都达成了目标。

导入器对 legacy 行类型是全的,而且是从已发布 tag 的真实写入方(v0.1.0v0.1.11,以及 v0.2.0-dev.9+)枚举,而不是从树里剩下的类型倒推。每一类要么被转换,要么被有意跳过且该事实由别处拥有。遇到未知类型会抛 StoredSessionMessageIncompatibleError 并让整次 readMessages 失败,于是转换根本不会开始 —— fail-closed 的会话,而不是被悄悄截断的历史,这是它该有的坏掉方式。

导入的暂存态可重入,且不会让会话搁浅。 transcriptLedgerVersion === 0 对目录隐藏、并被拦截在所有执行种类之外,转换在 admitTurn 中于 begin 之前运行,而 externalSessions.recover() 在每次 Host 启动时重试。重跑一次被中断的导入会写出同样的派生 id,生产 SQLite store 会去重 —— 这是在真实 store 上验证的,不是假定的。

五处删除的证据确实在别处。 handoff 的内容存在于 admission 的 sourceMessages 与随后的 user RuntimeEvent;目录投影的用户路径 fail-closed、助手路径 fail-open,而锁与预览共享同一个 SQLite 事务,所以「锁已取走、投影失败」不可能发生;readMessagesForRecoveryreadMessages 逐字节相同;buildTurnStateMessage 的 lineage 由 invocation.opening.lineage 在读模型中重建。

范围说明

WorkHub 联动那条车道仍在进行 —— 那是描述中点名需要人工过一遍的第二项,也是要紧的一项,因为 #4699 的目标联动此前从一张生命周期表枚举身份,而那张表正是本 PR 停止写入的。 结果我会另发一条,而不是修改本条。

一项披露:导入器、第二写点、崩溃/竞态三条车道由同一位评审者完成,因此它们彼此之间不是独立的交叉验证。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

Astro-Han added a commit that referenced this pull request Sep 6, 2026
…un without it

Review of #4879 found the failure the PR exists to remove, reintroduced on the
recovery path. `begin()` derived the prompt event's id as
`userMessageId ?? newId()`; recovery derived it as
`userMessageId ?? ${runId}-admitted-prompt`. The two rules agree only for a
single-source Root. A Root folded from several queued Messages has no Message
identity, so a Host that died after the prompt landed and before the terminal
came back to a ledger whose prompt it could not see, and recorded the same
executed prompt a second time.

`admittedPromptEventId` is now the one derivation, and recovery asks whether the
Turn has a prompt rather than whether it has one under that exact id: a Run
written by an older build derived the id differently, and matching on the id
would read its prompt as missing. Steering leaves that index — it is typed as a
user message but is something said into an already-admitted Turn, so it is never
the Turn's own prompt.

The same review found the in-process mirror of that crash: `begin()` failing
between opening the invocation and recording the prompt runs `failStart` ->
`finalize`, whose terminal event seals the run against every later append,
recovery's repair included. Before this cutover recovery could still append to
`session_messages`, which has no seal; a sealed ledger cannot be repaired, so
`finalize` records the prompt itself before sealing, next to the openInvocation
call that already keeps the sibling rule "a run cannot end without having begun".

The read marker's tail scan now pages past hidden records. It read one bounded
page and gave up, so a Turn ending on tool traffic could leave a Session showing
unread after it had been read. It never cleared falsely, so this is a badge, not
a lost message.

Two review points are not taken. The reviewer's fix for the id mismatch was to
inline the derived id in `begin()`; that leaves the same string template in two
packages, which is still two rules that happen to agree. The reviewer also read
the importer's "convert only turns that still have a user row" filter as a
silent drop with no producer. It has one: a turn whose only user row was
steering belongs to a Turn some durable Root already owns, and converting it
stands a second synthetic run beside that one. Ablating the filter fails
`does not import Host-handed-off transcript messages as synthetic runs`, so it
stays, with its reason written down.

storage 1092 pass, runtime-host 1711 pass, runtime 3130 pass, cli 805 pass, 0
failures.

Generated-by: Claude Code
@jackwener

Copy link
Copy Markdown
Member

Follow-up: the WorkHub linkage lane, posted separately as promised rather than amending the earlier review. Exact head a6fca96a. No finding — the substitution holds, including for Sessions that delegated before this PR.

This was the second item the description flags as needing a human pass, and it is a cross-PR question: #4699's target linkage enumerates a delegated Message's identity from three lifecycle tables, and one of those arms read the transcript row this PR stops writing.

The concern was the upgrade case, and it is closed. A Session that completed a delegation before this lands would have the old transcript row and, if the proofs table were new, no replacement — so its linkage would quietly disappear. That is not the situation: core_root_source_message_proofs is not created by this PR, and neither the table nor its INSERT is new. Both have been in agent-run-store.ts since 4153915b8 (#1682), through 1caea265c (#1994) and ce6534d65 (#2445). Historical Root admissions already wrote their proofs, so the replacement arm finds exactly the delegations the old arm found.

The identities line up because the proof is the same id. For each source of admission.sourceMessages the store inserts (sessionId, source.messageId, turnId) — the delegated Message's own id, which is the whm_ value the query's GLOB 'whm_*' and length(message_id) = 52 predicates already selected on. The predicates are unchanged; only the table they read moved.

It is also a better record than the one it replaces. The three arms now map cleanly onto the three states a delegated Message can be in — pending in message_admissions, admitted into a Turn, cancelled in cancelled_message_admissions — and the middle arm reads the admission that consumed the Message rather than a transcript row that happened to be written alongside it. The old arm depended on a side effect of the second write this PR removes; the new one depends on the record whose purpose is to say the Message was consumed. The dependency got more direct, not more fragile.

Two limits worth stating rather than leaving implied:

  • This was verified by reading the query, the insert site and that file's history — not by running a probe against a real workspace with a pre-PR delegation. The reasoning is that the proofs rows must already exist because their writer predates the change by many releases; a probe would make it a measurement instead of an inference.
  • One shape would diverge: a whm_ Message written into the transcript but never admitted and never cancelled would have been picked up by the old arm and is not picked up by the new one. I did not find a producer for that state — a delegated Message with no lifecycle record at all — so I am recording it as a shape I could not reach rather than as a risk.

With this, all four lanes on this PR are reported. The [P1] on the recovery path stands as the one thing to fix.

简体中文

补充:WorkHub 联动这条车道,按先前承诺另发一条,而不是修改已发出的评审。 exact head a6fca96a无 finding —— 这次替换是成立的,包括对本 PR 之前就发生过委派的 Session。

这是描述中点名需要人工过一遍的第二项,而且是个跨 PR 的问题:#4699 的目标联动从三张生命周期表枚举被委派 Message 的身份,其中一条臂读的正是本 PR 停止写入的 transcript 行。

我担心的是升级情形,而它已经排除。 一个在本 PR 落地之前完成过委派的 Session,会有旧的 transcript 行;若 proofs 表是新建的、没有替代记录,它的联动就会静默消失。事实并非如此:core_root_source_message_proofs 不是本 PR 创建的,表和它的 INSERT 都不是新的 —— 两者自 4153915b8(#1682)起就在 agent-run-store.ts 里,并经过 1caea265c(#1994)与 ce6534d65(#2445)。历史上的 Root admission 早已写下自己的 proof,所以替代臂找到的正是旧臂找到的那些委派。

身份能对上,是因为 proof 存的就是同一个 id。admission.sourceMessages 中的每个 source,store 插入 (sessionId, source.messageId, turnId) —— 就是被委派 Message 自身的 id,也正是查询里 GLOB 'whm_*'length(message_id) = 52 一直在筛选的那个 whm_ 值。谓词没有变,变的只是它读哪张表。

而且它比被取代的那条记录更合适。 三条臂现在干净地对应一条被委派 Message 可能处于的三种状态 —— 在 message_admissions 中待处理、已被纳入某个 Turn、在 cancelled_message_admissions 中被取消 —— 而中间那条臂读的是「消费了该 Message 的那次 admission」,不再是碰巧与之一同写下的 transcript 行。 旧臂依赖的是本 PR 所移除的第二次写入的副作用;新臂依赖的是其存在意义就是「该 Message 已被消费」的那条记录依赖变得更直接,而不是更脆弱。

有两处限制,与其留作暗示不如明说:

  • 这是通过阅读查询、插入点与该文件的历史核实的 —— 不是对一个含有 PR 之前委派的真实工作区跑探针跑出来的。 推理依据是:proofs 行必然已经存在,因为它的写入方比本次改动早了许多个发布;一次探针会把它从推断变成实测。
  • 有一种形状会分歧:一条被写入 transcript、却既未被纳入、也未被取消whm_ Message,旧臂会捡到而新臂不会。我没有找到能产出该状态的路径 —— 一条完全没有生命周期记录的被委派 Message —— 所以我把它记为我未能到达的形状,而不是一项风险。

至此本 PR 的四条车道全部报完。恢复路径上的那条 [P1] 仍是唯一需要修的东西。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

@Astro-Han

Copy link
Copy Markdown
Contributor Author

Head 7c2c74e5.

[P1] Fixed. Confirmed, and a regression this PR introduced — on the base that branch threw and continued, never wrote.

Not fixed the way you proposed: inlining ${runId}-admitted-prompt in begin() leaves the same template in two packages, which is still two rules that happen to agree. admittedPromptEventId() is now the single derivation.

Unifying it is also not sufficient. core_root_turn_admissions outlives an upgrade, and a Run written by the current release recorded its folded prompt under newId() — matching on the derived id would still miss it and still write a second. So recovery asks whether the Turn has a prompt, not whether it has one under that id. Steering left that index in the same change: it is a user message said into an already-admitted Turn, never the Turn's own prompt, and it would otherwise trip the multiple UserMessages guard.

Test: startup recovery leaves a folded Root prompt the Run already recorded alone.

[P2 seal] Fixed at finalize. Also a regression from here: the base repaired this via appendMessage, and session_messages has no seal. The run.terminalEvent guard is correct, so the repair moved to where the prompt is still known — finalize() records it before reserving the terminal, next to the openInvocation() that already keeps "a run cannot end without having begun". Ablation-verified.

Narrower than a generic append failure: a latching store error also refuses the terminal, leaving the run open for crash recovery. The sealed-without-prompt shape needs a non-latching refusal, which is what the test injects.

[P2 importer] No finding. I removed the filter as suggested; does not import Host-handed-off transcript messages as synthetic runs fails. The producer is a turn whose only user row carries steeringEventId — steering said into a Turn a durable Root already owns, so converting it stands a second synthetic run beside it. Load-bearing; now says so in a comment. The tools-only case you actually named still has no producer, and I am not widening the rule for it.

[P2 unread] Fixed, P3. Confirmed, never clears falsely. Raising the constant only moves the boundary, so the scan pages back until it finds a visible record or exhausts the ledger. Stale badge on a read Session, self-healing at the next Turn.

Your single-reviewer disclosure: taken — it is why I treated the importer item as a gap to close rather than a defect to patch, and closing it is what produced the counterexample.

storage 1092, runtime-host 1711, runtime 3130, cli 805 — 0 failures. WorkHub linkage lane: waiting on your post.

@jackwener

Copy link
Copy Markdown
Member

Re-reviewed at exact head 7c2c74e5. Both findings are closed, and closed at the right level — the P1 fix makes the derivation one rule rather than patching one side of the mismatch. Two narrower residuals below, neither blocking.

[P1] Closed, and it also covers rows already on disk

admittedPromptEventId(runId, userMessageId) is now a single function used by the writer (agent-run.ts:739) and by recovery (hosted-execution-recovery.ts:88, :256). A folded Root's null resolves to ${runId}-admitted-prompt on both sides, and appending that id twice against production SQLite yields one user event.

The part worth calling out is not in the summary: recovery also stopped filtering the turn's user messages down to the derived id. verifyUserMessage now treats a single user event on the turn as the prompt being present, judged on content and origin. That matters because it is what makes the fix reach runs already written by an older build under newId() — those are exactly the rows that would still have grown a second prompt if the fix had only aligned the two derivations going forward. The new test seeds a randomUUID prompt already on the ledger and asserts it stays alone.

The mirror risk is avoided. Where userMessageId is present it is returned unchanged — 'msg-queued-1' stays 'msg-queued-1' — and this.input.userMessageId ?? this.input.newId() is gone from agent-run.ts. A fix that rewrote single-source ids into the derived form would have reintroduced the same defect pointing the other way, at existing data.

[P2] Closed at the seam that made it worse than a crash

initialRuntimeEventPending is set after openInvocation returns and cleared once the prompt write succeeds; failStart still routes into finalize, and finalize now writes the prompt before the terminal when the flag is set. The comment names the reason precisely — the terminal seals the run against every later append, including the one crash recovery would have used to repair the same shape. Their test refuses run-1-admitted-prompt once so begin throws, then finds both that prompt and one terminal on the ledger after finalize.

Residuals, both narrower than the hole they came from:

  • The retry is .catch(() => {}), so if the prompt write fails a second time the run still seals empty. Refusing to finalize would be worse, so this is the right trade — it is worth knowing it exists, not worth changing.
  • A throw inside openInvocation never sets the flag, so finalize can still open-then-seal a run with no prompt. That is a smaller window than the named one and is not a regression from this change.

[P2] The unread tail is genuinely paged now, and the bound it replaces is not lost

#newestVisibleMessage pages backwards instead of reading one bounded tail, and the comment names the shape I reported: a Turn ending on tool traffic can put more hidden records at the tail than one page holds.

I checked the termination, since replacing a bound with a loop is where this kind of fix usually overcorrects. It stops on the first visible message or when nextPosition === null. So the only way to walk the whole ledger is a Session with no visible message anywhere — and such a Session has no unread badge to clear in the first place. The cost scales with the trailing hidden stretch, not with Session length, and it runs on a user action rather than a hot path. Trading a few extra pages in a rare shape for the removal of a permanently-stuck badge is the right direction.

[P2] The steering-only turn is now explained rather than changed

The filter still drops a turn whose only user row was steering, and three lines of comment now say why: that steering was said into a Turn a durable Root already owns, so converting it would stand a second synthetic run beside the real one. The reasoning holds and I am not asking for a behaviour change. Stating it plainly, though: the filter is still silent — what changed is that a reader of the code can now find out why, not that a workspace where it fires reports anything.

Standing

The [P1] is gone, so the objection that made this NO-GO is resolved. The PR is DIRTY against main, so I am not attaching an approval to this head — an approval survives later pushes here, and I would rather not have one carry across an unresolved rebase, the same way I handled the conflict on #4890. Once it rebases green I will approve without re-litigating any of the above.

Everything else from the earlier passes stands: the importer is total over the legacy row types enumerated from tagged writers, an unknown type fails closed rather than truncating history, import staging is reentrant with dedupe verified on the real store, the five deleted second-writes carry their proofs elsewhere, and the WorkHub linkage swap is safe including for Sessions that delegated before this PR.

简体中文

在 exact head 7c2c74e5 上复审。两条 finding 都已关闭,而且关在了正确的层次上 —— P1 的修法是把派生变成一条规则,而不是给不一致的其中一侧打补丁。 下面两处残留更窄,均不阻塞。

[P1] 已关闭,而且覆盖了已经落盘的行

admittedPromptEventId(runId, userMessageId) 现在是一个函数,写入方(agent-run.ts:739)与恢复方(hosted-execution-recovery.ts:88:256)共用。折叠 Root 的 null 在两侧都解析为 ${runId}-admitted-prompt;在生产 SQLite 上以该 id 追加两次,得到一条 user 事件。

值得点出的一处不在摘要里:恢复侧同时不再把该 turn 的 user 消息过滤到派生 id。 verifyUserMessage 现在按内容与来源,把「该 turn 上存在一条 user 事件」即视为 prompt 已在。这一点要紧,因为正是它让修复触及「更早的构建以 newId() 写下的既有 run」 —— 而那些行恰恰是「只对齐今后两侧派生」时仍会长出第二条 prompt 的那批。新测试预先在账本上放了一条 randomUUID 的 prompt,并断言它保持独一份。

镜像风险被避开了。 userMessageId 存在时原样返回('msg-queued-1' 仍是 'msg-queued-1'),而 this.input.userMessageId ?? this.input.newId() 已从 agent-run.ts 移除。一个把单源 id 改写成派生形式的修法,会以相反方向、在既有数据上重新制造同一个缺陷。

[P2] 关在了「它比崩溃更糟」的那个接缝上

initialRuntimeEventPendingopenInvocation 返回后置位,在 prompt 写入成功后清除;failStart 仍然走向 finalize,而 finalize 现在在标志置位时先写 prompt、再写 terminal。注释把原因说得很准 —— terminal 会把该 run 对其后所有 append 封死,包括崩溃恢复本会用来修同一形状的那一次。他们的测试让 run-1-admitted-prompt 被拒绝一次以致 begin 抛出,随后在 finalize 之后于账本上同时找到该 prompt 与一条 terminal。

两处残留,都比它们所出自的洞更窄:

  • 那次补写是 .catch(() => {}),所以 prompt 写入第二次再失败时,run 仍会被封存为空。拒绝 finalize 会更糟,所以这是正确的取舍 —— 值得知道它存在,不值得为它改动。
  • openInvocation 内部抛出时标志从未被置位,所以 finalize 仍可能「开了就封」而没有 prompt。这个窗口比被点名的那个更小,也不是本次改动引入的回退。

[P2] 未读尾部确实改成了翻页,而它取代的那个上限并没有丢

#newestVisibleMessage 改为向前翻页,而不是只读一段有界尾部,注释也点名了我报的形状:以工具流量结尾的 Turn,尾部隐藏记录可能多于一页所能容纳。

我核了终止条件 —— 因为「用循环取代上限」正是这类修复容易矫枉过正的地方。 它在遇到第一条可见消息、或 nextPosition === null 时停止。所以唯一会走遍整个账本的情形,是一个从头到尾没有任何可见消息的 Session —— 而这样的 Session 本来就没有未读角标可清。 代价与尾部隐藏连续段成正比,而不与 Session 长度成正比,并且它跑在一次用户操作上而非热路径。用罕见形状下多翻几页,换掉一个会永久卡住的角标,方向是对的。

[P2] 只有 steering 的 turn 现在是被解释了,而不是被改变了

该过滤仍会丢弃「唯一 user 行是 steering」的 turn,现在有三行注释说明原因:那段 steering 是说进某个已有持久 Root 所拥有的 Turn 里的,转换它就会在真实 run 旁边立起第二个合成 run。 这个理由成立,我不要求改变行为。 但把话说清楚:该过滤仍然是静默的 —— 改变的是读代码的人现在能查到原因,而不是「它触发的那个工作区会报告些什么」。

当前立场

[P1] 已消除,所以让本单成为 NO-GO 的那条反对意见已解决。该 PR 相对 main 处于 DIRTY,因此我不在此 head 上附加批准 —— 在本仓库批准会跨后续 push 存活,我不愿让一个批准跨过一次尚未解决的 rebase,与我在 #4890 上处理冲突的方式一致。待其 rebase 转绿,我会直接批准,不再重提以上任何一条。

先前各轮的其余结论均成立:导入器对从已发布 tag 枚举的 legacy 行类型是全的;未知类型 fail-closed 而不是截断历史;导入暂存可重入且去重已在真实 store 上验证;被删的五处第二写点其证据确在别处;WorkHub 联动的替换是安全的,包括对本 PR 之前就发生过委派的 Session。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

@Astro-Han
Astro-Han force-pushed the refactor/4791-single-transcript-authority branch from 7c2c74e to 82325ad Compare September 6, 2026 10:01
Astro-Han added a commit that referenced this pull request Sep 6, 2026
…un without it

Review of #4879 found the failure the PR exists to remove, reintroduced on the
recovery path. `begin()` derived the prompt event's id as
`userMessageId ?? newId()`; recovery derived it as
`userMessageId ?? ${runId}-admitted-prompt`. The two rules agree only for a
single-source Root. A Root folded from several queued Messages has no Message
identity, so a Host that died after the prompt landed and before the terminal
came back to a ledger whose prompt it could not see, and recorded the same
executed prompt a second time.

`admittedPromptEventId` is now the one derivation, and recovery asks whether the
Turn has a prompt rather than whether it has one under that exact id: a Run
written by an older build derived the id differently, and matching on the id
would read its prompt as missing. Steering leaves that index — it is typed as a
user message but is something said into an already-admitted Turn, so it is never
the Turn's own prompt.

The same review found the in-process mirror of that crash: `begin()` failing
between opening the invocation and recording the prompt runs `failStart` ->
`finalize`, whose terminal event seals the run against every later append,
recovery's repair included. Before this cutover recovery could still append to
`session_messages`, which has no seal; a sealed ledger cannot be repaired, so
`finalize` records the prompt itself before sealing, next to the openInvocation
call that already keeps the sibling rule "a run cannot end without having begun".

The read marker's tail scan now pages past hidden records. It read one bounded
page and gave up, so a Turn ending on tool traffic could leave a Session showing
unread after it had been read. It never cleared falsely, so this is a badge, not
a lost message.

Two review points are not taken. The reviewer's fix for the id mismatch was to
inline the derived id in `begin()`; that leaves the same string template in two
packages, which is still two rules that happen to agree. The reviewer also read
the importer's "convert only turns that still have a user row" filter as a
silent drop with no producer. It has one: a turn whose only user row was
steering belongs to a Turn some durable Root already owns, and converting it
stands a second synthetic run beside that one. Ablating the filter fails
`does not import Host-handed-off transcript messages as synthetic runs`, so it
stays, with its reason written down.

storage 1092 pass, runtime-host 1711 pass, runtime 3130 pass, cli 805 pass, 0
failures.

Generated-by: Claude Code

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving at exact head 82325adc. The findings from the earlier rounds are resolved, and I re-bound them to this head by comparing bytes rather than assuming they carried.

The rebase moved 205 files, so the conclusions were not taken on trust. Of the six files the findings and fixes live in, five are byte-identical to 7c2c74e5:

  • message-authority.ts, agent-run.ts, hosted-execution-recovery.ts — the P1 fix, unchanged;
  • runtime-ledger-repair.ts — the steering-only turn rationale, unchanged;
  • sqlite-session-metadata-store.ts — the WorkHub linkage arm, unchanged.

session-catalog-coordinator.ts does differ, so I read it rather than counting it: the delta is the import-model candidate work (NoUsableImportModelError, ImportModelCandidate) that arrived from main, and #newestVisibleMessage — the paging fix for the unread marker — is character-for-character what was verified before, including the termination on either the first visible message or nextPosition === null.

So the state is: the [P1] is closed by a shared derivation used on both sides, the sealing [P2] is closed by writing the prompt before the terminal, the unread tail pages instead of giving up, and the steering-only filter is documented. The two residuals I recorded stay recorded and neither is asked for here: the prompt retry is .catch(() => {}), and a throw inside openInvocation never sets the pending flag.

Two things this approval does not claim:

  • Required test is still pending on this head. Branch protection enforces it independently, so this approval is a statement about the code, not about the gate.
  • An independent blind review is in flight — a reviewer of a different lineage, working from the live head without reading these comments or any of the earlier conclusions. That seat exists because the first four lanes on this PR were carried out by the same reviewer, which I disclosed at the time. Its result will be posted separately whatever it says, and it may well find something these passes did not.
简体中文

在 exact head 82325adc 上批准。先前各轮的 finding 均已解决,而且我是通过比对字节把结论重新绑定到本 head 的,不是假定它们自动转移。

这次 rebase 动了 205 个文件,所以结论没有被采信。在 finding 与修复所在的六个文件中,五个与 7c2c74e5 逐字节相同:message-authority.tsagent-run.tshosted-execution-recovery.ts(P1 修复)、runtime-ledger-repair.ts(steering-only turn 的理由)、sqlite-session-metadata-store.ts(WorkHub 联动那条臂)。

session-catalog-coordinator.ts 确实不同,所以我是了它而不是数它:差异是从 main 带进来的导入模型候选改动(NoUsableImportModelErrorImportModelCandidate),而 #newestVisibleMessage —— 未读标记的翻页修复 —— 与此前验证过的逐字相同,包括「遇到第一条可见消息或 nextPosition === null 才停」这一终止条件。

所以当前状态是:[P1] 由两侧共用的同一条派生关闭;封存类 [P2] 由「先写 prompt 再写 terminal」关闭;未读尾部改为翻页而不是放弃;只有 steering 的 turn 得到了书面理由。 我记录的两处残留仍然记录在案,且此处都不要求改动:补写 prompt 用的是 .catch(() => {});以及 openInvocation 内部抛出时待写标志从未置位。

这条批准不主张两件事:

  • 本 head 上必需的 test 仍处于 pending。 分支保护会独立强制它,所以这条批准是对代码的陈述,不是对门禁的陈述。
  • 一次独立盲审正在进行中 —— 由不同谱系的评审者从 live head 开始,不读这些评论、也不读此前任何结论。设这一席的原因是:本 PR 最初的四条车道由同一位评审完成,这一点我当时已经披露。 无论它得出什么结论,都会另行发布;它完全可能发现这几轮没有发现的东西。

Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

@Astro-Han
Astro-Han marked this pull request as ready for review September 6, 2026 10:56
Astro-Han added a commit that referenced this pull request Sep 6, 2026
The pending flag was set after `openInvocation()`, so the one failure it did
not cover was a throw from the opening itself: `finalize` reopens what it can,
and a run it manages to open then sealed with a terminal and no prompt — the
same hole the previous commit closed, entered from one step earlier.

Moving the flag ahead of the opening costs nothing when the invocation never
opens: `finalize`'s reopen fails too, and the backfill is a no-op on a run that
does not exist.

Reported as a residual on #4879 and not asked for; it is one line and it closes
the last entrance to a shape that cannot be repaired after the fact.

runtime 3131 pass, runtime-host 1723 pass, cli 805 pass, 0 failures.

Generated-by: Claude Code

@M4n5ter M4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

English

Reviewed at 69e6d066ca1e6d71ca1eb1ab7be73830074b053f.

[P1] Interrupted migration cannot resume when the clock advances.

The converter now derives deterministic event IDs but still passes the live clock into backfill. The generated payload contains generatedAt: now(), and SQLite's duplicate handling requires the entire event to match.

Reproduced with the production SQLite stores: seed an unmarked legacy Session containing a user, an assistant answer and a completed turn; interrupt conversion immediately after the user event commits; retry with a later clock. The retry throws RuntimeEvent identity conflict for transcript-…-e1. Further conversion attempts encounter the same conflict, blocking reads through ensureTranscriptLedgerForRead.

Make the complete regenerated payload deterministic, including its recovery timestamps. The existing resumability test uses a constant clock and runs two successful, complete conversions; it never interrupts the first one.

[P1] Startup recovery seals an unfinished migration and then marks truncated history as migrated.

For an unmarked legacy Session, interrupt conversion at the same point, then run recoverInterruptedSessionsStrict() before reading it. Generic recovery treats the synthetic transcript-* invocation as an interrupted execution and appends a failed terminal event. The converter's terminal guard then skips that turn, and ensureTranscriptLedger writes transcriptLedgerVersion: 1.

Reproduced with a constant clock to isolate this from the first finding: the original user + assistant + completed becomes user + failed. All three legacy rows remain in SQLite, but normal reads no longer return the assistant answer and subsequent reads skip conversion altogether.

Keep incomplete conversion runs out of generic execution recovery, or finish their conversion before that recovery can seal them. Fixing timestamp determinism alone does not fix this path.

[P2] The new pager bounds its response, but not its storage reads.

endedInvocations and projectRun enumerate every invocation in the Session and load/project the entire selected Run before applying page limits. The SQLite invocation listing also performs a terminal lookup for each invocation. Even a tiny page therefore incurs work proportional to Session length and the full selected Turn, including payloads outside the requested page. Long Sessions and large Turns lose the storage/memory bounds required by #4791.

Apply the bounds to invocation/event retrieval before materializing transcript payloads. This finding follows from the read path; I did not reproduce an OOM.

Validation: 181 existing targeted tests passed. Both migration failures above were reproduced by separate fault-injection tests using the production SQLite stores. Full Host integration validation was blocked by local dependency version mismatches.

简体中文

审查版本:69e6d066ca1e6d71ca1eb1ab7be73830074b053f

[P1] 迁移中断后,时间一变,重试就会报事件身份冲突。

迁移器固定了事件 ID,但事件内容里仍有 generatedAt: now()。SQLite 去重要求同 ID 的整个事件完全一致。

已用生产 SQLite store 复现:准备一个没有迁移标记的旧会话,包含 user、assistant 回答和 completed 状态;迁移写入 user 事件后中断,再用稍晚的时间重试,直接报 RuntimeEvent identity conflict for transcript-…-e1。后续重试仍会撞上同一条记录,经过迁移入口的会话读取也会失败。

需要保证重试生成的整个事件内容一致,包括恢复时间戳。现有测试固定了时间,而且只是把完整迁移执行两遍,没有真正测中断。

[P1] 重启恢复会抢先封存未迁完的 Run,随后把缺失回答的会话标成迁移完成。

同样在旧会话迁移写入 user 后中断,如果接着先执行 recoverInterruptedSessionsStrict()通用恢复逻辑会把 transcript-* Run 当成崩溃的执行任务,写入 failed 终态。之后迁移器跳过已封存的 TurnensureTranscriptLedger却照常把版本写成 1

这次复现全程固定时间,排除了上一项的影响。实测原来的 user + assistant + completed 只剩 user + failed。旧表三条记录都还在,但正常读取不再返回原回答,后续也不会再尝试迁移。

需要让未完成的迁移避开通用执行恢复,或先迁完再允许恢复逻辑封存。只修时间戳,挡不住这个问题。

[P2] 分页限制了返回量,却没有限制底层读取量。

新读取器每次先枚举整个 Session 的 invocations,再完整读取并投影目标 Run,最后才裁剪页面;SQLite 枚举时还会逐个查询终态。即使只要很小的一页,开销也会随会话历史和单个 Turn 的大小增长,页外的大块内容同样先读进内存,不满足 #4791 对有界读取的要求。

需要在获取 invocation 和事件时就限定范围,再生成页面。这项依据是静态调用链,没有做 OOM 复现。

验证:181 项现有定向测试通过;两个迁移问题分别做了故障注入,均在生产 SQLite store 上复现。完整 Host 集成验证受本地依赖版本不匹配阻断。

@M4n5ter M4n5ter left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

English

Fixed all three findings in two follow-up commits:

  • fd073a8 makes the entire imported event payload deterministic, not just its ID. Startup recovery now leaves importer-owned invocations alone so an interrupted conversion resumes instead of sealing a partial transcript.
  • b7d27cf seeks Session event ordinals before loading message payloads, fetches only the selected message's projection context, and uses indexed queries for Turn summaries, landmarks, and live-to-durable message lookup. RuntimeEvents remain the only transcript authority; schema v17 adds indexes, not another transcript copy. Host epoch 119 fences the changed cursor semantics.

Regression coverage includes interruption after a committed user event, startup recovery followed by full history retrieval, and a single Turn exceeding 5 MiB. Small-page/index/lookup reads decode under 512 KiB in that fixture; complete pagination matches the full projection, including thinking, permissions, terminal state, and multibyte byte-fragment reassembly.

Local verification: 6,899 passed, 29 skipped, 0 failed across core, storage, runtime, and runtime-host. TypeScript builds and staged checks passed. No manual Desktop smoke test was performed.

中文

三个问题都已修好,分成两笔提交:

  • fd073a8:导入事件的内容也改成确定性的,重试不再撞上同 ID、不同 payload 的冲突。启动恢复不再提前结束导到一半的迁移 Run,重启后仍能接着导完,避免漏掉旧对话。
  • b7d27cf:分页先按 Session 事件位置定位,再取当前消息和它需要的上下文;Turn 摘要、跳转位置和实时消息转入历史时的 ID 查询也改走索引。没有再存一份 transcript,schema v17 只加索引;Host epoch 升到 119,防止新旧游标混用。

回归覆盖了“用户消息已落盘后中断 → 启动恢复 → 读取完整旧对话”,也覆盖了单个 Turn 超过 5 MiB 的情况。后者的小页、索引和 ID 查询合计解码不到 512 KiB;逐页拼回的内容与完整投影一致,thinking、权限、终态和中文跨字节分片都做了对照。

本地 core、storage、runtime、runtime-host 合计 6899 项通过、29 项跳过、0 失败,TypeScript 构建和提交前检查也已通过。尚未做桌面端人工验收。

Astro-Han added a commit that referenced this pull request Sep 6, 2026
…un without it

Review of #4879 found the failure the PR exists to remove, reintroduced on the
recovery path. `begin()` derived the prompt event's id as
`userMessageId ?? newId()`; recovery derived it as
`userMessageId ?? ${runId}-admitted-prompt`. The two rules agree only for a
single-source Root. A Root folded from several queued Messages has no Message
identity, so a Host that died after the prompt landed and before the terminal
came back to a ledger whose prompt it could not see, and recorded the same
executed prompt a second time.

`admittedPromptEventId` is now the one derivation, and recovery asks whether the
Turn has a prompt rather than whether it has one under that exact id: a Run
written by an older build derived the id differently, and matching on the id
would read its prompt as missing. Steering leaves that index — it is typed as a
user message but is something said into an already-admitted Turn, so it is never
the Turn's own prompt.

The same review found the in-process mirror of that crash: `begin()` failing
between opening the invocation and recording the prompt runs `failStart` ->
`finalize`, whose terminal event seals the run against every later append,
recovery's repair included. Before this cutover recovery could still append to
`session_messages`, which has no seal; a sealed ledger cannot be repaired, so
`finalize` records the prompt itself before sealing, next to the openInvocation
call that already keeps the sibling rule "a run cannot end without having begun".

The read marker's tail scan now pages past hidden records. It read one bounded
page and gave up, so a Turn ending on tool traffic could leave a Session showing
unread after it had been read. It never cleared falsely, so this is a badge, not
a lost message.

Two review points are not taken. The reviewer's fix for the id mismatch was to
inline the derived id in `begin()`; that leaves the same string template in two
packages, which is still two rules that happen to agree. The reviewer also read
the importer's "convert only turns that still have a user row" filter as a
silent drop with no producer. It has one: a turn whose only user row was
steering belongs to a Turn some durable Root already owns, and converting it
stands a second synthetic run beside that one. Ablating the filter fails
`does not import Host-handed-off transcript messages as synthetic runs`, so it
stays, with its reason written down.

storage 1092 pass, runtime-host 1711 pass, runtime 3130 pass, cli 805 pass, 0
failures.

Generated-by: Claude Code
Astro-Han added a commit that referenced this pull request Sep 6, 2026
The pending flag was set after `openInvocation()`, so the one failure it did
not cover was a throw from the opening itself: `finalize` reopens what it can,
and a run it manages to open then sealed with a terminal and no prompt — the
same hole the previous commit closed, entered from one step earlier.

Moving the flag ahead of the opening costs nothing when the invocation never
opens: `finalize`'s reopen fails too, and the backfill is a no-op on a run that
does not exist.

Reported as a residual on #4879 and not asked for; it is one line and it closes
the last entrance to a shape that cannot be repaired after the fact.

runtime 3131 pass, runtime-host 1723 pass, cli 805 pass, 0 failures.

Generated-by: Claude Code
@Astro-Han
Astro-Han force-pushed the refactor/4791-single-transcript-authority branch from b7d27cf to 816fa17 Compare September 6, 2026 13:32
@jackwener

Copy link
Copy Markdown
Member

New finding at exact head 816fa172[P2]: the first read of a legacy Session converts its entire history before it can serve a single page. This one has a producer, which is why I am raising it rather than noting it: the population it affects is exactly the population the converter exists for.

What the code does

materializeTranscriptLedger opens with const messages = await this.deps.readMessages(sessionId) — the whole legacy transcript, in one array — then groups all of it and converts sequentially. The version marker is written only after the full loop completes.

The reader gates on that. Every durable page goes through ensureTranscriptLedgerForRead before it is served (session-transcript-reader.tssession-manager.ts), so the Session cannot return even its first bounded page until the whole conversion has finished.

Why it matters

Open a long legacy Session after upgrading — a long transcript, or one carrying large tool results — and peak memory and latency scale with the entire history rather than with a page or batch budget. If that exceeds what the process can do, or if it is interrupted, the next read starts the same full load again, because nothing durable records partial progress. The Session stays unavailable for as long as that keeps failing.

Nothing is lost: the legacy rows are intact, the deterministic event ids make a re-run idempotent, and a successful run recovers the Session. That is why this is an availability and upgrade-cost regression rather than a correctness one, and why it is [P2] rather than [P1].

It is worth raising against this PR's own stated requirement. The design records that upgrade time needs a measured upper bound and crash-resumable execution. The deterministic ids deliver the idempotency half — an interrupted import re-derives the same events and the store dedupes them — but not the bounded half: there is no bounded read, no batch, and no recorded progress, so an interruption costs the whole conversion again rather than resuming.

Direction

Page the legacy rows or turns rather than reading them all; persist a per-Session conversion watermark with each committed batch; and let the reader expose either a completed bounded prefix or an explicit "preparing" state instead of re-reading everything before every page. That keeps the idempotency this already has and adds the bound the design asked for.

Test gap alongside it

The existing tests cover conversion semantics and restart idempotency, and they cover them well. What I could not find is a test that drives the production reader through an oversized legacy transcript, or one that asserts bounded peak work and forward progress across a restart. Without that, the property above has no guard: a later change could make the conversion heavier and nothing would notice.

Scope of this comment

This supersedes my approval at 82325adc — that head has been replaced, and under the review standard in use a PR with an open P2 is a comment rather than an approval. Everything I previously verified still stands on this head where the files are unchanged; this is an additional path, not a reversal of those findings.

简体中文

在 exact head 816fa172 上的新发现 —— [P2]:一个旧 Session 的首次读取,会在能够返回任何一页之前先转换它的全部历史。 这一条有产出方,所以我是把它当 finding 提出而不是记一句:它影响的人群,恰好就是这个转换器为之存在的那批人。

代码做了什么

materializeTranscriptLedger 一开头就是 const messages = await this.deps.readMessages(sessionId) —— 整份旧 transcript,一个数组,然后整体分组、顺序转换。版本标记只有在整个循环完成之后才写入。

而读取路径以此为闸:每一个持久页在被返回之前都要过 ensureTranscriptLedgerForRead(session-transcript-reader.tssession-manager.ts),所以在整次转换结束之前,这个 Session 连第一个有界页都返回不了。

为什么要紧

升级之后打开一个长的旧 Session —— 长 transcript,或带有大块工具结果的 —— 峰值内存与延迟按整份历史增长,而不是按页或批的预算。 如果它超出进程能承受的范围,或者中途被打断,下一次读取会重新开始同样的全量加载,因为没有任何持久记录保存部分进度。只要这件事持续失败,该 Session 就一直不可用。

没有东西丢失:旧行完好,确定性事件 id 让重跑幂等,一次成功的运行即可恢复该 Session。这也是它属于可用性与升级成本的回退、而不是正确性问题的原因,以及它是 [P2] 而不是 [P1] 的原因。

值得对照本 PR 自己写下的要求来看。 设计里记着:升级耗时需要一个可测量的上界可从崩溃续跑的执行。确定性 id 交付了幂等那一半 —— 被中断的导入会重新派生出同样的事件、由 store 去重 —— 但没有交付「有界」那一半:没有有界读取、没有分批、也没有记录进度,所以一次中断的代价是整次转换重来,而不是续跑。

方向

对旧行或旧 turn 分页,而不是一次读完;每提交一批就持久化一个按 Session 的转换水位;并让读取方要么暴露一个已完成的有界前缀、要么暴露一个明确的「准备中」状态,而不是在每一页之前把所有行重读一遍。这样既保住它已经具备的幂等,又补上设计所要求的那个上界。

与之相伴的测试缺口

既有测试覆盖了转换语义与重启幂等,而且覆盖得不错。我没有找到的是:驱动生产读取路径穿过一份超大旧 transcript 的测试,或断言「峰值工作量有界」与「跨重启有前进」的测试。 没有它,上面那条性质就没有守卫 —— 日后某次改动让转换变重,不会有任何东西发现。

本条评论的范围

取代我在 82325adc 上的批准 —— 那个 head 已被替换,而按当前采用的评审口径,存在未解决 P2 的 PR 应当是 comment 而不是 approve。我此前验证过的内容,在本 head 上文件未变之处依然成立;这是一条新增路径,不是对那些结论的推翻。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Inline finding at exact head 816fa172.

Comment thread packages/runtime/src/runtime-ledger-repair.ts Outdated

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two further findings at exact head 816fa172, one per line.

Comment thread packages/runtime/src/runtime-event-read-model.ts Outdated
Comment thread packages/runtime-host/src/server/session-transcript-reader.ts Outdated

@hqhq1025 hqhq1025 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.

结论:当前 head 816fa17 尚不具备合入条件。两项功能问题已分别附行内评论:P1,正常升级后,已发布 runtime 写出的用户可见 warning 从 transcript 中消失;P2,旧转换器留下的随机 ID opening 使中断后的升级读取持续报唯一键错误。两项均通过真实 SQLite 和公开 reader/SessionManager 入口复现,旧数据库行本身仍保留。

此 PR 将 RuntimeEvents 作为 Session transcript 的持久权威,移除重复写入并增加查询及历史转换逻辑。本次覆盖转换、分页/active overlay、读模型、恢复和终态持久化;确认功能失败后,没有继续推进复杂度或风格评价。

构建及 283 项定点测试通过。全量测试只有一项 Bash sandbox 集成失败,已在 exact base 3697e63 重现同一失败,不归因于本 PR。当前 head 的 hosted checks 成功(Eval 跳过),但 GitHub 同时报告与 main 冲突。未执行 merge、APPROVE 或 REQUEST_CHANGES。

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

Comment thread packages/runtime-host/src/server/session-transcript-reader.ts Outdated
Comment thread packages/runtime/src/runtime-ledger-repair.ts Outdated
@Astro-Han

Copy link
Copy Markdown
Contributor Author

Head e482026d3. Both migration findings fixed, two items deferred with reasons, one pushed back. Detail is in the inline threads; summary here.

Fixed

  • 6f1528e0f — a conversion a released build left part-written. Its opening is adopted (a run needs exactly one; the unique index refuses a second); a prefix that already converted messages is sealed as unfinished rather than rederived, because rederiving would stand a second copy of each message beside the one already there. Ablation reproduces the reported UNIQUE constraint failed.
  • e482026d3 — the converter reads the legacy rows a page at a time, carrying a page's last turn into the next so no turn converts from a prefix of itself. Peak memory is one page plus one turn. No new watermark: a terminal on the turn's invocation was already the durable progress record.

Deferred, stated rather than silent

  • The retained context notes a released build wrote transcript-only. Seven kinds, not one. They belong to turns a real run already sealed, and the seal is an interface obligation of every store — reaching them needs an exception to the guard that makes a run's ending final. Two of the seven are derivable in principle; the "one note per send" rule they carry cannot be reconstructed across bounded pages. Hidden from the model, no effect on context or execution, legacy rows intact. A comment at the version gate now says version 1 means a conversion ran, not that session_messages is empty.
  • The SQL seek beside the JS projection. It exists to satisfy this PR's own bounded-page acceptance; deriving one side from the other is a larger change than this should absorb now.

Pushed back

  • "Restore the dual write and the suite stays green" — it does not. Reintroduced on the live path: runtime 3123/8 fail, control 3132/0 fail. And runtime's SessionStore no longer has any write method, so the probe would not compile without a cast.

runtime 3135, runtime-host 1726, storage 1110 — 0 failures.

Thanks to @M4n5ter for fixing the three findings from the blind review directly, and to @hqhq1025 — the released-build compatibility case was reachable and neither of the earlier lanes had it.

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Two findings at exact head e482026d, one per line.

Comment thread packages/runtime/src/runtime-ledger-repair.ts
Comment thread packages/runtime/src/runtime-ledger-repair.ts

@hqhq1025 hqhq1025 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-GO。opening-only 唯一键阻断已修复,但更长的已发布随机 ID 前缀被封存后,完整 assistant 回答从生产 reader 消失,见下方 P1。分页探针确认跨页转换和事件去重有效;重启仍从首行扫描,首屏等待整次转换完成。构建及 291 项定点测试通过。本 head 当前与 main 冲突,GitHub 本次未返回 hosted checks。完整旧版本生成历史的端到端验收另行执行。

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

Comment thread packages/runtime/src/runtime-ledger-repair.ts Outdated

@hqhq1025 hqhq1025 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-GO,绑定 head 56af740a23cd86b90984106104ae6196dd12890d

已重新构建并通过改变后的生产读取路径复现既有 P1:旧随机转换前缀导致回答不可见。实际发布的 dev22 转换器提交 opening 和 user 后中断;当前 readDurablePage -> ensureTranscriptLedgerForRead 仍把该 turn 封存为 failed/missing_terminal_event,返回 user 和失败状态两条,原先可见的完整 assistant 回答缺失,并把 Session 置为版本 1。定位仍为 packages/runtime/src/runtime-ledger-repair.ts:123–135。原始 transcript 行保留在库里,问题是生产读取不可见。

e482026 相比,这一增量修改 16 个文件(+416/−969),将分页由逐事件选择改为逐 turn 投影。转换器 blob 相同,但 reader 与 read model 已变,因此本结论来自新 head 的短探针实测,不是沿用旧结论。opening-only 升级对照通过;clean install、build:test 和 reader/repair 14 项定点测试通过。完整长会话验收正在单独执行;此评论不评价另一审查者负责的 SQL/投影设计,也不构成完整新 head 放行。PR 当前与 main 冲突。

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Status at exact head 56af740a, and one finding I am closing.

Closed: the dual-reader P2. TRANSCRIPT_MESSAGE_KEY_SQL is gone, and with it the SQL side's own definition of what counts as a transcript row and its visibility filters. The SQL layer no longer produces rows, so there is no second definition left to drift. What remains is landmark reading the first text/user event for a scrollbar label (:188-194) — a lookup, not a row definition. This was fixed at the root rather than patched, which is the outcome that finding asked for.

Worth noting alongside it: compareRuntimeReadModelMessages was deleted rather than promoted to a production comparison. That is consistent — with one definition there is nothing to compare — but it does mean the safety net is gone if a second definition ever returns.

Still open, carried to this head:

  • The [P1] on the interrupted old conversion. Re-measured on this head rather than assumed: the old complete answer is still absent from the new reader; the opening-only control passes. This was worth re-running rather than transferring — the converter file is byte-identical to e482026d, but the read side changed substantially, and this finding's symptom is defined on what the reader shows.
  • Two conversion P2s — the allocations that still scale with the Session (carried across pages for a single long turn; the unbounded retry inventory), and restart still rescanning from sequence zero. runtime-ledger-repair.ts is byte-identical to e482026d, so the measurements there transfer directly.
  • The test gap above.
  • The disclosure. The body still says only that losing a legacy tool card is acceptable. Seven retained runtime note kinds, every pre-existing Session a released build touched, permanent — none of that is in the description yet.

One thing the end-to-end evidence settled, in the PR's favour: a real v0.1.8 Session — three rounds, a tool call and result, a successful compaction — upgrades through the production read path with all four turns and all body and tool content intact; the 24→16 record difference is fully accounted for as four session_resume notes and four stale running statuses. That is the first direct evidence that an ordinary legacy Session converts cleanly, and it narrows the P1 to the interrupted path rather than to upgrades in general. It does not cover long Sessions, which is where the two conversion P2s live.

简体中文

在 exact head 56af740a 上的状态,以及我要关闭的一条 finding。

关闭:双读者那条 P2。 TRANSCRIPT_MESSAGE_KEY_SQL 已被删除,SQL 侧对「什么算一条 transcript 行」的自有定义与可见性过滤随之消失。SQL 层不再产出行,所以不存在会漂移的第二份定义。 残留的只是 landmark 取第一条 text/user 事件作滚动条标签(:188-194)——那是一次查找,不是行定义。这是从根上修,而不是打补丁,正是那条 finding 所要的结果。

一并记一句:compareRuntimeReadModelMessages 是被删除了,而不是被提升为生产路径上的比对。这与「只剩一份定义就无需比对」是自洽的,但也意味着:若将来第二份定义回来,那张安全网已经不在。

仍然开着、并带到本 head 的:

  • 那条关于「被中断的旧转换」的 [P1]。 它是在本 head 上重新实测的,不是假定转移:旧的完整回答仍不在新 reader 的输出中,opening-only 对照通过。这一条值得重跑而不是转移 —— 转换器文件与 e482026d 逐字节相同,但读取侧变动很大,而这条 finding 的症状恰恰定义在 reader 显示什么之上。
  • 两条转换期 P2 —— 仍随 Session 增长的分配(单个长 turn 时 carried 跨页保留、以及无界的重试清单),以及中断后仍从 sequence 0 重扫。runtime-ledger-repair.tse482026d 逐字节相同,所以那边的实测直接转移。
  • 上面那条测试缺口。
  • 披露。 正文至今只说「丢一张 legacy tool card 是可接受的」。七类保留的 runtime note、任何已发布构建碰过的既有 Session、永久性 —— 都还没有进描述。

端到端证据确实定下了一件对本 PR 有利的事: 一个真实的 v0.1.8 Session —— 三轮对话、一次工具调用与结果、一次成功压缩 —— 经生产读取路径升级后,四个 turn 及全部正文与工具内容完好;24→16 的记录差额已被完整归因为四条 session_resume 与四条过时的 running 状态。这是「普通旧 Session 能干净转换」的第一份直接证据,它把那条 P1 收窄到被中断的那条路径上,而不是笼统的「升级」。它没有覆盖长会话,而那两条转换期 P2 恰恰住在长会话上。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

Comment thread packages/runtime-host/src/server/session-transcript-reader.ts
A note the runtime writes during a turn -- context compacted, step cap
reached, the turn aborted -- is a fact of that invocation, but the only
place it could be written was the Session transcript. That left the
ledger unable to state part of what a run did, and left the importer
dropping those rows on the floor.

Notes that happen between turns belong to no invocation, so they stay
Session transcript rows. The split is by owner, not by how they render.

Generated-by: Claude Code
…g some

The converter used to answer "this row cannot be recovered losslessly" by
writing nothing, which loses conversation the user can still read today.
Losslessness is a model-replay property, not a transcript one, so the rows
it could not replay now convert hidden: the card stays in the transcript
and no provider request is ever built from it.

A permission decision names its own tool, so it no longer needs a matching
call in the same turn; it carries the prompt's hint when nothing else
records it. A turn whose transcript never said how it ended now ends as
the failure it was, because an invocation left open is not a legal ledger
state and would strand the turn in recovery forever.

Generated-by: Claude Code
Every event id is now derived from the run it belongs to and its position
in that run, so converting the same transcript twice writes the same
events and the store keeps one copy. That is what lets an interrupted
conversion resume: a turn is skipped once its invocation has ended, and
re-derived until then. Before, a turn was skipped as soon as its opening
existed, which froze a half-converted turn in that state forever.

Maka's own history now converts whole. Only a foreign transcript stays
conversation-text: another runtime's tool calls belong to its protocol,
not to the provider this Session talks to next.

Generated-by: Claude Code
A running turn's rows came from the Session transcript store while every
finished turn's came from the RuntimeEvent ledger. That is the double
write: the same execution facts written twice so a reader could find them
in whichever place it looked.

Now one place answers. An open invocation is read the way the Host's
active overlay already read it -- arriving text presented as settled, a
step that has only thought given the empty assistant row that thinking
hangs on -- and that reading moves next to the projection so both readers
share it instead of keeping a copy each. "Still running" is the absence of
the terminal event, so it is stated on the turn record where it belongs
rather than as a transcript row.

Generated-by: Claude Code
@likun666661

Copy link
Copy Markdown
Member

Review: single authority is the right simplification, but the read and migration boundaries still need work

以下是本轮代码审查、定向验证,以及关于奥卡姆剃刀和最小正确实现的完整意见。

版本与验证范围:完整讨论及下面的动态复现基于 0d886e9f3a8ac721ffd29859e357d49aa955a610。发布评论前发现 HEAD 已推进到 4cddf6f15ec15e066dab0149475458ca19600e76,已检查两者之间的增量:该提交补回 WorkHub Coordination Session 自有记录的读取,并抽取共用分页逻辑;没有修改下面指出的 runtime-ledger-repair.tsruntime-transcript-query.ts 关键路径。未在新 HEAD 重跑测试,也不声称已完成新 HEAD 的全量复审。 下列代码链接固定到实际审查版本。

总体结论:认可单一事件账本和单一展示投影的方向,也认可写入侧的实质简化;但建议先修复下面三个已复现的问题,再合并。净删行数不能替代顺序、分页成本与内存边界的证明。

1. [P1] 混合历史转换后,早期消息被排到后期消息之后

转换器的 openedAt 计算 试图通过更早的时间戳将导入轮次放到已有运行之前;但新读者使用的是 Session event ordinal,而 insertRuntimeEvent 仍将转换出的事件追加到 MAX(ordinal) + 1。时间戳不能改变这个排序。

复现使用真实 SQLite Session/Runtime stores、当前转换器和当前 durable transcript reader:

  1. 一个未标记的旧 Session,早期 old-turn 只有 transcript 中的 user、assistant、completed 状态。
  2. 后期 new-turn 已有 RuntimeEvents,同时也有旧的 transcript 行,原历史顺序是 old → new。
  3. 转换器跳过已有真实 invocation 的 new-turn,补齐 old-turn。
  4. readDurableRecords(direction: 'newer') 返回的用户消息顺序为:
sequence 16: new question
sequence 40: old question

这不是消息内容丢失,而是该 PR 所支持的“旧 transcript + 已有 ledger”混合历史的顺序错误。需要迁移持久化顺序,而不只是迁移内容或调整 openedAt。如果某类历史不在支持范围内,应在兼容边界明确拒绝,不能成功转换后静默错序。

2. [P2] 分页查询的实际工作量仍随整段 Session 历史增长

RuntimeTranscriptQuery.invocations 先遍历水位以内的会话事件并 GROUP BY e.invocation_id,再通过 HAVING 过滤 position,最后排序并 LIMIT。limit: 1 限制的是输出 invocation 数,不是查询必须处理的数据量。

使用当前 schema v17 和真实 RuntimeTranscriptQuery 在内存 SQLite 上构造历史,每轮 12 个事件、普通文本约 1 KiB,固定只取最后一个 invocation。预热后四次读取均值约为:

Session 轮次 总事件数 只取最后一轮的平均耗时
100 1,200 2.6 ms
1,000 12,000 25 ms
3,000 36,000 77 ms

这些是本机合成数据测量,不是生产延迟。重要的是增长形态与执行计划一致:

SEARCH o USING PRIMARY KEY (session_id=? AND ordinal<?)
...
USE TEMP B-TREE FOR GROUP BY
USE TEMP B-TREE FOR ORDER BY

当前 reader 每次取一个 Turn,跨多个 Turn 的页面会重复支付这个全历史成本。因此“只解码一轮”并不等于“只读取一轮的成本”。现有分页测试统计 JS JSON.parse 字节数,无法覆盖 SQLite 内部的扫描、JSON 提取、分组和排序。

建议先通过位置索引定位候选 invocation,再读取该轮事件;不要为了保留单一 projector,就接受每次分页全历史聚合。测试应覆盖实际存储查询工作量,至少包含不同历史长度下相同尾页请求的查询计划和增长趋势,而不仅是响应大小或 JS 解码量。

3. [P2] 16 MiB / 8192 events 上限在整轮载入之后才检查

RuntimeTranscriptQuery.events.all(invocationId) 取回该 invocation 的全部 payload_json,然后检查行数、累计字节并抛错。

真实 SQLite 探针构造一轮 10 个约 2 MiB 文本事件,加 opening/terminal;调用设置 maxBytes = 16 * 1024 * 1024。在拒绝前,.all() 已返回:

allowedBytes:                   16,777,216
materializedBytesBeforeRefusal:  20,973,944

这证明当前实现是“载入后发现超限”,不是有界载入。更大的合法或导入轮次可能在保护检查执行前就造成大量分配。需要让预算在读取阶段生效;批量上限、单条 payload 的大小预检和累计预算都应覆盖,不能只把 .all() 改成一个仍会先物化任意大单条记录的循环。另外字节预算应以实际编码字节计,而不是 JS 字符串长度。

4. 已确认但暂不升级为权限问题的观察:catalog projection 的补写条件不完整

recordInitialRuntimeEvent 在 prompt 已落账本后才提交 catalog projection,并在后者成功之前清除 initialRuntimeEventPending。如果在两次提交之间退出,或 catalog 提交失败,就可能出现 prompt 已存在但连接锁/预览未更新的状态。

hosted recovery 只有在 prompt 缺失、调用 recordAdmittedUserMessage 时才补 catalog。以已有 prompt、connectionLocked: false 的状态调用真实 recovery 函数并替换依赖为测试桩,观测到 catalog projection 调用次数为 0。

因此“prompt 是否需要补写”与“必须可靠的派生状态是否已经提交”不能绑定在同一个分支。建议恢复时分别判断,幂等补齐必要状态;普通侧栏预览允许暂时陈旧即可,不必为它建立复杂恢复协议。

影响限定:本轮确认的是字段一致性/恢复遗漏,没有发现当前配置入口依据该字段拒绝切换的充分证据,不将其描述为已证实的权限绕过或安全漏洞。

5. 这个 PR 解决的问题,以及为什么主方向成立

核心问题不是重复占用磁盘,而是同一个普通执行事实同时具有两套独立持久化表示:RuntimeEvent 和 session_messages。运行中的展示和完成后的历史读取依赖不同表示,崩溃、重试、重连都要求它们保持等价,因此系统又需要比较、修复、补写和回退逻辑。

正确的收敛是:

执行 → RuntimeEvent → 一个展示 projector → StoredMessage DTO

本 PR 在写入侧完成了有意义的减法:

  • AgentRunAiSdkTurnToolRuntime 不再写第二份普通 transcript。
  • 活跃展示与历史读取共享执行事实来源和投影定义。
  • 旧数据通过一个单向转换器进入账本,而不是永久保留冻结前缀或双读回退。
  • 委派关联改读已有 admission proof,恢复改读执行事实,catalog 副作用变成显式接口。
  • 删除重复的状态推断、比较/修复逻辑以及 SQL 中另一套消息语义。

StoredMessage 作为 DTO 不必删除;WorkHub 自己拥有的协调事实也不必为了形式统一硬塞进普通运行账本。最新提交为 Coordination Session 接回其自有记录,属于保留不同领域事实的必要读取,不应与“同一个普通执行事实的双重权威”混为一谈。该新增读取实现本身未在本轮重跑验证。

6. 奥卡姆剃刀:问题化简正确,不等于当前代码已经是最小正确实现

认可从“怎样维护两份记录一致”转向“为什么需要两份独立记录”。真正应该减少的是需要维护的事实定义和不变量数量,不是单纯减少文件或行数。

但以下边界不能作为复杂度被一起删掉:

  • 历史先后顺序。
  • 实际存储读取、解码、内存占用的上界。
  • 崩溃后关键派生状态的恢复。
  • 单向迁移的中断恢复与明确兼容边界。

“真实样本最大只有 420 events / 843 KB”可以帮助评估常见负载,但不能证明读取上界,也不能替代超大单轮/单条记录的测试。

PR 已披露七类旧 context note 在切换后不可见,这是一项需要明确接受的兼容代价,不是从“单一权威”自动推导出的正确性。本评论不将已披露的 note 损失重新包装成新发现,但建议保留迁移不可逆、旧版本降级不受支持以及实际损失范围的清晰说明。

7. 建议的最小正确实现

保留主线:单一事件账本 + 单一展示投影 + 必要的位置索引 + 一次性幂等迁移。 不建议推翻 PR,也不建议恢复旧 transcript fallback。

写入:执行组件只提交 RuntimeEvent。展示/预览属于派生结果,可重建,不反向成为执行恢复的第二套事实。

读取

cursor → 索引定位 invocation → 有界读取该轮事件 → 统一投影 → 页面

优先复用已有位置索引;如果现有结构无法直接定位,增加最薄的一层 invocation 位置记录,只回答“这轮在哪里、是否已结束”,并与事件写入在同一 Runtime DB 事务中维护。不要把消息内容、可见性、thinking 附着等展示规则重新复制进 SQL。索引必须能由账本重建,读取过程必须有实际生效的预算。

整轮投影的前提必须写清楚:只有明确接受单轮硬上限时,它才是足够简单的方案。如果要求过去可以打开的超大历史轮次继续可读,就需要分段投影/必要依赖定位,不能把“现在超限拒绝”说成等价行为。是否支持这种历史是产品兼容性决策,不应通过内部常量暗中决定。

迁移:只保留一个转换器。补齐事件之后,依据旧 transcript 的持久化位置和已有事件顺序,幂等建立统一 ordinal 顺序,再校验投影并完成切换。迁移期间阻止该 Session 的新执行和对外发布半成品顺序;完成后 ordinal 不再改变。任一步中断重跑必须得到相同结果。不要把这个要求误解为新增跨数据库事务:当前 Session metadata 与 Runtime 不是一个可直接共同提交的本地事务,需要明确提交顺序和幂等重试边界。

恢复:补 prompt 和补关键派生状态分别判断;prompt 已存在不意味着 catalog 提交已经完成。普通预览可以 fail-open。无需为此重建 transcript 双写或增加通用“待补写消息”系统。

暂不增加:完整消息物化表、永久旧数据回退、SQL/TS 双投影、通用迁移框架,以及仅为降低一次性转换重试 I/O 而新增的复杂持久化游标。转换目前会等待全历史、重试会重新扫描,这是一次性代价,是否优化应由测量决定;但不能把单轮无限保留当作已经有了内存上界。

8. 验证与补测建议

本轮在 0d886e9f3 上通过了 183 项现有定向测试:

  • runtime-ledger-repair.testruntime-event-read-model.testsession-manager-terminal-ledger.test:121 项。
  • session-transcript-reader.testsession-catalog-coordinator.testworkhub-message-assignment.test:62 项。

另运行了上面的混合历史、真实 SQL 查询成本/计划、超限前物化量探针,以及使用真实 recovery 函数的依赖桩测试。没有修改业务代码。

Core/Storage/MCP 构建通过;Runtime/Runtime Host 完整构建受到本机复用依赖缺失(Slack、systeminformation)和代理库类型版本不匹配阻塞,不把这些环境错误归因于本 PR。未运行桌面 E2E、完整工作区测试或生产数据探针。

建议新增能直接区分正确/错误实现的回归测试:

  1. 纯旧、纯新、混合历史,转换及中断重跑后顺序一致。
  2. 不同 Session 历史长度下,同一尾页请求不进行全历史分组。
  3. 超事件数、超字节数和超大单条 payload,在超过预算前停止物化。
  4. prompt 提交后、catalog 提交前中断,恢复补齐必要状态但不重复 prompt。
  5. 保留跨终态、重连、重启的消息身份与 cursor 测试;实际旧 Session 打开以及 WorkHub Coordination 时间线仍应做桌面验证。

最终建议:保留写入侧的减法,修复迁移顺序,补上真正有界的索引读取,并拆开恢复中的错误条件依赖。目标是最少的、但完整成立的不变量,而不是最少的代码行。


本评论由 Codex 辅助完成代码阅读和本地验证,并按维护者明确要求整理发布;不代表独立人工复审或完整 CI/E2E 通过。

…prompt's catalog facts owed

Two findings from review, both introduced by this PR's own new code.

`RuntimeTranscriptQuery#events` read a whole invocation with `.all()` and
only then refused an oversized Turn, so the limits that exist to cap what one
Turn may pull into memory were checked after that memory was already paid: a
probe materialized 20,973,944 bytes against a 16,777,216-byte budget. It now
walks the rows and refuses on the row that crosses either limit. The byte
budget also counted `payload_json.length` — UTF-16 code units — which admitted
a CJK Turn three times the size it was asked to bound; it counts stored bytes.

`recordInitialRuntimeEvent` cleared `initialRuntimeEventPending` between the
ledger append and the catalog commit, and recovery's `recordAdmittedUserMessage`
runs only when the prompt event is missing. A crash or a throw between those two
writes therefore left the prompt on the ledger with no catalog projection, and
nothing recomputed it: the connection lock is one-way, so the Session could
still rebind its LLM connection after having run a Turn. On main the transcript
row and the lock were one `appendMessage` transaction, so this window is new
here. The flag now clears only after the projection lands, and recovery commits
the projection whether or not the ledger already holds the message — it is
idempotent, the lock is one-way, and the early return on a sealed run keeps it
away from Turns that have their own assistant preview.

Two other findings from the same review are not changed here. The mixed-history
ordering claim (imported turns sorting after native ones because Session
ordinals are `MAX(ordinal)+1` regardless of `openedAt`) describes a state no
supported path produces: `sendMessage` converts inside `admitTurn`, the two
`agentId` senders target sessions created at `transcriptLedgerVersion: 1`, and
staging sessions cannot start a Turn at all. `openedAt` orders the invocation
inventory, which is what it is for. The transcript pager's `GROUP BY
invocation_id` over every ordinal within the watermark is real — 2.6/25/77 ms at
100/1,000/3,000 rounds — but removing it needs a persisted per-invocation
ordinal range, which is new state and a migration, so it is deferred rather than
folded in here.

Generated-by: Claude Code
@Astro-Han

Copy link
Copy Markdown
Contributor Author

@likun666661 Thanks — I traced all four. Two are fixed, two are not, with reasons below. Pushed as 86f1d7608.

3. Bounds checked after full materialization — fixed

Confirmed. RuntimeTranscriptQuery#events called .all() to pull the whole invocation into memory, then checked rows.length > maxEvents, then accumulated bytes over rows that were already loaded. The limits exist to cap what one Turn may pull into memory, so the check ran after that memory had already been paid — which is your 20,973,944 bytes against a 16,777,216-byte budget. It now walks a cursor and refuses on the row that crosses either limit.

Your second point is confirmed and fixed with it: the budget counted payload_json.length, i.e. UTF-16 code units, not stored bytes. A CJK payload therefore admitted three times the size the budget asked for. It counts real bytes now.

Both are the letter of #4876's Bounded Read rule — "the budget applies before rows are materialized or decoded" and "measure once, carry the measurement".

Regression test bounds a transcript Turn by the bytes it stores, not by its JSON string length: a 4,000-character CJK Turn against a 6,000-byte budget, which the old code answered with Missing expected rejection.

4. Catalog projection backfill condition incomplete — fixed

Confirmed, and slightly wider than you wrote: it is not only the connectionLocked: false case. In any state where the ledger holds the prompt but the catalog does not, recovery makes zero calls.

  • agent-run.ts cleared initialRuntimeEventPending after recordRuntimeEvents but before commitMessageProjection, so finalize() would not retry;
  • hosted-execution-recovery.ts's recordAdmittedUserMessage was gated behind !verifyUserMessage(...).

What is lost is the Session-list preview line and the one-way connectionLocked latch. Losing the latch means a Session that has already run a Turn can still rebind its LLM connection. This window is one this PR introduces: on main the transcript row and the latch were a single appendMessage transaction; now the ledger append and the catalog commit are two writes.

The fix: the flag clears only after the projection lands (covering the in-process throw), and recovery commits the projection whether or not the ledger already holds the event (covering the crash). The projection is idempotent, the latch is one-way, and the early return on run.terminalEvent keeps it away from Turns that already have their own assistant preview.

Regression test startup recovery commits the catalog facts a crashed Turn wrote no projection for, which the old code answered with connectionLocked actual false / expected true.

1. Mixed-history conversion ordering — not changed; mechanism is right, the state is unreachable

Your mechanism is correct, and I confirmed both halves in the code: ordinals are COALESCE(MAX(ordinal),0)+1, and openedAt does not participate in transcript ordering. But the premise — one Session holding legacy rows for an early turn and ledger events for a later turn — is not producible on a supported path:

  • session-manager.ts:1978-1985 — a non-agentId sendMessage runs ensureTranscriptLedger inside admitTurn, so the conversion precedes any ledger append for that Session;
  • both agentId call sites (:2779, :3127) target a child Session created in that same flow, and session-store.ts:1293 creates at transcriptLedgerVersion: 1, so a legacy Session never receives an agentId send;
  • while transcriptLedgerVersion === 0 (import staging), host-session-availability.ts:66/110 reports the Session unavailable and no Turn can start.

Separately, openedAt is not dead: its consumer is ORDER BY opened_at at sqlite-runtime-store.ts:747, the invocation inventory. The comment's "ahead of the Session's own runs" is about that ordering, not the transcript's.

Your repro writes the state into both stores directly. If you can show a path that reaches it through the API, I will fix it immediately — until then I am treating it as unreachable.

2. Paging work grows with whole-session history — confirmed, tracked in #4876

The SQL does GROUP BY e.invocation_id over every ordinal row within the watermark before HAVING/ORDER BY/LIMIT, and runtime_session_event_ordinals carries only the PK (session_id, ordinal), so the temp b-tree cannot be planned away. Against main's sequence-indexed paging this is a genuine O(page) → O(session) regression, and I accept your 2.6 / 25 / 77 ms.

Not fixed in this PR: removing the scan requires persisting a per-invocation ordinal range — a new table, a schema migration, and consistency maintenance on the append path. That is its own change, and folding it into a PR already at -5657 lines makes both harder to review. The consequence is bounded (latency; no wrong state, nothing persisted incorrectly).

This is #4876's "a sessionId is not a bound — one session also grows without limit", so I am tracking it there rather than opening a parallel issue.

Verification: @maka/storage 1116 / @maka/runtime 3256 / @maka/runtime-host 1743, all green. You noted you had not re-run tests on the new head — 86f1d7608 is current, and a second look at the shape of fixes 3 and 4 would be welcome.

中文

四条都查了,两条修了两条没修。已推到 86f1d7608

3. 上限在全量物化之后才检查 —— 已修。 .all() 先把整个 invocation 读进内存,再判 rows.length > maxEvents,再在已加载的行上累加字节;护栏的目的是限制单个 Turn 拉进内存的量,检查点却在这份内存已经付掉之后,就是你测到的 20,973,944 对 16,777,216。改成游标逐行走,越界那行就抛。第二点一并修了:预算用的是 payload_json.length(UTF-16 code unit)而非存储字节数,CJK payload 会放到 3 倍。这两点正是 #4876 的 Bounded Read 原文:"the budget applies before rows are materialized or decoded""measure once, carry the measurement"

4. catalog 投影回填条件不完整 —— 已修,且比你写的更宽: 不只 connectionLocked: false,而是 ledger 有 prompt、catalog 没有的任何状态,恢复侧一次调用都不发。agent-run.ts 在投影之前就清了 initialRuntimeEventPendingfinalize() 不会重试;hosted-execution-recovery.ts 又被 !verifyUserMessage(...) 挡住。丢的是 Session 列表预览行和 connectionLocked 单向闩 —— 闩丢了意味着已经跑过 Turn 的 Session 还能重新绑定 LLM connection。这个窗口是本 PR 引入的:main 上 transcript 行和闩在同一个 appendMessage 事务里,现在是两次写。改法是标志推迟到投影落地后再清(覆盖进程内抛异常),恢复侧无论 ledger 有没有该事件都提交投影(覆盖崩溃);投影幂等、闩单向、run.terminalEvent 提前返回保证不动已有 assistant 预览的 Turn。

1. 混合历史转换顺序 —— 没改,机制对但状态不可达。 ordinal 是 MAX+1openedAt 不参与 transcript 排序,这两点我确认了。但"同一 Session 里 legacy 行属于早期 turn、ledger 事件属于后期 turn"这个前提在受支持路径上产生不出来:非 agentIdsendMessageensureTranscriptLedger 放进 admitTurnsession-manager.ts:1978-1985);两个 agentId 调用点目标都是本流程新建、transcriptLedgerVersion: 1 的 child session;staging(version 0)期间 Session 判不可用发不出 Turn。另外 openedAt 不是没用,它的消费者是 sqlite-runtime-store.ts:747 的 invocation 清单排序,注释说的是那个。你的复现是直接往两个 store 写状态构造的;能给出一条走 API 到达该状态的路径我马上改。

2. 分页工作量随整段历史增长 —— 确认,挂 #4876 跟。 SQL 确实在 watermark 内全部 ordinal 行上 GROUP BY 后再 HAVING/ORDER BY/LIMIT,索引只有 PK (session_id, ordinal),temp b-tree 消不掉;相对 main 的 sequence 索引分页是 O(page) → O(session) 的实质退化,2.6/25/77 ms 我认。没在本 PR 修:要消掉扫描得持久化每个 invocation 的 ordinal 区间——新表、migration、append 路径一致性维护,是独立一次改动,塞进已经 -5657 行的 PR 只会让两边都更难 review。后果有界(延迟,无错误状态、无持久化影响)。这正是 #4876"a sessionId is not a bound — one session also grows without limit",所以在那边跟,不另开 issue。

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My earlier APPROVE was on 1491feef and does not carry to 86f1d760

Recording that first, because a stale APPROVE sitting on a moved head is readable as current-head approval and it is not one. Between the two heads, runtime-transcript-query.ts, agent-run.ts, hosted-execution-recovery.ts and session-transcript-reader.ts (+345) all changed, and two of those are the production paths my earlier findings were about. I re-derived the conclusions below on 86f1d760 rather than carrying them forward.

You asked for a second look at the shape of fixes 3 and 4. Both hold, and in each case the load-bearing part was a claim in the comment rather than the code around it.

Fix 4, write side — the retry is safe, and for a narrower reason than the comment gives

Moving initialRuntimeEventPending = false after the projection means a projection failure now leaves the flag set, so finalize() re-enters recordInitialRuntimeEvent. The comment says re-recording is free "because its id is derived and the store dedupes it." The id half is immediate. The dedupe half is conditional: insertRuntimeEvent throws already exists outside this tool transaction when the id is present and allowExactDuplicate is false, and six of the nine call sites pass false. It holds here because the generic append path passes true (sqlite-runtime-store.ts:3710) — which is the path both writers use.

That dedupe also runs assertStoredRuntimeEventEquals, so the retry has to rebuild a byte-identical event, not merely a same-id one. It does: recordInitialRuntimeEvent(this.lastTs) reads a field fixed at agent-run.ts:700, and the later writers of lastTs (:760, :789, :888) all sit downstream of the throw, so the retry cannot observe an advanced timestamp.

Fix 4, recovery side — unconditional commit is safe, and the two halves compose

event.id uses the same shared admittedPromptEventId(runId, userMessageId) derivation as the write side, so the projected message id matches what the ledger already holds — the case that makes an unconditional commit meaningful rather than corrupting. All three writes it performs are idempotent: the preview is kind: 'replace', connectionLocked is a one-way latch, and lastMessageAt is a maxTimestamp fold.

The two halves cover different failure classes and hand off cleanly. The in-process retry is wrapped in .catch(() => {}), so a projection that keeps failing is silent there — and that is exactly the state the recovery side now commits unconditionally on the next start. Before this PR the two writes were one appendMessage transaction; splitting them is what created the window, and it now has a writer on both sides of a crash.

Fix 3 — correct, with one residual left as [P3] inline

Filed on the file rather than here.

Why I am not re-approving at this head

Finding 1 is open and disputed on reachability, not on mechanism. That is a live [P1] from another reviewer, and my own rule is that only P3 remains before an approve. I take no position on it here — I have not built the API-path probe you asked for, and saying it looks unreachable without having tried would be worth nothing to you.

If it is useful I will run that probe against the paths that append RuntimeEvents to an existing session without passing through admitTurn — import-then-continue, hosted execution recovery, and the WorkHub coordination session — since your three guards are all stated on the sendMessage/admitTurn side. If a path is found, it is your fix; if none is, that is a stronger form of your claim than the argument currently in the thread.

简体中文

我先前的 APPROVE 在 1491feef,不传递到 86f1d760

先记这一条 —— 一条停在旧 head 上的 APPROVE,会被读成对当前 head 的批准,而它不是。两个 head 之间,runtime-transcript-query.tsagent-run.tshosted-execution-recovery.tssession-transcript-reader.ts(+345)都动了,其中两处正是我先前 finding 所在的生产路径。所以下面的结论我是86f1d760 上重新推的,不是顺移过来的

你希望有人再看一眼 fixes 3 和 4 的形状。两条都成立,而且两次的承重点都在注释里的断言,而不在它周围的代码

Fix 4 写入侧 —— 重试是安全的,但理由比注释写的窄

initialRuntimeEventPending = false 挪到投影之后,意味着投影失败会让标志留着,于是 finalize() 会重进 recordInitialRuntimeEvent。注释说重复记录是免费的,「因为 id 是派生的,store 会去重」。id 那一半立刻成立;去重那一半是有条件的 —— id 已存在且 allowExactDuplicate 为 false 时,insertRuntimeEvent 会抛 already exists outside this tool transaction,而九个调用点里有六个传的正是 false。这里成立,是因为通用 append 路径传的是 true(sqlite-runtime-store.ts:3710),而两个写入方走的都是这条路径。

去重时还会跑 assertStoredRuntimeEventEquals,所以重试必须重建出逐字节相同的事件,而不只是同 id。它做到了:recordInitialRuntimeEvent(this.lastTs) 读的是 agent-run.ts:700 固定下来的字段,而后面写 lastTs 的三处(:760:789:888)都在抛出点的下游,重试观察不到被推进的时间戳。

Fix 4 恢复侧 —— 无条件提交是安全的,而且两半能组合

event.id 用的是与写入侧同一个共享派生 admittedPromptEventId(runId, userMessageId),所以投影出的 message id 与账本已持有的一致 —— 这正是「无条件提交」得以成立而非造成错位的前提。它做的三个写入全都幂等:preview 是 kind: 'replace'connectionLocked 是单向闩、lastMessageAtmaxTimestamp 折叠。

两半覆盖的是不同的失败类别,而且交接干净。 进程内重试被 .catch(() => {}) 吞掉,所以持续失败的投影在那一侧是静默的 —— 而那恰好就是恢复侧现在会在下次启动时无条件提交的状态。本 PR 之前这两个写入同处一个 appendMessage 事务;把它们拆开才产生了这个窗口,现在崩溃点的两侧各有一个写入方了

Fix 3 —— 正确,残留一条 [P3],已发在行内

为什么我不在这个 head 上重发批准

finding 1 仍然开着,争的是可达性而不是机制。那是另一位评审的 live [P1],而我自己的口径是:只剩 P3 才 approve这一条我在此不表态 —— 你要的那条走 API 的路径探针我还没做,没试过就说「看着不可达」,对你没有任何价值。

如果有用,我可以把探针跑在不经 admitTurn 而向既有 Session 追加 RuntimeEvent 的那些路径上:导入后继续、hosted execution recovery、以及 WorkHub coordination session —— 因为你那三条护栏都是在 sendMessage/admitTurn 这一侧陈述的。找到一条,就是你去修;一条都找不到,那也比线程里现在这段论证更强地支持你的结论


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

Comment thread packages/storage/src/runtime-transcript-query.ts

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Publishing this one finding now rather than holding it for the full report, because it is the blocker and waiting on the remaining lanes would only delay a fix.

[P1] inline: mixed legacy/native history reorders after conversion. The mechanism was already agreed; what was open was whether the state is reachable, and it now has a counter-example built from real implementations on a released build. The inline comment carries the reproduction and why the current send-path guards do not cover it.

My earlier APPROVE was on 1491feef and I withdrew it on this head in an earlier review; this does not change that.

Settled so far on this head, so the report is not only the blocker. The sparse-ordinal read paths are sound: durableCoverage's only durable semantic was forcing dense 0..throughSequence identity, while real durable identity is ordinal * stride + offset where legitimate gaps exist by construction — treating a gap as damage would reject healthy records. Fragment continuity, payload digest, strict cross-page ordering, cursor-must-advance, anchors, and the overlay's dense-identity contract all survive its removal, and a sparse probe on a real ledger paged both directions across gaps without loss, early stop, or looping. The two review fixes on this head hold, with one [P3] already filed against the bounded-read change.

Independently re-measured on this head rather than carried forward, because the read path moved: conversion is correct at 200/500/1000/2000 turns — no cross-page loss, no duplicates, ordering monotonic, conversion time linear, memory without a knee. Per-page read cost, however, grows linearly with Session length (×2.13 per doubling; roughly a second per page at 2000 turns), which bears on the "bounded consequence" wording used to defer the paging cost elsewhere, though not on the decision to defer it.

Two lanes are still outstanding — the Coordination-Session reader and the epoch-124 mixed-version rejection — and will follow.

简体中文

先单独发这一条而不等完整报告,因为它是阻断项,等其余几条线只会拖慢修复。

行内的 [P1]:混合 legacy/native 历史在转换后错序。 机制此前已达成一致,开着的是该状态是否可达;现在它有了一个在已发布构建上、全部由真实实现构成的反例。复现,以及当前 send 路径护栏为何覆盖不到它,都写在那条行内评论里。

我先前那条 APPROVE 绑在 1491feef,我已在本 head 上发评论作废它;本条不改变那一点。

本 head 上已经落定的部分也写在这里,免得这份报告只剩阻断项。 稀疏序号读路径是成立的:durableCoverage 唯一的持久语义是强制 0..throughSequence 的密集身份,而真实的持久身份是 ordinal * stride + offset —— 合法的空洞本就由构造产生,把空洞当成损坏会拒掉健康记录。 它被移除后,fragment 连续性、payload digest、跨页严格顺序、cursor 必须推进、锚点、以及 overlay 的密集身份契约都仍在;真实账本上的稀疏探针双向跨洞分页,无漏行、无提前结束、无循环。本 head 上那两处评审修复成立,其中有界读取那条我已另发一条 [P3]。

在本 head 上重新实测,而不是把旧结论顺移过来 —— 因为读路径变了: 200/500/1000/2000 轮下转换是正确的 —— 无跨页遗漏、无重复、序单调、转换耗时线性、内存无拐点。但每页读回成本随 Session 长度线性增长(规模每翻一倍 ×2.13;2000 轮时约每页一秒),这一点作用于别处用来推迟分页成本时所用的**「后果有界」这个措辞**,但不作用于「推迟」这个决定本身。

还有两条线未回 —— Coordination Session 的读取者,以及 epoch 124 的混版本拒绝 —— 随后补上。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

Comment thread packages/runtime/src/runtime-ledger-repair.ts
…iew back

86f1d76 made startup recovery commit a crashed Turn's catalog projection
unconditionally, on the argument that the early return on `run.terminalEvent`
kept it away from Turns that already carry a newer preview. That holds only for
sealed Turns. A steering message commits its own projection on a live run
(agent-run.ts:651), so prompt -> steering -> crash -> recovery replays the older
prompt projection over the newer steering preview.

`updateCatalogProjectionSync` already refuses to move `lastMessageAt` backwards
via `maxTimestamp`; the preview was left an unconditional replace. Fixed at that
same line rather than at the recovery call site: recovery is not the only replay
path, and the ordering rule belongs to the writer that already owns the
timestamp half of it.

The connection lock still latches unconditionally, which is the part recovery
actually owes -- it is a one-way latch, so an older message can only ever set it.

Ablation: reverting the guard fails the new test with
actual 'the original prompt' / expected 'the steering said later'.

Generated-by: Claude Code
@Astro-Han

Copy link
Copy Markdown
Contributor Author

@likun666661 Correction: I was wrong to call finding 1 unreachable. It stands.

I said no supported path leaves a Session with legacy rows for an early turn and ledger events for a later one, and offered to fix it if anyone showed one. An ordinary upgrade does it:

  1. On v0.1.8, import a legacy JSONL Session — createImportedSession writes session_messages only, never the ledger.
  2. Still on that build, send a message. The double write of the day gives that turn ledger events, so it takes Session ordinals first.
  3. Upgrade. Conversion assigns the imported turns MAX(ordinal) + 1, above the later send. The reader orders by ordinal, so the newer question renders first.

My three counter-arguments were true and beside the point: they describe what the current build can write, and this state is left behind by a previous release. I checked only the build in front of me. Verified, and I'm fixing it here rather than deferring.

Separately, my own fix in 86f1d7608 broke the Session-list preview, fixed in 41ae92cee.

I argued the early return on run.terminalEvent kept recovery's unconditional projection away from Turns with a newer preview. That holds only for sealed Turns — steering commits its projection on a live run (agent-run.ts:651), so prompt → steering → crash → recovery replays the older prompt preview over the newer one. updateCatalogProjectionSync made lastMessageAt monotonic but left the preview an unconditional replace; it now refuses to move backwards for the same reason the timestamp does. Test a replayed older message latches the connection without moving the preview back fails on the old behaviour. The connection lock still latches — that's the part recovery owes.

简体中文

更正:我判 finding 1「不可达」是错的,这条成立。

我说过没有受支持的路径能造出「同一 Session 里旧行属早期 turn、账本事件属后期 turn」,谁给出路径我就改。一次普通升级就够:

  1. 在 v0.1.8 上导入 legacy JSONL——createImportedSession 只写 session_messages,不写账本;
  2. 仍在该版本正常发一条消息——当时的双写让它拿到账本事件,于是它先占了 Session ordinal
  3. 升级。转换给导入的旧 turn 分配 MAX(ordinal) + 1,落在那次 send 之上。reader 按 ordinal 排序,新提问排在旧提问前面。

我那三条反驳本身都成立但跑题:它们说的是当前 build 能写出什么,而这个状态是上一个发布版本留下的。我只检查了眼前的 build。成立,本 PR 内修,不延后。

另外,86f1d7608 里我自己的修法弄坏了会话列表预览,已在 41ae92cee 修复。

我当时认为 run.terminalEvent 的提前返回能挡住已有更新预览的 Turn。这只对已封存的 Turn 成立——steering 会在活着的 run 上提交投影(agent-run.ts:651),于是「prompt → steering → 崩溃 → 恢复」会用较旧的 prompt 预览盖掉更新的那条。updateCatalogProjectionSynclastMessageAt 单调,却把预览留成无条件覆盖;现在预览按与时间戳相同的理由拒绝倒退。回归测试 a replayed older message latches the connection without moving the preview back 在旧行为下失败。连接闩仍照常上锁——那才是恢复真正欠的。

Conflicts and how each was settled, keeping both sides' intent:

- Protocol epoch: main took 122 for authenticated physical handoff, so this
  branch's 122/123/124 renumber to 123/124/125. Every reference is symbolic.
- SQLite runtime schema: main's v17 rewrites `runtime_continuation_claims`, so
  this branch's `runtime_events_terminal` index becomes v18.
- hosted-execution-recovery: keeps main's logical-execution read and its
  pending-handoff replay, alongside this branch's Turn-scoped prompt lookup --
  matching on the derived id is what would record a second prompt for runs an
  older build wrote under `newId()`.
- session-transcript-reader: keeps main's cross-run handoff overlay, but drops
  its local copy of `activePresentationEvents` in favour of the shared
  `activePresentationRuntimeEvents`, and reads invocations through core's
  `readRunInvocation`, which this branch made the store method optional behind.

Three of main's tests reach for what this branch retired. `appendMessage` and
`FakeBackend`'s header/store are gone from their inputs. `runtime-handoff`
sourced `copiedMessages` from `session_messages`, which is no longer written:
it now projects them from the ledger. The read model cannot serve a Session
whose run is paused mid-handoff without a terminal status, so the test projects
the user events directly rather than through the Session view -- production
reaches this copy path through `getSessionView` only after the handoff settles.

Generated-by: Claude Code
…rns were said

A Session's event ordinals are minted at append time, which is the
conversation's order for every run this build starts. It is not the order of a
turn converted from the legacy transcript: that turn was said before runs
already on the ledger, and it is appended after them. The durable reader orders
by ordinal, so a mixed Session read its older question after its newer one.

The state is reachable on an ordinary upgrade, and reviewers reproduced it end
to end: a released build imported a transcript and then sent on that Session
without converting first -- its `sendMessage` had no conversion gate -- so the
native run took the Session's ordinals while the imported turns held none. The
guards on this build's send path prevent a mixed Session from being created
from here on; they cannot retire a database an earlier release already wrote.

Fixed where the order is decided rather than at the converter: conversion ends
by renumbering the Session's ordinals in the order its invocations opened.
`openedAt` was already computed to put imported openings ahead of the Session's
own runs and no reader consulted it -- this is what makes the reader agree, so
the value stops being a second, dead expression of the same intent.

Renumbering is safe here because it is a recomputation, not a move: ordinals are
only read on this build and only through `ensureTranscriptLedgerForRead`, which
converts first, and a Session at transcript ledger version 0 cannot start a Turn
while the conversion holds its queue. Nothing has read these numbers yet.

Rejected: enforcing the rule inside `appendRuntimeEvent` so no caller has to
remember it. It would have to compare openings by wall clock, and a backwards
clock step would then reorder a live Session under readers holding its cursors.
Rejected: refusing a mixed history at the compatibility boundary, which trades a
silent misordering for a Session nobody can open.

The [P3] left open alongside this one -- a single row is decoded before
`Buffer.byteLength` measures it -- stays open. Refusing before the string
crosses into JS needs a second scan per Turn to read `octet_length`, which buys
back at most one row of the 50 KB that `tool-output.ts` already caps.

Ablation: without the renumber the new test fails with 'the newer question'
ordered ahead of 'the older question'.

Generated-by: Claude Code

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Re-reviewed at 957c2692 after the rebase and the ordering fix. The [P1] is fixed; a [P2] is open, so this is still not an approval.

The [P1] fix is verified, with a control rather than by reading. resequenceSessionEventOrdinals renumbers by COALESCE(opening.opened_at, committed_at) after conversion, inside the same transaction, which finally makes the reader agree with the openedAt the converter was already computing. I built both heads and ran the same probe — an imported Session carrying an older transcript turn plus a native invocation for a later turn, exactly the shape a released build could leave:

  • on 957c2692: the older turn reads first
  • on 86f1d760: the older turn reads last

Two caveats I would rather state than leave implicit. The control ran with the old converter and store against the newer reader, because @maka/runtime-host could not build at the old head in my environment; the module that decides ordering, runtime-transcript-query.ts, is byte-identical across both heads, so the difference measured is the fix and not the reader. And my own first two probe attempts were wrong in ways worth naming: one omitted a terminal event, so the durable page legitimately returned nothing for a still-running invocation; the other dated the native run before the Session was created, which reproduced the inversion on the fixed head and looked briefly like the fix had failed. It had not — that state cannot occur, since a send after an import is necessarily later than the import.

I also checked the failure mode the fix could introduce, because renumbering rewrites durable identity, which is an ordinal times a stride. It is unreachable: every durable read entry point awaits ensureTranscriptLedger before returning a reader, so conversion and the renumber complete before the first durable read, and no client can be holding a pre-conversion cursor.

The [P2] is filed inline. It is the Coordination Session's own admitted answers becoming unreadable after their Turn ends.

One correction to the record: the compatibility epoch on this head is 125, not 124 — and 125 is this PR's own bump, documented as transcript bootstraps dropping durableCoverage. Mixed-version rejection was re-verified against that: a real two-ended UDS probe accepts 125 with one domain call, and rejects 124, 123 and 122 with zero domain calls and zero drains, refusing after the hello and before session.run. The same-epoch positive control is what makes those zeros meaningful.

Also settled on this head: the sparse-ordinal read paths are sound, and the Coordination reader is genuinely required rather than a second authority for the rows only it can produce.

Required checks were not terminal while I wrote this; they are not being treated as evidence either way.

简体中文

在 rebase 与排序修复之后,于 957c2692 重审。[P1] 已修复;有一条 [P2] 开着,所以仍然不是批准。

[P1] 的修复是验证过的,带对照,不是靠读代码。 resequenceSessionEventOrdinals 在转换后于同一事务内按 COALESCE(opening.opened_at, committed_at) 重排,终于让读者认可转换器本来就已经算出的 openedAt。我把两个 head 都构建出来跑了同一个探针 —— 一个导入会话,带一个更早的 transcript turn,外加一个更晚 turn 的 native invocation,正是已发布构建可能留下的形状:

  • 957c2692:更早的 turn 排在前面
  • 86f1d760:更早的 turn 排在后面

两点限定我宁可明说:对照跑的是旧转换器与旧存储 + 较新的读取器,因为旧 head 上 @maka/runtime-host 在我的环境里构建不过;而决定排序的 runtime-transcript-query.ts 在两个 head 之间逐字节相同,所以测到的差异是修复本身,不是读取器。另外,我自己前两次探针都是错的,而且值得点名:一次漏写终态事件,于是持久页对一个仍在运行的 invocation 合理地什么也没返回;另一次把 native run 的时间戳放在了会话创建之前,结果在已修复的 head 上也复现了倒置,一度看起来像修复失效。并没有 —— 那个状态不可能发生,因为导入之后的一次 send 必然晚于导入本身。

我也核了这个修复可能引入的失败模式,因为重排会改写持久身份(ordinal 乘以 stride)。不可达:每一个持久读入口都会在返回读取者之前 await ensureTranscriptLedger,所以转换与重排在首次持久读之前完成,客户端不可能持有转换前的 cursor

[P2] 发在行内。 内容是 Coordination Session 自己的已准入回答,在其 Turn 结束后变得读不到。

一处事实更正: 本 head 的兼容 epoch 是 125,不是 124 —— 而 125 正是本 PR 自己的这次抬升,注释写明是「transcript bootstrap 去掉 durableCoverage」。混版本拒绝已据此重验:真实两端 UDS 探针接受 125 并产生一次 domain 调用,拒绝 124、123、122,domain 调用与 drain 均为零,拒绝发生在 hello 之后、session.run 之前。同 epoch 的正对照,才是让那些零有意义的东西。

本 head 上另外落定的: 稀疏序号读路径成立;Coordination 读取者确实是必要的,而不是它独有那些行的第二份权威。

我写这段时必需检查尚未终态;两个方向上我都没有把它当作证据。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

* `docs/architecture/workhub-coordination-session-adr.md`): the Session then
* reads like any other and this reader has nothing left to do.
*/
function createCoordinationTranscriptReader(stores: ExecutionStoresWriter<'interactive'>) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[P2] The Coordination Session now has two write substrates, and this reader sees only one of them.

Not a second authority — that part checks out. Delegation, stop, clarify and record append to session_messages under a turnId no admission minted, so the ledger has no invocation to hang them on and only this path can produce those rows. Judged by the same rule that closed the earlier dual-reader finding, this reader is required.

It is no longer sufficient. The answer_here disposition takes the ordinary root-Turn admission — #answer calls startWorkHubCoordinationMessage, which runs an AgentRun — and an AgentRun persists RuntimeEvents. It does not append a session_messages row, because this PR is what removed that second write. The neighbouring record path still calls appendMessages, and the code says the split out loud: "An answer owns its Turn identity in the root admission ledger", with record refusing a turn that already has a root admission. Exclusive writers, not a union resolved at read time.

What the user sees. While the answer's Turn is running, readActiveOverlay is not partitioned onto this reader, so it projects the ledger and the answer appears. When the Turn reaches terminal the overlay returns empty, and the WorkHub timeline falls back to this durable reader — which is scanning session_messages and has no row for what was just written. The exchange is on screen while it runs and gone once it finishes.

Classified as a regression from this change, not a pre-existing gap: before the dual write was removed, the answer's rows were in session_messages and this reader found them. Removing the second write is the right direction; what is missing is that the Coordination Session's own admitted Q&A moved to the ledger while its durable read did not.

The fix does not require restoring the dual write. It requires the Coordination read to cover both substrates for this Session, or the admitted answer to be reachable through the same projection the rest of the Session's durable history now comes from.

简体中文

[P2] Coordination Session 现在有两个写入底,而这个读取者只看得见其中一个。

它不是第二份权威 —— 这一点核过,成立。 委托、停止、澄清与 record 写入 session_messages,挂在没有 admission 铸过的 turnId 上,账本里没有可以承载它们的 invocation,且只有这条路径能产出那些行。用关掉先前那条「双读者」finding 的同一把尺子判,这个读取者是必要的

但它不再是充分的。 answer_here 走的是普通 root-Turn 准入 —— #answerstartWorkHubCoordinationMessage,后者跑一个 AgentRun,而 AgentRun 只持久化 RuntimeEvent。它不写 session_messages 行,因为正是本 PR 删掉了那次第二写。 紧邻的 record 路径仍然调 appendMessages,而代码把这个分野写在明处:「answer 的 Turn 身份归属 root admission ledger」,并且 record 会拒绝一个已有 root admission 的 turn。这是互斥的写入方,不是读时再合并的并集。

用户看到的是什么。 在这次 answer 的 Turn 运行期间,readActiveOverlay 并未被分流到这个读取者,它投影的是账本,所以回答是可见的。当 Turn 进入终态,overlay 返回空,WorkHub 时间线回落到这个持久读取者 —— 而它扫的是 session_messages,里面没有刚刚写下的那一行于是这段问答:跑的时候在屏幕上,结束之后就没了。

归类为本次改动引入的回归,而不是既有缺口:在双写被删之前,answer 的行就在 session_messages 里,这个读取者找得到。删掉第二次写的方向是对的;缺的是 —— Coordination Session 自己的已准入问答搬到了账本上,而它的持久读没有跟着搬。

修复不需要恢复双写。它需要的是:让 Coordination 的读覆盖这个 Session 的两个底,或者让已准入的 answer 能通过「该 Session 其余持久历史现在所走的同一份投影」被读到。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The mechanism holds; the reachability does not. Deferred at [P3], not fixed here.

What I verified. Everything you describe about the split is accurate: answer_here takes the ordinary root-Turn admission, startWorkHubCoordinationMessage runs an AgentRun that persists only RuntimeEvents, and this reader scans session_messages. The writers are exclusive, not a union.

Where it stops. workhub.coordination.answer has no caller. Across the repository the only references outside its own definition are two test files — nothing in packages/ui, packages/cli or apps/desktop invokes it. It is in REMOTE_OWNER_OPERATION_GRANTS, so a third-party client could reach it; no shipped one does. The reader's own doc comment says this at :563, and I re-checked it on this head rather than trusting it.

So the classification changes. Your symptom — visible while running, gone at terminal — requires a client that calls the operation. That is a contrived path today, and its consequence is display only: the answer's rows are in the ledger, intact and readable by the ordinary projection; what is missing is that the Coordination Session is routed away from that projection. Nothing is lost and nothing is unrecoverable, which is why this is [P3] rather than a merge-blocking regression.

And the end state is deletion, not coverage. Making this reader union both substrates would give the Coordination Session two read paths to keep in agreement — a second authority introduced to serve a caller that does not exist. The ADR (#3492) already requires the WorkHub to admit a Coordination Turn for every action; when it does, the Session reads like every other one and this whole source is deleted. Widening it now makes that deletion harder, not easier.

Left open in the sense that matters: the comment at :563 names the condition under which this file disappears, so the obligation is recorded where the next reader of this code will find it.

简体中文

机制成立,可达性不成立。定 [P3] 并延后,本 PR 不修。

核实到的部分。 你描述的分野全部准确:answer_here 走普通 root-Turn 准入,startWorkHubCoordinationMessage 跑的 AgentRun 只持久化 RuntimeEvent,而这个读取者扫的是 session_messages。写入方是互斥的,不是读时并集。

它止步于哪里。 workhub.coordination.answer 没有调用方。全仓中除它自身的定义外,只有两个测试文件引用它;packages/uipackages/cliapps/desktop 里没有任何调用。它在 REMOTE_OWNER_OPERATION_GRANTS 中,所以第三方客户端够得着;但没有任何已发布的客户端会走到。读取者自己在 :563 的注释就是这么写的,我在本 head 上重新核过,而不是信它。

于是分类要变。 你说的症状——运行时可见、终态即消失——需要一个会调用该操作的客户端。今天那是构造路径,且后果只在显示层:answer 的行就在账本里,完好且能被普通投影读出;缺的是 Coordination Session 被从那份投影上分流走了。没有东西丢失,也没有不可恢复的东西,所以这是 [P3],不是阻塞合并的回归。

而且终局是删除,不是覆盖。 让这个读取者并集两个底,等于给 Coordination Session 两条需要互相保持一致的读路径——为一个并不存在的调用方引入第二权威。ADR(#3492)本就要求 WorkHub 为每个 action 都准入一个 Coordination Turn;等它做到,这个 Session 就和其他 Session 一样读,整份来源随之删除。现在把它加宽,只会让那次删除更难,而不是更容易。

以真正要紧的方式留着::563 的注释写明了这个文件消失的条件,所以这笔义务记录在下一个读这段代码的人一定会看到的地方

🤖 Addressed by Claude Code

Selecting a transcript page grouped every ordinal row inside the
watermark by invocation and sorted the groups, so each page cost the
whole Session and reading one end to end was quadratic in its length.
The grouping was never needed: an invocation's first event is its
opening and its last is its terminal, and both already carry an ordinal.

Walking back — the direction history is read in — now takes the ordinal
index directly, descending from the caller's position and stopping at the
page. Walking forward cannot: "has an event at or after this ordinal" is a
claim about an invocation's ending, and no ordinal index answers it without
assuming Turns never interleave, which the runtime does not promise. That
direction reads a spine of one row per invocation instead, which costs the
Session in Turns rather than in events.

The record each page row carries is now looked up by invocation rather
than by run. Both opening shelves key on invocation_id — the ledger
through its unique partial index, the migrated shelf through its primary
key — while a run lookup had to scan the Session's openings, which left
the page cost growing with the Session even after the selection stopped.

Measured through readTranscriptInvocations on synthetic Sessions of
200/500/1000/2000 rounds, fetching the last invocation alone:
1.8 / 4.4 / 9.2 / 19.8 ms before, 0.2 ms flat after. Paging a whole
Session backwards 100 Turns per page is 8 ms per page at every length.

Generated-by: Claude Code
@Astro-Han

Copy link
Copy Markdown
Contributor Author

@likun666661 Finding 2 (transcript paging is O(session) per page) — fixed in 16f70e872.

Fixed in 16f70e872. The grouping is gone: an invocation's first event is its opening and its last is its terminal, both already carrying an ordinal, so walking back takes the ordinal index directly and stops at the page. Walking forward still reads a spine of one row per invocation — "has an event at or after this ordinal" is a claim about an invocation's ending, and no ordinal index answers it without assuming Turns never interleave, which the runtime does not promise.

One more term was hiding behind it: each page row's invocation record was looked up by run_id, which scans the Session's openings. Both opening shelves key on invocation_id — the ledger through its unique partial index, the migrated shelf through its primary key — so it now looks up by that instead.

Measured through readTranscriptInvocations at 200/500/1000/2000 rounds, fetching the last invocation alone: 1.8 / 4.4 / 9.2 / 19.8 ms before, 0.2 ms flat after. Paging a whole Session backwards at 100 Turns per page is 8 ms per page at every length.

main's #4951 gave failed turns canonical retry facts and moved the
`turn_state` projection onto the terminal RuntimeEvent, which is the same
seam this branch retires. Ten files conflicted; every resolution below.

Kept from main, adapted to the ledger:

- protocol/index.ts — main took epoch 123, so this branch's three epochs
  renumber to 124/125/126 and the constant becomes 126.
- protocol/session-turns.ts — a Turn record now carries `retry`. It comes
  off the recorded state, not from contribution shape flags: this branch
  stopped sending those (epoch 125), so `partialOutputRetained` reads the
  state alone and falls back to false only for a `turn_state` written
  before the field existed.
- runtime-event-read-model.ts — the ledger projector emits main's `retry`
  and keeps computing `partialOutputRetained`. main could drop that
  computation because its record derives the fact from contribution
  flags; with those flags gone, the state message is the only place left
  that can carry it, so the tests main changed to expect its absence are
  restored to expect it.
- ai-sdk-backend.test.ts — main's truncated-stream test parameterized
  over text and tool activity, without the retired `appendMessage` dep.

Kept from this branch, dropping main's additions:

- session-projection-helpers.ts, runtime-kernel.ts, session-manager.ts —
  `buildTurnStateMessage`, `appendTurnState` and the stop path's
  turn-state projections write to `session_messages`, which no longer
  feeds any reader here. The facts they carried are on the ledger.
- runtime-event-read-model.ts — main's `countSemanticMessages` and its
  semantic-key helpers exist for `compareRuntimeReadModelMessages`, whose
  legacy side this branch removed; nothing calls it on the merged tree.
- session-store.test.ts — main's turn-contribution, landmark and read-marker
  tests exercise `session_messages` APIs this branch removed.
- ai-sdk-backend.test.ts — main's idle-timeout test asserts on an
  assistant `appendMessage` failure, a write that no longer happens.
- session-manager-terminal-ledger.test.ts — main's rewrite reads the
  `turn_state` back out of `session_messages`; it reads through
  `getMessages` instead, keeping main's `stream_truncated` class, retry
  fact and cold-projection equivalence.

Full build, lint and tests pass across every workspace.

Generated-by: Claude Code

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed at a1db59a0b. Checks are terminal green. Two [P2]s are filed inline, so this is not an approval; one of them is the merge question you asked about, and the answer is yes.

The paging rewrite does what it claims, and I measured it rather than read it. Backward paging is flat at 19 / 18 / 16 ms per page across 200 / 500 / 1000 turns on this head, against 79 / 230 / 489 for the same probe on 86f1d760. That is the quadratic full-read gone.

The ordering fix from the previous round also still holds, and its own new failure mode — renumbering rewrites durable identity, which is an ordinal times a stride — remains unreachable, because every durable read entry point awaits the conversion before returning a reader. What the renumber costs, however, is the second [P2].

Also verified and sound: switching the per-page record lookup from run to invocation is safe. The fork is real — hosted handoff mints separate successor run and invocation ids, so the "one identity" assumption does not hold there — but the store enforces one invocation per run at append, so both filters name the same opening even when the strings differ. Probed with an equal pair and a handoff-style unequal pair, in both paging directions, including the cross-key negative.

Still open and not covered here: whether the Coordination Session's admitted answers became readable again, and a full audit of the seventeen conflicts beyond the one filed above. Those follow.

One correction to something I said earlier in this review's history: I reported at one point that conversion of a 500-turn Session exceeded ninety seconds. That was wrong — the command I used to bound it does not exist on this machine, so it never ran. The measurements above are from re-runs.

简体中文

a1db59a0b 上审查。检查已终态转绿。两条 [P2] 发在行内,所以这不是批准;其中一条正是你问的那个合并问题,而答案是「是」。

分页重写确实做到了它声称的事,而且我是测的不是读的。 本 head 上向后翻页在 200 / 500 / 1000 轮下是每页 19 / 18 / 16 ms,而同一探针在 86f1d760 上是 79 / 230 / 489。那条二次的全量读消失了。

上一轮的排序修复也仍然成立,它自身那个新失败模式 —— 重排会改写持久身份(ordinal 乘以 stride)—— 依然不可达,因为每个持久读入口都会在返回读取者之前 await 转换。但这次重排的代价,就是第二条 [P2]。

另外核过、成立的: 把每页的记录查找从 run 换成 invocation 是安全的。这个分叉是真实存在的 —— 托管 handoff 会铸出各自独立的后继 run 与 invocation id,所以「同一身份」的假设在那里不成立 —— 但存储在 append 时强制「一个 run 一个 invocation」,所以即便字符串不同,两种过滤命中的也是同一条 opening。 已用「相等的一对」与「handoff 式不相等的一对」在两个分页方向上探过,包含交叉用错 key 的反例。

仍然开着、本次未覆盖的: Coordination Session 的已准入回答是否重新可读,以及十七处冲突中除上述之外的完整审计。随后补上。

更正我在本轮过程中说过的一句话: 我一度报告「500 轮会话的转换超过九十秒」。那是错的 —— 我用来限时的命令在这台机器上并不存在,所以它根本没有运行。上面的数据来自重跑。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

Comment thread packages/runtime/src/runtime-event-read-model.ts Outdated
Comment thread packages/storage/src/sqlite-runtime-store.ts

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A third [P2] on a1db59a0b, and the closing report on the two dimensions that were still outstanding.

The conflict audit came back clean, and it was verified by mutation rather than by reading. The #4951 retry fact survives the whole production chain — ErrorEvent → mapper content → the single failed terminal RuntimeEvent → cold read-model turn_stateTurnRecord → Host, shared transcript and UI. Removing the mapper copy alone, and then the read-model copy alone, each built successfully and each made the terminal-ledger test fail with retry: undefined; restoring both passed. The agent-run.ts status deletion is not a lost invariant either: the mapped error event is non-terminal, the terminal fact arrives on the following complete(error), and that path already commits the same Session transition — main's extra updateStatus was duplicate. Every main fact in the seventeen blocks is either retained directly or moved onto the ledger authority; what was dropped is duplicate session_messages projection and scanning.

So the merge question has two different answers, and they are not in tension: the retry facts came through intact; the retained-output resolution did not. Only the second is filed.

The paging regression sweep is otherwise clean. Sequential ledgers page correctly in both directions across every shape probed — page size one, a single invocation, a page boundary landing exactly on a Turn, a Turn spanning pages, an unfinished Turn correctly excluded from durable, two Turns opening and closing in order, and a nested subagent filtered out by inline selection. Unique record counts match projected counts in both directions, and walking outward from a hole in the ordinal * stride + offset space loses nothing, stops nowhere early, and does not loop.

The one shape that fails is nesting, and it is filed inline.

简体中文

a1db59a0b 上的第三条 [P2],以及此前两个未决面的收尾报告。

冲突审计结果干净,而且是用变异而非阅读验证的。 #4951 的 retry 事实在整条生产链上存活 —— ErrorEvent → mapper 内容 → 唯一那条 failed 终态 RuntimeEvent → 冷读模型 turn_stateTurnRecord → Host、共享 transcript 与 UI。单独删掉 mapper 那份、再单独删掉读模型那份,各自都能构建成功,并且各自都让终态账本测试以 retry: undefined 失败;两处恢复后通过。agent-run.ts 里那处 status 删除也不是丢失的不变量:被映射的 error 事件是非终态的,终态事实随其后的 complete(error) 到达,而那条路径本就提交同一次 Session 状态迁移 —— main 那次额外的 updateStatus 是重复的。十七个冲突块里,每一项 main 的事实要么被直接保留、要么被搬到账本权威上;被丢掉的是重复的 session_messages 投影与扫描。

所以那个合并问题有两个不同的答案,而它们并不冲突:retry 事实完整穿过来了;保留输出的那次处置没有。只有后者被立项。

分页回归面除此之外是干净的。 顺序账本在所有探过的形状上双向分页都正确 —— 页大小为一、单个 invocation、页边界恰好落在一个 Turn 上、一个 Turn 跨页、未结束的 Turn 被正确排除在持久之外、两个 Turn 顺序开闭,以及一个被 inline 选择滤掉的嵌套 subagent。两个方向上唯一记录数都等于投影条数,而从 ordinal * stride + offset 空间中的空洞向外走,不漏行、不提前结束、也不循环。

唯一失败的形状是嵌套,已发在行内。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

Comment thread packages/runtime-host/src/server/session-transcript-reader.ts Outdated
…g it

`resequenceSessionEventOrdinals` reads its new ordinals from a windowed
`ordered` CTE inside a correlated subquery, one lookup per row. SQLite
treats such a CTE as a view by default and re-evaluates the whole
`ROW_NUMBER() OVER (...)` for every row it is asked about, so a repair
costs the Session squared: 5.2 s at 500 rounds and 22.9 s at 1000.

`AS MATERIALIZED` computes it once into a transient table. Same rows,
same order, same output: 84 ms and 357 ms for the same two Sessions.

Generated-by: Claude Code
Backward paging already walked openings on `runtime_session_event_ordinals`
and stopped at the page. Forward paging could not join it: "has an event at
or after `position`" is a claim about where an invocation *ends*, and the
query answered it by materializing a spine of every opening and every
ending in the Session. A page then cost the Session in Turns — 156, 235 and
552 ms per page at 200, 500 and 2000 rounds.

An ending is an event of its invocation like any other, so it has an ordinal
too. Forward paging now walks endings ascending from `position` and stops at
the page, the mirror of what backward paging does with openings: 9-11 ms per
page, flat across the same Session lengths. `highWater` is the first row of
that same walk taken descending, and `landmarks` — which samples the whole
Session by definition — keeps its full pass without a spine to build it on.

Neither direction assumes Turns do not overlap. That mattered: the runtime
permits concurrent visible invocations (RuntimeKernel holds a Set of
execution claims per Session and only the Host coordinator admits one root
Turn at a time), and `conversation-copy` has fixtures that interleave two of
them deliberately. A reader that assumed otherwise would silently drop a
Turn from a forward page.

Two things fall out of the rewrite:

- The SQL lineage predicate now matches `isSessionInlineInvocation`, its TS
  twin: `source.kind <> 'fresh'` rather than `= 'continuation'`. The old
  form excluded handoff-sourced continuations the TS side includes.
- `readInvocationOpeningsSync`'s outer `invocation_id`/`run_id` filter was
  dead — both UNION branches already filter on those columns inline.

The new test asserts the property rather than a duration: it runs
EXPLAIN QUERY PLAN on every statement the three bounded reads actually
execute and refuses any full scan. It fails on the spine with `SCAN spine`.

Generated-by: Claude Code
`partialOutputRetained` claimed a Turn kept some of its output. It was
derived twice and consumed nowhere.

Twice: `deriveTurnRecords` recomputed it from the Turn's own assistant and
tool_result rows and OR'd that with whatever the `turn_state` message
carried, while the ledger projector computed the same predicate over the
same rows a second time to fill that message in. Two authorities for one
fact, reconciled by an OR — the shape that survives only because nobody
checks whether they agree.

Nowhere: no renderer, presenter, or decision path reads `TurnRecord`'s or
`TurnStateMessage`'s copy. The one place that looked like a consumer —
`describeFailedTurnExecutionState` — already ignored the hint and derives
its guidance from tool activity counts; its test said so.

Removing it takes out the projection in `runtime-event-read-model`, the
default in `runtime-read-model`, the wire projection in `session-turns`
and `shared-session-transcript`, the field on both core types, its shape
and decoder entries, and the constant `true` the three external session
adapters wrote. Nothing replaces it: a Turn's retained output is its rows,
which the transcript already carries.

Epoch 127 — older peers require the field on `turn_state` and on the Turn
contribution.

Generated-by: Claude Code

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed at 8e20e6d5a. One [P2] inline. Two things about this head first, because they bound what any review of it can say: there are no check runs on this commit at all, and the branch is CONFLICTING. Local suites passing is a claim; resolving the conflict will move the head, and these conclusions bind to this SHA.

All three of the previous round's [P2]s are addressed.

The renumber is materialized, and the forward-paging rewrite works: the nested shape that lost a Turn on the previous head now returns both, at three levels of nesting too. The rewrite is also better than its own commit message claims — deleting settledInline did not merely move the "must be settled" predicate somewhere else; an invocation with no ending has no row in an ending walk and fails NULL <= position in an opening walk, so the filter and the index became the same thing. That is complexity removed rather than relocated.

Worth recording how the second fix was reached, since it is the part that will matter to whoever reads this later: the first attempt pushed a "one visible Turn at a time" invariant into the store, and an existing fixture that interleaves two visible root runs falsified it. Withdrawing it rather than weakening the fixture was the right call — the alternative traded a transcript that is slightly wrong for a Session that can lock permanently after a failed recovery.

One correction I owe on my own reasoning. I suspected the forward walk was ordering pages inconsistently, because it selects by ending and then sorts by opening within the page. Two Turns do come back in different orders through the two directions — [B,A] forward against [A,B] from the reversed backward walk — but that is ending-ascending against opening-descending over the same set, not a dropped Turn. I would have filed that as a defect; it is not one. The probe I wrote to check it also failed its own baseline, returning one record twice at a page size of two, so none of its numbers were usable.

Also verified independently: the plan ratchet does what it says — the three bounded reads execute six statements with no SCAN, and the previous spine query fails it with SCAN spine; landmarks samples the whole Session by design and is outside that scope rather than a gap in it. The SQL lineage predicate now matches its TypeScript twin across all six source/agent combinations, and the old = 'continuation' form did miss handoff-sourced continuations without over-including subagents now.

Not filed, and I agree with the reasoning: narrowing isRuntimeHostedRootAuthority and making the message authority required stay out. runtime is a published package, so the absence of external embedders cannot be shown from inside this repository — that is an unprovable premise, not an unwillingness — and changing it would churn 171 test constructions for it. Worth writing that judgement into the body or those call sites, so the next reader does not rediscover and re-litigate it.

简体中文

8e20e6d5a 上审查。一条 [P2] 发在行内。先说这个 head 的两件事,因为它们限定了任何针对它的审查能说什么:这个 commit 上完全没有 check run,而且分支是 CONFLICTING 本地套件通过是主张;解冲突会移动 head,而这些结论绑定在这个 SHA 上。

上一轮那三条 [P2] 都已处理。

重排已物化,前向分页重写有效:在上一个 head 上会丢一个 Turn 的嵌套形状,现在两个都返回,三层嵌套亦然。 而且这次重写比它自己的 commit 信息说得更好 —— 删掉 settledInline 并不只是把「必须已终结」这个谓词挪到别处;没有 ending 的 invocation 在 ending 走查里根本没有行,在 opening 走查里又过不了 NULL <= position,于是过滤与索引合成了同一件事。这是复杂度被削减,而不是被搬运。

第二个修复是怎么得到的,值得记下来,因为这是日后读到它的人真正会用到的部分:第一次尝试是把「同时只有一个可见 Turn」的不变量下沉到 store,而一个故意交错写两个可见 root run 的既有夹具证伪了它撤回它、而不是削弱那个夹具,是对的 —— 另一条路是拿「transcript 略有出入」去换「会话在一次失败恢复后可能永久锁死」。

我欠一条对自己推理的更正。 我曾怀疑前向遍历跨页顺序不一致,因为它按 ending 选页、再在页内按 opening 排序。两个 Turn 经由两个方向确实会以不同顺序回来 —— 前向 [B,A] 对上反转后的向后 [A,B] —— 但那是 ending 升序对 opening 降序、作用在同一个集合上,不是丢了 Turn。我原本会把它当成缺陷发出去;它不是。 我为此写的探针自己的基线也没过(页大小为二时同一条记录返回了两次),所以它的数字一个都不能用。

另外独立核过的: 那个执行计划闸门名副其实 —— 三个有界读实际执行六条语句、无 SCAN,而先前的 spine 查询会以 SCAN spine 失败;landmarks 按设计对整个 Session 取样,属于该范围之外,而不是其中的漏洞。SQL 的谱系谓词现在与它的 TypeScript 孪生体在全部六种 source/agent 组合上一致,而旧的 = 'continuation' 形式确实漏掉了 handoff 来源的 continuation,如今也没有反向多收 subagent。

未立项,且我同意其理由: 窄化 isRuntimeHostedRootAuthority、把 message authority 改为必填,都不进这个 PRruntime 是发布包,外部嵌入者的不存在无法从这个仓库内部证明 —— 那是一个证不了的前提,不是不愿意改 —— 而为它去改动会搅动 171 处测试构造。建议把这个判断写进正文或那些调用点,免得下一位读者重新发现、并重新纠结一遍。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

Comment thread packages/runtime-host/src/server/session-transcript-reader.ts Outdated

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A [P1] on this head, filed inline: the retained-output deletion is correct in intent but changes a persisted shape, and released rows carrying the old key now fail to decode entirely.

Everything else in that deletion is clean. A live search across apps/ and packages/ finds only the epoch comment — no empty shell left in the RuntimeEvent projection, the Host Turn and contribution wire, shared pages, Desktop, the UI, or current external adapters. A half-removed field would have been worse than either keeping or removing it, and that did not happen.

The epoch bump was verified against a real socket, with the positive control that makes the zeros mean something: an epoch-126 peer is refused, the transport closes, the business handler count is zero and beginDrain is zero, and the Host stays ready; an epoch-127 peer is accepted and the same catalog request reaches exactly one counted business handler with drain still zero.

The materialized renumber was checked for correctness rather than speed, and holds. The shift and the renumber are separate statements inside one write transaction, so the materialized CTE can only observe the shifted table and an interruption rolls back both — there is no committed "shifted but not renumbered" state to resume from. Tie-breaking is stable because the same constant is added to every row, leaving the third sort key's relative order intact; that was confirmed on a probe built specifically to make the tie-break load-bearing, with several invocations sharing one openedAt. Repeating the resequence twice produced an identical ordinal-to-event list, and empty and single-row Sessions behave.

One coverage boundary worth recording rather than filing: there is no direct contract test for the resequence itself — only implementation and interface references. That predates the one-word change and is not a regression from it, but it is why the correctness above rests on probes rather than on the suite.

An attribution correction to my previous comment on the backward-paging finding. I wrote that the resumption logic came in earlier in the same PR, which is right, and I should sharpen what that means: none of this ledger-Turn paging exists on main, so it is not repository-pre-existing in any sense. The two halves that combine into the dropped rows are both in this PR — the resume-from-record-sequence with sequence <= position, and the one-Turn-at-a-time backward walk. The commit that rewrote forward selection touched neither.

简体中文

本 head 上一条 [P1],已发在行内:保留输出的删除在意图上是对的,但它改变的是一个持久化形状,而携带旧键的已发布行现在完全无法解码。

这次删除的其余部分是干净的。apps/packages/ 上做实时搜索,只剩下 epoch 那处注释 —— RuntimeEvent 投影、Host 的 Turn 与 contribution 协议线、共享页、Desktop、UI 以及当前的外部适配器里,都没有留下空壳。 一个删了一半的字段会比「保留」或「删除」中的任何一个都糟,而那没有发生。

epoch 抬升是对着真实 socket 验的,而且带了那个让「零」有意义的正对照: epoch-126 的对端被拒、传输关闭、业务处理计数为零、beginDrain 为零,而 Host 保持就绪;epoch-127 的对端被接受,同一个 catalog 请求恰好到达一个被计数的业务处理器,drain 仍为零。

物化后的重排是按正确性而非速度核的,结论成立。 shift 与 renumber 是同一个写事务内的两条独立语句,所以物化的 CTE 只可能观察到已 shift 的表,而中断会把两者一起回滚 —— 不存在一个「已 shift、未 renumber」的已提交状态可供续跑。并列顺序是稳定的,因为每一行都加了同一个常量,第三排序键的相对次序不变;这一点是在一个专门让并列吃紧的探针上确认的(多个 invocation 共用同一个 openedAt)。连续重排两次得到完全相同的 ordinal 到事件的列表,空 Session 与单行 Session 表现正常。

一条值得记录而非立项的覆盖边界: 重排本身没有直接的契约测试 —— 全仓只有实现与接口引用。这早于这次一个词的改动、不是它带来的回归,但它正是上述正确性依赖探针而非依赖套件的原因。

对我上一条向后分页 finding 的归属更正。 我写了续读逻辑来自同一个 PR 中更早的提交,这没错,但我应当把它说得更准:main 上根本不存在这套账本 Turn 分页,所以它在任何意义上都不是「仓库既有」。 合起来导致漏行的两半都在本 PR 内 —— 一半是「从下一条 record sequence 续读」配上 sequence <= position,另一半是向后「一次一个 Turn」的走法。重写前向选择的那个提交,两处都没碰。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

Comment thread packages/core/src/session.ts
`hasExactShape` rejects any key outside `allowed`, so removing a field from a
shape stops every stored record that carries it from decoding at all. Dropping
`partialOutputRetained` did exactly that: rows written by every released build
failed `decodeMessage`, and a Session made of them could no longer be converted
or displayed. Nothing was lost on disk, but the upgrade path was.

What a shape emits may shrink freely; what it accepts may only grow. Those are
two contracts, and `defineObjectShape<T>()` derives them both from one type — so
a type change and a persisted-format change share one syntax, and `Covers` makes
the compatible option a type error inside the helper.

Name the second contract instead: a third `retired` argument, accepted on read
and dropped by `pickShape`. The obligation already had three hand-built copies
outside the helper — `withoutRetiredSubagentRuntimeKeys`, and the derived
`LAST_REQUEST_ANCHOR_DECODE_SHAPE` and `CONTEXT_BUDGET_SHAPE` — all of which
this removes. The field returns to no consumer and to no wire.

Generated-by: Claude Code
A page resumes from one record's sequence and drops everything on the other side
of it, so a scalar cursor is only sound over a stream totally ordered by that
sequence. The walk emitted Turn by Turn in opening order, which coincides with
sequence order only while Turns occupy disjoint ordinal ranges. A nested run
breaks that: the inner Turn is taken first, and the outer Turn's later rows are
then excluded by the next page's filter and never revisited. One sweep returned
them, page-size-1 did not — which is what located the defect in resumption
rather than in selection.

The invariant this relied on is the one this PR withdrew from the store, because
a fixture interleaves two visible root runs on purpose. So make emission monotone
instead of assuming disjointness: drain Turns whose ordinal ranges overlap as one
cluster, sorted. A Session without overlap yields a cluster of exactly one Turn,
and the lookahead read that ends a cluster is the next cluster's first Turn.

Generated-by: Claude Code
Four conflicts, all where main built on what this branch retires.

Epochs: main took 124 and 125 after this branch had claimed 124, so this
branch's four entries move up to 126-129 and main's two keep the numbers
their released peers already reject on.

Durable compaction notes (runtime-kernel): main writes the terminal
`context_compacted` / `context_compaction_failed_open` notes through
`store.appendMessage`, with a bounded retry and an ambiguous-write probe
because a message id is not unique in `session_messages`. This branch records
notes on the ledger inside the run, where the append is the run's own write and
the id is the event id, so the notes move to `run.recordSystemNote` and
`appendDurableCompactionNote` goes with its two retry tests. Note ordering
follows this branch's rule rather than main's: ahead of the terminal fact,
because the ledger seals on it.

`ai-sdk-backend.test.ts`: both sides rewrote the same call sites — main folded
the boilerplate into a local `createBackend`, this branch dropped the
`appendMessage` half of it. Main's wrapper is kept and this branch's semantic
edits are reapplied on top.

`tool-result-archive-evidence.test.ts` (new on main) corrupted a payload by
writing `'{'` over it. The terminal-event index this branch adds reads
payload_json, so SQLite now refuses that write outright; the reader's corrupt
branch is reached with an undecodable payload instead.

Generated-by: Claude Code
`runtime_events_terminal` is a partial index, and SQLite evaluates a partial
index's predicate over every row while building it. `json_extract` fails the
whole statement on a malformed payload, so one such row is enough to abort
migration 18 — which runs inside the upgrade's single write transaction, so the
rollback also undoes the version bump and the next open tries, and fails, the
same way. The store never reopens, and retrying cannot repair it.

No path was found by which a released build writes a malformed payload: every
writer encodes through JSON.stringify. But the costs are not symmetric — being
wrong about that costs a permanently unopenable store, and being wrong the other
way costs one clause. The guard belongs in the shared predicate rather than in
the index alone, so the index and the four queries that read it keep identical
text and the index stays usable; the semantics agree, since a row that cannot be
decoded is not a terminal fact.

Reported by review, verified here: one malformed row fails `CREATE INDEX` with
`malformed JSON` and leaves `user_version` at 17, while the guarded predicate
builds and selects the same rows over `{"status":"completed"}`, `{"status":null}`
and a payload without the key.

This also restores main's `tool-result-archive-evidence` test to its own form:
the malformed write it makes to reach the reader's corrupt branch is accepted
again.

Generated-by: Claude Code

@jackwener jackwener left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving at exact head 4e670b7ab. All required checks are terminal on this commit — thirteen success, one skipped — and every finding from the previous rounds is resolved and re-verified here rather than carried forward.

The ordering defect is fixed, and I verified it with the shape that produced it. A nested session — one Turn opening first and ending last, another opening and ending inside it — now pages identically in both directions: [48,40,32,16] walking back, [16,32,40,48] walking forward, sweep and paged equal and monotonic. The earlier failure does not reproduce.

The fix is also better than the one the finding implied. I had described a symptom — a page exhausted inside a nested Turn loses the outer one — which invites patching resumption. The change instead made emission monotonic: overlapping Turns are gathered into one cluster and sent in order, so a scalar cursor and the emitted stream agree by construction. That addresses why the cursor and the scan disagreed rather than the place where the disagreement surfaced.

The retired-key mechanism is sound and, more usefully, it names the rule it enforces. Retired keys are accepted on read and dropped by pickShape, unknown keys are still refused, and the helper's own comment states the asymmetry: what a shape emits may shrink freely while what it accepts only grows, and removing a key from optional without listing it as retired makes every stored record carrying it fail outright. That is the failure this PR hit, now expressed as a guard rather than as a lesson someone has to remember. Two of the three hand-rolled implementations are gone; the third became a call to the same helper with its retired list preserved, and the one behavioural difference that conversion could have introduced — pickShape stripping keys that were previously merged into allowed — is unreachable, since that path is not picked.

On the malformed-payload question: a single syntactically invalid payload_json does fail CREATE INDEX with malformed JSON, the failure aborts the migration and rolls back its version bump, and the next open retries and fails again. The json_valid guard belongs in the shared predicate rather than on the index, as it is here: putting it only on the index would leave the index and the four queries no longer textually identical, SQLite could not prove the partial index usable, and this branch's own plan check would fail. That the plan check would have caught the narrower fix is a good sign about the check.

This approval binds to this exact head. It is not the independent human review CONTRIBUTING.md requires, and it is not a merge decision.

简体中文

在 exact head 4e670b7ab 上批准。必需检查在本 commit 上已终态 —— 十三项成功、一项跳过 —— 而前几轮的每一条 finding 都是在这里重新验证过的,不是顺移过来的。

排序缺陷已修,而且我用产生它的那个形状验过。 一个嵌套会话(一个 Turn 先开最后终、另一个在其中开与终)现在两个方向分页结果一致:向后 [48,40,32,16],向前 [16,32,40,48],sweep 与 paged 相等且单调。先前的失败不再复现。

这个修复也比 finding 所暗示的更好。 我描述的是症状 —— 一页在嵌套 Turn 中用尽会丢掉外层 —— 那容易把人引向去补续读。而这次改动让发射单调:区间重叠的 Turn 合成一簇、按序发完,于是标量游标与发射流在构造上就一致它处理的是「游标与扫描为何不一致」,而不是「不一致在哪里显形」。

retired 键机制成立,而更有用的是它给自己所强制的规则起了名字。 retired 键读时接受、pickShape 时剥掉,未知键仍被拒;而 helper 自己的注释写明了那个不对称:形状发射什么可以自由收缩,接受什么只能扩张;把一个键从 optional 移除而不登记为 retired,会让每一条携带它的已存记录直接验证失败这正是本 PR 撞上的那次失败,如今被表达为一个守卫,而不是一条需要有人记住的教训。 三份手工实现中的两份已消失;第三份变成了对同一 helper 的调用并保留其 retired 列表,而那次转换本可能引入的唯一行为差异 —— pickShape 剥掉先前被并进 allowed 的键 —— 不可达,因为那条路径不会被 pick。

关于非法 payload 那个问题: 一条语法非法的 payload_json 确实会让 CREATE INDEXmalformed JSON 失败,失败会中止迁移并回滚其版本抬升,而下一次打开会重试并再次失败json_valid 守卫应当放在共享谓词上而不是索引上 —— 正如此处所做:只放在索引上会让索引与四个查询不再逐字相同,SQLite 便无法证明该部分索引可用,而本分支自己的执行计划检查会因此失败。那个检查本可以拦下更窄的那种修法 —— 这说明它是个好检查。

本批准绑定该 exact head。它不是 CONTRIBUTING.md 所要求的独立人类审查,也不是合并决定。


Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.

@Astro-Han
Astro-Han merged commit c58c481 into main Sep 7, 2026
14 checks passed
@Astro-Han
Astro-Han deleted the refactor/4791-single-transcript-authority branch September 7, 2026 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XXL Over 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor(runtime): derive Session transcripts from RuntimeEvents

5 participants