Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds invoice payment authorization and settlement to usage-based invoice lifecycle: new triggers and statuses, handler callbacks and adapter persistence for run-level invoiced payments, state-machine transitions for payment flows, ledger booking/settlement wiring, validations, and tests; also documents the lifecycle constraint. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant LineEngine
participant StateMachine
participant Handler
participant Adapter as PaymentAdapter
participant Ledger
Client->>LineEngine: OnPaymentAuthorized(invoice, run, line)
LineEngine->>StateMachine: FireAndActivate(TriggerInvoicePaymentAuthorized, payload)
StateMachine->>Handler: OnPaymentAuthorized(charge, run)
Handler->>Ledger: CommitGroup(fund receivable)
Ledger-->>Handler: GroupReference
StateMachine->>Adapter: CreateRunPayment(authorized)
Adapter-->>StateMachine: payment.Invoiced
StateMachine-->>LineEngine: status -> active.authorized
Client->>LineEngine: OnPaymentSettled(invoice, run, line)
LineEngine->>StateMachine: FireAndActivate(TriggerInvoicePaymentSettled, payload)
StateMachine->>Handler: OnPaymentSettled(charge, run)
Handler->>Ledger: CommitGroup(settle receivable)
Ledger-->>Handler: GroupReference
StateMachine->>Adapter: UpdateRunPayment(settled)
Adapter-->>StateMachine: payment.Invoiced
StateMachine-->>LineEngine: status -> final
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![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: 5
🤖 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/service/invoicable_test.go`:
- Around line 1560-1562: The test freezes global time with
clock.FreezeTime(createAt) but never restores it, causing later tests to inherit
a frozen clock; update the test to call the corresponding unfreeze method (e.g.,
clock.UnFreeze() or clock.Unfreeze()) before the flow returns or in a defer
immediately after FreezeTime to ensure time is restored; reference the existing
createAt and clock.FreezeTime(...) call and add the matching clock.UnFreeze()
(or appropriate UnfreezeTime) to guarantee global clock state is reset.
In `@openmeter/billing/charges/usagebased/adapter/payment.go`:
- Around line 14-28: CreateRunPayment currently validates runID and in
separately but can persist a payment where run and payment namespaces differ;
add an explicit namespace equality guard before the transaction: check that
runID.Namespace (or equivalent field on usagebased.RealizationRunID) equals
in.Namespace and return a descriptive error if they mismatch. Place this check
in CreateRunPayment just after the Validate() calls and before
entutils.TransactingRepo is invoked so CreateInvoiced and SetRunID cannot create
a cross-namespace record.
In `@openmeter/billing/charges/usagebased/service/creditheninvoice.go`:
- Around line 111-115: The state machine currently permits re-entry via
Configure(...).PermitReentry(meta.TriggerInvoicePaymentAuthorized) into
usagebased.StatusActiveAuthorized but re-running RecordPaymentAuthorized can
attempt to persist run.Payment again and be rejected by
BookInvoicedPaymentAuthorized; either remove the PermitReentry for
TriggerInvoicePaymentAuthorized or make RecordPaymentAuthorized idempotent by
checking run.Payment (or via BookInvoicedPaymentAuthorized) and returning early
when the payment is already authorized; update the same pattern in the other
Configure block that mirrors this behavior (the second Configure around the
275-300 region) so retried webhooks become no-ops instead of failing.
In `@openmeter/ledger/chargeadapter/usagebased.go`:
- Line 7: The ledger booking code currently uses input.Charge.Intent.InvoiceAt
for transaction timestamps which backdates events; change each payment handler
to capture eventTime := clock.Now() once at the start of handling the payment
event and use eventTime for all ledger bookings instead of
input.Charge.Intent.InvoiceAt (replace occurrences of
input.Charge.Intent.InvoiceAt in the handlers and in the blocks around the
occurrences at the other spots referenced). Locate uses of
input.Charge.Intent.InvoiceAt (including the blocks around lines 130-135 and
191-196) and update them to use the newly captured eventTime variable so
authorization/settlement entries reflect the actual event time.
In `@test/credits/sanity_test.go`:
- Around line 854-875: The "late arriving usage is ignored once the invoice
finalization cutoff has passed" test is inconsistent: the added event's StoredAt
(2026-02-02T12:00:00Z) is before the collection cutoff so it gets included; to
fix, update the test so the StoredAt is after
invoice.DefaultCollectionAtForStandardInvoice() (e.g. set StoredAt to a
timestamp > invoice.DefaultCollectionAtForStandardInvoice()) and assert the
invoice totals remain the original values (no +25), or alternatively rename the
subtest to indicate the event is included before cutoff; changes should be made
where s.MockStreamingConnector.AddSimpleEvent is called and in the subsequent
assertions that check invoice.Lines/Totals.
🪄 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: 816e9297-66ed-41f6-99ae-5ace0f455596
⛔ Files ignored due to path filters (2)
openmeter/ent/db/chargeusagebased/chargeusagebased.gois excluded by!**/ent/db/**openmeter/ent/db/migrate/schema.gois excluded by!**/ent/db/**
📒 Files selected for processing (19)
.agents/skills/charges/SKILL.mdopenmeter/billing/charges/meta/triggers.goopenmeter/billing/charges/service/handlers_test.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/statemachine/machine.goopenmeter/billing/charges/statemachine/machine_test.goopenmeter/billing/charges/testutils/handlers.goopenmeter/billing/charges/usagebased/adapter.goopenmeter/billing/charges/usagebased/adapter/payment.goopenmeter/billing/charges/usagebased/handler.goopenmeter/billing/charges/usagebased/service/creditheninvoice.goopenmeter/billing/charges/usagebased/service/lineengine.goopenmeter/billing/charges/usagebased/service/run/payment.goopenmeter/billing/charges/usagebased/service/run/payment_test.goopenmeter/billing/charges/usagebased/statemachine.goopenmeter/billing/invoicelinesplitgroup.goopenmeter/ledger/chargeadapter/usagebased.goopenmeter/ledger/chargeadapter/usagebased_test.gotest/credits/sanity_test.go
turip
added
release-note/feature
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
openmeter/billing/charges/usagebased/service/creditheninvoice.go (1)
274-336:RecordPaymentAuthorized/RecordPaymentSettled+getRunForLinelook good.Flow is tidy: validate input → find the run by LineID → delegate to
Runs.Book*/Settle*→ persist the updated run. SinceLineIDon realizations is only populated byStartInvoiceCreatedRunfor the final-realization run in this settlement mode, the first-match lookup ingetRunForLineis unambiguous in practice.One tiny nit you can take or leave: the error wrapping in
RecordPaymentSettledsays"settle invoice payment: %w"and inRecordPaymentAuthorizedsays"authorize invoice payment: %w"— fine, just make sure these line up with how you want error logs to read up the stack. No change needed if that's intentional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/usagebased/service/creditheninvoice.go` around lines 274 - 336, The error wrapping strings in RecordPaymentAuthorized and RecordPaymentSettled are inconsistent; standardize them so logs read uniformly (pick one phrasing, e.g., "authorize invoiced payment: %w" or "settle invoiced payment: %w") by updating the fmt.Errorf calls in methods RecordPaymentAuthorized and RecordPaymentSettled (and keep the existing "update realization run: %w" wrappers unchanged for SetRealizationRun errors); ensure the chosen wording is used in both BookInvoicedPaymentAuthorized and SettleInvoicedPayment error wraps.openmeter/billing/charges/service/invoicable_test.go (1)
1531-1674:TestUsageBasedCreditThenInvoiceDirectPaidFlowis a solid addition.This nails the
TriggerPaid-straight-from-PaymentProcessingPendingpath, correctly asserts bothonPaymentAuthorized+onPaymentSettledfire exactly once, and confirms the run ends onStatusFinalwith bothPayment.Authorized/Payment.Settledtransaction group IDs persisted. Thedefer clock.UnFreeze()on line 1562 is a nice cleanup too. ✨One small consistency note: line 1589 calls
res[0].GetChargeID()with a two-return form (valid sinceres[0]is typecharges.Charge), while most other tests in the file goAsUsageBasedCharge()first and then callGetChargeID()with a single return (e.g., lines 1062–1063). Both work fine, but aligning with the file's dominant pattern would improve readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/charges/service/invoicable_test.go` around lines 1531 - 1674, The test TestUsageBasedCreditThenInvoiceDirectPaidFlow currently calls res[0].GetChargeID() using the two-value form; to match the file's dominant pattern and improve readability, call AsUsageBasedCharge() on res[0] first and then call GetChargeID() (i.e., use res[0].AsUsageBasedCharge().GetChargeID() or assign the result of AsUsageBasedCharge() to a variable and call GetChargeID() on it) so the single-return GetChargeID() form is used consistently with other tests like those around lines 1062–1063.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@openmeter/billing/charges/service/invoicable_test.go`:
- Around line 1531-1674: The test TestUsageBasedCreditThenInvoiceDirectPaidFlow
currently calls res[0].GetChargeID() using the two-value form; to match the
file's dominant pattern and improve readability, call AsUsageBasedCharge() on
res[0] first and then call GetChargeID() (i.e., use
res[0].AsUsageBasedCharge().GetChargeID() or assign the result of
AsUsageBasedCharge() to a variable and call GetChargeID() on it) so the
single-return GetChargeID() form is used consistently with other tests like
those around lines 1062–1063.
In `@openmeter/billing/charges/usagebased/service/creditheninvoice.go`:
- Around line 274-336: The error wrapping strings in RecordPaymentAuthorized and
RecordPaymentSettled are inconsistent; standardize them so logs read uniformly
(pick one phrasing, e.g., "authorize invoiced payment: %w" or "settle invoiced
payment: %w") by updating the fmt.Errorf calls in methods
RecordPaymentAuthorized and RecordPaymentSettled (and keep the existing "update
realization run: %w" wrappers unchanged for SetRealizationRun errors); ensure
the chosen wording is used in both BookInvoicedPaymentAuthorized and
SettleInvoicedPayment error wraps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eed9f857-cc5f-41dc-849c-6043a5a8783d
📒 Files selected for processing (4)
openmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/usagebased/adapter/payment.goopenmeter/billing/charges/usagebased/service/creditheninvoice.gotest/credits/sanity_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/credits/sanity_test.go
This was referenced Apr 20, 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
Usage-based credit_then_invoice charges now model payment as part of the active lifecycle instead of hiding it inside final. The state machine was updated so finalized usage moves into active.payment_pending on invoice issuance, then into active.authorized on payment authorization, and only reaches final once payment is settled. This makes the charge status reflect the real economic lifecycle and keeps the final meta-state reserved for fully settled charges.
Note: Current states are temporary until we have progressive billing in place.
To support that flow, usage-based payment handling was added at the run level. The branch introduces run payment persistence for usage-based realization runs, expands the usage-based handler contract to cover invoice payment authorization and settlement, and reuses the flat-fee ledger handling pattern for authorized and settled events. The billing invoice/line flow now drives those usage-based payment transitions so late-
arriving payment events can update already-finalized usage runs correctly.
The generic charge state machine persistence behavior was also tightened up. FireAndActivate(...) now persists successful transitions directly, and AdvanceUntilStateStable(...) no longer performs an extra write afterward.
Notes for reviewer
Summary by CodeRabbit
New Features
Tests