fix(acp): emit plan session updates for TodoList calls on v2 engine - #2879
fix(acp): emit plan session updates for TodoList calls on v2 engine#2879strikegoose wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: c28868c The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c28868c3a4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| description, | ||
| display: { | ||
| kind: 'todo_list', | ||
| items: (args.todos ?? this.todo.getTodos()).map((todo) => ({ |
There was a problem hiding this comment.
Emit read-mode plans after execution to avoid stale ACP state
When a single assistant response batches TodoList({ todos: ... }) followed by TodoList({}), the executor prepares all calls and publishes each tool.call.started (and therefore each ACP plan) before executeBatch runs any task, while the default ToolAccesses.all() only serializes the later executions. This read-mode fallback snapshots the pre-write list, so the later read's plan update can overwrite the correct write plan with stale entries; the read tool result will see the updated list after the write executes, but ACP never gets a result-time plan refresh. Emit the query-mode plan from the executed/current list or suppress the started-time display for reads.
Useful? React with 👍 / 👎.
fix(acp): emit ACP
planupdates for TodoList calls on the v2 engineProblem
When running
kimi acp(the native ACP server on the agent-core-v2 engine, registered inapps/kimi-code/src/cli/sub/acp.ts), ACP clients never receive aplansession update, even though the agent actively maintains its todo list with the TodoList tool. Clients that render the ACPplan(e.g. Zed's plan view, Paseo's timeline todo card) stay empty for the whole session.Reproduction (any ACP client):
kimi acp.TodoListtool (any multi-step task).plansession updates on the wire.tool_call/tool_call_updatenotifications for the TodoList call arrive normally.Root cause
The ACP adapter's plan mapping is complete but unreachable:
packages/acp-adapter/src/events-map.ts:417(todoListToSessionUpdate) and:466(planFromDisplayBlock) translate atodo_listinput-display block into an ACPplannotification.packages/acp-adapter/src/session.ts:1146, gated onevent.displaybeing present on thetool.call.startedevent.On the v2 engine,
TodoListTool.resolveExecution()(packages/agent-core-v2/src/session/todo/tools/todo-list.ts) never attaches adisplayfield to the returned execution, soevent.displayis alwaysundefinedfor TodoList calls and the emission gate never passes — the plan mapping is effectively dead code. The v1 engine's TodoList tool (packages/agent-core/src/tools/builtin/state/todo-list.ts:108) does attachdisplay: { kind: 'todo_list', items: [...] }, which is why this worked before the v2 migration.Fix
Attach the
todo_listinput-display block in the v2 TodoList tool'sresolveExecution(), with the exact shapeplanFromDisplayBlockexpects ({ kind: 'todo_list', items: { title, status }[] },packages/agent-core-v2/src/tool/toolInputDisplay.ts:52):{ todos: [...] }) → items from the requested list.{}) → items from the current stored list (ISessionTodoService.getTodos()), matching the v1 behavior.{ todos: [] }) → empty items array.The display flows through the existing pipeline unchanged:
toolExecutorServicecopiesexecution.displayonto thetool.call.startedevent (packages/agent-core-v2/src/agent/toolExecutor/toolExecutorService.ts:754), and the ACP adapter emits theplanupdate from there.Why no
tool.resultre-emissionI evaluated also re-emitting the plan on
tool.resultto cover read/clear operations, and it is not necessary:tool.call.startedfires for all three TodoList modes (read / write / clear), and the display attached atresolveExecution()time already carries the correct items for each mode — reads included (current stored list).todoListToSessionUpdatedeliberately returnsnullfor empty plans (events-map.ts:427— "no useful client-side state ... deferred until kimi-code grows a clear-plan signal"). Re-emitting ontool.resultwould hit the same guard, so it changes nothing. Clearing the client-side plan is a separate, pre-existing gap that needs an explicit clear signal, not a second emission of the same payload.Tests
Extended the existing tool test file
packages/agent-core-v2/test/session/todo/tools/todo-list.test.ts(per repo convention, no new test file):resolveExecutionattaches atodo_listdisplay block with the requested items (write mode).The adapter-side mapping
display → planis already covered bypackages/acp-adapter/test/plan-and-commands.test.ts(unit + e2e, including the negative case), so this completes the chain: tool display →tool.call.started→ ACPplan.Verification run locally (Node 26.7.0, pnpm 10.33.0):
pnpm typecheck—@moonshot-ai/agent-core-v2clean,@moonshot-ai/acp-adapterclean.pnpm test—@moonshot-ai/agent-core-v2: 258 files / 3919 tests passed.@moonshot-ai/acp-adapter: 36 files / 326 tests passed.Notes
patchchangeset for@moonshot-ai/agent-core-v2and@moonshot-ai/kimi-code.