Skip to content

Remove unused mcpserver.exceptions.ValidationError #3199

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.

Sign up for GitHub

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

Merged
maxisbey merged 1 commit into from
Jul 28, 2026
+0 −4
Merged

Remove unused mcpserver.exceptions.ValidationError #3199

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/mcp/server/mcpserver/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,10 @@


class MCPServerError(Exception):
"""Base error for MCPServer."""


class ValidationError(MCPServerError):
"""Error in validating parameters or return values."""


class ResourceError(MCPServerError):

Check failure on line 8 in src/mcp/server/mcpserver/exceptions.py

View check run for this annotation

Claude / Claude Code Review

Breaking ValidationError removal missing from docs/migration.md 

This PR removes the public symbol `mcp.server.mcpserver.exceptions.ValidationError` — a breaking change per the PR's own checklist — but does not add the required entry to `docs/migration.md` (AGENTS.md: breaking changes *must* be documented there). Add a line to the existing exceptions inventory in the FastMCP→MCPServer section noting `ValidationError` was removed and that callers should catch `pydantic.ValidationError` or drop the import.
Comment on lines 5 to 8

@claude claude Bot Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 This PR removes the public symbol mcp.server.mcpserver.exceptions.ValidationError — a breaking change per the PR's own checklist — but does not add the required entry to docs/migration.md (AGENTS.md: breaking changes must be documented there). Add a line to the existing exceptions inventory in the FastMCP→MCPServer section noting ValidationError was removed and that callers should catch pydantic.ValidationError or drop the import.

Extended reasoning...

The gap. AGENTS.md states unconditionally: "Breaking changes (including those softened by a backwards-compatibility shim) must be documented in docs/migration.md", covering what changed, why, and how to migrate. This PR deletes the public class ValidationError from mcp.server.mcpserver.exceptions, and the PR description itself checks the Breaking change box ("Only for code importing a symbol the SDK never raised"). Yet the diff touches only src/mcp/server/mcpserver/exceptions.py — docs/migration.md is not updated.

Why it's breaking despite never being raised. The breaking surface is the import itself, not the raise. The symbol existed in v1's fastmcp/exceptions.py and in v2 up to this PR, so user code doing from mcp.server.mcpserver.exceptions import ValidationError — e.g. to catch it defensively alongside ToolError/ResourceError, or code mid-migration from v1's mcp.server.fastmcp.exceptions — now fails at import time with ImportError. "The SDK never raised it" is exactly the why that belongs in the migration entry; it is not an exemption from writing one.

Why existing docs don't cover it. Grepping docs/migration.md for ValidationError finds only references to pydantic's ValidationError (resource extra="forbid", elicitation, wire validation, etc.). None documents this removal. Meanwhile the guide already contains the exact inventory this entry belongs in: the FastMCP→MCPServer section's import list (around lines 664–668) enumerates ToolError, ResourceError, and MCPServerError as importable from mcp.server.mcpserver.exceptions. Per AGENTS.md's guidance to group related changes with existing sections rather than adding standalone ones, the removal note goes right there.

Step-by-step proof.

  1. A v1 user has from mcp.server.fastmcp.exceptions import ToolError, ValidationError (defensive catch-all around tool invocation).
  2. Following the migration guide, they rewrite the module path: from mcp.server.mcpserver.exceptions import ToolError, ValidationError.
  3. On a v2 build containing this PR, that line raises ImportError: cannot import name 'ValidationError' from 'mcp.server.mcpserver.exceptions'.
  4. They consult docs/migration.md, search for ValidationError, and find only pydantic-related entries — no explanation that the symbol was removed or what to do instead.

Impact and fix. Merging as-is leaves the v1→v2 migration guide concretely incomplete for an acknowledged breaking change, violating the repo's explicit "must". The fix is a one-line doc addition to the existing inventory, e.g.: ValidationError — removed; it was never raised by the SDK (v1 or v2) and only shadowed pydantic.ValidationError. Catch pydantic.ValidationError if that's what you meant, or drop the import.

Severity. Normal rather than nit because this isn't a stale-description mismatch — the code/docs state after merge violates a hard repo policy ("must be documented in docs/migration.md"), and the guide's whole purpose on the V2 branch is to be the complete record of the v1→v2 delta. All three verifiers independently confirmed with no refutations.

@maxisbey maxisbey Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

na don't care :)

"""Error in resource operations."""


Expand Down
Loading