diff --git a/internal/delivery/dispatcher.go b/internal/delivery/dispatcher.go index 22b7724..085e362 100644 --- a/internal/delivery/dispatcher.go +++ b/internal/delivery/dispatcher.go @@ -137,9 +137,10 @@ func NewDispatcher(cfg Config) *Dispatcher { // picks for (severity, presence), sends via the matching sink, and records // one nudge row per successful send. returns the dispatches (one per channel). // -// a Drop channel = no send, no record (the nudge was suppressed by routing, -// not by a failure — "a missed water nudge is noise"). a nil sink = channel -// not wired, skip silently. a send error stops the dispatch and returns what +// a Drop channel = no send (the nudge was suppressed by routing, not by a +// failure — "a missed water nudge is noise"), but it does leave a 'dropped' +// outbox row so the suppression is visible. a nil sink = channel not wired, +// skip silently. a send error stops the dispatch and returns what // got through — the daemon decides whether to retry. func (d *Dispatcher) DispatchNudge(ctx context.Context, pn PhrasedNudge, now time.Time) ([]Dispatch, error) { c := pn.Candidate @@ -148,6 +149,16 @@ func (d *Dispatcher) DispatchNudge(ctx context.Context, pn PhrasedNudge, now tim for i := 0; i < len(channels); i++ { ch := channels[i] if ch == ChannelDrop { + // the routing table suppressed this nudge on purpose (a care nudge + // while you're away is noise). that stays — but it must not be + // invisible, or "she dropped it" and "the rule never fired" look + // the same afterwards. no nudges row: that table feeds the + // ignored_rate signal, and a nudge nobody could see must not + // count as ignored. + id := d.beginOutbox(ctx, "nudge", c.Rule.Name, 0, ch, pn.Summary, now) + d.completeOutbox(ctx, id, store.DeliveryDropped, now) + log.Printf("dispatcher: dropped %s (sev%d, presence=%s) — routing table suppressed it", + c.Rule.Name, c.Severity, c.State.Presence) continue } s := Sendable{ @@ -396,6 +407,13 @@ func messageForChannel(s Sendable) string { if !isAway(s.Channel) { return s.Body } + return AwayMessage(s) +} + +// AwayMessage — the only text an off-box channel may ever carry. Exported so +// the away sinks share this one rule instead of each inventing a fallback: the +// summary if we have one, otherwise a fixed generic line. Never the body. +func AwayMessage(s Sendable) string { if s.Summary != "" { return s.Summary } diff --git a/internal/delivery/durability_test.go b/internal/delivery/durability_test.go index 0ef1fc8..4f015e7 100644 --- a/internal/delivery/durability_test.go +++ b/internal/delivery/durability_test.go @@ -37,10 +37,12 @@ func TestVoiceNoSessionFallthroughLeavesOutboxTrail(t *testing.T) { []string{"voice", "ntfy"}, []string{store.DeliveryFailed, store.DeliverySent}}, {"sev4 falls through to telegram", loop.Sev4, []string{"voice", "telegram"}, []string{store.DeliveryFailed, store.DeliverySent}}, - {"sev1 does not fall through", loop.Sev1, - []string{"voice"}, []string{store.DeliveryFailed}}, - {"sev2 does not fall through", loop.Sev2, - []string{"voice"}, []string{store.DeliveryFailed}}, + // care severities still don't reach an away channel; since #370 the + // drop itself is a visible row instead of nothing. + {"sev1 drops instead of falling through", loop.Sev1, + []string{"voice", "drop"}, []string{store.DeliveryFailed, store.DeliveryDropped}}, + {"sev2 drops instead of falling through", loop.Sev2, + []string{"voice", "drop"}, []string{store.DeliveryFailed, store.DeliveryDropped}}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { diff --git a/internal/delivery/ntfysink/ntfysink.go b/internal/delivery/ntfysink/ntfysink.go index dad2f9b..9e7fa3e 100644 --- a/internal/delivery/ntfysink/ntfysink.go +++ b/internal/delivery/ntfysink/ntfysink.go @@ -2,10 +2,10 @@ // // ntfy is the away-channel for sev3 (ops soft) nudges, sev4 (ops hard) // nudges when present (alongside voice), and reminders when away. the -// message body is the Sendable's Summary — the minimal-body rule from the +// message body is delivery.AwayMessage — the minimal-body rule from the // spec ("disk low on homesrv," not detail; no shoulder-surf exfil through -// the relay). voice gets Body; away channels get Summary, enforced at the -// sink so a phraser bug can't exfil. +// the relay). the dispatcher already strips detail off away sendables; the +// sink uses the same helper so it can't leak the body on its own either. // // ntfy runs locally (docker, 127.0.0.1:8085, deny-all auth). maven publishes // with a dedicated user (write-only to maven-* topics) — the credential is a @@ -69,18 +69,14 @@ func New(cfg Config) (*Sink, error) { }, nil } -// Send publishes one notification to ntfy. the body is the Sendable's Summary -// (minimal body); Title is "maven" (consistent sender identity on the lock -// screen — the content is in the body). Priority maps from severity/kind so +// Send publishes one notification to ntfy. the body is the minimal away +// message (never the full body); Title is "maven" (consistent sender identity +// on the lock screen — the content is in the body). Priority maps from severity/kind so // the phone client can ring differently for an alarm vs a soft ops nudge. func (s *Sink) Send(ctx context.Context, d delivery.Sendable) error { - body := d.Summary - if body == "" { - body = d.Body // terse full message beats no message - } - if body == "" { - return fmt.Errorf("ntfysink: empty message for %s", d.Channel) - } + // never fall back to d.Body: ntfy leaves the box, so an empty summary gets + // a generic line instead of the full detail. + body := delivery.AwayMessage(d) req, err := http.NewRequestWithContext(ctx, http.MethodPost, s.topicURL(), strings.NewReader(body)) if err != nil { diff --git a/internal/delivery/ntfysink/ntfysink_test.go b/internal/delivery/ntfysink/ntfysink_test.go index d106c42..c4edd1a 100644 --- a/internal/delivery/ntfysink/ntfysink_test.go +++ b/internal/delivery/ntfysink/ntfysink_test.go @@ -147,9 +147,9 @@ func TestSendBodyIsSummaryNotFullBody(t *testing.T) { } } -func TestSendFallsBackToBodyWhenSummaryEmpty(t *testing.T) { - // a terse full message is better than no message; the phraser should - // produce a summary for away-bound severities, but don't silently drop. +func TestSendNeverSendsTheBodyWhenSummaryEmpty(t *testing.T) { + // #368: this used to fall back to the full body. ntfy leaves the box, so + // an empty summary gets a fixed generic line plus the rule name instead. rs := newRecordingServer(t, 200, "") srv := httptest.NewServer(rs.handler()) defer srv.Close() @@ -160,12 +160,15 @@ func TestSendFallsBackToBodyWhenSummaryEmpty(t *testing.T) { t.Fatalf("Send: %v", err) } _, _, body, _, _, _ := rs.snapshot() - if body != s.Body { - t.Fatalf("fallback body: want %q, got %q", s.Body, body) + want := delivery.GenericAwayMessage + ": service_down" + if body != want { + t.Fatalf("body: want %q, got %q", want, body) } } -func TestSendRejectsEmptyMessage(t *testing.T) { +func TestSendNeverSendsAnEmptyMessage(t *testing.T) { + // with nothing at all to say we still send the generic line — an away + // channel can never carry detail, but it also never goes out blank. rs := newRecordingServer(t, 200, "") srv := httptest.NewServer(rs.handler()) defer srv.Close() @@ -173,9 +176,13 @@ func TestSendRejectsEmptyMessage(t *testing.T) { sink, _ := New(Config{BaseURL: srv.URL, Topic: "maven"}) s := nudgeSendable(loop.Sev3, "") s.Body = "" - err := sink.Send(context.Background(), s) - if err == nil { - t.Fatal("want error for empty message") + s.RuleName = "" + if err := sink.Send(context.Background(), s); err != nil { + t.Fatalf("Send: %v", err) + } + _, _, body, _, _, _ := rs.snapshot() + if body != delivery.GenericAwayMessage { + t.Fatalf("body: want %q, got %q", delivery.GenericAwayMessage, body) } } diff --git a/internal/delivery/routing_table_test.go b/internal/delivery/routing_table_test.go index f7c9129..a008880 100644 --- a/internal/delivery/routing_table_test.go +++ b/internal/delivery/routing_table_test.go @@ -208,10 +208,9 @@ func TestAwayChannelsGetMinimalBody(t *testing.T) { // 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 // rather than vanish. Today drop is a bare `continue`: no nudge row, no outbox -// attempt, no log — nothing an operator can see afterwards. +// attempt, no log — nothing an operator can see afterwards. now it leaves a +// 'dropped' outbox row. func TestCareAwayDropIsRecorded(t *testing.T) { - t.Skip("not implemented: dispatcher.go:149-151 skips a Drop channel with no record; there is no 'dropped' outcome in store/delivery.go:16-21") - ob := &fakeOutbox{} d := NewDispatcher(Config{Voice: &fakeSink{}, Nudges: &fakeNudgeRecorder{}, Outbox: ob}) diff --git a/internal/delivery/telegramsink/telegramsink.go b/internal/delivery/telegramsink/telegramsink.go index 6358992..f6db2d2 100644 --- a/internal/delivery/telegramsink/telegramsink.go +++ b/internal/delivery/telegramsink/telegramsink.go @@ -2,11 +2,12 @@ // // telegram is the away-channel for sev4 (ops hard) nudges — "disk-fire alarm // at 2am routes to telegram, repeat til ack." the message body is the -// Sendable's Summary — the minimal-body rule from the spec ("disk low on -// homesrv," not detail; no shoulder-surf exfil through the relay). voice gets -// Body; away channels get Summary, enforced at the sink so a phraser bug can't -// exfil. additionally, protect_content=true is passed on every send so the -// message can't be forwarded out of the chat — locks the minimal body further. +// delivery.AwayMessage — the minimal-body rule from the spec ("disk low on +// homesrv," not detail; no shoulder-surf exfil through the relay). the +// dispatcher already strips detail off away sendables; the sink uses the same +// helper so it can't leak the body on its own either. additionally, +// protect_content=true is passed on every send so the message can't be +// forwarded out of the chat — locks the minimal body further. // // telegram's bot API is region-restricted for this homesrv — direct egress to // api.telegram.org is unreliable. the spec's "away channels leave the box — @@ -140,18 +141,13 @@ type telegramResp struct { } // Send publishes one message to the configured telegram chat. the body is the -// Sendable's Summary (minimal body); empty Summary falls back to Body (terse -// full message beats no message). protect_content=true so a phraser bug (Body -// leaking detail through Summary) can't be forwarded onward by the user or a -// chat observer — locks the minimal-body rule at the channel's own last mile. +// minimal away message (never the full body). protect_content=true so even +// that can't be forwarded onward by the user or a chat observer — locks the +// minimal-body rule at the channel's own last mile. func (s *Sink) Send(ctx context.Context, d delivery.Sendable) error { - body := d.Summary - if body == "" { - body = d.Body - } - if body == "" { - return fmt.Errorf("telegramsink: empty message for %s", d.Channel) - } + // never fall back to d.Body: telegram leaves the box, so an empty summary + // gets a generic line instead of the full detail. + body := delivery.AwayMessage(d) payload := sendMessageReq{ ChatID: s.cfg.ChatID, diff --git a/internal/delivery/telegramsink/telegramsink_test.go b/internal/delivery/telegramsink/telegramsink_test.go index 5c378f4..60b66ff 100644 --- a/internal/delivery/telegramsink/telegramsink_test.go +++ b/internal/delivery/telegramsink/telegramsink_test.go @@ -173,9 +173,9 @@ func TestSendBodyIsSummaryNotFullBody(t *testing.T) { } } -func TestSendFallsBackToBodyWhenSummaryEmpty(t *testing.T) { - // terse full message beats none; the phraser should produce a summary for - // away-bound severities, but don't silently drop. +func TestSendNeverSendsTheBodyWhenSummaryEmpty(t *testing.T) { + // #368: this used to fall back to the full body. telegram leaves the box, + // so an empty summary gets a fixed generic line plus the rule name. rs := newRecordingServer(t, 200, "") srv := httptest.NewServer(rs.handler()) defer srv.Close() @@ -188,12 +188,14 @@ func TestSendFallsBackToBodyWhenSummaryEmpty(t *testing.T) { _, _, body, _, _ := rs.snapshot() var req sendMessageReq _ = json.Unmarshal([]byte(body), &req) - if req.Text != s.Body { - t.Fatalf("fallback text: want %q, got %q", s.Body, req.Text) + want := delivery.GenericAwayMessage + ": service_down" + if req.Text != want { + t.Fatalf("text: want %q, got %q", want, req.Text) } } -func TestSendRejectsEmptyMessage(t *testing.T) { +func TestSendNeverSendsAnEmptyMessage(t *testing.T) { + // with nothing at all to say we still send the generic line. rs := newRecordingServer(t, 200, "") srv := httptest.NewServer(rs.handler()) defer srv.Close() @@ -201,9 +203,15 @@ func TestSendRejectsEmptyMessage(t *testing.T) { sink, _ := New(sinkCfg(srv.URL)) s := nudgeSendable(loop.Sev4, "") s.Body = "" - err := sink.Send(context.Background(), s) - if err == nil { - t.Fatal("want error for empty message") + s.RuleName = "" + if err := sink.Send(context.Background(), s); err != nil { + t.Fatalf("Send: %v", err) + } + _, _, body, _, _ := rs.snapshot() + var req sendMessageReq + _ = json.Unmarshal([]byte(body), &req) + if req.Text != delivery.GenericAwayMessage { + t.Fatalf("text: want %q, got %q", delivery.GenericAwayMessage, req.Text) } } diff --git a/internal/store/delivery.go b/internal/store/delivery.go index 8c37b14..b0b46df 100644 --- a/internal/store/delivery.go +++ b/internal/store/delivery.go @@ -13,11 +13,15 @@ import ( // unknown = a pending row found stale at startup: the process that started it // is gone, and the send may or may not have reached the external channel. // Never auto-resolved into sent or failed — that would be guessing. +// dropped = the routing table deliberately suppressed this one (a care nudge +// while you're away). Nothing was sent and nothing went wrong; the row exists +// so "she dropped it" and "the rule never fired" don't look the same later. const ( DeliveryPending = "pending" DeliverySent = "sent" DeliveryFailed = "failed" DeliveryUnknown = "unknown" + DeliveryDropped = "dropped" ) // BeginDeliveryAttempt durably records intent to send BEFORE the external @@ -43,10 +47,11 @@ func (s *Store) BeginDeliveryAttempt(ctx context.Context, kind, rule string, rem } // CompleteDeliveryAttempt records the sink's outcome for a prior -// BeginDeliveryAttempt. status is "sent" or "failed" — never "pending" or -// "unknown" (those are set only by Begin and reconciliation respectively). +// BeginDeliveryAttempt. status is "sent", "failed" or "dropped" — never +// "pending" or "unknown" (those are set only by Begin and reconciliation +// respectively). func (s *Store) CompleteDeliveryAttempt(ctx context.Context, id int64, status string, now time.Time) error { - if status != DeliverySent && status != DeliveryFailed { + if status != DeliverySent && status != DeliveryFailed && status != DeliveryDropped { return fmt.Errorf("store: invalid delivery completion status %q", status) } _, err := s.db.ExecContext(ctx, diff --git a/internal/store/delivery_test.go b/internal/store/delivery_test.go new file mode 100644 index 0000000..4a7e23d --- /dev/null +++ b/internal/store/delivery_test.go @@ -0,0 +1,34 @@ +package store + +import ( + "context" + "testing" + "time" +) + +// TestDroppedDeliveryAttemptRoundTrips — Vikunja #370. A suppressed nudge is +// recorded as 'dropped'. The status column has a CHECK constraint, so this +// only works if migration #12 widened it; a fake outbox in a unit test would +// not catch that. +func TestDroppedDeliveryAttemptRoundTrips(t *testing.T) { + s := newTestStore(t) + ctx := context.Background() + now := time.Now() + + id, err := s.BeginDeliveryAttempt(ctx, "nudge", "water", 0, "drop", "abc123", now) + if err != nil { + t.Fatalf("BeginDeliveryAttempt: %v", err) + } + if err := s.CompleteDeliveryAttempt(ctx, id, DeliveryDropped, now); err != nil { + t.Fatalf("CompleteDeliveryAttempt: %v", err) + } + + var status string + err = s.db.QueryRowContext(ctx, `SELECT status FROM delivery_attempts WHERE id = ?`, id).Scan(&status) + if err != nil { + t.Fatalf("read back: %v", err) + } + if status != DeliveryDropped { + t.Fatalf("status: want %q, got %q", DeliveryDropped, status) + } +} diff --git a/internal/store/migrations.go b/internal/store/migrations.go index b9b7ff0..8bc86eb 100644 --- a/internal/store/migrations.go +++ b/internal/store/migrations.go @@ -88,6 +88,30 @@ ALTER TABLE reminders ADD COLUMN next_fire_ts INTEGER;`, // #2 key TEXT PRIMARY KEY, value TEXT NOT NULL );`, // #11 — small key/value table for facts about the DB itself; first key is embedder_id (Vikunja #378) + + // #12 — a suppressed nudge gets a 'dropped' row (Vikunja #370). sqlite + // can't widen a CHECK constraint in place, so the table is rebuilt; the + // index goes with the old table and is recreated. The columns are listed + // out rather than `SELECT *` — copying by position would silently shuffle + // every row if the old table's column order ever differed from this one. + `CREATE TABLE delivery_attempts_v12 ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + kind TEXT NOT NULL CHECK (kind IN ('nudge','reminder')), + rule TEXT NOT NULL DEFAULT '', + reminder_id INTEGER NOT NULL DEFAULT 0, + channel TEXT NOT NULL, + body_hash TEXT NOT NULL, + status TEXT NOT NULL DEFAULT 'pending' CHECK (status IN ('pending','sent','failed','unknown','dropped')), + created_ts INTEGER NOT NULL, + completed_ts INTEGER + ); + INSERT INTO delivery_attempts_v12 + (id, kind, rule, reminder_id, channel, body_hash, status, created_ts, completed_ts) + SELECT id, kind, rule, reminder_id, channel, body_hash, status, created_ts, completed_ts + FROM delivery_attempts; + DROP TABLE delivery_attempts; + ALTER TABLE delivery_attempts_v12 RENAME TO delivery_attempts; + CREATE INDEX IF NOT EXISTS idx_delivery_attempts_status ON delivery_attempts (status);`, } // migrate applies every migration with a number greater than the DB's current