Skip to content

fix(skills): rmtree scope guard + default pre_update_backup to true - #48340

Closed
Kewe63 wants to merge 1 commit into
NousResearch:mainfrom
Kewe63:fix/48200-skill-rmtree-scope-guard
Closed

fix(skills): rmtree scope guard + default pre_update_backup to true#48340
Kewe63 wants to merge 1 commit into
NousResearch:mainfrom
Kewe63:fix/48200-skill-rmtree-scope-guard

Conversation

@Kewe63

@Kewe63 Kewe63 commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Summary

A hermes update --yes run on an Ubuntu VPS silently wiped the user's entire
~/.hermes/ directory: .env (Telegram bot token, OpenRouter key, Granola API
key, GitHub PAT), MEMORY.md, SOUL.md, kanban.db, custom skills, scripts,
and the rest of the install. Recovery required restoring a Hetzner snapshot
from 12 hours earlier (#48200).

The fix is defense in depth — two changes that together prevent the silent-wipe
failure mode:

  1. Scope guard_rmtree_writable now refuses to remove anything outside
    SKILLS_DIR. A path-computation error (bad path join, malicious
    bundled-manifest entry, stale path after an exception) becomes a loud
    ValueError instead of silent data loss.
  2. Default backupupdates.pre_update_backup defaults to true. A few
    minutes of zip time per update is negligible compared to silent total data
    loss. The flag is still overridable; hermes update --no-backup still works
    for one-off opt-out.

Changes

  • tools/skills_sync.py_rmtree_writable: resolves the target path and
    checks it is the skills root itself or strictly under it; raises ValueError
    referencing hermes update --yes wiped entire ~/.hermes/ directory (.env, MEMORY.md, kanban.db, skills, scripts — all gone) #48200 otherwise. All five existing call sites pass paths under
    SKILLS_DIR, so this is a no-op for current code.
  • hermes_cli/config.py: changed updates.pre_update_backup default from
    false to true.
  • hermes_cli/main.py_run_pre_update_backup: tightened comment to
    reflect the new default and explain fallback semantics (older custom configs
    that omit the field still get the safe default).

How to Test

# Scope-guard tests
pytest tests/tools/test_skills_sync.py::TestRmtreeWritableScopeGuard -v
# ✅ 5/5 passed

# Default backup test
pytest tests/hermes_cli/test_backup.py::TestRunPreUpdateBackup -v
# ✅ 6/6 passed (1 renamed + 5 existing unchanged)

# Both affected files
pytest tests/tools/test_skills_sync.py tests/hermes_cli/test_backup.py -q
# ✅ 178/178 passed

# Lint
ruff check \
  tools/skills_sync.py hermes_cli/config.py hermes_cli/main.py \
  tests/tools/test_skills_sync.py tests/hermes_cli/test_backup.py
# ✅ All checks passed

The 5 new TestRmtreeWritableScopeGuard tests patch SKILLS_DIR to a temp
directory and verify: (a) the guard raises ValueError for /, the hermes
home itself, and a sibling of the skills root, and (b) allows the skills root
and any strict subdirectory.

test_default_enabled_creates_backup (renamed from
test_default_disabled_is_silent) drives the new default.
test_config_disabled_is_silent still proves that an explicit
pre_update_backup: false opt-out is honored.


Checklist

  • Tests pass — 178/178 in the two affected files
  • ruff check — PASS, 0 warnings
  • Follows Conventional Commits
  • Changes scoped to this fix only — 5 files

Risk & Impact

Low. The scope guard is a fail-safe — the existing five call sites already
follow the contract, so current behavior is unchanged. Future violations become
a loud ValueError instead of silent data loss. The default-backup change is
opt-out; hermes update --no-backup still works for one-off cases.

Type: 🐛 Bug fix (data-loss prevention)
Closes: #48200

…ousResearch#48200)

Defense-in-depth fix for the silent wipe of ~/.hermes/ documented in
NousResearch#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 NousResearch#48200
@daimon-nous daimon-nous Bot added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard tool/skills Skills system (list, view, manage) area/config Config system, migrations, profiles P1 High — major feature broken, no workaround labels Jun 18, 2026
@daimon-nous

daimon-nous Bot commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Related: #48200 (the data-loss incident this Closes), #48271 (open scope-guard PR for the same _rmtree_writable boundary).

This PR is a superset of #48271: it implements the same _rmtree_writable scope guard (resolve target, raise ValueError if it equals or escapes SKILLS_DIR) and additionally flips updates.pre_update_backup to default true in hermes_cli/config.py. Not marking either as a duplicate — same scope-guard mechanism, but this one adds the backup-default layer that would have made the #48200 wipe recoverable. Maintainer to choose between the focused guard (#48271) and this guard+backup-default combination.

Verified against main: _rmtree_writable has no scope guard and pre_update_backup still defaults to false, so both legs address live conditions.

@teknium1

Copy link
Copy Markdown
Contributor

Merged via #48423...

@teknium1 teknium1 closed this Jun 18, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Correction to my previous comment: merged via #48424 (not #48423). Your commit landed on current main with authorship preserved (f1254c8). I added one hardening commit on top — the guard now refuses SKILLS_DIR itself, not just out-of-tree paths, since no caller passes the root and a dest collapsing to it was the residual footgun. Thanks for the fast turnaround on a P0 data-loss fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard P1 High — major feature broken, no workaround tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants