fix: address code review findings

- Fix notifications_sent schema: change to composite PK (rgid, sent_at) per plan spec, remove FK constraint
- Change MarkNotificationSent from INSERT OR REPLACE to INSERT (composite PK semantics)
- Update test: replace idempotent test with duplicate-second and different-time tests
- Refactor UpdateArtistSettings to use switch-based column validation instead of fmt.Sprintf with map lookup
- Remove generated coverage.out from repo, add to .gitignore

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

internal/database/artist_settings.go

@@ -57,33 +57,48 @@ func GetAllArtistSettings(db *DB) ([]ArtistSettings, error) {
 // UpdateArtistSettings updates specific fields of an artist_settings row by ID.
 // The updates map keys must match column names: "name", "ignore_singles", "ignore_compilations", "monitored".
 func UpdateArtistSettings(db *DB, id string, updates map[string]interface{}) error {
-	// Build the SET clause dynamically from the provided updates.
-	allowed := map[string]bool{
-		"name":                true,
-		"ignore_singles":      true,
-		"ignore_compilations": true,
-		"monitored":           true,
+	if len(updates) == 0 {
+		return fmt.Errorf("no updates provided")
 	}
 
+	// Build the query using a fixed set of allowed columns to avoid dynamic SQL.
+	const baseQuery = "UPDATE artist_settings SET"
+
 	var args []interface{}
 	setClause := ""
 	for col, val := range updates {
-		if !allowed[col] {
+		switch col {
+		case "name":
+			if setClause != "" {
+				setClause += ", "
+			}
+			setClause += "name = ?"
+			args = append(args, val)
+		case "ignore_singles":
+			if setClause != "" {
+				setClause += ", "
+			}
+			setClause += "ignore_singles = ?"
+			args = append(args, val)
+		case "ignore_compilations":
+			if setClause != "" {
+				setClause += ", "
+			}
+			setClause += "ignore_compilations = ?"
+			args = append(args, val)
+		case "monitored":
+			if setClause != "" {
+				setClause += ", "
+			}
+			setClause += "monitored = ?"
+			args = append(args, val)
+		default:
 			return fmt.Errorf("unknown column: %s", col)
 		}
-		if setClause != "" {
-			setClause += ", "
-		}
-		setClause += col + " = ?"
-		args = append(args, val)
-	}
-
-	if len(args) == 0 {
-		return fmt.Errorf("no updates provided")
 	}
 
 	args = append(args, id)
-	query := fmt.Sprintf("UPDATE artist_settings SET %s WHERE id = ?", setClause)
+	query := fmt.Sprintf("%s %s WHERE id = ?", baseQuery, setClause)
 	result, err := db.Conn().Exec(query, args...)
 	if err != nil {
 		return fmt.Errorf("update artist settings: %w", err)

internal/database/database.go

@@ -96,8 +96,9 @@ func (db *DB) migrate() error {
 		{
 			name: "003_create_notifications_sent",
 			sql: `CREATE TABLE IF NOT EXISTS notifications_sent (
-				rgid TEXT PRIMARY KEY REFERENCES external_releases(rgid),
-				sent_at DATETIME DEFAULT CURRENT_TIMESTAMP
+				rgid TEXT NOT NULL,
+				sent_at DATETIME DEFAULT CURRENT_TIMESTAMP,
+				PRIMARY KEY (rgid, sent_at)
 			);`,
 		},
 	}

internal/database/notifications.go

@@ -7,7 +7,7 @@ import (
 // MarkNotificationSent records that a notification has been sent for the given RGID.
 func MarkNotificationSent(db *DB, rgid string) error {
 	_, err := db.Conn().Exec(
-		"INSERT OR REPLACE INTO notifications_sent (rgid) VALUES (?)",
+		"INSERT INTO notifications_sent (rgid) VALUES (?)",
 		rgid,
 	)
 	if err != nil {

internal/database/notifications_test.go

@@ -41,8 +41,10 @@ func TestMarkNotificationSent_New(t *testing.T) {
 	}
 }
 
-// TestMarkNotificationSent_Idempotent verifies that marking the same RGID twice does not fail.
-func TestMarkNotificationSent_Idempotent(t *testing.T) {
+// TestMarkNotificationSent_DuplicateSecond verifies that inserting the same RGID twice
+// within the same second fails due to the composite primary key (rgid, sent_at).
+// In practice, notifications are sent at most once per day, so this is acceptable.
+func TestMarkNotificationSent_DuplicateSecond(t *testing.T) {
 	db, err := New(":memory:")
 	if err != nil {
 		t.Fatalf("New() error: %v", err)
@@ -58,8 +60,10 @@ func TestMarkNotificationSent_Idempotent(t *testing.T) {
 	if err := MarkNotificationSent(db, "rgid-1"); err != nil {
 		t.Fatalf("first MarkNotificationSent() error: %v", err)
 	}
-	if err := MarkNotificationSent(db, "rgid-1"); err != nil {
-		t.Fatalf("second MarkNotificationSent() error: %v", err)
+	// Second insert in the same second should fail with a UNIQUE constraint error.
+	err = MarkNotificationSent(db, "rgid-1")
+	if err == nil {
+		t.Fatal("expected UNIQUE constraint error on duplicate insert, got nil")
 	}
 
 	// Should still have exactly one row.
@@ -73,6 +77,52 @@ func TestMarkNotificationSent_Idempotent(t *testing.T) {
 	}
 }
 
+// TestMarkNotificationSent_DifferentTime verifies that inserting the same RGID at a
+// different explicit sent_at time succeeds (composite PK allows multiple rows per RGID).
+func TestMarkNotificationSent_DifferentTime(t *testing.T) {
+	db, err := New(":memory:")
+	if err != nil {
+		t.Fatalf("New() error: %v", err)
+	}
+	defer db.Close()
+
+	if err := insertTestArtist(db, "artist-1"); err != nil {
+		t.Fatalf("insertTestArtist: %v", err)
+	}
+	if err := insertTestRelease(db, "rgid-1", "artist-1"); err != nil {
+		t.Fatalf("insertTestRelease: %v", err)
+	}
+
+	// Insert with explicit different timestamps.
+	_, err = db.Conn().Exec("INSERT INTO notifications_sent (rgid, sent_at) VALUES (?, ?)", "rgid-1", "2024-01-01T00:00:00Z")
+	if err != nil {
+		t.Fatalf("first insert error: %v", err)
+	}
+	_, err = db.Conn().Exec("INSERT INTO notifications_sent (rgid, sent_at) VALUES (?, ?)", "rgid-1", "2024-06-01T00:00:00Z")
+	if err != nil {
+		t.Fatalf("second insert error: %v", err)
+	}
+
+	// IsNotificationSent should return true.
+	sent, err := IsNotificationSent(db, "rgid-1")
+	if err != nil {
+		t.Fatalf("IsNotificationSent() error: %v", err)
+	}
+	if !sent {
+		t.Error("expected IsNotificationSent to return true")
+	}
+
+	// Should have two rows.
+	var count int
+	err = db.Conn().QueryRow("SELECT COUNT(*) FROM notifications_sent WHERE rgid = ?", "rgid-1").Scan(&count)
+	if err != nil {
+		t.Fatalf("count query error: %v", err)
+	}
+	if count != 2 {
+		t.Errorf("expected 2 notification rows, got %d", count)
+	}
+}
+
 // TestIsNotificationSent_True verifies true for a sent notification.
 func TestIsNotificationSent_True(t *testing.T) {
 	db, err := New(":memory:")