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 ignored due to path filters (1)
📒 Files selected for processing (76)
📝 WalkthroughWalkthroughThis PR implements a production collection-plan execution spine for mortgage collections, adding a cron-driven runner that executes due plan entries, reconciles transfer outcomes with collection attempts via inbound settlement/failure/cancellation paths, expands the collection-rules model to use explicit typed kinds and configs, and deprecates legacy PaymentMethod interfaces in favor of TransferProvider as the canonical inbound boundary. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Connorbelez, your pull request is larger than the review limit of 150000 diff characters
Owner
Author
This was referenced Apr 9, 2026
Merged
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Pull request overview
This PR strengthens the payments “execution spine” by (1) running due collectionPlanEntries through the canonical executePlanEntry path (including transfer initiation + attempt GT advancement), (2) reconciling transfer outcomes back into the originating collectionAttempt, and (3) upgrading collectionRules from legacy name/parameters rows to a typed rule envelope with canonical seeding and deterministic selection.
Changes:
- Added a scheduler-owned collection-plan runner + cron to execute due planned entries through
executePlanEntry, initiating transfers and advancing attempts. - Routed transfer confirm/fail/cancel/reverse outcomes through collection-attempt reconciliation and updated health/orphan detection to recognize attempt-owned postings.
- Introduced typed collection-rule contract (kind/status/scope/effective window/config), canonical seed/backfill, shared initial-scheduling seam, and expanded integration/contract test coverage.
Reviewed changes
Copilot reviewed 73 out of 74 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/convex/seed/seedPaymentData.test.ts | New integration tests for canonical seed + initial scheduling provenance and idempotency. |
| src/test/convex/payments/helpers.ts | Test helper now seeds canonical rules via internal seed mutation and returns canonical IDs. |
| src/test/convex/payments/endToEnd.test.ts | Relabels manual/PAD lifecycle suites as legacy compatibility flows. |
| src/test/convex/engine/transition.test.ts | Adds regression ensuring XState assign-based actions don’t get scheduled as effects/warnings. |
| specs/07-expand-collection-rule-model-and-complete-retry-late-fee-behaviors/tasks.md | Adds local page-07 task checklist artifact. |
| specs/07-expand-collection-rule-model-and-complete-retry-late-fee-behaviors/PRD.md | Adds local page-07 PRD snapshot for typed collection rules. |
| specs/07-expand-collection-rule-model-and-complete-retry-late-fee-behaviors/gap-analysis.md | Adds page-07 gap analysis + verification evidence. |
| specs/07-expand-collection-rule-model-and-complete-retry-late-fee-behaviors/design.md | Adds page-07 design snapshot for typed rule envelope and engine. |
| specs/06-correct-activation-and-initial-scheduling-handoff/tasks.md | Adds local page-06 task checklist artifact. |
| specs/06-correct-activation-and-initial-scheduling-handoff/PRD.md | Adds local page-06 PRD snapshot for canonical initial scheduling handoff. |
| specs/06-correct-activation-and-initial-scheduling-handoff/gap-analysis.md | Adds page-06 gap analysis + verification evidence. |
| specs/06-correct-activation-and-initial-scheduling-handoff/design.md | Adds page-06 design snapshot for shared initial scheduling seam. |
| specs/05-converge-inbound-provider-boundary-on-transferprovider/tasks.md | Adds local page-05 task checklist artifact. |
| specs/05-converge-inbound-provider-boundary-on-transferprovider/PRD.md | Adds local page-05 PRD snapshot for provider boundary convergence. |
| specs/05-converge-inbound-provider-boundary-on-transferprovider/gap-analysis.md | Adds page-05 gap analysis + verification evidence. |
| specs/05-converge-inbound-provider-boundary-on-transferprovider/design.md | Adds page-05 design snapshot for provider boundary and compatibility fencing. |
| specs/04-reconcile-collection-attempts-with-transfer-execution-and-cash-posting/tasks.md | Adds local page-04 task checklist artifact. |
| specs/04-reconcile-collection-attempts-with-transfer-execution-and-cash-posting/PRD.md | Adds local page-04 PRD snapshot for transfer→attempt reconciliation. |
| specs/04-reconcile-collection-attempts-with-transfer-execution-and-cash-posting/gap-analysis.md | Adds page-04 gap analysis + verification evidence. |
| specs/04-reconcile-collection-attempts-with-transfer-execution-and-cash-posting/design.md | Adds page-04 design snapshot for reconciliation coordinator. |
| specs/03-implement-collection-plan-collection-attempt-execution-spine/tasks.md | Adds local page-03 task checklist artifact. |
| specs/03-implement-collection-plan-collection-attempt-execution-spine/PRD.md | Adds local page-03 PRD snapshot for due-entry runner + execution spine. |
| specs/03-implement-collection-plan-collection-attempt-execution-spine/gap-analysis.md | Adds page-03 gap analysis + verification evidence. |
| specs/03-implement-collection-plan-collection-attempt-execution-spine/design.md | Adds page-03 design snapshot for runner/execution mapping and GT advancement. |
| specs/01-spec-and-contract-cleanup/gap-analysis.md | Fixes local absolute links to repo-relative links. |
| docs/technical-design/unified-payment-rails.md | Clarifies adapter/legacy registry are compatibility shims, not extension points. |
| docs/architecture/unified-payment-rails-technical-design.md | Clarifies legacy registry is compatibility-only and points to canonical transfer-provider registry. |
| convex/seed/seedPaymentData.ts | Refactors seeding to ensure canonical rules + run canonical initial scheduling seam (obligations-first). |
| convex/schema.ts | Expands collectionRules to typed envelope fields while keeping legacy compatibility fields. |
| convex/payments/transfers/reconciliation.ts | Treats attempt-linked inbound transfers as healthy based on attempt-owned postings; resolves healing accordingly. |
| convex/payments/transfers/providers/adapter.ts | Rejects non-legacy provider codes in PaymentMethodAdapter to fence compatibility usage. |
| convex/payments/transfers/providers/tests/adapter.test.ts | Updates adapter tests for legacy-only codes and adds rejection cases for canonical codes. |
| convex/payments/transfers/mutations.ts | Clarifies provider boundary and persists providerRef consistently; keeps internal initiation retry-safe. |
| convex/payments/transfers/collectionAttemptReconciliation.ts | New module: attempt-linked inbound settlement/failure/cancel/reversal reconciliation + posting health helpers. |
| convex/payments/transfers/tests/transferMachine.test.ts | Updates expectations for new cancellation effect action. |
| convex/payments/transfers/tests/reconciliation.test.ts | Updates orphan-detection framing for attempt-linked transfers (no longer “bridged == healthy”). |
| convex/payments/transfers/tests/handlers.integration.test.ts | Switches audit-log registration helper and makes cancelTransfer test timer-safe. |
| convex/payments/methods/registry.ts | Narrows legacy method registry to explicit legacy codes with clearer errors + deprecation markers. |
| convex/payments/methods/mockPAD.ts | Marks mock PAD method as deprecated compatibility-only. |
| convex/payments/methods/manual.ts | Marks manual method as deprecated compatibility-only. |
| convex/payments/methods/interface.ts | Introduces LEGACY_PAYMENT_METHOD_CODES + type guard; tightens InitiateParams.method typing; marks PaymentMethod deprecated. |
| convex/payments/collectionPlan/seed.ts | Delegates rule seeding to canonical typed defaultRules seeder. |
| convex/payments/collectionPlan/runner.ts | New internal action: bounded due-entry runner that executes plan entries via canonical spine. |
| convex/payments/collectionPlan/rules/scheduleRule.ts | Schedule rule now delegates to canonical initial scheduling mutation and reads typed config. |
| convex/payments/collectionPlan/rules/retryRule.ts | Retry rule now reads typed config with legacy fallback. |
| convex/payments/collectionPlan/rules/lateFeeRule.ts | Late-fee rule now reads typed config and uses typed fee surface/code. |
| convex/payments/collectionPlan/ruleContract.ts | New typed rule kind/status/scope/config contract + validators + legacy mapping helpers. |
| convex/payments/collectionPlan/queries.ts | Updates enabled-rule query to typed active/effective/scope filtering; adds due planned entries query. |
| convex/payments/collectionPlan/mutations.ts | Adds canonical scheduleInitialEntries internal mutation backed by shared implementation. |
| convex/payments/collectionPlan/initialScheduling.ts | New shared canonical initial scheduling seam (idempotent, obligation-first). |
| convex/payments/collectionPlan/executionContract.ts | Extends staging result to carry recovered existing transferRequestId. |
| convex/payments/collectionPlan/execution.ts | Extends executePlanEntry to initiate transfer and advance attempt via GT based on initiation outcome; improves replay recovery. |
| convex/payments/collectionPlan/engine.ts | Dispatches rule handlers by typed kind (not legacy name) and passes mortgageId/asOf for selection. |
| convex/payments/collectionPlan/defaultRules.ts | New canonical typed default rules + idempotent seed/backfill across legacy+typed rows. |
| convex/payments/collectionPlan/tests/runner.test.ts | New integration tests for due-entry runner (success, replay safety, failure→retry loop). |
| convex/payments/collectionPlan/tests/execution.test.ts | Updates execution integration tests to validate initiation + confirmed settlement + replay behavior. |
| convex/payments/collectionPlan/tests/engine.test.ts | New contract tests for typed rule selection, dispatch-by-kind, and legacy backfill seeding. |
| convex/payments/cashLedger/transferReconciliation.ts | Updates orphan detection/health checks for attempt-linked inbound settlements/reversals. |
| convex/payments/cashLedger/tests/transferReconciliation.test.ts | Adds tests covering attempt-linked settlement/reversal health classification. |
| convex/payments/tests/rules.test.ts | Updates rule tests to typed rule envelope + schedule-rule delegation behavior. |
| convex/payments/tests/methods.test.ts | Relabels as compatibility coverage; updates error patterns; adds transfer-only mock code rejection. |
| convex/engine/transition.ts | Switches to XState transition output for action extraction; avoids mis-scheduling assign actions as effects. |
| convex/engine/machines/transfer.machine.ts | Adds publishTransferCancelled action on TRANSFER_CANCELLED. |
| convex/engine/machines/collectionAttempt.machine.ts | Bumps version and adds initiated→failed transition that increments retryCount. |
| convex/engine/machines/tests/transfer.machine.test.ts | Updates machine tests to expect publishTransferCancelled action. |
| convex/engine/machines/tests/collectionAttempt.test.ts | Updates version assertion and initiated→failed behavior for DRAW_FAILED. |
| convex/engine/effects/transfer.ts | Routes transfer outcomes through attempt reconciliation and adds publishTransferCancelled effect. |
| convex/engine/effects/registry.ts | Registers publishTransferCancelled and scheduleRetryEntry in effect registry. |
| convex/engine/effects/collectionAttempt.ts | Skips legacy bridge transfer when attempt already has transferRequestId; factors shared failure→rules scheduling helper; adds scheduleRetryEntry effect. |
| convex/engine/effects/tests/transfer.test.ts | Updates audit-log registration and timer handling in transfer effect tests. |
| convex/crons.ts | Adds 15-min interval cron for collection plan execution spine. |
| convex/crm/tests/helpers.ts | Tightens typing for default view lookup in CRM test helper. |
| convex/_generated/api.d.ts | Updates generated API typings to include new modules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Connorbelez
changed the base branch from
04-03-active_mortgage_payment_system_re-alignemnt
to
graphite-base/388
Connorbelez
force-pushed
the
04-03-collection_attempt_execution_spine
branch
from
519541a to
11101d1
Compare
Connorbelez
force-pushed
the
graphite-base/388
branch
from
cc58a7c to
3f2a656
Compare
Connorbelez
changed the base branch from
graphite-base/388
to
04-03-active_mortgage_payment_system_re-alignemnt
Connorbelez
force-pushed
the
04-03-collection_attempt_execution_spine
branch
from
11101d1 to
40e4ebe
Compare
![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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
convex/engine/effects/__tests__/transfer.test.ts (1)
275-280:⚠️ Potential issue | 🟡 Minor“All transfer effects” assertion is missing the new cancellation effect.
The test title says all effects are registered, but it does not assert
effectRegistry.publishTransferCancelled. That leaves a regression gap for the cancel path.✅ Suggested patch
describe("transfer effects registry", () => { it("registers all transfer effects", () => { expect(effectRegistry.recordTransferProviderRef).toBeDefined(); expect(effectRegistry.publishTransferConfirmed).toBeDefined(); + expect(effectRegistry.publishTransferCancelled).toBeDefined(); expect(effectRegistry.publishTransferFailed).toBeDefined(); expect(effectRegistry.publishTransferReversed).toBeDefined(); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/engine/effects/__tests__/transfer.test.ts` around lines 275 - 280, The test "registers all transfer effects" is missing an assertion for the new cancellation effect; update the test to assert that effectRegistry.publishTransferCancelled is defined alongside effectRegistry.recordTransferProviderRef, publishTransferConfirmed, publishTransferFailed, and publishTransferReversed so the cancel path is covered and won't regress.convex/engine/effects/transfer.ts (1)
591-641:⚠️ Potential issue | 🟠 MajorShort-circuit transfer-owned reversal after the attempt-linked path claims the transfer.
Line 591 already routes attempt-linked inbound reversals through collection-attempt reconciliation, but Lines 598-625 will still post a transfer-owned reversal whenever a journal exists for the same
transferRequestId. That creates a double-reversal path for historical or partially migrated rows that have bothcollectionAttemptIdand a transfer journal. Once the attempt-linked reconciliation applies, this effect should skip the transfer-owned reversal branch entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/engine/effects/transfer.ts` around lines 591 - 641, After calling reconcileAttemptLinkedInboundReversal, short-circuit the transfer-owned reversal logic so you don't post a second reversal for transfers claimed by the collection-attempt path: immediately after await reconcileAttemptLinkedInboundReversal(ctx, {...}) check whether the attempt-linked path handled/claimed the transfer (e.g., transfer.collectionAttemptId is truthy or have reconcileAttemptLinkedInboundReversal return a boolean “handled” and check that) and if so return/skip the subsequent originalEntry/postTransferReversal branch; this prevents double-reversals even when a cash journal row exists for the same transferRequestId.convex/engine/effects/collectionAttempt.ts (1)
178-195:⚠️ Potential issue | 🟡 MinorGuard against empty
obligationIdsbefore accessing index[0].Line 180 accesses
planEntry.obligationIds[0]without first verifying the array is non-empty. AlthoughemitPaymentReceivediterates the array earlier (line 129), if all obligations are skipped due tocontinuestatements,remainingAmount > 0may still be true while the array could conceptually be empty (edge case with schema invariants). A defensive check improves robustness.🛡️ Proposed defensive check
// Route any remaining overpayment to UNAPPLIED_CASH if (remainingAmount > 0) { - const firstObligation = await ctx.db.get(planEntry.obligationIds[0]); + const firstObligationId = planEntry.obligationIds[0]; + if (!firstObligationId) { + console.warn( + `[emitPaymentReceived] Overpayment of ${remainingAmount} cents but no obligationIds on plan entry. attempt=${args.entityId}` + ); + return; + } + const firstObligation = await ctx.db.get(firstObligationId); if (firstObligation) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/engine/effects/collectionAttempt.ts` around lines 178 - 195, The code in emitPaymentReceived accesses planEntry.obligationIds[0] without confirming the array contains entries; add a defensive guard before calling ctx.db.get to ensure planEntry.obligationIds exists and has length > 0 (or grab const firstObligationId = planEntry.obligationIds?.[0] and check it) and only call ctx.db.get(firstObligationId) and postOverpaymentToUnappliedCash when a valid id is present; otherwise log the warning/error path and skip posting to UNAPPLIED_CASH to avoid undefined index access.
🧹 Nitpick comments (5)
convex/payments/transfers/mutations.ts (1)
383-389: UpdatepersistProviderRefdocblock to match current behavior.The mutation is no longer immediate-confirm-only, so its comment should be updated to avoid future maintenance drift.
📝 Suggested doc-comment update
- * Patches providerRef onto a transfer record. Used by the immediate-confirm - * path in initiateTransfer where the state machine transition (FUNDS_SETTLED) - * does not fire the recordTransferProviderRef effect. + * Patches `providerRef` onto a transfer record immediately after provider + * initiation, before firing transfer transitions. Used for both confirmed and + * pending initiation outcomes to preserve provider correlation.🤖 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 383 - 389, The docblock for the mutation persistProviderRef no longer matches its behavior (it's not immediate-confirm-only); update the comment above the persistProviderRef mutation to accurately describe current behavior, inputs (transferId, providerRef), side effects (persisting provider reference to the transfer record), when it is safe to call (e.g., used in multiple flows, not limited to immediate confirmation), and any invariants or error conditions that callers should expect so future readers won't assume the old immediate-confirm-only semantics.convex/payments/transfers/reconciliation.ts (1)
120-120: Thedirectionparameter is declared but not directly used in this function.The
directionfield is added to the transfer input type but the function body doesn't reference it directly. It's likely consumed byisAttemptLinkedInboundTransfer()which receives the full transfer object. Consider adding a brief inline comment clarifying thatdirectionis inspected by the helper functions for readers unfamiliar with the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/transfers/reconciliation.ts` at line 120, The parameter "direction" is declared on the transfer input type but not referenced directly in the function body; add a short inline comment near the declaration explaining that "direction" is intentionally present because helper functions like isAttemptLinkedInboundTransfer(transfer) and other reconciliation helpers inspect this field on the transfer object rather than the outer function using it directly—this clarifies intent for readers and prevents accidental removal of the field.convex/payments/collectionPlan/execution.ts (1)
239-256: Recovered transfer links should reuse the canonical handoff-success path.Lines 251-255 patch
transferRequestIddirectly, so replay recovery skips theproviderStatus: "transfer_requested"update and the audit record emitted byrecordTransferHandoffSuccess. The link gets healed, but the recovered attempt no longer matches the normal handoff-success path for metadata/observability. Please route this through shared success-handling logic instead of a bare patch.Based on learnings: Applies to /convex//*.{ts,tsx} : Implement auditability from the ground floor - every state transition must be journaled, hash-chained, and queryable for 5-year regulatory retention.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/collectionPlan/execution.ts` around lines 239 - 256, The recovery path currently patches transferRequestId directly on existingAttempt, skipping the normal handoff-success journal and providerStatus update; instead, after finding existingTransfer set recoveredTransferRequestId = existingTransfer._id and call the shared success handler (e.g. recordTransferHandoffSuccess) with the attempt id and transfer id (and ctx as needed) so that providerStatus is set to "transfer_requested" and the audit record is emitted; remove the direct ctx.db.patch call and ensure recoveredTransferRequestId is updated from the handler result if it returns one.convex/payments/collectionPlan/defaultRules.ts (1)
265-276: Verify intentional priority override on insert.Line 272 explicitly sets
priority: ruleDef.priority, which overwrites thepatch.priorityvalue (line 239). For new inserts this is fine, but ensure this is intentional since the spread ofpatchalready contains a potentially different priority value.♻️ Optional: Remove redundant priority assignment
If the intent is to always use
ruleDef.priorityfor new rules, thepatch.priorityassignment on line 239 could be simplified to always useruleDef.priorityfor clarity, or remove line 272:const now = Date.now(); const ruleId = await ctx.db.insert("collectionRules", { ...patch, createdAt: now, updatedAt: now, createdByActorId: SYSTEM_RULE_ACTOR_ID, updatedByActorId: SYSTEM_RULE_ACTOR_ID, - priority: ruleDef.priority, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/collectionPlan/defaultRules.ts` around lines 265 - 276, The insert currently overrides patch.priority with ruleDef.priority (in the insert to collectionRules) which can be redundant or confusing; pick one consistent source of truth and apply it: either remove the explicit priority: ruleDef.priority from the insert and rely on patch.priority (set earlier), or set patch.priority = ruleDef.priority when building patch so the value is consistent and the insert can simply spread patch; update references around patch, ruleDef, and the collectionRules insert accordingly to keep priority assignment single-sourced (you may also remove the redundant assignment to ruleDef.priority or patch.priority depending on which behavior you want).convex/payments/transfers/__tests__/collectionAttemptReconciliation.integration.test.ts (1)
273-278: Consider usingbeforeAll/afterAllfor timer setup.The current pattern calls
vi.useFakeTimers()at module level (line 273) and cleans up inafterAll. This works but could be clearer with both setup and teardown in lifecycle hooks.♻️ Optional: Move timer setup to beforeAll
+beforeAll(() => { + vi.useFakeTimers(); +}); + -vi.useFakeTimers(); - afterAll(() => { vi.clearAllTimers(); vi.useRealTimers(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/transfers/__tests__/collectionAttemptReconciliation.integration.test.ts` around lines 273 - 278, The test file currently calls vi.useFakeTimers() at module scope and relies on afterAll to clean up; move the timer setup into a beforeAll lifecycle to mirror the existing afterAll teardown for clarity and symmetry: replace the top-level vi.useFakeTimers() call with a beforeAll that calls vi.useFakeTimers(), keep the existing afterAll that calls vi.clearAllTimers() and vi.useRealTimers(), and ensure the hooks wrap the same tests that rely on fake timers (referencing vi.useFakeTimers, beforeAll, afterAll, vi.clearAllTimers, vi.useRealTimers).
🤖 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/ruleContract.ts`:
- Around line 151-165: The legacy helper readLegacyNumberParameter currently
accepts any numeric value; change it to validate that the numeric property is an
integer and non-negative (use Number.isInteger(value) && value >= 0) for fields
like delayDays, backoffBaseDays, and maxRetries, and otherwise return undefined
so the materializer falls back to defaults; locate readLegacyNumberParameter and
add the integer/non-negative guard around (parameters as Record<string,
unknown>)[key] before returning the value.
- Around line 167-184: getCollectionRuleKind currently prefers rule.kind and can
ignore a conflicting rule.config.kind; update getCollectionRuleKind to detect
mismatched kind pairs and fail fast: if both rule.kind and rule.config?.kind
exist and are not equal, return null (or throw) instead of picking one, so
downstream typed helpers (which rely on config.kind) won't be dispatched
incorrectly; implement this equality check inside getCollectionRuleKind and
ensure callers handle the null/throw case (or prefer throwing to surface invalid
rows for validation before persistence).
In `@convex/payments/collectionPlan/runner.ts`:
- Around line 59-96: The loop over dueEntries should not let a single
executePlanEntry throw abort the batch: wrap the per-entry call to
ctx.runAction(internal.payments.collectionPlan.execution.executePlanEntry, ...)
in a try/catch inside the for (const entry of dueEntries) loop, log the
entry._id (planEntryId) and the caught error via the existing logger so you can
trace the failing row, and update the summary (e.g., add an attempted or failure
counter) as appropriate before continuing to the next entry; ensure
buildSchedulerExecutionIdempotencyKey and requested metadata are still passed
unchanged and that any partial result handling (switch on result.outcome) only
runs when the call succeeds.
In `@convex/payments/transfers/__tests__/reconciliation.test.ts`:
- Around line 89-105: The two tests currently assert only the input shape
(Boolean(transfer.collectionAttemptId)) which is tautological; replace those
assertions with calls to the actual reconciliation/classification function used
by the codebase (e.g., classifyTransferHealth, isTransferHealthy, or
needsJournalChecks) and assert the expected reconciliation decision: for the
"collectionAttemptId alone no longer proves..." test call the classifier with
the transfer
`{status:"confirmed",direction:"inbound",collectionAttemptId:"attempt_001"}` and
assert it does NOT mark the transfer as healthy solely due to
collectionAttemptId (e.g., returns false for isTransferHealthy or returns a
decision like "needsJournalCheck"); for the "non-attempt-linked confirmed
transfers..." test call the same classifier with `collectionAttemptId:
undefined` and assert it requires/relies on transfer-owned journal checks (e.g.,
returns true for needsJournalChecks or the corresponding enum/flag). Ensure you
import the actual classifier used by reconciliation and replace the tautological
Boolean assertions with assertions against that function's output.
In `@convex/seed/seedPaymentData.ts`:
- Around line 102-113: The returned object exposes a top-level planEntryIds that
only contains created IDs while reused.planEntryIds holds reused IDs, but
callers never consume the top-level field; either remove the unused top-level
planEntryIds or rename it to createdPlanEntryIds for clarity and update related
types. Update the return in seedPaymentData (references: planEntryIds,
schedulingResult.createdPlanEntryIds, schedulingResult.reusedPlanEntryIds,
reused.planEntryIds) to either delete the top-level planEntryIds and add
reused.planEntryIds to the seedAll.ts interface, or rename planEntryIds to
createdPlanEntryIds and adjust the seedAll.ts interface and any call sites to
use createdPlanEntryIds and reused.planEntryIds consistently. Ensure the
schedulingResult usages and the generated/reused structure remain correct after
the rename/removal.
In `@src/test/convex/payments/helpers.ts`:
- Around line 61-73: The current code uses rules.find(...) which silently picks
an arbitrary match if duplicates exist; change each lookup to collect all
matches (e.g., use rules.filter(rule => (rule.code ?? rule.name) ===
"schedule_rule")) and assert the result length is exactly 1 before extracting
the single _id; do the same for "retry_rule" and "late_fee_rule" (replace
scheduleRuleId/retryRuleId/lateFeeRuleId assignments), and throw a clear error
if count !== 1 so the test fails when duplicates are present.
---
Outside diff comments:
In `@convex/engine/effects/__tests__/transfer.test.ts`:
- Around line 275-280: The test "registers all transfer effects" is missing an
assertion for the new cancellation effect; update the test to assert that
effectRegistry.publishTransferCancelled is defined alongside
effectRegistry.recordTransferProviderRef, publishTransferConfirmed,
publishTransferFailed, and publishTransferReversed so the cancel path is covered
and won't regress.
In `@convex/engine/effects/collectionAttempt.ts`:
- Around line 178-195: The code in emitPaymentReceived accesses
planEntry.obligationIds[0] without confirming the array contains entries; add a
defensive guard before calling ctx.db.get to ensure planEntry.obligationIds
exists and has length > 0 (or grab const firstObligationId =
planEntry.obligationIds?.[0] and check it) and only call
ctx.db.get(firstObligationId) and postOverpaymentToUnappliedCash when a valid id
is present; otherwise log the warning/error path and skip posting to
UNAPPLIED_CASH to avoid undefined index access.
In `@convex/engine/effects/transfer.ts`:
- Around line 591-641: After calling reconcileAttemptLinkedInboundReversal,
short-circuit the transfer-owned reversal logic so you don't post a second
reversal for transfers claimed by the collection-attempt path: immediately after
await reconcileAttemptLinkedInboundReversal(ctx, {...}) check whether the
attempt-linked path handled/claimed the transfer (e.g.,
transfer.collectionAttemptId is truthy or have
reconcileAttemptLinkedInboundReversal return a boolean “handled” and check that)
and if so return/skip the subsequent originalEntry/postTransferReversal branch;
this prevents double-reversals even when a cash journal row exists for the same
transferRequestId.
---
Nitpick comments:
In `@convex/payments/collectionPlan/defaultRules.ts`:
- Around line 265-276: The insert currently overrides patch.priority with
ruleDef.priority (in the insert to collectionRules) which can be redundant or
confusing; pick one consistent source of truth and apply it: either remove the
explicit priority: ruleDef.priority from the insert and rely on patch.priority
(set earlier), or set patch.priority = ruleDef.priority when building patch so
the value is consistent and the insert can simply spread patch; update
references around patch, ruleDef, and the collectionRules insert accordingly to
keep priority assignment single-sourced (you may also remove the redundant
assignment to ruleDef.priority or patch.priority depending on which behavior you
want).
In `@convex/payments/collectionPlan/execution.ts`:
- Around line 239-256: The recovery path currently patches transferRequestId
directly on existingAttempt, skipping the normal handoff-success journal and
providerStatus update; instead, after finding existingTransfer set
recoveredTransferRequestId = existingTransfer._id and call the shared success
handler (e.g. recordTransferHandoffSuccess) with the attempt id and transfer id
(and ctx as needed) so that providerStatus is set to "transfer_requested" and
the audit record is emitted; remove the direct ctx.db.patch call and ensure
recoveredTransferRequestId is updated from the handler result if it returns one.
In
`@convex/payments/transfers/__tests__/collectionAttemptReconciliation.integration.test.ts`:
- Around line 273-278: The test file currently calls vi.useFakeTimers() at
module scope and relies on afterAll to clean up; move the timer setup into a
beforeAll lifecycle to mirror the existing afterAll teardown for clarity and
symmetry: replace the top-level vi.useFakeTimers() call with a beforeAll that
calls vi.useFakeTimers(), keep the existing afterAll that calls
vi.clearAllTimers() and vi.useRealTimers(), and ensure the hooks wrap the same
tests that rely on fake timers (referencing vi.useFakeTimers, beforeAll,
afterAll, vi.clearAllTimers, vi.useRealTimers).
In `@convex/payments/transfers/mutations.ts`:
- Around line 383-389: The docblock for the mutation persistProviderRef no
longer matches its behavior (it's not immediate-confirm-only); update the
comment above the persistProviderRef mutation to accurately describe current
behavior, inputs (transferId, providerRef), side effects (persisting provider
reference to the transfer record), when it is safe to call (e.g., used in
multiple flows, not limited to immediate confirmation), and any invariants or
error conditions that callers should expect so future readers won't assume the
old immediate-confirm-only semantics.
In `@convex/payments/transfers/reconciliation.ts`:
- Line 120: The parameter "direction" is declared on the transfer input type but
not referenced directly in the function body; add a short inline comment near
the declaration explaining that "direction" is intentionally present because
helper functions like isAttemptLinkedInboundTransfer(transfer) and other
reconciliation helpers inspect this field on the transfer object rather than the
outer function using it directly—this clarifies intent for readers and prevents
accidental removal of the field.
🪄 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: 4b74d56b-cbc5-4fb6-8597-2493259a1f44
⛔ Files ignored due to path filters (1)
convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (74)
convex/crm/__tests__/helpers.tsconvex/crons.tsconvex/engine/effects/__tests__/transfer.test.tsconvex/engine/effects/collectionAttempt.tsconvex/engine/effects/registry.tsconvex/engine/effects/transfer.tsconvex/engine/machines/__tests__/collectionAttempt.test.tsconvex/engine/machines/__tests__/transfer.machine.test.tsconvex/engine/machines/collectionAttempt.machine.tsconvex/engine/machines/transfer.machine.tsconvex/engine/transition.tsconvex/payments/__tests__/methods.test.tsconvex/payments/__tests__/rules.test.tsconvex/payments/cashLedger/__tests__/transferReconciliation.test.tsconvex/payments/cashLedger/transferReconciliation.tsconvex/payments/collectionPlan/__tests__/engine.test.tsconvex/payments/collectionPlan/__tests__/execution.test.tsconvex/payments/collectionPlan/__tests__/initialScheduling.test.tsconvex/payments/collectionPlan/__tests__/runner.test.tsconvex/payments/collectionPlan/defaultRules.tsconvex/payments/collectionPlan/engine.tsconvex/payments/collectionPlan/execution.tsconvex/payments/collectionPlan/executionContract.tsconvex/payments/collectionPlan/initialScheduling.tsconvex/payments/collectionPlan/mutations.tsconvex/payments/collectionPlan/queries.tsconvex/payments/collectionPlan/ruleContract.tsconvex/payments/collectionPlan/rules/lateFeeRule.tsconvex/payments/collectionPlan/rules/retryRule.tsconvex/payments/collectionPlan/rules/scheduleRule.tsconvex/payments/collectionPlan/runner.tsconvex/payments/collectionPlan/seed.tsconvex/payments/methods/interface.tsconvex/payments/methods/manual.tsconvex/payments/methods/mockPAD.tsconvex/payments/methods/registry.tsconvex/payments/transfers/__tests__/collectionAttemptReconciliation.integration.test.tsconvex/payments/transfers/__tests__/handlers.integration.test.tsconvex/payments/transfers/__tests__/reconciliation.test.tsconvex/payments/transfers/__tests__/transferMachine.test.tsconvex/payments/transfers/collectionAttemptReconciliation.tsconvex/payments/transfers/mutations.tsconvex/payments/transfers/providers/__tests__/adapter.test.tsconvex/payments/transfers/providers/adapter.tsconvex/payments/transfers/reconciliation.tsconvex/schema.tsconvex/seed/seedPaymentData.tsdocs/architecture/unified-payment-rails-technical-design.mddocs/technical-design/unified-payment-rails.mdspecs/03-implement-collection-plan-collection-attempt-execution-spine/PRD.mdspecs/03-implement-collection-plan-collection-attempt-execution-spine/design.mdspecs/03-implement-collection-plan-collection-attempt-execution-spine/gap-analysis.mdspecs/03-implement-collection-plan-collection-attempt-execution-spine/tasks.mdspecs/04-reconcile-collection-attempts-with-transfer-execution-and-cash-posting/PRD.mdspecs/04-reconcile-collection-attempts-with-transfer-execution-and-cash-posting/design.mdspecs/04-reconcile-collection-attempts-with-transfer-execution-and-cash-posting/gap-analysis.mdspecs/04-reconcile-collection-attempts-with-transfer-execution-and-cash-posting/tasks.mdspecs/05-converge-inbound-provider-boundary-on-transferprovider/PRD.mdspecs/05-converge-inbound-provider-boundary-on-transferprovider/design.mdspecs/05-converge-inbound-provider-boundary-on-transferprovider/gap-analysis.mdspecs/05-converge-inbound-provider-boundary-on-transferprovider/tasks.mdspecs/06-correct-activation-and-initial-scheduling-handoff/PRD.mdspecs/06-correct-activation-and-initial-scheduling-handoff/design.mdspecs/06-correct-activation-and-initial-scheduling-handoff/gap-analysis.mdspecs/06-correct-activation-and-initial-scheduling-handoff/tasks.mdspecs/07-expand-collection-rule-model-and-complete-retry-late-fee-behaviors/PRD.mdspecs/07-expand-collection-rule-model-and-complete-retry-late-fee-behaviors/design.mdspecs/07-expand-collection-rule-model-and-complete-retry-late-fee-behaviors/gap-analysis.mdspecs/07-expand-collection-rule-model-and-complete-retry-late-fee-behaviors/tasks.mdsrc/test/convex/engine/transition.test.tssrc/test/convex/payments/crossEntity.test.tssrc/test/convex/payments/endToEnd.test.tssrc/test/convex/payments/helpers.tssrc/test/convex/seed/seedPaymentData.test.ts
Owner
Author
Merge activity
|
Connorbelez
changed the base branch from
04-03-active_mortgage_payment_system_re-alignemnt
to
graphite-base/388
- add canonical initial scheduling and default collection rules - route transfer confirm/fail/cancel/reverse events through attempt reconciliation - update payment method compatibility and reconciliation coverage
- Introduce typed rule contract, engine dispatch, and rule config helpers - Backfill and seed collection rules with canonical metadata - Add tests for deterministic selection and legacy rule migration
Connorbelez
force-pushed
the
04-03-collection_attempt_execution_spine
branch
from
6560585 to
c434a3f
Compare
This was referenced Apr 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
collection attempt execution spine
Link transfer execution to collection attempt reconciliation
Add typed collection-rule engine and seed migration
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements