fix(tools): preserve active venv on PATH across login-shell snapshots (#66642) - #66902
fix(tools): preserve active venv on PATH across login-shell snapshots (#66642)#66902Enough1122 wants to merge 2 commits into
Conversation
abfe123 to
d3eb524
Compare
|
The branch was force-pushed after an earlier triage bot comment ( Current diff is single-file and matches the PR description exactly:
This AI-assisted PR was drafted by Hermes Agent on behalf of @Enough1122. Happy to rebase / split / close if reviewers prefer a different shape. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for narrowing the force-pushed diff to the terminal change. The reported snapshot path is real enough to investigate, but this implementation cannot affect the stated repro.
Problems
LocalEnvironment._run_bash()calls_prepend_shell_init()only wheninit_filesis non-empty (tools/environments/local.py:1343-1346). The PR places its restoration in that helper, so the bare-container / empty-profile case described in the PR never executes it.- The proposed shell condition relies on
$VIRTUAL_ENV, but_make_run_env()removesVIRTUAL_ENVandCONDA_PREFIXbeforePopenreceives the environment (tools/environments/local.py:1172-1173; Popen at:1379-1389). This stripping is an intentional safety contract covered bytests/tools/test_local_env_blocklist.py:277-303and commitdbbf102b8.
Suggested changes
- Rework the login-snapshot path to inject a trusted, Python-derived interpreter bin directory after login initialization, independent of init files, without restoring the VIRTUAL_ENV marker.
- Add an end-to-end regression test for an empty init-file list and a login shell that resets PATH.
Automated hermes-sweeper review.
| # instead of the system one. Issue #66642. | ||
| prelude_parts.append( | ||
| 'if [ -n "$VIRTUAL_ENV" ] && [ -d "$VIRTUAL_ENV/bin" ]; then\n' | ||
| ' case ":$PATH:" in\n' |
There was a problem hiding this comment.
_make_run_env() strips VIRTUAL_ENV before this login-shell process is spawned (tools/environments/local.py:1172-1173), so this condition is false on the terminal path. The fix needs a trusted Python-derived bin path rather than the scrubbed marker.
|
Re-do of #66642. The original PR's shell prelude was dead code on the terminal path because of two gating conditions the implementation missed:
Rework to use a Python-derived anchor instead:
Adds
Local: 9/9 new tests pass; full hermes_state and local_env suites unchanged from baseline (no regressions). The 9 pre-existing Windows-only test failures in Branch: |
…NousResearch#66642) A bash -l login shell sources /etc/profile and friends, which on bare containers with empty profile files falls back to bash's compiled default PATH (/usr/local/bin:/usr/bin:/bin) and silently drops $VIRTUAL_ENV/bin. init_session runs a login shell and captures the resulting env via `export -p` into a session snapshot. Every subsequent foreground terminal command then replays that snapshot — so the venv-less PATH persists across the whole session, and `python` resolves to the system interpreter instead of the active venv interpreter. Steps-to-reproduce in NousResearch#66642: `command -v python` → `/usr/bin/python`, `sys.prefix` → `/usr`, while the agent process itself has the venv on PATH. Re-assert $VIRTUAL_ENV/bin on PATH after the profile-file prelude runs, before bash -l captures the snapshot. Uses the standard `case :$PATH:` prefix-match to avoid duplicating the entry if a profile already added it (bare /etc/profile on a real venv host sometimes does). Single-file change in tools/environments/local.py::_prepend_shell_init. No public API change. Fixes NousResearch#66642.
…L_ENV Re-do of NousResearch#66642. The original PR's shell prelude was dead code on the terminal path because of two gating conditions the implementation missed: 1. _prepend_shell_init() only runs when init_files is non-empty (tools/environments/local.py:1360-1363). On bare containers with no shell rc files the prelude never executes — the very case the bug report describes. 2. _make_run_env() strips VIRTUAL_ENV/CONDA_PREFIX from the subshell env before Popen (tools/environments/local.py:1172-1173) to prevent cross-project clobber (NousResearch#23473, dbbf102). The shell condition 'if [ -n "$VIRTUAL_ENV" ]' is therefore always false on the terminal path — the stripping is an intentional safety contract covered by tests/tools/test_local_env_blocklist.py :277-303. Rework to use a Python-derived anchor instead: - New _resolve_python_bin_dir() reads sys.executable's parent dir as the trusted venv bin. Cannot be polluted by caller env, since the running interpreter is by definition inside the Hermes venv. - New _prepend_python_bin_dir() prepends-if-missing to a PATH string, cross-platform via os.pathsep. First-occurrence wins so it composes safely with the existing _prepend_hermes_bin_dir and _prepend_git_bash_dirs. - Hook in _make_run_env() so the injection runs UNCONDITIONALLY regardless of init_files. Wrapped between _append_missing_sane_ path_entries and the Windows-only git_bash prepend so the venv outranks sane fallbacks but loses to platform coreutils. - Remove the broken VIRTUAL_ENV-based shell prelude from _prepend_shell_init. The reworked fix subsumes it. Adds TestPythonBinDirOnPath covering: - Resolution from sys.executable / unresolvable cases - prepend-if-missing + idempotency - end-to-end: PATH collapses to /usr/bin:/bin and the venv bin is still first (the original NousResearch#66642 repro) - works without VIRTUAL_ENV in the env (the stripped-marker case) - works with init_files=[] (the original bug's trigger) Closes NousResearch#66902 review feedback Fixes NousResearch#66642
a213b70 to
9374685
Compare
|
Review feedback addressed — |
|
Closing this PR as it has been labeled |
Summary
bash -llogin shell sources/etc/profileand friends. On bare containers with empty profile files it falls back to bash's compiled default PATH (/usr/local/bin:/usr/bin:/bin) and silently drops$VIRTUAL_ENV/bin.init_sessionruns a login shell and captures the resulting env viaexport -pinto a session snapshot. Every subsequent foreground terminal command then replays that snapshot — so the venv-less PATH persists across the whole session, andpythonresolves to/usr/bin/pythoninstead of the active venv interpreter. (sys.prefix→/usrrather than the venv root.)This matches the regression in #66642 (Hermes Agent v0.15.1, Debian 13 container, interpreter at
/opt/hermes/.venv).Fix
Re-assert
$VIRTUAL_ENV/binon PATH after the profile-file prelude runs, beforebash -lcaptures the snapshot:case :$PATH:prefix-match avoids duplicating the entry if a profile already added it. Single-file change intools/environments/local.py::_prepend_shell_init. No public API change.Test plan
Inside a venv-based install (Docker image with
/opt/hermes/.venv, empty/etc/profile.d):terminal_executewithcommand -v python; python -c "import sys; print(sys.prefix)".command -v python→/opt/hermes/.venv/bin/python(not/usr/bin/python).sys.prefix→/opt/hermes(not/usr)._make_run_env/ non-login path already worked (the agent's own process env is correct); this fix only touches thebash -lsnapshot path used byinit_session.Fixes #66642.