feat(mcp): host deployment-scoped and organization-scoped MCP servers#2207
Draft
hari-kuriakose wants to merge 11 commits into
Draft
feat(mcp): host deployment-scoped and organization-scoped MCP servers#2207hari-kuriakose wants to merge 11 commits into
hari-kuriakose wants to merge 11 commits into
Conversation
Exposes an Unstract API deployment to coding agents over the Model Context
Protocol, so an agent can run document extraction as a tool call instead of
hand-rolling HTTP requests.
Follows the hosted-MCP pattern used in the mfbt backend: a hand-rolled
JSON-RPC 2.0 endpoint mounted in the existing app (not a separate service),
with a declarative tool registry behind it.
Endpoint mirrors the deployment's own REST URL and reuses its API key, so
there is no second credential to mint or revoke:
POST /deployment/api/<org_name>/<api_name>/ # REST
POST /mcp/<org_name>/<api_name>/ # MCP
Tools: readMeFirst, getApiInfo, extractDocument, getExecutionStatus.
Auth goes through the same DeploymentHelper validation the REST endpoint
uses, and execution through ExecutionRequestSerializer, so the MCP surface
cannot drift from the REST one on who is allowed in or what input is valid.
All auth failures answer identically (401, no detail) so the endpoint cannot
be used to enumerate deployment names.
OAuth 2.1 with dynamic client registration is deliberately not implemented;
bearer auth covers Claude Code and API clients, and OAuth would be additive.
Tests: 20 passing (protocol/dispatch in the unit tier, auth boundary in the
integration tier). Registers the mcp-server-auth critical path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The transport and auth tests never reached the extraction helpers on a successful call, so the mapping from tool kwargs into execute_workflow was executed by nothing — a renamed kwarg would have passed every test and failed on the first real extraction. Adding that coverage surfaced three real problems: - Tool descriptions promised documents could be passed as any reachable URL. The execution serializer accepts *only* S3 pre-signed URLs, so an agent following the description would have failed every call. Descriptions, JSON schema and README now state the S3 requirement. - The tags schema advertised an unbounded array; the serializer caps it at one. Both limits are now sourced from the serializers rather than restated, so the advertised schema tracks what is actually enforced. - Documents were downloaded before the rate-limit slot was taken, so a call about to be rejected still pulled every document over the network. The slot is now acquired first, with the fetch inside the try block so a failed fetch still releases it. Also converts a RateLimitExceeded raised from deeper in the stack into an agent-readable message instead of letting it reach the generic "failed unexpectedly" branch. Tests: 34 passing (was 20). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The `_v2` suffix in this codebase marks apps carried through the v0.93.0 v1->v2 multitenancy data migration (see backend/migrating/v2/README.md), not current naming convention. Every app added since — pg_queue, dashboard_metrics, platform_api, configuration — is unsuffixed. A brand-new app with no v1 predecessor should not claim the marker. Named `mcp_server` rather than plain `mcp` because `mcp` is also the PyPI package name for the MCP SDK (which mfbt's server uses). A bare `mcp` app shadows it on the import path: `import mcp` resolved to the Django app, so a later `pip install mcp` would break silently. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ment one
The deployment MCP server can only ever expose one workflow: its credential is
an API deployment key, which resolves to no user and grants nothing beyond that
deployment. Reaching the rest of the platform needs a different credential —
and Unstract already has one.
PlatformApiKey (backend/platform_api/) is an org-scoped bearer key that
CustomAuthMiddleware resolves to a service-account User, validating the key's
organization against the URL and rejecting unknown, inactive and wrong-org keys
before any view runs. So the new server does not authenticate at all; it is
mounted where that middleware can do it.
That placement is the security-critical detail. WHITELISTED_PATHS is matched
with startswith, so the deployment server's /mcp/ prefix is exempt from the
middleware while the tenant path is not:
POST /mcp/<org>/<api_name>/ deployment key, view authenticates
POST /api/v1/unstract/<org>/mcp/ platform key, middleware authenticates
Moving the latter under the whitelisted prefix would silently remove all
authentication, so a test asserts it is not whitelisted.
Because that auth lives in middleware, the platform auth tests go through
django.test.Client and the real URL. APIRequestFactory — correct for the
deployment server, which owns its auth — bypasses middleware entirely and would
have passed against a completely open endpoint.
Shared JSON-RPC transport extracted to transport.py so the two servers cannot
drift on protocol behaviour; the deployment server's URL, behaviour and 34
tests are unchanged.
The platform tools are read-only by design (readMeFirst, whoami,
listApiDeployments, listWorkflows, listPromptStudioProjects). Listings go
through each model's for_user manager so platform sharing rules apply, and are
capped with an explicit truncation note rather than silently cut.
Two constraints are documented rather than worked around:
- A read-tier key cannot use the server at all: the middleware gates tiers on
HTTP method and every MCP call is a POST. Special-casing MCP paths in shared
middleware is a maintainer decision, not this app's to make.
- Service accounts bypass per-user sharing (for_user returns self.all()), so
listings cover the whole org. whoami and readMeFirst both say so, since an
agent assuming otherwise would misread a listing.
check_tool_allowed refuses write tools below read_write, and a test pins the
registry as read-only so adding the first write tool is a deliberate act.
Tests: 50 in this app (was 34); full backend unit tier 310 passing.
Registers the mcp-platform-auth critical path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
hari-kuriakose
changed the title
…rite tools
Three changes.
1. The deployment MCP endpoint moves to a suffix on the deployment's own
execution URL:
POST /deployment/api/<org>/<api_name>/ (REST)
POST /deployment/api/<org>/<api_name>/mcp (MCP)
Its URLs now live in api_v2/execution_urls.py, next to the endpoint they
extend, which also makes them inherit the deployment prefix's existing
WHITELISTED_PATHS entry instead of needing one of their own.
2. MCP_PATH_PREFIX is removed. It was introduced by this branch's first commit
and nothing outside it ever depended on it; the deployment MCP path is now
determined by API_DEPLOYMENT_PATH_PREFIX, and a second knob for a prefix
that should never change was just a way to break clients.
3. The platform server gains write tools: setApiDeploymentActive,
setPipelineActive and executePipeline, alongside a new listPipelines.
Authorization for the write tools reuses machinery that already exists rather
than inventing a scheme. Platform key tiers are defined over HTTP methods
(ApiKeyPermission.allows), but every MCP call is a POST — so the middleware's
tier check cannot tell listWorkflows from executePipeline. Each tool now
declares the method its REST equivalent would use, and check_tool_allowed
re-applies the key's tier against that. A tool marked DELETE is therefore
full_access-only for exactly the same reason a REST DELETE is.
Two categories stay out, deliberately:
* Credential operations. Key creation and rotation return the secret in their
response, so exposing them would give an agent processing untrusted document
content a way to mint or exfiltrate credentials. The codebase already reasons
this way — see CanRotatePlatformApiKey's docstring. A test asserts no tool
name suggests credential handling.
* Deletions. The write tools here are reversible by construction; removing a
workflow or project is not.
The previous read-only invariant test is replaced rather than dropped: it now
asserts every writes=True tool declares a non-GET required_method, so a write
tool cannot silently slip past the guard by keeping the default.
Write tools resolve their target through for_user, so a key cannot touch
another organization's resources — asserted directly, since the consequence
there is mutation rather than mere disclosure. The service-account caveat now
appears in every write tool's description, not just whoami: for reads it was a
disclosure note, for writes it means the key can modify any org resource.
Tests: 59 in this app (was 50); full backend unit tier 311 passing.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
for more information, see https://pre-commit.ci
…deferred read tools Four changes. 1. Documents the platform server's exclusions in its README — the credential-bearing surfaces and destructive operations no tool wraps, and why. Framed as what this server does not do rather than as an inventory of which endpoints return secrets, since this repository is public. 2. Adds the billable Prompt Studio operations: indexDocument, fetchResponse, bulkFetchResponse and singlePassExtraction. Each delegates to PromptStudioCoreView rather than reimplementing run-id generation, profile resolution, lookup gating and Celery dispatch — a parallel implementation would drift from the UI on the first change to either. 3. Adds a spend guard for them. There is no OSS token budget to gate on (usage_v2 reports after the fact; subscription enforcement is enterprise), so this is a per-organization *call* budget in Redis. Enforced at the transport via a `billable` flag, so a new costly tool cannot be added without being budgeted. Three deliberate behaviours: budget is consumed on invocation and never refunded — the opposite of the rate-limit slot in tools/execution.py, because that models concurrency while this models money already spent, and refunding would let an agent spend without limit by failing in a loop; exhaustion returns a retryable isError result rather than a permission error, so the agent waits instead of giving up; and it fails open, because bounding runaway loops is not worth taking the server offline when Redis blips. whoami reports remaining budget so an agent can pace itself. 4. Adds the read tools previously skipped for no good reason: listExecutions, getExecutionDetail, getUsageSummary, getWorkflowEndpoints, listToolInstances, listTags and listPipelines. Execution history in particular closes the gap where an agent could trigger work but not see how it went. Two things surfaced while building this, both now fixed: * A registry-wide test seeds an org with fake secrets, invokes every read tool and fails if any appears in the output. It caught getExecutionDetail being uncovered, then caught a real leak — an execution's error_message can embed the connection string a failed connector tried, which listExecutions returned verbatim. Error text is now passed through redact_secrets. * spend_guard.peek() did not fail open, so whoami broke when the cache was unavailable. Now matches consume(). getWorkflowEndpoints is the case to look at for the pattern: a workflow endpoint references a connector instance whose metadata decrypts on access, so the tool returns the shape of the connection and never the configuration. All responses here are built field-by-field from named attributes; never serializer.data. Tests: 82 in this app (was 59); full backend unit tier 331 passing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
for more information, see https://pre-commit.ci
Adds MCP_REDIS_DB, defaulting to REDIS_DB. Nothing about the current deployment changes: with both unset the budget counter stays in the shared cache DB and the guard keeps using django.core.cache, which is also what makes override_settings(CACHES=...) work in tests. The knob exists so MCP state can be moved to its own Redis DB later without that being a breaking change. When the two differ the guard builds a client for the configured DB, preferring a CACHES["mcp"] alias if the operator configured one so they control pooling and auth rather than having them inferred. This follows the shape of CacheService.clear_cache_optimized: Django cache by default, raw client only when a specific DB is required. Worth recording why the default matters. Sharing DB 0 means a FLUSHDB or a cache-wide eviction resets every organization's window — a fail-open outcome, consistent with a guard that already allows calls when the cache is unreachable. It is a loop bound, not an audited ledger, and the README now says so alongside the key layout and TTL-as-window mechanics. Also adds MCP_BILLABLE_CALL_LIMIT and MCP_BILLABLE_WINDOW_SECONDS to sample.env, which were introduced without being documented there. Removes CAPTURE3.md from the branch. The findings it captured are unfixed issues in the existing REST API and are being tracked elsewhere; a public PR was the wrong place for them. Tests: 87 in this app (was 82) — the new ones pin that a matching DB uses the shared cache, that REDIS_DB's empty-string default coerces to 0 the same way downstream code coerces it, and that a differing DB really does get a different client. peek() now has its own fail-open test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…r GET per spec The four billable Prompt Studio tools each require a document_id, and fetchResponse also a prompt_id, but nothing in the registry returned either. An MCP client sees only tools/list — it has no database and cannot invent a UUID — so the most expensive tools on the server were listed and uncallable. Adds listPromptStudioDocuments and listPrompts as the producers. Both resolve through the project rather than by a bare id lookup: the platform key authenticates as a service account, for which for_user() returns everything, so the project join is the scoping that actually holds. listPrompts builds its response field by field instead of via a serializer, because ToolStudioPrompt carries a profile_manager FK (the LLM, embedding and vector-store adapters) and a webhook URL that may embed a token — both of which the README promises never reach an agent, and neither of which was pinned by a test until now. The durable fix is test_registry_reachability: every id a tool requires must have a declared producer. The bug class was invisible per tool and only wrong in aggregate, so the invariant is asserted over the registry. Also, per the Streamable HTTP spec (rev 2025-06-18), GET opens a server-to-client SSE stream and a server offering none must answer 405. This server pushes nothing, so GET now returns 405 with Allow: POST, keeping the identity body for uptime probes. The "every MCP call is a POST" comments were imprecise in the same way and now say that every JSON-RPC *message* arrives as a POST — which is what makes required_method necessary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… docs test_no_credential_leak drives every read tool through a hardcoded argument map and raises if a tool requires an argument the map lacks. Both new tools require project_id, so without an entry the sweep would have failed outright — and the whole reason listPrompts builds its response by hand is that ToolStudioPrompt carries credential-bearing fields. Seeds a project, a document and a prompt whose webhook URL embeds a canary, so the sweep now actually exercises the tool whose leak risk motivated the hand-built dict. Also updates the README's GET description, which still promised a 200 identity response, and the last "every MCP call is a POST" comment. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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
1 participant
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
Hosts two MCP servers for Unstract, so coding agents can discover and run
document extraction as tool calls instead of hand-rolling HTTP requests.
PlatformApiKeyCustomAuthMiddleware/deployment/api/<org>/<api_name>/mcp/api/v1/unstract/<org>/mcp/They share the JSON-RPC transport (
transport.py) and differ only inauthentication and tool surface. Neither key works on the other server.
Modelled on how the mfbt backend hosts its MCP server (
Zipstack/mfbt,backend/app/routers/mcp_http.py): a hand-rolled JSON-RPC 2.0 endpoint mountedin the existing app, with a tool registry behind it. Same pattern, adapted from
FastAPI to Django/DRF.
How it mirrors mfbt
MCP_TOOLSname→callable dictMCPToolRegistry, declarative + JSON schemamfbt also ships a stdio server (
mcp_server.py, FastMCP). That is not thehosted path, so it is not what this replicates.
The app is
backend/mcp_server/— deliberately not_v2(that suffix marksapps carried through the v0.93.0 v1→v2 multitenancy migration; every app added
since is unsuffixed), and not bare
mcp, which would shadow the PyPImcpSDKpackage on the import path.
Endpoint
An MCP session is scoped to one API deployment and mirrors that deployment's
REST URL:
Auth is the deployment's existing API key — same key, same management UI.
There is no second credential to mint or revoke.
A
/<api_key>path variant exists for MCP clients that cannot set headers(mfbt carries the same workaround).
Tools
readMeFirstgetApiInfoextractDocumentgetExecutionStatusPlatform server
Discovery —
readMeFirst,whoami,listApiDeployments,listWorkflows,listPipelines,listPromptStudioProjects,getWorkflowEndpoints,listToolInstances,listTagsObservability —
listExecutions,getExecutionDetail,getUsageSummaryState changes (
read_write, reversible) —setApiDeploymentActive,setPipelineActiveBillable (
read_write, budgeted) —executePipeline,indexDocument,fetchResponse,bulkFetchResponse,singlePassExtraction19 tools. To extract through a deployed API, an agent calls
listApiDeploymentsfor anapi_namethen opens a separate session against thedeployment server; the Prompt Studio tools are for prompts before deployment.
The spend guard
Billable tools drive LLM inference and embedding, so they are budgeted per
organization over a rolling window (
MCP_BILLABLE_CALL_LIMIT, default 50/hour).It counts calls, not tokens. There is no OSS budget to gate on —
usage_v2reports after the fact, and subscription enforcement lives in the enterprise
overlay — so a call counter is the strongest guard implementable here.
Three deliberate behaviours: budget is consumed on invocation and never
refunded, which is the opposite of the rate-limit slot in
tools/execution.py(that models concurrency; this models money already spent, and refunding would
let an agent spend without limit by failing in a loop); exhaustion returns a
retryable
isErrorresult, not a permission error, so the agent waitsrather than concluding the tool is broken; and it fails open, because
bounding runaway loops isn't worth taking the server down when Redis blips.
whoamireports remaining budget so an agent can pace itself.How write tools are authorized
Tiers are defined over HTTP methods (
ApiKeyPermission.allows), but every MCPcall is a
POST— so the middleware's tier check can't telllistWorkflowsfrom
executePipeline. Each tool declares the method its REST equivalentwould use, and
check_tool_allowedre-applies the key's tier against that.Reusing those semantics rather than inventing a parallel scheme means a tool
marked
DELETEisfull_access-only for the same reason a RESTDELETEis.A
writes=Truetool left at the defaultrequired_method="GET"would slip pastthe guard, so a test asserts none exists.
Deliberately not exposed
configuration, notification webhooks, and all key management — several of
these return decrypted secrets in their ordinary responses, and an agent's
context is not a safe place for one. See the README for the full list.
role assignment, member removal). Every write tool here is reversible.
Two tests keep this true: one seeds an org with fake secrets, invokes every
read tool and fails if any appears in the output; another asserts no tool name
suggests credential, connector or adapter access. Where a tool must touch
something adjacent to a secret —
getWorkflowEndpointsreaches a connectorinstance — it returns named fields only, never
serializer.data.The URL placement is security-critical.
WHITELISTED_PATHSis matched withstartswith, so everything under/deployment/...— including the deploymentMCP server — is exempt from
CustomAuthMiddleware, while the tenant path isnot. That is exactly how this server inherits platform-key auth. Moving it under
the whitelisted prefix would silently remove all authentication, so a test
asserts it isn't whitelisted.
Because that auth is in middleware, the platform auth tests go through
django.test.Clientand the real URL.APIRequestFactory— correct for thedeployment server, which owns its auth — bypasses middleware entirely and would
have passed against a completely open endpoint.
Two constraints, documented rather than worked around:
read-tier key cannot use the platform server at all. The middlewaregates tiers on HTTP method and every MCP call is a
POST, so areadkey isrefused before the view runs — even for the read tools. Use
read_write.Special-casing MCP paths in shared middleware is a maintainer decision, not
this app's to make unilaterally.
service account, and
is_service_account=Truemakesfor_user()returnself.all(), ignoring per-user sharing regardless of theUSERrole the keywas created with. For reads that is a disclosure caveat; for writes it
means the key can modify any org resource.
whoami,readMeFirstand everywrite tool's description say so.
Design decisions
DeploymentHelper— the same validation the RESTendpoint uses, so the two surfaces cannot drift on who is allowed in.
Org scoping falls out of this: a valid key for org A cannot reach org B's
deployment (covered by the
wrong orgcase).ExecutionRequestSerializer— URL validation(S3-only, HTTPS-only) and the file-count cap live there. Reimplementing them
would let the MCP surface silently diverge from REST on what input is safe.
cannot be used to enumerate deployment names.
isError: true, not protocolerrors — clients treat protocol errors as unrecoverable transport faults,
whereas an agent-fixable problem should be readable and retryable.
Tests
82 passing. Protocol/dispatch, tool logic, the spend guard and redaction in
the unit tier; both auth boundaries and the credential-leak sweep in the
integration tier. Registers
mcp-server-authandmcp-platform-authcritical paths intests/critical_paths.yaml.Full backend unit tier: 331 passed, no regressions. The 35 integration
failures in
workflow_manager/execution/are pre-existing in my sandbox (noRedis) — the identical 35 fail on the base commit.
Cross-tenant tests assert a platform key can neither see nor modify another
organization's deployments, workflows, pipelines or Prompt Studio projects — the
write-side case matters more, since the consequence there is mutation rather
than disclosure. Each write tool is also asserted against its delegated helper
with
assert_called_once_with, the pattern that caught the bugs below.The credential-leak sweep caught a real leak: an execution's
error_messagecan embed the connection string a failed connector tried, and
listExecutionsreturned it verbatim. Error text now passes through
redact_secrets. The sametest also caught
getExecutionDetailbeing uncovered, and building it exposedthat
spend_guard.peek()did not fail open likeconsume().Earlier, writing the happy-path tests caught three more, all fixed:
accepts only S3 pre-signed URLs, so an agent following the description
would have failed every call.
tagsschema advertised an unbounded array; the serializer caps it at 1.call about to be rejected still pulled every document over the network.
Not implemented / notes for review
OAuth 2.1 + dynamic client registration is deliberately out of scope.
mfbt implements it for the claude.ai one-click browser connector; bearer auth
covers Claude Code and API clients. Adding it later is additive — discovery
endpoints alongside this router, no transport change.
MCP_PATH_PREFIXwas removed. It was introduced by this branch andnothing outside it depended on it; the deployment MCP path now follows
API_DEPLOYMENT_PATH_PREFIX. A second knob for a prefix that should neverchange was only a way to break already-configured clients.
No live-deployment smoke test. The happy path is covered by mocks at the
DeploymentHelperboundary, and a full MCP session (initialize → tools/list →tools/call) was exercised end-to-end against a real DB during development, but
nothing has run a real extraction through this path. Worth one manual check
against a live deployment before merge.
Cloud mounting: confirmed working. Mounted in
backend/base_urls.py.The enterprise overlay defines its own
ROOT_URLCONF, but that module isbuilt on top of this file's
urlpatternsrather than replacing them, so the/mcp/route is inherited automatically and no cloud-side change is needed.(Verified by inspection of the private repo; nothing from it is reproduced
here.)
Protocol version is pinned to
2024-11-05and does not negotiate againstthe client's requested version (mfbt does the same). Fine for current clients.
🤖 Generated with Claude Code