fix(cron): run scripts from job workdir - #57415
Conversation
Duplicate of #21397 (earliest open PR) — both thread the configured cron job |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused cron fix. The underlying issue is still present: current main hardcodes the script subprocess cwd at cron/scheduler.py:2106, and the agent pre-run path reaches it through cron/scheduler.py:2758.
Problems
- Current main added
_run_job_script_with_claim_heartbeat()incd537187611769ebb6a1aa9460265e9ef5694606; both no-agent and pre-run scripts now use it (cron/scheduler.py:2611,2758). The salvage must thread the cwd through that wrapper rather than replacing those calls with direct_run_job_script()calls, or one-shot scripts lose their claim-refresh protection. - Runtime workdir validation currently happens at
cron/scheduler.py:2868-2876, after the pre-run script. Resolve and validate the cwd before both script paths so a removed workdir retains the existing script-directory fallback. - The agent regression test mocks
_run_job_script, but production now calls the heartbeat wrapper. Update it to assert the current integration path and retain real-subprocess coverage for both modes.
Suggested changes
- Pass a validated optional cwd through
_run_job_script_with_claim_heartbeat()to_run_job_script()and remove the no-agentos.chdir()path. - Add a claimed one-shot regression test to ensure the heartbeat wrapper remains active.
Automated hermes-sweeper review.
| @@ -2288,23 +2304,7 @@ def run_job(job: dict) -> tuple[bool, str, str, Optional[str]]: | |||
| # Apply workdir if configured — lets scripts use predictable relative | |||
There was a problem hiding this comment.
Current main now routes no-agent scripts through _run_job_script_with_claim_heartbeat() to keep one-shot claims alive (cd5371876). During salvage, preserve that wrapper and thread cwd through it; replacing the call with direct _run_job_script() would drop the duplicate-dispatch protection.
What does this PR do?
Run cron script subprocesses from the job's configured
workdirwhen one is present, while preserving the existing default of running scripts fromHERMES_HOME/scriptswhen no workdir is configured.Why this approach: the script path should still be resolved and contained under
HERMES_HOME/scriptsfor safety, but the subprocess cwd should match the cron job's documented project workdir. Threading an explicitcwdinto_run_job_script()fixes the bug without relying on process-globalos.chdir().Related Issue
Fixes #21721
Related existing PRs: #21397 and #42920. This PR is intentionally draft because it overlaps with existing contributor work; maintainers can close this if one of those should land instead.
Type of Change
Changes Made
cron/scheduler.pycwdparameter to_run_job_script().subprocess.run(...)when provided.job["workdir"].tests/cron/test_cron_no_agent.pyworkdir.tests/cron/test_scheduler.pyHow to Test
Run the no-agent cron tests:
python -m pytest tests/cron/test_cron_no_agent.py -q -o 'addopts='Expected:
19 passedRun the targeted scheduler + no-agent cron tests:
python -m pytest tests/cron/test_scheduler.py tests/cron/test_cron_no_agent.py -q -o 'addopts='Expected:
227 passedRun lint on changed files:
Expected:
No issues foundChecklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AFor New Skills
N/A
Screenshots / Logs