From b82b4ac65cd4e7bf16fad77e5f4fdcc0595b4013 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sun, 4 Oct 2026 02:15:07 +0530 Subject: [PATCH] fix(skills): a disabled load path hides only the duplicate it names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- agent/skill_utils.py | 7 ++++--- tests/agent/test_skill_commands.py | 20 +++++++++++++++----- tools/skills_tool.py | 7 +++++-- 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/agent/skill_utils.py b/agent/skill_utils.py index a76c5cc1d2..51ecc7044b 100644 --- a/agent/skill_utils.py +++ b/agent/skill_utils.py @@ -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]]: diff --git a/tests/agent/test_skill_commands.py b/tests/agent/test_skill_commands.py index cf17816f6e..a799853aef 100644 --- a/tests/agent/test_skill_commands.py +++ b/tests/agent/test_skill_commands.py @@ -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: diff --git a/tools/skills_tool.py b/tools/skills_tool.py index 3cb4679539..572a859ebb 100644 --- a/tools/skills_tool.py +++ b/tools/skills_tool.py @@ -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(