Conversation
There was a problem hiding this comment.
Sorry @Connorbelez, your pull request is larger than the review limit of 150000 diff characters
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
📝 WalkthroughWalkthroughImplemented a disbursement bridge system that converts pending dispersal entries past their hold periods into outbound transfer requests of type Changes
Sequence Diagram(s)sequenceDiagram
actor Admin
participant Bridge as Disbursement Bridge
participant DB as dispersalEntries DB
participant Transfers as transferRequests
participant Effects as Transfer Effects
Admin->>Bridge: triggerDisbursementBridge(asOfDate, batchSize)
Bridge->>DB: findEligibleEntriesInternal(asOfDate)
DB-->>Bridge: pending entries past payoutEligibleAfter
loop for each eligible entry
Bridge->>Bridge: processSingleDisbursement(dispersalEntryId)
Bridge->>DB: read dispersal entry
DB-->>Bridge: entry data, validate status=pending & amount>0
Bridge->>Transfers: insert transferRequest (lender_dispersal_payout)
Transfers-->>Bridge: transfer created with idempotency key
end
Bridge-->>Admin: DisbursementBridgeResult (counts: created/skipped/failed)
Note over Effects: Later...
Effects->>DB: publishTransferConfirmed with settledAt
DB->>DB: patch dispersalEntry status=disbursed, payoutDate=settledAt
DB-->>Effects: ✓ Lifecycle synchronized
sequenceDiagram
participant DB as dispersalEntries DB
participant Cron as Daily Cron
participant Check as checkDisbursementsDue
participant Log as Admin Logs
Cron->>Check: trigger at 09:00 UTC (daily)
Check->>DB: query pending entries where payoutEligibleAfter <= today
DB-->>Check: eligible entries grouped by lenderId
alt entries due
Check->>Log: emit warning: "N entries due across M lenders with amounts"
Log-->>Check: ✓ Admin visibility
else no entries
Check->>Check: no-op
end
Check-->>Cron: ✓ Complete
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 |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
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.
Actionable comments posted: 3
🧹 Nitpick comments (5)
convex/payments/cashLedger/integrations.ts (1)
485-489: Add one regression for the newsettledAmountbranch.This now validates against
args.settledAmountwhen it is supplied, but the existing allocation tests shown inconvex/payments/cashLedger/__tests__/postingGroupIntegration.test.tsandconvex/payments/cashLedger/__tests__/e2eLifecycle.test.tsstill only exercise the fallbackobligation.amountpath. A case wheresettledAmount < obligation.amountwould lock the intended accounting contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/cashLedger/integrations.ts` around lines 485 - 489, Add a regression test that exercises the new settledAmount branch by supplying args.settledAmount less than obligation.amount so grossAllocation uses settledAmount; update or add a test in the existing postingGroupIntegration.test.ts (or e2eLifecycle.test.ts) to construct the same call-path that triggers validatePostingGroupAmounts (the code path in integrations.ts that computes grossAllocation and calls validatePostingGroupAmounts) and assert the expected outcome (e.g., the allocation validation succeeds/fails as intended and the accounting contract is locked for the correct amount). Ensure the test creates an obligation with amount > settledAmount, populates args.entries and args.servicingFee accordingly, invokes the integration code that leads to validatePostingGroupAmounts, and asserts the resulting contract lock/allocations reflect settledAmount rather than obligation.amount.convex/dispersal/disbursementBridge.ts (2)
283-287: Timestamp-based retry key may cause issues in rapid retries.Using
Date.now()in the idempotency key works but could theoretically produce duplicates if retries happen within the same millisecond. Consider using a counter or UUID suffix instead.However, this is low-risk since:
- Admin-triggered retries won't be sub-millisecond.
- Convex OCC would cause a retry anyway if collision occurred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/dispersal/disbursementBridge.ts` around lines 283 - 287, The idempotency key generation using Date.now() in effectiveIdempotencyKey can collide on rapid retries; update the fallback suffix to use a more robust unique value (e.g., a UUID or a per-request retry counter) instead of Date.now(). Locate the logic that computes effectiveIdempotencyKey (references: effectiveIdempotencyKey, existing.status, idempotencyKey) and replace the `:retry:${Date.now()}` suffix with a UUID (or incrementing retry counter tied to the request) so retries always produce a unique idempotency key without relying on millisecond timing.
528-572: Duplicated eligibility logic betweencheckDisbursementsDueandfindEligibleEntriesInternal.Lines 534-553 duplicate the two-bucket query logic from
findEligibleEntriesInternal(lines 105-133). Consider extracting this into a shared helper or havingcheckDisbursementsDuecallfindEligibleEntriesInternalvia the internal API.However, since
checkDisbursementsDueis aninternalMutationand cannot callinternalQuerydirectly, the duplication is currently necessary. A future refactor could extract the logic into a shared function that both can call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/dispersal/disbursementBridge.ts` around lines 528 - 572, The eligibility selection logic is duplicated between checkDisbursementsDue and findEligibleEntriesInternal; extract the two-bucket query + filtering into a shared helper (e.g., getEligibleDispersalEntries(ctx) or extractEligibleEntriesInternal) that accepts the Convex ctx/db and returns the combined eligible array, then update both checkDisbursementsDue and findEligibleEntriesInternal to call that helper instead of reimplementing the pendingPastHold/eligibleWithHold and pendingAll/eligibleLegacy logic; ensure the helper uses the same indices/filters (status === "pending", payoutEligibleAfter lte today) and preserves lenderId/amount fields so the existing byLender aggregation in checkDisbursementsDue continues to work.convex/payments/transfers/mutations.ts (2)
340-441:createTransferRequestInternalduplicates public mutation logic.This mutation mirrors
createTransferRequest(lines 69-176) with nearly identical validation and insertion logic. Consider extracting shared validation/insertion into a helper function to reduce duplication and ensure consistency.♻️ Suggested refactor
Extract common validation and insertion logic:
// Shared helper for transfer creation async function validateAndInsertTransfer( ctx: MutationCtx, args: TransferRequestArgs, source: CommandSource ): Promise<Id<"transferRequests">> { // Amount validation if (!Number.isInteger(args.amount) || args.amount <= 0) { throw new ConvexError("Amount must be a positive integer (cents)"); } // ... rest of shared logic } // Then in both mutations: export const createTransferRequest = paymentMutation .handler(async (ctx, args) => { const source = buildSource(ctx.viewer, "admin_dashboard"); return validateAndInsertTransfer(ctx, args, source); }); export const createTransferRequestInternal = internalMutation({ handler: async (ctx, args) => { return validateAndInsertTransfer(ctx, args, PIPELINE_SOURCE); }, });🤖 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 340 - 441, createTransferRequestInternal duplicates the public createTransferRequest logic; extract the shared validation and DB insertion into a single helper (e.g., validateAndInsertTransfer) and have both createTransferRequest and createTransferRequestInternal call it with the appropriate source. Move amount validation, validatePipelineFields, toDomainEntityId handling, mock provider check (areMockTransferProvidersEnabled), idempotency lookup (query("transferRequests").withIndex("by_idempotency")...), and the ctx.db.insert payload into the helper; accept ctx, the mutation args (e.g., TransferRequestArgs), and a source/CommandSource parameter so createTransferRequest passes buildSource(ctx.viewer, "admin_dashboard") and createTransferRequestInternal passes PIPELINE_SOURCE. Ensure helper returns Id<"transferRequests"> and preserve error handling for InvalidDomainEntityIdError and ConvexError.
596-605: Status check before pipeline creation has a potential race window.Between checking
deal.status !== "fundsTransfer.pending"and creating the pipeline, the deal could transition to a different state via a concurrentFUNDS_RECEIVEDevent. However, since the deal machine's OCC andexecuteTransitionprovide idempotency (rejecting duplicate transitions), and the pipeline checks for existing transfers at lines 608-625, this is a low-risk window.🤖 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 596 - 605, The current pre-check using ctx.runQuery(internal.deals.queries.getInternalDeal) before creating the transfer pipeline has a race where the deal may change (e.g., via FUNDS_RECEIVED) before pipeline creation; fix by making the check-and-create atomic: perform the deal status check and pipeline creation inside a single transactional operation (use the transaction helper or ctx.runMutation/ctx.withTransaction) or alternatively call the idempotent executeTransition for the deal first and only create the pipeline if that transition succeeds; reference getInternalDeal, executeTransition, and the transfer pipeline creation code so the check and pipeline creation happen in one transaction or are gated by executeTransition's result.
🤖 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/types.ts`:
- Around line 18-19: The new entity variant "dispersalEntry" was added to the
type union but reconciliation logic doesn't handle it; update the reconciliation
code to include explicit handling for dispersalEntry in both lookupStatus (in
convex/engine/reconciliationAction.ts) and getEntityStatus (in
convex/engine/reconciliation.ts): add a branch/case for "dispersalEntry" that
mirrors the reconciliation semantics used by similar entities (e.g., how
"lenderRenewalIntent" or other entry types are processed), ensuring
dispersalEntry records return the correct status/path and do not fall through to
the default/unhandled case.
In `@specs/ENG-206/eng-206-disbursement-bridge-plan.html`:
- Around line 569-571: The buttons call global handlers toggleTheme(),
mermaidZoom(), and toggleExpand() but the script only defines underscored
versions (_toggleTheme, _mermaidZoom, _toggleExpand), causing runtime errors;
fix by exposing the underscored implementations as the expected global names
(e.g., assign window.toggleTheme = _toggleTheme, window.mermaidZoom =
_mermaidZoom, window.toggleExpand = _toggleExpand) or rename the functions to
the non-underscored names so the markup and script match; update the same
pattern for any other occurrences mentioned (lines using
toggleTheme/mermaidZoom/toggleExpand).
- Around line 703-719: Wrap all raw '>' characters inside the Mermaid source
blocks (e.g., the div with class="mermaid" id="sm") as HTML entities (>) so
the spec-char-escape rule no longer fails; edit the Mermaid text lines
containing arrows like --> and -->|...| to replace each '>' with '>' (do this
in both diagram blocks mentioned, including the block around lines 740-762) and
keep the rest of the diagram text unchanged.
---
Nitpick comments:
In `@convex/dispersal/disbursementBridge.ts`:
- Around line 283-287: The idempotency key generation using Date.now() in
effectiveIdempotencyKey can collide on rapid retries; update the fallback suffix
to use a more robust unique value (e.g., a UUID or a per-request retry counter)
instead of Date.now(). Locate the logic that computes effectiveIdempotencyKey
(references: effectiveIdempotencyKey, existing.status, idempotencyKey) and
replace the `:retry:${Date.now()}` suffix with a UUID (or incrementing retry
counter tied to the request) so retries always produce a unique idempotency key
without relying on millisecond timing.
- Around line 528-572: The eligibility selection logic is duplicated between
checkDisbursementsDue and findEligibleEntriesInternal; extract the two-bucket
query + filtering into a shared helper (e.g., getEligibleDispersalEntries(ctx)
or extractEligibleEntriesInternal) that accepts the Convex ctx/db and returns
the combined eligible array, then update both checkDisbursementsDue and
findEligibleEntriesInternal to call that helper instead of reimplementing the
pendingPastHold/eligibleWithHold and pendingAll/eligibleLegacy logic; ensure the
helper uses the same indices/filters (status === "pending", payoutEligibleAfter
lte today) and preserves lenderId/amount fields so the existing byLender
aggregation in checkDisbursementsDue continues to work.
In `@convex/payments/cashLedger/integrations.ts`:
- Around line 485-489: Add a regression test that exercises the new
settledAmount branch by supplying args.settledAmount less than obligation.amount
so grossAllocation uses settledAmount; update or add a test in the existing
postingGroupIntegration.test.ts (or e2eLifecycle.test.ts) to construct the same
call-path that triggers validatePostingGroupAmounts (the code path in
integrations.ts that computes grossAllocation and calls
validatePostingGroupAmounts) and assert the expected outcome (e.g., the
allocation validation succeeds/fails as intended and the accounting contract is
locked for the correct amount). Ensure the test creates an obligation with
amount > settledAmount, populates args.entries and args.servicingFee
accordingly, invokes the integration code that leads to
validatePostingGroupAmounts, and asserts the resulting contract lock/allocations
reflect settledAmount rather than obligation.amount.
In `@convex/payments/transfers/mutations.ts`:
- Around line 340-441: createTransferRequestInternal duplicates the public
createTransferRequest logic; extract the shared validation and DB insertion into
a single helper (e.g., validateAndInsertTransfer) and have both
createTransferRequest and createTransferRequestInternal call it with the
appropriate source. Move amount validation, validatePipelineFields,
toDomainEntityId handling, mock provider check
(areMockTransferProvidersEnabled), idempotency lookup
(query("transferRequests").withIndex("by_idempotency")...), and the
ctx.db.insert payload into the helper; accept ctx, the mutation args (e.g.,
TransferRequestArgs), and a source/CommandSource parameter so
createTransferRequest passes buildSource(ctx.viewer, "admin_dashboard") and
createTransferRequestInternal passes PIPELINE_SOURCE. Ensure helper returns
Id<"transferRequests"> and preserve error handling for
InvalidDomainEntityIdError and ConvexError.
- Around line 596-605: The current pre-check using
ctx.runQuery(internal.deals.queries.getInternalDeal) before creating the
transfer pipeline has a race where the deal may change (e.g., via
FUNDS_RECEIVED) before pipeline creation; fix by making the check-and-create
atomic: perform the deal status check and pipeline creation inside a single
transactional operation (use the transaction helper or
ctx.runMutation/ctx.withTransaction) or alternatively call the idempotent
executeTransition for the deal first and only create the pipeline if that
transition succeeds; reference getInternalDeal, executeTransition, and the
transfer pipeline creation code so the check and pipeline creation happen in one
transaction or are gated by executeTransition's result.
🪄 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: 1bafd919-9a4f-4dfe-976d-ff14ed3b6723
⛔ Files ignored due to path filters (1)
convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (30)
convex/crons.tsconvex/dispersal/__tests__/createDispersalEntries.test.tsconvex/dispersal/__tests__/disbursementBridge.test.tsconvex/dispersal/createDispersalEntries.tsconvex/dispersal/disbursementBridge.tsconvex/dispersal/servicingFee.tsconvex/engine/effects/__tests__/transfer.test.tsconvex/engine/effects/transfer.tsconvex/engine/types.tsconvex/engine/validators.tsconvex/payments/cashLedger/integrations.tsconvex/payments/transfers/__tests__/pipeline.test.tsconvex/payments/transfers/mutations.tsconvex/payments/transfers/pipeline.tsconvex/payments/transfers/pipeline.types.tsconvex/payments/transfers/queries.tsspecs/ENG-206/chunks/chunk-01-bridge-core/context.mdspecs/ENG-206/chunks/chunk-01-bridge-core/tasks.mdspecs/ENG-206/chunks/chunk-02-effects-cron/context.mdspecs/ENG-206/chunks/chunk-02-effects-cron/tasks.mdspecs/ENG-206/chunks/chunk-03-tests/context.mdspecs/ENG-206/chunks/chunk-03-tests/tasks.mdspecs/ENG-206/chunks/manifest.mdspecs/ENG-206/eng-206-disbursement-bridge-plan.htmlspecs/ENG-206/tasks.mdspecs/ENG-217/chunks/chunk-01-code-docs-test/context.mdspecs/ENG-217/chunks/chunk-01-code-docs-test/status.mdspecs/ENG-217/chunks/chunk-01-code-docs-test/tasks.mdspecs/ENG-217/chunks/manifest.mdspecs/ENG-217/tasks.md
7b0fcb9 to
39fcfd7
Compare
…and getEntityStatus Prevents dispersalEntry from falling through to the unhandled default error branch in both reconciliation functions by explicitly returning undefined (non-governed entity, skipped by reconciliation). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…L spec Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
# ENG-206: Disbursement Bridge Implementation
This pull request implements the disbursement bridge that converts eligible dispersal entries into outbound lender payout transfers, completing the lender disbursement flow.
## Core Features
- **Bridge Module**: New `disbursementBridge.ts` with batch processing, per-entry validation, and idempotency
- **Transfer Effects Integration**: Dispersal entry status lifecycle tied to transfer confirmation/failure
- **Daily Alert Cron**: Monitors pending entries past hold period for admin visibility
- **Comprehensive Testing**: Unit and integration tests covering happy path, edge cases, and failure scenarios
## Key Components
### Bridge Functions
- `triggerDisbursementBridge`: Batch orchestrator finding eligible entries and creating transfers
- `processSingleDisbursement`: Per-entry processing with disbursement gate validation
- `findEligibleEntriesInternal`: Query for pending entries past hold period
- `resetFailedEntry`: Recovery mechanism for failed disbursements
- `checkDisbursementsDue`: Daily cron alert handler
### Transfer Lifecycle Integration
- `publishTransferConfirmed`: Updates dispersal entry to "disbursed" with payout date
- `publishTransferFailed`: Marks dispersal entry as "failed" for retry
- `publishTransferReversed`: Handles disbursement reversals
### Pipeline Support
- Multi-leg deal closing pipeline with Leg 1 (buyer → trust) and Leg 2 (trust → seller)
- Pipeline status computation from transfer leg statuses
- Automatic Leg 2 creation when Leg 1 confirms
## Technical Details
- **Idempotency**: `disbursement:{dispersalEntryId}` keys prevent duplicate transfers
- **Amount Preservation**: Uses `entry.amount` as-is per ENG-219 (no recomputation)
- **Balance Gate**: Validates disbursements against LENDER_PAYABLE balance
- **Provider Support**: Phase 1 uses `mock_eft`, ready for VoPay integration
- **Error Handling**: Graceful failure with detailed error reporting
## Testing Coverage
- Unit tests for helper functions and idempotency
- Integration tests for full disbursement lifecycle
- Edge case coverage including concurrent runs, insufficient balance, and transfer failures
- ENG-219 guard test ensuring amount passthrough
## Cron Schedule Update
Added daily disbursement check at 09:00 UTC (after payout batch at 08:00) for admin alerts.
## ENG-217: Servicing Fee Model Documentation
Enhanced documentation for the servicing fee calculation model, confirming current implementation decisions.
## Key Clarifications
- **Fee Basis**: Current outstanding principal (`mortgage.principal`) at settlement time
- **Fee Timing**: Pre-disbursement deduction during dispersal entry creation
- **Principal Sensitivity**: Fees decrease proportionally as principal is repaid
## Documentation Updates
- Added comprehensive JSDoc to `calculateServicingFee` explaining principal basis
- Inline comments in `calculateServicingSplit` documenting ENG-217 decisions
- New test case verifying fee decreases when mortgage principal decreases
## Technical Implementation
- Fee calculated using current outstanding balance, not original loan amount
- `principalBalance` stored in `servicingFeeEntries` for audit verification
- Standard amortizing mortgage behavior where servicing fees decrease with principal paydown
This addresses Tech Design §10 Open Decision 2 and Foot Gun 7, confirming the current implementation follows standard mortgage servicing practices.
<!-- This is an auto-generated comment: release notes by coderabbit.ai -->
## Summary by CodeRabbit
* **New Features**
* Disbursement bridge processes dispersal entries into outbound lender payouts with idempotency and retry support
* Deal closing pipeline orchestrates multi-leg transfer flows for principal transfers and seller payouts
* Daily cron monitors disbursement readiness and reports due entries by lender
* **Bug Fixes**
* Enhanced servicing fee calculation with principal sensitivity testing
* **Chores**
* Expanded transfer effects to manage dispersal entry lifecycle (status tracking and audit logging)
* Added comprehensive disbursement and transfer pipeline test coverage
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

ENG-206: Disbursement Bridge Implementation
This pull request implements the disbursement bridge that converts eligible dispersal entries into outbound lender payout transfers, completing the lender disbursement flow.
Core Features
disbursementBridge.tswith batch processing, per-entry validation, and idempotencyKey Components
Bridge Functions
triggerDisbursementBridge: Batch orchestrator finding eligible entries and creating transfersprocessSingleDisbursement: Per-entry processing with disbursement gate validationfindEligibleEntriesInternal: Query for pending entries past hold periodresetFailedEntry: Recovery mechanism for failed disbursementscheckDisbursementsDue: Daily cron alert handlerTransfer Lifecycle Integration
publishTransferConfirmed: Updates dispersal entry to "disbursed" with payout datepublishTransferFailed: Marks dispersal entry as "failed" for retrypublishTransferReversed: Handles disbursement reversalsPipeline Support
Technical Details
disbursement:{dispersalEntryId}keys prevent duplicate transfersentry.amountas-is per ENG-219 (no recomputation)mock_eft, ready for VoPay integrationTesting Coverage
Cron Schedule Update
Added daily disbursement check at 09:00 UTC (after payout batch at 08:00) for admin alerts.
ENG-217: Servicing Fee Model Documentation
Enhanced documentation for the servicing fee calculation model, confirming current implementation decisions.
Key Clarifications
mortgage.principal) at settlement timeDocumentation Updates
calculateServicingFeeexplaining principal basiscalculateServicingSplitdocumenting ENG-217 decisionsTechnical Implementation
principalBalancestored inservicingFeeEntriesfor audit verificationThis addresses Tech Design §10 Open Decision 2 and Foot Gun 7, confirming the current implementation follows standard mortgage servicing practices.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests