Drop later-revision cache-hint fields on pre-2026 sessions - #3223
Drop later-revision cache-hint fields on pre-2026 sessions#3223maxisbey wants to merge 4 commits into
Conversation
A client that negotiated a pre-2026 protocol version failed `list_tools()` (and every other cacheable list/read) with a ValidationError whenever the server put a `cacheScope` value on the result that the 2026-07-28 enum doesn't allow. Those cache-hint fields aren't part of a 2025 session's schema at all, so the call should never have been able to fail on them. Inbound results are validated against the negotiated version's schema-exact surface and then parsed into the version-free result models, which carry every revision's fields. Because that second step was fed the raw wire dict, a field belonging to a newer revision than the one negotiated (`ttlMs`, `cacheScope`, `resultType`) still reached the version-free model, and that revision's constraints fired on a session that never negotiated the field. Add `strip_era_foreign_fields` to `mcp_types.methods` and apply it at every point that parses a result into the version-free models. It removes exactly the keys a newer revision's surface declares for the method but the negotiated version's does not, derived from the surfaces themselves. Keys no surface knows about (extension result-claim payloads) pass through untouched, legal fields a strict surface merely omits (`_meta` on an empty result) are never treated as foreign, and on a session speaking the newest revision nothing is stripped. A legacy server that put valid hint keys on the wire previously leaked those values onto the model even though the cache never consulted them there; they now show the documented defaults consistently, so the caching page and its wire-presence tip are updated to match.
📚 Documentation preview
|
The reported failure is on the client's inbound path, so the fix now lives there as private helpers next to the existing inbound-ttl clamp rather than as a new public function in mcp_types.methods. Drop the exported strip_era_foreign_fields helper and the calls threaded through the standalone parse functions and the server-side result parsing (the server-side calls stripped nothing, since no client-result method has a later-revision field). Restore the caching docs paragraph and its docs test, which the client-local change does not contradict.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any bugs, but this changes result parsing in the core send_request path with an observable behaviour change (later-revision fields silently dropped on legacy sessions), so it's worth a human look at the design.
What was reviewed:
- The
_later_revision_fieldsderivation:KNOWN_PROTOCOL_VERSIONSis ordered oldest→newest andSERVER_RESULTSrows are per-version models/unions, so the newer-minus-current set is computed as intended and is empty at 2026-07-28. - Alias handling:
_wire_fieldsusesfield.alias or name, which matches theby_name=Falsewire-alias parse, so no legal current-version key can be misclassified as foreign. - Interaction with the existing
_clamp_inbound_ttland the surface gate ordering — the drop runs after the gate and rebuilds the dict rather than mutating it. - A candidate issue (an unreachable non-model filter branch in
_wire_fields) was examined and ruled out as a defensive no-op, not a bug.
Extended reasoning...
Overview
The PR adds two cached private helpers to src/mcp/client/session.py (_wire_fields, _later_revision_fields) and a single drop step in ClientSession.send_request: after the per-version surface gate, top-level result keys that only a newer protocol revision's surface declares for the method are removed before the raw dict is parsed into the version-free result type. A new parametrized test covers every legacy-reachable cacheable result carrying ttlMs: -1 / cacheScope: "session" on a 2025 session.
Security risks
None identified. The change only makes the client more tolerant of out-of-contract advisory fields from a server on legacy sessions; it does not widen what is accepted on modern (2026-07-28) sessions, where the enum/ge=0 constraints still reject bad values. The dropped fields were never consulted by the response cache on legacy sessions, so there is no cache-poisoning angle. Extension result-claim payloads and vendor keys are untouched because the foreign set is derived only from known surfaces.
Level of scrutiny
This is production client-protocol logic on the hot path of every request, encoding a cross-version negotiation policy (which fields count as "foreign" at each revision). The implementation checks out — ordering of KNOWN_PROTOCOL_VERSIONS, the shape of SERVER_RESULTS, and alias-vs-name handling were all verified against mcp_types — and the multi-agent review found no bugs (one candidate about a dead defensive branch in _wire_fields was examined and refuted). But the policy itself (silently dropping smuggled later-revision values rather than surfacing them, and leaving the in-era fail-closed behaviour as-is) is a design judgement a maintainer should own, and the PR itself flags a related follow-up in the public parse_server_result helpers.
Other factors
Test quality follows the repo's conventions (scripted dispatcher, fail_after, parametrized over all legacy-reachable cacheable results, asserts model_fields_set), and the author reports 100% coverage with strict-no-cover clean plus an end-to-end repro against main. No outstanding reviewer comments — the only timeline entry is the docs-preview bot.
A `RootModel` wrapper row (the empty result served at pre-2026 versions) reported the pydantic-internal `root` key rather than the keys of the model it wraps; unwrap it so a legal field a strict surface merely omits, such as `_meta`, is never mistaken for a later revision's field. This is latent against today's tables (none of the wrapper rows has a newer-revision counterpart) but keeps the field-set logic correct as revisions are added. Extend the legacy-session test over every handshake protocol version, cover the `resultType` tag on `tools/call` and `prompts/get` results, and register the inbound tolerance in the interaction requirements manifest alongside its existing outbound counterpart.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 3 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="tests/interaction/_requirements.py">
<violation number="1" location="tests/interaction/_requirements.py:3282">
P3: The requirements manifest will present this SDK-specific tolerance rule as spec-mandated even though the cited versioning material only introduces the newer fields; it does not require legacy clients to strip them. This makes future conformance audits treat a local compatibility choice as a protocol obligation. Suggest recording this requirement with `source="sdk"`, matching the behavior and the accompanying test docstrings.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| ), | ||
| ), | ||
| "hosting:client:legacy-inbound-modern-vocabulary-dropped": Requirement( | ||
| source=f"{SPEC_2026_BASE_URL}/basic/versioning", |
There was a problem hiding this comment.
P3: The requirements manifest will present this SDK-specific tolerance rule as spec-mandated even though the cited versioning material only introduces the newer fields; it does not require legacy clients to strip them. This makes future conformance audits treat a local compatibility choice as a protocol obligation. Suggest recording this requirement with source="sdk", matching the behavior and the accompanying test docstrings.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/interaction/_requirements.py, line 3282:
<comment>The requirements manifest will present this SDK-specific tolerance rule as spec-mandated even though the cited versioning material only introduces the newer fields; it does not require legacy clients to strip them. This makes future conformance audits treat a local compatibility choice as a protocol obligation. Suggest recording this requirement with `source="sdk"`, matching the behavior and the accompanying test docstrings.</comment>
<file context>
@@ -3278,6 +3278,19 @@ def __post_init__(self) -> None:
),
),
+ "hosting:client:legacy-inbound-modern-vocabulary-dropped": Requirement(
+ source=f"{SPEC_2026_BASE_URL}/basic/versioning",
+ behavior=(
+ "A client on a pre-2026 session that receives 2026-07-28 result vocabulary "
</file context>
| source=f"{SPEC_2026_BASE_URL}/basic/versioning", | |
| source="sdk", |
There was a problem hiding this comment.
I didn't find any bugs, but this changes result parsing on every client request path and encodes a versioning policy (silently dropping later-revision wire fields on legacy sessions), so it deserves a human maintainer's judgment.
What was reviewed:
- The new
_wire_fields/_later_revision_fieldshelpers against themcp_typestables:KNOWN_PROTOCOL_VERSIONSis ordered oldest→newest andSERVER_RESULTSvalues aretype[BaseModel] | UnionType, matching the helpers' assumptions; surfaces areextra="ignore", confirming the gate can't catch this and the drop must happen pre-parse. - Interaction with extension result claims: claims only activate on modern versions, where the foreign-field set is empty, so dropping
resultTypeon legacy sessions can't mis-route claimed results. - Two candidates were examined and ruled out: the drop firing on the pre-handshake/stateless
2025-11-25fallback (consistent with the existing fallback semantics elsewhere in the session), and the new requirement entry's spec-URL attribution (entry is explicitly marked as covering an SDK-defined seam).
Extended reasoning...
Overview
The PR touches ClientSession.send_request in src/mcp/client/session.py, adding two cached private helpers (_wire_fields, _later_revision_fields) that derive, from the per-version surface tables in mcp_types.methods, the set of result keys a newer protocol revision declares for a method but the negotiated version does not — and drops those keys from the raw wire dict before parsing into the version-free result type. New parametrized tests cover every legacy-reachable cacheable result plus the resultType tag on tools/call/prompts/get, and a requirements-registry entry is added.
Security risks
Low. The change makes the client more lenient on legacy sessions (dropping out-of-contract advisory fields instead of failing), and strictly only for keys that some known newer revision's surface declares — unknown/vendor keys still pass through and in-schema fields are never dropped (set difference against the negotiated version's own surface). No auth, crypto, or injection surface. The main risk class would be a client silently ignoring data it should enforce, but these fields (ttlMs, cacheScope, resultType) are outside the negotiated schema on the affected sessions and the response cache never consulted them there.
Level of scrutiny
High. This sits on the hot path of every client request and encodes a design decision about cross-revision field handling — the derivation via pydantic model introspection (union walking, RootModel unwrapping, alias resolution) is clever but subtle, and whether foreign later-revision fields should be dropped silently versus surfaced is a maintainer-level protocol-policy question. The PR itself flags an acknowledged inconsistency left for follow-up (the public parse_server_result/parse_client_result helpers still exhibit the bug). That combination is exactly what the approval guidelines say not to shadow-approve.
Other factors
The bug hunting system found no bugs; two finder candidates (the drop also firing on the stateless/pre-handshake fallback version, and the requirement entry's source attribution) were examined and refuted. I independently verified the helpers' assumptions against mcp_types/version.py and mcp_types/methods.py (version ordering, table value types, extra=ignore surfaces) and the non-interaction with extension result claims (_claim_active requires a modern version, where the foreign set is empty). Test coverage is thorough and follows the repo's test-quality conventions; the author reports 100% coverage and an end-to-end repro against main. No human reviewer has looked at the PR yet — only the docs-preview bot has commented — which further argues for leaving the substantive verdict to a maintainer.
The manifest's ID vocabulary is aligned entry-by-entry with the TypeScript SDK's requirements suite, so a new ID is a maintainer decision rather than something to mint alongside a bug fix. The client tests stand on their own without it. No-Verification-Needed: manifest-only revert with no runtime surface
There was a problem hiding this comment.
No bugs found, but this changes the result-parse path of every client request on legacy sessions and makes a small policy call (silently dropping out-of-schema hint fields rather than surfacing them), so it's worth a human maintainer's look.
What was reviewed:
- The drop-set derivation: newer-surface keys minus negotiated-surface keys — verified the legacy result classes are shared across pre-2026 versions, so only 2026-07-28 vocabulary (ttlMs/cacheScope/resultType) is ever dropped; vendor/extension keys and fields the negotiated version declares pass through.
_wire_fieldsagainst the actualSERVER_RESULTSrow shapes: PEP-604 unions (AnyCallToolResultetc.) andRootModel[Result]rows (EmptyResult) both match the branches it handles.- Modern (2026-07-28) sessions: no newer known revision, so the foreign set is empty and enum enforcement is unchanged.
KNOWN_PROTOCOL_VERSIONSordering (oldest→newest) backs theindex(version)+1:slice.
Extended reasoning...
Overview
The PR is confined to src/mcp/client/session.py plus tests. It adds two cached private helpers — _wire_fields (top-level wire keys a result model or union declares, with RootModel unwrapping) and _later_revision_fields (keys a newer revision's surface declares for a method that the negotiated version's does not) — and a single drop step in ClientSession.send_request that strips exactly those keys from the raw result before parsing into the version-free result type. This fixes a real bug: a pre-2026 session failing list_tools() etc. with a ValidationError when a server smuggles a 2026-07-28 cacheScope/ttlMs/resultType value the 2026 constraints reject, even though those keys are outside the negotiated schema.
Correctness checks performed
I verified the helpers' assumptions against the actual data they introspect: KNOWN_PROTOCOL_VERSIONS is documented and defined oldest→newest, so the index(version)+1: slice is sound; SERVER_RESULTS values are typed type[BaseModel] | UnionType, and the 2026 rows are PEP-604 unions (CallToolResult | InputRequiredResult) and plain classes, with EmptyResult = RootModel[Result] covered by the RootModel branch. Because all pre-2026 result rows share the same v2025 classes, the set difference only ever contains 2026-introduced keys — there is no version pair where a field legitimately meaningful at the negotiated version could be dropped. On a 2026-07-28 session the foreign set is empty, so in-era enum enforcement is unchanged, as the description claims. The new tests cover every legacy-reachable cacheable result across all four handshake versions plus the resultType mis-routing case.
Security risks
None identified. The change makes the client more lenient toward out-of-contract inbound fields on legacy sessions only; it does not touch auth, transport, or outbound data, and it cannot widen what a modern session accepts.
Level of scrutiny
This warrants human review despite the clean findings: it sits on the hot path of every client request, uses pydantic-internals-adjacent reflection (__pydantic_root_model__, model_fields, alias handling) that future mcp-types codegen changes could silently invalidate, and it encodes an SDK policy decision — dropping rather than surfacing or warning on out-of-schema hint fields. The PR description itself flags the sibling public helpers (parse_server_result/parse_client_result) as still affected, which is a scoping decision a maintainer should ratify.
Other factors
Test coverage is strong (parametrized over all handshake versions and all affected verbs, plus model_fields_set assertions proving the drop rather than a default coincidence), and the author reports 100% coverage with strict-no-cover clean and an end-to-end repro against a raw stdio server. The earlier cubic-dev-ai comment about the requirements-manifest source was resolved by removing the manifest entry from the PR.
1 participant
A client that negotiated a pre-2026 protocol version fails
list_tools()(and every other cacheable list/read) with aValidationErrorwhen the server puts acacheScopevalue on the result that the 2026-07-28 enum doesn't recognise — e.g. a server that has moved to a newer scheme, or a non-conformant one. On a 2025 session that field isn't part of the negotiated schema at all, and the client already treats these hints as inert on legacy sessions, so the call shouldn't be able to fail on it.Motivation and Context
ClientSession.send_requestvalidates a result against the negotiated version's schema-exact surface (the version gate), then parses the same raw wire dict into the version-free result type, which carries every revision's fields. So a field belonging to a later revision than the one negotiated (ttlMs,cacheScope,resultTypeat 2025-11-25) still reached that type, and its 2026-07-28 constraints (closedLiteral,ge=0) fired on a session that never negotiated the field — even though the surface had already ruled the key out-of-schema.send_requestnow drops exactly the keys a newer revision's surface declares for the method but the negotiated version's does not, before parsing. The set is derived from the surfaces themselves, so keys no surface knows about (extension result-claim payloads) pass through untouched, legal fields a strict surface merely omits (_metaon an empty result) are never treated as foreign, and on a session speaking the newest revision nothing is dropped. The helpers are private and sit next to the existing_clamp_inbound_ttl, which handles the same class of concern.Not changed here: on a modern (2026-07-28) session
cacheScopeis in-schema and the enum stays enforced, so an unrecognised value there still rejects the result. Whether an advisory hint should fail closed rather than fatally in-era is a separate policy question.How Has This Been Tested?
tools/list,prompts/list,resources/list,resources/templates/list,resources/read) carryingttlMs: -1andcacheScope: "session", and each call succeeds with the defaults and neither hint inmodel_fields_set.strict-no-coverclean.Client(stdio_client(...)): a 2025-06-18 session with a badcacheScope, garbageresultType, and a vendor key now completeslist_tools()/call_tool(); the identical script againstmainreproduces the reportedValidationError. A 2026-07-28 session sending the same bad value still rejects, as expected.Breaking Changes
None to public signatures. Observable behaviour change: on a pre-2026 session a result no longer carries
ttl_ms/cache_scope/result_typevalues a server smuggled onto the wire — they were outside that session's schema and are now dropped, so the models show their documented defaults (which the response cache never consulted on legacy sessions anyway).Types of changes
Checklist
Additional context
The fix is deliberately client-local. The public
parse_server_result/parse_client_resulthelpers inmcp_types.methodsshare the same parse shape and still hit this on a legacy body with a bad hint; that's left for a follow-up rather than widened into this change.AI Disclaimer