The MCP tool bridge derives `destructiveHint` / `readOnlyHint` from a hardcoded tool-name set, so every app-registered and action-backed tool is advertised as neither read-only nor destructive

[os-trump]

Found while fixing #13271 (the same call site, a different dropped key). Filed rather than folded: #13271's scope is parameters only, and this touches the safety annotations, which are a separately-gradeable behaviour change on the same published surface.

Measured at a876ebe6

packages/mcp/src/mcp-server-runtime.ts, registerToolFromDefinition, passes the SDK an annotations object built entirely from the tool's name:

annotations: {
  destructiveHint: this.isDestructiveTool(tool.name),
  readOnlyHint: this.isReadOnlyTool(tool.name),
  openWorldHint: false,
},

Both predicates are membership tests against two literal sets at the top of the same file:

const READ_ONLY_TOOLS = new Set([
  'list_objects', 'describe_object', 'query_records',
  'get_record', 'aggregate_records', 'aggregate_data',
]);
const DESTRUCTIVE_TOOLS = new Set(['delete_field']);

⇒ a bridged tool whose name is not one of those seven literals is served to every MCP client as readOnlyHint: false, destructiveHint: false — "not read-only, and not destructive", the least informative and most permissive combination the annotation pair can express. That is every tool an app registers under its own name, and every action-backed tool (delete_opportunity, void_invoice, archive_account, …).

This matters because destructiveHint is not decoration: it is the field an MCP host reads to decide whether to interrupt the user before a call. A destructive action-backed tool currently arrives flagged as safe.

Why it reads as a defect rather than a design choice

The definition the bridge is handed already carries a destructiveness signal it does not read. AIToolDefinition in packages/spec/src/contracts/ai-service.ts:

  /**
   * Whether invoking this tool requires human-in-the-loop confirmation.
   * Action-backed tools set this from the action's confirmation policy
   * (`action.ai.requiresConfirmation`, or the destructive-action default).
   */
  requiresConfirmation?: boolean;

git grep -n requiresConfirmation -- 'packages/mcp/src/**' returns nothing, against tool.name (5 sites) and tool.description (1 site) live in the same file — the same reverse-check shape #13271 used, and the same real absence.

Meanwhile the framework has a maintainer-ruled definition of what "destructive" means for an action, in packages/runtime/src/action-execution.ts:

export function actionLooksDestructive(_deps: ActionExecutionDeps, action: any): boolean {
    if (action?.ai?.requiresConfirmation !== undefined) return Boolean(action.ai.requiresConfirmation);
    return Boolean(action?.mode === 'delete' || action?.variant === 'danger');
}

whose docblock records the ruling behind it (#7828, Option A: mode: 'delete' / variant: 'danger' are the closed, declared signals; confirmText deliberately is not). So the repo has one declared answer to this question and the MCP bridge asks a different one — a seven-name allowlist that no metadata author can extend.

⛔ Not a re-litigation of #3715

#3715 removed ToolSchema.requiresConfirmation — the metadata key — under ADR-0033, because nothing enforced it. That ruling stands and nothing here asks for it back. The key named above is AIToolDefinition.requiresConfirmation, the runtime contract member, which #3715 did not touch and which is still declared and documented as carried by action-backed tools. Any fix must keep those two apart.

What a fix would need to decide

1. Whether the bridge should read AIToolDefinition.requiresConfirmation for destructiveHint, reuse actionLooksDestructive's ruling, or take a third source — and what a tool that declares nothing should then be annotated as. Note the MCP spec's own defaults are readOnlyHint: false and destructiveHint: **true**, so the current "declare nothing" path is not merely uninformative, it inverts the protocol's own conservative default.
2. Whether readOnlyHint has any declared source at all today, or whether the six-name set is the only thing that has ever populated it — in which case the honest interim shape may be to omit the hint rather than assert false.
3. Whether the two literal sets should survive as a fallback for the built-in names or be deleted once a declared source exists.

Either way, an annotation asserted from a name allowlist should stop being presented as a property of the tool.

[os-trump]

Claim: domain:cli PM seat · session session_01TvqBFLRzXdSPcbusDoED9k · branch claude/issue-13318-mcp-tool-safety-annotations

Serial constraints cleared: ✅ yes, as of a few minutes ago. packages/mcp/src/mcp-server-runtime.ts was under a same-file hard serial held by PR #13317 (card #13271, the parameters half of this same call site). #13317 merged at 04:42:09Z, so under ruling ① the serial is released. ⚠️ Branch from a main that contains it — that PR rewrote the docblocks and the registerTool call you are about to edit, and added toolInputSchema.

security is a topic marker, ⛔ not a verdict. No access-control boundary is crossed by the framework: destructiveHint is advisory, and a host decides what to do with it. What earns the p1 is that the advice is wrong in the unsafe direction, on every action-backed tool, and inverts the MCP spec's own conservative default.

⛔ Fences

• ⛔ Do not re-litigate [P2] Execute ADR-0033's deletion of the unenforced tool.requiresConfirmation (safety-shaped, gated by nothing) #3715. It removed ToolSchema.requiresConfirmation, the metadata key, under ADR-0033. That ruling stands and is not being asked back. The member this card names is AIToolDefinition.requiresConfirmation, the runtime contract member [P2] Execute ADR-0033's deletion of the unenforced tool.requiresConfirmation (safety-shaped, gated by nothing) #3715 never touched, still declared and documented. Keep the two apart in the diff and in the prose.
• ⛔ Do not invent a second definition of "destructive." The repo already has one, maintainer-ruled: actionLooksDestructive in packages/runtime/src/action-execution.ts, whose docblock records actionLooksDestructive keys destructiveness off confirmText — the exact signal #7278/#7309 are migrating away, and 6 of 14 actions flip #7828 Option A (mode: 'delete' / variant: 'danger' are the closed declared signals; confirmText deliberately is not). Reuse the ruling; a third answer in a third place is the defect this card is about, one level up.
• ⛔ Do not plumb outputSchema while you are in this call site. Measured on 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: validateToolOutput() throws "Tool X has an output schema but no structured content was provided" whenever an outputSchema is declared and the result carries none, and this bridge returns text content only — it would break every bridged tool. ⛔ Not a template.

⚠️ The one thing that may need to come back to me

The card's question 1 has a tail: what should a tool that declares nothing be annotated as? There is a strong non-novel answer — the MCP spec's own defaults are readOnlyHint: false and destructiveHint: **true**, so conforming to the protocol is not a fresh decision. And question 2's honest shape may be to omit a hint rather than assert a false nothing sources.

⇒ Dispatched to measure and implement the declared source, and to stop asserting values that have no source. But if the work lands on a genuine fork between omit the hint and assert the spec default — one that measurement cannot settle — ⛔ stop and report rather than picking silently. Flipping tools from destructiveHint: false to true/omitted makes hosts start prompting where they did not; that is the safe direction, but it is still a change in what a published integration surface emits and must be stated as one.

⭐ Provenance worth keeping: found by the #13271 dev while fixing the same call site, and filed rather than folded because the safety annotations are a separately-gradeable behaviour change. Its zero was reverse-checked the same way #13271's was — git grep -n requiresConfirmation -- 'packages/mcp/src/**' returns nothing, against tool.name (5 sites) and tool.description (1) live in the same file.

---

Generated by Claude Code

[os-trump]

os-dev-report

{
  "issue": 13318,
  "status": "done",
  "branch": "claude/issue-13318-mcp-tool-safety-annotations",
  "pr": "https://github.com/objectstack-ai/objectstack/pull/13351",
  "premise_still_valid": true,
  "summary": "registerToolFromDefinition now sources destructiveHint from AIToolDefinition.requiresConfirmation — the runtime-contract member that already carries actionLooksDestructive's ruling, because summarizeAction fills that field by calling it — and omits any hint it cannot source instead of asserting a fabricated false. The #3715 separation held: the retired metadata key ToolSchema.requiresConfirmation is untouched, still hard-rejects, and is unreachable from this path; its retirement rationale text was deliberately not edited, since rewriting a shipped retirement's prose is the edit most likely to read as the key coming back. No second definition of destructive was invented and none could be: packages/mcp does not depend on @objectstack/runtime and an MCP bridge never sees an action, so reuse means reading the verdict it is handed. The two literal sets survive DEMOTED — renamed PLATFORM_READ_ONLY_TOOL_NAMES / PLATFORM_DESTRUCTIVE_TOOL_NAMES, documented as a fallback for names the platform itself registers, outranked by anything declared, and pinned from outside the module to be a subset of PLATFORM_PROVIDED_TOOL_NAMES; aggregate_records left the read set because it was never a platform tool name and is annotated at its real registration site in mcp-http-tools.ts. FROM to TO: built-in platform names unchanged; a declare-nothing app-registered tool goes from {readOnlyHint:false, destructiveHint:false} to BOTH ABSENT (the MCP defaults then apply — readOnlyHint false, destructiveHint TRUE); an action-backed tool declaring requiresConfirmation:true goes from destructiveHint:false to destructiveHint:true with readOnlyHint:false; requiresConfirmation:false yields destructiveHint:false with no read-only claim. That makes hosts start prompting where they did not — the safe direction, stated as a behaviour change on a published surface in both the PR body and the changeset.",
  "tests": "All at HEAD d571c53f1 (branch pushed, worktree clean at that commit). (1) pnpm --filter '@objectstack/mcp^...' build under scripts/pm/os-verify-lock.sh — 'VERDICT command-exit 0', run before any judgement. (2) New wire pin packages/mcp/src/mcp-tool-bridge-safety-annotations.test.ts, 8 cases, each driving a real StdioServerTransport over newline-delimited JSON-RPC and reading tools/list, so the assertions are on what a client receives, not on a predicate's return. With #13317's sibling pin: 'Test Files 2 passed (2) / Tests 13 passed (13)', VERDICT command-exit 0. (3) Full package suite pnpm --filter @objectstack/mcp test — 'Test Files 25 passed (25) / Tests 266 passed (266)', VERDICT command-exit 0. (4) REVERSE VERIFICATION, direction predicted first (two controls green, six behaviour cases red): fix committed FIRST, then mcp-server-runtime.ts restored from origin/main. Mutation proven ON DISK by blob hash, not by an exit code — disk fe7722fd5e963930ca5f5d18087e5c989f6bc6bc (equal to HEAD's blob) became 0c6b4d2ca7fce75665c8bcc2325916c42c224628 (equal byte-for-byte to origin/main's blob), with marker counts safetyAnnotations 0 and isDestructiveTool 2 after the mutation. Result: 'Tests 6 failed | 7 passed (13)', the headline failure being 'AssertionError: expected false to be undefined' — the defect stated. The 7 green are this file's two controls plus all five #13317 input-schema pins, so the reds discriminate. RESTORE leg proven the same way and not by an exit code: git checkout HEAD -- ABSOLUTE_PATH (never a bare checkout), hash back to fe7722fd5, git diff HEAD empty, marker back to 1; a trap with absolute paths was armed for the whole run. NO REBUILD was needed on either leg and this is measured, not assumed: the pins import ./mcp-server-runtime.js RELATIVELY, so vitest resolves the checkout's source rather than dist/, and this package's vitest aliases touch only @objectstack/metadata-core and @objectstack/lint. (5) pnpm --filter @objectstack/mcp typecheck exit 0, but recorded as PARTIAL: the package tsconfig excludes **/*.test.ts and tsc --listFiles reports 0 test files in the program (mcp-server-runtime.ts: 1), so that green says NOTHING about the new pin. Stand-in measurement with tests included: 53 raw errors, exactly the number TEST_DEBT['@objectstack/mcp'] records, and 0 of them name the new test file or the edited source. (6) pnpm lint, the repo-wide 'eslint . --no-inline-config', exit 0 — not narrowed, so no narrowing needs justifying. (7) Gate families derived with node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack (28 matched plus the convention-triggered kinds, derived from the real change set, not recalled) and re-run at the final commit: 30 green, exit codes captured before any pipe. NOT MEASURED, never folded into that green list: scripts/check-test-completeness.mjs exit 3 'PREREQUISITE NOT MET — this gate grades a saved turbo run test log, and no log was named'; scripts/pm/check-half-states.mjs exit 3 'PREREQUISITE NOT MET — the token in the environment is not a valid GitHub credential'; pnpm check:dual-build-cjs-loads exit 3 'PREREQUISITE NOT MET — this gate reads built output, and some package has no dist/'; the --re-measure half of pnpm check:type-check-debt refused with '49 workspace dependencies of the ledgered packages have no built type entry point on disk' (its structural half printed 'check-type-check-coverage: OK'). CI runs all four.",
  "mcp_calls": "5 — 2 search_issues (one of which returned #13318 itself, the known-hit control proving search was not silently zeroed this session), 1 issue_write, 1 create_pull_request, 1 add_issue_comment. Card body, all comments, and the PR read-back went through the zero-quota public payload/page channel; repo-scoped REST is 403 on this seat (probed first).",
  "open_questions": [
    {
      "question": "For a bridged tool that declares nothing, should the bridge OMIT destructiveHint (shipped) or ASSERT destructiveHint: true? Reached the fork the dispatch flagged; measurement settled it far enough to ship, but it is a one-line reversal if you prefer the louder form.",
      "options": [
        "A (shipped) — omit. On any conforming host this IS destructiveHint: true, because @modelcontextprotocol/sdk 1.30.0's ToolAnnotationsSchema documents 'Default: true' for it; MCP has no spelling for 'unknown' other than absence, so omission is the only way to avoid presenting this bridge's ignorance as a property of the tool.",
        "B — assert destructiveHint: true. Identical on a conforming host, strictly louder on a host that ignores protocol defaults, but it makes a positive claim about a tool nothing here has measured and erases the difference between 'declared destructive' and 'unknown'."
      ],
      "recommendation": "A, because the two are the same verdict wherever the protocol is honoured, and only A keeps the card's own principle — stop presenting an unsourced value as a property of the tool. Flip to B with one line in safetyAnnotations if you weigh non-conforming hosts higher."
    }
  ],
  "out_of_scope_findings": [
    "filed as #13350: the same annotations literal asserts openWorldHint: false on EVERY bridged tool from no declared source — same class as this card, but deliberately not folded in because the repair is not mechanical (the SDK documents openWorldHint 'Default: true', so omitting it flips conforming hosts to assuming an OPEN world, which is not the conservative direction; mcp-http-tools.ts already asserts true for run_action and false for the object reads, and nothing carries that distinction across the AIToolDefinition seam).",
    "already filed by another seat as #13345, no duplicate opened: packages/spec/liveness/tool.json's `parameters` row still asserts the bridge registers tools with NO inputSchema, which merged PR #13317 falsified. Found while reading that ledger for the readOnlyHint question; confirmed open by a targeted search rather than filed again.",
    "NOT filed, deliberate, stated in the PR body instead: the #3715 retirement rationale (packages/spec/src/ai/tool.zod.ts, the ADR-0087 ledger entry, spec-changes.json, docs/protocol-upgrade-guide.md) carries the clause 'the MCP bridge, which derives destructiveHint from a hardcoded name list'. That describes the state at retirement time and its actual claim — the RETIRED metadata key is read by nothing — remains exactly true after this PR. Editing a shipped retirement's rationale is the change most likely to read as the key coming back, so it was left alone by design rather than overlooked."
  ]
}

---

Generated by Claude Code