Skip to content

fix(cron): resolve relative script paths relative to job workdir instead of cwd (#59599) - #60475

Open
webtecnica wants to merge 1 commit into
NousResearch:mainfrom
webtecnica:fix/59599-cron-script-paths
Open

webtecnica wants to merge 1 commit into
NousResearch:mainfrom
webtecnica:fix/59599-cron-script-paths

Conversation

@webtecnica

Copy link
Copy Markdown
Contributor

Resolve relative cron script paths relative to job workdir instead of cwd. All 3 _run_job_script call sites now pass base_dir from job's workdir. Closes #59599

@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 Jul 7, 2026
…ead of cwd

(cherry picked from commit 6c575c850b4ae3908ba94eeab6003690c6d59581)
@webtecnica
webtecnica force-pushed the fix/59599-cron-script-paths branch from 6c575c8 to 5592315 Compare July 11, 2026 12:49

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling cron script-path handling. The current patch needs rework before it can provide the stated behavior.

Problems

  • The new base_dir resolution still flows into the existing scripts_dir containment check at cron/scheduler.py:2059. Therefore a script under a job workdir outside HERMES_HOME/scripts remains blocked; the new branch does not enable the advertised ./scripts/foo.sh case.
  • Current main executes scripts through _run_job_script_with_claim_heartbeat (cron/scheduler.py:2611, 2758), whose internal calls at 2162, 2193, and 2196 do not pass a base directory. The PR's older direct-call edits do not cover those paths.
  • #59599 asks to accept absolute/home-relative paths already under ~/.hermes/scripts, but tools/cronjob_tools.py:546-550 rejects those values before the scheduler runs. This PR does not touch that boundary.

Suggested changes

  • Decide whether the intended scope is #59599's scripts-directory normalization or a broader workdir execution policy; the latter must preserve the containment guarantees introduced by 878b1d3d3.
  • Carry any approved resolution context through the heartbeat wrapper and add agent/no-agent regression coverage.

Automated hermes-sweeper review.

Comment thread cron/scheduler.py
raw = Path(script_path).expanduser()
if raw.is_absolute():
path = raw.resolve()
elif base_dir:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This resolution is immediately rejected by the unchanged path.relative_to(scripts_dir_resolved) guard below whenever base_dir is outside HERMES_HOME/scripts. Thus the advertised job-workdir-relative path still cannot run; the resolution and containment policy need to be designed together.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data 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 15, 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 P3 Low — cosmetic, nice to have 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 sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cronjob create rejects absolute script paths without clear error guidance

3 participants