Repository navigation
WebMCP: test that a tool unregistering itself mid-execution still delivers its result - #62642
mysticalseeker24 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The reviewed tests cover both execution and registration-state behavior with no unresolved issues.
Pull request overview
Adds WPT coverage for executeTool() when a tool unregisters itself during execution.
Changes:
- Verifies self-unregistration still delivers the tool result.
- Adds a control test for unregistering another tool.
File summaries
| File | Description |
|---|---|
webmcp/imperative/executeTool-self-unregistration.https.html |
Adds self-unregistration and control scenarios. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Verification update: the PR description said test 1 was expected to pass on 153. It has now been confirmed, on Chromium 153 via Edge 153.0.4234.32, running the same logic outside the harness:
So it fails on the build with the conformance bug and passes on the build with CL 8224887, which is the behaviour the case is meant to pin. One correction to the PR description while I am here. It frames the Chrome 152 result as though a page-state-driven registry were inherently exposed to this. That was wrong, and @mlmrx corrected it on webmachinelearning/webmcp#308: #248 already establishes that unregistering removes only the tool-map entry and must not orphan a started execution, so 152 is a conformance bug rather than a gap the spec leaves open. The tests are unaffected — they assert the behaviour, not a cause — but the surrounding narrative in the description should be read with that correction. |
It had drifted in one direction that matters and was missing the thing that changed most. Corrected: the copy cited "#165's requestUserInteraction(), which we feature-detect and found absent in Chrome 152", which reads as a spec feature a browser had not shipped. requestUserInteraction appears zero times in index.bs. It is the shape *proposed* in #165, and it is absent from Chrome 152 and Chromium 153 alike. This is the same conflation of draft and implementation we were corrected for upstream, sitting in the submission text, so it is now stated as a proposal. Added "What went upstream", which did not exist when this was written: enforcement by absence adopted as mitigation (d) in the body of #298, our reproduction cited in the body of #300, the WPT case open at web-platform-tests/wpt#62642 that fails on 152 and passes on 153, the #299 and #96 measurements, and the two occasions we were wrong and said so. A submission whose findings moved the standard is a different claim from one built against it. Added the verification numbers (385 unit tests, 36 real-Chrome assertions, 14 accessibility checks, 7 evals), all re-measured rather than recalled, with the commands to re-measure them in the paste comment so the next edit has no excuse. Added the accessibility honesty: contrast measured not estimated, and no screen reader run, no disabled user. Also covers the features added since: every surface reflecting a call made from any other, and constraints that grey the slots they exclude while saying which window excluded them. Co-Authored-By: Claude Opus 5 <[email protected]>
I think there's no distinction. In existing tests, the parent tells the child to unregister mid-flight, but the child still unregisters the tool on its own (hence the "self" bit in the file name not carrying much of a distinction)—because that's the only way to unregister a tool. The test in this PR seems redundant with the others, since I don't think there's a distinction between whether the unregistration happens literally inside of |
|
You're right, and I measured it rather than argue. Closing this. Two things I had wrong. First, "self" was a bad name: only the registering document can abort its own controller, so where the Three shapes, same page, no harness, on the build with the conformance bug and on the one with CL 8224887:
All three reject with One small thing you can take or leave, no PR attached: Thanks for reading it properly. |
Adds
webmcp/imperative/executeTool-self-unregistration.https.html, covering a case the existing tests leave open: a tool that aborts its own registration signal from insideexecute, before producing its result.Why this shape is not already covered
executeTool-unregister-resolution-race.https.htmlaborts the tool's own signal, but afterresolve()has been called, so the abort lands post-settlement.unregister-during-executeTool.https.htmlunregisters from outside the executing tool — the parent tells a cross-origin iframe to unregister mid-flight.Neither covers the abort landing inside
execute, before the result exists. That ordering is what a page produces when its registered set is derived from page state: the tool's side effect changes state, whatever observes that state re-syncs registrations, and the tool removes itself while still on the stack. Succeeding is often precisely what makes a tool no longer applicable, so for a state-derived toolset this is the ordinary outcome of any state-advancing tool rather than an edge case.Requested in webmachinelearning/webmcp#300 ("Add a WPT case for self-unregistration specifically").
What the tests assert
executeTool()must resolve with the tool's result — removal from the tool map after the execute steps began is not a cancellation. It also asserts the tool is gone fromgetTools()afterwards, so the test cannot be satisfied by ignoring the abort.Verification
Behaviour was checked against Chrome 152 with
--enable-blink-features=WebMCP. Test 1 fails there:with the side effects applied and the tool unregistered — so the result is real, and the rejection is spurious. Test 2 passes. Chrome's documentation indicates 153 preserves in-flight executions, so test 1 is expected to pass there.
One note on how that was checked: the committed tests call
executeTool(tool)with noinputObject, matching the IDL (optional any inputObject) and the neighbouring tests. Chrome 152 currently requires two arguments and a string, which is webmachinelearning/webmcp#243 and unrelated to what is under test here, so the behaviour above was confirmed by passing the argument that build requires. The committed tests follow the spec rather than that deviation.Reproduced independently on an unrelated page-state-driven implementation; notes in webmachinelearning/webmcp#300.