Skip to content

Commit c39369d

Browse files
Trumpclaude
andauthored
fix(mcp): scope the bridge's openWorldHint: false to platform-registered names (#13485)
`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`). Claude-Session: https://claude.ai/code/session_01TvqBFLRzXdSPcbusDoED9k Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent a9ee989 commit c39369d

3 files changed

Lines changed: 277 additions & 8 deletions

File tree

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
---
2+
"@objectstack/mcp": patch
3+
---
4+
5+
fix(mcp): assert `openWorldHint: false` only for platform-registered tool names (#13350)
6+
7+
The MCP tool bridge (`registerToolFromDefinition` in `mcp-server-runtime.ts`)
8+
put `openWorldHint: false` in every bridged tool's `annotations` — for every
9+
tool an app registers under its own name as well as the platform's own. No
10+
source existed for that claim: `AIToolDefinition` has no member expressing it,
11+
so the `false` was a property of the bridge file presented to every MCP client
12+
as a property of the tool. An app tool that calls a weather API, an LLM or any
13+
other outbound service was announced as having a closed, well-defined domain of
14+
interaction. Same defect class as the `readOnlyHint` / `destructiveHint` repair
15+
that preceded it.
16+
17+
The hint is now derived from `PLATFORM_PROVIDED_TOOL_NAMES`
18+
(`@objectstack/spec/system`) — the canonical registry of the statically named
19+
tools the cloud AI runtime registers, and the same registry the bridge's
20+
existing `readOnlyHint` name fallback is pinned to as a subset. A platform name
21+
keeps `openWorldHint: false`, which is known-correct: those tools act on this
22+
stack's own records and metadata. Every other bridged tool is served **no
23+
`openWorldHint` key at all**.
24+
25+
⚠️ **What omission means here, and why it differs from the sibling hints.**
26+
`@modelcontextprotocol/sdk` 1.30.0 documents `openWorldHint` as `Default: true`
27+
(`ToolAnnotationsSchema`), so an app tool that declares nothing is now read by
28+
a conforming host as reaching an **open** world — where before it was told the
29+
world was closed. That is the intended direction: the bridge has no source for
30+
an app tool, and the protocol's own default is a better answer than a
31+
fabricated one. It is the opposite of the `readOnlyHint` (`Default: false`) and
32+
`destructiveHint` (`Default: true`) cases, where omission lands on the cautious
33+
reading; the asymmetry is documented at the derivation site so it is not
34+
"tidied" back into the defect.
35+
36+
Hosts that keyed behaviour off a bridged app tool's `openWorldHint: false` will
37+
now see the annotation absent. The platform tools' `false` is unchanged, and
38+
the object-CRUD and action bridges in `mcp-http-tools.ts` — including
39+
`run_action`'s deliberate `openWorldHint: true` — are untouched.
40+
41+
Making the hint a property of the tool (a declared member on
42+
`AIToolDefinition`, with action-backed tools inheriting `run_action`'s
43+
`openWorldHint: true`) is a public contract extension and was ruled a
44+
follow-up; this is the zero-contract-change half.

packages/mcp/src/mcp-server-runtime.ts

Lines changed: 101 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { StdioServerTransport } from '@modelcontextprotocol/sdk/server/stdio.js'
55
import { WebStandardStreamableHTTPServerTransport } from '@modelcontextprotocol/sdk/server/webStandardStreamableHttp.js';
66
import type { Logger, IMetadataService, AIToolDefinition } from '@objectstack/spec/contracts';
77
import type { Agent } from '@objectstack/spec/ai';
8+
import { PLATFORM_PROVIDED_TOOL_NAMES } from '@objectstack/spec/system';
89
import type { ToolRegistry, ToolExecutionResult } from './types.js';
910
import { wireBridgeTools } from './mcp-http-tools.js';
1011
import type {
@@ -184,6 +185,96 @@ function safetyAnnotations(tool: AIToolDefinition): ToolSafetyHints {
184185
return {};
185186
}
186187

188+
/** The world-domain hint this bridge can source. */
189+
interface ToolWorldHint {
190+
openWorldHint?: boolean;
191+
}
192+
193+
/**
194+
* The `openWorldHint` this bridge can actually SOURCE — for the names the
195+
* PLATFORM registers, and for nobody else.
196+
*
197+
* THE DEFECT. This hint used to be a bare `openWorldHint: false` sitting in
198+
* the annotations literal below, asserted over EVERY bridged tool from
199+
* nothing at all — every tool an app registers under its own name included.
200+
* `AIToolDefinition` has no member expressing it, so that `false` was never a
201+
* property of the tool; it was a property of this file. An app can register a
202+
* tool that calls a weather API, an LLM, or any outbound service, and this
203+
* bridge told every MCP client its domain of interaction was closed. Same
204+
* class as the `readOnlyHint` / `destructiveHint` defect described above.
205+
*
206+
* THE SOURCE is {@link PLATFORM_PROVIDED_TOOL_NAMES}
207+
* (`@objectstack/spec/system`) — the canonical registry of the statically
208+
* named tools the cloud AI runtime registers (`PLATFORM_TOOLS_BY_PACKAGE`).
209+
* Every name in it acts on the ObjectStack environment itself, this stack's
210+
* records and its own metadata, which is a closed and well-defined domain in
211+
* exactly the sense the SDK gives the word. So `false` for those names is
212+
* known rather than assumed, and it is the information option 3 (drop the
213+
* hint outright) would have thrown away.
214+
*
215+
* WHY THE REGISTRY AND NOT THE TWO NAME SETS ABOVE. Same shape — a membership
216+
* test against platform-registered names — but a different question.
217+
* {@link PLATFORM_READ_ONLY_TOOL_NAMES} and
218+
* {@link PLATFORM_DESTRUCTIVE_TOOL_NAMES} answer "what SAFETY class does the
219+
* platform know for this name", and they are deliberately partial: they carry
220+
* only the platform names whose safety class this bridge knows. The question
221+
* here is OWNERSHIP, and the registry is what answers it. Keying the world
222+
* hint off the safety lists instead would drop `create_object`, `add_field`,
223+
* `list_metadata`, `describe_metadata` and twenty more — platform tools whose
224+
* world is just as closed — to the protocol default, reintroducing option 3's
225+
* accuracy loss under a narrower name. A sibling pin already holds the two
226+
* safety lists to this same registry as a SUBSET, so the hints read one
227+
* registry between them rather than three hand lists that can drift apart.
228+
*
229+
* ⚠️ WHY OMISSION IS NOT THE CONSERVATIVE DIRECTION HERE — AND WHY THE
230+
* ASYMMETRY WITH {@link safetyAnnotations} IS DELIBERATE, NOT AN OVERSIGHT
231+
* WAITING TO BE TIDIED. Structurally the two functions are one rule: assert
232+
* what the platform can source, omit what it cannot, because MCP has no
233+
* spelling for "unknown" other than absence. What DIFFERS is the price of
234+
* that absence, and the difference is the SDK's own. Measured in the pinned
235+
* `@modelcontextprotocol/sdk` 1.30.0 (`ToolAnnotationsSchema`, `dist/esm/types.js`):
236+
*
237+
* ```
238+
* readOnlyHint Default: false ← omission reads "not read-only"
239+
* destructiveHint Default: true ← omission reads "may be destructive"
240+
* openWorldHint Default: true ← omission reads "OPEN world"
241+
* ```
242+
*
243+
* For the two safety hints, omission lands on the cautious answer and costs
244+
* only information — which is why #13318 could move them to omit-when-unsourced
245+
* and call it conservative. For this one, omission lands on the LESS cautious
246+
* reading: a tool that sources nothing is understood by every conforming host
247+
* to reach an open world. That trade is accepted on purpose. For an
248+
* app-registered tool this bridge genuinely has no source, and falling to the
249+
* protocol's documented default is honest where asserting `false` was a lie.
250+
* ⛔ So do not "repair" the asymmetry by re-asserting `false` for everyone:
251+
* that IS the defect. The identical STRUCTURE of the two functions is what
252+
* keeps the one rule legible; identical consequences were never the point.
253+
*
254+
* ⛔ NOT the dynamic tool families. `PLATFORM_TOOL_FAMILY_PREFIXES`
255+
* (`action_<name>`) names tools the runtime materialises from an app's OWN
256+
* declarative actions: the platform registers the wrapper, the app defines the
257+
* behaviour, outbound calls included. `mcp-http-tools.ts` asserts
258+
* `openWorldHint: true` for `run_action` on exactly that reasoning. Widening
259+
* this membership test to the prefixes would re-import the defect under a new
260+
* name.
261+
*
262+
* ⛔ NOT `{ openWorldHint: undefined }`. The no-source answer is an absent
263+
* KEY, which is why this returns `{}`: an undefined-valued property is dropped
264+
* by JSON serialization but survives a spread, so "omitted" has to be true of
265+
* the object as well as of the wire.
266+
*
267+
* THE FOLLOW-UP THIS IS NOT. Making the hint a property of the TOOL — a
268+
* declared member on `AIToolDefinition`, with an action-backed tool inheriting
269+
* `run_action`'s `openWorldHint: true` — is the shape that would make it true
270+
* rather than merely defensible. That is a public contract extension in
271+
* `packages/spec/**` and was ruled a follow-up; this is the zero-contract-change
272+
* half, which stops the unsourced assertion now.
273+
*/
274+
function worldAnnotation(tool: AIToolDefinition): ToolWorldHint {
275+
return PLATFORM_PROVIDED_TOOL_NAMES.has(tool.name) ? { openWorldHint: false } : {};
276+
}
277+
187278
// ── AIToolDefinition.parameters → MCP inputSchema ────────────────────────────
188279

189280
/**
@@ -992,8 +1083,11 @@ export class MCPServerRuntime {
9921083
* The safety annotations come from {@link safetyAnnotations}, which reads
9931084
* what the definition DECLARES and omits the hints it cannot source; the
9941085
* name-derived `readOnlyHint: false, destructiveHint: false` this call used
995-
* to assert over every unlisted tool is gone. `openWorldHint` is untouched
996-
* by that change and still asserted for every bridged tool.
1086+
* to assert over every unlisted tool is gone. `openWorldHint` now goes
1087+
* through {@link worldAnnotation} on the same principle — asserted `false`
1088+
* for platform-registered names, omitted for everyone else — but read that
1089+
* function before assuming the two omissions cost the same thing: the SDK
1090+
* defaults them in opposite directions.
9971091
*/
9981092
private registerToolFromDefinition(tool: AIToolDefinition, toolRegistry: ToolRegistry): void {
9991093
const logger = this.config.logger;
@@ -1004,10 +1098,12 @@ export class MCPServerRuntime {
10041098
description: tool.description,
10051099
inputSchema: toolInputSchema(tool, logger),
10061100
annotations: {
1007-
// Only the hints {@link safetyAnnotations} can source — a tool that
1008-
// declares nothing is served neither, so the MCP defaults apply.
1101+
// Only the hints these two can source — a tool that declares nothing
1102+
// and is not a platform name is served neither set, so the MCP
1103+
// defaults apply. Those defaults are NOT symmetrical between them;
1104+
// {@link worldAnnotation} is where that is written down.
10091105
...safetyAnnotations(tool),
1010-
openWorldHint: false,
1106+
...worldAnnotation(tool),
10111107
},
10121108
},
10131109
async (args) => {

packages/mcp/src/mcp-tool-bridge-safety-annotations.test.ts

Lines changed: 132 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,26 @@
3636
* omission, and the SDK's own `ToolAnnotationsSchema` documents the defaults
3737
* that then apply (`readOnlyHint` false, `destructiveHint` **true**), which is
3838
* the conservative reading the old `false` inverted.
39+
*
40+
* THE SECOND DEFECT, PINNED HERE TOO. `openWorldHint: false` was asserted for
41+
* every bridged tool from no source at all — the sibling of the above, one
42+
* hint later. It is now sourced the same way the `readOnlyHint` fallback is,
43+
* from platform-registered names (`PLATFORM_PROVIDED_TOOL_NAMES`), and
44+
* omitted for everyone else.
45+
*
46+
* ⚠️ AND ITS OMISSION IS PINNED DIFFERENTLY, ON PURPOSE. The same SDK schema
47+
* documents `openWorldHint` as **`Default: true`**, so an omitted world hint
48+
* is read as an OPEN world — the one place in this file where absence is not
49+
* the cautious answer, only the honest one. A future reader tempted to make
50+
* the three hints behave alike has to make these pins red first, which is the
51+
* point of stating it here as well as at the call site.
52+
*
53+
* ⚠️ ABSENCE MEANS AN ABSENT KEY, and these cases say so with
54+
* `Object.hasOwn`, not with `toBeUndefined()`. A spread of
55+
* `{ openWorldHint: undefined }` would satisfy `toBeUndefined()` while still
56+
* putting the property on the object; every such case below carries a
57+
* same-object positive control (a platform tool listed in the same call) so a
58+
* `false` from {@link hasHint} is a reading rather than a typo'd key name.
3959
*/
4060

4161
import { describe, it, expect, afterEach } from 'vitest';
@@ -155,6 +175,19 @@ async function annotationsOf(
155175
return { session, byName: Object.fromEntries(listed.map((t: any) => [t.name, t])) };
156176
}
157177

178+
/**
179+
* Own-property presence on the PARSED WIRE object.
180+
*
181+
* ⚠️ Not `toBeUndefined()`, which a spread of `{ openWorldHint: undefined }`
182+
* would also satisfy while still putting the property on the object — the
183+
* distinction these cases exist to make. ⛔ And not `Object`.`hasOwn` either:
184+
* that is ES2022 and this repo compiles at `lib: ["ES2020"]`, so it type-errors
185+
* where the runtime (Node 22) would have run it happily — a gap this package's
186+
* own `typecheck` cannot report, because its tsconfig excludes test files.
187+
*/
188+
const hasHint = (annotations: Record<string, unknown> | undefined, hint: string): boolean =>
189+
Object.prototype.hasOwnProperty.call(annotations ?? {}, hint);
190+
158191
const tool = (name: string, extra: Partial<AIToolDefinition> = {}): AIToolDefinition => ({
159192
name,
160193
description: `the ${name} tool`,
@@ -215,11 +248,13 @@ describe('bridgeTools — the safety annotations a client receives', () => {
215248
const s = await annotationsOf([tool('send_invoice_email')]);
216249
openSession = s.session;
217250

218-
const annotations = s.byName.send_invoice_email.annotations;
251+
const annotations = s.byName.send_invoice_email.annotations ?? {};
219252
expect(annotations.destructiveHint).toBeUndefined();
220253
expect(annotations.readOnlyHint).toBeUndefined();
221-
// The hint the bridge does still assert for every tool, unchanged here.
222-
expect(annotations.openWorldHint).toBe(false);
254+
// ...and no world hint either. `send_invoice_email` is the case in the
255+
// name: an app tool that reaches an outbound service was being told to
256+
// every client as closed-world.
257+
expect(hasHint(annotations, 'openWorldHint')).toBe(false);
223258
});
224259

225260
it('what the definition declares outranks what its name suggests', async () => {
@@ -237,6 +272,9 @@ describe('bridgeTools — the safety annotations a client receives', () => {
237272
expect(PLATFORM_PROVIDED_TOOL_NAMES.has('aggregate_records')).toBe(false);
238273
expect(s.byName.aggregate_records.annotations.readOnlyHint).toBeUndefined();
239274
expect(s.byName.aggregate_records.annotations.destructiveHint).toBeUndefined();
275+
// Nor a world hint from this bridge. It gets one at its OWN registration
276+
// site in `mcp-http-tools.ts`, which is where that fact is known.
277+
expect(hasHint(s.byName.aggregate_records.annotations, 'openWorldHint')).toBe(false);
240278
});
241279

242280
/**
@@ -274,4 +312,95 @@ describe('bridgeTools — the safety annotations a client receives', () => {
274312
expect(s.byName[stranger.name].annotations.destructiveHint).toBeUndefined();
275313
}
276314
});
315+
316+
// ── openWorldHint ────────────────────────────────────────────────────────
317+
318+
it('CONTROL: a platform-registered name still receives `openWorldHint: false`', async () => {
319+
const s = await annotationsOf([
320+
tool('query_records'),
321+
tool('list_objects'),
322+
// Platform names OUTSIDE the two safety-class sets. These are the tools
323+
// a fallback keyed on those sets instead of the registry would have
324+
// silently flipped to the protocol's open-world default.
325+
tool('create_object'),
326+
tool('list_metadata'),
327+
tool('describe_metadata'),
328+
]);
329+
openSession = s.session;
330+
331+
for (const name of ['query_records', 'list_objects', 'create_object', 'list_metadata', 'describe_metadata']) {
332+
expect(PLATFORM_PROVIDED_TOOL_NAMES.has(name)).toBe(true);
333+
expect(s.byName[name].annotations.openWorldHint).toBe(false);
334+
}
335+
// The safety hints are sourced separately: `create_object` is a platform
336+
// name in NEITHER safety set, so it keeps the world hint and no other.
337+
expect(s.byName.create_object.annotations.readOnlyHint).toBeUndefined();
338+
expect(s.byName.create_object.annotations.destructiveHint).toBeUndefined();
339+
});
340+
341+
it('an app-registered tool receives NO `openWorldHint` KEY — absence, not `undefined`', async () => {
342+
const s = await annotationsOf([
343+
tool('check_weather'),
344+
tool('ask_llm', { requiresConfirmation: false }),
345+
tool('delete_opportunity', { requiresConfirmation: true }),
346+
// Positive control in the same `tools/list` answer: `hasOwn` must be
347+
// able to say `true` about this exact wire object, or the `false`s
348+
// below would be indistinguishable from a misspelled key.
349+
tool('query_records'),
350+
]);
351+
openSession = s.session;
352+
353+
expect(hasHint(s.byName.query_records.annotations, 'openWorldHint')).toBe(true);
354+
355+
for (const name of ['check_weather', 'ask_llm', 'delete_opportunity']) {
356+
const annotations = s.byName[name].annotations ?? {};
357+
expect(hasHint(annotations, 'openWorldHint')).toBe(false);
358+
expect(annotations.openWorldHint).toBeUndefined();
359+
}
360+
361+
// Independently sourced: declaring `requiresConfirmation` buys the SAFETY
362+
// hints and buys nothing about the world, which is the whole point of the
363+
// two derivations being separate.
364+
expect(s.byName.delete_opportunity.annotations.destructiveHint).toBe(true);
365+
expect(s.byName.ask_llm.annotations.destructiveHint).toBe(false);
366+
});
367+
368+
/**
369+
* The world-hint counterpart of the fallback invariant above, driven across
370+
* the whole registry at once: `openWorldHint: false` is a claim the platform
371+
* can source about the tools it registers, and about nothing else.
372+
*/
373+
it('exactly the platform-registered names carry `openWorldHint`, and every one of them carries `false`', async () => {
374+
const platform = [...PLATFORM_PROVIDED_TOOL_NAMES].map((name) => tool(name));
375+
const strangers = [
376+
'aggregate_records',
377+
'action_close_deal',
378+
'check_weather',
379+
'send_invoice_email',
380+
'void_invoice',
381+
].map((name) => tool(name));
382+
383+
const s = await annotationsOf([...platform, ...strangers]);
384+
openSession = s.session;
385+
386+
const withWorldHint = Object.values(s.byName)
387+
.filter((t: any) => hasHint(t.annotations, 'openWorldHint'))
388+
.map((t: any) => t.name)
389+
.sort();
390+
391+
// ⚠️ Non-vacuity first: `toEqual` between two empty arrays passes, so an
392+
// unbuilt or empty registry would make every assertion below say nothing.
393+
expect(PLATFORM_PROVIDED_TOOL_NAMES.size).toBeGreaterThan(0);
394+
expect(withWorldHint.length).toBe(PLATFORM_PROVIDED_TOOL_NAMES.size);
395+
expect(withWorldHint).toEqual([...PLATFORM_PROVIDED_TOOL_NAMES].sort());
396+
for (const name of withWorldHint) {
397+
expect(s.byName[name].annotations.openWorldHint).toBe(false);
398+
}
399+
400+
// `action_close_deal` is the family case stated explicitly: the runtime
401+
// materialises `action_<name>` wrappers around an app's OWN actions, so a
402+
// membership test widened to `PLATFORM_TOOL_FAMILY_PREFIXES` would put
403+
// this bridge back to claiming a closed world over app-defined behaviour.
404+
expect(hasHint(s.byName.action_close_deal.annotations, 'openWorldHint')).toBe(false);
405+
});
277406
});

0 commit comments

Comments
 (0)