fix(skills): a disabled load path hides only the duplicate it names
r2 gate (2c, Low): `skills.disabled: [a/one]` — what `hermes skills` / the web toggle save for the same-tier duplicate shown as `a/one` — matched every copy's relative path, so an unrelated, uniquely named skill in another tier at the same relative path (external `e1/a/one` named `other`) vanished from list, index, view, preload and cron while the config UI showed it enabled. is_disabled_entry now matches the declared name or the entry's load_name (the relative path only for an ambiguous same-tier row). skill_view applies the path rule only when that path is the resolved copy's catalog load_name. This is the reviewer's intent but not the literal `name == rel` check: that check would let the disabled a/one copy load through its directory alias (`one`), which r2 verified is refused. The extended test pins both cases.
This commit is contained in:
@@ -481,9 +481,10 @@ def pick_skill_candidate(candidates) -> Tuple[Optional[int], List[int]]:
|
||||
|
||||
|
||||
def is_disabled_entry(entry: Dict[str, Any], disabled: Set[str]) -> bool:
|
||||
"""``skills.disabled`` matches a resolved catalog entry by its declared name OR its exact relative
|
||||
path — the identifier a same-tier duplicate's list/config/web row shows (``a/one``) and saves."""
|
||||
return bool(disabled) and not disabled.isdisjoint({str(entry["name"]), entry["relative_path"]})
|
||||
"""``skills.disabled`` matches a resolved catalog entry by its declared name OR its ``load_name`` —
|
||||
the exact path a same-tier duplicate's list/config/web row shows (``a/one``) and saves. A unique
|
||||
copy elsewhere that merely sits at the same relative path is not matched."""
|
||||
return bool(disabled) and not disabled.isdisjoint({str(entry["name"]), entry.get("load_name")} - {None})
|
||||
|
||||
|
||||
def resolve_skill_catalog(entries: List[Dict[str, Any]]) -> List[Dict[str, Any]]:
|
||||
|
||||
@@ -592,14 +592,24 @@ class TestDuplicateNamesAgreeAcrossSurfaces:
|
||||
assert ambiguous in parts[0] and "LOCAL XDUP" in "\n".join(parts)
|
||||
|
||||
# Disabling a duplicate by the exact path its row shows (what `hermes skills` / the web toggle
|
||||
# save) hides it from list and index and refuses the load.
|
||||
# save) hides it from list and index and refuses the load — but not an unrelated, uniquely named
|
||||
# skill in another tier that merely sits at the same relative path.
|
||||
_make_skill(ext, "other", category="a", body="UNRELATED OTHER")
|
||||
(ext / "a" / "other").rename(ext / "a" / "one")
|
||||
disabled = {"a/one"}
|
||||
monkeypatch.setattr(skill_utils, "get_disabled_skill_names", lambda *a, **k: disabled)
|
||||
monkeypatch.setattr(pb, "get_disabled_skill_names", lambda *a, **k: disabled)
|
||||
monkeypatch.setattr(skills_tool_module, "_is_skill_disabled", lambda n, platform=None: n in disabled)
|
||||
assert sorted(s["name"] for s in json.loads(skills_list())["skills"]) == ["b/two", "xdup"]
|
||||
assert "a/one" not in pb.build_skills_system_prompt()
|
||||
assert "disabled" in json.loads(skill_view("a/one"))["error"]
|
||||
monkeypatch.setattr(skills_tool_module, "_is_skill_disabled", lambda *n, platform=None: not disabled.isdisjoint(n))
|
||||
monkeypatch.setattr(skills_tool_module, "_SKILLS_CACHE", {})
|
||||
pb.clear_skills_system_prompt_cache()
|
||||
assert sorted(s["name"] for s in json.loads(skills_list())["skills"]) == ["b/two", "other", "xdup"]
|
||||
index = [ln.strip() for ln in pb.build_skills_system_prompt().splitlines() if ln.startswith(" - ")]
|
||||
assert index == ["- other: Description for other.", "- b/two: Description for dup-demo.",
|
||||
"- xdup: Description for xdup."]
|
||||
for ident in ("a/one", "one"): # refused by any alias, as list/index hide it
|
||||
assert "disabled" in json.loads(skill_view(ident))["error"]
|
||||
assert "UNRELATED OTHER" in json.loads(skill_view("other"))["content"]
|
||||
assert build_preloaded_skills_prompt(["other"])[1:] == (["other"], [])
|
||||
|
||||
|
||||
class TestBuildSkillInvocationMessage:
|
||||
|
||||
@@ -607,8 +607,11 @@ def skill_view(
|
||||
if not skill_matches_platform(frontmatter):
|
||||
return _fail(f"Skill '{name}' is not supported on this platform.", readiness_status=SkillReadinessStatus.UNSUPPORTED.value)
|
||||
resolved_name = frontmatter.get("name", skill_md.parent.name)
|
||||
# Disabled by declared name or by the exact path its list row shows (same-tier duplicates).
|
||||
if _is_skill_disabled(resolved_name) or _is_skill_disabled(_owned_relative(skill_dir, skill_md, all_dirs)):
|
||||
# Disabled by declared name, or by the exact path that IS this copy's load name (a same-tier
|
||||
# duplicate's row) — whatever alias reached it; an unrelated copy at that path elsewhere is not.
|
||||
rel = _owned_relative(skill_dir, skill_md, all_dirs)
|
||||
if _is_skill_disabled(resolved_name) or (_is_skill_disabled(rel) and any(
|
||||
e["path"] == skill_md and e["load_name"] == rel for e in _skill_catalog(skip_disabled=True))):
|
||||
return _fail(f"Skill '{resolved_name}' is disabled. Enable it with `hermes skills` or inspect the files directly on disk.")
|
||||
if file_path and skill_dir:
|
||||
return _serve_skill_file(
|
||||
|
||||
Reference in New Issue
Block a user