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

Make accepted routines keep firing instead of nudging once #11

Closed
claude wants to merge 3 commits from overnight/routines-fire into overnight/router-prompt
pull from: overnight/routines-fire
merge into: kami:overnight/router-prompt
Conversation 2 Commits 3 Files Changed 16
+398 -85
claude commented 2026-07-31 08:08:15 +02:00
Contributor
Copy Link
Copy Source

Vikunja #366. Accepting a routine used to create one reminder. One reminder fires once. So a routine on any interval except weekly nudged you exactly once and then went quiet forever — and because the dedup key is UNIQUE(action, object), the detector could never propose it again either.

Accepting a routine was worse than ignoring it. Ignoring at least left the door open.

You asked for an assistant that notices patterns and builds routines from them. This is the part of that which did not work.

Confirmed before fixing

Two tests written first, both failing on the old code:

--- FAIL: TestTickFiresAcceptedRoutineEveryInterval
    first interval: sends = 0, want 1
--- FAIL: TestTickAcceptedRoutineRespectsGate
    present again: sends = 0, want 1

Weekly genuinely did work — narrower than the issue read. The accept path only built a cron string for intervals between 6.5 and 7.5 days, and cron reminders get rescheduled after delivery. Everything else got an empty cron and one shot.

The fix

The tick loop now reads accepted routines from the store every tick and works out what is due. The schedule lives in the database, so it survives a restart — unlike the in-memory map the older cron routines use.

  • No catch-up, ever. The fire time recorded is now, not when it was theoretically due. A month of downtime produces one nudge, never a backlog of thirty.
  • The gate applies. Quiet hours, away, and snooze all suppress a routine nudge. This is a deliberate change: the old reminder path bypassed the gate, because reminders are allowed to. A routine is not a reminder — you did not ask for this at 3am, a pattern detector guessed it.
  • Suppressed means held, not dropped: a routine that the gate blocks is not marked fired, so it goes out on the next allowed tick rather than being skipped for a whole interval.
  • It nudges. It never acts.

Two things I had to fix on merge

Both this branch and the snooze branch numbered their migration "#8". Migrations are positional in a slice, so that would have been a silent mess. Snooze keeps #8, routines becomes #9.

The web accept path had the same bug and this branch had not touched it — the agent branched from master and could not see the /routines accept button that landed earlier tonight. So reminder_id is gone from the accept call across IPC, and both surfaces now just flip the status. One test was named Accept_CreatesReminderAndLinksIt; it was asserting the bug, and now asserts that accepting creates no reminder.

Known gaps, filed not fixed

  • Rows accepted before this change stay stuck. They have accepted_ts IS NULL and get skipped on purpose — they still own a live reminder, and back-filling would double-nudge the weekly ones. A one-off backfill deserves its own task.
  • Nothing shows you accepted routines. /routines lists proposals only, so there is still no surface where a routine that stopped firing would be visible. That is what let this bug hide.
Vikunja **#366**. Accepting a routine used to create one reminder. One reminder fires once. So a routine on any interval except weekly nudged you exactly once and then went quiet forever — and because the dedup key is `UNIQUE(action, object)`, the detector could never propose it again either. **Accepting a routine was worse than ignoring it.** Ignoring at least left the door open. You asked for an assistant that notices patterns and builds routines from them. This is the part of that which did not work. ## Confirmed before fixing Two tests written first, both failing on the old code: ``` --- FAIL: TestTickFiresAcceptedRoutineEveryInterval first interval: sends = 0, want 1 --- FAIL: TestTickAcceptedRoutineRespectsGate present again: sends = 0, want 1 ``` **Weekly genuinely did work** — narrower than the issue read. The accept path only built a cron string for intervals between 6.5 and 7.5 days, and cron reminders get rescheduled after delivery. Everything else got an empty cron and one shot. ## The fix The tick loop now reads accepted routines from the store every tick and works out what is due. The schedule lives in the database, so it survives a restart — unlike the in-memory map the older cron routines use. - **No catch-up, ever.** The fire time recorded is *now*, not when it was theoretically due. A month of downtime produces one nudge, never a backlog of thirty. - **The gate applies.** Quiet hours, away, and snooze all suppress a routine nudge. This is a deliberate change: the old reminder path bypassed the gate, because reminders are allowed to. A routine is not a reminder — you did not ask for this at 3am, a pattern detector guessed it. - Suppressed means **held, not dropped**: a routine that the gate blocks is not marked fired, so it goes out on the next allowed tick rather than being skipped for a whole interval. - It nudges. It never acts. ## Two things I had to fix on merge **Both this branch and the snooze branch numbered their migration "#8".** Migrations are positional in a slice, so that would have been a silent mess. Snooze keeps #8, routines becomes #9. **The web accept path had the same bug** and this branch had not touched it — the agent branched from `master` and could not see the `/routines` accept button that landed earlier tonight. So `reminder_id` is gone from the accept call across IPC, and both surfaces now just flip the status. One test was named `Accept_CreatesReminderAndLinksIt`; it was asserting the bug, and now asserts that accepting creates no reminder. ## Known gaps, filed not fixed - **Rows accepted before this change stay stuck.** They have `accepted_ts IS NULL` and get skipped on purpose — they still own a live reminder, and back-filling would double-nudge the weekly ones. A one-off backfill deserves its own task. - **Nothing shows you accepted routines.** `/routines` lists proposals only, so there is still no surface where a routine that stopped firing would be visible. That is what let this bug hide.
kami commented 2026-07-31 18:45:48 +02:00
Owner
Copy Link
Copy Source

this one shouldn't go in master.

this one shouldn't go in master.
kami changed target branch from master to overnight/router-prompt 2026-07-31 19:12:50 +02:00
kami added 3 commits 2026-07-31 19:12:50 +02:00
Add accepted-routine timestamps to the store (Vikunja #366) 54dc43516b 
Data layer only. Migration #8 adds accepted_ts and last_fired_ts to
proposed_routines, plus ListAcceptedRoutines and MarkRoutineFired so the
tick loop can own the schedule. Accepting no longer links a reminder id.
Look at the TODO(vikunja#366) in cmd/mavend/tick.go for the next commit.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Fire accepted routines every interval, not once (Vikunja #366) 424d1b3446 
The tick loop now reads accepted routines from the store and nudges when
their interval has passed; accepting no longer builds a one-shot reminder.
Look at routine.DueAccepted for the schedule rule (no catch-up backlog) and
at fireAcceptedRoutines for the restraint gate — routines do not bypass it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
Merge the accepted-routine fix and drop reminder_id from accept 75b067ac51 
Two merge fixes on top of the branch:

- migrations: keep both new steps, snooze stays #8, the routine columns
  become #9. Both agents had numbered theirs #8.
- accepting no longer takes a reminder id, on the web surface too. The
  web accept path had the same one-shot-reminder bug the voice path did,
  so both now just flip the status and let the tick loop schedule.

The test that asserted "accept creates a reminder and links it" asserted
the bug. It now asserts that accepting creates no reminder.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CGeSZxh1DCtRxmFVSYVGvJ
kami commented 2026-07-31 20:21:45 +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:46 +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#11
Delete Branch "overnight/routines-fire"

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?