Make CacheConfig() the Client cache default and None the off switch - #3164
Make CacheConfig() the Client cache default and None the off switch#3164maxisbey merged 1 commit into
Conversation
Sorry, something went wrong.
The cache field was a three-state CacheConfig | Literal[False] | None where None meant "use the default config" and False meant "disabled". Since CacheConfig is a frozen dataclass and the store is instantiated per client, a plain CacheConfig() default is inert to share, so the sentinel bought nothing. Default to CacheConfig() and let None disable caching.
📚 Documentation preview
|
Sorry, something went wrong.
There was a problem hiding this comment.
LGTM — a clean three-state-to-two-state simplification of the Client.cache parameter.
What was reviewed:
- The __post_init__ collapse (is not False dance → is not None) preserves all three behaviors: default-on, custom CacheConfig, and off — each still pinned by a test.
- Checked whether sharing a default instance via default_factory=CacheConfig could leak state between clients — CacheConfig is frozen and store=None builds a per-client store, so it cannot (and the factory makes a fresh instance per client regardless).
- Grepped the whole repo for leftover cache=False spellings — none remain in src, tests, or docs; Literal stays imported for ConnectMode.
- Confirmed no docs/migration.md entry is needed: the cache is new on main and unreleased, matching the PR description.
Overview
The PR narrows Client.cache from CacheConfig | Literal[False] | None (where None meant default-on and False meant off) to CacheConfig | None with field(default_factory=CacheConfig), so the default is an explicit CacheConfig() and None is the single off switch. The substantive change is confined to two hunks in src/mcp/client/client.py (the field declaration + docstring, and the __post_init__ guard); the remaining four files are mechanical cache=False → cache=None renames in tests and docs.
Security risks
None introduced. The cache's security-relevant machinery — partition isolation, userinfo stripping, sha256-hashing of server identity, the custom-store target_id guard, and share_public gating — is untouched; the guard block that enforces them is byte-identical apart from the simplified entry condition. The one theoretical concern with a default_factory default (shared mutable state across clients) does not apply: CacheConfig is a frozen, slotted dataclass and store=None means a fresh per-client InMemoryResponseCacheStore, exactly as the PR description argues.
Level of scrutiny
Moderate: this is public-API surface on a client library, but the behavior change is a construction-time parameter spelling with no runtime-logic change. Since the response cache is new on main (SEP-2549) and not on the released v1.x line, there is no released API being broken, which also explains the absence of a docs/migration.md entry — consistent with the repo's guideline that migration docs cover changes to shipped APIs.
Other factors
Coverage is thorough: dedicated tests pin all three states (default-on with a per-client in-memory store, explicit CacheConfig, and cache=None disabling both the cache and the message-handler wrap), and the docs test suite proves the updated docs/client/caching.md claims against the real SDK. A repo-wide grep confirms no stale cache=False call sites or Literal[False] cache annotations remain. The bug hunting system reported no findings, the PR timeline has no outstanding reviewer comments, and the change matches the description exactly.
Sorry, something went wrong.
Client.cache was typed CacheConfig | Literal[False] | None, a three-state sentinel where None meant "use the default config" and False meant "disabled". This collapses it to CacheConfig | None with field(default_factory=CacheConfig): the default CacheConfig() is cache-on, and None turns caching off.
Motivation and Context
The None-as-auto sentinel exists to avoid sharing a default instance, but that concern isn't real here: CacheConfig is a frozen dataclass and store=None already means "build a fresh InMemoryResponseCacheStore per client", so the config carries no per-client state. With that gone, None can mean what it means everywhere else (no cache), and the if self.cache is not False: config = self.cache if self.cache is not None else CacheConfig() dance in __post_init__ collapses to if self.cache is not None.
How Has This Been Tested?
Updated the existing caching, subscription and docs tests for the new spelling (cache=None in place of cache=False); exercised default / CacheConfig() / None end-to-end against an in-process server with a ttlMs hint.
Breaking Changes
cache=False is now cache=None. The response cache is new on main (SEP-2549, not on v1.x), so there's no released API to migrate.
Types of changes
Checklist