spec(retry): pin the Retry-After contract, honour it at every retried status, and give conformance a clock - #855
Open
jeremy wants to merge 14 commits into
Open
spec(retry): pin the Retry-After contract, honour it at every retried status, and give conformance a clock#855jeremy wants to merge 14 commits into
jeremy wants to merge 14 commits into
Conversation
jeremy
added
bug
github-actions
Bot
added
typescript
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. |
This was referenced Sep 10, 2026
There was a problem hiding this comment.
🟡 Changes recommended
Go still drops Retry-After metadata for several status mappings, and the token grammar exceeds runner-supported ranges.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Pins the cross-SDK Retry-After contract, aligns retry behavior, and adds clock-aware conformance fixtures.
Changes:
- Honors
Retry-Afteracross all retryable statuses. - Standardizes parsing, rounding, saturation, and error propagation.
- Adds dynamic HTTP-date fixtures and six-language runner support.
[!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 |
|---|---|
SPEC.md |
Defines the consolidated retry contract. |
conformance/schema.json |
Documents the dynamic HTTP-date token. |
conformance/tests/retry.json |
Adds status and future-date cases. |
conformance/tests/downloads.json |
Covers gateway-status download retries. |
go/templates/client.tmpl |
Updates generated retry parsing and waiting. |
go/pkg/generated/client.gen.go |
Regenerates the Go client. |
go/pkg/generated/retry_after_test.go |
Tests generated Retry-After parsing. |
go/pkg/basecamp/client.go |
Propagates and honors gateway delays. |
go/pkg/basecamp/client_retry_after_test.go |
Tests saturation and 503 handling. |
go/pkg/basecamp/helpers.go |
Adds Retry-After to generic errors. |
go/pkg/basecamp/errors.go |
Updates RetryAfter documentation. |
go/pkg/basecamp/download.go |
Honors headers across download retries. |
typescript/src/errors.ts |
Propagates retry delays across errors. |
typescript/src/retry.ts |
Removes the 429-only gate. |
typescript/src/services/base.ts |
Updates service retry handling. |
typescript/src/download.ts |
Updates download policy documentation. |
typescript/tests/errors.test.ts |
Tests 503 error metadata. |
typescript/tests/retry-after.test.ts |
Tests 503 delay selection. |
ruby/lib/basecamp/api_error.rb |
Adds retry delay construction support. |
ruby/lib/basecamp/http.rb |
Propagates delays and rounds dates upward. |
ruby/test/basecamp/http_extended_test.rb |
Tests 503 and rounding behavior. |
python/src/basecamp/errors.py |
Adds deterministic upward date rounding. |
python/tests/test_errors.py |
Tests rounding and error metadata. |
python/tests/test_http.py |
Tests 503 retry delays. |
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/http/BasecampHttpClient.kt |
Removes the 429-only retry gate. |
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/Download.kt |
Aligns download retry behavior. |
kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/RetryTest.kt |
Tests 503 honoring. |
kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/HeaderTokens.kt |
Implements header-token resolution. |
kotlin/conformance/src/main/kotlin/com/basecamp/sdk/conformance/Main.kt |
Resolves tokens when serving responses. |
kotlin/conformance/src/test/kotlin/com/basecamp/sdk/conformance/HeaderTokensTest.kt |
Tests Kotlin token resolution. |
swift/Sources/Basecamp/HTTP/HTTPClient.swift |
Removes status-specific delay gating. |
swift/Sources/Basecamp/Download.swift |
Updates download contract documentation. |
swift/Tests/BasecampTests/RetryTests.swift |
Tests 503 honoring. |
conformance/runner/swift/Sources/ConformanceSupport/HeaderTokens.swift |
Implements Swift token resolution. |
conformance/runner/swift/Sources/ConformanceRunner/ScriptedTransport.swift |
Resolves headers at response time. |
conformance/runner/swift/Tests/ConformanceSupportTests/HeaderTokensTests.swift |
Tests Swift token resolution. |
conformance/runner/typescript/header-tokens.ts |
Implements TypeScript token resolution. |
conformance/runner/typescript/header-tokens.test.ts |
Tests TypeScript token resolution. |
conformance/runner/typescript/runner.test.ts |
Resolves dynamic response headers. |
conformance/runner/ruby/runner.rb |
Resolves queued headers at serve time. |
conformance/runner/ruby/header_tokens_test.rb |
Tests Ruby token resolution. |
conformance/runner/python/runner.py |
Implements and applies token resolution. |
conformance/runner/python/test_header_tokens.py |
Tests Python token resolution. |
conformance/runner/go/header_tokens.go |
Implements Go token resolution. |
conformance/runner/go/main.go |
Applies resolved response headers. |
conformance/runner/go/header_tokens_test.go |
Tests Go token resolution. |
Review details
- Files reviewed: 44/46 changed files
- Comments generated: 4
- 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: 1ad16855b8
ℹ️ 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: 46cc11458c
ℹ️ 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 f4a5d2a (46 checks) and the two review rounds — Copilot's four findings and Codex's three — are each fixed and answered in-thread; nothing new has landed since the last push. Local |
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
spec/retry-after-contract
branch
from
September 10, 2026 19:13
f4a5d2a to
38b4657
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38b4657ebf
ℹ️ 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".
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
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
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
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
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
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
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
…he 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.
…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.
…rated-client test
…ntories 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.
jeremy
force-pushed
the
spec/retry-after-contract
branch
from
September 10, 2026 21:46
2e8eecf to
3d2a62d
Compare
…ract * origin/main: fix(kotlin): project the transport error before it becomes the network message (#872) deps(rust): bump toml in /rust in the cargo-dependencies group (#863) deps(actions): bump the actions group across 1 directory with 2 updates (#865) Compile the CodeQL Kotlin build with the newest Kotlin CodeQL supports (#875)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd84e2b4b1
ℹ️ 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".
… 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6716fb113
ℹ️ 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".
| } | ||
| // Honoured at every status in the hop-1 set, not at 429 alone | ||
| // (SPEC §14 "Hop-1 Retry"). | ||
| retryAfter = parseRetryAfter(r.Header.Get("Retry-After")) |
There was a problem hiding this comment.
When a newly covered 502/503/504 download response carries an HTTP-date and execution crosses a whole-second boundary here, checkResponse has already parsed the header into lastErr.RetryAfter, but this second parse can return one second less. The retry hook then receives an error reporting a different delay from the one the loop actually sleeps, contrary to the one-parse contract; derive the sleep from the mapped error or pass one parsed value to both consumers.
Useful? React with 👍 / 👎.
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-first convergence of the Retry-After cluster (#775, #799, #798, #780): SPEC §6 now states the whole contract in one place, the conformance suite can finally fail on the parts that were unpinnable, and the SDK changes that were one-line gate deletions ride along so the new cases are green in all six. What is not mechanical is listed at the end for a second PR.
What SPEC pins (§6, §7, §14, §19, Appendix A/D)
retry.json(503 withRetry-After: 2) anddownloads.json(502/503/504).retry_afteron the error at every status. The Status Mapping Algorithm populates it wherever the header parses, not only in the 429 arm; one parse feeds the sleep and the field. Go, TypeScript, Ruby and Python do this now; Kotlin'sApiand Swift's.apihave no slot, recorded as a conflict (Swift's is a source-breaking enum change, so it is PR 2).1*DIGITonly (no sign, no fraction, no junk),0and a past date fall through, HTTP-date rounded up, and one ceiling —MAX_RETRY_AFTER_SECONDS = 2,147,483,647, Appendix A — that both wire forms saturate at, in the parser.retry_afterinteger any of the six ships (Go's 32-bitint, Kotlin'sInt) and the ceiling §16 already shares, so it is representability, not policy. Every host can schedule it, so no second bound is needed anywhere but TypeScript'ssetTimeoutclamp, and Swift's 86,400 s clamp is named as the one policy cap.{{httpdate+Ns}}, resolved by every runner at serve time to the IMF-fixdate offloor(now) + N + 1s. A compliant round-up parser computes at least N whole seconds for any serve-to-parse latency under a second, so the fixture assertsmin: N × 1000; a parser that drops the date form lands on the ~1 s curve. Relative and near on purpose — an absolute far-future date is differently behaved per host. Each runner's resolver is unit-tested against a frozen instant, the waycheckDelayGapsis; Ruby moves its header merge into theto_returnblock so the token sees the rightnow.behavior-model.jsonis untouched: nothing in the contract is per-operation.SDK changes in this PR (mechanical; each makes a new case green)
Atoiwith a SPEC §6 parser (digits-only, saturating, HTTP-date rounded up) used by the loop andcheckResponse— closes #798. Hand-written: 5xx arms carryRetryAfter(which is what lets the raw loop honour a 503), download hop parses at every status in its set, parser saturates any over-ceiling digit string instead of treating a >int64 one as malformed.status === 429ternary;errorFromParsedBodypassesretryAfteron every arm.handle_errorthreadsretry_afterinto every arm (ApiErrorgains the kwarg); HTTP-date rounds up; parser takes anow:seam.now=seam. Already conformant on the status set.status == 429 &&.calculateDelaydrops thestatusCode == 429guard and the parameter that existed for it.Tests: TypeScript's "ignores Retry-After on a status that is not 429" is flipped to the new contract; Go's fall-through table loses its two width rows to the saturating table and gains a 503 pin plus a generated-package parser test; Ruby/Python pin rounding against a frozen clock and a 503 that carries the field; Kotlin and Swift pin 503 honouring.
Not
breaking: every public change is additive (Error.RetryAfterset at more statuses,retryAfterpresent on more errors, a new optional kwarg on Ruby'sApiError).Left for PR 2 (per-SDK sweep), with the sites
.apigainsretryAfterSeconds(breaking; MIGRATING) and drops the 86,400 s clamp atHTTPClient.swiftsleepNanoseconds; KotlinBasecampException.ApigainsretryAfterSecondsandfromHttpStatusthreads it. Then a conformanceerrorField retryAftercase at 503.+5:typescript/src/errors.tsparseRetryAfter,kotlin/.../Pagination.ktparseRetryAfter,swift/.../BasecampError.swiftparseRetryAfter,python/src/basecamp/errors.py_parse_retry_after,ruby/lib/basecamp/http.rbparse_retry_after. Python and Ruby currently raise at the scheduler above their ceilings.typescript/src/services/base.ts(baresetTimeout),typescript/src/download.ts(executeWithRetrycalled without a signal),ruby/lib/basecamp/http.rb(sleep),python/src/basecamp/_http.py(time.sleep).http.go's hook-facing429 || 503parse andresilience.go's rate-limiter block to any status; hand-writtenparseRetryAfterdelegating to the generated one now that it exists._parse_retry_afteron asctime:parsedate_to_datetimereturns a naive datetime and the subtraction'sTypeErroris swallowed (a MAY, but a swallowed exception is a bug).Verification
make check -k(all six SDK checks, drift gates, doc-constants, six conformance runners);lint-actionsfails on the seven pre-existing zizmor lows and nothing else. Expected merge note: #853 also regeneratesgo/pkg/generated/client.gen.go; the hunks are disjoint (gauges types vs. the retry loop), so git should merge them cleanly, but whichever lands second wants amake -C go generateto confirm no drift.Fixes #798
Fixes #780
Refs #775
Refs #799
Summary by cubic
Pins the Retry-After contract in SPEC §6 and aligns all six SDKs to honour it at every retried status instead of only 429. Adds a conformance clock token so the HTTP-date branch is finally testable.
Contract
{{httpdate+Ns}}header values, resolved at serve time to floor(now)+N+1 seconds. The download 429 case now sendsRetry-After: 2with a 2000ms floor, so ignoring the header fails.SDK changes
Atoiwith the SPEC parser and drives its retry tests through the public client against anhttptestserver; both Go clients parse the header once and carryRetryAfteron every status-mapped error.retryAfterthrough error objects; TypeScript'sonRetryhook receives the status-mapped error carryinghttpStatusand the loop's single-parsedretryAfter.ApiErrorgains an optionalretry_afterkwarg and its public mapper forwards it on every status arm; no breaking changes.1*DIGITgate; both are left for a follow-up PR.Written for commit a6716fb. Summary will update on new commits.