Skip to content

Implement SEP-973 #570

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Implement SEP-973 #570

wants to merge 7 commits into from

Conversation

IAmSurajBobade
Copy link
Contributor

mcp/protocol: Implement SEP-973 specification

Fixes #552

@IAmSurajBobade
Copy link
Contributor Author

@findleyr, please note - this changes currently do not work with MCP Inspector 0.17.0, as modelcontextprotocol/modelcontextprotocol#1531 is not implemented yet. I have created issue modelcontextprotocol/inspector#861 to track this.

findleyr
findleyr reviewed Oct 11, 2025
View reviewed changes
Copy link
Contributor

@findleyr findleyr left a comment

Choose a reason for hiding this comment

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

Thanks for this change!

Can you add a basic smoke test (perhaps just adding to TestEndToEnd) that verifies the client receives the new metadata correctly?

(I'm sure it works, but good to have at least one end-to-end test).

@findleyr
Copy link
Contributor

@findleyr, please note - this changes currently do not work with MCP Inspector 0.17.0, as modelcontextprotocol/modelcontextprotocol#1531 is not implemented yet. I have created issue modelcontextprotocol/inspector#861 to track this.

Ah, this is a really important point. We need to not return this data based on the client's protocol version!
This definitely warrants a test!

@IAmSurajBobade
Copy link
Contributor Author

Thanks for this change!

Can you add a basic smoke test (perhaps just adding to TestEndToEnd) that verifies the client receives the new metadata correctly?

(I'm sure it works, but good to have at least one end-to-end test).

Yes, was about to ask, if we are good with this changes. I will add tests accordingly.

@IAmSurajBobade
Copy link
Contributor Author

IAmSurajBobade commented Oct 11, 2025 •
edited
Loading

I have updated E2E tests. And updated streamable tests - which were using shared object.

We need to not return this data based on the client's protocol version!

As of now, Inspector integration fails, as specification draft has changed after original changes were incorporated in inspector. I am not sure, if this would still be a braking change for clients implementing '2025-06-18' or earlier protocol.

@findleyr if we have to omit this info from server, could you please suggest, how we should go about it? Considering we may have to implement similar checks for future specs as well.

findleyr
findleyr reviewed Oct 16, 2025
View reviewed changes
mcp/streamable_test.go Outdated
},
ProtocolVersion: latestProtocolVersion,
ServerInfo: &Implementation{Name: "test", Version: "v1.0.0"},
ServerInfo: testImpl,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to change this.

Can you instead update conformance tests as follows:

  • Update the default server in runServerTest to have icons and website url.
  • Verify that when the client is on an older protocol version, these fields are not sent (existing conformance tests should verify that).
  • Update the logic to accept the protocol version "draft", for testing purposes. See negotiatedVersion. In other words: if the client sends protocol version "draft", we enable draft features. I think this is both necessary and useful for testing.
  • Add a new conformance test that sends protocol version "draft", and verify that it gets icons and website url.

@IAmSurajBobade
Copy link
Contributor Author

modelcontextprotocol/inspector#861 seems to be resolved now. I have tested server with multiple versions (0.16.0, 0.15.0, 0.17.1) of inspector - excluding 0.17.0 - this changes work fine without any errors.

Verify that when the client is on an older protocol version, these fields are not sent (existing conformance tests should verify that).

@findleyr , I actually wanted your input on this. For example, how do we handle older/newer protocol specific behavior throughout the SDK. For instance, if we were to determine client protocol is older, we have to do string compare, will this be a problem, as we need to do this for each initialize and list call. Or should we change the way we are listing protocols from string to may be a costomType int, so the comparison will be straightforward. Additionally, I analyzed the code for other changes required - and it will have considerable impact. For example, currently listTools, listPrompts, listResources methods, do not have direct access to protocol version information.

For now, to avoid updating existing conformance tests - I have added conditional logic to only add icon and websiteUrl info, if test name is version-draft.txtar.

@findleyr
Copy link
Contributor

@IAmSurajBobade you are right, this may be trickier to implement than first thought--I didn't account for the fact that we'd need to suppress these fields from all server features.

Given that clients should ignore extra fields, perhaps it is OK to do nothing. Can you look into how this is done in other SDKs? Do they have special handling for the protocol version? If not, then this CL becomes much simpler.

@IAmSurajBobade
Copy link
Contributor Author

(Just realised, i did not mention this earlier. 🥲) I have checked CSharp, Python, Typescript, Rust SDKs - all of them have implemented draft spec, and there is no special handling basis client version.

Refer 2025-09-18 SDK Implementation (view)

@findleyr
Copy link
Contributor

Ack, thanks @IAmSurajBobade, that's useful. Then I think we can keep it simple. Reviewing now.

findleyr
findleyr reviewed Oct 21, 2025
View reviewed changes
mcp/shared.go
protocolVersion20250618 = "2025-06-18"
latestProtocolVersion = protocolVersion20250618

protocolVersionDraft = "draft" // draft protocol version with experimental features for testing
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we can remove this now, if we're not attempting to gate this functionality on the latest spec.

mcp/conformance_test.go
return nil, incOutput{args.X + 1}, nil
}

var iconObj = Icon{Source: "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAEAAAABACAIAAAAlC+aJAAAAAXNSR0IArs4c6QAAAERlWElmTU0AKgAAAAgAAYdpAAQAAAABAAAAGgAAAAAAA6ABAAMAAAABAAEAAKACAAQAAAABAAAAQKADAAQAAAABAAAAQAAAAABGUUKwAAAJhElEQVRoBb2adaxUOxDGcXd3d3cLgaDBLbgT3N3dAgkuwSE4BAkQNLgGAsGCu7u7894v75Ayb84u2927y/3jZs7Zr9OZdjr9Oj3h/gn935kzZ8aMGRMlSpRw//3Fjh172rRpFy9eDErP4YKixZuSTZs2lSlTxpjuOOD8jx49etWqVXfv3u2treX7UDnw7NmzJk2aSIs9yhEjRuzcufP79+8tzXXDQuLAnTt3Chcu7NFijy8rVKjw8uVLt3E2b4LvwK1bt/LkyaMMjRQpUq5cuapUqVKpUqVs2bKFDx9eAWrUqPH161cbixUmyA5cv349Z86cyrhmzZqdOHHC2Pfp06dDhw7VqlVLwSZNmqSMs3kMpgNXr15ldKVZLN/58+d7s2PcuHFyKhInTvzo0SNvYG/vg+bApUuXsmTJIq2PGjXq8uXLvXXsvO/WrZtsMmXKlD/j3b8Gx4Hz589nzJhRmkKWXL16terv27dvP378kC+fP3+eMmVK05Cc+/PnTwnwKQfBgbNnz6ZPn94YgRAjRoy1a9eqvhctWpQ3b95ChQqxOcif2rVrZ9omS5bM33QUVgdOnTqVJk0aYwFCzJgxN2zYIE1EJjYMJn78+A8ePDCAefPmmZ+iRYt27do185ONECYHyC2pUqUy3SNAEzZv3qw6njBhgsQgQy4MhnVifmXR37hxw/xkIwTuwLFjx5InT276RogbN+62bdtUr6QaiUGuWLEimdTAxo8fbwDEHitn69ate/bsgSx9/PjRwLwJATpw5MiRpEmTmo4RCIydO3eqbkaPHi0xyKVLl3769KmElS9fXmGcR2aDtAbRYJ4lXsmBOHDw4EFytuw1QYIEjJlSPXz4cIlBLlu27IsXLyTswIEDkSNHVjD1iCdt2rR58uSJbGhkvx3Yt29fwoQJZR+JEiXav3+/0egIQ4YMkRhkIufVq1cS9vDhQ/e2rVqZxxw5cpDuZHNH9s+BXbt2ESpGKUKSJEkOHz6s9A4YMEBikCtXrvz69WsJIxEVLVpUwZxHdsAIESK4fyLduU8RfjiwY8eOePHiSb0s4qNHj0qzkPv06SMxyNWqVXv79q2E3bt3jw1BwfLnzz958mQUXr58+eTJkwsWLChXrpzCFChQQKmydWDLli1x4sSR6thBjx8/Ls1iE+3Ro4fEIEPaFN2HbGOrhDHeLHePOWfJkiUkNwkeMWKE7NTKAfbOWLFiSS2pU6dmkKQiOEKXLl0kBrlu3bofPnyQsJs3byqyDdOeO3euxCiZmWdzNJrJH48fPzYY3w6sX79etkdR2rRp5U6Eru/fv3fo0MH04QgNGjRQg+om26SghQsXGmu8CaNGjZLKYSUG6cOBNWvWQMtk4wwZMpw7d860R4CikeYkBpnz5OfPnyXsypUrbrJNhEgMMkcFwgkyIlkdWwfZwnTRuHFj0+pPDqxcuRJyYpohZMqU6cKFC6YxAseUli1bSgxyixYtvnz5ImGQ7cyZM0uYR7INLyLrOzAWtNRQvXp105wEYNzz6sDSpUvpw7RByJo1K/lBKmWMOW1JDHLr1q3N4csBW5LtWbNmccY32kg4si+5wBgLM72eHSDI1AbJPqJ4InymUaNGpj9HaN++PRElO2b3SZcunYRBeNatWycxyNOnT1e5n6GRGBml2bNnN2PkwQEOgWQG2WXu3LnJHlIdq7NevXoSg9ypUydWs4S5yTbZzE22iRZ5tkQVRQ1JuQmYYsWKme4gVKYX7cDs2bPlPNImX758t2/fNg0QyOt16tQx6hyBw6E6bQVMtosXL64OxwyEjOeePXsae/7nwIwZM9Q8Eoh37941aIR3795RAlHW9+rVy6wqBxww2S5ZsqTibcwq9RjZo6zn/XaAfPznecQyIsdtfb9+/aSHyGEh25T0pDbGpXv37tJ6UpBMcb8cYOWxTCXOPY/oZaQlBnnQoEGyP2Q32Ya92pBtmI8i28Rk165dZY8MMaRG9vjLAaZJrhL3PNIG1qkW97Bhw6Qu5L179yqyzc4P6VewwYMHS7OQ3WQbkzp27KhgzIZS9TuEsA92DnOqXbu2mkenjQoe99hbku3+/fsrs9xkm4ho27atgrG7y+BxrPrtAM8Qr/v37ysXnUdoDPnbaCxVqpTKmFAuRRvdZJuA7t27t1HiCGyxiiGT41u1aqVgzZs3d1uPbf9zwKPpzstly5ZJjaq2Y0m21XJEIbOtyDZWYqvsCxl/zM6ljLR1QB6yCGsZY1hvQ7Y5niuz3GSb3R2ipmDEktrdpQ+2DsiIZKkYjW/evFFMgUcbst2wYUNZXMEmcnT9+vWV9axjFavSemRbB2RCkFyKg7k8LViS7aZNmxo25hjE8nPv7uRQtbsr6/1wYOzYsWZssNjUz1iXhifCsdShm4C2Idvs7jVr1jT6HQG+oHZ3t/V+OLB9+3bZgbyMYIpZBtBvVbFijBlp2QrZTbYJQm77FKxv374ezXW/tA0hisYpUqQw3SBzNnerM2+Ib6Lc4B0Bsq0CmloLl04KNnDgQKPHp2DrAIoYFdlTiRIlJOOVPWEWGUaCkclCKqAZFHddcejQoVKVT9kPBzBXTgI2ccJcsWIFMWC6IZNQnS5YsKCynh1ABTRXG1xnKNjIkSONKkvBDwfQSIVCnRawgNsNij+cmDjiqGO7Yx+7r7Ke1cJerqwnT1gaLWH+OUDLmTNnun1QpshHdkDZHzJVHcJPYpApsiuY5aPfDqCXWouKJWWN8wg1wltlB/uGKonCkCdOnKhg9o+BOIB2Dpmkf+rSHk2nCMmRnGKEsgOmqEqiHACnTp2qYH49hgft0Qiblyxrqu2UYznyw8mgqxSQixQpwj2AurREG0dTlsrp06eNZkKRz1bcHMkArAS/3A0Y7P7+gLPRnDlzAlZoGgYYQqa9jeCxJEr13KatT0zIHeD7AziSDAaKh4sXL/ZpmSUgtA5Qigzg+wNL0x1YCB1gt1KVDkrFq1at8ss+n+AQOkDNQkYOOcr9/YFP+3wCQuUArFPeI3E16C6J+jTOBuDhMlAOW8AyhwGqVKY5h3f3F07m17AIoXKAcJcVro0bN8JSw2Kot7ahcoBdVp6zoP5cJuCGNzsCf28TZ4FhKDJzqSMtC8U6DtUidnzmQk2RoqBn0tA6gBsev6Vzf40W2CTTKuQO0IebTfCpDkfKgI2WDf+GA/Sn+Bz3ReraStrkl/yXHMAmGLU57HOtryoUfhktwWE60MgMYyNTv+AOge/qqKb4dbD+g/J/AVf65lqU7WK5AAAAAElFTkSuQmCC",
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the actual data does not matter, can we please make this "foobar".

I don't want arbitrary, unreviewable data to be checked into the repo (I don't know what this data represents).

mcp/conformance_test.go
s := NewServer(&Implementation{Name: "testServer", Version: "v1.0.0"}, nil)
impl := &Implementation{Name: "testServer", Version: "v1.0.0"}

// TODO(IAmSurajBobade): Remove this hack once we have a client protocol specific handling.
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove your self assignment here. We'll probably clean up by adding a more general "settings" file to the test archive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@findleyr findleyr findleyr left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Implement SEP-973: Icons and metadata support

2 participants

@IAmSurajBobade @findleyr