Add story-style examples suite (27 stories + harness + CI)#2957
Conversation
c757395 to
cb295ad
Compare
There was a problem hiding this comment.
19 issues found across 159 files
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="examples/stories/legacy_routing/server_lowlevel.py">
<violation number="1" location="examples/stories/legacy_routing/server_lowlevel.py:14">
P3: `server_lowlevel.py` claims predicate export parity but does not re-export `classify_era`. This creates a docs/API mismatch for users following the legacy_routing recipe.</violation>
</file>
<file name="examples/stories/error_handling/server_lowlevel.py">
<violation number="1" location="examples/stories/error_handling/server_lowlevel.py:26">
P2: `divide` argument parsing can throw uncaught exceptions and emit a legacy `code=0` error instead of a controlled `INVALID_PARAMS` MCPError. Validate/coerce args and raise `MCPError` for bad input.</violation>
</file>
<file name="examples/stories/dual_era/server_lowlevel.py">
<violation number="1" location="examples/stories/dual_era/server_lowlevel.py:33">
P2: Do not use `assert` for request validation in handlers; it is stripped in optimized Python and disables tool/argument checks. Use explicit conditional validation and raise `MCPError(INVALID_PARAMS)` instead.</violation>
</file>
<file name="examples/stories/legacy_routing/server.py">
<violation number="1" location="examples/stories/legacy_routing/server.py:31">
P1: `classify_era` treats all `INVALID_PARAMS` as legacy, which misroutes malformed modern requests. Only requests with no modern envelope should fall back to legacy; malformed `_meta` should return the rejection.</violation>
</file>
<file name="examples/stories/_hosting.py">
<violation number="1" location="examples/stories/_hosting.py:34">
P2: `argv_after` fails on flags that are present but missing a value. Handle the missing-next-token case and raise `SystemExit` with a clear message.</violation>
</file>
<file name="examples/stories/bearer_auth/server.py">
<violation number="1" location="examples/stories/bearer_auth/server.py:15">
P2: `resource_server_url` is pinned to port 8000 even though the app can run on any `--port`. Changing the port makes auth metadata point clients at the wrong resource URL.</violation>
</file>
<file name="examples/stories/legacy_elicitation/README.md">
<violation number="1" location="examples/stories/legacy_elicitation/README.md:6">
P3: Opening banner implies `mrtr/` is already the active replacement, but `mrtr` is currently a stub. Clarify it as planned/not yet implemented.</violation>
</file>
<file name="examples/stories/bearer_auth/server_lowlevel.py">
<violation number="1" location="examples/stories/bearer_auth/server_lowlevel.py:33">
P2: Avoid `assert` for request validation in tool handlers. Use an explicit runtime branch so invalid tool names return a controlled MCP error result.</violation>
</file>
<file name="examples/stories/custom_methods/server.py">
<violation number="1" location="examples/stories/custom_methods/server.py:30">
P2: The handler does not bound `params.limit`, allowing a client to trigger excessive memory/CPU usage by requesting an arbitrarily large result set.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Partial review: This PR has more than 50 files, so cubic reviewed the highest-priority files first. During the trial, paid plans get a higher file limit.
You can try an ultrareview to bypass the file limit, comment @cubic-dev-ai ultrareview. Learn more.
Re-trigger cubic
cb295ad to
6dd50e7
Compare
There was a problem hiding this comment.
2 issues found across 21 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="examples/stories/legacy_routing/server_lowlevel.py">
<violation number="1" location="examples/stories/legacy_routing/server_lowlevel.py:14">
P3: `server_lowlevel.py` claims predicate export parity but does not re-export `classify_era`. This creates a docs/API mismatch for users following the legacy_routing recipe.</violation>
</file>
<file name="examples/stories/bearer_auth/server.py">
<violation number="1" location="examples/stories/bearer_auth/server.py:15">
P2: `resource_server_url` is pinned to port 8000 even though the app can run on any `--port`. Changing the port makes auth metadata point clients at the wrong resource URL.</violation>
</file>
<file name="examples/stories/stateless_legacy/README.md">
<violation number="1" location="examples/stories/stateless_legacy/README.md:21">
P2: `kill %1` is shell-job-control specific and fragile for README runbooks. Use PID-based kill to reliably stop the prior background server before reusing port 8000.</violation>
</file>
<file name="examples/stories/reconnect/README.md">
<violation number="1" location="examples/stories/reconnect/README.md:17">
P3: `kill %1` can target an unrelated job; this makes the README run sequence unreliable in shells with existing background jobs.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
066df09 to
e362d8f
Compare
- examples/stories/: 29 narrative example scripts, each a self-contained client+server scenario that can be read top-to-bottom and run directly - examples/pyproject.toml: workspace member so stories resolve against the in-tree SDK - tests/examples/: pytest harness that imports and runs every story under the existing coverage gate (in-memory, no subprocesses) - .github/workflows/shared.yml: wire the stories harness into the test job - pyproject.toml / uv.lock: register examples workspace member
- Delete examples/clients/* and examples/servers/simple-*, sse-polling-demo (each has an equivalent under examples/stories/) - examples/mcpserver/*.py: add TODO pointers to the corresponding story - README.v2.md, examples/snippets/clients/oauth_client.py: update paths that pointed at the removed examples - src/mcp/server/auth/middleware/bearer_auth.py: drop pragma now that the bearer_auth story exercises this path
- Revert removal of examples/{clients,servers/simple-*,servers/sse-polling-demo}
so the old examples remain alongside stories/ for now.
- schema_validators: use typing_extensions.TypedDict so pydantic accepts
it as a tool parameter on Python 3.10/3.11.
- pyright: add explicit extraPaths for examples/servers/simple-auth — the
mcp-example-stories editable install puts examples/ on sys.path, which
defeats pyright's package-root auto-detection for that one example.
The else arm had it; the if arm did not, so on 3.10 the unreachable import tomllib line counted as a miss.
- Invert the harness contract: each client.py now defines main(target, *, mode) and constructs Client(target, mode=...) itself, so the construction users came to see is in every example. The Connect factory, client_kw export, and needs_connect plumbing are gone. - Remove custom_version (the SDK has no supported-protocol-versions knob yet, so it could not show one) and client_session (an escape hatch we do not want a headline example for); dual_era keeps the negotiated-version callout. - Rename elicitation -> legacy_elicitation and add status (current / legacy / deprecated) to the manifest, surfaced in the story index, with README banners and migration notes on the legacy and deprecated stories. - dual_era servers note that one factory serves both eras with no configuration.
- tests/examples/test_story_shape.py: an AST check that every client.py constructs Client(...) inline in main's body, imports only the small harness allowlist, reaches no private mcp attributes, and that server_lowlevel.py never imports the high-level server module. - stories/README.md gains a "Canonical shape" section with the pasteable skeleton and the import rules; non-canonical stories state why in their module docstring; server names normalised to "<story>-example". - mrtr/ and subscriptions/ stubs note the lowlevel registration surface now exists upstream and they graduate once this branch's base has it.
Rebased onto main: mode='auto' is now the Client default, so the harness drops the workaround prose; the mrtr/subscriptions stubs note their lowlevel surface is in this base.
- legacy_elicitation (lowlevel): thread related_request_id through elicit_form/elicit_url so the request rides the originating POST's stream, and make elicitation ids unique per request. - legacy_routing: complete the CORS recipe (allow_methods/allow_headers) in both server variants. - Harness: HTTP-only stories exit with a friendly message instead of hanging when run without --http; drop the unused manifest smoke key; declare the tomli marker dependency on the examples package. - Docs: one two-variant run recipe everywhere, index rows describe the real Python surface, and assorted README corrections.
`kill %1` assumes the server is the only background job; `SERVER_PID=$!` plus `kill "$SERVER_PID"` is the same one-line recipe but immune to whatever else the shell has running.
`python -m stories.<story>.client --http` with no URL now starts the sibling server on a port it owns, waits for it to listen, runs the scenario, and tears it down. The two-variant run recipe becomes two commands with nothing to background, kill, or collide; `--http <url>` still targets a server you run yourself. The smoke test reuses the same path instead of carrying its own spawn/poll copy, so it now exercises exactly the command the READMEs print. Auth stories keep their pinned :8000 via a manifest `fixed_port`.
- error_handling: use the public e.data property and drop the now-covered pragma on MCPError.data; remove the README workaround note - serve_one: SingleExchangeContext.send_raw_request raises NoBackChannelError(method) per the DispatchContext contract - parallel_calls README: stop claiming progress-token demux on a shared wire (the example uses two separate connections) - roots/sampling READMEs: align Caveats and See-also with the SEP-2577 deprecation banner instead of pointing at MRTR as a successor - starlette_mount README: correct the forgotten-lifespan failure mode (immediate 500 with RuntimeError, not a hang) - _harness: pin era from the manifest when a story is single-era - README spec links: /specification/2026-07-28/ -> /specification/draft/
…ify_inbound_request `classify_inbound_request` now also requires the `Mcp-Method` header to match the body's method (and `Mcp-Name` for name-bearing methods). The story's "shown directly" demonstration hand-builds a modern header set, so it now carries `Mcp-Method` too; the real HTTP arms already did via `Client`.
71bd40c to
fa9c543
Compare
The code demonstrates that both Mcp-Protocol-Version and Mcp-Method must mirror the body; say so in the prose that describes the rejection arm.
| MCP_EXPOSED_HEADERS = ["Mcp-Session-Id", "WWW-Authenticate", "Last-Event-Id", "Mcp-Protocol-Version"] | ||
| #: Request headers a browser-based MCP client must be allowed to send. | ||
| MCP_ALLOWED_HEADERS = ["Authorization", "Content-Type", "Mcp-Protocol-Version", "Mcp-Session-Id", "Last-Event-Id"] | ||
| #: Streamable HTTP verbs: POST requests, the standalone GET stream, DELETE session end. | ||
| MCP_ALLOWED_METHODS = ["GET", "POST", "DELETE"] |
There was a problem hiding this comment.
🔴 MCP_ALLOWED_HEADERS omits Mcp-Method and Mcp-Name, but every modern (2026-07-28) SDK request carries Mcp-Method (and Mcp-Name for name-bearing methods like tools/call), so a browser preflight including those headers is rejected by Starlette's CORSMiddleware and modern browser clients are blocked by the very CORS recipe this story teaches. Adding 'Mcp-Method' and 'Mcp-Name' to the list fixes both server.py and server_lowlevel.py (which reuses the constant).
Extended reasoning...
The bug. MCP_ALLOWED_HEADERS = ["Authorization", "Content-Type", "Mcp-Protocol-Version", "Mcp-Session-Id", "Last-Event-Id"] (examples/stories/legacy_routing/server.py:18) is passed as allow_headers to Starlette's CORSMiddleware in both server.py and server_lowlevel.py (which imports the constant). It omits Mcp-Method and Mcp-Name, which every modern-era request from the SDK client sends.
Why those headers are part of the modern wire contract, not optional decoration. src/mcp/client/session.py:82-85 (_make_modern_stamp) sets headers[MCP_METHOD_HEADER] = data["method"] on every modern request and headers[MCP_NAME_HEADER] = encode_header_value(name) for name-bearing methods like tools/call; line 434 also sends Mcp-Method on the discover probe. On the server side, the very classifier this story teaches enforces them: src/mcp/shared/inbound.py:306-318 rejects a modern request with a header-mismatch error when Mcp-Method (or Mcp-Name) does not mirror the body. The story's own client.py builds modern_headers with MCP_METHOD_HEADER to demonstrate exactly that. So a 2026-era browser client necessarily includes mcp-method (and often mcp-name) in its requests — and therefore in its CORS preflight's Access-Control-Request-Headers.
The code path that fails. With Starlette's CORSMiddleware, when allow_headers is a restricted list (not ["*"]), a preflight OPTIONS request whose Access-Control-Request-Headers contains a header that is neither CORS-safelisted (Accept, Accept-Language, Content-Language, Content-Type) nor in the explicit list fails the preflight (Starlette returns a 400 "Disallowed CORS headers" response). mcp-method and mcp-name are neither safelisted nor listed, so the preflight is refused and the browser never sends the actual POST.
Step-by-step proof. (1) A browser-based MCP client at https://app.example.com copies this recipe and connects to the dual-era /mcp route in modern mode. (2) The SDK-equivalent client stamps Mcp-Protocol-Version, Mcp-Method: tools/call, and Mcp-Name: <tool> on the POST. (3) Because the request is cross-origin with non-safelisted headers, the browser first sends OPTIONS /mcp with Access-Control-Request-Headers: content-type, mcp-protocol-version, mcp-method, mcp-name. (4) CORSMiddleware checks each requested header against MCP_ALLOWED_HEADERS + the safelist; mcp-method and mcp-name are not allowed, so the preflight fails. (5) The browser blocks the POST entirely — the modern era of the dual-era app is unreachable from a browser, while legacy (handshake-era) requests, which don't carry these headers, still work.
Why nothing catches it. The README explicitly advertises this story as 'the CORS recipe (methods, request headers, and expose_headers) browser-based MCP clients need', and the same app intentionally serves both eras on one route, so modern browser callers are an expected audience. But the in-process test matrix drives the app through an httpx ASGI bridge that never issues a CORS preflight, so the omission is invisible to CI.
Impact and fix. This is a teaching-surface defect: users who copy the recipe get a CORS configuration that blocks 2026-era browser clients while appearing to work in non-browser testing. The fix is one line — add 'Mcp-Method' and 'Mcp-Name' to MCP_ALLOWED_HEADERS in server.py; server_lowlevel.py picks it up automatically since it imports the constant.
| server = build_server() | ||
| async with server.lifespan(server) as lifespan_state: | ||
| async with stdio_server() as (read_stream, write_stream): | ||
| dispatcher: JSONRPCDispatcher[TransportContext] = JSONRPCDispatcher( | ||
| read_stream, write_stream, inline_methods=frozenset({"initialize"}) | ||
| ) | ||
| connection = Connection.for_loop(dispatcher) | ||
| await serve_connection(server, dispatcher, connection=connection, lifespan_state=lifespan_state) | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| anyio.run(main) |
There was a problem hiding this comment.
🟡 Following the suite-wide convention from the top-level stories README, python -m stories.serve_one.client --http self-hosts server.py --http --port <N>, but this file's __main__ is anyio.run(main) — the hand-built stdio loop — which never reads sys.argv, never binds the port, and never exits, so the harness's readiness poll spins until the 30s fail_after fires. Either honor the run_server_from_args-style argv convention here, or have run_client refuse --http for this story (e.g. via a manifest flag) with a clear message like the existing server_export == 'app' / needs_http guards.
Extended reasoning...
What happens. The top-level examples/stories/README.md states that bare --http "is the canonical HTTP run — it is complete on its own, and it is what every per-story README shows." For serve_one, the manifest entry only sets transports = ["in-memory"] and lowlevel = false, so it inherits the default server_export = "factory". run_client in _harness.py only guards on server_export == "app" and needs_http (both false here) and never consults transports, so python -m stories.serve_one.client --http falls into _self_hosted().
The code path. _self_hosted spawns python -m stories.serve_one.server --http --port <free-port> (the --http flag is included because server_export == "factory") and then polls while server.returncode is None and not await _accepting(port). But serve_one/server.py's __main__ is anyio.run(main) — the hand-built JSONRPCDispatcher + serve_connection stdio loop that is the story's teaching point. It never inspects sys.argv, so --http --port <N> are silently ignored: the process blocks reading the stdin pipe that anyio.open_process keeps open, never binds the port, and never exits.
Step-by-step. (1) User reads the suite README and runs python -m stories.serve_one.client --http. (2) run_client passes both SystemExit guards and enters _self_hosted. (3) The spawned server runs the stdio loop, ignoring the flags. (4) The readiness loop spins: returncode stays None and _accepting(port) stays false. (5) After 30 seconds anyio.fail_after(cfg["timeout_s"]) raises TimeoutError, and the user gets FAIL: serve_one (http/modern) plus a traceback — exactly the silent-hang failure mode _self_hosted's own docstring promises to avoid ("a loud SystemExit rather than a hang").
Why nothing prevents it. The other unsupported combinations get clear, immediate SystemExit messages: app-export stories on stdio, and needs_http stories without --http. There is no equivalent guard for "this story's server has no HTTP entry," because the manifest's transports field only constrains the pytest matrix, not run_client. serve_one's own README documents only the stdio command, which limits exposure, but the top-level README explicitly invites the bare---http invocation for every story.
Impact. Examples-scaffold ergonomics only — no SDK code or CI path is affected (the smoke test never runs serve_one over HTTP, and the in-memory matrix is untouched). The failure is loud (exit 1) but slow and confusing: a 30-second hang followed by a TimeoutError instead of a one-line explanation.
How to fix. Either (a) have run_client/_self_hosted consult the manifest (e.g. the transports list or a dedicated flag) and refuse --http for stories whose server has no HTTP entry, with a message pointing at the stdio command; or (b) make serve_one/server.py's __main__ honor the --http --port argv convention (it already builds a lowlevel.Server, so run_server_from_args(build_server) would work — though that trades away the hand-built-entry teaching point, so a guard or a one-line note in the top-level README is probably the lighter touch).
| | [`json_response`](json_response/) | `json_response=True` mode; raw 2026 POST envelope on the wire | current | | ||
| | [`legacy_routing`](legacy_routing/) | `classify_inbound_request()` era routing in front of a sessionful 1.x deploy | current | | ||
| | [`starlette_mount`](starlette_mount/) | mounting `streamable_http_app()` under a Starlette/FastAPI sub-path | current | | ||
| | [`sse_polling`](sse_polling/) | SEP-1699 `closeSSE()` + `Last-Event-ID` resume via `EventStore` | legacy | |
There was a problem hiding this comment.
🟡 The story index table describes sse_polling as "SEP-1699 closeSSE() + Last-Event-ID resume via EventStore", but closeSSE() is the TypeScript-SDK spelling and doesn't exist anywhere in this repository — the Python API the story actually teaches is ctx.close_sse_stream(). Update the table entry to close_sse_stream() (or describe the SEP feature without code formatting) so readers grepping the SDK find the real name.
Extended reasoning...
The issue. Line 147 of examples/stories/README.md (the story index table) describes the sse_polling story as "SEP-1699 closeSSE() + Last-Event-ID resume via EventStore". A repo-wide grep shows closeSSE appears exactly once in the entire repository — on this line. It is the TypeScript-SDK / SEP-1699 camelCase spelling; no such function is defined or exported anywhere in src/mcp or in the example code.
What the correct name is. The Python API the sse_polling story teaches is close_sse_stream(): the close_sse_stream callback on ServerRequestContext (src/mcp/server/context.py:41), wired by the streamable-HTTP transport (src/mcp/server/streamable_http.py), and called as await ctx.close_sse_stream() in examples/stories/sse_polling/server.py and server_lowlevel.py. The sse_polling story's own README consistently spells it close_sse_stream() as well — only this index entry uses the foreign spelling.
How a reader hits it. The index table otherwise uses real Python SDK names in code formatting (classify_inbound_request(), streamable_http_app(stateless_http=True), get_access_token(), add_request_handler, ...), so a code-formatted closeSSE() reads as a Python API. Step-by-step: (1) read the sse_polling row in the index; (2) grep the SDK for closeSSE — zero hits outside this README line; (3) open sse_polling/server.py — the call there is ctx.close_sse_stream(). The cross-reference points at a name that does not exist in this SDK.
Why nothing catches it. The story suite's checks (tests/examples/test_story_shape.py, the manifest consistency tests) validate code shape and manifest/filesystem agreement, not prose in READMEs, so a stale function name in the index table passes CI silently.
Why this is intentional-looking but still worth fixing. One could argue closeSSE() is deliberately the SEP-1699 spelling, but nothing in the table marks it as a TypeScript-SDK or spec-level name, and every neighboring entry uses the Python spelling. It is also the same class of issue as the is_legacy_request() stale-name reference in stateless_legacy/README.md that was already accepted and fixed — this is the remaining occurrence of a code-formatted function name that doesn't exist in the SDK.
Impact and fix. Doc-only, no runtime effect. One-word change in the table entry, e.g.: | sse_polling | SEP-1699 close_sse_stream() + Last-Event-ID resume via EventStore | legacy | — or drop the code formatting and refer to "SEP-1699 server-initiated SSE close".
Adds a story-style examples suite: one self-contained folder per protocol feature, each with
server.py(MCPServer),server_lowlevel.py(lowlevelServer),client.py, andREADME.md. Mirrors the typescript-sdkv2-2026-07-28examples layout.Motivation and Context
The existing examples are spread across four parallel layouts and only
everything-serverruns in CI. We want examples that:MCPServerand lowlevelServervariants wherever the feature allowsWhat's in this PR
examples/stories/: tools, prompts, resources, lifespan, dual_era, streaming, legacy_elicitation, sampling, stickynotes, custom_methods, schema_validators, middleware, parallel_calls, roots, pagination, error_handling, serve_one, stateless_legacy, json_response, legacy_routing, starlette_mount, sse_polling, standalone_get, reconnect, bearer_auth, oauth, oauth_client_credentials — 23 of them with a lowlevelServersibling.client.pyconstructs theClientinline. The body isasync def main(target, *, mode="auto"): async with Client(target, mode=mode) as client: ...— the one line a client example exists to teach is visible in every file, and the harness only decides whattargetis. An AST shape-check (tests/examples/test_story_shape.py) keeps it that way.tests/examples/):manifest.tomlparametrizes each story across (server variant × transport × era) — 266 legs in ~1.5s, all in-memory or ASGI-bridged, no subprocesses or sleeps. Plus a 3-leg subprocess smoke test (gated onMCP_EXAMPLES_SMOKE=1) that runs the literal documented commands over real stdio/uvicorn.python -m stories.<story>.client --httpwith no URL starts the sibling server on a port it owns, waits for it, runs the scenario, and tears it down — nothing to background, kill, or collide.--http <url>still targets a server you run yourself, and stays the documented form in the stories where hosting is the lesson.status = current | legacy | deprecated, surfaced in the story index; legacy-mechanism stories (e.g.legacy_elicitation, the handshake-era push that MRTR replaces) and deprecated features (roots,sampling, the logging half ofstreaming) open their READMEs with a banner and a migration note._harness.py(client-side) and_hosting.py(server-side, isolating the HTTP entry calls still being reshaped so one edit propagates),_shared/auth.py(in-process AS for the auth stories). Singleexamples/pyproject.tomlworkspace member, sofrom stories.tools.server import build_serverworks in scripts,-m, and pytest.examples/clients/,examples/servers/*,examples/mcpserver/, andexamples/snippets/directories are left in place for now.How Has This Been Tested?
./scripts/test: 100% coverage,strict-no-coverclean (net −6# pragma: no coverinsrc/— the stories exercise previously-uncovered branches)pytest tests/examples/: 261 passed, 2 xfail (a documented progress-drop on the modern HTTP path), 3 skipped (smoke without the env var)MCP_EXAMPLES_SMOKE=1 pytest tests/examples/test_stories_smoke.py: 3 passed over real subprocess stdio + uvicornBreaking Changes
None.
Types of changes
Checklist
Additional context
Intentional behavior changes
src/mcp/server/{elicitation,lowlevel/server,mcpserver/server}.py,src/mcp/shared/exceptions.py: removed 6# pragma: no covermarkers on branches the new stories now exercise. No logic change.Known caveats / follow-ups
mrtr/andsubscriptions/are stubs even though their lowlevel registration surface now exists onmain(lowlevel Server: widen on_* return types for InputRequiredResult; add subscriptions/listen slot #2967); implementing them is a deliberate follow-up to keep this PR reviewable. Same for the deferred extension stories.Client(stdio_params)overload, noauth=passthrough onClient(url), no publicMCPServermiddleware accessor, no server-side supported-protocol-versions override). The stories work around them in the harness, never in the user-facing files; tracked separately.examples/{clients,servers,mcpserver}/trees intostories/is a follow-up.AI Disclaimer