fix(cron): honor workdir for no_agent job scripts - #99
Conversation
|
Review Complete Files Reviewed: 2 By Severity:
PR adds workdir support to no_agent cron jobs but introduces two medium-severity bugs: pre-check scripts for LLM (agent) cron jobs ignore the configured workdir, and a TOCTOU race between workdir existence check and subprocess execution produces misleading error messages. Files Reviewed (2 files) |
There was a problem hiding this comment.
Risk: 🟡 Medium (42/100) — 1 medium finding · 154 LOC across 2 files
Findings
2 medium-severity bugs found in , both related to the new workdir parameter handling for cron job scripts.
Finding 1: Pre-check script ignores workdir in LLM path
The no_agent code path (lines 2021–2030) correctly extracts, validates, and passes to . However, the LLM (agent) pre-check script invocation at line 2113 calls without passing the parameter. This causes pre-check scripts for agent cron jobs to always execute from regardless of the job's configured .
Finding 2: TOCTOU race on workdir existence check
At line 1642, is checked non-atomically before at line 1643. If the workdir is removed between these two calls (plausible on NFS mounts, tmpfs, or during ephemeral environment teardown), raises caught by the generic handler at line 1677. The operator sees with no indication the root cause was a vanished directory rather than a script bug.
| from tools.environments.local import _sanitize_subprocess_env | ||
|
|
||
| popen_kwargs = {"creationflags": windows_hide_flags()} if sys.platform == "win32" else {} | ||
| run_cwd = cwd if (cwd and Path(cwd).is_dir()) else str(path.parent) |
There was a problem hiding this comment.
🟡 TOCTOU race between Path.is_dir() and subprocess.run cwd in _run_job_script (bug)
The cwd existence check at cron/scheduler.py:1642 is not atomic with subprocess.run at line 1643-1651. If the workdir is removed between the check and the subprocess execution, subprocess.run raises FileNotFoundError, caught generically at line 1677 and returned as 'Script execution failed: [error]'. This gives the operator no indication that the root cause was a vanished workdir rather than a script bug. The race is plausible on NFS mounts, tmpfs-based workdirs, or during ephemeral-environment teardown.
💡 Suggestion: Wrap the subprocess.run call to catch FileNotFoundError / NotADirectoryError specifically when cwd was provided, and return a clear error message like 'Workdir no longer exists: ' so operators can distinguish a vanished directory from a genuine script failure.
📋 Prompt for AI Agents
In cron/scheduler.py around lines 1643-1678, add a specific except clause before the generic except Exception to catch FileNotFoundError and NotADirectoryError when the cwd parameter was provided. Return (False, f'Workdir no longer exists: {cwd}') instead of the generic message. This gives operators an actionable signal when the configured workdir disappears between the existence check and subprocess execution.
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()Mirror-of: NousResearch#56005
NousResearch#56005