feat: use acp for desktop chat prompt (feature toggle off) - #9802
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c4d90ac1ff
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1 @@ | |||
| export const USE_ACP_CHAT = true; | |||
There was a problem hiding this comment.
Keep ACP chat disabled by default
useChatSession selects the implementation from this constant at module load, and BaseChat now calls useChatSession, so true sends every desktop chat through useAcpChatSession/acpPromptSession instead of the existing useChatStream path. If this rollout is meant to remain off by default, this enables the experimental ACP prompt flow for all users rather than preserving the fallback.
Useful? React with 👍 / 👎.
| default: | ||
| return []; |
There was a problem hiding this comment.
Handle ACP elicitation updates
When an MCP tool requests elicitation, the ACP server emits a goose custom interaction_update notification for the pending form, but this switch falls through to default and returns no state changes. In ACP chat sessions that means no actionRequired elicitation message is created for GooseMessage/ElicitationRequest, so the user has no way to submit the response and the prompt remains blocked.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We are going to apply elicitation with ACP after the acp server side elicitation pr is merged. At the moment there is no production issue as this is still behind feature toggle
| if (update.status !== 'completed' && update.status !== 'failed') { | ||
| return []; |
There was a problem hiding this comment.
Merge non-terminal tool call updates
The ACP server sends ToolCallUpdate notifications without a completed/failed status for live tool title and chain-summary updates, but this guard drops every non-terminal update before merging metadata into the existing tool request. During ACP chat runs the tool cards therefore keep their fallback labels/metadata until a reload replays persisted state, instead of updating live.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The tool update notification without Completed or Failed status are only:
- client side fs tooling
- title-only and toolChainSummary metadata updates. (We don't subscribe them)
We are not using them notifications in desktop. So it is ok.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdb817194a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| let run_id = format!("run_{}", Uuid::new_v4()); | ||
| let cancel_token = CancellationToken::new(); | ||
| let agent = self.get_session_agent(&session_id, None).await?; |
There was a problem hiding this comment.
Register prompt cancellation before agent activation
In the desktop ACP path, sessions are loaded via the existing resumeAgent flow and the first client.prompt can reach this lazy get_session_agent activation before start_active_run stores the cancel_token. If the user presses Stop while that activation is still loading the agent/extensions, on_cancel has no token/session run to cancel, so the prompt continues as soon as activation finishes. Register the run or otherwise store the cancellation token before the awaitable activation work.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is planned to handle in a separate PR
| toolResult: | ||
| update.status === 'failed' | ||
| ? { status: 'error', error: toolError(update) } |
There was a problem hiding this comment.
Preserve failed tool output text
For MCP tools that return isError with text content but no structured output, the ACP server marks the update as failed and carries the diagnostic text in update.content; this branch maps failures through toolError, which ignores update.content, so the tool card only gets the title/fallback instead of the actual error message. Include the text content when building the legacy error result so users can see why the tool failed.
Useful? React with 👍 / 👎.
* main: feat(session): add opt-in ACP last message snippets (#9798) fix: correctly map ollama_cloud to canonical provider and update max_… (#9639) fix incorrect max tokens values for groq provider (#9790) Mark stream decode errors retryable (#9723) Validate desktop i18n catalogs (#9776) fix: classify Bedrock ValidationException as ExecutionError (#9735) i18n: add Japanese locale support (#9768) docs: update docs for ACP clients (#9772) feat(security): unified OTLP logging schema for cross-tool detection (#9713) fix: page through all Databricks AI Gateway v2 endpoints when listing models (#9753) Add canonical thinking modes (#9743) fix: preserve unparseable extension entries during config refresh (#9439) feat(lang): add Hindi Desktop locale (#9733) docs: fix stale session navigation/delete docs (Session History) (#9727) feat: custom acp method to get session info (#9729) feat(acp): support GOOSE_SERVER__SECRET_KEY at goose serve acp endpoint (#9726) feat: surface Anthropic stream refusals as visible errors (#9724)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 678dbd5851
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| activeRequestSessionIdRef.current = targetSessionId; | ||
|
|
||
| try { | ||
| await acpPromptSession(targetSessionId, userMessage); |
There was a problem hiding this comment.
Propagate history replacements from ACP prompts
When ACP chat is enabled and a prompt produces AgentEvent::HistoryReplaced (for example /clear, /compact, or auto-compaction), the legacy /reply path translates that event into UpdateConversation, but the ACP prompt loop in crates/goose/src/acp/server.rs drops non-Message events. Because this new submit path only waits for client.prompt and never reloads or applies the replacement, the desktop keeps showing stale pre-clear/pre-compaction messages until the user manually reloads the session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this is correct call. I will address this in a separate pr as it requires server side change
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4c0d1117a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!incoming || incoming === existing || existing.endsWith(incoming)) { | ||
| return existing; |
There was a problem hiding this comment.
Append repeated ACP text deltas
When ACP chat is enabled and live text arrives as streaming deltas, this treats any incoming chunk that equals the current text or matches its suffix as a duplicate. The legacy stream path appends same-id text chunks, so ordinary outputs with repeated adjacent tokens/chars (for example Hel followed by l, or ha followed by ha) will drop the second chunk and show corrupted assistant text until the conversation is reloaded.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b1269eb. The ACP server emits incremental text deltas the same way the legacy stream does (each AgentEvent::Message text chunk for a given message id is appended, not cumulative), so the mergeTextChunk dedup heuristics were wrong. Replaced with a plain += append to match useChatStream.pushMessage, and added a regression test for the Hel+l case.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26bb389c84
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (USE_ACP_CHAT) { | ||
| if (!resolveAcpPermissionRequest(sessionId, id, action)) { | ||
| console.error('No pending ACP permission request found', { sessionId, id }); | ||
| return; |
There was a problem hiding this comment.
Fall back for non-ACP approvals
When ACP chat is enabled, this component can still be rendering a legacy REST confirmation: useAcpChatSession starts edit-in-place reruns through submitToSession(...), so any tool confirmation from that rerun is not present in the ACP pendingRequests map. This branch returns before calling confirmToolAction, leaving the edited run stuck whenever it asks for tool approval; route by the request source or fall back to the REST confirmation when no ACP request is pending.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b1269eb. handleAction now falls back to the REST confirmToolAction when resolveAcpPermissionRequest finds no pending ACP request, so edit-in-place reruns that go through submitToSession are no longer left stuck on tool approval. Updated the test to assert the fallback.
- append repeated adjacent ACP text deltas instead of dropping suffix/ duplicate chunks, matching the legacy useChatStream behavior - fall back to REST confirmToolAction when no ACP permission request is pending so edit-in-place reruns are not left stuck on tool approval Signed-off-by: Douwe M Osinga <douwe@sidewalklabs.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1269ebefb
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| activeRequestSessionIdRef.current = targetSessionId; | ||
|
|
||
| try { | ||
| await acpPromptSession(targetSessionId, userMessage); |
There was a problem hiding this comment.
Reattach ACP prompts after remount
When ACP chat is enabled and the user switches away from this chat while acpPromptSession is still running, the session-update subscriptions owned by this hook are removed, so live assistant/tool notifications are dropped. Unlike submitToSession, this path does not register a durable request id or reattach to the ACP active run, and the load effect later returns cached messages instead of reloading the session, so returning to the session can show only the stale optimistic prompt until a hard reload. Please keep/replay the active ACP run or bypass the cache/reload while an ACP prompt may still be active.
Useful? React with 👍 / 👎.
* main: (89 commits) refactor: make session.name the source of truth for displayed session titles (#9841) fix: remove hard-coded React alias that points to the wrong location and break the dev render (#9842) fix: pass thinking config to Bedrock Anthropic models (#9794) Split provider inventory out of providers (#9826) Support MCP extensions in open plugins (#9471) Add self-improving agents blog post (#9846) perf(build): drop debug info for dependencies in the dev profile (#9843) check for responses API support in databricks (#9347) Initial prompt with goose://new-session (#9427) fix(local-inference): use media marker parts for vision prompts (#9452) feat(gym): make Open Model Gym output dir configurable (#9789) fix(desktop): keep extensions icon visible after switching sessions (#9787) i18n: add Spanish (es) locale support (#9833) fix(summon): skip non-recipe project config files (#9808) Dedup duplicate tool-call ids within a turn (#9792) fix: resolve bundled extensions from discovery (#9759) Keep the version-bump PR up to date by re-running on merge conflicts (#9761) feat(providers): add EmpirioLabs as a declarative OpenAI-compatible provider (#9771) Add support for MLX models to the local inference provider (#9154) fix(acp): include agentInfo in initialize response (#9765) ...
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cee62dee7e
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| unsubscribeAcp(); | ||
| unsubscribeGoose(); | ||
| unsubscribePermissionRequests(); | ||
| cancelAcpPermissionRequestsForSession(sessionId); |
There was a problem hiding this comment.
Avoid cancelling ACP approvals on unmount
When ACP chat is enabled and a tool permission prompt is visible, switching away from the chat or otherwise remounting this hook runs this cleanup and resolves the pending RequestPermission promise as cancelled. That sends a cancel/deny result back to the agent even though the user did not choose an approval button, so ordinary navigation can abort a tool call; only resolve these pending approvals from an explicit user action such as Stop/Cancel, or re-route them when the session remounts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 401c96c5ba
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await acpPromptSession(targetSessionId, userMessage); | ||
| onFinish(); |
There was a problem hiding this comment.
Guard ACP prompt completion by run
When the user presses Stop and a queued/retyped message is submitted in the same session before the cancelled client.prompt promise settles, this stale submitToAcpSession invocation still calls onFinish(); its finally cleanup below also compares only targetSessionId, so it can clear activeRequestSessionIdRef for the newer prompt. In that stop-and-resend path the new ACP run can flip to idle and lose Stop cancellation while it is still streaming; track a per-prompt token/run before finishing or clearing refs.
Useful? React with 👍 / 👎.
michaelneale
left a comment
There was a problem hiding this comment.
did quick look over @lifeizhou-ap approving so it can help you move ahead when ready
Summary
This PR migrates the desktop chat reply path to ACP behind
USE_ACP_CHAT.When the flag is enabled, desktop reply submission uses ACP
session/prompt, and incoming ACP notifications are adapted into the existing desktop chat state.Changes
useAcpChatSessionand select it fromuseChatSessionwhenUSE_ACP_CHATis enabled.Testing
Unit testing and Manual Testing
Manual validation covered ACP reply flow, tool calls, permission approval, and MCP app rendering with the ACP
chat flag enabled.
TODO in subsequent PRs
HistoryReplacedhandling for/clear,/compact, and auto-compaction.