From e9ff2c4912c3083d332d572fbe3098ebd0d373a1 Mon Sep 17 00:00:00 2001 From: kami Date: Fri, 31 Jul 2026 14:23:54 +0400 Subject: [PATCH 1/3] Never send the full nudge body off-box (#368) The away sinks fell back to the whole Body when Summary was empty. ntfy and telegram leave the box, and the 0.8B phraser drops fields regularly, so that fallback could push full detail off the machine. The dispatcher already strips detail from away sendables. This exports that one rule as delivery.AwayMessage and has both sinks use it, so a sink can't leak the body on its own either: empty Summary means a generic line plus the rule name, never the body. The two sink tests named TestSendFallsBackToBodyWhenSummaryEmpty asserted the old, wrong behaviour, so they are rewritten to assert the generic line. TestSendRejectsEmptyMessage is likewise replaced: an away message can no longer be empty, so the sink has nothing left to reject. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ --- internal/delivery/dispatcher.go | 7 +++++ internal/delivery/ntfysink/ntfysink.go | 22 ++++++--------- internal/delivery/ntfysink/ntfysink_test.go | 25 +++++++++++------ .../delivery/telegramsink/telegramsink.go | 28 ++++++++----------- .../telegramsink/telegramsink_test.go | 26 +++++++++++------ 5 files changed, 61 insertions(+), 47 deletions(-) diff --git a/internal/delivery/dispatcher.go b/internal/delivery/dispatcher.go index 22b7724..d534eb4 100644 --- a/internal/delivery/dispatcher.go +++ b/internal/delivery/dispatcher.go @@ -396,6 +396,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/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/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) } } From 0272dc9d891e8cf5415e747449433de9b1b8bea9 Mon Sep 17 00:00:00 2001 From: kami Date: Fri, 31 Jul 2026 14:27:47 +0400 Subject: [PATCH 2/3] Record a suppressed care nudge instead of dropping it silently (#370) Dropping a sev1-2 care nudge while you're away is right and still happens. But it was a bare `continue`: no row, no log, so "she dropped it", "the gate suppressed it" and "the rule never fired" all looked identical afterwards. Adds a 'dropped' delivery status (migration #12 widens the CHECK constraint; sqlite can't do that in place, so the table is rebuilt) and records the drop as one delivery_attempts row plus a log line. No nudges row for a drop: that table feeds the ignored_rate signal, and a nudge nobody could see must not count as ignored. TestVoiceNoSessionFallthroughLeavesOutboxTrail expected exactly one row for sev1-2 when voice had no session. It now expects the voice failure plus the drop, which is the point of the change. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ --- internal/delivery/dispatcher.go | 17 ++++++++++--- internal/delivery/durability_test.go | 10 +++++--- internal/delivery/routing_table_test.go | 5 ++-- internal/store/delivery.go | 11 +++++--- internal/store/delivery_test.go | 34 +++++++++++++++++++++++++ internal/store/migrations.go | 19 ++++++++++++++ 6 files changed, 83 insertions(+), 13 deletions(-) create mode 100644 internal/store/delivery_test.go diff --git a/internal/delivery/dispatcher.go b/internal/delivery/dispatcher.go index d534eb4..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{ 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/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/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..e67bd43 100644 --- a/internal/store/migrations.go +++ b/internal/store/migrations.go @@ -88,6 +88,25 @@ 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. + `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 SELECT * 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 From 59cec63da116f785619b302e03e29193d752ca2d Mon Sep 17 00:00:00 2001 From: kami Date: Fri, 31 Jul 2026 14:30:54 +0400 Subject: [PATCH 3/3] List the columns in the table rebuild The migration copied rows with SELECT *, which matches columns by position. It is correct today, but if the old table's order ever differed it would shuffle every row instead of failing. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ --- internal/store/migrations.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/internal/store/migrations.go b/internal/store/migrations.go index e67bd43..8bc86eb 100644 --- a/internal/store/migrations.go +++ b/internal/store/migrations.go @@ -91,7 +91,9 @@ ALTER TABLE reminders ADD COLUMN next_fire_ts INTEGER;`, // #2 // #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. + // 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')), @@ -103,7 +105,10 @@ ALTER TABLE reminders ADD COLUMN next_fire_ts INTEGER;`, // #2 created_ts INTEGER NOT NULL, completed_ts INTEGER ); - INSERT INTO delivery_attempts_v12 SELECT * FROM delivery_attempts; + 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);`,