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>
|
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 (1)
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe changes add configuration and management rules for a managed workflow sandbox email sender. A project-level POST endpoint creates or refreshes its integration when configuration and the team feature flag are available. The changes exclude the sender from email reputation results and selected SES lifecycle operations. Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The sandbox sender remains feature-gated, and no actionable issue is established for this change. It is mergeable after normal checks; keep the feature flag off until the send path is ready. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new provisioning capability is project-scoped, write-authorized and feature-gated. Users cannot directly modify the managed sender, and provisioning does not call SES. Remaining uncertainty concerns downstream consumers and recovery when worker notification fails after a configuration update. 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.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
1c474561-211a-4b6c-8935-70b4be0c4d59
⛔ Files ignored due to path filters (1)
products/integrations/frontend/generated/api.tsis excluded by!**/generated/**
📒 Files selected for processing (11)
posthog/api/integration.pyposthog/models/integration/__init__.pyposthog/models/integration/email.pyposthog/settings/ses.pyproducts/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.
…equire write scope Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Exclude the sandbox domain, not only sandbox-provider rows. · hog_flow.py:2252-2260
products/workflows/backend/presentation/views/hog_flow.py:2252-2260
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExclude the sandbox domain, not only sandbox-provider rows.
If a pre-existing verified ordinary integration uses the configured sandbox domain, the provider-only filter keeps that domain in
_isp_domains. The endpoint can then include it in ISP metrics and shared or withheld domain results. Filter projected domains withis_sandbox_sender_domain, which also recognizes subdomains.Suggested fix
from posthog.models.integration import SANDBOX_EMAIL_PROVIDER, Integration +from posthog.models.integration.email import is_sandbox_sender_domain ... - if domain + if domain and not is_sandbox_sender_domain(domain)
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
e174666e-d7b5-4464-b61d-d2ab86b801cc
📒 Files selected for processing (6)
posthog/api/integration.pyposthog/models/integration/__init__.pyposthog/models/integration/email.pyproducts/integrations/mcp/tools.yamlproducts/workflows/backend/facade/contracts.pyproducts/workflows/backend/tests/api/test_sandbox_sender.py
💤 Files with no reviewable changes (2)
- posthog/models/integration/init.py
- posthog/models/integration/email.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 0 remain after this review.
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>
|
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
workflows-sandbox-senderflag. Design and slicing: Silthus/posthog#225.Changes
POST /api/projects/{id}/integrations/email_sandbox_sender/, creates or refreshes the project's sandbox sender and returns it.integration_id = posthog-sandbox,created_by = null.workflows sandbox sender provisionedon first creation only.ensure_sandbox_email_sender(team_id), writes the row directly instead of usingcreate_native_integration.config.provider = "sandbox", not by a boolean besideprovider = "ses".post_deleteidentity cleanup selectprovider = "ses"rows only, so they skip the row without a code change.posthog/settings/ses.py:WORKFLOWS_SANDBOX_SENDER_DOMAIN,WORKFLOWS_SANDBOX_SENDER_FROM_ADDRESS,SES_SANDBOX_TENANT_NAME. All default to empty.Note
Any project member can call the endpoint from the app. An API key, OAuth token or MCP client needs
integration:write, because the call writes a row. The row grants nothing beyond the sandbox sending rules, which later slices enforce in the Node send path.Warning
Keep
workflows-sandbox-senderoff until the Node send-path PR lands. The email worker has nosandboxprovider case yet, so a send through the row would log success without sending.How did you test this code?
Test seams: Django API tests through the integrations endpoints (
APIBaseTest, flag patched, boto3 SES client mocked and asserted never called), task and command tests for the lifecycle exclusions, and one Reputation endpoint test.Test rationale:
products/workflows/backend/tests/api/test_sandbox_sender.pycatches a second row per project, a stale display name, a row created while the flag is off or settings are empty, a verify, edit or delete that reaches the row, and an own sender on the sandbox domain. No existing integration test covers a PostHog-managed row.products/workflows/backend/test/test_sandbox_sender_lifecycle.pycatches tenant migration, the tenant-state sweep or thepost_deletecleanup touching the sandbox row or SES.provider = "ses"rows.sandboxas the provider value: switching to a flag on ansesrow fails them.test_sandbox_sender.pycatches a read-only API key provisioning the row. It fails with the action in the read list.test_email_reputation.pycatches the sandbox domain showing up as a withheld domain in the ISP breakdown. It fails on the base tree.One scripted pass through the real URL stack against the local dev database, with a throwaway organization named "Acme Rockets", removed afterwards. SES and the flag client were mocked:
Live request output
Not checked:
Recovered red, green and local gate proof
Tests, mypy and strict preflight all ran on the final commit
72ded8d9180.Repo-wide mypy and the strict pre-push gate ran on
72ded8d9180.The lifecycle baseline already passed because those tools only select SES rows. It is not red proof for changed behavior.
The unavailable endpoint, reserved-domain rejection and Reputation exclusion provide the failing baseline.
To repeat the green test command in the prepared development environment:
Expected result: 75 passed. The recorded run used an isolated test database.
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Release status
Automatic notifications
Docs update
None. The feature is flagged off and no existing doc covers it.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, claude-opus-5-5
GPT-6.1 Sol recovered the proof and completed the handoff. It made no code changes.
/tdd,/writing-tests,/improving-drf-endpoints,/writing-code-comments,/reviewing-with-coderabbit,/writing-pr-descriptions.Integrationrow, documented likeWorkflowBatchJob.SES_SANDBOX_TENANT_NAME(ticket Closes #228 update docker compose file #230 asks for it here), moving the name and cascade cases to lower-level tests (private helper and different setup), and the CodeRabbit outside-diff note to exclude ordinary rows on the sandbox domain (no customer row can be verified on that domain, and creating one is now refused).workflows sandbox sender provisionedevent is sent from the view, not the facade, because it needs the requesting user.example.com.🤖 Generated with Claude Code