fix(bash): rewrite background commands to prevent pipe-open hang
Root cause: when the LLM runs `cmd &`, bash forks the process and exits immediately. The forked process inherits Node's piped stdout/ stderr FDs. Node.js waits for all holders of those FDs to close before firing the 'close' event — so the tool hangs until the background process exits (which for a server is never). Fix: add rewriteBackgroundCommand() in bash.ts. Before exec, detect commands with a trailing & background operator and inject >/dev/null 2>&1 before the & when stdout is not already redirected. This severs the pipe inheritance so Node gets 'close' immediately when the shell exits. Guards: - Commands already redirecting stdout (>, >>, &>, |) are not rewritten - && (logical AND) is not affected - & inside single-quoted strings is not affected - A brief onUpdate advisory is surfaced when rewrite happens so the LLM knows to prefer nohup/setsid for robust detachment Export rewriteBackgroundCommand from pi-coding-agent for testability. Tests: bash-background.test.ts — 12 cases covering no-op paths, rewrite paths, compound commands, and already-safe nohup patterns. Closes #733
This commit is contained in:
parent
1a85853fd8
commit
a3ff25c668
4 changed files with 176 additions and 1 deletions
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
@ -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 /(?<!&)&\s*(?:disown\s*)?(?:#.*)?$/.test(stripped.trim());
|
||||
}
|
||||
|
||||
/**
|
||||
* Determine whether a command segment already redirects stdout away from the terminal.
|
||||
* Checks for >, >>, &>, |, /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 /(?<!\d)(?:>>?|&>|>&|\|)/.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(
|
||||
/(?<!&)(&\s*(?:disown\s*)?(?:#.*)?)$/,
|
||||
">/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) => {
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@ export {
|
|||
type BashToolOptions,
|
||||
bashTool,
|
||||
createBashTool,
|
||||
rewriteBackgroundCommand,
|
||||
} from "./bash.js";
|
||||
export {
|
||||
type BashInterceptorRule,
|
||||
|
|
|
|||
|
|
@ -235,6 +235,7 @@ export {
|
|||
type BashToolInput,
|
||||
type BashToolOptions,
|
||||
bashTool,
|
||||
rewriteBackgroundCommand,
|
||||
checkBashInterception,
|
||||
type CompiledInterceptor,
|
||||
compileInterceptor,
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue