Skip to content

fix(workflows): keep one mail from label per email sending domain - #345

Closed
Silthus wants to merge 1 commit into
fix/mcp-exclude-params-per-toolfrom
fix/email-mail-from-label-overwrite
Closed

Silthus wants to merge 1 commit into
fix/mcp-exclude-params-per-toolfrom
fix/email-mail-from-label-overwrite

Conversation

@Silthus

@Silthus Silthus commented Oct 7, 2026 •

Copy link
Copy Markdown
Owner

Problem

Feature flags: workflows-email-domain-agent-setup. Default off; this handoff does not activate them.

  • Two Workflows email senders on one domain can quietly undo each other's MAIL FROM subdomain (the Return-Path label, for example feedback).
  • SES stores that label on the domain identity, so every sender on the domain shares one label.
  • Path 1: a new sender with a different mail_from_subdomain re-set the label in SES for every sender on the domain.
  • Path 2: editing the label on one sender stored it on that sender only. The next verify of any other sender on the domain re-set SES to that sender's old label.

Note

🔵 Merge risk: low (merge-risk rubric)

  • With workflows-email-domain-agent-setup on, create, edit and verify hold a per-domain advisory lock across SES calls, and an edit rewrites the label on senders in other projects of the organization.
  • The flag is off by default. With it off, the three paths run master's logic, and test_each_sender_keeps_its_own_mail_from_label_while_the_flag_is_off pins that. The only flag-off additions are one flag evaluation and one Team lookup on create.

Fork handoff for source #111367. Modernized head 30eac42d0dfe604c23c6f402d8e34058e248ea24. This draft prepares review and recreation in the upstream repository.

Changes

Kind Files Lines
Production 3 +150 −17
Tests 3 +243 −1
Total 6 +393 −18
  • Everything below runs only when the workflows-email-domain-agent-setup flag is on, the flag that also gates the email domain MCP tools in #109703. Turning it on enables both PRs together. With it off, create, edit and verify behave as on master.
  • The flag is evaluated with the same arguments as feat(workflows): let agents set up an email sending domain via mcp PostHog/posthog#109703's API gate: the acting user's distinct_id, and the organization and project groups with their id group properties. The API views pass the request user, so create, edit and verify agree for the same caller.
  • A new sender takes the domain's existing label when the request omits it.
  • A new sender that asks for a different label gets a 400 that names the label in use. SES is not called.
  • When senders on a domain already have different labels (legacy data), a new sender gets a 400 that says to save one label on an existing sender first.
  • Editing the label on one sender now writes it to every sender on that domain in the organization. A later verify of any of them keeps the new label.
  • An edit with a blank label keeps the current label. A legacy blank stored label reads as the default feedback.
  • Create, edit and verify each run in one transaction under a per-domain advisory lock, and they re-read the sender under it. A verify request that loaded its sender before an edit can no longer re-set the old label.
  • The sibling rows are locked with FOR UPDATE OF posthog_integration, so no Team row is locked.
  • The worker reload after a successful verify now runs on commit.
  • Mechanical: the default label is one constant. Domain Connect reads the label through EmailIntegration, so it follows the same flag rule as verify.

Note

The setup modal pre-fills feedback. On a domain that already uses another label, the user now sees the 400 in a toast and edits the field. The modal does not look up the domain's label.

Warning

Roll the flag out by organization. A teammate with the flag off still edits only their own sender, and their verify can re-set the old label for the domain.

Read first: EmailIntegration._domain_guard and create_native_integration, where the flag picks the path. With the flag on, each holds a Postgres advisory lock for the length of its SES calls.

Overlap with #109703 at 058782e: a trial merge conflicts in two files. Both PRs can merge in either order, because neither changes behavior until the flag is on. The PR that lands second resolves them:

  • posthog/api/integration.py: feat(workflows): let agents set up an email sending domain via mcp PostHog/posthog#109703 routes email_update and email_verify through _get_email_integration(). That helper passes acting_user=cast(User, self.request.user), and domain_connect_apply_url passes it to resolve_email_context.
  • posthog/models/integration/email.py: both PRs add methods after update_native_integration. Keep both.
  • Both PRs define EMAIL_DOMAIN_AGENT_SETUP_FLAG and evaluate it with the same arguments. The second PR keeps one constant and one helper.

How did you test this code?

  • All 38 focused email/label/domain-connect tests passed in the task-owned test DB; evidence is in domain-111367-tests.log.
  • Ruff and commit-hook Python type checks passed; evidence is in domain-111367-lint.log and domain-111367-commit.log.
  • Strict ci:preflight passed; evidence is in domain-111367-preflight.log.
Prior validation retained from the original PR (2026-10-05)

Test seams: the public methods of EmailIntegration (create_native_integration, update_native_integration, verify), and the email edit endpoint for the request user. The SES facade and posthoganalytics.feature_enabled are mocked as external boundaries. The flag fake answers by organization group or by distinct_id, like a real rollout.

Test rationale:

  • test_new_sender_keeps_the_domain_mail_from_label and test_new_sender_cannot_move_the_domain_mail_from_label catch path 1, including a legacy blank stored label. The closest existing test is test_duplicate_domain_in_same_organization_allowed; no existing test sent a label on create.
  • test_verifying_any_sender_keeps_a_changed_mail_from_label catches path 2. It edits one sender, then verifies every sender through instances loaded before the edit, and asserts the label sent to SES. It fails without the sibling write, and it fails without the re-read under the lock.
  • test_editing_a_sender_without_a_label_keeps_the_current_domain_label catches a name-only or blank edit that re-sets a label changed since the sender loaded.
  • test_each_sender_keeps_its_own_mail_from_label_while_the_flag_is_off pins master's behavior with the flag off: a different label on create, an edit that leaves the sibling alone, and a verify with the sender's own label.
  • test_verifying_any_sender_keeps_a_changed_mail_from_label runs once with the flag rolled out to the organization and once with it on for the acting user only.
  • test_editing_the_label_shares_it_when_the_flag_is_on_for_the_requesting_user sends a real PATCH and fails if the view stops passing the request user.
  • These are Django TestCases because the shared label lives in stored Integration rows.
  • No test races two real transactions. A threaded TransactionTestCase would be slow and timing-dependent. The lock follows the existing pattern in posthog/models/integration/codex.py.

Ran locally on an isolated test database: posthog/models/test/integration/test_email.py, the email tests in posthog/api/test/test_integration.py, posthog/test/test_domain_connect.py, products/workflows/backend/test/test_email_integration_ses.py (96 passed). Repo-wide mypy and hogli ci:preflight --strict passed.

Not checked: a real SES account, and GitHub Actions CI. The fork's workflow runs wait for maintainer approval.

Release status

Fork handoff only. This task does not activate rollout flags. Deployment flag state was not inspected. Upstream CI and the unavailable fresh Opus review remain handoff gaps.

  • No feature flag controls this change
  • This change is behind a feature flag and is not available to users
  • This change makes a previously flagged feature available to everyone

Docs update

None. No doc under docs/ covers the email sender MAIL FROM setting.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Claude Opus 5.5 (1M context). Review panel: GPT-6.1 Sol (Codex) and Claude Opus 5.5.

  • Split out of #109703 after its QA review found path 2.
  • Skills: /tdd, /writing-tests, /writing-user-facing-copy, /writing-pr-descriptions, /reviewing-with-coderabbit, qa-swarm, leaving-pr-comments.
  • Four review rounds. They added the per-domain lock, the blank-label handling, the scoped row lock, and stale-instance tests.
  • Later gated behind workflows-email-domain-agent-setup, so this PR and feat(workflows): let agents set up an email sending domain via mcp PostHog/posthog#109703 roll out together. Two more review rounds ran on the gating.
  • Rejected with reasons: one UPDATE per sibling (bounded by the senders on one domain), a deadlock with a project cascade delete (the rollback keeps every sender consistent and verify re-syncs SES), and moving the label reader onto EmailIntegration (the Domain Connect tests mock that class).
  • CodeRabbit CLI: skipped, because the CLI was signed out on the devbox. The CodeRabbit bot raised one thread on the PR. The lock fixed it, and the bot resolved it.
  • Duplicate search: gh pr list --search "mail from subdomain" found only feat(workflows): let agents set up an email sending domain via mcp PostHog/posthog#109703 and unrelated layers of its stack.

Modernization used Codex, gpt-6.1-sol. Skills: modernize-pr, pr, writing-pr-descriptions, writing-voice, merge-risk, unslop. The source replays onto master at b75567aca6cce9f0499e882463fc7e71d65a064c. Original authorship and prior evidence remain above. The required current Opus review could not run because the Claude API rate limit blocked it.

Current review fixes and final upstream replay used Codex, gpt-6.1-sol. The fresh cross-family review remains blocked by the known Claude rate limit.

@Silthus Silthus self-assigned this Oct 7, 2026
@Silthus
Silthus added this pull request to stack #355 October 7, 2026 10:43
SES stores the MAIL FROM subdomain on the domain identity, so every sender
on a domain shares it. Creating a sender with a different label moved it in
SES for every sender on the domain, which only the admin-gated update route
should do.

Create now reuses the domain's label when the request omits it, and rejects
a different label with a 400 before SES is called. Update writes the new
label to every sender on the domain in the organization, so their stored
config stays in step with SES.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature-flagged PR or stack includes flagged behavior; individual increments may be unconditional

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant