Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
5 issues found across 58 files
Confidence score: 3/5
- There is meaningful regression risk in
server/src/internal/balances/updateBalance/updateNextResetAt.ts: allowing an emptycustomerIdwhenfullCustomer.idis missing could direct updates to an invalid customer record. server/src/internal/balances/updateBalance/updateGrantedBalance.tsappears user-impacting iffeatureIdis omitted, since updates may fall through to the first entitlement and modify the wrong balance; this is the highest-impact behavior risk in the set.- Test coverage signals are mixed:
server/tests/integration/balances/update/balance/update-balance-with-id.test.tsdoes not actually exercise the documented fallback path, andserver/tests/integration/balances/create/create-balance-with-id.test.tscan pass on non-conflict errors, so these may miss real regressions. - Pay close attention to
server/src/internal/balances/updateBalance/updateNextResetAt.ts,server/src/internal/balances/updateBalance/updateGrantedBalance.ts,server/tests/integration/balances/update/balance/update-balance-with-id.test.ts, andserver/tests/integration/balances/create/create-balance-with-id.test.ts- potential mis-targeted balance updates and insufficient assertions around fallback/conflict behavior.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="server/src/internal/balances/updateBalance/updateNextResetAt.ts">
<violation number="1" location="server/src/internal/balances/updateBalance/updateNextResetAt.ts:55">
P2: Avoid passing an empty string for customerId. If `fullCustomer.id` can be undefined, use a valid fallback (e.g., internal_id) or throw; otherwise updates may target an invalid customer record.</violation>
</file>
<file name="server/src/internal/customers/cusProducts/cusEnts/actions/updateCusEntDbAndCache.ts">
<violation number="1" location="server/src/internal/customers/cusProducts/cusEnts/actions/updateCusEntDbAndCache.ts:30">
P3: This cache update mapping duplicates the existing `CusEntService.syncUpdateToCache` logic. Consider reusing the existing helper or extracting a shared function to avoid divergence when the cache schema changes.</violation>
</file>
<file name="server/src/internal/balances/updateBalance/updateGrantedBalance.ts">
<violation number="1" location="server/src/internal/balances/updateBalance/updateGrantedBalance.ts:31">
P2: Making `featureId` optional without a guard allows updates to target the first entitlement when no feature or filters are provided, which can update the wrong balance. Consider requiring a featureId (or a specific filter like balance_id/cusEntId) and throwing an error when neither is supplied.</violation>
</file>
<file name="server/tests/integration/balances/update/balance/update-balance-with-id.test.ts">
<violation number="1" location="server/tests/integration/balances/update/balance/update-balance-with-id.test.ts:94">
P2: This test claims to validate `balance_id` fallback via internal id, but the update call uses a hardcoded id instead of `cusEntId`, so the fallback path is not actually exercised.</violation>
</file>
<file name="server/tests/integration/balances/create/create-balance-with-id.test.ts">
<violation number="1" location="server/tests/integration/balances/create/create-balance-with-id.test.ts:62">
P2: The duplicate-ID test claims to validate a 409 conflict but does not assert a specific error code/message, so it can pass on unrelated AutumnError failures.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| await customerEntitlementActions.updateDbAndCache({ | ||
| ctx, | ||
| customerId: fullCustomer.id ?? "", |
Contributor
There was a problem hiding this comment.
P2: Avoid passing an empty string for customerId. If fullCustomer.id can be undefined, use a valid fallback (e.g., internal_id) or throw; otherwise updates may target an invalid customer record.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/internal/balances/updateBalance/updateNextResetAt.ts, line 55:
<comment>Avoid passing an empty string for customerId. If `fullCustomer.id` can be undefined, use a valid fallback (e.g., internal_id) or throw; otherwise updates may target an invalid customer record.</comment>
<file context>
@@ -0,0 +1,59 @@
+
+ await customerEntitlementActions.updateDbAndCache({
+ ctx,
+ customerId: fullCustomer.id ?? "",
+ cusEntId: targetCusEnt.id,
+ updates: { next_reset_at: nextResetAt },
</file context>
| featureIds: [featureId], | ||
| entity: fullCus.entity, | ||
| fullCustomer, | ||
| featureIds: featureId ? [featureId] : undefined, |
Contributor
There was a problem hiding this comment.
P2: Making featureId optional without a guard allows updates to target the first entitlement when no feature or filters are provided, which can update the wrong balance. Consider requiring a featureId (or a specific filter like balance_id/cusEntId) and throwing an error when neither is supplied.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/internal/balances/updateBalance/updateGrantedBalance.ts, line 31:
<comment>Making `featureId` optional without a guard allows updates to target the first entitlement when no feature or filters are provided, which can update the wrong balance. Consider requiring a featureId (or a specific filter like balance_id/cusEntId) and throwing an error when neither is supplied.</comment>
<file context>
@@ -12,45 +11,38 @@ import {
- featureIds: [featureId],
- entity: fullCus.entity,
+ fullCustomer,
+ featureIds: featureId ? [featureId] : undefined,
+ entity: fullCustomer.entity,
inStatuses: orgToInStatuses({ org: ctx.org }),
</file context>
| customer_id: customerId, | ||
| feature_id: TestFeature.Messages, | ||
| included_grant: 100, | ||
| balance_id: "lifetime-balance", |
Contributor
There was a problem hiding this comment.
P2: This test claims to validate balance_id fallback via internal id, but the update call uses a hardcoded id instead of cusEntId, so the fallback path is not actually exercised.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/tests/integration/balances/update/balance/update-balance-with-id.test.ts, line 94:
<comment>This test claims to validate `balance_id` fallback via internal id, but the update call uses a hardcoded id instead of `cusEntId`, so the fallback path is not actually exercised.</comment>
<file context>
@@ -0,0 +1,202 @@
+ customer_id: customerId,
+ feature_id: TestFeature.Messages,
+ included_grant: 100,
+ balance_id: "lifetime-balance",
+ });
+
</file context>
| balance_id: "dup-balance", | ||
| }); | ||
|
|
||
| await expectAutumnError({ |
Contributor
There was a problem hiding this comment.
P2: The duplicate-ID test claims to validate a 409 conflict but does not assert a specific error code/message, so it can pass on unrelated AutumnError failures.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/tests/integration/balances/create/create-balance-with-id.test.ts, line 62:
<comment>The duplicate-ID test claims to validate a 409 conflict but does not assert a specific error code/message, so it can pass on unrelated AutumnError failures.</comment>
<file context>
@@ -0,0 +1,158 @@
+ balance_id: "dup-balance",
+ });
+
+ await expectAutumnError({
+ func: async () => {
+ await autumnV2.balances.create({
</file context>
| customerId, | ||
| }); | ||
|
|
||
| const cacheUpdates = [ |
Contributor
There was a problem hiding this comment.
P3: This cache update mapping duplicates the existing CusEntService.syncUpdateToCache logic. Consider reusing the existing helper or extracting a shared function to avoid divergence when the cache schema changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At server/src/internal/customers/cusProducts/cusEnts/actions/updateCusEntDbAndCache.ts, line 30:
<comment>This cache update mapping duplicates the existing `CusEntService.syncUpdateToCache` logic. Consider reusing the existing helper or extracting a shared function to avoid divergence when the cache schema changes.</comment>
<file context>
@@ -0,0 +1,53 @@
+ customerId,
+ });
+
+ const cacheUpdates = [
+ {
+ cus_ent_id: cusEntId,
</file context>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+3
to
+25
| export const DeleteBalanceParamsV0Schema = z.object({ | ||
| customer_id: z.string().meta({ | ||
| description: "The ID of the customer.", | ||
| }), | ||
| entity_id: z.string().optional().meta({ | ||
| description: "The ID of the entity.", | ||
| }), | ||
| feature_id: z.string().optional().meta({ | ||
| description: "The ID of the feature.", | ||
| }), | ||
| balance_id: z.string().optional().meta({ | ||
| description: "The ID of the balance to delete.", | ||
| }), | ||
|
|
||
| interval: z.enum(ResetInterval).optional().meta({ | ||
| description: | ||
| "Target a specific balance by its reset interval. Use when the customer has multiple balances for the same feature with different reset intervals.", | ||
| }), | ||
|
|
||
| customer_entitlement_id: z.string().optional().meta({ | ||
| internal: true, | ||
| }), | ||
| }); |
Contributor
There was a problem hiding this comment.
No targeting parameter required — could delete all entitlements
Both feature_id and balance_id are optional with no schema-level .refine() enforcing that at least one is provided. A caller passing only customer_id will cause fullCustomerToCustomerEntitlements to return all entitlements for that customer (across every feature). The isPaidCustomerEntitlement guard will block paid balances, but for a customer whose entitlements are all free/loose, every single entitlement will be deleted in one call.
Consider adding a refine at the bottom of the schema:
.refine(
(data) => data.feature_id !== undefined || data.balance_id !== undefined,
{ message: "Either feature_id or balance_id must be provided" },
)Prompt To Fix With AI
This is a comment left during a code review.
Path: shared/api/balances/delete/deleteBalanceParams.ts
Line: 3-25
Comment:
**No targeting parameter required — could delete all entitlements**
Both `feature_id` and `balance_id` are optional with no schema-level `.refine()` enforcing that at least one is provided. A caller passing only `customer_id` will cause `fullCustomerToCustomerEntitlements` to return **all** entitlements for that customer (across every feature). The `isPaidCustomerEntitlement` guard will block paid balances, but for a customer whose entitlements are all free/loose, **every single entitlement will be deleted in one call**.
Consider adding a refine at the bottom of the schema:
```typescript
.refine(
(data) => data.feature_id !== undefined || data.balance_id !== undefined,
{ message: "Either feature_id or balance_id must be provided" },
)
```
How can I resolve this? If you propose a fix, please make it concise.
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
1 participant
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.
Summary by cubic
Adds a new balances.delete endpoint and introduces balance_id on balances so you can target, update, and delete specific entries. Also switches APIs to included_grant, adds next_reset_at updates, and improves cache/DB sync.
New Features
Migration
Written for commit c4e6ac2. Summary will update on new commits.
Greptile Summary
This PR introduces a
balance_id/external_idscheme for customer entitlements, enabling callers to tag balances with stable user-defined IDs at create time and then target them precisely in subsequent update and delete calls. It also adds the newbalances.deleteRPC endpoint and refactors the update flow to fetch the full customer once upfront rather than once per sub-operation.Key changes:
balance_idfield on create/update/delete —external_idis stored oncustomer_entitlementsand exposed asbalance_idin the public API;fullCustomerToCustomerEntitlementsfilters byexternal_id ?? idso both explicit and fallback (internal-id) addressing work.balances.deleteendpoint —DELETE /v1/balances.deletewithDeleteBalanceParamsV0Schema; blocks deletion of paid (price-attached) entitlements and invalidates the full-customer cache on success.included_grantreplacesincluded/granted_balance— create and update params are renamed for consistency.updateNextResetAtextracted as a standalone helper — no longer requirescustomer_entitlement_id; targets by sorting onnext_reset_atascending with optionalbalance_id/intervalfilters.updateCusEntDbAndCacheaction — new action that atomically writes a cusEnt update to Postgres and the Redis FullCustomer cache, replacing the pattern of callingCusEntService.updateand then purging the full API cache.handleUpdateBalance— the customer is fetched once at the top of the handler and threaded throughrunUpdateBalanceV2,runUpdateUsage,updateGrantedBalance, andupdateNextResetAt.CusEntService.insertdefault balance — removes theas anycast and defaultsbalanceto0to avoid inserting nulls.customer_entitlements.entitlement_id— improves lookup performance for entitlement-based queries.Confidence Score: 1/5
.refine()guard in the delete params schema is a critical bug. A caller passing onlycustomer_idwill silently match and delete all non-paid entitlements for that customer in one call, when the intent is clearly to target a specific balance. This is a data loss issue that must be fixed before merge..refine()to enforce that eitherfeature_idorbalance_idis provided.Last reviewed commit: c4e6ac2