Skip to content

fix(cron): restore HERMES_CRON_SESSION after jobs - #43549

Closed
Alexevann wants to merge 1 commit into
NousResearch:mainfrom
Alexevann:fix/cron-session-env-pollution
Closed

fix(cron): restore HERMES_CRON_SESSION after jobs#43549
Alexevann wants to merge 1 commit into
NousResearch:mainfrom
Alexevann:fix/cron-session-env-pollution

Conversation

@Alexevann

Copy link
Copy Markdown

What does this PR do?

The cron scheduler runs in-process inside the gateway. Setting HERMES_CRON_SESSION=1 process-wide without restoring the prior value leaks cron approval context into later interactive gateway sessions, which can incorrectly block tools such as execute_code.

This PR saves the prior value before each job and restores it in finally:

  • unsets HERMES_CRON_SESSION if it was not previously set
  • restores the previous value if it was

Related Issue

Fixes #37968

Related: #35515 #43370

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

  • cron/scheduler.py: save and restore HERMES_CRON_SESSION around each cron job
  • tests/cron/test_scheduler.py: add regression tests for unset and pre-existing env values

How to Test

  1. uv run --extra dev pytest tests/cron/test_scheduler.py::TestRunJobSessionPersistence -q

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: macOS 26.5.1

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • 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
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

The cron scheduler runs in-process inside the gateway. Setting
HERMES_CRON_SESSION=1 process-wide without restoring the prior value
leaks cron approval context into later interactive gateway sessions,
which can incorrectly block tools such as execute_code.

Save the prior value before each job and restore it in finally:
- unset HERMES_CRON_SESSION if it was not previously set
- restore the previous value if it was

Add regression tests covering both the unset and pre-existing cases.

Fixes NousResearch#37968
Related: NousResearch#35515 NousResearch#43370
@liuhao1024

Copy link
Copy Markdown
Contributor

✅ Verified — cron session env var lifecycle

Reviewed cron/scheduler.py diff for HERMES_CRON_SESSION env var management.

  • Save/restore pattern: Confirmed _prior_cron_session = os.environ.get("HERMES_CRON_SESSION", "_UNSET_") captured before overwrite at L1580, restored in the finally block at L2012-2015. Follows the same sentinel pattern as the existing TERMINAL_CWD restore.
  • Gateway isolation: The fix correctly prevents a cron job's HERMES_CRON_SESSION=1 from persisting into subsequent interactive gateway sessions running in the same process.
  • Test coverage: Two regression tests — one for "unset before job" (verifies cleanup), one for "pre-existing value" (verifies restore). Both exercise run_job() end-to-end.

The fix is correct and complete. No issues found.

@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists duplicate This issue or pull request already exists labels Jun 10, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #35515 — identical fix (save/restore HERMES_CRON_SESSION around each cron job, same files cron/scheduler.py + tests/cron/test_scheduler.py). #35515 is the earlier open PR.

@liuhao1024

Copy link
Copy Markdown
Contributor

Verification

Reviewed the diff — this is a clean fix for a real environment-variable leak bug.

Root cause: os.environ["HERMES_CRON_SESSION"] = "1" was set at the start of _run_job_impl but never restored in finally. Since the gateway runs the scheduler in-process, the HERMES_CRON_SESSION=1 value persisted into subsequent interactive gateway sessions, causing execute_code to be blocked by cron approval policy.

Fix: saves _prior_cron_session before overwriting, restores in finally. Uses a "_UNSET_" sentinel to distinguish "was not set" from "was set to empty string". The pattern matches the existing TERMINAL_CWD save/restore in the same block.

Tests: two new tests — one verifying cleanup when the var was unset, one verifying restoration when it was set to a different value. Both are clean and cover the two branches.

No issues found.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for targeting a real approval-context leak. The premise remains live on current main: run_job writes HERMES_CRON_SESSION at cron/scheduler.py:2685-2688, and execute_code applies cron restrictions from that flag at tools/approval.py:3019-3035.

Problems

  • Per-job save/restore of a process-global flag is not safe with supported concurrent cron execution. cron/scheduler.py:293-300 defines the persistent parallel pool, and cron/scheduler.py:3651,3725-3728 dispatches workdir-less jobs through it. Overlapping jobs can restore snapshots out of order, clearing the flag during another cron run or leaving "1" behind after both finish. The added serial run_job() tests do not exercise that mode.
  • The proposed "_UNSET_" string sentinel cannot restore a pre-existing environment value equal to that literal.

Suggested changes

  • Use task-local cron state (the existing ContextVar pattern documented in gateway/session_context.py:20-22) and have approval checks read that scoped state rather than a process-global marker.
  • Add a concurrent-tick regression test covering two cron jobs and a later interactive context.

Automated hermes-sweeper review.

@alt-glitch alt-glitch added the sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state label Jul 14, 2026
@teknium1 teknium1 added 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-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Merged via #77022. Your PR's approach (save/restore the env var in finally) was a valid minimal fix, but the merged version uses a per-session ContextVar approach for stronger isolation that also covers the gateway/API/TUI entry points. Thank you for contributing!

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 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.

fix(cron): isolate gateway approvals from environment pollution

5 participants