Enforce MCP spec compliance by default (0.3.0) - #9
Merged
Merged
Conversation
Advertising the logging capability (use Urchin.Server, logging: true) now makes logging/setLevel succeed and apply the level to the session even when the server does not export set_log_level/2; an exported set_log_level/2 is still called as a hook. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Surface the session's initialized flag and add an opt-in :enforce_initialized transport option (default false) that rejects operation requests received before notifications/initialized with invalid_request; ping and logging/setLevel are always allowed. Default preserves current behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a :tool_errors transport option (:json_rpc default | :result). With :result,
a tools/call handler's {:error, binary} is returned as an isError CallToolResult
so the model can self-correct, instead of a JSON-RPC internal error. Protocol
errors via {:error, %Urchin.Error{}} always remain JSON-RPC errors. Default
preserves current behavior.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Duplicate tool names within a server are now always rejected at compile time
(a silently shadowed duplicate was previously accepted). Add an opt-in
validate_tool_names: true option to additionally enforce that every literal tool
name matches ~r/^[a-zA-Z0-9_.-]{1,128}$/ (default false).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Expose the per-session GET-stream replay buffer size as a :sse_buffer_limit transport option, forwarded to Session.start only when set (default nil keeps the Session default of 100). Validated as a positive integer at startup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The README bullet implied every SSE stream is resumable; only the GET (general) stream supports Last-Event-ID replay. POST request streams are not replayable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Need an answer fast? Review this PR in Change Stack to ask focused questions about the PR or a changed range. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUrchin adds Context fields for initialization and tool-error handling, enforces optional pre-initialization request gating, shapes tool failures into result objects when configured, enhances logging/setLevel to update session state and call optional hooks, validates tool names at compile time (duplicates always rejected, pattern opt-in), forwards SSE buffer limits into sessions, and adds tests and docs. ChangesRequest Lifecycle and Tool Handling Enhancements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 45-46: Changelog incorrectly states the transport option
:sse_buffer_limit has a default of 100 while the implementation sets it to nil
(so it falls back to Urchin.Session's internal default of 100); update the
CHANGELOG entry for :sse_buffer_limit to state the transport option's default is
nil (which preserves Urchin.Session’s internal default of 100) or reword to
clarify that leaving :sse_buffer_limit unset preserves Urchin.Session’s default
buffer size of 100 so there is no config-contract confusion.
In `@lib/urchin/transport/streamable_http.ex`:
- Around line 45-46: The docs incorrectly state that :sse_buffer_limit defaults
to 100; update the documentation text to reflect that the transport option
defaults to nil (so init/1 and maybe_put_buffer_limit/2 can preserve the
session’s internal default), and clarify that a positive integer overrides the
session default while nil leaves the session's own fallback (e.g., 100) intact;
reference the :sse_buffer_limit option, the init/1 function, and
maybe_put_buffer_limit/2 in the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: dc1c966a-0202-4e71-8f6b-9ec95eb8d770
📒 Files selected for processing (12)
CHANGELOG.mdREADME.mdlib/urchin/context.exlib/urchin/dispatcher.exlib/urchin/server.exlib/urchin/session.exlib/urchin/transport/streamable_http.extest/support/echo_server.extest/urchin/dispatcher_test.exstest/urchin/server_test.exstest/urchin/session_lifecycle_test.exstest/urchin/transport/streamable_http_test.exs
The transport option defaults to nil; an unset value preserves Urchin.Session's internal default buffer of 100. The previous wording (default 100) conflated the transport option default with the effective session fallback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses review of the P0 PR: - Validate the requested log level against the MCP levels; reject unknown values with invalid_params (-32602) instead of storing them on the session. - Offer logging/setLevel only when the server advertises the logging capability; otherwise return method_not_found (-32601). - Update the session level only after the optional set_log_level/2 hook succeeds, so a failed hook leaves no state change. - Allow only ping before notifications/initialized (client-side logging/setLevel is no longer pre-init exempt); fix the related comment and docs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add an integration test that initializes a session through the transport with sse_buffer_limit: 1 and confirms the replay buffer is capped, proving the option is forwarded from init/1 to Session.start. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/urchin/transport/streamable_http_test.exs (1)
381-412: ⚡ Quick winWrap session cleanup in
on_exitfor reliability.The test calls
Session.terminate(pid)on line 411, but if any assertion fails before that line (e.g., line 409), the session will not be terminated and may leak resources.Other tests in
dispatcher_test.exsconsistently useon_exit(fn -> Session.terminate(pid) end)immediately after session creation to ensure cleanup even when assertions fail (see lines 319, 348, 378 in the dispatcher test file).♻️ Proposed fix to add on_exit cleanup
assert conn.status == 200 [session_id] = get_resp_header(conn, "mcp-session-id") pid = Urchin.Session.whereis(session_id) + on_exit(fn -> Session.terminate(pid) end) Urchin.Session.notify(pid, "notifications/message", %{"n" => 1}) Urchin.Session.notify(pid, "notifications/message", %{"n" => 2}) # With the buffer capped at 1, only the most recent event is available to replay. {:ok, "g0", replay} = Urchin.Session.register_general_stream(pid, self(), {"g0", 0}) assert length(replay) == 1 - - Urchin.Session.terminate(pid) end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/urchin/transport/streamable_http_test.exs` around lines 381 - 412, After creating the session (captured as pid from Urchin.Session.whereis), ensure the session is always cleaned up by registering an on_exit callback that calls Urchin.Session.terminate(pid) immediately after pid is obtained; replace the lone Urchin.Session.terminate(pid) call at the end of the test with an on_exit(fn -> Urchin.Session.terminate(pid) end) so the session is terminated even if assertions around Urchin.Session.notify or Urchin.Session.register_general_stream fail.test/urchin/dispatcher_test.exs (1)
346-360: ⚡ Quick winCapture the initial state before asserting it's unchanged.
The assertion on line 359 only verifies that
min_log_levelis not set to"verbose", but doesn't confirm the session remains at its original value. A valid level other than"verbose"could theoretically be set, and the test would still pass.Compare this to the test at lines 374-390, which correctly captures the initial state (
before = Session.snapshot(pid).min_log_level) and then asserts== before.♻️ Proposed fix to capture initial state
test "rejects an invalid level with -32602 and leaves the session unchanged" do {:ok, _id, pid} = Session.start(server: EchoServer, protocol_version: "2025-11-25") on_exit(fn -> Session.terminate(pid) end) + before = Session.snapshot(pid).min_log_level assert {:error, error} = Dispatcher.handle_request( EchoServer, "logging/setLevel", %{"level" => "verbose"}, %Context{session: pid} ) assert error.code == -32_602 - assert Session.snapshot(pid).min_log_level != "verbose" + assert Session.snapshot(pid).min_log_level == before end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/urchin/dispatcher_test.exs` around lines 346 - 360, The test should capture the session's initial min_log_level before calling Dispatcher.handle_request and then assert it remains unchanged; modify the test by assigning before = Session.snapshot(pid).min_log_level prior to calling Dispatcher.handle_request (the call to Dispatcher.handle_request with EchoServer, "logging/setLevel", %{"level" => "verbose"}, %Context{session: pid}) and replace the final assertion assert Session.snapshot(pid).min_log_level != "verbose" with assert Session.snapshot(pid).min_log_level == before to ensure the session state is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/urchin/dispatcher_test.exs`:
- Around line 346-360: The test should capture the session's initial
min_log_level before calling Dispatcher.handle_request and then assert it
remains unchanged; modify the test by assigning before =
Session.snapshot(pid).min_log_level prior to calling Dispatcher.handle_request
(the call to Dispatcher.handle_request with EchoServer, "logging/setLevel",
%{"level" => "verbose"}, %Context{session: pid}) and replace the final assertion
assert Session.snapshot(pid).min_log_level != "verbose" with assert
Session.snapshot(pid).min_log_level == before to ensure the session state is
preserved.
In `@test/urchin/transport/streamable_http_test.exs`:
- Around line 381-412: After creating the session (captured as pid from
Urchin.Session.whereis), ensure the session is always cleaned up by registering
an on_exit callback that calls Urchin.Session.terminate(pid) immediately after
pid is obtained; replace the lone Urchin.Session.terminate(pid) call at the end
of the test with an on_exit(fn -> Urchin.Session.terminate(pid) end) so the
session is terminated even if assertions around Urchin.Session.notify or
Urchin.Session.register_general_stream fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fe9cb462-92c0-4357-a5c2-8195d5ef99ba
📒 Files selected for processing (6)
CHANGELOG.mdlib/urchin/context.exlib/urchin/dispatcher.exlib/urchin/transport/streamable_http.extest/urchin/dispatcher_test.exstest/urchin/transport/streamable_http_test.exs
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/urchin/context.ex
- lib/urchin/transport/streamable_http.ex
- lib/urchin/dispatcher.ex
…onously Make notifications/initialized a synchronous Session.mark_initialized/1 call and turn Session.set_log_level/2 into a call, so once the client has the 202 ack the next request always observes the committed state in the snapshot. Removes any reliance on mailbox-ordering reasoning for the enforce_initialized gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A hand-written capabilities/0 may return JSON-shaped string keys; logging_advertised? now checks both :logging and "logging". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Compile-time duplicate detection only inspects DSL declarations; reword the moduledoc and CHANGELOG so it does not imply hand-written list_tools/2 is covered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… nil Session.start(buffer_limit: nil) previously stored nil, crashing push_general via Enum.take(_, nil). The transport already guarded against this; this hardens the public Session API itself. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
akriaueno
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/urchin/session_lifecycle_test.exs`:
- Around line 89-95: The current test only sends one notification so it can’t
distinguish between a nil fallback and an accidental effective limit of 1;
change the test to send multiple notifications (e.g., 3) using Session.notify,
read the configured default buffer size via Session.default_buffer_limit()/0 (or
the actual module function that returns the default) and then call
Session.register_general_stream to get replay and assert that length(replay) ==
min(Session.default_buffer_limit(), 3) (and that Session.default_buffer_limit()
> 1) so the test fails if nil does not correctly fall back to the real default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e5f43076-4cd1-4561-adaa-25c70de0d5b5
📒 Files selected for processing (2)
lib/urchin/session.extest/urchin/session_lifecycle_test.exs
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/urchin/session.ex
Comment on lines
+89
to
+95
| test "buffer_limit: nil falls back to the default replay buffer" do | ||
| {:ok, _id, pid} = start(buffer_limit: nil) | ||
| Session.notify(pid, "notifications/message", %{"n" => 1}) | ||
|
|
||
| {:ok, "g0", replay} = Session.register_general_stream(pid, self(), {"g0", 0}) | ||
| assert length(replay) == 1 | ||
|
|
There was a problem hiding this comment.
Test does not actually verify default-buffer fallback semantics.
With only one notified message, length(replay) == 1 passes for both correct fallback-to-default and incorrect effective limit 1, so this test can’t detect the key regression it claims to cover.
Suggested tightening
test "buffer_limit: nil falls back to the default replay buffer" do
{:ok, _id, pid} = start(buffer_limit: nil)
- Session.notify(pid, "notifications/message", %{"n" => 1})
+ Session.notify(pid, "notifications/message", %{"n" => 1})
+ Session.notify(pid, "notifications/message", %{"n" => 2})
{:ok, "g0", replay} = Session.register_general_stream(pid, self(), {"g0", 0})
- assert length(replay) == 1
+ assert length(replay) == 2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test "buffer_limit: nil falls back to the default replay buffer" do | |
| {:ok, _id, pid} = start(buffer_limit: nil) | |
| Session.notify(pid, "notifications/message", %{"n" => 1}) | |
| {:ok, "g0", replay} = Session.register_general_stream(pid, self(), {"g0", 0}) | |
| assert length(replay) == 1 | |
| test "buffer_limit: nil falls back to the default replay buffer" do | |
| {:ok, _id, pid} = start(buffer_limit: nil) | |
| Session.notify(pid, "notifications/message", %{"n" => 1}) | |
| Session.notify(pid, "notifications/message", %{"n" => 2}) | |
| {:ok, "g0", replay} = Session.register_general_stream(pid, self(), {"g0", 0}) | |
| assert length(replay) == 2 | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/urchin/session_lifecycle_test.exs` around lines 89 - 95, The current
test only sends one notification so it can’t distinguish between a nil fallback
and an accidental effective limit of 1; change the test to send multiple
notifications (e.g., 3) using Session.notify, read the configured default buffer
size via Session.default_buffer_limit()/0 (or the actual module function that
returns the default) and then call Session.register_general_stream to get replay
and assert that length(replay) == min(Session.default_buffer_limit(), 3) (and
that Session.default_buffer_limit() > 1) so the test fails if nil does not
correctly fall back to the real default.
Per review: a tools/call handler's {:error, binary} is now returned as an isError
CallToolResult by default (the MCP-spec behavior) so models can self-correct. Set
tool_errors: :json_rpc for the legacy JSON-RPC-error behavior. Protocol errors via
{:error, %Urchin.Error{}} still surface as JSON-RPC errors. This changes the prior
default where such a handler error became a JSON-RPC error.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
^...$ matches before a trailing newline, so a tool name like "abc\n" passed validation; \A...\z requires a full-string match. Adds a regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
set_session_log_level now catches the GenServer.call exit from a dead session and returns invalid_request instead of crashing the handler with a generic error. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… wiring Integration tests through StreamableHTTP.init/1 -> Context -> Dispatcher: default tool_errors :result returns an isError result, logging/setLevel updates the session min_log_level, and client notifications are accepted (202) before initialized. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…replay Sync the tool-name pattern wording to \A...\z, note that enforce_initialized: true is required for strict MCP lifecycle compliance, and clarify that only the GET general stream is replayed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
akriaueno
changed the title
The :tool_errors default is :result, but several docs still described the
legacy :json_rpc behavior or omitted the new transport options:
- server.ex moduledoc: for tools/call an {:error, binary} (and a raised
exception) is an isError CallToolResult by default, not a JSON-RPC error;
only {:error, %Urchin.Error{}} and the other callbacks stay JSON-RPC errors.
- streamable_http.ex: fix the tool_errors_opt!/1 comment that still claimed
the default is :json_rpc.
- README: add the :enforce_initialized, :tool_errors and :sse_buffer_limit
rows to the options table, and make the manual scope-check example return an
Urchin.Error so it stays a JSON-RPC error like the declarative scopes path.
- CHANGELOG / SECURITY: separate error redaction from the tools/call response
envelope, which is governed by :tool_errors.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add transport-level coverage for tool_errors: a :json_rpc handler error surfaces as a JSON-RPC error end-to-end, and StreamableHTTP.init validates the option (:result default, :json_rpc accepted, any other value raises). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nitialized This PR made notifications/initialized commit synchronously via a GenServer.call so the next request observes initialized: true. lookup_session/2 only proves the session was alive a moment earlier, so a session that terminates in between would make the call exit and crash the Plug process. Catch the exit and return a clean 404 "Session not found", mirroring lookup_session/2 and the existing logging/setLevel guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rors Per the MCP tool error semantics, input validation is a tool-execution error, not a protocol error. With tool_errors: :result (the default) a tools/call input_schema mismatch is now an isError CallToolResult so the model can self-correct; under :json_rpc it stays a JSON-RPC invalid_params error (the legacy behavior). Malformed requests (missing/invalid name, non-object params), unknown tools, and scope denials remain JSON-RPC errors. Also fix the Urchin.Server moduledoc, which wrongly implied :tool_errors governs raised tool exceptions: a raising tool is always reported as an isError CallToolResult, regardless of :tool_errors. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…error A non-object CallToolRequestParams.arguments violates the request shape, so it must be a JSON-RPC invalid_params error regardless of :tool_errors, not an isError tool result. Input-schema validation now only runs on object arguments. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Make the previously opt-in spec-compliance behaviors the default, with no
opt-out, and validate the requests the spec requires:
- Validate tools/call arguments against each tool's input schema; a mismatch is
an isError CallToolResult. Parameterless tools default to a closed object
schema (additionalProperties: false).
- Reject operation requests received before notifications/initialized; only ping
and logging/setLevel are allowed before initialization.
- Surface a tools/call handler's {:error, binary} as an isError CallToolResult;
protocol errors stay JSON-RPC errors.
- Validate literal tool names against the spec pattern at compile time.
- Require protocolVersion, capabilities and clientInfo on initialize, and a
serverInfo carrying a string name and version.
- Validate the MCP-Protocol-Version header on DELETE, matching POST and GET.
- Cap completion/complete results at 100 values, setting hasMore when truncated.
Removes the :tool_errors, :validate_arguments and :enforce_initialized transport
options and the validate_tool_names server option; the behaviors they gated are
now always on. These options were never part of a release.
BREAKING CHANGE: spec-compliance behaviors are always on and several previously
lenient defaults now reject non-conforming input.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rewrite the unreleased CHANGELOG entries to describe the now always-on behaviors, drop the removed transport/server options from the README options table and SECURITY notes, and bump the version to 0.3.0 (a breaking change under pre-1.0 semantic versioning). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…/call
call_tool_result/4 rescued every exception into an isError CallToolResult, so a
handler raising Urchin.Error became a tool result while a returned
{:error, %Urchin.Error{}} became a JSON-RPC error — an asymmetry that also
contradicted the Urchin.Error docs. Rescue Urchin.Error first and surface it as
a JSON-RPC error; any other exception still becomes an isError result.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e convention Automatic input-schema validation lives in the DSL-generated call_tool/3, so a hand-written Urchin.Server validates its own arguments; say so in the README, SECURITY notes, transport moduledoc and CHANGELOG. Also note that the tool-name pattern is a tool-calling-SDK convention Urchin enforces, not an MCP-mandated pattern. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
akriaueno
changed the title
The MCP schema imposes no pattern on tool names, so rejecting non-conforming literal names at compile time was stricter than the spec required. Drop the pattern check entirely; only duplicate names within a server are still rejected, which prevents an unambiguous bug rather than enforcing a style. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
notifications/initialized was committed synchronously by the transport through mark_initialized/1 and also set initialized: true in the async handle_client notification path. The transport never routes the notification to the async path, so that clause was dead and risked drifting from the synchronous one. Drop it and let mark_initialized/1 be the single source. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Urchin enforces no tool-name pattern, but the MCP spec still recommends a conservative charset, a length bound and no whitespace. Say servers should follow those recommendations rather than implying names are unconstrained. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
logging/setLevel is allowed before initialization, so a direct test with initialized: false read ambiguously. Set initialized: true on the normal-path logging tests; the pre-init case is covered explicitly in the gating describe. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A tool that raises Urchin.Error is surfaced as a JSON-RPC error, like a returned
{:error, %Urchin.Error{}}; only other exceptions become an isError result. The
moduledoc read as if any raise became isError.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Only ping is exempt from the lifecycle gate. The MCP lifecycle's pings-and-logging exception covers the server's own requests/notifications, not the client's logging/setLevel, so a pre-init logging/setLevel is now rejected with invalid_request like any other operation request. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… wording Duplicate-name rejection only compares literal names (non-literal names cannot be compared at compile time), so say "duplicate literal tool names". Also correct the lifecycle wording so only ping is described as exempt before initialization. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Previously only ref and argument were checked for being maps. Validate the CompleteRequestParams shape per spec: ref as a ref/prompt (name) or ref/resource (uri) union, argument.name/value as required strings, and context.arguments values as strings, returning invalid_params when malformed. The result is also checked (values as strings, total a number, hasMore a boolean); a non-conforming result is an internal error rather than a shipped, non-spec response. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Tool.new/1 kept input_schema/output_schema as given, so a non-object schema (e.g. type: array, or a non-map) could be advertised in tools/list and skew the now-always-on argument validation. Validate that each, when present, is a JSON Schema object with root type "object", per the MCP tools spec; the DSL rejects a non-conforming schema at compile time. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This makes the server enforce the MCP
2025-11-25specification by default. Behaviors thatwere previously absent, lenient, or gated behind opt-in options are now always on, and the
server validates the requests the spec requires. The library is pre-1.0, so these breaking
changes replace the earlier opt-in options rather than adding more. The aim is to match the
spec exactly — neither more lenient nor stricter.
Targets
0.3.0.Behavior changes (breaking)
All of the following are enforced by default, with no opt-out:
Tool-call arguments are validated
A DSL tool's
tools/callarguments are validated against itsinput_schemabefore thehandler runs; a mismatch is a
CallToolResultwithisError: trueso the model canself-correct. A tool that declares no schema defaults to an object that accepts no properties
(
additionalProperties: false, the spec's recommendation for parameterless tools) — declare anexplicit
input_schemato accept arbitrary fields. A non-objectargumentsvalue remains aJSON-RPC
invalid_paramserror. Servers that implementcall_tool/3by hand validate theirown arguments.
Tool input/output schemas must be object schemas
A tool's
input_schemaandoutput_schema, when present, must be JSON Schema objects whoseroot
typeis"object"(per the MCP tools spec); the DSL rejects a non-conforming schema(e.g.
type: "array", or a non-map) at compile time, sotools/listcannot advertise anon-spec schema.
Tool errors are returned as tool results
A
tools/callhandler's{:error, "message"}(string) is surfaced as aCallToolResultwithisError: true. A protocol-level{:error, %Urchin.Error{}}— or a raisedUrchin.Error—stays a JSON-RPC error; any other raised exception becomes an
isErrorresult.The initialization lifecycle is enforced
Operation requests received before the client sends
notifications/initializedare rejectedwith
invalid_request; onlypingis allowed pre-init (the lifecycle's pings-and-loggingexception is for the server's own requests, not the client's
logging/setLevel). Thenotification is committed synchronously, so the next request observes the initialized state.
initializeandserverInfoare validatedinitializerequiresprotocolVersion(string),capabilities(object) andclientInfo(with a string
nameandversion); a missing or mistyped field isinvalid_params. Theserver's
serverInfomust carry a stringnameandversion.Duplicate literal tool names are rejected at compile time
Declaring two tools with the same literal name within a server fails compilation (previously
the last declaration silently won); non-literal names (a variable or expression) cannot be
compared statically and are not checked. No tool-name pattern is enforced — the MCP schema
imposes none, though servers should still follow its naming recommendations.
completion/completeis validated and capped at 100 valuesRequest params are validated per spec —
refas aref/prompt/ref/resourceunion,argument.name/valueas strings,context.argumentsvalues as strings — and malformedparams return
invalid_params. Results are capped at the top 100 (already ranked) withhasMoreset; a non-conforming result shape (non-stringvalues, etc.) is an internal error.MCP-Protocol-Versionis validated on DELETEMatching
POSTandGET.Other changes
logging/setLevelis a library builtin: when the server advertises theloggingcapabilityit succeeds and updates the session log level even without a
set_log_level/2callback; anexported callback still runs as a hook, the level is validated, and the session level changes
only after the hook succeeds. Servers without the capability return
method_not_found.:sse_buffer_limitsizes the per-session GET (SSE) replay buffer used for resumption(default
nilkeeps the built-in default of 100).0.3.0.Removed options
These transport/server options are removed; the behaviors they gated are now always on, and
they were never part of a release:
:tool_errors,:validate_arguments,:enforce_initialized,and
validate_tool_names(the latter's pattern check is dropped entirely rather than madedefault). Security options (
:expose_internal_errors,:auth) are unchanged.Migration from 0.2.0
notifications/initialized) before sending operationrequests.
initialize(protocolVersion,capabilities,clientInfo).tools/callstring error now arrives as anisErrorresult, not a JSON-RPC error — readCallToolResult.isError.input_schemaif they must accept extra fields, andensure any
input_schema/output_schemais an object schema (type: "object").Testing
mix test(run mode),mix format --check-formatted, andmix compile --warnings-as-errorsall pass (228 tests, 0 failures).