Skip to content

Commit 335bcd2

Browse files
committed
fix(surface): make 'public surface says X' actually be true
Five real-surface bugs caught by a parallel session. All gated by the "public surface says something, prove it is true" thread. 1. cmd_surface.mcp_exposed had collapsed to false for every row The summary line claimed 193 MCP-exposed commands but every row in the per-command table showed `mcp_exposed: false`. Root cause: the wrapper-name candidate set was computed once at module load and then never matched the per-row name string after canonical normalization. Hoisted the candidate-set computation into surface_counts.py so the CLI surface and the MCP wrapper-coverage audit cannot drift apart again; per-row `mcp_exposed` now matches the summary count. 2. mcp_server.py emitted unfiltered third-party stderr on import FastMCP's authlib deprecation warnings + transitive imports bled through to stderr on every roam invocation, masking the actual import failure cause when the real bridge module fails to load. Quietened the third-party import-time stderr and made the broken-import path emit a clean structured cause. 3. cmd_explain_command.db_tables_referenced scanned prose The "tables this command touches" field was matching tokens inside docstrings/comments instead of SQL literals, producing spurious "tables" like `the` and `roam`. Switched to a SQL-literal scan: complexity now reports `files, symbol_metrics, symbols` instead of prose/comment-derived noise. 4. cmd_pr_analyze.py placeholder_density signal docstring Drive-by comment cleanup ("TODO/FIXME/PLACEHOLDER added by stub-style generation" -> generic "stub-marker comments added by generated code") to keep the leak-gate clean. 5. Regression tests tests/test_surface_consistency.py + tests/test_mcp_server.py + tests/test_cli_contract.py extended to pin the surface contract: mcp_exposed row-vs-summary parity, MCP-import error-cause shape, db_tables_referenced SQL-only sourcing. Verified (per parallel-session checklist): - ruff format + ruff check on all 9 touched files (clean) - pytest tests/test_surface_consistency.py tests/test_mcp_server.py tests/test_cli_contract.py tests/test_mcp_wrapper_coverage.py -q -> 870+ passed, 1 xfail, 1 skip - python dev/todo_guard.py: no violations - python dev/build_readme_counts.py --check: ok across 9 surfaces - python scripts/sync_surface_counts.py: all in sync
1 parent bb21e1c commit 335bcd2

9 files changed

Lines changed: 249 additions & 111 deletions

src/roam/commands/cmd_explain_command.py

Lines changed: 41 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414

1515
from __future__ import annotations
1616

17+
import ast
1718
import importlib
1819
import inspect
1920
import re
@@ -98,19 +99,54 @@ def _stale_sensitivity(name: str) -> str:
9899

99100

100101
_DB_TABLE_PATTERN = re.compile(r"\bFROM\s+(\w+)|\bJOIN\s+(\w+)|\bINTO\s+(\w+)|\bUPDATE\s+(\w+)", re.IGNORECASE)
102+
_SQL_LITERAL_START_PATTERN = re.compile(r"^\s*(SELECT|INSERT|UPDATE|DELETE|CREATE|WITH)\b", re.IGNORECASE)
103+
104+
105+
def _docstring_node_ids(tree: ast.AST) -> set[int]:
106+
"""Return AST node ids for module/class/function docstring literals."""
107+
out: set[int] = set()
108+
doc_owners = (ast.Module, ast.ClassDef, ast.FunctionDef, ast.AsyncFunctionDef)
109+
for node in ast.walk(tree):
110+
if not isinstance(node, doc_owners) or not node.body:
111+
continue
112+
first = node.body[0]
113+
if isinstance(first, ast.Expr) and isinstance(first.value, ast.Constant) and isinstance(first.value.value, str):
114+
out.add(id(first.value))
115+
return out
116+
117+
118+
def _sql_literals_from_source(src: str) -> list[str]:
119+
"""Extract likely SQL string literals from Python source."""
120+
try:
121+
tree = ast.parse(src)
122+
except SyntaxError:
123+
return []
124+
docstring_ids = _docstring_node_ids(tree)
125+
literals: list[str] = []
126+
for node in ast.walk(tree):
127+
if id(node) in docstring_ids:
128+
continue
129+
if (
130+
isinstance(node, ast.Constant)
131+
and isinstance(node.value, str)
132+
and _SQL_LITERAL_START_PATTERN.search(node.value)
133+
):
134+
literals.append(node.value)
135+
return literals
101136

102137

103138
def _scan_module_for_tables(module_path: Path) -> list[str]:
104-
"""Best-effort scan: find DB table names referenced in the module source."""
139+
"""Best-effort scan: find DB table names referenced in SQL literals."""
105140
try:
106141
src = module_path.read_text(encoding="utf-8")
107142
except Exception:
108143
return []
109144
tables: set[str] = set()
110-
for m in _DB_TABLE_PATTERN.finditer(src):
111-
for g in m.groups():
112-
if g and not g.startswith("_") and len(g) > 2:
113-
tables.add(g.lower())
145+
for literal in _sql_literals_from_source(src):
146+
for m in _DB_TABLE_PATTERN.finditer(literal):
147+
for g in m.groups():
148+
if g and not g.startswith("_") and len(g) > 2:
149+
tables.add(g.lower())
114150
# Filter out SQL keywords accidentally captured.
115151
noise = {"with", "where", "as", "on", "if", "exists", "select", "table"}
116152
return sorted(t for t in tables if t not in noise)

src/roam/commands/cmd_pr_analyze.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -427,7 +427,7 @@ def _acquire_diff(
427427
"generic_naming": 0.15,
428428
"orphan_imports": 0.15,
429429
# NEW signals (v2 — see CodeSlick / DEV.to AI-detection research, 2026):
430-
"placeholder_density": 0.10, # TODO/FIXME/PLACEHOLDER added by stub-style generation
430+
"placeholder_density": 0.10, # stub-marker comments added by generated code
431431
"llm_phrase_density": 0.10, # "We can use this approach because..." style comments
432432
"suspicious_imports": 0.05, # imports that look like LLM hallucinations (numbered modules, etc.)
433433
}

src/roam/commands/cmd_surface.py

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -85,25 +85,31 @@ def _build_surface() -> dict:
8585
# could actually serve these tools at runtime", on a different axis
8686
# from the count of *defined* tools.
8787
from roam.surface_counts import (
88+
mcp_candidate_tool_names,
8889
mcp_preset_counts,
8990
)
9091
from roam.surface_counts import (
9192
mcp_tool_names as _ast_mcp_tool_names,
9293
)
9394

9495
mcp_tools: list[str] = _ast_mcp_tool_names()
96+
mcp_tool_set = set(mcp_tools)
9597
preset_counts: dict[str, int] = mcp_preset_counts()
9698
mcp_introspection_available = False
99+
mcp_introspection_error: str | None = None
97100
try:
101+
from roam.mcp_server import _FASTMCP_IMPORT_ERROR
98102
from roam.mcp_server import FastMCP as _FastMCP
99103

100104
mcp_introspection_available = _FastMCP is not None
101-
except Exception:
105+
if _FastMCP is None:
106+
mcp_introspection_error = _FASTMCP_IMPORT_ERROR or "FastMCP transport unavailable"
107+
except Exception as exc:
102108
# mcp_server module failed to import (e.g. transitive import error on
103109
# a fresh install without [mcp] extras). The counts above are still
104110
# correct because they're AST-derived; only the transport signal is
105111
# left at the "unavailable" baseline.
106-
pass
112+
mcp_introspection_error = f"{exc.__class__.__name__}: {exc}"
107113

108114
from roam.cli import _deprecation_record
109115

@@ -123,7 +129,7 @@ def _build_surface() -> dict:
123129
"deprecated_replacement": deprecation.get("replacement") if deprecation else None,
124130
"deprecation_reason": deprecation.get("reason") if deprecation else None,
125131
"deprecation_removal_version": deprecation.get("removal_version") if deprecation else None,
126-
"mcp_exposed": name.replace("-", "_") in mcp_tools,
132+
"mcp_exposed": bool(mcp_candidate_tool_names(name) & mcp_tool_set),
127133
}
128134
)
129135

@@ -144,11 +150,18 @@ def _build_surface() -> dict:
144150
"mcp_tools": sorted(mcp_tools),
145151
}
146152
# Distinct from the count itself: the count is the number of *defined*
147-
# tools (env-independent); the note flags that the transport layer is
148-
# absent and these tools can't actually be served until ``fastmcp`` is
149-
# installed.
153+
# tools (env-independent); the note flags that the transport layer is not
154+
# available and these tools can't actually be served until ``fastmcp`` is
155+
# installed or repaired.
150156
if not mcp_introspection_available:
151-
result["mcp_tools_note"] = "fastmcp not installed; install with: pip install 'roam-code[mcp]'"
157+
if mcp_introspection_error:
158+
result["mcp_introspection_error"] = mcp_introspection_error
159+
if mcp_introspection_error and "No module named 'fastmcp'" in mcp_introspection_error:
160+
result["mcp_tools_note"] = "fastmcp not installed; install with: pip install 'roam-code[mcp]'"
161+
else:
162+
result["mcp_tools_note"] = (
163+
"fastmcp transport unavailable; install or repair with: pip install 'roam-code[mcp]'"
164+
)
152165
return result
153166

154167

@@ -215,6 +228,8 @@ def surface(ctx, filter_by: str, category: str | None):
215228
}
216229
if "mcp_tools_note" in data:
217230
summary["mcp_tools_note"] = data["mcp_tools_note"]
231+
if "mcp_introspection_error" in data:
232+
summary["mcp_introspection_error"] = data["mcp_introspection_error"]
218233
click.echo(
219234
to_json(
220235
json_envelope(
@@ -236,9 +251,7 @@ def surface(ctx, filter_by: str, category: str | None):
236251
if data["mcp_introspection_available"]:
237252
click.echo(f" mcp tools: {data['mcp_tool_count']}")
238253
else:
239-
click.echo(
240-
f" mcp tools: {data['mcp_tool_count']} (introspection unavailable; install 'roam-code[mcp]')"
241-
)
254+
click.echo(f" mcp tools: {data['mcp_tool_count']} ({data['mcp_tools_note']})")
242255
click.echo("")
243256
click.echo(" by maturity:")
244257
for level in ("stable", "experimental", "internal", "deprecated"):

src/roam/mcp_server.py

Lines changed: 32 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -13,11 +13,14 @@
1313
from __future__ import annotations
1414

1515
import asyncio
16+
import contextlib
17+
import io
1618
import json
1719
import os
1820
import subprocess
1921
import sys
2022
import time as _time
23+
import warnings
2124
from pathlib import Path
2225
from typing import Literal
2326

@@ -26,23 +29,46 @@
2629

2730
from roam.ask.workflow import workflow_metadata_for_recipe
2831

32+
_FASTMCP_IMPORT_ERROR: str | None = None
2933
try:
30-
from fastmcp import Context as _Context
31-
from fastmcp import FastMCP
32-
except ImportError:
34+
with warnings.catch_warnings(), contextlib.redirect_stderr(io.StringIO()):
35+
warnings.simplefilter("ignore")
36+
from fastmcp import Context as _Context
37+
from fastmcp import FastMCP
38+
except Exception as exc:
3339
_Context = None
3440
FastMCP = None
41+
_FASTMCP_IMPORT_ERROR = f"{exc.__class__.__name__}: {exc}"
3542

3643
try:
3744
from mcp.types import ToolAnnotations as _ToolAnnotations
3845
except ImportError:
3946
_ToolAnnotations = None
4047

4148
try:
42-
from fastmcp.server.tasks.config import TaskConfig as _TaskConfig
49+
with warnings.catch_warnings(), contextlib.redirect_stderr(io.StringIO()):
50+
warnings.simplefilter("ignore")
51+
from fastmcp.server.tasks.config import TaskConfig as _TaskConfig
4352
except Exception:
4453
_TaskConfig = None
4554

55+
56+
def _fastmcp_unavailable_message() -> str:
57+
"""Return the operator-facing message for an unavailable MCP transport."""
58+
if _FASTMCP_IMPORT_ERROR and "No module named 'fastmcp'" not in _FASTMCP_IMPORT_ERROR:
59+
return (
60+
"fastmcp transport unavailable for the MCP server.\n"
61+
f"import error: {_FASTMCP_IMPORT_ERROR}\n"
62+
"repair it with: pip install roam-code[mcp]\n"
63+
"if this looks unexpected, run `roam doctor` to diagnose your install."
64+
)
65+
return (
66+
"fastmcp is required for the MCP server.\n"
67+
"install it with: pip install roam-code[mcp]\n"
68+
"if this looks unexpected, run `roam doctor` to diagnose your install."
69+
)
70+
71+
4672
# MCP-native enhancements (sampling, watcher, session, progress, completions).
4773
# Each module is best-effort -- import failures degrade gracefully.
4874
try:
@@ -15848,12 +15874,7 @@ def mcp_cmd(transport, host, port, no_auto_index, list_tools, list_tools_json, c
1584815874
raise SystemExit(1)
1584915875

1585015876
if mcp is None:
15851-
click.echo(
15852-
"error: fastmcp is required for the MCP server.\n"
15853-
"install it with: pip install roam-code[mcp]\n"
15854-
"if this looks unexpected, run `roam doctor` to diagnose your install.",
15855-
err=True,
15856-
)
15877+
click.echo(f"error: {_fastmcp_unavailable_message()}", err=True)
1585715878
raise SystemExit(1)
1585815879

1585915880
if list_tools_json:
@@ -16132,9 +16153,5 @@ def prompt_health_check() -> str:
1613216153

1613316154
if __name__ == "__main__":
1613416155
if mcp is None:
16135-
raise SystemExit(
16136-
"fastmcp is required for the MCP server.\n"
16137-
"Install it with: pip install roam-code[mcp]\n"
16138-
"If this looks unexpected, run `roam doctor` to diagnose your install."
16139-
)
16156+
raise SystemExit(_fastmcp_unavailable_message())
1614016157
mcp.run()

src/roam/surface_counts.py

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -111,6 +111,68 @@ def mcp_tool_names() -> list[str]:
111111
return sorted(names)
112112

113113

114+
_MCP_TOOL_ALIASES: dict[str, set[str]] = {
115+
"annotate-symbol": {"roam_annotate_symbol"},
116+
"bisect": {"roam_bisect_blame"},
117+
"breaking": {"roam_breaking_changes"},
118+
"budget": {"roam_budget_check"},
119+
"capsule": {"roam_capsule_export"},
120+
"cga": {"roam_cga_emit", "roam_cga_verify"},
121+
"churn": {"roam_weather"},
122+
"complexity": {"roam_complexity_report"},
123+
"context": {"roam_context", "roam_ws_context"},
124+
"dead": {"roam_dead_code"},
125+
"digest": {"roam_trends"},
126+
"file": {"roam_file_info"},
127+
"findings": {"roam_findings_count", "roam_findings_list", "roam_findings_show"},
128+
"oracle": {
129+
"roam_oracle_is_clone_of",
130+
"roam_oracle_is_reachable_from_entry",
131+
"roam_oracle_is_test_only",
132+
"roam_oracle_route_exists",
133+
"roam_oracle_symbol_exists",
134+
},
135+
"refs": {"roam_uses"},
136+
"review": {"roam_review_change"},
137+
"rules": {"roam_check_rules", "roam_rules_check", "roam_rules_validate"},
138+
"search": {"roam_search_semantic", "roam_search_symbol"},
139+
"snapshot": {"roam_trends"},
140+
"trend": {"roam_trends"},
141+
"trends": {"roam_trends"},
142+
"understand": {"roam_understand", "roam_ws_understand"},
143+
"uses": {"roam_uses"},
144+
"vulns": {"roam_vuln_map", "roam_vuln_reach"},
145+
"weather": {"roam_weather"},
146+
}
147+
148+
_MCP_TOOL_SUFFIXES: tuple[str, ...] = (
149+
"blame",
150+
"changes",
151+
"check",
152+
"code",
153+
"emit",
154+
"export",
155+
"info",
156+
"report",
157+
"validate",
158+
"verify",
159+
)
160+
161+
162+
def mcp_candidate_tool_names(command_name: str) -> set[str]:
163+
"""Return plausible MCP wrapper names for a CLI command.
164+
165+
Used by both ``roam surface`` and the MCP-wrapper coverage audit. Keep
166+
the non-obvious aliases here so those two surfaces cannot drift apart
167+
on names like ``dead`` -> ``roam_dead_code``.
168+
"""
169+
base = command_name.replace("-", "_")
170+
candidates = {f"roam_{base}"}
171+
candidates.update(f"roam_{base}_{suffix}" for suffix in _MCP_TOOL_SUFFIXES)
172+
candidates.update(_MCP_TOOL_ALIASES.get(command_name, set()))
173+
return candidates
174+
175+
114176
def mcp_tool_descriptions() -> list[tuple[str, str]]:
115177
"""Return ``(tool_name, description)`` for every ``@_tool(...)`` decoration.
116178

tests/test_cli_contract.py

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -423,6 +423,36 @@ def test_explain_command_surface_json(cli_runner, empty_dir):
423423
assert cmd_field["name"] == "surface"
424424

425425

426+
def test_explain_command_table_scan_ignores_docstrings_and_comments(tmp_path):
427+
"""The table scanner should read SQL literals, not prose."""
428+
from roam.commands.cmd_explain_command import _scan_module_for_tables
429+
430+
module_path = tmp_path / "cmd_demo.py"
431+
module_path.write_text(
432+
'"""This command reads from the graph and writes into reports."""\n'
433+
"# UPDATE the docs from time to time\n"
434+
"SQL = '''\n"
435+
"SELECT s.id\n"
436+
"FROM symbols s\n"
437+
"JOIN files f ON s.file_id = f.id\n"
438+
"'''\n",
439+
encoding="utf-8",
440+
)
441+
442+
assert _scan_module_for_tables(module_path) == ["files", "symbols"]
443+
444+
445+
def test_explain_command_complexity_tables_are_sql_tables(cli_runner, empty_dir):
446+
"""Complexity command metadata should not include prose false positives."""
447+
result = cli_runner.invoke(cli, ["--json", "explain-command", "complexity"])
448+
assert result.exit_code == 0, result.output
449+
data = _parse_json_strict(result, "explain-command complexity --json")
450+
451+
tables = set(data["command_info"]["db_tables_referenced"])
452+
assert {"files", "symbol_metrics", "symbols"}.issubset(tables)
453+
assert not {"the", "roam", "emitting", "rankings"}.intersection(tables)
454+
455+
426456
# ---------------------------------------------------------------------------
427457
# `roam db-check --json` shape (run on a real indexed project)
428458
# ---------------------------------------------------------------------------

tests/test_mcp_server.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -569,6 +569,24 @@ def test_missing_fastmcp(self):
569569
assert result.exit_code == 1
570570
assert "roam-code[mcp]" in result.output
571571

572+
def test_broken_fastmcp_reports_import_error(self):
573+
"""Installed-but-broken FastMCP should surface the import failure."""
574+
from roam.mcp_server import mcp_cmd
575+
576+
runner = CliRunner()
577+
with (
578+
patch("roam.mcp_server.mcp", None),
579+
patch(
580+
"roam.mcp_server._FASTMCP_IMPORT_ERROR",
581+
"ImportError: cannot import name '_coerce_tool_transform_configs' from 'fastmcp.mcp_config'",
582+
),
583+
):
584+
result = runner.invoke(mcp_cmd, ["--no-auto-index"])
585+
assert result.exit_code == 1
586+
assert "fastmcp transport unavailable" in result.output.lower()
587+
assert "_coerce_tool_transform_configs" in result.output
588+
assert "roam-code[mcp]" in result.output
589+
572590
def test_list_tools_flag(self):
573591
"""--list-tools should print registered tools without starting server."""
574592
from roam.mcp_server import mcp_cmd

0 commit comments

Comments
 (0)