fix(cron): restore cron session env after jobs

[ckellum]

Summary\n- restore HERMES_CRON_SESSION after each cron job instead of leaking it into the gateway process\n- add regression coverage for unset and pre-existing cron session env values\n\n## Why\nGateway cron ticks run in-process. Leaving HERMES_CRON_SESSION=1 set can make later live Telegram/API sessions inherit cron approval behavior, which can incorrectly block interactive tools such as execute_code.\n\n## Tests\n- python -m pytest tests/cron/test_scheduler.py::TestRunJobSessionPersistence::test_run_job_restores_cron_session_env_after_success tests/cron/test_scheduler.py::TestRunJobSessionPersistence::test_run_job_restores_preexisting_cron_session_env tests/tools/test_cron_approval_mode.py tests/tools/test_execute_code_approval_cluster.py -q\n\n## Cost impact\nNone. Local process env restoration only; no GCP/Cloud Run changes.

[rodriguez46p-ui]

Hermes hourly review

Automated check in an isolated worktree passed:

python -m pytest tests/cron/test_scheduler.py::TestRunJobSessionPersistence::test_run_job_restores_cron_session_env_after_success tests/cron/test_scheduler.py::TestRunJobSessionPersistence::test_run_job_restores_preexisting_cron_session_env tests/tools/test_cron_approval_mode.py tests/tools/test_execute_code_approval_cluster.py -q -o addopts=
43 passed, 1 warning in 5.86s

One small robustness suggestion before merge: avoid using the string sentinel _UNSET_ for _prior_cron_session. If an outer process ever legitimately has HERMES_CRON_SESSION=_UNSET_, the finally block will delete it instead of restoring it. A boolean like _had_cron_session = "HERMES_CRON_SESSION" in os.environ plus _prior_cron_session = os.environ.get("HERMES_CRON_SESSION") would make the restore exact.

No other blockers found in this focused review.

[alt-glitch]

Likely duplicate of #29854 — both fix HERMES_CRON_SESSION env var leaking from cron ticks into subsequent interactive sessions by saving/restoring the env var around job execution. See also #31184 (ContextVar-based approach) and #4262 (original issue, duped to #10769).

[tonydwb]

Code Review Summary

Verdict: Approved

PR: fix(cron): restore cron session env after jobs
Author: ckellum
Files changed: 2 (+79 / -2)

What this PR does

Restores HERMES_CRON_SESSION to its prior value (or removes it) after each cron job finishes, instead of leaving HERMES_CRON_SESSION=1 permanently set in the process environment. When the gateway runs cron ticks in-process alongside live Telegram/API sessions, the leaked env var was causing interactive tools (e.g., execute_code) to inherit cron approval semantics and block valid user commands.

Checklist

• Correctness: The _prior_cron_session sentinel pattern (_UNSET_ / actual value) mirrors the existing TERMINAL_CWD restoration pattern. The env var is saved before mutation and restored in finally, guaranteeing execution even on exceptions. ✓
• Security: No tokens, keys, or secrets in the diff. Environment variable handling is standard os.environ mutation. ✓
• Code Quality: Clean, minimal change (12 lines in cron/scheduler.py). Comments explain the why. Follows existing code conventions in the same function (sentinel pattern, finally block placement, ContextVar cleanup area). ✓
• Testing: Two well-written regression tests added in tests/cron/test_scheduler.py (+67 lines):
  • test_run_job_restores_cron_session_env_after_success — verifies the var is removed entirely when it wasn't set before the job
  • test_run_job_restores_preexisting_cron_session_env — verifies the original value is restored when it was set before the job
    Both use monkeypatch + the standard test patching pattern used by all other tests in this file. ✓
• Performance: Negligible — os.environ.get(), os.environ.pop(), and os.environ.__setitem__() are O(1) dict operations. ✓
• Documentation: Inline comments are clear and reference the specific problem (live sessions inheriting cron approval semantics). No README/docs changes needed — this is an internal implementation detail. ✓

Red flag check

Red flag	Status
Hardcoded secrets / API keys	✅ None
Bare except: pass	✅ None
Test rewrites (modifying existing tests)	✅ All new tests — no regression regressions
Broad except BaseException	✅ None
Unsafe eval/exec/pickle	✅ None
Thread safety concerns	✅ os.environ is process-global but the cron scheduler runs jobs sequentially within one tick, and the sentinel pattern is safe

Summary

A clean, focused fix for a real bug (env var leak causing incorrect tool approval behavior across cron ticks). The implementation is minimal, follows established patterns in the same file, and is well-covered by two explicit regression tests. Approved.

---

Reviewed by Hermes Agent

[liuhao1024]

Verified: this fix is correct and well-scoped.

The bug is real — when the gateway runs cron ticks in-process alongside live sessions, HERMES_CRON_SESSION=1 leaks into subsequent live sessions because it's a process-wide env var set once and never cleaned up. This causes execute_code and other user-present tools to be blocked with cron approval semantics.

The sentinel pattern (_UNSET_) correctly distinguishes "env var was absent before this job" from "env var was set to some prior value", matching the existing TERMINAL_CWD restoration pattern at lines 1821-1824. The try/finally ensures cleanup runs on both success and failure paths.

Test coverage: both scenarios (unset-before → removed after, preexisting-value → restored after) are covered. The test for the unset case asserts not in os.environ rather than checking for empty string — correct, since os.environ.pop removes the key entirely.

No issues found.

[tonydwb]

Review: fix(cron): restore cron session env after jobs

PR #35515 | Author: ckellum | Branch: fix/cron-session-env-leak → main

Summary

This PR fixes a subtle environment-variable leak in the cron scheduler. In gateway mode where cron ticks run in-process alongside live Telegram/API sessions, HERMES_CRON_SESSION=1 was being set process-wide and never unset after job completion. This caused subsequent live sessions to incorrectly inherit cron approval semantics, blocking interactive tools like execute_code that check for cron mode.

Changes

File	Additions	Deletions
cron/scheduler.py	12	2
tests/cron/test_scheduler.py	67	0

Total: +79 / -2

Code Review

Correctness ✅

• The fix follows the established _SET_ / _UNSET_ sentinel pattern already used in the same finally block for TERMINAL_CWD restoration (lines 1819-1823). This is consistent and proven.
• The sentinel "_UNSET_" avoids ambiguity with a legitimate env var value of empty string.
• Properly handles both cases: unsetting when no prior value existed, and restoring the prior value when one did (e.g., nested cron contexts).
• The save happens at the top of _run_job_impl (line 1404) before any other cron logic runs, and the restore happens at the end of the finally block — guaranteeing execution regardless of success, failure, or exception.

Security ✅

• No security concerns. This is a purely local env var manipulation that prevents a behavioral leak (cron approval mode spilling into user sessions).

Code Quality 💯

• Excellent code quality. The rationale is clearly documented in the updated comment (lines 1399-1403).
• The implementation mirrors existing patterns in the same function, making it easy to review and maintain.
• The variable name _prior_cron_session is descriptive and follows the naming conventions of nearby code (_prior_terminal_cwd).

Testing ✅

• Two new regression tests are added in the TestRunJobSessionPersistence class:
  1. test_run_job_restores_cron_session_env_after_success — verifies the env var is absent after a cron job when it was not set before.
  2. test_run_job_restores_preexisting_cron_session_env — verifies the original value is restored when HERMES_CRON_SESSION was already set.
• Both tests use the same mocking boilerplate as existing tests in the same class.
• The tests run against monkeypatch-controlled environment, making them deterministic.
• Test coverage for the failure/exception path is implicitly covered by existing tests (e.g., test_run_job_closes_agent_on_failure_to_prevent_fd_leak) since the restore is in the finally block.

Performance 🟢

• Zero cost: two os.environ get/set/pop calls per job. The existing TERMINAL_CWD restore already does the same work.

Verdict

This is a clean, well-scoped bug fix with appropriate regression tests. The implementation is correct, follows established patterns, and addresses a real behavioral issue (cron approval leaking into user sessions). No issues found.

Decision: APPROVE

[robinbraemer]

Independent live reproduction on a Matrix gateway as well.

Evidence pattern from the gateway logs:

inbound message: platform=matrix user=<redacted> ...
agent.turn_context: ... platform=matrix ...
Tool execute_code returned error: BLOCKED: execute_code runs arbitrary local Python ... Cron jobs run without a user present to approve it

This was a normal user-present Matrix turn, not a cron job. The process had previously run cron activity, so the leaked process-wide HERMES_CRON_SESSION=1 made the approval layer route the interactive turn through cron policy. The model then fell back to a terminal Python heredoc, which is exactly the bad UX/security shape this fix prevents.

I applied the same save/restore patch locally on a live gateway and restarted the service. Verification:

uv run --extra dev python -m pytest tests/cron/test_cron_script.py::TestRunJobEnvVarCleanup -q
3 passed

uv run --extra dev python -m pytest tests/tools/test_cron_approval_mode.py tests/tools/test_execute_code_approval_cluster.py -q
44 passed

execute_code smoke: print(40 + 2) -> 42

So this PR is the right minimal fix for the immediate gateway poisoning. A ContextVar refactor can still be a longer-term cleanup, but save/restore fixes the live failure without changing cron approval semantics.

[teknium1]

Thanks for targeting a real gateway-poisoning bug. Current main still sets HERMES_CRON_SESSION process-wide at cron/scheduler.py:2688, and tools/approval.py:194 gives that marker precedence over gateway context.

Problems

• The proposed per-job restore is unsafe with current concurrency. Workdir-less jobs are dispatched through the parallel pool at cron/scheduler.py:3725, and tests/cron/test_scheduler.py:3089 verifies overlap. If two jobs both snapshot an absent marker, the first job to finish removes it while the other is still running, changing that peer's approval behavior.
• _UNSET_ is not an exact absence sentinel: an ambient value literally equal to _UNSET_ would be removed rather than restored.

Suggested changes

• Use a lock-protected reference-counted marker lease: first entrant snapshots ambient state; final exiting job restores it. Add a two-thread regression that proves the marker remains set after one overlapping job exits, then restores the original value after the second.

This is an automated hermes-sweeper review.

[teknium1]

This per-job snapshot races with current workdir-less cron concurrency: two jobs can both observe an absent marker, then the first exiting job removes it while the other is still running. Use a shared lock/reference-counted lease so only the final active cron job restores the ambient value.

[alt-glitch]

This was generated by AI during triage.

Duplicate of open #43549: both restore the process-wide HERMES_CRON_SESSION marker after a job. #29854 remains related because it additionally addresses concurrent-job lifetime.