Skip to content

fix(cron): pin the runtime dependency path for POSIX .py cron scripts - #122742

Closed
nhod wants to merge 1 commit into
NousResearch:mainfrom
nhod:fix/cron-posix-script-deps
Closed

nhod wants to merge 1 commit into
NousResearch:mainfrom
nhod:fix/cron-posix-script-deps

Conversation

@nhod

@nhod nhod commented Sep 25, 2026

Copy link
Copy Markdown

What does this PR do?

POSIX .py cron scripts are launched as [sys.executable, path], and on a PM-managed install that interpreter is the store Python (its own site-packages holds only pip) while build_subprocess_env has just stripped both Hermes-owned PYTHONPATH entries — the repo root and the activated dependency environment. The child therefore has no importable path to the runtime's third-party dependencies, and a script's first non-stdlib import fails before it does any real work.

This adds the POSIX twin of the existing Windows overlay in the same function: keep the interpreter (it is already the runtime's — only the import path was missing) and overlay [repo root, activated dependency site-packages].

The dependency entry is derived from this process's own sys.path, where pm.environments.activate_dependencies publishes it (dependencies are exposed by path; sys.prefix is deliberately unchanged). That means no HERMES_HOME filesystem reads on the spawn path — a process running under an overridden HERMES_HOME never touches the real home — and it is version-guarded by the venv layout (<venv>/lib/pythonX.Y/site-packages) so a generation built for another interpreter is never pinned (the ABI concern from #122395). When the interpreter owns its dependencies (wheel/pipx install, dev venv) nothing is pinned: it already hands them to the child.

Related Issue

Refs #122222, #122529 — same class, different spawn site.

Those reports (and the in-flight PRs #122238 / #122290 to cron/scheduler_worker_env.py) cover the restart-safe external worker ([sys.executable, "-m", "cron.scheduler"]). This PR covers the script runner used by no_agent jobs, which is a separate Popen site in cron/scheduler_script.py and needs its own overlay even after the worker seam is fixed: the worker's pinned entries are Hermes-owned, so the sanitizer strips them again for the script child.

Observed live on macOS arm64 (Hermes main @ 59004a6235, self-managed install, store Python 3.14.7): the morning after the runtime moved off the in-tree venv, two jobs died in the same tick —

  • a Honcho health watchdog: cannot load honcho oauth plugin …: No module named 'ruamel'
  • the daily systems check: ModuleNotFoundError: No module named 'yaml'

(the second also because PyYAML is not a runtime dependency at all — Hermes parses YAML with ruamel via hermes_yaml). Chat/voice were unaffected; only scheduled script jobs died.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • cron/scheduler_script.py
    • _script_argv(): the non-Windows branch now picks up _posix_cron_python_invocation() and returns its env_overlay instead of an empty one.
    • _posix_cron_python_invocation(): new — POSIX twin of _windows_cron_python_invocation(), overlaying PYTHONPATH on the sanitized child env.
    • _activated_dependency_site_packages(): new — resolves the dependency site-packages this process actually runs on from sys.path, skipping the interpreter's own purelib and non-matching interpreter versions.
  • tests/cron/test_cron_script.py: three tests (all @pytest.mark.platforms("linux", "macos"), matching the repo's host-marker rule):
    • an end-to-end _run_job_script run whose dependency exists only in the pinned directory (fails without the fix),
    • the argv/overlay contract (interpreter unchanged; tree first, dependency entry present),
    • the two skip cases: the interpreter's own purelib, and a dir for another Python version.

How to Test

  1. scripts/run_tests.sh tests/cron/test_cron_script.py -q → 34 passed, 1 skipped (Windows bootstrap tests skip on POSIX hosts); the same file is 31 passed on main.
  2. Reproduce the regression without a gateway, on any PM-managed install (store Python owns nothing):
    STORE=~/.hermes/tools/python-*/bin/python3
    PYTHONPATH=<repo> $STORE -c 'import hermes_yaml'    # ModuleNotFoundError: No module named 'ruamel'
    _script_argv() handed the child exactly that shape (repo root only, before this change: nothing at all).
  3. End to end: hermes -p <profile> cron run <no_agent job id> on a job whose script imports any runtime dependency → Ran now: succeeded, and the job's last_status flips from error to ok. Verified here on two real jobs (the ruamel and yaml failures above) plus the hourly reconciler that was already working.

Checklist

Code

Notes for reviewers

`cron/scheduler_script.py::_script_argv` launches a `.py` job as
`[sys.executable, path]`. On a PM-managed install that interpreter is the store
Python, whose own site-packages holds only pip, and `build_subprocess_env`
strips both Hermes-owned `PYTHONPATH` entries (repo root + the activated
dependency environment) before the child spawns. So the first non-stdlib import
in a script died with `ModuleNotFoundError` — every config-reading or
plugin-importing `no_agent` job failed on POSIX the moment the runtime moved off
the in-tree venv, while Windows never regressed because its branch overlays
`PYTHONPATH` (and bootstraps `.pth` support).

Add the POSIX twin. The interpreter is kept (it already IS the runtime's; only
the import path was missing) and the overlay is `[repo root, activated
dependency site-packages]`:

- the dependency entry is read from this process's own `sys.path`, which is
  where `pm.environments.activate_dependencies` publishes it (dependencies are
  exposed by path, `sys.prefix` is deliberately unchanged). No HERMES_HOME
  filesystem reads on the spawn path, so a process running under an overridden
  HERMES_HOME never touches the real home.
- it is skipped when the interpreter owns its dependencies (wheel/pipx install,
  dev venv): that already hands them to the child and pinning its own purelib
  would only reorder `sys.path`.
- it is skipped when the directory is not shaped like a venv's own for THIS
  interpreter version (`<venv>/lib/pythonX.Y/site-packages`), so a generation
  built for another Python can never be pinned — that would trade
  `ModuleNotFoundError` for import errors from compiled modules.

Tests: `tests/cron/test_cron_script.py` gains an end-to-end run whose
dependency exists only in the pinned directory, the argv/overlay contract, and
the two skip cases above.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cron Cron scheduler and job management sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Sep 25, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

Summary

This PR fixes a real and well-scoped bug: on a PM-managed POSIX install the cron script child is spawned as a bare [sys.executable, path] with both Hermes-owned PYTHONPATH entries stripped by build_subprocess_env, so a job script's first non-stdlib import (ruamel, httpx, …) dies before any real work. The fix adds _posix_cron_python_invocation() (cron/scheduler_script.py:202-223) and a sys.path-derived resolver, _activated_dependency_site_packages() (cron/scheduler_script.py:164-199), and routes the non-Windows branch of _script_argv through it (cron/scheduler_script.py:395-401). The direction is right, the version guard is a good catch, and the tests are real (I confirmed the new argv test fails against base scheduler_script.py and passes against head — it is not a shell).

The resolver itself is well-guarded. My concerns are all about the shape of the overlay rather than the fix's premise, plus one test-coverage gap that the Windows branch already handles and this one does not.

Findings

1. Non-blocking: the overlay replaces the user's PYTHONPATH instead of extending it, dropping user library paths from every POSIX cron child

_posix_cron_python_invocation builds a fresh two-entry list and returns it as the whole PYTHONPATH (cron/scheduler_script.py:219-223):

entries = [str(Path(__file__).resolve().parents[1])]
dependencies = _activated_dependency_site_packages()
if dependencies is not None and str(dependencies) not in entries:
    entries.append(str(dependencies))
return python_exe, {"PYTHONPATH": os.pathsep.join(entries)}

_run_job_script then applies it with env.update(env_overlay) (cron/scheduler_script.py:452-453), so the overlay replaces whatever build_subprocess_env left. That is fine for the two Hermes-owned entries — the sanitizer already removed exactly those. But the sanitizer deliberately preserves everything else, and states the contract in its own docstring: "Everything else -- user libs, Nix plugin paths, a pythonX.Y/site-packages entry meant for a DIFFERENT child version -- is preserved byte-for-byte: ownership is decided by path provenance, never by a cross-version heuristic" (tools/environments/local_pythonpath.py:135).

This overlay discards that preserved tail. Reproduced end to end on this host, simulating a user who exports a library dir on PYTHONPATH:

after sanitize, child PYTHONPATH = '...\tmp5f_tuppr\userlibs'
  user entry survives sanitizer?    True
BASE  child PYTHONPATH = '...\tmp5f_tuppr\userlibs'
  user entry survives?              True
HEAD  child PYTHONPATH = '...\tmp5f_tuppr'
  user entry survives?              False

So this is a behaviour regression that only exists on the branch the PR introduces: before, a POSIX cron script inherited the user's PYTHONPATH; after, it silently loses it. A job script that imports a user-installed package, or a Nix-provided plugin, now fails with ModuleNotFoundError and no hint that the path was dropped.

The Windows twin already does the right thing and shows the intended pattern — it explicitly appends whatever was there (cron/scheduler_script.py:154-159):

pythonpath_entries = [
    str(_sched.Path(__file__).resolve().parents[1]), str(site_packages)]
existing_pythonpath = os.environ.get("PYTHONPATH", "")
if existing_pythonpath:
    pythonpath_entries.append(existing_pythonpath)
env_overlay["PYTHONPATH"] = os.pathsep.join(pythonpath_entries)

Suggested fix: mirror it, reading the sanitized value rather than os.environ (the overlay is computed before the sanitizer runs, so os.environ is not the same string the child would otherwise have seen). Either pass the sanitized env["PYTHONPATH"] into the helper, or have _posix_cron_python_invocation return the entries and let _run_job_script splice them onto env.get("PYTHONPATH", ""). Also worth a test asserting a pre-existing user entry survives the POSIX overlay — the current test at tests/cron/test_cron_script.py:340-365 only inspects the overlay in isolation, never through _run_job_script, so it cannot catch this.

2. Non-blocking: no .pth processing on POSIX, so editable/plugin workspace members stay unimportable in the child

The PR deliberately skips the site.addsitedir bootstrap on POSIX. The rationale in the description is that the overlay "only needs to make runtime modules importable", and that the repo root coming first satisfies the editable-install finder. That holds for the core package, but not for plugin workspace members.

PM installs plugins as uv workspace members (pm/packages.py:432 syncs root=generation / "workspace"; pm/workspace.py:252 places each member at plugin-sources/<key>), and pm/environments.py:339 documents the consequence directly: "uv editable members are activated by .pth files, not by sys.path alone." A member's own code is only reachable through a .pth line inside the generation's site-packages — which the child never processes, because PYTHONPATH is put on sys.path directly without site seeing it.

Three-way reproduction, with a member importable only through a path-style .pth:

A plain, no overlay                        FAIL -> ModuleNotFoundError: No module named 'mypkg'
B PYTHONPATH overlay (this PR, POSIX)      FAIL -> ModuleNotFoundError: No module named 'mypkg'
C addsitedir bootstrap (Windows)           OK

Case C is the same bootstrap _windows_cron_bootstrap_argv uses (cron/scheduler_script.py:260-268). The Windows path therefore still supports plugin imports in cron scripts, and after this PR the POSIX path does not — the two branches diverge in capability, which cuts against the "twin" framing in the function's own docstring (cron/scheduler_script.py:203).

I'd treat this as worth resolving before merge, or at minimum as an explicit, documented limitation. The cheap fix is to reuse the existing bootstrap on POSIX whenever _activated_dependency_site_packages() returned a directory — it already handles sys.path[0] and __file__ semantics correctly, and the interpreter is unchanged either way. If the maintainers prefer to keep this scoped, a follow-up issue referenced from the code would be enough, but the "twin" naming should not imply parity that does not exist.

3. Minor: sys.platform is checked twice; the guard inside the new POSIX helper is unreachable

_script_argv now dispatches on sys.platform == "win32" at cron/scheduler_script.py:395 and only calls _posix_cron_python_invocation on the other branch (cron/scheduler_script.py:400-401). The helper's own if sys.platform == "win32": return python_exe, {} (cron/scheduler_script.py:217-218) can therefore never fire from the production path. It does keep the helper safe to call directly, which is presumably why the argv test calls it on Windows hosts — but the docstring says "POSIX twin of _windows_cron_python_invocation", whose identical guard (cron/scheduler_script.py:122) is genuinely reachable because base _script_argv called it unconditionally. Not a bug; just a small asymmetry that could confuse the next reader. Either drop the inner guard and rely on the caller's dispatch, or note that it exists to make the helper independently callable.

4. Minor: three blank lines precede the first new test

tests/cron/test_cron_script.py:307-310 has four consecutive blank lines between the last Windows test and test_posix_script_pins_the_activated_dependency_environment at line 311. The surrounding file uses two throughout. Cosmetic, but it is the kind of thing E303 flags in repos that later enable it, and it suggests a merge artifact worth cleaning up in the same commit.

Verification notes

I did not run the suite (this is a POSIX-gated change on a Windows host, so all three new tests would be skipped here). Instead I exercised the two new functions directly against both the base and head sources: the new argv-contract test at tests/cron/test_cron_script.py:340-365 fails on base (PYTHONPATH missing) and passes on head, so it is a real regression test. Findings 1 and 2 are reproductions, not inferences — the transcript lines above are actual output from this host.

One note on the PR description: it says the same file is 31 passed on main and 34 passed, 1 skipped here, and that 83 pre-existing tests/cron failures reproduce with the fix reverted. That is consistent with what I would expect from an unprovisioned install, and the CI matrix does run a Linux lane (.github/workflows/tests.yml), so the platforms("linux", "macos") marks will be exercised. No objection there.

@kshitijk4poor

Copy link
Copy Markdown

Thanks @nhod (earliest of the fixes for this). This is superseded by #124973, which is armed for rebase-merge. It fixes the same POSIX managed-store cron-script ModuleNotFoundError (#123044 / #123440). It builds on #123051: the script runs on the selected venv's own interpreter and loads the live checkout through an in-process bootstrap. There's no PYTHONPATH overlay, because child processes inherit PYTHONPATH (the breakage in #123440). You're credited in the PR body. Closing; I'll reopen if #124973 doesn't land.

The restart-safe external worker in #122222 is a separate path. #124973 doesn't touch it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants