feat(api): use camelCase for TypeScript SDK#4622
Open
tothandras wants to merge 5 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds camelCase SDK mapping over snake_case wire payloads, with build-time casing checks, wire-mode schema emission, regenerated SDK request/response handling, and updated docs, tests, and tooling. ChangesWire mapping boundary feature
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant GeneratedSDK
participant toWire
participant assertValid
participant HTTP
participant fromWire
Caller->>GeneratedSDK: call endpoint(req)
GeneratedSDK->>toWire: map camelCase input to snake_case
GeneratedSDK->>assertValid: validate wire payload if enabled
GeneratedSDK->>HTTP: send request
HTTP-->>GeneratedSDK: raw JSON
GeneratedSDK->>assertValid: validate raw wire response if enabled
GeneratedSDK->>fromWire: map wire payload back to camelCase
GeneratedSDK-->>Caller: typed result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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 |
Contributor
Greptile SummaryThis PR changes the TypeScript SDK to use camelCase public shapes over snake_case wire payloads. The main changes are:
Confidence Score: 4/5This is close, but the discriminated response mapping should be fixed before merging.
api/spec/packages/typespec-typescript/src/runtime/wire.ts and the generated SDK runtime copy. Important Files Changed
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
api/spec/packages/typespec-typescript/src/runtime/wire.ts:257-264
**Literal Lookup Still Fails**
`literalValue` still reads the old single-value literal slot. With the generated SDK's Zod v4 dependency, `z.literal(...)` exposes its literal through the literal values collection rather than `schema.value`, so this returns `undefined` for each discriminated-union option. That leaves the discriminator map empty; a reachable response such as `getApp` then cannot select its variant and `fromWire` returns the raw wire object, leaking fields like `account_id` instead of the public `accountId` shape.
```suggestion
function literalValue(schema: ZodType | undefined): unknown {
const s = unwrap(schema)
if (def(s)?.type === 'literal') {
const literal = s as { value?: unknown; values?: Iterable<unknown> }
if (literal.value !== undefined) {
return literal.value
}
return literal.values ? Array.from(literal.values)[0] : undefined
}
/* v8 ignore next -- a discriminated-union variant's discriminator is a literal */
return undefined
}
```
Reviews (6): Last reviewed commit: "chore: review comments" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 11
🧹 Nitpick comments (1)
api/spec/packages/aip-client-javascript/.npmignore (1)
14-19: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso ignore
tests/in the npm package.Nice catch adding the Vitest config and coverage excludes. The new
tests/tree from this PR is still publishable, though, so consumers will get dev-only specs/helpers unless that folder is ignored too.Suggested tweak
vitest.config.ts .gitignore +tests/ # Test coverage output coverage/🤖 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/aip-client-javascript/.npmignore` around lines 14 - 19, The npm package ignore list is missing the new tests tree, so dev-only specs/helpers would still be published. Update the .npmignore entry set alongside vitest.config.ts and coverage/ to also exclude tests/, keeping the package publishable only with runtime files.
🤖 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/AGENTS.md`:
- Around line 200-213: The Markdown in this contract section is broken because
the asterisks in snake*case and \_inside* are being interpreted as emphasis
instead of literal text. Update the AGENTS.md prose near SDKOptions.validate and
request() to use proper inline code or plain Markdown emphasis for the intended
terms, and verify the surrounding wording still reads correctly after replacing
those malformed fragments.
In `@api/spec/packages/aip-client-javascript/src/funcs/apps.ts`:
- Around line 31-35: The listApps response mapping in apps.ts is dropping
server-added fields because fromWire() only keeps schema-defined keys. Update
the listApps transformation to preserve unknown response properties while still
camelizing and validating the known ones via schemas.listAppsResponseWire and
schemas.listAppsResponse, so additive fields remain available to consumers when
client._options.validate is off.
In `@api/spec/packages/aip-client-javascript/src/funcs/plans.ts`:
- Around line 29-38: The listPlans query path is serializing the wire object
without runtime validation, so invalid page/sort/filter values can slip through
when client._options.validate is enabled. Update the listPlans request
construction to validate the wire query object against
schemas.listPlansQueryParams before calling toURLSearchParams, using the same
validation flow used for request bodies so bad inputs fail fast. Use the
existing listPlans query builder, toWire, encodeSort, and
schemas.listPlansQueryParams symbols to place the check in the right spot.
In `@api/spec/packages/aip-client-javascript/src/lib/config.ts`:
- Around line 22-27: The JSDoc for the validation setting currently says
failures “reject,” but the SDK actually surfaces them as a failed Result via
assertValid(...) inside request(() => ...), so callers won’t hit catch handlers.
Update the wording in the config documentation and the emitting template to
describe that validation failures return Result.error/failed Result instead of
rejecting, and keep the description aligned with the assertValid and request
flow.
In `@api/spec/packages/aip-client-javascript/src/lib/wire.ts`:
- Around line 20-22: The helper def currently reads the wrong Zod internals, so
it never returns the schema definition for Zod 4 types. Update def in wire.ts to
read from schema._zod.def instead of schema.def, and keep the rest of the walk()
logic unchanged so object, union, and record payload key renaming paths are
reached.
In `@api/spec/packages/aip-client-javascript/tests/wire-helpers.ts`:
- Around line 115-121: The key collection logic in collectFieldKeys is skipping
the entire subtree for user_key_* entries, which causes nested schema fields to
be missed. Update the loop over Object.entries(value) so preserved user record
keys are not added as schema fields themselves, but their nested values are
still traversed for leak-checking; keep the existing handling in
collectFieldKeys and any caller that relies on the generated keys list.
In `@api/spec/packages/typespec-typescript/src/casing-gate.ts`:
- Around line 68-70: The casing gate is only checking namespace-owned unions
because it iterates userUnions(program), so inline request/response unions are
skipped. Update the union scan in mappedReachableUnions/casing-gate logic to
iterate mappedUnions instead, and keep the existing ambiguous-union and
discriminator/envelope casing checks applied to every mapped union.
In `@api/spec/packages/typespec-typescript/src/emitter.tsx`:
- Around line 102-105: The current casing check in assertCasingDerivable only
verifies that each wire key can be deterministically recovered, but it does not
catch collisions where two different wire names map to the same camelCase public
name. Extend the gate around assertCasingDerivable in emitter.tsx to also detect
duplicate camelized names before emission, and fail the build when a model’s
fields or a query object’s parameters would collapse to the same public key. Use
the existing model and operation traversal to compare the derived public names
for each group and reject any duplicates before the public emitters run.
In `@api/spec/packages/typespec-typescript/src/runtime/wire.ts`:
- Around line 20-22: The literal discriminator lookup is still reading the old
Zod shape, so discriminated union renaming won’t work. Update the discriminator
extraction in the runtime wire helpers by adjusting the `literalValue()` logic
to read from `def.values` via the existing `def()` helper instead of
`schema.value`, keeping the change aligned with `ZodDef` and the `def()`
function.
In `@api/spec/packages/typespec-typescript/src/sdk-files.ts`:
- Around line 6-17: The path templating in pathExpr() no longer preserves the
fail-fast missing-parameter guard from encodePath, so a missing runtime path arg
can be sent as an “undefined”/“null” segment. Update pathExpr() to keep the
inline template literal behavior while still validating required path params
before building the URL, using the same SdkOperation path-param handling that
encodePath previously enforced.
In `@api/spec/packages/typespec-typescript/src/ZodOperations.tsx`:
- Around line 18-23: Make the query-parameter schema generation in ZodOperations
wire-mode aware so the wire pass does not camelCase keys for query params.
Update the logic around paramObject(queryParams, true) and the related query
schema builders (including *QueryParamsWire and the helpers in ZodOperations) to
preserve wire-format names when generating wire schemas, while still using
toCamelCase only for the non-wire schema path. Ensure the fix covers the
query-params validation flow used after toWire(...).
---
Nitpick comments:
In `@api/spec/packages/aip-client-javascript/.npmignore`:
- Around line 14-19: The npm package ignore list is missing the new tests tree,
so dev-only specs/helpers would still be published. Update the .npmignore entry
set alongside vitest.config.ts and coverage/ to also exclude tests/, keeping the
package publishable only with runtime files.
🪄 Autofix (Beta)
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
Run ID: 5b7655d9-228b-4cd6-8ff4-9c0a6feae491
⛔ Files ignored due to path filters (1)
api/spec/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (52)
api/spec/AGENTS.mdapi/spec/package.jsonapi/spec/packages/aip-client-javascript/.gitignoreapi/spec/packages/aip-client-javascript/.npmignoreapi/spec/packages/aip-client-javascript/README.mdapi/spec/packages/aip-client-javascript/src/funcs/addons.tsapi/spec/packages/aip-client-javascript/src/funcs/apps.tsapi/spec/packages/aip-client-javascript/src/funcs/billing.tsapi/spec/packages/aip-client-javascript/src/funcs/currencies.tsapi/spec/packages/aip-client-javascript/src/funcs/customers.tsapi/spec/packages/aip-client-javascript/src/funcs/defaults.tsapi/spec/packages/aip-client-javascript/src/funcs/entitlements.tsapi/spec/packages/aip-client-javascript/src/funcs/events.tsapi/spec/packages/aip-client-javascript/src/funcs/features.tsapi/spec/packages/aip-client-javascript/src/funcs/governance.tsapi/spec/packages/aip-client-javascript/src/funcs/invoices.tsapi/spec/packages/aip-client-javascript/src/funcs/llmCost.tsapi/spec/packages/aip-client-javascript/src/funcs/meters.tsapi/spec/packages/aip-client-javascript/src/funcs/planAddons.tsapi/spec/packages/aip-client-javascript/src/funcs/plans.tsapi/spec/packages/aip-client-javascript/src/funcs/subscriptions.tsapi/spec/packages/aip-client-javascript/src/funcs/tax.tsapi/spec/packages/aip-client-javascript/src/index.tsapi/spec/packages/aip-client-javascript/src/lib/config.tsapi/spec/packages/aip-client-javascript/src/lib/encodings.tsapi/spec/packages/aip-client-javascript/src/lib/wire.tsapi/spec/packages/aip-client-javascript/src/models/operations/tax.tsapi/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip-client-javascript/tests/meters.spec.tsapi/spec/packages/aip-client-javascript/tests/wire-helpers.tsapi/spec/packages/aip-client-javascript/tests/wire.generated.spec.tsapi/spec/packages/aip-client-javascript/tests/wire.spec.tsapi/spec/packages/aip-client-javascript/vitest.config.tsapi/spec/packages/typespec-typescript/package.jsonapi/spec/packages/typespec-typescript/src/ZodOperations.tsxapi/spec/packages/typespec-typescript/src/casing-gate.tsapi/spec/packages/typespec-typescript/src/casing.tsapi/spec/packages/typespec-typescript/src/components/ZodSchema.tsxapi/spec/packages/typespec-typescript/src/components/ZodSchemaDeclaration.tsxapi/spec/packages/typespec-typescript/src/emitter.tsxapi/spec/packages/typespec-typescript/src/interface-types.tsapi/spec/packages/typespec-typescript/src/readme.tsapi/spec/packages/typespec-typescript/src/request-types.tsapi/spec/packages/typespec-typescript/src/runtime-templates.tsapi/spec/packages/typespec-typescript/src/runtime/wire.tsapi/spec/packages/typespec-typescript/src/sdk-files.tsapi/spec/packages/typespec-typescript/src/ts-types.tsapi/spec/packages/typespec-typescript/src/utils.tsxapi/spec/packages/typespec-typescript/src/wire-runtime.tsapi/spec/packages/typespec-typescript/src/zodBaseSchema.tsxapi/spec/packages/typespec-typescript/test/casing.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/aip-client-javascript/src/funcs/customers.ts`:
- Around line 75-80: The path-building IIFE in the customer SDK methods throws
before request(...) is invoked, so missing customerId/creditGrantId escapes the
SDK error wrapper. Move the missing-path validation and path construction inside
the request callback for the affected customer-related operations in
customers.ts so request(...) can return a Promise<Result<...>> consistently. Use
the existing request helper and the generated method bodies as the lookup
points, and apply the same pattern to all other listed endpoint builders in this
file.
In `@api/spec/packages/typespec-typescript/src/runtime/wire.ts`:
- Around line 151-155: The wire mapper is returning null-prototype records
instead of plain objects. Update the record-building logic in the wire
conversion path (the `wire.ts` code that iterates `Object.entries(record)` and
any similar helper around the other flagged location) to use normal plain
objects, ideally by building from entries rather than `Object.create(null)`, so
callers keep standard `Object.prototype` behavior while still preserving
`__proto__` as data.
🪄 Autofix (Beta)
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
Run ID: b0d601d8-ef95-439a-9285-2514d445fb3c
📒 Files selected for processing (22)
api/spec/packages/aip-client-javascript/src/funcs/addons.tsapi/spec/packages/aip-client-javascript/src/funcs/apps.tsapi/spec/packages/aip-client-javascript/src/funcs/billing.tsapi/spec/packages/aip-client-javascript/src/funcs/currencies.tsapi/spec/packages/aip-client-javascript/src/funcs/customers.tsapi/spec/packages/aip-client-javascript/src/funcs/entitlements.tsapi/spec/packages/aip-client-javascript/src/funcs/features.tsapi/spec/packages/aip-client-javascript/src/funcs/invoices.tsapi/spec/packages/aip-client-javascript/src/funcs/llmCost.tsapi/spec/packages/aip-client-javascript/src/funcs/meters.tsapi/spec/packages/aip-client-javascript/src/funcs/planAddons.tsapi/spec/packages/aip-client-javascript/src/funcs/plans.tsapi/spec/packages/aip-client-javascript/src/funcs/subscriptions.tsapi/spec/packages/aip-client-javascript/src/funcs/tax.tsapi/spec/packages/aip-client-javascript/src/index.tsapi/spec/packages/aip-client-javascript/src/lib/wire.tsapi/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/tests/wire.spec.tsapi/spec/packages/typespec-typescript/src/ZodOperations.tsxapi/spec/packages/typespec-typescript/src/casing-gate.tsapi/spec/packages/typespec-typescript/src/runtime/wire.tsapi/spec/packages/typespec-typescript/src/sdk-files.ts
✅ Files skipped from review due to trivial changes (1)
- api/spec/packages/aip-client-javascript/src/index.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- api/spec/packages/aip-client-javascript/src/funcs/entitlements.ts
- api/spec/packages/aip-client-javascript/src/funcs/invoices.ts
- api/spec/packages/aip-client-javascript/src/funcs/apps.ts
- api/spec/packages/typespec-typescript/src/ZodOperations.tsx
- api/spec/packages/aip-client-javascript/src/funcs/addons.ts
- api/spec/packages/typespec-typescript/src/casing-gate.ts
- api/spec/packages/aip-client-javascript/src/funcs/billing.ts
- api/spec/packages/aip-client-javascript/tests/wire.spec.ts
- api/spec/packages/aip-client-javascript/src/funcs/currencies.ts
- api/spec/packages/aip-client-javascript/src/funcs/tax.ts
- api/spec/packages/aip-client-javascript/src/funcs/features.ts
- api/spec/packages/aip-client-javascript/src/funcs/planAddons.ts
- api/spec/packages/aip-client-javascript/src/funcs/subscriptions.ts
- api/spec/packages/aip-client-javascript/src/funcs/llmCost.ts
- api/spec/packages/aip-client-javascript/src/funcs/plans.ts
- api/spec/packages/typespec-typescript/src/sdk-files.ts
- api/spec/packages/aip-client-javascript/src/funcs/meters.ts
tothandras
force-pushed
the
feat/camel-case-sdk
branch
from
2067210 to
ad6e993
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (1)
api/spec/packages/aip-client-javascript/src/funcs/customers.ts (1)
70-92: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPath-param-before-
request()bug still present, despite being marked "Addressed" previously.The past review comment on this file flagged exactly this pattern — path IIFEs (with
missing path parameter: ...throws) running beforerequest(...)— and it's noted as "✅ Addressed in commit 2067210". However, the code shown here still has the same structure across essentially every function with a path parameter (getCustomer,upsertCustomer,deleteCustomer,getCustomerBilling,updateCustomerBilling,updateCustomerBillingAppData, the Stripe session endpoints, all credit-grant/credit-transaction/charges endpoints, etc.). Worth double-checking whether the fix actually landed on this branch or got reverted/rebased away, since it contradicts AGENTS.md's stated design (validation "does not throw synchronously").As per coding guidelines,
api/spec/packages/aip-client-javascript/**: "Never hand-edit the generated SDK output; regenerate it from TypeSpec instead" — the fix needs to land in the emitter template so it propagates to every operation.Also applies to: 124-166, 168-190, 192-218, 220-246, 248-283, 285-314, 316-342, 344-371, 373-406, 408-441, 443-469, 471-511, 513-546, 548-583, 585-611
🤖 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/aip-client-javascript/src/funcs/customers.ts` around lines 70 - 92, The synchronous path-parameter IIFE in getCustomer (and the same pattern across the other generated endpoint functions) still throws before request(), which reintroduces the bug. Move required path-parameter validation out of the path construction so it happens inside the request() flow or via the shared request/validation helper, preserving the “does not throw synchronously” behavior. Since this is generated SDK output, fix the emitter template that generates these functions so the change applies to all affected operations (getCustomer, upsertCustomer, deleteCustomer, billing, Stripe session, credit, and charge endpoints).Source: Coding guidelines
🤖 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/aip-client-javascript/src/funcs/addons.ts`:
- Around line 79-100: Move the path construction in the addon client flow into
the request wrapper so missing addonId errors are returned as Result.error
instead of throwing synchronously. Update the emitter/template used by the
generated funcs (including the addon's update flow in the updateAddon request
path) so the path is built inside request() and any missing path parameter is
handled through the existing error result path rather than before request()
starts.
In `@api/spec/packages/aip-client-javascript/src/funcs/billing.ts`:
- Around line 70-91: Move the missing-id guard out of the eager path निर्माण in
getBillingProfile, updateBillingProfile, deleteBillingProfile, and the similar
generated addons.ts helpers so it runs inside the request() callback instead of
before it. Right now the inline id check in the path template throws
synchronously, bypassing Result.error; restructure these helpers so request()
wraps both path construction and the HTTP call, preserving the existing missing
path parameter: id behavior as a failed Result. Apply this in the shared
generator pattern used by the billing functions and addons helpers, not just a
single function.
In `@api/spec/packages/aip-client-javascript/src/funcs/currencies.ts`:
- Around line 70-103: The `listCostBases` path construction currently happens
before `request(...)`, so the `currencyId` check in that function throws
synchronously instead of returning a `Result.error`. Move the
`openmeter/currencies/custom/.../cost-bases` path build and its `currencyId`
validation inside the `request(() => ...)` callback, following the same pattern
used elsewhere (for example `createCostBasis`), so missing params are handled
through the request wrapper.
In `@api/spec/packages/aip-client-javascript/src/funcs/features.ts`:
- Around line 74-94: The featureId path construction in getFeature,
updateFeature, deleteFeature, and queryFeatureCost is happening before
request(), which causes missing featureId to throw synchronously instead of
becoming a Result.error. Move the path-building logic that uses req.featureId
inside the request() callback in the generated template, keeping the existing
encodeURIComponent and missing-parameter checks there, then regenerate the SDK
so all four methods follow the same error flow.
In `@api/spec/packages/aip-client-javascript/src/funcs/invoices.ts`:
- Around line 47-52: The invoice path template currently only rejects undefined
invoiceId, so null can still be encoded into the URL as “null”. Update the
path-parameter handling in the generated template logic used by sdk-files.ts so
the invoiceId check rejects both null and undefined before calling
encodeURIComponent, then regenerate the invoices client so the path builder in
invoices.ts reflects the new nullish guard.
In `@api/spec/packages/aip-client-javascript/src/funcs/meters.ts`:
- Around line 51-56: The path-parameter validation for meters is happening while
building the path, so `request()` never gets a chance to convert missing params
into `{ ok: false, error }`. Move the path construction and the `meterId` guard
into the `request()` callback in the generator so failures stay inside
`request()`; apply the same change to the other generated meter operations
referenced by this pattern.
In `@api/spec/packages/aip-client-javascript/src/funcs/planAddons.ts`:
- Around line 25-30: The required-parameter path checks are being executed
before the request() wrapper, causing synchronous throws instead of the SDK’s
normalized Result error shape. Move the path construction and its
missing-parameter guard into the callback passed to request() so the error is
raised inside the wrapper for each affected operation in planAddons.ts, using
the existing request() flow and req.planId-style guards. Regenerate the SDK
output from TypeSpec rather than hand-editing the generated file.
In `@api/spec/packages/aip-client-javascript/src/funcs/plans.ts`:
- Around line 81-86: The new `planId` check in `plans.ts` is throwing
synchronously while building `path`, which bypasses the SDK’s `Result` error
handling. Move the `openmeter/plans/...` path construction for the affected plan
methods into the `request()` callback so `missing path parameter: planId` is
returned through the normal error wrapper instead of escaping as a sync throw.
Apply the same fix to the corresponding plan-related methods in this file, using
the existing method bodies and `request()` call sites as the reference points.
In `@api/spec/packages/aip-client-javascript/src/funcs/subscriptions.ts`:
- Around line 85-90: The subscription path construction is happening before
request(), so missing IDs throw synchronously instead of being wrapped as { ok:
false, error }. Move the path building and required subscriptionId checks into
the request() callback for the affected subscription methods in
subscriptions.ts, using the existing request() flow and symbols like request(),
subscriptionId, and the generated path builders. Do not hand-edit the generated
SDK output; regenerate it from TypeSpec so the path-param validation occurs
inside the SDK error wrapper.
In `@api/spec/packages/aip-client-javascript/src/funcs/tax.ts`:
- Around line 47-52: The `taxCodeId` missing-ID check is happening while
building the path, so it throws before `request()` and bypasses the SDK `Result`
flow. Move the `openmeter/tax-codes/...` path construction into the `request()`
callback in the affected tax functions (including the similar blocks at the
other referenced spots) so validation happens inside the generated request
handling. Keep this change in the TypeSpec source and regenerate the SDK output
rather than editing the generated `tax.ts` by hand.
---
Duplicate comments:
In `@api/spec/packages/aip-client-javascript/src/funcs/customers.ts`:
- Around line 70-92: The synchronous path-parameter IIFE in getCustomer (and the
same pattern across the other generated endpoint functions) still throws before
request(), which reintroduces the bug. Move required path-parameter validation
out of the path construction so it happens inside the request() flow or via the
shared request/validation helper, preserving the “does not throw synchronously”
behavior. Since this is generated SDK output, fix the emitter template that
generates these functions so the change applies to all affected operations
(getCustomer, upsertCustomer, deleteCustomer, billing, Stripe session, credit,
and charge endpoints).
🪄 Autofix (Beta)
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
Run ID: 03eeec18-a717-4b67-bb89-8794b7b599c4
⛔ Files ignored due to path filters (1)
api/spec/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (52)
api/spec/AGENTS.mdapi/spec/package.jsonapi/spec/packages/aip-client-javascript/.gitignoreapi/spec/packages/aip-client-javascript/.npmignoreapi/spec/packages/aip-client-javascript/README.mdapi/spec/packages/aip-client-javascript/src/funcs/addons.tsapi/spec/packages/aip-client-javascript/src/funcs/apps.tsapi/spec/packages/aip-client-javascript/src/funcs/billing.tsapi/spec/packages/aip-client-javascript/src/funcs/currencies.tsapi/spec/packages/aip-client-javascript/src/funcs/customers.tsapi/spec/packages/aip-client-javascript/src/funcs/defaults.tsapi/spec/packages/aip-client-javascript/src/funcs/entitlements.tsapi/spec/packages/aip-client-javascript/src/funcs/events.tsapi/spec/packages/aip-client-javascript/src/funcs/features.tsapi/spec/packages/aip-client-javascript/src/funcs/governance.tsapi/spec/packages/aip-client-javascript/src/funcs/invoices.tsapi/spec/packages/aip-client-javascript/src/funcs/llmCost.tsapi/spec/packages/aip-client-javascript/src/funcs/meters.tsapi/spec/packages/aip-client-javascript/src/funcs/planAddons.tsapi/spec/packages/aip-client-javascript/src/funcs/plans.tsapi/spec/packages/aip-client-javascript/src/funcs/subscriptions.tsapi/spec/packages/aip-client-javascript/src/funcs/tax.tsapi/spec/packages/aip-client-javascript/src/index.tsapi/spec/packages/aip-client-javascript/src/lib/config.tsapi/spec/packages/aip-client-javascript/src/lib/encodings.tsapi/spec/packages/aip-client-javascript/src/lib/wire.tsapi/spec/packages/aip-client-javascript/src/models/operations/tax.tsapi/spec/packages/aip-client-javascript/src/models/schemas.tsapi/spec/packages/aip-client-javascript/src/models/types.tsapi/spec/packages/aip-client-javascript/tests/meters.spec.tsapi/spec/packages/aip-client-javascript/tests/wire-helpers.tsapi/spec/packages/aip-client-javascript/tests/wire.generated.spec.tsapi/spec/packages/aip-client-javascript/tests/wire.spec.tsapi/spec/packages/aip-client-javascript/vitest.config.tsapi/spec/packages/typespec-typescript/package.jsonapi/spec/packages/typespec-typescript/src/ZodOperations.tsxapi/spec/packages/typespec-typescript/src/casing-gate.tsapi/spec/packages/typespec-typescript/src/casing.tsapi/spec/packages/typespec-typescript/src/components/ZodSchema.tsxapi/spec/packages/typespec-typescript/src/components/ZodSchemaDeclaration.tsxapi/spec/packages/typespec-typescript/src/emitter.tsxapi/spec/packages/typespec-typescript/src/interface-types.tsapi/spec/packages/typespec-typescript/src/readme.tsapi/spec/packages/typespec-typescript/src/request-types.tsapi/spec/packages/typespec-typescript/src/runtime-templates.tsapi/spec/packages/typespec-typescript/src/runtime/wire.tsapi/spec/packages/typespec-typescript/src/sdk-files.tsapi/spec/packages/typespec-typescript/src/ts-types.tsapi/spec/packages/typespec-typescript/src/utils.tsxapi/spec/packages/typespec-typescript/src/wire-runtime.tsapi/spec/packages/typespec-typescript/src/zodBaseSchema.tsxapi/spec/packages/typespec-typescript/test/casing.test.ts
✅ Files skipped from review due to trivial changes (3)
- api/spec/packages/aip-client-javascript/README.md
- api/spec/packages/aip-client-javascript/.gitignore
- api/spec/packages/typespec-typescript/src/readme.ts
🚧 Files skipped from review as they are similar to previous changes (34)
- api/spec/packages/aip-client-javascript/vitest.config.ts
- api/spec/packages/aip-client-javascript/src/index.ts
- api/spec/packages/typespec-typescript/src/interface-types.ts
- api/spec/packages/aip-client-javascript/src/funcs/governance.ts
- api/spec/packages/aip-client-javascript/src/lib/config.ts
- api/spec/packages/typespec-typescript/src/wire-runtime.ts
- api/spec/packages/aip-client-javascript/src/funcs/entitlements.ts
- api/spec/packages/typespec-typescript/test/casing.test.ts
- api/spec/packages/typespec-typescript/src/ts-types.ts
- api/spec/packages/aip-client-javascript/src/lib/encodings.ts
- api/spec/packages/aip-client-javascript/src/funcs/apps.ts
- api/spec/packages/aip-client-javascript/tests/meters.spec.ts
- api/spec/packages/typespec-typescript/src/components/ZodSchema.tsx
- api/spec/packages/typespec-typescript/src/request-types.ts
- api/spec/packages/typespec-typescript/src/casing.ts
- api/spec/packages/typespec-typescript/src/components/ZodSchemaDeclaration.tsx
- api/spec/packages/aip-client-javascript/src/funcs/events.ts
- api/spec/packages/typespec-typescript/package.json
- api/spec/packages/typespec-typescript/src/runtime-templates.ts
- api/spec/packages/aip-client-javascript/.npmignore
- api/spec/packages/aip-client-javascript/tests/wire-helpers.ts
- api/spec/packages/aip-client-javascript/src/models/operations/tax.ts
- api/spec/packages/typespec-typescript/src/utils.tsx
- api/spec/packages/aip-client-javascript/tests/wire.generated.spec.ts
- api/spec/packages/typespec-typescript/src/zodBaseSchema.tsx
- api/spec/packages/typespec-typescript/src/runtime/wire.ts
- api/spec/packages/typespec-typescript/src/casing-gate.ts
- api/spec/packages/aip-client-javascript/src/funcs/llmCost.ts
- api/spec/packages/aip-client-javascript/src/funcs/defaults.ts
- api/spec/packages/typespec-typescript/src/ZodOperations.tsx
- api/spec/packages/typespec-typescript/src/emitter.tsx
- api/spec/packages/aip-client-javascript/src/lib/wire.ts
- api/spec/packages/aip-client-javascript/tests/wire.spec.ts
- api/spec/packages/typespec-typescript/src/sdk-files.ts
Address remaining valid PR review findings: path params are now validated inside the request() closure so a missing required param returns Result.error instead of throwing synchronously; the casing gate now walks every union reachable from an operation (not just namespace-owned ones) and rejects wire keys that collapse onto the same camelCase name; fixed a markdown formatting nit in AGENTS.md. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019cySbKnjMmWt4VzL7Dgf56
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
api/spec/packages/typespec-typescript/src/casing-gate.ts (2)
46-174: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winErrors throw one bucket at a time — later checks never even run.
collisionsthrows right after the model/operation loops (Line 90), beforemappedReachableTypesis even computed. So if a build hits a collision, the intersection-model check, ambiguous-union check, and the discriminator/envelopecheck()calls inside the union loop never execute in that run. Devs fix one category, rerun, hit the next — a bit of a whack-a-mole cycle for what could be a single combined error report.Consider collecting all four categories first, then throwing one aggregated error (or an
AggregateError) at the end so a single build attempt surfaces everything.💡 Sketch of aggregation approach
- if (collisions.length > 0) { - throw new Error( - `camelCase SDK: ${collisions.length} wire key group(s) collapse onto the ` + - `same camelCase name. Rename one side or add an explicit override.\n ${collisions.join('\n ')}`, - ) - } + const errorSections: string[] = [] + if (collisions.length > 0) { + errorSections.push( + `${collisions.length} wire key group(s) collapse onto the same camelCase name...\n ${collisions.join('\n ')}`, + ) + } // ... continue accumulating intersectionModels / ambiguousUnions / violations + if (errorSections.length > 0) { + throw new Error(`camelCase SDK:\n\n${errorSections.join('\n\n')}`) + }🤖 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 46 - 174, The validation in casing-gate.ts currently throws as soon as it finds the first category of issue, so later checks never run. Update the main flow in the casing-gate logic around checkNoCollisions, mappedReachableTypes, the mappedUnions loop, and the final violations check to collect all errors first and throw once at the end. Keep the existing symbols and categories (collisions, intersectionModels, ambiguousUnions, violations) but aggregate their messages into one combined error so a single build reports every casing problem at once.
68-74: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTighten
wireNametoModelProperty.bodyProperties()already returnsModelProperty[], soprop as Type & { name: string }is just noise here. NarrowingwireNametoModelPropertywould let you drop the cast.🤖 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 68 - 74, The `casing-gate.ts` loop is using an unnecessary cast because `bodyProperties()` already yields `ModelProperty` values. Update the `wireName` call site in the `models`/`bodyProperties()` iteration to accept `ModelProperty` directly, and adjust `wireName`’s parameter type accordingly so the `prop as Type & { name: string }` cast can be removed cleanly.
🤖 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.
Nitpick comments:
In `@api/spec/packages/typespec-typescript/src/casing-gate.ts`:
- Around line 46-174: The validation in casing-gate.ts currently throws as soon
as it finds the first category of issue, so later checks never run. Update the
main flow in the casing-gate logic around checkNoCollisions,
mappedReachableTypes, the mappedUnions loop, and the final violations check to
collect all errors first and throw once at the end. Keep the existing symbols
and categories (collisions, intersectionModels, ambiguousUnions, violations) but
aggregate their messages into one combined error so a single build reports every
casing problem at once.
- Around line 68-74: The `casing-gate.ts` loop is using an unnecessary cast
because `bodyProperties()` already yields `ModelProperty` values. Update the
`wireName` call site in the `models`/`bodyProperties()` iteration to accept
`ModelProperty` directly, and adjust `wireName`’s parameter type accordingly so
the `prop as Type & { name: string }` cast can be removed cleanly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3250f23b-da69-45b8-bc1a-eb6eee553871
📒 Files selected for processing (17)
api/spec/AGENTS.mdapi/spec/packages/aip-client-javascript/src/funcs/addons.tsapi/spec/packages/aip-client-javascript/src/funcs/apps.tsapi/spec/packages/aip-client-javascript/src/funcs/billing.tsapi/spec/packages/aip-client-javascript/src/funcs/currencies.tsapi/spec/packages/aip-client-javascript/src/funcs/customers.tsapi/spec/packages/aip-client-javascript/src/funcs/entitlements.tsapi/spec/packages/aip-client-javascript/src/funcs/features.tsapi/spec/packages/aip-client-javascript/src/funcs/invoices.tsapi/spec/packages/aip-client-javascript/src/funcs/llmCost.tsapi/spec/packages/aip-client-javascript/src/funcs/meters.tsapi/spec/packages/aip-client-javascript/src/funcs/planAddons.tsapi/spec/packages/aip-client-javascript/src/funcs/plans.tsapi/spec/packages/aip-client-javascript/src/funcs/subscriptions.tsapi/spec/packages/aip-client-javascript/src/funcs/tax.tsapi/spec/packages/typespec-typescript/src/casing-gate.tsapi/spec/packages/typespec-typescript/src/sdk-files.ts
🚧 Files skipped from review as they are similar to previous changes (16)
- api/spec/packages/aip-client-javascript/src/funcs/invoices.ts
- api/spec/packages/aip-client-javascript/src/funcs/entitlements.ts
- api/spec/packages/aip-client-javascript/src/funcs/currencies.ts
- api/spec/packages/aip-client-javascript/src/funcs/meters.ts
- api/spec/packages/aip-client-javascript/src/funcs/planAddons.ts
- api/spec/AGENTS.md
- api/spec/packages/aip-client-javascript/src/funcs/apps.ts
- api/spec/packages/aip-client-javascript/src/funcs/features.ts
- api/spec/packages/aip-client-javascript/src/funcs/subscriptions.ts
- api/spec/packages/aip-client-javascript/src/funcs/plans.ts
- api/spec/packages/aip-client-javascript/src/funcs/billing.ts
- api/spec/packages/aip-client-javascript/src/funcs/tax.ts
- api/spec/packages/aip-client-javascript/src/funcs/addons.ts
- api/spec/packages/aip-client-javascript/src/funcs/customers.ts
- api/spec/packages/aip-client-javascript/src/funcs/llmCost.ts
- api/spec/packages/typespec-typescript/src/sdk-files.ts
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
1 participant
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
SDKOptions.validate, returningResult.errorwithValidationError.DepthLimitExceededErrorfor deeply nested wire mapping.