Retry: hand out a copy of the declared status set, and run the parity guard in CI

[jeremy]

Two tails from #486. It merged with nine of ten review threads unresolved; an audit of all of them found exactly two real defects, and these are they. Neither is user-visible today.

1. GetOperationRetryOn handed out the shared table

go/pkg/generated/client.gen.go returned the slice stored in the package-level operationRetryOn map, and isRetryableStatus reads that same table on every response. Any caller who inspected the declared set could rewrite the retry policy for every client in the process, and race with in-flight requests while doing it.

Reproduced from an external module before fixing — an always-429 server:

before mutation: server saw 3 requests
after  set[0], set[1] = 599, 599
after  mutation: server saw 1 requests

It is the only exported accessor in the generated package that returns a reference type from a package-level table — GetOperationRetryMax returns an int, GetOperationMetadata a struct of two bools. Confirmed by sweeping for exported funcs returning slices or maps: one hit.

Fixed in go/templates/client.tmpl and regenerated. append([]int(nil), r...) rather than slices.Clone because the generated import block comes from oapi-codegen's base template and this needs no new import. The early return keeps ok — not nilness — as the signal separating a declared-empty set from an absent one, which is what isRetryableStatus depends on.

The regression test is a real red proof. Against the unfixed accessor it fails with:

accessor leaked the shared slice: after mutating the first result,
a second lookup returns [599 599], want [429 503]

It also covers the behavioural half — after a caller mutates the result, a 429 must still retry — and passes under -race.

2. The parity guard never ran in CI

scripts/check-retry-metadata-parity.py was reachable only through the root check target, and no workflow invokes it. grep -rn "check-retry-metadata-parity\|make check" .github/ returned nothing. Retry metadata could therefore drift into main unnoticed — the exact failure the guard was added to prevent. Now wired into the Go job beside the idempotency guard it was modelled on, and the Makefile comment matches that sibling's "enforced in CI" convention.

Stating the residual honestly, because it is narrower than it first appears: retry_on is uniform [429, 503] across all 226 operations, so a Go template regression would be global and generated_retry_statusset_test.go already catches it in CI. What this guard uniquely covers is a generator bug in the TypeScript, Kotlin, Swift, or Ruby metadata values, plus the criterion-2 consumption token-smoke. That makes it low severity, not medium.

Verification

Check	Result
Regression test vs unfixed accessor	fails with the leak message above
go test ./pkg/basecamp/ -run TestGeneratedRetryOn -race	3/3 pass
go test ./...	all packages ok
make go-check (clean lint cache)	exit 0
make go-check-generated-drift	no drift — committed output matches the template
make check-retry-metadata-parity	exit 0, 226 operations
make lint-actions (actionlint + zizmor)	no findings

The regenerated diff is exactly the accessor — 13 insertions, 3 deletions, no incidental churn.

Deliberately not touched

• Python and SPEC.md. Four of Retry: honor the declared retry_on and max_attempts in Go and Python #486's six review claims described code that was already correct at the merge tree; three of those were fixed before the squash while their threads stayed open, and the SPEC paragraph Codex flagged was rewritten by Retry: honor the declared retry_on and max_attempts in Go and Python #486's own merge commit. Nothing to do.
• python/README.md:452 still says GETs retry on "retryable ApiError (500, 502, 503, 504)", but the merged gate is the declared {429, 503} set. That is a real stale doc introduced by Retry: honor the declared retry_on and max_attempts in Go and Python #486, but it is out of scope here and belongs in its own change.

.github/workflows/ is a control-plane path, so the sensitive-change gate will comment — informational, shadow mode.

---

Summary by cubic

Return a copy from GetOperationRetryOn to prevent global retry policy mutation and data races. Add a CI step to enforce retry metadata parity across SDKs.

• Bug Fixes

  • GetOperationRetryOn now returns a copy of the per-operation status set, preserving the ok signal to distinguish empty vs. absent.
  • Updated the Go template and regenerated the client to apply the fix.

• New Features

  • CI runs make check-retry-metadata-parity to enforce cross-SDK retry metadata parity alongside the idempotency guard.

^{Written for commit 12fcdf5. Summary will update on new commits.}

[github-actions]

Sensitive Change Detection (shadow mode)

This PR modifies control-plane files:

• .github/workflows/test.yml

Shadow mode — this check is informational only. When activated, changes to these paths will require approval from a maintainer.

[Copilot]

Pull request overview

This PR addresses two retry-metadata hardening gaps: it prevents external callers from mutating Go’s per-operation retry status tables via GetOperationRetryOn, and it ensures the existing cross-SDK retry metadata parity guard actually runs in CI.

Changes:

• Go: GetOperationRetryOn now returns a defensive copy of the declared retryable status set to prevent global policy mutation and potential races.
• Go: adds a regression test proving the accessor doesn’t leak the shared slice and that 429 retry behavior remains intact after attempted mutation.
• CI: runs make check-retry-metadata-parity in the test-go workflow job so retry metadata drift is caught on PRs.

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 4 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
Makefile	Documents the retry-metadata parity guard as CI-enforced.
go/templates/client.tmpl	Updates the generated Go template so GetOperationRetryOn returns a copied slice.
go/pkg/generated/client.gen.go	Regenerates the Go client with the safe GetOperationRetryOn implementation.
go/pkg/basecamp/generated_retry_statusset_test.go	Adds a regression test covering slice non-leakage and preserved retry behavior.
.github/workflows/test.yml	Wires the retry-metadata parity guard into the test-go CI job.

---

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