-
Notifications
You must be signed in to change notification settings - Fork 806
fix(shell): prevent Rich default markdown styles from leaking background colors #1739
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,53 @@ | ||
| """Tests for NEUTRAL_MARKDOWN_THEME style overrides.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from kimi_cli.ui.shell.console import NEUTRAL_MARKDOWN_THEME | ||
|
|
||
|
|
||
| class TestNeutralMarkdownThemeNoBgColor: | ||
| """markdown.code and markdown.code_block must not inherit Rich's default | ||
| black background (``"cyan on black"`` / ``"bold cyan on black"``). | ||
|
|
||
| Rich's built-in default theme defines:: | ||
|
|
||
| "markdown.code": "bold cyan on black" | ||
| "markdown.code_block": "cyan on black" | ||
|
|
||
| Because NEUTRAL_MARKDOWN_THEME uses ``inherit=True``, any style key NOT | ||
| explicitly listed inherits the Rich default. If we forget to override | ||
| ``markdown.code`` and ``markdown.code_block``, inline code and fenced code | ||
| blocks will render with an opaque black background that looks wrong on | ||
| non-black terminals (the "black code block" bug, see issue #1681). | ||
| """ | ||
|
|
||
| def test_markdown_code_has_no_background(self) -> None: | ||
| style = NEUTRAL_MARKDOWN_THEME.styles.get("markdown.code") | ||
| assert style is not None, "markdown.code must be explicitly set in NEUTRAL_MARKDOWN_THEME" | ||
| assert style.bgcolor is None, ( | ||
| f"markdown.code should have no background color, got bgcolor={style.bgcolor}" | ||
| ) | ||
|
|
||
| def test_markdown_code_block_has_no_background(self) -> None: | ||
| style = NEUTRAL_MARKDOWN_THEME.styles.get("markdown.code_block") | ||
| assert style is not None, ( | ||
| "markdown.code_block must be explicitly set in NEUTRAL_MARKDOWN_THEME" | ||
| ) | ||
| assert style.bgcolor is None, ( | ||
| f"markdown.code_block should have no background color, got bgcolor={style.bgcolor}" | ||
| ) | ||
|
|
||
| def test_all_markdown_styles_have_no_background(self) -> None: | ||
| """No markdown.* style in NEUTRAL_MARKDOWN_THEME should carry a background color. | ||
|
|
||
| Rich's default theme may assign background colors to markdown styles | ||
| (e.g. ``"cyan on black"`` for code). Since NEUTRAL_MARKDOWN_THEME uses | ||
| ``inherit=True``, any key we forget to override will inherit the Rich | ||
| default. This test catches that for ALL markdown styles, not just the | ||
| ones we know about today. | ||
| """ | ||
| for name, style in NEUTRAL_MARKDOWN_THEME.styles.items(): | ||
| if name.startswith("markdown."): | ||
| assert style.bgcolor is None, ( | ||
| f"{name} should have no background color, got bgcolor={style.bgcolor}" | ||
| ) | ||
|
Comment on lines
+49
to
+53
|
||
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.
The newly added overrides
markdown.list,markdown.h7, andmarkdown.emphdon’t appear to be referenced anywhere else in the codebase (search only finds them in this theme). If they’re intended purely as a defensive override against Rich internals, it would help to add a brief comment explaining why these keys are needed here; otherwise consider dropping unused keys to keep the theme definition aligned with actual style names we render.