fix(cron): strip leading scripts/ prefix to avoid scripts/scripts/ trap (#26595)#26622
Closed
briandevans wants to merge 1 commit into
Closed
fix(cron): strip leading scripts/ prefix to avoid scripts/scripts/ trap (#26595)#26622briandevans wants to merge 1 commit into
briandevans wants to merge 1 commit into
Conversation
Contributor
Author
|
CI audit — all 4 test failures are pre-existing baselines on clean
Happy to re-run CI once these are addressed on main. |
This was referenced May 16, 2026
0a7546b to
e9a15d6
Compare
e9a15d6 to
e3618ad
Compare
…ap (NousResearch#26595) The scheduler resolves relative cron script paths against HERMES_HOME/scripts/, so a user-supplied "scripts/foo.py" resolves to HERMES_HOME/scripts/scripts/foo.py and silently falls through the "Script not found" branch at runtime. The script appears to vanish between configuration time and execution, leaving the agent to deliver stale data from the previous successful run with no surfaced error. Normalize the script field at create_job and update_job time: strip a leading "scripts" segment from relative paths and log the rewrite at info level. Absolute and ~-prefixed paths pass through unchanged so the scheduler's existing path-traversal guard keeps them honest. Regression guard runs create_job(script="scripts/monitor.py") through _run_job_script and asserts the script actually executes — the pre-fix double-prefix path would have hit the "not found" branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
e3618ad to
0414125
Compare
Contributor
Author
|
Closing to focus the queue on security/file-safety work where civilian merges are landing. Happy to reopen if maintainers want this picked up. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
scripts/segment from relative cron script paths at create/update time.script=\"scripts/foo.py\"from silently resolving toHERMES_HOME/scripts/scripts/foo.pyat runtime and falling through the "Script not found" branch with no surfaced error.~-prefixed paths pass through unchanged so the scheduler's existing path-traversal guard still validates them.The bug
cron.scheduler._run_job_scriptbuilds the runtime path as(scripts_dir / raw).resolve()wherescripts_dir = HERMES_HOME / \"scripts\". So a user-provided\"scripts/foo.py\"resolves toHERMES_HOME/scripts/scripts/foo.py. The file doesn't exist, the runtime returns(False, \"Script not found: ...\"), and the agent silently falls back to processing stale data from a prior successful run — no error surfaces at configuration time, only a quiet skip at runtime.This trap is non-obvious from the
cronjobtool's docstring ("Paths resolve under~/.hermes/scripts/") because the user reasonably reads the script field as "path relative to the profile root," not "path relative to the scripts dir."The fix
New
_normalize_script_path()in cron/jobs.py — small, local, mirrors the existing_normalize_workdirshape:None(preserves existing behaviour).~-prefixed paths pass through verbatim — the scheduler's path-traversal guard validates them at runtime, and rewriting them here could silently re-point a user's explicit absolute path.parts[0] == \"scripts\"and there is at least one trailing segment, drop the leadingscriptspart and log the rewrite atINFOlevel.Wired into both
create_job(replacing the inline strip) andupdate_job(new branch alongside the existingworkdirnormalization).Test plan
tests/cron/test_cron_script.py::TestScriptPathNormalization— 9 new cases covering: strip, nested-path strip, unprefixed passthrough, absolute passthrough, tilde passthrough, lonescriptssegment passthrough, update-time strip, update-clear via whitespace, and an end-to-end runtime guard that calls_run_job_scripton the normalized output and asserts the script actually executes (the symptom the issue reporter saw).tests/cron/— 356 passed locally (uv run --with pytest --with pytest-xdist --with pytest-asyncio python3 -m pytest tests/cron/)._normalize_script_path()and restoring the inline strip causes the newtest_normalized_script_resolves_to_real_file_at_runtimeandtest_create_job_strips_leading_scripts_prefix*tests to fail with the pre-fix "scripts/scripts/" path — confirmed before commit.Scope notes
Intentionally limited to path normalization at the config layer. The issue also raises (a) create-time existence validation and (b) skill-prefixed script references — both deferred:
/path/to/monitor.py) and several call sites where the script is deployed after the cron is registered. Worth doing, but a larger surface change with policy implications.skill:foo/scripts/bar.py) are a feature, not a bug — separate scope.Happy to widen if preferred. The current fix removes the silent-skip symptom for the common case.
Related