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>
|
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. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds managed sandbox email sender provisioning and API protections. The Node.js email service checks recipient membership, applies daily project and recipient caps, adds an identification footer, and routes sandbox messages through configured SES settings. Sandbox sends bypass team sending-budget claims. New tests cover provisioning, lifecycle behavior, recipient checks, send handling, and caps. Documentation records sandbox limits and rollout conditions. Priority: ⬇️ Low Merge Risk: 🔵 Low · up to Sandbox sending remains disabled by default. Before enabling it, confirm the SES settings are complete: an incomplete configuration can spend daily cap capacity before delivery succeeds. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The managed sender has strong project-ownership and recipient controls, including for test sends. The main design concern is that the advertised daily caps permit an initial burst plus continuous refill rather than enforcing a strict daily maximum. Deployment configuration and rollback behavior also need an explicit operational contract. 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🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
nodejs/src/cdp/services/messaging/email.service.ts-601-614 (1)
601-614: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winTreat missing sandbox SES routing settings as the switch being off.
The gate checks only
config.enabled.SES_SANDBOX_TENANT_NAME,SES_SANDBOX_CONFIGURATION_SET, andSES_SANDBOX_FROM_ADDRESSall default to''. If the flag is on but one of them is unset, the send still claims daily-cap tokens.sesRouteFor(Line 1194-1199) then passesTenantName: ''andConfigurationSetName: '', andresolveFromSender(Line 1220) falls back toemail: ''.The result is either an SES rejection after tokens were already spent, or a send with no configuration set. A send with no configuration set has no bounce or complaint event destination, so suppression and reputation tracking cannot see it. The other sandbox misconfigurations already fail closed with
check_failed. Handle these settings the same way.Proposed fix
- if (isSandbox && !this.sandboxSender?.config.enabled) { + const sandboxConfig = this.sandboxSender?.config + const sandboxReady = + !!sandboxConfig?.enabled && + !!sandboxConfig.tenantName && + !!sandboxConfig.configurationSetName && + !!sandboxConfig.fromAddress + if (isSandbox && !sandboxReady) {
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
08301290-a6bd-4c33-99b8-f249565ea002
⛔ 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 (23)
docs/internal/workflows-email-sending-tiers.mdnodejs/package.jsonnodejs/src/cdp/cdp-services.tsnodejs/src/cdp/config.tsnodejs/src/cdp/services/managers/organization-members.service.tsnodejs/src/cdp/services/messaging/email.service.test.tsnodejs/src/cdp/services/messaging/email.service.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.tsposthog/api/integration.pyposthog/models/integration/__init__.pyposthog/models/integration/email.pyposthog/settings/ses.pyproducts/integrations/mcp/tools.yamlproducts/workflows/backend/facade/api.pyproducts/workflows/backend/facade/contracts.pyproducts/workflows/backend/presentation/views/hog_flow.pyproducts/workflows/backend/services/sandbox_sender.pyproducts/workflows/backend/test/test_sandbox_sender_lifecycle.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; 6 remain after this review.
…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>
|
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 |
Warning
Merge risk: medium (qa-swarm danger rubric)
Problem
The sandbox sender lets a project email its own organization members before it verifies a domain, but nothing limits how much it sends per day.
The sending tiers do not apply to sandbox sends, and test sends skip workflow pacing.
Stacked on #111483. This cumulative fork PR targets
master. The review delta is754efd702a4...feat/workflows-sandbox-daily-caps.Changes
25 per day,1.5,0and an empty value all block. The shared env parser would read25 per dayas 25.workflows sandbox email blockedwithreasoncap_reachedorcheck_failed, and a count, never the addresses.WORKFLOWS_SANDBOX_DAILY_TEAM_CAPandWORKFLOWS_SANDBOX_DAILY_RECIPIENT_CAP. An absent setting uses the default innodejs/src/cdp/config.ts.claimAllOrNothingmethod onRateLimiterService, a dedicatedsandbox-emaillimiter instance for metrics, a lookup table for the sandbox skip copy, and one docs bullet.Warning
The CDP API deployment needs the
SES_RATE_LIMITER_VALKEY_*settings the email worker already has. Without them, sandbox test sends skip with "could not check the sandbox sender's daily limit".flowchart LR A[Sandbox send] --> B[Recipient and suppression gates] --> C[Footer and final recipient recheck] --> D[Claim daily caps] D -->|Granted| E[SES] D -->|Cap reached or check failed| F[Skip and explain] classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff; class A,F phYellow; class B,C,D phBlue; class E phRed;Before this PR, the same path went from the final recipient recheck straight to SES.
Note
Every bucket key shares the
{teamId}hash tag, so the one Lua call stays in one Valkey cluster slot.The per-address bucket is therefore per project and address, not global per address. A cross-project bucket would need a cross-slot call, which clustered Valkey rejects.
Review notes:
How did you test this code?
Test rationale: Four new parameterized cases in the
sandbox senderblock ofemail.service.test.tsdriveEmailService.executeSendEmailwith real Postgres, the real Valkey limiter, andsesV2Client.sendmocked. No existing test covered sandbox caps. Each case runs for live and test sends.Local gate: the email service and rate limiter suites, Node lint, scoped semgrep and strict preflight pass. Node-wide TypeScript reports only the unbuilt
@posthog/replay-anonymizerworkspace package, which this PR does not touch.Proof on cluster-mode Valkey
A scratch script (not committed) ran
SandboxEmailSender.claimDailyCapsagainst the dev stack's cluster-mode Valkey (cluster_enabled:1), with a project cap of 3 and an address cap of 1:The control shows the node rejects a cross-slot call, so the granted claim proves every key of a send shares one slot.
Not checked: a multi-node Valkey cluster, the cdp-api wiring in a running server (no test seam for
server.ts), and the fork's CI, which waits for a maintainer to approve the run.Release status
Automatic notifications
Docs update
docs/internal/workflows-email-sending-tiers.mdnames the two cap settings, the fail-closed rule, and the CDP API's need for the SES Valkey settings.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Claude Opus 5.5 (tests, review, refactor); Codex, GPT-6.1 Sol (implementation against the failing tests)
/tddand/writing-tests. Tests landed red in their own commit./tdd,/writing-tests,/reviewing-with-coderabbit,/writing-pr-descriptions.1bdf14051a7.server.tsis outside the ticket's write scope, but cdp-api had to receive the Valkey pool for test sends to work.🤖 Generated with Claude Code