diff --git a/tests/tools/test_skill_bundle_provenance.py b/tests/tools/test_skill_bundle_provenance.py index 2bf1e88ce6..1d5e6e02cf 100644 --- a/tests/tools/test_skill_bundle_provenance.py +++ b/tests/tools/test_skill_bundle_provenance.py @@ -146,7 +146,7 @@ def test_same_dir_link_without_extension_is_ignored(monkeypatch): """Prose targets that aren't file links (no extension) never fetch.""" from tools.skills_hub import _referenced_support_paths - skill = "---\nname: x\ndescription: x\---\nsee [notes](NOTES) and `README`\n" + skill = "---\nname: x\ndescription: x\n---\nsee [notes](NOTES) and `README`\n" assert _referenced_support_paths(skill) == set() @@ -155,7 +155,7 @@ def test_same_dir_link_query_and_fragment_are_stripped(): from tools.skills_hub import _referenced_support_paths skill = ( - "---\nname: x\ndescription: x\---\n" + "---\nname: x\ndescription: x\n---\n" "[a](CONTEXT-FORMAT.md?raw=1) [b](DEEPENING.md#usage)\n" ) assert _referenced_support_paths(skill) == {"CONTEXT-FORMAT.md", "DEEPENING.md"} @@ -165,7 +165,7 @@ def test_case_variant_of_skill_md_is_never_a_sibling_entry(): """skill.md must not ship as a bundle file (case-insensitive FS collision).""" from tools.skills_hub import _referenced_support_paths - skill = "---\nname: x\ndescription: x\---\n[home](skill.md)\n" + skill = "---\nname: x\ndescription: x\n---\n[home](skill.md)\n" assert _referenced_support_paths(skill) == set() @@ -173,7 +173,7 @@ def test_case_folded_sibling_collision_drops_the_pair(): """A.md + a.md would collide on install — neither ships.""" from tools.skills_hub import _referenced_support_paths - skill = "---\nname: x\ndescription: x\---\n[a](A.md) [a2](a.md)\n" + skill = "---\nname: x\ndescription: x\n---\n[a](A.md) [a2](a.md)\n" assert _referenced_support_paths(skill) == set() @@ -242,10 +242,10 @@ def test_github_source_fetch_downloads_full_skill_directory(monkeypatch): "See [audit](reference/audit.md) and run `node scripts/pin.mjs`.\n" ) fetched: list = [] - monkeypatch.setattr(source, "_fetch_file_content", lambda _repo, path: skill_md) + monkeypatch.setattr(source, "_fetch_file_content", lambda _repo, path, ref=None: skill_md) monkeypatch.setattr( source, "_fetch_file_bytes", - lambda _repo, path: fetched.append(path) or b"content-of-" + path.encode(), + lambda _repo, path, ref=None: fetched.append(path) or b"content-of-" + path.encode(), ) source._tree_cache["owner/repo"] = ( "main", @@ -275,20 +275,35 @@ def test_github_source_fetch_downloads_full_skill_directory(monkeypatch): } -def test_github_source_fetch_still_requires_linked_references(monkeypatch): - """A SKILL.md-linked references/ path missing from the tree rejects the bundle.""" +def test_github_source_fetch_dangling_linked_reference_warns_not_aborts(monkeypatch): + """A SKILL.md-linked references/ path absent from the tree installs + without the file (dangling links are prose over-matches / repo-only dev + tools — #66760/#90081); a SYMLINKED referenced path still hard-rejects.""" source = GitHubSource(GitHubAuth()) skill_md = ( "---\nname: dangling\ndescription: d\n---\n" "Read [the guide](references/guide.md).\n" ) - monkeypatch.setattr(source, "_fetch_file_content", lambda _repo, path: skill_md) - monkeypatch.setattr(source, "_fetch_file_bytes", lambda _repo, path: b"x") + monkeypatch.setattr(source, "_fetch_file_content", lambda _repo, path, ref=None: skill_md) + monkeypatch.setattr(source, "_fetch_file_bytes", lambda _repo, path, ref=None: b"x") + + # Missing entirely -> installs without it. source._tree_cache["owner/repo"] = ( "main", [{"path": "skill/SKILL.md", "type": "blob", "mode": "100644"}], ) + bundle = source.fetch("owner/repo/skill") + assert bundle is not None + assert "references/guide.md" not in bundle.files + # Present as a symlink -> hard rejection. + source._tree_cache["owner/repo"] = ( + "main", + [ + {"path": "skill/SKILL.md", "type": "blob", "mode": "100644"}, + {"path": "skill/references/guide.md", "type": "blob", "mode": "120000"}, + ], + ) assert source.fetch("owner/repo/skill") is None diff --git a/tools/skills_hub.py b/tools/skills_hub.py index 17e948ecac..3b2db4b705 100644 --- a/tools/skills_hub.py +++ b/tools/skills_hub.py @@ -220,11 +220,16 @@ def _referenced_support_paths(skill_md: str) -> Optional[set[str]]: except ValueError: return None if safe.split("/", 1)[0] in _ALLOWED_SUPPORT_DIRS: - # Prose globs/placeholders — e.g. ``references/type-*.md`` or - # ``references/type-.md`` (which the regex truncates to the - # bare prefix ``references/type-``) — are agent instructions, not - # files. Only tokens that can name an actual file are references. - if re.search(r"[*?<>]", safe) or "." not in safe.rsplit("/", 1)[-1]: + # Prose placeholders — e.g. ``references/type-.md`` (which + # the link regex truncates at ``<`` to the bare prefix + # ``references/type-``) — are agent instructions, not files. + # Glob shapes (*, ?, []) were already rejected on the raw + # candidate above; a truncated placeholder leaves a basename + # ending in a separator, which no real file uses. No extension + # requirement: extensionless support files + # (``references/LICENSE``) are legitimate. + base = safe.rsplit("/", 1)[-1] + if re.search(r"[*?<>]", safe) or not re.search(r"[A-Za-z0-9]$", base): continue paths.add(safe) for match in _SAMEDIR_LINK_RE.finditer(normalized): @@ -789,13 +794,15 @@ class GitHubSource(SkillSource): # install, and the scanner sees MORE this way, not less. branch, entries = tree prefix = f"{skill_path.rstrip('/')}/" + symlinked: set = set() for item in entries: - if item.get("type") != "blob" or item.get("mode") == "120000": - continue item_path = item.get("path", "") if not item_path.startswith(prefix): continue rel_path = item_path[len(prefix):] + if item.get("type") != "blob" or item.get("mode") == "120000": + symlinked.add(rel_path) + continue if rel_path == "SKILL.md": continue base = rel_path.rsplit("/", 1)[-1] @@ -812,16 +819,27 @@ class GitHubSource(SkillSource): "file; continuing without it: %s", item_path) continue files[rel_path] = content - # A support file SKILL.md links must actually exist as a regular - # file — a missing or symlinked referenced path rejects the - # bundle rather than installing a skill with dangling links. + # A SKILL.md-linked support path that isn't in the tree is a + # dangling link — a repo-only dev tool, prose over-match, or a + # file the author forgot to push. Warn and install without it + # rather than aborting the whole install (#66760/#90081): the + # skill body still works, and the gap is visible in the log. + # A referenced path that IS in the tree but as a symlink (or any + # non-regular entry) stays a hard rejection — that shape is an + # escape attempt, not a forgotten file. for rel_path in sorted(referenced): - if rel_path not in files: + if rel_path in symlinked: logger.warning( - "Referenced skill support file is missing: %s%s", - prefix, rel_path, + "Rejected non-regular referenced file in skill " + "bundle: %s%s", prefix, rel_path, ) return None + if rel_path not in files: + logger.warning( + "Referenced skill support file is missing; " + "continuing without it: %s%s", + prefix, rel_path, + ) revision = self._tree_revisions.get(repo) or branch else: for rel_path in referenced: