fix(scripts): make pr-wait.ps1 usable under Windows PowerShell 5.1 - #600
Merged
Conversation
AGENTS.md documents `scripts\pr-wait.ps1 <n>` as the Windows equivalent of
pr-wait.sh, but it had never been run under powershell.exe -- only under pwsh,
which hides all four of these.
Windows PowerShell 5.1 decodes a BOM-less script as the system ANSI codepage,
not UTF-8. The em dash in a string literal became `â€"` -- and that third byte
is CP1252 U+201D, a character PowerShell's tokenizer accepts as a string
delimiter. So the literal closed early and swallowed the rest of the file,
reported 75 lines away as "The Try statement is missing its Catch or Finally
block". Every .ps1 in the repo is now ASCII, with scripts/powershell_ascii_test.py
holding the invariant (CI already runs scripts/*_test.py). Checked on the bytes
rather than on string literals alone: a comment's em dash is one copy-paste from
being a message's, and the failure is remote from the cause either way.
Running it then surfaced three more, all from `$ErrorActionPreference = "Stop"`:
- Write-Error is promoted to a *terminating* error, so it unwound before the
`exit` on the next line and every documented non-zero code came back as -1.
Exit 2 and exit 8 are what callers switch on.
- PowerShell wraps each line a native command *redirects* to stderr in a
NativeCommandError, which "Stop" makes fatal. `gh pr checks` reports "no
checks reported" on stderr, so the wait loop died on precisely the state it
exists to sit through; likewise a 404 from the annotations endpoint and a job
with no failed-step log, leaving the script's own "(no failed-step log
available)" fallback unreachable. Unredirected stderr is untouched, which is
why the opening `gh pr view` worked and this stayed hidden.
- The --jq filter reached gh in pieces ("unknown shorthand flag: ')'") because
5.1 does not escape the quotes inside a native argument. The JSON is shaped in
PowerShell instead, byte-identical to the sh twin's output.
Verified against real PRs under powershell.exe: 0 (merged, and all-pass), 1
(three failing checks, annotations and logs fetched), 2 (no such PR, and no
checks appeared), 8 (head moved).
…t.py The file claimed shutil.which "honours PATH order and finds Git Bash". It does honour PATH order -- and on a stock Windows install System32 is ahead of Git, so `bash` resolves to C:\Windows\System32\bash.exe, the WSL launcher. WSL's filesystem has no F:\, and this test hands bash an absolute Windows path, so all seven tests failed with exit 127 for a script that was plainly there. CI is Ubuntu and never exercises that branch, so it stayed green throughout. Git for Windows' bash is now preferred explicitly, located next to git rather than searched for, and anything under %SystemRoot% is skipped. On Linux and macOS the added candidates do not exist and PATH's bash is returned unchanged. Also adds a test for the resolution itself: without it the mix-up arrives as seven CalledProcessErrors reporting exit 127, which reads like the log summarising being broken rather than bash never having opened the file. Verified in the shell that reproduced it: which() still returns the System32 launcher, find_bash() returns C:\Program Files\Git\bin\bash.exe, and all scripts/*_test.py pass.
Owner
Author
|
/no-changelog Local tooling only: makes scripts\pr-wait.ps1 and its unit test run under Windows PowerShell 5.1. Nothing in a shipped artifact changes -- no runtime, service, config, packaging or published docs is touched, and the only files are scripts/.ps1, compat/.ps1 and scripts/*_test.py. |
Contributor
No changelog fragment, deliberately
The check is green and this PR ships with no line in the release notes. The waiver covers the PR, not the commit it was written on — if later work here |
Compatibility Tests✅ No regressions against the baseline
Pass rate (excluding skips and N/A): 100.0% |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
scripts\pr-wait.ps1— documented in AGENTS.md as the Windows equivalent ofpr-wait.sh— could not run at all underpowershell.exe. It now works, and exits with the documented 0/1/2/8 status codes..ps1in the repo is ASCII, with a unit test holding the invariant.scripts/pr-wait_test.pyno longer resolvesbashto the WSL launcher on Windows, which made all seven of its tests fail on a dev box while CI stayed green.Verification
All against Windows PowerShell 5.1 (
5.1.26100.8972, ANSI codepage Windows-1252), and real PRs in this repo:PR_WAIT_APPEAR_TIMEOUT)bash scripts/pr-wait.sh 500— byte-identical, so the twins stay behaviorally identical.ghstub onPATH. Triggering them for real would have meant pushing to PRs I do not own.for t in scripts/*_test.py— all 11 green, including the two added here..ps1re-parsed with[Parser]::ParseFileunder 5.1: 0 errors across all 7 files.Notes
Root cause. Windows PowerShell 5.1 decodes a BOM-less script as the system ANSI codepage, not UTF-8.
—(UTF-8E2 80 94) becomesâ€", and that last byte is CP1252 U+201D — a character PowerShell's tokenizer accepts as a string delimiter. So a literal containing an em dash closed early and swallowed the rest of the file, reported 27 lines further on as "The Try statement is missing its Catch or Finally block".pwshreads UTF-8 and is unaffected, which is why this survived.Fixed by making the files ASCII rather than adding a BOM: it keeps them BOM-less, and
scripts/powershell_ascii_test.pycan then enforce it. That test checks the bytes, not just string literals — a comment's em dash is one copy-paste away from being a message's, and the failure is remote from the cause either way.compat/dev.ps1andcompat/run.ps1had the same characters and are included.Three further bugs, found by actually running it. All from
$ErrorActionPreference = "Stop", and none reachable before the file parsed:Write-Erroris promoted to a terminating error, so it unwound before theexiton the next line — every documented non-zero code came back as-1. That is precisely what callers switch on.NativeCommandError, whichStopmakes fatal.gh pr checksreports "no checks reported" on stderr, so the wait loop died on the exact state it exists to sit through; likewise a 404 from the annotations endpoint and a job with no failed-step log, leaving the script's own(no failed-step log available)fallback unreachable. Unredirected stderr is untouched, which is why the openinggh pr viewworked and this stayed hidden.--jqfilter reachedghin pieces (unknown shorthand flag: ')') because 5.1 does not escape quotes inside a native argument. The JSON is shaped in PowerShell instead.Changelog. None — this is local developer tooling and changes nothing in a shipped artifact. Waived on the PR with a reason, per
.changelog/README.md.Follow-up, deliberately not fixed here. The
CONFLICTINGguard is dead code in both twins. GitHub'smergeStateStatushas noCONFLICTINGmember (that belongs to the separatemergeablefield); a conflicting PR reportsDIRTY. Verified live: PR #395 is genuinely conflicting, printedmerge state: DIRTY, and sailed past the guard into the very timeout it was written to prevent. It affects both twins and changes behaviour, so it belongs in its own change.