Fix malformed tool registration and lookup steps - #302
Conversation
`registerTool()` checks the same aborted signal twice. webmachinelearning#240 added an early check before the `exposedTo` validation but left the original in place inside the signal block. Between the two, the algorithm only builds a list of origins, runs the URL parser, and creates a promise — no step yields to the event loop or invokes author code, so the signal's aborted flag cannot change and the second check is dead. Removing the later one preserves webmachinelearning#240's intent, which was to let an aborted signal win over a `SecurityError`. `getTools()` reads "If |toolOwnerIsRequested| be true if ...", which binds nothing; the next step reads that variable, so the step is a `Let`. `registerTool()` dereferences a bare {{ModelContextTool/description}} with no object to read it from. Every other member access in the algorithm is written |tool|'s {{ModelContextTool/...}}. The |tool name| half of the same step is already covered by the next step, so this drops it rather than repeating it.
| {{InvalidStateError}} {{DOMException}}. | ||
|
|
||
| 1. If |tool name| or {{ModelContextTool/description}} is an empty string, then return [=a promise | ||
| 1. If |tool|'s {{ModelContextTool/description}} is the empty string, then return [=a promise |
There was a problem hiding this comment.
|tool name| needs to be a non-empty string.
There was a problem hiding this comment.
Addressed in 63a8b3e: the validation step now explicitly requires |tool name| to be a non-empty string, while qualifying the description access with |tool|.
|
|
||
| 1. Let |signal| be |options|'s {{ModelContextRegisterToolOptions/signal}}. | ||
|
|
||
| 1. If |signal| is [=AbortSignal/aborted=], then return [=a promise rejected with=] |
There was a problem hiding this comment.
It's still required, othwerwise promise is not rejected.
There was a problem hiding this comment.
Restored in 63a8b3e so the named promise retains the specified rejection behavior.
|
|
||
| 1. If |toolOwnerIsRequested| be true if |targetOrigin| is [=same origin=] with |callerOrigin|, | ||
| or if |from origins| [=list/contains=] |targetOrigin|; otherwise, false. | ||
| 1. Let |toolOwnerIsRequested| be true if |targetOrigin| is [=same origin=] with |
There was a problem hiding this comment.
Thanks — kept this editorial correction unchanged in 63a8b3e.
| 1. If |tool name| is not a non-empty string or |tool|'s {{ModelContextTool/description}} is the | ||
| empty string, then return [=a promise rejected with=] an {{InvalidStateError}} {{DOMException}}. | ||
|
|
||
| 1. If either |tool name| is the empty string, or its [=string/length=] is greater than 128, or if |
There was a problem hiding this comment.
To match implementation in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/script_tools/model_context.cc;l=377;drc=d0e72e5549c17d6d9d5067af8e0b1e987b0bbca5, we should first check tool name (empty string, length, code point, etc.) and then tool description empty string.
If either |tool name| is the empty string, or its [=string/length=] is greater than 128, or if
|tool name| contains a [=code point=] that is not an [=ASCII alphanumeric=], U+005F (_),
U+002D (-), or U+002E (.), then return [=a promise rejected with=] an {{InvalidStateError}}
{{DOMException}}.If {{ModelContextTool/description}} is an empty string, then return [=a promise
rejected with=] an {{InvalidStateError}} {{DOMException}}.
There was a problem hiding this comment.
Updated in 8687256: name validation now precedes the description check, matching Chromium. Bikeshed and make lint pass.
beaufortfrancois
left a comment
There was a problem hiding this comment.
LGTM
@domfarolino Can you merge?
domfarolino
left a comment
There was a problem hiding this comment.
Looks good, thanks!
SHA: cc45efc Reason: push, by domfarolino Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Clarify
registerTool()validation by checking the tool name (empty string, length, and allowed code points) before the description, matching Chromium. Qualify the description member access with|tool|. The existing aborted-signal checks remain intact so the promise rejection behavior is preserved.Also replace the malformed
If ... be true ifstep withLet ... be true ifingetTools().Validation:
bikeshed --print=plain --dry-run --die-on=warning spec index.bsandmake lintpass without warnings. Branch is independent of the input/navigation fix and based on50c4b7f.Preview | Diff