feat(DRC-3526): MCP App widget POC — 15 widget tools + config-install helper

[iamcxa]

Summary

Recce MCP App POC (DRC-3526). Ships 15 widget-enabled MCP tools rendered as interactive HTML widgets in Claude Desktop and other MCP App-capable hosts, plus a recce mcp-config-install helper that auto-writes the Claude Desktop config so POC users skip the hand-edited JSON.

Architecture is a parallel FastMCP-based widget server (recce mcp-widget-server) alongside the existing low-level recce mcp-server, coordinated by RECCE_MCP_WIDGETS=1. Day 0 spike found low-level mcp.server.Server doesn't render widgets in Claude Desktop while mcp.server.fastmcp.FastMCP does — hence two servers rather than upgrading the existing one. Full FastMCP migration is deferred (design Open Q #6; now at 15/20 = 75% widget coverage, past the ≥50% reconsideration threshold).

Demo journeys (recorded)

Both journeys use plain reviewer-style prompts — no mcp__recce-widgets__<tool> tool-naming — to validate that a non-technical reviewer's natural phrasing reliably triggers the right widget.

Demo 1 — Quick scan (Journey A) · Loom

Replaces the AI-summary "block of text" with inline widgets so a reviewer grasps "what changed" in seconds. PR scenario: jaffle_shop_golden#15 (refactor customers.sql → two intermediate models).

Prompt 1 → row_count_diff widget:

I'm reviewing a refactor PR that splits customers.sql into two new intermediate
models. Before I approve, I want to verify that row counts haven't changed
across any of the affected models. Can you compare row counts for the models
modified in this PR?

Prompt 2 → schema_diff widget:

Now show me what columns changed — anything added, removed, or with a
different type — across the same modified models.

Demo 2 — Investigate impact (Journey 2) · Loom

Blast-radius summary → evidence drilldown, with the agent following the next_action advisory hint. PR scenario: jaffle_shop_golden#16 (amount DOUBLE → DECIMAL on stg_payments).

Prompt 1 → impact_analysis widget:

I'm reviewing a PR that changes the amount column type in stg_payments from
DOUBLE to DECIMAL(10,2). Can you show me the blast radius — which downstream
models are affected, and how serious the impact is?

Prompt 2 → profile_diff widget:

Following the next_action hint from the impact analysis, let me see the actual
statistical profile of the amount column in stg_payments — compare the mean,
distribution, null rate, and other stats between base and current to confirm
this is just a rounding effect within expected range.

Fixed in this PR (commit f692f086): impact_analysis's downstream value_diff step derived its per-column diff list from the CURRENT relation only (get_model → get_columns(base=False)), then applied b."col" IS DISTINCT FROM c."col" to both the base (b) and current (c) relations. When a column had drifted — present in current but not base — Snowflake raised a Binder Error (invalid identifier 'B."full_name"' / Table "b" does not have a column named "full_name"). The builder now diffs only the intersection of base ∩ current columns via get_model(node_id, base=True) (which manages its own warehouse connection), mirroring the canonical ValueDiffTask; drifted columns are reported via schema_changes, not fed into the diff expression, and value_diff is skipped when the PK itself has drifted. This was a pre-existing value_diff correctness bug surfaced by the demo, not introduced by the widget work. Verified by a new regression unit test (asserts drifted columns never reach the generated SQL) plus a read-only A/B against live Snowflake (old SQL shape → 000904 invalid identifier; intersected shape → executes clean).

What's in this PR

New surface:

• recce mcp-widget-server CLI subcommand (local mode only — iter 1 scope)
• recce mcp-config-install CLI helper — writes both server entries + RECCE_MCP_WIDGETS=1 into claude_desktop_config.json (macOS, iter 1 scope), with --dry-run and --yes, and a backup of the existing config
• recce/widget_server.py — FastMCP server with 15 @mcp.tool delegates + matching @mcp.resource handlers, idiomatic shape (Pydantic input/output models, CallToolResult with short content + structuredContent, tool annotations, ext-apps SDK theme helpers)
• recce/data/mcp/*.html — 15 self-contained HTML widgets, each with exhaustive @media (prefers-color-scheme: dark) overrides and Claude design-token theming
• WIDGET_TOOLS filter in recce/mcp_server.py — when RECCE_MCP_WIDGETS=1, removes the 15 widget-eligible tools from the main server's list_tools (defers to widget server)

15 widgets by tier:

Phase	Tier	Tools
A	1–2	row_count_diff, schema_diff, get_server_info, list_checks, get_model
B	3	query, query_diff, value_diff, value_diff_detail, top_k_diff
C	4 (charts)	histogram_diff, profile_diff
D	5 (mini-graph)	get_cll, impact_analysis
E	6 (DAG, v1)	lineage_diff — hand-roll SVG, hard 10-node cap, graceful empty-state over cap

Bug fixes (pre-existing, surfaced by the POC):

• recce/track.py print(traceback.format_exc()) → file=sys.stderr — stdout traceback is fatal for any stdio MCP server when RecceConfig fails
• recce mcp-server / mcp-widget-server now os.chdir(--project-dir) before RecceConfig, so Claude Desktop's cwd=/ spawn finds the dbt project + recce.yml (removes the bash-wrapper workaround)
• SchemaChange class-name collision — Phase D's impact_analysis defined a second module-level SchemaChange that shadowed schema_diff's model, breaking schema_diff serialization with a Pydantic "column / change_status Field required" error. Renamed to ColumnSchemaChange; regression test pins both field surfaces.

Quality / docs:

• tests/test_widget_server.py — 35 tests (registration via public FastMCP API, env-var coordination 18-vs-20 tools, file-missing graceful degradation, CallToolResult shape, Pydantic structuredContent round-trip, annotations, collision guard, lineage_diff 10-node-cap branch)
• tests/test_mcp_config_install.py — 5 tests (project-dir validation, dry-run no-write, backup creation)
• docs/mcp-widgets.md — template doc for Scott's hand-off: file layout, config example, "Add widget N+1" walkthrough, structuredContent contract, gotchas, Python-vs-TypeScript SDK note

Test plan

• pytest tests/test_widget_server.py → 35 passed
• pytest tests/test_mcp_config_install.py → 5 passed
• pytest tests/ -k "mcp or widget or schema_diff or impact or lineage" → 363 passed, no widget-related regressions
• Cwd=/ smoke (recce mcp-widget-server --project-dir <dbt>) — exit 0, clean stderr, no stdout pollution
• Manual end-to-end in Claude Desktop — Demo 1 + Demo 2 recorded (Looms above); widgets render with real data, dark mode correct, plain prompts trigger correct tools
• Manual end-to-end in Cowork (parallel OAuth POC, DRC-3527 — not a blocker)

Note: tests/test_server.py::test_spa_route_lineage fails locally because the frontend SPA build artifact (recce/data/lineage/index.html) isn't present in this worktree — pre-existing, unrelated to this PR (fails identically with the diff stashed; resolved by cd js && pnpm run build).

User-facing changes

For Claude Desktop POC users:

• Install config the easy way: recce mcp-config-install --project-dir /path/to/dbt (writes both entries + env var, backs up existing config). Or hand-edit per docs/mcp-widgets.md.
• RECCE_MCP_WIDGETS=0/unset → zero change, all 20 tools text-only from recce mcp-server, backward compatible.
• RECCE_MCP_WIDGETS=1 → recce mcp-server serves 5 non-widget tools, recce mcp-widget-server serves the 15 widget tools. No name collision.
• Other MCP hosts (no MCP Apps support) → graceful degradation: host ignores _meta.ui.resourceUri, uses the tool's one-sentence content text.

What's NOT in this PR

• Cloud session / snapshot support for recce mcp-widget-server (iter 1 = local mode only). This is the strategic next step — MCP ↔ Recce Cloud is what lets non-technical users skip git/dbt entirely and is where PR-selection + artifact-packaging are properly solved (not as extra local tools).
• Full FastMCP migration collapsing the two-server architecture (design Open Q [Chore] Add flake8 style check #6; now warranted at 75% coverage)
• lineage_diff beyond v1 — progressive degradation for >10 nodes (shrink / zoom / aggregate) and the ReactFlow @datarecce/ui/lineage borrow contract
• Lifespan refactor (_recce_server still module-global)
• OAuth POC for Cowork distribution (Kent Huang DRC-3527, parallel)

Reviewer notes

• docs/mcp-widgets.md is the canonical Scott hand-off doc — if the "Add widget N+1" walkthrough leaves a cold reader unsure, that's PR-blocking feedback.
• Commit history is deliberately preserved (not squashed) — each commit is one widget / fix / refactor step with meaningful diff boundaries; review in order.
• CSS follows project conventions: space-separated rgb(), no @datarecce/ui internal-path imports (widget HTML is plain JS).

🤖 Generated with Claude Code

---

Review follow-up — deferred items (⚠️ this PR is NOT rushing to merge)

A full review pass (kc-pr-review + kc-pr-review-resolve) ran on d089e737. Actionable fixes are committed (65ca0739→d089e737): ~ path expansion + atomic config write in mcp-config-install, case-insensitive RECCE_MCP_WIDGETS, stdio-safe --debug traceback, robust recce/data/** gitignore (nested build artifacts now ignored; recce/data/mcp/ source tracked), single-quote HTML escaping across all 15 widgets, rel="noopener noreferrer" on the PR link, widget HTML load anchored on the recce package (no longer depends on namespace-package resolution → reliable under pip install), and doc/test-count sync.

even-wei's NO-GO blocker (the test_widget_server.py black/isort format gate) was already resolved in 230af0f0 and is re-verified clean on the current head (black/isort/flake8 exit 0).

The items below are intentionally deferred — this is an iter-1 POC whose goal is breadth (a widget for every tool), not hardened design. Each is marked with a TODO(DRC-3526 follow-up) comment in code; a DRC follow-up ticket will track them:

• Widget tools bypass the central call_tool error classifier/telemetry (DRC-2754). Errors still surface (FastMCP wraps them); what's deferred is expected-error classification (table_not_found/permission_denied → warning) + Sentry metrics for the 15 widget tools. Resolve when the two-server architecture is consolidated. — recce/widget_server.py
• impact_analysis value_diff drift fix lacks a real-DuckDB e2e. Covered by a mock test (drifted column absent from generated SQL) + a manual Snowflake A/B; a real-adapter e2e with a PK + a column present only in current is the gap. — recce/mcp_server.py
• schema_diff has no single-env warning. The empty-state ("No models found") can read as a clean diff when there is simply no base to compare against. — recce/widget_server.py
• Type contracts use free str for fixed-set fields (status / change_status / priority) — batch a Literal/submodel tightening pass. — recce/widget_server.py
• impact_analysis doubles base-side warehouse introspection (the cost of the binder-error correctness fix) — consider caching/batching get_model(base=True). — even-wei N3
• 15 copy-pasted escapeHtml/svgEl helpers — extract a shared asset to keep the escaping contract single-sourced. — even-wei N5
• check_base_freshness SHA-freshness is opt-in with no production caller (the stale_sha/expected_base_sha path is a tested opt-in API, not pure dead code). Decide in a focused PR: leave as-is, wire a caller, or remove SHA-freshness — it touches the unrelated recce check-base command + tests, so it is out of scope here. — recce/cli.py

Pre-merge check: run a packaged-wheel smoke test (pip install the built wheel, start recce mcp-widget-server, load one widget) to confirm recce/data/mcp/ ships and resolves under the installed layout.

[codecov]

Codecov Report

❌ Patch coverage is 93.73421% with 124 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
recce/cli.py	53.79%	67 Missing ⚠️
recce/widget_server.py	91.94%	54 Missing ⚠️
recce/mcp_server.py	94.54%	3 Missing ⚠️

Files with missing lines	Coverage Δ
recce/track.py	86.84% <100.00%> (ø)
tests/test_check_base.py	100.00% <100.00%> (ø)
tests/test_mcp_cloud_backend.py	100.00% <100.00%> (ø)
tests/test_mcp_config_install.py	100.00% <100.00%> (ø)
tests/test_mcp_server.py	99.86% <100.00%> (+<0.01%)	⬆️
tests/test_widget_server.py	100.00% <100.00%> (ø)
recce/mcp_server.py	91.30% <94.54%> (+0.02%)	⬆️
recce/widget_server.py	91.94% <91.94%> (ø)
recce/cli.py	67.51% <53.79%> (-1.47%)	⬇️

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

Adds an Iter 1 POC for MCP Apps widgets by introducing a parallel FastMCP-based “widget server” that serves widget-capable versions of row_count_diff and schema_diff, coordinated with the existing stdio MCP server via RECCE_MCP_WIDGETS=1.

Changes:

• Added recce mcp-widget-server (FastMCP) with widget tool delegates + HTML widget resources for row_count_diff and schema_diff.
• Coordinated tool routing between servers using WIDGET_TOOLS + _widgets_enabled() filtering in recce/mcp_server.py.
• Added tests and documentation for the widget workflow; fixed one stdout→stderr issue for stdio MCP safety.

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
tests/test_widget_server.py	Adds test coverage for widget server registration, env-var coordination, and tool result shapes.
recce/widget_server.py	Implements FastMCP widget server with two widget tools and two HTML resources.
recce/mcp_server.py	Adds widget tool filtering/guardrails and extracts _compute_schema_changes() for shared schema-diff shape.
recce/cli.py	Adds mcp-widget-server CLI command and ensures --project-dir becomes cwd for config discovery.
recce/track.py	Sends traceback output to stderr to avoid corrupting stdio JSON-RPC.
recce/data/mcp/row_count_diff.html	Self-contained MCP Apps widget for row count diffs.
recce/data/mcp/schema_diff.html	Self-contained MCP Apps widget for schema diffs.
docs/mcp-widgets.md	Developer guide/template for adding future widgets.
.gitignore	Stops ignoring the entire recce/data tree; ignores only build output paths so recce/data/mcp/*.html can be committed.
CLAUDE.md	Adds gbrain search guidance block.

[iamcxa]

Acknowledged — minor. _compute_schema_changes can emit base_type / curr_type as None (from .get("type")), and the widget already tolerates it (c.type ?? ""). Tightening the SchemaChange annotation to the real shape ({name,type} / {name,base_type,curr_type} submodels, or Optional) is batched with the other type-contract polish (the Literal/submodel pass) — deferred for the POC, tracked in the follow-up discussion.

[even-wei]

Code Review: PR #1397

SHA 87f22fd3 · Verdict NO-GO

Verified checks: flake8 clean (exit 0); targeted suites 156 passed (test_widget_server.py + test_mcp_config_install.py + test_mcp_server.py) under the project-pinned mcp 1.27.0 / Python 3.13; black --check and isort --check FAIL on tests/test_widget_server.py; bundled security-review found no exploitable Critical/High.

Blockers

None.

Issues

1. tests/test_widget_server.py — Fails the project's required make format gate (black + isort).
   Evidence: black --check → "1 file would be reformatted" (line ~2218 over-length call); isort --check → import block at line ~2248 needs multi-line wrap. AGENTS.md lists make format as a pre-commit/CI requirement.
   Pass E / project-convention.

Notes

1. recce/cli.py:3174 (mcp_config_install) — On re-run the backup is overwritten with the already-modified config, so .recce.bak no longer holds the pristine pre-recce original; only the recce entries differ, and re-run is otherwise idempotent (deterministic new_entries). Consider skip-if-exists or timestamped backups.
   Pass D.
2. recce/data/mcp/*.html — Shared escapeHtml escapes & < > " but not ' ('). Not exploitable today (all interpolated attributes are double-quoted and " is escaped; SVG labels use textContent). Defense-in-depth only; add .replace(/'/g,"'") if a single-quoted attribute is ever introduced.
   Pass B (Security, LOW).
3. recce/mcp_server.py:1885 (impact_analysis) — New get_model(node_id, base=True) adds a second per-model warehouse introspection round-trip inside the impacted-models loop. Correctness fix (avoids binder failure on drifted columns) but doubles base-side introspection on large impacted sets.
   Pass G.
4. recce/cli.py:3012 (mcp_config_install) — recce mcp-config-install and the macOS-only path are correctly OS-guarded, but there is no uninstall / removal path; the docstring's "To undo: replace with .recce.bak" is the only recovery, which is lost after a second run (see Note 1). Untested uninstall for a config-mutating installer.
   Pass E.
5. recce/data/mcp/ — 15 hand-written HTML/JS widgets (~8.4k LOC) carry a copy-pasted escapeHtml + svgEl helper per file. For a POC this is acceptable, but it's 15 sources of truth for the escaping contract; a shared asset would prevent Note 2 drifting per-file.
   Pass I (architecture, copy-paste).

Limits

• uv run pytest (the default dev path) resolves to a STALE local env (Python 3.10 + an older mcp whose FastMCP.resource() lacks the meta kwarg), yielding 32 spurious TypeError: ... unexpected keyword argument 'meta' failures. These are an environment artifact, NOT a code defect — under the pinned mcp>=1.23.0 (resolved to 1.27.0, where resource() DOES accept meta) all 156 tests pass. Verified by forcing the 3.13 venv. CI using the locked deps will pass; a developer with a stale global mcp will see false failures.
• Did not exercise the widgets live in Claude Desktop (macOS MCP-app rendering) — XSS assessment is static data-flow only. SQL query/query_diff tool args flow to the pre-existing _tool_query path (unchanged by this PR), not re-reviewed for injection here.

[even-wei]

Claude Code Review: NO-GO. 1 issue (format gate) + 5 notes. See review comment.

[Copilot]

Pull request overview

Copilot reviewed 27 out of 28 changed files in this pull request and generated 5 comments.

[iamcxa]

@iamcxa — automated review (kc-pr-review, Full tier: code-reviewer + silent-failure-hunter + pr-test-analyzer + type-design-analyzer + HTML-XSS scan + ToB security). Event is COMMENT, not approve: (a) tests could not be re-run locally (env SIGKILL/OOM at pytest import — the first background run did exit 0, and the PR body documents 35+5 passing); (b) the check_base_freshness behavior change needs author intent; (c) the one production-risk path (Snowflake binder in the value_diff fix) is mock + manual-A/B only, no CI e2e. No CRITICAL/HIGH blocking bugs were found.

Goal-achievement verdict: ✅ Achieved (POC scope, with follow-up caveats)

Evidence

• All 15 tool/resource pairs register correctly; every _meta.ui.resourceUri maps to a real recce/data/mcp/*.html (verified file-by-file).
• All 15 HTML widgets escape warehouse data consistently (escapeHtml / textContent / numeric formatters) — no exploitable stored-XSS sink found.
• The impact_analysis value_diff drift fix is correct — it intersects base ∩ current columns and uses get_model(base=True) connection-managed introspection, matching the canonical ValueDiffTask.
• Type design is solid for a POC (None-vs-zero distinctions in RowCountModel, ValueDiffColumnRow.matched_p, HistogramEnvStats.total are deliberate and correct).

Verification Matrix

#	Concern (from PR body)	Verified?	Evidence
V1	RECCE_MCP_WIDGETS=0/unset → all 20 tools text-only, backward compatible	✅	_widgets_enabled() reads env at call time; filter only removes WIDGET_TOOLS when enabled (mcp_server.py:1294)
V2	RECCE_MCP_WIDGETS=1 → main 5 + widget 15, no collision	✅	WIDGET_TOOLS filter + widget server registers exactly the 15-name set; test pins it (test_widget_server.py:102/173)
V3	impact_analysis drift fix (Snowflake binder)	⚠️	Code correct, but e2e gap — see inline on mcp_server.py:1931
V4	schema_diff serialization (SchemaChange collision fix)	⚠️	Models distinct + field-pinned; no behavioral output test — see Advisory F
V5	stdio stdout purity (JSON-RPC channel)	✅	No stdout writes in widget_server.py; logging → stderr (widget_server.py:2521)
V6	chdir to --project-dir before RecceConfig	✅	Present in both subcommands; minor: no dir-existence guard (Advisory)

Verification Summary (tests)

Check	Result
Local re-execution	⚠️ Could not run — env SIGKILL/OOM at pytest import (0-byte output; killed during collection). Not a test-logic failure.
First background run (widget + config_install + check_base + mcp_server + mcp_cloud_backend)	exit 0
PR-body documented	test_widget_server.py 35 passed · test_mcp_config_install.py 5 (file actually has 7) · broad selection 363 passed

Advisory (not blocking)

• A — Bundled tooling / scope creep. CLAUDE.md (+37 gbrain boilerplate, includes a per-machine "on this machine" assertion in shared docs) and .claude/settings.json (enables mcp-server-dev for all repo contributors) are unrelated to the widget feature. Suggest splitting into a separate commit/PR, marking POC-only, or moving dev-plugin prefs to gitignored settings.local.json.
• B — External SDK dependency. All 15 widgets import https://unpkg.com/@modelcontextprotocol/ext-apps@0.4.0 (no SRI). Mitigated by each resource's csp.resourceDomains: ["https://unpkg.com"] (host-enforced origin restriction) and this is the standard MCP Apps SDK load pattern — acceptable for POC; consider local vendoring before productionizing.
• C — Type contracts. status / change_status / priority etc. are free str with legal values only in comments. Converting the widget-branched ones to Literal[...] is a zero-cost contract win.
• D — HTML hardening. No <meta> CSP in the HTML (partially mitigated by resourceDomains); get_server_info.html:317 pr.url href has no scheme validation (javascript: possible, though source is GitHub metadata not warehouse) and target="_blank" lacks rel="noopener".
• E — Mega-PR. Feature POC + 4 unrelated pre-existing bugfixes (track stderr / chdir / set_backend cache-key / freshness) + tooling, 37 commits / +14K. Hard to review and to revert. Future: land pre-existing bugfixes as their own PRs.
• F — Test gaps. schema_diff is the only widget with no behavioral model_validate round-trip test (the collision broke exactly that serialization); no absolute tool-count assertion (assert len==20/5) so dropping a non-widget tool would pass silently.
• G — impact_analysis per-step except Exception (pre-existing, mcp_server.py ~1866/1901/2042, not in this diff) appends to errors[] without logging unexpected exceptions — a code bug is indistinguishable from a warehouse failure. Worth narrowing + exc_info logging when that area is next touched.

Nice work on the POC — the escaping discipline, type care, and the documented drift fix are genuinely solid. The items above are hardening + a couple of bundled changes worth confirming.

[iamcxa]

MEDIUM — SHA freshness is now opt-in (and expected_base_sha), but neither shipping caller passes it: check_base CLI (L3488) and the MCP startup check (mcp_server.py:2807) both omit expected_base_sha. So stale_sha is now unreachable in production, current_sha is always None, and the stale_sha color-map (L3505) + exit-code-2 branch (L3522) are dead.

The rationale (base artifacts come from the base branch → base SHA ≠ feature HEAD → the old auto-check was a false-positive generator) is sound. But this is a behavior change to the existing recce check-base command, not mentioned in the PR body and bundled into a widget POC. Please confirm it's intentional — if so, clean up the dead stale_sha/current_sha path, or wire a caller that actually passes expected_base_sha.

[iamcxa]

Deferred to the follow-up discussion (POC scope) — leaving this thread open as a tracked TODO. The opt-in SHA change is intentional behavior-wise (base artifacts come from the base branch, so the base SHA always differs from feature HEAD → the old auto-check was a false-positive generator). But stale_sha / current_sha / the exit-code-2 branch are now dead for both callers. Decision pending: clean up the dead path vs. wire a caller that passes expected_base_sha.

[iamcxa]

On reflection, holding this rather than cleaning it up inside this widget PoC. The stale_sha / expected_base_sha path is not pure dead code — it's a tested opt-in API: test_status_stale_sha_when_expected_base_sha_mismatch exercises the mismatch → stale_sha branch, and test_status_stale_sha pins the no-arg = fresh behavior. So it's "no production caller yet", not "unreachable".

Removing it would touch the recce check-base command (color-map, exit-code-2, docstring) plus those tests — a cross-feature refactor outside this widget PoC's scope, and one I can't validate locally right now (the full pytest suite OOMs in this env; only the lint tools run). Proposal: either leave the opt-in as-is (harmless + tested), or do a focused follow-up that wires a caller passing expected_base_sha (giving it a purpose) or removes SHA-freshness entirely. Leaving this thread open for that decision.

[iamcxa]

MEDIUM — Widget tools call _recce_server._tool_*() directly, bypassing the central @server.call_tool handler in mcp_server.py:1426. That handler is where DRC-2754 error classification lives: _classify_db_error → downgrade table_not_found/permission_denied to warnings + sentry_metrics.count("mcp.expected_error", ...). For all 15 widget tools that machinery never runs — expected DB errors become hard errors and there's zero telemetry.

Not a silent failure (FastMCP still wraps the raised exception into an error result), so not blocking for the POC — but it quietly undoes documented investment. Suggest routing widget delegates through a shared classify/telemetry wrapper, or a tracking note that the widget path defers DRC-2754 parity.

[iamcxa]

Deferred to the follow-up discussion (POC scope) — leaving open as a tracked TODO. The widget delegates bypass the central call_tool classifier/telemetry (DRC-2754). Errors still surface (FastMCP wraps them); what's lost is expected-error classification + Sentry metrics for the 15 widget tools. Tracking a shared classify/telemetry wrapper for when the two-server architecture is consolidated.

[iamcxa]

TODO marker added in d080819e (above _recce_server) and tracked in the PR description's "Review follow-up — deferred items" section. Deferred per iter-1 POC scope; leaving open until the shared classify/telemetry wrapper lands (two-server consolidation).

[iamcxa]

MEDIUM (test gap) — This drift fix is correct (intersection + get_model(base=True) connection-managed introspection, matching ValueDiffTask). But it has no real-adapter e2e. test_mcp_e2e.py::test_schema_changes_detected builds the drift shape against real DuckDB, yet its users fixture declares no primary_key, so if not pk: continue (L1917) skips value_diff entirely and the IS DISTINCT FROM binder path is never exercised on a live warehouse. Coverage is mock-only (drifted column absent from generated SQL) + the documented manual Snowflake A/B.

Per project MEMORY, mocks can't catch the missing-warehouse-connection class of bug. Recommend an e2e fixture with a PK where current has a column absent in base; assert impact_analysis returns value_diff over the common non-PK columns and does not raise.

[iamcxa]

Deferred to the follow-up discussion — leaving open as a tracked test-coverage TODO. The fix is mock-tested (asserts drifted columns never reach the generated SQL) plus the documented manual Snowflake A/B, and even-wei independently verified 156 tests pass. A real-DuckDB e2e with a PK + a column present only in current is the gap to close; not a blocker for the POC.

[iamcxa]

TODO marker added in d080819e (at the value_diff intersection) and tracked in the PR description's deferred section. Deferred; leaving open until the real-DuckDB e2e fixture (PK + drifted column) lands.

[iamcxa]

LOW — SchemaDiffOutput has no warning field and schema_diff never extracts _warning, unlike the other 13 diff tools (e.g. L202 warning = result.pop("_warning", None)). In single-env mode it renders "No models found" (schema_diff.html:264), which reads as "no schema changes" when the truth is "no base to compare against". (_tool_schema_diff also lacks the single-env warning on the main server — pre-existing — but the widget's empty-state makes it more misleading.) Suggest adding a warning field + the single-env notice.

[iamcxa]

Deferred to the follow-up discussion (POC scope) — leaving open as a tracked TODO. Likely fix: add a warning field to SchemaDiffOutput + extract _warning, plus the single-env notice, so the empty-state ("No models found") isn't read as a clean diff when there's simply no base to compare against.

[iamcxa]

TODO marker added in d080819e (at the SchemaDiffOutput build) and tracked in the PR description's deferred section. Deferred; leaving open until the warning field + single-env notice are added.

[iamcxa]

@even-wei — addressing your NO-GO (reviewed at 87f22fd3; current head is c533be93, which has 3 commits after your review's SHA + this resolve pass):

Blocker (format gate) — already resolved before the current head: 230af0f0 reformatted tests/test_widget_server.py. Verified now on c533be93: black --check, isort --check-only, and flake8 all exit 0.

Note 1 / 4 (backup clobbered on re-run, undo lost) — fixed in eeb5002d: the backup is skip-if-exists, so .recce.bak keeps the pristine pre-recce original across re-runs. Atomic write also added (65ca0739): a mid-write failure now leaves the existing config intact.

Note 2 (escapeHtml doesn't escape ') — fixed in c6d3506e: all 15 widgets now escape ' → '.

Note 3 (double base-side introspection in impact_analysis) — acknowledged; this is the tradeoff for the binder-error correctness fix. Tracking a cache/batch of the base-side get_model as a follow-up.

Note 5 (15 copy-pasted escapeHtml/svgEl helpers) — acknowledged; the POC accepts per-file helpers. Shared-asset extraction tracked as a follow-up.

Also note your "Limits": the uv run pytest stale-env failures (older mcp lacking the resource(meta=...) kwarg) are an environment artifact — confirmed; under the pinned mcp>=1.23.0 all suites pass, which your forced-3.13 run verified (156 passed).

Re-requesting your review on c533be93.

[even-wei]

Code Review: PR #1397

SHA 6fb3bff · Verdict NO-GO

Blockers

1. recce/widget_server.py:84 — PullRequestInfo.id: Optional[str] rejects the int id the state actually carries, so get_server_info crashes whenever PR metadata is present.
   Evidence: recce/pull_request.py:14 is Optional[Union[int, str]] populated from pr.number (int); reproduced in this venv — ServerInfoOutput(..., pull_request={'id': 15, ...}) → ValidationError: pull_request.id Input should be a valid string. Tests only cover pull_request is None (tests/test_widget_server.py:427).
   Pass C.

2. recce/cli.py:3132-3135 — --dry-run (and a declined confirm prompt) still creates claude_desktop_config.json + parent dirs when the file doesn't exist, violating the documented "without writing" contract.
   Evidence: the skeleton write_text runs before the if dry_run: check at cli.py:3199; reproduced — mcp-config-install --dry-run with a nonexistent --config exits 0 and leaves the file on disk. The dry-run test pre-creates the file so the suite can't catch it (tests/test_mcp_config_install.py:166).
   Pass A.

Issues

1. recce/widget_server.py (all 15 tools) — single args: <Model> parameter makes FastMCP emit a nested inputSchema (properties.args.$ref) while every docstring documents flat parameters and the same-named tools on recce mcp-server use flat schemas.
   Evidence: nested {"properties": {"args": {...}}, "required": ["args"]} reproduced with mcp.server.fastmcp in this venv — description ≠ schema breaks the repo's "MCP tool description = LLM agent contract" rule, and flipping RECCE_MCP_WIDGETS silently changes the calling convention.
   Pass C.

2. recce/widget_server.py:692-699 (also :1162-1170) — query/query_diff declare readOnlyHint: True, destructiveHint: False but execute the raw sql_template (a DELETE runs); hosts use these hints to skip confirmations.
   Evidence: QueryTask → execute_sql_with_limit (recce/tasks/query.py:43) passes the statement straight to adapter.execute; relatedly tests/test_widget_server.py:336 pins openWorldHint: False for row_count_diff, which runs COUNT(*) against the warehouse.
   Pass B.

3. recce/data/mcp/get_server_info.html:317 — PR link href has no scheme allowlist; a javascript: URL executes on click (escapeHtml doesn't neutralize schemes).
   Evidence: `<a href="${escapeHtml(pr.url)}" target="_blank" ...>` — only href-from-data sink in the PR; gate on http(s):.
   Pass B (MEDIUM — pr.url originates from state/PR metadata, not direct user input).

4. recce/data/mcp/impact_analysis.html:546-561 — the mini-graph fabricates edges: every modified model is connected to every downstream model (cartesian product), visually asserting lineage relationships that may not exist.
   Evidence: for (const srcName of modifiedNames) { ... for (const dstName of downstreamNames) { ... appendChild(path) (comment admits "simplified").
   Pass A.

5. recce/data/mcp/query_diff.html:424-436 / value_diff_detail.html:384-397 — in_a/in_b are located by key || name but filtered out by name only → header/data column misalignment when they diverge; and when the columns are absent (inAIdx === -1) every row is labeled "Removed".
   Evidence: findIndex(c => c.key === "in_a" || c.name === "in_a") vs filter(c => c.name !== "in_a"); const status = (!inB) ? "removed" : "added" with inB=false fallback.
   Pass A.

6. recce/data/mcp/value_diff.html:401,426 — "All values match" banner renders when every matched_p is null (false-quiet, against the repo's false-loud principle), and the derived mismatched-row count can render negative.
   Evidence: null-excluded mismatchedCols + added === 0 && removed === 0 → "Every column is 100% identical"; Math.round(common - matchedCount) unclamped.
   Pass A.

7. 11 of 15 widgets call render() with no try/catch — any throw leaves the widget stuck on "Loading…" forever.
   Evidence: bare app.ontoolresult = ({ structuredContent }) => { render(structuredContent ?? {}); } in query, query_diff, row_count_diff, value_diff_detail, schema_diff, list_checks, top_k_diff, histogram_diff, value_diff, get_model, get_server_info; only get_cll/impact_analysis/lineage_diff/profile_diff wrap it.
   Pass D.

8. recce/widget_server.py:295-301 — schema_diff runs get_lineage_diff() + adapter.select_nodes synchronously inside the async handler (no executor), blocking the FastMCP event loop for all concurrent tool/resource calls; the get_model/get_cll/histogram_diff delegates share the pattern (inherited, but newly exposed to FastMCP concurrency).
   Pass H.

9. recce/widget_server.py:531,617-628 — the documented not_found: bool miss-signal is dead code: _tool_get_model raises ValueError when the model is missing (mcp_server.py:2179-2183), so the branch never fires and the docstring contract is wrong.
   Pass C.

10. recce/widget_server.py:360-370 — get_server_info docstring promises base_status "reveals stale or missing artifacts", but only run_mcp_server runs the freshness check; the widget server always reports "unknown"/"single_env".
    Evidence: _base_status set only at mcp_server.py:2815; run_widget_server (widget_server.py:2528-2573) never sets it.
    Pass C.

11. Tests — schema_diff is the only widget tool whose handler path is never executed (the very tool that had the serialization regression); zero error-path tests across the 15 delegates (handler raising, None raw results); the call_tool widget-rejection guard (mcp_server.py:1319-1321) and the run_widget_server/mcp_widget_server bootstrap are untested (every test injects ws._recce_server directly).
    Evidence: no ws.schema_diff(...) call in tests/test_widget_server.py; only error tests are missing-HTML (line 143) and lineage over-cap (line 2290).
    Pass E.

12. CLAUDE.md:101-137 — machine-local GBrain guidance ("GBrain is set up and synced on this machine", .gbrain-source, ~/.gstack/) committed into the shared repo's agent instructions; false/noise for every other contributor's environment.
    Pass F.

Notes

1. recce/widget_server.py:60-64 — SchemaChange fields typed List[Dict[str, str]] but _compute_schema_changes emits .get("type") values that can be None (the documented present-but-None CLAUDE.md gotcha); use Optional[str]. Pass C.
2. recce/widget_server.py:71-77 — GitInfo.base_branch/base_sha/current_sha can never populate (state GitRepoInfo has only branch); the matching render block in get_server_info.html:296-309 is dead. Pass C.
3. recce/widget_server.py:1975 — get_cll pops _warning but _tool_get_cll never adds one, and lineage_diff has no warning field at all — in single-env mode it renders an empty diff that reads as clean (same false-quiet class as the acknowledged schema_diff deferral, which covers only schema_diff). Pass A.
4. recce/data/mcp/profile_diff.html:283,333 — string stats are escaped twice (R&D displays as R&D); strStatRow null-checks miss undefined. Pass A.
5. recce/data/mcp/lineage_diff.html:193,255,389 — layout keyed on n.idx with no validation; missing/duplicate idx stacks all nodes at one position. Pass A.
6. recce/data/mcp/row_count_diff.html:296-311 — === null guards miss undefined (safe under the current Pydantic dump contract, fragile to it), and the summary "Changed" counter counts null-vs-value rows the badge shows as "N/A". Pass A.
7. recce/data/mcp/query_diff.html:485 / value_diff_detail.html:444 — inline onclick handlers break silently under a strict host CSP; prefer addEventListener. Pass A.
8. recce/data/mcp/top_k_diff.html:359,410 — only base.values is iterated (current-only categories depend on an unasserted server union contract) and counts aren't coerced to Number (string counts concatenate in the "shown" stat). Pass A.
9. tests/test_check_base.py:77+ — seven dead patch("recce.git.current_commit_hash") contexts and the stale test_status_stale_sha name survive the opt-in change; expected_base_sha has no production caller (acknowledged deferral). Pass E/F.
10. PR description says "18-vs-20 tools"; the real split is 20 widgets-off / 5 widgets-on / 15 widget tools. Stale test name ..._six_tools_and_six_resources (tests/test_widget_server.py:89) asserts 15. Pass F.
11. Verification: black/isort/flake8 clean at 6fb3bff; all PR test files pass; the 19 failing tests in this env (test_cli_cache, test_server SPA routes, …) fail identically on origin/main — pre-existing/environmental, not introduced here.

---

Multi-pass adversarial review (recce-dev:claude-code-review) — passes A–I + security skill; widget_server.py, 15 HTML widgets, and test suite each independently reviewed and cross-refuted.

[even-wei]

Claude Code Review: Critical issues found (2 blockers, 12 issues). See review comment: #1397 (comment)