Merge branch 'main' into fix-project-skills-loading

Co-authored-by: openhands <openhands@all-hands.dev>

openhands/app_server/app_conversation/app_conversation_service_base.py

@@ -84,6 +84,7 @@ class AppConversationServiceBase(AppConversationService, ABC):
             # Load skills from all sources
             sandbox_skills = load_sandbox_skills(sandbox)
             global_skills = load_global_skills()
+
             # Load user skills from ~/.openhands/skills/ directory
             # Uses the SDK's load_user_skills() function which handles loading from
             # ~/.openhands/skills/ and ~/.openhands/microagents/ (for backward compatibility)
@@ -101,6 +102,7 @@ class AppConversationServiceBase(AppConversationService, ABC):
                 remote_workspace, selected_repository, working_dir, self.user_context
             )
 
+            # Load repository skills via remote workspace (org/repo policies)
             repo_skills = await load_repo_skills(
                 remote_workspace, selected_repository, working_dir
             )
@@ -108,7 +110,13 @@ class AppConversationServiceBase(AppConversationService, ABC):
             # Merge all skills (later lists override earlier ones)
             # Precedence: sandbox < global < user < org < repo
             all_skills = merge_skills(
-                [sandbox_skills, global_skills, user_skills, org_skills, repo_skills]
+                [
+                    sandbox_skills,
+                    global_skills,
+                    user_skills,
+                    org_skills,
+                    repo_skills,
+                ]
             )
 
             _logger.info(

openhands/app_server/app_conversation/skill_loader.py

@@ -8,16 +8,20 @@ This module provides functions to load skills from various sources:
 All skills are used in V1 conversations.
 """
 
+import io
 import logging
 import os
 from pathlib import Path
 
+import frontmatter
+
 import openhands
 from openhands.app_server.sandbox.sandbox_models import SandboxInfo
 from openhands.app_server.user.user_context import UserContext
 from openhands.integrations.provider import ProviderType
 from openhands.integrations.service_types import AuthenticationError
 from openhands.sdk.context.skills import Skill
+from openhands.sdk.context.skills.trigger import KeywordTrigger, TaskTrigger
 from openhands.sdk.workspace.remote.async_remote_workspace import AsyncRemoteWorkspace
 
 _logger = logging.getLogger(__name__)
@@ -49,7 +53,7 @@ def _find_and_load_global_skill_files(skill_dir: Path) -> list[Skill]:
         # Load skills from the found files
         for file_path in md_files:
             try:
-                skill = Skill.load(file_path, skill_dir)
+                skill = Skill.load(file_path, skill_base_dir=skill_dir)
                 skills.append(skill)
                 _logger.debug(f'Loaded global skill: {skill.name} from {file_path}')
             except Exception as e:
@@ -122,6 +126,49 @@ def _determine_repo_root(working_dir: str, selected_repository: str | None) -> s
     return working_dir
 
 
+def _skill_from_markdown_content(
+    *,
+    name: str,
+    content: str,
+    source: str,
+) -> Skill:
+    """Create an SDK Skill from markdown content stored in a remote workspace.
+
+    We cannot call Skill.load() here because that API reads from local disk.
+    """
+    loaded = frontmatter.load(io.StringIO(content))
+    body = loaded.content
+    metadata = loaded.metadata or {}
+
+    # Prefer explicit frontmatter name, otherwise derive from filename
+    skill_name = str(metadata.get('name', name))
+
+    triggers = metadata.get('triggers')
+    if triggers is None:
+        trigger_obj = None
+    else:
+        if not isinstance(triggers, list):
+            raise ValueError('Skill triggers must be a list')
+
+        # inputs -> task triggers in SDK
+        if 'inputs' in metadata:
+            # Match SDK behavior: ensure /{name} trigger exists
+            slash = f'/{skill_name}'
+            if slash not in triggers:
+                triggers = [*triggers, slash]
+            trigger_obj = TaskTrigger(triggers=[str(t) for t in triggers])
+        else:
+            trigger_obj = KeywordTrigger(keywords=[str(t) for t in triggers])
+
+    return Skill(
+        name=skill_name,
+        content=body,
+        source=source,
+        trigger=trigger_obj,
+        mcp_tools=metadata.get('mcp_tools'),
+    )
+
+
 async def _is_gitlab_repository(repo_name: str, user_context: UserContext) -> bool:
     """Check if a repository is hosted on GitLab.
 
@@ -265,8 +312,11 @@ async def _load_special_files(
 
         if content:
             try:
-                # Use simple string path to avoid Path filesystem operations
-                skill = Skill.load(path=filename, skill_dir=None, file_content=content)
+                skill = _skill_from_markdown_content(
+                    name=Path(filename).stem,
+                    content=content,
+                    source=file_path,
+                )
                 skills.append(skill)
                 _logger.debug(f'Loaded special file skill: {skill.name}')
             except Exception as e:
@@ -315,9 +365,10 @@ async def _find_and_load_skill_md_files(
                     # Calculate relative path for skill name
                     rel_path = file_path.replace(f'{skill_dir}/', '')
                     try:
-                        # Use simple string path to avoid Path filesystem operations
-                        skill = Skill.load(
-                            path=rel_path, skill_dir=None, file_content=content
+                        skill = _skill_from_markdown_content(
+                            name=Path(rel_path).stem,
+                            content=content,
+                            source=file_path,
                         )
                         skills.append(skill)
                         _logger.debug(f'Loaded repo skill: {skill.name}')

tests/runtime/test_repo_skills_loading.py

@@ -0,0 +1,97 @@
+"""Runtime test: repo skills from .openhands/skills are discoverable in a remote workspace.
+
+This is a light-weight integration test that boots a DockerRuntime (action execution
+server), writes a .openhands/skills/repo.md inside the sandbox workspace, then runs the
+V1 app-server skill loader against that workspace via a minimal execute_command adapter.
+
+We specifically verify that a markdown file with NO `triggers` frontmatter produces a
+Skill with trigger=None (always-active repo context).
+"""
+
+from dataclasses import dataclass
+
+import pytest
+
+from openhands.app_server.app_conversation import skill_loader
+from openhands.events.action.action import ActionSecurityRisk
+from openhands.events.action.commands import CmdRunAction
+from tests.runtime.conftest import _close_test_runtime, _load_runtime
+
+
+@dataclass
+class _CommandResult:
+    exit_code: int
+    stdout: str
+
+
+class _RuntimeWorkspaceAdapter:
+    """Minimal AsyncRemoteWorkspace-like adapter for skill_loader tests."""
+
+    def __init__(self, runtime):
+        self._runtime = runtime
+
+    async def execute_command(
+        self, command: str, cwd: str | None = None, timeout: float = 30.0
+    ):
+        action = CmdRunAction(
+            command=command,
+            cwd=cwd,
+            security_risk=ActionSecurityRisk.LOW,
+        )
+        obs = self._runtime.run_action(action)
+        return _CommandResult(exit_code=obs.exit_code, stdout=obs.content or '')
+
+
+@pytest.mark.asyncio
+async def test_repo_skill_repo_md_without_triggers_is_repo_context(
+    temp_dir, runtime_cls, run_as_openhands
+):
+    if runtime_cls.__name__ != 'DockerRuntime':
+        pytest.skip(
+            'This runtime/integration test is only implemented for DockerRuntime'
+        )
+
+    runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands)
+    try:
+        sandbox_root = config.workspace_mount_path_in_sandbox
+        repo_root = sandbox_root
+
+        # Create .openhands/skills/repo.md in the *sandbox workspace*.
+        # No triggers frontmatter => trigger=None
+        cmd = (
+            f'mkdir -p {repo_root}/.openhands/skills && '
+            f"cat > {repo_root}/.openhands/skills/repo.md <<'EOF'\n"
+            'This is repo context loaded from .openhands/skills/repo.md\n'
+            'EOF\n'
+        )
+        runtime.run_action(
+            CmdRunAction(command=cmd, security_risk=ActionSecurityRisk.LOW)
+        )
+
+        ws = _RuntimeWorkspaceAdapter(runtime)
+
+        # selected_repository=None -> repo_root = working_dir
+        # Sanity check: file is actually present in the sandbox.
+        check = runtime.run_action(
+            CmdRunAction(
+                command=f'ls -la {repo_root}/.openhands/skills && cat {repo_root}/.openhands/skills/repo.md',
+                security_risk=ActionSecurityRisk.LOW,
+            )
+        )
+        assert check.exit_code == 0, check
+
+        skills = await skill_loader.load_repo_skills(
+            ws, selected_repository=None, working_dir=repo_root
+        )
+
+        repo_skill = next(
+            (s for s in skills if s.name.endswith('repo') or s.name == 'repo'), None
+        )
+        assert repo_skill is not None, (
+            f'Expected repo skill; got {[s.name for s in skills]}'
+        )
+        assert repo_skill.trigger is None
+        assert 'This is repo context loaded' in repo_skill.content
+
+    finally:
+        _close_test_runtime(runtime)

tests/unit/app_server/test_skill_loader.py

@@ -212,55 +212,40 @@ class TestLoadSpecialFiles:
     @patch(
         'openhands.app_server.app_conversation.skill_loader._read_file_from_workspace'
     )
-    @patch('openhands.app_server.app_conversation.skill_loader.Skill')
     async def test_load_all_special_files(
-        self,
-        mock_skill_class,
-        mock_read_file,
-        mock_async_remote_workspace,
-        mock_skills_list,
+        self, mock_read_file, mock_async_remote_workspace
     ):
         """Test loading all special files successfully."""
-        # Mock reading files - return content for each special file
         mock_read_file.side_effect = [
             'cursorrules content',
             'agents.md content',
             'agent.md content',
         ]
 
-        # Mock skill creation
-        mock_skill_class.load.side_effect = mock_skills_list
-
         result = await _load_special_files(
             mock_async_remote_workspace, '/repo', '/workspace'
         )
 
-        assert len(result) == 3
-        assert result == mock_skills_list
+        assert [s.name for s in result] == ['.cursorrules', 'agents', 'agent']
         assert mock_read_file.call_count == 3
-        assert mock_skill_class.load.call_count == 3
 
     @pytest.mark.asyncio
     @patch(
         'openhands.app_server.app_conversation.skill_loader._read_file_from_workspace'
     )
-    @patch('openhands.app_server.app_conversation.skill_loader.Skill')
     async def test_load_partial_special_files(
-        self, mock_skill_class, mock_read_file, mock_async_remote_workspace, mock_skill
+        self, mock_read_file, mock_async_remote_workspace
     ):
         """Test loading when only some special files exist."""
-        # Only .cursorrules exists
         mock_read_file.side_effect = ['cursorrules content', None, None]
-        mock_skill_class.load.return_value = mock_skill
 
         result = await _load_special_files(
             mock_async_remote_workspace, '/repo', '/workspace'
         )
 
         assert len(result) == 1
-        assert result[0] == mock_skill
+        assert result[0].name == '.cursorrules'
         assert mock_read_file.call_count == 3
-        assert mock_skill_class.load.call_count == 1
 
     @pytest.mark.asyncio
     @patch(
@@ -286,13 +271,10 @@ class TestFindAndLoadSkillMdFiles:
     @patch(
         'openhands.app_server.app_conversation.skill_loader._read_file_from_workspace'
     )
-    @patch('openhands.app_server.app_conversation.skill_loader.Skill')
     async def test_find_and_load_files_success(
         self,
-        mock_skill_class,
         mock_read_file,
         mock_async_remote_workspace,
-        mock_skills_list,
     ):
         """Test successfully finding and loading skill .md files."""
         result_obj = Mock()
@@ -302,27 +284,27 @@ class TestFindAndLoadSkillMdFiles:
         )
         mock_async_remote_workspace.execute_command.return_value = result_obj
 
-        mock_read_file.side_effect = ['content1', 'content2']
-        mock_skill_class.load.side_effect = mock_skills_list[:2]
+        mock_read_file.side_effect = [
+            '---\nname: test1\n---\ncontent1',
+            'content2',
+        ]
 
         result = await _find_and_load_skill_md_files(
             mock_async_remote_workspace, '/repo/.openhands/skills', '/workspace'
         )
 
         assert len(result) == 2
-        assert result == mock_skills_list[:2]
-
-        # Verify relative paths are used
-        assert mock_skill_class.load.call_args_list[0][1]['path'] == 'test1.md'
-        assert mock_skill_class.load.call_args_list[1][1]['path'] == 'test2.md'
+        assert result[0].name == 'test1'
+        assert result[0].trigger is None
+        assert result[1].name == 'test2'
+        assert result[1].trigger is None
 
     @pytest.mark.asyncio
     @patch(
         'openhands.app_server.app_conversation.skill_loader._read_file_from_workspace'
     )
-    @patch('openhands.app_server.app_conversation.skill_loader.Skill')
     async def test_find_and_load_excludes_readme(
-        self, mock_skill_class, mock_read_file, mock_async_remote_workspace, mock_skill
+        self, mock_read_file, mock_async_remote_workspace
     ):
         """Test that README.md files are excluded."""
         result_obj = Mock()
@@ -333,15 +315,13 @@ class TestFindAndLoadSkillMdFiles:
         mock_async_remote_workspace.execute_command.return_value = result_obj
 
         mock_read_file.return_value = 'content'
-        mock_skill_class.load.return_value = mock_skill
 
         result = await _find_and_load_skill_md_files(
             mock_async_remote_workspace, '/repo/.openhands/skills', '/workspace'
         )
 
         assert len(result) == 1
-        assert result[0] == mock_skill
-        # Verify README.md was not processed
+        assert result[0].name == 'test'
         assert mock_read_file.call_count == 1
 
     @pytest.mark.asyncio
@@ -389,20 +369,14 @@ class TestFindAndLoadSkillMdFiles:
 
         mock_read_file.side_effect = ['content1', None]
 
-        with patch(
-            'openhands.app_server.app_conversation.skill_loader.Skill'
-        ) as mock_skill_class:
-            mock_skill = Mock()
-            mock_skill_class.load.return_value = mock_skill
+        result = await _find_and_load_skill_md_files(
+            mock_async_remote_workspace,
+            '/repo/.openhands/skills',
+            '/workspace',
+        )
 
-            result = await _find_and_load_skill_md_files(
-                mock_async_remote_workspace,
-                '/repo/.openhands/skills',
-                '/workspace',
-            )
-
-            assert len(result) == 1
-            assert mock_skill_class.load.call_count == 1
+        assert len(result) == 1
+        assert result[0].name == 'test1'
 
 
 class TestFindAndLoadGlobalSkillFiles: