diff --git a/skillscope/references.py b/skillscope/references.py index 6f10e6d..d87ab7a 100644 --- a/skillscope/references.py +++ b/skillscope/references.py @@ -95,6 +95,15 @@ r"(?:href|src)\s*=\s*[\"'](?P[^\"']+)[\"']", re.IGNORECASE ) _BARE_URL = re.compile(r"https?://[^\s<>\"'`\\)\]}]+") + +# A path written as prose or inside a code span: at least one directory segment +# and a file extension. Skills point at their own scripts and data this way +# (`Run scripts/detect.py`, `Read data/epyc.json`), which is deliberately not a +# markdown link, so `_targets` never sees it. +_PATH_MENTION = re.compile(r"[\w./\-]+/[\w.\-]+\.[A-Za-z0-9]{1,6}") +# `${SKILL_DIR}/scripts/launch.sh` is rooted at the skill, so the tail is what +# has to resolve. Strip the variable rather than reading the rest as absolute. +_SHELL_VAR_PREFIX = re.compile(r"\$\{?\w+\}?/") # Trailing punctuation belongs to the sentence, not to the URL. _URL_TAIL = ".,;:!?'\"" @@ -248,6 +257,20 @@ def collect( return found +def path_mentions(text: str) -> set[str]: + """Every path-shaped token in the text, code spans and fences included. + + `collect` deliberately ignores code, because a link inside a fence is an + illustration rather than a promise. A *path* inside a fence is the opposite: + it is how a skill tells an agent which of its own files to run or read, so + this reads the raw text. + """ + return { + match.group(0).strip("`'\"(),") + for match in _PATH_MENTION.finditer(_SHELL_VAR_PREFIX.sub("", text)) + } + + def anchors(text: str) -> set[str]: """Every ``#fragment`` a markdown document offers. diff --git a/skillscope/structure.py b/skillscope/structure.py index 171eaf5..20ad2cd 100644 --- a/skillscope/structure.py +++ b/skillscope/structure.py @@ -121,7 +121,7 @@ def skill_errors(skill: str) -> list[str]: *_body_errors(body), ) ] - return found + _required_errors(skill, folder, cfg) + return found + _required_errors(skill, folder, cfg) + _path_errors(skill, folder) def _frontmatter(text: str) -> tuple[dict | None, str, str]: @@ -233,6 +233,96 @@ def _body_errors(body: str) -> list[str]: ] +EVALS_DIR = "evals" + + +def _shipped_files(folder: Path) -> dict[str, str]: + """Files the skill ships in a subdirectory, keyed by their name. + + Subdirectories only. A bare `SKILL.md` or `skill-card.md` name turns up in + install instructions for unrelated trees, so matching on it says nothing. + `evals/` is test data and is never the agent's to read. + """ + shipped: dict[str, str] = {} + for path in sorted(folder.rglob("*")): + if not path.is_file(): + continue + relative = path.relative_to(folder) + if len(relative.parts) < 2 or relative.parts[0] == EVALS_DIR: + continue + if any(part.startswith(".") for part in relative.parts): + continue + shipped.setdefault(path.name, relative.as_posix()) + return shipped + + +def _resolves(base: Path, mention: str, folder: Path) -> bool: + """Whether `mention` names a file that exists inside the skill, read from `base`.""" + candidate = (base / mention).resolve() + try: + candidate.relative_to(folder.resolve()) + except ValueError: + return False # climbed out of the skill, so it is not the skill's file + return candidate.is_file() + + +def _path_errors(skill: str, folder: Path) -> list[str]: + """Paths the skill's markdown points at that are not in the skill. + + A skill that names its own files by where they sit in its source repo is + right there and wrong everywhere else: nothing rewrites paths in the body + when the folder is vendored or renamed, so an agent follows the path, finds + nothing, and improvises. The skill keeps passing its evals, because + improvising often works. + + Reported only when the mention ends with exactly where the file ships, so + what was meant is not in doubt. + """ + shipped = _shipped_files(folder) + if not shipped: + return [] + + found: list[str] = [] + for path in sorted(folder.rglob("*")): + if not path.is_file(): + continue + if path.suffix.lower() not in references.MARKDOWN_SUFFIXES: + continue + relative = path.relative_to(folder) + if relative.parts[0] == EVALS_DIR: + continue + if any(part.startswith(".") for part in relative.parts): + continue + try: + text = path.read_text(encoding="utf-8") + except (OSError, UnicodeDecodeError): + continue # the reader that owns this file reports why + for mention in sorted(references.path_mentions(text)): + actual = shipped.get(mention.rsplit("/", 1)[-1]) + if actual is None or mention == actual: + continue + # The tail has to be where the file really is, or this is a path to + # something else that happens to share a name. + if not mention.endswith("/" + actual): + continue + # An absolute path is somewhere else entirely: a container mount, a + # host layout. Not the skill's to resolve. + if mention.startswith("/"): + continue + # Resolved against the file it is written in, the way a markdown + # link is, or against the skill root, the way an agent handed a + # skill folder reads it. Either is a real way to reach the file, so + # only a path that answers to neither is unreachable. + if _resolves(path.parent, mention, folder) or _resolves(folder, mention, folder): + continue + found.append( + f"{skill}/{relative.as_posix()}: `{mention}` is not in the " + f"skill; that file ships at `{actual}`. A path written for the " + "source repo's layout does not survive being vendored." + ) + return found + + def _required_errors(skill: str, folder: Path, cfg: config.Config) -> list[str]: """Files this repo requires of every skill, and the sections in them.""" found: list[str] = [] diff --git a/tests/test_skillscope.py b/tests/test_skillscope.py index 11cdee6..da8875e 100644 --- a/tests/test_skillscope.py +++ b/tests/test_skillscope.py @@ -1693,6 +1693,84 @@ def test_a_malformed_skill_stops_a_run_before_it_spends_anything(self) -> None: cli._structural_or_exit() +class TestPathsASkillWritesAboutItself(unittest.TestCase): + """A skill names its own scripts and data in prose and in code spans. + + Those are not markdown links, so the reference checks never see them. A + path that reaches outside the folder is dead in every install: vendored + into a catalog, copied into an agent's skills directory, or fetched with + the CLI. It resolves only for someone standing in the repo it was written + in, which is the one place it is never needed. + """ + + def setUp(self) -> None: + self.repo = Repo(self) + self.folder = self.repo.skill( + "demo-skill", + dataset=tier0_dataset("demo"), + workspace={ + "scripts/detect.py": "print('detecting')\n", + "templates/spec.md": "# Spec\n", + }, + ) + self.repo.activate() + + def body(self, text: str, where: str = "SKILL.md") -> None: + """Give the skill a body, in SKILL.md or in a file beside it.""" + path = self.folder / where + path.parent.mkdir(parents=True, exist_ok=True) + if where == "SKILL.md": + text = f"---\nname: demo-skill\ndescription: Does demo things.\n---\n{text}" + path.write_text(text, encoding="utf-8") + + def test_a_path_that_resolves_from_the_skill_root_is_left_alone(self) -> None: + self.body("Run `scripts/detect.py` before anything else.\n") + self.assertEqual(structure.errors(), []) + + def test_a_path_that_resolves_from_the_file_it_sits_in_is_left_alone(self) -> None: + self.body("Follow `../templates/spec.md`.\n", where="agents/helper.md") + self.assertEqual(structure.errors(), []) + + def test_a_path_carrying_the_source_repos_layout_is_reported(self) -> None: + self.body("Read `Upstream/Repo/skills/demo-skill/scripts/detect.py` first.\n") + errors = structure.errors() + self.assertEqual(len(errors), 1, errors) + self.assertIn("Upstream/Repo/skills/demo-skill/scripts/detect.py", errors[0]) + self.assertIn("`scripts/detect.py`", errors[0]) + + def test_a_variable_rooted_path_is_the_skills_own(self) -> None: + self.body("Run `${SKILL_DIR}/scripts/detect.py`.\n") + self.assertEqual(structure.errors(), []) + + def test_a_relative_path_inside_a_script_is_not_markdown(self) -> None: + # Resolved at runtime against the script, which is not where the agent + # is standing, so it is not the agent's path to follow. + self.body("Run `scripts/detect.py`.\n") + (self.folder / "scripts" / "run.sh").write_text( + 'source "$(dirname "$0")/../templates/spec.md"\n', encoding="utf-8" + ) + self.assertEqual(structure.errors(), []) + + def test_a_file_at_the_skill_root_is_not_matched_by_name(self) -> None: + # Install instructions name SKILL.md in trees that are nobody's skill. + self.body("Copy it to `.cursor/skills/demo-skill/SKILL.md`.\n") + self.assertEqual(structure.errors(), []) + + def test_an_absolute_path_belongs_to_somebody_else(self) -> None: + self.body("The container mounts it at `/opt/demo/scripts/detect.py`.\n") + self.assertEqual(structure.errors(), []) + + def test_a_path_ending_somewhere_else_is_a_different_file(self) -> None: + self.body("Compare against `vendor/other/detect.py`.\n") + self.assertEqual(structure.errors(), []) + + def test_the_dataset_is_not_the_agents_to_read(self) -> None: + (self.folder / "evals" / "fixtures").mkdir(parents=True, exist_ok=True) + (self.folder / "evals" / "fixtures" / "detect.py").write_text("", encoding="utf-8") + self.body("Run `scripts/detect.py`.\n") + self.assertEqual(structure.errors(), []) + + class TestARepoWhereNoSkillWasFound(unittest.TestCase): """Grading nothing is reported, because a green check for it would lie."""