Skip to content

fix(decisioning): project malformed-JSON 2xx responses to AdcpError#836

Draft
bokelley wants to merge 1 commit into
mainfrom
claude/issue-453-json-decode-error-recut
Draft

fix(decisioning): project malformed-JSON 2xx responses to AdcpError#836
bokelley wants to merge 1 commit into
mainfrom
claude/issue-453-json-decode-error-recut

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Refs #453

When an upstream returns a successful (2xx) status with a non-JSON body (e.g. a CDN or proxy returning an HTML error page), UpstreamHttpClient._request previously propagated a raw json.JSONDecodeError from response.json(). Adopters catch AdcpError throughout; an unhandled ValueError subclass breaks that contract and is invisible in typed code. This PR wraps the call in except ValueError and re-raises as AdcpError(SERVICE_UNAVAILABLE, recovery="transient") — the same code used for 5xx responses, since an upstream returning HTML for a JSON endpoint is indistinguishable from a transient infrastructure failure.

This is a re-cut of the original branch from #767 (claude/issue-453-json-decode-error-handling), which was behind main and had maintainer edits disabled. The diff is identical to #767 but rebased cleanly on current main.

What changed

  • src/adcp/decisioning/upstream.py: wrap response.json() in try/except ValueError; raise AdcpError(SERVICE_UNAVAILABLE) with recovery="transient" and from exc chain.
  • tests/test_upstream_helpers.py: add test_200_with_malformed_json_raises_service_unavailable — mocks a 200 with an HTML body and asserts code, recovery, and __cause__.

What tested

  • pytest tests/test_upstream_helpers.py -q35 passed, 0 failed
  • mypy src/adcp/decisioning/upstream.py — clean
  • ruff check src/adcp/decisioning/upstream.py tests/test_upstream_helpers.py — clean

Pre-PR review

  • code-reviewer: approved — flagged missing @pytest.mark.asyncio (false positive: asyncio_mode = "auto" in pyproject.toml); one nit on truncating exc in message (low severity, AdcpError is internal)
  • dx-expert: approved — SERVICE_UNAVAILABLE / transient is correct for CDN/proxy interception; nit on adding bounded response.text[:200] snippet to message; no blockers

Nits (not fixed):

  • Error message embeds raw exc text rather than a body-length hint; useful for debugging, acceptable for an internal error type
  • recovery="transient" is correct per the spec but can't distinguish "CDN blip" from "permanently misconfigured upstream" — no better enum value exists

Triage-managed PR. This bot does not currently iterate on
review comments or PR conversation threads (only on the source
issue). To unblock:

  • Push fixup commits directly: gh pr checkout <num>
    fix → push.
  • Or re-trigger: comment /triage execute on the source
    issue.

See adcp#3121
for context.

Session: https://claude.ai/code/session_018wq8sxhkG9c2WW4vgpYgS8


Generated by Claude Code

When an upstream returns a 2xx status with a non-JSON body (e.g. a CDN
or proxy returning an HTML error page), re-raise json.JSONDecodeError as
AdcpError(SERVICE_UNAVAILABLE, recovery="transient") so adopters get a
typed error consistent with the rest of the error-projection surface.

Refs #453

https://claude.ai/code/session_018wq8sxhkG9c2WW4vgpYgS8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants