fix(cron): skip shell-script reference walk for Python scripts (#77131) - #77230
fix(cron): skip shell-script reference walk for Python scripts (#77131)#77230RelaxJonh wants to merge 2 commits into
Conversation
…ousResearch#77211) When `hermes update` finds no new commits, it previously only checked the Python venv health (`_venv_core_imports_healthy`). A previous run that partially failed at the npm install step (EBADENGINE, network timeout, interrupted install) left node_modules in a mixed state, and the "Already up to date!" path never repaired it. Mirror the existing "repair if broken" pattern: consult `_npm_lockfile_changed()` on the no-new-commits path. When it reports stale (missing node_modules, lockfile hash mismatch, or incomplete web toolchain), run `_update_node_dependencies()` + `_build_web_ui()` to bring Node deps into a consistent state. Fixes NousResearch#77211
…esearch#77131) The lifecycle guard applied shell-style tokenization and script-reference resolution to Python script contents. Pathlib's "/" operator tokenized as a standalone "/" segment, which resolved to the filesystem root. Reading "/" returned unsafe=True (it's a directory, not a regular file), causing every Python script containing pathlib division to be blocked. Fix: for .py scripts, skip the shell-script reference walk entirely. Python files are executed by the interpreter, never through a POSIX shell, so shell-reference analysis produces false positives. The direct command regex scan still catches actual lifecycle commands like "hermes gateway restart" written in the script text. Fixes NousResearch#77131
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Five PRs address the two lifecycle-guard regressions. #76773 and #76813 target absolute-path executables, #77137 and #77230 target Python pathlib false positives, and #77201 combines both fixes; #77230 also includes an unrelated Node-dependency update-path change.
Related pull requests
- #76773
best fix— (+92/-2) — keep open with a salvage path: the diff fixes both absolute-path failure modes by catching ValueError and excluding NUL-bearing binaries before bounded script scanning, while preserving shebang-script traversal and adding coverage for ordinary and oversized binaries. This agrees with the maintainer-bot keep-open verdict and is the recorded best fix for #76762. - #76813 [closed]
partial— (+20/-2) — close as duplicate of #76773: this closed PR remains relevant as the source of the ValueError handling consolidated into #76773, but its diff does not prevent oversized absolute-path binaries from failing closed. Despite the keep_open review on #76813, the broader diff and tests in #76773 address that review's binary-classification concern, and the author already closed #76813 in its favor. - #77137
best fix— (+44/-5) — keep open with a salvage path: the current diff scopes recursive shell-reference scanning away from non-shell cron scripts, retains direct lifecycle-command detection, and tests both Path.home() division and Path("/tmp") absolute operands. This retains the recorded best fix for #77131 and agrees with the keep_open review; the current diff directly addresses that review's requested scheduler-aligned scoping and missing absolute-operand regression. - #77201
fixes— (+102/-8) — close as duplicate of #76773 and #77137: its combined diff catches ValueError, skips NUL-bearing binaries, and bypasses shell-reference traversal for Python scripts, but those two issue-specific fixes are already represented by the recorded best-fix PRs. Keeping the fixes separated avoids replacing the narrower duplicate chain with a combined alternative. - #77230
fixes— (+42/-1) — close as duplicate of #77137: its lifecycle-guard hunk uses the same Python-script bypass for #77131, while #77137 additionally removes the broader slash-containing executable heuristic for direct references and covers Path("/tmp") explicitly. The unrelated hermes_cli/update_cmd.py Node-dependency change should not remain bundled with this duplicate fix.
Duplicates
#76813 is superseded by and duplicates #76773. #77230 duplicates the #77131 fix in #77137, while #77201 combines changes substantially duplicating both #76773 and #77137.
Suggested consolidation
Keep #76773 open with its binary-versus-script classification and absolute-path regression suite, and keep #77137 open with its non-shell scoping and pathlib absolute-operand coverage. Close #76813 as duplicate of #76773, close #77230 as duplicate of #77137, and close the combined #77201 as duplicate of #76773 and #77137; any unrelated Node-dependency repair from #77230 should be split into its own PR.
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
I77131(["issue #77131 (open)"])
P77230["PR #77230 (open)"]
P77230 -->|fixes| I77131
class I77131 open
class P77230 open
class P77230 target
click I77131 "https://github.com/NousResearch/hermes-agent/issues/77131"
click P77230 "https://github.com/NousResearch/hermes-agent/pull/77230"
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 5 pull requests and 2 issues in this complex. Each diff was read against this issue; Assessment working set: 25 kB of PR diffs, 13 kB of issue/PR text, 18 kB of discussion (19 comments), 5 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
|
Merged via #77332 which salvaged @criptogus's implementation from #77201. Your PR also included unrelated changes to |
Summary
The lifecycle guard applied shell-style tokenization and script-reference resolution to Python script contents. Pathlib's
/operator tokenized as a standalone/segment, which resolved to the filesystem root. Reading/returnedunsafe=True(it's a directory, not a regular file), causing every Python script containing pathlib division to be blocked.Root Cause
_iter_command_segments()shlex-tokenizes each line of the script_iter_referenced_shell_scripts()(line 222):if "/" in executable— a bare/token is treated as an executable pathPath.home() / ".hermes" / ".env", pathlib's/operator tokenizes as a standalone/segment_read_referenced_script(Path("/"))returnsunsafe=Truebecause/is a directory_contains_unsafe_gateway_action()propagatesunsafe=True→ job creation failsFix
For
.pyscripts, skip the shell-script reference walk entirely. Python files are executed by the interpreter, never through a POSIX shell, so shell-reference analysis produces false positives. The direct command regex scan (which catcheshermes gateway restartetc.) still runs.Testing
Verified with the reproduction from the issue:
Before fix: blocked. After fix: allowed (correctly — no lifecycle commands present).
Fixes #77131