diff --git a/openhands/app_server/app_conversation/app_conversation_service_base.py b/openhands/app_server/app_conversation/app_conversation_service_base.py index d5d34bd109..fb1eb0001e 100644 --- a/openhands/app_server/app_conversation/app_conversation_service_base.py +++ b/openhands/app_server/app_conversation/app_conversation_service_base.py @@ -22,6 +22,7 @@ from openhands.app_server.app_conversation.app_conversation_service import ( ) from openhands.app_server.app_conversation.skill_loader import ( load_global_skills, + load_org_skills, load_repo_skills, load_sandbox_skills, merge_skills, @@ -94,13 +95,20 @@ class AppConversationServiceBase(AppConversationService, ABC): except Exception as e: _logger.warning(f'Failed to load user skills: {str(e)}') user_skills = [] + + # Load organization-level skills + org_skills = await load_org_skills( + remote_workspace, selected_repository, working_dir, self.user_context + ) + repo_skills = await load_repo_skills( remote_workspace, selected_repository, working_dir ) # Merge all skills (later lists override earlier ones) + # Precedence: sandbox < global < user < org < repo all_skills = merge_skills( - [sandbox_skills, global_skills, user_skills, repo_skills] + [sandbox_skills, global_skills, user_skills, org_skills, repo_skills] ) _logger.info( diff --git a/openhands/app_server/app_conversation/skill_loader.py b/openhands/app_server/app_conversation/skill_loader.py index d8fca7cfc3..d237ff0542 100644 --- a/openhands/app_server/app_conversation/skill_loader.py +++ b/openhands/app_server/app_conversation/skill_loader.py @@ -14,6 +14,9 @@ from pathlib import Path 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.workspace.remote.async_remote_workspace import AsyncRemoteWorkspace @@ -119,6 +122,96 @@ def _determine_repo_root(working_dir: str, selected_repository: str | None) -> s return working_dir +async def _is_gitlab_repository(repo_name: str, user_context: UserContext) -> bool: + """Check if a repository is hosted on GitLab. + + Args: + repo_name: Repository name (e.g., "gitlab.com/org/repo" or "org/repo") + user_context: UserContext to access provider handler + + Returns: + True if the repository is hosted on GitLab, False otherwise + """ + try: + provider_handler = await user_context.get_provider_handler() # type: ignore[attr-defined] + repository = await provider_handler.verify_repo_provider(repo_name) + return repository.git_provider == ProviderType.GITLAB + except Exception: + # If we can't determine the provider, assume it's not GitLab + # This is a safe fallback since we'll just use the default .openhands + return False + + +async def _is_azure_devops_repository( + repo_name: str, user_context: UserContext +) -> bool: + """Check if a repository is hosted on Azure DevOps. + + Args: + repo_name: Repository name (e.g., "org/project/repo") + user_context: UserContext to access provider handler + + Returns: + True if the repository is hosted on Azure DevOps, False otherwise + """ + try: + provider_handler = await user_context.get_provider_handler() # type: ignore[attr-defined] + repository = await provider_handler.verify_repo_provider(repo_name) + return repository.git_provider == ProviderType.AZURE_DEVOPS + except Exception: + # If we can't determine the provider, assume it's not Azure DevOps + return False + + +async def _determine_org_repo_path( + selected_repository: str, user_context: UserContext +) -> tuple[str, str]: + """Determine the organization repository path and organization name. + + Args: + selected_repository: Repository name (e.g., 'owner/repo' or 'org/project/repo') + user_context: UserContext to access provider handler + + Returns: + Tuple of (org_repo_path, org_name) where: + - org_repo_path: Full path to org-level config repo + - org_name: Organization name extracted from repository + + Examples: + - GitHub/Bitbucket: ('owner/.openhands', 'owner') + - GitLab: ('owner/openhands-config', 'owner') + - Azure DevOps: ('org/openhands-config/openhands-config', 'org') + """ + repo_parts = selected_repository.split('/') + + # Determine repository type + is_azure_devops = await _is_azure_devops_repository( + selected_repository, user_context + ) + is_gitlab = await _is_gitlab_repository(selected_repository, user_context) + + # Extract the org/user name + # Azure DevOps format: org/project/repo (3 parts) - extract org (first part) + # GitHub/GitLab/Bitbucket format: owner/repo (2 parts) - extract owner (first part) + if is_azure_devops and len(repo_parts) >= 3: + org_name = repo_parts[0] # Get org from org/project/repo + else: + org_name = repo_parts[-2] # Get owner from owner/repo + + # For GitLab and Azure DevOps, use openhands-config (since .openhands is not a valid repo name) + # For other providers, use .openhands + if is_gitlab: + org_openhands_repo = f'{org_name}/openhands-config' + elif is_azure_devops: + # Azure DevOps format: org/project/repo + # For org-level config, use: org/openhands-config/openhands-config + org_openhands_repo = f'{org_name}/openhands-config/openhands-config' + else: + org_openhands_repo = f'{org_name}/.openhands' + + return org_openhands_repo, org_name + + async def _read_file_from_workspace( workspace: AsyncRemoteWorkspace, file_path: str, working_dir: str ) -> str | None: @@ -322,6 +415,248 @@ async def load_repo_skills( return [] +def _validate_repository_for_org_skills(selected_repository: str) -> bool: + """Validate that the repository path has sufficient parts for org skills. + + Args: + selected_repository: Repository name (e.g., 'owner/repo') + + Returns: + True if repository is valid for org skills loading, False otherwise + """ + repo_parts = selected_repository.split('/') + if len(repo_parts) < 2: + _logger.warning( + f'Repository path has insufficient parts ({len(repo_parts)} < 2), skipping org-level skills' + ) + return False + return True + + +async def _get_org_repository_url( + org_openhands_repo: str, user_context: UserContext +) -> str | None: + """Get authenticated Git URL for organization repository. + + Args: + org_openhands_repo: Organization repository path + user_context: UserContext to access authentication + + Returns: + Authenticated Git URL if successful, None otherwise + """ + try: + remote_url = await user_context.get_authenticated_git_url(org_openhands_repo) + return remote_url + except AuthenticationError as e: + _logger.debug( + f'org-level skill directory {org_openhands_repo} not found: {str(e)}' + ) + return None + except Exception as e: + _logger.debug( + f'Failed to get authenticated URL for {org_openhands_repo}: {str(e)}' + ) + return None + + +async def _clone_org_repository( + workspace: AsyncRemoteWorkspace, + remote_url: str, + org_repo_dir: str, + working_dir: str, + org_openhands_repo: str, +) -> bool: + """Clone organization repository to temporary directory. + + Args: + workspace: AsyncRemoteWorkspace to execute commands + remote_url: Authenticated Git URL + org_repo_dir: Temporary directory path for cloning + working_dir: Working directory for command execution + org_openhands_repo: Organization repository path (for logging) + + Returns: + True if clone successful, False otherwise + """ + _logger.debug(f'Creating temporary directory for org repo: {org_repo_dir}') + + # Clone the repo (shallow clone for efficiency) + clone_cmd = f'GIT_TERMINAL_PROMPT=0 git clone --depth 1 {remote_url} {org_repo_dir}' + _logger.info('Executing clone command for org-level repo') + + result = await workspace.execute_command(clone_cmd, working_dir, timeout=120.0) + + if result.exit_code != 0: + _logger.info( + f'No org-level skills found at {org_openhands_repo} (exit_code: {result.exit_code})' + ) + _logger.debug(f'Clone command output: {result.stderr}') + return False + + _logger.info(f'Successfully cloned org-level skills from {org_openhands_repo}') + return True + + +async def _load_skills_from_org_directories( + workspace: AsyncRemoteWorkspace, org_repo_dir: str, working_dir: str +) -> tuple[list[Skill], list[Skill]]: + """Load skills from both skills/ and microagents/ directories in org repo. + + Args: + workspace: AsyncRemoteWorkspace to execute commands + org_repo_dir: Path to cloned organization repository + working_dir: Working directory for command execution + + Returns: + Tuple of (skills_dir_skills, microagents_dir_skills) + """ + skills_dir = f'{org_repo_dir}/skills' + skills_dir_skills = await _find_and_load_skill_md_files( + workspace, skills_dir, working_dir + ) + + microagents_dir = f'{org_repo_dir}/microagents' + microagents_dir_skills = await _find_and_load_skill_md_files( + workspace, microagents_dir, working_dir + ) + + return skills_dir_skills, microagents_dir_skills + + +def _merge_org_skills_with_precedence( + skills_dir_skills: list[Skill], microagents_dir_skills: list[Skill] +) -> list[Skill]: + """Merge skills from skills/ and microagents/ with proper precedence. + + Precedence: skills/ > microagents/ (skills/ overrides microagents/ for same name) + + Args: + skills_dir_skills: Skills loaded from skills/ directory + microagents_dir_skills: Skills loaded from microagents/ directory + + Returns: + Merged list of skills with proper precedence applied + """ + skills_by_name = {} + for skill in microagents_dir_skills + skills_dir_skills: + # Later sources (skills/) override earlier ones (microagents/) + if skill.name not in skills_by_name: + skills_by_name[skill.name] = skill + else: + _logger.debug( + f'Overriding org skill "{skill.name}" from microagents/ with skills/' + ) + skills_by_name[skill.name] = skill + + return list(skills_by_name.values()) + + +async def _cleanup_org_repository( + workspace: AsyncRemoteWorkspace, org_repo_dir: str, working_dir: str +) -> None: + """Clean up cloned organization repository directory. + + Args: + workspace: AsyncRemoteWorkspace to execute commands + org_repo_dir: Path to cloned organization repository + working_dir: Working directory for command execution + """ + cleanup_cmd = f'rm -rf {org_repo_dir}' + await workspace.execute_command(cleanup_cmd, working_dir, timeout=10.0) + + +async def load_org_skills( + workspace: AsyncRemoteWorkspace, + selected_repository: str | None, + working_dir: str, + user_context: UserContext, +) -> list[Skill]: + """Load organization-level skills from the organization repository. + + For example, if the repository is github.com/acme-co/api, this will check if + github.com/acme-co/.openhands exists. If it does, it will clone it and load + the skills from both the ./skills/ and ./microagents/ folders. + + For GitLab repositories, it will use openhands-config instead of .openhands + since GitLab doesn't support repository names starting with non-alphanumeric + characters. + + For Azure DevOps repositories, it will use org/openhands-config/openhands-config + format to match Azure DevOps's three-part repository structure (org/project/repo). + + Args: + workspace: AsyncRemoteWorkspace to execute commands in the sandbox + selected_repository: Repository name (e.g., 'owner/repo') or None + working_dir: Working directory path + user_context: UserContext to access provider handler and authentication + + Returns: + List of Skill objects loaded from organization repository. + Returns empty list if no repository selected or on errors. + """ + if not selected_repository: + return [] + + try: + _logger.debug( + f'Starting org-level skill loading for repository: {selected_repository}' + ) + + # Validate repository path + if not _validate_repository_for_org_skills(selected_repository): + return [] + + # Determine organization repository path + org_openhands_repo, org_name = await _determine_org_repo_path( + selected_repository, user_context + ) + + _logger.info(f'Checking for org-level skills at {org_openhands_repo}') + + # Get authenticated URL for org repository + remote_url = await _get_org_repository_url(org_openhands_repo, user_context) + if not remote_url: + return [] + + # Clone the organization repository + org_repo_dir = f'{working_dir}/_org_openhands_{org_name}' + clone_success = await _clone_org_repository( + workspace, remote_url, org_repo_dir, working_dir, org_openhands_repo + ) + if not clone_success: + return [] + + # Load skills from both skills/ and microagents/ directories + ( + skills_dir_skills, + microagents_dir_skills, + ) = await _load_skills_from_org_directories( + workspace, org_repo_dir, working_dir + ) + + # Merge skills with proper precedence + loaded_skills = _merge_org_skills_with_precedence( + skills_dir_skills, microagents_dir_skills + ) + + _logger.info( + f'Loaded {len(loaded_skills)} skills from org-level repository {org_openhands_repo}: {[s.name for s in loaded_skills]}' + ) + + # Clean up the org repo directory + await _cleanup_org_repository(workspace, org_repo_dir, working_dir) + + return loaded_skills + + except AuthenticationError as e: + _logger.debug(f'org-level skill directory not found: {str(e)}') + return [] + except Exception as e: + _logger.warning(f'Failed to load org-level skills: {str(e)}') + return [] + + def merge_skills(skill_lists: list[list[Skill]]) -> list[Skill]: """Merge multiple skill lists, avoiding duplicates by name. diff --git a/tests/unit/app_server/test_app_conversation_service_base.py b/tests/unit/app_server/test_app_conversation_service_base.py index 356c454fcf..01fc63bc5d 100644 --- a/tests/unit/app_server/test_app_conversation_service_base.py +++ b/tests/unit/app_server/test_app_conversation_service_base.py @@ -15,6 +15,7 @@ from openhands.app_server.app_conversation.app_conversation_models import AgentT from openhands.app_server.app_conversation.app_conversation_service_base import ( AppConversationServiceBase, ) +from openhands.app_server.sandbox.sandbox_models import SandboxInfo from openhands.app_server.user.user_context import UserContext @@ -916,3 +917,350 @@ async def test_configure_git_user_settings_special_characters_in_name(mock_works mock_workspace.execute_command.assert_any_call( 'git config --global user.name "Test O\'Brien"', '/workspace/project' ) + + +# ============================================================================= +# Tests for _load_and_merge_all_skills with org skills +# ============================================================================= + + +class TestLoadAndMergeAllSkillsWithOrgSkills: + """Test _load_and_merge_all_skills includes organization skills.""" + + @pytest.mark.asyncio + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_sandbox_skills' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_global_skills' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_user_skills' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_org_skills' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_repo_skills' + ) + async def test_load_and_merge_includes_org_skills( + self, + mock_load_repo, + mock_load_org, + mock_load_user, + mock_load_global, + mock_load_sandbox, + ): + """Test that _load_and_merge_all_skills loads and merges org skills.""" + # Arrange + mock_user_context = Mock(spec=UserContext) + with patch.object( + AppConversationServiceBase, + '__abstractmethods__', + set(), + ): + service = AppConversationServiceBase( + init_git_in_empty_workspace=True, + user_context=mock_user_context, + ) + + sandbox = Mock(spec=SandboxInfo) + sandbox.exposed_urls = [] + remote_workspace = AsyncMock() + + # Create distinct mock skills for each source + sandbox_skill = Mock() + sandbox_skill.name = 'sandbox_skill' + global_skill = Mock() + global_skill.name = 'global_skill' + user_skill = Mock() + user_skill.name = 'user_skill' + org_skill = Mock() + org_skill.name = 'org_skill' + repo_skill = Mock() + repo_skill.name = 'repo_skill' + + mock_load_sandbox.return_value = [sandbox_skill] + mock_load_global.return_value = [global_skill] + mock_load_user.return_value = [user_skill] + mock_load_org.return_value = [org_skill] + mock_load_repo.return_value = [repo_skill] + + # Act + result = await service._load_and_merge_all_skills( + sandbox, remote_workspace, 'owner/repo', '/workspace' + ) + + # Assert + assert len(result) == 5 + names = {s.name for s in result} + assert names == { + 'sandbox_skill', + 'global_skill', + 'user_skill', + 'org_skill', + 'repo_skill', + } + mock_load_org.assert_called_once_with( + remote_workspace, 'owner/repo', '/workspace', mock_user_context + ) + + @pytest.mark.asyncio + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_sandbox_skills' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_global_skills' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_user_skills' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_org_skills' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_repo_skills' + ) + async def test_load_and_merge_org_skills_precedence( + self, + mock_load_repo, + mock_load_org, + mock_load_user, + mock_load_global, + mock_load_sandbox, + ): + """Test that org skills have correct precedence (higher than user, lower than repo).""" + # Arrange + mock_user_context = Mock(spec=UserContext) + with patch.object( + AppConversationServiceBase, + '__abstractmethods__', + set(), + ): + service = AppConversationServiceBase( + init_git_in_empty_workspace=True, + user_context=mock_user_context, + ) + + sandbox = Mock(spec=SandboxInfo) + sandbox.exposed_urls = [] + remote_workspace = AsyncMock() + + # Create skills with same name but different sources + user_skill = Mock() + user_skill.name = 'common_skill' + user_skill.source = 'user' + + org_skill = Mock() + org_skill.name = 'common_skill' + org_skill.source = 'org' + + repo_skill = Mock() + repo_skill.name = 'common_skill' + repo_skill.source = 'repo' + + mock_load_sandbox.return_value = [] + mock_load_global.return_value = [] + mock_load_user.return_value = [user_skill] + mock_load_org.return_value = [org_skill] + mock_load_repo.return_value = [repo_skill] + + # Act + result = await service._load_and_merge_all_skills( + sandbox, remote_workspace, 'owner/repo', '/workspace' + ) + + # Assert + # Should have only one skill with repo source (highest precedence) + assert len(result) == 1 + assert result[0].source == 'repo' + + @pytest.mark.asyncio + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_sandbox_skills' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_global_skills' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_user_skills' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_org_skills' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_repo_skills' + ) + async def test_load_and_merge_org_skills_override_user_skills( + self, + mock_load_repo, + mock_load_org, + mock_load_user, + mock_load_global, + mock_load_sandbox, + ): + """Test that org skills override user skills for same name.""" + # Arrange + mock_user_context = Mock(spec=UserContext) + with patch.object( + AppConversationServiceBase, + '__abstractmethods__', + set(), + ): + service = AppConversationServiceBase( + init_git_in_empty_workspace=True, + user_context=mock_user_context, + ) + + sandbox = Mock(spec=SandboxInfo) + sandbox.exposed_urls = [] + remote_workspace = AsyncMock() + + # Create skills with same name + user_skill = Mock() + user_skill.name = 'shared_skill' + user_skill.priority = 'low' + + org_skill = Mock() + org_skill.name = 'shared_skill' + org_skill.priority = 'high' + + mock_load_sandbox.return_value = [] + mock_load_global.return_value = [] + mock_load_user.return_value = [user_skill] + mock_load_org.return_value = [org_skill] + mock_load_repo.return_value = [] + + # Act + result = await service._load_and_merge_all_skills( + sandbox, remote_workspace, 'owner/repo', '/workspace' + ) + + # Assert + assert len(result) == 1 + assert result[0].priority == 'high' # Org skill should win + + @pytest.mark.asyncio + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_sandbox_skills' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_global_skills' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_user_skills' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_org_skills' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_repo_skills' + ) + async def test_load_and_merge_handles_org_skills_failure( + self, + mock_load_repo, + mock_load_org, + mock_load_user, + mock_load_global, + mock_load_sandbox, + ): + """Test that failure to load org skills doesn't break the overall process.""" + # Arrange + mock_user_context = Mock(spec=UserContext) + with patch.object( + AppConversationServiceBase, + '__abstractmethods__', + set(), + ): + service = AppConversationServiceBase( + init_git_in_empty_workspace=True, + user_context=mock_user_context, + ) + + sandbox = Mock(spec=SandboxInfo) + sandbox.exposed_urls = [] + remote_workspace = AsyncMock() + + global_skill = Mock() + global_skill.name = 'global_skill' + repo_skill = Mock() + repo_skill.name = 'repo_skill' + + mock_load_sandbox.return_value = [] + mock_load_global.return_value = [global_skill] + mock_load_user.return_value = [] + mock_load_org.return_value = [] # Org skills failed/empty + mock_load_repo.return_value = [repo_skill] + + # Act + result = await service._load_and_merge_all_skills( + sandbox, remote_workspace, 'owner/repo', '/workspace' + ) + + # Assert + # Should still have skills from other sources + assert len(result) == 2 + names = {s.name for s in result} + assert names == {'global_skill', 'repo_skill'} + + @pytest.mark.asyncio + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_sandbox_skills' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_global_skills' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_user_skills' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_org_skills' + ) + @patch( + 'openhands.app_server.app_conversation.app_conversation_service_base.load_repo_skills' + ) + async def test_load_and_merge_no_selected_repository( + self, + mock_load_repo, + mock_load_org, + mock_load_user, + mock_load_global, + mock_load_sandbox, + ): + """Test skill loading when no repository is selected.""" + # Arrange + mock_user_context = Mock(spec=UserContext) + with patch.object( + AppConversationServiceBase, + '__abstractmethods__', + set(), + ): + service = AppConversationServiceBase( + init_git_in_empty_workspace=True, + user_context=mock_user_context, + ) + + sandbox = Mock(spec=SandboxInfo) + sandbox.exposed_urls = [] + remote_workspace = AsyncMock() + + global_skill = Mock() + global_skill.name = 'global_skill' + + mock_load_sandbox.return_value = [] + mock_load_global.return_value = [global_skill] + mock_load_user.return_value = [] + mock_load_org.return_value = [] + mock_load_repo.return_value = [] + + # Act + result = await service._load_and_merge_all_skills( + sandbox, remote_workspace, None, '/workspace' + ) + + # Assert + assert len(result) == 1 + # Org skills should be called even with None repository + mock_load_org.assert_called_once_with( + remote_workspace, None, '/workspace', mock_user_context + ) diff --git a/tests/unit/app_server/test_skill_loader.py b/tests/unit/app_server/test_skill_loader.py index c9e54ba5a1..e4daadfa14 100644 --- a/tests/unit/app_server/test_skill_loader.py +++ b/tests/unit/app_server/test_skill_loader.py @@ -11,15 +11,27 @@ from unittest.mock import AsyncMock, MagicMock, Mock, patch import pytest from openhands.app_server.app_conversation.skill_loader import ( + _cleanup_org_repository, + _clone_org_repository, + _determine_org_repo_path, _determine_repo_root, _find_and_load_global_skill_files, _find_and_load_skill_md_files, + _get_org_repository_url, + _is_azure_devops_repository, + _is_gitlab_repository, + _load_skills_from_org_directories, _load_special_files, + _merge_org_skills_with_precedence, _read_file_from_workspace, + _validate_repository_for_org_skills, load_global_skills, + load_org_skills, load_repo_skills, merge_skills, ) +from openhands.integrations.provider import ProviderType +from openhands.integrations.service_types import AuthenticationError # ===== Test Fixtures ===== @@ -667,6 +679,669 @@ class TestMergeSkills: assert len(result) == 2 +# ===== Tests for Organization Skills Functions ===== + + +class TestIsGitlabRepository: + """Test _is_gitlab_repository helper function.""" + + @pytest.mark.asyncio + async def test_is_gitlab_repository_true(self): + """Test GitLab repository detection returns True.""" + # Arrange + mock_user_context = AsyncMock() + mock_provider_handler = AsyncMock() + mock_repository = Mock() + mock_repository.git_provider = ProviderType.GITLAB + + mock_user_context.get_provider_handler.return_value = mock_provider_handler + mock_provider_handler.verify_repo_provider.return_value = mock_repository + + # Act + result = await _is_gitlab_repository('owner/repo', mock_user_context) + + # Assert + assert result is True + mock_provider_handler.verify_repo_provider.assert_called_once_with('owner/repo') + + @pytest.mark.asyncio + async def test_is_gitlab_repository_false(self): + """Test non-GitLab repository detection returns False.""" + # Arrange + mock_user_context = AsyncMock() + mock_provider_handler = AsyncMock() + mock_repository = Mock() + mock_repository.git_provider = ProviderType.GITHUB + + mock_user_context.get_provider_handler.return_value = mock_provider_handler + mock_provider_handler.verify_repo_provider.return_value = mock_repository + + # Act + result = await _is_gitlab_repository('owner/repo', mock_user_context) + + # Assert + assert result is False + + @pytest.mark.asyncio + async def test_is_gitlab_repository_exception_handling(self): + """Test exception handling returns False.""" + # Arrange + mock_user_context = AsyncMock() + mock_user_context.get_provider_handler.side_effect = Exception('API error') + + # Act + result = await _is_gitlab_repository('owner/repo', mock_user_context) + + # Assert + assert result is False + + +class TestIsAzureDevOpsRepository: + """Test _is_azure_devops_repository helper function.""" + + @pytest.mark.asyncio + async def test_is_azure_devops_repository_true(self): + """Test Azure DevOps repository detection returns True.""" + # Arrange + mock_user_context = AsyncMock() + mock_provider_handler = AsyncMock() + mock_repository = Mock() + mock_repository.git_provider = ProviderType.AZURE_DEVOPS + + mock_user_context.get_provider_handler.return_value = mock_provider_handler + mock_provider_handler.verify_repo_provider.return_value = mock_repository + + # Act + result = await _is_azure_devops_repository( + 'org/project/repo', mock_user_context + ) + + # Assert + assert result is True + mock_provider_handler.verify_repo_provider.assert_called_once_with( + 'org/project/repo' + ) + + @pytest.mark.asyncio + async def test_is_azure_devops_repository_false(self): + """Test non-Azure DevOps repository detection returns False.""" + # Arrange + mock_user_context = AsyncMock() + mock_provider_handler = AsyncMock() + mock_repository = Mock() + mock_repository.git_provider = ProviderType.GITHUB + + mock_user_context.get_provider_handler.return_value = mock_provider_handler + mock_provider_handler.verify_repo_provider.return_value = mock_repository + + # Act + result = await _is_azure_devops_repository('owner/repo', mock_user_context) + + # Assert + assert result is False + + @pytest.mark.asyncio + async def test_is_azure_devops_repository_exception_handling(self): + """Test exception handling returns False.""" + # Arrange + mock_user_context = AsyncMock() + mock_user_context.get_provider_handler.side_effect = Exception('Network error') + + # Act + result = await _is_azure_devops_repository('owner/repo', mock_user_context) + + # Assert + assert result is False + + +class TestDetermineOrgRepoPath: + """Test _determine_org_repo_path helper function.""" + + @pytest.mark.asyncio + @patch('openhands.app_server.app_conversation.skill_loader._is_gitlab_repository') + @patch( + 'openhands.app_server.app_conversation.skill_loader._is_azure_devops_repository' + ) + async def test_github_repository_path(self, mock_is_azure, mock_is_gitlab): + """Test org path for GitHub repository.""" + # Arrange + mock_user_context = AsyncMock() + mock_is_gitlab.return_value = False + mock_is_azure.return_value = False + + # Act + org_repo, org_name = await _determine_org_repo_path( + 'owner/repo', mock_user_context + ) + + # Assert + assert org_repo == 'owner/.openhands' + assert org_name == 'owner' + + @pytest.mark.asyncio + @patch('openhands.app_server.app_conversation.skill_loader._is_gitlab_repository') + @patch( + 'openhands.app_server.app_conversation.skill_loader._is_azure_devops_repository' + ) + async def test_gitlab_repository_path(self, mock_is_azure, mock_is_gitlab): + """Test org path for GitLab repository.""" + # Arrange + mock_user_context = AsyncMock() + mock_is_gitlab.return_value = True + mock_is_azure.return_value = False + + # Act + org_repo, org_name = await _determine_org_repo_path( + 'owner/repo', mock_user_context + ) + + # Assert + assert org_repo == 'owner/openhands-config' + assert org_name == 'owner' + + @pytest.mark.asyncio + @patch('openhands.app_server.app_conversation.skill_loader._is_gitlab_repository') + @patch( + 'openhands.app_server.app_conversation.skill_loader._is_azure_devops_repository' + ) + async def test_azure_devops_repository_path(self, mock_is_azure, mock_is_gitlab): + """Test org path for Azure DevOps repository.""" + # Arrange + mock_user_context = AsyncMock() + mock_is_gitlab.return_value = False + mock_is_azure.return_value = True + + # Act + org_repo, org_name = await _determine_org_repo_path( + 'org/project/repo', mock_user_context + ) + + # Assert + assert org_repo == 'org/openhands-config/openhands-config' + assert org_name == 'org' + + +class TestValidateRepositoryForOrgSkills: + """Test _validate_repository_for_org_skills helper function.""" + + def test_valid_repository_two_parts(self): + """Test validation passes for repository with two parts.""" + # Act + result = _validate_repository_for_org_skills('owner/repo') + + # Assert + assert result is True + + def test_valid_repository_three_parts(self): + """Test validation passes for repository with three parts (Azure DevOps).""" + # Act + result = _validate_repository_for_org_skills('org/project/repo') + + # Assert + assert result is True + + def test_invalid_repository_one_part(self): + """Test validation fails for repository with only one part.""" + # Act + result = _validate_repository_for_org_skills('repo') + + # Assert + assert result is False + + def test_invalid_repository_empty_string(self): + """Test validation fails for empty string.""" + # Act + result = _validate_repository_for_org_skills('') + + # Assert + assert result is False + + +class TestGetOrgRepositoryUrl: + """Test _get_org_repository_url helper function.""" + + @pytest.mark.asyncio + async def test_successful_url_retrieval(self): + """Test successfully retrieving authenticated URL.""" + # Arrange + mock_user_context = AsyncMock() + expected_url = 'https://token@github.com/owner/.openhands.git' + mock_user_context.get_authenticated_git_url.return_value = expected_url + + # Act + result = await _get_org_repository_url('owner/.openhands', mock_user_context) + + # Assert + assert result == expected_url + mock_user_context.get_authenticated_git_url.assert_called_once_with( + 'owner/.openhands' + ) + + @pytest.mark.asyncio + async def test_authentication_error(self): + """Test handling of authentication error returns None.""" + # Arrange + mock_user_context = AsyncMock() + mock_user_context.get_authenticated_git_url.side_effect = AuthenticationError( + 'Not found' + ) + + # Act + result = await _get_org_repository_url('owner/.openhands', mock_user_context) + + # Assert + assert result is None + + @pytest.mark.asyncio + async def test_general_exception(self): + """Test handling of general exception returns None.""" + # Arrange + mock_user_context = AsyncMock() + mock_user_context.get_authenticated_git_url.side_effect = Exception( + 'Network error' + ) + + # Act + result = await _get_org_repository_url('owner/.openhands', mock_user_context) + + # Assert + assert result is None + + +class TestCloneOrgRepository: + """Test _clone_org_repository helper function.""" + + @pytest.mark.asyncio + async def test_successful_clone(self, mock_async_remote_workspace): + """Test successful repository clone.""" + # Arrange + result_obj = Mock() + result_obj.exit_code = 0 + mock_async_remote_workspace.execute_command.return_value = result_obj + + # Act + success = await _clone_org_repository( + mock_async_remote_workspace, + 'https://github.com/owner/.openhands.git', + '/workspace/_org_openhands_owner', + '/workspace', + 'owner/.openhands', + ) + + # Assert + assert success is True + mock_async_remote_workspace.execute_command.assert_called_once() + call_args = mock_async_remote_workspace.execute_command.call_args + assert 'git clone' in call_args[0][0] + assert '--depth 1' in call_args[0][0] + + @pytest.mark.asyncio + async def test_failed_clone(self, mock_async_remote_workspace): + """Test failed repository clone.""" + # Arrange + result_obj = Mock() + result_obj.exit_code = 1 + result_obj.stderr = 'Repository not found' + mock_async_remote_workspace.execute_command.return_value = result_obj + + # Act + success = await _clone_org_repository( + mock_async_remote_workspace, + 'https://github.com/owner/.openhands.git', + '/workspace/_org_openhands_owner', + '/workspace', + 'owner/.openhands', + ) + + # Assert + assert success is False + + +class TestLoadSkillsFromOrgDirectories: + """Test _load_skills_from_org_directories helper function.""" + + @pytest.mark.asyncio + @patch( + 'openhands.app_server.app_conversation.skill_loader._find_and_load_skill_md_files' + ) + async def test_load_from_both_directories( + self, mock_find_and_load, mock_async_remote_workspace, mock_skills_list + ): + """Test loading skills from both skills/ and microagents/ directories.""" + # Arrange + skills_dir_skills = [mock_skills_list[0]] + microagents_dir_skills = [mock_skills_list[1], mock_skills_list[2]] + mock_find_and_load.side_effect = [skills_dir_skills, microagents_dir_skills] + + # Act + result_skills, result_microagents = await _load_skills_from_org_directories( + mock_async_remote_workspace, '/workspace/_org_openhands_owner', '/workspace' + ) + + # Assert + assert result_skills == skills_dir_skills + assert result_microagents == microagents_dir_skills + assert mock_find_and_load.call_count == 2 + + # Verify correct directories were checked + first_call = mock_find_and_load.call_args_list[0] + second_call = mock_find_and_load.call_args_list[1] + assert '/skills' in first_call[0][1] + assert '/microagents' in second_call[0][1] + + @pytest.mark.asyncio + @patch( + 'openhands.app_server.app_conversation.skill_loader._find_and_load_skill_md_files' + ) + async def test_load_with_empty_directories( + self, mock_find_and_load, mock_async_remote_workspace + ): + """Test loading when both directories are empty.""" + # Arrange + mock_find_and_load.side_effect = [[], []] + + # Act + result_skills, result_microagents = await _load_skills_from_org_directories( + mock_async_remote_workspace, '/workspace/_org_openhands_owner', '/workspace' + ) + + # Assert + assert result_skills == [] + assert result_microagents == [] + + +class TestMergeOrgSkillsWithPrecedence: + """Test _merge_org_skills_with_precedence helper function.""" + + def test_merge_no_duplicates(self, mock_skills_list): + """Test merging skills with no name conflicts.""" + # Arrange + skills_dir_skills = [mock_skills_list[0]] + microagents_dir_skills = [mock_skills_list[1], mock_skills_list[2]] + + # Act + result = _merge_org_skills_with_precedence( + skills_dir_skills, microagents_dir_skills + ) + + # Assert + assert len(result) == 3 + names = {s.name for s in result} + assert names == {'skill_0', 'skill_1', 'skill_2'} + + def test_merge_with_duplicate_skills_dir_wins(self): + """Test skills/ directory takes precedence over microagents/.""" + # Arrange + skill_from_microagents = Mock() + skill_from_microagents.name = 'common_skill' + skill_from_microagents.source = 'microagents' + + skill_from_skills = Mock() + skill_from_skills.name = 'common_skill' + skill_from_skills.source = 'skills' + + # Act + result = _merge_org_skills_with_precedence( + [skill_from_skills], [skill_from_microagents] + ) + + # Assert + assert len(result) == 1 + assert result[0].source == 'skills' + + def test_merge_with_empty_lists(self): + """Test merging with empty skill lists.""" + # Act + result = _merge_org_skills_with_precedence([], []) + + # Assert + assert result == [] + + def test_merge_with_only_skills_dir(self, mock_skills_list): + """Test merging with only skills/ directory populated.""" + # Act + result = _merge_org_skills_with_precedence([mock_skills_list[0]], []) + + # Assert + assert len(result) == 1 + assert result[0] == mock_skills_list[0] + + def test_merge_with_only_microagents_dir(self, mock_skills_list): + """Test merging with only microagents/ directory populated.""" + # Act + result = _merge_org_skills_with_precedence([], [mock_skills_list[0]]) + + # Assert + assert len(result) == 1 + assert result[0] == mock_skills_list[0] + + +class TestCleanupOrgRepository: + """Test _cleanup_org_repository helper function.""" + + @pytest.mark.asyncio + async def test_cleanup_successful(self, mock_async_remote_workspace): + """Test successful cleanup of org repository directory.""" + # Arrange + result_obj = Mock() + result_obj.exit_code = 0 + mock_async_remote_workspace.execute_command.return_value = result_obj + + # Act + await _cleanup_org_repository( + mock_async_remote_workspace, + '/workspace/_org_openhands_owner', + '/workspace', + ) + + # Assert + mock_async_remote_workspace.execute_command.assert_called_once() + call_args = mock_async_remote_workspace.execute_command.call_args + assert 'rm -rf' in call_args[0][0] + assert '/workspace/_org_openhands_owner' in call_args[0][0] + + +class TestLoadOrgSkills: + """Test load_org_skills main function.""" + + @pytest.mark.asyncio + async def test_load_org_skills_no_selected_repository( + self, mock_async_remote_workspace + ): + """Test load_org_skills returns empty list when no repository selected.""" + # Arrange + mock_user_context = AsyncMock() + + # Act + result = await load_org_skills( + mock_async_remote_workspace, None, '/workspace', mock_user_context + ) + + # Assert + assert result == [] + + @pytest.mark.asyncio + @patch( + 'openhands.app_server.app_conversation.skill_loader._validate_repository_for_org_skills' + ) + async def test_load_org_skills_invalid_repository( + self, mock_validate, mock_async_remote_workspace + ): + """Test load_org_skills returns empty list for invalid repository.""" + # Arrange + mock_validate.return_value = False + mock_user_context = AsyncMock() + + # Act + result = await load_org_skills( + mock_async_remote_workspace, 'invalid', '/workspace', mock_user_context + ) + + # Assert + assert result == [] + mock_validate.assert_called_once_with('invalid') + + @pytest.mark.asyncio + @patch( + 'openhands.app_server.app_conversation.skill_loader._validate_repository_for_org_skills' + ) + @patch( + 'openhands.app_server.app_conversation.skill_loader._determine_org_repo_path' + ) + @patch('openhands.app_server.app_conversation.skill_loader._get_org_repository_url') + async def test_load_org_skills_no_url_available( + self, + mock_get_url, + mock_determine_path, + mock_validate, + mock_async_remote_workspace, + ): + """Test load_org_skills returns empty list when URL cannot be retrieved.""" + # Arrange + mock_validate.return_value = True + mock_determine_path.return_value = ('owner/.openhands', 'owner') + mock_get_url.return_value = None + mock_user_context = AsyncMock() + + # Act + result = await load_org_skills( + mock_async_remote_workspace, + 'owner/repo', + '/workspace', + mock_user_context, + ) + + # Assert + assert result == [] + + @pytest.mark.asyncio + @patch( + 'openhands.app_server.app_conversation.skill_loader._validate_repository_for_org_skills' + ) + @patch( + 'openhands.app_server.app_conversation.skill_loader._determine_org_repo_path' + ) + @patch('openhands.app_server.app_conversation.skill_loader._get_org_repository_url') + @patch('openhands.app_server.app_conversation.skill_loader._clone_org_repository') + async def test_load_org_skills_clone_fails( + self, + mock_clone, + mock_get_url, + mock_determine_path, + mock_validate, + mock_async_remote_workspace, + ): + """Test load_org_skills returns empty list when clone fails.""" + # Arrange + mock_validate.return_value = True + mock_determine_path.return_value = ('owner/.openhands', 'owner') + mock_get_url.return_value = 'https://github.com/owner/.openhands.git' + mock_clone.return_value = False + mock_user_context = AsyncMock() + + # Act + result = await load_org_skills( + mock_async_remote_workspace, + 'owner/repo', + '/workspace', + mock_user_context, + ) + + # Assert + assert result == [] + + @pytest.mark.asyncio + @patch( + 'openhands.app_server.app_conversation.skill_loader._validate_repository_for_org_skills' + ) + @patch( + 'openhands.app_server.app_conversation.skill_loader._determine_org_repo_path' + ) + @patch('openhands.app_server.app_conversation.skill_loader._get_org_repository_url') + @patch('openhands.app_server.app_conversation.skill_loader._clone_org_repository') + @patch( + 'openhands.app_server.app_conversation.skill_loader._load_skills_from_org_directories' + ) + @patch('openhands.app_server.app_conversation.skill_loader._cleanup_org_repository') + async def test_load_org_skills_success( + self, + mock_cleanup, + mock_load_skills, + mock_clone, + mock_get_url, + mock_determine_path, + mock_validate, + mock_async_remote_workspace, + mock_skills_list, + ): + """Test successful org skills loading.""" + # Arrange + mock_validate.return_value = True + mock_determine_path.return_value = ('owner/.openhands', 'owner') + mock_get_url.return_value = 'https://github.com/owner/.openhands.git' + mock_clone.return_value = True + mock_load_skills.return_value = ([mock_skills_list[0]], [mock_skills_list[1]]) + mock_user_context = AsyncMock() + + # Act + result = await load_org_skills( + mock_async_remote_workspace, + 'owner/repo', + '/workspace', + mock_user_context, + ) + + # Assert + assert len(result) == 2 + mock_cleanup.assert_called_once() + + @pytest.mark.asyncio + @patch( + 'openhands.app_server.app_conversation.skill_loader._validate_repository_for_org_skills' + ) + async def test_load_org_skills_handles_authentication_error( + self, mock_validate, mock_async_remote_workspace + ): + """Test load_org_skills handles AuthenticationError gracefully.""" + # Arrange + mock_validate.side_effect = AuthenticationError('Auth failed') + mock_user_context = AsyncMock() + + # Act + result = await load_org_skills( + mock_async_remote_workspace, + 'owner/repo', + '/workspace', + mock_user_context, + ) + + # Assert + assert result == [] + + @pytest.mark.asyncio + @patch( + 'openhands.app_server.app_conversation.skill_loader._validate_repository_for_org_skills' + ) + async def test_load_org_skills_handles_general_exception( + self, mock_validate, mock_async_remote_workspace + ): + """Test load_org_skills handles general exceptions gracefully.""" + # Arrange + mock_validate.side_effect = Exception('Unexpected error') + mock_user_context = AsyncMock() + + # Act + result = await load_org_skills( + mock_async_remote_workspace, + 'owner/repo', + '/workspace', + mock_user_context, + ) + + # Assert + assert result == [] + + # ===== Integration Tests ===== @@ -754,3 +1429,110 @@ class TestSkillLoaderIntegration: # Should have only one skill with repo source (highest precedence) assert len(all_skills) == 1 assert all_skills[0].source == 'repo' + + @pytest.mark.asyncio + @patch('openhands.app_server.app_conversation.skill_loader.load_global_skills') + @patch('openhands.sdk.context.skills.load_user_skills') + @patch('openhands.app_server.app_conversation.skill_loader.load_org_skills') + @patch('openhands.app_server.app_conversation.skill_loader.load_repo_skills') + async def test_loading_with_org_skills_precedence( + self, + mock_load_repo, + mock_load_org, + mock_load_user, + mock_load_global, + mock_async_remote_workspace, + ): + """Test that org skills fit correctly in precedence order.""" + # Arrange + # Create skills with same name but different sources + global_skill = Mock() + global_skill.name = 'shared_skill' + global_skill.priority = 'low' + + user_skill = Mock() + user_skill.name = 'shared_skill' + user_skill.priority = 'medium' + + org_skill = Mock() + org_skill.name = 'shared_skill' + org_skill.priority = 'high' + + repo_skill = Mock() + repo_skill.name = 'shared_skill' + repo_skill.priority = 'highest' + + mock_load_global.return_value = [global_skill] + mock_load_user.return_value = [user_skill] + mock_load_org.return_value = [org_skill] + mock_load_repo.return_value = [repo_skill] + + mock_user_context = AsyncMock() + + # Act + global_skills = mock_load_global() + user_skills = mock_load_user() + org_skills = await mock_load_org( + mock_async_remote_workspace, 'owner/repo', '/workspace', mock_user_context + ) + repo_skills = await mock_load_repo( + mock_async_remote_workspace, 'owner/repo', '/workspace' + ) + + # Merge with correct precedence: global < user < org < repo + all_skills = merge_skills([global_skills, user_skills, org_skills, repo_skills]) + + # Assert + assert len(all_skills) == 1 + assert all_skills[0].priority == 'highest' # Repo has highest precedence + + @pytest.mark.asyncio + @patch('openhands.app_server.app_conversation.skill_loader.load_global_skills') + @patch('openhands.sdk.context.skills.load_user_skills') + @patch('openhands.app_server.app_conversation.skill_loader.load_org_skills') + @patch('openhands.app_server.app_conversation.skill_loader.load_repo_skills') + async def test_loading_org_skills_with_unique_names( + self, + mock_load_repo, + mock_load_org, + mock_load_user, + mock_load_global, + mock_async_remote_workspace, + ): + """Test loading org skills with unique names alongside other sources.""" + # Arrange + global_skill = Mock() + global_skill.name = 'global_skill' + + user_skill = Mock() + user_skill.name = 'user_skill' + + org_skill = Mock() + org_skill.name = 'org_skill' + + repo_skill = Mock() + repo_skill.name = 'repo_skill' + + mock_load_global.return_value = [global_skill] + mock_load_user.return_value = [user_skill] + mock_load_org.return_value = [org_skill] + mock_load_repo.return_value = [repo_skill] + + mock_user_context = AsyncMock() + + # Act + global_skills = mock_load_global() + user_skills = mock_load_user() + org_skills = await mock_load_org( + mock_async_remote_workspace, 'owner/repo', '/workspace', mock_user_context + ) + repo_skills = await mock_load_repo( + mock_async_remote_workspace, 'owner/repo', '/workspace' + ) + + all_skills = merge_skills([global_skills, user_skills, org_skills, repo_skills]) + + # Assert + assert len(all_skills) == 4 + names = {s.name for s in all_skills} + assert names == {'global_skill', 'user_skill', 'org_skill', 'repo_skill'}