Skip to content

fix(update): verify core deps after lazy refresh failures - #58246

Closed
tianma-if wants to merge 1 commit into
NousResearch:mainfrom
tianma-if:codex/fix-lazy-refresh-core-verify
Closed

fix(update): verify core deps after lazy refresh failures#58246
tianma-if wants to merge 1 commit into
NousResearch:mainfrom
tianma-if:codex/fix-lazy-refresh-core-verify

Conversation

@tianma-if

Copy link
Copy Markdown
Contributor

Summary

  • pass the update install target into the lazy-backend refresh step
  • rerun the existing core dependency verifier when a lazy refresh fails or unexpectedly raises
  • keep successful lazy refreshes on the current no-op path

Why

A failed optional lazy backend refresh can partially touch the venv and leave core packages corrupted. This reuses the existing verifier/repair path immediately after the risky step, before the update continues into Node/UI work.

Addresses #57828.

Tests

  • .venv/bin/python -m pytest tests/hermes_cli/test_verify_core_dependencies.py -q
  • .venv/bin/python -m pytest tests/hermes_cli/test_update_venv_health.py tests/hermes_cli/test_verify_core_dependencies.py -q
  • .venv/bin/python -m py_compile hermes_cli/main.py tests/hermes_cli/test_verify_core_dependencies.py
  • .venv/bin/python -m ruff check hermes_cli/main.py tests/hermes_cli/test_verify_core_dependencies.py
  • git diff --check

@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists labels Jul 4, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Comment

Summary

Adds verification of core dependencies after a failed lazy refresh during the update process. This prevents partially-failed optional backend installs from corrupting the base venv.

Looks Good

  • Well-scoped: 2 files (main.py + test), 114 additions
  • Clear docstring explaining the regression scenario
  • Proper fallback path: runs core dep verifier on both exception and partial-failure paths
  • Tests added covering the verification trigger

Reviewed by Hermes Agent

@teknium1 teknium1 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.

Thanks for targeting the post-refresh recovery path. Current main still reports failed lazy refreshes without re-running the existing core-dependency repair (hermes_cli/main.py:7614-7622), while the verifier already repairs missing declared packages (hermes_cli/main.py:7837-8017).

Problems

  • The new keyword-argument call at hermes_cli/main.py:9933-9937 breaks the shared update mock at tests/hermes_cli/test_update_autostash.py:396, which is lambda: None. Update tests using _setup_update_mocks() will raise TypeError when the update reaches the refresh step.

Suggested changes

  • Update that mock to accept *args, **kwargs, preferably capturing the arguments and asserting the update path forwards the selected install target.
  • Add coverage through _cmd_update_impl, not only the helper, for forwarding the uv/pip target into _refresh_active_lazy_features.

Automated hermes-sweeper review.

Comment thread hermes_cli/main.py
@@ -9904,7 +9930,11 @@ def _cmd_update_impl(args, gateway_mode: bool):
# UI, desktop rebuild) are non-core and can't brick the venv.
_clear_update_incomplete_marker()

_refresh_active_lazy_features()
_refresh_active_lazy_features(

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.

This new keyword-argument call breaks the shared update-test stub at tests/hermes_cli/test_update_autostash.py:396 (lambda: None). Update that mock to accept *args, **kwargs or capture and assert these arguments; otherwise tests using _setup_update_mocks() raise TypeError here.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
@tianma-if
tianma-if force-pushed the codex/fix-lazy-refresh-core-verify branch from d820f7c to 81d3364 Compare July 16, 2026 07:18
@teknium1 teknium1 added the area/install-update Installer, updater, packaging, wheels, doctor label Jul 19, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Closing as superseded by PR #69867 (#69867), which fixes #57828 via import-based probes rather than metadata verification. The reported failure state leaves distribution metadata intact while import files are wiped, so _verify_core_dependencies_installed() (metadata-based) can miss it — the merged fix probes real imports and force-reinstalls with pyproject pins. Thanks for the contribution and for moving repair into the failed-refresh path — that shape carried into the final design.

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:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants