kt drift gate: land the relative-path Gradle --args fix (post-#460 hotfix) - #476
Merged
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes the Kotlin regenerate-and-diff drift gate script to avoid Gradle --args path re-tokenization issues by passing only stable relative paths, and to restore the intended exit-code contract (0 = clean, 1 = drift, 2 = cannot run). It aligns the drift check behavior with make kt-generate-services and makes failures diagnosable while keeping the check non-mutating.
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:
- Regenerate into a unique gitignored
kotlin/build/kt-drift-check.XXXXXXdirectory and pass only relative generator paths (../openapi.json,../behavior-model.json,build/...). - Capture Gradle/generator output for failures and map any generator/Gradle failure to exit code
2(cannot run), preservingdiff-based drift as exit1.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 020ce1e0ee
ℹ️ 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".
jeremy
force-pushed
the
fix/kt-drift-relpath-args
branch
from
July 27, 2026 22:32
020ce1e to
a23a794
Compare
jeremy
force-pushed
the
fix/kt-drift-relpath-args
branch
from
July 27, 2026 22:36
a23a794 to
081455a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 081455a5c8
ℹ️ 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".
Follow-up to #460. The final commit on #460 described switching check-kotlin-generated-drift.sh to clean relative Gradle --args paths, but a staging slip left the change out of the merged commit — main still carries the earlier double-quote-wrapping approach, which mis-parses a path containing a literal double quote (the case flagged in review). Land the intended fix: generate into a unique gitignored kotlin/build subdir and pass only fixed RELATIVE paths (../openapi.json, ../behavior-model.json, build/kt-drift-check.XXXXXX), exactly like `make kt-generate-services`. This sidesteps Gradle's --args string tokenizer entirely — no repo/TMPDIR path character (space, apostrophe, double quote, backslash) ever appears in the argument string, so no quoting is needed and no path can be mis-split, whatever the checkout location. Writing under build/ (gitignored) keeps the check non-mutating w.r.t. the committed tree; the temp subdir is removed on exit. Also map any "cannot run" condition — temp-dir setup failure or a generator/Gradle failure — to exit 2 instead of letting the default exit 1 masquerade as drift, preserving the documented contract (0 = clean, 1 = drift, 2 = cannot run). Verified: shellcheck clean; normal → exit 0; a TMPDIR containing a space, apostrophe, AND double quote (now unused) → exit 0; extra-file drift and a missing committed tree → exit 1; a broken Java home that makes Gradle fail → exit 2; a missing JVM toolchain → exit 2; no tracked-file changes and no leftover build subdir.
jeremy
force-pushed
the
fix/kt-drift-relpath-args
branch
from
July 27, 2026 22:42
081455a to
74957f5
Compare
This was referenced Jul 28, 2026
jeremy
added a commit
that referenced
this pull request
Jul 28, 2026
The generated client's doWithRetry rebuilds each attempt's request via
buildRequest(), which recreates it from the caller's single-use io.Reader.
The raw *WithBody(..., body io.Reader) methods therefore shipped an empty
(or, after a partial-read network failure, mid-stream) body on every retry
attempt — the 42 idempotent raw *WithBody ops (Update*/Reposition*/Set*/
Replace*/Toggle*/MarkAsRead) reaching BC3 over PUT/DELETE all retry, so a
transient 429/503/network failure could resend a truncated body.
doWithRetry now snapshots the first finalized request's body exactly once
(after buildRequest + applyEditors) and replays THAT body on every later
attempt, under a documented, net/http-consistent replay contract:
- GetBody present -> GetBody is AUTHORITATIVE. This attempt is normalized to
send GetBody()'s bytes too, so attempt 1 and every retry ship byte-identical
content even if an editor mutated req.Body out of sync with GetBody
(including an in-place strings.Reader.Reset, which no identity check can
detect). A retrying request's body is changed by an editor only through
req.GetBody (set req.Body and req.GetBody together, as net/http requires) —
an editor rewriting req.Body alone is ignored for the body, exactly as it
would be on a 307/308 redirect. A large body with a valid GetBody stays
fully replayable (no buffering, no demotion).
- No GetBody, retry-eligible -> buffer up to maxReplayBodyBytes (1 MiB); the
buffered (finalized) body is what all attempts send, so here an editor's
req.Body change IS honored (it is the only body available). Over the cap ->
single attempt with a composite prefix+remainder body (Close delegates to
the original), streaming preserved.
- Retry-ineligible / no body -> streaming untouched, nothing buffered.
ContentLength is restored per replay path so a known-length body never
regresses to chunked encoding.
Fix lives in go/templates/client.tmpl (regenerated). Tests live outside
pkg/generated per the repo rule: eight red proofs (full-body replay incl.
retry GetBody, partial-read byte-0 restart, over-cap single attempt, retry
ContentLength, no-GetBody editor body buffered+prior-closed, editor req.Body
without GetBody ignored, in-place editor Reset stays consistent, editor
setting Body+GetBody not demoted) and two controls (happy path,
retry-ineligible buffers nothing).
Follows the retry-contract program (#456/#460/#461/#476).
jeremy
added a commit
that referenced
this pull request
Jul 28, 2026
SPEC §6 prescribes 400 -> validation to match the other five SDKs, but Go's checkResponse had no 400 case, so it fell through to the default arm and mislabeled 400 Bad Request as api_error. Add the missing case (code=validation, http_status=400, retryable=false). This resolves the [CONFLICT]/[static] carve-outs the SPEC carried for 400: mark the error-code table row, the mapping-algorithm step, and the non-retryable-errors note as [conformance]-verified, and drop the stale "Go currently maps 400 to api_error" note. Adds a 400 case to the shared error-mapping conformance fixture (verified green across all five runners: Go, Kotlin, TypeScript, Ruby, Python) and a 400 row to helpers_test.go (proven to fail pre-fix: api_error != validation). retryable stays false — this is a code/message correction, not a retry-behavior change. Follows the retry-contract program (#456/#460/#461/#476).
jeremy
added a commit
that referenced
this pull request
Jul 28, 2026
…uard
A4b2 — generated Go and Python drove their retry loops from the client-wide
cap only, ignoring the per-operation retry.max that TS/Swift/Kotlin already
honor (Python shipped it in metadata.json as dead data; Go emitted no per-op
table). Wire the per-op value in as a ceiling (upper bound on attempts):
effective_attempts = min(client_cap, op_max)
Precedence rationale: the per-op field is named `max`, so it composes with the
client cap by taking the smaller. TS/Swift/Kotlin expose no numeric client cap
and use op_max directly; because every op's max is <= the default cap of 3,
min() makes a default-or-raised Go/Python client produce exactly the per-op
attempt count (parity), while uniquely honoring a Go/Python client that lowered
its cap to 0/1 to disable/restrict retries. Observable change: a default client
sees only the 9 idempotent max:2 ops drop 3->2; a client that RAISED its cap
above 3 now sees all 187 retry-eligible ops clamped to their per-op max
(release-noted in SPEC).
- Go: emit OperationMetadata.RetryMax from x-basecamp-retry.maxAttempts
(client.tmpl) and apply the ceiling in doWithRetry by operationId.
- Python: thread the CANONICAL (PascalCase) operationId into
_request_with_retry (sync + async) for every retryable path. Crucially the
generated GET/list/pagination methods now pass operation="<OpId>" too —
previously they passed nothing or OperationInfo.operation (snake_case, not a
metadata key), so the ceiling silently never applied to the 118 GET ops.
generate_services.py now emits the operation= kwarg on GET/list/paginated
calls; _base/_async_base thread it through _request_list and all three
pagination wrappers.
A4c — add scripts/check-retry-metadata-parity.py, wired into `make check`.
Criterion 1 (static): emitted per-op retry values EQUAL behavior-model.json
across all six emitters (Python/Ruby/TS/Kotlin/Swift full tuple; Go max only —
a documented design deviation: Go consumes only max, so emitting the full inert
tuple into a Go struct would be dead data). Criterion 2 (token-smoke
classification, NOT behavioral proof): usage-form token presence/absence per
consuming source classifies TS/Swift/Kotlin=full tuple, Go/Python=max only,
Ruby=none, and catches gross removal of a consumption site (behavioral proof
lives in each SDK's retry tests).
Tests (outside pkg/generated): Go ceiling test proven to fail without the
min() application; Python sync+async ceiling tests; and INTEGRATION tests
through real generated services with the real bundled metadata.json
(projects.list -> ListProjects max:3; account.update_account_name ->
UpdateAccountName max:2), proven to fail against the snake_case-key bug (5
attempts vs 3). Parity guard self-tested against mutated values.
Follows the retry-contract program (#456/#460/#461/#476).
jeremy
added a commit
that referenced
this pull request
Jul 28, 2026
The generated client's doWithRetry rebuilds each attempt's request via
buildRequest(), which recreates it from the caller's single-use io.Reader.
The raw *WithBody(..., body io.Reader) methods therefore shipped an empty
(or, after a partial-read network failure, mid-stream) body on every retry
attempt — the 42 idempotent raw *WithBody ops (Update*/Reposition*/Set*/
Replace*/Toggle*/MarkAsRead) reaching BC3 over PUT/DELETE all retry, so a
transient 429/503/network failure could resend a truncated body.
doWithRetry now snapshots the first finalized request's body exactly once
(after buildRequest + applyEditors) and replays THAT body on every later
attempt, under a documented, net/http-consistent replay contract:
- Retry-ineligible (single attempt) -> stream whatever body the editors
finalized, untouched (no GetBody normalization, no buffering). Checked
first, so a non-retrying call never rewrites an editor's req.Body.
- GetBody present (retry-eligible) -> GetBody is AUTHORITATIVE. This attempt
is normalized to send GetBody()'s bytes too, so attempt 1 and every retry
ship byte-identical content even if an editor mutated req.Body out of sync
with GetBody (including an in-place strings.Reader.Reset). A large body
with a valid GetBody stays fully replayable (no demotion).
- No GetBody, retry-eligible -> buffer up to maxReplayBodyBytes (1 MiB). The
read honors ctx (via readAllContext), so a blocking body (e.g. an io.Pipe)
cannot hang past the request's deadline. Over the cap -> single attempt
with a composite prefix+remainder body.
On a retry the captured body is installed BEFORE the editors run (and
re-installed after), so a body-aware editor (Digest/HMAC) signs the bytes that
are actually sent. ContentLength is restored per replay path.
Fix lives in go/templates/client.tmpl (regenerated). Tests live outside
pkg/generated per the repo rule (13 total): 9 fail against the shipped
d560496 client (empty/truncated retry bodies, wrong ContentLength, editor-
body inconsistency, digest mismatch, uncancellable buffering); 1 regression
proof (a valid large GetBody body must not be demoted, guarding an earlier
iteration); 3 controls (happy path, retry-ineligible streams nothing / honors
the editor body). Each proof is individually red-proofed.
Behavior note: for retry-eligible *WithBody ops, attempt 1 now sends GetBody()'s
bytes (identical for correct callers); a request editor changes a retrying
request's body only via req.GetBody. No API signature changes.
Follows the retry-contract program (#456/#460/#461/#476).
jeremy
added a commit
that referenced
this pull request
Jul 28, 2026
…uard
A4b2 — generated Go and Python drove their retry loops from the client-wide
cap only, ignoring the per-operation retry.max that TS/Swift/Kotlin already
honor (Python shipped it in metadata.json as dead data; Go emitted no per-op
table). Wire the per-op value in as a ceiling (upper bound on attempts):
effective_attempts = min(client_cap, op_max)
Precedence rationale: the per-op field is named `max`, so it composes with the
client cap by taking the smaller. TS/Swift/Kotlin expose no numeric client cap
and use op_max directly; because every op's max is <= the default cap of 3,
min() makes a default-or-raised Go/Python client produce exactly the per-op
attempt count (parity), while uniquely honoring a Go/Python client that lowered
its cap to 0/1 to disable/restrict retries. Observable change: a default client
sees only the 9 idempotent max:2 ops drop 3->2; a client that RAISED its cap
above 3 now sees all 187 retry-eligible ops clamped to their per-op max
(release-noted in SPEC).
- Go: emit OperationMetadata.RetryMax from x-basecamp-retry.maxAttempts
(client.tmpl) and apply the ceiling in doWithRetry by operationId.
- Python: thread the CANONICAL (PascalCase) operationId into
_request_with_retry (sync + async) for every retryable path. Crucially the
generated GET/list/pagination methods now pass operation="<OpId>" too —
previously they passed nothing or OperationInfo.operation (snake_case, not a
metadata key), so the ceiling silently never applied to the 118 GET ops.
generate_services.py now emits the operation= kwarg on GET/list/paginated
calls; _base/_async_base thread it through _request_list and all three
pagination wrappers.
A4c — add scripts/check-retry-metadata-parity.py, wired into `make check`.
Criterion 1 (static): emitted per-op retry values EQUAL behavior-model.json
across all six emitters (Python/Ruby/TS/Kotlin/Swift full tuple; Go max only —
a documented design deviation: Go consumes only max, so emitting the full inert
tuple into a Go struct would be dead data). Criterion 2 (token-smoke
classification, NOT behavioral proof): usage-form token presence/absence per
consuming source classifies TS/Swift/Kotlin=full tuple, Go/Python=max only,
Ruby=none, and catches gross removal of a consumption site (behavioral proof
lives in each SDK's retry tests).
Tests (outside pkg/generated): Go ceiling test proven to fail without the
min() application; Python sync+async ceiling tests; and INTEGRATION tests
through real generated services with the real bundled metadata.json
(projects.list -> ListProjects max:3; account.update_account_name ->
UpdateAccountName max:2), proven to fail against the snake_case-key bug (5
attempts vs 3). Parity guard self-tested against mutated values.
Follows the retry-contract program (#456/#460/#461/#476).
jeremy
added a commit
that referenced
this pull request
Jul 28, 2026
The generated client's doWithRetry rebuilds each attempt's request via
buildRequest(), which recreates it from the caller's single-use io.Reader.
The raw *WithBody(..., body io.Reader) methods therefore shipped an empty
(or, after a partial-read network failure, mid-stream) body on every retry
attempt — the 42 idempotent raw *WithBody ops (Update*/Reposition*/Set*/
Replace*/Toggle*/MarkAsRead) reaching BC3 over PUT/DELETE all retry, so a
transient 429/503/network failure could resend a truncated body.
doWithRetry now snapshots the first finalized request's body once (after
buildRequest + applyEditors) and replays THAT body on every later attempt,
under a documented, net/http-consistent contract:
- Retry-ineligible (single attempt) -> stream the finalized body untouched
(no normalization). Checked first, so a non-retrying call never rewrites an
editor's req.Body.
- GetBody present (retry-eligible) -> GetBody is AUTHORITATIVE. This attempt
is normalized to send GetBody()'s bytes too, so attempt 1 and every retry
ship byte-identical content even if an editor mutated req.Body out of sync
with GetBody (including an in-place strings.Reader.Reset). A large body with
a valid GetBody stays fully replayable.
- No GetBody -> an unrewindable, single-use stream: NOT buffered or retried.
Buffering would risk unbounded memory, and a blocking reader cannot be
interrupted (there is no general way to abort an io.Reader), so the request
is sent exactly once. In-memory bodies net/http can snapshot
(*bytes.Reader/*bytes.Buffer/*strings.Reader — what the typed and generated
builders use) carry a GetBody and replay normally; only a caller passing a
genuinely streaming reader to a raw *WithBody method forgoes retries, which
is the safe behavior for an unrewindable stream.
On a retry the captured body is installed BEFORE the editors run (and
re-installed after), so a body-aware editor (Digest/HMAC) signs the bytes that
are actually sent, and an editor-installed retry body is discarded and closed.
ContentLength is restored per replay path.
Fix lives in go/templates/client.tmpl (regenerated). Tests live outside
pkg/generated per the repo rule (12 total): 8 fail against the shipped
d560496 client (empty/truncated retry bodies, no-GetBody stream retried with
a broken body, wrong ContentLength, editor-body inconsistency, digest
mismatch); 1 regression proof (a valid large GetBody body must not be demoted);
3 controls (happy path, retry-ineligible streams nothing / honors the editor
body). Each proof is individually red-proofed.
Behavior note: for retry-eligible *WithBody ops, attempt 1 now sends GetBody()'s
bytes (identical for correct callers), and a caller passing a non-snapshotable
streaming reader to a raw *WithBody method is sent once (no retry). A request
editor changes a retrying request's body only via req.GetBody. No API signature
changes.
Follows the retry-contract program (#456/#460/#461/#476).
jeremy
added a commit
that referenced
this pull request
Jul 28, 2026
…uard
A4b2 — generated Go and Python drove their retry loops from the client-wide
cap only, ignoring the per-operation retry.max that TS/Swift/Kotlin already
honor (Python shipped it in metadata.json as dead data; Go emitted no per-op
table). Wire the per-op value in as a ceiling (upper bound on attempts):
effective_attempts = min(client_cap, op_max)
Precedence rationale: the per-op field is named `max`, so it composes with the
client cap by taking the smaller. TS/Swift/Kotlin expose no numeric client cap
and use op_max directly; because every op's max is <= the default cap of 3,
min() makes a default-or-raised Go/Python client produce exactly the per-op
attempt count (parity), while uniquely honoring a Go/Python client that lowered
its cap to 0/1 to disable/restrict retries. Observable change: a default client
sees only the 9 idempotent max:2 ops drop 3->2; a client that RAISED its cap
above 3 now sees all 187 retry-eligible ops clamped to their per-op max
(release-noted in SPEC).
- Go: emit OperationMetadata.RetryMax from x-basecamp-retry.maxAttempts
(client.tmpl) and apply the ceiling in doWithRetry by operationId.
- Python: thread the CANONICAL (PascalCase) operationId into
_request_with_retry (sync + async) for every retryable path. Crucially the
generated GET/list/pagination methods now pass operation="<OpId>" too —
previously they passed nothing or OperationInfo.operation (snake_case, not a
metadata key), so the ceiling silently never applied to the 118 GET ops.
generate_services.py now emits the operation= kwarg on GET/list/paginated
calls; _base/_async_base thread it through _request_list and all three
pagination wrappers.
A4c — add scripts/check-retry-metadata-parity.py, wired into `make check`.
Criterion 1 (static): emitted per-op retry values EQUAL behavior-model.json
across all six emitters (Python/Ruby/TS/Kotlin/Swift full tuple; Go max only —
a documented design deviation: Go consumes only max, so emitting the full inert
tuple into a Go struct would be dead data). Criterion 2 (token-smoke
classification, NOT behavioral proof): usage-form token presence/absence per
consuming source classifies TS/Swift/Kotlin=full tuple, Go/Python=max only,
Ruby=none, and catches gross removal of a consumption site (behavioral proof
lives in each SDK's retry tests).
Tests (outside pkg/generated): Go ceiling test proven to fail without the
min() application; Python sync+async ceiling tests; and INTEGRATION tests
through real generated services with the real bundled metadata.json
(projects.list -> ListProjects max:3; account.update_account_name ->
UpdateAccountName max:2), proven to fail against the snake_case-key bug (5
attempts vs 3). Parity guard self-tested against mutated values.
Follows the retry-contract program (#456/#460/#461/#476).
jeremy
added a commit
that referenced
this pull request
Jul 28, 2026
The generated client's doWithRetry rebuilds each attempt's request via
buildRequest(), which recreates it from the caller's single-use io.Reader.
The raw *WithBody(..., body io.Reader) methods therefore shipped an empty
(or, after a partial-read network failure, mid-stream) body on every retry
attempt — the 42 idempotent raw *WithBody ops (Update*/Reposition*/Set*/
Replace*/Toggle*/MarkAsRead) reaching BC3 over PUT/DELETE all retry, so a
transient 429/503/network failure could resend a truncated body.
doWithRetry now snapshots the first finalized request's body once (after
buildRequest + applyEditors) and replays THAT body on every later attempt,
under a documented, net/http-consistent contract:
- Retry-ineligible (single attempt) -> stream the finalized body untouched
(no normalization). Checked first, so a non-retrying call never rewrites an
editor's req.Body.
- GetBody present (retry-eligible) -> GetBody is AUTHORITATIVE. This attempt
is normalized to send GetBody()'s bytes too, so attempt 1 and every retry
ship byte-identical content even if an editor mutated req.Body out of sync
with GetBody (including an in-place strings.Reader.Reset). A large body with
a valid GetBody stays fully replayable.
- No GetBody -> an unrewindable, single-use stream: NOT buffered or retried.
Buffering would risk unbounded memory, and a blocking reader cannot be
interrupted (there is no general way to abort an io.Reader), so the request
is sent exactly once. In-memory bodies net/http can snapshot
(*bytes.Reader/*bytes.Buffer/*strings.Reader — what the typed and generated
builders use) carry a GetBody and replay normally; only a caller passing a
genuinely streaming reader to a raw *WithBody method forgoes retries, which
is the safe behavior for an unrewindable stream.
On a retry the captured body is installed BEFORE the editors run (and
re-installed after), so a body-aware editor (Digest/HMAC) signs the bytes that
are actually sent, and an editor-installed retry body is discarded and closed.
ContentLength is restored per replay path.
Fix lives in go/templates/client.tmpl (regenerated). Tests live outside
pkg/generated per the repo rule (14 total): 9 fail against the shipped
d560496 client (empty/truncated retry bodies, no-GetBody stream retried with a
broken body, wrong ContentLength, editor-body inconsistency, digest mismatch,
leaked replay body on editor error); 2 regression proofs (a valid large GetBody
body must not be demoted; a GetBody failure must not fail the request); 3
controls. Each proof is individually red-proofed.
Behavior note: for retry-eligible *WithBody ops, attempt 1 now sends GetBody()'s
bytes (identical for correct callers), and a caller passing a non-snapshotable
streaming reader to a raw *WithBody method is sent once (no retry). A request
editor changes a retrying request's body only via req.GetBody. No API signature
changes.
Follows the retry-contract program (#456/#460/#461/#476).
jeremy
added a commit
that referenced
this pull request
Jul 28, 2026
The generated client's doWithRetry rebuilds each attempt's request via
buildRequest(), which recreates it from the caller's single-use io.Reader.
The raw *WithBody(..., body io.Reader) methods therefore shipped an empty
(or, after a partial-read network failure, mid-stream) body on every retry
attempt — the 42 idempotent raw *WithBody ops (Update*/Reposition*/Set*/
Replace*/Toggle*/MarkAsRead) reaching BC3 over PUT/DELETE all retry, so a
transient 429/503/network failure could resend a truncated body.
doWithRetry now snapshots the first finalized request's body once (after
buildRequest + applyEditors) and replays THAT body on every later attempt,
under a documented, net/http-consistent contract:
- Retry-ineligible (single attempt) -> stream the finalized body untouched.
- GetBody present (retry-eligible) -> GetBody is AUTHORITATIVE. This attempt
is normalized to send GetBody()'s bytes too, so attempt 1 and every retry
ship byte-identical content even if an editor mutated req.Body out of sync
with GetBody (including an in-place strings.Reader.Reset). A large body with
a valid GetBody stays fully replayable.
- No GetBody -> an unrewindable, single-use stream: NOT buffered or retried.
Buffering would risk unbounded memory, and a blocking reader cannot be
interrupted (no general way to abort an io.Reader), so it is sent once.
Because that policy keys retry off GetBody, the SDK's own serialized bodies
must be snapshotable. marshalBody (and the multipart account-logo path) now
return *bytes.Reader instead of the old custom rewindableReader (which had no
GetBody), so the eight hand-written idempotent update methods
(Todos/Cards/CardSteps/Checkins/Projects/People/Schedules.Update, etc.) — and
UpdateAccountLogo — replay their bodies and keep retrying on transient
failures. rewindableReader is removed. Only an external caller passing a
genuinely streaming, non-snapshotable reader to a raw *WithBody method forgoes
retries, which is the safe behavior for an unrewindable stream.
On a retry the captured body is installed BEFORE the editors run (and
re-installed after), so a body-aware editor (Digest/HMAC) signs the bytes that
are actually sent, and an editor-installed retry body is discarded and closed.
A GetBody snapshot failure disables retries (single attempt with the valid
body) rather than failing an otherwise-sendable request, and an installed
replay body is closed if a retry editor errors. ContentLength is restored per
replay path.
Fix lives in go/templates/client.tmpl (regenerated) plus helpers.go's
marshalBody. Tests: 14 raw-*WithBody replay tests outside pkg/generated (9 fail
against the shipped d560496 client, 2 regression proofs, 3 controls, each
red-proofed); a PUBLIC-service retry proof (ProjectsService.Update retries a
503 and replays the full body — red-proofed against a non-snapshotable body);
and a marshalBody snapshotability test. Go conformance's naturally-idempotent
PUT/DELETE cases pass.
No API signature changes. A request editor changes a retrying request's body
only via req.GetBody.
Follows the retry-contract program (#456/#460/#461/#476).
jeremy
added a commit
that referenced
this pull request
Jul 28, 2026
The generated client's doWithRetry rebuilds each attempt's request via
buildRequest(), which recreates it from the caller's single-use io.Reader.
The raw *WithBody(..., body io.Reader) methods therefore shipped an empty
(or, after a partial-read network failure, mid-stream) body on every retry
attempt — the 42 idempotent raw *WithBody ops (Update*/Reposition*/Set*/
Replace*/Toggle*/MarkAsRead) reaching BC3 over PUT/DELETE all retry, so a
transient 429/503/network failure could resend a truncated body.
doWithRetry now snapshots the first finalized request's body once (after
buildRequest + applyEditors) and replays THAT body on every later attempt,
under a documented, net/http-consistent contract:
- Retry-ineligible (single attempt) -> stream the finalized body untouched.
- GetBody present (retry-eligible) -> GetBody is AUTHORITATIVE. This attempt
is normalized to send GetBody()'s bytes too, so attempt 1 and every retry
ship byte-identical content even if an editor mutated req.Body out of sync
with GetBody (including an in-place strings.Reader.Reset). A large body with
a valid GetBody stays fully replayable.
- No GetBody -> an unrewindable, single-use stream: NOT buffered or retried.
Buffering would risk unbounded memory, and a blocking reader cannot be
interrupted (no general way to abort an io.Reader), so it is sent once.
Because that policy keys retry off GetBody, the SDK's own serialized bodies
must be snapshotable. marshalBody (and the multipart account-logo path) now
return *bytes.Reader instead of the old custom rewindableReader (which had no
GetBody), so the eight hand-written idempotent update methods
(Todos/Cards/CardSteps/Checkins/Projects/People/Schedules.Update, etc.) — and
UpdateAccountLogo — replay their bodies and keep retrying on transient
failures. rewindableReader is removed. Only an external caller passing a
genuinely streaming, non-snapshotable reader to a raw *WithBody method forgoes
retries, which is the safe behavior for an unrewindable stream.
On a retry the captured body is installed BEFORE the editors run (and
re-installed after), so a body-aware editor (Digest/HMAC) signs the bytes that
are actually sent, and an editor-installed retry body is discarded and closed.
A GetBody snapshot failure disables retries (single attempt with the valid
body) rather than failing an otherwise-sendable request; an installed replay
body is closed if a retry editor errors; and an explicitly empty body
(http.NoBody) is replayed as a non-nil empty body, not collapsed to nil. ContentLength is restored per
replay path.
Fix lives in go/templates/client.tmpl (regenerated) plus helpers.go's
marshalBody. Tests: 15 raw-*WithBody replay tests outside pkg/generated (9 fail against
the shipped d560496 client, 3 regression proofs, 3 controls, each red-proofed); a PUBLIC-service retry proof (ProjectsService.Update retries a
503 and replays the full body — red-proofed against a non-snapshotable body);
and a marshalBody snapshotability test. Go conformance's naturally-idempotent
PUT/DELETE cases pass.
No API signature changes. A request editor changes a retrying request's body
only via req.GetBody.
Follows the retry-contract program (#456/#460/#461/#476).
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.
Post-merge hotfix for #460.
What went wrong
#460's final commit message described switching
scripts/check-kotlin-generated-drift.shto clean relative Gradle--argspaths (the fix for the literal-double-quote review finding), but a staging slip meant the script change was nevergit added before the amend. So #460 merged (c541afe9) with the earlier double-quote-wrapping version — the merged code and its own commit message (and the resolved review reply) disagree. This PR lands the intended fix.The fix
kotlin/build/subdir and pass only fixed relative paths to the generator (../openapi.json,../behavior-model.json,build/kt-drift-check.XXXXXX), exactly likemake kt-generate-services. This sidesteps Gradle's--argsstring tokenizer entirely: no repo/TMPDIRpath character (space, apostrophe, double quote, backslash) ever appears in the argument string, so no quoting is needed and no path can be mis-split — whatever the checkout location. Writing underbuild/(gitignored) keeps the check non-mutating; the temp subdir is removed on exit.0 = clean, 1 = drift, 2 = cannot run.Verification
TMPDIRcontaining a space, apostrophe, and double quote (now unused) → exit 0build/subdirNote: this PR's
GitHub Actions auditshows the same exogenous zizmorref-version-mismatchfailure carried by current main's #463 seed (release-go.ymlcomment# v6.9.9for a v7.0.0 SHA) — unrelated to this diff; repaired by #466.Summary by cubic
Fixes the Kotlin drift check by regenerating into
kotlin/buildand passing only relative Gradle--args, removing path tokenization issues and restoring exit codes (0 clean, 1 drift, 2 cannot run). Follow-up to #460.kotlin/build/kt-drift-check.XXXXXXand pass only relative paths (../openapi.json,../behavior-model.json,build/...) fromkotlin/to Gradle--args.--argsre-tokenization; spaces, quotes, and backslashes no longer break paths. Committed tree stays untouched; temp dir and log are removed on exit. Build-dir/temp-dir creation errors map to exit 2.Written for commit 74957f5. Summary will update on new commits.