test(hooks): probe for a bash that runs, not one that exists - #2916
test(hooks): probe for a bash that runs, not one that exists#2916sashankh wants to merge 1 commit into
Conversation
…y-Labs#2915) `shutil.which("bash") is None` is not an availability check on Windows. Every Windows 10/11 install ships `C:\Windows\System32\bash.exe` - the WSL launcher - and System32 precedes Git Bash on PATH, so `which` always answers non-None. With no WSL distribution provisioned the launcher prints a UTF-16 notice and exits 1 without running the script, so the four allowlist tests failed on `bash exited 1` rather than skipping, while the working `C:\Program Files\Git\bin\bash.exe` further down PATH went unused. Walk every bash on PATH in order and probe each the same way the harness invokes it - a script file, run from the working directory - then take the first that answers. A probe that asked an easier question than the test (`bash --version`, say) would reintroduce exactly the mismatch Graphify-Labs#2641 fixed by baking the payload into a file. The interpreter is now passed as an absolute path; only argv[1:] goes through the MSYS re-parse that mangles backslashes, so the script argument stays a bare filename as before. On Windows without WSL this turns 2 failures into 4 tests that actually run against Git Bash; on Linux the first PATH hit still wins on the first probe. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Graphify reviewed this change.
Looks safe to merge — no coupling regressions and no blocking issues, checked against the code graph (not a self-assessment).
Graphify review — findings
Replaces the shutil.which("bash") skip guard in test_hooks.py with _resolve_bash, which probes each bash on PATH by running a real script file and picks the first that works, so Windows' System32 WSL launcher no longer causes allowlist tests to fail with bash exited 1 instead of skipping. Routes all bash invocations through the resolved _BASH and the _requires_bash marker, and adds two tests covering the reject/keep-looking behavior.
No blocking issues surfaced. 2 lower-confidence candidates did not survive cross-model review.
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 144 functions depend on the 144 functions this change touches.
Health — grade A; no new coupling hotspots.
Verification — 144 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 144 function(s) in the blast radius were not formally verified this run
Fixes #2915.
The problem
The four
#2641allowlist tests are gated on:@pytest.mark.skipif(shutil.which("bash") is None, reason="bash required to exercise emitted glob")On Windows that guard never fires, because
shutil.which("bash")finds theWSL launcher, not a shell:
System32precedes Git Bash onPATH, sowhichreturns the launcher. Withno WSL distribution provisioned it prints a UTF-16 notice and exits 1 without
running the script:
So on a stock Windows dev box
pytest tests/is red, the message points at theallowlist pattern rather than at the environment, and the coverage
#2641added— specifically so this would stop being vacuous on Windows — is not obtained at
all, even though a perfectly good bash is sitting two
PATHentries away.The change
_resolve_bash()walks everybashonPATHin order and probes each onethe same way the harness invokes it: a script file, run from the working
directory. The first that answers
okwins; if none does, the tests skip.The probe deliberately mirrors the real invocation. A cheaper check such as
bash --versionwould reintroduce precisely the mismatch#2641removed whenit stopped letting the payload transit argv — a guard that asks an easier
question than the test guards nothing.
The interpreter is now passed as an absolute path. That is safe:
argv[0]isconsumed by
CreateProcess, and onlyargv[1:]goes through the MSYS re-parsethat strips backslashes — so the script argument stays a bare filename, exactly
as
#2641requires.Two regression tests cover the guard itself, using
sys.executableas astand-in launcher (it exists and runs, but cannot execute a shell script):
PATH.Verification
Windows 11 / Python 3.12.10, Git Bash present, no WSL distribution:
pytest tests/test_hooks.py -qThe four allowlist tests now genuinely execute against
C:\Program Files\Git\bin\bash.exerather than failing or being skipped:Full suite on the same box goes from
1 failed, 1841 passed, 14 skippedto nofailures.
On Linux (CI) behaviour is unchanged: the first
PATHhit is a real bash andpasses the first probe, costing one extra
subprocess.runat import.Tests only — no change to
graphify/.