Make CacheConfig() the Client cache default and None the off switch#3164
Conversation
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
|
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 Falsedance →is not None) preserves all three behaviors: default-on, customCacheConfig, and off — each still pinned by a test. - Checked whether sharing a default instance via
default_factory=CacheConfigcould leak state between clients —CacheConfigis frozen andstore=Nonebuilds a per-client store, so it cannot (and the factory makes a fresh instance per client regardless). - Grepped the whole repo for leftover
cache=Falsespellings — none remain in src, tests, or docs;Literalstays imported forConnectMode. - Confirmed no
docs/migration.mdentry is needed: the cache is new onmainand unreleased, matching the PR description.
Extended reasoning...
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.
Client.cachewas typedCacheConfig | Literal[False] | None, a three-state sentinel whereNonemeant "use the default config" andFalsemeant "disabled". This collapses it toCacheConfig | Nonewithfield(default_factory=CacheConfig): the defaultCacheConfig()is cache-on, andNoneturns caching off.Motivation and Context
The
None-as-auto sentinel exists to avoid sharing a default instance, but that concern isn't real here:CacheConfigis a frozen dataclass andstore=Nonealready means "build a freshInMemoryResponseCacheStoreper client", so the config carries no per-client state. With that gone,Nonecan mean what it means everywhere else (no cache), and theif self.cache is not False: config = self.cache if self.cache is not None else CacheConfig()dance in__post_init__collapses toif self.cache is not None.How Has This Been Tested?
Updated the existing caching, subscription and docs tests for the new spelling (
cache=Nonein place ofcache=False); exercised default /CacheConfig()/Noneend-to-end against an in-process server with attlMshint.Breaking Changes
cache=Falseis nowcache=None. The response cache is new onmain(SEP-2549, not onv1.x), so there's no released API to migrate.Types of changes
Checklist