Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThreads optional lenderId through the deal-closing pipeline and adds an investor principal-return feature: pure proration/idempotency utilities, an internal orchestrator to create/initiate lender_principal_return transfers, an admin mutation to request returns, schema and validator updates, and accompanying tests and specs. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin User
participant Mutation as returnInvestorPrincipal<br/>(Public Action)
participant Orchestrator as createPrincipalReturn<br/>(Internal Action)
participant TransferMut as Transfer Mutations
participant DB as Database
Admin->>Mutation: submit dealId, sellerId, lenderId, mortgageId, principalAmount, prorationAdjustment
Mutation->>DB: load deal (validate status === "confirmed")
alt deal not confirmed
Mutation-->>Admin: throw ConvexError
else deal confirmed
Mutation->>DB: query by idempotency key
alt existing active transfer
Mutation-->>Admin: { transferId, alreadyExists: true }
else proceed
Mutation->>Orchestrator: delegate creation
Orchestrator->>Orchestrator: compute proration-adjusted amount\nbuild idempotency key
Orchestrator->>TransferMut: createTransferRequestInternal (outbound, lender_principal_return, lenderId, amount, source: principal_return)
TransferMut->>DB: insert transfer request
TransferMut-->>Orchestrator: { transferId }
Orchestrator->>TransferMut: initiateTransferInternal (transferId)
TransferMut->>DB: update transfer status / processing
TransferMut-->>Orchestrator: success / error
Orchestrator-->>Mutation: { transferId }
Mutation-->>Admin: { transferId, alreadyExists: false }
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 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 |
![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
Owner
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![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.
![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: 1
🧹 Nitpick comments (1)
convex/payments/transfers/__tests__/principalReturn.test.ts (1)
350-373: Consider replacing trivial assertions with actual behavior tests.These tests only verify string inequality (
status !== "confirmed") rather than testingreturnInvestorPrincipal's actual rejection behavior. While the file header notes these are scoped to data-layer validation logic, the tests could be more valuable by:
- Documenting the expected error message pattern, or
- Using a comment to clarify this is intentionally documenting the validation rule rather than exercising it
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/transfers/__tests__/principalReturn.test.ts` around lines 350 - 373, The current test merely asserts string inequality instead of exercising the actual validation; update the "T-011" tests in principalReturn.test.ts to call the real validation path (call returnInvestorPrincipal or the data-layer validator used by returnInvestorPrincipal) with mock deals having each non-confirmed status and assert that it throws the expected ConvexError (or matches the expected error message/pattern); for the positive case, call returnInvestorPrincipal with a mock deal.status === "confirmed" and assert it does not throw (or returns the expected result). If invoking the function is not feasible in this scope, replace the trivial expect checks with a clear comment above the test explaining that these are only documenting the validation rule rather than executing it.
🤖 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/__tests__/principalReturn.test.ts`:
- Around line 482-530: The test expects cancelled transfers to allow
re-creation, but returnInvestorPrincipal currently treats any status other than
"initiated" as terminal and throws; update the logic in returnInvestorPrincipal
(mutations.ts) where it checks existing.status to also treat "cancelled" as
non-terminal/allowed for re-creation (e.g., include "cancelled" alongside
"initiated" in the allowed-status check or refactor to an allowedStatuses set)
so the function returns/continues instead of throwing for cancelled transfers.
---
Nitpick comments:
In `@convex/payments/transfers/__tests__/principalReturn.test.ts`:
- Around line 350-373: The current test merely asserts string inequality instead
of exercising the actual validation; update the "T-011" tests in
principalReturn.test.ts to call the real validation path (call
returnInvestorPrincipal or the data-layer validator used by
returnInvestorPrincipal) with mock deals having each non-confirmed status and
assert that it throws the expected ConvexError (or matches the expected error
message/pattern); for the positive case, call returnInvestorPrincipal with a
mock deal.status === "confirmed" and assert it does not throw (or returns the
expected result). If invoking the function is not feasible in this scope,
replace the trivial expect checks with a clear comment above the test explaining
that these are only documenting the validation rule rather than executing it.
🪄 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: 434e654b-40c2-4040-a0ae-93495fc5229f
📒 Files selected for processing (17)
convex/engine/effects/transfer.tsconvex/payments/transfers/__tests__/pipeline.test.tsconvex/payments/transfers/__tests__/principalReturn.logic.test.tsconvex/payments/transfers/__tests__/principalReturn.test.tsconvex/payments/transfers/mutations.tsconvex/payments/transfers/pipeline.tsconvex/payments/transfers/pipeline.types.tsconvex/payments/transfers/principalReturn.logic.tsconvex/payments/transfers/principalReturn.tsconvex/payments/transfers/queries.tsconvex/schema.tsspecs/ENG-210/chunks/chunk-01-pipeline-fix-and-orchestrator/context.mdspecs/ENG-210/chunks/chunk-01-pipeline-fix-and-orchestrator/tasks.mdspecs/ENG-210/chunks/chunk-02-tests/context.mdspecs/ENG-210/chunks/chunk-02-tests/tasks.mdspecs/ENG-210/chunks/manifest.mdspecs/ENG-210/tasks.md
💤 Files with no reviewable changes (1)
- convex/payments/transfers/queries.ts
There was a problem hiding this comment.
Pull request overview
Fixes a deal-closing pipeline data gap by threading lenderId through Leg 1 metadata into Leg 2 transfer creation, and introduces an admin-triggered “investor principal return” flow (pure logic + internal action orchestrator + public admin action) with accompanying tests/docs.
Changes:
- Add
deals.lenderIdto the schema and passlenderIdthrough deal closing pipeline Leg 1 metadata → Leg 2 transfer creation. - Implement principal return logic (
computeProrationAdjustedAmount, idempotency key) and orchestration (createPrincipalReturn) plus admin entrypoint (returnInvestorPrincipal). - Add/update tests for pipeline metadata extraction and principal return logic/flow, plus ENG-210 spec task docs.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/ENG-210/tasks.md | ENG-210 task checklist for pipeline fix + principal return. |
| specs/ENG-210/chunks/manifest.md | Chunk manifest for ENG-210 execution plan. |
| specs/ENG-210/chunks/chunk-01-pipeline-fix-and-orchestrator/tasks.md | Chunk 01 tasks list (pipeline fix + module/orchestrator/action). |
| specs/ENG-210/chunks/chunk-01-pipeline-fix-and-orchestrator/context.md | Implementation context and intended design details for Chunk 01. |
| specs/ENG-210/chunks/chunk-02-tests/tasks.md | Chunk 02 tasks list (tests). |
| specs/ENG-210/chunks/chunk-02-tests/context.md | Test plan/context for unit + integration tests. |
| convex/schema.ts | Adds lenderId to deals table. |
| convex/payments/transfers/queries.ts | Removes stray whitespace in pipeline query response. |
| convex/payments/transfers/pipeline.types.ts | Extends Leg 1 metadata to include (optional) lenderId and validates on extraction. |
| convex/payments/transfers/pipeline.ts | Threads lenderId through pipeline creation and Leg 2 transfer creation. |
| convex/engine/effects/transfer.ts | Schedules Leg 2 creation with lenderId from Leg 1 metadata. |
| convex/payments/transfers/principalReturn.logic.ts | Adds pure proration + idempotency key helpers for principal return. |
| convex/payments/transfers/principalReturn.ts | Adds internalAction orchestrator to create+initiate a principal return transfer. |
| convex/payments/transfers/mutations.ts | Passes deal.lenderId into pipeline start and adds returnInvestorPrincipal admin action. |
| convex/payments/transfers/tests/principalReturn.logic.test.ts | Unit tests for principal return pure logic. |
| convex/payments/transfers/tests/principalReturn.test.ts | Adds principal return tests (currently mostly direct DB inserts / simulated logic). |
| convex/payments/transfers/tests/pipeline.test.ts | Updates metadata extraction tests to include lenderId cases. |
Comments suppressed due to low confidence (2)
convex/engine/effects/transfer.ts:258
- handlePipelineLegConfirmed forwards lenderId from Leg 1 metadata via a type cast. If the pipeline was started before lenderId was added to metadata (or if metadata is missing/invalid), this will schedule Leg 2 with lenderId undefined and cash ledger posting will later throw (transfer.lenderId required for LENDER_PAYOUT_SENT). Consider resolving lenderId more defensively here: if leg1Meta.lenderId is missing, load the deal (transfer.dealId) and use deal.lenderId; if still absent, throw before scheduling Leg 2.
await ctx.scheduler.runAfter(
0,
internal.payments.transfers.pipeline.createAndInitiateLeg2,
{
pipelineId: transfer.pipelineId,
dealId: transfer.dealId,
sellerId: leg1Meta.sellerId,
lenderId: leg1Meta.lenderId as Id<"lenders"> | undefined,
mortgageId: transfer.mortgageId,
leg2Amount: leg1Meta.leg2Amount,
providerCode: assertProviderCode(transfer.providerCode),
}
convex/payments/transfers/mutations.ts:730
- startDealClosingPipeline now forwards deal.lenderId into the pipeline, but deal.lenderId is optional and if it’s missing the Leg 2 transfer will still be created without lenderId and will later throw in cash ledger posting (postLenderPayoutForTransfer requires transfer.lenderId). Since this action only allows deals in "fundsTransfer.pending", consider failing fast here (or deriving lenderId deterministically) when deal.lenderId is null/undefined so pipelines can’t be started in a state that will inevitably fail on Leg 2 confirmation.
pipelineId,
buyerId: deal.buyerId,
sellerId: deal.sellerId,
lenderId: deal.lenderId,
mortgageId: deal.mortgageId,
leg1Amount: args.leg1Amount,
leg2Amount,
providerCode,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…Leg 2 transfers Add ConvexError guard at the top of the handler that throws immediately if lenderId is absent, before the deal_seller_payout transfer record is created. Without this guard, a Leg 2 transfer could be persisted but then fail during cash ledger posting because lenderId is undefined. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nvestorPrincipal Validates that args.mortgageId, args.sellerId, and args.lenderId belong to the loaded deal before proceeding, preventing mismatched IDs from corrupting cash ledger posting. Also updates the terminal-status error message to accurately reflect that re-creation is impossible due to the deterministic idempotency key. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…or auditability Added PRINCIPAL_RETURN_SOURCE constant (channel: "principal_return", actorType: "system") to principalReturn.ts and wired it through createTransferRequestInternal via a new optional source parameter. Also extended CommandChannel type and channelValidator to include the new channel. This ensures principal return transfers are attributed to the correct source in the audit trail and ops tooling, distinguishing them from scheduler-driven pipeline transfers. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…on tests - Fix A: Update the cancelled-status idempotency test to expect shouldThrow=true, matching the actual returnInvestorPrincipal implementation which treats any status !== "initiated" (after confirmed/pending/processing/failed checks) as terminal and throws a ConvexError. - Fix B: Add 4 integration tests that invoke real Convex functions via convex-test: - returnInvestorPrincipal (paymentAction) happy path and idempotency via t.withIdentity().action() - createPrincipalReturn (internalAction) happy path and idempotency via t.action(internal.xxx) These assert on the actual transfer record type, status, amount, and idempotency key. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Connorbelez
added a commit
that referenced
this pull request
Apr 20, 2026
### TL;DR Fixed critical `lenderId` missing from deal closing pipeline Leg 2 transfers and implemented investor principal return functionality with admin action and comprehensive test coverage. ### What changed? **Pipeline Fix:** - Added `lenderId` field to `DealClosingLeg1Metadata` interface and threaded it through the entire pipeline chain - Updated `extractLeg1Metadata` to validate and extract `lenderId` from metadata - Modified pipeline functions to pass `lenderId` from deal → Leg 1 metadata → Leg 2 transfer creation - Added `lenderId` field to deals schema for storing resolved lender on approval **Principal Return Implementation:** - Created `principalReturn.logic.ts` with pure functions for proration calculations and idempotency key generation - Built `principalReturn.ts` orchestrator with `createPrincipalReturn` internal action - Added `returnInvestorPrincipal` admin action with deal status validation and idempotency checks - Implemented comprehensive test suites covering both unit tests for pure logic and integration tests for the full flow ### How to test? **Pipeline Fix:** 1. Create a deal with `lenderId` set 2. Start deal closing pipeline via `startDealClosingPipeline` 3. Verify Leg 2 transfer includes `lenderId` field when created 4. Run existing pipeline tests to ensure no regressions **Principal Return:** 1. Run unit tests: `bun run test principalReturn.logic.test.ts` 2. Run integration tests: `bun run test principalReturn.test.ts` 3. Test admin action with confirmed deal via `returnInvestorPrincipal` 4. Verify idempotency by calling twice with same parameters 5. Test error cases (non-confirmed deals, failed transfers) ### Why make this change? **Critical Bug Fix:** The pipeline Leg 2 transfers were failing during cash ledger posting because `lenderId` was missing, causing `postLenderPayoutForTransfer` to throw errors. This fix ensures proper cash accounting for deal closing flows. **Product Requirement:** Investors need a way to receive principal returns when exiting deals. The new functionality provides both manual admin controls and the foundation for automated principal return triggers in future deal workflows. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Added admin action to initiate investor principal returns with proration support * Enhanced deal pipeline to track and propagate lender information through transfer legs * Introduced `lender_principal_return` transfer type with idempotency protection * **Tests** * Added unit tests for principal amount proration calculations * Added integration tests validating principal return creation and idempotency behavior <!-- 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.
TL;DR
Fixed critical
lenderIdmissing from deal closing pipeline Leg 2 transfers and implemented investor principal return functionality with admin action and comprehensive test coverage.What changed?
Pipeline Fix:
lenderIdfield toDealClosingLeg1Metadatainterface and threaded it through the entire pipeline chainextractLeg1Metadatato validate and extractlenderIdfrom metadatalenderIdfrom deal → Leg 1 metadata → Leg 2 transfer creationlenderIdfield to deals schema for storing resolved lender on approvalPrincipal Return Implementation:
principalReturn.logic.tswith pure functions for proration calculations and idempotency key generationprincipalReturn.tsorchestrator withcreatePrincipalReturninternal actionreturnInvestorPrincipaladmin action with deal status validation and idempotency checksHow to test?
Pipeline Fix:
lenderIdsetstartDealClosingPipelinelenderIdfield when createdPrincipal Return:
bun run test principalReturn.logic.test.tsbun run test principalReturn.test.tsreturnInvestorPrincipalWhy make this change?
Critical Bug Fix: The pipeline Leg 2 transfers were failing during cash ledger posting because
lenderIdwas missing, causingpostLenderPayoutForTransferto throw errors. This fix ensures proper cash accounting for deal closing flows.Product Requirement: Investors need a way to receive principal returns when exiting deals. The new functionality provides both manual admin controls and the foundation for automated principal return triggers in future deal workflows.
Summary by CodeRabbit
New Features
lender_principal_returntransfer type with idempotency protectionTests