fix: v1 audit — twelve verified defects and gaps (#104–#115) - #129
Merged
Merged
Conversation
Closes #104, closes #105, closes #106, closes #107, closes #108, closes #109, closes #110, closes #113. Middleware registered with `use()` wraps every mounted driver, and both the circuit breaker and the rate limiter kept one piece of state for all of them. A failing Resend opened the circuit for a healthy mounted SES, and traffic to one provider spent another provider's quota — the opposite of what mounting a second provider is for. State is now keyed by destination through `_scope.ts`; stream is part of the key because a provider can limit its streams separately. `driverHandler` checked `ctx.signal?.aborted` only in the sequential branch, so an already-aborted batch on a driver with a native `sendBatch` was sent anyway. The check now runs before the branch is chosen. A single send was always cancelled correctly, which is what made the gap easy to miss. `SendContext.meta` was documented as the bag middleware leave notes in, and nothing ever handed it back — a middleware could only get data out by closing over a variable, which breaks under concurrency. The core now copies it onto the `EmailResult` and, on failure, onto the `EmailError`. `memoryIdempotencyStore` evicted only on a `get` of the same key, and an idempotency key is unique per message and read at most once. 200k expired writes retained 61 MB. It now sweeps on write and holds a hard cap. Addresses are deduplicated across `to`/`cc`/`bcc`, not only within each. The same address in `to` and `cc` was one copy on the SMTP/SES envelope and two entries for an API driver, so identical input behaved differently depending on the transport — exactly what a `fallback([resend, ses])` pair exposes. The 30s HTTP timeout is now a `timeoutMs` option on every API driver, and resend and postmark forward `ctx.signal`, so an abort cancels the request in flight rather than only discarding its result. `mock({ failWhen })` was passed a hardcoded index 0 on the `send` path, so an index-based predicate silently never fired outside `sendBatch`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GQAPDWLnDrdYHKhou3Ntvb
Closes #111, closes #114. Nothing checked the package was JSR-publishable until the release workflow was already running, and this repo has shipped a JSR break before — `bd98b85 fix(jsr): resolve slow-type errors so JSR publish succeeds`. A slow type is a property of an ordinary source edit, so it now fails the pull request that introduced it. The release does a dry run before publishing to npm as well: a JSR failure afterwards would leave the two registries on different versions. `@vitest/coverage-v8` was a devDependency nothing ran. It is wired up now, with the floor set at what the suite actually measured rather than at a number I liked the look of — 82/74/81/85 against a measured 83.86/76.16/83.15/86.86. It is a ratchet: it can only be raised. `connection.ts` is no longer excluded from the measurement. Excluding it was flattering the number by 3 points; it sits at 60% and that should be visible. Docs follow the behaviour changes: `ctx.meta` reaching the caller, the `timeoutMs` option, cross-field address dedupe, and why stateful middleware is keyed per destination. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GQAPDWLnDrdYHKhou3Ntvb
Closes #112, closes #115. Eleven exported symbols and the `mounts` constructor option were named by no test. They all worked — I checked each by hand — but nothing guarded them, so a refactor could break a public API without turning anything red. `perMessage` was the sharpest case: exported, documented, and with zero callers anywhere in the repo. `src/drivers/_smtp` was at 37% statements and 31% functions, the least-tested code in the repo and, having been carried over from 0.x untouched, the code the rewrite reviewed least. It is now at 80%/79%. The DKIM tests do not compare the signer against itself. They re-derive the relaxed body canonicalization and the signed header block from RFC 6376 and then verify the signature with Web Crypto, for both rsa-sha256 and ed25519-sha256 — so passing means a real verifier would accept the message, not that the code agrees with its own bugs. It does pass, which is the first actual evidence the signer is correct. The SASL tests assert the exact payloads the RFCs define, including the RFC 2195 example challenge for CRAM-MD5 and that the challenge reaches the HMAC decoded rather than as base64. `CRAM-MD5` and `XOAUTH2` had no test at all before. Pool tests drive the fake server: reuse across sends, retirement at `maxMessagesPerConnection`, idle expiry, and that a connection which failed mid-transaction is discarded rather than handed to the next message. Regression tests accompany every fix in the two preceding commits. 190 tests before, 269 now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GQAPDWLnDrdYHKhou3Ntvb
This was referenced Sep 1, 2026
Closed
Closed
productdevbook
added a commit
that referenced
this pull request
Sep 2, 2026
Thirteen defects in the driver layer, each reproduced with a runnable probe before it was filed and each closed here with a regression test. Issues #116–#128. ## Silent corruption and lost mail **#116 — a short text attachment arrived corrupt.** `isBase64("test")` was `true`, so a four-byte text attachment went on the wire unencoded and the recipient's client decoded it into `"µë-"`. No error, on any driver. ``` "test" isBase64 = true -> sent as "test" what the recipient decodes: "µë-" correct would have been: "dGVzdA==" ``` This one is not fixable by improving the guess: `"test"` is valid text *and* valid base64, and so is `"data"`, `"Name"`, `"MyReport"`. A string is now text unless the caller says `encoding: "base64"`. **#117 — a failed pool handoff hung `send()` forever.** `release()` shifted the waiter off the queue and *then* awaited `create()`. When that rejected — the provider stopped accepting connections, DNS went away — the waiter had already been removed and was never resolved or rejected. `smtp.ts` swallowed the rejection, so nothing was logged either. Waiters now carry both outcomes. **#118 — a body dying mid-stream was classified as permanent.** `await response.text()` sat outside the try, and `fetch()` resolves as soon as the *headers* arrive. A proxy timing out while the body streamed threw past the driver boundary and arrived as `PROVIDER` / `retryable: false` — so retry skipped it and the mail was simply lost. It is a retryable `NETWORK` failure now. **#119 — `resend.sendBatch` dropped `idempotencyKey` entirely.** `send()` set the header; `sendBatch()` did not. Since any list of two or more goes down the batch path, a retried batch duplicated every message in it. A batch now presents one key derived from its messages' keys — stable for the same batch, different for any other. **#124 — SMTP sent a completely empty email.** A template-only message passes the core's body check, reaches SMTP with no `text` and no `html`, and `buildMime` renders an empty part. It was transmitted, accepted, and reported `ok`. The fix is general: `driver.features` was declared by all six drivers and **read by nothing**. The core reads it now, and a message asking for something the driver has said it cannot do comes back `UNSUPPORTED`. Only that message fails — the rest of the batch goes out — and a driver that declares no features at all is not second-guessed. ## Wrong on the wire **#121 — a long non-ASCII subject broke two RFCs at once.** One encoded-word of 124 characters against RFC 2047's cap of 75, on a header line of **1997 octets** against RFC 5322's hard limit of 998. `foldHeader` could not help: base64 has no spaces to fold on. Now split across several encoded-words, never mid-character, and round-tripping exactly. **#122 — a quote in a filename opened a second parameter.** ``` Content-Disposition: attachment; filename="report.txt"; filename="payload.exe" ``` Quotes are escaped now, and a non-ASCII filename uses RFC 2231 (`filename*=UTF-8''…`) rather than an encoded-word, which is not valid in a parameter. **#120 — `resend.sendBatch` sent attachments to an endpoint that rejects them.** Resend's reference: *"The attachments field is not supported yet."* Behaviour therefore changed with list length — `sendBatch([a])` attached the invoice, `sendBatch([a, b])` did not. Such a batch takes the single-send path now. **#123 — neither batch driver chunked at the provider's cap.** 150 messages in one Resend request (cap 100), 600 in one Postmark request (cap 500). The provider rejected the request and that single error was reported against every message in it. **#125 — every SMTP connection announced `localhost.localdomain`.** `resolveLocalName()` reached for `globalThis.require`, which does not exist in an ESM module, so `req?.(…)` short-circuited, the hostname branch was unreachable, and the `try/catch` never even fired. Dead code that the docs described as working. It reads `node:os` properly now, once per driver, and keeps the fallback for a name that is not fully qualified. **#126 — Postmark dropped the first tag's value.** The comment above the code said *"the rest carry as metadata so nothing the caller set is silently dropped"*; `tags[0].value` was exactly what got silently dropped. **#127 — DKIM documented PKCS1 and only imported PKCS8.** `openssl genrsa` writes PKCS1, and Web Crypto has no PKCS1 import format at all, so the most common key file failed with an unhelpful `Invalid keyData`. PKCS1 keys are now wrapped in the RFC 5208 PrivateKeyInfo envelope — and the resulting signature is byte-identical to the one the same key produces as PKCS8, which is how I know the DER is right. **#128 — the composites re-initialized on every send.** `fallback` and `roundRobin` reach their legs directly, so the core's memoized `ensureInitialized` never saw them. Five sends, five `initialize()` calls. ## Verification 269 tests before, **293** now. Lint, format, typecheck, JSR dry run, build with every `.d.mts` present, bundle budgets, version consistency — all clean. Coverage 84.53% statements / 87.29% lines, above the floors set in #129. ## Note on versioning `Attachment.encoding` is additive, but #116 changes what happens to a string attachment that looks like base64 and was not declared as such. That is a corruption fix rather than a feature removal, but it is a behaviour change — worth **1.1.0** rather than a patch, and worth a line in the release notes. ## Noticed, not fixed Two leads in `connection.ts`, the least-covered file left at 60%: - `sendInternal` awaits `"drain"` with no error path. A socket erroring while backpressured could hang the write — the same shape as #117. Not reproducible against the current fake server, so unproven rather than refuted. - The STARTTLS upgrade leaves `data`/`error`/`close` listeners on the pre-upgrade socket. Exercising it needs a TLS-capable fake server, which the harness does not have. Both are worth a follow-up, and both need the test harness extended before they can be settled either way. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01GQAPDWLnDrdYHKhou3Ntvb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A broad audit of v1.0.0, verified by running code rather than by reading it, plus the fixes. Twelve issues opened and closed here: #104–#115.
What was wrong
Middleware state was global when it should have been per destination (#104, #105)
use()registers a middleware once and it wraps every mounted driver. Both the circuit breaker and the rate limiter kept a single piece of state for all of them.Mounting a second provider exists so a message can go somewhere else when the first is in trouble. A shared breaker made one provider's outage everyone's, and a shared bucket spent quota you are paying for elsewhere. State is now keyed by
driver+streamthrough a shared_scope.ts; stream is in the key because Postmark limits its streams separately.An aborted signal did not stop a batch (#106)
driverHandlercheckedctx.signal?.abortedonly in the sequential branch. With a nativesendBatchand more than one message, the batch went to the provider regardless:A single
send()was cancelled correctly, which is what made the gap easy to miss. Sending anyway is the one failure mode a cancel API must not have.SendContext.metawas write-only (#108)Documented as the bag middleware leave notes in, demonstrated in
docs/architecture.md— and never handed back. A middleware could only get data out by closing over a variable, which breaks the moment two sends overlap. It now rides on theEmailResult, and on theEmailErrorwhen the send fails.The default idempotency store never shrank (#107)
Eviction happened only on a
getof the same key, and an idempotency key is unique per message and read at most once. 200 000 already-expired writes retained 61 MB. It now sweeps on write and holds a hard cap.Transports disagreed about a duplicate recipient (#109)
dedupeAddressesran per field, never across. The same address intoandcc:Identical input, different delivery depending on which transport is configured — precisely what a
fallback([resend, ses])pair exposes. Deduped across fields now,towinning overccoverbcc.The HTTP timeout was unreachable, and aborts were not forwarded (#110)
30 seconds, hardcoded;
HttpRequest.timeoutMsexisted and nothing ever set it. Far too long behind a user-facing handler — the caller's own request times out first and the retry middleware never gets control. Now atimeoutMsoption on every API driver, and resend and postmark forwardctx.signalso an abort cancels the request in flight instead of only discarding its result.mock({ failWhen })was passed a hardcoded index (#113)failWhen(msg, 0)on thesendpath, sofailWhen: (m, i) => i === 1fired insendBatchand silently never fired otherwise. A test helper that quietly does nothing is worse than one that is missing: the test passes and proves nothing.What was missing
Nothing checked JSR publishability until mid-release (#111)
This repo has shipped a JSR break before —
bd98b85 fix(jsr): resolve slow-type errors so JSR publish succeeds. A slow type is a property of an ordinary source edit, and CI never looked.jsr:checknow runs on every pull request, and the release dry-runs before publishing to npm, so a JSR failure cannot leave the two registries on different versions.Coverage was declared and not measured (#114)
@vitest/coverage-v8was a devDependency nothing ran. Wired up with the floor set at what the suite actually measured — 82/74/81/85 against 83.86/76.16/83.15/86.86 — rather than at a round number.connection.tsis no longer excluded: excluding it was flattering the total by three points while it sat at 60%.Eleven public exports had no test (#112)
compose,perMessage,driverHandler,unwrap,isOk,toBatchResult,createUnsupportedError,createRequiredError,toEmailError,defineDriver,version, and themountsoption. All worked; nothing guarded them.perMessagewas exported, documented, and had zero callers anywhere.The SMTP internals were at 37% (#115)
DKIM, four SASL mechanisms and the connection pool — about 1 100 lines carried over from 0.x untouched, and therefore the code the rewrite reviewed least. Now 80%/79%.
The DKIM tests are the ones worth reading. They do not compare the signer against itself: they re-derive the relaxed body canonicalization and the signed header block from RFC 6376 and then verify the signature with Web Crypto, for both
rsa-sha256anded25519-sha256. Passing means a real verifier would accept the message. It passes — the first actual evidence the signer is correct.CRAM-MD5andXOAUTH2had no test at all; they now assert the exact RFC payloads, including that the challenge reaches the HMAC decoded rather than as base64.Verification
190 tests before, 269 now, with a regression test for every fix. Lint, format, typecheck, version consistency, build with every
.d.mtspresent, and bundle budgets all clean.Noticed, did not fix
connection.tssits at 60%; the uncovered lines are the STARTTLS upgrade path, which needs a TLS-capable fake server._ses/sigv4.tshas 66% branch coverage — the session-token and unusual-region branches are unexercised.retry.tsis at 70% branches; three of the five backoff strategies are only covered indirectly.email.send()afterdispose()silently re-initializes and works. Defensible, but surprising enough to be worth a decision either way.A separate audit of the driver layer is running and will file its own issues.
🤖 Generated with Claude Code
https://claude.ai/code/session_01GQAPDWLnDrdYHKhou3Ntvb