-
Notifications
You must be signed in to change notification settings - Fork 291
Avoid ANSI and progress output when running in LLM environment #7649
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,193 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| namespace Microsoft.Testing.Platform.Helpers; | ||
|
|
||
| // Copy from https://github.com/dotnet/sdk/tree/1e5d8e39d3026edb222cdf4f8d8240f1eb99f24b/src/Cli/Microsoft.DotNet.Cli.Definitions/Telemetry | ||
| internal static class LLMEnvironmentDetector | ||
| { | ||
| private static readonly EnvironmentDetectionRuleWithResult<string>[] DetectionRules = | ||
| [ | ||
| // Claude Code | ||
| new EnvironmentDetectionRuleWithResult<string>("claude", new AnyPresentEnvironmentRule("CLAUDECODE", "CLAUDE_CODE_ENTRYPOINT")), | ||
| // Cursor AI | ||
| new EnvironmentDetectionRuleWithResult<string>("cursor", new AnyPresentEnvironmentRule("CURSOR_EDITOR", "CURSOR_AI")), | ||
| // Gemini | ||
| new EnvironmentDetectionRuleWithResult<string>("gemini", new BooleanEnvironmentRule("GEMINI_CLI")), | ||
| // GitHub Copilot (legacy gh extension: GITHUB_COPILOT_CLI_MODE=true; new Copilot CLI: GH_COPILOT_WORKING_DIRECTORY is set) | ||
| new EnvironmentDetectionRuleWithResult<string>("copilot", new AnyMatchEnvironmentRule( | ||
| new BooleanEnvironmentRule("GITHUB_COPILOT_CLI_MODE"), | ||
| new AnyPresentEnvironmentRule("GH_COPILOT_WORKING_DIRECTORY"))), | ||
| // Codex CLI | ||
| new EnvironmentDetectionRuleWithResult<string>("codex", new AnyPresentEnvironmentRule("CODEX_CLI", "CODEX_SANDBOX")), | ||
| // Aider | ||
| new EnvironmentDetectionRuleWithResult<string>("aider", new EnvironmentVariableValueRule("OR_APP_NAME", "Aider")), | ||
| // Plandex | ||
| new EnvironmentDetectionRuleWithResult<string>("plandex", new EnvironmentVariableValueRule("OR_APP_NAME", "plandex")), | ||
| // Amp | ||
| new EnvironmentDetectionRuleWithResult<string>("amp", new AnyPresentEnvironmentRule("AMP_HOME")), | ||
| // Qwen Code | ||
| new EnvironmentDetectionRuleWithResult<string>("qwen", new AnyPresentEnvironmentRule("QWEN_CODE")), | ||
| // Droid | ||
| new EnvironmentDetectionRuleWithResult<string>("droid", new BooleanEnvironmentRule("DROID_CLI")), | ||
| // OpenCode | ||
| new EnvironmentDetectionRuleWithResult<string>("opencode", new AnyPresentEnvironmentRule("OPENCODE_AI")), | ||
| // Zed AI | ||
| new EnvironmentDetectionRuleWithResult<string>("zed", new AnyPresentEnvironmentRule("ZED_ENVIRONMENT", "ZED_TERM")), | ||
| // Kimi CLI | ||
| new EnvironmentDetectionRuleWithResult<string>("kimi", new BooleanEnvironmentRule("KIMI_CLI")), | ||
| // OpenHands | ||
| new EnvironmentDetectionRuleWithResult<string>("openhands", new EnvironmentVariableValueRule("OR_APP_NAME", "OpenHands")), | ||
| // Goose | ||
| new EnvironmentDetectionRuleWithResult<string>("goose", new AnyPresentEnvironmentRule("GOOSE_TERMINAL")), | ||
| // Cline | ||
| new EnvironmentDetectionRuleWithResult<string>("cline", new AnyPresentEnvironmentRule("CLINE_TASK_ID")), | ||
| // Roo Code | ||
| new EnvironmentDetectionRuleWithResult<string>("roo", new AnyPresentEnvironmentRule("ROO_CODE_TASK_ID")), | ||
| // Windsurf | ||
| new EnvironmentDetectionRuleWithResult<string>("windsurf", new AnyPresentEnvironmentRule("WINDSURF_SESSION")), | ||
| // (proposed) generic flag for Agentic usage | ||
| new EnvironmentDetectionRuleWithResult<string>("generic_agent", new BooleanEnvironmentRule("AGENT_CLI")), | ||
| ]; | ||
|
|
||
| private static string? LLMEnvironment { get; } = GetLLMEnvironment(); | ||
|
|
||
| private static string? GetLLMEnvironment() | ||
| { | ||
| string?[] results = DetectionRules.Select(r => r.GetResult()).Where(r => r != null).ToArray(); | ||
Youssef1313 marked this conversation as resolved.
Show resolved
Hide resolved
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If the string is intended for future telemetry/logging, consider adding a comment documenting that intent. Otherwise, this could be simplified to a |
||
| return results.Length > 0 ? string.Join(", ", results) : null; | ||
| } | ||
|
|
||
| public static bool IsLLMEnvironment() => !RoslynString.IsNullOrEmpty(LLMEnvironment); | ||
|
|
||
| /// <summary> | ||
| /// Base class for environment detection rules that can be evaluated against environment variables. | ||
| /// </summary> | ||
| private abstract class EnvironmentDetectionRule | ||
| { | ||
| /// <summary> | ||
| /// Evaluates the rule against the current environment. | ||
| /// </summary> | ||
| /// <returns>True if the rule matches the current environment; otherwise, false.</returns> | ||
| public abstract bool IsMatch(); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Rule that matches when any of the specified environment variables is set to "true". | ||
| /// </summary> | ||
| private sealed class BooleanEnvironmentRule : EnvironmentDetectionRule | ||
| { | ||
| private readonly string[] _variables; | ||
|
|
||
| public BooleanEnvironmentRule(params string[] variables) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The constructors use Since this is a copy from dotnet/sdk, the deviation may be intentional to keep the code synchronized — but worth calling out for awareness. If strict alignment with project conventions is preferred, these should use |
||
| => _variables = variables ?? throw new ArgumentNullException(nameof(variables)); | ||
|
|
||
| public override bool IsMatch() | ||
| #pragma warning disable RS0030 // Do not use banned APIs - fine here. | ||
| => _variables.Any(variable => EnvironmentVariableParser.ParseBool(Environment.GetEnvironmentVariable(variable), defaultValue: false)); | ||
| #pragma warning restore RS0030 // Do not use banned APIs | ||
| } | ||
|
|
||
| private static class EnvironmentVariableParser | ||
| { | ||
| public static bool ParseBool(string? str, bool defaultValue) | ||
| { | ||
| if (str is "1" || | ||
| string.Equals(str, "true", StringComparison.OrdinalIgnoreCase) || | ||
| string.Equals(str, "yes", StringComparison.OrdinalIgnoreCase) || | ||
| string.Equals(str, "on", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return true; | ||
| } | ||
|
|
||
| if (str is "0" || | ||
| string.Equals(str, "false", StringComparison.OrdinalIgnoreCase) || | ||
| string.Equals(str, "no", StringComparison.OrdinalIgnoreCase) || | ||
| string.Equals(str, "off", StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| return false; | ||
| } | ||
|
|
||
| // Not set to a known value, return default value. | ||
| return defaultValue; | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Rule that matches when any of the specified environment variables is present and not null/empty. | ||
| /// </summary> | ||
| private sealed class AnyPresentEnvironmentRule : EnvironmentDetectionRule | ||
| { | ||
| private readonly string[] _variables; | ||
|
|
||
| public AnyPresentEnvironmentRule(params string[] variables) | ||
| => _variables = variables ?? throw new ArgumentNullException(nameof(variables)); | ||
|
|
||
| public override bool IsMatch() | ||
| #pragma warning disable RS0030 // Do not use banned APIs - fine here. | ||
| => _variables.Any(variable => !RoslynString.IsNullOrEmpty(Environment.GetEnvironmentVariable(variable))); | ||
| #pragma warning restore RS0030 // Do not use banned APIs | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Rule that matches when any of the specified sub-rules match. | ||
| /// </summary> | ||
| private sealed class AnyMatchEnvironmentRule : EnvironmentDetectionRule | ||
| { | ||
| private readonly EnvironmentDetectionRule[] _rules; | ||
|
|
||
| public AnyMatchEnvironmentRule(params EnvironmentDetectionRule[] rules) | ||
| => _rules = rules ?? throw new ArgumentNullException(nameof(rules)); | ||
|
|
||
| public override bool IsMatch() | ||
| => _rules.Any(rule => rule.IsMatch()); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Rule that matches when an environment variable contains a specific value (case-insensitive). | ||
| /// </summary> | ||
| private sealed class EnvironmentVariableValueRule : EnvironmentDetectionRule | ||
| { | ||
| private readonly string _variable; | ||
| private readonly string _expectedValue; | ||
|
|
||
| public EnvironmentVariableValueRule(string variable, string expectedValue) | ||
| { | ||
| _variable = variable ?? throw new ArgumentNullException(nameof(variable)); | ||
| _expectedValue = expectedValue ?? throw new ArgumentNullException(nameof(expectedValue)); | ||
| } | ||
|
|
||
| public override bool IsMatch() | ||
| { | ||
| #pragma warning disable RS0030 // Do not use banned APIs - fine here. | ||
| string? value = Environment.GetEnvironmentVariable(_variable); | ||
| #pragma warning restore RS0030 // Do not use banned APIs | ||
| return !RoslynString.IsNullOrEmpty(value) && value.Equals(_expectedValue, StringComparison.OrdinalIgnoreCase); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Rule that matches when any of the specified environment variables is present and not null/empty, | ||
| /// and returns the associated result value. | ||
| /// </summary> | ||
| /// <typeparam name="T">The type of the result value.</typeparam> | ||
| private sealed class EnvironmentDetectionRuleWithResult<T> | ||
| where T : class | ||
| { | ||
| private readonly EnvironmentDetectionRule _rule; | ||
| private readonly T _result; | ||
|
|
||
| public EnvironmentDetectionRuleWithResult(T result, EnvironmentDetectionRule rule) | ||
| { | ||
| _rule = rule ?? throw new ArgumentNullException(nameof(rule)); | ||
| _result = result ?? throw new ArgumentNullException(nameof(result)); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Evaluates the rule and returns the result if matched. | ||
| /// </summary> | ||
| /// <returns>The result value if the rule matches; otherwise, null.</returns> | ||
| public T? GetResult() | ||
| => _rule.IsMatch() ? _result : null; | ||
| } | ||
| } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The linked issue (#7647) specifically requests adding Consider whether the env var support from #7647 should also be included here, or if that's deferred to a follow-up. The naming convention would fit naturally alongside existing vars in |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -125,7 +125,9 @@ await _policiesService.RegisterOnAbortCallbackAsync( | |
| bool inCI = string.Equals(_environment.GetEnvironmentVariable("TF_BUILD"), "true", StringComparison.OrdinalIgnoreCase) || string.Equals(_environment.GetEnvironmentVariable("GITHUB_ACTIONS"), "true", StringComparison.OrdinalIgnoreCase); | ||
|
|
||
| AnsiMode ansiMode = AnsiMode.AnsiIfPossible; | ||
| if (noAnsi) | ||
| // In LLM environments, prefer simple text output so that LLM can parse it easily. | ||
| // Note that NoAnsi also implies no progress. | ||
| if (noAnsi || LLMEnvironmentDetector.IsLLMEnvironment()) | ||
| { | ||
Youssef1313 marked this conversation as resolved.
Show resolved
Hide resolved
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment inside this // User explicitly specified --no-ansi, or an LLM environment was auto-detected.
// Use plain text output for simpler parsing.(This was flagged as a suppressed low-confidence comment by the automated reviewer — I agree it should be addressed.) |
||
| // User explicitly specified --no-ansi. | ||
| // We should respect that. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class calls
Environment.GetEnvironmentVariable()directly (suppressing RS0030 each time), but the project'sBannedSymbols.txtbans the entireSystem.Environmenttype with the directive "Use 'IEnvironment' instead".TerminalOutputDevice(the caller of this class) already properly uses theIEnvironmentabstraction via constructor injection.CIEnvironmentDetectorin TestFramework follows the same DI pattern.Additionally, the static design (
static classwith a static auto-property initialized at class load) makes this completely untestable — you can't mock environment variables without actually setting them in the process, and the result is frozen for the process lifetime.Suggestion: Follow the
CIEnvironmentDetectorpattern — make this a non-static class with constructor-injectedIEnvironment. Provide a singleton default via a staticInstanceproperty if convenient static access is still desired. This aligns with the architectural conventions and enables unit testing.