Repository navigation
Conversation
TueJon
left a comment
There was a problem hiding this comment.
Review — #26 @ afd3bd9 (round 1, first review)
Docs-only PR (+56/−0 across integrate.md, security.md, verify.md). No runtime, template, manifest, or workflow changes. CI check green at this head; ai-review is this task.
Product fit
In scope and warranted. Each addition guards a real failure mode of the skill's own workflow: duplicate tool ownership when a framework adapter already exposes a form action, adapter registration flags / fulfilled calls masking failures during VERIFY, and cross-origin tool selection confusing discovery with authorization. All four fit the skill's purpose (correct inventory → integrate → verify). No new product surface, no dependency, no default behavior change.
Correctness — claims checked against the repo
- Angular Signal Forms ownership (
integrate.md):https://angular.dev/ai/webmcpresolves (200). Guidance is "inventory before annotating, never add a second owner" — consistent with the skill's baseline-aware ground rule and the existing duplicate-name rollback behavior inheal.md/runtime.md. - Adapter observations (
integrate.md): explicitly dated and versioned (2026-11-02) local observations, placed directly above the existing "not tested framework-version certifications" disclaimer — correctly hedged, no certification implied. Contrast with the vendored runtime is accurate:runtime.mdconfirmscreateToolScopedisposal is AbortController registration teardown, and the runtime forwards execution options unchanged and converts bare null/undefined to structured errors, so the "adapter flag is not native evidence / preserve explicit structured results" advice matches shipped runtime semantics. - Native Playwright
page.webmcp(verify.md):https://playwright.dev/docs/api/class-webmcpresolves (200), so the API exists as cited. TheisError-without-throwing warning is consistent with the repo's own fail-closed result conventions, and the paragraph keeps the pinned standalone-harness default unchanged with an explicit do-not-upgrade-toolchain guard. No conflict with the existing Puppeteer paragraph. - Cross-origin gates (
security.md): three-gate description (embedder delegation, providerexposedTo, consumerfromOrigins) matches the repo's ownwebmcp.d.ts(exposedToon register,fromOriginsongetTools,RegisteredToolcarryingorigin+window), and the cited CG anchor section exists (spec page 200). "Discovery does not authorize execution" restates the skill's existing server-as-trust-boundary rule.
What I checked and found clean
- No secrets, no code, no schema/manifest changes;
package.json/skill.json/gemini-extension.jsonall remain 0.7.1 andCHANGELOG.mduntouched — correct for a docs-only change under the repo's version-parity rule. - No frontmatter touched (the known
argument-hint/tagsvalidator rejection noted in the PR body is pre-existing and out of scope; base is equally affected, so not this PR's blocker per the scope-boundary test). - No dead code introduced; no docs invalidated (no env vars, runbooks, or examples changed); no
release-steps/trigger (docs-only, no rollout step). - No external effects: nothing sends, publishes, provisions, or mutates customer systems; no deploy blast radius (no workflow/runner changes).
Findings
None. No blocking, no caveats. Two untested areas (full router navigation, Angular hydration/declarative duplicate registration) are disclosed in the PR body itself, not hidden.
Verdict
ship_it — mergeable as-is.
TueJon
left a comment
There was a problem hiding this comment.
Review — #26 @ 7e3a551 (round 2, follow-up)
Round 1 (afd3bd9) was docs-only and ship_it, with no open checklist. This round reviews only the afd3bd9..7e3a551 delta (code fix a4d8cd5 + docs alignment 7e3a551); no new checklist items were open, so only regressions introduced by these commits could block.
What the delta does
webmcp-workbench.js:registerToolnow JSON-snapshotsinputSchema/outputSchemaat registration (throwsTypeErrorbeforeset/notifyon non-serializable input, leaving the name reusable), andgetToolsreturns freshJSON.parsecopies per enumeration. Output-aware status comparison, display, and refresh fingerprint.webmcp.d.ts/webmcp.spec.ts: optionaloutputSchemaonModelContextTool/RegisteredTool; comments explicitly deny automatic output validation and note browser-dependent scalar serialization.- Tests: snapshot isolation, pre-registration rejection with name reuse, omission/
toJSONhandling, wrapper metadata passthrough, and a read-only Playwright refresh regression.tsconfig.templates.jsoncovers the new compile-only fixture. - Docs:
client.mddrops the stale model/workspace matrix for generic eligibility checks;native-compatibility.mdadds dated 2026-11-09 Chrome 156 scalar observations explicitly marked non-WPT;verify.md/reverify.mdadd output-drift comparison rules without permitting retries of application tools.
Checked and clean
- Snapshot semantics correct: mutation-after-register and mutation-after-enumerate both verified by tests; rejection path performs no
setand nonotify(events assertion in test).function-typed schemas fall through to the not-serializableTypeError— safe. optionalSchemapreserves omission (undefinedstaysundefined, never coerced to empty schema);null/non-object inputs still reject at registration, consistent with the existingparseSchemadisplay behavior for inputs.- No version/manifest changes, correct: prior runtime fixes also shipped without same-PR bumps (releases cut versions separately).
- Exact-head CI:
checkSUCCESS at this head;ai-reviewis this task. - Links resolve (200):
learn.chatgpt.com/docs/webmcp,webmcpPR #254. New doc claims are dated, hedged, and keep the standalone-harness default unchanged. - No secrets, no external effects, no deploy/workflow changes, no
release-steps/trigger. Scope-boundary test: residualannotationsobject sharing in the simulation predates this PR (base equally affected) — not introduced here.
Notes (non-blocking, no action required)
CHANGELOG.md [Unreleased]has no entry for the simulation behavior change (precedent471dfcalogged its fix in-PR). Release notes may consolidate this at release time.- Cited approvals
d-20261102-0740/d-20261109-0644for the reference edits were not re-verified through the task API from this session; taken as stated in the PR body.
Verdict
ship_it — no blocking findings, no regressions from the fix commits.
Workbench schema metadata currently shares nested objects with callers, so changing a supplied or enumerated schema silently changes later evidence. Optional outputSchema is also absent from types and contract refresh comparisons.
The simulation now snapshots both schemas through JSON, rejects serialization failures before registration, and returns fresh copies. Types and Workbench comparison/display/refresh support optional outputSchema, with compile, unit and read-only browser regressions. The existing TS/JS runtime wrappers already forward metadata and remain unchanged.
The reference updates cover existing framework tool ownership, optional native Playwright verification, cross-origin identity, current Site tools form/frame discovery, and output-only contract invalidation. They keep draft serialization distinct from measured Chrome scalar behavior. Runtime dependencies, defaults, distribution versions and mutation-journal behavior do not change.
Validation: npm run check passes all type/syntax checks and 83/83 tests; node tests/schema-browser.mjs proves output-only drift updates the observed contract UI. The local headed Chrome 156.0.8078.47 proof recorded ten read-only scalar cases, schema snapshot/error behavior and target-document policy denial. Native outputSchema propagation is unsupported in that browser; the about:blank document-replacement probe timed out and is not a pass. No WPT, account-availability or universal framework certification claim.
Source-only PR; no merge, release or deployment. Companion site-copy PR updates the public summaries. Approvals d-20261102-0740 and d-20261109-0644 cover the scoped reference edits.