Skip to content

refactor(cdp): accept transactional sends in their own module - #128871

Open
Silthus wants to merge 7 commits into
scheduled-sends/n1-job-codecfrom
scheduled-sends/n2-transactional-sends
Open

Silthus wants to merge 7 commits into
scheduled-sends/n1-job-codecfrom
scheduled-sends/n2-transactional-sends

Conversation

@Silthus

@Silthus Silthus commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

Problem

  • Scheduled email sends (Silthus/personal#303) add time rules, cancel and reschedule to transactional sends. The send logic sits inside the cdp-api route handler, so each change grows the handler.

Merge first: #128868.

Warning

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

  • Every transactional email request runs the moved code.
  • The idempotency fingerprint must stay identical, because pods of both versions share stored keys during a deploy.
  • Revert: git revert the merge commit.

Landing: #129915 merges alone and its migration runs in US and EU first. #127745 merges next. Then one /trunk merge on #128929 lands N1 to N4.

Changes

  • Nothing user-visible changes: the PR moves code.
  • TransactionalSends.accept owns the send: idempotency, refusal checks, run row, queue insert and status row.
  • The route keeps the token check, body parsing, the team and workflow loads, and maps each outcome to its HTTP answer.
  • The CI report's comment-density warning counts moved comments. Every comment in the new module comes from the handler.
flowchart LR
  subgraph Before
    R1[send route: auth, parse, load, accept, answer]
  end
  subgraph After
    R2[send route: auth, parse, load, answer] --> T2[transactional sends: accept]
  end
  classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff;
  classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000;
  class T2 phBlue;
  class R1,R2 phGray;
Loading

How did you test this code?

  • cdp-api.serial.test.ts passes unchanged.
  • A scratch shadow run sends 14 scenarios through N1 and this PR. The answers, queued runs, fingerprints, status rows and metrics match.
Shadow run
  • Scenarios: template and code sends, a keyed send and its replay, a reused key, a key in flight, no key store, a draft workflow, sending suspended, sending paused, a suppressed recipient, quota on marketing and transactional flows, an unreachable recipient, and a failed insert with its retry.
  • Captured per scenario: each HTTP answer, each queued run, each reserve, markQueued and release call with its fingerprint, the status and result rows sent to Kafka, and the outcome and stage counters.
  • Result: the only differences are status row versions, which embed the send time. Compared by status rank, the two runs match.
  • Run at this PR's head fa5a21f against N1's head 0b6fb84.
  • The scratch test is not committed.

Release status

  • No feature flag controls this change

Docs update

  • None.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Claude Opus 5.5

Skills: /writing-pr-descriptions

More context for AI agents
  • Session: T3 Code stack thread for Silthus/personal#303, Claude Code.
  • Decisions: cdp-api builds the module per request from its current fields. The serial suite swaps the queue and the idempotency store on the API, and it stays unchanged.
  • Follow-ups: N3 adds scheduled_at and the time rules inside accept.
  • Read closely: nodejs/src/cdp/services/hogflows/transactional-sends.ts against the removed handler code.

🤖 Generated with Claude Code

@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 — 6% of added code lines are comments (17 of 286)

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.

Files with the most added comment lines:

File Comment lines Added lines
nodejs/src/cdp/services/hogflows/transactional-sends.ts 17 241

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

@Silthus
Silthus marked this pull request as ready for review November 5, 2026 07:33
@parameterai

parameterai Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

Risk: Low · 1 low

The change since the last review is a one-line lint cleanup in the new TransactionalSends module: the unused to binding is dropped from the destructure in accept, and to is still read from request in buildInvocation and transactionalEmailRecipients, so no behavior changes. I also re-verified deploy fingerprint parity (byte-identical to the removed handler) and re-checked the attachment filename validation, where the trailing-dot/space blocked-extension bypass from the earlier finding remains unmitigated.

Sentinel reviewed f1c6566 · Review settings

@pr-assigner-resolver-posthog
pr-assigner-resolver-posthog Bot requested a review from a team November 5, 2026 07:34
…ransactional-sends

# Conflicts:
#	nodejs/src/cdp/cdp-api.ts
#	nodejs/src/cdp/services/hogflows/transactional-email.ts
…ransactional-sends

# Conflicts:
#	nodejs/src/cdp/cdp-api.ts
}
if (files.length) {
stages.start('files')
await this.deps.emailAttachments.store(invocation.id, team.id, files)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Trailing dot or space in an attachment filename bypasses the blocked-extension list

A customer can attach an executable to a transactional email by ending the filename with a dot or a space: invoice.exe. or invoice.exe passes the blocked-extension check, and Windows strips trailing dots and spaces when the recipient saves the file, leaving a runnable .exe. The new module's store call at transactional-sends.ts:127 passes these files straight into the send pipeline.

How:

  1. In the attachment filename check (transactional-email.ts:153-159), name.lastIndexOf('.') on invoice.exe. finds the trailing dot and name.slice(dot + 1) yields '', which is not in BLOCKED_ATTACHMENT_EXTENSIONS — the suite otherwise expects setup.EXE to be rejected.
  2. A trailing space works too: CONTROL_CHARACTER_RE (transactional-email.ts:141) covers \x00-\x1F\x7F but not 0x20, so invoice.exe slices to 'exe ' and passes.
  3. The file is stored and delivered to the recipient under that name; SES's own executable filter may not strip trailing dots/spaces the way Windows does.

Fix: strip (or reject) trailing . and space characters from the filename before taking the extension, or reject filenames that end in either.


React with 👍 if useful or 👎 if not

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid, but the filename check comes from master and this PR leaves it unchanged. It gets its own fix outside this stack.

This branch has not been deployed

No deployments
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.

2 participants