From d003d3254f06ae756e2253afc169e8d9ec095ea8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 30 Aug 2026 14:00:24 +0000 Subject: [PATCH] fix(mcp): scope the bridge's `openWorldHint: false` to platform-registered names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `registerToolFromDefinition` asserted `openWorldHint: false` in every bridged tool's annotations, from no declared source — over every tool an app registers under its own name as well as the platform's. `AIToolDefinition` has no member expressing the hint, so the `false` was a property of this file served to every MCP client as a property of the tool: an app tool reaching a weather API, an LLM or any outbound service was announced as closed-world. The hint is now derived in `worldAnnotation()` from `PLATFORM_PROVIDED_TOOL_NAMES` (`@objectstack/spec/system`) — the same registry the bridge's `readOnlyHint` name fallback is already pinned to as a subset, so the two hints read one registry between them. Platform names keep the known-correct `false`; every other bridged tool is served no `openWorldHint` key at all. The asymmetry with the safety hints is deliberate and written down at the derivation site: SDK 1.30.0 documents `openWorldHint` as `Default: true`, so omission here is the honest direction rather than the conservative one, unlike `readOnlyHint` (`Default: false`) and `destructiveHint` (`Default: true`). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01TvqBFLRzXdSPcbusDoED9k --- .../mcp-openworldhint-platform-scope.md | 44 ++++++ packages/mcp/src/mcp-server-runtime.ts | 106 +++++++++++++- ...mcp-tool-bridge-safety-annotations.test.ts | 135 +++++++++++++++++- 3 files changed, 277 insertions(+), 8 deletions(-) create mode 100644 .changeset/mcp-openworldhint-platform-scope.md diff --git a/.changeset/mcp-openworldhint-platform-scope.md b/.changeset/mcp-openworldhint-platform-scope.md new file mode 100644 index 0000000000..cc16a08e4c --- /dev/null +++ b/.changeset/mcp-openworldhint-platform-scope.md @@ -0,0 +1,44 @@ +--- +"@objectstack/mcp": patch +--- + +fix(mcp): assert `openWorldHint: false` only for platform-registered tool names (#13350) + +The MCP tool bridge (`registerToolFromDefinition` in `mcp-server-runtime.ts`) +put `openWorldHint: false` in every bridged tool's `annotations` — for every +tool an app registers under its own name as well as the platform's own. No +source existed for that claim: `AIToolDefinition` has no member expressing it, +so the `false` was a property of the bridge file presented to every MCP client +as a property of the tool. An app tool that calls a weather API, an LLM or any +other outbound service was announced as having a closed, well-defined domain of +interaction. Same defect class as the `readOnlyHint` / `destructiveHint` repair +that preceded it. + +The hint is now derived from `PLATFORM_PROVIDED_TOOL_NAMES` +(`@objectstack/spec/system`) — the canonical registry of the statically named +tools the cloud AI runtime registers, and the same registry the bridge's +existing `readOnlyHint` name fallback is pinned to as a subset. A platform name +keeps `openWorldHint: false`, which is known-correct: those tools act on this +stack's own records and metadata. Every other bridged tool is served **no +`openWorldHint` key at all**. + +⚠️ **What omission means here, and why it differs from the sibling hints.** +`@modelcontextprotocol/sdk` 1.30.0 documents `openWorldHint` as `Default: true` +(`ToolAnnotationsSchema`), so an app tool that declares nothing is now read by +a conforming host as reaching an **open** world — where before it was told the +world was closed. That is the intended direction: the bridge has no source for +an app tool, and the protocol's own default is a better answer than a +fabricated one. It is the opposite of the `readOnlyHint` (`Default: false`) and +`destructiveHint` (`Default: true`) cases, where omission lands on the cautious +reading; the asymmetry is documented at the derivation site so it is not +"tidied" back into the defect. + +Hosts that keyed behaviour off a bridged app tool's `openWorldHint: false` will +now see the annotation absent. The platform tools' `false` is unchanged, and +the object-CRUD and action bridges in `mcp-http-tools.ts` — including +`run_action`'s deliberate `openWorldHint: true` — are untouched. + +Making the hint a property of the tool (a declared member on +`AIToolDefinition`, with action-backed tools inheriting `run_action`'s +`openWorldHint: true`) is a public contract extension and was ruled a +follow-up; this is the zero-contract-change half. diff --git a/packages/mcp/src/mcp-server-runtime.ts b/packages/mcp/src/mcp-server-runtime.ts index fe7722fd5e..58e73492bb 100644 --- a/packages/mcp/src/mcp-server-runtime.ts +++ b/packages/mcp/src/mcp-server-runtime.ts @@ -5,6 +5,7 @@ import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js' import { WebStandardStreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/webStandardStreamableHttp.js'; import type { Logger, IMetadataService, AIToolDefinition } from '@objectstack/spec/contracts'; import type { Agent } from '@objectstack/spec/ai'; +import { PLATFORM_PROVIDED_TOOL_NAMES } from '@objectstack/spec/system'; import type { ToolRegistry, ToolExecutionResult } from './types.js'; import { wireBridgeTools } from './mcp-http-tools.js'; import type { @@ -184,6 +185,96 @@ function safetyAnnotations(tool: AIToolDefinition): ToolSafetyHints { return {}; } +/** The world-domain hint this bridge can source. */ +interface ToolWorldHint { + openWorldHint?: boolean; +} + +/** + * The `openWorldHint` this bridge can actually SOURCE — for the names the + * PLATFORM registers, and for nobody else. + * + * THE DEFECT. This hint used to be a bare `openWorldHint: false` sitting in + * the annotations literal below, asserted over EVERY bridged tool from + * nothing at all — every tool an app registers under its own name included. + * `AIToolDefinition` has no member expressing it, so that `false` was never a + * property of the tool; it was a property of this file. An app can register a + * tool that calls a weather API, an LLM, or any outbound service, and this + * bridge told every MCP client its domain of interaction was closed. Same + * class as the `readOnlyHint` / `destructiveHint` defect described above. + * + * THE SOURCE is {@link PLATFORM_PROVIDED_TOOL_NAMES} + * (`@objectstack/spec/system`) — the canonical registry of the statically + * named tools the cloud AI runtime registers (`PLATFORM_TOOLS_BY_PACKAGE`). + * Every name in it acts on the ObjectStack environment itself, this stack's + * records and its own metadata, which is a closed and well-defined domain in + * exactly the sense the SDK gives the word. So `false` for those names is + * known rather than assumed, and it is the information option 3 (drop the + * hint outright) would have thrown away. + * + * WHY THE REGISTRY AND NOT THE TWO NAME SETS ABOVE. Same shape — a membership + * test against platform-registered names — but a different question. + * {@link PLATFORM_READ_ONLY_TOOL_NAMES} and + * {@link PLATFORM_DESTRUCTIVE_TOOL_NAMES} answer "what SAFETY class does the + * platform know for this name", and they are deliberately partial: they carry + * only the platform names whose safety class this bridge knows. The question + * here is OWNERSHIP, and the registry is what answers it. Keying the world + * hint off the safety lists instead would drop `create_object`, `add_field`, + * `list_metadata`, `describe_metadata` and twenty more — platform tools whose + * world is just as closed — to the protocol default, reintroducing option 3's + * accuracy loss under a narrower name. A sibling pin already holds the two + * safety lists to this same registry as a SUBSET, so the hints read one + * registry between them rather than three hand lists that can drift apart. + * + * ⚠️ WHY OMISSION IS NOT THE CONSERVATIVE DIRECTION HERE — AND WHY THE + * ASYMMETRY WITH {@link safetyAnnotations} IS DELIBERATE, NOT AN OVERSIGHT + * WAITING TO BE TIDIED. Structurally the two functions are one rule: assert + * what the platform can source, omit what it cannot, because MCP has no + * spelling for "unknown" other than absence. What DIFFERS is the price of + * that absence, and the difference is the SDK's own. Measured in the pinned + * `@modelcontextprotocol/sdk` 1.30.0 (`ToolAnnotationsSchema`, `dist/esm/types.js`): + * + * ``` + * readOnlyHint Default: false ← omission reads "not read-only" + * destructiveHint Default: true ← omission reads "may be destructive" + * openWorldHint Default: true ← omission reads "OPEN world" + * ``` + * + * For the two safety hints, omission lands on the cautious answer and costs + * only information — which is why #13318 could move them to omit-when-unsourced + * and call it conservative. For this one, omission lands on the LESS cautious + * reading: a tool that sources nothing is understood by every conforming host + * to reach an open world. That trade is accepted on purpose. For an + * app-registered tool this bridge genuinely has no source, and falling to the + * protocol's documented default is honest where asserting `false` was a lie. + * ⛔ So do not "repair" the asymmetry by re-asserting `false` for everyone: + * that IS the defect. The identical STRUCTURE of the two functions is what + * keeps the one rule legible; identical consequences were never the point. + * + * ⛔ NOT the dynamic tool families. `PLATFORM_TOOL_FAMILY_PREFIXES` + * (`action_`) names tools the runtime materialises from an app's OWN + * declarative actions: the platform registers the wrapper, the app defines the + * behaviour, outbound calls included. `mcp-http-tools.ts` asserts + * `openWorldHint: true` for `run_action` on exactly that reasoning. Widening + * this membership test to the prefixes would re-import the defect under a new + * name. + * + * ⛔ NOT `{ openWorldHint: undefined }`. The no-source answer is an absent + * KEY, which is why this returns `{}`: an undefined-valued property is dropped + * by JSON serialization but survives a spread, so "omitted" has to be true of + * the object as well as of the wire. + * + * THE FOLLOW-UP THIS IS NOT. Making the hint a property of the TOOL — a + * declared member on `AIToolDefinition`, with an action-backed tool inheriting + * `run_action`'s `openWorldHint: true` — is the shape that would make it true + * rather than merely defensible. That is a public contract extension in + * `packages/spec/**` and was ruled a follow-up; this is the zero-contract-change + * half, which stops the unsourced assertion now. + */ +function worldAnnotation(tool: AIToolDefinition): ToolWorldHint { + return PLATFORM_PROVIDED_TOOL_NAMES.has(tool.name) ? { openWorldHint: false } : {}; +} + // ── AIToolDefinition.parameters → MCP inputSchema ──────────────────────────── /** @@ -992,8 +1083,11 @@ export class MCPServerRuntime { * The safety annotations come from {@link safetyAnnotations}, which reads * what the definition DECLARES and omits the hints it cannot source; the * name-derived `readOnlyHint: false, destructiveHint: false` this call used - * to assert over every unlisted tool is gone. `openWorldHint` is untouched - * by that change and still asserted for every bridged tool. + * to assert over every unlisted tool is gone. `openWorldHint` now goes + * through {@link worldAnnotation} on the same principle — asserted `false` + * for platform-registered names, omitted for everyone else — but read that + * function before assuming the two omissions cost the same thing: the SDK + * defaults them in opposite directions. */ private registerToolFromDefinition(tool: AIToolDefinition, toolRegistry: ToolRegistry): void { const logger = this.config.logger; @@ -1004,10 +1098,12 @@ export class MCPServerRuntime { description: tool.description, inputSchema: toolInputSchema(tool, logger), annotations: { - // Only the hints {@link safetyAnnotations} can source — a tool that - // declares nothing is served neither, so the MCP defaults apply. + // Only the hints these two can source — a tool that declares nothing + // and is not a platform name is served neither set, so the MCP + // defaults apply. Those defaults are NOT symmetrical between them; + // {@link worldAnnotation} is where that is written down. ...safetyAnnotations(tool), - openWorldHint: false, + ...worldAnnotation(tool), }, }, async (args) => { diff --git a/packages/mcp/src/mcp-tool-bridge-safety-annotations.test.ts b/packages/mcp/src/mcp-tool-bridge-safety-annotations.test.ts index f1bceca9f9..d77e6ec909 100644 --- a/packages/mcp/src/mcp-tool-bridge-safety-annotations.test.ts +++ b/packages/mcp/src/mcp-tool-bridge-safety-annotations.test.ts @@ -36,6 +36,26 @@ * omission, and the SDK's own `ToolAnnotationsSchema` documents the defaults * that then apply (`readOnlyHint` false, `destructiveHint` **true**), which is * the conservative reading the old `false` inverted. + * + * THE SECOND DEFECT, PINNED HERE TOO. `openWorldHint: false` was asserted for + * every bridged tool from no source at all — the sibling of the above, one + * hint later. It is now sourced the same way the `readOnlyHint` fallback is, + * from platform-registered names (`PLATFORM_PROVIDED_TOOL_NAMES`), and + * omitted for everyone else. + * + * ⚠️ AND ITS OMISSION IS PINNED DIFFERENTLY, ON PURPOSE. The same SDK schema + * documents `openWorldHint` as **`Default: true`**, so an omitted world hint + * is read as an OPEN world — the one place in this file where absence is not + * the cautious answer, only the honest one. A future reader tempted to make + * the three hints behave alike has to make these pins red first, which is the + * point of stating it here as well as at the call site. + * + * ⚠️ ABSENCE MEANS AN ABSENT KEY, and these cases say so with + * `Object.hasOwn`, not with `toBeUndefined()`. A spread of + * `{ openWorldHint: undefined }` would satisfy `toBeUndefined()` while still + * putting the property on the object; every such case below carries a + * same-object positive control (a platform tool listed in the same call) so a + * `false` from {@link hasHint} is a reading rather than a typo'd key name. */ import { describe, it, expect, afterEach } from 'vitest'; @@ -155,6 +175,19 @@ async function annotationsOf( return { session, byName: Object.fromEntries(listed.map((t: any) => [t.name, t])) }; } +/** + * Own-property presence on the PARSED WIRE object. + * + * ⚠️ Not `toBeUndefined()`, which a spread of `{ openWorldHint: undefined }` + * would also satisfy while still putting the property on the object — the + * distinction these cases exist to make. ⛔ And not `Object`.`hasOwn` either: + * that is ES2022 and this repo compiles at `lib: ["ES2020"]`, so it type-errors + * where the runtime (Node 22) would have run it happily — a gap this package's + * own `typecheck` cannot report, because its tsconfig excludes test files. + */ +const hasHint = (annotations: Record | undefined, hint: string): boolean => + Object.prototype.hasOwnProperty.call(annotations ?? {}, hint); + const tool = (name: string, extra: Partial = {}): AIToolDefinition => ({ name, description: `the ${name} tool`, @@ -215,11 +248,13 @@ describe('bridgeTools — the safety annotations a client receives', () => { const s = await annotationsOf([tool('send_invoice_email')]); openSession = s.session; - const annotations = s.byName.send_invoice_email.annotations; + const annotations = s.byName.send_invoice_email.annotations ?? {}; expect(annotations.destructiveHint).toBeUndefined(); expect(annotations.readOnlyHint).toBeUndefined(); - // The hint the bridge does still assert for every tool, unchanged here. - expect(annotations.openWorldHint).toBe(false); + // ...and no world hint either. `send_invoice_email` is the case in the + // name: an app tool that reaches an outbound service was being told to + // every client as closed-world. + expect(hasHint(annotations, 'openWorldHint')).toBe(false); }); it('what the definition declares outranks what its name suggests', async () => { @@ -237,6 +272,9 @@ describe('bridgeTools — the safety annotations a client receives', () => { expect(PLATFORM_PROVIDED_TOOL_NAMES.has('aggregate_records')).toBe(false); expect(s.byName.aggregate_records.annotations.readOnlyHint).toBeUndefined(); expect(s.byName.aggregate_records.annotations.destructiveHint).toBeUndefined(); + // Nor a world hint from this bridge. It gets one at its OWN registration + // site in `mcp-http-tools.ts`, which is where that fact is known. + expect(hasHint(s.byName.aggregate_records.annotations, 'openWorldHint')).toBe(false); }); /** @@ -274,4 +312,95 @@ describe('bridgeTools — the safety annotations a client receives', () => { expect(s.byName[stranger.name].annotations.destructiveHint).toBeUndefined(); } }); + + // ── openWorldHint ──────────────────────────────────────────────────────── + + it('CONTROL: a platform-registered name still receives `openWorldHint: false`', async () => { + const s = await annotationsOf([ + tool('query_records'), + tool('list_objects'), + // Platform names OUTSIDE the two safety-class sets. These are the tools + // a fallback keyed on those sets instead of the registry would have + // silently flipped to the protocol's open-world default. + tool('create_object'), + tool('list_metadata'), + tool('describe_metadata'), + ]); + openSession = s.session; + + for (const name of ['query_records', 'list_objects', 'create_object', 'list_metadata', 'describe_metadata']) { + expect(PLATFORM_PROVIDED_TOOL_NAMES.has(name)).toBe(true); + expect(s.byName[name].annotations.openWorldHint).toBe(false); + } + // The safety hints are sourced separately: `create_object` is a platform + // name in NEITHER safety set, so it keeps the world hint and no other. + expect(s.byName.create_object.annotations.readOnlyHint).toBeUndefined(); + expect(s.byName.create_object.annotations.destructiveHint).toBeUndefined(); + }); + + it('an app-registered tool receives NO `openWorldHint` KEY — absence, not `undefined`', async () => { + const s = await annotationsOf([ + tool('check_weather'), + tool('ask_llm', { requiresConfirmation: false }), + tool('delete_opportunity', { requiresConfirmation: true }), + // Positive control in the same `tools/list` answer: `hasOwn` must be + // able to say `true` about this exact wire object, or the `false`s + // below would be indistinguishable from a misspelled key. + tool('query_records'), + ]); + openSession = s.session; + + expect(hasHint(s.byName.query_records.annotations, 'openWorldHint')).toBe(true); + + for (const name of ['check_weather', 'ask_llm', 'delete_opportunity']) { + const annotations = s.byName[name].annotations ?? {}; + expect(hasHint(annotations, 'openWorldHint')).toBe(false); + expect(annotations.openWorldHint).toBeUndefined(); + } + + // Independently sourced: declaring `requiresConfirmation` buys the SAFETY + // hints and buys nothing about the world, which is the whole point of the + // two derivations being separate. + expect(s.byName.delete_opportunity.annotations.destructiveHint).toBe(true); + expect(s.byName.ask_llm.annotations.destructiveHint).toBe(false); + }); + + /** + * The world-hint counterpart of the fallback invariant above, driven across + * the whole registry at once: `openWorldHint: false` is a claim the platform + * can source about the tools it registers, and about nothing else. + */ + it('exactly the platform-registered names carry `openWorldHint`, and every one of them carries `false`', async () => { + const platform = [...PLATFORM_PROVIDED_TOOL_NAMES].map((name) => tool(name)); + const strangers = [ + 'aggregate_records', + 'action_close_deal', + 'check_weather', + 'send_invoice_email', + 'void_invoice', + ].map((name) => tool(name)); + + const s = await annotationsOf([...platform, ...strangers]); + openSession = s.session; + + const withWorldHint = Object.values(s.byName) + .filter((t: any) => hasHint(t.annotations, 'openWorldHint')) + .map((t: any) => t.name) + .sort(); + + // ⚠️ Non-vacuity first: `toEqual` between two empty arrays passes, so an + // unbuilt or empty registry would make every assertion below say nothing. + expect(PLATFORM_PROVIDED_TOOL_NAMES.size).toBeGreaterThan(0); + expect(withWorldHint.length).toBe(PLATFORM_PROVIDED_TOOL_NAMES.size); + expect(withWorldHint).toEqual([...PLATFORM_PROVIDED_TOOL_NAMES].sort()); + for (const name of withWorldHint) { + expect(s.byName[name].annotations.openWorldHint).toBe(false); + } + + // `action_close_deal` is the family case stated explicitly: the runtime + // materialises `action_` wrappers around an app's OWN actions, so a + // membership test widened to `PLATFORM_TOOL_FAMILY_PREFIXES` would put + // this bridge back to claiming a closed world over app-defined behaviour. + expect(hasHint(s.byName.action_close_deal.annotations, 'openWorldHint')).toBe(false); + }); });