Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Issues Found
Important (P2)
- [P2] Revert docker-compose to published listmonk image (
docker-compose.yml:11-16)docker-compose.ymlnow referencesimage: listmonk-custom, which will fail for anyone using the repo's compose file without building/publishing that image first. This is a user-facing regression in the default local/deployment workflow and should point back to an official/published image (or be removed from the PR if it was only for local testing).
Summary
Total issues: 0 critical, 1 important, 0 minor.
Overall Verdict
Status: Patch has blocking issues
Explanation: The SMTP routing changes look internally consistent, but the included docker-compose.yml change makes the default compose setup unusable by pointing to a non-existent image name. This should be reverted or adjusted before merging.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 19 turns, 24 tool calls, 3m 37s
- Tokens: in
22.1K| cached302.7K| out10.8K(total335.6K) - Cost:
$0.2431
jaymzh
force-pushed
the
email-routing
branch
2 times, most recently
from
de25012 to
abf5b77
Compare
There was a problem hiding this comment.
Issues Found
No issues found.
Summary
Total issues: 0 critical, 0 important, 0 minor.
Overall Verdict
Status: Patch is correct
Explanation: The diff correctly adds from_addresses to SMTP settings, propagates it through the API/UI, and implements deterministic selection logic in the SMTP messenger with a safe fallback to existing random load balancing when no match exists. No production-impacting bugs were found in the introduced changes.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 20 turns, 24 tool calls, 4m 10s
- Tokens: in
18.0K| cached266.4K| out14.4K(total298.7K) - Cost:
$0.2792
There was a problem hiding this comment.
Issues Found
Important (P2)
- [P2] Parse From header robustly before SMTP routing lookup (
internal/messenger/email/email.go:123-142)Push()extracts the address fromm.Fromusingstrings.Indexon</>, which fails for valid RFC 5322 formats such as quoted display names containing angle brackets or aFromaddress list; in those casesfromAddrcan be mis-extracted and domain routing silently falls back to random server selection, defeating the feature for those campaigns. Usingnet/mail.ParseAddress/ParseAddressList(and falling back only on parse error) would make routing match the configured addresses/domains reliably.
Minor (P3)
- [P3] Avoid empty/duplicate from_addresses keys skewing server selection (
internal/messenger/email/email.go:98-110)- In
New(),from_addressesentries are lowercased and trimmed but not filtered for empty strings or de-duplicated, so inputs like"@example.com, @example.com"(or whitespace-only entries from config) will add the same server pointer multiple times for a key and biasrand.Intn()selection probability for that domain/address. Skipping empty keys and de-duping per server/key keeps routing distribution predictable.
- In
Summary
Total issues: 0 critical, 1 important, 1 minor.
Overall Verdict
Status: Patch has blocking issues
Explanation: The core routing idea is sound, but the current From parsing is not robust enough for all valid From header formats and can cause routing to silently not apply. Additionally, duplicate/empty from_addresses entries can skew the random selection for a given domain/address.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 23 turns, 27 tool calls, 4m 34s
- Tokens: in
58.3K| cached446.8K| out15.4K(total520.5K) - Cost:
$0.3954
Currently, you can add multiple SMTP servers, but they will be round-robin'd. It is useful to be able to pick an SMTP server based on the `from` of the campaign. For example, Mailgun will DKIM sign based on the domain of the authenticating user, so if you send campains from more than one domain, you need to have different SMTP servers setup for each domain. This adds an optional list of domains that each SMTP server should be used with. If multiple servers specify the domain in question, we will pick one at random, just as the logic today does. If no domains are specified, or non match, then we fall back to the existing behavior of picking one at random. Signed-off-by: Phil Dibowitz <[email protected]>
There was a problem hiding this comment.
Issues Found
No issues found.
Summary
Total issues: 0 critical, 0 important, 0 minor.
Overall Verdict
Status: Patch is correct
Explanation: Reviewed the PR diff across the 6 changed files. The new from_addresses setting is plumbed through the UI, settings model, and SMTP emailer routing logic, with safe normalization and a fallback to existing random selection when no match exists. No discrete production bugs introduced by the diff were identified.
Review generated by Hodor (model: gpt-5.2)
Review Metrics — 14 turns, 13 tool calls, 2m 31s
- Tokens: in
34.6K| cached170.8K| out8.3K(total213.7K) - Cost:
$0.2071
|
@knadh - any thoughts on this? |
|
Haven't had a chance to review this yet. Will review in the coming couple of weeks. Thanks @jaymzh. |
2 participants
Currently, you can add multiple SMTP servers, but they will be
round-robin'd.
It is useful to be able to pick an SMTP server based on the
fromof the campaign.
For example, Mailgun will DKIM sign based on the domain of the
authenticating user, so if you send campains from more than one domain,
you need to have different SMTP servers setup for each domain.
This adds an optional list of domains that each SMTP server should be
used with.
If multiple servers specify the domain in question, we will pick one
at random, just as the logic today does.
If no domains are specified, or non match, then we fall back
to the existing behavior of picking one at random.
Signed-off-by: Phil Dibowitz [email protected]