fix(core): ObjectLogger honors NO_COLOR and TTY detection before emitting ANSI colors

.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.

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/);
+        });
+    });
 });

packages/core/src/logger.ts

@@ -28,6 +28,22 @@ const LEVEL_COLORS: Record<LogLevel, string> = {
 
 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<Omit<LoggerConfig, 'file' | 'rotation' | 'name'>> & {
         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');
         }
     }