From d5f37757635b29ad5f65e97738f69b0c18c7bcf3 Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Fri, 24 Apr 2026 19:11:19 +0000 Subject: [PATCH 01/27] Append AgentContext prompt extensions for ACP agents --- .../openhands/sdk/agent/acp_agent.py | 30 ++++++---- openhands-sdk/openhands/sdk/settings/model.py | 6 +- tests/sdk/agent/test_acp_agent.py | 60 +++++++++++++++++++ 3 files changed, 80 insertions(+), 16 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index af351e2fce..35685f4715 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 activated skills, 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 @@ -810,7 +812,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 +831,6 @@ 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" - ) from openhands.sdk.utils.async_executor import AsyncExecutor @@ -1110,15 +1110,19 @@ 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, including any prompt-only extensions + # that Conversation.send_message() attached from AgentContext skills. 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 + message = event.to_llm_message() + text_parts = [ + content.text + for content in message.content + if isinstance(content, TextContent) and content.text.strip() + ] + if text_parts: + user_message = "\n\n".join(text_parts) if user_message: break diff --git a/openhands-sdk/openhands/sdk/settings/model.py b/openhands-sdk/openhands/sdk/settings/model.py index a3da50bfcf..3dd6bd4446 100644 --- a/openhands-sdk/openhands/sdk/settings/model.py +++ b/openhands-sdk/openhands/sdk/settings/model.py @@ -436,9 +436,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..a4a603062a 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,22 @@ 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) + # --------------------------------------------------------------------------- # init_state @@ -557,6 +575,48 @@ def _fake_run_async(_coro, **_kwargs): assert isinstance(content_block, TextContent) assert content_block.text == "The answer is 4" + def test_step_sends_extended_content_to_acp_server(self, tmp_path): + agent = _make_agent() + 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 + + 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 + + agent.step(conversation, on_event=lambda _: None) + + prompt_blocks = agent._conn.prompt.await_args.args[0] + assert len(prompt_blocks) == 1 + prompt_text = prompt_blocks[0].text + assert "Review this PR." in prompt_text + assert "Use strict review." in prompt_text + def test_step_includes_reasoning(self, tmp_path): agent = _make_agent() conversation = self._make_conversation_with_message(tmp_path) From c6c05288f71a8d84dc0b8db6916ad3f5c2c13a33 Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Fri, 24 Apr 2026 19:34:39 +0000 Subject: [PATCH 02/27] Authenticate ACP ChatGPT sessions from Codex auth --- openhands-sdk/openhands/sdk/agent/acp_agent.py | 12 +++++++++--- tests/sdk/agent/test_acp_agent.py | 8 ++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index 35685f4715..a13553201b 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -23,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 @@ -146,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] = { @@ -154,21 +155,26 @@ 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). """ 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 + if "chatgpt" in method_ids: + home = env.get("HOME") + if home and (Path(home) / _CHATGPT_AUTH_PATH).is_file(): + return "chatgpt" return None diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index a4a603062a..0498a70dec 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -2184,6 +2184,14 @@ 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") + + assert _select_auth_method(methods, {"HOME": str(tmp_path)}) == "chatgpt" + def test_empty_auth_methods(self): assert _select_auth_method([], {}) is None From b75e8954d7f2d45d57bf0346786a4a1e58066cca Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Fri, 24 Apr 2026 19:49:11 +0000 Subject: [PATCH 03/27] Fix ACP test typing --- tests/sdk/agent/test_acp_agent.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index 0498a70dec..9d60c0a9b9 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -611,7 +611,9 @@ def _fake_run_async(coro_factory, **_kwargs): agent.step(conversation, on_event=lambda _: None) - prompt_blocks = agent._conn.prompt.await_args.args[0] + 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 From ba9263f517ea11213a09d0967298d0eea558c790 Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Fri, 24 Apr 2026 19:54:54 +0000 Subject: [PATCH 04/27] Limit ACP AgentContext to skill catalog --- .../openhands/sdk/agent/acp_agent.py | 13 +++++--- .../openhands/sdk/context/agent_context.py | 33 +++++++++++++++++++ tests/sdk/agent/test_acp_agent.py | 22 +++++++++++-- 3 files changed, 61 insertions(+), 7 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index a13553201b..611cd758f7 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -4,7 +4,7 @@ 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 relays user messages and collects the response. OpenHands -can still append prompt-only context, such as activated skills, to the +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 @@ -1116,17 +1116,22 @@ def step( """Send the latest user message to the ACP server and emit the response.""" state = conversation.state - # Find the latest user message, including any prompt-only extensions - # that Conversation.send_message() attached from AgentContext skills. + # Find the latest user message. ACP receives only a prompt-only skill + # catalog from AgentContext; triggered skill content remains an OpenHands + # Agent feature because ACP servers own their own execution model. user_message = None for event in reversed(list(state.events)): if isinstance(event, MessageEvent) and event.source == "user": - message = event.to_llm_message() + message = event.llm_message text_parts = [ content.text for content in message.content if isinstance(content, TextContent) and content.text.strip() ] + if self.agent_context: + catalog_prompt = self.agent_context.get_skill_catalog_prompt() + if catalog_prompt: + text_parts.append(catalog_prompt) if text_parts: user_message = "\n\n".join(text_parts) if user_message: diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index 27a72b0e86..b69e42c981 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -3,6 +3,7 @@ import pathlib from collections.abc import Mapping from datetime import datetime +from xml.sax.saxutils import escape as xml_escape from pydantic import BaseModel, Field, field_validator, model_validator @@ -277,6 +278,38 @@ def get_system_message_suffix( return self.system_message_suffix.strip() return None + def get_skill_catalog_prompt(self) -> str | None: + """Return a prompt-only catalog of available skills. + + This is intentionally limited to skill names and descriptions. It does not + include full skill content, trigger-matched knowledge, file locations, + secrets, or execution guidance. ACP servers own tool execution, so SDK + consumers should only use this catalog to provide lightweight awareness of + SDK-loaded skills and plugin-contributed skills. + """ + if not self.skills: + return None + + lines = ["", "The following skills are available:"] + lines.append("") + for skill in self.skills: + name = xml_escape(skill.name.strip()) + description = skill.description + if not description: + for line in skill.content.split("\n"): + stripped = line.strip() + if stripped and not stripped.startswith("#"): + description = stripped + break + description = xml_escape((description or "").strip()) + lines.append(" ") + lines.append(f" {name}") + lines.append(f" {description}") + lines.append(" ") + lines.append("") + lines.append("") + return "\n".join(lines) + def get_user_message_suffix( self, user_message: Message, skip_skill_names: list[str] ) -> tuple[TextContent, list[str]] | None: diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index 9d60c0a9b9..e260206636 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -575,8 +575,19 @@ def _fake_run_async(_coro, **_kwargs): assert isinstance(content_block, TextContent) assert content_block.text == "The answer is 4" - def test_step_sends_extended_content_to_acp_server(self, tmp_path): - agent = _make_agent() + 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( @@ -617,7 +628,12 @@ def _fake_run_async(coro_factory, **_kwargs): assert len(prompt_blocks) == 1 prompt_text = prompt_blocks[0].text assert "Review this PR." in prompt_text - assert "Use strict review." in prompt_text + assert "review" in prompt_text + assert "Review pull requests." in prompt_text + assert "Use strict review." not in prompt_text + assert ( + "Full review instructions that ACP should not receive." not in prompt_text + ) def test_step_includes_reasoning(self, tmp_path): agent = _make_agent() From 8d08e108aa77f6b2ad75bab2af0a579a498387ec Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Fri, 24 Apr 2026 20:15:10 +0000 Subject: [PATCH 05/27] Add ACP prompt adapter for AgentContext --- .../openhands/sdk/agent/acp_agent.py | 13 +- .../openhands/sdk/context/agent_context.py | 146 ++++++++++++++---- tests/sdk/agent/test_acp_agent.py | 56 +++++++ 3 files changed, 183 insertions(+), 32 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index 611cd758f7..a3dbd0bfb6 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -1129,9 +1129,16 @@ def step( if isinstance(content, TextContent) and content.text.strip() ] if self.agent_context: - catalog_prompt = self.agent_context.get_skill_catalog_prompt() - if catalog_prompt: - text_parts.append(catalog_prompt) + acp_prompt_context = self.agent_context.to_acp_prompt_context( + include_skill_catalog=True, + include_system_suffix=True, + include_user_suffix=True, + include_secret_catalog=True, + include_current_datetime=True, + include_full_skill_content=False, + ) + if acp_prompt_context: + text_parts.append(acp_prompt_context) if text_parts: user_message = "\n\n".join(text_parts) if user_message: diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index b69e42c981..50801d1239 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -278,37 +278,125 @@ def get_system_message_suffix( return self.system_message_suffix.strip() return None - def get_skill_catalog_prompt(self) -> str | None: - """Return a prompt-only catalog of available skills. - - This is intentionally limited to skill names and descriptions. It does not - include full skill content, trigger-matched knowledge, file locations, - secrets, or execution guidance. ACP servers own tool execution, so SDK - consumers should only use this catalog to provide lightweight awareness of - SDK-loaded skills and plugin-contributed skills. + def to_acp_prompt_context( + self, + *, + include_skill_catalog: bool = True, + include_system_suffix: bool = True, + include_user_suffix: bool = True, + include_secret_catalog: bool = True, + include_current_datetime: bool = True, + include_full_skill_content: bool = False, + ) -> 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 must fail here instead of being silently approximated by + ACPAgent. """ - if not self.skills: - return None + supported_fields = { + "skills", + "system_message_suffix", + "user_message_suffix", + "load_user_skills", + "load_public_skills", + "marketplace_path", + "secrets", + "current_datetime", + } + unsupported_fields = set(type(self).model_fields) - supported_fields + if unsupported_fields: + fields = ", ".join(sorted(unsupported_fields)) + raise NotImplementedError( + "ACP prompt context does not support AgentContext field(s): " + f"{fields}" + ) - lines = ["", "The following skills are available:"] - lines.append("") - for skill in self.skills: - name = xml_escape(skill.name.strip()) - description = skill.description - if not description: - for line in skill.content.split("\n"): - stripped = line.strip() - if stripped and not stripped.startswith("#"): - description = stripped - break - description = xml_escape((description or "").strip()) - lines.append(" ") - lines.append(f" {name}") - lines.append(f" {description}") - lines.append(" ") - lines.append("") - lines.append("") - return "\n".join(lines) + parts: list[str] = [] + + if include_current_datetime: + formatted_datetime = self.get_formatted_datetime() + if formatted_datetime: + parts.append( + "\n".join( + [ + "", + f"The current date and time is: {formatted_datetime}", + "", + ] + ) + ) + + if include_skill_catalog and self.skills: + lines = ["", "The following skills are available:"] + lines.append("") + for skill in self.skills: + name = xml_escape(skill.name.strip()) + description = xml_escape((skill.description or "").strip()) + lines.append(" ") + lines.append(f" {name}") + lines.append(f" {description}") + if include_full_skill_content: + content = xml_escape(skill.content.strip()) + lines.append(f" {content}") + lines.append(" ") + lines.append("") + lines.append("") + parts.append("\n".join(lines)) + + if include_system_suffix and self.system_message_suffix: + system_suffix = self.system_message_suffix.strip() + if system_suffix: + parts.append( + "\n".join( + [ + "", + system_suffix, + "", + ] + ) + ) + + if include_user_suffix and self.user_message_suffix: + user_suffix = self.user_message_suffix.strip() + if user_suffix: + parts.append( + "\n".join( + [ + "", + user_suffix, + "", + ] + ) + ) + + if include_secret_catalog: + secret_infos = self.get_secret_infos() + if secret_infos: + lines = [ + "", + ( + "The following secret names are registered. " + "Secret values are not included." + ), + ] + for secret_info in secret_infos: + name = xml_escape(secret_info["name"] or "") + description = xml_escape((secret_info["description"] or "").strip()) + if description: + lines.append( + f" {name}" + f"{description}" + ) + else: + lines.append(f" {name}") + lines.append("") + parts.append("\n".join(lines)) + + if not parts: + return None + return "\n\n".join(parts) def get_user_message_suffix( self, user_message: Message, skip_skill_names: list[str] diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index e260206636..9bf77b37f9 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -10,6 +10,7 @@ import pytest from acp.exceptions import RequestError as ACPRequestError +from pydantic import SecretStr from openhands.sdk.agent.acp_agent import ( ACPAgent, @@ -30,6 +31,7 @@ ) from openhands.sdk.event import ACPToolCallEvent, MessageEvent, SystemPromptEvent from openhands.sdk.llm import Message, TextContent +from openhands.sdk.secret import StaticSecret from openhands.sdk.skills import KeywordTrigger, Skill from openhands.sdk.workspace.local import LocalWorkspace @@ -211,6 +213,60 @@ def test_allows_agent_context_for_prompt_extensions(self, tmp_path): 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.", + secrets={ + "GITHUB_TOKEN": StaticSecret( + value=SecretStr("ghp_secret"), + description="GitHub API token", + ) + }, + current_datetime="2026-04-24T00:00:00", + ) + + prompt = context.to_acp_prompt_context() + + assert prompt is not None + 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 "" in prompt + assert "Follow repository rules." in prompt + assert "" in prompt + assert "Prefer concise responses." in prompt + assert "" in prompt + assert "GITHUB_TOKEN" in prompt + assert "GitHub API token" in prompt + assert "ghp_secret" not in prompt + + def test_agent_context_to_acp_prompt_context_can_include_full_skill_content(self): + context = AgentContext( + skills=[ + Skill( + name="review", + content="Full review instructions", + description="Review pull requests.", + ) + ] + ) + + prompt = context.to_acp_prompt_context(include_full_skill_content=True) + + assert prompt is not None + assert "Full review instructions" in prompt + # --------------------------------------------------------------------------- # init_state From d36918dec4989e7462325d8f8e1cbe15f5b25c37 Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Sat, 25 Apr 2026 04:12:28 +0000 Subject: [PATCH 06/27] Apply ACP context formatting --- openhands-sdk/openhands/sdk/context/agent_context.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index 50801d1239..4300379657 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -309,8 +309,7 @@ def to_acp_prompt_context( if unsupported_fields: fields = ", ".join(sorted(unsupported_fields)) raise NotImplementedError( - "ACP prompt context does not support AgentContext field(s): " - f"{fields}" + f"ACP prompt context does not support AgentContext field(s): {fields}" ) parts: list[str] = [] From b7847b8e27f91d2c4992e4c941eafb8bbeae9432 Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Sat, 25 Apr 2026 14:37:43 +0000 Subject: [PATCH 07/27] Preserve legacy skills in ACP prompt context --- .../openhands/sdk/agent/acp_agent.py | 10 + .../openhands/sdk/context/agent_context.py | 50 ++++- tests/sdk/agent/test_acp_agent.py | 208 ++++++++++++++++++ 3 files changed, 262 insertions(+), 6 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index a3dbd0bfb6..0310806a79 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -1129,6 +1129,16 @@ def step( if isinstance(content, TextContent) and content.text.strip() ] if self.agent_context: + legacy_user_context = self.agent_context.get_user_message_suffix( + user_message=message, + skip_skill_names=[], + include_agentskills_format=False, + include_user_message_suffix=False, + ) + if legacy_user_context: + content, _activated_skill_names = legacy_user_context + if content.text.strip(): + text_parts.append(content.text) acp_prompt_context = self.agent_context.to_acp_prompt_context( include_skill_catalog=True, include_system_suffix=True, diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index 4300379657..bbf4d57beb 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -314,6 +314,14 @@ def to_acp_prompt_context( parts: list[str] = [] + repo_skills: list[Skill] = [] + available_skills: list[Skill] = [] + for skill in self.skills: + if skill.is_agentskills_format or skill.trigger is not None: + available_skills.append(skill) + else: + repo_skills.append(skill) + if include_current_datetime: formatted_datetime = self.get_formatted_datetime() if formatted_datetime: @@ -327,10 +335,26 @@ def to_acp_prompt_context( ) ) - if include_skill_catalog and self.skills: - lines = ["", "The following skills are available:"] - lines.append("") - for skill in self.skills: + if repo_skills: + repo_context = render_template( + prompt_dir=str(PROMPT_DIR), + template_name="system_message_suffix.j2", + repo_skills=repo_skills, + system_message_suffix="", + secret_infos=[], + available_skills_prompt="", + current_datetime=None, + ).strip() + if repo_context: + parts.append(repo_context) + + if include_skill_catalog and available_skills: + lines = [ + "", + "The following skills are available:", + "", + ] + for skill in available_skills: name = xml_escape(skill.name.strip()) description = xml_escape((skill.description or "").strip()) lines.append(" ") @@ -398,7 +422,13 @@ def to_acp_prompt_context( return "\n\n".join(parts) def get_user_message_suffix( - self, user_message: Message, skip_skill_names: list[str] + self, + user_message: Message, + skip_skill_names: list[str], + *, + include_agentskills_format: bool = True, + include_legacy_skills: bool = True, + include_user_message_suffix: bool = True, ) -> tuple[TextContent, list[str]] | None: """Augment the user’s message with knowledge recalled from skills. @@ -409,7 +439,11 @@ def get_user_message_suffix( """ # noqa: E501 user_message_suffix = None - if self.user_message_suffix and self.user_message_suffix.strip(): + if ( + include_user_message_suffix + and self.user_message_suffix + and self.user_message_suffix.strip() + ): user_message_suffix = self.user_message_suffix.strip() query = "\n".join( @@ -425,6 +459,10 @@ def get_user_message_suffix( for skill in self.skills: if not isinstance(skill, Skill): continue + if skill.is_agentskills_format and not include_agentskills_format: + continue + if not skill.is_agentskills_format and not include_legacy_skills: + continue trigger = skill.match_trigger(query) if trigger and skill.name not in skip_skill_names: logger.info( diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index 9bf77b37f9..5344712af0 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -251,6 +251,56 @@ def test_agent_context_to_acp_prompt_context(self): assert "GitHub API token" in prompt assert "ghp_secret" not in prompt + 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_agent_context_to_acp_prompt_context_can_include_full_skill_content(self): context = AgentContext( skills=[ @@ -258,6 +308,7 @@ def test_agent_context_to_acp_prompt_context_can_include_full_skill_content(self name="review", content="Full review instructions", description="Review pull requests.", + is_agentskills_format=True, ) ] ) @@ -267,6 +318,40 @@ def test_agent_context_to_acp_prompt_context_can_include_full_skill_content(self assert prompt is not None assert "Full review instructions" in prompt + def test_agent_context_acp_legacy_trigger_suffix_excludes_agentskills(self): + context = AgentContext( + skills=[ + Skill( + name="legacy-review", + content="Legacy triggered instructions.", + trigger=KeywordTrigger(keywords=["/review"]), + ), + Skill( + name="agentskill-review", + content="AgentSkills triggered instructions.", + trigger=KeywordTrigger(keywords=["/review"]), + is_agentskills_format=True, + ), + ] + ) + message = Message( + role="user", + content=[TextContent(text="/review this change")], + ) + + suffix = context.get_user_message_suffix( + message, + skip_skill_names=[], + include_agentskills_format=False, + include_user_message_suffix=False, + ) + + assert suffix is not None + content, activated_skills = suffix + assert activated_skills == ["legacy-review"] + assert "Legacy triggered instructions." in content.text + assert "AgentSkills triggered instructions." not in content.text + # --------------------------------------------------------------------------- # init_state @@ -691,6 +776,129 @@ def _fake_run_async(coro_factory, **_kwargs): "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.")], + ), + extended_content=[ + TextContent( + text="AgentSkills full instructions should not be sent." + ) + ], + ) + ) + conversation = MagicMock() + conversation.state = state + + 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 + + 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_legacy_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.")], + ), + ) + ) + conversation = MagicMock() + conversation.state = state + + 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 + + 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." not 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) From 16aabe62a65aa2b82c5dff838f6184bf6da01024 Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Sat, 25 Apr 2026 14:51:49 +0000 Subject: [PATCH 08/27] Load repo AgentSkills in ACP review context --- .../openhands/sdk/agent/acp_agent.py | 9 ++++--- openhands-sdk/openhands/sdk/skills/skill.py | 16 +++++++----- tests/sdk/agent/test_acp_agent.py | 4 +-- tests/sdk/skills/test_load_project_skills.py | 26 +++++++++++++++++++ 4 files changed, 43 insertions(+), 12 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index 0310806a79..ffad761938 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -1116,9 +1116,10 @@ def step( """Send the latest user message to the ACP server and emit the response.""" state = conversation.state - # Find the latest user message. ACP receives only a prompt-only skill - # catalog from AgentContext; triggered skill content remains an OpenHands - # Agent feature because ACP servers own their own execution model. + # Find the latest user message. ACP receives AgentSkills as a prompt-only + # catalog by default, but explicit keyword triggers still contribute the + # matched skill content as prompt text because ACP has no invoke_skill + # tool path to retrieve it later. user_message = None for event in reversed(list(state.events)): if isinstance(event, MessageEvent) and event.source == "user": @@ -1132,7 +1133,7 @@ def step( legacy_user_context = self.agent_context.get_user_message_suffix( user_message=message, skip_skill_names=[], - include_agentskills_format=False, + include_agentskills_format=True, include_user_message_suffix=False, ) if legacy_user_context: diff --git a/openhands-sdk/openhands/sdk/skills/skill.py b/openhands-sdk/openhands/sdk/skills/skill.py index 09dd3206ef..644f682b2d 100644 --- a/openhands-sdk/openhands/sdk/skills/skill.py +++ b/openhands-sdk/openhands/sdk/skills/skill.py @@ -830,8 +830,8 @@ def _load_and_merge_from_dirs( def load_project_skills(work_dir: str | Path) -> list[Skill]: """Load skills from project-specific directories. - Searches for skills in {work_dir}/.agents/skills/, {work_dir}/.openhands/skills/, - and {work_dir}/.openhands/microagents/ (legacy). + Searches for skills in {work_dir}/.agents/skills/, {work_dir}/skills/, + {work_dir}/.openhands/skills/, and {work_dir}/.openhands/microagents/ (legacy). If the working directory is inside a Git repository, this function also loads skills from the Git repo root, so running from a subdirectory still picks up @@ -840,8 +840,9 @@ def load_project_skills(work_dir: str | Path) -> list[Skill]: Skills are merged in priority order, with the *working directory* taking precedence over the Git repo root when duplicates exist. - Use .agents/skills for new skills. .openhands/skills is the legacy OpenHands - location, and .openhands/microagents is deprecated. + Use .agents/skills or the AgentSkills-compatible skills/ layout for new + skills. .openhands/skills is the legacy OpenHands location, and + .openhands/microagents is deprecated. Example: If "my-skill" exists in both .agents/skills/ and .openhands/skills/, the version from .agents/skills/ is used. @@ -886,11 +887,14 @@ def load_project_skills(work_dir: str | Path) -> list[Skill]: except (SkillError, OSError, yaml.YAMLError) as e: logger.warning(f"Failed to load third-party skill from {path}: {e}") - # Load project-specific skills from .agents/skills, .openhands/skills, - # and legacy microagents (priority order; first wins for duplicates) + # Load project-specific skills from .agents/skills, skills/, + # .openhands/skills, and legacy microagents (priority order; first wins + # for duplicates). The top-level skills/ layout is the AgentSkills + # convention used by repositories that keep shareable skills in-tree. for root in search_roots: project_skills_dirs = [ root / ".agents" / "skills", + root / "skills", root / ".openhands" / "skills", root / ".openhands" / "microagents", # Legacy support ] diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index 5344712af0..62e4c56e8f 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -842,7 +842,7 @@ def _fake_run_async(coro_factory, **_kwargs): ) assert "AgentSkills full instructions should not be sent." not in prompt_text - def test_step_sends_legacy_triggered_skill_content_to_acp_server(self, tmp_path): + def test_step_sends_triggered_skill_content_to_acp_server(self, tmp_path): agent = _make_agent( agent_context=AgentContext( skills=[ @@ -895,7 +895,7 @@ def _fake_run_async(coro_factory, **_kwargs): 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." not in prompt_text + assert "AgentSkills triggered review instructions." in prompt_text assert "agentskill-review" in prompt_text assert "AgentSkills review catalog." in prompt_text diff --git a/tests/sdk/skills/test_load_project_skills.py b/tests/sdk/skills/test_load_project_skills.py index e8f89f83d5..98432399a4 100644 --- a/tests/sdk/skills/test_load_project_skills.py +++ b/tests/sdk/skills/test_load_project_skills.py @@ -112,6 +112,32 @@ def test_load_project_skills_with_agents_directory(tmp_path): assert isinstance(skills[0].trigger, KeywordTrigger) +def test_load_project_skills_with_top_level_skills_directory(tmp_path): + """Test load_project_skills loads AgentSkills from top-level skills/.""" + skills_dir = tmp_path / "skills" / "custom-review" + skills_dir.mkdir(parents=True) + + (skills_dir / "SKILL.md").write_text( + "---\n" + "name: custom-review\n" + "description: Custom review guidance.\n" + "triggers:\n" + " - /codereview\n" + "---\n" + "# Custom Review\n\n" + "All review output must be Chinese." + ) + + skills = load_project_skills(tmp_path) + + assert len(skills) == 1 + assert skills[0].name == "custom-review" + assert skills[0].is_agentskills_format is True + assert skills[0].description == "Custom review guidance." + assert "All review output must be Chinese." in skills[0].content + assert isinstance(skills[0].trigger, KeywordTrigger) + + def test_load_project_skills_agents_directory_precedence(tmp_path): """Test .agents/skills takes precedence over other directories.""" agents_dir = tmp_path / ".agents" / "skills" From e4a34113edcf2bde0aefa9a8959739ce0da8db46 Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Sat, 25 Apr 2026 15:01:00 +0000 Subject: [PATCH 09/27] Reuse skill prompt rendering for ACP catalogs --- .../openhands/sdk/context/agent_context.py | 31 ++++++++----------- openhands-sdk/openhands/sdk/skills/skill.py | 12 ++++++- tests/sdk/skills/test_validation_prompt.py | 17 ++++++++++ 3 files changed, 41 insertions(+), 19 deletions(-) diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index bbf4d57beb..6a7caf1765 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -349,24 +349,19 @@ def to_acp_prompt_context( parts.append(repo_context) if include_skill_catalog and available_skills: - lines = [ - "", - "The following skills are available:", - "", - ] - for skill in available_skills: - name = xml_escape(skill.name.strip()) - description = xml_escape((skill.description or "").strip()) - lines.append(" ") - lines.append(f" {name}") - lines.append(f" {description}") - if include_full_skill_content: - content = xml_escape(skill.content.strip()) - lines.append(f" {content}") - lines.append(" ") - lines.append("") - lines.append("") - parts.append("\n".join(lines)) + parts.append( + "\n".join( + [ + "", + "The following skills are available:", + to_prompt( + available_skills, + include_content=include_full_skill_content, + ), + "", + ] + ) + ) if include_system_suffix and self.system_message_suffix: system_suffix = self.system_message_suffix.strip() diff --git a/openhands-sdk/openhands/sdk/skills/skill.py b/openhands-sdk/openhands/sdk/skills/skill.py index 644f682b2d..64a3674793 100644 --- a/openhands-sdk/openhands/sdk/skills/skill.py +++ b/openhands-sdk/openhands/sdk/skills/skill.py @@ -1150,7 +1150,12 @@ def load_available_skills( return available -def to_prompt(skills: list[Skill], max_description_length: int = 1024) -> str: +def to_prompt( + skills: list[Skill], + max_description_length: int = 1024, + *, + include_content: bool = False, +) -> str: """Generate XML prompt block for available skills. Creates an `` XML block suitable for inclusion @@ -1159,6 +1164,8 @@ def to_prompt(skills: list[Skill], max_description_length: int = 1024) -> str: Args: skills: List of skills to include in the prompt max_description_length: Maximum length for descriptions (default 1024) + include_content: Whether to include the full skill content in a + `` element. Defaults to False for progressive disclosure. Returns: XML string in AgentSkills format with name and description. The @@ -1227,6 +1234,9 @@ def to_prompt(skills: list[Skill], max_description_length: int = 1024) -> str: lines.append(" ") lines.append(f" {name}") lines.append(f" {description}") + if include_content: + content = xml_escape(skill.content.strip()) + lines.append(f" {content}") lines.append(" ") lines.append("") diff --git a/tests/sdk/skills/test_validation_prompt.py b/tests/sdk/skills/test_validation_prompt.py index c5413254a2..65af9ccb77 100644 --- a/tests/sdk/skills/test_validation_prompt.py +++ b/tests/sdk/skills/test_validation_prompt.py @@ -57,6 +57,23 @@ def test_to_prompt_escapes_xml() -> None: assert '"quotes"' in result +def test_to_prompt_can_include_full_content() -> None: + """to_prompt() can render full skill content for prompt-only adapters.""" + skill = Skill( + name="review", + content="Use review rules & explain findings.", + description="Review pull requests.", + ) + + result = to_prompt([skill], include_content=True) + + assert "Review pull requests." in result + assert ( + "Use <strict> review rules & explain findings." + in result + ) + + def test_to_prompt_uses_content_fallback() -> None: """to_prompt() should use content when no description.""" skill = Skill(name="test", content="# Header\n\nActual content here.") From bdca2e7e85a93be6868463ab42581bc4ce58a194 Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Sat, 25 Apr 2026 15:06:04 +0000 Subject: [PATCH 10/27] Share user prompt extension handling with ACP --- .../openhands/sdk/agent/acp_agent.py | 20 ++++++------ .../openhands/sdk/context/agent_context.py | 32 +++++++++++++++++++ .../conversation/impl/local_conversation.py | 16 +++++----- tests/sdk/context/test_agent_context.py | 28 ++++++++++++++++ 4 files changed, 78 insertions(+), 18 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index ffad761938..38b51536fb 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -1117,9 +1117,8 @@ def step( state = conversation.state # Find the latest user message. ACP receives AgentSkills as a prompt-only - # catalog by default, but explicit keyword triggers still contribute the - # matched skill content as prompt text because ACP has no invoke_skill - # tool path to retrieve it later. + # catalog by default, and shares AgentContext's normal per-turn trigger + # injection so explicit keyword matches still contribute skill content. user_message = None for event in reversed(list(state.events)): if isinstance(event, MessageEvent) and event.source == "user": @@ -1130,14 +1129,15 @@ def step( if isinstance(content, TextContent) and content.text.strip() ] if self.agent_context: - legacy_user_context = self.agent_context.get_user_message_suffix( - user_message=message, - skip_skill_names=[], - include_agentskills_format=True, - include_user_message_suffix=False, + prompt_extensions, _activated_skill_names = ( + self.agent_context.get_user_message_prompt_extensions( + user_message=message, + skip_skill_names=[], + include_agentskills_format=True, + include_user_message_suffix=False, + ) ) - if legacy_user_context: - content, _activated_skill_names = legacy_user_context + for content in prompt_extensions: if content.text.strip(): text_parts.append(content.text) acp_prompt_context = self.agent_context.to_acp_prompt_context( diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index 6a7caf1765..402eacfbae 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -488,3 +488,35 @@ def get_user_message_suffix( if user_message_suffix: return TextContent(text=user_message_suffix), [] return None + + def get_user_message_prompt_extensions( + self, + user_message: Message, + skip_skill_names: list[str], + *, + include_agentskills_format: bool = True, + include_legacy_skills: bool = True, + include_user_message_suffix: bool = True, + ) -> tuple[list[TextContent], list[str]]: + """Return per-turn prompt extensions for a user message. + + This is the shared entry point for conversation implementations and + prompt-only adapters that need trigger-based skill injection. It keeps + the matching behavior in one place while allowing adapters to decide + whether user_message_suffix should be included in this per-turn block + or injected through another prompt-context path. + """ + suffix = self.get_user_message_suffix( + user_message=user_message, + skip_skill_names=skip_skill_names, + include_agentskills_format=include_agentskills_format, + include_legacy_skills=include_legacy_skills, + include_user_message_suffix=include_user_message_suffix, + ) + if suffix is None: + return [], [] + + content, activated_skill_names = suffix + if not content.text.strip(): + return [], activated_skill_names + return [content], activated_skill_names diff --git a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py index 0b706dcf80..2ef0914fa6 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py @@ -691,21 +691,21 @@ def send_message(self, message: str | Message, sender: str | None = None) -> Non # Handle per-turn user message (i.e., knowledge agent trigger) if self.agent.agent_context: - ctx = self.agent.agent_context.get_user_message_suffix( - user_message=message, - # We skip skills that were already activated - skip_skill_names=self._state.activated_knowledge_skills, + extended_content, activated_skill_names = ( + self.agent.agent_context.get_user_message_prompt_extensions( + user_message=message, + # We skip skills that were already activated + skip_skill_names=self._state.activated_knowledge_skills, + ) ) # TODO(calvin): we need to update # self._state.activated_knowledge_skills # so condenser can work - if ctx: - content, activated_skill_names = ctx + if extended_content: logger.debug( - f"Got augmented user message content: {content}, " + f"Got augmented user message content: {extended_content}, " f"activated skills: {activated_skill_names}" ) - extended_content.append(content) self._state.activated_knowledge_skills.extend(activated_skill_names) user_msg_event = MessageEvent( diff --git a/tests/sdk/context/test_agent_context.py b/tests/sdk/context/test_agent_context.py index 0c59b7de75..929457dae2 100644 --- a/tests/sdk/context/test_agent_context.py +++ b/tests/sdk/context/test_agent_context.py @@ -294,6 +294,34 @@ def test_get_user_message_suffix_with_single_trigger(self): assert text_content.text == expected_output assert triggered_names == ["python_tips"] + def test_get_user_message_prompt_extensions_reuses_suffix_logic(self): + """Prompt extensions wrap triggered content for conversation adapters.""" + skill = Skill( + name="review-guide", + content="All review output must be Chinese.", + trigger=KeywordTrigger(keywords=["/codereview"]), + is_agentskills_format=True, + ) + context = AgentContext( + skills=[skill], + user_message_suffix="Prefer concise responses.", + ) + user_message = Message( + role="user", + content=[TextContent(text="/codereview this change")], + ) + + extensions, activated_names = context.get_user_message_prompt_extensions( + user_message=user_message, + skip_skill_names=[], + include_user_message_suffix=False, + ) + + assert activated_names == ["review-guide"] + assert len(extensions) == 1 + assert "All review output must be Chinese." in extensions[0].text + assert "Prefer concise responses." not in extensions[0].text + def test_get_user_message_suffix_with_multiple_triggers(self): """Test user message suffix with multiple triggered skills.""" python_agent = Skill( From 05c8fc73b69911e47edec185577726b9c05cda27 Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Sat, 25 Apr 2026 15:11:01 +0000 Subject: [PATCH 11/27] Simplify ACP prompt extension reuse --- .../openhands/sdk/agent/acp_agent.py | 21 +++--------- .../openhands/sdk/context/agent_context.py | 32 ------------------- .../conversation/impl/local_conversation.py | 16 +++++----- tests/sdk/agent/test_acp_agent.py | 11 +++---- tests/sdk/context/test_agent_context.py | 28 ---------------- 5 files changed, 18 insertions(+), 90 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index 38b51536fb..01bb66c354 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -1116,34 +1116,23 @@ def step( """Send the latest user message to the ACP server and emit the response.""" state = conversation.state - # Find the latest user message. ACP receives AgentSkills as a prompt-only - # catalog by default, and shares AgentContext's normal per-turn trigger - # injection so explicit keyword matches still contribute skill content. + # 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": - message = event.llm_message + message = event.to_llm_message() text_parts = [ content.text for content in message.content if isinstance(content, TextContent) and content.text.strip() ] if self.agent_context: - prompt_extensions, _activated_skill_names = ( - self.agent_context.get_user_message_prompt_extensions( - user_message=message, - skip_skill_names=[], - include_agentskills_format=True, - include_user_message_suffix=False, - ) - ) - for content in prompt_extensions: - if content.text.strip(): - text_parts.append(content.text) acp_prompt_context = self.agent_context.to_acp_prompt_context( include_skill_catalog=True, include_system_suffix=True, - include_user_suffix=True, + include_user_suffix=False, include_secret_catalog=True, include_current_datetime=True, include_full_skill_content=False, diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index 402eacfbae..6a7caf1765 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -488,35 +488,3 @@ def get_user_message_suffix( if user_message_suffix: return TextContent(text=user_message_suffix), [] return None - - def get_user_message_prompt_extensions( - self, - user_message: Message, - skip_skill_names: list[str], - *, - include_agentskills_format: bool = True, - include_legacy_skills: bool = True, - include_user_message_suffix: bool = True, - ) -> tuple[list[TextContent], list[str]]: - """Return per-turn prompt extensions for a user message. - - This is the shared entry point for conversation implementations and - prompt-only adapters that need trigger-based skill injection. It keeps - the matching behavior in one place while allowing adapters to decide - whether user_message_suffix should be included in this per-turn block - or injected through another prompt-context path. - """ - suffix = self.get_user_message_suffix( - user_message=user_message, - skip_skill_names=skip_skill_names, - include_agentskills_format=include_agentskills_format, - include_legacy_skills=include_legacy_skills, - include_user_message_suffix=include_user_message_suffix, - ) - if suffix is None: - return [], [] - - content, activated_skill_names = suffix - if not content.text.strip(): - return [], activated_skill_names - return [content], activated_skill_names diff --git a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py index 2ef0914fa6..0b706dcf80 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py @@ -691,21 +691,21 @@ def send_message(self, message: str | Message, sender: str | None = None) -> Non # Handle per-turn user message (i.e., knowledge agent trigger) if self.agent.agent_context: - extended_content, activated_skill_names = ( - self.agent.agent_context.get_user_message_prompt_extensions( - user_message=message, - # We skip skills that were already activated - skip_skill_names=self._state.activated_knowledge_skills, - ) + ctx = self.agent.agent_context.get_user_message_suffix( + user_message=message, + # We skip skills that were already activated + skip_skill_names=self._state.activated_knowledge_skills, ) # TODO(calvin): we need to update # self._state.activated_knowledge_skills # so condenser can work - if extended_content: + if ctx: + content, activated_skill_names = ctx logger.debug( - f"Got augmented user message content: {extended_content}, " + f"Got augmented user message content: {content}, " f"activated skills: {activated_skill_names}" ) + extended_content.append(content) self._state.activated_knowledge_skills.extend(activated_skill_names) user_msg_event = MessageEvent( diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index 62e4c56e8f..d9a5454536 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -771,7 +771,7 @@ def _fake_run_async(coro_factory, **_kwargs): assert "Review this PR." in prompt_text assert "review" in prompt_text assert "Review pull requests." in prompt_text - assert "Use strict review." not in prompt_text + assert "Use strict review." in prompt_text assert ( "Full review instructions that ACP should not receive." not in prompt_text ) @@ -803,11 +803,6 @@ def test_step_sends_legacy_repo_context_to_acp_server(self, tmp_path): role="user", content=[TextContent(text="Review this PR.")], ), - extended_content=[ - TextContent( - text="AgentSkills full instructions should not be sent." - ) - ], ) ) conversation = MagicMock() @@ -870,6 +865,10 @@ def test_step_sends_triggered_skill_content_to_acp_server(self, tmp_path): role="user", content=[TextContent(text="/review this PR.")], ), + extended_content=[ + TextContent(text="Legacy triggered review instructions."), + TextContent(text="AgentSkills triggered review instructions."), + ], ) ) conversation = MagicMock() diff --git a/tests/sdk/context/test_agent_context.py b/tests/sdk/context/test_agent_context.py index 929457dae2..0c59b7de75 100644 --- a/tests/sdk/context/test_agent_context.py +++ b/tests/sdk/context/test_agent_context.py @@ -294,34 +294,6 @@ def test_get_user_message_suffix_with_single_trigger(self): assert text_content.text == expected_output assert triggered_names == ["python_tips"] - def test_get_user_message_prompt_extensions_reuses_suffix_logic(self): - """Prompt extensions wrap triggered content for conversation adapters.""" - skill = Skill( - name="review-guide", - content="All review output must be Chinese.", - trigger=KeywordTrigger(keywords=["/codereview"]), - is_agentskills_format=True, - ) - context = AgentContext( - skills=[skill], - user_message_suffix="Prefer concise responses.", - ) - user_message = Message( - role="user", - content=[TextContent(text="/codereview this change")], - ) - - extensions, activated_names = context.get_user_message_prompt_extensions( - user_message=user_message, - skip_skill_names=[], - include_user_message_suffix=False, - ) - - assert activated_names == ["review-guide"] - assert len(extensions) == 1 - assert "All review output must be Chinese." in extensions[0].text - assert "Prefer concise responses." not in extensions[0].text - def test_get_user_message_suffix_with_multiple_triggers(self): """Test user message suffix with multiple triggered skills.""" python_agent = Skill( From 6b125ff8fcfa85363907ed5763b4e6b93e0cb9e1 Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Sat, 25 Apr 2026 15:15:43 +0000 Subject: [PATCH 12/27] Reject secrets in ACP prompt context --- .../openhands/sdk/agent/acp_agent.py | 1 - .../openhands/sdk/context/agent_context.py | 29 +++---------------- tests/sdk/agent/test_acp_agent.py | 18 ++++-------- 3 files changed, 10 insertions(+), 38 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index 01bb66c354..2d0e1a400f 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -1133,7 +1133,6 @@ def step( include_skill_catalog=True, include_system_suffix=True, include_user_suffix=False, - include_secret_catalog=True, include_current_datetime=True, include_full_skill_content=False, ) diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index 6a7caf1765..cad0ced7d5 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -3,7 +3,6 @@ import pathlib from collections.abc import Mapping from datetime import datetime -from xml.sax.saxutils import escape as xml_escape from pydantic import BaseModel, Field, field_validator, model_validator @@ -284,7 +283,6 @@ def to_acp_prompt_context( include_skill_catalog: bool = True, include_system_suffix: bool = True, include_user_suffix: bool = True, - include_secret_catalog: bool = True, include_current_datetime: bool = True, include_full_skill_content: bool = False, ) -> str | None: @@ -311,6 +309,10 @@ def to_acp_prompt_context( raise NotImplementedError( f"ACP prompt context does not support AgentContext field(s): {fields}" ) + if self.secrets: + raise NotImplementedError( + "ACP prompt context does not support AgentContext field(s): secrets" + ) parts: list[str] = [] @@ -389,29 +391,6 @@ def to_acp_prompt_context( ) ) - if include_secret_catalog: - secret_infos = self.get_secret_infos() - if secret_infos: - lines = [ - "", - ( - "The following secret names are registered. " - "Secret values are not included." - ), - ] - for secret_info in secret_infos: - name = xml_escape(secret_info["name"] or "") - description = xml_escape((secret_info["description"] or "").strip()) - if description: - lines.append( - f" {name}" - f"{description}" - ) - else: - lines.append(f" {name}") - lines.append("") - parts.append("\n".join(lines)) - if not parts: return None return "\n\n".join(parts) diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index d9a5454536..7a4df1b716 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -10,7 +10,6 @@ import pytest from acp.exceptions import RequestError as ACPRequestError -from pydantic import SecretStr from openhands.sdk.agent.acp_agent import ( ACPAgent, @@ -31,7 +30,6 @@ ) from openhands.sdk.event import ACPToolCallEvent, MessageEvent, SystemPromptEvent from openhands.sdk.llm import Message, TextContent -from openhands.sdk.secret import StaticSecret from openhands.sdk.skills import KeywordTrigger, Skill from openhands.sdk.workspace.local import LocalWorkspace @@ -225,12 +223,6 @@ def test_agent_context_to_acp_prompt_context(self): ], system_message_suffix="Follow repository rules.", user_message_suffix="Prefer concise responses.", - secrets={ - "GITHUB_TOKEN": StaticSecret( - value=SecretStr("ghp_secret"), - description="GitHub API token", - ) - }, current_datetime="2026-04-24T00:00:00", ) @@ -246,10 +238,12 @@ def test_agent_context_to_acp_prompt_context(self): assert "Follow repository rules." in prompt assert "" in prompt assert "Prefer concise responses." in prompt - assert "" in prompt - assert "GITHUB_TOKEN" in prompt - assert "GitHub API token" in prompt - assert "ghp_secret" not 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( From 12f4b384b7f2fb41599e3eda16a41c5cc1db6573 Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Sat, 25 Apr 2026 15:23:32 +0000 Subject: [PATCH 13/27] Tighten ACP prompt context support --- .../openhands/sdk/agent/acp_agent.py | 41 +++++++++++-------- .../openhands/sdk/context/agent_context.py | 9 +--- 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index 2d0e1a400f..0f2e1ab861 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -1106,6 +1106,28 @@ 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() + 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( + include_skill_catalog=True, + include_system_suffix=True, + include_user_suffix=False, + include_current_datetime=True, + include_full_skill_content=False, + ) + 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, @@ -1122,24 +1144,7 @@ def step( user_message = None for event in reversed(list(state.events)): if isinstance(event, MessageEvent) and event.source == "user": - message = event.to_llm_message() - 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( - include_skill_catalog=True, - include_system_suffix=True, - include_user_suffix=False, - include_current_datetime=True, - include_full_skill_content=False, - ) - if acp_prompt_context: - text_parts.append(acp_prompt_context) - if text_parts: - user_message = "\n\n".join(text_parts) + 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 cad0ced7d5..2488ff1a09 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -293,26 +293,21 @@ def to_acp_prompt_context( semantics must fail here instead of being silently approximated by ACPAgent. """ - supported_fields = { + compatible_fields = { "skills", "system_message_suffix", "user_message_suffix", "load_user_skills", "load_public_skills", "marketplace_path", - "secrets", "current_datetime", } - unsupported_fields = set(type(self).model_fields) - supported_fields + unsupported_fields = set(self.model_fields_set) - compatible_fields if unsupported_fields: fields = ", ".join(sorted(unsupported_fields)) raise NotImplementedError( f"ACP prompt context does not support AgentContext field(s): {fields}" ) - if self.secrets: - raise NotImplementedError( - "ACP prompt context does not support AgentContext field(s): secrets" - ) parts: list[str] = [] From f766cb05c49c8771ab175e09ca7fcf7888c4f0f4 Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Sat, 25 Apr 2026 15:51:20 +0000 Subject: [PATCH 14/27] Address ACP prompt review coverage --- .../openhands/sdk/agent/acp_agent.py | 4 ++ .../openhands/sdk/context/agent_context.py | 10 +++- openhands-sdk/openhands/sdk/skills/skill.py | 10 ++-- tests/sdk/agent/test_acp_agent.py | 31 +++++++++++ tests/sdk/skills/test_load_project_skills.py | 51 +++++++++++++++++-- tests/sdk/skills/test_validation_prompt.py | 13 +++++ 6 files changed, 110 insertions(+), 9 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index 0f2e1ab861..b57063a17d 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -1109,6 +1109,8 @@ def _cancel_inflight_tool_calls(self) -> None: 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 @@ -1118,6 +1120,8 @@ def _build_acp_prompt(self, event: MessageEvent) -> str | None: acp_prompt_context = self.agent_context.to_acp_prompt_context( include_skill_catalog=True, include_system_suffix=True, + # LocalConversation already applies user_message_suffix through + # event.to_llm_message(); adding it here would duplicate it. include_user_suffix=False, include_current_datetime=True, include_full_skill_content=False, diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index 2488ff1a09..6b64de2a3b 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -319,7 +319,13 @@ def to_acp_prompt_context( else: repo_skills.append(skill) - if include_current_datetime: + has_explicit_prompt_context = ( + bool(self.skills) + or bool(self.system_message_suffix) + or bool(self.user_message_suffix) + or "current_datetime" in self.model_fields_set + ) + if include_current_datetime and has_explicit_prompt_context: formatted_datetime = self.get_formatted_datetime() if formatted_datetime: parts.append( @@ -333,6 +339,8 @@ def to_acp_prompt_context( ) if repo_skills: + # Reuse the shared template only for REPO_CONTEXT rendering; ACP + # prompt-only sections are assembled explicitly below. repo_context = render_template( prompt_dir=str(PROMPT_DIR), template_name="system_message_suffix.j2", diff --git a/openhands-sdk/openhands/sdk/skills/skill.py b/openhands-sdk/openhands/sdk/skills/skill.py index 64a3674793..d0c796164f 100644 --- a/openhands-sdk/openhands/sdk/skills/skill.py +++ b/openhands-sdk/openhands/sdk/skills/skill.py @@ -1209,12 +1209,14 @@ def to_prompt( break description = description or "" - # Calculate total truncated characters - total_truncated = content_truncated + # Calculate total truncated characters. If full content is included, + # the catalog is self-contained and does not need invoke_skill guidance. + total_truncated = 0 if include_content else content_truncated - # Truncate description if needed and add truncation indicator + # Truncate description if needed and add truncation indicator. if len(description) > max_description_length: - total_truncated += len(description) - max_description_length + if not include_content: + total_truncated += len(description) - max_description_length description = description[:max_description_length] if total_truncated > 0: diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index 7a4df1b716..636459ccd9 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -239,6 +239,11 @@ def test_agent_context_to_acp_prompt_context(self): assert "" in prompt assert "Prefer concise responses." in prompt + def test_agent_context_to_acp_prompt_context_returns_none_for_empty_skills(self): + context = AgentContext(skills=[]) + + assert context.to_acp_prompt_context() is None + def test_agent_context_to_acp_prompt_context_rejects_secrets(self): context = AgentContext(secrets={"GITHUB_TOKEN": "ghp_secret"}) @@ -346,6 +351,32 @@ def test_agent_context_acp_legacy_trigger_suffix_excludes_agentskills(self): assert "Legacy triggered instructions." in content.text assert "AgentSkills triggered instructions." not in content.text + 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 diff --git a/tests/sdk/skills/test_load_project_skills.py b/tests/sdk/skills/test_load_project_skills.py index 98432399a4..588c7d48a6 100644 --- a/tests/sdk/skills/test_load_project_skills.py +++ b/tests/sdk/skills/test_load_project_skills.py @@ -138,19 +138,41 @@ def test_load_project_skills_with_top_level_skills_directory(tmp_path): assert isinstance(skills[0].trigger, KeywordTrigger) +def test_load_project_skills_with_top_level_regular_md_file(tmp_path): + """Test load_project_skills loads regular md files from top-level skills/.""" + skills_dir = tmp_path / "skills" + skills_dir.mkdir(parents=True) + + (skills_dir / "review.md").write_text( + "---\nname: review\ntriggers:\n - review\n---\nRegular markdown skill content." + ) + + skills = load_project_skills(tmp_path) + + assert len(skills) == 1 + assert skills[0].name == "review" + assert skills[0].content == "Regular markdown skill content." + assert isinstance(skills[0].trigger, KeywordTrigger) + + def test_load_project_skills_agents_directory_precedence(tmp_path): - """Test .agents/skills takes precedence over other directories.""" + """Test project skill directories follow the documented precedence order.""" agents_dir = tmp_path / ".agents" / "skills" - skills_dir = tmp_path / ".openhands" / "skills" + top_level_dir = tmp_path / "skills" + openhands_dir = tmp_path / ".openhands" / "skills" microagents_dir = tmp_path / ".openhands" / "microagents" agents_dir.mkdir(parents=True) - skills_dir.mkdir(parents=True) + top_level_dir.mkdir(parents=True) + openhands_dir.mkdir(parents=True) microagents_dir.mkdir(parents=True) (agents_dir / "duplicate.md").write_text( "---\nname: duplicate\n---\nFrom .agents/skills." ) - (skills_dir / "duplicate.md").write_text( + (top_level_dir / "duplicate.md").write_text( + "---\nname: duplicate\n---\nFrom top-level skills." + ) + (openhands_dir / "duplicate.md").write_text( "---\nname: duplicate\n---\nFrom .openhands/skills." ) (microagents_dir / "duplicate.md").write_text( @@ -163,6 +185,27 @@ def test_load_project_skills_agents_directory_precedence(tmp_path): assert skills[0].content == "From .agents/skills." +def test_load_project_skills_top_level_directory_precedes_openhands(tmp_path): + """Test top-level skills/ takes precedence over legacy OpenHands dirs.""" + top_level_dir = tmp_path / "skills" + openhands_dir = tmp_path / ".openhands" / "skills" + top_level_dir.mkdir(parents=True) + openhands_dir.mkdir(parents=True) + + (top_level_dir / "duplicate.md").write_text( + "---\nname: duplicate\n---\nFrom top-level skills." + ) + (openhands_dir / "duplicate.md").write_text( + "---\nname: duplicate\n---\nFrom .openhands/skills." + ) + + skills = load_project_skills(tmp_path) + + assert len(skills) == 1 + assert skills[0].name == "duplicate" + assert skills[0].content == "From top-level skills." + + def test_load_project_skills_merges_agents_and_openhands(tmp_path): """Test loading unique skills from .agents/skills and .openhands/skills.""" agents_dir = tmp_path / ".agents" / "skills" diff --git a/tests/sdk/skills/test_validation_prompt.py b/tests/sdk/skills/test_validation_prompt.py index 65af9ccb77..2f7203ed27 100644 --- a/tests/sdk/skills/test_validation_prompt.py +++ b/tests/sdk/skills/test_validation_prompt.py @@ -72,6 +72,7 @@ def test_to_prompt_can_include_full_content() -> None: "Use <strict> review rules & explain findings." in result ) + assert "invoke_skill" not in result def test_to_prompt_uses_content_fallback() -> None: @@ -82,6 +83,18 @@ def test_to_prompt_uses_content_fallback() -> None: assert "# Header" not in result +def test_to_prompt_include_content_with_content_fallback() -> None: + """include_content=True should include fallback description and full content.""" + skill = Skill(name="test", content="# Header\n\nBody.\n\nMore details.") + + result = to_prompt([skill], include_content=True) + + assert "Body." in result + assert "# Header\n\nBody.\n\nMore details." in result + assert "characters truncated" not in result + assert "invoke_skill" not in result + + def test_to_prompt_content_fallback_counts_remaining_as_truncated() -> None: """to_prompt() should count content after first line as truncated.""" # Content with header, description line, and additional content From 5105e317bf4d95b9353b2a3c9a1610789379e88f Mon Sep 17 00:00:00 2001 From: openhands Date: Sun, 26 Apr 2026 15:23:39 +0000 Subject: [PATCH 15/27] Remove unused include_user_suffix param from to_acp_prompt_context LocalConversation already applies user_message_suffix through event.to_llm_message(), so to_acp_prompt_context never needs to emit it. Drop the parameter and the associated code block instead of keeping an always-False argument. Co-authored-by: openhands --- openhands-sdk/openhands/sdk/agent/acp_agent.py | 3 --- .../openhands/sdk/context/agent_context.py | 18 ++++-------------- tests/sdk/agent/test_acp_agent.py | 6 ++++-- 3 files changed, 8 insertions(+), 19 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index b57063a17d..f3ad3d8ce5 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -1120,9 +1120,6 @@ def _build_acp_prompt(self, event: MessageEvent) -> str | None: acp_prompt_context = self.agent_context.to_acp_prompt_context( include_skill_catalog=True, include_system_suffix=True, - # LocalConversation already applies user_message_suffix through - # event.to_llm_message(); adding it here would duplicate it. - include_user_suffix=False, include_current_datetime=True, include_full_skill_content=False, ) diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index 6b64de2a3b..d32886c02f 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -282,7 +282,6 @@ def to_acp_prompt_context( *, include_skill_catalog: bool = True, include_system_suffix: bool = True, - include_user_suffix: bool = True, include_current_datetime: bool = True, include_full_skill_content: bool = False, ) -> str | None: @@ -292,6 +291,10 @@ def to_acp_prompt_context( this adapter only emits prompt-only context. Unsupported AgentContext semantics must fail here instead of being silently approximated by ACPAgent. + + ``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. """ compatible_fields = { "skills", @@ -381,19 +384,6 @@ def to_acp_prompt_context( ) ) - if include_user_suffix and self.user_message_suffix: - user_suffix = self.user_message_suffix.strip() - if user_suffix: - parts.append( - "\n".join( - [ - "", - user_suffix, - "", - ] - ) - ) - if not parts: return None return "\n\n".join(parts) diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index 636459ccd9..8c51406073 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -236,8 +236,10 @@ def test_agent_context_to_acp_prompt_context(self): assert "Full review instructions" not in prompt assert "" in prompt assert "Follow repository rules." in prompt - assert "" in prompt - assert "Prefer concise responses." in prompt + # user_message_suffix is not emitted by to_acp_prompt_context because + # LocalConversation already applies it via event.to_llm_message(). + assert "" not in prompt + assert "Prefer concise responses." not in prompt def test_agent_context_to_acp_prompt_context_returns_none_for_empty_skills(self): context = AgentContext(skills=[]) From 3679a47b63ab8f040fb9a8c44300abfff104d07c Mon Sep 17 00:00:00 2001 From: openhands Date: Sun, 26 Apr 2026 17:09:05 +0000 Subject: [PATCH 16/27] Revert top-level skills/ search path addition Remove root/skills/ from load_project_skills search paths. As noted in review, adding a bare skills/ directory could unintentionally load files from repositories that use skills/ for other purposes (documentation, team skills taxonomy, etc.). This doesn't add enough value to justify the risk. Co-authored-by: openhands --- openhands-sdk/openhands/sdk/skills/skill.py | 16 ++-- tests/sdk/skills/test_load_project_skills.py | 77 +------------------- 2 files changed, 10 insertions(+), 83 deletions(-) diff --git a/openhands-sdk/openhands/sdk/skills/skill.py b/openhands-sdk/openhands/sdk/skills/skill.py index d0c796164f..9c5d3b5ef6 100644 --- a/openhands-sdk/openhands/sdk/skills/skill.py +++ b/openhands-sdk/openhands/sdk/skills/skill.py @@ -830,8 +830,8 @@ def _load_and_merge_from_dirs( def load_project_skills(work_dir: str | Path) -> list[Skill]: """Load skills from project-specific directories. - Searches for skills in {work_dir}/.agents/skills/, {work_dir}/skills/, - {work_dir}/.openhands/skills/, and {work_dir}/.openhands/microagents/ (legacy). + Searches for skills in {work_dir}/.agents/skills/, {work_dir}/.openhands/skills/, + and {work_dir}/.openhands/microagents/ (legacy). If the working directory is inside a Git repository, this function also loads skills from the Git repo root, so running from a subdirectory still picks up @@ -840,9 +840,8 @@ def load_project_skills(work_dir: str | Path) -> list[Skill]: Skills are merged in priority order, with the *working directory* taking precedence over the Git repo root when duplicates exist. - Use .agents/skills or the AgentSkills-compatible skills/ layout for new - skills. .openhands/skills is the legacy OpenHands location, and - .openhands/microagents is deprecated. + Use .agents/skills for new skills. .openhands/skills is the legacy OpenHands + location, and .openhands/microagents is deprecated. Example: If "my-skill" exists in both .agents/skills/ and .openhands/skills/, the version from .agents/skills/ is used. @@ -887,14 +886,11 @@ def load_project_skills(work_dir: str | Path) -> list[Skill]: except (SkillError, OSError, yaml.YAMLError) as e: logger.warning(f"Failed to load third-party skill from {path}: {e}") - # Load project-specific skills from .agents/skills, skills/, - # .openhands/skills, and legacy microagents (priority order; first wins - # for duplicates). The top-level skills/ layout is the AgentSkills - # convention used by repositories that keep shareable skills in-tree. + # Load project-specific skills from .agents/skills, .openhands/skills, + # and legacy microagents (priority order; first wins for duplicates) for root in search_roots: project_skills_dirs = [ root / ".agents" / "skills", - root / "skills", root / ".openhands" / "skills", root / ".openhands" / "microagents", # Legacy support ] diff --git a/tests/sdk/skills/test_load_project_skills.py b/tests/sdk/skills/test_load_project_skills.py index 588c7d48a6..e8f89f83d5 100644 --- a/tests/sdk/skills/test_load_project_skills.py +++ b/tests/sdk/skills/test_load_project_skills.py @@ -112,67 +112,19 @@ def test_load_project_skills_with_agents_directory(tmp_path): assert isinstance(skills[0].trigger, KeywordTrigger) -def test_load_project_skills_with_top_level_skills_directory(tmp_path): - """Test load_project_skills loads AgentSkills from top-level skills/.""" - skills_dir = tmp_path / "skills" / "custom-review" - skills_dir.mkdir(parents=True) - - (skills_dir / "SKILL.md").write_text( - "---\n" - "name: custom-review\n" - "description: Custom review guidance.\n" - "triggers:\n" - " - /codereview\n" - "---\n" - "# Custom Review\n\n" - "All review output must be Chinese." - ) - - skills = load_project_skills(tmp_path) - - assert len(skills) == 1 - assert skills[0].name == "custom-review" - assert skills[0].is_agentskills_format is True - assert skills[0].description == "Custom review guidance." - assert "All review output must be Chinese." in skills[0].content - assert isinstance(skills[0].trigger, KeywordTrigger) - - -def test_load_project_skills_with_top_level_regular_md_file(tmp_path): - """Test load_project_skills loads regular md files from top-level skills/.""" - skills_dir = tmp_path / "skills" - skills_dir.mkdir(parents=True) - - (skills_dir / "review.md").write_text( - "---\nname: review\ntriggers:\n - review\n---\nRegular markdown skill content." - ) - - skills = load_project_skills(tmp_path) - - assert len(skills) == 1 - assert skills[0].name == "review" - assert skills[0].content == "Regular markdown skill content." - assert isinstance(skills[0].trigger, KeywordTrigger) - - def test_load_project_skills_agents_directory_precedence(tmp_path): - """Test project skill directories follow the documented precedence order.""" + """Test .agents/skills takes precedence over other directories.""" agents_dir = tmp_path / ".agents" / "skills" - top_level_dir = tmp_path / "skills" - openhands_dir = tmp_path / ".openhands" / "skills" + skills_dir = tmp_path / ".openhands" / "skills" microagents_dir = tmp_path / ".openhands" / "microagents" agents_dir.mkdir(parents=True) - top_level_dir.mkdir(parents=True) - openhands_dir.mkdir(parents=True) + skills_dir.mkdir(parents=True) microagents_dir.mkdir(parents=True) (agents_dir / "duplicate.md").write_text( "---\nname: duplicate\n---\nFrom .agents/skills." ) - (top_level_dir / "duplicate.md").write_text( - "---\nname: duplicate\n---\nFrom top-level skills." - ) - (openhands_dir / "duplicate.md").write_text( + (skills_dir / "duplicate.md").write_text( "---\nname: duplicate\n---\nFrom .openhands/skills." ) (microagents_dir / "duplicate.md").write_text( @@ -185,27 +137,6 @@ def test_load_project_skills_agents_directory_precedence(tmp_path): assert skills[0].content == "From .agents/skills." -def test_load_project_skills_top_level_directory_precedes_openhands(tmp_path): - """Test top-level skills/ takes precedence over legacy OpenHands dirs.""" - top_level_dir = tmp_path / "skills" - openhands_dir = tmp_path / ".openhands" / "skills" - top_level_dir.mkdir(parents=True) - openhands_dir.mkdir(parents=True) - - (top_level_dir / "duplicate.md").write_text( - "---\nname: duplicate\n---\nFrom top-level skills." - ) - (openhands_dir / "duplicate.md").write_text( - "---\nname: duplicate\n---\nFrom .openhands/skills." - ) - - skills = load_project_skills(tmp_path) - - assert len(skills) == 1 - assert skills[0].name == "duplicate" - assert skills[0].content == "From top-level skills." - - def test_load_project_skills_merges_agents_and_openhands(tmp_path): """Test loading unique skills from .agents/skills and .openhands/skills.""" agents_dir = tmp_path / ".agents" / "skills" From cb216f86dad6bfaec1d3ee020886a9e108ffe671 Mon Sep 17 00:00:00 2001 From: openhands Date: Sun, 26 Apr 2026 18:58:13 +0000 Subject: [PATCH 17/27] Remove speculative parameters from ACP prompt context and get_user_message_suffix Remove keyword arguments that are not used by any production code path: - to_acp_prompt_context(): remove include_skill_catalog, include_system_suffix, include_current_datetime, include_full_skill_content (all were always called with their default values) - get_user_message_suffix(): remove include_agentskills_format, include_legacy_skills, include_user_message_suffix (only used in tests, no production callers) - Simplify _build_acp_prompt() call to use parameterless to_acp_prompt_context() - Remove tests that only exercised the removed parameters Co-authored-by: openhands --- .../openhands/sdk/agent/acp_agent.py | 7 +-- .../openhands/sdk/context/agent_context.py | 38 +++----------- tests/sdk/agent/test_acp_agent.py | 51 ------------------- 3 files changed, 8 insertions(+), 88 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index f3ad3d8ce5..46eb25c030 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -1117,12 +1117,7 @@ def _build_acp_prompt(self, event: MessageEvent) -> str | None: if isinstance(content, TextContent) and content.text.strip() ] if self.agent_context: - acp_prompt_context = self.agent_context.to_acp_prompt_context( - include_skill_catalog=True, - include_system_suffix=True, - include_current_datetime=True, - include_full_skill_content=False, - ) + acp_prompt_context = self.agent_context.to_acp_prompt_context() if acp_prompt_context: text_parts.append(acp_prompt_context) if not text_parts: diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index d32886c02f..f3159f8664 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -277,14 +277,7 @@ def get_system_message_suffix( return self.system_message_suffix.strip() return None - def to_acp_prompt_context( - self, - *, - include_skill_catalog: bool = True, - include_system_suffix: bool = True, - include_current_datetime: bool = True, - include_full_skill_content: bool = False, - ) -> str | None: + 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 @@ -328,7 +321,7 @@ def to_acp_prompt_context( or bool(self.user_message_suffix) or "current_datetime" in self.model_fields_set ) - if include_current_datetime and has_explicit_prompt_context: + if has_explicit_prompt_context: formatted_datetime = self.get_formatted_datetime() if formatted_datetime: parts.append( @@ -356,22 +349,19 @@ def to_acp_prompt_context( if repo_context: parts.append(repo_context) - if include_skill_catalog and available_skills: + if available_skills: parts.append( "\n".join( [ "", "The following skills are available:", - to_prompt( - available_skills, - include_content=include_full_skill_content, - ), + to_prompt(available_skills), "", ] ) ) - if include_system_suffix and self.system_message_suffix: + if self.system_message_suffix: system_suffix = self.system_message_suffix.strip() if system_suffix: parts.append( @@ -389,13 +379,7 @@ def to_acp_prompt_context( return "\n\n".join(parts) def get_user_message_suffix( - self, - user_message: Message, - skip_skill_names: list[str], - *, - include_agentskills_format: bool = True, - include_legacy_skills: bool = True, - include_user_message_suffix: bool = True, + self, user_message: Message, skip_skill_names: list[str] ) -> tuple[TextContent, list[str]] | None: """Augment the user’s message with knowledge recalled from skills. @@ -406,11 +390,7 @@ def get_user_message_suffix( """ # noqa: E501 user_message_suffix = None - if ( - include_user_message_suffix - and self.user_message_suffix - and self.user_message_suffix.strip() - ): + if self.user_message_suffix and self.user_message_suffix.strip(): user_message_suffix = self.user_message_suffix.strip() query = "\n".join( @@ -426,10 +406,6 @@ def get_user_message_suffix( for skill in self.skills: if not isinstance(skill, Skill): continue - if skill.is_agentskills_format and not include_agentskills_format: - continue - if not skill.is_agentskills_format and not include_legacy_skills: - continue trigger = skill.match_trigger(query) if trigger and skill.name not in skip_skill_names: logger.info( diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index 8c51406073..f92628d485 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -302,57 +302,6 @@ def test_agent_context_to_acp_prompt_context_lists_legacy_triggered_skills(self) assert "Run a stricter review." in prompt assert "Use a stricter review tone." not in prompt - def test_agent_context_to_acp_prompt_context_can_include_full_skill_content(self): - context = AgentContext( - skills=[ - Skill( - name="review", - content="Full review instructions", - description="Review pull requests.", - is_agentskills_format=True, - ) - ] - ) - - prompt = context.to_acp_prompt_context(include_full_skill_content=True) - - assert prompt is not None - assert "Full review instructions" in prompt - - def test_agent_context_acp_legacy_trigger_suffix_excludes_agentskills(self): - context = AgentContext( - skills=[ - Skill( - name="legacy-review", - content="Legacy triggered instructions.", - trigger=KeywordTrigger(keywords=["/review"]), - ), - Skill( - name="agentskill-review", - content="AgentSkills triggered instructions.", - trigger=KeywordTrigger(keywords=["/review"]), - is_agentskills_format=True, - ), - ] - ) - message = Message( - role="user", - content=[TextContent(text="/review this change")], - ) - - suffix = context.get_user_message_suffix( - message, - skip_skill_names=[], - include_agentskills_format=False, - include_user_message_suffix=False, - ) - - assert suffix is not None - content, activated_skills = suffix - assert activated_skills == ["legacy-review"] - assert "Legacy triggered instructions." in content.text - assert "AgentSkills triggered instructions." not in content.text - def test_build_acp_prompt_preserves_all_text_blocks(self): agent = _make_agent( agent_context=AgentContext( From 1a31c5ac949a596c7bff9f477e333a9d08c215c1 Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 27 Apr 2026 15:06:44 +0000 Subject: [PATCH 18/27] refactor: extract _partition_skills() and deduplicate test mock setup - Extract shared skill partitioning logic into AgentContext._partition_skills() so get_system_message_suffix() and to_acp_prompt_context() share one source of truth for the repo-skills vs available-skills categorization rules. - Extract repeated ACP mock wiring (client, conn, executor) into TestACPAgentStep._wire_passthrough_mocks() to consolidate 3 identical 13-line blocks. Co-authored-by: openhands --- .../openhands/sdk/context/agent_context.py | 47 +++++++------- tests/sdk/agent/test_acp_agent.py | 62 ++++++------------- 2 files changed, 43 insertions(+), 66 deletions(-) diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index f3159f8664..c63ed8bbe8 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -169,6 +169,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 +219,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: @@ -307,13 +312,7 @@ def to_acp_prompt_context(self) -> str | None: parts: list[str] = [] - repo_skills: list[Skill] = [] - available_skills: list[Skill] = [] - for skill in self.skills: - if skill.is_agentskills_format or skill.trigger is not None: - available_skills.append(skill) - else: - repo_skills.append(skill) + repo_skills, available_skills = self._partition_skills() has_explicit_prompt_context = ( bool(self.skills) diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index f92628d485..391c5a0124 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -692,6 +692,23 @@ 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( @@ -722,20 +739,7 @@ def test_step_sends_skill_catalog_to_acp_server(self, tmp_path): ) conversation = MagicMock() conversation.state = state - - 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 + self._wire_passthrough_mocks(agent) agent.step(conversation, on_event=lambda _: None) @@ -783,20 +787,7 @@ def test_step_sends_legacy_repo_context_to_acp_server(self, tmp_path): ) conversation = MagicMock() conversation.state = state - - 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 + self._wire_passthrough_mocks(agent) agent.step(conversation, on_event=lambda _: None) @@ -849,20 +840,7 @@ def test_step_sends_triggered_skill_content_to_acp_server(self, tmp_path): ) conversation = MagicMock() conversation.state = state - - 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 + self._wire_passthrough_mocks(agent) agent.step(conversation, on_event=lambda _: None) From f4819d377bad1cfd639cca2efbc6a69d4603d253 Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Mon, 27 Apr 2026 18:22:52 +0100 Subject: [PATCH 19/27] Update openhands-sdk/openhands/sdk/context/agent_context.py Co-authored-by: Vasco Schiavo <115561717+VascoSch92@users.noreply.github.com> --- openhands-sdk/openhands/sdk/context/agent_context.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index c63ed8bbe8..b476da7c84 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -310,8 +310,6 @@ def to_acp_prompt_context(self) -> str | None: f"ACP prompt context does not support AgentContext field(s): {fields}" ) - parts: list[str] = [] - repo_skills, available_skills = self._partition_skills() has_explicit_prompt_context = ( @@ -320,6 +318,8 @@ def to_acp_prompt_context(self) -> str | None: or bool(self.user_message_suffix) or "current_datetime" in self.model_fields_set ) + + parts: list[str] = [] if has_explicit_prompt_context: formatted_datetime = self.get_formatted_datetime() if formatted_datetime: From 3dd3b87a847abc5f8210600a3be34da51bec81c5 Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 27 Apr 2026 17:23:24 +0000 Subject: [PATCH 20/27] refactor: remove unused include_content parameter from to_prompt() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The include_content parameter on to_prompt() was not called from any production code path — to_acp_prompt_context() uses the default include_content=False. Remove the speculative feature and its tests to keep the change set minimal. Co-authored-by: openhands --- openhands-sdk/openhands/sdk/skills/skill.py | 22 ++++----------- tests/sdk/skills/test_validation_prompt.py | 30 --------------------- 2 files changed, 5 insertions(+), 47 deletions(-) diff --git a/openhands-sdk/openhands/sdk/skills/skill.py b/openhands-sdk/openhands/sdk/skills/skill.py index 9c5d3b5ef6..09dd3206ef 100644 --- a/openhands-sdk/openhands/sdk/skills/skill.py +++ b/openhands-sdk/openhands/sdk/skills/skill.py @@ -1146,12 +1146,7 @@ def load_available_skills( return available -def to_prompt( - skills: list[Skill], - max_description_length: int = 1024, - *, - include_content: bool = False, -) -> str: +def to_prompt(skills: list[Skill], max_description_length: int = 1024) -> str: """Generate XML prompt block for available skills. Creates an `` XML block suitable for inclusion @@ -1160,8 +1155,6 @@ def to_prompt( Args: skills: List of skills to include in the prompt max_description_length: Maximum length for descriptions (default 1024) - include_content: Whether to include the full skill content in a - `` element. Defaults to False for progressive disclosure. Returns: XML string in AgentSkills format with name and description. The @@ -1205,14 +1198,12 @@ def to_prompt( break description = description or "" - # Calculate total truncated characters. If full content is included, - # the catalog is self-contained and does not need invoke_skill guidance. - total_truncated = 0 if include_content else content_truncated + # Calculate total truncated characters + total_truncated = content_truncated - # Truncate description if needed and add truncation indicator. + # Truncate description if needed and add truncation indicator if len(description) > max_description_length: - if not include_content: - total_truncated += len(description) - max_description_length + total_truncated += len(description) - max_description_length description = description[:max_description_length] if total_truncated > 0: @@ -1232,9 +1223,6 @@ def to_prompt( lines.append(" ") lines.append(f" {name}") lines.append(f" {description}") - if include_content: - content = xml_escape(skill.content.strip()) - lines.append(f" {content}") lines.append(" ") lines.append("") diff --git a/tests/sdk/skills/test_validation_prompt.py b/tests/sdk/skills/test_validation_prompt.py index 2f7203ed27..c5413254a2 100644 --- a/tests/sdk/skills/test_validation_prompt.py +++ b/tests/sdk/skills/test_validation_prompt.py @@ -57,24 +57,6 @@ def test_to_prompt_escapes_xml() -> None: assert '"quotes"' in result -def test_to_prompt_can_include_full_content() -> None: - """to_prompt() can render full skill content for prompt-only adapters.""" - skill = Skill( - name="review", - content="Use review rules & explain findings.", - description="Review pull requests.", - ) - - result = to_prompt([skill], include_content=True) - - assert "Review pull requests." in result - assert ( - "Use <strict> review rules & explain findings." - in result - ) - assert "invoke_skill" not in result - - def test_to_prompt_uses_content_fallback() -> None: """to_prompt() should use content when no description.""" skill = Skill(name="test", content="# Header\n\nActual content here.") @@ -83,18 +65,6 @@ def test_to_prompt_uses_content_fallback() -> None: assert "# Header" not in result -def test_to_prompt_include_content_with_content_fallback() -> None: - """include_content=True should include fallback description and full content.""" - skill = Skill(name="test", content="# Header\n\nBody.\n\nMore details.") - - result = to_prompt([skill], include_content=True) - - assert "Body." in result - assert "# Header\n\nBody.\n\nMore details." in result - assert "characters truncated" not in result - assert "invoke_skill" not in result - - def test_to_prompt_content_fallback_counts_remaining_as_truncated() -> None: """to_prompt() should count content after first line as truncated.""" # Content with header, description line, and additional content From 12140a6c003f5c0a5bc26cf37b81fc7ad1f17ef0 Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 27 Apr 2026 17:27:30 +0000 Subject: [PATCH 21/27] refactor: validate agent_context ACP compatibility at initialization time Move the unsupported-fields check from to_acp_prompt_context() (called at first step) into a dedicated validate_acp_compatibility() method on AgentContext, and call it during ACPAgent.initialize() alongside the existing tools/mcp_config/condenser checks. This makes misconfiguration fail at initialization time rather than at the first step(). to_acp_prompt_context() still delegates to validate_acp_compatibility() as a safety net. Co-authored-by: openhands --- .../openhands/sdk/agent/acp_agent.py | 2 + .../openhands/sdk/context/agent_context.py | 39 +++++++++++-------- tests/sdk/agent/test_acp_agent.py | 8 ++++ 3 files changed, 33 insertions(+), 16 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index 46eb25c030..a69e2831bb 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -837,6 +837,8 @@ def init_state( "ACPAgent does not support condenser; " "the ACP server manages its own context" ) + if self.agent_context: + self.agent_context.validate_acp_compatibility() from openhands.sdk.utils.async_executor import AsyncExecutor diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index b476da7c84..c62722cbf6 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -282,6 +282,27 @@ def get_system_message_suffix( return self.system_message_suffix.strip() return None + _ACP_COMPATIBLE_FIELDS: frozenset[str] = frozenset( + { + "skills", + "system_message_suffix", + "user_message_suffix", + "load_user_skills", + "load_public_skills", + "marketplace_path", + "current_datetime", + } + ) + + def validate_acp_compatibility(self) -> None: + """Raise if this context uses fields unsupported by ACP prompt mode.""" + unsupported = set(self.model_fields_set) - self._ACP_COMPATIBLE_FIELDS + 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. @@ -294,21 +315,7 @@ def to_acp_prompt_context(self) -> str | None: because ``LocalConversation`` already applies it through ``event.to_llm_message()``; including it would duplicate it. """ - compatible_fields = { - "skills", - "system_message_suffix", - "user_message_suffix", - "load_user_skills", - "load_public_skills", - "marketplace_path", - "current_datetime", - } - unsupported_fields = set(self.model_fields_set) - compatible_fields - if unsupported_fields: - fields = ", ".join(sorted(unsupported_fields)) - raise NotImplementedError( - f"ACP prompt context does not support AgentContext field(s): {fields}" - ) + self.validate_acp_compatibility() repo_skills, available_skills = self._partition_skills() @@ -318,7 +325,7 @@ def to_acp_prompt_context(self) -> str | None: or bool(self.user_message_suffix) or "current_datetime" in self.model_fields_set ) - + parts: list[str] = [] if has_explicit_prompt_context: formatted_datetime = self.get_formatted_datetime() diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index 391c5a0124..7ef64aa409 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -211,6 +211,14 @@ def test_allows_agent_context_for_prompt_extensions(self, tmp_path): 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=[ From 06329c2761efa088008d9429c9f140a50c9294b8 Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 27 Apr 2026 17:59:45 +0000 Subject: [PATCH 22/27] fix: use USERPROFILE fallback for Windows home directory in ChatGPT auth HOME is Unix-only; Windows uses USERPROFILE. Check both to ensure cross-platform compatibility when locating .codex/auth.json. Co-authored-by: openhands --- openhands-sdk/openhands/sdk/agent/acp_agent.py | 2 +- tests/sdk/agent/test_acp_agent.py | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index a69e2831bb..17fcfd005a 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -172,7 +172,7 @@ def _select_auth_method( if method_id in method_ids and env_var in env: return method_id if "chatgpt" in method_ids: - home = env.get("HOME") + home = env.get("HOME") or env.get("USERPROFILE") if home and (Path(home) / _CHATGPT_AUTH_PATH).is_file(): return "chatgpt" return None diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index 7ef64aa409..accf2d4696 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -2435,6 +2435,16 @@ def test_chatgpt_auth_file(self, tmp_path): assert _select_auth_method(methods, {"HOME": str(tmp_path)}) == "chatgpt" + def test_chatgpt_auth_file_userprofile(self, tmp_path): + """USERPROFILE is used as fallback on Windows.""" + methods = [self._make_auth_method("chatgpt")] + auth_dir = tmp_path / ".codex" + auth_dir.mkdir() + (auth_dir / "auth.json").write_text("{}", encoding="utf-8") + + env = {"USERPROFILE": str(tmp_path)} + assert _select_auth_method(methods, env) == "chatgpt" + def test_empty_auth_methods(self): assert _select_auth_method([], {}) is None From 46397c74298add87ef9e7303ac532807a912bc29 Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 27 Apr 2026 19:42:10 +0000 Subject: [PATCH 23/27] refactor: tag ACP-compatible fields via json_schema_extra instead of separate set Replace the static _ACP_COMPATIBLE_FIELDS frozenset with per-field json_schema_extra={"acp_compatible": True/False} tags. The validate_acp_compatibility() method now introspects these tags dynamically, so new fields declare their ACP compatibility at definition time rather than in a separate constant. Co-authored-by: openhands --- .../openhands/sdk/context/agent_context.py | 40 +++++++++++-------- 1 file changed, 24 insertions(+), 16 deletions(-) diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index c62722cbf6..eb10f73fc1 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") @@ -282,21 +292,19 @@ def get_system_message_suffix( return self.system_message_suffix.strip() return None - _ACP_COMPATIBLE_FIELDS: frozenset[str] = frozenset( - { - "skills", - "system_message_suffix", - "user_message_suffix", - "load_user_skills", - "load_public_skills", - "marketplace_path", - "current_datetime", - } - ) - def validate_acp_compatibility(self) -> None: - """Raise if this context uses fields unsupported by ACP prompt mode.""" - unsupported = set(self.model_fields_set) - self._ACP_COMPATIBLE_FIELDS + """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( From 12a4c4bda77dab7c31d9deb56fb18dc78cc45b3d Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 27 Apr 2026 19:56:43 +0000 Subject: [PATCH 24/27] Remove model_fields_set check for current_datetime in to_acp_prompt_context The has_explicit_prompt_context guard no longer inspects model_fields_set to detect whether current_datetime was explicitly provided. Since current_datetime uses default_factory=datetime.now, it is always truthy and cannot be checked with a simple bool like the other fields. Rather than relying on Pydantic internals (model_fields_set), we simply omit the check. The datetime section is already gated by has_explicit_prompt_context which requires at least one of skills, system_message_suffix, or user_message_suffix to be non-empty. An AgentContext with only current_datetime set and nothing else is not a practical use case for ACP. Co-authored-by: openhands --- openhands-sdk/openhands/sdk/context/agent_context.py | 1 - 1 file changed, 1 deletion(-) diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index eb10f73fc1..2a3fe1a061 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -331,7 +331,6 @@ def to_acp_prompt_context(self) -> str | None: bool(self.skills) or bool(self.system_message_suffix) or bool(self.user_message_suffix) - or "current_datetime" in self.model_fields_set ) parts: list[str] = [] From 967c4b9a912ea45e4cab6e06f7f2d3bb32982079 Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 27 Apr 2026 19:58:24 +0000 Subject: [PATCH 25/27] =?UTF-8?q?refactor:=20simplify=20to=5Facp=5Fprompt?= =?UTF-8?q?=5Fcontext=20=E2=80=94=20always=20emit=20datetime?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the has_explicit_prompt_context guard and model_fields_set check. current_datetime has default_factory=datetime.now so it is always set; the old guard only avoided emitting it when no other context was present. Now datetime is unconditionally emitted (consistent with get_system_message_suffix), and to_acp_prompt_context returns None only when current_datetime=None and no other sections contribute. Co-authored-by: openhands --- .../openhands/sdk/context/agent_context.py | 27 +++++++------------ tests/sdk/agent/test_acp_agent.py | 11 ++++++-- 2 files changed, 19 insertions(+), 19 deletions(-) diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index 2a3fe1a061..8fa20f98fd 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -327,25 +327,18 @@ def to_acp_prompt_context(self) -> str | None: repo_skills, available_skills = self._partition_skills() - has_explicit_prompt_context = ( - bool(self.skills) - or bool(self.system_message_suffix) - or bool(self.user_message_suffix) - ) - parts: list[str] = [] - if has_explicit_prompt_context: - formatted_datetime = self.get_formatted_datetime() - if formatted_datetime: - parts.append( - "\n".join( - [ - "", - f"The current date and time is: {formatted_datetime}", - "", - ] - ) + formatted_datetime = self.get_formatted_datetime() + if formatted_datetime: + parts.append( + "\n".join( + [ + "", + f"The current date and time is: {formatted_datetime}", + "", + ] ) + ) if repo_skills: # Reuse the shared template only for REPO_CONTEXT rendering; ACP diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index accf2d4696..d6649e5eeb 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -249,11 +249,18 @@ def test_agent_context_to_acp_prompt_context(self): assert "" not in prompt assert "Prefer concise responses." not in prompt - def test_agent_context_to_acp_prompt_context_returns_none_for_empty_skills(self): - context = AgentContext(skills=[]) + 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"}) From 15d4a63f6c2ce8158051126ecc505a5a3e08edeb Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 27 Apr 2026 20:05:23 +0000 Subject: [PATCH 26/27] refactor: deduplicate to_acp_prompt_context by reusing get_system_message_suffix to_acp_prompt_context now delegates to get_system_message_suffix() instead of manually assembling CURRENT_DATETIME, REPO_CONTEXT, SKILLS, and SYSTEM_CONTEXT sections. This ensures ACP agents receive the identical prompt layout as the general agent (minus secrets, which are rejected by validate_acp_compatibility). Co-authored-by: openhands --- .../openhands/sdk/context/agent_context.py | 72 +++---------------- tests/sdk/agent/test_acp_agent.py | 4 +- 2 files changed, 12 insertions(+), 64 deletions(-) diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index 8fa20f98fd..6da4f65c95 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -315,74 +315,22 @@ 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 must fail here instead of being silently approximated by - ACPAgent. + 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() - - repo_skills, available_skills = self._partition_skills() - - parts: list[str] = [] - formatted_datetime = self.get_formatted_datetime() - if formatted_datetime: - parts.append( - "\n".join( - [ - "", - f"The current date and time is: {formatted_datetime}", - "", - ] - ) - ) - - if repo_skills: - # Reuse the shared template only for REPO_CONTEXT rendering; ACP - # prompt-only sections are assembled explicitly below. - repo_context = render_template( - prompt_dir=str(PROMPT_DIR), - template_name="system_message_suffix.j2", - repo_skills=repo_skills, - system_message_suffix="", - secret_infos=[], - available_skills_prompt="", - current_datetime=None, - ).strip() - if repo_context: - parts.append(repo_context) - - if available_skills: - parts.append( - "\n".join( - [ - "", - "The following skills are available:", - to_prompt(available_skills), - "", - ] - ) - ) - - if self.system_message_suffix: - system_suffix = self.system_message_suffix.strip() - if system_suffix: - parts.append( - "\n".join( - [ - "", - system_suffix, - "", - ] - ) - ) - - if not parts: - return None - return "\n\n".join(parts) + # 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] diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index d6649e5eeb..2c3be30956 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -237,16 +237,16 @@ def test_agent_context_to_acp_prompt_context(self): 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 "" 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 "" not in prompt assert "Prefer concise responses." not in prompt def test_agent_context_to_acp_prompt_context_returns_none_when_empty(self): From 7b98547f998b79458361616192d6352bb08ebadb Mon Sep 17 00:00:00 2001 From: openhands Date: Mon, 27 Apr 2026 20:09:32 +0000 Subject: [PATCH 27/27] refactor: prefer ChatGPT subscription over API keys, use Path.home() _select_auth_method now checks for ~/.codex/auth.json first so ChatGPT subscription login takes precedence over explicit API key env vars. Replaced manual HOME/USERPROFILE env-var lookup with Path.home() from the Python standard library. Co-authored-by: openhands --- .../openhands/sdk/agent/acp_agent.py | 13 ++++-- tests/sdk/agent/test_acp_agent.py | 40 +++++++++++++------ 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/openhands-sdk/openhands/sdk/agent/acp_agent.py b/openhands-sdk/openhands/sdk/agent/acp_agent.py index 17fcfd005a..43bda63e8c 100644 --- a/openhands-sdk/openhands/sdk/agent/acp_agent.py +++ b/openhands-sdk/openhands/sdk/agent/acp_agent.py @@ -166,15 +166,20 @@ def _select_auth_method( Returns the ``id`` of the first matching method, or ``None`` if no 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 - if "chatgpt" in method_ids: - home = env.get("HOME") or env.get("USERPROFILE") - if home and (Path(home) / _CHATGPT_AUTH_PATH).is_file(): - return "chatgpt" return None diff --git a/tests/sdk/agent/test_acp_agent.py b/tests/sdk/agent/test_acp_agent.py index 2c3be30956..d63170bd78 100644 --- a/tests/sdk/agent/test_acp_agent.py +++ b/tests/sdk/agent/test_acp_agent.py @@ -2410,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"), @@ -2426,31 +2425,46 @@ 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"), ] - 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") - assert _select_auth_method(methods, {"HOME": str(tmp_path)}) == "chatgpt" + 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"), + ] + env = {"UNRELATED": "value"} + assert _select_auth_method(methods, env) is None - def test_chatgpt_auth_file_userprofile(self, tmp_path): - """USERPROFILE is used as fallback on Windows.""" + 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") - env = {"USERPROFILE": str(tmp_path)} - assert _select_auth_method(methods, env) == "chatgpt" + 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