feat(api): support elicitation from tool calls#804
Conversation
Signed-off-by: Calum Murray <cmurray@redhat.com>
| Elicit(ctx context.Context, message string, requestedSchema *jsonschema.Schema) (*ElicitResult, error) | ||
| } | ||
|
|
||
| type ElicitResult struct { |
There was a problem hiding this comment.
should we document this a bit? Like others, e.g. Tool ?
pkg/mcp/elicit.go
Outdated
| } | ||
|
|
||
| result, err := session.Elicit(ctx, &mcp.ElicitParams{Message: message, RequestedSchema: requestedSchema}) | ||
| // TODO: check if the error is because the client does not support elicitation, possibly return default value |
There was a problem hiding this comment.
The TODO should be more actionable for tool authors. When session.Elicit() fails because the client doesn't support elicitation, the go-sdk returns a JSON-RPC error with code -32602 and message "client does not support elicitation". Tool authors need to know how to distinguish this from other errors so they can implement fallback behavior.
Consider either:
- Documenting the expected error type/code here so callers know what to check
- Wrapping it in a sentinel error (e.g.,
ErrElicitationNotSupported) that callers can use witherrors.Is()
This becomes important as soon as actual tools start using elicitation.
There was a problem hiding this comment.
Pull Request Review: #804
Summary
Adds foundational MCP elicitation support: an Elicitor interface in pkg/api/, a sessionElicitor implementation in pkg/mcp/elicit.go that delegates to the go-sdk ServerSession.Elicit(), and wiring into both ToolHandlerParams and PromptHandlerParams. No tools use elicitation yet — this is plumbing only.
The design is clean and follows the existing patterns in the codebase (embedding interfaces in params structs, concrete implementations in pkg/mcp/).
Review Verdict
REQUEST_CHANGES
Findings
Critical Issues (Must Fix)
-
Rebase on main required — The latest main (
66106e7c) switched the test infrastructure frommark3labs/mcp-goto the officialmodelcontextprotocol/go-sdkclient. The go-sdk client supportsElicitationHandlerinClientOptions, which enables real end-to-end elicitation integration tests. Please rebase on main. -
Missing tests — After rebasing, the go-sdk test client can be extended with a
WithElicitationHandler(...)option (following the existingWithHTTPHeaders,WithClientInfopatterns ininternal/test/mcp.go). This enables behavior-based integration tests that exercise the actual elicitation protocol flow:TestElicitationAccepted— Client accepts with content, tool receivesaction=acceptand provided dataTestElicitationDeclined— Client declines, tool receivesaction=declineTestElicitationWithUnsupportedClient— Client has no handler, tool gets an error fromElicitor.Elicit()and handles it gracefully (no crash, returns fallback)
These test the observable end-to-end behavior (server → client → response → server) with no mocks of the elicitor itself.
Suggestions (Should Consider)
See inline comments for:
ElicitResult.Actionshould document valid values or define constants- The TODO in
elicit.goshould be more actionable for tool authors
Security Concerns
None. The elicitation mechanism delegates to the MCP SDK's session handling, and the jsonschema.Schema parameter constrains client responses.
Overall Assessment
The interface design and wiring are solid. The main gap is the rebase needed to align with the current test infrastructure and enable proper integration tests.
Signed-off-by: Calum Murray <cmurray@redhat.com>
Extend the Elicitor interface to support URL-mode elicitation added in go-sdk v1.4.0. Refactor Elicit() to accept an ElicitParams struct (instead of separate message/schema arguments) with fields for both form mode (Message, RequestedSchema) and URL mode (URL, ElicitationID). Fix error detection for mode-specific unsupported elicitation errors by matching all three go-sdk error variants. Follow-up to containers#804. Signed-off-by: Marc Nuri <marc@marcnuri.com>
Extend the Elicitor interface to support URL-mode elicitation added in go-sdk v1.4.0. Refactor Elicit() to accept an ElicitParams struct (instead of separate message/schema arguments) with fields for both form mode (Message, RequestedSchema) and URL mode (URL, ElicitationID). Fix error detection for mode-specific unsupported elicitation errors by matching all three go-sdk error variants. Follow-up to containers#804. Signed-off-by: Marc Nuri <marc@marcnuri.com>
Extend the Elicitor interface to support URL-mode elicitation added in go-sdk v1.4.0. Refactor Elicit() to accept an ElicitParams struct (instead of separate message/schema arguments) with fields for both form mode (Message, RequestedSchema) and URL mode (URL, ElicitationID). Fix error detection for mode-specific unsupported elicitation errors by matching all three go-sdk error variants. Follow-up to #804. Signed-off-by: Marc Nuri <marc@marcnuri.com>
* feat(mcp): support elicitation from tool calls Signed-off-by: Calum Murray <cmurray@redhat.com> * cleanup: address review comments Signed-off-by: Calum Murray <cmurray@redhat.com> --------- Signed-off-by: Calum Murray <cmurray@redhat.com>
Extend the Elicitor interface to support URL-mode elicitation added in go-sdk v1.4.0. Refactor Elicit() to accept an ElicitParams struct (instead of separate message/schema arguments) with fields for both form mode (Message, RequestedSchema) and URL mode (URL, ElicitationID). Fix error detection for mode-specific unsupported elicitation errors by matching all three go-sdk error variants. Follow-up to containers#804. Signed-off-by: Marc Nuri <marc@marcnuri.com>
This PR adds a first pass at adding elicitation support, so that toolset authors can use elicitation flows within their tools.
Note that I have not used this in any tools yet, just working to land support first here.
Fixes #801