diff --git a/.changeset/mcp-safety-name-sets-drift-pin.md b/.changeset/mcp-safety-name-sets-drift-pin.md new file mode 100644 index 0000000000..2a2eeba557 --- /dev/null +++ b/.changeset/mcp-safety-name-sets-drift-pin.md @@ -0,0 +1,36 @@ +--- +"@objectstack/mcp": patch +--- + +fix(mcp): pin the tool bridge's two hand-copied safety name sets in the direction the old pin could not see (#13486) + +`mcp-server-runtime.ts` keeps two literal name sets — `PLATFORM_READ_ONLY_TOOL_NAMES` +and `PLATFORM_DESTRUCTIVE_TOOL_NAMES` — that `safetyAnnotations` consults to decide a +bridged tool's `readOnlyHint` / `destructiveHint` when the definition declares nothing. +Both are hand copies of `PLATFORM_TOOLS_BY_PACKAGE` (`@objectstack/spec/system`), and +the docblock claimed a sibling pin held them there. + +It held them in one direction only. That pin bridges `[...PLATFORM_PROVIDED_TOOL_NAMES]` +and asserts every annotated name is in the registry, so its **iteration source is the +registry**: it sees a name added to a local set that the platform never registers. A name +**withdrawn** from `PLATFORM_TOOLS_BY_PACKAGE` while it stays in a local set is not among +the tools it bridges at all — nothing drives it, nothing is annotated, and the case stays +green over exactly the drift it is named for. + +The harm in that direction is not "a tool the platform no longer registers keeps a hint". +These sets annotate **by name**, so once a name leaves the registry, a **plugin** +registering a tool of that name inherits a `readOnlyHint` it never declared — a read-only +promise the plugin may not honour, handed to it by a stale literal. That is what makes the +gap worth closing while the data is still clean. + +The new pin iterates the thing that can drift — the two sets — and checks each name +against the registry, plus a coverage guard that fails if the module exports a +name-keyed safety set the pin does not cover. Reaching the sets from a test required +exporting them from `mcp-server-runtime.ts`; they are deliberately **not** re-exported +from `index.ts`, so `dist/index.d.ts` and the package's published surface are unchanged. +No runtime behaviour changes: no name was added, removed or reclassified, and all six +were re-verified present in the registry (size 30). + +`worldAnnotation` is untouched on purpose. It reads `PLATFORM_PROVIDED_TOOL_NAMES` +directly, so derivation and pin share one source and a withdrawn name simply stops being +annotated — the shape that does not get this disease, kept as the contrast. diff --git a/packages/mcp/src/mcp-server-runtime.ts b/packages/mcp/src/mcp-server-runtime.ts index 58e73492bb..3545ae84aa 100644 --- a/packages/mcp/src/mcp-server-runtime.ts +++ b/packages/mcp/src/mcp-server-runtime.ts @@ -63,9 +63,36 @@ interface ObjectDef { * Every name here is a tool the cloud AI runtime registers statically * (`PLATFORM_TOOLS_BY_PACKAGE` in `@objectstack/spec/system`) and hands to * this bridge through the AI service's `ToolRegistry` carrying no - * `requiresConfirmation`. A sibling pin holds both sets to that registry, so - * the lists cannot drift back into folklore: a name the platform does not - * register is a name this bridge knows nothing about. + * `requiresConfirmation`. Two sibling pins hold both sets to that registry — + * one per direction — so the lists cannot drift back into folklore: a name the + * platform does not register is a name this bridge knows nothing about. + * + * WHY BOTH SETS ARE EXPORTED, AND FOR WHAT. The older pin (`no tool outside + * PLATFORM_PROVIDED_TOOL_NAMES receives a hint it did not declare`) bridges + * `[...PLATFORM_PROVIDED_TOOL_NAMES]` and asserts every annotated name is in + * it. Its ITERATION SOURCE is the registry, so it can only ever see + * local-has → registry-lacks. The reverse — a name WITHDRAWN from + * `PLATFORM_TOOLS_BY_PACKAGE` while it stays in a set here — is not among the + * tools that pin bridges at all: nothing drives it, nothing is annotated, and + * the case stays green over exactly the drift it is named for. + * + * ⚠️ That silent direction is the dangerous one, and NOT because a tool the + * platform no longer registers keeps a hint. These sets annotate BY NAME. Once + * a name leaves the registry, a PLUGIN registering a tool of that name + * inherits a `readOnlyHint` it never declared — a read-only promise the plugin + * may not honour, handed to it by a stale literal in this file. So the second + * pin iterates the thing that can drift, which is these two sets, and that + * requires reaching them from outside this module. They are exported for that + * and are deliberately NOT re-exported from `index.ts`: the package's + * published surface is unchanged, and a new name-keyed set added here must be + * exported too or the pin's coverage guard goes red. + * + * ⛔ Do not "repair" this by filtering the literals through + * `PLATFORM_PROVIDED_TOOL_NAMES` at construction. That absorbs the drift + * instead of reporting it — the withdrawn name would simply stop annotating, + * nothing would go red, and the folklore would stay in this file forever. The + * pin exists to make a withdrawn name LOUD, in CI, at the one moment somebody + * can still delete it. * * ⛔ What the fallback must never do again is answer for tools it does NOT * contain. These two sets used to be the ONLY source of both hints, so the @@ -80,7 +107,7 @@ interface ObjectDef { * registers it — annotated `readOnlyHint: true` — at its own registration site * in `mcp-http-tools.ts`, and never reaches this path. */ -const PLATFORM_READ_ONLY_TOOL_NAMES = new Set([ +export const PLATFORM_READ_ONLY_TOOL_NAMES: ReadonlySet = new Set([ 'list_objects', 'describe_object', 'query_records', @@ -90,9 +117,10 @@ const PLATFORM_READ_ONLY_TOOL_NAMES = new Set([ /** * The destructive half of the same platform-name fallback — see - * {@link PLATFORM_READ_ONLY_TOOL_NAMES} for what it is and is not for. + * {@link PLATFORM_READ_ONLY_TOOL_NAMES} for what it is and is not for, and for + * why both halves are exported to a test rather than kept private. */ -const PLATFORM_DESTRUCTIVE_TOOL_NAMES = new Set([ +export const PLATFORM_DESTRUCTIVE_TOOL_NAMES: ReadonlySet = new Set([ 'delete_field', ]); 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 d77e6ec909..020cb347c4 100644 --- a/packages/mcp/src/mcp-tool-bridge-safety-annotations.test.ts +++ b/packages/mcp/src/mcp-tool-bridge-safety-annotations.test.ts @@ -65,7 +65,12 @@ import type { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; import type { AIToolDefinition, ToolCallPart } from '@objectstack/spec/contracts'; import { PLATFORM_PROVIDED_TOOL_NAMES } from '@objectstack/spec/system'; -import { MCPServerRuntime } from './mcp-server-runtime.js'; +import * as serverRuntimeModule from './mcp-server-runtime.js'; +import { + MCPServerRuntime, + PLATFORM_READ_ONLY_TOOL_NAMES, + PLATFORM_DESTRUCTIVE_TOOL_NAMES, +} from './mcp-server-runtime.js'; import type { ToolRegistry, ToolExecutionResult } from './types.js'; // --------------------------------------------------------------------------- @@ -278,11 +283,18 @@ describe('bridgeTools — the safety annotations a client receives', () => { }); /** - * The invariant that keeps the name fallback from drifting back into - * folklore, asserted from OUTSIDE the module (the two sets are private): - * only a name the platform itself registers may receive a hint it did not - * declare. Driving every platform name at once also proves the fallback is a - * SUBSET of that registry rather than merely overlapping it. + * ONE of the two directions that keep the name fallback from drifting back + * into folklore: only a name the platform itself registers may receive a + * hint it did not declare. Driving every platform name at once also proves + * the fallback is a SUBSET of that registry rather than merely overlapping + * it. + * + * ⚠️ Its ITERATION SOURCE is the registry, which is exactly what bounds it. + * A name WITHDRAWN from `PLATFORM_TOOLS_BY_PACKAGE` while it stays in a + * local set is not among the tools bridged here, so nothing drives it, + * `annotated` never contains it, and this case stays green. The other + * direction is pinned by the sibling describe at the foot of this file, + * which iterates the local sets instead. */ it('no tool outside `PLATFORM_PROVIDED_TOOL_NAMES` receives a hint it did not declare', async () => { const platform = [...PLATFORM_PROVIDED_TOOL_NAMES].map((name) => tool(name)); @@ -404,3 +416,87 @@ describe('bridgeTools — the safety annotations a client receives', () => { expect(hasHint(s.byName.action_close_deal.annotations, 'openWorldHint')).toBe(false); }); }); + +// --------------------------------------------------------------------------- + +/** + * THE DIRECTION THE CASE ABOVE CANNOT SEE (#13486). + * + * `safetyAnnotations` keeps two literal name sets that are hand copies of + * `PLATFORM_TOOLS_BY_PACKAGE`. The registry-driven pin above catches a name + * added to a set but never registered. It is structurally blind to the + * reverse: a name REMOVED from the registry while it stays in a set is simply + * not one of the tools that pin bridges, so nothing drives it and the case + * stays green. + * + * ⚠️ WHY THAT REVERSE MATTERS WHILE THE DATA IS CLEAN. The harm is not "a tool + * the platform no longer registers keeps a hint" — that tool is gone. These + * sets annotate BY NAME, so once a name leaves the registry, a PLUGIN + * registering a tool of that name inherits a `readOnlyHint` it never declared. + * A safety annotation acquired by name collision, from a stale literal. + * + * THE ITERATION SOURCE IS THE POINT. These cases iterate the two sets — the + * thing that can drift — and check each name against the registry. ⛔ The + * names are never re-typed here: a hard-coded list of the six would be a THIRD + * hand copy, i.e. the defect this pins, and it would pin a copy against a copy + * without ever reading what `safetyAnnotations` actually consults. + * + * These cases deliberately do not drive the transport. The wire behaviour of + * both sets is already pinned by the two CONTROL cases at the head of this + * file; what is unpinned is the CONTENT of the sets, which is data. + */ +describe('the hand-maintained safety name sets cannot drift out of the registry', () => { + /** + * The sets under test, keyed by their module-export name so a failure names + * the set to edit. VALUES are imported, never re-typed — see above. + */ + const COVERED_SETS: Readonly>> = { + PLATFORM_READ_ONLY_TOOL_NAMES, + PLATFORM_DESTRUCTIVE_TOOL_NAMES, + }; + + it('every name in the two safety sets is still a name the platform registers', () => { + // Non-vacuity first, on both sides: an empty registry would make every + // `has()` below false rather than silently true, but an empty SET would + // make the loop run zero times and pass saying nothing. + expect(PLATFORM_PROVIDED_TOOL_NAMES.size).toBeGreaterThan(0); + + const checked: string[] = []; + for (const [setName, names] of Object.entries(COVERED_SETS)) { + expect(names.size).toBeGreaterThan(0); + for (const name of names) { + checked.push(name); + expect( + PLATFORM_PROVIDED_TOOL_NAMES.has(name), + `\`${setName}\` still carries \`${name}\`, which \`PLATFORM_TOOLS_BY_PACKAGE\` no longer registers. ` + + `Delete the name from the set — do NOT widen the registry to match it. ` + + `Left there, any plugin registering a tool called \`${name}\` inherits a safety hint it never declared.`, + ).toBe(true); + } + } + expect(checked.length).toBeGreaterThan(0); + }); + + /** + * ⚠️ The case above can only iterate the sets it was told about. A third + * name-keyed set added to `mcp-server-runtime.ts` would be annotating tools + * with nothing holding its contents to the registry, and no existing + * assertion would notice — the same silence, one set over. + * + * This guard closes that by discovering the sets from the module's own + * exports. It cannot see a set that is left PRIVATE, which is why the source + * docblock instructs the author to export it; what it can do is refuse to + * let an exported one go unpinned. + */ + it('COVERAGE GUARD: every name-keyed safety set the module exports is covered above', () => { + const exported = Object.entries(serverRuntimeModule) + .filter(([name, value]) => /^PLATFORM_[A-Z0-9_]*_TOOL_NAMES$/.test(name) && value instanceof Set) + .map(([name]) => name) + .sort(); + + // Non-vacuity: without this, a regex that matches nothing would leave two + // empty arrays agreeing with each other. + expect(exported.length).toBeGreaterThan(0); + expect(exported).toEqual(Object.keys(COVERED_SETS).sort()); + }); +});