Repository navigation
fix(retry): converge every Retry-After parser on SPEC §6's table and surface the field in Kotlin and Swift - #857
Merged
Conversation
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
This was referenced Sep 10, 2026
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.
![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".
Member
Author
|
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. |
jeremy
added a commit
that referenced
this pull request
Sep 10, 2026
…time rust/ holds a two-member Cargo workspace: basecamp-sdk, the publishable crate, and generator, the in-repo emitter that reads openapi.json and behavior-model.json and writes basecamp-sdk/src/generated/ (types, routes, metadata, accessors and one module per service). names.toml carries the TAG_TO_SERVICE, SERVICE_SPLITS, METHOD_NAME_OVERRIDES and RESOURCE_TYPE_OVERRIDES tables the other generators transcribe by hand. The generator reads the OpenAPI 3.1 shapes the model uses — ["T","null"] unions, anyOf nullable references, x-go-type hints for dates, flexible integers and times — and every x-basecamp-* extension. Response models are #[non_exhaustive]; request bodies and params structs are plain structs with Default. metadata.rs emits one labelled struct literal per operation so the repository's regex-based parity readers can consume the retry tuple and the idempotent set. The hand-written runtime carries the SPEC §6 error record, the §7 three-gate retry loop over the generated metadata with log-domain saturating backoff and the post-#855/#857 Retry-After parser, §8 Link pagination with cross-origin refusal, §9 HTTPS enforcement and redaction, §12 hooks, §13 transport behind an HttpClient trait with the shipped reqwest client, and the §14 two-hop download.
jeremy
added a commit
that referenced
this pull request
Sep 10, 2026
…time rust/ holds a two-member Cargo workspace: basecamp-sdk, the publishable crate, and generator, the in-repo emitter that reads openapi.json and behavior-model.json and writes basecamp-sdk/src/generated/ (types, routes, metadata, accessors and one module per service). names.toml carries the TAG_TO_SERVICE, SERVICE_SPLITS, METHOD_NAME_OVERRIDES and RESOURCE_TYPE_OVERRIDES tables the other generators transcribe by hand. The generator reads the OpenAPI 3.1 shapes the model uses — ["T","null"] unions, anyOf nullable references, x-go-type hints for dates, flexible integers and times — and every x-basecamp-* extension. Response models are #[non_exhaustive]; request bodies and params structs are plain structs with Default. metadata.rs emits one labelled struct literal per operation so the repository's regex-based parity readers can consume the retry tuple and the idempotent set. The hand-written runtime carries the SPEC §6 error record, the §7 three-gate retry loop over the generated metadata with log-domain saturating backoff and the post-#855/#857 Retry-After parser, §8 Link pagination with cross-origin refusal, §9 HTTPS enforcement and redaction, §12 hooks, §13 transport behind an HttpClient trait with the shipped reqwest client, and the §14 two-hop download.
jeremy
added this pull request to stack #860
September 10, 2026 09:32
monorkin
pushed a commit
that referenced
this pull request
Sep 10, 2026
* Add the Rust SDK workspace: generator, generated surface and core runtime rust/ holds a two-member Cargo workspace: basecamp-sdk, the publishable crate, and generator, the in-repo emitter that reads openapi.json and behavior-model.json and writes basecamp-sdk/src/generated/ (types, routes, metadata, accessors and one module per service). names.toml carries the TAG_TO_SERVICE, SERVICE_SPLITS, METHOD_NAME_OVERRIDES and RESOURCE_TYPE_OVERRIDES tables the other generators transcribe by hand. The generator reads the OpenAPI 3.1 shapes the model uses — ["T","null"] unions, anyOf nullable references, x-go-type hints for dates, flexible integers and times — and every x-basecamp-* extension. Response models are #[non_exhaustive]; request bodies and params structs are plain structs with Default. metadata.rs emits one labelled struct literal per operation so the repository's regex-based parity readers can consume the retry tuple and the idempotent set. The hand-written runtime carries the SPEC §6 error record, the §7 three-gate retry loop over the generated metadata with log-domain saturating backoff and the post-#855/#857 Retry-After parser, §8 Link pagination with cross-origin refusal, §9 HTTPS enforcement and redaction, §12 hooks, §13 transport behind an HttpClient trait with the shipped reqwest client, and the §14 two-hop download. * Format the generator's naming test * Wire Rust into the release, CI, security and versioning surfaces release-rust.yml follows release-python.yml's shape: test, then a credential-free package job that pins SOURCE_DATE_EPOCH, asserts the crate's .cargo_vcs_info.json names this SHA and rust/basecamp-sdk, and uploads the .crate as evidence; then a publish job on push only, in the release-crates environment, that repackages and requires byte equality with that evidence, exchanges OIDC for a temporary crates.io token, and publishes only on a 404 for the version. Any other registry answer fails closed; a yanked 200 fails; a 200 re-run succeeds idempotently after comparing checksums; a crate that has never been bootstrapped rehearses with --dry-run so the release-github roster stays green until the manual first publish trusted publishing requires. test.yml gains an MSRV lane (library on exactly 1.88, locked, plus an advisory fresh-resolution build) and a stable lane that runs make rs-check, the conformance runner and its unit tests, and uploads the execution manifest. security.yml gains Trivy over Cargo.lock and a weekly cargo deny over both workspaces; CodeQL analyses rust with build-mode none and the SARIF filter strips the generated tree; labeler, dependabot (both workspaces), .mise.toml, .editorconfig and .gitignore learn rust. bump-version.sh writes the workspace version bounded to the [workspace.package] table and refreshes both Cargo.lock files; the release guard reads the version back through cargo metadata rather than a regex, checks both lockfiles with --locked, and runs cargo publish --dry-run; sync-api-version covers the generated API_VERSION constant; assert-lockfiles-unchanged hashes Cargo.lock and prunes target/. * Document the Rust SDK across the repo docs README gains the Rust language and feature-matrix rows, a Quick Start, the crates.io/docs.rs links and the environment-variable rows; SPEC adds Rust to every per-language table and roster it hand-enumerates (retry gate consumption, page selection, headers, integer width, truncation unit, hooks, OAuth applicability, the section 21 gate table, the section 23 options column, Appendix A constants and the Appendix F divergence tables, where the deferred event-feed connector is recorded); MIGRATING notes the new SDK; CONTRIBUTING carries the prerequisites, build block, generator entry and the one-time crates.io trusted-publishing bootstrap; SECURITY and AGENTS gain their Rust rows and the workflow and version file counts move to eight and eleven. * Add the Rust conformance runner A scripted HttpClient rather than a socket mock, so the SDK is driven through its own transport seam: the two origin-normalization cases the Go runner cannot dial run here, a networkError response is a transport failure after the request is recorded, and the download hops see the redirect the SDK is meant to handle itself. Serving semantics are the Go runner's: an over-run queue answers an empty page when the case auto-paginates and a 500 otherwise, a single-array-key success body is unwrapped to the wire shape, and {{httpdate+Ns}} header values resolve at serve time. The 22 assertion types keep the Go runner's index semantics (negative from the end, past-end fails, never vacuous), errorRaised stays code-agnostic, and the link-header tag suppresses only requestCount. Dispatch is one explicit arm per fixture operation; the composite and download arms are placeholders until the runtime lands. The runner takes the case census up front and writes conformance/manifests/rust.json even on failure. * Register the Rust runner and renderings in every cross-SDK gate One commit, so no gate ever sees a partial roster: the seventh runner joins EXPECTED_RUNNERS, the zero-skip roster (Rust skips nothing — its scripted transport runs the two origin-normalization cases Go cannot dial, and the link-header case runs with only its requestCount suppressed), the rendered SPEC section 19 block, the conformance and runner-test aggregates, the manifests-reset order-only edge, check-targets and the CI fan-in; the idempotency, retry-metadata, operation-assignment, service-inventory and deprecation gates each gain a Rust reader keyed on what the generator actually emits (metadata.rs literals, the doc line naming method and path, the accessor impl); the README env-var gate gains a Rust lexer with char-literal and raw-string rules; the runner test reachability gate gains a cargo test arm. Every gate's self-test grows the matching Rust cases, and the fixture-execution and doc-constants self-tests build seven-runner fixtures. * Wire the DownloadURL arm and pinned-page reads in the Rust runner * Add the compiled Rust examples, the README env-var table and the runner lockfile One example per README section: the first call, pagination a page at a time and collected under a cap, error handling over the structured Error, and a scripted HttpClient with Hooks that runs without a network. rs-test builds them, so a README snippet that stops compiling fails the gate. The env-var section becomes the table the README gate reads; the conformance runner's Cargo.lock is tracked like every other runner's. * Import the params type from its service module in the examples * Carry the Rust crate to 0.18.0 with main * Keep the Rust migration note under Unreleased after the 0.18.0 bump * License the conformance runner and allow its path dep under cargo deny * Address the adversarial review of the Rust integration A tag pushed before the crates.io bootstrap now fails the publish job instead of rehearsing green: a GitHub release advertising a crate version nobody can install is the state the release roster exists to prevent, and CONTRIBUTING says so. The release waiter selects push runs only, so a dispatch rehearsal on the same tag cannot stand in for the publish; the post-publish check refuses a yanked version like the pre-publish one; every registry call is bounded; cargo deny runs --locked everywhere and the release test job and CodeQL keep the lockfile guard; Trivy scans the runner workspace too; the publish job is bound to this repository; the packaging comment describes what cargo actually guarantees. bump-version.sh reads the crate version back through cargo metadata so a pattern that matched nothing cannot announce a bump. The runner reports an unknown or unwired operation as a harness failure rather than an SDK error an errorRaised assertion could accept, compares request bodies by canonical JSON like the Go runner, pins only a positive page and forwards it on ListTodos. The reachability gate refuses a src/ test file no mod declaration reaches; the README env-var gate follows use-imported std::env names and raw-string arguments. Root README quick start and the SPEC page-selection row now describe the crate's real API. * Add generator correctness fixtures, wire-level tests and the integration checklist The generator is exercised against a small model carrying every shape it has to read — nullable unions, anyOf references, flexible integers and times, sensitive and auth-routable strings, a deprecated component and parameter, a string enum, a multipart body, write semantics and pagination — with the rendered files compared against golden output, and the models it must refuse (an unsupported response representation, a method-name collision, a keyword method name, an operation absent from the behavior model) asserted to fail. The wire-level tests reproduce the bodies of the error-mapping, retry, pagination and download conformance cases against wiremock and a scripted transport, plus the 401 refresh budget gate and coalescing, the operation deadline, hook ordering, the public-API bounds and the route table's counts. rust/INTEGRATION.md lists every shared-file hunk the integration owner lands: Makefile targets, the drift script, the five parity readers, sync-api-version and bump-version, CI, and the doc rows. * Complete the Rust runtime: OAuth, the §18 composites, examples and the test suite The oauth module carries SPEC §16 end to end: PKCE and state, resource-first discovery with the origin-root profile and issuer binding, the authorization-code exchange and refresh with the RFC 8707 resource echo, the RFC 8628 device grant with login_hint and the full poll table on an injectable clock, the token-endpoint transport policy, and a refreshing token provider the client's coalesced 401 replay drives. Its tests are driven by the conformance/oauth and conformance/oauth-token fixture directories. The composites are the six SPEC §18 merge-safe surfaces — Todos, Todolists, Documents and Schedule entries update/edit, Cards update with the tri-state due_on, Uploads download — over the generated wire methods only, with every case of their six conformance fixtures reproduced against wiremock. Required members decode strictly: an absent key is a malformed body, never a zero value, per SPEC §10. Pagination gains impl Stream adapters over pages and items. The accessor block is rustfmt-skipped so its lines stay one per service for the inventory parity reader, and one generated call per service reaches the wire under test. * Wire the composite and download arms: the Rust runner executes every mock case With the SPEC section 18 composites in the crate, the ten remaining dispatch arms drive them through the same presence-bearing fixture reads the Go runner uses: update requests carry Option members with DateChange for the tri-state date, and the edit closures assign only the keys the fixture names, the schedule carve-outs through their setters so assignment rather than value marks them addressed. 228 of 228 non-live cases pass, nothing skipped, so the roster's empty RUST_SKIPS is now a fact the census checks. * Spell the Rust PKCE helpers as the crate exports them * Pin rust-cache to v2.9.2's commit, not its tag object * Test the library alone without default features; read raw-fenced env names * Name the copyright holder as 37signals LLC * Address the first Copilot round on the crate An unparseable download URL is reported through SPEC section 9's projection (the origin, or the fixed unparsable token) rather than verbatim, since the input may itself be a signed URL; the fragment is no longer copied onto the request URL, which http::Uri refuses; Config::base_delay, which nothing read, is gone — the backoff base is each operation's metadata; FlexInt accepts both i32 endpoints from the float branch; and a non-idempotent POST's generated doc no longer quotes a retry budget that Gate 2 makes inert. * Close the adversarial review's findings on the runtime Following a Link on a route with path parameters no longer refills the route's path; the continuation carries the route's identity and the absolute target only. A Link whose target is not a URL is an error at the follow-up rather than the end of the collection. The page stream yields the page in hand before it asks for the next one. The shipped reqwest client turns off reqwest's own retries and its default headers, so one SDK attempt is one request and a download's second hop goes out bare. Downloads run under the operation deadline, replay a 401 once behind the budget gate, and never render more than an origin of a URL that may carry a signature. A failed refresh is shared with every request that was authenticated under the same credentials, a refresh error surfaces as auth_required, and the generation is read after the credentials it stamps. Decoding a body happens inside the operation hook boundary. The Retry-After date form rounds any positive remainder up, a server message that happens to start with the fallback phrase is still composed with its field errors, the exponential backoff crosses the ceiling exactly, a non-numeric flexible id reads as the zero sentinel the other SDKs use, and an invalid webhook signature never panics. * Accept explicit input paths in the generator and add a device-login example * Answer the first review round on the runtime Config loses its unread base_delay: the backoff curve is each operation's own, from the model. A download URL's fragment never reaches the wire, the flexible dimension reader admits both i32 endpoints, and a generated method's wire line no longer promises a retry budget to an operation that is sent exactly once. * Close the adversarial review of the OAuth module and the composites A refresh attempt's verdict is shared with the requests authenticated before it and no further: the generation now moves on every completed attempt, so a request made after the issuer recovers may refresh again. The generation is read before authenticating, so a stale stamp shares a verdict rather than refreshing twice. Transport timeouts keep their classification through the shipped client, so the device poll backs off from one instead of ending the flow. The refresh and display callbacks run guarded and outside the token lock. Every 3xx from a token endpoint is a generic failure before its body is read. Every error message is bounded where the error is built. The shipped transport and the OAuth client construct fallibly rather than through a panicking Default. A download's first hop reports exactly one request-end per attempt, with the failure on it for every non-success status. The page stream yields a page before failing on its unresolvable cursor. The OAuth-only feature set builds and tests, and the Makefile's matrix runs it. * Cap error hints by byte on a char boundary; close the OAuth response types The message cap is 500 bytes as SPEC section 9 states for Rust, cut on a UTF-8 boundary, and a hint goes through the same cap since a transport's rendering of its own failure is not bounded by anyone else. Token, DeviceAuthorization, ServerMetadata and ProtectedResourceMetadata are read-only response models and DeviceFlowReason, FallbackReason and SelectionFailure are open enumerations, so they take non_exhaustive before the first publish, as lib.rs's policy says they must. * Close the review's correctness findings on the runtime and the release workflow A 401 on a non-idempotent POST now refreshes and replays under the caller's attempt budget rather than the transient-retry ceiling (SPEC §4); a refresh a deadline cuts short is finished by the next request instead of restarted; the operation deadline is applied at each await so a cut-short request still closes its hooks; a per-attempt timeout is terminal in both loops (SPEC §14) while a connection that breaks mid-body is retried like one that never answered; Retry-After accepts HTTP-dates only. The base URL's path prefix is kept in front of the account id, which is sent as one path segment; a follow-on page reports the first page's hook identity; a lone projectId names no resource; a pinned page is never followed and reports its cursor as truncated; the item stream ends with an error when the page cap cuts it short; a wrapped collection (GetPersonProgress) gathers through the generated PageItems impl, and the cursor-preserving Page::map is gone. Downloads name the file after the URL given, not the signed one. OAuth token errors carry Retry-After and X-Request-Id as fields, and every 3xx is classified off the status line. release-rust.yml's crates.io state check quoted its User-Agent by word splitting, which failed every tag publish; the command is an array now. * Settle the pre-publish surface and the runtime's remaining robustness findings Config is literal-constructible like the request structs; the route table's records are non_exhaustive so the model can grow them; chrono and reqwest are re-exported at the versions the crate was built against, with their minor bumps named as ours; the serde helpers for flexible integers are crate-private; next_page's future is Send whatever the page holds, and the guarantees suite pins that for the streams, a composite and the OAuth flows. A request authenticating during a token refresh no longer waits behind the token endpoint's round trip; the bearer header is marked sensitive and a Response's Debug form redacts credential-bearing headers; a request id is bounded like the message. unwrap and expect are denied workspace-wide (tests, examples and the generator opt out at file level), the generator takes the workspace lints, the semver check compares against the PR's base commit, the fixture-execution self-test covers every runner again, and the publish dry-run tolerates a dirty tree in the develop loop. * Tidy the docs, examples and gates the review named, and harden the generator's refusals The examples and README read BASECAMP_ACCOUNT_ID like every other language's quick start, the README's first call is a whole program, its links are intra-doc, and the SPEC rows for Rust say what the crate does: null status on a network error, retry_after carried, the body cap in config.rs, ListResult for the collected read. The construction policy names the shared-shape exception; the retry-after ceiling cites the row that sanctions it. DeviceFlowError carries the transport failure that ended a flow, advertised issuers are deduplicated in a set, the runner defaults a body field only when the fixture omits it, the download Retry-After test asserts the exact wait, the schedule round-trip test asserts the bounds on the wire, and every service has a rejection case beside its happy one. The generator refuses what it cannot spell instead of guessing: a union of two types, oneOf/allOf, nullable array items, 2xx responses that disagree, and a path parameter without a placeholder or a placeholder without a parameter; arguments follow the template's order. rs-generate is rs-generate-services like its siblings, and the runner workspace finds the shared deny.toml on its own. * Close the follow-up review of the deadline, the 401 replay and the download hop The deadline now bounds every wait and every failure-body read: a 401 whose body stalls after a declined refresh, a retry sleep a slow hook admitted, and an error body on either loop all end at the deadline rather than after it, and a bound the clock cannot represent is no bound. The 401 replay draws on the operation's own ceiling where the route has one, and on the caller's cap only where the gates make the route single-attempt. A download's direct answer is read inside the attempt so a body that breaks is retried, and a refresh that fails or times out on hop 1 is classified as it is on the API path. collect_all validates a successor only when it is about to fetch it, and a pinned page under an item cap still reports the cursor it did not follow. The PageItems impl is emitted once beside its envelope, two operations that disagree on the envelope's collection are refused, as is an operation with no tag and no split entry, and an operation with no 2xx response is the error it always meant to be. * Prove the hop-one refresh verdicts and the generator's no-2xx refusal A download's first hop replays a 401 once the provider renews the credentials, and a provider that fails to renew them answers auth_required with that failure chained and the 401 on it — the classification d633c85 gave the path, now pinned. The generator's fixture also proves an operation with no 2xx response is refused by name rather than crashing. * Keep line breaks out of multipart headers, end a direct download's attempt after its body, resolve a cursor only past the caps A multipart part's content type is written without CR or LF, as its filename already was. A direct download's request-end hook fires after the body is read, so an attempt whose body breaks is reported as the failure it was before the retry that follows it. collect_all resolves the next cursor only once it is past the item cap, so a page the cap ends the walk on may name a target that does not parse. The conformance runner's rejected-origin assertion requires the operation to have failed, not merely to have sent one request. * Date a token from the server's answer, and keep the timeout marker on an OAuth transport failure A token's lifetime now runs from the moment the token endpoint answered, not from when its body finished draining, on the exchange, refresh and device paths alike; and an OAuth request that timed out answers is_timeout() as the transport reported it, with the origin-only projection kept.
jeremy
force-pushed
the
fix/retry-after-sweep
branch
from
September 10, 2026 19:26
007fec3 to
8352f91
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8352f919cb
ℹ️ 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".
jeremy
force-pushed
the
fix/retry-after-sweep
branch
from
September 10, 2026 20:03
8352f91 to
67dadd4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67dadd442e
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f03e0c10ba
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9d638e72b
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2369482a8
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02e331ba02
ℹ️ 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".
jeremy
force-pushed
the
fix/retry-after-sweep
branch
from
September 10, 2026 21:46
02e331b to
1ee98b0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ee98b09f0
ℹ️ 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".
jeremy
force-pushed
the
fix/retry-after-sweep
branch
2 times, most recently
from
September 11, 2026 23:11
753f399 to
1a8f4bc
Compare
jeremy
added a commit
that referenced
this pull request
Sep 14, 2026
…ntory the download sites outside it §6's CONFLICT block said five SDKs populate retry_after at every status. Read against every place an SDK builds an error by hand, that was a claim about constructor sites the shared mapper never sees, and it kept coming up false one site at a time: Ruby's public mapper, then Ruby's download hops, now Go's hop-2 errors and hop-1 500, Python's download errors on both paths, and TypeScript's hop-2 errors — each building its api_error without the header. The claim now binds where it can hold: at the status mapping algorithm the retry loops and operation paths return through. A status mapped by hand elsewhere carries the value only where that site parses the header itself; Ruby's hops do, the Go, Python and TypeScript download sites do not, and the block names them as owed by the sweep (#857), where routing them through the mapper or parsing at the site is the change. A future hand-built error is no longer claimed covered by a sentence that cannot see it.
…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
…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.
…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.
The Rust conformance runner arrived on main after this branch taught the other six runners the field, so the new 503 case in error-mapping.json failed there alone with an unknown error field.
parsedate_to_datetime is RFC 5322's parser and hands back a naive datetime for more than asctime — a bare `1 Jan 2099 00:00:00`, an IMF-fixdate whose zone it does not know, a `-0000` zone. Reading every naive result as UTC turned those, none of them an HTTP-date, into a saturated delay where the aware-minus-naive TypeError used to drop them onto the backoff curve. The zone is now assigned only when the value has the asctime shape.
Both retry loops parsed the header in calculateDelay for the sleep and let fromHTTPResponse parse it again for the error onRetry receives, so an HTTP-date crossing a whole-second boundary between the two could report one second less than the loop waits, or nil near expiry. Each loop parses once and hands the value to both; the four-argument fromHTTPResponse keeps parsing for callers that hold no value.
…fter on a redirect with no Location parsedate_to_datetime is RFC 5322's parser and reads far more than RFC 7231's three forms — an aware value with a numeric zone as readily as a naive bare date — so gating naive results alone still let `Thu, 31 Dec 2099 23:59:59 +0000` saturate the delay. The three shapes are matched before the parser sees the value; anything else is not a date and falls through to the backoff curve. The two future/past tests format their dates with usegmt, since a +0000 zone was never an IMF-fixdate. Swift's download hop 1 built the no-Location redirect error with retryAfterSeconds nil though the response is the one whose header the every-status contract carries; it parses the header now.
…tion, and name Kotlin's constructor-reference break The transport test reached its 404 through the raw client.Get with a hand-written path, the shape the repository's hard rules keep out of SDK code; it goes through Projects.Get now. And the Kotlin migration note promised no break where an untyped constructor reference to Api becomes ambiguous between the six- and seven-argument overloads; it says so, and what to do about it.
DateFormatter's zzz reads any zone name it knows, so a far-future date in PST parsed as a date and saturated the delay, where SPEC §6's table says a value that is not an HTTP-date is not a date at all. The value must match IMF-fixdate before the formatter sees it, and the formatter reads GMT literally; RFC 850 and asctime stay outside the MAY here, as in Ruby and TypeScript.
The same hop-1 site Swift fixed in c236948: the no-Location redirect error was built through the six-argument constructor, which fixes retryAfterSeconds to null, though the response is the one whose header the every-status contract carries. It goes through the overload with the parsed header now.
jeremy
force-pushed
the
fix/retry-after-sweep
branch
from
September 14, 2026 02:28
1a8f4bc to
bab63c7
Compare
jeremy
added a commit
that referenced
this pull request
Sep 14, 2026
… status, and give conformance a clock (#855) * spec(retry): pin the Retry-After contract and give conformance a clock SPEC §6 now states the whole Retry-After contract in one place: the parsing table (delay-seconds 1*DIGIT only, one MAX_RETRY_AFTER_SECONDS ceiling saturating in the parser, HTTP-date rounded up, IMF-fixdate required and the obsolete forms permitted), the field populated at every status the header parses at, and the honouring rule the SDKs converge on. The two-tier width rule it replaces made the honoured wait depend on which integer type a stdlib parse happened to use. Fixtures pin what the prose says: a 503 carrying Retry-After is waited, hop-1 502/503/504 are waited, and the positive HTTP-date branch is finally falsifiable through a {{httpdate+Ns}} header token every runner resolves at serve time (Ruby moves its header merge into the serve block for it). Each runner's resolver is unit-tested against a frozen instant. Refs #775, #799, #780 * fix(go): honour Retry-After at every retried status, in both clients The generated loop gated on 429 and parsed with a bare Atoi: no HTTP-date form, an unchecked range error, and a seconds*time.Second product that wrapped negative for the largest int64, so a typed operation burned its attempt budget back to back against an origin that asked it to wait. The template now carries its own SPEC §6 parser (digits-only, saturating at the shared ceiling, HTTP-date rounded up), waits a server-directed delay exactly instead of adding jitter to it, and checkResponse reads the same parser. The hand-written loop's 5xx arms now carry RetryAfter, which is what lets the header govern a 503's wait there, and the download hop parses it at every status in its set. The hand-written parser drops its word-size tier: a digit string over the ceiling saturates whatever its width. Fixes #798. Refs #775, #799 * fix(typescript): honour Retry-After at every retried status Both retry loops read the header behind a status === 429 ternary, so a 503 carrying Retry-After: 120 backed off ~1s. The header now governs the wait at every status in the declared set, and errorFromParsedBody carries retryAfter at every status rather than only on the rate-limit arm. Refs #775 * fix(ruby): honour Retry-After at every retried status, round dates up handle_error attached the parsed header only to RateLimitError, so calculate_delay never saw a 503's value. Every arm now carries it, and the HTTP-date branch rounds a sub-second remainder up per SPEC §6 step 2 instead of truncating — which retried up to a second early and read a remainder under a second as no delay at all. Refs #775, #799 * fix(python): round a Retry-After HTTP-date remainder up SPEC §6 step 2: truncation retried up to a second before the moment the server named and turned a sub-second remainder into 0, which the loop reads as no usable value. The parser takes a now seam so the boundary is pinned against a frozen clock rather than a flaky literal. Refs #799 * fix(kotlin): honour Retry-After at every retried status Both loops picked between the header and the backoff on status == 429, so a 503 carrying Retry-After backed off ~1s. The header now replaces the curve at every status in the declared set, in the client loop and the download hop alike. Refs #775 * fix(swift): honour Retry-After at every retried status calculateDelay short-circuited on statusCode == 429, so a 503 carrying Retry-After backed off from the local curve. The header now replaces the curve at every status the caller's retry set admits, and the status parameter that existed only for that gate is gone. Refs #775 * Carry RetryAfter on every Go error arm, bound the clock token, stub the Ruby sleep Review follow-ups. checkResponse and singleRequest now parse the header once and carry it on every status-mapped error, which is what the SPEC §6 mapping rule says; the generated-package rounding test formats a whole- second target so the parse is measured against a moment the header actually named; the {{httpdate+Ns}} token bounds N to nine digits so all six resolvers compute it in exact integer arithmetic; and the Ruby retry-exhaustion test stubs sleep instead of waiting out two real 7s delays. * Drive the generated Go retry test through its public client; map the TypeScript hook error The generated-package test now lives in package generated_test beside auth_transport_test.go and exercises NewClient + GetProject against an httptest server, so it names nothing a regeneration could rename: a 503's Retry-After is waited in both wire forms, and an over-range value leaves the loop still waiting at the deadline instead of retrying at once. TypeScript's two retry loops hand onRetry the status-mapped BasecampError (SPEC §7 step 3i) rather than a bare Error, so a hook sees the httpStatus and the retryAfter that governs the very sleep it is being told about. * Close the response body and drop the unused server return in the generated-client test * Count Rust among the SDKs the Retry-After contract inventories * Hand the parsed Retry-After to the hook error, and true up three inventories TypeScript's retry loops parsed the header once for the sleep and let errorFromParsedBody parse it again for the on_retry error, so an HTTP-date crossing a whole-second boundary between the two could report one second less than the loop waits; the mapper now takes the value the caller holds. The Status Mapping conflict note counts Rust among the SDKs that populate retry_after at every status, the parsing note lists TypeScript among the parsers still owing the sign row (its matcher accepts a leading sign), and Ruby's README stops saying only a 429 carries the header. * Carry Retry-After through Ruby's public mapper, make the 429 download case discriminating, and inventory Python's wider parse Three findings from the review of the merged head, each true and each small. Ruby's public Basecamp.error_from_response took retry_after: and forwarded it only in the 429 arm, so error_from_response(503, nil, retry_after: 7) answered an error with retry_after nil — against §6's every-status guarantee that the private Http#handle_error already kept. The mapper now forwards the value on the 500, gateway and from_status arms and back-fills the rest exactly as handle_error does, and a test walks every arm. downloads.json's 429 case sent Retry-After: 1 and asserted a 1000ms floor, which the ordinary first backoff (1000ms base + 100ms jitter) already clears — so a loop that ignored the header stayed green, as the 502/503/504 siblings' descriptions said outright. It now sends 2 and asserts 2000ms like them, and the four descriptions describe the case as it is, which makes §14's "pins all four statuses" true. Python's _parse_retry_after delegates to int(), which admits `_` digit separators and surrounding whitespace as well as a sign; §6's convergence inventory named only the sign row as owed. The inventory now says what Python owes is the whole 1*DIGIT gate — the parser itself stays with #799/#775, where the other owed rows are. * Sleep the Retry-After the download loop mapped, and inventory Ruby's stdlib parse alongside Python's Two findings from the review of the rebased head. The download loop's hop-1 retry parsed Retry-After a second time for its own sleep after checkResponse had already parsed it onto the mapped error. The two agree for delay-seconds and can differ by one for an HTTP-date read across a whole-second boundary, so the OnRetry hook could report a delay the loop did not take — against §6's one-parse rule. The loop now reads the value off the mapped error, and a test pins that the hook's error at a 503 carries the value slept. §6's convergence inventory named Python as the one parser owing more than the sign row. Ruby's Integer() without a base is the other stdlib delegate: it admits `_` and the 0x/0b/0o prefixes and reads a leading zero as octal, so `010` sleeps 8 seconds and `09` is malformed. The inventory now names both and what each owes — the whole 1*DIGIT gate — with the parsers themselves still with #799/#775. * Carry Retry-After on Ruby's download errors too The download paths map their own responses rather than going through Http#handle_error: hop 2 raised ApiError.new with no retry_after, and hop 1's defensive fallback called Basecamp.error_from_response without the header. So a 503 with Retry-After: 7 on the signed hop answered retry_after nil — outside §6's every-status guarantee that the public mapper now keeps. Http exposes its parser for those callers, both hop-2 raises and the hop-1 fallback pass the parsed value, and two tests pin hop 2 (a 503 and a refused redirect) carrying the header's value. * Say which layer the every-status Retry-After claim binds at, and inventory the download sites outside it §6's CONFLICT block said five SDKs populate retry_after at every status. Read against every place an SDK builds an error by hand, that was a claim about constructor sites the shared mapper never sees, and it kept coming up false one site at a time: Ruby's public mapper, then Ruby's download hops, now Go's hop-2 errors and hop-1 500, Python's download errors on both paths, and TypeScript's hop-2 errors — each building its api_error without the header. The claim now binds where it can hold: at the status mapping algorithm the retry loops and operation paths return through. A status mapped by hand elsewhere carries the value only where that site parses the header itself; Ruby's hops do, the Go, Python and TypeScript download sites do not, and the block names them as owed by the sweep (#857), where routing them through the mapper or parsing at the site is the change. A future hand-built error is no longer claimed covered by a sentence that cannot see it.
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.
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
Aligns every SDK's
Retry-Afterparser and retry path with SPEC §6, completing #775 and #799. Signed values now fall back to backoff, over-range values saturate at 2,147,483,647 seconds, and Swift honors delays beyond its former 24-hour cap.Behavior
RequestResult.RetryAfterfor every response status, and the Kotlin and Swift hop-1 redirect errors carry it too.BasecampException.retryAfterSecondsonApiandRateLimit; Swift exposes it on.apiand.rateLimit.api_errorand non-retried 429 responses across all runners.Compatibility
.apipattern matches and constructor calls need the new sixthretryAfterSecondsvalue;MIGRATING.mddocuments the source break.Apiconstructor for binary and Java compatibility; theretryAfterSecondsoverload requires its seventh parameter, and untyped constructor references now need an expected function type or a lambda.Written for commit bab63c7. Summary will update on new commits.