fix: preserve the underlying cause in LookupError

[NeoCN]

ParseSMTPError replaced the error it was handed with one of a fixed set of canned messages and dropped the original. Callers could only recover the reason by substring-matching Details, and could not reach the underlying *net.DNSError, *net.OpError or *textproto.Error at all.

Wrapping instead of replacing

LookupError now records the error it was derived from and exposes it via Unwrap, so errors.Is and errors.As work:

var dnsErr *net.DNSError
if errors.As(err, &dnsErr) && dnsErr.IsNotFound {
    // the domain does not resolve, as opposed to the mail server refusing us
}

The field is unexported and not serialised, so the JSON and XML shapes are unchanged.

Fixing a nil that callers cannot handle

ParseSMTPError returned a nil *LookupError whenever the status line did not itself indicate a failure. It is only ever reached with a non-nil error, so that discarded a real failure — and because the result is returned through an error interface:

return &ret, ParseSMTPError(err)

callers were handed a non-nil interface wrapping a nil pointer. LookupError.Error dereferences its receiver, so calling it on that panics. cmd/apiserver does exactly that, on two lines.

An error that cannot be classified is now reported verbatim, so a non-nil input always yields a non-nil result. The classification logic itself is unchanged, just moved into an internal function behind a thin exported wrapper.

Compatibility

This is a breaking change for downstream code, in three ways. None of them affect the JSON or XML output.

ParseSMTPError never returns nil for a non-nil input. A caller writing if e := ParseSMTPError(err); e != nil { ... } now enters that branch for a reply whose status parses to 400 or below, where previously it did not, and e.Message there is the raw error string rather than one of the exported Err* constants. Code that assumes Message is always one of those constants will fall through to whatever it does by default. This repo's own use of that pattern, in CheckSMTP's catch-all switch, lands on an empty default: and is unaffected — but external code need not be so lucky.

LookupError gained an unexported field. An external unkeyed literal — emailverifier.LookupError{"msg", "details"} — no longer compiles, and cannot be fixed by adding a third value, since Go forbids supplying a value for another package's unexported field. go vet already discourages unkeyed literals for other packages' structs, but the break is real. The constructor in this package had to change for the same reason.

Whole-struct comparison no longer matches. assert.Equal and reflect.DeepEqual inspect unexported fields, and the cause is populated on every return path, so comparing a result against a hand-built &LookupError{Message: ..., Details: ...} now fails. Three tests in this PR had to change for exactly this reason, and any downstream test written the same way will too. Comparing Message and Details individually is the fix.

Tagged **Breaking** in the changelog, alongside #198.

Tests

TestParseError_UnclassifiedIsReportedVerbatim covers the new branch across 200 OK, 300 Redirect, 399 and 400, asserting the verbatim message, that Error() does not panic, and that the cause survives. That last assertion matters: with only the previous single-value test, removing .withCause(err) from this branch passed the entire suite.

Also covers a nil input, errors.As reaching the cause on the classified branch, and Unwrap on a nil receiver. Each was checked by reverting the corresponding production change and confirming the test fails.

TestParseError_Code400_Nil asserted the old nil return and is gone, replaced by the table above. Three tests that compared a whole LookupError now assert on Message and Details, matching the rest of the file.

Scope

Groundwork, not a behaviour change to classification: every reply that produced a given Message before still produces it.

Distinguishing rejections that concern the sender from those that concern the recipient — what #152, #126 and #72 need — means changing which Message a reply maps to, and the switch in CheckSMTP cases on exactly three of those values, so any remapping silently reroutes a reply to the empty default and flips CatchAll. Doing that safely needs deterministic tests for the 4xx, greylisting and blocked paths, which the live-network suite cannot produce. Left for a later change.

🤖 Generated with Claude Code