Repository navigation
fix: keep logging levels per session - #667
Conversation
86f54e2 to
7f43690
Compare
7f43690 to
20b8e86
Compare
| log_notifications = @mock_transport.notifications.select { |n| n[:method] == Methods::NOTIFICATIONS_MESSAGE } | ||
| assert_equal 1, log_notifications.size | ||
| assert_equal "broadcast log", log_notifications.first[:params]["data"] | ||
| assert_empty log_notifications |
There was a problem hiding this comment.
This test pinned the behavior the specification asks for; with the per-session filter it keeps its original assertion of one delivered notification.
There was a problem hiding this comment.
Restored the original assertion of one delivered notification and its data assertion. Broadcasts now use the recipient's session filter, so the stdio client still receives logs after setting its level.
| transport.open | ||
| ``` | ||
|
|
||
| `Server#notify_log_message` broadcasts use only the server-wide level, not a session's level. Applications can set the server-wide level explicitly: |
There was a problem hiding this comment.
With per-recipient filtering this paragraph, line 90 and the bullet on line 97 go away: broadcasts honor each session's level, and the application default remains the fallback.
There was a problem hiding this comment.
Restored docs/_server/index.md and docs/_server/logging.md to the base revision. The per-recipient filter uses each session's level and falls back to the application default, so that rewrite is no longer needed.
| if session | ||
| session.configure_logging(logging_message_notification) | ||
| else | ||
| @logging_message_notification = logging_message_notification |
There was a problem hiding this comment.
Keeping the level on the calling session is right. But Server#notify_log_message (lines 448-450) filters only on the server-wide level, so after this change a client's logging/setLevel no longer enables broadcasts at all, including on a single-session stdio server, which the PR now declares as a breaking change. That trades one deviation from the specification for another.
Filtering the broadcast per recipient session avoids both: compare the notification's level with that session's level and fall back to the server-wide default when the session set none. That is how the TypeScript and Python servers resolve the threshold, it keeps the existing stdio flow working, and the breaking-change section becomes unnecessary.
There was a problem hiding this comment.
Broadcasts now enumerate the transports' existing recipients and use each ServerSession to filter and deliver the log. A client's level stays local, the application default remains the fallback, and stdio delivery keeps working. HTTP tests cover different client levels and both existing and future unconfigured sessions.
|
Accurate problem definition, and the session-scoped half of the fix is correct. One change requested: filter broadcasts per recipient session instead of disabling them for clients that set a level (inline on lib/mcp/server.rb). With that, the docs rewrite and the breaking-change note are not needed. |
20b8e86 to
27e3536
Compare
|
Fixed per-recipient filtering, restored the original test assertions and docs, and removed the breaking-change note. I checked both protocol lifecycles and the TypeScript filters; the PR body now includes those references. |
| end | ||
| end | ||
|
|
||
| def logging_sessions |
There was a problem hiding this comment.
logging_sessions (here and in StdioTransport) is a hook Server#notify_log_message looks up by duck typing, so it has to be a public method, but it is not part of the published API. A short YARD comment would make that clear: @api private, what it returns (the sessions a server-wide log broadcast is offered to, each applying its own level or the server default; nil keeps the server-wide filter), and that a custom transport may implement it.
There was a problem hiding this comment.
Added short YARD comments to both hooks with @api private, their return types, the custom-transport contract and the nil fallback.
27e3536 to
bdf060f
Compare
Motivation and Context
Keep
logging/setLevelon the calling session. Filter server-wide log messages per recipient throughServerSession#notify_log_message, using the transports' existing session state. A session's own level takes precedence over the application default. Preserve session-less logging and the default path for custom transports that do not expose sessions.Protocol and TypeScript references checked before the change:
logging/setLevel. The legacy schema says the server should send logs "at this level and higher" to that client, and "the server MAY decide which messages to send automatically" before that client sets a level. Keep the existing application default as that fallback._loggingLevels.set(transportSessionId, parseResult.data). Its sendLoggingMessage callsisMessageIgnored(params.level, sessionId). Its legacy request context uses_loggingLevels.get(ctx.sessionId) ?? this._loggingLevels.get(undefined).io.modelcontextprotocol/logLeveloptional but says the server MUST NOT emit request logs without it. With it, the server MAY send logs on that request's response stream, not another stream orsubscriptions/listen. Keep modern logging request-scoped; do not include modern stdio sessions in broadcasts.Restore the original stdio broadcast assertion and documentation. No second session registry or new validation rules.
Fixes #658.
How Has This Been Tested?
The restored broadcast assertion and four new HTTP/stdio tests fail on the previous published head: 34 runs, 178 assertions, 5 failures and no errors. The failures cover stdio delivery, per-recipient HTTP thresholds, isolated defaults and modern request scope.
--force-exclusion: all six changed Ruby files clean.git diff --checkpasses.Tests use the installed Ruby 4.0.7 gems with
MT_NO_PLUGINS=1, Minitest~> 5.1and ActiveSupport7.2.2.2.Breaking Changes
None. Client-configured logging continues to enable stdio broadcasts. Application defaults and session-less logging remain supported.
Types of changes
Checklist