From bf18710a546b42740fcc01ab85ad0d3d23c1a34f Mon Sep 17 00:00:00 2001 From: kshitij <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 1 Aug 2026 16:01:13 +0530 Subject: [PATCH] fix(update): make the git-path import check non-destructive MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2 review (C2) and /simplify-code findings. C2 — the git path ran the import guard before `_clear_bytecode_cache`, wired into the syntax guard's `git reset --hard` rollback. But `cannot import name 'X'` is ALSO the documented signature of the stale-bytecode class (#6207, #60242, see _sweep_stale_bytecode_if_checkout_changed), which the very next steps — and the launch-time sweep — already self-heal. A false positive there would destroy a good update over a state that fixes itself. Remove the guard from the rollback path entirely and re-add it at the end of the git path, after bytecode sweep + dependency reinstall + lazy refresh, as a WARNING only. By then every benign source of a transient ImportError has run, and we never reset the user's checkout. W6 — the headline regression test was vacuous: it patched `hermes_main._UPDATE_CRITICAL_FILES`, but the syntax guard reads `update_cmd`'s global, so the stub files were never examined and the (True, None, None) came from "no files found" rather than "parses clean". Patch the right module; mutation-checked (the test now fails when the guard is disabled). S5 — `startswith(("tools","agent","hermes","gateway"))` also matched third-party `agents`/`agentops`/`toolsets`. Compare the first dotted segment against an exact set instead. S6 — hoist the per-line ChatConsole() instantiation. --- hermes_cli/cli_agent_setup_mixin.py | 5 +-- hermes_cli/update_cmd.py | 33 +++++++++++--------- hermes_constants.py | 8 ++++- tests/hermes_cli/test_update_import_guard.py | 24 +++++++++++++- 4 files changed, 52 insertions(+), 18 deletions(-) diff --git a/hermes_cli/cli_agent_setup_mixin.py b/hermes_cli/cli_agent_setup_mixin.py index 8b3ef0f1399d5..759c58c2eddcc 100644 --- a/hermes_cli/cli_agent_setup_mixin.py +++ b/hermes_cli/cli_agent_setup_mixin.py @@ -461,11 +461,12 @@ class CLIAgentSetupMixin: # Keep _pending_title so it can be retried after row creation succeeds return True except Exception as e: - ChatConsole().print(f"[bold red]Failed to initialize agent: {e}[/]") + console = ChatConsole() + console.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) + console.print(line) return False def _preload_resumed_session(self) -> bool: diff --git a/hermes_cli/update_cmd.py b/hermes_cli/update_cmd.py index b87e99ef73174..42c3382352dad 100644 --- a/hermes_cli/update_cmd.py +++ b/hermes_cli/update_cmd.py @@ -3819,22 +3819,9 @@ 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(f"✗ Pulled code {failure_label} in a critical file:") + print("✗ Pulled code has a syntax error in a critical file:") print(f" {failing_path}") if syntax_error: # py_compile errors can be multi-line; show the first @@ -4015,6 +4002,24 @@ def _cmd_update_impl(args, gateway_mode: bool): # plugin.yaml-declared deps that aren't in extras (#53272, #70636). _m()._refresh_active_memory_provider_dependencies() + # Everything that can legitimately produce a transient ImportError has + # now run (bytecode sweep, dependency reinstall, lazy refresh), so a + # module that still won't import is real breakage. Warn only — never + # roll back here: `cannot import name X` is also the signature of the + # stale-bytecode class (#6207, #60242), and the launch-time sweep in + # _sweep_stale_bytecode_if_checkout_changed() self-heals that on the + # next run. A destructive reset would undo a good update over a state + # that fixes itself. + import_ok, failing_module, import_error = _validate_critical_modules_import( + _m().PROJECT_ROOT + ) + if not import_ok: + print() + print(f" ⚠ {failing_module} still fails to import after updating:") + print(f" {import_error}") + print(" Run `hermes update` again — if it persists, reinstall:") + print(" https://hermes-agent.nousresearch.com") + node_failures = _update_node_dependencies() _m()._build_web_ui(_m().PROJECT_ROOT / "web") diff --git a/hermes_constants.py b/hermes_constants.py index 4ea0c8376d1e5..2f86ce88949ad 100644 --- a/hermes_constants.py +++ b/hermes_constants.py @@ -1276,7 +1276,13 @@ def partial_update_hint(exc: BaseException) -> list[str]: if isinstance(exc, ModuleNotFoundError): return [] name = getattr(exc, "name", None) - if not name or not str(name).startswith(("tools", "agent", "hermes", "gateway")): + # Compare the first dotted segment against an exact set: a bare + # ``startswith`` would also match third-party ``agents``, ``agentops``, + # ``toolsets``, etc. and blame our updater for their import errors. + root = str(name).split(".")[0] if name else "" + if root not in {"tools", "agent", "gateway", "plugins", "providers"} and not ( + root == "cli" or root.startswith("hermes_") or root == "hermes" + ): return [] return [ "", diff --git a/tests/hermes_cli/test_update_import_guard.py b/tests/hermes_cli/test_update_import_guard.py index a8cee3515c3a4..fbd14223e1405 100644 --- a/tests/hermes_cli/test_update_import_guard.py +++ b/tests/hermes_cli/test_update_import_guard.py @@ -44,8 +44,13 @@ def test_syntax_guard_passes_but_import_guard_catches_skew(monkeypatch, tmp_path _write_skewed_tree(tmp_path, skewed=True) # Both files are valid Python -- the syntax guard sees nothing wrong. + # NOTE: patch update_cmd's global, not hermes_main's. Both modules expose + # the name, but _validate_critical_files_syntax reads the one in its own + # module. Patching the re-export leaves the real list in place, the stub + # files are never looked at, and the guard returns a vacuous (True, None, + # None) that would make this test pass no matter what the code did. monkeypatch.setattr( - hermes_main, "_UPDATE_CRITICAL_FILES", ("consumer.py", "provider/thing.py") + update_cmd, "_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" @@ -171,3 +176,20 @@ def test_import_guard_flags_missing_first_party_module(monkeypatch, tmp_path): assert ok is False assert module == "consumer" assert error is not None and "tools.nonexistent_module" in error + + +@pytest.mark.parametrize("modname", ["agents", "agentops", "toolsets_x", "hermesx"]) +def test_hint_does_not_claim_partial_update_for_lookalike_third_party(modname): + """``startswith`` would match third-party ``agents``/``agentops`` and blame + our updater for someone else's import error.""" + exc = ImportError("boom") + exc.name = modname + assert partial_update_hint(exc) == [] + + +@pytest.mark.parametrize("modname", ["tools.todo_tool", "agent.context_compressor", + "hermes_constants", "cli"]) +def test_hint_fires_for_each_first_party_root(modname): + exc = ImportError("cannot import name 'X'") + exc.name = modname + assert partial_update_hint(exc), f"expected guidance for {modname}"