From bf5ff5107820babc3f060967bc7ca872cf49fc41 Mon Sep 17 00:00:00 2001 From: loulanyue <260355617@qq.com> Date: Fri, 21 Aug 2026 15:17:05 +0800 Subject: [PATCH] fix(update): check and apply config migrations on current checkout / retry paths (#91360) When an update was interrupted or failed mid-install (e.g. dependency install timeout) after pulling new code, the subsequent update run takes the 'commit_count == 0' path and early-returned without checking or migrating the configuration. Fresh code requiring a newer config version would fail to boot on the next run. Extract _check_and_apply_config_migration and invoke it across all update completion paths (normal update, current checkout / node repair, and python dependency repair). --- hermes_cli/update_cmd.py | 469 ++++++++++-------- ...te_config_migration_on_current_checkout.py | 122 +++++ 2 files changed, 372 insertions(+), 219 deletions(-) create mode 100644 tests/hermes_cli/test_update_config_migration_on_current_checkout.py diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 5fd27ae055..0d71c63804 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -288,6 +288,228 @@ def _migrate_sibling_profile_configs() -> list[tuple[str, int, int]]: return migrated +def _check_and_apply_config_migration( + *, + assume_yes: bool = False, + gateway_mode: bool = False, + pre_update_snapshot_id: str | None = None, +) -> None: + """Check and apply configuration migrations on an update completion path (#91360). + + CRITICAL: ``check_config_version`` and ``migrate_config`` must use + freshly-reloaded modules, not the ``sys.modules`` cache (see + ``_reload_config_modules``). This must run on EVERY update completion + path — the normal post-pull path, the venv-repair retry and the + Node-deps repair on the ``commit_count == 0`` "Already up to date" + branch — so an interrupted update that previously pulled new code does + not strand the user on an older config version. + """ + print() + print("→ Checking configuration for new options...") + + # Reload config modules BEFORE any config reads so get_missing_*, + # check_config_version, and migrate_config all use the updated code. + _reload_config_modules() + + from hermes_cli.config import ( + get_missing_env_vars, + 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() + + has_new_options = bool(missing_env or missing_config) + version_bump_only = ( + not has_new_options and current_ver < latest_ver + ) + needs_migration = has_new_options or current_ver < latest_ver + + if version_bump_only: + # Nothing for the user to fill in — only the config format version + # changed (new defaults already merge in transparently). Asking + # "configure new options now?" here is misleading: saying yes just + # bumps the version and looks like a no-op (issue: ScottFive / + # Tt2021). Apply it silently and say what actually happened. + print() + print( + f" ℹ Updating config format (v{current_ver} → v{latest_ver})…" + ) + try: + _mig_results = _run_migrate_config_fresh( + interactive=False, quiet=True + ) + print(" ✓ Config format updated (no new settings to configure)") + # quiet=True also mutes migration steps that RESET or REMOVE an + # existing setting (e.g. the v33→v34 personality reset from + # #81946, which records its note only in the results dict). + # Re-surface those notes so an unattended update never silently + # changes user configuration (#86656). In this branch + # missing_config is empty, so config_added can only contain + # migration-step mutations, not missing-key listings. + for _note in _mig_results.get("config_added") or []: + print(f" ℹ {_note}") + for _warn in _mig_results.get("warnings") or []: + print(f" ⚠️ {_warn}") + except Exception as _mig_err: + print(f" ⚠️ Config format update failed: {_mig_err}") + print(" Run 'hermes config migrate' to retry.") + elif needs_migration: + print() + # Show WHAT changed, not just a count, so the user can make an + # informed yes/no decision (previously the prompt named nothing). + def _print_items(items, label, key, fallback_key=None): + if not items: + return + print(f" {label}:") + shown = items[:8] + for it in shown: + if isinstance(it, dict): + name = it.get(key) or (fallback_key and it.get(fallback_key)) or "?" + desc = (it.get("description") or "").strip() + else: + # Defensive: some callers/mocks pass bare name strings. + name = str(it) + desc = "" + if desc: + print(f" • {name} — {desc}") + else: + print(f" • {name}") + extra = len(items) - len(shown) + if extra > 0: + print(f" … and {extra} more") + + if missing_env: + print( + f" ⚠️ {len(missing_env)} new required setting(s) need configuration" + ) + _print_items(missing_env, "New settings", "name") + if missing_config: + print(f" ℹ️ {len(missing_config)} new config option(s) available") + _print_items(missing_config, "New options", "key") + + print() + if assume_yes: + print( + " ℹ --yes: auto-applying config migration (skipping API-key prompts)." + ) + response = "y" + elif gateway_mode: + response = ( + _gateway_prompt( + "Would you like to configure new options now? [Y/n]", "n" + ) + .strip() + .lower() + ) + elif not (sys.stdin.isatty() and sys.stdout.isatty()): + print(" ℹ Non-interactive session — applying safe config migrations.") + response = "auto" + else: + try: + response = ( + input("Would you like to configure them now? [Y/n]: ") + .strip() + .lower() + ) + except EOFError: + response = "n" + except UnicodeDecodeError: + # input() can raise this when the terminal encoding can't + # decode the byte sequence (e.g. a non-UTF-8 locale, or an + # embedded terminal). Without this, the exception escapes + # here and crashes the update at this prompt. + print( + " ⚠ Could not read input (encoding issue). Skipping. " + "Run 'hermes config migrate' manually to configure." + ) + response = "n" + + if response in {"", "y", "yes", "auto"}: + print() + # Gateway mode, --yes, and non-interactive update contexts + # (dashboard / web server actions) cannot prompt for API keys. + # Still run the non-interactive migration pass before restarting + # so new default config fields and version bumps are written + # before the freshly updated gateway validates config at startup. + interactive_migration = not ( + gateway_mode or assume_yes or response == "auto" + ) + results = _run_migrate_config_fresh(interactive=interactive_migration, quiet=False) + + if results["env_added"] or results["config_added"]: + print() + print("✓ Configuration updated!") + if (gateway_mode or assume_yes or response == "auto") and missing_env: + print(" ℹ API keys require manual entry: hermes config migrate") + else: + print() + print("Skipped. Run 'hermes config migrate' later to configure.") + else: + print(" ✓ Configuration is up to date") + + # Fleet-wide config migration (#91277 Phase 2; #20438 earliest report, + # #54926, #79048): the shared checkout serves EVERY profile, but the + # migration above only touched the active profile's config.yaml. + # Sibling profiles kept their old _config_version and silently + # drifted (field repro: sibling gateway restarted onto new code but + # stayed at config v33 vs v37). Run the same NON-INTERACTIVE safe + # migration for every sibling profile home, scoped via the + # context-local HERMES_HOME override (never os.environ — other + # threads must not see it). + try: + _migrated_siblings = _migrate_sibling_profile_configs() + for _name, _from_ver, _to_ver in _migrated_siblings: + print( + f" ✓ Profile '{_name}': config format updated " + f"(v{_from_ver} → v{_to_ver})" + ) + except Exception as exc: + logger.debug("Sibling config migration failed: %s", exc) + + # Safety net: config-version migrations have been observed to leave + # cron/jobs.json valid-but-empty, silently dropping every scheduled + # job (issue #34600). The desktop scheduler can also overwrite with + # its own small set, causing partial loss (issue #52144). If the + # live file now has fewer jobs than the pre-update snapshot, restore + # it and warn loudly. + try: + from hermes_cli.backup import restore_cron_jobs_if_emptied + + cron_restore = restore_cron_jobs_if_emptied(pre_update_snapshot_id) + if cron_restore: + print() + print( + " ⚠️ cron/jobs.json lost jobs during this update — " + f"restored {cron_restore['job_count']} job(s) from " + f"pre-update snapshot {cron_restore['snapshot_id']}." + ) + except Exception as exc: + # Never let the cron safety net break an otherwise-good update. + logger.debug("Cron jobs auto-restore check failed: %s", exc) + + # #66140: run the same cron-jobs safety net for every sibling + # profile against ITS OWN pre-update snapshot (same-generation by + # construction — both taken by this run). + try: + from hermes_cli.backup import restore_cron_jobs_all_profiles + + for _restored in restore_cron_jobs_all_profiles( + _LAST_SIBLING_SNAPSHOTS + ): + print() + print( + f" ⚠️ Profile '{_restored['profile']}': cron/jobs.json " + f"lost jobs during this update — restored " + f"{_restored['job_count']} job(s) from pre-update " + f"snapshot {_restored['snapshot_id']}." + ) + except Exception as exc: + logger.debug("Sibling cron auto-restore check failed: %s", exc) + + # Critical files that Hermes must be able to import immediately after an # update/install. Most are imported on every CLI startup; ``web_server.py`` # is the desktop/dashboard backend path that a fresh Windows install launches @@ -3133,7 +3355,13 @@ def _record_npm_lockfile_hash(hermes_root: Path) -> None: except OSError: logger.debug("Could not write npm lockfile hash cache") -def _repair_node_deps_on_current_checkout(print_completion) -> None: +def _repair_node_deps_on_current_checkout( + print_completion, + *, + assume_yes: bool = False, + gateway_mode: bool = False, + pre_update_snapshot_id: str | None = None, +) -> None: """Repair Node deps on the ``commit_count == 0`` path (#77211). A current checkout does not imply healthy Node deps: a previous npm @@ -3158,6 +3386,11 @@ def _repair_node_deps_on_current_checkout(print_completion) -> None: # _update_node_dependencies call site; it staleness-checks internally, # so this is a no-op when nothing changed. _m()._build_web_ui(_m().PROJECT_ROOT / "web") + _check_and_apply_config_migration( + assume_yes=assume_yes, + gateway_mode=gateway_mode, + pre_update_snapshot_id=pre_update_snapshot_id, + ) print_completion("✓ Already up to date!") @@ -7397,12 +7630,22 @@ def _cmd_update_impl(args, gateway_mode: bool): healthy_after, detail_after = _venv_core_imports_healthy() if healthy_after: print("✓ Dependencies repaired!") + _check_and_apply_config_migration( + assume_yes=assume_yes, + gateway_mode=gateway_mode, + pre_update_snapshot_id=pre_update_snapshot_id, + ) _print_update_completion("✓ Update complete!") else: print(f"⚠ Venv still unhealthy after repair: {detail_after}") print(" Close all Hermes windows/gateways and re-run: hermes update") else: - _repair_node_deps_on_current_checkout(_print_update_completion) + _repair_node_deps_on_current_checkout( + _print_update_completion, + assume_yes=assume_yes, + gateway_mode=gateway_mode, + pre_update_snapshot_id=pre_update_snapshot_id, + ) if runtime_repaired is not None and not _m()._is_windows(): print() print( @@ -8053,225 +8296,13 @@ def _cmd_update_impl(args, gateway_mode: bool): except Exception: pass # honcho plugin not installed or not configured - # Check for config migrations. - # - # CRITICAL: check_config_version and migrate_config must use - # freshly-reloaded modules, not the sys.modules cache. The - # ``hermes update`` process is the PRE-pull Python process — its - # ``sys.modules`` cache holds the OLD ``hermes_cli.config`` and - # ``hermes_cli.config_migrations`` from before ``git pull`` updated - # the source files. A function-level ``from hermes_cli.config import - # check_config_version`` returns the cached module, so - # ``DEFAULT_CONFIG["_config_version"]`` is the OLD value and - # ``check_config_version()`` reports ``(33, 33)`` — "up to date" — - # even though the freshly-pulled code has v34 with a migration to - # run. The personality reset migration (#81946) was silently skipped - # this way, leaving ``display.personality: kawaii`` active after - # updates that should have reset it. - print() - print("→ Checking configuration for new options...") - - # Reload config modules BEFORE any config reads so get_missing_*, - # check_config_version, and migrate_config all use the updated code. - _reload_config_modules() - - from hermes_cli.config import ( - get_missing_env_vars, - get_missing_config_fields, + # Check for config migrations (#91360). + _check_and_apply_config_migration( + assume_yes=assume_yes, + gateway_mode=gateway_mode, + pre_update_snapshot_id=pre_update_snapshot_id, ) - missing_env = get_missing_env_vars(required_only=True) - missing_config = get_missing_config_fields() - current_ver, latest_ver = _run_config_check_fresh() - - has_new_options = bool(missing_env or missing_config) - version_bump_only = ( - not has_new_options and current_ver < latest_ver - ) - needs_migration = has_new_options or current_ver < latest_ver - - if version_bump_only: - # Nothing for the user to fill in — only the config format version - # changed (new defaults already merge in transparently). Asking - # "configure new options now?" here is misleading: saying yes just - # bumps the version and looks like a no-op (issue: ScottFive / - # Tt2021). Apply it silently and say what actually happened. - print() - print( - f" ℹ Updating config format (v{current_ver} → v{latest_ver})…" - ) - try: - _mig_results = _run_migrate_config_fresh( - interactive=False, quiet=True - ) - print(" ✓ Config format updated (no new settings to configure)") - # quiet=True also mutes migration steps that RESET or REMOVE an - # existing setting (e.g. the v33→v34 personality reset from - # #81946, which records its note only in the results dict). - # Re-surface those notes so an unattended update never silently - # changes user configuration (#86656). In this branch - # missing_config is empty, so config_added can only contain - # migration-step mutations, not missing-key listings. - for _note in _mig_results.get("config_added") or []: - print(f" ℹ {_note}") - for _warn in _mig_results.get("warnings") or []: - print(f" ⚠️ {_warn}") - except Exception as _mig_err: - print(f" ⚠️ Config format update failed: {_mig_err}") - print(" Run 'hermes config migrate' to retry.") - elif needs_migration: - print() - # Show WHAT changed, not just a count, so the user can make an - # informed yes/no decision (previously the prompt named nothing). - def _print_items(items, label, key, fallback_key=None): - if not items: - return - print(f" {label}:") - shown = items[:8] - for it in shown: - if isinstance(it, dict): - name = it.get(key) or (fallback_key and it.get(fallback_key)) or "?" - desc = (it.get("description") or "").strip() - else: - # Defensive: some callers/mocks pass bare name strings. - name = str(it) - desc = "" - if desc: - print(f" • {name} — {desc}") - else: - print(f" • {name}") - extra = len(items) - len(shown) - if extra > 0: - print(f" … and {extra} more") - - if missing_env: - print( - f" ⚠️ {len(missing_env)} new required setting(s) need configuration" - ) - _print_items(missing_env, "New settings", "name") - if missing_config: - print(f" ℹ️ {len(missing_config)} new config option(s) available") - _print_items(missing_config, "New options", "key") - - print() - if assume_yes: - print( - " ℹ --yes: auto-applying config migration (skipping API-key prompts)." - ) - response = "y" - elif gateway_mode: - response = ( - _gateway_prompt( - "Would you like to configure new options now? [Y/n]", "n" - ) - .strip() - .lower() - ) - elif not (sys.stdin.isatty() and sys.stdout.isatty()): - print(" ℹ Non-interactive session — applying safe config migrations.") - response = "auto" - else: - try: - response = ( - input("Would you like to configure them now? [Y/n]: ") - .strip() - .lower() - ) - except EOFError: - response = "n" - except UnicodeDecodeError: - # input() can raise this when the terminal encoding can't - # decode the byte sequence (e.g. a non-UTF-8 locale, or an - # embedded terminal). Without this, the exception escapes - # here and crashes the update at this prompt. - print( - " ⚠ Could not read input (encoding issue). Skipping. " - "Run 'hermes config migrate' manually to configure." - ) - response = "n" - - if response in {"", "y", "yes", "auto"}: - print() - # Gateway mode, --yes, and non-interactive update contexts - # (dashboard / web server actions) cannot prompt for API keys. - # Still run the non-interactive migration pass before restarting - # so new default config fields and version bumps are written - # before the freshly updated gateway validates config at startup. - interactive_migration = not ( - gateway_mode or assume_yes or response == "auto" - ) - results = _run_migrate_config_fresh(interactive=interactive_migration, quiet=False) - - if results["env_added"] or results["config_added"]: - print() - print("✓ Configuration updated!") - if (gateway_mode or assume_yes or response == "auto") and missing_env: - print(" ℹ API keys require manual entry: hermes config migrate") - else: - print() - print("Skipped. Run 'hermes config migrate' later to configure.") - else: - print(" ✓ Configuration is up to date") - - # Fleet-wide config migration (#91277 Phase 2; #20438 earliest report, - # #54926, #79048): the shared checkout serves EVERY profile, but the - # migration above only touched the active profile's config.yaml. - # Sibling profiles kept their old _config_version and silently - # drifted (field repro: sibling gateway restarted onto new code but - # stayed at config v33 vs v37). Run the same NON-INTERACTIVE safe - # migration for every sibling profile home, scoped via the - # context-local HERMES_HOME override (never os.environ — other - # threads must not see it). - try: - _migrated_siblings = _migrate_sibling_profile_configs() - for _name, _from_ver, _to_ver in _migrated_siblings: - print( - f" ✓ Profile '{_name}': config format updated " - f"(v{_from_ver} → v{_to_ver})" - ) - except Exception as exc: - logger.debug("Sibling config migration failed: %s", exc) - - # Safety net: config-version migrations have been observed to leave - # cron/jobs.json valid-but-empty, silently dropping every scheduled - # job (issue #34600). The desktop scheduler can also overwrite with - # its own small set, causing partial loss (issue #52144). If the - # live file now has fewer jobs than the pre-update snapshot, restore - # it and warn loudly. - try: - from hermes_cli.backup import restore_cron_jobs_if_emptied - - cron_restore = restore_cron_jobs_if_emptied(pre_update_snapshot_id) - if cron_restore: - print() - print( - " ⚠️ cron/jobs.json lost jobs during this update — " - f"restored {cron_restore['job_count']} job(s) from " - f"pre-update snapshot {cron_restore['snapshot_id']}." - ) - except Exception as exc: - # Never let the cron safety net break an otherwise-good update. - logger.debug("Cron jobs auto-restore check failed: %s", exc) - - # #66140: run the same cron-jobs safety net for every sibling - # profile against ITS OWN pre-update snapshot (same-generation by - # construction — both taken by this run). - try: - from hermes_cli.backup import restore_cron_jobs_all_profiles - - for _restored in restore_cron_jobs_all_profiles( - _LAST_SIBLING_SNAPSHOTS - ): - print() - print( - f" ⚠️ Profile '{_restored['profile']}': cron/jobs.json " - f"lost jobs during this update — restored " - f"{_restored['job_count']} job(s) from pre-update " - f"snapshot {_restored['snapshot_id']}." - ) - except Exception as exc: - logger.debug("Sibling cron auto-restore check failed: %s", exc) - _print_update_summary( node_failures=node_failures, desktop_build_ok=desktop_build_ok, diff --git a/tests/hermes_cli/test_update_config_migration_on_current_checkout.py b/tests/hermes_cli/test_update_config_migration_on_current_checkout.py new file mode 100644 index 0000000000..43e5eb8eb5 --- /dev/null +++ b/tests/hermes_cli/test_update_config_migration_on_current_checkout.py @@ -0,0 +1,122 @@ +"""Tests for config migration on the commit_count == 0 / retry path (#91360). + +When an update is interrupted (or dependency install fails) after code has already +been pulled, the subsequent update run takes the 'commit_count == 0' path. +It must run config version checking and migrations before completing so the +install is not left in a non-bootable state with new code on old config version. +""" + +from __future__ import annotations + +from unittest.mock import MagicMock, patch + +from hermes_cli import update_cmd + + +def test_repair_node_deps_runs_config_migration_on_version_bump(capsys): + """When on-disk config version is behind, _repair_node_deps_on_current_checkout + must run _check_and_apply_config_migration and migrate the config.""" + completion = MagicMock() + with ( + patch.object(update_cmd, "_update_node_dependencies", return_value=[]), + patch.object(update_cmd, "_m") as m, + patch.object(update_cmd, "_reload_config_modules"), + patch.object(update_cmd, "_run_config_check_fresh", return_value=(37, 38)), + 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_migrate_config_fresh", + return_value={"env_added": [], "config_added": ["migrated to v38"], "warnings": []}, + ) as mock_migrate, + ): + update_cmd._repair_node_deps_on_current_checkout(completion) + + m.return_value._build_web_ui.assert_called_once() + mock_migrate.assert_called_once_with(interactive=False, quiet=True) + completion.assert_called_once_with("✓ Already up to date!") + out = capsys.readouterr().out + assert "Checking configuration for new options..." in out + assert "Updating config format (v37 → v38)…" in out + assert "Config format updated" in out + + +def test_repair_node_deps_up_to_date_config(capsys): + """When config is already up to date, it reports up to date without error.""" + completion = MagicMock() + with ( + patch.object(update_cmd, "_update_node_dependencies", return_value=[]), + patch.object(update_cmd, "_m") as m, + patch.object(update_cmd, "_reload_config_modules"), + patch.object(update_cmd, "_run_config_check_fresh", return_value=(38, 38)), + 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_migrate_config_fresh") as mock_migrate, + ): + update_cmd._repair_node_deps_on_current_checkout(completion) + + m.return_value._build_web_ui.assert_called_once() + mock_migrate.assert_not_called() + completion.assert_called_once_with("✓ Already up to date!") + out = capsys.readouterr().out + assert "Checking configuration for new options..." in out + assert "Configuration is up to date" in out + + +def test_check_and_apply_config_migration_interactive_prompt(): + """When new config options exist in an interactive session, it prompts the user.""" + with ( + patch.object(update_cmd, "_reload_config_modules"), + patch.object(update_cmd, "_run_config_check_fresh", return_value=(37, 38)), + patch("hermes_cli.config.get_missing_env_vars", return_value=[{"name": "NEW_KEY", "description": "desc"}]), + patch("hermes_cli.config.get_missing_config_fields", return_value=[]), + patch("sys.stdin.isatty", return_value=True), + patch("sys.stdout.isatty", return_value=True), + patch("builtins.input", return_value="y"), + patch.object( + update_cmd, + "_run_migrate_config_fresh", + return_value={"env_added": ["NEW_KEY"], "config_added": [], "warnings": []}, + ) as mock_migrate, + ): + update_cmd._check_and_apply_config_migration(assume_yes=False, gateway_mode=False) + + mock_migrate.assert_called_once_with(interactive=True, quiet=False) + + +def test_check_and_apply_config_migration_assume_yes(): + """When assume_yes=True, it applies migrations non-interactively without prompting.""" + with ( + patch.object(update_cmd, "_reload_config_modules"), + patch.object(update_cmd, "_run_config_check_fresh", return_value=(37, 38)), + patch("hermes_cli.config.get_missing_env_vars", return_value=[{"name": "NEW_KEY"}]), + patch("hermes_cli.config.get_missing_config_fields", return_value=[]), + patch.object( + update_cmd, + "_run_migrate_config_fresh", + return_value={"env_added": [], "config_added": ["opt"], "warnings": []}, + ) as mock_migrate, + ): + update_cmd._check_and_apply_config_migration(assume_yes=True, gateway_mode=False) + + mock_migrate.assert_called_once_with(interactive=False, quiet=False) + + +def test_check_and_apply_config_migration_non_interactive(): + """In a non-interactive session (e.g. CI/scripts), it applies safe migrations automatically.""" + with ( + patch.object(update_cmd, "_reload_config_modules"), + patch.object(update_cmd, "_run_config_check_fresh", return_value=(37, 38)), + patch("hermes_cli.config.get_missing_env_vars", return_value=[]), + patch("hermes_cli.config.get_missing_config_fields", return_value=[{"key": "new_setting"}]), + patch("sys.stdin.isatty", return_value=False), + patch("sys.stdout.isatty", return_value=False), + patch.object( + update_cmd, + "_run_migrate_config_fresh", + return_value={"env_added": [], "config_added": ["new_setting"], "warnings": []}, + ) as mock_migrate, + ): + update_cmd._check_and_apply_config_migration(assume_yes=False, gateway_mode=False) + + mock_migrate.assert_called_once_with(interactive=False, quiet=False)