Skip to content

chore(server): migrate McpServer.tool() → registerTool() (#705)#763

Merged
bokelley merged 5 commits into
mainfrom
bokelley/register-tool
Apr 22, 2026
Merged

chore(server): migrate McpServer.tool() → registerTool() (#705)#763
bokelley merged 5 commits into
mainfrom
bokelley/register-tool

Conversation

@bokelley

Copy link
Copy Markdown
Contributor

Summary

Replaces every use of the MCP SDK's deprecated .tool() overload with registerTool(name, config, handler) across src/ and examples/. Behavior is unchanged. Closes #705.

Scope

Real call sites migrated:

  • src/lib/server/create-adcp-server.ts — AdcpToolMap loop + get_adcp_capabilities. annotations now declared at register time instead of via post-registration .update().
  • src/lib/server/test-controller.ts + src/lib/testing/comply-controller.tscomply_test_controller.
  • src/lib/testing/stubs/governance-agent-stub.ts — 5 sites.
  • examples/error-compliant-server.ts — canonical seller template.

Doc comments / JSDoc updated across responses.ts, errors.ts, serve.ts, adcp-server.ts, test-controller.ts, comply-controller.ts, tool-request-schemas.ts, and index.ts.

Verification: grep -r 'server\.tool(\|mcp\.tool(' src/ examples/ is empty.

outputSchema deliberately not wired on framework tools

The issue asks for outputSchema on every AdCP tool. While investigating I found a hard blocker in the current MCP SDK:

The client-side callTool validates structuredContent against outputSchema regardless of isError — see node_modules/@modelcontextprotocol/sdk/dist/esm/client/index.js:504. Our adcpError() envelope carries structuredContent: { adcp_error: {...} } with isError: true, which would fail every client-side outputSchema check.

The server-side validator at mcp.js:193 correctly skips on isError; the client-side path does not. Until the SDK gates the client check symmetrically, wiring outputSchema would turn every AdCP error into a client validation failure.

What covers drift in the meantime:

Test plan

  • npm test — 5164 pass / 0 fail
  • npm run format:check clean
  • Two new regression tests under customTools:
    • registerTool passes annotations through to the SDK tool definition — fails loudly if someone reverts to .tool() (no annotations param)
    • customTools with outputSchema store the schema on the SDK tool definition — proves the config-object plumbing still reaches the SDK
  • comply-controller.test.js fake server updated to the registerTool shape
  • Post-merge: confirm the upstream SDK issue before re-opening the outputSchema question

Follow-up

  • Consider filing an upstream MCP SDK issue to gate client-side outputSchema validation on !isError. If fixed upstream, we can wire outputSchema in a follow-up PR.

🤖 Generated with Claude Code

)

Replaces every use of the deprecated MCP SDK `.tool()` overload with
the supported `registerTool(name, config, handler)` form:

- src/lib/server/create-adcp-server.ts — AdcpToolMap loop and
  get_adcp_capabilities; annotations now declared at register time
  instead of via post-registration .update()
- src/lib/server/test-controller.ts + src/lib/testing/comply-controller.ts
  — comply_test_controller registration
- src/lib/testing/stubs/governance-agent-stub.ts — 5 governance tools
- examples/error-compliant-server.ts — canonical seller template
- JSDoc / prose comments updated to the new form

Deliberately skips `outputSchema` on framework-registered tools: the
MCP SDK's client-side callTool validates structuredContent against
outputSchema regardless of isError
(@modelcontextprotocol/sdk/dist/esm/client/index.js:504). Our
adcpError() envelope carries structuredContent + isError: true, which
would fail every client validation. Dispatcher-level AJV validation
(#727) covers response drift; customTools may opt into SDK validation
explicitly via customTools[*].outputSchema (covered by regression test).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley and others added 3 commits April 21, 2026 23:15
Upstream refresh adds a universal-macros note to `URLAsset.url` and
splits SI `context` into `intent` (natural-language handoff) plus
`context` (protocol envelope). Updates:

- src/lib/types/{core,tools,schemas}.generated.ts — regenerated
- src/lib/adapters/si-session-manager.ts — reads request.intent
  instead of request.context; renames the inner parameter to match
- docs/{llms.txt,TYPE-SUMMARY.md} — regenerated

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
# Conflicts:
#	docs/llms.txt
#	src/lib/adapters/si-session-manager.ts
#	src/lib/types/core.generated.ts
#	src/lib/types/schemas.generated.ts
…tive-agent SKILL

Expert review feedback (PR #763):
- security-reviewer M1 / code-reviewer Consider: document on
  `AdcpCustomToolConfig.outputSchema` that a too-strict schema silently
  breaks client-side validation for every buyer, and cross-reference the
  framework-wide rationale block
- debugger Nit: skill copy-paste examples for manual `preview_creative`
  registration now use `server.registerTool(...)` instead of the
  deprecated `server.tool(...)` so new agents land on the non-deprecated
  API. The other skill docs that mention `server.tool()` are anti-pattern
  call-outs ("don't use X") and stay as-is.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley

Copy link
Copy Markdown
Contributor Author

Ran through code-reviewer, security-reviewer, and debugger. All three said ship.

code-reviewer (APPROVE):

  • Migration correct at every site, handler type-casts acceptable
  • outputSchema skip is the right call; consider documenting footgun in CustomToolDefinition JSDoc ✅ addressed in cd0bd44
  • Regression tests sufficient
  • Merge conflict resolution (intent: string > main's unknown) correct

security-reviewer (Ship, 0 Must Fix):

  • H: none
  • M1: customTools[*].outputSchema can brick a seller's own tool with a too-strict schema → ✅ documented in cd0bd44 (JSDoc now names the footgun and points at the framework-wide rationale)
  • L1: registerTestController / createComplyController.register don't hard-gate on NODE_ENV=production — pre-existing; follow-up
  • L2: governance-agent-stub ships in the tarball (inert until start() with internally-generated token) — pre-existing; follow-up
  • Positive finding: annotations-at-registration (vs post-hoc .update()) closes a window where tools/list could observe a tool without its readOnlyHint / destructiveHint. get_adcp_capabilities now correctly advertises readOnlyHint: true.

debugger (No runtime regressions):

  • All _registeredTools consumers already run post-registration; timing is unchanged
  • Handler type-casts align with SDK's ToolCallback signature at runtime
  • intent: string is required per generated SIInitiateSessionRequest — safe
  • No orphaned { tool }-shaped test fakes
  • Nit: skill docs copy-paste examples for manual preview_creative registration still used server.tool(...) → ✅ updated in cd0bd44 (skills/build-creative-agent/SKILL.md lines 101, 137, 289)

Follow-ups not in this PR:

  • L1 (compliance tool prod-gate)
  • L2 (test-stub tarball scope)
  • Upstream MCP SDK: gate client-side outputSchema validation on !isError (would let us wire outputSchema on every framework tool)

Reviewer spot-check caught holdouts outside src/ and examples/:

- scripts/generate-agent-docs.ts — the server-quick-start example
  emitted into docs/llms.txt still showed `server.tool(...)`. Every
  agent reading the reference docs was being pointed at the deprecated
  API. Now emits `server.registerTool(name, { description, inputSchema },
  handler)`.
- test/oauth-client-credentials-integration.test.js
- test/request-signing-agent-integration.test.js (4 sites)
- test/lib/serve-tasks.test.js (3 sites)
- test/lib/mcp-tasks-compat.test.js (2 sites)

test-agents/** was grep-clean already. Verification:
`grep -R 'server\.tool(\|mcp\.tool(' src/ examples/ test/ test-agents/ scripts/ bin/`
returns empty.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley

Copy link
Copy Markdown
Contributor Author

Reviewer spot-check items landed.

1. test-agents/** etc.test-agents/ was grep-clean already, but found holdouts in:

  • scripts/generate-agent-docs.ts:492 — most importantly, the server-quick-start example emitted into docs/llms.txt still pointed agents at the deprecated server.tool(...). Every AI agent reading the reference docs was being pointed at the old API. Fixed.
  • test/oauth-client-credentials-integration.test.js, test/request-signing-agent-integration.test.js (4 sites), test/lib/serve-tasks.test.js (3 sites), test/lib/mcp-tasks-compat.test.js (2 sites)

Landed in e4f6ef9. Verification:

grep -R 'server\.tool(\|mcp\.tool(' src/ examples/ test/ test-agents/ scripts/ bin/

empty. docs/llms.txt regenerated and is also clean.

2. Upstream SDK issue filed: modelcontextprotocol/typescript-sdk#1943. Two-line fix — add !result.isError guard on the client-side outputSchema validator to match the server. Offered to send the PR if they want it.

If they merge, the regression test in test/server-create-adcp-server.test.js already proves our registerTool(..., { outputSchema }) plumbing is wired — we can then flip on outputSchema for framework tools in a small follow-up PR without any further changes to this migration.

Tests: 5179 pass / 0 fail. Prettier clean.

@bokelley
bokelley merged commit 6ae3169 into main Apr 22, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

tech-debt: migrate MCP tool registrations from deprecated tool() to registerTool()

1 participant