Split ValueError from unexpected errors during logs. - #3347
StarsExpress wants to merge 13 commits into
Conversation
ValueError only needs a warning. Unexpected errors need a full traceback.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
This comment was marked as spam.
This comment was marked as spam.
Now when PromptValidationError happens, no traceback shows up in request path.
This reverts commit 8c30d12.
|
@cubic-dev-ai review this pull request |
@StarsExpress I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 4 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_server.py">
<violation number="1" location="tests/server/mcpserver/test_server.py:1560">
P2: This regression test only inspects `mcp.server.mcpserver.server`, so it can pass even if the same missing-arg failure still emits a traceback from `mcp.shared.jsonrpc_dispatcher`. Assert the dispatcher logger too, or this test gives false confidence about the reported no-traceback behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| # wraps `async with Client(...): with pytest.raises(...): await ...` as | ||
| # a 4th nesting level around a single `await` statement (3 levels of | ||
| # nesting is OK; 4 is not). So `caplog.set_level` avoids extra `with` layer. | ||
| caplog.set_level(logging.WARNING, logger="mcp.server.mcpserver.server") |
There was a problem hiding this comment.
P2: This regression test only inspects mcp.server.mcpserver.server, so it can pass even if the same missing-arg failure still emits a traceback from mcp.shared.jsonrpc_dispatcher. Assert the dispatcher logger too, or this test gives false confidence about the reported no-traceback behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/server/mcpserver/test_server.py, line 1560:
<comment>This regression test only inspects `mcp.server.mcpserver.server`, so it can pass even if the same missing-arg failure still emits a traceback from `mcp.shared.jsonrpc_dispatcher`. Assert the dispatcher logger too, or this test gives false confidence about the reported no-traceback behavior.</comment>
<file context>
@@ -1536,6 +1536,67 @@ def prompt_fn(name: str) -> str: ... # pragma: no branch
+ # wraps `async with Client(...): with pytest.raises(...): await ...` as
+ # a 4th nesting level around a single `await` statement (3 levels of
+ # nesting is OK; 4 is not). So `caplog.set_level` avoids extra `with` layer.
+ caplog.set_level(logging.WARNING, logger="mcp.server.mcpserver.server")
+ async with Client(mcp, mode="legacy") as client:
+ with pytest.raises(MCPError, match="Missing required arguments"):
</file context>
| caplog.set_level(logging.WARNING, logger="mcp.server.mcpserver.server") | |
| caplog.set_level(logging.WARNING, logger="mcp.server.mcpserver.server") | |
| caplog.set_level(logging.WARNING, logger="mcp.shared.jsonrpc_dispatcher") |
There was a problem hiding this comment.
This was already investigated — see earlier discussion with @keeltrace on this PR.
I tried exactly this — raising MCPError from the PromptValidationError branch so the dispatcher recognizes it as expected too — in 8c30d12, and it broke tests/interaction/mcpserver/test_prompts.py and tests/docs_src/test_prompts.py::test_missing_required_argument_is_a_protocol_error.
Legacy JSONRPCDispatcher and modern Client/HTTP entry deliberately return different wire shapes — code=0, message=str(e) verbatim vs generic code=-32603, message="Internal server error" — for an unrecognized exception at that boundary, so a single exception raised from get_prompt can't satisfy both.
Reverted at ae720ad. Test's docstring documents this scoping decision — asserting dispatcher logger here would make the test fail against expected. This is intentional behavior and not a bug.
github-actions
Bot
added
the
missing-issue-link
StarsExpress
changed the title
StarsExpress
changed the title
ValueError from unexpected errors during logs.
2 participants
Fixes #3342.
Motivation and Context
When a prompt is called without a required argument, the request is correctly rejected, but Python-SDK logs validation error with a full traceback.
Validation error doesn't need a full traceback, which is typically reserved for unexpected errors.
Thus, there needs a fix to separate validation error from unexpected errors.
How Has This Been Tested?
Yes. Two tests are added inside
tests/server/mcpserver/test_server.py:test_get_prompt_missing_args_logs_warning_without_traceback— it asserts that missing-argument ValueErrors are an expected validation failure, so logger writes a plain warning withoutexc_info. No full traceback at all.test_get_prompt_unexpected_error_still_logs_traceback— it asserts that a prompt function raising an unexpected exception must still have logs containing a full traceback.Breaking Changes
No breaking changes at all. This PR #3347 is a pure bug fix.
Types of changes
Checklist
help wanted, or I'm a maintainer)Additional context
1. AI disclosure
I used Claude Sonnet 5 during chat sessions as a pair-programming and debugging assistant for:
PromptValidationErrorinsidesrc/mcp/server/mcpserver/prompts/base.py.2. Notes
I left a claim comment on issue #3342 before starting as repo convention per
CONTRIBUTING.md. As of opening this PR, no one else has commented on this issue, and it hasn't been formally assigned or labeled ready for work. Currently PR #3347 is a draft, so bug fix and CI status are verifiable and visible. Waiting on maintainers' buy-in.3. Final
I have reviewed, tested, and can explain every change in my own words.