Skip to content

fix(cron): convert Windows backslash script path to MSYS form for git-bash - #62516

Open
fhucko wants to merge 3 commits into
NousResearch:mainfrom
fhucko:fix/windows-cron-bash-path
Open

fhucko wants to merge 3 commits into
NousResearch:mainfrom
fhucko:fix/windows-cron-bash-path

Conversation

@fhucko

@fhucko fhucko commented Jul 11, 2026

Copy link
Copy Markdown

Summary

Fixes .sh/.bash cron jobs failing on Windows with exit 127 ("No such file or directory"). The scheduler passed a Windows backslash path directly to git-bash, which treats \ as an escape and collapses the path so the script is never found.

Root cause

In cron/scheduler.py, _run_job_script() did argv = [_bash, str(path)]. On Windows str(path) is a backslash path (C:\Users\...\x.sh); git-bash collapses it to C:Users... → exit 127. The script never runs. .py cron scripts are unaffected (run via sys.executable).

Fix

Normalize the path to MSYS form (/c/Users/...) when sys.platform == "win32", guarded so POSIX behavior is unchanged.

Test plan

  • Reproduced: nightly .sh cron job failed exit 127 on Windows 10
  • After patch: same job runs via hermes cron run → success, pushes to GitHub
  • ast.parse clean; no change to POSIX path handling
  • Regression tests added in tests/cron/test_cron_no_agent.py: test_run_job_script_windows_argv_uses_msys_path captures the argv passed to subprocess.run and asserts a native C:\... script path is converted to /c/... (no backslashes); test_run_job_script_posix_argv_unchanged asserts the POSIX argv is passed through verbatim.

Refs #62514

🤖 Generated with Claude Code

…-bash

On Windows, _run_job_script() passed str(path) (backslash form, e.g.
C:\Users\...\x.sh) directly to git-bash, which treats '\' as an escape
char and collapses the path so the script is never found (exit 127).
Normalize to MSYS form (/c/Users/...) before invoking bash. Fixes
.sh/.bash cron jobs on Windows.

Refs NousResearch#62514
@alt-glitch alt-glitch added type/bug Something isn't working comp/cron Cron scheduler and job management platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows P2 Medium — degraded but workaround exists duplicate This issue or pull request already exists labels Jul 11, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Duplicate of #23405 (earliest-open PR for the Windows-cron git-bash POSIX/MSYS path mechanism at the same _run_job_script site). Related to #62514 (this PR's spec issue, deduped into #23404) and #60892 (sibling fix). The fuller drive-letter conversion here is a minor extra over the same mechanism.

@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 addressing a real native-Windows cron failure. Current main still constructs shell-script argv as [_bash, str(path)] in cron/scheduler.py:2091, so the fix targets the live defect and covers both no-agent and pre-run script callers.

Problems

  • No regression test covers the new Windows-only conversion. The existing shell tests at tests/cron/test_cron_no_agent.py:288-309 execute .sh/.bash scripts but do not verify the path passed to bash.

Suggested changes

  • Add a focused Windows regression test that captures bash argv and asserts a native C:\\... script path is converted to /c/...; retain an assertion that POSIX argv is unchanged.

Automated hermes-sweeper review.

Comment thread cron/scheduler.py
# "C:Users..." so the file is never found (exit 127). Convert to the
# MSYS form git-bash expects (/c/Users/...) before invoking bash.
bash_path = str(path)
if sys.platform == "win32":

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.

Please add regression coverage for this Windows-only branch. Existing shell-script tests execute .sh/.bash files but do not assert the argv passed to bash; capture subprocess.run and verify a native drive path becomes /c/....

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 11, 2026
fhucko added 2 commits July 11, 2026 18:24
Capture the argv handed to git-bash and assert a native C:\... script path
is converted to /c/... (the exit-127 fix in NousResearch#62516). Also assert POSIX
argv is passed through unchanged, plus a focused unit test on the
extracted _bash_arg_for_script_path helper.
…sResearch#62516)

Capture the argv passed to subprocess.run and assert a native C:\... script
path is converted to /c/... on Windows (regressing the exit-127 failure),
with a companion test asserting POSIX argv is passed through unchanged.
The fix in cron/scheduler.py is otherwise left exactly as the maintainer
requested.
@fhucko

fhucko commented Jul 11, 2026

Copy link
Copy Markdown
Author

Added the regression coverage as requested. Both new tests are in tests/cron/test_cron_no_agent.py:

  • test_run_job_script_windows_argv_uses_msys_path — captures the argv passed to subprocess.run and asserts a native C:... script path is converted to /c/... (no backslashes), regressing the original exit-127 failure.
  • test_run_job_script_posix_argv_unchanged — asserts the POSIX argv is passed through verbatim (no /c/ MSYS transformation).

cron/scheduler.py is otherwise unchanged from the fix. Both tests pass against the real code. PTAL.

@monerostar

Copy link
Copy Markdown
Contributor

Native Win11 verification (monerostar)

Host: Windows 11 build 26200, Python 3.11.15. Bash on PATH is Hermes-bundled Git/MSYS bash 5.3.9 (…\hermes\git\usr\bin\bash.EXE).

Bug class is real

Passing a native backslash path as argv to bash can collapse separators (POSIX-style escape handling), producing the classic missing-file path:

native: C:\Users\Admin\AppData\Local\Temp\…\nightly.sh
# one invocation path:
err: /bin/bash: C:UsersAdminAppDataLocalTemp…nightly.sh: No such file or directory  (rc=127)

That matches the PR description (exit 127 / mangled C:Users…).

This PR’s conversion

C:\Users\Admin\…\nightly.sh  →  /c/Users/Admin/…/nightly.sh

Focused unit tests on this branch:

pytest tests/cron/test_cron_no_agent.py::test_run_job_script_windows_argv_uses_msys_path \
       tests/cron/test_cron_no_agent.py::test_run_job_script_posix_argv_unchanged \
       -o addopts= -v
# 2 passed

Live with Hermes bash + MSYS argv form outside %TEMP%: script runs (LIVE_OK, rc=0).
C:/… forward-slash form also runs on this bash binary.

Notes for maintainers

  • Sibling/overlapping approach: fix(cron): use path.as_posix() for .sh scripts on Windows #44350 (path.as_posix()C:/… rather than /c/…). Both target the same Windows .sh cron argv problem; worth picking one canonical form (or accepting either if bash on PATH is Git Bash).
  • Discovery is still shutil.which("bash") — fine when Git Bash is first on PATH.
  • On this host the usertemp mount maps %TEMP%/tmp; exercising scripts under AppData\Local\Temp can behave differently than under e.g. C:\Users\Admin\… depending on how argv is rewritten. Conversion still looks correct for normal HERMES_HOME/scripts/*.sh locations.

Verdict: LGTM for the Windows branch + tests. Happy to defer if maintainers prefer consolidating with #44350’s as_posix style.

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 platform/windows Native Windows-specific behavior or breakage sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

4 participants