diff --git a/README.md b/README.md index 5ddce8210..ab0b642ff 100644 --- a/README.md +++ b/README.md @@ -166,7 +166,8 @@ the scores. `novelty` refuses a self-judge; `arms run` does not check, so name - **Competitors** are the other skills an agent sees next to this one when it picks a skill to load. For a catalog skill they are its siblings in the same catalog: `--catalog ` takes every other skill under it (a `template/` copy and a - byte-identical mirror that loads under the same skill name are left out), and + byte-identical mirror that loads under the same skill name are left out, and vendored and + VCS directories such as `.git`, `node_modules`, `site-packages` and virtualenvs are not walked), and `--competitors skill: …` names them one by one. Routing asks whether this skill wins the user requests meant for it. If you only have the one skill, skip step 2 and say so in the review: a number measured against diff --git a/dokimi/changelog.d/20261024_130000_napetrov_discover-skills-skip-vendored.md b/dokimi/changelog.d/20261024_130000_napetrov_discover-skills-skip-vendored.md new file mode 100644 index 000000000..6fe816c74 --- /dev/null +++ b/dokimi/changelog.d/20261024_130000_napetrov_discover-skills-skip-vendored.md @@ -0,0 +1,3 @@ +### Fixed + +- **Vendored skills no longer load** — skill discovery (the `skill-catalog:` arm and the `catalog` commands) does not walk `.git`, `node_modules`, `site-packages`, `.venv`/`venv`, `.tox`/`.nox`, `.hg`/`.svn` or `__pycache__` below the catalog root, so a dependency's copy of a skill is no longer offered as a skill of the catalog. diff --git a/dokimi/docs/README.md b/dokimi/docs/README.md index 2f1c302e3..c2f3fdd60 100644 --- a/dokimi/docs/README.md +++ b/dokimi/docs/README.md @@ -37,7 +37,7 @@ flowchart LR | `skill:` | a skill (`SKILL.md`) injected as context | | `profile:` | an agent persona system prompt | | `agent:` / `skill-agent:` | **agentic** use — the model gets a tool and *decides* whether to fetch docs / load the skill | -| `skill-catalog:` | **selection** — every skill in a catalog is offered (a copy under a `template/` or `templates/` directory and a byte-identical mirror that loads under the same skill name are left out); scored on which one loads (`catalog trigger-set` builds the queries) | +| `skill-catalog:` | **selection** — every skill in a catalog is offered (a copy under a `template/` or `templates/` directory, a byte-identical mirror that loads under the same skill name, and anything under a vendored or VCS directory such as `.git`, `node_modules` or `.venv` are left out); scored on which one loads (`catalog trigger-set` builds the queries) | Two fine-print notes: diff --git a/dokimi/docs/evaluating-treatments.md b/dokimi/docs/evaluating-treatments.md index 493bd782f..de7824634 100644 --- a/dokimi/docs/evaluating-treatments.md +++ b/dokimi/docs/evaluating-treatments.md @@ -30,7 +30,7 @@ measure the marginal value of any context-augmentation artifact: | `skill:` | agent skill | injects a `SKILL.md` as context | | `agent[:source]` | **agentic** doc use | gives the model a doc-search tool; it decides when to call it | | `skill-agent:` | **agentic** skill use | offers the skill via progressive disclosure | -| `skill-catalog:` | **agentic** skill selection | offers every skill under `` (a `template/` copy and a byte-identical mirror that loads under the same skill name are left out); scores the one loaded against `expected_skill` | +| `skill-catalog:` | **agentic** skill selection | offers every skill under `` (a `template/` copy, a byte-identical mirror that loads under the same skill name, and anything under a vendored or VCS directory such as `.git` or `node_modules` are left out); scores the one loaded against `expected_skill` | | `+[+...]` | **combined** treatment | merges multiple treatments into one arm (e.g., `docs+skill:path`) | The last two run through a tool-calling loop @@ -290,7 +290,9 @@ artifact per cell, a `matrix_rollup.v1` JSON/Markdown report, and paired - **`skill-catalog:`** — skill selection. Every skill under `` is offered the same way, so the question is answered among its rivals. A `SKILL.md` under a `template/` or `templates/` directory, at any depth below ``, is not a skill and is - skipped (a real skill kept under such a directory is skipped too), and a `SKILL.md` + skipped (a real skill kept under such a directory is skipped too). Vendored and VCS + directories (`.git`, `.hg`, `.svn`, `node_modules`, `site-packages`, `.venv`, `venv`, + `.tox`, `.nox`, `__pycache__`) below `` are not walked, and no note says so. A `SKILL.md` byte-identical to another under `` that loads under the same skill name (a plugin's copy of `skills/`) is offered once. Questions that carry `expected_skill` are scored on the skill loaded first; see diff --git a/dokimi/docs/external-content.md b/dokimi/docs/external-content.md index 78302a4fa..ba7929a61 100644 --- a/dokimi/docs/external-content.md +++ b/dokimi/docs/external-content.md @@ -51,7 +51,8 @@ way the readers expect. Substitute your own checkout path. Both manifests are optional — with no manifest, the readers fall back to a plain filesystem scan of the same directories (a `SKILL.md` under a `template/` -or `templates/` directory is a scaffold and is left out). `skills.yaml` comes in two +or `templates/` directory is a scaffold and is left out; vendored and VCS directories such +as `.git`, `node_modules`, `site-packages` and `.venv` are not walked). `skills.yaml` comes in two shapes, and both hold a *list* of entries: ```yaml diff --git a/dokimi/docs/skill-novelty-walkthrough.md b/dokimi/docs/skill-novelty-walkthrough.md index 685e2d644..89644dffb 100644 --- a/dokimi/docs/skill-novelty-walkthrough.md +++ b/dokimi/docs/skill-novelty-walkthrough.md @@ -42,7 +42,9 @@ python cli.py novelty routing --artifact skill:skills/my-skill \ The competitors are the skills a harness lists next to this one: `--catalog ` takes every other `SKILL.md` under the directory (a `template/` copy and a byte-identical mirror -that loads under the same skill name are left out), and `--competitors` adds or names them one by +that loads under the same skill name are left out, and vendored and VCS directories such as +`.git`, `node_modules`, `site-packages` and virtualenvs are not walked), and `--competitors` +adds or names them one by one. With no real siblings at hand, skip this step and say so; a field of competitors made up for the run measures nothing. diff --git a/dokimi/docs/skill-novelty.md b/dokimi/docs/skill-novelty.md index 3788ed974..0ad1ba7ae 100644 --- a/dokimi/docs/skill-novelty.md +++ b/dokimi/docs/skill-novelty.md @@ -602,7 +602,8 @@ python cli.py novelty routing --artifact skill:path/to/skill \ `--catalog ` takes the competitors from a skill directory instead: every `SKILL.md` under it except the artifact itself (a `template/` copy and a byte-identical mirror that -loads under the same skill name are left out). +loads under the same skill name are left out, and vendored and VCS directories such as `.git`, +`node_modules`, `site-packages` and virtualenvs are not walked). Two numbers, reported side by side and never merged: diff --git a/dokimi/dokimi/commands/novelty.py b/dokimi/dokimi/commands/novelty.py index 1ed02cae4..c07bbf9f6 100644 --- a/dokimi/dokimi/commands/novelty.py +++ b/dokimi/dokimi/commands/novelty.py @@ -656,11 +656,11 @@ def register(sub, positive_int) -> None: "candidate is not routing: the sibling confusion is the " "failure mode (every `dpnp-*` skill scored 0.0 lexically)") routing_p.add_argument("--catalog", default=None, - help="Directory of skills to take the competitors from: every " - "SKILL.md under it except the artifact itself (a template/ " - "copy and a byte-identical mirror that loads under the same " - "skill name are left out), i.e. the siblings a harness " - "lists next to it. Combines with --competitors") + help="Directory of skills to take the competitors from: every SKILL.md " + "under it except the artifact itself (a template/ copy, a byte-" + "identical mirror that loads under the same skill name, and " + "vendored or VCS directories like .git are left out), i.e. the " + "siblings a harness lists next to it. Combines with --competitors") routing_p.add_argument("--queries", default=None, help="JSON list (or one per line) of user-shaped requests that " "should select this artifact, written independently of it") diff --git a/dokimi/dokimi/external/catalog.py b/dokimi/dokimi/external/catalog.py index b52e88120..0313762d3 100644 --- a/dokimi/dokimi/external/catalog.py +++ b/dokimi/dokimi/external/catalog.py @@ -30,7 +30,7 @@ from dokimi.harnesses.tasks import TaskSpec, discover_tasks, is_task_dir, load_task from dokimi.skills import Skill, load_skill -from dokimi.skills.loader import TEMPLATE_DIR_NAMES +from dokimi.skills.loader import TEMPLATE_DIR_NAMES, walk_skill_files DEFAULT_TASKS_SUBDIR = "evaluation/harbor/tasks" DEFAULT_SUITES_MANIFEST = "evaluation/harbor/suites.json" @@ -99,10 +99,11 @@ def _scan_skill_files(root: Path) -> list[SkillCatalogEntry]: A skill that does not load is an error row, so one broken skill does not abort the scan. A ``SKILL.md`` under a ``template``/``templates`` directory is a scaffold and is left - out, as :func:`dokimi.skills.discover_skills` leaves it out. + out, as :func:`dokimi.skills.discover_skills` leaves it out; one under a vendored or + VCS directory is a dependency's copy and is not walked, as there too. """ found: list[SkillCatalogEntry] = [] - for skill_file in sorted(root.rglob("SKILL.md")): + for skill_file in walk_skill_files(root): if TEMPLATE_DIR_NAMES.intersection(skill_file.relative_to(root).parts[:-1]): continue try: diff --git a/dokimi/dokimi/skills/formats.py b/dokimi/dokimi/skills/formats.py index 26830c499..7677c0d97 100644 --- a/dokimi/dokimi/skills/formats.py +++ b/dokimi/dokimi/skills/formats.py @@ -36,7 +36,7 @@ from pathlib import Path from typing import Any, Dict, List, Optional -from dokimi.skills.loader import skill_files +from dokimi.skills.loader import VENDORED_DIR_NAMES, skill_files from dokimi.utils import parse_frontmatter FORMAT_AGENT_SKILL = "agent-skill" @@ -60,7 +60,7 @@ (".agents/skills", "agents"), # the cross-tool alias Codex and Gemini CLI read ("environment/skills", "task-bundled"), # a SkillsBench / Harbor task's own skills ) -_SKIP_DIRS = frozenset({".git", "node_modules", "__pycache__", ".venv"}) +_SKIP_DIRS = VENDORED_DIR_NAMES # the same dependency and VCS dirs discover_skills does not walk @dataclass diff --git a/dokimi/dokimi/skills/loader.py b/dokimi/dokimi/skills/loader.py index 82f815f42..05c01c562 100644 --- a/dokimi/dokimi/skills/loader.py +++ b/dokimi/dokimi/skills/loader.py @@ -12,6 +12,7 @@ """ import hashlib +import os from dataclasses import dataclass, field from pathlib import Path from typing import Any, Dict, List, Optional, Tuple @@ -20,6 +21,10 @@ # Directory names that hold a scaffold to copy, not a skill to use (see discover_skills). TEMPLATE_DIR_NAMES = frozenset({"template", "templates"}) +# Directory names that hold version control, installed dependencies or virtualenvs: a +# SKILL.md there is a vendored copy of someone else's skill, not one of the catalog's. +VENDORED_DIR_NAMES = frozenset({".git", ".hg", ".svn", "node_modules", "site-packages", + ".venv", "venv", ".tox", ".nox", "__pycache__"}) @dataclass @@ -154,11 +159,28 @@ def load_skill(path) -> Skill: ) +def walk_skill_files(root: Path) -> List[Path]: + """Every entry named ``SKILL.md`` below *root*, sorted, without walking :data:`VENDORED_DIR_NAMES`. + + A dangling symlink or a directory is kept: ``catalog scan`` reports it as an error row. + """ + found: List[Path] = [] + for dirpath, dirnames, filenames in os.walk(root): + dirnames[:] = [d for d in dirnames if d not in VENDORED_DIR_NAMES] + if "SKILL.md" in filenames or "SKILL.md" in dirnames: + found.append(Path(dirpath) / "SKILL.md") + return sorted(found) + + def discover_skills(root, *, notes: Optional[List[str]] = None) -> List[Skill]: """Load every skill under *root* (each subdirectory with a ``SKILL.md``). - Two kinds of ``SKILL.md`` are not skills of *root* and are left out: + Three kinds of ``SKILL.md`` are not skills of *root* and are left out: + - one under a version-control, dependency or virtualenv directory + (:data:`VENDORED_DIR_NAMES`: ``.git``, ``node_modules``, ``site-packages``, + ``.venv`` ...) below *root*, a vendored copy of a package's skill that would + otherwise load as a skill of its own. Those directories are not walked; - one under a ``template`` or ``templates`` directory at any depth below *root*, a scaffold to copy (anthropics/skills keeps a blank one at ``template/SKILL.md``). A real skill kept under such a directory is left out too; @@ -171,6 +193,7 @@ def discover_skills(root, *, notes: Optional[List[str]] = None) -> List[Skill]: Args: root: Directory to scan. notes: When given, one line per skipped template or mirror is appended to it. + Vendored directories are skipped without a note. Returns: The skills, in path order. @@ -182,7 +205,7 @@ def skill_files(root, *, notes: Optional[List[str]] = None) -> List[Path]: """The ``SKILL.md`` files :func:`discover_skills` loads, in path order, unloaded.""" root = Path(root) found: List[Path] = [] - for skill_file in sorted(root.rglob("SKILL.md")): + for skill_file in walk_skill_files(root): if not skill_file.is_file(): continue if TEMPLATE_DIR_NAMES.intersection(skill_file.relative_to(root).parts[:-1]): diff --git a/dokimi/tests/test_catalog_command.py b/dokimi/tests/test_catalog_command.py index b86fa026e..2760a164f 100644 --- a/dokimi/tests/test_catalog_command.py +++ b/dokimi/tests/test_catalog_command.py @@ -116,6 +116,28 @@ def test_catalog_scan_exits_nonzero_on_load_error(tmp_path, capsys): assert exc_info.value.code == 1 +@pytest.mark.parametrize("broken", ["dangling-symlink", "directory"]) +def test_catalog_scan_exits_nonzero_on_a_skill_md_that_is_not_a_file(tmp_path, capsys, broken): + # discover_skills leaves such a SKILL.md out; the bare scan must still report it. + _write_skill(tmp_path, "good") + bad = tmp_path / "skills" / "bad" / "SKILL.md" + bad.parent.mkdir(parents=True) + if broken == "directory": + bad.mkdir() + else: + try: + bad.symlink_to(tmp_path / "missing.md") + except OSError: # Windows without Developer Mode or the symlink privilege + pytest.skip("this OS refuses an unprivileged symlink") + + parser = build_parser() + args = parser.parse_args(["catalog", "scan", "--root", str(tmp_path), "--kind", "skills"]) + with pytest.raises(SystemExit) as exc_info: + args.func(args) + assert exc_info.value.code == 1 + assert "bad" in capsys.readouterr().out + + def test_catalog_scan_exits_nonzero_on_a_task_that_exists_but_cannot_load(tmp_path, capsys): # The tasks-half sibling of the test above. Both halves need their own case: # skills failures carry an `error`, task failures a `failed` flag, and the diff --git a/dokimi/tests/test_external_catalog.py b/dokimi/tests/test_external_catalog.py index 6595a35ed..62ea6da58 100644 --- a/dokimi/tests/test_external_catalog.py +++ b/dokimi/tests/test_external_catalog.py @@ -187,6 +187,15 @@ def test_read_skill_manifest_falls_back_to_bare_scan_without_manifest(tmp_path): assert entries[0].status is None +@pytest.mark.parametrize("vendored", ["node_modules/pkg", ".venv/lib/python3.12/site-packages/pkg"]) +def test_read_skill_manifest_bare_scan_skips_vendored_copies(tmp_path, vendored): + _write_skill(tmp_path, "bare-skill") + _write_skill(tmp_path / vendored, "bare-skill") + + entries = read_skill_manifest(tmp_path) + assert [e.path for e in entries] == [tmp_path / "skills" / "bare-skill"] + + # ── suites.json manifest ────────────────────────────────────────────────────── diff --git a/dokimi/tests/test_skill_formats.py b/dokimi/tests/test_skill_formats.py index f2ae8a0d7..b110599a8 100644 --- a/dokimi/tests/test_skill_formats.py +++ b/dokimi/tests/test_skill_formats.py @@ -83,10 +83,18 @@ def test_each_ecosystem_is_named_with_how_it_is_selected(tmp_path): def test_every_loadable_package_is_one_the_skill_loader_loads(tmp_path): root = _tree(tmp_path) loadable = {p.name for p in sf.detect_skill_packages(root) if p.loadable} - # The loader does not skip node_modules/ (a vendored copy loads as a skill); the scan does. + # node_modules/ is a vendored directory: the loader and the scan both skip it. assert loadable == {s.name for s in discover_skills(root)} - {"x"} +def test_a_vendored_skill_is_not_listed_as_a_package(tmp_path): + _skill(tmp_path / "skills/a", "a") + _skill(tmp_path / "lib/site-packages/pkg/skills/b", "b") + _skill(tmp_path / "venv/skills/c", "c") + paths = [p.path.relative_to(tmp_path).as_posix() for p in sf.detect_skill_packages(tmp_path)] + assert paths == ["skills/a/SKILL.md"] + + def test_a_malformed_manifest_is_a_problem_not_a_crash(tmp_path): for value in (5, True, "abc"): root = tmp_path / str(value) diff --git a/dokimi/tests/test_skill_loader.py b/dokimi/tests/test_skill_loader.py index 7884ffcd3..339e80338 100644 --- a/dokimi/tests/test_skill_loader.py +++ b/dokimi/tests/test_skill_loader.py @@ -95,6 +95,46 @@ def test_discovery_skips_a_template_directory(tmp_path, dirname): assert [s.name for s in discover_skills(root)] == ["linux-perf"] +@pytest.mark.parametrize("vendored", ["node_modules/pkg", ".git/modules/sub", ".venv/lib/python3.12/site-packages/pkg"]) +def test_discovery_skips_vendored_copies(tmp_path, vendored): + root = tmp_path / "repo" + _write_skill(root / "skills" / "linux-perf", "linux-perf", "Profile Linux programs.") + # A different version of the same skill, so the mirror rule would not drop it. + _write_skill(root / vendored / "skills" / "linux-perf", "linux-perf", "Profile programs, v0.") + _write_skill(root / vendored / "skills" / "other", "other", "Someone else's skill.") + notes: list[str] = [] + assert [s.name for s in discover_skills(root, notes=notes)] == ["linux-perf"] + assert notes == [] + + +@pytest.mark.parametrize( + "name", + [ + ".git", + ".hg", + ".svn", + "node_modules", + "site-packages", + ".venv", + "venv", + ".tox", + ".nox", + "__pycache__", + ], +) +def test_discovery_skips_each_vendored_name(tmp_path, name): + root = tmp_path / "repo" + _write_skill(root / "skills" / "a", "a", "Skill a.") + _write_skill(root / "skills" / "a" / name / "pkg", "b", "Vendored b.") + assert [s.name for s in discover_skills(root)] == ["a"] + + +def test_discovery_walks_a_root_inside_a_vendored_directory(tmp_path): + root = tmp_path / "node_modules" / "pkg" / "skills" + _write_skill(root / "a", "a", "Skill a.") + assert [s.name for s in discover_skills(root)] == ["a"] + + def test_discovery_reports_a_skipped_template(tmp_path): root = tmp_path / "catalog" _write_skill(root / "template", "template-skill", "Replace with description of the skill.") @@ -263,6 +303,14 @@ def test_discovery_ignores_a_directory_named_skill_md(tmp_path): assert [s.name for s in discover_skills(root)] == ["linux-perf"] +def test_discovery_ignores_a_dangling_skill_md_symlink(tmp_path): + root = tmp_path / "catalog" + (root / "broken").mkdir(parents=True) + (root / "broken" / "SKILL.md").symlink_to(tmp_path / "missing.md") + _write_skill(root / "linux-perf", "linux-perf", "Profile Linux programs.") + assert [s.name for s in discover_skills(root)] == ["linux-perf"] + + def test_arms_run_exits_1_with_a_message_when_skill_md_is_a_directory(tmp_path, monkeypatch, capsys): from dokimi import cli