From 32e7afe3ce698fe18b63eaf2d4bb9f1faae1225b Mon Sep 17 00:00:00 2001 From: Drew Cain Date: Thu, 11 Jun 2026 12:28:36 -0500 Subject: [PATCH] fix(cli): escape user-sourced values in Rich output and fix frontmatter flag gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bug 1 — Rich markup injection: user-sourced titles, permalinks, snippets, observation categories/content, and frontmatter keys/values were interpolated directly into Rich markup strings, causing bracketed text (e.g. "[draft]", "[fact]") to be silently swallowed or restyled. Apply markup_escape() at every injection point in _display_search_results, _display_read_note, _display_build_context, and _display_recent_activity. Observation labels use markup_escape on the full "[category] content" fragment so the literal brackets around the category are also escaped. Bug 2 — frontmatter panel ignores flag: _display_read_note rendered the frontmatter panel whenever result["frontmatter"] was non-empty, but the JSON payload always carries that key regardless of --include-frontmatter. Thread the boolean flag as a keyword argument and gate the panel on both the flag and non-empty content. Bug 3 — "0 result(s)" subtitle: the search API returns total=0 even when results is non-empty. result.get("total", len(results)) never triggered its default because the key exists; fall back to len(results) when total is falsy but results is non-empty, keeping page-count math consistent. Tests: add cases for bracketed title surviving search output, "[fact]" category surviving build-context tree, read-note without --include-frontmatter asserting no frontmatter panel, and search with total=0 asserting correct subtitle count. Co-Authored-By: Claude Signed-off-by: Drew Cain --- src/basic_memory/cli/commands/tool.py | 86 +++++++++--- tests/cli/test_cli_tool_rich_output.py | 179 +++++++++++++++++++++++++ 2 files changed, 244 insertions(+), 21 deletions(-) diff --git a/src/basic_memory/cli/commands/tool.py b/src/basic_memory/cli/commands/tool.py index e6643b15..83a24eef 100644 --- a/src/basic_memory/cli/commands/tool.py +++ b/src/basic_memory/cli/commands/tool.py @@ -15,6 +15,7 @@ import typer from loguru import logger from rich.console import Console from rich.markdown import Markdown +from rich.markup import escape as markup_escape from rich.panel import Panel from rich.table import Table from rich.text import Text @@ -78,12 +79,23 @@ def _display_search_results(result: dict[str, Any], query: str = "") -> None: has_more: bool """ results = result.get("results", []) - total = result.get("total", len(results)) + # Trigger: API returns total=0 even when results is non-empty (upstream quirk). + # Why: the key exists with value 0, so result.get("total", len(results)) never + # applies its default, leaving the subtitle as "0 result(s)" under a + # populated table. + # Outcome: fall back to len(results) when total is falsy but results is non-empty. + raw_total = result.get("total", len(results)) + total = raw_total if raw_total else len(results) # Real key is "current_page"; fall back to "page" for forward-compat. page = result.get("current_page") or result.get("page", 1) page_size = result.get("page_size", len(results)) or 1 - title = f"Search: [bold cyan]{query}[/bold cyan]" if query else "Search results" + # Trigger: query is user-supplied text that may contain Rich markup characters. + # Why: interpolating it directly into a markup string causes brackets to be + # parsed as style tags, swallowing or restyling bracketed content. + # Outcome: escape the query so its literal characters are always displayed. + escaped_query = markup_escape(query) if query else query + title = f"Search: [bold cyan]{escaped_query}[/bold cyan]" if query else "Search results" subtitle = f"{total} result(s) • page {page} of {max(1, -(-total // page_size))}" if not results: @@ -99,26 +111,31 @@ def _display_search_results(result: dict[str, Any], query: str = "") -> None: for item in results: item_type = item.get("type", "") - item_title = item.get("title") or item.get("permalink", "") - permalink = item.get("permalink", "") + # Trigger: user-sourced title/permalink may contain bracketed text. + # Why: Rich table cells with a style column interpret markup in cell values, + # swallowing brackets (e.g. "Spec [draft] v2" → "Spec v2"). + # Outcome: escape every user-sourced cell value before adding to the table. + item_title = markup_escape(item.get("title") or item.get("permalink", "")) + permalink = markup_escape(item.get("permalink", "")) score = item.get("score") score_str = f"{score:.2f}" if score is not None else "" # Prefer matched_chunk as the most relevant snippet; fall back to content. raw_snippet = item.get("matched_chunk") or item.get("content") or "" # Truncate to ~200 chars so the table stays readable. - snippet = raw_snippet[:200].replace("\n", " ") if raw_snippet else "" + snippet = markup_escape(raw_snippet[:200].replace("\n", " ")) if raw_snippet else "" table.add_row(item_type, item_title, score_str, permalink, snippet) console.print(Panel(table, title=title, subtitle=subtitle, expand=False)) -def _display_read_note(result: dict[str, Any]) -> None: +def _display_read_note(result: dict[str, Any], *, include_frontmatter: bool = False) -> None: """Render read-note result: header panel + optional frontmatter + rendered Markdown content.""" title = result.get("title", "") permalink = result.get("permalink", "") content = result.get("content", "") frontmatter: dict[str, Any] = result.get("frontmatter") or {} + # The header already uses Text.append so title is never markup-interpreted. header = Text() header.append(title, style="bold cyan") if permalink: @@ -127,15 +144,20 @@ def _display_read_note(result: dict[str, Any]) -> None: console.print(Panel(header, expand=False)) # Trigger: --include-frontmatter was passed; the MCP tool populates "frontmatter". - # Why: without rendering it here, the explicitly requested metadata is silently - # dropped in the Rich path — users must also know to add --json to see it. - # Outcome: print a dim key/value block above the content when frontmatter is present. - if frontmatter: + # Why: the JSON payload always carries a "frontmatter" key regardless of the flag, + # so checking non-empty alone would render it even without the flag. The flag + # must be threaded in to gate the panel. + # Outcome: print a dim key/value block above the content only when the flag is set. + if include_frontmatter and frontmatter: fm_table = Table(show_header=False, box=None, padding=(0, 1), expand=False) fm_table.add_column("key", style="dim") fm_table.add_column("value", style="dim") for key, value in frontmatter.items(): - fm_table.add_row(str(key), str(value)) + # Trigger: frontmatter keys/values are user-sourced and may contain markup. + # Why: Rich table cells with a style column parse markup, so bracketed + # keys or values would be silently consumed or restyled. + # Outcome: escape both key and value before adding them to the table. + fm_table.add_row(markup_escape(str(key)), markup_escape(str(value))) console.print(Panel(fm_table, title="[dim]frontmatter[/dim]", expand=False)) if content: @@ -162,7 +184,11 @@ def _display_build_context(result: dict[str, Any]) -> None: uri = metadata.get("uri", "") context_items: list[dict[str, Any]] = list(result.get("results", [])) - label = f"[bold cyan]{uri}[/bold cyan]" if uri else "Context" + # Trigger: uri is user-sourced and may contain Rich markup characters. + # Why: interpolating it directly into a markup string causes brackets to be + # parsed as style tags, swallowing or restyling bracketed content. + # Outcome: escape the uri so its literal characters are always displayed. + label = f"[bold cyan]{markup_escape(uri)}[/bold cyan]" if uri else "Context" tree = Tree(f"[bold]Context:[/bold] {label}") if not context_items: @@ -171,8 +197,13 @@ def _display_build_context(result: dict[str, Any]) -> None: for context_result in context_items: # --- Primary result node --- primary = context_result.get("primary_result", {}) - p_title = primary.get("title") or primary.get("permalink", "") - p_type = primary.get("type", "") + # Trigger: p_title and p_type are user-sourced values from the knowledge graph. + # Why: embedding them in markup strings without escaping would cause any + # bracketed text (e.g. an entity titled "Spec [draft]") to be consumed + # by the Rich markup parser and silently dropped from output. + # Outcome: escape all user values before interpolating into markup strings. + p_title = markup_escape(primary.get("title") or primary.get("permalink", "")) + p_type = markup_escape(primary.get("type", "")) primary_label = f"[cyan]{p_title}[/cyan]" if p_type: primary_label = f"[dim]{p_type}[/dim] {primary_label}" @@ -193,15 +224,23 @@ def _display_build_context(result: dict[str, Any]) -> None: # Truncate long observations so the tree stays readable. if len(obs_content) > 120: obs_content = obs_content[:117] + "..." - obs_label = f"[dim][{category}] {obs_content}[/dim]" + # Trigger: category and obs_content are user-sourced strings that may + # contain Rich markup characters. The category is also wrapped + # in literal "[" "]" brackets in the label, which must be + # escaped too so Rich does not treat "[fact]" as a style tag. + # Why: embedding "[fact]" in a markup string causes Rich to parse it as + # an unknown tag and silently drop the text. + # Outcome: escape the full "[category] content" fragment including the + # surrounding brackets before embedding it in a styled label. + obs_label = f"[dim]{markup_escape(f'[{category}] {obs_content}')}[/dim]" primary_node.add(obs_label) # --- Related items as children --- related: list[dict[str, Any]] = list(context_result.get("related_results", [])) for rel_item in related: - rel_title = rel_item.get("title") or rel_item.get("permalink", "") - rel_type = rel_item.get("type", "") - relation = rel_item.get("relation_type", "") + rel_title = markup_escape(rel_item.get("title") or rel_item.get("permalink", "")) + rel_type = markup_escape(rel_item.get("type", "")) + relation = markup_escape(rel_item.get("relation_type", "")) parts = [] if relation: @@ -234,8 +273,13 @@ def _display_recent_activity(result: list[dict[str, Any]]) -> None: for item in result: item_type = item.get("type", "") - item_title = item.get("title") or item.get("permalink", "") - permalink = item.get("permalink", "") + # Trigger: title, permalink, and timestamps are user-sourced strings from the + # knowledge graph and may contain Rich markup characters. + # Why: Rich table cells with a style column parse markup in cell values, so + # bracketed content would be silently consumed or restyled. + # Outcome: escape all user-sourced cell values before adding to the table. + item_title = markup_escape(item.get("title") or item.get("permalink", "")) + permalink = markup_escape(item.get("permalink", "")) updated = str(item.get("updated_at") or item.get("created_at") or "") table.add_row(item_type, item_title, permalink, updated) @@ -447,7 +491,7 @@ def read_note( if json_output or not _use_rich() or isinstance(result, str): _print_json(result) else: - _display_read_note(result) + _display_read_note(result, include_frontmatter=include_frontmatter) except ValueError as e: typer.echo(f"Error: {e}", err=True) raise typer.Exit(1) diff --git a/tests/cli/test_cli_tool_rich_output.py b/tests/cli/test_cli_tool_rich_output.py index 9a7963ce..cb3fea8e 100644 --- a/tests/cli/test_cli_tool_rich_output.py +++ b/tests/cli/test_cli_tool_rich_output.py @@ -438,6 +438,28 @@ def test_read_note_rich_include_frontmatter(mock_mcp): assert "hello world" in result.output +@patch( + "basic_memory.cli.commands.tool.mcp_read_note", + new_callable=AsyncMock, + return_value=READ_NOTE_RESULT, +) +def test_read_note_rich_no_frontmatter_without_flag(mock_mcp): + """read-note WITHOUT --include-frontmatter must not render the frontmatter panel. + + Regression (Bug 2): the JSON payload always contains a non-empty "frontmatter" + key, so the previous `if frontmatter:` guard rendered it even without the flag. + The flag must be threaded into _display_read_note to gate the panel. + """ + result = _tty_runner(["tool", "read-note", "test-note"]) + + assert result.exit_code == 0, f"CLI failed: {result.output}" + # The note title and content must still appear + assert "Test Note" in result.output + assert "hello world" in result.output + # The frontmatter panel must NOT appear + assert "frontmatter" not in result.output + + # --------------------------------------------------------------------------- # build-context – observations rendering (issue #678) # --------------------------------------------------------------------------- @@ -463,3 +485,160 @@ def test_build_context_rich_renders_observations(mock_mcp): assert "key fact" in result.output # The subtitle should include an observations count assert "observations" in result.output + + +# --------------------------------------------------------------------------- +# Rich markup injection – bracketed user text must survive (Bug 1, issue #678) +# --------------------------------------------------------------------------- + +# Search result whose title contains a bracket expression like "[draft]". +SEARCH_RESULT_BRACKETED_TITLE = { + "total": 1, + "current_page": 1, + "page_size": 10, + "has_more": False, + "results": [ + { + "type": "entity", + "title": "Spec [draft] v2", + "permalink": "specs/spec-draft-v2", + "file_path": "specs/Spec [draft] v2.md", + "score": 0.90, + "matched_chunk": "An important [red] section", + "content": None, + }, + ], +} + +# build-context payload where the observation category is "fact" — previously +# `[fact]` in the obs_label markup was interpreted as an unknown Rich tag and +# the text was swallowed. +BUILD_CONTEXT_BRACKETED_OBS = { + "results": [ + { + "primary_result": { + "type": "entity", + "external_id": "xyz", + "title": "Joanna", + "permalink": "people/joanna", + "file_path": "people/Joanna.md", + "created_at": "2025-01-01T00:00:00", + }, + "observations": [ + { + "type": "observation", + "category": "fact", + "content": "Joanna lives in Austin", + "permalink": "people/joanna", + "file_path": "people/Joanna.md", + "created_at": "2025-01-01T00:00:00", + } + ], + "related_results": [], + } + ], + "metadata": {"uri": "people/joanna", "depth": 1}, + "page": 1, + "page_size": 10, + "has_more": False, +} + + +@patch( + "basic_memory.cli.commands.tool.mcp_search", + new_callable=AsyncMock, + return_value=SEARCH_RESULT_BRACKETED_TITLE, +) +def test_search_notes_rich_title_with_brackets_survives(mock_mcp): + """Bracketed text in a search result title must appear literally in Rich output. + + Regression (Bug 1): user-sourced titles were interpolated directly into Rich + markup strings, so "[draft]" was treated as an unknown style tag and stripped. + After escaping, the literal text "[draft]" must be present in the output. + """ + result = _tty_runner(["tool", "search-notes", "spec"]) + + assert result.exit_code == 0, f"CLI failed: {result.output}" + # The full title including the bracket expression must survive + assert "[draft]" in result.output + # The snippet "[red]" should also survive (not restyle the output) + assert "[red]" in result.output + + +@patch( + "basic_memory.cli.commands.tool.mcp_build_context", + new_callable=AsyncMock, + return_value=BUILD_CONTEXT_BRACKETED_OBS, +) +def test_build_context_rich_observation_category_bracket_survives(mock_mcp): + """Observation category "[fact]" must appear literally in build-context Rich tree. + + Regression (Bug 1): the obs_label was built as f"[dim][{category}] content[/dim]", + which caused the inner "[fact]" to be parsed as an unknown Rich tag and dropped, + rendering "Joanna lives in Austin" without the category prefix. + After escaping the category, "[fact]" must be present in the tree output. + """ + result = _tty_runner(["tool", "build-context", "memory://people/joanna"]) + + assert result.exit_code == 0, f"CLI failed: {result.output}" + # The observation category prefix must appear literally + assert "[fact]" in result.output + # The observation content must also appear + assert "Joanna lives in Austin" in result.output + + +# --------------------------------------------------------------------------- +# search-notes – total=0 with non-empty results subtitle (Bug 3, issue #678) +# --------------------------------------------------------------------------- + +# Fixture that mirrors the upstream quirk: total=0 but results is non-empty. +SEARCH_RESULT_ZERO_TOTAL = { + "total": 0, + "current_page": 1, + "page_size": 10, + "has_more": False, + "results": [ + { + "type": "entity", + "title": "Found Note", + "permalink": "notes/found-note", + "file_path": "notes/Found Note.md", + "score": 0.80, + "matched_chunk": "some content", + "content": None, + }, + { + "type": "entity", + "title": "Another Found", + "permalink": "notes/another-found", + "file_path": "notes/Another Found.md", + "score": 0.70, + "matched_chunk": "more content", + "content": None, + }, + ], +} + + +@patch( + "basic_memory.cli.commands.tool.mcp_search", + new_callable=AsyncMock, + return_value=SEARCH_RESULT_ZERO_TOTAL, +) +def test_search_notes_rich_zero_total_falls_back_to_result_count(mock_mcp): + """When the API returns total=0 but results is non-empty, subtitle shows real count. + + Regression (Bug 3): result.get("total", len(results)) never triggered its + default because the "total" key exists (with value 0), so the subtitle read + "0 result(s)" under a table showing rows. The fix detects a falsy total with + non-empty results and falls back to len(results). + """ + result = _tty_runner(["tool", "search-notes", "found"]) + + assert result.exit_code == 0, f"CLI failed: {result.output}" + # Both result rows must appear + assert "Found Note" in result.output + assert "Another Found" in result.output + # The subtitle must show the real count (2), not 0 + assert "2 result(s)" in result.output + assert "0 result(s)" not in result.output