Repository navigation
Conversation
There was a problem hiding this comment.
PostHog Review · Standard · reviewed ccb14b5: 1 Should fix.
This is the only automatic review while this pull request is a draft. Comment @posthog review to review new changes sooner. Otherwise, a review of the new changes runs when you mark it ready for review.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
| function base64Of(bytes: Uint8Array): string { | ||
| const chunks: string[] = [] | ||
| for (let start = 0; start < bytes.length; start += BASE64_CHUNK_BYTES) { | ||
| chunks.push(String.fromCharCode(...bytes.subarray(start, start + BASE64_CHUNK_BYTES))) | ||
| } | ||
| return btoa(chunks.join('')) |
There was a problem hiding this comment.
P2 · Use native Base64 encoding for Node attachment bytes
Sending multi-megabyte attachments blocks the event loop in base64Of, which spreads every byte into JavaScript arguments and builds intermediate strings. A 5 MiB Buffer took about 366 ms versus 3 ms with Buffer.toString('base64'); concurrent sends serialize this work and stall unrelated requests. Use native encoding on Node with an edge-compatible fallback.
There was a problem hiding this comment.
Fixed in 85cb4d9: Node encodes with Buffer.toString('base64'). Only the edge runtime, which has no Buffer, still uses btoa.
Coherence📄 0 spec file errors · ⛔ 0 broken rules · 🚪 2 unchecked inputs · 🐛 0 bugs that came back What is Coherence?Coherence compares the code with its spec files ( This check read 44 parts of the code and 7 rules. Note These findings do not make this check fail. None of these findings are in files that this pull request changes. You do not need to fix them in this pull request. 🚪 Unchecked inputs (2)
What this means and how to fix itInput from outside this code comes in at these places. The spec does not say what checks that input. 10 more places were already like this when Coherence was added. This list does not show them. To close one of these, find the place by its name in the
To get the line to add, run |
Node SDK acceptance v2Commit: 8dd465b Capture v0✅ 83 passed / ❌ 4 non-passing / Drill-down: run artifacts → ❌ server_snapshot_payload_false — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_zero — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_null — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ migration/yaml-parity-v1/remote-flags-v1.feature::Omitted GeoIP settings default to enabled in flag requests — failed_assertionFlags field differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v1.feature:134 the first flags request field "geoip_disable" should equal JSON false Operation: Field: Expected: false Actual value: true Harness exit: 1; report validation exit: 1. Capture v1✅ 148 passed / ❌ 4 non-passing / Drill-down: run artifacts → ❌ server_snapshot_payload_false — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_zero — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ server_snapshot_payload_null — failed_assertionExpected native data result Code: Failed step: acceptance/public/evaluate-flags.feature:868 the public snapshot outcomes should have these semantics: ❌ migration/yaml-parity-v1/remote-flags-v1.feature::Omitted GeoIP settings default to enabled in flag requests — failed_assertionFlags field differs Code: Failed step: migration/yaml-parity-v1/remote-flags-v1.feature:134 the first flags request field "geoip_disable" should equal JSON false Operation: Field: Expected: false Actual value: true Harness exit: 1; report validation exit: 1. |
The code form of posthog.emails.send() takes cc, bcc and attachments. A single address becomes a one-item list on the wire. Attachment bytes are Base64 encoded once, before the retry loop. The template form refuses these fields before any request. Co-Authored-By: Claude Opus 5.5 <[email protected]>
ccb14b5 to
a88a83d
Compare
Replay incident risk checkThis diff touches code involved in past incidents. This is a heads-up, not a verdict: read the matched sections of INCIDENTS.md and answer their review questions before merging. For a judgment on whether this diff has the same failure mode, run the |
|
Note 🤖 Claude Opus 5.5 writing on behalf of Michael Decisions for the attachments slice:
|
Array.map skips holes, so a sparse list sent null attachments past the check. A retry test now pins the bytes and key of the first attempt for a send with attachments. Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
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.
Independent assurance and STRONG author familiarity cover the risky areas, but @PostHog's substantive inline concern remains unfixed. Address the encoding concern in packages/node/src/emails/emails.ts with native Node encoding and an edge-compatible fallback, or obtain a non-author acceptance in the thread, before re-requesting.
- Author wrote 100% of the modified lines and has 0 merged PRs in these paths (familiarity STRONG).
- PostHog Review (Standard) reviewed the current head: no findings.
- @PostHog, packages/node/src/emails/emails.ts: the open performance concern remains unaddressed. base64Of still spreads byte chunks into JavaScript arguments, builds intermediate strings, and calls btoa rather than using native Node encoding; attachment conversion runs synchronously before the request.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 171L, 4F substantive, 290L/7F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1c-medium (290L, 7F, two-areas, feat) |
| stamphog 2.7.0 | .stamphog/policy.yml @ 5a1a78e · reviewed head 5a1a78e |
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
…nto emails-v2/04-code-fields # Conflicts: # packages/node/src/__tests__/emails.spec.ts # packages/node/src/emails/emails.ts
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
…ontentId Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Co-Authored-By: Claude Opus 5.5 <[email protected]>
New commits were pushed — dismissing the stamphog approval from an earlier head. This PR no longer qualifies for automatic review.
There was a problem hiding this comment.
Approved.
The disclosed API and write-path changes have current-head review assurance and a STRONG-familiarity author. The historical performance concern is addressed by native Node encoding, with regression coverage and no remaining substantive concerns found.
- Author wrote 100% of the modified lines and has 0 merged PRs in these paths (familiarity STRONG).
- PostHog Review (Standard) reviewed the current head: no findings.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 211L, 4F substantive, 377L/7F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (377L, 7F, two-areas, feat) |
| stamphog 2.7.0 | .stamphog/policy.yml @ 8dd465b · reviewed head 8dd465b |
Problem
An app moving its email from Resend to PostHog can't send a receipt with a PDF, or copy finance on an invoice. The code form of
posthog.emails.send()(#5509) takes renderedhtml/textonly.Impact: Grow Workflows. It removes a blocker for Resend migrations, seen as code sends with
migratedFromin project 2.Note
Merge danger: low, two-way door. New optional fields on an unreleased API. Release only after the server half deploys; until then a send with these fields gets a 400 that names the field, never a silent drop.
Changes
Re-cut onto #5509: the local-template work of #5513 (parked) is gone, and
headersandtagswait for a later slice.ccandbcc: one address or a list. A string becomes a one-item list. An empty list is left out, as feat(node): send email content from code with emails.send #5509 does forreplyTo.attachments:{ filename, content, contentType?, contentId? }. ABufferorUint8Arraybecomes Base64 once, before the retry loop: natively on Node, throughbtoaon the edge runtime. A string is sent as is. An empty list is left out, so an older server still sends the email.invalid_attachment, with the field: a non-list, a non-object item, apath(Resend style), an unknown key, a missing or emptyfilename, acontentTypeorcontentIdthat isn't a string, or content that is missing or isn't Base64. No error repeats the content.templateIdwith any of the three returnsinvalid_form, and the types refuse it.EmailAttachmentis exported.Test seams:
posthog.emails.send()on a real client with an injectedfetch, and the accepted and refused shapes as type tests.Proof gate passes at
8dd465b: CI green, review clean, and all three owed proofs captured. StampHog approved0d5386b; a new review is requested for8dd465b.attachmentsand nothing is sent. A plain send still delivers.replyToshape) post byte-identical requests on the base and on this branch.The Python twin is PostHog/posthog-python#1172.
Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Written by Claude Opus 5.5 (
claude-opus-5-5) in Claude Code, for Michael. The re-cut cherry-picked the old commitccb14b502onto #5509's head and dropped the local-template, headers and tags parts.Involvement: 2 questions answered. Reviews: GPT-6.1 Sol and Opus 5.5, rounds 1 and 2 and six final reads; PostHog Review; StampHog. Proof captured: live run, cross-version and shadow diff.
🤖 Generated with Claude Code