[daily-grumpy-reviewer] Daily Grumpy Review - 2026-05-29

[github-actions]

Five grumpies found today: four are the same stubborn recurring offenders that have been here for multiple consecutive runs, plus one new pattern. The codebase remains genuinely clean otherwise — but these four items are starting to look like deliberate negligence. Patterns are not improving on the open issues.

📋 Full Grumpy Report

Review Overview

Metric	Value
Files Reviewed	~15 (handler/, auth/, smtp/)
Grumpies Found	5
Recurring Patterns	4 (carried over from previous runs)
New Patterns	1
Review Date	2026-05-29

---

🐹 Go Backend

Error Handling (1 issue)

token-expiry-check-inconsistency — 7th consecutive run. Unacceptable.

File	Line	Issue	Suggestion
handler/password_reset.go	~144	ResetPassword checks ErrNotFound || ErrExpiredToken || ErrInvalidToken — three different sentinels for "token not found"	Pick one sentinel. PasswordResetStore.FindPasswordResetToken documents ErrNotFound for missing records; remove the ErrExpiredToken and ErrInvalidToken branches from the handler
handler/email_verification.go	~88	ConsumeEmailVerification only checks ErrNotFound	Consistent with the interface contract — this is the correct pattern
auth/types.go	—	PasswordResetStore contract says "Returns ErrNotFound if no matching record exists" but the handler defensively checks two additional sentinels anyway	The handler author doesn't trust the interface contract. Either the contract is wrong or the handler is wrong. Pick one and enforce it

The asymmetry between how ResetPassword and VerifyEmail handle missing tokens signals that the store contract isn't trusted. A callee contract only has value if the callers actually rely on it.

---

HTTP Handlers (3 issues)

oidc-callback-dead-verifier-fallback — 4th consecutive run.

File	Line	Issue	Suggestion
handler/oidc.go	~148–151	Callback checks if verifier == nil and re-creates it at request time	Validate() unconditionally sets IDTokenVerifier — this nil branch is unreachable. Delete it.

// Dead code — verifier cannot be nil here if Validate() was called.
verifier := h.IDTokenVerifier
if verifier == nil {
    verifier = h.Provider.Verifier(&oidc.Config{ClientID: h.OAuthConfig.ClientID})
}

If you don't trust Validate(), stop writing Validate(). If you do trust it, delete the dead branch.

---

oidc-oauth2-email-as-name-fallback — 4th consecutive run.

File	Line	Issue	Suggestion
handler/oidc.go	~174	if claims.Name == "" { claims.Name = claims.Email }	Use empty string, consistent with MagicLinkHandler (resolved 2026-05-25)
handler/oauth2.go	~167	if info.Name == "" { info.Name = info.Email }	Same fix

The MagicLinkHandler was fixed precisely because email-as-display-name is bad UX. Why are OIDC and OAuth2 still doing it?

---

apikey-delete-runtime-nil-check-redundant-after-validate — 3rd consecutive run.

File	Line	Issue	Suggestion
handler/apikey.go	~119–122	Delete() checks if h.URLParamFunc == nil at request time	Validate() already returns an error for nil URLParamFunc. This check is unreachable in a correctly initialised handler. Remove it.

This is the same class of problem as the OIDC verifier fallback. You've established a Validate() contract. Trust it.

---

Code Structure (1 issue — NEW)

handlers-missing-injectable-logger — first occurrence.

OIDCHandler and OAuth2Handler both have a Logger *slog.Logger field and a log() helper method that falls back to slog.Default(). Every other handler (APIKeyHandler, PasswordResetHandler, AuthHandler, TOTPHandler, PasskeyHandler, EmailVerificationHandler, MagicLinkHandler, SessionHandler) calls slog.ErrorContext() / slog.WarnContext() directly — no injectable logger, no way to route logs per handler in tests or production.

// OIDCHandler — has injectable logger ✅
func (h *OIDCHandler) log() *slog.Logger {
    if h.Logger != nil { return h.Logger }
    return slog.Default()
}

// APIKeyHandler — uses slog package directly ❌
slog.ErrorContext(r.Context(), "failed to list API keys", ...)

This is a consistency and testability problem. Either the Logger field pattern is the team standard (and needs to be applied everywhere), or it's unnecessary (and should be removed from OIDC/OAuth2). Pick a lane.

---

📈 Pattern Analysis

Recurring Themes (seen in previous runs)

Pattern	Occurrences Today	Total Seen	First Observed
token-expiry-check-inconsistency	1	7	2026-05-23
oidc-oauth2-email-as-name-fallback	2 files	4	2026-05-26
oidc-callback-dead-verifier-fallback	1	4	2026-05-26
apikey-delete-runtime-nil-check-redundant-after-validate	1	3	2026-05-27

New Patterns (first time seen)

• handlers-missing-injectable-logger: OIDCHandler and OAuth2Handler have Logger *slog.Logger + log() helper; all other 8 handlers call slog package functions directly. Either extend the pattern to all handlers or document why OIDC/OAuth2 are special.

---

✅ Positive Highlights

• ✅ All handlers now implement Validate() — the startup-time misconfiguration safety net is comprehensive
• ✅ Every 500 error path calls slog.ErrorContext before writeError — no silent internal errors
• ✅ auth/http.go encodes JSON errors correctly without silently discarding encode failures (fixed 2026-05-27)
• ✅ findOrCreateUser is correctly shared between OIDC and OAuth2 handlers via oauth2_common.go
• ✅ Error wrapping with descriptive fmt.Errorf prefixes is consistent throughout

---

💡 Recommendations

For immediate attention (these are in their 3rd–7th run):

1. handler/password_reset.go ~144: Remove the ErrExpiredToken and ErrInvalidToken branches. The interface contract says ErrNotFound. Trust it.
2. handler/oidc.go ~148–151: Delete the if verifier == nil fallback block. Validate() sets it unconditionally.
3. handler/oidc.go ~174 and handler/oauth2.go ~167: Replace claims.Name = claims.Email / info.Name = info.Email with empty string, matching the magiclink precedent.
4. handler/apikey.go ~119–122: Remove the URLParamFunc == nil guard in Delete(). Validate() already catches this.

Longer-term improvements:

1. Decide on the injectable logger pattern: either add Logger *slog.Logger + log() to all handlers, or remove it from OIDC/OAuth2 and document that slog.Default() is the team's global logger approach.

---

Daily grumpy review for amalgamated-tools/goauth · Run

Generated by Daily Grumpy Reviewer · ● 1.5M · ◷