feat(skills): add opt-in read-only boundary for external skills

[insecurejezza]

Summary

Add an opt-in skills.external_read_only setting that prevents skill_manage from mutating externally owned skills or resolved targets outside the active profile's local skills directory.

External skills remain discoverable and readable through the skills index, skills_list, skill_view, slash commands, and normal skill loading.

The setting defaults to false, preserving existing in-place external skill updates.

Motivation

Users may keep canonical skills in a shared directory such as ~/.agents/skills, where the same skills are consumed by several agents. Hermes should be able to discover and use those skills without allowing its procedural-memory tooling to rewrite the canonical copies.

Filesystem permissions are too broad for this use case because the user may still want to edit those files directly or through ordinary filesystem tools.

Behaviour

When skills.external_read_only: true:

• edit, patch, delete, write_file, and remove_file refuse externally owned skills.
• create remains local and refuses configured or symlinked paths that resolve into external storage.
• Configured external roots nested under the local skills tree are still treated as externally owned.
• Local symlinks cannot be used to write to external targets.
• remove_file may unlink a final symlink entry without modifying its target.
• Local skills containing symlinks may still be deleted or archived safely.
• The boundary runs before write-approval staging.
• Ordinary file and terminal tools are unaffected. This is a skill_manage ownership policy, not a filesystem sandbox.

The boundary derives from <active HERMES_HOME>/skills, so named profiles use their own local skills directory.

Backward compatibility

This does not revert the external mutation support added for #4759 and #4381. The new restriction is explicitly opt-in, and no local duplicate is created when a write is refused.

With the default configuration:

skills:
  external_read_only: false

existing behaviour is unchanged.

Verification

scripts/run_tests.sh \
  tests/tools/test_skill_manager_tool.py \
  tests/tools/test_skill_usage.py \
  tests/agent/test_external_skills.py \
  tests/hermes_cli/test_config.py \
  tests/tools/test_website_policy.py -q

359 passed, 0 failed

Also verified:

• Ruff passes on all changed Python files.
• git diff --check passes.
• No new ty diagnostics compared with upstream; the change removes one existing annotation diagnostic.
• English and Simplified Chinese documentation are updated together.

[teknium1]

Thanks for adding an opt-in boundary while preserving the in-place external-update behavior introduced by #17512. The underlying gap exists on current main: foreground skill_manage patch reaches _atomic_write_text after external discovery (tools/skill_manager_tool.py:934-1010).

Problems

• tools/skill_manager_tool.py:845 resolves _skills_dir() before containment. If <HERMES_HOME>/skills is a symlink to canonical external storage, the resolved root and target are identical in ownership terms and the preflight permits writes. agent/skill_utils.py:483-501 also resolves configured external roots and drops one equal to the resolved local root, so is_external_skill_path() cannot classify that destination as external. This contradicts the stated symlinked-target boundary.

Suggested changes

• Preserve lexical active-root ownership or explicitly reject a symlinked active skills root when skills.external_read_only is enabled.
• Add an E2E profile-config test for a symlinked <HERMES_HOME>/skills root, covering create and an existing-skill mutation.

Automated hermes-sweeper review.

[teknium1]

Resolving the local root loses the fact that <HERMES_HOME>/skills may itself be a symlink to external storage. Because get_external_skills_dirs() resolves and skips a configured root equal to the resolved local root (agent/skill_utils.py:483-501), a skill below that symlink passes this check with external_read_only=true. Preserve lexical ownership or explicitly reject a symlinked active skills root, and cover that case with an E2E test.

[insecurejezza]

Good catch. Fixed in 6201e81.

The preflight now checks the lexical active skills-root entry before resolving containment. When skills.external_read_only is enabled and <HERMES_HOME>/skills is itself a symlink, every skill_manage mutation is refused while skill_view and discovery remain available.

Added an end-to-end profile-config regression covering create, patch, and skill_view through a symlinked active root. The focused related suite passes with 526 tests, and ruff, ty diff, Windows-footgun checks, and both documentation builds are clean.

[GottZ]

This was generated by AI during triage.

Summary

Two open PRs address the same opt-in boundary against skill_manage mutations of external skills while preserving writable-by-default behavior. #51412 guards configured external skill directories, while #65270 extends the boundary to creation, resolved targets, nested external roots, and symlink escapes.

Related pull requests

• feat(skills): add opt-in external_dirs_readonly guard #51412 duplicate — (+199/-2) — n/a: Adds the default-false skills.external_dirs_readonly guard, reuses is_external_skill_path, updates documentation, and tests all existing-skill mutation actions, but only classifies the discovered skill directory and does not protect create targets, resolved target escapes, or a symlinked active skills root. Despite the contributor keep_open review and the non-contributor APPROVED review on feat(skills): add opt-in external_dirs_readonly guard #51412, the current diff is substantially subsumed by feat(skills): add opt-in read-only boundary for external skills #65270's broader ownership checks.
• feat(skills): add opt-in read-only boundary for external skills #65270 related — (+743/-33) — n/a: Adds the default-false skills.external_read_only preflight across every mutation, covering 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. Keep open with a salvage path consistent with the contributor keep_open review: the current diff explicitly rejects a symlinked <HERMES_HOME>/skills root and includes profile-config tests for create and existing-skill mutation, directly addressing that review's documented blocker.

Duplicates

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

Suggested consolidation

Keep #65270 open with its concrete salvage path centered on the broader ownership boundary and the added symlinked-root regression coverage. Close #51412 as duplicate of #65270; this departs from its contributor keep_open review because its current diff remains materially narrower, while #65270 now contains the requested symlinked-root handling and tests.

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 P65270 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 (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), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.