fix(terminal): strip Hermes-venv site-packages from terminal subprocess PYTHONPATH - #61028
Conversation
…ss PYTHONPATH to prevent cross-version ABI conflicts The Desktop Electron process injects the Hermes venv's site-packages path (e.g. .../python3.11/site-packages) into PYTHONPATH so the Python 3.11 backend can import its packages. When this PYTHONPATH leaks into terminal subprocesses running a different Python version (e.g. Python 3.13), 3.11 C extension modules appear on sys.path ahead of the correct 3.13 versions and crash with ImportError (PIL _imaging, cryptography, etc.). Replace the existing blunt pop of PYTHONPATH from _ACTIVE_VENV_MARKER_VARS with a surgical Hermes-venv-aware filter: - Parse each PYTHONPATH entry by path - Strip only paths under ~/.hermes/hermes-agent/venv/.../site-packages - Preserve the Hermes source root (needed for import hermes_cli) - Preserve all user-set PYTHONPATH entries The same filter is applied in all three env builders: - _make_run_env (foreground terminal commands) - _sanitize_subprocess_env (background/PTY spawns) - PTY env builder This preserves env_passthrough semantics and never silently discards the user's own PYTHONPATH configuration.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing the Desktop-to-terminal environment leak. Current main still injects the Desktop venv site-packages into the backend environment at apps/desktop/electron/main.ts:1496, and the local terminal inherits os.environ through tools/environments/local.py:801, so the bug class remains relevant.
Problems
tools/environments/local.py:887only detects"/site-packages". Desktop Windows buildsvenvRoot\\Lib\\site-packageswithpath.joinatapps/desktop/electron/main.ts:1860, so the proposed filter leaves the Windows entry intact.- The
VIRTUAL_ENVbranch at proposedtools/environments/local.py:901cannot run: every caller removes_ACTIVE_VENV_MARKER_VARSfirst (for example proposed lines 839-842). - The diff adds no regression tests for the three environment builders or Windows path handling.
Suggested changes
- Compare normalized path components against the actual Hermes venv site-packages location, rather than matching a Unix-only substring.
- Keep the existing unconditional marker stripping and remove the unreachable branch.
- Add POSIX/Windows and all-builder coverage while verifying source-root and user PYTHONPATH entries survive.
Automated hermes-sweeper review.
| if not entry: | ||
| continue | ||
| # Does this entry point at site-packages inside the Hermes venv? | ||
| if _hermes_venv and _hermes_venv in entry and "/site-packages" in entry: |
There was a problem hiding this comment.
This Unix-only "/site-packages" check misses the Desktop Windows entry: current apps/desktop/electron/main.ts:1860 builds venvRoot\\Lib\\site-packages with path.join. Normalize/compare path components against the Hermes venv instead.
| ) | ||
|
|
||
| # --- VIRTUAL_ENV: remove if pointing at the Hermes venv --- | ||
| ve = env.get("VIRTUAL_ENV") |
There was a problem hiding this comment.
VIRTUAL_ENV has already been removed by the _ACTIVE_VENV_MARKER_VARS loop before this helper is called in every proposed caller, so this branch is unreachable.
|
The predicate tests if _hermes_venv and _hermes_venv in entry and "/site-packages" in entry:but the Windows PYTHONPATH entry the gateway injects uses backslashes, so that substring never matches. Running the predicate verbatim against the real values on my box (Hermes 0.18.2, Windows 11): The parts = Path(entry).parts
if _hermes_venv and _is_relative_to(Path(entry), Path(_hermes_venv)) and "site-packages" in parts:Also worth noting Second repro data pointSame leak, different C extension — a Python 3.14 child instead of 3.13: The 3.14 interpreter finds Hermes' 3.11 numpy on PYTHONPATH ahead of its own venv's, and Re: #57470 as the companion fixOn this install PYTHONPATH has two independent sources, and env.Item("VIRTUAL_ENV") = "...\hermes-agent\venv"
env.Item("PYTHONPATH") = "...\hermes-agent;...\hermes-agent\venv\Lib\site-packages"
sh.Run "...\Python311\pythonw.exe -m hermes_cli.main gateway run", 0, FalseThat's deliberate — |
|
This PR addresses the defect later reported as #74817 (PYTHONPATH leaking from the gateway env into terminal-tool subprocesses, crashing cross-version compiled imports), and predates that issue by three weeks — but was not cross-referenced on it, so the issue timeline showed only the three later PRs (#74871, #74951, #78917). Posting here to create the link. A full tabulation of the cluster (approaches, dates, the additional cron script-job surface, and the ordering fact that answers the Windows-cron propagation concern for factory-side strips) is at #78917 (comment). Defect confirmed still live on Filed by an AI agent (Claude Fable 5) operating autonomously on @jeff-mettel's behalf. Code references were verified against |
|
The selective PYTHONPATH filtering approach you pioneered here has been merged via #88182, with your original commit and authorship preserved in the history (cherry-picked through #78917, which fixed the Windows path-matching and venv-detection review items on top of your work). Thank you @mmchuangyt-ai — you had the right shape first: surgical removal of Hermes-owned entries while preserving user paths, which is exactly what landed. Closing since the work is now on main. |
Problem
The Desktop Electron process injects the Hermes venv's site-packages path
(e.g.
.../python3.11/site-packages) into PYTHONPATH so the Python 3.11backend can import its packages. When this PYTHONPATH leaks into terminal
subprocesses running a different Python version (e.g. Python 3.13), 3.11
C extension modules (PIL
_imaging,cryptography, etc.) appear onsys.pathahead of the correct 3.13 versions and crash with ImportError.Solution
Replace the existing
ACTIVE_VENV_MARKER_VARSapproach (which onlycovered VIRTUAL_ENV/CONDA_PREFIX) with a new
_strip_mismatched_site_packagesfunction that surgically filters PYTHONPATH:
~/.hermes/hermes-agent/venv/.../site-packagesACTIVE_HERMES_ROOT)Applied to all three env builders in
tools/environments/local.py:_make_run_env,_sanitize_subprocess_env, and the PTY spawn builder.Why not fix at the Desktop Electron level?
The root cause is in
main.cjs:getVenvSitePackagesEntries()which addsthe venv's site-packages to PYTHONPATH — this is redundant because the
venv's own Python already knows its site-packages location. Fixing it
there requires rebuilding the Desktop app. The Python-side fix covers
all code paths regardless of how the environment was set up.
Verification
from PIL import Imageno longer crashes (was ABI conflict with 3.11_imaging)httpx,cryptographyload from correct 3.13 paths/home/user/my-lib) preservedVIRTUAL_ENVpointing at Hermes venv also stripped from subprocess env