fix(cron): scrub Kanban worker env from non-dispatcher subprocess spawns - #87756
Open
QDung210 wants to merge 2 commits into
Open
fix(cron): scrub Kanban worker env from non-dispatcher subprocess spawns#87756QDung210 wants to merge 2 commits into
QDung210 wants to merge 2 commits into
Conversation
`_scrub_delegated_child_kanban_env()` in tools/environments/local.py only checked `is_delegated_child_process_context()`, never the unifying `is_dispatcher_owned_worker_context()` predicate added for cron isolation. A cron job fired in-process from inside a Kanban worker is marked non-dispatcher-owned in-process, but every subprocess it spawned (via build_subprocess_env / hermes_subprocess_env) still inherited the worker's HERMES_KANBAN_* env, letting a cron subprocess act with the worker's board/task authority. Also wrap the two subprocess-launching call sites in cron/scheduler.py's run_job() (the no_agent script path and the wake-gate prerun-script path) with non_dispatcher_owned_context(), since both build their subprocess env before the existing marker is entered later in the function. Fixes NousResearch#87725
Contributor
fix(cron): scrub Kanban worker env from non-dispatcher subprocess spawns
|
…clarify comments Addresses AI review on NousResearch#87756: - Add explicit assertions that is_dispatcher_owned_worker_context() is False DURING the no_agent/prerun script execution itself (not just inferred from the scrubbed env), confirming the wrap covers the whole _run_job_script_with_claim_heartbeat call. - Clarify the wrap comments: the agent path's tool loop was already correctly covered by the existing later marker; only the two early script-launch call sites were the gap. Note explicitly that the two `with` blocks fully exit (ContextVar reset) before that later token-based entry, so they are sequential, not nested — no token-restore ordering hazard. No production behavior change; scrub_kanban_env() strips only HERMES_KANBAN_* keys (confirmed, unchanged) so dispatcher-owned subprocesses are unaffected.
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.
What does this PR do?
Closes a Kanban-authority leak in cron's non-dispatcher isolation. Cron jobs can be fired in-process from inside a Kanban worker (
cronjob(action="run")callsrun_job()in the worker's own process, whereHERMES_KANBAN_*is legitimately set).run_job()marks itself non-dispatcher-owned viaenter_non_dispatcher_owned_context()/is_dispatcher_owned_worker_context()so it isn't misidentified as the worker in-process — but that identity fix never extended to subprocesses the cron job spawns.Root cause:
_scrub_delegated_child_kanban_env()intools/environments/local.py(the sole chokepoint forbuild_subprocess_env()/hermes_subprocess_env()) only ever checked the olderis_delegated_child_process_context()(fordelegate_taskchildren). It was never migrated to also checkis_dispatcher_owned_worker_context()— "the single predicate everyHERMES_KANBAN_*identity gate should use" per its own docstring — after that predicate was introduced. So any subprocess a non-dispatcher-owned cron job spawned (script jobs, terminal calls, browser/tts/lazy-deps helpers, etc.) still inherited the worker'sHERMES_KANBAN_*env for its full duration.On top of that, two specific spawn sites in
cron/scheduler.py'srun_job()— theno_agentscript-launch path and the wake-gate prerun-script path — build their subprocess env before the existingenter_non_dispatcher_owned_context()call further down in the function, so even a corrected predicate wouldn't have covered them without also wrapping those two call sites explicitly.Fix:
tools/environments/local.py:_scrub_delegated_child_kanban_env()now also scrubs whennot is_dispatcher_owned_worker_context(), alongside the existing delegated-child check.cron/scheduler.py: wrap the two early script-launch call sites withnon_dispatcher_owned_context()so their subprocess env is built while the marker is active.No behavior changes to any dispatcher-owned (normal worker) or delegated-child path — the new condition only fires when one of the two ContextVars is actually set.
Note for maintainers: open PR #81843 edits the same
_scrub_delegated_child_kanban_envfunction (for a different, unrelated gap — terminal-spawned subprocess isolation from delegate_task children). The two patches don't touch the same lines but whichever lands second will need a small rebase.Related Issue
Fixes #87725
Type of Change
Changes Made
tools/environments/local.py:_scrub_delegated_child_kanban_env()now scrubs Kanban env whenis_dispatcher_owned_worker_context()is False, not just for delegated children.cron/scheduler.py: wrap theno_agentscript-launch call and the wake-gate prerun-script call innon_dispatcher_owned_context().tests/cron/test_cron_kanban_env_isolation.py: newTestSubprocessEnvScrubclass — 6 tests covering the direct predicate gap (build_subprocess_env/hermes_subprocess_envinsidenon_dispatcher_owned_context()), the pre-existing delegated-child path (unaffected), and bothrun_job()script-launch sites via a fake_run_job_scriptthat captures the env it was actually given.How to Test
non_dispatcher_owned_context(), calltools.environments.local.build_subprocess_env()—HERMES_KANBAN_TASK/_RUN_ID/_CLAIM_LOCK/etc. are present in the returned env despite the context marking this execution as not dispatcher-owned.pytest tests/cron/test_cron_kanban_env_isolation.py -q -k TestSubprocessEnvScrub— 4 of the 6 new tests fail against the pre-fix code (verified by stashing the two production-file changes) and pass after the fix; the other 2 assert unchanged baseline behavior (dispatcher-owned and delegated-child paths still work as before).pytest tests/cron/test_cron_kanban_env_isolation.py -q→ 24 passed.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/cron/test_cron_kanban_env_isolation.py -qand all tests pass (24 passed). I also ranpytest tests/cron/ -q(690 passed, 20 skipped, 7 pre-existing failures unrelated to this change — Windows POSIX-permission assertions intest_file_permissions.pyand a tilde-expansion assertion intest_cron_workdir.py, all reproduced identically onorigin/mainwithout this patch) andpytest tests/tools/test_delegate.py tests/tools/test_delegate_kanban_isolation.py tests/tools/test_delegate_cron_sync_fallback.py tests/test_delegate_cascade_49148.py tests/agent/test_subprocess_env_guard.py tests/cron/test_cron_script.py tests/tools/test_build_subprocess_env.py tests/tools/test_env_passthrough.py -q(133 passed, 1 skipped). I did not run the fulltests/suite (not attempted; only the modules above and directly-adjacent ones).Documentation & Housekeeping
docs/, docstrings) — or N/A (docstring on the changed function updated; no user-facing docs affected)cli-config.yaml.exampleif I added/changed config keys — or N/A (no config keys changed)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A (no architecture/workflow change)scripts/check-windows-footguns.pyagainst the changed files, no findingsUpdate (2026-08-16): addressed AI review feedback
non_dispatcher_owned_context()restores viaContextVar.reset(token)in afinally, and the two new wraps fully exit before the existing later token-basedenter_non_dispatcher_owned_context()call — they're sequential, not nested, so there's no token-restore hazard. Added an explicit comment saying so at both wrap sites.scrub_kanban_env()only ever pops theKANBAN_ENV_KEYStuple and sets the delegated-child marker — nothing else. In a plain (non-cron, non-delegated) processis_dispatcher_owned_worker_context()defaults toTrue, so the newnot is_dispatcher_owned_worker_context()condition isFalseand normal subprocess spawns take the same no-op path as before.test_no_agent_script_subprocess_env_is_scrubbedandtest_prerun_script_subprocess_env_is_scrubbedthatis_dispatcher_owned_worker_context()isFalseduring the script call itself (not just inferred from the scrubbed env), confirming the wrap covers the whole_run_job_script_with_claim_heartbeatcall as requested.with non_dispatcher_owned_context():one-liners at genuinely different call sites didn't seem worth an extraction; happy to do it if a maintainer prefers.Screenshots / Logs
N/A