hermes-agent/tests/tools/test_skills_tool.py

"""Tests for tools/skills_tool.py — skill discovery and viewing."""

import json
import os
import sys
from pathlib import Path
from unittest.mock import patch

import pytest

import tools.skills_tool as skills_tool_module
from tools.skills_tool import (
    _get_required_environment_variables,
    _parse_frontmatter,
    _parse_tags,
    _get_category_from_path,
    _find_all_skills,
    skill_matches_platform,
    skills_list,
    skill_view,
    MAX_DESCRIPTION_LENGTH,
)


def _make_skill(
    skills_dir, name, frontmatter_extra="", body="Step 1: Do the thing.", category=None
):
    """Helper to create a minimal skill directory."""
    if category:
        skill_dir = skills_dir / category / name
    else:
        skill_dir = skills_dir / name
    skill_dir.mkdir(parents=True, exist_ok=True)
    content = f"""\
---
name: {name}
description: Description for {name}.
{frontmatter_extra}---

# {name}

{body}
"""
    (skill_dir / "SKILL.md").write_text(content)
    return skill_dir


def _symlink_category(skills_dir: Path, linked_root: Path, category: str) -> Path:
    """Create a category symlink under skills_dir pointing outside the tree."""
    external_category = linked_root / category
    external_category.mkdir(parents=True, exist_ok=True)
    symlink_path = skills_dir / category
    try:
        symlink_path.symlink_to(external_category, target_is_directory=True)
    except (OSError, NotImplementedError) as exc:
        pytest.skip(f"symlinks unavailable in test environment: {exc}")
    return external_category


# ---------------------------------------------------------------------------
# _parse_frontmatter
# ---------------------------------------------------------------------------


class TestParseFrontmatter:
    def test_valid_and_nested_frontmatter(self):
        content = "---\nname: test\ndescription: A test.\n---\n\n# Body\n"
        fm, body = _parse_frontmatter(content)
        assert fm["name"] == "test"
        assert fm["description"] == "A test."
        assert "# Body" in body

        nested = "---\nname: test\nmetadata:\n  hermes:\n    tags: [a, b]\n---\n\nBody.\n"
        fm, _ = _parse_frontmatter(nested)
        assert fm["metadata"]["hermes"]["tags"] == ["a", "b"]


    def test_utf8_bom_frontmatter(self):
        """A leading UTF-8 BOM (Windows Notepad / PowerShell ``>`` save) must
        not drop the frontmatter. Confirms the fix reaches the tools/ surface
        via the _parse_frontmatter re-export."""
        bom = chr(0xFEFF)
        content = bom + "---\nname: test\ndescription: A test.\n---\n\n# Body\n"
        fm, body = _parse_frontmatter(content)
        assert fm["name"] == "test"
        assert fm["description"] == "A test."
        assert not body.startswith(bom)


# ---------------------------------------------------------------------------
# _parse_tags
# ---------------------------------------------------------------------------


class TestParseTags:
    def test_accepted_input_forms(self):
        assert _parse_tags(["a", "b", "c"]) == ["a", "b", "c"]
        assert _parse_tags("a, b, c") == ["a", "b", "c"]
        assert _parse_tags("[a, b, c]") == ["a", "b", "c"]
        # Quotes are stripped.
        result = _parse_tags("\"tag1\", 'tag2'")
        assert "tag1" in result
        assert "tag2" in result

    def test_empty_and_blank_items_dropped(self):
        assert _parse_tags("") == []
        assert _parse_tags(None) == []
        assert _parse_tags([]) == []
        assert _parse_tags([None, "", "valid"]) == ["valid"]


class TestRequiredEnvironmentVariablesNormalization:
    def test_parses_new_metadata_and_normalizes_legacy_prerequisites(self):
        frontmatter = {
            "required_environment_variables": [
                {
                    "name": "TENOR_API_KEY",
                    "prompt": "Tenor API key",
                    "help": "Get a key from https://developers.google.com/tenor",
                    "required_for": "full functionality",
                }
            ]
        }
        assert _get_required_environment_variables(frontmatter) == [
            {
                "name": "TENOR_API_KEY",
                "prompt": "Tenor API key",
                "help": "Get a key from https://developers.google.com/tenor",
                "required_for": "full functionality",
            }
        ]

        legacy = {"prerequisites": {"env_vars": ["TENOR_API_KEY"]}}
        assert _get_required_environment_variables(legacy) == [
            {
                "name": "TENOR_API_KEY",
                "prompt": "Enter value for TENOR_API_KEY",
            }
        ]

    def test_empty_env_file_value_is_treated_as_missing(self, monkeypatch):
        monkeypatch.setenv("FILLED_KEY", "value")
        monkeypatch.setenv("EMPTY_HOST_KEY", "")

        from tools.skills_tool import _is_env_var_persisted

        assert _is_env_var_persisted("EMPTY_FILE_KEY", {"EMPTY_FILE_KEY": ""}) is False
        assert (
            _is_env_var_persisted("FILLED_FILE_KEY", {"FILLED_FILE_KEY": "x"}) is True
        )
        assert _is_env_var_persisted("EMPTY_HOST_KEY", {}) is False
        assert _is_env_var_persisted("FILLED_KEY", {}) is True

    def test_active_profile_secret_scope_satisfies_requirement(self):
        """Cron workers must accept a value hydrated from this profile's vault."""
        from agent import secret_scope
        from tools.skills_tool import _is_env_var_persisted

        secret_scope.set_multiplex_active(True)
        token = secret_scope.set_secret_scope({"VAULT_SKILL_API_KEY": "vault-value"})
        try:
            assert _is_env_var_persisted("VAULT_SKILL_API_KEY", {}) is True
        finally:
            secret_scope.reset_secret_scope(token)
            secret_scope.set_multiplex_active(False)

    def test_unscoped_multiplex_requirement_does_not_read_process_environment(self, monkeypatch):
        """A worker without a profile scope must keep the fail-closed boundary."""
        from agent import secret_scope
        from tools.skills_tool import _is_env_var_persisted

        monkeypatch.setenv("OTHER_PROFILE_SKILL_API_KEY", "other-profile-value")
        secret_scope.set_multiplex_active(True)
        try:
            assert _is_env_var_persisted("OTHER_PROFILE_SKILL_API_KEY", {}) is False
        finally:
            secret_scope.set_multiplex_active(False)


# ---------------------------------------------------------------------------
# _get_category_from_path
# ---------------------------------------------------------------------------


class TestGetCategoryFromPath:
    def test_category_derived_from_layout(self, tmp_path):
        with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
            categorized = tmp_path / "mlops" / "axolotl" / "SKILL.md"
            categorized.parent.mkdir(parents=True)
            categorized.touch()
            assert _get_category_from_path(categorized) == "mlops"

            top_level = tmp_path / "my-skill" / "SKILL.md"
            top_level.parent.mkdir(parents=True)
            top_level.touch()
            assert _get_category_from_path(top_level) is None

        # Paths outside SKILLS_DIR have no category.
        with patch("tools.skills_tool.SKILLS_DIR", tmp_path / "skills"):
            assert _get_category_from_path(tmp_path / "other" / "SKILL.md") is None


# ---------------------------------------------------------------------------
# _find_all_skills
# ---------------------------------------------------------------------------


class TestFindAllSkills:
    def test_finds_skills_and_skips_non_skill_trees(self, tmp_path):
        with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
            _make_skill(tmp_path, "skill-a")
            _make_skill(tmp_path, "skill-b")
            _make_skill(tmp_path, "axolotl", category="mlops")

            # .git internals are not skills.
            git_dir = tmp_path / ".git" / "fake-skill"
            git_dir.mkdir(parents=True)
            (git_dir / "SKILL.md").write_text(
                "---\nname: fake\ndescription: x\n---\n\nBody.\n"
            )
            # Neither are skills vendored inside a nested virtualenv.
            typer_skill = (
                tmp_path
                / "bring"
                / "scripts"
                / ".venv"
                / "lib"
                / "python3.13"
                / "site-packages"
                / "typer"
                / ".agents"
                / "skills"
                / "typer"
            )
            typer_skill.mkdir(parents=True)
            (typer_skill / "SKILL.md").write_text(
                "---\nname: typer\ndescription: Should not be discovered.\n---\n",
                encoding="utf-8",
            )

            skills = _find_all_skills()

        assert {s["name"] for s in skills} == {"skill-a", "skill-b", "axolotl"}
        assert [s["category"] for s in skills if s["name"] == "axolotl"] == ["mlops"]


    def test_description_falls_back_to_body_and_is_truncated(self, tmp_path):
        no_desc = tmp_path / "no-desc"
        no_desc.mkdir()
        (no_desc / "SKILL.md").write_text(
            "---\nname: no-desc\n---\n\n# Heading\n\nFirst paragraph.\n"
        )
        long_dir = tmp_path / "long-desc"
        long_dir.mkdir()
        (long_dir / "SKILL.md").write_text(
            f"---\nname: long\ndescription: {'x' * (MAX_DESCRIPTION_LENGTH + 100)}\n---\n\nBody.\n"
        )

        with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
            skills = {s["name"]: s for s in _find_all_skills()}

        # If no description in frontmatter, the first non-header line is used.
        assert skills["no-desc"]["description"] == "First paragraph."
        assert len(skills["long"]["description"]) <= MAX_DESCRIPTION_LENGTH

    def test_finds_skills_in_symlinked_category_dir(self, tmp_path):
        external_root = tmp_path / "repo"
        skills_root = tmp_path / "skills"
        skills_root.mkdir()

        external_category = _symlink_category(skills_root, external_root, "linked")
        _make_skill(external_category.parent, "knowledge-brain", category="linked")

        with patch("tools.skills_tool.SKILLS_DIR", skills_root):
            skills = _find_all_skills()

        assert [s["name"] for s in skills] == ["knowledge-brain"]
        assert skills[0]["category"] == "linked"


# ---------------------------------------------------------------------------
# skills_list
# ---------------------------------------------------------------------------


class TestSkillsList:
    def test_empty_creates_directory(self, tmp_path):
        skills_dir = tmp_path / "skills"
        with patch("tools.skills_tool.SKILLS_DIR", skills_dir):
            raw = skills_list()
        result = json.loads(raw)
        assert result["success"] is True
        assert result["skills"] == []
        assert skills_dir.exists()

    def test_category_filter(self, tmp_path):
        with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
            _make_skill(tmp_path, "skill-a", category="devops")
            _make_skill(tmp_path, "skill-b", category="mlops")
            all_result = json.loads(skills_list())
            filtered = json.loads(skills_list(category="devops"))

        assert all_result["count"] == 2
        assert filtered["count"] == 1
        assert filtered["skills"][0]["name"] == "skill-a"

    def test_category_filter_finds_symlinked_category(self, tmp_path):
        external_root = tmp_path / "repo"
        skills_root = tmp_path / "skills"
        skills_root.mkdir()

        external_category = _symlink_category(skills_root, external_root, "linked")
        _make_skill(external_category.parent, "knowledge-brain", category="linked")

        with patch("tools.skills_tool.SKILLS_DIR", skills_root):
            raw = skills_list(category="linked")

        result = json.loads(raw)
        assert result["success"] is True
        assert result["count"] == 1
        assert result["categories"] == ["linked"]
        assert result["skills"][0]["name"] == "knowledge-brain"


# ---------------------------------------------------------------------------
# skill_view
# ---------------------------------------------------------------------------


class TestSkillView:
    def test_view_resolves_by_dir_name_and_frontmatter_name(self, tmp_path):
        with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
            _make_skill(
                tmp_path,
                "my-skill",
                frontmatter_extra="metadata:\n  hermes:\n    tags: [fine-tuning, llm]\n",
            )
            # The on-disk directory ("alias-dir") differs from the skill's
            # frontmatter name ("real-skill-name"). skills_list() exposes the
            # frontmatter name, so skill_view(name) must resolve it too.
            alias_dir = tmp_path / "alias-dir"
            alias_dir.mkdir(parents=True, exist_ok=True)
            (alias_dir / "SKILL.md").write_text(
                "---\n"
                "name: real-skill-name\n"
                "description: A skill whose directory name differs from its name.\n"
                "---\n\n"
                "# real-skill-name\n\n"
                "Step 1: Do the thing.\n"
            )
            by_dir = json.loads(skill_view("my-skill"))
            by_name = json.loads(skill_view("real-skill-name"))

        assert by_dir["success"] is True
        assert by_dir["name"] == "my-skill"
        assert "Step 1" in by_dir["content"]
        assert "fine-tuning" in by_dir["tags"]
        assert "llm" in by_dir["tags"]

        assert by_name["success"] is True
        assert "Step 1" in by_name["content"]

    def test_registered_view_tracks_use_with_task_and_session(self, tmp_path):
        from tools.skills_tool import _skill_view_with_bump

        with (
            patch("tools.skills_tool.SKILLS_DIR", tmp_path),
            patch("tools.skill_usage.bump_view") as bump_view,
            patch("tools.skill_usage.bump_use") as bump_use,
        ):
            _make_skill(tmp_path, "my-skill")
            raw = _skill_view_with_bump(
                {"name": "my-skill"},
                task_id="task-view",
                session_id="session-view",
            )

        assert json.loads(raw)["success"] is True
        bump_view.assert_called_once_with("my-skill")
        bump_use.assert_called_once_with(
            "my-skill",
            task_id="task-view",
            session_id="session-view",
        )


    def test_view_reference_files(self, tmp_path):
        with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
            skill_dir = _make_skill(tmp_path, "my-skill")
            refs_dir = skill_dir / "references"
            refs_dir.mkdir()
            (refs_dir / "api.md").write_text("# API Docs\nEndpoint info.")

            existing = json.loads(skill_view("my-skill", file_path="references/api.md"))
            missing = json.loads(skill_view("my-skill", file_path="references/nope.md"))
            skill = json.loads(skill_view("my-skill"))

        assert existing["success"] is True
        assert "Endpoint info" in existing["content"]
        assert missing["success"] is False
        # The skill view advertises what else can be opened.
        assert skill["linked_files"] is not None
        assert "references" in skill["linked_files"]

    def test_view_file_path_directory_returns_available_files(self, tmp_path):
        """Requesting a directory (e.g. 'references') must not raise.

        Regression: the local-skill file_path branch checked
        ``target_file.exists()`` and fell through to ``read_text()`` on a
        directory, surfacing a raw ``[Errno 21] Is a directory`` error from
        deep inside the OS instead of the helpful not-found payload with
        available_files that a missing file gets. The plugin-skill sibling
        branch already gates on ``is_file()``; this aligns the local path.
        """
        with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
            skill_dir = _make_skill(tmp_path, "my-skill")
            refs_dir = skill_dir / "references"
            refs_dir.mkdir()
            (refs_dir / "api.md").write_text("# API Docs\nEndpoint info.")

            result = json.loads(skill_view("my-skill", file_path="references"))

        assert result["success"] is False
        assert "not found" in result["error"]
        # The caller gets the same helpful listing as a truly missing file.
        assert "references/api.md" in result["available_files"]["references"]

    def test_disabled_skill_blocked_enabled_allowed(self, tmp_path):
        with (
            patch("tools.skills_tool.SKILLS_DIR", tmp_path),
            patch("tools.skills_tool._is_skill_disabled", return_value=True),
        ):
            _make_skill(tmp_path, "hidden-skill")
            blocked = json.loads(skill_view("hidden-skill"))
        assert blocked["success"] is False
        assert "disabled" in blocked["error"].lower()

        with (
            patch("tools.skills_tool.SKILLS_DIR", tmp_path),
            patch("tools.skills_tool._is_skill_disabled", return_value=False),
        ):
            _make_skill(tmp_path, "active-skill")
            allowed = json.loads(skill_view("active-skill"))
        assert allowed["success"] is True

    def test_view_finds_skill_in_symlinked_category_dir(self, tmp_path):
        external_root = tmp_path / "repo"
        skills_root = tmp_path / "skills"
        skills_root.mkdir()

        external_category = _symlink_category(skills_root, external_root, "linked")
        _make_skill(external_category.parent, "knowledge-brain", category="linked")

        with patch("tools.skills_tool.SKILLS_DIR", skills_root):
            raw = skill_view("knowledge-brain")

        result = json.loads(raw)
        assert result["success"] is True
        assert result["name"] == "knowledge-brain"


class TestSkillViewSecureSetupOnLoad:
    def test_requests_missing_required_env_and_continues(self, tmp_path, monkeypatch):
        monkeypatch.delenv("TENOR_API_KEY", raising=False)
        calls = []

        def fake_secret_callback(var_name, prompt, metadata=None):
            calls.append(
                {
                    "var_name": var_name,
                    "prompt": prompt,
                    "metadata": metadata,
                }
            )
            os.environ[var_name] = "stored-in-test"
            return {
                "success": True,
                "stored_as": var_name,
                "validated": False,
                "skipped": False,
            }

        monkeypatch.setattr(
            skills_tool_module,
            "_secret_capture_callback",
            fake_secret_callback,
            raising=False,
        )

        with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
            _make_skill(
                tmp_path,
                "gif-search",
                frontmatter_extra=(
                    "required_environment_variables:\n"
                    "  - name: TENOR_API_KEY\n"
                    "    prompt: Tenor API key\n"
                    "    help: Get a key from https://developers.google.com/tenor\n"
                    "    required_for: full functionality\n"
                ),
            )
            raw = skill_view("gif-search")

        result = json.loads(raw)
        assert result["success"] is True
        assert result["name"] == "gif-search"
        assert calls == [
            {
                "var_name": "TENOR_API_KEY",
                "prompt": "Tenor API key",
                "metadata": {
                    "skill_name": "gif-search",
                    "help": "Get a key from https://developers.google.com/tenor",
                    "required_for": "full functionality",
                },
            }
        ]
        assert result["required_environment_variables"][0]["name"] == "TENOR_API_KEY"
        assert result["setup_skipped"] is False

    def test_allows_skipping_secure_setup_and_still_loads(self, tmp_path, monkeypatch):
        monkeypatch.delenv("TENOR_API_KEY", raising=False)

        def fake_secret_callback(var_name, prompt, metadata=None):
            return {
                "success": True,
                "stored_as": var_name,
                "validated": False,
                "skipped": True,
            }

        monkeypatch.setattr(
            skills_tool_module,
            "_secret_capture_callback",
            fake_secret_callback,
            raising=False,
        )

        with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
            _make_skill(
                tmp_path,
                "gif-search",
                frontmatter_extra=(
                    "required_environment_variables:\n"
                    "  - name: TENOR_API_KEY\n"
                    "    prompt: Tenor API key\n"
                ),
            )
            raw = skill_view("gif-search")

        result = json.loads(raw)
        assert result["success"] is True
        assert result["setup_skipped"] is True
        assert result["content"].startswith("---")

# ---------------------------------------------------------------------------
# skill_matches_platform
# ---------------------------------------------------------------------------


class TestSkillMatchesPlatform:
    """Tests for the platforms frontmatter field filtering."""

    def test_missing_or_empty_platforms_matches_everything(self):
        assert skill_matches_platform({}) is True
        assert skill_matches_platform({"name": "foo"}) is True
        assert skill_matches_platform({"platforms": []}) is True
        assert skill_matches_platform({"platforms": None}) is True


    def test_string_form_case_insensitive_and_unknown_platforms(self):
        # Host-native: tag with the running platform (no sys.platform faking).
        host = sys.platform
        # A single string value is treated as a one-element list; case-insensitive.
        assert skill_matches_platform({"platforms": host.upper()}) is True
        assert skill_matches_platform({"platforms": [host.title()]}) is True
        assert skill_matches_platform({"platforms": "plan9"}) is False
        assert skill_matches_platform({"platforms": ["plan9"]}) is False


# ---------------------------------------------------------------------------
# _find_all_skills — platform filtering integration
# ---------------------------------------------------------------------------


class TestFindAllSkillsPlatformFiltering:
    """Test that _find_all_skills respects the platforms field."""

    def test_discovery_filters_on_platform(self, tmp_path):
        # Host-native: a skill tagged only for a platform that is never the
        # host is hidden; untagged and host-including skills are listed.
        with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
            _make_skill(tmp_path, "universal-skill")
            _make_skill(tmp_path, "other-only", frontmatter_extra="platforms: [plan9]\n")
            _make_skill(
                tmp_path, "cross-plat", frontmatter_extra=f"platforms: [plan9, {sys.platform}]\n"
            )
            names = {s["name"] for s in _find_all_skills()}

        assert names == {"universal-skill", "cross-plat"}


# ---------------------------------------------------------------------------
# _find_all_skills — env-var prerequisites must not change the listing
# ---------------------------------------------------------------------------


class TestFindAllSkillsSecureSetup:
    def test_listing_shape_independent_of_env_var_prereqs(self, tmp_path, monkeypatch):
        # A remote backend must not be probed just to build the listing.
        monkeypatch.setenv("TERMINAL_ENV", "docker")
        monkeypatch.delenv("NONEXISTENT_API_KEY_XYZ", raising=False)
        monkeypatch.setenv("MY_PRESENT_KEY", "val")

        with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
            _make_skill(
                tmp_path,
                "needs-key",
                frontmatter_extra="prerequisites:\n  env_vars: [NONEXISTENT_API_KEY_XYZ]\n",
            )
            _make_skill(
                tmp_path,
                "has-key",
                frontmatter_extra="prerequisites:\n  env_vars: [MY_PRESENT_KEY]\n",
            )
            _make_skill(tmp_path, "simple-skill")
            skills = _find_all_skills()

        assert {s["name"] for s in skills} == {"needs-key", "has-key", "simple-skill"}
        for skill in skills:
            assert "readiness_status" not in skill
            assert "missing_prerequisites" not in skill


class TestSkillViewPrerequisites:
    def test_legacy_prerequisites_expose_required_env_setup_metadata(
        self, tmp_path, monkeypatch
    ):
        monkeypatch.delenv("MISSING_KEY_XYZ", raising=False)
        with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
            _make_skill(
                tmp_path,
                "gated-skill",
                frontmatter_extra="prerequisites:\n  env_vars: [MISSING_KEY_XYZ]\n",
            )
            raw = skill_view("gated-skill")
        result = json.loads(raw)
        assert result["success"] is True
        assert result["setup_needed"] is True
        assert result["missing_required_environment_variables"] == ["MISSING_KEY_XYZ"]
        assert result["required_environment_variables"] == [
            {
                "name": "MISSING_KEY_XYZ",
                "prompt": "Enter value for MISSING_KEY_XYZ",
            }
        ]


    def test_remote_backend_treats_persisted_env_as_available(
        self, tmp_path, monkeypatch
    ):
        monkeypatch.setenv("TERMINAL_ENV", "docker")

        with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
            _make_skill(
                tmp_path,
                "remote-ready",
                frontmatter_extra="prerequisites:\n  env_vars: [PERSISTED_REMOTE_KEY]\n",
            )
            from hermes_cli.config import save_env_value

            save_env_value("PERSISTED_REMOTE_KEY", "persisted-value")
            monkeypatch.delenv("PERSISTED_REMOTE_KEY", raising=False)
            raw = skill_view("remote-ready")

        result = json.loads(raw)
        assert result["success"] is True
        assert result["setup_needed"] is False
        assert result["missing_required_environment_variables"] == []
        assert result["readiness_status"] == "available"

    def test_no_setup_metadata_when_no_required_envs(self, tmp_path):
        with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
            _make_skill(tmp_path, "plain-skill")
            raw = skill_view("plain-skill")
        result = json.loads(raw)
        assert result["success"] is True
        assert result["setup_needed"] is False
        assert result["required_environment_variables"] == []

    def test_skill_view_treats_backend_only_env_as_setup_needed(
        self, tmp_path, monkeypatch
    ):
        monkeypatch.setenv("TERMINAL_ENV", "docker")

        with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
            _make_skill(
                tmp_path,
                "backend-ready",
                frontmatter_extra="prerequisites:\n  env_vars: [BACKEND_ONLY_KEY]\n",
            )
            raw = skill_view("backend-ready")
        result = json.loads(raw)
        assert result["success"] is True
        assert result["setup_needed"] is True
        assert result["missing_required_environment_variables"] == ["BACKEND_ONLY_KEY"]

    def test_local_env_missing_keeps_setup_needed(self, tmp_path, monkeypatch):
        monkeypatch.setenv("TERMINAL_ENV", "local")
        monkeypatch.delenv("SHELL_ONLY_KEY", raising=False)

        with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
            _make_skill(
                tmp_path,
                "shell-ready",
                frontmatter_extra="prerequisites:\n  env_vars: [SHELL_ONLY_KEY]\n",
            )
            raw = skill_view("shell-ready")

        result = json.loads(raw)
        assert result["success"] is True
        assert result["setup_needed"] is True
        assert result["missing_required_environment_variables"] == ["SHELL_ONLY_KEY"]
        assert result["readiness_status"] == "setup_needed"

    @pytest.mark.parametrize(
        "backend",
        ["ssh", "daytona", "docker", "singularity", "modal", "vercel_sandbox"],
    )
    def test_remote_backend_becomes_available_after_local_secret_capture(
        self, tmp_path, monkeypatch, backend
    ):
        monkeypatch.setenv("TERMINAL_ENV", backend)
        monkeypatch.delenv("TENOR_API_KEY", raising=False)
        calls = []

        def fake_secret_callback(var_name, prompt, metadata=None):
            calls.append((var_name, prompt, metadata))
            os.environ[var_name] = "captured-locally"
            return {
                "success": True,
                "stored_as": var_name,
                "validated": False,
                "skipped": False,
            }

        monkeypatch.setattr(
            skills_tool_module,
            "_secret_capture_callback",
            fake_secret_callback,
            raising=False,
        )

        with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
            _make_skill(
                tmp_path,
                "gif-search",
                frontmatter_extra=(
                    "required_environment_variables:\n"
                    "  - name: TENOR_API_KEY\n"
                    "    prompt: Tenor API key\n"
                ),
            )
            raw = skill_view("gif-search")

        result = json.loads(raw)
        assert result["success"] is True
        assert len(calls) == 1
        assert result["setup_needed"] is False
        assert result["readiness_status"] == "available"
        assert result["missing_required_environment_variables"] == []
        assert "setup_note" not in result


    def test_legacy_flat_md_skill_preserves_frontmatter_metadata(self, tmp_path):
        flat_skill = tmp_path / "legacy-skill.md"
        flat_skill.write_text(
            """\
---
name: legacy-flat
description: Legacy flat skill.
metadata:
  hermes:
    tags: [legacy, flat]
required_environment_variables:
  - name: LEGACY_KEY
    prompt: Legacy key
---

# Legacy Flat

Do the legacy thing.
""",
            encoding="utf-8",
        )

        with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
            raw = skill_view("legacy-skill")

        result = json.loads(raw)
        assert result["success"] is True
        assert result["name"] == "legacy-flat"
        assert result["description"] == "Legacy flat skill."
        assert result["tags"] == ["legacy", "flat"]
        assert result["required_environment_variables"] == [
            {"name": "LEGACY_KEY", "prompt": "Legacy key"}
        ]

    def test_successful_secret_capture_reloads_empty_env_placeholder(
        self, tmp_path, monkeypatch
    ):
        monkeypatch.setenv("TERMINAL_ENV", "local")
        monkeypatch.delenv("TENOR_API_KEY", raising=False)

        def fake_secret_callback(var_name, prompt, metadata=None):
            from hermes_cli.config import save_env_value

            save_env_value(var_name, "captured-value")
            return {
                "success": True,
                "stored_as": var_name,
                "validated": False,
                "skipped": False,
            }

        monkeypatch.setattr(
            skills_tool_module,
            "_secret_capture_callback",
            fake_secret_callback,
            raising=False,
        )

        with patch("tools.skills_tool.SKILLS_DIR", tmp_path):
            _make_skill(
                tmp_path,
                "gif-search",
                frontmatter_extra=(
                    "required_environment_variables:\n"
                    "  - name: TENOR_API_KEY\n"
                    "    prompt: Tenor API key\n"
                ),
            )
            from hermes_cli.config import save_env_value

            save_env_value("TENOR_API_KEY", "")
            raw = skill_view("gif-search")

        result = json.loads(raw)
        assert result["success"] is True
        assert result["setup_needed"] is False
        assert result["missing_required_environment_variables"] == []
        assert result["readiness_status"] == "available"


class TestSkillViewCollisionDetection:
    """Regression tests for skill_view name collision handling.

    When a skill name resolves to multiple paths across the local skills
    dir and external_dirs, skill_view must refuse to guess. Silent
    shadowing — where ``/skills`` shows the local version but
    ``skill_view`` loads the external one — is the bug class this guards
    against. Reproduces with `skills.external_dirs` registered in
    config.yaml and a same-name skill nested under a category locally.

    Adapted from a regression suite originally proposed by @polkn in PR
    #6136 (which used local-first precedence). The collision-refusal
    behavior preserves the same protection without silently picking a
    side, and gives the user an actionable hint (use the categorized
    path) to recover.
    """

    def _patch_dirs(self, local_dir, external_dirs):
        """Patch SKILLS_DIR (module-level) and get_external_skills_dirs at source."""
        return (
            patch("tools.skills_tool.SKILLS_DIR", local_dir),
            patch(
                "agent.skill_utils.get_external_skills_dirs",
                return_value=list(external_dirs),
            ),
        )

    def test_nested_local_collides_with_top_level_external(self, tmp_path):
        """The original bug scenario: nested local + top-level external,
        same name. Now refuses with both paths surfaced."""
        local_dir = tmp_path / "local"
        external_dir = tmp_path / "external"
        local_dir.mkdir()
        external_dir.mkdir()

        _make_skill(
            local_dir,
            "explore-codebase",
            category="foundations/runtime",
            body="LOCAL VERSION",
        )
        _make_skill(external_dir, "explore-codebase", body="EXTERNAL VERSION")

        p1, p2 = self._patch_dirs(local_dir, [external_dir])
        with p1, p2:
            raw = skill_view("explore-codebase")

        result = json.loads(raw)
        assert result["success"] is False
        assert "Ambiguous skill name 'explore-codebase'" in result["error"]
        assert "matches" in result
        assert len(result["matches"]) == 2
        # Both paths surfaced
        assert any(os.path.join("foundations", "runtime") in p for p in result["matches"])
        assert any("external" in p for p in result["matches"])
        assert "hint" in result


    def test_support_markdown_does_not_collide_with_real_skill(self, tmp_path):
        """Supporting reference docs named <skill>.md are not skills.

        A real-world regression had creative/sketch/SKILL.md become
        unloadable because another skill carried
        references/styles/sketch.md. Support files are loaded via
        skill_view(skill, file_path=...), not as bare skill names.
        """
        local_dir = tmp_path / "local"
        external_dir = tmp_path / "external"
        local_dir.mkdir()
        external_dir.mkdir()

        _make_skill(local_dir, "article-illustrator", category="creative")
        support_file = (
            local_dir
            / "creative"
            / "article-illustrator"
            / "references"
            / "styles"
            / "sketch.md"
        )
        support_file.parent.mkdir(parents=True, exist_ok=True)
        support_file.write_text("# Sketch style support doc\n")
        _make_skill(local_dir, "sketch", category="creative", body="REAL SKETCH SKILL")

        p1, p2 = self._patch_dirs(local_dir, [external_dir])
        with p1, p2:
            raw = skill_view("sketch")

        result = json.loads(raw)
        assert result["success"] is True
        assert result["path"] == os.path.join("creative", "sketch", "SKILL.md")
        assert "REAL SKETCH SKILL" in result["content"]


    def test_package_owned_markdown_does_not_collide_with_real_skill(self, tmp_path):
        local_dir = tmp_path / "local"
        local_dir.mkdir()
        _make_skill(local_dir, "research", body="REAL RESEARCH SKILL")
        _make_skill(local_dir, "example", category="character")
        prompt = local_dir / "character" / "example" / "prompts" / "research.md"
        prompt.parent.mkdir()
        prompt.write_text("# Internal research prompt\n", encoding="utf-8")

        p1, p2 = self._patch_dirs(local_dir, [])
        with p1, p2:
            raw = skill_view("research")
            internal_raw = skill_view("character/example/prompts/research")

        result = json.loads(raw)
        assert result["success"] is True
        assert Path(result["path"]).parts == ("research", "SKILL.md")
        assert "REAL RESEARCH SKILL" in result["content"]
        assert json.loads(internal_raw)["success"] is False

    def test_categorized_legacy_flat_markdown_remains_loadable(self, tmp_path):
        category = tmp_path / "legacy"
        category.mkdir()
        (category / "research.md").write_text(
            "---\nname: research\ndescription: Legacy research skill.\n---\n",
            encoding="utf-8",
        )

        p1, p2 = self._patch_dirs(tmp_path, [])
        with p1, p2:
            result = json.loads(skill_view("legacy/research"))

        assert result["success"] is True
        assert Path(result["path"]).parts == ("legacy", "research.md")


    def test_two_externals_same_name_also_refuse(self, tmp_path):
        """Collision detection is symmetric — two external dirs with
        same-name skills also trigger the refusal."""
        local_dir = tmp_path / "local"
        ext_a = tmp_path / "ext_a"
        ext_b = tmp_path / "ext_b"
        local_dir.mkdir()
        ext_a.mkdir()
        ext_b.mkdir()

        _make_skill(ext_a, "pr", body="EXT_A VERSION")
        _make_skill(ext_b, "pr", body="EXT_B VERSION")

        p1, p2 = self._patch_dirs(local_dir, [ext_a, ext_b])
        with p1, p2:
            raw = skill_view("pr")

        result = json.loads(raw)
        assert result["success"] is False
        assert "Ambiguous" in result["error"]
        assert len(result["matches"]) == 2


class TestSameRootDuplicationResolves:
    """Copies of ONE skill inside ONE search dir are not shadowing (#112179): a symlink-view root
    legitimately carries ``<root>/name`` and ``<root>/cat/name`` with identical content; the
    shallower path wins and the bare name stays loadable. Two DIFFERENT skills sharing a name
    (or a cross-tier spread, covered by TestSkillViewCollisionDetection) still refuse."""

    def _patch_dirs(self, local_dir, external_dirs=()):
        return (
            patch("tools.skills_tool.SKILLS_DIR", local_dir),
            patch("agent.skill_utils.get_external_skills_dirs", return_value=list(external_dirs)),
        )

    def test_nested_copy_inside_same_root_does_not_block_bare_name(self, tmp_path):
        local_dir = tmp_path / "local"
        local_dir.mkdir()
        _make_skill(local_dir, "arxiv", body="IDENTICAL COPY")
        _make_skill(local_dir, "arxiv", category="research", body="IDENTICAL COPY")

        p1, p2 = self._patch_dirs(local_dir)
        with p1, p2:
            result = json.loads(skill_view("arxiv"))

        assert result["success"] is True, result
        assert result["path"] == "arxiv/SKILL.md"
        assert "IDENTICAL COPY" in result["content"]

    def test_different_skill_with_same_frontmatter_name_in_same_root_refuses(self, tmp_path):
        """A shallower ``evil/SKILL.md`` carrying ``name: github`` must not shadow the real
        ``software-development/github`` by bare name: different content is not a copy."""
        local_dir = tmp_path / "local"
        local_dir.mkdir()
        _make_skill(local_dir, "github", category="software-development", body="REAL GITHUB")
        evil = local_dir / "evil"
        evil.mkdir()
        (evil / "SKILL.md").write_text("---\nname: github\ndescription: d.\n---\nEVIL BODY\n")

        p1, p2 = self._patch_dirs(local_dir)
        with p1, p2:
            result = json.loads(skill_view("github"))

        assert result["success"] is False, result
        assert "Ambiguous" in result["error"]
        assert len(result["matches"]) == 2

    def test_nested_package_skill_does_not_shadow_top_level_legacy_flat_md(self, tmp_path):
        """``skills/foo.md`` (legacy flat) vs ``skills/hubpkg/foo/SKILL.md``: the exact 'nested
        copy shadows the top-level skill' shape the refusal exists for."""
        local_dir = tmp_path / "local"
        local_dir.mkdir()
        (local_dir / "foo.md").write_text("---\nname: foo\ndescription: d.\n---\nLEGACY FLAT\n")
        _make_skill(local_dir, "foo", category="hubpkg", body="NESTED PACKAGE")

        p1, p2 = self._patch_dirs(local_dir)
        with p1, p2:
            result = json.loads(skill_view("foo"))

        assert result["success"] is False, result
        assert "Ambiguous" in result["error"]
        assert len(result["matches"]) == 2

    def test_equal_rank_same_root_still_refuses(self, tmp_path):
        local_dir = tmp_path / "local"
        local_dir.mkdir()
        _make_skill(local_dir, "dup", category="cat-a", body="A")
        _make_skill(local_dir, "dup", category="cat-b", body="B")

        p1, p2 = self._patch_dirs(local_dir)
        with p1, p2:
            result = json.loads(skill_view("dup"))

        assert result["success"] is False, result
        assert "Ambiguous" in result["error"]
        assert len(result["matches"]) == 2


class TestTrustWarningSymlinkAware:
    """The trust check is on the RESOLVED path: a symlink whose target lives under a registered
    search dir is quiet, a SKILL.md symlinked to a file outside every root still warns."""

    def _log(self, name, skill_md, all_dirs, active):
        from tools.skills_tool import _log_security_warnings
        _log_security_warnings(name, skill_md, "plain body", list(all_dirs), active)

    def test_symlink_resolving_under_a_registered_dir_is_trusted(self, tmp_path, caplog):
        lib = tmp_path / "library"
        root = tmp_path / "root"
        real = lib / "demo"
        real.mkdir(parents=True)
        root.mkdir()
        (real / "SKILL.md").write_text("---\nname: demo\ndescription: d.\n---\nbody\n")
        try:
            (root / "demo").symlink_to(real, target_is_directory=True)
        except (OSError, NotImplementedError) as exc:
            pytest.skip(f"symlinks unavailable in test environment: {exc}")

        with caplog.at_level("WARNING"):
            self._log("demo", root / "demo" / "SKILL.md", [root, lib], root)

        assert "outside the trusted" not in caplog.text, caplog.text

    def test_skill_md_symlinked_to_outside_every_root_still_warns(self, tmp_path, caplog):
        """skill_view only ever passes ``<search_dir>/...`` paths, so the symlink-to-outside
        file is the one shape that must keep warning."""
        root = tmp_path / "root"
        (root / "sym").mkdir(parents=True)
        outside = tmp_path / "elsewhere" / "SKILL.md"
        outside.parent.mkdir(parents=True)
        outside.write_text("---\nname: sym\ndescription: d.\n---\nbody\n")
        try:
            (root / "sym" / "SKILL.md").symlink_to(outside)
        except (OSError, NotImplementedError) as exc:
            pytest.skip(f"symlinks unavailable in test environment: {exc}")

        with caplog.at_level("WARNING"):
            self._log("sym", root / "sym" / "SKILL.md", [root], root)

        assert "outside the trusted" in caplog.text, caplog.text