Skip to content

fix(install): keep venv Scripts dir off the user PATH (#83797) - #83830

Closed
zuowen7 wants to merge 3 commits into
NousResearch:mainfrom
zuowen7:fix/83797-no-venv-scripts-on-user-path
Closed

fix(install): keep venv Scripts dir off the user PATH (#83797)#83830
zuowen7 wants to merge 3 commits into
NousResearch:mainfrom
zuowen7:fix/83797-no-venv-scripts-on-user-path

Conversation

@zuowen7

@zuowen7 zuowen7 commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

On Windows, installs made by the pre-2026-08 install.ps1 prepend the venv Scripts dir (<install>\venv\Scripts) to the persisted User PATH so hermes is globally available. That dir also contains python.exe/pip.exe, so every new shell resolves python to the Hermes runtime instead of the user's own interpreter (reproduced on a real Windows 11 host: where.exe python returned ...\hermes-agent\venv\Scripts\python.exe ahead of the user's own Anaconda / Python 3.14).

This PR keeps the command surface global without hijacking python:

  • install.ps1 Set-PathVariable no longer puts venv\Scripts on the user PATH. It ships hermes/hermes-acp as .cmd forwarding shims in %LOCALAPPDATA%\hermes\bin (a Hermes-owned dir holding only Hermes-specific commands like uv) and runs a new Update-UserPathForHermes migration that drops the stale venv\Scripts entry from the User PATH, preserving every other entry (order, empty segments, case-insensitive).
  • hermes_cli/update_cmd.py _ensure_acp_launcher's Windows branch (previously a no-op relying on venv\Scripts being on PATH) now (re)creates the acp shim, since hermes update does not re-run install.ps1.
  • hermes_cli/uninstall.py _hermes_path_markers now also matches %LOCALAPPDATA%\hermes\bin so uninstall sweeps the shim dir.

Note on the 0.19.0 .hermes-runtime\python\... entry mentioned in the issue: the current codebase has never written .hermes-runtime to the user PATH (it is an internal runtime dir); that entry lives under the hermes-agent prefix and is already swept by the uninstaller's hermes-agent marker. This PR targets the current-installer layout (venv\Scripts).

Related Issue

Fixes #83797

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • scripts/install.ps1Set-PathVariable rewritten; new New-HermesShims + Update-UserPathForHermes (migration); backslash normalization + -NoNewline hardening
  • hermes_cli/update_cmd.py — new _ensure_windows_acp_shim(); Windows branch of _ensure_acp_launcher no longer a no-op; silent degradation for custom install dirs documented
  • hermes_cli/uninstall.py_hermes_path_markers now also matches %LOCALAPPDATA%\hermes\bin
  • scripts/ci/test_install_ps1_hermes_shim_path.ps1 — new behavior test (AST-lifted, same harness as test_install_ps1_path_migration.ps1; 24 assertions incl. New-HermesShims coverage)
  • tests/hermes_cli/test_ensure_acp_launcher.py — 4 new Windows-shim unit tests

How to Test

  1. pwsh -NoProfile -File scripts/ci/test_install_ps1_path_migration.ps1 — passes (no regression)
  2. pwsh -NoProfile -File scripts/ci/test_install_ps1_hermes_shim_path.ps1 — 24 assertions pass
  3. pytest tests/hermes_cli/test_ensure_acp_launcher.py — 4 new tests pass; the 2 pre-existing failures are environment-only on Windows (symlink perms / os.geteuid), identical failures on unmodified main (verified via git stash comparison)
  4. Real-host migration (Windows 11): ran the shipped Update-UserPathForHermes + New-HermesShims against the live registry (backed up first). After: where.exe pythonD:\env\anaconda\python.exe (user's own); hermes --version → v0.20.0 via ...\hermes\bin\hermes.cmd; hermes-acp shim present.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass — targeted files only; full suite not run locally, pre-existing Windows environment failures documented above
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Windows 11

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

Real-host verification (fresh-shell PATH reconstructed from registry):

> where.exe python
D:\env\anaconda\python.exe
C:\Users\zuowen\AppData\Local\Programs\Python\Python314\python.exe

> hermes --version
Hermes Agent v0.20.0 (2026.8.3)
Install directory: C:\Users\zuowen\AppData\Local\hermes\hermes-agent

zuowen7 and others added 3 commits August 11, 2026 18:01
…3797)

Pre-2026-08 installers prepended <install>\venv\Scripts to the persisted
User PATH so `hermes` would be globally available. That dir also hosts
python.exe/pip.exe, so every new shell resolved `python` to the Hermes
runtime instead of the user's own interpreter.

- install.ps1: Set-PathVariable now ships hermes/hermes-acp as .cmd
  forwarding shims in $LOCALAPPDATA\hermes\bin (a Hermes-owned dir that
  holds only Hermes-specific commands) and runs Update-UserPathForHermes,
  which migrates existing installs by dropping the stale venv\Scripts
  entry while preserving all other entries.
- update_cmd.py: _ensure_acp_launcher's Windows branch (previously a
  no-op relying on venv\Scripts being on PATH) now (re)creates the acp
  shim, since `hermes update` does not re-run install.ps1.
- Tests: behavior test for Update-UserPathForHermes (AST-lifted, same
  harness as test_install_ps1_path_migration.ps1) plus Windows shim
  unit tests for the acp launcher.

Verified on a real Windows host: after migration, where.exe python
resolves to the user's own interpreter and hermes/hermes-acp still
resolve via the bin shims.
The new install.ps1 (NousResearch#83797) prepends %LOCALAPPDATA%\hermes\bin (not
venv\Scripts) to the User PATH.  _hermes_path_markers was missing this
entry, so the uninstaller left it behind.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Update-UserPathForHermes: TrimEnd backslash on ShimDir so registry-style
  trailing slashes cannot defeat membership detection (mirrors the legacy
  entry handling).
- New-HermesShims: Set-Content -NoNewline so shim bytes exactly match the
  intended content (Set-Content appends a newline otherwise, breaking the
  idempotence check).
- _ensure_windows_acp_shim: document the silent degradation for custom
  install dirs.
- Behavior test now also covers New-HermesShims (create, idempotence,
  missing-target skip, empty args) and the trailing-backslash case (24
  assertions total).
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard platform/windows Native Windows-specific behavior or breakage area/install-update Installer, updater, packaging, wheels, doctor P2 Medium — degraded but workaround exists sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 11, 2026
@zuowen7

zuowen7 commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

@teknium1 — thanks for #84452, the $InstallDir\bin approach covers the main PATH issue.

One part of this PR that one doesn't cover: uninstall cleanup (hermes_cli/uninstall.py). #84452 migrates the legacy path at install time, but nothing removes $InstallDir\bin from the user PATH when Hermes is uninstalled — it stays behind. This PR has that cleanup (plus shim-path hardening + tests).

Happy to trim it down to just the uninstall piece if that keeps the diff cleaner.

@zuowen7

zuowen7 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

Correction on my earlier comment — I re-checked, and this PR has no independent value left: _hermes_path_markers already sweeps hermes\hermes-agent\bin via the hermes-agent prefix, so the uninstall cleanup is already covered, and the other two commits (shim handling, venv Scripts off PATH) are superseded by #84452. Closing to keep the queue clean. The root-cause analysis in #83797 still stands — #84452 adopted the same fix. Thanks for the quick turnaround.

@zuowen7 zuowen7 closed this Aug 13, 2026
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 platform/windows Native Windows-specific behavior or breakage 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.

[Bug]: Hermes changes the default python command on Windows

2 participants