fix: enable color output in agent container with --no-color opt-out#1431
fix: enable color output in agent container with --no-color opt-out#1431
Conversation
Co-authored-by: Mossaka <5447827+Mossaka@users.noreply.github.com> Agent-Logs-Url: https://github.com/github/gh-aw-firewall/sessions/19ea0fef-c430-46ab-aa1a-b5014671c224
|
@copilot what's your evaluation on this previous comment on the NO_COLOR:
|
The original The key tradeoff:
|
|
@copilot understood. |
Added a
Usage: Users can also override per-tool via |
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
There was a problem hiding this comment.
Pull request overview
Enables ANSI color output inside the agent container by default (even without a TTY) while adding a --no-color opt-out, aiming to stabilize CLI output and snapshot/assertion behavior across ecosystems.
Changes:
- Add
WrapperConfig.noColorand a--no-colorCLI flag to control color behavior in the agent container. - Default agent container env now uses
FORCE_COLOR=1plusCOLUMNS=120andTERM=xterm-256color. - Update docker-manager tests to validate default and opt-out color env settings.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/types.ts | Adds noColor?: boolean to wrapper configuration with documentation. |
| src/cli.ts | Introduces --no-color flag and maps it into WrapperConfig.noColor. |
| src/docker-manager.ts | Switches from NO_COLOR=1 default to FORCE_COLOR=1 default; sets COLUMNS/TERM; stops forwarding host TERM. |
| src/docker-manager.test.ts | Updates/expands assertions for new color env behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ...(config.noColor | ||
| ? { NO_COLOR: '1' } | ||
| : { FORCE_COLOR: '1' }), | ||
| // Provide a reasonable terminal width and terminal type so CLI tools | ||
| // that check COLUMNS or TERM can format output correctly. | ||
| COLUMNS: '120', | ||
| TERM: 'xterm-256color', |
There was a problem hiding this comment.
Color env vars can become contradictory when --env-all (or host env forwarding) introduces the other flag: e.g. default sets FORCE_COLOR=1 but --env-all may also add host NO_COLOR=1 (since it’s a different key and not present yet), and vice versa when config.noColor is true. This can reintroduce nondeterministic color behavior and undermine the purpose of --no-color.
Consider ensuring mutual exclusivity by excluding NO_COLOR/FORCE_COLOR from --env-all passthrough and/or normalizing after env merging so only one of them is present based on config.noColor (and any explicit --env overrides).
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
|
🔮 Oracle smoke trace for PR #1431
Warning
|
The agent container sets
NO_COLOR=1, which combined with no pseudo-TTY (isatty()→false) causes CLI tools to suppress ANSI color codes. This breaks snapshot tests and output assertions across Node.js, Python, Go, and Deno ecosystems.Changes
NO_COLOR=1withFORCE_COLOR=1in agent base env vars — follows the force-color.org convention to enable color output without a TTYCOLUMNS=120andTERM=xterm-256color— provides reasonable terminal width and capability detectionTERM— the host value (oftendumbin CI) was overriding our default; users can still override via--env TERM=<value>--no-colorCLI flag — makes color behavior configurable; when passed, setsNO_COLOR=1instead ofFORCE_COLOR=1in the agent container, following the no-color.org convention for tools that need plain text outputUsage:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.