feat: implement MCP Roots protocol for dynamic allowed-directories#95
Open
nicholasjayantylearns wants to merge 11 commits into
Open
feat: implement MCP Roots protocol for dynamic allowed-directories#95nicholasjayantylearns wants to merge 11 commits into
nicholasjayantylearns wants to merge 11 commits into
Conversation
Contributor
|
✅ DCO Check Passed Thanks @nicholasjayantylearns, all your commits are properly signed off. 🎉 |
Contributor
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
Author
|
Marking this draft. Accurate update on what was verified and what wasn't. What works (verified live on macOS + Claude Desktop)
What's blocked from "ship" (not introduced by this PR)The single-document conversion path ( What was ruled out as a workaround for this PR:
Proposed path forward: an eager-model-preload option at server startup, behind a CLI flag, so cold-start happens during server boot (where Claude Desktop's startup timeout is more generous) instead of during the first user-facing tool call. Tracked separately at #98. Why draft, not mergedThis PR was filed for issue #86 (Roots support). Roots are functional and unit-tested. But the user-facing workflow — install via uvx, ask Claude Desktop to parse a PDF in an authorized folder — doesn't complete cleanly inside the MCP timeout. Shipping the PR while the workflow doesn't work would misrepresent what it delivers, even though the cold-start cause is outside this PR's scope. Marking draft, filed the cold-start issue at #98, and proposing this PR re-opens to "ready for review" once either (a) the cold-start has a viable fix that lives alongside it, or (b) the maintainers prefer to merge this independently and track cold-start in its own ticket. Happy with either path — your call. cc maintainers from |
Author
Update — live verification passed, moving out of draftThe blocker described in the previous "marking draft" comment was addressed in subsequent commits ( End-to-end live test on macOS, 2026-05-13
Boot breadcrumbs from the server log confirm the full warm-up path executed before any tool call: 35/35 unit tests still passing on macOS (33 from earlier plus 2 wiring tests for the Both halves of the original blocker (Roots + cold-start) are now functional in a single branch. Acceptance criteria
Independent finding worth noting (out of scope for this PR)On image-heavy PDFs with stylized fonts over graphics, Docling's OCR layer produces hallucinated text in image-overlay regions. Reproduces against #98 (cold-start timeout) is also addressed by the cc @cau-git @dolfim-ibm @vagenas @cberrosp @PeterStaar-IBM @Zaalouk for review when convenient. |
Member
|
@nicholasjayantylearns lovely, thanks for the PR! |
PeterStaar-IBM
approved these changes
May 14, 2026
Member
|
@nicholasjayantylearns please run,
|
Introduce the server-side state that will back MCP Roots support. * docling_mcp/_path_utils.py — discriminator between remote URLs (http, https, ftp, ftps, s3) and filesystem-style sources (plain paths and file:// URLs). Roots authorize filesystem access only. * docling_mcp/roots.py — AllowedRootsRegistry with two layers: static_roots (seeded once from --allowed-directories) and client_roots (refreshed from roots/list_changed notifications). Client roots, when present, fully replace the static set, per the canonical MCP semantics in the modelcontextprotocol/servers filesystem reference. No callers wired up yet — this commit is the registry alone, smoke-tested in isolation. Next commits wire it into the server handshake and the conversion tools. Refs: docling-project#86 Signed-off-by: nicholasjayantylearns <nicholasjayantylearnswithyou@gmail.com>
Both convert_document_into_docling_document and convert_directory_files_into_docling_document now call allowed_roots.validate_source() before touching the filesystem. Remote URLs (http/https/ftp/etc.) pass through unchecked; local paths must lie under an active root. When neither --allowed-directories nor client-sent roots have populated the registry, validate_source is a no-op — preserves the pre-roots behavior for users who have not opted into the protocol. Refs: docling-project#86 Signed-off-by: nicholasjayantylearns <nicholasjayantylearnswithyou@gmail.com>
* docling_mcp/_roots_wiring.py — installs handlers for notifications/initialized (seeds registry on connect) and notifications/roots/list_changed (refreshes on change). The MCP Python SDK's notification dispatcher does not propagate the active ServerSession to handler callables; this module monkey-patches Server._handle_message to expose the session through a ContextVar so notification handlers can issue server-to-client list_roots requests. The patch is idempotent. * docling_mcp/servers/mcp_server.py — adds --allowed-directories Typer option, seeds allowed_roots.set_static_roots() before mcp.run(), and invokes install_roots_handlers() at startup. Backward compat: when neither --allowed-directories nor client-sent roots are present, the registry is unconstrained and validate_source() is a no-op, matching the pre-roots behavior. Refs: docling-project#86 Signed-off-by: nicholasjayantylearns <nicholasjayantylearnswithyou@gmail.com>
29 tests across three layers: * _path_utils: is_remote_url discriminator (6 schemes including case-insensitive HTTPS), to_filesystem_path handling for plain paths, file:// URLs, and rejection of remote URLs. * AllowedRootsRegistry: unconstrained default, static seeding, inside/ outside path validation, remote URL passthrough when constrained, client-roots-replace-static semantics (the canonical MCP rule), clear_client_roots restoring static, non-file:// URI filtering, symlink resolution preventing bypass, and ../-resolution preventing escape. * _roots_wiring: install_roots_handlers registers both notification types, is idempotent across multiple invocations, _on_initialized seeds from session.list_roots() when client advertises roots capability, skips list_roots() when client does not, and _on_roots_list_changed refreshes on subsequent notifications. End-to-end against a live MCP client (Claude Desktop / Claude Code via uvx) is documented in the PR description and run manually — these unit tests cover the protocol-correct behavior in isolation. Refs: docling-project#86 Signed-off-by: nicholasjayantylearns <nicholasjayantylearnswithyou@gmail.com>
New 'Filesystem access (MCP Roots)' section explains the three states the server can be in: * Roots-capable client connected — allowed paths are scoped to the user's client-side authorization, refreshed on list_changed. * Static --allowed-directories fallback for non-Roots clients. * No flag, no client roots — legacy unconstrained behavior preserved for backward compat. Refs: docling-project#86 Signed-off-by: nicholasjayantylearns <nicholasjayantylearnswithyou@gmail.com>
Signed-off-by: nicholasjayantylearns <nicholasjayantylearnswithyou@gmail.com>
Real-world clients (Claude Desktop, Cowork) emit file:// roots with
URL-encoded characters — most commonly file:///Users/x/Library/Application%20Support/...
on macOS where the directory name contains a space. urlparse(uri).path
preserves the percent-encoding, so storing parsed.path directly meant the
registry held '/Library/Application%20Support/...' while the real
filesystem path was '/Library/Application Support/...' (literal space).
The two strings never matched in Path.relative_to(), so validate_source
rejected every path inside any space-containing authorized root.
Fix: wrap parsed.path in urllib.parse.unquote() in both
update_from_client_roots() (registry seeding) and to_filesystem_path()
(per-call validation). The conversion now happens once at the seam
between URL space and filesystem space; downstream code sees real paths
exclusively.
Symptom that motivated the fix:
"path 'X/Library/Application Support/Y' is not under any allowed
root. active roots: ['X/Library/Application%20Support/Y']"
Visible because the registry and the validator disagreed on encoding for
exactly one character. Reproduced live with Claude Desktop on macOS
(uploads folder).
Tests: four new regression cases —
* to_filesystem_path decodes %20
* to_filesystem_path handles colons + parens via %3A
* registry accepts a real-space path when the client sends %20
* active_roots() exposes decoded paths so error messages are readable.
29 -> 33 tests passing.
Refs: docling-project#86
Signed-off-by: nicholasjayantylearns <nicholasjayantylearnswithyou@gmail.com>
Adds an opt-in CLI flag that triggers the @lru_cache-backed _get_converter() call once at server startup so the cold-start cost (PyTorch import, OCR model load, layout detector init) is paid during the MCP server-spawn window — which Claude Desktop honors with a more generous timeout than its per-request timeout. Without this flag, the very first call to convert_document_into_docling_document hits Claude Desktop's 60-second per-request MCP timeout because Docling's first invocation has to set up PyTorch + models. Symptom: 'MCP error -32001: Request timed out'. Reproduces against main; not specific to this PR's Roots work. This is the proposed fix for docling-project#98. It's gated on ToolGroups.CONVERSION being loaded so the warm-up doesn't fire when conversion tools are disabled. Default: False. Preserves the existing lazy-init behavior for users who don't opt in. Recommended in the README for interactive clients (Claude Desktop, Claude Code) where the per-request timeout matters. Tests: 2 new wiring smoke tests verify the flag is registered with Typer and defaults to False. End-to-end timing is left to manual verification on each client (Docling's load time is hardware-dependent and not deterministic). Refs: docling-project#98 Refs: docling-project#86 Signed-off-by: nicholasjayantylearns <nicholasjayantylearnswithyou@gmail.com>
Visibility-first response to 'Could not attach to MCP server' failure mode reported during live testing. Three behavior changes: 1. Add _crumb() helper that writes flushed stderr breadcrumbs at every boot step (main() entered, logger configured, preload scheduled, thread started, _get_converter() returned, pipeline init begin/end, READY or FAILED, mcp.run() called). Goes around Python logging buffering so output is visible in Claude Desktop's log file as each step actually executes. 2. Move --preload-models warm-up from foreground (which blocked mcp.run() and could exceed Claude Desktop's spawn timeout, causing 'Could not attach') to a daemon background thread. The MCP server handshake now completes fast regardless of model load duration. First convert call may block briefly on @lru_cache lock if it arrives mid-warmup, but the server is reachable from second one. 3. Inside the warm-up thread, explicitly call converter.initialize_pipeline(InputFormat.PDF) and initialize_pipeline(InputFormat.IMAGE) (when available) to actually force model load — DocumentConverter.__init__ is lazy on its own. Falls through to partial warm-up if Docling version lacks the method. The original synchronous, opaque preload is replaced. Tests still pass (the wiring tests check the flag exists and defaults to False, which they still do). Refs: docling-project#98 Signed-off-by: nicholasjayantylearns <nicholasjayantylearnswithyou@gmail.com>
…C240) Per maintainer review request to run pre-commit. Behavior unchanged; this commit is pure cleanup of style/lint issues across the PR's diff: - ruff format on the four files that needed reformatting - I001 import-block sort fixes (auto-applied by ruff --fix) in tests - D205 (blank line between summary and description) in the _crumb() docstring in mcp_server.py - E402 (module-level imports must be at top): reordered mcp_server.py so all imports come before _crumb()'s definition - ASYNC240 (sync os.path.realpath calls inside async test bodies): applied # noqa with rationale on the two test assertions where this is intentional unit-test hygiene, and on a pre-existing conversion.py iterdir() call that this PR's diff caused ruff to re-check All 35 unit tests still pass. Refs: docling-project#95 (review feedback) Signed-off-by: nicholasjayantylearns <nicholasjayantylearnswithyou@gmail.com>
…ndex imports Two follow-up mypy fixes for the cleanup pass: * docling_mcp/_roots_wiring.py: type-annotate the _handle_message_with_session monkey-patch wrapper as Any across the board. The function is a pass-through; coupling to the MCP SDK's internal Server._handle_message signature would be brittle, so Any matches the level of precision. Resolves [no-untyped-def]. * pyproject.toml: add 'llama_index.*' to mypy.overrides ignore_missing_imports, alongside the existing llama_stack_client.* entry. llama-index is an optional extra (llama-index-rag), not pinned in the base install, so mypy can't always find its stubs. Refs: docling-project#95 (review feedback) Signed-off-by: nicholasjayantylearns <nicholasjayantylearnswithyou@gmail.com>
nicholasjayantylearns
force-pushed
the
feat/mcp-roots-protocol
branch
from
5b57c8d to
2268b08
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What
Implement the MCP Roots protocol so that
docling-mcphonors the allowed-directories the client (Claude Desktop, Claude Code, etc.) authorizes at runtime viaroots/list_changed, with a static--allowed-directoriesCLI fallback for clients that don't support Roots.Closes the Roots half of #86. (PR #88 fixes the tensor-padding crash; this PR fixes the path-resolution failure for clients that send Roots, which is the other reason the issue is open.)
Why
Per the canonical MCP filesystem reference at
modelcontextprotocol/servers/tree/main/src/filesystem:Today, when a client like Claude Desktop sends
notifications/roots/list_changedtodocling-mcp, the server does not subscribe to it. Path resolution then fails for every path the client considered authorized — even when the user has explicitly granted access in the client UI. Multiple users in #86 hit this.This PR brings
docling-mcpin line with that reference behavior, in idiomatic Python on top of the existing FastMCP-based server, with no new top-level dependencies (the MCP SDK already pinned at>=1.9.4exposes everything we need).How
Five commits, each independently reviewable:
feat(roots): add AllowedRootsRegistry + URL-vs-path utilities— pure, dependency-free state and helpers.AllowedRootsRegistrykeeps two layers (static seed + client-provided) and exposesvalidate_source(source: str)that raisesPermissionErrorfor filesystem paths outside the active root set. Remote URLs (http://,https://,ftp://,s3://, …) pass through unchecked — Roots authorize filesystem access, not network access.file://URLs are treated as filesystem paths.feat(roots): authorize source paths in conversion tools— wirevalidate_sourceinto bothconvert_document_into_docling_documentandconvert_directory_files_into_docling_document. When the registry is unconstrained (no static flag, no client roots), it's a no-op — preserves the current behavior for users who haven't opted in.feat(roots): wire notification handlers + --allowed-directories CLI— adds the Typer flag and registers the two notification handlers on the underlying low-levelServer:notifications/initialized→ seeds the registry by callingsession.list_roots()if the client advertised the roots capability.notifications/roots/list_changed→ refreshes the registry the same way.The MCP Python SDK's notification dispatcher does not propagate the active
ServerSessionto handler callables, so this commit also installs a tiny ContextVar-based shim that captures session per-message inServer._handle_message. The patch is idempotent.test(roots): cover registry, path utils, and notification handlers— 29 unit tests covering: URL/path discriminator (incl. case-insensitive schemes), inside/outside path validation, remote-URL passthrough when constrained, the canonical "client roots fully replace static set" semantics, clear-then-restore-static, non-file://URI filtering, symlink resolution (preventing bypass),..-resolution (preventing escape), idempotent handler installation, and async handlers refreshing the registry against fake sessions.docs(roots): document MCP Roots support and --allowed-directories— new "Filesystem access (MCP Roots)" section in the README explaining the three states (Roots-capable client / static fallback / unconstrained legacy) and a concreteuvxinvocation example.Acceptance criteria checklist
docling-project/docling-mcpmain.modelcontextprotocol/servers/tree/main/src/filesystem— same "client roots completely replace static dirs" semantics, same notification subscription model.--allowed-directoriesbackward-compat path is wired and unit-tested.do_table_structure=True. (Pending — see follow-up comment.)--allowed-directoriesand a non-Roots-capable client successfully parses a PDF under the allowed dir and rejects a path outside it. (Pending — see follow-up comment.)The three live tests will be reported in a follow-up PR comment with screenshots / log excerpts so reviewers don't have to take it on faith.
Backward compatibility
validate_sourceis a no-op when the registry is unconstrained.--allowed-directoriesas a first-class CLI flag.Notes for reviewers
_roots_wiring.pyexists because the MCP Python SDK doesn't currently propagate session into notification handlers. If a future SDK release exposes a public hook for this, the shim should be replaced — there's a comment to that effect in the file.rootsis a client capability in the MCP spec; the server consumes it.sourceparameter. The validator only gates filesystem-style sources; this is documented in the new README section and in the registry's docstring.Provenance
Authored as a deliverable for an upstream-tracking ticket (
SKILLS-4) in thewearethehumansintheloopHITL project. The framing motivation is V-tax compression at the document-ingestion seam — every blueprint, PDF, screenshot, or structured doc that reaches a Docling-MCP-equipped Claude session today pays the V-tax independently because path resolution silently fails. With this fix, structure flows once and downstream consumers inherit it.