diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 31571b8e9..6f09e904e 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -5,6 +5,7 @@ ### Added - Extension packages can now show their changelog after an update. Pi reads `pi.changelogPath` from the package's `package.json`, or `CHANGELOG.md` in the package root, and on the next fresh launch shows entries newer than the last seen version under "Extension Updates". The `collapseChangelog` setting condenses this to one line per package ([#5958](https://github.com/earendil-works/pi/issues/5958), [#8790](https://github.com/earendil-works/pi/pull/8790)) +- `pi.exec()` accepts a `maxBuffer` option that limits stdout and stderr combined, and its result has a `truncated` field that is true when output exceeded the limit ([#11360](https://github.com/earendil-works/pi/issues/11360)) - Device-code login (for example GitHub Copilot) now opens the verification page and copies the code when you press Enter. Providers can turn either action off with `openBrowserOnConfirm: false` or `copyCodeOnConfirm: false` ([#9282](https://github.com/earendil-works/pi/issues/9282), [#9301](https://github.com/earendil-works/pi/pull/9301)) ### Changed @@ -15,6 +16,8 @@ ### Fixed - Fixed a redundant system prompt update (tools, rules) when an extension set the same active tools in a different order, for example a reversed `pi.setActiveTools()` list. Tools that stay active now keep their order and newly activated tools are appended ([#11495](https://github.com/earendil-works/pi/issues/11495)) +- Fixed `pi.exec()` crashing pi with `RangeError: Invalid string length` when a command printed more output than fits in a string. The default output cap is the smaller of 256 MiB and the runtime string limit; past the cap, the command is terminated and the result has `truncated: true` and a nonzero exit code ([#11360](https://github.com/earendil-works/pi/issues/11360)) +- Fixed `pi.exec()` never returning when a command ignored SIGTERM after a timeout or abort. pi now sends SIGKILL 5 seconds later, as intended - Fixed `pi install` and `pi remove` adding ever deeper nested entries to the npm `package-lock.json` when the agent directory is reached through a symlink, such as `~/.pi` linked into a dotfiles repo. pi now passes the resolved real path to npm ([#11358](https://github.com/earendil-works/pi/issues/11358)) - MCP sign-in now rejects authorization endpoints and other OAuth metadata URLs that are not `http:` or `https:`. Before, a malicious MCP server could supply a `file:` or custom protocol URL, which pi passed to the platform URL handler. - Automatic `fd` and `rg` downloads no longer fetch x86-64 binaries on ARM32 and other architectures without a prebuilt release. pi now warns and recommends installing the tool with the system package manager ([#11209](https://github.com/earendil-works/pi/issues/11209)) diff --git a/packages/coding-agent/docs/extensions.md b/packages/coding-agent/docs/extensions.md index 2439d1b45..7666d5645 100644 --- a/packages/coding-agent/docs/extensions.md +++ b/packages/coding-agent/docs/extensions.md @@ -1868,13 +1868,15 @@ pi.on("session_start", () => { ### pi.exec(command, args, options?) -Execute a shell command. +Execute a command without a shell. ```typescript const result = await pi.exec("git", ["status"], { signal, timeout: 5000 }); -// result.stdout, result.stderr, result.code, result.killed +// result.stdout, result.stderr, result.code, result.killed, result.truncated ``` +Options: `signal` and `timeout` terminate the command, `cwd` sets the working directory, and `maxBuffer` limits stdout and stderr combined (default 256 MiB). When output exceeds `maxBuffer`, pi terminates the command and returns the output up to the limit with `truncated: true`, `killed: true`, and a nonzero `code`. `killed` is true whenever pi terminated the command: timeout, abort, or `maxBuffer`. A command that ignores SIGTERM gets SIGKILL after 5 seconds. + ### pi.getActiveTools() / pi.getAllTools() / pi.setActiveTools(names) Manage active tools. This works for both built-in tools and dynamically registered tools. `pi.getActiveTools()` returns the active tool names as `string[]`; `pi.getAllTools()` returns metadata for all configured tools. diff --git a/packages/coding-agent/src/core/exec.ts b/packages/coding-agent/src/core/exec.ts index cf4205f7f..b6e845f45 100644 --- a/packages/coding-agent/src/core/exec.ts +++ b/packages/coding-agent/src/core/exec.ts @@ -2,10 +2,14 @@ * Shared command execution utilities for extensions and custom tools. */ +import { constants as bufferConstants } from "node:buffer"; import { spawn } from "node:child_process"; import { StringDecoder } from "node:string_decoder"; import { waitForChildProcess } from "../utils/child-process.ts"; +const DEFAULT_EXEC_MAX_BUFFER = Math.min(256 * 1024 * 1024, bufferConstants.MAX_STRING_LENGTH); +const FORCE_KILL_DELAY_MS = 5000; + /** * Options for executing shell commands. */ @@ -16,6 +20,11 @@ export interface ExecOptions { timeout?: number; /** Working directory */ cwd?: string; + /** + * Combined stdout/stderr byte limit; exceeding it terminates the command with `truncated: true`. + * Nonnegative integer up to the runtime string limit. Default: 256 MiB, capped by that limit. + */ + maxBuffer?: number; } /** @@ -25,12 +34,15 @@ export interface ExecResult { stdout: string; stderr: string; code: number; + /** The command was terminated by pi: timeout, abort, or output over `maxBuffer`. */ killed: boolean; + /** Output exceeded `maxBuffer`; stdout and stderr are incomplete and `code` is nonzero. */ + truncated: boolean; } /** * Execute a shell command and return stdout/stderr/code. - * Supports timeout and abort signal. + * Supports timeout, abort signal, and an output cap (`maxBuffer`). */ export async function execCommand( command: string, @@ -38,79 +50,109 @@ export async function execCommand( cwd: string, options?: ExecOptions, ): Promise { - return new Promise((resolve) => { - const proc = spawn(command, args, { - cwd, - shell: false, - stdio: ["ignore", "pipe", "pipe"], - }); + const maxBuffer = options?.maxBuffer ?? DEFAULT_EXEC_MAX_BUFFER; + if (!Number.isInteger(maxBuffer) || maxBuffer < 0 || maxBuffer > bufferConstants.MAX_STRING_LENGTH) { + throw new RangeError(`maxBuffer must be an integer between 0 and ${bufferConstants.MAX_STRING_LENGTH} bytes`); + } - let stdout = ""; - let stderr = ""; - // Decode across chunks: a multi-byte UTF-8 character can be split between two chunks, - // and decoding each chunk separately would corrupt it (earendil-works/pi#11312). - const stdoutDecoder = new StringDecoder("utf8"); - const stderrDecoder = new StringDecoder("utf8"); - let killed = false; - let timeoutId: NodeJS.Timeout | undefined; - - const killProcess = () => { - if (!killed) { - killed = true; - proc.kill("SIGTERM"); - // Force kill after 5 seconds if SIGTERM doesn't work - setTimeout(() => { - if (!proc.killed) { - proc.kill("SIGKILL"); - } - }, 5000); - } - }; - - // Handle abort signal - if (options?.signal) { - if (options.signal.aborted) { - killProcess(); - } else { - options.signal.addEventListener("abort", killProcess, { once: true }); - } - } - - // Handle timeout - if (options?.timeout && options.timeout > 0) { - timeoutId = setTimeout(() => { - killProcess(); - }, options.timeout); - } - - proc.stdout?.on("data", (data) => { - stdout += stdoutDecoder.write(data); - }); - - proc.stderr?.on("data", (data) => { - stderr += stderrDecoder.write(data); - }); - - // Wait for process termination without hanging on inherited stdio handles - // held open by detached descendants. - waitForChildProcess(proc) - .then((code) => { - if (timeoutId) clearTimeout(timeoutId); - stdout += stdoutDecoder.end(); - stderr += stderrDecoder.end(); - if (options?.signal) { - options.signal.removeEventListener("abort", killProcess); - } - resolve({ stdout, stderr, code: code ?? 0, killed }); - }) - .catch((error: Error) => { - if (timeoutId) clearTimeout(timeoutId); - stdout += stdoutDecoder.end(); - stderr += stderrDecoder.end(); - if (options?.signal) { - options.signal.removeEventListener("abort", killProcess); - } - resolve({ stdout, stderr: stderr || error.message, code: 1, killed }); - }); + const proc = spawn(command, args, { + cwd, + shell: false, + stdio: ["ignore", "pipe", "pipe"], }); + + let stdout = ""; + let stderr = ""; + // Decode across chunks: a multi-byte UTF-8 character can be split between two chunks, + // and decoding each chunk separately would corrupt it (earendil-works/pi#11312). + const stdoutDecoder = new StringDecoder("utf8"); + const stderrDecoder = new StringDecoder("utf8"); + let outputBytes = 0; + let truncated = false; + let killed = false; + let timeoutId: NodeJS.Timeout | undefined; + let forceKillId: NodeJS.Timeout | undefined; + + const killProcess = () => { + if (killed) return; + killed = true; + proc.kill("SIGTERM"); + forceKillId = setTimeout(() => { + // `proc.killed` only says a signal was sent, so check whether the child actually exited. + if (proc.exitCode === null && proc.signalCode === null) { + proc.kill("SIGKILL"); + } + }, FORCE_KILL_DELAY_MS); + }; + + if (options?.signal) { + if (options.signal.aborted) { + killProcess(); + } else { + options.signal.addEventListener("abort", killProcess, { once: true }); + } + } + + if (options?.timeout && options.timeout > 0) { + timeoutId = setTimeout(killProcess, options.timeout); + } + + // Keep only bytes within the combined cap (#11360). Close both pipes on overflow so writing + // descendants cannot keep waitForChildProcess busy after the child exits. Pausing is not + // enough: Node resumes the child's stdio on exit. + const takeWithinCap = (data: Buffer): Buffer | undefined => { + if (truncated) return undefined; + const remaining = maxBuffer - outputBytes; + if (data.length <= remaining) { + outputBytes += data.length; + return data; + } + truncated = true; + proc.stdout?.destroy(); + proc.stderr?.destroy(); + killProcess(); + return remaining > 0 ? data.subarray(0, remaining) : undefined; + }; + + proc.stdout?.on("data", (data: Buffer) => { + const chunk = takeWithinCap(data); + if (chunk) stdout += stdoutDecoder.write(chunk); + }); + + proc.stderr?.on("data", (data: Buffer) => { + const chunk = takeWithinCap(data); + if (chunk) stderr += stderrDecoder.write(chunk); + }); + + let code: number | null; + try { + // Do not hang on inherited stdio handles held open by detached descendants. + code = await waitForChildProcess(proc); + } catch (error) { + if (!(error instanceof Error)) throw error; + // Child-process errors (e.g. ENOENT) are failed executions, not host failures. + code = 1; + if (!stderr) stderr = error.message; + } finally { + clearTimeout(timeoutId); + clearTimeout(forceKillId); + options?.signal?.removeEventListener("abort", killProcess); + } + + // On truncation, discard any incomplete UTF-8 suffix buffered by either decoder. + // Flushing it would append a replacement character that was not in the accepted output. + if (!truncated) { + stdout += stdoutDecoder.end(); + stderr += stderrDecoder.end(); + } + + const exitCode = code ?? 0; + return { + stdout, + stderr, + // A truncated result must never read as success. + code: truncated && exitCode === 0 ? 1 : exitCode, + killed, + truncated, + }; } diff --git a/packages/coding-agent/test/exec-runtime-limit.test.ts b/packages/coding-agent/test/exec-runtime-limit.test.ts new file mode 100644 index 000000000..879a45911 --- /dev/null +++ b/packages/coding-agent/test/exec-runtime-limit.test.ts @@ -0,0 +1,25 @@ +import * as buffer from "node:buffer"; +import { expect, it, vi } from "vitest"; +import { execCommand } from "../src/core/exec.ts"; + +vi.mock("node:buffer", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + // 32-bit Node's maximum is 16 bytes below 256 MiB. + constants: { ...actual.constants, MAX_STRING_LENGTH: 268_435_440 }, + }; +}); + +// Regression tests for https://github.com/earendil-works/pi/issues/11360 +it("executes with the default cap when the runtime string limit is below 256 MiB", async () => { + const result = await execCommand(process.execPath, ["-e", 'process.stdout.write("ok")'], process.cwd()); + + expect(result).toEqual({ stdout: "ok", stderr: "", code: 0, killed: false, truncated: false }); +}); + +it("still rejects an explicit cap above the runtime string limit", async () => { + await expect( + execCommand(process.execPath, ["-e", ""], process.cwd(), { maxBuffer: buffer.constants.MAX_STRING_LENGTH + 1 }), + ).rejects.toThrow(RangeError); +}); diff --git a/packages/coding-agent/test/exec-utf8-chunks.test.ts b/packages/coding-agent/test/exec-utf8-chunks.test.ts new file mode 100644 index 000000000..5dad0aae0 --- /dev/null +++ b/packages/coding-agent/test/exec-utf8-chunks.test.ts @@ -0,0 +1,53 @@ +import type * as ChildProcessModule from "node:child_process"; +import { ChildProcess, spawn } from "node:child_process"; +import { PassThrough } from "node:stream"; +import { setImmediate } from "node:timers/promises"; +import { expect, it, vi } from "vitest"; +import { execCommand } from "../src/core/exec.ts"; + +vi.mock("node:child_process", async (importOriginal) => ({ + ...(await importOriginal()), + spawn: vi.fn(), +})); + +// #11360: real streams control chunk boundaries without relying on OS pipe timing. +// Only process creation and termination are stubbed; decoding and process waiting are real. +it.each(["stdout", "stderr"] as const)( + "discards pending %s UTF-8 bytes when either stream exceeds the cap", + async (stream) => { + const child = new ChildProcess(); + const stdout = new PassThrough(); + const stderr = new PassThrough(); + child.stdout = stdout; + child.stderr = stderr; + vi.mocked(spawn).mockReturnValueOnce(child); + vi.spyOn(child, "kill").mockImplementation(() => { + queueMicrotask(() => { + child.emit("exit", null, "SIGTERM"); + child.emit("close", null, "SIGTERM"); + }); + return true; + }); + + const resultPromise = execCommand("test-command", [], process.cwd(), { maxBuffer: 3 }); + const output = stream === "stdout" ? stdout : stderr; + const otherOutput = stream === "stdout" ? stderr : stdout; + // The decoder first returns "a" and buffers the first byte of €. + output.write(Buffer.from([0x61, 0xe2])); + await setImmediate(); + // The other decoder also buffers the first byte of € before the shared cap is exceeded. + otherOutput.write(Buffer.from([0xe2])); + await setImmediate(); + output.write(Buffer.from([0x82, 0xac])); + + const result = await resultPromise; + expect(result).toMatchObject({ + stdout: stream === "stdout" ? "a" : "", + stderr: stream === "stderr" ? "a" : "", + truncated: true, + killed: true, + }); + expect(result.code).not.toBe(0); + expect(Buffer.byteLength(result.stdout) + Buffer.byteLength(result.stderr)).toBeLessThanOrEqual(3); + }, +); diff --git a/packages/coding-agent/test/exec.test.ts b/packages/coding-agent/test/exec.test.ts index 96232a8b3..df5a63817 100644 --- a/packages/coding-agent/test/exec.test.ts +++ b/packages/coding-agent/test/exec.test.ts @@ -1,3 +1,4 @@ +import { constants as bufferConstants } from "node:buffer"; import { describe, expect, it } from "vitest"; import { execCommand } from "../src/core/exec.ts"; @@ -9,6 +10,7 @@ describe("execCommand", () => { expect(result).toMatchObject({ code: 1, killed: false, + truncated: false, stdout: "", }); expect(result.stderr).toMatch(/ENOENT/); @@ -17,6 +19,110 @@ describe("execCommand", () => { it("keeps stderr empty when a command exits silently", async () => { const result = await execCommand(process.execPath, ["-e", "process.exit(1)"], process.cwd()); - expect(result).toEqual({ stdout: "", stderr: "", code: 1, killed: false }); + expect(result).toEqual({ stdout: "", stderr: "", code: 1, killed: false, truncated: false }); }); + + // Regression tests for https://github.com/earendil-works/pi/issues/11360 + it.each([ + -1, + 0.5, + Number.NaN, + Number.POSITIVE_INFINITY, + Number.NEGATIVE_INFINITY, + bufferConstants.MAX_STRING_LENGTH + 1, + ])("rejects invalid maxBuffer %s instead of attempting execution", async (maxBuffer) => { + // A missing command normally resolves to a failed result; invalid options must reject instead. + await expect( + execCommand("pi-command-that-does-not-exist-11360", [], process.cwd(), { maxBuffer }), + ).rejects.toThrow(RangeError); + }); + + it.each([0, bufferConstants.MAX_STRING_LENGTH])("accepts maxBuffer boundary %s", async (maxBuffer) => { + const result = await execCommand(process.execPath, ["-e", ""], process.cwd(), { maxBuffer }); + + expect(result).toEqual({ stdout: "", stderr: "", code: 0, killed: false, truncated: false }); + }); + + it("truncates all output when maxBuffer is zero", async () => { + const result = await execCommand(process.execPath, ["-e", 'process.stdout.write("x")'], process.cwd(), { + maxBuffer: 0, + }); + + expect(result).toMatchObject({ stdout: "", stderr: "", truncated: true, killed: true }); + expect(result.code).not.toBe(0); + }); + + it("caps combined output at maxBuffer, terminates the command, and reports truncation", async () => { + const script = + 'process.stderr.write("e".repeat(600)); process.stdout.write("o".repeat(600)); setInterval(() => process.stdout.write("x".repeat(1000)), 10);'; + const result = await execCommand(process.execPath, ["-e", script], process.cwd(), { maxBuffer: 1000 }); + + expect(result).toMatchObject({ truncated: true, killed: true }); + expect(result.code).not.toBe(0); + expect(Buffer.byteLength(result.stdout) + Buffer.byteLength(result.stderr)).toBe(1000); + }); + + // #11360: a byte cap must not turn a partial UTF-8 character into replacement output. + it.each([ + ["stdout", "€", 1, ""], + ["stderr", "€", 1, ""], + ["stdout", "a€🙂", 5, "a€"], + ["stderr", "a€🙂", 5, "a€"], + ] as const)( + "keeps complete UTF-8 characters from %s output %s capped at %i bytes", + async (stream, output, maxBuffer, expected) => { + const script = `process.${stream}.write(${JSON.stringify(output)})`; + const result = await execCommand(process.execPath, ["-e", script], process.cwd(), { maxBuffer }); + + expect(result).toMatchObject({ [stream]: expected, truncated: true, killed: true }); + expect(result.code).not.toBe(0); + expect(Buffer.byteLength(result.stdout) + Buffer.byteLength(result.stderr)).toBeLessThanOrEqual(maxBuffer); + }, + ); + + it("still flushes incomplete UTF-8 at normal EOF when output was not truncated", async () => { + const script = "process.stdout.write(Buffer.from([0xe2])); process.stderr.write(Buffer.from([0xf0]));"; + const result = await execCommand(process.execPath, ["-e", script], process.cwd()); + + expect(result).toEqual({ stdout: "�", stderr: "�", code: 0, killed: false, truncated: false }); + }); + + it("leaves output up to maxBuffer unchanged", async () => { + const result = await execCommand( + process.execPath, + ["-e", 'process.stdout.write("a".repeat(1000))'], + process.cwd(), + { + maxBuffer: 1000, + }, + ); + + expect(result).toEqual({ stdout: "a".repeat(1000), stderr: "", code: 0, killed: false, truncated: false }); + }); + + it.skipIf(process.platform === "win32")( + "returns when a shell's child keeps writing after the shell is terminated", + async () => { + // SIGTERM kills only `sh`; `yes` keeps the output pipes open and writing. + const result = await execCommand("sh", ["-c", "yes; echo done"], process.cwd(), { maxBuffer: 1000 }); + + expect(result).toMatchObject({ killed: true, truncated: true }); + expect(result.stdout).toBe("y\n".repeat(500)); + }, + 10_000, + ); + + it.skipIf(process.platform === "win32")( + "force-kills a command that ignores SIGTERM", + async () => { + // The handler is installed before the first write, so the kill triggered by maxBuffer + // cannot arrive before the child ignores SIGTERM. + const script = 'process.on("SIGTERM", () => {}); process.stdout.write("xx"); setInterval(() => {}, 1000);'; + const result = await execCommand(process.execPath, ["-e", script], process.cwd(), { maxBuffer: 1 }); + + expect(result).toMatchObject({ stdout: "x", killed: true, truncated: true }); + expect(result.code).not.toBe(0); + }, + 15_000, + ); }); diff --git a/packages/coding-agent/test/git-merge-and-resolve-extension.test.ts b/packages/coding-agent/test/git-merge-and-resolve-extension.test.ts index 7c8741c7e..7903a4078 100644 --- a/packages/coding-agent/test/git-merge-and-resolve-extension.test.ts +++ b/packages/coding-agent/test/git-merge-and-resolve-extension.test.ts @@ -7,8 +7,8 @@ import type { ExecResult, ExtensionAPI, ExtensionContext } from "../src/core/ext type AgentEndHandler = (event: { type: "agent_end" }, ctx: ExtensionContext) => Promise; -const ok: ExecResult = { stdout: "", stderr: "", code: 0, killed: false }; -const fail: ExecResult = { stdout: "", stderr: "error", code: 1, killed: false }; +const ok: ExecResult = { stdout: "", stderr: "", code: 0, killed: false, truncated: false }; +const fail: ExecResult = { stdout: "", stderr: "error", code: 1, killed: false, truncated: false }; /** Standard exec results for a clean repo tracking origin/main, not in a merge. */ function withUpstream(results: Map): Map { diff --git a/packages/coding-agent/test/input-transform-streaming-example.test.ts b/packages/coding-agent/test/input-transform-streaming-example.test.ts index faf95b6ba..0eb1ccf60 100644 --- a/packages/coding-agent/test/input-transform-streaming-example.test.ts +++ b/packages/coding-agent/test/input-transform-streaming-example.test.ts @@ -35,9 +35,9 @@ function setup(execResult: ExecResult) { describe("input-transform-streaming example", () => { const diffOutput = " src/index.ts | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)"; - const gitSuccess: ExecResult = { stdout: diffOutput, stderr: "", code: 0, killed: false }; - const gitEmpty: ExecResult = { stdout: "", stderr: "", code: 0, killed: false }; - const gitFail: ExecResult = { stdout: "", stderr: "not a git repo", code: 128, killed: false }; + const gitSuccess: ExecResult = { stdout: diffOutput, stderr: "", code: 0, killed: false, truncated: false }; + const gitEmpty: ExecResult = { stdout: "", stderr: "", code: 0, killed: false, truncated: false }; + const gitFail: ExecResult = { stdout: "", stderr: "not a git repo", code: 128, killed: false, truncated: false }; it("skips exec during steering", async () => { const { emit, exec } = setup(gitSuccess);