mirror of
https://github.com/coder/coder.git
synced 2026-10-16 14:48:56 +08:00
fix(coderd): return database errors at chat create and subagent spawn (#30449)
Relates to CODAGT-1330 chatd stores the deployment and organization system prompts in a chat when it's created. If either read failed, we'd log it and use the built-in default, or skip the organization prompt, so the chat ignored the admin's instructions for its whole life. The subagent model override had the same problem, and the compaction and advisor overrides quietly fell back to the chat model for that turn. To fix, we'll just return the database error instead. Overrides that can't be used, e.g. the model was deleted or the user has no API key for its provider, still fall back like they do today.
This commit is contained in:
+57
-92
@@ -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) {
|
||||
|
||||
@@ -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.
|
||||
|
||||
+22
-23
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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 {
|
||||
|
||||
+24
-31
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user