Phase 20.3: route every shared portal client through the JSON decode guard - #4
Conversation
Phase 20.2 guarded shared/http.py:api_get and stopped there. The other three portal technologies decode JSON themselves and were left exposed: ArcGIS Hub and Socrata called response.json() raw, OGC WFS called json.loads on response.content, and the ArcGIS geojson hot path decoded through arcgis_hub._parse_raw_json and parsers._parse_geojson. json.JSONDecodeError subclasses ValueError, so a malformed body from any of those reached the `except ValueError -> INVALID_INPUT` arms in five tools — saskatchewan_get_fire_bans / _crop_yields / _mineral_mines and manitoba_get_livestock_prices / _provincial_waterways. Reproduced before fixing: both probes returned INVALID_INPUT for what is an upstream outage. Adds decode_json() and decode_json_bytes() to shared/http.py and routes all 14 decode sites through them, api_get included. A structural test now fails if any shared module decodes JSON without them — this phase exists precisely because one client was fixed and three were not. Honest note on the tests: the four shared-client tests are the ones that prove the fix (all fail when shared/ is reverted). The five tool-level tests inject httpx.DecodingError, which the tools' catch-all already handled, so they pass either way — they are forward-looking regression guards, not evidence. 3154 passed, 97.13% coverage.
The CLAUDE.md rule previously named api_get specifically, which is how three other portal clients were missed. It now names the helpers and is backed by a structural test rather than prose.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09b101a5f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| """ | ||
| try: | ||
| return response.json() | ||
| except json.JSONDecodeError as exc: |
There was a problem hiding this comment.
When an upstream returns bytes that are not valid UTF-8/16/32 (for example b"\xff"), both response.json() and json.loads(content) raise UnicodeDecodeError, not json.JSONDecodeError. Because UnicodeDecodeError is also a ValueError, it escapes these new guards and the Saskatchewan/Manitoba except ValueError arms still report the upstream failure as INVALID_INPUT, contrary to the purpose of this change. Convert this decode failure to httpx.DecodingError in both helpers as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in 30352d4. You are right, and I had considered UnicodeDecodeError earlier in this work and talked myself out of it as speculation — it is not speculative.
Verified your example before acting: json.loads(b"\xff") and response.json() on b"\xff" both raise UnicodeDecodeError, not JSONDecodeError. (My first probe used b"\xff\xfe\x00bad", which is a valid UTF-16 BOM, so it decoded and then failed as JSONDecodeError — that nearly led me to dismiss the report. Your exact byte was the one that mattered.)
Two changes:
decode_jsonanddecode_json_bytesnow catch(json.JSONDecodeError, UnicodeDecodeError), as you asked.- Beyond the report:
shared/envelope.py:upstream_guardhad the identical arm ordering, and I confirmed it returnedINVALID_INPUTfor a rawUnicodeDecodeErrorbefore fixing. That is the live path for every client that calls.json()itself — drug_database, nutrient_file, recalls, ckan, statcan — so the same masking existed there.
I also made the regression test assert the general property rather than enumerate exception types: test_decode_helpers_never_raise_a_valueerror_subclass feeds four malformed-body shapes and asserts nothing escaping the helpers is a ValueError subclass. Enumerating types is what let this through; the property does not have that failure mode.
Non-vacuity confirmed by reverting shared/http.py — all 4 new tests fail.
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. It does not reproduce: 23 Drug/Nutrient tests pass in 6.5s and all 340 live tests pass in a single run in 3m24s. Both reports corrected in place rather than left to justify future exclusions.
…error Addresses the Codex P2 on PR #4. Correct, and I had explicitly considered UnicodeDecodeError earlier in this work and dismissed it as speculation — it is not. A body that is not valid UTF-8/16/32 (b"\xff", a truncated multi-byte sequence) makes json.loads and response.json() raise UnicodeDecodeError, NOT json.JSONDecodeError. It subclasses ValueError too, so guarding only JSONDecodeError left the exact masking this phase set out to remove: the saskatchewan/manitoba `except ValueError` arms still reported INVALID_INPUT. Fixed in decode_json and decode_json_bytes as Codex asked, and — the step Codex did not ask for — in shared/envelope.py:upstream_guard, which had the identical arm ordering. Verified before fixing: the guard returned INVALID_INPUT for a raw UnicodeDecodeError, which is the live path for every client that calls .json() itself (drug_database, nutrient_file, recalls, ckan, statcan). test_decode_helpers_never_raise_a_valueerror_subclass now asserts the general property over four malformed-body shapes rather than enumerating exception types, so the next subclass in this family cannot slip through the same way. Verified non-vacuous: reverting shared/http.py fails all 4 new tests. 3159 passed, 97.13% coverage.
/gsd-progress reported the project stuck at 20.2 "planned" with 20.3 and 20.4 "empty", and it was right to: GSD infers completion from SUMMARY.md, and all three phases shipped with only a VERIFICATION.md. A future session running /gsd-progress --next would have been routed backwards into re-executing work that merged in PRs #3, #4 and #5. Written from the actual commits, review threads and measured outcomes — not reconstructed narrative. Each records what the phase shipped, the corrections made mid-flight, and the Codex findings. Roadmap now reads 20 / 20.1 / 20.2 / 20.3 / 20.4 all complete; next is Phase 21.
1 participant
Finishes what Phase 20.2 started — and closes a gap that PR #3 left open while claiming otherwise.
The gap
20.2 added the decode guard to
shared/http.py:api_getand stopped there. Three of the four portal technologies decode JSON themselves and were untouched:shared/http.py(CKAN family)response.json()shared/arcgis_hub.pyresponse.json()×6 +_parse_raw_jsonshared/socrata.pyresponse.json()×6shared/ogc.pyjson.loads(response.content)×2shared/parsers.py(geojson hot path)json.loads(content)×2Because
json.JSONDecodeErrorsubclassesValueError, a malformed body from any of those still landed in theexcept ValueError -> INVALID_INPUTarms of five tools. Reproduced before fixing:Both should be
UPSTREAM_ERROR. SoERR-03— "a malformed upstream body is classified as an upstream failure" — was true for CKAN modules and overclaimed for everything else.The fix
decode_json(response, url)anddecode_json_bytes(content, url)inshared/http.py, with all 14 decode sites routed through them,api_getincluded. Both raisehttpx.DecodingError— anHTTPError, not aValueError— so it bypasses theINVALID_INPUTarms into the catch-all every tool got in 20.2.test_no_shared_portal_client_decodes_json_unguardedfails if a raw decode reappears inshared/. That guard is the actual lesson here: 20.2 encoded the rule in prose namingapi_get, and prose is exactly what let three clients slip.Honest note on the tests
The four shared-client tests are the evidence — all fail when
shared/is reverted. The five tool-level tests injecthttpx.DecodingError, which the tools already handled after 20.2, so they pass either way; they are forward-looking regression guards, not proof of this fix.Gates
ruff clean; pyright 0 errors; catalog fresh; 3154 passed, 97.13% coverage. Full live integration run in progress.
Requirements: ERR-05.