fix: tighten subprocess Python environment isolation in #78917 - #1
Closed
Starfie1d1272 wants to merge 3 commits into
Closed
Starfie1d1272 wants to merge 3 commits into
Starfie1d1272 wants to merge 3 commits into
Conversation
…ow-up) Remove the cross-version heuristic from _strip_mismatched_site_packages: the subprocess env builder cannot know which Python version a child will run, so judging user PYTHONPATH entries against the backend interpreter's version deletes legitimate paths meant for a different child Python (e.g. /custom/lib/python3.13/site-packages while Hermes runs 3.11). Also fix over-strip: entries merely containing a pythonX.Y path component (e.g. /opt/tools/python3.13/bin) were stripped even though they are not site-packages. Hermes-owned entries (repo root, own venv site-packages) are now identified by path ownership, not by version. Regression tests cover both cases; user paths with any pythonX.Y component are preserved.
The gateway runs inside its own venv; if its PYTHONHOME leaks into subprocesses (terminal commands, cron no_agent scripts, TTS providers), any child interpreter redirects its stdlib search to the Hermes venv and crashes with version-mismatch errors before importing anything. PYTHONHOME is now part of _ACTIVE_VENV_MARKER_VARS so all env builders (_make_run_env, _sanitize_subprocess_env, hermes_subprocess_env, and build_subprocess_env used by cron) drop it, consistent with Hermes' existing PYTHONHOME handling in managed_uv.py and sqlite_runtime.py. execute_code already scrubbed it via _SAFE_ENV_PREFIXES. Tests cover all four builders plus the marker constant.
Adversarial review of the previous two commits (and NousResearch#78917 itself) found three ownership-boundary issues; this commit addresses them: 1. Repo direct-child over-strip (Finding A) No launcher injects <repo>/tools or another direct child as an independent PYTHONPATH entry - audited all four producers (Electron electron-main.mjs, gateway/run.py::_ensure_windows_gateway_venv_imports, cron/scheduler.py::_windows_cron_python_invocation, tui_gateway/host_supervisor.py). The depth<=1 rule deleted user paths that merely live under the repo directory; only the EXACT repo root is now stripped. 2. Windows junction/symlink alias (Finding B) The gateway launcher renders Hermes-owned paths under the configured HERMES_HOME spelling (gateway_windows.py::_preserve_hermes_home_path), which may be a junction to another drive, so it differs lexically from the resolved repo root. _hermes_repo_root_aliases now carries both the resolved and unresolved spellings; both are recognized as Hermes-owned. 3. Stale abstraction rename (Phase 4) _strip_mismatched_site_packages -> _strip_hermes_owned_pythonpath: the cross-version heuristic is gone, so the old name misdescribes the behavior (ownership-based, not version-based). Tests: direct-child now preserved; junction alias stripped (lexical pair monkeypatched); Windows-only real-semantics test added (POSIX test remains a safety test); mixed-ordering, duplicate-Hermes, and no-scrub PYTHONHOME contract tests added. Full file: 52 passed / 16 failed (identical failure set to base, all isolation-venv environment issues).
Author
|
The follow-up has now been consolidated into direct upstream PR NousResearch#82581 on current main. All commits from this PR and NousResearch#78917 were retained with their original Git authorship, with additional Windows runtime ownership hardening and paired pristine-base validation. I am leaving this PR open; its superseded/close status remains for the owners to decide. |
Yiipu
pushed a commit
that referenced
this pull request
Sep 2, 2026
… read Addresses teknium1's review (NousResearch#64195) finding #1: the previous PR placed the migration inside the connection IIFE, AFTER `resolveRemoteBackend(primaryProfileKey())`. When the preference file was missing, `primaryProfileKey()` resolved to 'default' and the remote branch returned immediately without ever reaching the migration. Remote- mode users got no migration at all. Move the call site to the top of `startHermes()`, before the connection IIFE that reads `primaryProfileKey()`. Both remote and local branches now flow through this path before any profile-dependent resolution, so the migration runs on first boot regardless of mode. The inlined implementation is replaced with a thin wrapper that builds a `MigrationDeps` bag and delegates to `migrateActiveProfileIfMissing` from `profile-migration.ts`. No production behavior change beyond the call- site move. Tests added in a separate commit.
Yiipu
pushed a commit
that referenced
this pull request
Sep 2, 2026
…reate Routing the branch create to the parent's owning connection was only half the job. The child then landed in the sidebar as a row that lied about who owned it, so the chat pane spun forever on "draft: branch #1" and never hydrated — the create was right, the row was wrong. upsertOptimisticSession stamps the row's profile from $activeGatewayProfile and omits connection_id entirely when no owner is passed (utils.ts:1318-1342), and it also skips setSessionOwnerHint. The branch call site passed no owner, so the child got NEITHER a row tag NOR a hint. resumeSession's owner ladder starts at `capturedOwner || getSessionOwnerHint(storedSessionId)` and forkBranch calls it without a capturedOwner, so the missing hint alone was enough to send the resume to whichever backend happened to be active. Pass the parent's route as the owner argument, restoring both mechanisms. The two sibling routed creates in this file already did exactly this. The tile path had the same defect one rung further out. A branch of a session that is not the open chat opens a tile instead of resuming, and SessionTileChrome resolved its owner from the tile route alone. openSessionTile is called for a branch child with no workspaceScope, and session-states.ts only persists a tile ownerRoute in bots mode, so that tile had no owner at all and its model + composer RPCs fell back to the ambient socket. Use the same tile-route-then-row ladder its sibling in session-tile-actions.ts already uses, resolved per render so it cannot go stale against the tile store, the recents/cron/messaging rows, or the hint map, with only the resulting identity memoised on primitives. An untagged parent row still reproduces the previous ambient behaviour exactly, so single-connection users are unaffected. Verified end to end against two real gateways: a session owned by a remote connection, branched through the actual sidebar context menu in a running dev app. The remote gateway served the create (ws closed ... messages=11 detached_sessions=1) and the resulting row polled stable at connection_id = the remote for the full 8s window. Before the fix the same gesture produced a row with no connection_id.
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
Follow-up to NousResearch#78917, based on independent local verification of that PR (see my Tested-by comment on NousResearch#78917, comment
5231755967). This does not re-implement the original PR — it fixes issues found in its_strip_mismatched_site_packages()implementation through adversarial review.Changes
1. Remove the cross-version heuristic (fixes over-strip + semantic problem)
The original Check 1 stripped any PYTHONPATH entry whose path contains a
python{X.Y}component when{X.Y}differs from the Hermes backend's interpreter version — without requiring the entry to be site-packages. Reproduced on macOS:The subprocess env builder cannot know which Python version a child will ultimately run, so judging user paths against the backend's interpreter version is incorrect. The cross-version check is removed entirely; Hermes-owned entries are now identified by path ownership, not by version.
2. Repo-root stripping narrowed to the exact root (over-strip fix)
Every real launcher producer was audited — Electron
electron-main.mjs,gateway/run.py::_ensure_windows_gateway_venv_imports,cron/scheduler.py::_windows_cron_python_invocation,tui_gateway/host_supervisor.py— and none injects<repo>/toolsor another direct child as an independent PYTHONPATH entry. The previousdepth <= 1rule deleted user paths that merely live under the repo directory. Only the exact repo root is now stripped; direct children and deeper user paths are preserved.3. Windows junction/symlink alias recognition
The Windows gateway launcher deliberately renders Hermes-owned paths under the configured
HERMES_HOMEspelling (gateway_windows.py::_preserve_hermes_home_path), which may be a junction/symlink to another drive. That spelling differs lexically fromPath(__file__).resolve(), so the ownership check previously could not recognize Hermes' own injected paths._hermes_repo_root_aliasesnow carries both the resolved and unresolved (HERMES_HOME) spellings; both are recognized as Hermes-owned.4. Sanitize inherited PYTHONHOME (completes NousResearch#75018)
NousResearch#75018 explicitly names
PYTHONHOMEas leaking intono_agentcron children (alongside PYTHONPATH and VIRTUAL_ENV). If the Hermes runtime inherits or carriesPYTHONHOMEin its process environment, it redirects the stdlib search of any child interpreter to the Hermes venv and crashes before a script even imports a package.PYTHONHOMEis now part of_ACTIVE_VENV_MARKER_VARS, so all default/sanitized child-environment paths drop it:_make_run_env(foreground terminal)_sanitize_subprocess_env(background/PTY + cronno_agentviabuild_subprocess_env)hermes_subprocess_env(non-terminal spawns: TTS providers, browser)This is consistent with Hermes' own child-process handling (
managed_uv.py,sqlite_runtime.pyalready remove it) and withexecute_code, which already scrubbed it via_SAFE_ENV_PREFIXES. The documented byte-for-byte escape hatchbuild_subprocess_env(scrub_secrets=False)is unchanged — it preserves everything, including PYTHONHOME, by contract (pinned by test).5. Rename stale abstraction
_strip_mismatched_site_packages→_strip_hermes_owned_pythonpath: the cross-version heuristic is gone, so the old name misdescribed the behavior. All call sites, tests, and docstrings updated; no old symbol remains.Known residual case
Nix
extraPythonPackagesare appended toPYTHONPATHby the Hermes Nix wrapper (nix/hermes-agent.nix,--suffix PYTHONPATH) outside the normal repo-root / venv-site-packages ownership boundary. This follow-up deliberately does not guess ownership from/nix/storepath shape: Nix users' legitimate PYTHONPATH entries also live under/nix/store, and there is no provenance data to distinguish Hermes-injected store paths from user-set ones. A provenance-aware producer-side solution (e.g. an explicit Hermes-internal marker) would be safer and is left to a future discussion.Windows test coverage — what it actually proves
test_windows_backslash_paths(POSIX CI): a safety test — Windows-looking user strings are never destroyed by the ownership filter on a POSIX host. It does NOT claim Windows stripping works.test_windows_hermes_owned_paths_stripped(@pytest.mark.skipif(sys.platform != "win32")): the real Windows behavior test — Hermes venv site-packages written with backslashes is stripped, user Windows paths preserved. Runs only on Windows CI.test_repo_root_junction_alias_stripped: the alias logic is tested on any host via a monkeypatched lexical pair (resolved vs junction spelling).Rationale
The original PR's selective-filter direction is correct and strictly better than blanket-strip alternatives (NousResearch#74951/NousResearch#74871). The problems were only in what counts as "dangerous": the backend's interpreter version is not a proxy for the child's,
pythonX.Yalone over-strips, and repo-root matching was broader than any real producer.Tests
Ran on macOS arm64 (Hermes backend Python 3.11.15) against this branch:
The 16 failures are pre-existing on the base branch (
yiipu/fix/pythonpath-selective-strip) — identical node IDs (verified by diff), all isolation-venv environment issues (module 'tools' has no attribute 'terminal_tool'in pkgutil traversal). Base run: 42 passed / 16 failed. Net: +10 passing tests, 0 regressions.Integration validation: the three-commit chain (this branch) cherry-picks cleanly onto current upstream
main(35e562e) — no conflicts, no duplicate env handling, same 52 passed / 16 failed there.Live E2E (this machine):
Relationship
5231755967