diff options
| -rw-r--r-- | internal/store/chains_test.go | 107 | ||||
| -rw-r--r-- | internal/store/native_tasks.go | 26 | ||||
| -rw-r--r-- | internal/store/sqlite_test.go | 9 |
3 files changed, 136 insertions, 6 deletions
diff --git a/internal/store/chains_test.go b/internal/store/chains_test.go index bcfc09e..d21490c 100644 --- a/internal/store/chains_test.go +++ b/internal/store/chains_test.go @@ -1,8 +1,12 @@ package store import ( + "os" + "strings" "testing" + "time" + "task-dashboard/internal/config" "task-dashboard/internal/models" ) @@ -377,3 +381,106 @@ func TestDeleteNativeTask_LastRemainingChainTask_MarksChainCompleted(t *testing. t.Errorf("chain.Status = %q, want completed", updatedChain.Status) } } + +// Pausing a chain is meant to take its work off the list entirely -- the whole +// point of the control. But the task-fetching queries only ever checked +// `chain_unlocked`, never the owning chain's status, so a paused chain's head +// task kept showing up exactly like an active one. Found live: 4 of the 8 chain +// tasks on the real dashboard came from chains that had been explicitly paused. +func TestPausedChain_HeadTaskHiddenFromDateRange(t *testing.T) { + s := newNativeTasksTestStore(t) + + chain, err := s.CreateChain("Paused Track", chainTasks("Step 1", "Step 2")) + if err != nil { + t.Fatal(err) + } + if err := s.SetChainStatus(chain.ID, "paused"); err != nil { + t.Fatal(err) + } + + now := config.Now() + tasks, err := s.GetNativeTasksByDateRange(now.Add(-24*time.Hour), now.Add(24*time.Hour)) + if err != nil { + t.Fatalf("GetNativeTasksByDateRange: %v", err) + } + for _, task := range tasks { + if task.ChainID == chain.ID { + t.Errorf("paused chain's task %q leaked into GetNativeTasksByDateRange", task.Content) + } + } +} + +func TestPausedChain_HeadTaskHiddenFromOverdue(t *testing.T) { + s := newNativeTasksTestStore(t) + + chain, err := s.CreateChain("Paused Track", chainTasks("Step 1", "Step 2")) + if err != nil { + t.Fatal(err) + } + if err := s.SetChainStatus(chain.ID, "paused"); err != nil { + t.Fatal(err) + } + + tasks, err := s.GetOverdueNativeTasks(config.Now().Add(24 * time.Hour)) + if err != nil { + t.Fatalf("GetOverdueNativeTasks: %v", err) + } + for _, task := range tasks { + if task.ChainID == chain.ID { + t.Errorf("paused chain's task %q leaked into GetOverdueNativeTasks", task.Content) + } + } +} + +func TestPausedChain_HeadTaskHiddenFromUndated(t *testing.T) { + s := newNativeTasksTestStore(t) + + chain, err := s.CreateChain("Paused Track", chainTasks("Step 1", "Step 2")) + if err != nil { + t.Fatal(err) + } + // CreateChain seeds the head with a due date; clear it so this task is + // actually a candidate for the undated query rather than passing vacuously. + if _, err := s.db.Exec(`UPDATE native_tasks SET due_date = NULL WHERE chain_id = ?`, chain.ID); err != nil { + t.Fatal(err) + } + if err := s.SetChainStatus(chain.ID, "paused"); err != nil { + t.Fatal(err) + } + + tasks, err := s.GetUndatedNativeTasks() + if err != nil { + t.Fatalf("GetUndatedNativeTasks: %v", err) + } + for _, task := range tasks { + if task.ChainID == chain.ID { + t.Errorf("paused chain's task %q leaked into GetUndatedNativeTasks", task.Content) + } + } +} + +// Structural guard for the whole class, not just the three queries fixed above: +// any future task query that filters on chain_unlocked must also constrain the +// owning chain's status, or it reintroduces exactly the leak these tests cover. +// Checked by source inspection because the failure mode is "someone wrote a +// fourth query and copied the old predicate," which no single behavioral test +// would catch. +func TestChainVisibilityPredicate_UsedEverywhereChainUnlockedIsFiltered(t *testing.T) { + src, err := os.ReadFile("native_tasks.go") + if err != nil { + t.Fatal(err) + } + text := string(src) + + // The copy-paste hazard is specifically the inline "chain_id = '' OR ..." + // predicate. It should appear exactly once: inside the shared constant's own + // definition. (A bare `chain_unlocked = 1` is fine elsewhere -- advanceChain + // legitimately SETs it.) + if inline := strings.Count(text, "chain_id = '' OR"); inline != 1 { + t.Errorf("found the inline chain-visibility predicate %d time(s) in native_tasks.go, want exactly 1 (the visibleChainTaskClause definition); a query spelling it out inline will silently skip the chain-status check", inline) + } + // Const declaration plus one reference per task query that hides chain work. + if refs := strings.Count(text, "visibleChainTaskClause"); refs < 4 { + t.Errorf("visibleChainTaskClause referenced %d time(s), want >= 4 (declaration + the task queries); a query that stopped using it has drifted off the shared predicate", refs) + } +} diff --git a/internal/store/native_tasks.go b/internal/store/native_tasks.go index 9d9af2c..ea5212c 100644 --- a/internal/store/native_tasks.go +++ b/internal/store/native_tasks.go @@ -21,6 +21,20 @@ import ( // completion: the HTTP response was 200, but nothing in the database changed. var ErrNativeTaskNotFound = errors.New("native task not found") +// visibleChainTaskClause is the single source of truth for "which chain tasks +// belong on the list." Every task query that hides locked chain work must use +// this rather than spelling the predicate out inline -- see +// TestChainVisibilityPredicate_UsedEverywhereChainUnlockedIsFiltered. +// +// The chain's own status matters as much as the task's lock state: pausing a +// chain is meant to take its work off the list entirely, but for a long time +// these queries checked only chain_unlocked, so a paused chain's head task kept +// appearing exactly like an active one -- the pause control did nothing to the +// list it was supposed to affect. +const visibleChainTaskClause = `(t.chain_id = '' OR (t.chain_unlocked = 1 AND EXISTS ( + SELECT 1 FROM task_chains c WHERE c.id = t.chain_id AND c.status = 'active' + )))` + // GetNativeTasks returns all non-completed native tasks. func (s *Store) GetNativeTasks() ([]models.Task, error) { rows, err := s.db.Query(` @@ -49,9 +63,9 @@ func (s *Store) GetNativeTasksByDateRange(start, end time.Time) ([]models.Task, recurrence_freq, recurrence_interval, recurrence_weekdays, recurrence_series_id, next_occurrence_override, estimated_minutes, chain_id, chain_position, chain_unlocked, bucket_id, bucket_state, bucket_last_active_at - FROM native_tasks + FROM native_tasks t WHERE completed = 0 AND due_date IS NOT NULL AND due_date >= ? AND due_date < ? - AND (chain_id = '' OR chain_unlocked = 1) + AND `+visibleChainTaskClause+` ORDER BY due_date ASC, priority DESC `, start, end) if err != nil { @@ -73,9 +87,9 @@ func (s *Store) GetOverdueNativeTasks(before time.Time) ([]models.Task, error) { recurrence_freq, recurrence_interval, recurrence_weekdays, recurrence_series_id, next_occurrence_override, estimated_minutes, chain_id, chain_position, chain_unlocked, bucket_id, bucket_state, bucket_last_active_at - FROM native_tasks + FROM native_tasks t WHERE completed = 0 AND due_date IS NOT NULL AND due_date < ? - AND (chain_id = '' OR chain_unlocked = 1) + AND `+visibleChainTaskClause+` ORDER BY due_date ASC, priority DESC `, before) if err != nil { @@ -92,9 +106,9 @@ func (s *Store) GetUndatedNativeTasks() ([]models.Task, error) { recurrence_freq, recurrence_interval, recurrence_weekdays, recurrence_series_id, next_occurrence_override, estimated_minutes, chain_id, chain_position, chain_unlocked, bucket_id, bucket_state, bucket_last_active_at - FROM native_tasks + FROM native_tasks t WHERE completed = 0 AND due_date IS NULL - AND (chain_id = '' OR chain_unlocked = 1) + AND `+visibleChainTaskClause+` AND bucket_state != 'dormant' ORDER BY priority DESC, created_at ASC `) diff --git a/internal/store/sqlite_test.go b/internal/store/sqlite_test.go index c64e6b3..95996b3 100644 --- a/internal/store/sqlite_test.go +++ b/internal/store/sqlite_test.go @@ -193,6 +193,15 @@ func setupTestStoreWithNativeTasks(t *testing.T) *Store { bucket_state TEXT DEFAULT '', bucket_last_active_at DATETIME ); + -- The task queries join against this to check the owning chain's status + -- (see visibleChainTaskClause), so it has to exist even for tests that + -- only ever create plain, chain-less tasks. + CREATE TABLE IF NOT EXISTS task_chains ( + id TEXT PRIMARY KEY, + project_id TEXT DEFAULT '', + status TEXT DEFAULT 'active', + created_at DATETIME DEFAULT CURRENT_TIMESTAMP + ); ` if _, err := db.Exec(schema); err != nil { t.Fatalf("Failed to create schema: %v", err) |