Skip to content

Conversation

Randgalt
Copy link
Contributor

@Randgalt Randgalt commented Aug 6, 2025

Motivation and Context

ServerCapabilities builder should not assign logging an initial value

How Has This Been Tested?

added assertThat(result.capabilities().logging()).isNull(); to testSuccessfulInitialization

Breaking Changes

ServerCapabilities builder no longer has logging enabled by default.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

@tzolov
Copy link
Contributor

tzolov commented Aug 7, 2025

Thanks @Randgalt. Could you elaborate on this? Does it break existing clients? I don't see how this could be harmful. Also, some existing (deprecated) code expects the logging to be present:

public Mono<Void> loggingNotification(LoggingMessageNotification loggingMessageNotification) {

@tzolov
Copy link
Contributor

tzolov commented Aug 7, 2025

I see one issue with logging enabled in the stateless server context: it will falsely claim logging capability but won't actually be able to send logs.

@Randgalt
Copy link
Contributor Author

Randgalt commented Aug 8, 2025

I see one issue with logging enabled in the stateless server context: it will falsely claim logging capability but won't actually be able to send logs.

There are two issues:

  • The stateless server currently has no way of sending log messages. This would require SSE (BTW, I have some ideas to support this for progress messages)
  • The spec for Logging from what I can tell requires sessions. This is because clients can send the server the log level that they will accept. Frankly, it would be better if the spec didn't have this or makes it optional, but it is there.

So, the stateless server advertises that it supports logging but it doesn't.

Copy link
Contributor

@tzolov tzolov left a comment

Choose a reason for hiding this comment

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

@Randgalt , we need to ensure backward compatibility for the non-stateless servers
by ensuring that the logging capabilities is enabled by default for those.

I've just added a ServerCapabilities#mutate() method that should help you do this.
(e.g. capabilities = capabilities.mutate().logging();)

@Randgalt
Copy link
Contributor Author

Randgalt commented Aug 8, 2025

OK - I understand. I'll close this.

@Randgalt Randgalt closed this Aug 8, 2025
@tzolov
Copy link
Contributor

tzolov commented Aug 11, 2025

I will reopen the PR and apply the missing change while merging it

@tzolov tzolov reopened this Aug 11, 2025
@tzolov tzolov added this to the 0.12.0 milestone Aug 11, 2025
tzolov added a commit that referenced this pull request Aug 11, 2025
- Change LoggingCapabilities from default-initialized to nullable in ServerCapabilities
- Add check if server logging is enabled in McpAsyncClient before setting logging level
- Ensure McpAsyncServer always enables logging capabilities when built
- Ensure McpStatelessAsyncServer has disabled logging capability by default
- Update tests to verify logging capabilities can be null

Signed-off-by: Christian Tzolov <[email protected]>
Co-authored-by: Christian Tzolov <[email protected]>
tzolov added a commit that referenced this pull request Aug 11, 2025
- Change LoggingCapabilities from default-initialized to nullable in ServerCapabilities
- Add check if server logging is enabled in McpAsyncClient before setting logging level
- Ensure McpAsyncServer always enables logging capabilities when built
- Ensure McpStatelessAsyncServer has disabled logging capability by default
- Update tests to verify logging capabilities can be null

Signed-off-by: Christian Tzolov <[email protected]>
Co-authored-by: Christian Tzolov <[email protected]>
@tzolov
Copy link
Contributor

tzolov commented Aug 11, 2025

Thanks @Randgalt
Rebased, extended and merged at 713ee1a

@tzolov tzolov closed this Aug 11, 2025
@olamy olamy mentioned this pull request Aug 18, 2025
9 tasks
tzolov added a commit to tzolov/mcp-java-sdk that referenced this pull request Aug 26, 2025
Remove LoggingCapabilities from McpStatelessServerFeatures.Async constructor
as stateless servers do not support setLogging operations. This aligns the
async implementation with the sync implementation which already has logging
disabled.

Follows up on modelcontextprotocol#463

Signed-off-by: Christian Tzolov <[email protected]>
tzolov added a commit to tzolov/mcp-java-sdk that referenced this pull request Aug 26, 2025
Remove LoggingCapabilities from McpStatelessServerFeatures.Async constructor
as stateless servers do not support setLogging operations. This aligns the
async implementation with the sync implementation which already has logging
disabled.

Follows up on modelcontextprotocol#463

Signed-off-by: Christian Tzolov <[email protected]>
tzolov added a commit that referenced this pull request Aug 26, 2025
Remove LoggingCapabilities from McpStatelessServerFeatures.Async constructor
as stateless servers do not support setLogging operations. This aligns the
async implementation with the sync implementation which already has logging
disabled.

Follows up on #463

Signed-off-by: Christian Tzolov <[email protected]>
tzolov added a commit that referenced this pull request Aug 26, 2025
Remove LoggingCapabilities from McpStatelessServerFeatures.Async constructor
as stateless servers do not support setLogging operations. This aligns the
async implementation with the sync implementation which already has logging
disabled.

Follows up on #463

Signed-off-by: Christian Tzolov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants