Retry: honor the declared retry_on and max_attempts in Go and Python

[jeremy]

Closes #479. Stacked on #484 → #483 → main.

#483 fixed the per-operation attempt ceiling. This fixes the other half of Gate 3: the status set.

What was wrong

behavior-model.json declares retry_on: [429, 503] for all 226 operations. Neither Go nor Python consulted it:

• Go used a global isRetryableStatus covering {429, 500, 502, 503, 504} (client.gen.go:3964-3974), regardless of what an operation declared.
• Python had no status gate at all — its loop keyed off e.retryable, and errors.py:153-156 marks 500 and 502/503/504 retryable.

So both retried three statuses the spec never sanctioned.

What this does

Go. The declared set is emitted as operationRetryOn, a map[string][]int sibling of #483's operationRetryMax — and for the same reason kept off the exported OperationMetadata struct, so wiring it up cannot break an external unkeyed composite literal. isRetryableStatus now takes the operationId and consults it, falling back to defaultRetryOn for an id absent from the table (unreachable from a generated call site).

Python. _is_retryable_error(error, operation) gates on the operation's declared retry_on, resolved by _operation_retry_on(operation) from the metadata #483 already threads through every call site. Because #483 threads a canonical operation id into get() as well, the read path is governed too — no separate mechanism.

errors.py and ParseHTTPError are untouched. They still classify 500/502/503/504 as retryable to callers. That is the caller-facing hint; it must not widen the transport's gate. error-mapping.json conformance cases pass unchanged.

Network errors stay retryable. They carry no HTTP status, so the status gate does not apply — SPEC §7's network-error rule still governs.

Ungoverned traffic is protected structurally, not by convention

get_absolute() — the hand-written Launchpad authorization request — passes no operation id. So both the status gate and #483's attempt ceiling no-op on it automatically, and it keeps its pre-Smithy contract. That is a better outcome than the explicit policy object this PR carried in its first revision, and it falls out of #483's threading rather than needing new machinery.

Four regressions (sync and async) pin it. They pass identically before and after this change — which is the point: they exist to fail if generated policy ever reaches OAuth traffic.

Guard

Extended #483's scripts/check-retry-metadata-parity.py rather than adding a second script:

• new from_go_retry_on() extractor + check_retry_on_only() parity check for operationRetryOn;
• Go and Python reclassified max only → max + retry_on. Go + Python: honor per-operation retry.max as a ceiling; add parity guard #483's forbidden token list asserted Go does not consume RetryOn; this makes that false, so leaving it would have failed the build.

Proof matrix — all fail against the un-fixed code

Python, the full 2×2 (sync/async × mutation/read):

FAILED TestDeclaredRetryStatuses::test_sync_mutation_status_gate[500-1]   assert 3 == 1
FAILED TestDeclaredRetryStatuses::test_sync_mutation_status_gate[502-1]   assert 3 == 1
FAILED TestDeclaredRetryStatuses::test_sync_mutation_status_gate[504-1]   assert 3 == 1
FAILED TestDeclaredRetryStatuses::test_async_mutation_status_gate[500-1]  assert 3 == 1
FAILED TestDeclaredRetryStatuses::test_async_mutation_status_gate[502-1]  assert 3 == 1
FAILED TestDeclaredRetryStatuses::test_async_mutation_status_gate[504-1]  assert 3 == 1
FAILED TestDeclaredRetryStatuses::test_sync_read_status_gate              assert 3 == 1
FAILED TestDeclaredRetryStatuses::test_async_read_status_gate             assert 3 == 1

Go: made 3 attempts for status 500, want 1 (likewise 502 and 504), plus a read-path case on GetAccount.

SPEC

• Removed "Implementations may expand this set to include other 5xx statuses (500, 502, 504)" — that sentence sanctioned the bug. The declared set is now stated as exhaustive, with an explicit note that an SDK may still classify those statuses as retryable in its error taxonomy without widening the gate.
• Added the Gate 3 consumption table. Corrected since first posting: only Kotlin turns the ceiling into a floor (it is the one SDK with both a numeric caller cap and a resolution order that discards it — opRetry?.maxRetries ?: config.maxRetries). TypeScript and Swift expose no numeric cap, so there is nothing to discard; Ruby's gap is the mirror image, honoring the caller and never consulting the operation. Kotlin applies per-operation maxAttempts as a floor; Ruby ignores it entirely #485 has been rescoped accordingly, and Go + Python: honor per-operation retry.max as a ceiling; add parity guard #483's contradicting rationale paragraph corrected on that branch.

Note for the merge

The base is a stack branch, so GitHub shows no closing reference for #479. Retarget to main before final merge, or close #479 explicitly with landed evidence.

make check exits 0 — real exit code, captured into the log rather than read from a trailing echo.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4dfa5f9288

ℹ️ 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".

[chatgpt-codex-connector]

Return a copy of generated retry statuses

When an SDK consumer calls GetOperationMetadata and modifies an element of the returned RetryOn slice, it modifies the backing array stored in the package-global operationMetadata map. Subsequent requests for that operation then use the caller-modified retry policy (and concurrent requests can race on it); for example, assigning meta.RetryOn[0] = 500 makes 500 retryable and stops treating 429 as retryable. Clone the slice before exposing it from the generated getter, implementing the change in this template rather than patching generated output.

AGENTS.md reference: AGENTS.md:L70-L70

Useful? React with 👍 / 👎.

[Copilot]

Pull request overview

This PR fixes retry behavior drift in the Go and Python SDKs by honoring each operation’s declared retry_on status set and max_attempts ceiling (as defined in behavior-model.json), and adds a parity guard to keep generated retry metadata consistent across all SDKs. It also updates SPEC.md to state the declared retry_on set is exhaustive and to define attempt resolution as a ceiling (not a floor).

Changes:

• Go: extend generated OperationMetadata with MaxAttempts/RetryOn, gate retries on the per-operation declared status set, and cap attempts via min(configured_attempts, operation_max).
• Python (sync + async): introduce a shared _retry_policy module, apply declared status gating + attempt ceiling to generated traffic, and explicitly keep get_absolute() (OAuth) ungoverned.
• Add scripts/check-retry-parity (wired into make check) to assert generated retry metadata parity across all six SDKs and enforce the “readonly uniform policy” invariant used by Python’s read path.

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.

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
SPEC.md	Updates Gate 3 to treat retry_on as exhaustive, defines attempt ceiling resolution, and documents current cross-SDK consumption.
Makefile	Adds check-retry-parity and wires it into make check.
scripts/check-retry-parity.sh	Shell wrapper to run the Ruby parity guard with a Ruby presence check.
scripts/check-retry-parity.rb	New guard asserting per-operation retry metadata parity across SDKs and readonly uniformity invariant.
go/templates/client.tmpl	Template changes to generate RetryOn/MaxAttempts into operation metadata and use them in retry gating/capping.
go/pkg/generated/client.gen.go	Regenerated Go client implementing per-operation retry gating and attempt ceiling via OperationMetadata.
go/pkg/basecamp/generated_retry_test.go	Adds Go tests pinning declared-status gating and operation max ceiling behavior.
python/src/basecamp/_retry_policy.py	New module modeling per-operation retry policy + ungoverned policy and attempt/status decision helpers.
python/src/basecamp/_http.py	Applies policy-aware retry gating/capping for sync HTTP client; keeps OAuth traffic ungoverned.
python/src/basecamp/_async_http.py	Same policy-aware retry logic for async HTTP client.
python/tests/test_http.py	Updates old “retry on 500” expectation and adds comprehensive sync/async read/write/ceiling/ungoverned regression coverage.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[cubic-dev-ai]

3 issues found across 11 files

_{You’re at about 90% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.}

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="scripts/check-retry-parity.rb">

<violation number="1" location="scripts/check-retry-parity.rb:96">
P1: `make check` cannot run this new guard: scanning Go generated source raises an invalid US-ASCII byte-sequence error before checking metadata. Read the file as UTF-8 before applying the regex.</violation>
</file>

<file name="go/pkg/generated/client.gen.go">

<violation number="1" location="go/pkg/generated/client.gen.go:19593">
P1: `OperationMetadata` now includes a mutable `[]int` `RetryOn` slice, so returning metadata by value leaks a reference to the package-global backing array. A caller can mutate `meta.RetryOn` and change retry behavior for subsequent requests, and concurrent callers can race on that shared slice. Cloning `RetryOn` in `GetOperationMetadata` before returning would keep the metadata read-only to consumers.</violation>
</file>

<file name="python/src/basecamp/_retry_policy.py">

<violation number="1" location="python/src/basecamp/_retry_policy.py:71">
P2: An operation declaring an empty `retry_on` set would retry 429/503 anyway, rather than retrying no HTTP statuses. Preserve an explicit empty list; only default when the field is absent.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P1: make check cannot run this new guard: scanning Go generated source raises an invalid US-ASCII byte-sequence error before checking metadata. Read the file as UTF-8 before applying the regex.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At scripts/check-retry-parity.rb, line 96:

<comment>`make check` cannot run this new guard: scanning Go generated source raises an invalid US-ASCII byte-sequence error before checking metadata. Read the file as UTF-8 before applying the regex.</comment>

<file context>
@@ -0,0 +1,242 @@
+
+def parse_go(path)
+  # "OpId": {Idempotent: …, HasSensitiveParams: …, MaxAttempts: 3, RetryOn: []int{429, 503}},
+  File.read(path).scan(
+    /"([A-Za-z0-9_]+)":\s*\{[^}]*?MaxAttempts:\s*(\d+),\s*RetryOn:\s*\[\]int\{\s*([0-9,\s]*?)\s*\}/
+  ).each_with_object({}) do |(op, max, statuses), acc|
</file context>

[cubic-dev-ai]

P1: OperationMetadata now includes a mutable []int RetryOn slice, so returning metadata by value leaks a reference to the package-global backing array. A caller can mutate meta.RetryOn and change retry behavior for subsequent requests, and concurrent callers can race on that shared slice. Cloning RetryOn in GetOperationMetadata before returning would keep the metadata read-only to consumers.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At go/pkg/generated/client.gen.go, line 19593:

<comment>`OperationMetadata` now includes a mutable `[]int` `RetryOn` slice, so returning metadata by value leaks a reference to the package-global backing array. A caller can mutate `meta.RetryOn` and change retry behavior for subsequent requests, and concurrent callers can race on that shared slice. Cloning `RetryOn` in `GetOperationMetadata` before returning would keep the metadata read-only to consumers.</comment>

<file context>
@@ -19564,238 +19585,244 @@ type OperationMetadata struct {
+	MaxAttempts int
+	// RetryOn is the operation's declared retryable status set from
+	// x-basecamp-retry. Statuses outside it are never retried.
+	RetryOn []int
 }
 
</file context>

[cubic-dev-ai]

P2: An operation declaring an empty retry_on set would retry 429/503 anyway, rather than retrying no HTTP statuses. Preserve an explicit empty list; only default when the field is absent.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At python/src/basecamp/_retry_policy.py, line 71:

<comment>An operation declaring an empty `retry_on` set would retry 429/503 anyway, rather than retrying no HTTP statuses. Preserve an explicit empty list; only default when the field is absent.</comment>

<file context>
@@ -0,0 +1,99 @@
+    retry_on = retry.get("retry_on")
+    return RetryPolicy(
+        max_attempts=max_attempts if isinstance(max_attempts, int) and max_attempts > 0 else DEFAULT_MAX_ATTEMPTS,
+        retry_on=frozenset(retry_on) if retry_on else DEFAULT_RETRY_ON,
+    )
+
</file context>

Suggested change

	retry_on=frozenset(retry_on) if retry_on else DEFAULT_RETRY_ON,
	retry_on=frozenset(retry_on) if retry_on is not None else DEFAULT_RETRY_ON,

[jeremy]

Correction to the PR description: I stated make check passes. It did not — py-check failed on ruff format --check for _http.py and tests/test_http.py. My verification command ended in ; echo "EXIT=$?", so the exit code I read was the echo's, not make's.

Fixed by running ruff format and amending the commit; the amended commit is the formatted one. Re-verifying the full suite now and will report the actual result.

[jeremy]

Re-verified after the formatting fix: make check exits 0 (real exit code, checked properly this time).

[github-actions]

⚠️ Potential breaking changes detected:

• Change in retry behavior due to honoring declared retry_on statuses (429, 503) and removing retrying for statuses (500, 502, 504), which alters the retry contract.
• Modification of the retry behavior may affect clients relying on previous retryable statuses, potentially leading to unexpected behavior and failures.

Review carefully before merging. Consider a major version bump.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8b117ca8c0

ℹ️ 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".

[chatgpt-codex-connector]

Run retry parity from a CI job

When generated retry metadata drifts, this prerequisite only catches it for developers who manually run the root make check: I checked .github/workflows/test.yml and the other workflow files, and none invokes make check or check-retry-parity; the Go job explicitly runs only scripts/check-idempotency-parity at lines 114–118. As a result, the new cross-SDK parity guard can remain green locally yet never execute on pull requests, allowing precisely the stale metadata this change is intended to prevent to merge.

Useful? React with 👍 / 👎.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated no new comments.

[jeremy]

Restacked. #483 merged while this was open, so the whole chain moved: #484 is now 1 commit on the post-#483 main (2 files, was 55 when it was sitting on the superseded 7a4c3297), and this PR is 1 commit on #484 — 8 files.

The PR body has also been rewritten. It described my first implementation, not what is in the branch: it claimed OperationMetadata.MaxAttempts, _retry_policy.effective_attempts, and a constant read-path gate guarded by a 118-operation invariant. None of those exist here — after rebasing onto #483 I dropped all of that in favour of #483's operation-id threading, and the shipped code uses sibling operationRetryMax/operationRetryOn maps plus a _is_retryable_error(error, operation) gate. Body now matches the diff.

The Gate 3 consumption table in SPEC.md is also corrected: only Kotlin turns the ceiling into a floor. TypeScript and Swift expose no numeric caller cap at all, so there is nothing for the operation value to override — I had lumped them in. Ruby is the mirror image. #485 is rescoped to Kotlin + Ruby, and #483's rationale paragraph was corrected before it merged (the corrected text is in main now).

make check exits 0.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

go/templates/client.tmpl:665

• GetOperationRetryOn returns the underlying slice stored in operationRetryOn. Because slices are mutable and share backing arrays, external callers can accidentally (or intentionally) modify the returned slice and thereby mutate the client's retry policy at runtime. Return a defensive copy to keep operationRetryOn effectively immutable from outside the package.

// GetOperationRetryOn returns the declared retryable status set for the given
// operation ID. ok is false when the operation carries no per-op set.
func GetOperationRetryOn(operationId string) ([]int, bool) {
	r, ok := operationRetryOn[operationId]
	return r, ok
}

python/tests/test_http.py:127

• This test documents that the no-operation GET path "still retries 500", but it doesn't assert the call count. Adding an explicit route.call_count assertion makes the intended retry contract precise.

        # Generated reads always pass an operation; see
        # TestDeclaredRetryStatuses for the governed path.
        route = respx.get("https://3.basecampapi.com/test")
        route.side_effect = [
            httpx.Response(500),

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a9c09410dc

ℹ️ 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".

[chatgpt-codex-connector]

Update the earlier retry-field consumption description

These new rows correctly say Go and Python consume the per-operation status set, but the §2 configuration discussion at SPEC.md:115 still says both SDKs consume only max and that retry_on remains inert. After this change those statements directly contradict each other, so readers using the specification to determine which metadata fields affect transport behavior receive the wrong contract; update the earlier paragraph to describe max + retry_on consistently.

Useful? React with 👍 / 👎.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

go/templates/client.tmpl:669

• GetOperationRetryOn returns the underlying slice stored in the internal operationRetryOn table. Because slices are mutable, callers can inadvertently (or concurrently) modify the SDK’s retry policy globally, which can also introduce data races if the client is used across goroutines. Return a defensive copy of the slice instead.

// GetOperationRetryOn returns the declared retryable status set for the given
// operation ID. ok is false when the operation carries no per-op set.
func GetOperationRetryOn(operationId string) ([]int, bool) {
	r, ok := operationRetryOn[operationId]
	return r, ok
}

[Copilot]

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

go/templates/client.tmpl:864

• GetOperationRetryOn returns the underlying []int stored in the global operationRetryOn map. Because this accessor is exported, callers can mutate the returned slice and silently change the SDK’s retry gate for all future requests (the slice shares the same backing array). Return a defensive copy instead.

// GetOperationRetryOn returns the declared retryable status set for the given
// operation ID. ok is false when the operation carries no per-op set.
func GetOperationRetryOn(operationId string) ([]int, bool) {
	r, ok := operationRetryOn[operationId]
	return r, ok
}

go/templates/client.tmpl:851

• The template only emits retryOn when index $retry "retryOn" is truthy. In Go templates, an explicitly empty list is falsey, so a declared retryOn: [] would be treated as absent and the runtime would incorrectly fall back to defaultRetryOn. Since isRetryableStatus explicitly distinguishes “absent” vs “present-but-empty”, the generator should preserve an empty list when the key exists (i.e., test for nil, not truthiness).

This issue also appears on line 859 of the same file.

{{if index $retry "retryOn" -}}
{{range $i, $status := index $retry "retryOn" -}}
{{if $i}}{{$retryOn = printf "%s, %v" $retryOn $status}}{{else}}{{$retryOn = printf "%v" $status}}{{end -}}
{{end -}}
{{end -}}

[Copilot]

Pull request overview

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.