From 53c95110d12aef0d0514b2e5b6c2ba4b438c86a6 Mon Sep 17 00:00:00 2001 From: Simon Rosenberg Date: Mon, 16 Mar 2026 11:38:04 -0300 Subject: [PATCH 1/6] feat(agent-server): add ACP remote runtime via v2 conversations API Co-authored-by: openhands --- .github/workflows/run-eval.yml | 16 +- .../01_standalone_sdk/40_acp_agent_example.py | 5 +- .../09_acp_agent_with_remote_runtime.py | 87 +++++ .../openhands/agent_server/api.py | 2 + .../agent_server/conversation_router_v2.py | 324 ++++++++++++++++ .../agent_server/conversation_service.py | 168 ++++++++- .../openhands/agent_server/docker/Dockerfile | 17 +- .../openhands/agent_server/event_service.py | 5 +- .../openhands/agent_server/models.py | 170 +++++++-- .../openhands/sdk/agent/acp_agent.py | 354 ++++++++++++++++-- openhands-sdk/openhands/sdk/agent/base.py | 7 + .../conversation/impl/local_conversation.py | 10 + .../conversation/impl/remote_conversation.py | 103 +++-- .../agent_server/test_conversation_router.py | 35 ++ .../test_conversation_router_v2.py | 92 +++++ .../test_openapi_discriminator.py | 20 + tests/sdk/agent/test_acp_agent.py | 327 ++++++++++++++-- .../remote/test_remote_conversation.py | 59 +++ 18 files changed, 1688 insertions(+), 113 deletions(-) create mode 100644 examples/02_remote_agent_server/09_acp_agent_with_remote_runtime.py create mode 100644 openhands-agent-server/openhands/agent_server/conversation_router_v2.py create mode 100644 tests/agent_server/test_conversation_router_v2.py diff --git a/.github/workflows/run-eval.yml b/.github/workflows/run-eval.yml index b5733711da..fe08c833ac 100644 --- a/.github/workflows/run-eval.yml +++ b/.github/workflows/run-eval.yml @@ -97,6 +97,18 @@ on: - gemini - gpt5 - planning + agent_type: + description: >- + Agent type: 'default' for standard Agent, + 'acp-claude' for ACPAgent with Claude Code, + 'acp-codex' for ACPAgent with Codex. + required: false + default: default + type: choice + options: + - default + - acp-claude + - acp-codex env: @@ -319,6 +331,7 @@ jobs: ENABLE_CONVERSATION_EVENT_LOGGING: ${{ github.event.inputs.enable_conversation_event_logging || false }} MAX_RETRIES: ${{ github.event.inputs.max_retries || '3' }} TOOL_PRESET: ${{ github.event.inputs.tool_preset || 'default' }} + AGENT_TYPE: ${{ github.event.inputs.agent_type || 'default' }} TRIGGERED_BY: ${{ github.actor }} run: | echo "Dispatching evaluation workflow with SDK commit: $SDK_SHA (benchmark: $BENCHMARK, eval branch: $EVAL_BRANCH, benchmarks branch: $BENCHMARKS_BRANCH, tool preset: $TOOL_PRESET)" @@ -337,8 +350,9 @@ jobs: --argjson enable_conversation_event_logging "$ENABLE_CONVERSATION_EVENT_LOGGING" \ --arg max_retries "$MAX_RETRIES" \ --arg tool_preset "$TOOL_PRESET" \ + --arg agent_type "$AGENT_TYPE" \ --arg triggered_by "$TRIGGERED_BY" \ - '{ref: $ref, inputs: {sdk_commit: $sdk, eval_limit: $eval_limit, models_json: ($models | tostring), trigger_reason: $reason, pr_number: $pr, benchmarks_branch: $benchmarks, benchmark: $benchmark, instance_ids: $instance_ids, num_infer_workers: $num_infer_workers, num_eval_workers: $num_eval_workers, enable_conversation_event_logging: $enable_conversation_event_logging, max_retries: $max_retries, tool_preset: $tool_preset, triggered_by: $triggered_by}}') + '{ref: $ref, inputs: {sdk_commit: $sdk, eval_limit: $eval_limit, models_json: ($models | tostring), trigger_reason: $reason, pr_number: $pr, benchmarks_branch: $benchmarks, benchmark: $benchmark, instance_ids: $instance_ids, num_infer_workers: $num_infer_workers, num_eval_workers: $num_eval_workers, enable_conversation_event_logging: $enable_conversation_event_logging, max_retries: $max_retries, tool_preset: $tool_preset, agent_type: $agent_type, triggered_by: $triggered_by}}') RESPONSE=$(curl -sS -o /tmp/dispatch.out -w "%{http_code}" -X POST \ -H "Authorization: token $PAT_TOKEN" \ -H "Accept: application/vnd.github+json" \ diff --git a/examples/01_standalone_sdk/40_acp_agent_example.py b/examples/01_standalone_sdk/40_acp_agent_example.py index cdd04bbbb4..6d91c20699 100644 --- a/examples/01_standalone_sdk/40_acp_agent_example.py +++ b/examples/01_standalone_sdk/40_acp_agent_example.py @@ -7,7 +7,7 @@ Prerequisites: - Node.js / npx available - - Claude Code CLI authenticated (or CLAUDE_API_KEY set) + - ANTHROPIC_BASE_URL and ANTHROPIC_API_KEY set (can point to LiteLLM proxy) Usage: uv run python examples/01_standalone_sdk/40_acp_agent_example.py @@ -38,6 +38,9 @@ "Based on what you just saw, which agent class is the newest addition?" ) print(f"ask_agent response: {response}") + # Report cost (ACP server reports usage via session_update notifications) + cost = agent.llm.metrics.accumulated_cost + print(f"EXAMPLE_COST: {cost:.4f}") finally: # Clean up the ACP server subprocess agent.close() diff --git a/examples/02_remote_agent_server/09_acp_agent_with_remote_runtime.py b/examples/02_remote_agent_server/09_acp_agent_with_remote_runtime.py new file mode 100644 index 0000000000..01a13ffed3 --- /dev/null +++ b/examples/02_remote_agent_server/09_acp_agent_with_remote_runtime.py @@ -0,0 +1,87 @@ +"""Example: ACPAgent with Remote Runtime via API. + +This example demonstrates running an ACPAgent (Claude Code via ACP protocol) +in a remote sandboxed environment via Runtime API. It follows the same pattern +as 04_convo_with_api_sandboxed_server.py but uses ACPAgent instead of the +default LLM-based Agent. + +Usage: + uv run examples/02_remote_agent_server/09_acp_agent_with_remote_runtime.py + +Requirements: + - LLM_BASE_URL: LiteLLM proxy URL (routes Claude Code requests) + - LLM_API_KEY: LiteLLM virtual API key + - RUNTIME_API_KEY: API key for runtime API access +""" + +import os +import time + +from openhands.sdk import ( + Conversation, + RemoteConversation, + get_logger, +) +from openhands.sdk.agent import ACPAgent +from openhands.workspace import APIRemoteWorkspace + + +logger = get_logger(__name__) + + +# ACP agents (Claude Code) route through LiteLLM proxy +llm_base_url = os.getenv("LLM_BASE_URL") +llm_api_key = os.getenv("LLM_API_KEY") +assert llm_base_url and llm_api_key, "LLM_BASE_URL and LLM_API_KEY required" + +# Set ANTHROPIC_* vars so Claude Code routes through LiteLLM +os.environ["ANTHROPIC_BASE_URL"] = llm_base_url +os.environ["ANTHROPIC_API_KEY"] = llm_api_key + +runtime_api_key = os.getenv("RUNTIME_API_KEY") +assert runtime_api_key, "RUNTIME_API_KEY required" + +# If GITHUB_SHA is set (e.g. running in CI of a PR), use that to ensure consistency +# Otherwise, use the latest image from main +server_image_sha = os.getenv("GITHUB_SHA") or "main" +server_image = f"ghcr.io/openhands/agent-server:{server_image_sha[:7]}-python-amd64" +logger.info(f"Using server image: {server_image}") + +with APIRemoteWorkspace( + runtime_api_url=os.getenv("RUNTIME_API_URL", "https://runtime.eval.all-hands.dev"), + runtime_api_key=runtime_api_key, + server_image=server_image, + image_pull_policy="Always", + target_type="binary", # CI builds binary target images + forward_env=["ANTHROPIC_BASE_URL", "ANTHROPIC_API_KEY"], +) as workspace: + agent = ACPAgent( + acp_command=["claude-agent-acp"], # Pre-installed in Docker image + ) + + received_events: list = [] + last_event_time = {"ts": time.time()} + + def event_callback(event) -> None: + received_events.append(event) + last_event_time["ts"] = time.time() + + conversation = Conversation( + agent=agent, workspace=workspace, callbacks=[event_callback] + ) + assert isinstance(conversation, RemoteConversation) + + try: + conversation.send_message( + "List the files in /workspace and describe what you see." + ) + conversation.run() + + while time.time() - last_event_time["ts"] < 2.0: + time.sleep(0.1) + + # Report cost + cost = conversation.conversation_stats.get_combined_metrics().accumulated_cost + print(f"EXAMPLE_COST: {cost:.4f}") + finally: + conversation.close() diff --git a/openhands-agent-server/openhands/agent_server/api.py b/openhands-agent-server/openhands/agent_server/api.py index 863188ebad..e4f09069fd 100644 --- a/openhands-agent-server/openhands/agent_server/api.py +++ b/openhands-agent-server/openhands/agent_server/api.py @@ -15,6 +15,7 @@ get_default_config, ) from openhands.agent_server.conversation_router import conversation_router +from openhands.agent_server.conversation_router_v2 import conversation_router_v2 from openhands.agent_server.conversation_service import ( get_default_conversation_service, ) @@ -199,6 +200,7 @@ def _add_api_routes(app: FastAPI, config: Config) -> None: api_router = APIRouter(prefix="/api", dependencies=dependencies) api_router.include_router(event_router) api_router.include_router(conversation_router) + api_router.include_router(conversation_router_v2) api_router.include_router(tool_router) api_router.include_router(bash_router) api_router.include_router(git_router) diff --git a/openhands-agent-server/openhands/agent_server/conversation_router_v2.py b/openhands-agent-server/openhands/agent_server/conversation_router_v2.py new file mode 100644 index 0000000000..a7f79a1524 --- /dev/null +++ b/openhands-agent-server/openhands/agent_server/conversation_router_v2.py @@ -0,0 +1,324 @@ +"""Versioned conversation router with ACP-capable agent contract.""" + +from typing import Annotated +from uuid import UUID + +from fastapi import APIRouter, Body, Depends, HTTPException, Query, Response, status +from pydantic import SecretStr + +from openhands.agent_server.conversation_service import ConversationService +from openhands.agent_server.dependencies import get_conversation_service +from openhands.agent_server.models import ( + AskAgentRequest, + AskAgentResponse, + ConversationInfoV2, + ConversationPageV2, + ConversationSortOrder, + GenerateTitleRequest, + GenerateTitleResponse, + SendMessageRequest, + SetConfirmationPolicyRequest, + SetSecurityAnalyzerRequest, + StartConversationRequestV2, + Success, + UpdateConversationRequest, + UpdateSecretsRequest, +) +from openhands.sdk import LLM, Agent, TextContent +from openhands.sdk.agent.acp_agent import ACPAgent +from openhands.sdk.conversation.state import ConversationExecutionStatus +from openhands.sdk.workspace import LocalWorkspace +from openhands.tools.preset.default import get_default_tools + + +conversation_router_v2 = APIRouter( + prefix="/v2/conversations", tags=["Conversations V2"] +) + +START_CONVERSATION_V2_EXAMPLES = [ + StartConversationRequestV2( + agent=Agent( + llm=LLM( + usage_id="your-llm-service", + model="your-model-provider/your-model-name", + api_key=SecretStr("your-api-key-here"), + ), + tools=get_default_tools(enable_browser=True), + ), + workspace=LocalWorkspace(working_dir="workspace/project"), + initial_message=SendMessageRequest( + role="user", content=[TextContent(text="Flip a coin!")] + ), + ).model_dump(exclude_defaults=True, mode="json"), + StartConversationRequestV2( + agent=ACPAgent(acp_command=["npx", "-y", "claude-agent-acp"]), + workspace=LocalWorkspace(working_dir="workspace/project"), + initial_message=SendMessageRequest( + role="user", + content=[TextContent(text="Inspect the repository and summarize it.")], + ), + ).model_dump(exclude_defaults=True, mode="json"), +] + + +@conversation_router_v2.get("/search") +async def search_conversations( + page_id: Annotated[ + str | None, + Query(title="Optional next_page_id from the previously returned page"), + ] = None, + limit: Annotated[ + int, + Query(title="The max number of results in the page", gt=0, lte=100), + ] = 100, + status: Annotated[ + ConversationExecutionStatus | None, + Query(title="Optional filter by conversation execution status"), + ] = None, + sort_order: Annotated[ + ConversationSortOrder, + Query(title="Sort order for conversations"), + ] = ConversationSortOrder.CREATED_AT_DESC, + conversation_service: ConversationService = Depends(get_conversation_service), +) -> ConversationPageV2: + """Search / List conversations using the v2 ACP-capable contract.""" + assert limit > 0 + assert limit <= 100 + return await conversation_service.search_conversations_v2( + page_id, limit, status, sort_order + ) + + +@conversation_router_v2.get("/count") +async def count_conversations( + status: Annotated[ + ConversationExecutionStatus | None, + Query(title="Optional filter by conversation execution status"), + ] = None, + conversation_service: ConversationService = Depends(get_conversation_service), +) -> int: + """Count conversations matching the given filters.""" + return await conversation_service.count_conversations_v2(status) + + +@conversation_router_v2.get( + "/{conversation_id}", responses={404: {"description": "Item not found"}} +) +async def get_conversation( + conversation_id: UUID, + conversation_service: ConversationService = Depends(get_conversation_service), +) -> ConversationInfoV2: + """Given an id, get a conversation using the v2 ACP-capable contract.""" + conversation = await conversation_service.get_conversation_v2(conversation_id) + if conversation is None: + raise HTTPException(status.HTTP_404_NOT_FOUND) + return conversation + + +@conversation_router_v2.get("") +async def batch_get_conversations( + ids: Annotated[list[UUID], Query()], + conversation_service: ConversationService = Depends(get_conversation_service), +) -> list[ConversationInfoV2 | None]: + """Get a batch of conversations given their ids. + + Returns null for any missing item. + """ + assert len(ids) < 100 + return await conversation_service.batch_get_conversations_v2(ids) + + +@conversation_router_v2.post("") +async def start_conversation( + request: Annotated[ + StartConversationRequestV2, Body(examples=START_CONVERSATION_V2_EXAMPLES) + ], + response: Response, + conversation_service: ConversationService = Depends(get_conversation_service), +) -> ConversationInfoV2: + """Start a conversation using the ACP-capable v2 contract.""" + info, is_new = await conversation_service.start_conversation_v2(request) + response.status_code = status.HTTP_201_CREATED if is_new else status.HTTP_200_OK + return info + + +@conversation_router_v2.post( + "/{conversation_id}/pause", responses={404: {"description": "Item not found"}} +) +async def pause_conversation( + conversation_id: UUID, + conversation_service: ConversationService = Depends(get_conversation_service), +) -> Success: + """Pause a conversation, allowing it to be resumed later.""" + paused = await conversation_service.pause_conversation(conversation_id) + if not paused: + raise HTTPException(status.HTTP_400_BAD_REQUEST) + return Success() + + +@conversation_router_v2.delete( + "/{conversation_id}", responses={404: {"description": "Item not found"}} +) +async def delete_conversation( + conversation_id: UUID, + conversation_service: ConversationService = Depends(get_conversation_service), +) -> Success: + """Permanently delete a conversation.""" + deleted = await conversation_service.delete_conversation(conversation_id) + if not deleted: + raise HTTPException(status.HTTP_400_BAD_REQUEST) + return Success() + + +@conversation_router_v2.post( + "/{conversation_id}/run", + responses={ + 404: {"description": "Item not found"}, + 409: {"description": "Conversation is already running"}, + }, +) +async def run_conversation( + conversation_id: UUID, + conversation_service: ConversationService = Depends(get_conversation_service), +) -> Success: + """Start running the conversation in the background.""" + event_service = await conversation_service.get_event_service(conversation_id) + if event_service is None: + raise HTTPException(status.HTTP_404_NOT_FOUND) + + try: + await event_service.run() + except ValueError as e: + if str(e) == "conversation_already_running": + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=( + "Conversation already running. Wait for completion or pause first." + ), + ) + raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) + + return Success() + + +@conversation_router_v2.post( + "/{conversation_id}/secrets", responses={404: {"description": "Item not found"}} +) +async def update_conversation_secrets( + conversation_id: UUID, + request: UpdateSecretsRequest, + conversation_service: ConversationService = Depends(get_conversation_service), +) -> Success: + """Update secrets for a conversation.""" + event_service = await conversation_service.get_event_service(conversation_id) + if event_service is None: + raise HTTPException(status.HTTP_404_NOT_FOUND) + from typing import cast + + from openhands.sdk.conversation.secret_registry import SecretValue + + secrets = cast(dict[str, SecretValue], request.secrets) + await event_service.update_secrets(secrets) + return Success() + + +@conversation_router_v2.post( + "/{conversation_id}/confirmation_policy", + responses={404: {"description": "Item not found"}}, +) +async def set_conversation_confirmation_policy( + conversation_id: UUID, + request: SetConfirmationPolicyRequest, + conversation_service: ConversationService = Depends(get_conversation_service), +) -> Success: + """Set the confirmation policy for a conversation.""" + event_service = await conversation_service.get_event_service(conversation_id) + if event_service is None: + raise HTTPException(status.HTTP_404_NOT_FOUND) + await event_service.set_confirmation_policy(request.policy) + return Success() + + +@conversation_router_v2.post( + "/{conversation_id}/security_analyzer", + responses={404: {"description": "Item not found"}}, +) +async def set_conversation_security_analyzer( + conversation_id: UUID, + request: SetSecurityAnalyzerRequest, + conversation_service: ConversationService = Depends(get_conversation_service), +) -> Success: + """Set the security analyzer for a conversation.""" + event_service = await conversation_service.get_event_service(conversation_id) + if event_service is None: + raise HTTPException(status.HTTP_404_NOT_FOUND) + await event_service.set_security_analyzer(request.security_analyzer) + return Success() + + +@conversation_router_v2.patch( + "/{conversation_id}", responses={404: {"description": "Item not found"}} +) +async def update_conversation( + conversation_id: UUID, + request: UpdateConversationRequest, + conversation_service: ConversationService = Depends(get_conversation_service), +) -> Success: + """Update conversation metadata.""" + updated = await conversation_service.update_conversation(conversation_id, request) + if not updated: + return Success(success=False) + return Success() + + +@conversation_router_v2.post( + "/{conversation_id}/generate_title", + responses={404: {"description": "Item not found"}}, + deprecated=True, +) +async def generate_conversation_title( + conversation_id: UUID, + request: GenerateTitleRequest, + conversation_service: ConversationService = Depends(get_conversation_service), +) -> GenerateTitleResponse: + """Generate a title for the conversation using LLM. + + Deprecated since v1.11.5 and scheduled for removal in v1.19.0. + """ + title = await conversation_service.generate_conversation_title( + conversation_id, request.max_length, request.llm + ) + if title is None: + raise HTTPException(status.HTTP_500_INTERNAL_SERVER_ERROR) + return GenerateTitleResponse(title=title) + + +@conversation_router_v2.post( + "/{conversation_id}/ask_agent", + responses={404: {"description": "Item not found"}}, +) +async def ask_agent( + conversation_id: UUID, + request: AskAgentRequest, + conversation_service: ConversationService = Depends(get_conversation_service), +) -> AskAgentResponse: + """Ask the agent a simple question without affecting conversation state.""" + response = await conversation_service.ask_agent(conversation_id, request.question) + if response is None: + raise HTTPException(status.HTTP_500_INTERNAL_SERVER_ERROR) + return AskAgentResponse(response=response) + + +@conversation_router_v2.post( + "/{conversation_id}/condense", + responses={404: {"description": "Item not found"}}, +) +async def condense_conversation( + conversation_id: UUID, + conversation_service: ConversationService = Depends(get_conversation_service), +) -> Success: + """Force condensation of the conversation history.""" + success = await conversation_service.condense(conversation_id) + if not success: + raise HTTPException(status.HTTP_404_NOT_FOUND, detail="Conversation not found") + return Success() diff --git a/openhands-agent-server/openhands/agent_server/conversation_service.py b/openhands-agent-server/openhands/agent_server/conversation_service.py index 96c06b2724..08f781db42 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_service.py +++ b/openhands-agent-server/openhands/agent_server/conversation_service.py @@ -3,25 +3,29 @@ import logging from dataclasses import dataclass, field from pathlib import Path -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, cast from uuid import UUID, uuid4 import httpx +from pydantic import BaseModel from openhands.agent_server.config import Config, WebhookSpec from openhands.agent_server.event_service import EventService from openhands.agent_server.models import ( ConversationInfo, + ConversationInfoV2, ConversationPage, + ConversationPageV2, ConversationSortOrder, StartConversationRequest, + StartConversationRequestV2, StoredConversation, UpdateConversationRequest, ) from openhands.agent_server.pub_sub import Subscriber from openhands.agent_server.server_details_router import update_last_execution_time from openhands.agent_server.utils import safe_rmtree, utc_now -from openhands.sdk import LLM, Event, Message +from openhands.sdk import LLM, Agent, Event, Message from openhands.sdk.conversation.state import ( ConversationExecutionStatus, ConversationState, @@ -38,9 +42,10 @@ logger = logging.getLogger(__name__) -def _compose_conversation_info( +def _compose_conversation_info_v1( stored: StoredConversation, state: ConversationState ) -> ConversationInfo: + assert isinstance(stored.agent, Agent) # Use mode='json' so SecretStr in nested structures (e.g. LookupSecret.headers, # agent.agent_context.secrets) serialize to strings. Without it, validation # fails because ConversationInfo expects dict[str, str] but receives SecretStr. @@ -53,6 +58,22 @@ def _compose_conversation_info( ) +def _compose_conversation_info_v2( + stored: StoredConversation, state: ConversationState +) -> ConversationInfoV2: + return ConversationInfoV2( + **state.model_dump(mode="json"), + title=stored.title, + metrics=stored.metrics, + created_at=stored.created_at, + updated_at=stored.updated_at, + ) + + +def _is_v1_conversation(stored: StoredConversation) -> bool: + return isinstance(stored.agent, Agent) + + def _register_agent_definitions( agent_defs: list["AgentDefinition"], *, @@ -105,13 +126,26 @@ class ConversationService: ) async def get_conversation(self, conversation_id: UUID) -> ConversationInfo | None: + if self._event_services is None: + raise ValueError("inactive_service") + event_service = self._event_services.get(conversation_id) + if event_service is None: + return None + if not _is_v1_conversation(event_service.stored): + return None + state = await event_service.get_state() + return _compose_conversation_info_v1(event_service.stored, state) + + async def get_conversation_v2( + self, conversation_id: UUID + ) -> ConversationInfoV2 | None: if self._event_services is None: raise ValueError("inactive_service") event_service = self._event_services.get(conversation_id) if event_service is None: return None state = await event_service.get_state() - return _compose_conversation_info(event_service.stored, state) + return _compose_conversation_info_v2(event_service.stored, state) async def search_conversations( self, @@ -120,14 +154,60 @@ async def search_conversations( execution_status: ConversationExecutionStatus | None = None, sort_order: ConversationSortOrder = ConversationSortOrder.CREATED_AT_DESC, ) -> ConversationPage: + items, next_page_id = await self._search_conversations( + page_id=page_id, + limit=limit, + execution_status=execution_status, + sort_order=sort_order, + include_v2=False, + ) + return ConversationPage( + items=cast(list[ConversationInfo], items), + next_page_id=next_page_id, + ) + + async def search_conversations_v2( + self, + page_id: str | None = None, + limit: int = 100, + execution_status: ConversationExecutionStatus | None = None, + sort_order: ConversationSortOrder = ConversationSortOrder.CREATED_AT_DESC, + ) -> ConversationPageV2: + items, next_page_id = await self._search_conversations( + page_id=page_id, + limit=limit, + execution_status=execution_status, + sort_order=sort_order, + include_v2=True, + ) + return ConversationPageV2( + items=cast(list[ConversationInfoV2], items), + next_page_id=next_page_id, + ) + + async def _search_conversations( + self, + page_id: str | None, + limit: int, + execution_status: ConversationExecutionStatus | None, + sort_order: ConversationSortOrder, + *, + include_v2: bool, + ) -> tuple[list[ConversationInfo | ConversationInfoV2], str | None]: if self._event_services is None: raise ValueError("inactive_service") # 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): + continue state = await event_service.get_state() - conversation_info = _compose_conversation_info(event_service.stored, state) + conversation_info = ( + _compose_conversation_info_v2(event_service.stored, state) + if include_v2 + else _compose_conversation_info_v1(event_service.stored, state) + ) # Apply status filter if provided if ( execution_status is not None @@ -168,11 +248,31 @@ async def search_conversations( break items.append(all_conversations[i][1]) - return ConversationPage(items=items, next_page_id=next_page_id) + return items, next_page_id async def count_conversations( self, execution_status: ConversationExecutionStatus | None = None, + ) -> int: + return await self._count_conversations( + execution_status=execution_status, + include_v2=False, + ) + + async def count_conversations_v2( + self, + execution_status: ConversationExecutionStatus | None = None, + ) -> int: + return await self._count_conversations( + execution_status=execution_status, + include_v2=True, + ) + + async def _count_conversations( + self, + execution_status: ConversationExecutionStatus | None, + *, + include_v2: bool, ) -> int: """Count conversations matching the given filters.""" if self._event_services is None: @@ -180,6 +280,8 @@ async def count_conversations( count = 0 for event_service in self._event_services.values(): + if not include_v2 and not _is_v1_conversation(event_service.stored): + continue state = await event_service.get_state() # Apply status filter if provided @@ -206,7 +308,18 @@ async def batch_get_conversations( ) return results - async def _notify_conversation_webhooks(self, conversation_info: ConversationInfo): + async def batch_get_conversations_v2( + self, conversation_ids: list[UUID] + ) -> list[ConversationInfoV2 | None]: + results = await asyncio.gather( + *[ + self.get_conversation_v2(conversation_id) + for conversation_id in conversation_ids + ] + ) + return results + + async def _notify_conversation_webhooks(self, conversation_info: BaseModel): """Notify all conversation webhook subscribers about conversation changes.""" if not self._conversation_webhook_subscribers: return @@ -241,16 +354,33 @@ async def _notify_and_log_errors(): async def start_conversation( self, request: StartConversationRequest ) -> tuple[ConversationInfo, bool]: + conversation_info, is_new = await self._start_conversation(request) + assert isinstance(conversation_info, ConversationInfo) + return conversation_info, is_new + + async def start_conversation_v2( + self, request: StartConversationRequestV2 + ) -> tuple[ConversationInfoV2, bool]: + conversation_info, is_new = await self._start_conversation(request) + assert isinstance(conversation_info, ConversationInfoV2) + return conversation_info, is_new + + async def _start_conversation( + self, request: StartConversationRequest | StartConversationRequestV2 + ) -> tuple[ConversationInfo | ConversationInfoV2, bool]: """Start a local event_service and return its id.""" if self._event_services is None: raise ValueError("inactive_service") conversation_id = request.conversation_id or uuid4() + use_v2 = isinstance(request, StartConversationRequestV2) existing_event_service = self._event_services.get(conversation_id) if existing_event_service and existing_event_service.is_open(): state = await existing_event_service.get_state() - conversation_info = _compose_conversation_info( - existing_event_service.stored, state + conversation_info = ( + _compose_conversation_info_v2(existing_event_service.stored, state) + if use_v2 + else _compose_conversation_info_v1(existing_event_service.stored, state) ) return conversation_info, False @@ -311,10 +441,16 @@ async def start_conversation( await event_service.send_message(message, True) state = await event_service.get_state() - conversation_info = _compose_conversation_info(event_service.stored, state) + conversation_info = ( + _compose_conversation_info_v2(event_service.stored, state) + if use_v2 + else _compose_conversation_info_v1(event_service.stored, state) + ) # Notify conversation webhooks about the started conversation - await self._notify_conversation_webhooks(conversation_info) + await self._notify_conversation_webhooks( + _compose_conversation_info_v2(event_service.stored, state) + ) return conversation_info, True @@ -326,7 +462,9 @@ async def pause_conversation(self, conversation_id: UUID) -> bool: await event_service.pause() # Notify conversation webhooks about the paused conversation state = await event_service.get_state() - conversation_info = _compose_conversation_info(event_service.stored, state) + conversation_info = _compose_conversation_info_v2( + event_service.stored, state + ) await self._notify_conversation_webhooks(conversation_info) return bool(event_service) @@ -346,7 +484,7 @@ async def delete_conversation(self, conversation_id: UUID) -> bool: # Notify conversation webhooks about the stopped conversation before closing try: state = await event_service.get_state() - conversation_info = _compose_conversation_info( + conversation_info = _compose_conversation_info_v2( event_service.stored, state ) conversation_info.execution_status = ( @@ -405,7 +543,7 @@ async def update_conversation( # Notify conversation webhooks about the updated conversation state = await event_service.get_state() - conversation_info = _compose_conversation_info(event_service.stored, state) + conversation_info = _compose_conversation_info_v2(event_service.stored, state) await self._notify_conversation_webhooks(conversation_info) logger.info( @@ -743,7 +881,7 @@ class ConversationWebhookSubscriber: spec: WebhookSpec session_api_key: str | None = None - async def post_conversation_info(self, conversation_info: ConversationInfo): + async def post_conversation_info(self, conversation_info: BaseModel): """Post conversation info to the webhook immediately (no batching).""" # Prepare headers headers = self.spec.headers.copy() diff --git a/openhands-agent-server/openhands/agent_server/docker/Dockerfile b/openhands-agent-server/openhands/agent_server/docker/Dockerfile index ef57817983..8b8cc5ae7e 100644 --- a/openhands-agent-server/openhands/agent_server/docker/Dockerfile +++ b/openhands-agent-server/openhands/agent_server/docker/Dockerfile @@ -85,7 +85,22 @@ RUN set -eux; \ chown -R ${USERNAME}:${USERNAME} /workspace; \ rm -rf /var/lib/apt/lists/* -# NOTE: we should NOT include UV_PROJECT_ENVIRONMENT here, +# Pre-install ACP servers for ACPAgent support (Claude Code + Codex) +# Install Node.js/npm if not present (SWE-bench base images may lack them) +RUN set -eux; \ + if ! command -v npm >/dev/null 2>&1; then \ + curl -fsSL https://deb.nodesource.com/setup_22.x | bash - && \ + apt-get install -y --no-install-recommends nodejs && \ + rm -rf /var/lib/apt/lists/*; \ + fi; \ + npm install -g @zed-industries/claude-agent-acp @zed-industries/codex-acp + +# 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 + +# NOTE: we should NOT include UV_PROJECT_ENVIRONMENT here, # since the agent might use it to perform other work (e.g. tools that use Python) COPY --from=ghcr.io/astral-sh/uv /uv /uvx /bin/ diff --git a/openhands-agent-server/openhands/agent_server/event_service.py b/openhands-agent-server/openhands/agent_server/event_service.py index 9b2dd03799..953251442a 100644 --- a/openhands-agent-server/openhands/agent_server/event_service.py +++ b/openhands-agent-server/openhands/agent_server/event_service.py @@ -11,7 +11,7 @@ StoredConversation, ) from openhands.agent_server.pub_sub import PubSub, Subscriber -from openhands.sdk import LLM, Agent, AgentBase, Event, Message, get_logger +from openhands.sdk import LLM, AgentBase, Event, Message, get_logger from openhands.sdk.conversation.impl.local_conversation import LocalConversation from openhands.sdk.conversation.secret_registry import SecretValue from openhands.sdk.conversation.state import ( @@ -429,7 +429,8 @@ async def start(self): workspace = self.stored.workspace assert isinstance(workspace, LocalWorkspace) Path(workspace.working_dir).mkdir(parents=True, exist_ok=True) - agent = Agent.model_validate( + agent_cls = type(self.stored.agent) + agent = agent_cls.model_validate( self.stored.agent.model_dump(context={"expose_secrets": True}), ) diff --git a/openhands-agent-server/openhands/agent_server/models.py b/openhands-agent-server/openhands/agent_server/models.py index 02810fccc2..05113f1b95 100644 --- a/openhands-agent-server/openhands/agent_server/models.py +++ b/openhands-agent-server/openhands/agent_server/models.py @@ -1,17 +1,17 @@ from abc import ABC from datetime import datetime from enum import Enum -from typing import Any, Literal -from uuid import uuid4 +from typing import Annotated, Any, Literal +from uuid import UUID, uuid4 -from pydantic import BaseModel, Field, field_validator +from pydantic import BaseModel, Discriminator, Field, Tag, field_validator from openhands.agent_server.utils import OpenHandsUUID, utc_now -from openhands.sdk import LLM, AgentBase, Event, ImageContent, Message, TextContent -from openhands.sdk.conversation.state import ( - ConversationExecutionStatus, - ConversationState, -) +from openhands.sdk import LLM, Agent, Event, ImageContent, Message, TextContent +from openhands.sdk.agent.acp_agent import ACPAgent +from openhands.sdk.conversation.conversation_stats import ConversationStats +from openhands.sdk.conversation.secret_registry import SecretRegistry +from openhands.sdk.conversation.state import ConversationExecutionStatus from openhands.sdk.hooks import HookConfig from openhands.sdk.llm.utils.metrics import MetricsSnapshot from openhands.sdk.plugin import PluginSource @@ -22,8 +22,19 @@ NeverConfirm, ) from openhands.sdk.subagent.schema import AgentDefinition -from openhands.sdk.utils.models import DiscriminatedUnionMixin, OpenHandsModel +from openhands.sdk.utils.models import ( + DiscriminatedUnionMixin, + OpenHandsModel, + kind_of, +) from openhands.sdk.workspace import LocalWorkspace +from openhands.sdk.workspace.base import BaseWorkspace + + +ACPEnabledAgent = Annotated[ + Annotated[Agent, Tag("Agent")] | Annotated[ACPAgent, Tag("ACPAgent")], + Discriminator(kind_of), +] class ConversationSortOrder(str, Enum): @@ -60,18 +71,14 @@ def create_message(self) -> Message: return message -class StartConversationRequest(BaseModel): - """Payload to create a new conversation. - - Contains an Agent configuration along with conversation-specific options. - """ +class _StartConversationRequestBase(BaseModel): + """Common conversation creation fields shared by v1 and v2 contracts.""" - agent: AgentBase workspace: LocalWorkspace = Field( ..., description="Working directory for agent operations and tool execution", ) - conversation_id: OpenHandsUUID | None = Field( + conversation_id: UUID | None = Field( default=None, description=( "Optional conversation ID. If not provided, a random UUID will be " @@ -144,13 +151,31 @@ class StartConversationRequest(BaseModel): ) -class StoredConversation(StartConversationRequest): +class StartConversationRequest(_StartConversationRequestBase): + """Payload to create a new conversation. + + Contains an Agent configuration along with conversation-specific options. + """ + + agent: Agent + + +class StartConversationRequestV2(_StartConversationRequestBase): + """Versioned payload to create a new conversation. + + Supports the ACP-capable polymorphic agent contract. + """ + + agent: ACPEnabledAgent + + +class StoredConversation(StartConversationRequestV2): """Stored details about a conversation. Extends StartConversationRequest with server-assigned fields. """ - id: OpenHandsUUID + id: UUID title: str | None = Field( default=None, description="User-defined title for the conversation" ) @@ -159,11 +184,83 @@ class StoredConversation(StartConversationRequest): updated_at: datetime = Field(default_factory=utc_now) -class ConversationInfo(ConversationState): - """Information about a conversation running locally without a Runtime sandbox.""" +class _ConversationInfoBase(BaseModel): + """Common conversation info fields shared by v1 and v2 contracts.""" - # ConversationState already includes id and agent - # Add additional metadata fields + id: UUID = Field(description="Unique conversation ID") + workspace: BaseWorkspace = Field( + ..., + description=( + "Workspace used by the agent to execute commands and read/write files. " + "Not the process working directory." + ), + ) + persistence_dir: str | None = Field( + default="workspace/conversations", + description="Directory for persisting conversation state and events. " + "If None, conversation will not be persisted.", + ) + max_iterations: int = Field( + default=500, + gt=0, + description=( + "Maximum number of iterations the agent can perform in a single run." + ), + ) + stuck_detection: bool = Field( + default=True, + description="Whether to enable stuck detection for the agent.", + ) + execution_status: ConversationExecutionStatus = Field( + default=ConversationExecutionStatus.IDLE + ) + confirmation_policy: ConfirmationPolicyBase = Field(default=NeverConfirm()) + security_analyzer: SecurityAnalyzerBase | None = Field( + default=None, + description="Optional security analyzer to evaluate action risks.", + ) + activated_knowledge_skills: list[str] = Field( + default_factory=list, + description="List of activated knowledge skills name", + ) + blocked_actions: dict[str, str] = Field( + default_factory=dict, + description="Actions blocked by PreToolUse hooks, keyed by action ID", + ) + blocked_messages: dict[str, str] = Field( + default_factory=dict, + description="Messages blocked by UserPromptSubmit hooks, keyed by message ID", + ) + last_user_message_id: str | None = Field( + default=None, + description=( + "Most recent user MessageEvent id for hook block checks. " + "Updated when user messages are emitted so Agent.step can pop " + "blocked_messages without scanning the event log. If None, " + "hook-blocked checks are skipped (legacy conversations)." + ), + ) + stats: ConversationStats = Field( + default_factory=ConversationStats, + description="Conversation statistics for tracking LLM metrics", + ) + secret_registry: SecretRegistry = Field( + default_factory=SecretRegistry, + description="Registry for handling secrets and sensitive data", + ) + agent_state: dict[str, Any] = Field( + default_factory=dict, + description="Dictionary for agent-specific runtime state that persists across " + "iterations.", + ) + hook_config: HookConfig | None = Field( + default=None, + description=( + "Hook configuration for this conversation. Includes definitions for " + "PreToolUse, PostToolUse, UserPromptSubmit, SessionStart, SessionEnd, " + "and Stop hooks." + ), + ) title: str | None = Field( default=None, description="User-defined title for the conversation" @@ -173,11 +270,40 @@ class ConversationInfo(ConversationState): updated_at: datetime = Field(default_factory=utc_now) +class ConversationInfo(_ConversationInfoBase): + """Information about a conversation running locally without a Runtime sandbox.""" + + agent: Agent = Field( + ..., + description=( + "The legacy v1 agent configuration. " + "This endpoint remains pinned to the standard Agent contract." + ), + ) + + class ConversationPage(BaseModel): items: list[ConversationInfo] next_page_id: str | None = None +class ConversationInfoV2(_ConversationInfoBase): + """Versioned conversation info that supports ACP-capable agent configs.""" + + agent: ACPEnabledAgent = Field( + ..., + description=( + "The agent running in the conversation. " + "Version 2 supports both Agent and ACPAgent payloads." + ), + ) + + +class ConversationPageV2(BaseModel): + items: list[ConversationInfoV2] + next_page_id: str | None = None + + class ConversationResponse(BaseModel): conversation_id: str state: ConversationExecutionStatus diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index 826a6ace5c..9359b1db49 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -11,8 +11,11 @@ from __future__ import annotations import asyncio +import json import os import threading +import time +import uuid from collections.abc import Generator from typing import TYPE_CHECKING, Any @@ -34,10 +37,23 @@ from openhands.sdk.agent.base import AgentBase from openhands.sdk.conversation.state import ConversationExecutionStatus -from openhands.sdk.event import ACPToolCallEvent, MessageEvent, SystemPromptEvent -from openhands.sdk.llm import LLM, Message, TextContent +from openhands.sdk.event import ( + ACPToolCallEvent, + ActionEvent, + MessageEvent, + ObservationEvent, + SystemPromptEvent, +) +from openhands.sdk.event.conversation_error import ConversationErrorEvent +from openhands.sdk.llm import LLM, Message, MessageToolCall, TextContent from openhands.sdk.logger import get_logger +from openhands.sdk.observability.laminar import maybe_init_laminar, observe from openhands.sdk.tool import Tool # noqa: TC002 +from openhands.sdk.tool.builtins.finish import FinishAction, FinishObservation + + +logger = get_logger(__name__) +maybe_init_laminar() if TYPE_CHECKING: @@ -49,8 +65,6 @@ ) -logger = get_logger(__name__) - # Seconds to wait after prompt() for pending session_update notifications # to be processed. This is a best-effort workaround: the ACP protocol does # not currently signal when all notifications for a turn have been delivered, @@ -64,6 +78,24 @@ os.environ.get("ACP_NOTIFICATION_DRAIN_DELAY", "0.1") ) +# Retry configuration for transient ACP connection errors. +# These errors can occur when the connection drops mid-conversation but the +# session state is still valid on the server side. +_ACP_PROMPT_MAX_RETRIES: int = int(os.environ.get("ACP_PROMPT_MAX_RETRIES", "3")) +_ACP_PROMPT_RETRY_DELAYS: tuple[float, ...] = (5.0, 15.0, 30.0) # seconds + +# Exception types that indicate transient connection issues worth retrying +_RETRIABLE_CONNECTION_ERRORS = (OSError, ConnectionError, BrokenPipeError, EOFError) + +# Limit for asyncio.StreamReader buffers used by the ACP subprocess pipes. +# The default (64 KiB) is too small for session_update notifications that +# carry large tool-call outputs (e.g. file contents, test results). When +# a single JSON-RPC line exceeds the limit, readline() raises +# 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 + async def _drain_notifications() -> None: """Best-effort drain of pending ``session_update`` notifications. @@ -86,6 +118,55 @@ def _make_dummy_llm() -> LLM: # --------------------------------------------------------------------------- +# Known ACP server name → bypass-permissions mode ID mappings. +_BYPASS_MODE_MAP: dict[str, str] = { + "claude-agent": "bypassPermissions", + "codex-acp": "full-access", +} +_DEFAULT_BYPASS_MODE = "full-access" + +# ACP auth method ID → environment variable that supplies the credential. +# When the server reports auth_methods, we pick the first method whose +# required env var is set. +# Note: claude-login is intentionally NOT included because Claude Code ACP +# uses bypassPermissions mode instead of API key authentication. +_AUTH_METHOD_ENV_MAP: dict[str, str] = { + "codex-api-key": "CODEX_API_KEY", + "openai-api-key": "OPENAI_API_KEY", +} + + +def _select_auth_method( + auth_methods: list[Any], + env: dict[str, str], +) -> str | None: + """Pick an auth method whose required env var is present. + + Returns the ``id`` of the first matching method, or ``None`` if no + env-var-based method is available (the server may not require auth). + """ + method_ids = {m.id for m in auth_methods} + for method_id, env_var in _AUTH_METHOD_ENV_MAP.items(): + if method_id in method_ids and env_var in env: + return method_id + return None + + +def _resolve_bypass_mode(agent_name: str) -> str: + """Return the session mode ID that bypasses all permission prompts. + + Different ACP servers use different mode IDs for the same concept: + - claude-agent-acp → ``bypassPermissions`` + - codex-acp → ``full-access`` + + Falls back to ``full-access`` for unknown servers. + """ + for key, mode in _BYPASS_MODE_MAP.items(): + if key in agent_name.lower(): + return mode + return _DEFAULT_BYPASS_MODE + + async def _filter_jsonrpc_lines(source: Any, dest: Any) -> None: """Read lines from *source* and forward only JSON-RPC lines to *dest*. @@ -315,7 +396,8 @@ class ACPAgent(AgentBase): acp_command: list[str] = Field( ..., description=( - "Command to start the ACP server, e.g. ['npx', '-y', 'claude-code-acp']" + "Command to start the ACP server, e.g." + " ['npx', '-y', '@zed-industries/claude-agent-acp']" ), ) acp_args: list[str] = Field( @@ -326,6 +408,26 @@ class ACPAgent(AgentBase): default_factory=dict, description="Additional environment variables for the ACP server process", ) + acp_session_mode: str | None = Field( + default=None, + description=( + "Session mode ID to set after creating a session. " + "If None (default), auto-detected from the ACP server type: " + "'bypassPermissions' for claude-agent-acp, 'full-access' for codex-acp." + ), + ) + acp_prompt_timeout: float = Field( + default=1800.0, + description=( + "Timeout in seconds for a single ACP prompt() call. " + "Prevents indefinite hangs when the ACP server fails to respond." + ), + ) + acp_model: str | None = Field( + default=None, + description="Model for the ACP server to use (e.g. 'claude-opus-4-6'). " + "Passed via session _meta. If None, the server picks its default.", + ) # Private runtime state _executor: Any = PrivateAttr(default=None) @@ -336,11 +438,28 @@ class ACPAgent(AgentBase): _filtered_reader: Any = PrivateAttr(default=None) # StreamReader _closed: bool = PrivateAttr(default=False) _working_dir: str = PrivateAttr(default="") + _agent_name: str = PrivateAttr( + default="" + ) # ACP server name from InitializeResponse + _agent_version: str = PrivateAttr( + default="" + ) # ACP server version from InitializeResponse # -- Helpers ----------------------------------------------------------- - def _record_usage(self, response: PromptResponse | None, session_id: str) -> None: - """Record token usage and notify stats callback from a PromptResponse.""" + def _record_usage( + self, + response: PromptResponse | None, + session_id: str, + elapsed: float | None = None, + ) -> None: + """Record token usage, latency, and notify stats callback from a PromptResponse. + + Args: + response: The ACP PromptResponse (may carry a ``usage`` field). + session_id: Session identifier used as the response_id for metrics. + elapsed: Wall-clock seconds for this prompt round-trip (optional). + """ if response is not None and response.usage is not None: usage = response.usage self.llm.metrics.add_token_usage( @@ -353,6 +472,9 @@ def _record_usage(self, response: PromptResponse | None, session_id: str) -> Non response_id=session_id, ) + if elapsed is not None: + self.llm.metrics.add_response_latency(elapsed, session_id) + if self.llm.telemetry._stats_update_callback is not None: try: self.llm.telemetry._stats_update_callback() @@ -365,6 +487,16 @@ def _record_usage(self, response: PromptResponse | None, session_id: str) -> Non def system_message(self) -> str: return "ACP-managed agent" + @property + def agent_name(self) -> str: + """Name of the ACP server (from InitializeResponse.agent_info).""" + return self._agent_name + + @property + def agent_version(self) -> str: + """Version of the ACP server (from InitializeResponse.agent_info).""" + return self._agent_version + def get_all_llms(self) -> Generator[LLM]: yield self.llm @@ -445,7 +577,7 @@ def _start_acp_server(self, state: ConversationState) -> None: working_dir = str(state.workspace.working_dir) - async def _init() -> tuple[Any, Any, Any, str]: + async def _init() -> tuple[Any, Any, Any, str, str, str]: # Spawn the subprocess directly so we can install a # filtering reader that skips non-JSON-RPC lines some # ACP servers (e.g. claude-code-acp v0.1.x) write to @@ -457,13 +589,14 @@ async def _init() -> tuple[Any, Any, Any, str]: stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.PIPE, env=env, + limit=_STREAM_READER_LIMIT, ) assert process.stdin is not None assert process.stdout is not None # Wrap the subprocess stdout in a filtering reader that # only passes lines starting with '{' (JSON-RPC messages). - filtered_reader = asyncio.StreamReader() + filtered_reader = asyncio.StreamReader(limit=_STREAM_READER_LIMIT) asyncio.get_event_loop().create_task( _filter_jsonrpc_lines(process.stdout, filtered_reader) ) @@ -474,19 +607,71 @@ async def _init() -> tuple[Any, Any, Any, str]: filtered_reader, # read filtered output ) - # Initialize the protocol - await conn.initialize(protocol_version=1) + # Initialize the protocol and discover server identity + init_response = await conn.initialize(protocol_version=1) + agent_name = "" + agent_version = "" + if init_response.agent_info is not None: + agent_name = init_response.agent_info.name or "" + agent_version = init_response.agent_info.version or "" + logger.info( + "ACP server initialized: agent_name=%r, agent_version=%r", + agent_name, + agent_version, + ) + + # Authenticate if the server requires it. Some ACP servers + # (e.g. codex-acp) require an explicit authenticate call + # before session creation. We auto-detect the method from + # the env vars that are available to the process. + auth_methods = init_response.auth_methods or [] + if auth_methods: + method_id = _select_auth_method(auth_methods, env) + if method_id is not None: + logger.info("Authenticating with ACP method: %s", method_id) + await conn.authenticate(method_id=method_id) + else: + logger.warning( + "ACP server offers auth methods %s but no matching " + "env var is set — session creation may fail", + [m.id for m in auth_methods], + ) + + # Build _meta content for session options (e.g. model selection). + # Extra kwargs to new_session() become the _meta dict in the + # JSON-RPC request — do NOT wrap in _meta= (that double-nests). + session_meta: dict[str, Any] = {} + if self.acp_model and "claude" in agent_name.lower(): + session_meta["claudeCode"] = {"options": {"model": self.acp_model}} # Create a new session - response = await conn.new_session(cwd=working_dir) + response = await conn.new_session(cwd=working_dir, **session_meta) session_id = response.session_id - return conn, process, filtered_reader, session_id + # Resolve the permission mode to use. Different ACP servers + # use different mode IDs for the same concept (no-prompts): + # - claude-agent-acp → "bypassPermissions" + # - codex-acp → "full-access" + mode_id = self.acp_session_mode + if mode_id is None: + mode_id = _resolve_bypass_mode(agent_name) + logger.info("Setting ACP session mode: %s", mode_id) + await conn.set_session_mode(mode_id=mode_id, session_id=session_id) + + return conn, process, filtered_reader, session_id, agent_name, agent_version result = self._executor.run_async(_init) - self._conn, self._process, self._filtered_reader, self._session_id = result + ( + self._conn, + self._process, + self._filtered_reader, + self._session_id, + self._agent_name, + self._agent_version, + ) = result self._working_dir = working_dir + @observe(name="acp_agent.step", ignore_inputs=["conversation", "on_event"]) def step( self, conversation: LocalConversation, @@ -517,6 +702,7 @@ def step( self._client.reset() self._client.on_token = on_token + t0 = time.monotonic() try: async def _prompt() -> PromptResponse: @@ -527,10 +713,52 @@ async def _prompt() -> PromptResponse: await _drain_notifications() return response - # Send prompt to ACP server - response = self._executor.run_async(_prompt) + # Send prompt to ACP server with retry logic for connection errors. + # Transient connection failures (network blips, server restarts) are + # retried to preserve session state and avoid losing progress. + logger.info( + "Sending ACP prompt (timeout=%.0fs, msg=%d chars)", + self.acp_prompt_timeout, + len(user_message), + ) - self._record_usage(response, self._session_id or "") + response: PromptResponse | None = None + max_retries = _ACP_PROMPT_MAX_RETRIES + + for attempt in range(max_retries + 1): + try: + response = self._executor.run_async( + _prompt, timeout=self.acp_prompt_timeout + ) + break # Success, exit retry loop + except TimeoutError: + # Timeout is handled separately below, don't retry + raise + except _RETRIABLE_CONNECTION_ERRORS as e: + if attempt < max_retries: + delay = _ACP_PROMPT_RETRY_DELAYS[ + min(attempt, len(_ACP_PROMPT_RETRY_DELAYS) - 1) + ] + logger.warning( + "ACP prompt failed with retriable error (attempt %d/%d), " + "retrying in %.0fs: %s", + attempt + 1, + max_retries + 1, + delay, + e, + ) + time.sleep(delay) + # Reset accumulators for retry (partial state may be stale) + self._client.reset() + self._client.on_token = on_token + else: + # Max retries exceeded + raise + + elapsed = time.monotonic() - t0 + logger.info("ACP prompt returned in %.1fs", elapsed) + + self._record_usage(response, self._session_id or "", elapsed=elapsed) # Emit ACPToolCallEvents for each accumulated tool call for tc in self._client.accumulated_tool_calls: @@ -563,23 +791,99 @@ async def _prompt() -> PromptResponse: llm_message=message, ) on_event(msg_event) + + # Emit FinishAction so evaluation frameworks (SWE-bench, etc.) + # recognize that the agent has completed its task. The regular + # Agent emits this when the LLM calls the "finish" tool; for + # ACPAgent each step() is a complete turn, so we always finish. + finish_action = FinishAction(message=response_text) + tc_id = str(uuid.uuid4()) + action_event = ActionEvent( + source="agent", + thought=[], + action=finish_action, + tool_name="finish", + tool_call_id=tc_id, + tool_call=MessageToolCall( + id=tc_id, + name="finish", + arguments=json.dumps({"message": response_text}), + origin="completion", + ), + llm_response_id=str(uuid.uuid4()), + ) + on_event(action_event) + on_event( + ObservationEvent( + observation=FinishObservation.from_text(text=response_text), + action_id=action_event.id, + tool_name="finish", + tool_call_id=tc_id, + ) + ) + state.execution_status = ConversationExecutionStatus.FINISHED + except TimeoutError: + elapsed = time.monotonic() - t0 + logger.error( + "ACP prompt timed out after %.1fs (limit=%.0fs). " + "The ACP server may have completed its work but failed to " + "send the JSON-RPC response. Accumulated %d text chunks, " + "%d tool calls.", + elapsed, + self.acp_prompt_timeout, + len(self._client.accumulated_text), + len(self._client.accumulated_tool_calls), + ) + error_message = Message( + role="assistant", + content=[ + TextContent( + text=( + f"ACP prompt timed out after {elapsed:.0f}s. " + "The agent may have completed its work but " + "the response was not received." + ) + ) + ], + ) + on_event(MessageEvent(source="agent", llm_message=error_message)) + state.execution_status = ConversationExecutionStatus.ERROR except Exception as e: logger.error("ACP prompt failed: %s", e, exc_info=True) - # Emit error as an agent message since AgentErrorEvent requires - # tool context we don't have + error_str = str(e) + + # Emit error as an agent message (existing behavior, preserved for + # consumers that inspect MessageEvents) error_message = Message( role="assistant", content=[TextContent(text=f"ACP error: {e}")], ) - error_event = MessageEvent( - source="agent", - llm_message=error_message, + on_event(MessageEvent(source="agent", llm_message=error_message)) + + # Emit typed ConversationErrorEvent so RemoteConversation can + # report the actual error detail via _get_last_error_detail() + # instead of falling back to "Remote conversation ended with error" + is_aup = ( + "usage policy" in error_str.lower() + or "content policy" in error_str.lower() + ) + on_event( + ConversationErrorEvent( + source="agent", + code="UsagePolicyRefusal" if is_aup else "ACPPromptError", + detail=error_str[:500], + ) ) - on_event(error_event) + state.execution_status = ConversationExecutionStatus.ERROR + # Re-raise so LocalConversation.run()'s outer except handler + # breaks the loop, emits ConversationErrorEvent, and raises + # ConversationRunError — matching how the regular Agent works + raise + def ask_agent(self, question: str) -> str | None: """Fork the ACP session, prompt the fork, and return the response.""" if self._conn is None: @@ -601,14 +905,16 @@ async def _fork_and_prompt() -> str: client._fork_session_id = fork_session_id client._fork_accumulated_text.clear() try: + fork_t0 = time.monotonic() response = await self._conn.prompt( [text_block(question)], fork_session_id, ) await _drain_notifications() + fork_elapsed = time.monotonic() - fork_t0 result = "".join(client._fork_accumulated_text) - self._record_usage(response, fork_session_id) + self._record_usage(response, fork_session_id, elapsed=fork_elapsed) return result finally: client._fork_session_id = None diff --git a/openhands-sdk/openhands/sdk/agent/base.py b/openhands-sdk/openhands/sdk/agent/base.py index 26ffd5204f..b3b822b2b0 100644 --- a/openhands-sdk/openhands/sdk/agent/base.py +++ b/openhands-sdk/openhands/sdk/agent/base.py @@ -576,3 +576,10 @@ def ask_agent(self, question: str) -> str | None: # noqa: ARG002 Response string, or ``None`` to use the default LLM-based approach. """ return None + + def close(self) -> None: + """Clean up agent resources. + + No-op by default; ACPAgent overrides to terminate subprocess. + """ + pass diff --git a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py index 176714f1c6..68b0e4997a 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py @@ -805,6 +805,11 @@ def close(self) -> None: except AttributeError: # Object may be partially constructed; span fields may be missing. pass + # Clean up agent resources (e.g., ACPAgent subprocess) + try: + self.agent.close() + except Exception as e: + logger.warning(f"Error closing agent: {e}") if self.delete_on_close: try: tools_map = self.agent.tools_map @@ -913,6 +918,11 @@ def generate_title(self, llm: LLM | None = None, max_length: int = 50) -> str: # Use provided LLM or fall back to agent's LLM llm_to_use = llm or self.agent.llm + # Skip LLM-based title generation for ACP agents with sentinel LLM + # The sentinel model "acp-managed" cannot make LLM calls directly + if llm_to_use.model == "acp-managed": + llm_to_use = None + return generate_conversation_title( events=self._state.events, llm=llm_to_use, max_length=max_length ) diff --git a/openhands-sdk/openhands/sdk/conversation/impl/remote_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/remote_conversation.py index e3b763fa7a..44aeba305b 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/remote_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/remote_conversation.py @@ -56,6 +56,21 @@ logger = get_logger(__name__) +V1_CONVERSATIONS_PATH = "/api/conversations" +V2_CONVERSATIONS_PATH = "/api/v2/conversations" + + +def _uses_v2_conversation_contract(agent: AgentBase) -> bool: + return getattr(agent, "kind", agent.__class__.__name__) == "ACPAgent" + + +def _validate_remote_agent(agent_data: dict) -> AgentBase: + if agent_data.get("kind") == "ACPAgent": + from openhands.sdk.agent.acp_agent import ACPAgent + + return ACPAgent.model_validate(agent_data) + return AgentBase.model_validate(agent_data) + def _send_request( client: httpx.Client, @@ -222,13 +237,17 @@ class RemoteEventsList(EventsListBase): _client: httpx.Client _conversation_id: str + _conversation_base_path: str _cached_events: list[Event] _cached_event_ids: set[str] _lock: threading.RLock - def __init__(self, client: httpx.Client, conversation_id: str): + def __init__( + self, client: httpx.Client, conversation_id: str, conversation_base_path: str + ): self._client = client self._conversation_id = conversation_id + self._conversation_base_path = conversation_base_path self._cached_events: list[Event] = [] self._cached_event_ids: set[str] = set() self._lock = threading.RLock() @@ -250,7 +269,7 @@ def _do_full_sync(self) -> None: resp = _send_request( self._client, "GET", - f"/api/conversations/{self._conversation_id}/events/search", + f"{self._conversation_base_path}/{self._conversation_id}/events/search", params=params, ) data = resp.json() @@ -292,7 +311,10 @@ def reconcile(self) -> int: resp = _send_request( self._client, "GET", - f"/api/conversations/{self._conversation_id}/events/search", + ( + f"{self._conversation_base_path}/" + f"{self._conversation_id}/events/search" + ), params=params, ) data = resp.json() @@ -378,14 +400,18 @@ class RemoteState(ConversationStateProtocol): _client: httpx.Client _conversation_id: str + _conversation_base_path: str _events: RemoteEventsList _cached_state: dict | None _lock: threading.RLock - def __init__(self, client: httpx.Client, conversation_id: str): + def __init__( + self, client: httpx.Client, conversation_id: str, conversation_base_path: str + ): self._client = client self._conversation_id = conversation_id - self._events = RemoteEventsList(client, conversation_id) + self._conversation_base_path = conversation_base_path + self._events = RemoteEventsList(client, conversation_id, conversation_base_path) # Cache for state information to avoid REST calls self._cached_state = None @@ -400,7 +426,9 @@ def _get_conversation_info(self) -> dict: # Fallback to REST API if no cached state resp = _send_request( - self._client, "GET", f"/api/conversations/{self._conversation_id}" + self._client, + "GET", + f"{self._conversation_base_path}/{self._conversation_id}", ) state = resp.json() self._cached_state = state @@ -497,7 +525,7 @@ def agent(self): agent_data = info.get("agent") if agent_data is None: raise RuntimeError("agent missing in conversation info: " + str(info)) - return AgentBase.model_validate(agent_data) + return _validate_remote_agent(agent_data) @property def workspace(self): @@ -564,6 +592,7 @@ class RemoteConversation(BaseConversation): _client: httpx.Client _cleanup_initiated: bool _terminal_status_queue: Queue[str] # Thread-safe queue for terminal status from WS + _conversation_base_path: str delete_on_close: bool = False def __init__( @@ -617,6 +646,11 @@ def __init__( self.max_iteration_per_run = max_iteration_per_run self.workspace = workspace self._client = workspace.client + self._conversation_base_path = ( + V2_CONVERSATIONS_PATH + if _uses_v2_conversation_contract(agent) + else V1_CONVERSATIONS_PATH + ) self._cleanup_initiated = False self._terminal_status_queue: Queue[str] = Queue() @@ -626,7 +660,7 @@ def __init__( resp = _send_request( self._client, "GET", - f"/api/conversations/{conversation_id}", + f"{self._conversation_base_path}/{conversation_id}", acceptable_status_codes={404}, ) if resp.status_code == 404: @@ -681,7 +715,10 @@ def __init__( if conversation_id is not None: payload["conversation_id"] = str(conversation_id) resp = _send_request( - self._client, "POST", "/api/conversations", json=payload + self._client, + "POST", + self._conversation_base_path, + json=payload, ) data = resp.json() # Expect a ConversationInfo @@ -693,7 +730,11 @@ def __init__( self._id = uuid.UUID(cid) # Initialize the remote state - self._state = RemoteState(self._client, str(self._id)) + self._state = RemoteState( + self._client, + str(self._id), + conversation_base_path=self._conversation_base_path, + ) # Add default callback to maintain local event state default_callback = self._state.events.create_default_callback() @@ -855,7 +896,10 @@ def send_message(self, message: str | Message, sender: str | None = None) -> Non if sender is not None: payload["sender"] = sender _send_request( - self._client, "POST", f"/api/conversations/{self._id}/events", json=payload + self._client, + "POST", + f"{self._conversation_base_path}/{self._id}/events", + json=payload, ) @observe(name="conversation.run") @@ -892,7 +936,7 @@ def run( resp = _send_request( self._client, "POST", - f"/api/conversations/{self._id}/run", + f"{self._conversation_base_path}/{self._id}/run", acceptable_status_codes={200, 201, 204, 409}, timeout=30, # Short timeout for trigger request ) @@ -1012,7 +1056,7 @@ def _poll_status_once(self) -> str | None: resp = _send_request( self._client, "GET", - f"/api/conversations/{self._id}", + f"{self._conversation_base_path}/{self._id}", timeout=30, ) info = resp.json() @@ -1081,7 +1125,7 @@ def set_confirmation_policy(self, policy: ConfirmationPolicyBase) -> None: _send_request( self._client, "POST", - f"/api/conversations/{self._id}/confirmation_policy", + f"{self._conversation_base_path}/{self._id}/confirmation_policy", json=payload, ) @@ -1091,7 +1135,7 @@ def set_security_analyzer(self, analyzer: SecurityAnalyzerBase | None) -> None: _send_request( self._client, "POST", - f"/api/conversations/{self._id}/security_analyzer", + f"{self._conversation_base_path}/{self._id}/security_analyzer", json=payload, ) @@ -1100,12 +1144,16 @@ def reject_pending_actions(self, reason: str = "User rejected the action") -> No _send_request( self._client, "POST", - f"/api/conversations/{self._id}/events/respond_to_confirmation", + f"{self._conversation_base_path}/{self._id}/events/respond_to_confirmation", json={"accept": False, "reason": reason}, ) def pause(self) -> None: - _send_request(self._client, "POST", f"/api/conversations/{self._id}/pause") + _send_request( + self._client, + "POST", + f"{self._conversation_base_path}/{self._id}/pause", + ) def update_secrets(self, secrets: Mapping[str, SecretValue]) -> None: # Convert SecretValue to strings for JSON serialization @@ -1121,7 +1169,10 @@ def update_secrets(self, secrets: Mapping[str, SecretValue]) -> None: payload = {"secrets": serializable_secrets} _send_request( - self._client, "POST", f"/api/conversations/{self._id}/secrets", json=payload + self._client, + "POST", + f"{self._conversation_base_path}/{self._id}/secrets", + json=payload, ) def ask_agent(self, question: str) -> str: @@ -1145,7 +1196,7 @@ def ask_agent(self, question: str) -> str: resp = _send_request( self._client, "POST", - f"/api/conversations/{self._id}/ask_agent", + f"{self._conversation_base_path}/{self._id}/ask_agent", json=payload, ) data = resp.json() @@ -1174,7 +1225,7 @@ def generate_title(self, llm: LLM | None = None, max_length: int = 50) -> str: resp = _send_request( self._client, "POST", - f"/api/conversations/{self._id}/generate_title", + f"{self._conversation_base_path}/{self._id}/generate_title", json=payload, ) data = resp.json() @@ -1193,7 +1244,11 @@ def condense(self) -> None: Raises: HTTPError: If the server returns an error (e.g., no condenser configured). """ - _send_request(self._client, "POST", f"/api/conversations/{self._id}/condense") + _send_request( + self._client, + "POST", + f"{self._conversation_base_path}/{self._id}/condense", + ) def execute_tool(self, tool_name: str, action: "Action") -> "Observation": """Execute a tool directly without going through the agent loop. @@ -1241,7 +1296,11 @@ def close(self) -> None: try: # trigger server-side delete_conversation to release resources # like tmux sessions - _send_request(self._client, "DELETE", f"/api/conversations/{self.id}") + _send_request( + self._client, + "DELETE", + f"{self._conversation_base_path}/{self.id}", + ) except Exception: pass diff --git a/tests/agent_server/test_conversation_router.py b/tests/agent_server/test_conversation_router.py index c271f12257..232d7d0283 100644 --- a/tests/agent_server/test_conversation_router.py +++ b/tests/agent_server/test_conversation_router.py @@ -612,6 +612,41 @@ def test_start_conversation_minimal_request( client.app.dependency_overrides.clear() +def test_start_conversation_legacy_request_without_agent_kind( + client, mock_conversation_service, sample_conversation_info +): + """v1 conversation creation should preserve the pre-ACP agent shape.""" + + mock_conversation_service.start_conversation.return_value = ( + sample_conversation_info, + True, + ) + + client.app.dependency_overrides[get_conversation_service] = ( + lambda: mock_conversation_service + ) + + try: + request_data = { + "agent": { + "llm": { + "model": "gpt-4o", + "api_key": "test-key", + "usage_id": "test-llm", + }, + "tools": [{"name": "TerminalTool"}], + }, + "workspace": {"working_dir": "/tmp/test"}, + } + + response = client.post("/api/conversations", json=request_data) + + assert response.status_code == 201 + mock_conversation_service.start_conversation.assert_called_once() + finally: + client.app.dependency_overrides.clear() + + def test_pause_conversation_success( client, mock_conversation_service, sample_conversation_id ): diff --git a/tests/agent_server/test_conversation_router_v2.py b/tests/agent_server/test_conversation_router_v2.py new file mode 100644 index 0000000000..152be787f6 --- /dev/null +++ b/tests/agent_server/test_conversation_router_v2.py @@ -0,0 +1,92 @@ +"""Tests for the v2 ACP-capable conversation router.""" + +from unittest.mock import AsyncMock +from uuid import uuid4 + +import pytest +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from openhands.agent_server.conversation_router_v2 import conversation_router_v2 +from openhands.agent_server.conversation_service import ConversationService +from openhands.agent_server.dependencies import get_conversation_service +from openhands.agent_server.models import ConversationInfoV2 +from openhands.agent_server.utils import utc_now +from openhands.sdk.agent.acp_agent import ACPAgent +from openhands.sdk.conversation.state import ConversationExecutionStatus +from openhands.sdk.workspace import LocalWorkspace + + +@pytest.fixture +def client(): + app = FastAPI() + app.include_router(conversation_router_v2, prefix="/api") + return TestClient(app) + + +@pytest.fixture +def mock_conversation_service(): + return AsyncMock(spec=ConversationService) + + +@pytest.fixture +def sample_conversation_info_v2(): + now = utc_now() + return ConversationInfoV2( + id=uuid4(), + agent=ACPAgent(acp_command=["echo", "test"]), + workspace=LocalWorkspace(working_dir="/tmp/test"), + execution_status=ConversationExecutionStatus.IDLE, + title="ACP Conversation", + created_at=now, + updated_at=now, + ) + + +def test_start_conversation_v2_accepts_acp_agent( + client, mock_conversation_service, sample_conversation_info_v2 +): + mock_conversation_service.start_conversation_v2.return_value = ( + sample_conversation_info_v2, + True, + ) + client.app.dependency_overrides[get_conversation_service] = ( + lambda: mock_conversation_service + ) + + try: + response = client.post( + "/api/v2/conversations", + json={ + "agent": { + "kind": "ACPAgent", + "acp_command": ["echo", "test"], + }, + "workspace": {"working_dir": "/tmp/test"}, + }, + ) + + assert response.status_code == 201 + assert response.json()["agent"]["kind"] == "ACPAgent" + mock_conversation_service.start_conversation_v2.assert_called_once() + finally: + client.app.dependency_overrides.clear() + + +def test_get_conversation_v2_returns_acp_agent( + client, mock_conversation_service, sample_conversation_info_v2 +): + mock_conversation_service.get_conversation_v2.return_value = ( + sample_conversation_info_v2 + ) + client.app.dependency_overrides[get_conversation_service] = ( + lambda: mock_conversation_service + ) + + try: + response = client.get(f"/api/v2/conversations/{sample_conversation_info_v2.id}") + + assert response.status_code == 200 + assert response.json()["agent"]["kind"] == "ACPAgent" + finally: + client.app.dependency_overrides.clear() diff --git a/tests/agent_server/test_openapi_discriminator.py b/tests/agent_server/test_openapi_discriminator.py index 9d40af385c..cef7da5a5b 100644 --- a/tests/agent_server/test_openapi_discriminator.py +++ b/tests/agent_server/test_openapi_discriminator.py @@ -164,3 +164,23 @@ def test_action_variants_have_proper_schemas(client): assert "title" in type_schema, ( f"{action_type} should have title for better docs" ) + + +def test_conversation_contracts_are_versioned(client): + """v1 conversations stay Agent-only while v2 exposes ACP support.""" + response = client.get("/openapi.json") + assert response.status_code == 200 + + schemas = response.json()["components"]["schemas"] + + v1_request = schemas["StartConversationRequest"] + assert ( + v1_request["properties"]["agent"]["$ref"] == "#/components/schemas/Agent-Input" + ) + + v2_request = schemas["StartConversationRequestV2"] + agent_schema = v2_request["properties"]["agent"] + assert "oneOf" in agent_schema + refs = {variant["$ref"] for variant in agent_schema["oneOf"]} + assert "#/components/schemas/Agent-Input" in refs + assert "#/components/schemas/ACPAgent-Input" in refs diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index 2d97d4babc..81c3b43848 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -9,13 +9,19 @@ import pytest -from openhands.sdk.agent.acp_agent import ACPAgent, _OpenHandsACPBridge +from openhands.sdk.agent.acp_agent import ( + ACPAgent, + _OpenHandsACPBridge, + _resolve_bypass_mode, + _select_auth_method, +) from openhands.sdk.agent.base import AgentBase from openhands.sdk.conversation.state import ( ConversationExecutionStatus, ConversationState, ) from openhands.sdk.event import ACPToolCallEvent, MessageEvent, SystemPromptEvent +from openhands.sdk.event.conversation_error import ConversationErrorEvent from openhands.sdk.llm import Message, TextContent from openhands.sdk.workspace.local import LocalWorkspace @@ -62,8 +68,8 @@ def test_requires_acp_command(self): ACPAgent() # type: ignore[call-arg] def test_acp_command_stored(self): - agent = ACPAgent(acp_command=["npx", "-y", "claude-code-acp"]) - assert agent.acp_command == ["npx", "-y", "claude-code-acp"] + agent = ACPAgent(acp_command=["npx", "-y", "claude-agent-acp"]) + assert agent.acp_command == ["npx", "-y", "claude-agent-acp"] def test_acp_args_default_empty(self): agent = _make_agent() @@ -102,7 +108,7 @@ def test_kind_is_acp_agent(self): def test_roundtrip_serialization(self): agent = ACPAgent( - acp_command=["npx", "-y", "claude-code-acp"], + acp_command=["npx", "-y", "claude-agent-acp"], acp_args=["--verbose"], acp_env={"FOO": "bar"}, ) @@ -294,7 +300,7 @@ def test_step_emits_message_event(self, tmp_path): agent._conn = MagicMock() agent._session_id = "test-session" - def _fake_run_async(_coro): + def _fake_run_async(_coro, **_kwargs): mock_client.accumulated_text.append("The answer is 4") mock_executor = MagicMock() @@ -303,7 +309,9 @@ def _fake_run_async(_coro): agent.step(conversation, on_event=events.append) - assert len(events) == 1 + # step() emits MessageEvent + ActionEvent(FinishAction) + # + ObservationEvent(FinishObservation) + assert len(events) == 3 assert isinstance(events[0], MessageEvent) assert events[0].source == "agent" content_block = events[0].llm_message.content[0] @@ -320,7 +328,7 @@ def test_step_includes_reasoning(self, tmp_path): agent._conn = MagicMock() agent._session_id = "test-session" - def _fake_run_async(_coro): + def _fake_run_async(_coro, **_kwargs): mock_client.accumulated_text.append("4") mock_client.accumulated_thoughts.append("I need to add 2+2") @@ -342,7 +350,7 @@ def test_step_sets_finished(self, tmp_path): agent._conn = MagicMock() agent._session_id = "test-session" - def _fake_run_async(_coro): + def _fake_run_async(_coro, **_kwargs): mock_client.accumulated_text.append("done") mock_executor = MagicMock() @@ -383,13 +391,19 @@ def test_step_error_sets_error_status(self, tmp_path): mock_executor.run_async = MagicMock(side_effect=RuntimeError("boom")) agent._executor = mock_executor - agent.step(conversation, on_event=events.append) + with pytest.raises(RuntimeError, match="boom"): + agent.step(conversation, on_event=events.append) assert conversation.state.execution_status == ConversationExecutionStatus.ERROR - assert len(events) == 1 + assert len(events) == 2 + # First event: MessageEvent with the error text content_block = events[0].llm_message.content[0] assert isinstance(content_block, TextContent) assert "ACP error: boom" in content_block.text + # Second event: ConversationErrorEvent with error detail + assert isinstance(events[1], ConversationErrorEvent) + assert events[1].code == "ACPPromptError" + assert "boom" in events[1].detail def test_step_no_response_text_fallback(self, tmp_path): agent = _make_agent() @@ -403,7 +417,7 @@ def test_step_no_response_text_fallback(self, tmp_path): agent._session_id = "test-session" mock_executor = MagicMock() - mock_executor.run_async = lambda _coro: None + mock_executor.run_async = lambda _coro, **_kwargs: None agent._executor = mock_executor agent.step(conversation, on_event=events.append) @@ -421,7 +435,7 @@ def test_step_passes_on_token(self, tmp_path): agent._conn = MagicMock() agent._session_id = "test-session" - def _fake_run_async(_coro): + def _fake_run_async(_coro, **_kwargs): mock_client.accumulated_text.append("ok") mock_executor = MagicMock() @@ -611,7 +625,7 @@ def test_step_records_token_usage(self, tmp_path): mock_response = MagicMock() mock_response.usage = mock_usage - def _fake_run_async(_coro): + def _fake_run_async(_coro, **_kwargs): mock_client.accumulated_text.append("response text") return mock_response @@ -645,7 +659,7 @@ def test_step_handles_no_usage(self, tmp_path): mock_response = MagicMock() mock_response.usage = None - def _fake_run_async(_coro): + def _fake_run_async(_coro, **_kwargs): mock_client.accumulated_text.append("response") return mock_response @@ -739,7 +753,7 @@ def test_stats_callback_invoked(self, tmp_path): mock_response = MagicMock() mock_response.usage = None - def _fake_run_async(_coro): + def _fake_run_async(_coro, **_kwargs): mock_client.accumulated_text.append("ok") return mock_response @@ -940,7 +954,7 @@ def test_step_emits_tool_call_events_before_message(self, tmp_path): agent._conn = MagicMock() agent._session_id = "test-session" - def _fake_run_async(_coro): + def _fake_run_async(_coro, **_kwargs): mock_client.accumulated_text.append("done") mock_client.accumulated_tool_calls.extend( [ @@ -970,7 +984,8 @@ def _fake_run_async(_coro): agent.step(conversation, on_event=events.append) # Should be: 2 tool call events + 1 message event - assert len(events) == 3 + # + finish action + finish observation + assert len(events) == 5 assert isinstance(events[0], ACPToolCallEvent) assert isinstance(events[1], ACPToolCallEvent) assert isinstance(events[2], MessageEvent) @@ -999,7 +1014,7 @@ def test_step_emits_no_tool_call_events_when_none(self, tmp_path): agent._conn = MagicMock() agent._session_id = "test-session" - def _fake_run_async(_coro): + def _fake_run_async(_coro, **_kwargs): mock_client.accumulated_text.append("no tools used") mock_executor = MagicMock() @@ -1008,7 +1023,8 @@ def _fake_run_async(_coro): agent.step(conversation, on_event=events.append) - assert len(events) == 1 + # MessageEvent + ActionEvent(FinishAction) + ObservationEvent(FinishObservation) + assert len(events) == 3 assert isinstance(events[0], MessageEvent) def test_tool_call_events_cleared_between_turns(self, tmp_path): @@ -1034,7 +1050,7 @@ def test_tool_call_events_cleared_between_turns(self, tmp_path): conversation = self._make_conversation_with_message(tmp_path) events: list = [] - def _fake_run_async(_coro): + def _fake_run_async(_coro, **_kwargs): # After reset, accumulated_tool_calls should be empty # Only add text so step() succeeds mock_client.accumulated_text.append("response") @@ -1046,8 +1062,9 @@ def _fake_run_async(_coro): # step() calls reset() which should clear old tool calls agent.step(conversation, on_event=events.append) - # Only the MessageEvent should appear — the old tool call was cleared - assert len(events) == 1 + # Only the MessageEvent + FinishAction + FinishObservation should appear — + # the old tool call was cleared by reset() + assert len(events) == 3 assert isinstance(events[0], MessageEvent) @@ -1094,7 +1111,7 @@ async def _fake_prompt(*args, **kwargs): mock_client._fork_accumulated_text.extend(["Hello", " world"]) return mock_prompt_response - def _fake_run_async(coro_fn): + def _fake_run_async(coro_fn, **_kwargs): """Simulate the async execution synchronously.""" loop = asyncio.new_event_loop() try: @@ -1139,7 +1156,7 @@ async def _fake_prompt(*args, **kwargs): mock_client._fork_accumulated_text.append("response") return mock_prompt_response - def _fake_run_async(coro_fn): + def _fake_run_async(coro_fn, **_kwargs): loop = asyncio.new_event_loop() try: agent._conn.fork_session = AsyncMock(return_value=mock_fork_response) @@ -1183,7 +1200,7 @@ async def _fake_prompt(*args, **kwargs): mock_client._fork_accumulated_text.append("ok") return mock_prompt_response - def _fake_run_async(coro_fn): + def _fake_run_async(coro_fn, **_kwargs): loop = asyncio.new_event_loop() try: agent._conn.fork_session = AsyncMock(return_value=mock_fork_response) @@ -1262,3 +1279,263 @@ async def test_no_fork_normal_routing(self): assert client.accumulated_text == ["normal text"] assert client._fork_accumulated_text == [] + + +# --------------------------------------------------------------------------- +# _resolve_bypass_mode +# --------------------------------------------------------------------------- + + +class TestResolveBypassMode: + def test_claude_agent(self): + assert _resolve_bypass_mode("claude-agent-acp") == "bypassPermissions" + + def test_claude_agent_with_scope(self): + assert ( + _resolve_bypass_mode("@zed-industries/claude-agent-acp") + == "bypassPermissions" + ) + + def test_codex_acp(self): + assert _resolve_bypass_mode("codex-acp") == "full-access" + + def test_codex_acp_with_version(self): + assert _resolve_bypass_mode("Codex-ACP v0.9.2") == "full-access" + + def test_unknown_server_defaults_to_full_access(self): + assert _resolve_bypass_mode("some-other-agent") == "full-access" + + def test_empty_name_defaults_to_full_access(self): + assert _resolve_bypass_mode("") == "full-access" + + +# --------------------------------------------------------------------------- +# acp_session_mode field +# --------------------------------------------------------------------------- + + +# --------------------------------------------------------------------------- +# _select_auth_method +# --------------------------------------------------------------------------- + + +class TestSelectAuthMethod: + """Test auto-detection of ACP auth method from env vars.""" + + @staticmethod + def _make_auth_method(method_id: str) -> MagicMock: + m = MagicMock() + m.id = method_id + return m + + def test_openai_api_key(self): + methods = [ + self._make_auth_method("chatgpt"), + self._make_auth_method("codex-api-key"), + self._make_auth_method("openai-api-key"), + ] + env = {"OPENAI_API_KEY": "sk-test"} + assert _select_auth_method(methods, env) == "openai-api-key" + + def test_codex_api_key_preferred(self): + """CODEX_API_KEY is checked first (appears first in the map).""" + methods = [ + self._make_auth_method("codex-api-key"), + self._make_auth_method("openai-api-key"), + ] + env = {"CODEX_API_KEY": "key1", "OPENAI_API_KEY": "key2"} + assert _select_auth_method(methods, env) == "codex-api-key" + + def test_no_matching_env_var(self): + methods = [ + self._make_auth_method("chatgpt"), + self._make_auth_method("openai-api-key"), + ] + env = {"UNRELATED": "value"} + assert _select_auth_method(methods, env) is None + + def test_empty_auth_methods(self): + assert _select_auth_method([], {}) is None + + def test_method_not_in_server_list(self): + """Even if env var is set, method must be offered by server.""" + methods = [self._make_auth_method("chatgpt")] + env = {"OPENAI_API_KEY": "sk-test"} + assert _select_auth_method(methods, env) is None + + +# --------------------------------------------------------------------------- +# acp_session_mode field +# --------------------------------------------------------------------------- + + +class TestACPSessionMode: + def test_default_is_none(self): + agent = _make_agent() + assert agent.acp_session_mode is None + + def test_can_set_explicit_mode(self): + agent = ACPAgent(acp_command=["echo"], acp_session_mode="custom-mode") + assert agent.acp_session_mode == "custom-mode" + + def test_serialization_roundtrip(self): + agent = ACPAgent( + acp_command=["codex-acp"], + acp_session_mode="full-access", + ) + dumped = agent.model_dump_json() + restored = AgentBase.model_validate_json(dumped) + assert isinstance(restored, ACPAgent) + assert restored.acp_session_mode == "full-access" + + +# --------------------------------------------------------------------------- +# Connection retry logic +# --------------------------------------------------------------------------- + + +class TestACPPromptRetry: + """Test retry logic for ACP prompt failures.""" + + def _make_conversation_with_message(self, tmp_path, text="Hello"): + """Create a mock conversation with a user message.""" + state = _make_state(tmp_path) + state.events.append( + SystemPromptEvent( + source="agent", + system_prompt=TextContent(text="ACP-managed agent"), + tools=[], + ) + ) + state.events.append( + MessageEvent( + source="user", + llm_message=Message(role="user", content=[TextContent(text=text)]), + ) + ) + + conversation = MagicMock() + conversation.state = state + return conversation + + def test_retry_on_connection_error_then_success(self, tmp_path): + """Retry succeeds after transient connection error.""" + agent = _make_agent() + conversation = self._make_conversation_with_message(tmp_path) + events: list = [] + + mock_client = _OpenHandsACPBridge() + agent._client = mock_client + agent._conn = MagicMock() + agent._session_id = "test-session" + + call_count = 0 + + def _fake_run_async(_coro, **_kwargs): + nonlocal call_count + call_count += 1 + if call_count == 1: + raise ConnectionError("Connection reset by peer") + # Second call succeeds - must populate text and return a response + mock_client.accumulated_text.append("Success after retry") + # Return a mock PromptResponse (can be MagicMock since we only check usage) + return MagicMock(usage=None) + + mock_executor = MagicMock() + mock_executor.run_async = _fake_run_async + agent._executor = mock_executor + + # Patch sleep to avoid actual delays in tests + with patch("openhands.sdk.agent.acp_agent.time.sleep"): + agent.step(conversation, on_event=events.append) + + assert call_count == 2 # First failed, second succeeded + assert ( + conversation.state.execution_status == ConversationExecutionStatus.FINISHED + ) + assert len(events) == 3 # MessageEvent, ActionEvent, ObservationEvent + assert "Success after retry" in events[0].llm_message.content[0].text + + def test_no_retry_on_non_connection_error(self, tmp_path): + """Non-connection errors (e.g., RuntimeError) fail immediately without retry.""" + agent = _make_agent() + conversation = self._make_conversation_with_message(tmp_path) + events: list = [] + + mock_client = _OpenHandsACPBridge() + agent._client = mock_client + agent._conn = MagicMock() + agent._session_id = "test-session" + + call_count = 0 + + def _fake_run_async(_coro, **_kwargs): + nonlocal call_count + call_count += 1 + raise RuntimeError("Some application error") + + mock_executor = MagicMock() + mock_executor.run_async = _fake_run_async + agent._executor = mock_executor + + with pytest.raises(RuntimeError, match="Some application error"): + agent.step(conversation, on_event=events.append) + + assert call_count == 1 # No retry attempted + assert conversation.state.execution_status == ConversationExecutionStatus.ERROR + + def test_no_retry_on_timeout(self, tmp_path): + """Timeout errors are not retried (handled separately).""" + agent = _make_agent() + conversation = self._make_conversation_with_message(tmp_path) + + mock_client = _OpenHandsACPBridge() + agent._client = mock_client + agent._conn = MagicMock() + agent._session_id = "test-session" + + call_count = 0 + + def _fake_run_async(_coro, **_kwargs): + nonlocal call_count + call_count += 1 + raise TimeoutError("ACP prompt timed out") + + mock_executor = MagicMock() + mock_executor.run_async = _fake_run_async + agent._executor = mock_executor + + agent.step(conversation, on_event=lambda _: None) + + assert call_count == 1 # No retry for timeout + assert conversation.state.execution_status == ConversationExecutionStatus.ERROR + + def test_max_retries_exceeded(self, tmp_path): + """Error raised after max retries exhausted.""" + agent = _make_agent() + conversation = self._make_conversation_with_message(tmp_path) + events: list = [] + + mock_client = _OpenHandsACPBridge() + agent._client = mock_client + agent._conn = MagicMock() + agent._session_id = "test-session" + + call_count = 0 + + def _fake_run_async(_coro, **_kwargs): + nonlocal call_count + call_count += 1 + raise ConnectionError("Persistent connection failure") + + mock_executor = MagicMock() + mock_executor.run_async = _fake_run_async + agent._executor = mock_executor + + with patch("openhands.sdk.agent.acp_agent.time.sleep"): + with pytest.raises(ConnectionError, match="Persistent connection failure"): + agent.step(conversation, on_event=events.append) + + # Default max retries is 3, so 4 total attempts (1 initial + 3 retries) + assert call_count == 4 + assert conversation.state.execution_status == ConversationExecutionStatus.ERROR diff --git a/tests/sdk/conversation/remote/test_remote_conversation.py b/tests/sdk/conversation/remote/test_remote_conversation.py index 6f859ca6e9..9646b7144b 100644 --- a/tests/sdk/conversation/remote/test_remote_conversation.py +++ b/tests/sdk/conversation/remote/test_remote_conversation.py @@ -8,6 +8,7 @@ from pydantic import SecretStr from openhands.sdk.agent import Agent +from openhands.sdk.agent.acp_agent import ACPAgent from openhands.sdk.conversation.exceptions import ConversationRunError from openhands.sdk.conversation.impl.remote_conversation import RemoteConversation from openhands.sdk.conversation.secret_registry import SecretValue @@ -170,6 +171,64 @@ def test_remote_conversation_initialization_new_conversation(self, mock_ws_clien "to fetch initial events" ) + @patch( + "openhands.sdk.conversation.impl.remote_conversation.WebSocketCallbackClient" + ) + def test_acp_remote_conversation_uses_v2_conversation_contract( + self, mock_ws_client + ): + acp_agent = ACPAgent(acp_command=["echo", "test"]) + conversation_id = str(uuid.uuid4()) + mock_client_instance = Mock() + self.workspace._client = mock_client_instance + + mock_conv_response = self.create_mock_conversation_response(conversation_id) + mock_events_response = self.create_mock_events_response() + + def request_side_effect(method, url, **kwargs): + if method == "POST" and url == "/api/v2/conversations": + return mock_conv_response + if method == "GET" and "/api/v2/conversations/" in url and "/events" in url: + return mock_events_response + if method == "GET" and url.startswith("/api/v2/conversations/"): + response = Mock() + response.status_code = 200 + response.raise_for_status.return_value = None + conv_info = mock_conv_response.json.return_value.copy() + conv_info["execution_status"] = "finished" + conv_info["agent"] = { + "kind": "ACPAgent", + "acp_command": ["echo", "test"], + } + response.json.return_value = conv_info + return response + response = Mock() + response.status_code = 200 + response.raise_for_status.return_value = None + response.json.return_value = {} + return response + + mock_client_instance.request.side_effect = request_side_effect + + mock_ws_instance = Mock() + mock_ws_client.return_value = mock_ws_instance + + RemoteConversation(agent=acp_agent, workspace=self.workspace) + + post_calls = [ + call + for call in mock_client_instance.request.call_args_list + if call[0][0] == "POST" and call[0][1] == "/api/v2/conversations" + ] + assert len(post_calls) == 1 + + get_events_calls = [ + call + for call in mock_client_instance.request.call_args_list + if call[0][0] == "GET" and "/api/v2/conversations/" in call[0][1] + ] + assert len(get_events_calls) >= 1 + @patch( "openhands.sdk.conversation.impl.remote_conversation.WebSocketCallbackClient" ) From 4f39a39dc844bce68685a3c9352ecc0bd5d384b1 Mon Sep 17 00:00:00 2001 From: Simon Rosenberg Date: Mon, 16 Mar 2026 11:44:33 -0300 Subject: [PATCH 2/6] fix(sdk): restore default remote conversation paths Co-authored-by: openhands --- .../sdk/conversation/impl/remote_conversation.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/openhands-sdk/openhands/sdk/conversation/impl/remote_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/remote_conversation.py index 44aeba305b..f858081e99 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/remote_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/remote_conversation.py @@ -243,7 +243,10 @@ class RemoteEventsList(EventsListBase): _lock: threading.RLock def __init__( - self, client: httpx.Client, conversation_id: str, conversation_base_path: str + self, + client: httpx.Client, + conversation_id: str, + conversation_base_path: str = V1_CONVERSATIONS_PATH, ): self._client = client self._conversation_id = conversation_id @@ -406,7 +409,10 @@ class RemoteState(ConversationStateProtocol): _lock: threading.RLock def __init__( - self, client: httpx.Client, conversation_id: str, conversation_base_path: str + self, + client: httpx.Client, + conversation_id: str, + conversation_base_path: str = V1_CONVERSATIONS_PATH, ): self._client = client self._conversation_id = conversation_id From be76de3fe811ce70b406ef4f56bea29fe125b3ed Mon Sep 17 00:00:00 2001 From: Simon Rosenberg Date: Mon, 16 Mar 2026 12:27:49 -0300 Subject: [PATCH 3/6] refactor(agent-server): scope ACP API to affected endpoints Co-authored-by: openhands --- .../openhands/agent_server/api.py | 4 +- .../agent_server/conversation_router_acp.py | 121 +++++++ .../agent_server/conversation_router_v2.py | 324 ------------------ .../agent_server/conversation_service.py | 91 +++-- .../openhands/agent_server/models.py | 23 +- .../conversation/impl/remote_conversation.py | 76 ++-- .../test_conversation_router_acp.py | 137 ++++++++ .../test_conversation_router_v2.py | 92 ----- .../test_openapi_discriminator.py | 26 +- .../remote/test_remote_conversation.py | 12 +- 10 files changed, 378 insertions(+), 528 deletions(-) create mode 100644 openhands-agent-server/openhands/agent_server/conversation_router_acp.py delete mode 100644 openhands-agent-server/openhands/agent_server/conversation_router_v2.py create mode 100644 tests/agent_server/test_conversation_router_acp.py delete mode 100644 tests/agent_server/test_conversation_router_v2.py diff --git a/openhands-agent-server/openhands/agent_server/api.py b/openhands-agent-server/openhands/agent_server/api.py index e4f09069fd..255d53a608 100644 --- a/openhands-agent-server/openhands/agent_server/api.py +++ b/openhands-agent-server/openhands/agent_server/api.py @@ -15,7 +15,7 @@ get_default_config, ) from openhands.agent_server.conversation_router import conversation_router -from openhands.agent_server.conversation_router_v2 import conversation_router_v2 +from openhands.agent_server.conversation_router_acp import conversation_router_acp from openhands.agent_server.conversation_service import ( get_default_conversation_service, ) @@ -200,7 +200,7 @@ def _add_api_routes(app: FastAPI, config: Config) -> None: api_router = APIRouter(prefix="/api", dependencies=dependencies) api_router.include_router(event_router) api_router.include_router(conversation_router) - api_router.include_router(conversation_router_v2) + api_router.include_router(conversation_router_acp) api_router.include_router(tool_router) api_router.include_router(bash_router) api_router.include_router(git_router) diff --git a/openhands-agent-server/openhands/agent_server/conversation_router_acp.py b/openhands-agent-server/openhands/agent_server/conversation_router_acp.py new file mode 100644 index 0000000000..75a61be689 --- /dev/null +++ b/openhands-agent-server/openhands/agent_server/conversation_router_acp.py @@ -0,0 +1,121 @@ +"""ACP-capable conversation routes for the schema-sensitive endpoints.""" + +from typing import Annotated +from uuid import UUID + +from fastapi import APIRouter, Body, Depends, HTTPException, Query, Response, status +from pydantic import SecretStr + +from openhands.agent_server.conversation_service import ConversationService +from openhands.agent_server.dependencies import get_conversation_service +from openhands.agent_server.models import ( + ACPConversationInfo, + ACPConversationPage, + ConversationSortOrder, + SendMessageRequest, + StartACPConversationRequest, +) +from openhands.sdk import LLM, Agent, TextContent +from openhands.sdk.agent.acp_agent import ACPAgent +from openhands.sdk.conversation.state import ConversationExecutionStatus +from openhands.sdk.workspace import LocalWorkspace +from openhands.tools.preset.default import get_default_tools + + +conversation_router_acp = APIRouter( + prefix="/acp/conversations", + tags=["ACP Conversations"], +) + +START_ACP_CONVERSATION_EXAMPLES = [ + StartACPConversationRequest( + agent=Agent( + llm=LLM( + usage_id="your-llm-service", + model="your-model-provider/your-model-name", + api_key=SecretStr("your-api-key-here"), + ), + tools=get_default_tools(enable_browser=True), + ), + workspace=LocalWorkspace(working_dir="workspace/project"), + initial_message=SendMessageRequest( + role="user", content=[TextContent(text="Flip a coin!")] + ), + ).model_dump(exclude_defaults=True, mode="json"), + StartACPConversationRequest( + agent=ACPAgent(acp_command=["npx", "-y", "claude-agent-acp"]), + workspace=LocalWorkspace(working_dir="workspace/project"), + initial_message=SendMessageRequest( + role="user", + content=[TextContent(text="Inspect the repository and summarize it.")], + ), + ).model_dump(exclude_defaults=True, mode="json"), +] + + +@conversation_router_acp.get("/search") +async def search_acp_conversations( + page_id: Annotated[ + str | None, + Query(title="Optional next_page_id from the previously returned page"), + ] = None, + limit: Annotated[ + int, + Query(title="The max number of results in the page", gt=0, lte=100), + ] = 100, + status: Annotated[ + ConversationExecutionStatus | None, + Query(title="Optional filter by conversation execution status"), + ] = None, + sort_order: Annotated[ + ConversationSortOrder, + Query(title="Sort order for conversations"), + ] = ConversationSortOrder.CREATED_AT_DESC, + conversation_service: ConversationService = Depends(get_conversation_service), +) -> ACPConversationPage: + """Search conversations using the ACP-capable contract.""" + assert limit > 0 + assert limit <= 100 + return await conversation_service.search_acp_conversations( + page_id, limit, status, sort_order + ) + + +@conversation_router_acp.get( + "/{conversation_id}", + responses={404: {"description": "Item not found"}}, +) +async def get_acp_conversation( + conversation_id: UUID, + conversation_service: ConversationService = Depends(get_conversation_service), +) -> ACPConversationInfo: + """Get a conversation using the ACP-capable contract.""" + conversation = await conversation_service.get_acp_conversation(conversation_id) + if conversation is None: + raise HTTPException(status.HTTP_404_NOT_FOUND) + return conversation + + +@conversation_router_acp.get("") +async def batch_get_acp_conversations( + ids: Annotated[list[UUID], Query()], + conversation_service: ConversationService = Depends(get_conversation_service), +) -> list[ACPConversationInfo | None]: + """Batch get conversations using the ACP-capable contract.""" + assert len(ids) < 100 + return await conversation_service.batch_get_acp_conversations(ids) + + +@conversation_router_acp.post("") +async def start_acp_conversation( + request: Annotated[ + StartACPConversationRequest, + Body(examples=START_ACP_CONVERSATION_EXAMPLES), + ], + response: Response, + conversation_service: ConversationService = Depends(get_conversation_service), +) -> ACPConversationInfo: + """Start a conversation using the ACP-capable contract.""" + info, is_new = await conversation_service.start_acp_conversation(request) + response.status_code = status.HTTP_201_CREATED if is_new else status.HTTP_200_OK + return info diff --git a/openhands-agent-server/openhands/agent_server/conversation_router_v2.py b/openhands-agent-server/openhands/agent_server/conversation_router_v2.py deleted file mode 100644 index a7f79a1524..0000000000 --- a/openhands-agent-server/openhands/agent_server/conversation_router_v2.py +++ /dev/null @@ -1,324 +0,0 @@ -"""Versioned conversation router with ACP-capable agent contract.""" - -from typing import Annotated -from uuid import UUID - -from fastapi import APIRouter, Body, Depends, HTTPException, Query, Response, status -from pydantic import SecretStr - -from openhands.agent_server.conversation_service import ConversationService -from openhands.agent_server.dependencies import get_conversation_service -from openhands.agent_server.models import ( - AskAgentRequest, - AskAgentResponse, - ConversationInfoV2, - ConversationPageV2, - ConversationSortOrder, - GenerateTitleRequest, - GenerateTitleResponse, - SendMessageRequest, - SetConfirmationPolicyRequest, - SetSecurityAnalyzerRequest, - StartConversationRequestV2, - Success, - UpdateConversationRequest, - UpdateSecretsRequest, -) -from openhands.sdk import LLM, Agent, TextContent -from openhands.sdk.agent.acp_agent import ACPAgent -from openhands.sdk.conversation.state import ConversationExecutionStatus -from openhands.sdk.workspace import LocalWorkspace -from openhands.tools.preset.default import get_default_tools - - -conversation_router_v2 = APIRouter( - prefix="/v2/conversations", tags=["Conversations V2"] -) - -START_CONVERSATION_V2_EXAMPLES = [ - StartConversationRequestV2( - agent=Agent( - llm=LLM( - usage_id="your-llm-service", - model="your-model-provider/your-model-name", - api_key=SecretStr("your-api-key-here"), - ), - tools=get_default_tools(enable_browser=True), - ), - workspace=LocalWorkspace(working_dir="workspace/project"), - initial_message=SendMessageRequest( - role="user", content=[TextContent(text="Flip a coin!")] - ), - ).model_dump(exclude_defaults=True, mode="json"), - StartConversationRequestV2( - agent=ACPAgent(acp_command=["npx", "-y", "claude-agent-acp"]), - workspace=LocalWorkspace(working_dir="workspace/project"), - initial_message=SendMessageRequest( - role="user", - content=[TextContent(text="Inspect the repository and summarize it.")], - ), - ).model_dump(exclude_defaults=True, mode="json"), -] - - -@conversation_router_v2.get("/search") -async def search_conversations( - page_id: Annotated[ - str | None, - Query(title="Optional next_page_id from the previously returned page"), - ] = None, - limit: Annotated[ - int, - Query(title="The max number of results in the page", gt=0, lte=100), - ] = 100, - status: Annotated[ - ConversationExecutionStatus | None, - Query(title="Optional filter by conversation execution status"), - ] = None, - sort_order: Annotated[ - ConversationSortOrder, - Query(title="Sort order for conversations"), - ] = ConversationSortOrder.CREATED_AT_DESC, - conversation_service: ConversationService = Depends(get_conversation_service), -) -> ConversationPageV2: - """Search / List conversations using the v2 ACP-capable contract.""" - assert limit > 0 - assert limit <= 100 - return await conversation_service.search_conversations_v2( - page_id, limit, status, sort_order - ) - - -@conversation_router_v2.get("/count") -async def count_conversations( - status: Annotated[ - ConversationExecutionStatus | None, - Query(title="Optional filter by conversation execution status"), - ] = None, - conversation_service: ConversationService = Depends(get_conversation_service), -) -> int: - """Count conversations matching the given filters.""" - return await conversation_service.count_conversations_v2(status) - - -@conversation_router_v2.get( - "/{conversation_id}", responses={404: {"description": "Item not found"}} -) -async def get_conversation( - conversation_id: UUID, - conversation_service: ConversationService = Depends(get_conversation_service), -) -> ConversationInfoV2: - """Given an id, get a conversation using the v2 ACP-capable contract.""" - conversation = await conversation_service.get_conversation_v2(conversation_id) - if conversation is None: - raise HTTPException(status.HTTP_404_NOT_FOUND) - return conversation - - -@conversation_router_v2.get("") -async def batch_get_conversations( - ids: Annotated[list[UUID], Query()], - conversation_service: ConversationService = Depends(get_conversation_service), -) -> list[ConversationInfoV2 | None]: - """Get a batch of conversations given their ids. - - Returns null for any missing item. - """ - assert len(ids) < 100 - return await conversation_service.batch_get_conversations_v2(ids) - - -@conversation_router_v2.post("") -async def start_conversation( - request: Annotated[ - StartConversationRequestV2, Body(examples=START_CONVERSATION_V2_EXAMPLES) - ], - response: Response, - conversation_service: ConversationService = Depends(get_conversation_service), -) -> ConversationInfoV2: - """Start a conversation using the ACP-capable v2 contract.""" - info, is_new = await conversation_service.start_conversation_v2(request) - response.status_code = status.HTTP_201_CREATED if is_new else status.HTTP_200_OK - return info - - -@conversation_router_v2.post( - "/{conversation_id}/pause", responses={404: {"description": "Item not found"}} -) -async def pause_conversation( - conversation_id: UUID, - conversation_service: ConversationService = Depends(get_conversation_service), -) -> Success: - """Pause a conversation, allowing it to be resumed later.""" - paused = await conversation_service.pause_conversation(conversation_id) - if not paused: - raise HTTPException(status.HTTP_400_BAD_REQUEST) - return Success() - - -@conversation_router_v2.delete( - "/{conversation_id}", responses={404: {"description": "Item not found"}} -) -async def delete_conversation( - conversation_id: UUID, - conversation_service: ConversationService = Depends(get_conversation_service), -) -> Success: - """Permanently delete a conversation.""" - deleted = await conversation_service.delete_conversation(conversation_id) - if not deleted: - raise HTTPException(status.HTTP_400_BAD_REQUEST) - return Success() - - -@conversation_router_v2.post( - "/{conversation_id}/run", - responses={ - 404: {"description": "Item not found"}, - 409: {"description": "Conversation is already running"}, - }, -) -async def run_conversation( - conversation_id: UUID, - conversation_service: ConversationService = Depends(get_conversation_service), -) -> Success: - """Start running the conversation in the background.""" - event_service = await conversation_service.get_event_service(conversation_id) - if event_service is None: - raise HTTPException(status.HTTP_404_NOT_FOUND) - - try: - await event_service.run() - except ValueError as e: - if str(e) == "conversation_already_running": - raise HTTPException( - status_code=status.HTTP_409_CONFLICT, - detail=( - "Conversation already running. Wait for completion or pause first." - ), - ) - raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail=str(e)) - - return Success() - - -@conversation_router_v2.post( - "/{conversation_id}/secrets", responses={404: {"description": "Item not found"}} -) -async def update_conversation_secrets( - conversation_id: UUID, - request: UpdateSecretsRequest, - conversation_service: ConversationService = Depends(get_conversation_service), -) -> Success: - """Update secrets for a conversation.""" - event_service = await conversation_service.get_event_service(conversation_id) - if event_service is None: - raise HTTPException(status.HTTP_404_NOT_FOUND) - from typing import cast - - from openhands.sdk.conversation.secret_registry import SecretValue - - secrets = cast(dict[str, SecretValue], request.secrets) - await event_service.update_secrets(secrets) - return Success() - - -@conversation_router_v2.post( - "/{conversation_id}/confirmation_policy", - responses={404: {"description": "Item not found"}}, -) -async def set_conversation_confirmation_policy( - conversation_id: UUID, - request: SetConfirmationPolicyRequest, - conversation_service: ConversationService = Depends(get_conversation_service), -) -> Success: - """Set the confirmation policy for a conversation.""" - event_service = await conversation_service.get_event_service(conversation_id) - if event_service is None: - raise HTTPException(status.HTTP_404_NOT_FOUND) - await event_service.set_confirmation_policy(request.policy) - return Success() - - -@conversation_router_v2.post( - "/{conversation_id}/security_analyzer", - responses={404: {"description": "Item not found"}}, -) -async def set_conversation_security_analyzer( - conversation_id: UUID, - request: SetSecurityAnalyzerRequest, - conversation_service: ConversationService = Depends(get_conversation_service), -) -> Success: - """Set the security analyzer for a conversation.""" - event_service = await conversation_service.get_event_service(conversation_id) - if event_service is None: - raise HTTPException(status.HTTP_404_NOT_FOUND) - await event_service.set_security_analyzer(request.security_analyzer) - return Success() - - -@conversation_router_v2.patch( - "/{conversation_id}", responses={404: {"description": "Item not found"}} -) -async def update_conversation( - conversation_id: UUID, - request: UpdateConversationRequest, - conversation_service: ConversationService = Depends(get_conversation_service), -) -> Success: - """Update conversation metadata.""" - updated = await conversation_service.update_conversation(conversation_id, request) - if not updated: - return Success(success=False) - return Success() - - -@conversation_router_v2.post( - "/{conversation_id}/generate_title", - responses={404: {"description": "Item not found"}}, - deprecated=True, -) -async def generate_conversation_title( - conversation_id: UUID, - request: GenerateTitleRequest, - conversation_service: ConversationService = Depends(get_conversation_service), -) -> GenerateTitleResponse: - """Generate a title for the conversation using LLM. - - Deprecated since v1.11.5 and scheduled for removal in v1.19.0. - """ - title = await conversation_service.generate_conversation_title( - conversation_id, request.max_length, request.llm - ) - if title is None: - raise HTTPException(status.HTTP_500_INTERNAL_SERVER_ERROR) - return GenerateTitleResponse(title=title) - - -@conversation_router_v2.post( - "/{conversation_id}/ask_agent", - responses={404: {"description": "Item not found"}}, -) -async def ask_agent( - conversation_id: UUID, - request: AskAgentRequest, - conversation_service: ConversationService = Depends(get_conversation_service), -) -> AskAgentResponse: - """Ask the agent a simple question without affecting conversation state.""" - response = await conversation_service.ask_agent(conversation_id, request.question) - if response is None: - raise HTTPException(status.HTTP_500_INTERNAL_SERVER_ERROR) - return AskAgentResponse(response=response) - - -@conversation_router_v2.post( - "/{conversation_id}/condense", - responses={404: {"description": "Item not found"}}, -) -async def condense_conversation( - conversation_id: UUID, - conversation_service: ConversationService = Depends(get_conversation_service), -) -> Success: - """Force condensation of the conversation history.""" - success = await conversation_service.condense(conversation_id) - if not success: - raise HTTPException(status.HTTP_404_NOT_FOUND, detail="Conversation not found") - return Success() diff --git a/openhands-agent-server/openhands/agent_server/conversation_service.py b/openhands-agent-server/openhands/agent_server/conversation_service.py index 08f781db42..f2dcebc49e 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_service.py +++ b/openhands-agent-server/openhands/agent_server/conversation_service.py @@ -12,13 +12,13 @@ from openhands.agent_server.config import Config, WebhookSpec from openhands.agent_server.event_service import EventService from openhands.agent_server.models import ( + ACPConversationInfo, + ACPConversationPage, ConversationInfo, - ConversationInfoV2, ConversationPage, - ConversationPageV2, ConversationSortOrder, + StartACPConversationRequest, StartConversationRequest, - StartConversationRequestV2, StoredConversation, UpdateConversationRequest, ) @@ -58,10 +58,10 @@ def _compose_conversation_info_v1( ) -def _compose_conversation_info_v2( +def _compose_acp_conversation_info( stored: StoredConversation, state: ConversationState -) -> ConversationInfoV2: - return ConversationInfoV2( +) -> ACPConversationInfo: + return ACPConversationInfo( **state.model_dump(mode="json"), title=stored.title, metrics=stored.metrics, @@ -136,16 +136,16 @@ async def get_conversation(self, conversation_id: UUID) -> ConversationInfo | No state = await event_service.get_state() return _compose_conversation_info_v1(event_service.stored, state) - async def get_conversation_v2( + async def get_acp_conversation( self, conversation_id: UUID - ) -> ConversationInfoV2 | None: + ) -> ACPConversationInfo | None: if self._event_services is None: raise ValueError("inactive_service") event_service = self._event_services.get(conversation_id) if event_service is None: return None state = await event_service.get_state() - return _compose_conversation_info_v2(event_service.stored, state) + return _compose_acp_conversation_info(event_service.stored, state) async def search_conversations( self, @@ -159,29 +159,29 @@ async def search_conversations( limit=limit, execution_status=execution_status, sort_order=sort_order, - include_v2=False, + include_acp=False, ) return ConversationPage( items=cast(list[ConversationInfo], items), next_page_id=next_page_id, ) - async def search_conversations_v2( + async def search_acp_conversations( self, page_id: str | None = None, limit: int = 100, execution_status: ConversationExecutionStatus | None = None, sort_order: ConversationSortOrder = ConversationSortOrder.CREATED_AT_DESC, - ) -> ConversationPageV2: + ) -> ACPConversationPage: items, next_page_id = await self._search_conversations( page_id=page_id, limit=limit, execution_status=execution_status, sort_order=sort_order, - include_v2=True, + include_acp=True, ) - return ConversationPageV2( - items=cast(list[ConversationInfoV2], items), + return ACPConversationPage( + items=cast(list[ACPConversationInfo], items), next_page_id=next_page_id, ) @@ -192,20 +192,20 @@ async def _search_conversations( execution_status: ConversationExecutionStatus | None, sort_order: ConversationSortOrder, *, - include_v2: bool, - ) -> tuple[list[ConversationInfo | ConversationInfoV2], str | None]: + include_acp: bool, + ) -> tuple[list[ConversationInfo | ACPConversationInfo], str | None]: if self._event_services is None: raise ValueError("inactive_service") # 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): + if not include_acp and not _is_v1_conversation(event_service.stored): continue state = await event_service.get_state() conversation_info = ( - _compose_conversation_info_v2(event_service.stored, state) - if include_v2 + _compose_acp_conversation_info(event_service.stored, state) + if include_acp else _compose_conversation_info_v1(event_service.stored, state) ) # Apply status filter if provided @@ -256,23 +256,14 @@ async def count_conversations( ) -> int: return await self._count_conversations( execution_status=execution_status, - include_v2=False, - ) - - async def count_conversations_v2( - self, - execution_status: ConversationExecutionStatus | None = None, - ) -> int: - return await self._count_conversations( - execution_status=execution_status, - include_v2=True, + include_acp=False, ) async def _count_conversations( self, execution_status: ConversationExecutionStatus | None, *, - include_v2: bool, + include_acp: bool, ) -> int: """Count conversations matching the given filters.""" if self._event_services is None: @@ -280,7 +271,7 @@ async def _count_conversations( count = 0 for event_service in self._event_services.values(): - if not include_v2 and not _is_v1_conversation(event_service.stored): + if not include_acp and not _is_v1_conversation(event_service.stored): continue state = await event_service.get_state() @@ -308,12 +299,12 @@ async def batch_get_conversations( ) return results - async def batch_get_conversations_v2( + async def batch_get_acp_conversations( self, conversation_ids: list[UUID] - ) -> list[ConversationInfoV2 | None]: + ) -> list[ACPConversationInfo | None]: results = await asyncio.gather( *[ - self.get_conversation_v2(conversation_id) + self.get_acp_conversation(conversation_id) for conversation_id in conversation_ids ] ) @@ -358,28 +349,28 @@ async def start_conversation( assert isinstance(conversation_info, ConversationInfo) return conversation_info, is_new - async def start_conversation_v2( - self, request: StartConversationRequestV2 - ) -> tuple[ConversationInfoV2, bool]: + async def start_acp_conversation( + self, request: StartACPConversationRequest + ) -> tuple[ACPConversationInfo, bool]: conversation_info, is_new = await self._start_conversation(request) - assert isinstance(conversation_info, ConversationInfoV2) + assert isinstance(conversation_info, ACPConversationInfo) return conversation_info, is_new async def _start_conversation( - self, request: StartConversationRequest | StartConversationRequestV2 - ) -> tuple[ConversationInfo | ConversationInfoV2, bool]: + self, request: StartConversationRequest | StartACPConversationRequest + ) -> tuple[ConversationInfo | ACPConversationInfo, bool]: """Start a local event_service and return its id.""" if self._event_services is None: raise ValueError("inactive_service") conversation_id = request.conversation_id or uuid4() - use_v2 = isinstance(request, StartConversationRequestV2) + use_acp_contract = isinstance(request, StartACPConversationRequest) existing_event_service = self._event_services.get(conversation_id) if existing_event_service and existing_event_service.is_open(): state = await existing_event_service.get_state() conversation_info = ( - _compose_conversation_info_v2(existing_event_service.stored, state) - if use_v2 + _compose_acp_conversation_info(existing_event_service.stored, state) + if use_acp_contract else _compose_conversation_info_v1(existing_event_service.stored, state) ) return conversation_info, False @@ -442,14 +433,14 @@ async def _start_conversation( state = await event_service.get_state() conversation_info = ( - _compose_conversation_info_v2(event_service.stored, state) - if use_v2 + _compose_acp_conversation_info(event_service.stored, state) + if use_acp_contract else _compose_conversation_info_v1(event_service.stored, state) ) # Notify conversation webhooks about the started conversation await self._notify_conversation_webhooks( - _compose_conversation_info_v2(event_service.stored, state) + _compose_acp_conversation_info(event_service.stored, state) ) return conversation_info, True @@ -462,7 +453,7 @@ async def pause_conversation(self, conversation_id: UUID) -> bool: await event_service.pause() # Notify conversation webhooks about the paused conversation state = await event_service.get_state() - conversation_info = _compose_conversation_info_v2( + conversation_info = _compose_acp_conversation_info( event_service.stored, state ) await self._notify_conversation_webhooks(conversation_info) @@ -484,7 +475,7 @@ async def delete_conversation(self, conversation_id: UUID) -> bool: # Notify conversation webhooks about the stopped conversation before closing try: state = await event_service.get_state() - conversation_info = _compose_conversation_info_v2( + conversation_info = _compose_acp_conversation_info( event_service.stored, state ) conversation_info.execution_status = ( @@ -543,7 +534,7 @@ async def update_conversation( # Notify conversation webhooks about the updated conversation state = await event_service.get_state() - conversation_info = _compose_conversation_info_v2(event_service.stored, state) + conversation_info = _compose_acp_conversation_info(event_service.stored, state) await self._notify_conversation_webhooks(conversation_info) logger.info( diff --git a/openhands-agent-server/openhands/agent_server/models.py b/openhands-agent-server/openhands/agent_server/models.py index 05113f1b95..f90daaaf3a 100644 --- a/openhands-agent-server/openhands/agent_server/models.py +++ b/openhands-agent-server/openhands/agent_server/models.py @@ -72,7 +72,7 @@ def create_message(self) -> Message: class _StartConversationRequestBase(BaseModel): - """Common conversation creation fields shared by v1 and v2 contracts.""" + """Common conversation creation fields shared by conversation contracts.""" workspace: LocalWorkspace = Field( ..., @@ -160,16 +160,13 @@ class StartConversationRequest(_StartConversationRequestBase): agent: Agent -class StartConversationRequestV2(_StartConversationRequestBase): - """Versioned payload to create a new conversation. - - Supports the ACP-capable polymorphic agent contract. - """ +class StartACPConversationRequest(_StartConversationRequestBase): + """Payload to create a conversation with ACP-capable agent support.""" agent: ACPEnabledAgent -class StoredConversation(StartConversationRequestV2): +class StoredConversation(StartACPConversationRequest): """Stored details about a conversation. Extends StartConversationRequest with server-assigned fields. @@ -185,7 +182,7 @@ class StoredConversation(StartConversationRequestV2): class _ConversationInfoBase(BaseModel): - """Common conversation info fields shared by v1 and v2 contracts.""" + """Common conversation info fields shared by conversation contracts.""" id: UUID = Field(description="Unique conversation ID") workspace: BaseWorkspace = Field( @@ -287,20 +284,20 @@ class ConversationPage(BaseModel): next_page_id: str | None = None -class ConversationInfoV2(_ConversationInfoBase): - """Versioned conversation info that supports ACP-capable agent configs.""" +class ACPConversationInfo(_ConversationInfoBase): + """Conversation info that supports ACP-capable agent configs.""" agent: ACPEnabledAgent = Field( ..., description=( "The agent running in the conversation. " - "Version 2 supports both Agent and ACPAgent payloads." + "Supports both Agent and ACPAgent payloads." ), ) -class ConversationPageV2(BaseModel): - items: list[ConversationInfoV2] +class ACPConversationPage(BaseModel): + items: list[ACPConversationInfo] next_page_id: str | None = None diff --git a/openhands-sdk/openhands/sdk/conversation/impl/remote_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/remote_conversation.py index f858081e99..03a68a4547 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/remote_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/remote_conversation.py @@ -56,11 +56,11 @@ logger = get_logger(__name__) -V1_CONVERSATIONS_PATH = "/api/conversations" -V2_CONVERSATIONS_PATH = "/api/v2/conversations" +LEGACY_CONVERSATIONS_PATH = "/api/conversations" +ACP_CONVERSATIONS_PATH = "/api/acp/conversations" -def _uses_v2_conversation_contract(agent: AgentBase) -> bool: +def _uses_acp_conversation_contract(agent: AgentBase) -> bool: return getattr(agent, "kind", agent.__class__.__name__) == "ACPAgent" @@ -237,7 +237,7 @@ class RemoteEventsList(EventsListBase): _client: httpx.Client _conversation_id: str - _conversation_base_path: str + _events_base_path: str _cached_events: list[Event] _cached_event_ids: set[str] _lock: threading.RLock @@ -246,11 +246,11 @@ def __init__( self, client: httpx.Client, conversation_id: str, - conversation_base_path: str = V1_CONVERSATIONS_PATH, + events_base_path: str = LEGACY_CONVERSATIONS_PATH, ): self._client = client self._conversation_id = conversation_id - self._conversation_base_path = conversation_base_path + self._events_base_path = events_base_path self._cached_events: list[Event] = [] self._cached_event_ids: set[str] = set() self._lock = threading.RLock() @@ -272,7 +272,7 @@ def _do_full_sync(self) -> None: resp = _send_request( self._client, "GET", - f"{self._conversation_base_path}/{self._conversation_id}/events/search", + f"{self._events_base_path}/{self._conversation_id}/events/search", params=params, ) data = resp.json() @@ -314,10 +314,7 @@ def reconcile(self) -> int: resp = _send_request( self._client, "GET", - ( - f"{self._conversation_base_path}/" - f"{self._conversation_id}/events/search" - ), + f"{self._events_base_path}/{self._conversation_id}/events/search", params=params, ) data = resp.json() @@ -403,7 +400,7 @@ class RemoteState(ConversationStateProtocol): _client: httpx.Client _conversation_id: str - _conversation_base_path: str + _conversation_info_base_path: str _events: RemoteEventsList _cached_state: dict | None _lock: threading.RLock @@ -412,12 +409,13 @@ def __init__( self, client: httpx.Client, conversation_id: str, - conversation_base_path: str = V1_CONVERSATIONS_PATH, + conversation_info_base_path: str = LEGACY_CONVERSATIONS_PATH, + events_base_path: str = LEGACY_CONVERSATIONS_PATH, ): self._client = client self._conversation_id = conversation_id - self._conversation_base_path = conversation_base_path - self._events = RemoteEventsList(client, conversation_id, conversation_base_path) + self._conversation_info_base_path = conversation_info_base_path + self._events = RemoteEventsList(client, conversation_id, events_base_path) # Cache for state information to avoid REST calls self._cached_state = None @@ -434,7 +432,7 @@ def _get_conversation_info(self) -> dict: resp = _send_request( self._client, "GET", - f"{self._conversation_base_path}/{self._conversation_id}", + f"{self._conversation_info_base_path}/{self._conversation_id}", ) state = resp.json() self._cached_state = state @@ -598,7 +596,8 @@ class RemoteConversation(BaseConversation): _client: httpx.Client _cleanup_initiated: bool _terminal_status_queue: Queue[str] # Thread-safe queue for terminal status from WS - _conversation_base_path: str + _conversation_info_base_path: str + _conversation_action_base_path: str delete_on_close: bool = False def __init__( @@ -652,11 +651,12 @@ def __init__( self.max_iteration_per_run = max_iteration_per_run self.workspace = workspace self._client = workspace.client - self._conversation_base_path = ( - V2_CONVERSATIONS_PATH - if _uses_v2_conversation_contract(agent) - else V1_CONVERSATIONS_PATH + self._conversation_info_base_path = ( + ACP_CONVERSATIONS_PATH + if _uses_acp_conversation_contract(agent) + else LEGACY_CONVERSATIONS_PATH ) + self._conversation_action_base_path = LEGACY_CONVERSATIONS_PATH self._cleanup_initiated = False self._terminal_status_queue: Queue[str] = Queue() @@ -666,7 +666,7 @@ def __init__( resp = _send_request( self._client, "GET", - f"{self._conversation_base_path}/{conversation_id}", + f"{self._conversation_info_base_path}/{conversation_id}", acceptable_status_codes={404}, ) if resp.status_code == 404: @@ -723,7 +723,7 @@ def __init__( resp = _send_request( self._client, "POST", - self._conversation_base_path, + self._conversation_info_base_path, json=payload, ) data = resp.json() @@ -739,7 +739,8 @@ def __init__( self._state = RemoteState( self._client, str(self._id), - conversation_base_path=self._conversation_base_path, + conversation_info_base_path=self._conversation_info_base_path, + events_base_path=self._conversation_action_base_path, ) # Add default callback to maintain local event state @@ -904,7 +905,7 @@ def send_message(self, message: str | Message, sender: str | None = None) -> Non _send_request( self._client, "POST", - f"{self._conversation_base_path}/{self._id}/events", + f"{self._conversation_action_base_path}/{self._id}/events", json=payload, ) @@ -942,7 +943,7 @@ def run( resp = _send_request( self._client, "POST", - f"{self._conversation_base_path}/{self._id}/run", + f"{self._conversation_action_base_path}/{self._id}/run", acceptable_status_codes={200, 201, 204, 409}, timeout=30, # Short timeout for trigger request ) @@ -1062,7 +1063,7 @@ def _poll_status_once(self) -> str | None: resp = _send_request( self._client, "GET", - f"{self._conversation_base_path}/{self._id}", + f"{self._conversation_info_base_path}/{self._id}", timeout=30, ) info = resp.json() @@ -1131,7 +1132,7 @@ def set_confirmation_policy(self, policy: ConfirmationPolicyBase) -> None: _send_request( self._client, "POST", - f"{self._conversation_base_path}/{self._id}/confirmation_policy", + f"{self._conversation_action_base_path}/{self._id}/confirmation_policy", json=payload, ) @@ -1141,7 +1142,7 @@ def set_security_analyzer(self, analyzer: SecurityAnalyzerBase | None) -> None: _send_request( self._client, "POST", - f"{self._conversation_base_path}/{self._id}/security_analyzer", + f"{self._conversation_action_base_path}/{self._id}/security_analyzer", json=payload, ) @@ -1150,7 +1151,10 @@ def reject_pending_actions(self, reason: str = "User rejected the action") -> No _send_request( self._client, "POST", - f"{self._conversation_base_path}/{self._id}/events/respond_to_confirmation", + ( + f"{self._conversation_action_base_path}/{self._id}" + "/events/respond_to_confirmation" + ), json={"accept": False, "reason": reason}, ) @@ -1158,7 +1162,7 @@ def pause(self) -> None: _send_request( self._client, "POST", - f"{self._conversation_base_path}/{self._id}/pause", + f"{self._conversation_action_base_path}/{self._id}/pause", ) def update_secrets(self, secrets: Mapping[str, SecretValue]) -> None: @@ -1177,7 +1181,7 @@ def update_secrets(self, secrets: Mapping[str, SecretValue]) -> None: _send_request( self._client, "POST", - f"{self._conversation_base_path}/{self._id}/secrets", + f"{self._conversation_action_base_path}/{self._id}/secrets", json=payload, ) @@ -1202,7 +1206,7 @@ def ask_agent(self, question: str) -> str: resp = _send_request( self._client, "POST", - f"{self._conversation_base_path}/{self._id}/ask_agent", + f"{self._conversation_action_base_path}/{self._id}/ask_agent", json=payload, ) data = resp.json() @@ -1231,7 +1235,7 @@ def generate_title(self, llm: LLM | None = None, max_length: int = 50) -> str: resp = _send_request( self._client, "POST", - f"{self._conversation_base_path}/{self._id}/generate_title", + f"{self._conversation_action_base_path}/{self._id}/generate_title", json=payload, ) data = resp.json() @@ -1253,7 +1257,7 @@ def condense(self) -> None: _send_request( self._client, "POST", - f"{self._conversation_base_path}/{self._id}/condense", + f"{self._conversation_action_base_path}/{self._id}/condense", ) def execute_tool(self, tool_name: str, action: "Action") -> "Observation": @@ -1305,7 +1309,7 @@ def close(self) -> None: _send_request( self._client, "DELETE", - f"{self._conversation_base_path}/{self.id}", + f"{self._conversation_action_base_path}/{self.id}", ) except Exception: pass diff --git a/tests/agent_server/test_conversation_router_acp.py b/tests/agent_server/test_conversation_router_acp.py new file mode 100644 index 0000000000..290b09894d --- /dev/null +++ b/tests/agent_server/test_conversation_router_acp.py @@ -0,0 +1,137 @@ +"""Tests for the ACP-capable conversation router.""" + +from unittest.mock import AsyncMock +from uuid import uuid4 + +import pytest +from fastapi import FastAPI +from fastapi.testclient import TestClient + +from openhands.agent_server.conversation_router_acp import conversation_router_acp +from openhands.agent_server.conversation_service import ConversationService +from openhands.agent_server.dependencies import get_conversation_service +from openhands.agent_server.models import ACPConversationInfo, ACPConversationPage +from openhands.agent_server.utils import utc_now +from openhands.sdk.agent.acp_agent import ACPAgent +from openhands.sdk.conversation.state import ConversationExecutionStatus +from openhands.sdk.workspace import LocalWorkspace + + +@pytest.fixture +def client(): + app = FastAPI() + app.include_router(conversation_router_acp, prefix="/api") + return TestClient(app) + + +@pytest.fixture +def mock_conversation_service(): + return AsyncMock(spec=ConversationService) + + +@pytest.fixture +def sample_acp_conversation_info(): + now = utc_now() + return ACPConversationInfo( + id=uuid4(), + agent=ACPAgent(acp_command=["echo", "test"]), + workspace=LocalWorkspace(working_dir="/tmp/test"), + execution_status=ConversationExecutionStatus.IDLE, + title="ACP Conversation", + created_at=now, + updated_at=now, + ) + + +def test_start_acp_conversation_accepts_acp_agent( + client, mock_conversation_service, sample_acp_conversation_info +): + mock_conversation_service.start_acp_conversation.return_value = ( + sample_acp_conversation_info, + True, + ) + client.app.dependency_overrides[get_conversation_service] = ( + lambda: mock_conversation_service + ) + + try: + response = client.post( + "/api/acp/conversations", + json={ + "agent": { + "kind": "ACPAgent", + "acp_command": ["echo", "test"], + }, + "workspace": {"working_dir": "/tmp/test"}, + }, + ) + + assert response.status_code == 201 + assert response.json()["agent"]["kind"] == "ACPAgent" + mock_conversation_service.start_acp_conversation.assert_called_once() + finally: + client.app.dependency_overrides.clear() + + +def test_get_acp_conversation_returns_acp_agent( + client, mock_conversation_service, sample_acp_conversation_info +): + mock_conversation_service.get_acp_conversation.return_value = ( + sample_acp_conversation_info + ) + client.app.dependency_overrides[get_conversation_service] = ( + lambda: mock_conversation_service + ) + + try: + response = client.get( + f"/api/acp/conversations/{sample_acp_conversation_info.id}" + ) + + assert response.status_code == 200 + assert response.json()["agent"]["kind"] == "ACPAgent" + finally: + client.app.dependency_overrides.clear() + + +def test_search_acp_conversations_returns_acp_page( + client, mock_conversation_service, sample_acp_conversation_info +): + mock_conversation_service.search_acp_conversations.return_value = ( + ACPConversationPage( + items=[sample_acp_conversation_info], + next_page_id=None, + ) + ) + client.app.dependency_overrides[get_conversation_service] = ( + lambda: mock_conversation_service + ) + + try: + response = client.get("/api/acp/conversations/search") + + assert response.status_code == 200 + assert response.json()["items"][0]["agent"]["kind"] == "ACPAgent" + finally: + client.app.dependency_overrides.clear() + + +def test_batch_get_acp_conversations_returns_acp_agents( + client, mock_conversation_service, sample_acp_conversation_info +): + mock_conversation_service.batch_get_acp_conversations.return_value = [ + sample_acp_conversation_info + ] + client.app.dependency_overrides[get_conversation_service] = ( + lambda: mock_conversation_service + ) + + try: + response = client.get( + f"/api/acp/conversations?ids={sample_acp_conversation_info.id}" + ) + + assert response.status_code == 200 + assert response.json()[0]["agent"]["kind"] == "ACPAgent" + finally: + client.app.dependency_overrides.clear() diff --git a/tests/agent_server/test_conversation_router_v2.py b/tests/agent_server/test_conversation_router_v2.py deleted file mode 100644 index 152be787f6..0000000000 --- a/tests/agent_server/test_conversation_router_v2.py +++ /dev/null @@ -1,92 +0,0 @@ -"""Tests for the v2 ACP-capable conversation router.""" - -from unittest.mock import AsyncMock -from uuid import uuid4 - -import pytest -from fastapi import FastAPI -from fastapi.testclient import TestClient - -from openhands.agent_server.conversation_router_v2 import conversation_router_v2 -from openhands.agent_server.conversation_service import ConversationService -from openhands.agent_server.dependencies import get_conversation_service -from openhands.agent_server.models import ConversationInfoV2 -from openhands.agent_server.utils import utc_now -from openhands.sdk.agent.acp_agent import ACPAgent -from openhands.sdk.conversation.state import ConversationExecutionStatus -from openhands.sdk.workspace import LocalWorkspace - - -@pytest.fixture -def client(): - app = FastAPI() - app.include_router(conversation_router_v2, prefix="/api") - return TestClient(app) - - -@pytest.fixture -def mock_conversation_service(): - return AsyncMock(spec=ConversationService) - - -@pytest.fixture -def sample_conversation_info_v2(): - now = utc_now() - return ConversationInfoV2( - id=uuid4(), - agent=ACPAgent(acp_command=["echo", "test"]), - workspace=LocalWorkspace(working_dir="/tmp/test"), - execution_status=ConversationExecutionStatus.IDLE, - title="ACP Conversation", - created_at=now, - updated_at=now, - ) - - -def test_start_conversation_v2_accepts_acp_agent( - client, mock_conversation_service, sample_conversation_info_v2 -): - mock_conversation_service.start_conversation_v2.return_value = ( - sample_conversation_info_v2, - True, - ) - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service - ) - - try: - response = client.post( - "/api/v2/conversations", - json={ - "agent": { - "kind": "ACPAgent", - "acp_command": ["echo", "test"], - }, - "workspace": {"working_dir": "/tmp/test"}, - }, - ) - - assert response.status_code == 201 - assert response.json()["agent"]["kind"] == "ACPAgent" - mock_conversation_service.start_conversation_v2.assert_called_once() - finally: - client.app.dependency_overrides.clear() - - -def test_get_conversation_v2_returns_acp_agent( - client, mock_conversation_service, sample_conversation_info_v2 -): - mock_conversation_service.get_conversation_v2.return_value = ( - sample_conversation_info_v2 - ) - client.app.dependency_overrides[get_conversation_service] = ( - lambda: mock_conversation_service - ) - - try: - response = client.get(f"/api/v2/conversations/{sample_conversation_info_v2.id}") - - assert response.status_code == 200 - assert response.json()["agent"]["kind"] == "ACPAgent" - finally: - client.app.dependency_overrides.clear() diff --git a/tests/agent_server/test_openapi_discriminator.py b/tests/agent_server/test_openapi_discriminator.py index cef7da5a5b..9e9f11e508 100644 --- a/tests/agent_server/test_openapi_discriminator.py +++ b/tests/agent_server/test_openapi_discriminator.py @@ -166,21 +166,37 @@ def test_action_variants_have_proper_schemas(client): ) -def test_conversation_contracts_are_versioned(client): - """v1 conversations stay Agent-only while v2 exposes ACP support.""" +def test_conversation_contracts_keep_v1_stable_and_add_acp_routes(client): + """v1 conversations stay Agent-only while ACP endpoints expose polymorphism.""" response = client.get("/openapi.json") assert response.status_code == 200 - schemas = response.json()["components"]["schemas"] + openapi_schema = response.json() + schemas = openapi_schema["components"]["schemas"] v1_request = schemas["StartConversationRequest"] assert ( v1_request["properties"]["agent"]["$ref"] == "#/components/schemas/Agent-Input" ) + v1_response = schemas["ConversationInfo"] + assert ( + v1_response["properties"]["agent"]["$ref"] + == "#/components/schemas/Agent-Output" + ) - v2_request = schemas["StartConversationRequestV2"] - agent_schema = v2_request["properties"]["agent"] + acp_request = schemas["StartACPConversationRequest"] + agent_schema = acp_request["properties"]["agent"] assert "oneOf" in agent_schema refs = {variant["$ref"] for variant in agent_schema["oneOf"]} assert "#/components/schemas/Agent-Input" in refs assert "#/components/schemas/ACPAgent-Input" in refs + + acp_response = schemas["ACPConversationInfo"] + acp_agent_schema = acp_response["properties"]["agent"] + assert "oneOf" in acp_agent_schema + acp_refs = {variant["$ref"] for variant in acp_agent_schema["oneOf"]} + assert "#/components/schemas/Agent-Output" in acp_refs + assert "#/components/schemas/ACPAgent-Output" in acp_refs + + assert "/api/v2/conversations" not in openapi_schema["paths"] + assert "/api/acp/conversations" in openapi_schema["paths"] diff --git a/tests/sdk/conversation/remote/test_remote_conversation.py b/tests/sdk/conversation/remote/test_remote_conversation.py index 9646b7144b..96fc4b9caa 100644 --- a/tests/sdk/conversation/remote/test_remote_conversation.py +++ b/tests/sdk/conversation/remote/test_remote_conversation.py @@ -174,7 +174,7 @@ def test_remote_conversation_initialization_new_conversation(self, mock_ws_clien @patch( "openhands.sdk.conversation.impl.remote_conversation.WebSocketCallbackClient" ) - def test_acp_remote_conversation_uses_v2_conversation_contract( + def test_acp_remote_conversation_uses_acp_conversation_contract( self, mock_ws_client ): acp_agent = ACPAgent(acp_command=["echo", "test"]) @@ -186,11 +186,11 @@ def test_acp_remote_conversation_uses_v2_conversation_contract( mock_events_response = self.create_mock_events_response() def request_side_effect(method, url, **kwargs): - if method == "POST" and url == "/api/v2/conversations": + if method == "POST" and url == "/api/acp/conversations": return mock_conv_response - if method == "GET" and "/api/v2/conversations/" in url and "/events" in url: + if method == "GET" and "/api/conversations/" in url and "/events" in url: return mock_events_response - if method == "GET" and url.startswith("/api/v2/conversations/"): + if method == "GET" and url.startswith("/api/acp/conversations/"): response = Mock() response.status_code = 200 response.raise_for_status.return_value = None @@ -218,14 +218,14 @@ def request_side_effect(method, url, **kwargs): post_calls = [ call for call in mock_client_instance.request.call_args_list - if call[0][0] == "POST" and call[0][1] == "/api/v2/conversations" + if call[0][0] == "POST" and call[0][1] == "/api/acp/conversations" ] assert len(post_calls) == 1 get_events_calls = [ call for call in mock_client_instance.request.call_args_list - if call[0][0] == "GET" and "/api/v2/conversations/" in call[0][1] + if call[0][0] == "GET" and "/api/conversations/" in call[0][1] ] assert len(get_events_calls) >= 1 From 7147a8093691ea6ea8098e0d72aecb20daffbbd8 Mon Sep 17 00:00:00 2001 From: Simon Rosenberg Date: Mon, 16 Mar 2026 13:10:27 -0300 Subject: [PATCH 4/6] fix(agent-server): preserve legacy webhook payloads Co-authored-by: openhands --- .../agent_server/conversation_service.py | 18 ++++++-- .../openhands/sdk/agent/acp_agent.py | 17 ++++--- .../agent_server/test_conversation_service.py | 46 +++++++++++++++++++ 3 files changed, 71 insertions(+), 10 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/conversation_service.py b/openhands-agent-server/openhands/agent_server/conversation_service.py index f2dcebc49e..eb10efc81d 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_service.py +++ b/openhands-agent-server/openhands/agent_server/conversation_service.py @@ -74,6 +74,14 @@ def _is_v1_conversation(stored: StoredConversation) -> bool: return isinstance(stored.agent, Agent) +def _compose_webhook_conversation_info( + stored: StoredConversation, state: ConversationState +) -> ConversationInfo | ACPConversationInfo: + if _is_v1_conversation(stored): + return _compose_conversation_info_v1(stored, state) + return _compose_acp_conversation_info(stored, state) + + def _register_agent_definitions( agent_defs: list["AgentDefinition"], *, @@ -440,7 +448,7 @@ async def _start_conversation( # Notify conversation webhooks about the started conversation await self._notify_conversation_webhooks( - _compose_acp_conversation_info(event_service.stored, state) + _compose_webhook_conversation_info(event_service.stored, state) ) return conversation_info, True @@ -453,7 +461,7 @@ async def pause_conversation(self, conversation_id: UUID) -> bool: await event_service.pause() # Notify conversation webhooks about the paused conversation state = await event_service.get_state() - conversation_info = _compose_acp_conversation_info( + conversation_info = _compose_webhook_conversation_info( event_service.stored, state ) await self._notify_conversation_webhooks(conversation_info) @@ -475,7 +483,7 @@ async def delete_conversation(self, conversation_id: UUID) -> bool: # Notify conversation webhooks about the stopped conversation before closing try: state = await event_service.get_state() - conversation_info = _compose_acp_conversation_info( + conversation_info = _compose_webhook_conversation_info( event_service.stored, state ) conversation_info.execution_status = ( @@ -534,7 +542,9 @@ async def update_conversation( # Notify conversation webhooks about the updated conversation state = await event_service.get_state() - conversation_info = _compose_acp_conversation_info(event_service.stored, state) + conversation_info = _compose_webhook_conversation_info( + event_service.stored, state + ) await self._notify_conversation_webhooks(conversation_info) logger.info( diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index 9359b1db49..b044136860 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -5,6 +5,10 @@ LLM calls. The ACP server manages its own LLM, tools, and execution; the ACPAgent simply relays user messages and collects the response. +Unlike the built-in Agent, one ACP ``step()`` maps to one complete remote +assistant turn. ACPAgent therefore emits a terminal ``FinishAction`` at the +end of each step to delimit that completed turn for downstream consumers. + See https://agentclientprotocol.com/protocol/overview """ @@ -92,8 +96,10 @@ # carry large tool-call outputs (e.g. file contents, test results). When # a single JSON-RPC line exceeds the limit, readline() raises # 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. +# leaving the prompt() future unresolved forever. 100 MiB is a pragmatic +# compatibility limit for current ACP servers, not an endorsement of huge +# JSON-RPC payloads; the long-term fix is protocol-level chunking/streaming +# for large tool output. _STREAM_READER_LIMIT: int = 100 * 1024 * 1024 # 100 MiB @@ -792,10 +798,9 @@ async def _prompt() -> PromptResponse: ) on_event(msg_event) - # Emit FinishAction so evaluation frameworks (SWE-bench, etc.) - # recognize that the agent has completed its task. The regular - # Agent emits this when the LLM calls the "finish" tool; for - # ACPAgent each step() is a complete turn, so we always finish. + # ACP step() boundaries are full remote assistant turns, not + # partial planning steps. Emit FinishAction to delimit that + # completed turn for eval/remote consumers, matching #2190. finish_action = FinishAction(message=response_text) tc_id = str(uuid.uuid4()) action_event = ActionEvent( diff --git a/tests/agent_server/test_conversation_service.py b/tests/agent_server/test_conversation_service.py index c5667cc665..a50c37d05c 100644 --- a/tests/agent_server/test_conversation_service.py +++ b/tests/agent_server/test_conversation_service.py @@ -14,6 +14,8 @@ ) from openhands.agent_server.event_service import EventService from openhands.agent_server.models import ( + ACPConversationInfo, + ConversationInfo, ConversationPage, ConversationSortOrder, StartConversationRequest, @@ -22,6 +24,7 @@ ) from openhands.agent_server.utils import safe_rmtree as _safe_rmtree from openhands.sdk import LLM, Agent, Message +from openhands.sdk.agent.acp_agent import ACPAgent from openhands.sdk.conversation.state import ( ConversationExecutionStatus, ConversationState, @@ -958,6 +961,48 @@ async def test_update_conversation_notifies_webhooks( call_args = mock_notify.call_args[0] conversation_info = call_args[0] assert conversation_info.title == new_title + assert isinstance(conversation_info, ConversationInfo) + + @pytest.mark.asyncio + async def test_update_acp_conversation_notifies_webhooks_with_acp_shape( + self, conversation_service + ): + stored_conversation = StoredConversation( + id=uuid4(), + agent=ACPAgent(acp_command=["echo", "test"]), + workspace=LocalWorkspace(working_dir="workspace/project"), + confirmation_policy=NeverConfirm(), + initial_message=None, + metrics=None, + created_at=datetime(2025, 1, 1, 12, 0, 0, tzinfo=UTC), + updated_at=datetime(2025, 1, 1, 12, 30, 0, tzinfo=UTC), + ) + mock_service = AsyncMock(spec=EventService) + mock_service.stored = stored_conversation + mock_state = ConversationState( + id=stored_conversation.id, + agent=stored_conversation.agent, + workspace=stored_conversation.workspace, + execution_status=ConversationExecutionStatus.IDLE, + confirmation_policy=stored_conversation.confirmation_policy, + ) + mock_service.get_state.return_value = mock_state + + conversation_id = stored_conversation.id + conversation_service._event_services[conversation_id] = mock_service + + with patch.object( + conversation_service, "_notify_conversation_webhooks", new=AsyncMock() + ) as mock_notify: + result = await conversation_service.update_conversation( + conversation_id, UpdateConversationRequest(title="ACP Title") + ) + + assert result is True + mock_notify.assert_called_once() + conversation_info = mock_notify.call_args[0][0] + assert isinstance(conversation_info, ACPConversationInfo) + assert conversation_info.agent.kind == "ACPAgent" @pytest.mark.asyncio async def test_update_conversation_persists_changes( @@ -1188,6 +1233,7 @@ async def test_delete_conversation_notifies_webhooks_with_deleting_status( conversation_info.execution_status == ConversationExecutionStatus.DELETING ) + assert isinstance(conversation_info, ConversationInfo) # Verify event service was closed mock_service.close.assert_called_once() From 8af3c6bc5ee65d6172f5d8e5626b0d4b001c1f57 Mon Sep 17 00:00:00 2001 From: Debug Agent Date: Mon, 16 Mar 2026 15:44:51 -0300 Subject: [PATCH 5/6] fix(agent-server): add ACP count and reject contract mismatch Co-authored-by: openhands --- .../agent_server/conversation_router.py | 18 +++- .../agent_server/conversation_router_acp.py | 12 +++ .../agent_server/conversation_service.py | 29 ++++++ .../conversation/impl/remote_conversation.py | 19 ++++ .../agent_server/test_conversation_router.py | 42 ++++++++- .../test_conversation_router_acp.py | 16 ++++ .../agent_server/test_conversation_service.py | 90 +++++++++++++++++++ .../test_openapi_discriminator.py | 1 + .../remote/test_remote_conversation.py | 59 ++++++++++++ 9 files changed, 282 insertions(+), 4 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/conversation_router.py b/openhands-agent-server/openhands/agent_server/conversation_router.py index 836c0cc418..12f52f46dc 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_router.py +++ b/openhands-agent-server/openhands/agent_server/conversation_router.py @@ -6,7 +6,10 @@ from fastapi import APIRouter, Body, Depends, HTTPException, Query, Response, status from pydantic import SecretStr -from openhands.agent_server.conversation_service import ConversationService +from openhands.agent_server.conversation_service import ( + ConversationContractMismatchError, + ConversationService, +) from openhands.agent_server.dependencies import get_conversation_service from openhands.agent_server.models import ( AskAgentRequest, @@ -125,7 +128,10 @@ async def batch_get_conversations( # Write Methods -@conversation_router.post("") +@conversation_router.post( + "", + responses={409: {"description": "Conversation contract mismatch"}}, +) async def start_conversation( request: Annotated[ StartConversationRequest, Body(examples=START_CONVERSATION_EXAMPLES) @@ -134,7 +140,13 @@ async def start_conversation( conversation_service: ConversationService = Depends(get_conversation_service), ) -> ConversationInfo: """Start a conversation in the local environment.""" - info, is_new = await conversation_service.start_conversation(request) + try: + info, is_new = await conversation_service.start_conversation(request) + except ConversationContractMismatchError as e: + raise HTTPException( + status_code=status.HTTP_409_CONFLICT, + detail=str(e), + ) from e response.status_code = status.HTTP_201_CREATED if is_new else status.HTTP_200_OK return info diff --git a/openhands-agent-server/openhands/agent_server/conversation_router_acp.py b/openhands-agent-server/openhands/agent_server/conversation_router_acp.py index 75a61be689..ceffb9f32a 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_router_acp.py +++ b/openhands-agent-server/openhands/agent_server/conversation_router_acp.py @@ -81,6 +81,18 @@ async def search_acp_conversations( ) +@conversation_router_acp.get("/count") +async def count_acp_conversations( + status: Annotated[ + ConversationExecutionStatus | None, + Query(title="Optional filter by conversation execution status"), + ] = None, + conversation_service: ConversationService = Depends(get_conversation_service), +) -> int: + """Count conversations using the ACP-capable contract.""" + return await conversation_service.count_acp_conversations(status) + + @conversation_router_acp.get( "/{conversation_id}", responses={404: {"description": "Item not found"}}, diff --git a/openhands-agent-server/openhands/agent_server/conversation_service.py b/openhands-agent-server/openhands/agent_server/conversation_service.py index eb10efc81d..37b715191a 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_service.py +++ b/openhands-agent-server/openhands/agent_server/conversation_service.py @@ -42,6 +42,18 @@ logger = logging.getLogger(__name__) +class ConversationContractMismatchError(ValueError): + """Raised when a conversation ID exists under a different REST contract.""" + + +def _conversation_contract_mismatch_message(conversation_id: UUID) -> str: + return ( + f"Conversation {conversation_id} exists but is only available through the " + "ACP conversation contract. Use /api/acp/conversations or attach with " + "ACPAgent." + ) + + def _compose_conversation_info_v1( stored: StoredConversation, state: ConversationState ) -> ConversationInfo: @@ -267,6 +279,15 @@ async def count_conversations( include_acp=False, ) + async def count_acp_conversations( + self, + execution_status: ConversationExecutionStatus | None = None, + ) -> int: + return await self._count_conversations( + execution_status=execution_status, + include_acp=True, + ) + async def _count_conversations( self, execution_status: ConversationExecutionStatus | None, @@ -374,6 +395,14 @@ async def _start_conversation( use_acp_contract = isinstance(request, StartACPConversationRequest) existing_event_service = self._event_services.get(conversation_id) + if ( + existing_event_service is not None + and not use_acp_contract + and not _is_v1_conversation(existing_event_service.stored) + ): + raise ConversationContractMismatchError( + _conversation_contract_mismatch_message(conversation_id) + ) if existing_event_service and existing_event_service.is_open(): state = await existing_event_service.get_state() conversation_info = ( diff --git a/openhands-sdk/openhands/sdk/conversation/impl/remote_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/remote_conversation.py index 03a68a4547..d9bac2dc6e 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/remote_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/remote_conversation.py @@ -64,6 +64,14 @@ def _uses_acp_conversation_contract(agent: AgentBase) -> bool: return getattr(agent, "kind", agent.__class__.__name__) == "ACPAgent" +def _conversation_contract_mismatch_message(conversation_id: ConversationID) -> str: + return ( + f"Conversation {conversation_id} exists but is only available through the " + "ACP conversation contract. Attach with ACPAgent or use " + "/api/acp/conversations." + ) + + def _validate_remote_agent(agent_data: dict) -> AgentBase: if agent_data.get("kind") == "ACPAgent": from openhands.sdk.agent.acp_agent import ACPAgent @@ -670,6 +678,17 @@ def __init__( acceptable_status_codes={404}, ) if resp.status_code == 404: + if not _uses_acp_conversation_contract(agent): + acp_resp = _send_request( + self._client, + "GET", + f"{ACP_CONVERSATIONS_PATH}/{conversation_id}", + acceptable_status_codes={404}, + ) + if acp_resp.status_code != 404: + raise ValueError( + _conversation_contract_mismatch_message(conversation_id) + ) # Conversation doesn't exist, we'll create it should_create = True else: diff --git a/tests/agent_server/test_conversation_router.py b/tests/agent_server/test_conversation_router.py index 232d7d0283..84bac7e87d 100644 --- a/tests/agent_server/test_conversation_router.py +++ b/tests/agent_server/test_conversation_router.py @@ -9,7 +9,10 @@ from pydantic import SecretStr from openhands.agent_server.conversation_router import conversation_router -from openhands.agent_server.conversation_service import ConversationService +from openhands.agent_server.conversation_service import ( + ConversationContractMismatchError, + ConversationService, +) from openhands.agent_server.dependencies import get_conversation_service from openhands.agent_server.event_service import EventService from openhands.agent_server.models import ( @@ -555,6 +558,43 @@ def test_start_conversation_existing( client.app.dependency_overrides.clear() +def test_start_conversation_contract_mismatch_returns_409( + client, mock_conversation_service +): + mock_conversation_service.start_conversation.side_effect = ( + ConversationContractMismatchError( + "Conversation 123 exists but is only available through the ACP " + "conversation contract. Use /api/acp/conversations or attach with " + "ACPAgent." + ) + ) + + client.app.dependency_overrides[get_conversation_service] = ( + lambda: mock_conversation_service + ) + + try: + request_data = { + "agent": { + "kind": "Agent", + "llm": { + "model": "gpt-4o", + "api_key": "test-key", + "usage_id": "test-llm", + }, + "tools": [{"name": "TerminalTool"}], + }, + "workspace": {"working_dir": "/tmp/test"}, + } + + response = client.post("/api/conversations", json=request_data) + + assert response.status_code == 409 + assert "ACP conversation contract" in response.json()["detail"] + finally: + client.app.dependency_overrides.clear() + + def test_start_conversation_invalid_request(client, mock_conversation_service): """Test start_conversation endpoint with invalid request data.""" diff --git a/tests/agent_server/test_conversation_router_acp.py b/tests/agent_server/test_conversation_router_acp.py index 290b09894d..9f654833fd 100644 --- a/tests/agent_server/test_conversation_router_acp.py +++ b/tests/agent_server/test_conversation_router_acp.py @@ -116,6 +116,22 @@ def test_search_acp_conversations_returns_acp_page( client.app.dependency_overrides.clear() +def test_count_acp_conversations_returns_count(client, mock_conversation_service): + mock_conversation_service.count_acp_conversations.return_value = 2 + client.app.dependency_overrides[get_conversation_service] = ( + lambda: mock_conversation_service + ) + + try: + response = client.get("/api/acp/conversations/count") + + assert response.status_code == 200 + assert response.json() == 2 + mock_conversation_service.count_acp_conversations.assert_called_once_with(None) + finally: + client.app.dependency_overrides.clear() + + def test_batch_get_acp_conversations_returns_acp_agents( client, mock_conversation_service, sample_acp_conversation_info ): diff --git a/tests/agent_server/test_conversation_service.py b/tests/agent_server/test_conversation_service.py index a50c37d05c..92ae9e7a32 100644 --- a/tests/agent_server/test_conversation_service.py +++ b/tests/agent_server/test_conversation_service.py @@ -10,6 +10,7 @@ from openhands.agent_server.conversation_service import ( AutoTitleSubscriber, + ConversationContractMismatchError, ConversationService, ) from openhands.agent_server.event_service import EventService @@ -491,6 +492,46 @@ async def test_count_conversations_status_filter(self, conversation_service): ) assert result == 0 + @pytest.mark.asyncio + async def test_count_acp_conversations_includes_legacy_and_acp( + self, conversation_service + ): + legacy_conversation = StoredConversation( + id=uuid4(), + agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), + workspace=LocalWorkspace(working_dir="workspace/project"), + confirmation_policy=NeverConfirm(), + initial_message=None, + metrics=None, + created_at=datetime(2025, 1, 1, 12, 0, 0, tzinfo=UTC), + updated_at=datetime(2025, 1, 1, 12, 30, 0, tzinfo=UTC), + ) + acp_conversation = StoredConversation( + id=uuid4(), + agent=ACPAgent(acp_command=["echo", "test"]), + workspace=LocalWorkspace(working_dir="workspace/project"), + confirmation_policy=NeverConfirm(), + initial_message=None, + metrics=None, + created_at=datetime(2025, 1, 1, 13, 0, 0, tzinfo=UTC), + updated_at=datetime(2025, 1, 1, 13, 30, 0, tzinfo=UTC), + ) + + for stored_conv in (legacy_conversation, acp_conversation): + mock_service = AsyncMock(spec=EventService) + mock_service.stored = stored_conv + mock_service.get_state.return_value = ConversationState( + id=stored_conv.id, + agent=stored_conv.agent, + workspace=stored_conv.workspace, + execution_status=ConversationExecutionStatus.IDLE, + confirmation_policy=stored_conv.confirmation_policy, + ) + conversation_service._event_services[stored_conv.id] = mock_service + + assert await conversation_service.count_conversations() == 1 + assert await conversation_service.count_acp_conversations() == 2 + class TestConversationServiceStartConversation: """Test cases for ConversationService.start_conversation method.""" @@ -675,6 +716,16 @@ async def test_start_conversation_reuse_checks_is_open(self, conversation_servic # Create a mock event service that exists but is not open mock_event_service = AsyncMock(spec=EventService) mock_event_service.is_open.return_value = False + mock_event_service.stored = StoredConversation( + id=custom_id, + agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), + workspace=LocalWorkspace(working_dir="workspace/project"), + confirmation_policy=NeverConfirm(), + initial_message=None, + metrics=None, + created_at=datetime(2025, 1, 1, 12, 0, 0, tzinfo=UTC), + updated_at=datetime(2025, 1, 1, 12, 30, 0, tzinfo=UTC), + ) conversation_service._event_services[custom_id] = mock_event_service with tempfile.TemporaryDirectory() as temp_dir: @@ -764,6 +815,45 @@ async def test_start_conversation_reuse_when_open(self, conversation_service): assert not is_new mock_start.assert_not_called() + @pytest.mark.asyncio + async def test_start_conversation_rejects_existing_acp_conversation_id( + self, conversation_service + ): + custom_id = uuid4() + + mock_event_service = AsyncMock(spec=EventService) + mock_event_service.is_open.return_value = True + mock_event_service.stored = StoredConversation( + id=custom_id, + agent=ACPAgent(acp_command=["echo", "test"]), + workspace=LocalWorkspace(working_dir="workspace/project"), + confirmation_policy=NeverConfirm(), + initial_message=None, + metrics=None, + created_at=datetime(2025, 1, 1, 12, 0, 0, tzinfo=UTC), + updated_at=datetime(2025, 1, 1, 12, 30, 0, tzinfo=UTC), + ) + conversation_service._event_services[custom_id] = mock_event_service + + with tempfile.TemporaryDirectory() as temp_dir: + request = StartConversationRequest( + agent=Agent(llm=LLM(model="gpt-4o", usage_id="test-llm"), tools=[]), + workspace=LocalWorkspace(working_dir=temp_dir), + confirmation_policy=NeverConfirm(), + conversation_id=custom_id, + ) + + with patch.object( + conversation_service, "_start_event_service" + ) as mock_start: + with pytest.raises( + ConversationContractMismatchError, + match="only available through the ACP conversation contract", + ): + await conversation_service.start_conversation(request) + + mock_start.assert_not_called() + @pytest.mark.asyncio async def test_start_event_service_failure_cleanup(self, conversation_service): """Test that event service is cleaned up when startup fails.""" diff --git a/tests/agent_server/test_openapi_discriminator.py b/tests/agent_server/test_openapi_discriminator.py index 9e9f11e508..af2280eeea 100644 --- a/tests/agent_server/test_openapi_discriminator.py +++ b/tests/agent_server/test_openapi_discriminator.py @@ -200,3 +200,4 @@ def test_conversation_contracts_keep_v1_stable_and_add_acp_routes(client): assert "/api/v2/conversations" not in openapi_schema["paths"] assert "/api/acp/conversations" in openapi_schema["paths"] + assert "/api/acp/conversations/count" in openapi_schema["paths"] diff --git a/tests/sdk/conversation/remote/test_remote_conversation.py b/tests/sdk/conversation/remote/test_remote_conversation.py index 96fc4b9caa..350564fe1e 100644 --- a/tests/sdk/conversation/remote/test_remote_conversation.py +++ b/tests/sdk/conversation/remote/test_remote_conversation.py @@ -311,6 +311,11 @@ def request_side_effect(method, url, **kwargs): response.status_code = 404 response.raise_for_status.side_effect = None return response + elif method == "GET" and url == f"/api/acp/conversations/{conversation_id}": + response = Mock() + response.status_code = 404 + response.raise_for_status.side_effect = None + return response elif method == "POST" and url == "/api/conversations": return mock_conv_response elif method == "GET" and "/events/search" in url: @@ -373,6 +378,60 @@ def request_side_effect(method, url, **kwargs): "POST payload should contain the specified conversation_id" ) + @patch( + "openhands.sdk.conversation.impl.remote_conversation.WebSocketCallbackClient" + ) + def test_remote_conversation_existing_acp_id_raises_clear_error( + self, mock_ws_client + ): + conversation_id = uuid.uuid4() + mock_client_instance = Mock() + self.workspace._client = mock_client_instance + + def request_side_effect(method, url, **kwargs): + if method == "GET" and url == f"/api/conversations/{conversation_id}": + response = Mock() + response.status_code = 404 + response.raise_for_status.side_effect = None + return response + if method == "GET" and url == f"/api/acp/conversations/{conversation_id}": + response = Mock() + response.status_code = 200 + response.raise_for_status.return_value = None + response.json.return_value = { + "id": str(conversation_id), + "execution_status": "idle", + "agent": { + "kind": "ACPAgent", + "acp_command": ["echo", "test"], + }, + } + return response + response = Mock() + response.status_code = 200 + response.raise_for_status.return_value = None + response.json.return_value = {} + return response + + mock_client_instance.request.side_effect = request_side_effect + + with pytest.raises( + ValueError, match="only available through the ACP conversation contract" + ): + RemoteConversation( + agent=self.agent, + workspace=self.workspace, + conversation_id=conversation_id, + ) + + mock_ws_client.assert_not_called() + post_create_calls = [ + call + for call in mock_client_instance.request.call_args_list + if call[0][0] == "POST" + ] + assert post_create_calls == [] + @patch( "openhands.sdk.conversation.impl.remote_conversation.WebSocketCallbackClient" ) From f1be4586795f6f37caa6c8a895b90b1b769130fd Mon Sep 17 00:00:00 2001 From: Debug Agent Date: Mon, 16 Mar 2026 17:12:10 -0300 Subject: [PATCH 6/6] fix(agent-server): keep stored conversation ids hex-serialized Co-authored-by: openhands --- openhands-agent-server/openhands/agent_server/models.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openhands-agent-server/openhands/agent_server/models.py b/openhands-agent-server/openhands/agent_server/models.py index f90daaaf3a..906fc95087 100644 --- a/openhands-agent-server/openhands/agent_server/models.py +++ b/openhands-agent-server/openhands/agent_server/models.py @@ -172,7 +172,7 @@ class StoredConversation(StartACPConversationRequest): Extends StartConversationRequest with server-assigned fields. """ - id: UUID + id: OpenHandsUUID title: str | None = Field( default=None, description="User-defined title for the conversation" )