fix(skills): let skill_manage modify skills in external_dirs in place#17512
Merged
Conversation
…rs in place Closes #4759, closes #4381. Mutating actions (patch, edit, write_file, remove_file, delete) used to refuse skills that lived under `skills.external_dirs` with 'Skill X is in an external directory and cannot be modified. Copy it to your local skills directory first.' Faced with that error, the agent would fall back to action='create', which always writes under ~/.hermes/skills/ — producing a silent duplicate of the external skill in the local store. Fix: drop the read-only gate. `skills.external_dirs` is configured by the user; if they pointed it at a directory, they already said 'these are my skills, treat them the same.' Filesystem permissions handle the genuine read-only case (write fails, agent sees the error). - New _containing_skills_root() resolves whichever dir actually contains the skill; _delete_skill uses it to bound empty-category cleanup so an external root is never rmdir'd. - _create_skill behavior is unchanged: new skills still land in local SKILLS_DIR only. Fewer moving parts. - Seven new TestExternalSkillMutations tests covering patch/edit/write_file/ remove_file/delete/create against a mocked two-root layout + a category rmdir-safety check.
This was referenced Apr 29, 2026
donald131
pushed a commit
to donald131/hermes-agent
that referenced
this pull request
May 2, 2026
…rs in place (NousResearch#17512) Closes NousResearch#4759, closes NousResearch#4381. Mutating actions (patch, edit, write_file, remove_file, delete) used to refuse skills that lived under `skills.external_dirs` with 'Skill X is in an external directory and cannot be modified. Copy it to your local skills directory first.' Faced with that error, the agent would fall back to action='create', which always writes under ~/.hermes/skills/ — producing a silent duplicate of the external skill in the local store. Fix: drop the read-only gate. `skills.external_dirs` is configured by the user; if they pointed it at a directory, they already said 'these are my skills, treat them the same.' Filesystem permissions handle the genuine read-only case (write fails, agent sees the error). - New _containing_skills_root() resolves whichever dir actually contains the skill; _delete_skill uses it to bound empty-category cleanup so an external root is never rmdir'd. - _create_skill behavior is unchanged: new skills still land in local SKILLS_DIR only. Fewer moving parts. - Seven new TestExternalSkillMutations tests covering patch/edit/write_file/ remove_file/delete/create against a mocked two-root layout + a category rmdir-safety check.
02356abc
pushed a commit
to 02356abc/hermes-agent
that referenced
this pull request
May 14, 2026
…rs in place (NousResearch#17512) Closes NousResearch#4759, closes NousResearch#4381. Mutating actions (patch, edit, write_file, remove_file, delete) used to refuse skills that lived under `skills.external_dirs` with 'Skill X is in an external directory and cannot be modified. Copy it to your local skills directory first.' Faced with that error, the agent would fall back to action='create', which always writes under ~/.hermes/skills/ — producing a silent duplicate of the external skill in the local store. Fix: drop the read-only gate. `skills.external_dirs` is configured by the user; if they pointed it at a directory, they already said 'these are my skills, treat them the same.' Filesystem permissions handle the genuine read-only case (write fails, agent sees the error). - New _containing_skills_root() resolves whichever dir actually contains the skill; _delete_skill uses it to bound empty-category cleanup so an external root is never rmdir'd. - _create_skill behavior is unchanged: new skills still land in local SKILLS_DIR only. Fewer moving parts. - Seven new TestExternalSkillMutations tests covering patch/edit/write_file/ remove_file/delete/create against a mocked two-root layout + a category rmdir-safety check.
jsboige
pushed a commit
to jsboige/hermes-agent
that referenced
this pull request
May 14, 2026
…rs in place (NousResearch#17512) Closes NousResearch#4759, closes NousResearch#4381. Mutating actions (patch, edit, write_file, remove_file, delete) used to refuse skills that lived under `skills.external_dirs` with 'Skill X is in an external directory and cannot be modified. Copy it to your local skills directory first.' Faced with that error, the agent would fall back to action='create', which always writes under ~/.hermes/skills/ — producing a silent duplicate of the external skill in the local store. Fix: drop the read-only gate. `skills.external_dirs` is configured by the user; if they pointed it at a directory, they already said 'these are my skills, treat them the same.' Filesystem permissions handle the genuine read-only case (write fails, agent sees the error). - New _containing_skills_root() resolves whichever dir actually contains the skill; _delete_skill uses it to bound empty-category cleanup so an external root is never rmdir'd. - _create_skill behavior is unchanged: new skills still land in local SKILLS_DIR only. Fewer moving parts. - Seven new TestExternalSkillMutations tests covering patch/edit/write_file/ remove_file/delete/create against a mocked two-root layout + a category rmdir-safety check.
This was referenced May 15, 2026
dannyJ848
pushed a commit
to dannyJ848/hermes-agent
that referenced
this pull request
May 17, 2026
…rs in place (NousResearch#17512) Closes NousResearch#4759, closes NousResearch#4381. Mutating actions (patch, edit, write_file, remove_file, delete) used to refuse skills that lived under `skills.external_dirs` with 'Skill X is in an external directory and cannot be modified. Copy it to your local skills directory first.' Faced with that error, the agent would fall back to action='create', which always writes under ~/.hermes/skills/ — producing a silent duplicate of the external skill in the local store. Fix: drop the read-only gate. `skills.external_dirs` is configured by the user; if they pointed it at a directory, they already said 'these are my skills, treat them the same.' Filesystem permissions handle the genuine read-only case (write fails, agent sees the error). - New _containing_skills_root() resolves whichever dir actually contains the skill; _delete_skill uses it to bound empty-category cleanup so an external root is never rmdir'd. - _create_skill behavior is unchanged: new skills still land in local SKILLS_DIR only. Fewer moving parts. - Seven new TestExternalSkillMutations tests covering patch/edit/write_file/ remove_file/delete/create against a mocked two-root layout + a category rmdir-safety check.
19 tasks
gweeteve
pushed a commit
to gweeteve/hermes-agent
that referenced
this pull request
Jun 2, 2026
…rs in place (NousResearch#17512) Closes NousResearch#4759, closes NousResearch#4381. Mutating actions (patch, edit, write_file, remove_file, delete) used to refuse skills that lived under `skills.external_dirs` with 'Skill X is in an external directory and cannot be modified. Copy it to your local skills directory first.' Faced with that error, the agent would fall back to action='create', which always writes under ~/.hermes/skills/ — producing a silent duplicate of the external skill in the local store. Fix: drop the read-only gate. `skills.external_dirs` is configured by the user; if they pointed it at a directory, they already said 'these are my skills, treat them the same.' Filesystem permissions handle the genuine read-only case (write fails, agent sees the error). - New _containing_skills_root() resolves whichever dir actually contains the skill; _delete_skill uses it to bound empty-category cleanup so an external root is never rmdir'd. - _create_skill behavior is unchanged: new skills still land in local SKILLS_DIR only. Fewer moving parts. - Seven new TestExternalSkillMutations tests covering patch/edit/write_file/ remove_file/delete/create against a mocked two-root layout + a category rmdir-safety check.
mbs-vhs
added a commit
to mbs-vhs/hermes-agent
that referenced
this pull request
Jun 4, 2026
…AWD-1110) The background skill-review fork can write into live dev repos that operators expose via skills.external_dirs (the 2026-06-03 incident: bg-review wrote references/ files + patched SKILL.md directly under ~/dev/devops-process/skills, exposed via external_dirs on all 10 profiles). Add skills.external_dirs_readonly (default True). When on, the five skill_manage mutating actions (edit/patch/write_file/remove_file/delete) refuse any skill whose resolved directory lies under a skills.external_dirs entry, returning an error WITHOUT writing. Reads/listing/consumption and create (always local-only) are unaffected. Set the flag false to restore the legacy in-place behavior (PR NousResearch#17512). The gate is a single chokepoint (_assert_writable) called after _find_skill resolves the target; realpaths are compared so a symlinked external dir can't slip past. Default reader mirrors _guard_agent_created_enabled but fails safe-ON. Tests: new TestExternalDirsReadonlyGate + TestExternalDirsReadonlyFlag classes (block on external / allow on local / create stays local / flag false = legacy / symlink-robust / config default-true). Existing TestExternalSkillMutations now pins the flag off to keep covering the legacy path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Egavasyug
pushed a commit
to Egavasyug/hermes-agent
that referenced
this pull request
Jun 10, 2026
…rs in place (NousResearch#17512) Closes NousResearch#4759, closes NousResearch#4381. Mutating actions (patch, edit, write_file, remove_file, delete) used to refuse skills that lived under `skills.external_dirs` with 'Skill X is in an external directory and cannot be modified. Copy it to your local skills directory first.' Faced with that error, the agent would fall back to action='create', which always writes under ~/.hermes/skills/ — producing a silent duplicate of the external skill in the local store. Fix: drop the read-only gate. `skills.external_dirs` is configured by the user; if they pointed it at a directory, they already said 'these are my skills, treat them the same.' Filesystem permissions handle the genuine read-only case (write fails, agent sees the error). - New _containing_skills_root() resolves whichever dir actually contains the skill; _delete_skill uses it to bound empty-category cleanup so an external root is never rmdir'd. - _create_skill behavior is unchanged: new skills still land in local SKILLS_DIR only. Fewer moving parts. - Seven new TestExternalSkillMutations tests covering patch/edit/write_file/ remove_file/delete/create against a mocked two-root layout + a category rmdir-safety check.
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
2 participants
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.
Closes #4759, closes #4381.
Summary
skill_managepatch/edit/write_file/remove_file/delete now work on skills that live underskills.external_dirs, editing in place instead of refusing with "external directory, copy it to your local skills first." The old gate is what caused the agent to fall back toaction='create'and silently duplicate the external skill into~/.hermes/skills/.Root cause
_find_skill()already scansget_all_skills_dirs()(local + external), so lookup worked._is_local_skill()and refused.action='create'with the same name → lands in~/.hermes/skills/because_create_skillalways writes to localSKILLS_DIR. That's the duplicate copy users saw.Changes
tools/skill_manager_tool.py: remove the read-only gate from_edit_skill,_patch_skill,_write_file,_remove_file,_delete_skill._is_local_skill()with_containing_skills_root()so_delete_skill's empty-category cleanup bounds correctly at whichever root holds the skill (never rmdir's an external root)._create_skillunchanged — new skills still land in localSKILLS_DIRonly. Fewer moving parts than adding adefault_write_dirconfig key.TestExternalSkillMutationstests (patch/edit/write_file/remove_file/delete in place, empty-category cleanup, create-still-local).Validation
~/.hermes/skills/scripts/run_tests.sh tests/tools/test_skill_manager_tool.py tests/agent/test_external_skills.py ...E2E verified against a real
HERMES_HOMEwithskills.external_dirspointing at a temp vault dir — all six mutating operations wrote in place, local dir stayed empty, external vault root was never removed.