Skip to content

feat[charges]: skaffold progressive billing support#4203

Merged
turip merged 6 commits intomainfrom
feat/skaffold-progressive-billing-support
Apr 23, 2026
Merged

feat[charges]: skaffold progressive billing support#4203
turip merged 6 commits intomainfrom
feat/skaffold-progressive-billing-support

Conversation

@turip
Copy link
Copy Markdown
Member

@turip turip commented Apr 22, 2026

Overview

This PR extends usage-based charge lifecycles to support partial invoicing in the billing line engine, while keeping rating’s progressive-billing limitations explicitly out of scope.

The main architectural change is that usage-based charges can now create and manage PartialInvoiceRuns in addition to final-realization runs, and the state machine now models partial invoicing as a first-class realization
branch. This lets billing progressively invoice usage-based charges without pretending every invoice-backed run is a final realization.

Warning: Rating is broken for progressive billing/partial invoices. That will be fixed going forward in the next prs.

Why

Before this change, usage-based charges had a structural mismatch with billing:

  • billing already knew how to progressively bill gathering lines
  • usage-based line engine explicitly opted out of partial billing
  • invoice-created flows treated every invoice-backed run as a final realization
  • payment handling was too tightly coupled to charge-state transitions even though payments can happen long after a run is finalized

That made partial invoicing hard to represent cleanly and forced semantics that were not true in the domain.

This PR fixes the structural model first:

  • progressive line splitting is now supported for usage-based charges
  • invoice-backed partial runs are represented explicitly
  • the charge state machine is focused on realization lifecycle
  • payment is handled as run-level facts plus a charge-level aggregate settlement gate

Architectural Changes

Usage-based line engine:

  • Usage-based gathering lines can now participate in progressive billing.
  • usage-based does not use SplitLineGroup for this

New realization run type:

  • usagebased.RealizationRunTypePartialInvoice
  • a partial invoice is not a final realization
  • forcing both through the same run type made downstream state handling ambiguous
  • this gives billing and usage-based lifecycle code a durable, explicit distinction

Usage-based state machine refactor:

  • The credit_then_invoice usage-based state machine now has two explicit realization branches:
    • partial_invoice
    • final_realization
  • Each branch now follows the same readable shape:
    • started
    • waiting_for_collection
    • processing
    • issuing
    • completed

Usage-based now enforces a strict invariant:

  • a new invoice-backed run cannot start while Charge.State.CurrentRealizationRunID is set thus enforcing that no parallel realities start to exist

Payment model redesign for usage-based:

  • Usage-based payment handling was refactored to separate realization from payment.
  • this is necessary as payment settlement can happen way later than invoice finalization

Summary by CodeRabbit

Release Notes

  • New Features

    • Added partial and final invoice lifecycle states for usage-based charges with distinct status progression (started → waiting for collection → processing → issuing → completed)
    • Introduced aggregated payment settlement handling that waits for all invoiced charges to settle before advancing to final status
  • Bug Fixes

    • Added validation to prevent concurrent invoice realizations on active charges
    • Updated charge status transitions for improved payment settlement tracking
  • Tests

    • Added comprehensive test coverage for usage-based billing workflows including partial invoice handling and payment settlement scenarios

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 22, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3612af05-adfe-4b71-8fd9-89026917b54e

📥 Commits

Reviewing files that changed from the base of the PR and between db51c1d and f57319a.

⛔ Files ignored due to path filters (2)
  • openmeter/ent/db/chargeusagebased/chargeusagebased.go is excluded by !**/ent/db/**
  • openmeter/ent/db/migrate/schema.go is excluded by !**/ent/db/**
📒 Files selected for processing (1)
  • openmeter/billing/charges/usagebased/service/lineengine.go
✅ Files skipped from review due to trivial changes (1)
  • openmeter/billing/charges/usagebased/service/lineengine.go

📝 Walkthrough

Walkthrough

Split usage-based invoice lifecycle into partial vs final flows: new triggers and run types, added partial-invoice substates, consolidated payment settlement into an awaiting-settlement state, added validation to prevent concurrent realization runs, and implemented per-line payment recording with tests and docs updates.

Changes

Cohort / File(s) Summary
Triggers & State Constants
openmeter/billing/charges/meta/triggers.go, openmeter/billing/charges/usagebased/statemachine.go
Replaced/added triggers (partial_invoice_created, final_invoice_created, all_payments_settled); removed old invoice/payment triggers. Added partial-invoice and final-issuing substates; removed payment_pending/authorized, added awaiting_payment_settlement.
Realization Run Types
openmeter/billing/charges/usagebased/realizationrun.go
Added RealizationRunTypePartialInvoice and accepted value in validation list.
State Machine & Run APIs
openmeter/billing/charges/usagebased/service/creditheninvoice.go
Split partial vs final invoice flows; new StartPartial/StartFinal methods; consolidated run creation (startInvoiceCreatedRun), added AreAllInvoicedRunsSettled, moved resolveInvoiceCreatedTrigger and getRunForLine; removed old RecordPayment* methods.
Line Engine & Triggering
openmeter/billing/charges/usagebased/service/lineengine.go
Enabled progressive billing check, implemented SplitGatheringLine, dynamic trigger resolution, validation guard on CurrentRealizationRunID, refactored trigger firing to structured input.
Payment Recording
openmeter/billing/charges/usagebased/service/payments.go
New file: per-line payment recording (recordRunPayments), handlers for authorized/settled that call run service and update charge realizations; aggregates settlement via areAllInvoicedRunsSettled.
Run Creation Validation
openmeter/billing/charges/usagebased/service/run/create.go
Added early validation rejecting run creation when Charge.State.CurrentRealizationRunID is already set.
Errors
openmeter/billing/charges/usagebased/errors.go
Added ErrActiveRealizationRunAlreadyExists validation issue and code.
Tests
openmeter/billing/charges/service/usagebased_test.go, openmeter/billing/charges/usagebased/service/creditheninvoice_test.go, openmeter/billing/charges/usagebased/service/lineengine_test.go, openmeter/billing/charges/service/invoicable_test.go, test/credits/sanity_test.go, openmeter/billing/charges/usagebased/service/run/payment_test.go
Added comprehensive usage-based lifecycle tests for partial/final flows and blocking invariants; updated existing tests and helpers to new statuses and expectations.
Docs
.agents/skills/charges/SKILL.md
Updated guidance describing partial vs final invoice behavior, lifecycle ordering, validation guard, and enum generation workflow.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant LineEngine
    participant StateMachine as CreditThenInvoice<br/>StateMachine
    participant RunSvc as RealizationRun<br/>Service
    participant PaymentSvc as Payment<br/>Recording

    Client->>LineEngine: OnStandardInvoiceCreated(invoice, lines)
    LineEngine->>LineEngine: resolveInvoiceCreatedTrigger()
    alt Partial invoice (billed < service period)
        LineEngine->>StateMachine: Fire TriggerPartialInvoiceCreated
        StateMachine->>RunSvc: StartPartialInvoiceRun(...)
        RunSvc-->>StateMachine: create partial realization
        StateMachine->>StateMachine: enter partial_waiting_for_collection
    else Final invoice (billed == service period)
        LineEngine->>StateMachine: Fire TriggerFinalInvoiceCreated
        StateMachine->>RunSvc: StartFinalInvoiceRun(...)
        RunSvc-->>StateMachine: create final realization
        StateMachine->>StateMachine: enter final_waiting_for_collection
    end

    Client->>LineEngine: OnPaymentAuthorized(invoice, lines)
    LineEngine->>PaymentSvc: recordPaymentAuthorized(...) 
    PaymentSvc->>RunSvc: BookInvoicedPaymentAuthorized(...)

    Client->>LineEngine: OnPaymentSettled(invoice, lines)
    LineEngine->>PaymentSvc: recordPaymentSettled(...)
    PaymentSvc->>RunSvc: SettleInvoicedPayment(...)
    PaymentSvc->>PaymentSvc: areAllInvoicedRunsSettled()?
    alt All runs settled
        PaymentSvc->>StateMachine: Fire TriggerAllPaymentsSettled
        StateMachine->>StateMachine: transition to final
    else Some runs pending
        StateMachine->>StateMachine: remain awaiting_payment_settlement
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

Suggested reviewers

  • tothandras
  • GAlexIHU
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'feat[charges]: skaffold progressive billing support' accurately reflects the core change—adding progressive billing support to usage-based charges—and is specific enough for context scanning.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/skaffold-progressive-billing-support

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@turip turip force-pushed the feat/skaffold-progressive-billing-support branch from 9c0da77 to 7bd337f Compare April 22, 2026 13:03
@turip turip force-pushed the feat/skaffold-progressive-billing-support branch from 7bd337f to db51c1d Compare April 22, 2026 13:07
@turip turip marked this pull request as ready for review April 22, 2026 13:08
@turip turip requested a review from a team as a code owner April 22, 2026 13:08
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (8)
openmeter/billing/charges/usagebased/service/run/create.go (1)

36-38: Use the typed validation issue here too.

This guard mirrors ErrActiveRealizationRunAlreadyExists, but currently returns a plain error. Direct CreateRatedRun callers would lose the stable error code / HTTP 400 mapping.

♻️ Proposed consistency tweak
 	if i.Charge.State.CurrentRealizationRunID != nil {
-		return fmt.Errorf("charge: current realization run already exists [charge_id=%s]", i.Charge.GetChargeID())
+		return usagebased.ErrActiveRealizationRunAlreadyExists.WithAttrs(models.Attributes{
+			"charge_id": i.Charge.ID,
+		})
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openmeter/billing/charges/usagebased/service/run/create.go` around lines 36 -
38, Replace the plain fmt.Errorf return in the guard that checks
i.Charge.State.CurrentRealizationRunID inside CreateRatedRun with the typed
validation issue ErrActiveRealizationRunAlreadyExists (or the package's
validation issue constructor) so callers get the stable error code/HTTP 400
mapping; include the charge id as context/meta on that validation issue (using
the same message or metadata currently passed to fmt.Errorf) instead of
returning a raw error string.
openmeter/billing/charges/service/usagebased_test.go (3)

594-617: Tiny shadow on partialInvoice inside the approval sub-run.

Line 608 uses := with (partialInvoice, err), and since partialInvoice from the outer function scope isn't in the same lexical scope as this s.Run(...) closure, := creates a new local partialInvoice that shadows the outer one. It doesn't break behavior today (no later sub-run reads the outer partialInvoice after this), but it's inconsistent with the earlier sub-runs (lines 541 and 547) that use = to reassign the outer. Swapping to = would also catch the case if someone later tries to use the approved invoice in a subsequent sub-run:

🪞 Nit fix
-		partialInvoice, err := s.BillingService.ApproveInvoice(ctx, partialInvoice.GetInvoiceID())
+		var err error
+		partialInvoice, err = s.BillingService.ApproveInvoice(ctx, partialInvoice.GetInvoiceID())
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openmeter/billing/charges/service/usagebased_test.go` around lines 594 - 617,
The test creates a new local variable by shadowing the outer partialInvoice when
calling s.BillingService.ApproveInvoice(...) inside the s.Run(...) closure
(using :=); change that declaration to use assignment (=) so the outer
partialInvoice is reassigned instead of shadowed, keeping behavior consistent
with the earlier sub-runs and preventing future bugs if later assertions
reference the approved invoice (also ensure you update the err handling to use =
for err if needed).

169-173: Consider explicit StoredAt on ingested events.

These MockStreamingConnector.AddSimpleEvent(...) calls set only the event time, letting StoredAt default to whatever the mock picks. That's fine for lifecycle-progression assertions, but the usage-based finalization path is specifically gated on stored_at < cutoff (where cutoff is clock.Now() - InternalCollectionPeriod), and the coding guideline asks for explicit StoredAt in billing tests so we exercise that cutoff on purpose. Since you're already freezing clock per sub-run, layering in streaming/testutils.WithStoredAt(...) (or the equivalent helper that takes explicit stored-at) on at least the mid-period event would make the intent crisper and protect against future cutoff regressions:

s.MockStreamingConnector.AddSimpleEvent(
    meterSlug,
    15,
    datetime.MustParseTimeInLocation(s.T(), "2026-01-15T00:00:00Z", time.UTC).AsTime(),
    // WithStoredAt(...) so the stored-at cutoff behavior is exercised explicitly.
)

As per coding guidelines: "Use MockStreamingConnector with explicit StoredAt values in usage-based billing tests to exercise stored-at cutoff logic in finalization".

Also applies to: 225-229, 305-309, 525-529, 564-568

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openmeter/billing/charges/service/usagebased_test.go` around lines 169 - 173,
The test uses MockStreamingConnector.AddSimpleEvent(...) without explicit
StoredAt which leaves stored_at to the mock's default and fails to exercise the
stored_at < cutoff path used by the usage-based finalization (cutoff =
clock.Now() - InternalCollectionPeriod). Update the AddSimpleEvent calls (e.g.,
the ones at 169-173 and also at 225-229, 305-309, 525-529, 564-568) to pass an
explicit StoredAt timestamp (use streaming/testutils.WithStoredAt or the
equivalent helper) for at least the mid-period event(s) so the stored_at cutoff
logic (stored_at < cutoff) is triggered deterministically against the frozen
clock; keep event time the same but set StoredAt relative to clock.Now() -
InternalCollectionPeriod to cover both sides of the cutoff as needed.

322-322: TODO left in the test — intentional?

The // TODO[rating]: totals are off due to rating not yet supporting progressive billing via charges comment is right where one might expect a RequireTotals assertion on finalInvoice. Just confirming this is intended to land as-is (tracked elsewhere) rather than getting a follow-up ticket, since without the totals check this sub-run's behavioral coverage on the final invoice is thinner than the mid-period one. Happy to help open an issue to track the rating work if useful.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openmeter/billing/charges/service/usagebased_test.go` at line 322, The test
currently omits a totals assertion for finalInvoice (see finalInvoice and the
missing RequireTotals call), so either restore the missing assertion or track it
with a follow-up ticket: open an issue for the rating/progressive-billing work,
then update the inline TODO to reference that issue (e.g., TODO[rating]: totals
are off — tracked in ISSUE-123) and add a short comment explaining why
RequireTotals was removed; alternatively, if you want the test to fail when
behavior changes, re-add the RequireTotals(finalInvoice, ...) assertion in
usagebased_test.go but guard it with a clear TODO and/or t.Skip with the issue
ID so the intent and tracking are explicit.
openmeter/billing/charges/usagebased/service/payments.go (1)

123-141: areAllInvoicedRunsSettled semantics look right, but worth a doc line.

The "must have at least one invoiced final-realization run" guard is subtle and not obvious from the function name alone — it returns false if there are only invoiced partial runs that are all settled. That matches the state-machine intent (StatusActiveAwaitingPaymentSettlement → StatusFinal must wait for the final invoice too), but a one-line comment explaining "requires the final run to be present and all invoiced runs settled" would help the next reader:

📝 Optional comment
+// areAllInvoicedRunsSettled returns true only when (a) at least one final-realization
+// run has been invoiced, and (b) every invoiced run (partial or final) is payment-settled.
 func areAllInvoicedRunsSettled(charge usagebased.Charge) bool {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openmeter/billing/charges/usagebased/service/payments.go` around lines 123 -
141, The function areAllInvoicedRunsSettled has subtle semantics (it returns
false unless there is at least one invoiced final-realization run and all
invoiced runs are settled); add a one-line doc comment immediately above the
areAllInvoicedRunsSettled function explaining that it requires the presence of
an invoiced final realization (usagebased.RealizationRunTypeFinalRealization)
and that it only returns true when every run with InvoiceUsage has a Payment
with StatusSettled, referencing the function name areAllInvoicedRunsSettled and
the payment.StatusSettled and usagebased.RealizationRunTypeFinalRealization
symbols so future readers understand the guard.
openmeter/billing/charges/usagebased/service/lineengine_test.go (1)

53-87: Test name promises more than it asserts.

The test is called "KeepsChargeGrouping...ButWithoutChildReferences", but we never actually assert that ChargeID is preserved on either PreSplitAtLine or PostSplitAtLine. Also, since the input line never sets SplitLineGroupID, the require.Nil(..., SplitLineGroupID) assertions on lines 79 and 84 are trivially satisfied — a regression where split forgets to clear it wouldn't fail this test.

Consider either setting SplitLineGroupID on the input and asserting it's cleared, or adding a ChargeID equality check to actually lock down "charge grouping is kept":

🧪 Proposed tightening
 	line := billing.GatheringLine{
 		GatheringLineBase: billing.GatheringLineBase{
 			...
 			ChargeID:               lo.ToPtr("01K00000000000000000000002"),
 			ChildUniqueReferenceID: lo.ToPtr("original"),
+			SplitLineGroupID:       lo.ToPtr("01K00000000000000000000003"),
 		},
 	}
 	...
 	require.Nil(t, result.PreSplitAtLine.SplitLineGroupID)
 	require.Nil(t, result.PreSplitAtLine.ChildUniqueReferenceID)
+	require.Equal(t, line.ChargeID, result.PreSplitAtLine.ChargeID)
 	require.Equal(t, splitAt, result.PreSplitAtLine.ServicePeriod.To)

 	require.NotNil(t, result.PostSplitAtLine)
 	require.Nil(t, result.PostSplitAtLine.SplitLineGroupID)
 	require.Nil(t, result.PostSplitAtLine.ChildUniqueReferenceID)
+	require.Equal(t, line.ChargeID, result.PostSplitAtLine.ChargeID)
 	require.Equal(t, splitAt, result.PostSplitAtLine.ServicePeriod.From)

Heads up: if you add the SplitLineGroupID input bit, double-check that SplitGatheringLine actually clears it — from the snippet of lineengine.go it only clears ChildUniqueReferenceID, so this could surface a real gap.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openmeter/billing/charges/usagebased/service/lineengine_test.go` around lines
53 - 87, The test name promises ensuring charge grouping is preserved without
child references but it never asserts ChargeID and uses a nil SplitLineGroupID
input (making nil assertions vacuous); update the test by setting
line.SplitLineGroupID to a non-nil value in the input GatheringLine and add
assertions that result.PreSplitAtLine.ChargeID and
result.PostSplitAtLine.ChargeID equal the input ChargeID, and assert that
result.PreSplitAtLine.SplitLineGroupID and
result.PostSplitAtLine.SplitLineGroupID are cleared (nil) after
SplitGatheringLine; also keep or adjust assertions for ChildUniqueReferenceID to
reflect that SplitGatheringLine clears ChildUniqueReferenceID per the
SplitGatheringLine implementation.
openmeter/billing/charges/usagebased/service/lineengine.go (1)

413-424: Small error-context nit on isUsageBasedSplitPeriodEmpty.

When price == nil we bubble up "price is nil" with no line/charge context, which will be painful to diagnose if it ever fires from SplitGatheringLine. The callers at lines 62 and 79 do wrap with "checking if post/pre split line is empty", so it's not catastrophic, but adding the line ID (or just returning nil for "not empty" in this case) would be friendlier:

🏷️ Optional tweak
-func isUsageBasedSplitPeriodEmpty(line billing.GatheringLine) (bool, error) {
-	price := line.GetPrice()
-	if price == nil {
-		return false, fmt.Errorf("price is nil")
-	}
+func isUsageBasedSplitPeriodEmpty(line billing.GatheringLine) (bool, error) {
+	price := line.GetPrice()
+	if price == nil {
+		return false, fmt.Errorf("usage based gathering line[%s]: price is nil", line.ID)
+	}

Also worth a quick sanity check: a usage-based gathering line with a nil price shouldn't exist in practice, right? If so, this branch is more of a defensive assertion — happy to leave it.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@openmeter/billing/charges/usagebased/service/lineengine.go` around lines 413
- 424, The error returned from isUsageBasedSplitPeriodEmpty when price == nil
lacks context; update isUsageBasedSplitPeriodEmpty to include the gathering line
identifier in the error message (e.g., use line.GetId() or line.GetLineId()) so
callers like SplitGatheringLine will get useful context, or alternatively return
false (treat as "not empty") if you prefer a defensive no-op; modify the branch
that currently returns fmt.Errorf("price is nil") to produce a contextual error
string referencing the line ID or to return (false, nil) depending on the chosen
behavior.
openmeter/billing/charges/usagebased/service/creditheninvoice.go (1)

263-269: resolveInvoiceCreatedTrigger — worth asserting the happy-path assumption.

This picks TriggerFinalInvoiceCreated only on exact equality of the normalized end timestamps, otherwise falls back to partial. That means if billedPeriod.To ever drifts past charge.Intent.ServicePeriod.To (e.g. a boundary rounding bug upstream), we'd quietly route it as a partial invoice — which the downstream StatusActivePartialInvoice* states would then try to own. Probably impossible in practice given the truncation rules, but a small defensive tweak would make the intent crisper:

🧭 Optional defensive tweak
 func resolveInvoiceCreatedTrigger(charge usagebased.Charge, billedPeriod timeutil.ClosedPeriod) meta.Trigger {
-	if meta.NormalizeTimestamp(billedPeriod.To).Equal(meta.NormalizeTimestamp(charge.Intent.ServicePeriod.To)) {
+	if !meta.NormalizeTimestamp(billedPeriod.To).Before(meta.NormalizeTimestamp(charge.Intent.ServicePeriod.To)) {
 		return meta.TriggerFinalInvoiceCreated
 	}
 
 	return meta.TriggerPartialInvoiceCreated
 }

Feel free to ignore if upstream invariants already forbid billedPeriod.To > servicePeriod.To.

🤖 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 263 - 269, Add a defensive assertion in resolveInvoiceCreatedTrigger:
normalize both billedPeriod.To and charge.Intent.ServicePeriod.To (using
meta.NormalizeTimestamp), then if billed > service, either log/error or panic to
surface the invariant breach instead of silently treating it as partial;
otherwise keep the exact-equality check and return
meta.TriggerFinalInvoiceCreated or meta.TriggerPartialInvoiceCreated as before.
This uses resolveInvoiceCreatedTrigger, billedPeriod.To and
charge.Intent.ServicePeriod.To to locate the 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/charges/service/usagebased_test.go`:
- Around line 594-617: The test creates a new local variable by shadowing the
outer partialInvoice when calling s.BillingService.ApproveInvoice(...) inside
the s.Run(...) closure (using :=); change that declaration to use assignment (=)
so the outer partialInvoice is reassigned instead of shadowed, keeping behavior
consistent with the earlier sub-runs and preventing future bugs if later
assertions reference the approved invoice (also ensure you update the err
handling to use = for err if needed).
- Around line 169-173: The test uses MockStreamingConnector.AddSimpleEvent(...)
without explicit StoredAt which leaves stored_at to the mock's default and fails
to exercise the stored_at < cutoff path used by the usage-based finalization
(cutoff = clock.Now() - InternalCollectionPeriod). Update the AddSimpleEvent
calls (e.g., the ones at 169-173 and also at 225-229, 305-309, 525-529, 564-568)
to pass an explicit StoredAt timestamp (use streaming/testutils.WithStoredAt or
the equivalent helper) for at least the mid-period event(s) so the stored_at
cutoff logic (stored_at < cutoff) is triggered deterministically against the
frozen clock; keep event time the same but set StoredAt relative to clock.Now()
- InternalCollectionPeriod to cover both sides of the cutoff as needed.
- Line 322: The test currently omits a totals assertion for finalInvoice (see
finalInvoice and the missing RequireTotals call), so either restore the missing
assertion or track it with a follow-up ticket: open an issue for the
rating/progressive-billing work, then update the inline TODO to reference that
issue (e.g., TODO[rating]: totals are off — tracked in ISSUE-123) and add a
short comment explaining why RequireTotals was removed; alternatively, if you
want the test to fail when behavior changes, re-add the
RequireTotals(finalInvoice, ...) assertion in usagebased_test.go but guard it
with a clear TODO and/or t.Skip with the issue ID so the intent and tracking are
explicit.

In `@openmeter/billing/charges/usagebased/service/creditheninvoice.go`:
- Around line 263-269: Add a defensive assertion in
resolveInvoiceCreatedTrigger: normalize both billedPeriod.To and
charge.Intent.ServicePeriod.To (using meta.NormalizeTimestamp), then if billed >
service, either log/error or panic to surface the invariant breach instead of
silently treating it as partial; otherwise keep the exact-equality check and
return meta.TriggerFinalInvoiceCreated or meta.TriggerPartialInvoiceCreated as
before. This uses resolveInvoiceCreatedTrigger, billedPeriod.To and
charge.Intent.ServicePeriod.To to locate the change.

In `@openmeter/billing/charges/usagebased/service/lineengine_test.go`:
- Around line 53-87: The test name promises ensuring charge grouping is
preserved without child references but it never asserts ChargeID and uses a nil
SplitLineGroupID input (making nil assertions vacuous); update the test by
setting line.SplitLineGroupID to a non-nil value in the input GatheringLine and
add assertions that result.PreSplitAtLine.ChargeID and
result.PostSplitAtLine.ChargeID equal the input ChargeID, and assert that
result.PreSplitAtLine.SplitLineGroupID and
result.PostSplitAtLine.SplitLineGroupID are cleared (nil) after
SplitGatheringLine; also keep or adjust assertions for ChildUniqueReferenceID to
reflect that SplitGatheringLine clears ChildUniqueReferenceID per the
SplitGatheringLine implementation.

In `@openmeter/billing/charges/usagebased/service/lineengine.go`:
- Around line 413-424: The error returned from isUsageBasedSplitPeriodEmpty when
price == nil lacks context; update isUsageBasedSplitPeriodEmpty to include the
gathering line identifier in the error message (e.g., use line.GetId() or
line.GetLineId()) so callers like SplitGatheringLine will get useful context, or
alternatively return false (treat as "not empty") if you prefer a defensive
no-op; modify the branch that currently returns fmt.Errorf("price is nil") to
produce a contextual error string referencing the line ID or to return (false,
nil) depending on the chosen behavior.

In `@openmeter/billing/charges/usagebased/service/payments.go`:
- Around line 123-141: The function areAllInvoicedRunsSettled has subtle
semantics (it returns false unless there is at least one invoiced
final-realization run and all invoiced runs are settled); add a one-line doc
comment immediately above the areAllInvoicedRunsSettled function explaining that
it requires the presence of an invoiced final realization
(usagebased.RealizationRunTypeFinalRealization) and that it only returns true
when every run with InvoiceUsage has a Payment with StatusSettled, referencing
the function name areAllInvoicedRunsSettled and the payment.StatusSettled and
usagebased.RealizationRunTypeFinalRealization symbols so future readers
understand the guard.

In `@openmeter/billing/charges/usagebased/service/run/create.go`:
- Around line 36-38: Replace the plain fmt.Errorf return in the guard that
checks i.Charge.State.CurrentRealizationRunID inside CreateRatedRun with the
typed validation issue ErrActiveRealizationRunAlreadyExists (or the package's
validation issue constructor) so callers get the stable error code/HTTP 400
mapping; include the charge id as context/meta on that validation issue (using
the same message or metadata currently passed to fmt.Errorf) instead of
returning a raw error string.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3f6ef651-1d0c-4e9a-8d52-2c94802b5b31

📥 Commits

Reviewing files that changed from the base of the PR and between f95c443 and db51c1d.

⛔ Files ignored due to path filters (3)
  • openmeter/ent/db/chargeusagebased/chargeusagebased.go is excluded by !**/ent/db/**
  • openmeter/ent/db/chargeusagebasedruns/chargeusagebasedruns.go is excluded by !**/ent/db/**
  • openmeter/ent/db/migrate/schema.go is excluded by !**/ent/db/**
📒 Files selected for processing (15)
  • .agents/skills/charges/SKILL.md
  • openmeter/billing/charges/meta/triggers.go
  • openmeter/billing/charges/service/invoicable_test.go
  • openmeter/billing/charges/service/usagebased_test.go
  • openmeter/billing/charges/usagebased/errors.go
  • openmeter/billing/charges/usagebased/realizationrun.go
  • openmeter/billing/charges/usagebased/service/creditheninvoice.go
  • openmeter/billing/charges/usagebased/service/creditheninvoice_test.go
  • openmeter/billing/charges/usagebased/service/lineengine.go
  • openmeter/billing/charges/usagebased/service/lineengine_test.go
  • openmeter/billing/charges/usagebased/service/payments.go
  • openmeter/billing/charges/usagebased/service/run/create.go
  • openmeter/billing/charges/usagebased/service/run/payment_test.go
  • openmeter/billing/charges/usagebased/statemachine.go
  • test/credits/sanity_test.go

@turip turip changed the title Feat/skaffold progressive billing support feat: skaffold progressive billing support Apr 22, 2026
@turip turip added release-note/feature Release note: Exciting New Features area/billing labels Apr 22, 2026
@turip turip changed the title feat: skaffold progressive billing support feat[charges]: skaffold progressive billing support Apr 22, 2026
@turip turip merged commit 1bc752b into main Apr 23, 2026
28 of 29 checks passed
@turip turip deleted the feat/skaffold-progressive-billing-support branch April 23, 2026 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/billing release-note/feature Release note: Exciting New Features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants