diff --git a/packages/pi-coding-agent/src/core/tools/bash-background.test.ts b/packages/pi-coding-agent/src/core/tools/bash-background.test.ts new file mode 100644 index 000000000..316f752de --- /dev/null +++ b/packages/pi-coding-agent/src/core/tools/bash-background.test.ts @@ -0,0 +1,91 @@ +/** + * bash-background.test.ts — Tests for rewriteBackgroundCommand + * + * Regression for #733: `cmd &` causes the bash tool to hang indefinitely + * because the background process inherits the piped stdout/stderr and keeps + * them open. rewriteBackgroundCommand injects >/dev/null 2>&1 before & when + * the command does not already redirect stdout. + */ + +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; +import { rewriteBackgroundCommand } from "./bash.js"; + +describe("rewriteBackgroundCommand", () => { + describe("no-op cases (no & operator)", () => { + it("passes through a plain command unchanged", () => { + const r = rewriteBackgroundCommand("python -m http.server 8080"); + assert.equal(r.rewritten, false); + assert.equal(r.command, "python -m http.server 8080"); + }); + + it("passes through a command with && (logical AND)", () => { + const r = rewriteBackgroundCommand("npm install && npm start"); + assert.equal(r.rewritten, false); + }); + + it("passes through a command with & inside a string", () => { + const r = rewriteBackgroundCommand("echo 'foo & bar'"); + assert.equal(r.rewritten, false); + }); + }); + + describe("rewrite cases (& backgrounding)", () => { + it("rewrites bare background command", () => { + const r = rewriteBackgroundCommand("python -m http.server 8080 &"); + assert.equal(r.rewritten, true); + assert.ok(r.command.includes(">/dev/null 2>&1"), "injects stdout redirect"); + assert.ok(r.command.includes("&"), "preserves background operator"); + }); + + it("rewrites background command with trailing whitespace", () => { + const r = rewriteBackgroundCommand("python -m http.server 8080 & "); + assert.equal(r.rewritten, true); + assert.ok(r.command.includes(">/dev/null 2>&1")); + }); + + it("rewrites background command with & disown", () => { + const r = rewriteBackgroundCommand("node server.js & disown"); + assert.equal(r.rewritten, true); + assert.ok(r.command.includes(">/dev/null 2>&1")); + }); + + it("does NOT double-inject when stdout already redirected (>)", () => { + const r = rewriteBackgroundCommand("python -m http.server 8080 > server.log &"); + assert.equal(r.rewritten, false, "already has > redirect"); + }); + + it("does NOT inject when already redirected to /dev/null", () => { + const r = rewriteBackgroundCommand("python -m http.server 8080 >/dev/null 2>&1 &"); + assert.equal(r.rewritten, false, "already fully redirected"); + }); + + it("does NOT inject when command uses a pipe", () => { + const r = rewriteBackgroundCommand("python -m http.server 8080 | tee server.log &"); + assert.equal(r.rewritten, false, "stdout piped elsewhere"); + }); + }); + + describe("compound commands", () => { + it("rewrites only the backgrounded segment in a compound command", () => { + const r = rewriteBackgroundCommand("echo starting; python -m http.server 8080 &"); + assert.equal(r.rewritten, true); + assert.ok(r.command.includes(">/dev/null 2>&1 &")); + assert.ok(r.command.includes("echo starting"), "non-background part preserved"); + }); + + it("handles multiple backgrounded commands", () => { + const r = rewriteBackgroundCommand("node server.js &\npython worker.py &"); + assert.equal(r.rewritten, true); + const occurrences = (r.command.match(/\/dev\/null/g) ?? []).length; + assert.ok(occurrences >= 2, "both background commands rewritten"); + }); + }); + + describe("nohup / already-safe patterns pass through", () => { + it("nohup ... & passes through unchanged (already redirects)", () => { + const r = rewriteBackgroundCommand("nohup python -m http.server 8080 > /dev/null 2>&1 &"); + assert.equal(r.rewritten, false); + }); + }); +}); diff --git a/packages/pi-coding-agent/src/core/tools/bash.ts b/packages/pi-coding-agent/src/core/tools/bash.ts index 6d84199c3..4e1d65257 100644 --- a/packages/pi-coding-agent/src/core/tools/bash.ts +++ b/packages/pi-coding-agent/src/core/tools/bash.ts @@ -43,6 +43,71 @@ function getTempFilePath(): string { return join(tmpdir(), `pi-bash-${id}.log`); } +/** + * Detect whether a command fragment ends with an unquoted & (background operator). + * Returns true for patterns like: `cmd &`, `cmd arg &`, `cmd & disown`, `(cmd) &`. + * Returns false when & appears inside a string literal or as &&. + */ +function endsWithBackgroundOperator(fragment: string): boolean { + // Remove content inside single-quoted strings to avoid false positives + const stripped = fragment.replace(/'[^']*'/g, "''"); + // Match trailing & not preceded by another & (i.e., not &&) + return /(?, >>, &>, |, /dev/null redirects. + */ +function hasOutputRedirect(segment: string): boolean { + // Remove single-quoted strings to avoid matching inside them + const stripped = segment.replace(/'[^']*'/g, "''"); + // Match >, >> not preceded by 2 (stderr-only) — we only care about stdout + // Also match &> (combined), >&, or a pipe | which routes stdout elsewhere + return /(?>?|&>|>&|\|)/.test(stripped); +} + +/** + * Rewrite a command that uses & for backgrounding so the background process + * does not inherit the bash tool's stdout/stderr pipes. + * + * Without this, `python -m http.server 8080 &` causes the bash tool to hang + * indefinitely because Node.js keeps the pipe open until every process that + * inherited it exits — including the long-running server. + * + * The rewrite adds `>/dev/null 2>&1` before each & where stdout is not already + * redirected, ensuring the background process detaches from the pipes while + * still producing a human-readable notice in the tool output. + * + * Returns { command: string; rewritten: boolean }. + */ +export function rewriteBackgroundCommand(command: string): { command: string; rewritten: boolean } { + // Quick pre-check: if there's no & at all, skip the more expensive processing + if (!command.includes("&")) return { command, rewritten: false }; + + // Split on ; and newlines to handle compound commands. + // We rewrite each segment independently. + // Note: this is intentionally simple and covers the common LLM patterns. + // It does not attempt to parse complex nested subshells. + const segments = command.split(/(?<=[;\n])/); + let anyRewritten = false; + + const rewrittenSegments = segments.map((segment) => { + if (!endsWithBackgroundOperator(segment)) return segment; + if (hasOutputRedirect(segment)) return segment; + + anyRewritten = true; + // Insert >/dev/null 2>&1 before the trailing & (and optional disown/comment) + return segment.replace( + /(?/dev/null 2>&1 $1", + ); + }); + + if (!anyRewritten) return { command, rewritten: false }; + return { command: rewrittenSegments.join(""), rewritten: true }; +} + const bashSchema = Type.Object({ command: Type.String({ description: "Bash command to execute" }), timeout: Type.Optional(Type.Number({ description: "Timeout in seconds (optional, no default timeout)" })), @@ -239,8 +304,25 @@ export function createBashTool(cwd: string, options?: BashToolOptions): AgentToo } } + // Rewrite background commands (&) to redirect output away from the pipes. + // Without this, `cmd &` causes the tool to hang because the background + // process inherits the piped stdout/stderr and keeps them open indefinitely. + const bgResult = rewriteBackgroundCommand(command); + const effectiveCommand = bgResult.command; + if (bgResult.rewritten) { + // Surface a brief advisory so the LLM knows what happened. + // The rewrite is transparent for the common case; explicit detachment + // (nohup, start_new_session) is preferred for robustness. + onUpdate?.({ + content: [{ + type: "text" as const, + text: "Note: Background command output redirected to /dev/null to prevent pipe hang. Use nohup or setsid for reliable detachment.", + }], + details: undefined, + }); + } // Apply command prefix if configured (e.g., "shopt -s expand_aliases" for alias support) - const resolvedCommand = sanitizeCommand(commandPrefix ? `${commandPrefix}\n${command}` : command); + const resolvedCommand = sanitizeCommand(commandPrefix ? `${commandPrefix}\n${effectiveCommand}` : effectiveCommand); const spawnContext = resolveSpawnContext(resolvedCommand, cwd, spawnHook); return new Promise((resolve, reject) => { diff --git a/packages/pi-coding-agent/src/core/tools/index.ts b/packages/pi-coding-agent/src/core/tools/index.ts index c95fe4b78..c321b3211 100644 --- a/packages/pi-coding-agent/src/core/tools/index.ts +++ b/packages/pi-coding-agent/src/core/tools/index.ts @@ -7,6 +7,7 @@ export { type BashToolOptions, bashTool, createBashTool, + rewriteBackgroundCommand, } from "./bash.js"; export { type BashInterceptorRule, diff --git a/packages/pi-coding-agent/src/index.ts b/packages/pi-coding-agent/src/index.ts index 86a808a05..a53d45f6a 100644 --- a/packages/pi-coding-agent/src/index.ts +++ b/packages/pi-coding-agent/src/index.ts @@ -235,6 +235,7 @@ export { type BashToolInput, type BashToolOptions, bashTool, + rewriteBackgroundCommand, checkBashInterception, type CompiledInterceptor, compileInterceptor,