Skip to content

fix(cron): skip non-script files in lifecycle guard script walk - #78108

Closed
jiangjingzwd wants to merge 1 commit into
NousResearch:mainfrom
jiangjingzwd:fix/lifecycle-guard-absolute-path-binaries
Closed

fix(cron): skip non-script files in lifecycle guard script walk#78108
jiangjingzwd wants to merge 1 commit into
NousResearch:mainfrom
jiangjingzwd:fix/lifecycle-guard-absolute-path-binaries

Conversation

@jiangjingzwd

Copy link
Copy Markdown

Summary

Follow-up to #76762 / #77332. The lifecycle guard's referenced-script walk
(_iter_referenced_shell_scripts) yields every token containing / as
a "referenced shell script", then reads and scans its bytes. For absolute
paths to non-scripts (binaries like /usr/bin/python3, logs, data files)
this is wasted I/O and a false-positive source — and it was the original
crash vector behind #76762.

This PR narrows the walk to files that actually look like shell scripts:

  • extension match (.sh / .bash / .zsh), or
  • a regular file with a #! shebang

Non-regular/unreadable paths fall through to the legacy scan so the
existing fails-closed behaviour (_read_referenced_script) is preserved.

Changes

  • cron/lifecycle_guard.py: add _looks_like_shell_script() and apply it
    in _iter_referenced_shell_scripts
  • tests/hermes_cli/test_gateway_restart_loop.py: new test
    test_absolute_path_extensionless_non_script_is_not_walked asserting a
    .txt file is not walked while an extensionless shebang script still is

Test

83 passed in 1.02s   (tests/hermes_cli/test_gateway_restart_loop.py)

Closes #76762 (completes the second proposed fix from the issue)

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cron Cron scheduler and job management labels Aug 4, 2026
@jiangjingzwd

Copy link
Copy Markdown
Author

Context: the merged fix (#77332 / c98ed22 + 037825c) does not fully close #76762

For maintainers reviewing this PR, there is important follow-up evidence on #76762 after it was closed on 2026-08-03:

AndrewRahman's comment (2026-08-03 09:38 UTC, after the issue was closed) shows the crash still reproduces on a gateway including both c98ed22e42 and 037825c1f2. The traceback moved from Path.resolve() to os.open() at line 260, and took down every absolute-path terminal command for ~4h in their environment:

File "cron/lifecycle_guard.py", line 260, in _read_referenced_script           # os.open(path, flags)
ValueError: embedded null byte

Root cause of the residual gap (from their analysis, which I verified against the current main):

  1. _read_referenced_script returns None for two different reasons — "couldn't read" and "it's a binary, skip it" (the b"\x00" in data branch). _contains_unsafe_gateway_action treats None as only the first and re-reads the same binary via read_remote_script, whose backend decodes with errors="replace" — NUL survives as a valid UTF-8 codepoint, undoing the binary-skip rule.
  2. os.open() raises ValueError (not OSError) on NUL-containing paths, so except OSError doesn't catch it — same shape as the original .resolve() bug.

This is why CI doesn't catch it either: tests call contains_gateway_lifecycle_command_or_referenced_script(cmd) without the read_remote_script fallback wired up.

How this PR differs

This PR removes the entire crash class rather than patching the instance: _iter_referenced_shell_scripts only follows files that actually look like shell scripts (.sh/.bash/.zsh extension, or a #! shebang). A binary like /usr/bin/python3 never enters the scan path at all — so neither the local read NOR the read_remote_script fallback can choke on it. The legacy fails-closed scan is preserved for non-regular/unreadable paths.

Test suite: 83 passed in 1.02s. The new test asserts a .txt file is not walked while an extensionless shebang script still is.

This is complementary to (and strictly more root-cause than) the merged fix — happy to rebase or adjust if maintainers prefer a narrower diff.

@teknium1

Copy link
Copy Markdown
Contributor

The binary false-positive class this PR targets is resolved on main: _read_referenced_script() skips directories (S_ISDIR), identifies real binaries by magic number (ELF/Mach-O/PE/archive/gzip/zip — PR #93411), and bounds oversized reads. Live-verified: full-path .venv/bin/python and multi-MB ELF invocations pass without blocking or crashing. Closing as implemented on main — thanks!

@teknium1 teknium1 closed this Aug 24, 2026
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 P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

terminal tool: lifecycle_guard crashes on absolute-path executables (ValueError: embedded null byte), blocks all such commands

3 participants