Conversation
There was a problem hiding this comment.
Pull request overview
This PR expands the nri-http error types to include more contextual information (response body/metadata) so failures in deployed environments are easier to diagnose.
Changes:
- Extend
Http.Internal.Errorto include response body text forBadStatusand a structuredBadBodyReasonfor JSON decoding failures. - Populate the new error details in
handleResponse(JSON decode failures) andexceptionToError(non-2xx responses).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
nri-http/src/Http/Internal.hs |
Changes the public Error constructors and introduces BadBodyReason to carry richer decode-failure context. |
nri-http/src/Http.hs |
Constructs the new enriched error values from real HTTP responses/exceptions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I'll re-request a review when things are looking good! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Many APIs (slack in my case) explain the error in the response body
The `expectRequest` test helper had a race condition: it threw an exception to capture the incoming `Wai.Request` before the request body was read from the socket. When the test later called `Wai.strictRequestBody`, Warp had sometimes already torn down the connection, resulting in an empty body. This caused the "JSON is encoded correctly" test to intermittently fail with `-threaded`. Fix by eagerly reading the request body inside the WAI handler before throwing the exception, and passing it alongside the request in the `FirstRequest` exception. This eliminates the race between body reading and Warp connection teardown. Also update test assertions for the new Error constructors introduced on this branch: - `BadStatus` now takes `Int Text` (was `Int`) - `BadBody` now takes a `BadBodyReason` record (was `Text`) Update golden test line numbers to reflect the changed test file. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
decodeUtf8 can throw UnicodeException on non-UTF8 response bodies (e.g. binary payloads or truncated multi-byte sequences from startOfBody). Use decodeUtf8With lenientDecode instead so we reliably return Err values rather than crashing error construction. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
BadStatus now carries an Int and Text, and BadBody now carries BadBodyReason instead of Text. Update the doc comment to reflect this. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
BadStatus now carries an additional Text field and BadBody now takes BadBodyReason instead of Text, which are breaking changes to pattern matches in downstream code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
omnibs
force-pushed
the
http-errors-with-more-info
branch
from
3bcbe3e to
d46c253
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
Closing: I found the issue, I think. I'd rather not pay the cost of fixing the breaking change right now in the NRI monorepo. |
1 participant
I'm getting errors from an API in the deployed environment in an application that's hard to run locally.
Our current errors didn't help me understand what I need to do to fix my code.