feat(agent-server): reintroduce ACP remote runtime via v2 conversations API#2462
feat(agent-server): reintroduce ACP remote runtime via v2 conversations API#2462simonrosenberg wants to merge 3 commits intomainfrom
Conversation
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Taste Rating: Acceptable - Solid architecture using API versioning to avoid breaking changes. A few maintainability concerns around error handling consistency and code duplication, but the core approach is sound. The polymorphic agent handling and v1/v2 split are pragmatic solutions to a real backward compatibility problem.
| _prompt, timeout=self.acp_prompt_timeout | ||
| ) | ||
| break # Success, exit retry loop | ||
| except TimeoutError: |
There was a problem hiding this comment.
🟠 Important: TimeoutError inconsistency - The general Exception handler re-raises with a comment explaining this is needed for LocalConversation.run() to work correctly, but TimeoutError does not re-raise. This creates an inconsistency where timeouts set error status but allow execution to continue normally, while other errors break the loop. Consider re-raising TimeoutError as well, or document why timeout is treated as a soft error.
|
|
||
|
|
||
| def _uses_v2_conversation_contract(agent: AgentBase) -> bool: | ||
| return getattr(agent, "kind", agent.__class__.__name__) == "ACPAgent" |
There was a problem hiding this comment.
🟠 Important: Fragile type checking - Using string comparison getattr(agent, "kind", agent.__class__.__name__) == "ACPAgent" is fragile. If the class is renamed or the kind field is missing, this breaks silently. Consider using isinstance() with proper imports.
| ) | ||
| return results | ||
|
|
||
| async def _notify_conversation_webhooks(self, conversation_info: BaseModel): |
There was a problem hiding this comment.
🟡 Suggestion: Type signature too broad - The parameter type is BaseModel but all callers pass ConversationInfoV2. Tightening this to ConversationInfoV2 would provide better type safety and make the intended usage clearer.
| # Collect all conversations with their info | ||
| all_conversations = [] | ||
| for id, event_service in self._event_services.items(): | ||
| if not include_v2 and not _is_v1_conversation(event_service.stored): |
There was a problem hiding this comment.
🟡 Suggestion: Code duplication pattern - The include_v2 flag creates parallel code paths in _search_conversations and _count_conversations that must be kept in sync. Any bug fix needs to be applied to both branches. This works but creates maintenance burden. Consider if the v1/v2 filtering could be unified to reduce duplication. Not blocking - the current approach is clear and testable.
Co-authored-by: openhands <openhands@all-hands.dev>
Summary
/api/conversationsREST contract backward-compatible for older clientsAgent | ACPAgentpayloadsACPAgentconversations through the new v2 contract while preserving existing defaults for regularAgentWhy this exists
#2190 was useful, but it was reverted by #2451 because eagerly registering
ACPAgentchanged the existingagentrequest/response schema on/api/conversations.Older clients that POST a plain
agentobject withoutagent.kindstarted failing with422, and the OpenAPI contract changed in place. That was a real breaking change.This PR takes the additive path instead:
AgentcontractACPAgentImplementation notes
StartConversationRequestV2/ConversationInfoV2plusACPEnabledAgent/api/v2/conversationsendpoints alongside the existing v1 endpointsAgentconversations onlyTest 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/agent/test_acp_agent.py tests/sdk/conversation/remote/test_remote_conversation.pyReferences
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:2251e5d-pythonRun
All tags pushed for this build
About Multi-Architecture Support
2251e5d-python) is a multi-arch manifest supporting both amd64 and arm642251e5d-python-amd64) are also available if needed