chore(billing): batch 4 — RUNBOOKS accuracy, params validation, cursor pagination, log levels#3623
Conversation
…r pagination, log levels 1. RUNBOOKS.md: #2 dead-letter retry count 3→5 (matches BILLING_WEBHOOK_MAX_ATTEMPTS=5); #3 plans/bump body: drop 'reason' field (not in AdminBumpPlanRequest schema, was 422) 2. Zod params validation on admin endpoints: AdminOrgIdParam (ObjectId regex) + AdminEventIdParam (evt_* regex) wired via safeParse in adminGetCustomerStatus, adminSyncFromStripe, adminCancelSubscription, adminPurgeDeadLetter → 422 on invalid input 3. Cursor-based pagination in reconcile service: replace skip+limit with _id > lastSeenId cursor in _fetchPage / findPageForReconciliation. Stable across new subscription inserts mid-run. Tests for multi-page + cursor stability. 4. dispute.funds_reinstated log level: logger.warn → logger.error in handleChargeDisputeFundsReinstated (action-required state, not warn-noise) 5. creditPack duplicate observability: log result.applied=false at debug level in handleCheckoutPaymentCompleted to surface Stripe redelivery phantom calls 6. billing.init.js console.warn → logger.warn (6 occurrences)
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR consolidates multiple billing module improvements: parameter validation for admin routes via new Zod schemas, cursor-based pagination in reconciliation for stability, migration of startup warnings to structured logging, and refinements to webhook idempotency handling and logging levels. All changes maintain backward compatibility and existing behavior while improving observability, robustness, and type safety. ChangesBilling Module Enhancements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #3623 +/- ##
==========================================
- Coverage 88.83% 88.82% -0.02%
==========================================
Files 134 134
Lines 4595 4616 +21
Branches 1416 1423 +7
==========================================
+ Hits 4082 4100 +18
- Misses 400 403 +3
Partials 113 113
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR is the final batch of the post‑V3 billing audit fix plan, focusing on operational correctness (runbooks/logging) and hardening billing admin + reconciliation flows (validation + stable pagination).
Changes:
- Added Zod-based path param validation for several billing admin endpoints (422 on invalid
orgId/eventId) with added/updated tests. - Replaced skip/limit pagination in billing reconciliation with cursor-based pagination (
_id > lastSeenId) and added unit tests for cursor behavior. - Improved operational observability: corrected runbook examples, adjusted log levels, added debug logging for idempotent duplicate Stripe deliveries, and replaced
console.warnwithlogger.warnin billing init (tests updated accordingly).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| modules/billing/tests/billing.refund.service.unit.tests.js | Updates schema module mock to include newly imported param validators. |
| modules/billing/tests/billing.reconcile.service.unit.tests.js | Adds unit tests verifying cursor pagination semantics and stability. |
| modules/billing/tests/billing.init.unit.tests.js | Updates tests to assert logger.warn instead of console.warn. |
| modules/billing/tests/billing.admin.integration.tests.js | Adds integration coverage for 422 behavior on invalid path params and “reaches service” on valid params. |
| modules/billing/services/billing.webhook.service.js | Adds debug log for duplicate extras creditPack; elevates dispute funds reinstated log to error level. |
| modules/billing/services/billing.reconcile.service.js | Switches reconciliation pagination from page index to _id cursor (lastSeenId). |
| modules/billing/RUNBOOKS.md | Fixes runbook attempt count and corrects bump-plan example body. |
| modules/billing/repositories/billing.subscription.repository.js | Implements cursor-based reconciliation page fetch using _id > lastSeenId and removes skip. |
| modules/billing/models/billing.subscription.schema.js | Introduces AdminOrgIdParam and AdminEventIdParam Zod schemas for path params. |
| modules/billing/controllers/billing.admin.controller.js | Wires new param schemas via safeParse for specific admin endpoints and returns 422 on invalid path params. |
| modules/billing/billing.init.js | Replaces console.warn with logger.warn for boot-time warnings and non-fatal analytics errors. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/billing/controllers/billing.admin.controller.js (1)
249-260:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
adminDisputeCreditis the only:orgIdroute that skipsAdminOrgIdParamvalidation.Line 252 reads
req.params.orgIdraw while every otherorgId-bearing route in this PR now callsAdminOrgIdParam.safeParse(req.params)first. With an invalidorgId, execution falls through toBillingAdminService.creditDisputeReinstated, which eventually hits Mongoose'sObjectId.isValidguard and returnsnull— likely surfacing as an opaque 404 or 500 instead of the expected 422 "Invalid path parameters".🛡️ Proposed fix
const adminDisputeCredit = async (req, res) => { try { - const { chargeId, amountCents, reason, refundRequestId } = req.body; - const { orgId } = req.params; + const parsedParams = AdminOrgIdParam.safeParse(req.params); + if (!parsedParams.success) { + return responses.error(res, 422, 'Unprocessable Entity', 'Invalid path parameters')(parsedParams.error); + } + const { orgId } = parsedParams.data; + const { chargeId, amountCents, reason, refundRequestId } = req.body;🤖 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 `@modules/billing/controllers/billing.admin.controller.js` around lines 249 - 260, The adminDisputeCredit handler reads req.params.orgId directly and skips the AdminOrgIdParam validation; update adminDisputeCredit to call AdminOrgIdParam.safeParse(req.params) (as other routes do), return a 422 error when safeParse fails, and then use the validated orgId value when calling BillingAdminService.creditDisputeReinstated (and any subsequent calls) so invalid path params are rejected early with the proper 422 instead of allowing Mongoose/ObjectId errors to surface.
🤖 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 `@modules/billing/tests/billing.admin.integration.tests.js`:
- Around line 535-550: Add an assertion that the mocked service method
BillingAdminService.purgeDeadLetter was invoked with the eventId parameter used
in the test ('evt_1ABC'); after runHandlers completes, add
expect(BillingAdminService.purgeDeadLetter).toHaveBeenCalledWith('evt_1ABC') (or
the equivalent Jest matcher) to ensure the DELETE /dead-letters/:eventId route
wires the eventId through to purgeDeadLetter.
- Around line 476-491: The test currently only asserts res.status(200) but
should also assert that the controller called the service; add an assertion that
BillingAdminService.getCustomerStatus was called once with the expected orgId
string (e.g. '507f1f77bcf86cd799439011') after runHandlers completes,
referencing the mocked BillingAdminService.getCustomerStatus to verify
controller→service wiring.
---
Outside diff comments:
In `@modules/billing/controllers/billing.admin.controller.js`:
- Around line 249-260: The adminDisputeCredit handler reads req.params.orgId
directly and skips the AdminOrgIdParam validation; update adminDisputeCredit to
call AdminOrgIdParam.safeParse(req.params) (as other routes do), return a 422
error when safeParse fails, and then use the validated orgId value when calling
BillingAdminService.creditDisputeReinstated (and any subsequent calls) so
invalid path params are rejected early with the proper 422 instead of allowing
Mongoose/ObjectId errors to surface.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: e7264ea4-157f-47cf-bc1a-c9e191ec4aab
📒 Files selected for processing (11)
modules/billing/RUNBOOKS.mdmodules/billing/billing.init.jsmodules/billing/controllers/billing.admin.controller.jsmodules/billing/models/billing.subscription.schema.jsmodules/billing/repositories/billing.subscription.repository.jsmodules/billing/services/billing.reconcile.service.jsmodules/billing/services/billing.webhook.service.jsmodules/billing/tests/billing.admin.integration.tests.jsmodules/billing/tests/billing.init.unit.tests.jsmodules/billing/tests/billing.reconcile.service.unit.tests.jsmodules/billing/tests/billing.refund.service.unit.tests.js
…th param Completes the Batch 4 admin endpoint param coverage — adminDisputeCredit /:orgId was missed in the initial pass. Now all 5 :orgId admin endpoints validate with AdminOrgIdParam.safeParse → 422 on invalid ObjectId.
…lidation tests CodeRabbit nit: tests asserting "reaches the service" now also verify the controller passed the validated param to the service method (getCustomerStatus called with orgId, purgeDeadLetter called with eventId).
PierreBrisorgueil
dismissed
coderabbitai[bot]’s stale review
All findings addressed: adminDisputeCredit orgId validation added (2 commits), 2 nitpick threads resolved.
Summary
Final batch (4/4) of the post-V3 billing audit fix plan. 6 items:
BILLING_WEBHOOK_MAX_ATTEMPTS = 5); 17 roles #3plans/bumpexample body dropsreasonfield (absent fromAdminBumpPlanRequestschema, was yielding 422 in production)AdminOrgIdParam(ObjectId regex) +AdminEventIdParam(evt_*regex) wired viasafeParseinadminGetCustomerStatus,adminSyncFromStripe,adminCancelSubscription,adminPurgeDeadLetter→ returns 422 on invalid input; test coverage addedskip+limitwith_id > lastSeenIdcursor in_fetchPage/findPageForReconciliation. Stable across new subscription inserts mid-run (skip offsets shift silently). Tests for first-page null cursor, second-page cursor, and 3-page mid-run stabilityhandleChargeDisputeFundsReinstated—logger.warn→logger.error(action-required state: ops must apply manual ledger credit)result.applied = falseatdebuglevel inhandleCheckoutPaymentCompletedto surface duplicate Stripe session redeliveriesbilling.init.js(6 occurrences); init tests updated to assert onmockLogger.warninstead ofconsole.warnspyTest plan
npm run lint— ESLint cleannpm run test:unit— 1321 passed (all green)npm run test:integration— 391 passed; 3 pre-existing failures inbilling.extraBalance.listLedger.perf(unrelated, confirmed on master)Summary by CodeRabbit
Bug Fixes
Improvements