Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds tenancy-aware verify-data-integrity tooling: new API helpers, a payments verifier that reconstructs ledgers from transactions, Stripe payout integrity checks, a recursive progress helper, tighter product filtering in payments code, a script entrypoint update, and richer test fixtures for subscriptions. Changes
Sequence Diagram(s)sequenceDiagram
participant Index as verify-data-integrity/index.ts
participant Recurse as Recurse Helper
participant Verifier as Payments Verifier
participant DB as Prisma DB
participant API as Stack API
participant Stripe as Stripe API
Index->>Recurse: createRecurse()
Index->>API: createApiHelpers(currentOutputData, targetOutputData?)
Index->>Verifier: createPaymentsVerifier(projectId, tenancy, paymentsConfig, prisma, expectStatusCode)
Index->>Verifier: for each customer -> verifyCustomerPayments(customer)
Verifier->>DB: fetch transactions, subscriptions, purchases, item changes
Verifier->>Verifier: build expected ledger & owned products
Verifier->>API: expectStatusCode(customer item-quantities endpoint)
API-->>Verifier: return current item quantities
Verifier->>Verifier: compare expected vs actual
alt mismatch
Verifier->>DB: fetch detailed records for diagnostics
DB-->>Verifier: return records
Verifier-->>Index: throw StackAssertionError
else match
Verifier->>API: expectStatusCode(owned-products endpoint)
API-->>Verifier: return owned products
Verifier->>Verifier: compare expected owned products
end
Index->>API: fetch project transaction totals
Index->>Stripe: fetch Stripe balance transactions (account)
Stripe-->>Index: return balance totals
Index->>Index: compare totals, throw on mismatch
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Greptile OverviewGreptile SummaryThis PR adds comprehensive payment transaction verification to the data integrity script and fixes a critical Key Changes:
Bug Fix: Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Script as verify-data-integrity.ts
participant DB as Database
participant API as Stack API
participant Stripe as Stripe API
Script->>DB: Fetch projects with stripeAccountId
loop For each project
Script->>API: Get tenancy config
alt Has payments config
Script->>Script: createPaymentsVerifier()
Script->>API: fetchAllTransactionsForProject()
Script->>DB: Fetch subscriptions, purchases, item changes
alt Stripe account exists & not mock
Script->>Script: verifyStripePayoutIntegrity()
Script->>API: Get all money_transfer transactions
Script->>Stripe: Fetch balance transactions
Script->>Script: Compare totals (must match)
end
loop For each user
Script->>Script: verifyCustomerPayments()
Script->>Script: buildExpectedItemQuantitiesForCustomer()
Script->>API: Get actual item quantities
Script->>Script: Compare expected vs actual
Script->>API: Get owned products
Script->>Script: Compare expected vs actual products
end
loop For each team
Script->>Script: verifyCustomerPayments()
Note over Script: Same verification as users
end
loop For each custom customer
Script->>Script: verifyCustomerPayments()
Note over Script: Same verification as users
end
end
end
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/backend/scripts/verify-data-integrity.ts`:
- Around line 1127-1139: The query that builds extraItemQuantityChanges omits
customerType, causing cross-type matches; update the itemQuantityChange.findMany
where clause to include customerType: customer.customerType (or the appropriate
property from the customer object) alongside tenancyId and customerId so it
matches how getCustomerKey uniquely identifies customers; ensure the filter uses
the same customerType enum/string shape as other queries.
🧹 Nitpick comments (4)
apps/backend/scripts/verify-data-integrity.ts (4)
19-20: Clarify that this is a sentinel value, not a real key.The static analysis tool flagged this as a potential Stripe access token. While this is a false positive (it's a sentinel value for mock mode detection), adding a comment will prevent future confusion and suppress tooling warnings.
📝 Suggested clarification
const STRIPE_SECRET_KEY = getEnvVariable("STACK_STRIPE_SECRET_KEY", ""); +// Sentinel value used to detect mock mode - not a real API key const USE_MOCK_STRIPE_API = STRIPE_SECRET_KEY === "sk_test_mockstripekey";
493-557: Significant code duplication withpayments.tsx.
computeLedgerBalanceAtNow(lines 493-533) andaddWhenRepeatedItemWindowTransactions(lines 535-557) are nearly identical to their counterparts inapps/backend/src/lib/payments.tsx(lines 119-183). This creates a maintenance burden where changes must be synchronized across both files.Consider extracting these functions to a shared utility module that both files can import.
957-960: Avoid hardcoding project IDs.This hardcoded UUID is fragile and difficult to maintain. If the dummy project ID changes or additional projects need to be skipped, this requires a code change.
Consider using project metadata or a configuration flag to mark projects that should skip Stripe verification.
📝 Suggested approach
- if (options.projectId === '6fbbf22e-f4b2-4c6e-95a1-beab6fa41063') { - // Dummy project doesn't have a real stripe account, so we skip the verification. - return; - } + // Skip projects without valid Stripe accounts (e.g., dummy/test projects) + // Consider adding a flag to project metadata or configuration instead of hardcoding + const SKIP_STRIPE_VERIFICATION_PROJECT_IDS = new Set([ + '6fbbf22e-f4b2-4c6e-95a1-beab6fa41063', // Dummy project + ]); + if (SKIP_STRIPE_VERIFICATION_PROJECT_IDS.has(options.projectId)) { + return; + }Better long-term: Check for a project flag like
project.skipStripeVerificationor verify that the Stripe account is properly configured before running verification.
29-29: Consider importing or aliasing the PrismaCustomerTypeif applicable.This local
CustomerTypedefinition includes"custom"which may differ from the Prisma-generated enum. If there's a specific reason for the local definition (e.g., the verification script needs to handle additional customer types), a brief comment explaining the divergence would improve clarity.
N2D4
left a comment
N2D4
left a comment
There was a problem hiding this comment.
Can we split this up into multiple files? It's getting a bit much
(asking AI to do it should prolly be enough)
There was a problem hiding this comment.
Additional Suggestion:
Missing null check before calling .map() on a potentially undefined object property will cause a TypeError if the output file is missing the /api/v1/internal/projects/current endpoint.
View Details
📝 Patch Details
diff --git a/apps/backend/scripts/verify-data-integrity/index.ts b/apps/backend/scripts/verify-data-integrity/index.ts
index d9da13e5..97154d8f 100644
--- a/apps/backend/scripts/verify-data-integrity/index.ts
+++ b/apps/backend/scripts/verify-data-integrity/index.ts
@@ -97,17 +97,19 @@ async function main() {
// TODO next-release these are hacks for the migration, delete them
if (targetOutputData) {
- targetOutputData["/api/v1/internal/projects/current"] = targetOutputData["/api/v1/internal/projects/current"].map(output => {
- if ("config" in output.responseJson) {
- delete output.responseJson.config.id;
- output.responseJson.config.oauth_providers = output.responseJson.config.oauth_providers
- // `any` because this is historical output JSON from disk.
- // We intentionally keep this "migration hack" untyped.
- .filter((provider: any) => provider.enabled)
- .map((provider: any) => omit(provider, ["enabled"]));
- }
- return output;
- });
+ if (targetOutputData["/api/v1/internal/projects/current"]) {
+ targetOutputData["/api/v1/internal/projects/current"] = targetOutputData["/api/v1/internal/projects/current"].map(output => {
+ if ("config" in output.responseJson) {
+ delete output.responseJson.config.id;
+ output.responseJson.config.oauth_providers = output.responseJson.config.oauth_providers
+ // `any` because this is historical output JSON from disk.
+ // We intentionally keep this "migration hack" untyped.
+ .filter((provider: any) => provider.enabled)
+ .map((provider: any) => omit(provider, ["enabled"]));
+ }
+ return output;
+ });
+ }
}
console.log(`Loaded previous output data for verification`);
Analysis
Missing null check on endpoint property before calling .map() in verify-data-integrity script
What fails: Line 100 in apps/backend/scripts/verify-data-integrity/index.ts calls .map() on targetOutputData["/api/v1/internal/projects/current"] without checking if this property exists. If the output file (loaded from OUTPUT_FILE_PATH) is missing the /api/v1/internal/projects/current endpoint key, accessing it returns undefined, and calling .map() on undefined throws: TypeError: Cannot read properties of undefined (reading 'map')
How to reproduce:
- Generate an output file with
--save-outputthat doesn't include the/api/v1/internal/projects/currentendpoint (e.g., a partial or legacy output file) - Run the script with
--verify-outputflag - The script throws
TypeError: Cannot read properties of undefined (reading 'map')
Result: Script terminates with unhandled TypeError when verifying incomplete output files
Expected: Script should gracefully handle output files that don't contain all endpoints, skipping migration transformations for missing endpoints
Fix implemented: Added guard condition if (targetOutputData["/api/v1/internal/projects/current"]) before the .map() call to check if the property exists before processing it
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/backend/src/lib/payments.tsx`:
- Around line 334-336: The code currently treats s.product as possibly undefined
(const product = s.product as yup.InferType<typeof productSchema> | undefined)
and simply skips pushing that subscription, which can silently drop
entitlements; instead update the logic in the loop that builds subscriptions to
(a) attempt to reconstruct the product from your canonical config using the
subscription's productId when product is undefined, and only if reconstruction
fails throw an explicit error (or log and throw) rather than continue; adjust
the block that references product, productSchema, s.product, productId and the
subscriptions.push call so missing product data is either rebuilt from config or
causes a loud failure.
N2D4
left a comment
N2D4
left a comment
There was a problem hiding this comment.
looks great! let's wait a few hours with merging tho until after the Swift SDK if that's okay? anything after 4pm today should be good
2 participants
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.