Skip to content

fix(kanban): keep task cwd off sys.path of module-form workers - #138506

Open
nareshh74 wants to merge 3 commits into
NousResearch:mainfrom
nareshh74:fix/kanban-worker-safe-path-upstream
Open

nareshh74 wants to merge 3 commits into
NousResearch:mainfrom
nareshh74:fix/kanban-worker-safe-path-upstream

Conversation

@nareshh74

@nareshh74 nareshh74 commented Oct 14, 2026 •

Copy link
Copy Markdown

What does this PR do?

Workers launched with python -m hermes_cli ... (module form) and a cwd that is itself a hermes-agent checkout import that checkout's hermes_cli instead of the pinned install. -m puts cwd first on sys.path, so the checkout's dependency-less copy shadows the pinned tree and the worker dies with no dependency environment is committed. Reproduced on main with a decoy hermes_cli/ in cwd (see regression test).

Fix: pin_hermes_tree_on_pythonpath (cron/scheduler_worker_env.py, the shared helper) now also sets PYTHONSAFEPATH=1, so cwd is never added to sys.path for the worker. All callers going through the helper are covered: kanban dispatcher (_propagate_module_import_root), cron external worker, bot-chat delivery. pyproject requires Python >=3.11, where PYTHONSAFEPATH exists.

Side effect handled: the agent's own children (terminal / execute_code) inherit the worker's os.environ. The helper records a private marker _HERMES_PINNED_SAFEPATH (holding the user's original PYTHONSAFEPATH, if any); the worker boot (hermes_cli/main.py, cron/worker_bootstrap.py) runs after the interpreter has latched sys.flags.safe_path and restores the user's original value and removes the marker. Absent, 1, and other nonempty values are restored exactly; on Windows an originally empty value becomes absent/unset (accepted behavior).

Related Issue

Related: #126127 (Kanban module-form worker cwd shadowing).

Type of Change

  • Bug fix (non-breaking)

Changes Made

  • cron/scheduler_worker_env.py: set PYTHONSAFEPATH=1 and a marker recording the original value.
  • cron/worker_bootstrap.py, hermes_cli/main.py: restore the user's original value at boot.
  • tests/cron/test_restart_safe_worker.py: updated pin expectation; added decoy-cwd test and eight preservation cases.
  • 4 files, +92/-1.

How to Test

Head baaaead4a28b21876bb0dbfcb8e6e353a169b36a, base 79651b71bd56efd76d06f3de1df64dde09b58ff3; Windows 11, Python 3.14.7.

  1. python -m pytest tests/cron/test_restart_safe_worker.py -k "pinned_safe_path_restores_user_value_for_children or pinned_worker_ignores_decoy_hermes_cli_in_cwd": 9 passed.

  2. python -m pytest tests/cron/test_restart_safe_worker.py: head 1 failed / 44 passed / 4 skipped; base 1 / 35 / 4.

  3. python -m pytest tests/cron tests/hermes_cli/test_kanban*: head 75 failed / 1861 passed / 72 skipped; base 75 / 1852 / 72. Failure IDs identical on base and head. Causes are mixed and partly undiagnosed; not all are Windows-only.

  4. python scripts/check-windows-footguns.py --diff: exit 1, 8 diagnostics. 2 are introduced by this PR: bare write_text calls (no explicit encoding) at tests/cron/test_restart_safe_worker.py:767 and :799; both write ASCII, but this does not satisfy the encoding rule. The other 6 diagnostics are outside the added hunks (pre-existing). This check is not a PASS; the 2 introduced findings are open and need a minimal encoding fix before merge.

Known baseline failure in the focused suite: test_scoped_wrapper_exit_without_user_bus_names_the_cause_and_invalidates. The 4 skips are the same on base and head.

Not run: full pytest tests/ and python scripts/check; CI is authoritative. Not tested on Linux/macOS. Suites above are not green; they fail identically on base.

Checklist

  • Read CONTRIBUTING / AGENTS.md rubric
  • Conventional Commits
  • Only changes related to this fix
  • Tests added (decoy test fails on main, passes here)
  • Full pytest tests/ and python scripts/check not run locally
  • Platform tested: Windows 11 only

Mirrors fork PR nareshh74#18.

kumarnareshh74 and others added 2 commits October 14, 2026 22:39
Set PYTHONSAFEPATH=1 when pinning the Hermes tree so a hermes-agent
checkout used as a worker cwd cannot shadow the pinned install.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ildren

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cron Cron scheduler and job management comp/cli CLI entry point, hermes_cli/, setup wizard labels Oct 14, 2026
The pin helper overwrote a user-set PYTHONSAFEPATH and the boot then dropped it
unconditionally. Record the user's value in _HERMES_PINNED_SAFEPATH ("-" absent,
"+<value>") and restore it at boot, in both bootstrap paths. cwd-shadow
protection in the worker is unchanged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
# the worker's boot (``hermes_cli.main`` / ``cron.worker_bootstrap``) drops the var so the agent's own children (terminal /
# execute_code) keep the normal script-dir/cwd sys.path entry. A user's own value
# (no marker) is left alone.
worker_env["PYTHONSAFEPATH"] = "1"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pin helper overwrites any inherited PYTHONSAFEPATH and always sets the marker, so hermes_cli.main/worker_bootstrap later remove the variable even when it was set by the user before dispatch. That contradicts the comment that a user-owned value is preserved and changes the environment passed to terminal/execute_code children. Could the helper preserve the prior value (for example in a separate marker value) and restore it after interpreter startup, with a test that starts from an env already containing PYTHONSAFEPATH?

@djagya djagya left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at head baaaead4a28b21876bb0dbfcb8e6e353a169b36a (macOS, Python 3.14.7, scripts/run_tests.sh).

The root cause matches one we hit and fixed in a downstream fork: -m puts the task workspace first on sys.path, so a workspace that is a Hermes checkout shadows the install. Our fix works at the argv level (python -P -c <runpy bootstrap> for the module-form kanban worker) and has run in a downstream deployment for 5+ days. The env-based pin here also covers the cron worker and bot-chat delivery, and the marker now restores the caller's own PYTHONSAFEPATH, so the earlier review point is addressed at this head.

What I ran

  • tests/cron/test_restart_safe_worker.py tests/hermes_cli/test_kanban_db.py: 98 passed, 1 failed, 5 skipped. The failure (test_infrastructure_spawn_refusal_never_charges_the_card) fails the same way on base 79651b71bd, so it is not from this PR.
  • Our fork's regression test, ported to this PR's pin: the real _module_hermes_argv() plus _propagate_module_import_root() env, running --version from a workspace that holds hermes_cli/main.py (exits 97) and an inspect.py (exits 98). It passes on this head and fails on base with rc=97.

One gap: wheel / pipx / uv-tool installs

cron/scheduler_worker_env.py:81-82 returns before the PYTHONSAFEPATH pin (:93) when the repo root is the interpreter's purelib. In that layout the installed hermes_cli lives in site-packages, which comes after the cwd entry on sys.path, so a workspace checkout still shadows it. Repro: the PR's own decoy test with _installed_purelib monkeypatched to return the repo root, and the root on PYTHONPATH standing in for site-packages. It fails with DECOY IMPORTED. This is a simulated layout; I did not build a wheel install.

Suggested patch: set the pin before the early return, so the purelib skip only guards the PYTHONPATH prepend.

@@ def pin_hermes_tree_on_pythonpath(worker_env: dict, repo_root: Path) -> dict:
-    root = str(repo_root)
-    if _installed_purelib() == Path(root).resolve():
-        return worker_env
     # The child's cwd (task workspace) is first on sys.path for ``-m``; ...
     if SAFE_PATH_PIN_MARKER not in worker_env:
         user = worker_env.get("PYTHONSAFEPATH")
         worker_env[SAFE_PATH_PIN_MARKER] = "-" if user is None else "+" + user
     worker_env["PYTHONSAFEPATH"] = "1"
+    root = str(repo_root)
+    if _installed_purelib() == Path(root).resolve():
+        return worker_env

The wheel-layout assertion at tests/cron/test_restart_safe_worker.py:689 then becomes "PYTHONPATH untouched, PYTHONSAFEPATH == "1"". With that applied locally, the two files above plus the purelib repro and the ported test: 100 passed, with the same single base failure.

Minor, already noted in the description: the new write_text calls at tests/cron/test_restart_safe_worker.py:767 and :799 need encoding="utf-8" for the Windows footgun check.

This branch has not been deployed

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

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants