Absorb bubble_up_url across the four shapes that carry it - #488
Conversation
Spec Change ImpactSummary of Changes
Impact on SDKs
SDK Regeneration Checklist
|
There was a problem hiding this comment.
Pull request overview
This PR absorbs the bubble_up_url field across the four response shapes that can carry it, modeling requiredness by call-site (required on Todolist/TodolistGroup, optional on the polymorphic Recording/SearchResult) and regenerating all downstream SDK artifacts plus fixtures/examples to match.
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:
- Update Smithy + OpenAPI to add
bubble_up_url, required onTodolist/TodolistGroupand optional onRecording/SearchResult. - Regenerate SDK models/types across TypeScript, Swift, Kotlin, Ruby, Python, and Go (including Go wrapper propagation).
- Extend fixtures + manifest coverage and adjust Go tests to pin discriminating presence/absence for polymorphic projections.
Reviewed changes
Copilot reviewed 17 out of 31 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| typescript/src/generated/schema.d.ts | Adds bubble_up_url to relevant TS schema types with requiredness aligned to shape. |
| typescript/src/generated/openapi-stripped.json | Updates stripped OpenAPI schema + examples to include bubble_up_url and required lists. |
| typescript/src/generated/metadata.ts | Regeneration timestamp update for TS metadata. |
| swift/Sources/Basecamp/Generated/Models/TodolistGroup.swift | Adds required bubbleUpUrl to generated Swift TodolistGroup. |
| swift/Sources/Basecamp/Generated/Models/Todolist.swift | Adds required bubbleUpUrl to generated Swift Todolist. |
| swift/Sources/Basecamp/Generated/Models/SearchResult.swift | Adds optional bubbleUpUrl to generated Swift SearchResult. |
| swift/Sources/Basecamp/Generated/Models/Recording.swift | Adds optional bubbleUpUrl to generated Swift Recording. |
| spec/overlays/examples.smithy | Updates Smithy examples to satisfy new required bubble_up_url members. |
| spec/fixtures/todolists/list.json | Adds required bubble_up_url to todolist list fixture elements. |
| spec/fixtures/todolists/get.json | Adds required bubble_up_url to todolist get fixture. |
| spec/fixtures/todolist_groups/list.json | Adds required bubble_up_url to todolist group list fixture elements. |
| spec/fixtures/todolist_groups/get.json | Adds required bubble_up_url to todolist group get fixture. |
| spec/fixtures/search/results.json | Adds a Todolist-shaped search result fixture carrying bubble_up_url. |
| spec/fixtures/recordings/list.json | Adds a Todolist-shaped recording fixture carrying bubble_up_url. |
| spec/fixtures/manifest.yaml | Registers new fixture targets and updates covered schemas for Recording/SearchResult. |
| spec/basecamp.smithy | Models bubble_up_url on Todolist, TodolistGroup, Recording, and SearchResult with correct requiredness. |
| spec/api-gaps/recording-bubbleupable-field.md | Clarifies that absorbing bubble_up_url does not close the separate bubbleupable boolean gap. |
| ruby/lib/basecamp/generated/types.rb | Regenerates Ruby types to include bubble_up_url (required where appropriate). |
| ruby/lib/basecamp/generated/metadata.json | Regeneration timestamp update for Ruby metadata. |
| python/src/basecamp/generated/types.py | Updates generated Python TypedDicts to include bubble_up_url (required vs NotRequired as modeled). |
| openapi.json | Updates canonical OpenAPI schema + examples and required lists for bubble_up_url. |
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/models/TodolistGroup.kt | Adds required bubbleUpUrl to Kotlin TodolistGroup. |
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/models/Todolist.kt | Adds required bubbleUpUrl to Kotlin Todolist. |
| kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/models/Recording.kt | Adds optional bubbleUpUrl to Kotlin Recording. |
| go/pkg/generated/client.gen.go | Updates Go generated models for bubble_up_url across the four shapes. |
| go/pkg/basecamp/todolists.go | Propagates required BubbleUpURL into Go wrapper Todolist + mapping. |
| go/pkg/basecamp/todolist_groups.go | Propagates required BubbleUpURL into Go wrapper TodolistGroup + mapping. |
| go/pkg/basecamp/search.go | Propagates optional BubbleUpURL into Go wrapper SearchResult + mapping. |
| go/pkg/basecamp/search_test.go | Updates Go search tests/fixtures expectations to assert discriminating bubble_up_url presence. |
| go/pkg/basecamp/recordings.go | Propagates optional BubbleUpURL into Go wrapper Recording + mapping. |
| go/pkg/basecamp/recordings_test.go | Updates Go recordings tests/fixtures expectations to assert discriminating bubble_up_url presence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71aeec274c
ℹ️ 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".
| @required | ||
| bubble_up_url: String |
There was a problem hiding this comment.
Repin provenance for this upstream absorption
This change absorbs a post-pin BC3 contract from the shared recording/todolist partials, but spec/api-provenance.json remains at revision c3086931 dated 2026-07-26. As a result, the recorded conformance baseline does not identify the upstream revision that supplied bubble_up_url, and subsequent drift checks will continue treating the absorbed change as post-pin drift. Update the BC3 revision/date to the current upstream HEAD and run make provenance-sync.
AGENTS.md reference: AGENTS.md:L263-L270
Useful? React with 👍 / 👎.
84b8cac to
3e36e73
Compare
71aeec2 to
093137e
Compare
There was a problem hiding this comment.
All reported issues were addressed
You’re at about 91% 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.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
You’re at about 95% 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.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
bd43995 to
bdbc370
Compare
091d4ad to
e7df40b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 32 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
spec/basecamp.smithy:8006
- Same doc-path consistency issue as the Recording shape: this comment cites
todolists/_todolist(missing.json.jbuilder) even though other new comments in this PR use the full partial filename. Prefer the fully-qualified partial path for consistency and easier source tracing. (This will require regenerating downstream artifacts that embed this description.)
/// URL of the Bubble Up record for this recording (BC5 addition). Optional
/// here because this is a polymorphic projection:
/// `recordings/_recording.json.jbuilder` emits the key only when the caller
/// passes `local_assigns[:bubbleupable]`, and `todolists/_todolist` is the
/// only partial that does. So a Todolist-shaped instance carries it and the
/// other recording types do not.
spec/basecamp.smithy:6331
- The doc comment references
todolists/_todolistwithout the.json.jbuildersuffix, while other newly-added comments in this PR (e.g., on Todolist/TodolistGroup) use the full partial filename. Using the full path here too would keep the spec documentation consistent and avoid confusion when tracing the call site that controlsbubble_up_urlemission. (This will require regenerating downstream artifacts that embed this description.)
This issue also appears on line 8001 of the same file.
/// URL of the Bubble Up record for this recording (BC5 addition). Optional
/// here because this is a polymorphic projection:
/// `recordings/_recording.json.jbuilder` emits the key only when the caller
/// passes `local_assigns[:bubbleupable]`, and `todolists/_todolist` is the
/// only partial that does. So a Todolist-shaped instance carries it and the
/// other recording types do not.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7df40bb42
ℹ️ 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".
| /// path renders that partial — so the key is present on every projection of | ||
| /// this shape. | ||
| @required | ||
| bubble_up_url: String |
There was a problem hiding this comment.
Update all response stubs for
bubble_up_url
Once this member is required, every Todolist/TodolistGroup response should contain it, but several endpoint tests still return malformed responses without bubble_up_url, including ruby/test/basecamp/services/todolists_service_test.rb:19-65, ruby/test/basecamp/services/todolist_groups_service_test.rb:20-34, python/tests/services/test_todolists_service.py:88-145, and typescript/tests/services/todolistGroups.test.ts:33-88. Because those clients accept these partial mappings without runtime schema validation, the tests continue passing even if the new field is dropped during decoding or service generation; update these stubs and assert that the field flows through.
AGENTS.md reference: AGENTS.md:L310-L315
Useful? React with 👍 / 👎.
e7df40b to
9479720
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 35 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
python/tests/services/test_todolists_service.py:26
- The docstring says this stub "must not contradict the contract", but the returned payload omits many required Todolist fields (e.g., app_url, bucket, created_at, etc.). That makes the comment misleading; either load the canonical fixture (as other Python tests do) or clarify that this helper is intentionally minimal and only includes bubble_up_url because it became required.
payload shape — but a stub must not contradict the contract either.
bubble_up_url is @required on Todolist (todolists/_todolist.json.jbuilder
always passes bubbleupable: true), so it belongs here even though nothing
below reads it. Full-shape coverage lives in spec/fixtures, which
`make check-fixture-coverage` validates.
dbbe7b2 to
fba66dd
Compare
9479720 to
78b436f
Compare
|
Correcting myself on the 2,000 ms assertion. I said
Three consecutive clean full-runner runs again — but I'm noting the number rather than resting on it, since that's exactly what I did last time. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 35 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
python/tests/services/test_todolists_service.py:33
- The
_todolist()helper claims the stub “must not contradict the contract”, but the returned dict is still missing many required Todolist members (e.g. app_url, url, type, status, inherits_status, created_at/updated_at, parent, bucket, creator). Since this helper is being updated for the new requiredbubble_up_url, it’s a good place to make the stub minimally contract-valid so these tests can’t accidentally normalize an impossible payload.
return {
"id": 2,
"name": name,
"description_attachments": [],
"bubble_up_url": "https://3.basecampapi.com/12345/buckets/1/recordings/2/bubble_up.json",
Already at the provenance pin in my/readings/_reading.json.jbuilder and modeled on Reading. The post-pin drift is the shared recording and todolist partials, and requiredness differs by shape. recordings/_recording.json.jbuilder emits the key only when the caller passes local_assigns[:bubbleupable]. todolists/_todolist.json.jbuilder is the ONLY partial that passes it, and it passes bubbleupable: true unconditionally. Every list, show, and group path renders that partial. So Todolist and TodolistGroup get @required; Recording and SearchResult stay optional, because they are polymorphic projections where only the Todolist variant receives the flag. SearchResult is in scope because Todolists are searchable — Search::Type::TYPE_FILTER_MAPPING maps "Todo" => %w[Todo Todolist] — and Searches::ApiHelper#api_search_result_template_path falls through to to_recordable_partial_path, which for a Todolist resolves to the same todolist partial. Adding required response members changes generated public types, so this is breaking. Fixtures: bubble_up_url added to the direct Todolist and group fixtures, plus field-bearing representatives for the two optional shapes — a Todolist-shaped element in recordings/list.json and a Todolist search result. recordings/get.json is a Message and correctly keeps omitting the key. Both new representatives are registered in the fixture manifest so the coverage guard validates them, and the Go tests assert the shape discriminates: Todolist carries it, every other type does not. spec/api-gaps/recording-bubbleupable-field.md stays open. It tracks a distinct boolean bubbleupable field expressing per-user eligibility. bubble_up_url is a URL whose presence is decided by the rendering call site, so it is constant per shape and cannot answer whether this user can bubble this up. Noted in the entry.
… flake Both Todolist elements I added were built by cloning a Message element and overwriting only the URLs, so their identity fields disagreed: URLs said bucket 2085958500 while bucket.id said 2085958499, parent was a Message::Board rather than a Todoset, and the bookmark SGIDs still encoded the Message's recording id. Every identity-bearing field is now taken from the real todolists/get.json — one bucket, one recording id, a Todoset parent, and a bookmark SGID that decodes to the right recording. Separately, the TypeScript conformance runner captured request timestamps with Date.now(), which floors to whole milliseconds. Two floored reads bracketing a 2000ms sleep can legitimately differ by 1999, which is what turned retry.json's delayBetweenRequests assertion red. The Go runner uses nanosecond time.Time and never saw it. Switched to performance.now(); the assertion is unchanged and the SDK really does sleep the full interval. Three consecutive clean runs of the TypeScript runner.
Ruby's todolists test_get hand-rolled { id, name, description_attachments }.
Todolist has a dozen required members and that stub omits nearly all of
them, so it was a payload BC3 cannot produce — it passed only because Ruby
does not validate required fields at runtime. Repointed at the canonical
spec/fixtures/todolists/get.json, which make check-fixture-coverage
validates, so it can no longer drift from the contract. It now also asserts
bubble_up_url is present.
Python's todolists stubs belong to metadata/hook-identity tests where the
payload is deliberately irrelevant, so rather than inflate them to full
shape I factored a small _todolist() helper that carries the required
bubble_up_url and says why it is there.
TypeScript needed no stub change: todolists.test.ts already imports
spec/fixtures/todolists/get.json directly, so it picked the field up with
the fixture. Added an explicit assertion so that is provable rather than
incidental, and so a fixture regression fails here too. todolistGroups
constructs no Todolist-shaped payload at all — it covers create/reposition
and the absence of get/update.
…recisely My previous commit blamed Date.now() truncation alone and claimed three clean runs proved it. That was wrong. With performance.now() the same assertion later failed at 1999.8753749999996ms — sub-millisecond, so not a rounding artifact: libuv rounds a timer's deadline down internally, so setTimeout(2000) can genuinely elapse in slightly under 2000ms of wall clock. Go's timers do not fire early, which is why only this runner sees it. performance.now() is still right (Date.now() truncation was a real second source of noise), but it needed a documented TIMER_SLACK_MS of 2ms on the comparison. That cannot mask a real regression: a dropped Retry-After halves the backoff (~1000ms off) and a missing delay misses by the whole interval.
c529eeb to
8365fea
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 35 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
python/tests/services/test_todolists_service.py:23
- The docstring says this stub "must not contradict the contract", but the returned dict still omits many required Todolist members (e.g., url/app_url/created_at/etc.), so that statement is inaccurate/misleading. Either load a full fixture here, or adjust the wording to reflect that this is intentionally not a contract-complete payload.
These tests are about operation identity and resource-id scoping, not
payload shape — but a stub must not contradict the contract either.
bubble_up_url is @required on Todolist (todolists/_todolist.json.jbuilder
Stacked on #487 → #486 → #484. Prerequisite for the #477 SDK follow-up.
bubble_up_urlwas already at the provenance pin inmy/readings/_reading.json.jbuilderand modeled onReading. The post-pin drift is the shared recording and todolist partials — and requiredness is not uniform across the shapes that carry it.Requiredness differs by shape, and the reason is the call site
app/views/api/recordings/_recording.json.jbuilder:19-21:app/views/api/todolists/_todolist.json.jbuilder:2:That is the only
bubbleupablepasser in the entire view tree (grep -rn "bubbleupable" app/views/returns exactly these two hits), and it passes it unconditionally.@required@requiredtodolists/groups/{index,show}.json.jbuilderrender the same partialSearchResultis in scope because Todolists are searchable:Search::Type::TYPE_FILTER_MAPPINGmaps"Todo" => %w[Todo Todolist], andSearches::ApiHelper#api_search_result_template_pathfalls through torecording.to_recordable_partial_path, which for a Todolist resolves totodolists/todolist— the partial that passes the flag.Adding required response members changes generated public types, so this is breaking.
Fixtures — including the field-bearing representatives
Direct shapes:
todolists/{get,list}.jsonandtodolist_groups/{get,list}.json.The two optional shapes needed representatives that actually carry the field, because their existing fixtures correctly do not:
recordings/get.jsonand both elements ofrecordings/list.jsonare Messages, which never receivebubbleupable. Added a third, Todolist-shaped element torecordings/list.json.spec/fixtures/search/results.json(with the required-and-nullablecontent/descriptionand a highlightedplain_text_description, per Search: model content/description as required and nullable, add the plain-text excerpts #487).Both are registered in
spec/fixtures/manifest.yamlas new targets (recording-list-todolist,search-result-todolist) and added tocovered_schemas, so the coverage guard validates them rather than leaving them unchecked.make check-fixture-coverage→ 22 manifest targets.URLs are derived from each fixture's existing
subscription_url/url, so they stay internally consistent and match the realbucket_recording_bubble_up_urlshape (.../buckets/<b>/recordings/<id>/bubble_up.json, from therecording_actionsconcern inconfig/routes.rb:231).Go wrappers
make go-check-wrapper-driftnamed all four shapes; all four wrappers now carryBubbleUpURLand assign it in their*FromGeneratedbodies. Required on Todolist/TodolistGroup (noomitempty),omitemptyon Recording/SearchResult. Gate reports83 pairs walked, 1123 generated fields verified.Go tests assert the field discriminates rather than merely existing: for every recording and every search result, a
Todolistmust haveBubbleUpURLset and every other type must not. A regeneration that made it unconditional would fail.Smithy examples
spec/overlays/examples.smithyis validated for required-member completeness — the firstmake generatefailed withMissing required structure member bubble_up_urlfor bothGetTodolistOrGroupexamples. Both updated.spec/api-gaps/recording-bubbleupable-field.mdstays openIt tracks a distinct boolean
bubbleupablefield expressing per-user eligibility.bubble_up_urlis a URL whose presence is decided by the rendering call site, so it is constant per shape and cannot answer "can this user bubble this up". Annotated the entry to say so, so nobody mistakes this absorption for closing it.make validate-api-gaps→ clean, 27 entries.make checkexits 0.Summary by cubic
Adds
bubble_up_urltoTodolist,TodolistGroup,Recording, andSearchResultacross the API and SDKs. Breaking change: it’s required onTodolistandTodolistGroup; regenerate SDKs and update fixtures/tests.New Features
bubble_up_urlacross four shapes; required onTodolist/TodolistGroup, optional onRecording/SearchResult(present for Todolist variants). OpenAPI/Smithy updated; Go/Swift/Kotlin/Python/Ruby/TypeScript SDKs regenerated.recordings/list.jsonandsearch/results.json.Bug Fixes
performance.now()and allow a 2msTIMER_SLACK_MSto handle early Node timers.todolists/get.jsonand assertsbubble_up_url; Python stubs include requiredbubble_up_url; TypeScript adds an explicit assertion.Written for commit 8365fea. Summary will update on new commits.