Retry the authenticated download hop under a declared set, in every SDK - #563
Merged
Conversation
The two-hop download algorithm prescribed throw-on-error with no retry,
while the conformance fixtures asserted retry the four runners had to
skip. Make §14 honest: hop 1 retries on network errors plus
{429, 502, 503, 504} — never 500 — with the per-SDK attempt budgets
(public total-attempt caps floored at one for Python/Ruby/Kotlin, a
fixed three-attempt policy for TypeScript/Swift), and hop 2 stays
no-retry, no-auth.
Fixture changes: the 503 case now asserts Authorization on EVERY hop-1
attempt (not just request 0) and drops its wrong all-5xx description;
new cases pin network-error retry and 500 non-retry. Appendix D gains
the two new rows. The eight download-retry roster lines (Python,
Ruby, TypeScript, Kotlin × 2) leave §19 — the per-SDK commits remove
the corresponding runner skips.
Lift the retry loop out of createRetryingFetch into retry.ts as
executeWithRetry(makeAttempt, config, emit) with four seams — attempt
begin/finalize/retrying emission, caller-owned attempt preparation
(request rebuild + auth refresh, terminal-marked so auth faults keep
their identity past retry classification), per-attempt fetch, and the
signal-aware backoff sleep. The client's middleware path delegates to
it unchanged.
downloadURL's hop 1 adopts the primitive with the fixed SPEC §14
policy passed directly (three attempts when enableRetry, retryOn
{429, 502, 503, 504}, never 500; DownloadURL is deliberately absent
from behavior-model.json). Hop 2 stays raw fetch: no retry, no auth —
pinned by a native test asserting Authorization on every hop-1
attempt and never on hop 2.
Native tables pin the complete retry set, 500 non-retry, the
enableRetry=false single attempt, timeout-abort terminality, balanced
start/end/onRetry hooks across retries, and client-level Retry-After.
The runner drops its two download skips and runs downloads.json
retry-enabled.
… set
get_no_retry becomes get_download: the authenticated hop-1 GET now runs
through _request_with_retry with an explicit retry_on set — SPEC §14's
{429, 502, 503, 504}, never 500 — passed directly because DownloadURL
is deliberately absent from behavior-model.json. The declared set is
authoritative in both directions, carving downloads out of the
ungoverned GET taxonomy (which retries all 5xx). Network errors keep
§7's classification; the public max_retries total-attempt cap applies,
floored at one. Hop 2 stays a fresh unauthenticated client: no retry,
no auth. Sync and async transports change identically.
Native tables (sync + async) pin the complete declared set, 500
non-retry, network-error retry, cap exhaustion, the max_retries: 0
single-attempt floor, Authorization on every hop-1 attempt and never
on hop 2, balanced start/end/on_retry hooks, Retry-After on 429, and
async cancellation as terminal.
The runner drops its two download skips (zero skips remain) and aligns
delayBetweenRequests to the first-gap semantic the Go, TypeScript, and
Kotlin runners already use — the download flow's final gap is the
redirect hop to the signed URL, which is deliberately un-delayed.
get_no_retry becomes get_download: the authenticated hop-1 GET now runs
through request_with_retry with an explicit retry_on set — SPEC §14's
{429, 502, 503, 504}, never 500 — passed directly because DownloadURL
has no behavior-model entry. The declared set is authoritative in both
directions, carving downloads out of the ungoverned GET taxonomy (which
retries all retryable 5xx). Status-less network errors keep the
taxonomy's judgment.
The download attempt budget is the public max_retries total-attempt
cap floored at one — FOR DOWNLOADS ONLY, by sharing the governed
caller_cap shape. The ungoverned general path keeps its unfloored cap;
its max_retries: 0 zero-attempt behavior is tracked separately (#532).
Hop 2 stays a bare Net::HTTP GET: no retry, no auth.
Native tests pin the complete declared set status by status, 500
non-retry, network-error retry, cap exhaustion, the max_retries: 0
single-attempt floor, Authorization on every hop-1 attempt and never
on hop 2, balanced start/end/on_retry hooks, and Retry-After on 429.
The runner drops its two unwaivered download skips (11 waivered 2B.3
GET-only skips remain) and aligns delayBetweenRequests to the
first-gap semantic the Go, TypeScript, and Kotlin runners already
use — the download flow's final gap is the redirect hop to the signed
URL, which is deliberately un-delayed.
… set downloadURL built a one-shot Ktor client with followRedirects off and fired a single unguarded request through it, so the authenticated hop surfaced every 429/502/503/504 straight to the caller and the two download-retry conformance fixtures had to be skipped. Hop 1 now runs in downloadHop1, a directive-shaped loop mirroring BasecampHttpClient.requestWithRetry (#517): the catch clauses only classify the attempt's outcome, and the retry side effects — onRetry, the backoff sleep, the next attempt — run outside any catch, so a CancellationException from the sleep propagates raw and no phantom request events fire for an attempt that already ended. The declared set is {429, 502, 503, 504} plus network errors; 500 stays out, matching the main GET loop's declared-set discipline rather than the error taxonomy's broader all-5xx flag. The budget is the public maxRetries total-attempt cap coerced to at least one, so an accepted maxRetries = 0 still sends exactly one attempt, and enableRetry = false collapses it to one. Every attempt re-runs the auth strategy so a rotated token is picked up. A throwing strategy is a configuration fault, not a transport fault: it surfaces raw through the DownloadAuthFailure tag, spends no retry budget, and matches BasecampHttpClient's auth-phase classification. The signed second hop is untouched — no retry, no auth. KOTLIN_SKIPS is now empty: the two download-retry fixtures run, and the remaining architectural skip is the tag-based link-header branch.
performDownloadRequest fired exactly one authenticated request, so downloadURL surfaced every 429/502/503/504 from the API hop straight to the caller — the one leg of the two-hop flow where a transient gateway blip is worth another attempt. It now runs the #517 directive loop: the do/catch classifies the attempt's outcome into done/retry/fail and the loop tail acts on it, so onRetry, the backoff sleep, and re-authentication never run inside a catch clause and a CancellationError from the sleep propagates raw. The policy is fixed and passed directly rather than looked up by operation — DownloadURL is deliberately absent from behavior-model.json, so there is no per-operation RetryConfig to resolve and no public numeric knob. Three attempts when enableRetry is true, exactly one when false; the declared set is {429, 502, 503, 504} plus network errors, never 500; exponential backoff from the 1-second base, honoring Retry-After on 429. Every attempt re-authenticates so a rotated token is picked up. The signed second hop is untouched: fetchSignedDownload still fires once, bare. Native tests pin the complete retry set status by status, the 500 non-retry, the fixed three-attempt exhaustion, the enableRetry = false single attempt, Authorization on every hop-1 attempt and never on hop 2, balanced start/end/onRetry hooks across retries, Retry-After, and cancellation during backoff. The Swift conformance runner lands separately (#558).
…honors
Go's fetchAPIDownload has always retried the declared hop-1 set, but the
tests only covered three of its five edges: 503, 429-with-Retry-After,
and network errors. 502 and 504 rode along untested, the 500 carve-out
was only implied by a 404 case, and nothing asserted that a retried
attempt still carries Authorization — the property that makes hop-1
retry safe to add elsewhere.
Two pins, no behavior change. A table walks the complete set status by
status, including the 500 row that must stay at one attempt and surface
a non-retryable error. A second test records the Authorization header on
every authenticated-hop attempt and on the signed hop, asserting the
credential boundary holds across a retry.
Both fail against mutated implementations: narrowing the switch to
{429, 503} reddens the 502 and 504 rows, widening it to all 5xx reddens
the 500 row, and authenticating only on attempt 1 reddens the header
test with an empty Authorization on attempt 2.
Routing download hop 1 through the shared retry primitives moved two of
the guard's watch points. TypeScript lifted the loop out of client.ts
into retry.ts, so client.ts no longer spells retryOn.includes or the
backoff math; Ruby's status gate became retry_eligible?, where the
declared set arrives either from the operation's metadata or from an
explicit set like the download flow's, so fetch("retryOn") and the
membership test are no longer one expression.
The guard now watches where the behavior actually lives: a client.ts row
for resolving the per-operation tuple and handing it to executeWithRetry,
a new retry.ts row for the loop that consumes it, and a Ruby row that
requires the metadata read and the membership test as separate tokens.
Strength is unchanged — stubbing config.retryOn.includes in retry.ts or
declared.include? in http.rb still reddens the check.
jeremy
added
enhancement
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c1e17fdfd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
13 issues found across 24 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="python/src/basecamp/_async_http.py">
<violation number="1" location="python/src/basecamp/_async_http.py:131">
P2: Retries-disabled downloads can still make a second hop-1 request after a 401 token refresh. Keep the 401 refresh retry from bypassing the download attempt cap so `max_retries=0` yields exactly one hop-1 request.</violation>
</file>
<file name="ruby/lib/basecamp/http.rb">
<violation number="1" location="ruby/lib/basecamp/http.rb:179">
P2: Download hop-1 retries now send `Accept: application/json` because they use `single_request`/`request_headers`. Scope headers by request type so this flow sends only its required auth and User-Agent headers.</violation>
</file>
<file name="python/src/basecamp/_http.py">
<violation number="1" location="python/src/basecamp/_http.py:131">
P2: Download hop-1 still sends `Accept: application/json` because this call uses the generic request path. Scope headers for this flow to Authorization and User-Agent so binary download requests meet the required per-hop header contract.
(Based on your team's feedback about scoping required headers by request type.) [FEEDBACK_USED]</violation>
<violation number="2" location="python/src/basecamp/_http.py:329">
P3: The new `DOWNLOAD_RETRY_ON` policy constant is defined identically in both `_http.py` and `_async_http.py`. This is a SPEC-policy value (not incidental), so any change to the hop-1 retry set must be edited in two places or the sync/async transports silently diverge in retry behavior. The same duplication already exists for `DEFAULT_RETRY_ON`, but this is the policy the new download feature depends on, so it is worth centralizing while the diff is fresh.</violation>
</file>
<file name="kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt">
<violation number="1" location="kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt:318">
P2: When hop 1 returns a retryable status ({429, 502, 503, 504}), the loop sleeps for backoff and starts the next attempt without discarding the prior HttpResponse. Ktor keeps the response channel and its underlying connection open until it's consumed or cancelled, so during backoff (which can be seconds, especially with Retry-After) this can tie up a connection from the pool, and concurrent downloads may end up forcing subsequent attempts onto new sockets. Consider consuming/cancelling the response body before calling delay().</violation>
<violation number="2" location="kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt:335">
P2: 429 responses with an HTTP-date `Retry-After` fall back to exponential delay instead of honoring the server throttle. Extend `parseRetryAfter` to parse RFC 7231 dates (or use a download-specific parser) before scheduling this retry.</violation>
<violation number="3" location="kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt:353">
P3: The new `DownloadAuthFailure` class duplicates the existing `AuthPhaseFailure` tag in BasecampHttpClient.kt — identical shape and identical purpose (tag an auth-strategy exception so it surfaces raw and spends no retry budget). Worth consolidating: promote `AuthPhaseFailure` to `internal` in the http package (already accessible from Download.kt) and reuse it here, so the auth-phase classification stays in one place rather than being re-declared per hop.</violation>
</file>
<file name="swift/Sources/Basecamp/HTTP/HTTPClient.swift">
<violation number="1" location="swift/Sources/Basecamp/HTTP/HTTPClient.swift:321">
P2: Hop-1 does not retry a transport failure already represented as `BasecampError.network`; this catch treats it as terminal. Classify `.network` as the network retry path while continuing to fail API/auth/usage errors immediately.</violation>
</file>
<file name="typescript/src/retry.ts">
<violation number="1" location="typescript/src/retry.ts:142">
P2: 429 responses with an HTTP-date `Retry-After` do not wait until the declared date; zero or negative values also bypass backoff. Parse positive deltas, then future HTTP-dates, and use exponential backoff for every other value.</violation>
</file>
<file name="typescript/src/download.ts">
<violation number="1" location="typescript/src/download.ts:205">
P2: A 429 download response with a valid HTTP-date `Retry-After` will retry after normal backoff instead of the server-requested time. Make this retry path use the spec-compliant positive-integer/HTTP-date parser (and fall back for zero or invalid values).</violation>
</file>
<file name="swift/Tests/BasecampTests/DownloadTests.swift">
<violation number="1" location="swift/Tests/BasecampTests/DownloadTests.swift:750">
P3: The new download retry tests incur real wall-clock backoff sleeps (~13s added to the suite) because performDownloadRequest hard-codes a 1s exponential base and 3 attempts with no test seam to shrink the delay. Consider injecting a delay clock or an internal/test-visible base-delay override for the download hop so the suite stays fast without exposing a public numeric knob; also add a timeout to the `while transport.requests.isEmpty` polling loop in the cancellation test to avoid a hang if the request never fires.</violation>
</file>
<file name="ruby/test/basecamp/download_test.rb">
<violation number="1" location="ruby/test/basecamp/download_test.rb:465">
P3: Consider pinning the "no backoff / no on_retry after the final doomed attempt" contract. Both new retry tests avoid exercising it: the balanced-hooks test ends with a 200 on attempt 3, and the exhaustion test (503x3, max_retries 3) only asserts request count, not hooks or sleep. Since the repo explicitly treats skipping the final retry sleep/on_retry as required behavior, a fully-failing variant that asserts on_retry == [2,3] and a single captured 429 delay would lock that in against regressions.</violation>
</file>
<file name="conformance/runner/python/runner.py">
<violation number="1" location="conformance/runner/python/runner.py:438">
P3: The retry-backoff verification is now limited to the first request gap. After this change (and un-skipping the download retry cases), the 503 download fixture — which has two retry gaps (503→503 and 503→302) before the terminal un-delayed redirect hop — only has its first retry gap checked. A regression that dropped the backoff on the second retry would no longer fail the conformance assertion, whereas the previous `any(d < min)` logic would have caught it. The redirect gap does need to be excluded, but it would preserve more coverage to require the minimum delay on every non-terminal gap (e.g. `any(d < min for d in delays[:-1])`) rather than only the first.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…d Ruby §14 has always said hop 1 sets Authorization and User-Agent only — "no Accept or Content-Type — this is a binary download, not a JSON API call". Python and Ruby never honored it: their download hop borrows the generic request path, which stamps Accept: application/json on every request. Go, TypeScript, Kotlin and Swift build the hop-1 headers themselves and were already correct, so this was a two-SDK divergence against an explicit clause, predating the retry work. Both transports gain an accept seam threaded from the retry entry point down to the header builder, defaulting to the JSON Accept so every other caller is untouched; the download hop passes none. Python's 401-refresh replay and Ruby's carry it through, so a refreshed retry does not silently re-acquire the header. What remains on the wire is the HTTP library's own default (httpx sends Accept: */*, Faraday sends nothing) — not something the SDK sets. The tests pin that honestly: Python asserts hop 1's Accept is identical to the bare hop-2 client's and never application/json; Ruby asserts no attempt carries the JSON Accept. Both fail against the un-fixed transports with the header present on every hop-1 attempt.
The download hop-1 loop had declared its own DownloadAuthFailure, an exact copy of BasecampHttpClient's AuthPhaseFailure — same shape, same job: mark an auth-strategy throw so it surfaces raw and spends no retry budget. Two copies means the classification can drift per hop, so AuthPhaseFailure is now internal and the download loop uses it. The Swift cancellation test spun on `while transport.requests.isEmpty` with no ceiling, so a regression that never issued the first attempt would hang the suite instead of failing it. It now gives up after five seconds with a named failure.
This was referenced Aug 1, 2026
rubocop's Rails/RefuteMethods prefers the assert_not_* spelling.
ruff's line-length allows the single-line call.
… index Two corrections from the fourth review pass. The previous commit made the 401 token-refresh replay draw from the transient-retry budget. That regressed SPEC §4, which is explicit: refresh is attempted at most once PER REQUEST, tracked "with a boolean (e.g. refresh_attempted) rather than a counter", and it is not subordinate to the retry cap. Under the change, max_retries: 1 refreshed the token and then rethrew the stale 401 without ever sending the refreshed request — for every GET in Python and Ruby, not just downloads. Reverted. The tension that prompted it is real but is a spec question, not a bug: §4 replays once per request, §14 promises exactly one hop-1 attempt when retry is disabled, and a 401 with retry off satisfies only one of them. #565 is where that gets settled. Both SDKs now pin the behavior the spec actually prescribes, so changing it later has to be a decision rather than a drift. The download fixtures also asserted a second retry gap that three runners could not see: Go, TypeScript and Kotlin always measured requestTimes[1] - requestTimes[0] and ignored the index, so an implementation that dropped its second backoff still passed there. All five runners now resolve the gap index, and a named gap that does not exist fails rather than passing silently — otherwise a dropped retry makes the assertion vanish instead of firing. Proven by raising the gap-1 minimum: Go reports "Expected delay >= 16m39.999s at gap 1, got 2.028111541s", the second exponential backoff it previously never looked at.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aca2c481b8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
12 issues found across 27 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="conformance/runner/typescript/runner.test.ts">
<violation number="1" location="conformance/runner/typescript/runner.test.ts:805">
P3: The new named-gap check can silently pass. Because the `expect(times.length).toBeGreaterThan(gap + 1)` guard only runs inside the existing `if (times.length >= 2)`, a fixture whose gap (even the default index 0) has fewer than 2 recorded requests skips the whole delay assertion instead of failing — contradicting the new 'fails rather than passing silently' comment and potentially masking a regression where the SDK stopped retrying. Consider hoisting the length check above the `if` so a missing gap always fails loudly.</violation>
</file>
<file name="conformance/runner/python/runner.py">
<violation number="1" location="conformance/runner/python/runner.py:442">
P2: An out-of-range `delayBetweenRequests.index` can crash the Python runner (large negative) or make its delay assertion pass without checking anything (large positive). Normalize negative gap indexes and record a failure when the selected gap is absent, matching the runner's indexed-request semantics.</violation>
</file>
<file name="conformance/runner/ruby/runner.rb">
<violation number="1" location="conformance/runner/ruby/runner.rb:539">
P2: An indexed delay assertion passes when its selected gap does not exist, masking a missing retry delay. Treat an out-of-range `index` as an assertion failure before comparing the delay.</violation>
</file>
<file name="python/src/basecamp/_http.py">
<violation number="1" location="python/src/basecamp/_http.py:131">
P1: Downloads with a refreshable-token 401 exceed the declared total-attempt cap: `max_retries=0`/`1` still sends a second Hop-1 request during the internal refresh replay. Account for that replay in the Hop-1 budget, or disable it for this policy, so retry-disabled downloads make exactly one request.</violation>
</file>
<file name="typescript/src/retry.ts">
<violation number="1" location="typescript/src/retry.ts:142">
P2: 429 responses with an HTTP-date `Retry-After` retry after the exponential delay instead of the server-requested time. Parse a valid HTTP-date after rejecting an invalid/nonpositive delta-seconds value; avoid `parseInt` partial parses such as `"1junk"` too.</violation>
</file>
<file name="kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt">
<violation number="1" location="kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt:418">
P2: Retry conformance cases with an omitted `index` validate only gap 0, not every inter-request gap required by the fixture schema. Preserve whether `index` was omitted and iterate all gaps in that case; otherwise an SDK can pass despite skipping or shortening later retry backoff.</violation>
</file>
<file name="kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt">
<violation number="1" location="kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt:321">
P3: Large allowed `maxRetries` values make hop-1 backoff overflow after the 55th failed attempt, so retries no longer use exponential delays and hooks receive a wrapped delay. Saturate/cap the backoff calculation (or bound the accepted retry count) before scheduling it.</violation>
</file>
<file name="conformance/runner/go/main.go">
<violation number="1" location="conformance/runner/go/main.go:952">
P2: The new bounds-check only runs when at least 2 requests were recorded (outer `if len(requestTimes) >= 2` guard stays), so a fully dropped retry that yields a single request still makes this delay assertion pass silently — the exact case the added comment claims to prevent. Consider removing the outer len>=2 guard and letting the `gap+1 >= len(requestTimes)` check fail whenever the requested gap has fewer recorded requests, so a vanished retry is pinned for the default gap to too.</violation>
</file>
<file name="python/tests/test_download.py">
<violation number="1" location="python/tests/test_download.py:354">
P3: The sync and async suites (`TestHop1Retry` and `TestHop1RetryAsync`) duplicate nearly all seven scenarios verbatim, differing only in `download_sync` vs `await download_async`, the transport helper, and a `pytest.mark.asyncio` marker. This doubles the surface area for the retry policy — any future policy change (retry set, backoff, cap semantics) has to be edited in two places or the suites silently drift apart. Consider sharing the scenarios through a transport-parameterized helper that runs the same table against both sync and async paths.</violation>
</file>
<file name="typescript/tests/download.test.ts">
<violation number="1" location="typescript/tests/download.test.ts:562">
P3: This test adds a real 1-second sleep and asserts on wall-clock elapsed time, which is inherently timing-sensitive and slows the suite. Since the retry loop already passes the computed delay to the onRetry hook, you can verify Retry-After deterministically and instantly by returning 429 with `Retry-After: 1` and asserting the onRetry delayMs is 1000 (the Retry-After path skips jitter), dropping the `performance.now()` threshold check entirely.</violation>
</file>
<file name="swift/Tests/BasecampTests/DownloadTests.swift">
<violation number="1" location="swift/Tests/BasecampTests/DownloadTests.swift:782">
P3: These new download tests couple correctness to wall-clock scheduling: two use hardcoded 5s polling budgets (1ms step) to detect when a hop-1 attempt reaches the transport, and the Retry-After test asserts `elapsed >= 2.0` against a real 2s sleep. On a loaded CI runner, a delayed first attempt can trip the fixed `waited < 5_000` budget and fail spuriously, and the cumulative sleeps add ~7s of serial latency to every run. Consider loosening the poll budget (retry a few times proportionally rather than a fixed bound) or restructuring to avoid the timing assertions, so retry semantics are asserted via hook/counter state rather than elapsed time.</violation>
</file>
<file name="swift/Sources/Basecamp/HTTP/HTTPClient.swift">
<violation number="1" location="swift/Sources/Basecamp/HTTP/HTTPClient.swift:201">
P2: Cancelling an in-flight URLSession request can still fire `onRetry` and be classified as a retryable network failure because URLSession cancellation is `URLError(.cancelled)`, not necessarily `CancellationError`. Treat `URLError.cancelled` (and task cancellation) as terminal in both retry loops.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Member
Author
|
Bot-review status: cubic delivered two substantive rounds (1×P1, 6×P2, 4×P3 in the second) — all addressed or refuted with evidence and every thread resolved; no suppressed-comment blocks in any review body. Codex posted no findings; Copilot errored, as it has on every PR in this series. Merging on convergence + green CI per the standing convention. |
jeremy
added a commit
that referenced
this pull request
Aug 1, 2026
…annot reach
Second review round on this PR. Five findings from cubic, two from the B4
agent, and one class the round exposed that neither had named: the runners'
assertion logic had no tests of its own, so every bounds branch here was
written blind and verified only by fixtures that pass.
An index of Int.MAX_VALUE crashed the Kotlin runner instead of failing its
assertion: `gap + 1 >= size` does the addition first, so the value wraps
negative and sails through the guard into an out-of-bounds read. Go had the
identical shape. Both now compare against the gap count and never add. Ruby
and Python were already safe — they compare against the gap count directly.
TypeScript did not reject a negative gap index at all: `toBeGreaterThan(index
+ 1)` is trivially satisfied by a negative, so `times[-1]` produced NaN and a
message about a delay rather than an out-of-range index — contradicting what
the other four runners did. All five now distinguish the two failures: a
negative index is refused categorically, which has nothing to do with how many
requests were made.
A `min` of zero silently disabled the whole assertion in Python and Ruby. Both
gated the check on the value's truthiness, and `0` is falsy in Python while an
absent `min` is nil in both — so `{"type": "delayBetweenRequests", "min": 0}`
asserted nothing at all. That is the exact false-green class this PR exists to
kill, reintroduced one layer up. The default now lands INSIDE the checked
function rather than at the call site, so no caller can gate it away, and a
zero minimum still requires the gap to EXIST.
Making the Kotlin index nullable to tell "omitted" from "0" also regressed
diagnostics: requestPath, requestMethod, requestBody and the header assertions
interpolated `assertion.index` into their failure messages, which printed
`index null` for an assertion that had actually used index 0. Each branch now
resolves the index once and reports the resolved value.
The Swift cancellation test caught every error and asserted nothing about it,
so it would have passed even if the cancellation were wrapped as
BasecampError.network — the exact regression it exists to catch. It now
requires URLError(.cancelled) to arrive raw. Inserting isCancellation above
sleepNanoseconds had also orphaned a doc comment, leaving generated
documentation claiming isCancellation clamps Retry-After; the comments are
reordered onto their own functions.
Underneath all of that: the delayBetweenRequests contract is now one function
per runner (checkDelayGaps / DelayGaps.check / check_delay_gaps) with a
committed unit test beside it, wired into `make check` and CI through a new
conformance-runner-tests target. Ten cases per runner, each named for a
behavior that regressed here or on #563 — omitted index catching a LATER
failing gap, omitted index with zero gaps failing, a named gap the run never
produced failing, a negative index rejected, an enormous index failing without
overflow, and a zero minimum still requiring the gap to exist. The runners are
test harnesses, but their assertion logic is code, and its bounds branches
never execute against a fixture that passes — which is precisely how #563
shipped an assertion that vacuously passed.
One residual false-green closed with them: an omitted index over zero gaps
passed in all five runners while checking nothing. Existing fixtures happen to
pair the delay assertion with requestCount, which limited the exposure, but the
contract itself was unsound and #558 is about to build conformance claims on
it.
conformance/schema.json and SPEC §19 now state the whole contract: never
vacuous, bounds-checked unconditionally, negatives rejected.
jeremy
added a commit
that referenced
this pull request
Aug 1, 2026
…sion's cancellation (#568) * Make the gap index bite in every runner and catch URLSession's cancellation Three follow-ups from the fourth review pass, all on code this branch added. The schema documents an omitted delayBetweenRequests index as "require the minimum on every gap", but Go, TypeScript and Kotlin still measured only gap 0 — so the documentation promised coverage three runners did not provide. All five now iterate every gap when the index is omitted. A NAMED gap that does not exist now fails in all five. The bounds check had been sitting inside each runner's "did we record two requests" guard, so a fully dropped retry left one request and the assertion evaporated instead of firing — the worst outcome for a timing pin. Ruby and Python additionally reject a negative index rather than wrapping to the end the way the per-request assertions do; there is no sensible "last gap" semantic when the point is to name a specific backoff. Swift only recognised Swift-concurrency cancellation. URLSession reports a cancelled task as URLError(.cancelled), not CancellationError, so a genuinely cancelled download was still classified a retryable network blip and spent the whole budget. Both loops now test for either shape through a shared isCancellation, pinned by a test that fails four ways against the previous commit. * Make the gap-index bounds bite, and unit-test the branches fixtures cannot reach Second review round on this PR. Five findings from cubic, two from the B4 agent, and one class the round exposed that neither had named: the runners' assertion logic had no tests of its own, so every bounds branch here was written blind and verified only by fixtures that pass. An index of Int.MAX_VALUE crashed the Kotlin runner instead of failing its assertion: `gap + 1 >= size` does the addition first, so the value wraps negative and sails through the guard into an out-of-bounds read. Go had the identical shape. Both now compare against the gap count and never add. Ruby and Python were already safe — they compare against the gap count directly. TypeScript did not reject a negative gap index at all: `toBeGreaterThan(index + 1)` is trivially satisfied by a negative, so `times[-1]` produced NaN and a message about a delay rather than an out-of-range index — contradicting what the other four runners did. All five now distinguish the two failures: a negative index is refused categorically, which has nothing to do with how many requests were made. A `min` of zero silently disabled the whole assertion in Python and Ruby. Both gated the check on the value's truthiness, and `0` is falsy in Python while an absent `min` is nil in both — so `{"type": "delayBetweenRequests", "min": 0}` asserted nothing at all. That is the exact false-green class this PR exists to kill, reintroduced one layer up. The default now lands INSIDE the checked function rather than at the call site, so no caller can gate it away, and a zero minimum still requires the gap to EXIST. Making the Kotlin index nullable to tell "omitted" from "0" also regressed diagnostics: requestPath, requestMethod, requestBody and the header assertions interpolated `assertion.index` into their failure messages, which printed `index null` for an assertion that had actually used index 0. Each branch now resolves the index once and reports the resolved value. The Swift cancellation test caught every error and asserted nothing about it, so it would have passed even if the cancellation were wrapped as BasecampError.network — the exact regression it exists to catch. It now requires URLError(.cancelled) to arrive raw. Inserting isCancellation above sleepNanoseconds had also orphaned a doc comment, leaving generated documentation claiming isCancellation clamps Retry-After; the comments are reordered onto their own functions. Underneath all of that: the delayBetweenRequests contract is now one function per runner (checkDelayGaps / DelayGaps.check / check_delay_gaps) with a committed unit test beside it, wired into `make check` and CI through a new conformance-runner-tests target. Ten cases per runner, each named for a behavior that regressed here or on #563 — omitted index catching a LATER failing gap, omitted index with zero gaps failing, a named gap the run never produced failing, a negative index rejected, an enormous index failing without overflow, and a zero minimum still requiring the gap to exist. The runners are test harnesses, but their assertion logic is code, and its bounds branches never execute against a fixture that passes — which is precisely how #563 shipped an assertion that vacuously passed. One residual false-green closed with them: an omitted index over zero gaps passed in all five runners while checking nothing. Existing fixtures happen to pair the delay assertion with requestCount, which limited the exposure, but the contract itself was unsound and #558 is about to build conformance claims on it. conformance/schema.json and SPEC §19 now state the whole contract: never vacuous, bounds-checked unconditionally, negatives rejected. * Report the real request count, and install the Ruby bundle before its helper test Third review round, two findings. The out-of-range diagnostic in Python and Ruby inferred the request count as `len(delays) + 1`. That inference assumes at least one request was made, so a run that failed during construction or dispatch — before the tracker recorded anything — reported "only 1 request(s) were made" when the true count was zero, pointing at the wrong failure. Both now take the count from the tracker and report it; Go, TypeScript and Kotlin already measured request times directly and were accurate. `make conformance-runner-tests` also ran `bundle exec` in the Ruby runner before anything had installed its gems. `make conformance` reaches the helper tests ahead of the `conformance-ruby` recipe that does the install, and invoking the target on its own had no installer at all, so a clean checkout hit a Bundler error instead of the test. It now mirrors the runner recipe with `bundle install --quiet` first.
jeremy
added a commit
that referenced
this pull request
Aug 1, 2026
…EC §4 to match (#571) * Count the 401 refresh replay against the attempt budget, and amend SPEC §4 to match `max_retries: 0` documented one attempt and sent two. A refreshable 401 put a second request on the wire from inside the single-request primitive, governed by its own counter rather than the caller's budget — so the promise §14 makes for downloads ("disabling retry yields exactly ONE hop-1 attempt") was false in Python and Ruby, and the total-attempt semantics #461 settled leaked. Counting the replay was tried once on #563 and reverted, correctly: on its own it regresses SPEC §4, which says refresh is attempted once PER REQUEST, tracked "with a boolean rather than a counter". Gated only by "have we already refreshed", `max_retries: 1` refreshed the token and then rethrew the stale 401 without ever sending the refreshed request — for every GET, not just downloads. That was worse than the bug. So this lands both halves together, which is what makes it a decision rather than a drift. The replay counts against the budget, AND §4 is amended to attempt the refresh only when another attempt remains. The gate is checked BEFORE refresh() rather than after: rotating a token the SDK has no budget left to use burns it for nothing and hands the caller the stale 401 anyway, so declining to refresh is both cheaper and easier to state. This supersedes §4's unqualified per-request reading and closes #565. The consequence is stated plainly in §4 rather than left to be discovered: with a budget of one attempt, a refreshable 401 is NOT replayed and surfaces as auth_required. Callers who want the replay must leave an attempt for it. Scope is Python (sync and async) and Ruby — the two SDKs whose transports carry a 401 replay under a governed budget. Go's main GET loop already counts it: a 401 with a successful refresh returns a retryable error that the loop re-issues as the next attempt, subject to MaxRetries. Its hand-written mutation path replays outside any budget, but mutations have no transient-retry budget to draw from — that is the documented divergence in §7, and §4's new gate says explicitly that it binds where a total-attempt budget governs the path. TypeScript, Kotlin and Swift have no 401 replay in the transport at all. Direct single-request callers keep the in-primitive replay. Python's and Ruby's mutations bypass the retry loop entirely, so without the seam they would lose 401 refresh outright; `refresh_replay:` marks which side owns it. In Ruby that also stops handle_error from rotating credentials as a side effect of classifying an error — the refresh now happens where the budget is known. Red-proofed against merged main on the plain GET path as well as the download hop, since the every-GET case is exactly what the earlier revert protected: tests/test_http.py::TestRefreshReplayAttemptBudget::test_no_budget_means_one_request_and_no_refresh E AssertionError: assert 2 == 1 tests/test_http.py::TestRefreshReplayAttemptBudget::test_replay_and_transient_retries_share_one_budget tests/test_download.py::TestHop1Retry::test_refresh_replay_is_not_attempted_without_budget and in Ruby: The request GET https://3.basecampapi.com/test.json was expected to execute 1 time but it executed 2 times The request GET https://3.basecampapi.com/test.json was expected to execute 2 times but it executed 3 times The request GET .../download/file.txt was expected to execute 1 time but it executed 2 times The middle one is the load-bearing case: at `max_retries: 2` the replay used to ride outside the cap for a total of three requests. Positive tests pin that the refreshed request is actually SENT rather than refreshed-then-discarded, and that mutations keep their replay. * Classify in the handler, refresh outside it, so a failing token endpoint still retries An exception raised inside an `except` suite is not offered to that `try`'s sibling handlers — same in Ruby for `rescue`. Calling refresh() from inside the AuthError handler therefore put the token endpoint outside the retry loop's reach: a NetworkError from a timing-out refresh escaped the whole loop and ended the request with budget still unspent. Before this branch the replay lived in the single-request primitive, where that failure landed in the transient handler and retried, so this was a regression introduced by moving it. Both loops now follow the shape the Swift and Kotlin download loops already use (#517): the handlers only CLASSIFY the attempt, and every side effect runs at the loop tail, outside any handler. A refresh that raises is classified as a transient failure of this attempt and retries under the same budget; a refresh that returns false surfaces the original 401; a refresh that succeeds replays immediately with no backoff, since the token is fresh and the server never asked us to wait. Red-proofed against the previous commit in both languages — the refresh's NetworkError propagated raw out of `client.get(...)` rather than being retried: E basecamp.errors.NetworkError: token endpoint timed out FAILED tests/test_http.py::TestRefreshReplayAttemptBudget::test_refresh_network_failure_still_retries_under_the_budget * Spend the one allowed refresh on the attempt, not on the success SPEC §4 tracks refresh with an "attempted" boolean — the algorithm's own condition is "refresh has not yet been attempted for this request" — but the flag was set only after a refresh returned true. A refresh that RAISES left it false, so the next 401 in the same request called the provider again. Reachable as soon as the token endpoint is flaky: attempt 1 401s, refresh throws a NetworkError, the attempt retries under the budget, attempt 2 401s with the same unchanged token, and refresh fires a second time. Beyond violating the at-most-once rule, that is unsafe with a rotating refresh token — if the first call reached the server and rotated before its response was lost, the second spends a credential that is already dead. All three paths now mark the attempt before invoking the provider. Red-proofed against the previous commit, with a provider that always throws and a server that always 401s: > assert provider.refreshes == 1 E assert 2 == 1 The existing network-failure tests missed this because their second request returns 200, so no second 401 ever arrives to trigger the second refresh.
jeremy
added a commit
that referenced
this pull request
Aug 1, 2026
A verification sweep found the new runner reporting green on things it was not checking. A conformance runner that does that is worse than no runner: it converts an untested SDK into a tested-looking one. The delayBetweenRequests evaluator measured gap 0 and ignored the assertion's index, and skipped the check entirely below two requests — the identical defect #568 had just fixed in the other five. It mattered here and not hypothetically: the two download-retry fixtures this branch un-skips carry index 0 AND index 1 assertions, so the second backoff was never measured. checkDelayGaps moves into an SDK-free ConformanceSupport target with #568's semantics and its committed test roster ported; 12 of the 13 cases fail against the evaluator as shipped. An executable target carrying @main cannot host XCTest, which is why the split exists. The two download skips are gone. #563 landed the authenticated hop-1 retry, and both cases now pass live rather than being deleted on faith. Nothing capability-shaped is skipped any more: the roster is empty and the only standing exclusion is the architectural link-header branch Kotlin and TypeScript share. The rest are review findings on this branch's own code, each a way a malformed fixture could have passed: - A case with no assertions ran an operation and verified nothing. An empty assertions array is schema-legal, so the fixture gate does not catch it. - An absent mockResponses key decoded as an empty queue, which is a deliberate declaration for the HTTPS case and a malformed fixture everywhere else. The two no longer collapse. - networkError: false alongside a status slipped past the exactly-one-of backstop and was served as a plain success. The schema pins the literal true; so does the runner now. - Path parameters coerced to 0 when missing or non-integral, so the request went to a different resource and the scripted queue answered it anyway. They throw. The two timesheet arms used a `== 0` sentinel to pick between two spellings of one key, which could not tell an absent key from an id of zero; they ask for the first key present instead. - configOverrides.maxItems reaches the SDK through one dispatch arm. Any other operation would have paginated unbounded while the fixture believed it had capped the walk, so that now fails loudly instead. - headerInjected ignored its index and validated the first request, which the schema documents as index-aware and the other runners implement. - errorType: "ambiguous" could never pass despite Swift mapping it. - An explicit expected: null compared nil against the literal "null" and always failed. - The HTTPS probe routed only http:// to the crash child, but the SDK traps on every non-HTTPS scheme outside the localhost carve-out, so an ftp:// or ws:// fixture would have taken the whole run down mid-suite. - SWIFT_CONFORMANCE_NO_SKIPS was enabled by the variable's presence, so an inherited empty value silently changed coverage. It compares to "1". One of these was not a runner bug at all. The single-key array unwrap that lets list fixtures decode also rewrote {"payload_url": ["is not a valid URL"]} into a bare array, so the SDK found no field errors and reported "bad request" — a false FAIL, and the same heuristic could as easily manufacture a pass. Kotlin took the success-only status guard in #549; this port predated it and now carries it too. SPEC retires the "where a runner does not exist yet, e.g. Swift" carve-out, adds swift to the §21 gate roster, and rosters the one architectural skip.
jeremy
added a commit
that referenced
this pull request
Aug 1, 2026
Swift was the only SDK not fixture-verified; the #508/#509 retry-lifecycle bugs lived exactly where no runner watched. This adds the sixth conformance runner, executing every mock fixture in conformance/tests/ through the SDK's public Transport seam — no @testable anywhere. 136 passed, 0 failed, 1 skipped of 137; the one skip is the architectural link-header line Kotlin and TypeScript share. Seven rounds of review turned it into the opposite of what it started as. The runner shipped with the same delayBetweenRequests defect #568 had just fixed in the other five — gap 0 only, index ignored, check skipped below two requests — which mattered because the two download fixtures this branch un-skips carry index 0 AND index 1. checkDelayGaps moves into an SDK-free ConformanceSupport target with #568's semantics and its test roster ported; 12 of 13 cases fail against the evaluator as shipped. Sixteen more findings, each a way a fixture could pass while testing nothing: a case with no assertions, an absent response queue, networkError:false, path params coerced to 0, maxItems silently ignored, headerInjected ignoring its index, errorType ambiguous unreachable, expected:null always failing, the HTTPS probe crashing the run on a non-http scheme, NO_SKIPS triggering on presence, requestCount as a lower bound that made the pagination cap assertions unfailable, no path check at all, an exemption too coarse to cover request zero, path and method checked on the first hop only, an unscoped request accepted, the query string dropped so refetching page 1 looked like following the link, and an operation short-circuited before the transport skipping every invariant at once. Eight carry a red proof against the un-fixed code. One was not a runner bug: the single-key array unwrap rewrote error bodies, so a bare field map decoded as an array and the SDK reported 'bad request'. Kotlin took the success-only status guard in #549; this port predated it. The two download-retry skips are deleted and pass live under SWIFT_CONFORMANCE_NO_SKIPS=1, #563 having landed the hop-1 retry. Five shared download fixtures gained indexed requestPath assertions, so all six runners now pin the redirect target rather than only counting hops. CI: test-swift runs the runner unit tests and the conformance suite on macos-15, and is in the required Conformance Tests fan-in's needs. SPEC retires the 'where a runner does not exist yet, e.g. Swift' carve-out, adds swift to the §21 gate roster, and rosters the one architectural skip. Follow-up: #573 (Kotlin carries the same requestCount lower bound).
Member
Author
|
Disposition of the 13 review threads left open when this merged. This PR was merged prematurely on a truncated thread query ( Fixed in #568 (
Fixed in #571 (
Tracked, not fixed:
Judged test-hygiene, no action: the sync/async duplication in Resolving all 13 against these dispositions. The durable lesson is recorded: a census must cursor-paginate, take its count from |
jeremy
added a commit
that referenced
this pull request
Aug 3, 2026
…d six runner unit tests Three verifier findings on #597, all about the new assertion proving less than it claimed. The Cards kill cases did not discriminate in TypeScript. The generated updateVerbatim guards due_on with /^\d{4}-\d{2}-\d{2}$/.test(req.dueOn), and RegExp.test coerces its argument to a string first, so ["x"] ("x") and {} ("[object Object]") were already rejected before the PUT with or without the guard -- TypeScript conformance failed 2 kill cases against the unfixed composites, not 4, and TypeScript Cards had no regression protection at all. A fifth case uses ["2024-02-01"], which String() renders as exactly "2024-02-01": the format check waves it through and only the guard stops it. It still discriminates in Python and Ruby, and Go, Kotlin and Swift reject it structurally as they do any JSON array in a String field, so the shared fixture stays shared. The errorRaised handler had no unit test in any runner. Its failing branch is unreachable from conformance/tests/ -- every case declaring it is one the SDK does refuse -- so a handler that accepted everything would report green in all six runners at once, which is how #563 shipped a vacuous delayBetweenRequests check. The predicate is split out per runner and tested on both directions, with the message pinned verbatim in all six. Go also asserts the wiring, since a typo'd case label would fall through to the default and assert nothing. evaluateAssertions(dispatchFailed:) loses its default. The one call site passes it, but the default fails closed: a future call site that omitted it would report "the call succeeded" on a call that did not, reddening every errorRaised fixture far from the actual bug. Also fixes the Codex P2: the Swift HTTPS-enforcement probe recorded caughtError without setting dispatchFailed, so a trapped child process read as a successful call. Runner.swift now flags it, and both Swift and Kotlin derive the assertion from the union of the two signals rather than from call-site discipline.
jeremy
added a commit
that referenced
this pull request
Aug 3, 2026
…d six runner unit tests Three verifier findings on #597, all about the new assertion proving less than it claimed. The Cards kill cases did not discriminate in TypeScript. The generated updateVerbatim guards due_on with /^\d{4}-\d{2}-\d{2}$/.test(req.dueOn), and RegExp.test coerces its argument to a string first, so ["x"] ("x") and {} ("[object Object]") were already rejected before the PUT with or without the guard -- TypeScript conformance failed 2 kill cases against the unfixed composites, not 4, and TypeScript Cards had no regression protection at all. A fifth case uses ["2024-02-01"], which String() renders as exactly "2024-02-01": the format check waves it through and only the guard stops it. It still discriminates in Python and Ruby, and Go, Kotlin and Swift reject it structurally as they do any JSON array in a String field, so the shared fixture stays shared. The errorRaised handler had no unit test in any runner. Its failing branch is unreachable from conformance/tests/ -- every case declaring it is one the SDK does refuse -- so a handler that accepted everything would report green in all six runners at once, which is how #563 shipped a vacuous delayBetweenRequests check. The predicate is split out per runner and tested on both directions, with the message pinned verbatim in all six. Go also asserts the wiring, since a typo'd case label would fall through to the default and assert nothing. evaluateAssertions(dispatchFailed:) loses its default. The one call site passes it, but the default fails closed: a future call site that omitted it would report "the call succeeded" on a call that did not, reddening every errorRaised fixture far from the actual bug. Also fixes the Codex P2: the Swift HTTPS-enforcement probe recorded caughtError without setting dispatchFailed, so a trapped child process read as a successful call. Runner.swift now flags it, and both Swift and Kotlin derive the assertion from the union of the two signals rather than from call-site discipline.
jeremy
added a commit
that referenced
this pull request
Aug 3, 2026
…d six runner unit tests Three verifier findings on #597, all about the new assertion proving less than it claimed. The Cards kill cases did not discriminate in TypeScript. The generated updateVerbatim guards due_on with /^\d{4}-\d{2}-\d{2}$/.test(req.dueOn), and RegExp.test coerces its argument to a string first, so ["x"] ("x") and {} ("[object Object]") were already rejected before the PUT with or without the guard -- TypeScript conformance failed 2 kill cases against the unfixed composites, not 4, and TypeScript Cards had no regression protection at all. A fifth case uses ["2024-02-01"], which String() renders as exactly "2024-02-01": the format check waves it through and only the guard stops it. It still discriminates in Python and Ruby, and Go, Kotlin and Swift reject it structurally as they do any JSON array in a String field, so the shared fixture stays shared. The errorRaised handler had no unit test in any runner. Its failing branch is unreachable from conformance/tests/ -- every case declaring it is one the SDK does refuse -- so a handler that accepted everything would report green in all six runners at once, which is how #563 shipped a vacuous delayBetweenRequests check. The predicate is split out per runner and tested on both directions, with the message pinned verbatim in all six. Go also asserts the wiring, since a typo'd case label would fall through to the default and assert nothing. evaluateAssertions(dispatchFailed:) loses its default. The one call site passes it, but the default fails closed: a future call site that omitted it would report "the call succeeded" on a call that did not, reddening every errorRaised fixture far from the actual bug. Also fixes the Codex P2: the Swift HTTPS-enforcement probe recorded caughtError without setting dispatchFailed, so a trapped child process read as a successful call. Runner.swift now flags it, and both Swift and Kotlin derive the assertion from the union of the two signals rather than from call-site discipline.
jeremy
added a commit
that referenced
this pull request
Aug 3, 2026
* Refuse a malformed GET field instead of writing it back (#576) The shipped Todos and Cards merge-safe composites in Python, Ruby and TypeScript read each writable field off a GET and PUT the FULL representation back. Every value read is therefore a value written -- on a call that never mentioned the field -- and none of the three validated what they read. Two failure modes, the same defect wearing different clothes: erasure a falsey non-string coalesced away, wiping the field corruption a non-string forwarded verbatim, writing a number, boolean, array or object where a string belongs Probed against the unfixed code, one call each, `update(content:)` and `update(title:)`: Python Todos description=False,0,[],{} -> PUT description="" description=42,True,["x"] -> PUT description=42 / True / ["x"] assignees[0].id="100" -> PUT assignee_ids=["100"] Python Cards due_on=False,0,[],{} -> PUT with due_on OMITTED, which is exactly how BC3 erases the date due_on=42,True,["x"] -> PUT due_on=42 / true / ["x"] Ruby Todos description=false -> PUT description="" description=0,[],{},42,... -> PUT description=0 / [] / {} / 42 Ruby Cards every shape -> PUT due_on=<shape verbatim> TS Todos all eight shapes -> PUT description=<shape verbatim> TS Cards due_on=false,0 -> PUT with due_on OMITTED (erased) All three now treat an absent key or an explicit null as genuinely empty, pass an actual string verbatim, and raise before the PUT naming the field. The ID-list fields get the analogous check: an array, of objects, each with an integer id. One level up, the response itself must be an object -- on main a scalar or null body produced a raw TypeError/AttributeError instead of the documented statusless api_error. The rule underneath: a composite is safe exactly when a decoder REJECTS a wrong-typed field at runtime, not when a type merely claims one. Go (json.Unmarshal) and Swift (Codable) genuinely refuse. TypeScript's schema.d.ts is erased at build time and the generated Python and Ruby services return an untyped dict/Hash, so those three do it by hand, in a shared per-language helper (_merge_safe.py, merge_safe.rb, merge-safe.ts) rather than six copies. Kill coverage lands in the SHARED conformance fixtures, not per language. This defect survived five consecutive review passes because each pass fixed one instance; a shared fixture catches every instance at once, in every runner, permanently. Four cases across todos_write.json and cards_write.json assert errorRaised + requestCount 1 -- the guard must fire BEFORE the PUT, because a guard that fires after has already lost the field. errorRaised is a new assertion type, the code-agnostic inverse of noError: the six SDKs refuse the same body by two different mechanisms (hand-written guard vs model decoder) that share no canonical error code. Declaring it also tells the Kotlin and Swift runners that a decoder rejection is the point of the case rather than an under-specified fixture body. Writing that fixture immediately earned its keep: it found a FOURTH affected language. Kotlin's client-wide `Json { isLenient = true }` coerces a JSON scalar into a String field, so `"description": 42` decodes to "42" and the composite writes it back -- proven on the wire with a temporary requestBody assertion. #576 lists Kotlin as structurally safe; it is not, for scalars. It cannot be fixed by this PR's pattern either, since the coercion happens at decode and the composite only ever sees a String, so the fixtures use array/object shapes (which kotlinx.serialization does reject) and the scalar hole is filed separately. Red proof, against unfixed composites: 85 Python, 81 Ruby, 78 TypeScript unit failures, and 4/4/2 conformance kill-case failures (py/rb/ts). Go, Kotlin and Swift pass the kill cases both before and after, which is the point. Deliberately out of scope: the caller-side mirror (a closure assigning 42 inside edit), the Kotlin lenient-decoder hole, and the generated validating layer that would make all of these guards deletable (#578). * Close the errorRaised coverage gaps: a TS-discriminating kill case and six runner unit tests Three verifier findings on #597, all about the new assertion proving less than it claimed. The Cards kill cases did not discriminate in TypeScript. The generated updateVerbatim guards due_on with /^\d{4}-\d{2}-\d{2}$/.test(req.dueOn), and RegExp.test coerces its argument to a string first, so ["x"] ("x") and {} ("[object Object]") were already rejected before the PUT with or without the guard -- TypeScript conformance failed 2 kill cases against the unfixed composites, not 4, and TypeScript Cards had no regression protection at all. A fifth case uses ["2024-02-01"], which String() renders as exactly "2024-02-01": the format check waves it through and only the guard stops it. It still discriminates in Python and Ruby, and Go, Kotlin and Swift reject it structurally as they do any JSON array in a String field, so the shared fixture stays shared. The errorRaised handler had no unit test in any runner. Its failing branch is unreachable from conformance/tests/ -- every case declaring it is one the SDK does refuse -- so a handler that accepted everything would report green in all six runners at once, which is how #563 shipped a vacuous delayBetweenRequests check. The predicate is split out per runner and tested on both directions, with the message pinned verbatim in all six. Go also asserts the wiring, since a typo'd case label would fall through to the default and assert nothing. evaluateAssertions(dispatchFailed:) loses its default. The one call site passes it, but the default fails closed: a future call site that omitted it would report "the call succeeded" on a call that did not, reddening every errorRaised fixture far from the actual bug. Also fixes the Codex P2: the Swift HTTPS-enforcement probe recorded caughtError without setting dispatchFailed, so a trapped child process read as a successful call. Runner.swift now flags it, and both Swift and Kotlin derive the assertion from the union of the two signals rather than from call-site discipline. * Keep the errorRaised kill fixtures honest with a control-sibling gate Codex P2 on Runner.swift: declaring errorRaised switches OFF the #555 stop-on-mismatch policy in the decoder-backed runners. Swift's DecodingError branch and Kotlin's MissingFieldException/SerializationException branches normally fail loudly when a mock body no longer decodes into the generated model; when the fixture declares errorRaised they treat the refusal as the behaviour under test and pass. So if a model later gains a required field, or any unrelated field in one of these large bodies drifts, the decode fails for a reason unrelated to the field under test -- and errorRaised, requestCount: 1, requestMethod and requestPath all still hold. The case keeps passing and stops proving anything. The protection turns out to already exist, structurally: every kill body is a passing case's body with exactly one field perturbed, and that sibling does NOT declare errorRaised, so it keeps the full #555 policy and fails loudly on drift. Cards kill cases pair with update-preserves-due-on (differing only in due_on), Todos with update-merge (differing only in description). But nothing enforced the coupling -- edit one body without the other and it silently breaks. So enforce the claim rather than asserting it in a comment, which is the #576 lesson applied to #576's own fixtures: for every case declaring errorRaised, some case in the same file that does not declare it must have a mock body with the identical key set, differing in exactly one field. Runs in make conformance-fixtures-check, which CI already invokes. Proven non-vacuous: perturbing a kill body in a second field fails the gate (exit 1) and names the case, the missing control and the repair. * Pin the control gate to the response the kill case actually decodes Codex P2 on the gate added a commit ago: it matched a kill body against ANY queued response of ANY control case. Every kill case queues two object-shaped responses -- response 0 is the GET whose decoder rejection is under test, and response 1 is a decoy, queued so a runner cannot pass by exhausting the queue instead of refusing the field. The decoy is never consumed. So an unconsumed decoy could satisfy the gate while the body that actually gets decoded drifted away from its control, which is the same vacuity the gate exists to prevent, one level up. Reachable, not theoretical. Drift the consumed body by a second field and make the decoy differ from its control by exactly one, and the two gates split cleanly: the old one reports `ok` at exit 0, the new one fails at exit 1 naming the case. Now restricted on both sides: the FIRST mock response only, and a control exercising the SAME operation -- a body that decodes into a different model says nothing about whether this one still decodes. * Move $comment out of the assertion properties map, and metaschema-check first Codex P2: the errorRaised annotation sat INSIDE properties.assertions.items.properties, so it declared a property literally named "$comment" whose schema was a string. Draft 2020-12 requires every value under `properties` to be a schema object or boolean, so conformance/schema.json was not itself a valid schema -- and tests.schema.json references it, meaning a validator that meta-validates would reject the whole conformance schema before looking at a single fixture. Moved alongside `properties`, where JSON Schema puts annotations. The reason this shipped is that nothing checked it: the fixture pass validates fixtures AGAINST the schema and never validates the schema itself, so an invalid schema sails through. conformance-fixtures-check now runs --check-metaschema over schema.json and tests.schema.json FIRST, because validating fixtures against a schema that is not a valid schema proves nothing. Red proof, through the make target rather than the bare validator: putting the annotation back inside properties fails at REAL_EXIT=2 with conformance/schema.json::$.properties.assertions.items.properties['$comment']: '...' is not of type 'object', 'boolean' * Require a kill case to deliver its malformed value in a 2xx response Codex P2, the residual hole in the control gate: it compared operation and body but not the response OUTCOME. Change an errorRaised case's first mock response from 200 to 500, or to networkError while keeping its body, and the SDK fails on the HTTP or transport error instead. errorRaised is satisfied by that failure, requestCount / requestMethod / requestPath all stay green, and the malformed field is never decoded -- the case goes green having tested nothing, and body equality cannot see it. A kill case's premise is that the malformed value arrived in a SUCCESSFUL API response. That is what makes it the SDK's problem rather than the server's, and it is why #576 classifies the refusal as a statusless api_error rather than a transport or HTTP failure. So require it: the first mock response must carry a 2xx status and no networkError. Both failure modes proved red, each naming what it would have cost: status 500 -> "...so the call fails on the HTTP error before the body is decoded" (REAL_EXIT=1) networkError -> "...so the call fails in transport and the body is never decoded" (REAL_EXIT=1) * Require the control response to reach its decoder too Codex P2, the symmetric half of the previous commit: not_a_success was applied to the kill response but not to the control. A control earns its keep only by being DECODED -- that is what makes it fail loudly (#555) on model drift, which is the entire protection the kill case borrows from it. A sibling answering 500 or networkError with an object body never reaches its decoder, so it can sit green on its own HTTP/transport assertions while the drift it was supposed to catch goes unnoticed in both bodies. Same check now, on both sides. Red proof needed a second attempt, which is worth recording. Breaking a single control (update-preserves-due-on -> 500) did NOT fail the gate: cards_write.json has four non-errorRaised UpdateCard cases, and the gate correctly fell back to update-explicit-clear, whose body also matches on the same key set differing only in due_on. That first proof was vacuous -- it demonstrated the fallback working, not the check. With all four UpdateCard controls answering 500 the two versions split cleanly: the pre-fix gate reports `ok` at REAL_EXIT=0, the post-fix gate fails all three Cards kill cases at REAL_EXIT=1, naming the missing SUCCESSFUL (2xx) control. * Reject a kill case whose 2xx never reaches a decoder, and self-test the gate The control-sibling gate accepted any 2xx on both sides, which let a 204 through. A 204 is short-circuited before any parse — TypeScript returns `undefined`, Kotlin returns `Unit` without calling `parse`, Go rewrites the body to JSON `null` — so a kill case answering 204 never decodes its malformed field. The composite fails because the record came back absent, `errorRaised`, `requestCount: 1`, `requestMethod` and `requestPath` all still hold, and the control, still a 200, stays green. The gate printed `ok` and exited 0 for exactly that input: the same false green this gate exists to prevent, one layer down. Statuses are now an allowlist, {200, 201}, rather than a 204 exclusion. Two constraints meet there: Go's success arm is exactly {200, 201, 204}, so 202, 203, 205 and 206 are never decoded there at all; and 204/205 carry no body by definition. Closed-by-default, because a gate whose whole job is to prove a body is decoded cannot prove that for a status nobody has reasoned about. `not_a_success` is renamed `not_decoded` — a 204 does not fail the call, it bypasses the decode, and the old name said the wrong thing. Every rejection this gate makes was, until now, correct by inspection alone, which is the standard that let #576 through five review passes. So it gets a self-test: `conformance/test_check_kill_case_controls.py` crafts one input per claimed rejection and asserts the gate refuses it, driven through the real entry point via a new optional FIXTURE_DIR argument, with the real fixture set run as a positive control. Reverting only the two new status branches turns exactly four cases red — 204, 205, an undecoded 2xx, and the control-side 204 — and nothing else, so the suite is measured non-vacuous rather than assumed so. It runs inside `make conformance-fixtures-check`, which CI already invokes. * Document errorRaised in SPEC.md §19's gated assertion table #590 landed `make doc-constants-check` on main after this branch was cut, and it gates SPEC.md §19's table against the `conformance/schema.json` assertion enum: a new type cannot ship undocumented. This branch adds `errorRaised` to that enum, so the rebase inherited the obligation and Spec Gates went red with "defines 22 assertion types, the table documents 21". The row says what the type is for and what declaring it costs — it switches the stop-on-mismatch policy off for that case, which is why every fixture declaring it needs a control sibling. The other finding in that same red run — SPEC.md §Documents restating the current pin — was #601's, not this branch's, and #605 has since fixed it on main. An earlier revision of this branch carried its own fix for it; that is dropped, so the only line this PR adds to SPEC.md is the one above.
jeremy
added a commit
that referenced
this pull request
Aug 4, 2026
Fourth review round on #642. Four findings, all upheld. The allowlist framing was wrong in the direction that matters. I wrote that fewer hook events are safe for an allowlist. True only if the allowlist named both operations: one that names UpdateCard and deliberately omits GetCard used to reject cards.update at its read, and after the collapse permits it end to end. Both policy shapes now carry the warning, labelled, plus the observation that they are the same hole seen twice — in each, the thing stopping the write was the read, expressed once as an omission and once as an entry. The class-A counting was inconsistent across all six SDKs, not the two flagged. Python and Kotlin excluded changes their own prose called "no signal whatsoever"; auditing every SDK against the definition moved the totals to 47 class A and 4 class B. The counting policy is now stated in the document so it can be checked against a rule rather than an impression: one entry per distinct change per SDK, counted where it bites; class A if any ordinary call-site shape stays silent even when another is compile-caught; second faces annotated as residue and counted once; raises-only-on-malformed-response is class B. Two things fell out that were not counting problems. Ruby's #563 was missing from the guide entirely — no mention of download_url anywhere in the chapter — verified against source rather than prose: v0.12.0 http.get_no_retry, which sent Accept: application/json and did not retry, became get_download calling request_with_retry with retry_on: DOWNLOAD_RETRY_ON and accept: nil. Ruby now has its own section. The same check confirmed Go's omission of #563 is correct, because Go already retried at v0.12.0. Separately, the Go note claiming the compiler catches only the pkg/generated half of Schedules().UpdateEntry was false: UpdateScheduleEntryRequest's fields became pointers, so any pkg/basecamp call site that set a field fails to build. The class-B definition described only half its own membership. It said the trigger is an absent field, but Ruby's entry fires only when the field is populated. It now says both, and says plainly that class B is a property of a call plus a response rather than of the call — the same method against the other shape is not a break at all. Class A has no such dependency. Stale counts in the chapter intros are fixed. The Go intro still said eleven silent and two panics, which is the first thing a #go link shows, and Swift claimed the most no-signal breaks, which stopped being true at Go ten. Also folds in #652 (projected-example gate, stacked on #648, takes check-targets to 43), moves #648 out of draft at cb438ce, and records that #647 is being reworked Smithy-first because the generated UpdateCardStepRequestContent.DueOn is *types.Date and cannot express "". The consumer-facing card shape is unaffected by that rework. Re-derived against #648: 238 -> 247 with 14 added, 5 removed and 11 same-ID route moves survives unchanged.
jeremy
added a commit
that referenced
this pull request
Aug 4, 2026
Fourth review round on #642. Four findings, all upheld. The allowlist framing was wrong in the direction that matters. I wrote that fewer hook events are safe for an allowlist. True only if the allowlist named both operations: one that names UpdateCard and deliberately omits GetCard used to reject cards.update at its read, and after the collapse permits it end to end. Both policy shapes now carry the warning, labelled, plus the observation that they are the same hole seen twice — in each, the thing stopping the write was the read, expressed once as an omission and once as an entry. The class-A counting was inconsistent across all six SDKs, not the two flagged. Python and Kotlin excluded changes their own prose called "no signal whatsoever"; auditing every SDK against the definition moved the totals to 47 class A and 4 class B. The counting policy is now stated in the document so it can be checked against a rule rather than an impression: one entry per distinct change per SDK, counted where it bites; class A if any ordinary call-site shape stays silent even when another is compile-caught; second faces annotated as residue and counted once; raises-only-on-malformed-response is class B. Two things fell out that were not counting problems. Ruby's #563 was missing from the guide entirely — no mention of download_url anywhere in the chapter — verified against source rather than prose: v0.12.0 http.get_no_retry, which sent Accept: application/json and did not retry, became get_download calling request_with_retry with retry_on: DOWNLOAD_RETRY_ON and accept: nil. Ruby now has its own section. The same check confirmed Go's omission of #563 is correct, because Go already retried at v0.12.0. Separately, the Go note claiming the compiler catches only the pkg/generated half of Schedules().UpdateEntry was false: UpdateScheduleEntryRequest's fields became pointers, so any pkg/basecamp call site that set a field fails to build. The class-B definition described only half its own membership. It said the trigger is an absent field, but Ruby's entry fires only when the field is populated. It now says both, and says plainly that class B is a property of a call plus a response rather than of the call — the same method against the other shape is not a break at all. Class A has no such dependency. Stale counts in the chapter intros are fixed. The Go intro still said eleven silent and two panics, which is the first thing a #go link shows, and Swift claimed the most no-signal breaks, which stopped being true at Go ten. Also folds in #652 (projected-example gate, stacked on #648, takes check-targets to 43), moves #648 out of draft at cb438ce, and records that #647 is being reworked Smithy-first because the generated UpdateCardStepRequestContent.DueOn is *types.Date and cannot express "". The consumer-facing card shape is unaffected by that rework. Re-derived against #648: 238 -> 247 with 14 added, 5 removed and 11 same-ID route moves survives unchanged.
jeremy
added a commit
that referenced
this pull request
Aug 4, 2026
* MIGRATING.md: the v0.13.0 upgrade guide, silent breaks first v0.13.0 breaks all six SDKs and 35 of those breaks are silent — no compile error, no exception, no decoder failure. Label-generated release notes list what merged; they cannot say what a consumer must react to or what wrong behaviour they get if they ignore it. That had no home in this repo. Adds MIGRATING.md at the root, linked from the root README and all six per-SDK READMEs. Silent breaks lead the document, then one section per SDK ordered by severity, plus an operator checklist, a "coverage: corrected and re-scoped" section for what did not ship, and known gaps. No CHANGELOG is reintroduced. The hand-maintained ones were deleted in #115 as superseded by auto-generated notes, and every release body since is machine-built. CONTRIBUTING records the resulting rule: label-generated notes say what merged, MIGRATING says what to do about it. Corrections to the source drafts, each re-derived rather than repeated: - TrashTodo was not a 404. bc3 draws `resources :todos, only: %i[show edit update destroy]`; DELETE /todos/:id returned 204 and set status to "archived", so every caller was archiving. It is the one #619 removal that takes away a working call, and it now carries its own carve-out. - #619 removed three operations, not nine. Nine were re-pathed. Fusing the two sets is what made the blanket 404 reassurance look safe. - Hook operation identity differs by SDK: Go and Ruby emit a short verb, the other four emit the wire operation ID, where the todolist pair kept its names — so an allowlist holding UpdateTodolistOrGroup passes the write and denies the new read. - 238 -> 241 measured at the v0.12.0 tag and at c95d81c, not assumed. - Kotlin binary compatibility is already disclaimed in kotlin/README.md; Swift has no written policy. Both are now stated rather than left unsaid. recordings.get is documented as a known gap with a list-and-filter recipe and its honest cost. The Go recipe compiles against this tree. #637, #629 and #635/#641 were open at the time of writing and are recorded under "Not in this release" rather than described as shipped. * Fix the Go pagination advice, cut the raw-wire workaround, absorb #637/#643 Addresses both P1 review threads on #642 and folds in the two PRs that landed since the first draft. Pagination (P1). Cross-SDK item 1 claimed `page` was a starting offset in every SDK and told readers to drop it to restore the old walk. For Go that was actively harmful: `git show v0.12.0:go/pkg/basecamp/bookmarks.go` returns before followPagination whenever page > 0, so a positive Page already meant one request, and dropping it converts a bounded call into a full account-wide traversal. The item is now scoped to the five SDKs where it holds — re-checked at the tag rather than assumed, since the universal claim had already failed once — with a Go subsection splitting the two real cases: services where the page number was already honored (Bookmarks, Drafts, Everything*, request unchanged) and the fourteen carrying the "not yet honored" doc, which sent no page at all and returned page 1's rows. Gauges is in neither; it had no page. Raw wire (P1). The Forwards().CreateReply example built a path with fmt.Sprintf and called the raw AccountClient.Post against a route with no upstream coverage, which is what AGENTS.md "Never Do These" 4 and 5 forbid. Removed rather than softened, and replaced with a known-gap section stating what a hand-built path gives up. Swept the document: the one other hit documents a real change to the raw client's error codes, so it stays, but its fabricated path is gone and it now says it is not a suggestion to reach for the escape hatch. #643 landed, so basecamp.Ptr and basecamp.Deref replace the hand-rolled ptr helper throughout, the Go section opens with the 300-pointer census and a command that reproduces it, and ParticipantIDs *[]int64 gets its own note: nil leaves participants alone, a pointer to an empty slice removes every one. #637 landed and does NOT add a break to any SDK. color and comments_app_url did not exist on Todolist at v0.12.0 in any of the six — both arrived with #628 earlier in this same release — so from the guide's baseline nothing turned from optional to required. Counts stay 27/20/16/14/16/14. Documented where it bites: color is required-and-nullable so explicit null decodes, comments_app_url rejects null and absence alike. Also: kotlin/README's append-only source-compat promise contradicted this release repeatedly, so it now describes documented pre-1.0 breaking correctness releases; the binary-compat disclaimer is kept and sharpened. release-github.yml links MIGRATING.md from every release body, guarded on the file, so the link cannot be forgotten at tag time. "Silent" is defined as source/runtime-silent against a live server, since a suite pinning request paths does catch some. Counts are stated as-of 51d0d86 with derivations inline, and each in-flight change names the numbers it invalidates so the pre-tag pass is arithmetic. * Split silent breaks into no-signal and fails-at-runtime; absorb #629 and cards Addresses the remaining P2 and a suppressed Copilot comment on #642, re-derives every count against main, and writes the cards due-date change. The P2 was right, and it was a contradiction with this guide's own definition rather than loose wording: "silent" was defined as "does not raise" and then used to file nil-pointer panics. The section is now "Breaks your compiler will not catch" — the property all of it actually shares — split into class A, no signal at all, and class B, compiles then panics or raises but only when a particular field is absent, so it passes every test where that field is populated. Applying the definition consistently moved four entries, not the three flagged: the three Go pointerization panics plus Ruby's Draft#scheduled_posting_at decode, which raises NoMethodError and TypeError and had the same defect. Two moved entries carry real no-signal residue, kept as sub-notes rather than double-counted. Per SDK: Go 8A/3B, Swift 9A, TypeScript 5A, Python 4A, Ruby 2A/1B, Kotlin 3A — 31 + 4 = 35, unchanged in total. Body counts verified against the table by parsing the section, not by eye. The Swift section claimed three new optional Todolist members and named one; the other two are required. Now singular, matching TypeScript. Counts re-derived at 9de44b2: the inventory is 238 -> 247, not 241, since #629 merged. Added, removed and route-moved lists are computed from openapi.json at both ends rather than hand-edited — 14 IDs added, 5 removed, 11 same-ID moves — and the Folders operations are flagged as drawn at /stacks, not /folders. Cards get their own section. The half that matters most is true in production today and is not caused by upgrading: every released SDK encodes "clear a card due date" as omission, bc3 stopped treating omission as a clear, so that call is a silent no-op right now. That is a reason to upgrade rather than a hazard of it, so it sits in the operator checklist. The SDK-side change is read from bf43715 and marked unmerged: single PUT, "due_on": "" as the clear encoding, UpdateStepRequest.DueOn becomes *string, and the GetCard preservation read goes away. The hook collapse is written as the inverse of the {Todolists,Update} split because it fails the opposite way — allowlists do not start denying, but a denylist on {Cards,Get} silently stops blocking the write it used to take down. Removing the preservation GET also removes three named errorRaised kill cases from cards_write.json; the class stays pinned on Todos, which still does a real read-modify-write, so that is said rather than filed as a redundant-GET cleanup. * Audit class A across all six SDKs; add Ruby's missing download retry Fourth review round on #642. Four findings, all upheld. The allowlist framing was wrong in the direction that matters. I wrote that fewer hook events are safe for an allowlist. True only if the allowlist named both operations: one that names UpdateCard and deliberately omits GetCard used to reject cards.update at its read, and after the collapse permits it end to end. Both policy shapes now carry the warning, labelled, plus the observation that they are the same hole seen twice — in each, the thing stopping the write was the read, expressed once as an omission and once as an entry. The class-A counting was inconsistent across all six SDKs, not the two flagged. Python and Kotlin excluded changes their own prose called "no signal whatsoever"; auditing every SDK against the definition moved the totals to 47 class A and 4 class B. The counting policy is now stated in the document so it can be checked against a rule rather than an impression: one entry per distinct change per SDK, counted where it bites; class A if any ordinary call-site shape stays silent even when another is compile-caught; second faces annotated as residue and counted once; raises-only-on-malformed-response is class B. Two things fell out that were not counting problems. Ruby's #563 was missing from the guide entirely — no mention of download_url anywhere in the chapter — verified against source rather than prose: v0.12.0 http.get_no_retry, which sent Accept: application/json and did not retry, became get_download calling request_with_retry with retry_on: DOWNLOAD_RETRY_ON and accept: nil. Ruby now has its own section. The same check confirmed Go's omission of #563 is correct, because Go already retried at v0.12.0. Separately, the Go note claiming the compiler catches only the pkg/generated half of Schedules().UpdateEntry was false: UpdateScheduleEntryRequest's fields became pointers, so any pkg/basecamp call site that set a field fails to build. The class-B definition described only half its own membership. It said the trigger is an absent field, but Ruby's entry fires only when the field is populated. It now says both, and says plainly that class B is a property of a call plus a response rather than of the call — the same method against the other shape is not a break at all. Class A has no such dependency. Stale counts in the chapter intros are fixed. The Go intro still said eleven silent and two panics, which is the first thing a #go link shows, and Swift claimed the most no-signal breaks, which stopped being true at Go ten. Also folds in #652 (projected-example gate, stacked on #648, takes check-targets to 43), moves #648 out of draft at cb438ce, and records that #647 is being reworked Smithy-first because the generated UpdateCardStepRequestContent.DueOn is *types.Date and cannot express "". The consumer-facing card shape is unaffected by that rework. Re-derived against #648: 238 -> 247 with 14 added, 5 removed and 11 same-ID route moves survives unchanged. * Correct four claims in the v0.13.0 guide that do not match the source The opening warning said the runtime failures need a payload where a field is absent. That holds for the three Go entries; Ruby's single class-B entry has the opposite trigger. Draft#scheduled_posting_at and MyNote#created_at/#updated_at run through parse_datetime, which returns nil for nil and a Time otherwise, so .start_with? and Time.parse raise only when the field is populated. A reader following the old text builds the wrong fixture and concludes they are unaffected. Both directions are now named, here and in the root README. Class A was described as breaking on every response. Most of it does, but two groups do not: the error-message and validation entries need an error status to reach the code at all, and the field-map half needs a body of a particular shape; downloadURL's hop-1 retry changes nothing until a network error or one of 429/502/503/504 occurs. Stated as preconditions rather than as a blanket claim. The Go pointer example said only the field selector panics. types.Date.String has a value receiver, so Go rewrites t.DueOn.String() to (*t.DueOn).String() and the nil dereference panics before String is entered. The same holds for IsZero, Before, After and Weekday on Date and for Format, Sub, Unix and Year on time.Time. The summary bullet already said both panic; the example contradicted it. The Accept-header note credited only Python. Ruby dropped it on the same hop: get_download passes accept: nil, and request_headers sets the header only when accept is truthy. Both are named, with the observation that the other four never sent it on that hop at v0.12.0 either. No counts are touched. * Re-derive every count against the final release commit Rebased onto 2afc977 and re-measured rather than incremented. Eight PRs merged since the branch was last updated, not the seven that carried the breaking label: #647 was on the "Not in this release" list and had landed. Counts. 55 class A and 6 class B, 61 surviving a clean build, up from 47/4/51. Per SDK the class split is Go 12/4, Swift 10/0, TypeScript 9/0, Python 8/0, Ruby 10/1, Kotlin 6/1, and the breaking-change column moves to 33/22/18/16/20/17. The body parses back to those numbers rather than agreeing with them by hand. The root README's aggregate sentence is re-derived to match, and now states both halves numerically instead of "most" and "a few". The operation inventory is unchanged at 238 -> 247 with the same 14 added, 5 removed and 11 same-ID route moves, computed from openapi.json at both ends. check-targets is 43, and the derivation is inline where the gate count was previously only projected. The release spans 67 merged PRs, 15 labelled breaking; the gh commands that produce both are embedded in the as-of block, with the note that a labelled PR is not the same unit as an entry, which is why the per-SDK columns exceed 15. #658 is class B, not class A. It does to five wrapper timestamps exactly what #615 did to five others: QuestionReminder.RemindAt, ClientApprovalResponse's CreatedAt and UpdatedAt, TimelineEvent.CreatedAt and WebhookDelivery.CreatedAt compile untouched through a value-receiver call and panic on nil. #615's own check could not see them because it keyed on the omitempty tag and these five did not carry one. The audit is ten fields, and the entry names the near-miss siblings that did not move, ClientApproval's pair in particular. #664 splits. The public CreateScheduleEntryRequest fields were already string and still are, so the wrapper half is silent: the RFC3339 ErrUsage guard is gone, a bare date now creates an all-day entry, and a malformed value reaches bc3 instead of failing locally. That is class A. The generated CreateScheduleEntryRequestContent went time.Time to string, which is a compile error for pkg/generated importers. ReplaceScheduleEntryRequestContent is not a migration from v0.12.0 at all; #632 introduced it. TypeScript and Ruby are doc-comment only. #647 is folded in as merged, with two corrections to what was written when it was still a branch. It touches no schema, so the claim that it had to go Smithy-first is withdrawn; UpdateCardStepRequestContent.DueOn was pointerized by #560. And the v0.12.0 preservation GET was conditional, taken only when the caller left due_on unaddressed, so the request-count table is scoped to that path rather than presented as universal. #648 adds no silent break anywhere. bc3's body is byte-identical before and after, so nothing that was populated stops being so; the assignable's title was never sent and is now spelled content. Every rename and retype is caught statically in Go, Swift, TypeScript and Kotlin and raised immediately in Python and Ruby, so it is one compile-or-runtime entry per SDK. Two corrections nobody asked for. The Go class list opened "Go carries every class-B break in the release", which stopped being true when Ruby's decode entry moved into class B; it now claims only the panic-shaped ones. And todos_write.json carries three errorRaised cases, not two, because #660 added a bare-scalar kill. #660 is a Kotlin class-B entry, which is new. Removing the client-wide isLenient means a present, populated, wrong-typed scalar throws SerializationException where it used to coerce to a string, and no signature moved to announce it. It throws in the response decode, so on a write the mutation has already landed, and it is not a BasecampException outside todolists. #656 is Ruby class A, scoped tightly: only max_retries 0, only an ungoverned GET, which means get_absolute and the Launchpad fetch rather than any operation lacking a policy. Every other configuration is bit-identical. Not in this release is now empty, and says so. * State the schedule-entry clear value per field instead of universally The Swift Behavioural bullet said an explicit "" clears any of the five full-state fields. Only description does. "" on summary is accepted and reads back "Untitled"; starts_at and ends_at are under validates_presence_of in Schedule::Entry, so "" is rejected rather than cleared; allDay is a boolean in every SDK, so "" does not typecheck at all. The carve-out half grouped notify with the three clearable fields even though it is a send directive with no state to clear. * Re-derive the per-SDK README banners against the final class A/B table The six SDK README banners still carried the counts from before the Go reclassification and the recount that followed it, summing to 51 where MIGRATING.md and the root README say 61. Each banner now matches its row in the class A/B table: Go 12+4, Swift 10, TypeScript 9, Python 8, Ruby 10+1, Kotlin 6+1. Kotlin also gains the runtime clause it was missing, since its one class B entry throws on a present field carrying a JSON number or boolean where the model declares a string. * Correct the merged-PR count and the two claims the reviewers caught The release spans 55 merged pull requests, not 67. The 67 came from comparing GitHub's Z-formatted mergedAt against a git timestamp formatted with a local offset, using jq's string >, which is lexicographic rather than temporal; it wrongly swept in twelve PRs merged in the hours before the v0.12.0 tag instant. The derivation embedded in the guide taught that same broken comparison, so it now uses %ct and fromdateiso8601 and says why. The breaking count of fifteen is unchanged, since all fifteen merged after the tag, so the class A/B split, the per-SDK tables and the six README banners are untouched. The header no longer calls 2afc977 the commit the release is cut from. That commit is the last of the release content and the baseline the counts were measured against, but it predates this guide; the tag is cut from main after this merges, on a tree that contains the file the release body links to. The release-body teaser claimed the guide covers only breaks with no exception and no decoder failure. The guide documents six breaks that do fail at runtime, including Ruby and Kotlin raises and a Kotlin decoder failure, so the teaser now names both the silent class and the runtime one.
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
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.
SPEC §14 prescribed a two-hop download that threw on the first error, while
conformance/tests/downloads.jsonasserted retry at hop 1 — so four of the five runners carried a skip for the same two fixtures and the spec was the thing that was wrong. This makes §14 honest and brings every SDK to it.The policy (§14, new "Hop-1 Retry
[conformance]" subsection):Retry-Afterhonored on 429.Attempt budgets, per SDK:
MaxRetriesas total attempts (hand-written client rejects < 1)max_retriesas total attempts, floored at onemax_retriesas total attempts, floored at one for downloads; the ungoverned GET path's zero-attempt behavior stays tracked in #532maxRetriesas total attempts, floored at one, gated onenableRetryenableRetry, one when false. No public numeric knob.enableRetry, one when false. No public numeric knob.DownloadURLis deliberately absent frombehavior-model.json, so every SDK passes this policy to its retry primitive directly rather than looking it up by operation.Per-SDK
TypeScript — lifted the retry loop out of
createRetryingFetchintoretry.tsasexecuteWithRetry(makeAttempt, config, emit, signal)with four seams: attempt begin/finalize/retrying emission, caller-owned attempt preparation (request rebuild + auth refresh, terminal-marked so auth faults keep their identity past retry classification), per-attempt fetch, and the signal-aware backoff sleep. The client's middleware path delegates to it unchanged;download.tsadopts it with the fixed policy.Python —
get_no_retrybecomesget_download, running through_request_with_retrywith an explicitretry_on. Sync and async transports change identically.Ruby —
get_no_retrybecomesget_download, running throughrequest_with_retrywith an explicitretry_on; the declared set is authoritative in both directions (the taxonomy neither widens nor vetoes it).Kotlin — hop 1 now runs in
downloadHop1, a directive-shaped loop mirroringBasecampHttpClient.requestWithRetry(#517): catch clauses only classify the attempt's outcome, and the retry side effects run outside any catch, so aCancellationExceptionfrom the backoff propagates raw and no phantom request events fire for an attempt that already ended. A throwing auth strategy surfaces raw through an internal tag, spending no retry budget.Swift —
performDownloadRequestgains the same #517 directive loop. Pinned by native XCTests only; the Swift conformance runner lands in #558.Go — already conformed; this is a pin, no behavior change. Its tests covered only three of five edges (503, 429-with-
Retry-After, network errors), so 502/504 rode along untested, the 500 carve-out was only implied by a 404 case, and nothing asserted a retried attempt still carriesAuthorization.Red proofs
Each SDK's new tests run against that SDK's pristine (pre-change) implementation, swapped in with
git show origin/main:<path>.TypeScript —
npx vitest run tests/download.test.tsagainst pristinedownload.ts:Python —
uv run pytest tests/test_download.pyagainst pristinedownload.py+_http.py+_async_http.py:Ruby —
bundle exec ruby -Itest -Ilib test/basecamp/download_test.rbagainst pristinehttp.rb+client.rb:Kotlin —
./gradlew :basecamp-sdk:jvmTest --tests "com.basecamp.sdk.DownloadTest" --rerun-tasksagainst pristineDownload.kt:Kotlin conformance, same pristine
Download.ktwithKOTLIN_SKIPSalready emptied:Swift —
swift test --filter DownloadTestsagainst pristineHTTPClient.swift+Download.swift:testDownloadURL_neverRetries500andtestDownloadURL_enableRetryFalseSendsExactlyOneAttemptpass against pristine by construction — pristine never retries. They pin the ceiling, not the floor.Go — the implementation is unchanged, so the pins are proved by mutating it instead. Three mutations, each restored:
Narrow the switch back to
{429, 503}:Widen it to the all-5xx taxonomy shape (
429 || >= 500):Authenticate only on attempt 1:
Hop-2 no-auth verification
The credential boundary is asserted at three levels, and every level checks both directions — present on hop 1, absent on hop 2:
downloads.json's 503 case now assertsheaderPresent: Authorizationat index 0, 1, and 2 (previously index 0 only, so a retry that dropped the token would have passed) andheaderAbsent: Authorizationat index -1. Both new cases carry the same pair.Authorizationheader on each hop-1 attempt and on the signed hop across a 503 → 302 → 200 flow, asserting["Bearer test-token", "Bearer test-token"]then absent. Go's is new in this PR; the others ship with their SDK commit. Proved load-bearing in Go by authenticating only on attempt 1 (above).fetchSignedDownload, a fresh unauthenticated client, a bareNet::HTTPGET, a Ktor request with no auth block, rawfetch), outside the retry loop entirely.Fixture and roster deltas
conformance/tests/downloads.json:Authorizationnow asserted on every hop-1 attempt, not just the first.{networkError: true}→ 302 → 200, 3 requests,delayBetweenRequests≥ 1000, auth on both hop-1 attempts and absent on hop 2.Both new cases are registered in SPEC Appendix D (§14, §7). The §19 category table needs no new row — they live in the already-listed
downloads.json.SPEC §19 zero-skip roster — all eight download-retry lines deleted (Python ×2, Ruby ×2, TypeScript ×2, Kotlin ×2), matching the runners:
SKIPSRUBY_SKIPSTS_SDK_SKIPSNumber)KOTLIN_SKIPSlink-headerbranch remains (architectural)goSDKSkipsNo unwaivered skips remain in any runner.
Guard repoint
scripts/check-retry-metadata-parity.py's token smoke watchedclient.tsandhttp.rbfor expressions the refactors relocated — TypeScript's loop moved toretry.ts, and Ruby's status gate becameretry_eligible?, where the declared set arrives from either the operation's metadata or an explicit set. The guard now watches where the behavior lives: aclient.tsrow for resolving the tuple and delegating, a newretry.tsrow for the loop, and a Ruby row requiring the metadata read and the membership test as separate tokens. Strength is unchanged — stubbingconfig.retryOn.includesinretry.tsordeclared.include?inhttp.rbstill reddens the check.Verification
make checkgreen end to end.okall packagesFollow-up
Draft PR #558 (Swift conformance runner) carries two temporary skips for exactly these fixtures — "DownloadURL retries on 503 at the auth'd first hop" and "DownloadURL honors Retry-After on 429 at the auth'd first hop" — which it flips to live once this merges. Not touched here; that flip belongs to #558's own landing checklist.
Review round
14 threads (Codex ×1, cubic ×13; Copilot errored out again). Four changed the code, four became issues or were declined on verified evidence, the rest are recorded below with reasoning.
Fixed
Accept: application/json(4710daa7c). Real, and a §14 violation — line 988 has always said hop 1 sets Authorization and User-Agent only. Not a regression, though:main'sget_no_retrywent through the same header builder, so the header rode on the single attempt too. Both transports now thread anacceptseam from the retry entry point to the header builder, defaulting to the JSON Accept so every other caller is untouched; the download hop passes none, and the 401-refresh replay carries it through. What remains on the wire is the HTTP library's own default (httpx*/*, Faraday nothing) — the tests pin hop 1 against the bare hop-2 client rather than pretending the SDK controls it. Red against the un-fixed transports in both languages.DownloadAuthFailureduplicatedAuthPhaseFailure(b1fd56787). Same shape, same job.AuthPhaseFailureis nowinternaland shared.b1fd56787).while transport.requests.isEmptynow gives up after five seconds with a named failure instead of hanging the suite.Filed, not fixed here
Retry-Afterignores the HTTP-date form and non-positive deltas. SPEC §6:421 specifies integer-> 0, then HTTP-date, then fall through. No SDK does step 2; TypeScript also skips the> 0guard, soRetry-After: 0gives a 0 ms delay. All pre-existing — the TypeScript block is a character-for-character lift ofclient.ts:1114-1121onmain. Fixing it means six parsers plus a fixture for the date form, and a decision on whether §6 step 2 survives._single_request, whichmain'sget_no_retryalready called. Definitional rather than mechanical — is re-authentication a retry? — so it wants a decision before a patch.Declined, with evidence
HttpClient's constructor installsSaveBodyunconditionally (verified injavapoutput), and the plugin's own deprecation text states "Request bodies are now saved in memory by default for all non-streaming responses".downloadHop1usesclient.request(url)— non-streaming — so the connection is released before it returns andresponse.cancel()would be a no-op. The shippingBasecampHttpClient.requestWithRetryhas the identical shape, so if the premise held it would be a live bug onmain.BasecampError.networkfrom the transport" (Swift). Deliberate, and identical toperformRequest's documented classification: the response-type guard raises it for a programming error, and a transport that pre-wraps failures asBasecampErrorhas opted into terminal handling. Diverging one loop from the other on a shared rule is worse than the gap.origin/main's files untouched. A seam has to exist in the source for the tests to compile, which makes "pristine" no longer pristine.delays[:-1]in the Python conformance runner. The change was an alignment, not a regression — Go and Ruby already gate on the first gap, andany(...)failed on a correct implementation because the download flow's final gap is the deliberately un-delayed redirect hop.[:-1]is a better rule, but applying it to one runner of five is how a regression gets caught in one language and tolerated in the rest.on_retry" pin, and hoistingDOWNLOAD_RETRY_ONout of the two Python transports. Both fair; both wrong to do in one SDK when five carry the same shape.make checkre-run green end to end after the round (REAL_EXIT=0): Gook, TypeScript 1051, Python 761, Ruby 1010 runs / 2283 assertions, Kotlin BUILD SUCCESSFUL, Swift 275 — conformance 131/132/153/122/133, zero failures.