fix(acp): forward image tool-result blocks to ACP clients#562
fix(acp): forward image tool-result blocks to ACP clients#562SharanKarchella wants to merge 1 commit into
Conversation
toolResultContent() forwarded only text blocks, so image blocks (e.g. from web_fetch, or an MCP image tool whose block survives transformMcpContent) were dropped and ACP clients like Zed saw a completed tool call with empty content. Resolves the maintainer TODO. pi-ai types tool-result content as (TextContent | ImageContent)[], and the MCP bridge (transformMcpContent) already flattens resource / resource_link / audio to text, so image is the only non-text block that reaches this translation. The TODO's note about dropped MCP resource blocks no longer applies; the comment now describes the real data flow (true resource passthrough would need an upstream pi-ai change). Tests cover the end-result path, the streaming partial path, and mixed text+image ordering. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Documentation Changes Added
Actions
If neither actions are selected, on PR close/merge the docs branch in ReadMe will remain open. |
Kimchi Code Review
Summary📊 Review Score: 93/100 (overall code quality — 0 lowest, 100 highest) 🧪 Tests: yes — Three targeted tests were added: one for image-only tool_execution_end, one for image blocks in streaming partialResult, and one verifying mixed text/image ordering. Together they cover both call sites that use toolResultContent. No significant issues found. LGTM! 🎉 What to expectKimchi will analyze the changes in this pull request and post:
The review typically completes within a few minutes. This comment will be updated once the review is ready. Interact with Kimchi
ConfigurationReviews are configured by your organization admin. Powered by Kimchi — AI-powered code review by CAST AI |
There was a problem hiding this comment.
📊 Review Score: 93/100 (overall code quality — 0 lowest, 100 highest)
⏱️ Estimated effort to review: 2/5 (1 = trivial, 5 = very complex)
🧪 Tests: yes — Three targeted tests were added: one for image-only tool_execution_end, one for image blocks in streaming partialResult, and one verifying mixed text/image ordering. Together they cover both call sites that use toolResultContent.
No significant issues found. LGTM! 🎉
Description
toolResultContent()insrc/modes/acp/server.tsforwarded onlytextblocks to ACP clients, so image blocks in a tool result were silently dropped — a client like Zed saw a completed tool call with empty content. This resolves the maintainerTODOon that function.Resolves the maintainer
TODOintoolResultContent(src/modes/acp/server.ts).What actually reaches this code (AGENTS.md "Before Adding Features" check)
The ACP tool-result translation is local to this repo:
node_modules/@earendil-works/pi-coding-agent/disthas no@agentclientprotocol/toolResultContentreferences, so this is the correct layer to fix (not upstream).Tool-result content is typed by pi-ai as
(TextContent | ImageContent)[](ToolResultMessage). Therefore:imageblocks genuinely occur (web_fetch, or an MCP image tool whose block survives the bridge) and were the ones being dropped → fixed here.resource/resource_link/audioblocks never reachtoolResultContent: the MCP bridgetransformMcpContent()(src/extensions/mcp-adapter/tool-registrar.ts) already flattens them to text. Aresourcebranch here would be unreachable dead code, so the stale TODO comment is corrected instead. True resource passthrough would require an upstream pi-ai change (worth a separate follow-up issue).Changes
imagetool-result blocks as ACP{ type: "content", content: { type: "image", data, mimeType } }, mirroring the existing inbound prompt → pi-ai image mapping inserver.ts. Guarded ondata/mimeTypebeing strings, consistent with thetextbranch.tool execution streamsuite): image ontool_execution_end, image on a streamingpartialResult, and mixed text+image ordering.Verification
pnpm run check(biome lint +tsc --noEmit) — passes.pnpm run test— full unit suite green (299 files, 4589 passed / 3 skipped); the ACP server test file is 124/124 (3 new).Type of Change