diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index 2db4342f80..cf64673d9e 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -655,9 +655,9 @@ _UPDATE_CRITICAL_MODULES = ( ) -def _validate_critical_modules_import( +def _critical_module_import_failures( root, *, report_runtime_errors: bool = False -) -> tuple[bool, str | None, str | None]: +) -> dict[str, str]: """Import each module in ``_UPDATE_CRITICAL_MODULES`` in a subprocess. ``_validate_critical_files_syntax`` only *parses* files, so it cannot see @@ -680,15 +680,18 @@ def _validate_critical_modules_import( different Python than the install's own, and probing the wrong interpreter would test a tree the user never runs. - Returns ``(ok, failing_module, error_message)``. Generic import-time - exceptions remain tolerated by default because they can depend on local - config or environment. ``report_runtime_errors=True`` exposes them so a - caller can compare two states of the same checkout. + Returns every failing module in probe order. Generic import-time exceptions + remain tolerated by default because they can depend on local config or + environment. ``report_runtime_errors=True`` exposes them so a caller can + compare two states of the same checkout without an earlier failure masking + a later one. """ from hermes_constants import FIRST_PARTY_MODULE_ROOTS + marker = "__HERMES_IMPORT_HEALTH__" probe = ( - "import importlib, sys\n" + "import importlib, json, sys\n" + "failures = []\n" "for name in %r:\n" " try:\n" " importlib.import_module(name)\n" @@ -699,16 +702,19 @@ def _validate_critical_modules_import( # from the hint the user is shown (they disagreed once already). " missing = (getattr(exc, 'name', '') or '').split('.')[0]\n" " if missing in %r or missing.startswith('hermes_'):\n" - " sys.stdout.write(name + '\\n' + str(exc))\n" - " raise SystemExit(3)\n" + " failures.append((name, str(exc)))\n" " except ImportError as exc:\n" - " sys.stdout.write(name + '\\n' + str(exc))\n" - " raise SystemExit(3)\n" + " failures.append((name, str(exc)))\n" " except Exception as exc:\n" - " sys.stdout.write(name + '\\n' + str(exc))\n" - " raise SystemExit(4)\n" - "raise SystemExit(0)\n" - % (_UPDATE_CRITICAL_MODULES, tuple(sorted(FIRST_PARTY_MODULE_ROOTS))) + " if %r:\n" + " failures.append((name, str(exc)))\n" + "sys.stdout.write('\\n%s' + json.dumps(failures))\n" + % ( + _UPDATE_CRITICAL_MODULES, + tuple(sorted(FIRST_PARTY_MODULE_ROOTS)), + report_runtime_errors, + marker, + ) ) try: interpreter = sys.executable @@ -731,20 +737,29 @@ def _validate_critical_modules_import( ) except (OSError, subprocess.SubprocessError): # Can't run the probe — don't block the update on our own tooling. - return True, None, None - if result.returncode != 0: - if result.returncode == 4 and not report_runtime_errors: - # Generic import-time failures can depend on local config or env, - # so the ordinary post-update guard keeps its historical tolerance. - # Stash restoration opts into reporting them and compares the clean - # tree with the restored tree to detect failures introduced by the - # user's local changes. - return True, None, None - output = result.stdout or result.stderr or "" - parts = output.split("\n", 1) - module = parts[0].strip() or "unknown" - detail = parts[1].strip() if len(parts) > 1 else "" - return False, module, detail + return {} + output = result.stdout or "" + if marker not in output: + return {} + try: + import json + + failures = json.loads(output.rsplit(marker, 1)[1]) + return {str(module): str(detail) for module, detail in failures} + except (TypeError, ValueError): + return {} + + +def _validate_critical_modules_import( + root, *, report_runtime_errors: bool = False +) -> tuple[bool, str | None, str | None]: + """Return the first critical-module import failure, if any.""" + failures = _critical_module_import_failures( + root, report_runtime_errors=report_runtime_errors + ) + if failures: + module = next(iter(failures)) + return False, module, failures[module] return True, None, None def _gateway_prompt(prompt_text: str, default: str = "", timeout: float = 300.0) -> str: @@ -2543,7 +2558,7 @@ def _restore_stashed_changes( return False preexisting_untracked = _git_untracked_paths(git_cmd, cwd) - clean_import_health = _validate_critical_modules_import( + clean_import_failures = _critical_module_import_failures( cwd, report_runtime_errors=True ) print("→ Restoring local changes...") @@ -2621,11 +2636,19 @@ def _restore_stashed_changes( syntax_error, ) - restored_import_health = _validate_critical_modules_import( + restored_import_failures = _critical_module_import_failures( cwd, report_runtime_errors=True ) - import_ok, failing_module, import_error = restored_import_health - if not import_ok and restored_import_health != clean_import_health: + changed_import_failure = next( + ( + (module, error) + for module, error in restored_import_failures.items() + if clean_import_failures.get(module) != error + ), + None, + ) + if changed_import_failure is not None: + failing_module, import_error = changed_import_failure _reject_unsafe_stash_restore( git_cmd, cwd, diff --git a/tests/hermes_cli/test_update_autostash.py b/tests/hermes_cli/test_update_autostash.py index 87f9f28e35..b189151fc7 100644 --- a/tests/hermes_cli/test_update_autostash.py +++ b/tests/hermes_cli/test_update_autostash.py @@ -563,6 +563,53 @@ def test_restore_allows_preexisting_import_time_failure(monkeypatch, tmp_path): assert git("stash", "list").stdout.strip() == "" +def test_restore_rejects_later_failure_masked_by_preexisting_failure( + monkeypatch, tmp_path, capsys +): + """Every critical module must be compared, not only the first failure.""" + import subprocess + from hermes_cli import update_cmd + + def git(*args, check=True): + return subprocess.run( + ["git", *args], + cwd=tmp_path, + capture_output=True, + text=True, + check=check, + ) + + git("init", "-q", "-b", "main") + git("config", "user.email", "t@example.com") + git("config", "user.name", "t") + (tmp_path / "first.py").write_text( + "raise RuntimeError('missing local config')\n", encoding="utf-8" + ) + second = tmp_path / "second.py" + second.write_text("VALUE = 1\n", encoding="utf-8") + git("add", "-A") + git("commit", "-qm", "init") + + second.write_text("raise RuntimeError('restored later failure')\n", encoding="utf-8") + stash_ref = hermes_main._stash_local_changes_if_needed(["git"], tmp_path) + assert stash_ref + monkeypatch.setattr(update_cmd, "_UPDATE_CRITICAL_MODULES", ("first", "second")) + + with pytest.raises(SystemExit) as exc_info: + hermes_main._restore_stashed_changes( + ["git"], tmp_path, stash_ref, prompt_user=False + ) + + assert exc_info.value.code == 1 + assert second.read_text(encoding="utf-8") == "VALUE = 1\n" + assert git("status", "--porcelain").stdout == "" + assert git("stash", "list").stdout.strip() + output = capsys.readouterr().out + assert "agent import second" in output + assert "restored later failure" in output + assert "gateway was not restarted" in output + + def test_gateway_restore_prompt_defaults_to_keep_stash(tmp_path, capsys): prompts = []