Skip to content

fix(cron): isolate approval context per job - #76749

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

fix(cron): isolate approval context per job#76749
brendanlees wants to merge 1 commit into
NousResearch:mainfrom
brendanlees:fix/cron-session-approval-context

Conversation

@brendanlees

Copy link
Copy Markdown

What does this PR do?

Scopes cron approval identity to the active cron task instead of setting process-global HERMES_CRON_SESSION=1 forever.

The gateway and cron scheduler share a process. After the first agent-backed cron run, the leaked flag caused later Discord MCP elicitations to be treated as CLI/cron approvals. They fell through to a terminal prompt with no stdin and failed closed as denied instead of rendering Discord approval controls.

The new task-local context preserves cron policy during the cron run, including worker threads and subprocess environment construction, while preventing completed or concurrent cron runs from changing live gateway approval routing. Legacy process-env callers still work through get_session_env() fallback.

Related Issue

Fixes #76748

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

  • Bind cron identity through gateway.session_context per task.
  • Make scheduler jobs opt into task-local cron context without mutating os.environ.
  • Route approval and bang-shell cron checks through context-local state with legacy env fallback.
  • Add scheduler, gateway, and MCP elicitation regression coverage for the observed failure.

How to Test

  1. Run an agent-backed cron job inside the gateway process.
  2. From a later Discord turn, invoke a form-eliciting MCP tool.
  3. Confirm the elicitation uses the Discord approval surface rather than terminal input.
  4. Confirm cron-originated approval checks still follow approvals.cron_mode.

Commands run:

scripts/run_tests.sh \
  tests/tools/test_cron_approval_mode.py \
  tests/tools/test_mcp_elicitation.py \
  tests/tools/test_request_tool_approval.py \
  tests/cron/test_cron_approval_context.py \
  tests/cli/test_bang_shell_mode.py \
  tests/gateway/test_session_context_inheritance.py \
  tests/tools/test_local_env_session_leak.py

scripts/run_tests.sh \
  tests/tools/test_execute_code_approval_cluster.py \
  tests/tools/test_hardline_blocklist.py \
  tests/tools/test_denial_circuit_breaker.py \
  tests/cron/test_sessiondb_init_hang.py \
  tests/cron/test_cron_provider_pin.py \
  tests/cron/test_codex_execution_paths.py

ruff check cron/scheduler.py gateway/session_context.py tools/approval.py \
  hermes_cli/bang_shell.py tests/tools/test_cron_approval_mode.py \
  tests/tools/test_request_tool_approval.py tests/tools/test_mcp_elicitation.py \
  tests/cron/test_cron_approval_context.py tests/cli/test_bang_shell_mode.py

python scripts/check-windows-footguns.py --diff origin/main

Results: 325 targeted tests passed, Ruff passed, and no Windows footguns were found.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits
  • I searched existing open and closed PRs/issues for duplicates
  • My PR contains only changes related to this fix
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes
  • I've tested on macOS 15 and a Linux production gateway

Documentation & Housekeeping

  • Documentation changes are N/A; behavior and concurrency rationale are documented in code
  • cli-config.yaml.example changes are N/A
  • CONTRIBUTING.md and AGENTS.md changes are N/A
  • Cross-platform impact was checked with check-windows-footguns.py
  • Tool description/schema changes are N/A

Screenshots / Logs

Before the fix, the live Discord request was routed to the gateway process terminal and failed closed:

Approval requested by MCP server 'soul_broker'.
Choice [o/s/D]: Denied

Both the scheduler lifecycle test and MCP context-bridge test cover this routing regression.

@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 approval-routing leak: current main still sets HERMES_CRON_SESSION process-wide in cron/scheduler.py:3014, and tools/approval.py:241-245 lets that stale flag override later gateway classification.

Problems

  • gateway/session_context.py:138 adds HERMES_CRON_SESSION to _VAR_MAP, which makes it a bridged child-process variable (tools/environments/local.py:445-453). But the shared shell snapshot exclusion is explicitly required to stay synchronized with _VAR_MAP (tools/environments/base.py:399-402) and currently only excludes HERMES_SESSION_*, HERMES_UI_SESSION_ID, and HERMES_CRON_AUTO_DELIVER_* (tools/environments/base.py:403-437). The new cron identity can therefore be persisted into the shared snapshot.

Suggested changes

  • Exclude HERMES_CRON_SESSION in both snapshot filtering paths in tools/environments/base.py, and add a snapshot regression test alongside tests/tools/test_snapshot_multiline_session_env_injection.py.

Automated hermes-sweeper review.

@@ -135,6 +136,7 @@ def session_context_engaged() -> bool:
"HERMES_UI_SESSION_ID": _SESSION_UI_SESSION_ID,
"HERMES_SESSION_MESSAGE_ID": _SESSION_MESSAGE_ID,
"HERMES_SESSION_PROFILE": _SESSION_PROFILE,

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 bridged name also requires updating the shared-shell snapshot exclusion in tools/environments/base.py: its _VAR_MAP synchronization contract currently excludes only HERMES_SESSION_*, HERMES_UI_SESSION_ID, and HERMES_CRON_AUTO_DELIVER_*. Otherwise a cron terminal child can persist HERMES_CRON_SESSION=1 in the shared snapshot.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists needs-decision Awaiting maintainer decision before any implementation comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery comp/cli CLI entry point, hermes_cli/, setup wizard comp/tools Tool registry, model_tools, toolsets 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 labels Aug 2, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: #43370 and #58663 address the same cron approval-marker leak. This patch covers scheduler, bang-shell, and MCP elicitation; #43370 also explicitly masks stale cron state across gateway, API, ACP, and TUI entrypoints. Maintainer decision needed on the preferred coverage/mechanism.

@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 labels Aug 2, 2026
@GottZ

GottZ commented Aug 3, 2026

Copy link
Copy Markdown

This was generated by AI during triage.

Summary

Thirty-nine PRs address or reference this approval-system complex: most cron fixes target the process-global HERMES_CRON_SESSION leak, while the remaining diffs cover plugin-driven tool approval, unattended-policy ordering, file/skill approvals, smart-prompt context, and adjacent hardening. The cron root cause is already fixed on main by #77022; the other subproblems require separate dispositions rather than treating every PR as interchangeable.

Related pull requests

Duplicates

The cron-leak chain is #35515/#43549/#46167/#57124/#59242/#60386/#60611/#76763 for env restoration and #29854/#36004/#37969/#43370/#56796/#58663/#59719/#62111/#64194/#69566/#69766/#70768/#76749 for task-local isolation, all superseded by merged #77022. The plugin chain is #58698#59163#60504, with #60544 duplicating the landed gateway-notify repair; #60547 is superseded by #61064.

Suggested consolidation

Close the remaining cron-leak PRs as already implemented on main by #77022 (commit fb6446f, tests/cron/test_scheduler_cron_session_isolation.py), including #76749 despite its keep_open review and best-fix record because the merged diff contains its mechanism plus the requested shell-snapshot exclusion. Keep #59179, #61064, and #46900 open only on their concrete salvage paths, require author action on #60550, and close the identified duplicate chains.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    I76748(["issue #76748 (open)"])
    P76749["PR #76749 (open)"]
    P76749 -->|best fix| I76748
    class I76748 open
    class P76749 open
    class P76749 best
    class P76749 target
    click I76748 "https://github.com/NousResearch/hermes-agent/issues/76748"
    click P76749 "https://github.com/NousResearch/hermes-agent/pull/76749"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).

Cross-PR triage: Reviewed 39 pull requests and 17 issues in this complex. Each diff was read against this issue; Assessment working set: 700 kB of PR diffs, 142 kB of issue/PR text, 129 kB of discussion (159 comments), 127 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard comp/cron Cron scheduler and job management comp/gateway Gateway runner, session dispatch, delivery comp/tools Tool registry, model_tools, toolsets needs-decision Awaiting maintainer decision before any implementation 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.

Cron session flag leaks into later gateway approvals

4 participants