fix(settings): AI, Gmail og kalenderen delte én række og slettede hinanden

Meldt ind som "AI-informationerne gemmes ikke". De blev gemt. De blev taget
bagefter.

`user_settings` blev oprettet med `user_id` som hele sin primærnøgle — én række
per person, uanset scope — og upserten skrev `scope` om ved konflikt. Der er tre
scopes i live: `ai`, `gmail` og `calendar`. De delte den ene række, så hver
skrivning slettede de to andre, og hver eneste svarede 204. Det virker begge
veje: at forbinde kalenderen slettede Gmail-forbindelsen, og Gmail slettede
kalenderen.

Fælden var set komme to gange. 0011 gav instansens indstillinger deres egen
tabel delvis af den grund og skrev advarslen ned med rene ord; 0014 flyttede
postkasserne ud og skrev, at advarslen "nær var trådt i for anden gang". Tredje
gang var kalenderen, som lagde et `calendar`-scope ved siden af de to andre.

Lektien værd at holde fast i: en advarsel i en migration bliver læst af den, der
redigerer *den* migration, og det var ingen. Formen er rettet her, så der ikke er
noget tilbage at advare om — `PRIMARY KEY (user_id, scope)`, og upserten rammer
den nøgle.

Tabellen bygges om, fordi SQLite ikke kan ændre en primærnøgle på plads. De rækker,
der findes, kopieres som de er: hver person har højst én, og den holder det scope,
der vandt sidst. Det, de tidligere skrivninger tog, er overskrevet og kan ikke
hentes tilbage her — efter opdateringen skal AI-udbyderen sættes op igen, og
Gmail-forbindelsen tjekkes.

To prøver: én mod store-laget, hvor formen var fejlen, og én gennem handlerne,
hvor den blev set.

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

internal/httpapi/integrations_test.go

@@ -562,6 +562,48 @@ func TestAIKeyIsNeverReturned(t *testing.T) {
 	}
 }
 
+// Connecting something else must not delete the AI provider.
+//
+// Reported from use as "the AI settings are not saved": they were saved, and then
+// taken. `user_settings` had `user_id` as its whole primary key, and the upsert
+// renamed `scope` on conflict — so the `ai`, `gmail` and `calendar` scopes shared
+// one row and each write deleted the other two. Every write returned 204.
+//
+// Through the handlers, because that is where it was seen. The shape is asserted
+// against the store as well: see TestOnePersonCanHaveSettingsForMoreThanOneThing.
+func TestTheAIProviderSurvivesConnectingSomethingElse(t *testing.T) {
+	ts := newTestServer(t)
+	ts.bootstrap(t)
+
+	ts.do(t, "PUT", "/api/v1/ai/settings", map[string]any{
+		"provider": "anthropic", "model": "claude-sonnet-5", "api_key": "sk-hemmelig-nøgle",
+	})
+
+	user, err := ts.db.UserByEmail(t.Context(), "kristian@example.dk")
+	if err != nil {
+		t.Fatal(err)
+	}
+	// What the calendar and the mailbox handlers do when a handshake starts: write
+	// their own scope for the same person.
+	for _, scope := range []string{"calendar", "gmail"} {
+		if err := ts.db.SetUserSettings(t.Context(), user.ID, scope,
+			map[string]any{"pkce_state": "s-" + scope}); err != nil {
+			t.Fatalf("save %s: %v", scope, err)
+		}
+	}
+
+	_, body := ts.do(t, "GET", "/api/v1/ai/settings", nil)
+	if body["provider"] != "anthropic" {
+		t.Errorf("the provider is %v after connecting two other things, want anthropic", body["provider"])
+	}
+	if body["model"] != "claude-sonnet-5" {
+		t.Errorf("the model is %v", body["model"])
+	}
+	if body["has_key"] != true {
+		t.Error("the key was taken with it")
+	}
+}
+
 func TestGmailSettings(t *testing.T) {
 	ts := newTestServer(t)
 	ts.bootstrap(t)

internal/store/mailboxes_test.go

@@ -195,10 +195,14 @@ func TestAGmailConnectedBeforeTheMoveSurvivesIt(t *testing.T) {
 	}
 	values := `{"refresh_token":"` + sealed + `","trigger":"label","label":"Verdande",` +
 		`"email":"kw@nolimit.dk","seen":["abc","def"],"project_id":"p-1"}`
+	// The conflict target is (user_id, scope) since 0025 — before that the table
+	// held one row per person and the upsert renamed its scope, which is the bug
+	// 0025 exists to undo. Nothing conflicts here; the clause only has to be legal
+	// against the schema this test is running on.
 	if _, err := db.ExecContext(context.Background(), `
 		INSERT INTO user_settings (user_id, scope, values_json, updated_at)
 		VALUES (?, 'gmail', ?, 0)
-		ON CONFLICT (user_id) DO UPDATE SET scope = 'gmail', values_json = excluded.values_json`,
+		ON CONFLICT (user_id, scope) DO UPDATE SET values_json = excluded.values_json`,
 		userID, values); err != nil {
 		t.Fatal(err)
 	}

internal/store/migrations/0025_user_settings_per_scope.sql

@@ -0,0 +1,37 @@
+-- One row per person *and scope*, which is what the table always meant.
+--
+-- `user_settings` was created with `user_id` as its whole primary key, and its
+-- upsert rewrote `scope` on conflict. So a person had exactly one settings row,
+-- and whichever scope was written last was the only one that survived: saving an
+-- AI provider deleted the Gmail connection, and connecting a calendar deleted the
+-- AI provider. Nothing reported it — the write succeeded, and the loss was in the
+-- row it landed on.
+--
+-- This was seen coming twice. Migration 0011 gave the instance's settings their
+-- own table partly for this reason and wrote the warning down in as many words;
+-- 0014 moved mailboxes out and said the warning "was nearly walked into a second
+-- time". The third time was the calendar, which added a `calendar` scope beside
+-- `ai` and `gmail` — and walked in.
+--
+-- The lesson worth keeping: a warning in a migration is read by whoever is editing
+-- *that* migration, and nobody was. The shape is fixed here so there is nothing
+-- left to warn about.
+--
+-- SQLite cannot change a primary key in place, so the table is rebuilt. Existing
+-- rows are copied as they are: each person has at most one, and it holds whatever
+-- scope won last. What the earlier writes lost is already gone and cannot be
+-- recovered here — the values were overwritten, not orphaned.
+CREATE TABLE user_settings_new (
+    user_id     TEXT NOT NULL REFERENCES users (id) ON DELETE CASCADE,
+    scope       TEXT NOT NULL DEFAULT 'general',
+    values_json TEXT NOT NULL DEFAULT '{}',
+    updated_at  INTEGER NOT NULL,
+    PRIMARY KEY (user_id, scope)
+);
+
+INSERT INTO user_settings_new (user_id, scope, values_json, updated_at)
+SELECT user_id, scope, values_json, updated_at FROM user_settings;
+
+DROP TABLE user_settings;
+
+ALTER TABLE user_settings_new RENAME TO user_settings;

internal/store/notifications.go

@@ -391,11 +391,15 @@ func (db *DB) SetUserSettings(ctx context.Context, userID, scope string, values
 	if err != nil {
 		return err
 	}
+	// On (user_id, scope), which is the key since 0025. It used to be on `user_id`
+	// alone and set `scope = excluded.scope` — so writing one scope did not add a
+	// row beside the others, it took theirs and renamed it. The `ai`, `gmail` and
+	// `calendar` settings each deleted the other two, and the write reported
+	// success every time.
 	_, err = db.ExecContext(ctx, `
 		INSERT INTO user_settings (user_id, scope, values_json, updated_at)
 		VALUES (?, ?, ?, ?)
-		ON CONFLICT (user_id) DO UPDATE SET
+		ON CONFLICT (user_id, scope) DO UPDATE SET
-		    scope = excluded.scope,
 		    values_json = excluded.values_json,
 		    updated_at = excluded.updated_at`,
 		userID, scope, string(raw), time.Now().Unix())

internal/store/secrets_test.go

@@ -124,3 +124,65 @@ func sealedStore(t *testing.T) (*DB, string) {
 	}
 	return db, user.ID
 }
+
+// Three integrations, one person, and none of them may delete the others.
+//
+// `user_settings` had `user_id` as its whole primary key and an upsert that
+// rewrote `scope` on conflict, so every person had exactly one settings row and
+// the last write won the lot: saving an AI provider deleted the Gmail connection,
+// and connecting a calendar deleted the AI provider. The write reported success
+// each time, which is why it was reported as "the AI settings are not saved"
+// rather than as "connecting the calendar deleted them".
+//
+// Written against the store rather than the handlers, because the shape was the
+// bug: every caller was correct on its own.
+func TestOnePersonCanHaveSettingsForMoreThanOneThing(t *testing.T) {
+	db, userID := sealedStore(t)
+	ctx := context.Background()
+
+	want := map[string]map[string]any{
+		"ai":       {"provider": "anthropic", "model": "claude-sonnet-5", "api_key": "sk-ant-xxx"},
+		"gmail":    {"refresh_token": "1//0-gmail", "label": "Verdande"},
+		"calendar": {"refresh_token": "1//0-calendar", "calendar_id": "primary"},
+	}
+	// In this order on purpose: the AI settings are written first and read last,
+	// so a scope that steals the row is caught by the one written earliest.
+	for _, scope := range []string{"ai", "gmail", "calendar"} {
+		if err := db.SetUserSettings(ctx, userID, scope, want[scope]); err != nil {
+			t.Fatalf("save %s: %v", scope, err)
+		}
+	}
+
+	for _, scope := range []string{"ai", "gmail", "calendar"} {
+		got, err := db.UserSettings(ctx, userID, scope)
+		if err != nil {
+			t.Fatalf("read %s: %v", scope, err)
+		}
+		for key, value := range want[scope] {
+			if got[key] != value {
+				t.Errorf("%s.%s is %v, want %v", scope, key, got[key], value)
+			}
+		}
+	}
+
+	// And a second write to one scope updates it rather than adding another row —
+	// the upsert still has to upsert.
+	if err := db.SetUserSettings(ctx, userID, "ai", map[string]any{"provider": "openai"}); err != nil {
+		t.Fatal(err)
+	}
+	var rows int
+	if err := db.QueryRowContext(ctx,
+		`SELECT count(*) FROM user_settings WHERE user_id = ?`, userID).Scan(&rows); err != nil {
+		t.Fatal(err)
+	}
+	if rows != 3 {
+		t.Errorf("the person has %d settings rows, want 3", rows)
+	}
+	back, err := db.UserSettings(ctx, userID, "ai")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if back["provider"] != "openai" {
+		t.Errorf("the second write did not land: %v", back["provider"])
+	}
+}