feat(mcp_servers): admin UI for managing MCP servers#1256
Open
srtab wants to merge 42 commits into
Open
Conversation
Move `import os` to module top; wrap per-row header read in `try/except DecryptionError` so a corrupted row is skipped with an error log while other servers still load. Add three tests covering env_ref happy path, missing env var drop, and decryption-error skip.
Wire MCPToolkit.get_tools() to call build_runtime_servers() via sync_to_async and pass the result into mcp_registry.get_connections_and_filters(), so DB-configured MCP servers are included in every tool fetch.
On app ready(), upsert_builtin_rows() creates a MCPServer row (source=BUILTIN, enabled=True) for each registered built-in class if one doesn't exist yet, preserving admin's enabled choice on subsequent boots. The base MCPServer class now delegates is_enabled() to a DB-backed _db_enabled() classmethod; SentryMCPServer and Context7MCPServer AND their URL check with super().is_enabled() to keep both gates in effect.
Call MCPServerDetailView._tools_or_empty on the edit GET path so the tool_filter_items field swaps from a textarea to checkboxes when tools are discoverable; updates form.html to render fields individually so the widget change is visible.
Querying the DB during AppConfig.ready() raised RuntimeWarning and broke in-memory test DB setup (every transactional async test failed with 'no such table: accounts_user'). post_migrate is the Django-recommended hook for 'ensure rows exist after migrations'.
Build ToolFilter DTO from DB row in build_runtime_servers so the registry filter is no longer silently None. Pass discovered_tools into MCPServerForm on POST so tool_filter_items uses MultipleChoiceField, preventing checkbox selections from collapsing to the last value.
Address PR review findings around silent failures and data loss:
- Refuse to POST edits when stored headers are undecryptable; the
formset would otherwise overwrite recoverable ciphertext with [].
- Iterate per server in MCPToolkit.get_tools so one bad endpoint
cannot blank tools from healthy peers; also fix a latent
SynchronousOnlyOperation by wrapping get_connections_and_filters
in sync_to_async.
- Only classify legacy headers as env_ref when the entire value is
${VAR}; preserve mixed strings like "Bearer ${TOKEN}" as literal.
- Render tool-filter as a textarea when discovery returns nothing,
so a transient handshake failure cannot wipe the persisted filter.
- Only swallow ProgrammingError in _db_enabled; let OperationalError
propagate so a DB outage cannot silently re-enable built-ins.
- test_connection logs via logger.exception and prefixes the
exception class name when str(err) is empty.
- New server_health() and "Broken: reason" badge in the list view
flag enabled servers that would silently be skipped at runtime.
- CheckConstraint forbids empty tool_filter_items when the filter
mode is set.
- Built-in edit form renders non-editable fields read-only; Save
is disabled when headers cannot be decrypted.
Tests cover the new error paths, the cache-bust on modify, the
per-server toolkit isolation, the form fallback to textarea, and
the new CheckConstraint.
srtab
force-pushed
the
feat/mcp-ui-config
branch
from
a3ec551 to
503dc6b
Compare
Address PR review feedback on the MCP servers admin UI: - Tool discovery: degrade to an empty list on DecryptionError (key rotation) in every entry point instead of 500-ing the tools endpoint; skip discovery for built-ins (their builtin:// URL can never handshake); negative-cache empty results with a short TTL so a broken server is not re-probed on every render nor pinned empty for the full TTL. - Migration 0002: normalize a non-"none" tool-filter mode with an empty item list to "none" so legacy import can't create a row that the 0003 check constraint would reject and abort the migration. - Admin UI: rebuild the list/detail/form/delete templates on the design system (card rows, btn-* and status-badge component classes); add the MCP servers entry to the admin sidebar and nav section map. - Models/registry: add MCPServer.is_builtin(); share one header-resolver between the runtime and test-connection paths (consistent warnings, fail-loud on an unknown header mode); skip and warn on an unknown transport in the registry; reject reserved names that shadow URL routes. - Fix inaccurate docstrings and add tests covering the discovery degradation, built-in skip, negative caching, migration normalization, the end-to-end tool filter, and _build_client transport mapping.
Replace the static header table with an Alpine-driven formset editor that lets admins add and remove HTTP header rows in the create/edit forms. - Extract the row markup into a reusable _header_row.html partial and wire an empty-form template plus an 'Add header' control. - Extend the shared django-formset.js to keep server-rendered rows (marked via -id or data-initial) in the POST so their index slots survive removal. - Override MCPServerHeaderForm.has_changed so a trailing blank extra row is skipped instead of failing clean_name, while blank initial rows are still rejected. - Blank the stored literal value on edit GET so secrets are never echoed, and preserve the stored value when a blank literal is re-submitted.
… legacy URL env vars
…ead of accumulating
Final whole-plan review fixes for the MCP built-in->remote migration: correct the false CHANGELOG upgrade note for v2.0.0-or-earlier upgrades, drop the stale built-in empty-state copy in the tools table template, fix the inverted is_builtin() docstring, mark MCP_SERVERS_CONFIG_FILE deprecated in the env-vars reference, update two e2e test docstrings to the real toolkit chain, and note the migration 0004 coupling on the deprecated Sentry/Context7 URL settings.
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.
Summary
Introduces a new
mcp_serversDjango app that lets admins manage MCP servers through the dashboard (create / edit / delete / toggle enable / test connection / discover tools), and wires the agent's MCP registry to read from the DB instead of.mcp.json.MCPServermodel with encrypted-header storage,source(builtin/custom),transport, env-var-ref headers, and a tool-filter (allow/block) — all enforced at the DB level via TextChoices and a CheckConstraintmcp_servers.services.build_runtime_servers()is the single adapter that converts DB rows into theUserMcpServerDTO the registry consumes;MCPRegistryis now a pure function over DTOsMCPToolkit.get_toolsiterates per server so a single bad endpoint cannot blank tools from healthy peers0002imports legacy JSON config (MCP_SERVERS_CONFIG_FILE) into the DB; built-ins are upserted viapost_migrate; a one-shot startup warning fires when the deprecated env var is still setAdminRequiredMixin), preserve-blank header semantics matchingsandbox_envs, and a 60s tool-discovery cache keyed onmodifiedenabledflip server-sideTest plan
make test— full unit suite (2530 passing)make lint-fix— cleanmake lint-typing— no new errors vs mainMCP_SERVERS_CONFIG_FILEset and verify the one-shot deprecation warning_headers_encrypted) and confirm:${VAR}header to an unset env var and confirm the list view shows "Broken: missing env var(s): VAR"