fix(approval): use per-job ContextVar for cron-session flag instead of leaking env var (#56771) - #56796
Conversation
…f leaking env var (NousResearch#56771) The scheduler set os.environ["HERMES_CRON_SESSION"]="1" process-wide at job start and never cleared it. When the gateway and scheduler share a process (the normal architecture), the env var leaked into concurrent interactive sessions, causing execute_code and dangerous commands to be blocked for users chatting via Telegram/CLI/TUI — even though they never ran a cron job in their session. Replace the process-global env var with a task-local ContextVar (_CRON_SESSION) that is set per-job in run_job() and cleared in the finally block. The approval system checks the ContextVar first (via is_cron_session()), falling back to the env var only for backward compat with tests and CLI cron paths that set it directly. Changes: - gateway/session_context.py: add _CRON_SESSION ContextVar + helpers (set_cron_session / clear_cron_session / is_cron_session) - cron/scheduler.py: replace os.environ set with set_cron_session(True); add clear_cron_session() to the finally block - tools/approval.py: replace all 4 env_var_enabled("HERMES_CRON_SESSION") checks with _is_cron_session() (contextvar-aware with env fallback) - tests/tools/test_cron_session_leak.py: 11 regression tests covering thread isolation, contextvar override, backward compat, and the full execute_code guard matrix
|
Recommendation: this looks like the strongest lane for the #56771 cron-session leak cluster, including the newer corroborating reports like #57736 and #60350. Why I would pick this over the sibling PRs:
Local proof on #56796: scripts/run_tests.sh tests/tools/test_cron_session_leak.py tests/tools/test_cron_approval_mode.py tests/tools/test_execute_code_approval_cluster.py -- -q
# 58 tests passed
git diff --check origin/main...HEAD
# clean
.venv/bin/python scripts/check-windows-footguns.py cron/scheduler.py gateway/session_context.py tools/approval.py tests/tools/test_cron_session_leak.py tests/tools/test_cron_approval_mode.py tests/tools/test_execute_code_approval_cluster.py
# passTwo-lens read: the runtime mechanism is covered because the new test demonstrates scheduler-thread cron state does not appear in a sibling gateway thread, while real cron contexts still hit |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for targeting the scheduler-owned process-global marker: current main still writes os.environ["HERMES_CRON_SESSION"] in cron/scheduler.py:2812, while the approval gates consume it in tools/approval.py:241, :2173, :2700, and :3121.
Problems
tools/approval.py:189now delegates cron detection togateway.session_context.is_cron_session(). The existing cron-mode tests intests/tools/test_request_tool_approval.py:106-107and:116-117mock onlyapproval.env_var_enabled, so that mock no longer controls the cron branch. Those tests need to set the environment marker or mock_is_cron_session.
Suggested changes
- Update both affected tests and include
tests/tools/test_request_tool_approval.pyin the focused validation set.
Automated hermes-sweeper review.
| try: | ||
| from gateway.session_context import is_cron_session | ||
|
|
||
| return is_cron_session() |
There was a problem hiding this comment.
This bypasses the approval.env_var_enabled mock used by tests/tools/test_request_tool_approval.py:106-107 and :116-117; those two cron-mode tests will no longer simulate cron in a clean environment. Please update them to set HERMES_CRON_SESSION or mock _is_cron_session.
…arch#56796) Sweeper feedback: update affected tests to test ContextVar-based approach instead of old env-var approach.
What
execute_code(and dangerous-command guards) were blocked in interactive gateway/CLI/TUI sessions wheneverHERMES_CRON_SESSION=1was present in the process environment — even though the user never ran a cron job in that session (#56771).Root cause
cron/scheduler.pysetos.environ["HERMES_CRON_SESSION"] = "1"process-wide at job start and never cleared it. The approval system then gates onenv_var_enabled("HERMES_CRON_SESSION")at 4 sites. When the gateway and scheduler share a process (the normal architecture), the env var leaks via inheritance into every concurrent interactive session, so the approval system treats user chats as cron and blocksexecute_code/ dangerous commands.This was the only place in the codebase that set the var, and
approval.pywas the only consumer — so replacing the process-global set has no other side effects.Fix
Replace the process-global env var with a task-local
ContextVarso the cron flag cannot leak into concurrent interactive sessions:gateway/session_context.py— add_CRON_SESSIONContextVar (mirroring the existing_UNSETsentinel pattern of the session vars) plusset_cron_session()/clear_cron_session()/is_cron_session()helpers.is_cron_session()checks the ContextVar first and falls back to the env var only for backward compat with tests / CLI cron paths that set it directly (in production the scheduler no longer sets it, so interactive sessions fall through toFalse).cron/scheduler.py— replace theos.environset withset_cron_session(True); addclear_cron_session()to therun_job()finally block. The flag is set before the scheduler's existingcopy_context()(line 2861) that the agent thread runs inside (_cron_context.run(agent.run_conversation)), so real cron jobs still seeTrueandcron_mode: denykeeps working.tools/approval.py— add_is_cron_session()(contextvar-aware, lazy-imported like the existingget_session_envcalls) and replace all 4env_var_enabled("HERMES_CRON_SESSION")checks with it.Why ContextVar and not env-var reordering
The scheduler sets the cron flag before spawning the agent in a worker thread via
copy_context().run(...). ContextVars propagate downward throughctx.run, so the agent thread seesTrue; concurrent interactive sessions run in their own tasks/contexts where the var is_UNSET, so they resolve toFalse. By contrastos.environis shared across all threads/process-children, which is precisely the leak this fixes. Competing approaches that merely reorder the gateway/cron precedence checks (or gate on interactive indicators) leave the env var leaking into spawned subprocesses and reverse the original design invariant that cron takes absolute priority over gateway approval.How verified
tests/tools/test_cron_session_leak.py): contextvar resolution, the core thread-isolation mechanism (scheduler thread sets the flag, concurrent gateway thread does not see it), env-var fallback for backward compat, contextvar-overrides-leaked-env, and the fullcheck_execute_code_guard+_is_gateway_approval_contextmatrix (cron blocks, interactive allowed, interactive-after-scheduler-ran still allowed).main(0 1).Closes #56771.
Auto-published by Moonsong via Path B automated pipeline.