Repository navigation
Introduce the Tool interface - #348
domfarolino wants to merge 8 commits into
Conversation
| : {{RegisteredTool/annotations}} | ||
| :: if |tool definition|'s [=tool definition/annotations=] is not null, a | ||
| : [=Tool/annotations=] | ||
| :: null if |tool definition|'s [=tool definition/annotations=] is null. Otherwise, the |
There was a problem hiding this comment.
We have to do a special Web IDL dictionary => JavaScript object conversion here that we didn't need previously, because Web IDL supports dictionary-typed members of a dictionary, but not dictionary-typed attributes of an interface object (which [=tool/annotations=] ultimately backs).
| and a [=tool execution error=]-or-null <var ignore>error</var>. | ||
| The <dfn>tool execute steps</dfn>, given a [=unique internal value=] |toolId|, a {{Document}} | ||
| |targetDocument|, a [=string=] |inputArguments|, an algorithm |completionSteps|, and a [=unique | ||
| internal value=] |uuid|, are as follows. The |completionSteps| algorithm takes a [=string=]-or-null |
There was a problem hiding this comment.
It may be time to introduce a more specific label for uuid to be more precise, similar to the exercise underway in the code. But fine to do in a different PR as that touches a lot of different algorithms.
and a [=unique internal value=] |executionId|
| 1. Let |tool| be |toolMap|[|toolName|]. | ||
| 1. Let |tool| be |toolMap|[|toolId|]. | ||
|
|
||
| 1. Run |tool|'s [=tool definition/execute steps=] given |targetDocument|, |inputArguments|, |
There was a problem hiding this comment.
Side comment: It might be simpler to directly invoke the execute callback here instead of calling out to this concept, which has the callback invocation implied by a note and doesn't describe how the arguments are passed to it.
|
|
||
| 1. [=map/Remove=] |tool map|[|tool id|]. | ||
|
|
||
| Note: Unregistering a tool does not cancel an execution that has already invoked its |
There was a problem hiding this comment.
More precisely, the execution must have passed the tool execute steps steps and reached the invoke an execute callback steps, as we check and look up |tool id| in the former, but don't care in the latter.
Maybe something like
cancel an execution that has reached the
invoke an execute callbacksteps, even when a {{ToolExcuteCallback}} itself performs the unregistration.
|
|
||
| 1. If |targetDocument| is not [=Document/fully active=], then return. | ||
|
|
||
| Note: This can happen if a tool's {{ModelContextRegisterToolOptions/signal}} is aborted after |
There was a problem hiding this comment.
Could be more precise about which signal was intended here:
tool was unregistered via its {{ModelContextRegisterToolOptions/signal}} after |targetDocument|...
| {{ModelContext/getTools()}}. But the tools registered in |targetDocument| can change after | ||
| {{ModelContext/getTools()}} resolves: for example, the tool can be unregistered, or | ||
| unregistered and replaced by a new tool with the same name but a different [=tool | ||
| definition/tool id=]. Rejecting with "{{NotFoundError}}" across all three checks above鈥攚hen |
There was a problem hiding this comment.
I sort of feel like this should be two distinct Note's.
Note: Rejecting with "{{NotFoundError}}" across all three checks above鈥攚hen...
| 1. Let |newTool| be a [=new=] {{Tool}} object created in [=this=]'s [=relevant realm=], | ||
| whose members are set as follows: | ||
|
|
||
| : [=Tool/tool id=] |
There was a problem hiding this comment.
This isn't in the IDL for {{Tool}}. Is there some way to mark it as internal or a slot value?
Edit: It looks like there is a Note: below. Maybe hiding from IDL is sufficient?
| ToolAnnotations annotations; | ||
| readonly attribute DOMString title; | ||
| readonly attribute DOMString description; | ||
| readonly attribute object? inputSchema; |
There was a problem hiding this comment.
Do we also need
readonly attribute object? outputSchema
|
|
||
| <div algorithm> | ||
| The <dfn attribute for=Tool>origin</dfn> getter steps are to return the [=serialization of an | ||
| origin|serialization=] of [=this=]'s [=Tool/origin=]. |
There was a problem hiding this comment.
Wondering what this might return for opaque origins, a problem for another PR :)
| <dl class="domintro"> | ||
| : <code><var ignore>tool</var>["{{RegisteredTool/name}}"]</code> | ||
| : <code><var ignore>tool</var>.{{Tool/name}}</code> | ||
| :: A unique identifier for the tool. It is the same value provided at tool registration, via |
There was a problem hiding this comment.
Probably need to amend this
A name for the tool.
This branch has not been deployed
2 participants
This PR introduces the
Toolinterface which encapsulates a tool's new UUID. Tools are now keyed and identified by this ID. This replaces theRegisteredTooldictionary with the newToolinterface, fixes a number of possible race conditions (such as https://webmachinelearning.github.io/webmcp/#unregistration-execution-race), and also unblocks us from being able to execute registered by opaque origin documents (see #349).Note that there are some compatibility changes with this:
RegisteredTool#inputSchemaandRegisteredTool#annotationscould be undefined before this PR, where asTool#inputSchemaandTool#annotationsare notobject?which makes more sense for an interface object. This means these two members can be null when their respective values are not provided. They are never purely undefined now.JSON.stringify(tool)returns{}, and the spread operator copies nothing.structuredClone(tool)throws, since this type is not yet[Serializable].This makes some progress towards the discussion in #236 (/cc @jakearchibald), and helps us out with future worker integration.
/cc @nmccormack-oai from who was interested in this fix from an agent's perspective. Note that the spec still doesn't define a native-agent-initiated tool execution path (equivalent to CDP-initiated tool execution in Chromium, or Gemini-in-Chrome/"actor stack"-initiated tool execution in branded Chrome), but that's coming soon, and the changes here are required to support the correctness / race fixes in those areas too.
Preview | Diff