feat(skills): add Agent Skills support with orchestrator-driven activation

cli/test/test_skills.py

@@ -0,0 +1,394 @@
+"""Tests for cli/planoai/skills.py and the config-rendering hooks that
+materialize SKILL.md bodies into the rendered plano config.
+"""
+
+from __future__ import annotations
+
+import json
+import os
+from pathlib import Path
+from unittest import mock
+
+import pytest
+
+from planoai.skills import (
+    AGENTS_SKILLS_DIR,
+    PROJECT_SKILLS_DIR,
+    USER_SKILLS_DIR,
+    Skill,
+    discover_skills,
+    parse_skill_md,
+    total_catalog_size,
+)
+
+
+@pytest.fixture(autouse=True)
+def _isolate_user_scopes(tmp_path, monkeypatch):
+    """Default both user-tier scopes to non-existent dirs so the dev's real
+    ~/.plano/skills and ~/.agents/skills cannot bleed into tests.
+    """
+    monkeypatch.setattr("planoai.skills.USER_SKILLS_DIR", tmp_path / "_no_user_skills")
+    monkeypatch.setattr(
+        "planoai.skills.AGENTS_SKILLS_DIR", tmp_path / "_no_agents_skills"
+    )
+
+
+def _write_skill(
+    base: Path,
+    name: str,
+    description: str = "Process PDFs. Use when handling PDF files.",
+    body: str = "# Body\n\nDo the thing.",
+    extra_frontmatter: str = "",
+) -> Path:
+    skill_dir = base / name
+    skill_dir.mkdir(parents=True, exist_ok=True)
+    frontmatter = f"name: {name}\ndescription: {description}\n{extra_frontmatter}"
+    (skill_dir / "SKILL.md").write_text(
+        f"---\n{frontmatter}---\n\n{body}",
+        encoding="utf-8",
+    )
+    return skill_dir / "SKILL.md"
+
+
+def test_parse_skill_md_minimal(tmp_path):
+    skill_md = _write_skill(tmp_path, "pdf-processing")
+
+    skill, diagnostics = parse_skill_md(skill_md)
+
+    assert skill is not None
+    assert skill.name == "pdf-processing"
+    assert skill.description.startswith("Process PDFs")
+    assert "Do the thing." in skill.body
+    assert skill.location == skill_md.resolve()
+    assert skill.base_dir == skill_md.parent.resolve()
+    # No warnings or errors for a well-formed skill.
+    assert diagnostics == []
+
+
+def test_parse_skill_md_lenient_when_name_mismatches_directory(tmp_path):
+    skill_dir = tmp_path / "some-dir"
+    skill_dir.mkdir()
+    (skill_dir / "SKILL.md").write_text(
+        "---\nname: different-name\ndescription: ok\n---\n\nbody",
+        encoding="utf-8",
+    )
+
+    skill, diagnostics = parse_skill_md(skill_dir / "SKILL.md")
+
+    assert skill is not None
+    assert skill.name == "different-name"
+    assert any(
+        "does not match parent directory" in d.message for d in diagnostics
+    ), diagnostics
+
+
+def test_parse_skill_md_warns_on_invalid_name(tmp_path):
+    skill_dir = tmp_path / "Bad-Name"
+    skill_dir.mkdir()
+    (skill_dir / "SKILL.md").write_text(
+        "---\nname: Bad-Name\ndescription: ok\n---\n\nbody",
+        encoding="utf-8",
+    )
+
+    skill, diagnostics = parse_skill_md(skill_dir / "SKILL.md")
+    assert skill is not None
+    assert any("violates spec naming rules" in d.message for d in diagnostics)
+
+
+def test_parse_skill_md_recovers_from_unquoted_colons(tmp_path):
+    skill_dir = tmp_path / "code-review"
+    skill_dir.mkdir()
+    (skill_dir / "SKILL.md").write_text(
+        "---\nname: code-review\n"
+        "description: Use this skill when: the user asks about code review\n"
+        "---\n\nbody",
+        encoding="utf-8",
+    )
+
+    skill, diagnostics = parse_skill_md(skill_dir / "SKILL.md")
+    # Lenient parse retries with quoted values and succeeds.
+    assert skill is not None
+    assert skill.name == "code-review"
+    assert "Use this skill when:" in skill.description
+
+
+def test_parse_skill_md_rejects_when_description_missing(tmp_path):
+    skill_dir = tmp_path / "broken"
+    skill_dir.mkdir()
+    (skill_dir / "SKILL.md").write_text(
+        "---\nname: broken\n---\n\nbody",
+        encoding="utf-8",
+    )
+
+    skill, diagnostics = parse_skill_md(skill_dir / "SKILL.md")
+    assert skill is None
+    assert any(d.severity == "error" for d in diagnostics)
+
+
+def test_parse_skill_md_rejects_when_frontmatter_missing(tmp_path):
+    skill_dir = tmp_path / "no-frontmatter"
+    skill_dir.mkdir()
+    (skill_dir / "SKILL.md").write_text("just markdown", encoding="utf-8")
+
+    skill, diagnostics = parse_skill_md(skill_dir / "SKILL.md")
+    assert skill is None
+    assert any("frontmatter" in d.message for d in diagnostics)
+
+
+def test_discover_skills_project_only(tmp_path, monkeypatch):
+    (tmp_path / ".plano").mkdir()
+    project_skills_dir = tmp_path / ".plano" / "skills"
+    project_skills_dir.mkdir()
+    _write_skill(project_skills_dir, "pdf-processing")
+    _write_skill(project_skills_dir, "code-review")
+
+    skills, diagnostics = discover_skills(
+        project_root=tmp_path, include_user_scope=False
+    )
+
+    names = sorted(s.name for s in skills)
+    assert names == ["code-review", "pdf-processing"]
+    assert all(s.scope == "project" for s in skills)
+
+
+def test_discover_skills_picks_up_agents_scope(tmp_path, monkeypatch):
+    """`npx skills add` writes into ~/.agents/skills/<name>. That directory
+    must be discovered as a user-tier (auto-trusted) scope so the upstream
+    CLI works without Plano-specific awareness.
+    """
+    agents_dir = tmp_path / "fake-home" / ".agents" / "skills"
+    agents_dir.mkdir(parents=True)
+    _write_skill(agents_dir, "pdf", description="agents-scope description")
+
+    monkeypatch.setattr("planoai.skills.AGENTS_SKILLS_DIR", agents_dir)
+
+    (tmp_path / ".plano").mkdir()
+    (tmp_path / ".plano" / "skills").mkdir()
+
+    skills, _ = discover_skills(project_root=tmp_path, include_user_scope=True)
+    by_name = {s.name: s for s in skills}
+    assert "pdf" in by_name
+    assert by_name["pdf"].scope == "agents"
+    assert by_name["pdf"].description == "agents-scope description"
+
+
+def test_discover_skills_user_scope_shadows_agents_scope(tmp_path, monkeypatch):
+    """When the same skill name exists in both ~/.plano/skills and
+    ~/.agents/skills, the Plano-native one wins and a diagnostic is emitted.
+
+    Project root lives in its own subtree (with no .plano/ ancestor) so it
+    cannot collide with the patched user-scope dir.
+    """
+    home = tmp_path / "fake-home"
+    home.mkdir()
+    agents_dir = home / ".agents" / "skills"
+    agents_dir.mkdir(parents=True)
+    _write_skill(agents_dir, "pdf", description="agents copy")
+    monkeypatch.setattr("planoai.skills.AGENTS_SKILLS_DIR", agents_dir)
+
+    user_dir = home / ".plano" / "skills"
+    user_dir.mkdir(parents=True)
+    _write_skill(user_dir, "pdf", description="user copy")
+    monkeypatch.setattr("planoai.skills.USER_SKILLS_DIR", user_dir)
+
+    project_root = tmp_path / "elsewhere" / "proj"
+    project_root.mkdir(parents=True)
+    skills, diagnostics = discover_skills(
+        project_root=project_root, include_user_scope=True
+    )
+    by_name = {s.name: s for s in skills}
+    assert by_name["pdf"].scope == "user"
+    assert by_name["pdf"].description == "user copy"
+    assert any("shadows ~/.agents/skills" in d.message for d in diagnostics)
+
+
+def test_discover_skills_project_overrides_user_scope(tmp_path, monkeypatch):
+    user_skills_dir = tmp_path / "fake-home" / ".plano" / "skills"
+    user_skills_dir.mkdir(parents=True)
+    _write_skill(user_skills_dir, "shared", description="user-scope description")
+
+    monkeypatch.setattr("planoai.skills.USER_SKILLS_DIR", user_skills_dir)
+
+    (tmp_path / ".plano").mkdir()
+    project_skills_dir = tmp_path / ".plano" / "skills"
+    project_skills_dir.mkdir()
+    _write_skill(project_skills_dir, "shared", description="project-scope description")
+    _write_skill(project_skills_dir, "only-project")
+
+    skills, diagnostics = discover_skills(
+        project_root=tmp_path, include_user_scope=True
+    )
+
+    by_name = {s.name: s for s in skills}
+    assert by_name["shared"].scope == "project"
+    assert by_name["shared"].description == "project-scope description"
+    assert by_name["only-project"].scope == "project"
+    assert any("shadows user-scope skill" in d.message for d in diagnostics)
+
+
+def test_total_catalog_size_counts_name_and_description():
+    skills = [
+        Skill(
+            name="a",
+            description="d1",
+            location=Path("/x"),
+            base_dir=Path("/"),
+            body="b",
+            scope="project",
+        ),
+        Skill(
+            name="bb",
+            description="dd",
+            location=Path("/y"),
+            base_dir=Path("/"),
+            body="b",
+            scope="project",
+        ),
+    ]
+    assert total_catalog_size(skills) == (1 + 2) + (2 + 2)
+
+
+def test_materialize_skills_in_config_default_inlines_bodies(tmp_path, monkeypatch):
+    project_root = tmp_path
+    (project_root / ".plano").mkdir()
+    project_skills_dir = project_root / ".plano" / "skills"
+    project_skills_dir.mkdir()
+    _write_skill(
+        project_skills_dir,
+        "pdf-processing",
+        description="Process PDFs.",
+        body="# Body\nfollow these steps.",
+    )
+
+    fake_home = tmp_path / "home"
+    fake_home.mkdir()
+    monkeypatch.setenv("HOME", str(fake_home))
+    # Mark the project trusted so .plano/skills is loaded.
+    trusted = fake_home / ".plano" / "trusted_projects.json"
+    trusted.parent.mkdir(parents=True, exist_ok=True)
+    trusted.write_text(
+        json.dumps({"trusted_projects": [str(project_root.resolve())]}),
+        encoding="utf-8",
+    )
+    monkeypatch.setattr(
+        "planoai.skills.USER_SKILLS_DIR",
+        fake_home / ".plano" / "skills",
+    )
+
+    from planoai.config_generator import materialize_skills_in_config
+
+    config_yaml = {"version": "v0.4.0"}
+    materialize_skills_in_config(config_yaml, project_root)
+
+    assert "skills" in config_yaml
+    materialized = config_yaml["skills"]
+    assert len(materialized) == 1
+    entry = materialized[0]
+    assert entry["name"] == "pdf-processing"
+    assert "follow these steps." in entry["body"]
+    assert entry["scope"] == "project"
+
+
+def test_materialize_skills_in_config_skips_untrusted_project_skills(
+    tmp_path, monkeypatch
+):
+    project_root = tmp_path
+    (project_root / ".plano").mkdir()
+    project_skills_dir = project_root / ".plano" / "skills"
+    project_skills_dir.mkdir()
+    _write_skill(project_skills_dir, "pdf-processing")
+
+    fake_home = tmp_path / "home"
+    fake_home.mkdir()
+    monkeypatch.setenv("HOME", str(fake_home))
+    monkeypatch.setattr(
+        "planoai.skills.USER_SKILLS_DIR",
+        fake_home / ".plano" / "skills",
+    )
+
+    from planoai.config_generator import materialize_skills_in_config
+
+    config_yaml = {"version": "v0.4.0"}
+    materialize_skills_in_config(config_yaml, project_root)
+    # Untrusted -> project skills are not loaded.
+    assert "skills" not in config_yaml
+
+
+def test_materialize_skills_in_config_loads_agents_scope_without_trust(
+    tmp_path, monkeypatch
+):
+    """Even with no project trust, skills installed by `npx skills add` into
+    ~/.agents/skills/<name> must materialize into the rendered config — that
+    directory is the universal Agent Skills install location and is
+    user-tier, not project-tier.
+    """
+    project_root = tmp_path / "proj"
+    (project_root / ".plano").mkdir(parents=True)
+
+    fake_home = tmp_path / "home"
+    fake_home.mkdir()
+    monkeypatch.setenv("HOME", str(fake_home))
+    monkeypatch.setattr(
+        "planoai.skills.USER_SKILLS_DIR",
+        fake_home / ".plano" / "skills",
+    )
+
+    agents_dir = fake_home / ".agents" / "skills"
+    agents_dir.mkdir(parents=True)
+    _write_skill(agents_dir, "pdf", body="# Body\nhandle the pdf.")
+    monkeypatch.setattr("planoai.skills.AGENTS_SKILLS_DIR", agents_dir)
+
+    from planoai.config_generator import materialize_skills_in_config
+
+    config_yaml = {"version": "v0.4.0"}
+    materialize_skills_in_config(config_yaml, project_root)
+
+    assert "skills" in config_yaml
+    materialized = config_yaml["skills"]
+    assert len(materialized) == 1
+    assert materialized[0]["name"] == "pdf"
+    assert materialized[0]["scope"] == "agents"
+    assert "handle the pdf." in materialized[0]["body"]
+
+
+def test_materialize_skills_in_config_respects_allow_list(tmp_path, monkeypatch):
+    project_root = tmp_path
+    (project_root / ".plano").mkdir()
+    skills_dir = project_root / ".plano" / "skills"
+    skills_dir.mkdir()
+    _write_skill(skills_dir, "skill-a")
+    _write_skill(skills_dir, "skill-b")
+
+    fake_home = tmp_path / "home"
+    fake_home.mkdir()
+    monkeypatch.setenv("HOME", str(fake_home))
+    trusted = fake_home / ".plano" / "trusted_projects.json"
+    trusted.parent.mkdir(parents=True, exist_ok=True)
+    trusted.write_text(
+        json.dumps({"trusted_projects": [str(project_root.resolve())]}),
+        encoding="utf-8",
+    )
+    monkeypatch.setattr(
+        "planoai.skills.USER_SKILLS_DIR",
+        fake_home / ".plano" / "skills",
+    )
+
+    from planoai.config_generator import materialize_skills_in_config
+
+    config_yaml = {
+        "version": "v0.4.0",
+        "skills": ["skill-a"],
+        "routing_preferences": [
+            {
+                "name": "demo route",
+                "description": "demo",
+                "models": ["openai/gpt-4o"],
+                "skills": ["skill-a", "does-not-exist"],
+            }
+        ],
+    }
+    materialize_skills_in_config(config_yaml, project_root)
+
+    assert [s["name"] for s in config_yaml["skills"]] == ["skill-a"]
+    # Unknown allow-list entries are pruned but the known one is kept.
+    assert config_yaml["routing_preferences"][0]["skills"] == ["skill-a"]

cli/test/test_skills_cmd.py

@@ -0,0 +1,235 @@
+"""CLI tests for the `planoai skills` command group."""
+
+from __future__ import annotations
+
+import json
+import os
+from pathlib import Path
+from unittest import mock
+
+import pytest
+from click.testing import CliRunner
+
+from planoai.skills_cmd import skills
+
+
+def _seed_project(tmp_path: Path) -> Path:
+    """Create a project that find_project_root will pick up via .plano/."""
+    project = tmp_path / "project"
+    project.mkdir()
+    (project / ".plano").mkdir()
+    return project
+
+
+@pytest.fixture(autouse=True)
+def _isolate_user_scopes(tmp_path, monkeypatch):
+    """Default both user-tier scopes to non-existent dirs so the dev's real
+    ~/.plano/skills and ~/.agents/skills cannot bleed into the test sandbox.
+    Individual tests can override these via further monkeypatching.
+    """
+    monkeypatch.setattr("planoai.skills.USER_SKILLS_DIR", tmp_path / "no-user-skills")
+    monkeypatch.setattr(
+        "planoai.skills.AGENTS_SKILLS_DIR", tmp_path / "no-agents-skills"
+    )
+
+
+def _write_skill(base: Path, name: str, description: str = "demo skill") -> None:
+    skill_dir = base / name
+    skill_dir.mkdir(parents=True, exist_ok=True)
+    (skill_dir / "SKILL.md").write_text(
+        f"---\nname: {name}\ndescription: {description}\n---\n\nbody",
+        encoding="utf-8",
+    )
+
+
+def test_list_empty(tmp_path, monkeypatch):
+    project = _seed_project(tmp_path)
+    monkeypatch.chdir(project)
+    # Isolate user-scope skills dir.
+    monkeypatch.setattr("planoai.skills.USER_SKILLS_DIR", tmp_path / "no-such-home")
+
+    runner = CliRunner()
+    result = runner.invoke(skills, ["list"])
+
+    assert result.exit_code == 0, result.output
+    assert "No skills installed" in result.output
+
+
+def test_list_shows_project_skills(tmp_path, monkeypatch):
+    project = _seed_project(tmp_path)
+    monkeypatch.chdir(project)
+    monkeypatch.setattr("planoai.skills.USER_SKILLS_DIR", tmp_path / "no-such-home")
+
+    _write_skill(project / ".plano" / "skills", "pdf-processing")
+    _write_skill(project / ".plano" / "skills", "code-review")
+
+    runner = CliRunner()
+    result = runner.invoke(skills, ["list", "--no-user-scope"])
+
+    assert result.exit_code == 0, result.output
+    assert "pdf-processing" in result.output
+    assert "code-review" in result.output
+
+
+def test_remove_deletes_skill_dir(tmp_path, monkeypatch):
+    project = _seed_project(tmp_path)
+    monkeypatch.chdir(project)
+    monkeypatch.setattr("planoai.skills.USER_SKILLS_DIR", tmp_path / "no-such-home")
+
+    skills_dir = project / ".plano" / "skills"
+    _write_skill(skills_dir, "pdf-processing")
+    (skills_dir / ".skills.json").write_text(
+        json.dumps(
+            {
+                "skills": {
+                    "pdf-processing": {
+                        "source": "git",
+                        "repo": "owner/pdf-processing",
+                    }
+                }
+            }
+        ),
+        encoding="utf-8",
+    )
+
+    runner = CliRunner()
+    result = runner.invoke(skills, ["remove", "pdf-processing"])
+
+    assert result.exit_code == 0, result.output
+    assert not (skills_dir / "pdf-processing").exists()
+    manifest = json.loads((skills_dir / ".skills.json").read_text(encoding="utf-8"))
+    assert "pdf-processing" not in manifest["skills"]
+
+
+def test_remove_unknown_skill_errors(tmp_path, monkeypatch):
+    project = _seed_project(tmp_path)
+    monkeypatch.chdir(project)
+    monkeypatch.setattr("planoai.skills.USER_SKILLS_DIR", tmp_path / "no-such-home")
+    (project / ".plano" / "skills").mkdir()
+
+    runner = CliRunner()
+    result = runner.invoke(skills, ["remove", "nope"])
+    assert result.exit_code != 0
+
+
+def test_add_falls_back_to_git_when_no_npx(tmp_path, monkeypatch):
+    project = _seed_project(tmp_path)
+    monkeypatch.chdir(project)
+
+    # Force the npx branch off and stub git clone to create a SKILL.md.
+    monkeypatch.setattr("planoai.skills_cmd._has_npx", lambda: False)
+    monkeypatch.setattr("planoai.skills_cmd._has_git", lambda: True)
+
+    def fake_subprocess_run(cmd, **kwargs):
+        # cmd is like ["git", "clone", ..., url, dest]
+        dest = Path(cmd[-1])
+        dest.mkdir(parents=True, exist_ok=True)
+        (dest / "SKILL.md").write_text(
+            "---\nname: my-skill\ndescription: example\n---\n\nbody",
+            encoding="utf-8",
+        )
+        (dest / ".git").mkdir()
+        return mock.Mock(returncode=0)
+
+    monkeypatch.setattr("planoai.skills_cmd.subprocess.run", fake_subprocess_run)
+
+    runner = CliRunner()
+    result = runner.invoke(skills, ["add", "owner/my-skill"])
+    assert result.exit_code == 0, result.output
+    assert (project / ".plano" / "skills" / "my-skill" / "SKILL.md").exists()
+    # Trust hint should be shown for untrusted projects with project-scope installs.
+    assert "planoai skills trust" in result.output
+
+
+def test_add_discovers_skill_installed_into_agents_scope_by_npx(tmp_path, monkeypatch):
+    """`npx skills add` writes to ~/.agents/skills/<name>; planoai must
+    pick it up from that universal scope and *not* nag about trust.
+    """
+    project = _seed_project(tmp_path)
+    monkeypatch.chdir(project)
+
+    agents_dir = tmp_path / "agents" / "skills"
+    agents_dir.mkdir(parents=True)
+    monkeypatch.setattr("planoai.skills.AGENTS_SKILLS_DIR", agents_dir)
+
+    # Pretend npx is on $PATH and succeeds, dropping the skill in ~/.agents/skills
+    # rather than .plano/skills (which is what the upstream CLI actually does).
+    monkeypatch.setattr("planoai.skills_cmd._has_npx", lambda: True)
+
+    def fake_install_via_npx(target, project_root, console):
+        skill_dir = agents_dir / "pdf"
+        skill_dir.mkdir(parents=True, exist_ok=True)
+        (skill_dir / "SKILL.md").write_text(
+            "---\nname: pdf\ndescription: process pdfs\n---\n\nbody",
+            encoding="utf-8",
+        )
+        return True
+
+    monkeypatch.setattr("planoai.skills_cmd._install_via_npx", fake_install_via_npx)
+
+    runner = CliRunner()
+    result = runner.invoke(skills, ["add", "openai/skills"])
+
+    assert result.exit_code == 0, result.output
+    assert "scope=agents" in result.output
+    assert "planoai skills trust" not in result.output
+
+
+def test_list_includes_agents_scope_entries(tmp_path, monkeypatch):
+    project = _seed_project(tmp_path)
+    monkeypatch.chdir(project)
+
+    agents_dir = tmp_path / "agents-skills"
+    agents_dir.mkdir()
+    monkeypatch.setattr("planoai.skills.AGENTS_SKILLS_DIR", agents_dir)
+    _write_skill(agents_dir, "pdf")
+
+    runner = CliRunner()
+    result = runner.invoke(skills, ["list"])
+
+    assert result.exit_code == 0, result.output
+    assert "pdf" in result.output
+    assert "agents" in result.output
+
+
+def test_remove_rejects_agents_scope_skill(tmp_path, monkeypatch):
+    project = _seed_project(tmp_path)
+    monkeypatch.chdir(project)
+    (project / ".plano" / "skills").mkdir()
+
+    agents_dir = tmp_path / "agents-skills"
+    agents_dir.mkdir()
+    monkeypatch.setattr("planoai.skills.AGENTS_SKILLS_DIR", agents_dir)
+    _write_skill(agents_dir, "pdf")
+
+    runner = CliRunner()
+    result = runner.invoke(skills, ["remove", "pdf"])
+
+    assert result.exit_code != 0
+    assert "npx skills remove" in result.output
+
+
+def test_trust_marks_project(tmp_path, monkeypatch):
+    project = _seed_project(tmp_path)
+    monkeypatch.chdir(project)
+
+    fake_home = tmp_path / "home"
+    fake_home.mkdir()
+    monkeypatch.setenv("HOME", str(fake_home))
+
+    runner = CliRunner()
+    result = runner.invoke(skills, ["trust"])
+
+    assert result.exit_code == 0, result.output
+    trusted_file = fake_home / ".plano" / "trusted_projects.json"
+    assert trusted_file.exists()
+    data = json.loads(trusted_file.read_text(encoding="utf-8"))
+    assert str(project.resolve()) in data["trusted_projects"]
+
+
+def test_add_rejects_invalid_target(tmp_path, monkeypatch):
+    project = _seed_project(tmp_path)
+    monkeypatch.chdir(project)
+    runner = CliRunner()
+    result = runner.invoke(skills, ["add", "not-a-spec"])
+    assert result.exit_code != 0