diff --git a/apps/control-plane/cmd/server/main.go b/apps/control-plane/cmd/server/main.go index 68bfd0aa5..4870c5370 100644 --- a/apps/control-plane/cmd/server/main.go +++ b/apps/control-plane/cmd/server/main.go @@ -675,6 +675,25 @@ func main() { } catalogVisibilityHandler = catalog.NewVisibilityHandler(catalogSvc, owuiSync) log.Println("tenant model visibility admin routes registered (Phase 20 Plan 04)") + + // Issue #772 — the OWUI chat picker listed hive-embedding-default, + // hive-stt and hive-tts as selectable chat models: picking one + // produced a broken conversation. syncOWUI now locks non-chat- + // modality aliases out of the OWUI picker, but that function only + // runs from the admin PUT/DELETE visibility mutation path, which a + // migration-seeded alias (all three of the above) never goes + // through. Reconcile once at boot so the fix actually applies to + // rows already sitting in model_aliases, and so it re-applies on + // its own after an Open WebUI image bump resets access_control. + // Best-effort: a failure here logs and does not block startup, the + // same posture syncOWUI itself takes for a single alias. + if owuiClient != nil { + if err := catalogVisibilityHandler.ReconcileOWUISync(runCtx); err != nil { + log.Printf("WARNING: owui non-chat-modality reconcile failed (issue #772): %v", err) + } else { + log.Println("owui non-chat-modality reconcile complete (issue #772)") + } + } } configuredSinks := configuredAuditSinks() diff --git a/apps/control-plane/internal/catalog/http.go b/apps/control-plane/internal/catalog/http.go index c443d233b..59f81a3bb 100644 --- a/apps/control-plane/internal/catalog/http.go +++ b/apps/control-plane/internal/catalog/http.go @@ -228,7 +228,7 @@ func (h *VisibilityHandler) handleUpsertVisibility(w http.ResponseWriter, r *htt return } - h.syncOWUI(r, aliasID) + h.syncOWUI(r.Context(), aliasID) writeJSON(w, http.StatusOK, map[string]string{"status": "ok"}) } @@ -238,37 +238,88 @@ func (h *VisibilityHandler) handleDeleteVisibility(w http.ResponseWriter, r *htt return } - h.syncOWUI(r, aliasID) + h.syncOWUI(r.Context(), aliasID) writeJSON(w, http.StatusOK, map[string]string{"status": "ok"}) } -// syncOWUI computes the full OWUI access_control state for aliasID after any -// visibility mutation and writes it atomically. It is best-effort: errors do -// not fail the HTTP response. +// nonChatModalityBadges lists the model_aliases.capability_badges values that +// mark an alias as something other than a text chat model (embeddings and +// speech aliases). Open WebUI's model picker is chat-only; an alias carrying +// any of these badges must never be chat-selectable there, independent of +// its tenant_model_visibility class (issue #772: hive-embedding-default, +// hive-stt, hive-tts were listed as pickable chat models and produced a +// broken conversation when chosen). // -// Visibility semantics: -// - restricted alias, no visible=true rows: model should be inaccessible in -// OWUI. We send a non-nil but empty-named sentinel group so OWUI does not -// fall back to public access (access_control:null = public for all users). -// Concretely we use the group "hive-restricted-placeholder" which by -// definition has no members. -// - restricted alias, visible=true rows exist: resolve real OWUI group UUIDs -// via EnsureGroup and send those as the allowlist. -// - public/preview alias with visible=false rows (explicit blocks): the -// model stays in OWUI as public (access_control:null) because OWUI cannot -// express per-user deny lists. Hive's catalog filtering is the enforcement -// layer for public-alias blocks; OWUI sync is skipped for public aliases. -func (h *VisibilityHandler) syncOWUI(r *http.Request, aliasID string) { +// ponytail: capability_badges is freeform jsonb with no CHECK constraint and +// model_aliases has no dedicated modality/is_chat_model column, so this is a +// string-membership test over admin-editable data rather than a schema +// guarantee. It is bounded today: every embeddings/stt/tts alias is seeded by +// a migration under our control (20260423_01_embedding_alias.sql, +// 20260717_02_voice_groq_stt_tts.sql) and TestIsNonChatModality pins the +// badges each one carries. The real fix is a modality or is_chat_model column +// on model_aliases; add it and switch this predicate to read it if +// capability_badges ever drifts from these values. +var nonChatModalityBadges = map[string]bool{ + "embeddings": true, + "stt": true, + "tts": true, + "voice": true, +} + +// isNonChatModality reports whether badges marks an alias as non-chat. This +// is deliberately exclusion logic (does any badge match a known non-chat +// tag) rather than inclusion logic (does "chat" appear): hive-auto is a +// legitimate chat-selectable alias whose badges are +// ["auto","fallback","preview"] with no explicit "chat" badge, so a +// chat-badge whitelist would wrongly hide it from the OWUI picker. +func isNonChatModality(badges []string) bool { + for _, b := range badges { + if nonChatModalityBadges[strings.ToLower(strings.TrimSpace(b))] { + return true + } + } + return false +} + +// syncOWUI computes the full OWUI access_control state for aliasID and writes +// it atomically. It runs after any visibility mutation and from the boot-time +// ReconcileOWUISync (reconcile.go). It is best-effort: errors do not fail the +// caller. +// +// Semantics: +// - non-chat-modality alias (isNonChatModality — embeddings/stt/tts/voice): +// always locked out of the OWUI chat picker via the placeholder group, +// regardless of Visibility class or any tenant_model_visibility grant. +// tenant_model_visibility governs API invocation entitlement, not chat +// dropdown selectability, and these aliases must never be chat-selectable +// (issue #772). +// - restricted chat alias, no visible=true rows: model should be +// inaccessible in OWUI. We send a non-nil but empty-named sentinel group +// so OWUI does not fall back to public access (access_control:null = +// public for all users). Concretely we use the group +// "hive-restricted-placeholder" which by definition has no members. +// - restricted chat alias, visible=true rows exist: resolve real OWUI group +// UUIDs via EnsureGroup and send those as the allowlist. +// - public/preview chat alias with visible=false rows (explicit blocks): +// the model stays in OWUI as public (access_control:null) because OWUI +// cannot express per-user deny lists. Hive's catalog filtering is the +// enforcement layer for public-alias blocks; OWUI sync is skipped for +// public chat aliases. +func (h *VisibilityHandler) syncOWUI(ctx context.Context, aliasID string) { if h.owui == nil { return } - ctx := r.Context() alias, err := h.svc.repo.GetAlias(ctx, aliasID) if err != nil { return } + if isNonChatModality(alias.CapabilityBadges) { + h.lockOWUIModel(ctx, aliasID) + return + } + // Public/preview aliases: Hive catalog is the enforcement layer for // visible=false blocks. OWUI has no deny-list primitive, so skip sync. if alias.Visibility != "restricted" { @@ -281,14 +332,8 @@ func (h *VisibilityHandler) syncOWUI(r *http.Request, aliasID string) { } if len(visibleRows) == 0 { - // Restricted alias with no active grants: lock it down in OWUI by - // using a placeholder group that has no members. Sending null would - // make it public, which is the opposite of the desired state. - placeholderID, err := h.owui.EnsureGroup(ctx, "hive-restricted-placeholder") - if err != nil { - return - } - _ = h.owui.SyncModelAccessControl(ctx, aliasID, []string{placeholderID}) + // Restricted alias with no active grants: lock it down in OWUI. + h.lockOWUIModel(ctx, aliasID) return } @@ -304,6 +349,20 @@ func (h *VisibilityHandler) syncOWUI(r *http.Request, aliasID string) { _ = h.owui.SyncModelAccessControl(ctx, aliasID, groupIDs) } +// lockOWUIModel points aliasID's OWUI access_control at the +// "hive-restricted-placeholder" group, a group that by definition has no +// members. Sending a nil/empty allowlist instead would make the model public +// (OWUI's access_control:null semantics), which is the opposite of "locked +// out". Shared by the non-chat-modality lock and the restricted-with-no- +// grants lock in syncOWUI. +func (h *VisibilityHandler) lockOWUIModel(ctx context.Context, aliasID string) { + placeholderID, err := h.owui.EnsureGroup(ctx, "hive-restricted-placeholder") + if err != nil { + return + } + _ = h.owui.SyncModelAccessControl(ctx, aliasID, []string{placeholderID}) +} + func writeJSON(w http.ResponseWriter, status int, body any) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(status) diff --git a/apps/control-plane/internal/catalog/http_test.go b/apps/control-plane/internal/catalog/http_test.go index f2e8c1de6..5ae6cd614 100644 --- a/apps/control-plane/internal/catalog/http_test.go +++ b/apps/control-plane/internal/catalog/http_test.go @@ -167,6 +167,16 @@ type stubOWUI struct { syncErr error // groupIDs maps group name to returned ID (simulates OWUI group store). groupIDs map[string]string + // calls records every SyncModelAccessControl invocation in order, unlike + // syncedModel/syncedGroups above (which only hold the most recent call). + // Reconcile tests walk multiple aliases in one pass and need the full + // history to assert which aliases were touched and which were not. + calls []owuiSyncCall +} + +type owuiSyncCall struct { + modelID string + groupIDs []string } func (s *stubOWUI) EnsureGroup(_ context.Context, name string) (string, error) { @@ -183,6 +193,7 @@ func (s *stubOWUI) EnsureGroup(_ context.Context, name string) (string, error) { func (s *stubOWUI) SyncModelAccessControl(_ context.Context, modelID string, groupIDs []string) error { s.syncedModel = modelID s.syncedGroups = groupIDs + s.calls = append(s.calls, owuiSyncCall{modelID: modelID, groupIDs: groupIDs}) return s.syncErr } @@ -261,16 +272,31 @@ func TestSyncOWUI_RestrictedAlias_WithGrants_UsesRealGroupIDs(t *testing.T) { } } -// TestSyncOWUI_PublicAlias_SkipsSync proves T5: public/preview aliases must -// not have their OWUI access_control overwritten when a visibility block row -// exists. Hive catalog filtering is the enforcement layer; OWUI sync is skipped. +// TestSyncOWUI_PublicAlias_SkipsSync proves T5: public/preview *chat* aliases +// must not have their OWUI access_control overwritten when a visibility block +// row exists. Hive catalog filtering is the enforcement layer; OWUI sync is +// skipped. +// +// The alias here carries real chat badges (["stable","chat","responses"], +// matching hive-default's seed row) rather than an empty CapabilityBadges +// slice. Before issue #772's fix this test passed even with no badges at all, +// which meant it was not actually distinguishing "public chat alias, skip +// sync" from "public alias with no badges, skip sync" — the same assertion +// happened to hold for the wrong reason. TestSyncOWUI_PublicNonChatAlias_ +// UsesPlaceholder below is the case that must NOT skip sync despite also +// being Visibility=="public". func TestSyncOWUI_PublicAlias_SkipsSync(t *testing.T) { tenantID := uuid.MustParse("ffffffff-0000-0000-0000-000000000001") aliasID := "pub-a" repo := &stubRepository{ aliases: []ModelAlias{ - {AliasID: aliasID, Visibility: "public", CreatedAt: time.Now()}, + { + AliasID: aliasID, + Visibility: "public", + CapabilityBadges: []string{"stable", "chat", "responses"}, + CreatedAt: time.Now(), + }, }, visibilityRows: []TenantModelVisibility{ {TenantID: tenantID, AliasID: aliasID, Visible: false}, @@ -286,10 +312,83 @@ func TestSyncOWUI_PublicAlias_SkipsSync(t *testing.T) { if rr.Code != http.StatusOK { t.Fatalf("expected 200, got %d: %s", rr.Code, rr.Body.String()) } - // OWUI sync must be skipped for public aliases — access_control:null is correct - // (OWUI has no deny-list primitive; Hive catalog is the enforcement layer). + // OWUI sync must be skipped for public chat aliases — access_control:null + // is correct (OWUI has no deny-list primitive; Hive catalog is the + // enforcement layer). if owuiStub.syncedModel != "" { - t.Fatalf("expected no OWUI sync for public alias, but SyncModelAccessControl was called for %q", owuiStub.syncedModel) + t.Fatalf("expected no OWUI sync for public chat alias, but SyncModelAccessControl was called for %q", owuiStub.syncedModel) + } +} + +// TestSyncOWUI_PublicNonChatAlias_UsesPlaceholder proves the issue #772 fix: +// an alias carrying a non-chat-modality badge (embeddings/stt/tts/voice) must +// get the OWUI placeholder lockout group even though Visibility=="public" — +// the gate that used to skip OWUI sync for every non-restricted alias must +// not skip this one. Badges mirror the real seed row for hive-embedding- +// default (20260423_01_embedding_alias.sql: ["stable","embeddings"]). +func TestSyncOWUI_PublicNonChatAlias_UsesPlaceholder(t *testing.T) { + tenantID := uuid.MustParse("11111111-2222-0000-0000-000000000001") + aliasID := "hive-embedding-default" + + repo := &stubRepository{ + aliases: []ModelAlias{ + { + AliasID: aliasID, + Visibility: "public", + CapabilityBadges: []string{"stable", "embeddings"}, + CreatedAt: time.Now(), + }, + }, + } + owuiStub := &stubOWUI{groupIDs: map[string]string{ + "hive-restricted-placeholder": "placeholder-id", + }} + vh := NewVisibilityHandler(NewService(repo), owuiStub) + + // Any visibility mutation call triggers syncOWUI; DELETE on an unrelated + // tenant is enough to exercise it, matching how the admin PUT/DELETE + // handlers invoke it in production. + req := httptest.NewRequest(http.MethodDelete, "/internal/catalog/visibility/"+tenantID.String()+"/"+aliasID, nil) + rr := httptest.NewRecorder() + vh.handleDeleteVisibility(rr, req, tenantID, aliasID) + + if rr.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rr.Code, rr.Body.String()) + } + if owuiStub.syncedModel != aliasID { + t.Fatalf("expected OWUI sync for %q despite Visibility==public, got %q", aliasID, owuiStub.syncedModel) + } + if len(owuiStub.syncedGroups) == 0 || owuiStub.syncedGroups[0] != "placeholder-id" { + t.Fatalf("expected placeholder-id lockout group for non-chat-modality alias, got %v", owuiStub.syncedGroups) + } +} + +// TestIsNonChatModality pins the exclusion predicate directly. hive-auto +// (["auto","fallback","preview"], no "chat" badge — see +// 20260331_01_model_catalog.sql) is the case that rules out an inclusion +// ("has chat badge") predicate: it is a legitimate chat-selectable alias that +// would be wrongly hidden by a chat-badge whitelist. +func TestIsNonChatModality(t *testing.T) { + tests := []struct { + name string + badges []string + want bool + }{ + {name: "hive-default chat badges", badges: []string{"stable", "chat", "responses"}, want: false}, + {name: "hive-fast chat badges", badges: []string{"fast", "chat", "responses"}, want: false}, + {name: "hive-auto has no chat badge but is chat-selectable", badges: []string{"auto", "fallback", "preview"}, want: false}, + {name: "hive-embedding-default badges", badges: []string{"stable", "embeddings"}, want: true}, + {name: "hive-stt badges", badges: []string{"voice", "stt"}, want: true}, + {name: "hive-tts badges", badges: []string{"voice", "tts"}, want: true}, + {name: "nil badges", badges: nil, want: false}, + {name: "empty badges", badges: []string{}, want: false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isNonChatModality(tt.badges); got != tt.want { + t.Fatalf("isNonChatModality(%v) = %v, want %v", tt.badges, got, tt.want) + } + }) } } diff --git a/apps/control-plane/internal/catalog/reconcile.go b/apps/control-plane/internal/catalog/reconcile.go new file mode 100644 index 000000000..0a7f54b01 --- /dev/null +++ b/apps/control-plane/internal/catalog/reconcile.go @@ -0,0 +1,38 @@ +package catalog + +import ( + "context" + "fmt" +) + +// ReconcileOWUISync walks every model alias and applies the OWUI +// access_control state syncOWUI would have applied had the alias passed +// through the admin visibility mutation path (PUT/DELETE +// /internal/catalog/visibility/{tenant}/{alias}). That path only fires on an +// explicit admin action, so it has never touched an alias a migration seeds +// directly into model_aliases (hive-embedding-default, hive-stt, hive-tts) — +// this is why the OWUI chat dropdown has surfaced them as pickable chat +// models since the day they were added (issue #772). +// +// Call once at boot, after the OWUI client and catalog service are both +// wired. Safe to call repeatedly: syncOWUI computes each alias's target state +// from scratch on every call, so this also makes the fix self-healing across +// an Open WebUI image bump that resets access_control on its own model rows. +// +// Best-effort like syncOWUI: a single alias failing to sync (OWUI +// unreachable, alias lookup error) does not abort the walk or fail startup. +func (h *VisibilityHandler) ReconcileOWUISync(ctx context.Context) error { + if h.owui == nil { + return nil + } + + aliases, err := h.svc.repo.ListAllAliases(ctx) + if err != nil { + return fmt.Errorf("catalog: reconcile owui sync: list aliases: %w", err) + } + + for _, alias := range aliases { + h.syncOWUI(ctx, alias.AliasID) + } + return nil +} diff --git a/apps/control-plane/internal/catalog/reconcile_test.go b/apps/control-plane/internal/catalog/reconcile_test.go new file mode 100644 index 000000000..8ebfbb902 --- /dev/null +++ b/apps/control-plane/internal/catalog/reconcile_test.go @@ -0,0 +1,100 @@ +package catalog + +import ( + "context" + "errors" + "testing" + "time" +) + +// TestReconcileOWUISync_MigrationSeededNonChatAlias_GetsPlaceholder proves the +// second half of the issue #772 fix: a non-chat-modality alias seeded +// straight into model_aliases by a migration (hive-embedding-default, +// hive-stt, hive-tts in production) — with zero tenant_model_visibility rows +// and zero prior admin visibility mutations — still ends up locked out of the +// OWUI chat picker once ReconcileOWUISync runs. Without this, extending +// syncOWUI's gate alone does nothing on an existing box: syncOWUI only fires +// from the PUT/DELETE admin mutation path, which a migration-seeded row never +// goes through. +// +// A genuinely chat-shaped alias (hive-default) is included alongside it to +// prove the reconcile does not touch aliases outside its scope: syncOWUI +// already skips public chat aliases with no restricted grants, and the walk +// must not somehow force a sync for them too. +func TestReconcileOWUISync_MigrationSeededNonChatAlias_GetsPlaceholder(t *testing.T) { + repo := &stubRepository{ + aliases: []ModelAlias{ + { + AliasID: "hive-embedding-default", + Visibility: "public", + CapabilityBadges: []string{"stable", "embeddings"}, + CreatedAt: time.Now(), + }, + { + AliasID: "hive-stt", + Visibility: "public", + CapabilityBadges: []string{"voice", "stt"}, + CreatedAt: time.Now(), + }, + { + AliasID: "hive-default", + Visibility: "public", + CapabilityBadges: []string{"stable", "chat", "responses"}, + CreatedAt: time.Now(), + }, + }, + // No visibilityRows at all — models this alias set has never been + // touched by the admin PUT/DELETE visibility endpoints. + } + owuiStub := &stubOWUI{groupIDs: map[string]string{ + "hive-restricted-placeholder": "placeholder-id", + }} + vh := NewVisibilityHandler(NewService(repo), owuiStub) + + if err := vh.ReconcileOWUISync(context.Background()); err != nil { + t.Fatalf("ReconcileOWUISync returned error: %v", err) + } + + synced := make(map[string][]string, len(owuiStub.calls)) + for _, call := range owuiStub.calls { + synced[call.modelID] = call.groupIDs + } + + for _, aliasID := range []string{"hive-embedding-default", "hive-stt"} { + groups, ok := synced[aliasID] + if !ok { + t.Fatalf("expected ReconcileOWUISync to sync %q, it did not", aliasID) + } + if len(groups) == 0 || groups[0] != "placeholder-id" { + t.Fatalf("expected %q locked with placeholder-id, got %v", aliasID, groups) + } + } + + if _, ok := synced["hive-default"]; ok { + t.Fatalf("expected no OWUI sync for chat-shaped public alias hive-default, but it was synced: %v", synced["hive-default"]) + } +} + +// TestReconcileOWUISync_NilOWUI_NoOp proves the reconcile is a no-op (and does +// not touch the repository) when OWUI is not configured, mirroring syncOWUI's +// own nil-owui guard. +func TestReconcileOWUISync_NilOWUI_NoOp(t *testing.T) { + repo := &stubRepository{err: errors.New("repository must not be queried when OWUI is nil")} + vh := NewVisibilityHandler(NewService(repo), nil) + + if err := vh.ReconcileOWUISync(context.Background()); err != nil { + t.Fatalf("expected nil error with nil OWUI client, got %v", err) + } +} + +// TestReconcileOWUISync_RepositoryError_ReturnsError proves a repository +// failure surfaces to the caller (main.go logs it) instead of failing silently. +func TestReconcileOWUISync_RepositoryError_ReturnsError(t *testing.T) { + repo := &stubRepository{err: errors.New("db unavailable")} + owuiStub := &stubOWUI{} + vh := NewVisibilityHandler(NewService(repo), owuiStub) + + if err := vh.ReconcileOWUISync(context.Background()); err == nil { + t.Fatal("expected error when ListAllAliases fails, got nil") + } +} diff --git a/apps/control-plane/internal/catalog/repository.go b/apps/control-plane/internal/catalog/repository.go index 8983d39a7..9e946bce1 100644 --- a/apps/control-plane/internal/catalog/repository.go +++ b/apps/control-plane/internal/catalog/repository.go @@ -31,6 +31,13 @@ type Repository interface { UpsertVisibility(ctx context.Context, row TenantModelVisibility) error DeleteVisibility(ctx context.Context, tenantID uuid.UUID, aliasID string) error GetAllVisibleTenantsForAlias(ctx context.Context, aliasID string) ([]TenantModelVisibility, error) + // ListAllAliases returns every model alias regardless of visibility class. + // Unlike ListPublicAliases (public/preview only) or ListAliasesForTenant + // (entitlement-filtered), this is the unfiltered row set: it exists for the + // boot-time OWUI reconcile, which must see restricted and internal aliases + // too so it can lock down a non-chat-modality alias no matter what + // visibility class it was seeded with. + ListAllAliases(ctx context.Context) ([]ModelAlias, error) } type pgxRepository struct { @@ -305,6 +312,47 @@ func (r *pgxRepository) GetAllVisibleTenantsForAlias(ctx context.Context, aliasI return out, nil } +// ListAllAliases returns every row in model_aliases, unfiltered by +// visibility. See the Repository interface doc for why this differs from +// ListPublicAliases and ListAliasesForTenant. +func (r *pgxRepository) ListAllAliases(ctx context.Context) ([]ModelAlias, error) { + rows, err := r.pool.Query(ctx, ` + SELECT + alias_id, + owned_by, + display_name, + summary, + visibility, + lifecycle, + capability_badges, + input_price_credits, + output_price_credits, + cache_read_price_credits, + cache_write_price_credits, + created_at, + updated_at + FROM public.model_aliases + ORDER BY alias_id ASC + `) + if err != nil { + return nil, fmt.Errorf("catalog: list all aliases: %w", err) + } + defer rows.Close() + + var aliases []ModelAlias + for rows.Next() { + alias, err := scanModelAlias(rows) + if err != nil { + return nil, err + } + aliases = append(aliases, alias) + } + if err := rows.Err(); err != nil { + return nil, fmt.Errorf("catalog: iterate all aliases: %w", err) + } + return aliases, nil +} + func (r *pgxRepository) GetSnapshot(ctx context.Context) (CatalogSnapshot, error) { aliases, err := r.ListPublicAliases(ctx) if err != nil { diff --git a/apps/control-plane/internal/catalog/service_test.go b/apps/control-plane/internal/catalog/service_test.go index 1dff75bcb..bf3964e39 100644 --- a/apps/control-plane/internal/catalog/service_test.go +++ b/apps/control-plane/internal/catalog/service_test.go @@ -125,6 +125,15 @@ func (s *stubRepository) GetAllVisibleTenantsForAlias(_ context.Context, aliasID return out, nil } +// ListAllAliases mirrors the real pgx query: every alias, no visibility +// filter. Used by the boot-time OWUI reconcile tests. +func (s *stubRepository) ListAllAliases(_ context.Context) ([]ModelAlias, error) { + if s.err != nil { + return nil, s.err + } + return append([]ModelAlias(nil), s.aliases...), nil +} + func (s *stubRepository) GetAlias(_ context.Context, aliasID string) (ModelAlias, error) { if s.err != nil { return ModelAlias{}, s.err