Repository navigation
feat(cdp): record skipped, failed and delivered transactional emails - #124751
Conversation
🤖 CI report✅ Trunk lane — non-backend laneThis PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes.
|
| First copy | Second copy | Lines | Tokens |
|---|---|---|---|
products/warehouse_sources/backend/temporal/data_imports/sources/gladly/gladly.py:10 |
products/warehouse_sources/backend/temporal/data_imports/sources/zep/zep.py:5 |
15 | 102 |
products/warehouse_sources/backend/temporal/data_imports/sources/meta_ads/schemas.py:298 |
products/warehouse_sources/backend/temporal/data_imports/sources/meta_ads/schemas.py:381 |
45 | 99 |
products/warehouse_sources/backend/temporal/data_imports/sources/meta_ads/schemas.py:299 |
products/warehouse_sources/backend/temporal/data_imports/sources/meta_ads/schemas.py:461 |
44 | 97 |
products/warehouse_sources/backend/temporal/data_imports/sources/mollie/mollie.py:3 |
products/warehouse_sources/backend/temporal/data_imports/sources/recruitee/recruitee.py:3 |
14 | 96 |
products/warehouse_sources/backend/temporal/data_imports/sources/recruitee/recruitee.py:2 |
products/warehouse_sources/backend/temporal/data_imports/sources/vendr/vendr.py:1 |
13 | 94 |
⚠️ Duplication (TypeScript) — 1 new duplicated block (worst 115 tokens)
New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.
| First copy | Second copy | Lines | Tokens |
|---|---|---|---|
products/workflows/frontend/OptOuts/CustomerIOImportModal.tsx:175 |
products/workflows/frontend/OptOuts/CustomerIOImportModal.tsx:332 |
25 | 115 |
🚨 Comment density — 8% of added code lines are comments (49 of 645)
This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.
Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.
Files with the most added comment lines:
| File | Comment lines | Added lines |
|---|---|---|
nodejs/src/cdp/services/messaging/email-status.service.ts |
21 | 88 |
nodejs/src/cdp/services/messaging/helpers/ses.ts |
15 | 76 |
nodejs/src/cdp/services/hogflows/actions/hog_function.ts |
3 | 23 |
nodejs/src/cdp/services/messaging/email-status.service.test.ts |
3 | 119 |
nodejs/src/cdp/services/hogflows/hogflow-executor.service.ts |
2 | 8 |
nodejs/src/cdp/services/messaging/email-tracking.service.test.ts |
2 | 39 |
nodejs/src/cdp/services/messaging/helpers/ses.test.ts |
2 | 68 |
nodejs/src/cdp/services/messaging/email-tracking.service.ts |
1 | 21 |
This check does not block merging. It updates on every push and clears when the share drops.
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
There was a problem hiding this comment.
PostHog Review · Standard · reviewed e417c58: 3 Should fix.
This Standard review started from the posthog-review label. Comment @posthog review --deep for a Deep review, or @posthog help to see all commands.
See this review and its findings in PostHog Review.
| case 'Complaint': | ||
| return reachesTo(rec.complaint.complainedRecipients.map((r) => r.emailAddress)) | ||
| ? { status: 'complained', timestamp: rec.complaint.timestamp } | ||
| : null |
There was a problem hiding this comment.
P2 · Exclude not-spam corrections from complained status
When SES sends a Complaint with complaintFeedbackType: 'not-spam' for the To recipient, resolveEmailStatusUpdate records complained even though this feedback explicitly reverses a spam classification. That highest-ranked status then overrides delivery permanently. Apply the same case-insensitive not-spam exclusion used by the suppression path.
There was a problem hiding this comment.
Fixed in 6452361. A not-spam feedback type, compared case-insensitively like the suppression path, no longer records complained. Covered in ses.test.ts.
| default: | ||
| return null |
There was a problem hiding this comment.
P2 · Record asynchronous SES rejections as terminal failures
When SES accepts a send and subsequently emits Reject, such as after detecting a virus, resolveEmailStatusUpdate returns null. The synchronous send already recorded sent, so callers retain that status despite SES refusing delivery; existing webhook handling only records a failure metric and log. Handle Reject with a failure version that supersedes sent.
There was a problem hiding this comment.
Fixed in 6452361. A Reject record now writes failed with reason provider_rejected at its own rank between sent and delivered, so it replaces sent. Covered in ses.test.ts and the rank-order test.
| failed: 2n, | ||
| sent: 3n, | ||
| delivered: 4n, | ||
| bounced: 5n, | ||
| complained: 6n, |
There was a problem hiding this comment.
P2 · Preserve the existing persisted status ranks
With EMAIL_STATUS_ENABLED, existing sent rows encode rank 2. This change reuses rank 2 for failed, so a skipped or rejected retry with the same invocation ID produces a larger version and replaces the earlier sent row, violating the ordering guarantee across upgrades. Preserve existing ranks and allocate run failures an unused intermediate version range.
- A skipped send is recorded as failed with its skip reason. - A run that fails or is canceled, or whose workflow or team is gone at dequeue, is recorded as failed. A run failure ranks below the email step's own failure, so `provider_error` survives the abort that follows it. - The SES webhook records delivered, bounced and complained for transactional sends, which carry the `ph_email_status` tag, and only for a `To` recipient. A not-spam correction is not a complaint. - An SES rejection after the send was accepted is recorded as failed with `provider_rejected`, ranked above `sent`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e417c58 to
6452361
Compare
|
Risk: No findings This PR records terminal statuses for transactional emails (skipped sends, failed/canceled runs, workflow-unavailable jobs, and SES delivery/bounce/complaint/reject events) into the email_status ClickHouse table via the existing Kafka output, and threads the EmailStatusService into the SES webhook, the hog function email handler, the hog flow executor, and the invocation-results sink. The main security surface — the new state-changing SES webhook write — is properly layered behind SNS signature verification with an allowlisted cert host, a TopicArn allowlist, an HMAC-signed (timing-safe-verified) tracking code, and a send-time-only SES tag, so no forged or cross-tenant status rows are possible; I found no security vulnerabilities. Sentinel reviewed |
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved.
The risky changes have assurance through owning-team authorship and STRONG familiarity, with targeted test coverage. The current code addresses all three historical review concerns; no remaining substantive concerns or policy refusal grounds were found.
- Author wrote 71% of the modified lines and has 35 merged PRs in these paths (familiarity STRONG).
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 260L, 11F substantive, 713L/18F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (713L, 18F, single-area, feat) |
| stamphog 2.6.1 | .stamphog/policy.yml @ 6452361 · reviewed head 6452361 |
|
/trunk merge |
|
😎 Stack merged successfully - details. |
Problem
queuedorsent, but a skipped send, a failed run, or a bounce never reaches its status.queuedfor 30 days, orsentfor an email that bounced.This PR completes the Node writes. #123932 reads the
email_statustable.Refs #120921.
Changes
failedwith the skip reason, for examplerecipient_suppressed,recipient_opted_outorfrequency_capped.failed, with the run's error kind orcanceledas the reason.failedwithworkflow_unavailable.provider_errorafter the run aborts on it.delivered,bounced(withhard,softorunknown) andcomplained.ph_email_statusSES tag, and the webhook only updates the status for aTorecipient. A bounce from a cc or bcc address does not change it.Toheader of an unexpected shape turns off the status update only, never the processing of the whole SES record.classifyErroris exported, and the hog function handler and the hog flow executor take the status service.How did you test this code?
Test rationale: each path that ends an email has a case, because a missed path leaves the email at
queuedwith no error.email-status.service.test.ts: each run outcome, a provider rejection that then fails its run, and a job whose workflow is gone.email.service.test.tsandhog_function.test.ts: the row for each skip reason, and none for a test send or a workflow email.ses.test.tsandemail-tracking.service.test.ts: the status for each SES event, theTomatch, and the tag check.Ran locally: the Jest suites under
src/cdp/services/messaging/, the hog flow worker, the hog function handler, the invocation results service, andtscwith no errors insrc/cdp.Not checked yet: a send end to end on the local stack.
Release status
Docs update
None. #123932 documents the endpoint.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: PostHog Desktop (Claude Code), Claude Opus 5.5
/qa-team,/writing-tests,/writing-code-comments,/writing-pr-descriptions.🤖 Generated with Claude Code