🤖 fix: script runner overflow policy, path handling & stale suggestions

Change-Id: I8223a444f1e46cbc156f0c23a811f65dcea8e126
Signed-off-by: Thomas Kosiewski <tk@coder.com>

Author: ThomasK33

src/browser/components/ChatInput/index.tsx

@@ -18,6 +18,7 @@ import { useMode } from "@/browser/contexts/ModeContext";
 import { ThinkingSliderComponent } from "../ThinkingSlider";
 import { ModelSettings } from "../ModelSettings";
 import { useSendMessageOptions } from "@/browser/hooks/useSendMessageOptions";
+import { useAvailableScripts } from "@/browser/hooks/useAvailableScripts";
 import {
   getModelKey,
   getInputKey,
@@ -131,9 +132,7 @@ export const ChatInput: React.FC<ChatInputProps> = (props) => {
   const [showCommandSuggestions, setShowCommandSuggestions] = useState(false);
   const [commandSuggestions, setCommandSuggestions] = useState<SlashSuggestion[]>([]);
   const [providerNames, setProviderNames] = useState<string[]>([]);
-  const [availableScripts, setAvailableScripts] = useState<
-    Array<{ name: string; description?: string }>
-  >([]);
+  const availableScripts = useAvailableScripts(workspaceId ?? null);
   const [toast, setToast] = useState<Toast | null>(null);
   const [imageAttachments, setImageAttachments] = useState<ImageAttachment[]>([]);
   const handleToastDismiss = useCallback(() => {
@@ -326,44 +325,6 @@ export const ChatInput: React.FC<ChatInputProps> = (props) => {
     };
   }, []);
 
-  useEffect(() => {
-    const currentWorkspaceId = workspaceId;
-    if (!currentWorkspaceId) {
-      setAvailableScripts([]);
-      return;
-    }
-
-    let isMounted = true;
-
-    const loadScripts = async () => {
-      try {
-        const result = await window.api.workspace.listScripts(currentWorkspaceId);
-        if (isMounted) {
-          if (result.success) {
-            const executableScripts = result.data
-              .filter((s) => s.isExecutable)
-              .map((s) => ({ name: s.name, description: s.description }));
-            setAvailableScripts(executableScripts);
-          } else {
-            // Clear scripts if listing fails (e.g. runtime error) to avoid stale suggestions
-            setAvailableScripts([]);
-          }
-        }
-      } catch (error) {
-        console.error("Failed to load scripts:", error);
-        if (isMounted) {
-          setAvailableScripts([]);
-        }
-      }
-    };
-
-    void loadScripts();
-
-    return () => {
-      isMounted = false;
-    };
-  }, [workspaceId]);
-
   // Allow external components (e.g., CommandPalette, Queued message edits) to insert text
   useEffect(() => {
     const handler = (e: Event) => {

src/browser/hooks/useAvailableScripts.test.ts

@@ -0,0 +1,100 @@
+import { GlobalWindow } from "happy-dom";
+
+const win = new GlobalWindow();
+// @ts-expect-error -- Patching global for test environment
+global.window = win;
+// @ts-expect-error -- Patching global for test environment
+global.document = win.document;
+// @ts-expect-error -- Patching global for test environment
+global.navigator = win.navigator;
+
+import { describe, it, expect, beforeEach, afterEach, mock, type Mock } from "bun:test";
+import { renderHook, waitFor } from "@testing-library/react";
+import { useAvailableScripts } from "./useAvailableScripts";
+
+// Define types for our mock
+type ListScriptsMock = Mock<() => Promise<{ success: boolean; data?: unknown[]; error?: string }>>;
+
+describe("useAvailableScripts", () => {
+  const mockListScripts = mock() as unknown as ListScriptsMock;
+
+  beforeEach(() => {
+    // Mock window.api attached to the global window
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access
+    (global.window as any).api = {
+      workspace: {
+        listScripts: mockListScripts,
+      },
+    };
+  });
+
+  afterEach(() => {
+    mock.restore();
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unsafe-member-access
+    delete (global.window as any).api;
+  });
+
+  it("should fetch and return executable scripts", async () => {
+    mockListScripts.mockResolvedValue({
+      success: true,
+      data: [
+        { name: "script1", isExecutable: true, description: "desc1" },
+        { name: "script2", isExecutable: false, description: "desc2" },
+      ],
+    });
+
+    const { result } = renderHook(() => useAvailableScripts("ws-1"));
+
+    // Initial state
+    expect(result.current).toEqual([]);
+
+    await waitFor(() => {
+      expect(result.current).toHaveLength(1);
+    });
+
+    expect(result.current).toEqual([{ name: "script1", description: "desc1" }]);
+  });
+
+  it("should clear scripts immediately when workspaceId changes", async () => {
+    // First load
+    mockListScripts.mockResolvedValue({
+      success: true,
+      data: [{ name: "script1", isExecutable: true }],
+    });
+
+    const { result, rerender } = renderHook(({ wsId }) => useAvailableScripts(wsId), {
+      initialProps: { wsId: "ws-1" as string | null },
+    });
+
+    await waitFor(() => {
+      expect(result.current).toHaveLength(1);
+    });
+
+    // Prepare for second load (hangs to prove clearance)
+    mockListScripts.mockReturnValue(
+      new Promise(() => {
+        // Never resolve
+      })
+    );
+
+    rerender({ wsId: "ws-2" });
+
+    // Should be cleared immediately
+    expect(result.current).toEqual([]);
+  });
+
+  it("should clear scripts if listing fails", async () => {
+    mockListScripts.mockResolvedValue({
+      success: false,
+      error: "Failed",
+    });
+
+    const { result } = renderHook(() => useAvailableScripts("ws-1"));
+
+    await waitFor(() => {
+      expect(mockListScripts).toHaveBeenCalled();
+    });
+
+    expect(result.current).toEqual([]);
+  });
+});

src/browser/hooks/useAvailableScripts.ts

@@ -0,0 +1,51 @@
+import { useState, useEffect } from "react";
+
+export interface AvailableScript {
+  name: string;
+  description?: string;
+}
+
+export function useAvailableScripts(workspaceId: string | null) {
+  const [availableScripts, setAvailableScripts] = useState<AvailableScript[]>([]);
+
+  useEffect(() => {
+    // Clear scripts immediately to prevent stale suggestions from previous workspace
+    setAvailableScripts([]);
+
+    if (!workspaceId) {
+      return;
+    }
+
+    let isMounted = true;
+
+    const loadScripts = async () => {
+      try {
+        const result = await window.api.workspace.listScripts(workspaceId);
+        if (isMounted) {
+          if (result.success) {
+            const executableScripts = result.data
+              .filter((s) => s.isExecutable)
+              .map((s) => ({ name: s.name, description: s.description }));
+            setAvailableScripts(executableScripts);
+          } else {
+            // Clear scripts if listing fails
+            setAvailableScripts([]);
+          }
+        }
+      } catch (error) {
+        console.error("Failed to load scripts:", error);
+        if (isMounted) {
+          setAvailableScripts([]);
+        }
+      }
+    };
+
+    void loadScripts();
+
+    return () => {
+      isMounted = false;
+    };
+  }, [workspaceId]);
+
+  return availableScripts;
+}

src/common/utils/tools/tools.test.ts

@@ -160,6 +160,16 @@ describe("getToolsForModel", () => {
     };
     const result = await diagnoseTool.execute({ args: [] });
 
+    expect(mockRunScript).toHaveBeenCalledWith(
+      config.runtime,
+      config.cwd,
+      "diagnose",
+      [],
+      expect.objectContaining({
+        overflowPolicy: "tmpfile",
+      })
+    );
+
     expect(result).toContain("Standard output");
     expect(result).toContain("--- MUX_OUTPUT ---\nUser notification");
     expect(result).toContain("--- MUX_PROMPT ---\nAgent instruction");

src/common/utils/tools/tools.ts

@@ -103,9 +103,12 @@ export async function getToolsForModel(
             config.cwd,
             script.name,
             args ?? [],
-            config.env ?? {}, // Extra env
-            config.secrets ?? {}, // Secrets
-            300 // timeout
+            {
+              env: config.env ?? {},
+              secrets: config.secrets ?? {},
+              timeoutSecs: 300,
+              overflowPolicy: "tmpfile",
+            }
           );
 
           if (!result.success) {

src/node/services/ipcMain.ts

@@ -1357,10 +1357,12 @@ export class IpcMain {
               workspacePath,
               scriptName,
               args,
-              {}, // Extra env
-              secretsToRecord(projectSecrets),
-              300, // timeout
-              signal
+              {
+                env: {},
+                secrets: secretsToRecord(projectSecrets),
+                timeoutSecs: 300,
+                abortSignal: signal,
+              }
             );
             const elapsedMs = scriptExecutionStartMs ? Date.now() - scriptExecutionStartMs : 0;
             scriptExecutionStartMs = null;

src/node/services/scriptRunner.path.test.ts

@@ -0,0 +1,112 @@
+import { describe, it, expect, mock } from "bun:test";
+import { runWorkspaceScript } from "./scriptRunner";
+import type { Runtime } from "@/node/runtime/Runtime";
+
+// Mock discovery utils to control paths
+void mock.module("@/utils/scripts/discovery", () => ({
+  getScriptPath: (wsPath: string, scriptName: string) => {
+    if (wsPath.includes("/")) {
+      return `${wsPath}/.cmux/scripts/${scriptName}`;
+    }
+    return `${wsPath}\\.cmux\\scripts\\${scriptName}`;
+  },
+  getScriptsDir: (wsPath: string) => {
+    if (wsPath.includes("/")) {
+      return `${wsPath}/.cmux/scripts`;
+    }
+    return `${wsPath}\\.cmux\\scripts`;
+  },
+}));
+
+// Mock createBashTool
+void mock.module("@/node/services/tools/bash", () => ({
+  createBashTool: () => ({
+    execute: mock().mockResolvedValue({
+      success: true,
+      exitCode: 0,
+      output: "mock output",
+      error: "",
+    }),
+  }),
+}));
+
+// Mock helpers
+void mock.module("@/node/utils/runtime/helpers", () => ({
+  execBuffered: mock().mockResolvedValue({ exitCode: 0, stdout: "/tmp/mock", stderr: "" }),
+  writeFileString: mock().mockResolvedValue(undefined),
+  readFileString: mock().mockResolvedValue(""),
+}));
+
+describe("runWorkspaceScript path handling", () => {
+  const mockRuntime = {
+    stat: mock().mockResolvedValue({ isDirectory: false }),
+    normalizePath: mock(),
+  };
+
+  it("should handle POSIX paths correctly (SSH scenario)", async () => {
+    // SSH runtime semantics: paths are POSIX
+    const workspacePath = "/home/user/workspace";
+    const scriptName = "deploy.sh";
+
+    // normalizePath just returns the path as-is for POSIX on POSIX
+    // But key is that it doesn't convert to backslashes
+    mockRuntime.normalizePath.mockImplementation((p: string) => p);
+
+    const result = await runWorkspaceScript(
+      mockRuntime as unknown as Runtime,
+      workspacePath,
+      scriptName,
+      []
+    );
+
+    expect(result.success).toBe(true);
+    expect(mockRuntime.normalizePath).toHaveBeenCalledTimes(2);
+    // Verify it didn't fail with escape error
+  });
+
+  it("should handle Windows paths correctly", async () => {
+    const workspacePath = "C:\\Users\\user\\workspace";
+    const scriptName = "deploy.bat";
+
+    mockRuntime.normalizePath.mockImplementation((p: string) => p);
+
+    const result = await runWorkspaceScript(
+      mockRuntime as unknown as Runtime,
+      workspacePath,
+      scriptName,
+      []
+    );
+
+    expect(result.success).toBe(true);
+  });
+
+  it("should detect path escape attempts", async () => {
+    const workspacePath = "/home/user/workspace";
+    const scriptName = "evil.sh";
+
+    // Mock getScriptPath returning something outside
+    // We can't easily mock the module conditionally inside test,
+    // but we can rely on normalizePath to simulate the escape check failure
+
+    // Simulate a case where script path is outside scripts dir
+    mockRuntime.normalizePath.mockImplementation((p: string, _base: string) => {
+      if (p.includes("scripts")) {
+        if (p.endsWith("scripts")) return "/home/user/workspace/.cmux/scripts";
+        return "/home/user/workspace/evil.sh"; // Escaped!
+      }
+      return p;
+    });
+
+    const result = await runWorkspaceScript(
+      mockRuntime as unknown as Runtime,
+      workspacePath,
+      scriptName,
+      []
+    );
+
+    expect(result.success).toBe(false);
+    if (!result.success) {
+      expect(result.error).toContain("Script path escapes scripts directory");
+    }
+  });
+});