Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Connorbelez, your pull request is larger than the review limit of 150000 diff characters
Contributor
|
Caution Review failedPull request was closed or merged during review 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:
📝 WalkthroughWalkthroughImplements Phase 1 of the Transfer domain (ENG-184): adds canonical transfer types/validators and schema, a transfer XState machine and registry entry, provider interfaces + manual/provider adapter, transfer CRUD/initiation, transfer engine effects wired to cash-ledger posting helpers, collection-attempt → bridged transfer insertion, VoPay webhook handling with HMAC verification, reconciliation cron for orphaned confirmed transfers, and extensive tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTP as VoPay HTTP Action
participant Verif as verifyVoPaySignatureAction
participant DB as webhookEvents (db)
participant Proc as processVoPayWebhook
participant TransfersDB as transferRequests (db)
participant Ledger as CashLedger Integrations
participant Scheduler as Scheduler
Client->>HTTP: POST /webhooks/pad_vopay (body + X-VoPay-Signature)
HTTP->>Verif: verify signature (body, signature)
Verif-->>HTTP: ok / invalid
alt signature ok
HTTP->>DB: persistRawWebhookEvent(provider, eventId, rawBody)
DB-->>HTTP: webhookEventId
HTTP->>Proc: processVoPayWebhook(transactionId, status, ...)
Proc->>TransfersDB: find transfer by providerCode+providerRef
alt transfer found and maps to event -> FUNDS_SETTLED / REVERSAL / FAILED
Proc->>TransfersDB: executeTransition(...) (may invoke effects)
alt transition posts cash
TransfersDB->>Ledger: postCashReceiptForTransfer / postLenderPayoutForTransfer
Ledger-->>Proc: journal entry
end
else no transfer or unmapped status
Proc-->>Proc: skip
end
Proc->>DB: updateWebhookEventStatus(webhookEventId, "processed")
Proc-->>HTTP: { accepted: true }
else invalid signature
HTTP-->>Client: 401
end
HTTP-->>Client: 200 / accepted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
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. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Copilot reviewed 51 out of 51 changed files in this pull request and generated no comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 12
🧹 Nitpick comments (6)
convex/payments/transfers/providers/adapter.ts (1)
30-31: Empty string fallback for optional IDs may cause silent failures.Converting
undefinedto""viaString(request.references.mortgageId ?? "")produces empty strings that may not be handled correctly downstream. Consider preservingundefinedor throwing if required fields are missing.♻️ Proposed: Preserve undefined or validate required fields
const params: InitiateParams = { amount: request.amount, borrowerId: request.counterpartyId, - mortgageId: String(request.references.mortgageId ?? ""), - planEntryId: String(request.references.planEntryId ?? ""), + mortgageId: request.references.mortgageId + ? String(request.references.mortgageId) + : undefined, + planEntryId: request.references.planEntryId + ? String(request.references.planEntryId) + : undefined, method: request.providerCode, metadata: request.metadata, };This requires
InitiateParamsto acceptundefinedfor these fields, which may already be the case for optional references.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/transfers/providers/adapter.ts` around lines 30 - 31, The current code forces optional IDs to empty strings via String(request.references.mortgageId ?? "") and String(request.references.planEntryId ?? ""), which can mask missing values; update the adapter to preserve undefined by assigning mortgageId: request.references.mortgageId and planEntryId: request.references.planEntryId (or explicitly validate and throw if these are required), and adjust the InitiateParams typing if needed to accept undefined—ensure any downstream consumers handle undefined or that the adapter throws a clear error when a required ID is missing.convex/payments/transfers/validators.ts (1)
33-46: Consider composingtransferTypeValidatorfrom existing validators.The combined validator duplicates all literals from
inboundTransferTypeValidatorandoutboundTransferTypeValidator. This works but requires manual sync if types change.♻️ Optional: Compose validators to avoid duplication
If Convex's
v.unionsupported spreading validators, you could compose them. Since it doesn't, consider adding a comment noting the sync requirement:+// NOTE: Keep in sync with inboundTransferTypeValidator and outboundTransferTypeValidator export const transferTypeValidator = v.union( // Inbound v.literal("borrower_interest_collection"),Alternatively, if this pattern appears elsewhere, consider extracting the literal arrays to a shared constant and using them for both TypeScript types and validators.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/transfers/validators.ts` around lines 33 - 46, transferTypeValidator currently duplicates all literals from inboundTransferTypeValidator and outboundTransferTypeValidator; avoid drift by extracting the literal strings into a shared constant (e.g., TRANSFER_TYPE_LITERALS or separate INBOUND_TRANSFER_LITERALS and OUTBOUND_TRANSFER_LITERALS) and build inboundTransferTypeValidator, outboundTransferTypeValidator and transferTypeValidator from those arrays, or at minimum add a clear comment on transferTypeValidator that it must be kept in sync with inboundTransferTypeValidator and outboundTransferTypeValidator; reference the symbols transferTypeValidator, inboundTransferTypeValidator, and outboundTransferTypeValidator when making the change so you update every validator to use the shared source-of-truth.convex/payments/webhooks/__tests__/vopayWebhook.test.ts (1)
76-99: Tests validate local constants rather than production code.The
targetStateMapis defined locally in this test file and the tests assert against that same local definition. This validates nothing about the actual implementation invopay.ts. Consider either:
- Importing the real
targetStateMapfrom the production module (if exported), or- Removing these tests since they're self-referential
The same concern applies to the "payload construction per event type" tests (lines 103-173) — they construct payloads locally and assert on those local objects rather than testing actual payload construction logic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/webhooks/__tests__/vopayWebhook.test.ts` around lines 76 - 99, The tests are verifying a locally defined targetStateMap instead of the production value; update the tests to import targetStateMap from vopay.ts (export it if necessary) and assert against that imported symbol instead of the locally declared one, and likewise replace locally-constructed event payloads with calls to the actual payload-construction functions in vopay.ts (export those builders if they aren’t exported) so the tests exercise real implementation logic rather than self-referential constants.convex/payments/transfers/__tests__/reconciliation.test.ts (1)
108-122: Test asserts trivially true condition.This test iterates over non-
confirmedstatuses and asserts they are not equal to"confirmed", which is tautologically true. The intent (documenting that onlyconfirmedtransfers are checked) is valid, but the test provides no actual verification of cron behavior.Consider reframing as a constant assertion or removing in favor of a comment, since the real filtering logic is in the cron implementation:
- it("only confirmed transfers are checked (not pending, failed, etc.)", () => { - const statuses = [ - "initiated", - "pending", - "processing", - "failed", - "cancelled", - "reversed", - ]; - for (const status of statuses) { - // The cron queries only status: "confirmed" — these would not appear - expect(status).not.toBe("confirmed"); - } - }); + it("documents that only confirmed transfers are eligible for orphan check", () => { + // The cron queries only status: "confirmed" — other statuses are not scanned. + // This is validated by integration tests in the actual reconciliation module. + const CRON_TARGET_STATUS = "confirmed"; + expect(CRON_TARGET_STATUS).toBe("confirmed"); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/transfers/__tests__/reconciliation.test.ts` around lines 108 - 122, The test "only confirmed transfers are checked (not pending, failed, etc.)" currently asserts a tautology by iterating over the statuses array and checking each is not "confirmed"; replace this with a meaningful check or remove it: either turn the statuses array into a documented constant (e.g., NON_CONFIRMED_STATUSES) and assert its value exactly once, or better, call the actual cron/filter function (the reconciliation cron or the function that queries transfers) and assert it only returns transfers with status "confirmed"; locate the statuses array and the test case name in reconciliation.test.ts and update the test accordingly.convex/engine/machines/__tests__/transfer.machine.test.ts (1)
599-625: Sentinel test provides useful coverage documentation but may require maintenance.The arithmetic breakdown documents expected test counts per section, which is helpful for understanding coverage scope. However, this test will break if tests are added or removed without updating the counts. Consider whether this maintenance burden is acceptable, or if a comment-only documentation approach would suffice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/engine/machines/__tests__/transfer.machine.test.ts` around lines 599 - 625, The sentinel test hard-codes numeric breakdowns (variables expectedMetadata, expectedStateCells, expectedTerminalCells, expectedActionTests, expectedHappyPaths, expectedSentinel) and will fail when tests change; either remove the expect(expectedTotal).toBe(70) assertion and keep the block as a documentation-only comment, or make the assertion derive its counts programmatically from the actual state/event/action/happy-path definitions used in the suite (compute lengths from the state matrix, terminal list, actionTests collection, and happyPaths array and compare to their summed total) so the check remains correct when tests are added/removed.convex/payments/transfers/__tests__/mutations.test.ts (1)
246-264: Idempotency key tests are tautological.These tests construct the key inline and assert it equals itself. They document the expected format but don't actually test any production code. Consider importing the actual key-generation function from the bridge module and testing it directly.
♻️ Suggested improvement
+import { buildBridgeIdempotencyKey } from "../bridge"; // if such a helper exists + describe("idempotency key format", () => { it("bridge idempotency key follows transfer:bridge:{attemptId} format", () => { const attemptId = "attempt_123"; - const key = `transfer:bridge:${attemptId}`; - expect(key).toBe("transfer:bridge:attempt_123"); + const key = buildBridgeIdempotencyKey(attemptId); + expect(key).toBe("transfer:bridge:attempt_123"); expect(key).toMatch(BRIDGE_PREFIX_RE); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/transfers/__tests__/mutations.test.ts` around lines 246 - 264, Tests currently build idempotency keys inline instead of exercising the production key generator; import the actual key-generation function from the bridge module (e.g., generateBridgeIdempotencyKey or buildBridgeIdempotencyKey) and replace the inline template strings with calls to that function, then assert the returned value matches BRIDGE_PREFIX_RE, is deterministic for the same attemptId, and differs for different attemptIds; ensure you reference the imported function and BRIDGE_PREFIX_RE in the three existing test cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/engine/effects/transfer.ts`:
- Around line 188-196: The else branch in publishTransferReversed currently only
logs when no journal entry exists for a non-bridged transfer
(transfer.collectionAttemptId is falsy), allowing reversedAt to be set without a
cash reversal; change this to fail closed or enqueue a healing action: modify
publishTransferReversed so that when transfer.collectionAttemptId is falsy and
no journal entry is found (args.entityId), it throws an error or calls the
existing healing/retry enqueuer (e.g., the function used elsewhere for
retry/healing) instead of returning, ensuring the reversal does not complete
silently and triggers explicit remediation.
- Around line 73-103: publishTransferConfirmed currently overwrites any
provider-supplied settledAt with Date.now() after posting cash; instead, compute
a settledAt value = args.payload?.settledAt ?? Date.now(), persist it to the
transfer record before calling postCashReceiptForTransfer or
postLenderPayoutForTransfer (so those posting helpers can observe the provider
timestamp), and then proceed with the existing posting logic; update the
ctx.db.patch call in publishTransferConfirmed to write this settledAt value (and
if the posting helpers take a timestamp arg, pass the same settledAt through to
postCashReceiptForTransfer/postLenderPayoutForTransfer).
In `@convex/payments/cashLedger/integrations.ts`:
- Around line 1774-1777: The journal entry effective dates currently use
transfer.confirmedAt but the persisted timestamp is transfer.settledAt; update
the postCashEntryInternal calls (the ones passing effectiveDate:
unixMsToBusinessDate(...)) to use transfer.settledAt instead of
transfer.confirmedAt, keeping the same fallback to Date.now() (i.e.,
unixMsToBusinessDate(transfer.settledAt ?? Date.now())); apply this change to
the occurrence around the CASH_RECEIVED call shown and the other similar call
referenced (the later call at ~1848-1851).
- Around line 1759-1772: The code currently falls back to a mortgage-scoped
receivable when creditFamily === "BORROWER_RECEIVABLE" but transfer.obligationId
is missing; instead validate and fail when obligationId is absent: before
building creditAccountArgs (used to call getOrCreateCashAccount), check
creditFamily === "BORROWER_RECEIVABLE" and if transfer.obligationId is falsy,
throw/return an error explaining that obligationId is required for
receivable-backed transfers; otherwise proceed to construct creditAccountArgs
and call getOrCreateCashAccount as before.
In `@convex/payments/transfers/mutations.ts`:
- Around line 194-206: The immediate-confirm path after provider.initiate()
drops any returned providerRef, so subsequent webhook/reversal correlation
fails; to fix, extract the providerRef (or providerData containing it) from the
result of provider.initiate() and persist it before triggering the FUNDS_SETTLED
transition—e.g. call the existing internal mutation recordTransferProviderRef
(or equivalent logic) using args.transferId and the providerRef/providerData,
then proceed to
ctx.runMutation(internal.payments.transfers.mutations.fireInitiateTransition,
{...}) so the providerRef is saved for later webhook/reversal matching.
In `@convex/payments/transfers/providers/adapter.ts`:
- Around line 26-36: The initiate method unconditionally maps
request.counterpartyId to borrowerId which breaks for outbound transfers and
non-borrower counterparties; update the initiate(Request: TransferRequestInput)
logic in adapter.ts to validate request.direction and request.counterpartyType
and either (a) only allow/accept calls where direction indicates inbound and
counterpartyType === "borrower" (throw or return an error otherwise), or (b) map
the counterpartyId to the correct param based on counterpartyType (e.g., set
borrowerId only when counterpartyType === "borrower" and populate a lenderId or
appropriate field for lenders) before calling this.inner.initiate; also add or
update tests (adapter.test.ts) covering outbound transfers and non-borrower
counterparties to ensure the new guard or mapping works as intended.
In `@convex/payments/transfers/reconciliation.ts`:
- Around line 123-126: The current query retrieving confirmedTransfers uses
.withIndex("by_status", q => q.eq("status","confirmed")).take(100) which
truncates results before subsequent bridged/journaled/fresh filtering and will
repeatedly scan the same first window; change this to a paginated cursor loop
(or add a tighter index/filter) so you iterate through all confirmed rows:
implement pagination using the query cursor/skip/token API provided by ctx.db
(or use a composite index that includes the field you later filter on) and
process batches until no more results, updating where you set confirmedTransfers
and the similar queries in the block around lines 128-161 to use the same
paginated approach so every confirmed transfer is eventually visited.
In `@convex/payments/webhooks/vopay.ts`:
- Around line 112-135: The catch block currently ACKs ({ accepted: true }) even
when processing fails, dropping transient failures; change flow so the raw
webhook is durably persisted before calling
internal.payments.webhooks.vopay.processVoPayWebhook (e.g., add a prior
ctx.runMutation to save the raw payload/metadata) and only return
jsonResponse({accepted: true}) after successful persistence (and ideally
successful processing); if persistence or processing fails, return a 5xx
jsonResponse (do not return accepted: true) so the provider will retry. Ensure
you update the code paths around ctx.runMutation,
internal.payments.webhooks.vopay.processVoPayWebhook, and jsonResponse
accordingly so failed processing is retriable or the event is stored durably.
In `@specs/ENG-184/chunks/chunk-01-schema-types/context.md`:
- Around line 68-73: Define a new union type PersistedTransferStatus that
includes both current TransferStatus and LegacyTransferStatus so the type system
models all values accepted by transferStatusValidator; add it alongside the
existing declarations (near TransferStatus, LEGACY_TRANSFER_STATUSES,
LegacyTransferStatus) with a short comment noting it represents persisted DB
values during the ENG-190 migration, and plan to use PersistedTransferStatus for
query/return types where DB rows are read (instead of TransferStatus) until
migration completes.
In `@specs/ENG-184/chunks/chunk-05-mutations-bridge/context.md`:
- Around line 167-205: The code currently inserts directly into
"transferRequests" with status: "confirmed", bypassing the Transition Engine and
its effects (e.g., publishTransferConfirmed). Instead, create the transfer
record without forcing status to "confirmed" (use the normal initial state like
"pending"/"created" and omit transition timestamps such as
confirmedAt/settledAt/lastTransitionAt), keep fields like idempotencyKey and
collectionAttemptId, then invoke the Transition Engine to perform the governed
transition to "confirmed" so publishTransferConfirmed runs; update the code
around the insert into "transferRequests" and the helper
mapPlanEntryToTransferType usage to reflect this flow.
- Around line 48-63: The initiateTransfer flow is using adminMutation but must
be an action to call external provider.initiate; change the exported declaration
from adminMutation to adminAction (keep the same input schema), move the
provider.initiate call and any external I/O into the adminAction handler, and
ensure you schedule any deterministic DB updates (e.g. executeTransition calls
or status updates resolved from provider.initiate) as mutations invoked from the
action rather than performing DB mutations directly in the action handler;
reference symbols: initiateTransfer, adminMutation -> adminAction,
provider.initiate, getTransferProvider, and executeTransition.
---
Nitpick comments:
In `@convex/engine/machines/__tests__/transfer.machine.test.ts`:
- Around line 599-625: The sentinel test hard-codes numeric breakdowns
(variables expectedMetadata, expectedStateCells, expectedTerminalCells,
expectedActionTests, expectedHappyPaths, expectedSentinel) and will fail when
tests change; either remove the expect(expectedTotal).toBe(70) assertion and
keep the block as a documentation-only comment, or make the assertion derive its
counts programmatically from the actual state/event/action/happy-path
definitions used in the suite (compute lengths from the state matrix, terminal
list, actionTests collection, and happyPaths array and compare to their summed
total) so the check remains correct when tests are added/removed.
In `@convex/payments/transfers/__tests__/mutations.test.ts`:
- Around line 246-264: Tests currently build idempotency keys inline instead of
exercising the production key generator; import the actual key-generation
function from the bridge module (e.g., generateBridgeIdempotencyKey or
buildBridgeIdempotencyKey) and replace the inline template strings with calls to
that function, then assert the returned value matches BRIDGE_PREFIX_RE, is
deterministic for the same attemptId, and differs for different attemptIds;
ensure you reference the imported function and BRIDGE_PREFIX_RE in the three
existing test cases.
In `@convex/payments/transfers/__tests__/reconciliation.test.ts`:
- Around line 108-122: The test "only confirmed transfers are checked (not
pending, failed, etc.)" currently asserts a tautology by iterating over the
statuses array and checking each is not "confirmed"; replace this with a
meaningful check or remove it: either turn the statuses array into a documented
constant (e.g., NON_CONFIRMED_STATUSES) and assert its value exactly once, or
better, call the actual cron/filter function (the reconciliation cron or the
function that queries transfers) and assert it only returns transfers with
status "confirmed"; locate the statuses array and the test case name in
reconciliation.test.ts and update the test accordingly.
In `@convex/payments/transfers/providers/adapter.ts`:
- Around line 30-31: The current code forces optional IDs to empty strings via
String(request.references.mortgageId ?? "") and
String(request.references.planEntryId ?? ""), which can mask missing values;
update the adapter to preserve undefined by assigning mortgageId:
request.references.mortgageId and planEntryId: request.references.planEntryId
(or explicitly validate and throw if these are required), and adjust the
InitiateParams typing if needed to accept undefined—ensure any downstream
consumers handle undefined or that the adapter throws a clear error when a
required ID is missing.
In `@convex/payments/transfers/validators.ts`:
- Around line 33-46: transferTypeValidator currently duplicates all literals
from inboundTransferTypeValidator and outboundTransferTypeValidator; avoid drift
by extracting the literal strings into a shared constant (e.g.,
TRANSFER_TYPE_LITERALS or separate INBOUND_TRANSFER_LITERALS and
OUTBOUND_TRANSFER_LITERALS) and build inboundTransferTypeValidator,
outboundTransferTypeValidator and transferTypeValidator from those arrays, or at
minimum add a clear comment on transferTypeValidator that it must be kept in
sync with inboundTransferTypeValidator and outboundTransferTypeValidator;
reference the symbols transferTypeValidator, inboundTransferTypeValidator, and
outboundTransferTypeValidator when making the change so you update every
validator to use the shared source-of-truth.
In `@convex/payments/webhooks/__tests__/vopayWebhook.test.ts`:
- Around line 76-99: The tests are verifying a locally defined targetStateMap
instead of the production value; update the tests to import targetStateMap from
vopay.ts (export it if necessary) and assert against that imported symbol
instead of the locally declared one, and likewise replace locally-constructed
event payloads with calls to the actual payload-construction functions in
vopay.ts (export those builders if they aren’t exported) so the tests exercise
real implementation logic rather than self-referential constants.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: b407854c-f6cd-47fc-913d-bca7a1680550
📒 Files selected for processing (51)
convex/engine/effects/__tests__/transfer.test.tsconvex/engine/effects/collectionAttempt.tsconvex/engine/effects/registry.tsconvex/engine/effects/transfer.tsconvex/engine/machines/__tests__/transfer.machine.test.tsconvex/engine/machines/registry.tsconvex/engine/machines/transfer.machine.tsconvex/engine/types.tsconvex/engine/validators.tsconvex/http.tsconvex/payments/cashLedger/__tests__/disbursementGate.test.tsconvex/payments/cashLedger/__tests__/reversalCascade.test.tsconvex/payments/cashLedger/__tests__/testUtils.tsconvex/payments/cashLedger/__tests__/transferReconciliation.test.tsconvex/payments/cashLedger/integrations.tsconvex/payments/transfers/__tests__/bridge.test.tsconvex/payments/transfers/__tests__/mutations.test.tsconvex/payments/transfers/__tests__/reconciliation.test.tsconvex/payments/transfers/interface.tsconvex/payments/transfers/mutations.tsconvex/payments/transfers/providers/__tests__/adapter.test.tsconvex/payments/transfers/providers/adapter.tsconvex/payments/transfers/providers/manual.tsconvex/payments/transfers/providers/registry.tsconvex/payments/transfers/queries.tsconvex/payments/transfers/reconciliation.tsconvex/payments/transfers/types.tsconvex/payments/transfers/validators.tsconvex/payments/webhooks/__tests__/vopayWebhook.test.tsconvex/payments/webhooks/processReversal.tsconvex/payments/webhooks/types.tsconvex/payments/webhooks/verification.tsconvex/payments/webhooks/vopay.tsconvex/schema.tsdocs/cash-ledger-developer-guide.mdspecs/ENG-184/chunks/chunk-01-schema-types/context.mdspecs/ENG-184/chunks/chunk-01-schema-types/tasks.mdspecs/ENG-184/chunks/chunk-02-machine-registration/context.mdspecs/ENG-184/chunks/chunk-02-machine-registration/tasks.mdspecs/ENG-184/chunks/chunk-03-provider-interface/context.mdspecs/ENG-184/chunks/chunk-03-provider-interface/tasks.mdspecs/ENG-184/chunks/chunk-04-effects-ledger/context.mdspecs/ENG-184/chunks/chunk-04-effects-ledger/tasks.mdspecs/ENG-184/chunks/chunk-05-mutations-bridge/context.mdspecs/ENG-184/chunks/chunk-05-mutations-bridge/tasks.mdspecs/ENG-184/chunks/chunk-06-webhook-reconciliation/context.mdspecs/ENG-184/chunks/chunk-06-webhook-reconciliation/tasks.mdspecs/ENG-184/chunks/chunk-07-tests/context.mdspecs/ENG-184/chunks/chunk-07-tests/tasks.mdspecs/ENG-184/chunks/manifest.mdspecs/ENG-184/tasks.md
…tledAt for effective dates - Add early validation that throws ConvexError when creditFamily is BORROWER_RECEIVABLE but transfer.obligationId is missing, preventing silently posting to a generic mortgage-scoped receivable account - Replace transfer.confirmedAt with transfer.settledAt in both postCashReceiptForTransfer and postLenderPayoutForTransfer so journal entries are dated with the actual settlement time Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…reversal without journal entry - publishTransferConfirmed: compute settledAt from args.payload?.settledAt (falling back to Date.now()), and persist it BEFORE calling posting helpers so ledger entries use the provider's authoritative timestamp instead of replay time. - publishTransferReversed: throw an error instead of silently returning when a non-bridged transfer has no journal entry, preventing permanent ledger drift with no retry/healing signal. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Addresses CodeRabbit review: TransferStatus excludes legacy statuses
('approved', 'completed') accepted by transferStatusValidator. Adds a
PersistedTransferStatus union type so query return types can accurately
model all values that may exist in the database until ENG-190 migration
retires legacy rows.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…100 rows Replace .take(100) with cursor-based .paginate() loop so all confirmed transfers are eventually visited during reconciliation, not just the first 100 returned by the index. Follows existing pagination pattern from convex/engine/reconciliation.ts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The adapter maps counterpartyId to borrowerId unconditionally, which only makes sense for inbound borrower-initiated transfers. Add explicit guards that reject outbound direction and non-borrower counterpartyType with clear error messages, and add test coverage for all rejection paths. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ef on immediate-confirm path - Add optional lenderId and borrowerId parameters to createTransferRequest input schema and persist them when inserting into transferRequests. These are required downstream by postLenderPayoutForTransfer() and for borrower-level ledger scoping. - Add persistProviderRef internal mutation and call it from the immediate-confirm path in initiateTransfer before firing FUNDS_SETTLED. Previously, when provider.initiate() returned "confirmed", the providerRef was dropped because FUNDS_SETTLED bypasses the recordTransferProviderRef machine effect. This broke webhook/reversal correlation for immediate confirmations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ence failure Add a webhookEvents table for durable persistence of raw webhook payloads. The VoPay webhook handler now persists the raw event before acknowledging the provider. If persistence fails, it returns 5xx so VoPay can retry. If processing fails after persistence, it returns 200 (accepted) since the durable record ensures eventual processing via a recovery mechanism. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
convex/engine/effects/transfer.ts (1)
52-59: GuardproviderRefagainst mismatched rewrites.This overwrites any existing
transfer.providerRef. A duplicate or misrouted webhook can silently replace the canonical external reference and make later reconciliation ambiguous. Safer pattern: set when empty, no-op when equal, and throw on mismatch.♻️ Suggested guard
const providerRef = args.payload?.providerRef; if (typeof providerRef === "string") { - await ctx.db.patch(transfer._id, { providerRef }); + if (!transfer.providerRef) { + await ctx.db.patch(transfer._id, { providerRef }); + } else if (transfer.providerRef !== providerRef) { + throw new Error( + `[recordTransferProviderRef] Ref mismatch for transfer ${args.entityId}: ` + + `${transfer.providerRef} -> ${providerRef}` + ); + } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/engine/effects/transfer.ts` around lines 52 - 59, The current logic unconditionally patches transfer.providerRef which can overwrite an existing canonical value; change record handling so you only set providerRef when the stored transfer has no providerRef, do nothing if the incoming providerRef equals the existing transfer.providerRef, and throw an error (or return failure) if there is a mismatch between a non-empty existing transfer.providerRef and a different incoming providerRef; implement this by reading the existing transfer (using transfer._id), comparing transfer.providerRef to args.payload?.providerRef, and only calling ctx.db.patch(transfer._id, { providerRef }) when transfer.providerRef is empty, otherwise no-op if equal, and raise on mismatch (include args.entityId in the thrown error message).convex/payments/transfers/reconciliation.ts (1)
41-105: Prefer a workflow primitive for this retry loop.This is hand-rolling durable retry state, rescheduling, and escalation with
transferHealingAttempts+runAfter(0). Moving the orphan-healing loop toconvex-dev-workflowwould give you first-class retry/backoff state and cleaner operations here.As per coding guidelines, "Use convex-dev-workflow component for long-running code flows with built-in retries, delays, and state persistence."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/transfers/reconciliation.ts` around lines 41 - 105, The current orphaned-transfer retry logic in processOrphanedTransfer (which inserts/patches transferHealingAttempts, uses scheduleHealingEffect that calls ctx.scheduler.runAfter(0) to invoke internal.engine.effects.transfer.publishTransferConfirmed, and checks MAX_HEALING_ATTEMPTS) should be migrated to use the convex-dev-workflow primitive: replace the manual durable-state table and runAfter rescheduling with a workflow that encapsulates retry/backoff and escalation; create a workflow that accepts transferId, performs publishTransferConfirmed, retries with backoff up to MAX_HEALING_ATTEMPTS, and marks/escalates the transfer on final failure (or writes a single final transferHealingAttempts record if needed), then refactor processOrphanedTransfer to start/trigger that workflow instead of inserting/patching transferHealingAttempts or calling scheduleHealingEffect.convex/payments/transfers/types.ts (1)
9-35: Make the “canonical” arrays drive the Convex validators.
convex/payments/transfers/validators.ts:11-46andconvex/schema.ts:1422-1432still repeat these literals, so a future type/status addition can compile here and then fail at runtime when writes hit validation. Consider deriving those unions from this module, or at least add a parity test that keeps them locked together.Also applies to: 54-76
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/transfers/types.ts` around lines 9 - 35, The transfer type literals are repeated in runtime validators and schema generation, causing drift between the TypeScript unions and Convex validators; update the validators and schema generator to import and use the canonical arrays INBOUND_TRANSFER_TYPES, OUTBOUND_TRANSFER_TYPES or ALL_TRANSFER_TYPES (and the TransferType union) from this module instead of re-declaring literals, or add a parity test that programmatically compares the validator lists to these exported arrays so any change here fails CI; ensure references in the validator functions and schema generation logic are switched to these exported symbols (INBOUND_TRANSFER_TYPES, OUTBOUND_TRANSFER_TYPES, ALL_TRANSFER_TYPES, TransferType) to keep compile-time and runtime in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@convex/payments/transfers/reconciliation.ts`:
- Around line 142-164: When a matching cash_ledger_journal_entries row is found,
the current code simply continues and leaves any transferHealingAttempts in
"retrying", so update the transferHealingAttempts record to mark it resolved
before continuing: locate the block where journalEntry is retrieved (variable
journalEntry) and, if healing exists (query via transferHealingAttempts with
index by_transfer_request) and healing.status !== "resolved", write/update that
healing row to set status = "resolved" (and optionally set resolvedAt/notes) or
call an existing resolver helper, then continue; ensure you reference the
transfer._id and the transferHealingAttempts record when performing the update
so healed transfers no longer linger.
---
Nitpick comments:
In `@convex/engine/effects/transfer.ts`:
- Around line 52-59: The current logic unconditionally patches
transfer.providerRef which can overwrite an existing canonical value; change
record handling so you only set providerRef when the stored transfer has no
providerRef, do nothing if the incoming providerRef equals the existing
transfer.providerRef, and throw an error (or return failure) if there is a
mismatch between a non-empty existing transfer.providerRef and a different
incoming providerRef; implement this by reading the existing transfer (using
transfer._id), comparing transfer.providerRef to args.payload?.providerRef, and
only calling ctx.db.patch(transfer._id, { providerRef }) when
transfer.providerRef is empty, otherwise no-op if equal, and raise on mismatch
(include args.entityId in the thrown error message).
In `@convex/payments/transfers/reconciliation.ts`:
- Around line 41-105: The current orphaned-transfer retry logic in
processOrphanedTransfer (which inserts/patches transferHealingAttempts, uses
scheduleHealingEffect that calls ctx.scheduler.runAfter(0) to invoke
internal.engine.effects.transfer.publishTransferConfirmed, and checks
MAX_HEALING_ATTEMPTS) should be migrated to use the convex-dev-workflow
primitive: replace the manual durable-state table and runAfter rescheduling with
a workflow that encapsulates retry/backoff and escalation; create a workflow
that accepts transferId, performs publishTransferConfirmed, retries with backoff
up to MAX_HEALING_ATTEMPTS, and marks/escalates the transfer on final failure
(or writes a single final transferHealingAttempts record if needed), then
refactor processOrphanedTransfer to start/trigger that workflow instead of
inserting/patching transferHealingAttempts or calling scheduleHealingEffect.
In `@convex/payments/transfers/types.ts`:
- Around line 9-35: The transfer type literals are repeated in runtime
validators and schema generation, causing drift between the TypeScript unions
and Convex validators; update the validators and schema generator to import and
use the canonical arrays INBOUND_TRANSFER_TYPES, OUTBOUND_TRANSFER_TYPES or
ALL_TRANSFER_TYPES (and the TransferType union) from this module instead of
re-declaring literals, or add a parity test that programmatically compares the
validator lists to these exported arrays so any change here fails CI; ensure
references in the validator functions and schema generation logic are switched
to these exported symbols (INBOUND_TRANSFER_TYPES, OUTBOUND_TRANSFER_TYPES,
ALL_TRANSFER_TYPES, TransferType) to keep compile-time and runtime in sync.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 449f5732-0905-4696-80ba-8e264e7cfcba
📒 Files selected for processing (4)
convex/engine/effects/transfer.tsconvex/payments/cashLedger/integrations.tsconvex/payments/transfers/reconciliation.tsconvex/payments/transfers/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- convex/payments/cashLedger/integrations.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
convex/payments/webhooks/vopay.ts (1)
231-253: Usev.id("webhookEvents")for type-safe document reference.The
webhookEventIdargument is typed asv.string()and then cast toId<"webhookEvents">at line 238. Usingv.id("webhookEvents")would provide compile-time type safety and eliminate the cast.♻️ Proposed fix
export const updateWebhookEventStatus = internalMutation({ args: { - webhookEventId: v.string(), + webhookEventId: v.id("webhookEvents"), status: v.union(v.literal("processed"), v.literal("failed")), error: v.optional(v.string()), }, handler: async (ctx, args) => { - const doc = await ctx.db.get(args.webhookEventId as Id<"webhookEvents">); + const doc = await ctx.db.get(args.webhookEventId); if (!doc) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/webhooks/vopay.ts` around lines 231 - 253, The handler updateWebhookEventStatus currently declares webhookEventId as v.string() and then casts it to Id<"webhookEvents">; change the args schema to use v.id("webhookEvents") for webhookEventId so the type is enforced at compile time, remove the manual cast in the handler, and keep the rest of the logic (ctx.db.get(args.webhookEventId) and ctx.db.patch) unchanged so the code uses the correctly typed args.webhookEventId throughout.convex/payments/transfers/mutations.ts (1)
219-253: Consider propagating transition failure to the caller.
executeTransitionreturns aTransitionResultwith asuccessboolean rather than throwing on failure (per the context snippet fromconvex/engine/transition.ts). The current code returns the result directly without checkingsuccess, so callers won't know if the transition failed unless they inspect the return value.This is a minor observability concern for Phase 1 admin tooling — consider either:
- Checking
result.successand throwing aConvexErroron failure, or- Documenting that callers must inspect the returned
TransitionResult.🔧 Optional: Throw on transition failure
if (result.status === "confirmed") { await ctx.runMutation( internal.payments.transfers.mutations.persistProviderRef, { transferId: args.transferId, providerRef: result.providerRef, } ); - return ctx.runMutation( + const transitionResult = await ctx.runMutation( internal.payments.transfers.mutations.fireInitiateTransition, { transferId: args.transferId, eventType: "FUNDS_SETTLED", payload: { settledAt: Date.now(), providerData: {}, providerRef: result.providerRef, }, source, } ); + if (!transitionResult.success) { + throw new ConvexError(`Transition failed: ${transitionResult.reason ?? "unknown"}`); + } + return transitionResult; } - return ctx.runMutation( + const transitionResult = await ctx.runMutation( internal.payments.transfers.mutations.fireInitiateTransition, { transferId: args.transferId, eventType: "PROVIDER_INITIATED", payload: { providerRef: result.providerRef }, source, } ); + if (!transitionResult.success) { + throw new ConvexError(`Transition failed: ${transitionResult.reason ?? "unknown"}`); + } + return transitionResult;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/transfers/mutations.ts` around lines 219 - 253, The code currently returns the result of calling internal.payments.transfers.mutations.fireInitiateTransition (via ctx.runMutation) without checking the TransitionResult.success flag; update the handling in the confirmed branch (after persistProviderRef) and the non-confirmed branch where fireInitiateTransition is invoked to inspect the returned TransitionResult and throw a ConvexError (or otherwise raise) when success is false so failures propagate to the caller; specifically modify the callers that call fireInitiateTransition in this file to check result.success and throw if not successful.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@convex/payments/transfers/mutations.ts`:
- Around line 219-253: The code currently returns the result of calling
internal.payments.transfers.mutations.fireInitiateTransition (via
ctx.runMutation) without checking the TransitionResult.success flag; update the
handling in the confirmed branch (after persistProviderRef) and the
non-confirmed branch where fireInitiateTransition is invoked to inspect the
returned TransitionResult and throw a ConvexError (or otherwise raise) when
success is false so failures propagate to the caller; specifically modify the
callers that call fireInitiateTransition in this file to check result.success
and throw if not successful.
In `@convex/payments/webhooks/vopay.ts`:
- Around line 231-253: The handler updateWebhookEventStatus currently declares
webhookEventId as v.string() and then casts it to Id<"webhookEvents">; change
the args schema to use v.id("webhookEvents") for webhookEventId so the type is
enforced at compile time, remove the manual cast in the handler, and keep the
rest of the logic (ctx.db.get(args.webhookEventId) and ctx.db.patch) unchanged
so the code uses the correctly typed args.webhookEventId throughout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b8f79e8-1afc-4c59-9133-07fe783b0009
📒 Files selected for processing (5)
convex/payments/transfers/mutations.tsconvex/payments/transfers/providers/__tests__/adapter.test.tsconvex/payments/transfers/providers/adapter.tsconvex/payments/webhooks/vopay.tsconvex/schema.ts
✅ Files skipped from review due to trivial changes (1)
- convex/payments/transfers/providers/tests/adapter.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- convex/payments/transfers/providers/adapter.ts
When reconciliation finds a journal entry for a previously-orphaned transfer, mark any open transferHealingAttempts row as "resolved" with a resolvedAt timestamp instead of silently skipping it. Also extracts per-transfer logic into reconcileTransfer() helper to keep handler complexity within Biome's threshold. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
convex/payments/transfers/reconciliation.ts (1)
184-199: Add lightweight reconciliation metrics for runtime visibility.Consider emitting counters for scanned / retried / resolved / escalated transfers so cron health and drift are easy to monitor in production.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/transfers/reconciliation.ts` around lines 184 - 199, The reconciliation loop lacks runtime metrics; add lightweight counters for scanned/retried/resolved/escalated transfers so cron health is observable. Increment a "scanned" counter inside the loop that pages over transferRequests (where RECONCILIATION_PAGE_SIZE, cursor and page are used), and modify reconcileTransfer to return a result/status (e.g., "retried" | "resolved" | "escalated") or throw distinct errors; then increment the corresponding counters (retried/resolved/escalated) in the caller after awaiting reconcileTransfer. Use the existing ctx (e.g., ctx.metrics.increment or a similar metrics helper) so metrics are emitted without changing control flow; ensure counters are namespaced and include retries/resolution reason where available.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@convex/payments/transfers/reconciliation.ts`:
- Around line 184-199: The reconciliation loop lacks runtime metrics; add
lightweight counters for scanned/retried/resolved/escalated transfers so cron
health is observable. Increment a "scanned" counter inside the loop that pages
over transferRequests (where RECONCILIATION_PAGE_SIZE, cursor and page are
used), and modify reconcileTransfer to return a result/status (e.g., "retried" |
"resolved" | "escalated") or throw distinct errors; then increment the
corresponding counters (retried/resolved/escalated) in the caller after awaiting
reconcileTransfer. Use the existing ctx (e.g., ctx.metrics.increment or a
similar metrics helper) so metrics are emitted without changing control flow;
ensure counters are namespaced and include retries/resolution reason where
available.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 84ed4aeb-95e6-4cfb-8230-f6de831b3129
📒 Files selected for processing (2)
convex/payments/transfers/reconciliation.tsconvex/schema.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- convex/schema.ts
This was referenced Mar 27, 2026
Merged
Merged
This was referenced Apr 10, 2026
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
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.
eng-175 (#283)
TL;DR
Added webhook infrastructure for handling payment reversals from Rotessa and Stripe, including signature verification, shared reversal processing logic, and comprehensive test coverage.
What changed?
Added webhook endpoints
/webhooks/rotessaand/webhooks/stripeto handle payment reversal events from both providers. The implementation includes:The system processes NSF, PAD returns, disputes, and manual reversals by transitioning collection attempts from "confirmed" to "reversed" state and posting cash ledger reversal entries.
How to test?
Run the test suite with
npm testto verify:For manual testing, configure
ROTESSA_WEBHOOK_SECRETandSTRIPE_WEBHOOK_SECRETenvironment variables and send POST requests to the webhook endpoints with properly signed payloads.Why make this change?
This enables automated handling of payment reversals from external providers, ensuring the system stays in sync when payments fail or are disputed. The webhook handlers provide real-time processing of NSF events, PAD returns, and disputes, automatically updating collection attempt states and posting appropriate cash ledger entries to maintain accurate financial records.
Summary by CodeRabbit
New Features
Infrastructure
Tests & Docs
eng-184