feat(eng-246): org-scoped audit and denormalized orgId on domain tables#341
Merged
Connorbelez merged 2 commits intomainfrom Mar 29, 2026
Merged
feat(eng-246): org-scoped audit and denormalized orgId on domain tables#341Connorbelez merged 2 commits intomainfrom
Connorbelez merged 2 commits intomainfrom
Conversation
- Add convex/lib/orgScope for broker/mortgage/transfer org resolution and audit journal organizationId extraction. - Extend schema with optional orgId on brokers, borrowers, lenders, mortgages, obligations, deals, dispersalEntries, transferRequests; organizationId on auditJournal; rename broker brokerageOrgId to orgId with org indexes. - Thread org through seeds, simulation, obligations, dispersal, transfers, onboarding, and transition audit entries. - Fix transfer internal-action circular typing (explicit return types); use legNumberValidator on principal return. - Split root vs Convex typecheck (exclude convex/ from root tsconfig). Made-with: Cursor
![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
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 45 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR introduces organization-level scoping across the data model and mutation flows. It adds optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~40 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Pull request overview
Adds organization scoping across domain tables and audit journaling by denormalizing orgId (WorkOS org id) onto core Convex tables and threading organizationId through GT/audit entry creation and seed/simulation flows.
Changes:
- Introduces
convex/lib/orgScopehelpers for deriving org scope (especially for transfers) and for extractingorganizationIdfor audit rows. - Extends Convex schema with optional
orgIdon multiple domain tables +organizationIdonauditJournal, including new org-based indexes. - Threads org scope through seeders, simulations, transfer creation, obligation generation/correctives, onboarding, and GT transition audit entries; splits root vs Convex TS typechecking.
Reviewed changes
Copilot reviewed 27 out of 29 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Excludes convex/** from root TS project to split typechecking concerns. |
| package.json | Runs root typecheck plus dedicated Convex project typecheck. |
| specs/ENG-20/chunks/chunk-01-foundation-and-profiles/status.md | Updates spec note to reflect brokers.orgId rename. |
| specs/ENG-20/chunks/chunk-01-foundation-and-profiles/context.md | Updates schema/spec references from brokerageOrgId to orgId. |
| convex/seed/seedOnboardingRequest.ts | Adds organizationId to onboarding audit journal seeding. |
| convex/seed/seedObligation.ts | Denormalizes obligation orgId from mortgage; threads organizationId into seeded audit entries. |
| convex/seed/seedMortgage.ts | Denormalizes mortgage orgId from broker; threads organizationId into seeded audit entries. |
| convex/seed/seedLender.ts | Denormalizes lender orgId from broker; threads organizationId into seeded audit entries. |
| convex/seed/seedHelpers.ts | Extends seed journal helpers to accept/persist organizationId. |
| convex/seed/seedDeal.ts | Denormalizes deal orgId from mortgage; threads organizationId into seeded audit entries. |
| convex/seed/seedBroker.ts | Renames broker brokerageOrgId → orgId; adds organizationId to audit seed entry payloads. |
| convex/seed/seedBorrower.ts | Sets seeded borrower orgId and corresponding audit organizationId. |
| convex/schema.ts | Adds optional orgId to multiple tables + org indexes; adds auditJournal.organizationId + org/time index. |
| convex/payments/transfers/principalReturn.ts | Fixes typing/circularity via explicit return type and uses legNumberValidator. |
| convex/payments/transfers/mutations.ts | Derives/persists transferRequests.orgId via orgIdForTransferRequest; adds explicit return types. |
| convex/payments/transfers/depositCollection.ts | Fixes typing/circularity via explicit return type. |
| convex/payments/obligations/generateImpl.ts | Denormalizes generated obligations’ orgId from the mortgage. |
| convex/payments/obligations/createCorrectiveObligation.ts | Copies orgId to corrective obligations and sets audit organizationId. |
| convex/onboarding/mutations.ts | Adds organizationId to onboarding request creation audit journal entry. |
| convex/obligations/mutations.ts | Denormalizes obligation orgId from mortgage; sets audit organizationId. |
| convex/lib/orgScope.ts | New helper module for org resolution and audit org extraction. |
| convex/engine/types.ts | Adds organizationId to AuditJournalEntry interface. |
| convex/engine/transition.ts | Extracts organizationId from entity documents and includes it in GT audit journal entries. |
| convex/engine/effects/transfer.ts | Adds organizationId to transfer-related audit journal entries. |
| convex/engine/effects/collectionAttempt.ts | Persists orgId on bridged transferRequests. |
| convex/dispersal/disbursementBridge.ts | Persists orgId on transferRequests created by the bridge. |
| convex/dispersal/createDispersalEntries.ts | Persists dispersal entry orgId from the mortgage. |
| convex/demo/simulation.ts | Threads org scope through simulation-created brokers/borrowers/lenders/mortgages. |
| convex/_generated/api.d.ts | Updates generated API typings to include new/updated modules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
convex/payments/transfers/mutations.ts (1)
539-552:⚠️ Potential issue | 🟠 MajorOnly make the retry-safe fast path for already-progressed states.
This branch now returns
{ success: true }for any status other than"initiated", which means"failed"or"cancelled"transfers are reported as a successful no-op. That is especially risky because the known orchestrators inconvex/payments/transfers/depositCollection.ts:79-92andconvex/payments/transfers/principalReturn.ts:86-100currently ignore the returnedTransitionResultand would still bubble up{ transferId }as if initiation had succeeded.Suggested patch
- if (transfer.status !== "initiated") { + if ( + transfer.status === "pending" || + transfer.status === "processing" || + transfer.status === "confirmed" + ) { console.info( `[initiateTransferInternal] Transfer ${args.transferId} already in "${transfer.status}" — skipping initiation (retry-safe).` ); return { success: true, previousState: transfer.status, newState: transfer.status, reason: "already_initiated_or_beyond", }; } + if (transfer.status !== "initiated") { + throw new ConvexError( + `Transfer must be in "initiated" status to initiate, currently: "${transfer.status}"` + ); + }🤖 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 539 - 552, The current retry-safe fast path in initiateTransferInternal returns success:true for any transfer.status !== "initiated", which incorrectly treats "failed" or "cancelled" as successful; change the condition to only short-circuit for explicitly acceptable progressed states (e.g., ["pending","confirmed"]) and return the existing success:true/previousState/newState only for those states; for "failed" or "cancelled" return a failure result (success:false with a clear reason) or throw so callers (like createDealClosingPipeline/createAndInitiateLeg2) don't treat failed/cancelled transfers as successful no-ops.
🧹 Nitpick comments (2)
convex/seed/seedDeal.ts (1)
219-222: Redundant mortgage lookup.The mortgage was already validated in
resolveMortgageIds(lines 131-136), somortgageIdis guaranteed to exist. This secondctx.db.getadds an unnecessary read.Consider fetching mortgages once during resolution and passing the documents directly, or removing this check since the ID pool is pre-validated.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/seed/seedDeal.ts` around lines 219 - 222, Remove the redundant database lookup for mortgageId in seedDeal.ts: since resolveMortgageIds already validates existence, eliminate the ctx.db.get(mortgageId) and the subsequent mortgageRow null check/ConvexError throw; instead use the validated mortgage IDs (or pass the pre-fetched mortgage documents from resolveMortgageIds into createDeal if available) so seedDeal no longer performs an extra ctx.db.get for mortgageId.convex/seed/seedObligation.ts (1)
229-234: Redundant mortgage lookup.The mortgage document is already fetched at line 182 into the
mortgagevariable for the samepair.mortgageId. This second lookup (mortgageRow) duplicates that read.♻️ Use the existing mortgage variable
- const mortgageRow = await ctx.db.get(pair.mortgageId); - if (!mortgageRow) { - throw new ConvexError( - `seedObligation: mortgage not found ${pair.mortgageId}` - ); - } - const obligationId = await ctx.db.insert("obligations", { - orgId: mortgageRow.orgId, + orgId: mortgage.orgId,Then update lines 258 and 275 to use
mortgage.orgIdinstead ofmortgageRow.orgId.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/seed/seedObligation.ts` around lines 229 - 234, Remove the redundant DB read that assigns mortgageRow by reusing the already-fetched mortgage (fetched earlier for pair.mortgageId); delete the await ctx.db.get(...) that creates mortgageRow, replace any uses of mortgageRow.orgId with mortgage.orgId (including the occurrences noted around where lines ~258 and ~275 reference orgId), and ensure any null-checks rely on the existing mortgage variable instead of mortgageRow to avoid duplicate reads.
🤖 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/lib/orgScope.ts`:
- Around line 26-31: orgIdFromMortgageId currently returns mortgage?.orgId and
can miss legacy scoping; update it to fall back: if mortgage.orgId is empty,
check mortgage.brokerOfRecordId and, if present, fetch that broker (ctx.db.get
on the broker id) and return broker.orgId if set; if still empty, iterate
mortgage.lenders (if present), for each lender fetch the lender row (ctx.db.get)
and then fetch that lender's broker via lender.brokerId to return its orgId when
found; otherwise return undefined. Apply the same fallback logic to the other
orgId-resolver function in this module so both mortgage-based lookups resolve
via broker relationships when orgId is missing.
In `@convex/payments/transfers/mutations.ts`:
- Around line 857-858: The retry path is copying transfer.orgId directly when
inserting a new transferRequest, which can leave retries unscoped if the
original row lacked orgId; update the retry flow to recompute scope using
orgIdForTransferRequest(...) instead of reusing transfer.orgId—find the code
that calls ctx.db.insert("transferRequests", { orgId: transfer.orgId, ... }) in
the retryTransfer logic and replace the orgId value with the result of
orgIdForTransferRequest(...) (pass the same transfer/related identifiers the
primary create flow uses) so retries resolve the org scope consistently.
In `@convex/schema.ts`:
- Around line 110-121: Existing brokers lack orgId causing them to be excluded
from the new indexes (by_org, by_org_status) and to break requireOrgIdFromBroker
in convex/lib/orgScope.ts (affecting seedLender/seedMortgage); add a one-off
production backfill that finds brokers with null orgId and populates orgId from
their WorkOS user context (or auditJournal.organizationId similarly) using a
safe migration: write a convex-dev-migrations or admin mutation that queries
records where orgId is null, resolves the correct WorkOS org id per broker (via
the same lookup logic used in requireOrgIdFromBroker), updates each
broker.auditJournal.organizationId and broker.orgId, and run/verify the
migration, or alternatively document and assert that no persisted brokers exist
so the backfill isn’t required.
---
Outside diff comments:
In `@convex/payments/transfers/mutations.ts`:
- Around line 539-552: The current retry-safe fast path in
initiateTransferInternal returns success:true for any transfer.status !==
"initiated", which incorrectly treats "failed" or "cancelled" as successful;
change the condition to only short-circuit for explicitly acceptable progressed
states (e.g., ["pending","confirmed"]) and return the existing
success:true/previousState/newState only for those states; for "failed" or
"cancelled" return a failure result (success:false with a clear reason) or throw
so callers (like createDealClosingPipeline/createAndInitiateLeg2) don't treat
failed/cancelled transfers as successful no-ops.
---
Nitpick comments:
In `@convex/seed/seedDeal.ts`:
- Around line 219-222: Remove the redundant database lookup for mortgageId in
seedDeal.ts: since resolveMortgageIds already validates existence, eliminate the
ctx.db.get(mortgageId) and the subsequent mortgageRow null check/ConvexError
throw; instead use the validated mortgage IDs (or pass the pre-fetched mortgage
documents from resolveMortgageIds into createDeal if available) so seedDeal no
longer performs an extra ctx.db.get for mortgageId.
In `@convex/seed/seedObligation.ts`:
- Around line 229-234: Remove the redundant DB read that assigns mortgageRow by
reusing the already-fetched mortgage (fetched earlier for pair.mortgageId);
delete the await ctx.db.get(...) that creates mortgageRow, replace any uses of
mortgageRow.orgId with mortgage.orgId (including the occurrences noted around
where lines ~258 and ~275 reference orgId), and ensure any null-checks rely on
the existing mortgage variable instead of mortgageRow to avoid duplicate reads.
🪄 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: a717cc2e-2462-4f7d-96b2-bcee1b9c5a6a
⛔ Files ignored due to path filters (1)
convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (28)
convex/demo/simulation.tsconvex/dispersal/createDispersalEntries.tsconvex/dispersal/disbursementBridge.tsconvex/engine/effects/collectionAttempt.tsconvex/engine/effects/transfer.tsconvex/engine/transition.tsconvex/engine/types.tsconvex/lib/orgScope.tsconvex/obligations/mutations.tsconvex/onboarding/mutations.tsconvex/payments/obligations/createCorrectiveObligation.tsconvex/payments/obligations/generateImpl.tsconvex/payments/transfers/depositCollection.tsconvex/payments/transfers/mutations.tsconvex/payments/transfers/principalReturn.tsconvex/schema.tsconvex/seed/seedBorrower.tsconvex/seed/seedBroker.tsconvex/seed/seedDeal.tsconvex/seed/seedHelpers.tsconvex/seed/seedLender.tsconvex/seed/seedMortgage.tsconvex/seed/seedObligation.tsconvex/seed/seedOnboardingRequest.tspackage.jsonspecs/ENG-20/chunks/chunk-01-foundation-and-profiles/context.mdspecs/ENG-20/chunks/chunk-01-foundation-and-profiles/status.mdtsconfig.json
…tions, tests - Backfill orgId on reused seed mortgages and brokers; broker/lender fallbacks in orgScope - Corrective obligations + audit journal use derived orgId from mortgage chain - Bridge transfers and disbursement bridge prefer entry/org resolution paths - retryTransfer recomputes orgId via orgIdForTransferRequest - Add convex-dev-migrations backfills for brokers, lenders, audit journal - Add orgIdForTransferRequest convex-test coverage Made-with: Cursor
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.
audit journal organizationId extraction.
obligations, deals, dispersalEntries, transferRequests; organizationId on
auditJournal; rename broker brokerageOrgId to orgId with org indexes.
onboarding, and transition audit entries.
use legNumberValidator on principal return.
Summary by CodeRabbit
Release Notes