-
Notifications
You must be signed in to change notification settings - Fork 52.5k
fix(cron): scope lifecycle guard to real shell scripts; per-job cron flag via ContextVar #76797
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
samson-mak
wants to merge
3
commits into
NousResearch:main
Choose a base branch
from
samson-mak:fix/approval-gate-cron-leak
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
f03e785
fix(cron): scope lifecycle guard to real shell scripts; per-job cron …
samson-mak f39ef75
test(approval): isolate execute-code cluster from permanent allowlist
samson-mak 54d173b
fix(cron): NUL-byte text detection in lifecycle guard; worker-handoff…
samson-mak File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,226 @@ | ||
| from pathlib import Path | ||
| import sys | ||
|
|
||
| from cron.lifecycle_guard import ( | ||
| _contains_unsafe_gateway_action, | ||
| contains_gateway_lifecycle_command_or_referenced_script, | ||
| ) | ||
| from gateway.session_context import HERMES_CRON_SESSION_CONTEXTVAR | ||
| from tools import approval | ||
|
|
||
|
|
||
| def test_full_path_non_shell_binary_is_not_scanned(tmp_path: Path) -> None: | ||
| binary = tmp_path / "python3" | ||
| binary.write_bytes(b"\x00" * 4096) | ||
|
|
||
| command = f'{binary} -c "print(1)"' | ||
|
|
||
| assert not contains_gateway_lifecycle_command_or_referenced_script(command, cwd=str(tmp_path)) | ||
| assert not _contains_unsafe_gateway_action( | ||
| command, cwd=str(tmp_path), depth=0, visited=set() | ||
| ) | ||
|
|
||
|
|
||
| def test_extensionless_text_script_without_shebang_is_scanned(tmp_path: Path) -> None: | ||
| script = tmp_path / "restart-helper" | ||
| script.write_text("hermes gateway restart\n") | ||
|
|
||
| assert _contains_unsafe_gateway_action( | ||
| str(script), cwd=".", depth=0, visited=set() | ||
| ) | ||
|
|
||
|
|
||
| def test_shell_script_reference_is_still_scanned(tmp_path: Path) -> None: | ||
| script = tmp_path / "script.sh" | ||
| script.write_text("#!/bin/sh\nhermes gateway restart\n") | ||
|
|
||
| assert contains_gateway_lifecycle_command_or_referenced_script( | ||
| f"./{script.name}", cwd=str(tmp_path) | ||
| ) | ||
|
|
||
|
|
||
| def test_actual_python_binary_is_not_scanned() -> None: | ||
| command = f'{sys.executable} -c "print(1)"' | ||
|
|
||
| assert not _contains_unsafe_gateway_action( | ||
| command, cwd=".", depth=0, visited=set() | ||
| ) | ||
|
|
||
|
|
||
| def test_dot_source_and_source_script_references_are_scanned(tmp_path: Path) -> None: | ||
| script = tmp_path / "evil.sh" | ||
| script.write_text("#!/bin/sh\nhermes gateway restart\n") | ||
|
|
||
| for prefix in (".", "source"): | ||
| assert _contains_unsafe_gateway_action( | ||
| f"{prefix} {script}", cwd=".", depth=0, visited=set() | ||
| ) | ||
|
|
||
|
|
||
| def test_bash_script_reference_is_scanned(tmp_path: Path) -> None: | ||
| script = tmp_path / "evil.sh" | ||
| script.write_text("#!/bin/sh\nhermes gateway restart\n") | ||
|
|
||
| assert _contains_unsafe_gateway_action( | ||
| f"bash {script}", cwd=".", depth=0, visited=set() | ||
| ) | ||
|
|
||
|
|
||
| def test_large_extensionless_shell_script_is_scanned(tmp_path: Path) -> None: | ||
| script = tmp_path / "large-script" | ||
| script.write_text("#!/bin/sh\n" + ("# " + "x" * 5000 + "\n") + "hermes gateway restart\n") | ||
|
|
||
| assert _contains_unsafe_gateway_action( | ||
| str(script), cwd=".", depth=0, visited=set() | ||
| ) | ||
|
|
||
|
|
||
| def test_embedded_null_path_does_not_raise() -> None: | ||
| command = "./script\x00.sh" | ||
|
|
||
| assert not _contains_unsafe_gateway_action( | ||
| command, cwd="/tmp", depth=0, visited=set() | ||
| ) | ||
|
|
||
|
|
||
| def test_cron_session_contextvar_takes_precedence(monkeypatch) -> None: | ||
| monkeypatch.delenv("HERMES_CRON_SESSION", raising=False) | ||
| token = HERMES_CRON_SESSION_CONTEXTVAR.set(True) | ||
| try: | ||
| assert approval._is_cron_session() | ||
| finally: | ||
| HERMES_CRON_SESSION_CONTEXTVAR.reset(token) | ||
|
|
||
|
|
||
| def test_cron_session_contextvar_reset_restores_false(monkeypatch) -> None: | ||
| monkeypatch.delenv("HERMES_CRON_SESSION", raising=False) | ||
| token = HERMES_CRON_SESSION_CONTEXTVAR.set(True) | ||
| try: | ||
| assert approval._is_cron_session() is True | ||
| finally: | ||
| HERMES_CRON_SESSION_CONTEXTVAR.reset(token) | ||
|
|
||
| assert approval._is_cron_session() is False | ||
|
|
||
|
|
||
| def test_cron_session_is_false_without_context_or_env(monkeypatch) -> None: | ||
| monkeypatch.delenv("HERMES_CRON_SESSION", raising=False) | ||
|
|
||
| assert not approval._is_cron_session() | ||
|
|
||
|
|
||
| def test_cron_session_env_fallback(monkeypatch) -> None: | ||
| monkeypatch.setenv("HERMES_CRON_SESSION", "1") | ||
|
|
||
| assert approval._is_cron_session() | ||
|
|
||
|
|
||
| def test_run_job_exception_releases_lock_and_resets_cron_flag(tmp_path, monkeypatch) -> None: | ||
| """The real scheduler cleanup path releases its lock and ContextVar.""" | ||
| import threading | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| import cron.scheduler as scheduler | ||
|
|
||
| workdir = tmp_path / "cron-workdir" | ||
| workdir.mkdir() | ||
| job = {"id": "r3-test", "name": "cleanup", "prompt": "hi", "workdir": str(workdir)} | ||
|
|
||
| monkeypatch.delenv("HERMES_CRON_SESSION", raising=False) | ||
| assert HERMES_CRON_SESSION_CONTEXTVAR.get() is False | ||
|
|
||
| # Check the writer lock is available before invoking run_job, then leave it | ||
| # free for run_job to acquire. | ||
| scheduler._terminal_cwd_lock.acquire_write() | ||
| scheduler._terminal_cwd_lock.release_write() | ||
|
|
||
| real_info = scheduler.logger.info | ||
|
|
||
| def raise_on_workdir_log(message, *args, **kwargs): | ||
| if isinstance(message, str) and "using workdir" in message: | ||
| raise RuntimeError("r3 cleanup probe") | ||
| return real_info(message, *args, **kwargs) | ||
|
|
||
| with patch("cron.scheduler._hermes_home", tmp_path), \ | ||
| patch("cron.scheduler._resolve_origin", return_value=None), \ | ||
| patch("hermes_cli.env_loader.load_hermes_dotenv"), \ | ||
| patch("hermes_cli.env_loader.reset_secret_source_cache"), \ | ||
| patch.object(scheduler.logger, "info", side_effect=raise_on_workdir_log), \ | ||
| patch("hermes_state.SessionDB", return_value=MagicMock()): | ||
| result = scheduler.run_job(job) | ||
|
|
||
| assert result[0] is False | ||
| assert HERMES_CRON_SESSION_CONTEXTVAR.get() is False | ||
|
|
||
| # A leaked writer would block this acquisition indefinitely; the lock test | ||
| # uses the same bounded thread pattern for this synchronization primitive. | ||
| acquired = threading.Event() | ||
|
|
||
| def acquire_and_release() -> None: | ||
| scheduler._terminal_cwd_lock.acquire_write() | ||
| try: | ||
| acquired.set() | ||
| finally: | ||
| scheduler._terminal_cwd_lock.release_write() | ||
|
|
||
| thread = threading.Thread(target=acquire_and_release, daemon=True) | ||
| thread.start() | ||
| assert acquired.wait(timeout=1), "writer lock was leaked by run_job" | ||
| thread.join(timeout=1) | ||
| assert not thread.is_alive() | ||
|
|
||
|
|
||
| def test_run_job_handoff_propagates_cron_context_and_isolates_concurrent_thread( | ||
| tmp_path, monkeypatch | ||
| ) -> None: | ||
| import threading | ||
| from unittest.mock import MagicMock, patch | ||
|
|
||
| import cron.scheduler as scheduler | ||
|
|
||
| workdir = tmp_path / "cron-workdir" | ||
| workdir.mkdir() | ||
| job = { | ||
| "id": "r2-handoff", | ||
| "name": "handoff", | ||
| "prompt": "probe", | ||
| "workdir": str(workdir), | ||
| } | ||
| worker_probe = [] | ||
| concurrent_probe = [] | ||
|
|
||
| def probe_run_conversation(*args, **kwargs): | ||
| worker_probe.append(HERMES_CRON_SESSION_CONTEXTVAR.get()) | ||
|
|
||
| def probe_concurrent_thread() -> None: | ||
| concurrent_probe.append(HERMES_CRON_SESSION_CONTEXTVAR.get()) | ||
|
|
||
| thread = threading.Thread(target=probe_concurrent_thread) | ||
| thread.start() | ||
| thread.join(timeout=1) | ||
| assert not thread.is_alive() | ||
| raise RuntimeError("r2 handoff probe") | ||
|
|
||
| monkeypatch.delenv("HERMES_CRON_SESSION", raising=False) | ||
| assert HERMES_CRON_SESSION_CONTEXTVAR.get() is False | ||
| scheduler._terminal_cwd_lock.acquire_write() | ||
| scheduler._terminal_cwd_lock.release_write() | ||
|
|
||
| agent = MagicMock() | ||
| agent.run_conversation.side_effect = probe_run_conversation | ||
| with patch("cron.scheduler._hermes_home", tmp_path), \ | ||
| patch("cron.scheduler._resolve_origin", return_value=None), \ | ||
| patch("hermes_cli.env_loader.load_hermes_dotenv"), \ | ||
| patch("hermes_cli.env_loader.reset_secret_source_cache"), \ | ||
| patch( | ||
| "hermes_cli.runtime_provider.resolve_runtime_provider", | ||
| return_value={"provider": "test", "model": "test", "api_key": "key"}, | ||
| ), \ | ||
| patch("hermes_state.SessionDB", return_value=MagicMock()), \ | ||
| patch("run_agent.AIAgent", return_value=agent): | ||
| result = scheduler.run_job(job) | ||
|
|
||
| assert result[0] is False | ||
| assert worker_probe == [True] | ||
| assert concurrent_probe == [False] | ||
| assert HERMES_CRON_SESSION_CONTEXTVAR.get() is False |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This excludes a directly executed extensionless text script with no shebang from scanning, so
./restart-helpercan containhermes gateway restartand evade the lifecycle guard. Please preserve scanning for text candidates and skip only clearly binary files (for example, a bounded NUL-byte header check).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved in 7deebd1a0 —
_is_shell_script_filenow uses a bounded NUL-byte header check: text candidates (no NUL in first 512 bytes) are scanned, clearly binary files are skipped../restart-helper(extensionless, no shebang) is covered by the newtest_extensionless_text_script_without_shebang_is_scanned.