Repository navigation
fix: preserve WebMCP cancellation and current execution contracts - #23
Merged
Merged
Conversation
TueJon
commented
Oct 5, 2026
TueJon
left a comment
Owner
Author
There was a problem hiding this comment.
Review — #23 @ 471dfca
First review (no prior reviews). Verdict: ship_it — no blocking or non-blocking findings.
What was reviewed
Full diff (+702/−53, 12 files) against main, plus repo rules (AGENTS.md), exact-head CI (check pass), and local runs at the PR head.
Coverage sweep (round 1)
- Product fit — real bug:
guardToolResultdropped the secondexecute()argument, so cooperative tools never received the caller cancellation signal. Preserving options identity, adding caller cancellation to the event bridge, and separating registration disposal from active execution belongs in this product. The README callout fix (v0.6.0 → v0.7.0) is correct: CHANGELOG shows 0.7.0 released 2026-09-19 with the mutation journal. - Correctness — traced every changed entry point: options forwarding through
guardToolResult(TS + JS),dispatchAndWaitcancel/settle/cleanup ordering (preabort-before-dispatch,once:truelistener removal, timer cleared on every settlement, completion listener removed beforeabort()so synchronous handlers can't double-settle), and workbench simulation deferred execution with preabort skip. No logic gaps found. - TS/JS sync (repo hard rule) — both variants changed symmetrically;
parity.test.mjsloops both variants over identity/cancellation/preabort cases. - Other
executeToolconsumers —webmcp.spec.ts,webmcp-compat.js,compat.test.mjs,verify.mdetc. referenceexecuteToolbut are untouched; the return-type change (Promise<string | null>) is covered by the full suite. - Security / external effects / deploy — no secrets, no network/provisioning code, no workflow changes. Client-side templates, docs, tests only.
- Docs — new
native-compatibility.mdcorrectly scopes evidence levels (CG draft + Chrome guides as documentation, Chrome 150 vs Beta 156 as separate observations, simulation explicitly not native proof). No SKILL.md/package.json churn, no version mismatch (changes under[Unreleased]).
Verification observed
- Exact-head CI
check: pass. Local at471dfca:npm test79/79 pass (matches PR body's 79/79 claim),typecheckclean (3 projects),syntaxcheckclean. Initial parity run failed only on missingesbuildbeforenpm ci— environmental, resolved, then green. - Native Beta 156/Chrome 150 browser probes cited in the PR body were not re-run here; the doc's own scoping makes them non-load-bearing for merge.
No findings. Nothing left open by design — no checklist needed.
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
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.
The vendored TS/JS result guards discarded execution callback options, preventing cooperative tools from receiving the caller cancellation signal. This preserves options identity, adds caller cancellation to the event bridge, and keeps registration disposal separate from active execution. Existing result guards and mutation safety remain intact.
Types and imperative simulation now distinguish callback values from serialized client results, default omitted input to {}, and include dated ModelContext event/debugging guidance. The README callout describes the existing v0.7.0 mutation journal.
Validation:
Cancellation remains cooperative; handlers must honor signals before state commits. No native WPT, declarative or debugging-annotation conformance claim. No continuation-token implementation. No website deployment, release, or package publication. Shipment requires an exact-head CI/AI-review verdict and separate approval.