fix(update): catch partially-updated trees that parse but can't import
A Windows user reported every startup dying with `ImportError: cannot import name 'TODO_INJECTION_HEADER' from 'tools.todo_tool'`. The symbol exists on main; their tree had the new `agent/context_compressor.py` (which imports it at module level) alongside a pre-update `tools/todo_tool.py`. The post-update guard missed it. `_validate_critical_files_syntax` only py_compiles files, and every file in a skewed tree parses fine — it is the combination that is broken. The guard reported success and the update completed over an install that could not start. The ZIP-update path (Windows-only, used when git file I/O is broken) is where the skew comes from: its copy loop replaces top-level entries one at a time in `os.listdir` order, so `agent/` lands at index 13 and `tools/` at index 66. Any failure between them leaves exactly this mismatch — and that path had no post-copy validation or rollback at all. - Add `_validate_critical_modules_import`: imports the four startup modules in a subprocess (~0.4s) so cross-module breakage is caught. Non-import errors (config/env) are ignored; a probe that cannot spawn is non-fatal so we never block an update on our own tooling. - Run it after the syntax guard on the git path, reusing the existing auto-rollback. - Run it on the ZIP path after dependency install (so a genuinely-new requirement is not misreported as a partial copy), and make the ZIP failure message state the install may be half-updated. - Add `partial_update_hint()` and print it under "Failed to initialize agent", so users see "re-run hermes update" instead of a bare ImportError. Stays silent for ModuleNotFoundError and third-party imports, which need different remediation. Verified by simulating the exact skew: the syntax guard returns ok=True while the import guard returns the user's error verbatim.
This commit is contained in:
parent
85e0073902
commit
baecc840e5
|
|
@ -462,6 +462,10 @@ class CLIAgentSetupMixin:
|
|||
return True
|
||||
except Exception as e:
|
||||
ChatConsole().print(f"[bold red]Failed to initialize agent: {e}[/]")
|
||||
from hermes_constants import partial_update_hint
|
||||
|
||||
for line in partial_update_hint(e):
|
||||
ChatConsole().print(line)
|
||||
return False
|
||||
|
||||
def _preload_resumed_session(self) -> bool:
|
||||
|
|
|
|||
|
|
@ -5071,6 +5071,7 @@ from hermes_cli.update_cmd import ( # noqa: F401
|
|||
_update_via_zip,
|
||||
_upgrade_pip_before_lazy_refresh,
|
||||
_validate_critical_files_syntax,
|
||||
_validate_critical_modules_import,
|
||||
_venv_core_imports_healthy,
|
||||
_venv_launcher_ancestors,
|
||||
_wait_for_windows_update_gateway_exit,
|
||||
|
|
@ -5083,6 +5084,7 @@ from hermes_cli.update_cmd import ( # noqa: F401
|
|||
_write_update_planned_stop_marker,
|
||||
_UPDATE_RUNTIME_RELOAD_MODULES,
|
||||
_UPDATE_CRITICAL_FILES,
|
||||
_UPDATE_CRITICAL_MODULES,
|
||||
OFFICIAL_REPO_URLS,
|
||||
OFFICIAL_REPO_URL,
|
||||
SKIP_UPSTREAM_PROMPT_FILE,
|
||||
|
|
|
|||
|
|
@ -157,6 +157,71 @@ def _validate_critical_files_syntax(root) -> tuple[bool, str | None, str | None]
|
|||
return False, str(path), f"could not read: {exc}"
|
||||
return True, None, None
|
||||
|
||||
|
||||
# Modules imported on every agent startup. Unlike _UPDATE_CRITICAL_FILES (which
|
||||
# is only parsed), these are actually *imported* so that cross-module breakage
|
||||
# is caught — a file can be syntactically perfect and still fail to import
|
||||
# because a name it pulls from a sibling module no longer exists.
|
||||
_UPDATE_CRITICAL_MODULES = (
|
||||
"hermes_cli.main",
|
||||
"run_agent",
|
||||
"model_tools",
|
||||
"toolsets",
|
||||
)
|
||||
|
||||
|
||||
def _validate_critical_modules_import(root) -> tuple[bool, str | None, str | None]:
|
||||
"""Import each module in ``_UPDATE_CRITICAL_MODULES`` in a subprocess.
|
||||
|
||||
``_validate_critical_files_syntax`` only *parses* files, so it cannot see
|
||||
cross-module breakage: a partially-updated tree where ``agent/`` is new but
|
||||
``tools/`` is old parses perfectly and still dies at startup with
|
||||
``ImportError: cannot import name 'TODO_INJECTION_HEADER' from
|
||||
'tools.todo_tool'``. Every file is valid Python; the *combination* is not.
|
||||
|
||||
That skew is reachable on the Windows ZIP-update path, whose copy loop
|
||||
walks top-level entries in ``os.listdir`` order and replaces each one
|
||||
independently — ``agent/`` lands long before ``tools/``, so a failure or
|
||||
interruption between them leaves exactly that mismatch on disk.
|
||||
|
||||
Runs in a subprocess because importing these modules into the running
|
||||
updater would pollute ``sys.modules`` and execute import-time side effects
|
||||
against the half-updated tree. Costs ~0.4s.
|
||||
|
||||
Returns ``(ok, failing_module, error_message)``.
|
||||
"""
|
||||
probe = (
|
||||
"import importlib, sys\n"
|
||||
"for name in %r:\n"
|
||||
" try:\n"
|
||||
" importlib.import_module(name)\n"
|
||||
" except ImportError as exc:\n"
|
||||
" sys.stdout.write(name + '\\n' + str(exc))\n"
|
||||
" raise SystemExit(3)\n"
|
||||
" except Exception:\n"
|
||||
" pass\n" # non-import errors (config/env) aren't update breakage
|
||||
"raise SystemExit(0)\n" % (_UPDATE_CRITICAL_MODULES,)
|
||||
)
|
||||
try:
|
||||
result = subprocess.run(
|
||||
[sys.executable, "-c", probe],
|
||||
cwd=str(root),
|
||||
capture_output=True,
|
||||
text=True,
|
||||
encoding="utf-8",
|
||||
errors="replace",
|
||||
timeout=120,
|
||||
)
|
||||
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 == 3:
|
||||
parts = (result.stdout or "").split("\n", 1)
|
||||
module = parts[0].strip() or "unknown"
|
||||
detail = parts[1].strip() if len(parts) > 1 else ""
|
||||
return False, module, detail
|
||||
return True, None, None
|
||||
|
||||
def _gateway_prompt(prompt_text: str, default: str = "", timeout: float = 300.0) -> str:
|
||||
"""File-based IPC prompt for gateway mode.
|
||||
|
||||
|
|
@ -627,6 +692,14 @@ def _update_via_zip(args):
|
|||
|
||||
except Exception as e:
|
||||
print(f"✗ ZIP update failed: {e}")
|
||||
print(
|
||||
" The install may be partially updated — some directories were "
|
||||
"replaced and others were not."
|
||||
)
|
||||
print(
|
||||
" Re-run `hermes update` to finish; if the agent won't start, "
|
||||
"reinstall from https://hermes-agent.nousresearch.com"
|
||||
)
|
||||
_m().sys.exit(1)
|
||||
finally:
|
||||
shutil.rmtree(tmp_dir, ignore_errors=True)
|
||||
|
|
@ -685,6 +758,27 @@ def _update_via_zip(args):
|
|||
# #70636).
|
||||
_m()._refresh_active_memory_provider_dependencies()
|
||||
|
||||
# Now that dependencies are installed, verify the tree actually imports.
|
||||
# The copy loop above replaces top-level entries one at a time in
|
||||
# os.listdir order, so an interruption between (say) `agent/` and `tools/`
|
||||
# leaves a tree whose files all parse but cannot be imported together —
|
||||
# the ImportError-on-startup class this guard exists to catch. Deliberately
|
||||
# placed *after* the dependency reinstall so a genuinely-new third-party
|
||||
# requirement isn't misreported as a partial copy. There is no SHA to roll
|
||||
# back to here, so surface it with a concrete recovery step rather than
|
||||
# reporting a successful update over a bricked install.
|
||||
import_ok, failing_module, import_error = _validate_critical_modules_import(
|
||||
_m().PROJECT_ROOT
|
||||
)
|
||||
if not import_ok:
|
||||
print()
|
||||
print("✗ Update left the install in an unimportable state:")
|
||||
print(f" {failing_module}: {import_error}")
|
||||
print()
|
||||
print(" This usually means the copy was interrupted partway through.")
|
||||
print(" Re-run `hermes update` to complete it.")
|
||||
_m().sys.exit(1)
|
||||
|
||||
node_failures = _update_node_dependencies()
|
||||
_m()._build_web_ui(_m().PROJECT_ROOT / "web")
|
||||
|
||||
|
|
@ -3701,9 +3795,22 @@ def _cmd_update_impl(args, gateway_mode: bool):
|
|||
syntax_ok, failing_path, syntax_error = _validate_critical_files_syntax(
|
||||
_m().PROJECT_ROOT
|
||||
)
|
||||
if syntax_ok:
|
||||
# Parsing clean isn't enough: a tree can be syntactically valid
|
||||
# and still unimportable when modules land out of sync (see
|
||||
# _validate_critical_modules_import). Catch that here too so the
|
||||
# same rollback path covers both failure modes.
|
||||
(
|
||||
syntax_ok,
|
||||
failing_path,
|
||||
syntax_error,
|
||||
) = _m()._validate_critical_modules_import(_m().PROJECT_ROOT)
|
||||
failure_label = "cannot be imported (partially-updated tree)"
|
||||
else:
|
||||
failure_label = "has a syntax error"
|
||||
if not syntax_ok:
|
||||
print()
|
||||
print("✗ Pulled code has a syntax error in a critical file:")
|
||||
print(f"✗ Pulled code {failure_label} in a critical file:")
|
||||
print(f" {failing_path}")
|
||||
if syntax_error:
|
||||
# py_compile errors can be multi-line; show the first
|
||||
|
|
|
|||
|
|
@ -1248,3 +1248,41 @@ OPENROUTER_BASE_URL = "https://openrouter.ai/api/v1"
|
|||
OPENROUTER_MODELS_URL = f"{OPENROUTER_BASE_URL}/models"
|
||||
|
||||
AI_GATEWAY_BASE_URL = "https://ai-gateway.vercel.sh/v1"
|
||||
|
||||
|
||||
# ─── Partial-update diagnostics ──────────────────────────────────────────────
|
||||
|
||||
def partial_update_hint(exc: BaseException) -> list[str]:
|
||||
"""Return recovery guidance lines when *exc* looks like a half-updated tree.
|
||||
|
||||
An interrupted or partially-applied update can leave the checkout with new
|
||||
files in one package and stale files in another. Every file still parses,
|
||||
so nothing is corrupt in the usual sense — but a module that imports a name
|
||||
added in the same release from a sibling that wasn't refreshed dies with
|
||||
``ImportError: cannot import name 'X' from 'y'`` on every startup.
|
||||
|
||||
Users hit this as an opaque crash with no indication that the *install*,
|
||||
rather than their config, is the problem — and `hermes update` is exactly
|
||||
the command they need but are least likely to trust after a failed update.
|
||||
Return the guidance so callers can print it alongside the raw error.
|
||||
|
||||
Returns an empty list for unrelated exceptions, so callers can splat it
|
||||
unconditionally.
|
||||
"""
|
||||
if not isinstance(exc, ImportError):
|
||||
return []
|
||||
# A missing third-party dependency is a different problem (bad venv, missing
|
||||
# extra) with different remediation, so don't claim a partial update.
|
||||
if isinstance(exc, ModuleNotFoundError):
|
||||
return []
|
||||
name = getattr(exc, "name", None)
|
||||
if not name or not str(name).startswith(("tools", "agent", "hermes", "gateway")):
|
||||
return []
|
||||
return [
|
||||
"",
|
||||
"This looks like a partially-updated install: one module was refreshed "
|
||||
"and a related one was not.",
|
||||
"Re-run the update to bring the whole tree to the same version:",
|
||||
" hermes update",
|
||||
"If that also fails, reinstall: https://hermes-agent.nousresearch.com",
|
||||
]
|
||||
|
|
|
|||
|
|
@ -0,0 +1,117 @@
|
|||
"""Tests for the post-update *import* guard in ``hermes update``.
|
||||
|
||||
``_validate_critical_files_syntax`` only parses files, so it cannot detect a
|
||||
partially-updated tree: when one package is refreshed and a sibling is not,
|
||||
every file still parses but importing them together raises ``ImportError``.
|
||||
|
||||
Reference incident: a Windows user reported
|
||||
``ImportError: cannot import name 'TODO_INJECTION_HEADER' from
|
||||
'tools.todo_tool'`` on every startup after an update. ``agent/`` carried the
|
||||
new ``context_compressor.py`` (which imports that name at module level) while
|
||||
``tools/`` still held the pre-update ``todo_tool.py``. The ZIP-update path
|
||||
replaces top-level entries one at a time in ``os.listdir`` order, so an
|
||||
interruption between ``agent/`` and ``tools/`` produces exactly that skew --
|
||||
and the syntax guard reported the update as successful.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
from hermes_cli import main as hermes_main
|
||||
from hermes_cli import update_cmd
|
||||
from hermes_constants import partial_update_hint
|
||||
|
||||
|
||||
def _write_skewed_tree(root: Path, *, skewed: bool) -> None:
|
||||
"""Build a tiny two-package tree that mimics the real failure.
|
||||
|
||||
``consumer`` imports a name from ``provider`` at module level. When
|
||||
``skewed`` is True the name is absent -- both files still parse.
|
||||
"""
|
||||
(root / "provider").mkdir(parents=True, exist_ok=True)
|
||||
(root / "provider" / "__init__.py").write_text("")
|
||||
(root / "provider" / "thing.py").write_text(
|
||||
"OTHER = 1\n" if skewed else "SHARED_NAME = 'x'\nOTHER = 1\n"
|
||||
)
|
||||
(root / "consumer.py").write_text("from provider.thing import SHARED_NAME\n")
|
||||
|
||||
|
||||
def test_syntax_guard_passes_but_import_guard_catches_skew(monkeypatch, tmp_path):
|
||||
"""The regression: a skewed tree parses cleanly but cannot be imported."""
|
||||
_write_skewed_tree(tmp_path, skewed=True)
|
||||
|
||||
# Both files are valid Python -- the syntax guard sees nothing wrong.
|
||||
monkeypatch.setattr(
|
||||
hermes_main, "_UPDATE_CRITICAL_FILES", ("consumer.py", "provider/thing.py")
|
||||
)
|
||||
syntax_ok, _, _ = hermes_main._validate_critical_files_syntax(tmp_path)
|
||||
assert syntax_ok, "sanity: the skewed tree must parse cleanly"
|
||||
|
||||
# The import guard catches it.
|
||||
monkeypatch.setattr(update_cmd, "_UPDATE_CRITICAL_MODULES", ("consumer",))
|
||||
ok, module, error = hermes_main._validate_critical_modules_import(tmp_path)
|
||||
assert ok is False
|
||||
assert module == "consumer"
|
||||
assert error is not None and "SHARED_NAME" in error
|
||||
|
||||
|
||||
def test_import_guard_passes_on_consistent_tree(monkeypatch, tmp_path):
|
||||
_write_skewed_tree(tmp_path, skewed=False)
|
||||
monkeypatch.setattr(update_cmd, "_UPDATE_CRITICAL_MODULES", ("consumer",))
|
||||
|
||||
assert hermes_main._validate_critical_modules_import(tmp_path) == (True, None, None)
|
||||
|
||||
|
||||
def test_import_guard_ignores_non_import_errors(monkeypatch, tmp_path):
|
||||
"""A module that raises at import time for config/env reasons is not
|
||||
update breakage -- the guard must not roll back a good update."""
|
||||
(tmp_path / "consumer.py").write_text(
|
||||
"raise RuntimeError('no API key configured')\n"
|
||||
)
|
||||
monkeypatch.setattr(update_cmd, "_UPDATE_CRITICAL_MODULES", ("consumer",))
|
||||
|
||||
ok, _, _ = hermes_main._validate_critical_modules_import(tmp_path)
|
||||
assert ok is True
|
||||
|
||||
|
||||
def test_import_guard_is_non_fatal_when_probe_cannot_run(monkeypatch, tmp_path):
|
||||
"""If we can't spawn the probe, don't block the user's update."""
|
||||
|
||||
def boom(*_a, **_kw):
|
||||
raise OSError("cannot spawn")
|
||||
|
||||
monkeypatch.setattr(update_cmd.subprocess, "run", boom)
|
||||
assert update_cmd._validate_critical_modules_import(tmp_path) == (True, None, None)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# partial_update_hint
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def test_hint_fires_for_first_party_import_error():
|
||||
exc = ImportError("cannot import name 'TODO_INJECTION_HEADER'")
|
||||
exc.name = "tools.todo_tool"
|
||||
|
||||
hint = partial_update_hint(exc)
|
||||
|
||||
assert hint, "expected recovery guidance for a first-party ImportError"
|
||||
assert any("hermes update" in line for line in hint)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"exc",
|
||||
[
|
||||
ModuleNotFoundError("No module named 'numpy'", name="numpy"),
|
||||
ValueError("unrelated"),
|
||||
ImportError("third-party broke"),
|
||||
],
|
||||
)
|
||||
def test_hint_stays_silent_for_unrelated_failures(exc):
|
||||
"""Missing third-party deps and non-import errors have different
|
||||
remediation -- claiming a partial update would misdirect the user."""
|
||||
if isinstance(exc, ImportError) and not isinstance(exc, ModuleNotFoundError):
|
||||
exc.name = "requests"
|
||||
assert partial_update_hint(exc) == []
|
||||
Loading…
Reference in New Issue