From 10ac04ef1f890f24541b68bf0616622eb695c360 Mon Sep 17 00:00:00 2001 From: VascoSch92 Date: Thu, 21 May 2026 15:13:37 +0200 Subject: [PATCH 1/6] feat(agent-context): add load_project_skills flag with server-side injection AgentContext could auto-load user and public skills, but not project skills from the conversation workspace: project skills require the workspace path, which is not known at AgentContext validation time. Add a `load_project_skills` request flag on AgentContext and resolve it server-side at conversation start (where the workspace path is known), merging project skills into agent_context.skills with project precedence. This replaces the client-side project-skill injection in agent-canvas#707. --- .../agent_server/conversation_service.py | 43 ++++++++ .../openhands/sdk/context/agent_context.py | 13 +++ .../agent_server/test_conversation_service.py | 99 +++++++++++++++++++ 3 files changed, 155 insertions(+) diff --git a/openhands-agent-server/openhands/agent_server/conversation_service.py b/openhands-agent-server/openhands/agent_server/conversation_service.py index ecd3d1d891..ae8288dc32 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_service.py +++ b/openhands-agent-server/openhands/agent_server/conversation_service.py @@ -42,6 +42,7 @@ from openhands.sdk.event.conversation_state import ConversationStateUpdateEvent from openhands.sdk.git.exceptions import GitCommandError, GitRepositoryError from openhands.sdk.git.utils import run_git_command, validate_git_repository +from openhands.sdk.skills import load_available_skills from openhands.sdk.utils.cipher import Cipher from openhands.sdk.workspace import LocalWorkspace @@ -92,6 +93,41 @@ def _append_worktree_guidance( return agent.model_copy(update={"agent_context": updated_context}) +def _inject_project_skills( + agent: AgentBase, + workspace: LocalWorkspace, +) -> AgentBase: + """Resolve project skills from the workspace and merge them into the context. + + ``AgentContext.load_project_skills`` is only a request flag: project skills + require the workspace path, which is not known at AgentContext validation + time, so its ``_load_auto_skills`` validator cannot resolve them. We resolve + them here, at conversation start, where the (possibly worktree) workspace + path is available. This replaces the client-side project-skill injection that + agent-canvas does today (agent-canvas#707). + + Project skills take precedence over same-named skills already present on the + context, matching the precedence used by ``load_all_skills``. + """ + context = agent.agent_context + if context is None or not context.load_project_skills: + return agent + + project_skills = load_available_skills( + work_dir=workspace.working_dir, + include_user=False, + include_project=True, + include_public=False, + ) + if not project_skills: + return agent + + merged = {s.name: s for s in context.skills} + merged.update(project_skills) + updated_context = context.model_copy(update={"skills": list(merged.values())}) + return agent.model_copy(update={"agent_context": updated_context}) + + def _has_git_remote(repo_root: Path, remote: str = "origin") -> bool: try: run_git_command(["git", "remote", "get-url", remote], repo_root) @@ -556,6 +592,13 @@ async def _start_conversation( request = _prepare_request_workspace(request, conversation_id) + # Resolve project skills from the (possibly worktree) workspace and merge + # them into the agent context — see agent-canvas#707. Done here rather + # than in AgentContext because the workspace path is only known now. + if request.agent is not None: + agent = _inject_project_skills(request.agent, request.workspace) + request = request.model_copy(update={"agent": agent}) + # Dynamically register tools from client's registry if request.tool_module_qualnames: import importlib diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index 17d9508427..3b8d906358 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -96,6 +96,19 @@ class AgentContext(BaseModel): ), json_schema_extra={"acp_compatible": True}, ) + load_project_skills: bool = Field( + default=False, + description=( + "Whether to automatically load project skills from the conversation " + "workspace (e.g. .openhands/skills/, .cursorrules, agents.md). " + "Unlike load_user_skills / load_public_skills, this flag is NOT " + "resolved by AgentContext itself: project skills require the workspace " + "path, which is not known at validation time. The agent-server " + "resolves and merges them into ``skills`` at conversation start when " + "this flag is set." + ), + json_schema_extra={"acp_compatible": True}, + ) secrets: Mapping[str, SecretValue] | None = Field( default=None, description=( diff --git a/tests/agent_server/test_conversation_service.py b/tests/agent_server/test_conversation_service.py index 8fd3df7eac..cca6c0b735 100644 --- a/tests/agent_server/test_conversation_service.py +++ b/tests/agent_server/test_conversation_service.py @@ -21,6 +21,7 @@ AutoTitleSubscriber, ConversationService, _get_worktree_start_point, + _inject_project_skills, ) from openhands.agent_server.event_service import EventService from openhands.agent_server.models import ( @@ -2849,3 +2850,101 @@ def test_non_acp_agent_unchanged(self): # Should not raise and should not set any attribute EventService._setup_acp_activity_heartbeat(service, agent) assert not hasattr(agent, "_on_activity") + + +class TestInjectProjectSkills: + """Tests for _inject_project_skills (agent-canvas#707).""" + + def _agent(self, agent_context): + return Agent( + llm=LLM(model="gpt-4o", usage_id="test-llm"), + tools=[], + agent_context=agent_context, + ) + + def test_no_agent_context_is_noop(self): + from openhands.sdk.workspace import LocalWorkspace + + agent = self._agent(None) + workspace = LocalWorkspace(working_dir="/tmp/ws") + + assert _inject_project_skills(agent, workspace) is agent + + def test_flag_off_is_noop(self): + from openhands.sdk import AgentContext + from openhands.sdk.workspace import LocalWorkspace + + agent = self._agent(AgentContext(load_project_skills=False)) + workspace = LocalWorkspace(working_dir="/tmp/ws") + + with patch( + "openhands.agent_server.conversation_service.load_available_skills" + ) as mock_load: + result = _inject_project_skills(agent, workspace) + + # Resolution must not even be attempted when the flag is off. + mock_load.assert_not_called() + assert result is agent + + def test_no_project_skills_returns_agent_unchanged(self): + from openhands.sdk import AgentContext + from openhands.sdk.workspace import LocalWorkspace + + agent = self._agent(AgentContext(load_project_skills=True)) + workspace = LocalWorkspace(working_dir="/tmp/ws") + + with patch( + "openhands.agent_server.conversation_service.load_available_skills", + return_value={}, + ): + result = _inject_project_skills(agent, workspace) + + assert result is agent + + def test_project_skills_merged_into_context(self): + from openhands.sdk import AgentContext + from openhands.sdk.skills import Skill + from openhands.sdk.workspace import LocalWorkspace + + existing = Skill(name="existing", content="keep me") + project = Skill(name="project-skill", content="from workspace") + agent = self._agent(AgentContext(skills=[existing], load_project_skills=True)) + workspace = LocalWorkspace(working_dir="/tmp/ws") + + with patch( + "openhands.agent_server.conversation_service.load_available_skills", + return_value={project.name: project}, + ) as mock_load: + result = _inject_project_skills(agent, workspace) + + mock_load.assert_called_once_with( + work_dir="/tmp/ws", + include_user=False, + include_project=True, + include_public=False, + ) + assert result is not agent + assert result.agent_context is not None + names = {s.name for s in result.agent_context.skills} + assert names == {"existing", "project-skill"} + + def test_project_skill_overrides_same_named_existing(self): + from openhands.sdk import AgentContext + from openhands.sdk.skills import Skill + from openhands.sdk.workspace import LocalWorkspace + + existing = Skill(name="dup", content="old") + project = Skill(name="dup", content="new from workspace") + agent = self._agent(AgentContext(skills=[existing], load_project_skills=True)) + workspace = LocalWorkspace(working_dir="/tmp/ws") + + with patch( + "openhands.agent_server.conversation_service.load_available_skills", + return_value={project.name: project}, + ): + result = _inject_project_skills(agent, workspace) + + assert result.agent_context is not None + skills = result.agent_context.skills + assert len(skills) == 1 + assert skills[0].content == "new from workspace" From 153eb642314c247c390736879cdb45605e2f0aaa Mon Sep 17 00:00:00 2001 From: VascoSch92 Date: Thu, 21 May 2026 15:19:57 +0200 Subject: [PATCH 2/6] test: flatten _inject_project_skills tests into parametrized functions --- .../agent_server/test_conversation_service.py | 161 ++++++++---------- 1 file changed, 70 insertions(+), 91 deletions(-) diff --git a/tests/agent_server/test_conversation_service.py b/tests/agent_server/test_conversation_service.py index cca6c0b735..1addd143e4 100644 --- a/tests/agent_server/test_conversation_service.py +++ b/tests/agent_server/test_conversation_service.py @@ -34,7 +34,7 @@ UpdateConversationRequest, ) from openhands.agent_server.utils import safe_rmtree as _safe_rmtree -from openhands.sdk import LLM, Agent, Message +from openhands.sdk import LLM, Agent, AgentContext, Message from openhands.sdk.agent.acp_agent import ACPAgent from openhands.sdk.conversation.state import ( ConversationExecutionStatus, @@ -49,6 +49,7 @@ from openhands.sdk.secret import SecretSource, StaticSecret from openhands.sdk.security.confirmation_policy import NeverConfirm from openhands.sdk.security.risk import SecurityRisk +from openhands.sdk.skills import Skill from openhands.sdk.utils.cipher import Cipher from openhands.sdk.workspace import LocalWorkspace from openhands.tools.terminal.definition import TerminalAction, TerminalObservation @@ -2852,99 +2853,77 @@ def test_non_acp_agent_unchanged(self): assert not hasattr(agent, "_on_activity") -class TestInjectProjectSkills: - """Tests for _inject_project_skills (agent-canvas#707).""" - - def _agent(self, agent_context): - return Agent( - llm=LLM(model="gpt-4o", usage_id="test-llm"), - tools=[], - agent_context=agent_context, - ) - - def test_no_agent_context_is_noop(self): - from openhands.sdk.workspace import LocalWorkspace - - agent = self._agent(None) - workspace = LocalWorkspace(working_dir="/tmp/ws") - - assert _inject_project_skills(agent, workspace) is agent - - def test_flag_off_is_noop(self): - from openhands.sdk import AgentContext - from openhands.sdk.workspace import LocalWorkspace - - agent = self._agent(AgentContext(load_project_skills=False)) - workspace = LocalWorkspace(working_dir="/tmp/ws") - - with patch( - "openhands.agent_server.conversation_service.load_available_skills" - ) as mock_load: - result = _inject_project_skills(agent, workspace) - - # Resolution must not even be attempted when the flag is off. - mock_load.assert_not_called() - assert result is agent - - def test_no_project_skills_returns_agent_unchanged(self): - from openhands.sdk import AgentContext - from openhands.sdk.workspace import LocalWorkspace - - agent = self._agent(AgentContext(load_project_skills=True)) - workspace = LocalWorkspace(working_dir="/tmp/ws") - - with patch( - "openhands.agent_server.conversation_service.load_available_skills", - return_value={}, - ): - result = _inject_project_skills(agent, workspace) +def _agent_with_context(agent_context): + return Agent( + llm=LLM(model="gpt-4o", usage_id="test-llm"), + tools=[], + agent_context=agent_context, + ) - assert result is agent - def test_project_skills_merged_into_context(self): - from openhands.sdk import AgentContext - from openhands.sdk.skills import Skill - from openhands.sdk.workspace import LocalWorkspace +# Tests for _inject_project_skills (agent-canvas#707). - existing = Skill(name="existing", content="keep me") - project = Skill(name="project-skill", content="from workspace") - agent = self._agent(AgentContext(skills=[existing], load_project_skills=True)) - workspace = LocalWorkspace(working_dir="/tmp/ws") - with patch( - "openhands.agent_server.conversation_service.load_available_skills", - return_value={project.name: project}, - ) as mock_load: - result = _inject_project_skills(agent, workspace) - - mock_load.assert_called_once_with( - work_dir="/tmp/ws", - include_user=False, - include_project=True, - include_public=False, - ) - assert result is not agent - assert result.agent_context is not None - names = {s.name for s in result.agent_context.skills} - assert names == {"existing", "project-skill"} - - def test_project_skill_overrides_same_named_existing(self): - from openhands.sdk import AgentContext - from openhands.sdk.skills import Skill - from openhands.sdk.workspace import LocalWorkspace - - existing = Skill(name="dup", content="old") - project = Skill(name="dup", content="new from workspace") - agent = self._agent(AgentContext(skills=[existing], load_project_skills=True)) - workspace = LocalWorkspace(working_dir="/tmp/ws") +@pytest.mark.parametrize( + "agent_context, expect_load_called", + [ + # No agent context at all → nothing to inject into. + (None, False), + # Flag off → resolution must not even be attempted. + (AgentContext(load_project_skills=False), False), + # Flag on but no project skills found → resolution attempted, no change. + (AgentContext(load_project_skills=True), True), + ], +) +def test_inject_project_skills_noop(agent_context, expect_load_called): + agent = _agent_with_context(agent_context) + workspace = LocalWorkspace(working_dir="/tmp/ws") - with patch( - "openhands.agent_server.conversation_service.load_available_skills", - return_value={project.name: project}, - ): - result = _inject_project_skills(agent, workspace) + with patch( + "openhands.agent_server.conversation_service.load_available_skills", + return_value={}, + ) as mock_load: + result = _inject_project_skills(agent, workspace) + + assert result is agent + assert mock_load.called is expect_load_called + + +@pytest.mark.parametrize( + "existing, project, expected", + [ + # Distinct names → both kept. + ( + [Skill(name="existing", content="keep me")], + [Skill(name="project-skill", content="from workspace")], + {"existing": "keep me", "project-skill": "from workspace"}, + ), + # Same name → project skill overrides the existing one. + ( + [Skill(name="dup", content="old")], + [Skill(name="dup", content="new from workspace")], + {"dup": "new from workspace"}, + ), + ], +) +def test_inject_project_skills_merges_with_project_precedence( + existing, project, expected +): + agent = _agent_with_context(AgentContext(skills=existing, load_project_skills=True)) + workspace = LocalWorkspace(working_dir="/tmp/ws") - assert result.agent_context is not None - skills = result.agent_context.skills - assert len(skills) == 1 - assert skills[0].content == "new from workspace" + with patch( + "openhands.agent_server.conversation_service.load_available_skills", + return_value={s.name: s for s in project}, + ) as mock_load: + result = _inject_project_skills(agent, workspace) + + mock_load.assert_called_once_with( + work_dir="/tmp/ws", + include_user=False, + include_project=True, + include_public=False, + ) + assert result is not agent + assert result.agent_context is not None + assert {s.name: s.content for s in result.agent_context.skills} == expected From a62107195a1ea4709e43e433f5fb16c853c1d669 Mon Sep 17 00:00:00 2001 From: VascoSch92 Date: Thu, 21 May 2026 15:25:42 +0200 Subject: [PATCH 3/6] docs: trim docstrings and comments --- .../agent_server/conversation_service.py | 16 ++++------------ .../openhands/sdk/context/agent_context.py | 10 ++++------ .../agent_server/test_conversation_service.py | 18 +++++------------- 3 files changed, 13 insertions(+), 31 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/conversation_service.py b/openhands-agent-server/openhands/agent_server/conversation_service.py index ae8288dc32..04bd28e1f2 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_service.py +++ b/openhands-agent-server/openhands/agent_server/conversation_service.py @@ -99,15 +99,9 @@ def _inject_project_skills( ) -> AgentBase: """Resolve project skills from the workspace and merge them into the context. - ``AgentContext.load_project_skills`` is only a request flag: project skills - require the workspace path, which is not known at AgentContext validation - time, so its ``_load_auto_skills`` validator cannot resolve them. We resolve - them here, at conversation start, where the (possibly worktree) workspace - path is available. This replaces the client-side project-skill injection that - agent-canvas does today (agent-canvas#707). - - Project skills take precedence over same-named skills already present on the - context, matching the precedence used by ``load_all_skills``. + Resolved here, at conversation start, because the workspace path is not known + at AgentContext validation time. Project skills take precedence over + same-named skills already on the context (agent-canvas#707). """ context = agent.agent_context if context is None or not context.load_project_skills: @@ -592,9 +586,7 @@ async def _start_conversation( request = _prepare_request_workspace(request, conversation_id) - # Resolve project skills from the (possibly worktree) workspace and merge - # them into the agent context — see agent-canvas#707. Done here rather - # than in AgentContext because the workspace path is only known now. + # Merge project skills from the workspace into the agent context. if request.agent is not None: agent = _inject_project_skills(request.agent, request.workspace) request = request.model_copy(update={"agent": agent}) diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index 3b8d906358..b43ceb0bdb 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -100,12 +100,10 @@ class AgentContext(BaseModel): default=False, description=( "Whether to automatically load project skills from the conversation " - "workspace (e.g. .openhands/skills/, .cursorrules, agents.md). " - "Unlike load_user_skills / load_public_skills, this flag is NOT " - "resolved by AgentContext itself: project skills require the workspace " - "path, which is not known at validation time. The agent-server " - "resolves and merges them into ``skills`` at conversation start when " - "this flag is set." + "workspace (e.g. .openhands/skills/, .cursorrules, agents.md). Unlike " + "load_user_skills / load_public_skills, this flag is not resolved by " + "AgentContext itself (the workspace path is unknown at validation " + "time); the agent-server resolves it at conversation start." ), json_schema_extra={"acp_compatible": True}, ) diff --git a/tests/agent_server/test_conversation_service.py b/tests/agent_server/test_conversation_service.py index 1addd143e4..0ea9b8cf24 100644 --- a/tests/agent_server/test_conversation_service.py +++ b/tests/agent_server/test_conversation_service.py @@ -2861,18 +2861,12 @@ def _agent_with_context(agent_context): ) -# Tests for _inject_project_skills (agent-canvas#707). - - @pytest.mark.parametrize( "agent_context, expect_load_called", [ - # No agent context at all → nothing to inject into. - (None, False), - # Flag off → resolution must not even be attempted. - (AgentContext(load_project_skills=False), False), - # Flag on but no project skills found → resolution attempted, no change. - (AgentContext(load_project_skills=True), True), + (None, False), # no context to inject into + (AgentContext(load_project_skills=False), False), # flag off + (AgentContext(load_project_skills=True), True), # flag on, no skills found ], ) def test_inject_project_skills_noop(agent_context, expect_load_called): @@ -2892,14 +2886,12 @@ def test_inject_project_skills_noop(agent_context, expect_load_called): @pytest.mark.parametrize( "existing, project, expected", [ - # Distinct names → both kept. - ( + ( # distinct names → both kept [Skill(name="existing", content="keep me")], [Skill(name="project-skill", content="from workspace")], {"existing": "keep me", "project-skill": "from workspace"}, ), - # Same name → project skill overrides the existing one. - ( + ( # same name → project skill wins [Skill(name="dup", content="old")], [Skill(name="dup", content="new from workspace")], {"dup": "new from workspace"}, From e2fb5ba06890e5c2753dcc431b1e44da63bc487e Mon Sep 17 00:00:00 2001 From: VascoSch92 Date: Thu, 21 May 2026 16:16:08 +0200 Subject: [PATCH 4/6] refactor: resolve project skills in LocalConversation, not agent-server Move project-skill resolution from the agent-server into LocalConversation._ensure_plugins_loaded, where the workspace path is also known. This makes AgentContext(load_project_skills=True) behave consistently for all SDK users (standalone LocalConversation as well as agent-server, which runs through LocalConversation) instead of being silently ignored outside agent-server. The agent-server _inject_project_skills helper is removed as redundant. Tests move to the SDK conversation suite. --- .../agent_server/conversation_service.py | 35 ------ .../conversation/impl/local_conversation.py | 27 ++++- .../agent_server/test_conversation_service.py | 72 +----------- .../test_repo_root_project_skills.py | 104 +++++++++++++++++- 4 files changed, 128 insertions(+), 110 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/conversation_service.py b/openhands-agent-server/openhands/agent_server/conversation_service.py index 04bd28e1f2..ecd3d1d891 100644 --- a/openhands-agent-server/openhands/agent_server/conversation_service.py +++ b/openhands-agent-server/openhands/agent_server/conversation_service.py @@ -42,7 +42,6 @@ from openhands.sdk.event.conversation_state import ConversationStateUpdateEvent from openhands.sdk.git.exceptions import GitCommandError, GitRepositoryError from openhands.sdk.git.utils import run_git_command, validate_git_repository -from openhands.sdk.skills import load_available_skills from openhands.sdk.utils.cipher import Cipher from openhands.sdk.workspace import LocalWorkspace @@ -93,35 +92,6 @@ def _append_worktree_guidance( return agent.model_copy(update={"agent_context": updated_context}) -def _inject_project_skills( - agent: AgentBase, - workspace: LocalWorkspace, -) -> AgentBase: - """Resolve project skills from the workspace and merge them into the context. - - Resolved here, at conversation start, because the workspace path is not known - at AgentContext validation time. Project skills take precedence over - same-named skills already on the context (agent-canvas#707). - """ - context = agent.agent_context - if context is None or not context.load_project_skills: - return agent - - project_skills = load_available_skills( - work_dir=workspace.working_dir, - include_user=False, - include_project=True, - include_public=False, - ) - if not project_skills: - return agent - - merged = {s.name: s for s in context.skills} - merged.update(project_skills) - updated_context = context.model_copy(update={"skills": list(merged.values())}) - return agent.model_copy(update={"agent_context": updated_context}) - - def _has_git_remote(repo_root: Path, remote: str = "origin") -> bool: try: run_git_command(["git", "remote", "get-url", remote], repo_root) @@ -586,11 +556,6 @@ async def _start_conversation( request = _prepare_request_workspace(request, conversation_id) - # Merge project skills from the workspace into the agent context. - if request.agent is not None: - agent = _inject_project_skills(request.agent, request.workspace) - request = request.model_copy(update={"agent": agent}) - # Dynamically register tools from client's registry if request.tool_module_qualnames: import importlib diff --git a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py index a6bb1ec60e..52c7ddb551 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py @@ -58,6 +58,7 @@ from openhands.sdk.security.confirmation_policy import ( ConfirmationPolicyBase, ) +from openhands.sdk.skills import load_available_skills from openhands.sdk.skills.utils import expand_mcp_variables from openhands.sdk.subagent import ( AgentDefinition, @@ -504,6 +505,26 @@ def _ensure_plugins_loaded(self) -> None: logger.info(f"Loaded {len(self._plugin_specs)} plugin(s) via Conversation") + # Resolve project skills from the workspace. AgentContext can't do this + # itself (the workspace path is unknown at validation time), so it is done + # here, where the path is known. Project skills take precedence over + # same-named skills already on the context. + project_skills_loaded = False + if merged_context is not None and merged_context.load_project_skills: + project_skills = load_available_skills( + work_dir=self.workspace.working_dir, + include_user=False, + include_project=True, + include_public=False, + ) + if project_skills: + merged_skills = {s.name: s for s in merged_context.skills} + merged_skills.update(project_skills) + merged_context = merged_context.model_copy( + update={"skills": list(merged_skills.values())} + ) + project_skills_loaded = True + # Expand MCP config variables with per-conversation secrets # This handles ${VAR} and ${VAR:-default} placeholders: # - Variables referencing secrets injected via API are expanded to secret values @@ -521,9 +542,9 @@ def _ensure_plugins_loaded(self) -> None: ) logger.debug("Expanded MCP config variables") - # Update agent with merged content only if we have plugins or MCP config - # Skip update when nothing changed to avoid unnecessary agent state mutations - if self._plugin_specs or has_mcp_config: + # Update agent with merged content only if something changed. + # Skip update otherwise to avoid unnecessary agent state mutations. + if self._plugin_specs or has_mcp_config or project_skills_loaded: self.agent = self.agent.model_copy( update={ "agent_context": merged_context, diff --git a/tests/agent_server/test_conversation_service.py b/tests/agent_server/test_conversation_service.py index 0ea9b8cf24..8fd3df7eac 100644 --- a/tests/agent_server/test_conversation_service.py +++ b/tests/agent_server/test_conversation_service.py @@ -21,7 +21,6 @@ AutoTitleSubscriber, ConversationService, _get_worktree_start_point, - _inject_project_skills, ) from openhands.agent_server.event_service import EventService from openhands.agent_server.models import ( @@ -34,7 +33,7 @@ UpdateConversationRequest, ) from openhands.agent_server.utils import safe_rmtree as _safe_rmtree -from openhands.sdk import LLM, Agent, AgentContext, Message +from openhands.sdk import LLM, Agent, Message from openhands.sdk.agent.acp_agent import ACPAgent from openhands.sdk.conversation.state import ( ConversationExecutionStatus, @@ -49,7 +48,6 @@ from openhands.sdk.secret import SecretSource, StaticSecret from openhands.sdk.security.confirmation_policy import NeverConfirm from openhands.sdk.security.risk import SecurityRisk -from openhands.sdk.skills import Skill from openhands.sdk.utils.cipher import Cipher from openhands.sdk.workspace import LocalWorkspace from openhands.tools.terminal.definition import TerminalAction, TerminalObservation @@ -2851,71 +2849,3 @@ def test_non_acp_agent_unchanged(self): # Should not raise and should not set any attribute EventService._setup_acp_activity_heartbeat(service, agent) assert not hasattr(agent, "_on_activity") - - -def _agent_with_context(agent_context): - return Agent( - llm=LLM(model="gpt-4o", usage_id="test-llm"), - tools=[], - agent_context=agent_context, - ) - - -@pytest.mark.parametrize( - "agent_context, expect_load_called", - [ - (None, False), # no context to inject into - (AgentContext(load_project_skills=False), False), # flag off - (AgentContext(load_project_skills=True), True), # flag on, no skills found - ], -) -def test_inject_project_skills_noop(agent_context, expect_load_called): - agent = _agent_with_context(agent_context) - workspace = LocalWorkspace(working_dir="/tmp/ws") - - with patch( - "openhands.agent_server.conversation_service.load_available_skills", - return_value={}, - ) as mock_load: - result = _inject_project_skills(agent, workspace) - - assert result is agent - assert mock_load.called is expect_load_called - - -@pytest.mark.parametrize( - "existing, project, expected", - [ - ( # distinct names → both kept - [Skill(name="existing", content="keep me")], - [Skill(name="project-skill", content="from workspace")], - {"existing": "keep me", "project-skill": "from workspace"}, - ), - ( # same name → project skill wins - [Skill(name="dup", content="old")], - [Skill(name="dup", content="new from workspace")], - {"dup": "new from workspace"}, - ), - ], -) -def test_inject_project_skills_merges_with_project_precedence( - existing, project, expected -): - agent = _agent_with_context(AgentContext(skills=existing, load_project_skills=True)) - workspace = LocalWorkspace(working_dir="/tmp/ws") - - with patch( - "openhands.agent_server.conversation_service.load_available_skills", - return_value={s.name: s for s in project}, - ) as mock_load: - result = _inject_project_skills(agent, workspace) - - mock_load.assert_called_once_with( - work_dir="/tmp/ws", - include_user=False, - include_project=True, - include_public=False, - ) - assert result is not agent - assert result.agent_context is not None - assert {s.name: s.content for s in result.agent_context.skills} == expected diff --git a/tests/sdk/conversation/test_repo_root_project_skills.py b/tests/sdk/conversation/test_repo_root_project_skills.py index 052b3f247d..a21f8417e3 100644 --- a/tests/sdk/conversation/test_repo_root_project_skills.py +++ b/tests/sdk/conversation/test_repo_root_project_skills.py @@ -7,10 +7,22 @@ from openhands.sdk.conversation.impl.local_conversation import LocalConversation from openhands.sdk.event import SystemPromptEvent from openhands.sdk.llm import Message, TextContent -from openhands.sdk.skills import load_project_skills +from openhands.sdk.skills import Skill, load_project_skills from openhands.sdk.testing import TestLLM +def _agent(agent_context: AgentContext) -> Agent: + return Agent( + llm=TestLLM.from_messages( + [Message(role="assistant", content=[TextContent(text="ok")])], + model="test-model", + ), + tools=[], + include_default_tools=[], + agent_context=agent_context, + ) + + def test_system_prompt_includes_repo_root_agents_md_when_workdir_is_subdir( tmp_path: Path, ): @@ -69,3 +81,93 @@ def test_system_prompt_includes_repo_root_agents_md_when_workdir_is_subdir( assert "SENTINEL_ROOT_123" in system_prompt_event.dynamic_context.text conversation.close() + + +def test_load_project_skills_flag_injects_skills_in_standalone_sdk(tmp_path: Path): + """``AgentContext(load_project_skills=True)`` works without agent-server. + + LocalConversation resolves project skills from the workspace at startup, + so the flag behaves consistently for standalone SDK usage (agent-canvas#574). + """ + (tmp_path / "AGENTS.md").write_text("# Guidelines\n\nSENTINEL_FLAG_456\n") + + agent = _agent( + AgentContext( + load_project_skills=True, + current_datetime="2026-01-01T00:00:00Z", + ) + ) + conversation = LocalConversation( + agent=agent, + workspace=tmp_path, + persistence_dir=tmp_path / "conversation", + delete_on_close=True, + ) + conversation.send_message("hi") + + # Skills are merged into the live agent context... + assert conversation.agent.agent_context is not None + assert "agents" in {s.name for s in conversation.agent.agent_context.skills} + # ...and rendered into the system prompt. + system_prompt_event = next( + e for e in conversation.state.events if isinstance(e, SystemPromptEvent) + ) + assert system_prompt_event.dynamic_context is not None + assert "SENTINEL_FLAG_456" in system_prompt_event.dynamic_context.text + + conversation.close() + + +def test_load_project_skills_flag_off_does_not_inject(tmp_path: Path): + """With the flag unset (default), project skills are not loaded.""" + (tmp_path / "AGENTS.md").write_text("# Guidelines\n\nSENTINEL_OFF_789\n") + + agent = _agent(AgentContext(current_datetime="2026-01-01T00:00:00Z")) + conversation = LocalConversation( + agent=agent, + workspace=tmp_path, + persistence_dir=tmp_path / "conversation", + delete_on_close=True, + ) + conversation.send_message("hi") + + assert conversation.agent.agent_context is not None + assert conversation.agent.agent_context.skills == [] + system_prompt_event = next( + e for e in conversation.state.events if isinstance(e, SystemPromptEvent) + ) + dynamic = system_prompt_event.dynamic_context + assert dynamic is None or "SENTINEL_OFF_789" not in dynamic.text + + conversation.close() + + +def test_load_project_skills_flag_merges_with_project_precedence(tmp_path: Path): + """Project skills override same-named context skills; others are preserved.""" + (tmp_path / "AGENTS.md").write_text("# Guidelines\n\nSENTINEL_NEW\n") + + agent = _agent( + AgentContext( + skills=[ + Skill(name="keep", content="keep me"), + Skill(name="agents", content="OLD_CONTENT"), + ], + load_project_skills=True, + current_datetime="2026-01-01T00:00:00Z", + ) + ) + conversation = LocalConversation( + agent=agent, + workspace=tmp_path, + persistence_dir=tmp_path / "conversation", + delete_on_close=True, + ) + conversation.send_message("hi") + + assert conversation.agent.agent_context is not None + skills = {s.name: s for s in conversation.agent.agent_context.skills} + assert skills["keep"].content == "keep me" + assert "SENTINEL_NEW" in skills["agents"].content + assert "OLD_CONTENT" not in skills["agents"].content + + conversation.close() From 718e73f1ca434d7b653f1c1255ebcef6948097e6 Mon Sep 17 00:00:00 2001 From: VascoSch92 Date: Thu, 21 May 2026 16:28:38 +0200 Subject: [PATCH 5/6] docs: correct load_project_skills resolution note (LocalConversation, lazy) --- openhands-sdk/openhands/sdk/context/agent_context.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index b43ceb0bdb..ab72e34818 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -103,7 +103,8 @@ class AgentContext(BaseModel): "workspace (e.g. .openhands/skills/, .cursorrules, agents.md). Unlike " "load_user_skills / load_public_skills, this flag is not resolved by " "AgentContext itself (the workspace path is unknown at validation " - "time); the agent-server resolves it at conversation start." + "time); LocalConversation resolves it lazily on the first " + "send_message() / run(), when the workspace is known." ), json_schema_extra={"acp_compatible": True}, ) From d67124f21e3926f30af5d77a3a01660118f368b8 Mon Sep 17 00:00:00 2001 From: VascoSch92 Date: Thu, 21 May 2026 16:31:50 +0200 Subject: [PATCH 6/6] =?UTF-8?q?fix:=20address=20review=20=E2=80=94=20doc?= =?UTF-8?q?=20example=20paths=20and=20best-effort=20project-skill=20load?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Correct the load_project_skills doc examples to match what is actually loaded (.openhands/skills/, AGENTS.md). - Guard project-skill loading so a workspace read error cannot block conversation start, with a test covering the failure path. --- .../openhands/sdk/context/agent_context.py | 2 +- .../conversation/impl/local_conversation.py | 22 +++++++++---- .../test_repo_root_project_skills.py | 32 +++++++++++++++++++ 3 files changed, 49 insertions(+), 7 deletions(-) diff --git a/openhands-sdk/openhands/sdk/context/agent_context.py b/openhands-sdk/openhands/sdk/context/agent_context.py index ab72e34818..7d30c9e551 100644 --- a/openhands-sdk/openhands/sdk/context/agent_context.py +++ b/openhands-sdk/openhands/sdk/context/agent_context.py @@ -100,7 +100,7 @@ class AgentContext(BaseModel): default=False, description=( "Whether to automatically load project skills from the conversation " - "workspace (e.g. .openhands/skills/, .cursorrules, agents.md). Unlike " + "workspace (e.g. .openhands/skills/, AGENTS.md). Unlike " "load_user_skills / load_public_skills, this flag is not resolved by " "AgentContext itself (the workspace path is unknown at validation " "time); LocalConversation resolves it lazily on the first " diff --git a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py index 52c7ddb551..1a70b592ee 100644 --- a/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py +++ b/openhands-sdk/openhands/sdk/conversation/impl/local_conversation.py @@ -511,12 +511,22 @@ def _ensure_plugins_loaded(self) -> None: # same-named skills already on the context. project_skills_loaded = False if merged_context is not None and merged_context.load_project_skills: - project_skills = load_available_skills( - work_dir=self.workspace.working_dir, - include_user=False, - include_project=True, - include_public=False, - ) + # Best-effort: a failure to load project skills must not prevent the + # conversation from starting. (load_available_skills already guards + # the project source internally; this is belt-and-suspenders.) + try: + project_skills = load_available_skills( + work_dir=self.workspace.working_dir, + include_user=False, + include_project=True, + include_public=False, + ) + except Exception: + logger.warning( + "Failed to load project skills; continuing without them", + exc_info=True, + ) + project_skills = {} if project_skills: merged_skills = {s.name: s for s in merged_context.skills} merged_skills.update(project_skills) diff --git a/tests/sdk/conversation/test_repo_root_project_skills.py b/tests/sdk/conversation/test_repo_root_project_skills.py index a21f8417e3..a0e38b2a37 100644 --- a/tests/sdk/conversation/test_repo_root_project_skills.py +++ b/tests/sdk/conversation/test_repo_root_project_skills.py @@ -1,6 +1,7 @@ from __future__ import annotations from pathlib import Path +from unittest.mock import patch from openhands.sdk.agent import Agent from openhands.sdk.context.agent_context import AgentContext @@ -171,3 +172,34 @@ def test_load_project_skills_flag_merges_with_project_precedence(tmp_path: Path) assert "OLD_CONTENT" not in skills["agents"].content conversation.close() + + +def test_load_project_skills_failure_does_not_block_conversation(tmp_path: Path): + """Project-skill loading is best-effort: a load error must not break startup.""" + (tmp_path / "AGENTS.md").write_text("# Guidelines\n\nSENTINEL\n") + + agent = _agent( + AgentContext( + skills=[Skill(name="keep", content="keep me")], + load_project_skills=True, + current_datetime="2026-01-01T00:00:00Z", + ) + ) + conversation = LocalConversation( + agent=agent, + workspace=tmp_path, + persistence_dir=tmp_path / "conversation", + delete_on_close=True, + ) + + with patch( + "openhands.sdk.conversation.impl.local_conversation.load_available_skills", + side_effect=PermissionError("workspace unreadable"), + ): + conversation.send_message("hi") # must not raise + + # Conversation started; pre-existing skills are untouched. + assert conversation.agent.agent_context is not None + assert {s.name for s in conversation.agent.agent_context.skills} == {"keep"} + + conversation.close()