Skip to content

fix: normalize Windows backslashes to forward slashes in cron bash script paths (#60857) - #60892

Closed
kyssta-exe wants to merge 1 commit into
NousResearch:mainfrom
kyssta-exe:fix/60857-cron
Closed

fix: normalize Windows backslashes to forward slashes in cron bash script paths (#60857)#60892
kyssta-exe wants to merge 1 commit into
NousResearch:mainfrom
kyssta-exe:fix/60857-cron

Conversation

@kyssta-exe

Copy link
Copy Markdown
Contributor

On Windows, MSYS2/Git Bash interprets backslashes as escape sequences, not path separators. Convert backslashes to POSIX forward slashes before passing the script path to bash.

@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 P2 Medium — degraded but workaround exists duplicate This issue or pull request already exists sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Jul 8, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.
Duplicate of #23405 — same POSIX-path conversion at the same code site (cron/scheduler.py::_run_job_script), fixing the Windows/Git-Bash backslash-mangling of .sh script paths. #23405 (open, 2026-05-10) is the earliest of the cluster; #44350 and #46364 are the same family. Also relates to the spec issue #60857. The fix is genuinely still needed (main still passes str(path) unconverted) — a maintainer should pick the canonical PR from this cluster.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Comment (token read-only)

PR 60892 normalizes Windows backslashes to forward slashes in cron bash script paths. Small fix (1 file, 5 additions, 1 deletion) for cross-platform compatibility.

LGTM - awaiting maintainer approval.

@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 targeting a real native-Windows cron failure: current main still passes str(path) to Bash at cron/scheduler.py:2091, matching the reports in #60857 and #23404.

Problems

  • cron/scheduler.py:2094 in this PR uses str(path).replace("\\", "/") for every host. That rewrites literal backslashes in valid POSIX filenames, so it can change a validated script path into a different path on Linux/macOS. path.as_posix() performs the intended Windows conversion without that POSIX behavior change.

Suggested changes

  • Replace the blanket string replacement with path.as_posix() for the Bash argv element.
  • Add a focused regression assertion with PureWindowsPath; the existing shell-script test at tests/cron/test_cron_no_agent.py:288 does not cover Windows serialization. Closed PR #43076 contains a minimal example of that test shape.

Automated hermes-sweeper review.

Comment thread cron/scheduler.py
# On Windows, MSYS2/Git Bash interprets backslashes as escape
# sequences, not path separators. Convert to POSIX forward
# slashes so C:\Users\... doesn't become C:Users... (#60857).
script_arg = str(path).replace("\\", "/")

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.

Please use path.as_posix() here rather than replacing backslashes unconditionally. On POSIX, \ is a valid filename character, so this replacement can turn a validated script path into a different path; as_posix() still converts native Windows paths as required.

@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 10, 2026
@kyssta-exe

Copy link
Copy Markdown
Contributor Author

Stale — no merge activity for 4-6 days. Can resubmit if still needed.

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-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.

4 participants