fix(windows): pass windowsHide to the where.exe helper lookups - #333
Merged
Conversation
#256 covered the summarizer-CLI spawns but not the binary lookups that precede them. Both run `where.exe` on Windows from inside detached workers, which have no console to inherit, so each lookup allocates its own visible window — the same flash, one layer earlier. resolveCliBin is reached from the backfill path via stage-memory; spawn-mine-local-worker's is the fallback when the bundled cli.js is missing. Found by codex review of #256.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughSubprocess discovery and detached worker launches now pass ChangesWindows-hidden subprocess processes
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
wiki-worker-windows.test.ts asserts the exact options object passed to execFileSync, so adding windowsHide to the where/which lookup broke both the win32 and the posix case. Exact-match is the right assertion here — it's what would catch the option silently disappearing — so update the expectations rather than loosen them.
spawn-wiki-worker.test.ts asserts the same exact options object as wiki-worker-windows.test.ts, for the codex/cursor/hermes resolvers. Both files had to move together; I only ran the files I touched, so CI caught the second one.
Contributor
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 9 files changed
Generated for commit fe5ee09. |
The first commit fixed two binary lookups but not the worker launches they feed, so the console still appeared one step later. Review found five more sites: - spawn-mine-local-worker.ts and spawn-backfill-memory-worker.ts launch their workers detached - client.ts auto-spawns the embedding daemon, reached from the backfill path - pi's auto-mine and openclaw's skillify launcher each pair a lookup with a detached spawn detached maps to DETACHED_PROCESS, which makes Windows ignore CREATE_NO_WINDOW — but libuv sets SW_HIDE from windowsHide as well, and that still applies, which is why spawn-detached.ts has always paired the two. These launches were simply missing it. Also corrects a comment: the mine-local lookup runs before its detached worker, not inside it. pi and openclaw hardcode Unix `which` for those lookups, so they cannot succeed on Windows at all. That is a separate bug — a feature silently absent rather than a visible console — and fixing it would enable code paths that never run there today, so it is marked with a NOTE and left for its own change. Found by codex review.
… lookup Ten more detached launches that fire during an ordinary session, so a missing SW_HIDE is the same visible flash #331 reported from a different worker: pi's embedding daemon, skillopt, wiki and skillify workers; the shared skillopt worker and standalone embedding daemon; codex's session-start setup; shared autoupdate; openclaw's graph build and pull. Also fixes the hardcoded Unix `which` rather than documenting it. The previous commit added windowsHide to those two lookups and left a NOTE saying they cannot succeed on Windows — which made the option a no-op there. Both now select where/which by platform, and pi takes the first non-empty line because `where` prints one match per line. Not included: git and provisioning calls made from inside workers that are already launched (graph-deps, graph-on-stop, deeplake-pull, docs candidates). Same mechanism, but this change already spans five subsystems; that tail belongs in its own sweep. Found by codex review.
Omitted from the previous commit's pathspec, so the source still had the hardcoded "which" while its guard asserted the platform-selected form. Local runs passed on the working tree; CI tested the committed tree and caught it.
…ction Two launches that windowsHide could not have helped, because neither could execute on Windows in the first place. openclaw's graph pull spawned `nohup node ...`. nohup is POSIX-only, so that ENOENT'd on Windows and the pull worker never ran there. Spawn process.execPath directly, as graph-on-stop already does — detached + unref is what provides survival. pi's launcher lookup took the first non-empty `where` line, but that can be an extensionless shim (not runnable) or a .cmd (needs a shell), and pi spawned either directly. Mirror src/utils/resolve-cli-bin.ts: prefer .exe, then .cmd/.bat, else first match; route a .cmd/.bat through a shell with only the fixed subcommand on the command line. Found by codex review.
Under `shell: true` Node concatenates file + args into one command string with no escaping, so an unquoted path containing a space is parsed as two tokens and the spawn fails. This is not hypothetical: npm ships claude.cmd with no .exe, so every Windows npm user takes the shell branch, and the default global bin for an account with a space in its name is C:\Users\Jane Doe\AppData\Roaming\npm\claude.cmd. Those users' summary runs fail — which is exactly what drives the respawn loop fixed in #332. Adds shellFile() beside binNeedsShell in resolve-cli-bin.ts and applies it to all three invocation builders. pi's launcher gets the same treatment, and its predicate is now win32-gated to match binNeedsShell — a POSIX file merely named *.cmd must still spawn directly. Behavioral tests cover a spaced .cmd path through every builder, plus a negative case: the non-shell argv path must NOT be quoted, since there a quoted path is a literally wrong filename. Found by codex review.
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.
Problem
Follow-up to #256 (merged) from codex's review of it. #256 added
windowsHide: trueto the summarizer-CLI spawns, but not to the binary lookups or the worker launches around them.On Windows a
detachedprocess getsDETACHED_PROCESSand has no console to inherit, so anything it launches —where.exe, agitcall, a worker — allocates its own visible console. That is the same flash #331 reported, from other workers.What this covers
21 spawn sites across six subsystems, found over four review rounds:
resolve-cli-bin.ts,spawn-mine-local-worker.tsdetached: truemakes Windows ignoreCREATE_NO_WINDOW, but libuv also setsSW_HIDEfromwindowsHide, and that still applies — which is whyspawn-detached.tshas always paired the two. These launches were simply missing it.Two Windows bugs found along the way
Neither is a console-flash issue; both meant the code could not run on Windows at all, so adding
windowsHideto them would have been a no-op.whichin pi's launcher fallback and openclaw's agent-gate detection. Both threw on every Windows call, so pi's auto-mine never resolved a binary and openclaw always reported "no agent found". Both now selectwhere/whichby platform. pi additionally mirrorsresolve-cli-bin.ts's selection — prefer.exe, then.cmd/.bat, else first match — becausewhereprints one match per line and an extensionless shim is not directly runnable; a.cmd/.batlauncher is routed through a shell with only the fixed subcommand on the command line.nohupin openclaw's graph pull worker. ItENOENT'd on Windows, so that worker has never run there. Now spawnsprocess.execPathdirectly, matchinggraph-on-stop;detached+unrefis what provided survival.This is NOT a complete sweep
gitand provisioning calls made from inside already-launched workers still lackwindowsHide—graph-deps.ts,graph-on-stop.ts:163,deeplake-pull.ts:270,docs/candidates.ts,commands/docs.ts.graph-on-stopand pull invokegitduring ordinary stop/start flows, so Windows console flashes are not fully eliminated by this PR. Deferred deliberately: this change already spans six subsystems, and that tail is its own sweep. Filing as a tracked follow-up.Deliberately not changed:
commit-kpi-extract.ts:121It pairs
detached: truewithwindowsHide: true. Codex rated the pairing P1 on the grounds that Windows ignoresCREATE_NO_WINDOWunderDETACHED_PROCESS— true, but libuv still suppliesSW_HIDE, so the flag is not inert. Droppingdetachedwould trade a documented process-survival guarantee for that, on a pathcapture.tscurrently has disabled, and unverifiable from here. Codex agreed with leaving it.Verification
flush-memory, tree-sitter-dependentcli-bundle-runtimeand two graph-build tests) fail identically onmainwithout this branch.windowsHideto each specific call so it fails if the option drifts out; the functional openclaw test asserts the spawn shape directly.tsc --noEmitclean.Not verified on Windows. No line of this has executed on a Windows machine. The mechanism follows from the documented
CREATE_NO_WINDOW/SW_HIDEsemantics and has been checked by codex across four rounds, but that is inference, not evidence.Refs #331
Summary by CodeRabbit
Bug Fixes
windowsHide: true) for detached subprocesses across embedding/daemon/workers and lifecycle hooks.where/shim handling) and hardened wiki/prompt worker command execution.nohupbehavior.Tests
windowsHide: trueduring CLI lookups and detached worker launches, including updated graph pull expectations.