Add FORCE_COLOR environment variable support

[kasperk81]

Fixes #124091

[stephentoub]

These tests for System.Console. What about for M.E.Logging.Console?

Also, these tests are only validating a single env var. Can we have a few tests that validate when multiple env vars are set and thus their precedence?

[stephentoub]

🤖 Copilot Code Review — PR #124107

Holistic Assessment

Motivation: The PR addresses a real need — adding FORCE_COLOR support per the informal standard at https://force-color.org/. This aligns with the existing NO_COLOR support and enables better integration with tools like dotnet watch that redirect output but still want color.

Approach: The approach of treating DOTNET_SYSTEM_CONSOLE_ALLOW_ANSI_COLOR_REDIRECTION as a legacy alias for FORCE_COLOR is reasonable, but the implementation changes the legacy variable's semantics in a breaking way.

Summary: ⚠️ Needs Changes. The PR has two blocking issues: (1) a breaking change to DOTNET_SYSTEM_CONSOLE_ALLOW_ANSI_COLOR_REDIRECTION semantics that will cause test failures and break existing users, and (2) the NO_COLOR handling doesn't match the spec for empty strings. These must be fixed before merge.

---

Detailed Findings

❌ Breaking change for DOTNET_SYSTEM_CONSOLE_ALLOW_ANSI_COLOR_REDIRECTION

Old behavior: Color was enabled only when the variable was set to "1" or "true" (case-insensitive).

New behavior: Color is enabled for any non-empty value, including "0", "false", "no", etc.

This is a breaking change that will:

1. Fail the tests you've written — Lines 86-87 expect DOTNET_SYSTEM_CONSOLE_ALLOW_ANSI_COLOR_REDIRECTION="0" and ="false" to return false, but the code uses !string.IsNullOrEmpty() which returns true for these values.
2. Break existing users — Anyone who set DOTNET_SYSTEM_CONSOLE_ALLOW_ANSI_COLOR_REDIRECTION=0 or =false to explicitly not force color will now get color forced on.

Suggested fix: Keep the legacy "1"/"true" parsing for DOTNET_SYSTEM_CONSOLE_ALLOW_ANSI_COLOR_REDIRECTION for backward compatibility:

// FORCE_COLOR (per https://force-color.org/) always overrides other settings.
if (!string.IsNullOrEmpty(Environment.GetEnvironmentVariable("FORCE_COLOR")))
{
    s_emitAnsiColorCodes = 1;
    return true;
}

// Legacy: DOTNET_SYSTEM_CONSOLE_ALLOW_ANSI_COLOR_REDIRECTION only if "1" or "true"
string? legacyVar = Environment.GetEnvironmentVariable("DOTNET_SYSTEM_CONSOLE_ALLOW_ANSI_COLOR_REDIRECTION");
if (legacyVar is not null && (legacyVar == "1" || legacyVar.Equals("true", StringComparison.OrdinalIgnoreCase)))
{
    s_emitAnsiColorCodes = 1;
    return true;
}

---

❌ NO_COLOR empty-string handling doesn't match spec

Spec (from https://no-color.org/): "when present and not an empty string (regardless of its value), prevents the addition of ANSI color"

Current code in ConsoleUtils.cs:

enabled = Environment.GetEnvironmentVariable("NO_COLOR") is null;

Issue: On Unix systems, a variable can be set to empty (export NO_COLOR=). In this case, GetEnvironmentVariable returns "", and the check is null returns false, so color is disabled. But per spec, an empty NO_COLOR should not disable color.

Suggested fix:

enabled = string.IsNullOrEmpty(Environment.GetEnvironmentVariable("NO_COLOR"));

The same issue exists in ConsoleLoggerProvider.DoesConsoleSupportAnsi():

if (Environment.GetEnvironmentVariable("NO_COLOR") is not null)  // Should be: !string.IsNullOrEmpty(...)

---

⚠️ Missing test cases for precedence and empty-string edge cases

The tests cover the basic cases but miss some important scenarios:

1. Precedence test: FORCE_COLOR=1 + NO_COLOR=1 simultaneously — should emit escapes per the "FORCE_COLOR overrides" rule
2. Empty string edge cases:
   • FORCE_COLOR="" should NOT force color
   • NO_COLOR="" should NOT disable color
3. Environment isolation: The tests set individual env vars but don't ensure other relevant vars (FORCE_COLOR, NO_COLOR) are explicitly unset in the child process. If CI has FORCE_COLOR set globally, tests expecting no escapes could fail.

---

⚠️ Test change in RedirectedOutputDoesNotUseAnsiSequences weakens assertion

-            Assert.Equal("1234", Encoding.UTF8.GetString(data.ToArray()));
+            Assert.Contains("1234", output);

This change from Assert.Equal to Assert.Contains weakens the test — it no longer verifies that "1234" is the only output. What additional output could appear? If ANSI codes could leak through (due to FORCE_COLOR being set in CI), this change would mask the failure.

Suggestion: Keep Assert.Equal("1234", output) unless there's a specific reason for the weaker assertion.

---

✅ Good: Logic consistency between the two implementations

Both ConsoleUtils.cs and ConsoleLoggerProvider.cs now use the same priority order:

1. FORCE_COLOR/DOTNET_SYSTEM_CONSOLE_ALLOW_ANSI_COLOR_REDIRECTION (force on)
2. NO_COLOR (force off)
3. Terminal detection (default)

This consistency is good once the implementation issues are fixed.

---

✅ Good: FORCE_COLOR implementation matches spec

The FORCE_COLOR handling correctly uses !string.IsNullOrEmpty() which matches the spec: "present and not an empty string (regardless of its value)".

---

💡 Consider adding NO_COLOR support to ConsoleLoggerProvider

The PR adds NO_COLOR support to ConsoleLoggerProvider.DoesConsoleSupportAnsi(), which it didn't have before. This is a good addition that brings it in line with ConsoleUtils, but it's worth calling out as an explicit behavioral change that was not mentioned in the PR description.

---

Review generated by multi-model analysis (Claude, Gemini, GPT).

[stephentoub]

As discussed the change around DOTNET_SYSTEM_CONSOLE_ALLOW_ANSI_COLOR_REDIRECTION is ok.

[stephentoub]

@kasperk81 why did you close this?

[kasperk81]

Tests were failing and I wasn’t sure how to move forward with fixing them, so I closed it.