Merge the routine, morning and tasks sweep (#242)

Four real bugs, all of them the kind that show as a wrong number or a silence.

tasks.Stalls compared Due against an instant while Rank compares whole calendar
days through dayDelta, under a long comment about that exact trap. Both render
on /tasks, so a task due at 09:00 counted as просрочено in the header from 09:01
while its own row still read сегодня. Stalls reads dayDelta now.

tasks.Stalls also counted a row with no capture time as sitting, because the
zero time is January of year 1. score() already guarded IsZero and Stalls did
not.

morning and routine both key their last-fired map by name, and neither Validate
rejected a duplicate. Two routines sharing one name take turns suppressing each
other, and the operator sees a routine that never runs and no error. Both
Validate functions reject it now.

parseHHMM checked digits arithmetically, so a stray character could cancel out.
window_start: "2 :00" parsed as 04:00 and passed the validation whose whole job
is catching that typo. All four positions are checked as digits, which makes the
negative bounds unreachable, so they are gone.

FormatRU and Spoken each carried a byte-identical open and candidate partition,
now one split. They have to agree on where that line falls, or she reads one
list and binds ordinals against another. The three copies of the unevidenced
item loop folded into one helper.

The not-a-nag check passes. All three packages are pure, return candidates, and
reach no sink.

(V-581)

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

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) {

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

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)
 		}

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) {

internal/tasks/rank.go

@@ -208,14 +208,7 @@ const SpokenLimit = 5
 // DayPlan.Spoken is built core-side: two formatters drift, and then she says
 // one order and shows another.
 func FormatRU(ranked []Ranked) string {
-	var open, cands []Ranked
-	for _, r := range ranked {
-		if r.Status == StatusCandidate {
-			cands = append(cands, r)
-		} else {
-			open = append(open, r)
-		}
-	}
+	open, cands := split(ranked)
 	if len(open) == 0 && len(cands) == 0 {
 		return say.S(say.TasksNone, nil)
 	}
@@ -243,6 +236,20 @@ func FormatRU(ranked []Ranked) string {
 	return b.String()
 }
 
+// split separates confirmed work from candidates, preserving Rank's order
+// within each half. FormatRU and Spoken have to agree on where the line falls,
+// so they read it from one place.
+func split(ranked []Ranked) (open, cands []Ranked) {
+	for _, r := range ranked {
+		if r.Status == StatusCandidate {
+			cands = append(cands, r)
+		} else {
+			open = append(open, r)
+		}
+	}
+	return open, cands
+}
+
 // joinRU lists up to limit tasks, then says how many are left. withReasons
 // attaches the parenthesised reason — candidates are listed bare, since their
 // due dates are Maven's reading of a mail and not something he stated.
@@ -273,14 +280,7 @@ func joinRU(rs []Ranked, limit int, withReasons bool) string {
 // an ordinal resolves against is built here and not by a caller guessing how
 // the renderer split and truncated it.
 func Spoken(ranked []Ranked) []Ranked {
-	var open, cands []Ranked
-	for _, r := range ranked {
-		if r.Status == StatusCandidate {
-			cands = append(cands, r)
-		} else {
-			open = append(open, r)
-		}
-	}
+	open, cands := split(ranked)
 	out := make([]Ranked, 0, 2*SpokenLimit)
 	for _, group := range [][]Ranked{open, cands} {
 		if len(group) > SpokenLimit {

internal/tasks/stall.go

@@ -56,10 +56,16 @@ func Stalls(items []Item, now time.Time) []Stall {
 			// meant to act on.
 			continue
 		}
-		if it.Due != nil && it.Due.Before(now) {
+		// Whole calendar days, the reading Rank already uses. An instant
+		// comparison calls a task due at 18:00 overdue from 18:01, so the count
+		// above the table would say "просрочено" beside a row whose own reason
+		// still said "сегодня".
+		if it.Due != nil && dayDelta(*it.Due, now) < 0 {
 			overdue++
 		}
-		if now.Sub(it.Created) >= StallDays*24*time.Hour {
+		// A row with no capture time has not sat for anything. Without the
+		// guard its zero time is January of year 1 and it always counts.
+		if !it.Created.IsZero() && now.Sub(it.Created) >= StallDays*24*time.Hour {
 			sitting++
 		}
 	}

internal/tasks/stall_test.go

@@ -54,6 +54,27 @@ func TestStallsSaysNothingWhenThereIsNothing(t *testing.T) {
 	}
 }
 
+func TestStallsCountsOverdueByCalendarDay(t *testing.T) {
+	// Same reading as Rank, or the count above the table contradicts the reason
+	// in the row: due at 09:00, asked at 12:00, and it is still due today.
+	now := stallNow()
+	earlier := now.Add(-3 * time.Hour)
+	items := []Item{{ID: 1, Text: "оплатить интернет", Status: StatusOpen, Created: now.Add(-time.Hour), Due: &earlier}}
+	if got := Stalls(items, now); len(got) != 0 {
+		t.Fatalf("shapes = %+v, want none — a task due today is not overdue", got)
+	}
+}
+
+func TestStallsIgnoresATaskWithNoCaptureTime(t *testing.T) {
+	// The zero time is January of year 1, so an unstamped row would count as
+	// sitting forever.
+	now := stallNow()
+	items := []Item{{ID: 1, Text: "купить молоко", Status: StatusOpen}}
+	if got := Stalls(items, now); len(got) != 0 {
+		t.Fatalf("shapes = %+v, want none", got)
+	}
+}
+
 func TestStallsCountsNoJudgement(t *testing.T) {
 	// The line this shape may not cross. Every sentence states a count; none of
 	// them says whether the work matters or should be dropped.