Skip to content

fix(cron): use path.as_posix() for .sh scripts on Windows - #44350

Open
franchaise wants to merge 1 commit into
NousResearch:mainfrom
franchaise:fix/cron-windows-sh-paths
Open

fix(cron): use path.as_posix() for .sh scripts on Windows#44350
franchaise wants to merge 1 commit into
NousResearch:mainfrom
franchaise:fix/cron-windows-sh-paths

Conversation

@franchaise

Copy link
Copy Markdown

On Windows, cron .sh and .bash scripts were passed to bash using str(path), which produced backslash-separated paths. Bash (especially Git Bash / WSL) interprets backslashes as escape characters, so the script path could be mangled and fail to execute.

This change passes path.as_posix() for shell scripts so the argument is a forward-slash path regardless of host OS.

  • cron/scheduler.py: use path.as_posix() when invoking .sh/.bash scripts
  • tests/cron/test_cron_no_agent.py: add regression test test_run_job_script_sh_uses_posix_paths

Closes #43073

On Windows, passing a pathlib Path to bash via str(path) produces
backslash separators (C:\Users\...). Bash interprets these as escape
sequences, mangling the path and causing exit code 127.

Switch to path.as_posix() so .sh cron scripts always receive
forward-slash paths regardless of platform.

Fixes NousResearch#43073
@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management P2 Medium — degraded but workaround exists duplicate This issue or pull request already exists labels Jun 11, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #23405 — identical one-line fix using path.as_posix() for .sh/.bash script paths on Windows so Git Bash doesn't mangle backslashes. #23405 (open) is the earliest; #23489 and #43076 take the same approach.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the focused Windows cron fix. The production hunk addresses a defect still present on current main: cron/scheduler.py:2092 passes str(path) to bash for .sh/.bash scripts.

Problems

  • The proposed regression assertion cannot fail on a POSIX host. hermes_env creates a host-native Path (tests/cron/test_cron_no_agent.py:21), so its argument already has / before this change; the added assertions at lines 366-367 of the PR diff therefore pass with the old str(path) implementation.
  • The linked #46332 reports a separate Windows failure: current cron/scheduler.py:2083 still uses shutil.which("bash"), which can select the WSL launcher ahead of Git Bash. This PR fixes path formatting but not that resolver case.

Suggested changes

  • Make the path-format regression meaningful on native Windows, using an actual Windows Path and a captured subprocess argv; skip outside win32 unless extracting a testable cross-platform helper.
  • Keep Git-Bash-versus-WSL selection as a focused follow-up for #46332.

Automated hermes-sweeper review.

assert ok is True
assert output == "ok"

# Second argument is the script path.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

On a POSIX runner, script_path is already a Path with / separators, so this assertion also passes before the production change. Please make this a native-Windows regression using a real Windows path (or extract and test a platform-independent conversion helper).

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 14, 2026
@andrexibiza

Copy link
Copy Markdown
Contributor

Reconciliation (2026-08-03): this PR's change (path.as_posix() for .sh scripts on Windows) is the Layer-2 path half, now canonical in #77393 (fix(cron): pass Git-Bash-safe script path to bash on Windows — all CI checks green, mergeable_state: CLEAN), which uses the established _bash_safe_path MSYS convention, also fixes the sibling tools/approval.py hint, and adds regression tests + docs. This branch is conflicting against main with no CI checks reported. Recommend closing as a duplicate of #77393.

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 duplicate This issue or pull request already exists 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 sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: .sh cron scripts fail on Windows - bash receives mangled backslash paths

4 participants