Repository navigation
fix(windows): suppress the console window on the last spawn site (#1427) - #1485
Merged
Merged
Conversation
#1448 set CREATE_NO_WINDOW on the subprocess spawn and on cbm_popen_isolated, but cbm_exec_no_shell (compat_fs.c) still passed a bare 0 for dwCreationFlags. That is the helper behind git, codesign and open, so the most frequently hit path kept flashing a console — and under a stdio MCP session with auto_watch those windows steal focus while the user is typing. Reported by @noctrex on #1448 after verifying the merged fix. Also adds the contract nobody had. The flag has to be set per call site, the four sites do not resemble each other, and reviewing "does this PR add the flag" says nothing about the sites a PR does not touch — which is exactly how the third one survived a fix applied twice: daemon/bootstrap.c had it from the start (DETACHED_PROCESS | ... | flag) subprocess.c added by #1448 (flags variable) compat_fs.c :313 added by #1448 (inline literal) compat_fs.c :689 missed by #1448 (bare 0) tests/test_spawn_no_window_contract.sh asserts the whole-tree property instead, so a fifth spawn site cannot be added without it. The contract strips C comments before matching, and that is load-bearing rather than tidiness: the first version of it PASSED with the fix reverted, because the explanatory comment above the fixed call contains the flag name and satisfied the match. A contract that can be satisfied by prose about the contract is a false guard. Verified in both directions afterwards — green with the fix, and red naming compat_fs.c:696 with the fix reverted and that comment still in place. Co-Authored-By: noctrex <noctrex@users.noreply.github.com> Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
DeusData
enabled auto-merge
August 7, 2026 12:58
Contributor
|
You da man |
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.
Follow-up to #1448 (thanks @mlandolfi90), closing out #1427.
The missed site
#1448 set
CREATE_NO_WINDOWon the subprocess spawn and oncbm_popen_isolated, butcbm_exec_no_shell(compat_fs.c) still passed a bare0fordwCreationFlags. That's the helper behindgit,codesignandopen— the most frequently hit path — so those console windows survived the merge. Spotted by @noctrex after verifying the merged fix on Windows 11, and credited on the commit.Why a contract test comes with it
The flag must be set at each call site independently, the sites don't resemble each other, and reviewing "does this PR add the flag" says nothing about the sites a PR doesn't touch. That's exactly how the third one survived a fix applied twice:
daemon/bootstrap.cDETACHED_PROCESS | … | CREATE_NO_WINDOW)subprocess.ccompat_fs.c:313compat_fs.c:6890)tests/test_spawn_no_window_contract.shasserts the whole-tree property instead, so a fifth spawn site can't be added without the flag. It's registered as Step 0k inscripts/test.sh, which runs the contracts first on every leg.One thing worth flagging in review
The contract strips C comments before matching, and that's load-bearing rather than tidiness. The first version of it passed with the fix reverted — my explanatory comment above the fixed call contains the string
CREATE_NO_WINDOW, which satisfied the match. A contract that can be satisfied by prose about the contract is a false guard, and this repo has been bitten by string-matching contracts before.After the fix, verified in both directions:
OK: all 4 CreateProcessW site(s) set CREATE_NO_WINDOWFAIL: … src/foundation/compat_fs.c:696 (cbm_exec_no_shell) spawns without CREATE_NO_WINDOW, exit 1It also fails if it ever finds zero call sites, so it can't quietly stop checking anything if the spawn layer moves.
Verification
make lint-ciclean; contract green both directions; exec-bit contract still passes (invoked viabash, and the file is committed100755to match its siblings).