implementing SEP-2549#889
Conversation
7175cdf to
a6151ad
Compare
a6151ad to
f8edae7
Compare
alexhancock
left a comment
There was a problem hiding this comment.
Pre-approving so you can merge when the below is addressed!
The cache hints (ttlMs and cacheScope) are written to _meta, but the spec defines them as first-class result fields
Per SEP-2549, ttlMs and cacheScope are properties on the result itself (CacheableResult), so the wire shape is { "tools": [...], "ttlMs": 300000, "cacheScope": "public" } (example, L112) — not nested under _meta
Fix list:
- Top-level fields: make
ttl_ms/cache_scopereal#[serde(rename = "ttlMs"/"cacheScope")]fields on the result struct instead ofmeta.insert(...). resources/read: the same meta.insert pattern is duplicated below in ReadResourceResult, writing into each ResourceContents. The spec attaches the hints to ReadResourceResult itself, so they should live on the result (otherwise they vanish when contents is empty). Scope guidance L90.- ttlMs rules: u64 correctly blocks negatives on the producing side 👍, but there's no read-side normalization (absent → 0, negative → 0, server MUST emit >= 0). Rules L39-43.
| #[serde(rename_all = "camelCase")] | ||
| #[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))] | ||
| #[non_exhaustive] | ||
| pub enum CacheScope { |
There was a problem hiding this comment.
https://modelcontextprotocol.io/seps/2549-TTL-for-list-results#cache-scope
This should have "public" and "private"
jamadeo
left a comment
There was a problem hiding this comment.
I think this does the right thing over the wire, but wouldn't it be a good bit simpler if added on the top level struct itself?
| pub struct $t { | ||
| #[serde(rename = "_meta", skip_serializing_if = "Option::is_none")] | ||
| pub meta: Option<Meta>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] |
There was a problem hiding this comment.
Why not add as new fields here and avoid the custom implementations of serialization/schema?
implementing #875
(assisted by mic and goose)