chore: cleanup follow-ups from #908/#911 reviews

[jiashuoz]

Summary

Small mechanical cleanup PR closing out the leftover findings from the two Opus reviews of #908 and #911 (both merged). Cosmetic/hygiene only — no runtime behavior change, except item 6 which changes what's reachable via errors.Is/errors.As on one error chain (no call site or test currently depends on that, confirmed below).

Re-verified every item against current main (467295318) before touching anything, since the findings were written against abb1faff5 and earlier and line numbers were stale.

Items

1. ForgetSendingIdentityManaged dead code — kept, not removed.
Confirmed zero non-test callers repo-wide (both ErrIdentityNotOwned call sites were removed by #908). I evaluated full removal (method, Store interface entry, storeAdapter shim, both fakes) first, per the task's stated preference.

Its retain/delete DB-test coverage is not actually at risk either way: TestFinalizeSendingIdentityTombstoneRetainsLedgerForLiveOwnedDomain and TestFinalizeSendingIdentityTombstoneRemovesLedgerForGenuinelyDeletedDomain in internal/identity/sender_identity_lock_test.go already pin the identical retain/delete invariant for FinalizeSendingIdentityTombstone (added in a later batch). So "folding tests" turned out to be a non-issue.

What made removal genuinely more invasive than it looked: internal/senderidentity/worker.go's two former call sites carry rationale comments that cite this method's guard by name while explaining why they must never call it, and internal/senderidentity/worker_test.go has three purpose-built regression tests (TestReconcileWorker_NotOwnedRetainsLedgerForLiveDomain, TestSyncWorker_NotOwnedRetainsLedgerForLiveDomain, TestReapWorker_GenuineDeleteStillFinalizesTombstone) that poison a fake implementation of this exact method and assert zero calls to it — built specifically to catch a reintroduced call. Removing the method would mean rewriting all of that to argue from compile-time absence instead, which is a bigger, riskier change than a hygiene pass warrants, with real risk of silently weakening deliberately-designed defense-in-depth tests.

Chose the fallback: kept the method, rewrote its doc comment (internal/identity/store.go) to state plainly it has no callers, why it's kept anyway, and that any future caller must not be an ownership-failure branch. Added a matching one-line pointer on the Store interface entry in worker.go.

2. Stale "Forget" comments in worker.go — fixed 3 (plus one analogous instance not explicitly called out but the same category): the removed-call-site example in reconcileProviderIdentity's closing comment, "out before Forget" → "out before FinalizeSendingIdentityTombstone", and "same rule as the two branches above ... Forget/MarkApplied" (that comparison no longer holds — the two ownership branches have no follow-on store call anymore — so it's dropped and the actual calls named).

3. Doc drift in docs/design/sender-identity-mailfrom.md:

• accountIDFromCallerIdentity — already fixed, zero occurrences anywhere in the repo. No change needed.
• "BATCH A fix" → reworded to "the adoption work above."
• "An earlier draft of this amendment claimed..." narration → rewritten to state the current position directly (same substance).
• Step 5 was actively wrong ("audit and tag it explicitly") now that adoption tags matching identities automatically — reconciled steps 1–3 (marked optional, with a lead-in explaining why) and rewrote step 5 to point at the ALERT log instead of prescribing a manual tag.
• Added the ARN-partition derivation to the Verification section (it had test coverage the doc never mentioned).

4. Two (really three) ses_test.go stubs passing for the wrong reason — TestSESProvider_ProvisionRefusesForeignConfiguration's two subtests and TestSESProvider_StatusRefusesMismatchedSelector built DkimAttributes without Status: DkimStatusSuccess, so canAdoptIdentity's DKIM-SUCCESS gate could pass the test even with the named guard deleted. Added the missing Status. Verified by mutation: temporarily disabled the foreign-config guard → TestSESProvider_ProvisionRefusesForeignConfiguration failed as expected (both subtests); temporarily disabled the selector-match guard → TestSESProvider_StatusRefusesMismatchedSelector failed as expected. Both restored; full suite green.

5. TestReapWorker_GenuineDeleteStillFinalizesTombstone comment overstated what it proves — fixed comment only, no assertion changes, per the task's instruction.

6. Inconsistent error-wrapping in ses.go — adoptIdentity's account-id-unavailable wrap used %w: ...: %v (making the underlying error unreachable via errors.Is/errors.As) while classifyAdoptionError deliberately double-wraps a few lines below. Switched to %w: ...: %w for consistency. Confirmed no test or call site depends on the original being unreachable: TestSESProvider_AdoptionDegradesWhenAccountIDUnavailable and TestSESProvider_AccountIDResolutionCachesOnlySuccess only assert errors.Is(err, ErrIdentityNotOwned), never against the wrapped resolver error.

7. go mod tidy — one-line diff as predicted: github.com/aws/smithy-go moved from indirect to direct (it's imported directly by ses.go for smithy.APIError). go.sum unchanged.

8. gofmt — internal/senderidentity/fakestore_test.go had two doc comments (immediately preceding a func decl) writing SQL's empty-string literal as two adjacent straight quotes (''); Go's doc-comment reformatter collapses that specific adjacent pair into a single stray Unicode right-double-quote character. Switched to "", which the reformatter leaves alone. Confirmed the several other gofmt-dirty files in the repo (internal/agent/..., internal/limits/..., internal/oauth/..., internal/outbound/compose.go, internal/webhook/ssrf_test.go, internal/webhookpub/outbox_integration_test.go) are pre-existing on main, not touched here.

Verification

go build ./...                                                            # clean
go vet ./internal/senderidentity/... ./internal/identity/...              # clean
gofmt -l ./internal/senderidentity ./internal/identity                    # clean
go test ./internal/senderidentity/... -count=1                            # ok
go test ./internal/identity/ -run 'SendingIdentity|Tombstone|Forget|Managed' -count=1   # ok

Per instructions, did not run the full ./internal/identity/... suite (known-unreliable, DB-shared, baseline-required).

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com