Skip to content

feat(terminal): Add an unconditional terminal colour policy #2808

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.

Sign up for GitHub

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

Merged
jviotti merged 2 commits into from
Sep 14, 2026
Merged

feat(terminal): Add an unconditional terminal colour policy #2808

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
357f18a
feat(terminal): add unconditional ANSI styling policy
HarshPopat23 Sep 11, 2026
d4e74ff
Merge branch 'main' into feat/terminal-color-always
HarshPopat23 Sep 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 49 additions & 2 deletions src/core/terminal/include/sourcemeta/core/terminal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,10 @@ enum class TerminalColorPolicy : std::uint8_t {
/// https://pubs.opengroup.org/onlinepubs/9699919799/functions/isatty.html
WhenInteractive,
/// Styling is unconditionally suppressed.
Disabled
Disabled,
/// Styling is unconditionally enabled regardless of destination
/// interactivity.
Always
};

/// @ingroup terminal
Expand Down Expand Up @@ -328,6 +331,49 @@ SOURCEMETA_CORE_TERMINAL_EXPORT
auto terminal_set_color_policy(TerminalStream stream,
TerminalColorPolicy policy) noexcept -> void;

/// @ingroup terminal
///
/// Reset the color policy across all streams to default
/// (`TerminalColorPolicy::WhenInteractive`).
///
/// For example:
///
/// ```cpp
/// #include <sourcemeta/core/terminal.h>
/// #include <cassert>
///
/// sourcemeta::core::terminal_set_color_policy(
/// sourcemeta::core::TerminalColorPolicy::Disabled);
/// sourcemeta::core::terminal_reset_color_policy();
/// assert(sourcemeta::core::terminal_color_policy() ==
/// sourcemeta::core::TerminalColorPolicy::WhenInteractive);
/// ```
SOURCEMETA_CORE_TERMINAL_EXPORT
auto terminal_reset_color_policy() noexcept -> void;

/// @ingroup terminal
///
/// Reset the color policy for a specific stream to default
/// (`TerminalColorPolicy::WhenInteractive`).
///
/// For example:
///
/// ```cpp
/// #include <sourcemeta/core/terminal.h>
/// #include <cassert>
///
/// sourcemeta::core::terminal_set_color_policy(
/// sourcemeta::core::TerminalStream::Stderr,
/// sourcemeta::core::TerminalColorPolicy::Disabled);
/// sourcemeta::core::terminal_reset_color_policy(
/// sourcemeta::core::TerminalStream::Stderr);
/// assert(sourcemeta::core::terminal_color_policy(
/// sourcemeta::core::TerminalStream::Stderr) ==
/// sourcemeta::core::TerminalColorPolicy::WhenInteractive);
/// ```
SOURCEMETA_CORE_TERMINAL_EXPORT
auto terminal_reset_color_policy(TerminalStream stream) noexcept -> void;

/// @ingroup terminal
///
/// Retrieve the current color policy for the specified stream.
Expand All @@ -340,7 +386,8 @@ auto terminal_set_color_policy(TerminalStream stream,
///
/// const auto policy{sourcemeta::core::terminal_color_policy()};
/// assert(policy == sourcemeta::core::TerminalColorPolicy::WhenInteractive ||
/// policy == sourcemeta::core::TerminalColorPolicy::Disabled);
/// policy == sourcemeta::core::TerminalColorPolicy::Disabled ||
/// policy == sourcemeta::core::TerminalColorPolicy::Always);
/// ```
SOURCEMETA_CORE_TERMINAL_EXPORT
auto terminal_color_policy(
Expand Down
22 changes: 19 additions & 3 deletions src/core/terminal/terminal.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#include <ostream> // std::ostream
#include <string> // std::string
#include <string_view> // std::string_view
#include <utility> // std::to_underlying
#include <utility> // std::to_underlying, std::unreachable

namespace {

Expand Down Expand Up @@ -49,14 +49,30 @@ auto terminal_set_color_policy(TerminalStream stream,
std::memory_order_relaxed);
}

auto terminal_reset_color_policy() noexcept -> void {

@cubic-dev-ai cubic-dev-ai Bot Sep 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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>

terminal_set_color_policy(TerminalColorPolicy::WhenInteractive);
}

auto terminal_reset_color_policy(TerminalStream stream) noexcept -> void {
terminal_set_color_policy(stream, TerminalColorPolicy::WhenInteractive);
}

auto terminal_color_policy(TerminalStream stream) noexcept
-> TerminalColorPolicy {
return stream_policies[stream_index(stream)].load(std::memory_order_relaxed);
}

auto terminal_color_enabled(TerminalStream stream) noexcept -> bool {
return terminal_color_policy(stream) != TerminalColorPolicy::Disabled &&
terminal_is_interactive(stream);
switch (terminal_color_policy(stream)) {
case TerminalColorPolicy::WhenInteractive:
return terminal_is_interactive(stream);
case TerminalColorPolicy::Always:
return true;
case TerminalColorPolicy::Disabled:
return false;
default:
std::unreachable();
}
}

auto terminal_sgr_reset() noexcept -> std::string_view {
Expand Down
17 changes: 17 additions & 0 deletions test/terminal/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,20 @@ sourcemeta_test(NAMESPACE sourcemeta PROJECT core NAME terminal

target_link_libraries(sourcemeta_core_terminal_unit
PRIVATE sourcemeta::core::terminal)

sourcemeta_executable(NAMESPACE sourcemeta PROJECT core NAME terminal
VARIANT test_helper SOURCES terminal_child_main.cc)
target_link_libraries(sourcemeta_core_terminal_test_helper
PRIVATE sourcemeta::core::terminal
PRIVATE sourcemeta::core::options)

macro(add_terminal_test name)
add_test(NAME core.terminal.${name} COMMAND
"$<TARGET_FILE:sourcemeta_core_clitest>"
"${CMAKE_CURRENT_SOURCE_DIR}/${name}.clitest"
--binary "$<TARGET_FILE:sourcemeta_core_terminal_test_helper>")
endmacro()

Comment thread
jviotti marked this conversation as resolved.
add_terminal_test(color_auto)
add_terminal_test(color_always)
add_terminal_test(color_never)
6 changes: 6 additions & 0 deletions test/terminal/color_always.clitest
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
RUN --color always STDIN /dev/null IN . INTO result.txt EXPECTING 0
WRITE expected.txt UNTIL END
1> streamed
1> painted
END
COMPARE result.txt AGAINST expected.txt
6 changes: 6 additions & 0 deletions test/terminal/color_auto.clitest
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
RUN --color auto STDIN /dev/null IN . INTO result.txt EXPECTING 0
WRITE expected.txt UNTIL END
1> streamed
1> painted
END
COMPARE result.txt AGAINST expected.txt
6 changes: 6 additions & 0 deletions test/terminal/color_never.clitest
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
RUN --color never STDIN /dev/null IN . INTO result.txt EXPECTING 0
WRITE expected.txt UNTIL END
1> streamed
1> painted
END
COMPARE result.txt AGAINST expected.txt
69 changes: 69 additions & 0 deletions test/terminal/terminal_child_main.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
#include <sourcemeta/core/options.h>
#include <sourcemeta/core/terminal.h>

#include <cassert>
#include <cstdlib>
#include <iostream>
#include <string_view>

#if defined(_WIN32)
#include <fcntl.h> // _O_BINARY
#include <io.h> // _setmode, _fileno
#endif

namespace {

auto run_mode(sourcemeta::core::TerminalColorPolicy policy) -> int {
sourcemeta::core::terminal_set_color_policy(
sourcemeta::core::TerminalStream::Stdout, policy);
assert(!sourcemeta::core::terminal_is_interactive(
sourcemeta::core::TerminalStream::Stdout));
sourcemeta::core::terminal_paint(
std::cout, sourcemeta::core::TerminalStream::Stdout, "streamed",
sourcemeta::core::TerminalStyle::Red);
std::cout << "\n"
<< sourcemeta::core::terminal_paint(
sourcemeta::core::TerminalStream::Stdout, "painted",
sourcemeta::core::TerminalStyle::Bold |
sourcemeta::core::TerminalStyle::Green)
<< "\n"
<< std::flush;
return EXIT_SUCCESS;
}

} // namespace

auto main(int argc, char **argv) -> int {
#if defined(_WIN32)
// Ensure standard output is binary on Windows so newlines are not converted
// to CRLF (\r\n), which would alter captured byte stream comparisons.
_setmode(_fileno(stdout), _O_BINARY);

@jviotti jviotti Sep 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this needed btw? Might be worth a comment explaining it?

#endif

sourcemeta::core::Options application;
application.option("color", {});
application.parse(argc, argv);

auto policy{sourcemeta::core::TerminalColorPolicy::WhenInteractive};
if (application.contains("color")) {
const auto &values{application.at("color")};
if (values.empty()) {

@cubic-dev-ai cubic-dev-ai Bot Sep 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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>

std::cerr << "missing value for --color\n";
return EXIT_FAILURE;
}

const auto value{values.front()};
if (value == "always") {
policy = sourcemeta::core::TerminalColorPolicy::Always;
} else if (value == "auto" || value == "when-interactive") {
policy = sourcemeta::core::TerminalColorPolicy::WhenInteractive;
} else if (value == "never" || value == "disabled") {
policy = sourcemeta::core::TerminalColorPolicy::Disabled;
} else {
std::cerr << "unknown --color option: " << value << "\n";
return EXIT_FAILURE;
}
}

return run_mode(policy);
}
39 changes: 38 additions & 1 deletion test/terminal/terminal_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,8 @@ TEST(policy_default_argument) {
}

TEST(policy_lifecycle_and_isolation) {
sourcemeta::core::terminal_reset_color_policy();

// Global configuration affects all streams
sourcemeta::core::terminal_set_color_policy(
sourcemeta::core::TerminalColorPolicy::WhenInteractive);
Expand Down Expand Up @@ -251,11 +253,46 @@ TEST(policy_lifecycle_and_isolation) {
EXPECT_EQ(stderr_stream.str(), "error_stream");

// Restore default policy across all streams
sourcemeta::core::terminal_reset_color_policy();
EXPECT_EQ(sourcemeta::core::terminal_color_policy(
sourcemeta::core::TerminalStream::Stderr),
sourcemeta::core::TerminalColorPolicy::WhenInteractive);
}

TEST(policy_reset_global) {
sourcemeta::core::terminal_set_color_policy(
sourcemeta::core::TerminalColorPolicy::WhenInteractive);
sourcemeta::core::TerminalColorPolicy::Always);
EXPECT_EQ(sourcemeta::core::terminal_color_policy(
sourcemeta::core::TerminalStream::Stdout),
sourcemeta::core::TerminalColorPolicy::Always);
sourcemeta::core::terminal_reset_color_policy();
EXPECT_EQ(sourcemeta::core::terminal_color_policy(
sourcemeta::core::TerminalStream::Stdout),
sourcemeta::core::TerminalColorPolicy::WhenInteractive);
EXPECT_EQ(sourcemeta::core::terminal_color_policy(
sourcemeta::core::TerminalStream::Stderr),
sourcemeta::core::TerminalColorPolicy::WhenInteractive);
EXPECT_EQ(sourcemeta::core::terminal_color_policy(
sourcemeta::core::TerminalStream::Stdin),
sourcemeta::core::TerminalColorPolicy::WhenInteractive);
}

TEST(policy_reset_per_stream) {
sourcemeta::core::terminal_set_color_policy(

@cubic-dev-ai cubic-dev-ai Bot Sep 13, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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>

sourcemeta::core::TerminalStream::Stderr,
sourcemeta::core::TerminalColorPolicy::Disabled);
sourcemeta::core::terminal_set_color_policy(
sourcemeta::core::TerminalStream::Stdout,
sourcemeta::core::TerminalColorPolicy::Always);
sourcemeta::core::terminal_reset_color_policy(
sourcemeta::core::TerminalStream::Stderr);
EXPECT_EQ(sourcemeta::core::terminal_color_policy(
sourcemeta::core::TerminalStream::Stderr),
sourcemeta::core::TerminalColorPolicy::WhenInteractive);
EXPECT_EQ(sourcemeta::core::terminal_color_policy(
sourcemeta::core::TerminalStream::Stdout),
sourcemeta::core::TerminalColorPolicy::Always);
sourcemeta::core::terminal_reset_color_policy();
}

TEST(stream_detection_runs_safely) {
Expand Down