Skip to content

fix(cron): pass arguments to job scripts#20354

Open
leprincep35700 wants to merge 1 commit into
NousResearch:mainfrom
leprincep35700:fix/cron-script-arguments
Open

fix(cron): pass arguments to job scripts#20354
leprincep35700 wants to merge 1 commit into
NousResearch:mainfrom
leprincep35700:fix/cron-script-arguments

Conversation

@leprincep35700

@leprincep35700 leprincep35700 commented May 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Parse cron job script entries with shlex.split() so a script path can be followed by CLI arguments.
  • Pass parsed arguments through to both Python and bash cron scripts.
  • Keep the existing scripts-directory containment check on the parsed script path before execution.
  • Add regression coverage for a relative script invoked with arguments.
  • Add the missing AUTHOR_MAP entry for this commit email so attribution checks can resolve the contributor.

Why this shape

Cron scripts are already configured as a single string, and users can reasonably write values such as memory-ttl.py expire or monitor.py --check-only. Previously _run_job_script() treated the entire string as the filename, so the script either failed lookup or ran without the intended mode depending on how it was configured downstream.

Using shlex.split() keeps quoted arguments predictable while preserving the existing explicit interpreter selection and path containment model.

Testing

  • Verified the new regression test fails before the fix.
  • /opt/venv/bin/python -m py_compile cron/scheduler.py tests/cron/test_cron_script.py scripts/release.py
  • /opt/venv/bin/python -m pytest tests/cron/test_cron_script.py::TestRunJobScript tests/cron/test_cron_script.py::TestScriptPathContainment tests/cron/test_cron_no_agent.py -q -o addopts=''

Result: 35 passed, 1 warning.

Notes for maintainers

This is intentionally limited to script argument parsing and does not change the trust boundary: the executable script itself still has to resolve inside HERMES_HOME/scripts/ before it runs.

Closes #20300.

@leprincep35700 leprincep35700 force-pushed the fix/cron-script-arguments branch from e1ed3c5 to cdccc19 Compare May 5, 2026 18:38
@leprincep35700

leprincep35700 commented May 5, 2026

Copy link
Copy Markdown
Contributor Author

CI note after the latest run:

  • check-attribution, supply-chain scan, e2e, and both Nix jobs are green.
  • The global test job is still red, but its failures are broad and not limited to this PR's cron argument path.
  • The one cron failure shown by the global job (TestBuildJobPromptWithScript::test_script_empty_output_noted) reproduces on the current origin/main at 87b113c, so it is not introduced by this branch.
  • The targeted coverage for this change is green locally:
    • py_compile for cron/scheduler.py, tests/cron/test_cron_script.py, and scripts/release.py
    • pytest tests/cron/test_cron_script.py::TestRunJobScript tests/cron/test_cron_script.py::TestScriptPathContainment tests/cron/test_cron_no_agent.py -q -o addopts=''
    • Result: 35 passed, 1 warning

Happy to rebase and re-run once the base test suite is green if that helps review.

@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 May 5, 2026
@leprincep35700 leprincep35700 mentioned this pull request May 9, 2026
Merged
25 tasks
@leprincep35700 leprincep35700 force-pushed the fix/cron-script-arguments branch from cdccc19 to f90dd81 Compare May 14, 2026 21:53
@JackALaing JackALaing mentioned this pull request May 15, 2026
Open
@leprincep35700 leprincep35700 force-pushed the fix/cron-script-arguments branch from f90dd81 to bd4ddec Compare May 18, 2026 20:49
This was referenced May 20, 2026
Open
Open
@leprincep35700 leprincep35700 force-pushed the fix/cron-script-arguments branch from bd4ddec to 7995b9c Compare May 26, 2026 07:31
@leprincep35700

leprincep35700 commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

Branch refreshed onto current main and the old legacy Tests / test timeout is gone on the new head.

  • New head: 7995b9ca4
  • Merge state: clean
  • Local validation rerun:
    • HERMES_HOME=$(mktemp -d /tmp/hermes-pr20354-home.XXXXXX) HERMES_CONFIG_DIR="$HERMES_HOME" /opt/venv/bin/python -m pytest tests/cron/test_cron_script.py::TestRunJobScript tests/cron/test_cron_script.py::TestScriptPathContainment tests/cron/test_cron_no_agent.py -q -o addopts= → 35 passed
    • /opt/venv/bin/python -m py_compile cron/scheduler.py tests/cron/test_cron_script.py scripts/release.py
    • git diff --check

GitHub now shows the PR as clean with successful current checks.

@leprincep35700

leprincep35700 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

Current-main reassessment:

I rechecked this against current main.

Conclusion: still useful and still clean.

What current main still lacks:

  • cron/scheduler.py still treats the configured script string as a single path.
  • It still does not split/pass additional script arguments through to Python/bash job scripts.
  • I did not find an equivalent merged fix.

Current update status:

  • GitHub reports the branch as cleanly mergeable.
  • The diff remains small and targeted: scheduler argument parsing/passing plus cron regression tests.

Recommendation:

  • Keep this PR as-is or lightly refresh CI; no substantive rework appears needed.

@leprincep35700 leprincep35700 force-pushed the fix/cron-script-arguments branch 2 times, most recently from 2f626df to f5f3a41 Compare June 1, 2026 09:13
@leprincep35700

leprincep35700 commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main and force-pushed the refreshed single-commit branch.

  • New head: f5f3a419c
  • Local validation:
    • /opt/venv/bin/python -m pytest tests/cron/test_cron_script.py::TestRunJobScript tests/cron/test_cron_script.py::TestScriptPathContainment -q -o addopts='' --tb=short → 17 passed
    • git diff --check → OK
  • Attribution was refreshed to the mapped GitHub noreply author email.

@leprincep35700 leprincep35700 force-pushed the fix/cron-script-arguments branch from f5f3a41 to fb91c43 Compare June 6, 2026 06:38
@leprincep35700 leprincep35700 force-pushed the fix/cron-script-arguments branch from fb91c43 to b0f6c44 Compare June 9, 2026 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cron: scheduler does not parse script arguments (e.g. memory-ttl.py expire)

2 participants

@leprincep35700 @alt-glitch