From fd528d12978f7be9fd89821e0dfbaaf13188576c Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Fri, 17 Jul 2026 17:58:14 +0800 Subject: [PATCH] fix(core): ObjectLogger honors NO_COLOR and TTY detection before emitting ANSI colors The kernel/plugin logger colorized pretty-format level tags unconditionally, so NO_COLOR=1 runs and piped/CI output still carried ANSI escapes, breaking plain-text log scanners (publish-smoke.sh had to strip ANSI before grepping). Per no-color.org, color is now emitted only when the destination stream (stdout, or stderr for error/fatal) is an interactive TTY and NO_COLOR is unset or empty. Interactive terminals keep colorized output. The optional file destination always receives plain text now. Co-Authored-By: Claude Fable 5 --- .changeset/logger-honors-no-color.md | 15 ++++ packages/core/src/logger.test.ts | 111 ++++++++++++++++++++++++++- packages/core/src/logger.ts | 54 +++++++++---- 3 files changed, 162 insertions(+), 18 deletions(-) create mode 100644 .changeset/logger-honors-no-color.md diff --git a/.changeset/logger-honors-no-color.md b/.changeset/logger-honors-no-color.md new file mode 100644 index 0000000000..5b95d7482e --- /dev/null +++ b/.changeset/logger-honors-no-color.md @@ -0,0 +1,15 @@ +--- +"@objectstack/core": patch +--- + +fix(core): ObjectLogger honors NO_COLOR and TTY detection before emitting ANSI colors + +The kernel/plugin logger (`ctx.logger`, wired by `os serve` / `os dev`) colorized its +`pretty`-format level tags unconditionally, so `NO_COLOR=1` runs and piped/CI output +still carried ANSI escapes (e.g. `\x1b[31m…ERROR\x1b[0m`), breaking plain-text log +scanners (see scripts/publish-smoke.sh, which had to strip ANSI before grepping). + +Per the no-color.org convention, color is now emitted only when the destination stream +(stdout, or stderr for error/fatal) is an interactive TTY **and** `NO_COLOR` is unset or +empty — any non-empty `NO_COLOR` value disables color. Interactive terminals keep the +existing colorized output. The optional file destination now always receives plain text. diff --git a/packages/core/src/logger.test.ts b/packages/core/src/logger.test.ts index c1d0f265ea..cafec7bb7b 100644 --- a/packages/core/src/logger.test.ts +++ b/packages/core/src/logger.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { createLogger, ObjectLogger } from './logger'; describe('ObjectLogger', () => { @@ -113,4 +113,113 @@ describe('ObjectLogger', () => { expect(() => logger.log('Compatible log')).not.toThrow(); }); }); + + describe('Color emission (no-color.org)', () => { + const ANSI = '\x1b['; + let stdoutChunks: string[]; + let stderrChunks: string[]; + const originalNoColor = process.env.NO_COLOR; + const originalStdoutTTY = Object.getOwnPropertyDescriptor(process.stdout, 'isTTY'); + const originalStderrTTY = Object.getOwnPropertyDescriptor(process.stderr, 'isTTY'); + + const setTTY = (stream: NodeJS.WriteStream, value: boolean) => { + Object.defineProperty(stream, 'isTTY', { value, configurable: true }); + }; + + // Assert on the single write() chunk carrying our message, so unrelated + // writes to the shared process streams (e.g. the test runner's own + // reporter output) can never leak into the assertions. + const chunkWith = (chunks: string[], text: string) => + chunks.find((c) => c.includes(text)) ?? ''; + + beforeEach(() => { + stdoutChunks = []; + stderrChunks = []; + vi.spyOn(process.stdout, 'write').mockImplementation(((chunk: any) => { + stdoutChunks.push(String(chunk)); + return true; + }) as any); + vi.spyOn(process.stderr, 'write').mockImplementation(((chunk: any) => { + stderrChunks.push(String(chunk)); + return true; + }) as any); + delete process.env.NO_COLOR; + }); + + afterEach(() => { + vi.restoreAllMocks(); + if (originalNoColor === undefined) delete process.env.NO_COLOR; + else process.env.NO_COLOR = originalNoColor; + const restoreTTY = (stream: NodeJS.WriteStream, desc?: PropertyDescriptor) => { + if (desc) Object.defineProperty(stream, 'isTTY', desc); + else delete (stream as any).isTTY; + }; + restoreTTY(process.stdout, originalStdoutTTY); + restoreTTY(process.stderr, originalStderrTTY); + }); + + it('colorizes pretty output on an interactive TTY by default', () => { + setTTY(process.stdout, true); + logger.info('tty message'); + const line = chunkWith(stdoutChunks, 'tty message'); + expect(line).toContain('\x1b[32m'); // info = green + expect(line).toContain('\x1b[0m'); + expect(line).toContain('INFO'); + }); + + it('emits no ANSI codes when NO_COLOR is set to any non-empty value', () => { + setTTY(process.stdout, true); + for (const value of ['1', 'true', '0']) { + stdoutChunks.length = 0; + process.env.NO_COLOR = value; + logger.info('no color message'); + const line = chunkWith(stdoutChunks, 'no color message'); + expect(line).not.toContain(ANSI); + expect(line).toMatch(/INFO no color message/); + } + }); + + it('treats an empty NO_COLOR as unset', () => { + setTTY(process.stdout, true); + process.env.NO_COLOR = ''; + logger.info('still colored'); + expect(chunkWith(stdoutChunks, 'still colored')).toContain(ANSI); + }); + + it('emits no ANSI codes when the stream is not a TTY (piped/CI output)', () => { + setTTY(process.stdout, false); + logger.info('piped message'); + const line = chunkWith(stdoutChunks, 'piped message'); + expect(line).not.toContain(ANSI); + expect(line).toMatch(/INFO piped message/); + }); + + it('gates error/fatal color on stderr TTY-ness and NO_COLOR', () => { + setTTY(process.stdout, false); + setTTY(process.stderr, true); + logger.error('boom'); + expect(chunkWith(stderrChunks, 'boom')).toContain('\x1b[31m'); // error = red + + stderrChunks.length = 0; + process.env.NO_COLOR = '1'; + logger.error('boom again'); + const line = chunkWith(stderrChunks, 'boom again'); + expect(line).not.toContain(ANSI); + expect(line).toMatch(/ERROR boom again/); + }); + + it('never writes ANSI codes to the file destination', () => { + setTTY(process.stdout, true); + const fileChunks: string[] = []; + (logger as any).fileStream = { + write: (chunk: string) => fileChunks.push(String(chunk)), + end: (cb: () => void) => cb(), + }; + logger.info('file message'); + expect(chunkWith(stdoutChunks, 'file message')).toContain(ANSI); // console keeps color… + const fileLine = chunkWith(fileChunks, 'file message'); + expect(fileLine).not.toContain(ANSI); // …the file copy stays plain + expect(fileLine).toMatch(/INFO file message/); + }); + }); }); diff --git a/packages/core/src/logger.ts b/packages/core/src/logger.ts index b6c9092b0e..cd1646ffd2 100644 --- a/packages/core/src/logger.ts +++ b/packages/core/src/logger.ts @@ -28,6 +28,22 @@ const LEVEL_COLORS: Record = { const RESET = '\x1b[0m'; +/** + * Whether ANSI color may be written to the given stream. + * + * Follows the https://no-color.org convention: a non-empty `NO_COLOR` env var + * disables color regardless of TTY, and non-TTY destinations (pipes, CI logs, + * redirected output) always get plain text so plain-text log scanners see + * uncolored level tags. Browser bundles have no `process`/TTY → plain text. + */ +function colorEnabled(stream: { isTTY?: boolean } | undefined): boolean { + if (typeof process !== 'undefined') { + const noColor = (process as any).env?.NO_COLOR; + if (noColor !== undefined && noColor !== '') return false; + } + return Boolean(stream?.isTTY); +} + export class ObjectLogger implements Logger { private config: Required> & { file?: string; @@ -96,10 +112,15 @@ export class ObjectLogger implements Logger { const hasContext = Object.keys(context).length > 0; const ts = new Date().toISOString(); - let line: string; + const isErrorLevel = level === 'error' || level === 'fatal'; + const proc = typeof process !== 'undefined' ? (process as any) : undefined; + const stream = proc ? (isErrorLevel ? proc.stderr : proc.stdout) : undefined; + + let line: string; // console output — may carry ANSI color + let plainLine: string; // file output — never colored if (this.config.format === 'json') { - line = JSON.stringify({ + line = plainLine = JSON.stringify({ time: ts, level, ...(this.config.name ? { name: this.config.name } : {}), @@ -109,27 +130,26 @@ export class ObjectLogger implements Logger { } else if (this.config.format === 'text') { const parts = [ts, level.toUpperCase(), message]; if (hasContext) parts.push(JSON.stringify(context)); - line = parts.join(' | '); + line = plainLine = parts.join(' | '); } else { // pretty - const color = LEVEL_COLORS[level] || ''; const label = this.config.name ? `[${this.config.name}] ` : ''; - line = `${color}${ts} ${level.toUpperCase()}${RESET} ${label}${message}`; - if (hasContext) line += ` ${JSON.stringify(context)}`; + const head = `${ts} ${level.toUpperCase()}`; + let tail = ` ${label}${message}`; + if (hasContext) tail += ` ${JSON.stringify(context)}`; + plainLine = head + tail; + const color = LEVEL_COLORS[level] || ''; + line = color && colorEnabled(stream) ? `${color}${head}${RESET}${tail}` : plainLine; } - const out = line + '\n'; - // Browser-safe output: prefer process streams when available, otherwise - // fall back to console. The previous unguarded `process.stderr?.write` - // throws `ReferenceError: process is not defined` in browsers because + // fall back to console. `process` may be missing entirely (browsers) or + // present without stdio streams (bundler shims) — both fall through to + // console. The previous unguarded `process.stderr?.write` threw + // `ReferenceError: process is not defined` in browsers because // `process` itself is the missing global, not just its `stderr` field. - if (typeof process !== 'undefined' && (process as any).stderr) { - if (level === 'error' || level === 'fatal') { - (process as any).stderr.write(out); - } else { - (process as any).stdout?.write(out); - } + if (stream) { + stream.write(line + '\n'); } else if (typeof console !== 'undefined') { const fn = level === 'error' || level === 'fatal' ? console.error @@ -140,7 +160,7 @@ export class ObjectLogger implements Logger { } if (this.fileStream) { - this.fileStream.write(out); + this.fileStream.write(plainLine + '\n'); } }