Add beginnings of declarative WebMCP (1/N) [chromium/src : main]
Khushal Sagar (Gerrit)
Khushal Sagar added 8 comments![]( "Open in Gerrit")
if (is_valid_mcp_form == !active_webmcp_name_.IsNull() && !name_changed) {nit: Put this in a `IsValidMcpForm() const` function?
if (name != active_webmcp_name_ && !active_webmcp_name_.IsNull()) {nit: The JS API throws an error if you try to register a tool with an existing name, even if all that's happening is a change in parameters. Maybe we should do the same here for consistency? It makes sure we teardown everything associated with the existing tool like ongoing executions.
If you go with that then you can put the unregister stuff before this if block. If there's a state change, always unregister the existing tool (if any). And then register the new one, if there is one.
using DeclarativeWebMCPCallback = base::RepeatingCallback<void(String)>;
using DeclarativeWebMCPUpdateInputSchemaCallback =
base::RepeatingCallback<String()>;Could we use an interface instead?
```
class DeclarativeWebMCPTool {
// Executes the associated tool and invokes `done_callback` with the result when the execution is finished. The callback is invoked with a null string if the execution failed.
virtual void ExecuteTool(base::OnceCallback<String> done_callback) = 0;
// Returns the input json-schema associated with the tool.
virtual String ComputeInputSchema() = 0;
}
```
// Always update the input schema, since the DOM might have changed.Hmmm, since we have to cache the input schema for script based tools here anyway, seems better to do the same for declarative tools for consistency? we can add a InvalidateBlinkToolSchema API for form to know when it's dirty. And use the ComputeInputSchema API to lazily generate and cache it again.
ExecuteBlinkTool(std::move(it->value->blink_tool_function),nitty nit: Lets use declarative instead of Blink everywhere? :)
input_arguments);The `tool_execited_cb` needs to be carried forward for declarative ones too.
if (tool_function.is_null()) {
return;
}This should be a CHECK?
script_tool->input_schema = "{}"; // For nowempty value is allowed btw but i guess it can never be empty for declarative tools? in that case input_schema being unset implies its dirty and needs to be computed?
Related details
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 9 comments![]( "Open in Gerrit")
Votes added by Mason Freed
| Commit-Queue | +1 |
9 comments
if (is_valid_mcp_form == !active_webmcp_name_.IsNull() && !name_changed) {nit: Put this in a `IsValidMcpForm() const` function?
Done
if (name != active_webmcp_name_ && !active_webmcp_name_.IsNull()) {nit: The JS API throws an error if you try to register a tool with an existing name, even if all that's happening is a change in parameters. Maybe we should do the same here for consistency? It makes sure we teardown everything associated with the existing tool like ongoing executions.
If you go with that then you can put the unregister stuff before this if block. If there's a state change, always unregister the existing tool (if any). And then register the new one, if there is one.
I'm not sure you really want to do that though. Many forms (e.g. React) are built incrementally, or at least imperatively. I'd be worried that a standard vdom operation might mutate the attributes while building the form. The end state is right, but the intermediate state might be different.
Since these tools are only needed to be correct when called, is there an issue? I can see the point of throwing an exception for imperatively-created tools, since that's likely a developer error. So helping the developer by telling them they've overwritten one is helpful. I'm not sure the same is true here.
Also, throwing exceptions for all of that means moving most of these checks way up front before the operation actually goes through. I.e. something like [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/container_node.cc;l=239;drc=aeb46299fefab41457b0c50e5386616d2e8d64a2) for DOM insertions.
using DeclarativeWebMCPCallback = base::RepeatingCallback<void(String)>;
using DeclarativeWebMCPUpdateInputSchemaCallback =
base::RepeatingCallback<String()>;Could we use an interface instead?
```
class DeclarativeWebMCPTool {
// Executes the associated tool and invokes `done_callback` with the result when the execution is finished. The callback is invoked with a null string if the execution failed.
virtual void ExecuteTool(base::OnceCallback<String> done_callback) = 0;
// Returns the input json-schema associated with the tool.
virtual String ComputeInputSchema() = 0;
}
```
Great idea, done!
bool input_schema_dirty;Mason Freednit: Blink Style Guide: Precede boolean values with words like βisβ and βdidβ. Consider renaming 'input_schema_dirty' to 'is_input_schema_dirty'.
To keep this interaction as brief and non-intrusive as possible, please consider responding with one of following options:
**Done** | **OK But Won't Fix**: reason | **Later**: b/<bug_id> | **Invalid:** reason
_This comment was generated by [Experimental Blink C++ Code Review Agent](http://go/blink-c++-code-review-agent)._
_AI reviews can sometimes be inaccurate; We appreciate your π feedback π to help us improve._
_[File a bug](http://go/blink-c++-code-review-agent-feedback) | [Provide feedback on chat](https://chat.google.com/room/AAQA0zhQHe0?cls=4) | [Opt-out](https://ganpati2.corp.google.com/group/peep-genai-blink-agent-optout.prod)_
Acknowledged
// Always update the input schema, since the DOM might have changed.Hmmm, since we have to cache the input schema for script based tools here anyway, seems better to do the same for declarative tools for consistency? we can add a InvalidateBlinkToolSchema API for form to know when it's dirty. And use the ComputeInputSchema API to lazily generate and cache it again.
Yeah, I started with this approach, but then realized that declarative tools are different in this regard. The form can (and very much does) change between the declaratively-created tool creation and this point in the code. I.e. I call `RegisterDeclarativeTool` as soon as the `<form>` is added to the document, but at that point, it has no children. So later it'll get children that will be part of the input schema. So I decided it would be best to always re-generate the input schema here, right before the model gets to see the schema and decides on a prompt object.
(See the new code, where this regen only happens for declarative tools.)
Does that make sense?
ExecuteBlinkTool(std::move(it->value->blink_tool_function),nitty nit: Lets use declarative instead of Blink everywhere? :)
Done. Blink was a placeholder I meant to change, thanks for the reminder.
The `tool_execited_cb` needs to be carried forward for declarative ones too.
Done
This should be a CHECK?
Removed in the new version.
script_tool->input_schema = "{}"; // For nowempty value is allowed btw but i guess it can never be empty for declarative tools? in that case input_schema being unset implies its dirty and needs to be computed?
Right, this will only be `"{}"` until the script tools are queried, and then it'll get filled in. If there's a good place to put a CHECK that it isn't empty, I'm happy to add that.
Related details
- Khushal Sagar
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Khushal Sagar (Gerrit)
Khushal Sagar voted and added 4 comments![]( "Open in Gerrit")
Votes added by Khushal Sagar
| Code-Review | +1 |
4 comments
if (name != active_webmcp_name_ && !active_webmcp_name_.IsNull()) {Mason Freednit: The JS API throws an error if you try to register a tool with an existing name, even if all that's happening is a change in parameters. Maybe we should do the same here for consistency? It makes sure we teardown everything associated with the existing tool like ongoing executions.
If you go with that then you can put the unregister stuff before this if block. If there's a state change, always unregister the existing tool (if any). And then register the new one, if there is one.
I'm not sure you really want to do that though. Many forms (e.g. React) are built incrementally, or at least imperatively. I'd be worried that a standard vdom operation might mutate the attributes while building the form. The end state is right, but the intermediate state might be different.
Since these tools are only needed to be correct when called, is there an issue? I can see the point of throwing an exception for imperatively-created tools, since that's likely a developer error. So helping the developer by telling them they've overwritten one is helpful. I'm not sure the same is true here.
Also, throwing exceptions for all of that means moving most of these checks way up front before the operation actually goes through. I.e. something like [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/dom/container_node.cc;l=239;drc=aeb46299fefab41457b0c50e5386616d2e8d64a2) for DOM insertions.
Sorry that statement was ambiguous. I didn't mean throw an exception but to call unregisterTool first, to tear down all existing state for that tool in ModelContext, if any state about the tool changes. Instead of looking up the entry from the map and updating it.
We can ignore for now. It only matters for edge cases where a tool execution is in-flight when the state changes. We can also change the behaviour in `RegisterDeclarativeTool` itself.
// Always update the input schema, since the DOM might have changed.Mason FreedHmmm, since we have to cache the input schema for script based tools here anyway, seems better to do the same for declarative tools for consistency? we can add a InvalidateBlinkToolSchema API for form to know when it's dirty. And use the ComputeInputSchema API to lazily generate and cache it again.
Yeah, I started with this approach, but then realized that declarative tools are different in this regard. The form can (and very much does) change between the declaratively-created tool creation and this point in the code. I.e. I call `RegisterDeclarativeTool` as soon as the `<form>` is added to the document, but at that point, it has no children. So later it'll get children that will be part of the input schema. So I decided it would be best to always re-generate the input schema here, right before the model gets to see the schema and decides on a prompt object.
(See the new code, where this regen only happens for declarative tools.)
Does that make sense?
Sounds good. We can consider caching the schema if it's expensive going forward.
std::move(tool_executed_cb).Run("Hello from declarative WebMCP");This should return `result` or `kToolInvocationFailed` in case of failure. LGTM if you want to defer that to a subsequent CL which wires up more things.
script_tool->input_schema = "{}"; // For nowMason Freedempty value is allowed btw but i guess it can never be empty for declarative tools? in that case input_schema being unset implies its dirty and needs to be computed?
Right, this will only be `"{}"` until the script tools are queried, and then it'll get filled in. If there's a good place to put a CHECK that it isn't empty, I'm happy to add that.
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |