diff --git a/coderd/exp_chats_test.go b/coderd/exp_chats_test.go index e67d004fe5..a39f37175a 100644 --- a/coderd/exp_chats_test.go +++ b/coderd/exp_chats_test.go @@ -210,6 +210,7 @@ type failNextChatSystemPromptStore struct { armedGetChatSystemPromptConfigFailures *atomic.Int64 getChatSystemPromptConfigCallsBeforeFailure *atomic.Int64 failNextUpsertChatIncludeDefaultSystemPrompt *atomic.Bool + failNextGetChatOrganizationSystemPrompt *atomic.Bool } func newFailNextChatSystemPromptStore(store database.Store) *failNextChatSystemPromptStore { @@ -220,6 +221,7 @@ func newFailNextChatSystemPromptStore(store database.Store) *failNextChatSystemP armedGetChatSystemPromptConfigFailures: &atomic.Int64{}, getChatSystemPromptConfigCallsBeforeFailure: &atomic.Int64{}, failNextUpsertChatIncludeDefaultSystemPrompt: &atomic.Bool{}, + failNextGetChatOrganizationSystemPrompt: &atomic.Bool{}, } } @@ -231,10 +233,18 @@ func (s *failNextChatSystemPromptStore) InTx(function func(database.Store) error armedGetChatSystemPromptConfigFailures: s.armedGetChatSystemPromptConfigFailures, getChatSystemPromptConfigCallsBeforeFailure: s.getChatSystemPromptConfigCallsBeforeFailure, failNextUpsertChatIncludeDefaultSystemPrompt: s.failNextUpsertChatIncludeDefaultSystemPrompt, + failNextGetChatOrganizationSystemPrompt: s.failNextGetChatOrganizationSystemPrompt, }) }, txOpts) } +func (s *failNextChatSystemPromptStore) GetChatOrganizationSystemPrompt(ctx context.Context, organizationID uuid.UUID) (database.ChatOrganizationSystemPrompt, error) { + if s.failNextGetChatOrganizationSystemPrompt.CompareAndSwap(true, false) { + return database.ChatOrganizationSystemPrompt{}, stderrors.New("forced organization system prompt read failure") + } + return s.Store.GetChatOrganizationSystemPrompt(ctx, organizationID) +} + func (s *failNextChatSystemPromptStore) GetChatIncludeDefaultSystemPrompt(ctx context.Context) (bool, error) { if s.failNextGetChatIncludeDefaultSystemPrompt.CompareAndSwap(true, false) { return false, stderrors.New("forced include-default read failure") @@ -14150,104 +14160,59 @@ If a workspace is needed, use list_templates before create_workspace and follow }) }) - t.Run("CreateChatFallsBackToDefaultWhenSystemPromptConfigReadFailsWithIncludeDefaultEnabled", func(t *testing.T) { - ctx := testutil.Context(t, testutil.WaitLong) + t.Run("CreateChatFailsWhenSystemPromptConfigReadFails", func(t *testing.T) { + t.Parallel() - rawDB, pubsub := dbtestutil.NewDB(t) - store := newFailNextChatSystemPromptStore(rawDB) - rawClient, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{ - Database: store, - Pubsub: pubsub, - DeploymentValues: coderdtest.DeploymentValues(t), - }) - aibridgedtest.StartTestAIBridgeDaemon(t.Context(), t, api, nil) - client := codersdk.NewExperimentalClient(rawClient) - firstUser := coderdtest.CreateFirstUser(t, client.Client) - _ = createChatModel(t, client) + for _, tc := range []struct { + name string + fail func(*failNextChatSystemPromptStore) + }{ + { + name: "Deployment", + fail: func(s *failNextChatSystemPromptStore) { s.failNextGetChatSystemPromptConfig.Store(true) }, + }, + { + name: "Organization", + fail: func(s *failNextChatSystemPromptStore) { s.failNextGetChatOrganizationSystemPrompt.Store(true) }, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitLong) - err := client.UpdateChatSystemPrompt(ctx, codersdk.UpdateChatSystemPromptRequest{ - SystemPrompt: "Keep custom instructions", - IncludeDefaultSystemPrompt: ptr.Ref(true), - }) - require.NoError(t, err) + rawDB, pubsub := dbtestutil.NewDB(t) + store := newFailNextChatSystemPromptStore(rawDB) + rawClient, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{ + Database: store, + Pubsub: pubsub, + DeploymentValues: coderdtest.DeploymentValues(t), + }) + aibridgedtest.StartTestAIBridgeDaemon(t.Context(), t, api, nil) + client := codersdk.NewExperimentalClient(rawClient) + firstUser := coderdtest.CreateFirstUser(t, client.Client) + _ = createChatModel(t, client) - store.failNextGetChatSystemPromptConfig.Store(true) - chat, err := client.CreateChat(ctx, codersdk.CreateChatRequest{ - OrganizationID: firstUser.OrganizationID, - Content: []codersdk.ChatInputPart{{ - Type: codersdk.ChatInputPartTypeText, - Text: fmt.Sprintf("config-read fallback %s", t.Name()), - }}, - }) - require.NoError(t, err) + err := client.UpdateChatSystemPrompt(ctx, codersdk.UpdateChatSystemPromptRequest{ + SystemPrompt: "Keep custom instructions", + IncludeDefaultSystemPrompt: ptr.Ref(false), + }) + require.NoError(t, err) - messages, err := rawDB.GetChatMessagesForPromptByChatID(dbauthz.AsSystemRestricted(ctx), chat.ID) - require.NoError(t, err) + tc.fail(store) + _, err = client.CreateChat(ctx, codersdk.CreateChatRequest{ + OrganizationID: firstUser.OrganizationID, + Content: []codersdk.ChatInputPart{{ + Type: codersdk.ChatInputPartTypeText, + Text: fmt.Sprintf("config-read failure %s", t.Name()), + }}, + }) + requireSDKError(t, err, http.StatusInternalServerError) - var systemTexts []string - for _, message := range messages { - if message.Role != database.ChatMessageRoleSystem { - continue - } - parts, err := chatprompt.ParseContent(message) - require.NoError(t, err) - require.Len(t, parts, 1) - require.Equal(t, codersdk.ChatMessagePartTypeText, parts[0].Type) - systemTexts = append(systemTexts, parts[0].Text) + chats, err := client.ListChats(ctx, nil) + require.NoError(t, err) + require.Empty(t, chats) + }) } - - require.Equal(t, []string{chatd.DefaultSystemPrompt, workspaceAwareness}, systemTexts) - }) - - t.Run("CreateChatFallbackIgnoresDisabledPreferenceWhenConfigReadFails", func(t *testing.T) { - ctx := testutil.Context(t, testutil.WaitLong) - - rawDB, pubsub := dbtestutil.NewDB(t) - store := newFailNextChatSystemPromptStore(rawDB) - rawClient, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{ - Database: store, - Pubsub: pubsub, - DeploymentValues: coderdtest.DeploymentValues(t), - }) - aibridgedtest.StartTestAIBridgeDaemon(t.Context(), t, api, nil) - client := codersdk.NewExperimentalClient(rawClient) - firstUser := coderdtest.CreateFirstUser(t, client.Client) - _ = createChatModel(t, client) - - err := client.UpdateChatSystemPrompt(ctx, codersdk.UpdateChatSystemPromptRequest{ - SystemPrompt: "Do not use the default prompt", - IncludeDefaultSystemPrompt: ptr.Ref(false), - }) - require.NoError(t, err) - - // A config read failure loses all admin preferences, including - // include_default=false, so chat creation falls back to the built-in default. - store.failNextGetChatSystemPromptConfig.Store(true) - chat, err := client.CreateChat(ctx, codersdk.CreateChatRequest{ - OrganizationID: firstUser.OrganizationID, - Content: []codersdk.ChatInputPart{{ - Type: codersdk.ChatInputPartTypeText, - Text: fmt.Sprintf("config-read fallback %s", t.Name()), - }}, - }) - require.NoError(t, err) - - messages, err := rawDB.GetChatMessagesForPromptByChatID(dbauthz.AsSystemRestricted(ctx), chat.ID) - require.NoError(t, err) - - var systemTexts []string - for _, message := range messages { - if message.Role != database.ChatMessageRoleSystem { - continue - } - parts, err := chatprompt.ParseContent(message) - require.NoError(t, err) - require.Len(t, parts, 1) - require.Equal(t, codersdk.ChatMessagePartTypeText, parts[0].Type) - systemTexts = append(systemTexts, parts[0].Text) - } - - require.Equal(t, []string{chatd.DefaultSystemPrompt, workspaceAwareness}, systemTexts) }) t.Run("NonAdminFails", func(t *testing.T) { diff --git a/coderd/x/chatd/ARCHITECTURE.md b/coderd/x/chatd/ARCHITECTURE.md index 7bb346693e..0683e941f3 100644 --- a/coderd/x/chatd/ARCHITECTURE.md +++ b/coderd/x/chatd/ARCHITECTURE.md @@ -1097,7 +1097,6 @@ Details that follow from the override: - Failure semantics: an unset override uses the chat model. A stored config that later becomes deleted or disabled, whose provider becomes disabled, or whose required credentials become unavailable is logged and falls back to the chat model during generation preparation, leaving only the chat trigger. Failure to read the override row or load provider credentials stops preparation. - A failure while resolving the referenced model config or provider is logged and falls back to the chat model. A usable override that fails at use (route or client construction, provider call failure) fails the generation visibly through the normal error path; there is no silent fallback. The override model client is constructed inside the compact generation action, not at prepare time, so a broken override cannot fail turns that finish without compacting (including turns over the threshold whose last assistant step already completed). - Prompt safety: the prompt is built and sanitized for the chat model, so when the override points at a different provider the compaction copy of the prompt is re-sanitized: provider-executed tool history is flattened into plain text parts (keeping its content while dropping the provider-specific wire shape), file parts the compaction model rejects are replaced with text placeholders, and Anthropic provider-tool sanitization is re-run for the compaction provider. The assistant generation prompt is never mutated. diff --git a/coderd/x/chatd/chatd.go b/coderd/x/chatd/chatd.go index 6a58571fba..452c56e79c 100644 --- a/coderd/x/chatd/chatd.go +++ b/coderd/x/chatd/chatd.go @@ -260,12 +260,10 @@ func (p *Server) resolveAdvisorModelOverride( logger slog.Logger, ) (resolvedModelCall, bool, error) { override, err := p.resolveModelOverride(ctx, modelOverrideSpec{ - context: advisorOverrideContext, - ownerID: chat.OwnerID, - organizationID: chat.OrganizationID, - queryFailure: modelOverrideFailureModeSoft, - configFailure: modelOverrideFailureModeSoft, - providerFailure: modelOverrideFailureModeHard, + context: advisorOverrideContext, + ownerID: chat.OwnerID, + organizationID: chat.OrganizationID, + failIfNoProviderKey: true, }) if err != nil { return resolvedModelCall{}, false, xerrors.Errorf("resolve advisor override model: %w", err) @@ -1319,8 +1317,14 @@ func (p *Server) CreateChat(ctx context.Context, opts CreateOptions) (database.C // Resolve the deployment prompt before opening the transaction so // chat creation does not hold one DB connection while waiting for // another pool checkout. - deploymentPrompt := p.resolveDeploymentSystemPrompt(ctx) - organizationPrompt := p.resolveOrganizationSystemPrompt(ctx, opts.OrganizationID) + deploymentPrompt, err := p.resolveDeploymentSystemPrompt(ctx) + if err != nil { + return database.Chat{}, err + } + organizationPrompt, err := p.resolveOrganizationSystemPrompt(ctx, opts.OrganizationID) + if err != nil { + return database.Chat{}, err + } if opts.ModelConfigID != uuid.Nil { if err := requireEnabledChatModelConfig(ctx, p.db, opts.OrganizationID, opts.ModelConfigID); err != nil { @@ -4359,13 +4363,10 @@ func (p *Server) resolveUserCompactionThreshold(ctx context.Context, userID uuid // resolveDeploymentSystemPrompt builds the deployment-level system // prompt from the built-in default and the admin-configured custom // prompt stored in site_configs. -func (p *Server) resolveDeploymentSystemPrompt(ctx context.Context) string { +func (p *Server) resolveDeploymentSystemPrompt(ctx context.Context) (string, error) { config, err := p.db.GetChatSystemPromptConfig(ctx) if err != nil { - // Fail open: use the built-in default so chats always have - // some system guidance. - p.logger.Error(ctx, "failed to fetch chat system prompt configuration, using default", slog.Error(err)) - return DefaultSystemPrompt + return "", xerrors.Errorf("get chat system prompt config: %w", err) } sanitizedCustom := codersdk.SanitizePromptText(config.ChatSystemPrompt) @@ -4384,23 +4385,21 @@ func (p *Server) resolveDeploymentSystemPrompt(ctx context.Context) string { if result == "" { p.logger.Warn(ctx, "resolved system prompt is empty, no system prompt will be injected into chats") } - return result + return result, nil } // resolveOrganizationSystemPrompt returns the sanitized system prompt // configured for the organization, or an empty string when none is set. -func (p *Server) resolveOrganizationSystemPrompt(ctx context.Context, organizationID uuid.UUID) string { +func (p *Server) resolveOrganizationSystemPrompt(ctx context.Context, organizationID uuid.UUID) (string, error) { //nolint:gocritic // Chat creators cannot read organization config, so chatd reads it. row, err := p.db.GetChatOrganizationSystemPrompt(dbauthz.AsChatd(ctx), organizationID) - if err != nil { - if !errors.Is(err, sql.ErrNoRows) { - // Fail open: the deployment prompt still applies. - p.logger.Warn(ctx, "failed to fetch organization chat system prompt, omitting it", - slog.F("organization_id", organizationID), slog.Error(err)) - } - return "" + if errors.Is(err, sql.ErrNoRows) { + return "", nil } - return codersdk.SanitizePromptText(row.SystemPrompt) + if err != nil { + return "", xerrors.Errorf("get organization chat system prompt: %w", err) + } + return codersdk.SanitizePromptText(row.SystemPrompt), nil } // resolveUserPrompt fetches the user's custom chat prompt from the diff --git a/coderd/x/chatd/chatd_test.go b/coderd/x/chatd/chatd_test.go index 131a543568..54cb13f2e4 100644 --- a/coderd/x/chatd/chatd_test.go +++ b/coderd/x/chatd/chatd_test.go @@ -13164,7 +13164,7 @@ type overrideReadFailStore struct { } func (s *overrideReadFailStore) GetChatOrganizationModelOverride(ctx context.Context, arg database.GetChatOrganizationModelOverrideParams) (database.ChatOrganizationModelOverride, error) { - if s.fail.Load() { + if s.fail.Load() && arg.Context == "compaction" { return database.ChatOrganizationModelOverride{}, xerrors.New("injected compaction override read failure") } return s.Store.GetChatOrganizationModelOverride(ctx, arg) diff --git a/coderd/x/chatd/generation_preparer.go b/coderd/x/chatd/generation_preparer.go index c5bd9c61c9..9c58303f60 100644 --- a/coderd/x/chatd/generation_preparer.go +++ b/coderd/x/chatd/generation_preparer.go @@ -822,12 +822,9 @@ func (server *Server) prepareGeneration( contextLimit: modelConfig.ContextLimit, } resolvedCompactionOverride, err := server.resolveModelOverride(ctx, modelOverrideSpec{ - context: compactionOverrideContext, - ownerID: chat.OwnerID, - organizationID: chat.OrganizationID, - queryFailure: modelOverrideFailureModeHard, - configFailure: modelOverrideFailureModeSoft, - providerFailure: modelOverrideFailureModeSoft, + context: compactionOverrideContext, + ownerID: chat.OwnerID, + organizationID: chat.OrganizationID, }) if err != nil { return generationPrepared{}, err diff --git a/coderd/x/chatd/model_override.go b/coderd/x/chatd/model_override.go index d801daa434..a6eba0f23b 100644 --- a/coderd/x/chatd/model_override.go +++ b/coderd/x/chatd/model_override.go @@ -24,29 +24,17 @@ const ( var errInvalidModelOverrideMetadata = xerrors.New("invalid model override metadata") -// providerLookupError marks linked-provider row lookup failures so they -// classify under providerFailure rather than configFailure. -type providerLookupError struct{ err error } - -func (e providerLookupError) Error() string { return e.err.Error() } -func (e providerLookupError) Unwrap() error { return e.err } - -type modelOverrideFailureMode int - -const ( - modelOverrideFailureModeSoft modelOverrideFailureMode = iota - modelOverrideFailureModeHard -) - type modelOverrideSpec struct { context string ownerID uuid.UUID organizationID uuid.UUID - queryFailure modelOverrideFailureMode - configFailure modelOverrideFailureMode - // providerFailure governs linked-provider failures: provider row lookup - // errors and unusable credentials. - providerFailure modelOverrideFailureMode + // failIfUnavailable returns an error, instead of no override, when the + // model config or its provider is missing, disabled, not visible to the + // owner, or invalid. + failIfUnavailable bool + // failIfNoProviderKey returns an error, instead of no override, when + // the owner has no usable key for the override's provider. + failIfNoProviderKey bool } type resolvedModelOverride struct { @@ -58,6 +46,7 @@ type resolvedModelOverride struct { } func (p *Server) resolveModelOverride(ctx context.Context, spec modelOverrideSpec) (resolvedModelOverride, error) { + label := modelOverrideErrorLabel(spec.context) //nolint:gocritic // Chatd reads organization-scoped runtime configuration. override, err := p.db.GetChatOrganizationModelOverride( dbauthz.AsChatd(ctx), @@ -66,24 +55,11 @@ func (p *Server) resolveModelOverride(ctx context.Context, spec modelOverrideSpe Context: spec.context, }, ) + if errors.Is(err, sql.ErrNoRows) { + return resolvedModelOverride{}, nil + } if err != nil { - if errors.Is(err, sql.ErrNoRows) { - return resolvedModelOverride{}, nil - } - if spec.queryFailure == modelOverrideFailureModeSoft { - p.logger.Warn(ctx, - "failed to load model override, ignoring", - slog.F("override_context", spec.context), - slog.F("organization_id", spec.organizationID), - slog.Error(err), - ) - return resolvedModelOverride{}, nil - } - return resolvedModelOverride{}, xerrors.Errorf( - "get %s model override: %w", - modelOverrideErrorLabel(spec.context), - err, - ) + return resolvedModelOverride{}, xerrors.Errorf("get %s model override: %w", label, err) } resolved := resolvedModelOverride{ @@ -96,60 +72,27 @@ func (p *Server) resolveModelOverride(ctx context.Context, spec modelOverrideSpe override.ModelConfigID, ) if err != nil { - mode := spec.configFailure - var lookupErr providerLookupError - if errors.As(err, &lookupErr) { - mode = spec.providerFailure - } - if mode == modelOverrideFailureModeHard { - label := modelOverrideErrorLabel(spec.context) - switch { - case errors.Is(err, sql.ErrNoRows): - return resolved, xerrors.Errorf( - "%s model override is unavailable: %s", - label, - override.ModelConfigID, - ) - case errors.Is(err, errInvalidModelOverrideMetadata): - return resolved, xerrors.Errorf( - "%s model override metadata is invalid for %s: %w", - label, - override.ModelConfigID, - err, - ) - default: - return resolved, xerrors.Errorf( - "resolve %s model override %s: %w", - label, - override.ModelConfigID, - err, - ) - } - } - switch { - case errors.Is(err, sql.ErrNoRows): + case !modelConfigUnavailable(err): + return resolved, xerrors.Errorf("resolve %s model override %s: %w", label, override.ModelConfigID, err) + case !spec.failIfUnavailable: p.logger.Info(ctx, - "model override is unavailable, ignoring", - slog.F("override_context", spec.context), - slog.F("model_config_id", override.ModelConfigID), - ) - case errors.Is(err, errInvalidModelOverrideMetadata): - p.logger.Info(ctx, - "model override metadata is invalid, ignoring", + "model override is unavailable, using default model", slog.F("override_context", spec.context), slog.F("model_config_id", override.ModelConfigID), slog.Error(err), ) + return resolvedModelOverride{}, nil + case errors.Is(err, errInvalidModelOverrideMetadata): + return resolved, xerrors.Errorf( + "%s model override metadata is invalid for %s: %w", + label, + override.ModelConfigID, + err, + ) default: - p.logger.Warn(ctx, - "failed to resolve model override, ignoring", - slog.F("override_context", spec.context), - slog.F("model_config_id", override.ModelConfigID), - slog.Error(err), - ) + return resolved, xerrors.Errorf("%s model override is unavailable: %s", label, override.ModelConfigID) } - return resolvedModelOverride{}, nil } providerKeys, err := p.resolveUserProviderAPIKeys(ctx, spec.ownerID, modelConfigAIProviderID(modelConfig)) @@ -157,15 +100,15 @@ func (p *Server) resolveModelOverride(ctx context.Context, spec modelOverrideSpe return resolvedModelOverride{}, xerrors.Errorf("resolve provider API keys: %w", err) } if !userCanUseProviderKeys(providerKeys, providerName) { - if spec.providerFailure == modelOverrideFailureModeHard { + if spec.failIfNoProviderKey { return resolved, xerrors.Errorf( "%s model override credentials are unavailable for provider %q", - modelOverrideErrorLabel(spec.context), + label, providerName, ) } p.logger.Info(ctx, - "model override credentials are unavailable, ignoring", + "model override credentials are unavailable, using default model", slog.F("override_context", spec.context), slog.F("model_config_id", override.ModelConfigID), slog.F("provider", providerName), @@ -175,7 +118,7 @@ func (p *Server) resolveModelOverride(ctx context.Context, spec modelOverrideSpe resolvedProvider, resolvedModel, err := chatprovider.ResolveModelWithProviderHint(modelConfig.Model, providerName) if err != nil { - return resolved, xerrors.Errorf("resolve %s model override identity: %w", modelOverrideErrorLabel(spec.context), err) + return resolved, xerrors.Errorf("resolve %s model override identity: %w", label, err) } resolved.Config = modelConfig resolved.ResolvedProvider = resolvedProvider @@ -183,6 +126,14 @@ func (p *Server) resolveModelOverride(ctx context.Context, spec modelOverrideSpe return resolved, nil } +// modelConfigUnavailable reports whether err from a model config lookup means +// the config cannot be used, as opposed to a failed lookup. +func modelConfigUnavailable(err error) bool { + return errors.Is(err, sql.ErrNoRows) || + errors.Is(err, errInvalidModelOverrideMetadata) || + dbauthz.IsNotAuthorizedError(err) +} + func modelOverrideErrorLabel(overrideContext string) string { return strings.ReplaceAll(overrideContext, "_", " ") } @@ -228,10 +179,7 @@ func (p *Server) resolveNormalizedProviderForModelConfig( //nolint:gocritic // Provider configuration remains a privileged Chatd read. provider, err := p.db.GetAIProviderByID(dbauthz.AsChatd(ctx), modelConfig.AIProviderID.UUID) if err != nil { - if errors.Is(err, sql.ErrNoRows) { - return database.ChatModelConfig{}, "", err - } - return database.ChatModelConfig{}, "", providerLookupError{err} + return database.ChatModelConfig{}, "", err } if !provider.Enabled { return database.ChatModelConfig{}, "", sql.ErrNoRows diff --git a/coderd/x/chatd/model_override_internal_test.go b/coderd/x/chatd/model_override_internal_test.go index 7d162439ef..fafb988126 100644 --- a/coderd/x/chatd/model_override_internal_test.go +++ b/coderd/x/chatd/model_override_internal_test.go @@ -10,6 +10,7 @@ import ( "cdr.dev/slog/v3/sloggers/slogtest" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/database/dbmock" "github.com/coder/coder/v2/coderd/util/ptr" "github.com/coder/coder/v2/coderd/x/chatd/chatprovider" @@ -27,45 +28,34 @@ func TestResolveModelOverride(t *testing.T) { { name: "Title", spec: modelOverrideSpec{ - context: titleGenerationOverrideContext, - queryFailure: modelOverrideFailureModeHard, - configFailure: modelOverrideFailureModeHard, - providerFailure: modelOverrideFailureModeHard, + context: titleGenerationOverrideContext, + failIfUnavailable: true, + failIfNoProviderKey: true, }, }, { name: "Compaction", - spec: modelOverrideSpec{ - context: compactionOverrideContext, - queryFailure: modelOverrideFailureModeHard, - configFailure: modelOverrideFailureModeSoft, - providerFailure: modelOverrideFailureModeSoft, - }, + spec: modelOverrideSpec{context: compactionOverrideContext}, }, { name: "Subagent", - spec: modelOverrideSpec{ - context: string(codersdk.ChatModelOverrideContextGeneral), - queryFailure: modelOverrideFailureModeHard, - configFailure: modelOverrideFailureModeSoft, - providerFailure: modelOverrideFailureModeSoft, - }, + spec: modelOverrideSpec{context: string(codersdk.ChatModelOverrideContextGeneral)}, }, { name: "Advisor", spec: modelOverrideSpec{ - context: advisorOverrideContext, - queryFailure: modelOverrideFailureModeSoft, - configFailure: modelOverrideFailureModeSoft, - providerFailure: modelOverrideFailureModeHard, + context: advisorOverrideContext, + failIfNoProviderKey: true, }, }, } + always := func(modelOverrideSpec) bool { return true } + ifFailIfUnavailable := func(spec modelOverrideSpec) bool { return spec.failIfUnavailable } tests := []struct { name string setup func(*dbmock.MockStore, database.Chat, database.ChatModelConfig, uuid.UUID, modelOverrideSpec) - failure func(modelOverrideSpec) modelOverrideFailureMode + wantErr func(modelOverrideSpec) bool wantSet bool wantModel bool }{ @@ -80,7 +70,7 @@ func TestResolveModelOverride(t *testing.T) { setup: func(db *dbmock.MockStore, chat database.Chat, _ database.ChatModelConfig, _ uuid.UUID, spec modelOverrideSpec) { db.EXPECT().GetChatOrganizationModelOverride(gomock.Any(), modelOverrideParams(chat, spec.context)).Return(database.ChatOrganizationModelOverride{}, sql.ErrConnDone) }, - failure: func(spec modelOverrideSpec) modelOverrideFailureMode { return spec.queryFailure }, + wantErr: always, }, { name: "MissingConfig", @@ -88,7 +78,25 @@ func TestResolveModelOverride(t *testing.T) { db.EXPECT().GetChatOrganizationModelOverride(gomock.Any(), modelOverrideParams(chat, spec.context)).Return(orgModelOverride(chat, spec.context, config.ID, "high"), nil) db.EXPECT().GetChatModelConfigByID(gomock.Any(), config.ID).Return(database.ChatModelConfig{}, sql.ErrNoRows) }, - failure: func(spec modelOverrideSpec) modelOverrideFailureMode { return spec.configFailure }, + wantErr: ifFailIfUnavailable, + wantSet: true, + }, + { + name: "NotAuthorizedConfig", + setup: func(db *dbmock.MockStore, chat database.Chat, config database.ChatModelConfig, _ uuid.UUID, spec modelOverrideSpec) { + db.EXPECT().GetChatOrganizationModelOverride(gomock.Any(), modelOverrideParams(chat, spec.context)).Return(orgModelOverride(chat, spec.context, config.ID, "high"), nil) + db.EXPECT().GetChatModelConfigByID(gomock.Any(), config.ID).Return(database.ChatModelConfig{}, dbauthz.NotAuthorizedError{}) + }, + wantErr: ifFailIfUnavailable, + wantSet: true, + }, + { + name: "ConfigError", + setup: func(db *dbmock.MockStore, chat database.Chat, config database.ChatModelConfig, _ uuid.UUID, spec modelOverrideSpec) { + db.EXPECT().GetChatOrganizationModelOverride(gomock.Any(), modelOverrideParams(chat, spec.context)).Return(orgModelOverride(chat, spec.context, config.ID, "high"), nil) + db.EXPECT().GetChatModelConfigByID(gomock.Any(), config.ID).Return(database.ChatModelConfig{}, sql.ErrConnDone) + }, + wantErr: always, wantSet: true, }, { @@ -98,7 +106,7 @@ func TestResolveModelOverride(t *testing.T) { db.EXPECT().GetChatModelConfigByID(gomock.Any(), config.ID).Return(config, nil) db.EXPECT().GetAIProviderByID(gomock.Any(), providerID).Return(database.AIProvider{ID: providerID, Type: database.AIProviderTypeOpenai}, nil) }, - failure: func(spec modelOverrideSpec) modelOverrideFailureMode { return spec.configFailure }, + wantErr: ifFailIfUnavailable, wantSet: true, }, { @@ -108,7 +116,7 @@ func TestResolveModelOverride(t *testing.T) { db.EXPECT().GetChatModelConfigByID(gomock.Any(), config.ID).Return(config, nil) db.EXPECT().GetAIProviderByID(gomock.Any(), providerID).Return(database.AIProvider{ID: providerID, Type: database.AIProviderType("invalid"), Enabled: true}, nil) }, - failure: func(spec modelOverrideSpec) modelOverrideFailureMode { return spec.configFailure }, + wantErr: ifFailIfUnavailable, wantSet: true, }, { @@ -118,7 +126,7 @@ func TestResolveModelOverride(t *testing.T) { db.EXPECT().GetChatModelConfigByID(gomock.Any(), config.ID).Return(config, nil) db.EXPECT().GetAIProviderByID(gomock.Any(), providerID).Return(database.AIProvider{}, sql.ErrConnDone) }, - failure: func(spec modelOverrideSpec) modelOverrideFailureMode { return spec.providerFailure }, + wantErr: always, wantSet: true, }, { @@ -129,7 +137,7 @@ func TestResolveModelOverride(t *testing.T) { db.EXPECT().GetAIProviderByID(gomock.Any(), providerID).Return(aibridgeTestAIProvider(providerID, "primary-openai", database.AIProviderTypeOpenai), nil).AnyTimes() db.EXPECT().GetAIProviderKeysByProviderID(gomock.Any(), providerID).Return(nil, nil) }, - failure: func(spec modelOverrideSpec) modelOverrideFailureMode { return spec.providerFailure }, + wantErr: func(spec modelOverrideSpec) bool { return spec.failIfNoProviderKey }, wantSet: true, }, { @@ -165,7 +173,7 @@ func TestResolveModelOverride(t *testing.T) { server := titleOverrideTestServer(db, slogtest.Make(t, &slogtest.Options{IgnoreErrors: true})) resolved, err := server.resolveModelOverride(ctx, spec) - wantErr := test.failure != nil && test.failure(spec) == modelOverrideFailureModeHard + wantErr := test.wantErr != nil && test.wantErr(spec) if wantErr { require.Error(t, err) } else { diff --git a/coderd/x/chatd/quickgen.go b/coderd/x/chatd/quickgen.go index 1d02ba86a5..2092901f2f 100644 --- a/coderd/x/chatd/quickgen.go +++ b/coderd/x/chatd/quickgen.go @@ -225,12 +225,11 @@ func (p *Server) resolveQuickgenModel( modelOpts modelBuildOptions, ) (resolvedModelCall, error) { override, overrideErr := p.resolveModelOverride(ctx, modelOverrideSpec{ - context: titleGenerationOverrideContext, - ownerID: chat.OwnerID, - organizationID: chat.OrganizationID, - queryFailure: modelOverrideFailureModeHard, - configFailure: modelOverrideFailureModeHard, - providerFailure: modelOverrideFailureModeHard, + context: titleGenerationOverrideContext, + ownerID: chat.OwnerID, + organizationID: chat.OrganizationID, + failIfUnavailable: true, + failIfNoProviderKey: true, }) if overrideErr != nil { if override.Set { diff --git a/coderd/x/chatd/subagent.go b/coderd/x/chatd/subagent.go index 9bfddce334..f29c0801aa 100644 --- a/coderd/x/chatd/subagent.go +++ b/coderd/x/chatd/subagent.go @@ -204,31 +204,21 @@ func (p *Server) resolvePersonalModelOverride( ) (database.ChatModelConfig, bool, error) { modelConfig, providerName, err := p.resolveModelConfigAndNormalizedProvider(ctx, ownerID, modelConfigID) if err != nil { - switch { - case xerrors.Is(err, sql.ErrNoRows): - p.logger.Debug(ctx, - "personal model override is unavailable, using deployment default", - slog.F("override_context", overrideContext), - slog.F("owner_id", ownerID), - slog.F("model_config_id", modelConfigID), - ) - case errors.Is(err, errInvalidModelOverrideMetadata): - p.logger.Debug(ctx, - "personal model override metadata is invalid, using deployment default", - slog.F("override_context", overrideContext), - slog.F("owner_id", ownerID), - slog.F("model_config_id", modelConfigID), - slog.Error(err), - ) - default: - p.logger.Warn(ctx, - "failed to resolve personal model override, using deployment default", - slog.F("override_context", overrideContext), - slog.F("owner_id", ownerID), - slog.F("model_config_id", modelConfigID), - slog.Error(err), + if !modelConfigUnavailable(err) { + return database.ChatModelConfig{}, false, xerrors.Errorf( + "resolve %s personal model override %s: %w", + subagentModelOverrideLogLabel(overrideContext), + modelConfigID, + err, ) } + p.logger.Debug(ctx, + "personal model override is unavailable, using deployment default", + slog.F("override_context", overrideContext), + slog.F("owner_id", ownerID), + slog.F("model_config_id", modelConfigID), + slog.Error(err), + ) return database.ChatModelConfig{}, false, nil } providerKeys, err := p.resolveUserProviderAPIKeys(ctx, ownerID, modelConfigAIProviderID(modelConfig)) @@ -286,12 +276,9 @@ func (p *Server) resolveSubagentModelConfigID( } resolved, err := p.resolveModelOverride(chatdCtx, modelOverrideSpec{ - context: string(overrideContext), - ownerID: ownerID, - organizationID: organizationID, - queryFailure: modelOverrideFailureModeHard, - configFailure: modelOverrideFailureModeSoft, - providerFailure: modelOverrideFailureModeSoft, + context: string(overrideContext), + ownerID: ownerID, + organizationID: organizationID, }) if err != nil { return uuid.Nil, nil, err @@ -1070,11 +1057,17 @@ func (p *Server) createChildSubagentChatWithOptions( // Resolve the deployment prompt before opening the transaction so // child chat creation does not hold one DB connection while waiting // for another pool checkout. - deploymentPrompt := p.resolveDeploymentSystemPrompt(ctx) + deploymentPrompt, err := p.resolveDeploymentSystemPrompt(ctx) + if err != nil { + return database.Chat{}, err + } // Delegated chats cannot call list_agents or message_agent, so // strip the root-only orchestration guidance from their prompt. deploymentPrompt = strings.Replace(deploymentPrompt, subagentOrchestrationPromptBlock, "", 1) - organizationPrompt := p.resolveOrganizationSystemPrompt(ctx, parent.OrganizationID) + organizationPrompt, err := p.resolveOrganizationSystemPrompt(ctx, parent.OrganizationID) + if err != nil { + return database.Chat{}, err + } // Review before persistence so spawned chats cannot bypass prompt policy. childChatID := uuid.New()