Skip to content

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

Merged
os-steve merged 3 commits into
mainfrom
claude/issue-13486-mcp-safety-set-drift-pin
Aug 31, 2026
Merged

fix(mcp): pin the tool bridge's two hand-copied safety name sets in the direction the old pin could not see#13888
os-steve merged 3 commits into
mainfrom
claude/issue-13486-mcp-safety-set-drift-pin

Conversation

@claude

@claude claude Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Fixes #13486

mcp-server-runtime.ts keeps two literal name sets that safetyAnnotations consults to
decide a bridged tool's readOnlyHint / destructiveHint when the definition declares
nothing:

PLATFORM_READ_ONLY_TOOL_NAMES   = list_objects, describe_object, query_records, get_record, aggregate_data
PLATFORM_DESTRUCTIVE_TOOL_NAMES = delete_field

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.

The danger is a safety annotation inherited by NAME COLLISION

The uncovered direction looks like the benign one — a name withdrawn from the registry
keeps being annotated, and the platform no longer registers it, so who is it annotating?

Somebody else. These sets annotate by name. Once a name leaves
PLATFORM_TOOLS_BY_PACKAGE, 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. The harm is not "we annotated a tool that no longer
exists"; that tool is gone. It is a safety hint acquired by collision, and it is why this
is worth closing while the data is still clean.

The criterion is the ITERATION SOURCE

The existing case, no tool outside PLATFORM_PROVIDED_TOOL_NAMES receives a hint it did
not declare
, builds its tools as the registry plus a hardcoded stranger list and asserts
every annotated name is in the registry. Its iteration source is the registry, so it
can only ever see local-has / registry-lacks. A name removed from the registry while
it stays in a local set is not among the tools it bridges at all: nothing drives it,
annotated never contains it, and the case stays green over exactly the drift it is
named for.

The new assertion therefore iterates the thing that can drift — the two local sets —
and checks each name against the registry.

The design trade-off, and which road was taken

The card named two roads:

  1. Export the two private sets so a test can assert over their own contents — cost:
    widening the module's surface for a test;
  2. Hard-code the six names in the test — a third hand-copied list.

Road (1) was taken. Road (2) is not merely inelegant, it fails the stated
criterion
: an assertion over a literal written in the test file never reads what
safetyAnnotations actually consults, so it pins a copy against a copy. Delete a name
from the runtime set and road (2) notices nothing; that is the same vacuity the card is
about, one file over. The cost of road (1) was measured rather than assumed (see Clause
② below) and turns out to be zero on the published surface.

The export is deliberately narrow: the two constants are exported from
mcp-server-runtime.ts only
and are not re-exported from index.ts, which is the
package's sole exports entry. They are typed ReadonlySet of string, mirroring the
registry's own spelling, so a consumer cannot mutate the fallback through the type.

A third road, considered and rejected

Filtering the literals through PLATFORM_PROVIDED_TOOL_NAMES at construction would make
the sets structurally immune the way worldAnnotation is. It was rejected because it
absorbs the drift instead of reporting it: the withdrawn name would simply stop
annotating, nothing would go red, and the folklore would stay in the file forever. A
hand-maintained list that silently self-heals is a list nobody ever corrects. The card
wants a withdrawn name to be loud, in CI, at the one moment somebody can still delete
it. The rejection is recorded in the source docblock so the next reader does not have to
re-derive it.

Scope

worldAnnotation (added by #13350, which is closed and merged) 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 — structurally immune, and kept as the
contrast for what shape does not get this disease.

Re-verified before building on the card

All six names are still in PLATFORM_PROVIDED_TOOL_NAMES (size 30), so this is a gap in a
guarantee, not a live defect. Controls both ways in the same run: aggregate_records,
forecast_revenue and a fabricated name all report ABSENT; create_object and
list_metadata — registry names outside the six — report PRESENT. Without those, an
all-PRESENT reading cannot be told from a membership test that answers true to
everything.

The reverse control, in full

Direction predicted in writing before either mutation ran. The mutation marker was chosen
for discriminating power: the fabricated token delete_field__ablated13486 occurs in 0
files before the mutation and 1 after, where the obvious alternative — the bare token
delete_field — occurs in 7 files and could not have distinguished "mutation landed" from
"mutation did nothing".

Experiment R — a fabricated name dropped into a local set. In-package mutation; the
suite imports ./mcp-server-runtime.js, a relative specifier vitest resolves to source.

  • Injected-text occurrences 0 to 1; blob e9eb93ce12f2 differs from HEAD 3545ae84aab7.
  • Predicted: exactly one red — the new case. Observed: exactly one red.
    AssertionError: `PLATFORM_READ_ONLY_TOOL_NAMES` still carries `__fabricated_never_registered__`,
    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
    `__fabricated_never_registered__` inherits a safety hint it never declared.
    Tests  1 failed | 12 passed (13)
    
  • The old registry-driven pin stayed green, as predicted. That is a second, narrower
    blindness
    than the card names: a fabricated local name is in neither the registry nor
    the hardcoded stranger list, so the old case never bridges it either. The new pin covers
    that direction too.
  • Restore proven: blob back to 3545ae84aab7, injected text 0 occurrences, git diff HEAD
    empty, whole-tree git status --porcelain empty.

Experiment D — the drift the card is actually about. Cross-package: the registry entry
delete_field renamed to delete_field__ablated13486, so the name has LEFT
PLATFORM_TOOLS_BY_PACKAGE while it remains in PLATFORM_DESTRUCTIVE_TOOL_NAMES.

@objectstack/spec is in this package's KNOWN_UNALIASED_TEST_IMPORTS entry, so
@objectstack/spec/system resolves through exports to packages/spec/dist and editing
the source alone changes nothing the suite can see. Both legs were rebuilt and both were
preflighted:

  • mutate leg: ablation-dist-preflight @objectstack/spec 'delete_field__ablated13486'
    "marker present in 4 built files ... the ablation is live in the artifact the suite
    consumes"
    .
  • restore leg: the same with --absent"marker absent from all 215 built files" and
    "tree: working tree clean against HEAD".

Predicted: two reds, and the old pin green. Observed exactly that:

Tests  2 failed | 11 passed (13)
 × CONTROL: `delete_field` keeps `destructiveHint: true` (unchanged by this fix)
 × every name in the two safety sets is still a name the platform registers
  • The new case fires with the collision message naming delete_field.
  • The CONTROL case fires on its openWorldHint half only — worldAnnotation correctly
    stops annotating a withdrawn name. That is the structural immunity above, firing as
    predicted, and it doubles as an independent proof that the rebuilt dist really reached
    the suite.
  • The old registry-driven pin stayed green. That is the card's claim, now measured
    rather than argued.
  • Post-restore: 13 passed, exit 0.

Without these controls the new pin would be exactly as possibly-vacuous as the old one,
which is this card's entire subject.

Clause ②: NO — the published surface is byte-identical, and both zeros carry controls

Four builds of @objectstack/mcp at one head:

artifact with change vs without control
dist/index.d.ts IDENTICAL acb8ba7e36a6 see below
dist/index.d.cts IDENTICAL acb8ba7e36a6 see below
dist/index.js IDENTICAL 8421e6a4060a moves to ce752f49f34b under the probe
dist/index.cjs IDENTICAL 020774999552 moves to 2efe3910ea04 under the probe

Two positive controls, because "IDENTICAL" is a zero and a zero with no control is
indistinguishable from an artifact that never moves:

  • Rebuild control. Adding one name to PLATFORM_READ_ONLY_TOOL_NAMES moves
    index.js / index.cjs and puts the probe string in dist/index.js — so the pipeline
    demonstrably carries an edit to this file into the published bundle.
  • Type-surface control. Appending export { PLATFORM_READ_ONLY_TOOL_NAMES } from './mcp-server-runtime.js'; to index.ts moves dist/index.d.ts from acb8ba7e36a6 to
    c53d61318a5d and puts the symbol in it twice — so the .d.ts is sensitive to
    exactly this class of change, and the IDENTICAL above means the export did not reach it,
    not that the file never moves. Reverted; the hash returns to acb8ba7e36a6 and the
    symbol count to 0.

npm pack --dry-run confirms what can carry a change at all: the tarball is dist/*,
README, CHANGELOG, LICENSE and package.json — no src/, no test files.

⇒ The module's surface widens by two bindings; the package's published surface does
not. Clause ② is declared no, on the measurement rather than on the argument. The PR
stays draft and unarmed regardless, per the dispatch contract.

Worth stating rather than resolving silently: option (1) was the road that risked tripping
clause ② and option (2) was the road that could not. Choosing the weaker design to duck a
review gate would have been the wrong reason to choose it, so the measurement was taken
after the design call, not instead of it.

Changeset: patch, proposed with reasoning

skip-changeset was considered and rejected. Its closed condition is a diff that publishes
nothing from any package, and this diff edits packages/mcp/src/mcp-server-runtime.ts
a package source file, outside that list — regardless of what dist turns out to show.
And where the two readings conflict, the seat rule prefers the failure mode that is release
noise over the one that is a silent gate bypass. patch is the honest minimum: no
runtime behaviour changes, no name was added, removed or reclassified, and the published
.d.ts is unchanged; what changes is a claim in shipped source that was not true.

Verification

Gate union derived after merging origin/main and after the last edit, with
scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands, then run at
df33dd2111 (the union and the final commit are the same tree):

union named 34, ran 34, unreconciled 0 — from
comm -23 <(sort gates.list) <(sort ran.list), exact string comparison, no prefix or
substring matching and no arithmetic on a counter.

31 of the 34 returned exit 0. The other three are NOT MEASURED, in each gate's own
words, and none is a red:

  • node scripts/check-test-completeness.mjs — exit 3: "the local reading for this gate is
    NOT MEASURED. ⛔ It is not a red, and there is nothing here to fix. Fix: pass a saved
    turbo run test log"
    .
  • pnpm check:dual-build-cjs-loads — exit 3: "PREREQUISITE NOT MET — this gate reads
    built output, and some package has no dist/. ... ⛔ This is NOT a pass: nothing was
    measured."
    Its 93-case self-test passed.
  • pnpm check:type-check-debt — exit 1: "--re-measure cannot run: 48 workspace
    dependenc(ies) of the ledgered packages have no built type entry point on disk ... Build
    the closure first, exactly as lint.yml does before this step"
    . All three need a
    full-workspace build, which is CI's run. (That this one signals an unmeasurable with
    exit 1 while its two siblings use exit 3 is filed separately as check:type-check-debt signals "closure not built, nothing measured" with exit 1, the code the repo reserves for a real finding #13885 — not addressed
    here.)

Package-level, all exit codes captured before any pipe:

  • pnpm --filter @objectstack/mcp exec vitest run271 passed, 25 files, exit 0.
  • pnpm --filter @objectstack/mcp run typecheck — exit 0. ⚠️ NOT MEASURED for the new
    test code
    : tsc --listFiles shows 402 files in that program and zero *.test.ts
    among them, because the package tsconfig excludes them. The green says nothing about the
    file this PR adds cases to.
  • Because that hole is ledgered at TEST_DEBT['@objectstack/mcp'] = 53 with the entry's
    own note recording no margin"the next new error in this package goes red on
    arrival"
    — the hidden layer was measured directly, on a synthesized test-inclusive
    program: 53 diagnostics before the change and 53 after, with 0 in
    mcp-tool-bridge-safety-annotations.test.ts
    in both. 53 is exactly the ledgered
    number, which cross-checks the program against the gate's own recording. Control: 6
    distinct *.test.ts files appear in the diagnostics, so the test layer really is in the
    program. (A first attempt returned 1 diagnostic and 0 test files — the control caught it
    as NOT MEASURED, not as a clean bill: the synthesized config could not resolve
    @types/node from its temp directory. Re-run with type roots pinned.)
  • turbo run build --filter=@objectstack/mcp — exit 0, 8 tasks. ⛔ The
    '@objectstack/mcp^...' filter spelling was avoided throughout (known false red,
    [finding] pnpm --filter '&lt;pkg&gt;^...' build fails on an UNMODIFIED origin/main — two independent observations, two different packages, same construct #13513).

CI convergence is the PM's step; nothing here waits on it.


Generated by Claude Code

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

2 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to listnot a clean bill of health. This check sees only pages that NAME a derived anchor: one that documents this change in prose, or enumerates it in an authoring dialect, names none and stays invisible to it on every run.

What this run could not see
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 12 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 9b21037d4198cd14105abd354b019e9323b4fe57packageMentionDocs.

Which tree this was computed on

This run read content/docs from 09bee5b57142b28800b7dff17ee67ad307f30ab1 — the merge of head df33dd2111fcbcc5afe56186b99581b0ceea2038 into base 9b21037d4198cd14105abd354b019e9323b4fe57, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 09bee5b57142b28800b7dff17ee67ad307f30ab1 && git checkout 09bee5b57142b28800b7dff17ee67ad307f30ab1
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 9b21037d4198cd14105abd354b019e9323b4fe57 df33dd2111fcbcc5afe56186b99581b0ceea2038 && git checkout -B drift-repro 9b21037d4198cd14105abd354b019e9323b4fe57 && git merge --no-ff df33dd2111fcbcc5afe56186b99581b0ceea2038

node scripts/docs-audit/affected-docs.mjs --json 9b21037d4198cd14105abd354b019e9323b4fe57

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Aug 31, 2026
@os-steve
os-steve marked this pull request as ready for review August 31, 2026 17:10
@os-steve
os-steve enabled auto-merge August 31, 2026 17:10
@os-steve
os-steve added this pull request to the merge queue Aug 31, 2026
Merged via the queue into main with commit 6193e57 Aug 31, 2026
35 checks passed
@os-steve
os-steve deleted the claude/issue-13486-mcp-safety-set-drift-pin branch August 31, 2026 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The MCP bridge's two platform-name sets are hand copies of the registry, and the pin that claims to hold them there cannot see a name that has LEFT it

2 participants