fix: add windowsHide to every child_process call (#149) - #151
Merged
Conversation
Runs the exact detached-parent + execFileSync option shapes used by keeper.js and the git.ts call sites on a real windows-latest runner, polling Win32 IsWindowVisible to confirm whether a console window actually appears. Isolates windowsHide as the only variable so we can verify the already-applied fix AND check whether the src/indexer/git.ts call sites (not covered by the fix) exhibit the same bug. workflow_dispatch only — not wired to any push/PR trigger. Temporary, pending a decision on whether to keep it.
workflow_dispatch requires the file to exist on the default branch to be dispatchable via API/UI — GitHub platform restriction, not something fixable here. Adding a push trigger scoped to this exact branch instead, so it fires without touching main.
Killing conhost/WindowsTerminal/OpenConsole indiscriminately after case 1 took out the pipe backing the runner's own pwsh session, crashing case 2 with "No process is on the other end of the pipe." Only ping.exe (the actual stray this test spawns) needs cleanup.
Adds a regression guard (tests/windows-hide-coverage.test.ts) that statically scans every execFile/execFileSync/spawn call site in src/ and hooks/ for windowsHide — including promisify() aliases like execFileAsync, which a naive name match misses (caught this via a deliberate falsification: removing windowsHide from git.ts and confirming the test failed at the right line, then restoring it). Also extends watch.ps1 with a second A/B pair that runs harness.mjs directly (no detached parent) — the shape hooks/*.mjs actually run under as a foreground child of the host CLI, distinct from the daemon's detached self-relaunch already covered.
On Windows, every execFile/execFileSync/spawn call without windowsHide: true pops a visible console window on screen. Because keeper.js's daemon runs detached with no console of its own, this hit the daemon's own self-relaunch plus every git/ripgrep call it makes while indexing — a constant, disruptive flash for as long as the daemon is active. Original patch and repro by @lnorton89 (#149), covering 7 call sites: keeper.js's self-relaunch, kb/git.ts, kb/commit.ts, init.ts, server/find.ts (x2), server/searcher.ts. This adds two sets of sites the original audit missed: - src/indexer/git.ts: 4 execFileAsync('git', ...) calls (getGitHead/getGitChangedFiles/getGitExactRenames/getGitNewFiles/ getGitDirtyFiles). getGitHead alone fires on every debounced keeper cache save (~5s after any edit settles while the daemon runs), and the others fire on every keeper-start reconcile — likely the highest-frequency trigger of the whole bug. - hooks/*.mjs (nudge-handler, codex-nudge-handler, elicit-core, kb-recall, cursor-kb-recall, codex-kb-recall): these ship with the package and get wired into a user's project by `coldstart init`, firing on every prompt submission in Claude Code/Cursor/Codex — a higher-frequency trigger than the daemon itself. Verified on a real windows-latest GitHub Actions runner: an A/B test polling the Win32 IsWindowVisible API confirms a visible window appears for the unpatched option shape and does not for the patched one, across both the daemon's detached-parent spawn and the hooks' plain foreground invocation (see scripts/win-repro/, temporary, not part of the package). Also adds tests/windows-hide-coverage.test.ts as a permanent, cross-platform regression guard: it statically scans every execFile/execFileSync/spawn call site (including promisify() aliases) and fails if any lacks windowsHide, so a future call site can't reintroduce this gap silently.
The earlier -Direct case ran harness.mjs straight from pwsh, which already has a console, so nothing flashed either way — that's not the condition hooks/*.mjs runs under. Claude Code/Cursor are Electron apps; on Windows their main process is a GUI-subsystem executable, which never has a console regardless of launch method (the same "no console" condition keeper.js's detached daemon has, already confirmed to flash). NoConsoleLauncher.cs compiles to /target:winexe (genuinely no console, built independently of Node's own spawn machinery, to rule out the daemon result being a Node-specific spawn quirk) and launches node with CreateNoWindow=true on that specific hop — isolating the test to what this repo's code controls (harness.mjs's own inner execFileSync call) rather than conflating it with whether plain node itself gets an unrelated auto-allocated console.
…rocess Start-Process launching NoConsoleLauncher.exe directly from pwsh still left pwsh's own console reachable in the inheritance chain (a plain launch doesn't sever it — only an explicit detach like DETACHED_PROCESS does), so that attempt was testing the same non-vulnerable condition as the earlier -Direct case in different clothes. orchestrator-noconsole.mjs routes through Node's detached:true spawn — already proven (via the daemon case, where the outer spawn call is identical across both A/B runs and only the inner windowsHide toggle explains the differing result) to genuinely sever console inheritance — before handing off to the WinExe launcher. This keeps the same clean isolation: only harness.mjs's inner execFileSync windowsHide toggle differs between the two runs.
It's done its job for the daemon/CLI shape (CI-confirmed) and the GUI-subsystem shape (still inconclusive after several attempts — see PR description). Once merged to main it becomes properly dispatchable on demand instead of needing a branch-scoped push hack.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| : { bin: 'xdg-open', args: [file] }; | ||
| try { | ||
| const child = execFile(cmd.bin, cmd.args, () => {}); | ||
| const child = execFile(cmd.bin, cmd.args, { windowsHide: true }, () => {}); |
…S for #149 The hooks-shape repro case (proxying "Electron app spawns a hooks/*.mjs subprocess") was inconclusive: NoConsoleLauncher.exe used .NET's ProcessStartInfo.CreateNoWindow on the intermediate node.exe process, which showed NO console flash in either the patched or unpatched branch. That's not evidence of safety — a bug can't be shown fixed if the unpatched baseline never reproduces it. Verified on real Windows 11 hardware (not just CI) that the root cause is CreateNoWindow leaving the process with a hidden-but-still-attached console object rather than genuinely none, so its own child (ping.exe) just reused that hidden console instead of requesting a new (visible) one, regardless of windowsHide. DetachedLauncher.cs calls Win32 CreateProcess directly with the real DETACHED_PROCESS flag, giving the child no console object at all — the actual condition hooks/*.mjs runs under when spawned by a console-less GUI host. Under that condition the split is exactly as expected: unpatched flashes a new console window, patched shows nothing. Confirmed both by direct human visual observation and the automated IsWindowVisible poll, repeatably. This directly closes the "hooks shape: not independently proven" gap called out in the PR description.
Addresses a CodeQL finding on this PR: the workflow had no explicit permissions block, so GITHUB_TOKEN defaulted to broader access than it needs. It only checks out code and runs a local script, so contents: read is sufficient.
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
2 participants
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
On Windows, every
execFile/execFileSync/spawncall withoutwindowsHide: truepops a visible console window. Becausekeeper.js's daemon runs detached with no console of its own, this hit the daemon's own self-relaunch plus everygit/ripgrep call it makes while indexing — a constant, disruptive flash for as long as the daemon is active.Closes #149.
Credit
Original repro, root-cause diagnosis, and patch for 7 call sites by @lnorton89 in #149 (PR access wasn't available to them as a non-collaborator, so their fix was posted directly on the issue). This PR incorporates their patch as-is and extends it — see below.
What this covers
Original 7 sites (from #149, unchanged):
keeper.js's self-relaunch,kb/git.ts,kb/commit.ts,init.ts,server/find.ts(×2),server/searcher.ts.Additional sites found and fixed in this PR:
src/indexer/git.ts— 4execFileAsync('git', ...)calls (getGitHead,getGitChangedFiles,getGitExactRenames,getGitNewFiles/getGitDirtyFiles), missed by the original audit.getGitHeadalone fires on every debounced keeper cache save (~5s after any edit settles while the daemon runs), and the others fire on every keeper-start reconcile — likely the single highest-frequency trigger of the whole bug.hooks/*.mjs(nudge-handler,codex-nudge-handler,elicit-core,kb-recall,cursor-kb-recall,codex-kb-recall) — ~13 more call sites. These ship with the npm package and get wired into a user's project bycoldstart init, firing on every prompt submission in Claude Code/Cursor/Codex.tests/windows-hide-coverage.test.ts(new, permanent) — a cross-platform static regression guard. Scans everyexecFile/execFileSync/spawncall site insrc/andhooks/(resolvingpromisify()aliases likeexecFileAsync, and stripping comments to avoid false matches) and fails if any lackswindowsHide. Verified it actually catches a regression by deliberately removing the flag fromgit.tsmid-session and confirming the test failed at the right line before restoring it. This is what prevents a future call site from reintroducing the gap silently — the runtime repro below only replays hardcoded shapes, it can't scan new code.Verification
Ran a real A/B test on a
windows-latestGitHub Actions runner (seescripts/win-repro/, temporary tooling, not part of the package) polling the actual Win32IsWindowVisibleAPI:keeper.js's detached self-relaunch +git.ts): confirmed a visible window appears for the unpatched shape and does not for the patched shape. This is solid, direct evidence.hooks/*.mjsruns when spawned by Claude Code/Cursor/Codex): tried three different mechanisms to reproduce the equivalent "no console ancestor" condition from CI (direct foreground exec, a compiled GUI-subsystem launcher viaStart-Process, the same launcher nested under Node's provendetached: truespawn) — none reproduced a flash either way. The most likely explanation is thatwindowsHide/CREATE_NO_WINDOWgives a process a hidden console rather than no console at all, so once any ancestor in the chain has one (hidden or visible), downstream children just reuse it instead of requesting a new one — meaning CI can't cleanly isolate this specific case without literally running Electron. The fix is still correct and safe to ship regardless (windowsHideis a documented no-op wherever it isn't needed), applied on the same reasoning the original patch used for its own non-daemon sites (kb/commit.ts,init.ts, etc. weren't individually CI-proven either) — it just isn't independently CI-confirmed for the hooks specifically. Planning to confirm on real Windows hardware and will follow up here.npm run buildclean,npm test→ 747/747 passed (49 test files).Test plan
npm run build/npm testlocally