Suppress banner and default stdout/stderr to failed in LLM environments - #8771
Conversation
…ents Extends LLMEnvironmentDetector to also drive user-facing output defaults so AI agents that run tests see less noise (and therefore consume fewer tokens) out of the box: - `--no-banner` is implicit (both platform and framework banners suppressed). - `--show-stdout` defaults to `Failed` instead of `All`. - `--show-stderr` defaults to `Failed` instead of `All`. Any explicit user value still wins. The opt-in is purely based on the existing LLM detection (CLAUDECODE, GEMINI_CLI, GITHUB_COPILOT_CLI_MODE, etc.), so humans and CI are unaffected. Refactors LLMEnvironmentDetector to instance-based (takes IEnvironment) so the new behaviors are testable via env-var injection in acceptance tests. Test infrastructure also broadly filters the LLM env-var list when spawning child processes so existing tests are not affected by the parent agent host. Related to #7664 (which proposes the same defaults change as a universal breaking change).
There was a problem hiding this comment.
Pull request overview
This PR extends Microsoft.Testing.Platform’s “LLM environment” detection so that known AI-agent CLI shells automatically get quieter default console output (banner suppressed; --show-stdout/--show-stderr default to Failed), while preserving existing behavior for humans/CI and ensuring explicit CLI arguments still take precedence.
Changes:
- Refactors
LLMEnvironmentDetectorto be instance-based with an injectedIEnvironment, enabling deterministic testing. - Updates console defaults in LLM environments: suppress banner and default stdout/stderr output to
Failedwhen the option is not explicitly provided. - Hardens acceptance test isolation by filtering LLM-related environment variables from spawned processes, and adds new acceptance tests validating the LLM-default behaviors.
Show a summary per file
| File | Description |
|---|---|
| test/Utilities/Microsoft.Testing.TestInfrastructure/WellKnownEnvironmentVariables.cs | Adds an LLM env-var allowlist and filters these vars from child processes so acceptance tests aren’t impacted by an ambient agent shell. |
| test/IntegrationTests/MSTest.Acceptance.IntegrationTests/ShowOutputOptionTests.cs | Adds acceptance coverage ensuring stdout/stderr defaults flip to Failed in LLM environments and explicit overrides still win. |
| test/IntegrationTests/Microsoft.Testing.Platform.Acceptance.IntegrationTests/NoBannerTests.cs | Adds acceptance coverage ensuring the banner is suppressed when an LLM env var is set. |
| src/Platform/Microsoft.Testing.Platform/OutputDevice/TerminalOutputDevice.cs | Defaults --show-stdout/--show-stderr to Failed when LLM is detected and the option is absent. |
| src/Platform/Microsoft.Testing.Platform/Hosts/TestHostBuilder.Utilities.cs | Skips displaying the banner when LLM environment is detected. |
| src/Platform/Microsoft.Testing.Platform/Helpers/LLMEnvironmentDetector.cs | Refactors detector to use injected IEnvironment and supports reuse for user-facing defaults. |
Copilot's findings
- Files reviewed: 6/6 changed files
- Comments generated: 1
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Evangelink
left a comment
There was a problem hiding this comment.
Review Summary
| # | Dimension | Result |
|---|---|---|
| 1 | Algorithmic Correctness | ✅ LGTM — detection logic, banner suppression, and GetShowOutputMode fallback are all correct. Explicit CLI values correctly take precedence. |
| 2 | Threading & Concurrency | ✅ LGTM — DetectionRules is static readonly; per-call instance creation avoids shared mutable state. |
| 3 | Security & IPC | ✅ LGTM — env-var reads only; no injection surface. |
| 4 | Public API Compatibility | ✅ LGTM — LLMEnvironmentDetector is internal; WellKnownEnvironmentVariables is in a test-utilities project with no PublicAPI analyzer. |
| 5 | Performance | IsLLMEnvironment() → GetLLMEnvironment() collects all matches into a string?[] and joins them, only to check is not null. The old code cached the result in a static property; the new instance design re-evaluates all 19+ rules on every call (at least twice per test startup). See inline comment on LLMEnvironmentDetector.cs:70. |
| 6 | Cross-TFM Compatibility | ✅ LGTM — only standard IEnvironment and LINQ used. |
| 7 | Resource Management | ✅ LGTM — no disposables introduced. |
| 8 | Defensive Coding | ✅ LGTM — null guards on constructor parameters. |
| 9 | Localization & Resources | ✅ LGTM — no new user-facing strings added to resources. |
| 10 | Test Isolation | ✅ LGTM — ToSkipEnvironmentVariables spreads LLMEnvironmentVariables so acceptance tests are isolated from ambient agent shells; new tests set the env var explicitly. |
| 11 | Assertion Quality | ✅ LGTM — AssertOutputContains / AssertOutputDoesNotContain are correct for the behavior under test. |
| 12 | Flakiness Patterns | ✅ LGTM — no timing-dependent code. |
| 13 | Test Completeness | ✅ LGTM — banner, stdout default, stderr default, and explicit-override tests are all covered. Stdout and stderr coverage is symmetric. |
| 14 | Data-Driven Coverage | ✅ LGTM — AllForDynamicData covers all TFMs. |
| 15 | Code Structure | ✅ LGTM — clear guard-clause structure. |
| 16 | Naming & Conventions | ✅ LGTM. |
| 17 | Documentation Accuracy | i️ NIT — --show-stdout/--show-stderr help text still says "Default is 'All'." which is wrong in LLM environments. Intentionally deferred but should be tracked. See inline comment on TerminalOutputDevice.cs:241. |
| 18 | Analyzer & Code Fix Quality | N/A — no src/Analyzers/ changes. |
| 19 | IPC Wire Compatibility | N/A — no serialized types changed. |
| 20 | Build Infrastructure | ✅ LGTM — no new dependencies. |
| 21 | Scope & PR Discipline | ✅ LGTM — OR_APP_NAME is stripped entirely from child-process env even though LLM detection only triggers for specific values (Aider, plandex, OpenHands); this is acceptable since the variable is tool-specific, and enumerating every possible value is not feasible. |
19 dimensions clean · 1 moderate · 1 nit · 0 blocking
The PR is well-structured and the core logic is correct. The one actionable item before merge is the performance concern: collapsing IsLLMEnvironment() to a short-circuiting Any(...) call (or adding a per-instance cached field) avoids unnecessary allocations and re-evaluation. The help-text gap should be tracked in a follow-up issue with a TODO(#XXXX) comment per the repo's TODO policy.
Generated by Expert Code Review (on open) for issue #8771 · sonnet46 2.8M
- Collapse IsLLMEnvironment() to short-circuiting Any() (no string allocation/join) - Guard against null IEnvironment in LLMEnvironmentDetector constructor - Expose LLMEnvironmentVariables as IReadOnlyList<string> instead of mutable array - Add TODO(#8772) for --show-stdout/--show-stderr help-text wording follow-up Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Summary
Extends
LLMEnvironmentDetectorso that AI agent CLIs (Claude Code, Gemini CLI, GitHub Copilot CLI, Aider, Cursor, etc.) also get quieter output defaults out of the box. This reduces token consumption on every test run an agent triggers, without changing anything for humans or CI.When an LLM environment is detected (existing detection — see
LLMEnvironmentDetector.DetectionRules), the following defaults flip:--no-banner)--show-stdoutAllFailed--show-stderrAllFailedANSI mode is already disabled in LLM environments today; this PR extends the same opt-in mechanism to the two largest contributors of run-time noise.
Any explicit user-provided value still wins (e.g.
--show-stdout alloverrides the new default).Why opt-in (instead of #7664's universal change)
Issue #7664 proposes changing
--show-stdout/--show-stderrdefaults toFailedfor everyone, which is a breaking change for human and CI workflows. This PR takes the LLM-opt-in subset:--show-stdoutand--show-stderrto failed. #7664 if/when it ships — the LLM defaults would simply become a no-op.Implementation notes
LLMEnvironmentDetectorrefactored fromstaticto instance with an injectedIEnvironment. Detection rules remainstatic. This makes the new behaviors testable via env-var injection.TerminalOutputDevicehoistsisLLMEnvironmentonce next to the existinginCIlookup and threads it intoGetShowOutputMode.DisplayBannerIfEnabledAsyncshort-circuits when LLM is detected (mirrors existing--no-banner/DOTNET_NOLOGObehavior; suppresses both platform and framework banners).WellKnownEnvironmentVariables.ToSkipEnvironmentVariables) broadly filters the 23 LLM env vars when spawning child processes, so existing tests are not affected by the parent agent host.--show-stdout/--show-stderrwas intentionally not changed (still saysDefault is 'All'.) to keep this PR tight and avoid 13-XLF churn. Defer to a wording-only follow-up.Tests
New acceptance tests (all green locally on
net8.0/net10.0/net462):NoBannerTests.UsingLLMEnvironmentVar_TheBannerDoesNotAppear— setsCLAUDECODE=1, asserts banner is suppressed.ShowOutputOptionTests.ShowStdout_DefaultInLLMEnvironment_ShowsStandardOutputOnlyForFailedTestsShowOutputOptionTests.ShowStderr_DefaultInLLMEnvironment_ShowsErrorOutputOnlyForFailedTestsShowOutputOptionTests.ShowStdout_All_InLLMEnvironment_StillShowsAllStandardOutput(explicit override still wins; stderr is symmetric and covered by impl)Existing
NoBannerTests,AnsiOptionTests,HelpInfoTests,HelpInfoAllExtensionsTests, and the fullShowOutputOptionTestssuite still pass.Related: #7664