chore: regen + 6 DevEx round-3 MCP server fixes#18
Merged
Conversation
Per the MCP spec, every tool that fails because of an upstream condition must surface as ``CallToolResult.isError = true`` so the LLM can distinguish a tool that *ran but failed* from one that *succeeded with an error message in its return value*. Before this change, every tool wrapper caught ``DevhelmError`` and returned the formatted error string as a regular tool result (``isError = false``). AI agents (Cursor, Claude Desktop) couldn't tell that a 4xx/5xx had occurred and confidently reported "monitor created" right after the API rejected the request. DevEx round-3 P1.Bug3. The new ``raise_tool_error`` helper in ``devhelm_mcp.client`` wraps ``format_error`` and raises ``fastmcp.exceptions.ToolError`` (typed ``NoReturn`` so mypy keeps reasoning about flow). FastMCP's tool runtime catches the ``ToolError`` and serializes it into ``CallToolResult(isError=True, content=[TextContent(...)])``, preserving the human-readable error envelope (status, code, request_id) the LLM already knows how to surface to the user. Every ``return format_error(e)`` callsite across the 15 tool modules is now ``raise_tool_error(e)``. ``format_error`` itself is unchanged so the existing string-formatter unit tests in ``tests/test_client.py`` keep asserting against the formatted message verbatim. uv.lock is also updated to reflect the 0.7.0 package version pinned in ``pyproject.toml`` after the spec-sync rebase onto main; this keeps ``uv sync`` deterministic for downstream consumers. See https://modelcontextprotocol.io/specification/server/tools#error-handling for the wire-format contract. Co-authored-by: Cursor <cursoragent@cursor.com>
…hema Every monitor created through the MCP server must be attributed to ``managedBy="MCP"`` so the dashboard can honestly count and filter agent-created monitors. Before this change the wrapper forwarded whatever ``managedBy`` value the LLM passed (or 4xx'd because the SDK's ``ManagedBy`` enum hadn't been re-released with the new value yet), which meant every "create a monitor for me" interaction had a non-deterministic attribution column. DevEx round-3 P0.Bug5 + P1.Bug4 + P1.Bug5. The fix is two-layer: 1. ``_McpCreateMonitorRequest`` (in ``tools/monitors.py``) subclasses the SDK's ``CreateMonitorRequest`` and re-declares ``managed_by`` as ``Optional[Any]`` with ``exclude=True``. That keeps Pydantic validation passing when the LLM omits the field (the parent class makes it required) without coupling the MCP server to the SDK's stale enum membership — the literal ``"MCP"`` is the source of truth here, and the SDK can catch up at its own release cadence. 2. ``_strip_managed_by_from_create_monitor_schema`` in ``server.py`` walks the ``create_monitor`` tool's emitted JSON Schema and removes ``managedBy`` from both ``properties`` and ``required``. The body schema sits behind a ``$ref`` into ``$defs`` because of the discriminated ``config`` union, so the strip recurses through the ref target before the dereference middleware inlines the def for the wire response. Source-of-truth tools are read with ``run_middleware=False`` so the edits land on the underlying provider objects (the middleware copies tools on every ``tools/list`` and would otherwise lose our mutations). Server-side, ``create_monitor`` ``pop()`` s ``managedBy`` / ``managed_by`` from the dict that ``as_payload(body)`` produces, then unconditionally sets ``managedBy=_MCP_MANAGED_BY``. Even if a permissive HTTP client smuggles the field past the schema strip (custom tooling that ignores the advertised ``inputSchema``), the SDK call still sees ``"MCP"``. Users that genuinely need a different attribution string (CI systems acting on behalf of the agent, etc.) can keep using the SDK / CLI directly — both expose ``managed_by`` as a first-class parameter. Co-authored-by: Cursor <cursoragent@cursor.com>
``api_token`` is still accepted as a Python kwarg on every tool wrapper
for back-compat with path-style ``/{api_key}/mcp`` clients (and direct
test invocation), but the field MUST NOT appear in the JSON Schema
FastMCP advertises to clients via ``tools/list``.
Surfacing it had a real cost: well-behaved LLMs noticed the field and
populated it from whatever chat context they had, which leaked the
user's actual API token into prompt traces, model telemetry, and the
Cursor / Claude Desktop tool-call history that gets attached to support
threads. DevEx round-3 P2.Bug7.
The fix extends the existing post-registration ``_strip_internal_schema_fields``
hook (which already drops ``managedBy`` from ``create_monitor``) to
also drop ``api_token`` from every tool's ``properties`` and
``required``. After 0.7.0 the token resolves cleanly from the
``Authorization: Bearer …`` header (hosted ``/mcp``) or the
``DEVHELM_API_TOKEN`` env var (stdio), so the LLM never needs to see
the field at all. Direct callers and tests can keep passing
``api_token=...`` on the function call — that path is unchanged.
The ``StatusPage`` tool-schema test that previously asserted
``api_token in properties`` is inverted; the new
``test_api_token_hidden_from_input_schema`` pins the post-fix shape so
a future schema-emitter regression surfaces immediately.
Co-authored-by: Cursor <cursoragent@cursor.com>
The MCP ``initialize`` handshake's ``serverInfo`` field flows directly from the FastMCP server's ``name`` / ``version`` constructor args to the client. Before this change the ``name`` was the human-readable ``"DevHelm"`` and the ``version`` was unspecified, so FastMCP defaulted to its own framework version (``"3.2.4"`` at the time of the round-3 audit) and reported it as the DevHelm server version on every handshake. That made it impossible to correlate user reports with a specific ``devhelm-mcp-server`` release — every Cursor / Claude Desktop log line showed the same FastMCP framework version regardless of which MCP release was actually installed. DevEx round-3 P2.Bug8. The fix lifts ``_package_version`` to module scope (it was previously defined deep in the CLI helpers and only used for ``--version``), exports it as ``__version__``, and threads both the package name and version into the ``FastMCP(...)`` constructor. ``--version`` now also reads the same module-level constant, so any future divergence between the wire identity and the CLI surface is impossible by construction. Co-authored-by: Cursor <cursoragent@cursor.com>
FastMCP's default ``run()`` prints a multi-line ASCII banner on stderr
that includes a third-party promo URL ("Made with Prefect Horizon"
during the round-3 audit window). On stdio that banner is the first
thing local MCP clients (Cursor, Claude Desktop, Windsurf) see when
they tail the server log, and it's been a recurring source of
"is the server actually working?" confusion in support threads.
DevEx round-3 P2.Bug9.
The new ``_run_stdio()`` helper passes ``show_banner=False`` to
``mcp.run()``, which is the FastMCP-supported off switch. The
``TypeError`` fallback covers older FastMCP releases that don't accept
the kwarg — the banner is cosmetic, never break startup over it.
The existing ``test_no_args_runs_stdio_transport`` test is tightened
from ``run()`` to ``run(show_banner=False)`` so a future "let's bring
the banner back" regression surfaces immediately.
The HTTP transport path is unchanged: the banner only fires from
``mcp.run()``, which the HTTP path doesn't call (it boots Uvicorn
directly against ``app``).
Co-authored-by: Cursor <cursoragent@cursor.com>
Starlette's ``Mount("/mcp", inner_app)`` only forwards requests under
``/mcp/`` to the inner app — the bare ``/mcp`` URL gets a 307 redirect
to ``/mcp/``. Naive HTTP clients (some MCP wrappers, raw curl scripts,
LangChain's older HTTP transport) drop the request body and the
``Authorization`` header on the redirect, then fail the JSON-RPC
handshake with ``-32600 invalid request`` or 401. Production users
have hit this on every documented client config that pointed at
``mcp.devhelm.io/mcp`` (no trailing slash). DevEx round-3 P1.Bug6.
The fix is a tiny ASGI middleware (``_NormalizeMcpPath``) that sits
*outside* the Starlette router and rewrites the ASGI scope's ``path`` /
``raw_path`` from ``/mcp`` → ``/mcp/`` (and ``/{api_key}/mcp`` →
``/{api_key}/mcp/``) before routing happens. The request is then
delivered straight to the inner streamable-HTTP handler with no
redirect, so the body and headers survive intact.
The rewrite is idempotent — paths that already end in ``/`` are left
alone — and only fires for the ``/mcp`` family, so ``/health`` and any
future top-level routes are untouched.
The Uvicorn ``proxy_headers=True`` setting in ``main()`` stays put: it
was originally added so the now-obsolete redirect's ``Location`` header
honored ``X-Forwarded-Proto: https``, but it remains useful for any
future redirect (auth challenge, OAuth bounce, etc.) the app might
emit. The comment is updated to reflect that the trailing-slash
redirect is no longer the live concern.
Co-authored-by: Cursor <cursoragent@cursor.com>
Each ``TestX`` class in ``tests/test_devex_round3_fixes.py`` pins one
fix from the round-3 audit so any future regression surfaces in a
named test rather than as a vague schema diff. Keeping the coverage in
a single, named-by-bug file (rather than scattering it across
``test_tools``, ``test_client``, etc.) makes it obvious which behavior
survives re-shuffles of the SDK or FastMCP framework.
Coverage map:
* P0.Bug5 + P1.Bug4 + P1.Bug5 — ``managedBy`` is hidden from
``create_monitor``'s body schema, the SDK call always carries
``managedBy="MCP"``, and a smuggled override is overwritten.
* P1.Bug3 — ``DevhelmApiError`` and ``DevhelmTransportError`` from
the SDK both surface as ``CallToolResult.isError = True`` (driven
through the real FastMCP middleware chain via ``Client``).
* P2.Bug7 — no tool's ``inputSchema.properties`` exposes
``api_token``, but the kwarg is still accepted at the Python layer
for back-compat with path-style ``/{api_key}/mcp`` callers.
* P2.Bug8 — the FastMCP ``name`` / ``version`` match the package
metadata and the MCP ``initialize`` handshake reports the package
version in ``serverInfo.version``.
* P1.Bug6 — POST ``/mcp``, ``/mcp/``, and ``/{api_key}/mcp`` all
return 200 (no 307); ``/health`` is unaffected by the path
normalizer.
The coverage is intentionally end-to-end: ``isError`` tests drive the
real ``Client`` → middleware path, and the trailing-slash tests drive
the real Starlette app via ``TestClient`` so a future regression in
either layer (e.g. a middleware reorder) catches the issue.
Co-authored-by: Cursor <cursoragent@cursor.com>
caballeto
force-pushed
the
chore/spec-sync-20260505205830
branch
from
02474af to
defe795
Compare
caballeto
changed the title
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.
Bundles the auto spec-sync (devhelm SDK refresh) with six round-3 DevEx fixes that make the published MCP server actually usable from AI agents. This is the catch-up release after rounds 1 + 2; once it merges I'll cut a single
0.7.0-line bump on the surface side.Commits (top to bottom = newest first)
test:add round-3 DevEx fix coveragetest_devex_round3_fixes.py, one class per bug below.fix(server): rewrite /mcp ↔ /mcp/ to skip 307 redirect_NormalizeMcpPathASGI middleware rewrites the bare/mcppath →/mcp/before Starlette's router can emit the 307. Naive HTTP clients (some MCP wrappers, raw curl, LangChain's older transport) drop the body +Authorizationheader on 307s.fix(server): suppress FastMCP ASCII banner on stdio transport_run_stdio()helper passesshow_banner=False(with aTypeErrorfallback for older FastMCP). The banner was the first line in every Cursor / Claude Desktop log and included a third-party promo URL.fix(server): pin MCP serverInfo to package name + versionname="devhelm-mcp-server"+version=__version__(read fromimportlib.metadata). Was reporting the FastMCP framework version (e.g.3.2.4) on everyinitialize, making user reports impossible to correlate with a release.fix(server): hide api_token from advertised tool input schemas_strip_internal_schema_fieldspost-registration hook dropsapi_tokenfrom every tool'sinputSchema.properties/required. Function signature kept for back-compat with path-style/{api_key}/mcpclients.feat(monitors): force managedBy=MCP on creates and hide from input schema_McpCreateMonitorRequestsubclass + server-sidepayload["managedBy"] = "MCP"+ schema strip. Belt-and-suspenders so attribution is guaranteed, even if a permissive client smuggles amanagedBykey past the schema.fix(tools): raise ToolError on upstream API failures so isError=trueraise_tool_errorhelper inclient.py; everyreturn format_error(e)callsite (101 across 15 tool modules) nowraise raise_tool_error(e). AI agents now seeCallToolResult.isError = Trueper the MCP spec instead of confidently reporting "monitor created" after a 4xx.chore: sync OpenAPI spec and refresh devhelm SDKspec_updatedevent from the monorepo. Bumpeddevhelm0.6.0 → 0.6.1.Verification
End-to-end smoke (against the real Starlette app):
POST /mcp→ 200 (was 307 + body lost)POST /mcp/→ 200POST /{api_key}/mcp→ 200GET /health→ 200 (path normalizer skips it)serverInfo.versionininitializeresponse = package versioncreate_monitorwith nomanagedByin body → SDK call carriesmanagedBy="MCP"create_monitorwithmanagedBy="DASHBOARD"smuggled in → SDK call still carriesmanagedBy="MCP"(overridden)list_monitorswhen SDK raisesDevhelmApiError(404)→is_error=True, content ="ApiError (404 NOT_FOUND): Monitor not found | request_id=..."inputSchema.propertiesexposesapi_token(89 tools checked)Notes
pyproject.tomlversion here — will tag/release on the surface side after merge.ManagedBy = ["DASHBOARD", "CLI", "TERRAFORM"](noMCPenum value yet). The MCP server uses the literal string"MCP"to bypass that staleness; the SDK can catch up at its own release cadence without reblocking this PR.format_error(e)is unchanged so existing string-formatter unit tests keep asserting against the formatted message verbatim.