fix(skills-hub): reconcile salvaged install fixes with full-directory fetch
Follow-up reconciling the four cherry-picked contributor fixes with the full-dir GitHubSource.fetch() that landed in #98246: - Missing SKILL.md-linked support paths now warn and install without the file at all three sources (GitHub full-dir, GitHub fallback, UrlSource) — dangling links are prose over-matches or repo-only dev tools, not install blockers (#66760/#90081). A referenced path present in the tree as a SYMLINK stays a hard rejection. - Extension requirement dropped from the glob/placeholder filter: references/LICENSE is a legitimate support file (82236's tests pin this). Truncated prose placeholders (references/type-<name>.md -> 'type-') are still rejected via the trailing-separator shape. - percent-quoted Contents-API path (82236) merged with revision pinning (96336) in _fetch_file_bytes. - Fixture typo fix: four cherry-picked test strings used '\---' where '\n---' was meant (DeprecationWarning + frontmatter never parsed). Validation: 167/167 across tests/tools/{skills_hub,skills_guard, skill_bundle_provenance} + tests/hermes_cli/test_skills_hub.py; live GitHub fetches (impeccable 163 files rev-pinned; anthropics frontend-design).
This commit is contained in:
Teknium
@@ -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
|
||||
|
||||
|
||||
|
||||
@@ -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-<name>.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-<name>.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:
|
||||
|
||||
Reference in New Issue
Block a user