fix(backup): floor pre-update backup_keep to 1 so the new backup survives
`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: <path>` 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) <noreply@anthropic.com>
This commit is contained in:
@@ -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: <path>`` 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
|
||||
|
||||
|
||||
@@ -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,
|
||||
},
|
||||
|
||||
|
||||
@@ -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 —
|
||||
|
||||
Reference in New Issue
Block a user