-
Notifications
You must be signed in to change notification settings - Fork 132
feat(elicitation): add user consent through elicitation MCP-185 #551
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
Conversation
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.
Pull Request Overview
This PR adds user confirmation functionality for certain MongoDB tools through the new elicitation API, providing an additional safety layer for potentially destructive operations. The implementation allows clients that support elicitation to prompt users for confirmation before executing specified tools, while automatically approving operations for clients that don't support this feature.
- Introduces an
Elicitation
class to handle confirmation requests via the MCP elicitation API - Adds
confirmationRequiredTools
configuration option with default tools requiring confirmation - Implements confirmation flow in the tool execution pipeline with customizable confirmation messages
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
src/elicitation.ts | New class implementing elicitation API for user confirmation requests |
src/tools/tool.ts | Enhanced base tool class with confirmation verification and customizable messages |
src/common/config.ts | Added confirmationRequiredTools configuration option with sensible defaults |
src/server.ts | Integrated elicitation into server construction and tool registration |
src/transports/base.ts | Added elicitation instance creation in transport runner |
src/tools/mongodb/delete/*.ts | Added custom confirmation messages for destructive MongoDB operations |
src/tools/atlas/create/*.ts | Added custom confirmation messages for Atlas user and access list creation |
src/tools/mongodb/connect/connect.ts | Updated constructor to use new tool parameter structure |
src/telemetry/types.ts | Added confirmation_required_tools to server event telemetry |
tests/**/*.ts | Comprehensive test coverage for elicitation functionality |
README.md | Documentation for the new confirmation feature |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
*/ | ||
public supportsElicitation(): boolean { | ||
const clientCapabilities = this.server.getClientCapabilities(); | ||
return clientCapabilities?.elicitation !== undefined; |
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.
If one wants to be paranoid... there's might be a chance some client will claim it supports elicitation
while not supporting it. Then this would likely make this tool a confusing no-op.
That said this would be client's fault and one could unblock themselves by setting confirmation required tools to nothing.
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.
Havan't looked at the tests yet, but the core implementation looks solid! Good job.
}; | ||
} | ||
|
||
protected getConfirmationMessage({ database, collection, filter }: ToolArgs<typeof this.argsShape>): 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.
q1: Do we want to get confirmation for deletions even when we have a filter specified? I thought we only want to do it if you try and delete everything in the collection.
q2: Do we want to give the approximate count of documents that will be deleted in the message?
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.
q1: Do we want to get confirmation for deletions even when we have a filter specified? I thought we only want to do it if you try and delete everything in the collection.
I don't remember the discussions around this but maybe we still should? I think with deletions it's still good to review the filter and whatnot.
q2: Do we want to give the approximate count of documents that will be deleted in the message?
This does seem useful but could this be an expensive operation for extremely large collections? Seems like a potentially problematic side effect.
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 would not expect the MCP server to be used for perf critical applications, so am more inclined to think it's a worthwhile tradeoff, but don't feel strongly either way.
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 do think running other DB functions for the sake of a confirmation message is a bit unexpected but probably not critically terrible yeah.
I'm going to leave it as is mainly because it'd require some changes to make the get confirmation async (which is fine and doomed to happen), so we can revisit later
commonProperties: this.telemetryProperties, | ||
}); | ||
|
||
const elicitation = new Elicitation({ server: mcpServer.server }); |
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.
q: should we just construct this in the Server
ctor instead?
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 don't mind either way but we don't really have any precedent for that, in a sense same would apply to the rest of the constructed instances here
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 think we're typically doing that to allow injecting mocked dependencies during tests, but wasn't sure I saw that being taken advantage of here (though I skipped the tests).
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 feel like having it outside is a good principle in general as it gets one to think about it if they are ever creating a new instance of the server in tests or otherwise.
…CP-185 Adds an option to require confirmation for certain tools using the new elicitation API. This is not supported by most client yet, only notably VSCode. Clients which support it will see a confirmation option with a summary before the action is run. If the client doesn't support elicitation, the action will simply be auto-approved. This option can be confirmed with `confirmationRequiredTools` and has a default set of `drop-database`, `drop-collection`, `delete-many`, `atlas-create-db-user`, `atlas-create-access-list` enabled. In VSCode one must first click "Respond" (which sets action to "accepted") and then choose a value. I decided to let there be an explcit choice of Yes / No in JSON schema instead of opting to just rely on "Respond" as it is not immediately clear that `Respond = Yes` and I imagine this vagueness in the API spec will lead to confusion across clients so it's best to have an explicit JSON schema value for confirmation. I also went with enum string Yes / No and not boolean since the displayed value for this is more user friendly.
f74e753
to
faa2824
Compare
Pull Request Test Coverage Report for Build 17673832519Details
💛 - Coveralls |
…ng of delete docs
Adds an option to require confirmation for certain tools using the new elicitation API. This is not supported by most client yet, only notably VSCode.
Clients which support it will see a confirmation option with a summary before the action is run. If the client doesn't support elicitation, the action will simply be auto-approved.
This option can be confirmed with
confirmationRequiredTools
and has a default set ofdrop-database
,drop-collection
,delete-many
,atlas-create-db-user
,atlas-create-access-list
enabled. These tools also have custom messages for confirmation. If a user adds other tools they'll be met with a more generic tool confirmation message.In VSCode one must first click "Respond" (which sets action to "accepted") and then choose a value. I decided to let there be an explcit choice of Yes / No in JSON schema instead of opting to just rely on "Respond" as it is not immediately clear that
Respond = Yes
and I imagine this vagueness in the API spec will lead to confusion across clients so it's best to have an explicit JSON schema value for confirmation.I also went with enum string Yes / No and not boolean since the displayed value for this is more user friendly.
In VSCode
Step 1: Click "Respond"
Step 2: Choose "Yes"
Step 3: Tool will get called
In Cursor (i.e. unsupported client)
The tool gets called without any confirmation.
