Repository navigation
Conversation
…lusions Unfinished: the red/green proof runs and the OpenAPI regeneration were interrupted by a devbox rebalance. Tests are written but not yet run green; generated OpenAPI output is not regenerated. Continue test-first. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ender endpoint Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…equire write scope Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…quest_access comment Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…haracters Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
A long blocked CC/BCC list wrote one log row per address. Rows now hold as many comma-joined addresses as fit under the untruncated log length, so the row count follows total characters. Every address stays visible. The case-insensitive membership matrix also stores member emails with mixed case, which proves the stored-side case fold. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…dbox send Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…r day Every sandbox send, test sends included, claims one token per recipient copy from a daily project bucket and one per distinct address from a daily per-address bucket, all or nothing in one Valkey call. A denied claim skips the send and spends nothing. A limiter error, a missing limiter, or an unset or invalid cap setting skips every sandbox send. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…final recipient check A cap setting with trailing text no longer reads as its numeric prefix, and a send the final membership recheck blocks no longer spends daily tokens. The project cap message also reads correctly for a send with more copies than the whole daily cap. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Test sends run inline in cdp-api, whose deps had no SES Valkey pool, so every sandbox test send failed closed. cdp-api now gets the same pool as the email workers and claims the same daily caps. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…t findings An older SES reading could overwrite a newer pause when a webhook sync and the sweep overlapped, and a failed recommendations listing discarded a known pause. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Also names the pause gate after what it returns and drops a null-seeding loop the lazy loader already performs. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…sandbox delivered event Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ndbox deliveries The tier sweep subtracts email_sandbox_sent from email_sent per team per day, floored at zero. The SES webhook captures workflows sandbox email delivered for a Delivery on the sandbox configuration set, for the team named by the signed tracking code. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…he clock in the tier test Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe pull request adds managed sandbox email sender provisioning and guarded email delivery. It synchronizes shared SES tenant state, checks recipient membership, and applies daily caps and pause-state checks before sandbox sends. SES delivery events are captured, and sandbox sends are excluded from email-tier send counts. Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to SES throttling can use up sandbox daily caps without delivering mail, causing later sends or retries to be blocked. Resolve that behavior before merging unless it is explicitly accepted. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The managed sender has meaningful recipient, identity, and quota safeguards. However, repeated provider throttling can consume sandbox quotas without delivering email and eventually cause pending sends to be skipped. Production enablement and provider-side pause enforcement remain unverified. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
d6291695-f20c-4272-86ae-c1f183c4c5c1
⛔ Files ignored due to path filters (2)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlproducts/integrations/frontend/generated/api.tsis excluded by!**/generated/**
📒 Files selected for processing (43)
.github/scripts/check-idor-model-coverage.pydocs/internal/workflows-email-sending-tiers.mdnodejs/package.jsonnodejs/src/cdp/cdp-api.tsnodejs/src/cdp/cdp-services.tsnodejs/src/cdp/config.tsnodejs/src/cdp/services/managers/organization-members.service.tsnodejs/src/cdp/services/managers/sandbox-sender-state.service.tsnodejs/src/cdp/services/messaging/email-tracking.service.test.tsnodejs/src/cdp/services/messaging/email-tracking.service.tsnodejs/src/cdp/services/messaging/email.service.test.tsnodejs/src/cdp/services/messaging/email.service.tsnodejs/src/cdp/services/messaging/helpers/ses.tsnodejs/src/cdp/services/messaging/sandbox-email-sender.tsnodejs/src/cdp/services/rate-limiter/rate-limiter.service.tsnodejs/src/cdp/types.tsnodejs/src/cdp/utils.tsnodejs/src/server.tsposthog/api/integration.pyposthog/models/integration/__init__.pyposthog/models/integration/email.pyposthog/plugins/plugin_server_api.pyposthog/settings/ses.pyproducts/integrations/mcp/tools.yamlproducts/workflows/backend/facade/api.pyproducts/workflows/backend/facade/contracts.pyproducts/workflows/backend/migrations/0030_sandboxsendertenantstate.pyproducts/workflows/backend/migrations/max_migration.txtproducts/workflows/backend/models/__init__.pyproducts/workflows/backend/models/sandbox_sender_tenant_state.pyproducts/workflows/backend/presentation/views/hog_flow.pyproducts/workflows/backend/providers/ses.pyproducts/workflows/backend/services/email_sending_tier.pyproducts/workflows/backend/services/sandbox_sender.pyproducts/workflows/backend/services/ses_tenant_events.pyproducts/workflows/backend/services/ses_tenant_state.pyproducts/workflows/backend/tasks/ses_tenant_state.pyproducts/workflows/backend/test/test_email_sending_tier.pyproducts/workflows/backend/test/test_sandbox_sender_lifecycle.pyproducts/workflows/backend/test/test_ses_tenant_events.pyproducts/workflows/backend/test/test_ses_tenant_state.pyproducts/workflows/backend/tests/api/test_email_reputation.pyproducts/workflows/backend/tests/api/test_sandbox_sender.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 1 remain after this review.
…subtraction Cover the signed-code requirement on the sandbox delivered event, the per-day floor at zero for sandbox sends, and that bounces and complaints of sandbox sends still count. Move the capture property mapping into a switch. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nodejs/src/cdp/services/messaging/email-tracking.service.test.ts (1)
772-775: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an unsigned-header delivery case.
When
signedCodeis false,postDeliveryputs the unsigned code inph_idand omitsX-PostHog-Tracking-Code. The negative case therefore does not catch a regression that accepts an unsigned code from the header. Add a header-only unsigned case and keep theph_idcase.Suggested test change
const postDelivery = async ( configurationSet: string, isTest: boolean, - signedCode = true + signedCode = true, + includeUnsignedHeader = false ): Promise<supertest.Response> => { @@ - headers: signedCode ? [{ name: TRACKING_CODE_HEADER_NAME, value: signedTrackingCode }] : [], + headers: signedCode + ? [{ name: TRACKING_CODE_HEADER_NAME, value: signedTrackingCode }] + : includeUnsignedHeader + ? [{ name: TRACKING_CODE_HEADER_NAME, value: unsignedTrackingCode }] + : [], tags: { 'ses:configuration-set': [configurationSet], - ...(signedCode ? {} : { ph_id: [unsignedTrackingCode] }), + ...(signedCode || includeUnsignedHeader ? {} : { ph_id: [unsignedTrackingCode] }), @@ it.each([ - ['a delivery on another configuration set', 'posthog-messaging-tracked', true], - ['a sandbox delivery whose tracking code is not signed', sandboxConfigurationSet, false], - ])('captures no delivered event for %s', async (_name, configurationSet, signedCode) => { - const res = await postDelivery(configurationSet, false, signedCode) + ['a delivery on another configuration set', 'posthog-messaging-tracked', true, false], + ['a sandbox delivery whose tracking code is not signed', sandboxConfigurationSet, false, false], + ['a sandbox delivery with an unsigned tracking code in the header', sandboxConfigurationSet, false, true], + ])( + 'captures no delivered event for %s', + async (_name, configurationSet, signedCode, includeUnsignedHeader) => { + const res = await postDelivery(configurationSet, false, signedCode, includeUnsignedHeader) - expect(res.status).toBe(200) - expect(capture).not.toHaveBeenCalled() - }) + expect(res.status).toBe(200) + expect(capture).not.toHaveBeenCalled() + } + )
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
48cb1ddf-c03e-4983-818a-fae734ed9791
📒 Files selected for processing (3)
nodejs/src/cdp/services/messaging/email-tracking.service.test.tsnodejs/src/cdp/services/messaging/sandbox-email-sender.tsproducts/workflows/backend/test/test_email_sending_tier.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 6 remain after this review.
|
Note 🤖 Claude Sonnet 5.5 responding on behalf of Michael @coderabbitai on the header-only unsigned case: no change. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
fd -i 'tracking.*code|ses\.ts$|email-tracking\.service\.test\.ts$' nodejs
rg -n -C3 'class EmailTrackingCodeSigner|codeIsTrusted|sandboxDeliveries|unsigned|ph_id' nodejs/src/cdp/services/messaging/helpers/ses.ts nodejs/src/cdp/services/messaging/email-tracking.service.test.ts
ast-grep outline nodejs/src/cdp/services/messaging/helpers/ses.ts --items allLength of output: 17658 🏁 Script executed: #!/bin/bash
ast-grep outline nodejs/src/cdp/services/messaging/helpers/tracking-code.ts --items all
ast-grep outline nodejs/src/cdp/services/messaging/email-tracking.service.test.ts --items all --match 'sandbox|delivery|Delivery'
rg -n -A85 -B5 'parse\(code' nodejs/src/cdp/services/messaging/helpers/tracking-code.ts
sed -n '695,720p' nodejs/src/cdp/services/messaging/helpers/ses.ts
sed -n '725,825p' nodejs/src/cdp/services/messaging/email-tracking.service.test.tsLength of output: 7579 🏁 Script executed: #!/bin/bash
rg -n -A125 -B3 '^\s+parse\(' nodejs/src/cdp/services/messaging/helpers/tracking-code.tsLength of output: 7108
✏️ Learnings added
You are interacting with an AI system. |
|
This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, please remove the |
Problem
A team that sends from the sandbox sender gains sending-tier history it never earned on its own domain, and nobody can see when its first sandbox email actually arrives.
Stacked on #111820. This cumulative fork PR targets
master. The review delta isa66bb200f89...feat/workflows-sandbox-first-delivery.Note
🔵 Merge risk: low (merge-risk rubric)
SES_SANDBOX_CONFIGURATION_SETkeeps the webhook branch off.Changes
email_sandbox_sentfromemail_sentper team per day, floored at zero. The email worker already pushes that companion metric besideemail_sentfor every live sandbox send.workflows sandbox email delivered(is_test) when aDeliveryevent'sses:configuration-settag isSES_SANDBOX_CONFIGURATION_SET.SES_SANDBOX_CONFIGURATION_SETnever matches, so the event stays off until the CDP API deployment carries that setting.createCdpCoreServicesexposes the existingSandboxEmailSender, so the email worker and the webhook build the event name in one place. Two lines in the sending tiers doc.The sandbox funnel for product analytics, which is not activation:
flowchart LR A[workflows sandbox sender provisioned] --> B[workflows sandbox email sent] --> C[workflows sandbox email delivered, first time] B -.->|Drop-off| D[workflows sandbox email blocked, by reason] classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000; class A phYellow; class B,C phBlue; class D phGray;"First sandbox delivery" is its own funnel step. The activation funnel must not count it, because activation means a real message to a real recipient from the team's own domain.
How did you test this code?
recompute_email_sending_tier_for_teamover realapp_metrics2rows in ClickHouse, clock frozen. Sandbox-only sends decay like no sends; own plus sandbox sends stay under the use bar; own sends over the bar still promote. A pure-function test would miss a sweep that stops fetching the companion metric.build_sending_historiesover the same rows. One day with more sandbox than own sends cannot erase another day's own sends, and bounces and complaints from sandbox sends still count. These catch a whole-window subtraction and a feedback filter.EmailTrackingService.handleSesWebhookthrough the CDP API route with a posted SNSDeliveryenvelope, signature check stubbed andcaptureTeamEventspied. A sandbox-set delivery captures one event for live and test sends; a delivery on the tracked set, or on the sandbox set with an unsigned tracking code, captures none.email.service.test.tsalready assertsemail_sandbox_sentbesideemail_sentfor live sandbox sends, and no metrics for test sends.Verify with
pytest products/workflows/backend/test/test_email_sending_tier.py -k TestSandboxSendsInTierHistory(6 passed) and, innodejs,npx jest src/cdp/services/messaging/email-tracking.service.test.ts -t 'sandbox deliveries'(4 passed).Local gate: the touched pytest files,
email-tracking.service.test.ts,ses.test.tsandemail.service.test.ts, repo-wide mypy, ruff, prettier, eslint, scoped security semgrep and strict preflight pass. Node-wide TypeScript reports only the unbuilt@posthog/replay-anonymizerworkspace package, which this PR does not touch.Not checked: a real SES
Deliveryevent from the sandbox configuration set. Whether SES carries theses:configuration-settag and the tracking header on that event belongs to the outside-code checklist.Release status
Sandbox sender rows exist only behind
workflows-sandbox-sender, and sandbox sending stays off untilWORKFLOWS_SANDBOX_SENDER_ENABLEDis on. Without sandbox sends, both changes are inert.Automatic notifications
Docs update
Two lines in
docs/internal/workflows-email-sending-tiers.md: the tier rule, and the CDP API setting the delivered event needs.🤖 Agent context
/tddand/writing-tests. The red tests were committed before the implementation.outcomePropertieshelper from review./qa-swarmin two rounds: six lenses on GPT-6.1 Sol and a fresh-eyes lens on Claude Sonnet 5.5. Round 2 produced no fix./tdd,/writing-tests,/reviewing-with-coderabbit,/writing-pr-descriptions,/merge-risk,/qa-swarm.SandboxEmailSender.capture, so all sandbox events share one name pattern and one team lookup.daily_sendsis built, so the use bar and the decay both see own sends only.🤖 Generated with Claude Code