diff --git a/.gitignore b/.gitignore index 6ae86265a60c..c783906d0fdc 100644 --- a/.gitignore +++ b/.gitignore @@ -70,3 +70,4 @@ mini-swe-agent/ result website/static/api/skills-index.json models-dev-upstream/ +.claude/scheduled_tasks.lock diff --git a/tests/tools/test_skill_manager_tool.py b/tests/tools/test_skill_manager_tool.py index 96c3a361f0c2..abfa588d9430 100644 --- a/tests/tools/test_skill_manager_tool.py +++ b/tests/tools/test_skill_manager_tool.py @@ -943,3 +943,72 @@ def test_broken_sidecar_fails_open(self, tmp_path): side_effect=RuntimeError("sidecar broken")): result = _delete_skill("my-skill") assert result["success"] is True + + +# --------------------------------------------------------------------------- +# external_dirs honored on create — #21810 +# --------------------------------------------------------------------------- + + +class TestCreateSkillRespectsExternalDirs: + """skill_manage(action='create') must drop the new skill into the first + configured ``skills.external_dirs`` entry instead of the local + ~/.hermes/skills/ when one is set. Closes #21810. + """ + + def test_no_external_dirs_uses_local_skills_dir(self, tmp_path): + """Regression guard: with no external_dirs, behavior is unchanged.""" + with patch("tools.skill_manager_tool.SKILLS_DIR", tmp_path), \ + patch("agent.skill_utils.get_all_skills_dirs", return_value=[tmp_path]), \ + patch("agent.skill_utils.get_external_skills_dirs", return_value=[]): + result = _create_skill("local-skill", VALID_SKILL_CONTENT) + assert result["success"] is True + assert (tmp_path / "local-skill" / "SKILL.md").exists() + + def test_external_dir_used_when_configured(self, tmp_path): + """First external_dirs entry wins over SKILLS_DIR.""" + local = tmp_path / "local" + external = tmp_path / "external" + local.mkdir() + external.mkdir() + with patch("tools.skill_manager_tool.SKILLS_DIR", local), \ + patch("agent.skill_utils.get_all_skills_dirs", + return_value=[local, external]), \ + patch("agent.skill_utils.get_external_skills_dirs", + return_value=[external]): + result = _create_skill("ext-skill", VALID_SKILL_CONTENT) + assert result["success"] is True + assert (external / "ext-skill" / "SKILL.md").exists() + assert not (local / "ext-skill").exists() + + def test_external_dir_used_with_category(self, tmp_path): + """Category nesting still applies under the external root.""" + local = tmp_path / "local" + external = tmp_path / "external" + local.mkdir() + external.mkdir() + with patch("tools.skill_manager_tool.SKILLS_DIR", local), \ + patch("agent.skill_utils.get_all_skills_dirs", + return_value=[local, external]), \ + patch("agent.skill_utils.get_external_skills_dirs", + return_value=[external]): + result = _create_skill("cat-skill", VALID_SKILL_CONTENT, category="ops") + assert result["success"] is True + assert (external / "ops" / "cat-skill" / "SKILL.md").exists() + assert result["path"] == str(Path("ops") / "cat-skill") + + def test_first_external_dir_wins(self, tmp_path): + """When multiple external dirs configured, the first is preferred.""" + first = tmp_path / "first" + second = tmp_path / "second" + first.mkdir() + second.mkdir() + with patch("tools.skill_manager_tool.SKILLS_DIR", tmp_path / "local"), \ + patch("agent.skill_utils.get_all_skills_dirs", + return_value=[first, second]), \ + patch("agent.skill_utils.get_external_skills_dirs", + return_value=[first, second]): + result = _create_skill("multi-skill", VALID_SKILL_CONTENT) + assert result["success"] is True + assert (first / "multi-skill" / "SKILL.md").exists() + assert not (second / "multi-skill").exists() diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index d253cd2a7cd6..d684e66dd09d 100644 --- a/tools/skill_manager_tool.py +++ b/tools/skill_manager_tool.py @@ -268,11 +268,34 @@ def _validate_content_size(content: str, label: str = "SKILL.md") -> Optional[st return None +def _default_creation_dir() -> Path: + """Return the directory new skills should be created in. + + When ``skills.external_dirs`` is configured in ``config.yaml``, prefer the + first existing entry so contributors editing an external skills repo can + create skills in-place. Falls back to local ``~/.hermes/skills/`` when no + external dirs are configured. + + Closes #21810 — ``skill_manage(action='create')`` previously hardcoded + ``SKILLS_DIR``, dropping new skills into the user-local store even when an + external dir was set as the canonical authoring location. + """ + try: + from agent.skill_utils import get_external_skills_dirs + external = get_external_skills_dirs() + except Exception: + external = [] + if external: + return external[0] + return SKILLS_DIR + + def _resolve_skill_dir(name: str, category: str = None) -> Path: """Build the directory path for a new skill, optionally under a category.""" + base = _default_creation_dir() if category: - return SKILLS_DIR / category / name - return SKILLS_DIR / name + return base / category / name + return base / name def _find_skill(name: str) -> Optional[Dict[str, Any]]: @@ -412,10 +435,15 @@ def _create_skill(name: str, content: str, category: str = None) -> Dict[str, An shutil.rmtree(skill_dir, ignore_errors=True) return {"success": False, "error": scan_error} + creation_root = _containing_skills_root(skill_dir) + try: + rel_path = skill_dir.relative_to(creation_root) + except ValueError: + rel_path = skill_dir result = { "success": True, "message": f"Skill '{name}' created.", - "path": str(skill_dir.relative_to(SKILLS_DIR)), + "path": str(rel_path), "skill_md": str(skill_md), } if category: