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

[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]

All reported issues were addressed 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.}

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

Re-trigger cubic</sub>

[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".

[Copilot]

Pull request overview

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

[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".

[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.

[jeremy]

Audited all nine unresolved threads against the merged tree. Six distinct claims; two shipped, four did not.

Claim	At merge 21a657b2	Verdict
Go accessor aliases the package global	present	real — shipped in v0.10.0
Parity guard absent from CI	present	real — enforcement gap
Explicit retry_on: [] defaults to 429/503	fixed	did not ship
SPEC §2 contradicts the Gate-3 table	fixed	did not ship
Python classification vetoes declared statuses	fixed	did not ship
Ruby guard encoding	files absent	intermediate-only, moot

Both real findings are fixed in #500, with a regression test that fails against the unfixed accessor (a second lookup returns [599 599], want [429 503]) and passes under -race. The four settled threads are resolved with final-head evidence; the four tracking the two real findings stay open until #500 merges.

Why nine. The branch was force-pushed 7 times over 16 hours and bots reviewed 8 distinct SHAs. Each force-push orphans a thread's anchor commit, so GitHub marks it outdated but never resolved, and the bots refile the same finding against the new head — the slice-aliasing point alone appears three times from three reviewers. Two files that drew comments (check-retry-parity.rb, _retry_policy.py) were replaced mid-PR and never merged at all.

Two things worth carrying forward:

isOutdated is unreliable in both directions. Two non-outdated threads described code that had already been corrected, while the one thread that was genuinely still defective — Copilot's, on the exact final head — sat untriaged. Staleness of the anchor says nothing about truth of the claim.

The merge window is the actual process failure. Copilot reviewed 904f38b0 at 22:46:36 and the PR merged at 22:53:17 — six minutes and 41 seconds, no triage. That review was correct.

Separately, and out of scope for #500: python/README.md:452 still advertises the pre-#486 contract ("retryable ApiError (500, 502, 503, 504)") when the merged gate is the declared {429, 503} set. This PR updated SPEC.md but left the Python README behind.