feat(studio): expose Studio's live state to an agentic browser (WebMCP) - #3511
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
Head SHA a47fee4e29dfc3daa5cdb3454bd3855e5adcaf2f. Nice PR — the ref-based registration, the reasoning about the AbortError handshake, the choice to resolve-with-failure rather than reject, and the deliberate "breaking the empty deps array fails exactly one test" invariant all land well. Handle scheme with lastIndexOf('#') correctly threads the CSS-id-in-selector needle. Findings are small.
Verdict: FINDINGS (2 medium, worth resolving before write tools stack on top).
Standards checklist
- Types / typecheck: clean; local WebIDL mirror in
types.tsis scoped and documented. - Tests: 43 tests, semantics-not-presence (StrictMode remount as AbortError, DOMException name preservation, cross-realm resolve, filter-vs-truncated count). Non-vacuous claim is verified by construction.
- Read-only guarantee: enforced by shape (execute returns pure-built objects; no live refs escape). Snapshot copies
selectedElementIds; liveplayer.elementsis immediately mapped into newLookElement[]. Cross-realmHTMLElementis not held across calls. - Serialization: all fields are JSON-safe (numbers, strings, bounded objects).
selection.element(HTMLElement) is deliberately not passed through — only measured fields. - i18n: N/A (headless tool surface).
Findings
-
[MEDIUM]
agentToolsEnabledis read once at mount and never re-read.useStudioAgentTools.ts:70-88— the effect has empty deps and callsreadStudioUiPreferences()inside the effect body. If a settings UI toggles the pref while Studio is open, nothing unregisters until full page reload. That's a silent divergence between the pref UI and reality — the very kind of "you turned it off but it's still on" story that the security posture leans on "the browser gates every call" to survive. Either subscribe to thestorageevent, subscribe to whatever emits the local change, or document the reload requirement next to the pref field. Not blocking this PR alone, but the write tools stack must not inherit this shape. -
[MEDIUM] Polyfill race with #3514 (lazy WebMCP polyfill).
useStudioAgentTools.ts:79-83—getModelContext()readsdocument.modelContextsynchronously in an empty-deps effect. If the lazy polyfill in sister PR #3514 installs afterStudioAgentToolsmounts (order ofEditorShellmount vs. polyfill install), the effect sawnull, logged "absent", and never retries. There's no ready-signal or MutationObserver ondocument.modelContext. Confirm the polyfill ships before this component mounts, or add a one-shot retry when the polyfill fires its ready signal. The commit message names "document not navigator" as a deliberate choice — worth naming the install-order dependency for the same reason. -
[LOW]
StudioLookhas noschemaVersion.lookTools.ts:20-45— the description prose is the only agent-facing contract, so field drift across Studio versions is invisible to agents that pin. A bareschemaVersion: 1on the returned object costs nothing now and lets future readers detect skew. TheSTUDIO_LOOK_DESCRIPTIONalready acts as a versioned contract for humans; make it one for machines too. -
[LOW]
filterinput has nomaxLength.lookTools.ts:120-124— an agent can send an arbitrarily large filter string;.trim().toLowerCase()allocates a copy. Cap at ~128 chars to match tool-name bounds. -
[LOW / carry-forward] Default-on relies entirely on the browser permission prompt for consent.
studioUiPreferences.ts:37-42— the code acknowledges this. Flagging so the write-tools PR treats the browser prompt as the sole gate rather than assuming this pref is meaningful protection.
CI: all 13 latest-per-name check runs pass (regression, preview-regression, player-perf, Preflight, Preview parity, WIP). Mid-stack ci.yml main-only trigger means the deep unit suite isn't visible here by design; author confirms 4528/407 local pass twice + tsc + fallow clean.
— Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
🟡 Second read on the read-only studio_look registration. Read-only-by-shape mechanism reads the same way as Via's review 5036546056; zero overlap on findings — three additional concerns worth naming. HEAD a47fee4.
Read-only enforcement is structural — co-witness
Same reads as Via: describeElement (lookTools.ts:132) and describeSelection (lookTools.ts:143) both mint fresh plain-scalar objects; the original TimelineElement and the raw HTMLElement reference on DomEditSelection never cross the tool boundary. Belt-and-suspenders: WebMCP JSON-serializes at the boundary. Freshness pinned by depsRef.current.getSnapshot() + usePlayerStore.getState() at StudioAgentTools.tsx:25. Not duplicating her mechanism trace.
Additional findings
-
canWriteis a documented lie in this PR (cross-stack).writeBlockedReasonis hardcodednullinStudioAgentTools.tsx:44, forcingcanWrite: trueon every response. The description prose (lookTools.ts:194) tells agents "CheckcanWritebefore attempting an edit." Miguel's PR body flags this and warns future write tools MUST NOT trust the field — good discipline. But sincecanWritestill ships as advice today, an intermediate agent build reading the tool description before write tools land will fail-open. Whichever PR wires the write tools needs the real save-queue / external-conflict wiring before the write descriptions also point atcanWrite. Flag for the stack. -
Multiple
EditorShellmounts silently collide on the tool name.registrar.ts:99dedupes within a batch but not across separately mounted components. Two liveStudioAgentToolson the samedocument.modelContextproduceInvalidStateErroron the second registration — the registrar catches and logs but silently drops the second mount's tools.EditorShelllooks singleton today; if a modal or side-preview ever mounted a second editor, agent tools go missing with no user-visible signal. Worth a docstring naming the singleton assumption. -
selectedElementIdspopulated but never surfaced.StudioAgentTools.tsx:34populates it intoStudioLookSnapshot;buildStudioLooknever reads it. Multi-select information is dropped at the tool boundary — either intentional (multi-select deferred) or oversight. One-line comment if intentional, follow-up if not. Non-blocking for a read-only PR.
On Via's polyfill-race finding (her F2 MEDIUM)
Correct at this PR's HEAD (base is fix/domedit-commit-reporting so #3514 isn't in tree here yet). At #3514's HEAD the polyfill load is composed INTO the same hook — native ?? (await loadModelContextPolyfill()) at useStudioAgentTools.ts:81-96, abort re-checked post-await — so the race resolves when #3514 lands. Worth calling the merge-order dependency out in the commit message.
What I didn't verify
- Real Chrome behavior with the Origin Trial (Miguel says the E2E capture lands with the write tools).
- Whether
usePlayerStore.getState()at first tool invocation could return partially-initialized state — probably fine post-mount, but I didn't trace the store's init path.
— Review by Rames D Jusso
miguel-heygen
left a comment
There was a problem hiding this comment.
Evidence for the preference lifecycle finding.
agentToolsEnabled is read once and never re-read.
Confirmed as the current mount boundary, not a live-toggle defect. useStudioAgentTools.ts:71-91 reads the preference in the intentionally mount-once effect. Repository search found no in-product writer or settings control; the persisted writer is currently used by tests and explicit storage configuration. The reload requirement is documented for users. Adding a storage subscription here would create a new live-toggle contract rather than repair an existing one.
miguel-heygen
left a comment
There was a problem hiding this comment.
Evidence for the polyfill ordering finding.
A polyfill installed after StudioAgentTools mounts would never register tools.
The race exists only if a separate late installer is introduced. At the #3511 head there is no polyfill installer. The stack dependency in #3514 resolves the fallback inside this same registration effect: useStudioAgentTools.ts:83-92 reads native context, awaits loadModelContextPolyfill() when absent, and rechecks abort before registration. #3514 must land after #3511 for non-native browsers; there is no independent installer left to race.
miguel-heygen
left a comment
There was a problem hiding this comment.
Resolution for the schema findings.
StudioLook has no schemaVersion.
Declined intentionally. WebMCP consumers discover tools dynamically and read the current description and input schema in-session rather than compiling against a versioned REST shape. A version field would not protect the actual discovery boundary here.
filter has no maxLength.
Fixed in 0f938fc6b. lookTools.ts:91-92,135-140,166-172 gives the schema and execution the same 128-character owner. lookTools.test.ts:137-148 proves execution is bounded before normalization and pins the schema limit.
miguel-heygen
left a comment
There was a problem hiding this comment.
Resolution for the remaining #3511 contract findings.
canWrite is always true, selectedElementIds is dead plumbing, and multiple shells can collide.
Fixed in b13832b44 and e205fdfcb. lookTools.ts:72-82,151-163 no longer exposes canWrite or writeBlockedReason; lookTools.test.ts:210-215 pins their absence until the real write gate lands. The unused multi-select snapshot field was removed from both the producer and contract. registrar.ts:1-8 now documents that tool names are document-scoped and registration relies on one live EditorShell; the existing duplicate check owns only one registration set.
Default-on relies on browser-level consent.
Kept as the explicit product choice. studioUiPreferences.ts:37-42 states that absent means on and that registration is not reachability. The default is pinned by useStudioAgentTools.test.tsx:176-184. This does not claim a per-call native prompt cadence.
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
🟢 #3511 R2 GREEN at e205fdfc. All three of my R1 concerns closed structurally, plus Via's F4 (filter maxLength) landed with the strongest pattern. Read-only PR is clean; Miguel's stated deltas match the diff exactly.
R1 concerns — verified fixed by shape:
canWritelie: PASS — option (a), the strongest.writeBlockedReasonandcanWriteare gone fromStudioLookSnapshot,StudioLook,buildStudioLook, andStudioAgentTools.tsx:34-44(the hardcodednullproducer). Tool-description guidance "CheckcanWritebefore attempting an edit" is deleted fromSTUDIO_LOOK_DESCRIPTION(lookTools.ts:190).lookTools.test.ts:210-215actively pins the absence with.not.toHaveProperty("canWrite")/.not.toHaveProperty("writeBlockedReason")— so a future accidental re-add fails a test. A field can't lie if it's not there.- EditorShell singleton doc: PASS. Module-level docblock at
registrar.ts:1-8names the assumption ("Studio relies on its single liveEditorShellmounting oneStudioAgentTools"), the failure mode ("a second live shell would register the same names and receiveInvalidStateError"), and what the dedup check does not own ("only owns duplicates within one registration set"). Doc lives at the collision site — exactly where a future refactor would trip over it. selectedElementIdsplumbing: PASS — option (a). Field removed from producer (StudioAgentTools.tsx:34), fromStudioLookSnapshotcontract (lookTools.ts:21-40), and from both test-fixture snapshots. Multi-select is deferred by absence rather than documented as intentionally deferred — cleaner outcome than the "one-line comment" fallback I offered.
Bounded filters (Miguel's addition, closes Via's F4):
- Mechanism is defense-in-depth with a single owner:
MAX_FILTER_LENGTH = 128const, applied twice — (1) at parse-time viaSTUDIO_LOOK_INPUT_SCHEMA.properties.filter.maxLength = 128so the agent sees the bound in the schema and the browser can reject over-limit input; (2) at execution-time viainput.filter?.slice(0, MAX_FILTER_LENGTH)(lookTools.ts:140) so a runtime bypass still can't allocate a giant lower-cased copy.lookTools.test.ts:137-148verifies BOTH — a filter of"x"*128 + "y"*10_000still matches the label of"x"*128, AND the schema'smaxLengthis pinned to the constant. Textbook pattern; nothing to add.
What I didn't verify:
- Real-Chrome behavior with the Origin Trial (still Miguel's "lands with the write tools" caveat, unchanged from R1).
- Whether any callsite still reads
selectedElementIdsoutside the WebMCP tree — the removal from producer+contract is symmetric withinpackages/studio/src/webmcp/, but I didn't grep for external consumers ofStudioLookSnapshot(unlikely since it's the tool-boundary type, but not exhaustive). - I did not re-verify Miguel's "no in-product writer for
agentToolsEnabled" claim to Via's F1 by grep. That's Via's finding; her call to endorse or push.
Peer state: Via has not posted an R2 review yet — her R1 (5036546056) is her only review. Miguel posted four evidence/resolution comment-reviews at R2 head (5036871384, 5036871424, 5036871467, 5036871588). Not parallel-posting Via's F1/F2 — my R2 layers on top of her F4 closure and covers my three R1 concerns; her F1 (mount-once pref) and F2 (polyfill race) are hers to close. Polyfill-race cross-stack: base still targets fix/domedit-commit-reporting (#3510), so my R1 note "resolves once #3514 lands" still holds.
— Review by Rames D Jusso
vanceingalls
left a comment
There was a problem hiding this comment.
R2 adversarial delta re-verify against head e205fdf.
Fix claims
- Bounded filters — CLOSED.
MAX_FILTER_LENGTH = 128atpackages/studio/src/webmcp/tools/lookTools.ts:1041;.slice(0, MAX_FILTER_LENGTH)atlookTools.ts:1089;Math.min(requested, DEFAULT_LIMIT)atlookTools.ts:1098; schemamaxLength: 128atlookTools.ts:1120; test atlookTools.test.ts:864-875asserts pre-normalize bound. - Premature write state removed — CLOSED.
LookSelectionatlookTools.ts:1003-1019carries nocanWrite/writeBlockedReason;lookTools.test.ts:937-942explicitly asserts absence. - Dead selection plumbing removed — CLOSED. Every field on
LookSelectionis populated fromDomEditSelectionbydescribeSelectionatlookTools.ts:1057-1074; no unread fields survive. - Singleton registration documented — PARTIAL.
registrar.ts:539-542names the "single liveEditorShellmounts oneStudioAgentTools" invariant, but see MED #2 below — the cross-file polyfill ordering invariant is a different assumption and is still undocumented.
R1 findings
- MED #1 (agentToolsEnabled read once at empty-deps mount) — SHIFTED, not closed.
useStudioAgentTools.ts:1508still readsreadStudioUiPreferences().agentToolsEnabledinside auseEffect(..., []), so toggling the preference mid-session neither registers nor unregisters until reload. The comment atuseStudioAgentTools.ts:1486-1502explains empty deps in terms ofDomEdithandler identity churn (that reasoning is fine), but does not address the flag semantic. Either add a "requires reload" note next to the preference, or hoist the flag into a subscribed read so a toggle re-runs the effect. - MED #2 (cross-PR race with #3514 polyfill) — NOT ADDRESSED.
types.ts:1220-1223readsdocument.modelContextsynchronously;useStudioAgentTools.ts:1514calls that inside the mount-time empty-deps effect. No deferral, observer, or retry — and no explicit documented ordering invariant that #3514's polyfill installs beforeEditorShellBodymounts. A polyfill that lands in a lazy chunk or a siblinguseEffectwill race, and the only trace is a debug log ("document.modelContextabsent"). Please either document the polyfill-install-early invariant intypes.ts/registrar.ts, or add a one-shot retry (rAF / microtask) againstdocument. - LOW
schemaVersiononStudioLook— NOT ADDRESSED.lookTools.ts:1021-1031still ships noschemaVersion. Given the pre-stable WebMCP surface and the "spec-mirror" framing oftypes.ts:1155-1163, aschemaVersion: 1(or an explicit comment declining it) would let agent-side / cache reasoning survive future field additions. - LOW premature
canWrite/writeBlockedReason— CLOSED. - LOW dead selection plumbing — CLOSED.
Adjacent-boundary defects (6 axes)
- Telemetry:
registerStudioToolsatregistrar.ts:624-648returns afailed[]list that distinguishesSecurityError/NotAllowedErrorfromInvalidStateError, butuseStudioAgentTools.ts:1521-1524routes the entire report to the debug logger. Aregistered:[]+ non-emptyfailed[]currently surfaces zero user-visible signal. Considerconsole.warn-ing the non-Abortcases at minimum. - HMR: the spec warning quoted at
useStudioAgentTools.ts:1495-1497— "a quick unregister-then-reregister can apply an old call's arguments against the new schema" — is defended forStrictModevia theisAbortErrorbranch inregistrar.ts:611, but HMR module reload triggers the same unregister-then-reregister window and is not distinguishable from a real teardown. Dev-only, low probability, worth naming in the comment. - Types:
describeSelectionatlookTools.ts:1072readsselection.gsapAnimations?.length ?? 0. The optional chain is defensive, but the testselectionfactory atlookTools.test.ts:752-782never setsgsapAnimations; if the realDomEditSelectiontype requires it, tests are silently under-covering it. Confirm against theDomEditSelectiondefinition.
Cross-PR race adversarial
The actual dependency on #3514 is: the polyfill writes document.modelContext before EditorShellBody's useEffect commits. Nothing in this PR asserts, tests, or documents that ordering, and nothing recovers if it fails to hold. This is the highest-value single change to make before landing — either write the invariant down, or add a bounded retry.
Singleton lifecycle
- Single live
EditorShell: assumed, documented atregistrar.ts:539-542, acceptable. StrictModedouble-invoke: handled (registrar.ts:611, test atregistrar.test.ts:438-455).- HMR: residual dev-only edge (see adjacent-defects note above).
- Concurrent second shell: fails deterministically with
InvalidStateError, surfaces in the failed[] report, not defended — acceptable given the documented invariant.
CI
All 12 checks green at e205fdfc; preview-regression, regression, player-perf, and Preflight (lint + format) all SUCCESS. mergeable=MERGEABLE.
Verdict
R1 findings partially resolved. The two MED items — flag-read-once semantic (MED #1) and polyfill-ordering invariant (MED #2) — remain. Ready for approval by another reviewer once those two are either addressed or explicitly deferred with a linked follow-up.
— Via
miguel-heygen
left a comment
There was a problem hiding this comment.
R2 follow-up fixed in 42a84c1:
- The preference contract now says changes require a Studio reload.
- The lifecycle comment now makes same-effect fallback ordering explicit and names the development-only HMR unregister/register window.
- The response contract explicitly declines schemaVersion because WebMCP consumers rediscover the current tool schema each session.
- Non-Abort registration failures now emit one bounded telemetry event per failed tool.
- animationCount now reads the existing selectedGsapAnimations channel. The prior selection.gsapAnimations source is never populated.
Verification: 22 focused tests passed, Studio typecheck passed, and all pre-commit checks passed. Both new witness tests were also proven non-vacuous by temporarily breaking their implementation paths and observing the intended failures.
|
Additional verification on 42a84c1: the complete Studio suite passes, 407 files and 4,531 tests, with 18 existing todos. |
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
🟢 #3511 R3 GREEN at 42a84c11. All R2 residuals (mine + Via's MED #1/#2 + her adjacent-boundary items) closed structurally or by explicit decision-log doc; the animationCount fix is a real stale-source bug repair, and the registration-failure telemetry lands with the exact surface Via asked for.
R2 residuals (Via's lane) — verified by shape at R3:
- Via's F1 (mount-once pref, MED #1) — CLOSED as decision-log.
studioUiPreferences.ts:41-42adds "Changes take effect on the next Studio reload because registration is intentionally scoped to one mount." Contract-of-record fix, not mechanism change. Matches your R2 evidence ("no in-product writer or settings control; persisted writer is currently used by tests and explicit storage configuration") — adding astoragesubscription would create a new live-toggle contract, not repair an existing one. - Via's F2 (polyfill race, MED #2) — CLOSED as decision-log + invariant.
useStudioAgentTools.ts:77-81names both invariants at the mount-only lookup site: "Any fallback must be awaited inside this effect before registration and then re-read here. Installing one from a sibling effect would race this mount-only lookup." That's the exact "polyfill-install-early invariant" Via asked to be written down. #3511 does not claim independent race-safety — it documents the same-effect fallback contract for #3514 to compose into, matching your R2 evidence aboutuseStudioAgentTools.ts:83-92on #3514's HEAD. - Via's LOW
schemaVersion— DECLINED with rationale, not silently.lookTools.ts:73-77new docblock: "no schema version: WebMCP consumers discover the current tool and schema when they connect rather than pinning a cached REST response contract." Right principle for the discovery boundary; the field would guard the wrong thing.
R3 fresh items — traced:
- Surfaced registration failures — mechanism is telemetry emit, one event per failed tool.
useStudioAgentTools.ts:18-26newreportRegistrationhelper:for (const failure of report.failed) trackEvent("webmcp_registration_failed", { error_name: failure.name, tool_name: failure.tool }). Abort/StrictMode-teardown is still excluded viaregistrar.tsisAbortErrorbranch (unchanged). Test pins the surfacing shape atuseStudioAgentTools.test.tsx:184-197— mocksregisterTool.mockRejectedValue(new DOMException("blocked", "NotAllowedError"))and assertstrackEventcalled with theNotAllowedErrorname +studio_looktool name. Stronger than theconsole.warnVia floated — dev-console warnings vanish; telemetry lands in observability. - Live
animationCountfix — real bug, not cosmetic. Before R3,describeSelectionatlookTools.tsreadselection.gsapAnimations?.length ?? 0. Your R3 note: "The priorselection.gsapAnimationssource is never populated" — so the field silently returned 0 for every non-empty selection in prod. Fix (a) addsselectionAnimationCount: numbertoStudioLookSnapshot(lookTools.ts:24), (b) wires it inStudioAgentTools.tsx:22,34,42by pullingselectedGsapAnimationsfromuseDomEditSelectionContextand adding it to thegetSnapshotdeps, (c) changesdescribeSelection(selection, animationCount)signature to accept it as an explicit param (lookTools.ts:114). Test atlookTools.test.ts:186-192pins the mechanism (selectionAnimationCount: 3→look.selection?.animationCount === 3). Fix routes through the DomEdit context that actually holds live animations — right source, no more silent-zero. - Schema decisions documented — two decisions land: (1) explicit no-
schemaVersionatlookTools.ts:73-77(rationale above), (2) reload-scoped preference lifecycle atstudioUiPreferences.ts:41-42. Both at the contract sites, not the PR body — so future readers touching the shape see the decision without a PR-history spelunk. - HMR — named at
useStudioAgentTools.ts:80-81: "Hot-module replacement can still create a brief unregister/register window in development; production has one document-scoped registration." Named, not defended — correct call for a dev-only edge that inherits the spec-warned unregister-then-reregister window. Matches Via's adjacent-boundary #2 exactly.
Cross-stack (carried from R2 hygiene note): baseRefOid=6cbbac02 — #3511 IS NOT on #3510 R2 HEAD 2a0a034d. Same base as R1/R2 — the restack is still pending as of R3. When it lands, #3511 picks up #3510 R2's {ok: true} as const return + preview-stale union member. Non-blocking for content review; matters for stack-merge ordering.
What I didn't verify:
- "Prior gsapAnimations source is never populated" as a codebase claim — took your R3 note at face value + verified the R3 mechanism repoints to a channel you say is populated. Full grep of every
DomEditSelectionproducer forgsapAnimationswrites would confirm; the new test pins that R3'sselectionAnimationCountparam is the live source, which is what matters for the tool contract. - Real-Chrome behavior with the Origin Trial — same "lands with the write tools" caveat from R1/R2, unchanged.
- Whether the
webmcp_registration_failedtelemetry event has a downstream monitor / dashboard wired — R3 pins emission at the source; downstream consumption is out of scope for this PR. - HMR window behavior in dev — didn't try to trigger it manually; docstring names it as a known development-only edge.
Peer state: Via's reviews to date — 5036546056 (R1, COMMENTED, a47fee4e), 5036903036 (R2, COMMENTED, e205fdfc, MED #1 + MED #2 open in her lane at that head). Miguel's R3 follow-up comment 5036930652 explicitly answers her R2 items point-by-point. Via has not posted an R3 review yet; my R3 delta is layered on hers, not parallel to it. HEAD 42a84c11cd4207a7ffa5b464db2dd8b0512e0fb2; CI UNSTABLE reason is a single pending Preview parity, not a red check; regression-shards skipping is the shell-skip-alongside-expanded pattern (benign, not the dangerous instead-of case); mergeable=MERGEABLE.
— Review by Rames D Jusso
vanceingalls
left a comment
There was a problem hiding this comment.
R3 adversarial delta re-verify against head 42a84c1.
R2 open findings — delta
- MED #1 (agentToolsEnabled mount-once semantic) — DOCUMENTED.
packages/studio/src/utils/studioUiPreferences.ts:41-42names the reload boundary on the field itself, which is where the next reader lands. Adversarial: an admin toggling the flag mid-session still silently no-ops until reload; there is no settings-UI callout, but the flag is currently opt-out with no in-product writer (per Miguel's repository-search note), so the exposure is bounded to storage-console users and tests. Acceptable as documented until a settings surface lands. - MED #2 (cross-PR polyfill ordering) — DOCUMENTED as invariant, not runtime-guarded.
useStudioAgentTools.ts:81-84states the "any fallback must be awaited inside this effect before registration and then re-read here" contract;useStudioAgentTools.ts:96-102still readsgetModelContext()once at mount and returns silently on null. #3514's polyfill has to satisfy the invariant at the bootstrap layer; a chunk-load failure that leavesmodelContextnull is silent (log("skipped")only). One deferred gap: no telemetry for the "polyfill expected, absent" case — distinct from the newly-instrumented registration failure. Acceptable to defer alongside the #3514 handoff. - LOW
schemaVersion— CLOSED with explicit refusal.lookTools.ts:74-78documents the WebMCP session-discovery model as the reason a version field would not defend the actual boundary. Defensible.
Adjacent-boundary items — delta
- Registration-failure telemetry — CLOSED.
useStudioAgentTools.ts:18-27emitswebmcp_registration_failedper failed tool with{ error_name, tool_name }. Both fields are bounded (error_nameis the DOMException name set byregistrar.ts:75-82;tool_nameis the internal enum). No message/URL bleed. Test atuseStudioAgentTools.test.tsx:184-196proves theNotAllowedErrorpath end-to-end via a realDOMExceptionrejection. - HMR unregister/reregister window — ACKNOWLEDGED, not guarded.
useStudioAgentTools.ts:83-84names the "development-only" window. Noimport.meta.hot?.disposecleanup. Dev-only, StrictMode path is defended byregistrar.ts:71-72; leaving HMR uninstrumented is a reasonable tradeoff. describeSelection.gsapAnimationsreading.length ?? 0— CLOSED.describeSelectionatlookTools.ts:114now takesanimationCountas a parameter;lookTools.ts:166sources it fromsnapshot.selectionAnimationCount;StudioAgentTools.tsx:34populates it fromuseDomEditSelectionContext().selectedGsapAnimations.length, which is the live channel exposed byDomEditContext.tsx:87. Test atlookTools.test.ts:186-191asserts the propagation with a realselection()shape.
Adversarial: new fix boundary (6 axes)
- Reader-side of new
selectionAnimationCountfield:StudioLookSnapshotinterface atlookTools.ts:23marks it required (non-optional). SolebuildStudioLookproduction caller isStudioAgentTools.tsx:26-45and populates it. No other consumer inpackages/studio/src/webmcp/**at HEAD. - Cardinality of the new event: bounded —
error_namevalues are constrained toAbortError/InvalidStateError/SecurityError/NotAllowedError/Error, andtool_nameis the internal enum.getBrowserSystemMeta()adds standard high-cardinality UA fields consistent with all other Studio events. - Non-Abort-but-benign classes:
registrar.ts:87classifies non-DOMExceptionthrows as"Error", so a synchronous JS bug in a future tool body would be tracked aserror_name: "Error". Fine. - Documentation-as-fix test discipline: both new tests assert real behavior (mock rejection →
trackEventcall; snapshot field →look.selection.animationCount). No presence-of-comment assertions.
Test discipline — 22 focused tests, 2 new (telemetry, animationCount). Both non-vacuous.
CI — 11 required checks green at 42a84c11c; Preview parity in-progress (non-blocking). mergeable=MERGEABLE.
Verdict
R2 findings closed or documented as intentional. No new defects at the R3 fix boundary. Ready for approval by another reviewer.
— Via
|
@vanceingalls @james-russo-rames-d-jusso The remaining R2 items are addressed at 42a84c1, including Via's mount-once preference and fallback-ordering findings, and the full Studio suite passes. This is a request for the formal APPROVED review, not another analysis pass. Via, 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 42a84c11cd4207a7ffa5b464db2dd8b0512e0fb2.
Read the read-boundary myself. No defects found — this is careful work, and a few things in it are the kind most people miss:
- Truncation is honest.
elementCountismatched.length, the full match count, whileelementsis the sliced page (lookTools.ts:81-82). An agent can always tell a truncated list from a complete one, which is the same class of problem #3510 exists to fix, handled before anyone asked. - Bounds are enforced twice.
MAX_FILTER_LENGTH = 128at parse time viaschema.maxLengthand again at execution via.slice(0, MAX_FILTER_LENGTH)(:147,:180). Thelimitschema advertisesmaximum: DEFAULT_LIMIT, and the clamp isMath.min(requested, DEFAULT_LIMIT)— advertised and enforced agree, so there's no silent gap where an agent asks for more than it can get.additionalProperties: false. handles.tsgets two subtle things right.asHtmlElementchecksinstanceofagainst the owning document's realm (:125-128) — the preview is an iframe, so the naive check fails on every real element. AndcssEscape(:117) closes the attribute-selector break-out with a fallback for engines missingCSS.escape.querySelectorAllis try/caught for selectors minted in a prior document.
One forward-looking note for the write PRs behind this — not actionable here, and not a security issue at this boundary:
handles.ts:12 states "The agent never builds one; it passes back what a read gave it." That's true by convention, not by construction. The sel: scheme resolves an arbitrary agent-supplied CSS selector straight through doc.querySelectorAll(parsed.value) (:106), so a handle can be forged. While this tool is read-only that costs nothing — same document, same project, same user, and the agent already receives the element list. Once write tools consume handles, "unforgeable" stops being a safe assumption to design against, and the sel: path becomes a write primitive against any element in the preview including ones a filter/limit deliberately withheld. Cheap to decide now, expensive to retrofit at PR 7 of 10.
A correction on the stale-approval mechanics, because it changes what you'll see: hyperframes does not dismiss approvals on push. I read ruleset 14211637 live just now — dismiss_stale_reviews_on_push: false, require_last_push_approval: true, required_approving_review_count: 1. So when you restack this, my approval stays visibly APPROVED; what changes is that it stops satisfying the gate, and reviewDecision flips back to REVIEW_REQUIRED. Same practical outcome — I re-stamp — but don't go looking for a dismissal event, because none will appear.
— Rames
The base branch was changed.
jrusso1020
left a comment
There was a problem hiding this comment.
APPROVED at 42a84c11cd4207a7ffa5b464db2dd8b0512e0fb2 — re-stamp after a base-change dismissal.
The head has not moved since review 5036979165, so this is the same code I read then and the verdict is unchanged: no defects, and the elementCount-vs-truncation handling plus the cross-realm instanceof in handles.ts:125-128 remain better than they needed to be. My forward-looking note about the sel: handle scheme being unforgeable-by-convention rather than by-construction still stands for the write PRs.
What actually dismissed it, since the head never moved: the timeline records base_ref_changed and review_dismissed at the same second (03:35:31Z) with dismissal_message: "The base branch was changed." This PR's base moved from fix/domedit-commit-reporting to main.
That retarget makes this PR cumulative, which is worth knowing before merge. Against main this now diffs 34 files / +1871, not the 13 files of its own WebMCP work — it carries #3510's domEditCommitRunner.ts, useElementLifecycleOps.ts, useDomEditTextCommits.ts and the propertyPanel files too. Two consequences:
- Merge ascending (#3510 → #3511 → #3514). Merging a later PR first lands its ancestors' content along with it.
- This head predates the annotation fix.
Promise<DomEditCommitOutcome>appears 0 times in this PR's copies of those two files, against 3 times at #3510's69c4402f. Merged ascending that is harmless — the three-way merge keeps main's newer version, since this branch hasn't touched those files since the divergence point. Merged out of order, main briefly gets the un-annotated copies until #3510 lands.
Good news on the re-stamp treadmill: now that all three sit on main, no further base-change dismissals should fire as they land.
— Rames
Registers a `studio_look` tool on `document.modelContext`, so an agent in a browser that supports it can read what Studio knows: the open project and composition, the playhead, the human's current selection with its capabilities, and the timeline's elements with a handle for each. The API is `document.modelContext`, not `navigator.modelContext`. The latter is a polyfill compatibility shim rather than a spec member, so feature detecting it is wrong even where a published sample appears to work. Three decisions worth knowing: Registration happens ONCE per mount, with the dependencies held in a ref that every render refreshes. Depending on the handlers instead re-runs on nearly every interaction, because the DomEdit actions object changes identity with the selection and the element list. Each re-run aborts the registration signal and unregisters everything, and the spec warns that a quick unregister-then- reregister can apply an old call's arguments against the new schema. The test for this is the important one in the unit; breaking the empty dependency array fails it and nothing else. Tools resolve with a tagged result, they never reject. That is forced by the spec: a rejected `execute` has its reason discarded and the caller sees a bare UnknownError, so rejecting would guarantee the agent cannot learn why an edit failed. Elements are addressed by a minted handle, not by `TimelineElement.id`. That id is a synthesised identity, so `getElementById` misses most elements; the handle carries `data-hf-id`, else the DOM id, else a selector plus occurrence. Mounted from `EditorShell` rather than `App`, because the DomEdit contexts are only readable below `DomEditProvider` and `App.tsx` is three lines under the 600-line cap. The undo signal is reported as the shell actually exposes it, `canUndo` and a label, rather than as a revision counter. The depth lives in component-local state and is not reachable without plumbing it through the shell context, so the field says what it is instead of implying precision it does not have. Writes are not in this change. `canWrite` is optimistic and the comment says so; the write tools need a real guard against the paused-save and external- conflict states, which are not on any context this component can reach yet.
42a84c1 to
3d3db47
Compare
What
Studio registers a
studio_looktool ondocument.modelContext, so an agent running in a browser that supports WebMCP can read what Studio knows: the open project and composition, the playhead, the human's current selection with its capabilities, and the timeline's elements with a handle for each.Read only. No write tools in this PR.
Stacked on #3510.
Why
An agent working on a composition today has two bad options. Editing the HTML blind means guessing coordinates from source, with no view of resolved geometry, the playhead, or which element the human is looking at. Driving the UI with synthetic events does not work at all:
packages/studio/AGENTS.mdrecords that the canvas overlay takes pointer capture and recognises a double press itself, so synthesised click pairs never open a text edit.The information already exists in Studio's running state. It just was not reachable from outside the page. The current handoff is
AskAgentModal, which copies a prompt to the clipboard and ends there.How
The API is
document.modelContext, notnavigator.modelContext. The latter is a polyfill compatibility shim rather than a spec member, so feature-detecting it is wrong even where a published sample appears to work. Local typings mirror the WebIDL and live in one file alongside the registrar, so a spec change (this is an Origin Trial) is a two-file edit.Registration happens once per mount, with the dependencies in a ref that every render refreshes. Depending on the handlers instead re-runs on nearly every interaction, because the DomEdit actions object changes identity with the selection and the element list. Each re-run aborts the registration signal and unregisters everything,
toolchangefires constantly, and the spec warns that a quick unregister-then-reregister can apply an old call's arguments against the new schema.Tools resolve with a tagged result, they never reject. That is forced by the spec, not a style choice: a rejected
executehas its reason discarded and the caller sees a bareUnknownError, so rejecting would guarantee the agent cannot learn why something failed. There is nooutputSchemain the platform yet, so the discriminant is stated in the tool's description prose.Elements are addressed by a minted handle, not
TimelineElement.id. That id is a synthesised identity, sogetElementByIdmisses most elements. The handle carriesdata-hf-id, else the DOM id, else a selector plus occurrence index, matching how Studio's own patcher addresses elements.Mounted from
EditorShellrather thanApp, because the DomEdit contexts are only readable belowDomEditProviderandApp.tsxsits three lines under the 600-line cap.The undo signal is reported as the shell actually exposes it,
canUndoplus a label, rather than as a revision counter. The depth lives in component-local state and is not reachable without plumbing it through the shell context, so the field says what it is instead of implying precision it does not have.Off by default is a
agentToolsEnabledfield on the existingStudioUiPreferences, not a new storage key. The browser still gates every actual invocation behind its own permission prompt.Test plan
43 new tests across four files.
instanceof HTMLElementfails because the preview is an iframe. Unaddressable, out-of-range and malformed handles each return null rather than the wrong element.AbortError, which is teardown working and must not surface as a failure), duplicate names caught before the browser rejects them, and theDOMExceptionname preserved because it is the only thing that tellsSecurityErrorfromNotAllowedErrorfromInvalidStateError.studio_look: handles, filtering, truncation that keeps the true match count, capability pass-through includingreasonIfDisabled.Full package suite 4528 passing across 407 files, run solo twice.
bunx tsc --noEmitclean.bunx fallow audit --fail-on-issuesclean; it caught four speculative exports and one over-threshold function, both fixed rather than suppressed.Not verified in a real browser yet. happy-dom and jsdom are not browsers, so these tests prove the wrappers do what they say, not that Chrome registers the tool. The end-to-end capture needs
chrome://flags/#enable-webmcp-testingand lands with the write tools.canWriteis optimistic in this PR and the code comment says so. The paused-save and external-conflict states are not on any context this component can reach yet, and the write tools must not ship trusting that field.