fix(cli): target the managed install root in the ZIP update path - #71510
dtarkent2-sys wants to merge 1 commit into
Conversation
PROJECT_ROOT is derived from the running hermes_cli package, so it is the install root only when the `hermes` entry point on PATH belongs to the managed install. The two diverge when the entry point lives elsewhere — a `hermes` shim in a conda env in front of an agent installed under %LOCALAPPDATA%\hermes\hermes-agent. The ZIP update then copied the new tree into the shim's site-packages and ran `uv pip install -e .` there. That directory has no pyproject.toml, so uv exits 2 on every run and the install is left half updated: code replaced in the wrong place, dependencies never installed. Resolve the managed checkout explicitly and use it for both halves of the update — the file replacement and the dependency install, plus the venv the install targets, the bytecode clear, and the web build. PROJECT_ROOT is probed first and is also the final fallback, so installs where both roots already agree behave exactly as before. Threads cwd through _install_python_dependencies_with_optional_fallback → _run_quarantined_install → _run_install_with_heartbeat, and lets _venv_scripts_dir take the same root so shim quarantine follows the venv being written to. That plumbing follows the approach proposed in NousResearch#59866. Fixes NousResearch#59850 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks for isolating the divergent-root ZIP-update failure; the current implementation still has that premise: Problems
Suggested changes
This is an automated hermes-sweeper review. |
SummaryTwo PRs address the same reported Windows ZIP-update failure. #59866 adds cwd plumbing but still passes PROJECT_ROOT at the failing call sites, while #71510 resolves the managed install root and uses it for replacement and dependency installation, with a divergent-root regression test. Related pull requests
Duplicates#59866 and #71510 overlap on the cwd-plumbing portion; #59866 is superseded by #71510, whose diff additionally resolves and consistently uses the managed install root. Close #59866 as a duplicate once #71510's salvage work is incorporated. Suggested consolidationKeep #71510 open with a salvage path, specifically addressing the maintainer-bot keep_open review: port the implementation to the current update pipeline, resolve or explicitly scope custom Windows -InstallDir handling, and pass the managed root through Node dependency updates. Close #59866 as a duplicate of #71510 because its ZIP call sites still use PROJECT_ROOT and do not fix the reported divergent-root cause. Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I59850(["issue #59850 (open)"])
subgraph Dup59866 ["PRs duplicating each other"]
P59866["PR #59866 (open)"]
P71510["PR #71510 (open)"]
end
P71510 -->|best fix| I59850
class I59850 open
class P59866 open
class P71510 open
class P59866 best
class P71510 best
class P71510 target
click I59850 "https://github.com/NousResearch/hermes-agent/issues/59850"
click P59866 "https://github.com/NousResearch/hermes-agent/pull/59866"
click P71510 "https://github.com/NousResearch/hermes-agent/pull/71510"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 2 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 24 kB of PR diffs, 14 kB of issue/PR text, 3 kB of discussion (3 comments), 5 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
…ENV is stale When Hermes is installed via pip / site-packages (e.g. the Windows installer), PROJECT_ROOT is the interpreter's site-packages directory and PROJECT_ROOT/venv is never created. The update and interrupted-install recovery paths still set VIRTUAL_ENV=PROJECT_ROOT/venv, so uv fails with 'Failed to inspect Python interpreter from active virtual environment' before installing anything — leaving the install partially updated. Detect the nonexistent VIRTUAL_ENV in the shared dependency-install helper and pin uv to the running interpreter (uv pip install --python sys.executable) instead, matching the fix already applied to lazy-deps (#83335) and the ZIP update path (#71510).
…ENV is stale When Hermes is installed via pip / site-packages (e.g. the Windows installer), PROJECT_ROOT is the interpreter's site-packages directory and PROJECT_ROOT/venv is never created. The update and interrupted-install recovery paths still set VIRTUAL_ENV=PROJECT_ROOT/venv, so uv fails with 'Failed to inspect Python interpreter from active virtual environment' before installing anything — leaving the install partially updated. Detect the nonexistent VIRTUAL_ENV in the shared dependency-install helper and pin uv to the running interpreter (uv pip install --python sys.executable) instead, matching the fix already applied to lazy-deps (#83335) and the ZIP update path (#71510).
…ENV is stale When Hermes is installed via pip / site-packages (e.g. the Windows installer), PROJECT_ROOT is the interpreter's site-packages directory and PROJECT_ROOT/venv is never created. The update and interrupted-install recovery paths still set VIRTUAL_ENV=PROJECT_ROOT/venv, so uv fails with 'Failed to inspect Python interpreter from active virtual environment' before installing anything — leaving the install partially updated. Detect the nonexistent VIRTUAL_ENV in the shared dependency-install helper and pin uv to the running interpreter (uv pip install --python sys.executable) instead, matching the fix already applied to lazy-deps (NousResearch#83335) and the ZIP update path (NousResearch#71510).
What does this PR do?
Makes the Windows ZIP update path write to the install it is actually managing.
PROJECT_ROOTis derived from the runninghermes_clipackage, so it equals the install root only when thehermesentry point on PATH belongs to the managed install. When they diverge — ahermesshim in a conda/miniforge env in front of an agent installed under%LOCALAPPDATA%\hermes\hermes-agent—_update_via_zipcopies the freshly downloaded tree into the shim'ssite-packagesand then runsuv pip install -e .in that same directory. There is nopyproject.tomlthere, so uv exits 2 on every run and the install is left half updated: files replaced in the wrong place, dependencies never installed._resolve_managed_install_root()probes, in order:PROJECT_ROOT— so nothing changes when both roots already agree$HERMES_INSTALL_DIR— the installer's own--install-diroverridesys.prefix/$VIRTUAL_ENV— the managed venv lives at<install root>/venvget_default_hermes_root() / "hermes-agent"— the installer default (%LOCALAPPDATA%\hermes\hermes-agenton Windows,$HERMES_HOME/hermes-agenton POSIX)/usr/local/lib/hermes-agent— the Linux root-install locationThe first candidate that looks like a hermes-agent checkout (
pyproject.toml+hermes_cli/main.py) wins, andPROJECT_ROOTis also the final fallback — an unrecognized layout keeps today's behavior rather than updating some unrelated directory.Related Issue
Fixes #59850
Builds on @liuhao1024's #59866, which correctly identified the
cwdplumbing but kept passingPROJECT_ROOTat the ZIP call sites, so the failing directory selection was unchanged (per the review on that PR). This adds the distinct resolved install root that review asked for, uses it for both the replacement and the dependency install, and adds the divergent-root regression test. Happy to close this in favour of #59866 if @liuhao1024 would rather fold the diff into their branch.Type of Change
Changes Made
hermes_cli/main.py_looks_like_hermes_checkout()/_resolve_managed_install_root()_update_via_zip()resolves the install root once and uses it for the file replacement, bytecode clear,VIRTUAL_ENV, dependency install, web build, and model-catalog seed; prints both paths when they divergecwdthreaded through_install_python_dependencies_with_optional_fallback→_run_quarantined_install→_run_install_with_heartbeat(defaults toPROJECT_ROOT)_venv_scripts_dir(root=None)so shim quarantine follows the venv being written totests/hermes_cli/test_update_zip_install_root.py— newtests/hermes_cli/test_verify_console_scripts.py— mock assertion picks up the newcwdkwargHow to Test
Reproduce (Windows,
hermesentry point outside the managed install):The same command with the cwd corrected by hand succeeds, which is what isolates it to the directory selection:
Automated:
test_update_via_zip_writes_and_installs_into_managed_rootbuilds the divergent case — asite-packagesmodule root that is not a checkout plus a separate managed checkout — and asserts the ZIP content lands in the managed root, not in the module root, and that the dependency install receives that same directory as itscwdwith its venv asVIRTUAL_ENV. All six tests in the new file fail againstmainand pass with this change.Checklist
Code
Test run:
tests/hermes_cli/update+install files — 112 passed. Two pre-existing failures intest_cmd_update.py(test_resolve_rejects_windows_npm_and_rescans_path,test_update_refreshes_repo_and_tui_node_dependencies) reproduce unchanged onmainon a native-Windows host; they simulate a WSL npm layout.Documentation & Housekeeping
cli-config.yaml.example— N/A (no new config keys;HERMES_INSTALL_DIRis the installer's existing flag)CONTRIBUTING.md/AGENTS.md— N/APROJECT_ROOTfirst and are unaffectedKnown remaining gap
_update_node_dependencies()is stillPROJECT_ROOT-relative, as is the git-pull update path in_cmd_update_impl. Both are the same class of assumption, but threading a root through them is a wider change than this fix needs, so it is left for a follow-up —_resolve_managed_install_root()is written to be reusable there. On a divergent install the Node step is a no-op today (nopackage.jsonin the shim's directory) rather than a corruption.