-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Bundle automatic validator defaults in client/server shims #2088
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
mattzcarey
wants to merge
3
commits into
main
Choose a base branch
from
fix/server-cfworker-json-schema-shim
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| --- | ||
| '@modelcontextprotocol/core': minor | ||
| '@modelcontextprotocol/client': patch | ||
| '@modelcontextprotocol/server': patch | ||
| --- | ||
|
|
||
| Bundle automatic JSON Schema validator defaults in `@modelcontextprotocol/client` and `@modelcontextprotocol/server` runtime shims. | ||
|
|
||
| Client/server select defaults automatically based on the runtime: Node shims use AJV, while browser/workerd shims use `@cfworker/json-schema`. Those backends are bundled into the shim chunks that select them, so consumers do not need to install validator packages or import explicit validators for default behavior. Advanced users can still pass their own `jsonSchemaValidator` interface implementation. | ||
|
|
||
| The `@modelcontextprotocol/{client,server}/validators/cf-worker` subpath export has been removed — there is no longer any public entry point for the SDK's built-in validator classes. `AjvJsonSchemaValidator` and `CfWorkerJsonSchemaValidator` are now `@internal` and no longer exported from `@modelcontextprotocol/client` or `@modelcontextprotocol/server` (not even as types). The `jsonSchemaValidator` interface remains the public extension point for custom validators, and example JSDoc snippets no longer demonstrate direct validator instantiation. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
110 changes: 110 additions & 0 deletions
110
packages/client/test/client/jsonSchemaValidatorOverride.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| import type { JSONRPCMessage, JsonSchemaType, JsonSchemaValidatorResult, jsonSchemaValidator } from '@modelcontextprotocol/core'; | ||
| import { InMemoryTransport, LATEST_PROTOCOL_VERSION } from '@modelcontextprotocol/core'; | ||
| import { Client } from '../../src/client/client.js'; | ||
| import { fromJsonSchema } from '../../src/fromJsonSchema.js'; | ||
|
|
||
| class RecordingValidator implements jsonSchemaValidator { | ||
| schemas: JsonSchemaType[] = []; | ||
| values: unknown[] = []; | ||
|
|
||
| getValidator<T>(schema: JsonSchemaType) { | ||
| this.schemas.push(schema); | ||
| return (value: unknown): JsonSchemaValidatorResult<T> => { | ||
| this.values.push(value); | ||
| return { valid: true, data: value as T, errorMessage: undefined }; | ||
| }; | ||
| } | ||
| } | ||
|
|
||
| async function connectInitializedClient(client: Client) { | ||
| const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair(); | ||
| serverTransport.onmessage = async message => { | ||
| if ('method' in message && 'id' in message && message.method === 'initialize') { | ||
| await serverTransport.send({ | ||
| jsonrpc: '2.0', | ||
| id: message.id, | ||
| result: { | ||
| protocolVersion: LATEST_PROTOCOL_VERSION, | ||
| capabilities: { tools: {} }, | ||
| serverInfo: { name: 'test-server', version: '1.0.0' } | ||
| } | ||
| }); | ||
| } else if ('method' in message && 'id' in message && message.method === 'tools/list') { | ||
| await serverTransport.send({ | ||
| jsonrpc: '2.0', | ||
| id: message.id, | ||
| result: { | ||
| tools: [ | ||
| { | ||
| name: 'structured-tool', | ||
| description: 'A tool with structured output', | ||
| inputSchema: { type: 'object' }, | ||
| outputSchema: { | ||
| type: 'object', | ||
| properties: { count: { type: 'number' } }, | ||
| required: ['count'] | ||
| } | ||
| } | ||
| ] | ||
| } | ||
| } satisfies JSONRPCMessage); | ||
| } | ||
| }; | ||
|
|
||
| await Promise.all([client.connect(clientTransport), serverTransport.start()]); | ||
| return { clientTransport, serverTransport }; | ||
| } | ||
|
|
||
| describe('client JSON Schema validator overrides', () => { | ||
| test('Client constructor uses a custom validator for tool output schema caching', async () => { | ||
| const validator = new RecordingValidator(); | ||
| const client = new Client( | ||
| { name: 'test-client', version: '1.0.0' }, | ||
| { | ||
| capabilities: {}, | ||
| jsonSchemaValidator: validator | ||
| } | ||
| ); | ||
| const { clientTransport, serverTransport } = await connectInitializedClient(client); | ||
|
|
||
| await expect(client.listTools()).resolves.toMatchObject({ | ||
| tools: [ | ||
| { | ||
| name: 'structured-tool', | ||
| outputSchema: { | ||
| type: 'object', | ||
| properties: { count: { type: 'number' } }, | ||
| required: ['count'] | ||
| } | ||
| } | ||
| ] | ||
| }); | ||
|
|
||
| expect(validator.schemas).toEqual([ | ||
| { | ||
| type: 'object', | ||
| properties: { count: { type: 'number' } }, | ||
| required: ['count'] | ||
| } | ||
| ]); | ||
|
|
||
| await client.close(); | ||
| await clientTransport.close(); | ||
| await serverTransport.close(); | ||
| }); | ||
|
|
||
| test('fromJsonSchema uses an explicitly supplied custom validator', async () => { | ||
| const validator = new RecordingValidator(); | ||
| const schema: JsonSchemaType = { | ||
| type: 'object', | ||
| properties: { name: { type: 'string' } }, | ||
| required: ['name'] | ||
| }; | ||
|
|
||
| const standardSchema = fromJsonSchema<{ name: string }>(schema, validator); | ||
| expect(standardSchema['~standard'].validate({ name: 123 })).toEqual({ value: { name: 123 } }); | ||
|
|
||
| expect(validator.schemas).toEqual([schema]); | ||
| expect(validator.values).toEqual([{ name: 123 }]); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 The migration docs added here say consumers should not install
@cfworker/json-schema, buttest/integration/test/server/cloudflareWorkers.test.ts:46— the only end-to-end test that runs the packed server tarball under real workerd — still pre-installs'@cfworker/json-schema': '^4.1.1'in the generated consumer package.json. That dep is now dead weight after this PR adds@cfworker/json-schematonoExternal, and it masks a future re-externalization regression because wrangler would resolve the bare import from the test's pre-installed copy instead of failing. Removing line 46 turns the test into a true regression guard and aligns it with the docs this PR adds.Extended reasoning...
What the issue is
docs/migration-SKILL.md:524(and the matching paragraph indocs/migration.mdand.changeset/workerd-shim-vendors-cfworker.md) now instructs consumers: "Do not installajv,ajv-formats, or@cfworker/json-schema; client/server bundle the runtime-selected defaults." But the SDK's own consumer simulation intest/integration/test/server/cloudflareWorkers.test.ts:44-47still does exactly the thing the docs say not to do — it writes a generatedpackage.jsonwith an explicit dependency:That file is not in this PR's diff (last touched in #1652), so this is a surviving instance of the pattern the PR replaces.
Why the explicit install used to be needed, and isn't anymore
At the PR base commit,
packages/server/tsdown.config.tshadnoExternal: ['@modelcontextprotocol/core']only. The builtdist/shimsWorkerd.mjs(and its chunks) therefore left a bareimport { Validator } from '@cfworker/json-schema'for the consumer's bundler to resolve. wrangler's esbuild step would fail without it, so the test pre-installed the package.This PR adds
'ajv', 'ajv-formats', '@cfworker/json-schema'tonoExternalin bothpackages/server/tsdown.config.tsandpackages/client/tsdown.config.ts, inlining the cfworker code into the workerd shim chunk. The explicit dep in the test consumer'spackage.jsonis now redundant.Why it matters: the dead dep masks a regression
cloudflareWorkers.test.tsis the only test that exercises the packed tarball under a real workerd runtime viawrangler dev. The newbarrelClean.test.tstests added in this PR only string-match thedist/*.mjsoutput for barefrom 'ajv'|'ajv-formats'|'@cfworker/json-schema'imports — useful but static. If a future tsdown/config change re-externalizes@cfworker/json-schema(e.g. someone removes it fromnoExternalwhile refactoring), the static test would catch it — but if that test is also touched, the workerd test should be the safety net. Today it isn't, because the consumer ships its own copy of@cfworker/json-schema, so wrangler's bundler would resolve the bare import fromnode_modulesand the test would keep passing.Step-by-step proof
@modelcontextprotocol/serverto a tarball and writes a consumerpackage.jsonlisting bothfile:./<tarball>and'@cfworker/json-schema': '^4.1.1'.npm installputs both in the consumer'snode_modules.server.tsconstructsnew McpServer(...)— theServerconstructor eagerly doesthis._jsonSchemaValidator = options?.jsonSchemaValidator ?? new DefaultJsonSchemaValidator(), so the workerd shim's static@cfworker/json-schemaimport path is exercised.wrangler devruns esbuild overserver.ts. After this PR, the import resolves to the bundled copy insidedist/shimsWorkerd.mjs— no bare import to satisfy.@cfworker/json-schemais dropped fromnoExternal.dist/shimsWorkerd.mjsnow hasfrom '@cfworker/json-schema'. Without line 46, esbuild fails ("Could not resolve") and the test catches it. With line 46, esbuild resolves it from the test's pre-installed copy, the test passes, and the regression ships.How to fix
Delete the
'@cfworker/json-schema': '^4.1.1'line from the generated consumerpackage.jsonattest/integration/test/server/cloudflareWorkers.test.ts:46. This is a one-line change that turns the test into a genuine end-to-end regression guard for the bundling invariant and brings it into agreement with the docs this PR adds.Filed as a nit: the test isn't directly modified by the PR, the static
barrelClean.test.tsalready guards the structural invariant, and removing the line is a hygiene improvement rather than a fix for broken behavior.