-
Notifications
You must be signed in to change notification settings - Fork 29
Add WebIDL and descriptions to spec draft #75
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: main
Are you sure you want to change the base?
Conversation
beaufortfrancois
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 for starting this Brandon!
index.bs
Outdated
|
|
||
| <dt><dfn method for="ModelContext">registerTool(tool)</dfn></dt> | ||
| <dd> | ||
| Registers a single tool without clearing the existing set of tools. If a tool with the same name already exists, the method throws an Error. |
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.
It will also raise an error in the following cases:
- tool name is missing
- description is missing
- input schema is invalid
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. I'll mention an error being thrown for input schema invalid. I would expect a TypeError on missing tool name or description is implied by the required keyword for these properties on ModelContextTool.
index.bs
Outdated
| boolean readOnly; | ||
| }; | ||
|
|
||
| callback ToolExecuteCallback = ToolResponse (object input, ModelContextClient client); |
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.
FYI Chromium currently defines this as Promise<any>(any... parameters);.
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.
Updated the return Promise to any. I'm keeping the parameters as-is since I think these are the current consensus, even if client (used for elicitation) isn't implemented in Chromium yet.
| [SecureContext, SameObject] readonly attribute ModelContextContainer modelContext; | ||
| [SecureContext, SameObject] readonly attribute ModelContext modelContext; | ||
| }; | ||
| </xmp> |
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.
One small thing is that you'll need to define what happens when modelContext is called and how its backing value is initialized. You'll want something like this: https://wicg.github.io/fenced-frame/#window-fence.
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.
Planning to cover that in a follow up PR along with some initial method algorithms.
beaufortfrancois
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.
LGTM
| interface ModelContextContainer { | ||
| interface ModelContext { | ||
| undefined provideContext(optional ModelContextOptions options = {}); | ||
| undefined clearContext(); |
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.
Provide a domintro for clearContext().
An initial pass at WebIDL for the current proposed API, and short descriptions of each property/method.
Preview | Diff