Skip to content

fix: detect color support for xterm-* TERM values (xterm-color, xterm-kitty)#318

Open
Sanjays2402 wants to merge 1 commit into
wolph:developfrom
Sanjays2402:fix/xterm-color-detection
Open

fix: detect color support for xterm-* TERM values (xterm-color, xterm-kitty)#318
Sanjays2402 wants to merge 1 commit into
wolph:developfrom
Sanjays2402:fix/xterm-color-detection

Conversation

@Sanjays2402

Copy link
Copy Markdown

What

ColorSupport.from_env() only recognised the bare TERM=xterm value. Any decorated variant — xterm-color, xterm-16color, xterm-kitty, xterm-ghostty, … — fell through to ColorSupport.NONE and the progress bar rendered without any color at all.

Root cause

The xterm branch used an exact-equality check while its sibling 256 branch uses containment:

elif '256' in value:
    support = max(cls.XTERM_256, support)
elif value == 'xterm':          # <-- exact match, misses xterm-color, xterm-kitty, ...
    support = max(cls.XTERM, support)

This is inconsistent with three things in the same codebase:

  1. The method's own docstring: "If they contain xterm, we will enable 16 color support." (containment, not equality)
  2. The sibling branch one line up already uses '256' in value.
  3. is_ansi_terminal(), whose ANSI_TERM_RE = ^([xe]|bv)term… matches xterm as a prefix. So the two public entry points disagreed:
TERM is_ansi_terminal() from_env() before from_env() after
xterm True XTERM XTERM
xterm-color True NONE XTERM
xterm-16color True NONE XTERM
xterm-kitty True NONE XTERM
xterm-ghostty True NONE XTERM
xterm-256color True XTERM_256 XTERM_256

from_env() gates all color output — Color.ansi (in terminal/base.py) returns None when COLOR_SUPPORT is NONE — so this silently disabled color for a large class of extremely common real-world terminals.

Fix

One-line change: value == 'xterm''xterm' in value. The 256 branch is checked first, so xterm-256color still resolves to XTERM_256; only values that would otherwise have been NONE are promoted to XTERM (16-color), matching the documented contract.

Test

Added test_color_support_from_env_term, which asserts the resolved ColorSupport for bare xterm, the previously-broken xterm-* variants, and the 256 cases (so the ordering guarantee is locked in too).

Proof it guards the bug (stash the source fix, run the test):

# without the fix:
FAILED tests/test_color.py::test_color_support_from_env_term[xterm-color-16]
FAILED tests/test_color.py::test_color_support_from_env_term[xterm-16color-16]
FAILED tests/test_color.py::test_color_support_from_env_term[xterm-kitty-16]
FAILED tests/test_color.py::test_color_support_from_env_term[xterm-ghostty-16]
4 failed, 3 passed
AssertionError: assert <ColorSupport.NONE: 0> == <ColorSupport.XTERM: 16>

# with the fix:
7 passed

Full tests/test_color.py + tests/test_utils.py = 100 passed (both orders), broader suite 463 passed, 12 skipped — no regressions. ruff check and ruff format --check clean on both changed files. +27 / -1.

…-kitty)

`ColorSupport.from_env()` matched the `xterm` TERM value with an exact
equality check (`value == 'xterm'`), so any decorated variant such as
`xterm-color`, `xterm-16color`, `xterm-kitty` or `xterm-ghostty` fell
through to `NONE` and rendered without any color at all.

This contradicted three things:

- the method's own docstring, which states "If they contain `xterm`, we
  will enable 16 color support";
- the sibling branch immediately above, which already uses containment
  (`'256' in value`); and
- `is_ansi_terminal()`, whose `ANSI_TERM_RE` matches `^xterm` as a
  prefix, so `is_ansi_terminal('xterm-color')` was True while
  `from_env()` reported no color support for the same terminal.

Switch the check to `'xterm' in value` so every `xterm-*` variant gets
at least 16-color support. The `256` branch runs first, so
`xterm-256color` still resolves to `XTERM_256`.

Add `test_color_support_from_env_term` asserting the mapping for bare
`xterm`, the previously-broken `xterm-*` variants, and the `256` cases.
It fails on the four variant cases before the fix and passes after.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request updates the color support detection in progressbar/env.py to check if 'xterm' is contained within the environment variable value instead of performing an exact match, and adds corresponding unit tests in tests/test_color.py. The review feedback suggests making the 'xterm' check case-insensitive by using value.lower() and adding an uppercase test case to verify this behavior.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread progressbar/env.py
elif '256' in value:
support = max(cls.XTERM_256, support)
elif value == 'xterm':
elif 'xterm' in value:

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The environment variable check for 'xterm' is case-sensitive. Since terminal environment variables (like TERM or COLORTERM) can sometimes be set in uppercase or mixed case (e.g., XTERM, Xterm), it is safer to perform a case-insensitive check by lowercasing the value before checking.

Suggested change
elif 'xterm' in value:
elif 'xterm' in value.lower():

Comment thread tests/test_color.py
Comment on lines +106 to +110
('xterm', env.ColorSupport.XTERM),
('xterm-color', env.ColorSupport.XTERM),
('xterm-16color', env.ColorSupport.XTERM),
('xterm-kitty', env.ColorSupport.XTERM),
('xterm-ghostty', env.ColorSupport.XTERM),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

To ensure that case-insensitive terminal names (like XTERM) are correctly supported, we should add a test case with uppercase characters.

Suggested change
('xterm', env.ColorSupport.XTERM),
('xterm-color', env.ColorSupport.XTERM),
('xterm-16color', env.ColorSupport.XTERM),
('xterm-kitty', env.ColorSupport.XTERM),
('xterm-ghostty', env.ColorSupport.XTERM),
('xterm', env.ColorSupport.XTERM),
('XTERM', env.ColorSupport.XTERM),
('xterm-color', env.ColorSupport.XTERM),
('xterm-16color', env.ColorSupport.XTERM),
('xterm-kitty', env.ColorSupport.XTERM),
('xterm-ghostty', env.ColorSupport.XTERM),

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant