kami/Maven
1
0
Fork 0
Code Issues 1 Pull Requests 1 Actions Packages Projects Releases Wiki Activity

Make snooze actually suppress nudges #7

Closed
claude wants to merge 0 commits from overnight/snooze-works into master
pull from: overnight/snooze-works
merge into: kami:master
Conversation 2 Commits - Files Changed -
claude commented 2026-07-31 00:34:39 +02:00
Contributor
Copy Link
Copy Source

Vikunja #364. Stacked on #2 — that PR added two tests marked t.Skip because the behaviour did not exist. This PR deletes both skips. That is the finish line, and it is the clearest thing to look at in the diff.

The bug. The restraint gate reads State.SnoozeUntil and honours it correctly. But the Gatherer hard-coded that map to nil (gather.go:153, comment "no snooze persistence yet"). There was no store table behind it.

So every snooze test passed while the running daemon could never fill the map. Snooze a nudge and Maven nudges you again anyway. For an assistant whose whole design principle is "not a nag", that was the wrong bug to have — and the signal was already sitting in the nudges table being thrown away.

Second, smaller half: reminders had no snooze check at all (loop.go:120). DESIGN.md says a reminder bypasses the gate — "wake me 7" has to fire at night — but snooze still applies. Now it does, and only snooze; quiet hours, presence and cooldown still do not apply to reminders.

Commit 1 — the read. store.SnoozedUntil turns recorded snoozed outcomes into a per-rule snooze-until. One query per tick, GROUP BY rule over snoozed rows only, with a partial index (migration #8).

Commit 2 — the wiring, plus deleting the two skips.

Decisions worth your eye:

  • A snooze lasts 2 hours, fixed. No new column: ResolveNudge(id, outcome, ts) is the whole signature out through IPC, so no caller anywhere could supply a length — a per-nudge column would have had no writer. 2h is longer than every rule's base cooldown (15–60m), so a snooze actually buys quiet instead of being swallowed by the cooldown, and short enough that a forgotten one clears the same day. Easy to change if it feels wrong in practice.
  • A snooze can never become permanent. Expiry is filtered in SQL, so an old row cannot come back as a forever-mute. If Maven goes quiet you should be able to ask why, not guess — DESIGN.md is explicit about that.

One honest limitation: nothing writes a reminder snooze yet. The read and the check are in place, but DispatchReminder records no nudge row, so no caller can resolve one with rule="reminder". The reminder half is correct and currently unreachable — it needs a "snooze this reminder" path in delivery or the web UI, which is a separate change. Worth knowing before you assume it works end to end.

Also noted, not fixed: a collapsed reminder digest is one synthetic row with ID=0, so a per-id snooze on one reminder inside a digest is not seen. Dropping the whole digest would be worse.

Vikunja #364. Stacked on **#2** — that PR added two tests marked `t.Skip` because the behaviour did not exist. **This PR deletes both skips.** That is the finish line, and it is the clearest thing to look at in the diff. **The bug.** The restraint gate reads `State.SnoozeUntil` and honours it correctly. But the Gatherer hard-coded that map to `nil` (`gather.go:153`, comment "no snooze persistence yet"). There was no store table behind it. So every snooze test passed while the running daemon could never fill the map. **Snooze a nudge and Maven nudges you again anyway.** For an assistant whose whole design principle is "not a nag", that was the wrong bug to have — and the signal was already sitting in the `nudges` table being thrown away. Second, smaller half: reminders had no snooze check at all (`loop.go:120`). DESIGN.md says a reminder bypasses the gate — "wake me 7" has to fire at night — **but snooze still applies.** Now it does, and only snooze; quiet hours, presence and cooldown still do not apply to reminders. **Commit 1 — the read.** `store.SnoozedUntil` turns recorded `snoozed` outcomes into a per-rule snooze-until. One query per tick, `GROUP BY rule` over snoozed rows only, with a partial index (migration #8). **Commit 2 — the wiring**, plus deleting the two skips. **Decisions worth your eye:** - **A snooze lasts 2 hours, fixed.** No new column: `ResolveNudge(id, outcome, ts)` is the whole signature out through IPC, so no caller anywhere could supply a length — a per-nudge column would have had no writer. 2h is longer than every rule's base cooldown (15–60m), so a snooze actually buys quiet instead of being swallowed by the cooldown, and short enough that a forgotten one clears the same day. Easy to change if it feels wrong in practice. - **A snooze can never become permanent.** Expiry is filtered in SQL, so an old row cannot come back as a forever-mute. If Maven goes quiet you should be able to ask why, not guess — DESIGN.md is explicit about that. **One honest limitation:** nothing writes a reminder snooze yet. The read and the check are in place, but `DispatchReminder` records no nudge row, so no caller can resolve one with `rule="reminder"`. The reminder half is correct and currently unreachable — it needs a "snooze this reminder" path in delivery or the web UI, which is a separate change. Worth knowing before you assume it works end to end. Also noted, not fixed: a collapsed reminder digest is one synthetic row with `ID=0`, so a per-id snooze on one reminder inside a digest is not seen. Dropping the whole digest would be worse.
claude added 4 commits 2026-07-31 00:34:40 +02:00
Unit-test all five proactive rule predicates 9e2af3286b 
Table-driven tests for water, meal, break, service_down and netdata_critical,
straight against the predicate with a fake State. Reviewers: the no-data rows
(every rule must stay quiet when its key is missing) and the ops forgery rows,
where a fact with the right value but the wrong source must be refused.
No rule fired on missing data, so no fix was needed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Test the universal restraint gate, including two gaps 9d8fcf42f3 
Pins the conservative side of Gate(): quiet hours, away, calendar-busy,
cooldown and snooze, plus one nudge per tick at max severity. Reviewers: the
two skipped tests at the bottom are real gaps, not flakes. Reminders ignore
snooze (loop.go:120) and the Gatherer never fills SnoozeUntil (gather.go:153),
so snooze does nothing at runtime. No behaviour was changed.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Read the recorded snooze outcomes back out of the nudges table (#364) 8acb8a97c6 
The gate honours State.SnoozeUntil but nothing ever filled it. New
store.SnoozedUntil returns, per rule, when the newest snooze runs out.
Reviewer: the fixed 2h SnoozeDuration and its reasoning in nudges.go —
nothing upstream can supply a per-nudge length, so no new column.
Expired snoozes are dropped in SQL, so silence can never be permanent.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Wire the snooze read into the Gatherer and honour it for reminders (#364) ee3e6a9eaf 
The Gatherer now fills State.SnoozeUntil from store.SnoozedUntil instead
of nil, so a snooze finally reaches the gate. RemindDecisions gains the
one restraint check that applies to a reminder — quiet hours, presence
and cooldown are still bypassed, so "wake me 7" is unchanged. Reviewer:
the two tests in internal/loop/gate_test.go are the contract.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
kami commented 2026-07-31 18:45:27 +02:00
Owner
Copy Link
Copy Source

this one shouldn't go in master.

this one shouldn't go in master.
kami commented 2026-07-31 20:21:38 +02:00
Owner
Copy Link
Copy Source

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:21:38 +02:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
kami
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: kami/Maven#7
Delete Branch "overnight/snooze-works"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?