fix: centralize ANSI codes and disable colors in non-TTY output#1399
Conversation
|
Claude Security Review: no high-confidence findings. (run) |
Package TarballHow to installgh release download pr-1399-tarball --repo aws/agentcore-cli --pattern "*.tgz" --dir /tmp/pr-tarball
npm install -g /tmp/pr-tarball/aws-agentcore-0.15.0.tgz |
agentcore-cli-automation
left a comment
There was a problem hiding this comment.
Thanks for centralizing ANSI handling and adding NO_COLOR support. I have one blocking concern about the TTY detection that I think defeats the very bug this PR aims to fix.
See inline comments. Happy to approve once the per-stream gating is addressed (or after we discuss).
| * | ||
| * If NO_COLOR is set, no colors. Otherwise, enable if we have a TTY. | ||
| */ | ||
| const isTTY = process.stdout.isTTY || process.stderr.isTTY; |
There was a problem hiding this comment.
The OR-based TTY check defeats the redirect case this PR is trying to fix.
Consider the canonical scenario from the PR description:
agentcore import --source agent.yaml 2> log.txtHere process.stdout.isTTY === true (terminal) and process.stderr.isTTY === undefined (file). The check evaluates to true || undefined === true, so useColor = true. All the new color writes that go to stderr (console.error in import/command.ts, printTelemetryNotice and printUpdateNotification writing to process.stderr) will still emit ANSI codes into log.txt. The exact case the PR description shows working should still be garbled.
A few options to fix:
-
Per-stream gating — expose two color sets (or a helper) keyed off the destination stream. e.g.
const stdoutColor = !process.env.NO_COLOR && !!process.stdout.isTTY; const stderrColor = !process.env.NO_COLOR && !!process.stderr.isTTY; export const ANSI = { stdout: makeCodes(stdoutColor), stderr: makeCodes(stderrColor), control: { ... } };
Call sites that write to stderr (
console.error,process.stderr.write) useANSI.stderr.red, etc. -
AND instead of OR —
process.stdout.isTTY && process.stderr.isTTY. Pessimistic but matches the spirit of the PR (any redirection disables colors everywhere). Simpler than option 1. -
Adopt an existing library — e.g. the
chalk/supports-colorecosystem already does this correctly per stream. May be overkill if you want to keep dependencies minimal.
Could you also add a test (or update the manual repro) that specifically exercises 2> with a TTY stdout? That would catch this regression.
There was a problem hiding this comment.
fixed and verified e2e by using the and approach.
c3df7df to
f1fa129
Compare
|
Claude Security Review: no high-confidence findings. (run) |
f1fa129 to
b9bfa19
Compare
|
Claude Security Review: no high-confidence findings. (run) |
b9bfa19 to
2e47cae
Compare
|
Claude Security Review: no high-confidence findings. (run) |
|
Claude Security Review: no high-confidence findings. (run) |
9b0342c to
c18ec1d
Compare
|
Claude Security Review: no high-confidence findings. (run) |
c18ec1d to
8ce22d2
Compare
|
Claude Security Review: no high-confidence findings. (run) |
8ce22d2 to
412626d
Compare
|
Claude Security Review: no high-confidence findings. (run) |
Description
Inline ANSI escape codes were scattered across the codebase and always emitted regardless of whether output was going to a terminal or a file. This caused garbled text when CLI output was redirected (e.g.
agentcore import --source agent.yaml 2> log.txt).This PR:
src/cli/constants.tsbehind a sharedANSIobjectNO_COLORsupport — colors auto-disable when output is piped or redirectedANSIdefinition fromsrc/cli/commands/import/constants.tsTerminal control sequences (alt screen, show cursor, clear line) remain unconditional since they are only used in TUI contexts that require a TTY.
Related Issue
Closes # #1391
Documentation PR
N/A
Type of Change
Testing
How have you tested the change?
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsManual verification:
Checklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.