Skip to content

Commit 50b314b

Browse files
committed
fix(envelope): classify malformed upstream JSON as UPSTREAM_ERROR
Addresses Codex review on PR #2 (P2, shared/envelope.py:113). json.JSONDecodeError subclasses ValueError, so when Health Canada answers HTTP 200 with an HTML error page, httpx's .json() raised inside a guarded tool and fell through to the ValueError arm — reporting INVALID_INPUT and blaming the caller for an upstream outage. It also broke live tests in a confusing way: assert_live_or_transient tolerates only UPSTREAM_ERROR, RATE_LIMITED and UPSTREAM_UNAVAILABLE, so a real outage failed as if the arguments were wrong. Handle JSONDecodeError ahead of the ValueError arm. A second test pins that genuine argument-validation ValueErrors still return INVALID_INPUT, so the fix cannot swallow real caller errors. Verified the mechanism rather than assuming it: JSONDecodeError does subclass ValueError, httpx .json() raises it on an HTML body, and both the drug and nutrient clients call .json() inside the guarded path. Also records in STATE.md that the same masking is latent in ~40 ValueError arms downstream of shared/http.py:api_get, which has no decode guard. That is pre-existing and best fixed once inside api_get; left out of this PR.
1 parent ccbaa6e commit 50b314b

3 files changed

Lines changed: 61 additions & 0 deletions

File tree

.planning/STATE.md

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,16 @@ Open defect (found 2026-07-26, deferred to its own phase):
8989
behind an env-var key like Manitoba 511. Alberta AHS, parks, forest-area and
9090
Saskatchewan/Manitoba Hub services were probed at the same time and are healthy.
9191

92+
- **Malformed-JSON masking is broader than the spot Codex flagged.** `upstream_guard`
93+
is fixed, which covers drug_database and nutrient_file (they use their own clients).
94+
But `shared/http.py:api_get` returns `response.json()` with no decode guard, and
95+
~40 `except ValueError -> INVALID_INPUT` arms across statcan, ircc, manitoba,
96+
saskatchewan, nova_scotia, british_columbia and datastore sit downstream of it.
97+
An upstream HTML error page therefore still surfaces as caller error in those
98+
modules. Pre-existing, not introduced by Phase 20.1. The high-leverage fix is one
99+
decode guard inside `api_get` rather than 40 call-site edits — deliberately left
100+
out of PR #2 to keep it scoped.
101+
92102
Remaining backlog (not defects):
93103

94104
- `.planning/todos/pending/2026-04-12-research-cross-canada-er-wait-times-datasets.md`

src/mcp_canada/shared/envelope.py

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
"""Response envelope and error builders for standardized tool responses."""
22

33
import functools
4+
import json
45
import httpx
56
from collections.abc import Callable
67

@@ -109,6 +110,15 @@ async def wrapper(*args: Any, **kwargs: Any) -> Any:
109110
f"{api_name} request failed: {type(exc).__name__}: {exc}",
110111
lang=lang,
111112
)
113+
except json.JSONDecodeError as exc:
114+
# Must precede the ValueError arm — JSONDecodeError subclasses it.
115+
# An upstream HTML error page reaching httpx's .json() is an
116+
# upstream failure, not a bad argument from the caller.
117+
return make_error(
118+
"UPSTREAM_ERROR",
119+
f"{api_name} returned a malformed JSON body: {exc}",
120+
lang=lang,
121+
)
112122
except ValueError as exc:
113123
return make_error("INVALID_INPUT", str(exc), lang=lang)
114124
return wrapper

tests/test_envelope.py

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,47 @@ async def boom(lang: str = "en") -> dict:
120120
assert result["error"]["code"] == "UPSTREAM_ERROR"
121121
assert "503" in result["error"]["message"]
122122

123+
@pytest.mark.asyncio
124+
async def test_malformed_json_body_is_an_upstream_error_not_invalid_input(self):
125+
"""HTTP 200 carrying HTML must blame the upstream, not the caller.
126+
127+
json.JSONDecodeError subclasses ValueError, so httpx's .json() raising on
128+
a Health Canada error page fell through to the ValueError handler and came
129+
back as INVALID_INPUT — a valid tool call blamed for an upstream outage.
130+
assert_live_or_transient tolerates only UPSTREAM_ERROR/RATE_LIMITED/
131+
UPSTREAM_UNAVAILABLE, so this also broke live tests with a misleading code.
132+
"""
133+
import httpx
134+
from mcp_canada.shared.envelope import upstream_guard
135+
136+
@upstream_guard("test-api")
137+
async def boom(lang: str = "en") -> dict:
138+
response = httpx.Response(
139+
200,
140+
text="<html>503 Service Unavailable</html>",
141+
request=httpx.Request("GET", "https://example.test"),
142+
)
143+
return response.json()
144+
145+
result = await boom()
146+
assert result["error"]["code"] == "UPSTREAM_ERROR", (
147+
f"malformed upstream JSON must not be reported as caller error: {result}"
148+
)
149+
assert "test-api" in result["error"]["message"]
150+
151+
@pytest.mark.asyncio
152+
async def test_genuine_value_error_is_still_invalid_input(self):
153+
"""The JSONDecodeError fix must not swallow real argument validation."""
154+
from mcp_canada.shared.envelope import upstream_guard
155+
156+
@upstream_guard("test-api")
157+
async def boom(lang: str = "en") -> dict:
158+
raise ValueError("din must be 8 digits")
159+
160+
result = await boom()
161+
assert result["error"]["code"] == "INVALID_INPUT"
162+
assert "din must be 8 digits" in result["error"]["message"]
163+
123164
@pytest.mark.asyncio
124165
async def test_lang_is_propagated(self):
125166
import httpx

0 commit comments

Comments
 (0)