From b46b0c98885c78c171d8bd52aee5dd28e082acec Mon Sep 17 00:00:00 2001 From: briandevans <252620095+briandevans@users.noreply.github.com> Date: Mon, 27 Apr 2026 06:18:33 -0700 Subject: [PATCH] fix(backup): floor pre-update backup_keep to 1 so the new backup survives MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `updates.backup_keep: 0` (or any negative value) wiped the freshly- created pre-update zip: _prune_pre_update_backups(backup_dir, keep=0): backups = sorted(..., reverse=True) # newest first, includes # the zip we just wrote for p in backups[0:]: # = all of them p.unlink() The wrapper in `main.py` then printed `Saved: ` for a file that no longer existed (the size lookup is wrapped in `try/except OSError` which silently degrades to "0 B"), leaving operators believing they had a recovery point when they had none. This is a real footgun because some config systems treat 0 as "keep unlimited"; here it does the opposite — every backup is destroyed right after creation. Fix: clamp `keep` to a minimum of 1 inside `_prune_pre_update_backups` since that helper is only invoked immediately after a fresh backup is written. Operators who genuinely want no backups should set `updates.pre_update_backup: false` (which gates creation entirely) rather than relying on `backup_keep: 0`. Also extends the `backup_keep` config docstring to spell out the floor and point at `pre_update_backup: false` as the off-switch. ## Tests Three regression tests added in `TestPreUpdateBackup`: - `test_keep_zero_does_not_delete_freshly_created_backup` — asserts the file persists after `keep=0` - `test_keep_negative_does_not_delete_freshly_created_backup` — same for negative values - `test_keep_zero_still_prunes_older_backups` — proves the floor only protects the new backup; older ones are still rotated out Verified the new tests fail on origin/main (without the floor) and pass with it; full `tests/hermes_cli/test_backup.py` suite green (84 tests). Co-Authored-By: Claude Opus 4.7 (1M context) --- hermes_cli/backup.py | 12 +++++++-- hermes_cli/config.py | 5 +++- tests/hermes_cli/test_backup.py | 47 +++++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+), 3 deletions(-) diff --git a/hermes_cli/backup.py b/hermes_cli/backup.py index 20ddb3c87d..dce199a5ab 100644 --- a/hermes_cli/backup.py +++ b/hermes_cli/backup.py @@ -793,9 +793,17 @@ def _prune_pre_update_backups(backup_dir: Path, keep: int) -> int: Returns the number of files deleted. Only touches files matching ``pre-update-*.zip`` so hand-made zips dropped in the same directory are never touched. + + ``keep`` is floored to 1 because this helper is only called immediately + after a fresh backup is written: deleting that backup right after the + user paid the disk/CPU cost to create it would leave them worse off + than no backup at all (and the wrapper in ``main.py`` would still print + a misleading ``Saved: `` line for a file that no longer exists). + Operators who genuinely don't want a backup should set + ``updates.pre_update_backup: false`` in config — that gates creation. """ - if keep < 0: - keep = 0 + if keep < 1: + keep = 1 if not backup_dir.exists(): return 0 diff --git a/hermes_cli/config.py b/hermes_cli/config.py index 98317a9043..0f34d98528 100644 --- a/hermes_cli/config.py +++ b/hermes_cli/config.py @@ -1286,7 +1286,10 @@ DEFAULT_CONFIG = { # for a single update run. "pre_update_backup": False, # How many pre-update backup zips to retain. Older ones are pruned - # automatically after each successful backup. + # automatically after each successful backup. Values below 1 are + # floored to 1 — the backup just created is always preserved. To + # disable backups entirely, set ``pre_update_backup: false`` above + # rather than ``backup_keep: 0``. "backup_keep": 5, }, diff --git a/tests/hermes_cli/test_backup.py b/tests/hermes_cli/test_backup.py index 9a99a035fa..ab7ba21370 100644 --- a/tests/hermes_cli/test_backup.py +++ b/tests/hermes_cli/test_backup.py @@ -1374,6 +1374,53 @@ class TestPreUpdateBackup: from hermes_cli.backup import create_pre_update_backup assert create_pre_update_backup(hermes_home=tmp_path / "does-not-exist") is None + def test_keep_zero_does_not_delete_freshly_created_backup(self, hermes_home): + """Regression: ``backup_keep: 0`` previously triggered ``backups[0:]`` + in the pruner — wiping the just-created zip and leaving the user + with no recovery point. The floor (keep>=1) preserves the new file + regardless of misconfiguration; users who don't want backups should + set ``pre_update_backup: false`` instead. + """ + from hermes_cli.backup import create_pre_update_backup + out = create_pre_update_backup(hermes_home=hermes_home, keep=0) + assert out is not None + assert out.exists(), ( + "keep=0 silently deleted the freshly-created backup; floor " + "should preserve the just-written file." + ) + + def test_keep_negative_does_not_delete_freshly_created_backup(self, hermes_home): + """Mirror coverage: any value <1 should be floored, not literally + applied as a slice index.""" + from hermes_cli.backup import create_pre_update_backup + out = create_pre_update_backup(hermes_home=hermes_home, keep=-3) + assert out is not None + assert out.exists() + + def test_keep_zero_still_prunes_older_backups(self, hermes_home): + """The floor preserves the new backup but should NOT regress the + rotation behaviour for older zips: a third call with keep=0 must + still remove pre-existing backups beyond the (floored) limit of 1. + """ + import time as _t + from hermes_cli.backup import create_pre_update_backup + + first = create_pre_update_backup(hermes_home=hermes_home, keep=5) + _t.sleep(1.05) + second = create_pre_update_backup(hermes_home=hermes_home, keep=5) + _t.sleep(1.05) + third = create_pre_update_backup(hermes_home=hermes_home, keep=0) + + remaining = { + p.name for p in (hermes_home / "backups").iterdir() + if p.name.startswith("pre-update-") + } + assert third.name in remaining, "Floor must preserve the new backup" + assert first.name not in remaining and second.name not in remaining, ( + f"keep=0 floor of 1 should still prune older backups; " + f"remaining={remaining}" + ) + class TestRunPreUpdateBackup: """Tests for the ``_run_pre_update_backup`` wrapper in main.py —