refactor: unify the HTTP exception hierarchy and route errors through the factory

[OmarAlJarrah]

Summary

Two related error-handling changes.

Unify the exception hierarchy (#37 — breaking)

HttpResponseException (an IOException carrying a Response) duplicated HttpException's role and had no production callers. It is removed; HttpException becomes the single response-carrying type and gains an optional value slot for a parsed error body. A small Retryable interface (isRetryable) is hoisted and implemented by both HttpException and NetworkException, so retry classification keys off the interface rather than matching concrete types (the two types previously exposed the flag under different names). This is a binary-incompatible change, appropriate at 0.0.1-alpha.

Route the error path through the factory (#23)

HttpExceptionFactory.fromResponse(...) existed but had no callers. A new ThrowOnHttpErrorStep (a response-pipeline step) turns a 4xx/5xx response into the factory's exception; because that exception is Retryable, it flows through the recovery chain and retry classification uniformly.

The retryable status set is unchanged. New tests cover the wired error path, the collapsed exception carrying what the removed type used to, and Retryable-driven retry classification. The API snapshot is regenerated.

Touches the retry classification path, so this and the retry PRs (#92 / #38) will need rebasing depending on merge order.

Closes #23
Closes #37

[OmarAlJarrah]

Review notes

This unifies the response-carrying exception story: it removes HttpResponseException, makes HttpException the single response-carrying base, adds an optional value slot for a parsed error payload, and hoists a Retryable interface so RetryStep classifies off the interface rather than concrete types. It also adds ThrowOnHttpErrorStep, which finally gives HttpExceptionFactory.fromResponse a caller. The refactor is clean and the docs, README, refs-comparison, and .api snapshot are all updated consistently. There is one functional contradiction worth fixing before merge and a gap in the codegen extension path.

Major

• Thrown error exception carries a body the pipeline immediately closes — pipeline/step/ThrowOnHttpErrorStep.kt:31-36 interacting with pipeline/ResponsePipeline.kt:100-106 — HttpExceptionFactory.fromResponse builds e.g. BadRequestException(response), which forwards body = response.body. When the step throws, applyResponseSteps calls closeQuietly(inResponse, t) → Response.close() → body?.close(), closing the exact ResponseBody instance the thrown HttpException still holds. So by the time a caller inspects the resulting ResponseOutcome.Failure, the exception's body is already closed — bodySnapshot() and any typed error-body deserialization off the failure path will fail or return nothing. This directly contradicts the new value-slot docs ("the generated service layer populates it once it has deserialized body") and HttpException's "stream it on demand" promise, and it makes ThrowOnHttpErrorStep's "intentionally does not consume Response.body" KDoc moot — intent aside, the body is unreadable downstream. Suggest buffering the error body into a replayable in-memory ResponseBody when constructing the exception on this path (bounded, consistent with the bodySnapshot cap so a large 5xx body can't OOM), or making the close-before-propagate discipline body-aware on the error path. If neither, the docs should stop promising readable error bodies via this route.

• Concrete HttpException subclasses don't forward the new value slot — http/response/exception/HttpExceptions.kt:32-44 and every sibling; KDoc at HttpException.kt — HttpException's KDoc says codegen introduces per-operation typed subclasses "by deriving from a concrete one and stamping a typed deserialized payload," and value is described as where that payload lands. But BadRequestException/NotFoundException/etc. only take (response, message, cause) and call the base with no value argument, so there is no path for a subclass-of-a-concrete-subclass to pass value through. The PR's own round-trip test has to derive from the abstract base (object : HttpException(..., value = payload)) precisely because the concrete subclasses can't forward it. Either add a value parameter to the concrete subclass constructors (forwarding to the base) or correct the KDoc to say typed subclasses extend the abstract base, not a concrete one.

Minor

• No constructor/overload sets value without also passing message and cause — http/response/exception/HttpException.kt:74-80; sdk-core/api/sdk-core.api — value is appended after message and cause under @JvmOverloads, so the generated overloads stop at (Status, Headers, ResponseBody, String, Throwable); the only one exposing value is the full (Status, Headers, ResponseBody, String, Throwable, Object). A Java caller or codegen wanting status+headers+body+value must pass message/cause positionally. Kotlin named-arg callers are fine. If the slot is for the generated layer, worth a conscious decision on parameter ordering rather than defaulting to append-at-end.

• ThrowOnHttpErrorStep redeclares the 400..599 boundary the factory already owns — pipeline/step/ThrowOnHttpErrorStep.kt:19-20,33-35 — ERROR_RANGE_MIN/MAX duplicate HttpExceptionFactory's SC_CLIENT_ERROR_MIN/SC_SERVER_ERROR_MAX. The pre-check is a reasonable guard against the factory's IllegalArgumentException on 1xx/2xx/3xx, but the magic boundary now lives in two files and will silently diverge if the factory's accepted range changes. Consider exposing a predicate on the factory (isErrorStatus(code) or a nullable fromResponseOrNull) so the range stays single-sourced.

• Confirm no doc-link check trips on the removed type — util/RetryUtils.kt:11-15 — the KDoc correctly repoints from the deleted HttpResponseException to HttpException/Retryable. A branch-wide grep shows no remaining HttpResponseException references in production code or docs, so this is just a green-build confirmation given allWarningsAsErrors and any wired doc validation.

Nits

• Round-trip test uses assertEquals where assertSame is more precise — http/response/exception/HttpExceptionFactoryTest.kt ("value slot round-trips" test) — assertEquals(payload, ex.value) passes by structural map equality, but the intent is that the same instance is carried through unchanged. The deleted HttpResponseExceptionTest used assertSame(payload, ex.value); matching that pins value as an unmodified pass-through reference rather than an equal object.

• Test comment reintroduces tracker-ID framing — pipeline/step/ThrowOnHttpErrorStepTest.kt:1061-1063 — the comment "End-to-end check of the Wire HttpExceptionFactory.fromResponse into the error path #23 / Collapse HttpResponseException into HttpException and hoist a Retryable interface #37 seam" embeds issue numbers in a checked-in comment, which a recent commit explicitly removed elsewhere. Reword to describe the behavior on its own terms (e.g. "the error path emits an HttpException that is Retryable, which is what RetryStep classifies on").

Questions

• Retryable widens the retry-classification surface — pipeline/step/retry/RetryStep.kt:335-348 — the rewrite to if (error !is Retryable || !error.isRetryable) return false; return error !is HttpException || settings.retryableStatuses.contains(error.status.code) is equivalent for the two existing types, and the extensibility is the point. But note the new contract: any future Retryable that is not an HttpException is retried with no status gate (the only remaining gate is the idempotency check), exactly as NetworkException is treated today. Confirm that's the intended boundary for third-party Retryable implementations.

• No default pipeline wires in ThrowOnHttpErrorStep — pipeline/step/ThrowOnHttpErrorStep.kt — nothing under sdk-core/src/main constructs a ResponsePipeline that includes this step, so phrasing like "this is wired through ThrowOnHttpErrorStep / the live error path" in the docs is forward-looking — it's an available building block, not yet assembled into a default. (The recovery-aware pipeline package is sync-only and has no async mirror, so there's no async sibling missing here; the http.pipeline async machinery is a separate layer.) Worth confirming the intended scope so the prose doesn't overstate the wiring.

Praise

• Clean single-source-of-truth retry design — http/response/exception/Retryable.kt; RetryStep.kt:335-348 — replacing concrete-type matching with a Retryable interface, while keeping isRetryable derived from RetryUtils rather than a per-subclass constant, removes both the duplicate exception type and the divergent flag names (retryable vs isRetryable). Docs, README, refs-comparison, and the .api snapshot were all updated together, and the breaking removal is appropriately scoped to 0.0.1-alpha.

• Good integration coverage on the new step — pipeline/step/ThrowOnHttpErrorStepTest.kt:990-1071 — the step is tested in isolation (2xx/3xx pass-through via assertSame, 404/503/429 mapping, header+status propagation) and wired through a real ResponsePipeline (5xx Success→Failure, 2xx stays Success with the same instance, mapped Failure is Retryable). That exercises both the close-before-propagate path and the Retryable seam end-to-end, which is exactly the integration risk for this change.