Repository navigation
fix(aws-documentation-mcp-server): raise ToolError so read-path failures reach the model - #4748
Merged
alexisareyn merged 6 commits intoOct 9, 2026
Merged
Conversation
…res reach the model MCP SDK 2.1.0 forwards only a ToolError's message to the client and replaces the text of every other exception with a bare "Error executing tool <name>", keeping crash details off the wire (modelcontextprotocol/python-sdk#3314). Every read-path failure this server raises deliberately already builds a message for the caller, often the "Requested <A>; served <B>." substitution note, and then raised a plain ValueError, so that message was discarded and the model saw no reason and no next step. A moved guide page that redirects to an index shell, a 4xx response, a page with nothing to convert, and a rejected URL were all indistinguishable. Anticipated failures now raise DocumentationToolError, which subclasses ToolError so the SDK forwards the message and ValueError so existing callers and tests keep working. UnreadablePageError subclasses it for the same reason, since it escapes to tool callers. The 23 deliberate raise sites across server_aws.py, server_utils.py and util.py are converted; the partition check in server.py is startup configuration and is untouched. Unexpected exceptions are deliberately NOT converted. The blanket handler in read_sections_impl still re-raises unchanged so a crash stays generic, which is what python-sdk#3314 set out to protect. Verified against SDK 2.3.0: DocumentationToolError arrives as "Error executing tool t: URL must end with .html" while a TypeError arrives as UnexpectedToolError with no detail. SDK 2.0.0 appends the text of any exception, so an environment that resolves 2.0.0 hides this entire class of bug, which is why it was not caught before release. Fixes awslabs#4705
alexisareyn
requested review from
a team,
AadityaBhoota,
JonLim,
artb30,
mc5672,
riaagnes and
zjerath
as code owners
October 8, 2026 14:23
alexisareyn
marked this pull request as draft
October 8, 2026 14:42
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4748 +/- ##
==========================================
+ Coverage 93.45% 93.47% +0.01%
==========================================
Files 1064 1064
Lines 91712 91791 +79
Branches 14887 14899 +12
==========================================
+ Hits 85709 85799 +90
+ Misses 3601 3595 -6
+ Partials 2402 2397 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ame a next step on recoverable failures Both from review feedback on this PR. Raise the floor. 2.1.0 is the release that stopped forwarding non-ToolError messages, so any environment resolving 2.0.0 forwards them anyway and hides this entire class of bug. The lock resolved 2.0.0, which is why the regression reached a release and why CI would not have caught a future one. The lock now resolves 2.3.0 and the suite runs against it. Name a next step. A 4xx, and a redirect that landed on a page with nothing to read, now add "The page may have moved or may no longer exist. Use search_documentation to find the current page.", in the same spirit as the existing missing-subsections message that points at read_documentation. The redirect case is the one behind awslabs#4705: the message already said "Requested <A>; served <B>" without saying what to do about it. Deliberately not added where a search would mislead. A page that is unreadable at the URL asked for has not moved, and a transport failure says nothing about whether the page exists. Page grows a `substituted` property so the two cases can be told apart, which also simplifies `message()`. One existing test asserted the 404 message by exact string equality. Its intent, per its class name, is that no substitution note is prepended without a redirect, so it now asserts that directly and no longer breaks on an intended wording change.
…ng the page moved The suggestion read "The page may have moved or may no longer exist. Use search_documentation to find the current page." A 4xx does not tell us which of those is true, or whether it is instead a typo'd URL, a region-gated page, or a transient edge error. The message is now just "Use search_documentation." - the action we can stand behind, with no diagnosis attached. The constant is renamed `_USE_SEARCH` accordingly; `_FIND_THE_PAGE` named an outcome the server cannot promise. Its comment is dropped rather than reworded, since it carried the same assertion. No test changes: the four tests in TestRecoverableFailuresNameANextStep assert on the `search_documentation` substring, never the sentence, so they pin the behaviour and not the wording.
alexisareyn
marked this pull request as ready for review
October 9, 2026 18:05
alexisareyn
enabled auto-merge
October 9, 2026 18:05
added 2 commits
October 9, 2026 15:26
`Page.message()` joined its parts with a bare space, and only the substitution note ended with a period, so a reason and the next step ran together: "... could not be read: Page failed to be simplified from HTML Use search_documentation." Parts now pass through `_sentence()`, which appends a period unless the part already ends in one. Fixed strings that bring their own punctuation are left alone, so nothing is doubled, and the exception text that supplies most reasons no longer has to remember to terminate itself. Two tests pin both halves of that: the run-on case Michael reported, and the already-a-sentence case that must not gain a second period.
…t in a helper Replaces the `_sentence()` helper from bd86e6d. The run-on it fixed only needed the two message parts that precede a next step to end with a period, which is a property of those strings rather than something the join should compute. `message()` goes back to joining verbatim. The status-code reason gets its period inline at its three call sites. The other reason ends with the exception text, so the period belongs at the `UnreadablePageError` raise sites; two of the five already had one. The two unit tests on the helper are replaced by one on the message it was there to produce, in the class that covers the next-step behaviour.
mc5672
approved these changes
Oct 9, 2026
riaagnes
approved these changes
Oct 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MCP SDK 2.1.0 (python-sdk#3314, merged 2026-08-24) forwards only a
ToolError's message to the client. Every other exception is re-raised asUnexpectedToolErrorwith its text discarded, so the client sees a bareError executing tool <name>. That was deliberate upstream, to keep crash details off the wire.This server's read path raises a plain
ValueErrorfor failures it anticipates, so those messages are discarded too, even though they are written for the caller. The result is that a moved guide page, a 4xx, an unreadable index shell, and a rejected URL are all indistinguishable to the model, and theRequested <A>; served <B>.note the server carefully builds never arrives.Reported in #4705.
Changes
Raise
ToolErrorfor anticipated failuresAdd
DocumentationToolError(ToolError, ValueError):ToolErrorso the SDK forwards the message.ValueErrorso existing callers and tests keep working unchanged. No existing test needed editing.UnreadablePageErrornow subclasses it, since it escapes to tool callers for the same reason.Converted the 23 deliberate raise sites across
server_aws.py,server_utils.pyandutil.py: URL and argument validation, transport failure, HTTP status >= 400, non-HTML responses, unreadable pages, and no-matching-section. The partition check inserver.pyis startup configuration, not a tool failure, so it is untouched.Unexpected exceptions are deliberately not converted. The blanket
except Exceptioninread_sections_implstill re-raises unchanged, so a genuine crash stays generic. Converting those as well would undo what python-sdk#3314 set out to do.Raise the
mcpfloor to 2.1.0SDK 2.0.0 appends the text of any exception, so it forwards the message even for a plain
ValueError. Any environment resolving 2.0.0 hides this entire class of bug, which is why this reached a release uncaught — the lock resolved 2.0.0, so CI would not have caught a regression here either.pyproject.tomlnow declaresmcp[cli]>=2.1.0,<3.0.0and the lock resolves 2.3.0, so the suite runs against the behaviour customers actually get. The new tests still assert the exception type rather than the forwarded string, so they stay meaningful on any 2.x.Name a next step on recoverable failures
A 4xx, and a redirect that landed on a page with nothing to read, now append
Use search_documentation.— in the same spirit as the existing missing-subsections message that points atread_documentation. The redirect case is the one behind #4705: the message already saidRequested <A>; served <B>without saying what to do about it.Deliberately not added where a search would mislead. A page that returned content at the URL asked for but could not be parsed has not moved, and a transport failure says nothing about whether the page exists.
Pagegrows asubstitutedproperty so the two cases can be told apart, which also simplifiesmessage().The suggestion asserts nothing about why the fetch failed. An earlier revision read "The page may have moved or may no longer exist", but a 4xx does not tell us which of those is true, or whether it is instead a typo'd URL, a region-gated page, or a transient edge error — so the message is just the action.
User Experience
Against SDK 2.3.0, through the real
MCPServer.call_toolpath:The message arrives for the anticipated failure; the
TypeError's text does not.Before and after on the same assertion:
449 passed, ruff clean, pyright clean. 428 of those are pre-existing and unmodified.Relationship to #4726
#4726 proposed the same
DocumentationToolError(ToolError, ValueError)approach and that idea is the right one, so credit to @Christian-Sidak for it. This PR differs in four ways:mcp.server.mcpserver.exceptions. fix(aws-documentation-mcp-server): surface read-path errors to MCP clients #4726 usesmcp.server.fastmcp.exceptions, which does not exist on any 2.x SDK (the module was renamed in 2.0.0), so the server fails to import on exactly the versions the fix targets.main. fix(aws-documentation-mcp-server): surface read-path errors to MCP clients #4726 branches from before fix(aws-documentation-mcp-server)!: correct read-path output and make failures raise #4650.read_documentation_implraises, which is the redirect case aws-documentation-mcp-server: read-path failure messages masked as "Error executing tool" since MCP SDK 2.x #4705 actually reports. fix(aws-documentation-mcp-server): surface read-path errors to MCP clients #4726 does not reach them, because they did not exist in its base.except Exceptionhandler alone rather than converting it.Fixes #4705
Checklist
Is this a breaking change? N
RFC issue number: N/A
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.