fix(issue-88): correct ledger-to-Stripe reconciliation across 5 bugs#149
Conversation
📝 WalkthroughWalkthroughReconciliation now uses a module child logger, wraps Stripe API calls in try/catch, paginates transactions with a MAX_TRANSACTIONS cap, computes sign-corrected net captured amounts (accounting for refunds), and flags missing virtual-card vs pot state discrepancies. Tests and integration fixtures were updated accordingly. ChangesStripe Reconciliation Bug Fixes
Sequence Diagram(s)sequenceDiagram
participant Reconciler as ReconciliationService
participant DB as Database (Prisma)
participant Stripe as Stripe API
participant Logger as Logger.child()
Reconciler->>DB: fetch intent, pot, ledger entries, virtualCard
DB-->>Reconciler: intent/pot/ledger/virtualCard
alt virtualCard missing
Reconciler->>Reconciler: inspect pot.status & pot.settledAmount
Reconciler->>Logger: warn/info about missing virtualCard and pot state
else virtualCard exists
Reconciler->>Stripe: cards.retrieve(cardId)
Stripe-->>Reconciler: card data or error
opt card retrieved
Reconciler->>Stripe: issuing.transactions.autoPagingToArray({ limit: 1000 })
Stripe-->>Reconciler: paged transactions or error
Reconciler->>Reconciler: compute net totalCaptured (abs amounts, subtract refunds)
Reconciler->>Logger: info/debug (capture totals, pagination, truncation)
end
end
Reconciler->>Reconciler: build report, add discrepancies as needed
Reconciler->>Logger: error (on Stripe failures) / info
Reconciler-->>Caller: ReconciliationReport (inSync, stripeReport, discrepancies)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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 docstrings
🧪 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. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/stripe/reconciliation.test.ts`:
- Around line 119-123: The cleanup currently swallows all errors via empty
.catch() on calls like prisma.ledgerEntry.deleteMany, prisma.pot.deleteMany,
prisma.virtualCard.deleteMany, prisma.purchaseIntent.deleteMany and
prisma.user.deleteMany which hides real teardown failures; change these to
explicitly guard for missing IDs (check intentId/userId before calling) and
remove the empty .catch() so real errors surface, or wrap the cleanup sequence
in a single try/catch that logs the error with context (including
intentId/userId) and rethrows so test harness can fail on genuine DB errors.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 51484180-135a-4c90-8cbb-db3bf84edff3
📒 Files selected for processing (3)
src/payments/providers/stripe/reconciliationService.tstests/integration/stripe/reconciliation.test.tstests/unit/payments/reconciliationService.test.ts
JonasBaeumer
left a comment
JonasBaeumer
left a comment
There was a problem hiding this comment.
Requesting changes on one critical safety issue, plus two minor nits.
Critical (must fix before merge)
The afterAll cleanup in tests/integration/stripe/reconciliation.test.ts combines unguarded module-scoped let intentId / let userId with prisma.<table>.deleteMany({ where: { intentId } }).catch(() => {}). Because Prisma treats where: { intentId: undefined } as no filter, a partial beforeAll failure (any Stripe API hiccup before intentId is assigned) would cause the cleanup to delete every row in LedgerEntry, Pot, VirtualCard, PurchaseIntent, and User — and the silent catch would hide it.
Full analysis and suggested patch in the existing CodeRabbit thread. The if (intentId) / if (userId) guards are the load-bearing fix; removing the catches is the secondary observability fix. Please do both.
Nitpicks
Two line comments below — non-blocking, do as you like.
Otherwise: strong PR
Clean fixes for all 5 reconciliation bugs, well-organized tests by bug number, and the integration test passes against real Stripe for the first time. The bug-1 sign analysis and bug-2 in-flight reasoning in the comments are particularly good. Resolving the cleanup safety issue and this is good to ship.
|
Addressed teardown safety review: guarded cleanup on intentId/userId and removed silent catches so teardown failures surface.\n\nCommit: 4ef3604\n\nLocal verification:\n- npm test -- --testPathPattern=unit/payments/reconciliationService (passes)\n- npm run lint / format:check / build (no errors)\n\nStripe integration test is still gated by STRIPE_SECRET_KEY, so I couldn't run it locally here without credentials; CI is running now. |
|
CI is green now (Lint/Typecheck/Unit/Integration/CodeQL all passed) and the teardown safety issue is fixed.\n\n@JonasBaeumer could you please re-review / dismiss the previous change request when you have a moment? |
JonasBaeumer
left a comment
JonasBaeumer
left a comment
There was a problem hiding this comment.
LGTM, please fix the conflicts and you are good to merge @georgyia
The reconciliation feature reported false discrepancies on every settled
intent and had no error handling around its Stripe calls. All five bugs
listed in the issue are addressed:
1. Sign convention. Stripe Issuing returns capture amounts as negative
integers and refund amounts as positive. The previous code summed
`t.amount` directly and compared the resulting negative number to a
positive `settledAmount`, producing a discrepancy on every run.
`totalCaptured` is now computed as `sum of |amount| for non-refunds
minus |amount| for refunds`, giving the comparable positive net.
2. Refunds were excluded by `type: 'capture'` filter, so a partial
refund overstated `totalCaptured`. The filter is removed; refunds
are now subtracted from the net.
3. Pagination. `transactions.list` returned at most 100 rows by
default, silently understating `totalCaptured` for high-volume
intents. We now use `autoPagingToArray({ limit: 1000 })` and emit a
discrepancy if the safety cap is hit so the truncation is visible
rather than silent.
4. Stripe API error handling. `cards.retrieve` and `transactions.list`
are wrapped in try/catch. A 404/429/network blip now produces a
structured discrepancy report (`{ inSync: false, stripe: null,
discrepancies: ['stripe API error: ...'] }`) and a logged error,
instead of an unhandled exception that the webhook caller's outer
`.catch` swallowed without trace.
5. Missing virtualCard. The previous behaviour returned
`{ inSync: true }` whenever no `VirtualCard` row existed — even
when the pot showed money had already moved (settled/returned).
That's a false clean signal: a settled pot with no card record is
itself the discrepancy. The check now distinguishes the two:
- no card AND (no pot OR active in-flight pot) → `inSync: true`
- no card AND settled/returned pot → `inSync: false` with discrepancy
Tests:
- 14 new unit tests in `tests/unit/payments/reconciliationService.test.ts`
cover each bug independently: negative-amount handling, refund
netting, both Stripe-error paths, paging with the safety cap,
truncation discrepancy, all four no-card states, plus the existing
card-status check.
- The integration test in `tests/integration/stripe/reconciliation.test.ts`
was previously broken at setup (missing required `idempotencyKey`,
`userId` on Pot/LedgerEntry, and an authorization-capture sequence
that needs a webhook approver). Switched it to
`testHelpers.issuing.transactions.createForceCapture` so it runs
self-contained, and supplied the required fields. Both integration
tests now PASS for the first time against real Stripe test mode.
- Improved cleanup to delete child rows in FK order so the test leaves
no orphans.
Closes #88
Guard intentId/userId before deleteMany to avoid unfiltered deletes if setup fails, and stop swallowing teardown errors so genuine cleanup issues fail the test.
georgyia
force-pushed
the
fix/issue-88-reconciliation-bugs
branch
from
4ef3604 to
877e53f
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/unit/payments/reconciliationService.test.ts (1)
78-95: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd one unit test for a non-
refundcredit-side transaction case.This block validates refund subtraction, but it doesn’t pin behavior for other credit-side rows. Adding that case will prevent regressions in
totalCapturedcalculation.🤖 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 `@tests/unit/payments/reconciliationService.test.ts` around lines 78 - 95, Add a second unit test in the same describe block that exercises reconcileIntent with a credit-side transaction that is NOT type 'refund' (e.g., type: 'credit' or 'adjustment') so we verify totalCaptured does not subtract it; update the mocked responses used in the existing test harness (mockPot, mockLedgerEntries, mockVirtualCard, mockStripe.issuing.cards.retrieve, mockAutoPagingToArray) to return a capture and a non-refund credit-side row and assert reconcileIntent('intent-3') produces the expected report.stripe.totalCaptured and inSync values, referencing the reconcileIntent function and mockAutoPagingToArray/mocked response shapes used in the current test file.src/payments/providers/stripe/reconciliationService.ts (1)
83-89:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle all credit-side transactions in net capture math, not only
refund.Line 85 only subtracts when
t.type === 'refund'. Credit-side rows that are not exactlyrefundare added in theelsebranch, which can overstatetotalCapturedand create false reconciliation outcomes.Suggested fix
let totalCaptured = 0; for (const t of transactions) { - const abs = Math.abs(t.amount); - if (t.type === 'refund') { - totalCaptured -= abs; - } else { - totalCaptured += abs; - } + // Stripe issuing captures are debit-side (negative), credits are positive. + // Convert to comparable positive net captured amount. + totalCaptured -= t.amount; }🤖 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 `@src/payments/providers/stripe/reconciliationService.ts` around lines 83 - 89, The loop that updates totalCaptured misclassifies credit-side transactions by only treating t.type === 'refund' as negative; update the logic in the for (const t of transactions) loop (where totalCaptured, t.amount, and t.type are used) to subtract abs when the transaction is a credit-type rather than only 'refund' — e.g., define a creditTypes set (e.g., ['refund','chargeback','credit','refund_reversal'] or use the existing t.side/direction if available) and change the condition to if (creditTypes.has(t.type) /* or t.side === 'credit' */) { totalCaptured -= abs } else { totalCaptured += abs }.
🤖 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.
Outside diff comments:
In `@src/payments/providers/stripe/reconciliationService.ts`:
- Around line 83-89: The loop that updates totalCaptured misclassifies
credit-side transactions by only treating t.type === 'refund' as negative;
update the logic in the for (const t of transactions) loop (where totalCaptured,
t.amount, and t.type are used) to subtract abs when the transaction is a
credit-type rather than only 'refund' — e.g., define a creditTypes set (e.g.,
['refund','chargeback','credit','refund_reversal'] or use the existing
t.side/direction if available) and change the condition to if
(creditTypes.has(t.type) /* or t.side === 'credit' */) { totalCaptured -= abs }
else { totalCaptured += abs }.
In `@tests/unit/payments/reconciliationService.test.ts`:
- Around line 78-95: Add a second unit test in the same describe block that
exercises reconcileIntent with a credit-side transaction that is NOT type
'refund' (e.g., type: 'credit' or 'adjustment') so we verify totalCaptured does
not subtract it; update the mocked responses used in the existing test harness
(mockPot, mockLedgerEntries, mockVirtualCard, mockStripe.issuing.cards.retrieve,
mockAutoPagingToArray) to return a capture and a non-refund credit-side row and
assert reconcileIntent('intent-3') produces the expected
report.stripe.totalCaptured and inSync values, referencing the reconcileIntent
function and mockAutoPagingToArray/mocked response shapes used in the current
test file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3c08ea8b-a3c7-4fa1-951a-e1a0f2f4a9f4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/payments/providers/stripe/reconciliationService.tstests/integration/stripe/reconciliation.test.tstests/unit/payments/reconciliationService.test.ts
|
Update (rebased onto latest
CI: Lint, typecheck, unit tests, integration tests, CodeQL, and CodeRabbit all passed on the latest run. Merge: I could not complete the merge from the CLI: base-branch policy still blocks merge from my account ( This addresses the critical teardown feedback from the review thread. |
- Type stripeCard and transactions explicitly as Stripe.Issuing.Card and Stripe.Issuing.Transaction[] so SDK-shape regressions surface at the let-declaration rather than far downstream. - Drop the redundant mockChildLogger.error.mockClear(); jest.clearAllMocks() on the line above already resets every mock's call/instance state.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/payments/providers/stripe/reconciliationService.ts (1)
84-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSubtract reversals from the captured net too.
This loop only treats
refundas money-out.reversaltransactions still fall through theelsebranch and inflatetotalCaptured, so intents with a returned/reversed Stripe transaction will still reconcile incorrectly.Proposed fix
let totalCaptured = 0; for (const t of transactions) { const abs = Math.abs(t.amount); - if (t.type === 'refund') { - totalCaptured -= abs; - } else { - totalCaptured += abs; - } + switch (t.type) { + case 'capture': + totalCaptured += abs; + break; + case 'refund': + case 'reversal': + totalCaptured -= abs; + break; + } }🤖 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 `@src/payments/providers/stripe/reconciliationService.ts` around lines 84 - 90, The loop computing totalCaptured incorrectly treats only t.type === 'refund' as money-out; update the conditional in the transactions loop that updates totalCaptured (the block referencing variables transactions, t.amount, and totalCaptured) to also treat 'reversal' as an outbound transaction (e.g., check t.type === 'refund' || t.type === 'reversal' or use a set of debit types) so you subtract Math.abs(t.amount) for both refunds and reversals rather than letting reversals fall into the else branch.tests/unit/payments/reconciliationService.test.ts (1)
77-94:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the reversal regression in this block too.
These cases only exercise
refund, so the suite still won't catch the other money-out path called out in issue#88. Areversalfixture here would pin the behavior the production loop currently misses.🤖 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 `@tests/unit/payments/reconciliationService.test.ts` around lines 77 - 94, The test only mocks a refund path; add a reversal transaction to the mockAutoPagingToArray fixture in the 'reconcileIntent — refunds (issue `#88` bug 3)' test so the reversal money-out path is exercised too: inside the mockAutoPagingToArray resolved array (used by reconcileIntent) include an entry like a reversal object (e.g. id 'itxn_rev', type 'reversal', appropriate amount) alongside the existing capture and refund entries so the test covers both refund and reversal handling while keeping the final expectation on report.stripe?.totalCaptured and report.inSync unchanged.
🤖 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.
Outside diff comments:
In `@src/payments/providers/stripe/reconciliationService.ts`:
- Around line 84-90: The loop computing totalCaptured incorrectly treats only
t.type === 'refund' as money-out; update the conditional in the transactions
loop that updates totalCaptured (the block referencing variables transactions,
t.amount, and totalCaptured) to also treat 'reversal' as an outbound transaction
(e.g., check t.type === 'refund' || t.type === 'reversal' or use a set of debit
types) so you subtract Math.abs(t.amount) for both refunds and reversals rather
than letting reversals fall into the else branch.
In `@tests/unit/payments/reconciliationService.test.ts`:
- Around line 77-94: The test only mocks a refund path; add a reversal
transaction to the mockAutoPagingToArray fixture in the 'reconcileIntent —
refunds (issue `#88` bug 3)' test so the reversal money-out path is exercised too:
inside the mockAutoPagingToArray resolved array (used by reconcileIntent)
include an entry like a reversal object (e.g. id 'itxn_rev', type 'reversal',
appropriate amount) alongside the existing capture and refund entries so the
test covers both refund and reversal handling while keeping the final
expectation on report.stripe?.totalCaptured and report.inSync unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1e2d390a-0dad-4306-b7fd-06421a8c49d7
📒 Files selected for processing (2)
src/payments/providers/stripe/reconciliationService.tstests/unit/payments/reconciliationService.test.ts
|
Addressed both review nits in d2bbc17 — ready for merge:
Local: Existing approval still stands on the prior commit; happy to re-request review if preferred. |
|
@JonasBaeumer this PR is ready from my side: approved, all checks green, and all review threads resolved. is blocked for me by the base-branch policy, so it needs a maintainer/admin merge when you have a minute. |
2 participants
Closes #88.
Summary
The reconciliation feature reported false discrepancies on every settled intent and had no error handling around its Stripe calls. All five bugs listed in the issue are addressed.
Bug 1 — Sign convention
Stripe Issuing returns capture amounts as negative integers (e.g. a €35 charge is
amount: -3500) and refunds as positive. The previous code summedt.amountdirectly and compared the resulting negative number to a positivesettledAmount, producing a discrepancy on every run.totalCapturedis now computed assum of |amount| for non-refunds minus |amount| for refunds, giving the comparable positive net.Bug 3 — Refunds excluded
The list query previously filtered with
type: 'capture', so a partial refund overstatedtotalCaptured. The filter is removed; refunds are now subtracted from the net (combined with the bug 1 fix above).Bug 5 — Pagination
stripe.issuing.transactions.listreturned at most 100 rows by default, silently understatingtotalCapturedfor high-volume intents. We now useautoPagingToArray({ limit: 1000 })and emit a discrepancy if the safety cap is hit so the truncation is visible rather than silent.Bug 4 — Stripe API error handling
cards.retrieveandtransactions.listare wrapped in try/catch. A 404 / 429 / network blip now produces a structured discrepancy report ({ inSync: false, stripe: null, discrepancies: ['stripe API error: ...'] }) and a logged error, instead of an unhandled exception that the webhook caller's outer.catchswallowed without trace.Bug 2 — Missing virtualCard
The previous behaviour returned
{ inSync: true }whenever noVirtualCardrow existed — even when the pot showed money had already moved (settled / returned). That's a false clean signal. The check now distinguishes:inSync: true(the in-flight case is the normal state betweenreserveForIntentcreating the pot andissueCardcreating the card)inSync: falsewith discrepancyvirtualCard missing for intent with pot status SETTLED (settledAmount 3500)Tests
Unit: 14 new tests in
tests/unit/payments/reconciliationService.test.tscover each bug independently: negative-amount handling, refund netting, both Stripe-error paths (cards.retrieveandtransactions.list), paging with the safety cap, truncation discrepancy, all four no-card states (no-pot, in-flight, SETTLED, RETURNED), plus the pre-existing card-status check.Integration (
tests/integration/stripe/reconciliation.test.ts): was previously broken at setup (missing requiredidempotencyKeyonPurchaseIntent, missinguserIdonPotandLedgerEntry, and atestHelpers.authorizations.create+.capturesequence that needs a running webhook approver to succeed). Switched totestHelpers.issuing.transactions.createForceCaptureso the test stays self-contained, supplied the required fields, and improved cleanup to delete child rows in FK order. Both integration tests now PASS for the first time against real Stripe test mode.This also removes the pre-existing reconciliation failures I'd been flagging in the PR descriptions of #147 and #148.
Test plan
Out of scope
reconcileIntent(intentId).then(...)inwebhookHandler.ts). With this PR the reconciliation no longer throws on Stripe failures — it surfaces them as audited discrepancies — so the webhook's outer.catchis now a true belt-and-braces.Summary by CodeRabbit
Bug Fixes
Tests