MCP Server for DQX#1252
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1252 +/- ##
==========================================
+ Coverage 92.56% 92.75% +0.18%
==========================================
Files 102 102
Lines 10429 10429
==========================================
+ Hits 9654 9673 +19
+ Misses 775 756 -19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
✅ 799/799 passed, 42 skipped, 5h15m1s total Running from acceptance #5156 |
|
✅ 194/194 passed, 2 skipped, 6h54m24s total Running from anomaly #1270 |
There was a problem hiding this comment.
Review: MCP Server for DQX
Strong PR — the OBO + definer's-rights-view + SP-job architecture is the right pattern, and it neatly avoids needing a Jobs scope at the user auth level. Nice touches: the 4.5 MB notebook.exit() guard, pure-ASGI OBO middleware, idempotent setup grants, and a fixed OPERATIONS dispatch (no arbitrary code exec).
A few things to address before merge — inline comments below. Two summary-level points:
Rebase on main (stale base). The root pyproject.toml mypy-comment change reintroduces a reference to apx dev check, but PR #1223 (already merged) removed apx and replaced it with first-party scripts (bun run tsc -b + basedpyright). The branch is behind main; please rebase so this drift is resolved.
Add an integration test with Genie (see inline on the tests). app.py is explicitly tuned for "Genie Code compatibility" (stateless_http, json_response, CORS preflight) — but nothing exercises that path. An end-to-end test that drives the MCP server through Databricks Genie (Genie Code as the MCP client) over the deployed app would protect exactly the behaviour these settings exist for.
Priorities: the rebase are blockers; CORS and the in-memory run-state are important follow-ups.
mwojtyczka
left a comment
There was a problem hiding this comment.
Going in the right direction, left some comments to address
`make fmt` runs update_github_urls.py, which pins databrickslabs/dqx GitHub links in docs to the latest release tag. Apply it to the new Feature lifecycle page so the formatting check produces no diff. Co-authored-by: Isaac
The mcp workflow already wrote a .coveragerc and set COVERAGE_FILE but never combined or published the result. Add the combine + codecov-action steps the other integration jobs use (flag: mcp), so MCP integration/e2e coverage is reported like the rest of the suite. Co-authored-by: Isaac
The Databricks Apps front-door only accepts OAuth tokens (user U2M or SP M2M-with-secret), not the acceptance harness's metadata-service token — so the deployed app's /mcp endpoint returns 401 in CI even though the identity owns the app (a token-type limitation, not permissions). Other suites are token-free only because they never leave the SDK/control plane. Add an app_auth fixture that mints an OAuth M2M bearer from DQX_MCP_APP_CLIENT_ID / DQX_MCP_APP_CLIENT_SECRET (an SP with CAN_USE on the app, provisioned in the acceptance vault) for the /mcp calls, while deploy + Model Serving keep using the control-plane bearer. MCP-specific env names avoid changing other suites' SDK auth. Falls back to ambient auth (OAuth profile) locally. Until the secret is provisioned, wait_until_ready turns the 401 into a clear skip rather than a 180s hard failure. Co-authored-by: Isaac
…a dedicated SP Governance / review feedback: - Enforce the caller's UC permissions before the runner job (OBO pre-checks): load_checks table reads route through a definer's-rights OBO view; file reads and writes are verified as the caller (owner-or-create-probe for tables, upload-probe for files) and distinguish no-access (deny) from not-found (allow create). - Narrow credentialed CORS from the multi-tenant *.databricksapps.com wildcard to an exact allow-list from DATABRICKS_HOST (+ DQX_MCP_EXTRA_CORS_ORIGINS). - setup.py: validate users_group / schema_name before SQL interpolation. - generate_rules_from_contract is deterministic-only (drop process_text_rules; the runner has no [llm] extra); normalize workspace contract paths to the cluster FUSE form. - get_run_result: structured 'not_found' for invalid/expired/foreign run_ids; surface the failed task's real error instead of a generic message. - Add the 'workspace.workspace' OBO scope for workspace-file backends. Runner identity (run the job as a least-privilege SP, not the deployer): - Convert the runner from a notebook_task to a python_wheel_task (new dqx-mcp-runner wheel), so its run_as SP needs no workspace-object ACLs (mirrors the DQX Studio task runner). Results are written to a UC volume (<catalog>.tmp.mcp_results) and read back by the app via the Files API (no SQL warehouse). - Run the job as run_as = runner_service_principal_id (a dedicated workspace SP; required). - setup.py: keep the deploy principal as tmp-schema owner (so it can create the results volume), grant the app SP + runner SP MANAGE (temp-view cleanup) and volume READ/WRITE, and grant the app SP CAN_MANAGE_RUN on the runner job. Tests: 78 -> 104 unit tests; black/ruff clean. Docs (dqx_mcp_server.mdx) updated for the new architecture (two SPs, wheel task, results volume, OBO enforcement, deploy prereqs). Co-authored-by: Isaac
- ci_deploy.sh: require + pass runner_service_principal_id (the runner job's run_as SP) so the integration/CI deploy works with the dedicated-runner-SP change — without it, bundle deploy would try the placeholder SP and fail. Validated: deploy succeeds and the app starts as a wheel-task, runner-SP app. - docs: fix the Architecture mermaid sequence diagram — ';' is a Mermaid statement separator, so "app SP submits; job runs as the runner SP" broke parsing (use ','). Co-authored-by: Isaac
…nstall)
The runner job runs as a least-privilege SP, which cannot read the runner wheel in the
deployer's workspace bundle folder — so the serverless env failed at library install with
"library file does not exist or no permission". setup now grants the runner SP CAN_READ on
the bundle artifact path (${workspace.artifact_path}), which propagates to the wheel.
Also corrects the earlier docs/comments that claimed a wheel task needs no workspace ACLs:
it avoids the notebook-object ACL, but the run_as SP still needs read on the wheel file.
Validated: the runner job installs the wheel and runs as the runner SP.
Co-authored-by: Isaac
…ecret Addresses Marcin's review comments on the MCP server plus a round of Genie feedback from driving the tools end to end. Write governance (#1-#3): the persisting tools no longer take a caller-supplied destination. save_checks / apply_checks_and_save_to_table now take a bare output name and the runner writes it into the caller's own SP-owned per-user schema (dqx_mcp_<user>), granting only that caller access. This lets me delete the OBO write pre-check entirely - it checked the caller's perms while the SP did the write (false assurance), rejected MODIFY-granted non-owners, and classified UC errors by string-matching. The SP now only ever writes where it owns. Runner robustness (#5, #6): save_checks applies write mode via config.replace() instead of attribute assignment (which silently no-op'd on file backends), with an up-front append/overwrite whitelist; generate_rules validates each profile and raises a clear InvalidParameterError on a missing name/column instead of a KeyError deep in the runner. Correctness & CI (#4, #7, #8): execute_sql follows next_chunk_index so wide schemas and the temp-view sweep aren't silently truncated to the first chunk; added basedpyright (make mcp-check, wired into push.yml) and fixed the type errors it surfaced; pointed the integration-coverage source at the MCP packages rather than the DQX library. Docs (#9): moved deploy/prerequisites into the installation page and linked back. Genie feedback (MCP-side wrappers): generate_rules_from_contract now accepts inline contract_content (or a file), reads it as the caller and stages a copy to a runner-readable volume - so a Workspace-file contract works even though the runner SP has no access to it; list_available_checks takes a filter substring so agents can search instead of scanning every entry; generate_rules flags that its bounds are data-derived and should be reviewed. Dropped the catalog secret: a UC catalog name isn't sensitive (access is governed by UC grants, not by hiding the name), so the app reads DQX_CATALOG as a plain config value set from the catalog_name deploy var (inline app config, no app.yaml) - nothing to create or manage out of band. Also fixed make mcp-integration for local runs (absolute UV_BUILD_CONSTRAINT, mirroring mcp-deploy) and updated the integration test + fixture for the new tool shapes. Extracted the runner's pure naming/validation helpers into naming.py so they're unit-tested without Spark.
The build job's make fmt tripped on the end-to-end integration test: - pylint R0915 (too-many-statements 52/50) after my save/load/apply edits - extracted the persisting-tools steps into a _assert_persisting_tools helper, which also reads better. - mypy flagged the nested index into EXPLICIT_CHECKS - annotated it as list[dict] so the check-function comprehensions type-check. Ran black/ruff/mypy/pylint (make fmt equivalent) locally; all clean.
Second round of Marcin's review comments, plus a one-command teardown. Cross-user result disclosure (IDOR): get_run_result read mcp_results/<run_id>.json as the app SP and returned it to whoever called, checking only that the run belonged to the runner job — not that the caller submitted it. Run-ids are guessable and every user has CAN_MANAGE_RUN, so user B could read user A's payload (incl. sampled rows of A's governed table). Now submit_job_async stamps the caller (OBO email) as a requesting_user job parameter, and get_run_status rejects a run whose requesting_user doesn't match the caller as not_found - before reading state or the result file. Temp-view leak: the OBO temp view was created before submit_job_async, so a submission that threw after creation (unset job id, run_now error, throttled sweep) left the view in the shared tmp schema until the TTL sweep. Added _submit_or_drop_view which drops the view as the caller on failure; used in profile_table, run_checks, load_checks, and apply_checks_and_save_to_table. Slow-statement handling: execute_sql now polls get_statement to completion instead of raising when a statement is still PENDING/RUNNING after the 30s wait window (cold-start warehouse, very wide table). make mcp-destroy: one-command teardown (app + jobs + runner-wheel volume). Nothing in the bundle is destroy-protected, so unlike the Studio's multi-step unbind-then-destroy uninstall this is a single command. Leaves the tmp + per-user output schemas intact. Also: ci_deploy.sh now deploys the app via `bundle run` (not raw `apps deploy`), which broke when app.yaml was removed in favour of the bundle's inline config; docs now give a cross-platform Databricks-CLI deploy path (Windows / no make) alongside the make wrapper, with the catalog and mcp-destroy documented; fixed stale databricks.yml comments.
Fixes from a review pass over the previous two commits. - Reject leading-digit output names. IDENTIFIER_RE / validate_output_name accepted e.g. "2024_clean", but the FQN is interpolated UNQUOTED into OutputConfig.location / spark.table, where a digit-leading identifier is a SQL parse error - and the per-user schema + grant (backticked) are created first, so the run half-applied. Now require a leading letter/underscore, rejected up front. - Harden the run-result IDOR guard: deny when the recorded submitter OR the caller is empty, so an unowned run (empty requesting_user) can't be read by an empty-identity caller (previously "" == "" passed). - execute_sql: guard result.status before reading .error so a status=None edge state raises a clean RuntimeError instead of AttributeError. - Runner: log a warning when the caller principal is rejected by PRINCIPAL_RE (schema created but no grant) instead of silently locking the caller out; and report access_granted_to only when EVERY created table was granted (a partial output/quarantine grant no longer overstates access). - Cleanup: removed the now-dead to_local_fuse_path (contract reads stage to a volume now); added the missing type annotations flagged by AGENTS.md (_to_dq_profile return, _submit_or_drop_view obo_ws); fixed a stale Makefile comment that still referenced the removed catalog secret. - Tests: unit coverage for leading-digit rejection, the empty-submitter/empty-caller IDOR denials, and the status=None SQL path; integration assertions that an invalid output_name is rejected and an unknown run_id returns not_found. Integration suite passes end to end.
- .github/workflows/mcp.yml: pass DQX_MCP_RUNNER_SERVICE_PRINCIPAL_ID (from the TOOLS_CLIENT_ID secret - a workspace SP with workspace + consumer access) to the acceptance harness, so ci_deploy.sh has the runner job's run_as identity and the MCP integration deploy no longer fast-fails with 'DQX_MCP_RUNNER_SERVICE_PRINCIPAL_ID is not set'. - tests/integration_mcp/test_mcp_server.py: drop the now-unused 'import pytest' (the invalid-output-name check uses try/except, not pytest.raises). This is what 'make fmt' (ruff --fix F401) was rewriting, failing the build job's diff check.
mwojtyczka
left a comment
There was a problem hiding this comment.
Re-review of the current head (bdc8f4b) after the fix commits. All three findings from the prior review are verified fixed and covered by regression tests — I've resolved those threads.
- IDOR / cross-user result disclosure —
submit_job_asyncnow stampsrequesting_user(from the OBOX-Forwarded-Email) as a declared job parameter, andget_run_statusreads it back and denies the read (not_found, no result download) unless caller == submitter, with an explicit empty-either-side guard. Verified against the SDK (Run.job_parametersround-trip) and the guard is correctly positioned before the result download / RUNNING branch. Tests:test_denies_run_submitted_by_another_user,test_denies_when_caller_has_no_identity,test_denies_when_run_has_no_submitter,test_allows_run_submitted_by_same_user(all assertfiles.downloadis not called on deny). Because submit and get_run_result both traverse the same OBO middleware, the guard is symmetric — no legitimate-caller lockout. - Temp-view leak on submit failure — new
_submit_or_drop_viewwrapper drops the OBO view on failure across all view-backed tools; file-backedload_checkscorrectly skips it. Test:test_drops_temp_view_if_submit_fails. execute_sql30s-timeout misreport — now pollsget_statementto completion (2s/120s) and hardens the None-status deref;get_statementreturns the sameStatementResponse(manifest+result populated on SUCCEEDED), so the chunk-follow stays valid. Test:test_none_status_raises_clean_error_not_attributeerror.
Proactive hardening in the same commits also checks out: leading-digit identifier rejection (unquoted-FQN safety), partial-grant reporting (access_granted_to null unless every created table was granted), and the removed dead to_local_fuse_path. The changed deploy/CI/docs (requesting_user declared at job level, ci_deploy.sh threads the runner SP, mcp.yml stays pull_request + fork-gated, docs anchors resolve) are all clean.
No new findings on this head. Remaining open threads are the two I intentionally left for the authors (the docs-placement suggestion) — the type-gating thread was resolved separately once mcp-check landed in CI.
…as 403) Pinning the runner job's run_as to a separate SP (TOOLS_CLIENT_ID) made the CI bundle deploy fail with 403 PERMISSION_DENIED: the bundle deployer is a different SP than TOOLS_CLIENT_ID and lacks servicePrincipal.user on it (the 403 itself proves they are not the same identity). ci_deploy.sh now leaves DQX_MCP_RUNNER_SERVICE_PRINCIPAL_ID optional and, when unset, resolves run_as to the deploying identity via 'databricks current-user me'. run_as then equals the job's creator, which needs no servicePrincipal.user grant — the same pattern as the demo asset-bundle test and the SDK-created integration jobs. A real deploy still passes a dedicated least-privilege runner SP explicitly. Also add front-door RCA diagnostics: when the app's /mcp front-door returns 401/403, the skip now surfaces the decoded bearer *claims* (never the raw token/signature) and the redacted response (WWW-Authenticate, Location, body) so we can determine whether the token is an opaque metadata token or an OAuth JWT missing the app's user_api_scopes. Token-shaped substrings are masked before logging.
The MCP integration suite drives the server in the deployed Databricks App and the runner in a job — both run *remotely*, not in the pytest process — so local `coverage` records nothing for mcp-server/server and mcp-server/runner. The workflow was still uploading those modules to Codecov, which reported them at 0% and dropped the PR from ~92.8% to ~85.9% project coverage with 0% patch coverage. Remove the coverage config/merge/publish steps so the MCP modules are no longer reported at a false 0%. This matches the sibling DQX Studio app (`make app-test` runs in CI with no Codecov upload) and the e2e job test (no coverage) — the repo only uploads coverage for suites whose code runs in-process (unit / integration / anomaly). The MCP unit and integration tests still run in CI; only the misleading coverage upload is dropped.
Changes
Adds an MCP (Model Context Protocol) server for DQX, exposing DQX's data-quality
capabilities as tools that any MCP-compatible AI agent (Claude, Genie Code, Cursor, Mosaic AI)
can discover and orchestrate. It runs as a Databricks App with on-behalf-of (OBO)
authentication, so all data access is governed by the calling user's Unity Catalog permissions.
Architecture
no PySpark/DQX dependency.
over the source table using the user's forwarded token (
X-Forwarded-Access-Token), so theservice principal reads data as the user, never directly.
job and return a
run_idimmediately; the client pollsget_run_result.get_run_statusdoesa single non-blocking poll (the client drives cadence) — this matches MCP's canonical
long-running-tool model and avoids holding the HTTP connection / saturating the worker pool.
stateless_http+json_response+ CORS preflight scoped toDatabricks domains.
Tools
get_workflowget_table_schemaprofile_tablegenerate_rulesgenerate_rules_from_contractvalidate_checksrun_checksapply_checks_and_save_to_tablesave_checks/load_checkslist_available_checksget_run_resultTemp-view lifecycle (stateless, restart/replica-safe)
finally(it runs as the SP, which owns thetemp schema — see setup), so cleanup happens in the guaranteed job execution regardless of
whether/where the user polls.
v_<epoch>_<uuid>view names) reaps any orphans whosejob never ran. No per-request server state is kept, so app restarts / multiple replicas don't
leak views or lose context.
Security & governance
tmpschema (object lifecycle), not the underlying data.and backtick-quoted; log values sanitized (CWE-117); catalog name sourced from a Databricks secret.
Deployment
databricks.yml): runner job, one-time setup job (UC grants + schemaownership), and the app.
requirements.txt(the App's runtime manifest) andpyproject.tomlarekept in sync; the runner installs
databricks-labs-dqx[datacontract].maketargets andfmt/CI wiring for the sub-project.Linked issues
Resolves #1045
Tests
Layered MCP-server test suite (62 passing, deterministic, no workspace needed for CI):
test_tools.py)Clientover the real MCP protocol (test_mcp_protocol.py)initialize/tools-list, and OBO tokenpropagation (
test_app_http.py); CORS policy (test_cors.py)endpoint): a tool-calling model is handed the tool schemas and an instruction, and we assert it
discovers and invokes the right tools (
test_integration_agent.py)Documentation and Demos
docs/dqx/docs/guide/dqx_mcp_server.mdx)This description was written by Isaac.