Skip to content

fix(cron): normalize bash script paths on Windows - #23489

Open
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:codex/23404-cron-bash-path
Open

fix(cron): normalize bash script paths on Windows#23489
LeonSGP43 wants to merge 1 commit into
NousResearch:mainfrom
LeonSGP43:codex/23404-cron-bash-path

Conversation

@LeonSGP43

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes native Windows Git Bash cron .sh / .bash script execution by normalizing the script argument to a forward-slash path before invoking bash.

Without this, _run_job_script() passes a native Windows path like C:\Users\denis\.hermes\scripts\hermes-backup.sh, and Git Bash treats the backslashes as escape characters, mangling the path into C:Usersdenis... and failing with exit code 127.

Related Issue

Fixes #23404

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • Added _format_bash_script_path() in /cron/scheduler.py to centralize bash path formatting.
  • Switched .sh / .bash execution in _run_job_script() to use a bash-safe path argument on Windows.
  • Added regression tests in /tests/cron/test_scheduler.py for Windows and POSIX path formatting behavior.

How to Test

  1. Run uv run --frozen pytest -q -o addopts='' tests/cron/test_scheduler.py
  2. Run uv run --frozen ruff check cron/scheduler.py tests/cron/test_scheduler.py
  3. On native Windows with Git Bash, configure a cron job using a .sh script and verify the script path reaches bash in forward-slash form instead of losing backslashes.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS (targeted regression tests); Windows path behavior covered by explicit unit tests

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

  • uv run --frozen pytest -q -o addopts='' tests/cron/test_scheduler.py -> 123 passed
  • uv run --frozen ruff check cron/scheduler.py tests/cron/test_scheduler.py -> All checks passed!
  • The repo-wide local pytest tests/ -q entrypoint still fails during collection on both this branch and a clean origin/main worktree with the same preexisting environment-level errors (tomllib, acp, websockets.asyncio, StrEnum), so I left that checklist item unchecked rather than claiming a false full-green run.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cron Cron scheduler and job management duplicate This issue or pull request already exists labels May 11, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Duplicate of #23405 which addresses the same issue (#23404) with an identical approach (path.as_posix() on Windows).

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused Windows compatibility fix. The premise remains live on current main: cron/scheduler.py:2091 still invokes Bash with str(path), while the .sh/.bash branch begins at cron/scheduler.py:2076. The proposed platform-gated conversion changes only the Bash script argument and leaves Python script execution unchanged.

The common _run_job_script() helper covers ordinary script injection (cron/scheduler.py:2190), no-agent jobs (cron/scheduler.py:2543), and wake-gated jobs (cron/scheduler.py:2632), so no sibling cron invocation bypasses this change. The added path-format tests cover both Windows and POSIX formatting behavior.

A separate open item, #46364, documents WSL bash.exe winning PATH resolution on some Windows systems; that is an additional interpreter-selection concern rather than a defect in this PR's stated path-normalization scope.

Automated hermes-sweeper review.

@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 13, 2026
@andrexibiza

Copy link
Copy Markdown
Contributor

Independent verification of this fix against current main (HEAD 6858e0d931) — dedup-campaign re-check (issue cluster #23404 / #65317).

Premise still live on current main: cron/scheduler.py _run_job_script() at L2325 still builds argv = [_bash, str(path)] — a native Windows path with backslashes passed verbatim to MSYS/Git-bash, which treats \ as escape sequences and collapses C:\Users\... to C:Users... → exit 127. Reproduced the mechanism: with the older MSYS runtime that ships in bundled hermes\git, the backslash argv is mangled; both forward-slash forms (C:/... as this PR produces, and /c/...) run clean.

Merge verification (3-way against origin/main 6858e0d):

  • cron/scheduler.py: auto-merged except one trivial conflict — main added _read_windows_pyvenv_cfg/_windows_cron_python_invocation + a workdir param to _run_job_script after the PR's base; resolved by keeping both (PR's _format_bash_script_path helper + main's Windows-python invocation helpers and signature).
  • tests/cron/test_scheduler.py: one trivial import-line conflict (main added _resolve_cron_enabled_toolsets, _merge_mcp_into_per_job_toolsets); resolved by keeping both imports.
  • No other conflicts.

Tests (merged tree, direct pytest, Windows native git-bash):

  • tests/cron/test_scheduler.py + tests/cron/test_cron_no_agent.py: 70 passed (incl. both new TestFormatBashScriptPath cases).
  • git diff --check clean; scripts/check-windows-footguns.py --diff origin/main shows only a pre-existing flag at tests/cron/test_scheduler.py:1142 (unrelated SKILL.md write_text in another test class, present on clean main, not introduced by this diff).

Recommendation: this is the cluster's canonical fix — the only open PR with a sweeper keep_open salvageability=high verdict, focused tests, and a clean current-main merge. Sibling PRs to consolidate onto this one: #23405 (same as_posix mechanism, no tests, no verdict), #62516 (same mechanism, fuller /c/... conversion, AI-triaged as dup of #23405), #60617 (out of scope — WSL-launcher-stub facet plus an unrelated bundled third-party memory plugin).

@alt-glitch alt-glitch added P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage and removed P3 Low — cosmetic, nice to have sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 3, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #52204: both patches normalize Windows cron shell-script paths; #52204 remains the open canonical PR.

@alt-glitch alt-glitch added duplicate This issue or pull request already exists and removed duplicate This issue or pull request already exists labels Aug 3, 2026
@andrexibiza

Copy link
Copy Markdown
Contributor

CI on this branch is stale (May 11 base) — not caused by this PR — and the fix is a strict subset of the open canonical #52204, which is fully green

Diagnosis of the three failing checks (runs 25645674966 / 25645675001):

  • test (9 failed) — all in tests/gateway/test_tts_media_routing.py, tests/gateway/test_update_streaming.py, tests/hermes_cli/test_update_gateway_restart.py, tests/hermes_cli/test_web_server.py, tests/run_agent/test_async_httpx_del_neuter.py. None touch cron/; this PR's diff is only cron/scheduler.py + tests/cron/test_scheduler.py. Those files were reworked on main since May 11 (e.g. 39975613b1 "test: prune wave 2", 7729c183b4 "test: restore four silently shadowed definitions").
  • e2e (9 failed) — reset_session mock assertions in tests/e2e/test_platform_commands.py; that file also changed on main since (ef8936d599, f96b2e6ef7).
  • Windows footguns (blocking) (2 found) — both at tools/process_registry.py:588 (os.killpg + bare signal.SIGKILL), a file this PR never touches. The bare usage is gone on current main; that file has been rewritten since May 11 (cd6585abf8 refactor, 0b149ca030 fix, 80e4fb5995 fix).

No failure in any failing job is attributable to this PR's two files.

On the fix itself: _format_bash_script_path() (path.as_posix() when os.name == "nt") is functionally identical to what open canonical #52204 already ships in _run_job_script()script_arg = path.as_posix() if sys.platform == "win32" else str(path) — and #52204 additionally fixes the sibling half this PR does not address: _find_bash() skipping the System32 WSL launcher (bash.exe exiting 1 with no distro installed). #52204 is green on all required checks and regression-tests both halves: TestCronShellScriptPosixArg asserts the exact backslash-mangling case (assert "\\" not in captured["argv"][1]) and TestFindBashSkipsWslLauncher pins the launcher behavior. This PR's change is a strict subset. (#23405 is the same cluster and equally covered.)

Premise check on current main (fe6330de): cron/scheduler.py:2291 still builds argv = [_bash, str(path)] — the bug class is live and unshipped; the work is preserved in #52204, which is why the sweeper's keep_open intent is honored by closing here rather than losing the fix.

Recommendation: close as duplicate of #52204 (matches the triage verdict). The fix and its regression tests are already carried there with green CI; nothing in this branch is lost.

@andrexibiza

Copy link
Copy Markdown
Contributor

Correction to my earlier comment: the canonical for this cluster is #77393 (open, all required checks green, mergeable_state: clean against current main fe6330de), not #52204.

Recommendation stands with the corrected pointer: close this PR as a duplicate of #77393. Nothing in this branch is lost — the as_posix() approach here is subsumed by _bash_safe_path().

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.

fix(cron): bash script path mangled by backslash escaping on native Windows

4 participants