Merge branch 'main' into refactor/tidy-up

Author: hqwlkj

src/mcp/mcp-client.ts

@@ -1,6 +1,5 @@
 import { spawn, type ChildProcess } from "child_process";
 import { createInterface, type Interface } from "readline";
-import * as os from "os";
 import * as path from "path";
 import { killProcessTree } from "../common/process-tree";
 
@@ -97,6 +96,13 @@ type ReadResourceResult = {
 
 export type McpNotificationHandler = (method: string, params?: Record<string, unknown>) => void;
 
+export type McpSpawnSpec = {
+  command: string;
+  args: string[];
+  shell: boolean;
+  windowsHide?: boolean;
+};
+
 export class McpClient {
   private process: ChildProcess | null = null;
   private reader: Interface | null = null;
@@ -130,25 +136,14 @@ export class McpClient {
         ...this.env,
       };
       const args = this.withNpxYesArg(this.command, this.args);
+      const spawnSpec = createMcpSpawnSpec(this.command, args);
 
-      const isWindows = os.platform() === "win32";
-
-      if (isWindows) {
-        // On Windows, shell: true lets cmd.exe resolve the command via
-        // PATHEXT (npx → npx.cmd, etc.) without blindly appending .cmd,
-        // which would break absolute paths like process.execPath.
-        this.process = spawn(this.command, args, {
-          stdio: ["pipe", "pipe", "pipe"],
-          env: childEnv,
-          shell: true,
-          windowsHide: true,
-        });
-      } else {
-        this.process = spawn(this.command, args, {
-          stdio: ["pipe", "pipe", "pipe"],
-          env: childEnv,
-        });
-      }
+      this.process = spawn(spawnSpec.command, spawnSpec.args, {
+        stdio: ["pipe", "pipe", "pipe"],
+        env: childEnv,
+        shell: spawnSpec.shell,
+        windowsHide: spawnSpec.windowsHide,
+      });
 
       let resolved = false;
       const safeReject = (err: Error) => {
@@ -421,3 +416,31 @@ export class McpClient {
     return new Error(stderr ? `${message}. stderr: ${stderr}` : message);
   }
 }
+
+export function createMcpSpawnSpec(
+  command: string,
+  args: string[],
+  platform: NodeJS.Platform = process.platform
+): McpSpawnSpec {
+  if (platform === "win32") {
+    return {
+      // On Windows, shell: true lets cmd.exe resolve the command via PATHEXT
+      // (npx -> npx.cmd, etc.). Pass one quoted command line with no spawn
+      // args to avoid Node 24 DEP0190.
+      command: [command, ...args].map(quoteWindowsShellArg).join(" "),
+      args: [],
+      shell: true,
+      windowsHide: true,
+    };
+  }
+
+  return {
+    command,
+    args,
+    shell: false,
+  };
+}
+
+function quoteWindowsShellArg(arg: string): string {
+  return `"${arg.replace(/(\\*)"/g, '$1$1\\"').replace(/\\+$/g, "$&$&")}"`;
+}

src/session.ts

@@ -2730,7 +2730,8 @@ ${skillMd}
       status === "waiting_for_user" ||
       status === "completed" ||
       status === "interrupted" ||
-      status === "ask_permission"
+      status === "ask_permission" ||
+      status === "permission_denied"
     ) {
       return status;
     }

src/tests/mcp-client.test.ts

@@ -0,0 +1,34 @@
+import { test } from "node:test";
+import assert from "node:assert/strict";
+import { createMcpSpawnSpec } from "../mcp/mcp-client";
+
+test("createMcpSpawnSpec keeps non-Windows MCP launches shell-free", () => {
+  assert.deepEqual(createMcpSpawnSpec("npx", ["-y", "@playwright/mcp@latest"], "darwin"), {
+    command: "npx",
+    args: ["-y", "@playwright/mcp@latest"],
+    shell: false,
+  });
+});
+
+test("createMcpSpawnSpec avoids Windows shell args for Node 24", () => {
+  assert.deepEqual(createMcpSpawnSpec("npx", ["-y", "@playwright/mcp@latest"], "win32"), {
+    command: '"npx" "-y" "@playwright/mcp@latest"',
+    args: [],
+    shell: true,
+    windowsHide: true,
+  });
+});
+
+test("createMcpSpawnSpec quotes Windows command paths and arguments", () => {
+  const spec = createMcpSpawnSpec(
+    String.raw`C:\Program Files\nodejs\node.exe`,
+    [String.raw`C:\tmp\mcp server.cjs`, 'a "quoted" value'],
+    "win32"
+  );
+
+  assert.equal(
+    spec.command,
+    String.raw`"C:\Program Files\nodejs\node.exe" "C:\tmp\mcp server.cjs" "a \"quoted\" value"`
+  );
+  assert.deepEqual(spec.args, []);
+});

src/tests/session.test.ts

@@ -1313,6 +1313,57 @@ test("activateSession pauses for permission when a tool call requires ask", asyn
   );
 });
 
+test("SessionManager preserves permission_denied status when sessions are reloaded", async () => {
+  const workspace = createTempDir("deepcode-permission-denied-workspace-");
+  const home = createTempDir("deepcode-permission-denied-home-");
+  setHomeDir(home);
+
+  const permissions = {
+    allow: [],
+    deny: [],
+    ask: [],
+    defaultMode: "askAll" as const,
+  };
+  const manager = createPermissionSessionManager(
+    workspace,
+    [
+      {
+        choices: [
+          {
+            message: {
+              content: "",
+              tool_calls: [
+                {
+                  id: "call-bash",
+                  type: "function",
+                  function: {
+                    name: "bash",
+                    arguments: JSON.stringify({
+                      command: "rg TODO src",
+                      description: "Search TODO markers",
+                      sideEffects: ["read-in-cwd"],
+                    }),
+                  },
+                },
+              ],
+            },
+          },
+        ],
+      },
+    ],
+    permissions
+  );
+
+  const sessionId = await manager.createSession({ text: "search todos" });
+  manager.denySessionPermission(sessionId);
+
+  const reloadedManager = createPermissionSessionManager(workspace, [], permissions);
+  const reloadedSession = reloadedManager.getSession(sessionId);
+
+  assert.equal(reloadedSession?.status, "permission_denied");
+  assert.equal(reloadedSession?.failReason, "Permission denied by user");
+});
+
 test("replySession applies permission replies, runs pending tools, and stores always allow scopes", async () => {
   const workspace = createTempDir("deepcode-permission-allow-workspace-");
   const home = createTempDir("deepcode-permission-allow-home-");

src/tests/sessionList.test.ts

@@ -18,6 +18,8 @@ test("formatSessionStatus maps status values to display labels", () => {
   assert.equal(formatSessionStatus("waiting_for_user"), "waiting");
   assert.equal(formatSessionStatus("failed"), "failed");
   assert.equal(formatSessionStatus("interrupted"), "stopped");
+  assert.equal(formatSessionStatus("ask_permission"), "waiting");
+  assert.equal(formatSessionStatus("permission_denied"), "denied");
   assert.equal(formatSessionStatus("unknown_status" as any), "unknown_status");
 });