You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
bc3 #12659 documented subtasks as a first-class resource with flat canonical routes, shaped after comments: a to-do or card advertises subtasks_count, subtasks_completed_count and subtasks_url, and the index paginates. This models it as a Subtasks service in all seven SDKs — list, get, create, update, complete, uncomplete, reposition, delete — reusing the CardStep structure, because the wire shape did not change: type stays "Kanban::Step", only the routes and the parent's accounting are new. Todo, Card and the generic Recording carry the three accounting members. CardSteps stays as it was; bc3 keeps the card-scoped /steps spellings served indefinitely as aliases of the same records.
Absorbing documented routes means moving the provenance pin, so this advances it from c680233ba0e to 5daa0911d33 and triages the range in spec/api-gaps/README.md. Client users had already been absorbed ahead of the repin. The three other documented additions — the unscoped recording show (#10158), backlinks (#13121) and bulk enrollment (#9962) — are registered as addressed-in-bc3-pr briefs for follow-up absorption rather than modelled here, with matching dispositions in the route allowlist, and the client-route waivers the repin makes redundant are deleted.
The breaking label is for one behaviour change, in Go: CardStepsService.Reposition now rejects 0. bc3's card_table_steps.md called the reposition position "Zero indexed" until #12659 fixed it to 1-based; the server always counted from 1, so a caller sending 0 was outside the documented range all along, and the member doc and Go's lower bound now say so (MIGRATING has the note). A second correction rode along but changes nothing at runtime: PUT /my/unreads refuses batches over 500 readables with a 422, so MarkAsRead declares ValidationError and a @length(max: 500). No generator enforces the length on the client, so callers get the same 422 from the server they always did; that part is documentation.
Verification notes, for the reviewer rather than the release notes.
make -k check-targets ran locally with every gate green except two the machine cannot run: swift-check (the Swift package uses CoreFoundation and builds only on macOS; the generator itself ran in a swift:6.1 container and its output is what is committed, so swift-check-drift in CI is the real verification) and rs-deny (no cargo-deny installed). Kotlin built and tested through Gradle, the conformance runners replayed 215 cases, TypeScript 1750 tests, Ruby 1537 runs, Go, Python and Rust suites all pass, and route parity reports 270 SDK routes against 398 bc3 routes at the new pin.
Design notes. The shape is deliberately still CardStep: renaming it Subtask across seven SDKs is a breaking rename with no wire change behind it, so it is a separate decision. The Go wrapper's Update hand-marshals its body the way CardStepsService.Update does, because "due_on": "" is the only spelling of a clear the generated *types.Date cannot express (SPEC §18 rule 1). DefaultSubtaskLimit is 100 to match the 100 a parent embeds under steps, so a default listing never returns less than the parent already showed.
The range triage counted eleven non-merge commits touching doc/api or app/views/api; the table grew by exactly the sixteen routes the entries above account for. Circle creation (#9949) gained API views but no documentation, so it is recorded in the triage and not registered — a documented contract is the trigger.
Two red checks are inherited from main, not this branch: cargo deny (Rust SDK) and the Rust stable (1.98.1) job both stop at the cargo-deny stage on RUSTSEC-2026-0285 (a rustls TLS 1.3 advisory in a transitive dependency). main at f2f900f fails the same two, and this branch touches no Rust dependencies; the Rust fmt, lint, tests and docs stages pass before deny runs. A dependency bump clears both, and belongs in its own PR.
position is an int, but the generated request field is int32. On 64-bit Go, values above math.MaxInt32 pass this check and are narrowed here; for example, math.MaxInt32+1 becomes a negative position, while math.MaxInt32+2^32+1 can wrap to position 1. Reject values above math.MaxInt32 before the cast, as the existing todolist reposition wrapper does.
if position < 1 {
err = ErrUsage("position must be at least 1")
return err
}
body := generated.RepositionCardStepJSONRequestBody{
SourceId: stepID,
Position: int32(position), // #nosec G115 -- position is validated and bounded by API
go/pkg/basecamp/subtasks.go:279
This introduces another explicit-body carve-out, but SPEC §18's allowlist still names only CardsService.UpdateVerbatim and CardStepsService.Update for date clears. Add SubtasksService.Update to that documented exception (and its conformance/architecture inventory) so this generated-client bypass remains auditable and does not violate the repository's hard rule by omission.
// Hand-marshaled map, not generated.UpdateSubtaskRequestContent — the
// SPEC §18 rule 1 carve-out CardStepsService.Update also takes: the
// "due_on": "" clear is unreachable through *types.Date.
go/pkg/basecamp/subtasks_test.go:379
This comparison is now false: the PR changed CardStepsService.Reposition and its generated documentation to a 1-based position, and the Go implementation rejects values below 1. The stale “0-indexed” wording will mislead maintainers about the contract.
// Positions are 1-based, unlike the 0-indexed card-step reposition.
spec/basecamp.smithy:12772
The new contract says that sending null clears due_on, but this conflicts with SPEC §18: the SDKs compact null/None and the cross-SDK clear encoding is an explicit empty string ("due_on": ""). The generated Python/TypeScript/Kotlin/Swift/Ruby APIs cannot send a JSON null here either, so documenting it as supported is misleading; describe only the empty-string clear and regenerate the derived documentation.
Files reviewed: 58/103 changed files
Comments generated: 1
Review effort level: Lite (auto)
Note
Copilot is running an experiment and ran this review at Lite.
zachasme
changed the title
Add the Subtasks service and repin to bc3 5daa0911d3
feat(subtasks): add the Subtasks service and repin to bc3 5daa0911d3
Sep 15, 2026
Codex adversarial review (read-only, against the local bc3 checkout at 5daa0911d3) and what came of it, folded into the one commit:
Fixed. Go's Reposition in both SubtasksService and CardStepsService cast int to int32 after checking only the lower bound, so 2147483648 would have wrapped negative and landed the item at the top instead of the bottom. Both now refuse anything above math.MaxInt32, with a test. The account-people-enrollment brief's suggested error list omitted the documented 507 Insufficient Storage for the account user limit, which bc3 raises from both the preflight and the enrollment path; the brief now names it, modelled like StorageLimitError.
Also from CONTRIBUTING, not Codex: the commit and title now follow Conventional Commits, and the root, TypeScript, Ruby and Go READMEs gained the Subtasks row their service tables were missing.
Noted, not changed. Rust cannot express bc3's due-date clear (null or "") on UpdateSubtask because the generated Option<Date> omits on None — the same standing limitation UpdateCardStep and every generated date field carry, and the reason the merge-safe composites exist; a Rust composite for subtasks would be a separate PR. Go's UpdateSubtaskRequest.Title treats "" as "leave unchanged", which mirrors UpdateStepRequest and is documented on the field; a pointer would let a caller send an empty title on purpose, but I kept the two request types symmetrical.
Not done: CONTRIBUTING step 5 asks for conformance fixtures for new operations. Neither Boosts nor CardSteps has one today, and a new fixture means dispatch arms in all seven runners, including Swift, which this machine cannot compile. Happy to add one if you want it in this PR.
bc3 #12659 documented subtasks as a first-class resource with flat canonical
routes, shaped after comments: the parent advertises subtasks_count,
subtasks_completed_count and subtasks_url, and the index paginates. Model it as
a Subtasks service — ListSubtasks, GetSubtask, CreateSubtask, UpdateSubtask,
CompleteSubtask, UncompleteSubtask, RepositionSubtask, DeleteSubtask — reusing
the CardStep structure, because the wire shape did not change: type stays
"Kanban::Step", only the routes and the parent's accounting are new. Todo, Card
and the generic Recording projection carry the three accounting members. The
CardSteps operations stay as they were; bc3 serves the card-scoped /steps
spellings indefinitely as aliases of the same records.
The route-parity gate compares the spec against bc3's docs at the provenance
pin, so absorbing documented routes means moving the pin. Advance it from
c680233ba0e to 5daa0911d33 and triage the 249-commit range in the gap
registry: client users were absorbed ahead of the repin, and the three other
documented additions — the unscoped recording show, backlinks, and bulk
enrollment — are registered as addressed-in-bc3-pr entries for later
absorption, with matching dispositions in the route allowlist. The stale
client-route waivers that this repin makes redundant are deleted.
Two contract corrections rode along. bc3's card_table_steps.md said the
reposition position was "Zero indexed" until #12659 corrected it to 1-based;
the server always counted from 1, so RepositionCardStep's member doc and the
Go wrapper's lower bound move with it (MIGRATING). And PUT /my/unreads now
refuses batches over 500 readables with 422, so MarkAsRead declares
ValidationError and a @Length(max: 500).
Fixtures for the two subtask reads come from the documented examples and are
validated as CardStep; todos/get.json and cards/get.json gain the accounting
keys. Every SDK has tests for the eight operations, and the pinned counts in
check-idempotency-parity, the Rust guarantees and the grouped-client inventory
are restated for 270 operations.
Output of make generate plus make bc3-routes at the 5daa0911d3 pin, on top of the hand-written commit: openapi.json, behavior-model.json, the bc3 route table, url-routes, and the generated clients, services, metadata and types for Go, TypeScript, Ruby, Python, Kotlin, Swift and Rust, with the API version constants synced to 2026-09-15.
…cases per operation
Build MaxInt32+1 at runtime in the two reposition range tests: the constant
expression overflows int on 32-bit and fails to compile there, which
page_param_test.go already documents.
subtasks_count, subtasks_completed_count and subtasks_url on Todo and Card
take omitempty like every neighbouring optional field, so a Todo decoded from
a server that renders none of them re-marshals without asserting zero
subtasks and an empty URL.
Add the error case Copilot asked for on every subtask operation that lacked
one: update and reposition 422 in TypeScript, Ruby and Python; complete and
uncomplete 404 in TypeScript and Ruby; uncomplete 404 in Python; get 404 in
Ruby.
The new fields on the generic Recording projection have no field-bearing fixture. The manifest's Recording representatives are a Message and a Todolist, and neither contains these keys; only the separate Todo and Card schemas were updated. Add a to-do/card-shaped Recording fixture entry containing all three members so this advertised generic projection is verified across generators.
The reason will be displayed to describe this comment to others. Learn more.
🤖 This review was drafted with AI assistance.
Hey Zach, this is a really solid piece of work. You modelled all eight operations across seven SDKs, absorbed the repin and triaged its range, and handled the Copilot feedback cleanly. The gap brief write-up and the verification notes made this much easier to review. Thank you.
I checked the routes, status codes and parameters against bc3's subtasks.md and the controller at 5daa0911d33, and they all line up. A few small things before I approve:
1. A fixture for the new Recording fields
subtasks_count, subtasks_completed_count and subtasks_url are new on the generic Recording, but neither of the manifest's Recording examples includes them. One is a Message, the other a Todolist. We hit the same thing with bubble_up_url, and the recording-list-todolist entry is the pattern that fixed it. Could you add a to-do- or card-shaped element that carries all three keys and register it in spec/fixtures/manifest.yaml? Then check-fixture-coverage will guard those fields like the rest.
2. due_on clear wording on UpdateSubtask
The doc comment says to send "due_on": "" to clear the date. That works: SubtasksController passes it to assign_attributes on a date column, which casts "" to nil. But bc3 documents "due_on": null for this endpoint, and that's also what its test covers ("API: update with an explicit null due date clears it"). Since this comment ends up in every SDK's docs, could it lead with null and mention "" as also accepted? Something like:
Clearing a value takes an explicit send — "due_on": null clears the due date (an empty string is also accepted), "assignee_ids": [] removes every assignee.
3. The breaking label
I think the label is fair, but the description makes it sound bigger than it is. The MarkAsRead change adds @length(max: 500) and a ValidationError, but no generator enforces the length on the client. So callers get the same 422 from the server they always would have, and that part only changes documentation. The one real behaviour change is Go's CardStepsService.Reposition rejecting 0. Could you tweak the description so someone skimming the release notes knows that's the only thing to check?
Two other things came up during review that aren't from this PR, so I've filed them separately instead of adding to your list:
CardStep doesn't model the completion object bc3 sends for completed steps and subtasks. Its completed_at and completer fields are never populated.
The Rust generator can't send an explicit null on optional request fields, so Rust users can't clear a due date on any update. UpdateSubtask just inherits that.
Neither needs to block this.
Thanks again, this is close. Happy to re-review as soon as those land. 🙏
…ar doc
Review follow-ups from Rob. A Todo-shaped element joins recordings/list.json and is registered as recording-list-todo, so check-fixture-coverage guards subtasks_count, subtasks_completed_count and subtasks_url on the generic Recording the way recording-list-todolist guards bubble_up_url; the Go list test asserts the three fields ride only on that element. The UpdateSubtask doc now leads with the null clear bc3 documents and tests, and says the empty string is what the Ruby, Python and TypeScript SDKs send because they drop nil from the body.
bc3 answers PUT /subtasks/:id with an empty body with 400: the controller's
require(:subtask) raises ParameterMissing once wrap_parameters has nothing to
wrap. Verified against a local bc3 (subtasks_controller.rb unchanged since
5daa0911d33). UpdateSubtask's doc now says to send at least one parameter,
and Go's SubtasksService.Update refuses an all-empty request with a usage
error before it reaches the wire.
The TypeScript "422 on update" test sent dueOn: "not-a-date", which the
generated method rejects locally, so the msw handler never ran and the test
passed on client-side validation alone. It now sends a well-formed update,
asserts the handler was reached, and drops the due_on error body bc3 never
emits (a malformed due_on is cast to nil and answered 200).
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
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.
zachasme
added
enhancement
github-actions
Bot
added
typescript
robzolkos
left a comment
bc3 #12659 documented subtasks as a first-class resource with flat canonical routes, shaped after comments: a to-do or card advertises
subtasks_count,subtasks_completed_countandsubtasks_url, and the index paginates. This models it as aSubtasksservice in all seven SDKs — list, get, create, update, complete, uncomplete, reposition, delete — reusing theCardStepstructure, because the wire shape did not change:typestays"Kanban::Step", only the routes and the parent's accounting are new.Todo,Cardand the genericRecordingcarry the three accounting members.CardStepsstays as it was; bc3 keeps the card-scoped/stepsspellings served indefinitely as aliases of the same records.Absorbing documented routes means moving the provenance pin, so this advances it from
c680233ba0eto5daa0911d33and triages the range inspec/api-gaps/README.md. Client users had already been absorbed ahead of the repin. The three other documented additions — the unscoped recording show (#10158), backlinks (#13121) and bulk enrollment (#9962) — are registered asaddressed-in-bc3-prbriefs for follow-up absorption rather than modelled here, with matching dispositions in the route allowlist, and the client-route waivers the repin makes redundant are deleted.The
breakinglabel is for one behaviour change, in Go:CardStepsService.Repositionnow rejects0. bc3'scard_table_steps.mdcalled the repositionposition"Zero indexed" until #12659 fixed it to 1-based; the server always counted from 1, so a caller sending0was outside the documented range all along, and the member doc and Go's lower bound now say so (MIGRATING has the note). A second correction rode along but changes nothing at runtime:PUT /my/unreadsrefuses batches over 500 readables with a 422, soMarkAsReaddeclaresValidationErrorand a@length(max: 500). No generator enforces the length on the client, so callers get the same 422 from the server they always did; that part is documentation.