feat: let ACP client expose the input/output properly#11303
feat: let ACP client expose the input/output properly#11303validatedev wants to merge 17 commits intoanomalyco:devfrom
Conversation
|
The following comment was made by an LLM, it may be inaccurate: No duplicate PRs found |
00637c0 to
71e0ba2
Compare
f1ae801 to
08fa7f7
Compare
|
Can you explain the changes, it looks like quite a bit |
|
any update @rekram1-node @adamdotdevin? I don't have any problems with this, works much better than the old structure (aligns other Zed ACP + Zed Agent interfaces) |
There was a problem hiding this comment.
Pull request overview
This PR refactors the ACP (Agent Client Protocol) client implementation to properly expose tool input/output information to ACP clients like Zed. The main change addresses issue #10998 where command details weren't visible in the Zed UI.
Changes:
- Refactored tool call/result formatting into a centralized
tool-format.tsmodule with structured formatting logic for different tool types - Changed tool call emission timing from
pendingtorunningstate with fullrawInput(command + cwd) included upfront - Updated tool kind from
"execute"to"other"for bash/shell/terminal commands
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| packages/opencode/src/acp/tool-format.ts | New module containing centralized tool formatting logic with toolCallFromPart and toolResultFromPart functions |
| packages/opencode/src/acp/parse-command.ts | New helper module for formatting bash/shell/terminal commands with title, kind, and locations |
| packages/opencode/src/acp/agent.ts | Refactored to use new formatting functions; removed old toToolKind and toLocations helpers; added emittedToolCalls tracking to prevent duplicate emissions |
| packages/opencode/test/acp/parse-command.test.ts | Added tests for ParseCommand.format function |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…pers - Remove parse-command.ts in favor of logic consolidated in tool-format.ts - Rewrite tool-format helpers (toolCallFromPart, toolResultFromPart, permissionDisplayInfo, fenceWith) as the single source of truth for tool call formatting - Adapt agent.ts to dev's post-namespace-refactor layout (flat exports with `export * as ACP` self-reexport, ProviderID/ModelID branded types, Hash.fast, InstallationVersion, ConfigMCP.Info) - Drop listSessions `unstable_` prefix to match SDK method name - Guard sendUsageUpdate against missing providerID/modelID
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- `read` tool: treat `offset` as 1-based (matches the read tool schema); previously off-by-one in the displayed line range and location. - `toolStart`: derive the initial `tool_call` status from `part.state.status` (pending/in_progress/completed/failed) instead of always emitting `pending`, so the first event reflects reality. - `bash`: fall back to the session cwd in both `locations` and `rawInput` when the model omits `workdir`, so Zed always shows where a command runs.
Guards against callers outside the zod-validated tool boundary (MCP tools, permission display, tests) producing NaN/Infinity titles like '(NaN - NaN)' in read ranges.
…ool status - Extract the bash-output hashing and content-block assembly into a single `runningContent(sessionId, part)` helper, and call it from both the `message.part.updated` live path and the `processMessage` replay path. Previously the replay path (loadSession/forkSession) emitted the output verbatim without seeding `bashSnapshots`, so the first live update after replay would re-emit the same content. The helper guarantees both paths stay in lockstep. - Replace the 4-level nested ternary in `toolStart` with a `Record<ToolPart state status, ToolCallStatus>` map plus a \`satisfies\` constraint, so adding a new tool-part state would be a compile error. - Cover the new contract with three tests: replay + live dedupe on identical bash output, first `tool_call` emitted at \`completed\` when a tool part is already completed on first observation, and the analogous \`failed\` status for error parts.
|
@adamdotdevin I really want this to be merged, and it works without any issues. Could you please review and/or merge if you have time? I've been waiting 3 months. The PR only affects the ACP part. I can fix it if you don't like any parts of the PR, and it fixes a real issue which should not be happening (and other providers' ACP clients do not have such issues). |
…run-command-message # Conflicts: # packages/opencode/src/acp/agent.ts
…validatedev/opencode into fix/acp-show-proper-run-command-message
…run-command-message # Conflicts: # packages/opencode/src/acp/agent.ts
…validatedev/opencode into fix/acp-show-proper-run-command-message
|
I'm also seeing this in Zed ACP mode: bash tool calls show only the human-readable This makes permission review and debugging difficult, because users cannot tell which command is about to run without digging into logs. It looks like the same issue reported in #10998 and #14034; both were auto-closed, but the behavior is still relevant. Could this PR be reviewed/merged, or could maintainers clarify what is blocking it? |
What does this PR do?
Fixes #10998.
Old behavior: we sent a
tool_callatpendingwithkind: "execute". As the tool ran, updates carried the content plus a completedrawOutput. Zed relied onkind: "execute"to render the blue "run command" box, but the actual command (rawInput) only appeared after completion, so you couldn't see what was running.New behavior: the first
tool_callnow reflects the tool part's actual status (e.g.in_progresswhen the tool is already running on first observation) and carries the fullrawInput— including the resolved working directory forbash— so Zed can show the command and where it's running before output lands. Subsequenttool_call_updates fill in streaming content and the finalrawOutput(stdout/stderr).kindis set toother(notexecute), which removes the blue run-box styling and instead exposesrawInput/rawOutputas collapsible panels.How did you verify your code works?
By running Zed's ACP inferface via:
Screenshot
Old Behavior
New Behavior