fix(stdio): always set windowsHide on Windows, not just in Electron - #1640
Conversation
Sorry, something went wrong.
🦋 Changeset detectedLatest commit: 63ff657 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Sorry, something went wrong.
npm i https://pkg.pr.new/@modelcontextprotocol/sdk@1640 commit: 63ff657 |
Sorry, something went wrong.
|
+1 — hitting this exact bug |
Sorry, something went wrong.
The windowsHide spawn option was only enabled for Electron apps, causing an empty cmd.exe console window to appear when launching MCP servers from non-Electron Node.js applications on Windows. Since windowsHide is a no-op on non-Windows platforms, just set it to true unconditionally. Also removes the now-unused isElectron() helper. Fixes modelcontextprotocol#1638
|
Thanks for this. Rebased onto latest v1.x and made two small tweaks:
Will also forward-port to main separately. Fixes #1638. |
Sorry, something went wrong.
felixweinberger
left a comment
There was a problem hiding this comment.
LGTM
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM — straightforward bug fix with good test coverage.
Extended reasoning...Overview
This PR changes windowsHide in StdioClientTransport.start() from process.platform === "win32" && isElectron() to process.platform === "win32", removing the now-unused isElectron() helper. A changeset and two new tests (verifying windowsHide: true on win32 and windowsHide: false on linux) are added.
Security risks
None. windowsHide is a cosmetic spawn option that suppresses the console window on Windows. It has no effect on process permissions, I/O, or any security-sensitive behavior.
Level of scrutiny
Low scrutiny is appropriate. This is a one-line behavioral change (plus cleanup of a dead helper), confined to a single spawn option. The maintainer (felixweinberger) already refined the original PR by making the platform check explicit rather than unconditional, which is a nice touch for clarity.
Other factors
- The bug hunting system found no issues.
- Tests are well-structured, using Object.defineProperty to mock process.platform and verifying the spawn call via expect.objectContaining.
- No outstanding reviewer comments to address.
- The fix addresses a real user-reported issue (#1638) with a +1 from another affected user.
Sorry, something went wrong.
Ported functional changes from upstream v1.x (17 commits behind, v1.28.0 through v1.29.0+). Only retained-fork-surface changes applied; auth, HTTP, and CI-only commits skipped. Changes ported: - stdio: ReadBuffer maxBufferSize limit (modelcontextprotocol#2239) - stdio: windowsHide always on Win32, removed isElectron() (modelcontextprotocol#1640) - stdio: try/catch error handling on read buffer overflow - types: extensions capability on ClientCapabilities/ServerCapabilities (modelcontextprotocol#1811) - types: size field on ResourceSchema (modelcontextprotocol#1575) - zod-compat: prioritize zod issues with path formatting (modelcontextprotocol#1503) - protocol: conditional abort controller cleanup (modelcontextprotocol#1462) Intentionally kept: - AuthInfo import from ./shared/auth-info.js (not reverted to auth/types.js) - All removed transports, auth, and HTTP modules stay out
Summary
StdioClientTransport only sets windowsHide: true when running inside
Electron, but non-Electron Node.js apps on Windows also get an unwanted
cmd.exe console window when spawning MCP server processes.
Since windowsHide is a no-op on non-Windows platforms, this PR sets it
to true unconditionally. The now-unused isElectron() helper is removed.
Related issue
Fixes #1638
Changes