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>
|
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 change adds a managed sandbox email sender that the integration API can provision or refresh. It adds settings and integration guards, and rejects sandbox sender use in broadcasts and unsupported workflow email configurations. Workflow validation uses request-scoped caches and retains existing email values to allow unchanged stored configurations. Tests cover provisioning, management restrictions, validation, and integration lifecycle behavior. Priority: ⬇️ Low Merge Risk: 🔵 Low · up to Sandbox sender validation remains enforced on the described save paths. Clarifying why drafts need the separate check would help prevent a future regression, but the change is mergeable with that follow-up. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change adds meaningful restrictions on sandbox sender use and preserves project-scoped access. No concrete authorization bypass was established. Recovery during sender configuration changes and deployment rollback remains insufficiently demonstrated, so the risk is not minimal. Retained concerns Security review detailsSecurity Blast Radius
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.
🧹 Nitpick comments (1)
products/workflows/backend/presentation/views/hog_flow.py (1)
1620-1624: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the duplicate sandbox validation call.
InputsItemSerializer.validatealready callsvalidate_sandbox_email_senderfornative_emailinputs (posthog/cdp/validation.pyLine 828). This loop calls the same validator for the same values. Lenient drafts are the exception: they useis_valid()without raising, so this loop is the only enforcement on that path. If that is the reason for the loop, add a short comment. Without the comment, a later cleanup can remove the only draft-path guard.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
- Review profile: QUIET
- Plan: Enterprise
- Run ID:
218355ec-146b-4ad4-a8a1-ecf043f2cb45
⛔ Files ignored due to path filters (1)
products/integrations/frontend/generated/api.tsis excluded by!**/generated/**
📒 Files selected for processing (15)
posthog/api/integration.pyposthog/cdp/test/test_validation.pyposthog/cdp/validation.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.pyproducts/workflows/backend/tests/api/test_sandbox_sender_validation.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 9 remain after this review.
|
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
Teams can save sandbox senders in broadcasts, destinations and custom sender identities. The sandbox sender supports workflow email steps and test sends.
Implements Silthus/posthog#231, following the sandbox sender spec.
Changes
Stored configurations match the complete From object and Reply-To for the same action. Matching only the sender ID would allow edited identity overrides.
This fork PR includes #111368. Review this layer.
Before:
flowchart LR Save[Save email configuration] --> Validate[Existing validation] --> Stored[Stored configuration] classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff; classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000; class Save phRed; class Validate phBlue; class Stored phGray;After:
flowchart LR Save[Save email configuration] --> Sandbox[Sandbox sender rules] Sandbox --> Error[Actionable 400] Sandbox --> Validate[Existing validation] --> Stored[Stored configuration] classDef phRed fill:#f54e00,stroke:#f54e00,color:#fff; classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000; classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; class Save phRed; class Sandbox,Validate phBlue; class Stored phGray; class Error phYellow;How did you test this code?
The API tests use real Postgres and a facade-created sandbox row. Analytics is mocked at the external SDK boundary.
Verify with
.codex/with-flox hogli test products/workflows/backend/tests/api/test_sandbox_sender_validation.py. Allowed workflow saves return 201; rejected selections return actionable 400 responses.Test rationale:
test_literal_off_domain_sender_override_is_rejected_at_saveandtest_stored_off_domain_sender_override_survives_a_resavecover own-domain overrides. New API cases cover sandbox policy, lenient drafts and stored configurations; the public invocation serializer covers test-send acceptance.Retained local proof at 97b8663
The recovered logs record outside-in red and green results before and after each fix. Rejected selections initially returned 201; malformed inputs returned 500. The final API suite asserts 400 responses. Unchanged stored sender edits initially returned 400 and now return 200.
The final suite used a fully migrated isolated Postgres database after shared test-database setup failed. No tests were skipped and no database was mocked.
Unchecked: delivery, frontend rendering, and remote patch coverage. These tests do not read event properties; no new-events-schema expansion applies.
Backend CI and Python CI await maintainer approval. This is an unrun CI gap, not a green result.
Release status
Automatic notifications
Docs update
No existing document covers sender save validation.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Codex, GPT-6.1 Sol. The inherited base is Claude Code, claude-opus-5-5, documented in #111368.
Skills:
/tdd,/writing-tests,/improving-drf-endpoints,/writing-user-facing-copy,/running-ci-preflight,/qa-swarm,/stacking-prs,/writing-pr-descriptions,/writing-voice,/unslop,/principle-prove-it-works,/hogli,/querying-local-postgres,/reviewing-with-coderabbit,/leaving-pr-comments,/writing-for-agents.CodeRabbit CLI was signed out; the local pass was skipped. The fresh QA swarm and remote CodeRabbit reviewed this layer on
97b86630a25.One clean round: six GPT-6.1 Sol lenses found no issues. Claude Sonnet5.5's flag-check duplication suggestion was rejected after both matching gates were verified.
CodeRabbit's duplicate-validation nit was rejected: the precheck enforces policy before lenient draft validation can swallow errors.
No fixes or decisions remain open. The code is ready for human review; the PR remains draft with the maintainer-approval CI gap above.
No duplicate validation PR was found. Fixtures use invented
example.comaddresses. GitHub denied assigning the verified author due account permissions.