fix(reconcile): honor product behavior, reject a credit amount without the credits behavior, and keep feature names verbatim on apply

[rohilsurana]

What

Two ways the BillingProduct file's stated intent was not what got applied:

1. Behavior on create. Service.Create forced behavior = credits whenever credit_amount > 0, ignoring the behavior the file set. Now an explicit behavior is honored; only an omitted behavior on a credit product falls back to credits (the server's create-time default). The reconciler's diff mirrors the same rule, so a file that names a behavior the product was not created with fails the plan instead of being silently overridden.

2. Feature name case. The diff compared feature names lowercased and trimmed, but the apply sent them raw. Since the server looks features up by name, a case difference read as "no change" yet could fork a duplicate feature on a co-occurring update. Both sides now use one normalizeFeatureName helper.

Why

RFC 0001 Rule 2: the file is the desired state. The applied result must match what the file stated, with no silent override and no duplicate created behind a "no change" plan.

Testing

• New TestService_Create cases: an explicit behavior is honored on a credit product, and an omitted behavior defaults to credits.
• Reworked the diff behavior test and added a body test asserting feature names are normalized on apply.
• billing/... and internal/reconcile pass.

Stack

Stacked on fix/reconcile-permission-spicedb-grammar.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
frontier	Ready	Preview	Aug 19, 2026 9:41am

[coderabbitai]

Warning

Review limit reached

@rohilsurana, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 44 minutes

Limit details: You’ve used all 2 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 5693e8cb-f2e8-4665-92bd-215978947b5c

📥 Commits

Reviewing files that changed from the base of the PR and between 4c26238 and 10ce276.

📒 Files selected for processing (5)

• billing/product/service.go
• billing/product/service_test.go
• internal/reconcile/billingproduct.go
• internal/reconcile/billingproduct_reconciler.go
• internal/reconcile/billingproduct_test.go

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coveralls]

Coverage Report for CI Build 32238858271

Coverage increased (+0.03%) to 48.807%

Details

• Coverage increased (+0.03%) from the base build.
• Patch coverage: 20 of 20 lines across 3 files are fully covered (100%).
• No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	40111
Covered Lines:	19577
Line Coverage:	48.81%
Coverage Strength:	15.72 hits per line

---

💛 - Coveralls

[rohilsurana]

Review: honor behavior on create, match feature-name case on apply (RFC 0001 Rule 2)

Verified on this branch: go build ./..., go test ./internal/reconcile/... ./billing/product/..., and golangci-lint run on the changed packages all pass.

Both claims hold. Case normalization is now one shared function normalizeFeatureName (billingproduct.go:305), used by the diff (billingFeatureSetsEqual) and by apply (billingProductBody, billingproduct_reconciler.go:293), so apply cannot disagree with what the plan showed. The behavior fix captures statedBehavior before defaults (billing/product/service.go:67) and only folds to credits when the behavior is omitted, and the diff's expectedBehavior fold matches. I traced every create/diff/export combination and Rule 4 and Rule 5 both hold.

I also checked whether the same raw-case-on-apply gap remains for price names. It does not bite, because the billing service lowercases price names server-side on create and update (billing/product/service.go:456 and :518), unlike features. So leaving prices untouched is correct.

Verdict: approve with nits.

Nit:

1. The fix makes the diff and apply agree with each other, but not with the server's stored case. A pre-existing mixed-case feature (created via the CreateFeature API, or by the reconciler before this fix) still forks a duplicate when a co-occurring product change triggers an update. Concrete case: the server holds feature Foo on product tok, the file lists foo and also edits the description. The plan prints update product tok (description) with no feature change, but apply sends foo, the case-sensitive server lookup misses Foo, and a second foo row is created. This is narrow (legacy or externally created data) and a strict improvement over the old symmetric risk. A complete fix needs the server to canonicalize feature names on store and lookup.

[rohilsurana]

Follow-up on the nit.

Agreed, and left as is on purpose. This PR makes the diff and the apply agree on feature-name case, which removes the symmetric fork risk. The residual case, a legacy or externally created mixed-case feature row, needs the server to canonicalize feature names on store and on lookup. That is a server change outside this reconcile PR, so it is a follow-up, not part of this change.

[rohilsurana]

Re-review (head 75aa967)

The head moved only because of the rebase onto #1889. #1890's own patch is byte-for-byte identical to the previous review, so nothing in this PR's content changed. Build, tests, and lint are all green.

Re-confirmed: feature-name case is normalized by one shared normalizeFeatureName on both the diff path and the apply path, so apply cannot disagree with the plan. The behavior-on-create fold matches the diff, and the export round-trip plans zero ops.

The residual mixed-case feature fork is still open and needs a server-side feature-name canonicalization (UpsertFeature stores the name raw, unlike prices which the service lowercases). That is out of scope for this PR.

Verdict: approve with nits.