diff --git a/openhands-sdk/openhands/sdk/context/skills/skill.py b/openhands-sdk/openhands/sdk/context/skills/skill.py index 8fabc39f11..b679ba127e 100644 --- a/openhands-sdk/openhands/sdk/context/skills/skill.py +++ b/openhands-sdk/openhands/sdk/context/skills/skill.py @@ -8,7 +8,7 @@ from fastmcp.mcp_config import MCPConfig from pydantic import BaseModel, Field, field_validator, model_validator -from openhands.sdk.context.skills.exceptions import SkillValidationError +from openhands.sdk.context.skills.exceptions import SkillError, SkillValidationError from openhands.sdk.context.skills.trigger import ( KeywordTrigger, TaskTrigger, @@ -562,21 +562,14 @@ def load_skills_from_dir( agent_skills: dict[str, Skill] = {} logger.debug(f"Loading agents from {skill_dir}") - # Discover all skill files - repo_root = skill_dir.parent.parent - third_party_files = find_third_party_files( - repo_root, Skill.PATH_TO_THIRD_PARTY_SKILL_NAME - ) + # Discover skill files in the skills directory + # Note: Third-party files (AGENTS.md, etc.) are loaded separately by + # load_project_skills() to ensure they're loaded even when this directory + # doesn't exist. skill_md_files = find_skill_md_directories(skill_dir) skill_md_dirs = {skill_md.parent for skill_md in skill_md_files} regular_md_files = find_regular_md_files(skill_dir, skill_md_dirs) - # Load third-party files - for path in third_party_files: - load_and_categorize( - path, skill_dir, repo_skills, knowledge_skills, agent_skills - ) - # Load SKILL.md files (auto-detected and validated in Skill.load) for skill_md_path in skill_md_files: load_and_categorize( @@ -659,6 +652,9 @@ def load_project_skills(work_dir: str | Path) -> list[Skill]: directories are merged, with skills/ taking precedence for duplicate names. + Also loads third-party skill files (AGENTS.md, .cursorrules, etc.) + directly from the work directory. + Args: work_dir: Path to the project/working directory. @@ -670,7 +666,22 @@ def load_project_skills(work_dir: str | Path) -> list[Skill]: work_dir = Path(work_dir) all_skills = [] - seen_names = set() + seen_names: set[str] = set() + + # First, load third-party skill files directly from work directory + # This ensures they are loaded even if .openhands/skills doesn't exist + third_party_files = find_third_party_files( + work_dir, Skill.PATH_TO_THIRD_PARTY_SKILL_NAME + ) + for path in third_party_files: + try: + skill = Skill.load(path) + if skill.name not in seen_names: + all_skills.append(skill) + seen_names.add(skill.name) + logger.debug(f"Loaded third-party skill: {skill.name} from {path}") + except (SkillError, OSError) as e: + logger.warning(f"Failed to load third-party skill from {path}: {e}") # Load project-specific skills from .openhands/skills and legacy microagents project_skills_dirs = [ @@ -691,7 +702,7 @@ def load_project_skills(work_dir: str | Path) -> list[Skill]: project_skills_dir ) - # Merge all skill categories + # Merge all skill categories (skip duplicates including third-party) for skills_dict in [repo_skills, knowledge_skills, agent_skills]: for name, skill in skills_dict.items(): if name not in seen_names: diff --git a/tests/sdk/context/skill/test_load_project_skills.py b/tests/sdk/context/skill/test_load_project_skills.py index c0df42d611..f9e97c1ff3 100644 --- a/tests/sdk/context/skill/test_load_project_skills.py +++ b/tests/sdk/context/skill/test_load_project_skills.py @@ -12,6 +12,68 @@ def test_load_project_skills_no_directories(tmp_path): assert skills == [] +def test_load_project_skills_agents_md_without_skills_directory(tmp_path): + """Test that AGENTS.md is loaded even when .openhands/skills doesn't exist. + + This is a regression test for the bug where third-party skill files like + AGENTS.md were not loaded when the .openhands/skills directory didn't exist. + """ + # Create AGENTS.md in the work directory (no .openhands/skills) + agents_md = tmp_path / "AGENTS.md" + agents_md.write_text("# Project Guidelines\n\nThis is the AGENTS.md content.") + + skills = load_project_skills(tmp_path) + assert len(skills) == 1 + assert skills[0].name == "agents" + assert "Project Guidelines" in skills[0].content + assert skills[0].trigger is None # Third-party skills are always active + + +def test_load_project_skills_agents_md_case_insensitive(tmp_path): + """Test that AGENTS.md is loaded with case-insensitive matching.""" + # Create agents.md (lowercase) in the work directory + agents_md = tmp_path / "agents.md" + agents_md.write_text("# Lowercase agents.md content") + + skills = load_project_skills(tmp_path) + assert len(skills) == 1 + assert skills[0].name == "agents" + + +def test_load_project_skills_multiple_third_party_files(tmp_path): + """Test loading multiple third-party skill files.""" + # Create AGENTS.md + (tmp_path / "AGENTS.md").write_text("# AGENTS.md content") + + # Create .cursorrules + (tmp_path / ".cursorrules").write_text("# Cursor rules content") + + skills = load_project_skills(tmp_path) + assert len(skills) == 2 + skill_names = {s.name for s in skills} + assert "agents" in skill_names + assert "cursorrules" in skill_names + + +def test_load_project_skills_third_party_with_skills_directory(tmp_path): + """Test third-party files are loaded alongside skills from .openhands/skills.""" + # Create AGENTS.md in work directory + (tmp_path / "AGENTS.md").write_text("# AGENTS.md content") + + # Create .openhands/skills directory with a skill + skills_dir = tmp_path / ".openhands" / "skills" + skills_dir.mkdir(parents=True) + (skills_dir / "test_skill.md").write_text( + "---\nname: test_skill\ntriggers:\n - test\n---\nTest skill content." + ) + + skills = load_project_skills(tmp_path) + assert len(skills) == 2 + skill_names = {s.name for s in skills} + assert "agents" in skill_names + assert "test_skill" in skill_names + + def test_load_project_skills_with_skills_directory(tmp_path): """Test load_project_skills loads from .openhands/skills directory.""" # Create .openhands/skills directory diff --git a/tests/sdk/context/skill/test_skill_utils.py b/tests/sdk/context/skill/test_skill_utils.py index fd887006d9..abea9f8d51 100644 --- a/tests/sdk/context/skill/test_skill_utils.py +++ b/tests/sdk/context/skill/test_skill_utils.py @@ -9,6 +9,7 @@ KeywordTrigger, Skill, SkillValidationError, + load_project_skills, load_skills_from_dir, ) @@ -337,17 +338,17 @@ def temp_skills_dir_with_cursorrules(): def test_load_skills_with_cursorrules(temp_skills_dir_with_cursorrules): """Test loading skills when .cursorrules file exists.""" - skills_dir = temp_skills_dir_with_cursorrules / ".openhands" / "skills" - - repo_agents, knowledge_agents, _ = load_skills_from_dir(skills_dir) + # Third-party files are loaded by load_project_skills(), not load_skills_from_dir() + skills = load_project_skills(temp_skills_dir_with_cursorrules) + skills_by_name = {s.name: s for s in skills} # Verify that .cursorrules file was loaded as a RepoSkill - assert len(repo_agents) == 2 # repo.md + .cursorrules - assert "repo" in repo_agents - assert "cursorrules" in repo_agents + assert len(skills_by_name) == 2 # repo.md + .cursorrules + assert "repo" in skills_by_name + assert "cursorrules" in skills_by_name # Check .cursorrules agent - cursorrules_agent = repo_agents["cursorrules"] + cursorrules_agent = skills_by_name["cursorrules"] assert cursorrules_agent.trigger is None assert cursorrules_agent.name == "cursorrules" assert "Always use TypeScript for new files" in cursorrules_agent.content @@ -394,25 +395,25 @@ def temp_skills_dir_with_context_files(): def test_load_skills_with_claude_gemini(temp_skills_dir_with_context_files): """Test loading skills when claude.md and gemini.md files exist.""" - skills_dir = temp_skills_dir_with_context_files / ".openhands" / "skills" - - repo_agents, knowledge_agents, _ = load_skills_from_dir(skills_dir) + # Third-party files are loaded by load_project_skills(), not load_skills_from_dir() + skills = load_project_skills(temp_skills_dir_with_context_files) + skills_by_name = {s.name: s for s in skills} # Verify that claude.md and gemini.md files were loaded as RepoSkills - assert len(repo_agents) == 3 # repo.md + claude.md + gemini.md - assert "repo" in repo_agents - assert "claude" in repo_agents - assert "gemini" in repo_agents + assert len(skills_by_name) == 3 # repo.md + claude.md + gemini.md + assert "repo" in skills_by_name + assert "claude" in skills_by_name + assert "gemini" in skills_by_name # Check CLAUDE.md agent - claude_agent = repo_agents["claude"] + claude_agent = skills_by_name["claude"] assert claude_agent.trigger is None assert claude_agent.name == "claude" assert "Claude-Specific Instructions" in claude_agent.content assert claude_agent.trigger is None # Check GEMINI.md agent - gemini_agent = repo_agents["gemini"] + gemini_agent = skills_by_name["gemini"] assert gemini_agent.trigger is None assert gemini_agent.name == "gemini" assert "Gemini-Specific Instructions" in gemini_agent.content @@ -461,24 +462,24 @@ def test_load_skills_with_uppercase_claude_gemini( temp_skills_dir_with_uppercase_context_files, ): """Test loading skills when CLAUDE.MD and GEMINI.MD files exist (uppercase).""" - skills_dir = temp_skills_dir_with_uppercase_context_files / ".openhands" / "skills" - - repo_agents, knowledge_agents, _ = load_skills_from_dir(skills_dir) + # Third-party files are loaded by load_project_skills(), not load_skills_from_dir() + skills = load_project_skills(temp_skills_dir_with_uppercase_context_files) + skills_by_name = {s.name: s for s in skills} # Verify that CLAUDE.MD and GEMINI.MD files were loaded as RepoSkills - assert len(repo_agents) == 3 # repo.md + CLAUDE.MD + GEMINI.MD - assert "repo" in repo_agents - assert "claude" in repo_agents - assert "gemini" in repo_agents + assert len(skills_by_name) == 3 # repo.md + CLAUDE.MD + GEMINI.MD + assert "repo" in skills_by_name + assert "claude" in skills_by_name + assert "gemini" in skills_by_name # Check CLAUDE.MD agent - claude_agent = repo_agents["claude"] + claude_agent = skills_by_name["claude"] assert claude_agent.trigger is None assert claude_agent.name == "claude" assert "Claude-Specific Instructions" in claude_agent.content # Check GEMINI.MD agent - gemini_agent = repo_agents["gemini"] + gemini_agent = skills_by_name["gemini"] assert gemini_agent.trigger is None assert gemini_agent.name == "gemini" assert "Gemini-Specific Instructions" in gemini_agent.content @@ -527,16 +528,17 @@ def test_load_skills_with_truncated_large_file(temp_skills_dir_with_large_contex from openhands.sdk.context.skills.skill import THIRD_PARTY_SKILL_MAX_CHARS root, original_size = temp_skills_dir_with_large_context_file - skills_dir = root / ".openhands" / "skills" - repo_agents, knowledge_agents, _ = load_skills_from_dir(skills_dir) + # Third-party files are loaded by load_project_skills(), not load_skills_from_dir() + skills = load_project_skills(root) + skills_by_name = {s.name: s for s in skills} # Verify that CLAUDE.md file was loaded but truncated - assert len(repo_agents) == 2 # repo.md + claude.md - assert "claude" in repo_agents + assert len(skills_by_name) == 2 # repo.md + claude.md + assert "claude" in skills_by_name # Check that content was truncated - claude_agent = repo_agents["claude"] + claude_agent = skills_by_name["claude"] assert claude_agent.trigger is None assert claude_agent.name == "claude" diff --git a/tests/sdk/context/test_agent_context_model_specific.py b/tests/sdk/context/test_agent_context_model_specific.py index 84cb69d0b2..638df1030d 100644 --- a/tests/sdk/context/test_agent_context_model_specific.py +++ b/tests/sdk/context/test_agent_context_model_specific.py @@ -2,7 +2,7 @@ from pathlib import Path from openhands.sdk.context.agent_context import AgentContext -from openhands.sdk.context.skills import load_skills_from_dir +from openhands.sdk.context.skills import load_project_skills def _write_repo_with_vendor_files(root: Path): @@ -24,9 +24,9 @@ def _write_repo_with_vendor_files(root: Path): def test_context_gates_claude_vendor_file(): with tempfile.TemporaryDirectory() as d: root = Path(d) - skills_dir = _write_repo_with_vendor_files(root) - repo_skills, _, _ = load_skills_from_dir(skills_dir) - ac = AgentContext(skills=list(repo_skills.values())) + _write_repo_with_vendor_files(root) + skills = load_project_skills(root) + ac = AgentContext(skills=skills) suffix = ac.get_system_message_suffix( llm_model="litellm_proxy/anthropic/claude-sonnet-4" ) @@ -39,9 +39,9 @@ def test_context_gates_claude_vendor_file(): def test_context_gates_gemini_vendor_file(): with tempfile.TemporaryDirectory() as d: root = Path(d) - skills_dir = _write_repo_with_vendor_files(root) - repo_skills, _, _ = load_skills_from_dir(skills_dir) - ac = AgentContext(skills=list(repo_skills.values())) + _write_repo_with_vendor_files(root) + skills = load_project_skills(root) + ac = AgentContext(skills=skills) suffix = ac.get_system_message_suffix(llm_model="gemini-2.5-pro") assert suffix is not None assert "Repo baseline" in suffix @@ -52,9 +52,9 @@ def test_context_gates_gemini_vendor_file(): def test_context_excludes_both_for_other_models(): with tempfile.TemporaryDirectory() as d: root = Path(d) - skills_dir = _write_repo_with_vendor_files(root) - repo_skills, _, _ = load_skills_from_dir(skills_dir) - ac = AgentContext(skills=list(repo_skills.values())) + _write_repo_with_vendor_files(root) + skills = load_project_skills(root) + ac = AgentContext(skills=skills) suffix = ac.get_system_message_suffix(llm_model="openai/gpt-4o") assert suffix is not None assert "Repo baseline" in suffix @@ -65,9 +65,9 @@ def test_context_excludes_both_for_other_models(): def test_context_uses_canonical_name_for_vendor_match(): with tempfile.TemporaryDirectory() as d: root = Path(d) - skills_dir = _write_repo_with_vendor_files(root) - repo_skills, _, _ = load_skills_from_dir(skills_dir) - ac = AgentContext(skills=list(repo_skills.values())) + _write_repo_with_vendor_files(root) + skills = load_project_skills(root) + ac = AgentContext(skills=skills) # Non-matching "proxy" model, but canonical matches Anthropic/Claude suffix = ac.get_system_message_suffix( llm_model="proxy/test-model", @@ -82,9 +82,9 @@ def test_context_uses_canonical_name_for_vendor_match(): def test_context_includes_all_when_model_unknown(): with tempfile.TemporaryDirectory() as d: root = Path(d) - skills_dir = _write_repo_with_vendor_files(root) - repo_skills, _, _ = load_skills_from_dir(skills_dir) - ac = AgentContext(skills=list(repo_skills.values())) + _write_repo_with_vendor_files(root) + skills = load_project_skills(root) + ac = AgentContext(skills=skills) # No model info provided -> backward-compatible include-all behavior suffix = ac.get_system_message_suffix() assert suffix is not None