Skip to content

fix(terminal): stop the lifecycle guard crashing on every command whose executable contains a slash - #78829

Closed
EagleClaw89 wants to merge 1 commit into
NousResearch:mainfrom
EagleClaw89:fix/lifecycle-guard-nul-path-from-remote-reader
Closed

fix(terminal): stop the lifecycle guard crashing on every command whose executable contains a slash#78829
EagleClaw89 wants to merge 1 commit into
NousResearch:mainfrom
EagleClaw89:fix/lifecycle-guard-nul-path-from-remote-reader

Conversation

@EagleClaw89

Copy link
Copy Markdown

The bug

When _HERMES_GATEWAY=1, tools/terminal_tool.py runs
contains_gateway_lifecycle_command_or_referenced_script over every terminal command, and
any slash-bearing executable is treated as a referenced shell script.
cron/lifecycle_guard.py._read_referenced_script correctly refuses binaries via its NUL check
(#76762), but the read_remote_script fallback the terminal tool passes
(_read_script_in_env) had no such check: it returned the binary decoded with
errors="replace".

That decoded ELF/Mach-O/PE went straight back into the recursive scanner, which tokenized
machine code into path candidates carrying NUL bytes. os.open then raised ValueError: embedded null character in path — and the handler on that call site only caught OSError, so
it escaped the guard.

Observed on a real deployment (v2026.8.3), scanning a multi-megabyte interpreter before
failing:

28.62s  ValueError  .venv/bin/python -m pytest tests/test_summarize.py -q
28.60s  ValueError  .venv/bin/python scripts/morning_brief.py
 0.45s  ValueError  /bin/ls -la
28.71s  ValueError  /usr/bin/python3 -m pytest -q

Note /bin/ls: this is not limited to interpreters or venvs. Every affected command surfaced
to the agent as Failed to execute command: open: embedded nul.... A kanban worker burned all
its allowed attempts unable to run either its program or its test suite, then was auto-blocked
for a protocol violation — with the work actually done and none of it reported.

The fix, root cause first

  • tools/terminal_tool.py — extract _script_text_if_not_binary and route both
    branches of _read_script_in_env through it, so a binary is never handed to the scanner.
    The cat fallback is the branch that actually did it: a multi-megabyte interpreter fails the
    local 1 MiB size check and falls through to env.execute. NUL is valid UTF-8 and survives an
    errors="replace" decode, so the same check is correct for bytes and for already-decoded
    output.
  • cron/lifecycle_guard.py — catch ValueError alongside OSError on the os.open call.
    _resolve_script_path in the same recursion already treats a NUL path this way; os.open
    did not.

The order matters. Catching the ValueError alone stops the crash but leaves the scanner
walking decoded machine code — measured at over 80s per command on the same interpreter,
worse than failing fast. Rejecting the binary at source keeps it at 0.06s.

Why the existing #76762 test did not catch it

test_absolute_path_binary_does_not_crash_guard passes on unfixed code because it omits
read_remote_script
— which is exactly the blind spot: the callback the terminal tool always
supplies was never exercised. Added:

  • test_nul_path_from_remote_reader_does_not_crash_guard — drives the guard with a reader that
    decodes anything, as the old callback did, and asserts no crash. Uses a small synthetic binary
    so it stays fast.
  • test_remote_reader_binary_rejected_before_scanning — covers _script_text_if_not_binary
    directly: binaries rejected as bytes and as already-decoded text, real shell scripts passed
    through unchanged so the referenced-script walk still works, and non-UTF-8-but-NUL-free
    content still treated as text.

Both fail on the parent commit. tests/hermes_cli/test_gateway_restart_loop.py goes from
83 passed / 1 failed to 84 passed; tests/tools -k terminal is 178 passed, 2 skipped.

Not a weakening

Lifecycle patterns are matched on the command itself before any path handling, so the
foot-guns this guard exists to stop are unaffected — verified that hermes gateway restart|stop, launchctl kickstart and systemctl restart hermes-gateway are all still
blocked, and the neighbouring nested-script / launchctl-submit tests still pass.

…se executable contains a slash

When _HERMES_GATEWAY=1, tools/terminal_tool.py runs
contains_gateway_lifecycle_command_or_referenced_script over EVERY terminal
command, and any slash-bearing executable is treated as a referenced shell
script. cron/lifecycle_guard.py._read_referenced_script correctly refuses
binaries via its NUL check (NousResearch#76762), but the read_remote_script fallback the
terminal tool passes (_read_script_in_env) had no such check: it returned the
binary decoded with errors="replace".

That decoded ELF/Mach-O/PE went straight back into the recursive scanner, which
tokenized machine code into path candidates carrying NUL bytes. os.open then
raised ValueError: open: embedded null character in path -- and the handler on
that call site only caught OSError, so it escaped the guard.

Observed on a real deployment (v2026.8.3), scanning a multi-megabyte
interpreter before failing:

    28.62s  ValueError  .venv/bin/python -m pytest tests/test_summarize.py -q
    28.60s  ValueError  .venv/bin/python scripts/morning_brief.py
     0.45s  ValueError  /bin/ls -la
    28.71s  ValueError  /usr/bin/python3 -m pytest -q

Note /bin/ls: this was not limited to interpreters or venvs. Every affected
command surfaced to the agent as
"Failed to execute command: open: embedded nul...". A kanban worker burned all
its allowed attempts unable to run either its program or its test suite, then
was auto-blocked for a protocol violation.

Two changes, root cause first:

* tools/terminal_tool.py: extract _script_text_if_not_binary and route BOTH
  branches of _read_script_in_env through it, so a binary is never handed to the
  scanner. The `cat` fallback is the branch that actually did it -- a
  multi-megabyte interpreter fails the local 1 MiB size check and falls through
  to env.execute. NUL is valid UTF-8 and survives an errors="replace" decode, so
  the same check is correct for bytes and for already-decoded output.

* cron/lifecycle_guard.py: catch ValueError alongside OSError on the os.open
  call. _resolve_script_path in the same recursion already treats a NUL path
  this way; os.open did not.

The order matters. Catching the ValueError alone stops the crash but leaves the
scanner walking decoded machine code -- measured at over 80s per command on the
same interpreter, worse than failing fast. Rejecting the binary at source keeps
it at 0.06s.

The existing NousResearch#76762 regression test passes on unfixed code because it omits
read_remote_script, which is exactly the blind spot: the callback the terminal
tool always supplies was never exercised. Added:

* test_nul_path_from_remote_reader_does_not_crash_guard -- drives the guard with
  a reader that decodes anything, as the old callback did, and asserts no crash.
  Uses a small synthetic binary so it stays fast.
* test_remote_reader_binary_rejected_before_scanning -- covers
  _script_text_if_not_binary directly: binaries rejected as bytes and as decoded
  text, real shell scripts passed through unchanged so the referenced-script walk
  still works, and non-UTF-8-but-NUL-free content still treated as text.

Both fail on the parent commit; the file goes 83 passed / 1 failed to 84 passed.
tests/tools -k terminal: 178 passed, 2 skipped.

Not a weakening: lifecycle patterns are matched on the command itself before any
path handling, and the neighbouring tests covering nested-script detection and
launchctl submit still pass.
@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management tool/terminal Terminal execution and process management sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists labels Aug 4, 2026

@EagleClaw89 EagleClaw89 left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

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.

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

Labels

comp/cron Cron scheduler and job management needs-decision Awaiting maintainer decision before any implementation P2 Medium — degraded but workaround exists sweeper:risk-automation Sweeper risk: may affect CI, automerge, label sync, or maintainer automation tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants