Skip to content

Fail the send when the email provider is unknown #311

Closed

Description

@Silthus

Part of #291

Question

Fix: an unknown provider value on the email integration reports email_sent without sending, because the provider switch in executeSendEmail has no default. Make it fail the send with a clear error (brief card C7, accepted brief). Found by reading code: the S1 case M6 is the red test. Lands before refactor slice R5. Behavior-change PR.

Activity

  1. Silthus commented on Oct 6, 2026

    @Silthus
    OwnerAuthor

    Scope note from the conductor: build on PostHog#112850's branch fix/workflows-email-reschedule-keeps-queue-metadata. Also fold in #310's test follow-up: the paused-workflow fixtures in the email tests keep a PubSub reload listener after a test ends (CodeRabbit found it on PostHog#112850). Give them a removable or one-shot subscription.

  2. Silthus commented on Oct 6, 2026

    @Silthus
    OwnerAuthor

    Runtime: T3 delegate_task (app-owned child of the conductor thread)
    Task: wf291-implement-311-r1
    Cast: codex / gpt-6.1-sol, reasoning high (bounded fix); review: claude -p claude-opus-5-5, or qa-swarm if merge-risk names a danger area
    Branch: fix/workflows-email-unknown-provider-fails (stacked on fix/workflows-email-reschedule-keeps-queue-metadata, PostHog#112850)
    CodeRabbit slot: 1 of 3
    Cap: 150 min wall time from 19:18 UTC

  3. self-assigned this
    on Oct 6, 2026
  4. Silthus commented on Oct 6, 2026

    @Silthus
    OwnerAuthor

    Note

    🤖 GPT-6.1 Sol responding on behalf of Michael

    Draft PR: PostHog#112859, stacked on PostHog#112850. The red/green proof and local gate are linked in the description. QA review is in progress.

  5. Silthus commented on Oct 6, 2026

    @Silthus
    OwnerAuthor

    Note

    🤖 GPT-6.1 Sol responding on behalf of Michael

    Resolved in draft PostHog/posthog#112859, stacked on PostHog#112850, targeting master from Silthus:fix/workflows-email-unknown-provider-fails.
    Reviewed and pushed head: bbc567c44fd402c06235a0d5511a20d47ec17f3d. The PR is linked to this worker and the conductor's T3 thread.

    Unknown providers now fail with Email provider not recognized. Select a different email integration. in the run log, a failed VM result, email_failed and $workflows_email_failed, no sent metric/event, no asset, and no delivery. unsupported still fails. The paused S2 fixture's temporary reload acknowledgement now uses a one-shot listener.

    Proof: public bundle.

    • Red: exact final M6 against parent a63fc2aa968a4; only M6 fails, 31 other S1 cases pass, none skipped.
    • Green: final tree, all 158 S1/S2/existing email-service tests pass; every assertion remains, none skipped.
    • Node.js lint and typecheck: both exit 0.
    • Final pre-push hook: hogli ci:preflight --strict, zero failures, push succeeds. No --no-verify or disabled hook. Exact commands and the dedicated test-database wrapper are in the bundle.

    Merge risk: medium, one-way recorded run outcomes and engagement events. The layer changes two production lines. Supported providers and absent/null SES fallback remain unchanged. This is a real-send danger area, so qa-swarm replaced the single-review step.

    Review calls and outcomes:

    • CodeRabbit: .codex/with-flox cr auth status --agent returned not_authenticated. AFK skip authorized; no CodeRabbit review slot was consumed. No bot review threads reported at the final read.
    • Three read-only GPT-6.1 Sol / high T3 delegate_task calls on 34e778e4, scoped against upstream/master excluding all parent commits: wf311-qa-correctness-r1-34e778e4, wf311-qa-risk-r1-34e778e4, wf311-qa-design-r1-34e778e4. Each returned no findings and covered all three changed files.
    • Read-only Opus round 1: claude -p --model claude-opus-5-5 --allowedTools Read,Grep,Glob --tools Read,Grep,Glob --no-session-persistence, waited for exit 0. One finding: dead success conditionals and a needless non-null assertion in M6/M7. Verified and fixed in bbc567c44fd40, preserving every failure/log/delivery assertion.
    • Read-only Opus round 2: same command, new session, only 34e778e4...bbc567c4; waited for exit 0. No findings; the changed test file is covered. Production and fixture code did not change after round 1.
    • No rejected findings, unresolved decisions, model substitutions, or review outages. The final local gate is green.

    Skips and limits: upstream workflows report action_required for the fork PR, so no upstream CI approval wait. GitHub denied the assignee update; the draft remains unassigned. Default preflight compares with the fork's old master, producing unrelated staleness/OpenAPI advisories and a Semgrep time-budget skip. An additional strict check against upstream/master also passed. No parent branch or PR was changed. No real SES or browser test was run.

    Proposed follow-ups for the conductor:

    • Clean up the existing constructor-only email-worker tests at cdp-cyclotron-worker-email.consumer.test.ts:47 and :52; their Redis/Valkey pools remain open. snappy also leaves a native handle. Observed with --detectOpenHandles. The suite uses the repository's --forceExit convention; this patch only removes the requested temporary reload listener.
    • Consider tokens spent on unknown/unsupported provider failures in the existing refund-policy work. Pacing happens before dispatch; this PR keeps that policy unchanged.

    The SES profile service was already running. No other Docker service was restarted or stopped. No downstream work was dispatched.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions