Enable ACPAgent on RemoteRuntime API via v2 conversations contract#2461
Enable ACPAgent on RemoteRuntime API via v2 conversations contract#2461simonrosenberg wants to merge 2 commits intomainfrom
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟡 Acceptable - Solid versioning strategy, but webhook contract change needs human review.
Verdict: ✅ Worth merging after addressing webhook concern. Core logic is sound, comprehensive tests, backward-compatible REST API.
Key Insight: The v1/v2 API split elegantly solves the ACP integration without breaking existing clients, but webhook notifications now use the v2 schema universally—verify this is acceptable for existing webhook subscribers before merging.
| _compose_conversation_info_v2(event_service.stored, state) | ||
| if use_v2 | ||
| else _compose_conversation_info_v1(event_service.stored, state) | ||
| ) |
There was a problem hiding this comment.
🟠 Important: Webhook contract breaking change.
The _notify_conversation_webhooks function now always sends ConversationInfoV2 format (via _compose_conversation_info_v2), even for v1 conversations. This means webhook subscribers will receive the polymorphic ACPEnabledAgent type in all notifications.
Impact:
- Subscribers doing strict schema validation against the old
ConversationInfotype may break - The change from
agent: Agenttoagent: Agent | ACPAgentis backward compatible for JSON parsing, but not for strict OpenAPI validation
Recommendation:
- Document this webhook schema change in the PR description
- Consider notifying known webhook users before merging
- Or: maintain separate webhook formats for v1 vs v2 conversations (more complex but fully backward compatible)
Is this intentional? Should webhooks preserve v1 format for v1 conversations?
| # LimitOverrunError, silently killing the filter/receive pipeline and | ||
| # leaving the prompt() future unresolved forever. 100 MiB is generous | ||
| # enough for any realistic message while still bounding memory. | ||
| _STREAM_READER_LIMIT: int = 100 * 1024 * 1024 # 100 MiB |
There was a problem hiding this comment.
🟡 Suggestion: Document memory impact of 100MB buffer.
The increase from 64KB to 100MB is well-justified (large tool outputs), but worth noting:
- Each ACP agent instance allocates this per subprocess stream (stdin, stdout, stderr filtered reader)
- In multi-agent scenarios (e.g., agent-server handling multiple conversations), this could add up to significant memory overhead
Consider adding an environment variable like ACP_STREAM_BUFFER_LIMIT to make this configurable for memory-constrained deployments. Not blocking, but would improve operational flexibility.
| # Configure Claude Code managed settings for headless operation: | ||
| # Allow all tool permissions (no human in the loop to approve). | ||
| RUN mkdir -p /etc/claude-code && \ | ||
| echo '{"permissions":{"allow":["Edit","Read","Bash"]}}' > /etc/claude-code/managed-settings.json |
There was a problem hiding this comment.
🟢 Nit: Verify managed-settings.json path.
Is /etc/claude-code/managed-settings.json the documented path for Claude Code managed settings? A comment with a reference to Claude Code docs would help future maintainers verify this is correct.
|
|
||
| agent: ACPEnabledAgent | ||
|
|
||
|
|
There was a problem hiding this comment.
🟢 Acceptable: StoredConversation schema evolution handled correctly.
StoredConversation now extends StartConversationRequestV2 (polymorphic agent), but this is backward compatible:
ACPEnabledAgent = Agent | ACPAgentmeans existingAgentinstances still validate- Pydantic discriminator handles both types correctly
- No migration needed for existing persisted conversations
Good type design here. 👍
Coverage Report •
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Co-authored-by: openhands <openhands@all-hands.dev>
|
Closing this PR since it's a duplicate of #2460 |
2 participants
Summary
main/api/conversationsREST contract as the v1 Agent-only shape/api/v2/conversationsand routeRemoteConversationthere automatically forACPAgentWhy this approach
#2190 was useful, but it widened the public v1 REST contract by eagerly registering
ACPAgentat theAgentBaseboundary. That changed the OpenAPI schema and caused older clients that posted a plainagentobject withoutkindto start failing with422.This PR keeps the old contract working in place and introduces ACP support through a versioned conversations API instead of changing v1 in place:
/api/conversationsremains backward-compatible and Agent-only/api/v2/conversationssupports bothAgentandACPAgentIncluded from #2190
Test plan
uv run pytest tests/agent_server/test_conversation_router.py tests/agent_server/test_conversation_router_v2.py tests/agent_server/test_openapi_discriminator.py tests/sdk/conversation/remote/test_remote_conversation.py tests/sdk/agent/test_acp_agent.pyuv run pre-commit run --files openhands-agent-server/openhands/agent_server/models.py openhands-agent-server/openhands/agent_server/conversation_service.py openhands-agent-server/openhands/agent_server/conversation_router_v2.py openhands-agent-server/openhands/agent_server/api.py openhands-sdk/openhands/sdk/conversation/impl/remote_conversation.py tests/agent_server/test_conversation_router.py tests/agent_server/test_conversation_router_v2.py tests/agent_server/test_openapi_discriminator.py tests/sdk/conversation/remote/test_remote_conversation.pyCloses the gap between #2190 and #2451 by reintroducing the feature behind a versioned REST contract.
Agent Server images for this PR
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22golang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:d7461a9-pythonRun
All tags pushed for this build
About Multi-Architecture Support
d7461a9-python) is a multi-arch manifest supporting both amd64 and arm64d7461a9-python-amd64) are also available if needed