fix(skills): add opt-in external read-only boundary - #91318
Closed
morelandreas wants to merge 2 commits into
Closed
morelandreas wants to merge 2 commits into
morelandreas wants to merge 2 commits into
Conversation
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
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.
Summary
This is a current-main rebase and salvage of #65270, preserving the two original commits and authorship from @insecurejezza. It supersedes the narrower approach in #51412: that PR guards configured skill directories, but not all resolved mutation targets or a symlinked active skills root.
Reproduction
On current
main(40643cbaf9b767af146694131ffb8f8160f25e1c), a temporaryHERMES_HOMEconfigured with bothskills.external_dirsandskills.external_read_only: truestill allowedskill_manage(action="patch")to mutate an externalSKILL.md. The regression assertion expected refusal and failed because the call returnedsuccess: true.Behavior contract
skills.external_read_only: false), preserving intentional foreground edits of external skills.skill_managefails closed forcreate,edit,patch,delete,write_file, andremove_filewhen the skill or resolved mutation target escapes the active profile skills root.remove_filecan still unlink a final local symlink without touching its target.skills_list,skill_view, slash-command discovery, and normal external discovery stay available. This is askill_manageownership policy, not an OS filesystem sandbox.Verification
scripts/run_tests.sh tests/tools/test_skill_manager_tool.py -k external_read_only -q— 19 passedscripts/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— 176 passed, 0 failedpython3 -m py_compile tools/skill_manager_tool.py hermes_cli/config_defaults.py agent/skill_utils.py agent/prompt_builder.pygit diff --check origin/main...HEADLocal
ruffis unavailable in this worktree and Nix is not installed; upstream CI remains the lint gate.Rollback
Set
skills.external_read_only: falseto restore the existing foreground-edit behavior, or revert these two commits. No change affects ordinary file or terminal tools.