From 5447f08c0698c49ea278f7697a7dd3b632e6a3f5 Mon Sep 17 00:00:00 2001 From: claude Date: Thu, 6 Aug 2026 03:12:36 +0400 Subject: [PATCH] morning, routine: reject the two configs that silently do nothing (V-581) Both Due functions key their last-fired map by routine name, so two routines sharing a name took turns suppressing each other and one of them never fired. Validate now rejects a duplicate name in either package. parseHHMM checked the digits arithmetically, which let a stray character cancel out: window_start of 2 :00 loaded as 04:00 and passed the validation that exists to catch that typo. Each of the four positions is now checked as a digit, which makes the negative bounds unreachable and they are gone. Folded the three copies of the unevidenced-item loop in Evaluate, Outstanding and Due into one helper. --- internal/morning/morning.go | 54 +++++++++++++++++++++----------- internal/morning/morning_test.go | 13 ++++++++ internal/morning/plan.go | 8 ++--- internal/routine/routine.go | 13 ++++++-- internal/routine/routine_test.go | 6 ++++ 5 files changed, 69 insertions(+), 25 deletions(-) diff --git a/internal/morning/morning.go b/internal/morning/morning.go index 418ee44..3a55488 100644 --- a/internal/morning/morning.go +++ b/internal/morning/morning.go @@ -104,14 +104,22 @@ func OptionalOnly(missing []Item) []Item { } // Validate reports the first structural problem with a routine set: missing -// name/items, an unparseable HH:MM, an inverted window, a duplicate item key -// within a routine, or an out-of-range weekday. Called at config load so a -// typo surfaces at startup, not as a silently-broken checklist at runtime. +// name/items, an unparseable HH:MM, an inverted window, a duplicate routine or +// item key, or an out-of-range weekday. Called at config load so a typo +// surfaces at startup, not as a silently-broken checklist at runtime. +// +// Routine names must be unique because Due keys its once-a-day map by name. Two +// routines sharing one would take turns suppressing each other's nudge. func Validate(routines []Routine) error { + names := make(map[string]bool, len(routines)) for _, r := range routines { if r.Name == "" { return fmt.Errorf("morning: name is required") } + if names[r.Name] { + return fmt.Errorf("morning routine %q: duplicate name", r.Name) + } + names[r.Name] = true if len(r.Items) == 0 { return fmt.Errorf("morning routine %q: at least one item is required", r.Name) } @@ -177,6 +185,17 @@ func Evaluate(r Routine, facts map[string]store.Fact, now time.Time) Status { return st } +// missing lists the items of r with no evidence in [start, now]. +func missing(r Routine, facts map[string]store.Fact, start, now time.Time) []Item { + var out []Item + for _, it := range r.Items { + if !evidenced(it, facts, start, now) { + out = append(out, it) + } + } + return out +} + // Outstanding reports the items of a routine that today has no evidence for, // whether or not the window is still open. Evaluate answers "what is missing // right now" and goes silent the moment the window closes; the day plan asks a @@ -191,13 +210,7 @@ func Outstanding(r Routine, facts map[string]store.Fact, now time.Time) []Item { if !ok || now.Before(start) { return nil } - var missing []Item - for _, it := range r.Items { - if !evidenced(it, facts, start, now) { - missing = append(missing, it) - } - } - return missing + return missing(r, facts, start, now) } // Due returns the routines that have reached their nudge time today with at @@ -222,24 +235,19 @@ func Due(routines []Routine, facts map[string]store.Fact, last map[string]time.T if !ok || now.Before(nudgeAt) { continue } - var missing []Item - for _, it := range r.Items { - if !evidenced(it, facts, start, now) { - missing = append(missing, it) - } - } + skipped := missing(r, facts, start, now) // A day where only the optional items were skipped is a fine day, and // nagging about it is what teaches him to stop listening (Vikunja // #473). The optional ones still travel in Missing so the message can // mention them when it is being sent anyway. - if len(Required(missing)) == 0 { + if len(Required(skipped)) == 0 { continue } if prev, seen := last[r.Name]; seen && sameDay(prev, now) { continue } last[r.Name] = now - out = append(out, Candidate{Routine: r, Missing: missing}) + out = append(out, Candidate{Routine: r, Missing: skipped}) } return out } @@ -284,13 +292,21 @@ func sameDay(a, b time.Time) bool { return ay == by && am == bm && ad == bd } +// parseHHMM reads a five-character "HH:MM". Every digit is checked as a digit: +// arithmetic alone lets a stray character cancel out, so "2 :00" used to load +// as 04:00 and Validate passed the typo it exists to catch. func parseHHMM(s string) (hour, min int, ok bool) { if len(s) != 5 || s[2] != ':' { return 0, 0, false } + for _, i := range [4]int{0, 1, 3, 4} { + if s[i] < '0' || s[i] > '9' { + return 0, 0, false + } + } h := int(s[0]-'0')*10 + int(s[1]-'0') m := int(s[3]-'0')*10 + int(s[4]-'0') - if h < 0 || h > 23 || m < 0 || m > 59 { + if h > 23 || m > 59 { return 0, 0, false } return h, m, true diff --git a/internal/morning/morning_test.go b/internal/morning/morning_test.go index 1a9385e..9b3338c 100644 --- a/internal/morning/morning_test.go +++ b/internal/morning/morning_test.go @@ -56,6 +56,19 @@ func TestValidate(t *testing.T) { if err := Validate([]Routine{bad}); err == nil { t.Fatal("expected error for duplicate item key") } + + // Due keys its once-a-day map by name, so two routines sharing one would + // suppress each other's nudge instead of both firing. + if err := Validate([]Routine{r, r}); err == nil { + t.Fatal("expected error for duplicate routine name") + } + + // Arithmetic alone let a stray character cancel out: "2 :00" read as 04:00. + bad = r + bad.WindowStart = "2 :00" + if err := Validate([]Routine{bad}); err == nil { + t.Fatal("expected error for a non-digit in window_start") + } } func TestEvaluateInactiveOutsideWindow(t *testing.T) { diff --git a/internal/morning/plan.go b/internal/morning/plan.go index 5129187..ee863a2 100644 --- a/internal/morning/plan.go +++ b/internal/morning/plan.go @@ -113,12 +113,12 @@ func BuildPlan(routines []Routine, facts map[string]store.Fact, events, reminder func checklistEntries(routines []Routine, facts map[string]store.Fact, now time.Time) []PlanEntry { var out []PlanEntry for _, r := range routines { - missing := Outstanding(r, facts, now) - if len(missing) == 0 { + left := Outstanding(r, facts, now) + if len(left) == 0 { continue } - labels := make([]string, 0, len(missing)) - for _, it := range missing { + labels := make([]string, 0, len(left)) + for _, it := range left { label := it.Label if label == "" { label = it.Key diff --git a/internal/routine/routine.go b/internal/routine/routine.go index 3ee082e..6955dbe 100644 --- a/internal/routine/routine.go +++ b/internal/routine/routine.go @@ -38,13 +38,22 @@ type Routine struct { } // Validate reports the first structural problem with a routine set: a missing -// name/cron/body or an unparseable cron expression. Called at config load so a -// typo surfaces at startup, not as a silently-never-firing routine at runtime. +// name/cron/body, a duplicate name or an unparseable cron expression. Called at +// config load so a typo surfaces at startup, not as a silently-never-firing +// routine at runtime. +// +// Names must be unique because Due keys its last-fired map by name. Two +// routines sharing one would take turns being suppressed by each other's fire. func Validate(routines []Routine) error { + seen := make(map[string]bool, len(routines)) for _, r := range routines { if r.Name == "" { return fmt.Errorf("routine: name is required") } + if seen[r.Name] { + return fmt.Errorf("routine %q: duplicate name", r.Name) + } + seen[r.Name] = true if r.Body == "" { return fmt.Errorf("routine %q: body is required", r.Name) } diff --git a/internal/routine/routine_test.go b/internal/routine/routine_test.go index 7ca89c5..8817cfb 100644 --- a/internal/routine/routine_test.go +++ b/internal/routine/routine_test.go @@ -60,6 +60,12 @@ func TestValidate(t *testing.T) { } }) } + + // Due keys its last-fired map by name, so two routines sharing one would + // take turns being suppressed by the other's fire. + if err := Validate(append(ok, ok[0])); err == nil { + t.Error("expected an error for a duplicate name, got nil") + } } func TestDue(t *testing.T) {