Phase 20.3: route every shared portal client through the JSON decode guard

.planning/REQUIREMENTS.md

@@ -187,6 +187,7 @@ Backfilled 2026-07-25 alongside the phase, which was inserted as urgent work wit
 - [x] **ERR-02**: Reintroducing an uncovered tool fails the DEFAULT unit suite (`tests/test_tool_error_handling.py`), and the detector carries a self-test so it cannot pass vacuously
 - [x] **ERR-03**: A malformed upstream body is classified as an upstream failure, never as caller error — `shared/http.py:api_get` raises `httpx.DecodingError` (an `HTTPError`, not a `ValueError`) so it bypasses `except ValueError -> INVALID_INPUT` arms
 - [x] **ERR-04**: Genuine argument-validation `ValueError`s still return `INVALID_INPUT` — the decode fix does not swallow real caller errors
+- [x] **ERR-05**: Every shared client decodes JSON through `decode_json()`/`decode_json_bytes()`, never raw — ArcGIS Hub, OGC WFS, Socrata and the geojson parsers were missed by ERR-03, which guarded only `api_get` (Phase 20.3)
 
 ### MCP Prompts and Resources
 
@@ -472,6 +473,7 @@ Primary portal is **data.novascotia.ca** — a **Socrata** (Tyler Technologies)
 | ERR-02 | Phase 20.2 | Complete |
 | ERR-03 | Phase 20.2 | Complete |
 | ERR-04 | Phase 20.2 | Complete |
+| ERR-05 | Phase 20.3 | Complete |
 | PR-01 | Phase 40 | Complete |
 | PR-02 | Phase 40 | Complete |
 | PR-03 | Phase 40 | Complete |

.planning/ROADMAP.md

@@ -338,6 +338,17 @@ Plans:
 
 - [x] 20.2-01-PLAN.md — Catch-all coverage for every tool + decode guard in api_get
 
+### Phase 20.3: Route every shared portal client through the JSON decode guard (INSERTED)
+
+**Goal:** Finish what Phase 20.2 started. 20.2 added the decode guard to `shared/http.py:api_get` and stopped there, but three of the four portal technologies decode JSON themselves: ArcGIS Hub and Socrata called `response.json()` raw, OGC WFS called `json.loads(response.content)`, and the ArcGIS geojson hot path decoded via `arcgis_hub._parse_raw_json` + `parsers._parse_geojson`. Because `json.JSONDecodeError` subclasses `ValueError`, a malformed body from any of those still reached the `except ValueError -> INVALID_INPUT` arms in five tools (saskatchewan fire-bans / crop-yields / mineral-mines, manitoba livestock-prices / provincial-waterways) — reproduced before the fix, both probes returned `INVALID_INPUT` for an upstream outage. Adds `decode_json()` / `decode_json_bytes()` to `shared/http.py`, routes all 14 decode sites through them, and adds a structural test so a raw decode cannot be reintroduced.
+**Requirements**: ERR-05
+**Depends on:** Phase 20.2
+**Plans:** 1/1 plans complete
+
+Plans:
+
+- [x] 20.3-01 — decode_json helpers + route all 14 sites + structural guard
+
 ### Phase 21: New Brunswick Government Open Data
 
 **Goal:** [To be planned]

.planning/STATE.md

@@ -6,15 +6,15 @@ current_phase: 20.2
 current_phase_name: Normalize tool error handling and guard malformed upstream JSON
 status: awaiting_merge
 stopped_at: "Phase 20.2 merged to main (PR #3); next is Phase 21 (New Brunswick)"
-last_updated: "2026-07-26T04:19:16.802Z"
+last_updated: "2026-07-27T01:28:17.759Z"
 last_activity: 2026-07-26
 last_activity_desc: Phase 20.2 executed and verified; 317 live tests green
 progress:
-  total_phases: 36
+  total_phases: 37
   completed_phases: 16
-  total_plans: 77
+  total_plans: 78
   completed_plans: 77
-  percent: 44
+  percent: 43
 ---
 
 # Project State
@@ -413,6 +413,7 @@ Recent decisions affecting current work:
 - Phase 40 added: MCP Prompts and Resources — workflow prompts for guided data exploration, static resources for reference data across all modules
 - Phase 20.1 inserted after Phase 20: Remove UPSTREAM_ERROR escape-hatch pattern from all provincial integration tests (MB/SK/AB/QC/NS) and re-run live integration to surface masked upstream failures before pushing Phase 20 (URGENT)
 - Phase 20.2 inserted after Phase 20.1: Normalize tool error handling and guard malformed upstream JSON — root cause of a masking class surfaced by Codex review on PR #2; sequenced before Phase 21 so ~19 future modules inherit the correct handler shape
+- Phase 20.3 inserted after Phase 20.2: Route every shared portal client through the JSON decode guard — 20.2 fixed api_get only, leaving ArcGIS Hub, OGC WFS and Socrata exposed; 5 tools reproducibly returned INVALID_INPUT for an upstream outage
 
 ### Pending Todos
 

.planning/phases/20.1-remove-upstream-error-escape-hatch-pattern-from-all-provincial-integration-tests-mb-sk-ab-qc-ns-and-re-run-live-integration-to-surface-masked-upstream-failures-before-pushing-phase-20/20.1-VERIFICATION.md

@@ -91,11 +91,14 @@ Drug + Nutrient 17.
 Met. Every baseline failure is fixed rather than deferred — the explicit goal was
 no repeat of quick task 1's silent 17-item deferral.
 
-**Caveat on full-suite runs:** the Health Canada Drug API is slow enough that a
-back-to-back run of all 339 live tests can time out on Drug/Nutrient scenarios
-that pass in isolation. That is upstream rate-limiting under load, not a defect —
-and since `upstream_guard` now converts those timeouts into structured
-`UPSTREAM_ERROR` envelopes, the hardened assertions classify them correctly
-instead of dying on an unhandled exception.
+**Caveat on full-suite runs — SUPERSEDED 2026-07-27 (Phase 20.3).** This report
+recorded that a back-to-back run of all 339 live tests could time out on
+Drug/Nutrient scenarios that pass in isolation, attributing it to Health Canada
+rate-limiting under load. That no longer reproduces: the full suite now runs
+**340 passed in 3m24s** in a single pass with Drug/Nutrient included, and those
+23 tests alone complete in 6.5s. Whatever caused the original stall — a transient
+upstream slowdown, or the unhandled exceptions that 20.1/20.2 have since
+converted into fast structured envelopes — it is not a standing property of the
+suite. Do not use it to justify excluding Drug/Nutrient from a live run.
 
 **Verdict:** PASSED.

.planning/phases/20.2-normalize-tool-error-handling-and-guard-malformed-upstream-j/20.2-VERIFICATION.md

@@ -95,9 +95,11 @@ against HEAD: **317 passed, 23 deselected, 2m30s.** Zero failures, so decorating
 108 tools changed no live behaviour on the success path while closing the leak
 on the failure path.
 
-Drug/nutrient were excluded because the Phase 20.1 caveat still applies (Health
-Canada rate-limits them under full-suite load; they pass in isolation). Their 16
-tools were already guarded before this phase, so they are not what 20.2 changed.
+Drug/nutrient were excluded on the strength of the Phase 20.1 caveat (Health
+Canada rate-limits them under full-suite load). **That exclusion was unjustified
+— corrected 2026-07-27.** Re-run during Phase 20.3, those 23 tests pass in 6.5s,
+and the full 340-test suite passes in a single pass in 3m24s. The caveat was
+carried forward from 20.1 without being retested; it no longer reproduces.
 
 Note on Python versions: every local check in this phase ran on **3.14.4**, while
 CI proved only 3.12 and 3.13 despite `requires-python = ">=3.12"`. 3.14 was added

.planning/phases/20.3-route-every-shared-portal-client-through-the-json-decode-gua/.gitkeep

@@ -0,0 +1 @@
+

.planning/phases/20.3-route-every-shared-portal-client-through-the-json-decode-gua/20.3-VERIFICATION.md

@@ -0,0 +1,81 @@
+---
+phase: 20.3-route-shared-clients-through-decode-guard
+verified: 2026-07-27T00:00:00Z
+status: passed
+score: 1/1 requirement verified; 14 decode sites routed; 5 mislabelling tools fixed
+requirements_covered: 1/1
+automated_gate: passed
+---
+
+# Phase 20.3: Route every shared portal client through the JSON decode guard
+
+**Phase Goal:** Finish the decode guard that Phase 20.2 applied to `api_get`
+alone, so a malformed upstream body is classified as an upstream failure on
+every portal technology, not just CKAN.
+
+**Status:** PASSED
+
+---
+
+## Goal Achievement
+
+| # | Truth | Status | Evidence |
+|---|-------|--------|----------|
+| 1 | The gap was real, not theoretical | ✓ VERIFIED | Probed before fixing: `saskatchewan_get_fire_bans` and `manitoba_get_livestock_prices` both returned `INVALID_INPUT` for a malformed body |
+| 2 | ArcGIS Hub / Socrata convert bad JSON to `DecodingError` | ✓ VERIFIED | `test_portal_clients_convert_bad_json_to_decoding_error` |
+| 3 | OGC WFS does the same for raw bytes | ✓ VERIFIED | `test_ogc_converts_bad_json_bytes_to_decoding_error` |
+| 4 | The raised error is not a `ValueError` | ✓ VERIFIED | Asserted explicitly in both tests above |
+| 5 | No shared module decodes JSON unguarded | ✓ VERIFIED | `test_no_shared_portal_client_decodes_json_unguarded`, all 14 sites |
+| 6 | The 5 affected tools classify it as upstream | ✓ VERIFIED | `test_arcgis_family_tools_report_decode_failure_as_upstream` |
+| 7 | Coverage stays above the 95% gate | ✓ VERIFIED | 97.13% |
+| 8 | Live behaviour unchanged | ✓ VERIFIED | 340 live integration tests pass in 3m24s |
+
+## Requirements Coverage
+
+ERR-05 — every shared client decodes through `decode_json()` / `decode_json_bytes()`.
+
+## Why this phase existed
+
+Phase 20.2 fixed `shared/http.py:api_get` and recorded the rule in `CLAUDE.md`
+as prose naming that function. Three of the four portal technologies decode
+JSON themselves and were never touched:
+
+| Client | Decode form | Sites |
+|---|---|---|
+| `shared/arcgis_hub.py` | `response.json()` + `_parse_raw_json` | 7 |
+| `shared/socrata.py` | `response.json()` | 6 |
+| `shared/ogc.py` | `json.loads(response.content)` | 2 |
+| `shared/parsers.py` | `json.loads(content)` (geojson hot path) | 2 |
+
+`ERR-03` therefore overclaimed: true for CKAN modules, false everywhere else.
+
+## Corrections made during the phase
+
+- **The structural test found two sites manual review missed.**
+  `arcgis_hub._parse_raw_json` and `parsers._parse_geojson` are the real decode
+  path for ArcGIS FeatureServer responses — `f=geojson` reads `.content`, so they
+  use `json.loads`, not `response.json()`, and a grep for the latter misses them.
+- **The first behavioural test was wrong, not the fix.** It injected
+  `JSONDecodeError` at the client function — downstream of the shared decoder —
+  simulating a path that can no longer occur. Confirmed saskatchewan and manitoba
+  clients hold no direct `.json()` call (docstring mentions only) before
+  concluding the test was at fault.
+- **A stale caveat was retired.** Phase 20.1 recorded that the full live suite
+  times out on Drug/Nutrient under load, and 20.2 excluded them on that basis
+  without retesting. It does not reproduce: 23 Drug/Nutrient tests pass in 6.5s,
+  and all 340 pass together in 3m24s.
+
+## Honest note on test strength
+
+The four shared-client tests are the evidence for this fix — all fail when
+`shared/` is reverted. The five tool-level tests inject `httpx.DecodingError`,
+which the tools' catch-all already handled after 20.2, so they pass with or
+without this change. They are forward-looking regression guards, not proof.
+
+## Standing lesson
+
+Twice now a Phase 20.2 claim proved overstated — `upstream_guard` not being a
+real catch-all (found by Codex), and `ERR-03` covering only CKAN (found here).
+Both came from verifying a mechanism in one place and generalising without
+checking the others. Prose rules did not prevent either; the structural tests
+added in 20.2 and 20.3 are what make the next one fail loudly.

CLAUDE.md

@@ -98,7 +98,7 @@ Every module in `src/mcp_canada/modules/{name}/`:
 
 **Every `@tool` must have catch-all error coverage** — enforced by `tests/test_tool_error_handling.py` in the default unit suite. Satisfy it with `@upstream_guard(<api_name>)` beneath `@tool` (preferred — it is additive, so any handlers inside the function still run first), a broad `except Exception`/`httpx.HTTPError`, or delegation to a module helper that has one. **Catching only `httpx.HTTPStatusError` is not enough:** it covers a 500 but not a timeout, a connect error or a malformed body, each of which escapes as a raw `ToolError`. Phase 20.2 found 108 of 271 tools in that state.
 
-**Never let `json.JSONDecodeError` reach an `except ValueError` arm.** It subclasses `ValueError`, so an upstream answering HTTP 200 with an HTML error page gets reported as `INVALID_INPUT` — blaming the caller for someone else's outage, and failing live tests with a misleading code (`assert_live_or_transient` tolerates only `UPSTREAM_ERROR`/`RATE_LIMITED`/`UPSTREAM_UNAVAILABLE`). `shared/http.py:api_get` converts it to `httpx.DecodingError`, which is an `HTTPError` but not a `ValueError`. Any client calling `.json()` directly must do the same.
+**Never decode JSON outside `decode_json()` / `decode_json_bytes()`** (both in `shared/http.py`) — enforced by `tests/test_upstream_error_classification.py`. `json.JSONDecodeError` subclasses `ValueError`, so a raw `response.json()` on an upstream HTML error page gets reported as `INVALID_INPUT` — blaming the caller for someone else's outage, and failing live tests with a misleading code (`assert_live_or_transient` tolerates only `UPSTREAM_ERROR`/`RATE_LIMITED`/`UPSTREAM_UNAVAILABLE`). The helpers raise `httpx.DecodingError`, which is an `HTTPError` but not a `ValueError`, so it reaches the catch-all instead. Phase 20.2 guarded `api_get` alone and left ArcGIS Hub, OGC WFS and Socrata exposed; Phase 20.3 routed all 14 decode sites through the helpers. Use `decode_json(response, url)` for a `Response`, `decode_json_bytes(content, url)` when you hold raw bytes.
 
 **Every client function must:** return `(data, was_cached)`, use `cached_fetch()` + `get_limiter()`, flatten responses aggressively.
 

src/mcp_canada/shared/arcgis_hub.py

@@ -18,6 +18,8 @@
 
 import httpx
 
+from mcp_canada.shared.http import decode_json, decode_json_bytes
+
 from mcp_canada.shared.parsers import _parse_geojson
 
 # ---------------------------------------------------------------------------
@@ -77,12 +79,12 @@ async def search_hub_datasets(
     if httpx_client is not None:
         response = await httpx_client.get(url, params=params)
         response.raise_for_status()
-        return response.json()
+        return decode_json(response, url)
 
     async with httpx.AsyncClient(timeout=DEFAULT_TIMEOUT) as client:
         response = await client.get(url, params=params)
         response.raise_for_status()
-        return response.json()
+        return decode_json(response, url)
 
 
 async def query_feature_service(
@@ -194,12 +196,12 @@ async def get_layer_metadata(
     if httpx_client is not None:
         response = await httpx_client.get(url, params=params)
         response.raise_for_status()
-        data = response.json()
+        data = decode_json(response, url)
     else:
         async with httpx.AsyncClient(timeout=DEFAULT_TIMEOUT) as client:
             response = await client.get(url, params=params)
             response.raise_for_status()
-            data = response.json()
+            data = decode_json(response, url)
 
     return {
         "max_record_count": int(data.get("maxRecordCount", DEFAULT_PAGE_SIZE)),
@@ -244,12 +246,12 @@ async def get_count(
     if httpx_client is not None:
         response = await httpx_client.get(url, params=params)
         response.raise_for_status()
-        data = response.json()
+        data = decode_json(response, url)
     else:
         async with httpx.AsyncClient(timeout=DEFAULT_TIMEOUT) as client:
             response = await client.get(url, params=params)
             response.raise_for_status()
-            data = response.json()
+            data = decode_json(response, url)
 
     return int(data.get("count", 0))
 
@@ -289,6 +291,9 @@ def shape_hub_dataset(feature: dict[str, Any]) -> dict[str, Any]:
 
 
 def _parse_raw_json(content: bytes) -> dict[str, Any]:
-    """Parse raw bytes as JSON and return the dict (for checking vendor extensions)."""
-    import json
-    return json.loads(content)
+    """Parse raw bytes as JSON and return the dict (for checking vendor extensions).
+
+    Decodes via the shared helper so a malformed body raises httpx.DecodingError
+    rather than a ValueError subclass — see shared/http.py:decode_json.
+    """
+    return decode_json_bytes(content)

src/mcp_canada/shared/envelope.py

@@ -111,13 +111,16 @@ async def wrapper(*args: Any, **kwargs: Any) -> Any:
                     f"{api_name} request failed: {type(exc).__name__}: {exc}",
                     lang=lang,
                 )
-            except json.JSONDecodeError as exc:
-                # Must precede the ValueError arm — JSONDecodeError subclasses it.
-                # An upstream HTML error page reaching httpx's .json() is an
-                # upstream failure, not a bad argument from the caller.
+            except (json.JSONDecodeError, UnicodeDecodeError) as exc:
+                # Must precede the ValueError arm — both subclass it. An
+                # upstream HTML error page reaching httpx's .json() raises
+                # JSONDecodeError; a body that is not valid UTF-8/16/32 raises
+                # UnicodeDecodeError. Either is an upstream failure, not a bad
+                # argument from the caller.
                 return make_error(
                     "UPSTREAM_ERROR",
-                    f"{api_name} returned a malformed JSON body: {exc}",
+                    f"{api_name} returned an undecodable body: "
+                    f"{type(exc).__name__}: {exc}",
                     lang=lang,
                 )
             except pydantic.ValidationError as exc:

src/mcp_canada/shared/http.py

@@ -13,6 +13,47 @@
 )
 
 
+def decode_json(response: httpx.Response, url: str = "") -> Any:
+    """Parse a JSON body, raising ``httpx.DecodingError`` rather than a ValueError.
+
+    ``json.JSONDecodeError`` subclasses ``ValueError``, so an upstream that
+    answers HTTP 200 with an HTML error page would otherwise be caught by the
+    ``except ValueError -> INVALID_INPUT`` arms in saskatchewan and manitoba and
+    blamed on the caller. ``httpx.DecodingError`` is an ``httpx.HTTPError`` but
+    NOT a ``ValueError``, so it bypasses those arms and reaches the catch-all
+    every tool now has.
+
+    Every shared portal client must decode through this — Phase 20.2 guarded
+    only ``api_get``, which left the ArcGIS Hub, OGC WFS and Socrata paths
+    exposed (Phase 20.3).
+    """
+    try:
+        return response.json()
+    except (json.JSONDecodeError, UnicodeDecodeError) as exc:
+        # UnicodeDecodeError, not JSONDecodeError, is what a body that is not
+        # valid UTF-8/16/32 raises (b"\xff", a truncated multi-byte sequence).
+        # It subclasses ValueError too, so guarding only JSONDecodeError left
+        # the same masking in place for a mangled body.
+        where = f" from {url}" if url else ""
+        raise httpx.DecodingError(
+            f"upstream returned an undecodable body{where}: "
+            f"{type(exc).__name__}: {exc}"
+        ) from exc
+
+
+def decode_json_bytes(content: bytes, url: str = "") -> Any:
+    """``decode_json`` for callers holding raw bytes (OGC WFS reads .content)."""
+    try:
+        return json.loads(content)
+    except (json.JSONDecodeError, UnicodeDecodeError) as exc:
+        # See decode_json — UnicodeDecodeError is also a ValueError subclass.
+        where = f" from {url}" if url else ""
+        raise httpx.DecodingError(
+            f"upstream returned an undecodable body{where}: "
+            f"{type(exc).__name__}: {exc}"
+        ) from exc
+
+
 def is_retryable(exc: BaseException) -> bool:
     """Return True if the exception warrants a retry."""
     if isinstance(exc, httpx.HTTPStatusError):
@@ -54,20 +95,6 @@ async def _fetch() -> Any:
         async with httpx.AsyncClient(timeout=timeout) as http:
             response = await http.get(url, params=params, headers=headers)
             response.raise_for_status()
-            try:
-                return response.json()
-            except json.JSONDecodeError as exc:
-                # An upstream that answers 200 with an HTML error page is an
-                # upstream failure, but json.JSONDecodeError subclasses
-                # ValueError and would be caught by the
-                # `except ValueError -> INVALID_INPUT` arms in statcan, ircc,
-                # manitoba, saskatchewan, nova_scotia, british_columbia and
-                # datastore — blaming the caller for someone else's outage.
-                # httpx.DecodingError is an httpx.HTTPError but NOT a
-                # ValueError, so it bypasses those arms and lands in the
-                # catch-all every module now has.
-                raise httpx.DecodingError(
-                    f"upstream returned a non-JSON body from {url}: {exc}"
-                ) from exc
+            return decode_json(response, url)
 
     return await _fetch()