Align prompt handler with resource pattern#2740
Conversation
Move task metadata extraction and MCP conversion from LowLevelServer.get_prompt() decorator into FastMCP._get_prompt_mcp(), matching the resource handler architecture from PR #2734.
|
Warning Rate limit exceeded@jlowin has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 0 minutes and 27 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Test Failure AnalysisSummary: The integration test is timing out after 30s and hitting a 429 (Too Many Requests) rate limit from GitHub's API during teardown. Root Cause: This failure is unrelated to the PR changes. The PR refactors prompt handler logic (moving task metadata extraction from to ), but the failing test is for GitHub MCP remote integration testing tool calls. The test is hitting GitHub Copilot's MCP endpoint () and encountering:
The error occurs at line 630 in Suggested Solution: This is a transient infrastructure issue with the external GitHub API, not a code problem. Options:
The PR changes themselves don't affect this code path. The test should pass once GitHub's rate limits reset. Detailed AnalysisTest Failure LogWhat the Test DoesFrom async def test_call_tool_ko(
self, streamable_http_client: Client[StreamableHttpTransport]
):
"""Test calling a non-existing tool"""
async with streamable_http_client:
assert streamable_http_client.is_connected()
with pytest.raises(McpError, match=r"unknown tool|tool not found"):
await streamable_http_client.call_tool("foo")The test calls a non-existent tool "foo" on GitHub's MCP server and expects an PR ChangesThe PR moves prompt-related logic from
These changes only affect prompt handling, not tool calls or client transport logic. Related Files
|
Test Failure AnalysisSummary: The integration test Root Cause: This failure is unrelated to the PR changes. The PR refactors prompt handler logic (moving task metadata extraction from
The error occurs at line 630 in Suggested Solution: This is a transient infrastructure issue with the external GitHub API, not a code problem. Options:
The PR changes themselves don't affect this code path. The test should pass once GitHub's rate limits reset. Detailed AnalysisTest Failure LogWhat the Test DoesFrom `test_github_mcp_remote.py:90-97`: async def test_call_tool_ko(
self, streamable_http_client: Client[StreamableHttpTransport]
):
"""Test calling a non-existing tool"""
async with streamable_http_client:
assert streamable_http_client.is_connected()
with pytest.raises(McpError, match=r"unknown tool|tool not found"):
await streamable_http_client.call_tool("foo")The test calls a non-existent tool "foo" on GitHub's MCP server and expects an `McpError`. Instead, it hangs for 30s then times out, and during cleanup gets a 429. PR ChangesThe PR moves prompt-related logic from `low_level.py` to `server.py`:
These changes only affect prompt handling, not tool calls or client transport logic. Related Files
|
Test Failure AnalysisSummary: Two integration tests for GitHub MCP remote are timing out after receiving HTTP 429 (Too Many Requests) errors from the GitHub Copilot API. Root Cause: The tests Suggested Solution: These are external integration tests that depend on third-party API availability and rate limits. Consider one of these approaches:
The most practical immediate fix would be option 2 - marking these specific tests to be skipped in CI or allowing them to be flaky, since they test integration with an external service that has rate limits outside of our control. Detailed AnalysisFailed Tests:
Error Log Excerpt: The test starts to call the tool but the request hangs waiting for a response. After 30 seconds (the pytest timeout), the test is killed. During teardown, the client tries to disconnect and that's when the 429 error surfaces in the exception handler. Timeline:
Related Files
|
Aligns the prompt handler architecture with the resource pattern established in PR #2734. Previously, task metadata extraction and MCP conversion for prompts happened in the low-level
LowLevelServer.get_prompt()decorator, while resources did this work in the FastMCP handler. This inconsistency made the codebase harder to reason about.Now both resources and prompts follow the same flow:
ServerResult_get_prompt_mcp): task metadata extraction, contextvars, MCP conversionrender_prompt): middleware, provider lookup_render): returns FastMCP types (PromptResult)This also removes several now-unused imports from
low_level.py.