feat(acp): add support for file reference in ACP protocol#1063
feat(acp): add support for file reference in ACP protocol#1063xiaoju111a wants to merge 1 commit intoMoonshotAI:mainfrom
Conversation
xiaoju111a
force-pushed
the
fix-acp-file-reference
branch
3 times, most recently
from
5f24b07 to
d5f3b10
Compare
Implements proper handling of ACP content blocks according to the official protocol specification: - EmbeddedResourceContentBlock: Extracts and converts embedded text content to TextPart. This is the preferred way for Clients to include context (e.g., @-mentions) that the Agent may not have direct access to. - ResourceContentBlock: Logs the resource reference but does not convert to content. According to ACP protocol, these are references to resources the Agent should be able to access directly through its own tools. This implementation follows the ACP design principle where Clients embed content they have access to, while resource links are left for the Agent to access when needed.
xiaoju111a
force-pushed
the
fix-acp-file-reference
branch
from
d5f3b10 to
a5eddc9
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| resource = block.resource | ||
| if isinstance(resource, acp.schema.TextResourceContents): | ||
| content.append(TextPart(text=f"File: {resource.uri}\n\n{resource.text}")) | ||
| elif resource.__class__.__name__ == "BlobResourceContents": |
There was a problem hiding this comment.
🟡 String-based class name check for BlobResourceContents is fragile and inconsistent
On line 36, resource.__class__.__name__ == "BlobResourceContents" is used instead of isinstance(resource, acp.schema.BlobResourceContents), while line 34 correctly uses isinstance(resource, acp.schema.TextResourceContents) for the sibling type.
Root Cause
The EmbeddedResourceContentBlock.resource field in ACP is a union of TextResourceContents | BlobResourceContents. Line 34 checks TextResourceContents via isinstance, but line 36 checks BlobResourceContents via string comparison on the class name. This is fragile because:
- It will fail to match subclasses of
BlobResourceContents(unlikeisinstance). - It could falsely match an unrelated class that happens to share the same name.
acp.schema.BlobResourceContentsshould be available inagent-client-protocol==0.7.0(theTextResourceContentssibling is available, and both are standard ACP/MCP resource content types). The kosong package in the same repo also referencesBlobResourceContentsatpackages/kosong/src/kosong/tooling/mcp.py.
Impact: If a future version of the ACP SDK introduces a subclass or alias of BlobResourceContents, the string check would silently fail, causing the resource to fall into the else branch and log an incorrect "Unknown resource type" warning instead of the intended "Skipping binary embedded resource" info message.
| elif resource.__class__.__name__ == "BlobResourceContents": | |
| elif isinstance(resource, acp.schema.BlobResourceContents): |
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Related Issue
Resolves #1054
Description
This PR adds support for embedded resources in the ACP (Agent Communication Protocol) by implementing proper handling of
EmbeddedResourceContentBlockaccording to the official ACP protocol specification. This enables IDE integrations (like Zed) to use @-mentions to reference files.Changes Made
src/kimi_cli/acp/convert.py:EmbeddedResourceContentBlock- extracts and converts embedded text content toTextPartTextResourceContents) and logs info for binary resources (BlobResourceContents)tests/ui_and_conv/test_acp_convert.py:EmbeddedResourceContentBlockwith embedded contentResourceContentBlock) are not convertedTechnical Details
According to the official ACP specification:
EmbeddedResourceContentBlock(type: "resource"):Checklist
make gen-changelogto update the changelog.make gen-docsto update the user documentation.