fix(mcp): source the bridged tools' safety annotations from what they declare

[os-trump]

Fixes#13318

registerToolFromDefinition built both safety hints from the tool's name — membership tests against two literal sets in the same file (READ_ONLY_TOOLS, 6 names; DESTRUCTIVE_TOOLS, 1). Every bridged tool outside those seven literals therefore reached each MCP client as readOnlyHint: false, destructiveHint: false: not a missing annotation but a positive claim of "not read-only, and not destructive", asserted over every tool an app registers under its own name and every action-backed tool, on the one field a host reads to decide whether to interrupt the user before a call.

The source now read for each hint, and why

hint	source	why that one
destructiveHint	AIToolDefinition.requiresConfirmation (packages/spec/src/contracts/ai-service.ts)	The member is documented on itself as carried by action-backed tools "from the action's confirmation policy (action.ai.requiresConfirmation, or the destructive-action default)". git grep -n requiresConfirmation -- 'packages/mcp/src/**' returned only test fixtures and the list_actions summary type — the bridge never read it.
readOnlyHint	no declared source exists	AIToolDefinition has no member expressing "this tool only reads" (its members are name, label, description, parameters, category, outputSchema, objectName, requiresConfirmation). The six-name set really was the only thing that has ever populated it — question 2 of the card, answered by measurement, not recall.
openWorldHint	unchanged	Still false for every bridged tool. Out of scope here and noted at the bottom.

Reusing the one ruling rather than inventing a second.actionLooksDestructive (packages/runtime/src/action-execution.ts, ruling #7828 Option A) is the framework's definition of destructive, and requiresConfirmation is literally that function's output: summarizeAction fills the field by calling it. An MCP bridge never sees an action, and @objectstack/mcp does not depend on @objectstack/runtime, so the reuse this bridge owes the ruling is to read the verdict it is handed. The same verdict already travels to MCP on the other path, as requiresConfirmation on each list_actions entry.

Kept apart from the retired metadata key. The member read here is the runtime contract's AIToolDefinition.requiresConfirmation. ToolSchema.requiresConfirmation — the authorable metadata key ADR-0033 §2 retired — is untouched, still hard-rejects with its prescription, and is unreachable from this path; no metadata author can set the member read here. The retirement's rationale text (packages/spec/src/ai/tool.zod.ts, the ADR-0087 ledger, the upgrade guide) is deliberately not edited: it records why an authorable key was removed, and that reason is unchanged — its clause "the MCP bridge derives destructiveHint from a hardcoded name list" describes the state at retirement time, and rewriting a shipped retirement's rationale is the one edit most likely to read as the key coming back.

FROM → TO: what tools/list serves

the tool	FROM	TO
a built-in platform name (list_objects, describe_object, query_records, get_record, aggregate_data)	readOnlyHint: true, destructiveHint: false, openWorldHint: false	unchanged
the built-in delete_field	readOnlyHint: false, destructiveHint: true, openWorldHint: false	unchanged
an app-registered name declaring nothing (send_invoice_email)	readOnlyHint: false, destructiveHint: false, openWorldHint: false	openWorldHint: false only — both safety hints absent
an action-backed tool with requiresConfirmation: true (delete_opportunity, void_invoice, archive_account)	readOnlyHint: false, destructiveHint: false, openWorldHint: false	readOnlyHint: false, destructiveHint: true, openWorldHint: false
an action-backed tool with requiresConfirmation: false	readOnlyHint: false, destructiveHint: false, openWorldHint: false	destructiveHint: false, openWorldHint: false (no read-only claim)
a platform read name that declares requiresConfirmation: true	readOnlyHint: true, destructiveHint: false	readOnlyHint: false, destructiveHint: true — declared outranks name
aggregate_recordson this path	readOnlyHint: true, destructiveHint: false, openWorldHint: false	openWorldHint: false only (see the sets, below)

⚠️This is a change in what a published integration surface emits. Rows 3 and 4 make MCP hosts start prompting where they previously did not. That is the safe direction and the point of the card, but it is a behaviour change for every already-deployed client, which is why it ships with a changeset.

What a tool that declares nothing now gets, and why

Neither hint. Measured in the pinned SDK (@modelcontextprotocol/sdk 1.30.0, ToolAnnotationsSchema in dist/esm/types.js): readOnlyHint documents Default: false, destructiveHint documents Default: true. So the old false, false did not merely under-inform — it inverted the protocol's own conservative default. MCP has no spelling for "unknown" other than absence, so omitting hands the question back to that default ("may perform destructive updates") while claiming nothing this framework can source.

The omit-vs-assert-the-default fork was reached and is settled by that measurement rather than by taste: on any conforming host the two are the same verdict, and only omission avoids presenting this bridge's ignorance as a property of the tool. Asserting destructiveHint: true for unknown tools would be a one-line change if a maintainer prefers the louder form for non-conforming hosts; nothing else in the design depends on it.

The fate of the two literal sets

They survive, demoted: renamed PLATFORM_READ_ONLY_TOOL_NAMES / PLATFORM_DESTRUCTIVE_TOOL_NAMES, documented as a last-resort fallback for names the platform itself registers, and now outranked by anything the definition declares. What is gone is the else branch — the fabricated false, false for everything else. The asymmetry between keeping the read set and the destructive set is the MCP defaults' own: a missing readOnlyHint reads as "not read-only", the conservative answer, so omission there loses information but never safety, and the platform's own readers are the one place that information exists.

They are no longer folklore: a pin drives every name in PLATFORM_PROVIDED_TOOL_NAMES (@objectstack/spec/system) plus a set of strangers through the wire and asserts that only a name that registry contains may receive a hint it did not declare. aggregate_records failed that check and left the set — it was never a platform tool name (aggregate_data is). It belongs to the object-CRUD bridge, which registers it, annotated readOnlyHint: true, at its own site in mcp-http-tools.ts, so nothing loses that annotation where it is actually served.

The long-term shape is a declared read-only signal (or platform tools that declare requiresConfirmation), at which point the fallback can go. That is a contract decision, not this card.

Evidence

Every case drives a real StdioServerTransport and reads tools/list over newline-delimited JSON-RPC — the shape #13317 established for this call site — so what is asserted is what a client receives, not what a predicate returned. packages/mcp/src/mcp-tool-bridge-safety-annotations.test.ts, 8 cases.

Reverse verification (prediction first: the two controls stay green, the six behaviour cases go red). Fix committed, then packages/mcp/src/mcp-server-runtime.ts restored from origin/main and the mutation proven on disk by blob hash (fe7722fd5 fixed / 0c6b4d2ca mutated, matching origin/main's blob exactly), plus a marker count (safetyAnnotations 0 hits, isDestructiveTool 2 hits after the mutation):

Test Files 1 failed | 1 passed (2)
Tests 6 failed | 7 passed (13)
AssertionError: expected false to be undefined ← the defect, stated

The 7 that stayed green are the two controls in this file plus all five of #13317's input-schema pins, so the reds discriminate. Restore proven the same way: on-disk hash back to fe7722fd5, git diff HEAD empty, marker back to 1. No rebuild was needed for either leg — the pins import ./mcp-server-runtime.jsrelatively, so vitest resolves the source in the checkout, not a dist/ artifact (the package's vitest aliases touch only @objectstack/metadata-core and @objectstack/lint).

Runs, at d571c53f1:

• pnpm --filter @objectstack/mcp test → Test Files 25 passed (25) · Tests 266 passed (266), under the shared verify lock (VERDICT command-exit 0).
• pnpm --filter '@objectstack/mcp^...' build → VERDICT command-exit 0 (dependency closure, run before any judgement).
• pnpm --filter @objectstack/mcp typecheck → exit 0. ⚠️Scope, measured: this package's tsconfig.json excludes **/*.test.ts, and tsc --listFiles shows 0 test files in the program, so that green says nothing about the new pin. Stand-in measurement: the same program with tests included reports 53 raw errors — exactly the number TEST_DEBT['@objectstack/mcp'] records — with 0 of them naming the new test file or the edited source, so the ratchet cannot move on this card.
• pnpm lint (repo-wide eslint . --no-inline-config) → exit 0. Not narrowed.
• Gate families derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (28 matched + the convention-triggered kinds), all run: 30 green, including check:nul-bytes, check:cross-package-test-inputs, check:test-source-alias, check:engine-double-contract, check:where-matcher, check:type-check-coverage, check:dispatcher-error-vocabulary, check:published-files, check:undeclared-dep-imports, check-adr-0087-registration, check-empty-changeset, check-changeset-no-major.
• NOT MEASURED, not green — two gates refused their prerequisite (exit 3, their own text): scripts/check-test-completeness.mjs ("PREREQUISITE NOT MET — this gate grades a saved turbo run test log, and no log was named"), scripts/pm/check-half-states.mjs ("the token in the environment is not a valid GitHub credential"). Two more need a full workspace build this container did not run: pnpm check:dual-build-cjs-loads ("PREREQUISITE NOT MET — this gate reads built output") and the --re-measure half of pnpm check:type-check-debt ("49 workspace dependencies of the ledgered packages have no built type entry point on disk"); its structural half passed (check-type-check-coverage: OK), and the stand-in measurement above covers what the re-measure would have graded for this package. CI runs all four.

Not done here, on purpose

openWorldHint: false is still asserted for every bridged tool from no source at all — same class as the defect above, but the correct shape is not mechanical: the SDK documents Default: true, so simply omitting it would flip hosts to assuming an open world, which is neither obviously right nor obviously safe. Filed separately as #13350, which stays open and is not addressed here. outputSchema was deliberately left alone at this call site, per the measurement recorded on #13317.

---

Generated by Claude Code

[github-actions]

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/mcp, touching 13 documentable anchor(s).

4 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

• content/docs/ai/actions-as-tools.mdx(via destructiveHint (symbol))
• content/docs/ai/connect-mcp.mdx(via aggregate_records (literal))
• content/docs/ai/index.mdx(via aggregate_records (literal))
• content/docs/ai/natural-language-queries.mdx(via aggregate_records (literal))

⛔ 1 release-owned page(s) also name something this change touched. These are read-only:

• content/docs/releases/v15.mdx(via aggregate_records (literal))

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

What this run could not see

• the SDK route bridge reached 47 of 219 client-bound route-ledger rows — the other 172 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 172: 14 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
• 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 e8ff4274b4e16a06465f05271423a2cc87774475 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from 682369c497a1ef92e18624c269437416be22b5d9 — the merge of head d571c53f10d797a0f86c0d47f754ff96dc325979 into base e8ff4274b4e16a06465f05271423a2cc87774475, 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 682369c497a1ef92e18624c269437416be22b5d9 && git checkout 682369c497a1ef92e18624c269437416be22b5d9
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin e8ff4274b4e16a06465f05271423a2cc87774475 d571c53f10d797a0f86c0d47f754ff96dc325979 && git checkout -B drift-repro e8ff4274b4e16a06465f05271423a2cc87774475 && git merge --no-ff d571c53f10d797a0f86c0d47f754ff96dc325979
node scripts/docs-audit/affected-docs.mjs --json e8ff4274b4e16a06465f05271423a2cc87774475

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

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs e8ff4274b4e16a06465f05271423a2cc87774475 → pass the list as
args.docs, on the commit named under Which tree this was computed on.

[os-trump]

✅ PM review — ACCEPT once CI is green; ⛔ not while it is running (#13318)

Undrafting now; arm follows on a complete green read. ⛔ No rework owed. Clause ②: does not attach — 3 files, none under packages/spec/src/**; MCP's tools/list is a protocol surface, not a public REST door (same ruling as #13317, kept consistent deliberately).

⚖️ The fork is RULED: option A (omit), as shipped. Ship it.

The dispatch said that if the work landed on a genuine fork between omit the hint and assert the MCP spec default, it should stop and report rather than pick silently. It did exactly that. Grading it here rather than handing it back.

Ruling: A. Reasons, in order of weight:

1. ⭐ B commits the same category error this card exists to fix, only in a nicer direction. The card's own closing principle is that an annotation asserted from a name allowlist should stop being presented as a property of the tool. Asserting destructiveHint: true for a tool that declared nothing still presents this bridge's ignorance as the tool's property — it just does so safely. A fixes the category; B fixes only the sign.
2. On a conforming host they are the same verdict. Measured in the pinned SDK: ToolAnnotationsSchema documents destructiveHint as Default: true. So B buys nothing wherever the protocol is honoured.
3. A preserves information B destroys — a consumer can distinguish "the framework knows this needs confirmation" from "the framework does not know". That distinction is precisely what lets a later card close the remaining gap; B erases the evidence that a gap exists.

⚠️The residual risk, stated rather than waved away: on a host that ignores the protocol's documented default and reads a missingdestructiveHint as "safe", B would be strictly safer. I am accepting that risk. Such a host is violating the default its own protocol documents, and buying protection from it by destroying real information on every conforming host is the wrong trade. ⭐ Worth recording that both A and B are large improvements over the asserted false that shipped before — the argument here is between two right answers.

⛔ Do not re-open this without new evidence about real non-conforming hosts. It is a one-line change in safetyAnnotations if that evidence appears.

✅ Fences held, verified from the diff

• [P2] Execute ADR-0033's deletion of the unenforced tool.requiresConfirmation (safety-shaped, gated by nothing) #3715 separation ✅. The retired metadata key ToolSchema.requiresConfirmation is untouched, still hard-rejects, and is unreachable from this path; what is read is AIToolDefinition.requiresConfirmation, the runtime contract member on a different object at a different layer. Both the docblock and the changeset say so explicitly, which is what I asked for — the separation had to hold in the prose too.
• No second definition of destructive ✅ — and ⭐ better than compliance, the dev showed one could not be reused directly: packages/mcp does not depend on @objectstack/runtime, and an MCP bridge never sees an action. So the reuse the ruling is owed is to read the verdict it is handed — requiresConfirmation is literally actionLooksDestructive's output, because summarizeAction fills that field by calling it. The actionLooksDestructive keys destructiveness off confirmText — the exact signal #7278/#7309 are migrating away, and 6 of 14 actions flip #7828 ruling is reused transitively through the field, which is the only reuse available.
• outputSchema not touched ✅.

What makes this a good change beyond the fix

• ⭐ The name sets survive DEMOTED and fenced, not deleted and not trusted. Renamed to PLATFORM_READ_ONLY_TOOL_NAMES / PLATFORM_DESTRUCTIVE_TOOL_NAMES, documented as a last-resort fallback for names the platform itself registers, outranked by anything declared — and pinned from outside the module (the sets are private) to be a subset of PLATFORM_PROVIDED_TOOL_NAMES. That invariant is what stops the fallback drifting back into folklore, which is the failure mode the card describes.
• ⭐ A real correction found in passing: aggregate_records left the read-only set because it was never a platform tool name (aggregate_data is). It belongs to the object-CRUD bridge, which registers it annotated readOnlyHint: true at its own site in mcp-http-tools.ts — so nothing loses the annotation where it is actually served, and the test pins PLATFORM_PROVIDED_TOOL_NAMES.has('aggregate_records') === false.
• The requiresConfirmation: false arm is careful: it yields destructiveHint: false and noreadOnlyHint — refusing to infer "only reads" from "needs no confirmation". A sloppier fix would have claimed both.
• ⭐ The wire harness is duplicated from fix(mcp): the tool bridge forwards AIToolDefinition.parameters as the tool's input schema, and the docblock stops describing a workaround that was never implemented #13317's pin on purpose, with the reason stated: "a pin that exists to observe the wire should not be able to go green because a sibling pin's helper changed." Normally I would flag copy-paste; here it is the right call and it is argued.
• Reverse verification used the previous card's pins as controls — the 7 green under mutation are this file's 2 controls plus all 5 of fix(mcp): the tool bridge forwards AIToolDefinition.parameters as the tool's input schema, and the docblock stops describing a workaround that was never implemented #13317's input-schema pins, so the 6 reds discriminate. Mutation proven by blob hash equal byte-for-byte to origin/main's blob, restore proven the same way with git diff HEAD empty.
• Typecheck recorded PARTIAL, not green-by-omission: the package tsconfig excludes **/*.test.ts and tsc --listFiles reports 0 test files, so a stand-in run with tests included was done — 53 raw errors, exactly TEST_DEBT['@objectstack/mcp'], none naming the new or edited files. Four gates recorded NOT MEASURED with their own refusal text.

On the retirement rationale left unedited — endorsed, with one nuance

The #3715 retirement prose carries the clause "the MCP bridge, which derives destructiveHint from a hardcoded name list", and the dev deliberately did not edit it, on the grounds that rewriting a shipped retirement's rationale is the change most likely to read as the key coming back.

✅ I endorse that, and add the nuance that makes it comfortable: after this PR that clause is imprecise rather than false — the name list still exists, demoted to a platform-name fallback. And the sentence's actual load-bearing claim (the retired key is read by nothing) is untouched and still exactly true. ⛔ Leave it alone.

Follow-up triaged

#13350 (bug · p2 · pm:queue · domain:cli) — the same annotations literal still asserts openWorldHint: false on every bridged tool from no declared source. ⭐ Correctly not folded in, and the reason is the valuable part: the repair is not the same shape. The SDK documents openWorldHint as Default: true, so omitting it would flip conforming hosts to assuming an open world — the non-conservative direction, the opposite of what omission achieves for destructiveHint. And mcp-http-tools.ts already asserts true for run_action and false for the object reads, with nothing carrying that distinction across the AIToolDefinition seam. ⛔ Do not treat this PR as its template.

---

Generated by Claude Code