Skip to content

fix(cli): remove keep=1 from pre-update snapshot to preserve prior snapshots - #58678

Open
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-58672-snapshot-keep
Open

liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-58672-snapshot-keep

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

Remove the explicit keep=1 argument from the pre-update safety snapshot call in hermes_cli/main.py. This one-liner was causing hermes update to delete ALL previously retained quick snapshots (state.db, config.yaml, pairing JSONs, etc.) on every update, defeating the purpose of the snapshot retention policy.

The default keep=20 (via _QUICK_DEFAULT_KEEP) is the correct limit for all callers — the comment on line 882-884 of backup.py documents that callers with "known high-churn safety snapshots" can pass a smaller value, but keep=1 was an over-aggressive choice that wiped the entire snapshot history.

Related Issue

Fixes #58672

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/main.py:9710 — Remove keep=1 from create_quick_snapshot(label="pre-update") so it uses the default _QUICK_DEFAULT_KEEP = 20.
  • tests/hermes_cli/test_backup.py — Add test_pre_update_snapshot_preserves_prior_snapshots regression test verifying that a pre-update snapshot coexists with 5 prior snapshots instead of pruning them.

How to Test

  1. Create 5 quick snapshots: hermes snapshot create --label test-{1..5}
  2. Run hermes update (which triggers a pre-update snapshot)
  3. Run hermes snapshot list — should show 6 snapshots (5 manual + 1 pre-update), not just 1
  4. pytest tests/hermes_cli/test_backup.py::TestQuickSnapshot -xvs — all 20 tests should pass

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/hermes_cli/test_backup.py -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A (snapshot logic is cross-platform; no OS-specific code touched)
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

…apshots

The pre-update safety snapshot in hermes_cli/main.py passed keep=1 to
create_quick_snapshot(), which caused _prune_quick_snapshots() to delete
ALL previously retained quick snapshots on every hermes update.  The
default keep=20 (defined in _QUICK_DEFAULT_KEEP) is the correct limit
for all callers.

Add a regression test verifying that a pre-update snapshot coexists with
prior snapshots instead of pruning them.

Fixes NousResearch#58672
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades P2 Medium — degraded but workaround exists labels Jul 5, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for identifying the destructive pruning behavior. The defect remains on current main: hermes_cli/main.py:9878 passes keep=1, while hermes_cli/backup.py:1113-1116 deletes every snapshot beyond that limit.

Problems

  • The proposed test calls create_quick_snapshot(..., hermes_home=...) directly without keep, so it uses the default on both current main and this branch. It does not execute or guard hermes_cli/main.py:9878.
  • Inheriting the default 20 reverses the pre-update-specific bounded-retention behavior introduced by 031983bbf; hermes_cli/backup.py:882-885 documents pre-update snapshots as a high-churn category. The desired retention policy should be explicit.

Suggested changes

  • Add an update-path regression test that reaches _cmd_update_impl's snapshot call and verifies the selected keep behavior.
  • Confirm whether the intended policy is the shared default, a smaller pre-update limit, or label-specific pruning that leaves manual/scheduled snapshots untouched.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@teknium1 teknium1 added the area/install-update Installer, updater, packaging, wheels, doctor label Jul 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: pre-update quick snapshot uses keep=1, silently pruning ALL other state snapshots

3 participants