Repository navigation
fix(gateway,cron): keep the pre-PM venv off sys.path on PM-managed Windows installs - #123996
Merged
kshitijk4poor merged 8 commits intoSep 26, 2026
Conversation
…usResearch#122183) _windows_cron_python_invocation selected the dependency tree with selected_venv, which falls back to base_venv and answers the leftover pre-PM <root>/venv when no generation is recorded. That tree belongs to whichever interpreter created it, so on a PM-managed install the managed store Python 3.14 got a cp311 site-packages on PYTHONPATH and every cron script died with "No module named 'pydantic_core._pydantic_core'" -- the cron sibling of the gateway crash in NousResearch#122183, which janviernine flagged there and left out of scope. committed_venv never answers with the in-tree venv. With nothing committed this install provisioned no tree, so overlay none and let the child keep the interpreter it was handed rather than borrowing a foreign ABI. Every other selected_venv caller (tools/environments/local_pythonpath, hermes_cli/_early_recovery, pm/extras, pm/environments_adopt) already gates on runtime_facts_path().is_file(); this was the only unguarded one. (cherry picked from commit dd2b718)
…itted (NousResearch#122183) The no-generation arm returned the bare store Python with a repo-only overlay, which is worse than the main behavior it replaced: a store Python with nothing committed is refused by _require_own_dependencies with "hermes pm repair", and a cron script has no hermes_bootstrap to refuse cleanly, so it runs and dies on its first third-party import. It also stripped the overlay's site-packages entry, so _windows_cron_bootstrap_argv warned on every spawn. Fall through to the uv overlay instead: the handed venv keeps its own base interpreter and its own packages, which is what worked on main. The committed-generation arm is unchanged and still returns the store Python with that generation's site-packages. Also corrects the comment: site_packages is a pure path join on Windows (pm/environments.py:250-255) and never raises there; the raise is the pyvenv.cfg probe inside _recorded_venv:192, already covered here, and dependency_site() is outside this try. (NousResearch#123668 review) (cherry picked from commit b3760ef)
…#122183) platforms("windows") instead of a bare skipif: list_os_marked_tests.py gates the Windows lane on that marker, so the skipif version ran on no CI lane at all -- the antipattern root AGENTS.md calls out. Four tests collapse to the two invariants that actually matter: the committed generation wins, and no-generation/unreadable-record both fall through to the handed venv. The legacy non-managed test is gone -- it passes on main by design, and the fall-through test's positive control now covers that path inside the managed arm. (cherry picked from commit 30c649f)
…h#122183) The salvage drops the try/except wrapper around committed_venv (a corrupt facts.json raises as it did on main), so the "unreadable record falls through" arm no longer describes the code. Keep the one invariant that pins the bug: a committed generation, never the stale in-tree venv, is what the cron child gets. Stack budget is two invariant tests. Co-authored-by: Halldrix <halldrix@users.noreply.github.com>
…ateway (NousResearch#122183) _ensure_windows_gateway_venv_imports prepended VIRTUAL_ENV or <root>/venv unconditionally. On a PM install hermes_bootstrap has already activated the store Python onto the committed generation, so that late overlay put a foreign-ABI tree (cp311 pydantic_core under 3.14) first on sys.path and the gateway died on import. - committed generation present -> return: bootstrap already made the one activation decision; no second, late injection. - committed_venv errors propagate (a corrupt facts.json is an error, not permission to load the legacy venv). - nothing committed -> only overlay a candidate that is the running interpreter's own prefix, instead of parsing pyvenv.cfg versions (uv writes version_info, which a version-only parser misses). Co-authored-by: Robby Slamet <robbyslmt@users.noreply.github.com>
…talls (NousResearch#122183) The `Path(sys.prefix).resolve() != resolved_venv` guard skipped exactly the case _ensure_windows_gateway_venv_imports exists for (264ac72): a gateway restarted under uv's base pythonw.exe that still needs venv/Lib/site-packages (MCP SDK). When sys.prefix IS the venv its site-packages is already on sys.path, so the guard turned the function into a no-op on non-PM installs. It is not needed for NousResearch#122183: the committed_venv early return alone keeps a PM install off the leftover pre-PM venv (and a corrupt facts.json raising there is fail-closed, matching hermes_bootstrap). Mutation-checked with a fake-win32 harness: removing that early return turns test_committed_generation_blocks_the_legacy_venv_overlay red.
…earch#122183) The 19-line block narrated review history and still carried the base claim "rather than interpreting relocated pyvenv.cfg", which is now wrong: with no generation committed the code falls through to the handed venv's pyvenv.cfg. Four lines state the rule. Comment-only.
…nv is patched (NousResearch#122183) Neither kept test reaches a pyvenv.cfg read: the committed-generation path returns before the cron fall-through, and the gateway overlay never parses it. The `version=` arg, base/home dirs and the legacy cfg content were setup nothing consumed. The cron test's selected_venv patch looks inert on head but is what makes it red on base, so it now says so. Both tests still pass on head and fail with base prod files (fake-win32 harness).
kshitijk4poor
enabled auto-merge (rebase)
September 26, 2026 14:14
This was referenced Sep 26, 2026
alt-glitch
added
type/bug
This was referenced Sep 26, 2026
alt-glitch
added
os/windows
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.
On a PM-managed Windows install, the gateway and cron scripts no longer put the stale pre-PM in-tree venv on
sys.path. They use the committed dependency generation, sohosted_room_workerstops crashing withNo module named 'pydantic_core._pydantic_core'. Fixes #122183. This salvages #123149 by @robbyslmt (gateway side) and #123668 by @Halldrix (cron side).Why
On a PM install,
hermes_bootstraphas already activated the process onto the committed generation. Two late paths then added the legacyvenv/Lib/site-packageson top of it, and that venv was built for a different Python ABI:_ensure_windows_gateway_venv_imports(gateway/run.py) always prepended the in-tree venv._windows_cron_python_invocation(cron/scheduler_script.py) overlaidselected_venv(repo). When nothing is committed, that falls back tobase_venv, which is the same in-tree venv.Changes
pm.environments.committed_venv(root)returns a generation, return before any overlay. Non-PM installs, where nothing is committed, keep the existing overlay unchanged. That overlay exists for the legacy base-pythonw.exelaunch (264ac72).committed_venv. When nothing is committed, fall through to the venv the job was handed, so the child keeps its own interpreter and packages.facts.jsonraises, as it does inhermes_bootstrap. It is not turned into permission to use the legacy venv. That is why this PR does not carry fix(gateway): never inject a version-mismatched venv into the Windows gateway; prefer the committed PM environment #123149'sexcept Exceptionor fix(cron): read the committed generation for Windows cron scripts #123668's try/except wrapper (43751c7). Both reviewers flagged that failure direction.pyvenv.cfgversion parser. The committed-generation check makes one unnecessary, so the uvversion_infogap does not arise.Validation
Tests:
tests/gateway/test_windows_venv_import_guard.pyandtests/cron/test_cron_windows_venv_abi.py. Both carrypytest.mark.platforms("windows"), soscripts/ci/list_os_marked_tests.py windowsselects both for the native Windows lanes.gateway/run.pyandcron/scheduler_script.py.uv syncfor the 3.14 pin, andTestRunJob*fails the same way on main.Not in this PR
hermes_cli/gateway.py_pm_runtime_venv_dirstill usesselected_venvand has no callers. It can be switched or deleted in a follow-up.get_python_pathand the gateway re-exec for [Bug] Windows gateway boots under newer system Python (PATH race) → pydantic_core ABI mismatch crash-loop even after activation fix #123185, which is a separate launch path. See the comment there.Credit
version_infogap, the fail-closed direction, the no-generation fall-through and the Windows CI selector each shaped this version.