fix(update): run config migration on the 'Already up to date' repair path (#91360)
A failed update attempt can pull fresh code onto disk and then die before the config-migration block (e.g. a PyPI timeout during the dependency sync). The desktop hand-off retries; the retry takes the commit_count == 0 branch, repairs deps, prints 'Already up to date!' and returns early - skipping _run_config_check_fresh / migrate_config entirely. The fresh code (requiring a newer _config_version) then refuses to start against the old config until 'hermes doctor --fix' is run. Fix: _maybe_migrate_config_on_current() mirrors the version_bump_only handling (silent, non-interactive) and is called on both repair-path completion points before claiming success. Also: scripts/desktop-update/posix.sh no longer retries when the update was deliberately SKIPPED (checkout parked on a non-target branch) -, the retry is deterministic and only wastes time. Uses a dedicated non- colliding exit code (8) and an honest message instead of 'Update failed'. New tests: tests/hermes_cli/test_update_config_migration_on_current.py (5 cases: migrate-when-behind, noop-current, noop-ahead, warning re- surface, silent check failure).
This commit is contained in:
fred0m
@@ -287,7 +287,6 @@ def _migrate_sibling_profile_configs() -> list[tuple[str, int, int]]:
|
||||
logger.debug("Sibling profile enumeration failed: %s", exc)
|
||||
return migrated
|
||||
|
||||
|
||||
def _check_and_apply_config_migration(
|
||||
*,
|
||||
assume_yes: bool = False,
|
||||
@@ -316,10 +315,18 @@ def _check_and_apply_config_migration(
|
||||
get_missing_config_fields,
|
||||
)
|
||||
|
||||
|
||||
missing_env = get_missing_env_vars(required_only=True)
|
||||
missing_config = get_missing_config_fields()
|
||||
current_ver, latest_ver = _run_config_check_fresh()
|
||||
# Defensive (#91360): this helper runs on repair/retry completion paths
|
||||
# too — a config-check failure must not break an otherwise-successful
|
||||
# update. Log, point at the manual command, and return.
|
||||
try:
|
||||
missing_env = get_missing_env_vars(required_only=True)
|
||||
missing_config = get_missing_config_fields()
|
||||
current_ver, latest_ver = _run_config_check_fresh()
|
||||
except Exception as exc:
|
||||
logger.debug("Config check during update failed: %s", exc)
|
||||
print(" ⚠️ Could not check config version.")
|
||||
print(" Run 'hermes config migrate' to check manually.")
|
||||
return
|
||||
|
||||
has_new_options = bool(missing_env or missing_config)
|
||||
version_bump_only = (
|
||||
|
||||
@@ -594,6 +594,19 @@ log "hermes update exit code: $CODE"
|
||||
if [ "$CODE" -ne 0 ] && [ "$CODE" -ne 2 ]; then
|
||||
# Retry once: update-boundary class (fresh code on disk, stale in memory).
|
||||
# Exit 2 ("close all Hermes windows") is not retryable.
|
||||
#
|
||||
# A parked-branch SKIP (checkout on a feature branch with unmerged
|
||||
# commits) is also deterministic — the retry would hit the exact same
|
||||
# branch state and skip again, so it only wastes time. Detect the skip
|
||||
# by its banner, skip the retry, and surface an honest message with a
|
||||
# dedicated exit code (8) so callers can distinguish "skipped" from a
|
||||
# real failure.
|
||||
if printf '%s' "$OUT" | grep -q "CODE UPDATE SKIPPED"; then
|
||||
log "hermes update skipped (checkout parked on a non-target branch); not retrying"
|
||||
FINAL_CODE=8
|
||||
FINAL_MSG="Update skipped: the git checkout is on a branch that isn't fully merged into $BRANCH. Switch to the target branch and update again (see the terminal output for the exact commands)."
|
||||
exit 8
|
||||
fi
|
||||
log "retrying once (freshly pulled fix loads on the second run)"
|
||||
publish_stage "Retrying update"
|
||||
OUT="$("$HERMES_BIN" update --yes --gateway $KEEP_STASH --branch "$BRANCH" 2>&1)"; CODE=$?
|
||||
|
||||
@@ -0,0 +1,122 @@
|
||||
"""Tests for config migration on the \"Already up to date\" repair path.
|
||||
|
||||
Covers ``_check_and_apply_config_migration`` on the ``commit_count == 0``
|
||||
retry paths (#91360): a previous update attempt can pull new code onto disk
|
||||
and then fail before reaching the config-migration block (e.g. PyPI timeout
|
||||
during dependency sync); the retry then enters the ``commit_count == 0``
|
||||
branch and returns early, skipping config migration entirely. The fresh
|
||||
code (which may require a newer ``_config_version``) keeps running against
|
||||
the old config and the next Hermes launch refuses to start.
|
||||
|
||||
Matrix: config behind / current / ahead of the code's version, plus the
|
||||
#86656 contract (quiet-migration warnings must be re-surfaced) and the
|
||||
check-failure contract (a config-check failure must not break the repair
|
||||
path).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import contextlib
|
||||
import io
|
||||
from unittest.mock import patch
|
||||
|
||||
import hermes_cli.update_cmd as update_cmd
|
||||
|
||||
|
||||
def _run(current: int, latest: int):
|
||||
"""Run _check_and_apply_config_migration with mocked config checks.
|
||||
|
||||
Returns (stdout, migrate_calls).
|
||||
"""
|
||||
migrate_calls = []
|
||||
|
||||
def _fake_migrate(interactive=False, quiet=False):
|
||||
migrate_calls.append((interactive, quiet))
|
||||
return {"env_added": [], "config_added": [], "warnings": []}
|
||||
|
||||
with patch.object(update_cmd, "_reload_config_modules"), patch(
|
||||
"hermes_cli.config.get_missing_env_vars", return_value=[]
|
||||
), patch(
|
||||
"hermes_cli.config.get_missing_config_fields", return_value=[]
|
||||
), patch.object(
|
||||
update_cmd, "_run_config_check_fresh", return_value=(current, latest)
|
||||
), patch.object(
|
||||
update_cmd, "_run_migrate_config_fresh", side_effect=_fake_migrate
|
||||
), patch.object(
|
||||
update_cmd, "_migrate_sibling_profile_configs", return_value=[]
|
||||
):
|
||||
buf = io.StringIO()
|
||||
with contextlib.redirect_stdout(buf):
|
||||
update_cmd._check_and_apply_config_migration()
|
||||
return buf.getvalue(), migrate_calls
|
||||
|
||||
|
||||
def test_migrates_when_config_behind():
|
||||
"""Version bump on the repair path must be applied silently."""
|
||||
out, calls = _run(current=37, latest=38)
|
||||
assert "v37 → v38" in out
|
||||
assert "Config format updated" in out
|
||||
assert calls == [(False, True)] # non-interactive, quiet
|
||||
|
||||
|
||||
def test_noop_when_config_current():
|
||||
"""No migration when the config version is already current."""
|
||||
out, calls = _run(current=38, latest=38)
|
||||
assert "Configuration is up to date" in out
|
||||
assert calls == []
|
||||
|
||||
|
||||
def test_noop_when_config_ahead():
|
||||
"""No migration when local config is newer than the code's default."""
|
||||
out, calls = _run(current=39, latest=38)
|
||||
assert "Configuration is up to date" in out
|
||||
assert calls == []
|
||||
|
||||
|
||||
def test_surfaces_migration_warnings():
|
||||
"""Warnings from a quiet migration must be re-surfaced (#86656)."""
|
||||
|
||||
def _fake_migrate(interactive=False, quiet=False):
|
||||
return {
|
||||
"env_added": [],
|
||||
"config_added": [],
|
||||
"warnings": ["personality reset: kawaii → default"],
|
||||
}
|
||||
|
||||
with patch.object(update_cmd, "_reload_config_modules"), patch(
|
||||
"hermes_cli.config.get_missing_env_vars", return_value=[]
|
||||
), patch(
|
||||
"hermes_cli.config.get_missing_config_fields", return_value=[]
|
||||
), patch.object(
|
||||
update_cmd, "_run_config_check_fresh", return_value=(37, 38)
|
||||
), patch.object(
|
||||
update_cmd, "_run_migrate_config_fresh", side_effect=_fake_migrate
|
||||
), patch.object(
|
||||
update_cmd, "_migrate_sibling_profile_configs", return_value=[]
|
||||
):
|
||||
buf = io.StringIO()
|
||||
with contextlib.redirect_stdout(buf):
|
||||
update_cmd._check_and_apply_config_migration()
|
||||
out = buf.getvalue()
|
||||
|
||||
assert "personality reset" in out
|
||||
|
||||
|
||||
def test_check_failure_does_not_break_repair_path():
|
||||
"""A config-check failure must not break the repair path."""
|
||||
with patch.object(update_cmd, "_reload_config_modules"), patch(
|
||||
"hermes_cli.config.get_missing_env_vars", return_value=[]
|
||||
), patch(
|
||||
"hermes_cli.config.get_missing_config_fields", return_value=[]
|
||||
), patch.object(
|
||||
update_cmd, "_run_config_check_fresh", side_effect=RuntimeError("boom")
|
||||
), patch.object(
|
||||
update_cmd, "_run_migrate_config_fresh", return_value={}
|
||||
) as mig:
|
||||
buf = io.StringIO()
|
||||
with contextlib.redirect_stdout(buf):
|
||||
# Should not raise and should not attempt migration.
|
||||
update_cmd._check_and_apply_config_migration()
|
||||
out = buf.getvalue()
|
||||
assert "Could not check config version" in out
|
||||
mig.assert_not_called()
|
||||
Reference in New Issue
Block a user