refactor(runtime): converge AgentRun metadata into the RuntimeInvocation event spine - #4631
refactor(runtime): converge AgentRun metadata into the RuntimeInvocation event spine#4631Astro-Han wants to merge 46 commits into
Conversation
1f6d52b to
fc298bd
Compare
99a8677 to
ee51697
Compare
ee51697 to
db26383
Compare
jackwener
left a comment
There was a problem hiding this comment.
I reviewed exact head ee51697a1ae3290d7f7291b855df80fa62383009. One P1. Not approving.
Two things are independent here, and only one of them is visible from the merge box. This branch is CONFLICTING against main — runtime-event.ts, runtime-kernel.ts, ai-sdk-backend.ts, ai-sdk-compaction.ts and one test. The P1 below is separate: resolving those conflicts will not fix it. Both need a pass.
P1 — a legacy run that recorded runComposition is dropped from the inventory, and its header is deleted immediately after
Three steps that are each defensible on their own, and lose data in sequence:
- The old
AgentRunHeadercarriesrunComposition, and the ordinary provider path on today'smainwrites it — dispatch goes throughexecution-model-composition.ts:328-345,453-456, and the oldruntime/src/agent-run.ts:438-455persists it into the header. - The new legacy shape does not accept that field:
packages/storage/src/legacy-run-header.ts:78-181. backfillInvocationOpeningFacts()swallows the decode error and continues past the row:sqlite-runtime-schema.ts:623-630. The core migration then dropsrecord_json:sqlite-core-execution-schema.ts:150-152.
So the first time a user opens a workspace that ran any provider turn on current main, that run has no opening event, no legacy shelf row, and no header any more. Its RuntimeEvents survive, but listSessionInvocations() cannot enumerate it — and prior context, host recovery, inspect, conversation copy and graph traversal all start from that inventory.
This was reproduced twice, independently. A schema-15 probe on this head with two otherwise identical legal headers differing only in runComposition: the control migrates, the one carrying composition is skipped, and record_json is confirmed gone afterwards. A second reviewer, working from the exact merge base rather than from a hand-written fixture, reproduced the same split — four paired cases, two passing and two failing.
That second run is worth a note for whoever fixes this. The first attempt at this area reported the migration as clean across twelve compatibility cases. It was wrong, and the reason is the fixture: it was written to the field set the new decoder accepts, so it only ever proved that new code can read what new code writes. Old-data fixtures have to come from the exact base's real write shape, or they are green by construction.
Fix: accept and validate the field in the legacy decoder so the opening projection completes, or backfill an equivalent fact before the header is dropped — and add a regression case built from a complete base-era header. If historical composition is genuinely disposable, that is a fine answer too, but it should be stated rather than left to a swallowed decode error.
Scope of this review
The reconciliation layer really is gone, not relocated: AgentRunHeader has no references left in the repository, and computeStatusConsistency, effectiveRunHeaderFromTerminalFact, terminalRunHeaderMatchesFact, status_consistency_mismatch and the after_terminal_header_committed failpoint are all absent. The premise of the change holds.
Not covered here: the 148-file diff was not read end to end, and the semantics of the conflicting files against current main were not merged and read. Hosted checks were terminal green on this SHA.
This conclusion binds to ee51697a only. After a rebase it needs re-checking — including whether the conflict resolution changes anything above.
简体中文
我审的是 ee51697a1ae3290d7f7291b855df80fa62383009。一条 P1。 不批准。
这里有两件独立的事,而合并框里只看得见其中一件。 这个分支相对 main 是 CONFLICTING(runtime-event.ts、runtime-kernel.ts、ai-sdk-backend.ts、ai-sdk-compaction.ts 和一个测试)。下面这条 P1 是另一回事:解掉冲突不会把它修好。 两件都要单独处理。
P1:记录过 runComposition 的旧 run 会从清单里消失,而它的 header 紧接着就被删除。
三步各自都说得通,连起来丢数据:
- 旧的
AgentRunHeader带runComposition,而今天main上的正常 provider 路径会写它——dispatch 经过execution-model-composition.ts:328-345,453-456,由旧的runtime/src/agent-run.ts:438-455持久化进 header。 - 新的 legacy 形状不接受这个字段:
packages/storage/src/legacy-run-header.ts:78-181。 backfillInvocationOpeningFacts()吞掉解码错误并跳过该行:sqlite-runtime-schema.ts:623-630。随后 core migration 删除record_json:sqlite-core-execution-schema.ts:150-152。
于是,当用户第一次用这个分支打开一个「在当前 main 上跑过任意 provider turn」的 workspace,那条 run 既没有 opening event,也没有 legacy 兜底行,header 也已不复存在。它的 RuntimeEvents 还在,但 listSessionInvocations() 枚举不到它——而 prior context、host recovery、inspect、会话复制和 graph 遍历,全都从这份清单起步。
这一条被独立复现了两次。 在这个 head 上做的 schema-15 探针:两个除 runComposition 外完全相同的合法 header,对照组迁移成功,带 composition 的被跳过,随后确认 record_json 已被删除。另一位审查者不用手写 fixture、而是从 exact merge base 出发,复现出同样的分裂——四组对照,两组通过两组失败。
第二次复现里有一点值得修这个问题的人留意。这一片最初的审查结论是「迁移干净,十二项兼容用例全过」。那个结论是错的,原因出在 fixture:它是照着新 decoder 接受的字段集写的,所以它只证明了「新代码能读新代码写的东西」。旧数据的 fixture 必须来自 exact base 的真实写入形态,否则它天然就是绿的。
修法:在 legacy decoder 里接受并校验该字段,让 opening 投影得以完成;或者在删掉 header 之前回填一条等价的事实——并补一条用 base 时代完整 header 构造的回归用例。如果历史 composition 确实可以丢弃,那也是个正当答案,但应该明说,而不是留给一个被吞掉的解码错误。
本次审查的范围:那层对账逻辑确实是被删掉了,不是挪了地方——仓库里 AgentRunHeader 已无任何引用,computeStatusConsistency、effectiveRunHeaderFromTerminalFact、terminalRunHeaderMatchesFact、status_consistency_mismatch 以及 after_terminal_header_committed 失败点全部不存在。这个改动的前提是成立的。
未覆盖:148 个文件没有通读;冲突文件相对当前 main 的语义没有合并后再读。托管检查在这个 SHA 上是终态通过。
本结论只绑定 ee51697a。 rebase 之后需要重新核对,包括冲突的解法本身是否影响上述任何一条。
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
left a comment
There was a problem hiding this comment.
Re-reviewed at exact head db2638338e6991eea3f23b29a2eba5ec65989a1b. The rebase resolved the conflicts and the merge tree against main is clean. Two P1s and one P2. My previous comment was bound to ee51697a and is superseded by this one.
P1 (a) — runComposition still drops a legacy run from the inventory, and the mechanism is now pinned
Unchanged from the last head: the four files on this path — legacy-run-header.ts, sqlite-runtime-schema.ts, sqlite-core-execution-schema.ts, record-schema.ts — are byte-identical across ee51697a..db263833. The rebase did not touch this chain.
The earlier write-up said the field is missing from the legacy shape without saying why that is fatal, and reading the per-field type checks alone does not explain it — those checks never reject an unknown key. The actual step is a guard that runs before them. decodeLegacyRunHeader calls hasExactShape() first (legacy-run-header.ts:230-233), and hasExactShape() is Object.keys(value).every(key => shape.allowed.has(key)) (core/record-schema.ts:64-68). One extra key makes the exact shape false immediately, so the per-field validation never runs. Instrumenting the decoder on this head: the control header passes, the one carrying runComposition fails with Invalid AgentRun header schema, and the top of the stack is the exact-shape branch.
From there it is the same sequence: sqlite-runtime-schema.ts:623-630 swallows the error and continues, sqlite-core-execution-schema.ts:150-152 then drops record_json. A schema-15 probe on a real database confirms only the control's opening survives.
The field is not hypothetical. The new merge base and current main both still carry AgentRunHeader.runComposition in the exact allowed shape, and the ordinary provider path still produces it — execution-model-composition.ts:328-345 builds the snapshot and :453-456 commits it as beforeRunProviderDispatch.
Fix: accepting the field in the decoder is necessary but not sufficient. Without also preserving it — as the immutable run_composition_recorded fact the new live writer already emits — before the header is dropped, the invocation stops disappearing but the composition is still silently lost.
Worth noting for the regression test: the focused backfill suite is 3/3 green on this head with the bug present, because its fixtures are written to the field set the new decoder accepts. A fixture built that way can only ever prove that new code reads what new code writes. The case that catches this has to be built from a complete base-era header.
P1 (b) — the rebase dropped #4393 from ai-sdk-backend.ts
main carries bf5ca672d (#4393): MAX_SEALED_THINKING_RETRIES_PER_STEP = 1 at ai-sdk-backend.ts:899, a sealedThinkingRecovery arm in the retry loop, mutually exclusive with the idle-watchdog and truncated-stream arms, plus roughly 629 lines of tests including the ECONNRESET group.
None of that is on this head. The retry loop has only idleWatchdogRecovery and incompleteStreamRecovery. Of the four conflict-resolved files, the backend is the only one shorter than main (5068 against 5125), and the sole const in the main-only difference is that constant.
So a thinking-only stream cut by a retryable network error, before the idle watchdog fires, ends the Turn on this branch, where main flushes the partial thinking into its own message and retries once. Merging as-is reverts a fix already on main and reopens #4284.
Fix: merge #4393's backend change and its tests back in, rather than letting the PR-side file win whole.
P2 — the two-terminal rule lives only in the in-memory rebuild
Unchanged and byte-identical to the previous head. runtime-invocation.ts:74-87 clears terminalEvent when more than one terminal event is counted; the production enumeration in completeInvocationRecordSync (sqlite-runtime-store.ts:739-755) still takes ORDER BY event_seq DESC LIMIT 1 and does not count. The same ledger therefore reads as finished through the index and as open through the rebuild, and the reverse splits too when a suffix follows a single terminal.
This is the shape the PR sets out to remove — one fact, two records that can disagree — reappearing between two derivations rather than between two tables. The rest of the retirement holds: AgentRunHeader has no references left, and computeStatusConsistency, effectiveRunHeaderFromTerminalFact, terminalRunHeaderMatchesFact, status_consistency_mismatch and the after_terminal_header_committed failpoint are all gone.
Checks and scope
The required test job is red on apps/desktop/e2e/code-scroll.spec.ts, which this PR does not touch. That looks like the same class of pre-existing instability as the Runtime Host cases being fixed elsewhere, but attribution is not established here — either way the gate is not green.
The other three conflict-resolved files were checked for what main brought in rather than read end to end: runtime-event.ts keeps the form-interaction decode alongside the new opening fact; runtime-kernel.ts keeps drain-refused → cancelled and has no header references; ai-sdk-compaction.ts reads the route from invocation.opening.route and is not missing functions relative to main. Cross-process ensureTerminal contention remains untested by anyone.
简体中文
在 db2638338e6991eea3f23b29a2eba5ec65989a1b 上重审。rebase 解掉了冲突,相对 main 的合并树是干净的。两条 P1、一条 P2。 我上一条评论绑定的是 ee51697a,由本条取代。
P1(a):runComposition 仍会让旧 run 从清单中消失,而且失败机制现在定位到了。
和上个 head 相比没有变化:这条链上的四个文件(legacy-run-header.ts、sqlite-runtime-schema.ts、sqlite-core-execution-schema.ts、record-schema.ts)在 ee51697a..db263833 之间逐字节相同,rebase 没有碰它们。
先前的说法只讲了「新的 legacy 形状缺这个字段」,没讲清为什么这会致命——而且单看逐字段类型检查是解释不通的,那些检查从不拒绝未知字段。真正的那一步是排在它们之前的一道 guard。 decodeLegacyRunHeader 会先调用 hasExactShape()(legacy-run-header.ts:230-233),而 hasExactShape() 就是 Object.keys(value).every(key => shape.allowed.has(key))(core/record-schema.ts:64-68)。多出一个键,exact shape 立刻为假,逐字段校验根本没有机会运行。 在这个 head 上给 decoder 打点:对照 header 通过,带 runComposition 的那个以 Invalid AgentRun header schema 失败,栈顶正是 exact-shape 分支。
之后是同一串连锁:sqlite-runtime-schema.ts:623-630 吞掉这个错误并跳过,sqlite-core-execution-schema.ts:150-152 随后删除 record_json。在真实数据库上做的 schema-15 探针确认:最后只剩对照组的 opening。
这个字段不是假想的。新的 merge base 和当前 main 都仍在 exact allowed shape 里带着 AgentRunHeader.runComposition,普通 provider 路径也仍在产生它——execution-model-composition.ts:328-345 构造快照,:453-456 作为 beforeRunProviderDispatch 提交。
修法:在 decoder 里接受该字段是必要的,但不充分。如果不在 header 被删除之前把它保存下来——存成新的 live writer 已经在写的那条不可变事实 run_composition_recorded——那么 invocation 不再整条消失,但 composition 本身仍会被静默丢弃。
回归测试有一点值得记:在缺陷存在的情况下,聚焦的回填套件在这个 head 上仍然 3/3 全绿,因为它的 fixture 是照着新 decoder 接受的字段集写的。这样构造的 fixture 只能证明「新代码读得懂新代码写的东西」。真正能抓住这个问题的用例,必须用 base 时代的完整 header 来构造。
P1(b):rebase 把 #4393 从 ai-sdk-backend.ts 弄丢了。
main 上有 bf5ca672d(#4393):ai-sdk-backend.ts:899 的 MAX_SEALED_THINKING_RETRIES_PER_STEP = 1、retry 循环里与 idle-watchdog / truncated-stream 互斥的 sealedThinkingRecovery 分支,以及约 629 行测试(含 ECONNRESET 那组)。
这些在本 head 上全都不在。retry 循环只剩 idleWatchdogRecovery 和 incompleteStreamRecovery。四个冲突解决过的文件里,backend 是唯一比 main 更短的(5068 对 5125),而 main-only 的 const 差集里就只有那一个常量。
于是:一条 thinking-only 的流被可重试的网络错误切断、且 idle watchdog 尚未触发时,本分支会直接结束这个 Turn;而 main 会把部分 thinking 冲刷成独立消息并重试一次。照这样合入,等于撤销一个已经在 main 上的修复,并让 #4284 重新出现。
修法:把 #4393 的 backend 改动和它的测试重新合并回来,而不是让 PR 侧的整份文件覆盖掉。
P2:两条 terminal 的规则只存在于内存重建函数里。
未变,与上个 head 逐字节相同。runtime-invocation.ts:74-87 在数到多于一条 terminal 事件时清空 terminalEvent;而生产枚举所用的 completeInvocationRecordSync(sqlite-runtime-store.ts:739-755)仍然是 ORDER BY event_seq DESC LIMIT 1,不做计数。于是同一份账本,经索引读出来是「已结束」,经重建读出来是「仍开放」;当单条 terminal 后面跟着后续事件时,分裂方向反过来同样成立。
这正是本 PR 立意要消除的形状——一份事实、两份可能互相矛盾的记录——只不过它从「两张表之间」搬到了「两条派生路径之间」。 退役的其余部分是站得住的:仓库里 AgentRunHeader 已无引用,computeStatusConsistency、effectiveRunHeaderFromTerminalFact、terminalRunHeaderMatchesFact、status_consistency_mismatch 以及 after_terminal_header_committed 失败点全部不存在。
检查与范围:必需的 test 任务红在 apps/desktop/e2e/code-scroll.spec.ts,而本 PR 并未触及该文件。它看起来和别处正在修复的 Runtime Host 用例属于同一类既有不稳定,但归因在此并未确立——无论归因如何,门禁都不是绿的。
另外三个冲突解决过的文件,我核的是「main 带进来的东西还在不在」,不是通读:runtime-event.ts 在新的 opening fact 之外保留了 form-interaction 解码;runtime-kernel.ts 保留了 drain-refused → cancelled 且无 header 引用;ai-sdk-compaction.ts 从 invocation.opening.route 取 route,相对 main 没有丢失函数。跨进程 ensureTerminal 争用仍然无人验证。
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.
|
Correcting one of my own findings, at P1 (b) — "the rebase dropped #4393" — is withdrawn. The observation was right and the inference was wrong. What is true: this head does not contain What does not follow is that merging would revert it. A three-way merge of current The real exposure moves to the next rebase: if a later resolution lets the PR-side file win whole, these lines would go then. Worth re-checking at that point rather than acting on now. One thing that does deserve attention, and that this correction surfaced. A clean merge tree is not the same as correct merged semantics. In The other findings stand unchanged: the 简体中文更正我自己的一条发现,针对 P1(b)——「rebase 丢掉了 #4393」——撤回。观察是对的,推论是错的。 成立的部分:这个 head 确实不含 不成立的是「合入会撤销它」。 用当前 真正的风险转移到了下一次 rebase:如果之后的冲突解决让 PR 侧的整份文件获胜,这些行才会在那时丢失。那时值得重新核对,而不是现在就动手。 有一点确实值得注意,而且是这次更正带出来的:合并树干净,不等于合并后的语义正确。在 其余发现不受影响,维持原判:
|
db26383 to
aa0d369
Compare
|
Thanks — the composition finding was real and the fixture critique was the right diagnosis. Addressed at head P1 (a) — fixed at the migration. Confirmed on a schema-15 database built from the base's own writer: the only key P1 (b) — not a rebase loss. P2 — the split is not reachable, and the rule behind it is gone. Found by the adversarial pass, fixed in the same push:
Verified intact by the rebase-fidelity pass: #4653, #4667/#4671, #4669, #4379, #4439, #4132 — every hunk Local: core / storage / runtime / runtime-host / desktop suites green (counts in the PR body), format and lint clean. The earlier 简体中文谢谢——composition 那条是真的,对 fixture 的批评也点在了要害上。已在 P1(a)——在迁移层修。 用 base 自己的 writer 造 schema-15 库确认: P1(b)——不是 rebase 丢失。 P2——分歧不可达,背后那条规则已删。 对抗性评审发现并随本次推送修复:
rebase 保真核实无损:#4653、#4667/#4671、#4669、#4379、#4439、#4132—— 本地:core / storage / runtime / runtime-host / desktop 套件全绿(数字在 PR 正文),format 与 lint 干净。此前
|
jackwener
left a comment
There was a problem hiding this comment.
Approving at exact head aa0d369db48de080d29462eff6a9fb45ddd09ba0. Both P1s are closed. One P2 remains, and it is the one thing here I would still change before this lands.
The migration P1 is closed, and the fix went deeper than the field
legacy-run-header.ts now accepts runComposition in the interface, the allowed shape and the type check, so the hasExactShape() guard no longer rejects a header the ordinary provider path wrote. Re-run against this head, the old probe fails at its own assert.throws — the header simply decodes now. Adapted to the new behaviour: both headers project an opening, both terminal headers produce completed, and both invocations remain enumerable after record_json is dropped.
More important than the field: sqlite-runtime-schema.ts:630-649 no longer swallows the error. A header that genuinely cannot be decoded or projected now aborts the migration and rolls back, where before it was catch { continue } followed by the header's permanent deletion. That is the difference between losing a run silently and refusing to proceed — and it protects the next field nobody thought to allow, not just this one.
On the historical snapshots: they are not carried to the spine, and the migration does not fabricate a run_composition_recorded fact for them. That is now a stated choice rather than a swallowed error. The description's migration section says the new fact begins with the first run after upgrade, and on current main nothing consumes AgentRunHeader.runComposition for behaviour — it is declared, decoded, written before provider dispatch, and read by storage for write-once immutability, but no product path branches on it and the execution inspect document does not project it. A disclosed data trade-off with no present behavioural effect is a fine answer to the question I raised; it just had to be the answer rather than an accident.
The backend P1 is closed on its own terms
ai-sdk-backend.ts is now 5125 lines, matching main; MAX_SEALED_THINKING_RETRIES_PER_STEP and the sealedThinkingRecovery arm are present, and the ECONNRESET tests appear on both sides. This is the head's contents agreeing with main, which is the check that was worth making — and a reminder that my earlier claim on this point was withdrawn because a three-way merge would have preserved those lines anyway.
P2 — the two-terminal answer is still split, and the rule changed without the split closing
This is worth stating precisely, because the code did move here and it would be easy to read that as a fix.
The rebuild used to count and clear: more than one terminal event meant no terminalEvent. It is now first-wins — runtime-invocation.ts:74-81, if (record && !record.terminalEvent) record.terminalEvent = event — with a comment reasoning that the store seals a run, so at most one can exist. The index side is unchanged: completeInvocationRecordSync still takes ORDER BY event_seq DESC LIMIT 1, which is last-wins. The new readRunInvocation and runId filter do not change that.
So the rule was rewritten on one side only, and the two sides still disagree — previously "cleared versus last", now "first versus last". Probed on this head with opening → completed → failed: the rebuild reports term-completed, the last-seq rule reports term-failed.
The comment's premise is the part I would push on. If a run really can have at most one terminal event, then first-wins and last-wins are equivalent and neither side needs defending — but then terminal-run-commit.ts:62-63 would not still be marking length > 1 as ambiguous. The codebase has three readers making three different assumptions about the same malformed ledger: one treats it as impossible and takes the first, one takes the last, one detects it and refuses. Reaching that state needs a raw insert, since the write path refuses a second terminal — which is why this is a P2 and not a blocker.
The fix is the same as before: give the index completion and the rebuild one rule, whichever it is, and add the malformed-ledger case to the contract tests, which still only exercise a single terminal event.
This matters more here than it would elsewhere, because the change's whole premise is that one fact should not have two records that can disagree. That premise holds across the rest of it — AgentRunHeader has no references left, and the reconciliation layer is genuinely gone rather than relocated.
Checks
audit and windows_recovery are green on this head; test was still running when I posted. This approval covers the review gate only — branch protection still requires that job, so nothing here bypasses it, and if test comes back red this approval should not be read as covering it. This is a refactor, so the merge decision is a human's regardless.
Evidence boundary: the probes and suite runs cited above were executed against this head by the reviewers on this side, not by me; I verified the code paths they name. The merge surface against current main was checked for what main contributes rather than read end to end, and cross-process ensureTerminal contention remains untested by anyone.
简体中文
在 aa0d369db48de080d29462eff6a9fb45ddd09ba0 上批准。两条 P1 都已关闭。 还剩一条 P2,那是我在合入前唯一仍希望改掉的东西。
迁移那条 P1 已关闭,而且修得比「补一个字段」更深。
legacy-run-header.ts 现在在接口、allowed shape 和类型校验里都接受了 runComposition,所以 hasExactShape() 这道 guard 不再拒绝一个由普通 provider 路径写出的 header。旧探针在这个 head 上会在它自己的 assert.throws 处失败——因为那个 header 现在能正常解码了。改写成适配新行为后:两个 header 都产生 opening,两个终态 header 都得到 completed,删除 record_json 之后两条 invocation 仍可枚举。
比字段更重要的是:sqlite-runtime-schema.ts:630-649 不再吞掉错误。 一个确实无法解码或投影的 header,现在会让整个迁移抛错回滚;而此前是 catch { continue },紧接着 header 被永久删除。这是「静默丢掉一条 run」和「拒绝继续」之间的区别——而且它保护的是下一个没人想到要放行的字段,不只是这一个。
关于历史快照:它们没有被搬到 spine,迁移也没有为它们伪造一条 run_composition_recorded 事实。这现在是一个明示的选择,而不是被吞掉的错误。 描述里的迁移段说明新事实从升级后的第一条 run 开始;而在当前 main 上,没有任何地方依据 AgentRunHeader.runComposition 产生行为——它被声明、被解码、在 provider dispatch 前写入、被 storage 为写一次不变性读取,但没有产品路径按它分支,execution inspect 的公开 document 也不投影它。一个已披露、且对当前行为无影响的数据取舍,是我提出的那个问题的正当答案;它只需要是个答案,而不是一次意外。
backend 那条 P1 按它自己的判据关闭。 ai-sdk-backend.ts 现在是 5125 行,与 main 一致;MAX_SEALED_THINKING_RETRIES_PER_STEP 和 sealedThinkingRecovery 分支都在,ECONNRESET 测试两边都有。这是 head 的内容与 main 相符——这才是值得做的那个检查;同时也提醒一句:我先前在这一点上的说法已经撤回,因为三方合并本来就会保留那些行。
P2:两条 terminal 的答案仍然分裂,规则改了,但分裂没关掉。
这一条值得说准确,因为这里的代码确实动了,很容易被读成已经修好。
重建过去是「计数并清空」:多于一条 terminal 事件就不给 terminalEvent。现在改成了先到者胜——runtime-invocation.ts:74-81,if (record && !record.terminalEvent) record.terminalEvent = event,注释的理由是 store 会封口,所以最多只能有一条。索引侧没有动:completeInvocationRecordSync 仍然是 ORDER BY event_seq DESC LIMIT 1,也就是后到者胜。新增的 readRunInvocation 和 runId 过滤并不改变这一点。
所以规则只在一侧被改写,而两侧仍然不一致——从原来的「清空 对 后到」变成了现在的「先到 对 后到」。在这个 head 上以 opening → completed → failed 探测:重建报告 term-completed,last-seq 规则报告 term-failed。
我想追问的是那条注释的前提。如果一条 run 真的最多只能有一条 terminal 事件,那么先到者胜和后到者胜是等价的,两侧都不需要辩护——可那样的话,terminal-run-commit.ts:62-63 就不会仍然把 length > 1 标为 ambiguous。 代码库里有三个读者,对同一份畸形账本抱着三种不同的假设:一个认为它不可能发生并取第一条,一个取最后一条,还有一个能检测出来并拒绝。要造出这种状态需要 raw insert(写路径会拒绝第二条 terminal),所以这是 P2 而不是阻塞项。
修法与之前相同:让索引的完成函数和重建函数用同一套规则(用哪一套都行,但必须相同),并把畸形账本这个用例加进契约测试——现有契约测试仍然只走单条 terminal。
这件事在这里比在别处更要紧,因为这次改动的全部立意就是:一份事实不应该有两份可能互相矛盾的记录。这个立意在其余部分是站得住的——AgentRunHeader 已无任何引用,对账层是真的消失了,而不是换了个地方。
关于检查:这个 head 上 audit 和 windows_recovery 是绿的;我发布时 test 仍在运行。本批准只覆盖审查门禁——分支保护仍然要求该任务通过,所以这里没有绕过任何东西;如果 test 最终变红,不应把本批准理解为涵盖了它。这是一次重构,合并决定无论如何都在人。
证据边界:上面引用的探针和套件运行是由这一侧的审查者针对这个 head 执行的,不是我本人;我核对了他们点名的代码路径。相对当前 main 的合并面,我核的是「main 贡献了什么」而不是通读;跨进程 ensureTerminal 争用仍然无人验证。
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.
|
Following up on I approved this head earlier. That approval stands for what it examined; it does not cover the finding below, which came out of a later pass and is verified by running the real path rather than reading it. P1 —
|
|
Following up on the P1 I raised for That comment showed the ghost is read as active. It did not show that anything acts on it, and that gap was a fair challenge. It has now been closed by running the production startup path. What actually happensFull production chain: create a real active Session; create a header-era completed run with Two independent runs agree:
So this is not a cosmetic inventory problem: startup writes data back into a session the user is deleting or rolling back. The trigger is narrower than I first impliedThis requires a crash between two commits — the operational purge, and the session metadata removal. If session removal succeeds, startup recovery enumerates from Session metadata and never reaches the ghost. That window is reachable in ordinary code paths rather than contrived: Bounds of the claimNo provider or tool execution was observed. The consequence is persistent resurrection plus an incorrect terminal written back — not a restarted provider process. The fix is unchanged and still small: include Evidence boundary: both runs are probes against this head, recorded with digests; the second one drives the real 简体中文接上一条针对 上一条只证明了这个幽灵被读成 active,并没有证明有任何东西会据此动作。这是一个中肯的质疑,现在通过跑真实启动路径把它补上了。 实际会发生什么 完整生产链:创建一个真实的 active Session;造一个 header 时代、带 两次独立运行结果一致:
所以这不是清单不好看的问题:启动过程会把数据写回一个用户正在删除或回滚的会话。 触发条件比我最初表述的更窄 它需要在两次提交之间发生崩溃——一次是 operational purge,一次是 session metadata removal。如果 session removal 成功了,启动恢复会从 Session metadata 枚举,根本触及不到这个幽灵。 但这个窗口在普通代码路径里是可达的,不是硬构造出来的: 这条主张的边界 没有观察到任何 provider 或 tool 执行。 后果是持久化的复活加上一条被写回的错误终态,不是「重启了提供方进程」。修法不变,而且仍然很小:把 证据边界:两次运行都是针对这个 head 的探针,均留有摘要;第二次驱动的是真实的
|
|
One addition to the contract question on My framing was: either the state is unrepresentable, and sealing after a terminal becomes an implementation obligation of every The weakness in the first option is that this repository has no storage-level constraint behind it. The unique index covers The stronger form:
The quarantine step is the one I would not have thought of, and it matters: a database that already contains a double-terminal ledger would fail the new constraint at migration time, and a user who cannot upgrade is worse off than a user with an inconsistent read. Adding the constraint without deciding what happens to existing violations trades one problem for a worse one. The purge fix is unchanged either way: delete the shelf row in the same transaction, or leave a tombstone. None of this changes the P1 or the finding beneath it — it is about which repair to choose once the contract is settled. 简体中文针对 我原来的框架是:要么这个状态不可表示,那么「终态之后封口」就成为每个 第一条路的弱点在于:这个仓库背后没有存储层的约束。 唯一索引只覆盖 更强的形式:
其中「隔离」这一步是我原本想不到的,而且它很关键:一个已经含有双终态账本的数据库,会在迁移时撞上新约束——而一个升级不上去的用户,比一个读到不一致状态的用户处境更糟。只加约束、不决定既有违例怎么办,是拿一个问题换一个更糟的问题。 purge 那条修法不受影响:在同一事务里删掉 shelf 行,或者留一个墓碑。 以上都不改变那条 P1 或它底下的发现——这只是关于契约定下来之后,该选哪一种修法。
|
aa0d369 to
394b164
Compare
|
Both findings hold. Fixed at P1 — the shelf outliving a purge: fixed, but not in
|
Route provenance, execution configuration, root authority and lineage are immutable the moment an invocation opens, but today they only exist on the mutable AgentRunHeader, so every RuntimeEvent that needs its own route has to join back through event.runId. That join is what removed compatible provider reasoning in #4286. Add `invocation_opened` as a closed, versioned RuntimeEvent content kind. It carries the route once per invocation so readers join it by invocationId instead of copying it onto every event, and it fails closed: a route whose connection identity cannot be established decodes as `provenance: 'unknown'` rather than as an authenticated route. The root authority is a discriminated union instead of a bag of mutually exclusive optional ids, so a reader names the root it wants rather than asserting that every other root field is absent. Refs #4311 Generated-by: Claude Code
Every run-kind invocation now commits its opening fact as its own first RuntimeEvent, before the run row, before the run_created ledger row and before any provider or tool dispatch. The store already requires a continuation's start event to be event one of its target invocation, so for a continuation the start event carries the opening fact instead of a second event preceding it. The opening fact is projected from the Run header by one shared function, so the two authorities cannot disagree while both exist, and the runtime protocol marker moves with it: it has always belonged to the invocation's first event, and that event is now the opening fact. Run and invocation converge on one identity. All three sites that used to mint them independently (continuation planning, conversation copy, imported transcript repair) now emit the same value for both, and the derived `invocation-` prefix is gone. Nothing renames a field yet; this only removes the multiplicity that would have made a rename a lie. The terminal event's own error message becomes the only source of a run's failure text. It was already the source the header copy was written from, so preferring the header only let a stale projection outlive its fact. Refs #4311 Generated-by: Claude Code
Enumerating a Session's runs has only ever been possible through `core_agent_runs`, which is also the only place the opening metadata lives. That is what makes the Run header impossible to retire: it is simultaneously the authority and the index. Add `listSessionInvocations`, a query over `runtime_events` that reads each invocation's opening fact and, where the invocation has ended, its terminal event. Nothing writes it and nothing repairs it, so dropping the physical index and rebuilding gives the same inventory. It sits beside `listSessionRuns` on the same facades so consumers can move one at a time. Runtime schema 16 adds the covering index for the opening lookup and gives every header-only run the opening fact it never wrote. A run that already owns an immutable sequence is left alone: its position one, digests and coverage are already signed by other facts, so inserting into it would rewrite history rather than record it. A header the projection cannot read fails closed and is skipped, keeping its transcript and tool evidence exactly as readable as before. Refs #4311 Generated-by: Claude Code
…vents carry The v16 backfill could only give an opening fact to runs that had never written a RuntimeEvent. A run that already owns an immutable sequence cannot take one: its position 1, digests and coverage are signed by other facts, so inserting there would rewrite history. That left those invocations with their opening on the Run header alone, which is exactly the authority this work is retiring. Record their openings in `runtime_legacy_invocation_openings` instead. Only the migration writes it, it is keyed by the invocation id the invocation's own events already carry, and it holds the same projection the live writer emits, produced by the same function. `listSessionInvocations` merges the two shelves and says nothing about which one a record came from. An opening is an opening; a consumer that could tell would be encoding the migration window into its own logic, and would then have to be changed again when the window closes. Refs #4311 Generated-by: Claude Code
…he header The composition was the one header field that was neither open-time nor lifecycle: a late-bound, write-once snapshot committed as a patch. It forced the header's whole mutable surface to stay open for a value that is by construction a fact about one moment, and its immutability had to be defended inside `updateRun`, which exists for values that do change. Append it as `run_composition_recorded` instead. Same payload, same single writer, same moment before provider dispatch, and the same guard: an identical re-append is the writer retrying and is absorbed, a different one is refused. The guard moves to where the record now lives. Nothing outside the writer read `header.runComposition`, so the field and its entry in the mutable-field set go with it, and reads go through one function over the ledger. Refs #4311 Generated-by: Claude Code
…not a Run header The claim embedded a whole pre-provider Run header, a second durable copy of a record that already exists, and then had to defend the copy against the original with two deep-equality checks over live lifecycle fields. Those checks could only ever fail for the wrong reason: the run's status and timestamps move as it runs, the copy's never do. What the claim is actually for is the start event. A continuation's start is event 1 of its target, so it is also that invocation's opening fact, and the claim has to say in advance exactly what that fact will be. So the claim carries the opening and nothing else. Everything the old header said is either the claim's own target identity, its `claimedAt`, or a restatement of its boundary, so the header is rebuilt from the claim where a header is still needed, and admission now round-trips through that rebuild — the run it computes must equal the one the claim reconstructs, which makes losing information a build failure rather than a silent drift. The start-event rule was implemented twice, once in the store and once in the runtime, and a fix to either left the other admitting what its twin rejected. There is one implementation now, in core, called from both. Lineage gains `resumedFromRunId` and `retriedFromRunId`. Without them the opening cannot say that a run resumes or retries another, which is the only thing that distinguishes a linked child's two admission kinds. The claim's opening is decoded strictly, with no legacy widening. A claim whose frozen opening cannot be read cannot authenticate the start it exists to authenticate, so the migration drops such a row rather than leaving one that would fail every later read and hold its boundary forever. Refs #4311 Generated-by: Claude Code
… union Matching a Run against the root the Host admitted took about 140 lines, almost all of it asserting that every other optional lineage and root-authority field was absent. That shape was forced by the header: an open bag of optionals where "this is a Goal root" could only be said as "goalId is set and the other four markers are not", and where adding one lineage field meant editing six negative lists or silently weakening every one of them. The opening fact names its root as a closed discriminated union, so each arm names the root it wants. What is left of lineage is one exactness check: an admitted root has the lineage its kind implies and no other edge, which is both stronger than the old per-field negatives and immune to a new field being forgotten. The matcher now takes the invocation rather than the Run header. Runtime Host still holds headers, so its one call site projects through the same mapping the rest of this work uses; phase 2c hands it the real opening. Refs #4311 Generated-by: Claude Code
`provider_request_captured`, `provider_request_attempt_recorded` and `task_gate_decided` have no writer in this build. `AGENT_RUN_EVENT_TYPES` is the catalogue of what this build may append, not what it may read, so keeping them there only kept alive the copy rewriters that existed to move their payloads. Persisted rows of these kinds keep working exactly as before, because the ledger's `type` has always been an open string: the diagnostic reader that folds a legacy provider attempt into a prompt composition still reads them, and a conversation copy now drops them the way it already drops every type this build cannot emit, rather than carrying source-owned identities into the target it cannot check. One thing does survive the deletion: the copy still harvests provider trace ids from those rows. A copied RuntimeEvent may point at a trace only a retired writer recorded, and pointing the target at a fresh id is right where pointing it at the source's id would not be. Refs #4311 Generated-by: Claude Code
Generated-by: Claude Code
A start the Interaction authority refuses because it is draining used to end as a failure or a cancellation depending on which writer won: if the Turn's stop fence had already stopped the run, the run recorded a cancellation; otherwise the same shutdown read as a Host fault and asked the Host to drain again. Shutdown is not a run failure, and a race is not a classification. State it once, where the errors that carry the reason are defined, and let both the kernel and the Host coordinator ask the same question: a draining authority cancels the run it refuses. Generated-by: Claude Code
Recovery walks every invocation on a Session's spine and, for each, commits the terminal fact and the terminal Turn state. It then reported every walk as a recovery, so a Session whose runs had all ended cleanly still had its status rewritten on every startup, bumping the header revision and invalidating the revision a caller was holding across the restart. The claim used to be at least literally true: that commit also wrote the Run header. With the header gone there is no second record left to write, so an already-terminal run makes the whole pass a no-op. Report a recovery only when this pass supplied something the run was missing: a terminal fact it did not have, or a terminal Turn state it did not carry. Generated-by: Claude Code
The opening fact is now event 1 of every invocation, so a source Run's RuntimeEvent high water sits one past where these fixtures expected it, and a branch's copied invocation opens on its own spine and projects the copied Turn as ended. Two Sessions that reuse a run id now open separate invocations, so the inspect fixtures name the second one explicitly, and the shared evidence budget accounts for the bytes the opening event itself occupies. The Agent Graph provider fixture asserted the child run's status off `agent_output`'s `header`, which is now `invocation`; assert it off the invocation's terminal event instead. Generated-by: Claude Code
…tcome `TerminalAgentRunStatus` was left as a bare alias of `RuntimeInvocationOutcome`, so the vocabulary this change retired survived as a second name for the same three values. Use the one name. The invocation fixture had been copied byte-for-byte into `runtime-host`, and the `storage` fixture hand-rolled the opening event instead of building it. Share the fixture through the `test-only` entry point this repo already uses for cross-workspace test modules, and build the storage fixture's event with `buildInvocationOpenedEvent` so no test can drift from how the runtime opens an invocation. Generated-by: Claude Code
…ings A Run ends exactly once. When the inventory found two terminal events on one invocation it kept whichever came last, so a ledger that contradicts itself read back as a settled run and the contradiction never reached anyone. Leave such an invocation without a terminal event instead, and let the readers that can act on it — the inspect model and the read model — classify off the events themselves, so the ambiguity surfaces as ambiguity rather than as a run that merely has not finished. Generated-by: Claude Code
…ional ledger The opening fact is a RuntimeEvent now, so the store that decides whether it can be written is the RuntimeEventStore. Gating it on the AgentRunStore left a run with a spine and no operational ledger invisible to the inventory, and gated the inventory on a store that no longer holds any part of it. Open it whenever finalize runs too. A run that ends before it ever started would otherwise leave a terminal event on an invocation nothing had opened, which is an ending the inventory cannot see. Generated-by: Claude Code
A terminal RuntimeEvent is immutable and must be the ledger tail, so a failure class or abort source it did not state can never be added afterwards. The header used to hold it, and recovery wrote 'app_restarted' there; with the header gone the read model refused the fact instead, and one such event made the whole Session unreadable. Read the event as what it is: the run ended, and the detail it omitted is `unknown`. The terminal-fact classifier keeps the diagnostic, the projection already rendered `unknown`, and recovery no longer has an incomplete-terminal case to repair — so `incomplete_single_terminal` and the projection's duplicate diagnostics go with it. Generated-by: Claude Code
Both tests reached for state the spine no longer keeps: one repaired a steering message on an invocation nothing had opened, the other awaited a settlement barrier that lives in acceptMappedEvent rather than in recordSessionEvent. Seed the opening and drive the real barrier. Generated-by: Claude Code
These tests wrote operational rows for a run nothing had opened, and read back inventories keyed by an invocation id two runs shared. Both worked only because the run header stood in for the opening; with the header gone the seeds have to state what they always meant. The checkpoint-unavailability test waited on a trace-failure row that the retired `run_created` write used to produce. It now says directly what it was arranging: the store goes unavailable just before the checkpoint write. Generated-by: Claude Code
The last places still asking the header what a run was are gone. `inspectAgentRunReadModel` looked its invocation up by the id it was given as an invocation id, which is right only while a run and its invocation share one identity — a continuation is a new run on the invocation it resumes. It now finds the invocation by run, and the store's own `readInvocation` fast path goes with the mistake. The conversation-copy guard against "a retained AgentRun without RuntimeEvent facts" describes an invocation with no events. An invocation is its opening event, so that state no longer exists; the guard and its test are removed. `after_run_created` named the header row that a continuation wrote before its durable start. A continuation's opening rides its continuation-start event, so nothing is durable there any more and the failpoint names no boundary. A crash after the terminal event is likewise not an unfinished claim: the event is the continuation's ending, so the boundary already has one. An imported transcript that never stated how a turn ended used to be repaired to failed once the header noticed the missing terminal. The terminal event is now written when the turn is materialized and can never be corrected, so an inferred status is recorded as the failure it is, which keeps an adapter's reason to emit its own cutoff true. Generated-by: Claude Code
Generated-by: Claude Code
`classifyAgentRunRecovery` still scanned the operational ledger for `run_completed`, `run_failed` and `run_cancelled`. Those types no longer exist, and its own contract already says the caller established there is no terminal event, so the scan could only ever answer the same way twice. Generated-by: Claude Code
Rebasing onto main brought in `readSessionRuntimeEventEntries`, the durable session order a read now sorts by. The doubles that stand in for a store have to answer it, and the read model no longer takes a run store at all. The upstream tests for the read model's projection-cache backfill go: that path only ran for a terminal run whose ledger was empty, and an invocation is its opening event, so a run with no events is not a run this model can see. Generated-by: Claude Code
Opening the invocation no longer awaits a header write, so onRunStarted now fires while the admission that started the Turn is still open, and its refreshCanonical is rejected as a nested admission. The Turn is not admission work: detach it from the admission context so its own admissions queue normally. Generated-by: Claude Code
Every test that needed an invocation record built its own copy of the opening fact: eight near-identical constructors plus a dozen inline literals, all restating defaults nothing asserts on. Fold them onto the one fixture, which now merges configuration field by field so a test states only the setting it is about, and writes failureClass where the read model looks for it. Drop the cases that only restate a schema — protocol literal, enum member, empty lineage, each empty legacy field — and keep the ones that carry a rule: which routes may name a connection, which roots exist, that a continuation names its boundary, that a malformed opening fails the whole decode. Generated-by: Claude Code
The stop test from #4439 read the run's status and abort source off the header. Both are the terminal event's to state, and the test already asserts on it. Generated-by: Claude Code
…uded The legacy decoder rejected any header carrying `runComposition`, which every run that reached a provider on main carries, because `hasExactShape` refuses unknown keys before a single field is checked. The backfill then swallowed the decode error and skipped the row, and the core-execution migration dropped `record_json` right after: the run had no opening, no shelf row and no header any more, so `listSessionInvocations` could not enumerate it while its events stayed behind as orphans. Three changes at the migration. The legacy shape accepts `runComposition` as an opaque record: nothing on the spine reads it back, so the migration only has to know a header carrying it is well formed. A header the migration cannot read now stops the migration — the transaction rolls back and the error names the row — because the alternative was a run silently ceasing to exist. And a header-only run whose header recorded an ending gets that terminal event as event 2, carrying the header's failure class, message and abort source; a completed legacy run migrated as an open one before, since the opening was the only fact projected. The synthetic terminal builder moves from runtime to core beside the opening builder so the migration and recovery state the same envelope, and `runtimeEventKind` moves to the schema module for the same reason. A partial unique index makes an opening unique per invocation by schema rather than by convention, and the anchor reader no longer guesses `sessionInline: false` for a run with no opening at all. The regression case is built from a base-era header with a real composition snapshot, not from the decoder's own accepted set: a fixture written to the new shape can only ever prove that new code reads what new code writes. Generated-by: Claude Code
The migration shelves the opening of any run that already owned events, since that run's sequence is immutable. A conversation copy cloned only the events, so the target Session had runs with no opening at all: `listSessionInvocations` returned nothing, and the branched Session's first send saw no history. The base cloned the header row explicitly, so this was a regression of the spine. The copy is a fresh sequence, so the run's opening can be event 1 there. `loadConversationCopyRunEvents` synthesizes it from the shelved record when the source events lack one, and the ledger copy inserts it ahead of the run's first source event. The regression test stages the shelved state the way the migration leaves it. Generated-by: Claude Code
The rebuild counted terminal events per invocation and, on two, declared the run ambiguous; two readers then branched on that ambiguity. But the store seals a run on its terminal event — `assertRunNotSealed` runs on every insert, in-process and across processes — so a second terminal cannot be written, and the only way to reach the branch was a test double arranging a state the index refuses. The rule also contradicted the index by construction. The rebuild now states the same rule as the index: an invocation's terminal event is its one terminal event, wherever a Session-ordered read places it relative to other invocations. The two reader branches and the test that arranged the unrepresentable state go with it. `acceptedInputBoundary` gains one sentence: an opening that could not prove its route never matches, even when the current run has none either. That is the one place the spine is stricter than the header comparison was, and it should be said where it is decided. Generated-by: Claude Code
`readRunIfPresent`, `readInvocationIfPresent` and `SessionManager.readInvocation` had become `listSessionInvocations()` followed by `find`, and a Turn calls them four to six times. The header era answered these with a keyed row read. `RuntimeEventStore` gains an optional `readRunInvocation(sessionId, runId)`; the SQLite store answers it off the opening index, and a helper in core answers it from the inventory for stores that do not implement it, so callers state one intent either way. Generated-by: Claude Code
The behaviour change was listed nowhere and had no test: work detached from an admission must take admissions of its own, queued behind the active one like any other caller. Generated-by: Claude Code
A run that already owned an immutable sequence could not be given an opening event at position 1, so the migration shelves its opening in runtime_legacy_invocation_openings. Readers surface a shelved opening only when its invocation has no opening event, which made deleting a Session's events turn the shelf back on: the inventory reported the run again, now with no terminal event, and a completed run came back as an active one. Startup recovery then wrote a fresh app_restarted terminal into a Session the user was deleting. Purge is not the owner of this rule. The shelved opening is a fact about a ledger that already exists, so it is anchored to that ledger's first event and cascades with it. Every path that removes a Session's events is now correct without having to remember the table, which is how runtime_session_event_ordinals already works. Migration 16 has not shipped, so the column goes into that migration rather than a new one. Generated-by: Claude Code
…does Four places state when an invocation ended. Recovery, the read model and continuation resume all take the run's first terminal event. The SQLite inventory took the last event and only when that event was terminal, which answers differently for a ledger that carries a straggler after the terminal. Those ledgers exist. Runs were only sealed from #4242 onward, and before that pressing stop ended a run while its stream was still draining, so the stragglers that window now refuses were written and nothing has ever removed them. On such a run the inventory said active while recovery said ended, so finalizeChildWorkspacePatches threw over a nonterminal run that had finished and continuation discovery could not see it at all. The inventory now states the same sentence as the other three. Sealing already makes the first terminal the only one for anything this codebase writes, so nothing changes for a ledger written since #4242. Two terminals is corruption, and refusing it stays with the readers that exist to refuse it; a Session list reports the ending it can see rather than failing the whole Session over one run. Sealing was an unstated obligation of one store. It is now on RuntimeEventStore, where every implementation and test double can be held to it, alongside the fact that a migrated Session's inventory genuinely cannot be rebuilt from its events. Generated-by: Claude Code
…ng once Three duplications the previous two commits left behind. The backfill asked twice for the same row — one subquery for the first event's invocation id, another for its event id — so the two could be reasoned about as if they might disagree, and a branch existed for a case the query made impossible. One join answers both. The terminal-event predicate was written out at each query that needed it. It is the SQL half of isTerminalRuntimeEvent, which stays the authority, so it is stated once and used twice. The cascade assertion in the backfill test restated what the purge regression already covers end to end. The UNIQUE on anchor_event_id stays and is now explained: SQLite indexes only the parent side of a foreign key, so without it every deleted RuntimeEvent scans the shelf. Measured at 20k events over a 4k-row shelf, removing it takes the delete from 8ms to 250ms. Generated-by: Claude Code
A continuation claim's start event belongs to the target Session, and the foreign key naming it had no ON DELETE clause. Purging that Session deletes its RuntimeEvents, so the constraint refused the delete and rolled the whole purge back — for the user's delete, a conversation-copy rollback, an import discard and Session retirement alike. A continuation whose target has been deleted no longer names anything, so the claim goes with it and the source boundary it was holding is free again. SET NULL would have been worse than the bug: the claim would survive looking like one that had been claimed but never started, and resume would try to start it against a target that no longer exists. Migration 16 was already renaming a column on this table, and SQLite cannot alter a foreign key in place, so the rename becomes a rebuild and does both at once. Migration 15 rebuilt the same table the same way. Generated-by: Claude Code
…tion Two terminal events was classified as `ambiguous`. Nothing is ambiguous about it: a store seals a run on its first terminal, so a second one means the ledger was written around the seal. It is `corrupt`, and the strict recovery error now says what it found rather than that it was unsure. The seal itself was an obligation only one double honoured, which is what let that state look reachable in the first place. It is stated once now and used by every RuntimeEventStore double, so a double cannot drift from the SQLite store or from the interface. Six fixtures were appending to a finished run through the API that forbids it, using append as a way to write into the ledger rather than to test the seal. They now seed the double directly, which is where a ledger shape no supported store can produce belongs. Generated-by: Claude Code
15d9d02 to
8cd09af
Compare
|
Correcting my own scope call from the previous comment, at I said the continuation-claim foreign key would get its own issue. That was wrong, and it is fixed here instead. The reason I gave — that this PR does not touch that path — does not survive checking: migration 16 was already renaming a column on The fix is smaller than the retirement rule I imagined needing. Two smaller things from the same pass, both of which you had flagged in substance.
The seal is now stated once for the test doubles and used by all of them. Only one double honoured it, which is exactly what let the state look reachable. Six fixtures had been appending to a finished run through the API whose contract forbids it, using An ablation pass, since a change that removes an authority should not be quietly adding them back. Three duplications came out: the backfill was asking twice for the same row, which also removed a branch the query made unreachable; the terminal-event predicate had been written out at each query that needed it, and is now stated once as the SQL half of Suites after a rebase onto 简体中文更正我上一条评论里的范围判断,对应 我说 continuation claim 的外键会另开 issue。那是错的,现在直接在本 PR 修掉了。 我给的理由——本 PR 不碰那条路径——经不起核对:migration 16 本来就在重命名 修法比我设想的退休规则小得多。 同一轮里的另外两件小事,你在实质上都提过。
封口现在为测试替身陈述一次,所有替身共用。此前只有一个替身遵守,这正是让那个状态看起来可达的原因。有六处 fixture 一直通过契约禁止的 API 往已结束的 run 上追加事件,把 做了一轮消融,因为一个以移除权威为目的的改动,不该悄悄把权威加回去。 砍掉三处重复:backfill 在重复问同一行,顺带消掉一个被查询本身变成不可达的分支;终态谓词此前在每个需要它的查询里各写一遍,现在作为 rebase 到
|
Two inspect readers already had the answer available as a point read and scanned the whole Session inventory instead. `readRunInvocation` exists so a caller holding a run id never pays for the list, and a hand-written scan beside it is a second implementation of the same lookup that nothing keeps in agreement with the first. Also drops an `export` on a status tuple no other module names. A symbol only its own file uses is not a contract other packages should be able to import. Generated-by: Claude Code
Summary
A Run's facts lived in two places at once. The RuntimeEvent ledger recorded what happened; a mutable
AgentRunHeaderrow recorded what the run was and how it ended. Every reader had to decide which one to trust, every writer that ended a run committed twice, and a whole layer existed only to keep the two in agreement —computeStatusConsistency,effectiveRunHeaderFromTerminalFact,terminalRunHeaderMatchesFact, thestatus_consistency_mismatchdiagnostic, and anafter_terminal_header_committedfailpoint for the window between the two commits.This retires the header. A Session is its RuntimeInvocations, and an invocation is an immutable opening fact, its events, and one terminal event:
invocation_opened_v1) carrying the route, configuration, root authority, open source and lineage the header used to hold. It is event 1 of every invocation.RuntimeInvocationRecordis a query over the spine, not a table.runtimeInvocationsFromSessionEventsis the definition of a Session's inventory; the SQLite index exists to address and page it, and a rebuild from events alone must agree with it.AgentRunStorekeeps only what it is the authority for — the operational event ledger.Fixes #4311
The write path and the read path, before and after
Red marks what this PR deletes and the failure it existed to paper over; the right-hand column has none of it.
同一张图的中文版:
Where the diff goes
+9753 / −8671is a net +1082, which is not what a change that removes a record should look like. The account, against the merge base:legacy-run-header.ts(+454), its decode tests, the backfill test, and the backfill insqlite-runtime-schema.tsWithout the migration this removes about 580 lines. The migration exists only to read databases the old code wrote, and comes out whole once those are gone.
Why the rest is only −580, when a whole authority went away:
The facts moved; they did not disappear. Route, configuration, root authority, lineage and open source still have to be declared somewhere — now in the opening fact rather than in a header row.
core/agent-run.ts(−557) pays forruntime-event.ts(+327, the closed content schema) andruntime-invocation.ts(+402, which also owns the synthetic terminal builder the migration and recovery share). Part of that is a verbatim move: hosted-root matching was 139 lines and is 159 now under a new name, which git records as one addition and one deletion.What actually went away was never a module. The reconciliation layer was a responsibility spread across six files —
runtime-ledger-repair.ts−430,terminal-run-commit.ts−192,runtime/agent-run.ts−151,storage/agent-run-store.ts−114,runtime-read-model.ts−104,prior-run-context.ts−103. Roughly a thousand lines of writing both records, deciding which to trust, and repairing one from the other. No file was deleted outright; each kept the job it was actually for.A derived inventory pays for what a table gave away. The header was a row, so addressing, ordering and paging came free. Deriving the inventory from events costs
sqlite-runtime-store.ts(+197) and its schema. The derivation itself is 36 lines —runtimeInvocationsFromSessionEvents; making it addressable, pageable and readable by run id is the rest.The count that matters is not lines:
AgentRunHeaderhas no references left in the repository, the write path commits once instead of twice, and no reader has to decide which of two records is true.Review focus
Not a diff to read front to back. Suggested order — each row is self-contained, and the behaviour rows are where judgement is actually needed:
Behaviour changes (6) — what the header used to hide
A start refused because the Interaction authority is draining now settles as
cancelled, notfailed. It used to be one or the other depending on which writer won the race with the Turn's stop fence. Shutdown is not a run failure; the classification is stated once, next to the errors that carry the reason.Recovery now reports only what it repaired. It used to report every invocation it walked. That was harmlessly true while the walk also rewrote the header; with the header gone an already-terminal run makes the pass a no-op, and the false claim rewrote the Session status on every startup, bumping the header revision out from under a caller holding it.
A terminal event that omits its failure class or abort source reads as
unknowninstead of making the Session unreadable. The event is immutable and must be the ledger tail, so a detail it did not state can never be added later. Recovery used to writeapp_restartedinto the header instead; with the header gone the read model refused the fact outright, and one such event took a whole Session down. This also removes recovery'sincomplete_single_terminalcase and the projection's duplicate diagnostics.A drained Turn runs outside the admission that started it. A Turn the Host drains at startup used to inherit the admission context of the recovery pass that started it, so whether its own first admission was rejected as re-entry came down to which finished first.
SessionAdmissionGate.detachleaves the context on purpose; the Turn takes admissions of its own, queued behind the pass like any other caller.Continuation crash boundaries.
after_run_creatednamed a header row written before the durable start — a continuation's opening rides its continuation-start event, so nothing is durable there any more and the failpoint, its startup repair and their tests are gone. A crash after the terminal event is likewise not an unfinished claim; the boundary now reportscontinuation_already_exists.Imported transcripts. A turn whose source never stated how it ended was materialized as completed and then repaired to failed once the header noticed the missing terminal. The terminal event is now written at materialization and can never be corrected, so an inferred status is recorded as the failure it is — which is also why an adapter emits a cutoff of its own.
Migration
A persisted legacy run header is decoded into an opening. A run that never wrote a RuntimeEvent gets the real thing — the opening as event 1 of its own invocation, and, if the header recorded an ending, the terminal event as event 2, carrying the header's failure class, message and abort source. A run that already has events cannot, because it owns an immutable sequence whose position 1, digests and coverage other facts already point at; its opening is shelved in
runtime_legacy_invocation_openings, which only this migration ever writes. Readers merge the two shelves, so nothing downstream knows which one an opening came off. The one place that re-materializes a run elsewhere — a conversation copy — gives such a run its opening back as event 1 of the copy, since the copy is a fresh sequence.The decoder accepts exactly what the header era wrote,
runCompositionincluded; its fixtures are built from that write shape, not from the decoder's own accepted set. The composition snapshot itself is not carried onto the spine: nothing onmainreads it back, and the live writer'srun_composition_recordedfact starts with the first run after the upgrade. A header the migration cannot read stops the migration — the transaction rolls back and the error names the row — because the next step drops the column, and a swallowed decode error would have been a run silently ceasing to exist.The migration also adds a partial unique index so that an invocation can carry at most one opening; the terminal event was already unique, because the store seals a run on it.
The migration also rebuilds
runtime_continuation_claimsrather than renaming one column on it. That claim's start event belongs to the target Session, and the foreign key naming it had noON DELETEclause, so purging that Session was refused by the constraint and the whole purge rolled back — user delete, copy rollback, import discard and Session retirement alike. A continuation whose target has been deleted no longer names anything, so the claim now goes with it and the source boundary it held is free again. SQLite cannot alter a foreign key in place, and migration 15 rebuilt this same table the same way.A shelved opening is a fact about a ledger that already exists, so it is anchored to that ledger's first event with a cascading foreign key and dies with it. Without that, deleting a Session's events would leave the opening behind, the shelf would switch back on, and a completed run would reappear as an active one with no ending — which startup recovery would then write an
app_restartedterminal into. Anchoring puts the shelf under the rule the rest of the schema already follows, so every path that removes a Session's events is correct without knowing the table exists.Tests deleted rather than translated
Some tests were removed instead of migrated. Each asserted a header/ledger disagreement, a crash in the window between the two commits, or a read-time repair of one record from the other — states this change makes unrepresentable, so there is nothing left for the test to arrange. The commit that removes each one names it and says why. Everything else was translated in place.
Two of those came in on the rebase, from #4445: the read model's projection-cache backfill only ran for a terminal run whose ledger was empty, and an invocation is its opening event, so a run with no events is not one this model can see. Everything else #4445 added — the durable session order a read sorts by, and its failure path — is kept and asserted on the spine.
A second pass removed cases that only restate a schema the decoder already enforces — a protocol literal, an enum member, an empty lineage object, each empty field of a legacy continuation source. What is left states a rule: which routes may name a connection and which may not, which root authorities exist and that none of them mix, that a continuation names the boundary it resumes from, and that a malformed opening fails the whole RuntimeEvent decode.
The same pass folded every test's idea of an invocation onto one fixture. Eight near-identical record constructors and a dozen inline opening literals became calls to it; its
configurationnow merges field by field, so a test states only the setting it is about.Verification
@maka/core@maka/storage@maka/runtime@maka/runtime-host@maka/desktopnpm run -ws typecheck,npm run format,npm run lintNot run: the full-repo suite. Of the Playwright E2E suite, only
workhub-reconstruction.spec.tswas run locally (3 passed). No user-visible surface changed, so no before/after screenshots.AI use
Select exactly one:
Tool(s) and scope: Claude Code, used throughout — the refactor across all workspaces, the test migration, the behaviour changes above, and this description. Every commit carries a
Generated-bytrailer. All reasoning about what to delete and what to keep was reviewed by me against the issue.Checklist
Does this PR entail a change in behavior?