Repository navigation
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.
… tree (NousResearch#122183) Three invariants over _windows_cron_python_invocation: a managed install with no committed generation never overlays the leftover pre-PM venv, a committed generation is the tree that gets overlaid, and a non-managed install keeps its uv base-interpreter overlay unchanged.
|
Beautifully scoped — this is exactly the sibling I flagged on #122183, and the Green/Red/Control test story (2 fail with the fix reverted, legacy stays green) is the right evidence. One residual, same failure-direction class as the two rounds on #122330:
The fail-closed shape matches this PR's own try:
environment = committed_venv(repo)
except Exception:
environment = NoneNon-blocking — the reported defect is fixed as is. Flagging it because the comment asserts the install provisioned no tree, and on the raise path the code never gets to assert anything. — reviewed with KBG Bot MKIV, courtesy of the KBGville Discord community. |
…dable (NousResearch#122183) committed_venv raises - it does not answer None - on an unparseable or out-of-tree facts.json (pm/environments.py:179-193), and site_packages raises when the committed tree vanished. _script_argv calls this unguarded, so a corrupt record turned every cron script into a spawn-time traceback with nothing in the log pointing at the record. Catch it and take the same shape as the None arm: repo-only overlay, the child keeps the interpreter it was handed, WARNING logged. Degraded rather than dead. The pre-existing selected_venv call could raise the same way, so this is not a regression introduced here - but the comment asserted an invariant that the raise path never reached.
…Research#122183) A committed_venv that raises must still hand the store interpreter to the child and overlay repo-only, never the stale in-tree tree.
…NousResearch#122183) The overlay carries Path(__file__).parents[1] - the real checkout - so the tmp_path repo built for the stale venv never appears in it. Caught by the W11 run: the assertion failed against correct code.
|
Good catch — verified at the source and taken.
One correction to the framing, though: this is not a regression introduced here. The pre-existing The guard takes the same shape as the try:
environment = committed_venv(repo)
except Exception:
logger.warning(
"Windows cron script: could not resolve the committed dependency environment "
"for %s; running without a dependency overlay", repo, exc_info=True)
environment = NoneRe-verified on a real Windows 11 host, now with both sabotages doing real work (the first harness re-added the worktree between phases and silently undid them, so the earlier RED run was vacuous):
RED2 is the one that matters for this change: it removes only the guard, keeps the
|
|
@Halldrix — correction accepted, pre-existing gap yours now closes rather than a regression it introduced. The RED2 isolation (guard-only revert, single failure) is exactly the evidence that matters. Thanks for carrying the sibling home. |
jonpol01
left a comment
There was a problem hiding this comment.
The premise holds on main d0288be. With no generation committed, cron/scheduler_script.py::_windows_cron_python_invocation overlays selected_venv(repo) on the store Python. selected_venv falls back to base_venv, which is the in-tree venv. Switching to committed_venv is the right resolver and matches ethernet8023's c821ecd (pm/environments.py::committed_venv, activate_dependencies). The remaining problem is the arm with nothing committed. The comment says the child "keeps the interpreter it was handed", but the code still hands the child the bare store Python. That breaks scripts that work on main (finding 1). I checked with a probe that imports the real module under a temp HERMES_HOME (committed_venv really returns None, selected_venv really returns <repo>/.venv), stubs only resolve_store_python, and sets sys.platform = "win32" in the probe process only. Main at d0288be returns (<store>/python.exe, PYTHONPATH=<repo>:<repo>/.venv/...site-packages), which reproduces the bug. Head b86dffa returns (<store>/python.exe, PYTHONPATH=<repo>). The tests: scripts/run_tests.sh -j 2 tests/cron/test_cron_windows_venv_abi.py tests/cron/test_cron_script.py on head gives 31 passed, 5 skipped, and 4 of the skips are the new file on darwin. I read the new tests but could not run them on Windows here. Preflight: #123479 edits the same managed block (it moves these imports, renames the function to _cron_python_invocation and keeps selected_venv), so the two conflict textually, and merging #123479 afterwards would bring back the in-tree fallback. #123548 (the POSIX early-return in the same function) and #122742 (the _script_argv call site) touch neighbouring hunks.
- [blocking] The no-generation arm discards the handed interpreter and gives the child a bare store Python —
cron/scheduler_script.py:164-167. Risk/repro: in the probe above, the handed interpreter is<tmp>/ownvenv/Scripts/python.exe(a uv venv withhome=<tmp>/uvbase). Head returnsinterpreter=<tmp>/store/python.exe,PYTHONPATH=<repo>, and_script_argvthen logsWARNING cron.scheduler: Windows cron script: venv site-packages Lib/site-packages not found; running without .pth processingon every spawn. The warning comes from_windows_cron_bootstrap_argv(head:277-279): the repo-only overlay has nosite-packagesentry, so it falls back to a relativeLib/site-packages. Why it matters:hermes_bootstrap.py:552-557→pm/environments.py:280::_require_own_dependenciesmakes a bare store Python exit withhermes pm repairwhen nothing is committed. So any Hermes process running cron in this state runs on a non-store interpreter, which for a uv install is a venv carrying its own packages ("a venv/Nix interpreter keeps its own packages", c821ecd). The child gets none of them. A cron script has nohermes_bootstrapto refuse cleanly (compare the comment atpm/environments.py:359), so it runs and fails on its first third-party import. Pure-Python packages from the in-tree venv loaded on main (the PR body says so too); on head they fail. Suggested fix: return from the managed branch only when a generation is committed, and otherwise fall through to the existing uv overlay below, which keeps the handed venv's own base interpreter and its own site-packages. With that change the same probe returnsinterpreter=<tmp>/uvbase/python.exe,VIRTUAL_ENV=<tmp>/ownvenv,PYTHONPATH=<repo>+<tmp>/ownvenv/Lib/site-packages, and no warning. A handed bare store Python falls through to(python_exe, {}), a plain spawn.test_corrupt_record_degrades_instead_of_aborting_the_spawnwould then assert on the handed interpreter instead ofstr(store). - [nonblocking] The new test file runs on no CI lane —
tests/cron/test_cron_windows_venv_abi.py:23-26(modulepytestmark). Risk/repro:python scripts/ci/list_os_marked_tests.py windows <tree containing only this file>printsNo test files gate on 'windows'and exits 1..github/workflows/tests-os.yml:121builds the Windows lane's file list with exactly that script, and the Linux/macOS lanes skip the file. So the RED/GREEN evidence exists only on the author's host.tests/_fixtures/platform_gating.pyand rootAGENTS.md("Use the marker, never a bareskipif") cover this case, andtests/cron/test_cron_script.py:166already uses@pytest.mark.platforms("windows")for this function's Windows tests. Suggested fix:pytestmark = pytest.mark.platforms("windows"). Those tests could also move intotests/cron/test_cron_script.pynext to the existing ones.
Minor
- The comment at
cron/scheduler_script.py:152-153sayssite_packagesraises when the committed tree has vanished. On Windows it is a pure path join (pm/environments.py:253). The vanished-tree check is thepyvenv.cfgprobe inside_recorded_venv(pm/environments.py:192), which thetryalready covers, anddependency_site(...)sits outside thetry. Please confirm the comment's intent and drop that clause. - Four tests exceed the repo's two-invariant-test bar.
test_non_managed_install_keeps_its_own_venv_overlaypasses on main by design (it is a control), and with finding 1 the no-generation and corrupt-record cases become one fall-through contract. - The body says every other
selected_venvcaller is guarded byruntime_facts_path(root).is_file().pm/environments_adopt.py:29,hermes_cli/runtime_state.py:184andpm/install.py:523are not guarded that way. They only use the result for identity comparisons and never compose a child'sPYTHONPATHfrom it, so the conclusion stands but the stated reason is wrong.
Related — #123479 (same block, see preflight). #122330 and #123149 make the same committed_venv switch in gateway/run.py::_ensure_windows_gateway_venv_imports. #123525 adds a POSIX sibling that also resolves through committed_venv.
| pythonpath = [str(repo)] | ||
| if environment is not None: | ||
| pythonpath.append(str(dependency_site(environment))) | ||
| return str(managed_python), {"PYTHONPATH": os.pathsep.join(pythonpath)} |
There was a problem hiding this comment.
[blocking] The no-generation arm discards the handed interpreter and gives the child a bare store Python.
Risk/repro: the probe imports the real module under a temp HERMES_HOME, so committed_venv returns None. It stubs only resolve_store_python and sets sys.platform = "win32" in the probe process only. The handed interpreter is a uv venv shim, <tmp>/ownvenv/Scripts/python.exe. On this head the function returns interpreter=<tmp>/store/python.exe, PYTHONPATH=<repo>. _script_argv then logs venv site-packages Lib/site-packages not found; running without .pth processing on every spawn, because the repo-only overlay has no site-packages entry for _windows_cron_bootstrap_argv to find. When nothing is committed, _require_own_dependencies (pm/environments.py:280, called from hermes_bootstrap.py:554) refuses a bare store Python at boot. So the process running cron here is a non-store interpreter (for a uv install, a venv that carries its own packages). The child gets none of them, and pure-Python packages that loaded on main now fail.
Suggested fix: return from the managed branch only when a generation is committed, and otherwise fall through to the uv overlay below. The comment's "child keeps the interpreter it was handed" then becomes true: the same probe returns <tmp>/uvbase/python.exe with <tmp>/ownvenv/Lib/site-packages, and there is no warning.
| pythonpath = [str(repo)] | |
| if environment is not None: | |
| pythonpath.append(str(dependency_site(environment))) | |
| return str(managed_python), {"PYTHONPATH": os.pathsep.join(pythonpath)} | |
| if environment is not None: | |
| return str(managed_python), { | |
| "PYTHONPATH": os.pathsep.join([str(repo), str(dependency_site(environment))])} |
| pytestmark = pytest.mark.skipif( | ||
| sys.platform != "win32", | ||
| reason="_windows_cron_python_invocation is the Windows cron spawn path (#122183/#123650)", | ||
| ) |
There was a problem hiding this comment.
[nonblocking] This file runs on no CI lane. Risk/repro: python scripts/ci/list_os_marked_tests.py windows <tree containing only this file> prints No test files gate on 'windows' and exits 1. .github/workflows/tests-os.yml:121 builds the Windows lane's file list with that script, so the bare skipif is skipped on Linux/macOS and never imported on Windows. tests/cron/test_cron_script.py:166 gates this function's Windows tests with the repo's marker.
| pytestmark = pytest.mark.skipif( | |
| sys.platform != "win32", | |
| reason="_windows_cron_python_invocation is the Windows cron spawn path (#122183/#123650)", | |
| ) | |
| pytestmark = pytest.mark.platforms("windows") |
…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)
…#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.
|
Fix 1 (blocking) — taken. The comment said the child "keeps the interpreter it was handed" and the code returned You're right that this is worse than main, and the No warning, Fix 2 (CI lane) — taken, and you were right that it was the marker. Minor 1 — confirmed, comment corrected. Minor 3 — confirmed, and your reason is the better one. Restated: this function was the only Minor 2 — taken, 4 tests → 2. Invariant 1 (committed generation wins), invariant 2 (no-generation and unreadable-record both land on the handed venv, checked as a table over both arms). The legacy non-managed test is gone — it passed on main by design, and the fall-through test's positive control now covers that path inside the managed arm. Not addressed: #123479. You're right that it edits the same block and keeps W11 receipts on |
NousResearch#122183) Late adversarial gate on NousResearch#123668. Two findings, both mine: 1. BLOCKER. tools/environments/local_pythonpath.py:81 read selected_venv behind runtime_facts_path().is_file() with no try. That gate suppresses the FALLBACK, never the RAISE: _recorded_venv raises at pm/environments.py :180,185,189,193, and this runs inside _finalize_child_env -- the guard every spawn surface shares -- so a malformed facts.json crashed the same way the cron path did. The body claimed this caller "never hands it to a child"; it feeds the strip list of every child env. Same defect, same fix. 2. CONDITION. The bare except Exception downgraded payload_venv's "payload environment escapes its root" (pm/environments.py:92) -- a fail-closed security check -- to a warning. Narrowed to the repo's PM-read idiom (OSError, RuntimeError, ValueError; environments_adopt.py:30) and the escape check re-raised, on both sites. Also writes the fall-through invariant the comment previously assumed: a cron script gets sys.executable, and on a managed install with nothing committed a booted process is by definition a venv interpreter (_require_own_dependencies) carrying its own packages.
…uard (NousResearch#122183) The fall-through had a second arm nobody asserted: a venv with no uv key skips the overlay and keeps its own python.exe. That is the invariant that keeps the arm from returning the bare store Python. Two tests for local_pythonpath: a malformed record degrades to no runtime venv, and payload_venv's escape check still propagates. The second test caught the narrow-catch fix being applied to cron but not to its sibling.
|
Thanks @Halldrix. Your three cron commits are cherry-picked into #123996 with authorship kept (merged with rebase). One difference from your head: the try/except wrapper from 43751c7 is not carried. A corrupt or unreadable
Closing in favour of #123996. I'll post the merge SHA here when it lands. |
…itted (#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. (#123668 review) (cherry picked from commit b3760ef)
What does this PR do?
Windows cron scripts could be spawned onto a venv built for a different interpreter than the one running them, killing every script with
No module named 'pydantic_core._pydantic_core'.cron/scheduler_script.py::_windows_cron_python_invocationanswers "which interpreter and which site-packages?" for a cron script child. On a PM-managed install it read the dependency tree throughpm.environments.selected_venv, which falls back tobase_venvand answers the leftover pre-PM<root>/venvwhen no generation is recorded. That tree belongs to whichever interpreter created it — typically CPython 3.11 while the managed store Python is 3.14 — so overlaying it onPYTHONPATHputs a cp311pydantic_corein front of the running interpreter. Pure-Python imports succeed; the first compiled extension does not:committed_venvis the resolver that never answers with the in-tree venv. With a generation committed, the child gets that generation's site-packages. With none — or a record too corrupt to read — the call falls through to the uv overlay below it, so the handed venv keeps its own interpreter and its own packages, exactly as on main.This is the cron sibling of the gateway crash in #122183 (and the family reported again in #123650):
gateway/run.py::_ensure_windows_gateway_venv_importsmade the same mistake with the same resolver, and no PR covers the cron side of it.Related Issue
Fixes the cron side of #122183. The gateway half is tracked by #122330, #122333, #123149, #122739 and #123206; this PR deliberately does not touch
gateway/run.pyor overlap any of them.Type of Change
Changes Made
cron/scheduler_script.py—_windows_cron_python_invocation: resolve the managed install's dependency tree withcommitted_venvinstead ofselected_venv. With a generation committed, return the store Python with that generation's site-packages. With none, or an unreadable record (which raises rather than answeringNone), fall through to the uv overlay. The PM read is wrapped in the repo's idiom (except (OSError, RuntimeError, ValueError)—environments_adopt.py:30,install.py:524), not bareException, andpayload_venv's fail-closed"payload environment escapes its root"(pm/environments.py:92) is re-raised rather than downgraded to a warning.tools/environments/local_pythonpath.py—_validated_runtime_venv: the same wrap on the sibling read. Itsruntime_facts_path().is_file()gate suppressed the fallback, never the raise, so a malformedfacts.jsonstill raised inside_finalize_child_env— the guard path every spawn surface shares.tests/cron/test_cron_windows_venv_abi.py(new, 3 tests) — gatedpytest.mark.platforms("windows")so the Windows CI lane actually imports the file. The committed generation wins over the in-tree venv; no-generation and unreadable-record both land on the handed venv; and the fall-through's stdlib arm (a venv with nouvkey) keeps its ownpython.exe.tests/tools/test_local_env_blocklist.py(+2 tests) — a malformed PM record degrades to no runtime venv instead of raising;payload_venv's escape check still propagates.How to Test
Verified on a real Windows 11 host — the change is Windows-only (
_windows_cron_python_invocationearly-returns elsewhere), so a Linux run proves only that the file is correctly gated.f29885969c), the 3 new + 2 added tests:3 passedfor the new cron file,2 passedfor the blocklist additions.local_pythonpath.pyback at30c649f16b): the malformed-record test fails with the rawRuntimeErrorfrom_recorded_venv—1 failed."escapes its root" → raiseline deleted): the escape-propagation test fails alone —1 failed. This is the isolation that shows the narrow catch is load-bearing, not incidental.tests/cron/test_cron_script.py31 passed / 1 skipped;tests/tools/test_local_env_blocklist.py74 passed, 1 failed — that one failure,test_runtime_provenance_is_independent_of_aliases_and_virtual_env[True], reproduces identically on pristinemainin a separate worktree, so it is pre-existing and not from this diff.python scripts/ci/list_os_marked_tests.py windows .liststests/cron/test_cron_windows_venv_abi.py(it did not with the originalskipifgate).No
WARNING ... venv site-packages not foundline, and the bootstrap processes.pthfiles. On the pre-fix head the same probe returned the bare store Python, a repo-onlyPYTHONPATH, and warned on every spawn.7. Host left clean — worktree removed, live repo back on
mainwith an emptygit status.Checklist
Code
fix(scope):,feat(scope):, etc.)main)Documentation & Housekeeping
cli-config.yaml.exampleif I added/changed config keys — or N/A (no config keys)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/Alocal_pythonpathis cross-platform, so the catch uses the repo's shared PM-read idiomNotes for reviewers
Why this is not a sixth PR on
_ensure_windows_gateway_venv_imports. That function already has five open PRs (#122330, #122333, #123149, #122739, #123206) and its canonical issue #122183 carries ~10 independent reproductions. This one covers a different file that none of them touch.Overlap with #123479. That PR also edits
cron/scheduler_script.py, but a different defect: its POSIX branch has no dependency environment at all. Its hunk is thesys.platform != "win32"early-return; this one is the managed-install block below it. Neither diff mentions the other's symbols, so they should merge independently — but note #123479 keepsselected_venv, so merging it after this one reintroduces the in-tree fallback.The class, and where it actually ends.
_windows_cron_python_invocationandlocal_pythonpath._validated_runtime_venvwere the two callers ofselected_venvthat fed a child's environment. Both now read through a wrappedcommitted_venv/ degrade-on-unreadable-record. The remaining callers —hermes_cli/_early_recovery.py:527,pm/extras.py:226,pm/environments_adopt.py:29,hermes_cli/runtime_state.py:184,pm/install.py:523— use the result for identity comparison or guarded lookup and never hand it to a child. (An earlier draft justified that with "every other caller is guarded byruntime_facts_path().is_file()"; that was wrong as stated for three of them. The conclusion holds for the better reason: none of them composes a child env from it.)hermes_cli/gateway.py::_pm_runtime_venv_diralso readsselected_venv, but it has no callers — dead code, left alone deliberately.Screenshots / Logs
W11 transcript (head
f29885969c):