fix(skills): follow symlinks in skill management

[americodias]

What does this PR do?

skill_manage used Path.rglob("SKILL.md") in _find_skill() and _find_skill_in_other_profiles(). Unlike the shared iter_skill_index_files() helper, Path.rglob() does not follow directory symlinks. A symlinked skill could therefore appear in skills_list and load through skill_view, but skill_manage reported it as missing.

This PR routes both skill-management lookups through the existing shared iterator. The change is deliberately narrow: it keeps current matching, exclusion, precedence, and mutation behavior, while making traversal consistent with the rest of the skills system.

This supersedes #35244 by preserving its minimal approach and adding the two items requested in maintainer review: platform-safe symlink setup and regression coverage for _find_skill_in_other_profiles(). Credit to @Linux2010 for #35244 and @liuhao1024 for the related #54200 implementation.

Related Issue

Fixes #35184
Closes #54195
Supersedes #35244

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✨ New feature (non-breaking change that adds functionality)
• 🔒 Security fix
• 📝 Documentation update
• ✅ Tests (adding or improving test coverage)
• ♻️ Refactor (no behavior change)
• 🎯 New skill (bundled or hub)

Changes Made

• tools/skill_manager_tool.py
  • replace both skill-discovery rglob() calls with iter_skill_index_files()
  • preserve existing exclusion and basename matching logic
• tests/tools/test_skill_manager_tool.py
  • verify _find_skill() discovers a skill installed as a directory symlink in an external skills root
  • verify _find_skill_in_other_profiles() discovers a skill below a symlinked category in another profile
  • skip cleanly when the host cannot create directory symlinks

How to Test

1. Configure an external skills directory containing a directory symlink to a real skill package.

2. Call skill_manage(action="patch", name="<skill>", old_string="__DOES_NOT_EXIST__", new_string="x") and confirm lookup reaches content matching instead of returning Skill '<skill>' not found.

3. Run:

   scripts/run_tests.sh tests/tools/test_skill_manager_tool.py -q
   python3 scripts/check-windows-footguns.py --diff origin/main
   uv run --extra dev ruff check .

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix/feature (no unrelated commits)
• I've run pytest tests/ -q and all tests pass — focused canonical suite passes; full-suite green is not claimed
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: macOS 15.7.7

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — N/A; no public API or configuration change
• I've updated cli-config.yaml.example if I added/changed config keys — N/A; no config keys changed
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A; existing shared iterator is reused
• I've considered cross-platform impact (Windows, macOS) per the compatibility guide — platform-safe symlink helper added; Windows-footgun scan passes
• I've updated tool descriptions/schemas if I changed tool behavior — N/A; no schema or description change

Screenshots / Logs

scripts/run_tests.sh tests/tools/test_skill_manager_tool.py -q
✓ targeted canonical test run passed

pytest TestFindSkillSymlinks
2 passed in 0.21s

python3 scripts/check-windows-footguns.py --diff origin/main
✓ No Windows footguns found (2 file(s) scanned).

uv run --extra dev ruff check .
All checks passed!

[teknium1]

Thanks for the focused symlink-lookup work. The underlying defect is still present on current main: tools/skill_manager_tool.py:638 and :702 use Path.rglob("SKILL.md"), while the established reader-side iterator uses os.walk(..., followlinks=True) in agent/skill_utils.py:821.

Problems

• The PR also includes commit 8468aaca446889653029092800de894d0f937da3, which adds the unrelated --oneshot-file CLI surface in hermes_cli/_parser.py:116, hermes_cli/main.py, and hermes_cli/oneshot.py. Please keep this PR to the stated skill-management fix.
• If proposed separately, the new helper's “without following symlinks” claim is not true where os.O_NOFOLLOW is unavailable: hermes_cli/oneshot.py:49-56 then opens and validates the symlink target, and tests/hermes_cli/test_oneshot_file.py:45-47 deliberately does not assert rejection on that path.

Suggested changes

• Split out 8468aaca; the first commit is a narrow, relevant salvage candidate.
• Make the separate file-input feature's symlink behavior consistent across supported platforms, or document its platform limitation and test it.

Automated hermes-sweeper review.

[teknium1]

This introduces a new CLI feature unrelated to symlinked skill management. Please move the --oneshot-file implementation and its tests into a separate PR so this fix remains the narrow traversal change described in the PR body.

[teknium1]

When os.O_NOFOLLOW is unavailable, the subsequent os.open() follows a symlink and fstat() validates the target, contradicting this helper's “without following symlinks” contract. Reject links consistently on supported platforms, or narrow the contract and add coverage for this fallback.

[americodias]

Split out 8468aaca4 as requested — this PR now carries only the symlink-lookup fix (719bf5b). The oneshot-file feature will be proposed separately, with the O_NOFOLLOW portability concern addressed (consistent behavior or a documented, tested platform limitation).

[GottZ]

This was generated by AI during triage.

Summary

35 PRs address or reference this issue complex: configurable skill write roots, external-directory mutation, and the recurring mismatch where symlink-aware readers can see skills that rglob-based management and auxiliary paths cannot. The diffs range from already-merged external-dir and reader-discovery fixes through competing skill-manager iterator substitutions; #67748 is the recorded best fix for the remaining management lookup defect across #4759, #35184, #44609, and #54195.

Related pull requests

• fix: skill_manage now finds skills from external_dirs #4868 [closed] fixes — (+12/-6) — n/a: Scans local and configured external roots but retains rglob, so it addressed external-root lookup rather than symlink traversal; closed because commit ad4feea implemented the behavior on main.
• fix: align skill manager lookup with local skills dir #5255 [closed] partial — (+40/-7) — n/a: Aligns lookup with a patched local root and protects delete cleanup boundaries, but does not replace rglob; closed by its author after surrounding code and tests changed.
• fix(skills): respect local skill dir in skill manager #5284 [closed] fixes — (+45/-2) — n/a: Restores local-first plus external-root lookup with tests but leaves symlink traversal unchanged; closed because commit ad4feea already implemented the external-dir behavior.
• fix(skills): copy-on-write for agent edits of external skills #5407 [closed] partial — (+467/-32) — n/a: Implements copy-on-write for external-skill mutations rather than fixing discovery; closed after main adopted hard-refuse boundary guards in commit e683c9d.
• fix(skills): follow symlinks with cycle guard across all skill discovery paths #7634 partial — (+75/-30) — n/a: Broadly replaces discovery scans with a cycle-guarded walker, but its open keep_open review requires preserving support-directory pruning and portable symlink tests before salvage.
• feat: support configurable default write directory for skills #8177 best fix — (+514/-134) — n/a: Recorded best fix for [Feature]: skills create / skills edit should support writing to a configurable target directory #4381, [Feature]: Config support assigning target dir for agent to write it's own skills #13963, and skill_manage(action='create') ignores skills.external_dirs, always writes to ~/.hermes/skills/ #21810, adding a profile-aware default_write_dir across discovery, creation, curation, archive/restore, menus, tests, and docs; keep open with the reviewed current-main rework as its salvage path.
• fix(skills): follow symlinked local skill dirs #8357 [closed] fixes — (+49/-4) — n/a: Routes list, categories, and bare-name view through symlink-aware discovery; closed because commits 02aba4a and a884f6d already implemented those reader-side paths.
• fix(skills): discover symlinked skill directories #8769 [closed] fixes — (+173/-19) — n/a: Adds cycle-safe symlink traversal and reader-side regressions, but the functional reader bug was already shipped in v2026.4.23; closed with cycle coverage left as a possible focused follow-up.
• fix: follow symlinks when scanning for SKILL.md #9253 [closed] fixes — (+18/-3) — n/a: Replaces three skills_tool rglob scans with a local followlinks walker; closed under the maintainer-bot verdict because commit 02aba4a and shared iterator call sites already cover them.
• fix(skills): follow symlinks in iter_skill_index_files #11847 [closed] partial — (+1/-1) — n/a: Adds followlinks=True to the canonical iterator; closed because its substantive commit was salvaged with attribution into merged fix(skills): follow symlinks in iter_skill_index_files (salvage #11847) #14230.
• fix(skills): follow symlinks when discovering SKILL.md #12624 [closed] fixes — (+87/-13) — n/a: Attempts a broad cross-subsystem rglob_follow migration, including skill management, but was closed by its author as superseded by other work.
• feat(skills): add configurable agent skills directory (skills.agent_dir) #13996 best fix — (+48/-5) — n/a: Recorded best fix for [Feature]: Config support assigning target dir for agent to write it's own skills #13963 but only adds skills.agent_dir to creation and partial discovery; its keep_open verdict identifies feat: support configurable default write directory for skills #8177 as the broader consolidation target, so retain only if split into non-overlapping work.
• fix(skills): follow symlinks in iter_skill_index_files (salvage #11847) #14230 [merged] partial — (+2/-1) — n/a: Merged reference implementation salvaging fix(skills): follow symlinks in iter_skill_index_files #11847, adding followlinks=True to iter_skill_index_files with preserved attribution.
• fix(skills): follow symlinks when discovering SKILL.md #14476 [closed] partial — (+60/-3) — n/a: Adds a cycle-guarded tools/skills_tool walker for list and view, but those reader paths were already implemented by commits 02aba4a and a884f6d; closed as implemented on main.
• fix(skills): follow symlinked category dirs consistently #14668 [closed] fixes — (+112/-7) — n/a: Moves residual reader and slash-command scans to the shared iterator with regressions; closed because its commit was salvaged with attribution into merged fix(skills): follow symlinked category dirs consistently #14740.
• fix(skills): follow symlinked category dirs consistently #14740 [merged] best fix — (+112/-7) — n/a: Recorded best fix for Bug: symlinked skills under ~/.hermes/skills are omitted from skills_list and bare-name skill_view lookup #8293 and merged reference implementation for symlinked category discovery in list, view, and slash-command paths, but it does not cover skill_manager lookup.
• fix(skills): let skill_manage modify skills in external_dirs in place #17512 [merged] best fix — (+171/-25) — n/a: Recorded best fix for skill_manage patch/edit/delete can't find skills from external_dirs #4759 and merged reference implementation enabling in-place mutation of discovered external skills while keeping creation local; it predates the residual symlink lookup fix.
• fix(skills): skill_manage create respects skills.external_dirs #21812 [closed] fixes — (+17/-2) — n/a: Routes creation to the first external directory, addressing skill_manage(action='create') ignores skills.external_dirs, always writes to ~/.hermes/skills/ #21810 narrowly; closed by its author after choosing the standard local path instead.
• fix(skills): skill_manage create respects skills.external_dirs #22236 [closed] best fix — (+101/-3) — n/a: Recorded best fix for skill_manage(action='create') ignores skills.external_dirs, always writes to ~/.hermes/skills/ #21810 with first-external-root creation and path-relative tests, but closed because a competing or cleaner alternative existed.
• fix(skill-tools): follow symlinks in skill discovery via rglob_follow #23484 [closed] partial — (+332/-18) — n/a: Adds a symlink-following helper for manager and usage paths but mixes extensive unrelated gateway, plugin, database, and model changes; closed as duplicate of fix(skills): follow symlinks when discovering SKILL.md #12624.
• fix(skills): create new skills in external skill dir #23562 partial — (+16/-8) — n/a: Routes creation to the first external root, but its keep_open review records a conflicting read-only contract and missing call-time profile fallback; author action is required rather than consolidation into the lookup fix.
• docs(skills): clarify external dir mutations #29411 [closed] related — (+3/-2) — n/a: Documents that existing external skills mutate in place while creation remains local; closed because its commit was salvaged with attribution into merged docs(skills): clarify external dir mutations (salvage of #29411) #29444.
• docs(skills): clarify external dir mutations (salvage of #29411) #29444 [merged] related — (+3/-2) — n/a: Merged documentation reference matching fix(skills): let skill_manage modify skills in external_dirs in place #17512's external mutation behavior, without changing configurable creation or symlink lookup.
• fix(skills): follow symlinks during skill discovery (#35184) #35213 fixes — (+62/-8) — n/a: Replaces both manager rglob lookups with the shared iterator, but its keep_open review requires portable symlink setup, cycle assessment, and consideration of sibling usage paths.
• fix(skill_manager): follow symlinks when discovering skills (#35184) #35244 duplicate — (+110/-4) — n/a: Implements the same two manager lookup substitutions with tests and received keep_open/high salvageability; despite that review, fix(skills): follow symlinks in skill management #67748 supersedes it by adding the requested platform-safe and cross-profile coverage.
• fix: rglob follow symlinks for skill discovery #35299 fixes — (+34/-7) — n/a: Covers manager and selected usage paths via a new parallel rglob_follow helper, but its keep_open review prefers the existing iterator and requires current-head sibling coverage plus tests.
• fix(skills): follow directory symlinks in SKILL.md discovery #44129 fixes — (+211/-97) — n/a: Broadly migrates many auxiliary discovery and counting paths to the canonical iterator, but its keep_open review identifies missing current-main sites and platform-unsafe symlink tests.
• fix(skills): use os.walk(followlinks=True) in _find_skill for symlinked skill dirs #44210 fixes — (+73/-8) — n/a: Fixes both manager lookups and adds direct, category, and cross-profile tests, but its keep_open review requires the shared iterator rather than an inline walk; its shown diff now uses that iterator and overlaps fix(skills): follow symlinks in skill management #67748.
• perf(profiles): allow lightweight profile listing #52433 [closed] related — (+214/-29) — n/a: Adds optional lightweight profile listing to avoid skill-count scans and is performance-oriented rather than a fix for external or symlinked skill discovery; closed.
• fix(skills): follow symlinks when discovering skills in external dirs #54200 [closed] duplicate — (+57/-4) — n/a: Applies the canonical iterator to both manager lookups with external-dir tests; closed as duplicate of earlier fix(skill_manager): follow symlinks when discovering skills (#35184) #35244 and remains relevant as the direct skill_manage cannot find symlinked skills in skills.external_dirs #54195 implementation.
• fix(skill_manage): _find_skill follows symlinked skill directories #56420 [closed] fixes — (+1129/-22) — n/a: Contains the relevant two-loop iterator substitution but bundles extensive unrelated approval, curator, deletion, and test changes; closed, with focused lookup work superseded by fix(skills): follow symlinks in skill management #67748.
• fix(skill_manage): _find_skill follows symlinked skill directories #56423 fixes — (+1129/-22) — n/a: Resubmits fix(skill_manage): _find_skill follows symlinked skill directories #56420's broad stale diff; despite its keep_open/high review, the review explicitly requests salvaging only the focused two-loop substitution, which fix(skills): follow symlinks in skill management #67748 already provides.
• Fix _find_skill: use iter_skill_index_files to follow symlinks #60960 partial — (+685/-5) — n/a: Changes only the active-profile lookup while bundling unrelated Codex delegation work; despite its keep_open/high verdict, it omits the reviewed cross-profile fix and is superseded by focused fix(skills): follow symlinks in skill management #67748.
• fix(skills): follow symlinks in skill management #67748 best fix — (+61/-4) — n/a: Recorded best fix for skill_manage patch/edit/delete can't find skills from external_dirs #4759, skill_manager_tool._find_skill uses Path.rglob() which does not follow symlinks, making symlinked skills invisible to skill_manage #35184, skill_manage _find_skill uses rglob which silently skips symlinked skills (Python 3.12) #44609, and skill_manage cannot find symlinked skills in skills.external_dirs #54195, replacing both manager rglob scans with the canonical iterator and adding platform-safe active/external and cross-profile regressions; the contributor-requested unrelated oneshot commit was split out.
• fix(skills): use symlink-following traversal in _find_skill #75225 best fix — (+4/-4) — n/a: Recorded best fix for Pending skill-proposal queue grows unbounded and self-invalidates when skills.write_approval is enabled (357 in 8 days, 21% dead) #75130's symlink-driven manager visibility slice and changes both lookup loops, but its keep_open/high verdict requires a real skill_manage patch regression and explicit preservation of symlink-delete refusal.

Duplicates

Manager-lookup duplicates: #35213, #35244, #35299, #44210, #54200, #56420, #56423, #60960, #67748, and #75225, with #67748 the focused superseding implementation for the core issues. Reader-discovery lineage: #8357, #8769, #9253, #11847, #12624, #14230, #14476, #14668, and #14740; configurable-write lineage: #8177, #13996, #21812, #22236, and #23562.

Suggested consolidation

Keep #67748 open with the salvage path of validating its now-focused head and closing #35213, #35244, #35299, #44210, #54200, #56423, and #60960 as duplicates; this does not disregard their keep_open reviews because #67748 carries the requested shared iterator, platform-safe setup, and cross-profile regression while the unrelated commit was removed. Keep #8177 open for the distinct configurable-write-root feature, and keep #75225 open for author action to add the requested manager-level patch regression and document retained delete refusal before deciding whether its #75130-specific framing should collapse into #67748.

Complex graph

flowchart TD
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I4759(["issue #4759 (closed)"])
    I35184(["issue #35184 (open)"])
    I44609(["issue #44609 (closed)"])
    I54195(["issue #54195 (open)"])
    subgraph Dup35213 ["PRs duplicating each other"]
        P35213["PR #35213 (open)"]
        P35244["PR #35244 (open)"]
        P44210["PR #44210 (open)"]
        P54200["PR #54200 (closed)"]
        P56420["PR #56420 (closed)"]
        P56423["PR #56423 (open)"]
        P60960["PR #60960 (open)"]
        P67748["PR #67748 (open)"]
        P75225["PR #75225 (open)"]
    end
    P67748 -->|best fix| I4759
    P67748 -->|best fix| I35184
    P67748 -->|best fix| I44609
    P67748 -->|best fix| I54195
    class I4759 closed
    class I35184 open
    class I44609 closed
    class I54195 open
    class P35213 open
    class P35244 open
    class P44210 open
    class P54200 closed
    class P56420 closed
    class P56423 open
    class P60960 open
    class P67748 open
    class P75225 open
    class P67748 best
    class P67748 best
    class P67748 best
    class P67748 best
    class P75225 best
    class P67748 target
    click I4759 "https://github.com/NousResearch/hermes-agent/issues/4759"
    click I35184 "https://github.com/NousResearch/hermes-agent/issues/35184"
    click I44609 "https://github.com/NousResearch/hermes-agent/issues/44609"
    click I54195 "https://github.com/NousResearch/hermes-agent/issues/54195"
    click P35213 "https://github.com/NousResearch/hermes-agent/pull/35213"
    click P35244 "https://github.com/NousResearch/hermes-agent/pull/35244"
    click P44210 "https://github.com/NousResearch/hermes-agent/pull/44210"
    click P54200 "https://github.com/NousResearch/hermes-agent/pull/54200"
    click P56420 "https://github.com/NousResearch/hermes-agent/pull/56420"
    click P56423 "https://github.com/NousResearch/hermes-agent/pull/56423"
    click P60960 "https://github.com/NousResearch/hermes-agent/pull/60960"
    click P67748 "https://github.com/NousResearch/hermes-agent/pull/67748"
    click P75225 "https://github.com/NousResearch/hermes-agent/pull/75225"

Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 35 pull requests and 9 issues in this complex. Each diff was read against this issue; Assessment working set: 438 kB of PR diffs, 102 kB of issue/PR text, 46 kB of discussion (81 comments), 69 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

[banozz0]

I reproduced this lookup approach against current main and found two safety gaps that the existing lookup-only tests do not cover.

1. Failed atomic batches can corrupt a directory-symlink installation. Discovery returns the lexical symlink path. Batch snapshotting follows that link, but rollback operates on the alias path: a failed one-operation patch can replace the symlink with a real directory, and a failed two-operation batch can leave the first mutation in the shared source while reporting that everything rolled back.

2. Background curation can misclassify the linked skill as locally owned. is_external_skill_path() resolves the candidate before comparing it with skills.external_dirs. If the symlink target is outside the configured external root, the external check returns false; after the read-before-write condition is satisfied, a curator-managed record can therefore allow an autonomous write to the shared source.

I built and independently reviewed a current-main fix that keeps the lexical alias for discovery, resolves the target only for batch snapshot/restore, and checks normalized lexical containment before resolved containment for external ownership. Public skill_manage(..., operations=[...]) regressions cover:

• successful in-place patch with the alias preserved;
• failed single-operation rollback preserving the link and source;
• failed multi-operation rollback undoing the first source mutation;
• background-review refusal for the linked external skill.

Verification: focused 4/4 passed; relevant skill-manager/discovery/ownership suite 168/168 passed; independent read-only review PASS with no findings. Reviewed diff SHA-256: 855b99050e78761952aad1c15d2fc2f52ed61f273095f3056fe654efe4f33880.

I have not pushed a competing branch or changed this PR. Happy to provide the reviewed four-file patch if useful; the important part is that the lookup fix lands together with the rollback and ownership safeguards.

[banozz0]

The reviewed patch offered above is now a current-main PR: #112321. It carries this PR's substitution (rebased onto _iter_skill_dirs, which both hunks here predate) plus the rollback and ownership safeguards, each with a public skill_manage(operations=[...]) regression. Happy to fold it into this PR instead if the maintainers prefer one head.