Merged
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:
📝 WalkthroughWalkthroughAdds a CreditAdjustment model and create endpoint, removes the void-credit-grant operation, introduces a credit-grant billing service with Create/Get/List, adds handlers (list/create/get), updates generated API/server routing, and wires the new service through DI and router configs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant V3Server as V3 Server
participant Handler as customersCredits Handler
participant CreditGrantSvc as creditgrant.Service
participant ChargesSvc as creditpurchase/Charges
participant CustomerSvc as customer.Service
Client->>V3Server: POST /customers/{id}/credits/grants
V3Server->>Handler: CreateCreditGrant(params, body)
Handler->>CreditGrantSvc: Create(CreateInput)
CreditGrantSvc->>CustomerSvc: GetCustomer(namespace, customerId)
CreditGrantSvc->>ChargesSvc: Create creditpurchase.Intent/Charge
ChargesSvc-->>CreditGrantSvc: creditpurchase.Charge
CreditGrantSvc-->>Handler: creditpurchase.Charge
Handler-->>V3Server: 201 Created (BillingCreditGrant)
V3Server-->>Client: HTTP 201 response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
turip
reviewed
Apr 2, 2026
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v3/api.gen.go (1)
3147-3212:⚠️ Potential issue | 🟡 MinorThe grant lifecycle docs are now out of sync with the public schema.
This request no longer exposes timing fields, but the generated
BillingCreditGrantStatusdocs still describepending/active/expiredin terms ofeffective_atandexpires_at. That makes the published contract confusing for SDK consumers. Please fix the source spec comments and regenerate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/api.gen.go` around lines 3147 - 3212, The generated docs for BillingCreditGrantStatus are out-of-date because CreateCreditGrantRequest no longer exposes timing fields (effective_at, expires_at); update the source OpenAPI/spec comments for BillingCreditGrantStatus (and any related schema comments) to remove references to effective_at/expires_at and instead describe the new lifecycle semantics (e.g., how pending/active/expired are determined without those fields), then regenerate api.gen.go so the comment block above type BillingCreditGrantStatus and any references in CreateCreditGrantRequest/Purchase/TaxConfig match the current public schema.
🧹 Nitpick comments (5)
openmeter/billing/creditgrant/service.go (1)
97-105: Avoid silently accepting unused purchase termsOn Line 97-Line 105,
Purchaseis allowed even whenFundingMethodisnone, but those fields are later ignored. I’d recommend rejecting that combination to keep API behavior explicit and predictable.Suggested validation tweak
if i.FundingMethod != FundingMethodNone && i.Purchase == nil { errs = append(errs, errors.New("purchase terms are required for funded grants")) } + +if i.FundingMethod == FundingMethodNone && i.Purchase != nil { + errs = append(errs, errors.New("purchase terms must be empty when funding method is none")) +} if i.Purchase != nil { if err := i.Purchase.Currency.Validate(); err != nil { errs = append(errs, fmt.Errorf("purchase currency: %w", err)) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/creditgrant/service.go` around lines 97 - 105, The validation currently allows i.Purchase when i.FundingMethod == FundingMethodNone and later ignores those fields; update the validation in the same function (where i is validated) to reject the combination by appending an error when i.FundingMethod == FundingMethodNone && i.Purchase != nil (e.g., "purchase terms not allowed for non-funded grants"), and keep the existing currency validation but guard it so it only runs when i.Purchase != nil and i.FundingMethod != FundingMethodNone to avoid validating ignored fields.openmeter/server/router/router.go (1)
138-248: Add an explicit credits-enabled guard forCreditGrantServiceSince
CreditGrantServiceis now part ofConfig, it’d be safer to enforce: if credits are enabled, the service must be non-nil. That makes failures deterministic at startup instead of later in request handling.Based on learnings Ensure
credits.enabledsetting is explicitly guarded at multiple layers: ledger-backed customer credit handlers inapi/v3/server, customer ledger hooks, and namespace/default-account provisioning; these are wired separately and must each stay disabled when credits are offSuggested guard in config validation
func (c Config) Validate() error { + if c.Credits.Enabled && c.CreditGrantService == nil { + return errors.New("credit grant service is required when credits are enabled") + } + if c.NamespaceManager == nil { return errors.New("namespace manager is required") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/server/router/router.go` around lines 138 - 248, Config.Validate must explicitly guard CreditGrantService when credits are enabled: add a check in Config.Validate that returns an error if the credits.enabled flag is true and c.CreditGrantService is nil. Implement this by reading your config flag (e.g., a boolean field like c.CreditsEnabled or a nested c.Credits.Enabled) and if it is true and CreditGrantService == nil, return errors.New("credit grant service is required when credits are enabled"); keep the check near the other connector/service validations in the Validate() method.api/spec/packages/aip/src/customers/credits/adjustment.tsp (1)
16-23: Clarify adjustment sign semantics in the field docsTiny docs nit: on Line 16 and Line 22, “granted” reads like only positive grants. For an adjustment endpoint, consider wording like “adjusted amount” and (if true) explicitly state positive vs negative behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/packages/aip/src/customers/credits/adjustment.tsp` around lines 16 - 23, Update the field JSDoc to clarify sign semantics: change "The currency of the granted credits." (currency: Currencies.CurrencyCode) to indicate it's the currency for the adjustment (e.g., "The currency of the adjusted credits."), and replace "Granted credit amount." on the adjustment amount field with wording like "Adjusted amount; positive values grant credits, negative values deduct credits." Ensure you update only the documentation comments for the relevant fields (currency and the adjustment amount) in the adjustment.tsp model.api/spec/packages/aip/src/customers/credits/grant.tsp (1)
258-279: Consider removing (not commenting) deprecated contract fields.Keeping full commented-out schema blocks in the TypeSpec source makes the contract harder to read and maintain. A short changelog note/PR reference is usually cleaner than inline commented definitions.
As per coding guidelines: "Review the TypeSpec code for conformity with TypeSpec best practices."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/packages/aip/src/customers/credits/grant.tsp` around lines 258 - 279, Remove the commented-out deprecated schema fields (effective_at, expires_after, expires_at) from the TypeSpec source so the contract is not cluttered with dead code; delete the entire commented blocks that reference those symbols and, if needed, add a short changelog/PR note near the top of the file or in the PR description documenting the removal rather than leaving inline commented definitions.api/spec/packages/aip/src/customers/credits/operations.tsp (1)
81-98: Can we remove the old void operation instead of parking it as comments?Keeping a full removed endpoint in the TypeSpec source gets stale fast and makes the contract a bit harder to trust. I’d just delete it and let git/history preserve the old shape.
As per coding guidelines, "Define API specifications using TypeSpec in
api/spec/as the source of truth for API contracts, then regenerate OpenAPI and SDKs withmake gen-api."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/packages/aip/src/customers/credits/operations.tsp` around lines 81 - 98, Remove the commented-out legacy endpoint block for CustomerCreditGrantVoidOperations (the commented interface and its voidGrant method including `@post/`@operationId/@summary annotations) from the TypeSpec file so the API spec no longer contains stale commented endpoints; rely on git history to retain the previous shape and then run the usual generation step (make gen-api) to update derived artifacts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v3/handlers/customers/credits/convert.go`:
- Around line 52-60: The convertGrantStatus function currently maps only
"created" and "active" and defaults everything else to
api.BillingCreditGrantStatusActive; update
convertGrantStatus(creditpurchase.Charge) to explicitly map all known domain
statuses (at minimum "active", "created", and "voided") to their corresponding
api.BillingCreditGrantStatus values and stop silently defaulting unknown strings
to Active — change the function to return (api.BillingCreditGrantStatus, error)
(or otherwise return a clearly invalid/unknown enum value) and return an error
for any unrecognized charge.Status so callers fail closed instead of
misreporting unknown states.
- Around line 244-256: The parsed per_unit_cost_basis must be validated as >0;
update validation to reject zero/negative values by adding an IsPositive() check
after alpacadecimal.NewFromString(...) and setting purchase.PerUnitCostBasis (or
centralize it in creditgrant.CreateInput.Validate()); specifically, in the
handler around the block that parses body.Purchase.PerUnitCostBasis ensure you
call costBasis.IsPositive() (or equivalent) and return an error like
"per_unit_cost_basis must be positive" if false, or alternatively add the same
positive check inside creditgrant.CreateInput.Validate() so CreateInput and
purchase.PerUnitCostBasis cannot be zero/negative.
In `@api/v3/handlers/customers/credits/get_grant.go`:
- Around line 17-20: GetCreditGrantParams currently includes CustomerID but that
value isn't used: update the handler that builds creditgrant.GetInput (and
related lookups used by GetCreditGrant handler) to pass args.CustomerID into the
service query or, after fetching the grant via creditgrant.GetInput (which
currently uses Namespace + ChargeID), add an ownership check comparing the
returned grant's CustomerID to args.CustomerID and return a not-found/forbidden
error if they differ; specifically modify where creditgrant.GetInput is
constructed and where the service returns the grant to either include CustomerID
in the input or validate ownership before returning in the GetCreditGrant
handler (also apply the same fix to the other similar handler referenced around
lines 32-35).
In `@api/v3/server/routes.go`:
- Around line 372-374: The CreateCreditAdjustment handler currently delegates to
unimplemented.CreateCreditAdjustment, exposing a 501 for the public API; either
remove/hide the route from the published spec or implement the handler now:
replace the unimplemented call in Server.CreateCreditAdjustment with the real
logic (e.g., parse request body, validate customerId, call your service/repo
method such as s.Service.CreateCreditAdjustment or s.AdjustmentStore.Create,
handle errors and write appropriate HTTP responses), or if staged, remove the
public registration so the contract matches implementation.
- Around line 337-347: The grant route currently only checks
s.customersCreditsHandler but can still panic if the CreditGrantService is nil;
update the guard in ListCreditGrants (and the other grant handlers around the
same area) to require both s.customersCreditsHandler != nil AND
s.CreditGrantService != nil before calling
customersCreditsHandler.ListCreditGrants().With(...).ServeHTTP; if either is
nil, call unimplemented.ListCreditGrants(w, r, customerId, params) (or the
corresponding unimplemented handler) and return. Ensure you apply the same
pattern to the other grant routes referenced (the blocks around lines 349–358
and 360–370) so grant-specific handlers are gated separately from balance
handlers.
In `@openmeter/billing/creditgrant/service/service.go`:
- Around line 126-128: The current call to s.chargesService.ListCharges(ctx,
listInput) ignores the FundingMethod, Status, and Currency filters; add a
fail-fast guard before invoking ListCharges that validates listInput (check the
fields named FundingMethod, Status, and Currency on the request/listInput struct
used by service.go) and return a clear error when any of these filters are set
(non-nil or non-empty) so callers are not silently given unfiltered results;
keep the TODO comment and replace the guard with real filtering logic once
ListCharges supports those filters.
---
Outside diff comments:
In `@api/v3/api.gen.go`:
- Around line 3147-3212: The generated docs for BillingCreditGrantStatus are
out-of-date because CreateCreditGrantRequest no longer exposes timing fields
(effective_at, expires_at); update the source OpenAPI/spec comments for
BillingCreditGrantStatus (and any related schema comments) to remove references
to effective_at/expires_at and instead describe the new lifecycle semantics
(e.g., how pending/active/expired are determined without those fields), then
regenerate api.gen.go so the comment block above type BillingCreditGrantStatus
and any references in CreateCreditGrantRequest/Purchase/TaxConfig match the
current public schema.
---
Nitpick comments:
In `@api/spec/packages/aip/src/customers/credits/adjustment.tsp`:
- Around line 16-23: Update the field JSDoc to clarify sign semantics: change
"The currency of the granted credits." (currency: Currencies.CurrencyCode) to
indicate it's the currency for the adjustment (e.g., "The currency of the
adjusted credits."), and replace "Granted credit amount." on the adjustment
amount field with wording like "Adjusted amount; positive values grant credits,
negative values deduct credits." Ensure you update only the documentation
comments for the relevant fields (currency and the adjustment amount) in the
adjustment.tsp model.
In `@api/spec/packages/aip/src/customers/credits/grant.tsp`:
- Around line 258-279: Remove the commented-out deprecated schema fields
(effective_at, expires_after, expires_at) from the TypeSpec source so the
contract is not cluttered with dead code; delete the entire commented blocks
that reference those symbols and, if needed, add a short changelog/PR note near
the top of the file or in the PR description documenting the removal rather than
leaving inline commented definitions.
In `@api/spec/packages/aip/src/customers/credits/operations.tsp`:
- Around line 81-98: Remove the commented-out legacy endpoint block for
CustomerCreditGrantVoidOperations (the commented interface and its voidGrant
method including `@post/`@operationId/@summary annotations) from the TypeSpec file
so the API spec no longer contains stale commented endpoints; rely on git
history to retain the previous shape and then run the usual generation step
(make gen-api) to update derived artifacts.
In `@openmeter/billing/creditgrant/service.go`:
- Around line 97-105: The validation currently allows i.Purchase when
i.FundingMethod == FundingMethodNone and later ignores those fields; update the
validation in the same function (where i is validated) to reject the combination
by appending an error when i.FundingMethod == FundingMethodNone && i.Purchase !=
nil (e.g., "purchase terms not allowed for non-funded grants"), and keep the
existing currency validation but guard it so it only runs when i.Purchase != nil
and i.FundingMethod != FundingMethodNone to avoid validating ignored fields.
In `@openmeter/server/router/router.go`:
- Around line 138-248: Config.Validate must explicitly guard CreditGrantService
when credits are enabled: add a check in Config.Validate that returns an error
if the credits.enabled flag is true and c.CreditGrantService is nil. Implement
this by reading your config flag (e.g., a boolean field like c.CreditsEnabled or
a nested c.Credits.Enabled) and if it is true and CreditGrantService == nil,
return errors.New("credit grant service is required when credits are enabled");
keep the check near the other connector/service validations in the Validate()
method.
🪄 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: 943aa1d2-1be4-43f8-8813-fcd427e160b4
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (23)
api/spec/packages/aip/src/customers/credits/adjustment.tspapi/spec/packages/aip/src/customers/credits/grant.tspapi/spec/packages/aip/src/customers/credits/operations.tspapi/spec/packages/aip/src/konnect.tspapi/spec/packages/aip/src/openmeter.tspapi/types/entitlement.goapi/v3/api.gen.goapi/v3/handlers/customers/credits/convert.goapi/v3/handlers/customers/credits/create_grant.goapi/v3/handlers/customers/credits/get_balance.goapi/v3/handlers/customers/credits/get_grant.goapi/v3/handlers/customers/credits/handler.goapi/v3/handlers/customers/credits/list_grants.goapi/v3/server/routes.goapi/v3/server/server.goapp/common/creditgrant.gocmd/server/main.gocmd/server/wire.gocmd/server/wire_gen.goopenmeter/billing/creditgrant/service.goopenmeter/billing/creditgrant/service/service.goopenmeter/server/router/router.goopenmeter/server/server.go
💤 Files with no reviewable changes (1)
- api/types/entitlement.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v3/api.gen.go (1)
310-323:⚠️ Potential issue | 🟡 MinorEnum docs and actual values are out of sync
Nice cleanup on the enum values, but the type docs still mention removed values (
on_authorization,on_settlement) at Line 1809 and Line 1810. That can mislead SDK/API consumers.Please fix this in the OpenAPI/TypeSpec source and regenerate this file (rather than editing this file directly).
As per coding guidelines, all generated files must have
// Code generated by X, DO NOT EDIT.headers at the top; never edit files bearing this header.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/api.gen.go` around lines 310 - 323, The generated enum docs for BillingCreditAvailabilityPolicy are stale—remove the references to the removed values ("on_authorization", "on_settlement") from the OpenAPI/TypeSpec source where BillingCreditAvailabilityPolicy is defined, ensure the enum definition only contains the current member (BillingCreditAvailabilityPolicyOnCreation) and update its description accordingly, then regenerate api.gen.go so the docblock and enum values stay in sync and the file contains the required "// Code generated by ..." header; do not edit api.gen.go directly.
🧹 Nitpick comments (2)
api/v3/handlers/customers/credits/convert.go (1)
134-143: Handle allpayment.Statusvalues explicitly for robustness.The code does sensibly default unknown payment statuses to
Pending, but you might want to addpayment.StatusOpen(or similar) to the switch if it exists in the domain. Currently:
Authorized→ AuthorizedSettled→ Settled- Everything else → Pending
If other statuses exist (e.g.,
StatusFailed,StatusOpen), they'd silently becomePending.openmeter/billing/creditgrant/service.go (1)
144-186: ListInput validation looks good, but optional filters are silently accepted.The validation properly checks required fields and validates optional filters when present. However, I noticed from reviewing the service implementation that
FundingMethod,Status, andCurrencyfilters pass validation here but are currently ignored downstream (see the TODO at line 137 inservice/service.go).Since validation passes but the filters have no effect, clients might not realize their filters aren't being applied. Consider either:
- Rejecting unsupported filters in validation until implemented
- Documenting this limitation clearly
This is a behavior quirk rather than a correctness bug, so flagging for awareness.
Optional: Fail fast on unsupported filters
func (i ListInput) Validate() error { var errs []error if i.Namespace == "" { errs = append(errs, errors.New("namespace is required")) } if i.CustomerID == "" { errs = append(errs, errors.New("customer ID is required")) } + // Filters are validated but not yet supported downstream + if i.FundingMethod != nil || i.Status != nil || i.Currency != nil { + errs = append(errs, errors.New("list filters are not yet supported")) + } + - if i.FundingMethod != nil { - if err := i.FundingMethod.Validate(); err != nil { - errs = append(errs, err) - } - } - - if i.Status != nil { - if err := i.Status.Validate(); err != nil { - errs = append(errs, err) - } - } - - if i.Currency != nil { - if err := i.Currency.Validate(); err != nil { - errs = append(errs, fmt.Errorf("currency: %w", err)) - } - } return models.NewNillableGenericValidationError(errors.Join(errs...)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/creditgrant/service.go` around lines 144 - 186, ListInput.Validate currently accepts optional filters (FundingMethod, Status, Currency) but those filters are ignored downstream; to fail fast, update ListInput.Validate (the Validate method on the ListInput type) to reject unsupported filters by appending validation errors when FundingMethod, Status, or Currency are non-nil (e.g., errors.New("funding method filter not supported"), errors.New("status filter not supported"), errors.New("currency filter not supported")), then return them via models.NewNillableGenericValidationError(errors.Join(errs...)); this ensures callers get a clear validation error instead of silently ignored filters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v3/api.gen.go`:
- Around line 8830-8832: Add the same guard used by other credit endpoints to
the CreateCreditAdjustment handler: detect if s.customersCreditsHandler == nil
and if so call unimplemented.CreateCreditAdjustment(w, r, customerId) and
return; modify the CreateCreditAdjustment implementation (the handler that
currently unconditionally calls unimplemented.CreateCreditAdjustment) to perform
this nil-check before delegating to s.customersCreditsHandler so the
credits.enabled feature flag gates the endpoint consistently.
---
Outside diff comments:
In `@api/v3/api.gen.go`:
- Around line 310-323: The generated enum docs for
BillingCreditAvailabilityPolicy are stale—remove the references to the removed
values ("on_authorization", "on_settlement") from the OpenAPI/TypeSpec source
where BillingCreditAvailabilityPolicy is defined, ensure the enum definition
only contains the current member (BillingCreditAvailabilityPolicyOnCreation) and
update its description accordingly, then regenerate api.gen.go so the docblock
and enum values stay in sync and the file contains the required "// Code
generated by ..." header; do not edit api.gen.go directly.
---
Nitpick comments:
In `@openmeter/billing/creditgrant/service.go`:
- Around line 144-186: ListInput.Validate currently accepts optional filters
(FundingMethod, Status, Currency) but those filters are ignored downstream; to
fail fast, update ListInput.Validate (the Validate method on the ListInput type)
to reject unsupported filters by appending validation errors when FundingMethod,
Status, or Currency are non-nil (e.g., errors.New("funding method filter not
supported"), errors.New("status filter not supported"), errors.New("currency
filter not supported")), then return them via
models.NewNillableGenericValidationError(errors.Join(errs...)); this ensures
callers get a clear validation error instead of silently ignored filters.
🪄 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: a311604d-4393-4a84-9acf-9febf3eb8a31
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (8)
api/spec/packages/aip/src/customers/credits/grant.tspapi/v3/api.gen.goapi/v3/handlers/customers/credits/convert.goapi/v3/handlers/customers/credits/get_grant.goapi/v3/server/routes.goopenmeter/billing/creditgrant/service.goopenmeter/billing/creditgrant/service/service.goopenmeter/ledger/chargeadapter/creditpurchase.go
🚧 Files skipped from review as they are similar to previous changes (2)
- api/spec/packages/aip/src/customers/credits/grant.tsp
- api/v3/handlers/customers/credits/get_grant.go
tothandras
force-pushed
the
feat/api-v3-credit-grants
branch
from
d384629 to
bd29012
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api/v3/api.gen.go (2)
3121-3143:⚠️ Potential issue | 🟡 MinorUse adjustment-specific field descriptions here.
CreateCreditAdjustmentRequest.Amountand.Currencystill describe "granted credits", so the new adjustment payload reads like a grant request in the generated docs. Easy thing to misread on the client side; please update the source descriptions and regenerate.As per coding guidelines, "All generated files must have
// Code generated by X, DO NOT EDIT.headers at the top; never edit files bearing this header".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/api.gen.go` around lines 3121 - 3143, The generated struct CreateCreditAdjustmentRequest has misleading field docs for Amount and Currency (they reference "granted credits"); update the source model/comments that define CreateCreditAdjustmentRequest.Amount and CreateCreditAdjustmentRequest.Currency in the generator input (not the generated file) to describe adjustment-specific semantics (e.g., adjustment amount and adjustment currency) and then re-run the codegen to regenerate api.gen.go so the correct descriptions appear; also ensure the generated file retains the required "// Code generated by X, DO NOT EDIT." header.
310-323:⚠️ Potential issue | 🟡 MinorRegenerate the credit docs from the updated source contract.
These changes narrow
BillingCreditAvailabilityPolicytoon_creationand remove the grant timing fields, but the generated descriptions later in this file still mentionon_authorization,on_settlement,effective_at, andexpires_at. That makes the published contract read as self-contradictory for API consumers. Please fix the source schema/comments and regenerate instead of patching this file directly.As per coding guidelines, "All generated files must have
// Code generated by X, DO NOT EDIT.headers at the top; never edit files bearing this header".Also applies to: 3145-3210
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/api.gen.go` around lines 310 - 323, The generated file api.v3/api.gen.go shows BillingCreditAvailabilityPolicy trimmed to only BillingCreditAvailabilityPolicyOnCreation but the later documentation still references removed enum values (on_authorization, on_settlement) and fields (effective_at, expires_at); do not hand-edit this generated file—update the source schema and/or comment blocks that define BillingCreditAvailabilityPolicy and the related grant-timing fields in the source contract so the descriptive text matches the new enum, then re-run the generator to regenerate api.v3/api.gen.go (and any other affected generated outputs), and ensure the regenerated files include the required "// Code generated by X, DO NOT EDIT." header.
🧹 Nitpick comments (1)
api/spec/packages/aip/src/customers/credits/operations.tsp (1)
81-98: Prefer removing retired operations instead of keeping them commented out.Small cleanup suggestion: delete this commented interface block so the spec stays a clean source of truth (and avoids confusion during future contract scans).
As per coding guidelines: “The declared API should be accurate, in parity with the actual implementation, and easy to understand for the user.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/spec/packages/aip/src/customers/credits/operations.tsp` around lines 81 - 98, Remove the retired commented interface block for CustomerCreditGrantVoidOperations (including the voidGrant method and its annotations such as `@operationId`("void-credit-grant")) from the spec file so the API surface only contains active declarations; locate the commented interface block referencing CustomerCreditGrantVoidOperations and voidGrant and delete the entire commented-out section to keep the spec clean and accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v3/handlers/customers/credits/list_grants.go`:
- Around line 69-72: The conversion function convertAPIStatusToChargeStatus
currently only handles active and pending and does an unsafe cast for other
BillingCreditGrantStatus values; update convert.go to explicitly handle
BillingCreditGrantStatusExpired and BillingCreditGrantStatusVoided (either map
them to appropriate meta.ChargeStatus constants or return an explicit
error/unsupported value), avoid the unchecked meta.ChargeStatus(status)
fallback, and then adjust the caller in list_grants.go (where
convertAPIStatusToChargeStatus is used to set req.Status) to handle the new
error/unsupported-case (e.g., skip setting req.Status or return a validation
error) so filtering won’t produce invalid meta.ChargeStatus values.
---
Outside diff comments:
In `@api/v3/api.gen.go`:
- Around line 3121-3143: The generated struct CreateCreditAdjustmentRequest has
misleading field docs for Amount and Currency (they reference "granted
credits"); update the source model/comments that define
CreateCreditAdjustmentRequest.Amount and CreateCreditAdjustmentRequest.Currency
in the generator input (not the generated file) to describe adjustment-specific
semantics (e.g., adjustment amount and adjustment currency) and then re-run the
codegen to regenerate api.gen.go so the correct descriptions appear; also ensure
the generated file retains the required "// Code generated by X, DO NOT EDIT."
header.
- Around line 310-323: The generated file api.v3/api.gen.go shows
BillingCreditAvailabilityPolicy trimmed to only
BillingCreditAvailabilityPolicyOnCreation but the later documentation still
references removed enum values (on_authorization, on_settlement) and fields
(effective_at, expires_at); do not hand-edit this generated file—update the
source schema and/or comment blocks that define BillingCreditAvailabilityPolicy
and the related grant-timing fields in the source contract so the descriptive
text matches the new enum, then re-run the generator to regenerate
api.v3/api.gen.go (and any other affected generated outputs), and ensure the
regenerated files include the required "// Code generated by X, DO NOT EDIT."
header.
---
Nitpick comments:
In `@api/spec/packages/aip/src/customers/credits/operations.tsp`:
- Around line 81-98: Remove the retired commented interface block for
CustomerCreditGrantVoidOperations (including the voidGrant method and its
annotations such as `@operationId`("void-credit-grant")) from the spec file so the
API surface only contains active declarations; locate the commented interface
block referencing CustomerCreditGrantVoidOperations and voidGrant and delete the
entire commented-out section to keep the spec clean and accurate.
🪄 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: 70ca4e8b-6cc9-4edb-ba3a-a549cfa7a24e
⛔ Files ignored due to path filters (1)
api/v3/openapi.yamlis excluded by!**/openapi.yaml
📒 Files selected for processing (23)
api/spec/packages/aip/src/customers/credits/adjustment.tspapi/spec/packages/aip/src/customers/credits/grant.tspapi/spec/packages/aip/src/customers/credits/operations.tspapi/spec/packages/aip/src/konnect.tspapi/spec/packages/aip/src/openmeter.tspapi/types/entitlement.goapi/v3/api.gen.goapi/v3/handlers/customers/credits/convert.goapi/v3/handlers/customers/credits/create_grant.goapi/v3/handlers/customers/credits/get_balance.goapi/v3/handlers/customers/credits/get_grant.goapi/v3/handlers/customers/credits/handler.goapi/v3/handlers/customers/credits/list_grants.goapi/v3/server/routes.goapi/v3/server/server.goapp/common/creditgrant.gocmd/server/main.gocmd/server/wire.gocmd/server/wire_gen.goopenmeter/billing/creditgrant/service.goopenmeter/billing/creditgrant/service/service.goopenmeter/server/router/router.goopenmeter/server/server.go
💤 Files with no reviewable changes (1)
- api/types/entitlement.go
✅ Files skipped from review due to trivial changes (4)
- cmd/server/main.go
- openmeter/server/server.go
- api/spec/packages/aip/src/customers/credits/adjustment.tsp
- api/v3/handlers/customers/credits/convert.go
🚧 Files skipped from review as they are similar to previous changes (8)
- api/v3/handlers/customers/credits/get_balance.go
- api/v3/handlers/customers/credits/handler.go
- api/v3/server/routes.go
- api/spec/packages/aip/src/customers/credits/grant.tsp
- api/spec/packages/aip/src/konnect.tsp
- api/v3/handlers/customers/credits/create_grant.go
- cmd/server/wire.go
- openmeter/billing/creditgrant/service.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
api/v3/handlers/customers/credits/convert.go (1)
84-88: Silent nil returns may mask conversion failuresWhen
AsInvoiceSettlement()orAsExternalSettlement()return errors (lines 86-88, 107-109), or whenconvertAvailabilityPolicyfails (lines 114-116), the function silently returnsnil. This makes it indistinguishable from a legitimate promotional grant.If these errors represent data inconsistencies (e.g., settlement type says "invoice" but the data can't be decoded as invoice settlement), returning
nilcould hide bugs. Consider logging a warning or returning an error so callers can handle the unexpected case.💡 One option: return an error alongside the purchase
-func convertPurchase(charge creditpurchase.Charge) *creditGrantPurchase { +func convertPurchase(charge creditpurchase.Charge) (*creditGrantPurchase, error) { settlement := charge.Intent.Settlement switch settlement.Type() { case creditpurchase.SettlementTypeInvoice: inv, err := settlement.AsInvoiceSettlement() if err != nil { - return nil + return nil, fmt.Errorf("failed to convert invoice settlement: %w", err) } // ... rest of invoice case case creditpurchase.SettlementTypeExternal: ext, err := settlement.AsExternalSettlement() if err != nil { - return nil + return nil, fmt.Errorf("failed to convert external settlement: %w", err) } // ... rest of external case default: - return nil + return nil, nil // Promotional grants have no purchase block } }Also applies to: 105-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/v3/handlers/customers/credits/convert.go` around lines 84 - 88, The handler currently swallows decoding/conversion failures in cases like creditpurchase.SettlementTypeInvoice when settlement.AsInvoiceSettlement(), settlement.AsExternalSettlement(), and convertAvailabilityPolicy fail by returning nil; instead, change these branches to surface the error (or at minimum log a warning with context) so callers can distinguish a real promotional grant from a conversion failure. Specifically, in the branches that call AsInvoiceSettlement, AsExternalSettlement, and convertAvailabilityPolicy, replace the silent "return nil" with returning a descriptive error (or calling your logger with the error and relevant identifiers) and propagate that error out of the surrounding function so upstream code can handle/alert on data inconsistencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/v3/handlers/customers/credits/convert.go`:
- Around line 241-243: The switch's default branch blindly casts an unknown API
status string to meta.ChargeStatus, producing an invalid downstream value;
instead update the default branch to return a well-defined sentinel (e.g.,
meta.ChargeStatusUnknown) or an error: modify the default case in the
status-conversion function to map unknown strings to meta.ChargeStatusUnknown
(create that constant in the meta package if it doesn't exist) or change the
function signature to return (meta.ChargeStatus, error) and return a descriptive
error for unknown statuses rather than casting arbitrarily.
- Around line 35-38: The conversion currently casts charge.Intent.Priority (a
*int) directly to int16 and assigns to grant.Priority, which can overflow if
domain input is out of expected range; update the conversion (the block handling
charge.Intent.Priority -> grant.Priority) to validate the value is within the
allowed bounds (e.g., 1..1000 as per API spec) before casting—if the pointer is
non-nil and value out of range, return/propagate a validation error (or handle
per existing error flow) instead of blindly casting; reference
charge.Intent.Priority, grant.Priority, and Intent.Validate() when adding this
check.
---
Nitpick comments:
In `@api/v3/handlers/customers/credits/convert.go`:
- Around line 84-88: The handler currently swallows decoding/conversion failures
in cases like creditpurchase.SettlementTypeInvoice when
settlement.AsInvoiceSettlement(), settlement.AsExternalSettlement(), and
convertAvailabilityPolicy fail by returning nil; instead, change these branches
to surface the error (or at minimum log a warning with context) so callers can
distinguish a real promotional grant from a conversion failure. Specifically, in
the branches that call AsInvoiceSettlement, AsExternalSettlement, and
convertAvailabilityPolicy, replace the silent "return nil" with returning a
descriptive error (or calling your logger with the error and relevant
identifiers) and propagate that error out of the surrounding function so
upstream code can handle/alert on data inconsistencies.
🪄 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: 8db8844f-21d6-43d7-b5a4-dab1e33e4637
📒 Files selected for processing (2)
api/v3/handlers/customers/credits/convert.goapi/v3/handlers/customers/credits/list_grants.go
🚧 Files skipped from review as they are similar to previous changes (1)
- api/v3/handlers/customers/credits/list_grants.go
turip
reviewed
Apr 7, 2026
turip
previously approved these changes
Apr 7, 2026
tothandras
force-pushed
the
feat/api-v3-credit-grants
branch
from
2df7efe to
ea120fb
Compare
turip
approved these changes
Apr 7, 2026
This was referenced Apr 7, 2026
Merged
This was referenced Apr 15, 2026
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
2 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
Changes
Removals