Conversation
Documents ACPAgent usage, configuration, metrics, and cleanup with a ready-to-run example referencing examples/01_standalone_sdk/40_acp_agent_example.py. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Documentation is clear and practical, but has organizational issues and unverified syntax.
Key Issue: Important information about ACP delegation model is scattered. The fact that this replaces direct LLM calls (no API key needed, server manages everything) should be front and center, not discovered at line 143.
|
|
||
| `ACPAgent` lets you use any [Agent Client Protocol](https://agentclientprotocol.com/protocol/overview) server as the backend for an OpenHands conversation. Instead of calling an LLM directly, the agent spawns an ACP server subprocess and communicates with it over JSON-RPC. The server manages its own LLM, tools, and execution — your code just sends messages and collects responses. | ||
|
|
||
| ## Basic Usage |
There was a problem hiding this comment.
🟠 Important: Verify wrap and focus attributes are valid Mintlify syntax. The SDK guidelines mention highlight for code blocks. If these are custom attributes, they may not render correctly.
Check the Mintlify docs or test in preview. Prefer highlight={5-7} if that's the intended effect.
sdk/guides/agent-acp.mdx
Outdated
| ### What ACPAgent Does Not Support | ||
|
|
||
| Because the ACP server manages its own tools and context, these `AgentBase` features are not available on `ACPAgent`: | ||
|
|
||
| - `tools` / `include_default_tools` — the server has its own tools | ||
| - `mcp_config` — configure MCP on the server side | ||
| - `condenser` — the server manages its own context window | ||
| - `critic` — the server manages its own evaluation | ||
| - `agent_context` — configure the server directly | ||
|
|
There was a problem hiding this comment.
🟡 Suggestion: This "what it does NOT support" section is critical context but comes too late. Users might waste time trying to configure MCP or tools before reading this.
Move this right after "Basic Usage" (around line 27) so developers understand the constraints upfront.
|
|
||
| print("Done!") | ||
| ``` | ||
|
|
There was a problem hiding this comment.
🟠 Important: This is the key differentiator of ACPAgent and it's buried at the end. Most developers scanning this doc will miss it.
Add a callout near the top (after "Basic Usage") explaining:
- ACP server manages its own LLM authentication
- No
LLM_API_KEYneeded in your code - This is delegation, not direct LLM calls
sdk/guides/agent-acp.mdx
Outdated
|
|
||
| 1. `ACPAgent` spawns the ACP server as a subprocess | ||
| 2. The SDK initializes the ACP protocol and creates a session | ||
| 3. When you call `conversation.send_message(...)`, the message is forwarded to the ACP server via `conn.prompt()` | ||
| 4. The server processes the request using its own LLM and tools, streaming session updates (text chunks, thought chunks, tool calls) back to the SDK | ||
| 5. The SDK accumulates the response and emits it as a `MessageEvent` | ||
| 6. Permission requests from the server are auto-approved |
There was a problem hiding this comment.
🟡 Suggestion: Step 6 "Permission requests from the server are auto-approved" is glossed over but has security implications. If the ACP server can execute arbitrary tools, users should know what they're granting.
Add one sentence explaining what "auto-approved" means in practice or link to ACP protocol docs about permissions.
| { | ||
| "group": "Agent Features", | ||
| "pages": [ | ||
| "sdk/guides/agent-acp", |
There was a problem hiding this comment.
✅ Placement looks correct - alphabetically ordered within "Agent Features" group.
|
@OpenHands read the PR and apply all suggestions |
|
I'm on it! simonrosenberg can track my progress at all-hands.dev |
- Replace 'wrap focus={5-7}' with 'highlight={5-7}' per Mintlify standards
- Remove 'wrap' attribute from other code blocks
- Add Note callout explaining key differentiator (no LLM_API_KEY needed)
- Move 'What ACPAgent Does Not Support' section after Basic Usage
- Expand permission auto-approval explanation with security context
Co-authored-by: openhands <openhands@all-hands.dev>
SummaryI've successfully applied all review suggestions from PR #351. The changes have already been committed and pushed to the Checklist of Review Suggestions Applied:
Commit Details:
The changes are concise and focused solely on addressing the review feedback. No extraneous modifications were made. |
3 participants
Summary
sdk/guides/agent-acp.mdxdocumentingACPAgentusage, configuration, metrics, and cleanupexamples/01_standalone_sdk/40_acp_agent_example.pyfrom software-agent-sdk#2133docs.jsonContext
This branch is needed by the
check-documented-examplesCI job on software-agent-sdk PR #2133, which checks out a matching docs branch (feat/acp-agent) to verify that new examples are documented.🤖 Generated with Claude Code