fix(skills): use symlink-following traversal in _find_skill

[JonthanaHanh]

Fixes #75130

rglob("SKILL.md") does not descend into directory symlinks, making symlinked skill directories invisible to skill_manage operations (create, patch, delete) while the prompt loader (which uses os.walk(followlinks=True)) still sees them.

The reporter measured: rglob found 393 skills, os.walk(followlinks=True) found 399 -- 6 symlinked skills were invisible to the tool.

Changes

Replace rglob("SKILL.md") with iter_skill_index_files(skills_dir, "SKILL.md") in both:

• _find_skill() (line 657) -- primary skill lookup used by all skill_manage actions
• _find_skill_in_other_profiles() (line 796) -- cross-profile lookup for error messages

iter_skill_index_files already uses os.walk(followlinks=True) and is the canonical skill enumeration path used by the prompt builder and sync subsystem. This makes the tool's visibility match what the model sees.

Known remaining sites

The same rglob("SKILL.md") pattern exists in ~25 other files (skills_sync.py, skills_hub.py, skill_usage.py, profiles.py, gateway/run.py, etc.). These affect skill counting, sync, and usage tracking but not the core skill-manage tool path. A follow-up PR could migrate those to iter_skill_index_files for full consistency.

Test plan

• python -m pytest tests/tools/test_skill_manager_tool.py -xvs passes
• Skills installed via symlink are now found by skill_manage

[teknium1]

Thanks for aligning the skill-manager lookup with the established symlink-following discovery path. Current main still uses rglob("SKILL.md") in _find_skill() (tools/skill_manager_tool.py:657) and _find_skill_in_other_profiles() (:796), while prompt discovery uses iter_skill_index_files() (agent/prompt_builder.py:1669), whose walk follows links (agent/skill_utils.py:880).

Problems

• Please add a skill_manage regression test. Existing symlink coverage proves slash-command discovery (tests/agent/test_skill_commands.py:61), but does not exercise _find_skill() or a write through the manager.
• The PR description should not imply that deletion is enabled. _delete_skill() still calls _validate_delete_target() (tools/skill_manager_tool.py:1219), which explicitly refuses a symlinked skill directory (:234-239). That safety behavior should remain.

Suggested changes

• Add a test that patches a SKILL.md reached through a directory symlink under the skills root.
• Narrow the description to lookup/non-delete mutations, or explicitly document the retained delete refusal.

Automated hermes-sweeper review.

[GottZ]

This was generated by AI during triage.

Summary

34 PRs address or reference this issue complex, but none implements #75130's queue cap, TTL, notification, stale-anchor detection, or conflict handling. Most diffs concern external write roots or symlink-aware skill discovery; #75225 addresses only the measured manager-visibility contributor by replacing two residual rglob lookups with the shared iterator.

Related pull requests

• fix: skill_manage now finds skills from external_dirs #4868 [closed] fixes — (+12/-6) — n/a: Scans external skill roots but retains rglob, so it fixes neither symlink lookup nor queue lifecycle; closed because commit ad4feea already implemented external-root discovery on main.
• fix: align skill manager lookup with local skills dir #5255 [closed] partial — (+40/-7) — n/a: Aligns patched SKILLS_DIR lookup and delete cleanup with the matched root, without changing symlink traversal or pending proposals; author-closed after current tests and lookup code superseded its premise.
• fix(skills): respect local skill dir in skill manager #5284 [closed] fixes — (+45/-2) — n/a: Restores local-first plus external-root lookup but still uses rglob and does not affect the queue; closed after ad4feea landed equivalent external_dirs 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 mutations rather than queue handling or configurable creation; closed after main adopted mutation guards in e683c9d.
• fix(skills): follow symlinks with cycle guard across all skill discovery paths #7634 partial — (+75/-30) — n/a: Broadly migrates discovery and adds a cycle guard, but the 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: Best available fix for [Feature]: skills create / skills edit should support writing to a configurable target directory #4381/skill_manage(action='create') ignores skills.external_dirs, always writes to ~/.hermes/skills/ #21810, adding profile-aware default_write_dir discovery, ownership, curation, archive, tests, and docs; keep open per review while rebasing and validating the updated ownership model.
• fix(skills): follow symlinked local skill dirs #8357 [closed] fixes — (+49/-4) — n/a: Makes reader-side list/category/view discovery symlink-aware, not skill_manage or queue handling; closed as already implemented by 02aba4a and a884f6d.
• fix(skills): discover symlinked skill directories #8769 [closed] fixes — (+173/-19) — n/a: Adds cycle-safe reader traversal and tests but does not repair manager lookup or proposals; closed because the functional discovery fix had shipped through 02aba4a/a884f6d5d.
• fix: follow symlinks when scanning for SKILL.md #9253 [closed] fixes — (+18/-3) — n/a: Replaces three skills_tool scans with a symlink-following walker; the automated close verdict cites 02aba4a and shared-iterator coverage already on main.
• fix(skills): follow symlinks in iter_skill_index_files #11847 [closed] partial — (+1/-1) — n/a: Adds followlinks=True to the canonical iterator and remains relevant as the source commit 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: Broadly replaces discovery scans with a new symlink-following helper but lacks cycle protection; author-closed after partial fixes and focused successors superseded it.
• fix(skills): follow symlinks in iter_skill_index_files (salvage #11847) #14230 [merged] partial — (+2/-1) — n/a: Merged reference implementation establishing followlinks=True in iter_skill_index_files, which later manager fixes reuse, but it does not touch queue behavior.
• fix(skills): follow symlinks when discovering SKILL.md #14476 [closed] partial — (+60/-3) — n/a: Adds a cycle-safe skills_tool walker for list/view discovery; closed after 02aba4a and a884f6d covered those reader paths on main.
• fix(skills): follow symlinked category dirs consistently #14668 [closed] fixes — (+112/-7) — n/a: Migrates list, view, and slash-command scans to the shared iterator; 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: Merged best fix for Bug: symlinked skills under ~/.hermes/skills are omitted from skills_list and bare-name skill_view lookup #8293, aligning reader-side discovery with the shared symlink-following iterator and exposing the residual manager mismatch.
• fix(skills): let skill_manage modify skills in external_dirs in place #17512 [merged] best fix — (+171/-25) — n/a: Merged best fix for skill_manage patch/edit/delete can't find skills from external_dirs #4759, enabling in-place external-skill mutations and preventing local fallback copies, while leaving creation location and symlinked manager lookup separate.
• fix(skills): skill_manage create respects skills.external_dirs #21812 [closed] fixes — (+17/-2) — n/a: Routes creation to the first external root but omits the necessary external-relative result handling; author-closed after abandoning that authoring policy.
• 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, routing creation to the first external root with correct result paths and tests; closed because a cleaner configurable-root alternative exists in feat: support configurable default write directory for skills #8177.
• fix(skill-tools): follow symlinks in skill discovery via rglob_follow #23484 [closed] partial — (+332/-18) — n/a: Adds symlink-aware management/usage traversal but carries 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 fixes — (+16/-8) — n/a: Routes creation implicitly to the first external root with stale SKILLS_DIR fallback; despite its keep_open review, the diff has no independent salvage beyond the explicit, profile-aware feat: support configurable default write directory for skills #8177 design.
• docs(skills): clarify external dir mutations #29411 [closed] related — (+3/-2) — n/a: Documents create-local/update-in-place behavior for external skills; closed because its exact docs change was salvaged with attribution into 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 for fix(skills): let skill_manage modify skills in external_dirs in place #17512's external-mutation contract, with no queue or lookup code change.
• fix(skills): follow symlinks during skill discovery (#35184) #35213 fixes — (+62/-8) — n/a: Applies the two manager iterator substitutions but has non-portable symlink coverage; despite its keep_open review, its production fix is superseded by the fully hardened fix(skills): follow symlinks in skill management #67748.
• fix(skill_manager): follow symlinks when discovering skills (#35184) #35244 duplicate — (+110/-4) — n/a: Earlier canonical two-site iterator swap with useful active/cross-profile tests; despite its keep_open review, fix(skills): follow symlinks in skill management #67748 explicitly supersedes it and incorporates the requested platform-safe coverage.
• fix: rglob follow symlinks for skill discovery #35299 fixes — (+34/-7) — n/a: Fixes manager and usage scans through a parallel rglob_follow helper without tests; despite its keep_open review, reuse of the established iterator is already provided by fix(skills): follow symlinks in skill management #67748.
• fix(skills): follow directory symlinks in SKILL.md discovery #44129 fixes — (+211/-97) — n/a: Broadly centralizes many discovery paths on the shared iterator, but its keep_open review identifies missing current-head sites and platform guards, so retain only as a broader follow-up salvage path.
• fix(skills): use os.walk(followlinks=True) in _find_skill for symlinked skill dirs #44210 fixes — (+73/-8) — n/a: Now applies the shared iterator to both manager lookups with symlink tests; despite its keep_open review, it duplicates the earlier and review-complete fix(skills): follow symlinks in skill management #67748 cut.
• perf(profiles): allow lightweight profile listing #52433 [closed] related — (+214/-29) — n/a: Adds lightweight profile listing that can skip recursive skill counts, but changes neither pending proposals nor manager lookup; closed and unrelated to Pending skill-proposal queue grows unbounded and self-invalidates when skills.write_approval is enabled (357 in 8 days, 21% dead) #75130's cause.
• fix(skills): follow symlinks when discovering skills in external dirs #54200 [closed] fixes — (+57/-4) — n/a: Applies the same two-site iterator fix for external symlinks; closed explicitly as duplicate of fix(skill_manager): follow symlinks when discovering skills (#35184) #35244 and transitively superseded by fix(skills): follow symlinks in skill management #67748.
• fix(skill_manage): _find_skill follows symlinked skill directories #56420 [closed] fixes — (+1129/-22) — n/a: Contains the correct two lookup substitutions inside a large unrelated historical diff; closed and resubmitted as fix(skill_manage): _find_skill follows symlinked skill directories #56423.
• fix(skill_manage): _find_skill follows symlinked skill directories #56423 fixes — (+1129/-22) — n/a: Repeats the correct manager fix but remains conflict-heavy and broadly unrelated; despite its keep_open review, only its narrow lookup idea is salvageable and already isolated in fix(skills): follow symlinks in skill management #67748.
• Fix _find_skill: use iter_skill_index_files to follow symlinks #60960 partial — (+685/-5) — n/a: Changes only the active finder while carrying unrelated Codex delegation and omitting cross-profile coverage; despite the automated keep_open verdict, fix(skills): follow symlinks in skill management #67748 supplies the requested focused two-finder fix and tests.
• 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/skill_manage cannot find symlinked skills in skills.external_dirs #54195, narrowly migrating both manager lookups with platform-safe active and cross-profile regressions after the unrelated commit was split out; keep open as the canonical related symlink fix.
• fix(skills): use symlink-following traversal in _find_skill #75225 best fix — (+4/-4) — n/a: Recorded best available 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 secondary visibility mechanism, replacing both residual manager rglob scans, but it leaves queue growth and stale anchors untouched; consistent with the keep_open verdict, add a manager patch regression and document that delete refusal remains.

Duplicates

Reader-discovery chain: #8357, #8769, #9253, #11847, #12624, #14476, and #14668 converge on merged #14230/#14740. Manager-lookup chain: #35213, #35244, #35299, #44210, #54200, #56420, #56423, #60960, and #75225 overlap #67748, while creation-root #21812/#22236/#23562 overlaps the broader #8177 design.

Suggested consolidation

Author action on #75225: add a platform-safe regression that patches SKILL.md through a directory symlink, narrow the claimed scope to non-delete mutations, and retain _validate_delete_target's symlink refusal; it remains only a partial mitigation for #75130, whose queue lifecycle needs a separate fix. Keep #67748 and #8177 open on their distinct salvage paths, close the listed manager-lookup and creation-root competitors as duplicates, and do not reopen the already implemented or salvaged closed PRs.

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
    I35184(["issue #35184 (open)"])
    I75130(["issue #75130 (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
    P75225 -->|fixes| I35184
    P75225 -->|best fix| I75130
    class I35184 open
    class I75130 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 P75225 target
    click I35184 "https://github.com/NousResearch/hermes-agent/issues/35184"
    click I75130 "https://github.com/NousResearch/hermes-agent/issues/75130"
    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 34 pull requests and 8 issues in this complex. Each diff was read against this issue; Assessment working set: 435 kB of PR diffs, 101 kB of issue/PR text, 46 kB of discussion (78 comments), 65 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.