Skip to content

Commit 7ce47d7

Browse files
phernandezclaude
andcommitted
refactor(mcp): inline _request_scope and tighten _extract_response_data
Two more simplifications in utils.py the reviewer/PM flagged: 1. _request_scope was a no-op wrapper around logfire.span that added nothing — same name, same attributes, one call deep. Inlined at all 5 call sites (call_get/put/post/patch/delete). The helper is gone. 2. _extract_response_data used `except Exception` to swallow any JSON decode failure. That's defensive code for upstream gateway HTML error pages (Fly/Cloudflare 5xx); FastAPI itself always emits JSON. Replaced with a content-type check: if the response doesn't claim `application/json`, return None; otherwise let any decode error surface. Matches CLAUDE.md's fail-fast guidance. Tests updated: MockResponse fixtures now include a `headers = {"content-type": "application/json"}` attribute so they pass the new gate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
1 parent 85ce163 commit 7ce47d7

3 files changed

Lines changed: 40 additions & 45 deletions

File tree

src/basic_memory/mcp/tools/utils.py

Lines changed: 38 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -109,15 +109,20 @@ def get_error_message(
109109
return f"HTTP error {status_code}: {method} request to '{path}' failed"
110110

111111

112-
def _extract_response_data(response: Response) -> typing.Any:
113-
"""Safely decode response payload for error reporting."""
114-
try:
115-
return response.json()
116-
except Exception:
112+
def _extract_response_data(response: Response) -> Any:
113+
"""Decode the JSON payload of an API response for error reporting.
114+
115+
Upstream gateways (Fly, Cloudflare, load balancers) can return HTML
116+
error pages before the request reaches our FastAPI app; those have no
117+
structured `detail` to surface, so we skip them. A malformed body with
118+
a JSON content-type is a server bug and we let it raise.
119+
"""
120+
if "application/json" not in response.headers.get("content-type", ""):
117121
return None
122+
return response.json()
118123

119124

120-
def _response_detail_text(response_data: typing.Any) -> str | None:
125+
def _response_detail_text(response_data: Any) -> str | None:
121126
"""Extract textual error detail from API payloads."""
122127
if isinstance(response_data, dict):
123128
detail = response_data.get("detail")
@@ -168,30 +173,6 @@ def _resolve_error_message(
168173
return get_error_message(status_code, url, method)
169174

170175

171-
@contextmanager
172-
def _request_scope(
173-
method: str,
174-
*,
175-
client_name: str | None,
176-
operation: str | None,
177-
path_template: str | None,
178-
params: QueryParamTypes | None = None,
179-
has_body: bool = False,
180-
):
181-
"""Create the shared MCP transport span used by all HTTP helpers."""
182-
with logfire.span(
183-
"mcp.http.request",
184-
method=method,
185-
client_name=client_name,
186-
operation=operation,
187-
path_template=path_template,
188-
phase="request",
189-
has_query=bool(params),
190-
has_body=has_body,
191-
) as active_span:
192-
yield active_span
193-
194-
195176
async def call_get(
196177
client: AsyncClient,
197178
url: URL | str,
@@ -231,12 +212,15 @@ async def call_get(
231212
request_span: logfire.LogfireSpan | None = None
232213

233214
try:
234-
with _request_scope(
235-
"GET",
215+
with logfire.span(
216+
"mcp.http.request",
217+
method="GET",
236218
client_name=client_name,
237219
operation=operation,
238220
path_template=path_template,
239-
params=params,
221+
phase="request",
222+
has_query=bool(params),
223+
has_body=False,
240224
) as request_span:
241225
response = await client.get(
242226
url,
@@ -328,12 +312,14 @@ async def call_put(
328312
request_span: logfire.LogfireSpan | None = None
329313

330314
try:
331-
with _request_scope(
332-
"PUT",
315+
with logfire.span(
316+
"mcp.http.request",
317+
method="PUT",
333318
client_name=client_name,
334319
operation=operation,
335320
path_template=path_template,
336-
params=params,
321+
phase="request",
322+
has_query=bool(params),
337323
has_body=any(value is not None for value in (content, data, files, json)),
338324
) as request_span:
339325
response = await client.put(
@@ -430,12 +416,14 @@ async def call_patch(
430416
request_span: logfire.LogfireSpan | None = None
431417

432418
try:
433-
with _request_scope(
434-
"PATCH",
419+
with logfire.span(
420+
"mcp.http.request",
421+
method="PATCH",
435422
client_name=client_name,
436423
operation=operation,
437424
path_template=path_template,
438-
params=params,
425+
phase="request",
426+
has_query=bool(params),
439427
has_body=any(value is not None for value in (content, data, files, json)),
440428
) as request_span:
441429
response = await client.patch(
@@ -538,12 +526,14 @@ async def call_post(
538526
request_span: logfire.LogfireSpan | None = None
539527

540528
try:
541-
with _request_scope(
542-
"POST",
529+
with logfire.span(
530+
"mcp.http.request",
531+
method="POST",
543532
client_name=client_name,
544533
operation=operation,
545534
path_template=path_template,
546-
params=params,
535+
phase="request",
536+
has_query=bool(params),
547537
has_body=any(value is not None for value in (content, data, files, json)),
548538
) as request_span:
549539
response = await client.post(
@@ -665,12 +655,15 @@ async def call_delete(
665655
request_span: logfire.LogfireSpan | None = None
666656

667657
try:
668-
with _request_scope(
669-
"DELETE",
658+
with logfire.span(
659+
"mcp.http.request",
660+
method="DELETE",
670661
client_name=client_name,
671662
operation=operation,
672663
path_template=path_template,
673-
params=params,
664+
phase="request",
665+
has_query=bool(params),
666+
has_body=False,
674667
) as request_span:
675668
response = await client.delete(
676669
url=url,

tests/mcp/test_tool_utils.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ def __init__(self, status_code=200):
2424
self.status_code = status_code
2525
self.is_success = status_code < 400
2626
self.json = lambda: {}
27+
self.headers = {"content-type": "application/json"}
2728

2829
def raise_for_status(self):
2930
if self.status_code >= 400:

tests/mcp/test_tool_utils_cloud_auth.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ def __init__(self, status_code: int, payload: dict):
1414
self.status_code = status_code
1515
self._payload = payload
1616
self.is_success = status_code < 400
17+
self.headers = {"content-type": "application/json"}
1718

1819
def json(self):
1920
return self._payload

0 commit comments

Comments
 (0)