Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/mcp/types/_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -1865,13 +1865,15 @@
class CancelledNotificationParams(NotificationParams):
request_id: RequestId | None = None
"""
The ID of the request to cancel.

This MUST correspond to the ID of a request previously issued in the same direction.
Required on the wire through 2025-06-18; optional from 2025-11-25.
Required on the wire through 2025-06-18; optional at 2025-11-25; required again from
2026-07-28, where it must name a request the client previously issued (servers send
this notification only to terminate a `subscriptions/listen` stream).
"""
reason: str | None = None
"""An optional string describing the reason for the cancellation."""

Check warning on line 1876 in src/mcp/types/_types.py

View check run for this annotation

Claude / Claude Code Review

Docstring 'same direction' sentence now contradicts the new 2026-07-28 note

The unchanged sentence "This MUST correspond to the ID of a request previously issued in the same direction" now contradicts the new 2026-07-28 clause added directly below it, which says the field must name a request the *client* previously issued (a server-sent cancellation terminating a `subscriptions/listen` stream names a request issued in the opposite direction). Consider qualifying the same-direction sentence as applying through 2025-11-25, matching the vendored `v2026_07_28` surface which
Comment on lines 1868 to 1876

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 The unchanged sentence "This MUST correspond to the ID of a request previously issued in the same direction" now contradicts the new 2026-07-28 clause added directly below it, which says the field must name a request the client previously issued (a server-sent cancellation terminating a subscriptions/listen stream names a request issued in the opposite direction). Consider qualifying the same-direction sentence as applying through 2025-11-25, matching the vendored v2026_07_28 surface which drops the same-direction wording entirely.

Extended reasoning...

The inconsistency. The PR extends the request_id docstring on CancelledNotificationParams (src/mcp/types/_types.py:1868-1876) with a 2026-07-28 version note: the field is "required again from 2026-07-28, where it must name a request the client previously issued (servers send this notification only to terminate a subscriptions/listen stream)". The sentence directly above it, untouched by the PR, still states without qualification: "This MUST correspond to the ID of a request previously issued in the same direction." Within the same docstring these two MUST-level statements now disagree for the exact case the new sentence describes.

Why they conflict. Through 2025-11-25, notifications/cancelled may only target a request the sender issued — each side cancels its own requests, so "same direction" is accurate. At 2026-07-28 the rule changes: the spec narrows the notification so that a server-sent notifications/cancelled names the client's subscriptions/listen request to terminate that stream. From the server's (sender's) perspective, that is a request issued in the opposite direction. The vendored 2026-07-28 surface confirms this: src/mcp/types/v2026_07_28/__init__.py (CancelledNotificationParams.request_id, around lines 2177-2189) drops the same-direction wording entirely and reads "This MUST correspond to the ID of a request the client previously issued", and its ClientNotification docstring describes the server-sent listen-termination case explicitly.

Concrete walk-through. On a 2026-07-28 session: (1) the client sends subscriptions/listen with id 42; (2) the server later decides to close the stream and sends notifications/cancelled with requestId: 42. Per the new sentence in this docstring (and the vendored schema), this is the only legitimate server-sent cancellation at 2026-07-28 — yet it violates the unqualified first sentence, since request 42 was issued client→server while the notification travels server→client. A reader applying the first sentence literally would conclude this spec-mandated message is malformed.

Why this matters (and why it doesn't matter much). This is the version-superset monolith whose per-field docstrings are the documented mechanism for recording version availability (see the module docstring), so internal consistency of those notes is the point of the file. That said, the impact is purely documentation-level: no validation logic encodes the same-direction rule, and the new clause does spell out the correct 2026 behavior, so a careful reader is unlikely to be misled — which is essentially the refuting reviewer's point. The refutation is fair that a reader gets the right answer for 2026 from the adjacent clause; the residual problem is that the first sentence is phrased as an unqualified MUST that the very next sentence contradicts, rather than as a general rule with a scoped exception. Since the PR is already touching this docstring, tightening it is nearly free.

Suggested fix. Qualify the first sentence, e.g.: "This MUST correspond to the ID of a request previously issued in the same direction (through 2025-11-25)." — or fold it into the version notes: "Through 2025-11-25 this MUST correspond to the ID of a request previously issued in the same direction. Required on the wire through 2025-06-18; optional at 2025-11-25; required again from 2026-07-28, where it must name a request the client previously issued ...". Either keeps the superset docstring accurate for every version it covers.



class CancelledNotification(Notification[CancelledNotificationParams, Literal["notifications/cancelled"]]):
Expand Down Expand Up @@ -1953,16 +1955,17 @@
message: str
"""The message to present to the user explaining why the interaction is needed."""

url: str
"""The URL that the user should navigate to."""

elicitation_id: str
elicitation_id: str | None = None
"""The ID of the elicitation, which must be unique within the context of the server.

The client MUST treat this ID as an opaque value.
The client MUST treat this ID as an opaque value. Required on the wire at
2025-11-25; removed at 2026-07-28.
"""

task: TaskMetadata | None = None

Check notice on line 1968 in src/mcp/types/_types.py

View check run for this annotation

Claude / Claude Code Review

Example url_elicitation_client.py reads non-existent 'elicitationId' attribute (pre-existing, related)

Pre-existing issue (not introduced by this PR, but adjacent to the field being relaxed): `examples/snippets/clients/url_elicitation_client.py:73` reads `getattr(params, "elicitationId", None)`, but pydantic only exposes the snake_case attribute `elicitation_id`, so the example always prints `Elicitation ID: None` even when the server supplied one. Now that `elicitation_id` can also legitimately be `None` at 2026-07-28, the example should read `params.elicitation_id` and handle a real `None` expl
Comment on lines 1958 to 1968

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 Pre-existing issue (not introduced by this PR, but adjacent to the field being relaxed): examples/snippets/clients/url_elicitation_client.py:73 reads getattr(params, "elicitationId", None), but pydantic only exposes the snake_case attribute elicitation_id, so the example always prints Elicitation ID: None even when the server supplied one. Now that elicitation_id can also legitimately be None at 2026-07-28, the example should read params.elicitation_id and handle a real None explicitly.

Extended reasoning...

The bug. examples/snippets/clients/url_elicitation_client.py:73 extracts the elicitation id with elicitation_id = getattr(params, "elicitationId", None). The params object delivered to the client elicitation callback is the monolith mcp.types.ElicitRequestURLParams, a pydantic v2 model whose MCPModel config sets alias_generator=to_camel and populate_by_name=True. In pydantic v2, the camelCase alias is a wire/validation name only — it is never exposed as an instance attribute. The only attribute is the Python field name elicitation_id, so the getattr lookup for "elicitationId" always misses and falls through to its None default.

Code path / proof. Walk through the URL elicitation flow with this example client:

  1. The server calls session.elicit_url("Authorize…", "https://example.com/oauth", "auth-001"), which sends elicitation/create with wire params {"mode": "url", "url": "…", "elicitationId": "auth-001", …}.
  2. The client session validates those params into ElicitRequestURLParams; the wire key elicitationId populates the model field elicitation_id (value "auth-001"). After validation the instance has the attribute params.elicitation_id == "auth-001" — and no attribute named elicitationId.
  3. Line 73: getattr(params, "elicitationId", None) → attribute does not exist → returns None.
  4. Line 98: print(f"\n Elicitation ID: {elicitation_id}") → always prints Elicitation ID: None, even though the server supplied auth-001.

Note line 72 right above it (url = getattr(params, "url", None)) works only because url has no snake/camel divergence; line 73 is simply the wrong attribute name, not intentional defensiveness.

Why nothing catches it. The example is a snippet, not exercised by the test suite for this output, and getattr with a default silently swallows the missing attribute, so there is no error — just a permanently wrong display value in a security-warning prompt that is supposed to show the user what they are consenting to.

Why it is worth mentioning on this PR. This PR relaxes ElicitRequestURLParams.elicitation_id to str | None so the monolith superset accepts 2026-07-28 bodies that omit elicitationId. After this change the correct attribute can also legitimately be None, so the example's unconditional Elicitation ID: None becomes ambiguous: a reader can no longer tell whether the id was genuinely absent (2026-07-28) or just dropped by the wrong attribute lookup (2025-11-25).

Fix. Replace line 73 with elicitation_id = params.elicitation_id if isinstance(params, types.ElicitRequestURLParams) else None (or simply getattr(params, "elicitation_id", None) to keep the existing defensive style), and ideally only print the Elicitation ID line when the value is not None. This is display-only in an example client and not introduced by this diff, so it should not block the PR.

"""If specified, the caller requests task-augmented execution (2025-11-25 only)."""


Expand Down
2 changes: 1 addition & 1 deletion tests/interaction/lowlevel/test_elicitation.py
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,7 @@ async def test_elicitation_complete_notification_carries_the_elicited_id_back_to
returns; the same ordering already holds on in-memory and SSE transports.
"""
elicitation_id = "auth-001"
elicited_ids: list[str] = []
elicited_ids: list[str | None] = []
received: list[IncomingMessage] = []

async def collect(message: IncomingMessage) -> None:
Expand Down
27 changes: 27 additions & 0 deletions tests/types/test_methods.py
Original file line number Diff line number Diff line change
Expand Up @@ -674,6 +674,33 @@ def test_embedded_input_request_entries_without_method_reject_at_the_surface_ste
methods.parse_server_result("tools/call", "2026-07-28", body)


def test_input_required_url_elicit_without_elicitation_id_parses_at_2026():
"""A 2026-07-28 `InputRequiredResult` embedding a URL-mode elicitation parses
through both the surface and monolith steps without `elicitationId`.

Spec-mandated: the field is required at 2025-11-25 only and removed at
2026-07-28; the monolith model carries it as optional so the superset can
accept both versions.
"""
body = {
"resultType": "input_required",
"inputRequests": {
"r1": {
"method": "elicitation/create",
"params": {"mode": "url", "message": "Please sign in", "url": "https://example.com/auth"},
}
},
}
parsed = methods.parse_server_result("tools/call", "2026-07-28", body)
assert isinstance(parsed, types.InputRequiredResult)
assert parsed.input_requests is not None
request = parsed.input_requests["r1"]
assert isinstance(request, types.ElicitRequest)
assert isinstance(request.params, types.ElicitRequestURLParams)
assert request.params.url == "https://example.com/auth"
assert request.params.elicitation_id is None


def test_none_params_omit_the_key_so_required_params_reject():
with pytest.raises(pydantic.ValidationError) as excinfo:
methods.parse_client_request("tools/call", "2025-11-25", None)
Expand Down
Loading