How should executeSendEmail decompose, and which seams do tests drive? #294

Activity

1. Silthus

   added

   wayfinder:prototype

   on Oct 6, 2026
2. Silthus

   added a parent issue on Oct 6, 2026

   • Map: refactor the workflow email send path and make SES sending testable for real #291
3. Silthus

   marked this as blocking on Oct 6, 2026

   • Missing EmailService cases through executeSendEmail (seam S1) #298
4. Silthus commented on Oct 6, 2026

   

   Silthus

   on Oct 6, 2026

   OwnerAuthor

   More actions

   Runtime: T3 Code
   Thread: thread:project:04faa8ec-7800-4e79-b460-1c525ea188e9:4fcd3d13-8e53-49b3-8edb-f0a0a4641db4
   Title: prototype #294: How should executeSendEmail decompose, and which seams do tests drive?
   Cast: claudeAgent / claude-opus-5-5 (architecture prototype is collaborative judgment; deep code research goes to GPT-6.1 Sol via delegate_task)
   Cap: 90 min AFK stretch per turn
5. Silthus

   self-assigned this

   on Oct 6, 2026
6. Silthus commented on Oct 6, 2026

   

   Silthus

   on Oct 6, 2026

   Last edited by Silthus

   OwnerAuthor

   More actions

   Parked: architecture brief, round 1 locked

   Brief: nodejs/src/cdp/services/messaging/email-send-path-architecture-prototype/index.html on branch prototype/email-send-path-shape (download and open it, or use the tailnet preview link in the T3 thread). Baseline: PostHog#112295's branch at 9668491.

   Proposed shape: executeSendEmail stays the only public method. Inside, one EmailSend request flows through send gates → sender check → suppression → tracking decision → pacing (WorkflowPacing, TeamSendingCap) → an EmailProvider adapter (SES, maildev). Every step returns a SendOutcome (sent / skipped / rescheduled / failed), and one applyOutcome writes the queue fields, metrics, VM stack and events. With one reschedule writer, the PostHog#112295 bug class can only happen in one place. Tests drive two seams: executeSendEmail for the outcome matrix, and an email-worker round-trip through the real job codec for reschedules.

   Locked so far

   • L1 executeSendEmail(invocation, isTest) stays the single public method, signature unchanged (map destination 1).
   • L2 SES is faked at HTTP through sesEndpoint, never by stubbing send() (map charting decision).
   • L3 The refactor is behavior-preserving and lands after the harness; behavior changes are separate PRs (map Notes).
   • L4 Step order is preserved: integration lookup → suspension → pause → sender check → suppression → tracking → workflow pacing → team cap → UTM → provider.
   • L5 A reschedule writes no metric, VM push or event.

   Locked in round 1 (Michael, 2026-10-06, all as recommended)

   • C1 SendOutcome union, one applyOutcome writer. C2 EmailServiceDeps object; submodules built inside. C3 explicit gate calls in runSend. C4 EmailProvider adapters for SES and maildev.
   • C5 queue seam: email worker processInvocations through the real job codec, plus one SES-throttle case in workflows-e2e.serial.test.ts. C6 real Valkey limiter in executeSendEmail tests. C9 direct tests only for pure policy functions.
   • C7 unknown provider, C13 queueMetadata drop, C14 post-send failures: each a separate fix PR; the refactor preserves today's behavior. C8 skip billing: separate grilling.
   • C10 land R1-R2 now, R3-R5 after the sandbox stack (feat(workflows): keep sandbox sends out of tier history and record deliveries PostHog/posthog#111904 → fix(workflows): guard sandbox sends on empty settings and SES throttle PostHog/posthog#112071). C11 services/messaging/GLOSSARY.md in R3. C12 comment changes ride with their slice.

   Found while grounding (read from code, not reproduced)

   • Every reschedule drops queueMetadata, so a run can return to the hogflow queue at the wrong priority after a throttle or pacing delay. Same bug class as fix(workflows): retry ses-throttled emails instead of dropping them PostHog/posthog#112295.
   • An unknown provider value reports email_sent without sending (the provider switch has no default).
   • Suspended and suppressed skips are billed; paused skips are not.
   • Two paths can turn an email that SES accepted into an errored run or email_failed.

   Pending question

   No card is open. Do you accept the brief as a whole? On acceptance I post the handoff (decomposition, seams, slices, missing cases as proposed follow-up tickets) and close this ticket.
7. Silthus

   added

   wayfinder:awaiting-humanParked HITL ticket waiting on the humanParked HITL ticket waiting on the human

   on Oct 6, 2026
8. Silthus commented on Oct 6, 2026

   

   Silthus

   on Oct 6, 2026

   OwnerAuthor

   More actions

   No description provided.
9. Silthus commented on Oct 6, 2026

   

   Silthus

   on Oct 6, 2026

   OwnerAuthor

   More actions

   Resolved: accepted decomposition and test seams for executeSendEmail

   Michael accepted the architecture brief on 2026-10-06, with all 14 decision cards locked as recommended.

   Brief (primary source): email-send-path-architecture-prototype/index.html on branch prototype/email-send-path-shape at ab55572. Baseline: PostHog#112295's branch at 9668491. Line references in the brief point at that baseline.

   Decomposition

   • EmailService.executeSendEmail(invocation, isTest) stays the only public method, with an unchanged signature (L1).
   • The constructor takes one EmailServiceDeps object. The service builds its submodules inside; none is injected (C2).
   • One EmailSend request runs through the steps in today's order (L4):
     • integration lookup
     • EmailSendGates: suspension, then workflow pause
     • resolveSender
     • suppression gate
     • TrackingDecision
     • WorkflowPacing, then TeamSendingCap (both SendPacer)
     • UTM tagging
     • an EmailProvider adapter: SesEmailProvider or MaildevEmailProvider (C3, C4)
   • Every step returns a SendOutcome: sent, skipped, rescheduled or failed.
     • One applyOutcome writes the queue fields, metrics, VM stack push, assets and engagement events (C1).
     • Brief section 6 gives the outcome matrix it implements.
     • With one reschedule writer, a fix(workflows): retry ses-throttled emails instead of dropping them PostHog/posthog#112295-style dropped field can only happen in one place.
   • The sandbox sender becomes a third EmailProvider with provider-scoped gates (brief section 5.4).

   Test seams

   • S1 executeSendEmail:
     • drives the gate × outcome × test-send matrix;
     • runs on real Postgres;
     • reaches SES through sesEndpoint to the local SES fake (L2);
     • uses a real RateLimiterService on the test Valkey (C6).
   • S2 email worker round-trip:
     • CdpCyclotronWorkerEmail.processInvocations, with each result passed through invocationToV2JobInit / v2JobToInvocation (C5);
     • plus one SES-throttle case in the workflows-e2e.serial.test.ts email suite.
   • S3 pure policy functions: only these get direct tests (C9). They are the retry-delay math, the cap tables, and address and subject parsing.
   • Requirements on the harness ticket:
     • the fake keeps the raw SendEmail request (tenant, configuration set, tags, headers);
     • it can return a throttle for the next N requests;
     • tests reach it through sesEndpoint alone;
     • then sesV2Client becomes private.

   Refactor slices (behavior-preserving, after the harness, stacked)

   #	Slice	When (C10)
   R1	EmailServiceDeps constructor	now
   R2	Extract email-pacing.ts (WorkflowPacing, TeamSendingCap, delay math, cap tables)	now
   R3	EmailSend + SendOutcome + applyOutcome; add services/messaging/GLOSSARY.md (C11). Gets a qa-swarm.	after the sandbox stack (PostHog#111904 → PostHog#112071) merges
   R4	Extract gates, resolveSender, TrackingDecision	after the sandbox stack
   R5	EmailProvider adapters; sesV2Client private	after the sandbox stack

   Comment changes ride with the slice that moves the code (C12), following brief section 11:

   • Four "choke point" comments collapse into one.
   • The two "re-attach payload" comments collapse into one.
   • Two wrong comments (L648 and L1004) and one stale one (L996) are deleted.
   • The claimUpTo name in L507-510 is fixed.
   • Comments that explain a real why survive, including the cross-file invariant at L226-227.

   Proposed follow-up tickets (for the conductor to graduate)

   #	Kind	Ticket	Depends on
   T1	tests	Missing cases at S1: M5-M16, M18-M21 (brief section 9). Covers the reschedule footprint for all three causes, the unknown and unsupported providers, maildev rejection, skip-flag characterization, integration lookup rejection, failure before the tracking decision, team cap shadow mode, tier fail-open and per-recipient charge, token non-refund, test-send throttle, no token spent on skipped sends, post-send failures, and cc/bcc suppression.	harness (SES fake)
   T2	tests	Round-trip cases at S2: M1-M4 (throttle, workflow pacing and team cap retries really send; a pause during a wait is honored), M17 (origin priority after a reschedule). Plus one E2E throttle case.	harness
   T3	fix	Re-attach queueMetadata on reschedule. Every reschedule drops the origin queue and priority today (C13). Lands before R3.	T2 (M17)
   T4	fix	An unknown provider value fails the send instead of reporting email_sent (C7). Lands before R5.	T1 (M6)
   T5	grilling	Should suspended and suppressed skips be billable? Paused skips are not (C8).	none
   T6	fix	Work after the provider accepted (asset row, engagement-config read) cannot turn a sent email into a failure (C14).	T1 (M19, M20)
   R1-R5	refactor	The slices above	T1, T2

   Findings T3, T4 and T6 come from reading the code; none was reproduced. The map's existing follow-ups (refund on throttle, give-up limit) are unchanged. M15 pins today's token non-refund, so the refund ticket flips that case.
10. Silthus

    removed

    wayfinder:awaiting-humanParked HITL ticket waiting on the humanParked HITL ticket waiting on the human

    on Oct 6, 2026
11. Silthus

    closed this as completedon Oct 6, 2026