From 77b313cd4fe0b189d2439196a342b72fcfe44a0f Mon Sep 17 00:00:00 2001 From: Leoyzen Date: Fri, 6 Feb 2026 22:43:26 +0800 Subject: [PATCH 1/2] feat: add configurable skills loading paths with YAML configuration Introduce skills configuration section to manifest allowing users to specify custom skill directories and control default path inclusion. Skills are discovered from configured paths with "first path wins" semantics for conflict resolution. Key changes: - Add SkillsConfig model with paths and include_default fields - Update AgentsManifest to support skills configuration - Enhance SkillsManager to use configured paths for discovery - Implement relative path resolution against config file location - Support remote filesystems (s3://, github://) for skill discovery - Add comprehensive tests for path resolution and backward compatibility The implementation ensures existing configurations without skills section continue to work exactly as before, with default paths (~/.claude/skills/, .claude/skills/) automatically included unless explicitly disabled. --- src/agentpool/delegation/pool.py | 12 +- src/agentpool/models/manifest.py | 17 ++ src/agentpool/skills/manager.py | 45 +++- src/agentpool/skills/registry.py | 6 +- src/agentpool_config/__init__.py | 3 + src/agentpool_config/skills.py | 100 ++++++++- tests/test_config/__init__.py | 1 + tests/test_config/test_skills_config.py | 197 ++++++++++++++++ tests/test_skills/test_manager_config.py | 122 ++++++++++ tests/test_skills/test_skills_integration.py | 222 +++++++++++++++++++ 10 files changed, 707 insertions(+), 18 deletions(-) create mode 100644 tests/test_config/__init__.py create mode 100644 tests/test_config/test_skills_config.py create mode 100644 tests/test_skills/test_manager_config.py create mode 100644 tests/test_skills/test_skills_integration.py diff --git a/src/agentpool/delegation/pool.py b/src/agentpool/delegation/pool.py index 3a3373890..759252660 100644 --- a/src/agentpool/delegation/pool.py +++ b/src/agentpool/delegation/pool.py @@ -10,7 +10,7 @@ from anyenv import ProcessManager import anyio -from upathtools import UPath +from upathtools import UPath, to_upath from agentpool.common_types import NodeName, SupportsStructuredOutput from agentpool.delegation.message_flow_tracker import MessageFlowTracker @@ -101,10 +101,13 @@ def __init__( # noqa: PLR0915 match manifest: case None: self.manifest = AgentsManifest() + self._config_file_path: UPath | None = None case str() | os.PathLike() | UPath(): + self._config_file_path = to_upath(manifest) self.manifest = AgentsManifest.from_file(manifest) case AgentsManifest(): self.manifest = manifest + self._config_file_path = None case _: raise ValueError(f"Invalid config type: {type(manifest)}") registry.configure_observability(self.manifest.observability) @@ -124,7 +127,12 @@ def __init__( # noqa: PLR0915 self.connection_registry = ConnectionRegistry() servers = self.manifest.get_mcp_servers() self.mcp = MCPManager(name="pool_mcp", servers=servers, owner="pool") - self.skills = SkillsManager(name="pool_skills", owner="pool") + self.skills = SkillsManager( + name="pool_skills", + owner="pool", + config=self.manifest.skills, + config_file_path=self._config_file_path, + ) self._tasks = TaskRegistry() self.prompt_manager = PromptManager(self.manifest.prompts) # Main agent name: explicit param > manifest.default_agent > None (will use first) diff --git a/src/agentpool/models/manifest.py b/src/agentpool/models/manifest.py index 25f6acd97..0949c9d73 100644 --- a/src/agentpool/models/manifest.py +++ b/src/agentpool/models/manifest.py @@ -25,6 +25,7 @@ from agentpool_config.observability import ObservabilityConfig from agentpool_config.output_types import StructuredResponseConfig from agentpool_config.pool_server import MCPPoolServerConfig +from agentpool_config.skills import SkillsConfig from agentpool_config.storage import StorageConfig from agentpool_config.system_prompts import PromptLibraryConfig from agentpool_config.task import Job @@ -329,6 +330,22 @@ class AgentsManifest(Schema): Docs: https://phil65.github.io/agentpool/YAML%20Configuration/prompt_configuration/ """ + skills: SkillsConfig = Field(default_factory=SkillsConfig) + """Custom skill discovery paths configuration. + + Defines where to search for custom skills. Skills are discovered from + configured directories following "first path wins" semantics. + + Example: + ```yaml + skills: + paths: + - ./my-skills + - s3://bucket/skills + include_default: true + ``` + """ + commands: dict[str, CommandConfig | str] = Field( default_factory=dict, examples=[ diff --git a/src/agentpool/skills/manager.py b/src/agentpool/skills/manager.py index fc69829ca..3ffb2ff96 100644 --- a/src/agentpool/skills/manager.py +++ b/src/agentpool/skills/manager.py @@ -8,11 +8,12 @@ from agentpool.log import get_logger from agentpool.skills.registry import SkillsRegistry +from agentpool_config.skills import SkillsConfig # noqa: TC001 if TYPE_CHECKING: from fsspec import AbstractFileSystem - from upathtools import JoinablePathLike + from upathtools import JoinablePathLike, UPath from agentpool.skills.skill import Skill @@ -33,6 +34,8 @@ def __init__( name: str = "skills", owner: str | None = None, skills_dirs: list[JoinablePathLike] | None = None, + config: SkillsConfig | None = None, + config_file_path: UPath | None = None, ) -> None: """Initialize the skills manager. @@ -40,20 +43,24 @@ def __init__( name: Name for this manager owner: Owner of this manager skills_dirs: Directories to search for skills + config: Optional skills configuration from manifest + config_file_path: Optional path to configuration file for resolving relative paths """ self.name = name self.owner = owner self.registry = SkillsRegistry(skills_dirs) self._initialized = False + self._config = config + self._config_file_path = config_file_path def __repr__(self) -> str: skill_count = len(self.registry.list_items()) if self._initialized else "?" return f"SkillsManager(name={self.name!r}, skills={skill_count})" async def __aenter__(self) -> Self: - """Initialize the skills manager and discover skills.""" + """Initialize to skills manager and discover skills.""" try: - await self.registry.discover_skills() + await self.discover_skills(self._config, self._config_file_path) self._initialized = True count = len(self.registry.list_items()) logger.info("Skills manager initialized", name=self.name, skill_count=count) @@ -101,9 +108,39 @@ async def add_skills_directory( await self.registry.register_skills_from_path(upath) logger.info("Added skills directory", path=str(path)) + async def discover_skills( + self, + config: SkillsConfig | None = None, + config_file_path: UPath | None = None, + ) -> None: + """Discover skills from configured paths. + + Args: + config: Optional skills configuration. + config_file_path: Optional path to the configuration file for resolving relative paths. + """ + from agentpool_config.skills import DEFAULT_SKILLS_PATHS + + if config: + paths = config.get_effective_paths(config_file_path) + default_paths = [p.expanduser() for p in DEFAULT_SKILLS_PATHS] + else: + paths = self.registry.skills_dirs + default_paths = [p.expanduser() for p in DEFAULT_SKILLS_PATHS] + + for path in reversed(paths): + upath = to_upath(path).expanduser() + if not upath.exists(): + if any(upath == dp for dp in default_paths): + logger.debug("Default skills directory not found", path=upath) + else: + logger.warning("Custom skills directory not found", path=upath) + continue + await self.registry.register_skills_from_path(upath, replace=True) + async def refresh(self) -> None: """Force rediscovery of all skills.""" - await self.registry.discover_skills() + await self.discover_skills() skill_count = len(self.registry.list_items()) logger.info("Skills refreshed", name=self.name, skill_count=skill_count) diff --git a/src/agentpool/skills/registry.py b/src/agentpool/skills/registry.py index 39cf2f5c3..c5a2d9ae0 100644 --- a/src/agentpool/skills/registry.py +++ b/src/agentpool/skills/registry.py @@ -55,6 +55,7 @@ async def register_skills_from_path( self, skills_dir: JoinablePathLike | AbstractFileSystem, base_path: str | None = None, + replace: bool = True, **storage_options: Any, ) -> None: """Register skills from a given path. @@ -63,6 +64,7 @@ async def register_skills_from_path( skills_dir: Path to the directory containing skills, or filesystem instance. base_path: When skills_dir is a filesystem, the path within that filesystem to look for skills. Defaults to root_marker if not specified. + replace: Whether to replace existing skills with same name. storage_options: Additional options to pass to the filesystem. """ if isinstance(skills_dir, AbstractFileSystem): @@ -80,7 +82,7 @@ async def register_skills_from_path( # List entries in skills directory entries = await fs._ls(search_path, detail=True) except FileNotFoundError: - logger.warning("Skills directory not found", path=search_path) + logger.debug("Skills directory not found", path=search_path) return # Filter for directories that might contain skills skill_dirs = [ @@ -111,7 +113,7 @@ async def register_skills_from_path( try: skill = self._parse_skill(skill_dir_path) - self.register(skill.name, skill, replace=True) + self.register(skill.name, skill, replace=replace) except Exception as e: # noqa: BLE001 # Log but don't fail discovery for one bad skill print(f"Warning: Failed to parse skill at {skill_dir_path}: {e}") diff --git a/src/agentpool_config/__init__.py b/src/agentpool_config/__init__.py index 1cd800144..db19d59e2 100644 --- a/src/agentpool_config/__init__.py +++ b/src/agentpool_config/__init__.py @@ -36,6 +36,7 @@ PromptHookConfig, ) from agentpool_config.toolsets import ToolsetConfig +from agentpool_config.skills import SkillsConfig, DEFAULT_SKILLS_PATHS from agentpool_config.resolution import ( ConfigLayer, ConfigSource, @@ -64,6 +65,7 @@ Field(discriminator="type"), ] __all__ = [ + "DEFAULT_SKILLS_PATHS", "AnyToolConfig", "BaseEventHandlerConfig", "BaseHookConfig", @@ -84,6 +86,7 @@ "ResolvedConfig", "SSEMCPServerConfig", "SessionQuery", + "SkillsConfig", "StdioMCPServerConfig", "StdoutEventHandlerConfig", "StreamableHTTPMCPServerConfig", diff --git a/src/agentpool_config/skills.py b/src/agentpool_config/skills.py index f6d2178dd..1aa0d59dc 100644 --- a/src/agentpool_config/skills.py +++ b/src/agentpool_config/skills.py @@ -1,17 +1,97 @@ """Skills configuration.""" -from dataclasses import dataclass +from __future__ import annotations +from pydantic import ConfigDict, Field +from schemez import Schema +from upathtools import UPath -@dataclass -class Skill: - """Skill configuration.""" - url: str - name: str +DEFAULT_SKILLS_PATHS = [ + UPath("~/.claude/skills/"), + UPath(".claude/skills/"), +] -dev_browser = Skill( - url="https://github.com/SawyerHood/dev-browser/tree/main/skills/dev-browser", - name="dev-browser", -) +class SkillsConfig(Schema): + """Configuration for custom skill discovery paths. + + Skills are discovered from configured directories, allowing + users to add custom skills from local paths. The discovery + follows "first path wins" semantics - earlier paths in the list + take precedence over later ones. + + Default paths (when include_default=True): + - ~/.claude/skills/ (user home directory) + - .claude/skills/ (relative to current directory) + """ + + model_config = ConfigDict( + json_schema_extra={ + "x-icon": "octicon:mortar-board-16", + "x-doc-title": "Skills Configuration", + } + ) + + paths: list[UPath] = Field( + default_factory=list, + title="Custom skill paths", + examples=[["/path/to/skills", "./my-skills", "s3://bucket/skills"]], + ) + """List of custom paths to search for skills. + + Paths can be: + - Absolute: /home/user/skills + - Relative: ./my-skills (resolved against config file location or CWD) + - Remote: s3://bucket/skills, github://org/repo/skills + + Earlier paths take precedence over later ones ("first path wins"). + """ + + include_default: bool = Field( + default=True, + title="Include default paths", + examples=[True, False], + ) + """Whether to include default skill paths in discovery. + + Default paths are appended after custom paths: + - ~/.claude/skills/ + - .claude/skills/ + + Set to False to disable default paths entirely. + """ + + def get_effective_paths(self, config_file_path: UPath | None = None) -> list[UPath]: + """Get the effective list of paths for skill discovery. + + Resolves relative paths against the config file location (if provided) + or current working directory, then appends default paths if enabled. + + Args: + config_file_path: Path to the YAML configuration file. + Relative paths in self.paths are resolved against this file's + parent directory. If None, relative paths are resolved against + the current working directory. + + Returns: + List of UPath objects for skill discovery, ordered by priority + (custom paths first, then default paths if enabled). + """ + result: list[UPath] = [] + + # Resolve custom paths + base_path = config_file_path.parent if config_file_path is not None else UPath.cwd() + + for path in self.paths: + if path.is_absolute(): + result.append(path) + else: + # Resolve relative paths against base path and normalize + result.append((base_path / path).resolve()) + + # Append default paths if enabled + if self.include_default: + result.extend(DEFAULT_SKILLS_PATHS) + + return result diff --git a/tests/test_config/__init__.py b/tests/test_config/__init__.py new file mode 100644 index 000000000..56da449c6 --- /dev/null +++ b/tests/test_config/__init__.py @@ -0,0 +1 @@ +"""Tests for configuration models.""" diff --git a/tests/test_config/test_skills_config.py b/tests/test_config/test_skills_config.py new file mode 100644 index 000000000..5300ec6b3 --- /dev/null +++ b/tests/test_config/test_skills_config.py @@ -0,0 +1,197 @@ +"""Tests for SkillsConfig model.""" + +from __future__ import annotations + +import pytest +from upathtools import UPath + +from agentpool_config.skills import DEFAULT_SKILLS_PATHS, SkillsConfig + + +def test_skills_config_default_values(): + """Test SkillsConfig with default values.""" + config = SkillsConfig() + + assert config.paths == [] + assert config.include_default is True + + +def test_skills_config_with_custom_paths(): + """Test SkillsConfig with custom paths.""" + config = SkillsConfig(paths=[UPath("./my-skills"), UPath("/absolute/path")]) + + assert len(config.paths) == 2 + assert config.paths[0] == UPath("./my-skills") + assert config.paths[1] == UPath("/absolute/path") + assert config.include_default is True + + +def test_skills_config_include_default_false(): + """Test SkillsConfig with include_default set to False.""" + config = SkillsConfig(include_default=False) + + assert config.paths == [] + assert config.include_default is False + + +def test_get_effective_paths_custom_only(): + """Test get_effective_paths with custom paths only (no defaults).""" + config = SkillsConfig( + paths=[UPath("./skills"), UPath("/absolute/skills")], + include_default=False, + ) + + result = config.get_effective_paths() + + assert len(result) == 2 + # Custom paths should be resolved to absolute + assert result[0].is_absolute() + assert str(result[0]).endswith("skills") + assert result[1] == UPath("/absolute/skills") + + +def test_get_effective_paths_with_defaults(): + """Test get_effective_paths includes default paths when enabled.""" + config = SkillsConfig( + paths=[UPath("./custom-skills")], + include_default=True, + ) + + result = config.get_effective_paths() + + assert len(result) == 3 + # First should be custom path (resolved to absolute) + assert result[0].is_absolute() + assert str(result[0]).endswith("custom-skills") + # Last two should be default paths + assert result[1] == DEFAULT_SKILLS_PATHS[0] # ~/.claude/skills/ + assert result[2] == DEFAULT_SKILLS_PATHS[1] # .claude/skills/ + + +def test_get_effective_paths_with_config_file_path(): + """Test get_effective_paths resolves relative paths against config file.""" + # Create a mock config file path + config_file = UPath("/home/user/project/config.yml") + + config = SkillsConfig( + paths=[UPath("../shared-skills"), UPath("./local-skills")], + include_default=False, + ) + + result = config.get_effective_paths(config_file_path=config_file) + + assert len(result) == 2 + # ../shared-skills from /home/user/project/config.yml -> /home/user/shared-skills + # Use endswith because resolve() may resolve to different absolute path on different OS + assert str(result[0]).endswith("/home/user/shared-skills") + # ./local-skills from /home/user/project/config.yml -> /home/user/project/local-skills + assert str(result[1]).endswith("/home/user/project/local-skills") + + +def test_get_effective_paths_absolute_paths_unaffected(): + """Test that absolute paths are not modified by config_file_path.""" + config_file = UPath("/some/other/path/config.yml") + + config = SkillsConfig( + paths=[UPath("/custom/absolute/skills")], + include_default=False, + ) + + result = config.get_effective_paths(config_file_path=config_file) + + assert len(result) == 1 + assert result[0] == UPath("/custom/absolute/skills") + + +def test_get_effective_paths_no_config_file_uses_cwd(): + """Test that relative paths resolve to CWD when no config_file_path.""" + # We can't easily test exact path without knowing test CWD, + # but we can verify the path is absolute + config = SkillsConfig( + paths=[UPath("./test-skills")], + include_default=False, + ) + + result = config.get_effective_paths(config_file_path=None) + + assert len(result) == 1 + assert result[0].is_absolute() + assert str(result[0]).endswith("test-skills") + + +def test_get_effective_paths_remote_paths(): + """Test that remote paths are preserved as-is.""" + config = SkillsConfig( + paths=[UPath("s3://bucket/skills"), UPath("github://org/repo/skills")], + include_default=False, + ) + + result = config.get_effective_paths() + + assert len(result) == 2 + assert result[0] == UPath("s3://bucket/skills") + assert result[1] == UPath("github://org/repo/skills") + + +def test_get_effective_paths_first_path_wins(): + """Test 'first path wins' priority - custom paths before defaults.""" + config = SkillsConfig( + paths=[UPath("./my-skills")], + include_default=True, + ) + + result = config.get_effective_paths() + + # Custom paths come first + assert str(result[0]).endswith("my-skills") + # Default paths come after + assert result[1] == DEFAULT_SKILLS_PATHS[0] + assert result[2] == DEFAULT_SKILLS_PATHS[1] + + +def test_pydantic_validation(): + """Test that SkillsConfig validates properly with Pydantic.""" + # Valid config + config = SkillsConfig(paths=[UPath("/path")], include_default=True) + assert config.paths == [UPath("/path")] + assert config.include_default is True + + # Invalid types should raise ValidationError + from pydantic import ValidationError + + with pytest.raises(ValidationError): + SkillsConfig(paths=["not", "a", "list"], include_default="not a bool") + + +def test_empty_config_no_defaults(): + """Test empty config with defaults disabled returns empty list.""" + config = SkillsConfig(paths=[], include_default=False) + + result = config.get_effective_paths() + + assert result == [] + + +def test_config_yaml_roundtrip(): + """Test that SkillsConfig can be serialized/deserialized.""" + config = SkillsConfig( + paths=[UPath("./skills"), UPath("/absolute/skills")], + include_default=True, + ) + + # Serialize to dict + config_dict = config.model_dump() + + # Deserialize back + config2 = SkillsConfig(**config_dict) + + assert config2.paths == config.paths + assert config2.include_default == config.include_default + + # Verify effective paths are the same + paths1 = config.get_effective_paths() + paths2 = config2.get_effective_paths() + + assert len(paths1) == len(paths2) + for p1, p2 in zip(paths1, paths2, strict=True): + assert p1 == p2 diff --git a/tests/test_skills/test_manager_config.py b/tests/test_skills/test_manager_config.py new file mode 100644 index 000000000..4937762e6 --- /dev/null +++ b/tests/test_skills/test_manager_config.py @@ -0,0 +1,122 @@ +"""Tests for SkillsManager configuration-based discovery.""" + +from __future__ import annotations + +import logging +from pathlib import Path +import tempfile +from textwrap import dedent + +import pytest +from upathtools import UPath + +from agentpool.skills.manager import SkillsManager +from agentpool_config.skills import SkillsConfig + + +@pytest.fixture +def skill_dirs(): + """Create two temporary directories with conflicting test skills.""" + with tempfile.TemporaryDirectory() as temp_dir: + base = Path(temp_dir) + dir_a = base / "dir_a" + dir_b = base / "dir_b" + dir_a.mkdir() + dir_b.mkdir() + + # Skill in dir_a + skill_a = dir_a / "my_skill" + skill_a.mkdir() + (skill_a / "SKILL.md").write_text( + dedent(""" + --- + name: my_skill + description: Description from A + --- + Instructions A + """).strip() + ) + + # Same skill name in dir_b + skill_b = dir_b / "my_skill" + skill_b.mkdir() + (skill_b / "SKILL.md").write_text( + dedent(""" + --- + name: my_skill + description: Description from B + --- + Instructions B + """).strip() + ) + + yield dir_a, dir_b + + +@pytest.mark.asyncio +async def test_discover_skills_priority(skill_dirs: tuple[Path, Path]): + """Test that the first path in the config takes precedence (first path wins).""" + dir_a, dir_b = skill_dirs + # config.paths = [dir_a, dir_b] -> A should win because it's processed LAST in reversed list + config = SkillsConfig(paths=[UPath(dir_a), UPath(dir_b)], include_default=False) + + manager = SkillsManager() + await manager.discover_skills(config=config) + + skill = manager.get_skill("my_skill") + assert skill.description == "Description from A" + + # Now swap priority: [dir_b, dir_a] -> B should win + config_swapped = SkillsConfig(paths=[UPath(dir_b), UPath(dir_a)], include_default=False) + manager_swapped = SkillsManager() + await manager_swapped.discover_skills(config=config_swapped) + + skill_swapped = manager_swapped.get_skill("my_skill") + assert skill_swapped.description == "Description from B" + + +@pytest.mark.asyncio +async def test_discover_skills_no_config(skill_dirs: tuple[Path, Path]): + """Test discovery without a config object.""" + dir_a, _ = skill_dirs + manager = SkillsManager(skills_dirs=[dir_a]) + await manager.discover_skills() + + skill = manager.get_skill("my_skill") + assert skill.description == "Description from A" + + +@pytest.mark.asyncio +async def test_discover_skills_logging( + caplog: pytest.LogCaptureFixture, monkeypatch: pytest.MonkeyPatch +): + """Test that missing custom paths log WARNING and missing default paths log DEBUG.""" + from agentpool_config import skills + + mock_default = [UPath("/non/existent/default/path")] + monkeypatch.setattr(skills, "DEFAULT_SKILLS_PATHS", mock_default) + + config = SkillsConfig(paths=[UPath("/non/existent/custom/path")], include_default=True) + + manager = SkillsManager() + with caplog.at_level(logging.DEBUG): + await manager.discover_skills(config=config) + + # Check for WARNING for custom path + assert any( + "Custom skills directory not found" in record.message and record.levelno == logging.WARNING + for record in caplog.records + ) + + # Check for DEBUG for default paths + assert any( + "Default skills directory not found" in record.message and record.levelno == logging.DEBUG + for record in caplog.records + ) + + # Check for DEBUG for default paths (they likely don't exist in the test environment) + print(f"Logged messages: {[r.message for r in caplog.records]}") + assert any( + "Default skills directory not found" in record.message and record.levelno == logging.DEBUG + for record in caplog.records + ) diff --git a/tests/test_skills/test_skills_integration.py b/tests/test_skills/test_skills_integration.py new file mode 100644 index 000000000..d3679f750 --- /dev/null +++ b/tests/test_skills/test_skills_integration.py @@ -0,0 +1,222 @@ +"""Integration tests for configurable skill loading paths in AgentPool.""" + +from __future__ import annotations + +import os +from pathlib import Path +import tempfile +from textwrap import dedent +from typing import Any + +import pytest +from upathtools import UPath +import yaml + +from agentpool.delegation.pool import AgentPool + + +def create_skill(path: Path, name: str, description: str, instructions: str): + """Create a skill in the specified directory.""" + skill_dir = path / name + skill_dir.mkdir(parents=True, exist_ok=True) + (skill_dir / "SKILL.md").write_text( + dedent(f""" + --- + name: {name} + description: {description} + --- + {instructions} + """).strip() + ) + + +@pytest.fixture +def temp_skills(): + """Create temporary skill directories.""" + with tempfile.TemporaryDirectory() as temp_dir: + base = Path(temp_dir) + dir_a = base / "dir_a" + dir_b = base / "dir_b" + dir_a.mkdir() + dir_b.mkdir() + + create_skill(dir_a, "skill_a", "Description A", "Instructions A") + create_skill(dir_b, "skill_b", "Description B", "Instructions B") + create_skill(dir_b, "conflict_skill", "Conflict from B", "Instructions Conflict B") + create_skill(dir_a, "conflict_skill", "Conflict from A", "Instructions Conflict A") + + yield dir_a, dir_b + + +@pytest.mark.asyncio +async def test_skills_backward_compatibility(): + """Init pool with a manifest having NO skills section. + + Assert that default paths are searched. + """ + # Create a manifest without skills section + manifest_dict: dict[str, Any] = { + "agents": { + "test_agent": { + "type": "native", + "model": "openai:gpt-4o", + } + } + } + + with tempfile.TemporaryDirectory() as temp_dir: + config_path = Path(temp_dir) / "config.yml" + config_path.write_text(yaml.dump(manifest_dict)) + + async with AgentPool(config_path) as pool: + # We can't easily assert default paths existence since they depend on the environment, + # but we can verify that the SkillsManager is initialized with default config. + assert pool.skills._config is not None # type: ignore + assert pool.skills._config.include_default is True # type: ignore + assert pool.skills._config.paths == [] # type: ignore + + +@pytest.mark.asyncio +async def test_skills_custom_path(temp_skills: tuple[Path, Path]): + """Init pool with a manifest having a custom skills.paths. + + Assert that skills from that path are loaded. + """ + dir_a, _ = temp_skills + + manifest_dict: dict[str, Any] = { + "skills": {"paths": [str(dir_a)], "include_default": False}, + "agents": { + "test_agent": { + "type": "native", + "model": "openai:gpt-4o", + } + }, + } + + with tempfile.TemporaryDirectory() as temp_dir: + config_path = Path(temp_dir) / "config.yml" + config_path.write_text(yaml.dump(manifest_dict)) + + async with AgentPool(config_path) as pool: + skills = pool.skills.list_skills() + skill_names = [s.name for s in skills] + assert "skill_a" in skill_names + assert "skill_b" not in skill_names + + skill = pool.skills.get_skill("skill_a") + assert skill.description == "Description A" + + +@pytest.mark.asyncio +async def test_skills_disable_defaults(monkeypatch: pytest.MonkeyPatch): + """Init pool with skills.include_default: false. + + Assert that default paths are NOT searched. + """ + from agentpool_config import skills + + # Mock default paths to something we can control + with tempfile.TemporaryDirectory() as temp_dir: + default_dir = Path(temp_dir) / "default_skills" + default_dir.mkdir() + create_skill(default_dir, "default_skill", "Default", "Instructions") + + monkeypatch.setattr(skills, "DEFAULT_SKILLS_PATHS", [UPath(default_dir)]) + + manifest_dict: dict[str, Any] = { + "skills": {"include_default": False}, + "agents": { + "test_agent": { + "type": "native", + "model": "openai:gpt-4o", + } + }, + } + + with tempfile.TemporaryDirectory() as temp_dir_2: + config_path = Path(temp_dir_2) / "config.yml" + config_path.write_text(yaml.dump(manifest_dict)) + + async with AgentPool(config_path) as pool: + skills_list = pool.skills.list_skills() + skill_names = [s.name for s in skills_list] + assert "default_skill" not in skill_names + + +@pytest.mark.asyncio +async def test_skills_conflict_resolution(temp_skills: tuple[Path, Path]): + """Init pool with two paths containing the same skill name. + + Assert that the version from the EARLIER path in the list is the one loaded. + """ + dir_a, dir_b = temp_skills + + # [dir_a, dir_b] -> dir_a should win + manifest_dict: dict[str, Any] = { + "skills": {"paths": [str(dir_a), str(dir_b)], "include_default": False}, + "agents": { + "test_agent": { + "type": "native", + "model": "openai:gpt-4o", + } + }, + } + + with tempfile.TemporaryDirectory() as temp_dir: + config_path = Path(temp_dir) / "config.yml" + config_path.write_text(yaml.dump(manifest_dict)) + + async with AgentPool(config_path) as pool: + skill = pool.skills.get_skill("conflict_skill") + assert skill.description == "Conflict from A" + + # [dir_b, dir_a] -> dir_b should win + manifest_dict["skills"]["paths"] = [str(dir_b), str(dir_a)] + + with tempfile.TemporaryDirectory() as temp_dir: + config_path = Path(temp_dir) / "config.yml" + config_path.write_text(yaml.dump(manifest_dict)) + + async with AgentPool(config_path) as pool: + skill = pool.skills.get_skill("conflict_skill") + assert skill.description == "Conflict from B" + + +@pytest.mark.asyncio +async def test_skills_relative_paths(): + """Init pool from a YAML file that specifies relative skill paths. + + Assert that they are resolved correctly relative to the YAML file. + """ + with tempfile.TemporaryDirectory() as temp_dir: + config_dir = Path(temp_dir) + # Create a relative path from config_dir to dir_a + # In this test, we can just move dir_a inside config_dir/skills + skills_dir = config_dir / "my_skills" + skills_dir.mkdir() + create_skill(skills_dir, "rel_skill", "Relative Description", "Instructions") + + manifest_dict: dict[str, Any] = { + "skills": {"paths": ["./my_skills"], "include_default": False}, + "agents": { + "test_agent": { + "type": "native", + "model": "openai:gpt-4o", + } + }, + } + + config_path = config_dir / "config.yml" + config_path.write_text(yaml.dump(manifest_dict)) + + # Run from a different CWD to ensure relative path is resolved against config file + old_cwd = Path.cwd() + os.chdir(tempfile.gettempdir()) + try: + async with AgentPool(config_path) as pool: + skill = pool.skills.get_skill("rel_skill") + assert skill is not None + assert skill.description == "Relative Description" + finally: + os.chdir(old_cwd) From ed98c95d4eb3a2d3f875cf9fdfed9d1e61a84816 Mon Sep 17 00:00:00 2001 From: Leoyzen Date: Mon, 9 Feb 2026 21:36:17 +0800 Subject: [PATCH 2/2] Update src/agentpool/skills/manager.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com> --- src/agentpool/skills/manager.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/agentpool/skills/manager.py b/src/agentpool/skills/manager.py index 3ffb2ff96..d84b8da0d 100644 --- a/src/agentpool/skills/manager.py +++ b/src/agentpool/skills/manager.py @@ -121,12 +121,11 @@ async def discover_skills( """ from agentpool_config.skills import DEFAULT_SKILLS_PATHS + default_paths = [p.expanduser() for p in DEFAULT_SKILLS_PATHS] if config: paths = config.get_effective_paths(config_file_path) - default_paths = [p.expanduser() for p in DEFAULT_SKILLS_PATHS] else: paths = self.registry.skills_dirs - default_paths = [p.expanduser() for p in DEFAULT_SKILLS_PATHS] for path in reversed(paths): upath = to_upath(path).expanduser()