Skip to content

implements the notifications/initialized method #84

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jkraemer
Copy link
Contributor

@jkraemer jkraemer commented Jul 21, 2025

According to https://modelcontextprotocol.io/specification/2025-06-18/basic/lifecycle, "After successful initialization, the client MUST send an initialized notification to indicate it is ready to begin normal operations".

This implementation just replies with an empty response (same as ping).

Motivation and Context

Claude Code assumed the MCP server wasn't working when this method was not handled gracefully.

How Has This Been Tested?

test case added, works in my app.

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

- According to
  https://modelcontextprotocol.io/specification/2025-06-18/basic/lifecycle,
  "After successful initialization, the client MUST send an initialized
  notification to indicate it is ready to begin normal operations".
- This implementation just replies with an empty response (same as ping).
koic
koic previously approved these changes Jul 22, 2025
Copy link
Member

@koic koic left a comment

Choose a reason for hiding this comment

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

Yeah, the current behavior that results in the following error is problematic.

{jsonrpc: "2.0", id: "123", error: {code: -32601, message: "Method not found", data: "notifications/initialized"}}

It must be able to respond to notifications/initialized in accordance with the specification.

@@ -66,6 +66,7 @@ def initialize(
Methods::PROMPTS_GET => method(:get_prompt),
Methods::INITIALIZE => method(:init),
Methods::PING => ->(_) { {} },
Methods::NOTIFICATIONS_INITIALIZED => ->(_) { {} },
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually shouldn't this return nil (i.e. return nothing)? Notifications are special methods that don't have a response.

Copy link
Contributor

Choose a reason for hiding this comment

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

request = {
jsonrpc: "2.0",
method: "notifications/initialized",
id: "123",
Copy link
Contributor

Choose a reason for hiding this comment

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

An id should not be present here, since this is a notification.

Copy link
Contributor

@atesgoral atesgoral left a comment

Choose a reason for hiding this comment

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

This should be implemented as an actual notification, not a method. See https://www.jsonrpc.org/specification#notification

@jkraemer
Copy link
Contributor Author

I have changed it to return nil and fixed the test case to not send an id. I did not find any code dedicated to handling incoming notifications outside the regular method call handling - do we leave it at that for now or would it make sense to have a separate handler for anything notifications/*?

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.

3 participants