PR #751 - basecamp/basecamp-sdk

Closes #730. Follow-up filed as #750.

The bug

The three Kotlin merge-safe composites identify a decode failure by inspecting the cause of the BasecampException.Api their GET raised:

} catch (e: BasecampException.Api) {
    val decodeFailure = e.cause as? SerializationException ?: throw e

That is a pattern, and anyone can match it. BasecampHttpClient.authenticateClassified propagates an already-classified BasecampException from the auth strategy untouched — deliberately, and its docstring says so. So an AuthStrategy that classifies its own JSON failure as Api(cause = SerializationException(...)) — a token provider decoding a token response, say — lands in this catch, matches, and the caller is told:

GetDocument returned a body that does not decode as a document: …

with the merge-safe hint attached, for a request that was never sent. Wrong message, misleading hint, and a pointer at the wrong subsystem. Same class of conflation #604 fixed in the base layer, surviving one layer up.

Verified in source before fixing: BasecampHttpClient.kt:69-79 (propagate-as-is), BaseService.request (catch (e: BasecampException) { … throw e }), composite catches at SchedulesService.kt:290, DocumentsService.kt:109, TodolistsService.kt:113.

The fix — identity, not shape

BasecampException.Api gains an internal decodeFailure: SerializationException? slot, fillable only through a private constructor reached by the internal factory Api.malformedBody, whose only caller is BaseService.malformedBody — the one place a decode failure is rendered. Its presence is the discriminator; its value is the decoder's message the composites were reaching into cause for. Each catch becomes:

val decodeFailure = e.decodeFailure ?: throw e

Nothing outside the SDK module can produce that slot, so the ambiguity is removed rather than tested for a second time.

Alternatives, and why they lost

Narrow the try/catch to the decode call — unavailable, not inferior. The composite's try already wraps exactly one expression, and the decode happens inside the generated wire method, where #604 already narrowed it to the decode expression. A composite doing its own decode is what AGENTS.md Hard Rule 2 forbids.
Wrap cause in an internal marker type — loses on evidence. cause is the cross-module channel: the Kotlin conformance runner reads (e as? BasecampException.Api)?.cause as? SerializationException (kotlin/conformance/.../Main.kt:554), MissingFieldException.missingFields is a real caller API, and DecodeIsolationTest pins cause.cause as the NumberFormatException under a numeric refusal. A wrapper shifts all three by one hop for no gain. cause is untouched here, so the conformance runner needs no change.
A flag set across the decode — shared mutable state on a service instance that concurrent coroutines share. No.

Public API is unchanged: Api's public constructor keeps its exact signature and defaults; the new slot and the constructor that sets it are internal.

Round 2 — keeping the marker off Java's API (`8b4f0b8d7`)

Codex raised a P2 that was correct and more reachable than argued. internal on a constructor does not survive the JVM boundary: <init> cannot be name-mangled, so the first version emitted

public BasecampException$Api(java.lang.String, kotlinx.serialization.SerializationException)

— public, non-synthetic, two args. And it was the convenient overload: Kotlin default arguments do not exist for Java callers and there is no @JvmOverloads, so the only other constructor Java saw was the full six-arg form. A Java-authored AuthStrategy classifying its own token-response decode failure writes new Api(message, decodeError), sets the marker, and the composite relabels an auth failure as a malformed response — #730 back through the slot added to kill it. Accident class; no adversary needs naming.

The marker constructor is now private, reached through an internal factory (internal functions are mangled: malformedBody$com_basecamp_basecamp_sdk). Emitted surface after the fix:

private   Api(String, Integer, String, boolean, String, Throwable, SerializationException)   ACC_PRIVATE
public    Api(String, Integer, String, boolean, String, Throwable)                           ACC_PUBLIC
public    Api(String, …, Throwable, int, DefaultConstructorMarker)                           ACC_PUBLIC, ACC_SYNTHETIC
public    Api(String, …, Throwable, SerializationException, DefaultConstructorMarker)        ACC_PUBLIC, ACC_SYNTHETIC

Both marker-bearing bridges are synthetic (javac will not resolve those from source) and carry a trailing DefaultConstructorMarker no call site would write. The public constructor's signature and defaults are byte-identical, as promised.

New guard ApiConstructorSurfaceTest lives in jvmTest, because this is a property of what the JVM target emits: (1) no Java-selectable — public, non-synthetic — constructor takes a SerializationException; (2) the Java-selectable set is exactly the six-arg constructor, so (1) cannot pass vacuously; (3) no plainly-named malformedBody on the Java surface while a mangled one exists.

It looks the companion up reflectively rather than naming Api.Companion, and that is load-bearing: the first draft named it, so against the un-fixed shape the "red" run was an unresolved reference, not a failing assertion — a guard that can only fail by failing to compile proves nothing about what the build emitted. Reflective, it compiles against both shapes and reports 3/3 assertion failures on the un-fixed code with 0 compile errors, the first naming (String, SerializationException), gradle REAL_EXIT=1.

What this does not prove, stated plainly: reflection over emitted constructors is the closest the toolchain gets to a Java caller here (no Java source set, no sample module), so it asserts the surface javac would choose from, not a compiled Java call site.

The cause channel the Kotlin conformance runner reads is untouched, and demonstrated rather than asserted: setting cause = null in the new factory fails 6 of 10 DecodeIsolationTest cases. The conformance suite passes 197/0/1-skipped — green there means the runner's cause branch does not misfire (it is a fixture-repair guard); DecodeIsolationTest is what proves it still fires.

Red proofs — one regression test per call site

Each site's test was shown to fail against the un-fixed line by reverting that one site alone (exactly one line changed, diffed and verified on disk, restored by cp + diff -q, never git checkout --):

mutated site	failing test	the other two suites
`DocumentsService.kt:117`	`DocumentsServiceTest.updateDoesNotRelabelAnAuthStrategyFailure` (1 of 10 failed)	Schedules 21/21, Todolists 24/24 green
`SchedulesService.kt:299`	`SchedulesServiceTest.updateDoesNotRelabelAnAuthStrategyFailure` (1 of 21 failed)	Documents 10/10, Todolists 24/24 green
`TodolistsService.kt:122`	`TodolistsServiceTest.updateDoesNotRelabelAnAuthStrategyFailure` (1 of 24 failed)	Schedules 21/21, Documents 10/10 green

Each failure reads the strategy's own exception must reach the caller unchanged ==> expected: <…the token endpoint returned a body that does not decode> but was: <…Get{Document,ScheduleEntry,TodolistOrGroup} returned a body that…>, i.e. the relabelling itself, with a gradle REAL_EXIT=1.

assertFailsWith<BasecampException.Api> alone would be vacuous here — the class is right in both worlds. The assertions are assertSame on the strategy's own instance, the message, hint == null, and zero requests issued.

The positive half is guarded too: mutating malformedBody to leave the slot unset (simulating an over-narrow discriminator) fails 11 tests across the three suites, so the fix cannot pass by never re-hinting anything. The existing malformed-body tests were strengthened from hint != null to assert the composite's message and the escape-hatch hint text.

Verification

Re-run on the round-2 head:

Cold run with all Kotlin build dirs removed (kotlin/{sdk,generator,conformance}/build) and --no-build-cache: :conformance:test :basecamp-sdk:check :generator:test — compileKotlinJvm, jvmTest, :conformance:test, :generator:test all EXECUTED, none FROM-CACHE/UP-TO-DATE, 555 jvm tests, 0 failures, REAL_EXIT=0.
make kt-test → REAL_EXIT=0; make kt-check-drift → 250/250, no drift, REAL_EXIT=0; make conformance-kotlin → 197 passed, 0 failed, 1 skipped, REAL_EXIT=0.
Regenerates nothing: the diff is 5 hand-written main files + 4 test files, no generated/ paths.

Scope

Kotlin-local by design. Swift answers the same question by substring sniff (malformedBodyPhrase in swift/Sources/Basecamp/Services/BaseService.swift), because BasecampError.api has no cause slot at all; unifying the two means an associated value — a Class C break — plus both conformance runners. That is filed as #750.

Summary by cubic

Distinguishes Kotlin composites’ own response decode failures from auth strategy exceptions and prevents accidental Java construction of decode-marked errors. Previously, composites relabeled any BasecampException.Api with a kotlinx.serialization.SerializationException cause; now they only re-hint when decodeFailure is present, which only the SDK sets.

BasecampException.Api adds internal decodeFailure; the marker-bearing constructor is private and reachable only via internal, mangled factory Api.malformedBody(...). The public 6‑arg constructor is unchanged; jvmTest ApiConstructorSurfaceTest asserts no Java-selectable constructor accepts SerializationException and the factory is only available under a mangled name.
BaseService.malformedBody now uses Api.malformedBody(...) to set the marker while keeping cause intact.
DocumentsService, SchedulesService, and TodolistsService now use e.decodeFailure ?: throw e, so auth-strategy Api exceptions pass through unchanged and only true response decode failures get composite-specific messages and hints.
Tests updated to prove non-relabeling of auth failures and correct re-hinting on real decode failures.

Impact

No migration required. Callers receive original auth-strategy Api exceptions; decode failures still get composite-specific messages and hints. Kotlin-only change; no generated files changed; Swift unaffected.