Stop failed reads from silently clobbering preserved save data

SaveAIFindingsWithSuppression and SavePatrolRunHistory read the existing
file to carry data forward (suppression rules, the daily run tally) and
treated a failed read as nothing to preserve, rewriting the file without
it. Same clobber mechanic as the system.json reset fixed in cf5b86e62.
Both loaders return empty data with no error for a missing file, so an
error at these sites is a real read failure.

Suppression rules are user-authored config, so that save now aborts and
returns the read error. The run tally is telemetry, so that save
proceeds but logs a warning that the tally restarts. Tests cover the
abort leaving the file intact, explicit rules saving despite a failing
read, and the tally warning plus restart.

internal/config/patrol_run_tally_test.go

@@ -1,6 +1,8 @@
 package config
 
 import (
+	"errors"
+	"strings"
 	"testing"
 	"time"
 )
@@ -97,3 +99,44 @@ func TestPatrolRunTallyPrunesOldDays(t *testing.T) {
 		t.Fatal("yesterday's tally day was pruned")
 	}
 }
+
+// A failed read of the existing history must not block the save — the tally is
+// telemetry — but the tally reset it causes has to be visible in the log, not
+// silent. (A genuinely missing file loads as empty data with no error, so it
+// never reaches the warning path.)
+func TestPatrolRunTallyReadErrorWarnsAndKeepsSaving(t *testing.T) {
+	p := NewConfigPersistence(t.TempDir())
+	now := time.Now().UTC()
+
+	// Seed history whose tally holds a run that has already fallen off the
+	// capped run list: only a preserved tally would still count it.
+	runA := PatrolRunRecord{ID: "a", StartedAt: now.Add(-2 * time.Hour), CompletedAt: now.Add(-2 * time.Hour)}
+	if err := p.SavePatrolRunHistory([]PatrolRunRecord{runA}); err != nil {
+		t.Fatalf("SavePatrolRunHistory seed: %v", err)
+	}
+
+	logs := captureConfigLogs(t)
+	mfs := &mockFSError{FileSystem: defaultFileSystem{}, readError: errors.New("transient read failure")}
+	p.SetFileSystem(mfs)
+
+	runB := PatrolRunRecord{ID: "b", StartedAt: now.Add(-1 * time.Hour), CompletedAt: now.Add(-1 * time.Hour)}
+	if err := p.SavePatrolRunHistory([]PatrolRunRecord{runB}); err != nil {
+		t.Fatalf("SavePatrolRunHistory with failing read must still save: %v", err)
+	}
+	if !strings.Contains(logs.String(), "daily run tally restarts") {
+		t.Fatalf("no tally-restart warning was logged; logs: %s", logs.String())
+	}
+
+	mfs.readError = nil
+	loaded, err := p.LoadPatrolRunHistory()
+	if err != nil {
+		t.Fatalf("LoadPatrolRunHistory: %v", err)
+	}
+	if len(loaded.Runs) != 1 || loaded.Runs[0].ID != "b" {
+		t.Fatalf("saved runs = %+v, want just run b", loaded.Runs)
+	}
+	// The tally restarted from the saved window: run a is gone, run b counted.
+	if got := loaded.PatrolRunsSince(now.AddDate(0, 0, -30)); got != 1 {
+		t.Fatalf("PatrolRunsSince = %d, want 1 after the tally restart", got)
+	}
+}

internal/config/persistence.go

@@ -2480,9 +2480,16 @@ func (c *ConfigPersistence) SaveAIFindings(findings map[string]*AIFindingRecord)
 // SaveAIFindingsWithSuppression persists AI findings + explicit suppression rules to disk.
 // If suppressionRules is nil, existing suppression rules (if any) are preserved.
 func (c *ConfigPersistence) SaveAIFindingsWithSuppression(findings map[string]*AIFindingRecord, suppressionRules map[string]*AISuppressionRuleRecord) error {
-	// Preserve suppression rules if caller didn't provide them.
+	// Preserve suppression rules if caller didn't provide them. A missing file
+	// loads as empty data with no error, so a load error here is a real read
+	// failure: abort rather than rewrite the file without the user-authored
+	// rules it still holds.
 	if suppressionRules == nil {
-		if existing, err := c.LoadAIFindings(); err == nil && existing != nil && existing.SuppressionRules != nil {
+		existing, err := c.LoadAIFindings()
+		if err != nil {
+			return fmt.Errorf("load existing AI findings to preserve suppression rules: %w", err)
+		}
+		if existing != nil && existing.SuppressionRules != nil {
 			suppressionRules = existing.SuppressionRules
 		}
 	}
@@ -3588,7 +3595,12 @@ func (c *ConfigPersistence) SavePatrolRunHistory(runs []PatrolRunRecord) error {
 		LastSaved: now,
 		Runs:      runs,
 	}
-	if existing, err := c.LoadPatrolRunHistory(); err == nil && existing != nil {
+	// A missing file loads as empty data with no error, so a load error here is
+	// a real read failure. The tally is telemetry, so keep the save — but say
+	// the tally is restarting rather than resetting it silently.
+	if existing, err := c.LoadPatrolRunHistory(); err != nil {
+		log.Warn().Err(err).Str("file", c.aiPatrolRunsFile).Msg("Failed to read existing patrol run history; daily run tally restarts from this save")
+	} else if existing != nil {
 		data.DailyRuns = existing.DailyRuns
 		data.RunTallyThrough = existing.RunTallyThrough
 	}

internal/config/persistence_ai_suppression_test.go

@@ -1,6 +1,7 @@
 package config
 
 import (
+	"errors"
 	"testing"
 	"time"
 
@@ -45,3 +46,54 @@ func TestSaveAIFindings_PreservesSuppressionRulesWhenNotProvided(t *testing.T) {
 	require.Contains(t, loaded.SuppressionRules, "rule1")
 	require.Equal(t, "Ignore for now", loaded.SuppressionRules["rule1"].Description)
 }
+
+// A failed read of the existing findings file must abort the save: proceeding
+// would rewrite the file without the user-authored suppression rules it still
+// holds. (A genuinely missing file loads as empty data with no error, so it
+// never reaches this path.)
+func TestSaveAIFindings_ReadErrorAbortsInsteadOfDroppingSuppressionRules(t *testing.T) {
+	tempDir := t.TempDir()
+	cp := NewConfigPersistence(tempDir)
+
+	findings := map[string]*AIFindingRecord{
+		"f1": {ID: "f1", Title: "High CPU", DetectedAt: time.Now(), LastSeenAt: time.Now()},
+	}
+	rules := map[string]*AISuppressionRuleRecord{
+		"rule1": {ID: "rule1", ResourceID: "res-1", Description: "Ignore for now", CreatedAt: time.Now()},
+	}
+	require.NoError(t, cp.SaveAIFindingsWithSuppression(findings, rules))
+
+	mfs := &mockFSError{FileSystem: defaultFileSystem{}, readError: errors.New("transient read failure")}
+	cp.SetFileSystem(mfs)
+
+	err := cp.SaveAIFindings(findings)
+	require.Error(t, err)
+	require.ErrorContains(t, err, "transient read failure")
+
+	// The aborted save must have left the file untouched: once reads work
+	// again, the rules are still there.
+	mfs.readError = nil
+	loaded, err := cp.LoadAIFindings()
+	require.NoError(t, err)
+	require.Contains(t, loaded.SuppressionRules, "rule1")
+}
+
+// Explicit suppression rules need no read of the existing file, so a failing
+// read must not block that save.
+func TestSaveAIFindings_ExplicitRulesSaveDespiteReadError(t *testing.T) {
+	tempDir := t.TempDir()
+	cp := NewConfigPersistence(tempDir)
+
+	mfs := &mockFSError{FileSystem: defaultFileSystem{}, readError: errors.New("transient read failure")}
+	cp.SetFileSystem(mfs)
+
+	rules := map[string]*AISuppressionRuleRecord{
+		"rule1": {ID: "rule1", ResourceID: "res-1", Description: "Ignore for now", CreatedAt: time.Now()},
+	}
+	require.NoError(t, cp.SaveAIFindingsWithSuppression(nil, rules))
+
+	mfs.readError = nil
+	loaded, err := cp.LoadAIFindings()
+	require.NoError(t, err)
+	require.Contains(t, loaded.SuppressionRules, "rule1")
+}