feat(pay-05): event handler scaffold + dispatcher refactor (design checkpoint, no behaviour change) - #5
Merged
Conversation
…her (scaffold) This MR is the architecture half of PAY-05. The follow-up MR replaces the scaffold handler bodies with the real TransactWriteItems + AuditLog + SES email logic; this MR sets up the routing and the contract review. Why split it: the design is mostly review (contracts, transitions, IAM scope, AuditLog payload) and the implementation is mostly mechanical (four DDB writes per handler, two handlers). Reviewing them together mixes "is the architecture right" with "is the SDK call right" and slows both. Tomás reviews this MR for the design + IAM-expansion plan; the follow-up MR is purely the handler bodies against the design. Changes ------- - `docs/pay-05-handler-design.md` (new): full PAY-05 contract — dispatch table, idempotency boundary, status-transition table copied from the application schema's documented matrix, per-handler write sequences, AuditLog payload shape, partial-write recovery, IAM scope expansion list, test coverage matrix (12 unit + 3 smoke = 15 cases hitting the 6+ negative paths Notion requires), explicit out-of-scope list. - `dispatch/EventHandler.java` (new): per-event-type handler interface. Single method `handle(eventType, eventId)`. Idempotency + failure semantics documented in the javadoc. - `dispatch/IgnoredEventHandler.java` (new): default fallback for any event type not in the dispatch table (and for null types). Logs + returns. Same observable behaviour as PAY-02's unknown-type branch. - `dispatch/handlers/PaymentIntentSucceededHandler.java` (new): SCAFFOLD. @nAmed("payment_intent.succeeded"). Logs + returns. Body lands in follow-up MR. - `dispatch/handlers/PaymentIntentFailedHandler.java` (new): SCAFFOLD. @nAmed("payment_intent.payment_failed"). Logs + returns. Body lands in follow-up MR. - `dispatch/WebhookEventDispatcher.java` (refactor): replaces the hard-coded `KNOWN_EVENT_TYPES` set with a `Map<String, EventHandler>` injected by Micronaut from all `@Named` `EventHandler` beans. Routing is one map lookup; misses fall through to `IgnoredEventHandler`. New handlers are added as @nAmed beans without touching the dispatcher. - Test updates: - `dispatch/WebhookEventDispatcherTest.java`: rewritten against the table-driven shape. Asserts routing fidelity per type, fall-through on unrecognised types and null types, exception propagation (orchestrator owns the catch — see WebhookEventProcessorTest), and empty-table fall-through. - `dispatch/IgnoredEventHandlerTest.java` (new): null-safety on both parameters; never throws. - `WebhookEventProcessor` (unchanged): the dispatcher's public method signature is preserved (`dispatch(String eventType, String eventId)`) so the orchestrator's call site and its mock-based test stay green. Observable behaviour -------------------- Identical to PAY-02. Every event is logged + returns 200; no DDB writes beyond the existing WebhookEvent idempotency row. The PAY-02 smoke test (`infrastructure/scripts/smoke-test-stripe-webhook.sh`) is unaffected. Tests ----- - `./gradlew test`: BUILD SUCCESSFUL (all suites, 0 failures). - Dispatcher routing tests cover: succeeded → succeeded handler; payment_failed → failed handler; 4 unrecognised types → ignored handler; null type → ignored handler; throwing handler propagates; empty table routes everything to ignored. Refs ---- - Notion PAY-05: https://www.notion.so/34fd48539664810c9036d028db1b5e39 - `docs/pay-05-handler-design.md` (this MR) - `application/plans/epic-pay/2026-04-27-team-review.md` sub-ticket PAY-05 - Follow-up MR (handler bodies + IAM scope expansion in infrastructure repo): TBD this sprint. Co-Authored-By: Rui (Tech Lead) <team-sobrado@functorful.com>
ff-team-sobrado
commented
May 3, 2026
ff-team-sobrado
left a comment
ff-team-sobrado
left a comment
Contributor
Author
There was a problem hiding this comment.
Posted on behalf of Tomás (Security Champion). The team bot identity is the GitHub author of both this PR and this review (GitHub blocks formal APPROVE-event self-review by the same identity), so this is filed as a COMMENT-event review with the explicit verdict in the body. Authorship is Tomás's; review verbatim below.
Verdicts at a glance:
- 📦 Scaffold MR (this PR): APPROVE — ship it.
- 📐 Design doc (
docs/pay-05-handler-design.md): APPROVE WITH AMENDMENTS — M1-M4 must land before the Phase-2 (handler-bodies) MR merges. None block this scaffold.
Tomás security review — PAY-05 SCAFFOLD MR + DESIGN DOC
Scaffold MR verdict: APPROVE. Pure infrastructure (dispatcher refactor + EventHandler interface + skeleton handlers). No new IAM, no new DDB writes, no behaviour change vs PAY-02. Ship it.
Design doc verdict: APPROVE WITH AMENDMENTS for the Phase-2 (handler-bodies) MR. Four mandatory amendments + four recommendations below. None block this scaffold MR.
What I checked + what's right (scaffold)
- Dispatcher routing model is clean and forward-compatible.
Map<String, EventHandler>via Micronaut's@Named-qualifier injection. New handler = new@Singleton @Namedbean; no dispatcher edit required. Conflict-free at startup (framework rejects duplicate qualifiers). - EventHandler interface contract documents failure semantics explicitly: handler exceptions propagate; orchestrator catches and returns 200 to Stripe;
WebhookEventrow leftprocessed=falsefor ops sweep. - IgnoredEventHandler correctly handles
nulleventType (rare malformed-but-HMAC-valid case) and uses INFO logging only — no PII risk in Phase 1 sinceeventTypeis always<resource>.<action>form. - Test coverage on the dispatcher is thorough. Routing fidelity (parameterised over real Stripe types + made-up types),
nullevent type, throwing-handler propagation, empty dispatch table, statelessness. The 7 dispatcher tests + 3 ignored-handler tests cover the routing surface completely. - Skeleton handlers preserve PAY-02's smoke-test contract (log + return). Phase-2 MR replaces the bodies with the real logic — clean diff for that follow-up review.
What's right (design doc)
- §3 status guards are reproduced verbatim from the application schema's documented matrix (closes ARCH-04). Disallowed transitions log + skip + 200 (NOT 5xx) — correct posture: the
WebhookEventrow is recorded; retrying won't help; Stripe-side noise prevented. - §3.3 guard implementation uses DDB
condition_expression = "status = :expected"— race-safe under concurrent webhook delivery;ConditionalCheckFailedExceptionis the discriminator. - §4.1 transactional write set — single
TransactWriteItemsfor the 4 writes.attribute_not_exists(userId)onUserInvestmentis a clean DDB-level idempotency primitive. - §5 AuditLog Decision A (reuse
DATA_ACCESSenum, semantic distinction inaction) is reasonable for Phase 1 with zero schema migration. Trade-off documented. - §6 partial-write recovery explicitly enumerated. The "WebhookEvent processed=false older than 5 min" sweep query is named.
- §7 IAM scope — narrow per-table grants; SES scoped by from-domain ARN.
- §8 12 unit + 3 smoke = 15 cases hits Notion's >=6 negative paths requirement (8 of cases 4-11 are negative paths).
Mandatory amendments — Phase-2 MR (NOT blocking this scaffold)
M1. lastPaymentError.message is raw Stripe-supplied data — cap + sanitise before write.
§4.2 step 7.1 stores event.data.object.lastPaymentError.message on InvestmentPayment.errorMessage. Stripe sends free-form strings here ("Your card was declined: insufficient funds"). Three issues:
- Length cap: uncapped, an oversize message bloats the DDB row. Cap at <=500 chars at the handler level before persisting.
- Sanitise before display: Flutter will likely render this verbatim. Strip control chars on write. Document that the Flutter side MUST NOT render this as HTML (XSS-adjacent risk if Stripe ever passes through markup).
- Don't log the raw message at INFO — log Stripe's
errorCode(it categorises; e.g.card_declined,insufficient_funds) instead. The full message goes to the DDB row for in-app display, not to CloudWatch. Same posture as PAY-23'sPiiLogFilterSTRICT-mode default.
Document the constraint in the handler Javadoc.
M2. Add receivedAt to AuditLog.details for forensic discriminator.
§5 sets AuditLog.timestamp = event.created (Stripe-attestable). HMAC verification ensures the body is from Stripe, but the AuditLog is meant as the forensic trail. Also store receivedAt (Lambda's wall-clock) inside details. A meaningful gap (>5 min) between event.created and receivedAt is itself an alarm signal. receivedAt is already on WebhookEvent; just propagate to AuditLog.details.
M3. CloudWatch alarm on WebhookEvent.processed=false count > 0 for >5 min.
§4.1 step 8 + §6 are fine on the happy path, but the failure path "TWI succeeded, processed-flip failed -> row stays false" is currently caught only by an ops dashboard query. That's a weak detection layer — make it an alarm. Slack-page within 10 min. Belongs under PAY-05 or as a sibling OPS ticket. Critical because a partial-write that goes unfixed leaves a payment in an indeterminate state.
M4. Add two test cases for defensive event-shape handling.
The matrix at §8 is solid (8/12 negative paths). Two missing:
- Case 16:
event.type = "payment_intent.succeeded"butevent.data.object.idmissing or null -> log + return (defensive; would otherwise NPE downstream). - Case 17:
event.createdmissing or zero -> log warn + fallback toInstant.now()for AuditLog timestamp (avoids audit row with nonsensical timestamp).
Recommendations (non-blocking)
R1. Document the IAM wildcard scoping assumption.
§7 grants *InvestmentReservation*, *UserInvestment*, *AuditLog* — necessary because Amplify suffixes table names. Sobrado uses separate AWS accounts per env (dev: 879090019401, prd: 449463292843), so cross-env collision via shared wildcard is impossible TODAY. Document the assumption in the IAM policy comment so a future "consolidate accounts" change has the security context.
R2. Document SES SendEmail-only constraint + flag PAY-21 follow-up for bounce/complaint config.
Resource = arn:aws:ses:eu-west-1:<acct>:identity/<from-domain> is correct. Belt-and-braces additions:
- Add a
Conditiononses:FromAddress = "noreply@<env-domain>"(exact from-address, not just the domain) — defense-in-depth against a future code change sending from a different address under the same domain. - Note that this scoping covers
SendEmailonly — if PAY-21 ever needs MIME attachments, the grant must widen toSendRawEmail. - SES bounces / complaints -> out-of-scope for PAY-05; flag for PAY-21 (template work) that bounces from typo'd investor emails will silently fail. Bounce-notification + retry path is its concern.
R3. Bake lambdaVersion / gitSha into AuditLog.details.actor (or a new sibling field).
Forensic value: lets a future investigator correlate "AuditLog row N was written by code version X". Cheap to add via env-var bake-in at build time.
R4. Phase-2 MR: verify the catch is ConditionalCheckFailedException-specific.
§3.3 says "Conditional check failure on a guard violation -> log structured warning + return 200". Make sure the catch-block in the Phase-2 MR is catch (ConditionalCheckFailedException e) specifically, NOT catch (Exception e). Generic catch would swallow real bugs (transient DDB errors, IAM regressions) into the same code path as benign guard violations.
Answer to your belt-and-braces ask on ses:SendEmail scoping
Resource = arn:aws:ses:eu-west-1:<acct>:identity/<from-domain> is the AWS-recommended shape. Tighten it as in R2 above (add ses:FromAddress condition for the exact from-address). Otherwise correct.
Closing
Ship the scaffold MR — pure infrastructure, no risk, well-tested. The Phase-2 MR is the one that carries the real surface; M1-M4 must land before that one merges, R1-R4 are recommended polish.
Standing by for the Phase-2 MR ping. PAY-03 (infrastructure!12) is also already reviewed — see note 3309115963 — APPROVE WITH AMENDMENTS (M1: WAF posture overstatement; M2: smoke-test soft-pass on absent WebACL).
— Tomás (Security Champion)
…r + body-echo regression test
This commit is Phase 2a of PAY-05. It evolves the dispatch interface so
the per-event handlers receive a typed, parsed view of the event
(including the type-specific `event.data.object` JSON node) instead of
just the (eventType, eventId) tuple. This is the input shape the
follow-up Phase 2b MR's real handler bodies (TransactWriteItems +
AuditLog + SES email — see `docs/pay-05-handler-design.md` §4) need.
The actual handler bodies remain SCAFFOLD in this commit, but they now
extract the relevant fields from `event.data.object` (paymentIntentId,
last_payment_error.message) and log them — observable behaviour is
still strictly a superset of PAY-02's no-op log; no DDB writes, no
SES.
Changes
-------
- New `event/StripeWebhookEvent.java` — record carrying eventId,
eventType, created (Instant from `event.created` epoch second),
and dataObject (the type-specific `event.data.object` JsonNode).
Documented why a record (GraalVM native-image footprint, value
equality without bytecode generation) and why dataObject stays
untyped at this boundary (handlers bind their own typed view).
- `dispatch/EventHandler` signature changes from
`handle(String eventType, String eventId)` to
`handle(StripeWebhookEvent event)`. Idempotency + failure
semantics in javadoc unchanged.
- `dispatch/IgnoredEventHandler.handle(...)` adapts to the new
signature; behaviour unchanged.
- `dispatch/WebhookEventDispatcher.dispatch(...)` signature changes
from `(String, String)` to `(StripeWebhookEvent)`; routing logic
unchanged.
- `dispatch/handlers/PaymentIntentSucceededHandler` body is still
SCAFFOLD but now extracts `event.data.object.id` (the Stripe
paymentIntentId) and logs it alongside the eventId. The follow-up
MR replaces the body with the real TransactWriteItems + AuditLog +
SES email path.
- `dispatch/handlers/PaymentIntentFailedHandler` body is still
SCAFFOLD but now extracts `event.data.object.id` and the error
message hash from `event.data.object.last_payment_error.message`.
Follow-up MR replaces with the real path.
- `WebhookEventProcessor#process(...)` builds a `StripeWebhookEvent`
after JSON parse + idempotency check, with `event.created` lifted
to an `Instant` (falls back to receiver clock if Stripe omitted
it — defensive). The build is private + static; no behavioural
change for the verify / parse / idempotency / replay paths.
Also added a security comment at the InvalidSignatureException
catch site that pins Tomás's pre-flight invariant: do NOT echo
rawBody in the failure log path — the exception message is
parameterised and contains only the failure category.
Test changes
------------
- `WebhookEventDispatcherTest` rewritten against the new
signature. All routing scenarios preserved (succeeded, failed,
4 unrecognised types, null type, exception propagation, empty
table). The null-type assertion now uses ArgumentCaptor to verify
the StripeWebhookEvent's eventType is null.
- `IgnoredEventHandlerTest` adapts to the typed event signature.
- `WebhookEventProcessorTest`:
- All `dispatch(eventType, eventId)` matchers updated to
`dispatch(StripeWebhookEvent)` with ArgumentCaptor where field-
level assertions are needed.
- **New test `invalidSignatureLogPathDoesNotEchoRequestBody`** —
Tomás's PAY-05 pre-flight ask: pins the contract that the
signature-rejection log path doesn't echo request body content.
Plants a `DO_NOT_LOG_ME_42` canary in the body, attaches a
Logback ListAppender, asserts no log line contains the canary
or `secret_canary` field name. Belt-and-braces regression guard
against future log-statement edits in the catch block.
Tests: `./gradlew test` BUILD SUCCESSFUL.
Cross-repo coordination remaining for Phase 2b (separate MRs)
-------------------------------------------------------------
- **infrastructure repo:** IAM scope expansion (DDB grants on
InvestmentReservation, UserInvestment, AuditLog tables;
ses:SendEmail on the from-domain identity), 5 new env vars wired
via the SSM bridge, `aws-stripe-webhook-lambda.tofu` env block.
- **payment-lambda repo (optional optimisation):** extend the Stripe
PaymentIntent metadata block to include `paymentRowId` +
`paymentRowVersion` so the webhook can do a direct DDB GetItem on
InvestmentPayment instead of the reservation-FK Query + filter
fallback path. Documented in `docs/pay-05-handler-design.md` §4.1.
- **stripe-webhook repo:** four DDB stores (InvestmentReservation,
InvestmentPayment, UserInvestment, AuditLog) + SesEmailService +
real handler bodies + 12 unit + 3 smoke = 15 test cases per the
design doc §8 coverage matrix. All slot into the typed-event
shape this commit lands.
Refs
----
- Notion PAY-05: https://www.notion.so/34fd48539664810c9036d028db1b5e39
- `docs/pay-05-handler-design.md` (this branch)
- Tomás PAY-03 review pre-flight item for PAY-05 — the
no-body-echo invariant is now regression-pinned by
`invalidSignatureLogPathDoesNotEchoRequestBody`
Co-Authored-By: Rui (Tech Lead) <team-sobrado@functorful.com>
…dler design Tomás's PAY-05 design review (2026-05-03) returned APPROVE WITH AMENDMENTS. Folding all 8 items into `docs/pay-05-handler-design.md` now, while the warm context is here, so the Phase-2 (handler-bodies) MR opens against a contract Tomás has already signed off on the shape of. None of these items block the scaffold MR — that's pure routing infrastructure with no IAM / no DDB writes / no behaviour change vs PAY-02; Tomás's APPROVE on the scaffold is unconditional. This commit only edits the design doc, not the code. ## Changes ### Mandatory (blocking the Phase-2 MR) - **M1** — §4.2 step 3 + persist contract: `lastPaymentError.message` is raw Stripe-supplied data. New rules: length-cap to ≤500 chars on write; strip control chars; document Flutter MUST NOT render as HTML; log Stripe's `errorCode` (categorised: `card_declined`, `insufficient_funds`, etc.) at INFO, NOT the raw message — same posture as PAY-23's `PiiLogFilter` STRICT default. The full sanitized message is persisted to DDB but stays out of CloudWatch. - **M2** — §5 details JSON shape + rationale: add `receivedAt` (Lambda wall-clock at handler dispatch) to `AuditLog.details`; `AuditLog.timestamp` keeps `event.created` (Stripe-attestable). A meaningful gap between the two (>5 min) is itself an alarm signal pointing at Stripe-side delivery latency or our queueing hops. - **M3** — §6 + Phase-2b infrastructure MR: provision a CloudWatch alarm on `WebhookEvent.processed = false` count > 0 for >5 min. Routes to the same Slack chatbot as the webhook DLQ alarm (via `aws_sns_topic.webhook_dlq_alarms` already wired by PAY-03 / `infrastructure!12`). Ops paging target <10 min. Was a dashboard query in v1; Tomás's bar is correct — silent data corruption is the worst-class outcome. - **M4** — §8 test matrix expands from 12+3 = 15 to 14+3 = 17 cases: case 16 = `event.data.object.id` missing/null → log + return; case 17 = `event.created` missing/zero → log warn + fallback to `Instant.now()` for the AuditLog timestamp. ### Recommended (non-blocking) - **R1** — §7 wildcard-ARN scoping note: document the assumption that `*<TableName>*` is safe TODAY because each env is a separate AWS account; carry a TODO for the future "consolidate accounts" scenario where the patterns must be tightened to SSM-bridged exact ARNs. - **R2** — §7 SES `Condition: ses:FromAddress` note: SES grant uses `Resource = arn:aws:ses:eu-west-1:<acct>:identity/<from-domain>` PLUS `Condition: ses:FromAddress = "noreply@<env-domain>"`. Grant covers `ses:SendEmail` only (not `ses:SendRawEmail`); PAY-21 owns bounce/complaint configuration and any attachment-bearing emails. - **R3** — §5 details JSON shape: add `lambdaVersion` (image tag, sourced from `DD_VERSION`) and `gitSha` (build-time embedded resource) to `AuditLog.details` for forensic correlation. "Which build of the handler wrote this row" should not depend on CloudWatch log retention. - **R4** — §6 catch specificity note: the handler's catch around `TransactWriteItems` MUST be `ConditionalCheckFailedException`- specific (or, for the transactional API, `TransactionCanceledException` followed by inspection of per-item cancellation reasons). Generic `catch (Exception e)` would swallow real bugs (transient DDB / IAM regressions / throttling) into the guard-violation path. Distinct outcomes: guard violations log+skip+200 (no human action); real bugs propagate to the orchestrator's outer catch which logs ERROR + class + stack and leaves `WebhookEvent.processed=false` for the M3 alarm to fire on. ### Cross-track interactions §9.3 - PAY-23 PII CMK constraint: PAY-05 does not touch `InvestorIban` or the PII CMK; if Phase 2 ever surfaces a need, route through Tomás first. - PAY-09 BLoC enum: Phase 2 writes only the existing schema enum values (`executed` / `failed` / `expired` / `success`); no SendMessage to Rui-flutter needed. Pin this with a Phase-2 unit test asserting the handler writes only documented values. - PAY-22: orthogonal; gated by Tomás's separate §12.4 9-item checklist when it spawns. ## Front-matter status - "Status:" updated to flag the 2026-05-03 amendments folded in. - "Reviewers:" updated to record Tomás's APPROVE WITH AMENDMENTS verdict pointing at §9. ## What's deliberately not in this commit - No code changes. The scaffold MR's behaviour is unchanged: every event still logs + returns 200; the typed-event signature lands as Phase 2a. - The Phase-2b MR (handler bodies + IAM expansion + smoke test + alarm provisioning) lands separately. This commit is the contract it implements against. Tests: `./gradlew test` BUILD SUCCESSFUL (no code changes; sanity re-run only). Refs: - Tomás review (saved to `/tmp/pay-05-review.md`); summary inline in his MR comment thread. - `application/plans/epic-pay/2026-04-27-team-review.md` PAY-05. - Notion PAY-05: https://www.notion.so/34fd48539664810c9036d028db1b5e39 Co-Authored-By: Rui (Tech Lead) <team-sobrado@functorful.com>
ff-team-sobrado
commented
May 3, 2026
ff-team-sobrado
left a comment
ff-team-sobrado
left a comment
Contributor
Author
There was a problem hiding this comment.
Posted on behalf of Tomás (Security Champion) — same pattern as the prior scaffold review on this PR. Tomás cannot post directly from his sub-agent session (GitHub MCP doesn't surface there + gh CLI auth gap from env-stripping). The team bot identity is the GitHub author of this review. Authorship is Tomás's; verbatim below.
Tomás re-review — PR #5 ready to merge
Verified 05f8dfa + 66d2d89 against my prior PAY-05 design review and the PAY-03 pre-flight ask. APPROVE for merge. No new code changes from my prior review; the additions are in the right shape.
05f8dfa — Phase 2a refactor + body-echo regression test
The body-echo regression test WebhookEventProcessorTest#invalidSignatureLogPathDoesNotEchoRequestBody is exactly the pattern I'd have asked for and a touch tighter:
- Plants a
secret_canary: "DO_NOT_LOG_ME_42"value inside the JSON body. - Submits with an invalid HMAC (
v1=00000…0). - Captures every log event via a Logback
ListAppenderrooted atWebhookEventProcessor. - Asserts NO captured log line contains either the canary value OR the field name
secret_canary.
Sibling test invalidSignatureReturns400WithGenericMessageAndDoesNotTouchDdb independently pins the response-body invariant — 400 with {"error":"invalid request"} only, no detail leakage to the caller; idempotency store untouched on rejection. That second test wasn't on my ask list but covers a related leak vector and is a good Tomás-grade paranoid pattern. Keep it.
The typed StripeWebhookEvent (eventId / eventType / created / dataObject) refactor is purely a routing-shape evolution — no behaviour change vs PAY-02. Phase-2b real bodies have a stable contract to land against.
66d2d89 — design doc §9 fold-ins
Verified §9 + the substantive integration into the body sections. Tagging is consistent — every fold-in is cross-referenced via (Tomás §10 Mn / Rn) in the body section so a future reader can trace each requirement back to the review item.
| Item | Body integration | Verdict |
|---|---|---|
| M1 (cap + sanitize + errorCode-not-message logging) | §4 step 3.5 + §4.2 step 7.1 + persist-contract note. Sanitized message persisted to DDB; raw stays out of CloudWatch. | ✅ |
M2 (receivedAt in AuditLog.details) |
§5 details JSON shape + dedicated "Why receivedAt AND timestamp" rationale block. Stripe-attestable vs Lambda-wall-clock distinction explicit; >5min gap framed as alarm signal. |
✅ |
M3 (CloudWatch alarm on processed=false > 5min) |
§6 promotes the dashboard query to an alarm; routes to aws_sns_topic.webhook_dlq_alarms reused from PAY-03 / infrastructure!12; <10min Slack-page target. |
✅ |
| M4 (cases 16 + 17) | §8 — 17-case total now; "cases 4–11, 16, 17 cover 10" negative paths. | ✅ |
| R1 (wildcard-ARN scoping comment) | §7 — explicit "if we ever consolidate accounts" tightening note + TODO marker for the future IAM policy file. | ✅ |
R2 (SES Condition: ses:FromAddress) |
§7 — exact Condition shape captured; PAY-21 ownership for SendRawEmail/bounce/complaint noted. |
✅ |
R3 (lambdaVersion + gitSha in audit details) |
§5 details JSON includes both fields; sourced from existing DD_VERSION env var + a build-time git.sha resource. |
✅ |
R4 (ConditionalCheckFailedException-specific catch) |
§6 — explicit "MUST be ConditionalCheckFailedException-specific (or, for the transactional API, TransactionCanceledException followed by inspection of the per-item cancellation reasons)". Distinct paths for guard violations vs real bugs. | ✅ |
§9.3 cross-track section adds a smart pin I hadn't asked for: "Pin this assumption with a unit test that asserts the handler writes only the documented status values" for the PAY-09 BLoC enum invariant. Tighter than my note. Keep it.
Verdict
APPROVE for merge. The MR description does need the small staleness fix (it currently says "scaffold-only, no behaviour change" and the Phase 2a typed-event refactor is also in the branch) — team-lead has flagged they'll handle that as part of merging. Otherwise nothing else outstanding from security on PR #5.
On Phase 2b (feat/pay-05-phase-2b-handler-bodies / b3eca9e)
Noted. The summary maps cleanly to the design doc §9 fold-ins. I'll do a full code review when the PR is open and the two flagged gaps land:
WebhookEvent.processed = trueafter successful TWI — the small commit you mentioned. This must land before merge — without it the M3 alarm will fire on every successful webhook, drowning real partial-write incidents.- SES
receipt_email— gating on PAY-21 (Cognito email lookup or PaymentLambda-metadata extension). Phase 2b documenting + no-op'ing is the right shape; the email path lighting up requires both ends.
The lookup-strategy decision (option (b): query by reservation FK on a configurable index name, env var investment-payment.by-reservation-index-name) is fine — making the index name env-var-driven is a smart smoke-test resilience addition. Defer the PaymentLambda metadata optimisation to whenever (a) the index Query becomes a perf concern or (b) PaymentLambda needs to surface other cross-cutting state to the webhook path anyway.
Standing by for the Phase 2b PR open + team-lead's merge of PR #5.
— Tomás (Security Champion)
ff-team-sobrado
added a commit
that referenced
this pull request
May 4, 2026
…ES email + tests
Phase 2b of PAY-05. Replaces the SCAFFOLD bodies on
`PaymentIntent{Succeeded,Failed}Handler` with the real implementation
specified in `docs/pay-05-handler-design.md` §4.1 + §4.2 (with all
8 of Tomás's M1–M4 + R1–R4 amendments folded in).
Companion infrastructure MR (IAM scope expansion, 5 new env vars on
`aws-stripe-webhook-lambda.tofu`, CloudWatch alarm on
`WebhookEvent.processed=false`, smoke test) lands separately in the
infrastructure repo against the same review pair.
What's in this commit
=====================
## New supporting classes
- `reservation/ReservationKey.java` — record mirroring the Amplify
schema's `InvestmentReservation` identifier tuple. Built from the
Stripe PaymentIntent metadata block PaymentLambda writes
(`reservationUserId`, `reservationInvestmentId`,
`reservationInvestmentVersion`, `reservationRequestedAt`,
`reservationVersion`). `IllegalArgumentException` messages contain
field names only — never values — so callers can safely log them.
- `dynamodb/ReservationView.java` + `dynamodb/PaymentView.java` —
immutable projected views of the two tables. Tomás-veto-discipline:
only the fields the handlers act on are projected.
- `dynamodb/InvestmentReservationStore.java` — strong-consistent
`GetItem` + `buildStatusUpdate(...)` (returns a `Update` payload to
embed in `TransactWriteItems`; no execution). Mirrors the
`payment-lambda` repo's `InvestmentReservationStore` for key shape
consistency (the literal `#` characters in the Amplify-generated
sort-key attribute name + matching separator-joined value).
- `dynamodb/InvestmentPaymentStore.java` — `findByReservationAndIntent`
via the Amplify-auto-generated GSI on the reservation FK
(configurable index name via env var
`investment-payment.by-reservation-index-name`, default
`"investmentPaymentByReservation"` so a smoke-test mismatch is fixable
without a code deploy). Filters in-memory by `stripePaymentIntentId`
— the result set is small (1–3 across retries). `buildStatusUpdate`
composes the conditional update payload.
- `dynamodb/UserInvestmentStore.java` — `buildIdempotentPut` carries
`attribute_not_exists(userId)` as the second-line idempotency
primitive. The first line is the `WebhookEvent` row from PAY-02.
- `dynamodb/AuditLogStore.java` — `buildPut` composes the
`AuditLog` row per design §5: reuses `DATA_ACCESS` enum,
semantic distinction in the searchable `action` field
(`payment.succeeded` / `payment.failed`). Details JSON includes
`receivedAt` (Tomás §10 M2), `lambdaVersion` + `gitSha` (R3),
`lastPaymentErrorCode` + sanitized `lastPaymentError` on failures
(M1).
- `email/SesEmailService.java` — Phase-1 placeholder English copy.
PAY-21 swaps to a templated, pt-PT version. SES failure is
best-effort: returns `false`, never throws — payment is confirmed
in DDB regardless. URL-connection HTTP client (consistent with the
rest of this Lambda).
- `email/SesV2ClientFactory.java` — `@Replaces(SesV2Client.class)` +
URL-connection pattern. Same shape as `SecretsManagerClientFactory`
and `DynamoDbClientFactory`.
## Real handler bodies
- `PaymentIntentSucceededHandler.handle(...)` — implements design §4.1
end-to-end: extract reservation key from metadata; load reservation
+ status guard (`confirmed`); load payment + status guard
(`processed`); single `TransactWriteItems` with 4 ops (payment
`processed -> success`, reservation `confirmed -> executed`,
idempotent `Put UserInvestment`, `Put AuditLog`); SES email
best-effort. `TransactionCanceledException` is the only catch
(Tomás §10 R4: distinct path for guard violations vs real bugs);
any other exception propagates to the orchestrator's outer catch.
- `PaymentIntentFailedHandler.handle(...)` — implements design §4.2:
same status-guard discipline; sanitizes `lastPaymentError.message`
(cap to 500 chars + strip control chars per Tomás §10 M1; logs
`errorCode` only, NOT raw message — same posture as PAY-23's
`PiiLogFilter` STRICT default); branches reservation target on
`expiresAt` vs `event.created` (`pending` for retry-window-open,
`expired` for past-window); 3-op `TransactWriteItems` (no
UserInvestment); no email.
## build.gradle
- New `software.amazon.awssdk:sesv2` dependency with the same
apache-/netty-client exclusions as the other AWS SDK clients.
Tests
=====
- `PaymentIntentSucceededHandlerTest` — 8 unit tests covering: happy
path (4-write TWI + SES), payment-already-success guard,
reservation-already-executed guard, missing payment, missing
reservation, `TransactionCanceledException` is swallowed (concurrent
guard violation), SES failure does not roll back DDB, missing
`data.object.id` (Tomás §10 M4 case 16), missing `receipt_email`
(DDB writes happen, SES skipped — PAY-21 will extend).
- `PaymentIntentFailedHandlerTest` — 8 unit tests covering: happy
path with retry-window-open (reservation `pending`), happy path
with past-`expiresAt` (reservation `expired`), error-message
sanitization (cap to 500 + strip control chars + null/empty
handling — Tomás §10 M1), missing-id defensive case (M4 case 16),
isPastExpiry helper edge cases (null / empty / unparseable
`expiresAt` — defensive against schema rows from before D-4
shipped per-method expiry).
`./gradlew test`: BUILD SUCCESSFUL.
What's still pending for Phase 2b (separate MRs)
================================================
- **Infrastructure repo:**
- IAM scope expansion on the Lambda role (DDB on
`*InvestmentReservation*` get/update + index/Query;
`*UserInvestment*` get/put; `*AuditLog*` put; `ses:SendEmail`
on the from-domain identity ARN with
`Condition: ses:FromAddress = "noreply@<env-domain>"` per
Tomás §10 R2).
- 5 new env vars on `aws-stripe-webhook-lambda.tofu` resolved via
the Amplify SSM bridge: `INVESTMENT_RESERVATION_TABLE_NAME`,
`INVESTMENT_PAYMENT_TABLE_NAME`,
`INVESTMENT_PAYMENT_BY_RESERVATION_INDEX_NAME`,
`USER_INVESTMENT_TABLE_NAME`, `AUDIT_LOG_TABLE_NAME`,
`SES_FROM_ADDRESS`.
- CloudWatch alarm on `WebhookEvent.processed=false` count > 0
for >5 min, routing to the existing `webhook_dlq_alarms` SNS
topic (Tomás §10 M3).
- `scripts/smoke-test-pay-05-handlers.sh` driving
`stripe trigger payment_intent.succeeded /
payment_intent.payment_failed` against dev — asserts the 4 DDB
writes + AuditLog + (when receipt_email present) email; replay
asserts single UserInvestment row (cases 13/14/15 of design §8).
- **payment-lambda repo (optional optimisation):**
- Extend `CreatePaymentIntentProcessor.stripeMetadata(...)` to
include `paymentRowId` and `paymentRowVersion`. Lets PAY-05's
webhook do a direct `GetItem` instead of the FK Query + filter
fallback. Not strictly needed (the fallback path works, and
has tests proving so) but cleaner.
- **stripe-webhook repo follow-up:**
- SES `receipt_email` extension: pull investor email from the
metadata block once PaymentLambda surfaces it (avoids depending
on Stripe's `receipt_email` field, which the current
PaymentLambda doesn't populate). PAY-21 owns this transition.
- `WebhookEvent.processed = true` update after successful TWI —
currently a TODO (the row stays `processed=false` even on
success today; the M3 alarm will catch any rows that linger).
Fix is small (extend `WebhookIdempotencyStore` with a
`markProcessed(eventId)` method); landing alongside the
infrastructure MR.
Refs
====
- Notion PAY-05: https://www.notion.so/34fd48539664810c9036d028db1b5e39
- `docs/pay-05-handler-design.md` (this branch — §9 Tomás review log)
- Scaffold PR (parent of this stack): #5
- Tomás's PAY-05 design review (saved /tmp/pay-05-review.md, summary
in PR #5 thread)
Co-Authored-By: Rui (Tech Lead) <team-sobrado@functorful.com>
ff-team-sobrado
added a commit
that referenced
this pull request
May 4, 2026
…ES email + tests
Phase 2b of PAY-05. Replaces the SCAFFOLD bodies on
`PaymentIntent{Succeeded,Failed}Handler` with the real implementation
specified in `docs/pay-05-handler-design.md` §4.1 + §4.2 (with all
8 of Tomás's M1–M4 + R1–R4 amendments folded in).
Companion infrastructure MR (IAM scope expansion, 5 new env vars on
`aws-stripe-webhook-lambda.tofu`, CloudWatch alarm on
`WebhookEvent.processed=false`, smoke test) lands separately in the
infrastructure repo against the same review pair.
What's in this commit
=====================
## New supporting classes
- `reservation/ReservationKey.java` — record mirroring the Amplify
schema's `InvestmentReservation` identifier tuple. Built from the
Stripe PaymentIntent metadata block PaymentLambda writes
(`reservationUserId`, `reservationInvestmentId`,
`reservationInvestmentVersion`, `reservationRequestedAt`,
`reservationVersion`). `IllegalArgumentException` messages contain
field names only — never values — so callers can safely log them.
- `dynamodb/ReservationView.java` + `dynamodb/PaymentView.java` —
immutable projected views of the two tables. Tomás-veto-discipline:
only the fields the handlers act on are projected.
- `dynamodb/InvestmentReservationStore.java` — strong-consistent
`GetItem` + `buildStatusUpdate(...)` (returns a `Update` payload to
embed in `TransactWriteItems`; no execution). Mirrors the
`payment-lambda` repo's `InvestmentReservationStore` for key shape
consistency (the literal `#` characters in the Amplify-generated
sort-key attribute name + matching separator-joined value).
- `dynamodb/InvestmentPaymentStore.java` — `findByReservationAndIntent`
via the Amplify-auto-generated GSI on the reservation FK
(configurable index name via env var
`investment-payment.by-reservation-index-name`, default
`"investmentPaymentByReservation"` so a smoke-test mismatch is fixable
without a code deploy). Filters in-memory by `stripePaymentIntentId`
— the result set is small (1–3 across retries). `buildStatusUpdate`
composes the conditional update payload.
- `dynamodb/UserInvestmentStore.java` — `buildIdempotentPut` carries
`attribute_not_exists(userId)` as the second-line idempotency
primitive. The first line is the `WebhookEvent` row from PAY-02.
- `dynamodb/AuditLogStore.java` — `buildPut` composes the
`AuditLog` row per design §5: reuses `DATA_ACCESS` enum,
semantic distinction in the searchable `action` field
(`payment.succeeded` / `payment.failed`). Details JSON includes
`receivedAt` (Tomás §10 M2), `lambdaVersion` + `gitSha` (R3),
`lastPaymentErrorCode` + sanitized `lastPaymentError` on failures
(M1).
- `email/SesEmailService.java` — Phase-1 placeholder English copy.
PAY-21 swaps to a templated, pt-PT version. SES failure is
best-effort: returns `false`, never throws — payment is confirmed
in DDB regardless. URL-connection HTTP client (consistent with the
rest of this Lambda).
- `email/SesV2ClientFactory.java` — `@Replaces(SesV2Client.class)` +
URL-connection pattern. Same shape as `SecretsManagerClientFactory`
and `DynamoDbClientFactory`.
## Real handler bodies
- `PaymentIntentSucceededHandler.handle(...)` — implements design §4.1
end-to-end: extract reservation key from metadata; load reservation
+ status guard (`confirmed`); load payment + status guard
(`processed`); single `TransactWriteItems` with 4 ops (payment
`processed -> success`, reservation `confirmed -> executed`,
idempotent `Put UserInvestment`, `Put AuditLog`); SES email
best-effort. `TransactionCanceledException` is the only catch
(Tomás §10 R4: distinct path for guard violations vs real bugs);
any other exception propagates to the orchestrator's outer catch.
- `PaymentIntentFailedHandler.handle(...)` — implements design §4.2:
same status-guard discipline; sanitizes `lastPaymentError.message`
(cap to 500 chars + strip control chars per Tomás §10 M1; logs
`errorCode` only, NOT raw message — same posture as PAY-23's
`PiiLogFilter` STRICT default); branches reservation target on
`expiresAt` vs `event.created` (`pending` for retry-window-open,
`expired` for past-window); 3-op `TransactWriteItems` (no
UserInvestment); no email.
## build.gradle
- New `software.amazon.awssdk:sesv2` dependency with the same
apache-/netty-client exclusions as the other AWS SDK clients.
Tests
=====
- `PaymentIntentSucceededHandlerTest` — 8 unit tests covering: happy
path (4-write TWI + SES), payment-already-success guard,
reservation-already-executed guard, missing payment, missing
reservation, `TransactionCanceledException` is swallowed (concurrent
guard violation), SES failure does not roll back DDB, missing
`data.object.id` (Tomás §10 M4 case 16), missing `receipt_email`
(DDB writes happen, SES skipped — PAY-21 will extend).
- `PaymentIntentFailedHandlerTest` — 8 unit tests covering: happy
path with retry-window-open (reservation `pending`), happy path
with past-`expiresAt` (reservation `expired`), error-message
sanitization (cap to 500 + strip control chars + null/empty
handling — Tomás §10 M1), missing-id defensive case (M4 case 16),
isPastExpiry helper edge cases (null / empty / unparseable
`expiresAt` — defensive against schema rows from before D-4
shipped per-method expiry).
`./gradlew test`: BUILD SUCCESSFUL.
What's still pending for Phase 2b (separate MRs)
================================================
- **Infrastructure repo:**
- IAM scope expansion on the Lambda role (DDB on
`*InvestmentReservation*` get/update + index/Query;
`*UserInvestment*` get/put; `*AuditLog*` put; `ses:SendEmail`
on the from-domain identity ARN with
`Condition: ses:FromAddress = "noreply@<env-domain>"` per
Tomás §10 R2).
- 5 new env vars on `aws-stripe-webhook-lambda.tofu` resolved via
the Amplify SSM bridge: `INVESTMENT_RESERVATION_TABLE_NAME`,
`INVESTMENT_PAYMENT_TABLE_NAME`,
`INVESTMENT_PAYMENT_BY_RESERVATION_INDEX_NAME`,
`USER_INVESTMENT_TABLE_NAME`, `AUDIT_LOG_TABLE_NAME`,
`SES_FROM_ADDRESS`.
- CloudWatch alarm on `WebhookEvent.processed=false` count > 0
for >5 min, routing to the existing `webhook_dlq_alarms` SNS
topic (Tomás §10 M3).
- `scripts/smoke-test-pay-05-handlers.sh` driving
`stripe trigger payment_intent.succeeded /
payment_intent.payment_failed` against dev — asserts the 4 DDB
writes + AuditLog + (when receipt_email present) email; replay
asserts single UserInvestment row (cases 13/14/15 of design §8).
- **payment-lambda repo (optional optimisation):**
- Extend `CreatePaymentIntentProcessor.stripeMetadata(...)` to
include `paymentRowId` and `paymentRowVersion`. Lets PAY-05's
webhook do a direct `GetItem` instead of the FK Query + filter
fallback. Not strictly needed (the fallback path works, and
has tests proving so) but cleaner.
- **stripe-webhook repo follow-up:**
- SES `receipt_email` extension: pull investor email from the
metadata block once PaymentLambda surfaces it (avoids depending
on Stripe's `receipt_email` field, which the current
PaymentLambda doesn't populate). PAY-21 owns this transition.
- `WebhookEvent.processed = true` update after successful TWI —
currently a TODO (the row stays `processed=false` even on
success today; the M3 alarm will catch any rows that linger).
Fix is small (extend `WebhookIdempotencyStore` with a
`markProcessed(eventId)` method); landing alongside the
infrastructure MR.
Refs
====
- Notion PAY-05: https://www.notion.so/34fd48539664810c9036d028db1b5e39
- `docs/pay-05-handler-design.md` (this branch — §9 Tomás review log)
- Scaffold PR (parent of this stack): #5
- Tomás's PAY-05 design review (saved /tmp/pay-05-review.md, summary
in PR #5 thread)
Co-Authored-By: Rui (Tech Lead) <team-sobrado@functorful.com>
ff-team-sobrado
added a commit
that referenced
this pull request
May 4, 2026
Resolves three add/add conflicts that surface because PR #5 was squash- merged: the same file contents now exist on both sides of the merge graph. HEAD's version is a strict superset (PR #5 content + Phase-2b real handler bodies + M-NEW-1 degraded-mode short-circuit), so "ours" is the correct resolution for all three: - docs/pay-05-handler-design.md (PR #5 added §1-§9; we added §11 M-NEW-1 + §9.3 review fold-in row). - src/main/java/com/functorful/stripewebhook/dispatch/handlers/PaymentIntentSucceededHandler.java (PR #5 added the SCAFFOLD shell; we replaced it with the real body). - src/main/java/com/functorful/stripewebhook/dispatch/handlers/PaymentIntentFailedHandler.java (same shape). ./gradlew test: BUILD SUCCESSFUL, 78 tests, 0 failures. Co-Authored-By: Rui (Tech Lead) <team-sobrado@functorful.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
1 participant
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.
Summary
This PR is deliberately scaffold-only — architecture and design contract land here, real handler bodies + IAM expansion + smoke test land in a Phase 2 PR. The split rationale: a design review and an SDK-call review are different shapes of feedback; folding them into a single PR makes both worse.
What's in this PR
docs/pay-05-handler-design.md— full contract for the two Phase-1 handlers:WebhookEventsdedup row from PAY-02).application/amplify/data/resource.ts— closes ARCH-04 in design).TransactWriteItemsper event).DATA_ACCESSenum, semantic distinction in theactionfield).EventHandlerinterface +IgnoredEventHandlerfallback for unknownevent.type.PaymentIntentSucceededHandler+PaymentIntentFailedHandlerscaffolds — log + return only, no DDB / SES yet.WebhookEventDispatcherrefactored from a hard-codedKNOWN_EVENT_TYPESset to aMap<String, EventHandler>Micronaut injection. New handlers add by@Namedbean only — no dispatcher edit needed../gradlew test→ BUILD SUCCESSFUL, 0 failures.What's NOT in this PR (Phase 2)
TransactWriteItems, SES, AuditLog rows.infrastructure(DDB on InvestmentReservation/UserInvestment/AuditLog tables,ses:SendEmailon the from-domain identity).infrastructure/scripts/smoke-test-pay-05-handlers.shdrivingstripe trigger payment_intent.succeededagainst dev with assertion of the 4 DDB writes + AuditLog + email + replay invariance.Phase 2 is mostly mechanical against
docs/pay-05-handler-design.mdonce approved.PR review pair
Acceptance criteria status (Notion PAY-05)
TransactWriteItems+WebhookEventsdedup); validated in Phase 2 smoke test.References
docs/pay-05-handler-design.md(this PR)application/amplify/data/resource.tsinfrastructure!12(PAY-03)🤖 Generated with Claude Code