feat(terminal): Add an unconditional terminal colour policy - #2808
Conversation
🤖 Augment PR SummarySummary: This PR introduces an explicit always-on terminal color policy.
Always emits ANSI sequences even when stdout/stderr are pipes or files.
🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| sourcemeta::core::terminal_paint( | ||
| std::cout, sourcemeta::core::TerminalStream::Stdout, "streamed", | ||
| sourcemeta::core::TerminalStyle::Red); | ||
| std::cout << "\n" |
There was a problem hiding this comment.
On Windows, the child process's stdout remains in text mode, so this newline is emitted as \r\n; spawn_and_capture preserves those bytes. Consequently the exact \n assertions in noninteractive_child_execution fail on Windows.
Severity: medium
Other Locations
test/terminal/terminal_test.cc:687test/terminal/terminal_test.cc:704
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
3d17300 to
1308a1b
Compare
| /// | ||
| /// Determine whether styling is enabled for the specified stream. | ||
| /// | ||
| /// Under `TerminalColorPolicy::WhenInteractive`, styling is enabled if the |
There was a problem hiding this comment.
I feel these doc additions are just re-explaining what you documented in the enum class? Can we remove them?
| target_link_libraries(sourcemeta_core_terminal_unit | ||
| PRIVATE sourcemeta::core::terminal) | ||
|
|
||
| if(SOURCEMETA_CORE_LANG_PROCESS) |
There was a problem hiding this comment.
Why make it conditional though, with the corresponding conditional preprocessor checks in the test file? We can just make it a required dependency of this to avoid the extra complexity?
| #endif | ||
| } | ||
|
|
||
| auto main(int argc, char **argv) -> int { |
There was a problem hiding this comment.
We shouldn't have a single test file doing both TEST() invocations AND a main entry point. As the sourcemeta/core/test.h runner IS providing its own entry point to execute the other ones.
I think a better design here is to split this test CLI into a NEW .cc file in test/terminal. Then this file can spawn it on its test logic, or you can hook it directly over CMake's add_test?
It also raises an interesting point: here you are trying to test an actual CLI for this. Which means: why can't we use .clitest in this module itself for this? Sounds like the perfect dog-fooding?
1308a1b to
2499842
Compare
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
2499842 to
afe62ca
Compare
|
Thanks for the review @jviotti! I've updated the PR with all requested changes:
All CI matrix checks (including macOS, Windows MSVC/MinGW, and AddressSanitizer) are passing green. |
| PRIVATE sourcemeta::core::process) | ||
|
|
||
| sourcemeta_executable(NAMESPACE sourcemeta PROJECT core NAME terminal | ||
| VARIANT unit_child_main SOURCES terminal_child_main.cc) |
There was a problem hiding this comment.
| VARIANT unit_child_main SOURCES terminal_child_main.cc) | |
| VARIANT test_helper SOURCES terminal_child_main.cc) |
Nicer name?
|
|
||
| auto main(int argc, char **argv) -> int { | ||
| #if defined(_WIN32) | ||
| _setmode(_fileno(stdout), _O_BINARY); |
There was a problem hiding this comment.
Why is this needed btw? Might be worth a comment explaining it?
|
|
||
| if (argc > 1) { | ||
| const std::string_view mode{argv[1]}; | ||
| if (mode == "--child-noninteractive-when-interactive") { |
There was a problem hiding this comment.
At least for encouraging re-use, it might be worth using src/lang/options to parse CLI options. That's the CLI option parser we have in this project
|
|
||
| namespace { | ||
|
|
||
| class ScopedColorPolicy { |
There was a problem hiding this comment.
Interesting. I think the fact we need a complex RAII guard for this makes me think whether we should have a proper _reset() function in src/core/terminal? Then the module, inside of itself, can always figure out how to fallback to the original policy?
| EXPECT_EQ(stream_output.str(), "Streamed"); | ||
| } | ||
|
|
||
| TEST(noninteractive_child_execution) { |
There was a problem hiding this comment.
I still think replacing this test case with a couple of real .clitest files in test/terminal and registering them like that in test/terminal/CMakeLists.txt would be nicer
Add TerminalColorPolicy::Always to enable unconditional ANSI styling regardless of destination interactivity. This allows callers to explicitly request color output when streams are captured through pipes, such as in automated CLI test runners, while keeping terminal detection truthful and preserving WhenInteractive and Disabled semantics. Provide terminal_reset_color_policy() to restore stream coloring to the default policy, and exercise CLI color output across non-interactive executions using .clitest suites with sourcemeta::core::options parsing. Signed-off-by: HarshPopat23 <musichk61@gmail.com>
afe62ca to
357f18a
Compare
There was a problem hiding this comment.
3 issues found across 8 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/core/terminal/terminal.cc">
<violation number="1" location="src/core/terminal/terminal.cc:52">
P2: These reset functions hardcode `TerminalColorPolicy::WhenInteractive` and do not restore the policy that was active before the change, even though the review feedback that motivated them asked for a reset that lets the module "fall back to the original policy". Any caller that sets `Disabled` or `Always` and later invokes `terminal_reset_color_policy()` (expecting the usual restore-previous semantics of a "reset" API) silently gets `WhenInteractive` instead — for example re-enabling ANSI styling on an interactive terminal that the application explicitly disabled. Either track and restore the prior per-stream policy, or rename/document these as set-to-default so the contract is not misleading.</violation>
</file>
<file name="test/terminal/terminal_test.cc">
<violation number="1" location="test/terminal/terminal_test.cc:281">
P2: The ScopedColorPolicy guard was removed in favor of manual terminal_reset_color_policy() calls placed only at the end of each policy test. Because the test runner executes every TEST in this file sequentially in a single process, any test that aborts before its trailing reset (a debug assert, crash, or a future early return) leaks the global per-stream policy into all subsequent tests, producing cascade failures. The removed RAII guard restored state in its destructor unconditionally, so this is a real loss of test isolation. Reset the policy at the start of each policy test (several tests, including policy_reset_global, policy_always_enables_styling, policy_disabled_suppresses_styling and paint_destination_aware_*, only reset at the end), or keep a fixture/teardown that always restores.</violation>
</file>
<file name="test/terminal/terminal_child_main.cc">
<violation number="1" location="test/terminal/terminal_child_main.cc:50">
P3: The `values.empty()` branch can never run: `Options::parse` always pushes at least one value for a declared option, and throws `OptionsMissingOptionValueError` when `--color` has no value, so the intended "missing value for --color" message is unreachable and a bare `--color` aborts via an uncaught exception instead. Drop the dead branch, or wrap `application.parse(argc, argv)` in a try/catch over `sourcemeta::core::OptionsError` to emit a clean failure message.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| std::memory_order_relaxed); | ||
| } | ||
|
|
||
| auto terminal_reset_color_policy() noexcept -> void { |
There was a problem hiding this comment.
P2: These reset functions hardcode TerminalColorPolicy::WhenInteractive and do not restore the policy that was active before the change, even though the review feedback that motivated them asked for a reset that lets the module "fall back to the original policy". Any caller that sets Disabled or Always and later invokes terminal_reset_color_policy() (expecting the usual restore-previous semantics of a "reset" API) silently gets WhenInteractive instead — for example re-enabling ANSI styling on an interactive terminal that the application explicitly disabled. Either track and restore the prior per-stream policy, or rename/document these as set-to-default so the contract is not misleading.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/core/terminal/terminal.cc, line 52:
<comment>These reset functions hardcode `TerminalColorPolicy::WhenInteractive` and do not restore the policy that was active before the change, even though the review feedback that motivated them asked for a reset that lets the module "fall back to the original policy". Any caller that sets `Disabled` or `Always` and later invokes `terminal_reset_color_policy()` (expecting the usual restore-previous semantics of a "reset" API) silently gets `WhenInteractive` instead — for example re-enabling ANSI styling on an interactive terminal that the application explicitly disabled. Either track and restore the prior per-stream policy, or rename/document these as set-to-default so the contract is not misleading.</comment>
<file context>
@@ -49,6 +49,14 @@ auto terminal_set_color_policy(TerminalStream stream,
std::memory_order_relaxed);
}
+auto terminal_reset_color_policy() noexcept -> void {
+ terminal_set_color_policy(TerminalColorPolicy::WhenInteractive);
+}
</file context>
| } | ||
|
|
||
| TEST(policy_reset_per_stream) { | ||
| sourcemeta::core::terminal_set_color_policy( |
There was a problem hiding this comment.
P2: The ScopedColorPolicy guard was removed in favor of manual terminal_reset_color_policy() calls placed only at the end of each policy test. Because the test runner executes every TEST in this file sequentially in a single process, any test that aborts before its trailing reset (a debug assert, crash, or a future early return) leaks the global per-stream policy into all subsequent tests, producing cascade failures. The removed RAII guard restored state in its destructor unconditionally, so this is a real loss of test isolation. Reset the policy at the start of each policy test (several tests, including policy_reset_global, policy_always_enables_styling, policy_disabled_suppresses_styling and paint_destination_aware_*, only reset at the end), or keep a fixture/teardown that always restores.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/terminal/terminal_test.cc, line 281:
<comment>The ScopedColorPolicy guard was removed in favor of manual terminal_reset_color_policy() calls placed only at the end of each policy test. Because the test runner executes every TEST in this file sequentially in a single process, any test that aborts before its trailing reset (a debug assert, crash, or a future early return) leaks the global per-stream policy into all subsequent tests, producing cascade failures. The removed RAII guard restored state in its destructor unconditionally, so this is a real loss of test isolation. Reset the policy at the start of each policy test (several tests, including policy_reset_global, policy_always_enables_styling, policy_disabled_suppresses_styling and paint_destination_aware_*, only reset at the end), or keep a fixture/teardown that always restores.</comment>
<file context>
@@ -311,11 +253,46 @@ TEST(policy_lifecycle_and_isolation) {
+}
+
+TEST(policy_reset_per_stream) {
+ sourcemeta::core::terminal_set_color_policy(
+ sourcemeta::core::TerminalStream::Stderr,
+ sourcemeta::core::TerminalColorPolicy::Disabled);
</file context>
| auto policy{sourcemeta::core::TerminalColorPolicy::WhenInteractive}; | ||
| if (application.contains("color")) { | ||
| const auto &values{application.at("color")}; | ||
| if (values.empty()) { |
There was a problem hiding this comment.
P3: The values.empty() branch can never run: Options::parse always pushes at least one value for a declared option, and throws OptionsMissingOptionValueError when --color has no value, so the intended "missing value for --color" message is unreachable and a bare --color aborts via an uncaught exception instead. Drop the dead branch, or wrap application.parse(argc, argv) in a try/catch over sourcemeta::core::OptionsError to emit a clean failure message.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At test/terminal/terminal_child_main.cc, line 50:
<comment>The `values.empty()` branch can never run: `Options::parse` always pushes at least one value for a declared option, and throws `OptionsMissingOptionValueError` when `--color` has no value, so the intended "missing value for --color" message is unreachable and a bare `--color` aborts via an uncaught exception instead. Drop the dead branch, or wrap `application.parse(argc, argv)` in a try/catch over `sourcemeta::core::OptionsError` to emit a clean failure message.</comment>
<file context>
@@ -24,31 +26,44 @@ auto run_mode(sourcemeta::core::TerminalColorPolicy policy) -> int {
+ auto policy{sourcemeta::core::TerminalColorPolicy::WhenInteractive};
+ if (application.contains("color")) {
+ const auto &values{application.at("color")};
+ if (values.empty()) {
+ std::cerr << "missing value for --color\n";
+ return EXIT_FAILURE;
</file context>
|
Thank you @jviotti for the review! I have addressed all feedback. |
|
@HarshPopat23 Though looks like there are conflicts! |
Signed-off-by: HarshPopat23 <musichk61@gmail.com>
|
PTAL : @jviotti |
2 participants
Summary
Captured output is noninteractive, but callers sometimes explicitly request ANSI styling.
TerminalColorPolicy::Alwayssupports that use case while preserving automatic detection (WhenInteractive) and disabled styling (Disabled). It also enables downstream CLI tests to assert ANSI output through ordinary capture (e.g..clitestrunners).Changes
TerminalColorPolicy::Alwaystosourcemeta/core/terminal.h.terminal_color_enabledinterminal.ccto evaluate all three policies:WhenInteractive: preserves destination-based terminal detection.Always: unconditionally enables styling for all destinations (including pipes and redirected files).Disabled: unconditionally suppresses styling.terminal_is_interactivetruthful:Alwaysdoes not alter terminal detection.terminal.h.terminal_test.cc:ScopedColorPolicyto isolate and restore policy state across test cases.Always,Disabled, andWhenInteractivepolicy behavior and transitions.terminal_paintoverloads (string and ostream) underAlwaysandDisabled.Alwaysand plain text underWhenInteractive/Disabledwhen output is captured through pipes.