Fix docs/release follow-ups from the mcp-types package split#2977
Fix docs/release follow-ups from the mcp-types package split#2977maxisbey wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
No bugs found in this review. Deferring to a human since the changes rework the PyPI publish workflow (a release-critical path that PR CI never exercises) and lean on out-of-band PyPI trusted-publisher setup, plus an open API-naming question (mcp_types.version) that maintainers should weigh in on.
Extended reasoning...
Overview
This PR is a follow-up to the mcp-types package split (#2973): it reworks the PyPI publish workflow into two ordered publish steps (mcp-types first, then mcp), fixes the docs reference generator so mcp_types modules get importable identifiers, adds src/mcp-types to the mkdocs paths/watch and the deploy-docs trigger, replaces the sdist-only LICENSE force-include with a real LICENSE file in src/mcp-types (content matches the root LICENSE exactly), adds an AST-based test asserting mcp_types only imports its declared dependencies, moves a test file, and updates several docs (migration guide, RELEASE.md, AGENTS.md, schema/README.md).
Security risks
No direct security-sensitive code is touched (no auth, crypto, or permission logic). The publish-pypi.yml changes do affect the trusted-publishing release path (the job has id-token: write), so they are supply-chain-adjacent: the action ref stays pinned to the same SHA and the new inputs (packages-dir, skip-existing) are standard, but a mistake here surfaces only at release time, not in PR CI.
Level of scrutiny
Most of the diff is documentation and a small, self-contained docs-generation hook change that I would happily approve on its own. However, the publish workflow restructuring is release-critical, cannot be validated by any CI that runs on this PR, and explicitly depends on PyPI-side configuration (the mcp-types trusted publisher and ownership) that only maintainers can confirm. The PR also intentionally leaves open a public-API naming decision (mcp_types.version) that the author flags as needing a maintainer call before the first mcp-types release. Those are exactly the kinds of judgment calls a human owner should make.
Other factors
The new tests/types/test_standalone.py is well-constructed (static AST analysis covers TYPE_CHECKING-only imports) and the author reports mutation-testing it. The docs changes were verified against an actual mkdocs build per the description. I found no correctness issues; the deferral is purely about the release-path ownership and the open design question, not about suspected bugs.
- gen_ref_pages.py: derive each module identifier from its own package root, so the API reference emits `mcp_types.*` instead of the unimportable `mcp-types.mcp_types.*`. Add `src/mcp-types` to the mkdocs paths/watch lists and to the deploy-docs trigger filter. - publish-pypi.yml: build the two distributions into separate directories and publish `mcp-types` before `mcp`, so a failed dependency upload can never leave `mcp` published with an unsatisfiable exact pin. `skip-existing` only on the mcp-types step, so a re-run recovers from a partial publish without muting a duplicate `mcp` upload. - mcp-types: ship a real LICENSE file instead of the sdist-only force-include, so the wheel carries License-File in every build mode. - tests: add a static check that every `mcp_types` module imports only the standard library and the dependencies `src/mcp-types/pyproject.toml` declares; move test_version.py to mirror its source module. - docs: the migration guide taught `from mcp import CallToolResult`, which raises ImportError (`CallToolResult` is not in `mcp.__all__`) -- use `Resource`. Name `mcp_types.version` as the home of the protocol-version constants, link the new API reference, correct the "depends only on pydantic" wording, and point schema/README.md at the new codegen path. - AGENTS.md / RELEASE.md: document that the root project's runtime dependencies are dynamic and live in the uv-dynamic-versioning hook table, and that a release now publishes both distributions.
20d2832 to
631efd8
Compare
There was a problem hiding this comment.
I didn't find any bugs — the docs/test changes look correct, but the publish-pypi.yml restructuring changes the release pipeline (which never runs in PR CI) and depends on out-of-band Trusted Publisher setup, so a maintainer should look this over before it lands.
Extended reasoning...
Overview
This PR is a set of follow-ups from the mcp-types package split (#2973): a fix to docs/hooks/gen_ref_pages.py so the new package's modules get importable mkdocstrings identifiers, mkdocs paths/watch and deploy-docs.yml trigger updates, a restructured publish-pypi.yml that builds and publishes mcp-types and mcp from separate dist directories with mcp-types first, a new AST-based test asserting mcp_types only imports its declared dependencies, a moved test file, and documentation updates (migration guide, AGENTS.md, RELEASE.md, schema/README.md).
Security risks
No meaningful security risk: no auth, crypto, or input-handling code is touched. The publish workflow keeps the same pinned action SHA and trusted-publishing setup; the only behavioral change is splitting the upload into two ordered steps with skip-existing: true on the mcp-types step.
Level of scrutiny
Most of the PR (docs, mkdocs config, the new static-import test) is low risk and reads correctly — the gen_ref_pages.py loop change is straightforward and the failure mode it fixes is well explained. The part that warrants a human maintainer's eyes is the release path: publish-pypi.yml cannot be exercised in PR CI, the ordering/skip-existing semantics directly determine what happens during a partially failed release, and the PR explicitly calls out a non-code precondition (the mcp-types PyPI project needs a matching Trusted Publisher and owners) that only a maintainer can verify and act on. Release-pipeline changes are exactly the kind of thing that should not be merged on a bot's say-so alone.
Other factors
The PR description is detailed and includes verification of the docs build, the new test's mutation behavior, and the build artifacts; no bugs were found by the bug hunting system, and there are no outstanding reviewer comments in the timeline. The decision to defer is about ownership of the release process and the out-of-band PyPI configuration, not about any defect found in the changes.
…fier - publish-pypi: build both packages into one dist/ and publish in a single step with skip-existing, so a partially failed upload is fixed by re-running the job rather than by step ordering. - mkdocs: drop the watch package list instead of extending it. - The Development Status classifier is permanently 5 - Production/Stable; say so in RELEASE.md and set it on mcp-types, which #2975 left at Alpha.
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/publish-pypi.yml">
<violation number="1" location=".github/workflows/publish-pypi.yml:59">
P1: Publishing both `mcp` and `mcp-types` in one Twine upload can leave PyPI in a partial state where `mcp` is published but `mcp-types` fails, breaking the exact-pin dependency for the released version.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| @@ -58,3 +58,7 @@ jobs: | |||
|
|
|||
| - name: Publish package distributions to PyPI | |||
There was a problem hiding this comment.
P1: Publishing both mcp and mcp-types in one Twine upload can leave PyPI in a partial state where mcp is published but mcp-types fails, breaking the exact-pin dependency for the released version.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/publish-pypi.yml, line 59:
<comment>Publishing both `mcp` and `mcp-types` in one Twine upload can leave PyPI in a partial state where `mcp` is published but `mcp-types` fails, breaking the exact-pin dependency for the released version.</comment>
<file context>
@@ -56,18 +56,9 @@ jobs:
- # lets a re-run after a failed `mcp` upload get past the already-published
- # `mcp-types`; the `mcp` step omits it so a duplicate upload still fails loudly.
- - name: Publish mcp-types to PyPI
+ - name: Publish package distributions to PyPI
uses: pypa/gh-action-pypi-publish@cef221092ed1bacb1cc03d23a2d87d1d172e277b # release/v1
with:
</file context>
What
Follow-ups from the review of #2973 (the review comment) — the items #2973 itself didn't pick up before it merged. Two of them are invisible to every check that ran green on that PR, because neither the docs build nor the publish path runs in PR CI.
Key changes
The published API reference (PR-introduced by #2973, no CI sees it).
docs/hooks/gen_ref_pages.pyderives the mkdocstrings identifier from the path relative tosrc/, which for the new package yields::: mcp-types.mcp_types.*— and griffe resolves modules by walking the filesystem, notimportlib, so it happily collects the hyphenated directory as a namespace package. The result is a greenmkdocs build --strictthat publishes the entire type system under the headingmcp-types.mcp_types(a dotted path that is a syntax error to import) while every existing link to/api/mcp/types/*404s. Fixed by deriving each module's identifier from its own package root, addingsrc/mcp-typesto the mkdocspaths:list and to thedeploy-docs.ymltrigger filter (which only listedsrc/mcp/**), and dropping the mkdocswatch:package list rather than extending it.The publish path. The release job uploads both distributions in one trusted-publishing step. A partial upload — say, the
mcp-typestrusted publisher isn't configured yet, or PyPI hiccups on one project — used to be unrecoverable from the workflow alone, because re-running it failed on the files that had already landed.skip-existing: truemakes a re-run skip whatever is already on PyPI and upload the rest, so a partial failure is fixed by re-running the job.Development Status classifier. #2975 set
mcpto5 - Production/Stablebut leftsrc/mcp-types/pyproject.tomlat3 - Alpha, and the two distributions are published in lockstep with an exact==pin — so their PyPI metadata would contradict each other, immutably.mcp-typesnow matches, andRELEASE.mdrecords that the classifier is permanently5 - Production/Stableand is not part of any release.A test for the package's headline contract. Nothing in the suite would fail if
mcp_typeslater grew animport httpx—tests/conftest.pyimports the SDK before any test runs, so the full transport stack is always already insys.modules.tests/types/test_standalone.pyreads everymcp_typesmodule's imports from source withastand asserts they're a subset of the standard library plus the dependenciessrc/mcp-types/pyproject.tomldeclares. Static analysis is deliberate: it holds lazy andTYPE_CHECKING-only imports to the same bar, which a runtime probe never executes. Also movestests/shared/test_version.pytotests/types/to mirror its source module.Docs. The migration guide's headline "After (v2)" example,
from mcp import Tool, CallToolResult, raisesImportError—CallToolResultwas never inmcp.__all__. Swapped forResource, which is. Also: namemcp_types.versionas the home of the protocol-version constants (they're not inmcp_types.__all__and the section never said where they went), link the new API reference from the section about the move, correct "depends only onpydantic" to also nametyping-extensions, and pointschema/README.mdat the new codegen output path.Contributor docs.
[project].dependenciesbecoming dynamic meansuv add <runtime-pkg>on the root project now fails (and with a misleading build-backend traceback that never points at the real edit site), so AGENTS.md's "Installation:uv add <package>" gets the exception. RELEASE.md now covers the two-distribution release and the trusted-publisher precondition.Not done here, deliberately
mcp-typesPyPI project (currently owned byKludexalone, vsdsp/jspahrsummers/maxisbeyonmcp) needs a Trusted Publisher matchingmcp's — this repository, workflowpublish-pypi.yml, environmentrelease— plus the same owners. Without it the upload is rejected; the new step ordering bounds the blast radius to "nothing publishes" instead of "pip install mcpbreaks", but it doesn't make the release succeed.mkdocs build --strictjob in PR CI is the mechanism that would have caught the gen_ref_pages bug (and its whole class) before merge. Left out of this PR because the build fetches three externalobjects.invinventories, so it adds a real external-flakiness surface to every PR — that tradeoff deserves its own decision.mcp_types.versionis now load-bearing public API under a name that reads like "the package's version". Renaming (or re-exporting the registry from the top level) is free until the firstmcp-typesrelease and not after; raised on Split protocol types into a standalonemcp-typespackage #2973 and intentionally not decided unilaterally here.Verification
gen_ref_pages.pyexecuted against the tree: 97 pages, zero hyphenated identifiers, all sixmcp_types.*ones resolve via griffe with the newpaths;mkdocs build --strictexits 0 withapi/mcpandapi/mcp_typesboth emitted.import anyio, afrom mcp.server import ..., aTYPE_CHECKING-onlyimport httpx, and an undeclared directimport annotated_typeseach make it fail.pypa/gh-action-pypi-publishat the pinned ref acceptsskip-existing.strict-no-coverclean; the full pre-commit suite (ruff, pyright strict, markdownlint, theuv.lockcheck) passes.AI Disclaimer
This PR was developed with the assistance of Claude. The changes and the verification above were reviewed.
AI Disclaimer