Skip to content

fix(cron): scope no_agent job workdir to the script subprocess - #42274

Open
itswane wants to merge 1 commit into
NousResearch:mainfrom
itswane:fix/cron-no-agent-workdir-process-cwd
Open

fix(cron): scope no_agent job workdir to the script subprocess#42274
itswane wants to merge 1 commit into
NousResearch:mainfrom
itswane:fix/cron-no-agent-workdir-process-cwd

Conversation

@itswane

@itswane itswane commented Jun 8, 2026

Copy link
Copy Markdown

What does this PR do?

A no_agent cron job with a configured workdir changed the scheduler's
process-global working directory via os.chdir() for the duration of its
script run. That mutation is not isolated to the job: tick() partitions due
jobs into a single-thread sequential pool (workdir/profile jobs) and a parallel
pool (everything else), and dispatches both fire-and-forget in the same tick.
While the sequential no_agent job held the altered cwd, any workdir-less job
running concurrently in the parallel pool — which deliberately leaves
TERMINAL_CWD unset and relies on the scheduler's own cwd for its
terminal/file/code-exec tools — resolved its relative paths against the foreign
job's directory.

The os.chdir() was also inert for its stated purpose: _run_job_script
already passes an explicit cwd=str(path.parent) to subprocess.run, which
overrides the process cwd, so the documented "script runs from the workdir"
behaviour never actually took effect.

The fix threads the workdir through to the subprocess as an explicit
cwd_override instead of mutating the process. This scopes the working
directory to the child process — removing the cross-job contamination — and, as
a side effect, makes the documented per-job workdir behaviour work correctly.

Related Issue

N/A

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • cron/scheduler.py: added an optional cwd_override parameter to
    _run_job_script; when set to an existing directory it becomes the
    subprocess cwd, otherwise the call falls back to the script's own directory
    (with a logged warning for a stale/invalid override).
  • cron/scheduler.py: removed the process-global os.chdir()/restore block in
    the no_agent branch of _run_job_impl and now pass the job's workdir
    through as cwd_override.
  • tests/cron/test_cron_no_agent.py: added coverage for cwd_override
    (honoured / fallback) and a regression test asserting a no_agent+workdir
    job runs its script from the workdir without changing the scheduler's process
    cwd.

How to Test

  1. Run the targeted suite: scripts/run_tests.sh tests/cron/test_cron_no_agent.py.
  2. Confirm test_run_job_no_agent_does_not_mutate_process_cwd passes — it
    asserts the script's pwd equals the configured workdir while
    os.getcwd() is unchanged before and after the run.
  3. Run the surrounding suites for regressions:
    scripts/run_tests.sh tests/cron/.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (Darwin 25.5.0)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — docstring for _run_job_script updated
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — subprocess.run(cwd=...) is portable; no platform-specific paths added
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

## What does this PR do?

A `no_agent` cron job with a configured `workdir` changed the scheduler's
process-global working directory via `os.chdir()` for the duration of its
script run. That mutation is not isolated to the job: `tick()` partitions due
jobs into a single-thread sequential pool (workdir/profile jobs) and a parallel
pool (everything else), and dispatches both fire-and-forget in the same tick.
While the sequential `no_agent` job held the altered cwd, any workdir-less job
running concurrently in the parallel pool — which deliberately leaves
`TERMINAL_CWD` unset and relies on the scheduler's own cwd for its
terminal/file/code-exec tools — resolved its relative paths against the foreign
job's directory.

The `os.chdir()` was also inert for its stated purpose: `_run_job_script`
already passes an explicit `cwd=str(path.parent)` to `subprocess.run`, which
overrides the process cwd, so the documented "script runs from the workdir"
behaviour never actually took effect.

The fix threads the workdir through to the subprocess as an explicit
`cwd_override` instead of mutating the process. This scopes the working
directory to the child process — removing the cross-job contamination — and, as
a side effect, makes the documented per-job workdir behaviour work correctly.

## Related Issue

N/A

## Type of Change

- [x] 🐛 Bug fix (non-breaking change that fixes an issue)

## Changes Made

- `cron/scheduler.py`: added an optional `cwd_override` parameter to
  `_run_job_script`; when set to an existing directory it becomes the
  subprocess `cwd`, otherwise the call falls back to the script's own directory
  (with a logged warning for a stale/invalid override).
- `cron/scheduler.py`: removed the process-global `os.chdir()`/restore block in
  the `no_agent` branch of `_run_job_impl` and now pass the job's `workdir`
  through as `cwd_override`.
- `tests/cron/test_cron_no_agent.py`: added coverage for `cwd_override`
  (honoured / fallback) and a regression test asserting a `no_agent`+`workdir`
  job runs its script from the workdir without changing the scheduler's process
  cwd.

## How to Test

1. Run the targeted suite: `scripts/run_tests.sh tests/cron/test_cron_no_agent.py`.
2. Confirm `test_run_job_no_agent_does_not_mutate_process_cwd` passes — it
   asserts the script's `pwd` equals the configured workdir while
   `os.getcwd()` is unchanged before and after the run.
3. Run the surrounding suites for regressions:
   `scripts/run_tests.sh tests/cron/`.

## Checklist

### Code

- [x] I've read the Contributing Guide
- [x] My commit messages follow Conventional Commits (`fix(scope):`, `feat(scope):`, etc.)
- [x] I searched for existing PRs to make sure this isn't a duplicate
- [x] My PR contains **only** changes related to this fix/feature (no unrelated commits)
- [x] I've run `pytest tests/ -q` and all tests pass
- [x] I've added tests for my changes (required for bug fixes, strongly encouraged for features)
- [x] I've tested on my platform: macOS 15 (Darwin 25.5.0)

### Documentation & Housekeeping

- [x] I've updated relevant documentation (README, `docs/`, docstrings) — docstring for `_run_job_script` updated
- [x] I've updated `cli-config.yaml.example` if I added/changed config keys — or N/A
- [x] I've updated `CONTRIBUTING.md` or `AGENTS.md` if I changed architecture or workflows — or N/A
- [x] I've considered cross-platform impact (Windows, macOS) per the compatibility guide — `subprocess.run(cwd=...)` is portable; no platform-specific paths added
- [x] I've updated tool descriptions/schemas if I changed tool behavior — or N/A
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cron Cron scheduler and job management labels Jun 8, 2026
@liuhao1024

Copy link
Copy Markdown
Contributor

Verification review — reviewed the full diff; clean implementation.

The os.chdir()subprocess.run(cwd=) refactor correctly eliminates the process-global working directory mutation that bled into concurrent jobs in the parallel pool. Key observations:

  • The cwd_override parameter is properly scoped to the child process only
  • Graceful fallback when the configured workdir no longer exists (logs warning, uses script's own directory)
  • Tests cover: honoured override, invalid override fallback, and the regression case (process cwd unchanged after job with workdir)
  • The os.getcwd() / os.chdir() / finally-restore pattern is fully removed, not just patched over

No issues found. LGTM.

@liuhao1024

Copy link
Copy Markdown
Contributor

✅ Verification Review

Scope: Process-safety fix — replaces os.chdir() (process-global) with subprocess.run(cwd=...) (per-child) for no_agent cron jobs.

Assessment: Clean implementation.

  1. Correctness: The os.chdircwd= migration is straightforward. The cwd_override parameter is validated with Path.is_dir() before use, with a clean fallback to the script's own directory and a logged warning.

  2. Concurrency safety: The docstring and inline comments correctly identify the race condition — os.chdir is process-global, so concurrent parallel-pool jobs would see the wrong cwd. The fix scopes the cwd to subprocess.run() only.

  3. Test coverage: Three tests cover the happy path (cwd_override honored), invalid workdir (falls back gracefully), and the regression guarantee (scheduler's os.getcwd() unchanged after a workdir job). The symlink resolution via Path.resolve() handles macOS /tmp/private/tmp correctly.

  4. No side effects: The finally: os.chdir(_prior_cwd) block was the previous mitigation — fragile because it could fail on cleanup (OSError), and it still left a window where concurrent jobs saw the wrong cwd. The subprocess-scoped approach eliminates the window entirely.

No issues found. LGTM.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused no-agent fix. I verified the premise against current main: cron/scheduler.py:2535-2551 still mutates the scheduler process with os.chdir() before calling the script helper, while _run_job_script() hardcodes subprocess.run(..., cwd=str(path.parent)) at cron/scheduler.py:2100-2106. That makes the mutation ineffective for the child script's cwd and leaves a process-global window outside the later TERMINAL_CWD lock.

The proposed explicit subprocess cwd removes that mutation and gives the no-agent script the configured workdir. The existing regression coverage is appropriately targeted.

Scope note: agent pre-run scripts still call the same helper without a workdir at cron/scheduler.py:2632-2635; related PR #57415 is the broader general-script counterpart. This PR remains cleanly scoped to no_agent.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants