-
Notifications
You must be signed in to change notification settings - Fork 25
feat(mcp): single-env onboarding, --help prerequisites, and error classification #1175
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
iamcxa
wants to merge
15
commits into
main
Choose a base branch
from
fix/drc-2796-mcp-help-prerequisites
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
058f874
feat(mcp): add single-env onboarding mode for MCP server
iamcxa 32741b8
test(mcp): add single-env and E2E tests
iamcxa 9c80611
feat(mcp): add error classification to reduce Sentry noise
iamcxa 8e2dc61
test(mcp): add error classification tests
iamcxa f020bb5
fix(mcp): address PR review concerns and add CLI coverage
iamcxa be88132
test(mcp): add coverage for call_tool handler and schema_diff classif…
iamcxa e5f4fab
test(mcp): close remaining Codecov patch coverage gaps
iamcxa f99524a
docs: add MCP developer skill and update AGENTS.md
iamcxa d74e933
docs: add MCP E2E verification skill with test template
iamcxa af4c3cc
chore: revert personal Makefile changes
iamcxa 2e4c392
docs(mcp): add test coverage gap analysis and E2E verification gate t…
iamcxa 43a9d9d
feat(mcp): add server instructions for single-env mode awareness
iamcxa 8802942
fix(mcp): address code review findings from PR review
iamcxa bb509f1
fix(mcp): address reviewer feedback — rename and improve N/A display
iamcxa 65eda1a
Merge branch 'main' into fix/drc-2796-mcp-help-prerequisites
gcko File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,82 @@ | ||
| --- | ||
| name: recce-mcp-dev | ||
| description: Use when modifying recce/mcp_server.py, MCP tool handlers, error classification, or MCP-related tests. Also use when adding new MCP tools or changing tool response formats. | ||
| --- | ||
|
|
||
| # Recce MCP Server Development | ||
|
|
||
| ## Architecture | ||
|
|
||
| `RecceMCPServer` registers `list_tools`/`call_tool` handlers via MCP SDK `Server`. `call_tool` dispatches to `_tool_*` methods, classifies errors, logs/emits metrics, re-raises. | ||
|
|
||
| Entry point `run_mcp_server()` pops `single_env` before passing kwargs to `load_context()`. | ||
|
|
||
| ## Key Patterns | ||
|
|
||
| **Error classification** — Shared indicator lists defined in `recce/tasks/rowcount.py`. Priority order (`PERMISSION_DENIED` > `TABLE_NOT_FOUND` > `SYNTAX_ERROR`) enforced by `_classify_db_error()` in `mcp_server.py` and `_query_row_count()` in `rowcount.py`. Classified → `logger.warning()` + `sentry_metrics.count()` (when sentry_sdk available). Unclassified → `logger.error()` + traceback. | ||
|
|
||
| **MCP SDK quirk** — Handler must **raise** for SDK to set `isError=True`. | ||
|
|
||
| **Response contracts** — See CLAUDE.md. Additive `_meta` only. `summary.py`: guard with `is None`, not `dict.get(key, 0)`. | ||
|
|
||
| **Single-env** — `_maybe_add_single_env_warning()` adds `_warning` to diff results. Descriptions get conditional note. | ||
|
|
||
| ## Testing (Three Layers) | ||
|
|
||
| | Layer | File | Data Source | Runs In | Purpose | | ||
| |-------|------|-------------|---------|---------| | ||
| | Unit | `tests/test_mcp_server.py` | Mock `RecceContext` | CI (`pytest`) | Logic correctness — tool handlers, error classification, response format | | ||
| | Integration | `tests/test_mcp_e2e.py` | `DbtTestHelper` + DuckDB (fixed data) | CI (`pytest`) | MCP protocol works end-to-end via anyio memory streams | | ||
| | Smoke (E2E) | `/recce-mcp-e2e` skill | User's real dbt project + real database | Manual | All 8 tools return valid results against real data | | ||
|
|
||
| Each new MCP feature or behavior change should be covered at all three layers. | ||
|
|
||
| ## Test Coverage Gap Analysis | ||
|
|
||
| After completing a round of MCP changes (see E2E Gate below for definition), proactively scan for missing test coverage across the three layers before asking about E2E verification. | ||
|
|
||
| **How to check:** | ||
| 1. Identify what changed — new tool handler? new error path? new response field? | ||
| 2. For each change, verify coverage exists at each layer: | ||
| - **Unit**: Does `tests/test_mcp_server.py` have a test case for the new behavior? (happy path + error path) | ||
| - **Integration**: Does `tests/test_mcp_e2e.py` exercise the new tool/feature via MCP protocol? | ||
| - **Smoke**: Will `/recce-mcp-e2e` template cover the new tool? (If a new tool was added, the template may need updating) | ||
|
|
||
| **If gaps are found**, report them to the user before the E2E gate prompt: | ||
|
|
||
| > Test coverage gaps found: | ||
| > - Unit: missing test for `_tool_foo` error path when table not found | ||
| > - Integration: `test_mcp_e2e.py` does not exercise `foo` tool | ||
| > - Smoke: `/recce-mcp-e2e` template does not include `foo` tool | ||
| > | ||
| > Want to fill these gaps before running E2E? | ||
|
|
||
| **Do NOT scan** after: test-only changes, comment/doc edits, import reordering. | ||
|
|
||
| ## E2E Verification Gate | ||
|
|
||
| After each meaningful round of MCP changes, you MUST ask the user: | ||
|
|
||
| > MCP changes complete for this round. Run `/recce-mcp-e2e` to verify? | ||
|
|
||
| If the user says yes, invoke `/recce-mcp-e2e`. If a dbt project path was used earlier in this session, reuse it automatically; otherwise ask. | ||
|
|
||
| **What counts as "a round":** | ||
| - A tool handler added or modified + its unit tests pass | ||
| - Error classification logic changed + tests pass | ||
| - Single-env or response format changed + tests pass | ||
|
|
||
| **Do NOT ask** after: test-only changes, comment/doc edits, import reordering. | ||
|
|
||
| **This is separate from `tests/test_mcp_e2e.py`** — that file tests with DbtTestHelper + DuckDB in CI. `/recce-mcp-e2e` verifies all 8 tools against a real dbt project with a real database. | ||
|
|
||
| ## Pitfalls | ||
|
|
||
| - `sentry_sdk` import: `# pragma: no cover` on except (CI always has it) | ||
| - Python 3.9: `Union[X, Y]` not `X | Y` | ||
| - Pre-commit: black/isort may reformat — re-stage and commit | ||
| - `run.py` `schema_diff_should_be_approved()` try/except is intentional (ensures check creation) | ||
|
|
||
| ## File Map | ||
wcchang1115 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| `recce/mcp_server.py` (server + handlers), `recce/tasks/rowcount.py` (error indicators, RowCountStatus), `recce/run.py` (CLI preset), `recce/summary.py` (display logic), `recce/event/__init__.py` (Sentry) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| --- | ||
| name: recce-mcp-e2e | ||
| description: Use when MCP server code is modified and needs full E2E verification against a real dbt project. Triggers after changes to recce/mcp_server.py, MCP tool handlers, single-env logic, or error classification. Also use before merging MCP PRs. | ||
| --- | ||
|
|
||
| # MCP E2E Verification | ||
|
|
||
| Full-stack verification of all 8 MCP tools against a real dbt project. | ||
|
|
||
| ## When to Use | ||
|
|
||
| - After modifying `recce/mcp_server.py` or `_tool_*` handlers | ||
| - After changing single-env logic or error classification | ||
| - Before merging any MCP-related PR | ||
| - **Not for**: unit test changes only, frontend-only changes, docs-only changes | ||
|
|
||
| ## Usage | ||
|
|
||
| Invoke as `/recce-mcp-e2e` or `/recce-mcp-e2e <project_path>`. | ||
|
|
||
| - **With argument**: use the given path as the dbt project directory | ||
| - **Without argument**: ask the user for the dbt project path | ||
|
|
||
| The project directory must contain `target/manifest.json` and `target-base/manifest.json`. | ||
|
|
||
| ## Process | ||
|
|
||
| 1. **Resolve project path** from argument or user input | ||
| 2. **Validate** `target/` and `target-base/` exist with `manifest.json` | ||
| 3. **Detect recce source** — find the repo root containing `recce/mcp_server.py`. If `recce-nightly` is also installed (`pip show recce recce-nightly`), set `PYTHONPATH=<RECCE_REPO_ROOT>:$PYTHONPATH` | ||
| 4. **Generate** `test_mcp_e2e.py` in the project directory from `test_mcp_e2e_template.py` (in this skill directory). Replace `PROJECT_DIR_PLACEHOLDER` with the resolved absolute path. | ||
| 5. **Execute** with appropriate PYTHONPATH prefix | ||
| 6. **Report** results — all 13 checks must show PASS. Expected output: | ||
| ``` | ||
| === FULL MODE (8 tools) === | ||
| PASS lineage_diff: PASS | ||
| ... | ||
| === SINGLE-ENV MODE === | ||
| PASS row_count_diff (_warning): PASS | ||
| ... | ||
| ALL PASS | ||
| ``` | ||
| 7. **Clean up** — delete `test_mcp_e2e.py` | ||
|
|
||
| ## Quick Reference | ||
|
|
||
| | Test Suite | Checks | What's Verified | | ||
| |-----------|--------|----------------| | ||
| | Full mode (8 tools) | lineage_diff, schema_diff, row_count_diff, query, query_diff, profile_diff, list_checks, run_check | Non-empty results from each tool | | ||
| | Single-env _warning (3) | row_count_diff, query_diff, profile_diff | `_warning` field present with `SINGLE_ENV_WARNING` | | ||
| | Single-env no _warning (2) | lineage_diff, schema_diff | `_warning` field NOT present | | ||
|
|
||
| **Additional manual checks** (not in script): | ||
|
|
||
| | Check | Command/Action | | ||
| |-------|---------------| | ||
| | --help | `recce mcp-server --help` shows Prerequisites section | | ||
| | Server modes | Non-server mode: `list_tools` returns only lineage_diff + schema_diff | | ||
|
|
||
| ## Common Mistakes | ||
|
|
||
| | Problem | Fix | | ||
| |---------|-----| | ||
| | `ImportError: cannot import name 'SINGLE_ENV_WARNING'` | recce-nightly conflict — use `PYTHONPATH=<RECCE_REPO_ROOT>:$PYTHONPATH` | | ||
| | lineage_diff returns empty | Use `view_mode="all"` (default `changed_models` filters out unchanged) | | ||
| | list_checks returns empty | Preset checks from `recce.yml` must be loaded via `load_preset_checks()` — script handles this | | ||
| | `portalocker` FileNotFoundError on exit | Cosmetic thread error in event collector — does not affect results | | ||
| | Single-env test uses target-base | By design — `load_context` needs both, `single_env=True` flag simulates the mode | |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,143 @@ | ||
| """MCP E2E — temporary, delete after verification.""" | ||
|
|
||
| import asyncio | ||
| import json | ||
| import os | ||
| import sys | ||
|
|
||
| PROJECT_DIR = "PROJECT_DIR_PLACEHOLDER" | ||
| os.chdir(PROJECT_DIR) | ||
|
|
||
| TOOL_METHODS = { | ||
| "lineage_diff": "_tool_lineage_diff", | ||
| "schema_diff": "_tool_schema_diff", | ||
| "row_count_diff": "_tool_row_count_diff", | ||
| "query": "_tool_query", | ||
| "query_diff": "_tool_query_diff", | ||
| "profile_diff": "_tool_profile_diff", | ||
| "list_checks": "_tool_list_checks", | ||
| "run_check": "_tool_run_check", | ||
| } | ||
|
|
||
|
|
||
| def discover_model(manifest_path): | ||
| with open(manifest_path) as f: | ||
| manifest = json.load(f) | ||
| for uid, node in manifest.get("nodes", {}).items(): | ||
| if node.get("resource_type") == "model": | ||
| return node["name"] | ||
| return None | ||
|
|
||
|
|
||
| MODEL = discover_model(os.path.join(PROJECT_DIR, "target", "manifest.json")) | ||
| if not MODEL: | ||
| print("ERROR: No model found in manifest") | ||
| sys.exit(1) | ||
|
|
||
| TOOL_ARGS = { | ||
| "lineage_diff": {"select": MODEL, "view_mode": "all"}, | ||
| "schema_diff": {"select": MODEL}, | ||
| "row_count_diff": {"node_names": [MODEL]}, | ||
| "query": {"sql_template": f"SELECT count(*) as cnt FROM {{{{ ref('{MODEL}') }}}}"}, | ||
| "query_diff": {"sql_template": f"SELECT count(*) as cnt FROM {{{{ ref('{MODEL}') }}}}"}, | ||
| "profile_diff": {"model": MODEL}, | ||
| "list_checks": {}, | ||
| "run_check": None, # resolved after list_checks | ||
| } | ||
|
|
||
| WARNING_TOOLS = {"row_count_diff", "query_diff", "profile_diff"} | ||
| NO_WARNING_TOOLS = {"lineage_diff", "schema_diff"} | ||
|
|
||
|
|
||
| async def call_tool(server, name, args): | ||
| return await getattr(server, TOOL_METHODS[name])(args) | ||
|
|
||
|
|
||
| async def test_full_mode(ctx): | ||
| from recce.config import RecceConfig | ||
| from recce.mcp_server import RecceMCPServer | ||
| from recce.run import load_preset_checks | ||
|
|
||
| config_file = os.path.join(PROJECT_DIR, "recce.yml") | ||
| if os.path.exists(config_file): | ||
| config = RecceConfig(config_file=config_file) | ||
| preset_checks = config.get("checks", []) | ||
| if preset_checks: | ||
| load_preset_checks(preset_checks) | ||
|
|
||
| server = RecceMCPServer(ctx) | ||
| results = {} | ||
|
|
||
| for name, args in TOOL_ARGS.items(): | ||
| if name == "run_check": | ||
| continue | ||
| try: | ||
| result = await call_tool(server, name, args) | ||
| ok = result is not None and isinstance(result, (dict, list)) | ||
| results[name] = "PASS" if ok else "FAIL (empty)" | ||
| if name == "list_checks" and isinstance(result, dict): | ||
| checks = result.get("checks", []) | ||
| if checks: | ||
| TOOL_ARGS["run_check"] = {"check_id": checks[0]["check_id"]} | ||
| except Exception as e: | ||
| results[name] = f"ERROR: {e}" | ||
|
|
||
| if TOOL_ARGS["run_check"]: | ||
| try: | ||
| result = await call_tool(server, "run_check", TOOL_ARGS["run_check"]) | ||
| results["run_check"] = "PASS" if result else "FAIL" | ||
| except Exception as e: | ||
| results["run_check"] = f"ERROR: {e}" | ||
| else: | ||
| results["run_check"] = "SKIP (no checks in recce.yml)" | ||
|
|
||
| return results | ||
|
|
||
|
|
||
| async def test_single_env(ctx): | ||
| from recce.mcp_server import SINGLE_ENV_WARNING, RecceMCPServer | ||
|
|
||
| server = RecceMCPServer(ctx, single_env=True) | ||
| results = {} | ||
|
|
||
| for name in WARNING_TOOLS: | ||
| try: | ||
| result = await call_tool(server, name, TOOL_ARGS[name]) | ||
| has = "_warning" in result and result["_warning"] == SINGLE_ENV_WARNING | ||
| results[f"{name} (_warning)"] = "PASS" if has else "FAIL" | ||
| except Exception as e: | ||
| results[f"{name} (_warning)"] = f"ERROR: {e}" | ||
|
|
||
| for name in NO_WARNING_TOOLS: | ||
| try: | ||
| result = await call_tool(server, name, TOOL_ARGS[name]) | ||
| has = "_warning" in result if isinstance(result, dict) else False | ||
| results[f"{name} (no _warning)"] = "PASS" if not has else "FAIL" | ||
| except Exception as e: | ||
| results[f"{name} (no _warning)"] = f"ERROR: {e}" | ||
|
|
||
| return results | ||
|
|
||
|
|
||
| async def main(): | ||
| from recce.core import load_context | ||
|
|
||
| ctx = load_context(target_path="target", target_base_path="target-base") | ||
|
|
||
| print("=== FULL MODE (8 tools) ===") | ||
| full = await test_full_mode(ctx) | ||
| for k, v in full.items(): | ||
| print(f" {'PASS' if 'PASS' in v else 'FAIL'} {k}: {v}") | ||
|
|
||
| print("\n=== SINGLE-ENV MODE ===") | ||
| single = await test_single_env(ctx) | ||
| for k, v in single.items(): | ||
| print(f" {'PASS' if 'PASS' in v else 'FAIL'} {k}: {v}") | ||
|
|
||
| all_pass = all("PASS" in v for v in {**full, **single}.values()) | ||
| print(f"\n{'ALL PASS' if all_pass else 'SOME FAILED'}") | ||
| return 0 if all_pass else 1 | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| sys.exit(asyncio.run(main())) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.