fix(sync): run gstack-brain-sync through bash, not cmd.exe, on Windows - #2510
Open
ShahriarLak wants to merge 1 commit into
Open
fix(sync): run gstack-brain-sync through bash, not cmd.exe, on Windows#2510ShahriarLak wants to merge 1 commit into
ShahriarLak wants to merge 1 commit into
Conversation
The brain-sync stage failed on EVERY Windows run with "is not recognized as an internal or external command", so /sync-gbrain always reported ERR brain-sync among otherwise green stages. garrytan#1731 gave these spawns shell: NEEDS_SHELL_ON_WINDOWS. That is correct for the gbrain.cmd shim and does nothing here: shell:true routes through cmd.exe, which resolves .cmd/.bat via PATHEXT but has no concept of a shebang, so an extension-less bash script is rejected outright. A .cmd shim needs a shell; a shebang script needs an interpreter. The two cases look identical and are not. The failure was quiet rather than loud. artifacts_sync_mode defaults to pushing curated artifacts to git, so a Windows user's learnings piled up uncommitted in ~/.gstack indefinitely while the sync report showed one red line out of four. New bashScriptInvocation() resolves Git for Windows' bash explicitly and passes the script as argv[0]. It prefers Git bash over a bare `bash` on PATH because WindowsApps ships a bash.exe that is the WSL launcher, which would read C:\... as a Linux path; GSTACK_BASH overrides for unusual installs; forward slashes because bash treats backslashes as escapes; and it returns null when no bash exists so the stage says so plainly instead of surfacing an unactionable spawn error. The garrytan#1731 tripwire asserted the shape that does not work, so it now asserts the opposite (never a raw spawnSync(brainSyncPath, ...)) and six unit tests cover the resolver. Verified on Windows: the stage now reports "OK brain-sync curated artifacts pushed (4.2s)" and the artifacts repo committed + pushed on its own. Affected-test set unchanged at 14 pre-existing failures before and after, with 6 new passing tests.
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
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
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.
The bug
On Windows the
brain-syncstage of/sync-gbrainfails on every run:Why the #1731 fix didn't cover it
#1731 added
shell: NEEDS_SHELL_ON_WINDOWSto these spawns. That is correct for thegbrain.cmdshim and does nothing here:shell: trueroutes through cmd.exe, which resolves.cmd/.batvia PATHEXT but has no concept of a shebang, so an extension-less bash script is rejected outright.A
.cmdshim needs a shell. A shebang script needs an interpreter. The two cases look identical and are not, which is why one fix appeared to cover both.Why it matters more than one red line
artifacts_sync_modedefaults to pushing curated artifacts to git. With this stage dead, a Windows user's learnings accumulate in~/.gstackand are never committed, while the sync report shows one red line among otherwise green stages. It reads as a minor stage failure and is actually silent data stagnation.The change
New
bashScriptInvocation()inlib/gbrain-exec.ts:bash.exeand passes the script asargv[0].bashon PATH: WindowsApps ships abash.exethat is the WSL launcher, and if it wins PATH order it readsC:\...as a Linux path and the script never sees the repo.GSTACK_BASHoverrides for unusual installs.nullwhen no bash exists, so the stage reportsskipped (no bash found; set GSTACK_BASH to your Git bash.exe)rather than an unactionable spawn error.POSIX is unchanged: the script is exec'd directly, no interpreter, no shell.
Tests
The #1731 tripwire asserted the shape that does not work (
spawnSync(brainSyncPath, ...)+shell: NEEDS_SHELL_ON_WINDOWS), so it now asserts the opposite and fails if a raw spawn returns. Six unit tests cover the resolver: POSIX passthrough, Git-bash resolution, path slashing,shell: false,GSTACK_BASHoverride, and the no-bashnull.The
gbrain.cmdassertions in that file are untouched — that part of #1731 is right and still enforced.Verification (Windows 11, gstack 1.61.0.0, bun 1.3.11)
Before:
ERR brain-sync gstack-brain-sync exited 1 (0.0s)After:
OK brain-sync curated artifacts pushed (4.2s)— and the artifacts repo committed and pushed on its own.The suite is nondeterministic on Windows (the same tree gave 743 and 741 failures on back-to-back runs), so I compared the affected files rather than the whole run: 14 failures before, 14 after, with 6 new passing tests. No behaviour change on POSIX, where the invocation is byte-identical to today's.
🤖 Generated with Claude Code