fix(cron): make the lifecycle guard total — sanitize at ingestion, not per-syscall - #80258
Merged
kshitijk4poor merged 3 commits intoAug 6, 2026
Merged
Conversation
…t per-syscall The guard feeds untrusted byte streams (tokenized binaries, remote cat output) into OS-path and shell-text operations; every incident so far (NousResearch#76762, NousResearch#77703, NousResearch#77780, NousResearch#78256, NousResearch#77729) was hot-fixed with an except at whichever frame crashed that week. tilllt's regression suite on NousResearch#79454 showed 4 members of the class still open on merged main. Close the class at three boundaries instead: - _expand_candidate_path(): single ingestion chokepoint for path candidates — reject NUL/empty tokens before any Path OS call and tolerate ValueError/RuntimeError/OSError from expanduser (T1/T2, plus the HOME-unset launchd crash). Both _resolve_terminal_script_path and _resolve_script_path now go through it. - _sanitize_remote_script_text(): apply the local-read contract (NUL = binary = nothing to scan; >1MiB = fail closed) to whatever any read_remote_script callback returns, at the recursion boundary — the guard stops trusting its callbacks (T3/T4). - contains_gateway_lifecycle_command_or_referenced_script() is now total by construction: direct regex scans (pure string ops) run first; the best-effort filesystem walk is wrapped so an unexpected failure logs a warning and falls back to the direct-scan verdict instead of killing every terminal command until gateway restart. terminal_tool's remote fallback also bounds the read at the source (head -c 1MiB+1 instead of cat), so a 166MB ELF never crosses the wire — the superlinear-shlex 30-minute stall from NousResearch#79838's field report drops to a 0.02s fail-closed verdict. Regression tests: tilllt's T1-T4 adopted verbatim, plus an adversarial never-raises sweep (NUL paths, unset HOME, over-long paths) and a walk-crash fallback test.
- _sanitize_remote_script_text: compare re-encoded BYTES against the cap, not characters — a >1MiB multibyte file truncated at the head -c byte bound decodes to fewer chars than bytes and would have scanned the truncated text instead of failing closed (the exact local/remote divergence this PR closes). - terminal_tool: replace the three hardcoded 1MiB literals with lifecycle_guard._MAX_REFERENCED_SCRIPT_BYTES so the budget cannot drift; use the redirect-safe 'head -c N < path' form from tools/image_source.py so leading-dash paths stay out of argv. - Public guard wrapper: drop the duplicate depth-0 direct scan — the walk already runs it; the except-path now falls back to the pure string scans, preserving the direct verdict when the walk crashes.
monerostar
reviewed
Aug 6, 2026
monerostar
left a comment
Contributor
There was a problem hiding this comment.
Ubuntu 26.04 on linux-5800x.
Ran the PR's tests/hermes_cli/test_gateway_restart_loop.py against three trees:
| tree | result |
|---|---|
| origin/main | 7 failed, 84 passed (ValueError embedded null on tilde+NUL candidates; remote/oversized/adversarial cases) |
| sibling #80241 | 6 failed, 85 passed (still misses tilde-NUL walk, oversized remote fail-closed, total-function cases) |
| this PR | 91 passed |
Sanitizing at ingestion (_expand_candidate_path / remote text contract) is the right whole-class shape vs per-syscall catches. Prefer this over the narrower #80241 patch.
Looks good from Linux here.
…esolvable totality 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).
This was referenced Aug 6, 2026
This was referenced Aug 6, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
The gateway lifecycle guard is now a total function — every input maps to a verdict, no input can crash it or stall it — by sanitizing untrusted bytes at three boundaries instead of catching exceptions at whichever syscall crashed that week.
Root cause of the class: the guard feeds untrusted byte streams (shlex-tokenized binaries, remote
catoutput) into OS-path and shell-text operations with no ingestion boundary. Each incident (#76762, #77703, #77780, #78256, #77729) got a per-callsiteexcept; @tilllt's regression suite on #79454 (comment) showed 4 members of the class still open on merged main, and ~30 open community PRs each hot-fix another fragment.Changes
cron/lifecycle_guard.py—_expand_candidate_path(): single ingestion chokepoint for path candidates. Rejects NUL/empty tokens before anyPathOS call and toleratesValueError/RuntimeError/OSErrorfromexpanduser()(T1/T2, plus the HOME-unset launchd crash from fix(guard): never crash or false-positive on binary/executable paths #78056). Both_resolve_terminal_script_pathand_resolve_script_pathroute through it.cron/lifecycle_guard.py—_sanitize_remote_script_text(): applies the local-read contract (NUL = binary = nothing to scan; >1 MiB = fail closed) to whatever anyread_remote_scriptcallback returns, at the recursion boundary. The guard stops trusting its callbacks — fix(terminal): skip binary content on referenced-script remote-read fallback (#77703) #79454 hardened one callback; this covers every current and future one (T3/T4).cron/lifecycle_guard.py—contains_gateway_lifecycle_command_or_referenced_script()is total by construction: direct regex scans (pure string ops) run first; the best-effort filesystem walk is wrapped so an unexpected failure logs a warning and falls back to the direct-scan verdict instead of breaking every terminal command until gateway restart.tools/terminal_tool.py— remote fallback readshead -c 1048577instead ofcat, so an oversized file never crosses the wire. One byte over budget is enough for the sanitizer to fail closed.tests/hermes_cli/test_gateway_restart_loop.py— tilllt's T1–T4 adopted as regression tests, plus an adversarial never-raises sweep (NUL paths, unset HOME, over-long paths) and a walk-crash fallback test.Validation
tilllt's T1–T4 harness against merged main (
49d8a155c) vs this branch:~\x00candidate via terminal walkValueError: embedded null byte)~\x00candidate via cron script pathValueError: embedded null byte)Live probes (real imports, no guard mocks): all positive detections preserved — direct commands, launchctl kickstart/submit, nested wrapper scripts, remote-callback detection of a real lifecycle script; a 170 MB callback payload goes from a 30+ minute superlinear-shlex stall (#79838's field report) to a 0.02 s fail-closed verdict.
Tests:
test_gateway_restart_loop.py91 passed;test_terminal_tool.py+tests/cron/422 passed.Credit
ValueError: embedded null bytefrom os.open — breaks all terminal commands #77780/lifecycle_guard crashes with ValueError: embedded null byte on 'python -m pip' commands (incomplete #76762 fix) #78256, fix(terminal): skip binary content on the referenced-script remote-read fallback (#77703) #77729, fix(guard): bound lifecycle-guard shlex scan against binary/oversized input #79838 (166 MB ELF stall), fix(guard): never crash or false-positive on binary/executable paths #78056 (HOME-unset launchd crash).