-
Notifications
You must be signed in to change notification settings - Fork 85
RFD Draft: Session Config Options #210
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
|
Just some thaught here: MCP's elicitation feature uses a restricted subset of JSON Schema when it comes to asking information from the user via client. Would it make sense to align to that? |
|
@phil65 it's a good point. The enum type here could work: https://modelcontextprotocol.io/specification/draft/client/elicitation#supported-schema-types It wouldn't be exactly the same as it wouldn't follow the larger request/response lifecycle of an elicitation, but we could at least align on the schemas. I'll take a look before this moves from Draft to Preview for sure |
|
There is also the interesting distinction here in that we want to enforce a default value, in fact it is required, and we want to also send form state each time, which makes it harder to represent as a schema. But we could pull out the form state to a separate struct on the side, so you get a schema + current values |
|
Yes agreed, perhaps its not a 100% fit for this mechanism, but (slightly offtopic), I think some equivalent to MCP elicitation would be a nice feature for ACP (could potentially even replace the Permission-Request mechanism, which basically is like elicitation, but limited to 4 buttons?) |
|
Yep agreed this needs some work, and definitely fits with the use case |
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.
Excited for this! 🎉
| } | ||
| } | ||
| } | ||
| ``` |
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 could simplify the Client-side implementation by only including the config options state in this notification, and not in the session/set_config_option. The rationale being that the Client is gonna have to handle the notification anyway for changes originating from the Agent, so we could have just have one source of these.
Let me know if you disagree, though.
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.
So you're suggesting, if I set an value, I just get a null back and rely on the agent sending an update if other values have changed as a result?
Proposing the ability to add Agent provided configuration options to a session. This makes the current pattern used by session modes more flexible, rather than adding additional hard-coded selectors to the protocol.
5877e70 to
c036e08
Compare
|
Thanks for the feedback everyone! I incorporated it and am going to merge in Draft state. Will get it ready for Preview soon |
Proposing the ability to add Agent provided configuration options to a session.
This makes the current pattern used by session modes more flexible, rather than adding additional hard-coded selectors to the protocol.