fix(runtime-host): carry the approved Plan steps and report their progress - #5336
shengfuandqin-stack wants to merge 8 commits into
Conversation
…n request The transition Turn that starts or resumes an approved Plan execution carried only `Execute the approved plan execution <id>.` after apache#4278 removed the per-Turn tail that used to include the approved steps and the update_plan instruction. Nothing replaced it on the Plan path, so the model never learns that an execution is in progress, what its step ids are, or that a progress tool exists: the execution stays active, the Desktop Plan panel stays at 0/N, and no plan_progress_updated event is ever written. Render the execution request — execution id, plan title, every step id with its title and status, and the instruction to keep progress current with update_plan (cancel_plan when the user abandons) — into that Turn's own durable user content, and keep the transcript on the one-line action through MessageContent.displayText. A resume therefore reports the progress reached before the interruption, and a replay reproduces the same content instead of re-deriving it from live state. Generated-by: DeepSeek Harness
…mits The Desktop reads the Plan execution projection once and refreshes it on `plan-mode:changed`, but only user-controlled Plan operations published that frame. A tool-driven `update_plan` therefore stayed invisible for the whole Turn: the panel kept `0/N` while the store advanced, and only caught up when the Turn ended (observed on the dev build: three progress updates about six seconds apart, all first shown at completion). Publish the `plan` Session-domain invalidation immediately after a committed `update_plan` / `cancel_plan`, alongside the existing HostPlanCoordinator wiring, and cover both the committed write and the rejected one. Generated-by: DeepSeek Harness
… follow-up The execution request asked for an update after each step and "once more before your final response". The call that marks the last step does not merely update: it closes the execution, and the store rejects any further update as a stale execution. A model following the wording literally could therefore produce one call too many. Say instead that the last call before the final response leaves every step completed or skipped so the execution closes, and that update_plan must not be called again after it has closed. Generated-by: DeepSeek Harness
The stub returned a `plan_progress_updated` event for `cancelExecution`, an event storeVersion ahead of its state, and steps that stayed pending after a reported update — enough to prove the callback ran, not enough to model the store contract the wiring sits on. Mirror the store: the event type follows the mutation, event and state agree on one storeVersion, and the state carries the steps the mutation asked for. Also name the invalidation test for what it covers, rather than claiming a running Turn. Generated-by: DeepSeek Harness
Writing progress now refreshes the Plan panel while the Turn runs, so two reads can overlap: a slower earlier response was free to put stale progress back on screen, and a response for the Session the user had just left could land on the new one. Both are reachable — the new tests fail without this guard. Track the newest read per hook instance and publish only it. No new state library: one ref beside the existing refresh callback. Generated-by: DeepSeek Harness
The read guard only dropped a superseded success. A superseded read that failed still raised the panel error, and a read started by the previous effect run survived a Session switch, a close and an unmount, so it could land on a panel that no longer owns it. Supersede those reads as well: a stale failure stays silent, the effect invalidates from its cleanup, and only the newest read reports. The newest read still surfaces its own failure. Four ordered-completion tests cover the cases; the stale-failure and closed-Session ones fail without this change. Generated-by: DeepSeek Harness
…utionId The stub returned a cancelled execution while still naming it as the Session's active execution, which no store does: cancellation clears the selection. Keep the fixture honest so a future assertion cannot pass against an impossible projection. Generated-by: DeepSeek Harness
…ak test The effect bumped the read sequence before subscribing, but React already runs the previous effect's cleanup first, and that cleanup is what supersedes the reads a Session switch, a close or an unmount leaves behind. Keep the cleanup increment and the comment that explains it; drop the duplicate. The removed test asserted that a read outstanding at unmount does not surface an error, which React satisfies on its own, so it constrained nothing. The closed-Session test exercises the same cleanup path and does fail without it. Generated-by: DeepSeek Harness
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for the careful write-up, and for opening the issue with the store evidence; that made the trace easy to follow. Reviewed at head d5466b1. I read this as a draft, so these are shape comments, not a line-by-line pass.
1. This is a Plan execution problem, not a Plan mode problem, and the PR should keep the two apart. In the runtime they are already distinct: plan mode exposes SubmitPlan and read tools only, and update_plan/cancel_plan are bound after approval in agent mode while an execution is active (selectCollaborationTools). #5335 is entirely on the execution side. The PR puts the execution request renderer into plan-mode.ts and describes everything as "Plan", which blurs that boundary. The transition Turn is owned by HostPlanCoordinator, so the request content belongs there; the plan-mode prompt stays planning-only. On the same note, the description says plan-mode.ts now lets the last update_plan close the execution, but that is what PlanStore.updateExecution already does on main (plan-store.ts:307); the diff has no such code, so please drop the claim.
2. Root cause and smallest shape. Three things decide whether #5335 is actually fixed, and the current diff covers one of them by prompt only:
- Tool visibility. The issue's trace is the strongest evidence: zero
update_plancalls with the tool deferred behindtool_search, becauseDIRECT_TOOL_NAMES(tool-availability.ts:39) holds exactly the twelve tools you counted. The PR asks the model to call a tool whose schema is not in its list. Addingupdate_planandcancel_planto that set is two lines, and binding is already scoped to an active execution, so nothing leaks into other Turns. With that in place the request text can shrink to the step block plus one sentence namingupdate_plan; the rest restates the tool description and a subagent rule the runtime already enforces. - Invalidation at the write authority. Publishing
planafter a committed write is right, but attaching it to two named tools leavesSubmitPlanandinterruptActivePlanExecutionunpublished (masked today by the coincident Turn terminal) and adds a second producer next toHostPlanCoordinator.onProjectionChanged. Thetododomain already has the shape for this: wrap the store writer once and publish after every successful mutation (session-todo-coordinator.ts:60). Doing the same foropenedPlanStoreinexecution-composition.tscovers every writer and letsonProjectionChangedbe deleted, so the PR becomes a net removal. - The Host has to close what the model leaves open.
interruptPlanAfterUnsuccessfulTurnhandles the failed/cancelled side only. When the root Turn endscompletedwith steps still pending, nothing closes the execution, the panel stays at0/N, and the next Turn keeps bindingupdate_plan. That is the second half of #5335's acceptance, and right now it rests on the model following a sentence. The Host should commit the terminal transition itself when a Turn completes with an execution active (complete if every step is terminal, otherwise interrupt with a reason).
The renderer supersession guard is a real defect on main (switching Session can paint the previous Session's plan) and follows the sequence-ref idiom the other hooks use, so keep it; it just needs far less commentary than it has.
3. Tests. Roughly 1,050 of the 1,173 added lines are tests, for about 120 lines of production change, and I think about 90% can go without losing coverage of a distinct obligation:
plan-execution-request-e2e.test.tsandplan-execution-request.test.tsassert the same six regexes against the same fixture; one short case through the real coordinator that the approval Turn carries the step ids is enough. We don't need an end-to-end kernel test for this.- The composer cases prove the wrapper calls
notifyafter a stub resolves. If the publish moves to the store writer per point 2, one case that a committedupdate_planon a real in-process store publishesplanreplaces them. - The panel file: the first case passes unchanged against
origin/main(the panel already subscribed toplan-mode:changed), and itssessionEventsassertion can never fail because the stub drops the handler. One supersession case that fails onmaincovers the guard; the other four are variations of it. - The runtime
plan-modecases test the renderer as a pure function; two lines of assertion on the rendered text is all that is needed once it moves into the coordinator.
Facts to sort before undraft: the branch is 10 commits behind main and conflicts with #5325 in interactive-run-composer.test.ts, so the "conflict-free merge" in the description predates that.
AI assistance: I used Claude Code to trace the tool-availability and invalidation paths and to run the panel-test ablation against main; the conclusions were checked by me.
中文版
感谢细致的说明,也感谢先在 issue 里给出存储层的证据,追踪起来很顺。评审基于 head d5466b1。这是 draft,所以下面只谈整体形状,不逐行。
1. 这是 Plan execution 的问题,不是 Plan mode 的问题,PR 应把两者分开。 运行时里两者本来就是分开的:plan 模式只暴露 SubmitPlan 和只读工具;update_plan/cancel_plan 是批准之后、在 agent 模式下、有活跃执行时才绑定(selectCollaborationTools)。#5335 完全在执行这一侧。PR 把执行请求的渲染器放进了 plan-mode.ts,通篇也都叫 "Plan",模糊了这条边界。transition Turn 归 HostPlanCoordinator 管,请求内容应该留在那里;plan-mode 的提示词只管规划阶段。另外,正文说 plan-mode.ts 现在让最后一次 update_plan 关闭执行,但那是 main 上 PlanStore.updateExecution 早有的行为(plan-store.ts:307),diff 里没有这段代码,请把这句去掉。
2. 根因与最小形状。 有三件事决定 #5335 是否真的修好,当前 diff 只靠提示词覆盖了其中一件:
- 工具可见性。 issue 里的 trace 是最强的证据:
update_plan被延迟到tool_search后面,调用次数为零,因为DIRECT_TOOL_NAMES(tool-availability.ts:39)恰好就是你数出的那 12 个工具。PR 让模型去调一个不在它工具列表里的工具。把update_plan和cancel_plan加进这个集合只要两行,绑定本来就限定在活跃执行内,不会泄漏到其他 Turn。有了这一步,请求文本可以缩到步骤列表加一句点名update_plan;其余内容都在复述工具描述和运行时已经结构性执行的 subagent 规则。 - 在写权威处发布失效通知。 写入提交后发布
plan是对的,但挂在两个具名工具上,SubmitPlan和interruptActivePlanExecution就不会发布(现在被同时发生的 Turn 终态掩盖),还在HostPlanCoordinator.onProjectionChanged旁边多了第二个发布者。todo域已经有现成形状:包装一次 store writer,每次成功写入后发布(session-todo-coordinator.ts:60)。对execution-composition.ts里的openedPlanStore照做,就能覆盖所有写入者,并删掉onProjectionChanged,PR 变成净删除。 - 模型没收尾的执行要由 Host 收尾。
interruptPlanAfterUnsuccessfulTurn只处理失败/取消侧。root Turn 以completed结束而步骤仍 pending 时,没有任何东西关闭执行,面板停在0/N,下一个 Turn 还会继续绑定update_plan。这是 #5335 验收的后半条,现在完全靠模型听一句提示词。Turn 完成且仍有活跃执行时,Host 应自己提交终态(步骤全部终结就 complete,否则带原因 interrupt)。
renderer 的过期读取守卫是 main 上的真缺陷(切换 Session 可能画出上一个 Session 的 plan),也沿用了其他 hook 的 sequence-ref 惯例,保留;只是注释可以少很多。
3. 测试。 新增 1,173 行里约 1,050 行是测试,对应约 120 行生产改动。我认为砍掉九成左右也不会失去任何一条独立义务的覆盖:
plan-execution-request-e2e.test.ts和plan-execution-request.test.ts用同一个夹具断言同样六条正则;经真实 coordinator 验证批准 Turn 携带步骤 id 的一个短用例就够了。这里不需要端到端的 kernel 测试。- composer 的用例证明的是 stub resolve 后包装器调用了
notify。如果按第 2 点把发布移到 store writer,一个"真实进程内 store 上提交update_plan后发布plan"的用例即可替代。 - 面板测试文件:第一个用例对
origin/main原样通过(面板早就订阅了plan-mode:changed),而且它的sessionEvents断言永远不会失败,因为 stub 把 handler 丢掉了。一个在main上失败的 supersession 用例就覆盖了守卫,另外四个是它的变体。 - runtime
plan-mode用例把渲染器当纯函数测;渲染器移进 coordinator 之后,对渲染文本的两行断言就够了。
undraft 前要处理的事实: 分支落后 main 10 个 commit,与 #5325 在 interactive-run-composer.test.ts 冲突,正文里"合并无冲突"是在那之前的结果。
AI 辅助:我用 Claude Code 追踪了 tool-availability 和失效通知的路径,并对 main 做了面板测试的消融;结论由我核对。
Summary
An approved Plan could remain at
0/Nin the Desktop panel even after the agent finished its work. This change addresses two missing links and protects the resulting asynchronous UI refreshes:Execute the approved plan execution <id>.. The per-Turn execution context had been removed in perf(runtime): remove generic turn tail injection #4278, whileupdate_planremained a deferred tool. The durable transition request now carries the approved step IDs, titles and statuses, plus instructions to maintain progress and close the execution with the final update. Resume uses committed progress.displayTextpreserves the existing transcript text. No volatile per-Turn prompt tail or tool-visibility change is introduced.update_planandcancel_plancalls now enqueue the existingplanSession-domain invalidation. Previously, that notification came from user-controlled Plan operations, not execution tools. The panel also refreshes on terminal Session events, which explains why a completed execution could disappear at the end without showing intermediate progress.Fixes #5335
Verification
Manual observations and stored events
Validation was staged on Windows using isolated dev profiles, with an additional observation from the packaged nightly. These were not all identical A/B tasks: the baseline and initial fix used small file tasks; run C was a longer six-step repository inspection. The table separates persisted state from screen observations.
update_plancalls0/9after the work was reported finished0/3Run C's stored completed-step counts were
0 → 1 → 3 → 5 → 6. Progress writes occurred at 01:40:00, 01:40:49, 01:42:04 and 01:44:37 on 2026-09-15 (UTC+08:00); completion was recorded at 01:45:54. These timestamps prove persistence, not the exact time each value was painted on screen. The model sometimes reported multiple finished steps together. The patch publishes invalidation after successful tool writes; it does not force one model call per step or manufacture intermediate counts.Automated checks
Focused suites passed serially on Windows 11, Node 24.14.0:
plan-modeinteractive-run-composerplan-execution-requestplan-execution-request-e2eplan-protocolplan-two-client-udsplan-mode-panel-refreshplan-mode-panel-pendingThe request tests cover approval, resume and repeated request content. The Host request end-to-end test verifies that the production composition delivers the full request to the backend; it does not test tool execution through to a client notification. Composer tests cover successful notification and rejected writes. The panel tests render
0/3 → 1/3from a Plan-change callback without a terminal Session event, and cover obsolete successes, obsolete failures, Session changes/close, and current-read errors.During development, removing the coordinator change made the request tests fail; removing the tool notification wrapper made the notification test fail; removing the read protections exposed stale-success, stale-failure and Session-close failures. These negative controls were performed at different development revisions, so their historical suite totals are not presented as totals for the final six-test panel suite.
The branch was merged with main at
9ce2032cin a temporary worktree (merge08fe9fd1), without rebasing the submitted branch. The merge was conflict-free. Recorded verification on that tree includes a successful rootnpm run buildand the focused suites above. The temporary worktree was subsequently removed.Local checks recorded during preparation also passed: lint, format checking, runtime/runtime-host/desktop typechecks, and the desktop/UI knip checks. These are local results, not a claim that remote CI has run or that the entire workspace test suite is green. A review found stale local compiled output after a negative control; rebuilding restored passing results. Positive validation must use rebuilt output matching the restored source.
Remaining test limitations
AI use
DeepSeek Harness implemented the changes and tests and assisted with local verification. Astra assisted with code review, evidence checking and publication-text editing. The contributor manually exercised the Desktop behavior. The eight commits retain their
Generated-by: DeepSeek Harnesstrailers.Checklist
Does this PR entail a change in behavior?
中文说明
概述
批准计划后,即使 Maka 已经完成任务,桌面端计划面板仍可能停在
0/N。本次修改补上两个缺失环节,并保护由此产生的异步界面刷新:Execute the approved plan execution <id>.。perf(runtime): remove generic turn tail injection #4278 移除了每轮注入的执行上下文,而update_plan仍是延迟工具。现在持久化的执行请求会携带已批准的步骤编号、标题和状态,以及维护进度、并在最后一次更新时关闭执行的指引;恢复计划时使用已提交的进度。displayText保留原有对话文本。没有重新引入每轮动态提示词,也没有改变工具可见性策略。update_plan和cancel_plan调用现在会发布已有的plan域变更通知。此前该通知来自用户操作路径,而非执行工具。界面也会在会话结束等事件时刷新,这解释了为什么执行完成时面板可能直接消失,而没有显示中间进度。验证
人工观察与存储事件
验证在 Windows 上使用隔离的开发配置分阶段进行,另有一次打包 nightly 版本的观察。这些并非全部为相同的 A/B 任务:基线和第一层修复使用小型文件任务,C 轮是较长的六步仓库检查任务。下表把持久化状态与屏幕观察分开陈述。
update_plan调用0/90/3C 轮记录的已完成步骤数为
0 → 1 → 3 → 5 → 6。进度写入时间为 2026-09-15(UTC+08:00)01:40:00、01:40:49、01:42:04、01:44:37,完成时间为 01:45:54。这些时间戳证明数据已持久化,不代表屏幕显示这些数值的准确时刻。模型有时会一起上报多个已完成步骤。本补丁在工具成功写入后发布失效通知,不强制每步一次调用,也不人为制造中间数值。自动测试
在 Windows 11、Node 24.14.0 上串行运行,聚焦测试通过:
plan-modeinteractive-run-composerplan-execution-requestplan-execution-request-e2eplan-protocolplan-two-client-udsplan-mode-panel-refreshplan-mode-panel-pending请求相关测试覆盖批准、恢复和重复请求内容一致。Host 请求端到端测试验证生产组合会把完整请求送达后端,但不覆盖工具执行一直到客户端通知这一段。composer 测试覆盖成功通知与写入被拒两种情况。面板测试验证仅凭计划变更回调即可从
0/3渲染到1/3,且不依赖会话结束事件;同时覆盖过期成功、过期失败、会话切换/关闭以及当前请求的错误显示。开发过程中,去掉协调器改动会使请求测试失败;去掉工具通知包装会使通知测试失败;去掉读取保护会暴露过期成功、过期失败和会话关闭三类失败。这些反向验证在不同开发版本上进行,因此不把当时的历史套件总数当作最终六条面板测试的总数。
修复分支在临时 worktree 中与 main(
9ce2032c)合并,合并提交为08fe9fd1,未对提交分支做 rebase,合并无冲突。该合并树上有成功的根npm run build记录和上述聚焦测试记录。临时 worktree 随后已删除。准备期间记录的本地检查也通过:lint、格式检查、runtime/runtime-host/desktop 类型检查,以及 desktop/UI 的 knip 检查。这些是本地结果,不代表远程 CI 已经运行,也不代表整个工作区测试套件全绿。审查过程中曾发现反向验证后残留的本地编译产物与源码不一致;重新构建后恢复通过。正向验证必须使用与恢复后源码一致的新构建产物。
仍然存在的测试限制
AI 辅助说明
DeepSeek Harness 实现代码与测试并协助本地验证;Astra 协助代码审查、证据核对与发布文本整理;贡献者本人进行了桌面端实际操作验证。八个提交保留
Generated-by: DeepSeek Harnesstrailer。检查项
本次 PR 是否包含行为变更?