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
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (21)
📝 WalkthroughWalkthroughAdds end-to-end webhook reversal infrastructure: signature verification, Rotessa and Stripe HTTP handlers, normalized reversal types, shared orchestration and cascade mutation/workflow, router registration, and comprehensive tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
participant Provider as Webhook Provider
participant HTTP as HTTP Handler
participant Verif as Signature Verification
participant Orch as Reversal Orchestrator
participant DB as Database
participant SM as State Machine
Provider->>HTTP: POST /webhooks/{rotessa|stripe} (raw body + signature)
HTTP->>Verif: verify(body, header) via internal action
alt invalid signature
Verif-->>HTTP: {ok:false}
HTTP-->>Provider: 401
else valid signature
Verif-->>HTTP: {ok:true}
HTTP->>HTTP: parse JSON, filter reversal events
alt non-reversal
HTTP-->>Provider: 200 { ignored: true }
else reversal
HTTP->>Orch: handlePaymentReversal(payload)
Orch->>DB: getAttemptByProviderRef(providerRef)
alt attempt not found
DB-->>Orch: undefined
Orch-->>HTTP: { success:false, reason:"attempt_not_found" }
else attempt found
DB-->>Orch: attempt
Orch->>SM: processReversalCascade(attemptId, metadata)
SM->>DB: execute PAYMENT_REVERSED transition
SM-->>Orch: { success:true }
Orch-->>HTTP: { success:true, attemptId }
end
HTTP-->>Provider: 200 JSON
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ 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.
Connorbelez has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
There was a problem hiding this comment.
Pull request overview
Adds Convex webhook endpoints and supporting infrastructure to process payment reversal events from Rotessa and Stripe by verifying signatures, normalizing provider payloads, and triggering the GT PAYMENT_REVERSED transition.
Changes:
- Added signature verification utilities (plus internalActions) for Rotessa and Stripe webhooks.
- Implemented Rotessa/Stripe
httpActionhandlers and registered routes inconvex/http.ts. - Introduced shared reversal handling (
handlePaymentReversal,processReversalCascade) and unit tests for signature verification + payload mapping.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| specs/ENG-175/tasks.md | Task checklist for ENG-175 workstream. |
| specs/ENG-175/chunks/manifest.md | Chunk breakdown and dependencies. |
| specs/ENG-175/chunks/chunk-01-infrastructure-core/tasks.md | Chunk-01 task list. |
| specs/ENG-175/chunks/chunk-01-infrastructure-core/context.md | Implementation plan/context for infra + core logic. |
| specs/ENG-175/chunks/chunk-02-provider-handlers-router/tasks.md | Chunk-02 task list. |
| specs/ENG-175/chunks/chunk-02-provider-handlers-router/context.md | Implementation plan/context for provider handlers + router wiring. |
| specs/ENG-175/chunks/chunk-03-tests/tasks.md | Chunk-03 test task list. |
| specs/ENG-175/chunks/chunk-03-tests/context.md | Test plan/context. |
| convex/payments/webhooks/verification.ts | Signature verification helpers + internalActions for Node crypto. |
| convex/payments/webhooks/utils.ts | Shared JSON response helper for webhook handlers. |
| convex/payments/webhooks/types.ts | Shared normalized payload/result types for reversal handling. |
| convex/payments/webhooks/rotessa.ts | Rotessa webhook handler + payload normalization helpers. |
| convex/payments/webhooks/stripe.ts | Stripe webhook handler + payload normalization helpers. |
| convex/payments/webhooks/processReversal.ts | Internal mutation to fire PAYMENT_REVERSED transition. |
| convex/payments/webhooks/handleReversal.ts | Shared orchestration: lookup attempt by providerRef, validate state, invoke mutation. |
| convex/payments/webhooks/tests/handleReversal.test.ts | Unit tests for signature verification functions. |
| convex/payments/webhooks/tests/rotessaWebhook.test.ts | Unit tests for Rotessa event filtering + payload mapping helpers. |
| convex/payments/webhooks/tests/stripeWebhook.test.ts | Unit tests for Stripe event filtering + payload mapping helpers. |
| convex/http.ts | Registers /webhooks/rotessa and /webhooks/stripe routes. |
💡 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: 2
🧹 Nitpick comments (4)
convex/payments/webhooks/handleReversal.ts (1)
43-50: Consider logging for observability on "attempt_not_found".When no matching attempt is found, the handler silently returns failure. Adding a warning log would help debug cases where
providerRefmapping is misconfigured or events arrive for untracked transactions.📋 Optional: Add observability logging
if (!attempt) { + console.warn( + `[handlePaymentReversal] No attempt found for providerRef: ${payload.providerRef}` + ); return { success: false, reason: "attempt_not_found" }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/webhooks/handleReversal.ts` around lines 43 - 50, When the lookup in handleReversal returns no attempt (the block checking !attempt after calling internal.payments.webhooks.handleReversal.getAttemptByProviderRef), add an observability warning before returning: emit a warning-level log that includes the providerRef (payload.providerRef) and any minimal identifying payload/context (e.g., event type or timestamp) using the project's logger (e.g., processLogger.warn or ctx.log.warn if available), then return the same { success: false, reason: "attempt_not_found" } to preserve behavior.convex/payments/webhooks/verification.ts (1)
19-26: Consider validating hex encoding before buffer conversion.
Buffer.from(signature, "hex")silently drops non-hex characters (e.g.,"abc!"→Buffer.from("abc", "hex")→ 1 byte). While the length check catches most issues, a malformed signature like"aaaa!!!"would produce a 2-byte buffer that fails the length check anyway.This is low-risk since the length check provides a safety net, but explicit hex validation could make failures more debuggable.
🔧 Optional: Add explicit hex validation
export function verifyRotessaSignature( body: string, signature: string, secret: string ): boolean { + // Reject obviously invalid signatures early + if (!/^[0-9a-f]+$/i.test(signature)) { + return false; + } + const expected = createHmac("sha256", secret).update(body).digest("hex"); const sigBuffer = Buffer.from(signature, "hex");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/webhooks/verification.ts` around lines 19 - 26, Validate that the incoming hex strings are properly hex-encoded before calling Buffer.from: check that both signature and expected match a hex regexp (e.g., /^[0-9a-fA-F]+$/) and have even length, and return false (or throw) if validation fails; then proceed to create sigBuffer/expectedBuffer and call timingSafeEqual as before (referencing the signature and expected variables, Buffer.from usage, and timingSafeEqual) so malformed inputs are rejected explicitly rather than silently trimmed.convex/payments/webhooks/stripe.ts (2)
61-62: Minor: Consider trimming trailing separator when failure_message is empty.When
failure_messageis undefined/empty, this produces"ACH Failure: unknown — "with a trailing separator. This is a minor cosmetic issue but could be cleaned up.💅 Optional: Cleaner formatting
case "payment_intent.payment_failed": - return `ACH Failure: ${obj.failure_code ?? "unknown"} — ${obj.failure_message ?? ""}`; + return obj.failure_message + ? `ACH Failure: ${obj.failure_code ?? "unknown"} — ${obj.failure_message}` + : `ACH Failure: ${obj.failure_code ?? "unknown"}`;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/webhooks/stripe.ts` around lines 61 - 62, Update the "payment_intent.payment_failed" case in the webhook formatter so it doesn't leave a trailing " — " when obj.failure_message is empty; use the existing symbols (the switch case for "payment_intent.payment_failed" and obj.failure_code / obj.failure_message) and return "ACH Failure: <code>" plus " — <message>" only when obj.failure_message is truthy, otherwise omit the separator and message.
9-26: Consider extending the type to match Stripe's actual event structure.The
StripeWebhookEventinterface is a minimal subset that works for the current use cases. However, Stripe's actual event structure has additional fields likelivemode,api_version, and nested object types vary significantly between event types (e.g.,charge.dispute.createdhas aDisputeobject, not aCharge).This minimal typing works but may cause issues if you need to access additional fields later. Consider documenting that this is intentionally a minimal subset, or verify field availability at runtime for less common event types.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@convex/payments/webhooks/stripe.ts` around lines 9 - 26, The StripeWebhookEvent interface is too minimal; update StripeWebhookEvent to include common top-level fields (e.g., livemode?: boolean, api_version?: string | null, request?: { id?: string | null }) and make data.object generic/union (e.g., data: { object: T } or data: { object: Charge | Dispute | any }) so different event types (charge, dispute, payment_intent) are represented, or alternatively add a clear comment above StripeWebhookEvent stating it is an intentional minimal subset and ensure handlers (wherever StripeWebhookEvent is used) perform runtime checks for fields like status, failure_code, metadata before access (reference: StripeWebhookEvent).
🤖 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/rotessa.ts`:
- Line 65: The current assignment to originalAmount uses
Math.round(event.data.amount * 100) which can suffer floating-point precision
errors; update the calculation for originalAmount (in rotessa.ts where
originalAmount is set from event.data.amount) to first normalize the amount to
two decimal places (e.g., use toFixed(2) on event.data.amount and parse that
string back to a number) and then multiply by 100 and round so cents are
computed deterministically without IEEE-754 artifacts.
In `@specs/ENG-175/chunks/chunk-01-infrastructure-core/context.md`:
- Around line 88-168: The spec text incorrectly states that
processReversalCascade directly calls postPaymentReversalCascade before
executeTransition for atomicity; update the spec to match the implementation:
state that processReversalCascade (internalMutation handler) triggers
executeTransition for eventType "PAYMENT_REVERSED" and the emitPaymentReversed
effect calls postPaymentReversalCascade for each obligation, and adjust the
described function signature/payload to remove obligationId and mortgageId and
to omit postingGroupId and clawbackRequired (since those are produced by the
effect), or alternatively, if you prefer the original atomic design, modify the
implementation of processReversalCascade to call postPaymentReversalCascade(ctx,
{...}) (preserving CommandSource actor info) before calling executeTransition so
the handler includes obligationId and mortgageId in its args and passes
postingGroupId/clawbackRequired in the transition payload; reference
processReversalCascade, postPaymentReversalCascade, emitPaymentReversed, and
executeTransition when making the alignment.
---
Nitpick comments:
In `@convex/payments/webhooks/handleReversal.ts`:
- Around line 43-50: When the lookup in handleReversal returns no attempt (the
block checking !attempt after calling
internal.payments.webhooks.handleReversal.getAttemptByProviderRef), add an
observability warning before returning: emit a warning-level log that includes
the providerRef (payload.providerRef) and any minimal identifying
payload/context (e.g., event type or timestamp) using the project's logger
(e.g., processLogger.warn or ctx.log.warn if available), then return the same {
success: false, reason: "attempt_not_found" } to preserve behavior.
In `@convex/payments/webhooks/stripe.ts`:
- Around line 61-62: Update the "payment_intent.payment_failed" case in the
webhook formatter so it doesn't leave a trailing " — " when obj.failure_message
is empty; use the existing symbols (the switch case for
"payment_intent.payment_failed" and obj.failure_code / obj.failure_message) and
return "ACH Failure: <code>" plus " — <message>" only when obj.failure_message
is truthy, otherwise omit the separator and message.
- Around line 9-26: The StripeWebhookEvent interface is too minimal; update
StripeWebhookEvent to include common top-level fields (e.g., livemode?: boolean,
api_version?: string | null, request?: { id?: string | null }) and make
data.object generic/union (e.g., data: { object: T } or data: { object: Charge |
Dispute | any }) so different event types (charge, dispute, payment_intent) are
represented, or alternatively add a clear comment above StripeWebhookEvent
stating it is an intentional minimal subset and ensure handlers (wherever
StripeWebhookEvent is used) perform runtime checks for fields like status,
failure_code, metadata before access (reference: StripeWebhookEvent).
In `@convex/payments/webhooks/verification.ts`:
- Around line 19-26: Validate that the incoming hex strings are properly
hex-encoded before calling Buffer.from: check that both signature and expected
match a hex regexp (e.g., /^[0-9a-fA-F]+$/) and have even length, and return
false (or throw) if validation fails; then proceed to create
sigBuffer/expectedBuffer and call timingSafeEqual as before (referencing the
signature and expected variables, Buffer.from usage, and timingSafeEqual) so
malformed inputs are rejected explicitly rather than silently trimmed.
🪄 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: 79afa3e1-94ed-4311-b63d-3669b3bd9a57
📒 Files selected for processing (19)
convex/http.tsconvex/payments/webhooks/__tests__/handleReversal.test.tsconvex/payments/webhooks/__tests__/rotessaWebhook.test.tsconvex/payments/webhooks/__tests__/stripeWebhook.test.tsconvex/payments/webhooks/handleReversal.tsconvex/payments/webhooks/processReversal.tsconvex/payments/webhooks/rotessa.tsconvex/payments/webhooks/stripe.tsconvex/payments/webhooks/types.tsconvex/payments/webhooks/utils.tsconvex/payments/webhooks/verification.tsspecs/ENG-175/chunks/chunk-01-infrastructure-core/context.mdspecs/ENG-175/chunks/chunk-01-infrastructure-core/tasks.mdspecs/ENG-175/chunks/chunk-02-provider-handlers-router/context.mdspecs/ENG-175/chunks/chunk-02-provider-handlers-router/tasks.mdspecs/ENG-175/chunks/chunk-03-tests/context.mdspecs/ENG-175/chunks/chunk-03-tests/tasks.mdspecs/ENG-175/chunks/manifest.mdspecs/ENG-175/tasks.md
… on processing errors - Verification actions now return discriminated union: ok | missing_secret | invalid_signature - Missing webhook secret returns HTTP 500 (server misconfiguration) - Invalid signature returns HTTP 401 (unauthorized) - Reversal processing errors return HTTP 200 with structured error payload to prevent retry storms - Fixed floating-point precision in Rotessa currency conversion (IEEE 754 guard) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Wrap emitPaymentReversed effect in @convex-dev/workflow durable workflow with automatic retries. Ensures cash-ledger reversal entries are posted even if the initial scheduled mutation fails. Idempotent via postingGroupId. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
10 tests covering: confirmed→reversed transition, provider ref lookup, duplicate/idempotent webhook handling, out-of-order rejection, reversal journal entries, cash ledger balance verification, posting group integrity. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update processReversalCascade spec to reflect actual architecture: effect-driven via emitPaymentReversed, not direct postPaymentReversalCascade call. Remove obligationId/mortgageId from signature, postingGroupId/clawbackRequired from payload. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Merged
Connorbelez
added a commit
that referenced
this pull request
Mar 27, 2026
eng-175 (#283) ### TL;DR Added webhook infrastructure for handling payment reversals from Rotessa and Stripe, including signature verification, shared reversal processing logic, and comprehensive test coverage. ### What changed? Added webhook endpoints `/webhooks/rotessa` and `/webhooks/stripe` to handle payment reversal events from both providers. The implementation includes: - **Signature verification** for both Rotessa (HMAC-SHA256) and Stripe (timestamped HMAC) webhooks using Node.js crypto - **Shared reversal processing** that looks up collection attempts, validates state, and fires GT transitions to reverse payments - **Provider-specific handlers** that parse webhook payloads and normalize them into a common format - **Comprehensive test suite** covering signature verification, payload mapping, reversal logic, and edge cases like duplicate webhooks and invalid states The system processes NSF, PAD returns, disputes, and manual reversals by transitioning collection attempts from "confirmed" to "reversed" state and posting cash ledger reversal entries. ### How to test? Run the test suite with `npm test` to verify: - Signature verification for both providers - Webhook payload parsing and normalization - Reversal processing logic including idempotency - Edge cases like already-reversed attempts and missing collection attempts For manual testing, configure `ROTESSA_WEBHOOK_SECRET` and `STRIPE_WEBHOOK_SECRET` environment variables and send POST requests to the webhook endpoints with properly signed payloads. ### Why make this change? This enables automated handling of payment reversals from external providers, ensuring the system stays in sync when payments fail or are disputed. The webhook handlers provide real-time processing of NSF events, PAD returns, and disputes, automatically updating collection attempt states and posting appropriate cash ledger entries to maintain accurate financial records. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Transfer domain: create/initiate transfers, lifecycle state machine, manual transfer provider, and bridge for bridged transfers * VoPay webhook endpoint for incoming transfer status updates * **Infrastructure** * Cash-ledger posting for receipts and lender payouts; schema extended with transfer fields and idempotency * Reconciliation cron with retry/escalation and healing workflows * **Tests & Docs** * Extensive unit and state-machine tests; added Cash & Obligations Ledger developer guide <!-- end of auto-generated comment: release notes by coderabbit.ai --> eng-184
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 webhook infrastructure for handling payment reversals from Rotessa and Stripe, including signature verification, shared reversal processing logic, and comprehensive test coverage.
What changed?
Added webhook endpoints
/webhooks/rotessaand/webhooks/stripeto handle payment reversal events from both providers. The implementation includes:The system processes NSF, PAD returns, disputes, and manual reversals by transitioning collection attempts from "confirmed" to "reversed" state and posting cash ledger reversal entries.
How to test?
Run the test suite with
npm testto verify:For manual testing, configure
ROTESSA_WEBHOOK_SECRETandSTRIPE_WEBHOOK_SECRETenvironment variables and send POST requests to the webhook endpoints with properly signed payloads.Why make this change?
This enables automated handling of payment reversals from external providers, ensuring the system stays in sync when payments fail or are disputed. The webhook handlers provide real-time processing of NSF events, PAD returns, and disputes, automatically updating collection attempt states and posting appropriate cash ledger entries to maintain accurate financial records.
Summary by CodeRabbit
New Features
Reliability / Security
Tests