Fix TypeError: CORSMiddleware.__call__() missing 2 required positiona… - #523
Conversation
…l arguments The _wrap_well_known_endpoint function assumed all route endpoints are regular request handlers (async def handler(request) -> Response). However, the MCP SDK's cors_middleware wraps handlers with CORSMiddleware, which is an ASGI app expecting (scope, receive, send). When the wrapper called `await endpoint(request)` on a CORSMiddleware instance, it passed only 1 argument instead of the required 3 ASGI args. The fix detects whether the endpoint is a regular handler function or an ASGI app (using the same inspect check as Starlette's Route constructor), and uses the appropriate calling convention: - Regular handlers: called as `await endpoint(request)` (existing behavior) - ASGI apps: invoked via the ASGI interface `await endpoint(scope, receive, send)` with response capture to apply cache-busting headers https://claude.ai/code/session_011S5zFTWRfKBJBUEanrhvQg
📝 WalkthroughWalkthroughAdds a new WellKnownCacheControlMiddleware in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ASGI_Server
participant WellKnownMW as WellKnownCacheControlMiddleware
participant SessionMW as SessionMiddleware
participant App as Application
Client->>ASGI_Server: HTTP request (path)
ASGI_Server->>WellKnownMW: ASGI scope, receive, send
WellKnownMW->>SessionMW: forward scope
SessionMW->>App: forward scope
App-->>SessionMW: response (headers, body)
SessionMW-->>WellKnownMW: response (headers, body)
WellKnownMW-->>WellKnownMW: if well-known path -> compute ETag, mutate headers (Cache-Control, ETag) via MutableHeaders
WellKnownMW-->>ASGI_Server: send (possibly mutated) response
ASGI_Server-->>Client: HTTP response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/server.py`:
- Around line 105-115: The current code converts raw_headers into a dict
(headers) which collapses duplicate keys; instead, keep raw_headers and after
creating the Response object (Response(...)), iterate over raw_headers, decoding
keys/values (handle bytes) and call response.headers.append(decoded_key,
decoded_value) for each pair so duplicate headers like Set-Cookie are preserved
(refer to raw_headers, headers variable, Response, and
response.headers.append()).
- Line 103: Replace direct access to the private attribute by calling the public
receive property on the Request object: in the call to endpoint use
request.receive instead of request._receive so the invocation becomes await
endpoint(request.scope, request.receive, send); this avoids using the private
attribute and uses Starlette's documented Request.receive wrapper.
| elif message["type"] == "http.response.body": | ||
| body_parts.append(message.get("body", b"")) | ||
|
|
||
| await endpoint(request.scope, request._receive, send) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Starlette Request.receive public API documentation
💡 Result:
Starlette’s official “Requests” documentation does not document a Request.receive method/property as part of its public API. Instead, it documents that Request wraps the ASGI scope and (optionally) the ASGI receive callable, and encourages using higher-level APIs like await request.body(), await request.json(), and request.stream() rather than working with the receive channel directly. [1]
If what you’re looking for is the raw ASGI receive callable, the closest “public” documentation you’ll find is in FastAPI’s autogenerated reference for Request (which is Starlette’s Request), where receive appears as a documented attribute/property name (alongside scope, headers, etc.), but it’s not described in detail there either. [2]
For background on how Starlette intends you to use receive (pass it into Request(...), then use the higher-level request interfaces), Encode’s ASGI/HTTP article explains this design at a conceptual level. [3]
Sources: Starlette Requests docs [1]; FastAPI Request reference (Starlette-based) [2]; Encode ASGI/HTTP article [3].
🏁 Script executed:
# Check the codebase for context about how endpoint is used
cd core && grep -r "endpoint" server.py | head -20Repository: taylorwilsdon/google_workspace_mcp
Length of output: 1102
🏁 Script executed:
# Look for imports and see what Request object is being used
head -50 core/server.pyRepository: taylorwilsdon/google_workspace_mcp
Length of output: 1857
🏁 Script executed:
# Search for how Starlette Request handles receive
rg "class Request" -A 20 --type py | head -60Repository: taylorwilsdon/google_workspace_mcp
Length of output: 60
🏁 Script executed:
# Also check if there's any Starlette version info in the codebase
rg "starlette" --type py | grep -i "version\|import" | head -20Repository: taylorwilsdon/google_workspace_mcp
Length of output: 702
🌐 Web query:
Starlette Request object receive attribute availability source code
💡 Result:
In Starlette, the ASGI receive callable is stored on the Request instance as self._receive, and exposed via the receive property:
Request.__init__(scope, receive=empty_receive, send=empty_send)assignsself._receive = receive@property def receive(self) -> Receive: return self._receive- If you instantiate
Request(scope)without passingreceive, the defaultempty_receive()raisesRuntimeError("Receive channel has not been made available")if anything tries to read from it. [1]
This matches the docs: Request(scope, receive=None) and “If you don't need to access the request body you can instantiate a request without providing an argument to receive.” (But body/stream access needs a working receive callable.) [2]
FastAPI’s Request is Starlette’s Request and documents the same receive property and points to starlette/requests.py as the source. [3]
Sources:
[1] https://raw.githubusercontent.com/encode/starlette/master/starlette/requests.py
[2] https://www.starlette.io/requests/
[3] https://fastapi.tiangolo.com/reference/request/
Use the public Request.receive property instead of private _receive.
Line 103 directly accesses the private _receive attribute. Starlette's Request object provides a public receive property that wraps this attribute and is documented in the official API.
Proposed fix
- await endpoint(request.scope, request._receive, send)
+ await endpoint(request.scope, request.receive, send)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await endpoint(request.scope, request._receive, send) | |
| await endpoint(request.scope, request.receive, send) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/server.py` at line 103, Replace direct access to the private attribute
by calling the public receive property on the Request object: in the call to
endpoint use request.receive instead of request._receive so the invocation
becomes await endpoint(request.scope, request.receive, send); this avoids using
the private attribute and uses Starlette's documented Request.receive wrapper.
| headers = { | ||
| (k.decode() if isinstance(k, bytes) else k): ( | ||
| v.decode() if isinstance(v, bytes) else v | ||
| ) | ||
| for k, v in raw_headers | ||
| } | ||
| response = Response( | ||
| content=b"".join(body_parts), | ||
| status_code=status_code, | ||
| headers=headers, | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's explore the repository structure and find the file
git ls-files | head -20Repository: taylorwilsdon/google_workspace_mcp
Length of output: 530
🏁 Script executed:
# Check the specific file and its context
wc -l core/server.pyRepository: taylorwilsdon/google_workspace_mcp
Length of output: 96
🏁 Script executed:
# Read the relevant section around lines 105-115
sed -n '95,125p' core/server.pyRepository: taylorwilsdon/google_workspace_mcp
Length of output: 1260
🌐 Web query:
Starlette Response raw_headers MutableHeaders duplicate headers preservation
💡 Result:
In Starlette, duplicate header preservation depends on how you mutate headers:
Response(headers=...)cannot preserve duplicates, because it accepts aMapping[str, str]and buildsraw_headersfromheaders.items()(a mapping can only hold one value per key). (github.abhinayambati.codes)response.headers[...] = ...(i.e.,MutableHeaders.__setitem__) removes duplicates for that header name by design. (github.abhinayambati.codes)- To preserve duplicates, use either:
response.headers.append(key, value)— explicitly documented as “preserving any duplicate entries”. (github.abhinayambati.codes)- or mutate
response.raw_headersdirectly (it’s the actual ASGIList[Tuple[bytes, bytes]]sent on the wire).Response.headersis just aMutableHeaders(raw=self.raw_headers)wrapper over that list. (github.abhinayambati.codes)
Practical patterns:
# Preserve duplicates (e.g., multiple Set-Cookie)
response.headers.append("set-cookie", cookie1)
response.headers.append("set-cookie", cookie2)# Or do it at the raw ASGI level
response.raw_headers.append((b"set-cookie", cookie1.encode("latin-1")))
response.raw_headers.append((b"set-cookie", cookie2.encode("latin-1")))(Starlette’s own Response.set_cookie() uses self.raw_headers.append((b"set-cookie", ...)), which is why multiple cookies work.) (github.abhinayambati.codes)
Citations:
- 1: https://github.abhinayambati.codes/repo/AiImpactAnalyzer/blob/backend/venv/Lib/site-packages/starlette/responses.py
- 2: https://github.abhinayambati.codes/repo/AiImpactAnalyzer/blob/backend/venv/Lib/site-packages/starlette/datastructures.py
- 3: https://github.abhinayambati.codes/repo/AiImpactAnalyzer/blob/backend/venv/Lib/site-packages/starlette/datastructures.py
- 4: https://github.abhinayambati.codes/repo/AiImpactAnalyzer/blob/backend/venv/Lib/site-packages/starlette/responses.py
- 5: https://github.abhinayambati.codes/repo/AiImpactAnalyzer/blob/backend/venv/Lib/site-packages/starlette/responses.py
Preserve raw headers; dict conversion collapses duplicate headers.
Lines 105–110 convert raw_headers to a dict, which loses all duplicate header keys (e.g., Set-Cookie, Vary). ASGI responses can include multiple headers with the same name, but converting to Mapping[str, str] retains only the last value per key. Instead, preserve the raw ASGI header list and use response.headers.append() to add new headers without collapsing duplicates:
Proposed fix
+from starlette.datastructures import MutableHeaders
...
- headers = {
- (k.decode() if isinstance(k, bytes) else k): (
- v.decode() if isinstance(v, bytes) else v
- )
- for k, v in raw_headers
- }
- response = Response(
- content=b"".join(body_parts),
- status_code=status_code,
- headers=headers,
- )
- response.headers["Cache-Control"] = "no-store, must-revalidate"
- response.headers["ETag"] = etag
+ response = Response(
+ content=b"".join(body_parts),
+ status_code=status_code,
+ )
+ response.raw_headers = list(raw_headers)
+ headers = MutableHeaders(raw=response.raw_headers)
+ headers.append("Cache-Control", "no-store, must-revalidate")
+ headers.append("ETag", etag)
return response🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/server.py` around lines 105 - 115, The current code converts raw_headers
into a dict (headers) which collapses duplicate keys; instead, keep raw_headers
and after creating the Response object (Response(...)), iterate over
raw_headers, decoding keys/values (handle bytes) and call
response.headers.append(decoded_key, decoded_value) for each pair so duplicate
headers like Set-Cookie are preserved (refer to raw_headers, headers variable,
Response, and response.headers.append()).
|
I'm not clear what we're trying to fix here. Do you have logs or a screenshot showing the issue you encountered? Thanks! |
…ace_mcp into claude/fix-cors-middleware-error-sPbo6
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/server.py`:
- Around line 56-58: The current is_oauth_well_known check uses startswith and
thus matches unintended prefixes; change the logic in the is_oauth_well_known
assignment to only accept the exact well-known path or that path followed by a
slash (i.e., use path == "/.well-known/oauth-authorization-server" or
path.startswith("/.well-known/oauth-authorization-server/") and likewise for
"/.well-known/oauth-protected-resource") so you don't match things like
"/.well-known/oauth-authorization-server-extra".
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/core/test_well_known_cache_control_middleware.py (1)
54-88: Consider test isolation for module reload pattern.The test reloads
core.servermodule and modifies global configuration, which could affect subsequent tests in the same process. While this pattern is common for integration tests, consider adding cleanup in a fixture or documenting test ordering requirements.Optional: Add cleanup fixture
import pytest `@pytest.fixture` def reset_server_module(): """Reset server module state after test.""" yield # Restore original module state import core.server as core_server importlib.reload(core_server)Then use:
def test_configured_server_...(monkeypatch, reset_server_module):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/test_well_known_cache_control_middleware.py` around lines 54 - 88, The test mutates and reloads the core.server module and global config (calls reload_oauth_config, importlib.reload(core.server), core.server.set_transport_mode, core.server.configure_server_for_http), which can leak state to other tests; add a cleanup fixture (e.g., reset_server_module) that yields and then importlib.reload(core.server) and any other affected modules (and/or clear env changes applied by monkeypatch) after the test, and include that fixture in the test signature (e.g., def test_configured_server_applies_no_cache_to_served_oauth_discovery_routes(monkeypatch, reset_server_module):) to ensure module state and environment are restored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/core/test_well_known_cache_control_middleware.py`:
- Around line 54-88: The test mutates and reloads the core.server module and
global config (calls reload_oauth_config, importlib.reload(core.server),
core.server.set_transport_mode, core.server.configure_server_for_http), which
can leak state to other tests; add a cleanup fixture (e.g., reset_server_module)
that yields and then importlib.reload(core.server) and any other affected
modules (and/or clear env changes applied by monkeypatch) after the test, and
include that fixture in the test signature (e.g., def
test_configured_server_applies_no_cache_to_served_oauth_discovery_routes(monkeypatch,
reset_server_module):) to ensure module state and environment are restored.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
core/server.pytests/core/test_well_known_cache_control_middleware.py
3 participants
…l arguments
The _wrap_well_known_endpoint function assumed all route endpoints are regular request handlers (async def handler(request) -> Response). However, the MCP SDK's cors_middleware wraps handlers with CORSMiddleware, which is an ASGI app expecting (scope, receive, send). When the wrapper called
await endpoint(request)on a CORSMiddleware instance, it passed only 1 argument instead of the required 3 ASGI args.The fix detects whether the endpoint is a regular handler function or an ASGI app (using the same inspect check as Starlette's Route constructor), and uses the appropriate calling convention:
await endpoint(request)(existing behavior)await endpoint(scope, receive, send)with response capture to apply cache-busting headershttps://claude.ai/code/session_011S5zFTWRfKBJBUEanrhvQg
Description
Brief description of the changes in this PR.
Type of Change
Testing
Checklist
Additional Notes
Add any other context about the pull request here.
To enable this setting:
Summary by CodeRabbit