Skip to content

fix(windows): add windowsHide to all Windows spawn resolution paths#48320

Closed
TerminalsandCoffee wants to merge 1 commit intoopenclaw:mainfrom
TerminalsandCoffee:fix/windows-acpx-console-flash
Closed

fix(windows): add windowsHide to all Windows spawn resolution paths#48320
TerminalsandCoffee wants to merge 1 commit intoopenclaw:mainfrom
TerminalsandCoffee:fix/windows-acpx-console-flash

Conversation

@TerminalsandCoffee
Copy link
Copy Markdown

@TerminalsandCoffee TerminalsandCoffee commented Mar 16, 2026

Summary

  • Adds windowsHide: true to the direct and shell-fallback resolution paths in src/plugin-sdk/windows-spawn.ts
  • These were the only two Windows-path resolutions missing the flag — all other paths (node-entrypoint, exe-entrypoint) already set it
  • All downstream spawn call sites (acpx runtime, lobster, ACP client) correctly forward windowsHide from the resolution, so the fix propagates automatically
  • Adds test coverage for direct .exe resolution asserting windowsHide: true
  • Updates shell-fallback test expectation to include windowsHide: true
  • Adds windowsHide: true to raw spawn() calls in test files to prevent console flash during test runs on Windows

Root Cause

resolveWindowsSpawnProgramCandidate() only set windowsHide: true for node-entrypoint and exe-entrypoint resolutions. The direct path (e.g. .exe files resolved on Windows) and the shell-fallback path (unresolved .cmd wrappers) left windowsHide as undefined, causing every ACP spawn to flash a visible cmd.exe console window.

Test plan

  • pnpm vitest run extensions/acpx/src/runtime-internals/process.test.ts — 15/15 passing
  • New test: sets windowsHide on direct exe resolution on windows
  • Updated test: falls back to shell mode now asserts windowsHide: true
  • No-op on macOS/Linux — windowsHide is ignored on non-Windows platforms

Closes #40340

🤖 Generated with Claude Code

@TerminalsandCoffee @claude
fix(windows): add windowsHide to all Windows spawn resolution paths
8d36db3 
The direct and shell-fallback resolution paths in the plugin SDK's
Windows spawn resolver were missing windowsHide: true, causing visible
cmd.exe console windows to flash on every ACP spawn on Windows desktop.

Closes openclaw#40340

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 16, 2026

Greptile Summary

This PR closes a Windows UX regression where every ACP spawn would flash a visible cmd.exe console window by adding windowsHide: true to the two resolution paths (direct and shell-fallback) that were previously missing the flag in resolveWindowsSpawnProgramCandidate / applyWindowsSpawnProgramPolicy.

  • Core fix (src/plugin-sdk/windows-spawn.ts): windowsHide: true added to the direct return (line 252) and to the shell-fallback return inside applyWindowsSpawnProgramPolicy (line 275). The remaining paths (node-entrypoint, exe-entrypoint) already carried the flag, so all four resolution branches are now consistent.
  • New test (process.test.ts): "sets windowsHide on direct exe resolution on windows" exercises the previously uncovered direct path with an absolute .exe path on a simulated win32 runtime.
  • Updated test (process.test.ts): The "falls back to shell mode" expectation is updated to assert windowsHide: true, aligning with the production change.
  • Housekeeping (process.test.ts, mcp-proxy.test.ts): Raw spawn() calls in test files now include windowsHide: true to suppress console flash during CI runs on Windows — these do not affect test logic.

Confidence Score: 5/5

  • This PR is safe to merge — it is a minimal, targeted bug fix with full test coverage and no behavioral change on non-Windows platforms.
  • The change is confined to two one-line additions in well-isolated helper functions. All resolution paths are now consistently marked with windowsHide: true on Windows. The fix propagates automatically to all downstream spawn call sites through the existing forwarding logic in applyWindowsSpawnProgramPolicy. New and updated tests cover both previously-untested paths. The change is a no-op on macOS/Linux.
  • No files require special attention.

Last reviewed commit: 8d36db3

@BradGroux BradGroux mentioned this pull request Mar 17, 2026
Closed
@johnsonshi
Copy link
Copy Markdown
Contributor

johnsonshi commented Mar 18, 2026

I reviewed this change and the implementation looks sound. The patch fills the two missing Windows resolution outcomes in src/plugin-sdk/windows-spawn.ts—direct and shell-fallback—by setting windowsHide: true, which matches the gap here.

I also checked the surrounding call sites, and the resolved windowsHide value is already propagated through the relevant ACPX / ACP client / lobster spawn paths, so this should flow through as intended without additional plumbing in those layers.

The tests line up with the change: there’s a new direct-.exe resolution assertion, the shell-fallback expectation now checks windowsHide: true, and the small test-only raw spawn() updates are consistent with avoiding Windows console flashes during test runs.

From a code and scope perspective, this looks reasonable to land.

Copilot AI mentioned this pull request Mar 27, 2026
Closed
17 tasks
@BingqingLyu BingqingLyu mentioned this pull request Apr 27, 2026
Open
4 tasks
@clawsweeper
Copy link
Copy Markdown
Contributor

clawsweeper Bot commented Apr 28, 2026 •
edited
Loading

Codex review: needs changes before merge.

Summary
The branch adds windowsHide: true to the shared Windows direct and shell-fallback spawn resolver paths and updates ACPX-related test spawn expectations.

Reproducibility: yes. Source inspection on current main shows Win32 direct .exe resolution and opt-in shell fallback both materialize without windowsHide, and the SDK shell-fallback test currently expects that missing value.

Next step before merge
A narrow automated repair can preserve the valid resolver fix, relocate coverage to the current SDK test file, and add the missing changelog without a product decision.

Security
Cleared: The diff only changes spawn option plumbing and tests; it does not alter workflows, dependencies, lockfiles, scripts, downloads, secrets, permissions, or package publishing metadata.

Review findings

  • [P2] Move Windows spawn coverage to the SDK test — extensions/acpx/src/runtime-internals/process.test.ts:134-153
  • [P3] Add the required changelog entry — src/plugin-sdk/windows-spawn.ts:249
Review details

Best possible solution:

Land or replace this PR with the shared resolver fix on current main, SDK-level regression tests, and a changelog entry, while leaving MCP proxy production spawn work to #60678.

Do we have a high-confidence way to reproduce the issue?

Yes. Source inspection on current main shows Win32 direct .exe resolution and opt-in shell fallback both materialize without windowsHide, and the SDK shell-fallback test currently expects that missing value.

Is this the best way to solve the issue?

No for the branch as currently shaped. Setting windowsHide in the shared resolver is the narrow maintainable production fix, but the tests need to move to src/plugin-sdk/windows-spawn.test.ts and the changelog entry needs to be added before merge.

Full review comments:

  • [P2] Move Windows spawn coverage to the SDK test — extensions/acpx/src/runtime-internals/process.test.ts:134-153
    The new direct .exe and shell-fallback assertions are added to extensions/acpx/src/runtime-internals/process.test.ts, but current main no longer has that test file. After a rebase this either conflicts or resurrects stale extension-local coverage; put these resolver cases in src/plugin-sdk/windows-spawn.test.ts next to the shared helper being changed.
    Confidence: 0.9
  • [P3] Add the required changelog entry — src/plugin-sdk/windows-spawn.ts:249
    This changes user-facing Windows spawn behavior for ACP/Codex/Docker consumers, but the branch does not update CHANGELOG.md. Add a single-line entry under the active Fixes section before merge.
    Confidence: 0.84

Overall correctness: patch is incorrect
Overall confidence: 0.88

Acceptance criteria:

  • pnpm test src/plugin-sdk/windows-spawn.test.ts src/acp/client.test.ts extensions/codex/src/app-server/transport-stdio.test.ts src/agents/sandbox/docker.windows.test.ts
  • pnpm exec oxfmt --check --threads=1 src/plugin-sdk/windows-spawn.ts src/plugin-sdk/windows-spawn.test.ts CHANGELOG.md
  • pnpm check:changed in Testbox after the rebase or replacement branch

What I checked:

Likely related people:

  • Takhoffman: Introduced the shared non-core Windows spawn handling across ACP, QMD, and Docker that this resolver patch now extends. (role: introduced behavior; confidence: high; commits: cd653c55d7ce; files: src/plugin-sdk/windows-spawn.ts, src/acp/client-helpers.ts, src/agents/sandbox/docker.ts)
  • steipete: Authored the earlier ACPX Windows wrapper resolver sharing and strict wrapper default work that led into the shared resolver path. (role: adjacent owner; confidence: medium; commits: 12c12570238c, 68a8a98ab761; files: src/plugin-sdk/windows-spawn.ts, extensions/acpx/runtime-api.ts)
  • vincentkoc: Exported and maintained the Windows spawn SDK seam and nearby test/helper surfaces after the shared resolver moved into plugin SDK. (role: recent maintainer; confidence: high; commits: b4f16bad327c, db7f4d3193f0; files: src/plugin-sdk/windows-spawn.ts, src/plugin-sdk/windows-spawn.test.ts, src/plugin-sdk/AGENTS.md)
  • Agustin Rivera: Recently changed the unresolved Windows wrapper fail-closed behavior and tests around the same shell-fallback policy. (role: recent adjacent maintainer; confidence: medium; commits: 5874a387aefa; files: src/plugin-sdk/windows-spawn.ts, src/plugin-sdk/windows-spawn.test.ts, docs/plugins/sdk-migration.md)

Codex review notes: model gpt-5.5, reasoning high; reviewed against 5f373ae4d3db.

@BradGroux BradGroux mentioned this pull request Apr 29, 2026
Draft
@clawsweeper clawsweeper Bot mentioned this pull request May 1, 2026
Open
This was referenced May 6, 2026
Open
Closed
@XuZehan-iCenter XuZehan-iCenter mentioned this pull request May 6, 2026
Closed
XuZehan-iCenter added a commit to XuZehan-iCenter/openclaw that referenced this pull request May 6, 2026
@XuZehan-iCenter
fix(io): add windowsHide to login shell env probe on Windows
15171d4 
The execLoginShellEnvZero helper was missing windowsHide: true, causing
a PowerShell console window to flash briefly during gateway startup on
Windows. This completes the fix started in openclaw#48320 and openclaw#70308 for the
v2026.5.4 code path.

Closes openclaw#78159
XuZehan-iCenter added a commit to XuZehan-iCenter/openclaw that referenced this pull request May 6, 2026
@XuZehan-iCenter
fix(io): add windowsHide to login shell env probe on Windows
d2693c2 
The execLoginShellEnvZero helper was missing windowsHide: true, causing
a PowerShell console window to flash briefly during gateway startup on
Windows. This completes the fix started in openclaw#48320 and openclaw#70308 for the
v2026.5.4 code path.

Closes openclaw#78159
@BradGroux
Copy link
Copy Markdown
Member

BradGroux commented May 8, 2026

Thanks for the fix here. I rechecked this against current main while working through the Microsoft tracker, and this PR is now obsolete rather than mergeable as-is.\n\nCurrent main already carries the Windows spawn hiding behavior in the shared Windows spawn helper paths, including windowsHide: true for the resolved Windows candidates, and the linked issue #40340 has been closed as implemented after maintainer review. The old Matter/ACPX-era branch would now re-touch stale code around behavior that has moved forward.\n\nClosing this as superseded by the current implementation rather than rebasing stale changes. Thanks @TerminalsandCoffee for reporting and patching the original Windows console flash path.

@BradGroux BradGroux closed this May 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

extensions: acpx size: XS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug(acpx): Windows console windows flash on every ACP spawn — missing windowsHide: true

3 participants

@TerminalsandCoffee @johnsonshi @BradGroux