docs: cover tool content types, prompt image content, elicitation defaults, ping, legacy SSE serving - #2679
Conversation
…aults, ping, legacy SSE serving No-Verification-Needed: docs and guide-companion examples only
|
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… Express options No-Verification-Needed: docs and guide-companion examples only
…bare host No-Verification-Needed: docs and guide-companion examples only
There was a problem hiding this comment.
Beyond the one new inline note, this round re-verified that the fixes from the earlier reviews all landed (400 vs 404 branching on sessionId, allowedHosts + jsonLimit: '4mb' on the SSE app, encodeURIComponent in the embedded-resource URI) and spot-checked the new factual claims: ping is present in the 2025-11-25 spec types and absent from 2026-07-28, matching the new protocol-versions row, and the elicitation.form.applyDefaults flag the docs describe exists in packages/client/src/client/client.ts.
Extended reasoning...
This is a findings-present run with one new minor [quality] inline comment (missing self-verification in the doc-quoted harness rounds). Rather than restate it, the body records what else was concretely checked this round: all three prior-round findings were confirmed fixed in the current HEAD of examples/guides/serving/legacy-clients.examples.ts, docs/serving/legacy-clients.md, and examples/guides/servers/tools.examples.ts, and the two most checkable new factual claims (the ping era-table row against the 2025 vs 2026 spec type files, and the client-side applyDefaults capability flag) were verified against the source. Not approving because a quality finding is being posted this run and the repo's CODEOWNERS assigns all paths to a team owner, so a human pass is expected regardless.
No-Verification-Needed: docs and guide-companion examples only
There was a problem hiding this comment.
LGTM — all points from the earlier review rounds are now addressed, and this run over the latest push found no new issues.
What was reviewed: confirmed the SSE example now splits 400 (missing sessionId) from 404 (unknown session), passes { host: '0.0.0.0', allowedHosts, jsonLimit: '4mb' } (and jsonLimit is a real createMcpExpressApp option), and the docs prose matches; confirmed the embedded-resource URI now uses encodeURIComponent with the quoted output updated to Travel%20mug; confirmed the four new harness rounds (ping, elicitation defaults, prompt image, tool content types) assert the exact output the pages quote and throw on mismatch; checked the examples/ package.json / tsconfig / lockfile plumbing for the server-legacy workspace link — consistent with the existing path-mapping pattern.
Extended reasoning...
Overview
Docs-only PR: five guide pages gain short sections (client ping, elicitation default + applyDefaults, prompt image content, mixed tool content blocks, mounting the frozen v1 SSEServerTransport on Express), each backed by new //#region blocks in the corresponding examples/guides/*.examples.ts companion, plus a one-row addition to the docs/protocol-versions.md era table and workspace plumbing (examples/package.json, examples/tsconfig.json, pnpm-lock.yaml) for the @ modelcontextprotocol/server-legacy dependency. No package source code changes.
Security risks
The only security-relevant surface is the copy-paste SSE mounting recipe, which earlier rounds of this review flagged (bare host value silently disabling Host/Origin validation, conflated 400/404 session handling, 100kb body cap). Commits 465dc34 and 472c0ce fixed all three: the example now uses { host: '0.0.0.0', allowedHosts: ['sse.example.com'], jsonLimit: '4mb' } matching the express.md recipe, and the POST route branches typeof sessionId !== 'string' → 400 before the map lookup → 404, exactly per the Recurring Catches rule (#1707/#1770). I verified jsonLimit is a real option in packages/middleware/express/src/express.ts. No other injection/auth/data-exposure surface exists in a docs diff.
Level of scrutiny
Moderate scrutiny was appropriate because documented recipes get copied verbatim into deployments, and three prior review rounds applied it — every finding was fixed in a follow-up commit, and the final push (aabf3c4) added the missing self-verification assertions so pnpm docs:examples now fails if any quoted output rots (ping {}, Exported quarterly-sales as pdf., image mimeType, image,audio,resource block order). The bug hunt on the latest push ran dry with zero findings and zero ruled-out candidates.
Other factors
The companions are typechecked and executed in CI (docs:examples gate), the fence bodies match their regions in the diff, and the URI-encoding fix is reflected in the quoted output (catalog://products/Travel%20mug). CODEOWNERS only has a repo-wide default team plus auth-path entries that this diff does not touch. No unresolved third-party objections appear in the timeline — the only review activity is this bot's own, and every raised item is verifiably fixed in the code. Given the docs-only scope, self-verifying CI coverage, and the completed feedback loop, I have high confidence no human needs to re-review this.
There was a problem hiding this comment.
LGTM — all feedback from the earlier review rounds has been addressed, and a fresh pass over the current diff found no new issues.
What was reviewed: the reworked SSE mounting example now branches 400 (missing sessionId) vs 404 (unknown session), uses host: '0.0.0.0' with allowedHosts per the express.md recipe, and raises jsonLimit to the transport's 4mb; the embedded-resource URI now uses encodeURIComponent (docs quote Travel%20mug); each new doc-quoted round (ping, applyDefaults, prompt image, product-card content types) now has a throwing assertion so pnpm docs:examples catches drift; fenced code in all five pages byte-matches the companion //#region blocks, and the new ping era row lives only in the protocol-versions table.
Extended reasoning...
Overview
This is a docs-only PR: five new guide sections (client ping, elicitation default/applyDefaults, prompt image content, mixed tool content blocks, legacy SSEServerTransport Express mounting) with matching type-checked companion snippets under examples/guides/, plus plumbing (examples/package.json, examples/tsconfig.json, pnpm-lock.yaml) to let the companions typecheck against @ modelcontextprotocol/server-legacy. No SDK source, transport, or auth code is touched.
Security risks
The only security-relevant surface is the documented SSE deployment recipe, which earlier rounds of this review flagged twice (Host/Origin validation silently disabled by a bare non-localhost host value; body-parser limit below the transport's 4mb). The current code fixes both: createMcpExpressApp({ host: '0.0.0.0', allowedHosts: ['sse.example.com'], jsonLimit: '4mb' }) matches the repo's own express.md recipe and the accompanying prose explains why. The session routing now distinguishes 400 (missing param) from 404 (unknown session), satisfying the REVIEW.md recurring catch for HTTP transports. Nothing else in the diff executes at runtime outside CI.
Level of scrutiny
This PR received four full review rounds already; each finding was fixed by a follow-up commit visible in the current diff. This run's bug hunt exited on a dry streak with zero findings and zero ruled-out candidates. I re-read the entire diff and verified every previously raised item against the current code rather than trusting thread resolutions (which were bot-session resolutions). With all findings demonstrably closed in code and the change confined to docs plus self-verifying, CI-executed examples, remaining risk is low enough that a human pass is not required.
Other factors
The companions are executed by pnpm docs:examples in CI and now assert every quoted output (ping {}, applyDefaults producing 'Exported quarterly-sales as pdf.', the image block round-trip, the image/audio/resource block sequence), so documentation drift will fail CI rather than rot silently. The era-difference claim for ping was added to the single canonical table in docs/protocol-versions.md rather than duplicated, per that file's stated convention. The workspace-dependency and tsconfig path additions follow the existing pattern for sibling packages exactly.
claude
Bot
added
the
v2
1 participant
Documents the last non-experimental features missing from the v2 guide site: tool results with
image/audio/ embeddedresourcecontent, prompt messages with image content, elicitationdefaultvalues,ping, and serving the frozenSSEServerTransportfrom@modelcontextprotocol/server-legacy/sse.Motivation and Context
The SEP-1730 documentation check for v2 came back 41/48: these seven features had no page or no example on
main(v1.x'sdocs/protocol.mdcovered ping and content types; the v2 site did not). Each addition is a short section on the existing page with a synced, executed companion example.How Has This Been Tested?
pnpm sync:snippets --check,pnpm --filter @modelcontextprotocol/examples typecheck,pnpm docs:examples(37 run / 0 failed — every quoted output block is real companion output),pnpm docs:build.Breaking Changes
None.
Types of changes
Checklist
Additional context
The SSE serving snippet adds
@modelcontextprotocol/server-legacyas a workspace dependency ofexamples/(plus atsconfigpath mapping) so the companion typechecks against source like the other packages.