admin/basicmachines-co-basic-memory
1
0
Fork 0
mirror of https://github.com/basicmachines-co/basic-memory synced 2026-06-21 13:47:35 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(cli): escape user-sourced values in Rich output and fix frontmatter flag gate

Browse Source 
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 <noreply@anthropic.com>
Signed-off-by: Drew Cain <groksrc@gmail.com>
This commit is contained in:
Drew Cain
2026-06-11 12:28:36 -05:00
parent e079eca42b
commit 32e7afe3ce
2 changed files with 244 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/basic_memory/cli/commands/tool.py
+65 -21
View File
@@ -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)
tests/cli/test_cli_tool_rich_output.py
+179
View File
@@ -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