Skip to content

Commit b47c85b

Browse files
committed
fix: escape Rich markup with official helper
Signed-off-by: Nanook <nanookclaw@users.noreply.github.com>
1 parent 6b3838c commit b47c85b

2 files changed

Lines changed: 107 additions & 22 deletions

File tree

openhands_cli/tui/widgets/richlog_visualizer.py

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import threading
88
from typing import TYPE_CHECKING
99

10+
from rich.markup import escape as rich_markup_escape
1011
from rich.text import Text
1112
from textual.widgets import Markdown
1213

@@ -526,12 +527,13 @@ def _build_observation_content(
526527
def _escape_rich_markup(self, text: str) -> str:
527528
"""Escape Rich markup characters in text to prevent markup errors.
528529
529-
This is needed to handle content with special characters (e.g., Chinese text
530-
with brackets) that would otherwise cause MarkupError when rendered in
531-
Collapsible widgets with markup=True.
530+
Delegates to ``rich.markup.escape`` so pre-existing backslashes in the
531+
input (e.g. shell-quoted patterns like ``grep "#\\[cfg(test)\\]"``) are
532+
doubled correctly. A hand-rolled ``[`` -> ``\\[`` replacement would turn
533+
``\\[`` into ``\\\\[``, which Rich then interprets as a style tag and
534+
raises ``MissingStyle`` (see OpenHands-CLI#749).
532535
"""
533-
# Escape square brackets which are used for Rich markup
534-
return text.replace("[", r"\[").replace("]", r"\]")
536+
return rich_markup_escape(text)
535537

536538
def _truncate_for_display(
537539
self, text: str, max_length: int = MAX_LINE_LENGTH, *, from_start: bool = True

tests/tui/widgets/test_richlog_visualizer.py

Lines changed: 100 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -114,14 +114,19 @@ class TestChineseCharacterMarkupHandling:
114114
"""Tests for handling Chinese characters with special markup symbols."""
115115

116116
def test_escape_rich_markup_escapes_brackets(self, visualizer):
117-
"""Test that _escape_rich_markup properly escapes square brackets."""
117+
"""Test that _escape_rich_markup neutralises Rich markup safely.
118+
119+
Rich's official ``rich.markup.escape`` only needs to escape the opening
120+
``[`` of a markup tag (a stand-alone ``]`` cannot start a tag). These
121+
cases verify the output is round-trippable via ``Text.from_markup``.
122+
"""
118123
test_cases = [
119-
("[test]", r"\[test\]"),
120-
("处于历史40%分位]", r"处于历史40%分位\]"),
121-
("[cyan]colored[/cyan]", r"\[cyan\]colored\[/cyan\]"),
124+
("[test]", r"\[test]"),
125+
("处于历史40%分位]", "处于历史40%分位]"),
126+
("[cyan]colored[/cyan]", r"\[cyan]colored\[/cyan]"),
122127
(
123128
"+0.3%,月变化+0.8%,处于历史40%分位]",
124-
r"+0.3%,月变化+0.8%,处于历史40%分位\]",
129+
"+0.3%,月变化+0.8%,处于历史40%分位]",
125130
),
126131
]
127132

@@ -131,17 +136,21 @@ def test_escape_rich_markup_escapes_brackets(self, visualizer):
131136
f"Failed to escape '{input_text}': expected '{expected_output}', "
132137
f"got '{result}'"
133138
)
139+
# The escaped form must round-trip through markup parsing
140+
# without losing characters.
141+
assert Text.from_markup(result).plain == input_text
134142

135143
def test_safe_content_string_escapes_problematic_content(self, visualizer):
136144
"""Test that _escape_rich_markup escapes MarkupError content."""
137145
problematic_content = "+0.3%,月变化+0.8%,处于历史40%分位]"
138146
safe_content = visualizer._escape_rich_markup(str(problematic_content))
139147

140-
# Verify brackets are escaped
141-
assert r"\]" in safe_content
142148
# Verify Chinese characters are preserved
143149
assert "月变化" in safe_content
144150
assert "处于历史" in safe_content
151+
# Stand-alone closing bracket is not markup so it does not need escaping
152+
# — what matters is that the content round-trips cleanly.
153+
assert Text.from_markup(safe_content).plain == problematic_content
145154

146155
def test_unescaped_content_with_close_tag_causes_markup_error(self):
147156
"""Verify that certain bracket patterns can cause MarkupError.
@@ -217,9 +226,9 @@ def test_various_chinese_patterns_are_escaped(self, visualizer, test_content):
217226
"""Test that various patterns of Chinese text with special chars are handled."""
218227
safe_content = visualizer._escape_rich_markup(str(test_content))
219228

220-
# Verify brackets are escaped
221-
assert "[" not in safe_content or r"\[" in safe_content
222-
assert "]" not in safe_content or r"\]" in safe_content
229+
# The escaped content must round-trip cleanly through markup parsing,
230+
# i.e. no styles get applied and no characters are dropped.
231+
assert Text.from_markup(safe_content).plain == test_content
223232

224233
# Should be able to create a Static widget without error
225234
widget = Static(safe_content, markup=True)
@@ -269,12 +278,15 @@ class TestVisualizerIntegration:
269278
def test_end_to_end_chinese_content_visualization(self, visualizer):
270279
"""End-to-end test: create event with Chinese content and visualize it.
271280
272-
Uses TerminalAction since the title includes the command for terminal actions.
281+
Uses TerminalAction since the title includes the command for terminal
282+
actions. The command contains a ``[red]`` lookalike token so that the
283+
escape logic must rewrite a bracket Rich would otherwise treat as a
284+
style tag.
273285
"""
274286
from openhands.tools.terminal.definition import TerminalAction
275287

276288
action = TerminalAction(
277-
command="分析结果: 增长率+0.3%,月变化+0.8%,处于历史40%分位]"
289+
command="echo '分析结果: 增长率+0.3% [red]处于历史40%分位[/red]'"
278290
)
279291
tool_call = create_tool_call("call_test", "terminal")
280292

@@ -287,16 +299,21 @@ def test_end_to_end_chinese_content_visualization(self, visualizer):
287299
llm_response_id="resp_test",
288300
)
289301

290-
# This entire flow should work without errors
291302
collapsible = visualizer._create_event_collapsible(event)
292303
assert collapsible is not None
293304
assert collapsible.title is not None
294305

295-
# The title should contain escaped content
296306
title_str = str(collapsible.title)
297-
# For terminal actions, the command is included in the title
298-
# Brackets in the command should be escaped
299-
assert r"\]" in title_str, f"Expected escaped bracket in title: {title_str}"
307+
# The "[red]" / "[/red]" patterns from the command would be parsed as
308+
# style tags by Rich, so the escape must have inserted '\[' before each.
309+
assert r"\[red]" in title_str, (
310+
f"Expected escaped opening bracket in title: {title_str}"
311+
)
312+
assert r"\[/red]" in title_str, (
313+
f"Expected escaped closing-tag bracket in title: {title_str}"
314+
)
315+
# The full title must parse without raising a MarkupError.
316+
Text.from_markup(title_str)
300317

301318
def test_visualizer_handles_mistral_xml_function_call_syntax(self, visualizer):
302319
"""Test that visualizer can handle ActionEvent with Mistral XML function call.
@@ -344,6 +361,72 @@ def test_visualizer_handles_mistral_xml_function_call_syntax(self, visualizer):
344361
assert len(title_str) > 0 # Title should not be empty
345362

346363

364+
class TestBackslashBracketEscaping:
365+
"""Regression tests for OpenHands/OpenHands-CLI#749.
366+
367+
The hand-rolled ``str.replace("[", r"\\[")`` escape doubled any backslash
368+
already in the command (``\\[`` -> ``\\\\[``). Rich then interpreted
369+
``\\\\[name]`` as a style tag and raised ``MissingStyle: unable to parse
370+
'name\\\\' as color``. ``rich.markup.escape`` doubles existing backslashes
371+
correctly so the round-trip is safe.
372+
"""
373+
374+
def test_escape_preserves_backslash_bracket_sequences(self, visualizer):
375+
"""``rich.markup.escape`` round-trips ``\\[`` through markup parsing."""
376+
# Command as the user typed it in the shell: grep "#\[cfg(test)\]"
377+
command = 'grep "#\\[cfg(test)\\]"'
378+
379+
escaped = visualizer._escape_rich_markup(command)
380+
381+
# Round-trip: parsing the escaped form must give back the original.
382+
assert Text.from_markup(escaped).plain == command
383+
384+
def test_terminal_action_title_with_backslash_bracket_command(self, visualizer):
385+
"""Reproduces the exact title from #749 — must not raise MissingStyle.
386+
387+
With the hand-rolled escape, the title was
388+
``[dim]: $ grep -c "#\\\\[test\\\\]" ...[/dim]`` which Rich parsed as a
389+
``test\\`` style tag and raised ``StyleSyntaxError``/``MissingStyle``.
390+
"""
391+
# The literal command from the bug report.
392+
command = (
393+
'grep -c "#\\[test\\]" '
394+
"/Users/tomasic/proj/hypatia/crates/hypatia-parser/src/parser.rs"
395+
)
396+
event = create_terminal_action_event(command, summary="Counting tests")
397+
398+
title = visualizer._build_action_title(event)
399+
400+
# Must parse without raising — this is what was failing before the fix.
401+
parsed = Text.from_markup(title)
402+
403+
# And the escaped bracketed pattern must survive round-trip
404+
# rather than being silently truncated at the first stray bracket.
405+
assert "#\\[test\\]" in parsed.plain, (
406+
f"Backslash-escaped brackets were dropped from title: {parsed.plain!r}"
407+
)
408+
409+
def test_terminal_action_title_with_backslash_bracket_no_style_error(
410+
self, visualizer
411+
):
412+
"""Parse the title via Console rendering, which is where MissingStyle
413+
actually surfaces (Text.from_markup is lenient about unknown styles)."""
414+
from io import StringIO
415+
416+
from rich.console import Console
417+
418+
command = 'grep -c "#\\[cfg(test)\\]" src/lib.rs'
419+
event = create_terminal_action_event(command, summary="Search")
420+
421+
title = visualizer._build_action_title(event)
422+
423+
# Rendering through a Console forces every span's style to be parsed,
424+
# which is where the original bug raised
425+
# ``StyleSyntaxError: unable to parse 'test\\\\' as color``.
426+
console = Console(file=StringIO(), force_terminal=False, color_system=None)
427+
console.print(title) # must not raise
428+
429+
347430
class TestConversationErrorEventHandling:
348431
"""Tests for ConversationErrorEvent handling in the visualizer."""
349432

0 commit comments

Comments
 (0)