Skip to content

fix(server): sanitize unexpected tool errors - #3293

Open
uuzzrm wants to merge 4 commits into
modelcontextprotocol:mainfrom
uuzzrm:codex/fix-698-tool-error-details
Open

fix(server): sanitize unexpected tool errors#3293
uuzzrm wants to merge 4 commits into
modelcontextprotocol:mainfrom
uuzzrm:codex/fix-698-tool-error-details

Conversation

@uuzzrm

@uuzzrm uuzzrm commented Aug 12, 2026

Copy link
Copy Markdown

Fixes #698

Summary

  • Preserve explicit ToolError messages for expected, model-facing failures.
  • Log unexpected tool exceptions server-side and return a generic client-facing message.
  • Keep exception chains and validation details out of tool results.
  • Update regression coverage, examples, and documentation for the distinction.

Testing

  • uv run --frozen ruff check on changed Python files
  • uv run --frozen ruff format --check on changed Python files
  • uv run --frozen pyright
  • 88 focused tests covering tool execution and documentation examples
  • The full parallel suite ran 5,576 tests successfully; the remaining xdist timeouts passed when the affected modules were rerun serially.

AI-assisted contribution disclosure

I used AI assistance to inspect the issue and draft the implementation and tests. I reviewed the final diff, ran the checks above, and own the change for review.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

1 issue found across 35 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="tests/server/mcpserver/test_resolve.py">

<violation number="1" location="tests/server/mcpserver/test_resolve.py:1435">
P2: These legacy-mode assertions codify the same client-caused, expected failure (empty accepted elicitation / schema-mismatched accepted answer) being surfaced as a generic "An unexpected error occurred while executing tool ...", while auto mode preserves the specific ToolError message ("received an accepted elicitation with no content" / "does not match the requested schema"). This contradicts the PR's stated goal of preserving explicit ToolError messages for expected, model-facing failures: in legacy mode a bad elicitation response is indistinguishable from a server crash, so the client gets no actionable feedback. The root cause is that the legacy path elicits via ctx.elicit()/elicit_with_validation() which raises a plain ValueError (src/mcp/server/elicitation.py:119,128) caught by the generic handler in tools/base.py:187-189, whereas >= 2026-07-28 raises ToolError in resolve.py:619/624. Consider raising ToolError (or a ValueError subclass the tool runner preserves) from elicit_with_validation so both transports surface the same explicit message.</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread tests/server/mcpserver/test_resolve.py Outdated
assert "Resolver" in text
else:
assert "Received an accepted elicitation" in text # the legacy path has no wire key to name
assert text == "An unexpected error occurred while executing tool whoami"

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2: These legacy-mode assertions codify the same client-caused, expected failure (empty accepted elicitation / schema-mismatched accepted answer) being surfaced as a generic "An unexpected error occurred while executing tool ...", while auto mode preserves the specific ToolError message ("received an accepted elicitation with no content" / "does not match the requested schema"). This contradicts the PR's stated goal of preserving explicit ToolError messages for expected, model-facing failures: in legacy mode a bad elicitation response is indistinguishable from a server crash, so the client gets no actionable feedback. The root cause is that the legacy path elicits via ctx.elicit()/elicit_with_validation() which raises a plain ValueError (src/mcp/server/elicitation.py:119,128) caught by the generic handler in tools/base.py:187-189, whereas >= 2026-07-28 raises ToolError in resolve.py:619/624. Consider raising ToolError (or a ValueError subclass the tool runner preserves) from elicit_with_validation so both transports surface the same explicit message.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/server/mcpserver/test_resolve.py, line 1435:

<comment>These legacy-mode assertions codify the same client-caused, expected failure (empty accepted elicitation / schema-mismatched accepted answer) being surfaced as a generic "An unexpected error occurred while executing tool ...", while auto mode preserves the specific ToolError message ("received an accepted elicitation with no content" / "does not match the requested schema"). This contradicts the PR's stated goal of preserving explicit ToolError messages for expected, model-facing failures: in legacy mode a bad elicitation response is indistinguishable from a server crash, so the client gets no actionable feedback. The root cause is that the legacy path elicits via ctx.elicit()/elicit_with_validation() which raises a plain ValueError (src/mcp/server/elicitation.py:119,128) caught by the generic handler in tools/base.py:187-189, whereas >= 2026-07-28 raises ToolError in resolve.py:619/624. Consider raising ToolError (or a ValueError subclass the tool runner preserves) from elicit_with_validation so both transports surface the same explicit message.</comment>

<file context>
@@ -1425,12 +1428,12 @@ async def whoami(login: Annotated[Login, Resolve(ask)]) -> str:
+            assert "Resolver" in text
         else:
-            assert "Received an accepted elicitation" in text  # the legacy path has no wire key to name
+            assert text == "An unexpected error occurred while executing tool whoami"
+        assert "errors.pydantic.dev" not in text
 
</file context>

Comment thread tests/interaction/mcpserver/test_prompts.py Outdated
Signed-off-by: Ruiming Zhao <uuzzrm@gmail.com>
@uuzzrm

uuzzrm commented Aug 12, 2026

Copy link
Copy Markdown
Author

Thanks for the review. I addressed both findings in commit 5ebaf73.

  • Legacy elicit_with_validation() now raises ToolError for accepted responses with missing or schema-invalid content, so legacy and auto elicitation paths preserve the same actionable message. Unexpected exceptions remain sanitized by Tool.run().
  • Corrected the prompt test docstring to describe the actual behavior: the prompt path logs the failure and returns the raw validation message with the stable rendering-error prefix.

Verification:

  • tests/server/mcpserver/test_resolve.py: 120 passed
  • tests/interaction/mcpserver/test_prompts.py: passed
  • tests/server/mcpserver/tools/test_base.py tests/interaction/mcpserver/test_tools.py: 81 passed
  • ruff check, ruff format --check, and targeted Pyright: passed
  • git diff --check: passed

The full Pyright run still reports the repository's existing 34 platform-specific typing errors in POSIX/Windows utility and stdio files; none are in the changed files.

AI assistance was used and the final diff and test results were reviewed.

@uuzzrm

uuzzrm commented Aug 12, 2026

Copy link
Copy Markdown
Author

The latest CI failure was one stale documentation regression assertion in tests/docs_src/test_elicitation.py:127. It still expected the generic unexpected-tool message, while the legacy elicitation path now intentionally preserves the stable schema-validation ToolError message.

Updated in commit 3898b88 and pushed. Local verification: 179 affected tests passed; Ruff, format check, and git diff --check passed.

AI assistance was used and the final diff and test results were reviewed.

@uuzzrm

uuzzrm commented Aug 12, 2026

Copy link
Copy Markdown
Author

I also addressed the standards review findings in 47d4e09:

  • Documented the intentional broad catch at the Tool.run execution boundary, where arbitrary tool exceptions must be sanitized before crossing MCP.
  • Added return annotations and behavior/provenance docstrings to the new tests.
  • Replaced message-regex pytest.raises(..., match=...) assertions with captured exception message assertions.

Verification: 131 affected tests passed; Ruff check, format check, and git diff --check passed. The local Pyright binary is currently blocked by a missing Homebrew libllhttp.9.3.dylib; the previous GitHub Pyright check passed on the PR before this comment-only/test-quality commit.

AI assistance was used and the final diff and test results were reviewed.

@uuzzrm

uuzzrm commented Aug 12, 2026

Copy link
Copy Markdown
Author

The latest head also includes the standards-review follow-up in 47d4e09.

The legacy elicitation behavior, stale docs expectation, and test-quality findings are now addressed. The latest CI run 31555163205 is green across the full Python 3.10–3.14 locked/lowest-direct Linux and Windows matrix, including docs, conformance, pre-commit, security analysis, and all-green.

The PR remains open and awaits maintainer review; mergedAt is still null. AI assistance was used and the final diff and test results were reviewed.

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.

Tool.run should not reveal exception value to the client

1 participant