fix(cron): ignore pathlib division in lifecycle guard - #77137
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a regression in the cron lifecycle guard where Python pathlib expressions containing a standalone / token (e.g., Path.home() / ".hermes") were mistakenly treated as executable script references, causing no_agent=True Python cron job creation to be blocked.
Changes:
- Adjust referenced-shell-script detection to ignore a standalone
"/"token during scanning. - Add a regression test ensuring a Python script containing
Path.home() / ...is not blocked bycheck_gateway_lifecycle().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
cron/lifecycle_guard.py |
Refines _iter_referenced_shell_scripts() to avoid treating a standalone / token as an executable path. |
tests/hermes_cli/test_gateway_restart_loop.py |
Adds a regression test covering Python pathlib division (Path.home() / ...) to prevent the false-positive block. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # A standalone slash is a Python arithmetic operator in expressions | ||
| # such as ``Path.home() / ".hermes"``. It is not an executable path; | ||
| # treating it as one makes the scanner resolve ``/`` and fail closed | ||
| # on every ordinary pathlib-based Python cron script. | ||
| if (executable != "/" and "/" in executable) or executable.endswith( |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the standalone-/ pathlib failure; that false positive is real on current main.
Problems
cron/lifecycle_guard.py:226only excludes the exact/token.Path("/tmp") / "x"tokenizes into/tmpand/;/tmpstill enters the referenced-script walk and fails closed because it is a directory. The existing Copilot inline review identifies this same case.- The new test at
tests/hermes_cli/test_gateway_restart_loop.py:631coversPath.home()but not an absolute-string pathlib operand.
Suggested changes
- Align the guard with
cron/scheduler.py:2204-2284: shell-reference traversal should be scoped to scripts the scheduler invokes through Bash, while retaining direct lifecycle-command detection for Python scripts. - Add a regression case for
Path("/tmp") / "x".
Automated hermes-sweeper review.
| # such as ``Path.home() / ".hermes"``. It is not an executable path; | ||
| # treating it as one makes the scanner resolve ``/`` and fail closed | ||
| # on every ordinary pathlib-based Python cron script. | ||
| if (executable != "/" and "/" in executable) or executable.endswith( |
There was a problem hiding this comment.
This only exempts the exact / token. Path("/tmp") / "x" tokenizes into /tmp and /, so /tmp still reaches the referenced-script walk and fails closed as a directory. Please cover absolute-path pathlib operands or scope shell-reference scanning to scripts actually run through Bash.
|
Merged via #77332 which salvaged @criptogus's implementation from #77201 — the most thorough of the three competing fixes. Your approach (removing |
Summary
/arithmetic operator in Python pathlib expressions as an executable script pathPath.home() / ...Fixes #77131.
Verification
python3 -m compileall -q cron/lifecycle_guard.py tests/hermes_cli/test_gateway_restart_loop.pygit diff --checkpassesThe repository checkout did not have pytest installed, so the focused pytest command was attempted but could not run (
No module named pytest).