diff --git a/assistant/src/memory/db-init.ts b/assistant/src/memory/db-init.ts index 047b1badb12..9de83d08a12 100644 --- a/assistant/src/memory/db-init.ts +++ b/assistant/src/memory/db-init.ts @@ -47,6 +47,7 @@ import { migrateBackfillContactInteractionStats, migrateBackfillGuardianPrincipalId, migrateBackfillInlineAttachmentsToDisk, + migrateBackfillProviderConnectionLabel, migrateBackfillUsageCacheAccounting, migrateCallSessionInviteMetadata, migrateCallSessionMode, @@ -422,6 +423,7 @@ export function initializeDb(): void { migrateCreateProviderConnections, migrateProviderConnectionStatusLabel, migrateMemoryRetrospectiveState, + migrateBackfillProviderConnectionLabel, ]; // Run each migration step, catching and logging individual failures so one diff --git a/assistant/src/memory/migrations/246-backfill-provider-connection-label.ts b/assistant/src/memory/migrations/246-backfill-provider-connection-label.ts new file mode 100644 index 00000000000..e351e089640 --- /dev/null +++ b/assistant/src/memory/migrations/246-backfill-provider-connection-label.ts @@ -0,0 +1,81 @@ +import { type DrizzleDb, getSqliteFrom } from "../db-connection.js"; + +/** + * One-shot backfill: set `label = name` for any `provider_connections` row + * whose `label` is NULL (or empty after trim). Migration 244 added the + * `label` column as nullable with no default, so every row that existed + * pre-244 (and any row created against an older client that didn't yet + * write a label) ended up with NULL. + * + * Why this matters for users: in 0.8.x the editor modal renders Display + * Name from `label` and shows the empty-state placeholder ("e.g. My + * OpenAI") whenever it's null. Users who created connections before the + * label field existed open the editor and see what looks like an empty + * field, then conclude their data was wiped during the upgrade. + * + * The fix is to give every pre-feature row a sensible default — its own + * connection name. The list view already falls back to `name` when label + * is empty (see ProvidersSheet.swift `connectionRow`), so this brings the + * editor into agreement with what's already on screen. + * + * Idempotency: guarded by `memory_checkpoints` so a second boot won't + * re-clobber a label the user has intentionally cleared after the + * backfill ran. New rows created post-backfill follow the normal + * label-is-optional contract. + */ +export function migrateBackfillProviderConnectionLabel(database: DrizzleDb): void { + const raw = getSqliteFrom(database); + const checkpointKey = "backfill_provider_connection_label"; + + const checkpoint = raw + .query(`SELECT 1 FROM memory_checkpoints WHERE key = ?`) + .get(checkpointKey); + if (checkpoint) return; + + // Guard: skip if the table doesn't exist yet (first-boot edge case where + // migration 243 hasn't run, e.g. fresh install ordering test harness). + const tableExists = raw + .query( + `SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = 'provider_connections'`, + ) + .get(); + if (!tableExists) { + raw + .query( + `INSERT OR IGNORE INTO memory_checkpoints (key, value, updated_at) VALUES (?, '1', ?)`, + ) + .run(checkpointKey, Date.now()); + return; + } + + // Guard: also skip if the label column hasn't been added yet (migration + // 244 hasn't run). Treat as no-op + don't checkpoint, so the backfill + // runs the next time it gets a chance against a populated schema. + const columns = raw + .query(`PRAGMA table_info(provider_connections)`) + .all() as Array<{ name: string }>; + if (!columns.some((c) => c.name === "label")) { + return; + } + + try { + raw.exec("BEGIN"); + + raw.exec(/*sql*/ ` + UPDATE provider_connections + SET label = name + WHERE label IS NULL OR TRIM(label) = '' + `); + + raw + .query( + `INSERT OR IGNORE INTO memory_checkpoints (key, value, updated_at) VALUES (?, '1', ?)`, + ) + .run(checkpointKey, Date.now()); + + raw.exec("COMMIT"); + } catch (err) { + raw.exec("ROLLBACK"); + throw err; + } +} diff --git a/assistant/src/memory/migrations/__tests__/246-backfill-provider-connection-label.test.ts b/assistant/src/memory/migrations/__tests__/246-backfill-provider-connection-label.test.ts new file mode 100644 index 00000000000..c5a97334328 --- /dev/null +++ b/assistant/src/memory/migrations/__tests__/246-backfill-provider-connection-label.test.ts @@ -0,0 +1,192 @@ +import { Database } from "bun:sqlite"; +import { describe, expect, test } from "bun:test"; + +import { drizzle } from "drizzle-orm/bun-sqlite"; + +import { getSqliteFrom } from "../../db-connection.js"; +import * as schema from "../../schema.js"; +import { migrateCreateProviderConnections } from "../243-provider-connections.js"; +import { migrateProviderConnectionStatusLabel } from "../244-provider-connection-status-label.js"; +import { migrateBackfillProviderConnectionLabel } from "../246-backfill-provider-connection-label.js"; + +interface ConnectionRow { + name: string; + label: string | null; +} + +function createTestDb() { + const sqlite = new Database(":memory:"); + sqlite.exec("PRAGMA journal_mode=WAL"); + sqlite.exec("PRAGMA foreign_keys = ON"); + return drizzle(sqlite, { schema }); +} + +/// Ensure the `memory_checkpoints` table exists in test DBs — it's normally +/// provisioned by earlier infra migrations that the test harness doesn't +/// pull in. Schema mirrors what other backfill migrations expect. +function ensureCheckpointsTable(db: ReturnType): void { + const raw = getSqliteFrom(db); + raw.exec(` + CREATE TABLE IF NOT EXISTS memory_checkpoints ( + key TEXT PRIMARY KEY, + value TEXT, + updated_at INTEGER + ) + `); +} + +/// Bring the test DB up to the state right before migration 246 runs: +/// provider_connections table present + status/label columns added. +function prepareSchemaThroughMigration244( + db: ReturnType, +): void { + migrateCreateProviderConnections(db); + migrateProviderConnectionStatusLabel(db); + ensureCheckpointsTable(db); +} + +describe("migration 246 — backfill provider_connection label", () => { + test("sets label = name for rows where label is NULL", () => { + const db = createTestDb(); + const raw = getSqliteFrom(db); + prepareSchemaThroughMigration244(db); + + // Insert a row with label = NULL (simulating a pre-244 connection). + const now = Date.now(); + raw + .query( + `INSERT INTO provider_connections (name, provider, auth, status, label, created_at, updated_at) VALUES (?, ?, ?, 'active', NULL, ?, ?)`, + ) + .run("anthropic-personal", "anthropic", JSON.stringify({ type: "api_key", credential: "credential/anthropic/api_key" }), now, now); + + migrateBackfillProviderConnectionLabel(db); + + const row = raw + .query(`SELECT name, label FROM provider_connections WHERE name = ?`) + .get("anthropic-personal") as ConnectionRow; + expect(row.label).toBe("anthropic-personal"); + }); + + test("sets label = name for rows where label is empty/whitespace", () => { + const db = createTestDb(); + const raw = getSqliteFrom(db); + prepareSchemaThroughMigration244(db); + + const now = Date.now(); + raw + .query( + `INSERT INTO provider_connections (name, provider, auth, status, label, created_at, updated_at) VALUES (?, ?, ?, 'active', ?, ?, ?)`, + ) + .run("openai-empty", "openai", JSON.stringify({ type: "api_key", credential: "credential/openai/api_key" }), "", now, now); + raw + .query( + `INSERT INTO provider_connections (name, provider, auth, status, label, created_at, updated_at) VALUES (?, ?, ?, 'active', ?, ?, ?)`, + ) + .run("openai-whitespace", "openai", JSON.stringify({ type: "api_key", credential: "credential/openai/api_key" }), " ", now, now); + + migrateBackfillProviderConnectionLabel(db); + + const empty = raw + .query(`SELECT label FROM provider_connections WHERE name = ?`) + .get("openai-empty") as ConnectionRow; + const whitespace = raw + .query(`SELECT label FROM provider_connections WHERE name = ?`) + .get("openai-whitespace") as ConnectionRow; + expect(empty.label).toBe("openai-empty"); + expect(whitespace.label).toBe("openai-whitespace"); + }); + + test("does NOT overwrite rows where the user has set a non-empty label", () => { + const db = createTestDb(); + const raw = getSqliteFrom(db); + prepareSchemaThroughMigration244(db); + + const now = Date.now(); + raw + .query( + `INSERT INTO provider_connections (name, provider, auth, status, label, created_at, updated_at) VALUES (?, ?, ?, 'active', ?, ?, ?)`, + ) + .run("anthropic-work", "anthropic", JSON.stringify({ type: "api_key", credential: "credential/anthropic/api_key" }), "Anthropic — Work", now, now); + + migrateBackfillProviderConnectionLabel(db); + + const row = raw + .query(`SELECT label FROM provider_connections WHERE name = ?`) + .get("anthropic-work") as ConnectionRow; + expect(row.label).toBe("Anthropic — Work"); + }); + + test("is idempotent — second run does not re-clobber a later user-cleared label", () => { + const db = createTestDb(); + const raw = getSqliteFrom(db); + prepareSchemaThroughMigration244(db); + + const now = Date.now(); + raw + .query( + `INSERT INTO provider_connections (name, provider, auth, status, label, created_at, updated_at) VALUES (?, ?, ?, 'active', NULL, ?, ?)`, + ) + .run("anthropic-personal", "anthropic", JSON.stringify({ type: "api_key", credential: "credential/anthropic/api_key" }), now, now); + + // First run: backfills NULL → "anthropic-personal". + migrateBackfillProviderConnectionLabel(db); + expect( + (raw.query(`SELECT label FROM provider_connections WHERE name = ?`).get("anthropic-personal") as ConnectionRow).label, + ).toBe("anthropic-personal"); + + // User clears the label after the backfill ran. + raw.query(`UPDATE provider_connections SET label = NULL WHERE name = ?`).run("anthropic-personal"); + + // Second run: should NOT re-clobber the user's deliberate clear, because + // the checkpoint was set on the first successful run. + migrateBackfillProviderConnectionLabel(db); + expect( + (raw.query(`SELECT label FROM provider_connections WHERE name = ?`).get("anthropic-personal") as ConnectionRow).label, + ).toBeNull(); + }); + + test("checkpoint is set after a successful run", () => { + const db = createTestDb(); + const raw = getSqliteFrom(db); + prepareSchemaThroughMigration244(db); + + migrateBackfillProviderConnectionLabel(db); + + const checkpoint = raw + .query(`SELECT key FROM memory_checkpoints WHERE key = ?`) + .get("backfill_provider_connection_label") as { key: string } | null; + expect(checkpoint).not.toBeNull(); + expect(checkpoint?.key).toBe("backfill_provider_connection_label"); + }); + + test("no-op when provider_connections table is absent (first-boot edge)", () => { + const db = createTestDb(); + ensureCheckpointsTable(db); + + // Don't run 243/244 — the table doesn't exist yet. + expect(() => migrateBackfillProviderConnectionLabel(db)).not.toThrow(); + + // Checkpoint should still get set so we don't retry forever. + const raw = getSqliteFrom(db); + const checkpoint = raw + .query(`SELECT key FROM memory_checkpoints WHERE key = ?`) + .get("backfill_provider_connection_label"); + expect(checkpoint).not.toBeNull(); + }); + + test("no-op when label column is absent (244 hasn't run yet) — no checkpoint set", () => { + const db = createTestDb(); + const raw = getSqliteFrom(db); + migrateCreateProviderConnections(db); // 243 only, NOT 244 + ensureCheckpointsTable(db); + + expect(() => migrateBackfillProviderConnectionLabel(db)).not.toThrow(); + + // Crucially: no checkpoint should be set, so the backfill can run later + // once 244 has provisioned the column. + const checkpoint = raw + .query(`SELECT key FROM memory_checkpoints WHERE key = ?`) + .get("backfill_provider_connection_label"); + expect(checkpoint).toBeNull(); + }); +}); diff --git a/assistant/src/memory/migrations/index.ts b/assistant/src/memory/migrations/index.ts index e038f477fd6..3bb81f47f6b 100644 --- a/assistant/src/memory/migrations/index.ts +++ b/assistant/src/memory/migrations/index.ts @@ -207,6 +207,7 @@ export { migrateMessageBookmarks } from "./242-message-bookmarks.js"; export { migrateCreateProviderConnections } from "./243-provider-connections.js"; export { migrateProviderConnectionStatusLabel } from "./244-provider-connection-status-label.js"; export { migrateMemoryRetrospectiveState } from "./245-memory-retrospective-state.js"; +export { migrateBackfillProviderConnectionLabel } from "./246-backfill-provider-connection-label.js"; export { MIGRATION_REGISTRY, type MigrationRegistryEntry,