active mortgage payment system re-alignemnt#387
Merged
Connorbelez merged 4 commits intomainfrom Apr 10, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a canonical internal execution API to convert eligible collection plan entries into collection attempts, with idempotency, eligibility guards, transfer-request handoff, schema audit fields, comprehensive backend tests, and documentation aligning TransferProvider as the canonical inbound provider while marking PaymentMethod as legacy compatibility. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant ExecuteAPI as executePlanEntry (Action)
participant StageMut as stagePlanEntryExecutionMutation
participant GuardLoad as loadExecutionPlanEntry
participant GuardClass as classifyExecutionEligibility
participant AttemptDB as collectionAttempts
participant PaymentRails as createTransferRequestInternal
participant HandoffMut as recordTransferHandoffMutation
Caller->>ExecuteAPI: executePlanEntry(args)
ExecuteAPI->>StageMut: stagePlanEntryExecutionMutation(ctx, args)
StageMut->>GuardLoad: loadExecutionPlanEntry(ctx, planEntryId)
GuardLoad-->>StageMut: LoadedExecutionPlanEntry
StageMut->>GuardClass: classifyExecutionEligibility(loaded, request)
alt Not eligible / rejected
GuardClass-->>StageMut: rejected / not_eligible result
StageMut-->>ExecuteAPI: StagePlanEntryExecutionResult
ExecuteAPI-->>Caller: ExecutePlanEntryResult (rejected/not_eligible)
else Eligible
GuardClass-->>StageMut: null (eligible)
StageMut->>AttemptDB: Create collectionAttempts(initiated + metadata)
StageMut->>AttemptDB: Patch planEntry -> executing + link attemptId
StageMut-->>ExecuteAPI: StagePlanEntryExecutionResult (attempt_created + transferHandoffRequest)
ExecuteAPI->>PaymentRails: createTransferRequestInternal(transferHandoffRequest)
alt Handoff Success
PaymentRails-->>ExecuteAPI: transferRequestId
ExecuteAPI->>HandoffMut: recordTransferHandoffSuccessMutation(attemptId, transferRequestId)
HandoffMut->>AttemptDB: Patch attempt (transfer_requested + transferRequestId)
ExecuteAPI-->>Caller: ExecutePlanEntryResult (attempt_created, success)
else Handoff Failure
PaymentRails-->>ExecuteAPI: Error
ExecuteAPI->>HandoffMut: recordTransferHandoffFailureMutation(attemptId, failureReason)
HandoffMut->>AttemptDB: Patch attempt (transfer_handoff_failed + failureReason)
ExecuteAPI-->>Caller: ExecutePlanEntryResult (attempt_created, transfer_handoff_failed)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 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 |
Owner
Author
This was referenced Apr 9, 2026
Merged
Reviewer's GuideIntroduces a canonical internal Collection Plan execution command that turns executable plan entries into governed collection attempts and hands off to Unified Payment Rails, enriches collection-related schemas with execution metadata, clarifies TransferProvider vs PaymentMethod roles in docs and interfaces, hardens the admin record-detail UI against missing route state, and adds local specs/PRDs plus contract-focused backend tests for the new execution API. Sequence diagram for canonical collection plan entry execution commandsequenceDiagram
actor Caller
participant ExecutePlanEntryAction as executePlanEntry_action
participant StageExecution as stagePlanEntryExecution_mutation
participant ExecutionGuards as executionGuards
participant PlanEntries as collectionPlanEntries_table
participant Attempts as collectionAttempts_table
participant Obligations as obligations_table
participant Transfers as transfers_domain
participant TransferRequests as transferRequests_table
participant AuditLog as auditLog_service
Caller->>ExecutePlanEntryAction: executePlanEntry(args)
ExecutePlanEntryAction->>StageExecution: stagePlanEntryExecutionMutation(args)
Note over StageExecution,PlanEntries: Validate idempotency key and load plan entry
StageExecution->>StageExecution: normalizeExecutionIdempotencyKey
alt invalid idempotency key
StageExecution->>AuditLog: log rejected_result invalid_idempotency_key
StageExecution-->>ExecutePlanEntryAction: StagePlanEntryExecutionResult(rejected)
ExecutePlanEntryAction-->>Caller: ExecutePlanEntryResult(rejected)
else valid idempotency key
StageExecution->>PlanEntries: get(planEntryId)
alt plan entry not found
StageExecution->>AuditLog: log rejected_result plan_entry_not_found
StageExecution-->>ExecutePlanEntryAction: StagePlanEntryExecutionResult(rejected)
ExecutePlanEntryAction-->>Caller: ExecutePlanEntryResult(rejected)
else plan entry loaded
StageExecution->>Attempts: get(planEntry.collectionAttemptId?)
StageExecution->>Attempts: query by_plan_entry(planEntryId) if needed
StageExecution->>Obligations: load each obligationId
StageExecution->>ExecutionGuards: classifyExecutionEligibility(loaded,args)
alt not eligible
ExecutionGuards-->>StageExecution: ExecutePlanEntryResult(not_eligible)
StageExecution->>AuditLog: log not_eligible_result
StageExecution-->>ExecutePlanEntryAction: StagePlanEntryExecutionResult(not_eligible)
ExecutePlanEntryAction-->>Caller: ExecutePlanEntryResult(not_eligible)
else existing attempt found
ExecutionGuards-->>StageExecution: null
StageExecution->>PlanEntries: patch sync collectionAttemptId and status
StageExecution->>AuditLog: log already_executed_result
StageExecution-->>ExecutePlanEntryAction: StagePlanEntryExecutionResult(already_executed)
ExecutePlanEntryAction-->>Caller: ExecutePlanEntryResult(already_executed)
else dry run requested
StageExecution->>AuditLog: log noop_result dry_run_requested
StageExecution-->>ExecutePlanEntryAction: StagePlanEntryExecutionResult(noop)
ExecutePlanEntryAction-->>Caller: ExecutePlanEntryResult(noop)
else eligible for execution
Note over StageExecution,Attempts: Create governed collection attempt and link plan entry
StageExecution->>Attempts: insert collectionAttempt (initiated)
StageExecution->>Attempts: patch machineContext with attemptId
StageExecution->>PlanEntries: patch status=executing, executedAt, executionIdempotencyKey, collectionAttemptId
StageExecution->>AuditLog: log attempt_created_result
StageExecution-->>ExecutePlanEntryAction: StagePlanEntryExecutionResult(attempt_created + transferHandoffRequest)
end
end
end
alt outcome is attempt_created
Note over ExecutePlanEntryAction,Transfers: Unified Payment Rails handoff
ExecutePlanEntryAction->>Transfers: createTransferRequestInternal(transferHandoffRequest)
alt transfer handoff succeeds
Transfers->>TransferRequests: insert transferRequest
Transfers-->>ExecutePlanEntryAction: transferRequestId
ExecutePlanEntryAction->>StageExecution: recordTransferHandoffSuccessMutation(attemptId, transferRequestId)
StageExecution->>Attempts: patch transferRequestId, providerStatus=transfer_requested
StageExecution->>AuditLog: log transfer_handoff.succeeded
ExecutePlanEntryAction-->>Caller: ExecutePlanEntryResult(attempt_created + transferRequestId)
else transfer handoff fails
Transfers-->>ExecutePlanEntryAction: throw error
ExecutePlanEntryAction->>StageExecution: recordTransferHandoffFailureMutation(attemptId, reasonDetail)
StageExecution->>Attempts: patch failureReason, providerStatus=transfer_handoff_failed
StageExecution->>AuditLog: log transfer_handoff.failed
ExecutePlanEntryAction-->>Caller: ExecutePlanEntryResult(attempt_created, reasonCode=transfer_handoff_failed)
end
else outcome is not attempt_created
ExecutePlanEntryAction-->>Caller: ExecutePlanEntryResult(result)
end
Entity relationship diagram for collection plan execution and transfer handofferDiagram
collectionPlanEntries {
string id
string[] obligationIds
string method
int amount
string status
int scheduledDate
int executedAt
string executionIdempotencyKey
string collectionAttemptId
}
collectionAttempts {
string id
string planEntryId
string method
int amount
string status
string triggerSource
int executionRequestedAt
string executionIdempotencyKey
string requestedByActorType
string requestedByActorId
string executionReason
string transferRequestId
string providerStatus
string failureReason
}
transferRequests {
string id
string direction
string transferType
int amount
string counterpartyType
string counterpartyId
string mortgageId
string obligationId
string planEntryId
string collectionAttemptId
string borrowerId
string providerCode
string idempotencyKey
}
obligations {
string id
string borrowerId
string mortgageId
string status
string type
}
borrowers {
string id
}
mortgages {
string id
}
collectionPlanEntries ||--o{ obligations : references
collectionPlanEntries ||--o| collectionAttempts : has_optional
collectionAttempts ||--o| transferRequests : has_optional
obligations }o--|| borrowers : belongs_to
obligations }o--|| mortgages : belongs_to
transferRequests }o--|| borrowers : counterparty
transferRequests }o--|| mortgages : related_mortgage
Class diagram for collection plan execution contract and guardsclassDiagram
class ExecutePlanEntryArgs {
string idempotencyKey
Id_collectionPlanEntries planEntryId
string triggerSource
int requestedAt
string requestedByActorType
string requestedByActorId
string reason
boolean dryRun
}
class ExecutionTriggerSource {
<<enumeration>>
system_scheduler
admin_manual
workflow_replay
migration_backfill
}
class ExecutionRequestedByActorType {
<<enumeration>>
system
admin
workflow
}
class ExecutePlanEntryReasonCode {
<<enumeration>>
dry_run_requested
invalid_idempotency_key
missing_execution_metadata
obligation_not_collectible
obligation_not_found
plan_entry_not_due
plan_entry_not_executable_state
plan_entry_not_found
plan_entry_already_executed
transfer_handoff_failed
}
class PlanEntryExecutionOutcome {
<<enumeration>>
attempt_created
already_executed
not_eligible
rejected
noop
}
class PlanEntryStatus {
<<enumeration>>
planned
executing
completed
cancelled
rescheduled
}
class ExecutePlanEntryResultBase {
int executionRecordedAt
string idempotencyKey
PlanEntryExecutionOutcome outcome
Id_collectionPlanEntries planEntryId
PlanEntryStatus planEntryStatusAfter
ExecutePlanEntryReasonCode reasonCode
string reasonDetail
}
class ExecutePlanEntryAttemptResult {
string attemptStatusAfter
Id_collectionAttempts collectionAttemptId
Id_transferRequests transferRequestId
}
class AttemptCreatedResult {
}
class AlreadyExecutedResult {
}
class NotEligibleResult {
}
class RejectedResult {
}
class NoopResult {
}
class ExecutePlanEntryResult {
<<union_type>>
}
class TransferHandoffRequest {
int amount
Id_borrowers borrowerId
Id_collectionAttempts collectionAttemptId
string counterpartyId
string idempotencyKey
string method
Id_mortgages mortgageId
Id_obligations[] obligationIds
Id_collectionPlanEntries planEntryId
string primaryObligationType
CommandSource source
}
class StagePlanEntryExecutionResult {
ExecutePlanEntryResult result
TransferHandoffRequest transferHandoffRequest
}
class LoadedExecutionPlanEntry {
PlanEntryStatus currentPlanEntryStatus
Doc_collectionAttempts existingAttempt
Doc_obligations[] obligations
Doc_collectionPlanEntries planEntry
}
class ExecutionContractFunctions {
+CommandSource buildExecutionSource(ExecutePlanEntryArgs args)
+string normalizeExecutionIdempotencyKey(string idempotencyKey)
+string buildTransferHandoffIdempotencyKey(Id_collectionPlanEntries planEntryId)
+any buildTransferHandoffMetadata(TransferHandoffRequest request, string firstObligationType)
+AttemptCreatedResult buildAttemptCreatedResult(AttemptCreatedResult result)
+AlreadyExecutedResult buildAlreadyExecutedResult(AlreadyExecutedResult result)
+NotEligibleResult buildNotEligibleResult(NotEligibleResult result)
+RejectedResult buildRejectedResult(RejectedResult result)
+NoopResult buildNoopResult(NoopResult result)
}
class ExecutionGuardsFunctions {
+Promise~LoadedExecutionPlanEntry~ loadExecutionPlanEntry(MutationCtx ctx, Id_collectionPlanEntries planEntryId)
+ExecutePlanEntryResult classifyExecutionEligibility(int executionRecordedAt, string idempotencyKey, LoadedExecutionPlanEntry loaded, ExecutePlanEntryArgs request)
}
ExecutePlanEntryArgs --> ExecutionTriggerSource : uses
ExecutePlanEntryArgs --> ExecutionRequestedByActorType : uses
ExecutePlanEntryResultBase <|-- ExecutePlanEntryAttemptResult
ExecutePlanEntryResultBase <|-- NotEligibleResult
ExecutePlanEntryResultBase <|-- RejectedResult
ExecutePlanEntryResultBase <|-- NoopResult
ExecutePlanEntryAttemptResult <|-- AttemptCreatedResult
ExecutePlanEntryAttemptResult <|-- AlreadyExecutedResult
ExecutePlanEntryResult "1" o-- "1" AttemptCreatedResult
ExecutePlanEntryResult "1" o-- "1" AlreadyExecutedResult
ExecutePlanEntryResult "1" o-- "1" NotEligibleResult
ExecutePlanEntryResult "1" o-- "1" RejectedResult
ExecutePlanEntryResult "1" o-- "1" NoopResult
StagePlanEntryExecutionResult "1" o-- "1" ExecutePlanEntryResult
StagePlanEntryExecutionResult "0..1" o-- "1" TransferHandoffRequest
LoadedExecutionPlanEntry --> ExecutePlanEntryArgs : evaluated_against
ExecutionGuardsFunctions --> LoadedExecutionPlanEntry : returns
ExecutionGuardsFunctions --> ExecutePlanEntryResult : returns
ExecutionContractFunctions ..> ExecutePlanEntryArgs : helper_for
ExecutionContractFunctions ..> ExecutePlanEntryResult : builds
ExecutionContractFunctions ..> TransferHandoffRequest : metadata_for
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
mapMethodToProviderCode, silently defaulting unknown methods tomanualrisks masking config/enum drift; consider either tightening the mapping (e.g. rejecting unknown methods) or at least logging/recording an explicit reason code when a fallback provider is used. - The execution result/reason-code taxonomy is spread across
executionContract,executionGuards, and the transfer handoff catch block; you might centralize reason-code construction helpers or enums so that adding new outcomes or codes is less error-prone and easier to audit. - The
dryRunhandling currently returns anoopoutcome without exercising downstream paths; if you expect to rely on this for operational tooling, consider extending it to run the full eligibility logic and return the would-be attempt/transfer metadata (without writes) so callers can inspect what would happen.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `mapMethodToProviderCode`, silently defaulting unknown methods to `manual` risks masking config/enum drift; consider either tightening the mapping (e.g. rejecting unknown methods) or at least logging/recording an explicit reason code when a fallback provider is used.
- The execution result/reason-code taxonomy is spread across `executionContract`, `executionGuards`, and the transfer handoff catch block; you might centralize reason-code construction helpers or enums so that adding new outcomes or codes is less error-prone and easier to audit.
- The `dryRun` handling currently returns a `noop` outcome without exercising downstream paths; if you expect to rely on this for operational tooling, consider extending it to run the full eligibility logic and return the would-be attempt/transfer metadata (without writes) so callers can inspect what would happen.
## Individual Comments
### Comment 1
<location path="convex/payments/collectionPlan/__tests__/execution.test.ts" line_range="138-51" />
<code_context>
+ it("returns already_executed on replay without creating a duplicate attempt", async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider a test for reconciling an existing attempt when the plan entry’s collectionAttemptId is unset or mismatched
The mutation also handles the case where an `existingAttempt` exists but the plan entry’s `collectionAttemptId` is missing or points to a different attempt. Your current test only covers the aligned case.
Please add a test that:
- Pre-creates a `collectionAttempt` for the plan entry,
- Leaves `planEntry.collectionAttemptId` unset or set to a different ID,
- Calls `executePlanEntry`, and
- Asserts the result is `already_executed` and that `collectionAttemptId`, `executedAt`, `executionIdempotencyKey`, and status on the plan entry are updated to match the existing attempt.
This will directly validate the reconciliation behavior.
Suggested implementation:
```typescript
expect(transfer?.planEntryId).toBe(planEntryId);
});
it("reconciles existing attempt when plan entry collectionAttemptId is mismatched", async () => {
const t = createGovernedTestConvex();
const { planEntryId } = await seedExecutionFixture(t);
const requestedAt = Date.now();
// First execution creates the initial attempt
const firstResult = await t.mutation(
api.payments.collectionPlan.executePlanEntry,
{
planEntryId,
triggerSource: "system_scheduler",
requestedAt,
idempotencyKey: "exec-plan-entry-reconcile-1",
requestedByActorType: "system",
requestedByActorId: "scheduler",
}
);
if (firstResult.type !== "success") {
throw new Error(
`Expected first execution to succeed, got ${firstResult.type}`
);
}
const existingAttemptId = firstResult.collectionAttemptId;
// Sanity check: we have an existing attempt associated with this plan entry
expect(existingAttemptId).toBeDefined();
// Load the plan entry created by the fixture / first execution
const originalPlanEntry = await getPlanEntry(t, planEntryId);
expect(originalPlanEntry?.collectionAttemptId).toEqual(existingAttemptId);
// Simulate a mismatched collectionAttemptId on the plan entry while the attempt still exists
// (either unset or pointing at a different attempt).
// Here we explicitly clear it to exercise the reconciliation path.
await updatePlanEntryCollectionAttemptIdForTest(t, planEntryId, null);
const mutatedPlanEntry = await getPlanEntry(t, planEntryId);
expect(mutatedPlanEntry?.collectionAttemptId).toBeNull();
// Re‑execute with a different idempotency key to hit the reconciliation logic
const replayResult = await t.mutation(
api.payments.collectionPlan.executePlanEntry,
{
planEntryId,
triggerSource: "system_scheduler",
requestedAt: requestedAt + 1000,
idempotencyKey: "exec-plan-entry-reconcile-2",
requestedByActorType: "system",
requestedByActorId: "scheduler",
}
);
// The mutation should recognize the existing attempt and treat this as already executed
expect(replayResult.type).toBe("already_executed");
expect(replayResult.collectionAttemptId).toEqual(existingAttemptId);
// The plan entry should now be reconciled to match the existing attempt
const reconciledPlanEntry = await getPlanEntry(t, planEntryId);
expect(reconciledPlanEntry?.collectionAttemptId).toEqual(existingAttemptId);
expect(reconciledPlanEntry?.executedAt).toEqual(
originalPlanEntry?.executedAt
);
expect(reconciledPlanEntry?.executionIdempotencyKey).toEqual(
originalPlanEntry?.executionIdempotencyKey
);
expect(reconciledPlanEntry?.status).toEqual(originalPlanEntry?.status);
});
it("returns already_executed on replay without creating a duplicate attempt", async () => {
```
To make this compile and align with the rest of your test utilities, you’ll need to:
1. Ensure `api` is imported at the top of this file (most Convex tests already import it via `import { api } from "convex/_generated/api";` or similar).
2. Implement or wire the following helper functions used in the new test (if they don’t already exist):
- `getPlanEntry(t, planEntryId)` – should query the plan entry document (including `collectionAttemptId`, `executedAt`, `executionIdempotencyKey`, and `status`) by its ID.
- `updatePlanEntryCollectionAttemptIdForTest(t, planEntryId, collectionAttemptId)` – should update just the `collectionAttemptId` field on the given plan entry (used here to clear it to `null`), ideally in a small internal/test‑only mutation.
3. If your plan entry model uses a different field name for `status`, `executedAt`, or `executionIdempotencyKey`, adjust the property names in the assertions accordingly.
4. If the existing tests use a different way to invoke the mutation (e.g. `t.runMutation` instead of `t.mutation`), update the two `t.mutation(api.payments.collectionPlan.executePlanEntry, ...)` calls to match the prevailing pattern in this file.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR realigns the Active Mortgage Payment System around the transfer-domain provider abstraction and introduces a canonical internal Collection Plan execution API that creates governed collectionAttempts and hands off to Unified Payment Rails via transferRequests, alongside a small admin UI robustness fix and extensive in-repo spec/docs alignment.
Changes:
- Add canonical internal
executePlanEntrycontract (types/validators/guards + internal action/mutations) and schema fields for replay-safe execution + traceability. - Add backend contract tests for plan-entry execution outcomes and transfer-handoff failure preservation.
- Update docs/spec artifacts to reflect the transfer-domain direction; harden admin record-link rendering when route state is incomplete.
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/routes/admin/route.tsx | Import ordering cleanup for admin route states. |
| src/lib/admin-detail-route-state.ts | Formatting-only signature change. |
| src/components/admin/shell/RecordSidebarProvider.tsx | Formatting/type import ordering only. |
| src/components/admin/shell/AdminDetailSheet.tsx | Makes entityType optional and guards record-link creation to prevent runtime/type errors. |
| convex/schema.ts | Adds execution/linkage metadata fields to collectionPlanEntries and collectionAttempts. |
| convex/payments/collectionPlan/executionContract.ts | Defines canonical execution input/result contract, validators, and metadata helpers. |
| convex/payments/collectionPlan/executionGuards.ts | Adds plan-entry loading + eligibility/replay classification helpers. |
| convex/payments/collectionPlan/execution.ts | Implements the canonical internal execution command + transfer-request handoff recording. |
| convex/payments/collectionPlan/tests/execution.test.ts | Adds contract-focused tests for executePlanEntry. |
| convex/_generated/api.d.ts | Updates generated API typings to include new collectionPlan execution modules. |
| convex/payments/transfers/interface.ts | Clarifies TransferProvider as canonical for new inbound provider work (comments only). |
| convex/payments/transfers/providers/adapter.ts | Updates adapter docs to emphasize temporary migration bridge. |
| convex/payments/transfers/providers/manual.ts | Updates manual provider docs to emphasize canonical transfer-domain boundary. |
| convex/payments/methods/interface.ts | Clarifies PaymentMethod as legacy compatibility (comments only). |
| convex/payments/methods/manual.ts | Clarifies manual method as legacy compatibility (comments only). |
| convex/payments/methods/registry.ts | Clarifies registry as legacy compatibility lookup (comments only). |
| docs/cash-ledger-developer-guide.md | Adds contract alignment notes for execution boundary and provider abstraction. |
| docs/technical-design/unified-payment-rails.md | Adds status note + updates wording to reflect transfer-domain direction. |
| docs/architecture/unified-payment-rails-technical-design.md | Adds status note and clarifies legacy vs canonical abstractions. |
| specs/active-mortgage-payment-system-alignment-2026-04-03.md | Adds comprehensive alignment/gap analysis doc. |
| specs/01-spec-and-contract-cleanup/* | Adds local PRD/design/tasks/gap-analysis artifacts for doc/contract cleanup workstream. |
| specs/02-define-canonical-plan-entry-execution-api/* | Adds local PRD/design/tasks/gap-analysis artifacts for execution API workstream. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Connorbelez
force-pushed
the
04-03-active_mortgage_payment_system_re-alignemnt
branch
from
cc58a7c to
3f2a656
Compare
Owner
Author
|
@coderabbitai please review |
Contributor
|
🧠 Learnings used✅ Actions performedReview 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/technical-design/unified-payment-rails.md (1)
178-191:⚠️ Potential issue | 🟡 MinorAlign the concrete provider examples with the new canonical boundary.
These sections still describe new Rotessa/Stripe integrations as
PaymentMethodimplementations, which conflicts with the historical note at the top and with the new execution flow handing off through the transfer domain. Leaving these examples as-is makes the doc internally inconsistent and points future provider work at the legacy abstraction.Also applies to: 233-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/technical-design/unified-payment-rails.md` around lines 178 - 191, The document's integration examples still model Rotessa/Stripe as direct PaymentMethod implementations (e.g., PaymentMethod, RealPADMethod.initiate(), Collection Plan entries using method: "rotessa_pad"), which conflicts with the new canonical boundary that routes provider-specific work through the transfer domain; update the examples and flow text to replace direct PaymentMethod implementation language with the new transfer-domain handoff: show creating a transfer instruction/transfer request in place of calling PaymentMethod implementations, describe RealPADMethod.initiate() as producing a transfer-domain payload handed to TransferService (or equivalent), and adjust Collection Plan steps to record a transfer-type method and reference the transfer processing and webhooks rather than legacy PaymentMethod APIs so all provider examples align with the transfer-domain execution flow.
🤖 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/collectionPlan/execution.ts`:
- Around line 42-46: The current mapMethodToProviderCode silently maps
unknown/typoed method strings to "manual", which masks bad input; change
mapMethodToProviderCode to fail closed by validating that method exists in
PROVIDER_CODES (the PROVIDER_CODES array and ProviderCode type) and throwing a
descriptive Error (including the invalid method value) if it's not present so
callers cannot accidentally route to the manual provider; after this change
update any call sites that relied on the implicit fallback to handle or
propagate the thrown error appropriately.
- Around line 198-208: The code currently uses only firstObligation for
borrowerId/mortgageId/type and downstream handoff; change execution validation
to ensure all obligations in the obligations array share the same borrowerId,
mortgageId and type before proceeding (i.e., iterate obligations and compare
each obligation.borrowerId, obligation.mortgageId and obligation.type to
firstObligation's values), and if any mismatch is found return
buildRejectedResult (same shape as existing rejection using executionRecordedAt,
normalizedIdempotencyKey, planEntry._id, planEntry.status and reasonCode
"mixed_obligation_context" with a clear reasonDetail). Update any other branches
that read only firstObligation (referenced symbols: firstObligation,
obligations, buildRejectedResult, normalizedIdempotencyKey, planEntry) to rely
on this validation so mixed or malformed plan entries are rejected rather than
producing an incorrect downstream transfer.
- Around line 134-168: The existingAttempt branch currently returns early with
buildAlreadyExecutedResult and prevents reconciling a partially completed
handoff (createTransferRequestInternal succeeded but
recordTransferHandoffSuccessMutation never ran), so change the logic in the
existingAttempt handling inside execution.ts to detect a partial handoff
(existingAttempt exists AND existingAttempt.transferRequestId is null but the
idempotency matches normalizedIdempotencyKey /
planEntry.executionIdempotencyKey) and instead continue reconciliation: update
planEntry fields as before, then invoke the handoff reconciliation path (e.g.,
call or re-run recordTransferHandoffSuccessMutation or the same
success-recording routine that sets transferRequestId) and only return
buildAlreadyExecutedResult after transferRequestId has been reconciled/logged
via logPlanEntryExecutionAudit; reference symbols: existingAttempt, planEntry,
normalizedIdempotencyKey, transferRequestId, createTransferRequestInternal,
recordTransferHandoffSuccessMutation, buildAlreadyExecutedResult,
logPlanEntryExecutionAudit.
In `@convex/payments/collectionPlan/executionContract.ts`:
- Around line 148-171: The buildExecutionSource function collapses the
"workflow_replay" trigger into the default "scheduler" case; add an explicit
case for "workflow_replay" in the switch (alongside the existing "admin_manual"
and "migration_backfill" cases) that returns channel: "workflow_replay",
actorId: args.requestedByActorId, and actorType: "system" so workflow replay
executions preserve their distinct trigger source and actor attribution in
downstream records.
In `@specs/01-spec-and-contract-cleanup/gap-analysis.md`:
- Around line 28-30: The markdown table contains machine-local absolute file
paths; update those three links to repo-relative paths so they work for
everyone: replace "/Users/.../convex/payments/transfers/interface.ts",
"/Users/.../convex/payments/methods/interface.ts", and
"/Users/.../docs/cash-ledger-developer-guide.md" with relative links like
"convex/payments/transfers/interface.ts",
"convex/payments/methods/interface.ts", and
"docs/cash-ledger-developer-guide.md" (preserve any markdown link text and
ensure paths are correct relative to the repository root).
In `@specs/02-define-canonical-plan-entry-execution-api/design.md`:
- Around line 83-90: The doc incorrectly calls executePlanEntry an internal
mutation; update the design text to state that executePlanEntry is an internal
action and that the staging step is the internal mutation (referencing the
"staging mutation" that creates the Collection Attempt). Explicitly document
that replay/eligibility guards and the staging mutation occur in the transaction
boundary, while the transfer-request handoff (creation through the Unified
Payment Rails contract / transfer-request) occurs after the staging mutation as
a separate step and therefore outside the same transaction boundary.
In `@specs/active-mortgage-payment-system-alignment-2026-04-03.md`:
- Around line 43-50: Update the alignment document to remove or rephrase the
obsolete "canonical plan-entry → collection-attempt execution path is missing"
entries and mark that the execution spine added in this PR is implemented;
specifically edit the occurrences of that assessment in the bullet list ("What
is still missing") and the other referenced sections (around the blocks noted as
102-105, 232-243, 361-362) to reflect that the API and tests for the plan-entry
→ collection-attempt path now exist, and if appropriate add a short note linking
to the PR/tests or the canonical API name so readers know the implemented
coverage.
---
Outside diff comments:
In `@docs/technical-design/unified-payment-rails.md`:
- Around line 178-191: The document's integration examples still model
Rotessa/Stripe as direct PaymentMethod implementations (e.g., PaymentMethod,
RealPADMethod.initiate(), Collection Plan entries using method: "rotessa_pad"),
which conflicts with the new canonical boundary that routes provider-specific
work through the transfer domain; update the examples and flow text to replace
direct PaymentMethod implementation language with the new transfer-domain
handoff: show creating a transfer instruction/transfer request in place of
calling PaymentMethod implementations, describe RealPADMethod.initiate() as
producing a transfer-domain payload handed to TransferService (or equivalent),
and adjust Collection Plan steps to record a transfer-type method and reference
the transfer processing and webhooks rather than legacy PaymentMethod APIs so
all provider examples align with the transfer-domain execution flow.
🪄 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: 62595e4f-fe6c-4721-97fb-565fff4ed363
⛔ Files ignored due to path filters (1)
convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (23)
convex/payments/collectionPlan/__tests__/execution.test.tsconvex/payments/collectionPlan/execution.tsconvex/payments/collectionPlan/executionContract.tsconvex/payments/collectionPlan/executionGuards.tsconvex/payments/methods/interface.tsconvex/payments/methods/manual.tsconvex/payments/methods/registry.tsconvex/payments/transfers/interface.tsconvex/payments/transfers/providers/adapter.tsconvex/payments/transfers/providers/manual.tsconvex/schema.tsdocs/architecture/unified-payment-rails-technical-design.mddocs/cash-ledger-developer-guide.mddocs/technical-design/unified-payment-rails.mdspecs/01-spec-and-contract-cleanup/PRD.mdspecs/01-spec-and-contract-cleanup/design.mdspecs/01-spec-and-contract-cleanup/gap-analysis.mdspecs/01-spec-and-contract-cleanup/tasks.mdspecs/02-define-canonical-plan-entry-execution-api/PRD.mdspecs/02-define-canonical-plan-entry-execution-api/design.mdspecs/02-define-canonical-plan-entry-execution-api/gap-analysis.mdspecs/02-define-canonical-plan-entry-execution-api/tasks.mdspecs/active-mortgage-payment-system-alignment-2026-04-03.md
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
convex/payments/collectionPlan/__tests__/execution.test.ts (1)
39-63: Minor:seedMortgageisn't linked to the seeded borrower.
seedMortgage(t)usesHARDCODED_BORROWER_IDinternally whileseedObligationuses the freshly seededborrowerId. The execution logic readsborrowerIdfrom the obligation (not the mortgage), so tests pass, but the fixture has a subtle data inconsistency.Consider passing the borrowerId to seedMortgage for cleaner test data:
const borrowerId = await seedBorrowerProfile(t); -const mortgageId = await seedMortgage(t); +const mortgageId = await seedMortgage(t, { borrowerId });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/collectionPlan/__tests__/execution.test.ts` around lines 39 - 63, The fixture seeds a mortgage with seedMortgage(t) using HARDCODED_BORROWER_ID which mismatches the borrower created by seedBorrowerProfile(t); update seedExecutionFixture to pass the freshly created borrowerId into seedMortgage (i.e., call seedMortgage(t, borrowerId)) and adjust seedMortgage's signature/implementation to accept and use an optional borrowerId (falling back to HARDCODED_BORROWER_ID only if none provided), so seedMortgage, seedObligation and the resulting planEntry all reference the same borrower.convex/payments/collectionPlan/executionContract.ts (1)
182-187: Consider documenting the null-return behavior.
normalizeExecutionIdempotencyKeyreturnsnullfor empty/whitespace-only keys. The caller (stagePlanEntryExecution) correctly handles this, but a brief JSDoc would clarify this is a validation helper, not just a normalizer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/collectionPlan/executionContract.ts` around lines 182 - 187, Add a short JSDoc to the normalizeExecutionIdempotencyKey function documenting that it trims the input and returns null for empty or whitespace-only strings (i.e., it's a validation helper, not just a normalizer), and mention that callers such as stagePlanEntryExecution must handle the null case; keep the comment directly above the normalizeExecutionIdempotencyKey function and be concise about its contract and return type.
🤖 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/collectionPlan/__tests__/execution.test.ts`:
- Around line 39-63: The fixture seeds a mortgage with seedMortgage(t) using
HARDCODED_BORROWER_ID which mismatches the borrower created by
seedBorrowerProfile(t); update seedExecutionFixture to pass the freshly created
borrowerId into seedMortgage (i.e., call seedMortgage(t, borrowerId)) and adjust
seedMortgage's signature/implementation to accept and use an optional borrowerId
(falling back to HARDCODED_BORROWER_ID only if none provided), so seedMortgage,
seedObligation and the resulting planEntry all reference the same borrower.
In `@convex/payments/collectionPlan/executionContract.ts`:
- Around line 182-187: Add a short JSDoc to the normalizeExecutionIdempotencyKey
function documenting that it trims the input and returns null for empty or
whitespace-only strings (i.e., it's a validation helper, not just a normalizer),
and mention that callers such as stagePlanEntryExecution must handle the null
case; keep the comment directly above the normalizeExecutionIdempotencyKey
function and be concise about its contract and return type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a677066a-55bc-4683-8c32-0949ac86c128
📒 Files selected for processing (10)
convex/crm/__tests__/viewEngine.test.tsconvex/crm/viewState.tsconvex/payments/collectionPlan/__tests__/execution.test.tsconvex/payments/collectionPlan/execution.tsconvex/payments/collectionPlan/executionContract.tsconvex/payments/collectionPlan/executionGuards.tsconvex/test/registerAuditLogComponent.tsspecs/01-spec-and-contract-cleanup/gap-analysis.mdspecs/02-define-canonical-plan-entry-execution-api/design.mdspecs/active-mortgage-payment-system-alignment-2026-04-03.md
✅ Files skipped from review due to trivial changes (4)
- convex/crm/tests/viewEngine.test.ts
- convex/crm/viewState.ts
- specs/active-mortgage-payment-system-alignment-2026-04-03.md
- specs/02-define-canonical-plan-entry-execution-api/design.md
🚧 Files skipped from review as they are similar to previous changes (1)
- convex/payments/collectionPlan/executionGuards.ts
Owner
Author
Merge activity
|
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.
Summary by Sourcery
Align the Active Mortgage Payment System architecture and contracts with the unified transfer-domain design while introducing a canonical collection plan execution API.
Enhancements:
TransferProvideras the canonical inbound contract andPaymentMethodas legacy compatibility.Documentation:
Tests:
Summary by CodeRabbit
New Features
Schema
Tests
Documentation