feat(studio): fall back to a WebMCP polyfill where the browser has none - #3514
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
Adversarial R1 on the WebMCP polyfill lazy-load path. HEAD 10888aa. Design is deliberate and the tradeoffs in the description are honest — this is not a REQUEST_CHANGES; it is a FINDINGS note before merge because two of them turn silent in production.
Standards checklist
- (a) Types across the async boundary: OK —
Reflect.get+isModelContextguard avoids the double-declaration ofDocument.modelContextcleanly. - (b) Tests: partial — see F2.
- (c) Edge cases: mostly covered; see F1 and F4.
- (d) Contract invariants (no double-load, no clobber of partial native): OK for the happy paths; see F3 for the partial-native case.
- (e) Telemetry: not wired — see F1.
- (f) i18n: N/A.
Findings, by severity
F1 — HIGH — polyfill load path is invisible in production.
polyfill.ts:31,35 and useStudioAgentTools.ts:83 both route through makeStudioDebugLogger("webmcp"), which is off unless localStorage['hf-webmcp-debug']==="1". packages/studio/src/telemetry/ already ships a real PostHog client with agentRuntimeProperty() tagging that is designed for exactly this. Concrete failure: an ad-blocker / enterprise CSP / stale-chunk after a deploy blocks the dynamic import; importPolyfill swallows it and returns null, useStudioAgentTools logs "skipped" at the debug channel, tools go silent — and nobody watching the rollout of an Origin Trial fallback can see it. Please emit at least three distinct events into telemetry/client.ts: webmcp.native_present, webmcp.polyfill_loaded, webmcp.polyfill_failed (with error.name). Without them this ships with aggregate silence on the failure class the PR was written to catch.
F2 — MEDIUM — the branch that motivates the whole file has no test.
polyfill.test.ts uses vi.mock("@mcp-b/global", () => ({})) — the mock never throws, so the catch in importPolyfill is unreached. That means the memo-null semantics after a failed load (does a second call retry, or is it stuck at null for the life of the tab?) are also unverified. Reading the code: pending is set to the rejected/null-resolving promise and reused, so a transient CSP failure sticks until reload. Worth a test that mocks @mcp-b/global to throw, asserts resolves.toBeNull(), asserts the second call returns the same settled null, and pins that as intended (or, if it isn't, resets pending on failure).
F3 — LOW — partial-native + non-configurable descriptor is a silent no-op.
isModelContext correctly rejects a native document.modelContext whose registerTool isn't a function, and the code falls through to loadModelContextPolyfill(). @mcp-b/global defines the property via Object.defineProperty; if a future Chrome ships document.modelContext as configurable: false, the polyfill's define will throw and land in the catch. F1's telemetry would surface it; without F1 you learn about it from a support ticket.
Nits
isModelContextis atypeof registerTool === "function"probe. A stubbed extension writing{ registerTool: () => 1 }(non-Promise-returning) passes the guard.registrar.ts's failure path catches thrown errors but a stub that neither throws nor resolves would hang. Very low likelihood, calling it out only because "typeof vs behavior" is the pattern to name.- No
size-limit/bundlewatchgate visible on the studio bundle. The dynamic chunk brings 5 transitive@mcp-b/*packages +@modelcontextprotocol/server+@types/chrome. Never fetched by native-capable browsers, but worth a follow-up size gate so a@mcp-b/globalminor bump can't quietly balloon the fallback chunk.
Verdict: FINDINGS — F1 is the merge-blocker for me; F2/F3 are cleanups. Design + PR body + comment discipline are unusually good.
— Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
🟡 Endorsing Via's F1 telemetry finding as merge-material and adding one cross-stack observation. HEAD 10888aaf.
Mechanism verified — co-witness
Same reads as Via: feature detection via Reflect.get(doc, "modelContext") + isModelContext() structural guard (not UA sniffing); native ?? (await loadModelContextPolyfill()) composition at useStudioAgentTools.ts:81-96 with abort re-check post-await; dynamic import("@mcp-b/global") at polyfill.ts:28 per Miguel's bundle-preservation note; pending ??= importPolyfill() giving one module-level promise for two-mount dedup; native path never touches the polyfill. Not duplicating Via's checklist.
Endorse Via's F1 as merge-material
The failure class this PR exists to catch is invisible in production without telemetry wiring. Ad-blocker / enterprise CSP / stale-chunk after a deploy blocks the dynamic import; importPolyfill swallows it and returns null; the app boots with tools silently disabled; the debug logger is off unless localStorage['hf-webmcp-debug']==="1". Concur webmcp.native_present / webmcp.polyfill_loaded / webmcp.polyfill_failed (with error.name) into telemetry/client.ts is the right shape — Studio already ships a PostHog client with agentRuntimeProperty() tagging. Without it this ships with aggregate silence on the exact failure the fallback was written to catch.
Cross-stack observation (differentiated)
#3510's write-failure reporting channel is not affected by the polyfill layer. Traced: runReportedDomEditCommit's outcome contract lives per-tool at the app boundary; the polyfill only stands up document.modelContext.registerTool for reachability, not the tool result shape. The tagged-union outcome that #3510 establishes will preserve across native | polyfill paths equally. No cross-PR leak on the failure signal.
On Via's F2 (catch-block untested)
Concur — vi.mock("@mcp-b/global", () => ({})) never throws, so the memo-null semantics (does a second call retry, or does pending stick at rejected null for the tab's life?) are untested. Reading the code: pending ??= importPolyfill() sticks the failed load. Worth pinning intent-or-fix.
What I didn't verify
- Emitted bundle actually keeps a bare
import("@mcp-b/global")— trusted Miguel's word, didn't rebuild. - Runtime behavior of
@mcp-b/global's in-page MCP server (transitively via@modelcontextprotocol/server@2.0.0) — trusted Miguel's package audit, didn't read the transitive source. - Real Chrome behavior with Origin Trial — Miguel's E2E capture confirms.
— Review by Rames D Jusso
miguel-heygen
left a comment
There was a problem hiding this comment.
Decision for the bundle-size finding.
No size gate protects the fallback chunk.
Declined for this PR as a separate CI initiative. The runtime boundary remains narrow: polyfill.ts:27-29 uses a dynamic import, and useStudioAgentTools.ts:83-86 never enters it when native WebMCP is present. A repository-wide size budget needs its own owner and threshold rather than an unreviewed policy baked into this fallback change.
miguel-heygen
left a comment
There was a problem hiding this comment.
Resolution for failure semantics and the shape-guard nit.
The failure branch is untested and a transient failure sticks for the tab.
Fixed in 0903e01fc. polyfill.ts:49-58 shares one concurrent attempt, keeps successful loads cached, and clears only a null result so a later mount can retry. polyfill.test.ts:64-97 covers missing context, a thrown TypeError, telemetry, and successful retry.
A non-Promise registerTool stub could hang.
That specific failure does not reproduce. registrar.ts:71-73 awaits the return value, and JavaScript await resolves scalar values immediately. The structural guard can still accept a nonconforming injected function, but behavior-probing it would require calling an untrusted method during feature detection, so no probe was added.
miguel-heygen
left a comment
There was a problem hiding this comment.
Resolution for the production visibility and partial-native findings.
Native, loaded, and failed fallback paths are invisible in production.
Fixed in 0903e01fc. polyfill.ts:27-44 emits webmcp.polyfill_loaded or webmcp.polyfill_failed with only the error class name; useStudioAgentTools.ts:83-99 emits webmcp.native_present. Tests pin success, missing context, and thrown failure telemetry.
A partial non-configurable native descriptor becomes a silent no-op.
The fallback package still owns installation behavior, but this case is no longer silent. A thrown descriptor failure reaches polyfill.ts:39-44 and emits webmcp.polyfill_failed with the error name.
vanceingalls
left a comment
There was a problem hiding this comment.
R2 delta re-verify. HEAD 0903e01f. Prior HEAD was 10888aa. Read-only pass; not stamping.
Verdict: R1 findings resolved; ready for approval by another reviewer.
Per-R1 finding delta
HIGH — invisible polyfill telemetry. FIXED. Three events wire through packages/studio/src/telemetry/client.ts — the real fetch-based PostHog client that POSTs to us.i.posthog.com/batch/, not the debug logger:
webmcp.native_present— fires inuseStudioAgentTools.tsL92 whengetModelContext()returns a native context, before falling back.webmcp.polyfill_loaded— fires inpolyfill.tsL34 afterawait import("@mcp-b/global")succeeds AND the model context materialises.webmcp.polyfill_failed({ error_name })— fires on both failure branches: the caught-error path (L44) and the "package loaded but defined nothing" path (L30) witherror_name: "ModelContextMissingError".
The debug logger stays alongside (dev only), but the PostHog transport is the primary and matches the R1 recommendation.
MED — motivating branch has no test. FIXED. polyfill.test.ts adds 5 tests, including two that exercise the fallback branch specifically:
- "returns null when the package loads but defines nothing" — asserts null return and
polyfill_failed(ModelContextMissingError). - "reports a polyfill failure and lets a later mount retry" — mocks a getter that throws
TypeError, asserts null +polyfill_failed(TypeError), then swaps in a real modelContext and asserts the retry returns it. This is a genuine transient-then-recover test, not always-fail-or-always-succeed (R2 lens 4 satisfied). Thepending = nullreset on null-return enables the retry.
LOW — partial-native / non-configurable descriptor no-op. SHIFTED-TO-OBSERVABLE. The new isModelContext guard in types.ts rejects shapes without a registerTool function; if a browser exposes a partial or malformed document.modelContext, the code falls back to the polyfill path — and if the polyfill also can't populate a valid context (non-configurable descriptor), it emits polyfill_failed(ModelContextMissingError). Silent no-op is no longer silent. The non-configurable-descriptor case isn't explicitly test-covered, but the behavior is safe (returns null, boot continues) and is now countable in PostHog. Acceptable resolution.
Adversarial cardinality check
Clean. native_present and polyfill_loaded carry zero properties. polyfill_failed carries only error_name — error.name is a bounded set (TypeError, SyntaxError, ChunkLoadError, ModelContextMissingError, NonError). No error.message, no error.stack, no PII, no user IDs. Cardinality budget is respected.
New findings
INFO — no timing signal for cross-PR #3511 race. #3511 reads document.modelContext at mount. If it mounts before this PR's polyfill import resolves, the read is a miss. The new events let you count native_present + polyfill_loaded vs polyfill_failed per session, which is a reasonable rate proxy, but there's no timing marker (polyfill_loaded_at or duration property) that would let #3511 diagnose "my mount happened during the load window." Not a blocker for THIS PR — #3511 owns its own read-time contract — but worth a follow-up if the race turns out to matter in the wild.
CI
All 8 required checks green on 0903e01f. regression, preview-regression, player-perf, Preflight (lint + format), Perf: {drift,fps,load,parity,scrub}, Preview parity, WIP all pass.
— Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
🟢 #3514 R2 LGTM at 0903e01f. All three R1 concerns closed by durable shape change, not test-only patches.
R1 concerns — verified fixed by shape:
- Via's F1 telemetry (I endorsed as merge-material): PASS.
webmcp.native_presentfires atuseStudioAgentTools.ts:84whengetModelContext()returns a truthy native.webmcp.polyfill_loadedfires atpolyfill.ts:37on the truthy branch afterimport("@mcp-b/global")resolves.webmcp.polyfill_failedfires twice with structured discriminator:polyfill.ts:35for loaded-but-empty (error_name: "ModelContextMissingError"),polyfill.ts:43for the outercatch(error_name: error.namewith"NonError"fallback). All route throughtrackEventfrom../telemetry/client— the existing fire-and-forget PostHog client withshouldTrack()policy gate, canary tagging, browser sys-meta, and breadcrumb capture already baked in. Exactly the wiring Via prescribed and I endorsed. - Retry coverage for transient failures: PASS via reset-on-failure with promise-identity race guard.
polyfill.ts:49-58: concurrent callers still share the in-flightpending(if (pending) return pending), so the racing-mounts dedup Miguel's original guard existed for is preserved; on settlement,void attempt.then((mc) => { if (mc === null && pending === attempt) pending = null })clearspendingonly if the null-resolving attempt is still the current one — so a later mount re-entersimportPolyfill(). Successful loads (mc !== null) stick. Thepending === attemptcheck is belt-and-braces against a stale reset from a superseded attempt. Right choice — no backoff (would re-fetch the failing chunk on every mount), no test-only pin. - Via's F2 catch-block untested: PASS.
polyfill.test.ts:75-97new test "reports a polyfill failure and lets a later mount retry" installs a throwingObject.defineProperty(document, "modelContext", { get() { throw new TypeError(...) } })sogetModelContext()throws into the outercatch, assertstrackEventwas called withwebmcp.polyfill_failed+{ error_name: "TypeError" }, then removes the throwing getter, installs a real modelContext, calls again and asserts the retry is a fresh promise (retry).not.toBe(first)) that resolves to the new modelContext. That's stronger than Via's ask — she said "assert same settled null OR reset-on-failure"; Miguel picked reset-on-failure and pinned it with an actual successful-retry assertion. The loaded-but-empty branch (polyfill.test.ts:63-72) also now asserts telemetry + retry-yields-fresh-promise.
Cross-stack (my R1 observation): #3510's outcome-contract preservation stands. R2 delta touches only polyfill.ts (telemetry + retry mechanism), one trackEvent("webmcp.native_present") line in useStudioAgentTools.ts:84, and their tests. No writes to document.modelContext, no changes to the tool-invocation path, no touch of runReportedDomEditCommit's tagged-union result shape. The polyfill layer remains a reachability layer; #3510's write-failure reporting channel is unaffected across native | polyfill paths equally.
What I didn't verify:
- Whether
agentRuntimeProperty()decoration (Via's phrasing) is applied to these specific events —trackEventroutes through the standard pipeline that addscanaryEventProperties()+getBrowserSystemMeta()universally, so events land in the same PostHog project with the same tagging as every other studio event. IfagentRuntimeProperty()is a separate caller-applied decorator elsewhere, that's an orthogonal follow-up, not an R2 blocker. - Real-browser telemetry fire on a stable Chrome without WebMCP — pinned by tests, not by rebuild + PostHog inspection.
webmcp.native_presentfires before the polyfill await and before the abort re-check inuseStudioAgentTools.ts:83-85, so a StrictMode double-mount or a fast unmount will over-countnative_presentby one. Not a blocker (browser-capability signal, not registration-success signal) but noting for observability tuning.
Peer state: Via's R1 5036545493 at 10888aa. No R2 from Via yet at 0903e01f — the three most recent reviews are Miguel's explanatory replies (5036871351/386/512) walking through the resolutions. My R2 posts as first delta pass on the fix HEAD. HEAD 0903e01fcd76d4d461ee51ecadbe75568deb7f74; CI green (Detect changes / Perf drift/fps/load/parity/scrub / Preflight / Preview parity all PASS); mergeStateStatus: CLEAN; mergeable: MERGEABLE; base feat/studio-webmcp-look (stacked on #3511).
— Review by Rames D Jusso
|
@vanceingalls @james-russo-rames-d-jusso Both R2 verdicts are green at 0903e01, so this is a request for the formal APPROVED review, not another analysis pass. Via, your timing-signal INFO note is acknowledged as follow-up work and is not a blocker on this PR. There is no other reviewer assigned to this stack, and APPROVED is a review state you have used in this repo before. Please submit it yourself, or name the specific remaining reason you will not. Rames, please say whether GREEN is your terminal state or whether you can submit APPROVED; if you can, please stamp this PR. |
jrusso1020
left a comment
There was a problem hiding this comment.
APPROVED at 0903e01fcd76d4d461ee51ecadbe75568deb7f74.
Read the load and failure paths independently. No defects.
The retry-reset is correct, including the part that's easy to get wrong: pending === attempt (polyfill.ts:57) keeps the concurrent-mount dedup while still clearing a failed attempt, and because importPolyfill catches everything and resolves rather than rejecting, callers already holding the promise still get null instead of an unhandled rejection. Telemetry is mutually exclusive across the three outcomes and the only dimension is a bounded error_name.
One thing I specifically chased and cleared: :57 tests modelContext === null with strict equality, which would silently fail to reset pending if the resolved value were ever undefined — the "a transient failure must not disable WebMCP for the rest of the tab" guarantee would quietly not hold. It's safe: getModelContext returns isModelContext(candidate) ? candidate : null (types.ts:89), exactly null, never undefined. Worth keeping that in mind if that helper's return type is ever loosened, since the failure would be invisible.
Worth knowing rather than fixing: this adds a new third-party runtime dependency whose entire job is installing a global capability surface an external agent talks to — @mcp-b/global@^5.0.1, pulling @mcp-b/transports, @mcp-b/webmcp-polyfill, @mcp-b/webmcp-ts-sdk, @mcp-b/webmcp-types and @modelcontextprotocol/server@2.0.0. The caret range is pinned in practice by the committed bun.lock (39 lines, scoped cleanly to this tree, no unrelated churn), and the dynamic import means a browser with native support never downloads it. Flagging it because it's the first dependency of its kind in Studio, not because anything here is wrong.
Not gating on mergeStateStatus=DIRTY / mergeable=CONFLICTING — merge state isn't mine to hold a stamp on. The restack onto the new #3511 head is still yours before this can land.
— Rames
The base branch was changed.
jrusso1020
left a comment
There was a problem hiding this comment.
APPROVED at 0903e01fcd76d4d461ee51ecadbe75568deb7f74 — re-stamp after a base-change dismissal.
Head unchanged since review 5036979253, so the verdict carries: no defects. The pending === attempt retry-reset is still correct, and getModelContext still returns exactly null, so the strict-equality check at polyfill.ts:57 remains safe.
Dismissal cause: base_ref_changed + review_dismissed at 03:35:51Z, dismissal_message: "The base branch was changed." — base moved from feat/studio-webmcp-look to main. Nothing to do with the head or the code.
This is now the cumulative tip of the stack. Against main it diffs 38 files / +2047 — every file from #3510 and #3511 plus its own 7. Merging this one alone ships all three PRs' content in a single go, which is the thing to be deliberate about: it is no longer possible to land "just the polyfill." Merge ascending (#3510 → #3511 → #3514) so each lands the increment it was reviewed as.
Like #3511, this head predates #3510's annotation fix (0 occurrences of Promise<DomEditCommitOutcome> here vs 3 at 69c4402f). Ascending order makes that a non-issue — the merge keeps main's newer copies.
Still yours before this can merge: it was DIRTY/CONFLICTING against the old parent, and the retarget onto main changes what that resolution looks like.
— Rames
WebMCP is an Origin Trial. Chrome 149 and Edge 150 have it behind a flag,
ChatGPT Desktop ships it, and everything else does not. Without a fallback the
tools registered in the previous change are invisible on stable Chrome, which
is exactly where a bridge extension would connect from.
Adds `@mcp-b/global` (MIT) as a DYNAMIC import, so a browser with native
support never fetches it. Verified in the build output rather than asserted:
the bundle keeps a bare `import("@mcp-b/global")` instead of inlining it.
Chosen over the smaller `@mcp-b/webmcp-polyfill` because that one only defines
`document.modelContext`. `@mcp-b/global` also stands up the in-page MCP server
a bridge extension attaches to, and serving that case is the only reason the
fallback exists at all.
The load is guarded by a module-level promise so two mounts racing share one
load, and an import failure is caught and logged rather than thrown: a missing
agent surface must never stop Studio booting. The registration path re-checks
the abort signal after the await, so unmounting mid-import registers nothing.
Two things the type checker forced, both worth keeping:
Installing the package brings its own global `Document.modelContext`
declaration, which collided with the local one. Studio now reads the property
through a type guard instead of augmenting `Document`, so there is only one
declaration of that global and it is the package's.
Studio keeps its own narrow tool types rather than importing the package's.
Theirs overload `registerTool` to infer argument types from a literal
`inputSchema`, which helps when registering one tool inline and fights a
uniform registration loop. The comment in `types.ts` says so, and names the
drift risk that choice accepts.
The polyfill test asserts promise identity rather than counting imports. The
ESM registry dedupes the import either way, so a call count would pass whether
or not the guard existed.
0903e01 to
4cac053
Compare
What
Falls back to
@mcp-b/globalwhen the browser has no nativedocument.modelContext, so the tools from #3511 are reachable on browsers that have not shipped WebMCP.Stacked on #3511.
Why
WebMCP is an Origin Trial. Chrome 149 and Edge 150 have it behind a flag, ChatGPT Desktop ships it, Brave Leo is experimental, and everything else has nothing. Without a fallback the tools are invisible on stable Chrome, which is exactly where a bridge extension would connect from.
How
Dynamic
import(), so a browser with native support never fetches it. Verified in the build output rather than asserted: the bundle keeps a bareimport("@mcp-b/global")instead of inlining the package.Chosen over the smaller
@mcp-b/webmcp-polyfillbecause that one only definesdocument.modelContext.@mcp-b/globalalso stands up the in-page MCP server that a bridge extension attaches to, and serving that case is the only reason the fallback exists.The load is guarded by a module-level promise so two mounts racing share one load. An import failure is caught and logged rather than thrown, because a missing agent surface must never stop Studio booting. The registration path re-checks the abort signal after the await, so unmounting mid-import registers nothing.
Two things the type checker forced, both worth keeping:
Installing the package brings its own global
Document.modelContextdeclaration, which collided with the local one. Studio now reads the property through a type guard instead of augmentingDocument, so there is exactly one declaration of that global and it is the package's.Studio keeps its own narrow tool types rather than importing the package's. Theirs overload
registerToolto infer argument types from a literalinputSchema, which helps when registering one tool inline and fights a uniform registration loop.types.tssays so, and names the drift risk that choice accepts.Dependency note for review
@mcp-b/globalis MIT and comes from the publicWebMCP-org/npm-packagesrepository. It pulls four transitive@mcp-b/*packages plus@modelcontextprotocol/server. It is loaded lazily and only on browsers without native support, but it does run in a page that has file-write access to the user's project through the dev server. Worth a look if that tradeoff is not one we want.Test plan
Unit tests added/updated
Manual testing performed
Documentation updated (if applicable)
polyfill.test.ts: the shared-load guard, reuse after settling, a fresh load after reset, and null when the package loads without defining anything.The test asserts promise identity rather than counting imports. The ESM registry dedupes the import either way, so a call count would have passed whether or not the guard existed. Worth noting because the first version of this test did exactly that and was vacuous.
Full package suite 4533 passing across 408 files.
bunx tsc --noEmitclean.bunx fallow audit --fail-on-issuesclean.Not verified in a real browser. The end-to-end capture needs
chrome://flags/#enable-webmcp-testingand lands with the write tools.Scope added during review