Skip to content

fix(update): reload config's import surface before config in the in-place updater - #90554

Open
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-90535
Open

fix(update): reload config's import surface before config in the in-place updater#90554
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-90535

Conversation

@liuhao1024

@liuhao1024 liuhao1024 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

hermes update runs in the PRE-pull Python process, and after the pull _reload_config_modules() force-reloads hermes_cli.config from disk. But importlib.reload re-executes config's module-level from X import Y against the cached X in sys.modules. When d0132b5 added from hermes_cli.cli_output import line_input to config.py, an updater process whose cached cli_output predated the symbol died with cannot import name 'line_input' and aborted the gateway auto-restart — the checkout and venv were healthy; only the mixed-generation sys.modules was broken (#90535).

The fix reloads every hermes_cli module that config imports at module level (colors, secret_prompt, cli_output, route_identity, default_soul, personality) before config itself, with the dependency ordering documented in the docstring. All six are leaf modules with no hermes_cli imports of their own, so the order among them is free but all must precede config. This covers not just the cli_output/line_input instance but any future symbol added to config's import surface.

Related Issue

Refs #90535 — addresses the current config import-surface manifestation; coherent-generation post-pull orchestration remains open under #90535 / #90145 / #90144 (intentionally non-closing per review).

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

How to Test

  1. python -m pytest tests/hermes_cli/test_update_stale_dashboard.py -q — should pass (37 passed, 2 skipped), including the two new tests
  2. python -m pytest tests/hermes_cli/test_cmd_update.py tests/hermes_cli/test_update_yes_flag.py -q — should pass (no collateral damage to the update suites; 78 passed combined)
  3. Observed result: with the fix stashed, both new tests fail (cli_output missing from the reload list / stale symbol not restored); with the fix, all green

Checklist

  • Code follows the project's style guidelines
  • Self-review completed
  • Comments added for complex logic (reload ordering rationale in docstring)
  • Tests added that prove the fix (dependency-order assertion + stale-symbol restoration)
  • All new and existing tests pass locally (update suites: 78 + 57 passed)
  • Platform: macOS (tests are platform-independent; the bug reproduces on Linux/macOS git installs alike)

…lace updater

hermes update runs in the PRE-pull Python process. After the pull,
_reload_config_modules force-reloads config — but importlib.reload
re-executes config's module-level `from X import Y` against the CACHED
X in sys.modules. When d0132b5 added `from hermes_cli.cli_output
import line_input` to config.py, an updater process whose cached
cli_output predated the symbol died with "cannot import name
'line_input'" and aborted the gateway auto-restart (NousResearch#90535) — the
checkout and venv were healthy; only the mixed-generation sys.modules
was broken.

Reload every hermes_cli module config imports at module level (colors,
secret_prompt, cli_output, route_identity, default_soul, personality)
BEFORE config itself, with the dependency ordering documented. All six
are leaf modules with no hermes_cli imports of their own, so the order
among them is free but all must precede config.
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard area/install-update Installer, updater, packaging, wheels, doctor P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 20, 2026

Copy link
Copy Markdown
Contributor

Architecture note for review: this PR is a valid tactical repair for #90535, but it should not be treated as closure of the mixed-generation updater class tracked by #90145.

Reloading config's current import surface reduces today's failure modes; it does not establish that the mutation-bearing post-pull import graph is wholly generation-coherent. A future dependency edge can recreate the same N/N+1 sys.modules mixture outside this enumerated set.

Please preserve the distinction in merge/closure semantics:

Interlocks: #90145 (generation authority) and #90144 (proof scope must cover the actual execution/mutation graph).

Copy link
Copy Markdown
Contributor

One additional acceptance note from the packaged-artifact side of the same architecture: this tactical reload fix should be validated against the post-update runtime users actually execute, not only against the helper’s reload list.

The important chain is checkout N+1 → installed dependency state N+1 → post-update interpreter/module graph N+1 → restarted runtime N+1. A passing reload test proves only one interior transformation. It does not prove the restarted process is rooted in one coherent installed generation.

That does not expand this PR’s implementation scope; it clarifies why the existing #90535/#90145 architecture residue must remain open after this manifestation is repaired. #89875, #88233, and #82450 are the corresponding bundle/native/install precedents: earlier-stage success is evidence, not acceptance of the final executable artifact.

@liuhao1024

Copy link
Copy Markdown
Contributor Author

Thanks for both notes — the boundaries are clear and I agree with them.

To confirm my understanding: this PR only repairs the enumerated import surface of hermes_cli.config, and the regression tests prove one interior transformation of the chain you described (checkout N+1 → installed dependency state N+1 → post-update interpreter/module graph N+1 → restarted runtime N+1), not that the restarted runtime is rooted in one coherent installed generation. The reload list is static, but the first regression test fails if config's module-level import surface ever grows beyond it. What it cannot cover is a new import edge introduced elsewhere in the update path — that could still recreate an N/N+1 sys.modules mixture outside this enumerated set, which is exactly why the mixed-generation residue should stay open under #90145 / #90144 rather than be treated as closed by this tactical fix.

One concrete question on merge semantics: the body currently says Fixes #90535. If #90535 also carries the broader architecture layer you want kept open, I'll switch it to a non-closing reference (e.g. "Addresses the config import-surface manifestation of #90535; the broader mixed-generation orchestration stays open under #90145/#90144") so merging doesn't auto-close it. If #90535 is the concrete manifestation and the architecture lives in the interlocked issues, the current form is already correct — happy to adjust either way.

Copy link
Copy Markdown
Contributor

Yes—please switch Fixes #90535 to a non-closing reference.

#90535 is not limited to the single cli_output.line_input manifestation. Its issue body and architecture interlocks now own the updater-specific closure contract: after the checkout changes generation, mutation-bearing migration/restart/cleanup must execute under one coherent N+1 code generation, and the restarted runtime must reach its own typed terminal acceptance result. #90145 and #90144 carry the repository-wide invariants; they do not replace #90535 as the concrete updater implementation owner.

Suggested wording:

Addresses the enumerated hermes_cli.config import-surface manifestation of #90535. The updater generation-coherence and end-to-end runtime acceptance work remains open in #90535, interlocked with #90145 and #90144.

This PR can merge as tactical containment, but it should not auto-close #90535.

Copy link
Copy Markdown
Contributor

Please switch the body to non-closing semantics: Refs #90535, with wording such as “addresses the current config import-surface manifestation; coherent-generation post-pull orchestration remains open under #90535 / #90145 / #90144.”

#90535 itself states the broader invariant (“An update should not execute newly pulled modules against stale cached dependencies”) and names a fresh subprocess as the stronger direction. This PR proves the enumerated config seam, not arbitrary post-pull dependency coherence, so Fixes #90535 would auto-close more than the diff proves.

Exact-head CI 32336809924, Docker 32336809325, and Nix 32336809309 are all green, so after the body topology is corrected I regard this tactical slice as contributor-complete.

Closure-sweep receipt: I attempted to make the body-only change directly, but GitHub rejected PR metadata mutation with 403 Resource not accessible by integration; the requested textual change is therefore the only remaining contributor-side action.

@andrexibiza andrexibiza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact head c3019206a94e1fa4fb82144fc242babab4ea78b3 after the merge-semantics correction. The PR now uses a non-closing Refs #90535, accurately scopes itself to the enumerated hermes_cli.config import surface, and preserves #90535/#90145/#90144 as the coherent-generation closure owners. Exact-head CI 32336809924, Docker 32336809325, and Nix 32336809309 are all green. No remaining code-review blocker for this tactical containment slice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/install-update Installer, updater, packaging, wheels, doctor comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants