Repository navigation
fix(client): compile output validators lazily for the called tool - #2998
epistemedeus wants to merge 3 commits into
Conversation
🦋 Changeset detectedLatest commit: cccc109 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
koriyoshi2041
left a comment
There was a problem hiding this comment.
Reviewed exact head 1fce22a1, focusing on stamp invalidation and negative-result caching. The implementation keeps the complete decoded catalog but compiles only the requested name, bounds memoization to catalogued tools, and replaces the validator map whenever the backing stamp changes. That preserves refresh/list-change/reconnect isolation while preventing an uncalled invalid schema from blocking a valid tool.
The focused cold-call/invalidation/transport coverage is present, git diff --check passes, and the hosted Node 20/22/24, e2e, conformance, runtime, build, examples, and package checks are green. I did not find a distinct correctness issue in this head.
|
Verified this against REA's current 170-tool production catalog ( I compared the exact published cache-owner source recompiled unchanged with the exact source at Five pairs, ordered 3 AB / 2 BA, on Node 24.18.0 with two CPUs and a 768 MiB Node heap:
The median within-pair call reduction was 5,533.5 ms (median paired fraction 88.0%). The catalog stayed complete and identical: 170 tools, 1,954,693 JSON bytes; all 340 advertised input/output schemas passed Ajv2020 meta-validation. Every trial exercised explicit same-catalog refresh, reconnect, retained JavaScript analysis summary/page, ping and close. Refresh/reconnect prepared only the called validator in the candidate; the provider's existing schema-ID cache avoided recompilation in both variants. Complete analysis/view bodies, Evidence IDs, metadata and text/structured parity matched the published baseline. A separately labeled malformed-wire-response injection was rejected by the actual client, followed by successful recovery. I also used Flameox 0.2.9 on separate native V8 client profiles, restricted to the recorded first-call interval. Complete rankings confirmed Ajv compiler hotspots: |
|
@N0zoM1z0 Thanks for checking this against a real 170-tool catalog and keeping the owning-module experiment distinct from the published SDK. Would you be comfortable sharing a sanitized catalog snapshot or measurement harness that we could adapt into a maintained regression? No private traces or customer data needed. The existing changed-schema/list-changed tests and upstream CI remain separate evidence; your product-dependency and release distinction is helpful. |
|
Thanks. I shared REA’s public 170-tool catalog and a runnable stdio test in #3001. It reports cold validator preparation and compilation counts, and checks invalid-output rejection, recovery, and cache reuse. |
|
@epistemedeus, thanks for adding tests for the selected tool. The fixture records provider # Node >=20; pnpm 10.26.1
set -e
git clone --filter=blob:none https://github.com/modelcontextprotocol/typescript-sdk.git
cd typescript-sdk
git fetch origin b022522089a0c8b632595c6e7b536453945ed5a9 pull/2998/head
mkdir -p lab-cases/mcp2998
fixture_source=https://raw.githubusercontent.com/probityai/agent-evidence-atlas/ece6bded7399177202a69efe90f5bbf80a264f6a/lab-cases/mcp2998
for file in consumer.test.mjs server.mjs; do
curl --fail --location "$fixture_source/$file" -o "lab-cases/mcp2998/$file"
done
for revision in base head; do
ref=b022522089a0c8b632595c6e7b536453945ed5a9
if [ "$revision" = head ]; then
ref=1fce22a12ab91fb890aa7255f5448e1e1ff0b4a1
fi
git checkout --detach "$ref"
pnpm --filter @modelcontextprotocol/client... install --frozen-lockfile
pnpm --filter @modelcontextprotocol/client... run build
CALLED_TOOL_REVISION="$revision" node --test lab-cases/mcp2998/consumer.test.mjs
doneBoth accepted pick with an unusable uncalled tool schema: first-call requests fell from 2 on base to 1 on head, with 0 more on repeat. Base catches compilation errors per tool, and this measurement excludes AJV compilation, latency, and whole-catalog validation. |
Preserve the public 170-tool catalog fixture and deterministic lazy-compile, refusal, recovery, invalidation and portable stdio-exit regressions. Timing benchmark remains optional and environment-specific. Signed-off-by: Lucian Constantinescu <epistemedeus@gmail.com>
koriyoshi2041
left a comment
There was a problem hiding this comment.
Re-reviewed the single-commit delta at cccc109. The production-size fixture now exercises the same lazy-cache boundaries against 170 tools: one called validator, rejection and recovery, list-change/manual-refresh invalidation, configured-engine reuse, and stdio child shutdown. The timing path stays explicitly optional, while the assertions are count/body based. Hosted Node 20/22/24, e2e, conformance, runtime, build, examples, and package checks are green. I did not find a new correctness issue in this head.
|
@epistemedeus, I replayed the unchanged Atlas fixture at SDK head cccc109 on Node 20.20.2: six cases pass at both pinned revisions, alongside 31 current-head upstream tests. Both revisions already accept the valid call with an unusable uncalled schema; first-call provider |
Summary
tools/list.toolDefinition, and useful validation errors for the called tool.undefinedvalidators for catalogued tools, but do not retain arbitrary caller names absent from the catalog.Fixes #2997.
Verification
On an actual remote Node 22.14.0 / pnpm 10.26.1 / Vitest 4.1.2 checkout: 22 focused tests and 1,019 full client tests pass; client typecheck, lint, build, and
git diff --checkpass. The extra absent-name cardinality regression failed before its shared-cache correction (21 pass / 1 fail).A freshly deployed client calling a local 26-tool fixture observes zero output-validator compiles at list time and one for the first called tool across stdio and Streamable HTTP. Repeated stdio call adds no compile; invalid called schemas refuse before server execution; changed catalog rejects obsolete output; an explicit tool definition uses one compile and zero lists. These are local mechanism checks, not a reproduction of the reporter's 139-tool latency or a provider-performance claim.
This change addresses the Client list/call compile strategy. It is separate from #2950's provider-level AJV validator reuse and #2976's v1.x failed-refresh handling.