fix(security): scope the cron-session approval marker to the job context

[Adolanium]

What does this PR do?

The cron scheduler marks its execution as a cron session so the approval gate can apply approvals.cron_mode. It did this with a process-global env var: run_job set os.environ["HERMES_CRON_SESSION"] = "1" and never cleared it.

The default deployment runs the cron ticker in-process inside the gateway (InProcessCronScheduler in a daemon thread of the same process that serves Telegram/Discord/Slack). So after the first cron tick the marker is set for the whole gateway process, permanently. _is_gateway_approval_context() checks HERMES_CRON_SESSION first and returns False, shadowing the per-session HERMES_SESSION_PLATFORM contextvar the concurrent gateway path relies on (the gateway sets no HERMES_GATEWAY_SESSION env var). Every interactive user is then misclassified as a cron session:

• Default cron_mode: deny: the user's dangerous command is hard-blocked with a "cron jobs run without a user present" message and the interactive approve flow is never reached.
• cron_mode: approve: the user's dangerous command is auto-approved with no prompt, silently bypassing the human approval gate.

The fix carries the marker on a per-job HERMES_CRON_SESSION contextvar (set in run_job, cleared in its finally), mirroring the cron delivery targets that were already moved to contextvars for the same process-global reason. The four approval readers go through a new _is_cron_session() helper that reads the contextvar first via get_session_env and falls back to os.environ for the standalone hermes cron process and for tests. The marker is now isolated to the cron job's own context, so concurrent interactive sessions are unaffected and cron jobs still get cron_mode applied.

Related Issue

Fixes #58662

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✨ New feature (non-breaking change that adds functionality)
• 🔒 Security fix
• 📝 Documentation update
• ✅ Tests (adding or improving test coverage)
• ♻️ Refactor (no behavior change)
• 🎯 New skill (bundled or hub)

Changes Made

• gateway/session_context.py: add a HERMES_CRON_SESSION ContextVar and register it in _VAR_MAP so get_session_env resolves it (contextvar first, os.environ fallback).
• cron/scheduler.py: in run_job, set the marker via the ContextVar instead of os.environ, and reset it to a falsy value in the existing finally alongside the cron delivery vars.
• tools/approval.py: add _is_cron_session() (reads the marker via get_session_env) and route the four HERMES_CRON_SESSION readers (_is_gateway_approval_context, check_dangerous_command, check_all_command_guards, check_execute_code_guard) through it.
• tests/cron/test_cron_session_marker_isolation.py: new regression tests.

How to Test

1. Reproduce on main: call cron.scheduler.run_job(...) once, then os.environ.get("HERMES_CRON_SESSION") is "1", and a bound gateway session (set_session_vars(platform="telegram", ...)) has _is_gateway_approval_context() return False.
2. With this change run_job leaves no HERMES_CRON_SESSION in os.environ, and the bound gateway session's _is_gateway_approval_context() returns True.
3. Proof the new tests exercise the bug: with the tools/approval.py + cron/scheduler.py + gateway/session_context.py changes reverted, test_run_job_does_not_leak_cron_marker_into_process_env and test_bound_gateway_session_not_shadowed_by_in_process_cron fail. With the change in place all five pass.
4. python -m pytest tests/cron/ tests/tools/test_approval.py tests/tools/test_hardline_blocklist.py tests/tools/test_cron_approval_mode.py tests/tools/test_execute_code_approval_cluster.py -q - 1140 passed (the 7 remaining failures are pre-existing Windows-only file-mode / tilde-expansion tests, identical with the change reverted).
5. ruff check tools/approval.py cron/scheduler.py gateway/session_context.py tests/cron/test_cron_session_marker_isolation.py - clean.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix/feature (no unrelated commits)
• I've run pytest tests/ -q and all tests pass
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: Windows 11

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — or N/A (docstrings updated in place)
• I've updated cli-config.yaml.example if I added/changed config keys — or N/A (no config keys)
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
• I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A (contextvar/env logic, platform-independent)
• I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

Behavior before and after, reproduced against run_job and the approval context:

# on main (leak)
run_job(...) once
os.environ["HERMES_CRON_SESSION"]         -> "1"   (never cleared)
_is_gateway_approval_context() for a bound telegram session -> False   (misrouted to cron)

# with this PR
run_job(...) once
os.environ.get("HERMES_CRON_SESSION")     -> None  (marker was a per-job contextvar)
_is_gateway_approval_context() for a bound telegram session -> True    (interactive)
_is_cron_session() inside the running job -> True   (cron_mode still applies)

[AmirF194]

This is a well-scoped fix for a genuine security bug, and the diagnosis holds up. On main, run_job sets HERMES_CRON_SESSION=1 in os.environ and never clears it, and since the default scheduler ticks in-process, after the first tick every interactive user is misclassified as cron: cron_mode: deny hard-blocks their commands, cron_mode: approve silently auto-approves them. I ran your test file in a clean Python 3.11 container matching CI and reverted the three source files to main: the two core tests fail on the real assertions (run_job leaves HERMES_CRON_SESSION=1, and a bound session is misrouted to cron), the other three fail on the missing _is_cron_session helper. With the fix restored, 5 passed.

Putting the marker in _VAR_MAP is what makes this complete rather than a symptom patch: it plugs into the existing _inject_session_context_env leak-guard, so a cron job's subprocess or delegated child still inherits the marker while concurrent interactive sessions get it stripped. Combined with the per-job copy_context() dispatch, overlapping jobs and child sessions are both handled, and moving off os.environ also closes the across-restart and reused-session-id angles. Reader coverage looks complete (all four consumers route through _is_cron_session).

Two non-blocking notes. The marker .set("1") sits just outside the try/finally that clears it, so an exception in that window could leave it set in the standalone hermes cron loop thread (harmless in the in-process default since the context is discarded). Tucking it inside the try would match the delivery vars. And since three of the five tests only fail on the missing helper import when source is reverted, a test that drives two real run_job calls in overlapping contexts would round out the raw-contextvar isolation test. Neither blocks.

[Adolanium]

Thanks for the thorough read, and for reverting the three source files in a clean 3.11 container to check the two core tests fail on the real assertions rather than just the missing import. That is the signal I wanted from them.

Both notes are addressed in the follow-up commit.

The marker .set("1") now sits as the first statement inside the try, so the finally always clears it. You are right that on the in-process default it was harmless (the tick's context is discarded), but on the standalone hermes cron loop thread the context is reused, so a raise between the old set and the try (in set_session_vars, the workdir check, or the cwd-lock acquire) could have left it set for the next read. The set still precedes the copy_context() dispatch, so the pool thread still inherits it, and it now matches the delivery vars by living under the same finally.

I also added the test you suggested: two real run_job calls, each dispatched in its own context the way the ticker does, asserting each turn sees cron_mode while neither leaves the marker set in a sibling context or the base thread. That drives the raw ContextVar isolation through the real run_job path instead of the synthetic set() the previous isolation test used.

Left the _VAR_MAP marker, the reader routing, and the per-job copy_context() dispatch as they were, since those were the parts you confirmed complete.

[egilewski]

suggesting changes

I reviewed a run-owned patch replay of this change against current GitHub main because the submitted branch currently conflicts in tools/approval.py; that replay keeps current-main approval behavior and does not prove the submitted branch itself merges cleanly.

The cron marker fix still leaves the advertised os.environ fallback broken after a cron job has run in the same process. run_job() clears the new HERMES_CRON_SESSION ContextVar by setting it to "", but gateway.session_context.get_session_env() treats any explicit ContextVar value, including "", as authoritative and does not fall back to os.environ. After any earlier cron test/job leaves that empty value in the context, a later standalone/env-marked cron path with HERMES_CRON_SESSION=1 is misclassified as non-cron and dangerous commands are auto-approved instead of blocked under cron_mode: deny.

Blocking check:

python -m pytest -p no:cacheprovider tests/cron/ tests/tools/test_cron_approval_mode.py::TestCronDenyMode::test_dangerous_command_blocked_in_cron_deny_mode -q

That fails with assert not True because check_dangerous_command("rm -rf /tmp/stuff", "local") returns approved after tests/cron/ has exercised run_job(). The broader affected command also fails with 14 cron/approval regressions:

python -m pytest -p no:cacheprovider tests/cron/ tests/tools/test_approval.py tests/tools/test_hardline_blocklist.py tests/tools/test_cron_approval_mode.py tests/tools/test_execute_code_approval_cluster.py -q

Security evidence:

• trust boundary: the cron scheduler marker decides whether the approval gate applies cron policy or interactive gateway policy.
• source/sink/invariant: cron.scheduler.run_job() must scope the marker to the job without breaking tools.approval._is_cron_session() env fallback for standalone cron/test contexts.
• current-main reproduction: a module-path-asserting probe on current main shows run_job() leaves os.environ["HERMES_CRON_SESSION"] == "1" and a later bound gateway session is misclassified as non-gateway.
• PR-head or patch-replay validation: the patch replay fixes that leak (env_after is None, gateway context remains true), but the same replay fails the env-fallback sequence above because the cleared ContextVar shadows os.environ.
• positive/negative cases: the new marker-isolation tests pass, and the focused approval/hardline suites pass after preserving current-main conflict hunks; the ordered cron-to-env fallback test fails.
• residual bypass search: I checked all four cron marker readers through _is_cron_session() and reduced the remaining failure to the ContextVar clear/fallback interaction.
• reviewer validation: CodeRabbit completed on the replay diff with no findings; I reproduced the blocker locally with the reduced pytest command above.

Please clear the cron marker back to the _UNSET/token-reset state, or otherwise make _is_cron_session() fall back to os.environ after run_job() clears the per-job marker, then rerun the ordered cron/approval command.

Signed: GPT-5.5-xhigh in Codex

[AmirF194]

Thanks for moving the .set inside the try and adding the two-run_job isolation test, both look right.

@egilewski's blocking find is real, I reproduced it in a clean Python 3.11 container against the current head. Running his command as a single pytest process:

python -m pytest -p no:cacheprovider tests/cron/ tests/tools/test_cron_approval_mode.py::TestCronDenyMode::test_dangerous_command_blocked_in_cron_deny_mode -q

fails with the dangerous command auto-approved:

AUTO-APPROVED dangerous command in non-interactive non-gateway context (pattern: delete in root path): rm -rf /tmp/stuff
1 failed, 624 passed

The mechanism is the reset value. run_job's finally does _VAR_MAP["HERMES_CRON_SESSION"].set(""), and get_session_env returns any explicitly-set ContextVar value including "" before it falls back to os.environ (session_context.py:318-324). So once a job has run in a shared context, a later standalone or env-marked cron read sees "" and is misclassified as non-cron, and under cron_mode: deny the command is approved instead of blocked.

One thing worth flagging: this is invisible to scripts/run_tests.sh because it runs each file in its own subprocess, so the marker-isolation tests and CI stay green. It only shows up in a single-process run like the one above, which is why the new isolation test does not catch it.

Resetting to _UNSET instead of "" restores the fallback and fixes it. I ran it locally:

from gateway.session_context import ..., _UNSET
_VAR_MAP["HERMES_CRON_SESSION"].set(_UNSET)

With that change the same single-process command plus tests/cron/test_cron_session_marker_isolation.py goes to 625 passed, so it closes the hole without regressing the isolation the PR added. A var.set(token) / var.reset(token) pair around the per-job set would work equally well. Happy to see this land once the reset is switched off the empty string.

I should be straight that my earlier read missed this. I verified the two core tests fail-first but ran them through the per-file runner, which masked exactly this cross-context interaction. Good catch.

[Adolanium]

Confirmed and fixed, thanks both. The reset value was the bug: the finally cleared the marker with set(""), and get_session_env treats any explicitly-set ContextVar value, including "", as authoritative with no fallback to os.environ. So after the first run_job in a context, an env-marked cron read (HERMES_CRON_SESSION=1) was misclassified as non-cron and a dangerous command was auto-approved instead of blocked under cron_mode: deny. The production ticker confines this to its per-job copy_context(), but any caller that runs run_job directly in its own context hit it, which is exactly what the single-process ordered run exposes.

Went with the token pair as suggested: run_job now keeps the token from .set("1") and the finally does .reset(token), restoring the true pre-job state instead of pinning an explicit value. Added a regression test that drives a real run_job and then asserts the env fallback still classifies an env-marked read as cron. It fails on the previous head with exactly that assertion and passes with the reset.

Also rebased onto current main, which resolves the tools/approval.py conflict. The #58698 refactor extracted the cron branch into _run_approval_gate, so the reader there is now routed through _is_cron_session() along with the other three.

On the broader ordered command: your blocking check now passes, and while chasing the remaining failures I found the TestApprovalTimeoutIsNotConsent ones are pre-existing on an unmodified current main. tests/cron/test_scheduler.py followed by that class fails the same 3 tests in one process without this PR: run_job's clear_session_vars finally intentionally pins every session var to an explicit "" (the gateway relies on that to suppress the env fallback), so the timeout tests stop resolving their session key from os.environ and never find their registered gateway callback. Since this PR adds more direct run_job tests to the same suite, I fixed it here with a shared autouse fixture in tests/cron/conftest.py that resets every session-context var to its _UNSET default around each test, making the cron suite order-independent.

Local runs on the new head: your reduced blocking command passes, and the full ordered command (tests/cron/ + test_approval.py + test_hardline_blocklist.py + test_cron_approval_mode.py + test_execute_code_approval_cluster.py in one process) is 1146 passed. The only remaining failures in my local run are Windows-only file-permission and tilde-expansion tests that fail identically on an unmodified tree. Isolation file is 7 passed, ruff clean.

@AmirF194 thanks for the independent repro and for flagging that the per-file runner masks this class entirely. That also explains how it got past the original fail-first runs.

[adambiggs]

Confirmed this exact failure in a live in-process Telegram gateway: after cron ran, an interactive DM's execute_code call was rejected with the cron-only 'no user present' message. I deployed this PR's commit to that gateway; the same interactive guard now returns pending_approval and routes to the normal interactive approval path. The focused cron/approval suite passes (298 tests).

[teknium1]

Thanks for preserving the cron policy while moving the marker off process-global state. The underlying bug remains present on current main: cron/scheduler.py:2812 writes a persistent process-global marker, and the gateway starts the default in-process ticker at gateway/run.py:21110-21128.

Problems

• tools/approval.py:2041 changes the shared gate to _is_cron_session(), but tests/tools/test_request_tool_approval.py:103-120 still monkeypatches env_var_enabled to simulate cron. That stub no longer controls the branch: _is_cron_session() reads session context / os.environ. The deny case therefore reaches the non-cron fail-closed path, and the approve case does not establish cron mode.

Suggested changes

• Update those tests to set/reset _VAR_MAP["HERMES_CRON_SESSION"] or mock _is_cron_session() directly; update the non-cron case at tests/tools/test_request_tool_approval.py:148-155 to use the same seam.

This is an automated hermes-sweeper review.

[teknium1]

The existing shared-gate cron tests at tests/tools/test_request_tool_approval.py:103-120 only monkeypatch env_var_enabled. After this replacement, that stub no longer establishes a cron context because _is_cron_session() reads session context/env directly. Update those tests to set/reset _VAR_MAP["HERMES_CRON_SESSION"] or mock _is_cron_session(); otherwise the deny/approve cases exercise non-cron behavior.

[Adolanium]

Good catch. The gate now keys off _is_cron_session(), so stubbing env_var_enabled("HERMES_CRON_SESSION") no longer put those cases on the cron branch (deny hit fail-closed, approve never got cron_mode).

I updated tests/tools/test_request_tool_approval.py and squashed it into the PR commit (559be6bdd):

• test_cron_deny_mode_blocks / test_cron_approve_mode_allows mock _is_cron_session to True
• test_no_human_non_cron_fails_closed mocks _is_cron_session to False

Full file is 13 passed.

[Adolanium]

Addressed the hermes-sweeper review (review body + inline on tools/approval.py).

What was wrong
The shared gate at _run_approval_gate now uses _is_cron_session(), but tests/tools/test_request_tool_approval.py still monkeypatched env_var_enabled to fake cron. That stub no longer controls the branch, so the deny case landed on non-cron fail-closed and the approve case never entered cron mode.

What I did
Squashed into a single commit on the branch (559be6bdd):

• cron deny/approve cases: monkeypatch.setattr(approval, "_is_cron_session", lambda: True)
• non-cron fail-closed case: same seam set to False
• left production code unchanged
• force-pushed so the PR stays one commit

Checks

• tests/tools/test_request_tool_approval.py: 13 passed
• ruff on that file: clean

[egilewski]

suggesting changes

Please rebase this change and update the new cron regression-test mock for the current runtime-provider interface. On a patch replay onto current main, tests/cron/test_cron_session_marker_isolation.py fails four tests before reaching the cron-marker assertions because _patch_agent_bootstrap() replaces resolve_runtime_provider with lambda requested=None, but run_job() now calls it with target_model=...:

TypeError: ... <lambda>() got an unexpected keyword argument 'target_model'

Make the stub accept the current keyword arguments (while retaining the provider fixture), then rerun the cron-marker coverage. The ContextVar approach itself is directionally appropriate, but the submitted test suite does not pass against current main.

Security evidence:

• trust boundary: HERMES_CRON_SESSION selects the cron approval policy.
• source/sink/invariant: the per-job marker must not leak, and its regression tests must execute against the current run_job() provider interface.
• current-main reproduction: replayed the declared one-commit PR delta onto current GitHub main a41d280f95.
• PR-head or patch-replay validation: the replay applied cleanly; the focused suite collected 20 tests and failed 4 new cron-marker tests before their assertions.
• positive/negative cases: 16 tests passed, including the existing approval cases; the four failures consistently raise the same incompatible-mock TypeError.
• residual bypass search: git diff --check passed; this is a test harness interface mismatch, not patch whitespace or application failure.
• reviewer validation: PR fix(security): scope the cron-session approval marker to the job context #58663 remained open and non-draft at head 559be6bdd758323582cbebd6f819c4b2e772d9ab during validation.

Signed: GPT-5.6-sol-xhigh in Codex

[Adolanium]

Addressed @egilewski's latest review.

On a patch replay onto current main the four new run_job-driven isolation tests never reached their marker assertions. _patch_agent_bootstrap() stubbed resolve_runtime_provider as lambda requested=None, but run_job() now always passes target_model=... (and can pass other kwargs). That raised:

TypeError: ... <lambda>() got an unexpected keyword argument 'target_model'

Changes in this push:

1. Rebased onto current main (477c08b44).
2. Stub is now lambda requested=None, **kwargs: {...} so it accepts the current provider kwargs (target_model, explicit_base_url, explicit_api_key, etc.) and still returns the fixed provider fixture.
3. Added test_marker_cleared_even_when_agent_raises - mid-tick exception still unwinds the marker via finally / reset(token).
4. Hardened _is_cron_session() so a leftover process-global HERMES_CRON_SESSION=1 cannot reclassify a bound interactive gateway session as cron when the per-job ContextVar was never set for that turn. Env fallback still works for standalone hermes cron and tests with no live gateway identity.
5. Policy-path coverage: ContextVar-bound cron + cron_mode=deny hard-blocks dangerous terminal commands and execute_code; stale env + bound gateway stays on approval_required.
6. TestCronWithGatewayOrigin binds the per-job ContextVar (the production marker) instead of the process env, so co-bound platform routing still hits cron_mode.

Approach otherwise unchanged: marker in _VAR_MAP so it rides _inject_session_context_env, run_job uses set + reset(token) rather than set("") so the env fallback survives after a job in the same context, and all four approval readers go through _is_cron_session().

Checks:

• isolation + request/cron/execute_code approval suites: 77 passed
• ruff on the touched files: clean

Head: 234b775ab

[egilewski]

looks mergeable

Security evidence:

• trust boundary: an in-process cron ticker shares its process with interactive gateway approval sessions.
• source/sink/invariant: the cron marker is bound in the job ContextVar, inherited by the copied worker context, then reset to its prior state in finally.
• current-main reproduction: the prior process-global marker could persist after a cron job.
• PR-head or patch-replay validation: the approval lookup prefers a bound marker but retains standalone environment fallback.
• positive/negative cases: focused tests cover normal completion, exceptions, concurrent contexts, gateway-origin cron jobs, and approval modes.
• residual bypass search: a residual process environment marker cannot override an active gateway session.
• reviewer validation: CodeRabbit completed with no findings in the clean-pass flow.

Signed: GPT-5.6-sol-xhigh in Codex

[im47cn]

Two enhancements from a duplicate PR (#69766) that diagnosed this bug in production (Feishu gateway).

1. Defense-in-depth: gate the cron flag in set_session_vars()

In set_session_vars(), explicitly set the cron flag to "":

_CRON_SESSION_FLAG.set("")

Why: Every set_session_vars() call is binding a non-cron session (gateway, TUI, CLI, API server). Setting the flag to "" suppresses the os.environ fallback. Even if another process leaked HERMES_CRON_SESSION=1 into the environment, a gateway session that binds its contextvars can never be miscategorised as cron.

Without this, the os.environ fallback in _is_cron_session() is the last line of defense — if something sets the env var, the bug returns.

2. Feishu reply-to-cron-message scenario test

We hit this bug in production when a Feishu user replied to a cron-delivered message card. The exact reproduction:

1. Cron job fires → delivers report card to Feishu group chat
2. User replies to the thread with a follow-up task
3. execute_code blocked → agent falls back to terminal call-by-call

A test case (tests/gateway/test_cron_session_contextvar.py, 19 tests) covers this end-to-end:

• HERMES_CRON_SESSION=1 leaked in os.environ
• HERMES_GATEWAY_SESSION=1 set
• Gateway handler calls reset_session_vars() → set_session_vars()
• is_cron_session() → False
• check_execute_code_guard() → approved (not cron-blocked)

Happy to contribute these as a PR against this branch if you prefer.

[Adolanium]

@im47cn thanks for bringing the Feishu production case over. I checked both suggestions against the current head.

The extra set_session_vars() assignment should not be needed now. _is_cron_session() already reads the per-job ContextVar first, then suppresses the os.environ fallback whenever a gateway platform or HERMES_GATEWAY_SESSION is bound. In the sequence you described, set_session_vars(platform="feishu", ...) binds the platform, so a stale HERMES_CRON_SESSION=1 in the process environment is ignored and the turn is classified as interactive.

I would rather keep that protection in _is_cron_session() than set the cron flag to "" in every set_session_vars() call. set_session_vars() is also used by run_job(), so it is not a safe general contract that every call represents a non-cron context. An unconditional empty value could erase a real cron marker if the call order changes or a session is bound inside an existing cron context.

The current regression coverage also has most of the Feishu scenario already. test_stale_process_env_does_not_reclassify_bound_gateway_session leaks HERMES_CRON_SESSION=1, binds an interactive gateway session, checks that _is_cron_session() is false, confirms gateway approval context remains active, and verifies a dangerous command reaches approval_required instead of cron deny. test_cron_contextvar_drives_deny_for_dangerous_and_execute_code separately checks that execute_code is blocked when the real cron ContextVar is bound.

The one combination not asserted directly is execute_code under the stale-env interactive gateway case. That would be useful focused coverage, but it does not require a production change or a Feishu-specific test file because this classification path is platform-independent. I am going to leave the production code as it is. A small assertion for that remaining combination would still be welcome.

[Adolanium]

Pulled in @im47cn's focused Feishu execute_code regression test from #69822 as commit 12ce3cbe5. The commit remains separate and keeps their authorship.

I added a small follow-up in 12c7fa4c7 to pin manual approval mode and assert the exact interactive result: approved is false, approval_pending is true, and status is pending_approval. This makes the test prove that the request reached the normal gateway approval path rather than only proving it avoided cron deny.

Checks:

• tests/cron/test_cron_session_marker_isolation.py: 12 passed
• Ruff and git diff --check: clean