Conversation
Motivation: call_tool() revalidates a successful CallToolResult's structured_content against the tool's declared output_schema after every call. When the session's output-schema cache is empty -- which it always is on a short-lived session, the pattern stateless gateways and proxies use (one ClientSession per call) -- that revalidation triggers a tools/list request to discover the schema. This doubles round-trips on every call_tool and, when the server behind the session is itself an aggregator, adds that aggregator's slowest-backend tools/list latency to every single call, with no way to opt out short of subclassing ClientSession. Approach: Add a validate_tool_results: bool = True constructor parameter to both ClientSession and the high-level Client. When False, call_tool skips the automatic validate_tool_result() call entirely -- on both the direct result path and the SEP-2133 claimed-extension-result path -- so no tools/list is issued and no RuntimeError is raised for output that doesn't match a schema the caller never listed. The default stays True, so existing behavior, including the tests that rely on a fresh session auto-discovering the schema via its first validate_tool_result() call, is unchanged. This is the constructor opt-out shape from the issue's three proposed options (the alternative of skipping the refresh only on a wholly empty cache would have changed default behavior on a fresh session, which several existing tests -- test_validate_tool_result_passes_a_conforming_result and friends in tests/client/test_session_promotions.py -- deliberately lock in). Validation: - `uv run --frozen pytest tests/client/` -- 782 passed, 1 skipped, 1 xfailed - `uv run --frozen ruff format --check .` / `ruff check .` -- clean - `uv run --frozen pyright` on changed files -- 0 errors - `./scripts/test` (full coverage-gated suite) -- 5970 passed, 100.00% coverage, strict-no-cover clean - `uv run --frozen pre-commit run --files <changed>` -- markdownlint and ruff hooks pass; the pyright hook's only failure (tests/transports/stdio/test_lifecycle.py:190, os.waitid) is confirmed pre-existing on a clean main via git stash, unrelated to this change - Base branch CI (`gh run list --branch main --event push`) is green as of the last push Report: modelcontextprotocol#3513 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com> Assisted-by: claude-sonnet-5 (via Claude Code)
|
This PR has been closed automatically. This repo only keeps pull requests open when they come from a maintainer, or from a contributor a maintainer has assigned to the linked issue, and you aren't currently assigned to #3513. If a maintainer assigns you to #3513, this PR reopens on its own and there's nothing more you need to do here. Assignment is a maintainer call based on capacity; comments that only ask to be assigned don't factor in. What does help is engaging on the issue itself by confirming the repro, explaining why it matters for your use case, or describing the approach you'd take. You're welcome to keep pushing commits here (just avoid force-pushing, since GitHub can't reopen a rewritten branch), but that on its own won't get the PR reviewed or the issue assigned, and realistically most auto-closed PRs stay closed. There's no need to open a new PR either way. CONTRIBUTING.md has the full reasoning, but in short:
Maintainers: reopen, remove |
Fixes #3513
Add a
validate_tool_results: bool = Trueconstructor parameter toClientSessionand the high-levelClient. When set toFalse,call_toolskips the automaticvalidate_tool_result()call it otherwise runs after every successful result.Motivation and Context
call_tool()revalidates a successfulCallToolResult'sstructured_contentagainst the tool's declaredoutput_schemaafter every call. When the session's output-schema cache is empty — which it always is on a short-lived session, the pattern stateless gateways and proxies use (oneClientSessionper call) — that revalidation triggers atools/listrequest to discover the schema. This doubles round-trips on everycall_tool, and when the server behind the session is itself an aggregator, it adds that aggregator's slowest-backendtools/listlatency to every single call. There was previously no way to opt out short of subclassingClientSession.This is the "constructor opt-out" shape from the three options the issue proposed. The alternative (skip the refresh only when the cache is wholly empty) would have changed default behavior on a fresh session — several existing tests (
test_validate_tool_result_passes_a_conforming_resultand others intests/client/test_session_promotions.py) deliberately lock in today's auto-discover-and-validate behavior on a first call, so changing that default felt like a maintainer design call rather than an obvious bug fix. The opt-out is purely additive: the default (True) preserves existing behavior exactly.How Has This Been Tested?
uv run --frozen pytest tests/client/— 782 passed, 1 skipped, 1 xfaileduv run --frozen ruff format --check ./uv run --frozen ruff check .— cleanuv run --frozen pyrighton changed files — 0 errors./scripts/test(full coverage-gated suite) — 5970 passed, 100.00% coverage,strict-no-covercleanuv run --frozen pre-commit run --files <changed>— markdownlint and ruff hooks pass. The pyright hook's only failure (tests/transports/stdio/test_lifecycle.py:190,os.waitid) is confirmed pre-existing on a cleanmainviagit stash, unrelated to this change.call_toolpaths that revalidate: the direct path (tests/client/test_session_promotions.py) and the SEP-2133 claimed-extension-result path (tests/client/test_client_extensions.py), each asserting that with the flag off, a schema-violating result passes through unraised and (for the direct path) that notools/listrequest is issued — the test server has noon_list_toolshandler at all, so such a request would raiseMETHOD_NOT_FOUND.gh run list --branch main --event push) is green as of the last push.Breaking Changes
None. The new parameter defaults to
True, matching current behavior exactly; all existing tests pass unmodified.Types of changes
Checklist
help wanted, or I'm a maintainer)Additional context
This PR was developed with AI assistance (Claude). I reviewed the change and can explain the approach and trade-offs described above.
Docs: added a short paragraph to
docs/advanced/low-level-server.mdright after the section that already describes theClient's automaticstructured_content/output_schemarevalidation, explaining thetools/listround-trip cost and the new escape hatch.AI assistance: this change was drafted with Claude Code.