From 27c7b6ff48ccca57694a960e4af10529f77deae6 Mon Sep 17 00:00:00 2001 From: David Brailovsky Date: Wed, 7 Oct 2026 15:02:59 +0200 Subject: [PATCH] fix(coding-agent): strip ANSI sequences split across user bash output chunks closes #10504 --- packages/coding-agent/CHANGELOG.md | 1 + .../coding-agent/src/core/bash-executor.ts | 94 +++++++++---------- packages/coding-agent/src/utils/ansi.ts | 46 ++++++--- .../agent-session-bash-persistence.test.ts | 59 ++++++++++++ 4 files changed, 138 insertions(+), 62 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index ec49723ae..d887dbf94 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -25,6 +25,7 @@ - Fixed `/mcp` waiting for all servers to connect before opening; the manager now updates live and remains usable while enabling, reconnecting, or disabling servers ([#10562](https://github.com/earendil-works/pi/issues/10562)) - Fixed images being dropped as "could not be resized" when running under `node --watch` on Node 24.19+ and 26.x, where Node posts its own messages on the image resize worker channel ([#10527](https://github.com/earendil-works/pi/issues/10527)) - Fixed clipboard paste doing nothing in Termux, and failed copies there omitting the Termux:API install hint ([#10391](https://github.com/earendil-works/pi/issues/10391)) +- Fixed `!` and RPC `bash` output keeping fragments of color codes, such as a stray `m`, when a code was split across output chunks ([#10504](https://github.com/earendil-works/pi/issues/10504)) ## [1.0.4] - 2026-10-05 diff --git a/packages/coding-agent/src/core/bash-executor.ts b/packages/coding-agent/src/core/bash-executor.ts index 48d0eb717..679543ec4 100644 --- a/packages/coding-agent/src/core/bash-executor.ts +++ b/packages/coding-agent/src/core/bash-executor.ts @@ -7,7 +7,7 @@ */ import type { WriteStream } from "node:fs"; -import { stripAnsi } from "../utils/ansi.ts"; +import { splitIncompleteAnsiSuffix, stripAnsi } from "../utils/ansi.ts"; import { createOutputFileStream } from "../utils/output-files.ts"; import { sanitizeBinaryOutput } from "../utils/shell.ts"; import type { BashOperations } from "./tools/bash.ts"; @@ -70,12 +70,15 @@ export async function executeBashWithOperations( }; const decoder = new TextDecoder(); + // Unfinished escape sequence at the end of the previous chunk, completed by the next chunk. + let pendingAnsi = ""; - const onData = (data: Buffer) => { - totalBytes += data.length; - + const appendText = (rawText: string) => { // Sanitize: strip ANSI, replace binary garbage, normalize newlines - const text = sanitizeBinaryOutput(stripAnsi(decoder.decode(data, { stream: true }))).replace(/\r/g, ""); + const text = sanitizeBinaryOutput(stripAnsi(rawText)).replace(/\r/g, ""); + if (!text) { + return; + } // Start writing to temp file if exceeds threshold if (totalBytes > DEFAULT_MAX_BYTES) { @@ -100,53 +103,44 @@ export async function executeBashWithOperations( } }; + const onData = (data: Buffer) => { + totalBytes += data.length; + const { complete, pending } = splitIncompleteAnsiSuffix(pendingAnsi + decoder.decode(data, { stream: true })); + pendingAnsi = pending; + appendText(complete); + }; + + const flushOutput = () => { + const rest = pendingAnsi + decoder.decode(); + pendingAnsi = ""; + appendText(rest); + }; + + let exitCode: number | null = null; try { - const result = await operations.exec(command, cwd, { - onData, - signal: options?.signal, - }); - - const fullOutput = outputChunks.join(""); - const truncationResult = truncateTail(fullOutput); - if (truncationResult.truncated) { - ensureTempFile(); - } - if (tempFileStream) { - tempFileStream.end(); - } - const cancelled = options?.signal?.aborted ?? false; - - return { - output: truncationResult.truncated ? truncationResult.content : fullOutput, - exitCode: cancelled ? undefined : (result.exitCode ?? undefined), - cancelled, - truncated: truncationResult.truncated, - fullOutputPath: tempFilePath, - }; + ({ exitCode } = await operations.exec(command, cwd, { onData, signal: options?.signal })); } catch (err) { - // Check if it was an abort - if (options?.signal?.aborted) { - const fullOutput = outputChunks.join(""); - const truncationResult = truncateTail(fullOutput); - if (truncationResult.truncated) { - ensureTempFile(); - } - if (tempFileStream) { - tempFileStream.end(); - } - return { - output: truncationResult.truncated ? truncationResult.content : fullOutput, - exitCode: undefined, - cancelled: true, - truncated: truncationResult.truncated, - fullOutputPath: tempFilePath, - }; + // An aborted command still returns the output it produced so far + if (!options?.signal?.aborted) { + tempFileStream?.end(); + throw err; } - - if (tempFileStream) { - tempFileStream.end(); - } - - throw err; } + + flushOutput(); + const fullOutput = outputChunks.join(""); + const truncationResult = truncateTail(fullOutput); + if (truncationResult.truncated) { + ensureTempFile(); + } + tempFileStream?.end(); + const cancelled = options?.signal?.aborted ?? false; + + return { + output: truncationResult.truncated ? truncationResult.content : fullOutput, + exitCode: cancelled ? undefined : (exitCode ?? undefined), + cancelled, + truncated: truncationResult.truncated, + fullOutputPath: tempFilePath, + }; } diff --git a/packages/coding-agent/src/utils/ansi.ts b/packages/coding-agent/src/utils/ansi.ts index a95ded68f..cad5eb65b 100644 --- a/packages/coding-agent/src/utils/ansi.ts +++ b/packages/coding-agent/src/utils/ansi.ts @@ -26,23 +26,45 @@ * SOFTWARE. */ -function ansiRegex({ onlyFirst = false }: { onlyFirst?: boolean } = {}): RegExp { - // Valid string terminator sequences are BEL, ESC\, and 0x9c - const ST = "(?:\\u0007|\\u001B\\u005C|\\u009C)"; +// Valid string terminator sequences are BEL, ESC\, and 0x9c +const ST = "(?:\\u0007|\\u001B\\u005C|\\u009C)"; - // OSC sequences only: ESC ] ... ST (non-greedy until the first ST) - const osc = `(?:\\u001B\\][\\s\\S]*?${ST})`; +// OSC sequences: ESC ] ... ST +const OSC_START = "\\u001B\\]"; - // CSI and related: ESC/C1, optional intermediates, optional params (supports ; and :) then final byte - const csi = "[\\u001B\\u009B][[\\]()#;?]*(?:\\d{1,4}(?:[;:]\\d{0,4})*)?[\\dA-PR-TZcf-nq-uy=><~]"; +// CSI and related: ESC/C1, optional intermediates, optional params (supports ; and :), then final byte +const CSI_START = "[\\u001B\\u009B][[\\]()#;?]*(?:\\d{1,4}(?:[;:]\\d{0,4})*)?"; +const CSI_FINAL = "[\\dA-PR-TZcf-nq-uy=><~]"; - const pattern = `${osc}|${csi}`; +// Complete sequences. OSC is non-greedy until the first ST. +const ansiRegex = new RegExp(`(?:${OSC_START}[\\s\\S]*?${ST})|${CSI_START}${CSI_FINAL}`, "g"); - return new RegExp(pattern, onlyFirst ? undefined : "g"); +// Unfinished sequence at the end of the text: OSC without its ST (a trailing ESC may start ESC\), +// or CSI without its final byte. +const unfinishedAnsiAtEndRegex = new RegExp( + `(?:${OSC_START}(?:[^\\u0007\\u009C\\u001B]|\\u001B(?!\\\\))*|${CSI_START})$`, +); + +// Longest unfinished sequence held back while streaming. Longer ones are processed as-is. +const MAX_PENDING_ANSI_LENGTH = 256; + +/** + * Split streamed text into a part that is safe to pass to stripAnsi now and a trailing + * unfinished escape sequence that should be prepended to the next chunk. + */ +export function splitIncompleteAnsiSuffix(value: string): { complete: string; pending: string } { + if (!value.includes("\u001B") && !value.includes("\u009B")) { + return { complete: value, pending: "" }; + } + const windowStart = Math.max(0, value.length - MAX_PENDING_ANSI_LENGTH); + const match = unfinishedAnsiAtEndRegex.exec(value.slice(windowStart)); + if (!match) { + return { complete: value, pending: "" }; + } + const splitAt = windowStart + match.index; + return { complete: value.slice(0, splitAt), pending: value.slice(splitAt) }; } -const regex = ansiRegex(); - export function stripAnsi(value: string): string { if (typeof value !== "string") { throw new TypeError(`Expected a \`string\`, got \`${typeof value}\``); @@ -56,5 +78,5 @@ export function stripAnsi(value: string): string { // Even though the regex is global, we don't need to reset the `.lastIndex` // because unlike `.exec()` and `.test()`, `.replace()` does it automatically // and doing it manually has a performance penalty. - return value.replace(regex, ""); + return value.replace(ansiRegex, ""); } diff --git a/packages/coding-agent/test/suite/agent-session-bash-persistence.test.ts b/packages/coding-agent/test/suite/agent-session-bash-persistence.test.ts index a444439cc..ee9758ab9 100644 --- a/packages/coding-agent/test/suite/agent-session-bash-persistence.test.ts +++ b/packages/coding-agent/test/suite/agent-session-bash-persistence.test.ts @@ -337,4 +337,63 @@ describe("AgentSession bash and persistence characterization", () => { { id: "bash-1", delta: "world" }, ]); }); + + // Regression tests for https://github.com/earendil-works/pi/issues/10504 + describe("escape sequences split across output chunks", () => { + /** Runs a user bash command whose fake shell emits the given chunks; `beforeExit` sees what was streamed by then. */ + async function runChunks(chunks: Array, beforeExit?: (streamed: string) => void) { + const harness = await createHarness(); + harnesses.push(harness); + const deltas: string[] = []; + const operations: BashOperations = { + exec: async (_command, _cwd, options) => { + for (const chunk of chunks) { + options.onData(typeof chunk === "string" ? Buffer.from(chunk) : chunk); + } + beforeExit?.(deltas.join("")); + return { exitCode: 0 }; + }, + }; + const result = await harness.session.executeBash("custom", (delta) => deltas.push(delta), { operations }); + const recorded = harness.session.messages[harness.session.messages.length - 1]; + return { + output: result.output, + streamed: deltas.join(""), + recorded: recorded?.role === "bashExecution" ? recorded.output : undefined, + }; + } + + it("strips a color reset split inside its parameters", async () => { + const { output, streamed, recorded } = await runChunks(["\x1b[31mERROR: file.py:1\x1b[0", "m\n"]); + expect(output).toBe("ERROR: file.py:1\n"); + expect(streamed).toBe("ERROR: file.py:1\n"); + expect(recorded).toBe("ERROR: file.py:1\n"); + }); + + it("strips a color code split right after ESC", async () => { + const { output, streamed } = await runChunks(["before\x1b", "[32mafter\n"]); + expect(output).toBe("beforeafter\n"); + expect(streamed).toBe("beforeafter\n"); + }); + + it("strips an OSC sequence split before its terminator", async () => { + const { output } = await runChunks(["a\x1b]0;window ", "title\x1b", "\\b\n"]); + expect(output).toBe("ab\n"); + }); + + it("flushes an incomplete multi-byte character at the end of output", async () => { + const { output, streamed } = await runChunks(["ok", Buffer.from("\u00e9").subarray(0, 1)]); + expect(output).toBe("ok\uFFFD"); + expect(streamed).toBe("ok\uFFFD"); + }); + + it("does not hold back output behind a long unterminated sequence", async () => { + const long = "x".repeat(300); + let streamedBeforeExit = ""; + await runChunks([`\x1b]${long}`], (streamed) => { + streamedBeforeExit = streamed; + }); + expect(streamedBeforeExit).toBe(`]${long}`); + }); + }); });