Merged
Conversation
![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
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (3)
📝 WalkthroughWalkthroughThis PR records ownership-snapshot metadata on dispersal entries and tightens disbursement validation. Changes
Sequence Diagram(s)sequenceDiagram
participant Creator as CreateDispersalEntries
participant Rerouter as applyDealReroutes
participant DB as DispersalEntries (DB)
Creator->>Rerouter: compute & attempt reroutes
Rerouter-->>Creator: number of applied reroutes (n)
Creator->>DB: insert dispersalEntry with calculationDetails { ownershipSnapshotDate, reroutesAppliedCount: n }
DB-->>Creator: persisted entry id
sequenceDiagram
participant Bridge as processSingleDisbursement
participant DB as DispersalEntries (DB)
participant Ledger as CashLedger / Provider
participant Transfers as TransferRequests (DB)
Bridge->>DB: re-read dispersal entry
DB-->>Bridge: entry (incl. calculationDetails)
Bridge->>Bridge: validate calculationDetails present & positive settledAmount
alt amount <= distributableAmount
Bridge->>Ledger: balance & provider checks...
Ledger-->>Bridge: ok
Bridge->>Transfers: insert transferRequest
Transfers-->>Bridge: transfer recorded
else amount > distributableAmount
Bridge-->>Bridge: throw AMOUNT_EXCEEDS_DISTRIBUTABLE
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
This 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
Implements ENG-219’s “effective-date ownership snapshot” enforcement by persisting snapshot audit metadata on dispersal entries and adding defensive validation in the disbursement bridge so post-calculation ownership changes can’t affect disbursement amounts.
Changes:
- Added optional audit metadata fields (
ownershipSnapshotDate,reroutesAppliedCount) to dispersalcalculationDetails(validator + TS types) and persisted them during dispersal creation. - Updated
applyDealReroutesto return the number of reroutes actually applied and recorded that count incalculationDetails. - Added bridge assertions for missing/invalid calculation details and for entry amounts exceeding
distributableAmount, plus tests covering the new behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/ENG-219/tasks.md | Master task list for ENG-219. |
| specs/ENG-219/chunks/manifest.md | Chunk manifest for ENG-219 work breakdown. |
| specs/ENG-219/chunks/chunk-01-schema-backend/tasks.md | Chunk 01 task checklist (schema/backend). |
| specs/ENG-219/chunks/chunk-01-schema-backend/context.md | Implementation context/plan for schema/backend updates. |
| specs/ENG-219/chunks/chunk-02-tests/tasks.md | Chunk 02 task checklist (tests). |
| specs/ENG-219/chunks/chunk-02-tests/context.md | Test plan/context for ENG-219 scenarios. |
| convex/dispersal/validators.ts | Extends calculationDetailsValidator with optional snapshot audit fields. |
| convex/dispersal/types.ts | Extends CalculationDetails interface with optional snapshot audit fields. |
| convex/dispersal/createDispersalEntries.ts | Returns applied reroute count and persists snapshot metadata to entries. |
| convex/dispersal/disbursementBridge.ts | Adds defensive assertions for missing calc details and distributable amount limit. |
| convex/dispersal/tests/disbursementBridge.test.ts | Adds ENG-219 tests and adjusts a gate test to satisfy new assertions. |
| convex/dispersal/tests/createDispersalEntries.test.ts | Adds metadata persistence tests for snapshot date and reroute count. |
💡 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.
🧹 Nitpick comments (3)
specs/ENG-219/chunks/chunk-01-schema-backend/tasks.md (1)
4-8: Minor: Task checkboxes are unchecked but master list shows tasks as complete.These tasks are listed as
- [ ](unchecked) whilespecs/ENG-219/tasks.mdmarks them as[x](complete). Consider updating for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/ENG-219/chunks/chunk-01-schema-backend/tasks.md` around lines 4 - 8, The checklist in specs/ENG-219/chunks/chunk-01-schema-backend/tasks.md shows T-001 through T-005 as unchecked; update those five items to checked to match the master spec (specs/ENG-219/tasks.md) that marks them complete. Specifically change the list entries for T-001, T-002, T-003, T-004, and T-005 in chunk-01-schema-backend/tasks.md to use the completed checkbox state so they are consistent with the master task file.specs/ENG-219/chunks/chunk-02-tests/tasks.md (1)
4-7: Minor: Task checkboxes are unchecked but manifest shows chunk as complete.The tasks here are listed as
- [ ](unchecked), whilespecs/ENG-219/chunks/manifest.mdmarks chunk 02 as "complete". Consider updating to- [x]for consistency with the manifest status.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/ENG-219/chunks/chunk-02-tests/tasks.md` around lines 4 - 7, The task list for chunk 02 shows unchecked items but the manifest marks the chunk as complete; update the four checklist lines for T-006, T-007, T-008, and T-009 by changing each "- [ ]" to "- [x]" so the tasks (T-006 through T-009) reflect the completed status and remain consistent with the manifest.specs/ENG-219/chunks/chunk-02-tests/context.md (1)
17-19: Clarify reroute narrative to avoid contradictory test guidance.These sections mix “must match lender auth ID / after dispersal” guidance with later clarification that the bridge only consumes
entry.amount. Consider simplifying to “insert reroute after entry creation; identity matching is not required for this bridge passthrough assertion” so the instructions stay internally consistent.As per coding guidelines, Markdown findings should focus on architecture/implementation consistency, not formatting.
Also applies to: 28-40, 47-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/ENG-219/chunks/chunk-02-tests/context.md` around lines 17 - 19, Update the narrative to remove the contradictory identity-matching requirement and clearly state that you should insert a dealReroutes record (with effectiveAfterDate set after dispersal but before disbursement) after the entry creation; emphasize that processSingleDisbursement (via the bridge) only consumes entry.amount and therefore the test should assert the transfer uses the ORIGINAL amount (45_000) — identity/lender auth ID matching is not required for this passthrough assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@specs/ENG-219/chunks/chunk-01-schema-backend/tasks.md`:
- Around line 4-8: The checklist in
specs/ENG-219/chunks/chunk-01-schema-backend/tasks.md shows T-001 through T-005
as unchecked; update those five items to checked to match the master spec
(specs/ENG-219/tasks.md) that marks them complete. Specifically change the list
entries for T-001, T-002, T-003, T-004, and T-005 in
chunk-01-schema-backend/tasks.md to use the completed checkbox state so they are
consistent with the master task file.
In `@specs/ENG-219/chunks/chunk-02-tests/context.md`:
- Around line 17-19: Update the narrative to remove the contradictory
identity-matching requirement and clearly state that you should insert a
dealReroutes record (with effectiveAfterDate set after dispersal but before
disbursement) after the entry creation; emphasize that processSingleDisbursement
(via the bridge) only consumes entry.amount and therefore the test should assert
the transfer uses the ORIGINAL amount (45_000) — identity/lender auth ID
matching is not required for this passthrough assertion.
In `@specs/ENG-219/chunks/chunk-02-tests/tasks.md`:
- Around line 4-7: The task list for chunk 02 shows unchecked items but the
manifest marks the chunk as complete; update the four checklist lines for T-006,
T-007, T-008, and T-009 by changing each "- [ ]" to "- [x]" so the tasks (T-006
through T-009) reflect the completed status and remain consistent with the
manifest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c565559-cfed-40e9-a3e7-2d65e0dedaca
📒 Files selected for processing (12)
convex/dispersal/__tests__/createDispersalEntries.test.tsconvex/dispersal/__tests__/disbursementBridge.test.tsconvex/dispersal/createDispersalEntries.tsconvex/dispersal/disbursementBridge.tsconvex/dispersal/types.tsconvex/dispersal/validators.tsspecs/ENG-219/chunks/chunk-01-schema-backend/context.mdspecs/ENG-219/chunks/chunk-01-schema-backend/tasks.mdspecs/ENG-219/chunks/chunk-02-tests/context.mdspecs/ENG-219/chunks/chunk-02-tests/tasks.mdspecs/ENG-219/chunks/manifest.mdspecs/ENG-219/tasks.md
…ursementBridge Extend the calculationDetails guard (step 2b) to also assert that `distributableAmount` is a finite number via `Number.isFinite` before the `entry.amount > entry.calculationDetails.distributableAmount` comparison is reached. Also upgrade the `settledAmount` check from `typeof ... !== "number"` to `Number.isFinite` for consistency, so both fields reject NaN, Infinity, and undefined/missing values from legacy or corrupted rows. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… regressions) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Connorbelez
added a commit
that referenced
this pull request
Apr 20, 2026
### TL;DR Implemented effective-date ownership snapshot enforcement for dispersal entries to prevent post-calculation reroutes from affecting disbursement amounts. ### What changed? Added audit metadata fields `ownershipSnapshotDate` and `reroutesAppliedCount` to dispersal entry calculation details. The `applyDealReroutes` function now returns the count of applied reroutes, and both fields are recorded during dispersal creation. Added defensive assertions in the disbursement bridge to validate calculation details exist and that entry amounts don't exceed distributable amounts. ### How to test? Run the dispersal test suite to verify: - Snapshot metadata is correctly recorded during dispersal creation - Reroutes applied after dispersal calculation but before disbursement don't change the disbursement amount - Bridge properly rejects entries with missing calculation details or amounts exceeding distributable limits ```bash bun run test -- --run convex/dispersal/__tests__/ ``` ### Why make this change? Ensures temporal consistency in the dispersal system by capturing ownership state at the time of calculation and preventing subsequent ownership changes from affecting already-calculated disbursement amounts. This maintains audit trails and prevents potential financial discrepancies. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Dispersal entries now record ownership snapshot dates and the count of reroutes applied. * Disbursement processing includes new validation gates to ensure calculation details are present and amounts do not exceed distributable values. * **Tests** * Added tests validating snapshot metadata persistence and reroute scenarios. * Added tests covering disbursement validation failures for missing calculation details and excessive amounts. * **Documentation** * New spec and task docs describing schema/backend changes and test plans. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
Implemented effective-date ownership snapshot enforcement for dispersal entries to prevent post-calculation reroutes from affecting disbursement amounts.
What changed?
Added audit metadata fields
ownershipSnapshotDateandreroutesAppliedCountto dispersal entry calculation details. TheapplyDealReroutesfunction now returns the count of applied reroutes, and both fields are recorded during dispersal creation. Added defensive assertions in the disbursement bridge to validate calculation details exist and that entry amounts don't exceed distributable amounts.How to test?
Run the dispersal test suite to verify:
bun run test -- --run convex/dispersal/__tests__/Why make this change?
Ensures temporal consistency in the dispersal system by capturing ownership state at the time of calculation and preventing subsequent ownership changes from affecting already-calculated disbursement amounts. This maintains audit trails and prevents potential financial discrepancies.
Summary by CodeRabbit
New Features
Tests
Documentation