fix(cron): catch ValueError in lifecycle_guard resolve/open paths - #76813
fix(cron): catch ValueError in lifecycle_guard resolve/open paths#76813blut-agent wants to merge 1 commit into
Conversation
The lifecycle guard's _contains_unsafe_gateway_action and _read_referenced_script only caught OSError when resolving or opening script paths. Paths with embedded null bytes (common with absolute-path binaries like /usr/bin/python3) raise ValueError instead, which propagated uncaught and caused the terminal tool to reject any command invoking an absolute-path executable (NousResearch#76762). Catch ValueError alongside OSError in both sites so the guard degrades gracefully: an unreadable path is treated as a non-script and skipped, not treated as an unsafe lifecycle command. Also adds a regression test covering the null-byte path case.
Duplicate of #76773 — it covers the same lifecycle_guard ValueError repair for absolute-path executables and adds broader binary/script handling. |
monerostar
left a comment
There was a problem hiding this comment.
Ubuntu 26.04 on linux-5800x, CPython 3.14.4.
Checked the two sites in cron/lifecycle_guard.py: os.open and Path.resolve now catch (OSError, ValueError) on this tip (4404777). origin/main still OSError-only there.
Focused pytest -k "ValueError or resolve or open_path or non_regular" on tests/hermes_cli/test_gateway_restart_loop.py: 2 passed (incl. null-byte path regression).
Full file needs more deps for the terminal_tool matrix here; the ValueError path this PR cares about is covered. Looks good.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused exception-path repair. Current main does have the ValueError exposure at cron/lifecycle_guard.py:298-302, and catching it at both resolve and open is directionally correct.
Problems
- The patch still leaves
cron/lifecycle_guard.py:222treating every slash-containing executable as a referenced script._read_referenced_script()then marks regular files larger than 1 MiB unsafe atcron/lifecycle_guard.py:261-262, so normal absolute-path binaries can remain blocked even after theValueErroris caught. - The new test covers only a literal-NUL executable token. It does not exercise ordinary or oversized binaries, nor verify that real shebang scripts remain scanned.
Suggested changes
- Extend the guard to skip binary executables before resolving/reading them while preserving scans of shell scripts. The linked duplicate #76773 contains a broader implementation and coverage for these cases.
Automated hermes-sweeper review.
| try: | ||
| descriptor = os.open(path, flags) | ||
| except OSError: | ||
| except (OSError, ValueError): |
There was a problem hiding this comment.
Catching ValueError protects the literal-NUL case, but it does not prevent _iter_referenced_shell_scripts() from yielding every slash-containing executable (cron/lifecycle_guard.py:222). _read_referenced_script() marks regular targets over 1 MiB unsafe at line 261, so ordinary absolute-path binaries can still be blocked. Please skip binary executables while retaining scans of scripts; #76773 has the broader approach.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs address #76762. #76813 only catches ValueError in the open/resolve paths, whereas #76773 also prevents NUL-containing binaries from being scanned as scripts while retaining scans for actual shebang scripts.
Related pull requests
- #76773
duplicate— (+92/-2) — comprehensive fix candidate: The diff catches ValueError, filters binary executables before recursive script inspection, and tests ordinary and oversized binaries alongside benign and lifecycle-triggering scripts. This supports the maintainer-bot keep_open verdict and provides the concrete salvage path for the full failure class. - #76813
partial— (+20/-2) — partial duplicate of #76773: The diff catches ValueError and tests the literal-NUL path, but it leaves slash-containing executables subject to script scanning and therefore leaves oversized-binary false blocking unresolved. Despite the keep_open review on #76813, the diff and the blocking contributor feedback show that its useful exception handling is already subsumed by the broader #76773 implementation.
Duplicates
#76813 and #76773 overlap on the ValueError repair; #76813 is the narrower duplicate, while #76773 additionally addresses binary-versus-script misclassification and preserves script scanning with dedicated regression coverage.
Suggested consolidation
Keep #76773 open with a salvage path: retain its ValueError handling, binary-header filtering, and regression coverage for ordinary and oversized binaries plus real scripts. Close #76813 as a duplicate of #76773; this differs from the visible keep_open review on #76813 because its diff does not address the contributor-identified oversized-binary path, while every substantive change it does provide is already present in #76773.
Complex graph
flowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I76762(["issue #76762 (open)"])
subgraph Dup76773 ["PRs duplicating each other"]
P76773["PR #76773 (open)"]
P76813["PR #76813 (open)"]
end
P76813 -.->|partial| I76762
class I76762 open
class P76773 open
class P76813 open
class P76773 best
class P76813 target
click I76762 "https://github.com/NousResearch/hermes-agent/issues/76762"
click P76773 "https://github.com/NousResearch/hermes-agent/pull/76773"
click P76813 "https://github.com/NousResearch/hermes-agent/pull/76813"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label).
Cross-PR triage: Reviewed 2 pull requests and 1 issue in this complex. Each diff was read against this issue; Assessment working set: 9 kB of PR diffs, 4 kB of issue/PR text, 7 kB of discussion (10 comments), 3 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
|
Closing in favor of #76773 (by @webtecnica) — the GottZ AI triage identified this as a duplicate. #76773 covers the same ValueError handling plus additional binary-vs-script classification that my narrower fix doesn't address. Thanks @teknium1 for the review and @monerostar for the live-verify. |
|
Thanks @blut-agent for the graceful consolidation — appreciate the collaboration and the shout-out. The ValueError handling from #76813 is the core of the fix, and the binary-vs-script classification builds on top of it. Happy to iterate together on #76773. |
Fixes #76762
Problem
The lifecycle guard's
_contains_unsafe_gateway_actionand_read_referenced_scriptonly caughtOSErrorwhen resolving or opening script paths. Paths with embedded null bytes (common with absolute-path binaries like/usr/bin/python3) raiseValueErrorinstead, which propagated uncaught and caused the terminal tool to reject any command invoking an absolute-path executable.Fix
Catch
ValueErroralongsideOSErrorin both sites so the guard degrades gracefully: an unreadable path is treated as a non-script and skipped, not treated as an unsafe lifecycle command.Changes
cron/lifecycle_guard.py: AddedValueErrorto exception handlers in_read_referenced_script(line 254) and_contains_unsafe_gateway_action(line 301)tests/hermes_cli/test_gateway_restart_loop.py: Added regression testtest_absolute_path_with_null_byte_does_not_crashTesting
All 78 existing tests pass. The new regression test verifies that absolute-path executables with embedded null bytes no longer crash the lifecycle guard.