Skip to content

fix: enable color output in agent container with --no-color opt-out#1431

Draft
Copilot wants to merge 3 commits intomainfrom
copilot/fix-agent-container-tty-issue
Draft

fix: enable color output in agent container with --no-color opt-out#1431
Copilot wants to merge 3 commits intomainfrom
copilot/fix-agent-container-tty-issue

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Mar 25, 2026

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

  • Replace NO_COLOR=1 with FORCE_COLOR=1 in agent base env vars — follows the force-color.org convention to enable color output without a TTY
  • Add COLUMNS=120 and TERM=xterm-256color — provides reasonable terminal width and capability detection
  • Stop forwarding host TERM — the host value (often dumb in CI) was overriding our default; users can still override via --env TERM=<value>
  • Add --no-color CLI flag — makes color behavior configurable; when passed, sets NO_COLOR=1 instead of FORCE_COLOR=1 in the agent container, following the no-color.org convention for tools that need plain text output
// Before
NO_COLOR: '1',
// ...
if (process.env.TERM) environment.TERM = process.env.TERM;

// After (default)
FORCE_COLOR: '1',
COLUMNS: '120',
TERM: 'xterm-256color',

// After (with --no-color)
NO_COLOR: '1',
COLUMNS: '120',
TERM: 'xterm-256color',

Usage:

# Default: color enabled
awf --allow-domains example.com 'npm test'

# Opt out: color disabled
awf --no-color --allow-domains example.com 'npm test'

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

Copilot AI changed the title [WIP] Fix agent container TTY allocation for color detection fix: enable color output in agent container Mar 25, 2026
Copilot AI requested a review from Mossaka March 25, 2026 18:37
@Mossaka
Copy link
Copy Markdown
Collaborator

Mossaka commented Mar 25, 2026

@copilot what's your evaluation on this previous comment on the NO_COLOR:

// Disable ANSI color output from CLI tools (Rich, Chalk, etc.) inside the container.
// Tools like Rich inject ANSI escape codes that break test assertions expecting plain text.
// NO_COLOR is a standard convention (https://no-color.org/) supported by many libraries.

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 25, 2026

@copilot what's your evaluation on this previous comment on the NO_COLOR:

// Disable ANSI color output from CLI tools (Rich, Chalk, etc.) inside the container.
...

The original NO_COLOR=1 rationale was valid for a specific scenario (Rich/Chalk injecting ANSI codes that broke assertions expecting plain text), but it created a broader problem: most CLI tool test suites write their snapshot expectations with color enabled, since that's what developers see locally. Setting NO_COLOR=1 globally suppresses color for every tool in the container, causing widespread snapshot mismatches.

The key tradeoff:

  • NO_COLOR=1 protects tests that assert on plain output, but breaks tests that assert on colored output
  • FORCE_COLOR=1 protects tests that assert on colored output, but could break tests that assert on plain output

FORCE_COLOR=1 is the better default because:

  1. Most snapshot tests are written against colored output (matching the developer's local TTY)
  2. FORCE_COLOR only affects libraries that opt into the force-color.org convention — it doesn't inject ANSI codes into tools that don't support it
  3. Tools that genuinely need plain output can still set NO_COLOR=1 themselves via --env NO_COLOR=1
  4. The container already has tty: false by default, so without FORCE_COLOR, color detection fails for all proxy-aware libraries (chalk, kleur, supports-color, rich, click, etc.)

@Mossaka
Copy link
Copy Markdown
Collaborator

Mossaka commented Mar 25, 2026

@copilot understood.
Can we make this configurable?

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Mar 25, 2026

@copilot understood.
Can we make this configurable?

Added a --no-color CLI flag in eaaf7c3. It follows the same Commander --no-* pattern as --no-rate-limit:

  • Default behavior: FORCE_COLOR=1 (color enabled)
  • With --no-color: NO_COLOR=1 (color suppressed)

Usage: awf --no-color --allow-domains example.com 'npm test'

Users can also override per-tool via --env NO_COLOR=1 or --env FORCE_COLOR=0 without needing the global flag.

Copilot AI changed the title fix: enable color output in agent container fix: enable color output in agent container with --no-color opt-out Mar 25, 2026
@Mossaka Mossaka requested a review from Copilot March 25, 2026 22:02
@github-actions
Copy link
Copy Markdown
Contributor

✅ Coverage Check Passed

Overall Coverage

Metric Base PR Delta
Lines 82.69% 82.79% 📈 +0.10%
Statements 82.35% 82.48% 📈 +0.13%
Functions 81.11% 81.11% ➡️ +0.00%
Branches 75.88% 76.00% 📈 +0.12%
📁 Per-file Coverage Changes (1 files)
File Lines (Before → After) Statements (Before → After)
src/docker-manager.ts 86.3% → 86.7% (+0.42%) 85.7% → 86.2% (+0.53%)

Coverage comparison generated by scripts/ci/compare-coverage.ts

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.noColor and a --no-color CLI flag to control color behavior in the agent container.
  • Default agent container env now uses FORCE_COLOR=1 plus COLUMNS=120 and TERM=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.

Comment on lines +486 to +492
...(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',
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copilot uses AI. Check for mistakes.
@github-actions
Copy link
Copy Markdown
Contributor

🏗️ Build Test Suite Results

Ecosystem Project Build/Install Tests Status
Bun elysia 1/1 passed ✅ PASS
Bun hono 1/1 passed ✅ PASS
C++ fmt N/A ✅ PASS
C++ json N/A ✅ PASS
Deno oak N/A 1/1 passed ✅ PASS
Deno std N/A 1/1 passed ✅ PASS
.NET hello-world N/A ✅ PASS
.NET json-parse N/A ✅ PASS
Go color 1/1 passed ✅ PASS
Go env 1/1 passed ✅ PASS
Go uuid 1/1 passed ✅ PASS
Java gson 1/1 passed ✅ PASS
Java caffeine 1/1 passed ✅ PASS
Node.js clsx All passed ✅ PASS
Node.js execa All passed ✅ PASS
Node.js p-limit All passed ✅ PASS
Rust fd 1/1 passed ✅ PASS
Rust zoxide 1/1 passed ✅ PASS

Overall: 8/8 ecosystems passed — ✅ PASS

Generated by Build Test Suite for issue #1431 ·

@github-actions
Copy link
Copy Markdown
Contributor

🔮 Oracle smoke trace for PR #1431
Merged PR titles: "Pre-install commonly needed system packages in agent container image" | "docs: Fix proxy env var docs and add missing CLI flags"
Queried PR titles: "chore(deps): bump smol-toml from 1.6.0 to 1.6.1 in /docs-site" | "chore(deps): bump h3 from 1.15.6 to 1.15.10 in /docs-site"
GitHub MCP (last 2 merged PRs): ✅
safeinputs-gh PR query: ❌
Playwright title contains GitHub: ✅
Tavily web search returned results: ❌
File write + bash cat check: ✅
npm ci && npm run build: ✅
Overall status: FAIL

🔮 The oracle has spoken through Smoke Codex

Warning

⚠️ Firewall blocked 2 domains

The following domains were blocked by the firewall during workflow execution:

  • ab.chatgpt.com
  • registry.npmjs.org

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "ab.chatgpt.com"
    - "registry.npmjs.org"

See Network Configuration for more information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Agent container lacks TTY, breaking color detection and interactive output

3 participants