mirror of
https://github.com/OpenHands/OpenHands.git
synced 2025-12-26 05:48:36 +08:00
fix(backend): organizational skills do not trigger (v1 conversations) (#12037)
This commit is contained in:
Hiep Le
GitHub
parent
eb9a22ef7e
commit
f2def8fd7f
@ -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(
|
||||
|
||||
@ -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.
|
||||
|
||||
|
||||
@ -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
|
||||
)
|
||||
|
||||
@ -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'}
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user