fix(update): harden venv rebuild + verify core deps after install#38887
Merged
Conversation
Contributor
🔎 Lint report:
|
| Rule | Count |
|---|---|
unused-type-ignore-comment |
1 |
unresolved-import |
1 |
unresolved-attribute |
1 |
First entries
hermes_cli/main.py:9122: [unused-type-ignore-comment] unused-type-ignore-comment: Unused blanket `type: ignore` directive
tests/hermes_cli/test_verify_core_dependencies.py:24: [unresolved-import] unresolved-import: Cannot resolve imported module `pytest`
hermes_cli/main.py:9150: [unresolved-attribute] unresolved-attribute: Object of type `~None` has no attribute `evaluate`
✅ Fixed issues: none
Unchanged: 5086 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
Two complementary fixes for a silent partial-install failure that bit
``hermes update`` in the wild: a fresh checkout pulled 145 commits,
``rebuild_venv`` failed to recreate the venv on Windows because
``shutil.rmtree(ignore_errors=True)`` couldn't delete files held open by
the running ``hermes.exe`` shim. ``uv venv`` then refused with
"A directory already exists at: venv" and the update fell back to
installing on top of the stale venv. The resulting partial install
missed exactly one newly-added base dep — ``pathspec==1.1.1`` — which
``hermes desktop --build-only`` imports at the top of its content-hash
check. The desktop rebuild died with ModuleNotFoundError and the parent
update only logged "⚠ Desktop build failed (non-fatal)". Same root cause
made the "default: sync failed" line in the skill-sync stage, because
that sync subprocess hit the same missing import.
Fix 1: ``rebuild_venv`` retries with ``--clear``
------------------------------------------------
If ``uv venv`` fails with "already exists" in stderr (which is what uv
prints, and what uv's own hint tells you to fix with --clear), retry
once with ``--clear``. Only this specific failure pattern triggers the
retry — disk-full / interpreter-download failures still surface as
before so we don't mask real problems.
Fix 2: post-install dep verification
------------------------------------
Belt-and-suspenders so future uv resolver quirks (or any other cause of
partial installs) surface immediately instead of hours later in a
downstream subprocess. After ``_install_python_dependencies_with_optional_fallback``
runs, ``_verify_core_dependencies_installed``:
1. Reads ``[project.dependencies]`` straight from pyproject.toml
(so we don't trust the venv's stale metadata).
2. Filters by environment markers via ``packaging.requirements.Requirement``
so cross-platform exclusions (``ptyprocess ; sys_platform != 'win32'``)
don't false-positive on Windows.
3. Runs ``importlib.metadata.version()`` for each remaining dep inside
the *target* venv interpreter (resolved from ``VIRTUAL_ENV``, not
``sys.executable``).
4. If anything is missing, reinstalls the base group with
``--reinstall`` to force re-resolution. If a second probe still
reports missing deps, force-installs each one with its pinned spec.
5. Treats final failure as a warning rather than a hard error — a
single broken-on-PyPI dep shouldn't block an otherwise-successful
update — but the message points at ``hermes update --force`` and
names the missing packages so the user knows what's wrong.
Tests
-----
- ``TestRebuildVenv::test_retries_with_clear_when_dir_already_exists`` —
simulates the rmtree-couldn't-delete-it failure mode and asserts the
``--clear`` retry path is taken and succeeds.
- ``TestRebuildVenv::test_does_not_retry_when_first_failure_is_not_dir_exists``
— guards against masking real failures (disk full, etc.).
- ``test_verify_core_dependencies.py`` — 7 tests covering the happy
path, the regression (missing pathspec triggers --reinstall), the
per-package fallback when --reinstall doesn't help, the platform-
marker filter so Windows doesn't try to install ptyprocess, the
missing-pyproject noop, and the VIRTUAL_ENV resolver.
Co-authored-by: Kyssta <218078013+kyssta-exe@users.noreply.github.com>
661a2b4 to
285afd6
Compare
This was referenced Jun 4, 2026
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a silent partial-install failure in
hermes updatethat broke desktop GUI rebuilds and per-profile skill sync in the wild.What broke
A real-world
hermes updaterun on Windows (logs: this morning, my local box):managed_uv.rebuild_venv()tried to recreate the venv.shutil.rmtree(ignore_errors=True)couldn't delete files held open by the runninghermes.exeshim (and AV scanner). Some files were left behind.uv venvthen errored withCaused by: A directory already exists at: venv(and uv's own hint said to use--clear). Return value was ignored.pathspec==1.1.1.hermes desktop --build-only(called next) importspathspecat the top of its content-hash skip check. It died withModuleNotFoundError. The parent update only saw a non-zero exit code and printed⚠ Desktop build failed (non-fatal; run hermes desktop to retry).Net result: a "successful" update with a broken desktop app and broken profile sync.
Two complementary fixes
1.
rebuild_venvretries with--clearIf
uv venvfails withalready existsin stderr, retry once with--clear. Only matches that specific failure pattern — disk-full / interpreter-download errors still surface unchanged so we don't mask real problems.2. Post-install dep verification (
_verify_core_dependencies_installed)Belt-and-suspenders so future uv resolver quirks (or any other cause of partial installs) surface immediately instead of hours later in a downstream subprocess:
[project.dependencies]straight frompyproject.toml(doesn't trust the venv's stale metadata).packaging.requirements.Requirementso cross-platform exclusions (ptyprocess ; sys_platform != 'win32') don't false-positive on Windows.importlib.metadata.version()for each remaining dep inside the target venv interpreter (resolved fromVIRTUAL_ENV, notsys.executable— they're different during update).--reinstallto force re-resolution. If still missing, force-install each one with its pinned spec from pyproject.hermes update --forceso the user knows what to do.Test Plan
hermes desktop --build-only, desktop rebuild now succeeds.TestRebuildVenv::test_retries_with_clear_when_dir_already_existssimulates the rmtree-couldn't-delete-it failure mode and asserts the--clearretry path is taken and succeeds.TestRebuildVenv::test_does_not_retry_when_first_failure_is_not_dir_existsguards against masking real failures (disk full, etc.).test_verify_core_dependencies.py— 7 tests covering happy path, missing-dep triggers--reinstall, per-package fallback, platform-marker filter (Windows doesn't try ptyprocess), missing-pyproject is a no-op, and theVIRTUAL_ENVresolver.TestRebuildVenvtests still pass.ruff checkclean on all modified files.Known pre-existing test failures (unrelated)
Confirmed by running tests against unmodified
origin/main:test_managed_uv.py(TestResolveUv, TestEnsureUv, TestUpdateManagedUv, TestInstallUvInternals) fail on Windows due to fake-executable subprocess permission issues — pre-existing, not introduced here.test_cmd_update.pyandtest_update_autostash.pyfail becausecmd_updatenow wraps git calls with-cconfig args that the tests' assertions don't account for — pre-existing, not introduced here.These deserve their own PR; flagging so reviewers don't think they're regressions.