-
Notifications
You must be signed in to change notification settings - Fork 62
[Server] Added possibility to set custom ServerCapabilities. #95
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
[Server] Added possibility to set custom ServerCapabilities. #95
Conversation
I don't see the value in additional functional tests tbh, but would rather focus on unit tests + high level inspector-based tests. Can we decouple that discussion please from the actual feature about custom server capabilities. what is your use case of defining that explicitly? is the built-in logic wrong or not sophisticated enough? |
Sure I will move tests to a separate PR. As for custom For example:
If you prefer managing capabilities internally we can close issue/PR. EDIT: removed tests, squashed |
5c63f22
to
9b25e06
Compare
+1 on adding a custom way to control I think it makes perfect sense as users should be able to toggle off things like list change notifications if they don’t need them, or as @ineersa mentioned, disable completion or logging altogether. Also, looking ahead: once we support user-defined handlers, they’ll definitely need a way to manage their own capabilities too and enable only what’s relevant. |
Needs a rebase, but than a good start. I wonder if going forward more explicit methods like |
9b25e06
to
ac7238d
Compare
Rebased, it makes sense yeah. |
src/Capability/Registry.php
Outdated
logging: false, | ||
completions: true, | ||
); | ||
return $this->serverCapabilities; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i guess we could get rid off the two additional ifs (here and in the setter) by just going with:
return $this->serverCapabilities; | |
return $this->serverCapabilities ?? new ServerCapabilities( | |
tools: [] !== $this->tools, | |
toolsListChanged: $this->eventDispatcher instanceof EventDispatcherInterface, | |
resources: [] !== $this->resources || [] !== $this->resourceTemplates, | |
resourcesSubscribe: false, | |
resourcesListChanged: $this->eventDispatcher instanceof EventDispatcherInterface, | |
prompts: [] !== $this->prompts, | |
promptsListChanged: $this->eventDispatcher instanceof EventDispatcherInterface, | |
logging: false, | |
completions: true, | |
); |
or am I wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reason to have if here is that some tests was using get without set, so I didn't want to break it.
I removed new ServerCapabilities
from setter, and had to add guard in \Mcp\Server\Builder::build
.
It doesn't feel right either way.
Maybe we could create new ServerCapabilities
inside \Mcp\Server\Builder::registerCapabilities
which would feel better?
If you want to keep it in Registry
I modified code accordingly.
Also needs to be a signed commit, please |
ac7238d
to
e4a665c
Compare
fe6a072
to
e1c2bf0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @ineersa!
Added
setServerCapabilities
method to builder to allow setting of custom ServerCapabilities.Implemented application tests as proof of concept.
Motivation and Context
Resolves #66
It allows setting of custom server capabilities, for example if we will be implementing logging capability.
How Has This Been Tested?
Added unit test, implemented application tests
Breaking Changes
Non breaking
Types of changes
Checklist
Additional context
I couldn't test
initialize
response via Inspector tests, so as discussed in slack tried to implement Application tests as proof of concept.CONS of Inspector tests:
Supported methods include: tools/list, tools/call, resources/list, resources/read, resources/templates/list, prompts/list, prompts/get, logging/setLevel
, Inspector doesn't provide access to initialize response, so we can't test server capabilities for example.CONS of application tests:
Failed to connect to MCP server: MCP error -32601: No handler found for method "logging/setLevel".
If we will add some client logic to
ApplicationTestCase
like initialization, logger setting etc. it will work same as Inspector but without overheads of npm. All test in\Mcp\Tests\Application\ManualStdioExampleTest
runs in 200ms for example, and are quite easy to implement.