Repository navigation
fix(workflows): drop suppressed cc/bcc instead of skipping the email - #118456
Conversation
|
😎 Merged successfully - details. |
🤖 CI report✅ Trunk lane — non-backend laneThis 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) — cleanNew 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) — cleanNew 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 — 11% of added code lines are comments (30 of 284)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 Files with the most added comment lines:
This check does not block merging. It updates on every push and clears when the share drops. |
|
|
|
Risk: No findings This increment closes the quoted-local-part bypass in Sentinel reviewed |
![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
Owning-team authorship provides assurance for the risky changes, but @parameterai[bot]'s substantive parser concern remains unaddressed. Fix the suppression parsing mismatch and obtain review from @PostHog/team-workflows before re-requesting; Meikel Ratz or Arthur Moreira de Deus are suggested reviewers.
- @parameterai[bot]'s discussion concern remains in nodejs/src/cdp/services/messaging/helpers/suppressed-recipients.ts:25-30: a bracketed address followed by non-whitespace text is checked as the entire entry instead of its bare address. EmailSuppressionService only trims and lowercases that lookup, while email.service.ts:1129-1140 forwards the retained entry to SES unchanged. This leaves the reported suppression-check mismatch unfixed. Normalize supported address forms consistently with delivery parsing and add a trailing-comment regression test; request review from @PostHog/team-workflows before re-requesting.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 200L, 3F substantive, 355L/6F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (355L, 6F, single-area, fix) |
| stamphog 2.6.0 | .stamphog/policy.yml @ 944c494 · reviewed head 944c494 |
|
No review started. stamphog started a review of this pull request less than 10 minutes ago, and a label can request at most one review per 10 minutes. The |
![parameterai[bot]](https://avatars.githubusercontent.com/in/2665233?s=60&v=4){kind=small}
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 1 · PR risk: 0/10 |
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
Owning-team authorship supplies assurance, but both reviewers' substantive suppression-bypass concerns remain unresolved. Address @parameterai's and @veria-ai's comments with quote-aware mailbox parsing shared by suppression and delivery, add regression coverage, and request review from @PostHog/team-workflows before re-requesting.
- @parameterai on nodejs/src/cdp/services/messaging/helpers/suppressed-recipients.ts:20: the parser takes the first angle-bracket group, including one inside a quoted display name, rather than the actual mailbox. Lines 57–75 check that incorrect identifier and retain the original entry; email.service.ts:328–336 and 1130–1140 forward it to SES without correcting the mailbox, leaving the reported suppression bypass unfixed.
- @veria-ai on nodejs/src/cdp/services/messaging/helpers/suppressed-recipients.ts:20: the same quoted-display-name mismatch permits a blocked CC or BCC mailbox to survive suppression filtering when the display name contains an unsuppressed address. The current code does not reconcile the checked identifier with the delivered mailbox.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 201L, 3F substantive, 361L/6F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (361L, 6F, single-area, fix) |
| stamphog 2.6.0 | .stamphog/policy.yml @ c49c0d9 · reviewed head c49c0d9 |
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
Owning-team authorship provides assurance for the risky changes, and the earlier quoted-display-name concern is addressed. Address @parameterai[bot]'s remaining parser/delivery mismatch concern and request review from @PostHog/team-workflows before re-requesting.
- @parameterai[bot], discussion comment concerning nodejs/src/cdp/services/messaging/email.service.ts and helpers/suppressed-recipients.ts: the no-removals path still discards the helper's reconstructed recipient lists and sends the original strings. This leaves the reported quoted-local-part parsing concern unaddressed; suppression lookup only trims and lowercases identifiers, while delivery receives a different representation. Use a consistent mailbox representation for suppression and delivery on every path, and add a quoted-local-part regression test before re-requesting.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 212L, 3F substantive, 380L/6F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (380L, 6F, single-area, fix) |
| stamphog 2.6.0 | .stamphog/policy.yml @ 34bdd8d · reviewed head 34bdd8d |
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
The author belongs to @PostHog/team-workflows, but @parameterai's substantive concern remains present in the current code. Address that thread and request follow-up review from @PostHog/team-workflows before re-requesting; Meikel Ratz and Arthur Moreira de Deus are suggested reviewers.
- @parameterai on nodejs/src/cdp/services/messaging/helpers/suppressed-recipients.ts: the quoted-local-part concern remains unfixed. The helper checks the primary recipient without unquoting it, while EmailSuppressionService only trims and lowercases identifiers. email.service.ts also passes that quoted identifier to tracking-consent lookup and SES delivery, so a stored bare-address suppression or tracking opt-out is not matched. Normalize the primary recipient consistently for suppression and consent, preserve valid delivery formatting, and add regression tests before re-requesting review.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 237L, 3F substantive, 410L/6F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (410L, 6F, single-area, fix) |
| stamphog 2.6.0 | .stamphog/policy.yml @ 7e60173 · reviewed head 7e60173 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
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 |
A suppressed address in cc or bcc no longer stops delivery to everyone else on the email. Suppressed cc and bcc addresses are removed at send time. When the to address is suppressed, the first deliverable cc address takes its place. The send is skipped only when no cc address is left. Bcc addresses are never promoted, so hidden recipients stay hidden.
900f249 to
1e9acf7
Compare
|
/trunk merge |
A new stamphog review started for this PR — the fresh verdict replaces this approval.
There was a problem hiding this comment.
Approved.
The change enters risky territory, but owning-team authorship supplies the policy's assurance path and the diff includes focused tests. No unresolved substantive concern was established from the current code; the reported maildev test failure alone does not demonstrate a production defect.
- Author wrote 8% of the modified lines and has 5 merged PRs in these paths (familiarity MODERATE).
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 251L, 3F substantive, 435L/6F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (435L, 6F, single-area, fix) |
| stamphog 2.6.1 | .stamphog/policy.yml @ 1e9acf7 · reviewed head 1e9acf7 |
Problem
Raised in Slack: https://posthog.slack.com/archives/C06GG249PR6/p1792145215385039
Changes
RecipientPreferencesService) and the send-time check (EmailService) share one helper,removeSuppressedRecipients, so they cannot disagree about when to skip.Note
A workflow that addresses one person in to and a colleague in cc now sends to the colleague when the person's address is suppressed. Before, the step was skipped.
How did you test this code?
helpers/suppressed-recipients.test.tsis new and covers the removal and promotion rules at the cheapest level. No existing test covered partial delivery.email.service.test.tsgets one wiring test that checks the SES destination after promotion. It catches a send-time path that ignores the helper result.recipient-preferences.service.test.ts, the cases that expected a suppressed cc or bcc to skip the step now expect the step to run. A new case checks that a step with only bcc left is still skipped.Test rationale: the new cases fail if a suppressed cc again blocks the whole email, or if a bcc address is promoted to to.
Release status
Docs update
None. No doc in
docs/covers suppression behaviour.🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: PostHog Desktop (Claude Code), claude-opus-5-5
/writing-tests,/writing-code-comments,/writing-pr-descriptions.Created with PostHog Desktop
🤖 Generated with Claude Code