Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughImplements ENG-172 payment reversal cascade feature for cash ledger. Adds two idempotent reversal mutations ( Changes
Sequence DiagramsequenceDiagram
participant Client
participant Mutation as postPaymentReversalCascade
participant Database as Journal DB
participant Validation as Validators
participant Cascade as Reversal Logic
Client->>Mutation: Call with attemptId/transferRequestId + identifiers
Mutation->>Database: Load CASH_RECEIVED entry
Database-->>Mutation: Original entry
Mutation->>Validation: Validate dimension match + amount bounds
Validation-->>Mutation: Valid
Mutation->>Database: Check if posting group exists (idempotency)
alt Posting Group Exists
Database-->>Mutation: Return existing reversals
else New Reversal Cascade
Mutation->>Cascade: Post REVERSAL for CASH_RECEIVED
Cascade->>Database: Insert REVERSAL entry
Database-->>Cascade: Entry ID
Mutation->>Database: Load allocation entries (LENDER_PAYABLE_CREATED, SERVICING_FEE_RECOGNIZED)
Database-->>Mutation: Allocation entries
Mutation->>Cascade: Post REVERSAL for each allocation
Cascade->>Database: Insert allocation REVERSAL entries
Database-->>Cascade: Entry IDs
Mutation->>Database: Find LENDER_PAYOUT_SENT per lender (clawback detection)
Database-->>Mutation: Payout entries (if any)
alt Payouts Found
Mutation->>Cascade: Post REVERSAL entries for LENDER_PAYOUT_SENT
Cascade->>Database: Insert payout REVERSAL entries
Database-->>Cascade: Entry IDs
Cascade-->>Mutation: clawbackRequired=true
else No Payouts
Mutation-->>Cascade: clawbackRequired=false
end
Mutation->>Database: Return all reversal entries
end
Mutation-->>Client: {reversalEntries[], postingGroupId, clawbackRequired}
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Rationale: The reversal cascade logic in Possibly Related Issues
Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Connorbelez, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Connorbelez has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
Pull request overview
Implements payment reversal support in the cash-ledger by introducing a REVERSAL cascade (with posting-group semantics), adding a reconciliation query to detect reversed settled obligations, and adding unit/integration tests plus accompanying ENG-172 spec artifacts.
Changes:
- Added
postPaymentReversalCascade()(multi-leg settlement reversal) andpostTransferReversal()plus shared amount validation inintegrations.ts. - Added
findSettledObligationsWithNonZeroBalance()and aninternalQuerywrapper to surface reversal indicators from reconciliation. - Added new test suites covering reversal cascade, reconciliation detection, and an end-to-end pipeline.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/ENG-172/tasks.md | Master task list for ENG-172 implementation and verification steps. |
| specs/ENG-172/chunks/manifest.md | Chunk breakdown and execution order for ENG-172 work. |
| specs/ENG-172/chunks/chunk-01-core-reversal-cascade/tasks.md | Task checklist for adding reversal cascade + transfer reversal. |
| specs/ENG-172/chunks/chunk-01-core-reversal-cascade/context.md | Detailed design/context for reversal posting sequence and constraints. |
| specs/ENG-172/chunks/chunk-02-reconciliation-detection/tasks.md | Task checklist for adding reversal-indicator reconciliation query. |
| specs/ENG-172/chunks/chunk-02-reconciliation-detection/context.md | Detailed design/context for the reversal-indicator query and internal wrapper. |
| specs/ENG-172/chunks/chunk-03-unit-tests-cascade/tasks.md | Task checklist for cascade unit tests. |
| specs/ENG-172/chunks/chunk-03-unit-tests-cascade/context.md | Guidance for building cascade tests and expected accounting behavior. |
| specs/ENG-172/chunks/chunk-04-unit-tests-reconciliation/tasks.md | Task checklist for reconciliation unit tests. |
| specs/ENG-172/chunks/chunk-04-unit-tests-reconciliation/context.md | Guidance for building reconciliation detection tests. |
| specs/ENG-172/chunks/chunk-05-integration-test/tasks.md | Task checklist for end-to-end integration test. |
| specs/ENG-172/chunks/chunk-05-integration-test/context.md | Guidance for end-to-end pipeline test setup and assertions. |
| specs/ENG-172/chunks/chunk-06-quality-gate/tasks.md | Checklist for lint/typecheck/codegen/test gate. |
| specs/ENG-172/chunks/chunk-06-quality-gate/context.md | Commands and common issue patterns for the quality gate. |
| convex/payments/cashLedger/reconciliation.ts | Adds reversal-indicator reconciliation query and internal wrapper. |
| convex/payments/cashLedger/integrations.ts | Adds reversal amount validation helper, reversal cascade orchestrator, and transfer reversal helper. |
| convex/payments/cashLedger/tests/reversalReconciliation.test.ts | New unit tests for reversal-indicator reconciliation behavior. |
| convex/payments/cashLedger/tests/reversalIntegration.test.ts | New end-to-end test exercising full settlement pipeline + reversal + detection. |
| convex/payments/cashLedger/tests/reversalCascade.test.ts | New unit tests for reversal cascade, clawback handling, idempotency, and transfer reversal. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Owner
Author
|
@coderabbitai please review |
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
…rror code Renumbered sequentially 1→13 throughout postPaymentReversalCascade Added auditLog.log() call at step 13 with severity “warning”, including postingGroupId, clawbackRequired, entryCount. Registered auditLogTest in test harnesses. Added doc comment noting Phase 1 acceptability + Phase 2 pagination Added mismatch check with TRANSFER_REQUEST_MISMATCH error code
…tion test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ro reversal balance) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…k logic - Comment 1: Add dispersalEntryId and obligationId optional args to postLenderPayout so payout entries carry stable settlement linkage. Extract tiered payout detection into findPayoutEntryForClawback helper with 4-tier search strategy (dispersalEntryId, lender-scoped dispersalEntryId, postingGroupId, lenderId+mortgageId fallback). - Comment 2: postTransferReversal now requires original.transferRequestId to be present (NOT_A_TRANSFER_ENTRY error) before checking equality, preventing transfer-scoped reversals for non-transfer entries. - Comment 3: Add assertDimensionMatch helper that validates args.mortgageId and args.obligationId match the original CASH_RECEIVED entry dimensions before posting reversals. Throws DIMENSION_MISMATCH on inconsistency. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Owner
Author
|
@coderabbitai where's that review? |
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
![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: 4
🧹 Nitpick comments (3)
specs/ENG-172/chunks/chunk-02-reconciliation-detection/tasks.md (1)
1-4: Tasks appear complete but unchecked.Both
findSettledObligationsWithNonZeroBalance()andfindSettledObligationsWithNonZeroBalanceInternalare implemented inreconciliation.ts. Consider marking these tasks as complete ([x]) to reflect the current state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/ENG-172/chunks/chunk-02-reconciliation-detection/tasks.md` around lines 1 - 4, The checklist in specs/ENG-172/chunks/chunk-02-reconciliation-detection/tasks.md is stale: both findSettledObligationsWithNonZeroBalance() and findSettledObligationsWithNonZeroBalanceInternal are already implemented in reconciliation.ts, so update the markdown to mark T-004 and T-005 as completed by changing their checkboxes from [ ] to [x]; ensure the two task lines referencing those exact function names are updated.specs/ENG-172/chunks/chunk-01-core-reversal-cascade/tasks.md (1)
1-5: Tasks appear complete but unchecked.Per the PR summary,
assertReversalAmountValid(),postPaymentReversalCascade(), andpostTransferReversal()are implemented inintegrations.ts. Consider marking these tasks as complete ([x]) to reflect the current state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/ENG-172/chunks/chunk-01-core-reversal-cascade/tasks.md` around lines 1 - 5, The checklist in tasks.md still shows T-001, T-002, and T-003 as incomplete; update the markdown to mark these tasks as done by replacing the unchecked boxes with checked ones for assertReversalAmountValid(), postPaymentReversalCascade(), and postTransferReversal so the file reflects that these functions in integrations.ts are implemented.convex/payments/cashLedger/reconciliation.ts (1)
270-306: Potential N+1 query pattern — acceptable for Phase 1, but worth noting for Phase 2.The function iterates over all settled obligations and calls
getJournalSettledAmountForObligationfor each, which itself queries the journal. This is an N+1 pattern that could become expensive as obligation count grows.The doc comment already acknowledges pagination for Phase 2. Consider also batching the journal lookups or pre-fetching all relevant entries in a single query when optimizing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/cashLedger/reconciliation.ts` around lines 270 - 306, The current implementation of findSettledObligationsWithNonZeroBalance performs an N+1 query by calling getJournalSettledAmountForObligation for each obligation; to fix, change the workflow to fetch journal data in bulk (e.g., query the journal once for all obligation IDs returned by ctx.db.query("obligations") and aggregate settled amounts per obligation into a Map), then iterate the settledObligations and compute outstandingBalance using the precomputed journalSettledAmount from that Map; alternatively add a batch API like getJournalSettledAmountsForObligations(obligationIds: string[]) and use it in findSettledObligationsWithNonZeroBalance to avoid per-obligation journal queries.
🤖 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/cashLedger/__tests__/reversalCascade.test.ts`:
- Around line 262-290: The tests exercising postTransferReversal are seeding
state.cashReceivedEntry with only attemptId, but postTransferReversal now
rejects originals missing transferRequestId; update the tests (around use of
postCashEntryInternal / state.cashReceivedEntry) to create a CASH_RECEIVED entry
that includes the same transferRequestId used by each reversal test (or add a
reusable fixture helper that calls postCashEntryInternal with transferRequestId
and returns that created entry), so T-010 and T-013 use a transfer-backed
original instead of hitting NOT_A_TRANSFER_ENTRY.
In `@convex/payments/cashLedger/integrations.ts`:
- Around line 1354-1364: The postPaymentReversalCascade logic currently prefers
attemptId when both identifiers are provided; change this to either (a) require
exactly one identifier by throwing a ConvexError if both attemptId and
transferRequestId are present, or (b) if you must accept both, locate the
CASH_RECEIVED row (using postingGroupId logic) and verify its transferRequestId
matches the provided transferRequestId and its attemptId matches the provided
attemptId, throwing a ConvexError on mismatch; update the postingGroupId
resolution and the duplicate-check around postPaymentReversalCascade (also
adjust the analogous check at the second occurrence around the 1393–1399 block)
to enforce this validation so stale/wrong transferRequestId cannot be silently
ignored.
- Around line 1315-1323: The current fallback returns the first
LENDER_PAYOUT_SENT for a lender+mortgage which can be ambiguous; instead collect
all lenderEntries matching entryType === "LENDER_PAYOUT_SENT" && mortgageId ===
mortgageId, and if exactly one match return it, otherwise fail closed by
returning null (or throwing) so ambiguity is surfaced for manual handling;
reference the query to "cash_ledger_journal_entries"
withIndex("by_lender_and_sequence") and the lenderEntries variable—do not ignore
obligationId/dispersalEntryId here, but refuse to auto-select when multiple
matches exist.
In `@specs/ENG-172/chunks/chunk-01-core-reversal-cascade/context.md`:
- Around line 36-54: Update the spec to reflect the implemented reversal
contract: modify the assertReversalAmountValid() description to state it throws
INVALID_REVERSAL_AMOUNT for non-positive or unsafe integer reversalAmount (not
just over-limit), and update references to how clawbacks are resolved to use
findPayoutEntryForClawback() with the by_lender_and_sequence and
by_posting_group lookups; remove or replace any mention of
by_lender_and_entry_type and ensure examples and any pointers (e.g., the
paragraph previously pointing to line 166) reference the correct function names
and index names.
---
Nitpick comments:
In `@convex/payments/cashLedger/reconciliation.ts`:
- Around line 270-306: The current implementation of
findSettledObligationsWithNonZeroBalance performs an N+1 query by calling
getJournalSettledAmountForObligation for each obligation; to fix, change the
workflow to fetch journal data in bulk (e.g., query the journal once for all
obligation IDs returned by ctx.db.query("obligations") and aggregate settled
amounts per obligation into a Map), then iterate the settledObligations and
compute outstandingBalance using the precomputed journalSettledAmount from that
Map; alternatively add a batch API like
getJournalSettledAmountsForObligations(obligationIds: string[]) and use it in
findSettledObligationsWithNonZeroBalance to avoid per-obligation journal
queries.
In `@specs/ENG-172/chunks/chunk-01-core-reversal-cascade/tasks.md`:
- Around line 1-5: The checklist in tasks.md still shows T-001, T-002, and T-003
as incomplete; update the markdown to mark these tasks as done by replacing the
unchecked boxes with checked ones for assertReversalAmountValid(),
postPaymentReversalCascade(), and postTransferReversal so the file reflects that
these functions in integrations.ts are implemented.
In `@specs/ENG-172/chunks/chunk-02-reconciliation-detection/tasks.md`:
- Around line 1-4: The checklist in
specs/ENG-172/chunks/chunk-02-reconciliation-detection/tasks.md is stale: both
findSettledObligationsWithNonZeroBalance() and
findSettledObligationsWithNonZeroBalanceInternal are already implemented in
reconciliation.ts, so update the markdown to mark T-004 and T-005 as completed
by changing their checkboxes from [ ] to [x]; ensure the two task lines
referencing those exact function names are updated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a348a82c-b083-418b-a6d9-3dd038e213c3
📒 Files selected for processing (20)
convex/payments/cashLedger/__tests__/reversalCascade.test.tsconvex/payments/cashLedger/__tests__/reversalIntegration.test.tsconvex/payments/cashLedger/__tests__/reversalReconciliation.test.tsconvex/payments/cashLedger/integrations.tsconvex/payments/cashLedger/mutations.tsconvex/payments/cashLedger/reconciliation.tsspecs/ENG-172/chunks/chunk-01-core-reversal-cascade/context.mdspecs/ENG-172/chunks/chunk-01-core-reversal-cascade/tasks.mdspecs/ENG-172/chunks/chunk-02-reconciliation-detection/context.mdspecs/ENG-172/chunks/chunk-02-reconciliation-detection/tasks.mdspecs/ENG-172/chunks/chunk-03-unit-tests-cascade/context.mdspecs/ENG-172/chunks/chunk-03-unit-tests-cascade/tasks.mdspecs/ENG-172/chunks/chunk-04-unit-tests-reconciliation/context.mdspecs/ENG-172/chunks/chunk-04-unit-tests-reconciliation/tasks.mdspecs/ENG-172/chunks/chunk-05-integration-test/context.mdspecs/ENG-172/chunks/chunk-05-integration-test/tasks.mdspecs/ENG-172/chunks/chunk-06-quality-gate/context.mdspecs/ENG-172/chunks/chunk-06-quality-gate/tasks.mdspecs/ENG-172/chunks/manifest.mdspecs/ENG-172/tasks.md
- assertReversalAmountValid() now documents both INVALID_REVERSAL_AMOUNT (non-positive/unsafe integer) and REVERSAL_EXCEEDS_ORIGINAL checks - Replaced stale by_lender_and_entry_type reference with the actual findPayoutEntryForClawback() tiered lookup using by_lender_and_sequence and by_posting_group indexes - Updated step 11 in the cascade logic to reference the tiered strategy Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…al tests T-010 and T-013 used state.cashReceivedEntry which lacks transferRequestId, causing postTransferReversal to reject with NOT_A_TRANSFER_ENTRY before exercising the intended assertions. Each test now creates a dedicated transfer-backed CASH_RECEIVED entry with the matching transferRequestId. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…reversal identifiers - Tier 4 fallback in findPayoutEntryForClawback now returns undefined when multiple LENDER_PAYOUT_SENT entries match by lenderId+mortgageId, preventing wrong clawback attachment on ambiguous payout matching. - postPaymentReversalCascade now rejects with AMBIGUOUS_REVERSAL_IDENTIFIER when both attemptId and transferRequestId are supplied, preventing silent mismatch on conflicting identifier combinations. - Extracted assertExactlyOneReversalIdentifier helper to reduce cognitive complexity below the linter threshold. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Connorbelez
added a commit
that referenced
this pull request
Apr 20, 2026
# ENG-172: REVERSAL Entry Type with Posting Group Semantics This pull request implements payment reversal functionality for the cash ledger system, enabling the reversal of settled payments through a cascade mechanism that maintains posting group integrity. ## Core Reversal Functions Added `assertReversalAmountValid()` helper with Number.isSafeInteger + > 0 guard and INVALID_REVERSAL_AMOUNT error code for amount validation. Implemented `postPaymentReversalCascade()` - a multi-leg reversal orchestrator that reverses entire settlement posting groups atomically. The function handles: - CASH_RECEIVED entry reversal (step 1) - LENDER_PAYABLE_CREATED entry reversals (step 2) - SERVICING_FEE_RECOGNIZED entry reversal (step 3) - Conditional LENDER_PAYOUT_SENT reversals for clawback scenarios (step 4) All reversal entries share a single postingGroupId for atomicity and include proper causedBy linkage to original entries. Added `postTransferReversal()` for single-entry transfer reversals with TRANSFER_REQUEST_MISMATCH error code validation. ## Reconciliation Detection Implemented `findSettledObligationsWithNonZeroBalance()` to detect settled obligations where journal-derived receivable balance is non-zero (the reversal indicator). Added doc comment noting Phase 1 acceptability with Phase 2 pagination recommendations. Added `findSettledObligationsWithNonZeroBalanceInternal` internalQuery wrapper for external consumption. ## Audit Trail & Compliance Added auditLog.log() call at step 13 with severity "warning", including postingGroupId, clawbackRequired, and entryCount metadata for O.Reg 189/08 compliance. ## Test Coverage Created comprehensive test suites covering: - Full reversal cascade scenarios (T-006 through T-013) - Reconciliation detection logic (T-014 through T-016) - End-to-end integration flows (T-017 through T-020) Registered auditLogTest in test harnesses and verified zero regressions in existing cash ledger tests. ## Quality Assurance All code passes lint, type checking, and codegen validation. The implementation maintains append-only journal integrity while providing idempotent reversal operations with proper posting group semantics. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added payment reversal cascade functionality for handling settlement reversals with automatic payout clawback detection * Added transfer-scoped reversal capability for single-entry reversals * Added reconciliation detection to identify reversed obligations with outstanding balances * Enabled optional dispersal entry and obligation tracking for payout operations * **Tests** * Added comprehensive reversal cascade behavior tests covering clawback scenarios, idempotency, and amount validation * Added integration tests validating end-to-end reversal flows with account balance reconciliation * Added reversal reconciliation detection tests <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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-172: REVERSAL Entry Type with Posting Group Semantics
This pull request implements payment reversal functionality for the cash ledger system, enabling the reversal of settled payments through a cascade mechanism that maintains posting group integrity.
Core Reversal Functions
Added
assertReversalAmountValid()helper with Number.isSafeInteger + > 0 guard and INVALID_REVERSAL_AMOUNT error code for amount validation.Implemented
postPaymentReversalCascade()- a multi-leg reversal orchestrator that reverses entire settlement posting groups atomically. The function handles:All reversal entries share a single postingGroupId for atomicity and include proper causedBy linkage to original entries.
Added
postTransferReversal()for single-entry transfer reversals with TRANSFER_REQUEST_MISMATCH error code validation.Reconciliation Detection
Implemented
findSettledObligationsWithNonZeroBalance()to detect settled obligations where journal-derived receivable balance is non-zero (the reversal indicator). Added doc comment noting Phase 1 acceptability with Phase 2 pagination recommendations.Added
findSettledObligationsWithNonZeroBalanceInternalinternalQuery wrapper for external consumption.Audit Trail & Compliance
Added auditLog.log() call at step 13 with severity "warning", including postingGroupId, clawbackRequired, and entryCount metadata for O.Reg 189/08 compliance.
Test Coverage
Created comprehensive test suites covering:
Registered auditLogTest in test harnesses and verified zero regressions in existing cash ledger tests.
Quality Assurance
All code passes lint, type checking, and codegen validation. The implementation maintains append-only journal integrity while providing idempotent reversal operations with proper posting group semantics.
Summary by CodeRabbit
New Features
Tests