Repository navigation
Conversation
chemicL
added
enhancement
Completion in the modern server was a single function registered through CompletionsFeature. It received every completion/complete request, so it had to know about every prompt and resource template and keep its own copy of which arguments each of them takes. A prompt now carries its completion handler, set with McpSyncPrompt.withCompletionHandler, and a resource template does the same through the new McpSyncResourceTemplate, which a resource repository returns from findTemplate(ctx, uriTemplate). A repository opts in with supportsCompletion(), as it already does for subscriptions with supportsSubscribe(). The server then serves completion/complete and advertises completions on its own; there is no feature to register. The server rejects a reference to an unknown prompt or template, or an argument it does not declare, with invalid params, and answers with no values when a prompt or template has no completion handler. Custom features can complete their own reference types through McpFeature.completionRefTypes() and McpFeature.complete(). CompletionsFeature and McpSync/AsyncCompletionRepository are removed; the completion function itself is now McpSync/AsyncCompletionHandler. This brings completions in line with the rest of the modern feature model, where each feature is responsible for its own primitives. Fixes #1312 Signed-off-by: Dariusz Jedrzejczyk <2554306+chemicL@users.noreply.github.com>
17913c2 to
8177c3d
Compare
| if (map.get("argument") == null) { | ||
| return Mono.error(McpException.invalidParams("params.argument is required")); | ||
| } | ||
| McpFeature feature = this.byRefType.get(ref.get("type")); |
There was a problem hiding this comment.
A ref with no type is likely to get an internal error here instead of invalid params?
byRefType comes from Map.copyOf, and get(null) on an immutable map throws an NPE and the {"ref": {"name": "x"}} will never reaches the "Unsupported ref type".
Perhaps something like this would do:
| McpFeature feature = this.byRefType.get(ref.get("type")); | |
| McpFeature feature = ref.get("type") instanceof String type ? this.byRefType.get(type) : null; |
There was a problem hiding this comment.
Makes sense, thanks for catching it.
| * the server serves {@code completion/complete} and advertises {@code completions} | ||
| * when any feature answers one. Default: none. | ||
| */ | ||
| default Set<String> completionRefTypes() { |
There was a problem hiding this comment.
The #1312 promises that "Custom features can complete their own references" (as it the docs/server-modern.md). But it seems this extension point won't work for a new reference type. The CompletionsFeature decodes every request into CompleteRequest, and CompleteReference only lists ref/prompt and ref/resource in @JsonSubTypes.
Though, I'm not sure if the MCP spec requires support of custom features?
There was a problem hiding this comment.
It might be true. What do you propose?
There was a problem hiding this comment.
Not sure. Perhaps we can support unknown reference types by a fallback CompleteReference, so complete() can receive them. But this means changes it the McpSchema.
Or let each feature decode its own params: complete(ctx, Object params), matching handle(). Custom features could then use their own types without changing McpSchema, and the built-in features would go back to Params.decode.
| public Mono<CompleteResult> complete(McpRequestContext ctx, CompleteRequest request) { | ||
| String uriTemplate = ((ResourceReference) request.ref()).uri(); | ||
| String argument = request.argument().name(); | ||
| return this.repository.findTemplate(ctx, uriTemplate) |
There was a problem hiding this comment.
Is rejecting a concrete resource URI intended here? ref/resource is only resolved through an findTemplate match, but a reference to an existing resource like test://static-text gets -32602
According to the spec the field is "The URI or URI template of the resource", so to me it looks like a valid reference?
Perhaps for empty findTemplate, we can try repository.find(ctx, uri), return no values if the resource exists, and keep the invalid-params error otherwise?
There was a problem hiding this comment.
I'm not sure I understand. If the uri is not a template, what is there to complete?
There was a problem hiding this comment.
TypeScript SDK and our legacy server return an empty result here. But I guess -32602 if well documented can be fine
Completion in the modern server was a single function registered through
CompletionsFeature. It received every completion/complete request, so it had to know about every prompt and resource template and keep its own copy of which arguments each of them takes.A prompt now carries its completion handler, set with
McpSyncPrompt.withCompletionHandler, and a resource template does the same through the newMcpSyncResourceTemplate, which a resource repository returns fromfindTemplate(ctx, uriTemplate). A repository opts in withsupportsCompletion(), as it already does for subscriptions withsupportsSubscribe(). The server then serves completion/complete and advertises completions on its own; there is no feature to register.The server rejects a reference to an unknown prompt or template, or an argument it does not declare, with invalid params, and answers with no values when a prompt or template has no completion handler. Custom features can complete their own reference types through
McpFeature.completionRefTypes()andMcpFeature.complete().CompletionsFeature and
McpSync/AsyncCompletionRepositoryare removed; the completion function itself is nowMcpSync/AsyncCompletionHandler.This brings completions in line with the rest of the modern feature model, where each feature is responsible for its own primitives.
Fixes #1312