Skip to content

fix(cron): prefer Git Bash and POSIX paths on Windows (#46332) - #46364

Open
Tranquil-Flow wants to merge 1 commit into
NousResearch:mainfrom
Tranquil-Flow:fix/46332-windows-bash-posix-paths
Open

fix(cron): prefer Git Bash and POSIX paths on Windows (#46332)#46364
Tranquil-Flow wants to merge 1 commit into
NousResearch:mainfrom
Tranquil-Flow:fix/46332-windows-bash-posix-paths

Conversation

@Tranquil-Flow

Copy link
Copy Markdown
Contributor

Fixes #46332.

Summary

  • Prefer Git for Windows bash over WSL bash.exe when running .sh/.bash cron scripts on native Windows.
  • Pass shell script paths to bash in forward-slash form on Windows so MSYS/Git Bash does not treat backslashes as escape characters.
  • Preserve existing fallback behavior when Git Bash is not installed and preserve non-Windows path behavior.

Why

Issue #46332 reports two separate Windows-only root causes:

  1. shutil.which("bash") may resolve to WSL bash before Git Bash; WSL bash cannot execute Windows paths even when they use forward slashes.
  2. Git Bash/MSYS mangles backslash-separated Windows paths such as C:\Users\... into C:Users....

This PR covers both layers. Existing open PRs I found (#23405, #23489, #43076, #44350) only address the POSIX/forward-slash path layer and do not resolve the WSL bash preference layer.

Verification

  • /Users/evinova-self/.hermes/hermes-agent/venv/bin/python3 -m pytest tests/cron/test_cron_no_agent.py -v -o "addopts=" --tb=short
    • Result: 26 passed, 1 warning in 0.48s
  • Branch verification after rebase: git rev-list --left-right --count upstream/main...HEAD0 1

Notes

Auto-published by Moonsong via Path B automated pipeline.

@Tranquil-Flow
Tranquil-Flow force-pushed the fix/46332-windows-bash-posix-paths branch from ae72e81 to 76018f2 Compare June 15, 2026 00:24
@liuhao1024

Copy link
Copy Markdown
Contributor

Verification — reviewed diff, no issues found.

Checked:

  • _resolve_bash() correctly prioritizes Git for Windows directories over WSL bash, with fallback to shutil.which() then /bin/bash
  • _bash_script_path_arg() applies as_posix() only on win32, keeping native paths on Unix
  • _GIT_BASH_DIRS are literal Windows paths (not os.path.join'd) to avoid forward-slash divergence in tests
  • _run_job_script integration: the POSIX conversion is applied to argv[1] only for .sh/.bash suffixes
  • Tests cover both Windows (forward-slash assertion) and Unix (native path assertion) code paths
  • No behavioral change on non-Windows platforms — the _resolve_bash() fallback is identical to the old inline logic

Clean fix for issue #46332.

@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 addressing a real native-Windows cron failure. Current main still invokes shell scripts as [_bash, str(path)] after shutil.which("bash") (cron/scheduler.py:2077-2092), so both the WSL-first and backslash-path premises remain valid.

Problems

  • The new resolver at cron/scheduler.py:914-920 only checks Git\\usr\\bin\\bash.exe. It bypasses the established resolution contract in tools/environments/local.py:561-598: HERMES_GIT_BASH_PATH, Hermes-managed PortableGit/MinGit, and standard Git-for-Windows Git\\bin\\bash.exe all precede PATH. A normal Git\\bin or Hermes PortableGit install can therefore still fall through to WSL bash.

Suggested changes

  • Reuse or extract the existing _find_bash() resolution logic, while retaining the Windows-only path.as_posix() argv conversion.
  • Add resolver tests for the custom override, Hermes portable install, and standard Git\\bin before asserting the PATH fallback.

Automated hermes-sweeper review.

Comment thread cron/scheduler.py Outdated
On non-Windows platforms ``shutil.which("bash")`` is sufficient.
"""
if sys.platform == "win32":
for _dir in _GIT_BASH_DIRS:

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.

This duplicated resolver skips the supported HERMES_GIT_BASH_PATH, Hermes PortableGit locations, and standard Git\\bin\\bash.exe layout implemented by tools/environments/local.py:_find_bash. Reuse or extract that resolution chain; otherwise normal Git/PortableGit installs can still fall through to WSL via PATH.

@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
Tranquil-Flow pushed a commit to Tranquil-Flow/hermes-agent that referenced this pull request Jul 14, 2026
…NousResearch#46364)

Sweeper feedback: replace hardcoded Git\usr\bin resolver with
established _find_bash() from tools/environments.local which checks
HERMES_GIT_BASH_PATH, PATH, and Git\bin locations.

Updated tests to cover custom override, standard Git\bin, and
RuntimeError fallback.
@andrexibiza

Copy link
Copy Markdown
Contributor

Reconciliation (2026-08-03): thank you for the diagnosis and the two-layer structure — the Layer 1 (resolver) / Layer 2 (path form) framing is reused in the canonical. Current state: this branch is now conflicting against main (Jun 15 base). The resolver half ships via #77532, which reuses the repository's shared _find_bash() (merged #47837) rather than re-implementing Git-for-Windows directory checks — covering this PR's cases plus Hermes' portable-Git install location. The path half ships via #77393 (all CI green, CLEAN). Recommend closing this PR as superseded by #77532 + #77393; your diagnosis is credited in #77532's body.

@luckystar2026

Copy link
Copy Markdown

Verified locally on Windows 10 + desktop gateway process — the approach in this PR is correct and matches what I needed to fix the same bug (#43073). Preferring Git Bash over WSL's bash.exe and converting paths to /c/... MSYS form are both necessary; either one alone is insufficient.

One thing worth double-checking: on current main, tools/environments/local.py already contains _find_bash() and _bash_safe_path(), but cron/scheduler.py is NOT wired to use them for .sh jobs (it still passes the raw resolved path to whatever bash it finds). That suggests this PR may be only partially merged, or the scheduler integration regressed. Please confirm #46364 includes the scheduler.py wiring.

Local verification data (against main + local patch):

  • _find_bash()C:\Program Files\Git\bin\bash.exe (Git Bash; WSL's C:\Windows\System32\bash.exe is correctly skipped)
  • _bash_safe_path('C:/Users/<user>/AppData/Local/hermes/scripts/evolution-run.sh')/c/Users/<user>/AppData/Local/hermes/scripts/evolution-run.sh (backslash form converts identically)
  • End-to-end: cron .sh job executed successfully after the fix (previously 5/5 jobs failed with exit 127 / E_UNEXPECTED)

Thanks for the PR — happy to help with anything else needed to get it merged.

@alt-glitch alt-glitch added platform/windows Native Windows-specific behavior or breakage needs-decision Awaiting maintainer decision before any implementation labels Aug 10, 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 needs-decision Awaiting maintainer decision before any implementation 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.

[Windows] Cron jobs with .sh scripts fail because WSL bash is picked over Git Bash + backslashes get eaten by MSYS"

6 participants