fix(cron): honor workdir for no_agent job scripts - #56005
Open
Frowtek wants to merge 1 commit into
Open
Conversation
A no_agent cron job's configured `workdir` was silently ignored. The no_agent path called os.chdir(workdir) to set the script's working directory, but _run_job_script() runs the subprocess with an explicit cwd=str(path.parent) (HERMES_HOME/scripts), which overrides the parent process's cwd. The script therefore always ran from the scripts dir, so relative paths in a watchdog/data-collection script resolved against the wrong directory. Thread the workdir into _run_job_script() as an optional cwd argument and pass it straight to subprocess.run(). Drop the ineffective os.chdir dance, which was also process-global and unsafe while parallel jobs run. When the workdir no longer exists, log and fall back to the scripts dir (mirrors the agent path). workdir-less jobs are unchanged. Adds regression tests for _run_job_script(cwd=...) and the end-to-end no_agent execution path.
13 tasks
19 tasks
Contributor
|
Thanks for isolating the explicit subprocess cwd override; the no-agent defect is real on current main. Problems
Suggested changes
Automated hermes-sweeper review. |
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
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.
What does this PR do?
Fixes a bug where a
no_agentcron job's configuredworkdirwas silently ignored — the job's script always ran fromHERMES_HOME/scripts/instead of the configured working directory.The
no_agentpath inrun_job()calledos.chdir(workdir)to set the script's working directory (its own comment: "For no_agent jobs this is just the subprocess cwd"). But_run_job_script()launches the subprocess with an explicitcwd=str(path.parent)(the scripts dir), and an explicitcwd=argument tosubprocess.run()sets the child's working directory absolutely — it overrides the parent process'sos.chdir(). So thechdirwas a no-op for the subprocess and the workdir never took effect.Impact: a
no_agentwatchdog/data-collection script that uses relative paths (./state.json,python ./sub.py, reading./config) resolved them against the scripts directory rather than the project directory the user configured — silent wrong reads/writes or "file not found".The fix threads the workdir into
_run_job_script()as an optionalcwdargument and passes it straight tosubprocess.run(). It also removes theos.chdir()dance, which was process-global and unsafe while other jobs run in parallel.Related Issue
No existing issue — found via a code audit of the cron execution path.
Type of Change
Changes Made
cron/scheduler.py_run_job_script()gains an optionalcwdparameter. The subprocess now runs withcwdwhen it is an existing directory; otherwise it falls back to the historical default (the script's own dir underHERMES_HOME/scripts/). Backward compatible — the two callers that don't passcwdare unchanged.run_job()no_agentpath: pass the job's validatedworkdirvia_run_job_script(script_path, cwd=_job_workdir)and drop the ineffective (and process-global)os.chdir()/restore block. When the workdir no longer exists, log and fall back to running without it — mirroring the agent path's existing behavior.tests/cron/test_cron_workdir.pyTestRunJobScriptCwd—cwdhonored when provided, default scripts-dir when omitted, and fallback whencwddoesn't exist.TestNoAgentScriptWorkdir— end-to-end: ano_agentjob's script runs in the configured workdir, defaults to the scripts dir without a workdir, and safely falls back when the workdir vanished after job creation.How to Test
Reproduction (before the fix):
no_agentcron job with aworkdirpointing at some project dir, and a script underHERMES_HOME/scripts/that prints its own cwd (import os; print(os.getcwd())).HERMES_HOME/scripts— not the configured workdir. Any relative path in the script resolves against the wrong directory.After the fix: the same job's script prints (and runs from) the configured workdir.
Automated regression tests (added in this PR):
Before/after proof — with the
scheduler.pyfix reverted (tests kept), the end-to-end test fails, demonstrating the bug:With the fix applied, all 6 pass.
Full cron suite:
pytest tests/cron/→571 passed(plus the 6 new).ruff checkon the changed files passes clean; the Windows-footgun checker reports no findings on the diff.Checklist
Code
fix(cron):)tests/cron/) and it passes; added 6 regression tests that all passDocumentation & Housekeeping
_run_job_script's docstring documents the newcwdargumentcli-config.yaml.exampleif I added/changed config keys — N/A (no config keys)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/Asubprocess.run(cwd=...)+Path.is_dir(), both cross-platform; removes a process-globalos.chdir()