fix(cron): skip binary content returned by remote script readers

[dvbaecker]

What does this PR do?

Fixes a false-positive block (and an uncaught crash) in the gateway lifecycle guard when a terminal command executes a large binary by absolute path from inside the gateway (_HERMES_GATEWAY=1).

When a referenced file exceeds the 1MB local read limit (_MAX_REFERENCED_SCRIPT_BYTES), the terminal tool's reader falls back to a backend shell read (cat ...) and hands the guard the entire binary as text. The guard then scanned that decoded machine code: junk tokens tokenized out of it (which contain / and NUL bytes) were fed into the referenced-script recursion, causing:

1. Uncaught crash: ValueError: embedded null byte from os.open on junk path tokens.
2. Fail-closed block: innocent commands like /.../venv/bin/python --version return "Blocked: command or referenced script cannot restart or stop the gateway..." even though they are harmless.

This is the residual half of the fix from #76762: that patch added the NUL-byte binary skip to the local read path (_read_referenced_script), but the remote-reader content path was left unguarded, and the os.open there only caught OSError, not ValueError.

The fix treats NUL-byte content returned by the remote reader exactly like the existing local binary skip — a binary is never a referenced shell script — and catches ValueError on os.open for junk paths. The security boundary must be immune to whatever the reader callback returns (local, SSH, Modal, or any other backend).

Related Issue

Fixes #76510

(Follow-up hardening to #76762; that issue fixed the local read path but left the remote-reader content path and the os.open crash.)

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/lifecycle_guard.py:
  • _contains_unsafe_gateway_action: skip NUL-byte content returned by read_remote_script (mirrors the local binary skip from terminal tool: lifecycle_guard crashes on absolute-path executables (ValueError: embedded null byte), blocks all such commands #76762).
  • _read_referenced_script: catch ValueError alongside OSError on os.open (embedded NUL byte in junk paths tokenized from binary content).
• tests/cron/test_lifecycle_guard_binary_false_positive.py: new regression tests (red on main, green with this patch).

How to Test

1. Regression tests (fail on unpatched main with ValueError: embedded null byte, pass with this patch):

   pytest tests/cron/test_lifecycle_guard_binary_false_positive.py -v

2. Live reproduction (inside a running gateway, _HERMES_GATEWAY=1):

   /home/<user>/.hermes/hermes-agent/venv/bin/python --version
   

   Before: Blocked: command or referenced script cannot restart or stop the gateway... (or a ValueError traceback). After: runs normally.
3. Security invariants preserved (covered by tests): direct hermes gateway restart and genuine shell scripts containing it are still blocked, both from the local reader and via the remote reader.

Verified with the full cron + gateway suite: pytest tests/cron/ tests/hermes_cli/test_gateway_restart_loop.py tests/hermes_cli/test_gateway_service.py tests/tools/test_terminal_none_command_guard.py → 551 passed.

Full suite (pytest tests/ -q --ignore=tests/docker): 1897 passed. One pre-existing, order-dependent failure in tests/agent/test_file_safety.py::TestCacheFileReadBlocking::test_hub_index_cache_blocked — verified to fail identically on unmodified main in the same run order (passes in isolation); unrelated to this change.

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: Arch Linux (CachyOS BORE kernel), Python 3.11.15, gateway as systemd user service

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — docstring comments explain the NUL-skip and reference both issues
• I've updated cli-config.yaml.example if I added/changed config keys — N/A (no config changes)
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
• I've considered cross-platform impact (Windows, macOS) per the compatibility guide — the fix is pure-Python on the guard's reader boundary, no platform-specific syscalls; the NUL-byte check works identically on Windows/macOS
• I've updated tool descriptions/schemas if I changed tool behavior — N/A (no tool schema changes)

Screenshots / Logs

Red on main (unpatched guard):

FAILED tests/cron/test_lifecycle_guard_binary_false_positive.py::TestRemoteReaderBinarySkip::test_large_binary_via_remote_reader_not_blocked
FAILED tests/cron/test_lifecycle_guard_binary_false_positive.py::TestReadReferencedScriptHardening::test_null_byte_path_does_not_raise
E   ValueError: embedded null byte
cron/lifecycle_guard.py:260: ValueError
2 failed, 5 passed

Green with this patch:

7 passed in 0.13s

Production scenario (21MB venv interpreter returned by the backend reader):

Reader liefert 21479135 chars Binary-Muell
1) Produktionsszenario: venv/bin/python --version  -> PASS
2) Sicherheits-Invariante: echtes Restart-Script via Reader  -> BLOCKED
3) Direktes Lifecycle-Kommando wird weiterhin geblockt  -> BLOCKED

[alt-glitch]

This was generated by AI during triage.

Duplicate of #77729: both add the remote referenced-script fallback binary/NUL skip for the lifecycle guard. This PR also catches the resulting invalid-path ValueError, but the primary repair mechanism is already open there.

[dvbaecker]

Thanks for the triage. After reading #77729 carefully, I confirm the core repair (skipping binary content from the remote-read fallback) and the os.open ValueError catch are present in both PRs. Two deltas worth flagging for whoever merges:

1. Fix placement. #77729 applies the NUL skip inside tools/terminal_tool.py::_read_script_in_env — the current sole reader. This PR applies it in cron/lifecycle_guard.py, where the read_remote_script callback's return value is consumed. The guard is a security boundary and read_remote_script is a public callback parameter, so placing the check there keeps the boundary immune to any future reader that returns binary content, instead of requiring each reader to remember to filter. The two placements are complementary (defense at the reader + defense at the chokepoint), not conflicting.

2. Test coverage. This PR adds 7 regression tests including the security invariants: a genuine lifecycle command is still blocked directly and via a referenced script, both through the local reader and through the remote reader. Those guard-still-blocks assertions are not covered by #77729's two crash-focused tests.

Happy to close this in favor of #77729 and contribute the chokepoint placement + invariant tests there if that is cleaner for review. Deferring to maintainer preference.

[kshitijk4poor]

Closing as superseded by #80258, which fixes this whole bug class architecturally rather than per-callsite: path candidates are sanitized once at the ingestion boundary (NUL/empty/unexpandable tokens rejected before any OS call), text from any read_remote_script callback is sanitized at the recursion boundary (NUL = binary = nothing to scan; >1 MiB = fail closed), the remote fallback read is bounded at the source (head -c, so oversized binaries never cross the wire), and the public guard is total by construction — an unexpected walk failure logs and falls back to the direct-scan verdict instead of breaking every terminal command.

Your report and fix targeted a real member of this class — thank you. The per-callsite patches kept leaving sibling frames exposed (#76762 → #77703 → #77780 → #78256 each crashed one frame away from the previous fix), which is why we went with the boundary fix instead of merging the fragments individually. #80258 carries regression tests for the NUL-path, binary-callback, oversized-read, unset-HOME, and walk-crash cases plus an adversarial never-raises sweep.