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, your pull request is larger than the review limit of 150000 diff characters
Contributor
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR introduces a comprehensive test suite and specification framework (8 test files totaling ~2,500 lines, plus 6 specification markdown documents and 1 HTML overview) for validating transfer state machine behavior, provider registry resolution, cash ledger bridge mapping, webhook handling, financial properties, and end-to-end integration scenarios. Minor production code changes include exporting a helper function and adding runtime type validators for webhook event normalization. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
This 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 an ENG-214 spec bundle plus a large new automated test suite to harden the unified payment rails domain against regressions across transfer state transitions, provider resolution, cash-ledger posting behavior, and key financial invariants.
Changes:
- Added multiple new Vitest suites covering transfer machine transitions, mock-provider webhook simulations, cash-ledger mapping/idempotency conventions, and end-to-end inbound/outbound transfer posting flows.
- Added deterministic “financial property” tests for rounding conservation, exactly-once posting, replay safety, and reversal completeness.
- Added ENG-214 planning/spec artifacts (task lists, chunk context, and an HTML overview) and a small formatting-only change in
createTransferRequesterror handling.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/ENG-214/tasks.md | High-level task checklist for the ENG-214 test effort. |
| specs/ENG-214/plan-overview.html | Detailed visual spec/plan for the ENG-214 test suite. |
| specs/ENG-214/chunks/manifest.md | Chunk execution/dependency manifest for the work. |
| specs/ENG-214/chunks/chunk-01-transfer-machine-provider-registry/tasks.md | Chunk 1 task list. |
| specs/ENG-214/chunks/chunk-01-transfer-machine-provider-registry/context.md | Chunk 1 implementation/testing guidance. |
| specs/ENG-214/chunks/chunk-02-cashledger-bridge-mapping/tasks.md | Chunk 2 task list. |
| specs/ENG-214/chunks/chunk-02-cashledger-bridge-mapping/context.md | Chunk 2 mapping/webhook/reconciliation guidance. |
| specs/ENG-214/chunks/chunk-03-inbound-flow-integration/tasks.md | Chunk 3 task list. |
| specs/ENG-214/chunks/chunk-03-inbound-flow-integration/context.md | Chunk 3 end-to-end inbound/bridge guidance. |
| specs/ENG-214/chunks/chunk-04-outbound-multileg-integration/tasks.md | Chunk 4 task list. |
| specs/ENG-214/chunks/chunk-04-outbound-multileg-integration/context.md | Chunk 4 outbound + multi-leg safety guidance. |
| specs/ENG-214/chunks/chunk-05-financial-property-regression/tasks.md | Chunk 5 task list. |
| specs/ENG-214/chunks/chunk-05-financial-property-regression/context.md | Chunk 5 deterministic invariant + regression guidance. |
| convex/payments/transfers/providers/tests/registry.test.ts | New unit tests for provider registry resolution + mock-provider gating. |
| convex/payments/transfers/mutations.ts | Formatting/indentation fix inside createTransferRequest error handling block. |
| convex/payments/transfers/tests/transferMachine.test.ts | New unit tests covering valid/invalid transfer machine transitions + actions. |
| convex/payments/transfers/tests/webhookPipeline.test.ts | New unit tests simulating mock webhooks and verifying machine dedup behavior. |
| convex/payments/transfers/tests/cashLedgerMapping.test.ts | New unit tests asserting transfer-type → cash-ledger mapping + idempotency key format. |
| convex/payments/transfers/tests/inboundFlow.integration.test.ts | New integration tests for inbound posting, D4 bridge skip, and bridge idempotency/full flow. |
| convex/payments/transfers/tests/outboundFlow.integration.test.ts | New integration tests for outbound posting, failure safety, and multi-leg scenarios. |
| convex/payments/transfers/tests/financialProperties.test.ts | New deterministic invariant tests for rounding, exactly-once posting, replay safety, and reversals. |
💡 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: 6
🧹 Nitpick comments (3)
convex/payments/transfers/providers/__tests__/registry.test.ts (1)
71-76: Minor: Test comment says "unset" but stubs to empty string.The comment "when ENABLE_MOCK_PROVIDERS is unset" may be slightly misleading since
vi.stubEnv("ENABLE_MOCK_PROVIDERS", "")sets the variable to an empty string rather than leaving it undefined. The test behavior is correct sinceareMockProvidersEnabled()checks for=== "true", but consider clarifying the comment to "is empty" for accuracy.📝 Suggested comment clarification
- it("mock_pad throws when ENABLE_MOCK_PROVIDERS is unset", () => { + it("mock_pad throws when ENABLE_MOCK_PROVIDERS is empty", () => { vi.stubEnv("ENABLE_MOCK_PROVIDERS", "");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/transfers/providers/__tests__/registry.test.ts` around lines 71 - 76, Update the test description to accurately reflect that the env var is being set to an empty string: change the it(...) title from "mock_pad throws when ENABLE_MOCK_PROVIDERS is unset" to something like "mock_pad throws when ENABLE_MOCK_PROVIDERS is empty" (the test still stubs the env via vi.stubEnv("ENABLE_MOCK_PROVIDERS", "") and asserts getTransferProvider("mock_pad") throws using MOCK_PROVIDERS_DISABLED_RE).convex/payments/transfers/__tests__/cashLedgerMapping.test.ts (1)
30-84: Prefer asserting against a canonical mapping source instead of re-implementing mapping logic in tests.Line 34 through Line 84 duplicates the transfer→family mapping logic. This can let the test drift from runtime behavior. Consider centralizing expected mappings in a shared constant (or exposing a pure mapping helper from production code) so tests validate the actual contract, not a parallel implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/transfers/__tests__/cashLedgerMapping.test.ts` around lines 30 - 84, The test re-implements the transfer→account-family mapping (functions inboundTransferCreditFamily and getCashLedgerMapping), which can drift from production; instead expose or import a single canonical mapping from production (e.g., export a shared constant or pure helper from convex/payments/cashLedger/integrations.ts) and have the test assert against that exported mapping/CashLedgerMapping shape; update the test to remove the duplicate inboundTransferCreditFamily/getCashLedgerMapping implementations and reference the production symbol (the exported mapping or helper) to derive expected creditFamily/debitFamily/entryType values.convex/payments/transfers/__tests__/financialProperties.test.ts (1)
33-55: Extract the shared transfer-test harness and fixtures.The component registration, seeded entity graph, and
_handlerwrappers are now copied across the inbound, outbound, and financial suites. Pulling them into a sharedtestUtils.tswill make future schema/setup changes much cheaper and reduce drift between these files.Based on learnings, shared test utility files that export helpers must use the
.tsextension (not.test.ts) because Biome'snoExportsInTestrule prohibits exports from.test.tsfiles.Also applies to: 65-83, 87-238
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/transfers/__tests__/financialProperties.test.ts` around lines 33 - 55, Extract the repeated harness/fixture setup into a shared helper file (e.g., testUtils.ts) by moving the modules, auditTrailModules, workflowModules, workpoolModules declarations, the TestHarness type alias, the createFullHarness function, the seeded entity graph, and any _handler wrapper helpers into that file and export them; update inbound, outbound, and financial test suites to import these helpers from testUtils.ts (use the .ts extension — not .test.ts — to satisfy Biome's noExportsInTest rule) so createFullHarness and related symbols are referenced from the single shared module.
🤖 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/transfers/__tests__/outboundFlow.integration.test.ts`:
- Around line 589-647: The test is invoking publishTransferConfirmed._handler
directly which bypasses the public manual-confirm path (confirmManualTransfer)
and misses provider/status guards and providerRef updates; replace the direct
effect call with invoking confirmManualTransfer (or change the test to be an
effect-level test) so the test exercises the real public flow, including
rejection when already confirmed, and still verifies ledger idempotency
(postCashEntryInternal); update references in this test to call
confirmManualTransfer with the same payload and audit/journal info, or if you
intend an effect-only test, rename and document the test accordingly and keep
publishTransferConfirmed._handler usage limited to that renamed effect-level
test.
- Around line 398-429: The test is missing a seeded deal and deal linkage: seed
a deal (e.g., create or reuse the existing seeded.dealId) before creating the
two transfers and pass that deal's id as dealId on both insertTransfer calls
(the two calls that create leg1TransferId and leg2TransferId, and any similar
calls in the 505-525 block), so the transfers are associated with the deal
pipeline; ensure the seeded deal's pipelineId matches pipelineId and that both
legs include dealId so the test exercises deal-state transitions rather than
independent transfers.
- Around line 222-259: The test "T-015: obligation settlement -> dispersal ->
outbound transfer -> LENDER_PAYOUT_SENT" creates a standalone outbound transfer
via insertTransfer without seeding a dispersalEntries row or setting
dispersalEntryId, so it never exercises the dispersal→disbursement flow; update
the test to seed a dispersal entry (insert into dispersalEntries) and attach its
id to the transfer record by setting dispersalEntryId when calling
insertTransfer (use the same seeded.mortgageId/lenderId and matching amount) so
the manual bridge path exercised by the dispersal→disbursement link is covered
by this fixture instead of duplicating T-018 coverage.
In `@convex/payments/transfers/__tests__/webhookPipeline.test.ts`:
- Around line 353-580: The test suite T-009 overstates coverage: it asserts
state-machine idempotency but does not verify webhook deduplication because
MockTransferProvider.simulateWebhook only echoes eventId and no receiver/store
of processed eventIds is checked; either add assertions that the webhook
receiver or dedup store (the component that records seen eventIds) was
called/updated (e.g., assert the store contains "webhook-evt-001" after
firstPayload and unchanged after secondPayload) or rename the suite to remove
the "webhook deduplication" claim; locate MockTransferProvider.simulateWebhook,
the test case "full pipeline: initiate → webhook(confirmed) → duplicate webhook
→ no additional effect", and any receiver/store module used for dedup to
implement the fix.
In `@specs/ENG-214/chunks/chunk-02-cashledger-bridge-mapping/context.md`:
- Around line 57-64: The spec's cash entry type list uses WAIVER which doesn't
match the implemented enum names; update the entry type(s) in
specs/ENG-214/chunks/chunk-02-cashledger-bridge-mapping/context.md to use the
actual implemented identifiers from convex/payments/cashLedger/types.ts (e.g.,
OBLIGATION_WAIVED and/or OBLIGATION_WRITTEN_OFF as appropriate), and ensure any
other listed entry names (CASH_RECEIVED, LENDER_PAYOUT_SENT, REVERSAL,
CORRECTION) exactly match the symbols in types.ts to avoid spec/implementation
mismatch.
In `@specs/ENG-214/plan-overview.html`:
- Around line 596-612: Escape the Mermaid arrow characters inside the
pre.mermaid block so HTMLHint no longer flags spec-char-escape: replace each raw
'-->' token in the stateDiagram-v2 diagram (lines containing transitions like
"initiated --> pending", "pending --> processing", etc.) with an escaped form
using '>' for the greater-than (e.g., '-->') so the diagram still renders
but the HTML is valid.
---
Nitpick comments:
In `@convex/payments/transfers/__tests__/cashLedgerMapping.test.ts`:
- Around line 30-84: The test re-implements the transfer→account-family mapping
(functions inboundTransferCreditFamily and getCashLedgerMapping), which can
drift from production; instead expose or import a single canonical mapping from
production (e.g., export a shared constant or pure helper from
convex/payments/cashLedger/integrations.ts) and have the test assert against
that exported mapping/CashLedgerMapping shape; update the test to remove the
duplicate inboundTransferCreditFamily/getCashLedgerMapping implementations and
reference the production symbol (the exported mapping or helper) to derive
expected creditFamily/debitFamily/entryType values.
In `@convex/payments/transfers/__tests__/financialProperties.test.ts`:
- Around line 33-55: Extract the repeated harness/fixture setup into a shared
helper file (e.g., testUtils.ts) by moving the modules, auditTrailModules,
workflowModules, workpoolModules declarations, the TestHarness type alias, the
createFullHarness function, the seeded entity graph, and any _handler wrapper
helpers into that file and export them; update inbound, outbound, and financial
test suites to import these helpers from testUtils.ts (use the .ts extension —
not .test.ts — to satisfy Biome's noExportsInTest rule) so createFullHarness and
related symbols are referenced from the single shared module.
In `@convex/payments/transfers/providers/__tests__/registry.test.ts`:
- Around line 71-76: Update the test description to accurately reflect that the
env var is being set to an empty string: change the it(...) title from "mock_pad
throws when ENABLE_MOCK_PROVIDERS is unset" to something like "mock_pad throws
when ENABLE_MOCK_PROVIDERS is empty" (the test still stubs the env via
vi.stubEnv("ENABLE_MOCK_PROVIDERS", "") and asserts
getTransferProvider("mock_pad") throws using MOCK_PROVIDERS_DISABLED_RE).
🪄 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: 8a509c1f-15fb-4716-9dd5-97891b514aaf
📒 Files selected for processing (21)
convex/payments/transfers/__tests__/cashLedgerMapping.test.tsconvex/payments/transfers/__tests__/financialProperties.test.tsconvex/payments/transfers/__tests__/inboundFlow.integration.test.tsconvex/payments/transfers/__tests__/outboundFlow.integration.test.tsconvex/payments/transfers/__tests__/transferMachine.test.tsconvex/payments/transfers/__tests__/webhookPipeline.test.tsconvex/payments/transfers/mutations.tsconvex/payments/transfers/providers/__tests__/registry.test.tsspecs/ENG-214/chunks/chunk-01-transfer-machine-provider-registry/context.mdspecs/ENG-214/chunks/chunk-01-transfer-machine-provider-registry/tasks.mdspecs/ENG-214/chunks/chunk-02-cashledger-bridge-mapping/context.mdspecs/ENG-214/chunks/chunk-02-cashledger-bridge-mapping/tasks.mdspecs/ENG-214/chunks/chunk-03-inbound-flow-integration/context.mdspecs/ENG-214/chunks/chunk-03-inbound-flow-integration/tasks.mdspecs/ENG-214/chunks/chunk-04-outbound-multileg-integration/context.mdspecs/ENG-214/chunks/chunk-04-outbound-multileg-integration/tasks.mdspecs/ENG-214/chunks/chunk-05-financial-property-regression/context.mdspecs/ENG-214/chunks/chunk-05-financial-property-regression/tasks.mdspecs/ENG-214/chunks/manifest.mdspecs/ENG-214/plan-overview.htmlspecs/ENG-214/tasks.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
convex/payments/transfers/types.ts (1)
194-199: Prefer deriving the inverse map from the forward map.
OBLIGATION_TYPE_TO_TRANSFER_TYPEnow duplicates information already encoded inTRANSFER_TYPE_TO_OBLIGATION_TYPE. The new tests will catch drift, but deriving the reverse lookup once from the forward constant would remove the extra sync point entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/transfers/types.ts` around lines 194 - 199, OBLIGATION_TYPE_TO_TRANSFER_TYPE duplicates TRANSFER_TYPE_TO_OBLIGATION_TYPE; replace the hardcoded constant by deriving it from the existing map instead of maintaining both. Implement OBLIGATION_TYPE_TO_TRANSFER_TYPE by inverting TRANSFER_TYPE_TO_OBLIGATION_TYPE at module runtime (e.g., Object.entries/fromEntries or a small reducer), then apply the same readonly/type assertion (as const satisfies Record<ObligationType, InboundTransferType>) so the inverted result has the correct static type and removes the duplicate sync point.convex/engine/effects/__tests__/transfer.test.ts (1)
370-391: Consider adding a comment explaining the mock ctx approach.This test uses a mock
ctxobject rather than the full harness to test the edge case of a malformed transfer with nodirection. While this is a valid approach for testing error paths, a brief comment explaining why the mock is used (to avoid needing to create an invalid database state) would improve clarity.🤖 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 370 - 391, Add a brief inline comment above the mocked ctx in the "fails loudly when a malformed non-bridged transfer has no direction" test explaining that the test intentionally constructs a minimal mock MutationCtx (db.get/db.patch) instead of using the full test harness to avoid creating an invalid DB state; reference the mocked variable name "ctx", the tested handler "publishTransferConfirmedMutation._handler", the helper "buildEffectArgs", and the expected error constant "NO_DIRECTION_RE" so future readers understand the rationale for the lightweight mock approach.convex/payments/webhooks/rotessaPad.ts (1)
120-168: Consider extracting shared helpers to reduce duplication withvopay.ts.The
finalizeWebhookEventandpatchWebhookEventMetadatafunctions are nearly identical to those inconvex/payments/webhooks/vopay.ts. While the current duplication is acceptable for a test-focused PR, consider extracting these intotransferCore.tsin a follow-up to maintain DRY principles.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/webhooks/rotessaPad.ts` around lines 120 - 168, Duplicate helper logic exists in rotessaPad.ts and vopay.ts; extract finalizeWebhookEvent and patchWebhookEventMetadata into a new shared module (transferCore.ts) and import them from both webhook handlers. Move the two functions (keeping their current signatures and types: MutationCtx, NormalizedTransferWebhookEventType, Id<"webhookEvents"> / Id<"transferRequests">) into transferCore.ts, export them, and replace the local definitions in rotessaPad.ts and vopay.ts with imports; ensure any referenced symbols (ctx.db.patch, ctx.db.get, Date.now(), attempts handling) remain unchanged and update imports/exports so type resolution compiles.
🤖 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/webhooks/__tests__/vopayWebhook.test.ts`:
- Around line 199-248: The seeded bridged transfer in seedBridgedTransfer is
starting in a state the real bridge path never reaches (it inserts
collectionAttemptId but sets transferRequests.status to "pending"); fix by
either removing collectionAttemptId to model a provider-owned transfer, or keep
collectionAttemptId and change the seeded transferRequests.status to "confirmed"
so tests exercise the idempotent webhook branch; update both occurrences in
seedBridgedTransfer (the transferRequests insert blocks where
providerCode/providerRef are used) to implement the chosen approach.
In `@convex/payments/webhooks/types.ts`:
- Around line 3-17: The runtime validator currently uses v.optional(v.string())
which allows any string to be stored for the normalizedEventType; update the
Convex schema and validators to enforce the exact literal union defined by
NormalizedTransferWebhookEventType. Replace v.optional(v.string()) with a
v.optional(v.union(...)) (or equivalent Convex literal union validator) that
enumerates
"FUNDS_SETTLED","TRANSFER_FAILED","TRANSFER_REVERSED","PROCESSING_UPDATE", and
remove the unsafe cast in transferCore.ts by validating args.normalizedEventType
against that validator before using it (affecting the validator in
convex/schema.ts and usages in convex/payments/webhooks/transferCore.ts, and the
TransferWebhookMetadataPatch shape).
In `@src/test/convex/payments/webhooks/convexTestHarness.ts`:
- Around line 46-57: The file filter currently accepts ".ts" and ".js" and thus
includes TypeScript declaration files like ".d.ts" which are non-executable and
cause dynamic-import failures; update the condition in the loop that builds
moduleEntries (the code referencing entry.name, moduleKey, moduleEntries,
nextRelativePath) to explicitly exclude declaration files (e.g., skip names
ending with ".d.ts") so only executable ".ts" and ".js" files are queued, and
simplify the dynamic import to use nextUrl.href directly instead of
import(pathToFileURL(nextUrl.pathname).href).
---
Nitpick comments:
In `@convex/engine/effects/__tests__/transfer.test.ts`:
- Around line 370-391: Add a brief inline comment above the mocked ctx in the
"fails loudly when a malformed non-bridged transfer has no direction" test
explaining that the test intentionally constructs a minimal mock MutationCtx
(db.get/db.patch) instead of using the full test harness to avoid creating an
invalid DB state; reference the mocked variable name "ctx", the tested handler
"publishTransferConfirmedMutation._handler", the helper "buildEffectArgs", and
the expected error constant "NO_DIRECTION_RE" so future readers understand the
rationale for the lightweight mock approach.
In `@convex/payments/transfers/types.ts`:
- Around line 194-199: OBLIGATION_TYPE_TO_TRANSFER_TYPE duplicates
TRANSFER_TYPE_TO_OBLIGATION_TYPE; replace the hardcoded constant by deriving it
from the existing map instead of maintaining both. Implement
OBLIGATION_TYPE_TO_TRANSFER_TYPE by inverting TRANSFER_TYPE_TO_OBLIGATION_TYPE
at module runtime (e.g., Object.entries/fromEntries or a small reducer), then
apply the same readonly/type assertion (as const satisfies
Record<ObligationType, InboundTransferType>) so the inverted result has the
correct static type and removes the duplicate sync point.
In `@convex/payments/webhooks/rotessaPad.ts`:
- Around line 120-168: Duplicate helper logic exists in rotessaPad.ts and
vopay.ts; extract finalizeWebhookEvent and patchWebhookEventMetadata into a new
shared module (transferCore.ts) and import them from both webhook handlers. Move
the two functions (keeping their current signatures and types: MutationCtx,
NormalizedTransferWebhookEventType, Id<"webhookEvents"> /
Id<"transferRequests">) into transferCore.ts, export them, and replace the local
definitions in rotessaPad.ts and vopay.ts with imports; ensure any referenced
symbols (ctx.db.patch, ctx.db.get, Date.now(), attempts handling) remain
unchanged and update imports/exports so type resolution compiles.
🪄 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: a8903f45-6627-446e-9994-def0f7305dd1
⛔ Files ignored due to path filters (1)
convex/_generated/api.d.tsis excluded by!**/_generated/**
📒 Files selected for processing (56)
convex/engine/effects/__tests__/transfer.test.tsconvex/engine/effects/collectionAttempt.tsconvex/http.tsconvex/payments/cashLedger/integrations.tsconvex/payments/transfers/__tests__/bridge.test.tsconvex/payments/transfers/__tests__/cashLedgerMapping.test.tsconvex/payments/transfers/__tests__/handlers.integration.test.tsconvex/payments/transfers/__tests__/inboundFlow.integration.test.tsconvex/payments/transfers/__tests__/mutations.test.tsconvex/payments/transfers/__tests__/outboundFlow.integration.test.tsconvex/payments/transfers/__tests__/types.test.tsconvex/payments/transfers/__tests__/webhookPipeline.test.tsconvex/payments/transfers/mutations.tsconvex/payments/transfers/providers/__tests__/mock.test.tsconvex/payments/transfers/providers/manual.tsconvex/payments/transfers/types.tsconvex/payments/webhooks/__tests__/eftVopayWebhook.test.tsconvex/payments/webhooks/__tests__/rotessaWebhook.test.tsconvex/payments/webhooks/__tests__/vopayWebhook.test.tsconvex/payments/webhooks/eftVopay.tsconvex/payments/webhooks/rotessaPad.tsconvex/payments/webhooks/transferCore.tsconvex/payments/webhooks/types.tsconvex/payments/webhooks/vopay.tsconvex/schema.tsspecs/ENG-194/chunks/chunk-01-transfer-effect-tests/context.mdspecs/ENG-194/chunks/chunk-01-transfer-effect-tests/status.mdspecs/ENG-194/chunks/chunk-01-transfer-effect-tests/tasks.mdspecs/ENG-194/chunks/manifest.mdspecs/ENG-194/tasks.mdspecs/ENG-196/chunks/chunk-01-manual-bidirectional/context.mdspecs/ENG-196/chunks/chunk-01-manual-bidirectional/status.mdspecs/ENG-196/chunks/chunk-01-manual-bidirectional/tasks.mdspecs/ENG-196/chunks/manifest.mdspecs/ENG-196/tasks.mdspecs/ENG-197/chunks/chunk-01-bridge-mapping/context.mdspecs/ENG-197/chunks/chunk-01-bridge-mapping/status.mdspecs/ENG-197/chunks/chunk-01-bridge-mapping/tasks.mdspecs/ENG-197/chunks/chunk-02-tests-verification/context.mdspecs/ENG-197/chunks/chunk-02-tests-verification/status.mdspecs/ENG-197/chunks/chunk-02-tests-verification/tasks.mdspecs/ENG-197/chunks/manifest.mdspecs/ENG-197/tasks.mdspecs/ENG-198/chunks/chunk-01-schema-shared-core/context.mdspecs/ENG-198/chunks/chunk-01-schema-shared-core/status.mdspecs/ENG-198/chunks/chunk-01-schema-shared-core/tasks.mdspecs/ENG-198/chunks/chunk-02-vopay-eft/context.mdspecs/ENG-198/chunks/chunk-02-vopay-eft/status.mdspecs/ENG-198/chunks/chunk-02-vopay-eft/tasks.mdspecs/ENG-198/chunks/chunk-03-rotessa-tests/context.mdspecs/ENG-198/chunks/chunk-03-rotessa-tests/status.mdspecs/ENG-198/chunks/chunk-03-rotessa-tests/tasks.mdspecs/ENG-198/chunks/manifest.mdspecs/ENG-198/tasks.mdspecs/ENG-214/chunks/chunk-02-cashledger-bridge-mapping/context.mdsrc/test/convex/payments/webhooks/convexTestHarness.ts
✅ Files skipped from review due to trivial changes (26)
- specs/ENG-196/chunks/manifest.md
- specs/ENG-198/chunks/chunk-01-schema-shared-core/tasks.md
- specs/ENG-194/chunks/manifest.md
- specs/ENG-194/chunks/chunk-01-transfer-effect-tests/status.md
- convex/payments/webhooks/eftVopay.ts
- specs/ENG-197/chunks/chunk-02-tests-verification/status.md
- specs/ENG-197/chunks/manifest.md
- specs/ENG-198/chunks/manifest.md
- specs/ENG-196/chunks/chunk-01-manual-bidirectional/tasks.md
- specs/ENG-197/chunks/chunk-02-tests-verification/tasks.md
- specs/ENG-198/chunks/chunk-03-rotessa-tests/status.md
- specs/ENG-198/chunks/chunk-01-schema-shared-core/status.md
- specs/ENG-198/chunks/chunk-03-rotessa-tests/tasks.md
- specs/ENG-197/chunks/chunk-01-bridge-mapping/status.md
- specs/ENG-198/chunks/chunk-02-vopay-eft/tasks.md
- specs/ENG-198/chunks/chunk-03-rotessa-tests/context.md
- specs/ENG-197/chunks/chunk-02-tests-verification/context.md
- specs/ENG-196/chunks/chunk-01-manual-bidirectional/status.md
- specs/ENG-197/chunks/chunk-01-bridge-mapping/tasks.md
- specs/ENG-197/tasks.md
- specs/ENG-198/chunks/chunk-02-vopay-eft/status.md
- specs/ENG-194/chunks/chunk-01-transfer-effect-tests/tasks.md
- specs/ENG-194/tasks.md
- specs/ENG-196/tasks.md
- specs/ENG-198/tasks.md
- specs/ENG-198/chunks/chunk-02-vopay-eft/context.md
🚧 Files skipped from review as they are similar to previous changes (5)
- convex/payments/transfers/mutations.ts
- convex/payments/transfers/tests/cashLedgerMapping.test.ts
- convex/payments/transfers/tests/outboundFlow.integration.test.ts
- convex/payments/transfers/tests/webhookPipeline.test.ts
- convex/payments/transfers/tests/inboundFlow.integration.test.ts
…ment Add afterAll cleanup that calls vi.clearAllTimers() and vi.useRealTimers() to prevent fake timer leakage into subsequent test files. Update the ORDERING NOTE header comment to accurately reflect that only T-011 uses finishAllScheduledFunctions (T-014 does not). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ncy" T-009 tests that the XState transfer machine ignores replayed events when already in a terminal state (confirmed/failed/reversed), producing zero transitions and zero side-effect actions. It does NOT test the actual webhook dedup store (persistTransferWebhookEvent), which is covered by the integration tests in eftVopayWebhook.test.ts. Renames the suite and its describe blocks to accurately reflect what is being tested, and adds a doc comment pointing to where real webhook dedup coverage lives. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…licating it in test The test helper had divergent behavior from production (throwing on unknown types vs falling back to UNAPPLIED_CASH). Export the production function and import it directly, eliminating the duplication and behavioral mismatch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- T-015: Seed obligation + dispersalEntry and link to transfer via dispersalEntryId/obligationId to exercise the full settlement -> dispersal -> outbound transfer pipeline. - T-017: Seed a deal entity and pass dealId on both transfer legs. Add deal-state assertion to catch regressions where Leg 2 failure incorrectly advances the deal. Add dealId assertions on pipeline query. - T-018: Rename test to "effect-level" to clarify it directly invokes publishTransferConfirmed handler (bypassing confirmManualTransfer's provider/status guards). Update comments to distinguish ledger-level idempotency from public API rejection behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace WAIVER with OBLIGATION_WAIVED and OBLIGATION_WRITTEN_OFF to match convex/payments/cashLedger/types.ts. Add missing entry types (OBLIGATION_ACCRUED, CASH_APPLIED, LENDER_PAYABLE_CREATED, SERVICING_FEE_RECOGNIZED, SUSPENSE_ESCALATED, SUSPENSE_ROUTED) and missing account families (SERVICING_REVENUE, WRITE_OFF). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Connorbelez
force-pushed
the
Connorbelez/eng-214-payment-rails-tests
branch
from
8b14c8f to
e0c633f
Compare
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 comprehensive test suite for unified payment rails with 158 new tests across 7 test files, covering transfer state machine transitions, cash ledger mappings, webhook pipelines, and financial property invariants.
What changed?
Added extensive test coverage for the unified payment rails system:
How to test?
Run the new test suites:
Why make this change?
This comprehensive test suite ensures zero regression when introducing unified payment rails by:
The test suite provides confidence that the unified payment rails can be deployed without breaking existing payment flows or introducing financial inconsistencies.
Summary by CodeRabbit
Release Notes
Tests
Code Quality