Skip to content

fix: isolate cron approval context - #36004

Open
HeyClaw123 wants to merge 1 commit into
NousResearch:mainfrom
HeyClaw123:fix/cron-approval-context-leak
Open

fix: isolate cron approval context#36004
HeyClaw123 wants to merge 1 commit into
NousResearch:mainfrom
HeyClaw123:fix/cron-approval-context-leak

Conversation

@HeyClaw123

Copy link
Copy Markdown

Summary

  • move the cron approval marker into session contextvars instead of process-global os.environ
  • make approval guards use the context-local cron marker so gateway sessions are not poisoned by stale HERMES_CRON_SESSION
  • add regression coverage for live Discord/gateway approval flows with a stale process-level cron flag, while preserving cron cron_mode behavior

Test plan

  • python -m pytest tests/gateway/test_session_env.py tests/tools/test_cron_approval_mode.py tests/tools/test_execute_code_approval_cluster.py tests/tools/test_approval.py::TestApprovalTimeoutIsNotConsent tests/cron/test_cron_script.py tests/cron/test_scheduler.py -q

Context

In a long-lived gateway process, cron scheduler ticks could leave HERMES_CRON_SESSION=1 in process-global environment. Later live Discord/Telegram sessions in the same process could then be misclassified as cron and get cron-only approval behavior, e.g. execute_code being blocked with “Cron jobs run without a user present...” even though a live gateway user was present. This keeps the cron safety default intact while preventing that marker from leaking across sessions.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P2 Medium — degraded but workaround exists comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery labels May 31, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Competing with #31184 — same core fix (move HERMES_CRON_SESSION from os.environ to contextvars). This PR also adds gateway session hygiene and broader regression test coverage.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for isolating a real process-global approval leak. Current main still writes HERMES_CRON_SESSION globally at cron/scheduler.py:2688, and the approval layer still consumes it before gateway classification at tools/approval.py:194 and in the execute-code guard at tools/approval.py:3019.

Problems

  • This branch is based on 1044d9f25d63b48c51fe40af0a4cfeea3b6de516; current main has substantially changed all touched files, including the session-context lifecycle and approval implementation. This needs manual salvage rather than a clean cherry-pick.
  • The regression coverage does not exercise the scheduler's real contextvars.copy_context() worker handoff at cron/scheduler.py:3138-3139, where the cron marker must survive into agent.run_conversation.

Suggested changes

  • During salvage, add a scheduler-worker regression proving that cron retains cron_mode in the copied worker context while a concurrent gateway context is not classified as cron.

This is an automated hermes-sweeper review.

Comment thread cron/scheduler.py
@@ -1430,6 +1425,7 @@ def _run_job_impl(job: dict) -> tuple[bool, str, str, Optional[str]]:
platform="",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please add a regression through the scheduler's copied worker context as well: current main submits contextvars.copy_context().run(agent.run_conversation, prompt) at cron/scheduler.py:3138-3139, so the cron marker must survive that hop without appearing in a concurrent live gateway context.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit labels Jul 13, 2026
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 comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants