-
Notifications
You must be signed in to change notification settings - Fork 381
VUFIND-1809: Add Model Context Protocol (MCP) server support #4939
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
base: dev
Are you sure you want to change the base?
Conversation
|
This POC doesn't do anything useful yet (check out the amazing addition function), but it is a working MCP server embedded in VuFind. Next step will be to build some real capabilities, i.e. searching and document retrieval. The implementation adapts the SDK's example of Symfony integration as well as the work @demiankatz is doing on #4672. There would be some major dependency roadblocks to actually integrating this into VuFind, as I mentioned on the Jira. This POC uses the official PHP SDK, |
|
Note for testing, I've locally modified the vendor file (yes I know) laminas-servicemanager/src/ServiceManager to make the |
| 'vufind_api' => [ | ||
| 'register_controllers' => [ | ||
| \VuFindApi\Controller\AdminApiController::class, | ||
| // \VuFindApi\Controller\McpController::class, |
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'm not sure whether this actually belongs in VuFindApi, and if so whether it should be discoverable from the main API endpoint, given the protocol is quite different. To consider.
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.
Perhaps @EreMaijala will have an opinion on this.
|
You can test the server in a couple of ways:
either way, hitting a URL like http://localhost:[port]/vufind/api/v1/mcp |
It now does a basic keyword search, and record retrieval by ID. |
To get around this, I've temporarily forked |
| recordPageFullUrl: | ||
| vufind.method: "Formatter::getRecordPageFullUrl" | ||
| description: Link to the record page from external sites with a fully qualified URL | ||
| type: string |
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.
Will extract this to a separate PR
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.
| * | ||
| * @return array The record | ||
| */ | ||
| #[McpResourceTemplate( |
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'm conflicted about keeping these attributes. In a simple MCP server they are a great, quick way to define its capabilities. But they don't support variable data, or i18n, or dynamic enabling, so in practice they need to be paired with the yaml configuration. As a counter-example I have tools currently defined entirely via yaml, but that leads to bloated configuration (the json schema is pretty verbose) that I don't love either.
I am leaning to just consistently use the yaml config for every capability, verbose but consistent. but open to other thoughts.
| } | ||
|
|
||
| // $content = json_decode($this->params()->getController()->getRequest()->getContent(), true); | ||
| // $mcpMethod = $content['method'] ?? ''; |
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 have to think through if this is useful. Methods in MCP are not specific enough, i.e. all tools are method "tools/call" and you need a further param "name" to know what's actually being called. But if we go down the route of allowing selective methods/names, then I also have to consider the fact that "tools/list" would list them all. Etc. for resources, prompts, etc. It's probably a good start just to indivisibly allow or disallow MCP. Of course the permissions.ini still lets you narrow down that single permission by IP address, etc.
| protected function outputAuthError(string $messageId): Response | ||
| { | ||
| $error = new Error($messageId, $this->AUTH_ERROR, 'Access denied'); | ||
| $response = $this->getResponse(); |
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.
Much of this is borrowed from ApiTrait. Not sure if it's worth refactoring that method for the few bits that are useful here.
Forking is no longer necessary, as the upstream |
demiankatz
left a comment
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.
Thanks, @maccabeelevine, I took a first look through this. I'm not yet familiar with the libraries and technologies being used, so I mainly focused on general design and documentation for this initial pass.
| General: | ||
| # Enable the MCP Server and register all capabilities defined below. Disabled by default. | ||
| # Access must also be granted in permissions.ini. | ||
| # enabled: true |
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.
Would it be better to have either enabled: false or #enabled: true here? Usually we only put a comment-space before a descriptive comment, and a comment-no-space before a disabled setting... but with YAML being so finicky about whitespace, anything we can do to reduce the chances of the user messing up indentation will be helpful!
(And whatever pattern we decide on here we should probably try to apply to other commented-out stuff below).
|
|
||
| General: | ||
| # Enable the MCP Server and register all capabilities defined below. Disabled by default. | ||
| # Access must also be granted in permissions.ini. |
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.
Worth mentioning the specific access.mcp permission here while we're at it? Or is this intentionally vague for flexibility?
| 'vufind_api' => [ | ||
| 'register_controllers' => [ | ||
| \VuFindApi\Controller\AdminApiController::class, | ||
| // \VuFindApi\Controller\McpController::class, |
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.
Perhaps @EreMaijala will have an opinion on this.
| "process-timeout": 0, | ||
| "allow-plugins": { | ||
| "composer/package-versions-deprecated": true, | ||
| "php-http/discovery": true, |
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.
Is this needed? If so, how is it used?
| } | ||
|
|
||
| $builder = Server::builder() | ||
| ->setServerInfo(name: 'VuFind Server', version: '0.0.1', description: 'The library catalog') |
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.
Should add the ® symbol here. Might also be nice to replace the description with the title value from config.ini.
| ->setServerInfo(name: 'VuFind Server', version: '0.0.1', description: 'The library catalog') | |
| ->setServerInfo(name: 'VuFind® Server', version: '0.0.1', description: 'The library catalog') |
| * | ||
| * @return string | ||
| */ | ||
| abstract protected function getSearchClassId(); |
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.
Might as well add return types when defining new functions...
| abstract protected function getSearchClassId(); | |
| abstract protected function getSearchClassId(): string; |
| * | ||
| * @return string | ||
| */ | ||
| protected function getRequestParam() |
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.
| protected function getRequestParam() | |
| protected function getRequestParam(): string |
| * | ||
| * @return string | ||
| */ | ||
| protected function getSearchClassId() |
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.
| protected function getSearchClassId() | |
| protected function getSearchClassId(): string |
| public function searchRecords(string $keywords, ?string $contentType = null): array | ||
| { | ||
| $limit = $this->limit; | ||
| $rawRequest = [$this->getRequestParam() => urldecode($keywords)]; |
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.
Do we really expect $keywords to come in url-encoded? Is that normal?
| * | ||
| * @return string | ||
| */ | ||
| protected function getRecordPageFullUrl($record) |
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.
We'll need to be sure to reconcile this with #4954 when that is done.
Per VUFIND-1809, from https://modelcontextprotocol.io/docs/getting-started/intro
This extends VuFind's existing API capabilities to provide an MCP server, i.e. to respond to MCP queries from AI models.
TODO