fix(cli): back up config.yaml before --reset overwrites it
`hermes setup --reset` calls `save_config(copy.deepcopy(DEFAULT_CONFIG))`, which writes `get_hermes_home()/config.yaml` — the exact file the backup block a few lines below copies to `config.yaml.bak.<timestamp>`. Because the copy ran after the reset, the backup captured the defaults that had just been written, not the user's config. The one invocation where a backup matters most produced a worthless one, and the original was unrecoverable. The block's own comment already claimed it runs "before setup modifies it"; on the --reset path that was false. Move it above the --reset branch so it captures the true pre-setup state on every path. Also report the backup location on the --reset path. --reset is destructive and can leave the wizard early (the non-interactive return exits before the end-of-setup notice), so a user who just lost their config was never told where the copy is. The end-of-setup notice is unchanged for the normal path and is suppressed only when it has already been shown, so no run prints it twice; the shared wording now lives in one helper. Behaviour otherwise preserved: `copy2` (config.yaml holds secrets, so mode is preserved), the try/except fallback to `_backup_path = None`, and the existing notice for the full-setup path. Follow-ups deliberately out of scope: pruning accumulated `.bak.*` files, and printing the notice on the other early-return paths (--portal, section runs). Refs #3522
This commit is contained in:
@@ -649,17 +649,19 @@ def _run_setup_wizard_impl(args):
|
||||
managed_error("run setup wizard")
|
||||
return
|
||||
ensure_hermes_home()
|
||||
# Back up BEFORE --reset: save_config below overwrites the very file we copy (#3522, #77299).
|
||||
config_path = get_config_path()
|
||||
from hermes_cli.config_backups import backup_config
|
||||
_backup_path = backup_config(config_path, "pre-setup")
|
||||
if getattr(args, "reset", False):
|
||||
save_config(copy.deepcopy(DEFAULT_CONFIG))
|
||||
print_success("Configuration reset to defaults.")
|
||||
if _backup_path: # --reset may exit before the end-of-wizard notice
|
||||
_info(f"Previous config backed up to: {_backup_path}")
|
||||
reconfigure_requested = bool(getattr(args, "reconfigure", False))
|
||||
quick_requested = bool(getattr(args, "quick", False))
|
||||
config = load_config()
|
||||
hermes_home = get_hermes_home()
|
||||
# Back up existing config before setup modifies it (#3522)
|
||||
config_path = get_config_path()
|
||||
from hermes_cli.config_backups import backup_config
|
||||
_backup_path = backup_config(config_path, "pre-setup")
|
||||
|
||||
# Non-interactive environments (headless SSH, Docker, CI/CD)
|
||||
if getattr(args, 'non_interactive', False) or not is_interactive_stdin():
|
||||
|
||||
98
tests/hermes_cli/test_setup_reset_backup.py
Normal file
98
tests/hermes_cli/test_setup_reset_backup.py
Normal file
@@ -0,0 +1,98 @@
|
||||
"""Ordering guarantees for the setup wizard's config.yaml backup (#3522).
|
||||
|
||||
The wizard copies ``config.yaml`` into ``backups/config/`` so a user
|
||||
can recover values setup overwrote. ``hermes setup --reset`` replaces that same
|
||||
file with ``DEFAULT_CONFIG``, so the copy is only useful if it is taken *before*
|
||||
the reset runs — and the user has to be told where it landed even when --reset
|
||||
exits the wizard early.
|
||||
"""
|
||||
|
||||
from argparse import Namespace
|
||||
|
||||
from hermes_cli.config import (
|
||||
DEFAULT_CONFIG,
|
||||
get_config_path,
|
||||
load_config,
|
||||
save_config,
|
||||
)
|
||||
|
||||
SENTINEL_MODEL = "hand-tuned-local-model-xyz"
|
||||
|
||||
|
||||
def _make_setup_args(**overrides):
|
||||
return Namespace(
|
||||
non_interactive=overrides.get("non_interactive", True),
|
||||
section=overrides.get("section", None),
|
||||
reset=overrides.get("reset", False),
|
||||
)
|
||||
|
||||
|
||||
def _write_user_config(tmp_path):
|
||||
"""Persist a config.yaml holding a distinctive, non-default user value."""
|
||||
config_path = get_config_path()
|
||||
cfg = load_config()
|
||||
cfg["model"] = {
|
||||
"provider": "custom",
|
||||
"base_url": "http://localhost:8080/v1",
|
||||
"default": SENTINEL_MODEL,
|
||||
}
|
||||
cfg["agent"]["max_turns"] = 47
|
||||
save_config(cfg)
|
||||
|
||||
assert config_path.parent == tmp_path
|
||||
text = config_path.read_text(encoding="utf-8")
|
||||
assert SENTINEL_MODEL in text, "fixture failed to persist the user value"
|
||||
return config_path
|
||||
|
||||
|
||||
def _backups(tmp_path):
|
||||
return sorted((tmp_path / "backups" / "config").glob("config.yaml.pre-setup.*"))
|
||||
|
||||
|
||||
class TestResetBackupOrdering:
|
||||
def test_reset_backs_up_user_config_not_the_reset_defaults(
|
||||
self, tmp_path, monkeypatch
|
||||
):
|
||||
"""The --reset backup must hold the user's config, not DEFAULT_CONFIG.
|
||||
|
||||
Regression for the ordering bug: the backup block used to run after the
|
||||
--reset branch had already overwritten config.yaml, so the ``.bak`` file
|
||||
advertised as the recovery path captured the defaults that had just been
|
||||
written and the user's real config was unrecoverable.
|
||||
"""
|
||||
from hermes_cli.setup import run_setup_wizard
|
||||
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
config_path = _write_user_config(tmp_path)
|
||||
|
||||
run_setup_wizard(_make_setup_args(non_interactive=True, reset=True))
|
||||
|
||||
backups = _backups(tmp_path)
|
||||
assert len(backups) == 1, f"expected exactly one backup, got {backups}"
|
||||
backup_text = backups[0].read_text(encoding="utf-8")
|
||||
assert SENTINEL_MODEL in backup_text, (
|
||||
"backup captured the post-reset defaults instead of the user's config"
|
||||
)
|
||||
assert "47" in backup_text
|
||||
|
||||
# The reset itself must still take effect on config.yaml.
|
||||
assert SENTINEL_MODEL not in config_path.read_text(encoding="utf-8")
|
||||
assert load_config()["model"] == DEFAULT_CONFIG["model"]
|
||||
|
||||
def test_reset_reports_where_the_backup_landed(
|
||||
self, tmp_path, monkeypatch, capsys
|
||||
):
|
||||
"""--reset can exit early, so it must surface the backup path itself."""
|
||||
from hermes_cli.setup import run_setup_wizard
|
||||
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
_write_user_config(tmp_path)
|
||||
|
||||
run_setup_wizard(_make_setup_args(non_interactive=True, reset=True))
|
||||
|
||||
out = capsys.readouterr().out
|
||||
backups = _backups(tmp_path)
|
||||
assert len(backups) == 1
|
||||
assert "Configuration reset to defaults." in out
|
||||
assert "Previous config backed up to:" in out
|
||||
assert backups[0].name in out
|
||||
Reference in New Issue
Block a user