Refuse a recipient HEY would drop instead of losing the message - #534
Merged
Merged
Conversation
HEY does not refuse an address it cannot deliver to: it drops it, sends to whoever is left, and when nobody is left saves the message as a draft and redirects to it, which the SDK follows and reports as sent. So "a" in the To field closed the composer as if the message had gone. Every recipient is now checked first, by HEY's own rule: it parses as an address, bare or with a name, and its domain ends in a top-level domain on the public suffix list. The TUI composer stays open and names the address; hey compose, reply, forward and draft edit refuse it as a usage error before any request.
Codex compared the check with HEY's pinned mail and public_suffix gems and found it refused addresses HEY delivers to, which is worse than the silence it was meant to end: - whole country domains under a wildcard rule (.np, .ck, .jm) failed a lookup of the top-level domain alone; the whole domain is looked up now, with the top-level domain asked on its own only to see past a private rule. - net/mail rejects quoted local parts, comments and stray spaces that Ruby's parser accepts and HEY prefills in replies; an address net/mail cannot parse is judged on its bare address instead of refused outright. - the length limit counted bytes of the bare address; HEY counts characters of the whole address, name included. It also let through addresses HEY drops: a punycode or fullwidth top-level domain, which HEY's Unicode suffix list never matches, and an encoded word before the @. And with --account the account was resolved over the network before the check ran; the recipient flags are checked with the arguments now, which cobra does before the root's PersistentPreRunE.
Sending each disputed address to the dev server showed HEY's lookup comes down to whether the top-level domain is known: annie@np and annie@example..com are delivered, so only the last label is asked about, under a label so that a top-level domain with only a wildcard rule matches. An unclosed angle bracket and a second @ outside quotes are dropped, so the fallback parser refuses those. 34 addresses now agree with HEY's own parser.
The recipient check imports golang.org/x/net/publicsuffix and idna, which go mod vendor now includes.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Recipient parsing still rejects valid quoted names and permits some addresses HEY would silently drop.
Review effort: Balanced
Findings: 4
Open (5)
What changed in this PR
Adds pre-send recipient validation to prevent HEY from silently dropping invalid addresses.
Changes:
- Validates CLI and TUI recipients against HEY-compatible rules.
- Keeps TUI forms open and returns CLI usage errors for invalid addresses.
- Adds validation tests and user documentation.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
internal/mail/address.go |
Implements recipient validation. |
internal/mail/address_test.go |
Tests HEY-compatible address cases. |
internal/cmd/compose.go |
Adds shared CLI validation. |
internal/cmd/compose_test.go |
Verifies rejection before requests. |
internal/cmd/reply.go |
Enables validation for replies. |
internal/cmd/forward.go |
Enables validation for forwards. |
internal/cmd/draft.go |
Enables validation for draft edits. |
internal/tui/compose.go |
Validates TUI compose recipients. |
internal/tui/compose_test.go |
Tests TUI rejection and accepted prefills. |
docs/cli.md |
Documents CLI validation behavior. |
docs/tui.md |
Documents TUI validation feedback. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Every recipient list was split on each comma, so "Bryan, Annie" <annie@example.com> went out as two broken fragments, and with the check in place was refused as "Bryan. The CLI and the TUI now split with mail.SplitAddresses, which leaves a comma inside quotes, a comment or angle brackets alone, and the same split is used to check and to send. The fallback parser also counts a display name towards HEY's 500 characters, and compose, reply, forward and draft edit say in --help what they refuse.
The mail gem quotes a display name holding a comma or another special and escapes any quote or backslash inside, so a 479-character "Bryan, Annie"-style name with annie@example.com is 501 characters as HEY writes it. The size now includes those, keeping it a lower bound on HEY's.
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
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.
Typing
ain the TUI composer's To field and pressing Ctrl+S closed the form as if the message had gone.hey compose --to asaid "Message sent" too.HEY doesn't refuse an address it can't deliver to, it drops it. If nobody is left, it saves the message as a draft and answers the send with a redirect to that draft. The SDK follows the redirect, gets a 200, and reports success. If somebody is left, the message goes to them alone, so
a, annie@example.comreached Annie with no word abouta.Now every recipient is checked before anything is sent:
Not a valid email address: a.hey compose,hey reply,hey forward,hey draft edit: refuse with a usage error naming the address. The check runs with argument validation, so it happens before account resolution, uploads or the editor, and no request is made.The rule is HEY's own (
LenientMailFieldsParserplusContact::CertifiedMailAddress): a part before the@, a domain whose top-level domain is on the public-suffix list (orlocaldomain), and no more than 500 characters written out. Refusing an address HEY would deliver is worse than the bug, so the check leans towards letting addresses through: quoted local parts, comments and stray spaces that Go'snet/mailrejects but HEY accepts (and prefills in replies) are judged on the bare address instead.How it was checked: an adversarial Codex review compared it with HEY's pinned
mailandpublic_suffixgems and found both false rejections (wildcard country domains like.np, quoted local parts) and false acceptances (punycode TLDs, encoded words). All are fixed. I then sent every disputed address to a local HEY and checked the log for its "saved as a draft" rescue. 34 addresses now get the same verdict from this check as from HEY, and they're in the tests.Basecamp card: https://app.basecamp.com/2914079/buckets/48521764/card_tables/cards/10357051380
Recorded against the local dev server:
ain To, a subject and a body, then Ctrl+S.Follow-up in hey-sdk: a send that HEY answers by redirecting to the draft should come back as an error rather than success, as a safety net for anything this check can't know about (the recipient limit, for one).
Summary by cubic
Refuses recipients HEY would silently drop before any message is sent, so senders aren't told a message went out when it didn't. Previously, typing
ain the TUI composer's To field and pressing Ctrl+S closed the form as if the message had gone, andhey compose --to areported success, because HEY drops undeliverable addresses (or saves a draft when none remain) and the SDK follows the redirect as a successful send. Recipient lists are also no longer split on every,, so a comma inside a quoted name stays part of that one address instead of becoming a broken fragment.Now every recipient is validated first:
Not a valid email address: a);hey compose,hey reply,hey forward, andhey draft editrefuse with a usage error before account resolution, uploads, or any request, and their--helpstates what they refuse.LenientMailFieldsParserplusContact::CertifiedMailAddress): one address with a local part before@, a domain ending in a known public suffix orlocaldomain, and no more than 500 characters. It leans toward letting addresses through, so quoted local parts, comments, and stray spaces that Go'snet/mailrejects are judged on the bare address, the length counts the name HEY writes out (including the quotes and escapes HEY adds when the name needs them), and punycode/fullwidth TLDs and encoded words are refused as HEY would.Written for commit 1db1400. Summary will update on new commits.