fix(skills): rmtree scope guard + default pre_update_backup to true (#48200) - #48424
Merged
Conversation
…48200) Defense-in-depth fix for the silent wipe of ~/.hermes/ documented in #48200. A `hermes update --yes` run silently destroyed a user's .env, MEMORY.md, kanban.db, custom skills, and scripts. Two changes: 1. `_rmtree_writable` in tools/skills_sync.py now refuses to rmtree anything outside SKILLS_DIR (the HERMES_HOME/skills/ root). All five call sites pass paths under SKILLS_DIR, so the guard is a no-op for current code and a loud, recoverable failure for any future regression (bad path join, malicious bundled manifest, stale path in scope after an exception). 2. The default `updates.pre_update_backup` flips from false to true in hermes_cli/config.py. A few minutes of zip per update is negligible compared to silent total data loss. Still overridable; --no-backup still works for one-off opt-out. Five new tests in TestRmtreeWritableScopeGuard (root path, hermes home, sibling dir, skills root itself, subdir) plus a flipped `test_default_enabled_creates_backup` in test_backup.py. 178/178 tests pass in the two affected files. Public method signatures unchanged, no test-stub blast radius. Closes #48200
…-tree The salvaged guard allowed _rmtree_writable(SKILLS_DIR) itself. No call site ever passes the root — every site passes a skill subdir or its .bak sibling — so allowing the root only preserves the #48200 footgun (a dest that collapses to the root wipes every installed skill). Require a strict strict-child relationship and update the test that documented the nonexistent 'full reset' capability.
Contributor
🔎 Lint report:
|
| Rule | Count |
|---|---|
unresolved-import |
1 |
First entries
tests/tools/test_skills_sync.py:5: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
✅ Fixed issues: none
Unchanged: 5770 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
alt-glitch
added
type/security
Collaborator
|
Related: #48200 (the P0 data-loss bug this fixes), #48271 (scope-guard-only fix), #48340 (same scope-guard + backup-default approach). This is the authoritative version of the #48200 fix — combining the strict-child scope guard on |
egilewski
approved these changes
Jun 18, 2026
egilewski
left a comment
egilewski
left a comment
Contributor
There was a problem hiding this comment.
looks mergeable
Checked against current main (ef4b897a1843cd32c4f141f55db60f0f0602cc98) and PR head cc9a86ad3d4de23c98670dd0e88516b0d15aeb9a.
git merge-tree --write-tree refs/remotes/upstream/main refs/remotes/upstream/pr/48424merged cleanly.- A synthetic current-main probe reproduced the dangerous path:
_rmtree_writable(HERMES_HOME)removed the temp Hermes home and.env; the same probe on PR head raisedValueErrorand preserved.env. /home/mac/hermes-agent/.venv/bin/python -B -m pytest -p no:cacheprovider -o addopts= tests/tools/test_skills_sync.py tests/hermes_cli/test_backup.py -qpassed:178 passed in 25.36s.- CodeRabbit clean-pass completed with no findings.
Worth improving
- Refresh stale wording that still says pre-update backups are off/default false in
_run_pre_update_backupdocs, the--backuphelp text, and a couple of test comments.
Signed: GPT-5.5-xhigh in Codex
4 tasks
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
4 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.
Summary
hermes updatecan no longer silently wipe~/.hermes/. Two independent defenses for the #48200 data-loss class:_rmtree_writable— refuses to remove anything that isn't a strict child ofHERMES_HOME/skills/. The five call sites all pass a skill subdir or its.baksibling; if any ever computes a path that escapes (bad join, missing default, mid-flight exception, degeneratedestcollapsing to the skills root), the helper now raises a loudValueErrorinstead ofshutil.rmtree(~/.hermes).pre_update_backupdefaults totrue— everyhermes updatenow zipsHERMES_HOMEtobackups/before any destructive step. Opt out with--no-backuporpre_update_backup: false.Changes
tools/skills_sync.py: strict-child scope guard in_rmtree_writable.hermes_cli/config.py+hermes_cli/main.py: flippre_update_backupdefault totrue(incl. the missing-key fallback).tests/tools/test_skills_sync.py,tests/hermes_cli/test_backup.py: regression tests for both.Validation
skills/.baksibling underskills/skills/root itself~/.hermes/(HERMES_HOME)skills.bak(the.with_suffixsibling, lands in HERMES_HOME)/E2E verified against a real temp
HERMES_HOME: legitimate removals succeed, every escape is refused,.envandMEMORY.mdsurvive.pre_update_backupdefault confirmedTrue. 124 targeted tests pass.Follow-up on top of the salvage
The salvaged guard allowed
_rmtree_writable(SKILLS_DIR)itself ("full reset"). No caller passes the root, so allowing it only preserved the #48200 footgun — adestcollapsing to the root would wipe every installed skill. Tightened to a strict-child relationship and updated the test accordingly.Salvaged from #48340 by @Kewe63 (commit cherry-picked, authorship preserved) + one follow-up hardening commit. Closes #48200.
Infographic