fix(gauges): absorb five spec drifts from the contract bc3 serves - #853
Conversation
github-actions
Bot
added
typescript
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
The empty update payload remains invalid, TypeScript validation lacks coverage, and migration guidance misstates breaking and nil behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Aligns the Gauges specification and all six SDKs with bc3’s served contract.
Changes:
- Models
comment_count, nullable previous positions, andToggleGauge404s. - Requires the gauge-needle update wrapper and corrects notification documentation.
- Regenerates SDK artifacts, fixtures, tests, and migration guidance.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
spec/basecamp.smithy |
Updates the canonical Gauges contract. |
openapi.json |
Regenerates the OpenAPI model. |
scripts/enhance-openapi-go-types.sh |
Marks previous needle position nullable. |
spec/fixtures/manifest.yaml |
Registers the first-needle fixture. |
spec/fixtures/gauges/needle_get.json |
Adds singular comment count. |
spec/fixtures/gauges/get_first_needle.json |
Covers a null previous position. |
typescript/src/generated/services/gauges.ts |
Requires and validates the update wrapper. |
typescript/src/generated/schema.d.ts |
Regenerates Gauge types and responses. |
typescript/src/generated/openapi-stripped.json |
Synchronizes generated OpenAPI input. |
typescript/src/generated/metadata.ts |
Refreshes generation metadata. |
swift/Sources/Basecamp/Generated/Models/UpdateGaugeNeedleRequest.swift |
Makes the payload non-optional. |
swift/Sources/Basecamp/Generated/Models/GaugeNeedle.swift |
Adds required commentCount. |
swift/Tests/BasecampTests/GaugesServiceTests.swift |
Tests singular comment-count decoding. |
ruby/lib/basecamp/generated/services/gauges_service.rb |
Requires the update keyword. |
ruby/lib/basecamp/generated/types.rb |
Adds the comment-count model field. |
ruby/lib/basecamp/generated/metadata.json |
Refreshes generation metadata. |
python/src/basecamp/generated/services/gauges.py |
Requires the update argument. |
python/src/basecamp/generated/types.py |
Updates Gauge typing. |
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/services/Types.kt |
Makes the update payload non-nullable. |
kotlin/sdk/src/commonMain/kotlin/com/basecamp/sdk/generated/services/gauges.kt |
Always serializes the wrapper. |
kotlin/sdk/src/commonTest/kotlin/com/basecamp/sdk/GaugesServiceTest.kt |
Updates wrapper serialization coverage. |
go/pkg/generated/client.gen.go |
Regenerates Gauge models and 404 handling. |
go/pkg/basecamp/gauges.go |
Preserves null positions and comment counts. |
go/pkg/basecamp/gauges_test.go |
Tests both corrected decode behaviors. |
MIGRATING.md |
Documents consumer-facing changes. |
Review details
- Files reviewed: 11/25 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![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: a61be65d24
ℹ️ 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".
|
Converged: CI green on 591cc24 (46 checks), the first review round's five threads all fixed and answered (description required on the update payload at the spec layer, migration text corrected on explicit nil and on the comment_count break), and no review activity since. Labelled breaking; MIGRATING.md carries the Unreleased section. Ready for a human look and merge. |
591cc24 to
bb4d1ff
Compare
GaugeNeedle models the singular comment_count bc3 emits unconditionally; UpdateGaugeNeedle requires its gauge_needle wrapper, as bc3 does; the notify doc names the values Subscribers#find_subscribers accepts; Gauge's previous_needle_position is nullable, and Go's wrapper takes a pointer so the first needle's null is not a move from 0; ToggleGauge declares NotFoundError. Regenerated into all six SDKs. Fixes #731
…n empty wrapper as missing
…migration note The Rust SDK landed on main while this branch was open, and it is generated from the same openapi.json: gauge_needle and its description become required members, comment_count arrives on GaugeNeedle, and the one generated-call test that answered a needle now carries it.
bb4d1ff to
84161f7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84161f7642
ℹ️ 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".
…st type, and carry comment_count in every inline needle stub SPEC's upload paragraph cited the gauge-needle as the precedent where a nil *string leaves the field alone, which UpdateNeedle now refuses as a usage error: the description is the payload's only member and bc3 requires it, so there is no third state to express. The migration note named a Rust type that does not exist — the generated service takes UpdateGaugeNeedleRequestContent — and left out that Python's Gauge TypedDict now types previous_needle_position NotRequired[Optional[int]]. The Go and Python needle stubs that omitted comment_count decoded to a zero the contract no longer allows; they carry the required key now, as the Kotlin and Swift stubs and the shared fixture already did.
* origin/main: fix(kotlin): project the transport error before it becomes the network message (#872) deps(rust): bump toml in /rust in the cargo-dependencies group (#863) deps(actions): bump the actions group across 1 directory with 2 updates (#865) Compile the CodeQL Kotlin build with the newest Kotlin CodeQL supports (#875)
Summary
Five drifts between the gauges spec and the contract bc3 serves at the pinned revision, all read off the jbuilder/controller source and
doc/api/sections/gauges.md, fixed in the spec (and the one Go hand-written wrapper) and regenerated into all six SDKs:GaugeNeedle.comment_count—_needle.json.jbuilderemits the singular key unconditionally, next to the envelope's pluralcomments_count. Modeled@required, with the same doc noteSearchResult.comment_countcarries. GoGaugeNeedle.CommentCount, SwiftcommentCount; the dict-shaped SDKs already passed it through.spec/fixtures/gauges/needle_get.jsonnow carries it and the manifest's KNOWN INCOMPLETE note is gone.UpdateGaugeNeedleInput.gauge_needleis@required, and so is itsdescription—needle_paramsopens withparams.require(:gauge_needle), which rejects a missing wrapper and an empty one alike, anddescriptionis the only member the update accepts (needle_params.except(:color, :position)). So the wrapper-less and empty-wrapper bodies every SDK permitted were a 400, never a no-op. MatchesCreateGaugeNeedle. Go's wrapper keepsDescription *stringand refusesnilbefore the wire as a usage error.notifydoc string — nameseveryone,default,custom, which is exactly whatSubscribers#find_subscribersaccepts; the previousworking_onwas invented and silently notified nobody. Surfaced in generated docs in all six SDKs.Gauge.previous_needle_positionis nullable — JSONnullfor a gauge's first needle (gauges.mdshows it).nullable: truelayered inenhance-openapi-go-types.shlike the other nullable-when-present members; Go's wrapper field becomes*int32so the null is distinguishable from a genuine move from 0. A second fixture,gauges/get_first_needle.json, pins the null against the schema.ToggleGaugedeclaresNotFoundError—ProjectScopedraisesRecordNotFoundfor an unknown project, asListGaugeNeedleson the same label already declared.The three "noted, not proposed" items in the issue (
.jsonsuffix asymmetry,maxPageSize: 50, comma-joinedbucket_ids) are left as they are.Red-proof, from the literal wire bodies
TestGaugeNeedle_DecodesTheSingularCommentCount(comment_count: 5next tocomments_count: 2— the field did not exist before) andTestGauge_PreviousNeedlePositionNullIsNotZero(null→ nil, wire0→ non-nil 0; the value type decoded both to 0).GaugeNeedleis typed, so the test fixture gainedcomment_countand the decode test assertscommentCount— before the spec change the field was discarded.make check-fixture-coveragevalidatesneedle_get.jsonagainst the now-required member andget_first_needle.json's null against the now-nullable one.Breaking
Labelled
breaking; the# Unreleasedsection inMIGRATING.mdcovers both:gauge_needleand itsdescriptionbecome required onUpdateGaugeNeedlein TypeScript, Python, Ruby, Kotlin and Swift (a call that omitted them stops compiling or raises before the wire — it always got a 400; an explicitNone/nilin Python/Ruby still compacts to an empty body and gets the 400).GaugeNeedle.comment_countis required, so Swift's public initializer and the TypeScript/Python model types gain a required member.Gauge.PreviousNeedlePositionis*int32(apidiff flags the type change).Fixes #731