Repository navigation
Introduce the Tool interface - #348
domfarolino wants to merge 11 commits into
Conversation
|
Binding a A question from the extension side. We discover tools from a content script and have to pass them on to a service worker and an extension page before the model calls one, so a tool has to cross a few contexts. Since A smaller one: as far as I can tell there is no way to change a registered tool, so a changed description is a new |
|
Thanks for the reviews! I'll hold off on landing this for a little bit to give @jakearchibald more time to take a look.
This was broken before this PR too, right @kenchris? (Since
Extensions-specific work or something of the sort will take care of this, right. I don't think it'll propagate to any spec changes here, although we may independently make |
|
Yes, same before, |
| :: |tool definition|'s [=tool definition/description=] | ||
|
|
||
| : {{RegisteredTool/inputSchema}} | ||
| : [=Tool/input schema=] |
There was a problem hiding this comment.
inputSchema and annotations objects are created inside the getTools() task with parse a JSON string to a JavaScript value and dictionary → JS conversion. Both use the current realm, which is undefined in a task and may not be the Tool's realm.
How about we store the input schema string and the annotations struct on the Tool. The getters would then create the objects on first access in [=this=]'s relevant realm (via realm.[[Intrinsics]]) and cache them.
Here's how AI thinks we could fix this issue:
Toolmembers: replaceinput schema(object) withserialized input schema(the string from the tool definition) plus aninput schema objectcache (initially null). Makeannotationsan [=annotations=] struct plus anannotations objectcache.getTools()task: just copy the tool definition's input schema string and annotations struct. The task then creates no JS objects other than theTools themselves.inputSchemagetter: return null if the string is empty. Otherwise, on first access, set the cache to! Call(realm.[[Intrinsics]].[[%JSON.parse%]], undefined, « serialized input schema »), whererealmis [=this=]'s relevant realm. Return the cache.annotationsgetter: return null if the struct is null. Otherwise, on first access, buildOrdinaryObjectCreate(realm.[[Intrinsics]].[[%Object.prototype%]])with the four boolean properties in alphabetical order (matching WebIDL dictionary conversion), freeze it, and cache it. Return the cache.
There was a problem hiding this comment.
Both use the current realm, which is undefined in a task and may not be the Tool's realm.
I don't think this is true. Does that render this comment moot?
There was a problem hiding this comment.
Does the following make sense to you or is AI nitpicking me?
The argument
1. The current realm only exists while JS code is on the execution context stack. ECMA-262 §9.4:
"The running execution context is always the top element of this stack."
"The value of the Realm component of the running execution context is also called the current Realm Record."
So if the execution context stack is empty, there is no running execution context and no current realm.
2. Nothing in HTML puts anything on the stack when a task starts. The event loop just does: "Set the event loop's currently running task to oldestTask. Perform oldestTask's steps." Queue a global task uses global only to pick the event loop and document. It doesn't enter that global's realm. Realms are only entered by prepare to run script / prepare to run a callback. These are used around script and callback calls, not around task steps.
3. Both operations in the PR's getTools() task implicitly use the current realm:
- Infra parse a JSON string to a JavaScript value is
? Call(%JSON.parse%, undefined, « string »). - WebIDL dictionary → JS conversion starts with
OrdinaryObjectCreate(%Object.prototype%). - ECMA-262 §6.1.7.4 says: "a reference such as %name% means the intrinsic object, associated with the current realm".
So inside a task, %JSON.parse% and %Object.prototype% refer to nothing. 1 + 2 + 3 means the realm is undefined.
4. HTML says this itself. Its in-parallel template example captures realm before going in parallel. Inside the queued task it does "converting encryptedURLs to a JavaScript array, in realm" and explains:
"The creation of a JavaScript array object also happens during the queued task, and is careful to specify which realm it creates the array in since that is no longer obvious from context."
It also links whatwg/webidl#135 as the known open issue.
Two caveats for the reply
- "May not be the Tool's realm" is the weaker half. The task is queued on
this's relevant global. Any sensible implementation would therefore use that realm, so it's unlikely to be wrong in practice. The real problem is that the spec doesn't define it. - Step 2 is an argument from silence. HTML never says "the stack is empty when a task starts". It just never pushes anything. The supporting evidence is clean up after running script ("If the JavaScript execution context stack is now empty, perform a microtask checkpoint"), which shows the stack empties between scripts. Implementations match this: Blink must explicitly enter a
ScriptStatebefore creating JS objects in a task.
Fix
The PR already creates each Tool "in this's relevant realm". The smallest fix is to capture realm (this's relevant realm) before going in parallel and use it explicitly in the task:
- Input schema:
! Call(realm.[[Intrinsics]].[[%JSON.parse%]], undefined, « input schema ») - Annotations: convert the dictionary "in realm", HTML-example style.
The lazy-getter approach from the original comment also works, because getters always run with a current realm.
| </dl> | ||
|
|
||
| <h4 id="registered-tool">RegisteredTool Dictionary</h4> | ||
| <h4 id="tool-interface">Tool Interface</h4> |
There was a problem hiding this comment.
There's no built-in way to compare two Tools. Each getTools() call creates new objects, so toolFromFirstCall !== toolFromSecondCall even for the same registration. Agents that cache tools across toolchange events can't tell which tools are unchanged. We could either cache one Tool per (realm, tool id), or at lease have a note saying this is intentional.
| </dl> | ||
|
|
||
| <h4 id="registered-tool">RegisteredTool Dictionary</h4> | ||
| <h4 id="tool-interface">Tool Interface</h4> |
There was a problem hiding this comment.
Tool is a very generic global name. A more specific name such as ModelContextRegisteredTool would reduce web-compat risk. WDYT?
| ToolAnnotations annotations; | ||
| readonly attribute DOMString title; | ||
| readonly attribute DOMString description; | ||
| readonly attribute object? inputSchema; |
There was a problem hiding this comment.
Does this mean we resolved #224 with this change and decided we should use null. If that's the case, shall we change to DOMString title to DOMString? title?
| Promise<DOMString> executeTool(Tool tool, optional object inputObject, optional ModelContextExecuteToolOptions options = {}); | ||
|
|
||
| attribute EventHandler ontoolchange; | ||
| attribute EventHandler ontoolactivated; |
There was a problem hiding this comment.
toolcancel and toolactivated still report only the tool name (pending tool execution/tool name). Unregistering doesn't cancel an execution that's already running, so after a same-name re-registration, the owner page can't tell whether a toolcancel refers to the old or the new tool. A note would help.
OR (better?) shall we fire ToolCancel and ToolActivated events with Tool objects instead of tool names?
| 1. Let |tool definition| be |targetToolMap|[|toolName|]. | ||
| 1. Let |tool definition| be |targetToolMap|[|toolId|]. | ||
|
|
||
| 1. [=Assert=]: |tool definition|'s [=tool definition/name=] is |tool|'s [=Tool/name=]. |
There was a problem hiding this comment.
Now that lookup is by tool id not tool name, the targetOrigin / expectedTargetOrigin check can't fail anymore from a spec perspective right?
I'd suggest replacing the "targetOrigin / expectedTargetOrigin check" with something like "Assert: targetOrigin is same origin with expectedTargetOrigin" after the id lookup, and updating the 'all three checks' note to two.
|
|
||
| 1. If [=this=]'s [=Tool/window=]'s [=Window/browsing context=] is null, then return null. | ||
|
|
||
| 1. Return [=this=]'s [=Tool/window=]. |
There was a problem hiding this comment.
Shall we add a note saying that once window's browsing context becomes null, neither this getter
nor executeTool() observes window again? This way, implementations can hold window weakly, so that a Tool does not keep a destroyed Document alive.
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