Skip to content

Commit 777fa44

Browse files
committed
fix(york_region): honour the classified markers in _call_client
Addresses the Codex P2 on PR #5. Correct — and the same generalisation gap as the previous three findings: I converted the raise in york_region/client.py without checking the handler that catches it. york_region carries no @upstream_guard; its _call_client helper is the catch-all (that is why Phase 20.2 skipped its 28 tools). So NotFound raised for an unknown dataset id fell past NoPortalError and the HTTP arms into the generic `except Exception` and a routine missing record was reported as an upstream outage. Marker arms now come first. Audited the rest behaviourally rather than assuming york_region was the only one: every module whose own client raises a marker now returns the right code (york_region/manitoba/saskatchewan/statcan NotFound, ircc/saskatchewan/ manitoba InvalidInput). A first pass over ALL tools reported 118 mishandlings — that was my probe passing junk arguments like station="x", which trips each tool's own validation before the mock is reached. Restricted to real paths, mishandlings are zero. test_markers_survive_the_module_handler covers the four modules that raise markers, so a handler that drops one fails loudly. Verified non-vacuous: reverting york_region/tools.py fails exactly that case. 3170 passed, 97.15% coverage.
1 parent 5131cb5 commit 777fa44

2 files changed

Lines changed: 58 additions & 2 deletions

File tree

src/mcp_canada/modules/york_region/tools.py

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
)
3333
from mcp_canada.modules.york_region.constants import PORTAL_URLS
3434
from mcp_canada.shared.envelope import make_error, make_response
35+
from mcp_canada.shared.errors import InvalidInput, NotFound, UpstreamData
3536

3637
API_NAME = "arcgis-hub"
3738

@@ -49,8 +50,14 @@ async def _call_client(
4950
) -> dict[str, Any]:
5051
"""Run a client coroutine and wrap result with make_response / make_error.
5152
52-
Centralises NoPortalError -> NOT_FOUND, HTTP 404 -> NOT_FOUND,
53-
other HTTPStatusError -> UPSTREAM_ERROR, generic Exception -> UPSTREAM_ERROR.
53+
Centralises the classified markers, NoPortalError -> NOT_FOUND,
54+
HTTP 404 -> NOT_FOUND, other HTTPStatusError -> UPSTREAM_ERROR,
55+
generic Exception -> UPSTREAM_ERROR.
56+
57+
The marker arms must come first. These tools carry no ``@upstream_guard``
58+
(this helper is their catch-all), so without them an unknown dataset id
59+
raised as ``NotFound`` fell into the generic arm below and a routine
60+
missing record was reported as an upstream outage.
5461
"""
5562
try:
5663
data, cached = await coro
@@ -61,6 +68,12 @@ async def _call_client(
6168
cached=cached,
6269
lang=lang,
6370
)
71+
except InvalidInput as e:
72+
return make_error("INVALID_INPUT", str(e), lang=lang)
73+
except NotFound as e:
74+
return make_error("NOT_FOUND", str(e), lang=lang)
75+
except UpstreamData as e:
76+
return make_error("UPSTREAM_ERROR", f"upstream returned unusable data: {e}", lang=lang)
6477
except NoPortalError as e:
6578
return make_error("NOT_FOUND", str(e), lang=lang)
6679
except httpx.HTTPStatusError as e:

tests/test_error_classification_defaults.py

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -169,3 +169,46 @@ def test_invalid_input_is_a_valueerror_for_backwards_compatibility():
169169
from mcp_canada.shared.errors import InvalidInput
170170

171171
assert issubclass(InvalidInput, ValueError)
172+
173+
174+
@pytest.mark.asyncio
175+
@pytest.mark.parametrize(
176+
("module", "client_attr", "tool_name", "kwargs", "marker", "expected"),
177+
[
178+
("york_region", "fetch_get_dataset_details", "york_region_get_dataset_details",
179+
{"dataset_id": "bogus"}, "NotFound", "NOT_FOUND"),
180+
("manitoba", "fetch_livestock_prices", "manitoba_get_livestock_prices",
181+
{}, "InvalidInput", "INVALID_INPUT"),
182+
("saskatchewan", "fetch_fire_bans", "saskatchewan_get_fire_bans",
183+
{"ban_scope": "urban"}, "InvalidInput", "INVALID_INPUT"),
184+
("ircc", "fetch_permanent_residents", "ircc_get_permanent_residents",
185+
{}, "InvalidInput", "INVALID_INPUT"),
186+
],
187+
)
188+
async def test_markers_survive_the_module_handler(
189+
module, client_attr, tool_name, kwargs, marker, expected
190+
):
191+
"""Classifying at the raise site is useless if the handler drops it.
192+
193+
york_region carries no @upstream_guard — its `_call_client` helper is the
194+
catch-all — so a NotFound raised for an unknown dataset id fell into the
195+
generic arm and a routine missing record was reported as an upstream
196+
outage. Caught by Codex on PR #5. This covers each module whose own client
197+
raises a marker.
198+
"""
199+
import importlib
200+
from unittest.mock import AsyncMock, patch
201+
202+
import mcp_canada.shared.errors as errors
203+
204+
tools = importlib.import_module(f"mcp_canada.modules.{module}.tools")
205+
client = importlib.import_module(f"mcp_canada.modules.{module}.client")
206+
target = tools if hasattr(tools, client_attr) else client
207+
exc = getattr(errors, marker)("probe")
208+
209+
with patch.object(target, client_attr, new=AsyncMock(side_effect=exc)):
210+
result = await getattr(tools, tool_name)(**kwargs)
211+
212+
assert result["error"]["code"] == expected, (
213+
f"{module}.{tool_name} dropped a {marker} raised by its client"
214+
)

0 commit comments

Comments
 (0)