Fix malformed tool registration and lookup steps - #302
Open
emecii wants to merge 2 commits into
Open
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.
beaufortfrancois
suggested changes
Sep 11, 2026
| {{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 |
Collaborator
There was a problem hiding this comment.
|tool name| needs to be a non-empty string.
Contributor
Author
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=] |
Collaborator
There was a problem hiding this comment.
It's still required, othwerwise promise is not rejected.
Contributor
Author
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 |
Contributor
Author
There was a problem hiding this comment.
Thanks — kept this editorial correction unchanged in 63a8b3e.
emecii
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Clarify
registerTool()validation by requiring|tool name|to be a non-empty string and qualifying 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