feat(core,client,server): ttl on list results (SEP-2549)#2070
feat(core,client,server): ttl on list results (SEP-2549)#2070felixweinberger wants to merge 3 commits into
Conversation
Client: ttl marks the tool-metadata cache stale (validators stay enforced; clamped to >=1s). isToolCacheStale getter for poll-based refresh.
felixweinberger
added
the
v2-stateless
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
@claude review |
1 similar comment
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /** | ||
| * Optional `ttl` (in seconds) included on `tools/list`, `prompts/list`, `resources/list`, | ||
| * and `resources/templates/list` responses (SEP-2549). Tells clients how long the response | ||
| * may be considered fresh; clients may cache and re-poll on this schedule. Supplements, does | ||
| * not replace, the `list_changed` notification mechanism. | ||
| */ | ||
| listTtlSeconds?: number; |
There was a problem hiding this comment.
🟡 listTtlSeconds is declared on ServerOptions (the low-level Server option type) but is only consumed by McpServer — a user writing new Server(info, { listTtlSeconds: 60 }) will see this JSDoc promising "included on tools/list ... responses" yet the option is silently ignored, since Server has no built-in list handlers. Consider qualifying the JSDoc with "Only applied by McpServer; low-level Server users must add ttl to list responses manually", or introducing an McpServerOptions extends ServerOptions type for the field.
Extended reasoning...
What the issue is
The new listTtlSeconds option is declared on ServerOptions in packages/server/src/server/server.ts:90-96, which is the constructor-options type for the low-level Server class. However, the Server constructor never reads this field — it reads options.capabilities, options.instructions, and options.jsonSchemaValidator, but not options.listTtlSeconds. The only consumer is McpServer at packages/server/src/server/mcp.ts:82-83, which reads it to populate its built-in list handlers.
This makes listTtlSeconds the first field on ServerOptions that the low-level Server silently ignores, breaking the prior contract that every ServerOptions field is consumed by Server itself.
How it manifests
A user of the low-level API writes:
const server = new Server({ name: 's', version: '1' }, { listTtlSeconds: 60 });
server.setRequestHandler('tools/list', () => ({ tools: [...] }));IntelliSense surfaces listTtlSeconds with the JSDoc "Optional ttl (in seconds) included on tools/list, prompts/list, resources/list, and resources/templates/list responses" — an unconditional promise. The user reasonably expects ttl: 60 to appear on their list responses. It does not: Server has no built-in list handlers and never injects anything into user-registered handler results, so the option is a silent no-op.
Why existing code doesn't prevent it
There is no separate McpServerOptions type — McpServer's constructor signature is (serverInfo, options?: ServerOptions), so the field had to land on ServerOptions to reach McpServer. That's a reasonable structural choice, but it means the JSDoc is now read by two audiences (low-level Server users and high-level McpServer users) while only being true for one of them. Nothing in the type system or runtime warns low-level users that the option is inert.
Step-by-step proof
- Construct
new Server(info, { listTtlSeconds: 60 }). Serverconstructor (server.ts) assigns_capabilities,_instructions,_jsonSchemaValidatorfromoptions—listTtlSecondsis never referenced.- User calls
server.setRequestHandler('tools/list', () => ({ tools: [] })). - Client sends
tools/list;Protocoldispatches directly to the user handler. No code path readslistTtlSecondsor spreads attlinto the result. - Response is
{ tools: [] }with nottl, contradicting the JSDoc.
Contrast with McpServer: its constructor stores Math.max(0, Math.round(options.listTtlSeconds)) in _listTtl and spreads ...this._ttl() into every built-in list handler result.
Impact
Low — Server is @deprecated ("Use McpServer instead"), and low-level users author their own list handlers anyway, so Server couldn't auto-inject ttl even if it wanted to. This is a documentation-accuracy / API-placement nit (matching the repo's recurring "prose that promises behavior the code doesn't ship" pattern), not a correctness bug.
Suggested fix
Either:
- Append to the JSDoc: "Only applied by {@linkcode McpServer}; the low-level
Serverdoes not register list handlers, so directServerusers must includettlon their list responses manually.", or - Introduce
type McpServerOptions = ServerOptions & { listTtlSeconds?: number }and move the field there, narrowing the type to the class that actually honors it.
| get isToolCacheStale(): boolean { | ||
| this._maybeExpireToolCache(); | ||
| return this._toolCacheStale; | ||
| } |
There was a problem hiding this comment.
🟡 The new public Client.isToolCacheStale getter (and the _maybeExpireToolCache / ≥1s-clamp / reset-on-listTools() logic behind it) has no test coverage — grepping for isToolCacheStale/toolCacheExpiresAt matches only client.ts. The PR description claims "New tests for ttl population + client staleness flag", but the added tests in mcp.test.ts only assert server-side ttl population; a fake-timer test exercising expiry, the clamp, and the stale-flag reset would close the gap.
Extended reasoning...
What's missing
Client gains a new public API surface in this PR:
get isToolCacheStale(): boolean— calls_maybeExpireToolCache()then returns_toolCacheStale._maybeExpireToolCache()— flips_toolCacheStale = trueand clears_toolCacheExpiresAtonceDate.now() >= _toolCacheExpiresAt.cacheToolMetadata(tools, ttlSeconds?)— now sets_toolCacheExpiresAt = Date.now() + Math.max(1, ttlSeconds) * 1000(the ≥1s clamp) and resets_toolCacheStale = falseon every freshlistTools().
Grepping the repo for isToolCacheStale|toolCacheStale|toolCacheExpiresAt returns only packages/client/src/client/client.ts. No test file — neither packages/client/test/** nor test/integration/** — references the getter or the underlying state.
Why the PR description is misleading
"How Has This Been Tested?" says: "New tests for ttl population + client staleness flag". The diff adds exactly two tests to test/integration/test/server/mcp.test.ts:
should include ttl on list results when listTtlSeconds is configured— assertsresult.ttl === 60on the four list endpoints, callingclient.request({ method: '…/list' })directly (which bypassesClient.listTools()and therefore never touchescacheToolMetadata).should omit ttl on list results when listTtlSeconds is not configured— asserts the property is absent.
Both are server-side wire-format tests. Neither calls client.listTools(), neither reads client.isToolCacheStale, and neither advances time. The "client staleness flag" half of the claim is not backed by the diff — this is the REVIEW.md recurring catch "prose that promises behavior the code no longer ships."
Step-by-step proof
rg -l 'isToolCacheStale'→packages/client/src/client/client.ts(1 hit, the definition).rg -l 'toolCacheExpiresAt|_toolCacheStale'→ same single file.- Inspect the two new
mcp.test.tscases: they useclient.request(…)(raw protocol), notclient.listTools(), so evencacheToolMetadata(tools, result.ttl)is never reached in those tests. - Therefore none of: (a)
isToolCacheStaleflipping totrueafter the ttl elapses, (b)Math.max(1, ttlSeconds)clamping a server-suppliedttl: 0, (c)_toolCacheStaleresetting tofalseon a freshlistTools(), is exercised anywhere.
Why it matters
REVIEW.md checklist: "New behavior has vitest coverage including error paths". isToolCacheStale is a new public getter on the Client class with non-trivial, time-dependent semantics (and a security-adjacent clamp whose comment says "so a server cannot force immediate expiry of the validator cache"). It also currently has zero callsites in the SDK, so tests are the only thing that would catch a regression. Shipping it untested while the PR description asserts otherwise is worth flagging.
Suggested fix
Add a vitest case (e.g. in packages/client/test/client/client.test.ts or alongside the new integration tests) using vi.useFakeTimers():
test('isToolCacheStale tracks SEP-2549 ttl', async () => {
vi.useFakeTimers();
const mcpServer = new McpServer({ name: 's', version: '1' }, { listTtlSeconds: 0 }); // exercises the ≥1s clamp
mcpServer.registerTool('t', {}, async () => ({ content: [] }));
const client = new Client({ name: 'c', version: '1' });
const [ct, st] = InMemoryTransport.createLinkedPair();
await Promise.all([client.connect(ct), mcpServer.connect(st)]);
await client.listTools();
expect(client.isToolCacheStale).toBe(false); // fresh after fetch
vi.advanceTimersByTime(500);
expect(client.isToolCacheStale).toBe(false); // clamp: 0 → 1s, not yet stale
vi.advanceTimersByTime(600);
expect(client.isToolCacheStale).toBe(true); // ttl elapsed
await client.listTools();
expect(client.isToolCacheStale).toBe(false); // reset on re-fetch
vi.useRealTimers();
});This covers the getter, the clamp, and the reset in one place and makes the PR description accurate.
…-expect-error markers for SEP-2549 ttl pending upstream
|
@claude review |
| /** | ||
| * Optional `ttl` (in seconds) included on `tools/list`, `prompts/list`, `resources/list`, | ||
| * and `resources/templates/list` responses (SEP-2549). Tells clients how long the response | ||
| * may be considered fresh; clients may cache and re-poll on this schedule. Supplements, does | ||
| * not replace, the `list_changed` notification mechanism. | ||
| */ | ||
| listTtlSeconds?: number; |
There was a problem hiding this comment.
🟡 This PR adds two new public API knobs (ServerOptions.listTtlSeconds and Client.isToolCacheStale) but does not add prose documentation for them under docs/ — the PR checklist's "I have added or updated documentation as needed" tick is currently inaccurate. A short section in docs/server.md (alongside the existing ## Server instructions section) and a mention of the staleness getter in docs/client.md would close the gap.
Extended reasoning...
What's missing
REVIEW.md's Tests & docs checklist says: "New feature: verify prose documentation is added (not just JSDoc), and assess whether examples/ needs a new or updated example." This PR introduces two user-facing API surfaces:
ServerOptions.listTtlSeconds(server.ts:90-96) — a new constructor option that causesMcpServerto attachttlto all four list responses.Client.isToolCacheStale(client.ts) — a new public getter that tells callers when the SEP-2549ttlhas elapsed and the tool list should be re-fetched.
Both have JSDoc, but neither appears in the prose docs.
Step-by-step proof
docs/exists and containsserver.md,client.md,server-quickstart.md,migration.md, etc.rg 'listTtlSeconds|isToolCacheStale|SEP-2549' docs/→ zero hits.rg 'listTtlSeconds|isToolCacheStale|SEP-2549' examples/→ zero hits.- The full-repo grep matches only the six source/test files in this diff.
- Sibling
ServerOptionsfields do have prose docs, establishing the convention:docs/server.mdhas a dedicated## Server instructionssection with example code, andjsonSchemaValidator/enforceStrictCapabilitiesappear indocs/server.md,docs/client.md, anddocs/migration.md. SolistTtlSecondsis the firstServerOptionsfield with no prose coverage at all. - The PR's own checklist ticks "[x] I have added or updated documentation as needed", while "[ ] Documentation update" under Types of changes is unticked — only JSDoc was added.
Why existing artifacts don't cover it
JSDoc surfaces in IntelliSense and the generated API reference, but the repo's review checklist explicitly distinguishes prose documentation from JSDoc, and the docs/*.md guides are where users learn that a feature exists in the first place. A user reading docs/server.md to discover "what options can I pass to McpServer?" will not see listTtlSeconds; a user reading docs/client.md to learn the caching behavior of listTools() will not learn that isToolCacheStale exists or that staleness is a re-fetch hint (validators stay enforced) rather than a cache-invalidation signal — a nuance worth spelling out in prose.
Impact
Documentation-completeness only — no runtime correctness impact, hence nit. The convention is also not perfectly uniform (not every ServerOptions field has its own ## heading), so this is a should-fix rather than a blocker. It is distinct from the three already-posted inline comments on this PR (spec.types.ts hand-edit, ServerOptions JSDoc placement, and missing client-side test coverage); none of those mention docs/.
Suggested fix
docs/server.md: add a short## List result TTL (SEP-2549)section near## Server instructions, showingnew McpServer(info, { listTtlSeconds: 60 })and noting that the value is rounded to a non-negative integer and supplements (does not replace)list_changednotifications.docs/client.md: add a paragraph under thelistTools()/ caching discussion explainingclient.isToolCacheStale, that it flipstrueonce the server-suppliedttlelapses, and that validators remain enforced while stale (it's a re-fetch hint, not a drop-enforcement signal).- Optionally untick the docs checkbox in the PR description until the above lands.
|
@claude review |
| nextCursor: CursorSchema.optional(), | ||
| /** | ||
| * How long (in seconds) this result may be considered fresh before re-fetching (SEP-2549). | ||
| * Allows clients to cache list responses and poll on a predictable schedule, supplementing | ||
| * (not replacing) the `list_changed` notification mechanism. | ||
| */ | ||
| ttl: z.number().int().nonnegative().optional() |
There was a problem hiding this comment.
🟡 This PR adds public API to three published packages — @modelcontextprotocol/core (PaginatedResult.ttl), @modelcontextprotocol/server (ServerOptions.listTtlSeconds), and @modelcontextprotocol/client (Client.isToolCacheStale) — but ships no .changeset/*.md entry (changeset-bot confirms "No Changeset found" at b8536fd). Without one, changesets/action will not bump versions or write CHANGELOG entries for any of these packages, so SEP-2549 ships silently; add a minor-bump changeset for core/client/server (the bot comment has the one-click link).
Extended reasoning...
What's missing
This PR adds new public API surface to three separately-published packages but contains no changeset file:
@modelcontextprotocol/core—PaginatedResultSchemagainsttl: z.number().int().nonnegative().optional()(schemas.ts:616-622), which propagatesttl?: numberonto the inferredPaginatedResult,ListToolsResult,ListResourcesResult,ListResourceTemplatesResult,ListPromptsResult, andListTasksResulttypes.@modelcontextprotocol/server—ServerOptionsgainslistTtlSeconds?: number(server.ts:90-96) andMcpServerreads it to populate every list response.@modelcontextprotocol/client—Clientgains a publicget isToolCacheStale(): booleangetter andcacheToolMetadatagrows attlSeconds?parameter.
The changeset-bot comment on the PR explicitly reports ".changeset/.
Why the repo expects one
.changeset/ already contains dozens of entries for directly comparable changes — e.g. add-resource-size-field.md (a single optional schema field, just like ttl) and respect-capability-negotiation.md — and .github/workflows/release.yml uses changesets/action to drive version bumps and CHANGELOG generation. Grepping .changeset/ for SEP-2549|listTtl|isToolCacheStale returns zero hits; the only ttl match is busy-rice-smoke.md, which is the unrelated "tasks - disallow requesting a null TTL" entry. So this PR is the first feature-level addition in the directory's history without a corresponding changeset.
Step-by-step proof
- PR diff touches
packages/{core,server,client}/src/**andtest/**— no.changeset/*.mdis added. - changeset-bot pinned comment at HEAD
b8536fd: "Merging this PR will not cause a version bump for any packages." .github/workflows/release.ymlrunschangesets/action; with no pending changeset for these packages, the next release PR will not include a version bump or CHANGELOG entry forcore/client/server.- Consumers therefore receive the new
ttlwire field, the newlistTtlSecondsoption, and the newisToolCacheStalegetter under an unchanged version number with no release-notes mention — exactly the silent-API-addition the changesets workflow exists to prevent.
Why nothing else covers it
This is distinct from the four inline comments already on the PR (spec.types.ts hand-edit — now resolved; ServerOptions JSDoc placement; missing client-side test coverage; missing docs/ prose). None of those mention changesets or the release workflow. The bot comment surfaces the gap on the PR page, but it does not block merge, and three @claude review rounds have gone by without it being addressed.
Impact
Process / release-hygiene only — there is no runtime defect, hence nit. The practical consequence is that SEP-2549 ships without a semver bump or CHANGELOG line, which (a) makes the feature undiscoverable in release notes and (b) means downstream lockfiles won't pick it up as a new version. Easy to fix and routinely caught at merge time, but worth flagging since the PR claims "non-breaking change which adds functionality", which is precisely the changeset minor use-case.
Suggested fix
Add a single changeset (the bot's "Click here if you're a maintainer who wants to add a changeset" link pre-fills it), e.g.:
---
"@modelcontextprotocol/core": minor
"@modelcontextprotocol/server": minor
"@modelcontextprotocol/client": minor
---
feat: SEP-2549 list-result `ttl` — `PaginatedResult.ttl`, `ServerOptions.listTtlSeconds`, `Client.isToolCacheStale`(@modelcontextprotocol/test-integration is private/test-only and does not need an entry; the bot's auto-suggested patch for it can be dropped.)
|
@claude review |
1 participant
Adds optional
ttl?: numbertoListToolsResult/ListResourcesResult/ListPromptsResult/ListResourceTemplatesResult.McpServerOptions.listTtlSecondspopulates it.Clientmarks its tool-metadata cache stale afterttlelapses (validators stay enforced; staleness is a re-fetch hint, not a drop-enforcement signal).ttlis clamped to ≥1s.Motivation and Context
Implements SEP-2549 (list freshness hint).
How Has This Been Tested?
New tests for ttl population + client staleness flag.
pnpm test:allpasses.Breaking Changes
None.
Types of changes
Checklist
Additional context
On
main(independent). Review guide: https://gist.github.com/felixweinberger/5a48e0f14d5aced39ed6a91b61940711.