diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index af351e2fce..43bda63e8c 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -3,7 +3,9 @@ The Agent Client Protocol (ACP) lets OpenHands power conversations using ACP-compatible servers (Claude Code, Gemini CLI, etc.) instead of direct LLM calls. The ACP server manages its own LLM, tools, and execution; -the ACPAgent simply relays user messages and collects the response. +the ACPAgent relays user messages and collects the response. OpenHands +can still append prompt-only context, such as a skill catalog, to the +user message before it is sent to the ACP server. Unlike the built-in Agent, one ACP ``step()`` maps to one complete remote assistant turn. ACPAgent therefore emits a terminal ``FinishAction`` at the @@ -21,6 +23,7 @@ import time import uuid from collections.abc import Generator +from pathlib import Path from typing import TYPE_CHECKING, Any from acp.client.connection import ClientSideConnection @@ -144,7 +147,7 @@ def _make_dummy_llm() -> LLM: # 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. +# required credential source is present. # 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] = { @@ -152,18 +155,28 @@ def _make_dummy_llm() -> LLM: "openai-api-key": "OPENAI_API_KEY", "gemini-api-key": "GEMINI_API_KEY", } +_CHATGPT_AUTH_PATH = Path(".codex") / "auth.json" def _select_auth_method( auth_methods: list[Any], env: dict[str, str], ) -> str | None: - """Pick an auth method whose required env var is present. + """Pick an auth method whose required credentials are 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). + supported credential source is available (the server may not require auth). + + ChatGPT subscription login (device-code flow stored in + ``~/.codex/auth.json``) is checked first so it takes precedence over + explicit API keys, which serve as the fallback. """ method_ids = {m.id for m in auth_methods} + # Prefer ChatGPT subscription login when the auth file is present. + if "chatgpt" in method_ids: + if (Path.home() / _CHATGPT_AUTH_PATH).is_file(): + return "chatgpt" + # Fall back to explicit API key env vars. 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 @@ -810,7 +823,10 @@ def init_state( ) ) - # Validate no unsupported features + # Validate unsupported execution features. agent_context is allowed + # because it contributes prompt-only extensions to user messages; ACP + # server tools, MCP configuration, and context-window management remain + # owned by the server. if self.tools: raise NotImplementedError( "ACPAgent does not support custom tools; " @@ -826,11 +842,8 @@ def init_state( "ACPAgent does not support condenser; " "the ACP server manages its own context" ) - if self.agent_context is not None: - raise NotImplementedError( - "ACPAgent does not support agent_context; " - "configure the ACP server directly" - ) + if self.agent_context: + self.agent_context.validate_acp_compatibility() from openhands.sdk.utils.async_executor import AsyncExecutor @@ -1100,6 +1113,24 @@ def _cancel_inflight_tool_calls(self) -> None: exc_info=True, ) + def _build_acp_prompt(self, event: MessageEvent) -> str | None: + """Build the prompt text for one ACP user turn.""" + message = event.to_llm_message() + # Preserve all text blocks produced by the conversation layer, including + # any extended_content it already attached to the user turn. + text_parts = [ + content.text + for content in message.content + if isinstance(content, TextContent) and content.text.strip() + ] + if self.agent_context: + acp_prompt_context = self.agent_context.to_acp_prompt_context() + if acp_prompt_context: + text_parts.append(acp_prompt_context) + if not text_parts: + return None + return "\n\n".join(text_parts) + @observe(name="acp_agent.step", ignore_inputs=["conversation", "on_event"]) def step( self, @@ -1110,15 +1141,13 @@ def step( """Send the latest user message to the ACP server and emit the response.""" state = conversation.state - # Find the latest user message + # Find the latest user message. Conversation implementations already + # attach per-turn AgentContext extensions to MessageEvent.extended_content; + # MessageEvent.to_llm_message() merges those extensions with the user text. user_message = None for event in reversed(list(state.events)): if isinstance(event, MessageEvent) and event.source == "user": - # Extract text from the message - for content in event.llm_message.content: - if isinstance(content, TextContent) and content.text.strip(): - user_message = content.text - break + user_message = self._build_acp_prompt(event) if user_message: break diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index 27a72b0e86..6da4f65c95 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -50,12 +50,17 @@ class AgentContext(BaseModel): skills: list[Skill] = Field( default_factory=list, description="List of available skills that can extend the user's input.", + json_schema_extra={"acp_compatible": True}, ) system_message_suffix: str | None = Field( - default=None, description="Optional suffix to append to the system prompt." + default=None, + description="Optional suffix to append to the system prompt.", + json_schema_extra={"acp_compatible": True}, ) user_message_suffix: str | None = Field( - default=None, description="Optional suffix to append to the user's message." + default=None, + description="Optional suffix to append to the user's message.", + json_schema_extra={"acp_compatible": True}, ) load_user_skills: bool = Field( default=False, @@ -63,6 +68,7 @@ class AgentContext(BaseModel): "Whether to automatically load user skills from ~/.openhands/skills/ " "and ~/.openhands/microagents/ (for backward compatibility). " ), + json_schema_extra={"acp_compatible": True}, ) load_public_skills: bool = Field( default=False, @@ -71,6 +77,7 @@ class AgentContext(BaseModel): "skills repository at https://github.com/OpenHands/extensions. " "This allows you to get the latest skills without SDK updates." ), + json_schema_extra={"acp_compatible": True}, ) marketplace_path: str | None = Field( default=DEFAULT_MARKETPLACE_PATH, @@ -78,6 +85,7 @@ class AgentContext(BaseModel): "Relative marketplace JSON path within the public skills repository. " "Set to None to load all public skills without marketplace filtering." ), + json_schema_extra={"acp_compatible": True}, ) secrets: Mapping[str, SecretValue] | None = Field( default=None, @@ -87,6 +95,7 @@ class AgentContext(BaseModel): "Values can be either strings or SecretSource instances " "(str | SecretSource)." ), + json_schema_extra={"acp_compatible": False}, ) current_datetime: datetime | str | None = Field( default_factory=datetime.now, @@ -97,6 +106,7 @@ class AgentContext(BaseModel): "included in the system prompt to give the agent awareness of " "the current time context. Defaults to the current datetime." ), + json_schema_extra={"acp_compatible": True}, ) @field_validator("skills") @@ -169,6 +179,27 @@ def get_formatted_datetime(self) -> str | None: return self.current_datetime.isoformat() return self.current_datetime + def _partition_skills(self) -> tuple[list[Skill], list[Skill]]: + """Split skills into repo-context and available-skills lists. + + Categorization rules (shared by system-message and ACP adapters): + - AgentSkills-format: always in available_skills (progressive disclosure). + Triggers also auto-inject via ``get_user_message_suffix``. + - Legacy with ``trigger=None``: full content in REPO_CONTEXT (always active). + - Legacy with triggers: listed in available_skills, injected on trigger. + + Returns: + ``(repo_skills, available_skills)`` tuple. + """ + repo_skills: list[Skill] = [] + available_skills: list[Skill] = [] + for s in self.skills: + if s.is_agentskills_format or s.trigger is not None: + available_skills.append(s) + else: + repo_skills.append(s) + return repo_skills, available_skills + def get_system_message_suffix( self, llm_model: str | None = None, @@ -198,23 +229,7 @@ def get_system_message_suffix( - Legacy with trigger=None: Full content in (always active) - Legacy with triggers: Listed in , injected on trigger """ - # Categorize skills based on format and trigger: - # - AgentSkills-format: always in available_skills (progressive disclosure) - # - Legacy: trigger=None -> REPO_CONTEXT, else -> available_skills - repo_skills: list[Skill] = [] - available_skills: list[Skill] = [] - - for s in self.skills: - if s.is_agentskills_format: - # AgentSkills: always list (triggers also auto-inject via - # get_user_message_suffix) - available_skills.append(s) - elif s.trigger is None: - # Legacy OpenHands: no trigger = full content in REPO_CONTEXT - repo_skills.append(s) - else: - # Legacy OpenHands: has trigger = list in available_skills - available_skills.append(s) + repo_skills, available_skills = self._partition_skills() # Gate vendor-specific repo skills based on model family. if llm_model or llm_model_canonical: @@ -277,6 +292,46 @@ def get_system_message_suffix( return self.system_message_suffix.strip() return None + def validate_acp_compatibility(self) -> None: + """Raise if this context uses fields unsupported by ACP prompt mode. + + Compatibility is determined by the ``acp_compatible`` tag in each + field's ``json_schema_extra``. + """ + acp_compatible = { + name + for name, info in type(self).model_fields.items() + if isinstance(info.json_schema_extra, dict) + and info.json_schema_extra.get("acp_compatible") is True + } + unsupported = set(self.model_fields_set) - acp_compatible + if unsupported: + fields = ", ".join(sorted(unsupported)) + raise NotImplementedError( + f"ACP prompt context does not support AgentContext field(s): {fields}" + ) + + def to_acp_prompt_context(self) -> str | None: + """Return the AgentContext fields that ACP can consume as prompt text. + + ACP servers own their tools, MCP servers, hooks, and execution model, so + this adapter only emits prompt-only context. Unsupported AgentContext + semantics (e.g. secrets) are rejected by + :meth:`validate_acp_compatibility`. + + The rendering reuses :meth:`get_system_message_suffix` with the same + ``system_message_suffix.j2`` template so that ACP agents receive the + identical prompt layout as the general agent (minus secrets). + + ``user_message_suffix`` is a compatible field but is not emitted here + because ``LocalConversation`` already applies it through + ``event.to_llm_message()``; including it would duplicate it. + """ + self.validate_acp_compatibility() + # ACP doesn't support secrets (enforced above) and has no model-specific + # skill filtering, so we delegate to the shared renderer with no extras. + return self.get_system_message_suffix() + def get_user_message_suffix( self, user_message: Message, skip_skill_names: list[str] ) -> tuple[TextContent, list[str]] | None: diff --git a/openhands-sdk/openhands/sdk/settings/model.py b/openhands-sdk/openhands/sdk/settings/model.py index 94666735e8..0378785919 100644 --- a/openhands-sdk/openhands/sdk/settings/model.py +++ b/openhands-sdk/openhands/sdk/settings/model.py @@ -533,9 +533,9 @@ def _start_request_kwargs(self, **kwargs: Any) -> dict[str, Any]: payload["agent"] = self.agent_settings.create_agent() # --- secrets (from agent's context) --------------------------------- - # ACPAgent doesn't carry an ``agent_context`` at all; its context is - # owned by the subprocess. ``getattr(..., None)`` keeps this no-op - # for the ACP variant. + # ACPAgent may carry prompt-only context, but its execution context is + # owned by the subprocess. ``getattr(..., None)`` keeps this no-op for + # agents without AgentContext. agent = payload.get("agent") if "secrets" not in payload and agent is not None: ctx = getattr(agent, "agent_context", None) diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index b7e6c8260b..d63170bd78 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -23,12 +23,14 @@ _serialize_tool_content, ) from openhands.sdk.agent.base import AgentBase +from openhands.sdk.context import AgentContext from openhands.sdk.conversation.state import ( ConversationExecutionStatus, ConversationState, ) from openhands.sdk.event import ACPToolCallEvent, MessageEvent, SystemPromptEvent from openhands.sdk.llm import Message, TextContent +from openhands.sdk.skills import KeywordTrigger, Skill from openhands.sdk.workspace.local import LocalWorkspace @@ -193,6 +195,154 @@ def test_rejects_mcp_config(self, tmp_path): with pytest.raises(NotImplementedError, match="mcp_config"): self._init_with_patches(agent, tmp_path) + def test_allows_agent_context_for_prompt_extensions(self, tmp_path): + agent = ACPAgent( + acp_command=["echo"], + agent_context=AgentContext( + skills=[ + Skill( + name="review", + content="Review instructions", + trigger=KeywordTrigger(keywords=["/review"]), + ) + ] + ), + ) + + self._init_with_patches(agent, tmp_path) + + def test_rejects_unsupported_agent_context_at_init(self, tmp_path): + agent = ACPAgent( + acp_command=["echo"], + agent_context=AgentContext(secrets={"GITHUB_TOKEN": "ghp_secret"}), + ) + with pytest.raises(NotImplementedError, match="secrets"): + self._init_with_patches(agent, tmp_path) + + def test_agent_context_to_acp_prompt_context(self): + context = AgentContext( + skills=[ + Skill( + name="review", + content="Full review instructions", + trigger=KeywordTrigger(keywords=["/review"]), + description="Review pull requests.", + ) + ], + system_message_suffix="Follow repository rules.", + user_message_suffix="Prefer concise responses.", + current_datetime="2026-04-24T00:00:00", + ) + + prompt = context.to_acp_prompt_context() + + assert prompt is not None + # Reuses the same system_message_suffix.j2 template as the general + # agent, so the rendered sections are identical. + assert "" in prompt + assert "2026-04-24T00:00:00" in prompt + assert "review" in prompt + assert "Review pull requests." in prompt + assert "Full review instructions" not in prompt + assert "Follow repository rules." in prompt + # user_message_suffix is not emitted by to_acp_prompt_context because + # LocalConversation already applies it via event.to_llm_message(). + assert "Prefer concise responses." not in prompt + + def test_agent_context_to_acp_prompt_context_returns_none_when_empty(self): + context = AgentContext(skills=[], current_datetime=None) + + assert context.to_acp_prompt_context() is None + + def test_agent_context_to_acp_prompt_context_emits_datetime_by_default(self): + context = AgentContext(skills=[]) + + prompt = context.to_acp_prompt_context() + assert prompt is not None + assert "" in prompt + + def test_agent_context_to_acp_prompt_context_rejects_secrets(self): + context = AgentContext(secrets={"GITHUB_TOKEN": "ghp_secret"}) + + with pytest.raises(NotImplementedError, match="secrets"): + context.to_acp_prompt_context() + + def test_agent_context_to_acp_prompt_context_includes_legacy_repo_skills(self): + context = AgentContext( + skills=[ + Skill( + name="claude", + content="Always follow the repository review checklist.", + trigger=None, + ), + Skill( + name="repo-skill", + content="Full AgentSkills instructions should stay out.", + description="Use repo-specific tools.", + is_agentskills_format=True, + ), + ], + current_datetime=None, + ) + + prompt = context.to_acp_prompt_context() + + assert prompt is not None + assert "" in prompt + assert "[BEGIN context from [claude]]" in prompt + assert "Always follow the repository review checklist." in prompt + assert "repo-skill" in prompt + assert "Use repo-specific tools." in prompt + assert "Full AgentSkills instructions should stay out." not in prompt + assert "claude" not in prompt + + def test_agent_context_to_acp_prompt_context_lists_legacy_triggered_skills(self): + context = AgentContext( + skills=[ + Skill( + name="roasted-review", + content="Use a stricter review tone.", + trigger=KeywordTrigger(keywords=["/roasted"]), + description="Run a stricter review.", + ) + ], + current_datetime=None, + ) + + prompt = context.to_acp_prompt_context() + + assert prompt is not None + assert "" not in prompt + assert "roasted-review" in prompt + assert "Run a stricter review." in prompt + assert "Use a stricter review tone." not in prompt + + def test_build_acp_prompt_preserves_all_text_blocks(self): + agent = _make_agent( + agent_context=AgentContext( + user_message_suffix="Prefer concise responses.", + current_datetime=None, + ) + ) + event = MessageEvent( + source="user", + llm_message=Message( + role="user", + content=[ + TextContent(text="First block."), + TextContent(text="Second block."), + ], + ), + extended_content=[TextContent(text="Prefer concise responses.")], + ) + + prompt = agent._build_acp_prompt(event) + + assert prompt is not None + assert "First block." in prompt + assert "Second block." in prompt + assert prompt.count("Prefer concise responses.") == 1 + # --------------------------------------------------------------------------- # init_state @@ -557,6 +707,166 @@ def _fake_run_async(_coro, **_kwargs): assert isinstance(content_block, TextContent) assert content_block.text == "The answer is 4" + @staticmethod + def _wire_passthrough_mocks(agent: ACPAgent) -> None: + """Wire mock ACP internals that relay prompt() calls through asyncio.""" + mock_client = _OpenHandsACPBridge() + mock_client.get_turn_usage_update = MagicMock(return_value=object()) + agent._client = mock_client + agent._conn = MagicMock() + agent._conn.prompt = AsyncMock(return_value=None) + agent._session_id = "test-session" + + def _fake_run_async(coro_factory, **_kwargs): + return asyncio.run(coro_factory()) + + mock_executor = MagicMock() + mock_executor.run_async = _fake_run_async + agent._executor = mock_executor + + def test_step_sends_skill_catalog_to_acp_server(self, tmp_path): + agent = _make_agent( + agent_context=AgentContext( + skills=[ + Skill( + name="review", + content="Full review instructions that ACP should not receive.", + trigger=KeywordTrigger(keywords=["/review"]), + description="Review pull requests.", + ) + ] + ) + ) + state = _make_state(tmp_path) + state.events.append( + MessageEvent( + source="user", + llm_message=Message( + role="user", + content=[TextContent(text="Review this PR.")], + ), + extended_content=[ + TextContent( + text="Use strict review." + ) + ], + ) + ) + conversation = MagicMock() + conversation.state = state + self._wire_passthrough_mocks(agent) + + agent.step(conversation, on_event=lambda _: None) + + prompt_call = agent._conn.prompt.await_args + assert prompt_call is not None + prompt_blocks = prompt_call.args[0] + assert len(prompt_blocks) == 1 + prompt_text = prompt_blocks[0].text + assert "Review this PR." in prompt_text + assert "review" in prompt_text + assert "Review pull requests." in prompt_text + assert "Use strict review." in prompt_text + assert ( + "Full review instructions that ACP should not receive." not in prompt_text + ) + + def test_step_sends_legacy_repo_context_to_acp_server(self, tmp_path): + agent = _make_agent( + agent_context=AgentContext( + skills=[ + Skill( + name="claude", + content="Always follow repository-specific review rules.", + trigger=None, + ), + Skill( + name="agent-skill", + content="AgentSkills full instructions should not be sent.", + is_agentskills_format=True, + description="Use the agent skill catalog entry.", + ), + ], + current_datetime=None, + ) + ) + state = _make_state(tmp_path) + state.events.append( + MessageEvent( + source="user", + llm_message=Message( + role="user", + content=[TextContent(text="Review this PR.")], + ), + ) + ) + conversation = MagicMock() + conversation.state = state + self._wire_passthrough_mocks(agent) + + agent.step(conversation, on_event=lambda _: None) + + prompt_call = agent._conn.prompt.await_args + assert prompt_call is not None + prompt_text = prompt_call.args[0][0].text + assert "Review this PR." in prompt_text + assert "" in prompt_text + assert "Always follow repository-specific review rules." in prompt_text + assert "agent-skill" in prompt_text + assert ( + "Use the agent skill catalog entry." + in prompt_text + ) + assert "AgentSkills full instructions should not be sent." not in prompt_text + + def test_step_sends_triggered_skill_content_to_acp_server(self, tmp_path): + agent = _make_agent( + agent_context=AgentContext( + skills=[ + Skill( + name="legacy-review", + content="Legacy triggered review instructions.", + trigger=KeywordTrigger(keywords=["/review"]), + ), + Skill( + name="agentskill-review", + content="AgentSkills triggered review instructions.", + trigger=KeywordTrigger(keywords=["/review"]), + is_agentskills_format=True, + description="AgentSkills review catalog.", + ), + ], + current_datetime=None, + ) + ) + state = _make_state(tmp_path) + state.events.append( + MessageEvent( + source="user", + llm_message=Message( + role="user", + content=[TextContent(text="/review this PR.")], + ), + extended_content=[ + TextContent(text="Legacy triggered review instructions."), + TextContent(text="AgentSkills triggered review instructions."), + ], + ) + ) + conversation = MagicMock() + conversation.state = state + self._wire_passthrough_mocks(agent) + + agent.step(conversation, on_event=lambda _: None) + + prompt_call = agent._conn.prompt.await_args + assert prompt_call is not None + prompt_text = prompt_call.args[0][0].text + assert "Legacy triggered review instructions." in prompt_text + assert "AgentSkills triggered review instructions." in prompt_text + assert "agentskill-review" in prompt_text + assert "AgentSkills review catalog." in prompt_text + def test_step_includes_reasoning(self, tmp_path): agent = _make_agent() conversation = self._make_conversation_with_message(tmp_path) @@ -2100,14 +2410,13 @@ def _make_auth_method(method_id: str) -> MagicMock: 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): + def test_codex_api_key_preferred_over_openai(self): """CODEX_API_KEY is checked first (appears first in the map).""" methods = [ self._make_auth_method("codex-api-key"), @@ -2116,7 +2425,31 @@ def test_codex_api_key_preferred(self): 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): + def test_chatgpt_preferred_over_api_key(self, tmp_path): + """ChatGPT subscription login takes precedence over API keys.""" + methods = [ + self._make_auth_method("chatgpt"), + self._make_auth_method("openai-api-key"), + ] + auth_dir = tmp_path / ".codex" + auth_dir.mkdir() + (auth_dir / "auth.json").write_text("{}", encoding="utf-8") + + env = {"OPENAI_API_KEY": "sk-test"} + with patch("openhands.sdk.agent.acp_agent.Path.home", return_value=tmp_path): + assert _select_auth_method(methods, env) == "chatgpt" + + def test_api_key_fallback_when_no_chatgpt_file(self): + """Falls back to API key when chatgpt is offered but auth file absent.""" + methods = [ + self._make_auth_method("chatgpt"), + self._make_auth_method("openai-api-key"), + ] + env = {"OPENAI_API_KEY": "sk-test"} + # Path.home() points to a real dir without .codex/auth.json + assert _select_auth_method(methods, env) == "openai-api-key" + + def test_no_matching_credentials(self): methods = [ self._make_auth_method("chatgpt"), self._make_auth_method("openai-api-key"), @@ -2124,6 +2457,15 @@ def test_no_matching_env_var(self): env = {"UNRELATED": "value"} assert _select_auth_method(methods, env) is None + def test_chatgpt_auth_file(self, tmp_path): + methods = [self._make_auth_method("chatgpt")] + auth_dir = tmp_path / ".codex" + auth_dir.mkdir() + (auth_dir / "auth.json").write_text("{}", encoding="utf-8") + + with patch("openhands.sdk.agent.acp_agent.Path.home", return_value=tmp_path): + assert _select_auth_method(methods, {}) == "chatgpt" + def test_empty_auth_methods(self): assert _select_auth_method([], {}) is None