Skip to content

fix(cron): isolate approval state per session - #70768

Open
DOUIF wants to merge 1 commit into
NousResearch:mainfrom
DOUIF:fix/cron-context-isolation
Open

fix(cron): isolate approval state per session#70768
DOUIF wants to merge 1 commit into
NousResearch:mainfrom
DOUIF:fix/cron-context-isolation

Conversation

@DOUIF

@DOUIF DOUIF commented Jul 24, 2026

Copy link
Copy Markdown

Summary

  • replace process-global HERMES_CRON_SESSION mutation with session-local ContextVar state
  • preserve environment fallback for standalone/legacy cron invocations
  • add regression coverage proving cron state is cleaned up and does not leak into normal gateway turns

Root cause

The gateway scheduler and normal Telegram/API turns share one process. Setting os.environ["HERMES_CRON_SESSION"] = "1" inside a job permanently changes process-global state, so later non-cron turns and MCP children inherit cron approval behavior.

Verification

pytest -q tests/cron tests/tools/test_cron_context_isolation.py tests/tools/test_cron_approval_mode.py tests/tools/test_local_env_session_leak.py tests/gateway/test_session_context_inheritance.py
794 passed, 1 pre-existing RuntimeWarning

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery 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 duplicate This issue or pull request already exists labels Jul 24, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #58663: both use a job-local ContextVar to replace the process-global cron marker. #58663 additionally covers stale-env masking for interactive gateway turns and inherited ask-mode behavior.

@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 cron provenance from the gateway process; the current-main leak is real (cron/scheduler.py:3014 and tools/approval.py:241).

Problems

  • gateway/session_context.py:244 adds _CRON_SESSION to clear_session_vars(), which writes "". get_session_env() returns any non-_UNSET ContextVar value without consulting os.environ (gateway/session_context.py:325-331). After run_job() calls its finally cleanup (cron/scheduler.py:3765), a later standalone/legacy HERMES_CRON_SESSION=1 path in that context is misclassified as non-cron and can bypass cron_mode: deny.

Suggested changes

  • Bind the cron marker directly in run_job() and retain its ContextVar token; reset that token in the finally block after generic session cleanup. This restores the pre-job _UNSET state and preserves the documented environment fallback.
  • Add a real run_job()-then-env-fallback regression. Open duplicate #58663 already carries this token-reset pattern and broader stale-environment coverage.

Automated hermes-sweeper review.

_SESSION_UI_SESSION_ID,
_SESSION_MESSAGE_ID,
_SESSION_PROFILE,
_CRON_SESSION,

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.

Adding this marker to the generic clear list makes clear_session_vars() set it to "". get_session_env() treats that explicit empty value as authoritative, so a later legacy HERMES_CRON_SESSION=1 environment fallback in the same context is ignored. Keep a token from the job-local marker binding and reset it in run_job()'s finally instead.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/sessions Session lifecycle, resume, persistence, history labels Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants