Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions skillscope/references.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,15 @@
r"(?:href|src)\s*=\s*[\"'](?P<target>[^\"']+)[\"']", 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 = ".,;:!?'\""

Expand Down Expand Up @@ -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.

Expand Down
92 changes: 91 additions & 1 deletion skillscope/structure.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]:
Expand Down Expand Up @@ -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] = []
Expand Down
78 changes: 78 additions & 0 deletions tests/test_skillscope.py
Original file line number Diff line number Diff line change
Expand Up @@ -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."""

Expand Down
Loading