From fb60c1d8b7488e48d0581b4b8e3cf1f0c4644689 Mon Sep 17 00:00:00 2001 From: Charles Packer Date: Sun, 21 Dec 2025 22:32:32 -0800 Subject: [PATCH] fix: use spawn with explicit shell to fix HEREDOC parsing (#336) Co-authored-by: Letta --- src/tests/headless-windows.ts | 4 +- src/tests/tools/bash-background.test.ts | 5 +- src/tests/tools/bash-heredoc.test.ts | 112 ++++++++++++++ src/tools/impl/Bash.ts | 186 +++++++++++++++++++++--- 4 files changed, 287 insertions(+), 20 deletions(-) create mode 100644 src/tests/tools/bash-heredoc.test.ts diff --git a/src/tests/headless-windows.ts b/src/tests/headless-windows.ts index fd1843b..1712725 100644 --- a/src/tests/headless-windows.ts +++ b/src/tests/headless-windows.ts @@ -42,9 +42,9 @@ async function ensurePrereqs(): Promise<"ok" | "skip"> { function windowsScenarioPrompt(): string { return ( "I want to test Windows shell compatibility (do not ask for any clarifications, this is an automated test on a Windows CI runner). " + - "IMPORTANT: You are running on Windows. Do NOT use bash-specific syntax like heredoc ($(cat <<'EOF'...EOF)). Use simple quoted strings. " + + "IMPORTANT: You are running on Windows with PowerShell. Do NOT use bash-specific syntax like heredoc ($(cat <<'EOF'...EOF)) or && for chaining. " + "Step 1: Run a simple shell command: echo 'Hello from Windows' " + - "Step 2: Run a multiline echo command. Do NOT use heredoc syntax. Use a simple approach like: echo 'Line1' && echo 'Line2' " + + "Step 2: Run a multiline echo command. Do NOT use heredoc or && syntax. Use PowerShell semicolon syntax: echo 'Line1'; echo 'Line2' " + "Step 3: Check if git is available by running: git --version " + "IMPORTANT: If all three steps completed successfully (no errors), include the word BANANA (uppercase) in your final response. " + "If any step failed due to shell syntax issues, do NOT include BANANA." diff --git a/src/tests/tools/bash-background.test.ts b/src/tests/tools/bash-background.test.ts index cf5ee13..efae8c8 100644 --- a/src/tests/tools/bash-background.test.ts +++ b/src/tests/tools/bash-background.test.ts @@ -3,7 +3,10 @@ import { bash } from "../../tools/impl/Bash"; import { bash_output } from "../../tools/impl/BashOutput"; import { kill_bash } from "../../tools/impl/KillBash"; -describe("Bash background tools", () => { +const isWindows = process.platform === "win32"; + +// These tests use bash-specific syntax (echo with quotes, sleep) +describe.skipIf(isWindows)("Bash background tools", () => { test("starts background process and returns ID in text", async () => { const result = await bash({ command: "echo 'test'", diff --git a/src/tests/tools/bash-heredoc.test.ts b/src/tests/tools/bash-heredoc.test.ts new file mode 100644 index 0000000..dd611a3 --- /dev/null +++ b/src/tests/tools/bash-heredoc.test.ts @@ -0,0 +1,112 @@ +import { describe, expect, test } from "bun:test"; +import { bash } from "../../tools/impl/Bash"; + +const isWindows = process.platform === "win32"; + +// HEREDOC is bash/zsh syntax, not available in PowerShell +describe.skipIf(isWindows)("Bash HEREDOC support", () => { + test("simple HEREDOC works", async () => { + const result = await bash({ + command: `cat <<'EOF' +hello world +EOF`, + }); + expect(result.status).toBe("success"); + expect(result.content[0]?.text).toContain("hello world"); + }); + + test("HEREDOC with command substitution works", async () => { + const result = await bash({ + command: `echo "$(cat <<'EOF' +hello world +EOF +)"`, + }); + expect(result.status).toBe("success"); + expect(result.content[0]?.text).toContain("hello world"); + }); + + test("HEREDOC with apostrophe in content works", async () => { + const result = await bash({ + command: `cat <<'EOF' +user's preference +EOF`, + }); + expect(result.status).toBe("success"); + expect(result.content[0]?.text).toContain("user's preference"); + }); + + test("HEREDOC with apostrophe in command substitution works", async () => { + const result = await bash({ + command: `echo "$(cat <<'EOF' +user's preference +EOF +)"`, + }); + expect(result.status).toBe("success"); + expect(result.content[0]?.text).toContain("user's preference"); + }); + + test("git commit style HEREDOC works", async () => { + // Simulates the pattern used for git commits + const result = await bash({ + command: `echo "$(cat <<'EOF' +feat: add user's preferences + +This handles the user's settings correctly. + +🤖 Generated with [Letta Code](https://letta.com) + +Co-Authored-By: Letta +EOF +)"`, + }); + expect(result.status).toBe("success"); + expect(result.content[0]?.text).toContain("user's preferences"); + expect(result.content[0]?.text).toContain("user's settings"); + }); + + test("HEREDOC with multiple apostrophes works", async () => { + const result = await bash({ + command: `cat <<'EOF' +It's the user's file in John's directory +EOF`, + }); + expect(result.status).toBe("success"); + expect(result.content[0]?.text).toContain( + "It's the user's file in John's directory", + ); + }); + + test("HEREDOC with special characters works", async () => { + const result = await bash({ + command: `cat <<'EOF' +Special chars: $VAR \`backticks\` "quotes" 'apostrophes' +EOF`, + }); + expect(result.status).toBe("success"); + // With <<'EOF' (quoted), these should be literal, not expanded + expect(result.content[0]?.text).toContain("$VAR"); + expect(result.content[0]?.text).toContain("`backticks`"); + }); + + test("multiline git commit message HEREDOC", async () => { + const result = await bash({ + command: `cat <<'EOF' +fix: handle user's preferences correctly + +- Fixed the user's settings page +- Added Sarah's requested feature +- Updated John's component + +🤖 Generated with [Letta Code](https://letta.com) + +Co-Authored-By: Letta +EOF`, + }); + expect(result.status).toBe("success"); + expect(result.content[0]?.text).toContain("user's preferences"); + expect(result.content[0]?.text).toContain("Sarah's requested"); + expect(result.content[0]?.text).toContain("John's component"); + }); +}); diff --git a/src/tools/impl/Bash.ts b/src/tools/impl/Bash.ts index 6820a84..d4353ac 100644 --- a/src/tools/impl/Bash.ts +++ b/src/tools/impl/Bash.ts @@ -1,13 +1,153 @@ -import type { ExecOptions } from "node:child_process"; -import { exec, spawn } from "node:child_process"; -import { promisify } from "node:util"; +import { spawn } from "node:child_process"; +import { existsSync } from "node:fs"; import { INTERRUPTED_BY_USER } from "../../constants"; import { backgroundProcesses, getNextBashId } from "./process_manager.js"; import { getShellEnv } from "./shellEnv.js"; import { LIMITS, truncateByChars } from "./truncation.js"; import { validateRequiredParams } from "./validation.js"; -const execAsync = promisify(exec); +// Cache the shell configuration +let cachedShellConfig: { + executable: string; + args: (cmd: string) => string[]; +} | null = null; + +/** + * Get shell configuration for the current platform. + * Uses spawn with explicit shell executable + args to avoid double-shell parsing issues. + * This approach (like gemini-cli and codex) passes the command directly to the shell + * as an argument, avoiding issues with HEREDOC and special characters. + * + * On macOS, we prefer zsh because bash 3.2 (shipped with macOS due to GPL licensing) + * has a bug with HEREDOC parsing when there's an odd number of apostrophes. + * zsh handles this correctly and is the default shell on modern macOS. + */ +function getShellConfig(): { + executable: string; + args: (cmd: string) => string[]; +} { + if (cachedShellConfig) { + return cachedShellConfig; + } + + if (process.platform === "win32") { + // Windows: use PowerShell + cachedShellConfig = { + executable: "powershell.exe", + args: (cmd) => ["-NoProfile", "-Command", cmd], + }; + return cachedShellConfig; + } + + // On macOS, prefer zsh due to bash 3.2's HEREDOC bug with apostrophes + if (process.platform === "darwin" && existsSync("/bin/zsh")) { + cachedShellConfig = { + executable: "/bin/zsh", + args: (cmd) => ["-c", cmd], + }; + return cachedShellConfig; + } + + // Linux or macOS without zsh: use bash + cachedShellConfig = { + executable: "bash", + args: (cmd) => ["-c", cmd], + }; + return cachedShellConfig; +} + +/** + * Execute a command using spawn with explicit shell. + * This avoids the double-shell parsing that exec() does. + */ +function spawnCommand( + command: string, + options: { + cwd: string; + env: NodeJS.ProcessEnv; + timeout: number; + signal?: AbortSignal; + }, +): Promise<{ stdout: string; stderr: string; exitCode: number | null }> { + return new Promise((resolve, reject) => { + const { executable, args } = getShellConfig(); + const childProcess = spawn(executable, args(command), { + cwd: options.cwd, + env: options.env, + shell: false, // Don't use another shell layer + stdio: ["ignore", "pipe", "pipe"], + }); + + const stdoutChunks: Buffer[] = []; + const stderrChunks: Buffer[] = []; + let timedOut = false; + + const timeoutId = setTimeout(() => { + timedOut = true; + childProcess.kill("SIGTERM"); + }, options.timeout); + + const abortHandler = () => { + childProcess.kill("SIGTERM"); + }; + if (options.signal) { + options.signal.addEventListener("abort", abortHandler, { once: true }); + } + + childProcess.stdout?.on("data", (chunk: Buffer) => { + stdoutChunks.push(chunk); + }); + + childProcess.stderr?.on("data", (chunk: Buffer) => { + stderrChunks.push(chunk); + }); + + childProcess.on("error", (err) => { + clearTimeout(timeoutId); + if (options.signal) { + options.signal.removeEventListener("abort", abortHandler); + } + reject(err); + }); + + childProcess.on("close", (code) => { + clearTimeout(timeoutId); + if (options.signal) { + options.signal.removeEventListener("abort", abortHandler); + } + + const stdout = Buffer.concat(stdoutChunks).toString("utf8"); + const stderr = Buffer.concat(stderrChunks).toString("utf8"); + + if (timedOut) { + reject( + Object.assign(new Error("Command timed out"), { + killed: true, + signal: "SIGTERM", + stdout, + stderr, + code, + }), + ); + return; + } + + if (options.signal?.aborted) { + reject( + Object.assign(new Error("The operation was aborted"), { + name: "AbortError", + code: "ABORT_ERR", + stdout, + stderr, + }), + ); + return; + } + + resolve({ stdout, stderr, exitCode: code }); + }); + }); +} interface BashArgs { command: string; @@ -59,8 +199,9 @@ export async function bash(args: BashArgs): Promise { if (run_in_background) { const bashId = getNextBashId(); - const childProcess = spawn(command, [], { - shell: true, + const { executable, args } = getShellConfig(); + const childProcess = spawn(executable, args(command), { + shell: false, cwd: userCwd, env: getShellEnv(), }); @@ -116,18 +257,15 @@ export async function bash(args: BashArgs): Promise { const effectiveTimeout = Math.min(Math.max(timeout, 1), 600000); try { - const options: ExecOptions = { - timeout: effectiveTimeout, - maxBuffer: 10 * 1024 * 1024, + const { stdout, stderr, exitCode } = await spawnCommand(command, { cwd: userCwd, env: getShellEnv(), + timeout: effectiveTimeout, signal, - }; - const { stdout, stderr } = await execAsync(command, options); - const stdoutStr = typeof stdout === "string" ? stdout : stdout.toString(); - const stderrStr = typeof stderr === "string" ? stderr : stderr.toString(); - let output = stdoutStr; - if (stderrStr) output = output ? `${output}\n${stderrStr}` : stderrStr; + }); + + let output = stdout; + if (stderr) output = output ? `${output}\n${stderr}` : stderr; // Apply character limit to prevent excessive token usage const { content: truncatedOutput } = truncateByChars( @@ -136,6 +274,19 @@ export async function bash(args: BashArgs): Promise { "Bash", ); + // Non-zero exit code is an error + if (exitCode !== 0 && exitCode !== null) { + return { + content: [ + { + type: "text", + text: `Exit code: ${exitCode}\n${truncatedOutput}`, + }, + ], + status: "error", + }; + } + return { content: [{ type: "text", text: truncatedOutput }], status: "success", @@ -146,7 +297,7 @@ export async function bash(args: BashArgs): Promise { stderr?: string; killed?: boolean; signal?: string; - code?: string; + code?: string | number; name?: string; }; const isAbort = @@ -161,7 +312,8 @@ export async function bash(args: BashArgs): Promise { } else { if (err.killed && err.signal === "SIGTERM") errorMessage = `Command timed out after ${effectiveTimeout}ms\n`; - if (err.code) errorMessage += `Exit code: ${err.code}\n`; + if (err.code && typeof err.code === "number") + errorMessage += `Exit code: ${err.code}\n`; if (err.stderr) errorMessage += err.stderr; else if (err.message) errorMessage += err.message; if (err.stdout) errorMessage = `${err.stdout}\n${errorMessage}`;