-
Notifications
You must be signed in to change notification settings - Fork 89
fix(providers): backfill missing label = name on existing connections #30412
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -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"; | ||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚩 Checkpoint key will trigger 'unknown checkpoint' warning in validateMigrationState The checkpoint key However, this is a pre-existing pattern — several recent migrations (239, 242, 245) also use Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| 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; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
|
Comment on lines
+42
to
+49
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Premature checkpoint when table is absent prevents future backfill after migration 243 recovers When the Contrast with the label-column guard at lines 51-59: when the column is absent, the migration correctly does not set a checkpoint, so it can retry on the next boot once migration 244 has run. The table-doesn't-exist case should follow the same pattern — skip without checkpointing — so the backfill runs once 243 eventually succeeds.
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| // 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; | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| } | ||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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<typeof createTestDb>): 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<typeof createTestDb>, | ||
| ): 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(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This migration writes
backfill_provider_connection_labeltomemory_checkpoints, but no corresponding entry was added toassistant/src/memory/migrations/registry.ts.validateMigrationState()treatsbackfill_*keys that are absent fromMIGRATION_REGISTRYas unknown "newer version" checkpoints, so after this migration runs the daemon will emit a persistent incompatibility warning on every startup and rollback tooling will not know about this migration. Add a registry entry (withdown) for this checkpoint to keep validation and migration metadata consistent.Useful? React with 👍 / 👎.