fix(mcp): pin the tool bridge's two hand-copied safety name sets in the direction the old pin could not see

.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.

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<string> = 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<string> = new Set([
   'delete_field',
 ]);
 

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<Record<string, ReadonlySet<string>>> = {
+    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());
+  });
+});