Fix checkins answer update returning 422 validation error

[robzolkos]

UpdateAnswer fails with a generic validation error on every call, regardless of content.

Ref: https://3.basecamp.com/2914079/buckets/46292715/card_tables/cards/9714931651

Docs Update: https://github.com/basecamp/bc3/pull/10075

Root cause

The BC3 Question::Answer model requires both content and group_on:

# app/models/question/answer.rb
validates_presence_of :group_on, :content

The controller rebuilds the recordable from params rather than merging with the existing record:

def new_question_answer
  Question::Answer.new params.require(:question_answer).permit(:content, :group_on)
end

The SDK was only sending content, so group_on was nil → validation failure → 422.

Fix

• UpdateAnswer now auto-fetches the existing answer to carry its group_on forward when the caller doesn't provide one
• Adds GroupOn to UpdateAnswerRequest so callers can explicitly override it
• Switches to WithBody to bypass the generated type which lacks the field
• Removes the unused createAnswerRequestWrapper and updateAnswerRequestWrapper types that were defined and tested but never wired in

---

Summary by cubic

Fixes 422 errors when updating Basecamp check-in answers by always sending the required group_on. Updates keep the existing date when not provided and accept an explicit override.

• Bug Fixes

  • Always include group_on on update: auto-fetch when absent, validate YYYY-MM-DD, and error if missing/invalid.
  • Use the WithBody path with a replayable body so content + group_on are reliably sent.
  • Update the spec to include group_on in QuestionAnswerUpdatePayload and regenerate SDKs (Go/TypeScript/Ruby/Python/Kotlin/Swift); tests verify group_on is sent when provided and omitted when not.

• Refactors

  • Remove unused create/update wrapper types and centralize the rewindable body reader in helpers via marshalBody.
  • Refresh generated metadata/route files to align with the latest spec.

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

[Copilot]

Pull request overview

Fixes CheckinsService.UpdateAnswer returning 422s by ensuring group_on is always sent (either explicitly from the caller or preserved from the existing answer), working around a generated request type that lacks group_on.

Changes:

• Adds GroupOn to UpdateAnswerRequest and updates marshaling tests.
• Updates UpdateAnswer to fetch the existing answer when GroupOn is omitted and to send a custom JSON body including group_on.
• Removes unused create/update wrapper types and replaces wrapper tests with direct JSON body tests.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
go/pkg/basecamp/checkins.go	Adds GroupOn to update requests and updates UpdateAnswer to preserve/send group_on via a custom body.
go/pkg/basecamp/checkins_test.go	Updates marshaling tests and adds service-level tests covering preserved vs explicit group_on behavior.

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.

---

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

[cubic-dev-ai]

No issues found across 2 files

[github-actions]

⚠️ Potential breaking changes detected:

• Renamed or removed createAnswerRequestWrapper struct, previously exported and wrapping CreateAnswerRequest.
• Renamed or removed updateAnswerRequestWrapper struct, previously exported and wrapping UpdateAnswerRequest.
• Changed UpdateAnswerRequest struct: added GroupOn field. This field introduction imposes new validation checks.
• Added validation for group_on in UpdateAnswer function requiring it to be non-empty and in ISO 8601 format. This changes the function's behavior and could lead to errors for inputs that were previously valid.
• Changed behavior of UpdateAnswer to require group_on parameter if not provided in the request, necessitating an additional API call to fetch it.

Review carefully before merging. Consider a major version bump.

[cubic-dev-ai]

1 issue found across 5 files (changes from recent commits).

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="go/pkg/basecamp/helpers.go">

<violation number="1" location="go/pkg/basecamp/helpers.go:54">
P1: Retries after partial body reads can resend from the middle of the payload because the reader only rewinds after EOF. Reset on request close so every retry starts at byte 0.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[jeremy]

High: Spec gap — Go-only patch, all other SDKs still broken

QuestionAnswerUpdatePayload in spec/basecamp.smithy:6254-6257 has only content. The create counterpart (QuestionAnswerPayload, line 6215-6222) correctly includes group_on. The update payload needs the same field.

Because the spec is the source of truth for codegen across all SDKs, this omission means TypeScript, Ruby, Python, Kotlin, and Swift all have the identical bug. Evidence from each SDK's generated update-answer code:

• typescript/src/generated/services/checkins.ts:183 — updateAnswer sends only content
• python/src/basecamp/generated/services/checkins.py:26 — update_answer signature has no group_on
• ruby/lib/basecamp/generated/services/checkins_service.rb:31 — update_answer(answer_id:, content:) only
• swift/Sources/Basecamp/Generated/Models/QuestionAnswerUpdatePayload.swift:4 — only content: String
• kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/services/checkins.kt:60 — JSON body has only "content"

The Go fix hand-builds a map[string]any and calls UpdateAnswerWithBodyWithResponse, bypassing the generated typed body entirely. This works for Go but leaves five SDKs broken.

Fix: Add group_on: ISO8601Date to QuestionAnswerUpdatePayload in Smithy, regenerate OpenAPI, regenerate all SDKs. The Go marshalBody pattern is still needed (to avoid types.Date{} serializing as null on partial updates), but it should be fixing a zero-value problem, not compensating for a missing spec field.

marshalBody doc comment (helpers.go:26-34): CheckinsService.UpdateAnswer should be added to the method list since it now uses this pattern.

---

Medium: rewindableReader generalization in marshalBody

Previously marshalBody returned bytes.NewReader(b). Now it returns &rewindableReader{data: b}. This changes request framing for all 8+ callers.

What improves

doWithRetry (generated client.gen.go:3126) re-calls buildRequest() on each attempt, passing the same body io.Reader. With bytes.NewReader, after the first attempt reads to EOF, retries get an empty body — a pre-existing silent bug. rewindableReader auto-rewinds after EOF, so retries get the full payload. This is genuinely better for the full-read-then-retry path.

What regresses

http.NewRequest only special-cases *bytes.Buffer, *bytes.Reader, and *strings.Reader (see net/http/request.go:923). With *rewindableReader, ContentLength stays 0 with a non-nil body and no GetBody, which the transport treats as unknown length and sends chunked over HTTP/1.1.

	*bytes.Reader	*rewindableReader
ContentLength	actual length	0 with non-nil body (unknown length)
GetBody	snapshot closure	nil (no redirect replay)

Partial-read on retry

If a network failure interrupts mid-body-write, both readers leave the position in the middle. Neither rewinds. doWithRetry calls buildRequest() again with the same reader, so the retry sends only the unread suffix. This is equally broken for both implementations — not a regression, but not fixed either.

Test gap

TestMarshalBody_ReturnsReplayableReader (helpers_test.go:151) proves EOF-to-EOF replay with io.ReadAll. It does not cover the actual doWithRetry retry path, the partial-read case, or the changed request framing.

Fix options: Either fix retries centrally so each attempt gets a fresh *bytes.Reader/GetBody from raw bytes, or keep the current body builder and accept the framing change explicitly. Do not rely on adding Len() int to rewindableReader — http.NewRequest will not use it.

---

Low: Cleanup is clean

• Wrapper types (createAnswerRequestWrapper, updateAnswerRequestWrapper) were dead code. Removal is correct.
• rewindableReader move from account_service.go to helpers.go compiles (same package).
• UpdateAnswerRequest.GroupOn with omitempty correctly controls auto-fetch vs explicit path.
• Four new test cases cover the key scenarios: auto-preserve, explicit, invalid format, missing resolved. Test design is good.

---

Upstream contract inconsistency

The public API docs (bc3-api/sections/question_answers.md) show PUT /question_answers/:id with only content in the body, while the Rails controller and model require group_on. Decide which contract is authoritative, then update Smithy/OpenAPI/docs/tests to match.

---

Recommendation

1. Add group_on to QuestionAnswerUpdatePayload in Smithy, regenerate OpenAPI + all SDKs
2. Either fix retries centrally so each attempt gets a fresh *bytes.Reader/GetBody from raw bytes, or keep the current body builder and accept the framing change explicitly
3. Add CheckinsService.UpdateAnswer to the marshalBody doc comment
4. Coordinate with the bc3-api docs to clarify whether group_on is required on update

[github-actions]

Spec Change Impact

• Modified Operation: Fixes a validation issue in the UpdateCheckinAnswer operation that previously returned a 422 error.
• No operations, types, or resources were added or removed.
• This change is not a breaking API change (no operations or fields removed).

SDKs Needing Updates:

• Go
• TypeScript
• Ruby
• Kotlin
• Swift

All SDKs require regeneration to incorporate the fix.

[jeremy]

All four items from the review have been addressed:

1. Spec gap: Added group_on: ISO8601Date to QuestionAnswerUpdatePayload in Smithy, regenerated OpenAPI and all six SDKs (fbfb6bc, 9cb0685)
2. rewindableReader framing: Accepted as-is — already in production via account_service.go:219, and the retry improvement outweighs the chunked-encoding trade-off
3. marshalBody doc comment: Added CheckinsService.UpdateAnswer to the method list (6e7bf68)
4. Upstream contract: Out-of-repo — bc3-api docs need a separate update to document group_on as required on PUT

Additionally: added update-answer group_on tests for TypeScript, Ruby, and Python (35a94a7). Ruby/Swift generator catchup from prior spec changes isolated in a separate commit (d7617f6).

[cubic-dev-ai]

1 issue found across 62 files (changes from recent commits).

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="python/tests/services/test_checkins.py">

<violation number="1" location="python/tests/services/test_checkins.py:51">
P1: This test asserts the old broken behavior by requiring `group_on` to be omitted, so it won’t catch regressions of the 422 fix that should always send `group_on` on update.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

[Copilot]

Pull request overview

Copilot reviewed 11 out of 24 changed files in this pull request and generated 5 comments.

---

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