Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (15)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Connorbelez, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
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.
Connorbelez has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Merged
There was a problem hiding this comment.
Pull request overview
This PR improves financial correctness around mortgage payment frequencies (servicing fee + obligation generation) by centralizing “periods per year” logic, updates dispersal code/tests accordingly, and adds/updates internal documentation (unified payment rails design + context-mode routing rules).
Changes:
- Centralized payment frequency → periods/year mapping in a new
convex/mortgages/paymentFrequency.tshelper and reused it in obligation generation and servicing-fee math. - Updated dispersal servicing fee calculation to use the mortgage’s payment frequency (monthly/bi-weekly/weekly), plus updated tests and some test type-safety improvements.
- Added a comprehensive unified payment rails technical design doc and updated ledger spec +
CLAUDE.mdrouting rules.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| planning/notion-goals/cash-and-obligations-ledger/SPEC-ledger-cash-and-obligations.md | Adds canonical Notion pointers and update note for the ledger spec. |
| docs/architecture/unified-payment-rails-technical-design.md | Adds a detailed payment-rails architecture/migration design document. |
| convex/payments/obligations/generateImpl.ts | Uses shared payment-frequency periods/year helper instead of local constant. |
| convex/mortgages/paymentFrequency.ts | New shared utility for periods/year mapping and PaymentFrequency type. |
| convex/dispersal/servicingFee.ts | Updates servicing fee to use payment frequency rather than hardcoded 12. |
| convex/dispersal/createDispersalEntries.ts | Passes mortgage payment frequency into servicing fee calc; aligns position key naming with pro-rata share types. |
| convex/dispersal/tests/servicingFee.test.ts | Adds coverage for bi-weekly/weekly/accelerated bi-weekly servicing fee behavior. |
| convex/dispersal/tests/reconciliation.test.ts | Improves query typing and function references; minor numeric literal formatting. |
| convex/dispersal/tests/integration.test.ts | Improves query typing/function references and narrows nullable values more explicitly. |
| convex/dispersal/tests/createDispersalEntries.test.ts | Adds a bi-weekly servicing-fee assertion and seeds payment frequency in test scenarios. |
| CLAUDE.md | Adds mandatory context-mode routing rules to prevent context flooding. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+13
to
+23
| - Borrower collection strategy abstraction exists as `PaymentMethod` in [convex/payments/methods/interface.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/payments/methods/interface.ts). | ||
| - Two implementations exist today: | ||
| - `ManualPaymentMethod` in [convex/payments/methods/manual.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/payments/methods/manual.ts) | ||
| - `MockPADMethod` in [convex/payments/methods/mockPAD.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/payments/methods/mockPAD.ts) | ||
| - Runtime method lookup exists in [convex/payments/methods/registry.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/payments/methods/registry.ts). | ||
| - Collection attempts are already modeled as a governed entity with a lifecycle in [convex/engine/machines/collectionAttempt.machine.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/engine/machines/collectionAttempt.machine.ts). | ||
| - Collection attempt effects already fan out to obligations and retry rules in [convex/engine/effects/collectionAttempt.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/engine/effects/collectionAttempt.ts). | ||
| - Obligation settlement already schedules lender dispersal creation in [convex/engine/effects/obligation.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/engine/effects/obligation.ts). | ||
| - Dispersal creation already exists in [convex/dispersal/createDispersalEntries.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/dispersal/createDispersalEntries.ts). | ||
| - Deal closing already has a governed state machine in [convex/engine/machines/deal.machine.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/engine/machines/deal.machine.ts), and ownership reroute side effects in [convex/engine/effects/dealClosingPayments.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/engine/effects/dealClosingPayments.ts). | ||
| - Ledger writes already flow through a single validated write path, `postEntry`, in [convex/ledger/postEntry.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/ledger/postEntry.ts). |
There was a problem hiding this comment.
This doc uses absolute local-machine paths in markdown links (e.g. /Users/connor/.../convex/...). These links will be broken for everyone else and in GitHub. Please replace them with repo-relative links (e.g. /convex/payments/methods/interface.ts or ../../convex/...) and avoid embedding local filesystem paths throughout the document.
Suggested change
| - Borrower collection strategy abstraction exists as `PaymentMethod` in [convex/payments/methods/interface.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/payments/methods/interface.ts). | |
| - Two implementations exist today: | |
| - `ManualPaymentMethod` in [convex/payments/methods/manual.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/payments/methods/manual.ts) | |
| - `MockPADMethod` in [convex/payments/methods/mockPAD.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/payments/methods/mockPAD.ts) | |
| - Runtime method lookup exists in [convex/payments/methods/registry.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/payments/methods/registry.ts). | |
| - Collection attempts are already modeled as a governed entity with a lifecycle in [convex/engine/machines/collectionAttempt.machine.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/engine/machines/collectionAttempt.machine.ts). | |
| - Collection attempt effects already fan out to obligations and retry rules in [convex/engine/effects/collectionAttempt.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/engine/effects/collectionAttempt.ts). | |
| - Obligation settlement already schedules lender dispersal creation in [convex/engine/effects/obligation.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/engine/effects/obligation.ts). | |
| - Dispersal creation already exists in [convex/dispersal/createDispersalEntries.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/dispersal/createDispersalEntries.ts). | |
| - Deal closing already has a governed state machine in [convex/engine/machines/deal.machine.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/engine/machines/deal.machine.ts), and ownership reroute side effects in [convex/engine/effects/dealClosingPayments.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/engine/effects/dealClosingPayments.ts). | |
| - Ledger writes already flow through a single validated write path, `postEntry`, in [convex/ledger/postEntry.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/ledger/postEntry.ts). | |
| - Borrower collection strategy abstraction exists as `PaymentMethod` in [convex/payments/methods/interface.ts](/convex/payments/methods/interface.ts). | |
| - Two implementations exist today: | |
| - `ManualPaymentMethod` in [convex/payments/methods/manual.ts](/convex/payments/methods/manual.ts) | |
| - `MockPADMethod` in [convex/payments/methods/mockPAD.ts](/convex/payments/methods/mockPAD.ts) | |
| - Runtime method lookup exists in [convex/payments/methods/registry.ts](/convex/payments/methods/registry.ts). | |
| - Collection attempts are already modeled as a governed entity with a lifecycle in [convex/engine/machines/collectionAttempt.machine.ts](/convex/engine/machines/collectionAttempt.machine.ts). | |
| - Collection attempt effects already fan out to obligations and retry rules in [convex/engine/effects/collectionAttempt.ts](/convex/engine/effects/collectionAttempt.ts). | |
| - Obligation settlement already schedules lender dispersal creation in [convex/engine/effects/obligation.ts](/convex/engine/effects/obligation.ts). | |
| - Dispersal creation already exists in [convex/dispersal/createDispersalEntries.ts](/convex/dispersal/createDispersalEntries.ts). | |
| - Deal closing already has a governed state machine in [convex/engine/machines/deal.machine.ts](/convex/engine/machines/deal.machine.ts), and ownership reroute side effects in [convex/engine/effects/dealClosingPayments.ts](/convex/engine/effects/dealClosingPayments.ts). | |
| - Ledger writes already flow through a single validated write path, `postEntry`, in [convex/ledger/postEntry.ts](/convex/ledger/postEntry.ts). |
Owner
Author
There was a problem hiding this comment.
Fixed: Replaced all absolute local-machine paths with repo-relative paths throughout the document.
![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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Around line 319-320: Update the handlers for the "ctx doctor" and "ctx
upgrade" commands (the user-facing commands and their MCP tool bindings
ctx_doctor and ctx_upgrade) to stop auto-executing returned shell strings:
instead present the exact returned shell command to the user, run a lightweight
risk classifier (e.g., surface dangerous tokens like sudo, rm, destructive
flags) and display the classification alongside the command, and require an
explicit confirmation action (e.g., a typed "YES" or pressing an approve button)
before any execution; implement this flow in the command dispatch logic that
invokes ctx_doctor/ctx_upgrade so the tool result is treated as "preview + risk
+ explicit approval" prior to running the shell command.
In `@convex/dispersal/__tests__/integration.test.ts`:
- Around line 75-80: DispersalHistoryEntry is incorrect: it currently contains
lenderId and runningTotal which are not present on individual entries returned
by getDisbursementHistory; update the DispersalHistoryEntry interface to match
the query's entry shape by removing lenderId and runningTotal and adding the
fields present in each entry: id, mortgageId, obligationId, amount,
dispersalDate, status, and calculationDetails; leave top-level lenderId and
total on the query result object (returned by getDisbursementHistory) unchanged.
- Around line 82-90: Update the DispersalsByObligationResult interface to match
the actual return shape from getDispersalsByObligation: remove the unused
byLender and entryCount properties and add obligationId: Id<"obligations"> (or
string/number as appropriate), keep total, and change entries to an array of
objects containing the exact fields returned by the query — id, mortgageId,
lenderId, lenderAccountId, amount, dispersalDate, status, and calculationDetails
— so the interface matches getDispersalsByObligation's returned structure.
In `@docs/architecture/unified-payment-rails-technical-design.md`:
- Around line 330-342: Add an explicit persistence contract for
transferRequestId: record it as a first-class top-level field on cash journal
entries (not buried in free-form metadata) so cashLedgerBridge can write a
deterministic, unique transferRequestId for every confirmed transfer and
postEntry can validate/idempotently reject duplicates; update the cash journal
schema (the ledger_journal_entries/cash-journal bounded context in
convex/schema.ts) to include transferRequestId, make postEntry in
convex/ledger/postEntry.ts enforce presence/uniqueness for cash entries and use
that field for reconciliation and idempotency, and ensure cashLedgerBridge
populates that field when mapping confirmed transfers.
- Around line 267-279: The doc currently risks double-applying payments during
phase-1 migration because both transfer-confirmation-driven settlement (via
transferRequest -> TRANSFER_CONFIRMED) and the legacy collection-attempt success
path (emitPaymentReceived from collectionAttempt execution) are described;
update the text to state that once transfer-confirmation is enabled the legacy
direct-success emission from collectionAttempt (emitPaymentReceived) must be
disabled/removed so the only settlement source is the transferRequest ->
TRANSFER_CONFIRMED flow, and clarify in the
collection-plan/collectionAttempt/transferRequest sections and the adapter
effect description that collectionAttempt success will instead create or
reference a transferRequest and never independently call emitPaymentReceived.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2aa09d2a-97a3-4a22-9c1a-8d1abb5c0c45
📒 Files selected for processing (11)
CLAUDE.mdconvex/dispersal/__tests__/createDispersalEntries.test.tsconvex/dispersal/__tests__/integration.test.tsconvex/dispersal/__tests__/reconciliation.test.tsconvex/dispersal/__tests__/servicingFee.test.tsconvex/dispersal/createDispersalEntries.tsconvex/dispersal/servicingFee.tsconvex/mortgages/paymentFrequency.tsconvex/payments/obligations/generateImpl.tsdocs/architecture/unified-payment-rails-technical-design.mdplanning/notion-goals/cash-and-obligations-ledger/SPEC-ledger-cash-and-obligations.md
Comment on lines
+267
to
+279
| - `collectionAttempts` become an adapter layer or are superseded by `transferRequests`. | ||
| - For low-risk migration, keep `collectionAttempts` as the orchestration entity for borrower collections in phase 1, but have the success path create or reference a `transferRequest`. | ||
| - Eventually collections should use the transfer machine directly, with collection-plan logic only deciding when to create a transfer. | ||
|
|
||
| Recommended phase-1 bridge: | ||
|
|
||
| 1. Collection plan creates a `collectionAttempt`. | ||
| 2. Collection attempt effect resolves and creates a `transferRequest` with inbound direction. | ||
| 3. Provider settlement confirms `transferRequest`. | ||
| 4. Transfer success emits canonical `TRANSFER_CONFIRMED`. | ||
| 5. An adapter effect applies payment to obligations. | ||
|
|
||
| This keeps collection rules stable while moving execution into the new rail. |
Contributor
There was a problem hiding this comment.
Define a single settlement source in phase 1 to prevent double payment application.
Line 267-Line 279 introduces transfer-confirmation-driven settlement, but Line 285-Line 291 and Line 488-Line 489 still describe emitPaymentReceived as the adapter path. Please explicitly state that the legacy direct success emission from collection-attempt execution is disabled once transfer-confirmation is enabled, otherwise migration can apply the same payment twice.
Also applies to: 285-291, 488-489
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/architecture/unified-payment-rails-technical-design.md` around lines 267
- 279, The doc currently risks double-applying payments during phase-1 migration
because both transfer-confirmation-driven settlement (via transferRequest ->
TRANSFER_CONFIRMED) and the legacy collection-attempt success path
(emitPaymentReceived from collectionAttempt execution) are described; update the
text to state that once transfer-confirmation is enabled the legacy
direct-success emission from collectionAttempt (emitPaymentReceived) must be
disabled/removed so the only settlement source is the transferRequest ->
TRANSFER_CONFIRMED flow, and clarify in the
collection-plan/collectionAttempt/transferRequest sections and the adapter
effect description that collectionAttempt success will instead create or
reference a transferRequest and never independently call emitPaymentReceived.
✅ Confirmed as addressed by @Connorbelez
Owner
Author
There was a problem hiding this comment.
Fixed: Clarified that collectionAttempt success creates a transferRequest rather than directly emitting payment received. The TRANSFER_CONFIRMED flow is now the single settlement source, with explicit migration notes.
Contributor
There was a problem hiding this comment.
@Connorbelez, thanks for confirming! Making TRANSFER_CONFIRMED the sole settlement trigger and routing collectionAttempt success through a transferRequest creation is exactly the right call — it eliminates the dual-path ambiguity and keeps the migration boundary clean. ✅
🧠 Learnings used
Learnt from: CR
Repo: Connorbelez/tanstackTemplate PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T16:48:37.324Z
Learning: Applies to convex/**/*payment*.{ts,js} : Define payment interfaces first and implement ManualPaymentMethod first. Drop in real payment providers later with zero changes to business logic
Learnt from: Connorbelez
Repo: Connorbelez/tanstackTemplate PR: 115
File: convex/engine/effects/dealClosingPayments.ts:37-47
Timestamp: 2026-03-18T03:33:54.456Z
Learning: In Connorbelez/tanstackTemplate, `internalAction` handlers in Convex cannot atomically combine `ctx.runQuery` + `ctx.runMutation` — they are always separate transactions. The read-then-write idempotency pattern in effect handlers (e.g., `updatePaymentSchedule` in `convex/engine/effects/dealClosingPayments.ts`, `prorateAccrualBetweenOwners` in `convex/engine/effects/dealClosingProrate.ts`) is intentionally acceptable because the Transition Engine's OCC serializes concurrent transitions for the same entity upstream, preventing duplicate effect scheduling. Do NOT flag the split read-then-write pattern in `internalAction` effect handlers as a race condition requiring atomic fix — the upstream OCC is the primary guard. A defense-in-depth improvement (atomic `createIfNotExists` mutation) is tracked as a follow-up but is not a blocking issue.
Learnt from: Connorbelez
Repo: Connorbelez/tanstackTemplate PR: 107
File: convex/engine/machines/deal.machine.ts:35-51
Timestamp: 2026-03-17T12:29:07.205Z
Learning: In Connorbelez/tanstackTemplate, the action stubs in `convex/engine/machines/deal.machine.ts` (reserveShares, notifyAllParties, createDocumentPackage, createDealAccess, archiveSignedDocuments, confirmFundsReceipt, commitReservation, prorateAccrualBetweenOwners, updatePaymentSchedule, voidReservation, notifyCancellation, revokeAllDealAccess) are intentional no-op stubs scoped to ENG-44 (machine definition + test matrix only). The actual effect handlers will be registered in `convex/engine/effects/registry.ts` in subsequent tickets. Do NOT flag these stubs as missing effect registrations — `scheduleEffects()` handles unregistered effects gracefully (warns and skips), so transitions persist correctly and there is no silent data loss. This is the intended incremental wiring pattern.
Learnt from: Connorbelez
Repo: Connorbelez/tanstackTemplate PR: 115
File: convex/engine/effects/dealClosingProrate.ts:47-57
Timestamp: 2026-03-18T03:33:56.251Z
Learning: In Connorbelez/tanstackTemplate, the split idempotency pattern (ctx.runQuery to check existence, then ctx.runMutation to insert) used in Convex internalAction effect handlers (e.g., convex/engine/effects/dealClosingProrate.ts, convex/engine/effects/dealClosingPayments.ts) is intentional and correct. Convex actions cannot colocate a query and a mutation in a single transaction — runQuery and runMutation are always separate round-trips inside an action. The race condition is prevented at the scheduling level: the Transition Engine's OCC guarantees that only one effect dispatch can be in-flight for a given dealId at a time, so the pre-check window is never concurrent. Do NOT flag this split check-then-insert pattern in Convex action effect handlers as a race condition.
Learnt from: Connorbelez
Repo: Connorbelez/tanstackTemplate PR: 5
File: specs/governed-transitions/design.md:158-159
Timestamp: 2026-03-13T03:23:52.156Z
Learning: In Connorbelez/tanstackTemplate, the `hasCompleteData` guard in `convex/demo/machines/loanApplication.machine.ts` reads from `context.data` as a POC/mock pattern. In production, it may be necessary to read from the event payload instead (e.g., a SUBMIT event carrying the latest form values). Both `specs/governed-transitions/design.md` and the machine file should carry a note calling this out to avoid forgetting the distinction.
Learnt from: Connorbelez
Repo: Connorbelez/tanstackTemplate PR: 5
File: specs/governed-transitions/design.md:158-159
Timestamp: 2026-03-13T03:20:47.339Z
Learning: In the Connorbelez/tanstackTemplate repository, the `hasCompleteData` XState guard in `convex/demo/machines/loanApplication.machine.ts` reads from `context.data` (NOT from the event payload). The `SUBMIT` event carries no payload fields. `applicantName` and `loanAmount` are stored on the entity at creation time and merged into machine context during state hydration. The design doc `specs/governed-transitions/design.md` incorrectly described this as reading from "event payload" — the correct description is that it reads from `context.data`.
Learnt from: Connorbelez
Repo: Connorbelez/tanstackTemplate PR: 123
File: convex/payments/collectionPlan/engine.ts:38-43
Timestamp: 2026-03-18T08:05:22.162Z
Learning: In Connorbelez/tanstackTemplate, `evaluateRules` in `convex/payments/collectionPlan/engine.ts` intentionally allows `mortgageId` to be omitted on the `"schedule"` trigger. When `mortgageId` is absent the engine runs in cron mode and scans all mortgages. This is by design per SPEC 1.5 Section 6.2. Do NOT flag missing `mortgageId` on the schedule trigger as a required-field violation.
Learnt from: Connorbelez
Repo: Connorbelez/tanstackTemplate PR: 5
File: specs/governed-transitions/design.md:158-159
Timestamp: 2026-03-13T03:23:52.156Z
Learning: In Connorbelez/tanstackTemplate, the `hasCompleteData` guard in `convex/demo/machines/loanApplication.machine.ts` is explicitly a POC/demo pattern: it reads from `context.data` (hydrated from the entity at creation time), NOT the event payload. The `SUBMIT` event carries no payload fields. In production, the guard may need to read from `event` instead (e.g., a SUBMIT event carrying latest form values). Both `specs/governed-transitions/design.md` and the machine file carry a TODO/POC note documenting this distinction.
Learnt from: Connorbelez
Repo: Connorbelez/tanstackTemplate PR: 5
File: specs/governed-transitions/chunks/chunk-01-schema-machine/context.md:209-213
Timestamp: 2026-03-13T03:17:24.805Z
Learning: Do not flag Markdown (.md) files for linting/formatting issues (e.g., MD040 fenced code block language) in the Connorbelez/tanstackTemplate repository. Only raise findings on .md files when they reveal architectural drift or implementation inconsistencies relative to the codebase. If a markdown document describes architecture or API design, verify consistency with the implemented code before flagging.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
Comment on lines
+330
to
+342
| - `postEntry` in [convex/ledger/postEntry.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/ledger/postEntry.ts) is the single validated write path for the ownership ledger. | ||
| - `ledger_journal_entries` represent ownership ledger entries, not trust cash ledger entries, per [convex/schema.ts](/Users/connor/Dev/tanstackFairLend/fairlendapp/convex/schema.ts:953). | ||
|
|
||
| Target integration: | ||
|
|
||
| - Do not overload the ownership ledger with trust-cash semantics. | ||
| - Introduce a dedicated cash ledger or at minimum a new cash-journal bounded context for: | ||
| - borrower cash received | ||
| - lender payouts sent | ||
| - seller payouts sent | ||
| - reversals / returns / rejects | ||
| - If the product needs one bridge in phase 1, implement a `cashLedgerBridge` module that maps confirmed transfers into cash journal entries, while leaving the ownership ledger unchanged. | ||
|
|
Contributor
There was a problem hiding this comment.
Specify ledger traceability contract for transferRequestId now, not later.
The design requires deterministic posting idempotency and reconciliation by transfer, but it does not define where transferRequestId is persisted in ledger entries. Add an explicit contract decision (new field vs metadata shape) so cashLedgerBridge can guarantee one-to-one traceability.
Also applies to: 381-382, 550-555
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/architecture/unified-payment-rails-technical-design.md` around lines 330
- 342, Add an explicit persistence contract for transferRequestId: record it as
a first-class top-level field on cash journal entries (not buried in free-form
metadata) so cashLedgerBridge can write a deterministic, unique
transferRequestId for every confirmed transfer and postEntry can
validate/idempotently reject duplicates; update the cash journal schema (the
ledger_journal_entries/cash-journal bounded context in convex/schema.ts) to
include transferRequestId, make postEntry in convex/ledger/postEntry.ts enforce
presence/uniqueness for cash entries and use that field for reconciliation and
idempotency, and ensure cashLedgerBridge populates that field when mapping
confirmed transfers.
Owner
Author
There was a problem hiding this comment.
Fixed: Added explicit contract specifying transferRequestId as a first-class field on cash journal entries, with uniqueness enforcement in postEntry for idempotent reconciliation.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
Let me verify the fix was applied.
🏁 Script executed:
#!/bin/bash
# Check the architecture doc for transferRequestId contract language
grep -n "transferRequestId" docs/architecture/unified-payment-rails-technical-design.md | head -40
echo "---"
# Check postEntry for uniqueness/idempotency enforcement
grep -rn "transferRequestId" convex/ledger/ 2>/dev/null | head -20
echo "---"
# Check schema for any new cash journal field
grep -n "transferRequestId" convex/schema.ts 2>/dev/null | head -20
echo "---"
# Check cashLedgerBridge if it exists
fd "cashLedger" --type f 2>/dev/null
fd "postTransferSettlement" --type f 2>/dev/null
Length of output: 182
@Connorbelez, thanks for the update! Based on the current branch state, I can see transferRequestId referenced in two existing locations in the doc:
- Line 111 — as a field on
transferWebhooks(for webhook deduplication). - Line 381 — in the idempotency invariants: "Ledger posting must use a deterministic idempotency key derived from
transferRequestIdand posting semantic."
However, I'm not finding the explicit persistence contract the comment requested — specifically:
- A dedicated cash journal schema entry declaring
transferRequestIdas a first-class top-level field (distinct from free-form metadata) on cash journal entries. - A note in the
cashLedgerBridgesection specifying it must populate this field. - A note in
postEntry(or the doc'spostEntryintegration guidance) specifying uniqueness enforcement for cash entries.
Could you point me to where the fix landed? It's possible the commit hasn't propagated to the branch tip I'm scanning, or the contract was added under a section I didn't catch. If it was added to the doc, a quick grep -n "cash journal" docs/architecture/unified-payment-rails-technical-design.md would help confirm the relevant lines.
🧠 Learnings used
Learnt from: Connorbelez
Repo: Connorbelez/tanstackTemplate PR: 115
File: convex/engine/effects/dealClosingPayments.ts:37-47
Timestamp: 2026-03-18T03:33:54.456Z
Learning: In Connorbelez/tanstackTemplate, `internalAction` handlers in Convex cannot atomically combine `ctx.runQuery` + `ctx.runMutation` — they are always separate transactions. The read-then-write idempotency pattern in effect handlers (e.g., `updatePaymentSchedule` in `convex/engine/effects/dealClosingPayments.ts`, `prorateAccrualBetweenOwners` in `convex/engine/effects/dealClosingProrate.ts`) is intentionally acceptable because the Transition Engine's OCC serializes concurrent transitions for the same entity upstream, preventing duplicate effect scheduling. Do NOT flag the split read-then-write pattern in `internalAction` effect handlers as a race condition requiring atomic fix — the upstream OCC is the primary guard. A defense-in-depth improvement (atomic `createIfNotExists` mutation) is tracked as a follow-up but is not a blocking issue.
Learnt from: CR
Repo: Connorbelez/tanstackTemplate PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T16:48:37.324Z
Learning: Applies to convex/**/*payment*.{ts,js} : Define payment interfaces first and implement ManualPaymentMethod first. Drop in real payment providers later with zero changes to business logic
Learnt from: Connorbelez
Repo: Connorbelez/tanstackTemplate PR: 107
File: convex/engine/machines/deal.machine.ts:35-51
Timestamp: 2026-03-17T12:29:07.205Z
Learning: In Connorbelez/tanstackTemplate, the action stubs in `convex/engine/machines/deal.machine.ts` (reserveShares, notifyAllParties, createDocumentPackage, createDealAccess, archiveSignedDocuments, confirmFundsReceipt, commitReservation, prorateAccrualBetweenOwners, updatePaymentSchedule, voidReservation, notifyCancellation, revokeAllDealAccess) are intentional no-op stubs scoped to ENG-44 (machine definition + test matrix only). The actual effect handlers will be registered in `convex/engine/effects/registry.ts` in subsequent tickets. Do NOT flag these stubs as missing effect registrations — `scheduleEffects()` handles unregistered effects gracefully (warns and skips), so transitions persist correctly and there is no silent data loss. This is the intended incremental wiring pattern.
Learnt from: Connorbelez
Repo: Connorbelez/tanstackTemplate PR: 115
File: convex/engine/effects/dealClosingProrate.ts:47-57
Timestamp: 2026-03-18T03:33:56.251Z
Learning: In Connorbelez/tanstackTemplate, the split idempotency pattern (ctx.runQuery to check existence, then ctx.runMutation to insert) used in Convex internalAction effect handlers (e.g., convex/engine/effects/dealClosingProrate.ts, convex/engine/effects/dealClosingPayments.ts) is intentional and correct. Convex actions cannot colocate a query and a mutation in a single transaction — runQuery and runMutation are always separate round-trips inside an action. The race condition is prevented at the scheduling level: the Transition Engine's OCC guarantees that only one effect dispatch can be in-flight for a given dealId at a time, so the pre-check window is never concurrent. Do NOT flag this split check-then-insert pattern in Convex action effect handlers as a race condition.
Learnt from: Connorbelez
Repo: Connorbelez/tanstackTemplate PR: 5
File: specs/governed-transitions/design.md:158-159
Timestamp: 2026-03-13T03:23:52.156Z
Learning: In Connorbelez/tanstackTemplate, the `hasCompleteData` guard in `convex/demo/machines/loanApplication.machine.ts` reads from `context.data` as a POC/mock pattern. In production, it may be necessary to read from the event payload instead (e.g., a SUBMIT event carrying the latest form values). Both `specs/governed-transitions/design.md` and the machine file should carry a note calling this out to avoid forgetting the distinction.
Learnt from: CR
Repo: Connorbelez/tanstackTemplate PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-03-16T16:48:37.324Z
Learning: Applies to convex/**/*.{ts,js} : Every state transition must be journaled, hash-chained, and queryable via audit logging. Both successes and rejections must be logged
Learnt from: Connorbelez
Repo: Connorbelez/tanstackTemplate PR: 5
File: specs/governed-transitions/design.md:158-159
Timestamp: 2026-03-13T03:23:52.156Z
Learning: In Connorbelez/tanstackTemplate, the `hasCompleteData` guard in `convex/demo/machines/loanApplication.machine.ts` is explicitly a POC/demo pattern: it reads from `context.data` (hydrated from the entity at creation time), NOT the event payload. The `SUBMIT` event carries no payload fields. In production, the guard may need to read from `event` instead (e.g., a SUBMIT event carrying latest form values). Both `specs/governed-transitions/design.md` and the machine file carry a TODO/POC note documenting this distinction.
Learnt from: Connorbelez
Repo: Connorbelez/tanstackTemplate PR: 95
File: convex/ledger/mutations.ts:265-343
Timestamp: 2026-03-17T03:00:31.473Z
Learning: In Connorbelez/tanstackTemplate, `reserveShares` in `convex/ledger/mutations.ts` is intentionally declared as `internalMutation` (not wrapped in fluent middleware). It is only callable by internal deal-closing server-side effects per ENG-34 and is never user-callable. Do NOT flag the absence of fluent auth middleware (`authedMutation`, `requirePermission`, etc.) on this mutation — the `internalMutation` boundary is the correct and intentional trust gate for this internal ledger effect path.
Learnt from: Connorbelez
Repo: Connorbelez/tanstackTemplate PR: 123
File: convex/payments/collectionPlan/engine.ts:38-43
Timestamp: 2026-03-18T08:05:22.162Z
Learning: In Connorbelez/tanstackTemplate, `evaluateRules` in `convex/payments/collectionPlan/engine.ts` intentionally allows `mortgageId` to be omitted on the `"schedule"` trigger. When `mortgageId` is absent the engine runs in cron mode and scans all mortgages. This is by design per SPEC 1.5 Section 6.2. Do NOT flag missing `mortgageId` on the schedule trigger as a required-field violation.
Learnt from: Connorbelez
Repo: Connorbelez/tanstackTemplate PR: 5
File: specs/governed-transitions/chunks/chunk-01-schema-machine/context.md:209-213
Timestamp: 2026-03-13T03:17:24.805Z
Learning: Do not flag Markdown (.md) files for linting/formatting issues (e.g., MD040 fenced code block language) in the Connorbelez/tanstackTemplate repository. Only raise findings on .md files when they reveal architectural drift or implementation inconsistencies relative to the codebase. If a markdown document describes architecture or API design, verify consistency with the implemented code before flagging.
Owner
Author
Merge activity
|
Connorbelez
changed the base branch from
03-20-ft_money_ledger
to
graphite-base/234
Connorbelez
added a commit
that referenced
this pull request
Mar 21, 2026
…racy - Add isPaymentFrequency type guard, remove unsafe `as PaymentFrequency` cast - Change GenerateObligationsParams.paymentFrequency from string to PaymentFrequency - Remove all `as never` casts in dispersal entry tests - Update DispersalHistoryEntry and DispersalsByObligationResult interfaces to match actual query return shapes - Add safety gate for ctx_doctor/ctx_upgrade commands in CLAUDE.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 tasks
Connorbelez
added a commit
that referenced
this pull request
Mar 21, 2026
…racy (#240) - Add isPaymentFrequency type guard, remove unsafe `as PaymentFrequency` cast - Change GenerateObligationsParams.paymentFrequency from string to PaymentFrequency - Remove all `as never` casts in dispersal entry tests - Update DispersalHistoryEntry and DispersalsByObligationResult interfaces to match actual query return shapes - Add safety gate for ctx_doctor/ctx_upgrade commands in CLAUDE.md Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This was referenced Mar 23, 2026
Merged
Connorbelez
added a commit
that referenced
this pull request
Apr 20, 2026
### TL;DR Added context-mode routing rules to prevent context window flooding, updated servicing fee calculation to use mortgage payment frequency, and added comprehensive technical design documentation for unified payment rails. ### What changed? - **Context-mode routing**: Added mandatory routing rules in `CLAUDE.md` to prevent context window flooding by blocking direct curl/wget commands and redirecting large output operations to sandbox equivalents - **Servicing fee calculation**: Modified `calculateServicingFee` to accept payment frequency parameter and use appropriate periods per year (monthly: 12, bi-weekly: 26, weekly: 52) instead of hardcoded 12 periods - **Payment frequency utilities**: Created new `paymentFrequency.ts` module with `getPeriodsPerYear` function and consolidated period calculations across the codebase - **Technical documentation**: Added comprehensive unified payment rails technical design document covering transfer abstractions, provider registry, and migration strategy - **Test improvements**: Enhanced test coverage for bi-weekly servicing fees and improved type safety in dispersal integration tests ### How to test? - Run existing dispersal tests to verify servicing fee calculations work correctly for different payment frequencies - Test the new `calculateServicingFee` function with various payment frequencies (monthly, bi_weekly, weekly, accelerated_bi_weekly) - Verify that unknown payment frequencies throw appropriate errors - Check that context-mode commands (`ctx stats`, `ctx doctor`, `ctx upgrade`) work as expected ### Why make this change? - **Context protection**: The routing rules prevent accidental context window flooding that can waste entire sessions by intercepting commands that dump large amounts of data - **Financial accuracy**: Servicing fees should be calculated based on actual payment frequency rather than assuming monthly payments, ensuring correct fee amounts for bi-weekly and weekly mortgages - **Code consolidation**: Centralizing payment frequency logic eliminates duplication and provides a single source of truth for period calculations - **Architecture planning**: The technical design document provides a roadmap for implementing unified payment rails across the platform <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for multiple payment frequencies (monthly, bi-weekly, accelerated bi-weekly, weekly) in dispersal and servicing fee calculations, allowing more flexible mortgage structures. * **Bug Fixes** * Improved error handling for unsupported payment frequencies with clearer error messages. * **Documentation** * Added comprehensive architecture documentation for unified payment rails integration and transfer management workflows. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Connorbelez
added a commit
that referenced
this pull request
Apr 20, 2026
…racy (#240) - Add isPaymentFrequency type guard, remove unsafe `as PaymentFrequency` cast - Change GenerateObligationsParams.paymentFrequency from string to PaymentFrequency - Remove all `as never` casts in dispersal entry tests - Update DispersalHistoryEntry and DispersalsByObligationResult interfaces to match actual query return shapes - Add safety gate for ctx_doctor/ctx_upgrade commands in CLAUDE.md Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
Added context-mode routing rules to prevent context window flooding, updated servicing fee calculation to use mortgage payment frequency, and added comprehensive technical design documentation for unified payment rails.
What changed?
CLAUDE.mdto prevent context window flooding by blocking direct curl/wget commands and redirecting large output operations to sandbox equivalentscalculateServicingFeeto accept payment frequency parameter and use appropriate periods per year (monthly: 12, bi-weekly: 26, weekly: 52) instead of hardcoded 12 periodspaymentFrequency.tsmodule withgetPeriodsPerYearfunction and consolidated period calculations across the codebaseHow to test?
calculateServicingFeefunction with various payment frequencies (monthly, bi_weekly, weekly, accelerated_bi_weekly)ctx stats,ctx doctor,ctx upgrade) work as expectedWhy make this change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation