fix(db): make task_action_counts trigger follow action task_id changes

.goldenrules-rule17-allowlist

@@ -19,13 +19,18 @@
 #     O(N), shows as SCAN in EXPLAIN but not actionable via index)
 
 db/action.go:399 SCAN actions
-db/db.go:167 SCAN tasks
-db/db.go:184 SCAN actions
-db/db.go:336 SCAN actions
-db/db.go:352 SCAN schedules
-db/db.go:399 SCAN CONSTANT
-db/db.go:399 SCAN task_action_counts
-db/db.go:405 SCAN actions
+db/db.go:173 SCAN tasks
+db/db.go:190 SCAN actions
+db/db.go:348 SCAN actions
+db/db.go:364 SCAN schedules
+# db/db.go:408 — dropStaleCountUpdateTrigger: sqlite_master lookup, no index possible
+db/db.go:408 SCAN sqlite_master
+# db/db.go:429-432 — rebuildTaskActionCounts: one-shot full rebuild on trigger migration
+db/db.go:429 SCAN task_action_counts
+db/db.go:432 SCAN actions
+db/db.go:446 SCAN CONSTANT
+db/db.go:446 SCAN task_action_counts
+db/db.go:452 SCAN actions
 # db/event.go:76 — ListRecentEvents: LIMIT-bounded PK-order scan
 # (ORDER BY id DESC LIMIT ?), not a real full scan; no index will help.
 db/event.go:76 SCAN events

db/db.go

@@ -5,6 +5,7 @@ import (
 	"database/sql"
 	_ "embed"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"strings"
 
@@ -102,6 +103,11 @@ func (db *DB) hasColumn(table, column string) (bool, error) {
 }
 
 func (db *DB) Migrate() error {
+	staleCountTrigger, err := db.dropStaleCountUpdateTrigger()
+	if err != nil {
+		return fmt.Errorf("migrate task_action_counts trigger: %w", err)
+	}
+
 	if _, err := db.Exec(schemaSQL); err != nil {
 		return err
 	}
@@ -319,6 +325,12 @@ func (db *DB) Migrate() error {
 		return fmt.Errorf("migrate experimental_bg mode: %w", err)
 	}
 
+	if staleCountTrigger {
+		if err := db.rebuildTaskActionCounts(); err != nil {
+			return fmt.Errorf("rebuild task_action_counts: %w", err)
+		}
+	}
+
 	if err := db.backfillTaskActionCounts(); err != nil {
 		return fmt.Errorf("backfill task_action_counts: %w", err)
 	}
@@ -391,21 +403,57 @@ func (db *DB) backfillSearchFTS() error {
 	return nil
 }
 
+func (db *DB) dropStaleCountUpdateTrigger() (bool, error) {
+	var sqlText string
+	err := db.QueryRow("SELECT sql FROM sqlite_master WHERE type = 'trigger' AND name = 'trg_actions_count_update'").Scan(&sqlText)
+	if errors.Is(err, sql.ErrNoRows) {
+		return false, nil
+	}
+	if err != nil {
+		return false, fmt.Errorf("read trigger definition: %w", err)
+	}
+	if strings.Contains(sqlText, "OLD.task_id != NEW.task_id") {
+		return false, nil
+	}
+	if _, err := db.Exec("DROP TRIGGER IF EXISTS trg_actions_count_update"); err != nil {
+		return false, fmt.Errorf("drop stale trigger: %w", err)
+	}
+	return true, nil
+}
+
+const recountTaskActionCountsSQL = "INSERT INTO task_action_counts (task_id, status, count) SELECT task_id, status, COUNT(*) FROM actions GROUP BY task_id, status"
+
+func (db *DB) rebuildTaskActionCounts() error {
+	ctx := context.Background()
+	return db.withTxRetry(ctx, "rebuildTaskActionCounts", func(tx *sql.Tx) error {
+		if _, err := tx.ExecContext(ctx, "DELETE FROM task_action_counts"); err != nil {
+			return fmt.Errorf("clear: %w", err)
+		}
+		if _, err := tx.ExecContext(ctx, recountTaskActionCountsSQL); err != nil {
+			return fmt.Errorf("recount: %w", err)
+		}
+		return nil
+	})
+}
+
 // backfillTaskActionCounts populates task_action_counts from existing actions
 // rows on the first migration. Idempotent: if the table already has rows,
 // triggers have been keeping it in sync, so skip.
 func (db *DB) backfillTaskActionCounts() error {
-	var hasRows int
-	if err := db.QueryRow("SELECT EXISTS(SELECT 1 FROM task_action_counts LIMIT 1)").Scan(&hasRows); err != nil {
-		return fmt.Errorf("check rows: %w", err)
-	}
-	if hasRows != 0 {
+	ctx := context.Background()
+	return db.withTxRetry(ctx, "backfillTaskActionCounts", func(tx *sql.Tx) error {
+		var hasRows int
+		if err := tx.QueryRowContext(ctx, "SELECT EXISTS(SELECT 1 FROM task_action_counts LIMIT 1)").Scan(&hasRows); err != nil {
+			return fmt.Errorf("check rows: %w", err)
+		}
+		if hasRows != 0 {
+			return nil
+		}
+		if _, err := tx.ExecContext(ctx, recountTaskActionCountsSQL); err != nil {
+			return fmt.Errorf("insert: %w", err)
+		}
 		return nil
-	}
-	if _, err := db.Exec("INSERT INTO task_action_counts (task_id, status, count) SELECT task_id, status, COUNT(*) FROM actions GROUP BY task_id, status"); err != nil {
-		return fmt.Errorf("insert: %w", err)
-	}
-	return nil
+	})
 }
 
 func (db *DB) Close() error {

db/schema.sql

@@ -81,8 +81,8 @@ BEGIN
 END;
 
 CREATE TRIGGER IF NOT EXISTS trg_actions_count_update
-AFTER UPDATE OF status ON actions
-WHEN OLD.status != NEW.status
+AFTER UPDATE OF task_id, status ON actions
+WHEN OLD.task_id != NEW.task_id OR OLD.status != NEW.status
 BEGIN
   UPDATE task_action_counts SET count = count - 1
   WHERE task_id = OLD.task_id AND status = OLD.status;

db/task_action_counts_test.go

@@ -2,6 +2,7 @@ package db_test
 
 import (
 	"sort"
+	"strings"
 	"testing"
 
 	"github.com/MH4GF/tq/db"
@@ -361,3 +362,162 @@ func TestTaskActionCounts_BackfillFromExisting(t *testing.T) {
 		t.Errorf("backfill mismatch:\ngot=%v\nwant=%v", got, want)
 	}
 }
+
+func TestTaskActionCounts_TaskIDChange(t *testing.T) {
+	d := testutil.NewTestDB(t)
+	testutil.SeedTestProjects(t, d)
+	task1, err := d.InsertTask(1, "task1", "{}", "")
+	if err != nil {
+		t.Fatalf("InsertTask task1: %v", err)
+	}
+	task2, err := d.InsertTask(1, "task2", "{}", "")
+	if err != nil {
+		t.Fatalf("InsertTask task2: %v", err)
+	}
+	a1, err := d.InsertAction("a1", task1, "{}", db.ActionStatusPending, nil, "")
+	if err != nil {
+		t.Fatalf("InsertAction: %v", err)
+	}
+
+	steps := []struct {
+		name   string
+		mutate func() error
+		want   []countRow
+	}{
+		{
+			name:   "initial: pending on task1",
+			mutate: func() error { return nil },
+			want:   []countRow{{task1, db.ActionStatusPending, 1}},
+		},
+		{
+			name: "task_id only: task1 → task2",
+			mutate: func() error {
+				_, err := d.Exec("UPDATE actions SET task_id = ? WHERE id = ?", task2, a1)
+				return err
+			},
+			want: []countRow{{task2, db.ActionStatusPending, 1}},
+		},
+		{
+			name: "task_id and status together: task2/pending → task1/running",
+			mutate: func() error {
+				_, err := d.Exec("UPDATE actions SET task_id = ?, status = ? WHERE id = ?", task1, db.ActionStatusRunning, a1)
+				return err
+			},
+			want: []countRow{{task1, db.ActionStatusRunning, 1}},
+		},
+	}
+
+	for _, st := range steps {
+		t.Run(st.name, func(t *testing.T) {
+			if err := st.mutate(); err != nil {
+				t.Fatalf("mutate: %v", err)
+			}
+			got := dumpCounts(t, d)
+			if !equalCountRows(got, st.want) {
+				t.Errorf("counts: got %v, want %v", got, st.want)
+			}
+			if want := dumpGroupBy(t, d); !equalCountRows(got, want) {
+				t.Errorf("counts diverge from GROUP BY actions:\ngroup_by=%v\ncounts=%v", want, got)
+			}
+		})
+	}
+}
+
+func TestTaskActionCounts_TaskReassignReleasesCloseGuard(t *testing.T) {
+	d := testutil.NewTestDB(t)
+	testutil.SeedTestProjects(t, d)
+	task1, err := d.InsertTask(1, "task1", "{}", "")
+	if err != nil {
+		t.Fatalf("InsertTask task1: %v", err)
+	}
+	task2, err := d.InsertTask(1, "task2", "{}", "")
+	if err != nil {
+		t.Fatalf("InsertTask task2: %v", err)
+	}
+	a1, err := d.InsertAction("a1", task1, "{}", db.ActionStatusPending, nil, "")
+	if err != nil {
+		t.Fatalf("InsertAction: %v", err)
+	}
+	if _, err := d.Exec("UPDATE actions SET task_id = ? WHERE id = ?", task2, a1); err != nil {
+		t.Fatalf("reassign: %v", err)
+	}
+
+	if err := d.UpdateTask(task1, db.TaskStatusDone, "all actions reassigned"); err != nil {
+		t.Errorf("close task1 after reassign: %v", err)
+	}
+	err = d.UpdateTask(task2, db.TaskStatusDone, "should be blocked")
+	if err == nil || !strings.Contains(err.Error(), "pending/running/dispatched") {
+		t.Errorf("close task2 with active action: got %v, want close-guard error", err)
+	}
+}
+
+const preTaskIDTriggerDDL = `CREATE TRIGGER trg_actions_count_update
+AFTER UPDATE OF status ON actions
+WHEN OLD.status != NEW.status
+BEGIN
+  UPDATE task_action_counts SET count = count - 1
+  WHERE task_id = OLD.task_id AND status = OLD.status;
+  INSERT INTO task_action_counts (task_id, status, count)
+  VALUES (NEW.task_id, NEW.status, 1)
+  ON CONFLICT(task_id, status) DO UPDATE SET count = count + 1;
+END`
+
+func TestTaskActionCounts_MigrateRecreatesStaleTrigger(t *testing.T) {
+	d := testutil.NewTestDB(t)
+	testutil.SeedTestProjects(t, d)
+	task1, err := d.InsertTask(1, "task1", "{}", "")
+	if err != nil {
+		t.Fatalf("InsertTask task1: %v", err)
+	}
+	task2, err := d.InsertTask(1, "task2", "{}", "")
+	if err != nil {
+		t.Fatalf("InsertTask task2: %v", err)
+	}
+	if _, err := d.Exec("DROP TRIGGER trg_actions_count_update"); err != nil {
+		t.Fatalf("drop trigger: %v", err)
+	}
+	if _, err := d.Exec(preTaskIDTriggerDDL); err != nil {
+		t.Fatalf("create stale trigger: %v", err)
+	}
+
+	a1, err := d.InsertAction("a1", task1, "{}", db.ActionStatusPending, nil, "")
+	if err != nil {
+		t.Fatalf("InsertAction: %v", err)
+	}
+	if _, err := d.Exec("UPDATE actions SET task_id = ? WHERE id = ?", task2, a1); err != nil {
+		t.Fatalf("reassign: %v", err)
+	}
+
+	ghost := []countRow{{task1, db.ActionStatusPending, 1}}
+	if got := dumpCounts(t, d); !equalCountRows(got, ghost) {
+		t.Fatalf("stale trigger should leave ghost count: got %v, want %v", got, ghost)
+	}
+	err = d.UpdateTask(task1, db.TaskStatusDone, "blocked by ghost count")
+	if err == nil || !strings.Contains(err.Error(), "pending/running/dispatched") {
+		t.Fatalf("close task1 before Migrate: got %v, want close-guard error", err)
+	}
+
+	if err := d.Migrate(); err != nil {
+		t.Fatalf("Migrate: %v", err)
+	}
+
+	var triggerSQL string
+	if err := d.QueryRow("SELECT sql FROM sqlite_master WHERE type = 'trigger' AND name = 'trg_actions_count_update'").Scan(&triggerSQL); err != nil {
+		t.Fatalf("read trigger: %v", err)
+	}
+	if !strings.Contains(triggerSQL, "OLD.task_id != NEW.task_id") {
+		t.Errorf("trigger not recreated with task_id coverage: %s", triggerSQL)
+	}
+
+	got := dumpCounts(t, d)
+	want := []countRow{{task2, db.ActionStatusPending, 1}}
+	if !equalCountRows(got, want) {
+		t.Errorf("counts after rebuild: got %v, want %v", got, want)
+	}
+	if groupBy := dumpGroupBy(t, d); !equalCountRows(got, groupBy) {
+		t.Errorf("counts diverge from GROUP BY actions:\ngroup_by=%v\ncounts=%v", groupBy, got)
+	}
+	if err := d.UpdateTask(task1, db.TaskStatusDone, "ghost count repaired"); err != nil {
+		t.Errorf("close task1 after Migrate: %v", err)
+	}
+}

docs/golden-rules.md

@@ -162,8 +162,8 @@ Current status totals are captured after each rule as `current violations: N`. A
 
 **Rule 18 [enforced] — `tui/` and `dispatch/` MUST NOT contain `SELECT COUNT/SUM/AVG` string literals. Aggregate-driven counts on `actions` go through `db.Store.GetTaskActionCount`, which reads from the trigger-maintained `task_action_counts` table.**
 
-- Why: `turso db inspect tq --queries` repeatedly surfaces `SELECT COUNT(*) FROM actions WHERE task_id = ? AND status IN (...)` in the top-N rows-read consumers. Turso bills per row read, so per-tick aggregate scans dominate quota. The `task_action_counts(task_id, status, count)` table is maintained by `AFTER INSERT / AFTER UPDATE OF status / AFTER DELETE` triggers on `actions`, so any task's status counts are a 1-row index lookup. This rule structurally bans hot-path code from re-introducing aggregate scans — it is a specialization of Rule 11 narrowed to the most expensive aggregates.
-- Assumption: `actions.task_id` is immutable. Triggers do not handle `task_id` changes; no production path issues `UPDATE actions SET task_id = ?`. If that changes, add a trigger or migrate the data manually.
+- Why: `turso db inspect tq --queries` repeatedly surfaces `SELECT COUNT(*) FROM actions WHERE task_id = ? AND status IN (...)` in the top-N rows-read consumers. Turso bills per row read, so per-tick aggregate scans dominate quota. The `task_action_counts(task_id, status, count)` table is maintained by `AFTER INSERT / AFTER UPDATE OF task_id, status / AFTER DELETE` triggers on `actions`, so any task's status counts are a 1-row index lookup. This rule structurally bans hot-path code from re-introducing aggregate scans — it is a specialization of Rule 11 narrowed to the most expensive aggregates.
+- Task reassignment: `tq action update --task` issues `UPDATE actions SET task_id = ?`, so `trg_actions_count_update` fires on `task_id` changes as well as `status` changes, moving the count between tasks. DBs whose trigger predates `task_id` coverage are healed on `db.Migrate()`: the stale trigger is dropped, recreated from the schema, and `task_action_counts` is rebuilt from `actions` (`dropStaleCountUpdateTrigger` / `rebuildTaskActionCounts` in `db/db.go`).
 - Backfill: `db.Migrate()` runs an idempotent `INSERT INTO task_action_counts SELECT task_id, status, COUNT(*) FROM actions GROUP BY task_id, status` only when `task_action_counts` is empty (`backfillTaskActionCounts` in `db/db.go`). Subsequent runs see existing rows kept in sync by the triggers and skip the backfill.
 - Verify: Go test harness `internal/goldenrules/` scans `tui/`, `dispatch/` for `"...SELECT COUNT|SUM|AVG..."` string literals. Ceiling-based: violations below the ceiling pass, regressions fail. Run `go test ./internal/goldenrules/`.
 - Detection limits (same as Rule 11/16): line-split (`"SELECT " + "COUNT(*)"`) and cross-literal concatenation (`prefix + " COUNT(*)"`) bypass detection. Reviewers MUST reject rewrites that exploit these to hide hot-path aggregates.