feat: add support for negative credit allocations#4035
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR introduces credit-realization correction flows and refactors credit allocation/correction handling across flat-fee and usage-based charging. It defines separate allocation and correction input types with validation, updates handler interfaces to return typed inputs, adds correction request orchestration with multi-step validation, extends the database schema to support self-referential corrections and type tracking, and refactors usage-based and flat-fee state machines to handle re-rating with delta-based branching (allocating additional credits, correcting prior allocations, or performing no action). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 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 |
turip
force-pushed
the
feat/add-support-for-negative-credit-allocations
branch
from
f948785 to
2c1b42a
Compare
turip
force-pushed
the
feat/add-support-for-negative-credit-allocations
branch
from
fbc0eed to
65c9683
Compare
turip
added
release-note/misc
![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: 2
🧹 Nitpick comments (2)
openmeter/billing/charges/usagebased/service/rating.go (1)
90-99: Consider centralizing totals rounding to reduce future drift.
roundTotalsToCurrencyPrecisionis clear, but it’s easy to miss new monetary fields iftotals.Totalsevolves. Atotals-level rounding helper would make this safer long-term.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/rating.go` around lines 90 - 99, The current roundTotalsToCurrencyPrecision implementation individually rounds each field on totals.Totals which is fragile if totals.Totals grows; create a helper method on the totals type (e.g., add Totals.RoundToPrecision(calc currencyx.Calculator) or a function totals.Round(calc currencyx.Calculator, t *totals.Totals)) that encapsulates rounding logic and call that from roundTotalsToCurrencyPrecision (or replace it entirely) so any future monetary fields are handled in one place; update usages to call the new totals-level helper and remove per-field rounding to avoid drift.openmeter/billing/charges/models/creditrealization/models.go (1)
66-79: Consider whether zero amounts should be explicitly handled.The validation rejects non-positive amounts for allocations and non-negative amounts for corrections, which means zero is rejected for both types. This seems intentional (why create a zero-value realization?), but worth confirming this is the desired behavior.
If zero should be allowed in edge cases, the checks would need to change. If zero should always be rejected, you might want to add a brief comment explaining why.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/models/creditrealization/models.go` around lines 66 - 79, The validation currently treats zero as invalid for both TypeAllocation and TypeCorrection (using Amount.IsPositive() and Amount.IsNegative()); decide whether zero realizations are allowed — if you want to allow zero, change the checks in the switch for TypeAllocation to use a non-negative check (e.g., IsNonNegative or equivalent) and for TypeCorrection to use a non-positive check (and keep verifying CorrectsRealizationID for TypeCorrection); if zero should remain invalid, add a short comment near the switch (and on TypeAllocation/TypeCorrection behavior) explaining why zero amounts are rejected to make the intent clear for future readers and tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/billing/charges/models/creditrealization/correction_test.go`:
- Around line 1044-1068: The test currently returns a single correction of -15
which triggers the capacity guard instead of the mismatch-total path; change the
callback in the "error: callback returns mismatched corrections" test (the
lambda passed to Realizations.Correct) to return corrections whose summed Amount
does NOT equal the requested -5 (e.g., return a single correction of -4 or -6
for the same alloc.ID) so the code exercises the total-mismatch branch in
Realizations.Correct/CorrectionRequest handling, and update the assertion to
check the error text for the mismatch-specific message (e.g.,
assert.Contains(err.Error(), "mismatch" or the actual mismatch error string
emitted by Correct)).
In `@openmeter/billing/charges/models/creditrealization/correction.go`:
- Around line 96-140: The ValidateWith method fails to enforce that the provided
totalAmountToCorrect equals the sum of absolute correction inputs; compute
sumAbs := sum of input.Amount.Abs() across CreateCorrectionInputs (after
per-input validation) and compare it to totalAmountToCorrect (respecting
currency precision/rounding check already done); if they differ append a
validation error like "sum of correction amounts must equal
totalAmountToCorrect" and return via
models.NewNillableGenericValidationError(errors.Join(errs...)). Update
CreateCorrectionInputs.ValidateWith to perform this check before calling
existingRealizations.allocationsWithCorrections() so we fail fast when totals
don't reconcile.
---
Nitpick comments:
In `@openmeter/billing/charges/models/creditrealization/models.go`:
- Around line 66-79: The validation currently treats zero as invalid for both
TypeAllocation and TypeCorrection (using Amount.IsPositive() and
Amount.IsNegative()); decide whether zero realizations are allowed — if you want
to allow zero, change the checks in the switch for TypeAllocation to use a
non-negative check (e.g., IsNonNegative or equivalent) and for TypeCorrection to
use a non-positive check (and keep verifying CorrectsRealizationID for
TypeCorrection); if zero should remain invalid, add a short comment near the
switch (and on TypeAllocation/TypeCorrection behavior) explaining why zero
amounts are rejected to make the intent clear for future readers and tests.
In `@openmeter/billing/charges/usagebased/service/rating.go`:
- Around line 90-99: The current roundTotalsToCurrencyPrecision implementation
individually rounds each field on totals.Totals which is fragile if
totals.Totals grows; create a helper method on the totals type (e.g., add
Totals.RoundToPrecision(calc currencyx.Calculator) or a function
totals.Round(calc currencyx.Calculator, t *totals.Totals)) that encapsulates
rounding logic and call that from roundTotalsToCurrencyPrecision (or replace it
entirely) so any future monetary fields are handled in one place; update usages
to call the new totals-level helper and remove per-field rounding to avoid
drift.
🪄 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: b1b1a8bb-eec0-4099-a2fc-a19196b3308b
⛔ Files ignored due to path filters (17)
go.sumis excluded by!**/*.sum,!**/*.sumopenmeter/ent/db/chargeflatfeecreditallocations.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeecreditallocations/chargeflatfeecreditallocations.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeecreditallocations/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeecreditallocations_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeecreditallocations_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations/chargeusagebasedruncreditallocations.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations_update.gois excluded by!**/ent/db/**openmeter/ent/db/entmixinaccessor.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**openmeter/ent/db/runtime.gois excluded by!**/ent/db/**openmeter/ent/db/setorclear.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (29)
.agents/skills/charges/SKILL.mdopenmeter/billing/charges/flatfee/handler.goopenmeter/billing/charges/flatfee/service/creditsonly.goopenmeter/billing/charges/flatfee/service/invoice.goopenmeter/billing/charges/models/creditrealization/allocation.goopenmeter/billing/charges/models/creditrealization/correction.goopenmeter/billing/charges/models/creditrealization/correction_test.goopenmeter/billing/charges/models/creditrealization/mixin.goopenmeter/billing/charges/models/creditrealization/models.goopenmeter/billing/charges/models/creditrealization/realizations.goopenmeter/billing/charges/service/handlers_test.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/testutils/handlers.goopenmeter/billing/charges/usagebased/adapter.goopenmeter/billing/charges/usagebased/adapter/creditallocation.goopenmeter/billing/charges/usagebased/charge.goopenmeter/billing/charges/usagebased/handler.goopenmeter/billing/charges/usagebased/realizationrun.goopenmeter/billing/charges/usagebased/service/creditsonly.goopenmeter/billing/charges/usagebased/service/mutations.goopenmeter/billing/charges/usagebased/service/rating.goopenmeter/billing/charges/usagebased/service/rating_test.goopenmeter/billing/charges/usagebased/service/statemachine.goopenmeter/billing/charges/usagebased/service/triggers.goopenmeter/ledger/chargeadapter/flatfee.goopenmeter/ledger/chargeadapter/flatfee_test.gopkg/currencyx/currency.gotools/migrate/migrations/20260331103521_charges-negative-realization-runs.down.sqltools/migrate/migrations/20260331103521_charges-negative-realization-runs.up.sql
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openmeter/billing/charges/models/creditrealization/mixin.go (1)
91-95: Consider scopingallocationto sibling realizations.This edge only proves that
corrects_realization_idexists in the table. It can still point at another charge's / run's realization, or at another correction row. If those states are invalid for the domain, I’d backstop that in the concrete schema/migration or reject them in the adapter before insert.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/models/creditrealization/mixin.go` around lines 91 - 95, The edge "allocation" defined in mixin.go currently only enforces existence of corrects_realization_id and can point to realizations from other charges or runs; update the schema and adapter to prevent cross-sibling links by adding a scope check and/or DB constraint: in the concrete schema migration add a composite foreign key or check constraint that ensures corrects_realization_id references a realization with the same parent keys (e.g., same charge_id or run_id), and additionally add a validation in the adapter logic that creates/updates the allocation (the code path that sets corrects_realization_id) to reject or rewrite references that do not belong to the same sibling realization. Ensure you reference the "allocation" edge and "corrects_realization_id" when making the migration and adapter change so the constraint and validation target the correct field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/billing/charges/models/creditrealization/mixin.go`:
- Around line 70-76: Add a DB index on the "corrects_realization_id" column to
speed up self-referencing lookups: update the mixin to return an ent.Index that
includes index.Fields("corrects_realization_id") (e.g., implement or extend the
mixin's Indexes() to append ent.Index{}.Fields("corrects_realization_id")). Keep
the field declaration (field.String("corrects_realization_id")...) unchanged and
ensure the new index targets that exact field name so
QueryCorrections/WithCorrections use the index.
---
Nitpick comments:
In `@openmeter/billing/charges/models/creditrealization/mixin.go`:
- Around line 91-95: The edge "allocation" defined in mixin.go currently only
enforces existence of corrects_realization_id and can point to realizations from
other charges or runs; update the schema and adapter to prevent cross-sibling
links by adding a scope check and/or DB constraint: in the concrete schema
migration add a composite foreign key or check constraint that ensures
corrects_realization_id references a realization with the same parent keys
(e.g., same charge_id or run_id), and additionally add a validation in the
adapter logic that creates/updates the allocation (the code path that sets
corrects_realization_id) to reject or rewrite references that do not belong to
the same sibling realization. Ensure you reference the "allocation" edge and
"corrects_realization_id" when making the migration and adapter change so the
constraint and validation target the correct field.
🪄 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: fb4e2b6a-71d3-4a20-b881-cd8f002b4d29
⛔ Files ignored due to path filters (16)
openmeter/ent/db/chargeflatfeecreditallocations.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeecreditallocations/chargeflatfeecreditallocations.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeecreditallocations/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeecreditallocations_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeecreditallocations_query.gois excluded by!**/ent/db/**openmeter/ent/db/chargeflatfeecreditallocations_update.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations/chargeusagebasedruncreditallocations.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations/where.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations_create.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations_query.gois excluded by!**/ent/db/**openmeter/ent/db/chargeusagebasedruncreditallocations_update.gois excluded by!**/ent/db/**openmeter/ent/db/client.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**openmeter/ent/db/mutation.gois excluded by!**/ent/db/**tools/migrate/migrations/atlas.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (5)
openmeter/billing/charges/models/creditrealization/mixin.goopenmeter/ent/schema/chargesflatfee.goopenmeter/ent/schema/chargesusagebased.gotools/migrate/migrations/20260331103521_charges-negative-realization-runs.down.sqltools/migrate/migrations/20260331103521_charges-negative-realization-runs.up.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/migrate/migrations/20260331103521_charges-negative-realization-runs.down.sql
- tools/migrate/migrations/20260331103521_charges-negative-realization-runs.up.sql
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
openmeter/billing/charges/models/creditrealization/correction.go (1)
123-123: Consider usingalpacadecimal.Zerofor consistency.A tiny style nit - you can use
alpacadecimal.Zeroinstead ofalpacadecimal.NewFromFloat(0)to match the pattern used elsewhere (like in your tests at line 103 of the test file).✨ Suggested fix
- correctionTotal := alpacadecimal.NewFromFloat(0) + correctionTotal := alpacadecimal.Zero🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/models/creditrealization/correction.go` at line 123, Replace the explicit zero construction for correctionTotal with the package-level constant to match project style: change the initialization of correctionTotal (the variable in correction.go) from using alpacadecimal.NewFromFloat(0) to using alpacadecimal.Zero so it aligns with other uses (e.g., tests) and avoids unnecessary allocation.openmeter/billing/models/totals/model_test.go (1)
16-17: Consider adding a test case for zero-decimal currencies.USD has 2 decimal places, but currencies like JPY have 0. Adding a quick test with a zero-subunit currency would catch any edge cases in the rounding logic and increase confidence that the method handles different precisions correctly.
💡 Example additional test case
func TestTotalsRoundToPrecision_ZeroDecimal(t *testing.T) { t.Parallel() calc, err := currencyx.Code(currency.JPY).Calculator() require.NoError(t, err) in := Totals{ Amount: alpacadecimal.NewFromFloat(100.5), Total: alpacadecimal.NewFromFloat(99.4), } got := in.RoundToPrecision(calc) require.Equal(t, "101", got.Amount.String()) require.Equal(t, "99", got.Total.String()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/models/totals/model_test.go` around lines 16 - 17, Add a unit test that covers zero-decimal currencies to ensure RoundToPrecision handles precision=0 correctly: in openmeter/billing/models/totals/model_test.go create a new test (e.g., TestTotalsRoundToPrecision_ZeroDecimal) that obtains a calculator with currencyx.Code(currency.JPY).Calculator(), assert no error with require.NoError, construct a Totals value (e.g., Amount 100.5, Total 99.4), call in.RoundToPrecision(calc) and assert the rounded results with require.Equal (expect "101" for Amount and "99" for Total) to validate rounding behavior for JPY.openmeter/billing/rating/service/detailedline.go (1)
100-120: Inconsistent rounding between detailed line and invoice total calculations.The detailed lines calculation rounds the aggregated sum (
in.Totals = totals.Sum(...).RoundToPrecision(calc)), but the invoice calculation sums pre-rounded line totals without a finalRoundToPrecision()call. Since eachStandardLine.Totalscomes from the already-roundedGenerateDetailedLinesResult.Totals, the pattern works, but it's asymmetric. Consider applying final rounding at the invoice level too for consistency—especially if precision handling ever changes in either path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/rating/service/detailedline.go` around lines 100 - 120, The invoice-level totals assignment in getTotalsFromDetailedLines is missing a final rounding step: after computing in.Totals via totals.Sum(lo.Map(in.DetailedLines, ... )...), call RoundToPrecision(calc) on that result (same as used for line totals) so in.Totals = totals.Sum(...).RoundToPrecision(calc); this ensures GenerateDetailedLinesResult.Totals uses the same precision logic as the individual DetailedLine.Totals and keeps rounding behavior symmetric if precision handling changes later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openmeter/billing/charges/models/creditrealization/correction.go`:
- Around line 134-138: The error text in the validation block (where errs is
appended via fmt.Errorf for input.CorrectsRealizationID) is misleading because
the lookup map only contains allocations (from allocationsWithCorrections()), so
change the message to say "not found or is not an allocation" instead of "not
found or is not a correction"; update the fmt.Errorf string used when !ok (for
correction input[%d] ...) so it accurately reflects that the referenced
realization id is missing or isn't an allocation.
---
Nitpick comments:
In `@openmeter/billing/charges/models/creditrealization/correction.go`:
- Line 123: Replace the explicit zero construction for correctionTotal with the
package-level constant to match project style: change the initialization of
correctionTotal (the variable in correction.go) from using
alpacadecimal.NewFromFloat(0) to using alpacadecimal.Zero so it aligns with
other uses (e.g., tests) and avoids unnecessary allocation.
In `@openmeter/billing/models/totals/model_test.go`:
- Around line 16-17: Add a unit test that covers zero-decimal currencies to
ensure RoundToPrecision handles precision=0 correctly: in
openmeter/billing/models/totals/model_test.go create a new test (e.g.,
TestTotalsRoundToPrecision_ZeroDecimal) that obtains a calculator with
currencyx.Code(currency.JPY).Calculator(), assert no error with require.NoError,
construct a Totals value (e.g., Amount 100.5, Total 99.4), call
in.RoundToPrecision(calc) and assert the rounded results with require.Equal
(expect "101" for Amount and "99" for Total) to validate rounding behavior for
JPY.
In `@openmeter/billing/rating/service/detailedline.go`:
- Around line 100-120: The invoice-level totals assignment in
getTotalsFromDetailedLines is missing a final rounding step: after computing
in.Totals via totals.Sum(lo.Map(in.DetailedLines, ... )...), call
RoundToPrecision(calc) on that result (same as used for line totals) so
in.Totals = totals.Sum(...).RoundToPrecision(calc); this ensures
GenerateDetailedLinesResult.Totals uses the same precision logic as the
individual DetailedLine.Totals and keeps rounding behavior symmetric if
precision handling changes later.
🪄 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: 4da16dab-b671-4a68-801b-df0d0aee1fd8
📒 Files selected for processing (7)
openmeter/billing/charges/models/creditrealization/correction.goopenmeter/billing/charges/models/creditrealization/correction_test.goopenmeter/billing/charges/usagebased/service/rating.goopenmeter/billing/models/totals/model.goopenmeter/billing/models/totals/model_test.goopenmeter/billing/rating/service/detailedline.gotools/migrate/migrations/20260331103521_charges-negative-realization-runs.down.sql
✅ Files skipped from review due to trivial changes (1)
- openmeter/billing/charges/usagebased/service/rating.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/migrate/migrations/20260331103521_charges-negative-realization-runs.down.sql
tothandras
approved these changes
Apr 1, 2026
This was referenced Apr 1, 2026
This was referenced Apr 9, 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.
Overview
Introduces explicit allocation vs correction credit-realization types, standardizes sign handling so allocations are positive and corrections are negative everywhere in charges.
Adds correction creation/validation logic plus coverage for over-correction and remaining-balance behavior.
On the usage-based side, it fixes final-realization run state handling, adds support for persisted negative credit corrections during finalization, and ensures rated totals are rounded to currency precision before they are stored or handed to allocation/correction handlers.
It also adds lifecycle coverage for tiered usage corrections and updates related charge/subscription-sync test scaffolding.
Notes for reviewer
Summary by CodeRabbit
Release Notes
New Features
Improvements