feat(skills): add opt-in external_dirs_readonly guard

[chdlc]

What

Adds skills.external_dirs_readonly as an opt-in guard for skills in external directories.

Skills in external_dirs are typically managed by an external package manager (npx skills, git, etc.). When enabled, the guard prevents skill_manage from modifying them, avoiding changes that may be silently overwritten on the next update.

The default remains false to preserve the existing foreground-edit contract. Users can enable protection with:

skills:
  external_dirs_readonly: true

The guard reuses the canonical is_external_skill_path helper. New skills are still created locally.

How to test

1. Add an external directory to config.yaml:

   skills:
     external_dirs:
       - ~/test-external-skills

2. Create a skill there and run Hermes.

3. Set skills.external_dirs_readonly: true and try to patch it via skill_manage. The operation should fail with an error mentioning external_dirs_readonly.

4. Remove the flag or set it to false. The foreground patch should be allowed again.

5. Run:

   python -m pytest tests/tools/test_skill_manager_tool.py -v

   Expected: 118 passed, including real temporary-HERMES_HOME config-path coverage.

Platforms tested

• Linux (WSL2)

Related

• Relates to [Bug]: Hermes unexpectedly modifies its own skills/system prompts during normal task execution, even when the user request is unrelated to agent configuration. #32497 (Hermes unexpectedly modifies its own skills during normal task execution)
• Relates to Feature: Immutable/protected skills - prevent agent from modifying critical skills without user approval #25083 (immutable skills feature request)

[tonydwb]

Code Review Summary

Verdict: Approved

Add external_dirs_readonly config flag (default true).

Looks Good

• Good safety feature: prevents agent from modifying skills in external directories
• Properly implemented with config flag and skill_manage integration
• Default (True) is the safe choice
• Well-documented with clear rationale
• Tests verify the guard behavior

---

Reviewed by Hermes Agent

[teknium1]

Thanks for the focused guard and mutation coverage. The default needs a maintainer design decision before this can be salvaged.

Problems

• hermes_cli/config.py:2014 changes the default to reject foreground edits, but current main deliberately allows them: tools/skill_manager_tool.py:302-307 limits the external read-only boundary to autonomous curation, and website/docs/user-guide/features/skills.md:333-334 documents update-in-place. Commit 8c8fc6c1 chose this specifically to avoid silently duplicating external skills locally.
• Current main already owns this classification in agent/skill_utils.py:579-596 as is_external_skill_path; the new is_external_skill_dir duplicates that mechanism.
• The behavior change needs a corresponding update to website/docs/user-guide/features/skills.md:333-334 and an integration test using a temporary HERMES_HOME, rather than only mocked config/discovery.

Suggested changes

• Resolve whether foreground external-directory edits should change from the established contract. If approved, reuse is_external_skill_path, document the changed default, and add the real config-path test.

Automated hermes-sweeper review.

[teknium1]

Making this default true reverses the current foreground-edit contract: tools/skill_manager_tool.py:302-307 permits user-directed external-skill edits, and commit 8c8fc6c1 deliberately removed the old read-only gate to avoid duplicate local skills. Please obtain a maintainer decision before changing this default.

[GottZ]

This was generated by AI during triage.

Summary

Two PRs address the same opt-in protection against skill_manage mutating external skills while preserving the existing writable-by-default contract. #51412 guards skills discovered under configured external directories, whereas #65270 implements the broader ownership boundary needed to cover configured roots, resolved targets, nested external roots, and symlink escapes.

Related pull requests

• #51412 related — (+199/-2) — superseded duplicate: Adds skills.external_dirs_readonly, reuses is_external_skill_path, documents the default-false behavior, and tests all existing-skill mutation actions, but its guard only classifies the skill directory and does not cover resolved mutation targets or a symlinked active skills root. Despite the visible APPROVED review on #51412, the diff is materially narrower than #65270; the contributor review's earlier default/helper/docs/integration concerns are addressed in the current revision, but the broader ownership cases remain absent.
• #65270 duplicate — (+743/-33) — preferred implementation: Adds default-false skills.external_read_only preflight enforcement across every mutation, including create, configured roots nested under local storage, resolved targets outside the active profile, local symlink escapes, and a symlinked active skills root, while preserving reads and safe final-symlink unlinking. The current diff directly addresses the contributor keep_open review on #65270 by rejecting a symlinked <HERMES_HOME>/skills root and adding profile-config coverage for both create and an existing-skill mutation.

Duplicates

#51412 and #65270 implement the same opt-in external-skill read-only feature; #65270 substantially subsumes #51412 with a resolved ownership boundary and symlink-aware coverage.

Suggested consolidation

Merge #65270 after normal CI verification because it preserves the established default while covering the full mutation and path-resolution boundary, including the contributor-reviewed symlinked-root gap. Close #51412 as superseded by #65270; despite its APPROVED review, its diff provides only the narrower configured-external-directory guard.

Complex graph

flowchart LR
    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
    subgraph Dup51412 ["PRs duplicating each other"]
        P51412["PR #51412 (open)"]
        P65270["PR #65270 (open)"]
    end
    class P51412 open
    class P65270 open
    class P51412 target
    click P51412 "https://github.com/NousResearch/hermes-agent/pull/51412"
    click P65270 "https://github.com/NousResearch/hermes-agent/pull/65270"

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 or no verify verdict yet (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 59 kB of PR diffs, 4 kB of issue/PR text, 3 kB of discussion (3 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.