diff --git a/packages/cli/src/render.ts b/packages/cli/src/render.ts index adfbc08..1da43f5 100644 --- a/packages/cli/src/render.ts +++ b/packages/cli/src/render.ts @@ -38,7 +38,9 @@ * not rendered — the child Agent's final text is already streamed through the parent * tool's output gutter. * - * No third-party color library is used; only minimal ANSI escapes. + * No third-party color library is used; only minimal ANSI escapes — and they are emitted at + * all only when the output stream supports color (see `supportsColor`): piped output, e.g. a + * nested `penguin run` driven through `exec_command`, must stay plain (#102). */ import { isEventMessage, isModelMessage, parseUserSteeringText } from "@prismshadow/penguin-core"; import type { @@ -64,31 +66,71 @@ import { renderFileToolApprovalPayload, renderPartialToolCall } from "./tool-ren import { defaultMessages } from "./i18n.js"; import type { Messages } from "./i18n.js"; -const DIM = "\x1b[2m"; -const GREEN = "\x1b[32m"; -const RED = "\x1b[31m"; -const CYAN = "\x1b[36m"; -const MAGENTA = "\x1b[35m"; -const RESET = "\x1b[0m"; +/** The ANSI codes the renderer uses; the plain palette maps every code to "" so all writes degrade to uncolored text with no further branching. */ +export interface Palette { + readonly dim: string; + readonly green: string; + readonly red: string; + readonly cyan: string; + readonly magenta: string; + readonly reset: string; +} -export function dim(text: string): string { - return `${DIM}${text}${RESET}`; +const ANSI_PALETTE: Palette = { + dim: "\x1b[2m", + green: "\x1b[32m", + red: "\x1b[31m", + cyan: "\x1b[36m", + magenta: "\x1b[35m", + reset: "\x1b[0m", +}; + +const PLAIN_PALETTE: Palette = { dim: "", green: "", red: "", cyan: "", magenta: "", reset: "" }; + +/** + * Whether ANSI color should be emitted on `stream`: requires a TTY with `NO_COLOR` + * unset/empty and `TERM` other than `dumb`; a non-empty `FORCE_COLOR` overrides all of that — + * `"0"` forces plain, anything else forces color (Node's own semantics, where `FORCE_COLOR` + * defeats `NO_COLOR`). Keeps a piped nested CLI — `penguin run` driven through + * `exec_command` — from leaking escapes into tool output (#102). + */ +export function supportsColor( + stream: { isTTY?: boolean | undefined }, + env: NodeJS.ProcessEnv = process.env, +): boolean { + const force = env.FORCE_COLOR; + if (force !== undefined && force !== "") return force !== "0"; + if (env.NO_COLOR !== undefined && env.NO_COLOR !== "") return false; + if (env.TERM === "dumb") return false; + return stream.isTTY === true; +} + +/** The palette for one output stream, decided once per renderer rather than per write. */ +function paletteFor(stream: NodeJS.WritableStream): Palette { + return supportsColor(stream as { isTTY?: boolean }) ? ANSI_PALETTE : PLAIN_PALETTE; +} + +/** stdout's palette, decided at startup: the default for the standalone helpers below (hosts write their own notices to stdout). */ +const STDOUT_PALETTE = paletteFor(process.stdout); + +export function dim(text: string, c: Palette = STDOUT_PALETTE): string { + return `${c.dim}${text}${c.reset}`; } /** The two file tools whose outputs carry git-style diffs; their `+`/`-`/`@@` lines get colored. */ const DIFF_OUTPUT_TOOLS = new Set(["edit_file", "write_file"]); -/** Color for one diff-output line, picked from its first character (null = plain). */ -function diffLineColor(firstChar: string | undefined): string | null { - if (firstChar === "+") return GREEN; - if (firstChar === "-") return RED; - if (firstChar === "@") return DIM; +/** Color for one diff-output line, picked from its first character (null = plain; a colorless palette yields "", equally falsy for callers). */ +function diffLineColor(firstChar: string | undefined, c: Palette): string | null { + if (firstChar === "+") return c.green; + if (firstChar === "-") return c.red; + if (firstChar === "@") return c.dim; return null; } /** Colors a tool call line cyan, distinguishing it from body text/thinking (review comment #5). */ -function cyan(text: string): string { - return `${CYAN}${text}${RESET}`; +function cyan(text: string, c: Palette): string { + return `${c.cyan}${text}${c.reset}`; } /** Takes the last 3 characters of an id as the on-screen pairing number. */ @@ -140,8 +182,8 @@ function humanizeDuration(ms: number): string { return `${Math.floor(whole / 60)}m${whole % 60}s`; } -export function formatAbort(p: AbortPayload, t: Messages): string { - return dim(t.abortLabel(p.reason ?? undefined)); +export function formatAbort(p: AbortPayload, t: Messages, c: Palette = STDOUT_PALETTE): string { + return dim(t.abortLabel(p.reason ?? undefined), c); } /** @@ -165,6 +207,7 @@ export function renderHistory( out: NodeJS.WritableStream, t: Messages = defaultMessages(), ): void { + const c = paletteFor(out); // tool_call_id -> tool name (keyed with the origin chain: parent/child ids may collide), // so output lines can be labeled with the tool name of their preceding call. const toolNames = new Map(); @@ -172,7 +215,7 @@ export function renderHistory( for (const msg of messages) { if (isEventMessage(msg)) { const p = msg.payload as { type?: string } & AbortPayload; - if (p.type === "abort") out.write(`${formatAbort(p, t)}\n`); + if (p.type === "abort") out.write(`${formatAbort(p, t, c)}\n`); continue; } if (!isModelMessage(msg)) continue; @@ -188,7 +231,8 @@ export function renderHistory( tool_call_id?: string; stop_reason?: string; }; - const marker = p.stop_reason && p.stop_reason !== "completed" ? dim(` [${p.stop_reason}]`) : ""; + const marker = + p.stop_reason && p.stop_reason !== "completed" ? dim(` [${p.stop_reason}]`, c) : ""; switch (p.type) { case "text": if (p.role === "user") { @@ -196,7 +240,7 @@ export function renderHistory( // rendered as distinct user-speech lines instead of a raw marker block or a prompt. const steering = parseUserSteeringText(p.text ?? ""); if (steering !== null) { - writeSteeringLines(out, steering, t); + writeSteeringLines(out, steering, t, c); } else { out.write(`\n> ${p.text ?? ""}\n`); } @@ -205,17 +249,17 @@ export function renderHistory( } break; case "image_url": - out.write(`\n> ${dim("[image]")}\n`); + out.write(`\n> ${dim("[image]", c)}\n`); break; case "thinking": - out.write(`${dim(p.thinking ?? "")}${marker}\n`); + out.write(`${dim(p.thinking ?? "", c)}${marker}\n`); break; case "tool_call": { if (p.name) toolNames.set(nameKey(msg, p.tool_call_id ?? ""), p.name); const preview = renderPartialToolCall(p.name ?? "", p.arguments ?? "", { final: true }) ?? `${p.name} ${p.arguments}`; - out.write(`${cyan(`[${callTag(p.tool_call_id ?? "")}] ${preview}`)}${marker}\n`); + out.write(`${cyan(`[${callTag(p.tool_call_id ?? "")}] ${preview}`, c)}${marker}\n`); break; } case "tool_call_output": { @@ -226,16 +270,16 @@ export function renderHistory( const label = name ? `${tag} ${name}` : tag; const colorDiff = name !== undefined && DIFF_OUTPUT_TOOLS.has(name); for (const line of (p.output ?? "").split("\n")) { - const color = colorDiff ? diffLineColor(line[0]) : null; + const color = colorDiff ? diffLineColor(line[0], c) : null; out.write( color - ? `${DIM}${label} -> ${RESET}${color}${line}${RESET}\n` - : `${DIM}${label} -> ${RESET}${line}\n`, + ? `${c.dim}${label} -> ${c.reset}${color}${line}${c.reset}\n` + : `${c.dim}${label} -> ${c.reset}${line}\n`, ); } // Attached images aren't rendered by the terminal; print one placeholder line per image. for (const _ of p.images ?? []) { - out.write(`${DIM}${label} -> [image]${RESET}\n`); + out.write(`${c.dim}${label} -> [image]${c.reset}\n`); } break; } @@ -246,9 +290,14 @@ export function renderHistory( } /** Writes a steering message's lines with the colored user prefix (shared by history rendering and the streaming renderer). */ -function writeSteeringLines(out: NodeJS.WritableStream, text: string, t: Messages): void { +function writeSteeringLines( + out: NodeJS.WritableStream, + text: string, + t: Messages, + c: Palette, +): void { for (const line of text.split("\n")) { - out.write(`${MAGENTA}${t.steerLinePrefix()}${line}${RESET}\n`); + out.write(`${c.magenta}${t.steerLinePrefix()}${line}${c.reset}\n`); } } @@ -260,6 +309,8 @@ function writeSteeringLines(out: NodeJS.WritableStream, text: string, t: Message export class StreamRenderer { private readonly out: NodeJS.WritableStream; private readonly t: Messages; + /** This renderer's palette, decided once at construction from the output stream and NO_COLOR/FORCE_COLOR/TERM (see supportsColor). */ + private readonly c: Palette; /** Pending render queue: while the screen is held (a streaming segment is in progress / awaiting user input), messages queue up here. */ private pending: OmniMessage[] = []; @@ -386,6 +437,7 @@ export class StreamRenderer { constructor(out: NodeJS.WritableStream = process.stdout, t: Messages = defaultMessages()) { this.out = out; this.t = t; + this.c = paletteFor(out); } handle(msg: OmniMessage): void { @@ -414,7 +466,7 @@ export class StreamRenderer { toolCall.payload.arguments, ); if (payload !== null) { - for (const line of payload.split("\n")) this.out.write(`${dim(line)}\n`); + for (const line of payload.split("\n")) this.out.write(`${dim(line, this.c)}\n`); // lastLineKey stays on the call's key: the payload lines belong to this call, so // the later noteApprovalDecision must not re-render the call line as "not adjacent". } @@ -449,7 +501,7 @@ export class StreamRenderer { this.renderedDecisions.add(key); this.ensureAdjacentCallLine(toolCall); this.finishLine(); - this.out.write(`${dim(this.t.approvalDecision(decision))}\n`); + this.out.write(`${dim(this.t.approvalDecision(decision), this.c)}\n`); this.lastLineKey = null; } @@ -491,7 +543,7 @@ export class StreamRenderer { const preview = renderPartialToolCall(p.name, p.arguments, { final: true }) ?? `${p.name} ${p.arguments}`; this.finishLine(); - this.out.write(`${cyan(`[${callTag(p.tool_call_id, origin)}] ${preview}`)}\n`); + this.out.write(`${cyan(`[${callTag(p.tool_call_id, origin)}] ${preview}`, this.c)}\n`); this.lastLineKey = key; } @@ -645,7 +697,7 @@ export class StreamRenderer { const steering = parseUserSteeringText(p.text); if (steering !== null) { this.finishLine(); - writeSteeringLines(this.out, steering, this.t); + writeSteeringLines(this.out, steering, this.t, this.c); this.lastLineKey = null; } } @@ -683,14 +735,14 @@ export class StreamRenderer { const p = payload as ApprovalDecisionPayload; if (this.renderedDecisions.delete(this.callLineKey(p.tool_call_id))) return; this.finishLine(); - this.out.write(`${dim(this.t.approvalDecision(p.decision))}\n`); + this.out.write(`${dim(this.t.approvalDecision(p.decision), this.c)}\n`); this.lastLineKey = null; } else if (payload.type === "abort") { // Run ended (user interrupt / retries exhausted): clear any pending retry state so the next run doesn't mistakenly print a retry line. this.pendingRetry = null; this.pendingRetryAttempt = undefined; this.finishLine(); - this.out.write(`${formatAbort(payload as AbortPayload, this.t)}\n`); + this.out.write(`${formatAbort(payload as AbortPayload, this.t, this.c)}\n`); this.lastLineKey = null; } else if (payload.type === "request_begin") { // The previous request ended in a retryable status -> this request is a retry @@ -703,7 +755,7 @@ export class StreamRenderer { const attempt = this.pendingRetryAttempt ?? 1; this.pendingRetryAttempt = undefined; this.finishLine(); - this.out.write(`${dim(this.t.reconnectLabel(this.pendingRetry, attempt))}\n`); + this.out.write(`${dim(this.t.reconnectLabel(this.pendingRetry, attempt), this.c)}\n`); this.lastLineKey = null; this.pendingRetry = null; } @@ -739,7 +791,7 @@ export class StreamRenderer { this.finishLine(); this.compactionActive = true; this.compactionTokens = 0; - this.out.write(`${dim(this.t.compactionStart(p.mode, p.reason))}\n`); + this.out.write(`${dim(this.t.compactionStart(p.mode, p.reason), this.c)}\n`); this.lastLineKey = null; } else if (payload.type === "compaction_end") { // end signals the result and shows the tokens consumed by the compaction request (if any). @@ -756,7 +808,7 @@ export class StreamRenderer { : undefined; this.compactionTokens = 0; this.out.write( - `${dim(this.t.compactionStop(p.mode, p.status, tokens, p.error_message))}\n`, + `${dim(this.t.compactionStop(p.mode, p.status, tokens, p.error_message), this.c)}\n`, ); this.lastLineKey = null; } @@ -813,7 +865,7 @@ export class StreamRenderer { return; } this.finishLine(); - this.out.write(`${dim(this.t.approvalDecision(p.decision))}\n`); + this.out.write(`${dim(this.t.approvalDecision(p.decision), this.c)}\n`); this.lastLineKey = null; } else if (msg.payload.type === "token_usage") { // Child-session usage counts toward this task's Token delta and the Session total (parent and child use the same accounting); context still follows parent-session accounting. @@ -845,7 +897,7 @@ export class StreamRenderer { return; } if (!this.inDim) { - this.out.write(DIM); + this.out.write(this.c.dim); this.inDim = true; } if (p.thinking) { @@ -870,7 +922,7 @@ export class StreamRenderer { const preview = renderPartialToolCall(partial.name, partial.arguments, { final: true }); if (preview === null) return; this.finishLine(); - this.out.write(`${cyan(`[${callTag(toolCallId)}] ${preview}`)}\n`); + this.out.write(`${cyan(`[${callTag(toolCallId)}] ${preview}`, this.c)}\n`); this.lastLineKey = key; } @@ -915,14 +967,14 @@ export class StreamRenderer { if (this.partialToolCallLineId !== p.tool_call_id) { this.finishLine(); this.partialToolCallLineId = p.tool_call_id; - this.out.write(cyan(`[${callTag(p.tool_call_id)}] ${preview}`)); + this.out.write(cyan(`[${callTag(p.tool_call_id)}] ${preview}`, this.c)); } else if (preview.startsWith(partial.lastPreview)) { - this.out.write(cyan(preview.slice(partial.lastPreview.length))); + this.out.write(cyan(preview.slice(partial.lastPreview.length), this.c)); } else { // The preview usually grows monotonically with the arguments; if escaping/folding makes it non-appendable, start a new line with the current readable state. this.finishLine(); this.partialToolCallLineId = p.tool_call_id; - this.out.write(cyan(`[${callTag(p.tool_call_id)}] ${preview}`)); + this.out.write(cyan(`[${callTag(p.tool_call_id)}] ${preview}`, this.c)); } partial.lastPreview = preview; this.inLine = true; @@ -945,7 +997,7 @@ export class StreamRenderer { if (p.images && p.images.length > 0) { this.finishLine(); for (const _ of p.images) { - this.out.write(`${DIM}${label} -> [image]${RESET}\n`); + this.out.write(`${this.c.dim}${label} -> [image]${this.c.reset}\n`); } this.lastLineKey = null; } @@ -970,19 +1022,19 @@ export class StreamRenderer { while (i < chunk.length) { let lineColor: string | null = null; if (this.toolOutLineStart) { - this.out.write(`${DIM}${label} -> ${RESET}`); + this.out.write(`${this.c.dim}${label} -> ${this.c.reset}`); this.toolOutLineStart = false; this.inLine = true; // Diff coloring keys off the line's first character. File-tool outputs arrive as // one delta of whole lines, so the first character is always in this chunk; a // line continued from a previous chunk stays plain. - if (colorDiff) lineColor = diffLineColor(chunk[i]); + if (colorDiff) lineColor = diffLineColor(chunk[i], this.c); } const nl = chunk.indexOf("\n", i); const end = nl === -1 ? chunk.length : nl; const segment = chunk.slice(i, end); if (segment) { - this.out.write(lineColor ? `${lineColor}${segment}${RESET}` : segment); + this.out.write(lineColor ? `${lineColor}${segment}${this.c.reset}` : segment); } if (nl === -1) { i = chunk.length; @@ -1038,6 +1090,7 @@ export class StreamRenderer { elapsed: humanizeDuration(this.sessionElapsedMs), elapsedDelta: signedDelta(humanizeDuration(elapsed)), }), + this.c, )}\n`, ); this.contextAtTaskStart = this.contextNow; @@ -1081,7 +1134,7 @@ export class StreamRenderer { private closeDim(): void { if (this.inDim) { - this.out.write(RESET); + this.out.write(this.c.reset); this.inDim = false; } } diff --git a/packages/cli/test/render.test.ts b/packages/cli/test/render.test.ts index 08c2a3e..92480a3 100644 --- a/packages/cli/test/render.test.ts +++ b/packages/cli/test/render.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { Writable } from "node:stream"; import { approvalDecision, @@ -21,11 +21,23 @@ import { withOrigin, } from "@prismshadow/penguin-core"; import type { MessageOrigin } from "@prismshadow/penguin-core"; -import { StreamRenderer, formatAbort, humanizeTokens, renderHistory } from "../src/render.js"; +import { + StreamRenderer, + formatAbort, + humanizeTokens, + renderHistory, + supportsColor, +} from "../src/render.js"; import { getMessages } from "../src/i18n.js"; const t = getMessages("en"); +// Several tests stub FORCE_COLOR/NO_COLOR to pin the palette decision (the collector streams +// below are not TTYs); make sure no stub leaks into the next test. +afterEach(() => { + vi.unstubAllEnvs(); +}); + function collector(): { stream: Writable; text: () => string } { let buf = ""; const stream = new Writable({ @@ -226,6 +238,9 @@ describe("StreamRenderer", () => { }); it("colors edit_file diff output lines green/red and dims hunk headers", () => { + // The collector stream is not a TTY, so color must be forced on for this test (and the + // renderer captures the decision at construction, so the stub must precede it). + vi.stubEnv("FORCE_COLOR", "1"); const { stream, text } = collector(); const r = new StreamRenderer(stream, t); r.handle(partialToolCall({ eventType: "start", name: "edit_file", toolCallId: "d1" })); @@ -259,6 +274,8 @@ describe("StreamRenderer", () => { }); it("does not diff-color non-file-tool output", () => { + // Forced color, as above: with a colorless palette this assertion would pass vacuously. + vi.stubEnv("FORCE_COLOR", "1"); const { stream, text } = collector(); const r = new StreamRenderer(stream, t); r.handle(partialToolCall({ eventType: "start", name: "exec_command", toolCallId: "d2" })); @@ -985,3 +1002,95 @@ describe("mid-run steering rendering ([user_steering] user messages)", () => { expect(stripAnsi(text())).toContain("tail output"); }); }); + +describe("supportsColor (color gating, issue #102)", () => { + it("requires a TTY when no color variables are set", () => { + expect(supportsColor({ isTTY: true }, {})).toBe(true); + expect(supportsColor({ isTTY: false }, {})).toBe(false); + expect(supportsColor({}, {})).toBe(false); + }); + + it("NO_COLOR (non-empty) and TERM=dumb turn color off even on a TTY", () => { + expect(supportsColor({ isTTY: true }, { NO_COLOR: "1" })).toBe(false); + expect(supportsColor({ isTTY: true }, { TERM: "dumb" })).toBe(false); + // An empty NO_COLOR counts as unset (no-color.org). + expect(supportsColor({ isTTY: true }, { NO_COLOR: "" })).toBe(true); + }); + + it("FORCE_COLOR overrides everything, NO_COLOR and pipes included (Node semantics)", () => { + expect(supportsColor({ isTTY: false }, { FORCE_COLOR: "1" })).toBe(true); + // The exact environment #102 observed in the nested CLI: FORCE_COLOR=3 + NO_COLOR=1 + TERM=dumb. + expect(supportsColor({ isTTY: false }, { FORCE_COLOR: "3", NO_COLOR: "1", TERM: "dumb" })).toBe( + true, + ); + expect(supportsColor({ isTTY: true }, { FORCE_COLOR: "0" })).toBe(false); + // An empty FORCE_COLOR is not an override; the normal gate applies. + expect(supportsColor({ isTTY: false }, { FORCE_COLOR: "" })).toBe(false); + }); +}); + +describe("StreamRenderer color wiring (issue #102)", () => { + it("a piped (non-TTY) stream gets no ANSI escapes at all", () => { + // Neutralize any ambient override; the collector stream is not a TTY, so the palette is plain. + vi.stubEnv("FORCE_COLOR", ""); + const { stream, text } = collector(); + const r = new StreamRenderer(stream, t); + r.handle(partialToolCall({ eventType: "start", name: "exec_command", toolCallId: "p1" })); + r.handle( + partialToolCall({ + eventType: "delta", + name: "", + arguments: '{"cmd":"cat todo.md"}', + toolCallId: "p1", + }), + ); + r.handle(partialToolCall({ eventType: "stop", name: "", toolCallId: "p1" })); + r.handle(partialToolCallOutput({ eventType: "start", toolCallId: "p1" })); + r.handle(partialToolCallOutput({ eventType: "delta", output: "hello\n", toolCallId: "p1" })); + r.handle(partialToolCallOutput({ eventType: "stop", toolCallId: "p1" })); + r.handle(thinkingMessage("hmm")); + r.handle(partialThinking("start", "")); + r.handle(partialThinking("delta", "pondering")); + r.handle(partialThinking("stop")); + const raw = text(); + expect(raw).not.toContain("\x1b"); + expect(raw).toContain("[tool-p1] exec_command <- $ cat todo.md"); + expect(raw).toContain("[tool-p1] exec_command -> hello"); + }); + + it("renderHistory on a piped stream is escape-free too", () => { + vi.stubEnv("FORCE_COLOR", ""); + const { stream, text } = collector(); + renderHistory( + [ + userText("hi"), + thinkingMessage("t"), + toolCall({ name: "exec_command", arguments: '{"cmd":"ls"}', toolCallId: "h1" }), + toolCallOutput({ output: "a.txt\n", toolCallId: "h1" }), + ], + stream, + t, + ); + const raw = text(); + expect(raw).not.toContain("\x1b"); + expect(raw).toContain("[tool-h1] exec_command <- $ ls"); + }); + + it("FORCE_COLOR=1 re-enables color on a pipe and defeats NO_COLOR", () => { + vi.stubEnv("FORCE_COLOR", "1"); + vi.stubEnv("NO_COLOR", "1"); + const { stream, text } = collector(); + const r = new StreamRenderer(stream, t); + r.handle(partialToolCall({ eventType: "start", name: "exec_command", toolCallId: "f1" })); + r.handle( + partialToolCall({ + eventType: "delta", + name: "", + arguments: '{"cmd":"ls"}', + toolCallId: "f1", + }), + ); + r.handle(partialToolCall({ eventType: "stop", name: "", toolCallId: "f1" })); + expect(text()).toContain("\x1b[36m"); + }); +}); diff --git a/packages/core/src/environment/tools/command/session-manager.ts b/packages/core/src/environment/tools/command/session-manager.ts index f0ed217..c366414 100644 --- a/packages/core/src/environment/tools/command/session-manager.ts +++ b/packages/core/src/environment/tools/command/session-manager.ts @@ -33,8 +33,8 @@ const HARDENED_ENV: NodeJS.ProcessEnv = { }; /** - * Harness-owned variables **removed** from the child environment (removed, not blanked: a - * program that checks `PORT` for presence rather than value must see nothing at all). + * Variables **removed** from the child environment (removed, not blanked: a program that + * checks `PORT` for presence rather than value must see nothing at all). * * `PORT` / `HOST` are stripped because they are never about the command being run. On the * serving paths they are the harness's own listener: `penguin web` / `penguin server` write both @@ -56,6 +56,13 @@ const HARDENED_ENV: NodeJS.ProcessEnv = { * PenguinHarness server would otherwise serve the deployment's assets instead of the ones it just * built in the workspace, silently and with no error to read. * + * `FORCE_COLOR` / `CLICOLOR_FORCE` are color-forcing overrides that Node (and the chalk-family + * libraries) deliberately let defeat `NO_COLOR`, so an inherited value would cancel the + * `NO_COLOR=1` + `TERM=dumb` hardening above and leak ANSI escapes into tool output (#102). + * Removal, not blanking, matters here too: Node reads an *empty* `FORCE_COLOR` as "force 16 + * colors on". The vault still wins, so a user who genuinely wants forced color in commands can + * set it there. + * * Deliberately **not** stripped: `PENGUIN_HOME`, `PENGUIN_WEB_DB` and the rest of the user-facing * `PENGUIN_*` settings. Those select the *data* an Agent-started harness works against, and the * self-development case may legitimately want the same data root — sharing state is a config @@ -66,6 +73,8 @@ const STRIPPED_ENV_KEYS = new Set([ "HOST", "PENGUIN_CLI_ENTRY", "PENGUIN_WEB_DIST", + "FORCE_COLOR", + "CLICOLOR_FORCE", // Desktop-mode process credentials and wiring: the shell's token authorizes the // server shutdown endpoint (and desktop-login until redeemed), and the port file is // the shell's private channel — neither is a user-facing setting, and leaking them diff --git a/packages/core/test/exec-session.test.ts b/packages/core/test/exec-session.test.ts index 0593e82..4596164 100644 --- a/packages/core/test/exec-session.test.ts +++ b/packages/core/test/exec-session.test.ts @@ -355,6 +355,29 @@ describe("harness environment variables never reach a spawned command", () => { } }); + it("inherited FORCE_COLOR is removed, so the NO_COLOR=1 hardening actually wins", async () => { + // Node deliberately lets FORCE_COLOR defeat NO_COLOR, so a nested `penguin run` under a + // color-forcing parent (issue #102 observed FORCE_COLOR=3, NO_COLOR=1, TERM=dumb at once) + // would keep emitting ANSI escapes unless the inherited override is removed outright. + const saved = { + FORCE_COLOR: process.env.FORCE_COLOR, + CLICOLOR_FORCE: process.env.CLICOLOR_FORCE, + }; + process.env.FORCE_COLOR = "3"; + process.env.CLICOLOR_FORCE = "1"; + try { + const res = await runTool(env, "exec_command", { + cmd: `node -e "console.log('F=[' + (process.env.FORCE_COLOR ?? '') + '] C=[' + (process.env.CLICOLOR_FORCE ?? '') + '] N=[' + (process.env.NO_COLOR ?? '') + ']')"`, + }); + expect(res.output).toContain("F=[] C=[] N=[1]"); + } finally { + for (const [k, v] of Object.entries(saved)) { + if (v === undefined) delete process.env[k]; + else process.env[k] = v; + } + } + }); + it("the rest of the host environment still passes through", async () => { process.env.PENGUIN_TEST_PASSTHROUGH = "kept"; try { diff --git a/packages/web/src/components/account/update-dialog.tsx b/packages/web/src/components/account/update-dialog.tsx index 78222da..2d30c14 100644 --- a/packages/web/src/components/account/update-dialog.tsx +++ b/packages/web/src/components/account/update-dialog.tsx @@ -15,6 +15,7 @@ import type { UpdateRunResponse } from "@prismshadow/penguin-server/api"; import * as api from "../../api/endpoints"; import { S } from "../../lib/strings"; import { apiErrorText } from "../../lib/api-error"; +import { stripAnsi } from "../../lib/strip-ansi"; import { Button } from "../ui/button"; import { Modal } from "../ui/modal"; @@ -117,7 +118,8 @@ export function UpdateDialog({ )} {result.output !== "" && (
-              {result.output}
+              {/* The update runs a package manager whose output may carry ANSI color when the server env forces it. */}
+              {stripAnsi(result.output)}
             
)} diff --git a/packages/web/src/features/chat/tool-call-card.tsx b/packages/web/src/features/chat/tool-call-card.tsx index 885162e..1206bfb 100644 --- a/packages/web/src/features/chat/tool-call-card.tsx +++ b/packages/web/src/features/chat/tool-call-card.tsx @@ -14,9 +14,10 @@ * segment is shown; the wait itself is marked by the amber hourglass icon alone, since the * approval block below the row is always on screen and names the tool and its arguments. */ -import { useRef, useState } from "react"; +import { useMemo, useRef, useState } from "react"; import { S } from "../../lib/strings"; import { humanizeDuration } from "../../lib/format"; +import { stripAnsi } from "../../lib/strip-ansi"; import type { StopReason } from "@prismshadow/penguin-core/omnimessage"; import { approvalKey } from "../../lib/omni/stream-model"; import type { ToolCallItem } from "../../lib/omni/stream-model"; @@ -208,6 +209,11 @@ export function ToolCallCard({ item, ctx }: { item: ToolCallItem; ctx: StreamRen const pending = ctx.pendingApprovals.get(approvalKey(ctx.origin, item.toolCallId)); const preview = previewArguments(item.name, item.argumentsText); + // Escape sequences are stripped at render time only (the stored stream/trace data keeps its + // raw bytes): hardened child envs should no longer produce any, but historical traces and + // force-color programs still can (#102). Memoized — the aggregated output can be large and + // grows on every streamed delta. + const output = useMemo(() => stripAnsi(item.output), [item.output]); // Settled once argument streaming stopped (or the complete call arrived): the subtitle's // completeness gate is lifted — whatever is there is final. const subtitle = headerSubtitle(item.name, item.argumentsText, !item.callStreaming); @@ -390,7 +396,7 @@ export function ToolCallCard({ item, ctx }: { item: ToolCallItem; ctx: StreamRen )} {(item.output || item.outputStreaming) && (
-              {item.output}
+              {output}
               {item.outputStreaming && ▌}
             
)} diff --git a/packages/web/src/lib/strip-ansi.ts b/packages/web/src/lib/strip-ansi.ts new file mode 100644 index 0000000..99e5bc1 --- /dev/null +++ b/packages/web/src/lib/strip-ansi.ts @@ -0,0 +1,32 @@ +/** + * ANSI escape stripping for raw command/tool output display. + * + * The command tools harden the child environment against color (`NO_COLOR=1`, `TERM=dumb`, + * inherited `FORCE_COLOR` removed — see core's command session-manager), but traces recorded + * before that fix and programs that force color unconditionally can still carry escape + * sequences (#102). The chat surface therefore strips them defensively at RENDER time only — + * stored trace/stream data is never mutated. + */ + +/** + * One complete ANSI escape sequence: + * - CSI: `ESC [` + parameter bytes (0x30–0x3F) + intermediate bytes (0x20–0x2F) + one final + * byte (0x40–0x7E) — covers SGR color codes such as `\x1b[36m` and `\x1b[1;31m`. + * - OSC: `ESC ]` + payload, terminated by BEL or ST (`ESC \`) — window titles, hyperlinks. + * - Other Fe escapes: `ESC` + one byte in 0x40–0x5F (minus `[` / `]`, handled above). + */ +const ANSI_SEQUENCE = /\x1b(?:\[[0-9:;<=>?]*[ -/]*[@-~]|\][^\x07\x1b]*(?:\x07|\x1b\\)|[@-Z\\^_])/g; + +/** + * An incomplete escape sequence at end-of-string: chunks reassemble into one aggregated + * string before rendering, but live output can still pause mid-sequence, so a trailing bare + * `ESC`, unfinished CSI (`ESC [ 3`) or unterminated OSC is dropped rather than shown as + * garbage; once the tail arrives the whole sequence is removed by the pattern above. + */ +const ANSI_TRAILER = /\x1b(?:\[[0-9:;<=>?]*[ -/]*|\][^\x07]*)?$/; + +/** Strips ANSI escape sequences for display. Fast path: input without an ESC byte is returned as-is (same reference). */ +export function stripAnsi(text: string): string { + if (!text.includes("\x1b")) return text; + return text.replace(ANSI_SEQUENCE, "").replace(ANSI_TRAILER, ""); +} diff --git a/packages/web/test/strip-ansi.test.ts b/packages/web/test/strip-ansi.test.ts new file mode 100644 index 0000000..45bc96e --- /dev/null +++ b/packages/web/test/strip-ansi.test.ts @@ -0,0 +1,54 @@ +/** + * stripAnsi unit tests: render-time removal of ANSI escape sequences from raw command/tool + * output (#102), including sequences split across stream chunks and cut off at end-of-string. + */ +import { describe, expect, it } from "vitest"; +import { stripAnsi } from "../src/lib/strip-ansi"; + +describe("stripAnsi", () => { + it("returns text without escapes unchanged (fast path, same reference)", () => { + const s = "plain text, no escapes [36m literal brackets stay"; + expect(stripAnsi(s)).toBe(s); + expect(stripAnsi("")).toBe(""); + }); + + it("strips plain SGR color codes", () => { + expect(stripAnsi("\x1b[36mcyan\x1b[0m")).toBe("cyan"); + expect(stripAnsi("\x1b[2mdim\x1b[0m and \x1b[32mgreen\x1b[0m")).toBe("dim and green"); + }); + + it("strips multi-parameter SGR codes", () => { + expect(stripAnsi("\x1b[1;31mbold red\x1b[0m")).toBe("bold red"); + expect(stripAnsi("\x1b[38;5;208morange\x1b[0m")).toBe("orange"); + }); + + it("cleans the issue's nested-CLI gutter shape", () => { + // What #102 showed in the tool card: a nested `penguin run`'s colored call line. + const raw = "\x1b[36m[tool-752] $ \x1b[0m\x1b[36mcat todo.md\x1b[0m\n"; + expect(stripAnsi(raw)).toBe("[tool-752] $ cat todo.md\n"); + }); + + it("strips OSC sequences (BEL- and ST-terminated) and two-byte escapes", () => { + expect(stripAnsi("\x1b]0;window title\x07rest")).toBe("rest"); + expect(stripAnsi("\x1b]8;;https://example.com\x1b\\link text\x1b]8;;\x1b\\")).toBe("link text"); + expect(stripAnsi("a\x1bMb")).toBe("ab"); + }); + + it("handles a sequence split across a chunk boundary once chunks are concatenated", () => { + const chunk1 = "before \x1b[3"; + const chunk2 = "6mblue\x1b[0m after"; + expect(stripAnsi(chunk1 + chunk2)).toBe("before blue after"); + }); + + it("drops an incomplete trailing escape sequence (stream cut mid-sequence)", () => { + expect(stripAnsi("text\x1b")).toBe("text"); + expect(stripAnsi("text\x1b[")).toBe("text"); + expect(stripAnsi("text\x1b[36")).toBe("text"); + expect(stripAnsi("text\x1b[1;3")).toBe("text"); + expect(stripAnsi("text\x1b]0;half a title")).toBe("text"); + }); + + it("keeps surrounding text intact when stripping mid-string sequences", () => { + expect(stripAnsi("ok\x1b[31mfail\x1b[0mok")).toBe("okfailok"); + }); +});