refactor(runtime): derive Session transcripts from RuntimeEvents - #4879
refactor(runtime): derive Session transcripts from RuntimeEvents#4879Astro-Han wants to merge 7 commits into
Conversation
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
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.userMessageIdisnullandbegin()writes its user event under a freshnewId(). - 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 finalizeFailedRunStart → failStart → finalize. 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.0–v0.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.userMessageId 为 null,于是 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 走 finalizeFailedRunStart → failStart → finalize。若 opening 已提交,第二次 openInvocation 是 no-op,随后写下一个没有 user 事件的 terminal;已封存跳过规则此后永久拒绝补写。
不对称才是要点:同样的状态下,进程崩溃能被恢复,而这条异常路径不能。 用户看到那一轮失败、可以重发,所以代价是一个永久的空封存 Run 而非丢失工作 —— 但它是两者中更常见的那一个,也是唯一无法修复的那一个。
[P2] steering 过滤后不再剩 user 的 turn 被静默丢弃
materializeTranscriptLedger 仅在去掉带 steeringEventId 的 user 行之后仍有 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.0–v0.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 事务,所以「锁已取走、投影失败」不可能发生;readMessagesForRecovery 与 readMessages 逐字节相同;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.
…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
|
Follow-up: the WorkHub linkage lane, posted separately as promised rather than amending the earlier review. Exact head 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: The identities line up because the proof is the same id. For each 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 Two limits worth stating rather than leaving implied:
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 这是描述中点名需要人工过一遍的第二项,而且是个跨 PR 的问题:#4699 的目标联动从三张生命周期表枚举被委派 Message 的身份,其中一条臂读的正是本 PR 停止写入的 transcript 行。 我担心的是升级情形,而它已经排除。 一个在本 PR 落地之前完成过委派的 Session,会有旧的 transcript 行;若 proofs 表是新建的、没有替代记录,它的联动就会静默消失。事实并非如此: 身份能对上,是因为 proof 存的就是同一个 id。 对 而且它比被取代的那条记录更合适。 三条臂现在干净地对应一条被委派 Message 可能处于的三种状态 —— 在 有两处限制,与其留作暗示不如明说:
至此本 PR 的四条车道全部报完。恢复路径上的那条 [P1] 仍是唯一需要修的东西。
|
|
Head [P1] Fixed. Confirmed, and a regression this PR introduced — on the base that branch threw and Not fixed the way you proposed: inlining Unifying it is also not sufficient. Test: [P2 seal] Fixed at 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; [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. |
|
Re-reviewed at exact head [P1] Closed, and it also covers rows already on disk
The part worth calling out is not in the summary: recovery also stopped filtering the turn's user messages down to the derived id. The mirror risk is avoided. Where [P2] Closed at the seam that made it worse than a crash
Residuals, both narrower than the hole they came from:
[P2] The unread tail is genuinely paged now, and the bound it replaces is not lost
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 [P2] The steering-only turn is now explained rather than changedThe 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. StandingThe [P1] is gone, so the objection that made this NO-GO is resolved. The PR is 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 [P1] 已关闭,而且覆盖了已经落盘的行
值得点出的一处不在摘要里:恢复侧同时不再把该 turn 的 user 消息过滤到派生 id。 镜像风险被避开了。 [P2] 关在了「它比崩溃更糟」的那个接缝上
两处残留,都比它们所出自的洞更窄:
[P2] 未读尾部确实改成了翻页,而它取代的那个上限并没有丢
我核了终止条件 —— 因为「用循环取代上限」正是这类修复容易矫枉过正的地方。 它在遇到第一条可见消息、或 [P2] 只有 steering 的 turn 现在是被解释了,而不是被改变了 该过滤仍会丢弃「唯一 user 行是 steering」的 turn,现在有三行注释说明原因:那段 steering 是说进某个已有持久 Root 所拥有的 Turn 里的,转换它就会在真实 run 旁边立起第二个合成 run。 这个理由成立,我不要求改变行为。 但把话说清楚:该过滤仍然是静默的 —— 改变的是读代码的人现在能查到原因,而不是「它触发的那个工作区会报告些什么」。 当前立场 [P1] 已消除,所以让本单成为 NO-GO 的那条反对意见已解决。该 PR 相对 先前各轮的其余结论均成立:导入器对从已发布 tag 枚举的 legacy 行类型是全的;未知类型 fail-closed 而不是截断历史;导入暂存可重入且去重已在真实 store 上验证;被删的五处第二写点其证据确在别处;WorkHub 联动的替换是安全的,包括对本 PR 之前就发生过委派的 Session。
|
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
…rest A system note stated one of two things. The ones that describe what happened inside an invocation — compaction, context pressure, the step cap — are facts of that invocation, and now live where its facts live: the RuntimeEvent ledger, through `AgentRun.recordSystemNote` and a `recordSystemNote` hook the backend reaches like its other recorders. They stay `modelVisibility: 'hidden'`, so the reader sees them and the provider never replays them. The others said something that already had an owner. The Session header carries the mode, the model and the copy lineage; the invocation's opening fact carries its own configuration; the terminal event carries the abort and its source. `session_start`, `session_resume`, `mode_change`, `model_change`, `error` and `abort` only wrote those facts a second time, into rows nothing rendered. Their write sites are gone; the kinds stay decodable so legacy transcripts still read. Two readers depended on `session_start` as a position marker for "this revision copy admitted a turn of its own". The admission ledger answers that directly — a copy clones history but never admissions — and the Host revision coordinator, which already reads it, settles every `preparing` copy at recovery before SessionManager's duplicate check ever ran. Generated-by: Claude Code
…uthority
Every execution fact was written twice: once as a RuntimeEvent and once as a
`session_messages` row. Two authorities for the same fact means every writer has
to keep them in step, every reader has to pick one, and a crash between the two
writes leaves a Session that disagrees with itself.
This cuts the second write. The ledger is the durable record; `session_messages`
survives only as input to the one-way importer that converts a pre-ledger
transcript on first read, and as the WorkHub Coordination Session's own store,
which is out of scope here.
What moved:
- `markMessagesHandedOff` no longer projects admitted Messages into transcript
rows. It validates the admission and retires it; the durable proof the rows
used to carry already lives in the agent-run admission's `sourceMessages` and
in the RuntimeEvent steering proof.
- Catalog projection (`lastMessagePreview`, `lastMessageAt`, `connectionLocked`)
is committed by `AgentRun` through `commitMessageCatalogProjection` instead of
falling out of a transcript insert. It is 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 consumer, so `hasUnread` is the only decision left:
it clears when the client has caught up with the ledger's newest visible
message, read off a bounded tail of the last run.
- 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 has
no single admitted Message identity, so the prompt is durable under a derived
`${runId}-admitted-prompt`, which makes recovering the same crash twice a
no-op append. 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 (#4699) enumerated a delegated Message's identity from
three lifecycle tables, one of which was the transcript row this change 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` (identical to
`readMessages`), `listForRecovery`'s separate query, the transcript-ordering
privates in the SQLite store, and `buildTurnStateMessage` with its lineage types.
Ablations kept out: an `existing.some(role !== 'system')` guard in recovery on
top of the terminal-event check (the terminal check alone is exact), and
removing the singular `appendMessage` (pure test churn for no production gain).
Verified on this base: storage 1092 pass, runtime-host 1709 pass, runtime 3129
pass, core 821 pass, cli 805 pass; 0 failures.
Closes #4791
Generated-by: Claude Code
…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
7c2c74e to
82325ad
Compare
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
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
testis 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.ts、agent-run.ts、hosted-execution-recovery.ts(P1 修复)、runtime-ledger-repair.ts(steering-only turn 的理由)、sqlite-session-metadata-store.ts(WorkHub 联动那条臂)。
session-catalog-coordinator.ts 确实不同,所以我是读了它而不是数它:差异是从 main 带进来的导入模型候选改动(NoUsableImportModelError、ImportModelCandidate),而 #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.
2 participants
Closes #4791.
The problem
Every execution fact was written twice: once as a
RuntimeEvent, once as asession_messagesrow. 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
RuntimeEventledger the only durable transcript authority and deletes the second write.session_messagessurvives 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, which is out of scope for this issue.The six 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
markMessagesHandedOffno 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'ssourceMessagesand in the RuntimeEvent steering proof.lastMessagePreview,lastMessageAt,connectionLocked) used to fall out of a transcript insert.AgentRunnow commits it explicitly throughcommitMessageCatalogProjection: 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.lastReadMessageIdhas no client consumer, sohasUnreadis 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.${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.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 toreadMessages),listForRecovery's separate query, the transcript-ordering privates in the SQLite store, andbuildTurnStateMessagewith its lineage types.Migration
transcriptLedgerVersiondistinguishes the three states: absent means pre-ledger and converted on read,0means an imported transcript staged for conversion,1means 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.Ablations kept out
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.appendMessage: pure test churn with no production gain, so it stayed.Verification
Net -2545 lines. On this base, all five workspace suites pass with zero failures: storage 1092, runtime-host 1709, runtime 3129, core 821, cli 805.
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, and the desktop WorkHub view after a delegated Message has been consumed by a Turn.