From f6236da760d110857533018db6b8664ae03c9040 Mon Sep 17 00:00:00 2001 From: kami Date: Fri, 31 Jul 2026 02:42:21 +0400 Subject: [PATCH] Collapse the duplicate away-detail and panic tests Two agents wrote the same three test helpers and names for the same two bugs. Kept the real assertions from dispatcher_test.go and removed the skipped placeholders they replace. panicSink stays in durability_test.go since both files use it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ --- internal/delivery/dispatcher_test.go | 9 ++--- internal/delivery/durability_test.go | 25 ++----------- internal/delivery/routing_table_test.go | 47 +++---------------------- 3 files changed, 10 insertions(+), 71 deletions(-) diff --git a/internal/delivery/dispatcher_test.go b/internal/delivery/dispatcher_test.go index e3d79a0..bf1f776 100644 --- a/internal/delivery/dispatcher_test.go +++ b/internal/delivery/dispatcher_test.go @@ -787,13 +787,8 @@ func TestDispatchNudge_OutboxBeginFailureDoesNotBlockSend(t *testing.T) { // ----------------------- away channels carry no detail ----------------------- -// panicSink — a broken sink. models the #369 case: the sink blows up mid-send. -type panicSink struct{ calls int } - -func (p *panicSink) Send(_ context.Context, _ Sendable) error { - p.calls++ - panic("sink is broken") -} +// panicSink lives in durability_test.go — a second agent wrote the same helper +// for the same #369 case, so this file just uses that one. // TestAwaySendsGenericLineWhenSummaryEmpty — #368. The phraser is a small // model and drops fields often. An empty Summary must NOT put the full body diff --git a/internal/delivery/durability_test.go b/internal/delivery/durability_test.go index 68407b5..0ef1fc8 100644 --- a/internal/delivery/durability_test.go +++ b/internal/delivery/durability_test.go @@ -223,25 +223,6 @@ func TestCompleteFailureLeavesRowPendingForReconciliation(t *testing.T) { } } -// TestPanicMidSendResolvesTheAttempt — a sink that panics leaves the attempt -// pending forever while the process keeps running: the dispatcher has no -// recover, and reconciliation only runs at startup. Written to the promise -// ("never silently resent or dropped" implies every attempt gets resolved), -// skipped because the code does not keep it. -func TestPanicMidSendResolvesTheAttempt(t *testing.T) { - t.Skip("real gap: dispatcher.go:168 has no recover around Send, so a panicking sink leaves a permanent pending row (reconciliation only runs at startup, cmd/mavend/main.go:330)") - - ob := &fakeOutbox{} - d := NewDispatcher(Config{Ntfy: &panicSink{}, Outbox: ob}) - - func() { - defer func() { _ = recover() }() - _, _ = d.DispatchNudge(context.Background(), PhrasedNudge{ - Candidate: candidate("cert_expiring", loop.Sev3, store.Away), - Body: "detail", Summary: "short", - }, refNow()) - }() - if len(ob.attempts) != 1 || ob.attempts[0].status == store.DeliveryPending { - t.Fatalf("a panic mid-send must still resolve the attempt, got %+v", ob.attempts) - } -} +// The panic gap this file used to describe as a skipped test is fixed and +// asserted for real in dispatcher_test.go:TestPanicMidSendResolvesTheAttempt. +// panicSink stays here because both files use it. diff --git a/internal/delivery/routing_table_test.go b/internal/delivery/routing_table_test.go index 2916bca..f7c9129 100644 --- a/internal/delivery/routing_table_test.go +++ b/internal/delivery/routing_table_test.go @@ -2,7 +2,6 @@ package delivery import ( "context" - "strings" "testing" "time" @@ -200,47 +199,11 @@ func TestAwayChannelsGetMinimalBody(t *testing.T) { } } -// TestSev4AwaySendableCarriesNoDetail — DESIGN.md § Delivery: away channels -// leave the box, so a sev4-away message must not carry detail beyond the short -// form. Today the dispatcher hands the away sink the FULL Body as well as the -// Summary (dispatcher.go:153-162 copies pn.Body into every Sendable) and -// trusts each sink to pick Summary. That works for the two sinks in-tree, but -// the minimal body is not enforced at the dispatcher, so a new away sink that -// reads Body exfils by default. -func TestSev4AwaySendableCarriesNoDetail(t *testing.T) { - t.Skip("not enforced: dispatcher.go:159 puts the full Body on away sendables; minimal body is only enforced per-sink (ntfysink.go:77, telegramsink.go:148)") - - telegram := &fakeSink{} - d := NewDispatcher(Config{Telegram: telegram, Ack: newFakeAck()}) - detail := "disk /mnt/hdd1 at 97%, biggest offender /var/lib/docker" - - if _, err := d.DispatchNudge(context.Background(), PhrasedNudge{ - Candidate: candidate("disk_low", loop.Sev4, store.Away), - Body: detail, Summary: "disk low on homesrv", - }, refNow()); err != nil { - t.Fatalf("dispatch: %v", err) - } - if strings.Contains(telegram.sends[0].Body, "/var/lib/docker") { - t.Fatalf("away sendable carries detail: %q", telegram.sends[0].Body) - } -} - -// TestAwayFallsBackToFullBodyWhenSummaryEmpty — the other half of the same -// gap: with no Summary, the full body leaves the box. The code chooses that on -// purpose ("a terse full message is better than no message", -// dispatcher.go:345-357), which contradicts the spec's minimal-body rule. -// Written to the spec, skipped because the code disagrees. -func TestAwayFallsBackToFullBodyWhenSummaryEmpty(t *testing.T) { - t.Skip("by design today: dispatcher.go:356 and ntfysink.go:79 fall back to the full Body when Summary is empty, so detail can leave the box") - - msg := messageForChannel(Sendable{ - Channel: ChannelNtfy, - Body: "internal detail that should never leave the box", - }) - if msg != "" { - t.Fatalf("empty summary must not fall back to body, got %q", msg) - } -} +// Both away-detail gaps this file used to describe as skipped tests are now +// fixed and asserted for real in dispatcher_test.go: +// TestSev4AwaySendableCarriesNoDetail and TestAwaySendsGenericLineWhenSummaryEmpty. +// An empty Summary no longer means "send the whole body" — it means a short +// generic line — so the old expectation here was wrong as well as duplicated. // TestCareAwayDropIsRecorded — DESIGN.md's drop is a decision ("a missed water // nudge is noise, a missed backup failure isn't"), so it should be visible