You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "Astro-Han (via GitHub)" <gi...@apache.org> on 2026/09/03 07:30:07 UTC
[PR] refactor(runtime): converge AgentRun metadata into the RuntimeInvocation event spine [maka]
Astro-Han opened a new pull request, #4631:
URL: https://github.com/apache/maka/pull/4631
## Summary
A Run's facts lived in two places at once. The RuntimeEvent ledger recorded what happened, and a mutable `AgentRunHeader` row recorded what the run was and how it ended. Every reader that needed either had to decide which one to trust, every writer that ended a run had to commit twice, and the code between them existed only to keep the two in agreement: `statusConsistency`, `effectiveRunHeaderFromTerminalFact`, `terminalRunHeaderMatchesFact`, a `status_consistency_mismatch` diagnostic, and a `after_terminal_header_committed` crash failpoint for the window between the two commits.
This retires the header. A Session is now its RuntimeInvocations, and an invocation is an immutable opening fact plus its events plus exactly one terminal event:
- The opening fact is a hidden system RuntimeEvent (`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.
- `RuntimeInvocationRecord` is a query over the spine, not a table. `runtimeInvocationsFromSessionEvents` **is** 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.
- A Run ends exactly once, and its terminal RuntimeEvent is the only statement that it ended.
`AgentRunStore` keeps only what it is actually the authority for — the operational event ledger. `createRun`, `updateRun`, `readRun` and `listSessionRuns` are gone.
Five behaviours the single spine forced into the open, all settled here rather than left for a follow-up:
- A start the Interaction authority refuses because it is draining used to settle as `failed` or `cancelled` depending on which writer won the race with the Turn's stop fence. Shutdown is not a run failure; the classification is now stated once, next to the errors that carry the reason.
- Recovery reported every invocation it walked as a recovery. That was harmlessly true while the walk also rewrote the Run 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. Recovery now reports only what it supplied.
- 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 later. Recovery used to write `app_restarted` into the header instead; with the header gone the read model refused the fact, and one such event made a whole Session unreadable. Such an event now reads as what it is — the run ended, and the detail it omitted is `unknown` — which also removes recovery's `incomplete_single_terminal` case and the projection's duplicate diagnostics.
- A crash between a continuation's claim and its durable start used to leave a Run header row behind, which the `after_run_created` failpoint named and startup repaired. A continuation's opening rides its continuation-start event, so nothing is durable at that point any more: the failpoint, its repair and their tests are gone. A crash *after* the terminal event is likewise no longer an unfinished claim — the event is the continuation's ending, so the boundary reports `continuation_already_exists`.
- An imported transcript that never stated how a turn ended was materialized as completed and then 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 afterwards, so an inferred status is recorded as the failure it is — which is also why an adapter emits a cutoff of its own.
Existing sessions migrate: a persisted legacy run header is decoded into an opening and stored beside the ones events carry, so an invocation opened before this change reads the same as one opened after.
Fixes #4311
## Verification
- `@maka/core` `test:dist` — 778 pass, 0 fail
- `@maka/storage` `test:dist` — 1097 pass, 8 skipped, 0 fail
- `@maka/runtime` `test:dist` — 3162 tests, 3149 pass, 0 fail
- `@maka/runtime-host` `test:dist` — 1591 tests, 1579 pass, 0 fail
- `@maka/desktop` `test:dist` — 1965 pass, 0 fail
- `npm run -ws typecheck`, `npm run format`, `npm run lint` — clean
Not run: the full-repo suite, and the Playwright E2E suite. No user-visible surface changed, so no before/after screenshots.
## Review focus
A number of tests were deleted rather than translated. Each asserted a header/ledger disagreement, a crash in the window between the two commits, or a read-time repair of one from the other — failure modes this change makes unrepresentable, so there is nothing left for the test to arrange. The commit that removes each one says which and why. Everything else was translated in place, and the suites below cover the same ground they did before.
## AI use
Select exactly one:
- [ ] No generative tool made a substantive contribution
- [x] Generative tooling made a substantive contribution
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-by` trailer. All reasoning about what to delete and what to keep was reviewed by me against the issue.
## Checklist
- [x] Tests cover the change and fail without it
- [x] Lint, format, typecheck and the affected suites pass locally
Does this PR entail a change in behavior?
- [x] Yes — described under Summary above
- [ ] No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(runtime): converge AgentRun metadata into the RuntimeInvocation event spine [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #4631:
URL: https://github.com/apache/maka/pull/4631#issuecomment-5534745469
Both findings hold. Fixed at `394b164af9`, though not where either of us proposed — the purge fix in particular. I also ran two adversarial passes of my own against this head, and one of them overturned a conclusion I had already committed to.
## P1 — the shelf outliving a purge: fixed, but not in `purge()`
Confirmed end to end, including the consequence you measured: strict recovery returns the purged session id and writes an `app_restarted` terminal plus a `failed` turn state back into it.
**Your fix would have worked and I did write it first, then threw it away.** Deleting the shelf row inside the purge transaction adds a second statement of "what a Session's operational state consists of", and that duplication is exactly what produced this bug — this PR added a table and the hand-written list in `conversation-operational-state.ts` did not follow. The next table would have the same fate.
Two facts decided it. First, the shelf is the only table this purge actually missed: `runtime_session_event_ordinals` cascades from `runtime_events` and the purge lease runs `PRAGMA foreign_keys = ON`; `runtime_workspace_epochs` is CHECK-constrained to the workspace authority session, which this purge refuses outright; `runtime_continuation_claims` is cross-Session lineage that `main` deliberately keeps. Second, deriving the list is not available either — `session-bundle-policy.ts` already discovers session-scoped tables by column name and gets the new table right for free, but its predicate keeps a row only when *both* ends belong to the exported Session, so inverting it for a live purge would delete continuation claims whose other end is still live.
So the shelf gets an `anchor_event_id` with `REFERENCES runtime_events(event_id) ON DELETE CASCADE`, pointing at the first event of the ledger it describes. A shelved opening is a fact *about* an existing ledger; when the ledger is gone the subject of that fact is gone. `backfillInvocationOpeningFacts` already selected that run's first event to key the row, so it now returns the event id too. `purge()` is unchanged, and every future path that deletes a Session's events is correct without knowing this table exists. Migration 16 has not shipped, so the column goes into it rather than a new migration — no quarantine question arises.
Your four checks: a shelved invocation always has at least one event (only the `existing_invocation_id !== null` branch shelves); the sole production deletion of `runtime_events` is the whole-Session purge; `WITHOUT ROWID` takes cascading foreign keys, which `runtime_session_event_ordinals` already relies on; and neither `foreign_keys = OFF` site leaves a dangling row — `operational-target-schema.ts` builds an in-memory schema with no data, and `session-bundle-policy.ts` filters the shelf by its own `session_id` predicate and ends with `foreign_key_check`.
**On naming it a durable authority: accepted, and the doc claim was false as written.** `RuntimeEventStore.listSessionInvocations` said clearing the index and rebuilding from events yields the same inventory. For a migrated Session it does not and never will. That is now stated on the interface as the durable exception it is. The retention question answers itself once the row is anchored: it lives exactly as long as the ledger it describes.
## The contract question — the answer changed my own patch, not just yours
**Your option 1 is buildable; the reason to reject it is not the one you gave.** `isTerminalRuntimeEvent` reads only `status` and `actions.endInvocation`, both fixed at write time, and `assertRunNotSealed` already expresses that predicate in SQL. The same predicate goes straight into a partial unique index — I built one:
```
index created: OK
second terminal refused: UNIQUE constraint failed: e.run_id
post-terminal text event: ACCEPTED — index does not seal
```
The third line is why it loses. The index refuses a second terminal and nothing else, while `assertRunNotSealed` refuses *any* event after the terminal, checks partials too, and produces a typed `RunSealedError` inside `BEGIN IMMEDIATE`. So the index deletes no existing statement, adds one, and adds an error-translation path to keep the cross-process semantics you measured. Net positive complexity for a strictly weaker guarantee. Option 3 falls with it, and it is worth naming that quarantine exists only to clean up a failure mode option 1 introduces. Option 2 is this PR: there is no second record left to commit alongside the terminal event.
**Option 4 is the one that was right, and following it properly reversed the change I had already made.** I had converged the readers by moving the core rebuild to "the last event, if terminal" — matching the SQLite index. That was backwards. Four readers state this rule, not three: recovery, `classifyRuntimeEventTerminalFact`, and the tail check in `runtime-resume.ts:576` all take the run's **first** terminal. The SQLite inventory was the only one reading the tail.
That difference is reachable, and not through corruption. Runs have only been sealed since #4242; before it, pressing stop ended a run while its stream drained, and those post-terminal stragglers were committed and have never been cleaned up. On such a ledger the inventory said active while recovery said ended — so `finalizeChildWorkspacePatches` threw "cannot finalize while Run is nonterminal" over a run that had completed, and continuation discovery could not see it at all. The header used to absorb this, because "did it end" was read off the header and a trailing event was harmless.
So the inventory now states the sentence the other three already state. Core and the read model are unchanged. For any ledger written since #4242 nothing changes at all, because sealing makes the first terminal the only one. Two terminals remains corruption and the readers that refuse it keep refusing it; a Session listing reports the ending it can see rather than failing a whole Session over one run.
And sealing is now an obligation on `RuntimeEventStore` rather than a property of one store — stated on `appendRuntimeEvent`, with `ensureTerminalRuntimeEventDurable` required to commit the terminal and its seal check in one transaction. Your point that it had nothing underneath it was fair; the answer is to put it on the interface every implementation and double must honour, not to add a weaker constraint beneath it.
## Deferred, with evidence
`runtime_continuation_claims.start_event_id` is `REFERENCES runtime_events(event_id)` with no `ON DELETE` clause, so it defaults to NO ACTION. Purging a Session that ever started a continuation is refused outright:
```
purge REFUSED: FOREIGN KEY constraint failed
```
This predates the PR — the table and that column are from migration 15 and earlier — and nothing here makes it worse. It needs its own retirement rule for cross-Session claims rather than being folded into this change. I will open an issue.
## Verification
core 802 / storage 1108 / runtime 3209 / runtime-host 1674 / desktop 2078, zero failures; format and lint clean. Rebased onto `main` at `b39e8d36ef`, no conflicts. New regression cases: the shelved opening is anchored to its run's first event and vanishes when that event is deleted; a migrated Session whose events are purged leaves an empty shelf and an empty inventory through both readers; and an unsealed-era straggler after the terminal still reads as ended, with the index and a rebuild agreeing.
<details><summary>简体中文</summary>
两条发现都成立，已在 `394b164af9` 修复，但修的位置和你我先前提的都不同——purge 那条尤其如此。我另外对这个 head 做了两轮自己的对抗性评审，其中一轮推翻了我已经提交过的一个结论。
**P1（shelf 在 purge 后残留）——修了，但不是在 `purge()` 里。** 端到端确认，包括你实测的后果：strict recovery 返回被 purge 的 session id，并往里写 `app_restarted` 终态和 `failed` 的 turn state。
**你的修法可行，我也确实先写了，然后扔掉了。** 在 purge 事务里删 shelf 行，等于给「一个会话的运行态由哪些表构成」加了第二处陈述，而这正是本 bug 的成因——本 PR 加了一张表，`conversation-operational-state.ts` 里手抄的清单没跟上。下一张表还会重蹈。
两个事实定了取舍。其一，shelf 是这个 purge 真正漏掉的唯一一张表：`runtime_session_event_ordinals` 由 `runtime_events` 级联，purge 的 lease 开着 `PRAGMA foreign_keys = ON`；`runtime_workspace_epochs` 被 CHECK 约束死为工作区权威会话，而 purge 明确拒绝该会话；`runtime_continuation_claims` 是跨会话谱系，`main` 有意保留。其二，派生清单这条路也不通——`session-bundle-policy.ts` 已经在按列名发现会话作用域的表，新表不用改就对了，但它的谓词是「两端都属于所导出会话才保留」，为 live purge 反用会删掉另一端仍存活的 continuation claim。
于是给 shelf 加 `anchor_event_id`，`REFERENCES runtime_events(event_id) ON DELETE CASCADE`，指向它所描述的那个账本的第一条事件。被搁置的 opening 是**关于**一个已存在账本的事实；账本没了，这个事实的对象就没了。`backfillInvocationOpeningFacts` 本来就在查该 run 的第一条事件来给行定键，现在顺带返回 event id。`purge()` 一行未动，未来任何删除会话事件的路径都自动正确。migration 16 尚未发布，所以列直接进它，不需要新 migration，隔离问题也就不存在。
你的四个核查点：被 shelve 的 invocation 必然至少有一条事件（只有 `existing_invocation_id !== null` 分支才上货架）；生产上删 `runtime_events` 只有整会话 purge 一处；`WITHOUT ROWID` 支持级联外键，`runtime_session_event_ordinals` 今天就靠它；两处 `foreign_keys = OFF` 都不留悬空行——`operational-target-schema.ts` 只在内存里建 schema 无数据，`session-bundle-policy.ts` 用自己的 `session_id` 谓词过滤 shelf 并以 `foreign_key_check` 收尾。
**关于「把它命名为持久权威」：接受，而且那句文档原本就是假的。** `RuntimeEventStore.listSessionInvocations` 说清空索引、仅凭事件重建可得同一份清单。对迁移过的 Session 这是假的，而且会一直是假的。现在它作为持久例外写在接口上。保留期问题在加了锚之后自动消解：它与所描述的账本同寿命。
**契约问题——答案改的是我自己的补丁，不只是你的。**
**你的方案 1 建得出来，但该否掉它的理由不是你给的那个。** `isTerminalRuntimeEvent` 只读 `status` 和 `actions.endInvocation`，两者写入时即确定，`assertRunNotSealed` 已经用 SQL 表达了这个谓词。同一个谓词直接进 partial unique index——我建了一个：
```
index created: OK
second terminal refused: UNIQUE constraint failed: e.run_id
post-terminal text event: ACCEPTED — index does not seal
```
第三行是它输的原因。索引只拒第二条终态，而 `assertRunNotSealed` 拒绝终态之后的**任何**事件，也管 partial，并在 `BEGIN IMMEDIATE` 里给出类型化的 `RunSealedError`。所以索引删不掉任何既有陈述，只增加一条，还要再加一条错误翻译路径才能保住你测过的跨进程语义。净增复杂度，换来一个严格更弱的保证。方案 3 随之落空，而且值得点明：隔离这一步的存在只是为了收拾方案 1 引入的故障模式。方案 2 就是本 PR：已经没有第二份记录需要与终态事件同事务提交。
**方案 4 才是对的，而认真执行它反倒推翻了我已经做出的改动。** 我先前把核心重建改成「最后一个事件若为终态」来向 SQLite 索引看齐。那是反的。陈述这条规则的读者是四个不是三个：recovery、`classifyRuntimeEventTerminalFact`、以及 `runtime-resume.ts:576` 的尾检查，全都取该 run 的**第一条** terminal。只有 SQLite 清单在读尾。
这个差异可达，而且不经由损坏。run 从 #4242 起才封口；在那之前按 stop 会在流仍在 drain 时结束 run，那些 post-terminal straggler 被真实提交，且从没有任何机制清理过。在这类账本上，清单说 active 而 recovery 说 ended——于是 `finalizeChildWorkspacePatches` 对一个已完成的 run 抛「cannot finalize while Run is nonterminal」，continuation discovery 则完全看不到它。header 时代这被吸收掉了，因为「是否结束」读的是 header，尾随事件无害。
所以现在由清单来陈述其余三处已经在陈述的那句话。core 和 read model 一行未改。对 #4242 之后写下的任何账本，行为完全不变，因为封口使第一条终态就是唯一那条。两条终态仍是损坏，拒绝它的读者继续拒绝；会话列表报它能看到的终态，而不是因为一个 run 就让整个会话不可读。
封口现在是 `RuntimeEventStore` 的义务而非某个 store 的性质——声明在 `appendRuntimeEvent` 上，并要求 `ensureTerminalRuntimeEventDurable` 在同一事务里提交终态与封口检查。你说它底下没有东西托着，这个批评是中肯的；答案是把它放到每个实现和替身都必须遵守的接口上，而不是在它下面加一个更弱的约束。
**延后处理，附证据。** `runtime_continuation_claims.start_event_id` 是 `REFERENCES runtime_events(event_id)` 且无 `ON DELETE` 子句，默认 NO ACTION。purge 一个曾启动过 continuation 的会话会被直接拒绝：
```
purge REFUSED: FOREIGN KEY constraint failed
```
这早于本 PR——该表和该列来自 migration 15 及更早——本次改动也没让它变糟。它需要一条自己的跨会话 claim 退休规则，不该并进本次改动。我会另开 issue。
**验证。** core 802 / storage 1108 / runtime 3209 / runtime-host 1674 / desktop 2078，零失败；format 与 lint 干净。已 rebase 到 `main` 的 `b39e8d36ef`，无冲突。新增回归用例：shelved opening 锚定其 run 的第一条事件，删掉该事件后随之消失；迁移过的会话在事件被删后，shelf 为空、两个 reader 的清单均为空；以及封口之前时代的 straggler 跟在终态后面时仍读作 ended，索引与重建结果一致。
</details>
---
> Written with Claude Code; every verdict above was checked against the code and the probes before posting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(runtime): converge AgentRun metadata into the RuntimeInvocation event spine [maka]
Posted by "jackwener (via GitHub)" <gi...@apache.org>.
jackwener commented on PR #4631:
URL: https://github.com/apache/maka/pull/4631#issuecomment-5530254306
Correcting one of my own findings, at `db2638338e6991eea3f23b29a2eba5ec65989a1b`.
**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 `bf5ca672d`, `ai-sdk-backend.ts` is shorter than `main`'s, and `MAX_SEALED_THINKING_RETRIES_PER_STEP` and its tests are absent here.
What does not follow is that merging would revert it. A three-way merge of current `main` with this head produces no content conflict in that file, and the resulting tree carries `main`'s implementation and its tests — I checked the merged tree directly, and the constant is present there exactly as it is on `main`. The branch never deleted those lines; it is simply based on an older point, and a three-way merge keeps what only one side added. So nothing is lost by merging as it stands, and **#4393 does not need to be re-merged into this branch.** I should have tested the merge result rather than reasoning from the head's contents; my apologies for the noise.
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 `runtime-event-read-model.ts`, `main`'s change is expressed against the old header and this branch's against the new invocation spine. The two texts merge without conflict because they touch different lines, but automatic merging cannot judge whether the combination is right. That one needs a compile and a behaviour check on the post-rebase head — from whoever does the next rebase, not from the merge tool.
**The other findings stand unchanged**: the `runComposition` migration P1, whose mechanism is the `hasExactShape()` guard ahead of the per-field checks, and the P2 where the two-terminal rule lives only in the in-memory rebuild while `completeInvocationRecordSync` still reads the last event. Neither is affected by this correction.
<details><summary>简体中文</summary>
更正我自己的一条发现,针对 `db2638338e6991eea3f23b29a2eba5ec65989a1b`。
**P1(b)——「rebase 丢掉了 #4393」——撤回。观察是对的,推论是错的。**
成立的部分:这个 head 确实不含 `bf5ca672d`,`ai-sdk-backend.ts` 比 `main` 的短,`MAX_SEALED_THINKING_RETRIES_PER_STEP` 及其测试在这里都不存在。
**不成立的是「合入会撤销它」。** 用当前 `main` 与这个 head 做三方合并,该文件没有内容冲突,而且**合成树里带着 `main` 的实现和测试**——我直接检查了合并后的树,那个常量在其中,和 `main` 上一模一样。这个分支从未删除那些行,它只是基于一个较旧的起点,而三方合并会保留只有一侧新增的内容。**所以照现状合入不会丢东西,#4393 也不需要重新合并进这个分支。** 我应该去检验合并结果,而不是从 head 的内容去推断;为这次噪音致歉。
真正的风险转移到了下一次 rebase:如果之后的冲突解决让 PR 侧的整份文件获胜,这些行才会在那时丢失。那时值得重新核对,而不是现在就动手。
**有一点确实值得注意,而且是这次更正带出来的**:合并树干净,不等于合并后的语义正确。在 `runtime-event-read-model.ts` 里,`main` 的改动是针对旧 header 口径写的,而本分支是针对新的 invocation spine 写的。两段文本因为触及不同的行而无冲突地合在一起,**但自动合并无法判断这个组合是否正确**。这一处需要在 rebase 之后的 head 上做一次编译和行为复核——由做下一次 rebase 的人来做,而不是交给合并工具。
**其余发现不受影响,维持原判**:`runComposition` 迁移那条 P1(其机制是排在逐字段校验之前的 `hasExactShape()` guard),以及那条 P2(两条 terminal 的规则只存在于内存重建中,而 `completeInvocationRecordSync` 仍读取最后一条事件)。
</details>
---
> **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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(runtime): converge AgentRun metadata into the RuntimeInvocation event spine [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #4631:
URL: https://github.com/apache/maka/pull/4631#issuecomment-5535158746
Correcting my own scope call from the previous comment, at `8cd09afaca`.
**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 `runtime_continuation_claims` and rewriting every row's JSON through `projectContinuationClaimOpenings`. The table is squarely inside this change, so leaving a known refusal on it would have been the kind of intermediate state a later PR has to explain.
The fix is smaller than the retirement rule I imagined needing. `start_event_id` gets `ON DELETE CASCADE`. A continuation whose target Session 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 claimed but never started, and resume would try to start it against a target that no longer exists. SQLite cannot alter a foreign key in place, so the column rename becomes a table rebuild and does both at once — migration 15 rebuilt this same table the same way. The regression case starts a claim, purges the target Session's events, and asserts the claim is gone rather than the delete being refused.
**Two smaller things from the same pass, both of which you had flagged in substance.**
`ambiguous` is now `corrupt`. You were right that nothing is ambiguous about a second terminal event — a store seals a run on its first one, so a second means the ledger was written around the seal. The strict recovery error now says what it found instead of that it was unsure.
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 `appendRuntimeEvent` as a way to write into the ledger rather than to exercise the seal; they now seed the double directly, which is where a ledger shape no supported store can produce belongs.
**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 `isTerminalRuntimeEvent`; and one test assertion restated what the purge regression already covers end to end. One thing survived the ablation and is now explained in the schema rather than left to be re-derived: the `UNIQUE` on `anchor_event_id` is not a redundant constraint, it is the index for that side of the foreign key. SQLite indexes only the parent, 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.
Suites after a rebase onto `main`: core 804 / storage 1109 / runtime 3209 / runtime-host 1674 / desktop 2105, zero failures, lint and format clean.
<details><summary>简体中文</summary>
更正我上一条评论里的范围判断，对应 `8cd09afaca`。
**我说 continuation claim 的外键会另开 issue。那是错的，现在直接在本 PR 修掉了。** 我给的理由——本 PR 不碰那条路径——经不起核对：migration 16 本来就在重命名 `runtime_continuation_claims` 的一列，并通过 `projectContinuationClaimOpenings` 重写了每一行的 JSON。这张表完全在本次改动范围内，把一个已知的拒绝留在它上面，正好是那种要靠后续 PR 才能解释的中间态。
修法比我设想的退休规则小得多。`start_event_id` 加 `ON DELETE CASCADE`。目标 Session 已被删除的 continuation 不再指向任何东西，于是 claim 随之消失，它占着的 source 边界也重新释放。`SET NULL` 会比这个 bug 更糟：claim 会以「已认领但从未启动」的样子存活下来，resume 会试图把它启动到一个已经不存在的 target 上。SQLite 无法原地修改外键，所以那次列重命名变成一次表重建，两件事一起做——migration 15 对同一张表就是这么重建的。回归用例启动一个 claim，purge 掉目标 Session 的事件，断言 claim 消失而不是删除被拒绝。
**同一轮里的另外两件小事，你在实质上都提过。**
`ambiguous` 改成了 `corrupt`。你说得对，第二条 terminal 事件没有任何含糊之处——store 在第一条 terminal 上封口，所以出现第二条意味着这份账本是绕过封口写的。strict recovery 的错误信息现在陈述它发现了什么，而不是说自己不确定。
封口现在为测试替身陈述一次，所有替身共用。此前只有一个替身遵守，这正是让那个状态看起来可达的原因。有六处 fixture 一直通过契约禁止的 API 往已结束的 run 上追加事件，把 `appendRuntimeEvent` 当成写入账本的手段而不是在检验封口；它们现在直接给替身塞数据，那才是「任何受支持的 store 都产生不了的账本形态」该待的地方。
**做了一轮消融，因为一个以移除权威为目的的改动，不该悄悄把权威加回去。** 砍掉三处重复：backfill 在重复问同一行，顺带消掉一个被查询本身变成不可达的分支；终态谓词此前在每个需要它的查询里各写一遍，现在作为 `isTerminalRuntimeEvent` 的 SQL 那一半只陈述一次；还有一条测试断言重复了 purge 回归已经端到端覆盖的内容。有一样东西挺过了消融，现在把理由写进 schema 而不是留给人重新推导：`anchor_event_id` 上的 `UNIQUE` 不是冗余约束，它是那一侧外键的索引。SQLite 只索引父侧，没有它每删一条 RuntimeEvent 都要全扫 shelf——两万事件配四千行 shelf 实测，去掉它删除从 8ms 变成 250ms。
rebase 到 `main` 之后的套件：core 804 / storage 1109 / runtime 3209 / runtime-host 1674 / desktop 2105，零失败，lint 与 format 干净。
</details>
---
> Written with Claude Code; every verdict above was checked against the code and the probes before posting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(runtime): converge AgentRun metadata into the RuntimeInvocation event spine [maka]
Posted by "jackwener (via GitHub)" <gi...@apache.org>.
jackwener commented on PR #4631:
URL: https://github.com/apache/maka/pull/4631#issuecomment-5531810934
Following up on the P1 I raised for `aa0d369db48de080d29462eff6a9fb45ddd09ba0` — **with the execution consequence measured, and with a narrower trigger condition than my earlier comment implied.**
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 happens
Full production chain: create a real active Session; create a header-era completed run with `text` and `completed` events; migrate to schema 16; call the production `purge(sessionId)`; **leave the Session metadata in place** — modelling a crash after the purge commits and before session removal does; reopen the real Session, AgentRun and RuntimeEvent stores; then call `SessionManager.recoverInterruptedSessionsStrict(...)`, which is what Runtime Host startup actually calls.
Two independent runs agree:
- Strict recovery **returns that session id** — it neither skips it nor fails closed.
- It writes a **new terminal RuntimeEvent into the purged session**: `event_kind=error`, `status=failed`, `failureClass=app_restarted`.
- It also writes a `turn_state` of `failed` / `app_restarted` for that turn, and sets the Session header back to **`active`**.
- The shelf row is still there afterwards, and `readRunInvocation` then composes the shelved opening with the new terminal into a `failed` invocation.
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 implied
This 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: `execution-composition.ts:1585-1593` commits the import-discard purge and the session removal separately, and `session-revision-coordinator.ts:874-890` purges sidecars before discarding the session. Neither pair is one transaction. So it is a fault-and-recovery path, not a steady-state one — which is worth stating plainly, because it changes how the fix should be prioritised and tested.
### Bounds of the claim
No 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 `runtime_legacy_invocation_openings WHERE session_id = ?` in the same purge transaction, and add a regression case that ends with both inventory readers and the shelf empty.
*Evidence boundary: both runs are probes against this head, recorded with digests; the second one drives the real `SessionManager` entry point rather than a stub. Hosted checks remain green — this is not something CI would catch.*
<details><summary>简体中文</summary>
接上一条针对 `aa0d369db48de080d29462eff6a9fb45ddd09ba0` 的 P1——**这次带上了实测出来的执行后果,同时给出一个比我先前表述更窄的触发条件。**
上一条只证明了这个幽灵**被读成** active,并没有证明有任何东西会据此动作。**这是一个中肯的质疑**,现在通过跑真实启动路径把它补上了。
**实际会发生什么**
完整生产链:创建一个真实的 active Session;造一个 header 时代、带 `text` 与 `completed` 事件的 completed run;迁移到 schema 16;调用生产的 `purge(sessionId)`;**保留 Session metadata**——模拟「purge 已提交、session removal 尚未提交」时发生崩溃;重新打开真实的 Session、AgentRun、RuntimeEvent stores;然后调用 `SessionManager.recoverInterruptedSessionsStrict(...)`,也就是 Runtime Host 启动时实际调用的那个入口。
两次独立运行结果一致:
- strict recovery **返回了那个 session id**——既没有跳过,也没有 fail closed。
- 它**向这个已被 purge 的 session 写入了一条新的 terminal RuntimeEvent**:`event_kind=error`、`status=failed`、`failureClass=app_restarted`。
- 它还为该 turn 写入了 `failed` / `app_restarted` 的 `turn_state`,并把 Session header 重新置为 **`active`**。
- 之后 shelf 行仍然存在,`readRunInvocation` 于是把这条被搁置的 opening 与新写的 terminal 合成为一条 `failed` invocation。
**所以这不是清单不好看的问题:启动过程会把数据写回一个用户正在删除或回滚的会话。**
**触发条件比我最初表述的更窄**
它需要在两次提交之间发生崩溃——一次是 operational purge,一次是 session metadata removal。**如果 session removal 成功了,启动恢复会从 Session metadata 枚举,根本触及不到这个幽灵。**
但这个窗口在普通代码路径里是可达的,不是硬构造出来的:`execution-composition.ts:1585-1593` 把导入丢弃的 purge 与 session removal **分两次提交**;`session-revision-coordinator.ts:874-890` 也是先 purge sidecar、再 discard session。**两者都不在同一个事务里。** 所以这是一条 fault/recovery 路径,而不是稳态路径——**这一点值得写明**,因为它会影响这个修复的优先级和测试方式。
**这条主张的边界**
**没有观察到任何 provider 或 tool 执行。** 后果是持久化的复活加上一条被写回的错误终态,**不是「重启了提供方进程」**。修法不变,而且仍然很小:把 `runtime_legacy_invocation_openings WHERE session_id = ?` 纳入同一个 purge 事务,并补一条回归用例,以「两个 inventory reader 与 shelf 均为空」收尾。
*证据边界:两次运行都是针对这个 head 的探针,均留有摘要;第二次驱动的是真实的 `SessionManager` 入口,不是 stub。托管检查仍然是绿的——**这不是 CI 能抓到的东西。***
</details>
---
> **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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(runtime): converge AgentRun metadata into the RuntimeInvocation event spine [maka]
Posted by "jackwener (via GitHub)" <gi...@apache.org>.
jackwener commented on PR #4631:
URL: https://github.com/apache/maka/pull/4631#issuecomment-5531344010
Following up on `aa0d369db48de080d29462eff6a9fb45ddd09ba0` with **one P1 that my earlier approval does not cover**, and one design question that has to be answered before the P1 can be fixed correctly.
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 — `purge()` leaves the legacy opening behind, and a completed invocation comes back as an active ghost
This PR adds `runtime_legacy_invocation_openings` (`sqlite-runtime-schema.ts:508-518,625-629`) to hold the opening of a legacy run that already owns an immutable event sequence — such a run cannot have an opening inserted at position 1 without rewriting signed history, so the opening is shelved instead. **That design is sound.** The problem is its lifecycle.
In production code that table is only ever written by the migration and read by two queries. There is no `DELETE` and no `DROP` anywhere outside tests. In particular, `conversation-operational-state.ts:45-93` deletes `runtime_events`, the AgentRun rows and a dozen sidecar tables — and not this one.
That matters because of how the shelf is read. `sqlite-runtime-store.ts:687-693` selects a shelved opening only when the invocation has **no** `invocation_opened` event. Purging the session deletes those events, so the condition becomes true and the shelf reappears.
Measured end to end against a real schema-16 migration, the production `purge(sessionId)`, both production inventory readers and the production `RuntimeReadModel`:
- Before purge — shelf 1 row, events `text,completed`, both readers return the invocation with `terminalEvent.status = completed`.
- After purge — `runtime_events` 0, `core_agent_runs` 0, **shelf still 1**. `listSessionInvocations` and `readRunInvocation` **still return the same invocation**, now with `terminalEvent` undefined, while `readRuntimeEvents` is empty.
- Fed to `RuntimeReadModel`, that state produces zero terminal facts and records `incomplete_event: active run is using the in-flight projection cache` — the read model classifies the ghost as **active** (`runtime-read-model.ts:134-158`).
To be precise about what a user sees: a deleted session is usually gone from the directory too, so this does not necessarily surface as a visible "running" row every time. But **this purge is not only the user-delete path** — conversation-copy rollback, import discard and session retirement all call it. Any path that still reads by that session id gets an active, incomplete ghost of a run that had completed.
**Fix**: delete from `runtime_legacy_invocation_openings` by `session_id` in the same transaction, and add a regression case covering migrated-run → purge → both inventory readers empty and shelf empty.
### The design question underneath it
The shelf is not a derived index. It cannot be rebuilt from events — that is the whole reason it exists — so it is a durable authority of its own, written only by the migration, with no stated exit condition. Worth naming it that way in the code rather than leaving it to be mistaken for a cache: `RuntimeEventStore`'s claim that dropping the index and rebuilding from events alone yields the same inventory is **false for a migrated Session**, and will stay false. Deciding explicitly whether it is retained for the life of the Session or has a real compatibility deadline is part of closing this.
### Still open from my previous comment
The two-terminal split is unchanged, and it needs a contract decision before anyone writes the fix. Three readers currently hold three different assumptions about the same ledger: the core rebuild takes the first terminal, the SQLite inventory takes the last one and only when it is the tail event, and `terminal-run-commit.ts` plus `runtime-event-read-model.ts` call more than one ambiguous. A retained test double still permits two terminals and asserts the read model rejects them.
Either answer closes it:
- **The state is unrepresentable.** Then sealing after a terminal becomes an implementation obligation of every `RuntimeEventStore`, carried by the interface and a shared conformance suite so the doubles obey it too, and the remaining ambiguous branches and their tests come out.
- **It is recoverable corruption.** Then one shared projector fails closed, in a single place.
**What cannot stay is the present third state**, where three readers disagree silently. Whichever is intended, saying so in the PR would help — the person who writes this fix needs the contract, not just the observation.
*Evidence boundary: the purge measurements above are probe runs against this head, reproduced twice; the probe is recorded with its digest. Hosted checks are green on this head — this finding is not a CI failure, and CI would not have caught it.*
<details><summary>简体中文</summary>
针对 `aa0d369db48de080d29462eff6a9fb45ddd09ba0` 追加一条 **P1,我先前的批准不涵盖它**,以及一个必须先回答、否则那条 P1 无法正确修复的设计问题。
我先前批准过这个 head。那次批准对它当时检查过的内容仍然有效;**但它不涵盖下面这条**——这条来自后续一轮,而且是**跑出来的,不是读出来的**。
**P1:`purge()` 留下了 legacy opening,一个已完成的 invocation 复活成了 active 幽灵。**
本 PR 新增 `runtime_legacy_invocation_openings`(`sqlite-runtime-schema.ts:508-518,625-629`),用来存放那些「已经拥有不可变事件序列」的旧 run 的开场——这类 run 无法在位置 1 插入开场而不改写已签名的历史,所以把开场另存一处。**这个设计是站得住的。问题在它的生命周期。**
在生产代码里,这张表**只被迁移写入、被两条查询读取。测试之外没有任何 `DELETE`,也没有 `DROP`。** 特别是 `conversation-operational-state.ts:45-93` 删除了 `runtime_events`、AgentRun 行和十几张 sidecar 表——**唯独没有这一张**。
这一点之所以要紧,在于 shelf 的读取方式:`sqlite-runtime-store.ts:687-693` 只在该 invocation **没有** `invocation_opened` 事件时才选出被搁置的开场。而清除会话会删掉这些事件,**于是那个条件反而成立,shelf 重新显影**。
用真实的 schema-16 迁移、生产 `purge(sessionId)`、两个生产 inventory reader 以及生产 `RuntimeReadModel` 端到端实测:
- **purge 前**——shelf 1 行,事件为 `text,completed`,两个 reader 都返回该 invocation,`terminalEvent.status = completed`。
- **purge 后**——`runtime_events` 为 0,`core_agent_runs` 为 0,**shelf 仍为 1**。`listSessionInvocations` 与 `readRunInvocation` **仍然返回同一个 invocation**,此时 `terminalEvent` 为 undefined,而 `readRuntimeEvents` 是空数组。
- 把这个状态喂给 `RuntimeReadModel`,它产出零条 terminal fact,并记录 `incomplete_event: active run is using the in-flight projection cache`——**读模型把这个幽灵判定为 active**(`runtime-read-model.ts:134-158`)。
**用户会看到什么,要说精确**:被删除的会话通常也已从目录中移除,所以这不一定每次都表现为一条可见的「进行中」。但**这个 purge 并不只是用户删除那条路径**——conversation-copy 回滚、导入丢弃、会话退役都会调用它。**任何仍按该 session id 读取的路径,都会拿到一个本已完成的 run 的 active、未完成幽灵。**
**修法**:在同一事务里按 `session_id` 删除 `runtime_legacy_invocation_openings`,并补一条回归用例,覆盖「迁移过的 run → purge → 两个 inventory reader 均为空、shelf 为空」。
**它底下的那个设计问题**
**这张 shelf 不是派生索引。它无法由事件重建**——那正是它存在的理由——所以它是一份自身的持久权威,只由迁移写入,且没有声明过的退场条件。**与其让它被误当成缓存,不如在代码里就这么命名它**:`RuntimeEventStore` 那句「丢掉索引、仅凭事件重建也能得到同一份清单」的声明,**对一个迁移过的 Session 是假的**,而且会一直是假的。明确决定它是随 Session 永久保留,还是有一个真实的兼容截止期,是收口这件事的一部分。
**我上一条评论里仍未关闭的**
两条 terminal 的分裂没有变化,而且**在有人动手修之前,需要先做一个契约决定**。目前三个读者对同一份账本抱着三种不同假设:core 重建取**第一条** terminal,SQLite 清单取**最后一条**且仅当它是尾事件时,而 `terminal-run-commit.ts` 与 `runtime-event-read-model.ts` 把多于一条判为 ambiguous。保留下来的一个 test double 仍然允许写入两条 terminal,并断言读模型会拒绝它们。
两个答案都能收口:
- **该状态不可表示。** 那么「终态之后封口」就成为每个 `RuntimeEventStore` 的实现义务,由接口和一套共用的 conformance 承载,**使 test double 也必须遵守**,并把剩余的 ambiguous 分支及其测试删掉。
- **它是可恢复的损坏。** 那么由**一个**共用 projector 统一 fail closed。
**不能保留的是现在这第三种状态**——三个读者静默地互不一致。无论意图是哪一个,**在 PR 里把它写明会有帮助**:动手修的人需要的是契约,而不只是现象。
*证据边界:上述 purge 的测量是针对这个 head 的探针运行,已独立复现两次,探针本身连同摘要一并留存。这个 head 上托管检查是绿的——**这条发现不是 CI 失败,CI 也抓不到它。***
</details>
---
> **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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(runtime): converge AgentRun metadata into the RuntimeInvocation event spine [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on PR #4631:
URL: https://github.com/apache/maka/pull/4631#issuecomment-5530422860
Thanks — the composition finding was real and the fixture critique was the right diagnosis. Addressed at head `aa0d369db4`, with an adversarial pass of my own alongside (six angles: migration fidelity against the merge base's real write shape, rebase fidelity per upstream PR, terminal derivation, host recovery and concurrency, hidden-event leakage into consumers, smallest shape). Verdicts and what changed:
**P1 (a) — fixed at the migration.** Confirmed on a schema-15 database built from the base's own writer: the only key `main` writes that the legacy shape lacked was `runComposition`, and `hasExactShape` rejected the header before any field check. Three changes, one commit: the legacy shape accepts the field; a header the migration cannot read now **stops the migration** (the transaction rolls back, the error names the row) instead of being skipped ahead of the column drop; and a header-only run whose header recorded an ending now gets that terminal event too, so a completed legacy run does not migrate as open. The regression case is built from a base-era header with a real `createRunCompositionSnapshot()` payload, and there is a second case asserting the refusal leaves every header in place. The composition snapshot itself is deliberately not carried onto the spine: nothing on `main` reads it back (the only consumer is the write-once comparison at append time), so the PR body no
w states that rather than leaving it to a swallowed error.
**P1 (b) — not a rebase loss.** `bf5ca672dc` (#4393) is not an ancestor of this branch's merge base `ab7b739260`; it landed on `main` after the rebase. `git merge-tree --write-tree HEAD origin/main` is clean, and #4393's four hunks (`:892`, `:2085`, `:2108`, `:2713`) do not touch the PR's two single-line hunks in that file. It will come in whole on the next rebase, which this push includes.
**P2 — the split is not reachable, and the rule behind it is gone.** `insertRuntimeEvent` runs `assertRunNotSealed` unconditionally (`sqlite-runtime-store.ts:3612-3630`, present on the merge base), so neither a second terminal nor an event after the terminal can be written — probed in-process and across two processes (`BEGIN IMMEDIATE`; the loser gets `RunSealedError`). That makes the count-two-terminals branch in `runtimeInvocationsFromSessionEvents` a rule only a test double could reach, and one that contradicted the index by construction. It is deleted, along with the two reader branches added for it and the test that arranged the unrepresentable state; the rebuild now states the same rule as the index: an invocation's terminal event is its one terminal event.
**Found by the adversarial pass, fixed in the same push:**
- **P1 — a conversation copy of a migrated Session lost the invocation.** The migration shelves the opening of any run that already owned events; a copy cloned only events, so the target had no opening, `listSessionInvocations` returned nothing, and the branched Session's first send saw no history. The base cloned the header explicitly, so this was a regression. The copy now restores a shelved opening as event 1 of the copied run (the copy is a fresh sequence), with a regression test that stages the shelved state.
- **P2 — point reads had become full-Session scans.** `readRunIfPresent`, `readInvocationIfPresent` and `SessionManager.readInvocation` were `listSessionInvocations().find()`, called 4–6 times per Turn. `RuntimeEventStore` gains an optional `readRunInvocation(sessionId, runId)` fast path (the SQLite store indexes openings), and one helper in core answers it from the inventory for stores without one.
- **P2 — `SessionAdmissionGate.detach` had no test and was not listed.** Now both.
- **P3 — an opening is unique per invocation by index**, not by convention (partial unique index in the same migration); the anchor reader's guess of `sessionInline: false` for a run with no opening is replaced by an error; and `acceptedInputBoundary` states that an `unknown` route never proves a span, which is the one place the branch is stricter than the header comparison was.
Verified intact by the rebase-fidelity pass: #4653, #4667/#4671, #4669, #4379, #4439, #4132 — every hunk `main` added is present or reads the same fact off the spine.
Local: core / storage / runtime / runtime-host / desktop suites green (counts in the PR body), format and lint clean. The earlier `Desktop e2e` red was `code-scroll.spec.ts:22`, tracked as #4633; it passed on rerun and this PR does not touch `apps/desktop/e2e`.
<details><summary>简体中文</summary>
谢谢——composition 那条是真的，对 fixture 的批评也点在了要害上。已在 `aa0d369db4` 处理，同时我自己也做了一轮对抗性评审（六个角度：以 merge base 真实写入形态核迁移、逐上游 PR 核 rebase 保真、终态派生、Host 恢复与并发、隐藏事件对消费者的泄漏、最小形态）。裁决与改动：
**P1(a)——在迁移层修。** 用 base 自己的 writer 造 schema-15 库确认：`main` 会写而 legacy 形状缺的键只有 `runComposition`，且 `hasExactShape` 在逐字段校验前就拒绝。一个 commit 三处改动：legacy 形状接受该字段；迁移读不懂的 header 现在**中止整个迁移**（事务回滚，错误指名该行），不再在删列之前跳过；header 记录了终态的零事件 run 也补上 terminal 事件，已完成的旧 run 不再迁成 open。回归用例用 base 时代 header 加真实 `createRunCompositionSnapshot()` 构造，另有一例断言拒绝时所有 header 原地保留。composition 快照本身刻意不搬上 spine：`main` 上没人读它（唯一消费者是 append 时的 write-once 比对），PR 正文现在明说这一点。
**P1(b)——不是 rebase 丢失。** `bf5ca672dc`（#4393）不是本分支 merge base `ab7b739260` 的祖先，它是在 rebase 之后才进 `main` 的。`git merge-tree --write-tree HEAD origin/main` 干净，#4393 的四个 hunk 与本 PR 在该文件的两个单行 hunk 互不相接。下次 rebase 会整个带入，本次推送已包含。
**P2——分歧不可达，背后那条规则已删。** `insertRuntimeEvent` 无条件执行 `assertRunNotSealed`（`sqlite-runtime-store.ts:3612-3630`，merge base 已有），第二条 terminal 和 terminal 之后的事件都写不进去——进程内和跨进程（`BEGIN IMMEDIATE`，输家收到 `RunSealedError`）都探针验证过。于是 `runtimeInvocationsFromSessionEvents` 里数两条 terminal 的分支只有测试替身能到达，而且按构造就与索引矛盾。已删除，连同为它加的两处读取分支和构造该不可表示状态的测试；重建现在与索引陈述同一规则：invocation 的 terminal 事件就是它唯一的那条。
**对抗性评审发现并随本次推送修复：**
- **P1——复制已迁移的 Session 会丢掉 invocation。** 迁移把已有事件的 run 的 opening 放上货架；复制只克隆事件，目标没有 opening，`listSessionInvocations` 为空，分支出的 Session 首次发送看不到任何历史。base 显式克隆 header，所以这是回归。复制现在把货架上的 opening 还原为复制 run 的事件 1（复制是全新序列），附带构造货架状态的回归用例。
- **P2——定点读退化成整会话扫描。** `readRunIfPresent`、`readInvocationIfPresent`、`SessionManager.readInvocation` 都是 `listSessionInvocations().find()`，每 Turn 调 4–6 次。`RuntimeEventStore` 增加可选的 `readRunInvocation(sessionId, runId)` 快路径（SQLite store 对 opening 有索引），core 里一个 helper 为没有快路径的 store 从清单作答。
- **P2——`SessionAdmissionGate.detach` 无测试且未列出。** 现在两者都有。
- **P3——opening 每 invocation 唯一由索引保证**，不再靠约定（同一 migration 里的 partial unique index）；anchor reader 对无 opening 的 run 猜 `sessionInline: false` 改为报错；`acceptedInputBoundary` 注明 `unknown` route 永不构成已证明跨度——这是本分支唯一比 header 比较更严的地方。
rebase 保真核实无损：#4653、#4667/#4671、#4669、#4379、#4439、#4132——`main` 加的每个 hunk 要么在，要么改为从 spine 读同一事实。
本地：core / storage / runtime / runtime-host / desktop 套件全绿（数字在 PR 正文），format 与 lint 干净。此前 `Desktop e2e` 的红是 `code-scroll.spec.ts:22`，已由 #4633 跟踪；重跑通过，本 PR 不触碰 `apps/desktop/e2e`。
</details>
---
> Written with Claude Code; the verdicts above were checked by me against the code and the probes before posting.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(runtime): converge AgentRun metadata into the RuntimeInvocation event spine [maka]
Posted by "jackwener (via GitHub)" <gi...@apache.org>.
jackwener commented on PR #4631:
URL: https://github.com/apache/maka/pull/4631#issuecomment-5531868669
One addition to the contract question on `aa0d369db48de080d29462eff6a9fb45ddd09ba0`. I gave two ways to close it earlier; **there is a third that is better than either, and it removes a weakness I named but did not solve.**
My framing was: either the state is unrepresentable, and sealing after a terminal becomes an implementation obligation of every `RuntimeEventStore` — or it is recoverable corruption, and one shared projector fails closed.
The weakness in the first option is that this repository has no storage-level constraint behind it. The unique index covers `invocation_opened` only, and `runtimeEventKind` resolves from `content.kind` / `status`, so most terminal events are not `invocation_end` and a kind-based unique index would not catch them either. What actually refuses a second terminal today is `assertRunNotSealed` inside the write transaction, with `BEGIN IMMEDIATE`. That has been measured as effective — two independent cross-process probes, one running 25 rounds of `completed` against `failed` and another releasing two processes together past their preliminary "no terminal" read, both produced exactly one commit and a typed `RunSealedError`, with same-terminal retries idempotent. **But it is an application invariant, not a storage guarantee.** Choosing "unrepresentable" without adding a constraint leaves that sentence with nothing underneath it.
The stronger form:
- **Make single-terminal a persistent uniqueness constraint**, so the guarantee lives where the data lives.
- **Commit the terminal event and its fact in one transaction**, so there is no ordering in which one exists without the other.
- **Quarantine on migration** when a legacy ledger already carries more than one terminal.
- **Read canonical facts only**, on both sides — which is what removes the third assumption rather than relocating it.
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.
<details><summary>简体中文</summary>
针对 `aa0d369db48de080d29462eff6a9fb45ddd09ba0` 的契约问题补充一点。我先前给了两条收口路径;**其实还有第三条,比那两条都好,而且它补掉了一个我点出过、却没有解决的弱点。**
我原来的框架是:要么这个状态**不可表示**,那么「终态之后封口」就成为每个 `RuntimeEventStore` 的实现义务;要么它是**可恢复的损坏**,那么由一个共用 projector 统一 fail closed。
**第一条路的弱点在于:这个仓库背后没有存储层的约束。** 唯一索引只覆盖 `invocation_opened`;而 `runtimeEventKind` 是从 `content.kind` / `status` 解析的,**大多数终态事件并不是 `invocation_end`**,所以基于 kind 的唯一索引同样拦不住。今天真正拒绝第二条终态的,是写事务内的 `assertRunNotSealed` 配合 `BEGIN IMMEDIATE`。这一点已被实测证明有效——两次独立的跨进程探针,一次跑了 25 轮 `completed` 对 `failed`,另一次让两个进程越过各自的「尚无终态」预读后同时释放,**都只产生一次提交加一个类型化的 `RunSealedError`,同终态重试是幂等的**。**但它是应用层不变量,不是存储保证。** 选了「不可表示」却不加约束,这句话底下就没有东西托着。
**更强的形式:**
- **把「单终态」提升为持久化的唯一性约束**,让保证长在数据所在的地方。
- **在同一个事务里提交终态事件和它的事实**,这样就不存在「一个已存在而另一个还没有」的中间序。
- **迁移时对已存在多终态的旧账本做隔离。**
- **两边都只读规范事实**——这才是把第三种假设**消除**,而不是换个地方安放。
**其中「隔离」这一步是我原本想不到的,而且它很关键**:一个已经含有双终态账本的数据库,会在迁移时撞上新约束——**而一个升级不上去的用户,比一个读到不一致状态的用户处境更糟**。只加约束、不决定既有违例怎么办,是拿一个问题换一个更糟的问题。
purge 那条修法不受影响:在同一事务里删掉 shelf 行,或者留一个墓碑。
以上都不改变那条 P1 或它底下的发现——这只是关于**契约定下来之后,该选哪一种修法**。
</details>
---
> **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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] refactor(runtime): converge AgentRun metadata into the RuntimeInvocation event spine [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han merged PR #4631:
URL: https://github.com/apache/maka/pull/4631
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org