fix: review fixes for the spend API contract - #4879
Open
tothandras wants to merge 8 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThe change updates charge creation contracts, feature-reference handling, realization identifiers, service-period filter documentation, generated clients, conversion tests, wire mapping tests, and nested union validation. It also changes Go union accessor naming to use mapped model and nested union names. ChangesCharge API contracts and generated models
Nested union validation and naming
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
chrisgacsal
approved these changes
Aug 7, 2026
- nil-safe flat-fee feature mapping; serialize required realizations as [] - create requests take a feature reference instead of the expandable union - expose optional realization run id; window filter recipe uses gte/lte - name Go union accessors after nested unions (AsInvoice, not AsVariantName) - tsp doc cleanups and regenerated OpenAPI/Go/TS artifacts
borosr
force-pushed
the
feat/v3-charges-spend-api-review
branch
from
August 7, 2026 13:37
a8378e1 to
30d1707
Compare
Neither codegen gate reached into a union-typed variant, so the models a nested union contributes were never verified against the outer variants they compete with in the mapper's key-coverage pick. Both gates now check them. Nested models are checked against the map built from the outer union's direct variants, never merged into it: a nested discriminated union's own variants legitimately disagree on non-discriminator keys because that union selects on its discriminator rather than by shape, and merging them would fail the build on unions that decode correctly. Nested-vs-nested stays unchecked and is now stated as such, in the gates and in the wire runtime comment that previously claimed nested variants were not verified at all. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
selectVariant starts its search below zero matches, so a non-discriminated union with any object variant always resolves one — data matching no variant key selects the narrowest, and the walk then drops the undeclared keys. The pass-through comment claimed the opposite; it describes only a discriminated miss or a union with no object variant, and now says so. Keeping the behavior rather than requiring a match: passing unknown keys through would put snake_case names into a value fromWire's return type declares to be camelCase, and dropping them is what the object branch already does for a non-union field. Tests pin the pick (narrowest of three, identified by its default), the key dropping, the defaults materialized onto zero-coverage data, and the pass-through path that survives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The gate tests exercised conflictingVariantProperty and variantGoName, but none of the three throws GoUnion itself raises: conflicting variants in a non-discriminated union, a discriminated union mixing model and non-model variants, and an unsupported discriminator envelope. All three fire before any JSX is produced, so calling GoUnion as a plain function drives them. Also covers the nested-variant conflict end to end, not just through the predicate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The per-operation sweep samples a union's first option only, so the reference side of every expandable reference goes unexercised and nothing asserts that the expanded side keeps its fields instead of collapsing onto the reference. Charge realizations are where both shapes land: `invoice` is a union whose expanded arm is itself a discriminated union, and `detailed_lines` is an array of another. Verified as a regression guard: three of the four cases fail when the nested-union resolution in selectVariant is removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review asked why ChargeFilter.FeatureID is *StringExactFilter when the spec declares ULIDFieldFilter. It is the emitter's mapping, applied SDK- wide, not a charges-specific slip. Say so where the mapping lives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/spec/packages/typespec-typescript/src/casing-gate.ts`:
- Around line 231-238: Update
api/spec/packages/typespec-typescript/src/casing-gate.ts lines 231-238 to retain
each nested model’s immediate outer-variant provenance and compare shared fields
across models from different outer branches, while preserving the exemption for
siblings within the same discriminated union. Apply the same provenance-aware
conflict detection in api/spec/packages/typespec-go/src/components/GoUnion.tsx
lines 324-359, and add a regression fixture covering nested union branches with
incompatible field types; ensure unsafe non-discriminated unions with multiple
object variants in request or success-response bodies are rejected.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6afa1dbd-9477-4047-9aad-fe3f7f6c04a1
📒 Files selected for processing (10)
api/spec/packages/aip-client-javascript/src/lib/wire.tsapi/spec/packages/aip-client-javascript/tests/charges.wire.spec.tsapi/spec/packages/aip-client-javascript/tests/wire.spec.tsapi/spec/packages/typespec-go/src/components/GoUnion.tsxapi/spec/packages/typespec-go/src/go-types.tsxapi/spec/packages/typespec-go/test/union-gate.test.tsapi/spec/packages/typespec-typescript/src/casing-gate.tsapi/spec/packages/typespec-typescript/src/runtime/wire.tsapi/spec/packages/typespec-typescript/test/nested-union.test.tsapi/v3/handlers/customers/charges/convert.go
Comment on lines
+231
to
238
| for (const { model, name } of nestedObjectVariants(program, union)) { | ||
| for (const [key, prop] of effectiveShape(program, model)) { | ||
| const seen = byKey.get(key) | ||
| if (seen && seen.type !== prop.type) { | ||
| return `'${key}' has a different type in ${seen.variant} and ${name}` | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Check conflicts between separate nested-union branches.
The current code compares nested models only with direct object variants. For an outer non-discriminated union such as NestedA | NestedB, incompatible shared fields in NestedA and NestedB are never compared. The mapper and generated As* accessors can then select or decode incompatible payloads.
api/spec/packages/typespec-typescript/src/casing-gate.ts#L231-L238: retain the immediate outer-variant origin for each nested model, then compare models from different outer branches while preserving the exemption for siblings of the same discriminated union.api/spec/packages/typespec-go/src/components/GoUnion.tsx#L324-L359: retain the same branch provenance before conflict detection, and add a regression fixture for two nested union branches with incompatible field types.
As per coding guidelines, reject unsafe non-discriminated unions with multiple object variants in request or success-response bodies.
📍 Affects 2 files
api/spec/packages/typespec-typescript/src/casing-gate.ts#L231-L238(this comment)api/spec/packages/typespec-go/src/components/GoUnion.tsx#L324-L359
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/spec/packages/typespec-typescript/src/casing-gate.ts` around lines 231 -
238, Update api/spec/packages/typespec-typescript/src/casing-gate.ts lines
231-238 to retain each nested model’s immediate outer-variant provenance and
compare shared fields across models from different outer branches, while
preserving the exemption for siblings within the same discriminated union. Apply
the same provenance-aware conflict detection in
api/spec/packages/typespec-go/src/components/GoUnion.tsx lines 324-359, and add
a regression fixture covering nested union branches with incompatible field
types; ensure unsafe non-discriminated unions with multiple object variants in
request or success-response bodies are rejected.
Source: Coding guidelines
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
3 participants
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.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Greptile Summary
The PR aligns the spend API contract and generated clients around charge creation, realization metadata, and nested-union handling.
Confidence Score: 5/5
The PR appears safe to merge because no blocking failure remains.
No blocking failure remains.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart LR TSP[TypeSpec charge contract] --> OA[OpenAPI v3 schema] TSP --> TS[TypeScript SDK models] TSP --> GO[Go client models] OA --> H[Charge HTTP handler] H --> C[Domain/API conversion] C --> R[Charge response] R --> W[SDK wire mapping]Reviews (3): Last reviewed commit: "docs(emitter): record the deliberate ULI..." | Re-trigger Greptile
Context used: