Skip to content

fix(windows): route bash tool and shell hooks through Git Bash, fix ruff SIM117 lint - #32

Closed
truecallerabreham wants to merge 1 commit into
he-yufeng:mainfrom
truecallerabreham:fix/windows-bash-hooks-ci
Closed

truecallerabreham wants to merge 1 commit into
he-yufeng:mainfrom
truecallerabreham:fix/windows-bash-hooks-ci

Conversation

@truecallerabreham

Copy link
Copy Markdown

Root Cause

Currently, upstream CI is red across all Windows lanes (windows-latest, Python 3.10, 3.11, 3.12, 3.13) and the lint job:

  1. Ruff SIM117 in tests/test_core.py: A nested with block (with mock.patch(...): with pytest.raises(...):) fails ruff check corecoder tests.
  2. Windows Shell Execution in BashTool and Hooks: On Windows, Python's subprocess.run(command, shell=True) defaults to cmd.exe (%COMSPEC%), where POSIX commands (pwd, ls, sh, cat, sleep) are not recognized. Commit ae0c9aa introduced norm_dir anticipating git-bash's /c/Users/... output format, but BashTool and Hooks were executing in cmd.exe rather than Git Bash.
  3. MSYS2 /tmp Mount Point on Windows: In Git Bash, pytest's tmp_path under AppData/Local/Temp resolves to /tmp/pytest-of-..., causing norm_dir comparisons to fail if the /tmp prefix is not expanded to tempfile.gettempdir().

Changes

  • corecoder/tools/bash.py: Added a cached _find_bash() helper locating Git Bash on Windows (C:\Program Files\Git\bin\bash.exe, relative to git, or in PATH outside System32). If found, BashTool.execute runs [bash_exe, "-c", command]; otherwise it falls back to shell=True.
  • corecoder/hooks.py: Routed shell hook commands through Git Bash on Windows when present, and automatically quoted drive-letter executables (sys.executable).
  • tests/test_core.py: Combined the nested with statements on line 717 to satisfy SIM117, normalized /tmp to tempfile.gettempdir() on Windows in norm_dir, and used target.as_posix() in the parallel cd test.
  • tests/test_hooks.py: Used .as_posix() when interpolating paths into shell commands (sh, cat).
  • README.md & README_CN.md: Synchronized the engine LoC and physical/net line count badges with the current tree (1,324 engine LoC, 2,621 physical lines, 2,113 net), keeping test_readme_line_counts_are_current green.

Local Verification

  • uv run ruff check corecoder tests -> All checks passed
  • python -m compileall -q corecoder tests -> 0 errors
  • uv run pytest tests/ -> 178 passed in 28.24s (full suite green on Windows)

On Windows, subprocess with shell=True defaults to cmd.exe (%COMSPEC%), where POSIX commands (pwd, ls, sh, cat, sleep) fail, causing all Windows test lanes to fail in CI.

- Route BashTool and Hooks through Git Bash on Windows when present, falling back to default shell when unavailable.
- In hooks._fire, quote Windows drive-letter executables when invoking bash.
- In tests/test_core.py, combine nested with contexts to fix ruff SIM117, and normalize Git Bash's /tmp mount point in norm_dir.
- In tests/test_hooks.py, use .as_posix() for path interpolation in shell command strings.
- Synchronize README line count badges and prose with the current tree.

All 178 tests passing across the full suite; ruff check clean; compileall clean.
@truecallerabreham

Copy link
Copy Markdown
Author

Verified full CI matrix (14/14 jobs) green on GitHub Actions:

  • package: Success
  • lint (ruff): Success
  • ubuntu-latest (Python 3.10, 3.11, 3.12, 3.13): 4/4 Success
  • macos-latest (Python 3.10, 3.11, 3.12, 3.13): 4/4 Success
  • windows-latest (Python 3.10, 3.11, 3.12, 3.13): 4/4 Success

Workflow run receipt: https://github.com/truecallerabreham/CoreCoder/actions/runs/35665113803

@he-yufeng

Copy link
Copy Markdown
Owner

Thanks for the thorough diagnosis — it was right on all three counts. Main's CI has been red on lint (the SIM117 nested with) and every windows-latest lane (POSIX commands going through cmd.exe), and the MSYS2 /tmp mount broke the parallel-cwd comparison.

I landed the fix as my own commit (8a44515) rather than merging this, per the project's habit of keeping external PRs as reference and landing reviewed work directly. What it does, with credit to your analysis:

  • one shared corecoder/shell.py routes every shell spawn: Git Bash on Windows (Program Files first, then the git on PATH, never System32's WSL bash), platform default elsewhere;
  • BashTool and the shell hooks both go through it;
  • the SIM117 merge and the /tmp expansion in the parallel-cwd test;
  • unit tests for the routing so this doesn't silently regress.

CI should go green on the next run. Thanks again for the careful root-cause work.

@he-yufeng he-yufeng closed this Sep 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants