refactor(kit): keep the response body on an HTTP failure, opt-in - #106
Merged
Merged
Conversation
…Xcode 27
`dev` stopped building when Xcode was updated on this machine mid-session. The
macOS 27 SDK adds a `Document` protocol to SwiftUI —
public protocol Document: ReadableDocument, WritableDocument
— which collides with the domain's `Document` struct in any file importing both.
That is every documents-feature view, and the same unchanged source went from
compiling to eleven `'Document' is ambiguous for type lookup` errors across eight
files.
Only the SwiftUI-importing files are affected, which is what makes the diagnosis
unambiguous: the view models import Foundation, Observation and InterlinedDomain
but not SwiftUI, and they compile untouched.
The fix is to qualify the type at the use sites. Three alternatives were
considered and rejected. Renaming the domain model is the tail wagging the dog —
`Document` is the right name, and it is correct across Kit, Domain, Persistence
and their tests. A module-level typealias would shorten the use sites at the cost
of giving one concept two names, so the next reader has to learn they are the
same thing. Dropping `import SwiftUI` is not available; these are views.
Every edit is a type position. No user-facing string, accessibility label or
other identifier containing the word Document is touched — the diff is eleven
lines, each one a `Document` that the compiler itself pointed at.
Worth knowing rather than fixing: this is a standing hazard. Any domain type
sharing a name with a SwiftUI symbol is one SDK update away from the same break,
and the diagnosis is written down at the top of DocumentsListView so the next
occurrence takes minutes.
Refs #98
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
APIError keeps only the decoded `{error}` string; everything else in the body is
discarded before any caller sees it. That is right for almost every failure,
where a sentence is all the UI needs — and wrong for the few where the server
answers a question rather than reporting a malfunction. The list-schema
destructive-change guard replies 400 with a `propertiesWithData` array naming
the columns that still hold data, and the app-settings family is compare-and-set,
replying 409 with the current document attached. Both callers exist and both are
written around the gap with a comment saying so.
The obvious fix — a second associated value on the error cases — is not viable.
420 sites across 78 files construct or pattern-match those cases, including
every test. That is an enormous mechanical change, every line of it a chance to
get a case wrong, in service of two call sites.
So this is opt-in. `sendCapturingFailure(_:)` throws an `APIFailure` carrying the
body; every other entry point keeps throwing plain `APIError` and not one of the
420 sites changes. `APIFailure` is deliberately not an APIError, so a caller
opts in by choosing the richer send rather than by accident — and it forwards
`localizedDescription` so nothing regresses if one reaches generic error display.
`details(as:)` returns nil rather than throwing on a mismatch. A caller asking
for details is asking an optional question, and a decode failure there must
degrade to today's behaviour instead of masking the HTTP failure being handled.
The transport raises the richer error internally so one code path serves both
entry points, and `send`/`sendVoid`/`sendRaw`/`sendWithRateLimitInfo` flatten it
back. Writing that revealed a real trap: `performWithSafetyNet` and
`performWithRetry` both catch `APIError`, so raising `APIFailure` silently
disabled the 401 session-retry and the whole retry policy. The existing
AuthTransport test caught it. Both now match on `underlyingError` and rethrow the
`APIFailure` whole — flattening there would drop the body before
`sendCapturingFailure` ever saw it, which is the entire point of the type.
A decode failure on a 2xx stays a plain `APIError.decoding`: it is a client-side
problem with no server body to offer, and dressing it as an `APIFailure`
carrying nothing would be a lie about where the fault is.
Refs #103
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 16, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #103.
APIErrorkeeps only the decoded{error}string. Everything else in the body is discarded atAPIClient.swift:234before any caller sees it — right for almost every failure, wrong for the few where the server answers a question rather than reporting a malfunction:PUT /api/lists/{id}/schemarefuses a destructive rebuild with400plus apropertiesWithDataarray naming the columns that still hold data (PR fix(lists): the schema wire contract was a string client-side and an object server-side #87 / bug(lists): the schema wire contract is a DSL string client-side and an object server-side — create, read and save are all broken #85).baseVersionanswers409with thecurrentdocument attached (PR feat(settings): finish the Applications pane and fix its wire contract #102 / feat(settings): finish the Applications pane - main workstation, rename, remove, copy to shared #56).Both callers already exist, and both are written around the gap with a comment explaining why the better behaviour is absent.
Why not just add an associated value
420 sites across 78 files construct or pattern-match those cases, including every test. That is an enormous mechanical change — every line of it a chance to get a case wrong — in service of two call sites. It is also exactly why both PRs flagged this and moved on rather than doing it in passing.
The design: opt-in
sendCapturingFailure(_:)throws anAPIFailurecarrying the body. Every other entry point keeps throwing plainAPIError, and not one of the 420 sites changes.APIFailureis deliberately not anAPIError, so a caller opts in by choosing the richer send rather than by accident. It forwardslocalizedDescriptionanddescription, so nothing regresses if one reaches generic error-display code.details(as:)returnsnilrather than throwing on a mismatch. A caller asking for details is asking an optional question, and a decode failure there must degrade to today's behaviour — a message with no details — instead of masking the HTTP failure actually being handled.A protocol-extension default wraps with
body: nil, so every existing stub and fake conforms unchanged and an opted-in caller against a stub simply getsnildetails. That is honest: a transport failure has no body either.The trap this surfaced
The transport now raises
APIFailureinternally so one code path serves both entry points, withsend/sendVoid/sendRaw/sendWithRateLimitInfoflattening it back.Doing that silently disabled the 401 session-retry and the entire retry policy —
performWithSafetyNetandperformWithRetrybothcatch let error as APIError, which stopped matching.The existing
AuthTransportTestscaught it, which is the system working. Both now match onunderlyingErrorand rethrow theAPIFailurewhole — flattening there would drop the body beforesendCapturingFailureever saw it, which is the entire point of the type.Worth knowing in review: that is the one genuinely risky part of this change, and it is covered by a test that predates it.
A decode failure is not an HTTP failure
A 2xx whose body does not match stays a plain
APIError.decoding. It is a client-side problem with no server body to offer, and dressing it as anAPIFailurecarrying nothing would be a lie about where the fault is. There is a test.Verification
xcodebuild build→** BUILD SUCCEEDED **xcodebuild test(App) →Executed 968 tests, with 0 failures·** TEST SUCCEEDED **swift test InterlinedKit --skip ContractTests→Executed 483 tests, with 0 failures(was 477)swift test InterlinedDomain→Executed 1012 tests, with 0 failuresswift test InterlinedPersistence→Executed 140 tests, with 0 failuresNew tests (6): details decode · message unchanged from
APIError· a normalsendstill throws a plainAPIError(the property that must not break) · a non-matching body degrades tonil· no body at all · an undecodable 2xx stays.decoding.Wiring the two consumers
Deliberately not in this PR.
ListsService.updateSchemalives on #87 andAppSettingsServiceon #102; wiring them here would mean either duplicating those branches' work or creating a three-way stack.Once #87, #102 and this have merged, each consumer is a one-line change — swap
api.sendforapi.sendCapturingFailureand readdetails(as:)— andListSchemaConflictDTOalready exists for the first.Branches from #99 (Xcode 27
Documentfix), without which the App target does not build.🤖 Generated with Claude Code