Merge remote-tracking branch 'origin/farm/c4e0cc3d/fix-windows-mcp-conhost-windows'

2026-06-26 15:56:22 +02:00
parent 44368e3459 52d1a04652
commit 6ecdb5637d
4 changed files with 55 additions and 38 deletions
@@ -8,6 +8,7 @@

 ### Fixed

+- Fixed Windows stdio MCP wrapper chains spawning visible PowerShell/cmd windows on startup after the #3544 fix. `StdioTransport.connect()` now probes whether OMP already has an inheritable console and `resolveStdioSpawnCommand` skips `windowsHide`/`CREATE_NO_WINDOW` in that case, so `cmd.exe`/PowerShell grandchildren reuse the terminal console instead of allocating visible conhosts during MCP startup or reconnects. ([#3567](https://github.com/can1357/oh-my-pi/issues/3567))
 - Fixed Kimi-family models defaulting to hashline edit mode; they now fall back to `replace` unless `edit.modelVariants`, `PI_EDIT_VARIANT`, or `PI_STRICT_EDIT_MODE` explicitly opts into hashline.
 - Fixed MCP OAuth discovery rejecting Atlassian-style cross-host issuer metadata during the resource-server fallback probe; issuer matching now remains enforced for advertised auth-server candidates but no longer blocks fallback metadata where the resource host and authorization-server issuer differ. ([#3551](https://github.com/can1357/oh-my-pi/issues/3551))
 - Fixed plan approval applying the wrong execution model when the model-tier slider sat on the model that exit would restore. The match check now compares the selected role's effective thinking level against the pre-plan thinking level, so picking the active planning tier is retained and picking a same-model tier with an explicit thinking suffix (e.g. `default = sonnet:off` while plan-mode raised thinking to `high`) goes through `applyRoleModel` instead of silently restoring the pre-plan level. ([#3554](https://github.com/can1357/oh-my-pi/issues/3554))
@@ -3,14 +3,13 @@ import { describe, expect, it } from "bun:test";
 import { resolveStdioSpawnCommand } from "./stdio";

 describe("resolveStdioSpawnCommand", () => {
-	it("hides AND stays attached to direct Windows executable MCP servers", async () => {
-		// Hidden so the direct .exe does not pop a console (#3536); attached so
-		// nested console grandchildren do not allocate a new visible conhost
-		// that strips their stdout from our pipe (#3544).
+	it("hides Windows executable MCP servers when the host has no console", async () => {
+		// Hidden so a console-app child does not allocate a visible window when
+		// OMP is launched without a terminal console (#3536).
 		await expect(
 			resolveStdioSpawnCommand(
 				{ command: "server.exe", args: ["--stdio"] },
-				{ cwd: process.cwd(), env: {}, platform: "win32" },
+				{ cwd: process.cwd(), env: {}, platform: "win32", hostHasInheritableConsole: false },
 			),
 		).resolves.toEqual({
 			cmd: ["server.exe", "--stdio"],
@@ -19,6 +18,19 @@ describe("resolveStdioSpawnCommand", () => {
 		});
 	});

+	it("inherits an attached Windows console instead of forcing CREATE_NO_WINDOW", async () => {
+		await expect(
+			resolveStdioSpawnCommand(
+				{ command: "server.exe", args: ["--stdio"] },
+				{ cwd: process.cwd(), env: {}, platform: "win32", hostHasInheritableConsole: true },
+			),
+		).resolves.toEqual({
+			cmd: ["server.exe", "--stdio"],
+			windowsHide: false,
+			detached: false,
+		});
+	});
+
 	it("detaches off-Windows MCP servers so terminal job-control signals cannot stop them", async () => {
 		await expect(
 			resolveStdioSpawnCommand(
@@ -7,9 +7,9 @@

 import * as fs from "node:fs/promises";
 import * as path from "node:path";
-
 import { getProjectDir, readJsonl, Snowflake } from "@oh-my-pi/pi-utils";
 import { type Subprocess, spawn } from "bun";
+import { hostHasInheritableConsole } from "../../eval/py/spawn-options";
 import type {
 	JsonRpcError,
 	JsonRpcMessage,
@@ -25,6 +25,16 @@ import { isMCPTimeoutEnabled, resolveMCPTimeoutMs } from "../timeout";
 /** Subprocess argv and platform-derived spawn flags for an MCP stdio server. */
 export interface StdioSpawnCommand {
 	cmd: string[];
+	/**
+	 * Hide the Windows console window for the direct child.
+	 *
+	 * Windows uses this only when the OMP host has no console to share. When
+	 * the host is running inside a terminal, `windowsHide: true` maps to
+	 * `CREATE_NO_WINDOW`, which strips that inheritable console from hidden
+	 * `cmd.exe` / PowerShell wrapper chains. Their console grandchildren then
+	 * allocate fresh visible conhost windows during startup or reconnects
+	 * (#3567).
+	 */
 	windowsHide?: boolean;
 	/**
 	 * Run the subprocess in its own session.
@@ -34,15 +44,9 @@ export interface StdioSpawnCommand {
 	 * background-read SIGTTIN) cannot stop stdio servers such as
 	 * `chrome-devtools-mcp` and leave our read loop blocked on silent pipes.
 	 *
-	 * Windows: `false`. There is no SIGTSTP/SIGTTIN to escape, but detaching
-	 * passes `DETACHED_PROCESS` to `CreateProcess`, which strips the parent's
-	 * inherited console. A hidden `cmd.exe` wrapper whose grandchild is a
-	 * console app (`node`, `npx.cmd`, `mcp-remote`) then allocates a brand-new
-	 * visible conhost for that grandchild, and its stdout no longer routes
-	 * back through our pipe — the proxy reports it is up while OMP times out
-	 * waiting for the MCP `initialize` response (#3544). `windowsHide` only
-	 * hides the direct child's console window (#3536); without `detached:
-	 * false` the nested grandchild still pops.
+	 * Windows: `false`. There is no SIGTSTP/SIGTTIN to escape, and Windows
+	 * wrapper chains must stay in the OMP console session so nested console
+	 * grandchildren keep stdout routed through our pipe (#3544).
 	 */
 	detached: boolean;
 }
@@ -51,6 +55,7 @@ export interface StdioSpawnCommand {
 export interface ResolveStdioSpawnOptions {
 	cwd: string;
 	env: Record<string, string | undefined>;
+	hostHasInheritableConsole?: boolean;
 	platform?: NodeJS.Platform;
 }

@@ -161,6 +166,7 @@ async function resolveWindowsNpmShimCommand(
 	command: string,
 	args: readonly string[],
 	cwd: string,
+	windowsHide: boolean,
 ): Promise<StdioSpawnCommand | null> {
 	if (!isWindowsBatchCommand(command)) return null;
 	if (!hasPathSegment(command)) return null;
@@ -195,7 +201,7 @@ async function resolveWindowsNpmShimCommand(
 	const nodeCommand = (await fileExists(siblingNode)) ? siblingNode : "node";
 	return {
 		cmd: [nodeCommand, target, ...args],
-		windowsHide: true,
+		windowsHide,
 		detached: false,
 	};
 }
@@ -251,18 +257,18 @@ export async function resolveStdioSpawnCommand(
 	const args = config.args ?? [];
 	if (options.platform !== "win32") return { cmd: [config.command, ...args], detached: true };

+	const windowsHide = options.hostHasInheritableConsole === undefined ? true : !options.hostHasInheritableConsole;
 	const resolved = await resolveWindowsCommandPath(config.command, options.cwd, options.env);
 	const resolvedCommand = resolved ?? config.command;
-	const npmShimCommand = await resolveWindowsNpmShimCommand(resolvedCommand, args, options.cwd);
+	const npmShimCommand = await resolveWindowsNpmShimCommand(resolvedCommand, args, options.cwd, windowsHide);
 	if (npmShimCommand) return npmShimCommand;

 	// Direct-spawn only when we resolved to a concrete file AND its extension
 	// is not a batch script. Everything else (resolved .cmd/.bat, or an
 	// unresolved extensionless command) goes through cmd.exe so PATHEXT runs.
-	// Every Windows stdio server launch hides its console window AND stays
-	// attached: detaching on Windows breaks nested console grandchildren
-	// (see `StdioSpawnCommand.detached`).
-	const windowsHide = true;
+	// Windows stdio servers stay attached so wrapper grandchildren inherit the
+	// same console session. Only hide the child when OMP itself has no console
+	// to share; CREATE_NO_WINDOW breaks console inheritance for nested wrappers.
 	const detached = false;
 	const needsCmdExe = resolved === null || isWindowsBatchCommand(resolvedCommand);
 	if (!needsCmdExe) return { cmd: [resolvedCommand, ...args], windowsHide, detached };
@@ -364,14 +370,14 @@ export class StdioTransport implements MCPTransport {
 			cwd,
 			env,
 			platform: process.platform,
+			hostHasInheritableConsole: hostHasInheritableConsole(),
 		});

 		// Platform-derived session and console-window handling come from
 		// `resolveStdioSpawnCommand`: POSIX detaches into its own session to
 		// escape terminal job-control signals (SIGTSTP, SIGTTIN); Windows stays
-		// attached and hidden so nested console grandchildren do not allocate a
-		// brand-new conhost that strips their stdout from our pipe. See
-		// `StdioSpawnCommand.detached`.
+		// attached, and only hides the child when the host has no console to
+		// share. See `StdioSpawnCommand`.
 		this.#process = spawn({
 			cmd: spawnCommand.cmd,
 			cwd,
@@ -109,11 +109,12 @@ describe("resolveStdioSpawnCommand", () => {
 						PATHEXT: ".cmd",
 					},
 					platform: "win32",
+					hostHasInheritableConsole: true,
 				},
 			);

 			expect(result.cmd).toEqual(["node", entry, "serve", "--mcp"]);
-			expect(result.windowsHide).toBe(true);
+			expect(result.windowsHide).toBe(false);
 			expect(result.detached).toBe(false);
 		} finally {
 			await fs.rm(tempDir, { recursive: true, force: true });
@@ -339,18 +340,14 @@ describe("resolveStdioSpawnCommand", () => {
 		expect(result.detached).toBe(true);
 	});

-	it("never detaches Windows stdio launches so nested cmd.exe wrappers keep stdout routed back to the parent pipe (#3544)", async () => {
-		// The reporter's failing shape is `cmd.exe` → `node wrapper` →
-		// `cmd.exe /C npx.cmd -y mcp-remote`. Detaching the direct hidden
-		// `cmd.exe` strips its inherited console; the nested console
-		// grandchildren (`node`, `npx.cmd`, `mcp-remote`) then allocate a
-		// brand-new visible conhost whose stdout no longer routes back through
-		// our pipe — the proxy reports the bridge is up while OMP times out on
-		// the MCP `initialize` response. `windowsHide` only hides the direct
-		// child's window (#3536); the only fix that keeps the grandchild
-		// attached to the same console session is `detached: false`. Pin every
-		// Windows return shape here so the contract cannot regress for the
-		// direct-`cmd.exe` launcher the reporter used.
+	it("keeps console-attached Windows cmd.exe wrapper chains out of CREATE_NO_WINDOW (#3567)", async () => {
+		// The #3544 shape is `cmd.exe` → `node wrapper` → another console
+		// launcher (`cmd.exe /C npx.cmd`, PowerShell, similar). If the OMP host
+		// already owns a terminal console, `windowsHide: true` maps to
+		// CREATE_NO_WINDOW and strips that inheritable console from the direct
+		// hidden wrapper. Grandchildren then allocate fresh visible conhost
+		// windows during startup or reconnect loops (#3567). Keep the tree
+		// attached to OMP's console instead.
 		const result = await resolveStdioSpawnCommand(
 			{ type: "stdio", command: "cmd.exe", args: ["/C", "node .codex\\mcp-wrapper.js"] },
 			{
@@ -361,11 +358,12 @@ describe("resolveStdioSpawnCommand", () => {
 					PATHEXT: ".COM;.EXE;.BAT;.CMD",
 				},
 				platform: "win32",
+				hostHasInheritableConsole: true,
 			},
 		);

 		expect(result.detached).toBe(false);
-		expect(result.windowsHide).toBe(true);
+		expect(result.windowsHide).toBe(false);
 	});
 });