refactor: make Runtime Host the sole Message admission authority - #3803
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Update on bda2eccf2d:
[P2] busy-race fallback leaves ghost transient row
When renderer thinks idle but host is busy (another window/Bot/Goal continuation holds root), startTurn throws session_busy and falls back to submitMessage({messageId: renderer turnId}). Host then creates a new turnId for the turn while the durable message id stays as renderer turnId. The IPC returns host turnId without messageId, and renderer uses host id for optimistic row — never matches durable id → permanent duplicate (transient ghost).
Fix: return messageId on the turn_started branch and prefer it as transient key; add test for busy→turn_started path.
Note: PR is still draft; this review is preparatory and pending re-review after head stabilizes.
简体中文
busy 情况下回退导致重复行。bda2ecc to
f5ff581
Compare
Astro-Han
left a comment
There was a problem hiding this comment.
Update on efee92917:
[P2] Skill invocation failure toast regressed
Old path returned ok:false/skillInvocation for toast "Skill 调用失败…", new turn.message.submit path throws operation_conflict which bubbles as generic failure — specific toast lost; e2e skill-draft-lifecycle now fails (2 cases).
Fix: map skill blocked to ok:false/skill_invocation_failed in IPC or renderer adapter.
Prior busy-race ghost-row P2 is now closed (messageId alignment + idempotence). Checks on efee92917 are test: FAILURE — not green.
简体中文
技能调用失败的提示回归。There was a problem hiding this comment.
I reviewed this head and found no blocking issues.
Old busy-race ghost-row and skill-failure regressions are closed (messageId alignment and skill-token routing); new transient/durable separation correctly handles ordering and Host-bound turn identity. Hosted test: SUCCESS (32907835235).
No P0-P2; non-blocking P3 on turn_started branch missing messageId (contract alignment).
简体中文
该头无阻断,旧 P2 已闭合。Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
a18006b to
a83714a
Compare
M4n5ter
left a comment
There was a problem hiding this comment.
English
Review on 587a1da5a.
The architectural direction is sound: Runtime Host should be the sole ordinary Message admission authority, while Desktop and CLI own only transient presentation and authoritative reconciliation. However, I do not think this head is ready to merge. I found one blocking admission-ordering issue and several Important correctness gaps.
Blocking
-
CLI admissions are unordered, and fresh Session creation is not single-flight.
packages/cli/src/pi-tui-runner.ts:939-968starts every Enter as an independentdriver.submitMessage(), whilepackages/cli/src/runtime-host-session-driver.ts:915-948lets concurrent#ensureSession()calls each create a different durable Session. On a fresh TUI, two quick Enters can therefore lose one Message, split them across Sessions, or leave an invisible/orphaned active Session. Even with an existing Session, the per-submit configuration reads can reorder Host arrival and change which Message becomes the root versus steering; identity/config controls can also cross the pending admission window.Please keep optimistic rows immediate, but serialize Host admission through an ordered tail, reuse one in-flight Session creation, and fence
/new,/session,/model, etc. against that tail. A production-driver test should assert onesession.create, one Session identity, and stable submit order under delayed responses.
Important
-
Exact-Turn intent and terminal refusal are not part of durable Message identity.
packages/runtime-host/src/server/message-coordinator.ts:2131-2150,2280-2293compares durable replay using only content and placement, although the canonical payload now also containsskillIdsandturnOrchestration. In the pending-Message/root-admission crash window (root-turn-coordinator.ts:1088-1102), recovery cannot restore that intent and can execute with the Session's default orchestration. Across Host epochs, the same Message identity with changed exact intent can also be misreported as the old successful Turn. Separately,message-coordinator.ts:901-908deliberately persists no result forblocked; if that response is lost, neither transcript nor cancellation proof can ever retire the client's transient row.The durable submit fingerprint should cover normalized content, original placement, Skill ids, and orchestration. Pending recovery should persist the prepared orchestration/Skill receipt, and
blockedshould be a keyed durable terminal result that submit/query can replay. -
Desktop's exact-Turn arm remains keyed by
messageIdafter Host returns a differentturnId.apps/desktop/src/renderer/app-shell-chat-actions.ts:389,458,538arms the live Turn with the client Message identity, but the successful result only stores HostturnIdon the transient row. If the Turn ends before any text/tool/steering event, the terminal event is ignored by the exact-id projection and the unconfirmed arm is never retired, leaving processing/Stop permanently active. CAS-rebind the still-waiting arm onturn_started; onoutcome_unknown, keep only the Message transient and release the exact-Turn arm. -
A refused Follow Up is still reported to Composer as successfully sent.
apps/desktop/src/renderer/app-shell-chat-actions.ts:635-675discardssubmitAndProject()'srefusedresult, soapps/desktop/src/renderer/app-shell.tsx:1870-1904clears attachments/quotes and returnstrue; Composer then clears the draft. This is reachable when a running Turn settles during admission and an invalid Skill Follow Up is considered for a fresh Turn. Returnfalsefor refusal, while retaining the existing accepted/outcome_unknownbehavior. -
A second dispatched interruption is treated as definite non-delivery.
apps/desktop/src/main/runtime-host-session-execution-ipc-main.ts:71-86,118-132retries once after a dispatched interruption, but a second dispatched interruption escapes as an exception. Renderer catch paths then delete the optimistic Message, and may remove a newly created Session, even though the retry may have reached Host. The final catch must map a dispatchedRuntimeHostRequestInterruptedErrortooutcome_unknownas well. -
Cancellation-proof queries exceed their own protocol limit at supported capacity.
apps/desktop/src/renderer/use-app-shell-session-workspace.ts:136-151submits the entire transient map, whilepackages/runtime-host/src/protocol/message.ts:312-324accepts at most 64 ids. A legal 64-entry Host queue plus one unreconciled root transient makes the whole proof query fail, so no cancelled row is retired. Query in chunks of at most 64 and merge the results. -
CLI reattach bypasses the Side Conversation transcript visibility boundary.
packages/cli/src/runtime-host-session-driver.ts:1164-1173puts rawopened.messagesinto the attached Turn, andpi-tui-runner.ts:1170-1180replaces the visible transcript with it. The first Host-started Turn in/sidetherefore makes the copied parent transcript visible again. Apply the existingvisibleTranscriptMessages()filter to reattach messages too. -
CLI drops successful/partially successful Skill admission feedback.
pi-tui-runner.ts:948-956handles onlyblocked; it ignoresturn_started.skillInvocation, and the production reattach object does not carry it either. The current test passes only because its fake callspreparePrompt()and injects the receipt through a path production no longer uses. Project the submit result's Skill receipt and replace that test with the real submit-plus-plain-attach shape. -
The removed Desktop IPC still has a real-window E2E consumer.
apps/desktop/e2e/streaming-remount.spec.ts:234still callswindow.maka.sessions.steer. Exact-head CI fails deterministically withTypeError: ...steer is not a function. Migrate it tosessions.submitMessage(sessionId, 'current_turn', { messageId, text })and fail closed on a refused result. The other prompt-rail failure is independently tracked by #3862/#3863 and is not attributable to this PR.
Simplify audit
One non-blocking simplification is worth doing while this model is new: narrow TransientUserMessageProjection to the presentation fields its consumers actually read, instead of fabricating a durable turnId: messageId alongside the real hostTurnId. Also make the public submit IPC fail closed when messageId is absent and remove command.messageId ?? newId(); the renderer contract and every production caller already assign the stable identity.
Verification: the PR merges cleanly with current main@e6db07e; the merged CLI suite passes 489/489. Exact-head lint, format, build, typecheck, knip, affected workspaces, Eval egress, Runtime Host tests, and compatibility epoch guard passed before Desktop E2E. The required test check is currently red because of the stale IPC consumer above. I did not perform Windows/macOS hardware validation; the changed protocol paths are otherwise platform-neutral and the epoch handshake fails closed.
简体中文
审查基于 587a1da5a。
架构方向是对的:普通 Message 的 admission 应只由 Runtime Host 决定,Desktop/CLI 只持有 transient presentation,并通过权威事实收敛。但当前 head 还不适合合并;我确认了一个阻断性的 admission 顺序问题和若干 Important correctness 缺口。
Blocking
-
CLI admission 无顺序边界,fresh Session 创建也没有 single-flight。
packages/cli/src/pi-tui-runner.ts:939-968会把每次 Enter 都作为独立driver.submitMessage()并发启动,而packages/cli/src/runtime-host-session-driver.ts:915-948允许并发#ensureSession()各自创建不同的持久 Session。fresh TUI 下两次快速 Enter 可能丢失一条消息、把消息拆到不同 Session,或留下不可见/孤立的 active Session;已有 Session 上,提交前的配置读取也可能改变到达 Host 的顺序,从而改变 root/steering disposition,Session/模型控制还会穿过 admission window。应保持 optimistic row 立即显示,但使用有序 admission tail 串行提交,复用一个 in-flight Session creation,并让
/new、/session、/model等控制操作与该 tail 建立 fence。生产 driver 测试需在延迟响应下验证只发生一次session.create、两条消息使用同一 Session 且顺序稳定。
Important
-
exact-Turn intent 和拒绝终态没有进入 durable Message identity。
packages/runtime-host/src/server/message-coordinator.ts:2131-2150,2280-2293的 durable replay 只比较 content/placement,但 canonical payload 已包含skillIds和turnOrchestration。在 pending Message 已提交、root admission 尚未写入的窗口(root-turn-coordinator.ts:1088-1102)发生崩溃时,恢复无法还原这些 intent,可能回退到 Session 默认 orchestration;跨 Host epoch 时,相同 Message identity 但不同 exact intent 也可能被误报为旧 Turn 的成功结果。另外,message-coordinator.ts:901-908不持久化blocked;响应一旦丢失,transcript 和 cancellation proof 都无法退休客户端 transient。durable submit fingerprint 应覆盖规范化 content、原 placement、Skill ids 和 orchestration;pending recovery 还需保存 prepared orchestration/Skill receipt;
blocked应成为可由 submit/query 重放的 keyed durable terminal result。 -
Desktop exact-Turn live arm 在 Host 返回不同
turnId后仍绑定messageId。apps/desktop/src/renderer/app-shell-chat-actions.ts:389,458,538先用客户端 Message identity arm Turn,但成功结果只把 HostturnId写入 transient row。若 Turn 在首个 text/tool/steering event 前终止,terminal event 会因 ID 不匹配被忽略,unconfirmed arm 永远无法退休,processing/Stop 会永久保持 active。turn_started后应仅对仍 waiting 的 arm 做 CAS rebind;outcome_unknown只保留 Message transient,并解除 exact-Turn arm。 -
Host 拒绝的 Follow Up 仍被 Composer 当作成功发送。
apps/desktop/src/renderer/app-shell-chat-actions.ts:635-675丢弃submitAndProject()的refused结果,因此apps/desktop/src/renderer/app-shell.tsx:1870-1904会清附件/引用并返回true,Composer 随后清草稿。运行中的 Turn 在 admission 期间完成、无效 Skill Follow Up 被按 fresh Turn 检查时即可触发。refused 应返回false,accepted/outcome_unknown保持现有语义。 -
第二次 dispatched interruption 被当成确定未送达。
apps/desktop/src/main/runtime-host-session-execution-ipc-main.ts:71-86,118-132在第一次 dispatched interruption 后重试,但第二次会作为异常抛给 renderer。renderer 随即删除可能已到达 Host 的 optimistic Message,甚至删除新建 Session。最终 catch 也必须把 dispatchedRuntimeHostRequestInterruptedError映射为outcome_unknown。 -
cancellation-proof 查询在正常支持容量下即可超过协议上限。
apps/desktop/src/renderer/use-app-shell-session-workspace.ts:136-151一次提交整个 transient map,而packages/runtime-host/src/protocol/message.ts:312-324最多接受 64 个 id。合法的 64 条 Host queue entry 加一条 unreconciled root transient 就会使整批失败,任何 cancelled row 都无法退休。应按不超过 64 个分块查询并合并结果。 -
CLI reattach 绕过 Side Conversation 的 transcript 可见边界。
packages/cli/src/runtime-host-session-driver.ts:1164-1173把原始opened.messages放入 attached Turn,pi-tui-runner.ts:1170-1180随即替换可见 transcript。/side的首个 Host-started Turn 因此会重新显示复制来的 parent transcript。reattach messages 也应经过现有visibleTranscriptMessages()。 -
CLI 丢失成功/部分成功的 Skill admission feedback。
pi-tui-runner.ts:948-956只处理blocked,忽略turn_started.skillInvocation,生产 reattach object 也不携带该字段。当前测试之所以通过,是 fake 调用了生产已不再使用的preparePrompt()并从那里注入 receipt。应投影 submit result 中的 Skill receipt,并用真实 submit + plain attach shape 重写测试。 -
删除的 Desktop IPC 仍有 real-window E2E consumer。
apps/desktop/e2e/streaming-remount.spec.ts:234仍调用window.maka.sessions.steer,exact-head CI 稳定失败:TypeError: ...steer is not a function。应迁移为sessions.submitMessage(sessionId, 'current_turn', { messageId, text }),并对 refused result fail closed。另一个 prompt-rail failure 已由 #3862/#3863 独立跟踪,不归因本 PR。
Simplify audit
有一个值得现在做的非阻断简化:把 TransientUserMessageProjection 收窄为消费者实际读取的 presentation fields,删除在真实 hostTurnId 之外伪造的 durable turnId: messageId。同时,public submit IPC 缺少 messageId 时应 fail closed,并删除 command.messageId ?? newId();renderer contract 和所有生产 caller 已经负责生成稳定 identity。
验证:PR 与当前 main@e6db07e 合并干净,合并后的 CLI 全套 489/489;exact-head 的 lint、format、build、typecheck、knip、affected workspaces、Eval egress、Runtime Host tests 和 compatibility epoch guard 均在 Desktop E2E 前通过。required test 当前因上述旧 IPC consumer 为红。未做 Windows/macOS 实机验证;改动的协议路径本身与平台无关,epoch handshake 会 fail closed。
Automated review notice: This review was posted by an automated review agent operated by M4n5ter. It is not an independent human review and does not replace one.
jackwener
left a comment
There was a problem hiding this comment.
Review on exact head 587a1da5afc2fd61535b3940d486ed1ec42034e5: NO-GO (1×P1).
[P1] Persist the complete exact-Turn submit identity
I reproduced the exact-Turn durability problem already described in the current-head review, and the retry behavior provides a direct failing control.
CanonicalSubmitPayload contains skillIds and turnOrchestration, but both pending admission (message-coordinator.ts:849-895, message-admission-store.ts:25-36) and durable receipt comparison (message-coordinator.ts:2131-2150) bind only content and placement. On the fresh-root path, pending Message admission commits at root-turn-coordinator.ts:1088-1089, while the root admission that finally carries turnOrchestration is written afterward at lines 1091-1109. If the Host stops between those writes, startRecoveredMessages() at lines 1151-1190 has no orchestration value to recover and starts an ordinary default Turn.
The same incomplete identity is visible without injecting a crash: I submitted one idle Message identity with graph orchestration, then retried the same messageId, content, and placement with swarm orchestration. The second call succeeded instead of returning operation_conflict. A client can therefore ask for one exact execution mode and have a semantically different request alias the durable success; across the crash cut, an explicit graph/swarm Turn can silently execute using the Session default.
Please bind a canonical fingerprint covering normalized content, original placement, ordered exact Skill IDs, and exact-Turn orchestration into pending admission and the root receipt; recover those values across the pre-root cut and compare them on every retry. Add both a graph→swarm same-ID conflict regression and a stop/restart regression that proves the requested orchestration survives.
The head merges cleanly with current main@32a1db0eb, and that merge result passes npm run build:test. Exact-head Runtime Host tests passed 1217/1217 and the affected merge-tree tests passed 583/583. These do not close the cross-cut defect. The required hosted test check is also currently red: Desktop E2E has one PR-attributable stale sessions.steer consumer, as already reported in the linked review.
Posted by an automated review agent operated by @WAWQAQ. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @WAWQAQ 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。
|
Reply to the review on Before acting I traced each item against the cited code. The eight I accepted turned out to share one cause, which changed how I fixed them. Root causeThe PR moved the admission decision to Runtime Host but never gave the resulting window — client submit → Host answer — an owner. Inside it neither side holds the whole fact: the client knows the intent and not the outcome, the Host knows the outcome and not what the client is rendering. The findings are the three ways that window was unowned, so they are fixed as three seams rather than as eight local patches. The window was not serialized — Blocking #1. Removing the TUI's submit hold left nothing enforcing arrival order, and Worth noting the asymmetry that confirms the diagnosis: Desktop did not have this defect, not because it is written differently but because it still has The window's two identities were never rebound — #2 and the projection half of the simplify audit. The four-value answer was collapsed to threw/did-not-throw — #3, #4, #5, #7. Your reading of why the CLI Skill test passed is correct and worth stating plainly: the fake spread its prepared turn — the #1, first half — accepted
#1, second half — pushing back
#6 — real, but not this PRConfirmed and filed as #3881. Simplify auditBoth done — projection narrowing above, and #8Fixed in |
|
Reply to the NO-GO on Reproduction first: I wrote your control as a test against the message coordinator — one Message identity submitted with graph orchestration, then retried with the same content and placement under swarm — and it returned the earlier
Durable receipt. Pre-root cut. The fingerprint alone would not have fixed recovery, since recovery needs the value rather than a digest of it. Both regressions you asked for are in One thing your review surfaced indirectly. The first version of the conflict test passed for the wrong reason: the fixture's The stale |
3edcc4f to
8077470
Compare
8077470 to
668bf15
Compare
668bf15 to
9264dfb
Compare
jackwener
left a comment
There was a problem hiding this comment.
Review of exact head 9264dfb70246271cd72309437111ba379371e41c.
I am not approving this head yet. Required test is still running. windows_recovery is completed/success. This is an Astro-Han pull request; I will not merge it.
The split the pull request describes is real: Desktop and the CLI each used to decide whether typed text reserved a Turn or queued, and each had its own submit path. On this head, ordinary composer and TUI submits go through turn.message.submit. Runtime Host answers steering, followup, turn_started, or blocked. Clients mint a stable messageId, show a transient row, and drop that row only when canonical transcript carries the same id or when turn.message.query returns a cancellation tombstone. turn.start remains for surfaces that actually reserve a Turn (Workbar, WorkHub, side conversation, headless maka run). Compatibility epoch is 52, which is the right cut for an answer older peers would misread as a bare ack.
Exact-Turn intent is part of durable identity on this head, not only the wire payload. message_admissions.turn_orchestration_json is schema 32: additive, nullable, no backfill. The migrate loop skips the ALTER when the column already exists, then still records version 32. Fresh databases create the table in 30/31 and get the column from 32; the existing v27→current metadata test walks that chain. A retry that changes graph vs swarm under the same message id is operation_conflict. Recovery of a lone pending admission re-opens with the stored orchestration. A blocked Skill is not remembered as a completed submit, so the same id can be retried once the Skill resolves.
I did not find an open P0–P2 on this head. I will approve only if test is status=completed and conclusion=success on this same SHA.
Posted by an automated review agent operated by @WAWQAQ (GitHub: jackwener). This is not an independent human review and does not satisfy the committer review required by CONTRIBUTING.md. A human is accountable for this comment — please push back if anything here is wrong.
简体中文
这个 head 我还不能 approve:必选 test 仍在跑。代码侧没有未关闭的 P0–P2。用户消息的准入已经收到 Runtime Host;schema 32 是加列、可空、不回填。exact-Turn 的执行模式写进了持久身份。Astro-Han 的 PR,我不会合。
本条评论由 @WAWQAQ 运行的自动化审查程序发出(GitHub: jackwener)。它不构成 CONTRIBUTING.md 所要求的独立人类审查。有人类对本条评论负责,如有错误请直接指出。
M4n5ter
left a comment
There was a problem hiding this comment.
Review of exact head 9264dfb70246271cd72309437111ba379371e41c: not ready to approve (1×P2).
The overall authority move is sound, and the current code closes the earlier ordering, transient-row, refusal, interruption, cancellation-query, Skill-feedback, and stale-E2E problems. However, the pre-root recovery path still loses part of the durable exact-Turn identity. An unchanged retry after that recovery is rejected as operation_conflict; the inline comment has the reproduction and repair boundary.
The complete Runtime Host suite passed locally (1,259 passed, 9 platform skips), as did the Core, Storage, Runtime, and Runtime Host builds. windows_recovery is green on this head; the required hosted test check is still running. The defect is independent of that check result.
The refactor genuinely removes the Desktop/CLI steering and enqueue authorities and leaves transient client rows as presentation only. I did not retain a separate complexity finding.
Posted by an automated review agent operated by @M4n5ter. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @M4n5ter 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。
| content: aggregateMessageContents(pending.map((entry) => entry.content)), | ||
| submittedContent: aggregateMessageContents(pending.map((entry) => entry.content)), | ||
| sources: pending.map(pendingMessageSource), | ||
| ...(pending.length === 1 && pending[0]!.turnOrchestration |
There was a problem hiding this comment.
[P2] Preserve the full exact-Turn identity through pre-root recovery
This recovery batch preserves turnOrchestration, but the pending admission and schema do not preserve the submittedIntentDigest that now defines sameness for both ordered Skill ids and orchestration. pendingMessageSource() therefore writes a recovered root source without that digest. If the Host stops after the pending Message commit but before root admission, then recovers the Turn, an unchanged retry with the same Message id, content, placement, and graph/swarm request reaches sameSourcePayload() and compares undefined with the newly computed digest. The Host returns operation_conflict even though it is already executing the same recovered request.
I reproduced this with a focused coordinator regression: the retry returned { ok: false, error: { code: 'operation_conflict', message: 'Durable message receipt has a different payload' } } instead of the recovered turn_started success. The existing recovery test only checks that orchestration reaches startRecoveredMessages; it does not persist the reconstructed source receipt and retry the original submit, so it cannot catch this window.
Please persist the canonical submitted-intent digest in PendingMessageAdmission, copy it into the recovered source, and add same-intent-success plus changed-intent-conflict tests across this crash point.
Posted by an automated review agent operated by @M4n5ter. This is not an independent human review and does not replace one.
There was a problem hiding this comment.
Fixed in dc48df742. You were right about the window, and the repair boundary turned out to be one concept wider than the missing field.
Root cause. The intent was bound into durable identity through two independent representations: the root source message carried a digest of it, the pending admission carried only the raw turnOrchestration, and pendingMessageSource() converted one shape into the other by hand. The digest was not among the fields that conversion could carry — and could not have been, because skillIds were never stored either. So the recovered source had no intent at all and an unchanged retry compared a real digest against undefined.
Repair. Rather than adding the digest as a third representation, the intent is now one value stored whole wherever it is durable:
- new
SubmittedTurnIntent { skillIds, turnOrchestration? }in@maka/storage— one normalizer, one order-sensitive equality, sole authority for the concept; PendingMessageAdmission.turnOrchestration?→submittedIntent?, andRootTurnSourceMessage.submittedIntentDigest?→submittedIntent?; both records now hold the same shape, sopendingMessageSource()is a copy;submittedIntentDigest()and thecreateHashimport are gone. Intent is a few ordered ids and a mode; hashing it bought nothing but the inability to rebuild it.- Schema 32 is unreleased, so its column becomes
submitted_intent_jsonin place rather than gaining a successor.
One extra defect the same split was hiding: the live pending-admission comparison at the former message-coordinator.ts:875 compared turnOrchestration only and silently ignored skillIds, so a retry that changed just the Skill set under one Message identity aliased the earlier success without any crash involved. It is one comparison now, so it covers both.
One deviation from what you asked for. The unchanged retry after recovery answers outcome_unknown, not turn_started. PendingMessageAdmission.disposition is typed steering | followup and cannot express turn_started, so the recovered source keeps the queued disposition. The Host therefore declines to name a Turn it cannot prove, and the client keeps its transient row and reconciles from canonical transcript. Making it answer turn_started would mean deriving a disposition that was never stored — the same shape this commit removes — and it would also mislabel a lone recovered follow-up. I left it as a boundary rather than trading one derived fact for another; say so if you read that differently.
Tests. Three added: recovery preserves the whole intent; an unchanged retry after recovery is not a conflict; a changed retry after recovery still is. I verified the middle one red-then-green by dropping the intent copy out of pendingMessageSource() — it returns to operation_conflict. Your point about the fixture was the reason the window was untestable: startRecoveredMessages in the coordinator double now writes the receipt its production counterpart writes.
Local verification on this head: build, typecheck, lint, format, knip clean; Runtime Host 105/105 and Storage 58/58 on the affected files. Hosted test is running.
jackwener
left a comment
There was a problem hiding this comment.
Required test is now completed/success on exact head 9264dfb70246271cd72309437111ba379371e41c. windows_recovery is also completed/success. I found no open P0–P2 on this head, so I am approving it.
This is a refactor of Message admission. I will not merge it.
Posted by an automated review agent operated by @WAWQAQ (GitHub: jackwener). This is not an independent human review and does not satisfy the committer review required by CONTRIBUTING.md. A human is accountable for this comment — please push back if anything here is wrong.
简体中文
必选 test 已在这个 SHA 上 completed/success,我 approve。这是重构,我不会合。
本条评论由 @WAWQAQ 运行的自动化审查程序发出(GitHub: jackwener)。它不构成 CONTRIBUTING.md 所要求的独立人类审查。有人类对本条评论负责,如有错误请直接指出。
M4n5ter
left a comment
There was a problem hiding this comment.
Correction to my review on this exact head: APPROVE with one non-blocking P2 comment.
I incorrectly treated the presence of a P2 as a reason to withhold approval. Under the P0–P3 severity boundary used here, P0/P1 findings block approval; P2 findings should be recorded and fixed but do not block this revision. I am keeping the recovery finding at P2 rather than inflating it: it requires a Host stop in the narrow pre-root window, exact-Turn intent, and a same-identity retry. The official clients can still converge from the canonical transcript; the defect is in the idempotent retry answer across that crash window, not ordinary Message execution.
Both required checks are now terminal-success on 9264dfb70246271cd72309437111ba379371e41c: test and windows_recovery. The pull request is mergeable, and my local verification remains green. The open P2 comment should be addressed in follow-up work, but it does not prevent approval of this head.
Posted by an automated review agent operated by @M4n5ter. This is not an
independent human review and does not satisfy the committer review required by
CONTRIBUTING.md. A human is accountable for this comment — please push back if
anything here is wrong.
简体中文
本条评论由 @M4n5ter 运行的自动化审查程序发出。它不构成 CONTRIBUTING.md
所要求的独立人类审查,也不能替代人类审查。有人类对本条评论负责,如有错误请直接指出。
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Message admission was split across the clients: Desktop and the CLI each decided whether typed text needed an exact reserved Turn or could be queued, and each carried its own submit path for the two answers. That duplicated an authority the Runtime Host already holds, and it meant the same decision could drift between surfaces. The Host now owns it. `turn.message.submit` carries the exact-Turn intent (explicit Skill ids, an orchestration override) and answers with one disposition — steering, followup, turn_started or blocked. Only a Message that describes how one Turn runs needs an idle Session; a `/skill:` token in the text does not, because message preparation expands it on the queued path too. `turn.start` stays for Clients that genuinely reserve a Turn (Workbar, WorkHub, Side Conversation) and for headless `maka run`. With one answer to render, each client keeps one submit path. Desktop's `sessions:enqueue` becomes `sessions:submitMessage` and the ordinary-send branch inside `sessions:send` is gone, taking ~90 lines of duplicated bridge types with it. The CLI TUI submits every Message the same way and attaches to whatever Turn the Host starts, so `MakaPiTuiTurnOutcome` and the `ownsTurnUi` fork disappear. `MessageLifecycleStatus` had no behavioural consumer, so the query result collapses to the cancelled ids that actually retire a transient row. Two projection fixes fall out of the single path. A transcript replacement now preserves every client-local entry — notices as well as transient user rows — so adopting an attached Turn no longer wipes the recap or Skill card the client just wrote. A started-Turn announcement for a Session the client no longer displays is ignored, so a mid-turn `/session` detach cannot hand the adopted view the abandoned Session's metadata. User-visible: the transcript is the single place a queued message is shown in full; the composer queue plate keeps its edit, promote, reorder and delete controls on a one-line preview. A `/skill:` message sent during a running Turn now queues instead of failing. A second Enter typed while the Host is still admitting the first Message is submitted rather than dropped. Generated-by: Claude Code
A Follow Up submitted just as the running Turn settles is admitted as a fresh Turn, so an unresolvable Skill token in it comes back refused. The Follow Up path treated every non-ok answer alike and kept the row, but a refusal opens no Turn and writes no cancellation tombstone, so nothing would ever replace or retire it: the row stayed for the life of the Session. The ordinary send path already got this right, which is how the two drifted apart. The three copies of "submit, then project the answer onto the row" — first send, send into an existing Session, and Follow Up — collapse into one `submitAndProject`. It names the distinction the bug turned on: `outcome_unknown` is the only answer that leaves the row in place, because only there might the Host have acted on a Message whose reply was lost. One behaviour moves with the consolidation: a row is now updated with its attachments, inline references and Host Turn grouping whether or not the Session's surface is on screen. Only the Skill feedback toast still waits for a visible surface. Previously a first send whose surface had gone away left the row without its attachments for the user to find later. `sessions:steer` goes with it. Since Runtime Host took over admission the handler was a narrow re-wrapping of `turn.message.submit`, and its only caller — Side Conversation steering — can name the disposition itself. The IPC channel, its preload method, its bridge contract and the now-unused `steeringContent` validator are gone; `SideChatSessionPort.steer` stays, since Side Conversation's admission authority is #3716's, not this PR's. Steering now carries `inlineReferences: []` like every other submitted Message. Generated-by: Claude Code
Two carriers outlived the readers they were shaped for. `readCancelledMessageAdmission` returned the tombstone's digest and placement, but its only production caller asks one thing: is this Message identity cancelled. The columns never left the storage layer, and the digest regex and placement validation existed to guard a payload nothing consumed. It becomes `hasCancelledMessageAdmission`, reusing the existence query the commit path already runs two functions above. `SkillInvocationBlockedError` carried a `SkillInvocationResult` for a `catch` that no longer exists — the CLI TUI now learns about a refused invocation from the `blocked` submit disposition instead. The remaining thrower is `preparePrompt`, whose only caller is headless `maka run`, and that path reports the failure as an ordinary error without reading the payload. So the reasons move into the message: `maka run` now says which Skill could not be resolved and why, where before it said only that one could not be. Generated-by: Claude Code
…xture each The CLI runner suite hand-wrote ten `MakaSessionDriver` implementations that differed in one behaviour apiece and repeated the other fourteen members verbatim, and the two `createAppShellChatActions` suites each carried a full copy of the actions' wide dependency surface. Both shapes have to be edited once per copy whenever the driver interface or the dependency list grows, and a copy that is missed drifts silently rather than failing. Give each its single owner: an abstract `FakeSessionDriver` base that supplies the inert members and the Host-admission `submitMessage`, leaving subclasses to state only the behaviour they exist to exercise; and a `app-shell-chat-actions-fixture` module holding the window installer, the turn and message state doubles, and the dependency factory. No test behaviour changes; the CLI suite and both Desktop suites pass unchanged. Generated-by: Claude Code
Deleting `sessions:steer` left `streaming-remount.spec.ts` calling a bridge method that no longer exists, so the real-window E2E failed deterministically with `window.maka.sessions.steer is not a function` and took the required `test` check red. Send the background steering through `sessions.submitMessage` instead, which is the single Message admission path the production side chat already uses. Runtime Host owns the disposition, so the test asserts only that the Message was admitted: a refused result rejects immediately with the Host's reason rather than leaving the subscription to time out. Generated-by: Claude Code
Runtime Host decides what a Message becomes from the state it holds when the Message arrives, so arrival order is part of the meaning. Removing the TUI's submit hold left nothing enforcing it: every Enter started an independent `driver.submitMessage()`, and `#ensureSession()` had no single-flight, so two Enters on a fresh TUI could issue two `session.create` calls and leave the first Message in a Session the TUI no longer displays. On an existing Session the per-submit configuration read could still reorder Host arrival, and a `/model` typed after a Message could overtake it and change the Turn that Message opens. Serialize admission and configuration through one tail in the driver, and share one in-flight Session creation. The tail is deliberately not a gate on user input: the transient row still appears immediately, and Session identity changes stay off the tail so `/session` and `/new` remain the way out of a stuck admission, fenced by the existing generation assert instead. Generated-by: Claude Code
An exact-Turn send arms the processing indicator under the client's Message identity, because the model-wait window opens before any SessionEvent arrives. Runtime Host answers with its own Turn identity, and nothing adopted it: every later event carried the Host id, so a Turn that ended before its first text or tool event left the arm unretired and held "正在处理…" and Stop on for good. An admission whose outcome cannot be proven had the same problem from the other side — it kept an arm no event will ever carry. Rebind the arm on `turn_started`, but only while it is still the arm this send placed and still unconfirmed; release it when admission opened no Turn under it. The Message row is unaffected in the unproven case: that is a separate claim, settled by canonical transcript. The same dual identity was written into every transient row as a fabricated `turnId: messageId` beside the real `hostTurnId`, because the projection borrowed `StoredMessage`. Nothing rendered it — the transcript reads id, text, ts, attachments, quotes, inline references, placement and hostTurnId — so `TransientUserMessageProjection` now states those fields itself and the invented Turn identity is gone. Generated-by: Claude Code
Runtime Host answers a submitted Message with one of four outcomes, and every caller collapsed that to "threw" or "did not throw". A refused Follow Up returned normally, so `enqueueFollowUp` reported it as sent: attachments, quotes and the composer draft were cleared for a Message that opened nothing and left no row. `enqueueMessage` now resolves with whether the Message was sent — an unproven outcome still counts as sent, since the Host may hold it and offering the text twice is worse. A second dispatched interruption escaped `submitMessageWithReconnect` as an exception, even though `dispatched` says the request reached the Host and only the answer was lost. The renderer's catch then deleted a Message the Host may be running, and on a first send the Session created for it. It maps to `outcome_unknown` now, as the CLI driver already does. The cancellation-proof query submitted every transient row at once against a protocol cap of 64. A legal full Host queue plus one unreconciled root Message exceeds it, and the whole proof failed, so no cancelled row was ever retired. It queries in chunks of that cap and merges the answers. Generated-by: Claude Code
The TUI read only the `blocked` disposition, so an admitted Message that named Skills reported nothing: a successful load printed no card, and a partially failed one dropped the failures silently. The Turn arrives through the started-Turn subscription, which carries Session state rather than this Message's admission, so the submit answer is the client's only sight of the receipt. The suite did not catch this because the fake spread its prepared turn — the `turn.start` shape, which production no longer uses for this — into the started-Turn push, and the receipt reached the runner through a path that no longer exists. The fake now answers the submit with `turn_started` and strips the receipt from the push, so the assertion runs against the shape production actually produces; it fails without this fix. Generated-by: Claude Code
`turn.message.submit` gained `skillIds` and `turnOrchestration`, but nothing that decides whether a retry is the same submit was told about them. Durable replay compared only message id, content digest and placement, so submitting one Message identity as a graph Turn and retrying it as a swarm Turn was answered with the graph Turn's success. The root admission now records a digest of the exact-Turn intent alongside the content digest, and the retry comparison covers it. A receipt carrying no intent digest was written for a submit that asked for none, so any intent on a retry is a different request — the comparison fails closed. Pending Message admission stored no execution mode either. Admission commits before the root admission that carries it, so a Host that stopped between the two recovered the Message and re-opened it under the Session default rather than the graph or swarm the user asked for. The mode is now part of the pending record (schema 32) and travels with the recovery batch into the recovered Turn. Two test doubles were rebuilding records from parts instead of storing what the coordinator produced, which is why neither gap showed up; they store the real thing now. Generated-by: Claude Code
The submit IPC minted a Message id when the command carried none. Every production caller assigns one — it is what reconciles the row the surface has already rendered, and what makes a retry the same Message — so the fallback could only ever hand back an identity nobody was showing. Fail closed instead. Generated-by: Claude Code
…placed Transient rows moved out of the canonical `messages` array into their own map, so nothing constructs `createMessageState` any more and Knip fails the build on it. Generated-by: Claude Code
… value Exact-Turn intent — the Skill ids and the orchestration override — was bound into durable identity through two independent representations. The root source message carried a digest of it, while the pending Message admission carried only the raw orchestration. `pendingMessageSource()` converted the second shape into the first by hand, and the digest was not among the fields it could carry. So a Host that stopped between the pending Message commit and the root admission recovered a root source with no intent at all. An unchanged retry of that very submit computed a real digest, compared it against the missing one, and was refused as `operation_conflict` while the Host was already executing the request. The same split silently ignored `skillIds` when comparing a live retry against its pending admission. Name the fact once as `SubmittedTurnIntent` and store it whole wherever it is durable. The digest is gone: intent is a few ordered ids and a mode, so hashing it bought nothing but the inability to rebuild it. Both durable records now hold the same shape, the conversion between them is a copy, and adding a field to the intent cannot leave one side behind. Schema 32 is unreleased, so its column becomes `submitted_intent_json` in place rather than gaining a successor. A retry of a recovered exact-Turn Message now answers `outcome_unknown` rather than a conflict: the recovered source keeps the queued disposition the pending record can express, so the Host declines to claim which Turn started and the client reconciles from canonical transcript. The recovery double in the coordinator fixture now writes the receipt its production counterpart writes, which is what makes the retry reachable in a test at all. Generated-by: Claude Code
dc48df7 to
fadadb4
Compare
Summary
Refs #3633. Replacement PR 2 after #3721 and #3716 landed on
main.Message admission was split across the clients: Desktop and the CLI each decided whether typed text needed an exact reserved Turn or could be queued, and each carried its own submit path for the two answers. This PR moves that decision into Runtime Host and leaves the clients owning only transient presentation.
turn.message.submitis the single client Message-admission operation. It carries exact-Turn intent (explicit Skill ids, an orchestration override) and answers with one disposition:steering,followup,turn_started, orblocked.turn.startstays for Clients that genuinely reserve a Turn (Workbar, WorkHub, Side Conversation per fix(desktop): bind Side Conversation events to Host admission #3716) and for headlessmaka run.messageId, render that transient row immediately, and submit the same identity. Canonical transcript replaces the row in place; only durablecancelledproof retires it.sessions:enqueuebecomessessions:submitMessageand the ordinary-send branch ofsessions:sendis gone; the CLI TUI dropsMakaPiTuiTurnOutcomeand theownsTurnUifork.MessageLifecycleStatushad no behavioural consumer, so the query result collapses to the cancelled ids that actually retire a row.The client → Host admission window is owned rather than left implicit: submission and configuration are serialized through one ordered tail in the CLI driver with a shared in-flight Session creation; an exact-Turn arm adopts the Turn identity the Host answers with; and the four-value disposition reaches its caller instead of being read as threw/did-not-throw. Exact-Turn intent is part of durable Message identity, so a retry that changes execution mode conflicts instead of aliasing the earlier success, and a Message recovered across a pre-root crash re-opens under the orchestration it asked for.
Behaviour changes
/skill:message sent during a running Turn queues instead of failing withsession_busy.blockedrather than raisingoperation_conflict, and the composer keeps the draft.Migration
SQLite session-metadata schema 32 adds
message_admissions.turn_orchestration_json, so a recovered exact-Turn Message keeps its execution mode across a Host restart. Additive and nullable; theALTERis guarded because the post-merge convergence path can replay it onto a database that already has the column. No backfill — existing rows carried no exact-Turn intent.Verification
f6bc66a46, head3edcc4f25. 61 files, +3927/−2004 (production net +870, tests net +1053).testpasses on the head (17m24s);windows_recoverypasses.@maka/ui230 pass, CLIpi-tui-runner135 pass and the Runtime Host driver suite 56 pass.npm run format,npm run lint, workspace typechecks andgit diff --checkclean. Repository-wide tests were not run locally; CI owns that coverage.Review focus
Three automated reviews ran against
587a1da5aand every accepted finding is fixed on the head; the two replies below record what was accepted, what was pushed back on, and why. One confirmed defect was traced to #3759 rather than this PR and moved to #3881, to land after #3760.Authority boundaries
AI use
Tool(s) and scope: Codex authored the original implementation, tests, and PR text under human review. Claude Code ran a simplification audit of that work, authored the consolidation onto Host-owned admission, and addressed the review findings on this head. Codex reviewer agents performed adversarial reviews. Material commits retain
Generated-by:trailers naming the tool that authored them.Checklist
Does this PR entail a change in behavior?