fix(mcp): pin the tool bridge's two hand-copied safety name sets in the direction the old pin could not see - #13888
Conversation
📓 Docs Drift Check2 anchor(s) derived from 1 changed package(s); no hand-written page names any of them, so this run has nothing to list — not 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
Coarse fallback — 12 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # 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 |
Fixes #13486
mcp-server-runtime.tskeeps two literal name sets thatsafetyAnnotationsconsults todecide a bridged tool's
readOnlyHint/destructiveHintwhen the definition declaresnothing:
Both are hand copies of
PLATFORM_TOOLS_BY_PACKAGE(@objectstack/spec/system), and thedocblock 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 areadOnlyHintit never declared — a read-only promise the plugin may not honour, handedto 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_NAMESreceives a hint it didnot 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,
annotatednever contains it, and the case stays green over exactly the drift it isnamed 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:
widening the module's surface for a test;
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
safetyAnnotationsactually consults, so it pins a copy against a copy. Delete a namefrom 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.tsonly and are not re-exported fromindex.ts, which is thepackage's sole
exportsentry. They are typedReadonlySetof string, mirroring theregistry'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_NAMESat construction would makethe sets structurally immune the way
worldAnnotationis. It was rejected because itabsorbs 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_NAMESdirectly, so derivation and pin share one sourceand 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 aguarantee, not a live defect. Controls both ways in the same run:
aggregate_records,forecast_revenueand a fabricated name all report ABSENT;create_objectandlist_metadata— registry names outside the six — report PRESENT. Without those, anall-PRESENT reading cannot be told from a membership test that answers
truetoeverything.
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__ablated13486occurs in 0files 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.e9eb93ce12f2differs from HEAD3545ae84aab7.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.
3545ae84aab7, injected text 0 occurrences,git diff HEADempty, whole-tree
git status --porcelainempty.Experiment D — the drift the card is actually about. Cross-package: the registry entry
delete_fieldrenamed todelete_field__ablated13486, so the name has LEFTPLATFORM_TOOLS_BY_PACKAGEwhile it remains inPLATFORM_DESTRUCTIVE_TOOL_NAMES.@objectstack/specis in this package'sKNOWN_UNALIASED_TEST_IMPORTSentry, so@objectstack/spec/systemresolves throughexportstopackages/spec/distand editingthe source alone changes nothing the suite can see. Both legs were rebuilt and both were
preflighted:
ablation-dist-preflight @objectstack/spec 'delete_field__ablated13486'—"marker present in 4 built files ... the ablation is live in the artifact the suite
consumes".
--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:
delete_field.CONTROLcase fires on itsopenWorldHinthalf only —worldAnnotationcorrectlystops annotating a withdrawn name. That is the structural immunity above, firing as
predicted, and it doubles as an independent proof that the rebuilt
distreally reachedthe suite.
rather than argued.
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/mcpat one head:dist/index.d.tsacb8ba7e36a6dist/index.d.ctsacb8ba7e36a6dist/index.js8421e6a4060ace752f49f34bunder the probedist/index.cjs0207749995522efe3910ea04under the probeTwo positive controls, because "IDENTICAL" is a zero and a zero with no control is
indistinguishable from an artifact that never moves:
PLATFORM_READ_ONLY_TOOL_NAMESmovesindex.js/index.cjsand puts the probe string indist/index.js— so the pipelinedemonstrably carries an edit to this file into the published bundle.
export { PLATFORM_READ_ONLY_TOOL_NAMES } from './mcp-server-runtime.js';toindex.tsmovesdist/index.d.tsfromacb8ba7e36a6toc53d61318a5dand puts the symbol in it twice — so the.d.tsis sensitive toexactly 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
acb8ba7e36a6and thesymbol count to 0.
npm pack --dry-runconfirms what can carry a change at all: the tarball isdist/*,README, CHANGELOG, LICENSE and
package.json— nosrc/, 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 reasoningskip-changesetwas considered and rejected. Its closed condition is a diff that publishesnothing from any package, and this diff edits
packages/mcp/src/mcp-server-runtime.ts—a package source file, outside that list — regardless of what
distturns 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.
patchis the honest minimum: noruntime behaviour changes, no name was added, removed or reclassified, and the published
.d.tsis unchanged; what changes is a claim in shipped source that was not true.Verification
Gate union derived after merging
origin/mainand after the last edit, withscripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands, then run atdf33dd2111(the union and the final commit are the same tree):union named 34, ran 34, unreconciled 0— fromcomm -23 <(sort gates.list) <(sort ran.list), exact string comparison, no prefix orsubstring 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 isNOT MEASURED. ⛔ It is not a red, and there is nothing here to fix. Fix: pass a saved
turbo run testlog".pnpm check:dual-build-cjs-loads— exit 3: "PREREQUISITE NOT MET — this gate readsbuilt 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 workspacedependenc(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-debtsignals "closure not built, nothing measured" with exit 1, the code the repo reserves for a real finding #13885 — not addressedhere.)
Package-level, all exit codes captured before any pipe:
pnpm --filter @objectstack/mcp exec vitest run— 271 passed, 25 files, exit 0.pnpm --filter @objectstack/mcp run typecheck— exit 0.test code:
tsc --listFilesshows 402 files in that program and zero*.test.tsamong them, because the package tsconfig excludes them. The green says nothing about the
file this PR adds cases to.
TEST_DEBT['@objectstack/mcp'] = 53with the entry'sown 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.tsin both. 53 is exactly the ledgerednumber, which cross-checks the program against the gate's own recording. Control: 6
distinct
*.test.tsfiles appear in the diagnostics, so the test layer really is in theprogram. (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/nodefrom 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 '<pkg>^...' buildfails 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