Remove Context.client_id - #3167
Conversation
Sorry, something went wrong.
The property echoed a non-standard `client_id` key from the request's `_meta`, which nothing in the SDK or the spec populates, so it was `None` unless a caller injected the meta key by hand. The name also collided with the OAuth `client_id` that callers usually mean, and the line carried a `# pragma: no cover` because it had never been exercised. The migration guide covers both replacements: reading the raw `_meta` key from `ctx.request_context.meta`, and the OAuth client via `get_access_token().client_id`.
📚 Documentation preview
|
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM — a clean removal of a dead property, with the breaking change documented in the migration guide.
What was reviewed:
- Searched the whole repo for remaining ctx.client_id / Context.client_id references — none remain outside the new migration entry (the other .client_id hits are all OAuth code, unrelated).
- Verified the migration example's replacement path: get_access_token() exists at mcp.server.auth.middleware.auth_context and returns AccessToken | None, which the example handles.
- Confirmed the docstring example in Context was updated alongside the property removal, and no tests referenced it (the removed line carried # pragma: no cover).
Overview
The PR removes the Context.client_id property from src/mcp/server/mcpserver/context.py and adds a corresponding breaking-change section to docs/migration.md. The property read a non-standard client_id key out of the request's _meta — nothing in the SDK or spec populates that key, it carried a # pragma: no cover (never exercised by any test), and a TODO questioning its existence. The docstring example referencing it is also removed.
Security risks
None introduced — this deletes code. If anything, it reduces a footgun: the old property's name collided with the OAuth client_id and could be mistaken for an authenticated identity when it was actually client-supplied _meta data. The migration entry explicitly points users to get_access_token().client_id for the authenticated identity, which is the correct guidance.
Level of scrutiny
Low. This is a dead-code removal on the v2 rework branch, where breaking changes are expected and must be documented in docs/migration.md — which this PR does, following the established section format. I verified nothing else in the repo (src, tests, docs, examples) references the removed property, and that the migration example's import path and None-handling are correct against the current code.
Other factors
The bug hunting system found no issues, and my own checks turned up nothing: the removed line was the only uncovered line in the file's property, so no test changes are needed, and coverage should be unaffected (the pragma'd line is gone). The PR timeline has no prior reviews or comments. The change matches the repo's stated conventions for intentional API removals.
Sorry, something went wrong.
Removes the Context.client_id property. It never returned an authenticated client identity: it echoed a non-standard client_id key from the request's _meta, which nothing in the SDK or the MCP spec populates, so it was None unless a caller injected meta={"client_id": ...} by hand. The name also collided with the OAuth client_id, which is what people usually mean by "the client" — see #373 for the recurring confusion.
Motivation and Context
A leftover from the original FastMCP integration. It carried a # pragma: no cover because it had never been exercised, and a TODO to see if it was needed. It isn't: the meta path is reachable via ctx.request_context.meta.get("client_id"), and the authenticated client is get_access_token().client_id.
How Has This Been Tested?
Existing suite; the removed line was the only untested one in the property. docs/migration.md gains a section with both replacements.
Breaking Changes
Yes — ctx.client_id now raises AttributeError. Migration entry added under the MCPServer Context section:
Types of changes
Checklist
AI Disclaimer