fix: ignore invalid entitlement value#4270
Draft
chrisgacsal wants to merge 1 commit into
Draft
Conversation
chrisgacsal
requested review from
GAlexIHU,
gergely-kurucz-konghq,
tothandras and
turip
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a metered-entitlement validation API and exported ErrInvalidEntitlementValue, validates entitlement values in the threshold selection flow, changes per-threshold guards (use totalGrants vs absolute zero), adjusts activation logic to use usage+overage for certain types, and updates tests to assert new validation/error cases. ChangesEntitlement validation → Threshold evaluation
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 1
🧹 Nitpick comments (1)
openmeter/notification/consumer/entitlementbalancethreshold_test.go (1)
650-655: 💤 Low valueTiny nit: assert
actualisnilin the error path too.When
ExpectedErr != nil, the test only checks the error but not the returned value. SincegetActiveThresholdsWithHighestPriorityalways returnsnilon error, adding one line keeps the test contract tight and protects against future refactors that might accidentally return a partial result.🔧 Suggested addition
} else { assert.ErrorIsf(t, err, test.ExpectedErr, "must return the expected error: %s", err) + assert.Nilf(t, actual, "must not return a value when an error is expected") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openmeter/notification/consumer/entitlementbalancethreshold_test.go` around lines 650 - 655, The error-path in the test for getActiveThresholdsWithHighestPriority only asserts the error and not the returned value; update the else branch (where test.ExpectedErr != nil) to also assert that actual is nil (e.g., using assert.Nil or assert.Empty) so the test verifies the function returns no result on error and prevents regressions returning partial data.
🤖 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/notification/consumer/entitlementbalancethreshold.go`:
- Around line 406-408: The new validation incorrectly compares b := totalGrants
- (usage + overage) to balance without an epsilon, causing float-noise false
positives and breaking the test; change the check in
entitlementbalancethreshold.go to use the existing absoluteZero tolerance (e.g.
if b - balance > absoluteZero { return ... }) so tiny rounding differences are
ignored, and update the failing test data in entitlementbalancethreshold_test.go
(Test_GetActiveThresholdsWithHighestPriority_Error) to make
TotalAvailableGrantAmount equal to 25.0 + 2e-9 (so b == 1e-9) so the test
satisfies the new invariant while keeping the original absoluteZero boundary
assertions.
---
Nitpick comments:
In `@openmeter/notification/consumer/entitlementbalancethreshold_test.go`:
- Around line 650-655: The error-path in the test for
getActiveThresholdsWithHighestPriority only asserts the error and not the
returned value; update the else branch (where test.ExpectedErr != nil) to also
assert that actual is nil (e.g., using assert.Nil or assert.Empty) so the test
verifies the function returns no result on error and prevents regressions
returning partial data.
🪄 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: 4c35a3d0-2a6d-4e59-8235-e143ec362834
📒 Files selected for processing (2)
openmeter/notification/consumer/entitlementbalancethreshold.goopenmeter/notification/consumer/entitlementbalancethreshold_test.go
chrisgacsal
force-pushed
the
fix/invalid-entitlement-value
branch
from
aaed905 to
8829064
Compare
chrisgacsal
force-pushed
the
fix/invalid-entitlement-value
branch
from
8829064 to
c2d63db
Compare
gergely-kurucz-konghq
previously approved these changes
May 5, 2026
chrisgacsal
force-pushed
the
fix/invalid-entitlement-value
branch
from
c2d63db to
6911d0d
Compare
tothandras
approved these changes
May 5, 2026
tothandras
reviewed
May 5, 2026
| if balance > absoluteZero && overage > absoluteZero { | ||
| return nil, errors.New("balance and overage cannot be positive number at the same time") | ||
| // TotalAvailableGrants must be equal or greater than the sum of Balance, Usage and Overage. | ||
| if balance+usage+overage < totalGrants { |
tothandras
previously approved these changes
May 5, 2026
chrisgacsal
force-pushed
the
fix/invalid-entitlement-value
branch
from
6911d0d to
048d3ee
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
openmeter/notification/consumer/entitlementbalancethreshold_test.go (1)
99-113: 💤 Low valueTiny nit: this fixture is internally inconsistent — consider mirroring the sibling case.
The next case ("with overage over the total available grant amount", L114-129) properly sets
Overage: 20.0so thatusage - overagematches the granted amount. This one hasUsage: 100,TotalAvailableGrantAmount: 50and no overage, which can't physically happen (you can't burn 100 from 50 grants without overage). The new loose invariant check still passes it, but it's a bit of a misleading fixture — addingOverage: lo.ToPtr(50.0)would make it model a realistic state.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/notification/consumer/entitlementbalancethreshold_test.go` around lines 99 - 113, Test fixture "Usage values only - over the total available grant amount" has inconsistent EntitlementValue (Usage 100 with TotalAvailableGrantAmount 50 and no Overage); update the EntitlementValue in that case to include an Overage (e.g. set Overage via lo.ToPtr(50.0)) so that Usage - Overage equals TotalAvailableGrantAmount, mirroring the sibling case and keeping the fixture physically realistic; locate the case by its Name string and modify the EntitlementValue.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openmeter/notification/consumer/entitlementbalancethreshold_test.go`:
- Around line 732-745: The test case named "Invalid entitlement value - total
available grants exceeds balance+usage+overage" is incorrectly using
TotalAvailableGrantAmount: -1.0 (which triggers the negative-value guard)
instead of a positive value larger than the sum of Balance+Usage+Overage; update
the EntitlementValue in that test (the TotalAvailableGrantAmount field in the
table entry) to a non-negative number greater than Balance+Usage+Overage (e.g.,
if Balance/Usage/Overage are 0.0 set TotalAvailableGrantAmount to 1.0) so the
code path in the entitlement validation (the invariant check around the logic
that raises ErrInvalidEntitlementValue) is actually exercised.
---
Nitpick comments:
In `@openmeter/notification/consumer/entitlementbalancethreshold_test.go`:
- Around line 99-113: Test fixture "Usage values only - over the total available
grant amount" has inconsistent EntitlementValue (Usage 100 with
TotalAvailableGrantAmount 50 and no Overage); update the EntitlementValue in
that case to include an Overage (e.g. set Overage via lo.ToPtr(50.0)) so that
Usage - Overage equals TotalAvailableGrantAmount, mirroring the sibling case and
keeping the fixture physically realistic; locate the case by its Name string and
modify the EntitlementValue.
🪄 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: b53cffb9-ec32-4812-9b43-cfd3ea1509b0
📒 Files selected for processing (2)
openmeter/notification/consumer/entitlementbalancethreshold.goopenmeter/notification/consumer/entitlementbalancethreshold_test.go
tothandras
previously approved these changes
May 6, 2026
chrisgacsal
force-pushed
the
fix/invalid-entitlement-value
branch
from
048d3ee to
d587f8c
Compare
GAlexIHU
reviewed
May 6, 2026
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
openmeter/entitlement/snapshot/event.go (1)
134-137:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNo FP tolerance on the sum comparison — risk of false invalidations.
balance+usage+overage < totalGrantsis a bare floating-point compare. When these values come out of multi-step aggregations, rounding drift can produce something likebalance+usage+overage = 99.99999999versustotalGrants = 100.0for what should be an equal pair, which would then incorrectly raiseErrInvalidEntitlementValueand cause legitimate snapshots to be dropped. A small epsilon keeps things consistent withabsoluteZeroused elsewhere.🛡️ Proposed fix (mirror the absoluteZero pattern)
- // TotalAvailableGrants must be less than or equal to the sum of Balance, Usage and Overage. - if balance+usage+overage < totalGrants { + // TotalAvailableGrants must be less than or equal to the sum of Balance, Usage and Overage (with FP tolerance). + const epsilon = 1e-9 + if totalGrants-(balance+usage+overage) > epsilon { return fmt.Errorf("%w: total available grants must be less than or equal to the sum of balance, usage and overage [balance=%f, usage=%f, overage=%f, total_grants=%f]", ErrInvalidEntitlementValue, balance, usage, overage, totalGrants) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/entitlement/snapshot/event.go` around lines 134 - 137, The strict float compare in the TotalAvailableGrants check should use the project's epsilon tolerance to avoid false negatives; replace the bare `balance+usage+overage < totalGrants` check in event.go with an epsilon-aware comparison using the existing `absoluteZero` constant (e.g., compute sum := balance+usage+overage and test `if sum < totalGrants - absoluteZero { ... }`) and keep the same error return using `ErrInvalidEntitlementValue` so behavior is identical except for FP tolerance.
🧹 Nitpick comments (1)
openmeter/entitlement/snapshot/event.go (1)
122-140: ⚡ Quick winPrefer a function declaration over a mutable package-level
var.
ValidateMeteredEntitlementValueis exposed as a publicvarholding a func, which means any importer can reassign it at runtime (e.g.snapshot.ValidateMeteredEntitlementValue = func(EntitlementValue) error { return nil }) and silently disable validation. Since the signature already matchespkgmodels.ValidatorFunc[EntitlementValue], a regularfuncworks just as well and gives you immutability + a clean stack trace.♻️ Proposed refactor
-var ValidateMeteredEntitlementValue = func(value EntitlementValue) error { +func ValidateMeteredEntitlementValue(value EntitlementValue) error { var ( balance = lo.FromPtr(value.Balance) usage = lo.FromPtr(value.Usage) overage = lo.FromPtr(value.Overage) totalGrants = lo.FromPtr(value.TotalAvailableGrantAmount) ) @@ return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openmeter/entitlement/snapshot/event.go` around lines 122 - 140, The exported ValidateMeteredEntitlementValue is defined as a package-level var holding a func which allows runtime reassignment; change it to a regular function declaration "func ValidateMeteredEntitlementValue(value EntitlementValue) error" preserving the existing logic and error returns (references: EntitlementValue, ErrInvalidEntitlementValue, lo.FromPtr) so it cannot be mutated by importers and retains proper stack traces.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@openmeter/entitlement/snapshot/event.go`:
- Around line 134-137: The strict float compare in the TotalAvailableGrants
check should use the project's epsilon tolerance to avoid false negatives;
replace the bare `balance+usage+overage < totalGrants` check in event.go with an
epsilon-aware comparison using the existing `absoluteZero` constant (e.g.,
compute sum := balance+usage+overage and test `if sum < totalGrants -
absoluteZero { ... }`) and keep the same error return using
`ErrInvalidEntitlementValue` so behavior is identical except for FP tolerance.
---
Nitpick comments:
In `@openmeter/entitlement/snapshot/event.go`:
- Around line 122-140: The exported ValidateMeteredEntitlementValue is defined
as a package-level var holding a func which allows runtime reassignment; change
it to a regular function declaration "func ValidateMeteredEntitlementValue(value
EntitlementValue) error" preserving the existing logic and error returns
(references: EntitlementValue, ErrInvalidEntitlementValue, lo.FromPtr) so it
cannot be mutated by importers and retains proper stack traces.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 79ec3647-b82e-4b6e-b4c0-15fd2bdde5db
📒 Files selected for processing (4)
openmeter/entitlement/snapshot/event.goopenmeter/entitlement/snapshot/event_test.goopenmeter/notification/consumer/entitlementbalancethreshold.goopenmeter/notification/consumer/entitlementbalancethreshold_test.go
chrisgacsal
force-pushed
the
fix/invalid-entitlement-value
branch
from
d587f8c to
b7be048
Compare
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
4 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
Validate entitlement value before processing balance threshold events.
Summary by CodeRabbit
New Features
Bug Fixes
Tests