Skip to content

fix: only emit ANSI color when stdout is a TTY - #551

Merged
shenxianpeng merged 1 commit into
commit-check:mainfrom
larsch:bugfix/tty-aware-color-output
Aug 16, 2026
Merged

fix: only emit ANSI color when stdout is a TTY#551
shenxianpeng merged 1 commit into
commit-check:mainfrom
larsch:bugfix/tty-aware-color-output

Conversation

@larsch

@larsch larsch commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Problem

The CLI emitted raw ANSI color escapes unconditionally. RED, GREEN,
YELLOW, and RESET_COLOR were hardcoded escape sequences in
commit_check/__init__.py, and print_error_message / print_suggestion
wrapped output in them without checking whether stdout is a terminal. Piped or
redirected output (CI logs, agent harnesses, files) therefore contained escape
garbage.

Fix

Add a supports_color() probe that mirrors the existing
supports_hyperlinks() convention, and gate the constants themselves:

RED = "\033[91m" if supports_color() else ""
  • FORCE_COLOR overrides in both directions (0 off, anything else on),
    matching the FORCE_HYPERLINK convention already documented in
    supports_hyperlinks().
  • A non-TTY stdout disables color.
  • TERM=dumb and an empty TERM (TERM=) disable color — an empty value
    is the same deliberate "no terminal type" signal as dumb, while an unset
    TERM still allows color on a real terminal.
  • No new dependencies.

Moving the decision to the constant definitions means commit_check/util.py
is unchanged: the print functions keep interpolating the constants, which are
already empty when color is off.

Tests

  • supports_color() behavior across FORCE_COLOR, TTY, and TERM cases.
  • Constant values (empty vs. raw escape codes) verified via module reload.

Summary by CodeRabbit

  • New Features

    • Added automatic detection of terminal color support.
    • Honors explicit color settings, terminal environment values, and whether output is connected to an interactive terminal.
    • ANSI colors are now enabled only when supported, improving output in redirected or non-color environments.
  • Tests

    • Added coverage for forced color, piped output, terminal settings, and color configuration behavior.

@larsch
larsch requested a review from a team as a code owner August 16, 2026 10:31
@larsch
larsch requested review from shenxianpeng and removed request for a team August 16, 2026 10:31
@github-actions github-actions Bot added the bug Something isn't working label Aug 16, 2026
@coderabbitai

coderabbitai Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f8690345-28e5-4992-95b2-ae41cebf46d5

📥 Commits

Reviewing files that changed from the base of the PR and between 41df972 and 616bdaf.

📒 Files selected for processing (2)
  • commit_check/__init__.py
  • tests/util_test.py

Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The package adds supports_color() and uses it to enable or disable ANSI color constants. Tests cover TTY status, TERM, FORCE_COLOR, and module reload behavior.

Changes

Terminal color support

Layer / File(s) Summary
Detect and validate color support
commit_check/__init__.py, tests/util_test.py
supports_color() evaluates TTY status, TERM, and FORCE_COLOR. ANSI constants become empty when color is unsupported. Tests cover detection and reloaded constants.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 616bd

This localized CLI change gates ANSI color for non-TTY output, and no concrete user or production risk remains at the current head; it is merge-ready after normal checks.

Suggested reviewers: shenxianpeng

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: limiting ANSI color output to TTY-supported stdout.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sonarqubecloud

Copy link
Copy Markdown

@codecov

codecov Bot commented Aug 16, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.14%. Comparing base (41df972) to head (616bdaf).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #551      +/-   ##
==========================================
+ Coverage   98.13%   98.14%   +0.01%     
==========================================
  Files          12       12              
  Lines        1337     1348      +11     
==========================================
+ Hits         1312     1323      +11     
  Misses         25       25              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@shenxianpeng shenxianpeng left a comment

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.

Thank you for your PR!

@shenxianpeng
shenxianpeng merged commit b446cb8 into commit-check:main Aug 16, 2026
26 checks passed
@codspeed-hq

codspeed-hq Bot commented Aug 16, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 490 untouched benchmarks
🆕 9 new benchmarks
⏩ 121 skipped benchmarks1

Performance Changes

Benchmark BASE HEAD Efficiency
🆕 test_constants_empty_when_color_off N/A 12.1 ms N/A
🆕 test_constants_set_when_color_on N/A 11.6 ms N/A
🆕 test_dumb_term_turns_color_off N/A 2.9 ms N/A
🆕 test_empty_term_turns_color_off N/A 2.9 ms N/A
🆕 test_force_empty_falls_through_to_detection N/A 2.9 ms N/A
🆕 test_force_zero_turns_color_off N/A 2.8 ms N/A
🆕 test_forced_even_when_piped N/A 2.8 ms N/A
🆕 test_not_supported_when_piped N/A 2.8 ms N/A
🆕 test_unset_term_still_allows_color_on_a_tty N/A 2.9 ms N/A

Comparing larsch:bugfix/tty-aware-color-output (616bdaf) with main (60d7586)2

Open in CodSpeed

Footnotes

  1. 121 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

  2. No successful run was found on main (41df972) during the generation of this report, so 60d7586 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

shenxianpeng added a commit that referenced this pull request Aug 16, 2026
supports_color() from #551 answered FORCE_COLOR, the TTY and TERM, but
not NO_COLOR — the variable users actually export globally to opt out of
color (https://no-color.org). Any non-empty value now disables color,
outranking detection and yielding only to an explicit FORCE_COLOR.

Two gaps in the #551 tests are closed alongside. The reload-based tests
recomputed the module constants under a patched environment and left the
last reload's values in place for every test that ran afterwards; a
fixture now re-derives them on teardown. And nothing exercised the copies
commit_check.util binds at import — the ones the print functions actually
read — so two subprocess tests now run the real import path end to end
and assert on what gets printed.
shenxianpeng added a commit that referenced this pull request Aug 16, 2026
supports_color() from #551 answered FORCE_COLOR, the TTY and TERM, but
not NO_COLOR — the variable users actually export globally to opt out of
color (https://no-color.org). Any non-empty value now disables color,
outranking detection and yielding only to an explicit FORCE_COLOR.

Two gaps in the #551 tests are closed alongside. The reload-based tests
recomputed the module constants under a patched environment and left the
last reload's values in place for every test that ran afterwards; a
fixture now re-derives them on teardown. And nothing exercised the copies
commit_check.util binds at import — the ones the print functions actually
read — so two subprocess tests now run the real import path end to end
and assert on what gets printed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants