fix(client): retain API status when error body reads fail

[rudycelekli]

Summary

The client reference promises non-2xx statuses as APIError. A truncated HTTP error body instead returned only a read error, discarding its actionable status. Preserve errors.As(APIError) and errors.Is(read cause) through standard multiple wrapping without changing the public APIError struct layout.

Reference: upstream contract.

Reproduction and producer proof

Actual HTTP server sends 429, 503 or 401 with Content-Length 100 and a shorter body. Before: 'goplaces: read response: unexpected EOF', errors.As(*APIError) false. After: 'goplaces: api error (429): goplaces: read response: unexpected EOF' (analogous 503/401), errors.As returns the exact status and errors.Is(io.ErrUnexpectedEOF) stays true. Truncated 200 remains a read failure without an invented APIError. Partial fixture-key contents are not exposed.

Owned local HTTP fixtures only; no live Google calls, credentials, quota usage or billing verification are claimed.

Verification

• Public consumer regression: go test . -run 'TestTruncated(ErrorResponsePreservesHTTPStatusAndReadCause|SuccessResponseRetainsReadFailure)' -count=1; fails before, passes after.
• make lint-check test coverage with pinned golangci-lint v2.13.2: passes; zero lint issues and repository coverage above its 90% gate.
• go build ./...: passes.
• go test -race ./...: passes.
• git diff --check: passes.

Verified signed head: 87a2f5f (DCO sign-off). Repository coverage 92.1%. Hosted checks are separate from these local results.

Assistance

Prepared with AI assistance; the reproduced behavior, source review and checks were completed before submission.

Captured native output

Actual producer observations from the owned loopback fixture (the API key is a dummy value):

{
  "before": [
    {
      "api_error": false,
      "api_status": 0,
      "error": "goplaces: read response: unexpected EOF",
      "http_status": 429,
      "unexpected_eof": true
    },
    {
      "api_error": false,
      "api_status": 0,
      "error": "goplaces: read response: unexpected EOF",
      "http_status": 503,
      "unexpected_eof": true
    },
    {
      "api_error": false,
      "api_status": 0,
      "error": "goplaces: read response: unexpected EOF",
      "http_status": 401,
      "unexpected_eof": true
    },
    {
      "api_error": false,
      "api_status": 0,
      "error": "goplaces: read response: unexpected EOF",
      "http_status": 200,
      "unexpected_eof": true
    }
  ],
  "after": [
    {
      "api_error": true,
      "api_status": 429,
      "error": "goplaces: api error (429): goplaces: read response: unexpected EOF",
      "http_status": 429,
      "unexpected_eof": true
    },
    {
      "api_error": true,
      "api_status": 503,
      "error": "goplaces: api error (503): goplaces: read response: unexpected EOF",
      "http_status": 503,
      "unexpected_eof": true
    },
    {
      "api_error": true,
      "api_status": 401,
      "error": "goplaces: api error (401): goplaces: read response: unexpected EOF",
      "http_status": 401,
      "unexpected_eof": true
    },
    {
      "api_error": false,
      "api_status": 0,
      "error": "goplaces: read response: unexpected EOF",
      "http_status": 200,
      "unexpected_eof": true
    }
  ]
}

[clawsweeper]

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

[clawsweeper]

Codex review: needs maintainer review before merge. Reviewed October 6, 2026, 3:28 AM ET / 07:28 UTC.

ClawSweeper review

What this changes

The client preserves HTTP error status and the underlying read failure when an error response body is interrupted, with public-client regression coverage and an Unreleased changelog entry.

Merge readiness

✅ Ready for maintainer review

This PR fixes a documented error-handling gap that remains on current main and in v0.4.11. The supplied real HTTP before/after observations support the repair, and no actionable patch defect was found.

Priority: P2
Reviewed head: 87a2f5f0387f3041f57106a7460b83b96ebfd2e2

Review scores

Measure	Result	What it means
Overall readiness	🐚 platinum hermit (4/6)	A focused repair with relevant real HTTP observations, public regression coverage, and no identified blocking defect.
Proof confidence	🐚 platinum hermit (4/6)	Sufficient (live_output): Captured before/after output exercises public Search through the production HTTP client against an owned local server with truncated 429/503/401 responses, showing recovered APIError status and preserved unexpected EOF; truncated 200 remains unchanged. This is real transport fault proof, supported by regression tests, and no stored-data contract changes.
Patch quality	🐚 platinum hermit (4/6)	No actionable review findings were identified.

Verification

Check	Result	Evidence
Real behavior	Verified	Sufficient (live_output): Captured before/after output exercises public Search through the production HTTP client against an owned local server with truncated 429/503/401 responses, showing recovered APIError status and preserved unexpected EOF; truncated 200 remains unchanged. This is real transport fault proof, supported by regression tests, and no stored-data contract changes.
Evidence reviewed	7 items	Verified patch ownership: The pinned main-to-head delta changes only the shared client's read-error branch, adds public consumer tests, and adds an Unreleased changelog entry. Existing contract: The client reference promises APIError for non-2xx responses and errors.Is/errors.As access to underlying causes. Current main returns immediately on a body-read error before constructing APIError. Real transport proof: The complete supplied PR body records public-client observations against an owned loopback HTTP server returning short bodies with Content-Length 100. After the fix, 429/503/401 remain discoverable through errors.As and unexpected EOF through errors.Is; truncated 200 remains a read failure without APIError. This exercises the changed shared response-reading owner through real HTTP transport rather than a mocked transport. Reviewer tests were not executed.
Findings	None	None.
Security	None	None.

How this fits together

goplaces routes public library and CLI requests through a shared HTTP client for Google Places and Routes. That client turns upstream responses into result payloads or errors that callers can inspect.

flowchart TD
  A[Library or CLI request] --> B[Shared HTTP client]
  B --> C[Upstream HTTP response]
  C --> D[Read bounded response body]
  D --> E{Body read fails}
  E -->|Non-success status| F[HTTP status and read cause]
  E -->|Success status| G[Read cause only]
  E -->|No| H[Normal status handling]

Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric	Value	Why it matters
Production and test delta	production +8/-1; tests +49/-0	The small production addition repairs the documented error contract, with public-client coverage for three error statuses and a successful-status control.

Technical review

Best possible solution:

Keep interrupted non-2xx responses inspectable by both HTTP status and read cause without exposing partial bodies or altering successful-response handling.

Do we have a high-confidence way to reproduce the issue?

Yes: a short HTTP body with a larger declared Content-Length reaches main's early read-error return and loses APIError status. Source inspection and the contributor's real HTTP before/after output support this path; the reviewer did not execute it.

Is this the best way to solve the issue?

Yes: standard multiple error wrapping preserves the documented status and cause without changing the public error struct or duplicating request handling.

AGENTS.md: not found in the target repository.

Codex review notes: model internal, reasoning medium; reviewed against 9883e1044f50.

Labels

Label changes:

• add P2: This is a bounded client error-classification repair for interrupted responses, with no evidence of an urgent widespread outage.
• add proof: sufficient: Contributor real behavior proof is sufficient. Captured before/after output exercises public Search through the production HTTP client against an owned local server with truncated 429/503/401 responses, showing recovered APIError status and preserved unexpected EOF; truncated 200 remains unchanged. This is real transport fault proof, supported by regression tests, and no stored-data contract changes.
• add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
• add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): Captured before/after output exercises public Search through the production HTTP client against an owned local server with truncated 429/503/401 responses, showing recovered APIError status and preserved unexpected EOF; truncated 200 remains unchanged. This is real transport fault proof, supported by regression tests, and no stored-data contract changes.

Label justifications:

• P2: This is a bounded client error-classification repair for interrupted responses, with no evidence of an urgent widespread outage.
• rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
• status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): Captured before/after output exercises public Search through the production HTTP client against an owned local server with truncated 429/503/401 responses, showing recovered APIError status and preserved unexpected EOF; truncated 200 remains unchanged. This is real transport fault proof, supported by regression tests, and no stored-data contract changes.
• proof: sufficient: Contributor real behavior proof is sufficient. Captured before/after output exercises public Search through the production HTTP client against an owned local server with truncated 429/503/401 responses, showing recovered APIError status and preserved unexpected EOF; truncated 200 remains unchanged. This is real transport fault proof, supported by regression tests, and no stored-data contract changes.

Evidence

What I checked:

• Verified patch ownership: The pinned main-to-head delta changes only the shared client's read-error branch, adds public consumer tests, and adds an Unreleased changelog entry. (internal/places/client.go:123, 87a2f5f0387f)
• Existing contract: The client reference promises APIError for non-2xx responses and errors.Is/errors.As access to underlying causes. Current main returns immediately on a body-read error before constructing APIError. (docs/client-reference.md:20, 9883e1044f50)
• Real transport proof: The complete supplied PR body records public-client observations against an owned loopback HTTP server returning short bodies with Content-Length 100. After the fix, 429/503/401 remain discoverable through errors.As and unexpected EOF through errors.Is; truncated 200 remains a read failure without APIError. This exercises the changed shared response-reading owner through real HTTP transport rather than a mocked transport. Reviewer tests were not executed. (error_response_read_test.go:14, 87a2f5f0387f)
• Latest release remains affected: The v0.4.11 release commit also returns the read failure before inspecting the HTTP status, so it does not already contain this repair. (internal/places/client.go, c0cdace7197b)
• Related merged work is distinct: fix: reject truncated and non-success API responses #60 is verified merged and addresses oversized responses and non-success status decoding. Its implementation still leaves interrupted body reads without APIError. A GitHub search for error-body work found that PR, fix(security): scope API redirects and sanitize diagnostics #53, and this PR; no verified replacement owns this exact repair. (internal/places/client.go, 415ff611d515)
• History routing and inspection limits: Available file history identifies Peter Steinberger across response handling, diagnostic sanitization, and shared-client refactoring. Historical blob inspection and blame encountered unavailable promisor objects with HTTP 403; therefore no exact source-line introduction is claimed. Current main and head source were readable, and release source was independently read through GitHub. (internal/places/client.go, 9883e1044f50)

Likely related people:

• steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rating scale

Score	Internal tier	Crab rank	Meaning
6/6	S	🦀 challenger crab	Exceptional readiness
5/6	A	🦞 diamond lobster	Very strong readiness
4/6	B	🐚 platinum hermit	Good normal PR; ordinary maintainer review
3/6	C	🦐 gold shrimp	Useful, but confidence is limited
2/6	D	🦪 silver shellfish	Proof or implementation needs work
1/6	F	🧂 unranked krab	Not merge-ready
N/A	NA	🌊 off-meta tidepool	Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

• ClawSweeper keeps one durable marker-backed review comment per issue or PR.
• Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
• A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
• PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
• Maintainers can also comment @clawsweeper review to request a fresh review only.
• Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
• Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
• Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.