fix(skills): same-root duplicate skill names resolve instead of refusing, symlink-view roots stop warning (#112179, salvage #112180) - #113126
Merged
Conversation
_locate_skill refused every name with more than one candidate. That is right for one skill reachable through two tiers (silent shadowing) but wrong for duplication inside a single search dir: the runtime root is a symlink view of the library, so a top-level symlink and a nested category copy of the same skill collided and made the bare name unusable — bundle members were then reported as missing and skipped. Candidates are now ranked within one root: a real SKILL.md wins over a legacy <name>.md, then the shallower path. Cross-tier ambiguity keeps refusing. Ownership is decided lexically so a symlinked entry is not reclassified into another root. Trust warnings accept the resolved target of every root entry as well as the lexical path, so a root that exposes skills through symlinks no longer warns on every load and the injection-pattern signal stays readable. Tests: 183 passed (tests/tools/test_skills_tool.py + tests/agent/test_skill_*.py). The two new trust tests fail against the previous code; the outside-file test passes on both and guards the invariant.
…shape Slim redo of the cherry-picked #112180 hunks without changing behaviour: `_owning_search_dir` / `_rank_same_root_candidate` lose their unreachable fallbacks (the owning root is chosen by lexical containment, so `relative_to(root)` cannot fail), the trust check accepts the lexical path inline instead of through a nested helper, and the WHY-only comments replace the install-specific narration. Tests trimmed to two invariants per fix: same-root nested copy resolves to the shallower path, equal-rank tie still refuses; symlinked entry is trusted by the root that exposes it, a genuinely outside file still warns. Dropped the legacy flat `<name>.md` test (the rank still covers it; re-probed live).
૮ >ﻌ< ა ci reviewran on 09b7383 — fix: collapse same-root duplicate skills only when provably
|
…skill The same-root ranking added for #112179 was depth-only, so it silently resolved any two skills sharing a bare name inside one search dir instead of refusing: a shallower ~/.hermes/skills/evil/SKILL.md with `name: github` shadowed software-development/github, and a hub package's nested <pkg>/foo/SKILL.md won over the user's top-level legacy foo.md — the exact shadowing shape the 'Ambiguous skill name' refusal exists for. Gate the collapse on identity: candidates must share one resolved SKILL.md (symlink view) or byte-identical content (copy). Different content keeps the loud refusal, as on main. The log no longer labels the top-level file as 'nested'; the project-tier comment now says what still refuses there. _log_security_warnings: drop the lexical is_relative_to escape. skill_view only ever passes <search_dir>/... paths, so that escape made the resolved 'outside the trusted skills directory' warning unreachable — including for a SKILL.md symlinked to a file outside every root, which is what it guards. A symlink is quiet only when its target resolves under a registered dir. Tests: the two PR tests that encoded the old behaviour are reshaped (identical-copy collapse; symlink-into-registered-dir quiet), plus one invariant per finding, each red on the pre-fix code.
12 of 13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A skill whose name appears twice inside ONE skills root (a symlink-view entry
~/.hermes/skills/demoplus a nested copy~/.hermes/skills/cat/demo) now loads by its bare name — inskill_view,/skill, and skill bundles — instead of being refused as a collision, and a symlink-view skills root no longer emits a "skill file is outside the trusted skills directory" warning on every load.tools/skills_tool.py::_locate_skill: when every candidate belongs to the same search dir (ownership decided lexically via_owning_search_dir, so a symlinked entry stays with the root that exposes it), rank them — realSKILL.mdbeats a legacy flat<name>.md, then the shallower path wins — and log the pick at info level. An equal-rank tie or candidates spanning two tiers (project / local / external) still refuse exactly as before, so the anti-shadowing guard from 59da8ec is unchanged.tools/skills_tool.py::_log_security_warnings: the trust check also accepts the lexical (unresolved) path under a configured root, matching whatagent/skill_utils.py::normalize_skill_identifieralready does ("prefer the lexical path under a trusted root before resolving symlinks"). A genuinely outside file still warns; the injection-pattern check is untouched.tests/tools/test_skills_tool.py): two invariants per fix — same-root nested copy resolves to the shallower path / equal-rank tie still refuses; symlinked entry is trusted by the root that exposes it / outside file still warns. The two positive tests fail onorigin/main.Validation (live probe, temp
HERMES_HOME, real_locate_skill/skill_view/ bundle_load_skill_payload)demo=skills/demo→ symlink to lib +skills/cat/democopyAmbiguous skill name 'demo': 2 skills match…skills/demo/SKILL.md; bundle memberdemoloads (path: demo/SKILL.md) instead ofSkills missing (skipped)sharedin localskills/tools/sharedANDexternal_dirspduptwice inside the project tieroverin project tier and localskills/demo/SKILL.mdskill file is outside the trusted skills directoryelsewhere/x/SKILL.mdA/B: swapping
origin/main'stools/skills_tool.pyback in turns thedemo/ctx/bundle rows red again.Root cause:
_locate_skilltreated any second candidate as cross-tier shadowing, and the trust check compared only symlink-resolved paths against resolved roots, so a root that exposes skills through symlinks failed both.Fixes #112179
Salvages #112180 (@ankinow) — cherry-picked 7a155c0, then trimmed in a follow-up commit.
Dropped hunks
agent/skill_bundles.py+tests/agent/test_skill_bundles.py(commit d8f5cd2, "bundle discovery survives a non-UTF-8 manifest"): unrelated to Skill names fail to resolve when duplicated inside a single search root (bundles report them as missing) #112179 (separate bug, authored under an agent identity); left for its own PR.test_legacy_flat_md_never_shadows_real_skill: third test on the same rank; the flat-vs-SKILL.mdcase was re-probed live (ctxrow above) and the rank logic is kept._owning_search_dir/_rank_same_root_candidateand the nested_exposed_underhelper (behaviour identical).Infographic
Review follow-up
All three majors reproduced on
f3c52f5(probe: tmpHERMES_HOME,skill_view(name);origin/mainrefused/warned in every case) and are fixed @ 09b7383._locate_skillsame-root collapse is depth-only —skills/evil/SKILL.md(name: github) resolved oversoftware-development/github. Fixed: collapse now requires_provably_same_skill(oneos.path.realpathfor the SKILL.md, or byte-identical content); different content keeps theAmbiguous skill namerefusal. Probe after fix:skill_view('github')→success: False, 2 matches. Invariant testtest_different_skill_with_same_frontmatter_name_in_same_root_refuses(red pre-fix).<pkg>/foo/SKILL.mdbeat legacy top-levelfoo.md, log mislabelledfoo.mdas "nested" — Fixed by the same identity rule (content differs → refuse; identical copies may still collapse). Log now readsidentical same-root copies, resolved to … (duplicates: …). Invariant testtest_nested_package_skill_does_not_shadow_top_level_legacy_flat_md(red pre-fix)._log_security_warningslexicalis_relative_toescape made the resolved-path warning unreachable — Fixed: escape removed, check is realpath-only against the resolved registered dirs (as on main). Probe after fix:skills/sym/SKILL.md → /tmp/outside/SKILL.mdlogsoutside the trusted skills directoryagain. PR tests reshaped:test_symlink_resolving_under_a_registered_dir_is_trusted(target under a registered root → quiet) andtest_skill_md_symlinked_to_outside_every_root_still_warnsreplacestest_genuinely_outside_file_still_warns(red pre-fix).test_nested_copy_inside_same_root_does_not_block_bare_nameused different bodies for the two copies; it now uses identical content, which is the Skill names fail to resolve when duplicated inside a single search root (bundles report them as missing) #112179 symlink-view + copy case.Verification:
ruff checkclean,check-windows-footguns.py --allclean,git diff --checkclean,scripts/run_tests.sh tests/tools/test_skill*.py→ 29 files, 665 passed, 0 failed.