From 863e31318553cda8ad61df681d08175364d4164b Mon Sep 17 00:00:00 2001 From: kshitij Date: Thu, 6 Aug 2026 17:31:25 +0530 Subject: [PATCH] =?UTF-8?q?fix:=20close=20simplify-pass=20findings=20?= =?UTF-8?q?=E2=80=94=20scheduler=20sibling=20site=20+=20home-unresolvable?= =?UTF-8?q?=20totality?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 3-reviewer simplify pass (reuse/quality/efficiency) findings: - cron/scheduler.py _run_job_script: the ORIGINAL that lifecycle_guard._resolve_script_path documents mirroring had the exact same unguarded expanduser() — a NUL-bearing script value survives creation (the guard treats it as nothing-to-scan) and crashed the scheduler at fire time with ValueError instead of a clean job failure. Same ingestion contract applied; regression test added. - lifecycle_guard._resolve_script_path: get_hermes_home() -> Path.home() raises RuntimeError when neither HERMES_HOME nor HOME resolves (arbitrary-UID containers); the cron entry point called it bare. Caught -> None; totality test added. - terminal_tool: stale 'cat ...' docstring updated to the bounded head -c form. - lifecycle_guard: dead 'script_text and' condition dropped (guarded by 'if not script_text: continue' directly above). Efficiency reviewer: no material findings (measured — encode/expand costs negligible vs walk I/O, no timing regression vs base). --- cron/lifecycle_guard.py | 10 ++++++-- cron/scheduler.py | 11 ++++++++- tests/cron/test_cron_no_agent.py | 13 ++++++++++ tests/hermes_cli/test_gateway_restart_loop.py | 24 +++++++++++++++++++ tools/terminal_tool.py | 2 +- 5 files changed, 56 insertions(+), 4 deletions(-) diff --git a/cron/lifecycle_guard.py b/cron/lifecycle_guard.py index 514d238bfdb53..ba783c94969b6 100644 --- a/cron/lifecycle_guard.py +++ b/cron/lifecycle_guard.py @@ -410,7 +410,7 @@ def _contains_unsafe_gateway_action( # Relative references inside a script resolve against that script's # directory, not the original command's cwd. script_dir = _resolve_script_directory(str(resolved)) or cwd - if script_text and _contains_unsafe_gateway_action( + if _contains_unsafe_gateway_action( script_text, cwd=script_dir, depth=depth + 1, @@ -488,7 +488,13 @@ def _resolve_script_path(script_path: str) -> Optional[Path]: return None if raw.is_absolute(): return raw - return get_hermes_home() / "scripts" / raw + try: + return get_hermes_home() / "scripts" / raw + except (RuntimeError, OSError): + # get_hermes_home() falls back to Path.home(), which raises when + # neither HERMES_HOME nor HOME is resolvable (launchd/systemd + # environments) — same ingestion contract: nothing to scan. + return None def _read_script_for_scanning(script_path: str) -> str: diff --git a/cron/scheduler.py b/cron/scheduler.py index 4fa234e6dde15..f64adde27cbfd 100644 --- a/cron/scheduler.py +++ b/cron/scheduler.py @@ -2251,7 +2251,16 @@ def _run_job_script( scripts_dir.mkdir(parents=True, exist_ok=True) scripts_dir_resolved = scripts_dir.resolve() - raw = Path(script_path).expanduser() + try: + raw = Path(script_path).expanduser() + except (ValueError, RuntimeError, OSError): + # Same ingestion contract as cron.lifecycle_guard: a NUL-bearing + # value (ValueError) or an unexpandable ``~`` (RuntimeError with no + # resolvable HOME) can never name a real script. The creation-time + # guard tolerates such values as "nothing to scan", so they can + # reach fire time — fail the run with a report instead of crashing + # the scheduler with an unhandled exception. + return False, f"Blocked: script path is not a valid filesystem path: {script_path!r}" if raw.is_absolute(): path = raw.resolve() else: diff --git a/tests/cron/test_cron_no_agent.py b/tests/cron/test_cron_no_agent.py index 7dd7a4e8a0e38..6ba5afecf0ee5 100644 --- a/tests/cron/test_cron_no_agent.py +++ b/tests/cron/test_cron_no_agent.py @@ -118,3 +118,16 @@ def test_run_job_script_path_traversal_still_blocked(hermes_env): ok, output = _run_job_script("/etc/passwd") assert ok is False assert "Blocked" in output or "outside" in output + + +def test_run_job_script_nul_path_fails_cleanly(hermes_env): + """Sibling of the lifecycle-guard ingestion fix: a NUL-bearing script + value can survive to fire time (the creation-time guard treats it as + "nothing to scan"), and ``Path.expanduser()`` raises ValueError — not + OSError — on it. The scheduler must fail the run with a report, not + crash with an unhandled exception.""" + from cron.scheduler import _run_job_script + + ok, output = _run_job_script("~user\x00bad.sh") + assert ok is False + assert "Blocked" in output diff --git a/tests/hermes_cli/test_gateway_restart_loop.py b/tests/hermes_cli/test_gateway_restart_loop.py index c39d2d88cbd19..09682010b89bf 100644 --- a/tests/hermes_cli/test_gateway_restart_loop.py +++ b/tests/hermes_cli/test_gateway_restart_loop.py @@ -867,6 +867,30 @@ class TestLifecycleGuardModule: "echo hello" ) is False + def test_cron_guard_total_when_home_unresolvable(self, monkeypatch): + """`get_hermes_home()` falls back to Path.home(), which raises + RuntimeError when neither HERMES_HOME nor HOME resolves + (arbitrary-UID containers, launchd). The cron entry point must + treat a relative script value as unresolvable — nothing to scan — + not crash.""" + from pathlib import Path + + from cron.lifecycle_guard import check_gateway_lifecycle + + monkeypatch.delenv("HERMES_HOME", raising=False) + monkeypatch.delenv("HOME", raising=False) + monkeypatch.setattr( + Path, + "home", + classmethod( + lambda cls: (_ for _ in ()).throw( + RuntimeError("Could not determine home directory") + ) + ), + ) + # Must not raise; relative script cannot resolve without a home. + check_gateway_lifecycle("daily ops", "relative-script.sh") + # --------------------------------------------------------------------------- # Defense 2 (chokepoint): cron.jobs.create_job blocks the AGENT model-tool path diff --git a/tools/terminal_tool.py b/tools/terminal_tool.py index 3ab6aa4d2a4ba..6457ec76dcedd 100644 --- a/tools/terminal_tool.py +++ b/tools/terminal_tool.py @@ -2533,7 +2533,7 @@ def terminal_tool( For local backends the script path is on the host filesystem. For SSH/Modal/Daytona the same path is remote; the local read misses, so we - fall back to ``env.execute('cat ...')``. + fall back to a bounded ``env.execute('head -c ... < path')`` read. """ if env is None: return None