Retry docs: correct the mutation and 401 claims, add Python to the roster - #484
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates retry-related documentation to match current SDK behavior (no runtime changes), correcting prior claims about mutation retries and 401 refresh behavior and adding Python to the set of “shipping SDKs” referenced by the spec.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Changes:
- Update
SPEC.mdto include Python among shipping SDK exemplars and to document Gate 3 retry-metadata fidelity drift for Go/Python. - Update
SECURITY.mdretry and 401 sections to reflect per-operation idempotency gating and per-SDK 401 refresh differences.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| SPEC.md | Adds Python to the “shipping SDKs” references and documents known retry-metadata fidelity divergences (Gate 3). |
| SECURITY.md | Rewrites retry/401 behavior documentation to align with actual per-SDK behavior and idempotency-based retry eligibility. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 2. **Shipping SDK code** (consensus of Go, Ruby, TypeScript, Kotlin, Swift) — implementation truth. When 4+ SDKs agree, that's the contract. | ||
| 2. **Shipping SDK code** (consensus of Go, Ruby, Python, TypeScript, Kotlin, Swift) — implementation truth. When 4+ SDKs agree, that's the contract. | ||
| 3. **`behavior-model.json`** — machine-readable metadata. Descriptive of retry/idempotency semantics, but the retry block alone does not activate retry for POST (see §7). | ||
| 4. **`rubric-audit.json`** — audit snapshot. Known to drift (e.g., 3C.3 claims 1024 chars; all 5 SDKs use 500). Trust code over audit. |
| `SubscribeToCardColumn`). The other 39 POSTs are attempted exactly once. SPEC.md §7 specifies the | ||
| three-gate algorithm and the per-SDK divergences. | ||
|
|
||
| - **Reads (GET/HEAD)**: retried with exponential backoff on 429/503 in every SDK. |
![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: d2ea6f6d6e
ℹ️ 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".
| duplicate a resource, which is why the gate is idempotency rather than "is it a mutation". | ||
| **Ruby is the sole exception** — its transport retries GET only. | ||
| Go's separate hand-written `pkg/basecamp` HTTP helper is also GET-only. | ||
| - **Non-idempotent POSTs**: never retried, in any SDK — a retry could create a duplicate resource. |
There was a problem hiding this comment.
When a non-idempotent POST receives a 401 and a refreshable token provider successfully refreshes, Ruby and both Python transports replay the request once regardless of operation idempotency (ruby/lib/basecamp/http.rb:416-420, python/src/basecamp/_http.py:237-251, and the async equivalent), as the table immediately below acknowledges. Therefore these POSTs are not universally “never retried” or always attempted exactly once; scope this claim to retries after 429/503 or network failures.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="SPEC.md">
<violation number="1" location="SPEC.md:405">
P1: Claim in Gate 3 fidelity note about Go's `isRetryableStatus` covering `{429, 500, 502, 503, 504}` needs verification against the actual generated code in `client.gen.go`. The hand-written `checkResponse` in `helpers.go` uses `resp.StatusCode >= 500 && resp.StatusCode < 600` (all 5xx, not just 500/502/503/504), so the doc's narrower set may be incorrect if `isRetryableStatus` diverges from the hand-written path.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| **Non-retryable statuses (never retry regardless of method):** 401, 403, 404, 400, 422. | ||
|
|
||
| **Gate 3 metadata fidelity `[CONFLICT]`.** Gates 1 and 2 read `behavior-model.json`; Gate 3's |
There was a problem hiding this comment.
P1: Claim in Gate 3 fidelity note about Go's isRetryableStatus covering {429, 500, 502, 503, 504} needs verification against the actual generated code in client.gen.go. The hand-written checkResponse in helpers.go uses resp.StatusCode >= 500 && resp.StatusCode < 600 (all 5xx, not just 500/502/503/504), so the doc's narrower set may be incorrect if isRetryableStatus diverges from the hand-written path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At SPEC.md, line 405:
<comment>Claim in Gate 3 fidelity note about Go's `isRetryableStatus` covering `{429, 500, 502, 503, 504}` needs verification against the actual generated code in `client.gen.go`. The hand-written `checkResponse` in `helpers.go` uses `resp.StatusCode >= 500 && resp.StatusCode < 600` (all 5xx, not just 500/502/503/504), so the doc's narrower set may be incorrect if `isRetryableStatus` diverges from the hand-written path.</comment>
<file context>
@@ -398,10 +398,19 @@ If `behavior-model.json` marks an operation with `idempotent: true`, the POST be
**Non-retryable statuses (never retry regardless of method):** 401, 403, 404, 400, 422.
+**Gate 3 metadata fidelity `[CONFLICT]`.** Gates 1 and 2 read `behavior-model.json`; Gate 3's
+*parameters* are not uniformly sourced from it. **Go and Python ignore the per-operation `retry`
+block entirely**: both substitute a client-global attempt count for `retry.max` (Go
</file context>
d2ea6f6 to
3edf1b3
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3edf1b37cc
ℹ️ 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".
| | SDK | 401 behavior | | ||
| |-----|--------------| | ||
| | Go — hand-written `pkg/basecamp` | Refresh via `AuthManager`, then a single retry (`client.go`) | | ||
| | Go — generated `pkg/generated` operations | **No reactive refresh.** `AuthTransport` obtains a token proactively per request via `TokenProvider.AccessToken`; a 401 is returned as `AuthError` | |
There was a problem hiding this comment.
When a generated *WithResponse operation receives a well-formed JSON 401, it does not return an AuthError: for example, GetAccountWithResponse delegates to ParseGetAccountResponse, which populates response.JSON401 and returns response, nil (go/pkg/generated/client.gen.go:29549-29554,32195-32218). Callers following this table may therefore check only err and mistakenly treat the request as successful; document the typed 401 response behavior instead.
Useful? React with 👍 / 👎.
| | Swift | Three-gate: retries when the method is naturally idempotent (GET/HEAD/PUT/DELETE) or the operation is marked `idempotent: true`; non-idempotent POSTs make a single attempt. Gate covers both HTTP status and network-error retries, so Swift *does* retry network errors (unlike Kotlin/TS), gated by idempotency. | | ||
|
|
||
| The table above describes **Gate 1 and Gate 2** — *whether* an operation retries. Gate 3's parameters | ||
| are tracked separately: the per-operation attempt ceiling is honored in every SDK, but the declared |
There was a problem hiding this comment.
When a Ruby caller raises max_retries above 3 and a GET repeatedly receives a retryable error, request_with_retry loops until @config.max_retries (ruby/lib/basecamp/http.rb:354-370) without consulting operation metadata, even though the current model caps retryable GETs at 3. Thus the per-operation ceiling is not honored in every SDK—consistent with the earlier statement that Ruby's per-op fields are inert—so this should explicitly describe the Ruby divergence rather than promise a universal ceiling.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
SECURITY.md:209
- The Go paths in this table should be
go/pkg/...(notpkg/...). Also, the generated client returns a structuredAPIErrorwith codeauth_requiredon 401 (there is noAuthErrortype ingo/pkg/generated).
| Go — hand-written `pkg/basecamp` | Refresh via `AuthManager`, then a single retry (`client.go`) |
| Go — generated `pkg/generated` operations | **No reactive refresh.** `AuthTransport` obtains a token proactively per request via `TokenProvider.AccessToken`; a 401 is returned as `AuthError` |
SPEC.md:415
errors.rbis referenced here, but the Ruby retryable-status logic lives inruby/lib/basecamp/http.rb(seehandle_error, which marks 500/502/503/504 asretryable: true). This makes the citation misleading/non-actionable for readers.
Gate 3's *status set* is not. **Go and Ruby retry statuses the spec never declared retryable**: Go's
`isRetryableStatus` covers `{429, 500, 502, 503, 504}` regardless of the operation's declared
`retry_on`, and Ruby's loop keys off `retryable?`, which `errors.rb` sets for 500/502/503/504.
**Python** has no status gate at all — its loop keys off the `retryable` flag `errors.py` sets for
SECURITY.md:197
- The referenced Go package paths don't exist in this repo (they live under
go/pkg/...). Using the correct paths makes it easier for readers to verify the claims.
This issue also appears on line 208 of the same file.
- **Naturally-idempotent mutations (PUT/DELETE) and the 3 flagged POSTs**: *are* retried on 429/503
by Go (generated operation path), Python, TypeScript, Kotlin, and Swift. Retrying these cannot
duplicate a resource, which is why the gate is idempotency rather than "is it a mutation".
**Ruby is the sole exception** — its transport retries GET only.
Go's separate hand-written `pkg/basecamp` HTTP helper is also GET-only.
- **Non-idempotent POSTs**: never retried, in any SDK — a retry could create a duplicate resource.
7a4c329 to
aee8bd9
Compare
3edf1b3 to
acdb157
Compare
|
Rebased onto current #483 ( All six review findings applied — each verified against source first:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acdb157155
ℹ️ 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".
| duplicate a resource, which is why the gate is idempotency rather than "is it a mutation". | ||
| **Ruby is the sole exception** — its transport retries GET only. | ||
| Go's separate hand-written `pkg/basecamp` HTTP helper is also GET-only. | ||
| - **Non-idempotent POSTs**: never retried on 429/503 or network failure, in any SDK — a retry could create a duplicate resource. This does **not** mean such a POST is always attempted exactly once: a 401 that triggers a successful token refresh replays the request once regardless of idempotency, in Ruby and both Python transports (see the 401 table below). |
There was a problem hiding this comment.
When a Ruby attachment or campfire upload receives a 401, the generated methods call http_post_raw, which delegates to single_request_raw (ruby/lib/basecamp/http.rb:145-147); unlike single_request, that raw path raises the mapped error directly without the refresh-and-replay branch (http.rb:461-471). These non-idempotent POSTs therefore are not replayed after refresh, so qualify both this sentence and the blanket Ruby row in the 401 table to exclude raw upload requests.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
SECURITY.md:190
- This paragraph says the 39 non-idempotent POST operations are "attempted exactly once", but later bullets note some SDKs replay a request once after a successful 401 refresh. To avoid a contradiction, describe them as non-retryable on 429/503/network errors rather than always single-attempt.
Retry eligibility is decided per *operation*, not per HTTP method. `behavior-model.json` classifies
all 226 operations: the 118 GETs are retryable by method, and 69 mutations are flagged
`idempotent: true` — all 45 PUTs, all 21 DELETEs, and 3 POSTs (`CompleteTodo`, `PauseQuestion`,
`SubscribeToCardColumn`). The other 39 POSTs are attempted exactly once. SPEC.md §7 specifies the
three-gate algorithm and the per-SDK divergences.
SPEC.md:412
- This sentence claims the per-operation attempt ceiling is "honored everywhere", but earlier in §2 the spec notes Ruby does not enforce per-op retry.max at all, and Kotlin treats per-op maxRetries as an override (can exceed a lower client cap). The Gate 3 note should be consistent with those documented divergences.
**Gate 3 status-set fidelity `[CONFLICT]`.** Gates 1 and 2 read `behavior-model.json`, and the
per-operation attempt ceiling is now honored everywhere (see *Per-operation retry ceiling* in §2).
SPEC.md:1615
- Appendix F repeats that the per-operation attempt ceiling is honored in every SDK, which conflicts with the earlier §2 discussion (Ruby ignores per-op retry.max; Kotlin treats it as an override rather than a ceiling). Align this summary sentence with the detailed behavior described earlier.
The table above describes **Gate 1 and Gate 2** — *whether* an operation retries. Gate 3's parameters
are tracked separately: the per-operation attempt ceiling is honored in every SDK, but the declared
`retry_on` set is not — Go, Python, and Ruby all retry a superset of it. See the Gate 3 status-set
note at §7 above.
acdb157 to
4908d52
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4908d52a67
ℹ️ 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".
| | Go — generated `pkg/generated` operations | **No reactive refresh.** `AuthTransport` obtains a token proactively per request via `TokenProvider.AccessToken`. A 401 is **not** returned as an error by the `*WithResponse` variants: they populate `response.JSON401` and return `(response, nil)`, so a caller checking only `err` would read the request as successful. Check `JSON401` (or the status code) explicitly. The `ParseHTTPError` helper is what maps a 401 to `AuthError` | | ||
| | Ruby | Refresh, then a single retry (`http.rb`) | | ||
| | Python (sync **and** async) | Refresh, then a single retry (`_http.py`, `_async_http.py`) | | ||
| | TypeScript | No refresh; the 401 is raised to the caller | |
There was a problem hiding this comment.
When consumers use the public raw API (BasecampClient extends RawClient), a 401 is not raised: client.GET(...) resolves with { data: undefined, error }, as asserted in typescript/tests/client.test.ts:335-353. Only generated service wrappers convert that result into a thrown BasecampError, so callers following this row may rely on try/catch and miss authentication failures from direct GET/POST calls; distinguish service-wrapper behavior from the raw openapi-fetch client.
AGENTS.md reference: AGENTS.md:L25-L28
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
SPEC.md:1610
- The Appendix F Ruby row says “All non-GET methods never retry”, but Ruby does replay a request once after a successful 401 token refresh (see
ruby/lib/basecamp/http.rbwheresingle_requestretries once whenAuthError401 and@token_refreshedis true). The Ruby row should mention this exception so it matches the actual transport behavior described elsewhere.
| Ruby | Simplified: only GET retries. All non-GET methods never retry. Ruby retries on any error with `retryable? == true`. |
| Python | Three-gate, sync and async: `_mutation()` retries only when `behavior-model` metadata classifies the operation retryable, so non-idempotent POSTs are single-attempt; GETs always retry. Retries on any error with `retryable == True`. |
SPEC.md:417
errors.rbis referenced as the place Ruby marks 500/502/503/504 as retryable, but there is noruby/lib/basecamp/errors.rbin this repo. Ruby’s retryability is set by the HTTP error mapping (handle_error) inruby/lib/basecamp/http.rb, which constructsBasecamp::ApiError(..., retryable: true)for 500/502/503/504; the doc should point at the correct file to keep this claim verifiable.
This issue also appears on line 1609 of the same file.
Gate 3's *status set* is not. **Go and Ruby retry statuses the spec never declared retryable**: Go's
`isRetryableStatus` covers `{429, 500, 502, 503, 504}` regardless of the operation's declared
`retry_on`, and Ruby's loop keys off `retryable?`, which `errors.rb` sets for 500/502/503/504.
**Python** has no status gate at all — its loop keys off the `retryable` flag `errors.py` sets for
the same statuses. TypeScript, Kotlin, and Swift do gate on the declared set.
SECURITY.md:192
- This bullet reads as if GET retries are limited to 429/503, but several SDKs also retry other 5xx statuses (e.g., Go generated client retries 500/502/504 via
isRetryableStatus, Ruby marks 500/502/503/504 errorsretryable: truefor GET requests, and Python marks 500/502/503/504 retryable inerrors.py). The docs should either mention that superset or explicitly point readers to SPEC.md §7 for the expanded status set so this doesn’t understate retry behavior.
- **Reads (GET)**: retried with exponential backoff on 429/503 in every SDK. (HEAD is idempotent by method too, but Ruby's transport gates on `method == :get` specifically, so a HEAD would not retry there. The API surface has no HEAD operations today, so this is theoretical.)
4908d52 to
6b71f7a
Compare
|
Three more corrections, all verified against source first. Ruby is excluded from the per-operation ceiling claim. The Appendix F footnote said the ceiling is "honored in every SDK". Ruby's 401 replay excludes raw uploads. TypeScript's 401 depends on which API you call. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b71f7af77
ℹ️ 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".
| BC5 issuer that was then selected.) | ||
|
|
||
| #### SSRF hardening — both hops, all five SDKs `[conformance]` | ||
| #### SSRF hardening — both hops, all six SDKs `[conformance]` |
There was a problem hiding this comment.
When readers rely on the [conformance] marker, this heading incorrectly implies that OAuth discovery and its SSRF checks exist in every SDK. A repo-wide search finds discovery implementations and fixture coverage for Go, TypeScript, Ruby, Kotlin, and Python only; Swift has no OAuth implementation, and make conformance has no Swift runner. Keep this scoped to the five implementing SDKs unless Swift support and coverage are added.
AGENTS.md reference: AGENTS.md:L36-L39
Useful? React with 👍 / 👎.
| **Non-retryable statuses (never retry regardless of method):** 401, 403, 404, 400, 422. | ||
|
|
||
| **Gate 3 status-set fidelity `[CONFLICT]`.** Gates 1 and 2 read `behavior-model.json`, and the | ||
| per-operation attempt ceiling is now honored everywhere (see *Per-operation retry ceiling* in §2). |
There was a problem hiding this comment.
When a Ruby caller configures more attempts than an operation's declared retry.max, this statement is false because request_with_retry uses only @config.max_retries. Fresh evidence in this revision is the newly added Appendix F note at lines 1613–1617, which explicitly says Ruby never consults the per-operation metadata; qualify “everywhere” here to avoid giving implementers the opposite contract.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
SPEC.md:412
- This sentence says the per-operation attempt ceiling is honored “everywhere”, but Appendix F below explicitly notes Ruby never consults the per-operation ceiling. Tighten this claim to the SDKs that actually honor the ceiling to avoid an internal contradiction.
**Gate 3 status-set fidelity `[CONFLICT]`.** Gates 1 and 2 read `behavior-model.json`, and the
per-operation attempt ceiling is now honored everywhere (see *Per-operation retry ceiling* in §2).
SPEC.md:415
- This references
errors.rb, but Ruby’s retryability is set viahandle_errorinruby/lib/basecamp/http.rb(andApiError.from_statusinapi_error.rb). Updating the file reference will make the citation actionable.
`isRetryableStatus` covers `{429, 500, 502, 503, 504}` regardless of the operation's declared
`retry_on`, and Ruby's loop keys off `retryable?`, which `errors.rb` sets for 500/502/503/504.
SECURITY.md:190
- This summary says “The other 39 POSTs are attempted exactly once”, but Ruby and Python both may replay a request once after a successful 401 refresh (and you note that later in this section). Consider softening this sentence so it doesn’t contradict the 401 behavior described below.
Retry eligibility is decided per *operation*, not per HTTP method. `behavior-model.json` classifies
all 226 operations: the 118 GETs are retryable by method, and 69 mutations are flagged
`idempotent: true` — all 45 PUTs, all 21 DELETEs, and 3 POSTs (`CompleteTodo`, `PauseQuestion`,
`SubscribeToCardColumn`). The other 39 POSTs are attempted exactly once. SPEC.md §7 specifies the
three-gate algorithm and the per-SDK divergences.
…ster SECURITY.md said mutations are never retried and that a 401 triggers a token refresh plus a single retry "for all methods". Both are false. All 45 PUTs, 21 DELETEs, and the 3 flagged POSTs do retry — in Go's generated operation path, Python, TypeScript, Kotlin, and Swift. Ruby is the sole GET-only transport (as is Go's separate hand-written HTTP helper). The mutations bullet dates to PR #60, before the generated client existed. Reactive 401 refresh exists in exactly three places: hand-written Go, Ruby, and Python (sync and async). Generated Go operations do not refresh reactively — AuthTransport fetches a token proactively per request and a 401 becomes AuthError. TypeScript, Kotlin, and Swift have no refresh at all. Replaced the bullet with a per-SDK matrix. SPEC.md: - "Five shipping SDKs" and the implementation-consensus list both omitted Python. - The network-error divergence named Go, Ruby, and Swift. Python belongs in that group; Ruby does not — it retries network errors only on GET, since every non-GET routes to a single-attempt path. - Appendix F's retry table had no Python row. - Added a Gate 3 fidelity note: Go and Python honor the per-operation idempotent classification but ignore the per-operation retry block, substituting a client-global attempt count and a superset of retry_on. Tracked in #479. Documentation only; no behavior change. The TypeScript one-retry waiver (2B.1) is current, not stale — client.ts:1007 returns native fetch(), bypassing middleware — so it is left as written.
6b71f7a to
8af46a1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8af46a1f4f
ℹ️ 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".
| - **Naturally-idempotent mutations (PUT/DELETE) and the 3 flagged POSTs**: *are* retried on 429/503 | ||
| by Go (generated operation path), Python, TypeScript, Kotlin, and Swift. Retrying these cannot |
There was a problem hiding this comment.
When UpdateAccountLogo receives a 429 or 503, Kotlin does not retry it: the generated account service routes this PUT through httpPutMultipart, which calls requestBinaryWithRetry, and that method returns the first response without any retry/status check (BaseService.kt:146-151, BasecampHttpClient.kt:211-218). Therefore Kotlin is another exception to this blanket mutation claim, despite the operation being marked idempotent.
Useful? React with 👍 / 👎.
| ``` | ||
|
|
||
| `[CONFLICT: rubric-audit.json 3C.3 says 1024; all 5 SDKs use 500. Code wins.]` | ||
| `[CONFLICT: rubric-audit.json 3C.3 says 1024; all six SDKs use 500. Code wins.]` |
There was a problem hiding this comment.
When Python receives a 401 or 403 whose JSON error string exceeds 500 bytes, error_from_response passes that message directly to AuthError or ForbiddenError (python/src/basecamp/errors.py:143-146), unlike the branches that call _truncate. Thus Python does not satisfy the following universal error-message truncation contract for those responses, so claiming all six SDKs use the 500-byte limit is misleading unless these statuses are qualified.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
SPEC.md:418
- This section contradicts the preceding Gate 3 bullet: the spec explicitly allows implementations to expand the retryable status set beyond
retry_on: [429, 503], so these statuses are spec-sanctioned. It looks like you meant that these statuses are not declared inbehavior-model.json’sretry_onmetadata.
Also, errors.rb doesn’t exist in the Ruby SDK; the 500/502/503/504 retryable classification is set in ruby/lib/basecamp/http.rb (handle_error).
by fewer still. **Go and Ruby retry statuses the spec never declared retryable**: Go's
`isRetryableStatus` covers `{429, 500, 502, 503, 504}` regardless of the operation's declared
`retry_on`, and Ruby's loop keys off `retryable?`, which `errors.rb` sets for 500/502/503/504.
**Python** has no status gate at all — its loop keys off the `retryable` flag `errors.py` sets for
Documentation only — no behavior change. Every claim below was re-derived from source at
d5604962.SECURITY.md— two false bullets"Mutations (POST/PUT/DELETE): NOT automatically retried on 429/503". They are.
behavior-model.jsonflags 69 operationsidempotent: true— all 45 PUTs, all 21 DELETEs, and 3 POSTs (CompleteTodo,PauseQuestion,SubscribeToCardColumn) — and those retry in Go's generated operation path, Python, TypeScript, Kotlin, and Swift. Ruby is the only transport that is GET-only (http.rbroutes every non-GET tosingle_request), as is Go's separate hand-writtenpkg/basecamphelper. The bullet dates to #60, before the generated client existed.The 39 non-idempotent POSTs are single-attempt everywhere, which is the property the bullet was reaching for — so the replacement states the gate as idempotency rather than "is it a mutation".
"401 responses: Token refresh attempted, then single retry for all methods". True in three transports, false in four:
pkg/basecampAuthManager, then one retry (client.go:794-800)pkg/generatedAuthTransport.RoundTripfetches a token proactively (client.gen.go:19851);ParseHTTPErrorturns 401 intoAuthError(:20237)http.rb:417)_http.py:237,_async_http.py:237)Swift's
HTTPClient.swift:157re-runs the auth strategy before each retry, but 401 is not a retryable status, so that path never fires for a 401.SPEC.md— Python was missing, Ruby was miscategorized:11"Five shipping SDKs" and:38"consensus of Go, Ruby, TypeScript, Kotlin, Swift" both omitted Python. Six now.:401named "Go, Ruby, and Swift" as retrying network errors. Python belongs there (NetworkErrorisretryable=Trueand the loop keys off it). Ruby does not belong in the same group — it retries network errors only on GET, becauserequestsends every non-GET tosingle_request. Listing it alongside Go/Swift implied a parity it does not have.New: Gate 3 fidelity note
Gates 1 and 2 read
behavior-model.json. Gate 3's parameters do not, in two SDKs — Go and Python substitute a client-global attempt count for the per-operationretry.maxand treat a superset ofretry_onas retryable. Go'sOperationMetadatacarries no attempt count at all. That is now stated in §7 and footnoted under Appendix F, and filed as #479 with the fix design.Also filed #480:
SubscribeandEnableCardColumnOnHoldare under-classified as non-idempotent (Subscribecalls the identicalRecording::Subscribable#subscribethat the flaggedSubscribeToCardColumncalls;EnableCardColumnOnHold's create isunless @on_hold-guarded and its DELETE sibling is already flagged). Both verified against BC32f01d0da.Deliberately left alone
SPEC.md:400— "Implementations may expand this set to include other 5xx statuses" sanctions the Go/Python behavior. It should go, but only together with the Retry metadata fidelity: Go and Python ignore per-operation retry_on and max_attempts #479 fix; removing it now would make the spec false in the other direction.SPEC.md:407/:411/:1591and waiver 2B.1 — the TypeScript one-retry cap is current, not stale:client.ts:1007returns nativefetch(retryRequest), bypassing middleware, so theX-Retry-Attemptheader it sets is never re-consumed.conformance/runner/typescript/runner.test.ts:73-78pins it.make checkpasses.Summary by cubic
Updates retry docs to match reality: idempotent mutations do retry on 429/503, 401 handling varies by SDK, and Python is now included. Also clarifies network-error retry behavior, per-transport 401 differences, Ruby’s raw-upload exception, and Gate 3 fidelity.
Bug Fixes
*WithResponsefillsJSON401and returns no error; TypeScript service wrappers throw on 401 but the raw client returns{ data, error }without throwing; Kotlin/Swift surface 401; Ruby raw uploads (post_raw/put_raw) do not replay.Spec Updates
retry_on; Python has no status gate (uses aretryableflag); TypeScript/Kotlin/Swift use the declared set. Ruby ignores per-operation retry ceilings; others honor them.Written for commit 8af46a1. Summary will update on new commits.