feat(builtin-models): reconcile YAML lifecycle with drift sweep

Extend the builtin_models.yaml loader so the YAML file becomes a complete source of truth for the rows it owns. Builds on the previous commit's managed_by column. Lifecycle contract: - Every UPSERTed entry is tagged managed_by="yaml". - The DoUpdates list now includes deleted_at, so an entry that was soft-deleted (e.g. via UI/API) is automatically resurrected when it reappears in the file. Closes the "ghost row that exists but is invisible" failure mode. - After all UPSERTs, the loader soft-deletes rows where managed_by='yaml' AND id NOT IN (current YAML id set). Removing an entry from YAML is now the supported way to retire a built-in model — no manual SQL needed. - Rows tagged managed_by='' (UI/API/SQL-seeded built-ins) are invisible to the reconcile path and never touched. - When a YAML entry sets is_default=true, the loader first clears is_default on any other rows in the same (tenant_id, type) bucket, mirroring the invariant enforced by the API path (repository.UnsetDefaultModel). Failure handling stays defensive: - File missing / not a regular file / parse error: warn and skip; the drift sweep is NOT executed so a malformed file cannot wipe rows. - Per-entry UPSERT error: warn, drop the id from the keep-set so the sweep also leaves the existing row alone ("leave alone on failure"). Tests cover: file-missing, parse-error, basic upsert + defaults, idempotency, ${ENV} interpolation (set vs unset), drift sweep removing YAML rows, drift sweep ignoring manual rows, soft-delete resurrection, is_default cleanup across tenant+type, explicit empty list sweeping all yaml-managed rows, and a regression guard ensuring BeforeCreate does not overwrite YAML-supplied stable ids. Docs are rewritten so operators see "delete from YAML and restart" as the supported removal path; SQL is retained only for the legacy managed_by='' slice.
2026-06-04 13:30:32 +08:00 · 2026-05-26 11:20:29 +08:00
parent fdc22fd7a5
commit 7b192e546f
3 changed files with 417 additions and 21 deletions
--- a/docs/BUILTIN_MODELS.md
+++ b/docs/BUILTIN_MODELS.md
@@ -237,7 +237,18 @@ WHERE id = '模型ID';

 ## 移除内置模型

-YAML 配置删除条目后**不会**自动清理 DB 里的对应行（避免误删 ops 手工维护的内置模型）。如需删除：
+**从 YAML 删除条目即可。** 应用启动时会自动软删除 `models` 表中 YAML 不再声明的 YAML 托管行 —— 你不再需要手工跑 SQL。
+
+工作原理：每条由 YAML 写入的行会被打上 `managed_by = 'yaml'` 标记。重启时 loader 走两步：
+
+1. UPSERT 当前 YAML 中的所有条目（按 `id` 幂等，包含把之前软删过的 `deleted_at` 重置为 NULL —— 也就是说从 YAML 拿掉再加回来等于"复活"）
+2. 软删除 `is_builtin = true AND managed_by = 'yaml' AND id NOT IN (当前 YAML 中的 id 集合)` 的行
+
+**手工通过 SQL 插入的 builtin 行（`managed_by = ''`）永远不会被 loader 触碰**，与 YAML 完全隔离。
+
+### 手工路径补充
+
+如果你是走 SQL 路径管理的（`managed_by = ''`），删除仍然走老方法：

 ```sql
 -- 取消 builtin 标记，恢复为普通模型
@@ -247,11 +258,17 @@ UPDATE models SET is_builtin = false WHERE id = '模型ID';
 DELETE FROM models WHERE id = '模型ID';
 ```

+### 紧急关闭 YAML 接管
+
+如果误改了 YAML 想立刻停用接管又不想清空文件，最快的方法是：把环境变量 `BUILTIN_MODELS_CONFIG` 指向一个不存在的路径并重启 —— loader 看到文件缺失会直接 no-op，**包括跳过 drift sweep**，已经写入的 YAML 托管行保留原状。
+
 ## 注意事项

 1. **ID 命名规范**：建议使用 `builtin-{type}-{slug}` 的格式，例如 `builtin-openai-chat`、`builtin-rerank`
 2. **租户ID**：内置模型可以属于任意租户，默认 `10000`（与 `tenants_id_seq` 起点一致）
-3. **YAML 与 SQL 并存**：两种方式可以同时使用，YAML 不会覆盖通过 SQL 插入但未在 YAML 出现的 builtin 行
-4. **重启即生效**：修改 YAML 后 `docker compose restart app` 即可让新配置生效
-5. **加密**：API Key 在 `parameters` JSONB 中以加密形式存储（若 `SYSTEM_AES_KEY` 已配置），未配置时降级为明文兼容路径
-6. **安全性**：前端会自动隐藏内置模型的 API Key 和 Base URL，但数据库中的原始数据仍然存在，请妥善保管数据库访问权限
+3. **YAML 与 SQL 并存**：两种方式可以同时使用，loader 只动 `managed_by='yaml'` 的行；通过 SQL 插入的 builtin 行对 loader 完全不可见
+4. **`is_default` 单一保证**：YAML 中将某条 entry 标记 `is_default: true` 时，loader 会先把同 `(tenant_id, type)` 下的其它默认模型置为 `false`，避免 API 路径维护的"每类型一个默认模型"语义被破坏
+5. **重启即生效**：修改 YAML 后 `docker compose restart app` 即可让新配置生效
+6. **加密**：API Key 在 `parameters` JSONB 中以加密形式存储（若 `SYSTEM_AES_KEY` 已配置），未配置时降级为明文兼容路径
+7. **安全性**：前端会自动隐藏内置模型的 API Key 和 Base URL，但数据库中的原始数据仍然存在，请妥善保管数据库访问权限
+8. **解析错误自我保护**：YAML 解析失败时 loader 仅打 warning 并跳过 reconcile，**不会**执行 drift sweep，确保一个手抖的 YAML 改动不会大规模软删既有内置模型
--- a/internal/types/builtin_models_config.go
+++ b/internal/types/builtin_models_config.go
@@ -12,6 +12,12 @@ import (
 	"gorm.io/gorm/clause"
 )

+// BuiltinModelManagedBy is the value written into `models.managed_by` for
+// rows whose lifecycle is owned by config/builtin_models.yaml. Other rows
+// (UI / API / hand-written SQL) keep the column at its default empty string
+// and are never touched by the YAML loader.
+const BuiltinModelManagedBy = "yaml"
+
 // BuiltinModelEntry mirrors one entry in builtin_models.yaml.
 // Each entry becomes a row in the models table with is_builtin=true.
 type BuiltinModelEntry struct {
@@ -50,14 +56,34 @@ func interpolateBuiltinModelEnv(s string) string {
 }

 // LoadBuiltinModelsConfig reads builtin_models.yaml (or the path pointed to by
-// BUILTIN_MODELS_CONFIG) and UPSERTs each entry into the models table.
+// BUILTIN_MODELS_CONFIG) and reconciles the YAML-managed slice of `models`
+// with the file contents.
 //
-// Behaviour:
+// Lifecycle contract:
+//   - YAML loader only ever reads/writes rows tagged managed_by="yaml". Rows
+//     created via UI/API/SQL (managed_by="") are invisible to the loader and
+//     are never modified.
+//   - Each YAML entry is UPSERTed by id, with deleted_at force-reset to NULL
+//     (so a row that was soft-deleted previously is resurrected when it
+//     reappears in the file).
+//   - After all UPSERTs, any pre-existing yaml-managed row whose id is no
+//     longer in the file is soft-deleted. Removing an entry from YAML is
+//     therefore the supported way to retire a built-in model — no manual
+//     SQL needed.
+//   - If is_default=true is set on a YAML entry, the loader first clears
+//     is_default on any other rows in the same (tenant_id, type) bucket,
+//     mirroring the invariant enforced by the API path. Multiple entries
+//     with is_default=true for the same bucket result in last-one-wins with
+//     a warning.
+//
+// Failure handling:
 //   - file not found / mount point is a directory / path unset: no-op
-//   - YAML parse error: prints a warning, returns nil (never aborts startup)
-//   - per-entry UPSERT error: prints a warning, continues with the next entry
-//   - never deletes entries that disappeared from YAML: operators may have
-//     seeded extras manually via SQL; removing them here would be surprising
+//   - YAML parse error: prints a warning and aborts the reconcile (the
+//     drift sweep is NOT run, so a malformed file cannot accidentally wipe
+//     YAML-managed rows)
+//   - per-entry UPSERT error: prints a warning, the entry is dropped from
+//     the "current YAML id set" so the sweep won't delete its existing
+//     row either (treats the failure as "leave alone")
 func LoadBuiltinModelsConfig(ctx context.Context, db *gorm.DB, configDir string) error {
 	path := os.Getenv("BUILTIN_MODELS_CONFIG")
 	if path == "" {
@@ -83,16 +109,16 @@ func LoadBuiltinModelsConfig(ctx context.Context, db *gorm.DB, configDir string)

 	var file builtinModelsFile
 	if err := yaml.Unmarshal([]byte(expanded), &file); err != nil {
-		fmt.Printf("Warning: parse built-in models config %s failed: %v; skipping.\n", path, err)
-		return nil
-	}
-
-	if len(file.BuiltinModels) == 0 {
-		fmt.Printf("Built-in models config %s contains no entries.\n", path)
+		fmt.Printf("Warning: parse built-in models config %s failed: %v; skipping reconcile.\n", path, err)
 		return nil
 	}

+	// yamlIDs collects every id we successfully upserted this run. Used by
+	// the drift sweep below to determine which previously-yaml-managed rows
+	// have disappeared and should be retired.
+	yamlIDs := make([]string, 0, len(file.BuiltinModels))
 	applied := 0
+
 	for i := range file.BuiltinModels {
 		e := &file.BuiltinModels[i]
 		if e.ID == "" {
@@ -100,11 +126,27 @@ func LoadBuiltinModelsConfig(ctx context.Context, db *gorm.DB, configDir string)
 			continue
 		}
 		m := e.toModel()
+
+		// Mirror the API path's "single default per (tenant_id, type)"
+		// invariant: clear other defaults before promoting this one.
+		// Excluded our own id so we don't churn against ourselves.
+		if m.IsDefault {
+			if err := db.WithContext(ctx).
+				Model(&Model{}).
+				Where("tenant_id = ? AND type = ? AND id <> ? AND is_default = ?",
+					m.TenantID, m.Type, m.ID, true).
+				Update("is_default", false).Error; err != nil {
+				fmt.Printf("Warning: clear existing default for tenant=%d type=%s failed: %v; continuing.\n",
+					m.TenantID, m.Type, err)
+			}
+		}
+
 		res := db.WithContext(ctx).Clauses(clause.OnConflict{
 			Columns: []clause.Column{{Name: "id"}},
 			DoUpdates: clause.AssignmentColumns([]string{
 				"tenant_id", "name", "type", "source", "description",
-				"parameters", "is_default", "status", "is_builtin", "updated_at",
+				"parameters", "is_default", "status", "is_builtin",
+				"managed_by", "deleted_at", "updated_at",
 			}),
 		}).Create(&m)
 		if res.Error != nil {
@@ -112,16 +154,50 @@ func LoadBuiltinModelsConfig(ctx context.Context, db *gorm.DB, configDir string)
 			continue
 		}
 		applied++
+		yamlIDs = append(yamlIDs, e.ID)
 		fmt.Printf("Built-in model upserted: id=%s name=%s type=%s\n", e.ID, e.Name, e.Type)
 	}
-	fmt.Printf("Built-in models config applied: %d entries from %s.\n", applied, path)
+
+	// Drift sweep: retire YAML-managed rows that no longer appear in the
+	// file. Manual rows (managed_by='') are untouched. Uses GORM soft delete
+	// so the row remains recoverable.
+	pruned, sweepErr := pruneOrphanYAMLManagedModels(ctx, db, yamlIDs)
+	if sweepErr != nil {
+		fmt.Printf("Warning: drift sweep for YAML-managed models failed: %v; continuing.\n", sweepErr)
+	}
+
+	fmt.Printf("Built-in models config applied: %d upserted, %d pruned from %s.\n", applied, pruned, path)
 	return nil
 }

+// pruneOrphanYAMLManagedModels soft-deletes rows where managed_by='yaml'
+// and id is NOT in keepIDs. Used to retire entries that have been removed
+// from builtin_models.yaml. Returns the number of rows affected.
+//
+// Safety:
+//   - Only rows tagged managed_by='yaml' are eligible — manual rows are
+//     never visible to this query.
+//   - Already-soft-deleted rows are skipped (GORM default scope), so this
+//     is idempotent across restarts.
+//   - When keepIDs is empty the sweep retires ALL yaml-managed rows. That
+//     matches the natural reading of "the YAML file declares zero entries
+//     ⇒ no yaml-managed models should exist". The caller has already
+//     short-circuited on parse failure, so we only reach this branch when
+//     the operator deliberately reduced the file to an empty list.
+func pruneOrphanYAMLManagedModels(ctx context.Context, db *gorm.DB, keepIDs []string) (int64, error) {
+	q := db.WithContext(ctx).
+		Where("managed_by = ?", BuiltinModelManagedBy)
+	if len(keepIDs) > 0 {
+		q = q.Where("id NOT IN ?", keepIDs)
+	}
+	res := q.Delete(&Model{})
+	return res.RowsAffected, res.Error
+}
+
 // toModel converts a YAML entry to a runtime Model with sensible defaults.
 // tenant_id defaults to 10000 (matches the seed value of tenants_id_seq);
-// source defaults to "remote"; status defaults to "active". IsBuiltin is
-// always forced to true regardless of YAML input.
+// source defaults to "remote"; status defaults to "active". IsBuiltin and
+// ManagedBy are always forced regardless of YAML input.
 func (e *BuiltinModelEntry) toModel() Model {
 	tenantID := e.TenantID
 	if tenantID == 0 {
@@ -145,6 +221,7 @@ func (e *BuiltinModelEntry) toModel() Model {
 		Parameters:  e.Parameters,
 		IsDefault:   e.IsDefault,
 		IsBuiltin:   true,
+		ManagedBy:   BuiltinModelManagedBy,
 		Status:      status,
 	}
 }
--- a/internal/types/builtin_models_config_test.go
+++ b/internal/types/builtin_models_config_test.go
@@ -0,0 +1,302 @@
+package types
+
+import (
+	"context"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"gorm.io/driver/sqlite"
+	"gorm.io/gorm"
+)
+
+// setupBuiltinModelsDB creates an in-memory SQLite DB with `models` migrated
+// via GORM AutoMigrate. AutoMigrate honours the struct tags so the
+// `managed_by` and soft-delete columns are present, matching what the real
+// migrations produce.
+func setupBuiltinModelsDB(t *testing.T) *gorm.DB {
+	t.Helper()
+	db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{})
+	require.NoError(t, err)
+	require.NoError(t, db.AutoMigrate(&Model{}))
+	return db
+}
+
+// writeYAML writes a builtin_models.yaml file inside a fresh temp dir and
+// returns the dir (which is what LoadBuiltinModelsConfig expects as its
+// configDir argument).
+func writeYAML(t *testing.T, body string) string {
+	t.Helper()
+	dir := t.TempDir()
+	path := filepath.Join(dir, "builtin_models.yaml")
+	require.NoError(t, os.WriteFile(path, []byte(body), 0o644))
+	return dir
+}
+
+// countModels returns (live, soft-deleted) counts for the given id, used to
+// distinguish "row gone via UPSERT failure" from "row soft-deleted via sweep".
+func countModels(t *testing.T, db *gorm.DB, id string) (live, deleted int64) {
+	t.Helper()
+	require.NoError(t, db.Model(&Model{}).Where("id = ?", id).Count(&live).Error)
+	require.NoError(t,
+		db.Unscoped().Model(&Model{}).Where("id = ? AND deleted_at IS NOT NULL", id).Count(&deleted).Error,
+	)
+	return live, deleted
+}
+
+func TestLoadBuiltinModelsConfig_FileMissing(t *testing.T) {
+	db := setupBuiltinModelsDB(t)
+	dir := t.TempDir() // empty — no builtin_models.yaml inside
+
+	// Pre-seed a yaml-managed row to verify it is NOT touched when the
+	// config file is absent (file-missing path must skip the sweep).
+	require.NoError(t, db.Create(&Model{
+		ID: "pre-existing-yaml", Name: "n", Type: ModelTypeKnowledgeQA,
+		Source: ModelSourceRemote, Status: ModelStatusActive,
+		IsBuiltin: true, ManagedBy: BuiltinModelManagedBy,
+	}).Error)
+
+	require.NoError(t, LoadBuiltinModelsConfig(context.Background(), db, dir))
+
+	live, deleted := countModels(t, db, "pre-existing-yaml")
+	assert.Equal(t, int64(1), live, "row must remain when YAML file is absent")
+	assert.Equal(t, int64(0), deleted)
+}
+
+func TestLoadBuiltinModelsConfig_ParseError(t *testing.T) {
+	db := setupBuiltinModelsDB(t)
+	// Malformed YAML — should warn + return without sweeping.
+	dir := writeYAML(t, "builtin_models: [oops: : bad")
+
+	require.NoError(t, db.Create(&Model{
+		ID: "pre-existing-yaml", Name: "n", Type: ModelTypeKnowledgeQA,
+		Source: ModelSourceRemote, Status: ModelStatusActive,
+		IsBuiltin: true, ManagedBy: BuiltinModelManagedBy,
+	}).Error)
+
+	require.NoError(t, LoadBuiltinModelsConfig(context.Background(), db, dir))
+
+	live, deleted := countModels(t, db, "pre-existing-yaml")
+	assert.Equal(t, int64(1), live, "parse error must NOT trigger sweep")
+	assert.Equal(t, int64(0), deleted)
+}
+
+func TestLoadBuiltinModelsConfig_BasicUpsert(t *testing.T) {
+	db := setupBuiltinModelsDB(t)
+	dir := writeYAML(t, `builtin_models:
+  - id: builtin-llm
+    name: gpt-4o-mini
+    type: KnowledgeQA
+    is_default: true
+    parameters:
+      provider: openai
+      api_key: sk-123
+`)
+	require.NoError(t, LoadBuiltinModelsConfig(context.Background(), db, dir))
+
+	var m Model
+	require.NoError(t, db.Where("id = ?", "builtin-llm").First(&m).Error)
+	assert.Equal(t, "gpt-4o-mini", m.Name)
+	assert.Equal(t, ModelTypeKnowledgeQA, m.Type)
+	assert.Equal(t, ModelSourceRemote, m.Source, "default source must be remote")
+	assert.Equal(t, ModelStatusActive, m.Status, "default status must be active")
+	assert.Equal(t, uint64(10000), m.TenantID, "default tenant_id must be 10000")
+	assert.True(t, m.IsBuiltin, "IsBuiltin must be forced to true")
+	assert.True(t, m.IsDefault)
+	assert.Equal(t, BuiltinModelManagedBy, m.ManagedBy)
+	assert.Equal(t, "openai", m.Parameters.Provider)
+	assert.Equal(t, "sk-123", m.Parameters.APIKey)
+}
+
+func TestLoadBuiltinModelsConfig_Idempotent(t *testing.T) {
+	db := setupBuiltinModelsDB(t)
+	dir := writeYAML(t, `builtin_models:
+  - id: builtin-llm
+    name: gpt-4o-mini
+    type: KnowledgeQA
+    parameters:
+      provider: openai
+`)
+	require.NoError(t, LoadBuiltinModelsConfig(context.Background(), db, dir))
+	require.NoError(t, LoadBuiltinModelsConfig(context.Background(), db, dir))
+
+	var count int64
+	require.NoError(t, db.Model(&Model{}).Where("id = ?", "builtin-llm").Count(&count).Error)
+	assert.Equal(t, int64(1), count, "second load must not duplicate")
+}
+
+func TestLoadBuiltinModelsConfig_EnvInterpolation(t *testing.T) {
+	db := setupBuiltinModelsDB(t)
+	t.Setenv("BUILTIN_TEST_KEY", "sk-from-env")
+	dir := writeYAML(t, `builtin_models:
+  - id: builtin-llm
+    name: gpt-4o-mini
+    type: KnowledgeQA
+    parameters:
+      provider: openai
+      api_key: ${BUILTIN_TEST_KEY}
+      base_url: ${BUILTIN_TEST_UNSET}
+`)
+	require.NoError(t, LoadBuiltinModelsConfig(context.Background(), db, dir))
+
+	var m Model
+	require.NoError(t, db.Where("id = ?", "builtin-llm").First(&m).Error)
+	assert.Equal(t, "sk-from-env", m.Parameters.APIKey, "set env var must be substituted")
+	assert.Equal(t, "${BUILTIN_TEST_UNSET}", m.Parameters.BaseURL,
+		"unset env var must be kept as literal placeholder")
+}
+
+func TestLoadBuiltinModelsConfig_DriftSweepRemovesYAMLManaged(t *testing.T) {
+	db := setupBuiltinModelsDB(t)
+
+	// Round 1: YAML declares two entries.
+	dir := writeYAML(t, `builtin_models:
+  - id: builtin-llm
+    name: gpt-4o-mini
+    type: KnowledgeQA
+  - id: builtin-rerank
+    name: bge
+    type: Rerank
+`)
+	require.NoError(t, LoadBuiltinModelsConfig(context.Background(), db, dir))
+
+	// Round 2: rerank entry removed.
+	require.NoError(t, os.WriteFile(
+		filepath.Join(dir, "builtin_models.yaml"),
+		[]byte(`builtin_models:
+  - id: builtin-llm
+    name: gpt-4o-mini
+    type: KnowledgeQA
+`), 0o644,
+	))
+	require.NoError(t, LoadBuiltinModelsConfig(context.Background(), db, dir))
+
+	live, deleted := countModels(t, db, "builtin-rerank")
+	assert.Equal(t, int64(0), live, "rerank row must be soft-deleted")
+	assert.Equal(t, int64(1), deleted, "rerank row must remain recoverable in raw table")
+
+	live, _ = countModels(t, db, "builtin-llm")
+	assert.Equal(t, int64(1), live, "llm row must survive the sweep")
+}
+
+func TestLoadBuiltinModelsConfig_DriftSweepIgnoresManual(t *testing.T) {
+	db := setupBuiltinModelsDB(t)
+
+	// Seed a manual SQL-style builtin (managed_by="").
+	require.NoError(t, db.Create(&Model{
+		ID: "manual-builtin", Name: "manual", Type: ModelTypeKnowledgeQA,
+		Source: ModelSourceRemote, Status: ModelStatusActive,
+		IsBuiltin: true, ManagedBy: "",
+	}).Error)
+
+	// YAML declares a different id only. Sweep must NOT touch manual row.
+	dir := writeYAML(t, `builtin_models:
+  - id: builtin-llm
+    name: gpt-4o-mini
+    type: KnowledgeQA
+`)
+	require.NoError(t, LoadBuiltinModelsConfig(context.Background(), db, dir))
+
+	live, _ := countModels(t, db, "manual-builtin")
+	assert.Equal(t, int64(1), live, "manual SQL-seeded builtin must survive YAML sweep")
+}
+
+func TestLoadBuiltinModelsConfig_ResurrectsSoftDeleted(t *testing.T) {
+	db := setupBuiltinModelsDB(t)
+	dir := writeYAML(t, `builtin_models:
+  - id: builtin-llm
+    name: gpt-4o-mini
+    type: KnowledgeQA
+`)
+	require.NoError(t, LoadBuiltinModelsConfig(context.Background(), db, dir))
+
+	// Simulate operator soft-deleting the row via API/UI.
+	require.NoError(t, db.Where("id = ?", "builtin-llm").Delete(&Model{}).Error)
+	live, deleted := countModels(t, db, "builtin-llm")
+	require.Equal(t, int64(0), live)
+	require.Equal(t, int64(1), deleted)
+
+	// Re-running the loader must resurrect the row (deleted_at -> NULL).
+	require.NoError(t, LoadBuiltinModelsConfig(context.Background(), db, dir))
+	live, deleted = countModels(t, db, "builtin-llm")
+	assert.Equal(t, int64(1), live, "row must be resurrected by UPSERT")
+	assert.Equal(t, int64(0), deleted, "deleted_at must be cleared")
+}
+
+func TestLoadBuiltinModelsConfig_ClearsExistingDefault(t *testing.T) {
+	db := setupBuiltinModelsDB(t)
+
+	// Seed an existing default (manual, e.g. user picked it via UI).
+	require.NoError(t, db.Create(&Model{
+		ID: "user-default", Name: "user", Type: ModelTypeKnowledgeQA,
+		TenantID: 10000, Source: ModelSourceRemote, Status: ModelStatusActive,
+		IsDefault: true, IsBuiltin: false, ManagedBy: "",
+	}).Error)
+
+	dir := writeYAML(t, `builtin_models:
+  - id: builtin-llm
+    name: gpt-4o-mini
+    type: KnowledgeQA
+    is_default: true
+`)
+	require.NoError(t, LoadBuiltinModelsConfig(context.Background(), db, dir))
+
+	var prev Model
+	require.NoError(t, db.Where("id = ?", "user-default").First(&prev).Error)
+	assert.False(t, prev.IsDefault,
+		"YAML is_default=true must clear other defaults in same (tenant,type)")
+
+	var newDefault Model
+	require.NoError(t, db.Where("id = ?", "builtin-llm").First(&newDefault).Error)
+	assert.True(t, newDefault.IsDefault)
+}
+
+func TestLoadBuiltinModelsConfig_EmptyListSweepsAllYAMLManaged(t *testing.T) {
+	db := setupBuiltinModelsDB(t)
+
+	// Seed one yaml-managed and one manual builtin.
+	require.NoError(t, db.Create(&Model{
+		ID: "yaml-old", Name: "y", Type: ModelTypeKnowledgeQA,
+		Source: ModelSourceRemote, Status: ModelStatusActive,
+		IsBuiltin: true, ManagedBy: BuiltinModelManagedBy,
+	}).Error)
+	require.NoError(t, db.Create(&Model{
+		ID: "manual-builtin", Name: "m", Type: ModelTypeKnowledgeQA,
+		Source: ModelSourceRemote, Status: ModelStatusActive,
+		IsBuiltin: true, ManagedBy: "",
+	}).Error)
+
+	// Explicit empty list. Loader treats this as "no YAML-managed models
+	// declared" and sweeps the entire yaml-managed slice. Manual rows are
+	// untouched.
+	dir := writeYAML(t, "builtin_models: []\n")
+	require.NoError(t, LoadBuiltinModelsConfig(context.Background(), db, dir))
+
+	live, _ := countModels(t, db, "yaml-old")
+	assert.Equal(t, int64(0), live, "yaml-managed row must be swept")
+
+	live, _ = countModels(t, db, "manual-builtin")
+	assert.Equal(t, int64(1), live, "manual row must survive empty-list sweep")
+}
+
+func TestLoadBuiltinModelsConfig_PreservesEntryIDOverBeforeCreate(t *testing.T) {
+	// Regression guard: BeforeCreate now only generates a UUID when ID is
+	// empty. YAML always supplies a stable id; if BeforeCreate ever forgets
+	// the guard and overwrites it, the UPSERT key changes and idempotency
+	// breaks. Reload the file twice and assert the same row id survives.
+	db := setupBuiltinModelsDB(t)
+	dir := writeYAML(t, `builtin_models:
+  - id: stable-id-123
+    name: n
+    type: KnowledgeQA
+`)
+	require.NoError(t, LoadBuiltinModelsConfig(context.Background(), db, dir))
+	require.NoError(t, LoadBuiltinModelsConfig(context.Background(), db, dir))
+
+	var ids []string
+	require.NoError(t, db.Model(&Model{}).Pluck("id", &ids).Error)
+	assert.Equal(t, []string{"stable-id-123"}, ids,
+		"YAML-declared id must survive across reloads (no UUID regeneration)")
+}