refactor(cli): simplify live tool transcript reconciliation - #3569
Conversation
36cbeb9 to
2fd085f
Compare
c948b2e to
53fab5c
Compare
M4n5ter
left a comment
There was a problem hiding this comment.
Independent review of exact head 53fab5c4620107a6bc5f0dec9e32567b4e757d28: APPROVE — no P0–P3 findings.
The refactor removes real duplicate state rather than moving it: the hidden shell-poll map, Runtime materializer/export, Channel snapshot mirror, duplicate settlement callbacks, and unreachable pending tool state are gone. Terminal, live, and reconnect replacements now converge on one settlement path and one monotonic transcript sequence fence.
I specifically re-reviewed the conflict resolution against #3568 after two rebases. The current tree preserves both contracts: terminal/live/reconnect all advance the shared #transcriptRefreshSequence; reconnect still fences by session identity and generation before publishing; external new Turns reattach without replaying stale transcript state. The final c948b2ec..53fab5c4 rebase has a zero tree diff.
Verification: full CLI suite 401/401 passed locally; exact-head test and audit are both completed / success; current-main merge tree and git diff --check are clean. All three review endpoints were empty at publication time.
中文
独立审查绑定精确 head 53fab5c4620107a6bc5f0dec9e32567b4e757d28:APPROVE,无 P0–P3。
这次删掉的是真正重复的状态/权威,不是搬目录:隐藏 shell-poll Map、Runtime materializer/export、Channel snapshot 镜像、重复 settlement callbacks、不可达的 pending tool state 都已消失;terminal/live/reconnect 三类替换统一到一条 settlement 路径和同一个单调 transcript sequence fence。
两次 rebase 后我专门复核了与 #3568 的冲突收口:三类来源都推进共享 #transcriptRefreshSequence;reconnect 发布前仍校验 session identity + generation;外部新 Turn 能重新挂接且不会回放陈旧 transcript。最后一次 rebase 的 tree diff 为 0。
验证:CLI 全套 401/401;exact-head test、audit 双绿;当前 main merge-tree 与 git diff --check 干净;发布时三类 review 端点均为空。
jackwener
left a comment
There was a problem hiding this comment.
代表 @WAWQAQ 执行独立审查发布,授权范围:对 apache/maka PR #3569 提交技术 review;绑定 exact head 53fab5c4620107a6bc5f0dec9e32567b4e757d28。
结论:1×P2,暂不 APPROVE。
[P2] packages/cli/src/pi-transcript.ts:352-388 — settlement reconciliation 会把另一个仍在运行的内部 shell poll 取消隐藏。
正常并发路径可以触发:两个 Bash 后台任务 A/B 各自存在隐藏的内部 Read poll;A 先完成并触发 reconcileToolsWithStoredMessages(),此时 B 的 durable transcript 仍只有 tool call、没有 result,也不携带 live-only 的 hidden presentation 状态。匹配 B 后,L387 无条件执行 entry.hidden = durable.hidden,将正在运行的 B poll 从 hidden=true 改为可见。
这违反 tool_start 在 L551-L575 声明的契约:属于可见 Bash parent 的 Read / StopBackgroundTask 内部 poll 在运行期间不应产生独立 row。若该 entry 已在 viewportTop 之上,新增行还会进入本模块专门规避的 full redraw / terminal scrollback 清空路径;若随后 abort/error,dropHiddenTools() 也无法再识别并清理这个 poll。
现有守卫和测试漏掉的原因:reconcile 按 toolUseId 找到了 durable entry,所以不会走 !durable 分支;测试没有覆盖“两组 Bash parent + 两个 hidden poll,其中一个 settle/reconcile、另一个仍 running”的组合时序。
最小修法:匹配到的 durable poll 尚无 result/error 时保留 live hidden=true;只有 durable terminal result/error 必须独立显示时才 reveal。补两条 production-seam 回归:A settle/reconcile 时 B 仍 hidden;reconcile-before-abort 后 B 仍可被 interruption 清理。不要恢复已删除的 pendingShellRunPolls——单一 durable authority 的降熵方向是正确的,缺口只在 durable state 与 live-only presentation state 的边界。
验证:reviewer 在读取任何既有 discussion 前封存 provisional,并在 exact head 上复现 readBHidden=false / readBStatus=running;focused CLI/Runtime Host 150/150、Desktop/UI emitted suites 91/91;exact-head test 与 audit 均 completed/success。发布前 head 未漂移、PR OPEN/non-draft;GitHub mergeability 正在重新计算,不影响本 COMMENT finding。新 head 修复后请按 exact head 增量复核。
此 Agent review 不声称满足仓库仍待统一的 ASF required-human-review 口径。
Approval withdrawn: current head has a reproducible P2 where durable reconciliation clears the hidden flag on a still-running internal shell poll. 撤回批准:当前 head 的 durable reconcile 会把仍在运行的内部 shell poll 从 hidden 翻为 visible,存在可复现 P2。
53fab5c to
a33001d
Compare
|
Fixed in |
jackwener
left a comment
There was a problem hiding this comment.
代表 @WAWQAQ,授权范围:Maka PR review / GitHub review action;exact head a33001db07fd24e8914161aa009cd4767796fbf2。
增量复核结论:旧 P2 已关闭;当前 head 仍有 1 条新的 P2,因此暂不 APPROVE。
[P2] packages/cli/src/pi-transcript.ts:381-391 — durable failed poll can be revealed in its old off-screen position instead of at the transcript tail.
Reachability: ② 合理失败/慢消费者并发。一个 background Bash 的 hidden Read/StopBackgroundTask poll 已被后续输出推到 viewport 之前;poll 随后失败,而 TUI consumer 比 durable refresh 慢。
Mechanism: runtime-host-session-channel.ts:628-630 invokes onTranscriptSettlement() before it pushes the live tool_result. If the durable refresh wins, reconciliation copies the durable error/status and clears hidden in place. When the live event later reaches pi-transcript.ts:613-615, the entry is already visible, so revealToolAtTail() is skipped.
Outcome: the next render changes content before viewportTop; pi-tui takes the full-redraw path this module explicitly avoids, clearing terminal scrollback and resetting the user's position. The production-function probe produced an error card before the viewport with a changed prefix. A related terminal boundary also remains: a completed turn can materialize a resultless poll as status: 'aborted', but durable.result === undefined leaves it hidden and complete does not remove it.
Why tests miss it: the existing suite covers live-event tail reveal, and the two new tests cover only a durable call that is still running. It does not combine off-screen geometry with a durable terminal error, nor a terminal resultless poll.
Minimal/clean fix: preserve live hidden only while durable.status === 'running'. When durable state is terminal and the live entry is still hidden, reuse the live tail-reveal/reorder contract, or explicitly remove interrupted internal polls if that is the chosen terminal contract. Do not restore the deleted poll side map. Add production-seam tests for (1) durable error reconciliation before the live event while off-screen, asserting the viewport prefix stays byte-identical and the error card moves to the tail, and (2) a terminal resultless poll leaving no hidden stale tool after complete.
The old A-settled/B-running regression is genuinely closed: the new tests are mutation-sensitive and preserve B as hidden/running through reconcile and later abort cleanup. The single durable authority remains the correct simplification.
Fresh gate: OPEN, non-draft, MERGEABLE/BLOCKED; exact-head test and audit are completed/success. No merge action taken.
a33001d to
e41df32
Compare
jackwener
left a comment
There was a problem hiding this comment.
代表 @WAWQAQ 执行 Maka PR review / GitHub review action;绑定 exact head e41df3252e1b99158bcb57b47b63513c08e9503b。
结论:旧 scrollback P2 与 resultless-terminal 边界均已关闭;当前 head 仍有 1×P2,暂不 APPROVE。
[P2] packages/cli/src/pi-transcript.ts:565-602 — omitted live result can mark a still-running background Bash card done after durable hydration.
Runtime Host deliberately projects every live tool_result as contentOmitted:true plus empty text (packages/runtime-host/src/adapter/session-projector.ts:524-533). The Channel starts transcript settlement before queueing that event (runtime-host-session-channel.ts:628-630). If the durable refresh completes before the TUI drains the queued event, hydrateToolsWithStoredMessages() first installs the real {kind:'shell_run', status:'running'} and marks the Bash card running. The later placeholder has no shellRun, so L596 overwrites presentation status to done, while L598-600 correctly preserves the durable running result because content is omitted.
I reproduced the exact production-function ordering: before the placeholder the card was status=running, result.status=running, elapsed ticker active; after it the same card was status=done, result.status=running, elapsed ticker inactive. The row becomes a green completed tool, its elapsed time freezes, and the expanded “Ask Maka to stop this task” affordance disappears even though the background process is still running. It can persist until a later resource update; a quiet background process may not update again before it settles.
Existing guards miss the split: they cover hydration itself, omitted poll correlation, and preserving hydrated text/subagent content, but not “running Bash shell_run hydrated before omitted live result”. Minimal fix: when contentOmitted preserves a structured result, derive/preserve presentation status from that result (while retaining isError as the call-level override) instead of deriving status from the placeholder text. Add the exact hydrate-before-event regression and assert both tool.status === 'running' and elapsed ticker continuity.
Other reconciliation results: suppressed is now live-only presentation authority; hydration no longer rebuilds/reorders entries or changes suppression, and error/abort/complete all drop suppressed polls. The 298-line Runtime materializer deletion is a real simplification: CLI was its only production consumer, and the direct conversion preserves user origins/display text, thinking order, shared tool-result status derivation, system notes, and token aggregation without creating a second durable authority.
Verification: full maka-agent suite 410/410; focused production-function probe above; exact-head hosted test and audit both completed/success; current head OPEN/non-draft/MERGEABLE. Three GitHub discussion endpoints were read separately; only old-head reviews and the old-head author reply exist, with no current-head finding that changes this result. No merge action taken.
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
d396067 to
412e1ff
Compare
There was a problem hiding this comment.
代表 @WAWQAQ 执行 Maka PR review / GitHub review action;授权范围:技术审查与 APPROVE;绑定 exact head fc30892351f9b5b96dea53b6d598a183ed98f96d。
结论:APPROVE。当前 head 未发现 P0–P3;旧 hydrate-before-omitted-event P2 已关闭。
覆盖与排除:复核了 CLI/UI 的 invocation status 与 ShellRun resource status 分权、isError 优先级、WriteStdin/subagent 语义、revision 与 ownership 配对、duration/ticker/stop hint/render signature,以及相对旧 head 的 range-diff。真实 durable running ShellRun → contentOmitted live settlement 在两端均保持 presentation running;删除各端派生逻辑时,定向生产链分别以 done !== running 和 completed !== running 变红。CLI 416/416、UI 222/222。
Fresh gate:PR OPEN、非 draft、head 未漂移;exact-head test、audit 均 completed/success;GitHub MERGEABLE。main 刚推进到 42cc5108c21e8ae44afae765f3367467e0b6e031,对该 live main 的本地 merge-tree 也无冲突。reviews / inline comments / issue comments 三端点已分开核对,无 current-head P0–P2 或 live hold。
无 merge 动作。本 Agent APPROVE 不声称满足仓库仍待统一的 ASF required-human-review 口径。
Summary
Follow up on #3524 by reducing live tool-result rendering to one presentation authority:
tool_start/tool_resultevents alone own row visibility, ordering, and terminal cleanupcontentOmittedinstead of guessing from an empty-text placeholdershellRunReffor internal Read/Stop polling, without exposing full tool argumentsThe branch changes 953 lines and deletes 1,114, for a net reduction of 161 lines. Production code alone changes 262 lines and deletes 639, net -377. More importantly, it removes the durable presentation state machine, off-screen tombstones, pending-poll side state, Runtime materializer view model, Channel snapshot mirror, duplicate settlement callbacks, and the
pendingtool lifecycle state.The user-visible behavior remains the #3524 outcome: a tool card settles as soon as its result lands, without waiting for the turn to end. Failed internal polls move to the live tail; successful or resultless internal polls leave no row or tombstone. The Runtime Host compatibility epoch is 43 because the exact wire schema now carries bounded poll correlation.
Deletion probes
Accepted probes removed:
pendingShellRunPollsand its dedicated result branch@maka/runtime/materializerexport, implementation, and testspendingtool-activity statusTwo larger deletions failed the net-simplification probes:
Both probes keep the existing boundary only where deleting it would create more state elsewhere.
Verification
Before the final one-commit rebase onto current
main:npm --workspace @maka/core test(638 passed)npm --workspace @maka/runtime-host test(1,113 passed)npm --workspace @maka/ui test(220 passed)npm --workspace maka-agent test(410 passed)On exact head after rebase:
npm --workspace maka-agent test(410 passed)npm --workspace @maka/ui test(220 passed)npm run typechecknpm run lintnpm run format:checknpm run check:asf-headersgit diff --checkAI use
Select exactly one:
Tool(s) and scope: Codex implemented the simplification, wrote deletion and race probes, and coordinated independent explorer audits. Material commits include
Generated-by: Codextrailers.Checklist
Does this PR entail a change in behavior?