Repository navigation
fix(openapi): preserve timeouts so the server maps them to the actionable message - #5847
Conversation
This comment has been minimized.
This comment has been minimized.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
marvin-context-protocol
Bot
added
bug
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac082e7ad5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
|
|
||
| def _upstream(handler) -> httpx2.AsyncClient: |
There was a problem hiding this comment.
The new _upstream helper leaves its handler parameter untyped even though this repository requires full type annotations. Give it a callable annotation such as Callable[[httpx2.Request], httpx2.Response] so the transport boundary remains type-checkable.
AGENTS.md reference: AGENTS.md:L145-L145
Useful? React with 👍 / 👎.
| Native ``httpx2`` transport errors are re-raised unchanged so the server's | ||
| actionable error mapping (for example, timeouts) can recognize them. | ||
| Legacy-httpx clients, still accepted through the deprecation bridge, are | ||
| normalized to ``ValueError`` with the original error details. |
There was a problem hiding this comment.
The newly added httpx2 and ValueError references use RST-style double backticks. Source docstrings in this repository must use Markdown single-backtick spans for the MDX documentation pipeline, so replace both new spans with single backticks.
AGENTS.md reference: AGENTS.md:L176-L176
Useful? React with 👍 / 👎.
4944798 to
58e4e48
Compare
OpenAPI tool and resource requests normalized every httpx2 transport error to ValueError, so the server-level actionable timeout mapping never matched. Re-raise native httpx2 request/timeout errors unchanged and keep normalization only for legacy-httpx clients, removing the now-dead rewrap handlers in the tool and resource components.
58e4e48 to
b75f9f2
Compare
![fastmcp-triage-reviewer[bot]](https://avatars.githubusercontent.com/u/31014960?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review by the FastMCP triage reviewer bot.
The change repairs timeout normalization at its source, allowing the existing actionable message to reach callers without disabling ordinary-error masking. Independent checks also confirm resource-template timeouts work for all four native timeout subclasses with masking enabled or disabled. No consequential code defect was found; the earlier annotation and docstring findings are addressed.
Checks
The focused PR modules pass all 21 tests. Independent template and native connection-error probes pass all 9 cases; timeout regressions fail on the unchanged base. Static validation and completed current-head CI remain pending.
zzstoatzz
left a comment
zzstoatzz
left a comment
There was a problem hiding this comment.
Letting native httpx2 errors through _send_request lets the server's existing timeout mapping see them, so timeouts get the actionable message again, masked or not. The legacy-httpx path is unchanged.
Non-blocking: with masking off, a non-timeout transport error now shows only its own message, which can be empty for ReadError. Worth a fallback in a follow-up.
Checks
Timeout regressions fail on main and pass here; OpenAPI suite passes.
Generated by Claude Code
Fixes #6167
4.x scope of #5846; that report remains open for the maintained 3.x/native-httpx bug. No backport is included.
Description
With
mask_error_details=True, upstream timeouts from OpenAPI tools and resources surfaced the generic masked message (Error calling tool 'slow'/Error reading resource ...) instead ofUpstream request timed out, please retry._send_requestrewrapped every exception asValueError, so the server-levelis_timeout_error()mapping never matched and the component-level timeout handlers were dead code.What changed
_send_requestnow re-raises nativehttpx2.RequestError(includingTimeoutException) unchanged, so the existing server-level timeout/rate-limit mapping works. Legacy-httpx exceptions keep the previous normalization toValueError(the deprecation bridge still accepts those clients).except httpx2.TimeoutException/RequestErrorrewrap handlers (and their emptytrywrappers) fromOpenAPITool.runandOpenAPIResource.read.test_comprehensive.py::test_timeout_error_produces_useful_message, which asserted the oldHTTP request timed out (ReadTimeout)text, to assert the actionable mapped message.Contribution type
Bug fix verification
Test path:
tests/server/providers/openapi/test_transport_errors.py(five tests).Red at
adf90b66(implementation reverted, final tests applied): 3 failed, 3 passed, all assertion failures, no collection errors (masked tool timeout, masked resource timeout, and the updated message expectation fail; the ordinary-error and legacy-normalization controls pass).Green at the fix commit: 1209 passed, 26 warnings across
tests/server/providers/ tests/server/test_providers.py.Validation
uv run --isolated pytest tests/server/providers/ tests/server/test_providers.py -q-> 1209 passeduv run --isolated ruff checkandruff format --checkon the changed files -> cleanuv run --isolated ty check-> All checks passedAI assistance
Tool(s) used: Claude Code
How you used it: investigated from a verified reproduction, implemented the error-preservation change and the regression tests.
Checklist
uv run prek run --all-filesand all checks pass (focused checks passed)Review follow-up
Codex review asked for AGENTS.md compliance: the mock transport handler is now fully annotated, and the new docstring uses Markdown single-backtick spans instead of RST double backticks. Behavior is unchanged.