Add the todos-server reference example#3048
Conversation
![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 8 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/servers/todos-server/mcp_todos_server/todos.py">
<violation number="1" location="examples/servers/todos-server/mcp_todos_server/todos.py:643">
P2: `complete_task` can complete the wrong task when called with an empty `task` argument, because empty-string substring matching succeeds on the first title. Validating non-empty input before substring lookup would prevent unintended board mutations.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| task: Annotated[str, Field(description="Task id, or part of its title")], | ||
| ctx: Context, | ||
| ) -> CallToolResult: | ||
| needle = task.lower() |
There was a problem hiding this comment.
P2: complete_task can complete the wrong task when called with an empty task argument, because empty-string substring matching succeeds on the first title. Validating non-empty input before substring lookup would prevent unintended board mutations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At examples/servers/todos-server/mcp_todos_server/todos.py, line 643:
<comment>`complete_task` can complete the wrong task when called with an empty `task` argument, because empty-string substring matching succeeds on the first title. Validating non-empty input before substring lookup would prevent unintended board mutations.</comment>
<file context>
@@ -0,0 +1,824 @@
+ task: Annotated[str, Field(description="Task id, or part of its title")],
+ ctx: Context,
+) -> CallToolResult:
+ needle = task.lower()
+ found = tasks.get(task) or next(
+ (candidate for candidate in tasks.values() if needle in candidate.title.lower()), None
</file context>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| async def handle_completion( | ||
| ref: PromptReference | ResourceTemplateReference, | ||
| argument: CompletionArgument, | ||
| context: CompletionContext | None, | ||
| ) -> Completion | None: | ||
| if isinstance(ref, PromptReference): | ||
| if ref.name == "seed-board" and argument.name == "theme": | ||
| return Completion(values=[theme for theme in THEME_SUGGESTIONS if theme.startswith(argument.value)]) | ||
| if ref.name == "plan-my-day" and argument.name == "focus": | ||
| return Completion(values=[project for project in projects() if project.startswith(argument.value)]) | ||
| if isinstance(ref, ResourceTemplateReference) and ref.uri == TASK_URI_TEMPLATE and argument.name == "id": | ||
| return Completion(values=[task_id for task_id in tasks if task_id.startswith(argument.value)]) | ||
| return None |
There was a problem hiding this comment.
🟡 The task-id completion for todos://tasks/{id} returns every matching task id with no cap, but completion values are limited to 100 items by the spec (and enforced by the 2026-07-28 wire model's max_length=100), so once the board exceeds 100 tasks a short/empty prefix produces an oversized result — spec-violating on 2025-era connections and a failed request on 2026-era ones. Slicing the list to 100 (optionally with total/has_more) matches what the TypeScript reference does.
Extended reasoning...
What the bug is. In handle_completion (todos.py:444-456), the ResourceTemplateReference branch for the id argument returns Completion(values=[task_id for task_id in tasks if task_id.startswith(argument.value)]) — every matching task id, uncapped. The MCP spec limits completion.values to at most 100 entries, and the SDK's own Completion docstring in mcp_types._types says "Must not exceed 100 items". The other two completion branches (theme suggestions, project names) stay small naturally, but the task-id one scales with the board.
How the board exceeds 100 tasks. brainstorm_tasks accepts counts up to 100 per call, add_tasks is unbounded, and the module-level tasks dict accumulates across calls — and, over Streamable HTTP, across sessions in the same process. All ids share the t prefix (t1, t2, …), so a completion request for the id argument with an empty prefix or just t matches the whole board.
Why nothing else prevents it. The @mcp.completion() plumbing (src/mcp/server/mcpserver/server.py:698-705) wraps the returned Completion verbatim into a CompleteResult with no truncation, and the SDK-level Completion model has no max_length constraint despite its docstring. However, the 2026-07-28 wire surface does enforce it: mcp_types/v2026_07_28/__init__.py:92 declares values with Field(max_length=100), and server results are serialized against the versioned surface (_methods.serialize_server_result, called from server/runner.py).
Impact. With >100 tasks on the board:
- On a 2025-era connection, the server emits a spec-violating completion result with more than 100 values and no
total/hasMorepagination hints. - On a 2026-07-28 connection, serialization against the versioned wire model raises a
ValidationError, so thecompletion/completerequest fails with an internal error instead of returning any values at all.
This also diverges from the TypeScript todos-server this PR ports: the TS SDK's completion path truncates to 100 values and sets total/hasMore, so the TS server never emits an oversized completion.
Step-by-step proof. 1) Client calls brainstorm_tasks, answers the count elicitation with custom → 100, and the sampling round returns 100 lines — the board now holds 100 tasks (t1…t100). 2) Client calls add_task once more → 101 tasks. 3) Client sends completion/complete with ref = {type: "ref/resource", uri: "todos://tasks/{id}"}, argument = {name: "id", value: "t"}. 4) Every id starts with t, so the list comprehension yields 101 values. 5) On a 2026-07-28 connection, CompleteResult serialization against the v2026_07_28 model trips max_length=100 and the request errors out; on a 2025-era connection, a 101-value result goes on the wire, exceeding the spec limit.
How to fix. Slice the matches to 100, e.g. matches = [task_id for task_id in tasks if task_id.startswith(argument.value)] then return Completion(values=matches[:100], total=len(matches), has_more=len(matches) > 100) — mirroring the TypeScript reference's behaviour.
Severity. This is example-only code, requires accumulating more than 100 tasks plus a short completion prefix, and the fix is a one-liner — nice to fix for parity and spec compliance, but not merge-blocking.
12bc70f to
d1812a9
Compare
A Python port of the TypeScript SDK's examples/todos-server: a small project todo board where every server-side MCP feature has a real job — CRUD tools, structured output, resources and a task template, prompts with completions, sampling- and elicitation-backed interactive tools written once as input_required state machines with a legacy fulfilment driver, progress, request-tied logging, and per-resource subscriptions — served to both protocol revisions over stdio and Streamable HTTP.
Rewrite clear_done, prioritize, and brainstorm_tasks from hand-rolled InputRequiredResult state machines onto Resolve dependencies, now that Sample landed alongside Elicit: the framework carries the rounds on 2026-07-28 connections and push-style requests on pre-2026 ones, seals the multi-round state, validates answers against the form schemas, and gates on client capabilities. This deletes the example's local legacy fulfilment driver, literal schema dicts, and JSON step state. The elicitation forms render byte-identical to the TypeScript reference's literals; the count-form theme field advertises its default via json_schema_extra while staying nullable, so an omitted answer still falls back to the tool argument like the reference. Resolver outcomes, not board recounts, drive the empty-board paths so concurrent mutations cannot desync the resolver from the tool body.
Re-verified against the TypeScript reference at its current main: the missing-capability shape is now a -32021 protocol error on both servers on 2026-07-28 connections (README fidelity note updated), the TS entry's protected HTTP wiring is matched by the SDK's streamable HTTP defaults (noted in server.py), and 2026-era watchers are pointed at the client's listen() driver. Also renames the module-level task store so the add_tasks parameter no longer shadows it, folds the brainstorm outcome union into one alias, types the log threshold as LoggingLevel, and makes comments and docstrings ASCII-only.
d1812a9 to
a05dd3d
Compare
There was a problem hiding this comment.
Beyond the two inline nits, this pass also examined and ruled out: empty-string complete_task arguments completing an arbitrary task (intentionally matches the TS reference's substring semantics, per the PR's adversarial-probe verification); per-task todos://tasks/{id} subscriptions being accepted but never producing update notifications; and the same cross-thread tasks_by_id iteration concern for the sync board resource reader.
Extended reasoning...
This run's bug hunt surfaced two nits (sync resolvers iterating shared state from a worker thread, and warnings.catch_warnings held across an await in log_info), which are posted as inline comments. Verifiers also investigated and refuted three further candidates, recorded in the message so later passes don't re-explore them from scratch. Note the prior review's uncapped task-id completion comment (todos.py line ~378) still applies to the current revision — not restating it here since it's already on the thread. This is informational only, not a correctness guarantee.
| def confirm_clear() -> Elicit[ClearConfirmation] | ClearConfirmation: | ||
| """Ask only when there is something to delete. | ||
|
|
||
| An empty board resolves to a non-confirmation with no round-trip, so even if tasks complete | ||
| concurrently before the tool body runs, nothing is deleted without the user being asked. | ||
| """ | ||
| done = sum(1 for task in tasks_by_id.values() if task.status == "done") | ||
| if done == 0: | ||
| return ClearConfirmation(confirm=False) | ||
| return Elicit(f"Delete {done} completed task(s) from the board?", ClearConfirmation) |
There was a problem hiding this comment.
🟡 The sync resolvers (confirm_clear, rank_open_tasks, and the brainstorm chain) run on a worker thread via anyio.to_thread.run_sync, where they iterate the module-global tasks_by_id while the event-loop thread stays free to mutate it (add_task_record inserts, clear_done pops) — a mutation landing mid-iteration raises RuntimeError("dictionary changed size during iteration") and fails the whole tools/call. Snapshot with list(tasks_by_id.values()) before iterating, or declare the resolvers async so they run on the event loop like the tool bodies.
Extended reasoning...
What the bug is. The three interactive tools' resolvers are plain sync functions: confirm_clear (todos.py:590-599) iterates tasks_by_id.values() via sum(...), rank_open_tasks (todos.py:620-622) iterates it via open_tasks(), and the brainstorm chain follows the same pattern. Because they are sync, the resolver framework dispatches them off the event loop: src/mcp/server/mcpserver/resolve.py:556 does result = await anyio.to_thread.run_sync(lambda: fn(**kwargs)). So these functions iterate the module-global tasks_by_id dict from a worker thread while the event-loop thread remains free to run other handlers that mutate the same dict — add_task_record inserts (tasks_by_id[created.id] = created) and clear_done deletes (tasks_by_id.pop).
Why this can crash. CPython raises RuntimeError('dictionary changed size during iteration') whenever a dict iterator observes an insert or delete between next() calls. The GIL serializes bytecodes but does not make a multi-step iteration atomic: a preemptive thread switch can land mid-iteration in the resolver thread, and if the loop thread inserts or pops during that window, the resolver's generator raises. Note this hazard is specific to the sync resolvers — the async tool bodies all share the event-loop thread and are cooperatively scheduled, so they never observe a mutation mid-iteration (they don't await inside these comprehensions).
Concrete trigger (step-by-step). The README explicitly advertises that multiple sessions can talk to one Streamable HTTP process simultaneously:
- Session B calls
add_taskswith a batch of items. The handler runs on the event loop, inserting one record per iteration interleaved withawait anyio.sleep(0.1)— so inserts keep landing over an extended window. - Session A calls
prioritize(orclear_done). The framework seesrank_open_tasks/confirm_clearis sync and dispatches it to a worker thread viaanyio.to_thread.run_sync. - The worker thread begins iterating
tasks_by_id.values(). A GIL preemption occurs mid-iteration; the event loop runs session B's next insert. - The worker thread resumes, its dict iterator detects the size change, and
RuntimeError('dictionary changed size during iteration')propagates out of the resolver — session A's entiretools/callfails with an internal error instead of a result.
Why nothing prevents it. tasks_by_id is a bare dict with no lock or snapshot discipline, and nothing serializes requests: the low-level server dispatches each request in its own task, and streamable HTTP serves multiple sessions per process. The other cross-session sharing quirks the example documents (the subscriptions set, the log threshold) are semantic, not crash-inducing — this is the only place shared state is iterated from a foreign thread.
Impact and severity. The window is genuinely narrow — the dict iteration takes microseconds against a ~5ms GIL switch interval, and even a hit costs only one failed request with no state corruption (the dict itself stays consistent). Nothing concretely breaks in realistic demo usage, so this isn't merge-blocking. It's still worth fixing because this is a reference example that teaches the resolver pattern, and "sync resolvers run on a worker thread; don't touch loop-thread state unguarded there" is exactly the kind of lesson a reference server should model correctly.
How to fix. Either of two one-liners per resolver:
- Snapshot before iterating:
sum(1 for task in list(tasks_by_id.values()) if ...)inconfirm_clear, andlist(tasks_by_id.values())insideopen_tasks()/projects(). - Or declare the resolvers
async def— they do no blocking work, so running them on the event loop (like the tool bodies) removes the cross-thread access entirely and is arguably the more idiomatic fix for the example.
| # The logging capability is deprecated at 2026-07-28, but this notification is still the | ||
| # only wire shape for it on both eras; the deprecation warning is expected, so silence it. | ||
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("ignore", MCPDeprecationWarning) | ||
| await ctx.log("info", text, logger_name="todos") # pyright: ignore[reportDeprecated] |
There was a problem hiding this comment.
🟡 log_info holds warnings.catch_warnings() across the awaited ctx.log(...) call; catch_warnings() saves/restores the process-global warnings.filters and is not task-safe, so two overlapping tool calls (easy over streamable HTTP, where every mutating tool logs) can interleave enter/exit non-LIFO and permanently leak the ignore filter into the global filter list. Since this is a reference example that teaches patterns, install the filter once at module import — warnings.filterwarnings("ignore", category=MCPDeprecationWarning) — instead of toggling global state per request. Nothing user-visible breaks in this standalone demo, so this is a non-blocking cleanup.
Extended reasoning...
What the bug is. log_info (todos.py:200-204) wraps the awaited ctx.log(...) in warnings.catch_warnings() + warnings.simplefilter("ignore", MCPDeprecationWarning). On every standard CPython build this repo supports (3.10–3.14; the context-aware catch_warnings in 3.14 is free-threaded-only), catch_warnings snapshots and restores the process-global warnings.filters list. The Python docs flag it as not thread-safe, and holding it across an await makes it equally task-unsafe: ctx.log → session.send_log_message → an anyio memory-stream send, which checkpoints unconditionally, so the coroutine genuinely suspends inside the with block.
The code path that triggers it. Concurrency is an advertised scenario — the README says a 2025-era and a 2026-era client can talk to the same HTTP process simultaneously, and every mutating tool calls log_info (work_through_tasks calls it repeatedly with multi-second sleeps between calls, so overlap is easy). While one task is suspended at the await inside the block, the inserted ignore filter applies to every other task in the process. Worse, a non-LIFO enter/exit interleave corrupts the filter state permanently.
Step-by-step proof (reproduced by verifiers on Python 3.11): 1) Session 1 calls work_through_tasks; its log_info enters catch_warnings, saving the original filters S0 — warnings.filters is now [ignore] + S0. 2) It suspends at await ctx.log(...). 3) Session 2's add_task runs its own log_info, entering a second catch_warnings that saves [ignore] + S0. 4) Session 1's block exits first, restoring S0. 5) Session 2's block exits, restoring [ignore] + S0. The ignore entry is now leaked into the global filter list for the life of the process — a small script simulating this enter/enter/exit/exit order confirms the leaked ('ignore', None, MCPDeprecationWarning, ...) entry persists after both exits.
Why nothing prevents it. Nothing in the SDK or the example serializes log_info calls; streamable HTTP runs concurrent sessions on one event loop, and catch_warnings has no awareness of tasks. The # pyright: ignore[reportDeprecated] comment shows the deprecation is expected and deliberately silenced — the problem is only how it is silenced.
Honest scoping of the impact (per one verifier's counter-analysis, which is largely right): in this program the observable harm is essentially nil. The MCPDeprecationWarning from the @deprecated ctx.log wrapper is emitted synchronously at call time, before the first suspension, so the intended suppression always works; the only filter that can leak is an ignore for MCPDeprecationWarning, which is exactly the steady state the fix below would install deliberately; and this is a standalone demo process, not a library embedded in an app whose -W/filterwarnings config could be clobbered. No board behavior changes under any interleaving. That is why this is a nit, not a blocker.
Why it is still worth fixing. This is the reference example — the "polls app" of MCP servers that people copy patterns from — and holding catch_warnings across an await is an objectively task-unsafe pattern that copies badly into real servers, where a leaked or window-scoped global filter can swallow warnings other code (an embedding app, a test suite with filterwarnings = error) needs to see. The fix is one line: drop the with block and install the filter once at module import — warnings.filterwarnings("ignore", category=MCPDeprecationWarning) — which matches the comment's intent ("the deprecation warning is expected") and removes all per-request global-state toggling. Alternatively, keep the block but move the await outside it (call ctx.log synchronously enough to trigger the warning inside, awaiting the coroutine outside), though the module-level filter is simpler and matches what the code already wants.
1 participant
Adds
examples/servers/todos-server/— a Python port of the TypeScript SDK's reference server,examples/todos-server: a small project todo board where every server-side MCP feature has a real job, built on the high-levelMCPServer.Motivation and Context
The TypeScript SDK ships a reference host/server pair (
cli-client+todos-server) that exercises tools, resources, prompts, sampling, elicitation, multi-roundinput_requiredflows, progress, logging, and subscriptions in one small, readable app, serving both protocol revisions (2026-07-28 and 2025-11-25) over stdio and Streamable HTTP. This PR brings the server half to the Python SDK so the two SDKs have a directly comparable reference workload — the TScli-clientconnects to this server out of the box over HTTP.Notable porting decisions (details in the example README's "Fidelity" section):
clear_done,prioritize,brainstorm_tasks) are written once as resolver dependencies (Resolve+Elicit/Sample, the latter from Extend resolver DI to sampling and roots requests #3049): the framework carries the questions asInputRequiredResultrounds on 2026-07-28 connections and as push-style requests on pre-2026 ones, so no handler branches on the era and the example does no round bookkeeping.brainstorm_tasksshows the multi-round shape as a resolver chain — count/theme form → conditional custom-amount form → sampling round derived from the recorded answers.resources/subscribe/unsubscribe,logging/setLevel, and a dynamicresources/list(one entry per task, like the TSResourceTemplatelist callback) are registered on the low-level server viamcp._lowlevel_server.add_request_handler— the same pattern (andTODO(felix)gap) as the everything-server.logging/setLevelon 2025 sessions and the per-requestio.modelcontextprotocol/logLevel_metaopt-in on 2026-07-28 sessions, matching the TS server's semantics.REQUEST_STATE_SECRETmaps toRequestStateSecurity(keys=[...])like the TS example's HMAC env key.How Has This Been Tested?
Drove this server and the TS reference server through an identical scripted scenario (~38 steps: all eight tools including the three-round brainstorm flow and its decline/cancel branches, resources, templates, prompts, completions, progress, logging thresholds, subscriptions) with the same client and scripted elicitation/sampling callbacks:
descriptioninprompts/getresults and advertisesexperimental: {}on legacy initialize).subscriptions/listenstreams (ack + per-mutation board updates on both servers). On the legacy HTTP leg the Python server's default stateful sessions serve push-style elicitation/sampling, where the TS server's stateless posture refuses (its documented caveat). The TS entry has since moved to its protected HTTP wiring (createMcpHonoApp, loopback bind + localhost Host/Origin validation); the Python entry gets the equivalent from the SDK's streamable-HTTP defaults, noted inserver.py.-32021error from both servers on 2026-07-28 connections (the TS SDK's capability gate); on pre-2026 connections the TS server reports anisErrortool result where this server still answers-32021— README fidelity section updated accordingly.work_through_tasksmid-flight stops the loop on both eras (after Make client-side cancellation work over the 2026 transports #3046); the README documents the remaining granularity difference vs TS.uv run --frozen ruff format --check,ruff check,pyright(strict, root config), and the full pre-commit suite pass.Breaking Changes
None — example only; no
src/changes.Types of changes
Checklist
Additional context
Two SDK gaps surfaced while verifying parity, documented in the example README rather than worked around:
subscriptions/listenis not served over stdio (2026-era stdio clients get METHOD_NOT_FOUND, so board-change notifications over stdio reach 2025-era subscribers only), and there is no public seam to advertiselistChangedcapabilities to pre-2026 HTTP clients (serve_stdiopatches the stdio path via the low-level server). A third gap noted in the first revision — no courtesynotifications/cancelledfrom the client on 2026-era transports — was fixed by #3046. Re-checked against currentmainafter the latest rebase: both remaining gaps still stand, and nothing that landed since (the client-sideClient.listen()driver from #3047, the httpx2 swap, Streamable HTTP body limits) changes the example's server-side surface. The README now points 2026-era watchers atclient.listen(...)and its docs page.AI Disclaimer