From 210d286114fafd713c59f82635f03bbf8d74de12 Mon Sep 17 00:00:00 2001 From: Manas Srivastava Date: Sat, 30 May 2026 22:16:43 +0530 Subject: [PATCH] fix(expiry_reminder_email): default empty hours_remaining to "1" so subject and body agree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EMAIL-BUGBASH-WORKER-2026-05-19 P3-1: when hours_remaining was an empty string in the params map, the subject defaulted to "expires in 1h" but the body rendered "expires in hours" — empty number, plural-true (since `"" != "1"`). Recipients saw a contradictory subject vs body and a grammatically broken " hours" with no count. Default hours_remaining to "1" BEFORE deriving Plural, so the body matches the subject's existing "1h" fallback (final-reminder cadence is the safest default for a missing field — better to nudge as last-chance than to render mysteriously empty copy). Coverage: new TestRenderAnonExpiryEmail_EmptyHoursConsistency pins subject-vs-body parity across all rendered surfaces (subject, html H2, html "Time left" row, text body) and uses a digit-precedes guard (`hasDigitBefore`) that fails if any future surface ever renders " hours" with no preceding number. Registry-style invariant per rule 18 — catches the bug class, not just this exact site. Symptom: " hours" / "1h vs (blank) hours" subject-body mismatch in anon expiry warnings Enumeration: rg -nE 'hours_remaining|Plural' internal/jobs/expir* Sites found: 1 (renderAnonExpiryEmail in expiry_reminder_email.go) Sites touched: 1 Coverage test: TestRenderAnonExpiryEmail_EmptyHoursConsistency (digit-before-hours invariant) Live verified: unit (no live email surface — Brevo sender still unvalidated per CLAUDE.md P0) Co-Authored-By: Claude Opus 4.7 (1M context) --- .../jobs/expire_unexported_coverage_test.go | 69 +++++++++++++++++++ internal/jobs/expiry_reminder_email.go | 23 +++++-- 2 files changed, 87 insertions(+), 5 deletions(-) diff --git a/internal/jobs/expire_unexported_coverage_test.go b/internal/jobs/expire_unexported_coverage_test.go index 84e29cf..af15830 100644 --- a/internal/jobs/expire_unexported_coverage_test.go +++ b/internal/jobs/expire_unexported_coverage_test.go @@ -185,6 +185,75 @@ func TestRenderAnonExpiryEmail_MissingParamsRenderEmpty(t *testing.T) { } } +// TestRenderAnonExpiryEmail_EmptyHoursConsistency pins the +// subject-vs-body parity for an empty hours_remaining param. Prior to +// the EMAIL-BUGBASH-WORKER-2026-05-19 P3-1 fix, the subject defaulted +// to "1h" while the body rendered " hours" (empty value, plural form +// = true). The body and subject MUST agree on the same fallback so +// recipients never see " hours" with no number or a singular/plural +// mismatch. This is a registry-style guard: every rendered surface +// (subject, html body H2, html body "Time left" row, text body) must +// include a non-empty hour count. +func TestRenderAnonExpiryEmail_EmptyHoursConsistency(t *testing.T) { + subj, html, text := renderAnonExpiryEmail(map[string]string{ + "resource_type": "postgres", + "reminder_index": "3", + "token_prefix": "tok-abcd", + "expires_at": "2026-05-22T00:00:00Z", + "upgrade_url": "https://x/upgrade", + "resource_url": "https://x/res", + // hours_remaining intentionally omitted. + }) + + // Subject MUST end with "1h" (the subject's existing fallback). + if !strings.HasSuffix(subj, "1h") { + t.Errorf("subject must end with '1h' on empty hours_remaining, got %q", subj) + } + + // Every rendered surface must contain "1 hour" (singular, matching + // the subject's "1h") — never " hour" or " hours" (empty number). + // The H2, "Time left" row, and text body all use the same template. + surfaces := map[string]string{"html": html, "text": text} + for name, body := range surfaces { + if !strings.Contains(body, "1 hour") { + t.Errorf("%s body must include '1 hour' (singular) on empty hours_remaining, got: %s", name, body) + } + if strings.Contains(body, "1 hours") { + t.Errorf("%s body must not include '1 hours' (incorrect plural), got: %s", name, body) + } + // Never an empty-number rendering. Catches re-introducing the bug. + if strings.Contains(body, " hour ") && !strings.Contains(body, "1 hour ") { + t.Errorf("%s body has bare ' hour ' with no preceding number, got: %s", name, body) + } + if strings.Contains(body, " hours") && !strings.Contains(body, "1 hours") && + !hasDigitBefore(body, " hours") { + t.Errorf("%s body has ' hours' with no preceding digit, got: %s", name, body) + } + } +} + +// hasDigitBefore reports whether every occurrence of `needle` in `s` +// is preceded by an ASCII digit. Helper for the empty-number guard: +// "12 hours" passes (digit before), " hours" fails (space before). +func hasDigitBefore(s, needle string) bool { + i := 0 + for { + idx := strings.Index(s[i:], needle) + if idx == -1 { + return true + } + pos := i + idx + if pos == 0 { + return false + } + c := s[pos-1] + if c < '0' || c > '9' { + return false + } + i = pos + len(needle) + } +} + // TestAnonExpirySubject_AllBranches walks every reminder_index prefix // and the default fallback (any other value). func TestAnonExpirySubject_AllBranches(t *testing.T) { diff --git a/internal/jobs/expiry_reminder_email.go b/internal/jobs/expiry_reminder_email.go index b5716f3..91314d3 100644 --- a/internal/jobs/expiry_reminder_email.go +++ b/internal/jobs/expiry_reminder_email.go @@ -170,19 +170,32 @@ type anonExpiryView struct { // template.Must). On the off-chance a future template change breaks // parsing, the panic surfaces in CI rather than at runtime. // -// Missing param keys render as empty strings. The caller (forwarder) -// is responsible for not invoking this for kinds other than -// anon.expiry_warning. +// Missing param keys render as empty strings, EXCEPT hours_remaining +// which defaults to "1" so the subject ("expires in 1h") and the body +// ("expires in 1 hour") stay consistent. Without this defaulting, an +// empty hours_remaining would render "expires in hours" (note the +// double space + plural "s") — a P3 bug from EMAIL-BUGBASH-WORKER +// 2026-05-19. The caller (forwarder) is responsible for not invoking +// this for kinds other than anon.expiry_warning. func renderAnonExpiryEmail(params map[string]string) (subject, html, text string) { + // Default the hours_remaining BEFORE deciding plurality so the body + // matches the subject's "1h" fallback path (see anonExpirySubject). + // "1" is the conservative default for a final-reminder cadence — if + // the forwarder ever loses the field, the email still reads as a + // last-chance nudge rather than mysteriously empty. + hoursRemaining := params["hours_remaining"] + if hoursRemaining == "" { + hoursRemaining = "1" + } view := anonExpiryView{ ResourceType: params["resource_type"], - HoursRemaining: params["hours_remaining"], + HoursRemaining: hoursRemaining, ExpiresAt: params["expires_at"], ReminderIndex: params["reminder_index"], TokenPrefix: params["token_prefix"], UpgradeURL: params["upgrade_url"], ResourceURL: params["resource_url"], - Plural: params["hours_remaining"] != "1", + Plural: hoursRemaining != "1", } subject = anonExpirySubject(view.ReminderIndex, view.ResourceType, view.HoursRemaining)