Never send the nudge body off-box; record a suppressed nudge (#368, #370) #33

2026-07-31T12:31:55+02:00

claude commented

2026-07-31 12:31:55 +02:00

Three commits, ~150 lines. Vikunja #368 and #370 — both about the delivery boundary.

#368 — the body must never leave the box. DESIGN.md § Delivery: away-channels leave the box, so the message must be minimal, and "don't make notifications a shoulder-surf exfil surface".

Most of this turned out to be already fixed on the branch below this one — the dispatcher already strips detail for away channels. What was still there: both away sinks still had their own if body == "" { body = d.Body } fallback. Dead code today, because the dispatcher gets there first. But it is the wrong default sitting one refactor away from being live, in the two files that actually talk to the network.

So the rule is now one exported function, delivery.AwayMessage, and both sinks call it. Summary if we have one, otherwise a fixed generic line plus the rule name. Never the body — a sink can no longer leak it on its own initiative. This matters because the 0.8B model produces malformed output regularly, and an empty field is one of the likelier malformations, so "the phraser failed" must not mean "send everything".

Four sink tests asserted the old behaviour and were rewritten, not deleted — they now prove the generic line goes out where they used to prove the body did.

#370 — a drop was invisible. Suppressing a sev1-2 care nudge while you're away is correct and still happens; "a missed water nudge is noise, a missed backup failure isn't". Only the visibility was missing: the drop branch was a bare continue, so afterwards "she dropped it", "the gate suppressed it" and "the rule never fired" all looked identical. DESIGN.md: "why Maven went quiet should be a query, not a mystery."

Now it leaves a dropped outbox row and a log line. New status ⇒ migration #12, and sqlite can't widen a CHECK constraint in place, so the table is rebuilt and its index recreated. CompleteDeliveryAttempt also validated against a hardcoded pair and would have rejected the new status.

The third commit is the risk in that migration. The row copy used SELECT *, which matches columns by position. Correct today — I checked it against migration #6 column by column — but if the order ever differed it would shuffle every row rather than fail. The columns are listed out now.

One judgement call worth your disagreement: a drop leaves an outbox row but no nudges row. That table is the feedback loop's only input, and a nudge you could never have seen must not count against ignored_rate. The written test only asked for the outbox row, so that is what it does.

Tests: the two skipped tests from the tasks are un-skipped and passing. Added one that hits real sqlite — every other delivery test uses a fake outbox, so nothing would have caught the CHECK constraint rejecting dropped. One existing test changed expectation: with no live voice session, sev1-2 reroutes through away and now hits the drop, so it records two rows instead of one. That is the change working.

Two things you should know:

The dropped rows still can't be seen anywhere. There is no read path for delivery_attempts at all — no store lister, nothing in mavweb. Filed as #390; without it this fix only reaches someone reading daemon logs or the DB by hand.
If the outbox write itself fails, the drop leaves a pending row, which startup reconciliation later marks unknown — "may or may not have been sent". For a drop that's a small lie. Local DB write failure only, and the log line fires regardless, so I left it.

Three commits, ~150 lines. Vikunja #368 and #370 — both about the delivery boundary. **#368 — the body must never leave the box.** DESIGN.md § Delivery: away-channels leave the box, so the message must be minimal, and "don't make notifications a shoulder-surf exfil surface". Most of this turned out to be **already fixed** on the branch below this one — the dispatcher already strips detail for away channels. What was still there: **both away sinks still had their own `if body == "" { body = d.Body }` fallback.** Dead code today, because the dispatcher gets there first. But it is the wrong default sitting one refactor away from being live, in the two files that actually talk to the network. So the rule is now one exported function, `delivery.AwayMessage`, and both sinks call it. Summary if we have one, otherwise a fixed generic line plus the rule name. **Never the body** — a sink can no longer leak it on its own initiative. This matters because the 0.8B model produces malformed output regularly, and an empty field is one of the likelier malformations, so "the phraser failed" must not mean "send everything". Four sink tests asserted the old behaviour and were rewritten, not deleted — they now prove the generic line goes out where they used to prove the body did. **#370 — a drop was invisible.** Suppressing a sev1-2 care nudge while you're away is correct and **still happens**; "a missed water nudge is noise, a missed backup failure isn't". Only the visibility was missing: the drop branch was a bare `continue`, so afterwards "she dropped it", "the gate suppressed it" and "the rule never fired" all looked identical. DESIGN.md: "why Maven went quiet should be a query, not a mystery." Now it leaves a `dropped` outbox row and a log line. New status ⇒ **migration #12**, and sqlite can't widen a `CHECK` constraint in place, so the table is rebuilt and its index recreated. `CompleteDeliveryAttempt` also validated against a hardcoded pair and would have rejected the new status. **The third commit is the risk in that migration.** The row copy used `SELECT *`, which matches columns **by position**. Correct today — I checked it against migration #6 column by column — but if the order ever differed it would shuffle every row rather than fail. The columns are listed out now. **One judgement call worth your disagreement:** a drop leaves an outbox row but **no `nudges` row**. That table is the feedback loop's only input, and a nudge you could never have seen must not count against `ignored_rate`. The written test only asked for the outbox row, so that is what it does. **Tests:** the two skipped tests from the tasks are un-skipped and passing. Added one that hits **real sqlite** — every other delivery test uses a fake outbox, so nothing would have caught the CHECK constraint rejecting `dropped`. One existing test changed expectation: with no live voice session, sev1-2 reroutes through away and now hits the drop, so it records two rows instead of one. That is the change working. **Two things you should know:** 1. **The `dropped` rows still can't be seen anywhere.** There is no read path for `delivery_attempts` at all — no store lister, nothing in mavweb. Filed as #390; without it this fix only reaches someone reading daemon logs or the DB by hand. 2. If the outbox write itself fails, the drop leaves a `pending` row, which startup reconciliation later marks `unknown` — "may or may not have been sent". For a drop that's a small lie. Local DB write failure only, and the log line fires regardless, so I left it.

claude added 4 commits 2026-07-31 12:31:56 +02:00

Never send the full nudge body off-box (#368 ) e9ff2c4912

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ

Record a suppressed care nudge instead of dropping it silently (#370 ) 0272dc9d89

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ

List the columns in the table rebuild 59cec63da1

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ

Merge the delivery boundary fixes fa5aebfbe4

kami commented

2026-07-31 20:22:25 +02:00

Superseded by #47, which landed this whole stack on master as one reviewed integration merge. This PR head is an ancestor of master — its commits are in, nothing here is lost. Closing as merged-by-proxy rather than merged, since the merge came in through #47.

Review threads on this PR were answered or acted on before the merge; the Russian wording fixes went in as #48.

Superseded by #47, which landed this whole stack on master as one reviewed integration merge. This PR head is an ancestor of master — its commits are in, nothing here is lost. Closing as merged-by-proxy rather than merged, since the merge came in through #47. Review threads on this PR were answered or acted on before the merge; the Russian wording fixes went in as #48.

kami closed this pull request

2026-07-31 20:22:26 +02:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: kami/Maven#33