fix(sdk): advertise intent context as optional on widget-invoked tools (0.3.2) - #72
Conversation
…s (0.3.2) Enabling intent capture advertises a REQUIRED `context` parameter on every tool that lacks one — including tools an app's widget iframe invokes itself. The widget sends only the tool's real arguments; it cannot know to send `context`. Hosts validate widget calls against the advertised schema, so they refuse every such call — and only once the host refreshes its cached schemas, i.e. the widget breaks silently some time AFTER intent capture is enabled, with no server-side trace because the calls never arrive. Observed in production (billiger-mietwagen, 2026-08-07): the widget's 3s auto-refresh went from 83 filter-offers calls on Aug 5 to zero the moment the ChatGPT connector re-fetched schemas; the live result count froze on the first search snapshot and the widget's detail view stopped working. Stale-schema sessions kept working in parallel, masking the cause. Detect widget-invoked tools in injectIntoListedTool — the listed entry already carries the tool's _meta (`openai/widgetAccessible` from the OpenAI Apps SDK, `ui.visibility` containing "app" from MCP Apps) — and skip the `required` push for them. Everything else is unchanged: `context` is still advertised, and capture and stripping still apply whenever a model call fills the optional field. This is deliberately automatic rather than a config option. A required `context` on a widget-invoked tool has no valid use — the failure is an invariant violation, not a preference — and the SDK already holds the information needed to decide. An option nobody knows to set would leave the trap armed for every customer who combines intent capture with a widget. Closes #71
Coverage Report for sdk
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
An adversarial review of the widget exemption found four confirmed coverage holes that each re-arm the exact incident it fixes, plus three operational gaps. All addressed here; each detection case carries a regression test. Detection (metaIndicatesWidget): - MCP Apps default visibility. An omitted `ui.visibility` defaults to ["model", "app"] per spec, so a tool with a `resourceUri` (nested or flat) and no visibility is app-callable — skybridge emits exactly this shape for every view-owning tool, and the old check missed it. Explicit visibility still wins in both directions: `ui.visibility: ["model"]` is the documented escape hatch that keeps required context on a view tool the widget never calls. - Flat-key shadowing. `isPlainObject(ui) ? ui.visibility : meta["ui/visibility"]` never consulted the flat key once a nested `ui` object existed; now the nested value falls through to the flat key with `??`. - Bare-string visibility. `ui.visibility: "app"` (off-spec authoring slip) failed the Array.isArray guard and reproduced the silent breakage; accepted leniently now, misreading it costs more than tolerating it. - Peer-range inertness. MCP SDK 1.12-1.17 accept `_meta` in registerTool but drop it before the registry and tools/list, so list-time detection alone is silently inert on every supported version below 1.18. The proxy's registerTool interceptor sees the config on all versions and now records widget-invokability at registration; the list handler falls back to that record when a listed entry carries no `_meta`. No peer-floor bump needed. Behavior around the exemption: - A pre-existing "context" entry in a customer's `required` array (legal JSON Schema without the property declared) is removed on widget tools instead of silently defeating the exemption. - The override is no longer invisible: one `[yavio]` log line per downgraded tool, and the startup message no longer claims required unconditionally. - Context-less calls on widget-invoked tools skip `config.fallback`: they are presumptively the widget's own machine traffic (3s auto-refresh), and inferring an intent per poll would record boilerplate at machine frequency. Docs: the dashboard-status troubleshooting section now names the widget exemption as an expected cause of missing intents, next to clients that do not fill unknown parameters. Tests: the two widget-call tests share one registration helper instead of a duplicated block, the widget capture path asserts the same input_values / input_keys leak guards as the model-tool test it mirrors, the control tool is asserted in every case, and an it.each covers all seven meta conventions. Two stub-server tests pin the registration-time classification (old-SDK list shape) and the required-array cleanup. 317/317 passing.
|
An adversarial review (10 findings, 7 confirmed) ran against this PR; aa40406 responds to all of them. Mapping:
317/317 tests, typecheck and lint clean. |
marselsel
left a comment
marselsel
left a comment
There was a problem hiding this comment.
Review from a parallel Claude Code session, at Marcel's request. The core fix is right — a required context on a widget-invoked tool does break every widget call once the host refreshes cached schemas, the failure is silent and delayed, and deciding it automatically rather than via config is the correct trade for that failure mode. The incident writeup in #71 is convincing.
Four concrete things worth fixing before merge, plus one design question, are left as inline comments.
Two findings I looked at and believe are NOT defects, recorded here so nobody re-litigates them:
-
"
injectIntoListedTooloverwrites the widget record, violating the invariant the other two writers enforce." It doesn't — all three writers gate on_meta !== undefined, including this one. The residual concern (a lossy proxy forwarding a partial_metathat drops the widget key) applies identically tonoteToolRegistrationandinstall(), so it's a shared, deliberate property — a present_metais trusted — not an inconsistency. A latertools/listcarrying complete metadata also restores the record, so it isn't permanent. -
"Not recognising
openai/outputTemplatecontradicts theresourceUrirule." Rule 3's own rationale is that MCP Apps defaults omitted visibility to["model", "app"]. OpenAI has no equivalent default:openai/widgetAccessible: trueis an explicit opt-in, and a widget cannot call a tool that hasn't opted in — so a requiredcontextis not the binding constraint for an OpenAI view-owning tool that omits it. Two specs, two different defaults; both rules can be correct simultaneously. Flagging rather than asserting, since you have live incident ground truth here and I don't — if you saw an OpenAI app whose widget calls a tool carrying onlyopenai/outputTemplate, that would settle it the other way.
Remaining items are maintainability judgement calls I'd leave to you: hoisting metaIndicatesWidget to module scope would make the detection matrix a table test instead of seven full client round-trips; loggedWidgetOverride is per-controller, so in the stateless per-request deployments install() explicitly supports the "once per tool" log guarantee doesn't hold; and RegisteredToolsHost could carry _meta?: unknown so install() doesn't need an inline cast on a private-API read.
Not touching the branch — it's checked out in the shared worktree and I didn't want to collide with you mid-flight.
| if (config.intent.enabled) { | ||
| console.info( | ||
| "[yavio] Intent capture enabled: tools advertise a required 'context' parameter (pass intent: false to disable).", | ||
| "[yavio] Intent capture enabled: tools advertise a 'context' parameter — required on model-facing tools, optional on widget-invoked ones (pass intent: false to disable).", |
There was a problem hiding this comment.
This log is gated only on config.intent.enabled, but required is independently configurable (intent: { required: false }, documented at 07-intent-capture.mdx). A server started that way advertises context as optional on every tool while this line claims it is "required on model-facing tools".
The previous wording was vaguer and therefore accidentally correct; making it specific made it wrong in that configuration. Suggest branching on config.intent.required, or dropping back to naming only the widget exemption.
| // Widget-invoked tools get `context` as optional regardless of config: | ||
| // capture still works when a model call fills it, while the widget's own | ||
| // context-less calls stay schema-valid. See metaIndicatesWidget. | ||
| if (config.required && !widget) { |
There was a problem hiding this comment.
The stale-required cleanup below (the copy.required.filter(k => k !== "context") in the if (widget) branch) only runs for widget tools, but the problem it solves isn't widget-specific.
With intent.required: false and a customer schema carrying a leftover required: ["search_id", "context"] — legal JSON Schema, and exactly the case that filter was written for — the non-widget path never strips it, so the advertised schema requires context on a tool whose operator explicitly configured it optional. That's the hard-validating-client breakage required: false exists to avoid.
Suggest running the filter whenever the SDK owns the context key (i.e. once optOut() hasn't fired), and letting the config.required && !widget branch be the only thing that adds it back.
| // refresh, a filter-bar click), and inferring an intent for each would | ||
| // record boilerplate "inferred" entries at machine frequency, | ||
| // drowning the real intents the fallback exists to approximate. | ||
| if (!captured && config.fallback && !isWidgetTool(toolName)) { |
There was a problem hiding this comment.
The fallback is suppressed per tool, not per call, so a model-initiated call to a widget-accessible tool that omits context now records nothing at all — not even source: "inferred".
That compounds with the other half of this PR: context also stops being required on these tools, so models omit it far more often. widgetAccessible doesn't remove a tool from the model's list, and MCP Apps' default ["model", "app"] is explicitly both — so on an app whose primary tool is widget-accessible, this is its most-called tool.
The rationale in the comment is sound (don't record boilerplate inferred entries at a widget's 3s-refresh frequency), but the discriminator exists on the request — widget calls arrive with the host's _meta and no conversation — so it could be applied per call and keep model-side coverage.
No live impact today: I checked all four instrumented Yavio apps and none configures intent.fallback, so nothing regresses now. Reasonable as a follow-up issue rather than a blocker — but worth a line in the docs, which currently say "Capture still works whenever a model call fills the optional field" without mentioning the fallback is off for these tools.
| * `ui.visibility: ["app"]`) `context` is advertised as OPTIONAL — the widget | ||
| * iframe calls those tools without it, and a required parameter would make | ||
| * the host refuse every such call once it refreshes cached schemas. Capture | ||
| * still applies when the value is present. See isWidgetInvoked. |
There was a problem hiding this comment.
isWidgetInvoked doesn't exist — grep -rn isWidgetInvoked packages/ returns this comment and nothing else. The predicate is metaIndicatesWidget and the lookup helper is isWidgetTool.
Trivial, but this is the top-of-module explainer for the subtlest logic in the SDK, so it's the first thing the next maintainer reads before touching it.
| // advertised as optional, while capture still applies when a model fills it. | ||
| beforeEach(() => _resetGlobalState()); | ||
|
|
||
| const WIDGET_METAS: Array<[string, Record<string, unknown>]> = [ |
There was a problem hiding this comment.
WIDGET_METAS has seven positive cases and the suite has one negative (ui.visibility: ["model"]), but nothing pins { "openai/widgetAccessible": false }.
That leaves the strict === true check at intent.ts:337 unguarded — it's the obvious line for a later cleanup to "simplify" into a truthiness check. Behaviour would survive that change (false is still falsy), but the deliberate strictness would be lost with no failing test to explain why it was there. Given every other detection branch got a dedicated case, this is the one gap in the matrix.
| - **Your handler never sees it.** Tool code and input schemas stay untouched — strict schemas included. | ||
| - **A call without `context` never fails.** The parameter is required only in the advertised schema; the server tolerates its absence. | ||
| - **Tools that define their own `context` parameter are left completely alone** — no injection, no capture, no stripping. | ||
| - **Widget-invoked tools get `context` as optional, automatically.** A tool marked as callable from an app widget (`_meta["openai/widgetAccessible"]: true`, or MCP Apps `ui.visibility` containing `"app"`) is invoked by the widget iframe with only its real arguments — an iframe cannot know to send `context`. Hosts validate widget calls against the advertised schema, so a required `context` would make them refuse every such call — and only once the host refreshes its cached schemas, i.e. the widget breaks silently some time *after* you enable intent capture. The SDK therefore never marks `context` as required on these tools. Capture still works whenever a model call fills the optional field. |
There was a problem hiding this comment.
This bullet documents two of the three detection rules but omits the broadest one and the only escape hatch.
Missing: a ui.resourceUri / ui/resourceUri with no visibility declared also downgrades the tool (intent.ts:346-347) — that's every view-owning tool of every MCP App, i.e. the widest-reaching rule here. Also missing: ui.visibility: ["model"], which both the code comment and the test call "the documented escape hatch" but which the docs never mention.
A customer whose intent coverage drops after upgrading currently has nothing here pointing at either the cause or the remedy — the same diagnostic dead end as the open Shipal coverage bug. Worth naming both, plus the [yavio] Intent context advertised as OPTIONAL… log line as the way to find affected tools.
- The startup log claimed "required on model-facing tools" unconditionally;
with intent: { required: false } that is false on every tool. The log now
branches on config.required.
- The stale-required cleanup (a leftover "context" in a customer required
array without the property) ran only in the widget branch, but the problem
is not widget-specific: under required: false an ordinary tool would still
advertise context as required against the operator's explicit
configuration. The strip now runs whenever the SDK owns the context key;
the policy branch is the only thing that adds it back. Stub test added.
- New negative test pins the strict `=== true` check for
openai/widgetAccessible: an explicit false must not downgrade the tool,
and a future "simplification" to truthiness now has a failing test.
- The module header referenced isWidgetInvoked, which no longer exists;
corrected to metaIndicatesWidget.
- The docs bullet now names all three detection rules — including the
broadest, resourceUri with omitted visibility (every view-owning tool of
every MCP App) — plus the ui.visibility: ["model"] escape hatch, the
per-tool downgrade log line, and the fact that the fallback is skipped
for context-less calls on these tools.
Per-call (rather than per-tool) fallback discrimination stays a follow-up:
no instrumented app configures a fallback today, and the request-side
discriminator it needs is tracked in #73.
|
All six review comments addressed in 726d6a6:
319/319 tests, typecheck and lint clean. |
1 participant
Closes #71 — full incident writeup and design discussion there.
What
injectIntoListedTool()now checks the listed tool's_meta(which the MCP SDK forwards into tools/list):openai/widgetAccessible: trueor MCP Appsui.visibilitycontaining"app"marks a widget-invoked tool, and for thosecontextis advertised without being added torequired. Capture and stripping are untouched — a model call that fills the optional field is still recorded.Why automatic instead of a config option
A required
contexton a widget-invoked tool has no valid use: the iframe sends only the tool's real arguments and cannot know to sendcontext, so the host refuses every widget call once it refreshes cached schemas. That makes this an invariant, not a preference — and the failure mode is silent, delayed, and invisible server-side (the calls never arrive), so an opt-out that customers must know about would leave the trap armed for exactly the people who need protection. The SDK already holds the information required to decide. A per-tool override can follow later if a genuine need appears.Tests
Four new integration tests (real
McpServer+InMemoryTransport+ real client): optional-contextadvertisement for both meta conventions with a same-server control asserting ordinary tools keep the required parameter, a context-less widget call that succeeds and records no intent, and a model call on the same tool whosecontextis still captured and stripped. Full suite: 309/309, typecheck + lint clean.Docs
docs/02-sdk/07-intent-capture.mdxgains a bullet under "How it works" explaining the exception and the silent-breakage mechanism.Downstream
Once released, the app-side workaround in billiger-mietwagen (
59cfcaf: own optionalcontextonfilter-offers/offer-details) can be reverted, restoring intent capture for model-sideoffer-detailscalls.🤖 Generated with Claude Code