fix(retry): converge every Retry-After parser on SPEC §6's table and surface the field in Kotlin and Swift - #857
Conversation
…ners the field error-mapping.json asserts the parsed Retry-After on the error a 503 POST surfaces (non-idempotent, so nothing sleeps), retry.json's non-retried 429 POST asserts it too, and all six runners map the retryAfter error field. SPEC's conflict note narrows to the Kotlin/Swift shapes that still lack a slot; MIGRATING carries the Swift enum change and the parser convergence. Refs #775, #799
…at the ceiling Refs #799
…ad asctime as UTC The arbitrary-precision int reached float() on the retry path and raised OverflowError above ~1.8e308; parsedate_to_datetime hands the zoneless asctime form back naive and the TypeError was swallowed into no delay. Refs #799
The bignum reached sleep on the retry path and raised RangeError. Refs #799
… converge the parser .api gains a sixth associated value populated at every api_error status, read through a retryAfterSeconds property that also covers .rateLimit. The parser rejects a sign and saturates at maxRetryAfterSeconds, whose nanosecond product fits UInt64, so the sleep keeps only a representability bound where it used to cap a server-directed wait at a day. Refs #775, #799
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
typescript
There was a problem hiding this comment.
🟡 Changes recommended
Python still throws for sufficiently wide valid digit strings, and Kotlin introduces an unaddressed JVM constructor compatibility break.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Converges Retry-After parsing and surfaced error metadata across all SDKs according to SPEC §6.
Changes:
- Rejects signed delays and saturates oversized values.
- Adds Retry-After fields to Kotlin and Swift errors.
- Extends conformance assertions and Go hook metadata.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
typescript/tests/retry-after.test.ts |
Tests strict parsing and saturation. |
typescript/src/errors.ts |
Implements the TypeScript parser ceiling. |
swift/Tests/BasecampTests/TransportNetworkErrorRetryTests.swift |
Updates .api construction. |
swift/Tests/BasecampTests/TodolistsServiceExtensionsTests.swift |
Updates .api patterns. |
swift/Tests/BasecampTests/SchedulesServiceExtensionsTests.swift |
Updates .api patterns. |
swift/Tests/BasecampTests/PaginationTests.swift |
Updates .api patterns. |
swift/Tests/BasecampTests/ErrorTests.swift |
Tests parsing and error fields. |
swift/Tests/BasecampTests/DownloadTests.swift |
Updates .api patterns. |
swift/Tests/BasecampTests/DocumentsServiceExtensionsTests.swift |
Updates .api patterns. |
swift/Tests/BasecampTests/DecodeIsolationTests.swift |
Updates .api patterns. |
swift/Sources/Basecamp/TodolistsServiceExtensions.swift |
Supplies the new associated value. |
swift/Sources/Basecamp/Services/BaseService.swift |
Supplies the new associated value. |
swift/Sources/Basecamp/SchedulesServiceExtensions.swift |
Supplies the new associated value. |
swift/Sources/Basecamp/HTTP/HTTPClient.swift |
Replaces the one-day sleep cap. |
swift/Sources/Basecamp/Download.swift |
Supplies the new associated value. |
swift/Sources/Basecamp/DocumentsServiceExtensions.swift |
Supplies the new associated value. |
swift/Sources/Basecamp/BasecampError.swift |
Adds Retry-After metadata and parsing. |
SPEC.md |
Records resolved SDK behavior. |
ruby/test/basecamp/http_extended_test.rb |
Tests Ruby parser convergence. |
ruby/lib/basecamp/http.rb |
Adds Ruby saturation and sign rejection. |
python/tests/test_errors.py |
Tests Python parsing behavior. |
python/src/basecamp/errors.py |
Adds Python saturation and UTC asctime handling. |
MIGRATING.md |
Documents API and parsing changes. |
kotlin/sdk/src/jvmTest/kotlin/com/basecamp/sdk/ApiConstructorSurfaceTest.kt |
Updates the JVM constructor assertion. |
kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/PaginationTest.kt |
Tests Kotlin parser convergence. |
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Pagination.kt |
Implements strict, saturating parsing. |
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/BasecampException.kt |
Adds Retry-After error metadata. |
kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt |
Exposes the conformance error field. |
go/pkg/basecamp/http.go |
Populates hook metadata at every status. |
conformance/tests/retry.json |
Asserts the non-retried 429 field. |
conformance/tests/error-mapping.json |
Adds the 503 error-field case. |
conformance/runner/typescript/runner.test.ts |
Reads the TypeScript error field. |
conformance/runner/swift/Sources/ConformanceRunner/Assertions.swift |
Reads the Swift error field. |
conformance/runner/ruby/runner.rb |
Reads the Ruby error field. |
conformance/runner/python/runner.py |
Reads the Python error field. |
conformance/runner/go/main.go |
Reads the Go error field. |
Review details
- Files reviewed: 36/36 changed files
- Comments generated: 7
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| retryAfterSeconds: Int? = null, | ||
| ) : this(message, httpStatus, hint, retryable, requestId, cause, decodeFailure = null, retryAfterSeconds = retryAfterSeconds) |
There was a problem hiding this comment.
Documented rather than bridged, in c88c61a's MIGRATING section. A second Java-selectable Api constructor is exactly what #751 removed and what ApiConstructorSurfaceTest exists to keep out — its shape pin moves to the seven-parameter descriptor in this PR on purpose. The PR carries breaking; MIGRATING now says a compiled caller needs a recompile and Java source passes a seventh null.
| // Carried at every status, as the error's field is (SPEC §6): the | ||
| // resilience hook decides for itself which statuses it acts on. | ||
| result.RetryAfter = parseRetryAfter(resp.Header.Get("Retry-After")) |
There was a problem hiding this comment.
Fixed in c88c61a: TestTransport_RequestResultCarriesRetryAfterAtEveryStatus drives a 404 with Retry-After: 17 through the real transport and reads it off OnRequestEnd's RequestResult.
| # 1*DIGIT has no upper bound and no digit string is malformed for its | ||
| # length. | ||
| if _DELAY_SECONDS.match(value): | ||
| seconds = int(value) |
There was a problem hiding this comment.
Fixed in c88c61a: leading zeros are stripped and the significant width compared before int() runs, and the grammar uses fullmatch. Test adds a 5000-digit value — past int()'s own digit limit — saturating, and 5000 zeros parsing as no delay.
| /** | ||
| * Seconds the response's `Retry-After` named, parsed per SPEC §6, at | ||
| * whatever status carried it (SPEC §6 "HTTP Status Mapping Algorithm"). | ||
| * Null when the header was absent, malformed or already past — and for | ||
| * the error shapes no response produced. | ||
| */ | ||
| open val retryAfterSeconds: Int? = null, |
There was a problem hiding this comment.
Fixed in c88c61a: the KDoc names RateLimit and Api as the carriers and says the other shapes have no slot, matching SPEC's conflict note.
| case api( | ||
| message: String, httpStatus: Int?, hint: String?, requestId: String?, | ||
| decodeFailure: (any Error & Sendable)?) | ||
| decodeFailure: (any Error & Sendable)?, retryAfterSeconds: Int?) |
There was a problem hiding this comment.
Fixed in c88c61a: the README switch gains the sixth placeholder.
| /// Seconds the response's `Retry-After` named, parsed per SPEC §6, at | ||
| /// whatever status carried it. Nil when the header was absent, malformed, | ||
| /// or already past — and for the error shapes no response produced. |
There was a problem hiding this comment.
Fixed in c88c61a: the doc names .rateLimit and .api as the carriers and says the other cases have no slot.
| * parser consumes and the surrounding whitespace `parseInt` already tolerated. | ||
| */ | ||
| const DELAY_SECONDS = /^[+-]?\d+$/; | ||
| const DELAY_SECONDS = /^\d+$/; |
There was a problem hiding this comment.
Fixed in c88c61a: the comment now describes unsigned ASCII digits and the caller's trim.
…x the docs Review follow-ups. Python compared the width after converting, and int() itself refuses a string past the interpreter's digit limit, so a 5000-digit header raised the very exception the branch exists to keep off the retry path. Go's transport gains a test for RequestResult.RetryAfter on a 404. The Kotlin and Swift retryAfterSeconds docs say which shapes carry it, the Swift README's .api pattern gains its sixth placeholder, TypeScript's DELAY_SECONDS comment describes the grammar it matches, and MIGRATING records the JVM descriptor change on Api's constructor.
![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: d399674df8
ℹ️ 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".
| if _DELAY_SECONDS.match(value): | ||
| seconds = int(value) | ||
| return seconds if seconds > 0 else None | ||
| except ValueError: | ||
| pass | ||
| return min(seconds, MAX_RETRY_AFTER_SECONDS) if seconds > 0 else None |
There was a problem hiding this comment.
On Python 3.11+, a valid all-digit Retry-After longer than the interpreter's default 4,300-digit conversion limit reaches int(value) and raises ValueError, so a response with such a header escapes the normal error/retry path instead of saturating at MAX_RETRY_AFTER_SECONDS. Compare the significant digit count to the ceiling before calling int, as the TypeScript, Kotlin, Swift, and Go implementations do.
Useful? React with 👍 / 👎.
| // arm, which would make it a retryable Api. | ||
| 507 -> LimitExceeded(msg, hint, requestId) | ||
| else -> Api(msg, httpStatus, hint, httpStatus in 500..599, requestId) | ||
| else -> Api(msg, httpStatus, hint, httpStatus in 500..599, requestId, retryAfterSeconds = retryAfterSeconds) |
There was a problem hiding this comment.
When a retryable response such as a 503 has attempts remaining, BasecampHttpClient.requestWithRetry parses the header and uses it for delayMs, but constructs the onRetry error as BasecampException.Api("HTTP $status", status) without the new argument. Consequently retryAfterSeconds is still null for the hook error even though the final mapped error now carries it; pass the parsed value into that construction so hooks observe the delay that governs the retry.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 007fec3: both Kotlin loops — requestWithRetry and the download hop — construct the onRetry Api with retryAfterSeconds = the parsed value.
| retryable: Boolean = httpStatus != null && httpStatus in 500..599, | ||
| requestId: String? = null, | ||
| cause: Throwable? = null, | ||
| ) : this(message, httpStatus, hint, retryable, requestId, cause, decodeFailure = null) | ||
| retryAfterSeconds: Int? = null, |
There was a problem hiding this comment.
Java callers cannot use Kotlin default arguments, so adding this seventh parameter removes the only Java-selectable six-argument BasecampException.Api constructor; any Java AuthStrategy that recompiles an existing new Api(message, status, hint, retryable, requestId, cause) call now fails. The updated reflection test confirms that only the seven-argument constructor remains, despite its comment claiming earlier positional calls still work, so retain a six-argument overload that delegates with retryAfterSeconds = null.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 007fec3, differently from the suggestion: the six-argument constructor is left exactly as #751 shipped it, and retryAfterSeconds arrives through a second overload whose seventh parameter has no default. That keeps the old JVM descriptor byte-identical and avoids the ambiguity two fully-defaulted overloads would give every short Kotlin call. ApiConstructorSurfaceTest now pins both shapes, and MIGRATING no longer records a Kotlin break.
| status. Kotlin and Swift carry it on their rate-limit and `api_error` shapes — every status a retry | ||
| loop reaches, and every 5xx — but not on the 401/403/404/400/422/507 shapes, whose classes have no | ||
| slot; `error-mapping.json` pins the `api_error` arm and `retry.json` the 429 arm.]` |
There was a problem hiding this comment.
This paragraph now says Kotlin and Swift carry the field, but later sections of the same normative SPEC still state that the parser ceiling/sign work is owed by five SDKs (lines 744–748), that retry_after remains absent in Kotlin and Swift and Swift still has the policy cap (lines 982–987), and that MAX_RETRY_AFTER_SECONDS is owed by the other five (lines 4067 and 4081). Those stale conflict and constants inventories directly contradict this commit's implementation and can cause future work to treat completed behavior as outstanding; update them as part of the convergence change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 007fec3: the parsing-table note, the representability conflict, the standing paragraph and the Appendix A row and sentence now describe the converged state — all seven parsers implement the table, no policy cap remains — leaving only the four cancellation-handle paths as owed.
…nto its hook errors, retire the stale SPEC inventories The retryAfterSeconds overload is a second constructor with the seventh parameter required, so #751's six-argument descriptor stays byte-identical for compiled and Java callers and no short call becomes ambiguous; the shape pin expects both. Both Kotlin retry loops hand onRetry an Api that carries the delay governing the sleep. SPEC's parsing-table note, representability conflict, standing paragraph and Appendix A row now describe the converged state instead of naming work this stack did.
|
Converged: CI is green on 007fec3 (39 checks) and the two review rounds — Copilot's seven findings and Codex's four — are each answered in-thread, ten fixed and one (the Kotlin constructor) resolved by a no-default overload that keeps #751's descriptor. Nothing new since the last push. Stacked on #855; ready for a human look once that merges. |
The per-SDK sweep that #855's spec-first PR left listed: every remaining parser converges on SPEC §6's Retry-After table, and the two SDKs whose error shapes had no slot for
retry_aftergain one. Stacked on #855 (spec/retry-after-contract); the diff againstmainincludes that PR until it merges.Parsers (#799)
All five remaining parsers now match the table Go already implemented in #855:
+5is not1*DIGIT; Ruby, Python, Kotlin and Swift honoured it as 5 becauseInteger(),int(),toIntOrNull()andInt(_:)all read a sign. TypeScript'sDELAY_SECONDSdrops its[+-]?.MAX_RETRY_AFTER_SECONDS(2,147,483,647) saturates there in both wire forms, whatever its width — TypeScript refused aboveMAX_SAFE_INTEGER, Kotlin aboveInt.MAX_VALUE, Swift aboveInt, and Python/Ruby parsed the bignum and then raised on the retry path (OverflowErrorfromfloat(),RangeErrorfromsleep). Width is compared on the digit string before any conversion, so nothing overflows on the way to the comparison. Each SDK names the constant (MAX_RETRY_AFTER_SECONDS,BasecampError.maxRetryAfterSeconds,Basecamp::Http::MAX_RETRY_AFTER_SECONDS).parsedate_to_datetimehands the zoneless asctime form back naive, and the subtraction'sTypeErrorwas swallowed into "no delay"; it is now read as UTC per RFC 7231.The error field at every status (#775)
.apigains a sixth associated value,retryAfterSeconds, populated byfromHTTPResponseat everyapi_errorstatus; a newretryAfterSecondsproperty covers it and.rateLimit. Source-breaking for anyone matching.api(...)— MIGRATING has the section, and the PR carriesbreaking. With the parser saturating at a ceiling whose nanosecond product fitsUInt64, the sleep drops its 86,400 s clamp — the one policy cap SPEC §7 named — and keeps only the representability bound.BasecampExceptiongains a base-classretryAfterSeconds, populated onApi(public constructor grows a trailing defaulted parameter;ApiConstructorSurfaceTest's shape pin moves with it) andRateLimit(now an override).RequestResult.RetryAfteris populated at every status rather than 429/503 only; the resilience hook's own 429/503 policy is unchanged.error-mapping.jsonpins the field on a 503api_error(a non-idempotent POST, so nothing sleeps), andretry.json's non-retried 429 POST now asserts it too. All six runners learn theretryAftererror field.Still open after this PR (on the card)
typescript/src/services/base.ts,typescript/src/download.ts,ruby/lib/basecamp/http.rb,python/src/basecamp/_http.py. Each is a caller-facing API decision (where the handle comes from), not a mechanical change.api_errorshapes, not on the 401/403/404/400/422/507 classes; SPEC's conflict note says so.parseRetryAfterdelegating to the generated copy.Verification
make check-equivalent per SDK on the branch (Go vet/lint/tests, TypeScript typecheck/lint/1733 tests, Ruby rubocop/1515 tests, Python ruff/mypy/tests, Swift 471 tests, Kotlin jvmTest + runner tests),doc-constants-check, and all six conformance runners.Refs #775
Refs #799
Summary by cubic
All six SDKs'
Retry-Afterparsers now follow SPEC §6's table, and Kotlin and Swift error shapes carry the parsedretry_afterat every status. Signed values like+5are no longer accepted as delays, and over-range values saturate at 2,147,483,647 seconds instead of being refused or raising on the retry path.Parsers
DELAY_SECONDSdrops its[+-]?; Ruby, Python, Kotlin, and Swift used stdlib parsers that read a sign.int(), which refuses strings past its own digit limit.TypeErrorused to be swallowed into "no delay".Error field
.apigains a sixth associated value,retryAfterSeconds— source-breaking for pattern matches;MIGRATING.mdhas the section.BasecampExceptiongains a base-classretryAfterSeconds;Api's six-argument constructor is unchanged, with a second overload takingretryAfterSecondsas a required seventh parameter — no recompile for Java callers.onRetryanApicarrying the delay that governs the sleep.RequestResult.RetryAfterat every status instead of 429/503 only; a new test pins it on a 404.UInt64.api_errorand a non-retried 429; all six runners learn theretryAfterfield.Written for commit 007fec3. Summary will update on new commits.