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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds per-engine lifecycle hooks (CalculateLines, OnCollectionCompleted), a usage-based LineEngine and CreditThenInvoice state machine, centralizes line calculation/validation across engines, and extends the line-engine registry with register/deregister/list plus collection-time invoicing wiring to call engines during collection completion. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant BillingSvc as Billing Service
participant Registry as Line Engine Registry
participant Engine as Line Engine (usage-based)
participant StateMachine as UsageBased StateMachine
participant Adapter as Charge Adapter/DB
Caller->>BillingSvc: AdvanceInvoice(invoiceID)
BillingSvc->>Registry: Group invoice.StandardLines by Engine & Get(engineType)
loop per engine group
BillingSvc->>Engine: OnCollectionCompleted(ctx, {Invoice, Lines})
Engine->>StateMachine: Load/Create state machine for charge(s)
Engine->>StateMachine: FireTrigger("collection_completed")
StateMachine->>Adapter: Persist charge/state changes
Adapter-->>StateMachine: Updated charge
Engine->>Engine: Populate standard lines from realization run(s) & CalculateLines
Engine-->>BillingSvc: Return updated lines
end
BillingSvc->>BillingSvc: Validate standard-line IDs match exactly
BillingSvc->>BillingSvc: ReplaceLinesByID(updated lines)
BillingSvc-->>Caller: Invoice advanced (QuantitySnapshotedAt set / validation issues merged)
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
added
release-note/feature
turip
force-pushed
the
feat/invoice-calculation-hook
branch
from
d03972e to
76739b4
Compare
turip
force-pushed
the
feat/invoice-calculation-hook
branch
from
76739b4 to
ac990ec
Compare
turip
force-pushed
the
feat/invoice-calculation-hook
branch
from
ac990ec to
ffa753b
Compare
turip
force-pushed
the
feat/invoice-calculation-hook
branch
from
ffa753b to
f867ac8
Compare
turip
force-pushed
the
feat/invoice-calculation-hook
branch
from
f867ac8 to
b72e47d
Compare
![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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openmeter/billing/service/lineengine.go (1)
97-104:⚠️ Potential issue | 🟠 MajorNormalize blank
StandardLine.Enginebefore grouping.If there are already-persisted draft lines with
engine="", this new path will fail collection-time advancement instead of falling back toinvoicing. We already havepopulateStandardLineEnginefor that defaulting, so I'd either reuse it here or treat the rollout as requiring a backfill for existing standard lines.Also applies to: 137-155
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/service/lineengine.go` around lines 97 - 104, When grouping/persisting StandardLine instances ensure blank Engine values are normalized to the default before validation/grouping: call engineRegistry.populateStandardLineEngine(line) (which handles defaulting to billing.LineEngineTypeInvoice and delegates to validateLineEngine) for each StandardLine prior to any collection-time grouping logic (also update the other grouping path referenced in the diff around the secondary block) so persisted draft lines with Engine=="" are treated as invoicing instead of failing.
🧹 Nitpick comments (2)
openmeter/billing/lineengine/engine.go (1)
60-63: Use the shared input validator here.Right now this only checks
Invoice.ID, so an emptyLinesslice can slip through even thoughbilling.OnCollectionCompletedInput.Validate()requires lines. Reusing the shared validator keeps the hook aligned with the contract in one place.♻️ Suggested cleanup
func (e *Engine) OnCollectionCompleted(ctx context.Context, input billing.OnCollectionCompletedInput) (billing.StandardLines, error) { - if input.Invoice.ID == "" { - return nil, fmt.Errorf("invoice is required") + if err := input.Validate(); err != nil { + return nil, fmt.Errorf("validate: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/lineengine/engine.go` around lines 60 - 63, Replace the ad-hoc Invoice.ID check in Engine.OnCollectionCompleted with the shared validator by calling input.Validate() at the start of the method; if Validate() returns an error, return that error (or wrap it with context) so the method enforces the full billing.OnCollectionCompletedInput contract (including non-empty Lines) rather than only checking Invoice.ID.openmeter/billing/service/lineengine.go (1)
166-168: Rename this before the API escapes.
GetRegisterdLineEnginesbakes a typo into a new public method. Since this surface is still fresh, I'd fix it toGetRegisteredLineEnginesnow rather than carrying the misspelling into callers and docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/service/lineengine.go` around lines 166 - 168, The exported method name GetRegisterdLineEngines contains a typo; rename the public method to GetRegisteredLineEngines (update the function declaration and any internal references to s.GetRegisterdLineEngines) and ensure the returned behavior still calls s.lineEngines.List(); update any callers, tests, and documentation to use GetRegisteredLineEngines to avoid breaking the public API.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.agents/skills/charges/SKILL.md:
- Around line 187-188: The bullet claiming
`usagebased.Service.AdvanceCharge(...)` supports `CreditOnly` and noops for
`CreditThenInvoice`, and that `flatfee.Service.AdvanceCharge(...)` only supports
`CreditOnly`, is stale; update this text to reflect the new settlement-mode
dispatch where `AdvanceCharge(...)` (both usagebased.Service.AdvanceCharge and
flatfee.Service.AdvanceCharge) can build the
`NewCreditThenInvoiceStateMachine(...)` path for `CreditThenInvoice` rather than
noop, and clarify that invoice creation and collection completion remain
billing-triggered steps (invoice-created / collection-completed) handled
downstream; replace the contradictory wording with a concise statement that
AdvanceCharge routes to the appropriate settlement-mode state machine (e.g.,
CreditOnly vs CreditThenInvoice via NewCreditThenInvoiceStateMachine) and keep
invoice-created/collection-completed described as billing-triggered events.
In `@AGENTS.md`:
- Line 139: Move the guidance about avoiding context.Background()/context.TODO()
out of the "## Testing" section and place it under the "Code conventions"
section (or nearest existing conventions subsection) in AGENTS.md; update the
heading to match the conventions style, merge with any duplicate/stale notes
about context propagation or API parameter design, and remove the original entry
in "## Testing" so the guidance appears only once in the most relevant spot.
In `@openmeter/billing/charges/usagebased/service/creditheninvoice.go`:
- Around line 85-99: The state machine configuration in creditheninvoice.go
misses a transition from usagebased.StatusActiveFinalRealizationCompleted to
usagebased.StatusFinal, causing charges to be stuck; update the
s.Configure(usagebased.StatusActiveFinalRealizationCompleted) block (the one
that currently only calls Permit(meta.TriggerDelete, usagebased.StatusDeleted))
to also Permit(meta.TriggerNext, usagebased.StatusFinal) so the flow can advance
(mirror the behavior in creditsonly.go).
In `@openmeter/billing/charges/usagebased/service/lineengine.go`:
- Around line 122-178: After updating each stdLine from the realization run in
OnCollectionCompleted, re-run the invoice line recalculation to keep
totals/pricing consistent: call the project’s CalculateLines function (e.g. the
service/helper method named CalculateLines) with the same ctx, input.Invoice and
the modified input.Lines before returning; propagate and wrap any error (return
nil, fmt.Errorf(...)) and replace/return the recalculated billing.StandardLines
so the returned lines reflect final usage and pricing.
In `@openmeter/billing/service/gatheringinvoicependinglines.go`:
- Around line 632-649: After validating the shape with stdLines.Validate(),
verify that the set (or ordered list, depending on intended semantics) of IDs in
stdLines exactly matches the IDs in item.Lines before calling
invoice.Lines.Append; if they differ return an error referencing the engine (use
item.Engine.GetLineEngineType()) and include a clear message and the mismatched
IDs so the caller can handle it. Locate this check immediately after
stdLines.Validate() in the BuildStandardInvoiceLines path (where stdLines,
item.Lines and invoice.Lines.Append are used) and fail fast if any ID is
missing, added, or reordered in a way that violates expected replacement
semantics.
In `@openmeter/billing/service/stdinvoicestate.go`:
- Around line 856-865: The current code sets m.Invoice.QuantitySnapshotedAt
regardless of validationErrs and unconditionally overwrites any existing
timestamp; change it so the snapshot timestamp is only set when there are
groupedLines, there were no validationErrs (and mergeLineEngineValidationIssues
returned nil), and we do not overwrite an existing timestamp. Concretely: after
handling validationErrs via mergeLineEngineValidationIssues, only call
clock.Now().UTC() and assign to m.Invoice.QuantitySnapshotedAt when
len(groupedLines) > 0 AND len(validationErrs) == 0 (or after
mergeLineEngineValidationIssues succeeded) and m.Invoice.QuantitySnapshotedAt ==
nil.
In `@openmeter/billing/stdinvoice.go`:
- Around line 563-575: ReplaceLinesByID currently mutates c one-by-one and can
leave partial updates if a later replacement fails; change it to a two-phase
operation: first validate all inputs (ensure none are nil and each line.ID
exists in the receiver by calling ReplaceByID existence check or a dedicated
exists check) and collect any error, and only if validation passes, perform a
second loop that applies replacements by calling ReplaceByID for each line;
reference the ReplaceLinesByID method and the ReplaceByID helper so you validate
existence before applying changes and return early on validation failure without
mutating state.
In `@test/billing/collection_test.go`:
- Around line 364-367: Guard the pointer invoice.QuantitySnapshotedAt before
dereferencing: first assert/require that invoice.QuantitySnapshotedAt is not nil
(e.g., s.Require().NotNil(invoice.QuantitySnapshotedAt)) and only then call
clock.FreezeTime(*invoice.QuantitySnapshotedAt) and assign previousSnapshot :=
*invoice.QuantitySnapshotedAt; also replace the ineffective
s.NotNil(previousSnapshot) (a time.Time value) with a meaningful check such as
asserting previousSnapshot is not zero (e.g., !previousSnapshot.IsZero()) or
another appropriate time comparison.
In `@test/billing/lineengine_test.go`:
- Around line 71-76: The mock methods BuildStandardInvoiceLines and
OnCollectionCompleted currently type-assert args.Get(0) directly to
ombilling.StandardLines, which will panic when tests pass a function via
Return(); update each mock implementation to inspect args.Get(0) with a type
switch: if it's a function (matching the signature your tests use) invoke it
with the same input parameters to produce the ombilling.StandardLines result,
otherwise cast the value to ombilling.StandardLines (handling nil safely);
return the produced value and args.Error(1). Ensure you reference the mock
methods' names (BuildStandardInvoiceLines, OnCollectionCompleted) so the change
is localized to those implementations.
---
Outside diff comments:
In `@openmeter/billing/service/lineengine.go`:
- Around line 97-104: When grouping/persisting StandardLine instances ensure
blank Engine values are normalized to the default before validation/grouping:
call engineRegistry.populateStandardLineEngine(line) (which handles defaulting
to billing.LineEngineTypeInvoice and delegates to validateLineEngine) for each
StandardLine prior to any collection-time grouping logic (also update the other
grouping path referenced in the diff around the secondary block) so persisted
draft lines with Engine=="" are treated as invoicing instead of failing.
---
Nitpick comments:
In `@openmeter/billing/lineengine/engine.go`:
- Around line 60-63: Replace the ad-hoc Invoice.ID check in
Engine.OnCollectionCompleted with the shared validator by calling
input.Validate() at the start of the method; if Validate() returns an error,
return that error (or wrap it with context) so the method enforces the full
billing.OnCollectionCompletedInput contract (including non-empty Lines) rather
than only checking Invoice.ID.
In `@openmeter/billing/service/lineengine.go`:
- Around line 166-168: The exported method name GetRegisterdLineEngines contains
a typo; rename the public method to GetRegisteredLineEngines (update the
function declaration and any internal references to s.GetRegisterdLineEngines)
and ensure the returned behavior still calls s.lineEngines.List(); update any
callers, tests, and documentation to use GetRegisteredLineEngines to avoid
breaking the public API.
🪄 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: 85c627f5-21ad-4e8a-bf5a-674201e05e5a
📒 Files selected for processing (35)
.agents/skills/charges/SKILL.md.agents/skills/test/SKILL.mdAGENTS.mdapp/common/charges.goopenmeter/billing/charges/creditpurchase/lineengine/engine.goopenmeter/billing/charges/flatfee/lineengine/engine.goopenmeter/billing/charges/meta/triggers.goopenmeter/billing/charges/service/advance_test.goopenmeter/billing/charges/service/base_test.goopenmeter/billing/charges/service/invoicable_test.goopenmeter/billing/charges/service/invoice.goopenmeter/billing/charges/testutils/service.goopenmeter/billing/charges/usagebased/service.goopenmeter/billing/charges/usagebased/service/creditheninvoice.goopenmeter/billing/charges/usagebased/service/lineengine.goopenmeter/billing/charges/usagebased/service/service.goopenmeter/billing/charges/usagebased/service/statemachine.goopenmeter/billing/charges/usagebased/service/triggers.goopenmeter/billing/lineengine.goopenmeter/billing/lineengine/engine.goopenmeter/billing/lineengine/splitlinegroup.goopenmeter/billing/lineengine/stdinvoice.goopenmeter/billing/service.goopenmeter/billing/service/gatheringinvoicependinglines.goopenmeter/billing/service/invoice.goopenmeter/billing/service/invoicecalc/calculator.goopenmeter/billing/service/invoicecalc/details.goopenmeter/billing/service/lineengine.goopenmeter/billing/service/stdinvoicestate.goopenmeter/billing/stdinvoice.goopenmeter/billing/stdinvoiceline.goopenmeter/billing/validationissue.goopenmeter/server/server_test.gotest/billing/collection_test.gotest/billing/lineengine_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
openmeter/billing/stdinvoice.go (1)
563-581: Nice two-phase validation-then-mutate pattern!The previous review comment about partial mutations has been addressed - you're now validating all inputs exist before any replacements happen. That's solid.
One small edge case: if
c.IsAbsent()is true,GetByIDwill returnnilfor every line, causing the error "line not found" which is technically correct but maybe a bit misleading. Consider adding an early return if lines are absent:💡 Optional early-out for absent lines
func (c *StandardInvoiceLines) ReplaceLinesByID(lines ...*StandardLine) error { + if c.IsAbsent() { + if len(lines) > 0 { + return fmt.Errorf("cannot replace lines: invoice lines are absent") + } + return nil + } + for _, line := range lines {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/billing/stdinvoice.go` around lines 563 - 581, In ReplaceLinesByID, add an early check for c.IsAbsent() at the top and return a clear error instead of letting per-line GetByID produce "line not found" errors; update the function ReplaceLinesByID to return something like fmt.Errorf("lines are absent") when c.IsAbsent() is true so callers get an explicit, accurate failure; reference the methods ReplaceLinesByID, IsAbsent, and GetByID when making this change.
🤖 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/stdinvoice.go`:
- Around line 563-581: In ReplaceLinesByID, add an early check for c.IsAbsent()
at the top and return a clear error instead of letting per-line GetByID produce
"line not found" errors; update the function ReplaceLinesByID to return
something like fmt.Errorf("lines are absent") when c.IsAbsent() is true so
callers get an explicit, accurate failure; reference the methods
ReplaceLinesByID, IsAbsent, and GetByID when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c953f0af-5fcd-453e-bfc1-d74040c06b06
📒 Files selected for processing (8)
AGENTS.mdopenmeter/billing/lineengine.goopenmeter/billing/service/gatheringinvoicependinglines.goopenmeter/billing/service/stdinvoicestate.goopenmeter/billing/stdinvoice.gotest/billing/collection_test.gotest/billing/lineengine_test.gotest/billing/suite.go
✅ Files skipped from review due to trivial changes (1)
- test/billing/collection_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- AGENTS.md
- test/billing/lineengine_test.go
- openmeter/billing/lineengine.go
GAlexIHU
previously approved these changes
Apr 10, 2026
| type LineEngineService interface { | ||
| RegisterLineEngine(engine LineEngine) error | ||
| DeregisterLineEngine(engineType LineEngineType) error | ||
| GetRegisterdLineEngines() []LineEngineType |
GAlexIHU
approved these changes
Apr 10, 2026
This was referenced Apr 13, 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
This PR introduces a billing line-engine abstraction for invoice materialization, collection-time processing, and recalculation, and moves
charge-backed invoice-line behavior behind explicit flat-fee, credit-purchase, and usage-based engine implementations.
Billing now owns invoice orchestration and validates all line-engine inputs and outputs, while engines are required to return updated lines with
the exact same IDs so billing can merge them back into the invoice explicitly.
For usage-based
credit_then_invoice, the branch replaces generic snapshot-style behavior with explicit lifecycle triggers such asinvoice_createdandcollection_completed, keeping charge lifecycle ownership inside the charge domain.The split-line flow was also tightened so billing fetches the current gathering line and merges split results itself, while optional split
outputs are represented with pointers instead of zero-value sentinels.
Collection-time engine failures are now accumulated and surfaced as invoice validation issues through a dedicated line-engine validation path
rather than failing the invoice state machine directly.
Verification included
env GOCACHE=/tmp/go-build go test ./openmeter/billing/...andenv GOCACHE=/tmp/go-build go test ./test/billing -run TestLineEngine -count=1 -tags=dynamic, both of which passed.Notes for reviewer
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests / Documentation