Merge pull request 'DIS-002: shell:false + cross-spawn Windows-Fix im Runner' (#21) from phase-0/harden-spawn into main

Reviewed-on: #21

package.json

@@ -6,18 +6,26 @@
   "scripts": {
     "build": "tsc",
     "start": "node dist/index.js",
-    "dev": "tsc && node dist/index.js"
+    "dev": "tsc && node dist/index.js",
+    "test": "vitest run"
+  },
+  "vitest": {
+    "include": ["tests/**/*.test.ts"],
+    "environment": "node"
   },
   "dependencies": {
     "better-sqlite3": "^11.0.0",
+    "cross-spawn": "^7.0.6",
     "discord.js": "^14.16.0",
     "dotenv": "^16.4.0",
     "yaml": "^2.6.0"
   },
   "devDependencies": {
     "@types/better-sqlite3": "^7.6.0",
+    "@types/cross-spawn": "^6.0.6",
     "@types/node": "^22.0.0",
-    "typescript": "^5.7.0"
+    "typescript": "^5.7.0",
+    "vitest": "^4.1.4"
   },
   "engines": {
     "node": ">=20.0.0"

src/agent/runner.ts

@@ -1,8 +1,9 @@
-import { spawn } from "child_process";
+import spawn from "cross-spawn";
 import * as path from "path";
 import type { Conversation } from "../db/database";
 import { loadAgentIdentity } from "./identity";
 import { loadDisclawConfig } from "../config/loader";
+import { resolveClaude } from "../runtime/resolve-claude";
 
 export interface RunAgentOptions {
   workspacePath: string;
@@ -12,38 +13,20 @@ export interface RunAgentOptions {
   abortController?: AbortController;
 }
 
-interface ClaudeSettings {
-  command: string;
-  timeoutMs: number;
-}
-
 /**
- * Resolves the Claude CLI command and timeout from configuration.
- * Priority: CLAUDE_PATH env var > disclaw.yaml claude_command > "claude" default.
+ * Resolves the Claude CLI timeout from configuration.
  */
-function resolveClaudeSettings(): ClaudeSettings {
-  let command = "claude";
-  let timeoutMs = 120_000;
-
+function resolveTimeoutMs(): number {
   try {
     const rootDir = path.resolve(__dirname, "../..");
     const config = loadDisclawConfig(rootDir);
-    if (config.claude_command) {
-      command = config.claude_command;
-    }
     if (config.claude_timeout_seconds) {
-      timeoutMs = config.claude_timeout_seconds * 1000;
+      return config.claude_timeout_seconds * 1000;
     }
   } catch {
-    // Config not available, use defaults
+    // Config not available, use default
   }
-
-  // CLAUDE_PATH env var takes highest priority for the command
-  if (process.env.CLAUDE_PATH) {
-    command = process.env.CLAUDE_PATH;
-  }
-
-  return { command, timeoutMs };
+  return 120_000;
 }
 
 /**
@@ -164,9 +147,9 @@ export async function runAgent(options: RunAgentOptions): Promise<string> {
   // Verify the workspace has an agent.yaml (validates it exists)
   loadAgentIdentity(workspacePath);
 
-  const settings = resolveClaudeSettings();
+  const claudeCommand = await resolveClaude();
   const prompt = buildPrompt(userMessage, conversationHistory, channelName);
-  const timeoutMs = settings.timeoutMs;
+  const timeoutMs = resolveTimeoutMs();
 
   return new Promise<string>((resolve, reject) => {
     const args = [
@@ -176,7 +159,7 @@ export async function runAgent(options: RunAgentOptions): Promise<string> {
       "json",
     ];
 
-    const child = spawn(settings.command, args, {
+    const child = spawn(claudeCommand, args, {
       cwd: workspacePath,
       env: {
         ...process.env,
@@ -184,7 +167,7 @@ export async function runAgent(options: RunAgentOptions): Promise<string> {
         CI: "true",
       },
       stdio: ["pipe", "pipe", "pipe"],
-      shell: true,
+      windowsHide: true,
     });
 
     let stdout = "";
@@ -192,14 +175,20 @@ export async function runAgent(options: RunAgentOptions): Promise<string> {
     let killed = false;
 
     // Collect stdout
-    child.stdout.on("data", (data: Buffer) => {
-      stdout += data.toString("utf-8");
-    });
+    if (child.stdout) {
+      child.stdout.setEncoding("utf8");
+      child.stdout.on("data", (data: string) => {
+        stdout += data;
+      });
+    }
 
     // Collect stderr
-    child.stderr.on("data", (data: Buffer) => {
-      stderr += data.toString("utf-8");
-    });
+    if (child.stderr) {
+      child.stderr.setEncoding("utf8");
+      child.stderr.on("data", (data: string) => {
+        stderr += data;
+      });
+    }
 
     // Timeout handling
     const timer = setTimeout(() => {
@@ -223,12 +212,12 @@ export async function runAgent(options: RunAgentOptions): Promise<string> {
       });
     }
 
-    child.on("error", (err) => {
+    child.on("error", (err: NodeJS.ErrnoException) => {
       clearTimeout(timer);
-      if (err.message.includes("ENOENT")) {
+      if (err.code === "ENOENT") {
         reject(
           new Error(
-            `Claude Code CLI not found at "${settings.command}". ` +
+            `Claude Code CLI not found at "${claudeCommand}". ` +
             `Install it with: npm install -g @anthropic-ai/claude-code\n` +
             `Or set CLAUDE_PATH in your .env file.`
           )
@@ -257,6 +246,8 @@ export async function runAgent(options: RunAgentOptions): Promise<string> {
     });
 
     // Close stdin immediately since we pass everything via args
-    child.stdin.end();
+    if (child.stdin) {
+      child.stdin.end();
+    }
   });
 }

src/runtime/resolve-claude.ts

@@ -0,0 +1,66 @@
+import { execFile } from "child_process";
+import { promisify } from "util";
+
+const execFileAsync = promisify(execFile);
+
+/** Cached result after first successful resolution. */
+let cachedPath: string | null = null;
+
+/**
+ * Resolves the Claude CLI executable name or path.
+ *
+ * Resolution order:
+ * 1. `CLAUDE_PATH` environment variable (absolute path or command name)
+ * 2. PATH lookup via `where` (Windows) or `which` (Unix)
+ *
+ * The result is cached in-memory after the first successful call so
+ * subsequent invocations are synchronous-cheap.
+ *
+ * On Windows, `claude` is installed as `claude.cmd`. `cross-spawn` handles
+ * `.cmd` wrappers transparently, so returning the bare name `"claude"` is
+ * correct — cross-spawn will find and invoke `claude.cmd` automatically.
+ *
+ * @throws {Error} with a clear installation hint if claude cannot be found.
+ */
+export async function resolveClaude(): Promise<string> {
+  if (cachedPath !== null) {
+    return cachedPath;
+  }
+
+  // 1. Explicit override via environment variable
+  const envPath = process.env.CLAUDE_PATH;
+  if (envPath && envPath.trim() !== "") {
+    cachedPath = envPath.trim();
+    return cachedPath;
+  }
+
+  // 2. PATH lookup
+  const isWindows = process.platform === "win32";
+  const lookupCmd = isWindows ? "where" : "which";
+
+  try {
+    const { stdout } = await execFileAsync(lookupCmd, ["claude"], {
+      timeout: 5_000,
+    });
+    const firstLine = stdout.trim().split(/\r?\n/)[0].trim();
+    if (firstLine) {
+      cachedPath = firstLine;
+      return cachedPath;
+    }
+  } catch {
+    // where/which returned non-zero or was not found — fall through to error
+  }
+
+  throw new Error(
+    `Claude Code CLI not found. Install it with:\n` +
+      `  npm install -g @anthropic-ai/claude-code\n` +
+      `Or set CLAUDE_PATH in your .env file to the full path of the claude executable.`
+  );
+}
+
+/**
+ * Resets the in-memory cache. Intended for use in tests only.
+ */
+export function _resetResolveClaudeCache(): void {
+  cachedPath = null;
+}

tests/unit/resolve-claude.test.ts

@@ -0,0 +1,118 @@
+import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
+
+// We mock child_process so execFile never actually runs
+vi.mock("child_process", async (importOriginal) => {
+  const actual = await importOriginal<typeof import("child_process")>();
+  return {
+    ...actual,
+    execFile: vi.fn(),
+  };
+});
+
+// Import after mock is registered so the module picks up the mock
+import { execFile } from "child_process";
+import { resolveClaude, _resetResolveClaudeCache } from "../../src/runtime/resolve-claude";
+
+// util.promisify wraps the callback form; we need to make execFile call its callback
+const mockedExecFile = execFile as unknown as ReturnType<typeof vi.fn>;
+
+/** Helper: make execFile invoke its last argument (callback) with success */
+function mockExecFileSuccess(stdout: string): void {
+  mockedExecFile.mockImplementation(
+    (
+      _cmd: string,
+      _args: string[],
+      _opts: object,
+      callback: (err: null, result: { stdout: string; stderr: string }) => void
+    ) => {
+      callback(null, { stdout, stderr: "" });
+    }
+  );
+}
+
+/** Helper: make execFile invoke its callback with an error */
+function mockExecFileFailure(message: string): void {
+  mockedExecFile.mockImplementation(
+    (
+      _cmd: string,
+      _args: string[],
+      _opts: object,
+      callback: (err: Error) => void
+    ) => {
+      callback(new Error(message));
+    }
+  );
+}
+
+describe("resolveClaude", () => {
+  const originalEnv = process.env;
+
+  beforeEach(() => {
+    // Reset module-level cache before each test
+    _resetResolveClaudeCache();
+    // Clone env so mutations don't bleed between tests
+    process.env = { ...originalEnv };
+    vi.clearAllMocks();
+  });
+
+  afterEach(() => {
+    process.env = originalEnv;
+  });
+
+  it("(a) returns CLAUDE_PATH directly when env var is set", async () => {
+    process.env.CLAUDE_PATH = "/usr/local/bin/claude";
+
+    const result = await resolveClaude();
+
+    expect(result).toBe("/usr/local/bin/claude");
+    // Should NOT have called execFile at all
+    expect(mockedExecFile).not.toHaveBeenCalled();
+  });
+
+  it("(b) falls back to PATH lookup when CLAUDE_PATH is not set", async () => {
+    delete process.env.CLAUDE_PATH;
+    mockExecFileSuccess("/home/user/.nvm/bin/claude\n");
+
+    const result = await resolveClaude();
+
+    expect(result).toBe("/home/user/.nvm/bin/claude");
+    expect(mockedExecFile).toHaveBeenCalledOnce();
+  });
+
+  it("(b) trims whitespace and picks first line from PATH lookup output", async () => {
+    delete process.env.CLAUDE_PATH;
+    // `where` on Windows can return multiple matches, one per line
+    mockExecFileSuccess(
+      "C:\\Users\\dev\\AppData\\Roaming\\npm\\claude.cmd\r\nC:\\Program Files\\claude\\claude.cmd\r\n"
+    );
+
+    const result = await resolveClaude();
+
+    expect(result).toBe("C:\\Users\\dev\\AppData\\Roaming\\npm\\claude.cmd");
+  });
+
+  it("(c) throws a clear error when neither CLAUDE_PATH nor PATH lookup finds claude", async () => {
+    delete process.env.CLAUDE_PATH;
+    mockExecFileFailure("not found");
+
+    await expect(resolveClaude()).rejects.toThrow(
+      /Claude Code CLI not found/
+    );
+    await expect(resolveClaude()).rejects.toThrow(
+      /npm install -g @anthropic-ai\/claude-code/
+    );
+  });
+
+  it("caches the result after first successful resolution", async () => {
+    delete process.env.CLAUDE_PATH;
+    mockExecFileSuccess("/usr/bin/claude");
+
+    const first = await resolveClaude();
+    const second = await resolveClaude();
+
+    expect(first).toBe("/usr/bin/claude");
+    expect(second).toBe("/usr/bin/claude");
+    // execFile should only have been called once due to caching
+    expect(mockedExecFile).toHaveBeenCalledOnce();
+  });
+});