Skip to content

feat(cdp): deliver sandbox email to members' stored addresses - #129050

Closed
Silthus wants to merge 57 commits into
workflows-sandbox-split/member-cachefrom
workflows-sandbox-split/stored-member-address
Closed

Silthus wants to merge 57 commits into
workflows-sandbox-split/member-cachefrom
workflows-sandbox-split/stored-member-address

Conversation

@Silthus

@Silthus Silthus commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Problem

  • The member check ignores case, but the worker delivers to the address as the step wrote it. A step that writes a member's address in another case sends to an address the member never stored.
  • Part 3 of the Node caps stack, on #128946: #129048 → #129049 → this PR → #129051 → #129052. Split from #128410, which stays open until this split replaces it.
  • Landing order: the Node pieces deploy after the skeleton and before the flag reaches any team beyond ours.

Merge danger

Warning

🟡 Merge risk: medium · two-way door (merge-risk rubric)

  • Sandbox sends now go to the member's stored address in To, CC and BCC. A bug here sends a sandbox email to the wrong mailbox or skips it, with nobody told.
  • What limits it: only sandbox sends change, and no team has the sandbox sender until the flag provisions one. Own-domain sends keep the step's addresses.
  • Revert: git revert the merge commit. Nothing to clean up.

Changes

  • Sandbox sends deliver to each member's address as the member stored it, for To, CC, BCC and test sends.
  • When two verified members stored the address in different cases, the step must write one of them exactly, or the send skips. Two stored values equal after trim count as one.
  • checkRecipients returns { blocked, deliverTo } instead of the blocked list alone.

How did you test this code?

  • Live run and shadow diff at this head: Not run yet.
  • email.service.test.ts covers mixed-case and display-name recipients, quoted commas and escaped quotes, in workflow and test sends. It reads the stored addresses back from the database.
  • organization-members.service.test.ts checks that the lookup returns the stored address, and that two casings equal after trim deliver to one mailbox.
Earlier runs at older heads, not this head

Live run at the #129052 head bb10d28, which contains this PR. Same devbox kit as #128946: private Django and Node cdp-api, a seeded org, a local server that records each SES request, and the dev Valkey for the allowances. Sends used the editor's "Run test" path.

Shadow diff at this PR's head 47e749e against its base #129049 00c9b47, eleven sandbox sends through the same kit:

Sends Result
Member typed in another casing in To, or in lower case in To or BCC SES receives the stored address in place of the typed one
MEMBER@ that matches neither of two stored casings Skipped as blocked; the base sent it to the typed address
Exact member, mixed members in To, CC and BCC, outsiders, unverified members, blank CC entries Same outcome and the same SES request body as the base

Test rationale: each case catches a sandbox email delivered to an address no member stored.

Release status

  • This change is behind a feature flag and is not available to users

Docs update

  • docs/internal/workflows-email-sending-tiers.md says the worker delivers to the stored address.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Claude Opus 5.5

Skills: /writing-tests, /writing-code-comments, /stacking-prs, /writing-pr-descriptions

More context for AI agents
  • Session: Claude Code. Review rounds 1 and 2 and final reads 1 to 6 ran on Claude Opus 5.5 only, because the GPT reviewers were unreachable for the whole session.
  • Decisions: the risk grade went from low to medium, because a bug here misroutes a sandbox email without a visible error.
  • Decisions: the member service returns stored addresses, and the caller remaps the route, so the service knows nothing about SES.
  • Follow-ups: a verified Twin@ and an unverified twin@ still deliver twin@ to the verified member. That decision stays open.
  • Follow-ups: two verified casings with a step that matches neither skip with "not a member"; a log hint could list the stored casings.
  • Read closely: checkAgainst in organization-members.service.ts, and checkSandboxRecipients in email.service.ts.
  • Origin: split from #128410. The merged tops of the new stacks match the old stack's production code; see the split report.

Stack decisions, moved here from the visible body:

Question Answer Who Where
Who gets a sandbox email Verified members of the team's organization, checked at send time over To, CC and BCC 👤 Human decision record
Sender identity Fixed From address on a dedicated PostHog domain, "Acme via PostHog" display name, no From or Reply-To override 👤 Human decision record
Where it can send Workflow email steps and test sends only, not broadcasts or transactional email 👤 Human decision record
How the sender is stored One managed email integration per team, provider sandbox, created only by PostHog code 👤 Human decision record
SES setup One shared identity, configuration set and tenant for all sandbox sends 👤 Human decision record
Landing order Three-PR skeleton, then the flag to our team, then caps and polish (replaced landing the twelve-layer stack at once) 👤 Human decision record
Node gate Environment switch WORKFLOWS_SANDBOX_SENDER_ENABLED, no feature flag in CDP Node code 👤 Human decision record
Member check in the skeleton Keep it, although the ask was a skeleton with no limits 👍 Accepted default decision record
Behavior with everything off No change while the flag and switch are off 👍 Accepted default decision record
Earlier PRs Close #127149 to #127153, #127136 and #127137, each linked to its successor 👍 Accepted default decision record
PR shape Two native stacks of small PRs: Node (4) and product (5) 👍 Accepted default decision record
Member lookup cache in the skeleton Dropped; every check is one fresh lookup, the cache returns with the caps stack 👍 Accepted default decision record
Member lookup that gets its connection late Runs no query; the cap on waiting lookups returns with the cache 🤖 Agent decision record
Tier budget Sandbox sends never spend the team's tier budget 🤖 Agent decision record
When members are checked Before preparing the send and again fresh before delivery 🤖 Agent decision record
Member lookup fails Skip the send with a log 🤖 Agent decision record
Blocked-send events At most one per team, reason and hour 🤖 Agent decision record
How a team gets the sender Opening Channels with the flag on creates it 🤖 Agent decision record
Managed sender guards Nobody can edit, verify, delete or default the sandbox sender 🤖 Agent decision record
Quoted member addresses Skip the send, and canonicalize in the caps stack instead of respinning the skeleton 🤖 Agent decision record
Re-split of #128410 to #128414 Pieces of about 400 changed lines or fewer, Node and product code in separate PRs 👤 Human requested in the agent session that wrote this split

🤖 Generated with Claude Code

The member check ignores case, so the worker now sends to each member
address as the member stored it, for To, CC and BCC, instead of the
address the step wrote.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

✅ Trunk lane — non-backend lane

This PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes.

✅ Duplication (Python) — clean

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Duplication (TypeScript) — clean

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Comment density — 0% of added code lines are comments (0 of 202)

This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.

Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.

This check does not block merging. It updates on every push and clears when the share drops.

Silthus and others added 18 commits November 4, 2026 11:58
…ndbox-split/stored-member-address

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ndbox-split/stored-member-address

# Conflicts:
#	nodejs/src/cdp/services/managers/organization-members.service.test.ts
…it/stored-member-address

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

# Conflicts:
#	docs/internal/workflows-email-sending-tiers.md
#	nodejs/src/cdp/services/managers/organization-members.service.test.ts
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When two members stored an address in different cases, a step that wrote a
third case was delivered as written, to a mailbox no member verified. That
send now skips, unless the step writes one of the stored addresses exactly.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…it/stored-member-address

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…it/stored-member-address

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…it/stored-member-address

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

# Conflicts:
#	nodejs/src/cdp/services/managers/organization-members.service.test.ts
…it/stored-member-address

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…it/stored-member-address

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…it/stored-member-address

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…it/stored-member-address

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…it/stored-member-address

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…it/stored-member-address

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Silthus and others added 3 commits November 4, 2026 21:08
…it/stored-member-address

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…it/stored-member-address

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…it/stored-member-address

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…it/stored-member-address

# Conflicts:
#	nodejs/src/cdp/services/messaging/email.service.ts
@stamphog
stamphog Bot dismissed their stale review November 6, 2026 08:17

New commits were pushed — dismissing the stamphog approval from an earlier head. Re-add the trigger label to request a fresh review.

Silthus and others added 2 commits November 6, 2026 08:32
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
stamphog[bot]

This comment was marked as outdated.

Silthus and others added 3 commits November 6, 2026 18:44
…it/stored-member-address

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Two member rows can hold the same address once trimmed. Keeping both copies made the
single-casing shortcut fail and blocked a send to the only mailbox there is.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Drop the empty placeholder map in checkSandboxRecipients, and read the stored addresses
back in the delivery test instead of recomputing initcap's output.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@stamphog
stamphog Bot dismissed their stale review November 6, 2026 19:43

New commits were pushed — dismissing the stamphog approval from an earlier head. Re-add the trigger label to request a fresh review.

Silthus and others added 11 commits November 6, 2026 20:24
…it/stored-member-address

# Conflicts:
#	docs/internal/workflows-email-sending-tiers.md
#	nodejs/src/cdp/services/managers/organization-members.service.test.ts
#	nodejs/src/cdp/services/managers/organization-members.service.ts
# Conflicts:
#	nodejs/src/cdp/services/managers/organization-members.service.ts
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…it/stored-member-address

# Conflicts:
#	nodejs/src/cdp/services/managers/organization-members.service.test.ts
stamphog[bot]

This comment was marked as outdated.

Silthus and others added 2 commits November 9, 2026 04:01
…it/stored-member-address

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…it/stored-member-address

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@stamphog
stamphog Bot dismissed their stale review November 9, 2026 06:03

New commits were pushed — dismissing the stamphog approval from an earlier head. Re-add the trigger label to request a fresh review.

@veria-ai

veria-ai Bot commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Reviews

@Silthus
Silthus marked this pull request as draft November 9, 2026 10:02
@Silthus

Silthus commented Nov 9, 2026

Copy link
Copy Markdown
Contributor Author

Note

🤖 Claude Opus 5.5 responding on behalf of Michael

Replaced by #131731 in the isolated sandbox rebuild.

@Silthus Silthus closed this Nov 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant