From 51218e96ed43c21da26902ece6fa9e4467ac21b5 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Tue, 11 Aug 2026 18:44:29 +0000 Subject: [PATCH 01/13] test(coderd/x/chatd): lock model-call request shapes before resolver refactor --- .../x/chatd/modelcall_shape_internal_test.go | 347 ++++++++++++++++++ 1 file changed, 347 insertions(+) create mode 100644 coderd/x/chatd/modelcall_shape_internal_test.go diff --git a/coderd/x/chatd/modelcall_shape_internal_test.go b/coderd/x/chatd/modelcall_shape_internal_test.go new file mode 100644 index 00000000000..31a4efe8e28 --- /dev/null +++ b/coderd/x/chatd/modelcall_shape_internal_test.go @@ -0,0 +1,347 @@ +package chatd //nolint:testpackage // Locks unexported model-call construction behavior. + +import ( + "context" + "encoding/json" + "io" + "net/http" + "strconv" + "strings" + "sync" + "testing" + "time" + + "charm.land/fantasy" + fantasyopenai "charm.land/fantasy/providers/openai" + "github.com/google/uuid" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + "cdr.dev/slog/v3/sloggers/slogtest" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbgen" + "github.com/coder/coder/v2/coderd/database/dbmock" + "github.com/coder/coder/v2/coderd/database/dbtestutil" + "github.com/coder/coder/v2/coderd/util/ptr" + "github.com/coder/coder/v2/coderd/x/chatd/chatprompt" + "github.com/coder/coder/v2/coderd/x/chatd/chatprovider" + "github.com/coder/coder/v2/coderd/x/chatd/chatstate" + "github.com/coder/coder/v2/coderd/x/chatd/chattest" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/testutil" + "github.com/coder/quartz" +) + +// The tests in this file lock the outgoing request shape of each LLM call +// flow: which flows carry model-config provider options and which +// deliberately omit them, plus token defaults. They guard the model-call +// resolver refactor against silent behavior changes. + +func modelCallSentinelOptions(t *testing.T, user string) json.RawMessage { + t.Helper() + raw, err := json.Marshal(codersdk.ChatModelCallConfig{ + ProviderOptions: &codersdk.ChatModelProviderOptions{ + OpenAI: &codersdk.ChatModelOpenAIProviderOptions{ + User: ptr.Ref(user), + }, + }, + }) + require.NoError(t, err) + return raw +} + +func openAIResponsesObjectBody(t *testing.T, object string) string { + t.Helper() + text := strconv.Quote(object) + return `{"id":"resp_test","object":"response","created_at":0,"status":"completed","model":"gpt-4o-mini","output":[{"id":"msg_test","type":"message","role":"assistant","content":[{"type":"output_text","text":` + text + `}]}],"usage":{"input_tokens":1,"output_tokens":1,"total_tokens":2}}` +} + +func TestModelCallShapeStandardTurn(t *testing.T) { + t.Parallel() + + db, ps := dbtestutil.NewDB(t) + ctx := chatdTestContext(t) + user := dbgen.User(t, db, database.User{}) + org := dbgen.Organization(t, db, database.Organization{}) + dbgen.OrganizationMember(t, db, database.OrganizationMember{ + UserID: user.ID, + OrganizationID: org.ID, + }) + provider := dbgen.AIProviderWithOptionalKey(t, db, database.AIProvider{ + Type: database.AIProviderTypeOpenai, + }, "test-key") + modelConfig := dbgen.ChatModelConfig(t, db, database.ChatModelConfig{ + Model: "gpt-4o-mini", + Options: modelCallSentinelOptions(t, "turn-options-sentinel"), + AIProviderID: uuid.NullUUID{UUID: provider.ID, Valid: true}, + }, func(p *database.InsertChatModelConfigParams) { + p.Enabled = true + }) + + created, err := chatstate.CreateChat(ctx, db, ps, chatstate.CreateChatInput{ + OrganizationID: org.ID, + OwnerID: user.ID, + LastModelConfigID: modelConfig.ID, + Title: "standard turn request shape", + ClientType: database.ChatClientTypeApi, + InitialMessages: []chatstate.Message{ + { + Role: database.ChatMessageRoleUser, + Content: mustMarshalText(t, "hello"), + Visibility: database.ChatMessageVisibilityBoth, + ModelConfigID: uuid.NullUUID{UUID: modelConfig.ID, Valid: true}, + CreatedBy: uuid.NullUUID{UUID: user.ID, Valid: true}, + ContentVersion: chatprompt.CurrentContentVersion, + }, + }, + }) + require.NoError(t, err) + + server := newInternalTestServer( + t, + db, + ps, + chatprovider.ProviderAPIKeys{}, + withInternalTestServerTransportFactory(&aibridgeTestFactory{}), + ) + prepared, err := server.prepareGeneration(ctx, generationPrepareInput{ + Chat: created.Chat, + Messages: created.InitialMessages, + }) + require.NoError(t, err) + t.Cleanup(prepared.Cleanup) + + providerOptions, ok := prepared.ProviderOptions[fantasyopenai.Name].(*fantasyopenai.ResponsesProviderOptions) + require.True(t, ok, "%T", prepared.ProviderOptions[fantasyopenai.Name]) + require.NotNil(t, providerOptions.User) + require.Equal(t, "turn-options-sentinel", *providerOptions.User) + + require.NotNil(t, prepared.ModelConfig.MaxOutputTokens) + require.Equal(t, int64(32_000), *prepared.ModelConfig.MaxOutputTokens) + + // The chat-model compaction summary call carries no provider options + // even when the model config has them. + require.NotNil(t, prepared.Compaction) + require.Nil(t, prepared.Compaction.Options.ProviderOptions) +} + +func TestModelCallShapeManualTitleCarriesProviderOptions(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + ctrl := gomock.NewController(t) + db := dbmock.NewMockStore(ctrl) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + chat, messages := titleOverrideTestChatAndMessages(t) + chat.OrganizationID = uuid.New() + overrideConfig := titleOverrideModelConfig("gpt-4.1", true) + providerID := uuid.New() + overrideConfig.AIProviderID = uuid.NullUUID{UUID: providerID, Valid: true} + overrideConfig.Options = modelCallSentinelOptions(t, "title-options-sentinel") + provider := database.AIProvider{ + ID: providerID, + Name: "primary-openai", + Type: database.AIProviderTypeOpenai, + Enabled: true, + } + + var ( + bodyMu sync.Mutex + bodies [][]byte + ) + factory := &aibridgeTestFactory{rt: roundTripFunc(func(req *http.Request) (*http.Response, error) { + bodyBytes, err := io.ReadAll(req.Body) + require.NoError(t, err) + bodyMu.Lock() + bodies = append(bodies, bodyBytes) + bodyMu.Unlock() + body := openAIResponsesObjectBody(t, `{"title":"Locked title"}`) + return &http.Response{ + StatusCode: http.StatusOK, + Header: http.Header{"Content-Type": []string{"application/json"}}, + Body: io.NopCloser(strings.NewReader(body)), + Request: req, + }, nil + })} + + db.EXPECT().GetChatMessagesByChatIDAscPaginated(gomock.Any(), database.GetChatMessagesByChatIDAscPaginatedParams{ + ChatID: chat.ID, + AfterID: 0, + LimitVal: manualTitleMessageWindowLimit, + }).Return(messages, nil) + db.EXPECT().GetChatMessagesByChatIDDescPaginated(gomock.Any(), database.GetChatMessagesByChatIDDescPaginatedParams{ + ChatID: chat.ID, + BeforeID: 0, + LimitVal: manualTitleMessageWindowLimit, + }).Return(nil, nil) + db.EXPECT().GetChatGatewayAPIKey(gomock.Any(), database.GetChatGatewayAPIKeyParams{ + UserID: chat.OwnerID, + TokenName: GatewayTokenName(chat.OwnerID), + }).Return(database.APIKey{ + ID: uuid.NewString(), + UserID: chat.OwnerID, + ExpiresAt: time.Now().Add(48 * time.Hour), + }, nil) + db.EXPECT().GetChatTitleGenerationModelOverride(gomock.Any()).Return(overrideConfig.ID.String(), nil) + db.EXPECT().GetChatModelConfigByID(gomock.Any(), overrideConfig.ID).Return(overrideConfig, nil) + db.EXPECT().GetAIProviderByID(gomock.Any(), providerID).Return(provider, nil).AnyTimes() + db.EXPECT().GetAIProviderKeysByProviderID(gomock.Any(), providerID).Return([]database.AIProviderKey{{ + ProviderID: providerID, + APIKey: "test-key", + }}, nil).AnyTimes() + + server := titleOverrideTestServer(db, logger) + server.clock = quartz.NewReal() + server.aibridgeTransportFactory = aibridgeTestFactoryPointer(factory) + title, err := server.generateManualTitleCandidate(ctx, db, chat) + require.NoError(t, err) + require.Equal(t, "Locked title", title) + + bodyMu.Lock() + defer bodyMu.Unlock() + require.Len(t, bodies, 1) + var raw map[string]any + require.NoError(t, json.Unmarshal(bodies[0], &raw)) + require.Equal(t, "title-options-sentinel", raw["user"]) +} + +func TestModelCallShapeChatSummaryOmitsProviderOptions(t *testing.T) { + t.Parallel() + + db, ps := dbtestutil.NewDB(t) + ctx := chatdTestContext(t) + user := dbgen.User(t, db, database.User{}) + org := dbgen.Organization(t, db, database.Organization{}) + dbgen.OrganizationMember(t, db, database.OrganizationMember{ + UserID: user.ID, + OrganizationID: org.ID, + }) + provider := dbgen.AIProviderWithOptionalKey(t, db, database.AIProvider{ + Type: database.AIProviderTypeOpenai, + }, "test-key") + modelConfig := dbgen.ChatModelConfig(t, db, database.ChatModelConfig{ + Model: "gpt-4o-mini", + Options: modelCallSentinelOptions(t, "summary-options-sentinel"), + AIProviderID: uuid.NullUUID{UUID: provider.ID, Valid: true}, + }, func(p *database.InsertChatModelConfigParams) { + p.Enabled = true + }) + + longPrompt := strings.Repeat("please summarize this conversation carefully ", 10) + created, err := chatstate.CreateChat(ctx, db, ps, chatstate.CreateChatInput{ + OrganizationID: org.ID, + OwnerID: user.ID, + LastModelConfigID: modelConfig.ID, + Title: "summary request shape", + ClientType: database.ChatClientTypeApi, + InitialMessages: []chatstate.Message{ + { + Role: database.ChatMessageRoleUser, + Content: mustMarshalText(t, longPrompt), + Visibility: database.ChatMessageVisibilityBoth, + ModelConfigID: uuid.NullUUID{UUID: modelConfig.ID, Valid: true}, + CreatedBy: uuid.NullUUID{UUID: user.ID, Valid: true}, + ContentVersion: chatprompt.CurrentContentVersion, + }, + }, + }) + require.NoError(t, err) + + var ( + bodyMu sync.Mutex + bodies [][]byte + ) + factory := &aibridgeTestFactory{rt: roundTripFunc(func(req *http.Request) (*http.Response, error) { + bodyBytes, err := io.ReadAll(req.Body) + require.NoError(t, err) + bodyMu.Lock() + bodies = append(bodies, bodyBytes) + bodyMu.Unlock() + body := openAIResponsesObjectBody(t, `{"summary":"A locked summary."}`) + return &http.Response{ + StatusCode: http.StatusOK, + Header: http.Header{"Content-Type": []string{"application/json"}}, + Body: io.NopCloser(strings.NewReader(body)), + Request: req, + }, nil + })} + + server := newInternalTestServer( + t, + db, + ps, + chatprovider.ProviderAPIKeys{}, + withInternalTestServerTransportFactory(factory), + ) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + server.generateAndStoreChatSummary(ctx, logger, created.Chat) + + fetched, err := db.GetChatByID(ctx, created.Chat.ID) + require.NoError(t, err) + require.True(t, fetched.Summary.Valid) + require.Equal(t, "A locked summary.", fetched.Summary.String) + + bodyMu.Lock() + defer bodyMu.Unlock() + require.Len(t, bodies, 1) + var raw map[string]any + require.NoError(t, json.Unmarshal(bodies[0], &raw)) + // The summary call omits model-config provider options entirely. + require.NotContains(t, raw, "user") +} + +func TestModelCallShapeTurnStatusLabelEnvelope(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + + var ( + callMu sync.Mutex + captured []fantasy.ObjectCall + ) + model := &chattest.FakeModel{ + ProviderName: fantasyopenai.Name, + ModelName: "gpt-4o-mini", + GenerateObjectFn: func(_ context.Context, call fantasy.ObjectCall) (*fantasy.ObjectResponse, error) { + callMu.Lock() + captured = append(captured, call) + callMu.Unlock() + return &fantasy.ObjectResponse{ + Object: map[string]any{"label": "Finished the tests"}, + }, nil + }, + } + + server := &Server{logger: logger} + label := server.generateTurnStatusLabel( + ctx, + database.Chat{ID: uuid.New(), OwnerID: uuid.New(), Title: "status shape"}, + database.ChatStatusWaiting, + "All tests pass now.", + fantasyopenai.Name, + "gpt-4o-mini", + chatprovider.NewModel(model, nil), + aiGatewayModelRoute{}, + modelBuildOptions{}, + modelCallSentinelOptions(t, "status-options-sentinel"), + logger, + nil, + 0, + 0, + ) + require.Equal(t, "Finished the tests", label) + + callMu.Lock() + defer callMu.Unlock() + require.Len(t, captured, 1) + call := captured[0] + // The status-label call omits model-config provider options even though + // the config JSON carries them. + require.Nil(t, call.ProviderOptions) + require.NotNil(t, call.MaxOutputTokens) + require.Equal(t, int64(64), *call.MaxOutputTokens) + require.NotNil(t, call.Temperature) + require.Equal(t, quickgenTemperature, *call.Temperature) + require.Equal(t, "propose_turn_status_label", call.SchemaName) +} From 2b6cda0c3a8afb4a331cd98242eb8cc780f1dbd1 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Tue, 11 Aug 2026 19:00:22 +0000 Subject: [PATCH 02/13] refactor(coderd/x/chatd): add model-call resolver and migrate standard turn Add modelCallSpec/resolveModelCall as the single pipeline from a call spec to a ready model client: config selection, call-config parse, route and identity resolution, client construction with debug recording, and provider-option derivation. Migrate prepareGeneration and the computer-use substitution onto the resolver, delete resolveComputerUseModel, and turn resolveChatModel into a transitional wrapper for the remaining quickgen callers. --- coderd/x/chatd/chatd.go | 40 +-- coderd/x/chatd/computer_use.go | 48 --- coderd/x/chatd/generation_preparer.go | 82 ++--- coderd/x/chatd/model_routing_internal_test.go | 65 ++-- coderd/x/chatd/modelcall.go | 332 ++++++++++++++++++ 5 files changed, 389 insertions(+), 178 deletions(-) create mode 100644 coderd/x/chatd/modelcall.go diff --git a/coderd/x/chatd/chatd.go b/coderd/x/chatd/chatd.go index 792e72bcd6a..33cc2264e77 100644 --- a/coderd/x/chatd/chatd.go +++ b/coderd/x/chatd/chatd.go @@ -4026,6 +4026,9 @@ func buildProviderTools(options *codersdk.ChatModelProviderOptions) []chatloop.P return tools } +// resolveChatModel resolves the chat's model without deriving per-call +// provider options. Transitional wrapper over resolveModelCall; remaining +// callers migrate to purpose-specific specs. func (p *Server) resolveChatModel( ctx context.Context, chat database.Chat, @@ -4039,44 +4042,11 @@ func (p *Server) resolveChatModel( resolvedModel string, err error, ) { - dbConfig, err = p.resolveModelConfig(ctx, chat) - if err != nil { - return chatprovider.Model{}, database.ChatModelConfig{}, aiGatewayModelRoute{}, false, "", "", xerrors.Errorf("resolve model config: %w", err) - } - - if !dbConfig.Enabled { - return chatprovider.Model{}, database.ChatModelConfig{}, aiGatewayModelRoute{}, false, "", "", xerrors.Errorf("chat model config %s is disabled", dbConfig.ID) - } - - route, err = p.resolveModelRouteForConfig(ctx, chat.OwnerID, dbConfig) + resolved, err := p.resolveModelCall(ctx, chatModelSpec(callPurposeStandardTurn, chat, modelOpts)) if err != nil { return chatprovider.Model{}, database.ChatModelConfig{}, aiGatewayModelRoute{}, false, "", "", err } - - providerHint := route.ModelProviderHint - resolvedProvider, resolvedModel, err = chatprovider.ResolveModelWithProviderHint( - dbConfig.Model, - providerHint, - ) - if err != nil { - return chatprovider.Model{}, database.ChatModelConfig{}, aiGatewayModelRoute{}, false, "", "", xerrors.Errorf( - "resolve model metadata: %w", err, - ) - } - - model, debugEnabled, err = p.newDebugAwareModel(ctx, modelClientRequest{ - Chat: chat, - ModelName: dbConfig.Model, - UserAgent: chatprovider.UserAgent(), - ExtraHeaders: chatprovider.CoderHeaders(chat), - ConfigOptions: dbConfig.Options, - }, route, modelOpts) - if err != nil { - return chatprovider.Model{}, database.ChatModelConfig{}, aiGatewayModelRoute{}, false, "", "", xerrors.Errorf( - "create model: %w", err, - ) - } - return model, dbConfig, route, debugEnabled, resolvedProvider, resolvedModel, nil + return resolved.model, resolved.dbConfig, resolved.route, resolved.debugEnabled, resolved.resolvedProvider, resolved.resolvedModel, nil } func (p *Server) aiProviderConfig(ctx context.Context, provider database.AIProvider) (chatprovider.ConfiguredProvider, error) { diff --git a/coderd/x/chatd/computer_use.go b/coderd/x/chatd/computer_use.go index 249f4b3ae33..5e4ab54e6c7 100644 --- a/coderd/x/chatd/computer_use.go +++ b/coderd/x/chatd/computer_use.go @@ -7,11 +7,9 @@ import ( "golang.org/x/xerrors" "cdr.dev/slog/v3" - "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/x/chatd/chatloop" openaicomputeruse "github.com/coder/coder/v2/coderd/x/chatd/chatopenai/computeruse" - "github.com/coder/coder/v2/coderd/x/chatd/chatprovider" "github.com/coder/coder/v2/coderd/x/chatd/chattool" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/workspacesdk" @@ -56,52 +54,6 @@ func (p *Server) computerUseProviderAndModelFromConfig( return provider, modelProvider, modelName, nil } -func (p *Server) resolveComputerUseModel( - ctx context.Context, - chat database.Chat, - route aiGatewayModelRoute, - computerUseProvider codersdk.ChatComputerUseProvider, - computerUseModelProvider string, - computerUseModelName string, - modelOpts modelBuildOptions, -) ( - model chatprovider.Model, - debugEnabled bool, - resolvedProvider string, - resolvedModel string, - err error, -) { - resolvedProvider, resolvedModel, err = chatprovider.ResolveModelWithProviderHint( - computerUseModelName, - computerUseModelProvider, - ) - if err != nil { - return chatprovider.Model{}, false, "", "", xerrors.Errorf( - "resolve computer use model metadata for provider %q model %q: %w", - computerUseProvider, - computerUseModelName, - err, - ) - } - - model, debugEnabled, err = p.newDebugAwareModel(ctx, modelClientRequest{ - Chat: chat, - ModelName: computerUseModelName, - UserAgent: chatprovider.UserAgent(), - ExtraHeaders: chatprovider.CoderHeaders(chat), - }, route, modelOpts) - if err != nil { - return chatprovider.Model{}, false, "", "", xerrors.Errorf( - "resolve computer use model for provider %q model %q: %w", - computerUseProvider, - computerUseModelName, - err, - ) - } - - return model, debugEnabled, resolvedProvider, resolvedModel, nil -} - type computerUseProviderToolOptions struct { provider codersdk.ChatComputerUseProvider isPlanModeTurn bool diff --git a/coderd/x/chatd/generation_preparer.go b/coderd/x/chatd/generation_preparer.go index 6602d11c84b..9e48978d09c 100644 --- a/coderd/x/chatd/generation_preparer.go +++ b/coderd/x/chatd/generation_preparer.go @@ -2,7 +2,6 @@ package chatd import ( "context" - "encoding/json" "slices" "strings" "sync" @@ -83,17 +82,9 @@ func (server *Server) prepareGeneration( ) var ( - model chatprovider.Model - modelConfig database.ChatModelConfig - modelRoute aiGatewayModelRoute - modelOpts modelBuildOptions - callConfig codersdk.ChatModelCallConfig - promptRows []database.ChatMessage - mcpConfigs []database.MCPServerConfig - mcpTokens []database.MCPServerUserToken - debugEnabled bool - resolvedProvider string - debugModel string + promptRows []database.ChatMessage + mcpConfigs []database.MCPServerConfig + mcpTokens []database.MCPServerUserToken ) var g errgroup.Group @@ -118,22 +109,12 @@ func (server *Server) prepareGeneration( if err != nil { return generationPrepared{}, xerrors.Errorf("ensure synthetic API key: %w", err) } - modelOpts = modelBuildOptions{ActiveAPIKeyID: apiKeyID} + modelOpts := modelBuildOptions{ActiveAPIKeyID: apiKeyID} - model, modelConfig, modelRoute, debugEnabled, resolvedProvider, debugModel, err = server.resolveChatModel(ctx, chat, modelOpts) + resolved, err := server.resolveModelCall(ctx, standardTurnSpec(chat, modelOpts)) if err != nil { return generationPrepared{}, err } - if len(modelConfig.Options) > 0 { - if err := json.Unmarshal(modelConfig.Options, &callConfig); err != nil { - return generationPrepared{}, xerrors.Errorf("parse model call config: %w", err) - } - } - - if callConfig.MaxOutputTokens == nil { - maxOutputTokens := int64(32_000) - callConfig.MaxOutputTokens = &maxOutputTokens - } // Computer-use turns swap in a specialized model, so the substitution // must happen before anything model-sensitive runs: file-part @@ -147,28 +128,31 @@ func (server *Server) prepareGeneration( if err != nil { return generationPrepared{}, xerrors.Errorf("resolve computer use provider and model: %w", err) } - computerUseRoute, keyErr := server.resolveModelRouteForProviderType(ctx, chat.OwnerID, cuModelProvider) - if keyErr != nil { - return generationPrepared{}, xerrors.Errorf("resolve computer use provider route: %w", keyErr) - } - modelRoute = computerUseRoute - cuModel, cuDebugEnabled, cuResolvedProvider, cuResolvedModel, cuErr := server.resolveComputerUseModel( - ctx, + cuResolved, cuErr := server.resolveModelCall(ctx, computerUseSpec( chat, - computerUseRoute, - computerUseProvider, cuModelProvider, cuModelName, + resolved.callConfig, modelOpts, - ) + )) if cuErr != nil { - return generationPrepared{}, cuErr + return generationPrepared{}, xerrors.Errorf( + "resolve computer use model for provider %q model %q: %w", + computerUseProvider, + cuModelName, + cuErr, + ) } - model = cuModel - debugEnabled = cuDebugEnabled - resolvedProvider = cuResolvedProvider - debugModel = cuResolvedModel + // The chat model's config row keeps driving compaction, history + // sanitization, and debug attribution; only the client and its + // call identity are swapped. + cuResolved.dbConfig = resolved.dbConfig + resolved = cuResolved } + model := resolved.model + modelConfig := resolved.dbConfig + callConfig := resolved.callConfig + modelRoute := resolved.route currentPlanMode := chat.PlanMode isPlanModeTurn := currentPlanMode.Valid && currentPlanMode.ChatPlanMode == database.ChatPlanModePlan @@ -580,12 +564,6 @@ func (server *Server) prepareGeneration( } } - var requestedEffort *string - if chat.LastReasoningEffort.Valid { - requestedEffort = new(string(chat.LastReasoningEffort.ChatReasoningEffort)) - } - providerOptions := chatprovider.ProviderOptionsForCall(model, callConfig, requestedEffort) - activeToolNames := activeToolNamesForTurn(tools, currentPlanMode, chat.ParentChatID, approvedPlanMCPConfigIDs) if isExploreSubagent { activeToolNames = allowedExploreToolNames(tools) @@ -601,7 +579,7 @@ func (server *Server) prepareGeneration( triggerMessageID, historyTipMessageID, triggerLabel := deriveChatDebugSeed(promptRows) debugSvc := server.existingDebugService() var debug *generationDebug - if debugEnabled { + if resolved.debugEnabled { if debugSvc == nil { cleanup() return generationPrepared{}, xerrors.New("chat debug service missing after enablement check") @@ -609,8 +587,8 @@ func (server *Server) prepareGeneration( debug = &generationDebug{ Enabled: true, Service: debugSvc, - Provider: resolvedProvider, - Model: debugModel, + Provider: resolved.resolvedProvider, + Model: resolved.resolvedModel, TriggerMessageID: triggerMessageID, HistoryTipMessageID: historyTipMessageID, TriggerLabel: triggerLabel, @@ -653,8 +631,8 @@ func (server *Server) prepareGeneration( DebugSvc: debugSvc, ChatID: chat.ID, HistoryTipMessageID: historyTipMessageID, - ResolvedProvider: resolvedProvider, - ResolvedModel: debugModel, + ResolvedProvider: resolved.resolvedProvider, + ResolvedModel: resolved.resolvedModel, ModelConfigID: modelConfig.ID, StepUsage: compactionStepUsage, } @@ -678,10 +656,10 @@ func (server *Server) prepareGeneration( ProviderTools: providerTools, ModelRoute: modelRoute, ModelBuildOptions: modelOpts, - ResolvedProvider: resolvedProvider, + ResolvedProvider: resolved.resolvedProvider, ModelConfigID: modelConfig.ID, ModelConfig: callConfig, - ProviderOptions: providerOptions, + ProviderOptions: resolved.providerOptions, ContextLimitFallback: modelConfig.ContextLimit, DynamicToolNames: dynamicToolNames, StopAfterTools: stopAfterBehaviorTools(currentPlanMode, chat.Mode, chat.ParentChatID), diff --git a/coderd/x/chatd/model_routing_internal_test.go b/coderd/x/chatd/model_routing_internal_test.go index 6a6d1324ee6..f938b4dd8ae 100644 --- a/coderd/x/chatd/model_routing_internal_test.go +++ b/coderd/x/chatd/model_routing_internal_test.go @@ -652,20 +652,15 @@ func TestAIBridgeComputerUseModelUsesRoute(t *testing.T) { require.True(t, ok) ctx := aibridge.WithDelegatedAPIKeyID(t.Context(), "context-key-must-be-ignored") - model, debugEnabled, resolvedProvider, resolvedModel, err := server.resolveComputerUseModel( - ctx, - chat, - aibridgeTestRoute(aibridgeTestAIProvider(providerID, "primary-openai", database.AIProviderTypeOpenai)), - provider, - modelProvider, - modelName, - modelBuildOptions{ActiveAPIKeyID: apiKeyID}, - ) + spec := computerUseSpec(chat, modelProvider, modelName, codersdk.ChatModelCallConfig{}, modelBuildOptions{ActiveAPIKeyID: apiKeyID}) + route := aibridgeTestRoute(aibridgeTestAIProvider(providerID, "primary-openai", database.AIProviderTypeOpenai)) + spec.routeOverride = &route + resolved, err := server.resolveModelCall(ctx, spec) require.NoError(t, err) - require.True(t, model.Valid()) - require.False(t, debugEnabled) - require.EqualValues(t, codersdk.ChatComputerUseProviderOpenAI, resolvedProvider) - require.Equal(t, modelName, resolvedModel) + require.True(t, resolved.model.Valid()) + require.False(t, resolved.debugEnabled) + require.EqualValues(t, codersdk.ChatComputerUseProviderOpenAI, resolved.resolvedProvider) + require.Equal(t, modelName, resolved.resolvedModel) gotProvider, gotSource := factory.recorded() require.Equal(t, "primary-openai", gotProvider) require.Equal(t, aibridge.SourceAgents, gotSource) @@ -674,7 +669,7 @@ func TestAIBridgeComputerUseModelUsesRoute(t *testing.T) { // The computer-use model is a hardcoded default with no config of its own, so // its transport must come from its own client rather than inheriting the chat // model's openai_config. Request preparation reads the same value back. -func TestResolveComputerUseModel_TransportIndependentOfChatConfig(t *testing.T) { +func TestComputerUseModelCall_TransportIndependentOfChatConfig(t *testing.T) { t.Parallel() providerID := uuid.New() @@ -689,20 +684,14 @@ func TestResolveComputerUseModel_TransportIndependentOfChatConfig(t *testing.T) modelProvider, modelName, ok := chattool.DefaultComputerUseModel(provider) require.True(t, ok) - //nolint:dogsled // Only the built model matters for the transport assertion. - model, _, _, _, err := server.resolveComputerUseModel( - t.Context(), - chat, - aibridgeTestRoute(aibridgeTestAIProvider(providerID, "primary-openai", database.AIProviderTypeOpenai)), - provider, - modelProvider, - modelName, - modelBuildOptions{ActiveAPIKeyID: uuid.NewString()}, - ) + spec := computerUseSpec(chat, modelProvider, modelName, codersdk.ChatModelCallConfig{}, modelBuildOptions{ActiveAPIKeyID: uuid.NewString()}) + route := aibridgeTestRoute(aibridgeTestAIProvider(providerID, "primary-openai", database.AIProviderTypeOpenai)) + spec.routeOverride = &route + resolved, err := server.resolveModelCall(t.Context(), spec) require.NoError(t, err) wantTransport := chatopenai.TransportFor(modelProvider, modelName, nil) - require.Equal(t, wantTransport, model.Transport()) + require.Equal(t, wantTransport, resolved.model.Transport()) // The assertion above only has teeth if an override could have changed the // result for this model. @@ -710,7 +699,7 @@ func TestResolveComputerUseModel_TransportIndependentOfChatConfig(t *testing.T) require.NotEqual(t, wantTransport, chatopenai.TransportFor(modelProvider, modelName, &opposite)) } -func TestResolveComputerUseModel_AIGatewayMissingAPIKeyID(t *testing.T) { +func TestComputerUseModelCall_AIGatewayMissingAPIKeyID(t *testing.T) { t.Parallel() providerID := uuid.New() @@ -726,24 +715,14 @@ func TestResolveComputerUseModel_AIGatewayMissingAPIKeyID(t *testing.T) { modelProvider, modelName, ok := chattool.DefaultComputerUseModel(provider) require.True(t, ok) - model, debugEnabled, resolvedProvider, resolvedModel, err := server.resolveComputerUseModel( - t.Context(), - chat, - aibridgeTestRoute(aibridgeTestAIProvider(providerID, "primary-openai", database.AIProviderTypeOpenai)), - provider, - modelProvider, - modelName, - modelBuildOptions{}, // no ActiveAPIKeyID - ) + spec := computerUseSpec(chat, modelProvider, modelName, codersdk.ChatModelCallConfig{}, modelBuildOptions{}) // no ActiveAPIKeyID + route := aibridgeTestRoute(aibridgeTestAIProvider(providerID, "primary-openai", database.AIProviderTypeOpenai)) + spec.routeOverride = &route + resolved, err := server.resolveModelCall(t.Context(), spec) require.Error(t, err) - require.False(t, model.Valid()) - require.False(t, debugEnabled) - require.Empty(t, resolvedProvider) - require.Empty(t, resolvedModel) - require.Contains(t, err.Error(), fmt.Sprintf( - `resolve computer use model for provider "openai" model %q`, - chattool.ComputerUseOpenAIModelName, - )) + require.False(t, resolved.model.Valid()) + require.False(t, resolved.debugEnabled) + require.Contains(t, err.Error(), "create model") require.Contains(t, err.Error(), "active turn API key ID") } diff --git a/coderd/x/chatd/modelcall.go b/coderd/x/chatd/modelcall.go new file mode 100644 index 00000000000..0b977379543 --- /dev/null +++ b/coderd/x/chatd/modelcall.go @@ -0,0 +1,332 @@ +package chatd + +import ( + "context" + + "charm.land/fantasy" + "golang.org/x/xerrors" + + "cdr.dev/slog/v3" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbauthz" + "github.com/coder/coder/v2/coderd/util/ptr" + "github.com/coder/coder/v2/coderd/x/chatd/chatdebug" + "github.com/coder/coder/v2/coderd/x/chatd/chatprovider" + "github.com/coder/coder/v2/codersdk" +) + +// callPurpose labels the flow a model call serves. It only feeds logging and +// debug attribution; behavior is driven by the other modelCallSpec fields. +type callPurpose string + +const ( + callPurposeStandardTurn callPurpose = "standard_turn" + callPurposeComputerUse callPurpose = "computer_use" + callPurposeTitle callPurpose = "title" + callPurposeSummary callPurpose = "chat_summary" + callPurposeStatusLabel callPurpose = "turn_status_label" + callPurposeCompaction callPurpose = "compaction" + callPurposeAdvisor callPurpose = "advisor" + callPurposeDebugRebuild callPurpose = "debug_rebuild" +) + +// defaultChatMaxOutputTokens caps standard-turn output when the model config +// leaves MaxOutputTokens unset. +const defaultChatMaxOutputTokens = int64(32_000) + +type configSelectionMode int + +const ( + // configFromChat resolves the chat's last model config, falling back to + // the deployment default. The config must be enabled. + configFromChat configSelectionMode = iota + // configExplicit uses a config row the caller already selected (override + // and preferred-model flows own their selection and fallback policy). + configExplicit + // configFixedModel builds a client for a provider/model pair without a + // config row (computer use, debug transport rebuilds). + configFixedModel +) + +type configSelection struct { + mode configSelectionMode + config database.ChatModelConfig + // providerType routes configFixedModel calls when no route override is + // supplied. + providerType string + modelName string + // configOptions is the raw options JSON applied to client construction + // for configFixedModel (beta headers, OpenAI transport override). + configOptions []byte + // callConfig supplies the per-call config for configFixedModel option + // derivation, since there is no config row to parse. + callConfig codersdk.ChatModelCallConfig +} + +type debugPolicy int + +const ( + // debugPolicyOff builds a plain client with no debug recording. + debugPolicyOff debugPolicy = iota + // debugPolicyAware records HTTP traffic and wraps the model when the + // chat debug service enables this chat. + debugPolicyAware + // debugPolicyForced always records and wraps; used to rebuild a debug + // transport after the caller verified debug is enabled. + debugPolicyForced +) + +type providerOptionPolicy int + +const ( + // providerOptionsDerive converts the call config into per-call provider + // options. + providerOptionsDerive providerOptionPolicy = iota + // providerOptionsOmit skips derivation. Used by flows that historically + // never sent provider options and by callers that derive separately. + providerOptionsOmit +) + +// modelCallSpec describes one LLM call to resolve: which config to use, how +// to route it, and which construction policies apply. Build specs via the +// purpose-specific constructors so per-flow policy stays declared in one +// place. +type modelCallSpec struct { + purpose callPurpose + chat database.Chat + config configSelection + requestedEffort *string + providerOptions providerOptionPolicy + debug debugPolicy + // debugSvc, debugWrapProvider, and debugWrapModel label forced debug + // recordings; callers keep their historical attribution labels. + debugSvc *chatdebug.Service + debugWrapProvider string + debugWrapModel string + // routeOverride reuses a previously resolved route instead of resolving + // one (debug transport rebuilds). + routeOverride *aiGatewayModelRoute + // chatdScopedRoute resolves the route with chatd scope. Deployment-wide + // override models must route for user-owned chats regardless of the + // caller's actor. + chatdScopedRoute bool + // defaultMaxOutputTokens applies the standard-turn output cap when the + // config leaves MaxOutputTokens unset. + defaultMaxOutputTokens bool + buildOptions modelBuildOptions +} + +// chatRequestedEffort is the user's per-turn reasoning effort choice, which +// the config's bounds clamp during option derivation. +func chatRequestedEffort(chat database.Chat) *string { + if !chat.LastReasoningEffort.Valid { + return nil + } + return new(string(chat.LastReasoningEffort.ChatReasoningEffort)) +} + +func standardTurnSpec(chat database.Chat, buildOpts modelBuildOptions) modelCallSpec { + return modelCallSpec{ + purpose: callPurposeStandardTurn, + chat: chat, + config: configSelection{mode: configFromChat}, + requestedEffort: chatRequestedEffort(chat), + providerOptions: providerOptionsDerive, + debug: debugPolicyAware, + defaultMaxOutputTokens: true, + buildOptions: buildOpts, + } +} + +// chatModelSpec resolves the chat's model without deriving provider options +// or applying the standard-turn token default. Callers that need per-call +// options derive them from their own spec. +func chatModelSpec(purpose callPurpose, chat database.Chat, buildOpts modelBuildOptions) modelCallSpec { + return modelCallSpec{ + purpose: purpose, + chat: chat, + config: configSelection{mode: configFromChat}, + providerOptions: providerOptionsOmit, + debug: debugPolicyAware, + buildOptions: buildOpts, + } +} + +// computerUseSpec swaps in the deployment's computer-use model. The client is +// built without config options because the fixed model has no config row; the +// chat model's call config still drives per-call provider options so admin +// tuning follows the turn. +func computerUseSpec( + chat database.Chat, + modelProvider string, + modelName string, + chatCallConfig codersdk.ChatModelCallConfig, + buildOpts modelBuildOptions, +) modelCallSpec { + return modelCallSpec{ + purpose: callPurposeComputerUse, + chat: chat, + config: configSelection{ + mode: configFixedModel, + providerType: modelProvider, + modelName: modelName, + callConfig: chatCallConfig, + }, + requestedEffort: chatRequestedEffort(chat), + providerOptions: providerOptionsDerive, + debug: debugPolicyAware, + buildOptions: buildOpts, + } +} + +// resolvedModelCall is the output of resolveModelCall: a ready client plus +// the metadata callers need for prompts, metrics, and debug attribution. +type resolvedModelCall struct { + model chatprovider.Model + dbConfig database.ChatModelConfig + callConfig codersdk.ChatModelCallConfig + // providerOptions is nil when the spec's policy omits derivation. + providerOptions fantasy.ProviderOptions + resolvedProvider string + resolvedModel string + route aiGatewayModelRoute + debugEnabled bool +} + +// resolveModelCall is the single pipeline from a call spec to a ready model +// client: config selection, call-config parse, route and identity resolution, +// client construction (including debug recording), and provider-option +// derivation. +func (p *Server) resolveModelCall(ctx context.Context, spec modelCallSpec) (resolvedModelCall, error) { + out := resolvedModelCall{} + + var modelName string + var configOptions []byte + switch spec.config.mode { + case configFromChat: + dbConfig, err := p.resolveModelConfig(ctx, spec.chat) + if err != nil { + return resolvedModelCall{}, xerrors.Errorf("resolve model config: %w", err) + } + if !dbConfig.Enabled { + return resolvedModelCall{}, xerrors.Errorf("chat model config %s is disabled", dbConfig.ID) + } + out.dbConfig = dbConfig + modelName = dbConfig.Model + configOptions = dbConfig.Options + case configExplicit: + out.dbConfig = spec.config.config + modelName = out.dbConfig.Model + configOptions = out.dbConfig.Options + case configFixedModel: + modelName = spec.config.modelName + configOptions = spec.config.configOptions + } + + switch spec.config.mode { + case configFixedModel: + out.callConfig = spec.config.callConfig + default: + var err error + out.callConfig, err = parseModelConfigOptions(configOptions) + if err != nil { + return resolvedModelCall{}, xerrors.Errorf("parse model call config: %w", err) + } + } + if spec.defaultMaxOutputTokens && out.callConfig.MaxOutputTokens == nil { + out.callConfig.MaxOutputTokens = ptr.Ref(defaultChatMaxOutputTokens) + } + + if spec.routeOverride != nil { + out.route = *spec.routeOverride + } else { + routeCtx := ctx + if spec.chatdScopedRoute { + //nolint:gocritic // Deployment-wide override models need chatd-scoped provider reads for user-owned chats. + routeCtx = dbauthz.AsChatd(ctx) + } + var err error + if spec.config.mode == configFixedModel { + out.route, err = p.resolveModelRouteForProviderType(routeCtx, spec.chat.OwnerID, spec.config.providerType) + } else { + out.route, err = p.resolveModelRouteForConfig(routeCtx, spec.chat.OwnerID, out.dbConfig) + } + if err != nil { + return resolvedModelCall{}, err + } + } + + var err error + out.resolvedProvider, out.resolvedModel, err = chatprovider.ResolveModelWithProviderHint( + modelName, + out.route.ModelProviderHint, + ) + if err != nil { + return resolvedModelCall{}, xerrors.Errorf("resolve model metadata: %w", err) + } + + debugSvc := spec.debugSvc + switch spec.debug { + case debugPolicyAware: + if debugSvc == nil { + debugSvc = p.debugService() + } + out.debugEnabled = debugSvc != nil && debugSvc.IsEnabled(ctx, spec.chat.ID, spec.chat.OwnerID) + case debugPolicyForced: + out.debugEnabled = true + case debugPolicyOff: + } + + clientModelName := modelName + clientRoute := out.route + if spec.debug == debugPolicyAware { + // Preserved from newDebugAwareModel: debug-aware flows build the + // client from the resolved identity while other flows pass the raw + // configured model name. The distinction only matters for malformed + // slash-namespaced model names, so it is kept rather than unified. + clientRoute.ModelProviderHint = out.resolvedProvider + clientModelName = out.resolvedModel + } + + buildOpts := spec.buildOptions + buildOpts.RecordHTTP = out.debugEnabled + model, err := p.newModel(ctx, modelClientRequest{ + Chat: spec.chat, + ModelName: clientModelName, + UserAgent: chatprovider.UserAgent(), + ExtraHeaders: chatprovider.CoderHeaders(spec.chat), + ConfigOptions: configOptions, + }, clientRoute, buildOpts) + if err != nil { + return resolvedModelCall{}, xerrors.Errorf("create model: %w", err) + } + + if out.debugEnabled && debugSvc != nil { + wrapProvider := out.resolvedProvider + wrapModel := out.resolvedModel + if spec.debug == debugPolicyForced { + wrapProvider = spec.debugWrapProvider + wrapModel = spec.debugWrapModel + } + model = model.WithLanguageModel(chatdebug.WrapModel(model.LanguageModel(), debugSvc, chatdebug.RecorderOptions{ + ChatID: spec.chat.ID, + OwnerID: spec.chat.OwnerID, + Provider: wrapProvider, + Model: wrapModel, + })) + } + out.model = model + + if spec.providerOptions == providerOptionsDerive { + out.providerOptions = chatprovider.ProviderOptionsForCall(model, out.callConfig, spec.requestedEffort) + } + + p.logger.Debug(ctx, "resolved model call", + slog.F("purpose", spec.purpose), + slog.F("chat_id", spec.chat.ID), + slog.F("provider", out.resolvedProvider), + slog.F("model", out.resolvedModel), + slog.F("debug_enabled", out.debugEnabled), + ) + return out, nil +} From 82a27898d4543fc9ce315f4fb5ef32b6ad94fcd5 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Tue, 11 Aug 2026 19:33:15 +0000 Subject: [PATCH 03/13] refactor(coderd/x/chatd): migrate quickgen title, summary, and status-label flows to resolver Title (auto, manual, override), whole-chat summary, and turn-status label flows now resolve their model through resolveModelCall with purpose-specific specs. fantasy.ObjectCall envelopes come from resolvedModelCall.newObjectCall. Deletes titleGenerationProviderOptions, newQuickgenDebugModel, and the transitional resolveChatModel wrapper. Summary and status-label calls keep their historical provider-option omission as declared policy. --- coderd/x/chatd/chatd.go | 112 ++----- coderd/x/chatd/generation_preparer.go | 12 +- .../generation_preparer_internal_test.go | 15 +- coderd/x/chatd/modelcall.go | 67 +++- .../x/chatd/modelcall_shape_internal_test.go | 11 +- coderd/x/chatd/quickgen.go | 299 +++++++----------- coderd/x/chatd/quickgen_internal_test.go | 45 +-- coderd/x/chatd/subagent_internal_test.go | 8 +- coderd/x/chatd/title_override.go | 25 +- .../x/chatd/title_override_internal_test.go | 82 +++-- 10 files changed, 302 insertions(+), 374 deletions(-) diff --git a/coderd/x/chatd/chatd.go b/coderd/x/chatd/chatd.go index 33cc2264e77..276a5abf4ce 100644 --- a/coderd/x/chatd/chatd.go +++ b/coderd/x/chatd/chatd.go @@ -2529,23 +2529,23 @@ func (p *Server) generateManualTitleCandidate( } modelOpts := modelBuildOptions{ActiveAPIKeyID: apiKeyID} - model, modelConfig, err := p.resolveManualTitleModel(ctx, store, chat, modelOpts) + resolved, err := p.resolveManualTitleModel(ctx, store, chat, modelOpts) if err != nil { return "", err } titleCtx := ctx - titleModel := model + titleModel := resolved.model finishDebugRun := func(error) {} if debugSvc := p.debugService(); debugSvc != nil && debugSvc.IsEnabled(ctx, chat.ID, chat.OwnerID) { titleCtx, titleModel, finishDebugRun = p.prepareManualTitleDebugRun( ctx, debugSvc, chat, - modelConfig, + resolved.dbConfig, modelOpts, messages, - model, + resolved.model, ) } @@ -2554,7 +2554,7 @@ func (p *Server) generateManualTitleCandidate( messages, pasteText, titleModel.LanguageModel(), - p.titleGenerationProviderOptions(ctx, titleModel, modelConfig), + titleObjectCall(resolved), ) finishDebugRun(err) if err != nil { @@ -2780,15 +2780,15 @@ func (p *Server) resolveManualTitleModel( store database.Store, chat database.Chat, modelOpts modelBuildOptions, -) (chatprovider.Model, database.ChatModelConfig, error) { - overrideConfig, overrideModel, _, overrideSet, overrideErr := p.resolveTitleGenerationModelOverride( +) (resolvedModelCall, error) { + overrideResolved, overrideSet, overrideErr := p.resolveTitleGenerationModelOverride( ctx, chat, modelOpts, ) if overrideErr != nil { if overrideSet { - return chatprovider.Model{}, database.ChatModelConfig{}, xerrors.Errorf( + return resolvedModelCall{}, xerrors.Errorf( "resolve manual title generation model override: %w", overrideErr, ) @@ -2798,7 +2798,7 @@ func (p *Server) resolveManualTitleModel( slog.Error(overrideErr), ) } else if overrideSet { - return overrideModel, overrideConfig, nil + return overrideResolved, nil } configs, err := store.GetEnabledChatModelConfigs(ctx) @@ -2815,7 +2815,7 @@ func (p *Server) resolveManualTitleModel( return p.resolveFallbackManualTitleModel(ctx, chat, modelOpts) } - route, err := p.resolveModelRouteForConfig(ctx, chat.OwnerID, config) + resolved, err := p.resolveModelCall(ctx, manualTitleSpec(chat, config, modelOpts)) if err != nil { p.logger.Debug(ctx, "manual title preferred model unavailable", slog.F("chat_id", chat.ID), @@ -2824,55 +2824,29 @@ func (p *Server) resolveManualTitleModel( ) return p.resolveFallbackManualTitleModel(ctx, chat, modelOpts) } - model, err := p.newModel(ctx, modelClientRequest{ - Chat: chat, - ModelName: config.Model, - UserAgent: chatprovider.UserAgent(), - ExtraHeaders: chatprovider.CoderHeaders(chat), - ConfigOptions: config.Options, - }, route, modelOpts) - if err != nil { - p.logger.Debug(ctx, "manual title preferred model unavailable", - slog.F("chat_id", chat.ID), - slog.F("model", config.Model), - slog.Error(err), - ) - return p.resolveFallbackManualTitleModel(ctx, chat, modelOpts) - } - - return model, config, nil + return resolved, nil } func (p *Server) resolveFallbackManualTitleModel( ctx context.Context, chat database.Chat, modelOpts modelBuildOptions, -) (chatprovider.Model, database.ChatModelConfig, error) { +) (resolvedModelCall, error) { config, err := p.resolveModelConfig(ctx, chat) if err != nil { - return chatprovider.Model{}, database.ChatModelConfig{}, xerrors.Errorf( + return resolvedModelCall{}, xerrors.Errorf( "resolve fallback manual title model config: %w", err, ) } - route, err := p.resolveModelRouteForConfig(ctx, chat.OwnerID, config) + resolved, err := p.resolveModelCall(ctx, manualTitleSpec(chat, config, modelOpts)) if err != nil { - return chatprovider.Model{}, database.ChatModelConfig{}, err - } - model, err := p.newModel(ctx, modelClientRequest{ - Chat: chat, - ModelName: config.Model, - UserAgent: chatprovider.UserAgent(), - ExtraHeaders: chatprovider.CoderHeaders(chat), - ConfigOptions: config.Options, - }, route, modelOpts) - if err != nil { - return chatprovider.Model{}, database.ChatModelConfig{}, xerrors.Errorf( + return resolvedModelCall{}, xerrors.Errorf( "create fallback manual title model: %w", err, ) } - return model, config, nil + return resolved, nil } func mergeManualTitleMessages( @@ -3447,13 +3421,11 @@ func (p *Server) trackWorkspaceUsage( } type runChatResult struct { - FinalAssistantText string - StatusLabelModel chatprovider.Model - FallbackProvider string - FallbackRoute aiGatewayModelRoute - FallbackModel string + FinalAssistantText string + // StatusLabel is the resolved chat-model call used to generate the + // end-of-turn status label; nil when model resolution failed. + StatusLabel *resolvedModelCall ModelBuildOptions modelBuildOptions - StatusLabelOptions json.RawMessage TriggerMessageID int64 HistoryTipMessageID int64 } @@ -4026,29 +3998,6 @@ func buildProviderTools(options *codersdk.ChatModelProviderOptions) []chatloop.P return tools } -// resolveChatModel resolves the chat's model without deriving per-call -// provider options. Transitional wrapper over resolveModelCall; remaining -// callers migrate to purpose-specific specs. -func (p *Server) resolveChatModel( - ctx context.Context, - chat database.Chat, - modelOpts modelBuildOptions, -) ( - model chatprovider.Model, - dbConfig database.ChatModelConfig, - route aiGatewayModelRoute, - debugEnabled bool, - resolvedProvider string, - resolvedModel string, - err error, -) { - resolved, err := p.resolveModelCall(ctx, chatModelSpec(callPurposeStandardTurn, chat, modelOpts)) - if err != nil { - return chatprovider.Model{}, database.ChatModelConfig{}, aiGatewayModelRoute{}, false, "", "", err - } - return resolved.model, resolved.dbConfig, resolved.route, resolved.debugEnabled, resolved.resolvedProvider, resolved.resolvedModel, nil -} - func (p *Server) aiProviderConfig(ctx context.Context, provider database.AIProvider) (chatprovider.ConfiguredProvider, error) { keys, err := p.db.GetAIProviderKeysByProviderID(ctx, provider.ID) if err != nil { @@ -4596,7 +4545,7 @@ func (p *Server) generateFinalTurnStatusLabel( } assistantText := strings.TrimSpace(runResult.FinalAssistantText) - if assistantText == "" || !runResult.StatusLabelModel.Valid() { + if assistantText == "" || runResult.StatusLabel == nil { return fallbackTurnStatusLabel(status) } @@ -4605,12 +4554,8 @@ func (p *Server) generateFinalTurnStatusLabel( chat, status, assistantText, - runResult.FallbackProvider, - runResult.FallbackModel, - runResult.StatusLabelModel, - runResult.FallbackRoute, + *runResult.StatusLabel, runResult.ModelBuildOptions, - runResult.StatusLabelOptions, logger, p.existingDebugService(), runResult.TriggerMessageID, @@ -4831,14 +4776,14 @@ func (p *Server) generateAndStoreChatSummary( } modelOpts := modelBuildOptions{ActiveAPIKeyID: apiKeyID} - model, _, ok := p.resolveChatSummaryModel(ctx, logger, chat, modelOpts) + resolved, ok := p.resolveChatSummaryModel(ctx, logger, chat, modelOpts) if !ok { return } summaryCtx, cancelGen := context.WithTimeout(ctx, chatSummaryGenerateTimeout) defer cancelGen() - summary, _, genErr := generateChatSummary(summaryCtx, model, transcript) + summary, _, genErr := generateChatSummary(summaryCtx, resolved.model.LanguageModel(), summaryObjectCall(resolved), transcript) if genErr != nil { logger.Debug(ctx, "failed to generate chat summary", @@ -4854,15 +4799,14 @@ func (p *Server) resolveChatSummaryModel( logger slog.Logger, chat database.Chat, modelOpts modelBuildOptions, -) (fantasy.LanguageModel, database.ChatModelConfig, bool) { - //nolint:dogsled // resolveChatModel returns rich routing metadata; summary generation only needs the model and its config. - model, dbConfig, _, _, _, _, err := p.resolveChatModel(ctx, chat, modelOpts) +) (resolvedModelCall, bool) { + resolved, err := p.resolveModelCall(ctx, chatModelSpec(callPurposeSummary, chat, modelOpts)) if err != nil { logger.Debug(ctx, "failed to resolve chat model for summary", slog.F("chat_id", chat.ID), slog.Error(err)) - return nil, database.ChatModelConfig{}, false + return resolvedModelCall{}, false } - return model.LanguageModel(), dbConfig, true + return resolved, true } func shouldGenerateChatSummary(chat database.Chat, messages []database.ChatMessage) bool { diff --git a/coderd/x/chatd/generation_preparer.go b/coderd/x/chatd/generation_preparer.go index 9e48978d09c..6ac6bb5500c 100644 --- a/coderd/x/chatd/generation_preparer.go +++ b/coderd/x/chatd/generation_preparer.go @@ -779,18 +779,16 @@ func (server *Server) deriveFinalTurnRunResult( return runChatResult{} } - // resolvedProvider/resolvedModel describe the model the fallback handle was - // built from; they only feed the status-label fallback candidate's labels. apiKeyID, err := server.ensureSyntheticAPIKeyID(ctx, chat.OwnerID) if err != nil { logger.Warn(ctx, "derive final turn status label: ensure synthetic API key", slog.Error(err)) return runChatResult{FinalAssistantText: finalAssistantText, TriggerMessageID: triggerMessageID, HistoryTipMessageID: historyTipMessageID} } modelOpts := modelBuildOptions{ActiveAPIKeyID: apiKeyID} - model, dbConfig, modelRoute, _, resolvedProvider, resolvedModel, err := server.resolveChatModel(ctx, chat, modelOpts) + resolved, err := server.resolveModelCall(ctx, chatModelSpec(callPurposeStatusLabel, chat, modelOpts)) if err != nil { // Return what we have; generateFinalTurnStatusLabel falls back to a - // generic label when StatusLabelModel is nil. + // generic label when StatusLabel is nil. logger.Warn(ctx, "derive final turn status label: resolve model", slog.Error(err)) return runChatResult{ FinalAssistantText: finalAssistantText, @@ -801,12 +799,8 @@ func (server *Server) deriveFinalTurnRunResult( return runChatResult{ FinalAssistantText: finalAssistantText, - StatusLabelModel: model, - FallbackProvider: resolvedProvider, - FallbackRoute: modelRoute, - FallbackModel: resolvedModel, + StatusLabel: &resolved, ModelBuildOptions: modelOpts, - StatusLabelOptions: dbConfig.Options, TriggerMessageID: triggerMessageID, HistoryTipMessageID: historyTipMessageID, } diff --git a/coderd/x/chatd/generation_preparer_internal_test.go b/coderd/x/chatd/generation_preparer_internal_test.go index 83fd496739b..1574403def4 100644 --- a/coderd/x/chatd/generation_preparer_internal_test.go +++ b/coderd/x/chatd/generation_preparer_internal_test.go @@ -442,10 +442,11 @@ func TestDeriveFinalTurnRunResult(t *testing.T) { require.Equal(t, "the answer is 42", result.FinalAssistantText) require.Equal(t, lastUserID, result.TriggerMessageID) require.Equal(t, tipID, result.HistoryTipMessageID) - require.True(t, result.StatusLabelModel.Valid()) - require.Equal(t, "openai", result.FallbackProvider) - require.Equal(t, "gpt-4o-mini", result.FallbackModel) - require.JSONEq(t, `{"openai_config":{"use_responses_api":false}}`, string(result.StatusLabelOptions)) + require.NotNil(t, result.StatusLabel) + require.True(t, result.StatusLabel.model.Valid()) + require.Equal(t, "openai", result.StatusLabel.resolvedProvider) + require.Equal(t, "gpt-4o-mini", result.StatusLabel.resolvedModel) + require.JSONEq(t, `{"openai_config":{"use_responses_api":false}}`, string(result.StatusLabel.dbConfig.Options)) }) t.Run("NonWaitingReturnsEmpty", func(t *testing.T) { @@ -481,7 +482,7 @@ func TestDeriveFinalTurnRunResult(t *testing.T) { UserID: user.ID, OrganizationID: org.ID, }) - // A disabled AI provider makes resolveChatModel fail, exercising the + // A disabled AI provider makes model resolution fail, exercising the // degraded path that still returns the re-derived text and IDs. provider := insertInternalAIProvider(t, db, database.AIProviderTypeOpenai, "provider-api-key", false) modelCfg := dbgen.ChatModelConfig(t, db, database.ChatModelConfig{ @@ -519,9 +520,7 @@ func TestDeriveFinalTurnRunResult(t *testing.T) { require.Equal(t, "the answer is 42", result.FinalAssistantText) require.NotZero(t, result.TriggerMessageID) require.NotZero(t, result.HistoryTipMessageID) - require.False(t, result.StatusLabelModel.Valid()) - require.Empty(t, result.FallbackProvider) - require.Empty(t, result.FallbackModel) + require.Nil(t, result.StatusLabel) }) } diff --git a/coderd/x/chatd/modelcall.go b/coderd/x/chatd/modelcall.go index 0b977379543..70c3aa1ee93 100644 --- a/coderd/x/chatd/modelcall.go +++ b/coderd/x/chatd/modelcall.go @@ -139,8 +139,9 @@ func standardTurnSpec(chat database.Chat, buildOpts modelBuildOptions) modelCall } // chatModelSpec resolves the chat's model without deriving provider options -// or applying the standard-turn token default. Callers that need per-call -// options derive them from their own spec. +// or applying the standard-turn token default. Summary and status-label +// calls historically send no provider options; that omission is preserved +// here as declared policy. func chatModelSpec(purpose callPurpose, chat database.Chat, buildOpts modelBuildOptions) modelCallSpec { return modelCallSpec{ purpose: purpose, @@ -152,6 +153,48 @@ func chatModelSpec(purpose callPurpose, chat database.Chat, buildOpts modelBuild } } +// titleChatSpec resolves the chat's own model as the title-generation +// fallback candidate. Title calls derive provider options without a +// requested effort: the user's per-turn effort choice applies to turns, not +// background title generation. +func titleChatSpec(chat database.Chat, buildOpts modelBuildOptions) modelCallSpec { + return modelCallSpec{ + purpose: callPurposeTitle, + chat: chat, + config: configSelection{mode: configFromChat}, + providerOptions: providerOptionsDerive, + debug: debugPolicyAware, + buildOptions: buildOpts, + } +} + +// titleOverrideSpec builds the deployment-wide title override model from the +// caller-selected config row. The route resolves with chatd scope so the +// override works for chats whose owner cannot read the provider. +func titleOverrideSpec(chat database.Chat, config database.ChatModelConfig, buildOpts modelBuildOptions) modelCallSpec { + return modelCallSpec{ + purpose: callPurposeTitle, + chat: chat, + config: configSelection{mode: configExplicit, config: config}, + providerOptions: providerOptionsDerive, + chatdScopedRoute: true, + buildOptions: buildOpts, + } +} + +// manualTitleSpec builds a caller-selected manual-title model: the preferred +// small model or the chat's own config as fallback. Debug recording is +// handled by a separate rebuild, matching the historical construction. +func manualTitleSpec(chat database.Chat, config database.ChatModelConfig, buildOpts modelBuildOptions) modelCallSpec { + return modelCallSpec{ + purpose: callPurposeTitle, + chat: chat, + config: configSelection{mode: configExplicit, config: config}, + providerOptions: providerOptionsDerive, + buildOptions: buildOpts, + } +} + // computerUseSpec swaps in the deployment's computer-use model. The client is // built without config options because the fixed model has no config row; the // chat model's call config still drives per-call provider options so admin @@ -330,3 +373,23 @@ func (p *Server) resolveModelCall(ctx context.Context, spec modelCallSpec) (reso ) return out, nil } + +// objectCallOverrides carries the caller-owned schema and token cap for a +// structured-output call. Quickgen flows use fixed caps instead of the model +// config's tuning. +type objectCallOverrides struct { + schemaName string + schemaDescription string + maxOutputTokens int64 +} + +// newObjectCall builds the fantasy.ObjectCall envelope for one +// structured-output call. The caller attaches the prompt before sending. +func (r resolvedModelCall) newObjectCall(o objectCallOverrides) fantasy.ObjectCall { + return fantasy.ObjectCall{ + SchemaName: o.schemaName, + SchemaDescription: o.schemaDescription, + MaxOutputTokens: ptr.Ref(o.maxOutputTokens), + ProviderOptions: r.providerOptions, + } +} diff --git a/coderd/x/chatd/modelcall_shape_internal_test.go b/coderd/x/chatd/modelcall_shape_internal_test.go index 31a4efe8e28..6bd2d768e14 100644 --- a/coderd/x/chatd/modelcall_shape_internal_test.go +++ b/coderd/x/chatd/modelcall_shape_internal_test.go @@ -319,12 +319,13 @@ func TestModelCallShapeTurnStatusLabelEnvelope(t *testing.T) { database.Chat{ID: uuid.New(), OwnerID: uuid.New(), Title: "status shape"}, database.ChatStatusWaiting, "All tests pass now.", - fantasyopenai.Name, - "gpt-4o-mini", - chatprovider.NewModel(model, nil), - aiGatewayModelRoute{}, + resolvedModelCall{ + model: chatprovider.NewModel(model, nil), + dbConfig: database.ChatModelConfig{Options: modelCallSentinelOptions(t, "status-options-sentinel")}, + resolvedProvider: fantasyopenai.Name, + resolvedModel: "gpt-4o-mini", + }, modelBuildOptions{}, - modelCallSentinelOptions(t, "status-options-sentinel"), logger, nil, 0, diff --git a/coderd/x/chatd/quickgen.go b/coderd/x/chatd/quickgen.go index ccc73b3ed30..2fb4092e300 100644 --- a/coderd/x/chatd/quickgen.go +++ b/coderd/x/chatd/quickgen.go @@ -2,7 +2,6 @@ package chatd import ( "context" - "encoding/json" "errors" "fmt" "net/http" @@ -137,13 +136,42 @@ var preferredTitleModels = []struct { {fantasyvercel.Name, "anthropic/claude-haiku-4.5"}, } +// shortTextCandidate is one quickgen model candidate. provider and model +// label debug runs: title flows use the route's provider type and configured +// model name while the status-label flow uses the resolved identity. type shortTextCandidate struct { - provider string - model string - route aiGatewayModelRoute - lm chatprovider.Model - providerOptions fantasy.ProviderOptions - configOptions json.RawMessage + provider string + model string + resolved resolvedModelCall +} + +// quickgenDebugSpec rebuilds a quickgen candidate's client with HTTP +// recording after the caller verified debug is enabled. The client keeps the +// candidate's model name, config options, and route; the wrap labels keep +// the candidate's attribution. +func quickgenDebugSpec( + chat database.Chat, + candidate shortTextCandidate, + debugSvc *chatdebug.Service, + buildOpts modelBuildOptions, +) modelCallSpec { + route := candidate.resolved.route + return modelCallSpec{ + purpose: callPurposeDebugRebuild, + chat: chat, + config: configSelection{ + mode: configFixedModel, + modelName: candidate.model, + configOptions: candidate.resolved.dbConfig.Options, + }, + providerOptions: providerOptionsOmit, + debug: debugPolicyForced, + debugSvc: debugSvc, + debugWrapProvider: candidate.provider, + debugWrapModel: candidate.model, + routeOverride: &route, + buildOptions: buildOpts, + } } func selectPreferredConfiguredShortTextModelConfig( @@ -231,7 +259,7 @@ func (p *Server) GenerateChatTitleAsync(ctx context.Context, chat database.Chat) } modelOpts := modelBuildOptions{ActiveAPIKeyID: apiKeyID} turnCtx := titleCtx - model, modelConfig, route, _, _, _, err := p.resolveChatModel(turnCtx, chat, modelOpts) + fallback, err := p.resolveModelCall(turnCtx, titleChatSpec(chat, modelOpts)) if err != nil { logger.Debug(titleCtx, "failed to resolve model for automatic title generation", slog.Error(err), @@ -243,10 +271,7 @@ func (p *Server) GenerateChatTitleAsync(ctx context.Context, chat database.Chat) chat, messages, pasteText, - string(route.Provider.Type), - modelConfig, - model, - route, + fallback, modelOpts, &generatedChatTitle{}, logger, @@ -274,10 +299,7 @@ func (p *Server) maybeGenerateChatTitle( chat database.Chat, messages []database.ChatMessage, pasteText map[uuid.UUID]string, - fallbackProvider string, - fallbackConfig database.ChatModelConfig, - fallbackModel chatprovider.Model, - fallbackRoute aiGatewayModelRoute, + fallback resolvedModelCall, modelOpts modelBuildOptions, generatedTitle *generatedChatTitle, logger slog.Logger, @@ -292,7 +314,7 @@ func (p *Server) maybeGenerateChatTitle( titleCtx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() - overrideConfig, overrideModel, overrideRoute, overrideSet, overrideErr := p.resolveTitleGenerationModelOverride( + overrideResolved, overrideSet, overrideErr := p.resolveTitleGenerationModelOverride( titleCtx, chat, modelOpts, @@ -313,25 +335,14 @@ func (p *Server) maybeGenerateChatTitle( ) } - var candidate shortTextCandidate + selected := fallback if overrideSet { - candidate = shortTextCandidate{ - provider: string(overrideRoute.Provider.Type), - model: overrideConfig.Model, - route: overrideRoute, - lm: overrideModel, - providerOptions: p.titleGenerationProviderOptions(ctx, overrideModel, overrideConfig), - configOptions: overrideConfig.Options, - } - } else { - candidate = shortTextCandidate{ - provider: fallbackProvider, - model: fallbackConfig.Model, - route: fallbackRoute, - lm: fallbackModel, - providerOptions: p.titleGenerationProviderOptions(ctx, fallbackModel, fallbackConfig), - configOptions: fallbackConfig.Options, - } + selected = overrideResolved + } + candidate := shortTextCandidate{ + provider: string(selected.route.Provider.Type), + model: selected.dbConfig.Model, + resolved: selected, } var historyTipMessageID int64 @@ -355,7 +366,7 @@ func (p *Server) maybeGenerateChatTitle( ) candidateCtx := titleCtx - candidateModel := candidate.lm + candidateModel := candidate.resolved.model finishDebugRun := func(error) {} if debugEnabled { candidateCtx, candidateModel, finishDebugRun = p.prepareQuickgenDebugCandidate( @@ -372,7 +383,7 @@ func (p *Server) maybeGenerateChatTitle( ) } - title, err := generateTitle(candidateCtx, candidateModel.LanguageModel(), candidate.providerOptions, input) + title, err := generateTitle(candidateCtx, candidateModel.LanguageModel(), titleObjectCall(candidate.resolved), input) finishDebugRun(err) if err != nil { if overrideSet { @@ -411,52 +422,14 @@ func (p *Server) maybeGenerateChatTitle( p.publishChatPubsubEvent(chat, codersdk.ChatWatchEventKindTitleChange, nil) } -func (p *Server) titleGenerationProviderOptions( - ctx context.Context, - model chatprovider.Model, - config database.ChatModelConfig, -) fantasy.ProviderOptions { - callConfig := codersdk.ChatModelCallConfig{} - if len(config.Options) > 0 { - if err := json.Unmarshal(config.Options, &callConfig); err != nil { - p.logger.Debug(ctx, "failed to parse title generation model call config", - slog.F("model_config_id", config.ID), - slog.Error(err), - ) - } - } - return chatprovider.ProviderOptionsForCall(model, callConfig, nil) -} - -func (p *Server) newQuickgenDebugModel( - ctx context.Context, - chat database.Chat, - debugSvc *chatdebug.Service, - provider string, - model string, - route aiGatewayModelRoute, - modelOpts modelBuildOptions, - configOptions json.RawMessage, -) (chatprovider.Model, error) { - debugOpts := modelOpts - debugOpts.RecordHTTP = true - debugModel, err := p.newModel(ctx, modelClientRequest{ - Chat: chat, - ModelName: model, - UserAgent: chatprovider.UserAgent(), - ExtraHeaders: chatprovider.CoderHeaders(chat), - ConfigOptions: configOptions, - }, route, debugOpts) - if err != nil { - return chatprovider.Model{}, err - } +const titleMaxOutputTokens = int64(256) - return debugModel.WithLanguageModel(chatdebug.WrapModel(debugModel.LanguageModel(), debugSvc, chatdebug.RecorderOptions{ - ChatID: chat.ID, - OwnerID: chat.OwnerID, - Provider: provider, - Model: model, - })), nil +func titleObjectCall(resolved resolvedModelCall) fantasy.ObjectCall { + return resolved.newObjectCall(objectCallOverrides{ + schemaName: "propose_title", + schemaDescription: "Propose a short chat title.", + maxOutputTokens: titleMaxOutputTokens, + }) } func (p *Server) prepareQuickgenDebugCandidate( @@ -473,19 +446,10 @@ func (p *Server) prepareQuickgenDebugCandidate( ) (context.Context, chatprovider.Model, func(error)) { finishDebugRun := func(error) {} if debugSvc == nil { - return ctx, candidate.lm, finishDebugRun + return ctx, candidate.resolved.model, finishDebugRun } - debugModel, err := p.newQuickgenDebugModel( - ctx, - chat, - debugSvc, - candidate.provider, - candidate.model, - candidate.route, - modelOpts, - candidate.configOptions, - ) + debugResolved, err := p.resolveModelCall(ctx, quickgenDebugSpec(chat, candidate, debugSvc, modelOpts)) if err != nil { logger.Warn(ctx, "failed to build short-text debug model", slog.F("chat_id", chat.ID), @@ -494,8 +458,9 @@ func (p *Server) prepareQuickgenDebugCandidate( slog.F("model", candidate.model), slog.Error(err), ) - return ctx, candidate.lm, finishDebugRun + return ctx, candidate.resolved.model, finishDebugRun } + debugModel := debugResolved.model // Debug instrumentation must not eat into the quickgen budget // (30s titleCtx / summaryCtx on the caller). Detach and bound @@ -524,7 +489,7 @@ func (p *Server) prepareQuickgenDebugCandidate( slog.F("model", candidate.model), slog.Error(err), ) - return ctx, candidate.lm, finishDebugRun + return ctx, candidate.resolved.model, finishDebugRun } runContext := chatdebugRunContext(run) @@ -548,16 +513,34 @@ func (p *Server) prepareQuickgenDebugCandidate( return runCtx, debugModel, finishDebugRun } +// quickgenPrompt pairs a system prompt with one user message. +func quickgenPrompt(systemPrompt, userInput string) fantasy.Prompt { + return fantasy.Prompt{ + { + Role: fantasy.MessageRoleSystem, + Content: []fantasy.MessagePart{ + fantasy.TextPart{Text: systemPrompt}, + }, + }, + { + Role: fantasy.MessageRoleUser, + Content: []fantasy.MessagePart{ + fantasy.TextPart{Text: userInput}, + }, + }, + } +} + // generateTitle calls the model with a title-generation system prompt // and returns the normalized result. It retries transient LLM errors // (rate limits, overloaded, etc.) with exponential backoff. func generateTitle( ctx context.Context, model fantasy.LanguageModel, - providerOptions fantasy.ProviderOptions, + call fantasy.ObjectCall, input string, ) (string, error) { - title, err := generateStructuredTitle(ctx, model, providerOptions, titleGenerationPrompt, input) + title, err := generateStructuredTitle(ctx, model, call, titleGenerationPrompt, input) if err != nil { return "", err } @@ -567,14 +550,14 @@ func generateTitle( func generateStructuredTitle( ctx context.Context, model fantasy.LanguageModel, - providerOptions fantasy.ProviderOptions, + call fantasy.ObjectCall, systemPrompt string, userInput string, ) (string, error) { title, _, err := generateStructuredTitleWithUsage( ctx, model, - providerOptions, + call, systemPrompt, userInput, ) @@ -587,7 +570,7 @@ func generateStructuredTitle( func generateStructuredTitleWithUsage( ctx context.Context, model fantasy.LanguageModel, - providerOptions fantasy.ProviderOptions, + call fantasy.ObjectCall, systemPrompt string, userInput string, ) (string, fantasy.Usage, error) { @@ -596,29 +579,8 @@ func generateStructuredTitleWithUsage( return "", fantasy.Usage{}, xerrors.New("title input was empty") } - prompt := fantasy.Prompt{ - { - Role: fantasy.MessageRoleSystem, - Content: []fantasy.MessagePart{ - fantasy.TextPart{Text: systemPrompt}, - }, - }, - { - Role: fantasy.MessageRoleUser, - Content: []fantasy.MessagePart{ - fantasy.TextPart{Text: userInput}, - }, - }, - } - - var maxOutputTokens int64 = 256 - result, err := generateQuickgenObject[generatedTitle](ctx, model, fantasy.ObjectCall{ - Prompt: prompt, - SchemaName: "propose_title", - SchemaDescription: "Propose a short chat title.", - MaxOutputTokens: &maxOutputTokens, - ProviderOptions: providerOptions, - }) + call.Prompt = quickgenPrompt(systemPrompt, userInput) + result, err := generateQuickgenObject[generatedTitle](ctx, model, call) if err != nil { var usage fantasy.Usage var noObjErr *fantasy.NoObjectGeneratedError @@ -926,7 +888,7 @@ func generateManualTitle( messages []database.ChatMessage, pasteText map[uuid.UUID]string, fallbackModel fantasy.LanguageModel, - providerOptions fantasy.ProviderOptions, + call fantasy.ObjectCall, ) (string, error) { turns := extractManualTitleTurns(messages, pasteText) selected := selectManualTitleTurnIndexes(turns) @@ -957,7 +919,7 @@ func generateManualTitle( title, _, err := generateStructuredTitleWithUsage( titleCtx, fallbackModel, - providerOptions, + call, systemPrompt, userInput, ) @@ -1088,12 +1050,21 @@ func boundTranscriptHeadTail(lines []string, maxRunes int) string { return out.String() } +func summaryObjectCall(resolved resolvedModelCall) fantasy.ObjectCall { + return resolved.newObjectCall(objectCallOverrides{ + schemaName: "chat_summary", + schemaDescription: "Summarize the whole chat in 1-3 sentences.", + maxOutputTokens: summaryMaxOutputTokens, + }) +} + // generateChatSummary generates a 1-3 sentence whole-chat summary from a // transcript. A blank or invalid result returns an error so callers preserve // any existing summary rather than clearing it. func generateChatSummary( ctx context.Context, model fantasy.LanguageModel, + call fantasy.ObjectCall, transcript string, ) (string, fantasy.Usage, error) { transcript = strings.TrimSpace(transcript) @@ -1101,31 +1072,11 @@ func generateChatSummary( return "", fantasy.Usage{}, xerrors.New("chat summary transcript was empty") } - prompt := fantasy.Prompt{ - { - Role: fantasy.MessageRoleSystem, - Content: []fantasy.MessagePart{ - fantasy.TextPart{Text: chatSummaryGenerationPrompt}, - }, - }, - { - Role: fantasy.MessageRoleUser, - Content: []fantasy.MessagePart{ - fantasy.TextPart{Text: transcript}, - }, - }, - } - - maxOutputTokens := int64(summaryMaxOutputTokens) + call.Prompt = quickgenPrompt(chatSummaryGenerationPrompt, transcript) var result *fantasy.ObjectResult[generatedChatSummary] err := chatretry.Retry(ctx, func(retryCtx context.Context) error { var genErr error - result, genErr = object.Generate[generatedChatSummary](retryCtx, model, fantasy.ObjectCall{ - Prompt: prompt, - SchemaName: "chat_summary", - SchemaDescription: "Summarize the whole chat in 1-3 sentences.", - MaxOutputTokens: &maxOutputTokens, - }) + result, genErr = object.Generate[generatedChatSummary](retryCtx, model, call) return genErr }, nil) if err != nil { @@ -1321,19 +1272,25 @@ const turnStatusLabelPrompt = "You write compact chat status labels for a sideba "Prefer short action or state phrases such as Finished, Submitted, Fixed, Testing, Still working, or Waiting for. " + "No quotes, emoji, markdown, or trailing punctuation." +const turnStatusLabelMaxOutputTokens = int64(64) + +func turnStatusLabelObjectCall(resolved resolvedModelCall) fantasy.ObjectCall { + return resolved.newObjectCall(objectCallOverrides{ + schemaName: "propose_turn_status_label", + schemaDescription: "Propose a compact chat status label.", + maxOutputTokens: turnStatusLabelMaxOutputTokens, + }) +} + // generateTurnStatusLabel produces a short turn status label using the -// caller-supplied fallback model. Returns "" on any failure. +// resolved chat-model call. Returns "" on any failure. func (p *Server) generateTurnStatusLabel( ctx context.Context, chat database.Chat, status database.ChatStatus, assistantText string, - fallbackProvider string, - fallbackModelName string, - fallbackModel chatprovider.Model, - fallbackRoute aiGatewayModelRoute, + resolved resolvedModelCall, modelOpts modelBuildOptions, - configOptions json.RawMessage, logger slog.Logger, debugSvc *chatdebug.Service, triggerMessageID int64, @@ -1350,17 +1307,15 @@ func (p *Server) generateTurnStatusLabel( "\n\nAgent's latest message:\n" + assistantText candidate := shortTextCandidate{ - provider: fallbackProvider, - model: fallbackModelName, - route: fallbackRoute, - lm: fallbackModel, - configOptions: configOptions, + provider: resolved.resolvedProvider, + model: resolved.resolvedModel, + resolved: resolved, } statusSeedSummary := chatdebug.SeedSummary("Turn status label") candidateCtx := labelCtx - candidateModel := candidate.lm + candidateModel := candidate.resolved.model finishDebugRun := func(error) {} if debugEnabled { candidateCtx, candidateModel, finishDebugRun = p.prepareQuickgenDebugCandidate( @@ -1380,6 +1335,7 @@ func (p *Server) generateTurnStatusLabel( generatedLabel, err := generateStructuredTurnStatusLabel( candidateCtx, candidateModel.LanguageModel(), + turnStatusLabelObjectCall(resolved), turnStatusLabelPrompt, input, ) @@ -1396,6 +1352,7 @@ func (p *Server) generateTurnStatusLabel( func generateStructuredTurnStatusLabel( ctx context.Context, model fantasy.LanguageModel, + call fantasy.ObjectCall, systemPrompt string, userInput string, ) (string, error) { @@ -1404,28 +1361,8 @@ func generateStructuredTurnStatusLabel( return "", xerrors.New("turn status label input was empty") } - prompt := fantasy.Prompt{ - { - Role: fantasy.MessageRoleSystem, - Content: []fantasy.MessagePart{ - fantasy.TextPart{Text: systemPrompt}, - }, - }, - { - Role: fantasy.MessageRoleUser, - Content: []fantasy.MessagePart{ - fantasy.TextPart{Text: userInput}, - }, - }, - } - - var maxOutputTokens int64 = 64 - result, err := generateQuickgenObject[generatedTurnStatusLabel](ctx, model, fantasy.ObjectCall{ - Prompt: prompt, - SchemaName: "propose_turn_status_label", - SchemaDescription: "Propose a compact chat status label.", - MaxOutputTokens: &maxOutputTokens, - }) + call.Prompt = quickgenPrompt(systemPrompt, userInput) + result, err := generateQuickgenObject[generatedTurnStatusLabel](ctx, model, call) if err != nil { return "", xerrors.Errorf("generate structured turn status label: %w", err) } diff --git a/coderd/x/chatd/quickgen_internal_test.go b/coderd/x/chatd/quickgen_internal_test.go index a761bebc96d..9305b91e64c 100644 --- a/coderd/x/chatd/quickgen_internal_test.go +++ b/coderd/x/chatd/quickgen_internal_test.go @@ -586,10 +586,10 @@ func TestMaybeGenerateChatTitlePreservesUpdatedAt(t *testing.T) { chat, []database.ChatMessage{message}, nil, - "openai", - database.ChatModelConfig{Model: "test-model"}, - chatprovider.NewModel(model, nil), - aiGatewayModelRoute{}, + resolvedModelCall{ + model: chatprovider.NewModel(model, nil), + dbConfig: database.ChatModelConfig{Model: "test-model"}, + }, modelBuildOptions{}, generated, logger, @@ -650,15 +650,22 @@ func TestMaybeGenerateChatTitleAppliesModelConfigReasoningEffort(t *testing.T) { logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) server := titleOverrideTestServer(db, logger) + fallbackModel := chatprovider.NewModel(model, nil) + fallbackConfig := database.ChatModelConfig{Model: "gpt-4o-mini", Options: modelConfigRaw} + callConfig, err := parseModelConfigOptions(fallbackConfig.Options) + require.NoError(t, err) server.maybeGenerateChatTitle( ctx, chat, messages, nil, - fantasyopenai.Name, - database.ChatModelConfig{Model: "gpt-4o-mini", Options: modelConfigRaw}, - chatprovider.NewModel(model, nil), - aiGatewayModelRoute{}, + resolvedModelCall{ + model: fallbackModel, + dbConfig: fallbackConfig, + // Mirrors titleChatSpec: derive with no requested effort so the + // config's default reasoning effort applies. + providerOptions: chatprovider.ProviderOptionsForCall(fallbackModel, callConfig, nil), + }, modelBuildOptions{}, &generatedChatTitle{}, logger, @@ -710,7 +717,7 @@ func Test_generateManualTitle_UsesTimeout(t *testing.T) { messages, nil, model, - nil, + titleObjectCall(resolvedModelCall{}), ) require.NoError(t, err) require.Equal(t, "Refresh title", title) @@ -748,7 +755,7 @@ func Test_generateManualTitle_TruncatesFirstUserInput(t *testing.T) { messages, nil, model, - nil, + titleObjectCall(resolvedModelCall{}), ) require.NoError(t, err) } @@ -783,7 +790,7 @@ func Test_generateManualTitle_ErrorsOnEmptyNormalizedTitle(t *testing.T) { messages, nil, model, - nil, + titleObjectCall(resolvedModelCall{}), ) require.ErrorContains(t, err, "generated title was empty") } @@ -885,7 +892,7 @@ func TestGenerateStructuredTitleWithUsage_OpenAICompatibleRequiredToolChoice(t * title, _, err := generateStructuredTitleWithUsage( t.Context(), model.LanguageModel(), - nil, + titleObjectCall(resolvedModelCall{}), titleGenerationPrompt, "summarize failed workspace build logs", ) @@ -930,7 +937,7 @@ func TestGenerateStructuredTitleWithUsage_DropsRejectedTemperature(t *testing.T) title, _, err := generateStructuredTitleWithUsage( t.Context(), model, - nil, + titleObjectCall(resolvedModelCall{}), titleGenerationPrompt, "summarize failed workspace build logs", ) @@ -1031,7 +1038,7 @@ func TestGenerateStructuredTurnStatusLabel(t *testing.T) { }, } - label, err := generateStructuredTurnStatusLabel(t.Context(), model, turnStatusLabelPrompt, "done") + label, err := generateStructuredTurnStatusLabel(t.Context(), model, turnStatusLabelObjectCall(resolvedModelCall{}), turnStatusLabelPrompt, "done") require.NoError(t, err) require.Equal(t, "Submitted PR", label) }) @@ -1042,7 +1049,7 @@ func TestGenerateStructuredTurnStatusLabel(t *testing.T) { server, requests := newOpenAICompatStructuredOutputServer(t, "propose_turn_status_label", `{"label":"Submitted PR"}`) model := openAICompatTestModel(t, server.URL) - label, err := generateStructuredTurnStatusLabel(t.Context(), model.LanguageModel(), turnStatusLabelPrompt, "done") + label, err := generateStructuredTurnStatusLabel(t.Context(), model.LanguageModel(), turnStatusLabelObjectCall(resolvedModelCall{}), turnStatusLabelPrompt, "done") require.NoError(t, err) require.Equal(t, "Submitted PR", label) require.Len(t, requests, 1) @@ -1069,7 +1076,7 @@ func TestGenerateStructuredTurnStatusLabel(t *testing.T) { }, } - label, err := generateStructuredTurnStatusLabel(t.Context(), model, turnStatusLabelPrompt, "done") + label, err := generateStructuredTurnStatusLabel(t.Context(), model, turnStatusLabelObjectCall(resolvedModelCall{}), turnStatusLabelPrompt, "done") require.NoError(t, err) require.Equal(t, "Submitted PR", label) require.Equal(t, []bool{true, false}, sawTemperature, @@ -1091,7 +1098,7 @@ func TestGenerateStructuredTurnStatusLabel(t *testing.T) { }, } - _, err := generateStructuredTurnStatusLabel(t.Context(), model, turnStatusLabelPrompt, "done") + _, err := generateStructuredTurnStatusLabel(t.Context(), model, turnStatusLabelObjectCall(resolvedModelCall{}), turnStatusLabelPrompt, "done") require.ErrorContains(t, err, "JSON schema is invalid") require.Equal(t, 1, calls, "bad requests unrelated to temperature should not trigger a second attempt") @@ -1108,7 +1115,7 @@ func TestGenerateStructuredTurnStatusLabel(t *testing.T) { }, } - _, err := generateStructuredTurnStatusLabel(t.Context(), model, turnStatusLabelPrompt, "done") + _, err := generateStructuredTurnStatusLabel(t.Context(), model, turnStatusLabelObjectCall(resolvedModelCall{}), turnStatusLabelPrompt, "done") require.ErrorContains(t, err, "generated turn status label was invalid") }) @@ -1116,7 +1123,7 @@ func TestGenerateStructuredTurnStatusLabel(t *testing.T) { t.Parallel() model := &chattest.FakeModel{} - _, err := generateStructuredTurnStatusLabel(t.Context(), model, turnStatusLabelPrompt, " ") + _, err := generateStructuredTurnStatusLabel(t.Context(), model, turnStatusLabelObjectCall(resolvedModelCall{}), turnStatusLabelPrompt, " ") require.ErrorContains(t, err, "turn status label input was empty") }) } diff --git a/coderd/x/chatd/subagent_internal_test.go b/coderd/x/chatd/subagent_internal_test.go index 9f1e55c54fd..290c43fb34e 100644 --- a/coderd/x/chatd/subagent_internal_test.go +++ b/coderd/x/chatd/subagent_internal_test.go @@ -539,13 +539,9 @@ func TestResolveChatModel_AIProviderDisabled(t *testing.T) { LastModelConfigID: modelConfig.ID, }) - model, config, _, debugEnabled, resolvedProvider, resolvedModel, err := server.resolveChatModel(ctx, chat, modelBuildOptions{}) + resolved, err := server.resolveModelCall(ctx, standardTurnSpec(chat, modelBuildOptions{})) require.ErrorContains(t, err, "is disabled") - require.False(t, model.Valid()) - require.Equal(t, database.ChatModelConfig{}, config) - require.False(t, debugEnabled) - require.Empty(t, resolvedProvider) - require.Empty(t, resolvedModel) + require.Equal(t, resolvedModelCall{}, resolved) } func TestResolveUserProviderAPIKeys_PreservesAnthropicKeyFromDBProvider(t *testing.T) { diff --git a/coderd/x/chatd/title_override.go b/coderd/x/chatd/title_override.go index 4056fdcfe13..4f2dfb92c42 100644 --- a/coderd/x/chatd/title_override.go +++ b/coderd/x/chatd/title_override.go @@ -60,10 +60,10 @@ func (p *Server) resolveTitleGenerationModelOverride( ctx context.Context, chat database.Chat, modelOpts modelBuildOptions, -) (database.ChatModelConfig, chatprovider.Model, aiGatewayModelRoute, bool, error) { +) (resolvedModelCall, bool, error) { raw, err := readTitleGenerationModelOverride(ctx, p.db) if err != nil { - return database.ChatModelConfig{}, chatprovider.Model{}, aiGatewayModelRoute{}, false, xerrors.Errorf( + return resolvedModelCall{}, false, xerrors.Errorf( "read title generation model override: %w", err, ) @@ -81,30 +81,19 @@ func (p *Server) resolveTitleGenerationModelOverride( modelOverrideFailureModeHard, ) if err != nil { - return database.ChatModelConfig{}, chatprovider.Model{}, aiGatewayModelRoute{}, overrideSet, err + return resolvedModelCall{}, overrideSet, err } if !overrideSet { - return database.ChatModelConfig{}, chatprovider.Model{}, aiGatewayModelRoute{}, false, nil + return resolvedModelCall{}, false, nil } modelConfig = withResolvedReasoningEffort(modelConfig, overrideEffort) - //nolint:gocritic // Title overrides need chatd-scoped provider reads for user-owned chats. - route, err := p.resolveModelRouteForConfig(dbauthz.AsChatd(ctx), chat.OwnerID, modelConfig) + resolved, err := p.resolveModelCall(ctx, titleOverrideSpec(chat, modelConfig, modelOpts)) if err != nil { - return database.ChatModelConfig{}, chatprovider.Model{}, aiGatewayModelRoute{}, true, err - } - model, err := p.newModel(ctx, modelClientRequest{ - Chat: chat, - ModelName: modelConfig.Model, - UserAgent: chatprovider.UserAgent(), - ExtraHeaders: chatprovider.CoderHeaders(chat), - ConfigOptions: modelConfig.Options, - }, route, modelOpts) - if err != nil { - return database.ChatModelConfig{}, chatprovider.Model{}, aiGatewayModelRoute{}, true, xerrors.Errorf( + return resolvedModelCall{}, true, xerrors.Errorf( "create title generation model override: %w", err, ) } - return modelConfig, model, route, true, nil + return resolved, true, nil } diff --git a/coderd/x/chatd/title_override_internal_test.go b/coderd/x/chatd/title_override_internal_test.go index 36ebef72260..318255f7778 100644 --- a/coderd/x/chatd/title_override_internal_test.go +++ b/coderd/x/chatd/title_override_internal_test.go @@ -69,10 +69,10 @@ func TestMaybeGenerateChatTitle_TitleGenerationOverrideUnset(t *testing.T) { chat, messages, nil, - "openai", - database.ChatModelConfig{Model: "fallback-chat-model"}, - chatprovider.NewModel(fallbackModel, nil), - aiGatewayModelRoute{}, + resolvedModelCall{ + model: chatprovider.NewModel(fallbackModel, nil), + dbConfig: database.ChatModelConfig{Model: "fallback-chat-model"}, + }, modelBuildOptions{}, generated, logger, @@ -119,10 +119,10 @@ func TestMaybeGenerateChatTitle_TitleGenerationOverrideReadDBError(t *testing.T) chat, messages, nil, - "openai", - database.ChatModelConfig{Model: "fallback-chat-model"}, - chatprovider.NewModel(fallbackModel, nil), - aiGatewayModelRoute{}, + resolvedModelCall{ + model: chatprovider.NewModel(fallbackModel, nil), + dbConfig: database.ChatModelConfig{Model: "fallback-chat-model"}, + }, modelBuildOptions{}, generated, logger, @@ -168,10 +168,10 @@ func TestMaybeGenerateChatTitle_TitleGenerationOverrideMalformedFallsThrough(t * chat, messages, nil, - "openai", - database.ChatModelConfig{Model: "fallback-chat-model"}, - chatprovider.NewModel(fallbackModel, nil), - aiGatewayModelRoute{}, + resolvedModelCall{ + model: chatprovider.NewModel(fallbackModel, nil), + dbConfig: database.ChatModelConfig{Model: "fallback-chat-model"}, + }, modelBuildOptions{}, generated, logger, @@ -255,10 +255,10 @@ func TestMaybeGenerateChatTitle_TitleGenerationOverrideSetUsable(t *testing.T) { chat, messages, nil, - "openai", - database.ChatModelConfig{Model: "fallback-chat-model"}, - chatprovider.NewModel(fallbackModel, nil), - aiGatewayModelRoute{}, + resolvedModelCall{ + model: chatprovider.NewModel(fallbackModel, nil), + dbConfig: database.ChatModelConfig{Model: "fallback-chat-model"}, + }, modelBuildOptions{ActiveAPIKeyID: uuid.NewString()}, generated, logger, @@ -297,10 +297,10 @@ func TestMaybeGenerateChatTitle_TitleGenerationOverrideSetUnusableSkips(t *testi chat, messages, nil, - "openai", - database.ChatModelConfig{Model: "fallback-chat-model"}, - chatprovider.NewModel(fallbackModel, nil), - aiGatewayModelRoute{}, + resolvedModelCall{ + model: chatprovider.NewModel(fallbackModel, nil), + dbConfig: database.ChatModelConfig{Model: "fallback-chat-model"}, + }, modelBuildOptions{}, generated, logger, @@ -351,10 +351,10 @@ func TestMaybeGenerateChatTitle_TitleGenerationOverrideCallFailureSkipsFallback( chat, messages, nil, - "openai", - database.ChatModelConfig{Model: "fallback-chat-model"}, - chatprovider.NewModel(fallbackModel, nil), - aiGatewayModelRoute{}, + resolvedModelCall{ + model: chatprovider.NewModel(fallbackModel, nil), + dbConfig: database.ChatModelConfig{Model: "fallback-chat-model"}, + }, modelBuildOptions{ActiveAPIKeyID: uuid.NewString()}, generated, logger, @@ -390,15 +390,15 @@ func TestResolveManualTitleModel_TitleGenerationOverrideUnset(t *testing.T) { db.EXPECT().GetAIProviderByID(gomock.Any(), providerID).Return(aibridgeTestAIProvider(providerID, "primary-openai", database.AIProviderTypeOpenai), nil).AnyTimes() server := titleOverrideTestServer(db, logger) - model, gotConfig, err := server.resolveManualTitleModel( + resolved, err := server.resolveManualTitleModel( ctx, db, chat, modelBuildOptions{ActiveAPIKeyID: uuid.NewString()}, ) require.NoError(t, err) - require.True(t, model.Valid()) - require.Equal(t, preferredConfig, gotConfig) + require.True(t, resolved.model.Valid()) + require.Equal(t, preferredConfig, resolved.dbConfig) } func TestResolveManualTitleModel_TitleGenerationOverrideUnsetAIProvider(t *testing.T) { @@ -439,15 +439,15 @@ func TestResolveManualTitleModel_TitleGenerationOverrideUnsetAIProvider(t *testi }}, nil).AnyTimes() server := titleOverrideTestServer(db, logger) - model, gotConfig, err := server.resolveManualTitleModel( + resolved, err := server.resolveManualTitleModel( ctx, db, chat, modelBuildOptions{ActiveAPIKeyID: uuid.NewString()}, ) require.NoError(t, err) - require.True(t, model.Valid()) - require.Equal(t, preferredConfig, gotConfig) + require.True(t, resolved.model.Valid()) + require.Equal(t, preferredConfig, resolved.dbConfig) } func TestResolveManualTitleModel_TitleGenerationOverrideReadDBError(t *testing.T) { @@ -474,15 +474,15 @@ func TestResolveManualTitleModel_TitleGenerationOverrideReadDBError(t *testing.T db.EXPECT().GetAIProviderByID(gomock.Any(), providerID).Return(aibridgeTestAIProvider(providerID, "primary-openai", database.AIProviderTypeOpenai), nil).AnyTimes() server := titleOverrideTestServer(db, logger) - model, gotConfig, err := server.resolveManualTitleModel( + resolved, err := server.resolveManualTitleModel( ctx, db, chat, modelBuildOptions{ActiveAPIKeyID: uuid.NewString()}, ) require.NoError(t, err) - require.True(t, model.Valid()) - require.Equal(t, preferredConfig, gotConfig) + require.True(t, resolved.model.Valid()) + require.Equal(t, preferredConfig, resolved.dbConfig) } func TestResolveManualTitleModel_TitleGenerationOverrideSetUsable(t *testing.T) { @@ -506,15 +506,15 @@ func TestResolveManualTitleModel_TitleGenerationOverrideSetUsable(t *testing.T) }}, nil).AnyTimes() server := titleOverrideTestServer(db, logger) - model, gotConfig, err := server.resolveManualTitleModel( + resolved, err := server.resolveManualTitleModel( ctx, db, chat, modelBuildOptions{ActiveAPIKeyID: uuid.NewString()}, ) require.NoError(t, err) - require.True(t, model.Valid()) - require.Equal(t, overrideConfig, gotConfig) + require.True(t, resolved.model.Valid()) + require.Equal(t, overrideConfig, resolved.dbConfig) } func TestResolveManualTitleModel_TitleGenerationOverrideMissingCredentials(t *testing.T) { @@ -540,7 +540,7 @@ func TestResolveManualTitleModel_TitleGenerationOverrideMissingCredentials(t *te db.EXPECT().GetAIProviderKeysByProviderID(gomock.Any(), providerID).Return(nil, nil).AnyTimes() server := titleOverrideTestServer(db, logger) - model, gotConfig, err := server.resolveManualTitleModel( + resolved, err := server.resolveManualTitleModel( ctx, db, chat, @@ -549,8 +549,7 @@ func TestResolveManualTitleModel_TitleGenerationOverrideMissingCredentials(t *te require.Error(t, err) require.ErrorContains(t, err, "resolve manual title generation model override") require.ErrorContains(t, err, "credentials are unavailable") - require.False(t, model.Valid()) - require.Equal(t, database.ChatModelConfig{}, gotConfig) + require.Equal(t, resolvedModelCall{}, resolved) } func TestGenerateManualTitleCandidate_UsesSyntheticAPIKey(t *testing.T) { @@ -636,7 +635,7 @@ func TestResolveManualTitleModel_TitleGenerationOverrideSetUnusable(t *testing.T db.EXPECT().GetChatModelConfigByID(gomock.Any(), overrideConfig.ID).Return(overrideConfig, nil) server := titleOverrideTestServer(db, logger) - model, gotConfig, err := server.resolveManualTitleModel( + resolved, err := server.resolveManualTitleModel( ctx, db, chat, @@ -645,8 +644,7 @@ func TestResolveManualTitleModel_TitleGenerationOverrideSetUnusable(t *testing.T require.Error(t, err) require.ErrorContains(t, err, "resolve manual title generation model override") require.ErrorContains(t, err, "title generation model override is unavailable") - require.False(t, model.Valid()) - require.Equal(t, database.ChatModelConfig{}, gotConfig) + require.Equal(t, resolvedModelCall{}, resolved) } func TestParseModelOverride(t *testing.T) { From 563ef86c12fa78412a4670c633e7606591b59502 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Tue, 11 Aug 2026 19:50:40 +0000 Subject: [PATCH 04/13] refactor(coderd/x/chatd): migrate compaction, advisor, and manual-title debug paths to resolver Compaction overrides build through compactionOverrideSpec, advisor overrides through advisorOverrideSpec with a typed modelCallConfigParseError preserving the soft fallback on corrupt options JSON, and manual-title debug rebuilds through manualTitleDebugSpec. Deletes newDebugAwareModel, buildCompactionOverrideModel, and compactionOverrideProviderOptions. --- coderd/x/chatd/advisor_internal_test.go | 51 ++++++++-- coderd/x/chatd/chatd.go | 99 +++++-------------- coderd/x/chatd/chatd_debug.go | 34 ------- coderd/x/chatd/compaction_override.go | 92 +---------------- .../compaction_override_internal_test.go | 69 +++++-------- coderd/x/chatd/generation.go | 9 +- coderd/x/chatd/generation_preparer.go | 3 +- coderd/x/chatd/modelcall.go | 79 ++++++++++++++- 8 files changed, 178 insertions(+), 258 deletions(-) diff --git a/coderd/x/chatd/advisor_internal_test.go b/coderd/x/chatd/advisor_internal_test.go index 8c1e979eb85..bf58530d77b 100644 --- a/coderd/x/chatd/advisor_internal_test.go +++ b/coderd/x/chatd/advisor_internal_test.go @@ -113,12 +113,11 @@ func (p *Server) resolveAdvisorModelOverrideOrFallback( modelOpts modelBuildOptions, logger slog.Logger, ) (chatprovider.Model, codersdk.ChatModelCallConfig) { - model, cfg, err := p.resolveAdvisorModelOverride( + resolved, err := p.resolveAdvisorModelOverride( ctx, chat, advisorCfg, - fallbackModel, - fallbackCallConfig, + resolvedModelCall{model: fallbackModel, callConfig: fallbackCallConfig}, modelOpts, logger, ) @@ -126,7 +125,7 @@ func (p *Server) resolveAdvisorModelOverrideOrFallback( logger.Warn(ctx, "failed to resolve advisor model override, continuing with chat model", slog.Error(err)) return fallbackModel, fallbackCallConfig } - return model, cfg + return resolved.model, resolved.callConfig } func (p *Server) newAdvisorRuntimeOrFallback( @@ -142,8 +141,7 @@ func (p *Server) newAdvisorRuntimeOrFallback( ctx, chat, advisorCfg, - fallbackModel, - fallbackCallConfig, + resolvedModelCall{model: fallbackModel, callConfig: fallbackCallConfig}, modelOpts, logger, ) @@ -268,6 +266,40 @@ func TestResolveAdvisorModelOverride(t *testing.T) { require.Equal(t, fallbackCallConfig, gotCfg) }) + // Corrupt options JSON on a provider-linked config must still fall + // back softly, unlike route or client failures which hard-fail for + // linked providers. Guards the modelCallConfigParseError distinction. + t.Run("InvalidOptionsJSONWithLinkedProviderReturnsFallback", func(t *testing.T) { + t.Parallel() + ctx := testutil.Context(t, testutil.WaitShort) + configID := uuid.New() + store := &advisorOverrideStubStore{ + getEnabledChatModelConfigByID: func(context.Context, uuid.UUID) (database.ChatModelConfig, error) { + return database.ChatModelConfig{ + ID: configID, + Model: "gpt-5.2", + Enabled: true, + Options: []byte("not valid json"), + DisplayName: "gpt-5.2", + AIProviderID: uuid.NullUUID{UUID: uuid.New(), Valid: true}, + }, nil + }, + } + p := newAdvisorTestServer(ctx, t, store) + + resolved, err := p.resolveAdvisorModelOverride( + ctx, + database.Chat{}, + codersdk.AdvisorConfig{ModelConfigID: configID}, + resolvedModelCall{model: fallbackModel, callConfig: fallbackCallConfig}, + modelBuildOptions{}, + logger, + ) + require.NoError(t, err) + require.Equal(t, fallbackModel, resolved.model) + require.Equal(t, fallbackCallConfig, resolved.callConfig) + }) + t.Run("MissingProviderKeyReturnsFallback", func(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitShort) @@ -446,17 +478,16 @@ func TestResolveAdvisorModelOverridePromotesAIBridgeErrors(t *testing.T) { p := newAdvisorTestServer(ctx, t, store) ctx = aibridge.WithDelegatedAPIKeyID(ctx, uuid.NewString()) - model, _, err := p.resolveAdvisorModelOverride( + resolved, err := p.resolveAdvisorModelOverride( ctx, database.Chat{ID: uuid.New(), OwnerID: uuid.New()}, codersdk.AdvisorConfig{ModelConfigID: configID}, - chatprovider.NewModel(&chattest.FakeModel{ProviderName: "stub", ModelName: "stub"}, nil), - codersdk.ChatModelCallConfig{}, + resolvedModelCall{model: chatprovider.NewModel(&chattest.FakeModel{ProviderName: "stub", ModelName: "stub"}, nil)}, modelBuildOptions{ActiveAPIKeyID: uuid.NewString()}, slog.Make(), ) require.ErrorContains(t, err, "AI Gateway transport factory") - require.False(t, model.Valid()) + require.False(t, resolved.model.Valid()) } // TestStripAdvisorGuidanceBlock exercises the filter that keeps the advisor diff --git a/coderd/x/chatd/chatd.go b/coderd/x/chatd/chatd.go index 276a5abf4ce..d5097df9412 100644 --- a/coderd/x/chatd/chatd.go +++ b/coderd/x/chatd/chatd.go @@ -246,13 +246,12 @@ func (p *Server) resolveAdvisorModelOverride( ctx context.Context, chat database.Chat, advisorCfg codersdk.AdvisorConfig, - fallbackModel chatprovider.Model, - fallbackCallConfig codersdk.ChatModelCallConfig, + fallback resolvedModelCall, modelOpts modelBuildOptions, logger slog.Logger, -) (chatprovider.Model, codersdk.ChatModelCallConfig, error) { +) (resolvedModelCall, error) { if advisorCfg.ModelConfigID == uuid.Nil { - return fallbackModel, fallbackCallConfig, nil + return fallback, nil } // Re-read the override instead of using the cache so disabled models @@ -268,7 +267,7 @@ func (p *Server) resolveAdvisorModelOverride( "advisor model config is disabled or unavailable, continuing with chat model", slog.F("model_config_id", advisorCfg.ModelConfigID), ) - return fallbackModel, fallbackCallConfig, nil + return fallback, nil } logger.Warn( ctx, @@ -276,90 +275,56 @@ func (p *Server) resolveAdvisorModelOverride( slog.F("model_config_id", advisorCfg.ModelConfigID), slog.Error(err), ) - return fallbackModel, fallbackCallConfig, nil + return fallback, nil } - overrideCallConfig := codersdk.ChatModelCallConfig{} - if len(overrideConfig.Options) > 0 { - if err := json.Unmarshal(overrideConfig.Options, &overrideCallConfig); err != nil { - logger.Warn( - ctx, - "failed to parse advisor model config, continuing with chat model", - slog.F("model_config_id", advisorCfg.ModelConfigID), - slog.Error(err), - ) - return fallbackModel, fallbackCallConfig, nil - } - } - - route, err := p.resolveModelRouteForConfig( - ctx, - chat.OwnerID, - overrideConfig, - ) + resolved, err := p.resolveModelCall(ctx, advisorOverrideSpec(chat, overrideConfig, modelOpts)) if err != nil { - if overrideConfig.AIProviderID.Valid { - return chatprovider.Model{}, codersdk.ChatModelCallConfig{}, xerrors.Errorf("resolve advisor override route: %w", err) + // Corrupt options JSON always falls back so a bad admin edit cannot + // break every turn; route and client failures fall back only when + // the config has no linked provider. + var parseErr modelCallConfigParseError + if overrideConfig.AIProviderID.Valid && !xerrors.As(err, &parseErr) { + return resolvedModelCall{}, xerrors.Errorf("resolve advisor override model: %w", err) } logger.Warn( ctx, - "failed to resolve advisor override route, continuing with chat model", + "failed to resolve advisor override model, continuing with chat model", slog.F("model_config_id", advisorCfg.ModelConfigID), slog.Error(err), ) - return fallbackModel, fallbackCallConfig, nil - } - overrideModel, err := p.newModel(ctx, modelClientRequest{ - Chat: chat, - ModelName: overrideConfig.Model, - UserAgent: chatprovider.UserAgent(), - ExtraHeaders: chatprovider.CoderHeaders(chat), - ConfigOptions: overrideConfig.Options, - }, route, modelOpts) - if err != nil { - if overrideConfig.AIProviderID.Valid { - return chatprovider.Model{}, codersdk.ChatModelCallConfig{}, xerrors.Errorf("create advisor override model: %w", err) - } - logger.Warn( - ctx, - "failed to create advisor override model, continuing with chat model", - slog.F("model_config_id", advisorCfg.ModelConfigID), - slog.Error(err), - ) - return fallbackModel, fallbackCallConfig, nil + return fallback, nil } if advisorCfg.ReasoningEffort != nil { resolvedEffort := chatprovider.ResolveReasoningEffort( advisorCfg.ReasoningEffort, - overrideCallConfig.ReasoningEffort, + resolved.callConfig.ReasoningEffort, ) if resolvedEffort != nil { - overrideCallConfig.ReasoningEffort = &codersdk.ChatModelReasoningEffortConfig{ + resolved.callConfig.ReasoningEffort = &codersdk.ChatModelReasoningEffortConfig{ Default: resolvedEffort, Max: resolvedEffort, } } } - return overrideModel, overrideCallConfig, nil + return resolved, nil } func (p *Server) newAdvisorRuntime( ctx context.Context, chat database.Chat, advisorCfg codersdk.AdvisorConfig, - fallbackModel chatprovider.Model, - fallbackCallConfig codersdk.ChatModelCallConfig, + fallback resolvedModelCall, modelOpts modelBuildOptions, logger slog.Logger, ) (*chatadvisor.Runtime, error) { - advisorModel, advisorCallConfig, err := p.resolveAdvisorModelOverride( + advisor, err := p.resolveAdvisorModelOverride( ctx, chat, advisorCfg, - fallbackModel, - fallbackCallConfig, + fallback, modelOpts, logger, ) @@ -389,13 +354,14 @@ func (p *Server) newAdvisorRuntime( maxOutputTokens = defaultAdvisorMaxOutputTokens } + advisorCallConfig := advisor.callConfig advisorCallConfig.MaxOutputTokens = ptr.Ref(maxOutputTokens) // The override resolver pins an explicit advisor effort into the model // config. Fallback models keep their configured default effort. - providerOptions := chatprovider.ProviderOptionsForCall(advisorModel, advisorCallConfig, nil) + providerOptions := advisor.deriveProviderOptions(advisorCallConfig, nil) rt, err := chatadvisor.NewRuntime(chatadvisor.RuntimeConfig{ - Model: advisorModel.LanguageModel(), + Model: advisor.model.LanguageModel(), ModelConfig: advisorCallConfig, ProviderOptions: providerOptions, MaxUsesPerRun: maxUsesPerRun, @@ -2622,20 +2588,14 @@ func (p *Server) prepareManualTitleDebugRun( routeProvider = string(provider.Type) } } - debugOpts := modelOpts - debugOpts.RecordHTTP = true var debugModelErr error var debugModel chatprovider.Model if routeErr != nil { debugModelErr = routeErr } else { - debugModel, debugModelErr = p.newModel(ctx, modelClientRequest{ - Chat: chat, - ModelName: modelConfig.Model, - UserAgent: chatprovider.UserAgent(), - ExtraHeaders: chatprovider.CoderHeaders(chat), - ConfigOptions: modelConfig.Options, - }, route, debugOpts) + var debugResolved resolvedModelCall + debugResolved, debugModelErr = p.resolveModelCall(ctx, manualTitleDebugSpec(chat, modelConfig, route, debugSvc, routeProvider, modelOpts)) + debugModel = debugResolved.model } switch { case debugModelErr != nil: @@ -2650,12 +2610,7 @@ func (p *Server) prepareManualTitleDebugRun( slog.F("model", modelConfig.Model), ) default: - titleModel = debugModel.WithLanguageModel(chatdebug.WrapModel(debugModel.LanguageModel(), debugSvc, chatdebug.RecorderOptions{ - ChatID: chat.ID, - OwnerID: chat.OwnerID, - Provider: routeProvider, - Model: modelConfig.Model, - })) + titleModel = debugModel } var historyTipMessageID int64 diff --git a/coderd/x/chatd/chatd_debug.go b/coderd/x/chatd/chatd_debug.go index 8fdf19c6a2e..3787a3a073f 100644 --- a/coderd/x/chatd/chatd_debug.go +++ b/coderd/x/chatd/chatd_debug.go @@ -6,7 +6,6 @@ import ( "cdr.dev/slog/v3" "github.com/coder/coder/v2/coderd/x/chatd/chatdebug" - "github.com/coder/coder/v2/coderd/x/chatd/chatprovider" ) const ( @@ -110,36 +109,3 @@ func (p *Server) scheduleDebugCleanup( p.logger.Error(context.WithoutCancel(ctx), "failed to schedule chat debug cleanup", logFields...) } } - -func (p *Server) newDebugAwareModel( - ctx context.Context, - req modelClientRequest, - route aiGatewayModelRoute, - opts modelBuildOptions, -) (chatprovider.Model, bool, error) { - provider, resolvedModel, err := chatprovider.ResolveModelWithProviderHint(req.ModelName, route.ModelProviderHint) - if err != nil { - return chatprovider.Model{}, false, err - } - route.ModelProviderHint = provider - req.ModelName = resolvedModel - - debugSvc := p.debugService() - debugEnabled := debugSvc != nil && debugSvc.IsEnabled(ctx, req.Chat.ID, req.Chat.OwnerID) - opts.RecordHTTP = debugEnabled - - model, err := p.newModel(ctx, req, route, opts) - if err != nil { - return chatprovider.Model{}, debugEnabled, err - } - if !debugEnabled { - return model, false, nil - } - - return model.WithLanguageModel(chatdebug.WrapModel(model.LanguageModel(), debugSvc, chatdebug.RecorderOptions{ - ChatID: req.Chat.ID, - OwnerID: req.Chat.OwnerID, - Provider: provider, - Model: resolvedModel, - })), true, nil -} diff --git a/coderd/x/chatd/compaction_override.go b/coderd/x/chatd/compaction_override.go index fc764ab58db..fb8941b7b7f 100644 --- a/coderd/x/chatd/compaction_override.go +++ b/coderd/x/chatd/compaction_override.go @@ -2,16 +2,13 @@ package chatd import ( "context" - "encoding/json" - "charm.land/fantasy" "github.com/google/uuid" "golang.org/x/xerrors" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbauthz" "github.com/coder/coder/v2/coderd/x/chatd/chatprovider" - "github.com/coder/coder/v2/codersdk" ) const compactionOverrideContext = "compaction" @@ -32,18 +29,6 @@ func readCompactionModelOverride( return raw, nil } -// compactionModelOverride carries the built compaction override model plus -// the identity metadata debug runs and prompt sanitization need. -type compactionModelOverride struct { - modelConfig database.ChatModelConfig - model chatprovider.Model - resolvedProvider string - resolvedModel string - // providerOptions include the override's reasoning effort for the - // summary call. - providerOptions fantasy.ProviderOptions -} - // resolvedCompactionOverride is the compaction override resolved at // prepare time. The provider/model identity is resolved without building // the model client so metrics recorded before the client exists @@ -52,8 +37,8 @@ type resolvedCompactionOverride struct { Config database.ChatModelConfig // ResolvedProvider and ResolvedModel match the built client's // identity: ResolveModelWithProviderHint normalizes its hint, so the - // normalized provider name here and the route's raw provider type in - // buildCompactionOverrideModel yield the same result. + // normalized provider name here and the route hint resolveModelCall + // uses at build time yield the same result. ResolvedProvider string ResolvedModel string } @@ -106,76 +91,3 @@ func (p *Server) resolveCompactionOverrideConfig( ResolvedModel: resolvedModel, }, nil } - -// buildCompactionOverrideModel resolves the route and constructs the model -// client for a usable override config. Errors are hard failures: a usable -// override that cannot be constructed must fail the generation visibly -// instead of silently compacting with the chat model. -func (p *Server) buildCompactionOverrideModel( - ctx context.Context, - chat database.Chat, - modelConfig database.ChatModelConfig, - modelOpts modelBuildOptions, -) (compactionModelOverride, error) { - //nolint:gocritic // Compaction overrides need chatd-scoped provider reads for user-owned chats. - route, err := p.resolveModelRouteForConfig(dbauthz.AsChatd(ctx), chat.OwnerID, modelConfig) - if err != nil { - return compactionModelOverride{}, xerrors.Errorf( - "resolve compaction model override route: %w", - err, - ) - } - resolvedProvider, resolvedModel, err := chatprovider.ResolveModelWithProviderHint( - modelConfig.Model, - route.ModelProviderHint, - ) - if err != nil { - return compactionModelOverride{}, xerrors.Errorf( - "resolve compaction model override metadata: %w", - err, - ) - } - model, _, err := p.newDebugAwareModel(ctx, modelClientRequest{ - Chat: chat, - ModelName: modelConfig.Model, - UserAgent: chatprovider.UserAgent(), - ExtraHeaders: chatprovider.CoderHeaders(chat), - ConfigOptions: modelConfig.Options, - }, route, modelOpts) - if err != nil { - return compactionModelOverride{}, xerrors.Errorf( - "create compaction model override: %w", - err, - ) - } - providerOptions, err := compactionOverrideProviderOptions(model, modelConfig) - if err != nil { - return compactionModelOverride{}, err - } - return compactionModelOverride{ - modelConfig: modelConfig, - model: model, - resolvedProvider: resolvedProvider, - resolvedModel: resolvedModel, - providerOptions: providerOptions, - }, nil -} - -// compactionOverrideProviderOptions converts the override config's call -// options, including the admin-resolved reasoning effort, into provider -// options for the summary call. -func compactionOverrideProviderOptions( - model chatprovider.Model, - modelConfig database.ChatModelConfig, -) (fantasy.ProviderOptions, error) { - callConfig := codersdk.ChatModelCallConfig{} - if len(modelConfig.Options) > 0 { - if err := json.Unmarshal(modelConfig.Options, &callConfig); err != nil { - return nil, xerrors.Errorf( - "parse compaction model override call config: %w", - err, - ) - } - } - return chatprovider.ProviderOptionsForCall(model, callConfig, nil), nil -} diff --git a/coderd/x/chatd/compaction_override_internal_test.go b/coderd/x/chatd/compaction_override_internal_test.go index 166263c3d52..2bcaa1a599a 100644 --- a/coderd/x/chatd/compaction_override_internal_test.go +++ b/coderd/x/chatd/compaction_override_internal_test.go @@ -5,7 +5,7 @@ import ( "encoding/json" "testing" - fantasyanthropic "charm.land/fantasy/providers/anthropic" + fantasyopenai "charm.land/fantasy/providers/openai" "github.com/google/uuid" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" @@ -13,49 +13,10 @@ import ( "cdr.dev/slog/v3/sloggers/slogtest" "github.com/coder/coder/v2/coderd/database" "github.com/coder/coder/v2/coderd/database/dbmock" - "github.com/coder/coder/v2/coderd/x/chatd/chatprovider" - "github.com/coder/coder/v2/coderd/x/chatd/chattest" "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/testutil" ) -func TestCompactionOverrideProviderOptions(t *testing.T) { - t.Parallel() - - model := chatprovider.NewModel(&chattest.FakeModel{ProviderName: "anthropic", ModelName: "claude-3-5-haiku"}, nil) - - t.Run("NoOptions", func(t *testing.T) { - t.Parallel() - opts, err := compactionOverrideProviderOptions(model, database.ChatModelConfig{}) - require.NoError(t, err) - require.Nil(t, opts) - }) - - t.Run("ReasoningEffort", func(t *testing.T) { - t.Parallel() - effort := "low" - options, err := json.Marshal(codersdk.ChatModelCallConfig{ - ReasoningEffort: &codersdk.ChatModelReasoningEffortConfig{ - Default: &effort, - Max: &effort, - }, - }) - require.NoError(t, err) - opts, err := compactionOverrideProviderOptions(model, database.ChatModelConfig{Options: options}) - require.NoError(t, err) - anthropicOpts, ok := opts[fantasyanthropic.Name].(*fantasyanthropic.ProviderOptions) - require.True(t, ok) - require.NotNil(t, anthropicOpts.Effort) - require.Equal(t, fantasyanthropic.Effort("low"), *anthropicOpts.Effort) - }) - - t.Run("MalformedOptions", func(t *testing.T) { - t.Parallel() - _, err := compactionOverrideProviderOptions(model, database.ChatModelConfig{Options: []byte("{")}) - require.ErrorContains(t, err, "parse compaction model override call config") - }) -} - func TestResolveCompactionOverrideConfig_Unset(t *testing.T) { t.Parallel() @@ -184,6 +145,15 @@ func TestCompactionOverride_SetUsable(t *testing.T) { overrideConfig := titleOverrideModelConfig("gpt-4.1", true) providerID := uuid.New() overrideConfig.AIProviderID = uuid.NullUUID{UUID: providerID, Valid: true} + effort := "low" + options, err := json.Marshal(codersdk.ChatModelCallConfig{ + ReasoningEffort: &codersdk.ChatModelReasoningEffortConfig{ + Default: &effort, + Max: &effort, + }, + }) + require.NoError(t, err) + overrideConfig.Options = options db.EXPECT().GetChatCompactionModelOverride(gomock.Any()).Return(overrideConfig.ID.String(), nil) db.EXPECT().GetChatModelConfigByID(gomock.Any(), overrideConfig.ID).Return(overrideConfig, nil) @@ -199,19 +169,28 @@ func TestCompactionOverride_SetUsable(t *testing.T) { require.NotNil(t, resolved) require.Equal(t, overrideConfig.ID, resolved.Config.ID) - override, err := server.buildCompactionOverrideModel( - ctx, + override, err := server.resolveModelCall(ctx, compactionOverrideSpec( chat, resolved.Config, modelBuildOptions{ActiveAPIKeyID: uuid.NewString()}, - ) + )) require.NoError(t, err) - require.NotNil(t, override.model) - require.Equal(t, overrideConfig.ID, override.modelConfig.ID) + require.True(t, override.model.Valid()) + require.Equal(t, overrideConfig.ID, override.dbConfig.ID) require.Equal(t, "openai", override.resolvedProvider) require.Equal(t, "gpt-4.1", override.resolvedModel) // Prepare-time identity must match the built client's so // still-over-limit metrics land on the same series. require.Equal(t, override.resolvedProvider, resolved.ResolvedProvider) require.Equal(t, override.resolvedModel, resolved.ResolvedModel) + // The summary call derives provider options from the override config, + // including the admin-resolved reasoning effort. + switch opts := override.providerOptions[fantasyopenai.Name].(type) { + case *fantasyopenai.ResponsesProviderOptions: + require.Equal(t, fantasyopenai.ReasoningEffort(effort), *opts.ReasoningEffort) + case *fantasyopenai.ProviderOptions: + require.Equal(t, fantasyopenai.ReasoningEffort(effort), *opts.ReasoningEffort) + default: + t.Fatalf("unexpected openai provider options type %T", opts) + } } diff --git a/coderd/x/chatd/generation.go b/coderd/x/chatd/generation.go index 5fb7f00e6fd..377322fa97a 100644 --- a/coderd/x/chatd/generation.go +++ b/coderd/x/chatd/generation.go @@ -919,7 +919,10 @@ func (s *taskStarter) generateCompaction( compactionOpts := prepared.Compaction.Options metricProvider, metricModel := compactionMetricIdentity(prepared.Compaction) if override := prepared.Compaction.Override; override != nil { - overrideModel, err := s.server.buildCompactionOverrideModel(ctx, prepared.Chat, override.Config, prepared.ModelBuildOptions) + // Errors are hard failures: a usable override that cannot be + // constructed must fail the generation visibly instead of silently + // compacting with the chat model. + overrideModel, err := s.server.resolveModelCall(ctx, compactionOverrideSpec(prepared.Chat, override.Config, prepared.ModelBuildOptions)) if err != nil { return xerrors.Errorf("build compaction model override: %w", err) } @@ -930,7 +933,7 @@ func (s *taskStarter) generateCompaction( compactionOpts.Model = overrideModel.model.LanguageModel() compactionOpts.ResolvedProvider = overrideModel.resolvedProvider compactionOpts.ResolvedModel = overrideModel.resolvedModel - compactionOpts.ModelConfigID = overrideModel.modelConfig.ID + compactionOpts.ModelConfigID = overrideModel.dbConfig.ID compactionOpts.ProviderOptions = overrideModel.providerOptions compactionOpts.Messages = sanitizeCompactionPrompt( ctx, @@ -938,7 +941,7 @@ func (s *taskStarter) generateCompaction( compactionOpts.Messages, overrideModel.model, prepared.Compaction.ChatModelConfig, - overrideModel.modelConfig, + overrideModel.dbConfig, ) } preResult, err := s.server.hooks.Trigger(ctx, chathooks.ChatFor(prepared.Chat, input.hookTurnID()), chathooks.Message{}, agenthooks.EventPreCompact, dispatch.CapacityClassGeneration) diff --git a/coderd/x/chatd/generation_preparer.go b/coderd/x/chatd/generation_preparer.go index 6ac6bb5500c..af88ccbe093 100644 --- a/coderd/x/chatd/generation_preparer.go +++ b/coderd/x/chatd/generation_preparer.go @@ -181,8 +181,7 @@ func (server *Server) prepareGeneration( ctx, chat, advisorCfg, - model, - callConfig, + resolved, modelOpts, logger, ) diff --git a/coderd/x/chatd/modelcall.go b/coderd/x/chatd/modelcall.go index 70c3aa1ee93..131f9c39497 100644 --- a/coderd/x/chatd/modelcall.go +++ b/coderd/x/chatd/modelcall.go @@ -195,6 +195,61 @@ func manualTitleSpec(chat database.Chat, config database.ChatModelConfig, buildO } } +// compactionOverrideSpec builds the deployment-wide compaction override +// model from the caller-selected config row, whose reasoning effort was +// already resolved at prepare time. The route resolves with chatd scope so +// the override works for chats whose owner cannot read the provider. +func compactionOverrideSpec(chat database.Chat, config database.ChatModelConfig, buildOpts modelBuildOptions) modelCallSpec { + return modelCallSpec{ + purpose: callPurposeCompaction, + chat: chat, + config: configSelection{mode: configExplicit, config: config}, + providerOptions: providerOptionsDerive, + debug: debugPolicyAware, + chatdScopedRoute: true, + buildOptions: buildOpts, + } +} + +// advisorOverrideSpec builds the advisor's override model from the config +// row the caller re-read from the database. Provider options are omitted +// because the advisor derives them after pinning its reasoning effort and +// output cap into the call config. +func advisorOverrideSpec(chat database.Chat, config database.ChatModelConfig, buildOpts modelBuildOptions) modelCallSpec { + return modelCallSpec{ + purpose: callPurposeAdvisor, + chat: chat, + config: configSelection{mode: configExplicit, config: config}, + providerOptions: providerOptionsOmit, + buildOptions: buildOpts, + } +} + +// manualTitleDebugSpec rebuilds the manual-title client with HTTP recording +// after the caller verified debug is enabled and resolved the route itself +// (the route's provider type also labels the debug run record). +func manualTitleDebugSpec( + chat database.Chat, + config database.ChatModelConfig, + route aiGatewayModelRoute, + debugSvc *chatdebug.Service, + routeProvider string, + buildOpts modelBuildOptions, +) modelCallSpec { + return modelCallSpec{ + purpose: callPurposeDebugRebuild, + chat: chat, + config: configSelection{mode: configExplicit, config: config}, + providerOptions: providerOptionsOmit, + debug: debugPolicyForced, + debugSvc: debugSvc, + debugWrapProvider: routeProvider, + debugWrapModel: config.Model, + routeOverride: &route, + buildOptions: buildOpts, + } +} + // computerUseSpec swaps in the deployment's computer-use model. The client is // built without config options because the fixed model has no config row; the // chat model's call config still drives per-call provider options so admin @@ -222,6 +277,18 @@ func computerUseSpec( } } +// modelCallConfigParseError marks malformed model-config options JSON. The +// advisor override falls back to the chat model on corrupt options while +// hard-failing on route and client errors, so it matches this type with +// xerrors.As. +type modelCallConfigParseError struct{ err error } + +func (e modelCallConfigParseError) Error() string { + return "parse model call config: " + e.err.Error() +} + +func (e modelCallConfigParseError) Unwrap() error { return e.err } + // resolvedModelCall is the output of resolveModelCall: a ready client plus // the metadata callers need for prompts, metrics, and debug attribution. type resolvedModelCall struct { @@ -273,7 +340,7 @@ func (p *Server) resolveModelCall(ctx context.Context, spec modelCallSpec) (reso var err error out.callConfig, err = parseModelConfigOptions(configOptions) if err != nil { - return resolvedModelCall{}, xerrors.Errorf("parse model call config: %w", err) + return resolvedModelCall{}, modelCallConfigParseError{err: err} } } if spec.defaultMaxOutputTokens && out.callConfig.MaxOutputTokens == nil { @@ -361,7 +428,7 @@ func (p *Server) resolveModelCall(ctx context.Context, spec modelCallSpec) (reso out.model = model if spec.providerOptions == providerOptionsDerive { - out.providerOptions = chatprovider.ProviderOptionsForCall(model, out.callConfig, spec.requestedEffort) + out.providerOptions = out.deriveProviderOptions(out.callConfig, spec.requestedEffort) } p.logger.Debug(ctx, "resolved model call", @@ -374,6 +441,14 @@ func (p *Server) resolveModelCall(ctx context.Context, spec modelCallSpec) (reso return out, nil } +// deriveProviderOptions converts a call config into per-call provider +// options for this resolved model. resolveModelCall derives from the spec's +// parsed config; callers that mutate the call config after resolution +// (advisor) re-derive here. +func (r resolvedModelCall) deriveProviderOptions(callConfig codersdk.ChatModelCallConfig, requestedEffort *string) fantasy.ProviderOptions { + return chatprovider.ProviderOptionsForCall(r.model, callConfig, requestedEffort) +} + // objectCallOverrides carries the caller-owned schema and token cap for a // structured-output call. Quickgen flows use fixed caps instead of the model // config's tuning. From 16936bb2032abaf01a4a1c37fe528282b99ec0da Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Tue, 11 Aug 2026 19:59:41 +0000 Subject: [PATCH 05/13] refactor(coderd/x/chatd): route call envelopes through resolver-built templates resolvedModelCall.newCall is now the only production fantasy.Call construction point. chatloop.GenerateAssistantOptions carries a CallTemplate instead of ModelConfig plus ProviderOptions, compaction options carry a prebuilt SummaryCall with tool use forbidden, and chatadvisor.RuntimeConfig carries a CallTemplate that runs clone before mutating provider options. The chat-model compaction summary keeps its historical provider-option omission via compactionSummaryOverrides. --- coderd/x/chatd/chatadvisor/runner.go | 12 ++--- coderd/x/chatd/chatadvisor/runner_test.go | 9 ++-- coderd/x/chatd/chatadvisor/runtime.go | 24 +++++----- coderd/x/chatd/chatd.go | 8 ++-- coderd/x/chatd/chatloop/chatloop.go | 28 +++++------ coderd/x/chatd/chatloop/compaction.go | 15 +++--- coderd/x/chatd/generation.go | 12 ++--- coderd/x/chatd/generation_preparer.go | 4 +- .../generation_preparer_internal_test.go | 8 ++-- coderd/x/chatd/modelcall.go | 47 +++++++++++++++++++ .../x/chatd/modelcall_shape_internal_test.go | 14 +++--- 11 files changed, 110 insertions(+), 71 deletions(-) diff --git a/coderd/x/chatd/chatadvisor/runner.go b/coderd/x/chatd/chatadvisor/runner.go index da847ce1b0f..0f3e9c1f0c3 100644 --- a/coderd/x/chatd/chatadvisor/runner.go +++ b/coderd/x/chatd/chatadvisor/runner.go @@ -47,14 +47,14 @@ func (rt *Runtime) RunAdvisor( // resetProviderOptionsForNestedCall mutates its argument; give it a // clone so the Runtime's stored options stay unchanged across calls. - nestedProviderOptions := cloneProviderOptions(rt.cfg.ProviderOptions) - resetProviderOptionsForNestedCall(nestedProviderOptions) + nestedCall := rt.cfg.CallTemplate + nestedCall.ProviderOptions = cloneProviderOptions(rt.cfg.CallTemplate.ProviderOptions) + resetProviderOptionsForNestedCall(nestedCall.ProviderOptions) assistantOpts := chatloop.GenerateAssistantOptions{ - Model: rt.cfg.Model, - Messages: BuildAdvisorMessages(question, conversationSnapshot), - ModelConfig: rt.cfg.ModelConfig, - ProviderOptions: nestedProviderOptions, + Model: rt.cfg.Model, + Messages: BuildAdvisorMessages(question, conversationSnapshot), + CallTemplate: nestedCall, } if opts != nil && opts.OnAdviceDelta != nil { assistantOpts.PublishMessagePart = func(role codersdk.ChatMessageRole, part codersdk.ChatMessagePart) { diff --git a/coderd/x/chatd/chatadvisor/runner_test.go b/coderd/x/chatd/chatadvisor/runner_test.go index c3830ec4fee..84b4144a03e 100644 --- a/coderd/x/chatd/chatadvisor/runner_test.go +++ b/coderd/x/chatd/chatadvisor/runner_test.go @@ -14,7 +14,6 @@ import ( "github.com/coder/coder/v2/coderd/x/chatd/chatadvisor" "github.com/coder/coder/v2/coderd/x/chatd/chattest" - "github.com/coder/coder/v2/codersdk" ) func TestAdvisorRunAdvice(t *testing.T) { @@ -425,12 +424,12 @@ func TestNewRuntimeValidation(t *testing.T) { errText: "advisor max output tokens must be positive", }, { - name: "MismatchedModelConfigMaxOutputTokens", + name: "MismatchedCallTemplateMaxOutputTokens", cfg: chatadvisor.RuntimeConfig{ Model: model, MaxUsesPerRun: 1, MaxOutputTokens: matchingTokens, - ModelConfig: codersdk.ChatModelCallConfig{ + CallTemplate: fantasy.Call{ MaxOutputTokens: &mismatchedTokens, }, }, @@ -473,7 +472,7 @@ func TestNewRuntimeDeepClonesOpenAIResponsesProviderOptions(t *testing.T) { }), nil }, }, - ProviderOptions: parentProviderOpts, + CallTemplate: fantasy.Call{ProviderOptions: parentProviderOpts}, MaxUsesPerRun: 1, MaxOutputTokens: 64, }) @@ -532,7 +531,7 @@ func TestAdvisorRunDisablesStoreAndIsConsistentAcrossCalls(t *testing.T) { }), nil }, }, - ProviderOptions: parentProviderOpts, + CallTemplate: fantasy.Call{ProviderOptions: parentProviderOpts}, MaxUsesPerRun: 2, MaxOutputTokens: 64, }) diff --git a/coderd/x/chatd/chatadvisor/runtime.go b/coderd/x/chatd/chatadvisor/runtime.go index d7282e9706d..092ffbed1c9 100644 --- a/coderd/x/chatd/chatadvisor/runtime.go +++ b/coderd/x/chatd/chatadvisor/runtime.go @@ -6,15 +6,15 @@ import ( "charm.land/fantasy" fantasyopenai "charm.land/fantasy/providers/openai" "golang.org/x/xerrors" - - "github.com/coder/coder/v2/codersdk" ) // RuntimeConfig configures a single advisor runtime instance. type RuntimeConfig struct { - Model fantasy.LanguageModel - ModelConfig codersdk.ChatModelCallConfig - ProviderOptions fantasy.ProviderOptions + Model fantasy.LanguageModel + // CallTemplate is the prebuilt advisor call envelope. Each advisor run + // copies it, clones its provider options, and attaches the nested + // prompt. + CallTemplate fantasy.Call MaxUsesPerRun int MaxOutputTokens int64 } @@ -44,19 +44,19 @@ func NewRuntime(cfg RuntimeConfig) (*Runtime, error) { if cfg.MaxOutputTokens <= 0 { return nil, xerrors.New("advisor max output tokens must be positive") } - if cfg.ModelConfig.MaxOutputTokens != nil && - *cfg.ModelConfig.MaxOutputTokens != cfg.MaxOutputTokens { + if cfg.CallTemplate.MaxOutputTokens != nil && + *cfg.CallTemplate.MaxOutputTokens != cfg.MaxOutputTokens { return nil, xerrors.Errorf( - "advisor model_config.max_output_tokens (%d) must match runtime max output tokens (%d)", - *cfg.ModelConfig.MaxOutputTokens, + "advisor call template max output tokens (%d) must match runtime max output tokens (%d)", + *cfg.CallTemplate.MaxOutputTokens, cfg.MaxOutputTokens, ) } normalized := cfg - normalized.ProviderOptions = cloneProviderOptions(cfg.ProviderOptions) + normalized.CallTemplate.ProviderOptions = cloneProviderOptions(cfg.CallTemplate.ProviderOptions) maxOutputTokens := cfg.MaxOutputTokens - normalized.ModelConfig.MaxOutputTokens = &maxOutputTokens + normalized.CallTemplate.MaxOutputTokens = &maxOutputTokens return &Runtime{cfg: normalized}, nil } @@ -134,7 +134,7 @@ func (rt *Runtime) ProviderOptions() fantasy.ProviderOptions { if rt == nil { return nil } - return rt.cfg.ProviderOptions + return rt.cfg.CallTemplate.ProviderOptions } func (rt *Runtime) tryAcquire() bool { diff --git a/coderd/x/chatd/chatd.go b/coderd/x/chatd/chatd.go index d5097df9412..e56a8d79360 100644 --- a/coderd/x/chatd/chatd.go +++ b/coderd/x/chatd/chatd.go @@ -354,16 +354,14 @@ func (p *Server) newAdvisorRuntime( maxOutputTokens = defaultAdvisorMaxOutputTokens } - advisorCallConfig := advisor.callConfig - advisorCallConfig.MaxOutputTokens = ptr.Ref(maxOutputTokens) + advisor.callConfig.MaxOutputTokens = ptr.Ref(maxOutputTokens) // The override resolver pins an explicit advisor effort into the model // config. Fallback models keep their configured default effort. - providerOptions := advisor.deriveProviderOptions(advisorCallConfig, nil) + advisor.providerOptions = advisor.deriveProviderOptions(advisor.callConfig, nil) rt, err := chatadvisor.NewRuntime(chatadvisor.RuntimeConfig{ Model: advisor.model.LanguageModel(), - ModelConfig: advisorCallConfig, - ProviderOptions: providerOptions, + CallTemplate: advisor.newCall(callOverrides{}), MaxUsesPerRun: maxUsesPerRun, MaxOutputTokens: maxOutputTokens, }) diff --git a/coderd/x/chatd/chatloop/chatloop.go b/coderd/x/chatd/chatloop/chatloop.go index 89063ff0612..20f244a9363 100644 --- a/coderd/x/chatd/chatloop/chatloop.go +++ b/coderd/x/chatd/chatloop/chatloop.go @@ -215,8 +215,10 @@ type GenerateAssistantOptions struct { Clock quartz.Clock ContextLimitFallback int64 - ModelConfig codersdk.ChatModelCallConfig - ProviderOptions fantasy.ProviderOptions + // CallTemplate is the prebuilt call envelope (sampling fields, token + // cap, provider options). GenerateAssistant copies it and attaches the + // prepared prompt and tool definitions. + CallTemplate fantasy.Call PublishMessagePart func(codersdk.ChatMessageRole, codersdk.ChatMessagePart) // OnModelStreamStart runs immediately before the provider stream is @@ -305,9 +307,11 @@ type GenerateCompactionOptions struct { ResolvedModel string ModelConfigID uuid.UUID - // ProviderOptions carry summary-model call options such as an - // override's reasoning effort. - ProviderOptions fantasy.ProviderOptions + // SummaryCall is the prebuilt summary-call envelope, carrying the + // tool-choice mode and summary-model provider options such as an + // override's reasoning effort. The summary prompt is attached before + // sending. + SummaryCall fantasy.Call PublishMessagePart func(codersdk.ChatMessageRole, codersdk.ChatMessagePart) @@ -399,17 +403,9 @@ func GenerateAssistant(ctx context.Context, opts GenerateAssistantOptions) (Assi opts.Metrics.PromptSizeBytes.WithLabelValues(provider, modelName).Observe(float64(EstimatePromptSize(prepared))) opts.Metrics.StepsTotal.WithLabelValues(provider, modelName).Inc() - call := fantasy.Call{ - Prompt: prepared, - Tools: buildToolDefinitions(opts.Tools, opts.ActiveTools, opts.ProviderTools), - MaxOutputTokens: opts.ModelConfig.MaxOutputTokens, - Temperature: opts.ModelConfig.Temperature, - TopP: opts.ModelConfig.TopP, - TopK: opts.ModelConfig.TopK, - PresencePenalty: opts.ModelConfig.PresencePenalty, - FrequencyPenalty: opts.ModelConfig.FrequencyPenalty, - ProviderOptions: opts.ProviderOptions, - } + call := opts.CallTemplate + call.Prompt = prepared + call.Tools = buildToolDefinitions(opts.Tools, opts.ActiveTools, opts.ProviderTools) stepStart := opts.Clock.Now() if opts.OnModelStreamStart != nil { diff --git a/coderd/x/chatd/chatloop/compaction.go b/coderd/x/chatd/chatloop/compaction.go index 320edca3d92..155e1acc894 100644 --- a/coderd/x/chatd/chatloop/compaction.go +++ b/coderd/x/chatd/chatloop/compaction.go @@ -94,12 +94,12 @@ type CompactionOptions struct { ChatID uuid.UUID HistoryTipMessageID int64 - // Summary model identity and call options; see + // Summary model identity and call envelope; see // GenerateCompactionOptions. ResolvedProvider string ResolvedModel string ModelConfigID uuid.UUID - ProviderOptions fantasy.ProviderOptions + SummaryCall fantasy.Call // Force skips the threshold gate (including the threshold=100 // disable and the zero-usage early return). Set for manual, @@ -236,7 +236,7 @@ func normalizedCompactionGenerateConfig(opts GenerateCompactionOptions) (Compact ResolvedProvider: opts.ResolvedProvider, ResolvedModel: opts.ResolvedModel, ModelConfigID: opts.ModelConfigID, - ProviderOptions: opts.ProviderOptions, + SummaryCall: opts.SummaryCall, Force: opts.Force, Source: opts.Source, ToolCallID: opts.ToolCallID, @@ -440,7 +440,6 @@ func generateCompactionSummary( Role: fantasy.MessageRoleUser, Content: summaryParts, }) - toolChoice := fantasy.ToolChoiceNone summaryCtx, finishDebugRun := startCompactionDebugRun(ctx, options) defer func() { @@ -458,11 +457,9 @@ func generateCompactionSummary( finishDebugRun(err) }() - response, err := model.Generate(summaryCtx, fantasy.Call{ - Prompt: summaryPrompt, - ToolChoice: &toolChoice, - ProviderOptions: options.ProviderOptions, - }) + call := options.SummaryCall + call.Prompt = summaryPrompt + response, err := model.Generate(summaryCtx, call) if err != nil { return "", xerrors.Errorf("generate summary text: %w", err) } diff --git a/coderd/x/chatd/generation.go b/coderd/x/chatd/generation.go index 377322fa97a..5a0fc192383 100644 --- a/coderd/x/chatd/generation.go +++ b/coderd/x/chatd/generation.go @@ -52,9 +52,10 @@ type generationPrepared struct { // user-facing errors. See chatloop.GenerateAssistantOptions.ErrorProvider. ResolvedProvider string - ModelConfigID uuid.UUID - ModelConfig codersdk.ChatModelCallConfig - ProviderOptions fantasy.ProviderOptions + ModelConfigID uuid.UUID + // CallTemplate is the resolver-built assistant call envelope; see + // chatloop.GenerateAssistantOptions.CallTemplate. + CallTemplate fantasy.Call ContextLimitFallback int64 DynamicToolNames map[string]bool @@ -731,8 +732,7 @@ func (s *taskStarter) generateAssistant( ActiveTools: prepared.ActiveTools, ProviderTools: prepared.ProviderTools, ContextLimitFallback: prepared.ContextLimitFallback, - ModelConfig: prepared.ModelConfig, - ProviderOptions: prepared.ProviderOptions, + CallTemplate: prepared.CallTemplate, PublishMessagePart: attempt.publish, OnModelStreamStart: attempt.startModelInvocation, Logger: s.opts.Logger, @@ -934,7 +934,7 @@ func (s *taskStarter) generateCompaction( compactionOpts.ResolvedProvider = overrideModel.resolvedProvider compactionOpts.ResolvedModel = overrideModel.resolvedModel compactionOpts.ModelConfigID = overrideModel.dbConfig.ID - compactionOpts.ProviderOptions = overrideModel.providerOptions + compactionOpts.SummaryCall = overrideModel.newCall(compactionSummaryOverrides(false)) compactionOpts.Messages = sanitizeCompactionPrompt( ctx, logger, diff --git a/coderd/x/chatd/generation_preparer.go b/coderd/x/chatd/generation_preparer.go index af88ccbe093..7dd212a7487 100644 --- a/coderd/x/chatd/generation_preparer.go +++ b/coderd/x/chatd/generation_preparer.go @@ -634,6 +634,7 @@ func (server *Server) prepareGeneration( ResolvedModel: resolved.resolvedModel, ModelConfigID: modelConfig.ID, StepUsage: compactionStepUsage, + SummaryCall: resolved.newCall(compactionSummaryOverrides(true)), } // workspaceCtx.currentChatSnapshot may carry a freshly persisted @@ -657,8 +658,7 @@ func (server *Server) prepareGeneration( ModelBuildOptions: modelOpts, ResolvedProvider: resolved.resolvedProvider, ModelConfigID: modelConfig.ID, - ModelConfig: callConfig, - ProviderOptions: resolved.providerOptions, + CallTemplate: resolved.newCall(callOverrides{}), ContextLimitFallback: modelConfig.ContextLimit, DynamicToolNames: dynamicToolNames, StopAfterTools: stopAfterBehaviorTools(currentPlanMode, chat.Mode, chat.ParentChatID), diff --git a/coderd/x/chatd/generation_preparer_internal_test.go b/coderd/x/chatd/generation_preparer_internal_test.go index 1574403def4..503329d53d5 100644 --- a/coderd/x/chatd/generation_preparer_internal_test.go +++ b/coderd/x/chatd/generation_preparer_internal_test.go @@ -155,8 +155,8 @@ func TestPrepareGenerationClampsRequestedReasoningEffortToMax(t *testing.T) { require.NoError(t, err) t.Cleanup(prepared.Cleanup) - providerOptions, ok := prepared.ProviderOptions[fantasyopenai.Name].(*fantasyopenai.ResponsesProviderOptions) - require.True(t, ok, "%T", prepared.ProviderOptions[fantasyopenai.Name]) + providerOptions, ok := prepared.CallTemplate.ProviderOptions[fantasyopenai.Name].(*fantasyopenai.ResponsesProviderOptions) + require.True(t, ok, "%T", prepared.CallTemplate.ProviderOptions[fantasyopenai.Name]) require.NotNil(t, providerOptions.ReasoningEffort) require.Equal(t, fantasyopenai.ReasoningEffortMedium, *providerOptions.ReasoningEffort) } @@ -248,8 +248,8 @@ func TestPrepareGenerationComputerUseIgnoresChatTransportOverride(t *testing.T) // The computer-use model is Responses-selected by the SDK and its client // ignores the config's forced Chat Completions, so the options must be the // Responses type or the SDK discards them. - _, ok := prepared.ProviderOptions[fantasyopenai.Name].(*fantasyopenai.ResponsesProviderOptions) - require.True(t, ok, "%T", prepared.ProviderOptions[fantasyopenai.Name]) + _, ok := prepared.CallTemplate.ProviderOptions[fantasyopenai.Name].(*fantasyopenai.ResponsesProviderOptions) + require.True(t, ok, "%T", prepared.CallTemplate.ProviderOptions[fantasyopenai.Name]) // File classification must also key on the substituted model: the // Responses transport drops native text file parts, so the attachment diff --git a/coderd/x/chatd/modelcall.go b/coderd/x/chatd/modelcall.go index 131f9c39497..b5a0de91a00 100644 --- a/coderd/x/chatd/modelcall.go +++ b/coderd/x/chatd/modelcall.go @@ -441,6 +441,53 @@ func (p *Server) resolveModelCall(ctx context.Context, spec modelCallSpec) (reso return out, nil } +// callOverrides carries the few envelope deviations flows need beyond the +// resolved call config. +type callOverrides struct { + // toolChoice forces the provider tool-choice mode. + toolChoice *fantasy.ToolChoice + // bare drops the sampling and token fields. Compaction summary calls + // historically send only prompt, tool choice, and provider options. + bare bool + // omitProviderOptions drops the resolved provider options. The + // chat-model compaction summary historically sends none. + omitProviderOptions bool +} + +// compactionSummaryOverrides is the envelope both compaction summary paths +// share: a bare call that forbids tool use. +func compactionSummaryOverrides(omitProviderOptions bool) callOverrides { + toolChoiceNone := fantasy.ToolChoiceNone + return callOverrides{ + bare: true, + toolChoice: &toolChoiceNone, + omitProviderOptions: omitProviderOptions, + } +} + +// newCall builds the fantasy.Call template for one model call. Prompt and +// tools stay caller-owned: downstream packages copy the template and attach +// them before sending (see chatloop.GenerateAssistantOptions.CallTemplate). +func (r resolvedModelCall) newCall(o callOverrides) fantasy.Call { + call := fantasy.Call{ + ToolChoice: o.toolChoice, + ProviderOptions: r.providerOptions, + } + if o.omitProviderOptions { + call.ProviderOptions = nil + } + if o.bare { + return call + } + call.MaxOutputTokens = r.callConfig.MaxOutputTokens + call.Temperature = r.callConfig.Temperature + call.TopP = r.callConfig.TopP + call.TopK = r.callConfig.TopK + call.PresencePenalty = r.callConfig.PresencePenalty + call.FrequencyPenalty = r.callConfig.FrequencyPenalty + return call +} + // deriveProviderOptions converts a call config into per-call provider // options for this resolved model. resolveModelCall derives from the spec's // parsed config; callers that mutate the call config after resolution diff --git a/coderd/x/chatd/modelcall_shape_internal_test.go b/coderd/x/chatd/modelcall_shape_internal_test.go index 6bd2d768e14..21326380b3b 100644 --- a/coderd/x/chatd/modelcall_shape_internal_test.go +++ b/coderd/x/chatd/modelcall_shape_internal_test.go @@ -111,18 +111,20 @@ func TestModelCallShapeStandardTurn(t *testing.T) { require.NoError(t, err) t.Cleanup(prepared.Cleanup) - providerOptions, ok := prepared.ProviderOptions[fantasyopenai.Name].(*fantasyopenai.ResponsesProviderOptions) - require.True(t, ok, "%T", prepared.ProviderOptions[fantasyopenai.Name]) + providerOptions, ok := prepared.CallTemplate.ProviderOptions[fantasyopenai.Name].(*fantasyopenai.ResponsesProviderOptions) + require.True(t, ok, "%T", prepared.CallTemplate.ProviderOptions[fantasyopenai.Name]) require.NotNil(t, providerOptions.User) require.Equal(t, "turn-options-sentinel", *providerOptions.User) - require.NotNil(t, prepared.ModelConfig.MaxOutputTokens) - require.Equal(t, int64(32_000), *prepared.ModelConfig.MaxOutputTokens) + require.NotNil(t, prepared.CallTemplate.MaxOutputTokens) + require.Equal(t, int64(32_000), *prepared.CallTemplate.MaxOutputTokens) // The chat-model compaction summary call carries no provider options - // even when the model config has them. + // even when the model config has them, and it forbids tool use. require.NotNil(t, prepared.Compaction) - require.Nil(t, prepared.Compaction.Options.ProviderOptions) + require.Nil(t, prepared.Compaction.Options.SummaryCall.ProviderOptions) + require.NotNil(t, prepared.Compaction.Options.SummaryCall.ToolChoice) + require.Equal(t, fantasy.ToolChoiceNone, *prepared.Compaction.Options.SummaryCall.ToolChoice) } func TestModelCallShapeManualTitleCarriesProviderOptions(t *testing.T) { From 4d370fba23afdc796647773898e298091e43f305 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Tue, 11 Aug 2026 20:01:59 +0000 Subject: [PATCH 06/13] docs(coderd/x/chatd): document the model-call resolver in ARCHITECTURE.md --- coderd/x/chatd/ARCHITECTURE.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/coderd/x/chatd/ARCHITECTURE.md b/coderd/x/chatd/ARCHITECTURE.md index bef7b57116b..7d5554519d6 100644 --- a/coderd/x/chatd/ARCHITECTURE.md +++ b/coderd/x/chatd/ARCHITECTURE.md @@ -854,6 +854,14 @@ The generation goroutine supports: - turn limit after a user message (the LLM shouldn't be able to spin forever in loop) - and other things +##### Model call resolution + +Every LLM client chatd builds comes out of a single pipeline, `Server.resolveModelCall` in `modelcall.go`. A caller describes the call with a `modelCallSpec`, built by a purpose-specific constructor such as `standardTurnSpec`, `titleChatSpec`, `compactionOverrideSpec`, or `computerUseSpec`, and receives a `resolvedModelCall`: a ready client plus the parsed call config, derived provider options, resolved provider/model identity, and route. The pipeline owns config selection (the chat's config, an explicitly selected row, or a fixed provider/model pair), `chat_model_configs.options` parsing, AI Gateway route resolution, client construction, the debug-recording wrap, and provider-option derivation. + +Callers keep only flow-specific policy: which config row to prefer, whether a resolution failure is a hard error or falls back to the chat model, plus prompts, tools, schemas, and timeouts. Per-flow envelope differences are declared on the spec rather than re-implemented at call sites; for example, summary and turn-status-label specs omit provider options and the standard turn defaults `MaxOutputTokens`. + +Call envelopes are centralized the same way: `resolvedModelCall.newCall` and `newObjectCall` are the only production constructors of `fantasy.Call` and `fantasy.ObjectCall`. Flows that hand generation to another package pass a prebuilt template through its options (`chatloop.GenerateAssistantOptions.CallTemplate`, the compaction `SummaryCall`, `chatadvisor.RuntimeConfig.CallTemplate`); the downstream package copies the template and attaches the prompt and tools it owns. + ##### Reasoning effort Model configs may carry a `reasoning_effort` config (`{default, max}`) inside `chat_model_configs.options`. Users select a per-turn effort when sending or editing a message; the value is stored on `chat_messages.reasoning_effort` and on `chat_queued_messages.reasoning_effort` for queued messages. Queued messages carry the value through promotion, and `chats.last_reasoning_effort` tracks the most recent message that set one, mirroring `last_model_config_id`. @@ -876,7 +884,7 @@ Request preparation reads the transport from the model instead of recomputing it The first two happen together in `chatprovider.ProviderOptionsForCall`, the only entry point in `chatprovider` that builds provider options for a call; it delegates transport-aware OpenAI conversion to `chatopenai.ProviderOptionsFromChatConfig`. Config conversion and effort injection cannot pick different option types because one function owns both. -Paths that build their own clients get a `Model` from the same constructor, including the compaction override, quick generation (used by turn status labels and debug models), and the advisor runtime. Within quick generation, only title generation converts the model config through `ProviderOptionsForCall`; the turn status label and chat summary paths deliberately send no provider options, because they are short structured calls that set their own output bounds. Debug recording replaces the wrapped client and preserves the resolved transport. Computer-use turns substitute a hardcoded default model that has no config of its own; it carries its own transport, so the chat model's `openai_config` does not follow it. +Paths that build their own clients get a `Model` from the same pipeline (`resolveModelCall`, see [Model call resolution](#model-call-resolution)), including the compaction override, quick generation (used by turn status labels and debug models), and the advisor runtime. Within quick generation, only title generation converts the model config through `ProviderOptionsForCall`; the turn status label and chat summary paths deliberately send no provider options, because they are short structured calls that set their own output bounds. Debug recording replaces the wrapped client and preserves the resolved transport. Computer-use turns substitute a hardcoded default model that has no config of its own; it carries its own transport, so the chat model's `openai_config` does not follow it. Azure is deliberately exempt: its provider always enables the Responses API for known models and exposes no equivalent per-model hook, so the transport keeps following the known-model list for Azure. Ignoring the override there is what keeps the decisions above in agreement with the Azure client. The exemption is narrower than it appears, because chatd never builds an azure-typed provider as a fantasy azure client: `fantasyConfigForAIBridge` folds every provider type other than anthropic, bedrock, and openai into openai-compat, which always speaks Chat Completions. From 05542e560340eb8ada08ff137f4eef8742e397f0 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Tue, 11 Aug 2026 20:30:52 +0000 Subject: [PATCH 07/13] refactor(coderd/x/chatd): apply cleanup-gate audit fixes Comment audit: delete restating comments, shorten verbose ones, and fix the advisor fallback description. Simplify findings: parse model config options once in resolveModelCall and pass the typed CallConfig to newModel, and stop grafting the chat config onto the computer-use resolution. Deslop findings: rename runChatResult.StatusLabel to StatusLabelCall and lock the resolver's provider-option policy with TestModelCallShapeProviderOptionPolicy instead of a misleading fixture. --- coderd/x/chatd/ARCHITECTURE.md | 4 +- coderd/x/chatd/advisor_internal_test.go | 3 - coderd/x/chatd/chatadvisor/runtime.go | 4 +- coderd/x/chatd/chatd.go | 14 +- coderd/x/chatd/chatloop/chatloop.go | 11 +- coderd/x/chatd/chatloop/compaction.go | 2 - coderd/x/chatd/compaction_override.go | 6 +- .../compaction_override_internal_test.go | 2 - coderd/x/chatd/generation.go | 8 +- coderd/x/chatd/generation_preparer.go | 13 +- .../generation_preparer_internal_test.go | 14 +- coderd/x/chatd/model_routing.go | 7 +- coderd/x/chatd/model_routing_aibridge.go | 8 +- coderd/x/chatd/model_routing_internal_test.go | 13 +- coderd/x/chatd/modelcall.go | 187 +++++++----------- .../x/chatd/modelcall_shape_internal_test.go | 57 ++++-- coderd/x/chatd/quickgen.go | 15 +- coderd/x/chatd/quickgen_internal_test.go | 6 +- 18 files changed, 157 insertions(+), 217 deletions(-) diff --git a/coderd/x/chatd/ARCHITECTURE.md b/coderd/x/chatd/ARCHITECTURE.md index 7d5554519d6..0a0b3323bdc 100644 --- a/coderd/x/chatd/ARCHITECTURE.md +++ b/coderd/x/chatd/ARCHITECTURE.md @@ -856,9 +856,9 @@ The generation goroutine supports: ##### Model call resolution -Every LLM client chatd builds comes out of a single pipeline, `Server.resolveModelCall` in `modelcall.go`. A caller describes the call with a `modelCallSpec`, built by a purpose-specific constructor such as `standardTurnSpec`, `titleChatSpec`, `compactionOverrideSpec`, or `computerUseSpec`, and receives a `resolvedModelCall`: a ready client plus the parsed call config, derived provider options, resolved provider/model identity, and route. The pipeline owns config selection (the chat's config, an explicitly selected row, or a fixed provider/model pair), `chat_model_configs.options` parsing, AI Gateway route resolution, client construction, the debug-recording wrap, and provider-option derivation. +Every LLM client chatd builds comes out of a single pipeline, `Server.resolveModelCall` in `modelcall.go`. A caller describes the call with a `modelCallSpec`, built by a purpose-specific constructor such as `standardTurnSpec`, `titleChatSpec`, `compactionOverrideSpec`, or `computerUseSpec`, and receives a `resolvedModelCall`: a ready client plus the parsed call config, provider options (derived when the spec requests them), resolved provider/model identity, and route. The pipeline owns config selection (the chat's config, an explicitly selected row, or a fixed provider/model pair), `chat_model_configs.options` parsing, AI Gateway route resolution, client construction, the debug-recording wrap, and provider-option derivation. -Callers keep only flow-specific policy: which config row to prefer, whether a resolution failure is a hard error or falls back to the chat model, plus prompts, tools, schemas, and timeouts. Per-flow envelope differences are declared on the spec rather than re-implemented at call sites; for example, summary and turn-status-label specs omit provider options and the standard turn defaults `MaxOutputTokens`. +Callers keep only flow-specific policy: which config row to prefer, whether a resolution failure is a hard error or falls back to the chat model, plus prompts, tools, schemas, and timeouts. Per-flow envelope differences are declared on the spec rather than re-implemented at call sites; for example, summary and turn-status-label specs omit provider options, the standard turn defaults `MaxOutputTokens`, and the advisor re-derives its options after pinning its reasoning effort and output cap into the call config. Call envelopes are centralized the same way: `resolvedModelCall.newCall` and `newObjectCall` are the only production constructors of `fantasy.Call` and `fantasy.ObjectCall`. Flows that hand generation to another package pass a prebuilt template through its options (`chatloop.GenerateAssistantOptions.CallTemplate`, the compaction `SummaryCall`, `chatadvisor.RuntimeConfig.CallTemplate`); the downstream package copies the template and attaches the prompt and tools it owns. diff --git a/coderd/x/chatd/advisor_internal_test.go b/coderd/x/chatd/advisor_internal_test.go index bf58530d77b..cdbb5cfbf12 100644 --- a/coderd/x/chatd/advisor_internal_test.go +++ b/coderd/x/chatd/advisor_internal_test.go @@ -266,9 +266,6 @@ func TestResolveAdvisorModelOverride(t *testing.T) { require.Equal(t, fallbackCallConfig, gotCfg) }) - // Corrupt options JSON on a provider-linked config must still fall - // back softly, unlike route or client failures which hard-fail for - // linked providers. Guards the modelCallConfigParseError distinction. t.Run("InvalidOptionsJSONWithLinkedProviderReturnsFallback", func(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitShort) diff --git a/coderd/x/chatd/chatadvisor/runtime.go b/coderd/x/chatd/chatadvisor/runtime.go index 092ffbed1c9..a15c0eaa92c 100644 --- a/coderd/x/chatd/chatadvisor/runtime.go +++ b/coderd/x/chatd/chatadvisor/runtime.go @@ -11,9 +11,7 @@ import ( // RuntimeConfig configures a single advisor runtime instance. type RuntimeConfig struct { Model fantasy.LanguageModel - // CallTemplate is the prebuilt advisor call envelope. Each advisor run - // copies it, clones its provider options, and attaches the nested - // prompt. + // CallTemplate's provider options are cloned for each nested call. CallTemplate fantasy.Call MaxUsesPerRun int MaxOutputTokens int64 diff --git a/coderd/x/chatd/chatd.go b/coderd/x/chatd/chatd.go index e56a8d79360..03916438f51 100644 --- a/coderd/x/chatd/chatd.go +++ b/coderd/x/chatd/chatd.go @@ -280,9 +280,8 @@ func (p *Server) resolveAdvisorModelOverride( resolved, err := p.resolveModelCall(ctx, advisorOverrideSpec(chat, overrideConfig, modelOpts)) if err != nil { - // Corrupt options JSON always falls back so a bad admin edit cannot - // break every turn; route and client failures fall back only when - // the config has no linked provider. + // Malformed options always fall back; route and client errors are + // hard failures only when the config has a linked provider. var parseErr modelCallConfigParseError if overrideConfig.AIProviderID.Valid && !xerrors.As(err, &parseErr) { return resolvedModelCall{}, xerrors.Errorf("resolve advisor override model: %w", err) @@ -3375,9 +3374,8 @@ func (p *Server) trackWorkspaceUsage( type runChatResult struct { FinalAssistantText string - // StatusLabel is the resolved chat-model call used to generate the - // end-of-turn status label; nil when model resolution failed. - StatusLabel *resolvedModelCall + // StatusLabelCall is nil when status-label model resolution failed. + StatusLabelCall *resolvedModelCall ModelBuildOptions modelBuildOptions TriggerMessageID int64 HistoryTipMessageID int64 @@ -4498,7 +4496,7 @@ func (p *Server) generateFinalTurnStatusLabel( } assistantText := strings.TrimSpace(runResult.FinalAssistantText) - if assistantText == "" || runResult.StatusLabel == nil { + if assistantText == "" || runResult.StatusLabelCall == nil { return fallbackTurnStatusLabel(status) } @@ -4507,7 +4505,7 @@ func (p *Server) generateFinalTurnStatusLabel( chat, status, assistantText, - *runResult.StatusLabel, + *runResult.StatusLabelCall, runResult.ModelBuildOptions, logger, p.existingDebugService(), diff --git a/coderd/x/chatd/chatloop/chatloop.go b/coderd/x/chatd/chatloop/chatloop.go index 20f244a9363..13a154b6940 100644 --- a/coderd/x/chatd/chatloop/chatloop.go +++ b/coderd/x/chatd/chatloop/chatloop.go @@ -215,9 +215,8 @@ type GenerateAssistantOptions struct { Clock quartz.Clock ContextLimitFallback int64 - // CallTemplate is the prebuilt call envelope (sampling fields, token - // cap, provider options). GenerateAssistant copies it and attaches the - // prepared prompt and tool definitions. + // CallTemplate is copied before GenerateAssistant attaches the prompt and + // tools. CallTemplate fantasy.Call PublishMessagePart func(codersdk.ChatMessageRole, codersdk.ChatMessagePart) @@ -307,10 +306,8 @@ type GenerateCompactionOptions struct { ResolvedModel string ModelConfigID uuid.UUID - // SummaryCall is the prebuilt summary-call envelope, carrying the - // tool-choice mode and summary-model provider options such as an - // override's reasoning effort. The summary prompt is attached before - // sending. + // SummaryCall is copied before GenerateCompaction attaches the summary + // prompt. SummaryCall fantasy.Call PublishMessagePart func(codersdk.ChatMessageRole, codersdk.ChatMessagePart) diff --git a/coderd/x/chatd/chatloop/compaction.go b/coderd/x/chatd/chatloop/compaction.go index 155e1acc894..1b8edb6106b 100644 --- a/coderd/x/chatd/chatloop/compaction.go +++ b/coderd/x/chatd/chatloop/compaction.go @@ -94,8 +94,6 @@ type CompactionOptions struct { ChatID uuid.UUID HistoryTipMessageID int64 - // Summary model identity and call envelope; see - // GenerateCompactionOptions. ResolvedProvider string ResolvedModel string ModelConfigID uuid.UUID diff --git a/coderd/x/chatd/compaction_override.go b/coderd/x/chatd/compaction_override.go index fb8941b7b7f..2db780a947d 100644 --- a/coderd/x/chatd/compaction_override.go +++ b/coderd/x/chatd/compaction_override.go @@ -34,11 +34,7 @@ func readCompactionModelOverride( // the model client so metrics recorded before the client exists // (still-over-limit) attribute to the same model as the compact action's. type resolvedCompactionOverride struct { - Config database.ChatModelConfig - // ResolvedProvider and ResolvedModel match the built client's - // identity: ResolveModelWithProviderHint normalizes its hint, so the - // normalized provider name here and the route hint resolveModelCall - // uses at build time yield the same result. + Config database.ChatModelConfig ResolvedProvider string ResolvedModel string } diff --git a/coderd/x/chatd/compaction_override_internal_test.go b/coderd/x/chatd/compaction_override_internal_test.go index 2bcaa1a599a..bc810bd2817 100644 --- a/coderd/x/chatd/compaction_override_internal_test.go +++ b/coderd/x/chatd/compaction_override_internal_test.go @@ -183,8 +183,6 @@ func TestCompactionOverride_SetUsable(t *testing.T) { // still-over-limit metrics land on the same series. require.Equal(t, override.resolvedProvider, resolved.ResolvedProvider) require.Equal(t, override.resolvedModel, resolved.ResolvedModel) - // The summary call derives provider options from the override config, - // including the admin-resolved reasoning effort. switch opts := override.providerOptions[fantasyopenai.Name].(type) { case *fantasyopenai.ResponsesProviderOptions: require.Equal(t, fantasyopenai.ReasoningEffort(effort), *opts.ReasoningEffort) diff --git a/coderd/x/chatd/generation.go b/coderd/x/chatd/generation.go index 5a0fc192383..fb60d5d593d 100644 --- a/coderd/x/chatd/generation.go +++ b/coderd/x/chatd/generation.go @@ -52,9 +52,7 @@ type generationPrepared struct { // user-facing errors. See chatloop.GenerateAssistantOptions.ErrorProvider. ResolvedProvider string - ModelConfigID uuid.UUID - // CallTemplate is the resolver-built assistant call envelope; see - // chatloop.GenerateAssistantOptions.CallTemplate. + ModelConfigID uuid.UUID CallTemplate fantasy.Call ContextLimitFallback int64 @@ -919,9 +917,7 @@ func (s *taskStarter) generateCompaction( compactionOpts := prepared.Compaction.Options metricProvider, metricModel := compactionMetricIdentity(prepared.Compaction) if override := prepared.Compaction.Override; override != nil { - // Errors are hard failures: a usable override that cannot be - // constructed must fail the generation visibly instead of silently - // compacting with the chat model. + // A usable override that fails to build is a hard generation failure. overrideModel, err := s.server.resolveModelCall(ctx, compactionOverrideSpec(prepared.Chat, override.Config, prepared.ModelBuildOptions)) if err != nil { return xerrors.Errorf("build compaction model override: %w", err) diff --git a/coderd/x/chatd/generation_preparer.go b/coderd/x/chatd/generation_preparer.go index 7dd212a7487..692c521ae4f 100644 --- a/coderd/x/chatd/generation_preparer.go +++ b/coderd/x/chatd/generation_preparer.go @@ -115,6 +115,9 @@ func (server *Server) prepareGeneration( if err != nil { return generationPrepared{}, err } + // The chat config keeps driving compaction, sanitization, and debug + // attribution even when computer use swaps the resolved call below. + modelConfig := resolved.dbConfig // Computer-use turns swap in a specialized model, so the substitution // must happen before anything model-sensitive runs: file-part @@ -143,14 +146,9 @@ func (server *Server) prepareGeneration( cuErr, ) } - // The chat model's config row keeps driving compaction, history - // sanitization, and debug attribution; only the client and its - // call identity are swapped. - cuResolved.dbConfig = resolved.dbConfig resolved = cuResolved } model := resolved.model - modelConfig := resolved.dbConfig callConfig := resolved.callConfig modelRoute := resolved.route @@ -786,8 +784,7 @@ func (server *Server) deriveFinalTurnRunResult( modelOpts := modelBuildOptions{ActiveAPIKeyID: apiKeyID} resolved, err := server.resolveModelCall(ctx, chatModelSpec(callPurposeStatusLabel, chat, modelOpts)) if err != nil { - // Return what we have; generateFinalTurnStatusLabel falls back to a - // generic label when StatusLabel is nil. + // Preserve the text and IDs for the generic-label fallback. logger.Warn(ctx, "derive final turn status label: resolve model", slog.Error(err)) return runChatResult{ FinalAssistantText: finalAssistantText, @@ -798,7 +795,7 @@ func (server *Server) deriveFinalTurnRunResult( return runChatResult{ FinalAssistantText: finalAssistantText, - StatusLabel: &resolved, + StatusLabelCall: &resolved, ModelBuildOptions: modelOpts, TriggerMessageID: triggerMessageID, HistoryTipMessageID: historyTipMessageID, diff --git a/coderd/x/chatd/generation_preparer_internal_test.go b/coderd/x/chatd/generation_preparer_internal_test.go index 503329d53d5..9ef8e8e8b73 100644 --- a/coderd/x/chatd/generation_preparer_internal_test.go +++ b/coderd/x/chatd/generation_preparer_internal_test.go @@ -442,11 +442,11 @@ func TestDeriveFinalTurnRunResult(t *testing.T) { require.Equal(t, "the answer is 42", result.FinalAssistantText) require.Equal(t, lastUserID, result.TriggerMessageID) require.Equal(t, tipID, result.HistoryTipMessageID) - require.NotNil(t, result.StatusLabel) - require.True(t, result.StatusLabel.model.Valid()) - require.Equal(t, "openai", result.StatusLabel.resolvedProvider) - require.Equal(t, "gpt-4o-mini", result.StatusLabel.resolvedModel) - require.JSONEq(t, `{"openai_config":{"use_responses_api":false}}`, string(result.StatusLabel.dbConfig.Options)) + require.NotNil(t, result.StatusLabelCall) + require.True(t, result.StatusLabelCall.model.Valid()) + require.Equal(t, "openai", result.StatusLabelCall.resolvedProvider) + require.Equal(t, "gpt-4o-mini", result.StatusLabelCall.resolvedModel) + require.JSONEq(t, `{"openai_config":{"use_responses_api":false}}`, string(result.StatusLabelCall.dbConfig.Options)) }) t.Run("NonWaitingReturnsEmpty", func(t *testing.T) { @@ -482,8 +482,6 @@ func TestDeriveFinalTurnRunResult(t *testing.T) { UserID: user.ID, OrganizationID: org.ID, }) - // A disabled AI provider makes model resolution fail, exercising the - // degraded path that still returns the re-derived text and IDs. provider := insertInternalAIProvider(t, db, database.AIProviderTypeOpenai, "provider-api-key", false) modelCfg := dbgen.ChatModelConfig(t, db, database.ChatModelConfig{ Model: "gpt-4o-mini", @@ -520,7 +518,7 @@ func TestDeriveFinalTurnRunResult(t *testing.T) { require.Equal(t, "the answer is 42", result.FinalAssistantText) require.NotZero(t, result.TriggerMessageID) require.NotZero(t, result.HistoryTipMessageID) - require.Nil(t, result.StatusLabel) + require.Nil(t, result.StatusLabelCall) }) } diff --git a/coderd/x/chatd/model_routing.go b/coderd/x/chatd/model_routing.go index a099b651572..c089570846b 100644 --- a/coderd/x/chatd/model_routing.go +++ b/coderd/x/chatd/model_routing.go @@ -2,7 +2,6 @@ package chatd import ( "context" - "encoding/json" "net/http" "github.com/google/uuid" @@ -18,9 +17,9 @@ type modelClientRequest struct { ModelName string UserAgent string ExtraHeaders map[string]string - // ConfigOptions holds the model config row's Options JSONB; empty for - // paths without a config row. - ConfigOptions json.RawMessage + // CallConfig is the parsed model config row's options; zero for paths + // without a config row. + CallConfig codersdk.ChatModelCallConfig } type modelBuildOptions struct { diff --git a/coderd/x/chatd/model_routing_aibridge.go b/coderd/x/chatd/model_routing_aibridge.go index 0311c597f20..924dfb87de2 100644 --- a/coderd/x/chatd/model_routing_aibridge.go +++ b/coderd/x/chatd/model_routing_aibridge.go @@ -166,11 +166,7 @@ func (p *Server) newModel( } config := fantasyConfigForAIBridge(route.Provider.Type) - callConfig, err := parseModelConfigOptions(req.ConfigOptions) - if err != nil { - return chatprovider.Model{}, err - } - extraHeaders := mergeConfigBetaHeaders(req.ExtraHeaders, config.ProviderHint, callConfig) + extraHeaders := mergeConfigBetaHeaders(req.ExtraHeaders, config.ProviderHint, req.CallConfig) return newLanguageModel( config.ProviderHint, req.ModelName, @@ -178,7 +174,7 @@ func (p *Server) newModel( req.UserAgent, extraHeaders, &http.Client{Transport: baseRT}, - callConfig.OpenAIConfig, + req.CallConfig.OpenAIConfig, ) } diff --git a/coderd/x/chatd/model_routing_internal_test.go b/coderd/x/chatd/model_routing_internal_test.go index f938b4dd8ae..a0b80011553 100644 --- a/coderd/x/chatd/model_routing_internal_test.go +++ b/coderd/x/chatd/model_routing_internal_test.go @@ -397,13 +397,10 @@ func TestAIGatewayModelAppliesResponsesAPIOverride(t *testing.T) { return &Server{aibridgeTransportFactory: aibridgeTestFactoryPointer(factory)} } - configOptions := func(t *testing.T, useResponsesAPI *bool) json.RawMessage { - t.Helper() - raw, err := json.Marshal(codersdk.ChatModelCallConfig{ + callConfig := func(useResponsesAPI *bool) codersdk.ChatModelCallConfig { + return codersdk.ChatModelCallConfig{ OpenAIConfig: &codersdk.ChatModelOpenAIConfig{UseResponsesAPI: useResponsesAPI}, - }) - require.NoError(t, err) - return raw + } } forceResponses := true @@ -428,7 +425,7 @@ func TestAIGatewayModelAppliesResponsesAPIOverride(t *testing.T) { server := newServer(t, paths) provider := aibridgeTestAIProvider(uuid.New(), "primary-openai", database.AIProviderTypeOpenai) req := aibridgeTestRequest(database.Chat{ID: uuid.New(), OwnerID: uuid.New()}, tt.model) - req.ConfigOptions = configOptions(t, tt.override) + req.CallConfig = callConfig(tt.override) model, err := server.newModel( t.Context(), @@ -715,7 +712,7 @@ func TestComputerUseModelCall_AIGatewayMissingAPIKeyID(t *testing.T) { modelProvider, modelName, ok := chattool.DefaultComputerUseModel(provider) require.True(t, ok) - spec := computerUseSpec(chat, modelProvider, modelName, codersdk.ChatModelCallConfig{}, modelBuildOptions{}) // no ActiveAPIKeyID + spec := computerUseSpec(chat, modelProvider, modelName, codersdk.ChatModelCallConfig{}, modelBuildOptions{}) route := aibridgeTestRoute(aibridgeTestAIProvider(providerID, "primary-openai", database.AIProviderTypeOpenai)) spec.routeOverride = &route resolved, err := server.resolveModelCall(t.Context(), spec) diff --git a/coderd/x/chatd/modelcall.go b/coderd/x/chatd/modelcall.go index b5a0de91a00..17f714c1f85 100644 --- a/coderd/x/chatd/modelcall.go +++ b/coderd/x/chatd/modelcall.go @@ -15,8 +15,8 @@ import ( "github.com/coder/coder/v2/codersdk" ) -// callPurpose labels the flow a model call serves. It only feeds logging and -// debug attribution; behavior is driven by the other modelCallSpec fields. +// callPurpose labels logs and debug attribution; it does not affect call +// behavior. type callPurpose string const ( @@ -30,8 +30,6 @@ const ( callPurposeDebugRebuild callPurpose = "debug_rebuild" ) -// defaultChatMaxOutputTokens caps standard-turn output when the model config -// leaves MaxOutputTokens unset. const defaultChatMaxOutputTokens = int64(32_000) type configSelectionMode int @@ -49,75 +47,54 @@ const ( ) type configSelection struct { - mode configSelectionMode - config database.ChatModelConfig - // providerType routes configFixedModel calls when no route override is - // supplied. - providerType string - modelName string - // configOptions is the raw options JSON applied to client construction - // for configFixedModel (beta headers, OpenAI transport override). + mode configSelectionMode + config database.ChatModelConfig + providerType string + modelName string configOptions []byte - // callConfig supplies the per-call config for configFixedModel option - // derivation, since there is no config row to parse. - callConfig codersdk.ChatModelCallConfig + callConfig codersdk.ChatModelCallConfig } type debugPolicy int const ( - // debugPolicyOff builds a plain client with no debug recording. debugPolicyOff debugPolicy = iota - // debugPolicyAware records HTTP traffic and wraps the model when the - // chat debug service enables this chat. + // debugPolicyAware records only when chat debug is enabled. debugPolicyAware - // debugPolicyForced always records and wraps; used to rebuild a debug - // transport after the caller verified debug is enabled. + // debugPolicyForced records after the caller has enabled debugging. debugPolicyForced ) type providerOptionPolicy int const ( - // providerOptionsDerive converts the call config into per-call provider - // options. providerOptionsDerive providerOptionPolicy = iota // providerOptionsOmit skips derivation. Used by flows that historically // never sent provider options and by callers that derive separately. providerOptionsOmit ) -// modelCallSpec describes one LLM call to resolve: which config to use, how -// to route it, and which construction policies apply. Build specs via the -// purpose-specific constructors so per-flow policy stays declared in one -// place. +// modelCallSpec declares config, routing, and construction policy for one LLM +// call. Build it with a purpose-specific constructor. type modelCallSpec struct { - purpose callPurpose - chat database.Chat - config configSelection - requestedEffort *string - providerOptions providerOptionPolicy - debug debugPolicy - // debugSvc, debugWrapProvider, and debugWrapModel label forced debug - // recordings; callers keep their historical attribution labels. + purpose callPurpose + chat database.Chat + config configSelection + requestedEffort *string + providerOptions providerOptionPolicy + debug debugPolicy debugSvc *chatdebug.Service debugWrapProvider string debugWrapModel string - // routeOverride reuses a previously resolved route instead of resolving - // one (debug transport rebuilds). - routeOverride *aiGatewayModelRoute + routeOverride *aiGatewayModelRoute // chatdScopedRoute resolves the route with chatd scope. Deployment-wide // override models must route for user-owned chats regardless of the // caller's actor. - chatdScopedRoute bool - // defaultMaxOutputTokens applies the standard-turn output cap when the - // config leaves MaxOutputTokens unset. + chatdScopedRoute bool defaultMaxOutputTokens bool buildOptions modelBuildOptions } -// chatRequestedEffort is the user's per-turn reasoning effort choice, which -// the config's bounds clamp during option derivation. func chatRequestedEffort(chat database.Chat) *string { if !chat.LastReasoningEffort.Valid { return nil @@ -138,10 +115,8 @@ func standardTurnSpec(chat database.Chat, buildOpts modelBuildOptions) modelCall } } -// chatModelSpec resolves the chat's model without deriving provider options -// or applying the standard-turn token default. Summary and status-label -// calls historically send no provider options; that omission is preserved -// here as declared policy. +// chatModelSpec preserves summary and status-label behavior: no provider +// options and no standard-turn token default. func chatModelSpec(purpose callPurpose, chat database.Chat, buildOpts modelBuildOptions) modelCallSpec { return modelCallSpec{ purpose: purpose, @@ -153,10 +128,8 @@ func chatModelSpec(purpose callPurpose, chat database.Chat, buildOpts modelBuild } } -// titleChatSpec resolves the chat's own model as the title-generation -// fallback candidate. Title calls derive provider options without a -// requested effort: the user's per-turn effort choice applies to turns, not -// background title generation. +// Background title generation uses the config's default reasoning effort, not +// the user's per-turn choice. func titleChatSpec(chat database.Chat, buildOpts modelBuildOptions) modelCallSpec { return modelCallSpec{ purpose: callPurposeTitle, @@ -168,9 +141,8 @@ func titleChatSpec(chat database.Chat, buildOpts modelBuildOptions) modelCallSpe } } -// titleOverrideSpec builds the deployment-wide title override model from the -// caller-selected config row. The route resolves with chatd scope so the -// override works for chats whose owner cannot read the provider. +// titleOverrideSpec uses chatd scope so owners need not have provider read +// access. func titleOverrideSpec(chat database.Chat, config database.ChatModelConfig, buildOpts modelBuildOptions) modelCallSpec { return modelCallSpec{ purpose: callPurposeTitle, @@ -182,9 +154,8 @@ func titleOverrideSpec(chat database.Chat, config database.ChatModelConfig, buil } } -// manualTitleSpec builds a caller-selected manual-title model: the preferred -// small model or the chat's own config as fallback. Debug recording is -// handled by a separate rebuild, matching the historical construction. +// manualTitleSpec leaves debug instrumentation to a separate rebuild to +// preserve manual-title behavior. func manualTitleSpec(chat database.Chat, config database.ChatModelConfig, buildOpts modelBuildOptions) modelCallSpec { return modelCallSpec{ purpose: callPurposeTitle, @@ -195,10 +166,8 @@ func manualTitleSpec(chat database.Chat, config database.ChatModelConfig, buildO } } -// compactionOverrideSpec builds the deployment-wide compaction override -// model from the caller-selected config row, whose reasoning effort was -// already resolved at prepare time. The route resolves with chatd scope so -// the override works for chats whose owner cannot read the provider. +// compactionOverrideSpec receives a config with resolved reasoning effort and +// uses chatd scope so owners need not have provider read access. func compactionOverrideSpec(chat database.Chat, config database.ChatModelConfig, buildOpts modelBuildOptions) modelCallSpec { return modelCallSpec{ purpose: callPurposeCompaction, @@ -211,10 +180,8 @@ func compactionOverrideSpec(chat database.Chat, config database.ChatModelConfig, } } -// advisorOverrideSpec builds the advisor's override model from the config -// row the caller re-read from the database. Provider options are omitted -// because the advisor derives them after pinning its reasoning effort and -// output cap into the call config. +// advisorOverrideSpec omits provider options until the advisor pins its +// reasoning effort and output cap. func advisorOverrideSpec(chat database.Chat, config database.ChatModelConfig, buildOpts modelBuildOptions) modelCallSpec { return modelCallSpec{ purpose: callPurposeAdvisor, @@ -225,9 +192,8 @@ func advisorOverrideSpec(chat database.Chat, config database.ChatModelConfig, bu } } -// manualTitleDebugSpec rebuilds the manual-title client with HTTP recording -// after the caller verified debug is enabled and resolved the route itself -// (the route's provider type also labels the debug run record). +// manualTitleDebugSpec preserves the resolved route and caller-selected +// attribution labels while enabling HTTP recording. func manualTitleDebugSpec( chat database.Chat, config database.ChatModelConfig, @@ -250,10 +216,8 @@ func manualTitleDebugSpec( } } -// computerUseSpec swaps in the deployment's computer-use model. The client is -// built without config options because the fixed model has no config row; the -// chat model's call config still drives per-call provider options so admin -// tuning follows the turn. +// computerUseSpec uses the chat config only for per-call options because the +// fixed computer-use model has no config row. func computerUseSpec( chat database.Chat, modelProvider string, @@ -277,10 +241,8 @@ func computerUseSpec( } } -// modelCallConfigParseError marks malformed model-config options JSON. The -// advisor override falls back to the chat model on corrupt options while -// hard-failing on route and client errors, so it matches this type with -// xerrors.As. +// modelCallConfigParseError lets the advisor distinguish malformed options +// from route and client failures when deciding whether to fall back. type modelCallConfigParseError struct{ err error } func (e modelCallConfigParseError) Error() string { @@ -289,13 +251,10 @@ func (e modelCallConfigParseError) Error() string { func (e modelCallConfigParseError) Unwrap() error { return e.err } -// resolvedModelCall is the output of resolveModelCall: a ready client plus -// the metadata callers need for prompts, metrics, and debug attribution. type resolvedModelCall struct { - model chatprovider.Model - dbConfig database.ChatModelConfig - callConfig codersdk.ChatModelCallConfig - // providerOptions is nil when the spec's policy omits derivation. + model chatprovider.Model + dbConfig database.ChatModelConfig + callConfig codersdk.ChatModelCallConfig providerOptions fantasy.ProviderOptions resolvedProvider string resolvedModel string @@ -303,10 +262,8 @@ type resolvedModelCall struct { debugEnabled bool } -// resolveModelCall is the single pipeline from a call spec to a ready model -// client: config selection, call-config parse, route and identity resolution, -// client construction (including debug recording), and provider-option -// derivation. +// resolveModelCall is the single pipeline from a spec to a ready model +// client plus the call metadata flows need. func (p *Server) resolveModelCall(ctx context.Context, spec modelCallSpec) (resolvedModelCall, error) { out := resolvedModelCall{} @@ -333,15 +290,18 @@ func (p *Server) resolveModelCall(ctx context.Context, spec modelCallSpec) (reso configOptions = spec.config.configOptions } - switch spec.config.mode { - case configFixedModel: + // clientCallConfig always comes from configOptions: it drives client + // construction (beta headers, OpenAI transport override), while + // out.callConfig drives per-call option derivation and can differ for + // configFixedModel (computer use derives options from the chat model). + clientCallConfig, err := parseModelConfigOptions(configOptions) + if err != nil { + return resolvedModelCall{}, modelCallConfigParseError{err: err} + } + if spec.config.mode == configFixedModel { out.callConfig = spec.config.callConfig - default: - var err error - out.callConfig, err = parseModelConfigOptions(configOptions) - if err != nil { - return resolvedModelCall{}, modelCallConfigParseError{err: err} - } + } else { + out.callConfig = clientCallConfig } if spec.defaultMaxOutputTokens && out.callConfig.MaxOutputTokens == nil { out.callConfig.MaxOutputTokens = ptr.Ref(defaultChatMaxOutputTokens) @@ -366,7 +326,6 @@ func (p *Server) resolveModelCall(ctx context.Context, spec modelCallSpec) (reso } } - var err error out.resolvedProvider, out.resolvedModel, err = chatprovider.ResolveModelWithProviderHint( modelName, out.route.ModelProviderHint, @@ -390,10 +349,8 @@ func (p *Server) resolveModelCall(ctx context.Context, spec modelCallSpec) (reso clientModelName := modelName clientRoute := out.route if spec.debug == debugPolicyAware { - // Preserved from newDebugAwareModel: debug-aware flows build the - // client from the resolved identity while other flows pass the raw - // configured model name. The distinction only matters for malformed - // slash-namespaced model names, so it is kept rather than unified. + // Debug-aware calls preserve their historical use of the resolved identity; + // other flows pass the configured model name. clientRoute.ModelProviderHint = out.resolvedProvider clientModelName = out.resolvedModel } @@ -401,11 +358,11 @@ func (p *Server) resolveModelCall(ctx context.Context, spec modelCallSpec) (reso buildOpts := spec.buildOptions buildOpts.RecordHTTP = out.debugEnabled model, err := p.newModel(ctx, modelClientRequest{ - Chat: spec.chat, - ModelName: clientModelName, - UserAgent: chatprovider.UserAgent(), - ExtraHeaders: chatprovider.CoderHeaders(spec.chat), - ConfigOptions: configOptions, + Chat: spec.chat, + ModelName: clientModelName, + UserAgent: chatprovider.UserAgent(), + ExtraHeaders: chatprovider.CoderHeaders(spec.chat), + CallConfig: clientCallConfig, }, clientRoute, buildOpts) if err != nil { return resolvedModelCall{}, xerrors.Errorf("create model: %w", err) @@ -441,10 +398,7 @@ func (p *Server) resolveModelCall(ctx context.Context, spec modelCallSpec) (reso return out, nil } -// callOverrides carries the few envelope deviations flows need beyond the -// resolved call config. type callOverrides struct { - // toolChoice forces the provider tool-choice mode. toolChoice *fantasy.ToolChoice // bare drops the sampling and token fields. Compaction summary calls // historically send only prompt, tool choice, and provider options. @@ -454,8 +408,6 @@ type callOverrides struct { omitProviderOptions bool } -// compactionSummaryOverrides is the envelope both compaction summary paths -// share: a bare call that forbids tool use. func compactionSummaryOverrides(omitProviderOptions bool) callOverrides { toolChoiceNone := fantasy.ToolChoiceNone return callOverrides{ @@ -465,9 +417,8 @@ func compactionSummaryOverrides(omitProviderOptions bool) callOverrides { } } -// newCall builds the fantasy.Call template for one model call. Prompt and -// tools stay caller-owned: downstream packages copy the template and attach -// them before sending (see chatloop.GenerateAssistantOptions.CallTemplate). +// newCall builds a call template; downstream packages copy it and attach the +// prompt and tools they own. func (r resolvedModelCall) newCall(o callOverrides) fantasy.Call { call := fantasy.Call{ ToolChoice: o.toolChoice, @@ -488,25 +439,21 @@ func (r resolvedModelCall) newCall(o callOverrides) fantasy.Call { return call } -// deriveProviderOptions converts a call config into per-call provider -// options for this resolved model. resolveModelCall derives from the spec's -// parsed config; callers that mutate the call config after resolution -// (advisor) re-derive here. +// deriveProviderOptions is the only production ProviderOptionsForCall call +// site; callers that mutate the call config after resolution re-derive here. func (r resolvedModelCall) deriveProviderOptions(callConfig codersdk.ChatModelCallConfig, requestedEffort *string) fantasy.ProviderOptions { return chatprovider.ProviderOptionsForCall(r.model, callConfig, requestedEffort) } -// objectCallOverrides carries the caller-owned schema and token cap for a -// structured-output call. Quickgen flows use fixed caps instead of the model -// config's tuning. +// Quickgen flows use fixed output caps instead of the model config's tuning. type objectCallOverrides struct { schemaName string schemaDescription string maxOutputTokens int64 } -// newObjectCall builds the fantasy.ObjectCall envelope for one -// structured-output call. The caller attaches the prompt before sending. +// newObjectCall builds a structured-output call envelope; the caller attaches +// the prompt before sending. func (r resolvedModelCall) newObjectCall(o objectCallOverrides) fantasy.ObjectCall { return fantasy.ObjectCall{ SchemaName: o.schemaName, diff --git a/coderd/x/chatd/modelcall_shape_internal_test.go b/coderd/x/chatd/modelcall_shape_internal_test.go index 21326380b3b..1f9bde83360 100644 --- a/coderd/x/chatd/modelcall_shape_internal_test.go +++ b/coderd/x/chatd/modelcall_shape_internal_test.go @@ -32,11 +32,6 @@ import ( "github.com/coder/quartz" ) -// The tests in this file lock the outgoing request shape of each LLM call -// flow: which flows carry model-config provider options and which -// deliberately omit them, plus token defaults. They guard the model-call -// resolver refactor against silent behavior changes. - func modelCallSentinelOptions(t *testing.T, user string) json.RawMessage { t.Helper() raw, err := json.Marshal(codersdk.ChatModelCallConfig{ @@ -119,8 +114,6 @@ func TestModelCallShapeStandardTurn(t *testing.T) { require.NotNil(t, prepared.CallTemplate.MaxOutputTokens) require.Equal(t, int64(32_000), *prepared.CallTemplate.MaxOutputTokens) - // The chat-model compaction summary call carries no provider options - // even when the model config has them, and it forbids tool use. require.NotNil(t, prepared.Compaction) require.Nil(t, prepared.Compaction.Options.SummaryCall.ProviderOptions) require.NotNil(t, prepared.Compaction.Options.SummaryCall.ToolChoice) @@ -288,10 +281,55 @@ func TestModelCallShapeChatSummaryOmitsProviderOptions(t *testing.T) { require.Len(t, bodies, 1) var raw map[string]any require.NoError(t, json.Unmarshal(bodies[0], &raw)) - // The summary call omits model-config provider options entirely. require.NotContains(t, raw, "user") } +// TestModelCallShapeProviderOptionPolicy locks the resolver's +// provider-option policy handling and each flow's declared policy, so the +// summary and status-label omission cannot silently regress. +func TestModelCallShapeProviderOptionPolicy(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + ctrl := gomock.NewController(t) + db := dbmock.NewMockStore(ctrl) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + chat, _ := titleOverrideTestChatAndMessages(t) + providerID := uuid.New() + config := titleOverrideModelConfig("gpt-4o-mini", true) + config.AIProviderID = uuid.NullUUID{UUID: providerID, Valid: true} + config.Options = modelCallSentinelOptions(t, "policy-sentinel") + + db.EXPECT().GetAIProviderByID(gomock.Any(), providerID).Return(aibridgeTestAIProvider(providerID, "primary-openai", database.AIProviderTypeOpenai), nil).AnyTimes() + db.EXPECT().GetAIProviderKeysByProviderID(gomock.Any(), providerID).Return([]database.AIProviderKey{{ + ProviderID: providerID, + APIKey: "test-key", + }}, nil).AnyTimes() + + server := titleOverrideTestServer(db, logger) + + spec := modelCallSpec{ + purpose: callPurposeStatusLabel, + chat: chat, + config: configSelection{mode: configExplicit, config: config}, + providerOptions: providerOptionsOmit, + buildOptions: modelBuildOptions{ActiveAPIKeyID: uuid.NewString()}, + } + omitted, err := server.resolveModelCall(ctx, spec) + require.NoError(t, err) + require.Nil(t, omitted.providerOptions) + + spec.providerOptions = providerOptionsDerive + derived, err := server.resolveModelCall(ctx, spec) + require.NoError(t, err) + require.NotNil(t, derived.providerOptions) + + require.Equal(t, providerOptionsOmit, chatModelSpec(callPurposeSummary, chat, modelBuildOptions{}).providerOptions) + require.Equal(t, providerOptionsOmit, chatModelSpec(callPurposeStatusLabel, chat, modelBuildOptions{}).providerOptions) + require.Equal(t, providerOptionsDerive, standardTurnSpec(chat, modelBuildOptions{}).providerOptions) + require.Equal(t, providerOptionsDerive, titleChatSpec(chat, modelBuildOptions{}).providerOptions) +} + func TestModelCallShapeTurnStatusLabelEnvelope(t *testing.T) { t.Parallel() @@ -323,7 +361,6 @@ func TestModelCallShapeTurnStatusLabelEnvelope(t *testing.T) { "All tests pass now.", resolvedModelCall{ model: chatprovider.NewModel(model, nil), - dbConfig: database.ChatModelConfig{Options: modelCallSentinelOptions(t, "status-options-sentinel")}, resolvedProvider: fantasyopenai.Name, resolvedModel: "gpt-4o-mini", }, @@ -339,8 +376,6 @@ func TestModelCallShapeTurnStatusLabelEnvelope(t *testing.T) { defer callMu.Unlock() require.Len(t, captured, 1) call := captured[0] - // The status-label call omits model-config provider options even though - // the config JSON carries them. require.Nil(t, call.ProviderOptions) require.NotNil(t, call.MaxOutputTokens) require.Equal(t, int64(64), *call.MaxOutputTokens) diff --git a/coderd/x/chatd/quickgen.go b/coderd/x/chatd/quickgen.go index 2fb4092e300..c7e917ef90e 100644 --- a/coderd/x/chatd/quickgen.go +++ b/coderd/x/chatd/quickgen.go @@ -136,19 +136,16 @@ var preferredTitleModels = []struct { {fantasyvercel.Name, "anthropic/claude-haiku-4.5"}, } -// shortTextCandidate is one quickgen model candidate. provider and model -// label debug runs: title flows use the route's provider type and configured -// model name while the status-label flow uses the resolved identity. +// Debug attribution uses configured identities for title calls and the +// resolved identity for status-label calls. type shortTextCandidate struct { provider string model string resolved resolvedModelCall } -// quickgenDebugSpec rebuilds a quickgen candidate's client with HTTP -// recording after the caller verified debug is enabled. The client keeps the -// candidate's model name, config options, and route; the wrap labels keep -// the candidate's attribution. +// quickgenDebugSpec preserves the candidate's route, client options, and +// attribution labels while enabling HTTP recording. func quickgenDebugSpec( chat database.Chat, candidate shortTextCandidate, @@ -513,7 +510,6 @@ func (p *Server) prepareQuickgenDebugCandidate( return runCtx, debugModel, finishDebugRun } -// quickgenPrompt pairs a system prompt with one user message. func quickgenPrompt(systemPrompt, userInput string) fantasy.Prompt { return fantasy.Prompt{ { @@ -1282,8 +1278,7 @@ func turnStatusLabelObjectCall(resolved resolvedModelCall) fantasy.ObjectCall { }) } -// generateTurnStatusLabel produces a short turn status label using the -// resolved chat-model call. Returns "" on any failure. +// generateTurnStatusLabel returns an empty string if generation fails. func (p *Server) generateTurnStatusLabel( ctx context.Context, chat database.Chat, diff --git a/coderd/x/chatd/quickgen_internal_test.go b/coderd/x/chatd/quickgen_internal_test.go index 9305b91e64c..e5208928801 100644 --- a/coderd/x/chatd/quickgen_internal_test.go +++ b/coderd/x/chatd/quickgen_internal_test.go @@ -660,10 +660,8 @@ func TestMaybeGenerateChatTitleAppliesModelConfigReasoningEffort(t *testing.T) { messages, nil, resolvedModelCall{ - model: fallbackModel, - dbConfig: fallbackConfig, - // Mirrors titleChatSpec: derive with no requested effort so the - // config's default reasoning effort applies. + model: fallbackModel, + dbConfig: fallbackConfig, providerOptions: chatprovider.ProviderOptionsForCall(fallbackModel, callConfig, nil), }, modelBuildOptions{}, From 2abd7aa53fc9881f26c5f01401419214a711d76e Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Thu, 13 Aug 2026 09:58:02 +0000 Subject: [PATCH 08/13] refactor(coderd/x/chatd): collapse model-call construction abstractions Replace the providerOptionPolicy enum with an omitProviderOptions bool, replace callOverrides and compactionSummaryOverrides with newCall and newCompactionSummaryCall, pass newObjectCall arguments directly, and turn callPurpose into a plain log string. No behavior change. --- coderd/x/chatd/chatd.go | 4 +- coderd/x/chatd/generation.go | 2 +- coderd/x/chatd/generation_preparer.go | 10 +- coderd/x/chatd/modelcall.go | 194 +++++++----------- .../x/chatd/modelcall_shape_internal_test.go | 20 +- coderd/x/chatd/quickgen.go | 34 +-- 6 files changed, 103 insertions(+), 161 deletions(-) diff --git a/coderd/x/chatd/chatd.go b/coderd/x/chatd/chatd.go index 03916438f51..a4989a61918 100644 --- a/coderd/x/chatd/chatd.go +++ b/coderd/x/chatd/chatd.go @@ -360,7 +360,7 @@ func (p *Server) newAdvisorRuntime( rt, err := chatadvisor.NewRuntime(chatadvisor.RuntimeConfig{ Model: advisor.model.LanguageModel(), - CallTemplate: advisor.newCall(callOverrides{}), + CallTemplate: advisor.newCall(), MaxUsesPerRun: maxUsesPerRun, MaxOutputTokens: maxOutputTokens, }) @@ -4751,7 +4751,7 @@ func (p *Server) resolveChatSummaryModel( chat database.Chat, modelOpts modelBuildOptions, ) (resolvedModelCall, bool) { - resolved, err := p.resolveModelCall(ctx, chatModelSpec(callPurposeSummary, chat, modelOpts)) + resolved, err := p.resolveModelCall(ctx, chatModelSpec("chat_summary", chat, modelOpts)) if err != nil { logger.Debug(ctx, "failed to resolve chat model for summary", slog.F("chat_id", chat.ID), slog.Error(err)) diff --git a/coderd/x/chatd/generation.go b/coderd/x/chatd/generation.go index fb60d5d593d..66a551672a2 100644 --- a/coderd/x/chatd/generation.go +++ b/coderd/x/chatd/generation.go @@ -930,7 +930,7 @@ func (s *taskStarter) generateCompaction( compactionOpts.ResolvedProvider = overrideModel.resolvedProvider compactionOpts.ResolvedModel = overrideModel.resolvedModel compactionOpts.ModelConfigID = overrideModel.dbConfig.ID - compactionOpts.SummaryCall = overrideModel.newCall(compactionSummaryOverrides(false)) + compactionOpts.SummaryCall = overrideModel.newCompactionSummaryCall() compactionOpts.Messages = sanitizeCompactionPrompt( ctx, logger, diff --git a/coderd/x/chatd/generation_preparer.go b/coderd/x/chatd/generation_preparer.go index 692c521ae4f..a371dba4178 100644 --- a/coderd/x/chatd/generation_preparer.go +++ b/coderd/x/chatd/generation_preparer.go @@ -615,6 +615,10 @@ func (server *Server) prepareGeneration( } compactionStepUsage := latestPromptUsage(promptRows) compactionNeeded := shouldCompactPromptUsage(compactionStepUsage, compactionContextLimit, effectiveThreshold) + // The chat-model compaction summary historically sends no provider + // options; the override-model summary in generateCompaction keeps them. + summaryCall := resolved.newCompactionSummaryCall() + summaryCall.ProviderOptions = nil // The options carry the chat model; generateCompaction swaps in the // override client when one is configured. compactionOptions := chatloop.GenerateCompactionOptions{ @@ -632,7 +636,7 @@ func (server *Server) prepareGeneration( ResolvedModel: resolved.resolvedModel, ModelConfigID: modelConfig.ID, StepUsage: compactionStepUsage, - SummaryCall: resolved.newCall(compactionSummaryOverrides(true)), + SummaryCall: summaryCall, } // workspaceCtx.currentChatSnapshot may carry a freshly persisted @@ -656,7 +660,7 @@ func (server *Server) prepareGeneration( ModelBuildOptions: modelOpts, ResolvedProvider: resolved.resolvedProvider, ModelConfigID: modelConfig.ID, - CallTemplate: resolved.newCall(callOverrides{}), + CallTemplate: resolved.newCall(), ContextLimitFallback: modelConfig.ContextLimit, DynamicToolNames: dynamicToolNames, StopAfterTools: stopAfterBehaviorTools(currentPlanMode, chat.Mode, chat.ParentChatID), @@ -782,7 +786,7 @@ func (server *Server) deriveFinalTurnRunResult( return runChatResult{FinalAssistantText: finalAssistantText, TriggerMessageID: triggerMessageID, HistoryTipMessageID: historyTipMessageID} } modelOpts := modelBuildOptions{ActiveAPIKeyID: apiKeyID} - resolved, err := server.resolveModelCall(ctx, chatModelSpec(callPurposeStatusLabel, chat, modelOpts)) + resolved, err := server.resolveModelCall(ctx, chatModelSpec("turn_status_label", chat, modelOpts)) if err != nil { // Preserve the text and IDs for the generic-label fallback. logger.Warn(ctx, "derive final turn status label: resolve model", slog.Error(err)) diff --git a/coderd/x/chatd/modelcall.go b/coderd/x/chatd/modelcall.go index 17f714c1f85..791199be468 100644 --- a/coderd/x/chatd/modelcall.go +++ b/coderd/x/chatd/modelcall.go @@ -15,21 +15,6 @@ import ( "github.com/coder/coder/v2/codersdk" ) -// callPurpose labels logs and debug attribution; it does not affect call -// behavior. -type callPurpose string - -const ( - callPurposeStandardTurn callPurpose = "standard_turn" - callPurposeComputerUse callPurpose = "computer_use" - callPurposeTitle callPurpose = "title" - callPurposeSummary callPurpose = "chat_summary" - callPurposeStatusLabel callPurpose = "turn_status_label" - callPurposeCompaction callPurpose = "compaction" - callPurposeAdvisor callPurpose = "advisor" - callPurposeDebugRebuild callPurpose = "debug_rebuild" -) - const defaultChatMaxOutputTokens = int64(32_000) type configSelectionMode int @@ -65,28 +50,22 @@ const ( debugPolicyForced ) -type providerOptionPolicy int - -const ( - providerOptionsDerive providerOptionPolicy = iota - // providerOptionsOmit skips derivation. Used by flows that historically - // never sent provider options and by callers that derive separately. - providerOptionsOmit -) - // modelCallSpec declares config, routing, and construction policy for one LLM // call. Build it with a purpose-specific constructor. type modelCallSpec struct { - purpose callPurpose - chat database.Chat - config configSelection - requestedEffort *string - providerOptions providerOptionPolicy - debug debugPolicy - debugSvc *chatdebug.Service - debugWrapProvider string - debugWrapModel string - routeOverride *aiGatewayModelRoute + // purpose labels resolver logs only; it does not affect call behavior. + purpose string + chat database.Chat + config configSelection + requestedEffort *string + // omitProviderOptions skips derivation. Used by flows that historically + // never sent provider options and by callers that derive separately. + omitProviderOptions bool + debug debugPolicy + debugSvc *chatdebug.Service + debugWrapProvider string + debugWrapModel string + routeOverride *aiGatewayModelRoute // chatdScopedRoute resolves the route with chatd scope. Deployment-wide // override models must route for user-owned chats regardless of the // caller's actor. @@ -104,11 +83,10 @@ func chatRequestedEffort(chat database.Chat) *string { func standardTurnSpec(chat database.Chat, buildOpts modelBuildOptions) modelCallSpec { return modelCallSpec{ - purpose: callPurposeStandardTurn, + purpose: "standard_turn", chat: chat, config: configSelection{mode: configFromChat}, requestedEffort: chatRequestedEffort(chat), - providerOptions: providerOptionsDerive, debug: debugPolicyAware, defaultMaxOutputTokens: true, buildOptions: buildOpts, @@ -117,14 +95,14 @@ func standardTurnSpec(chat database.Chat, buildOpts modelBuildOptions) modelCall // chatModelSpec preserves summary and status-label behavior: no provider // options and no standard-turn token default. -func chatModelSpec(purpose callPurpose, chat database.Chat, buildOpts modelBuildOptions) modelCallSpec { +func chatModelSpec(purpose string, chat database.Chat, buildOpts modelBuildOptions) modelCallSpec { return modelCallSpec{ - purpose: purpose, - chat: chat, - config: configSelection{mode: configFromChat}, - providerOptions: providerOptionsOmit, - debug: debugPolicyAware, - buildOptions: buildOpts, + purpose: purpose, + chat: chat, + config: configSelection{mode: configFromChat}, + omitProviderOptions: true, + debug: debugPolicyAware, + buildOptions: buildOpts, } } @@ -132,12 +110,11 @@ func chatModelSpec(purpose callPurpose, chat database.Chat, buildOpts modelBuild // the user's per-turn choice. func titleChatSpec(chat database.Chat, buildOpts modelBuildOptions) modelCallSpec { return modelCallSpec{ - purpose: callPurposeTitle, - chat: chat, - config: configSelection{mode: configFromChat}, - providerOptions: providerOptionsDerive, - debug: debugPolicyAware, - buildOptions: buildOpts, + purpose: "title", + chat: chat, + config: configSelection{mode: configFromChat}, + debug: debugPolicyAware, + buildOptions: buildOpts, } } @@ -145,10 +122,9 @@ func titleChatSpec(chat database.Chat, buildOpts modelBuildOptions) modelCallSpe // access. func titleOverrideSpec(chat database.Chat, config database.ChatModelConfig, buildOpts modelBuildOptions) modelCallSpec { return modelCallSpec{ - purpose: callPurposeTitle, + purpose: "title", chat: chat, config: configSelection{mode: configExplicit, config: config}, - providerOptions: providerOptionsDerive, chatdScopedRoute: true, buildOptions: buildOpts, } @@ -158,11 +134,10 @@ func titleOverrideSpec(chat database.Chat, config database.ChatModelConfig, buil // preserve manual-title behavior. func manualTitleSpec(chat database.Chat, config database.ChatModelConfig, buildOpts modelBuildOptions) modelCallSpec { return modelCallSpec{ - purpose: callPurposeTitle, - chat: chat, - config: configSelection{mode: configExplicit, config: config}, - providerOptions: providerOptionsDerive, - buildOptions: buildOpts, + purpose: "title", + chat: chat, + config: configSelection{mode: configExplicit, config: config}, + buildOptions: buildOpts, } } @@ -170,10 +145,9 @@ func manualTitleSpec(chat database.Chat, config database.ChatModelConfig, buildO // uses chatd scope so owners need not have provider read access. func compactionOverrideSpec(chat database.Chat, config database.ChatModelConfig, buildOpts modelBuildOptions) modelCallSpec { return modelCallSpec{ - purpose: callPurposeCompaction, + purpose: "compaction", chat: chat, config: configSelection{mode: configExplicit, config: config}, - providerOptions: providerOptionsDerive, debug: debugPolicyAware, chatdScopedRoute: true, buildOptions: buildOpts, @@ -184,11 +158,11 @@ func compactionOverrideSpec(chat database.Chat, config database.ChatModelConfig, // reasoning effort and output cap. func advisorOverrideSpec(chat database.Chat, config database.ChatModelConfig, buildOpts modelBuildOptions) modelCallSpec { return modelCallSpec{ - purpose: callPurposeAdvisor, - chat: chat, - config: configSelection{mode: configExplicit, config: config}, - providerOptions: providerOptionsOmit, - buildOptions: buildOpts, + purpose: "advisor", + chat: chat, + config: configSelection{mode: configExplicit, config: config}, + omitProviderOptions: true, + buildOptions: buildOpts, } } @@ -203,16 +177,16 @@ func manualTitleDebugSpec( buildOpts modelBuildOptions, ) modelCallSpec { return modelCallSpec{ - purpose: callPurposeDebugRebuild, - chat: chat, - config: configSelection{mode: configExplicit, config: config}, - providerOptions: providerOptionsOmit, - debug: debugPolicyForced, - debugSvc: debugSvc, - debugWrapProvider: routeProvider, - debugWrapModel: config.Model, - routeOverride: &route, - buildOptions: buildOpts, + purpose: "debug_rebuild", + chat: chat, + config: configSelection{mode: configExplicit, config: config}, + omitProviderOptions: true, + debug: debugPolicyForced, + debugSvc: debugSvc, + debugWrapProvider: routeProvider, + debugWrapModel: config.Model, + routeOverride: &route, + buildOptions: buildOpts, } } @@ -226,7 +200,7 @@ func computerUseSpec( buildOpts modelBuildOptions, ) modelCallSpec { return modelCallSpec{ - purpose: callPurposeComputerUse, + purpose: "computer_use", chat: chat, config: configSelection{ mode: configFixedModel, @@ -235,7 +209,6 @@ func computerUseSpec( callConfig: chatCallConfig, }, requestedEffort: chatRequestedEffort(chat), - providerOptions: providerOptionsDerive, debug: debugPolicyAware, buildOptions: buildOpts, } @@ -384,7 +357,7 @@ func (p *Server) resolveModelCall(ctx context.Context, spec modelCallSpec) (reso } out.model = model - if spec.providerOptions == providerOptionsDerive { + if !spec.omitProviderOptions { out.providerOptions = out.deriveProviderOptions(out.callConfig, spec.requestedEffort) } @@ -398,45 +371,28 @@ func (p *Server) resolveModelCall(ctx context.Context, spec modelCallSpec) (reso return out, nil } -type callOverrides struct { - toolChoice *fantasy.ToolChoice - // bare drops the sampling and token fields. Compaction summary calls - // historically send only prompt, tool choice, and provider options. - bare bool - // omitProviderOptions drops the resolved provider options. The - // chat-model compaction summary historically sends none. - omitProviderOptions bool -} - -func compactionSummaryOverrides(omitProviderOptions bool) callOverrides { - toolChoiceNone := fantasy.ToolChoiceNone - return callOverrides{ - bare: true, - toolChoice: &toolChoiceNone, - omitProviderOptions: omitProviderOptions, +// newCall builds a call template; downstream packages copy it and attach the +// prompt and tools they own. +func (r resolvedModelCall) newCall() fantasy.Call { + return fantasy.Call{ + ProviderOptions: r.providerOptions, + MaxOutputTokens: r.callConfig.MaxOutputTokens, + Temperature: r.callConfig.Temperature, + TopP: r.callConfig.TopP, + TopK: r.callConfig.TopK, + PresencePenalty: r.callConfig.PresencePenalty, + FrequencyPenalty: r.callConfig.FrequencyPenalty, } } -// newCall builds a call template; downstream packages copy it and attach the -// prompt and tools they own. -func (r resolvedModelCall) newCall(o callOverrides) fantasy.Call { - call := fantasy.Call{ - ToolChoice: o.toolChoice, +// newCompactionSummaryCall builds the compaction summary template, which +// historically sends only prompt, tool choice, and provider options. +func (r resolvedModelCall) newCompactionSummaryCall() fantasy.Call { + toolChoiceNone := fantasy.ToolChoiceNone + return fantasy.Call{ + ToolChoice: &toolChoiceNone, ProviderOptions: r.providerOptions, } - if o.omitProviderOptions { - call.ProviderOptions = nil - } - if o.bare { - return call - } - call.MaxOutputTokens = r.callConfig.MaxOutputTokens - call.Temperature = r.callConfig.Temperature - call.TopP = r.callConfig.TopP - call.TopK = r.callConfig.TopK - call.PresencePenalty = r.callConfig.PresencePenalty - call.FrequencyPenalty = r.callConfig.FrequencyPenalty - return call } // deriveProviderOptions is the only production ProviderOptionsForCall call @@ -445,20 +401,14 @@ func (r resolvedModelCall) deriveProviderOptions(callConfig codersdk.ChatModelCa return chatprovider.ProviderOptionsForCall(r.model, callConfig, requestedEffort) } -// Quickgen flows use fixed output caps instead of the model config's tuning. -type objectCallOverrides struct { - schemaName string - schemaDescription string - maxOutputTokens int64 -} - // newObjectCall builds a structured-output call envelope; the caller attaches -// the prompt before sending. -func (r resolvedModelCall) newObjectCall(o objectCallOverrides) fantasy.ObjectCall { +// the prompt before sending. Quickgen flows pass fixed output caps instead of +// the model config's tuning. +func (r resolvedModelCall) newObjectCall(schemaName, schemaDescription string, maxOutputTokens int64) fantasy.ObjectCall { return fantasy.ObjectCall{ - SchemaName: o.schemaName, - SchemaDescription: o.schemaDescription, - MaxOutputTokens: ptr.Ref(o.maxOutputTokens), + SchemaName: schemaName, + SchemaDescription: schemaDescription, + MaxOutputTokens: ptr.Ref(maxOutputTokens), ProviderOptions: r.providerOptions, } } diff --git a/coderd/x/chatd/modelcall_shape_internal_test.go b/coderd/x/chatd/modelcall_shape_internal_test.go index 1f9bde83360..115b48f4a52 100644 --- a/coderd/x/chatd/modelcall_shape_internal_test.go +++ b/coderd/x/chatd/modelcall_shape_internal_test.go @@ -309,25 +309,25 @@ func TestModelCallShapeProviderOptionPolicy(t *testing.T) { server := titleOverrideTestServer(db, logger) spec := modelCallSpec{ - purpose: callPurposeStatusLabel, - chat: chat, - config: configSelection{mode: configExplicit, config: config}, - providerOptions: providerOptionsOmit, - buildOptions: modelBuildOptions{ActiveAPIKeyID: uuid.NewString()}, + purpose: "turn_status_label", + chat: chat, + config: configSelection{mode: configExplicit, config: config}, + omitProviderOptions: true, + buildOptions: modelBuildOptions{ActiveAPIKeyID: uuid.NewString()}, } omitted, err := server.resolveModelCall(ctx, spec) require.NoError(t, err) require.Nil(t, omitted.providerOptions) - spec.providerOptions = providerOptionsDerive + spec.omitProviderOptions = false derived, err := server.resolveModelCall(ctx, spec) require.NoError(t, err) require.NotNil(t, derived.providerOptions) - require.Equal(t, providerOptionsOmit, chatModelSpec(callPurposeSummary, chat, modelBuildOptions{}).providerOptions) - require.Equal(t, providerOptionsOmit, chatModelSpec(callPurposeStatusLabel, chat, modelBuildOptions{}).providerOptions) - require.Equal(t, providerOptionsDerive, standardTurnSpec(chat, modelBuildOptions{}).providerOptions) - require.Equal(t, providerOptionsDerive, titleChatSpec(chat, modelBuildOptions{}).providerOptions) + require.True(t, chatModelSpec("chat_summary", chat, modelBuildOptions{}).omitProviderOptions) + require.True(t, chatModelSpec("turn_status_label", chat, modelBuildOptions{}).omitProviderOptions) + require.False(t, standardTurnSpec(chat, modelBuildOptions{}).omitProviderOptions) + require.False(t, titleChatSpec(chat, modelBuildOptions{}).omitProviderOptions) } func TestModelCallShapeTurnStatusLabelEnvelope(t *testing.T) { diff --git a/coderd/x/chatd/quickgen.go b/coderd/x/chatd/quickgen.go index c7e917ef90e..e158817b884 100644 --- a/coderd/x/chatd/quickgen.go +++ b/coderd/x/chatd/quickgen.go @@ -154,20 +154,20 @@ func quickgenDebugSpec( ) modelCallSpec { route := candidate.resolved.route return modelCallSpec{ - purpose: callPurposeDebugRebuild, + purpose: "debug_rebuild", chat: chat, config: configSelection{ mode: configFixedModel, modelName: candidate.model, configOptions: candidate.resolved.dbConfig.Options, }, - providerOptions: providerOptionsOmit, - debug: debugPolicyForced, - debugSvc: debugSvc, - debugWrapProvider: candidate.provider, - debugWrapModel: candidate.model, - routeOverride: &route, - buildOptions: buildOpts, + omitProviderOptions: true, + debug: debugPolicyForced, + debugSvc: debugSvc, + debugWrapProvider: candidate.provider, + debugWrapModel: candidate.model, + routeOverride: &route, + buildOptions: buildOpts, } } @@ -422,11 +422,7 @@ func (p *Server) maybeGenerateChatTitle( const titleMaxOutputTokens = int64(256) func titleObjectCall(resolved resolvedModelCall) fantasy.ObjectCall { - return resolved.newObjectCall(objectCallOverrides{ - schemaName: "propose_title", - schemaDescription: "Propose a short chat title.", - maxOutputTokens: titleMaxOutputTokens, - }) + return resolved.newObjectCall("propose_title", "Propose a short chat title.", titleMaxOutputTokens) } func (p *Server) prepareQuickgenDebugCandidate( @@ -1047,11 +1043,7 @@ func boundTranscriptHeadTail(lines []string, maxRunes int) string { } func summaryObjectCall(resolved resolvedModelCall) fantasy.ObjectCall { - return resolved.newObjectCall(objectCallOverrides{ - schemaName: "chat_summary", - schemaDescription: "Summarize the whole chat in 1-3 sentences.", - maxOutputTokens: summaryMaxOutputTokens, - }) + return resolved.newObjectCall("chat_summary", "Summarize the whole chat in 1-3 sentences.", summaryMaxOutputTokens) } // generateChatSummary generates a 1-3 sentence whole-chat summary from a @@ -1271,11 +1263,7 @@ const turnStatusLabelPrompt = "You write compact chat status labels for a sideba const turnStatusLabelMaxOutputTokens = int64(64) func turnStatusLabelObjectCall(resolved resolvedModelCall) fantasy.ObjectCall { - return resolved.newObjectCall(objectCallOverrides{ - schemaName: "propose_turn_status_label", - schemaDescription: "Propose a compact chat status label.", - maxOutputTokens: turnStatusLabelMaxOutputTokens, - }) + return resolved.newObjectCall("propose_turn_status_label", "Propose a compact chat status label.", turnStatusLabelMaxOutputTokens) } // generateTurnStatusLabel returns an empty string if generation fails. From bcbb630596ca5d2e9e2da09933bbcb338c8a281c Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Thu, 13 Aug 2026 10:05:49 +0000 Subject: [PATCH 09/13] test(coderd/x/chatd): consolidate model-call shape tests into flow owners Fold standard-turn and compaction-summary template assertions into the reasoning-effort clamp test, manual-title provider-option assertions into the synthetic API key test, and status-label envelope assertions into the structured status-label test. Replace the end-to-end summary fixture with a focused resolver-level omission test that also covers summaryObjectCall. Delete modelcall_shape_internal_test.go. --- .../generation_preparer_internal_test.go | 19 + coderd/x/chatd/modelcall_internal_test.go | 64 +++ .../x/chatd/modelcall_shape_internal_test.go | 385 ------------------ coderd/x/chatd/quickgen_internal_test.go | 4 + .../x/chatd/title_override_internal_test.go | 11 + 5 files changed, 98 insertions(+), 385 deletions(-) create mode 100644 coderd/x/chatd/modelcall_internal_test.go delete mode 100644 coderd/x/chatd/modelcall_shape_internal_test.go diff --git a/coderd/x/chatd/generation_preparer_internal_test.go b/coderd/x/chatd/generation_preparer_internal_test.go index 9ef8e8e8b73..139b06bd074 100644 --- a/coderd/x/chatd/generation_preparer_internal_test.go +++ b/coderd/x/chatd/generation_preparer_internal_test.go @@ -104,6 +104,11 @@ func TestPrepareGenerationClampsRequestedReasoningEffortToMax(t *testing.T) { Type: database.AIProviderTypeOpenai, }, "test-key") modelConfigRaw, err := json.Marshal(codersdk.ChatModelCallConfig{ + ProviderOptions: &codersdk.ChatModelProviderOptions{ + OpenAI: &codersdk.ChatModelOpenAIProviderOptions{ + User: ptr.Ref("turn-options-sentinel"), + }, + }, ReasoningEffort: &codersdk.ChatModelReasoningEffortConfig{ Default: ptr.Ref(codersdk.ChatModelReasoningEffortLow), Max: ptr.Ref(codersdk.ChatModelReasoningEffortMedium), @@ -159,6 +164,20 @@ func TestPrepareGenerationClampsRequestedReasoningEffortToMax(t *testing.T) { require.True(t, ok, "%T", prepared.CallTemplate.ProviderOptions[fantasyopenai.Name]) require.NotNil(t, providerOptions.ReasoningEffort) require.Equal(t, fantasyopenai.ReasoningEffortMedium, *providerOptions.ReasoningEffort) + + // The standard-turn template carries the config's provider options and + // the default output cap. + require.NotNil(t, providerOptions.User) + require.Equal(t, "turn-options-sentinel", *providerOptions.User) + require.NotNil(t, prepared.CallTemplate.MaxOutputTokens) + require.Equal(t, defaultChatMaxOutputTokens, *prepared.CallTemplate.MaxOutputTokens) + + // The prepared compaction summary template historically sends no + // provider options and forbids tool calls. + require.NotNil(t, prepared.Compaction) + require.Nil(t, prepared.Compaction.Options.SummaryCall.ProviderOptions) + require.NotNil(t, prepared.Compaction.Options.SummaryCall.ToolChoice) + require.Equal(t, fantasy.ToolChoiceNone, *prepared.Compaction.Options.SummaryCall.ToolChoice) } func TestPrepareGenerationComputerUseIgnoresChatTransportOverride(t *testing.T) { diff --git a/coderd/x/chatd/modelcall_internal_test.go b/coderd/x/chatd/modelcall_internal_test.go new file mode 100644 index 00000000000..6423cbe958a --- /dev/null +++ b/coderd/x/chatd/modelcall_internal_test.go @@ -0,0 +1,64 @@ +package chatd + +import ( + "encoding/json" + "testing" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" + + "cdr.dev/slog/v3/sloggers/slogtest" + "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/dbmock" + "github.com/coder/coder/v2/coderd/util/ptr" + "github.com/coder/coder/v2/codersdk" + "github.com/coder/coder/v2/testutil" +) + +// modelCallSentinelOptions builds config options whose OpenAI user field acts +// as a sentinel: its presence in a request proves provider options were +// derived from the config, and its absence proves they were omitted. +func modelCallSentinelOptions(t *testing.T, user string) json.RawMessage { + t.Helper() + raw, err := json.Marshal(codersdk.ChatModelCallConfig{ + ProviderOptions: &codersdk.ChatModelProviderOptions{ + OpenAI: &codersdk.ChatModelOpenAIProviderOptions{ + User: ptr.Ref(user), + }, + }, + }) + require.NoError(t, err) + return raw +} + +// TestChatModelSpecOmitsProviderOptions locks the historical omission for +// whole-chat summaries and status labels: the resolver must not derive +// provider options even when the config declares them. +func TestChatModelSpecOmitsProviderOptions(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + ctrl := gomock.NewController(t) + db := dbmock.NewMockStore(ctrl) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + chat, _ := titleOverrideTestChatAndMessages(t) + providerID := uuid.New() + config := titleOverrideModelConfig("gpt-4o-mini", true) + config.AIProviderID = uuid.NullUUID{UUID: providerID, Valid: true} + config.Options = modelCallSentinelOptions(t, "summary-options-sentinel") + chat.LastModelConfigID = config.ID + + db.EXPECT().GetChatModelConfigByID(gomock.Any(), config.ID).Return(config, nil) + db.EXPECT().GetAIProviderByID(gomock.Any(), providerID).Return(aibridgeTestAIProvider(providerID, "primary-openai", database.AIProviderTypeOpenai), nil).AnyTimes() + db.EXPECT().GetAIProviderKeysByProviderID(gomock.Any(), providerID).Return([]database.AIProviderKey{{ + ProviderID: providerID, + APIKey: "test-key", + }}, nil).AnyTimes() + + server := titleOverrideTestServer(db, logger) + resolved, err := server.resolveModelCall(ctx, chatModelSpec("chat_summary", chat, modelBuildOptions{ActiveAPIKeyID: uuid.NewString()})) + require.NoError(t, err) + require.Nil(t, resolved.providerOptions) + require.Nil(t, summaryObjectCall(resolved).ProviderOptions) +} diff --git a/coderd/x/chatd/modelcall_shape_internal_test.go b/coderd/x/chatd/modelcall_shape_internal_test.go deleted file mode 100644 index 115b48f4a52..00000000000 --- a/coderd/x/chatd/modelcall_shape_internal_test.go +++ /dev/null @@ -1,385 +0,0 @@ -package chatd //nolint:testpackage // Locks unexported model-call construction behavior. - -import ( - "context" - "encoding/json" - "io" - "net/http" - "strconv" - "strings" - "sync" - "testing" - "time" - - "charm.land/fantasy" - fantasyopenai "charm.land/fantasy/providers/openai" - "github.com/google/uuid" - "github.com/stretchr/testify/require" - "go.uber.org/mock/gomock" - - "cdr.dev/slog/v3/sloggers/slogtest" - "github.com/coder/coder/v2/coderd/database" - "github.com/coder/coder/v2/coderd/database/dbgen" - "github.com/coder/coder/v2/coderd/database/dbmock" - "github.com/coder/coder/v2/coderd/database/dbtestutil" - "github.com/coder/coder/v2/coderd/util/ptr" - "github.com/coder/coder/v2/coderd/x/chatd/chatprompt" - "github.com/coder/coder/v2/coderd/x/chatd/chatprovider" - "github.com/coder/coder/v2/coderd/x/chatd/chatstate" - "github.com/coder/coder/v2/coderd/x/chatd/chattest" - "github.com/coder/coder/v2/codersdk" - "github.com/coder/coder/v2/testutil" - "github.com/coder/quartz" -) - -func modelCallSentinelOptions(t *testing.T, user string) json.RawMessage { - t.Helper() - raw, err := json.Marshal(codersdk.ChatModelCallConfig{ - ProviderOptions: &codersdk.ChatModelProviderOptions{ - OpenAI: &codersdk.ChatModelOpenAIProviderOptions{ - User: ptr.Ref(user), - }, - }, - }) - require.NoError(t, err) - return raw -} - -func openAIResponsesObjectBody(t *testing.T, object string) string { - t.Helper() - text := strconv.Quote(object) - return `{"id":"resp_test","object":"response","created_at":0,"status":"completed","model":"gpt-4o-mini","output":[{"id":"msg_test","type":"message","role":"assistant","content":[{"type":"output_text","text":` + text + `}]}],"usage":{"input_tokens":1,"output_tokens":1,"total_tokens":2}}` -} - -func TestModelCallShapeStandardTurn(t *testing.T) { - t.Parallel() - - db, ps := dbtestutil.NewDB(t) - ctx := chatdTestContext(t) - user := dbgen.User(t, db, database.User{}) - org := dbgen.Organization(t, db, database.Organization{}) - dbgen.OrganizationMember(t, db, database.OrganizationMember{ - UserID: user.ID, - OrganizationID: org.ID, - }) - provider := dbgen.AIProviderWithOptionalKey(t, db, database.AIProvider{ - Type: database.AIProviderTypeOpenai, - }, "test-key") - modelConfig := dbgen.ChatModelConfig(t, db, database.ChatModelConfig{ - Model: "gpt-4o-mini", - Options: modelCallSentinelOptions(t, "turn-options-sentinel"), - AIProviderID: uuid.NullUUID{UUID: provider.ID, Valid: true}, - }, func(p *database.InsertChatModelConfigParams) { - p.Enabled = true - }) - - created, err := chatstate.CreateChat(ctx, db, ps, chatstate.CreateChatInput{ - OrganizationID: org.ID, - OwnerID: user.ID, - LastModelConfigID: modelConfig.ID, - Title: "standard turn request shape", - ClientType: database.ChatClientTypeApi, - InitialMessages: []chatstate.Message{ - { - Role: database.ChatMessageRoleUser, - Content: mustMarshalText(t, "hello"), - Visibility: database.ChatMessageVisibilityBoth, - ModelConfigID: uuid.NullUUID{UUID: modelConfig.ID, Valid: true}, - CreatedBy: uuid.NullUUID{UUID: user.ID, Valid: true}, - ContentVersion: chatprompt.CurrentContentVersion, - }, - }, - }) - require.NoError(t, err) - - server := newInternalTestServer( - t, - db, - ps, - chatprovider.ProviderAPIKeys{}, - withInternalTestServerTransportFactory(&aibridgeTestFactory{}), - ) - prepared, err := server.prepareGeneration(ctx, generationPrepareInput{ - Chat: created.Chat, - Messages: created.InitialMessages, - }) - require.NoError(t, err) - t.Cleanup(prepared.Cleanup) - - providerOptions, ok := prepared.CallTemplate.ProviderOptions[fantasyopenai.Name].(*fantasyopenai.ResponsesProviderOptions) - require.True(t, ok, "%T", prepared.CallTemplate.ProviderOptions[fantasyopenai.Name]) - require.NotNil(t, providerOptions.User) - require.Equal(t, "turn-options-sentinel", *providerOptions.User) - - require.NotNil(t, prepared.CallTemplate.MaxOutputTokens) - require.Equal(t, int64(32_000), *prepared.CallTemplate.MaxOutputTokens) - - require.NotNil(t, prepared.Compaction) - require.Nil(t, prepared.Compaction.Options.SummaryCall.ProviderOptions) - require.NotNil(t, prepared.Compaction.Options.SummaryCall.ToolChoice) - require.Equal(t, fantasy.ToolChoiceNone, *prepared.Compaction.Options.SummaryCall.ToolChoice) -} - -func TestModelCallShapeManualTitleCarriesProviderOptions(t *testing.T) { - t.Parallel() - - ctx := testutil.Context(t, testutil.WaitShort) - ctrl := gomock.NewController(t) - db := dbmock.NewMockStore(ctrl) - logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) - chat, messages := titleOverrideTestChatAndMessages(t) - chat.OrganizationID = uuid.New() - overrideConfig := titleOverrideModelConfig("gpt-4.1", true) - providerID := uuid.New() - overrideConfig.AIProviderID = uuid.NullUUID{UUID: providerID, Valid: true} - overrideConfig.Options = modelCallSentinelOptions(t, "title-options-sentinel") - provider := database.AIProvider{ - ID: providerID, - Name: "primary-openai", - Type: database.AIProviderTypeOpenai, - Enabled: true, - } - - var ( - bodyMu sync.Mutex - bodies [][]byte - ) - factory := &aibridgeTestFactory{rt: roundTripFunc(func(req *http.Request) (*http.Response, error) { - bodyBytes, err := io.ReadAll(req.Body) - require.NoError(t, err) - bodyMu.Lock() - bodies = append(bodies, bodyBytes) - bodyMu.Unlock() - body := openAIResponsesObjectBody(t, `{"title":"Locked title"}`) - return &http.Response{ - StatusCode: http.StatusOK, - Header: http.Header{"Content-Type": []string{"application/json"}}, - Body: io.NopCloser(strings.NewReader(body)), - Request: req, - }, nil - })} - - db.EXPECT().GetChatMessagesByChatIDAscPaginated(gomock.Any(), database.GetChatMessagesByChatIDAscPaginatedParams{ - ChatID: chat.ID, - AfterID: 0, - LimitVal: manualTitleMessageWindowLimit, - }).Return(messages, nil) - db.EXPECT().GetChatMessagesByChatIDDescPaginated(gomock.Any(), database.GetChatMessagesByChatIDDescPaginatedParams{ - ChatID: chat.ID, - BeforeID: 0, - LimitVal: manualTitleMessageWindowLimit, - }).Return(nil, nil) - db.EXPECT().GetChatGatewayAPIKey(gomock.Any(), database.GetChatGatewayAPIKeyParams{ - UserID: chat.OwnerID, - TokenName: GatewayTokenName(chat.OwnerID), - }).Return(database.APIKey{ - ID: uuid.NewString(), - UserID: chat.OwnerID, - ExpiresAt: time.Now().Add(48 * time.Hour), - }, nil) - db.EXPECT().GetChatTitleGenerationModelOverride(gomock.Any()).Return(overrideConfig.ID.String(), nil) - db.EXPECT().GetChatModelConfigByID(gomock.Any(), overrideConfig.ID).Return(overrideConfig, nil) - db.EXPECT().GetAIProviderByID(gomock.Any(), providerID).Return(provider, nil).AnyTimes() - db.EXPECT().GetAIProviderKeysByProviderID(gomock.Any(), providerID).Return([]database.AIProviderKey{{ - ProviderID: providerID, - APIKey: "test-key", - }}, nil).AnyTimes() - - server := titleOverrideTestServer(db, logger) - server.clock = quartz.NewReal() - server.aibridgeTransportFactory = aibridgeTestFactoryPointer(factory) - title, err := server.generateManualTitleCandidate(ctx, db, chat) - require.NoError(t, err) - require.Equal(t, "Locked title", title) - - bodyMu.Lock() - defer bodyMu.Unlock() - require.Len(t, bodies, 1) - var raw map[string]any - require.NoError(t, json.Unmarshal(bodies[0], &raw)) - require.Equal(t, "title-options-sentinel", raw["user"]) -} - -func TestModelCallShapeChatSummaryOmitsProviderOptions(t *testing.T) { - t.Parallel() - - db, ps := dbtestutil.NewDB(t) - ctx := chatdTestContext(t) - user := dbgen.User(t, db, database.User{}) - org := dbgen.Organization(t, db, database.Organization{}) - dbgen.OrganizationMember(t, db, database.OrganizationMember{ - UserID: user.ID, - OrganizationID: org.ID, - }) - provider := dbgen.AIProviderWithOptionalKey(t, db, database.AIProvider{ - Type: database.AIProviderTypeOpenai, - }, "test-key") - modelConfig := dbgen.ChatModelConfig(t, db, database.ChatModelConfig{ - Model: "gpt-4o-mini", - Options: modelCallSentinelOptions(t, "summary-options-sentinel"), - AIProviderID: uuid.NullUUID{UUID: provider.ID, Valid: true}, - }, func(p *database.InsertChatModelConfigParams) { - p.Enabled = true - }) - - longPrompt := strings.Repeat("please summarize this conversation carefully ", 10) - created, err := chatstate.CreateChat(ctx, db, ps, chatstate.CreateChatInput{ - OrganizationID: org.ID, - OwnerID: user.ID, - LastModelConfigID: modelConfig.ID, - Title: "summary request shape", - ClientType: database.ChatClientTypeApi, - InitialMessages: []chatstate.Message{ - { - Role: database.ChatMessageRoleUser, - Content: mustMarshalText(t, longPrompt), - Visibility: database.ChatMessageVisibilityBoth, - ModelConfigID: uuid.NullUUID{UUID: modelConfig.ID, Valid: true}, - CreatedBy: uuid.NullUUID{UUID: user.ID, Valid: true}, - ContentVersion: chatprompt.CurrentContentVersion, - }, - }, - }) - require.NoError(t, err) - - var ( - bodyMu sync.Mutex - bodies [][]byte - ) - factory := &aibridgeTestFactory{rt: roundTripFunc(func(req *http.Request) (*http.Response, error) { - bodyBytes, err := io.ReadAll(req.Body) - require.NoError(t, err) - bodyMu.Lock() - bodies = append(bodies, bodyBytes) - bodyMu.Unlock() - body := openAIResponsesObjectBody(t, `{"summary":"A locked summary."}`) - return &http.Response{ - StatusCode: http.StatusOK, - Header: http.Header{"Content-Type": []string{"application/json"}}, - Body: io.NopCloser(strings.NewReader(body)), - Request: req, - }, nil - })} - - server := newInternalTestServer( - t, - db, - ps, - chatprovider.ProviderAPIKeys{}, - withInternalTestServerTransportFactory(factory), - ) - logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) - server.generateAndStoreChatSummary(ctx, logger, created.Chat) - - fetched, err := db.GetChatByID(ctx, created.Chat.ID) - require.NoError(t, err) - require.True(t, fetched.Summary.Valid) - require.Equal(t, "A locked summary.", fetched.Summary.String) - - bodyMu.Lock() - defer bodyMu.Unlock() - require.Len(t, bodies, 1) - var raw map[string]any - require.NoError(t, json.Unmarshal(bodies[0], &raw)) - require.NotContains(t, raw, "user") -} - -// TestModelCallShapeProviderOptionPolicy locks the resolver's -// provider-option policy handling and each flow's declared policy, so the -// summary and status-label omission cannot silently regress. -func TestModelCallShapeProviderOptionPolicy(t *testing.T) { - t.Parallel() - - ctx := testutil.Context(t, testutil.WaitShort) - ctrl := gomock.NewController(t) - db := dbmock.NewMockStore(ctrl) - logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) - chat, _ := titleOverrideTestChatAndMessages(t) - providerID := uuid.New() - config := titleOverrideModelConfig("gpt-4o-mini", true) - config.AIProviderID = uuid.NullUUID{UUID: providerID, Valid: true} - config.Options = modelCallSentinelOptions(t, "policy-sentinel") - - db.EXPECT().GetAIProviderByID(gomock.Any(), providerID).Return(aibridgeTestAIProvider(providerID, "primary-openai", database.AIProviderTypeOpenai), nil).AnyTimes() - db.EXPECT().GetAIProviderKeysByProviderID(gomock.Any(), providerID).Return([]database.AIProviderKey{{ - ProviderID: providerID, - APIKey: "test-key", - }}, nil).AnyTimes() - - server := titleOverrideTestServer(db, logger) - - spec := modelCallSpec{ - purpose: "turn_status_label", - chat: chat, - config: configSelection{mode: configExplicit, config: config}, - omitProviderOptions: true, - buildOptions: modelBuildOptions{ActiveAPIKeyID: uuid.NewString()}, - } - omitted, err := server.resolveModelCall(ctx, spec) - require.NoError(t, err) - require.Nil(t, omitted.providerOptions) - - spec.omitProviderOptions = false - derived, err := server.resolveModelCall(ctx, spec) - require.NoError(t, err) - require.NotNil(t, derived.providerOptions) - - require.True(t, chatModelSpec("chat_summary", chat, modelBuildOptions{}).omitProviderOptions) - require.True(t, chatModelSpec("turn_status_label", chat, modelBuildOptions{}).omitProviderOptions) - require.False(t, standardTurnSpec(chat, modelBuildOptions{}).omitProviderOptions) - require.False(t, titleChatSpec(chat, modelBuildOptions{}).omitProviderOptions) -} - -func TestModelCallShapeTurnStatusLabelEnvelope(t *testing.T) { - t.Parallel() - - ctx := testutil.Context(t, testutil.WaitShort) - logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) - - var ( - callMu sync.Mutex - captured []fantasy.ObjectCall - ) - model := &chattest.FakeModel{ - ProviderName: fantasyopenai.Name, - ModelName: "gpt-4o-mini", - GenerateObjectFn: func(_ context.Context, call fantasy.ObjectCall) (*fantasy.ObjectResponse, error) { - callMu.Lock() - captured = append(captured, call) - callMu.Unlock() - return &fantasy.ObjectResponse{ - Object: map[string]any{"label": "Finished the tests"}, - }, nil - }, - } - - server := &Server{logger: logger} - label := server.generateTurnStatusLabel( - ctx, - database.Chat{ID: uuid.New(), OwnerID: uuid.New(), Title: "status shape"}, - database.ChatStatusWaiting, - "All tests pass now.", - resolvedModelCall{ - model: chatprovider.NewModel(model, nil), - resolvedProvider: fantasyopenai.Name, - resolvedModel: "gpt-4o-mini", - }, - modelBuildOptions{}, - logger, - nil, - 0, - 0, - ) - require.Equal(t, "Finished the tests", label) - - callMu.Lock() - defer callMu.Unlock() - require.Len(t, captured, 1) - call := captured[0] - require.Nil(t, call.ProviderOptions) - require.NotNil(t, call.MaxOutputTokens) - require.Equal(t, int64(64), *call.MaxOutputTokens) - require.NotNil(t, call.Temperature) - require.Equal(t, quickgenTemperature, *call.Temperature) - require.Equal(t, "propose_turn_status_label", call.SchemaName) -} diff --git a/coderd/x/chatd/quickgen_internal_test.go b/coderd/x/chatd/quickgen_internal_test.go index e5208928801..af2ee1c32b7 100644 --- a/coderd/x/chatd/quickgen_internal_test.go +++ b/coderd/x/chatd/quickgen_internal_test.go @@ -1030,6 +1030,10 @@ func TestGenerateStructuredTurnStatusLabel(t *testing.T) { model := &chattest.FakeModel{ GenerateObjectFn: func(_ context.Context, call fantasy.ObjectCall) (*fantasy.ObjectResponse, error) { require.Equal(t, "propose_turn_status_label", call.SchemaName) + require.NotNil(t, call.MaxOutputTokens) + require.Equal(t, turnStatusLabelMaxOutputTokens, *call.MaxOutputTokens) + require.NotNil(t, call.Temperature) + require.Equal(t, quickgenTemperature, *call.Temperature) return &fantasy.ObjectResponse{ Object: map[string]any{"label": "Submitted PR"}, }, nil diff --git a/coderd/x/chatd/title_override_internal_test.go b/coderd/x/chatd/title_override_internal_test.go index 318255f7778..8c547cbb232 100644 --- a/coderd/x/chatd/title_override_internal_test.go +++ b/coderd/x/chatd/title_override_internal_test.go @@ -564,6 +564,7 @@ func TestGenerateManualTitleCandidate_UsesSyntheticAPIKey(t *testing.T) { overrideConfig := titleOverrideModelConfig("gpt-4.1", true) providerID := uuid.New() overrideConfig.AIProviderID = uuid.NullUUID{UUID: providerID, Valid: true} + overrideConfig.Options = modelCallSentinelOptions(t, "title-options-sentinel") provider := database.AIProvider{ ID: providerID, Name: "primary-openai", @@ -573,9 +574,13 @@ func TestGenerateManualTitleCandidate_UsesSyntheticAPIKey(t *testing.T) { apiKeyID := uuid.NewString() wantTitle := "Synthetic title" seenAPIKeyID := make(chan string, 1) + seenBody := make(chan []byte, 1) factory := &aibridgeTestFactory{rt: roundTripFunc(func(req *http.Request) (*http.Response, error) { delegatedID, _ := aibridge.DelegatedAPIKeyIDFromContext(req.Context()) seenAPIKeyID <- delegatedID + bodyBytes, err := io.ReadAll(req.Body) + require.NoError(t, err) + seenBody <- bodyBytes text := strconv.Quote(`{"title":"` + wantTitle + `"}`) body := `{"id":"resp_test","object":"response","created_at":0,"status":"completed","model":"gpt-4.1","output":[{"id":"msg_test","type":"message","role":"assistant","content":[{"type":"output_text","text":` + text + `}]}],"usage":{"input_tokens":1,"output_tokens":1,"total_tokens":2}}` return &http.Response{ @@ -619,6 +624,12 @@ func TestGenerateManualTitleCandidate_UsesSyntheticAPIKey(t *testing.T) { require.NoError(t, err) require.Equal(t, wantTitle, title) require.Equal(t, apiKeyID, testutil.RequireReceive(ctx, t, seenAPIKeyID)) + + // The manual-title flow derives provider options from the override + // config, so the sentinel must reach the request body. + var raw map[string]any + require.NoError(t, json.Unmarshal(testutil.RequireReceive(ctx, t, seenBody), &raw)) + require.Equal(t, "title-options-sentinel", raw["user"]) } func TestResolveManualTitleModel_TitleGenerationOverrideSetUnusable(t *testing.T) { From b6ca22befb83e2b3eb814250e7d867f1fb6c23db Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Thu, 13 Aug 2026 10:19:52 +0000 Subject: [PATCH 10/13] refactor(coderd/x/chatd): tighten comments from cleanup audit --- coderd/x/chatd/generation_preparer.go | 4 ++-- coderd/x/chatd/generation_preparer_internal_test.go | 4 ---- coderd/x/chatd/modelcall.go | 11 ++--------- coderd/x/chatd/modelcall_internal_test.go | 6 ------ coderd/x/chatd/title_override_internal_test.go | 2 -- 5 files changed, 4 insertions(+), 23 deletions(-) diff --git a/coderd/x/chatd/generation_preparer.go b/coderd/x/chatd/generation_preparer.go index a371dba4178..7235167d895 100644 --- a/coderd/x/chatd/generation_preparer.go +++ b/coderd/x/chatd/generation_preparer.go @@ -615,8 +615,8 @@ func (server *Server) prepareGeneration( } compactionStepUsage := latestPromptUsage(promptRows) compactionNeeded := shouldCompactPromptUsage(compactionStepUsage, compactionContextLimit, effectiveThreshold) - // The chat-model compaction summary historically sends no provider - // options; the override-model summary in generateCompaction keeps them. + // Base-model summaries omit provider options; generateCompaction replaces + // this call when an override model is configured. summaryCall := resolved.newCompactionSummaryCall() summaryCall.ProviderOptions = nil // The options carry the chat model; generateCompaction swaps in the diff --git a/coderd/x/chatd/generation_preparer_internal_test.go b/coderd/x/chatd/generation_preparer_internal_test.go index 139b06bd074..5a139ea5b7d 100644 --- a/coderd/x/chatd/generation_preparer_internal_test.go +++ b/coderd/x/chatd/generation_preparer_internal_test.go @@ -165,15 +165,11 @@ func TestPrepareGenerationClampsRequestedReasoningEffortToMax(t *testing.T) { require.NotNil(t, providerOptions.ReasoningEffort) require.Equal(t, fantasyopenai.ReasoningEffortMedium, *providerOptions.ReasoningEffort) - // The standard-turn template carries the config's provider options and - // the default output cap. require.NotNil(t, providerOptions.User) require.Equal(t, "turn-options-sentinel", *providerOptions.User) require.NotNil(t, prepared.CallTemplate.MaxOutputTokens) require.Equal(t, defaultChatMaxOutputTokens, *prepared.CallTemplate.MaxOutputTokens) - // The prepared compaction summary template historically sends no - // provider options and forbids tool calls. require.NotNil(t, prepared.Compaction) require.Nil(t, prepared.Compaction.Options.SummaryCall.ProviderOptions) require.NotNil(t, prepared.Compaction.Options.SummaryCall.ToolChoice) diff --git a/coderd/x/chatd/modelcall.go b/coderd/x/chatd/modelcall.go index 791199be468..2e108522bd7 100644 --- a/coderd/x/chatd/modelcall.go +++ b/coderd/x/chatd/modelcall.go @@ -58,8 +58,7 @@ type modelCallSpec struct { chat database.Chat config configSelection requestedEffort *string - // omitProviderOptions skips derivation. Used by flows that historically - // never sent provider options and by callers that derive separately. + omitProviderOptions bool debug debugPolicy debugSvc *chatdebug.Service @@ -371,8 +370,6 @@ func (p *Server) resolveModelCall(ctx context.Context, spec modelCallSpec) (reso return out, nil } -// newCall builds a call template; downstream packages copy it and attach the -// prompt and tools they own. func (r resolvedModelCall) newCall() fantasy.Call { return fantasy.Call{ ProviderOptions: r.providerOptions, @@ -385,8 +382,7 @@ func (r resolvedModelCall) newCall() fantasy.Call { } } -// newCompactionSummaryCall builds the compaction summary template, which -// historically sends only prompt, tool choice, and provider options. +// Compaction summaries omit sampling and output-token options. func (r resolvedModelCall) newCompactionSummaryCall() fantasy.Call { toolChoiceNone := fantasy.ToolChoiceNone return fantasy.Call{ @@ -401,9 +397,6 @@ func (r resolvedModelCall) deriveProviderOptions(callConfig codersdk.ChatModelCa return chatprovider.ProviderOptionsForCall(r.model, callConfig, requestedEffort) } -// newObjectCall builds a structured-output call envelope; the caller attaches -// the prompt before sending. Quickgen flows pass fixed output caps instead of -// the model config's tuning. func (r resolvedModelCall) newObjectCall(schemaName, schemaDescription string, maxOutputTokens int64) fantasy.ObjectCall { return fantasy.ObjectCall{ SchemaName: schemaName, diff --git a/coderd/x/chatd/modelcall_internal_test.go b/coderd/x/chatd/modelcall_internal_test.go index 6423cbe958a..e969e75338c 100644 --- a/coderd/x/chatd/modelcall_internal_test.go +++ b/coderd/x/chatd/modelcall_internal_test.go @@ -16,9 +16,6 @@ import ( "github.com/coder/coder/v2/testutil" ) -// modelCallSentinelOptions builds config options whose OpenAI user field acts -// as a sentinel: its presence in a request proves provider options were -// derived from the config, and its absence proves they were omitted. func modelCallSentinelOptions(t *testing.T, user string) json.RawMessage { t.Helper() raw, err := json.Marshal(codersdk.ChatModelCallConfig{ @@ -32,9 +29,6 @@ func modelCallSentinelOptions(t *testing.T, user string) json.RawMessage { return raw } -// TestChatModelSpecOmitsProviderOptions locks the historical omission for -// whole-chat summaries and status labels: the resolver must not derive -// provider options even when the config declares them. func TestChatModelSpecOmitsProviderOptions(t *testing.T) { t.Parallel() diff --git a/coderd/x/chatd/title_override_internal_test.go b/coderd/x/chatd/title_override_internal_test.go index 8c547cbb232..7499066f65e 100644 --- a/coderd/x/chatd/title_override_internal_test.go +++ b/coderd/x/chatd/title_override_internal_test.go @@ -625,8 +625,6 @@ func TestGenerateManualTitleCandidate_UsesSyntheticAPIKey(t *testing.T) { require.Equal(t, wantTitle, title) require.Equal(t, apiKeyID, testutil.RequireReceive(ctx, t, seenAPIKeyID)) - // The manual-title flow derives provider options from the override - // config, so the sentinel must reach the request body. var raw map[string]any require.NoError(t, json.Unmarshal(testutil.RequireReceive(ctx, t, seenBody), &raw)) require.Equal(t, "title-options-sentinel", raw["user"]) From fb84101639c60ec53ef8ea9f99b3b02faea587bc Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Thu, 13 Aug 2026 10:27:42 +0000 Subject: [PATCH 11/13] refactor(coderd/x/chatd): pass compaction summary provider options as data Cleanup-gate follow-up: replace the newCompactionSummaryCall method plus caller-side ProviderOptions mutation with a compactionSummaryCall free function that takes the provider options directly. --- coderd/x/chatd/generation.go | 2 +- coderd/x/chatd/generation_preparer.go | 7 ++----- coderd/x/chatd/modelcall.go | 8 +++++--- 3 files changed, 8 insertions(+), 9 deletions(-) diff --git a/coderd/x/chatd/generation.go b/coderd/x/chatd/generation.go index 66a551672a2..8ff9890159d 100644 --- a/coderd/x/chatd/generation.go +++ b/coderd/x/chatd/generation.go @@ -930,7 +930,7 @@ func (s *taskStarter) generateCompaction( compactionOpts.ResolvedProvider = overrideModel.resolvedProvider compactionOpts.ResolvedModel = overrideModel.resolvedModel compactionOpts.ModelConfigID = overrideModel.dbConfig.ID - compactionOpts.SummaryCall = overrideModel.newCompactionSummaryCall() + compactionOpts.SummaryCall = compactionSummaryCall(overrideModel.providerOptions) compactionOpts.Messages = sanitizeCompactionPrompt( ctx, logger, diff --git a/coderd/x/chatd/generation_preparer.go b/coderd/x/chatd/generation_preparer.go index 7235167d895..60c2c69d5c1 100644 --- a/coderd/x/chatd/generation_preparer.go +++ b/coderd/x/chatd/generation_preparer.go @@ -615,10 +615,6 @@ func (server *Server) prepareGeneration( } compactionStepUsage := latestPromptUsage(promptRows) compactionNeeded := shouldCompactPromptUsage(compactionStepUsage, compactionContextLimit, effectiveThreshold) - // Base-model summaries omit provider options; generateCompaction replaces - // this call when an override model is configured. - summaryCall := resolved.newCompactionSummaryCall() - summaryCall.ProviderOptions = nil // The options carry the chat model; generateCompaction swaps in the // override client when one is configured. compactionOptions := chatloop.GenerateCompactionOptions{ @@ -636,7 +632,8 @@ func (server *Server) prepareGeneration( ResolvedModel: resolved.resolvedModel, ModelConfigID: modelConfig.ID, StepUsage: compactionStepUsage, - SummaryCall: summaryCall, + // The chat-model summary historically sends no provider options. + SummaryCall: compactionSummaryCall(nil), } // workspaceCtx.currentChatSnapshot may carry a freshly persisted diff --git a/coderd/x/chatd/modelcall.go b/coderd/x/chatd/modelcall.go index 2e108522bd7..6d7033729f1 100644 --- a/coderd/x/chatd/modelcall.go +++ b/coderd/x/chatd/modelcall.go @@ -382,12 +382,14 @@ func (r resolvedModelCall) newCall() fantasy.Call { } } -// Compaction summaries omit sampling and output-token options. -func (r resolvedModelCall) newCompactionSummaryCall() fantasy.Call { +// Compaction summaries omit sampling and output-token options. The chat-model +// summary passes nil provider options; the override-model summary passes its +// resolved options. +func compactionSummaryCall(providerOptions fantasy.ProviderOptions) fantasy.Call { toolChoiceNone := fantasy.ToolChoiceNone return fantasy.Call{ ToolChoice: &toolChoiceNone, - ProviderOptions: r.providerOptions, + ProviderOptions: providerOptions, } } From 4a5f5f066c9dbd5f525a9f09927393a812a5884e Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Thu, 13 Aug 2026 19:55:48 +0000 Subject: [PATCH 12/13] refactor(coderd/x/chatd): unify model-call behavior behind data-only spec options Delete the purpose-specific spec constructors and the per-flow policy flags (omitProviderOptions, debug tri-state, defaultMaxOutputTokens, routeOverride). Every resolved call now derives provider options, defaults MaxOutputTokens, and wraps for debug recording when chat debug is enabled; clients are always constructed with the configured model string so gateway validation sees the configured name. Debug helpers only create and finalize run records instead of rebuilding models. Callers describe calls with inline modelCallSpec literals whose fields are plain data: model source, requested effort, and chatd-scoped routing. --- coderd/x/chatd/ARCHITECTURE.md | 12 +- coderd/x/chatd/advisor_internal_test.go | 4 +- coderd/x/chatd/chatd.go | 94 ++---- coderd/x/chatd/chatd_internal_test.go | 70 ---- .../compaction_override_internal_test.go | 12 +- coderd/x/chatd/generation.go | 10 +- coderd/x/chatd/generation_preparer.go | 36 +- .../generation_preparer_internal_test.go | 10 +- coderd/x/chatd/model_routing_internal_test.go | 52 +-- coderd/x/chatd/modelcall.go | 316 ++++-------------- coderd/x/chatd/modelcall_internal_test.go | 30 +- coderd/x/chatd/quickgen.go | 73 +--- coderd/x/chatd/subagent_internal_test.go | 5 +- coderd/x/chatd/title_override.go | 8 +- 14 files changed, 235 insertions(+), 497 deletions(-) diff --git a/coderd/x/chatd/ARCHITECTURE.md b/coderd/x/chatd/ARCHITECTURE.md index 0a0b3323bdc..f5255dae622 100644 --- a/coderd/x/chatd/ARCHITECTURE.md +++ b/coderd/x/chatd/ARCHITECTURE.md @@ -856,11 +856,11 @@ The generation goroutine supports: ##### Model call resolution -Every LLM client chatd builds comes out of a single pipeline, `Server.resolveModelCall` in `modelcall.go`. A caller describes the call with a `modelCallSpec`, built by a purpose-specific constructor such as `standardTurnSpec`, `titleChatSpec`, `compactionOverrideSpec`, or `computerUseSpec`, and receives a `resolvedModelCall`: a ready client plus the parsed call config, provider options (derived when the spec requests them), resolved provider/model identity, and route. The pipeline owns config selection (the chat's config, an explicitly selected row, or a fixed provider/model pair), `chat_model_configs.options` parsing, AI Gateway route resolution, client construction, the debug-recording wrap, and provider-option derivation. +TODO(PR author): Document the model-call resolver introduced in `modelcall.go`: -Callers keep only flow-specific policy: which config row to prefer, whether a resolution failure is a hard error or falls back to the chat model, plus prompts, tools, schemas, and timeouts. Per-flow envelope differences are declared on the spec rather than re-implemented at call sites; for example, summary and turn-status-label specs omit provider options, the standard turn defaults `MaxOutputTokens`, and the advisor re-derives its options after pinning its reasoning effort and output cap into the call config. - -Call envelopes are centralized the same way: `resolvedModelCall.newCall` and `newObjectCall` are the only production constructors of `fantasy.Call` and `fantasy.ObjectCall`. Flows that hand generation to another package pass a prebuilt template through its options (`chatloop.GenerateAssistantOptions.CallTemplate`, the compaction `SummaryCall`, `chatadvisor.RuntimeConfig.CallTemplate`); the downstream package copies the template and attaches the prompt and tools it owns. +- `Server.resolveModelCall` is the single pipeline from a `modelCallSpec` to a ready client plus call metadata (`resolvedModelCall`). It owns config selection, `chat_model_configs.options` parsing, route resolution, client construction, the debug-recording wrap, and provider-option derivation. +- Every call behaves the same: provider options are always derived, `MaxOutputTokens` always defaults, and debug recording is always on when chat debug is enabled. The spec carries only flow-specific inputs: the model source (chat config, explicit config row, or fixed provider/model pair for computer use), the requested reasoning effort, chatd-scoped route resolution for deployment-selected override models, and the active API key for AI Gateway transport. +- `resolvedModelCall.newCall` and `newObjectCall` are the only production constructors of `fantasy.Call` and `fantasy.ObjectCall`; flows pass prebuilt templates through options (`chatloop.GenerateAssistantOptions.CallTemplate`, the compaction `SummaryCall`, `chatadvisor.RuntimeConfig.CallTemplate`) and downstream packages copy the template and attach the prompt and tools they own. ##### Reasoning effort @@ -884,7 +884,9 @@ Request preparation reads the transport from the model instead of recomputing it The first two happen together in `chatprovider.ProviderOptionsForCall`, the only entry point in `chatprovider` that builds provider options for a call; it delegates transport-aware OpenAI conversion to `chatopenai.ProviderOptionsFromChatConfig`. Config conversion and effort injection cannot pick different option types because one function owns both. -Paths that build their own clients get a `Model` from the same pipeline (`resolveModelCall`, see [Model call resolution](#model-call-resolution)), including the compaction override, quick generation (used by turn status labels and debug models), and the advisor runtime. Within quick generation, only title generation converts the model config through `ProviderOptionsForCall`; the turn status label and chat summary paths deliberately send no provider options, because they are short structured calls that set their own output bounds. Debug recording replaces the wrapped client and preserves the resolved transport. Computer-use turns substitute a hardcoded default model that has no config of its own; it carries its own transport, so the chat model's `openai_config` does not follow it. +TODO(PR author): Update this paragraph for the model-call resolver. All client-building paths (compaction override, quick generation, advisor runtime) now go through `resolveModelCall`, and every path derives provider options through `ProviderOptionsForCall`; the previous exceptions for turn status labels and chat summaries are gone. Computer-use turns still substitute a hardcoded default model that has no config of its own; it carries its own transport, so the chat model's `openai_config` does not follow it. + +Paths that build their own clients get a `Model` from the same constructor, including the compaction override, quick generation (used by turn status labels and debug models), and the advisor runtime. Within quick generation, only title generation converts the model config through `ProviderOptionsForCall`; the turn status label and chat summary paths deliberately send no provider options, because they are short structured calls that set their own output bounds. Debug recording replaces the wrapped client and preserves the resolved transport. Computer-use turns substitute a hardcoded default model that has no config of its own; it carries its own transport, so the chat model's `openai_config` does not follow it. Azure is deliberately exempt: its provider always enables the Responses API for known models and exposes no equivalent per-model hook, so the transport keeps following the known-model list for Azure. Ignoring the override there is what keeps the decisions above in agreement with the Azure client. The exemption is narrower than it appears, because chatd never builds an azure-typed provider as a fantasy azure client: `fantasyConfigForAIBridge` folds every provider type other than anthropic, bedrock, and openai into openai-compat, which always speaks Chat Completions. diff --git a/coderd/x/chatd/advisor_internal_test.go b/coderd/x/chatd/advisor_internal_test.go index cdbb5cfbf12..20b07d8a40a 100644 --- a/coderd/x/chatd/advisor_internal_test.go +++ b/coderd/x/chatd/advisor_internal_test.go @@ -445,7 +445,9 @@ func TestResolveAdvisorModelOverride(t *testing.T) { require.True(t, gotModel.Valid()) require.Equal(t, "openai", gotModel.Provider()) require.Equal(t, "gpt-5.2", gotModel.ModelID()) - require.Equal(t, fallbackCallConfig, gotCfg) + require.Equal(t, codersdk.ChatModelCallConfig{ + MaxOutputTokens: ptr.Ref(defaultChatMaxOutputTokens), + }, gotCfg) }) } diff --git a/coderd/x/chatd/chatd.go b/coderd/x/chatd/chatd.go index a4989a61918..0e6b938028e 100644 --- a/coderd/x/chatd/chatd.go +++ b/coderd/x/chatd/chatd.go @@ -278,7 +278,12 @@ func (p *Server) resolveAdvisorModelOverride( return fallback, nil } - resolved, err := p.resolveModelCall(ctx, advisorOverrideSpec(chat, overrideConfig, modelOpts)) + resolved, err := p.resolveModelCall(ctx, modelCallSpec{ + purpose: "advisor", + chat: chat, + explicitConfig: &overrideConfig, + buildOptions: modelOpts, + }) if err != nil { // Malformed options always fall back; route and client errors are // hard failures only when the config has a linked provider. @@ -2498,17 +2503,14 @@ func (p *Server) generateManualTitleCandidate( } titleCtx := ctx - titleModel := resolved.model finishDebugRun := func(error) {} - if debugSvc := p.debugService(); debugSvc != nil && debugSvc.IsEnabled(ctx, chat.ID, chat.OwnerID) { - titleCtx, titleModel, finishDebugRun = p.prepareManualTitleDebugRun( + if resolved.debugEnabled { + titleCtx, finishDebugRun = p.prepareManualTitleDebugRun( ctx, - debugSvc, + p.debugService(), chat, - resolved.dbConfig, - modelOpts, + resolved, messages, - resolved.model, ) } @@ -2516,7 +2518,7 @@ func (p *Server) generateManualTitleCandidate( titleCtx, messages, pasteText, - titleModel.LanguageModel(), + resolved.model.LanguageModel(), titleObjectCall(resolved), ) finishDebugRun(err) @@ -2565,50 +2567,12 @@ func (p *Server) prepareManualTitleDebugRun( ctx context.Context, debugSvc *chatdebug.Service, chat database.Chat, - modelConfig database.ChatModelConfig, - modelOpts modelBuildOptions, + resolved resolvedModelCall, messages []database.ChatMessage, - fallbackModel chatprovider.Model, -) (context.Context, chatprovider.Model, func(error)) { +) (context.Context, func(error)) { titleCtx := ctx - titleModel := fallbackModel finishDebugRun := func(error) {} - - route, routeErr := p.resolveModelRouteForConfig(ctx, chat.OwnerID, modelConfig) - var routeProvider string - if routeErr == nil { - routeProvider = string(route.Provider.Type) - } else if modelConfig.AIProviderID.Valid { - // Route resolution failed, but the linked provider still identifies the - // type for the debug run record. Best-effort: leave empty if disabled. - if provider, err := p.enabledAIProviderByID(ctx, modelConfig.AIProviderID.UUID); err == nil { - routeProvider = string(provider.Type) - } - } - var debugModelErr error - var debugModel chatprovider.Model - if routeErr != nil { - debugModelErr = routeErr - } else { - var debugResolved resolvedModelCall - debugResolved, debugModelErr = p.resolveModelCall(ctx, manualTitleDebugSpec(chat, modelConfig, route, debugSvc, routeProvider, modelOpts)) - debugModel = debugResolved.model - } - switch { - case debugModelErr != nil: - p.logger.Warn(ctx, "failed to create debug-aware manual title model", - slog.F("chat_id", chat.ID), - slog.F("model", modelConfig.Model), - slog.Error(debugModelErr), - ) - case !debugModel.Valid(): - p.logger.Warn(ctx, "manual title debug model creation returned nil", - slog.F("chat_id", chat.ID), - slog.F("model", modelConfig.Model), - ) - default: - titleModel = debugModel - } + modelConfig := resolved.dbConfig var historyTipMessageID int64 if len(messages) > 0 { @@ -2636,7 +2600,7 @@ func (p *Server) prepareManualTitleDebugRun( debugRun, createRunErr := debugSvc.CreateRun(createRunCtx, chatdebug.CreateRunParams{ ChatID: chat.ID, ModelConfigID: modelConfig.ID, - Provider: routeProvider, + Provider: string(resolved.route.Provider.Type), Model: modelConfig.Model, Kind: chatdebug.KindTitleGeneration, Status: chatdebug.StatusInProgress, @@ -2651,7 +2615,7 @@ func (p *Server) prepareManualTitleDebugRun( slog.F("model", modelConfig.Model), slog.Error(createRunErr), ) - return titleCtx, titleModel, finishDebugRun + return titleCtx, finishDebugRun } runContext := chatdebugRunContext(debugRun) @@ -2671,7 +2635,7 @@ func (p *Server) prepareManualTitleDebugRun( } } - return titleCtx, titleModel, finishDebugRun + return titleCtx, finishDebugRun } func chatdebugRunContext(run database.ChatDebugRun) chatdebug.RunContext { @@ -2767,7 +2731,12 @@ func (p *Server) resolveManualTitleModel( return p.resolveFallbackManualTitleModel(ctx, chat, modelOpts) } - resolved, err := p.resolveModelCall(ctx, manualTitleSpec(chat, config, modelOpts)) + resolved, err := p.resolveModelCall(ctx, modelCallSpec{ + purpose: "title", + chat: chat, + explicitConfig: &config, + buildOptions: modelOpts, + }) if err != nil { p.logger.Debug(ctx, "manual title preferred model unavailable", slog.F("chat_id", chat.ID), @@ -2791,7 +2760,12 @@ func (p *Server) resolveFallbackManualTitleModel( err, ) } - resolved, err := p.resolveModelCall(ctx, manualTitleSpec(chat, config, modelOpts)) + resolved, err := p.resolveModelCall(ctx, modelCallSpec{ + purpose: "title", + chat: chat, + explicitConfig: &config, + buildOptions: modelOpts, + }) if err != nil { return resolvedModelCall{}, xerrors.Errorf( "create fallback manual title model: %w", @@ -3376,7 +3350,6 @@ type runChatResult struct { FinalAssistantText string // StatusLabelCall is nil when status-label model resolution failed. StatusLabelCall *resolvedModelCall - ModelBuildOptions modelBuildOptions TriggerMessageID int64 HistoryTipMessageID int64 } @@ -4500,13 +4473,12 @@ func (p *Server) generateFinalTurnStatusLabel( return fallbackTurnStatusLabel(status) } - statusLabel := p.generateTurnStatusLabel( + statusLabel := generateTurnStatusLabel( ctx, chat, status, assistantText, *runResult.StatusLabelCall, - runResult.ModelBuildOptions, logger, p.existingDebugService(), runResult.TriggerMessageID, @@ -4751,7 +4723,11 @@ func (p *Server) resolveChatSummaryModel( chat database.Chat, modelOpts modelBuildOptions, ) (resolvedModelCall, bool) { - resolved, err := p.resolveModelCall(ctx, chatModelSpec("chat_summary", chat, modelOpts)) + resolved, err := p.resolveModelCall(ctx, modelCallSpec{ + purpose: "chat_summary", + chat: chat, + buildOptions: modelOpts, + }) if err != nil { logger.Debug(ctx, "failed to resolve chat model for summary", slog.F("chat_id", chat.ID), slog.Error(err)) diff --git a/coderd/x/chatd/chatd_internal_test.go b/coderd/x/chatd/chatd_internal_test.go index 4d16d0b5f3a..00b2ecde400 100644 --- a/coderd/x/chatd/chatd_internal_test.go +++ b/coderd/x/chatd/chatd_internal_test.go @@ -31,12 +31,10 @@ import ( coderdpubsub "github.com/coder/coder/v2/coderd/pubsub" "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/workspacestats" - "github.com/coder/coder/v2/coderd/x/chatd/chatdebug" "github.com/coder/coder/v2/coderd/x/chatd/chatloop" openaicomputeruse "github.com/coder/coder/v2/coderd/x/chatd/chatopenai/computeruse" "github.com/coder/coder/v2/coderd/x/chatd/chatprompt" "github.com/coder/coder/v2/coderd/x/chatd/chatprovider" - "github.com/coder/coder/v2/coderd/x/chatd/chattest" "github.com/coder/coder/v2/coderd/x/chatd/chattool" skillspkg "github.com/coder/coder/v2/coderd/x/skills" "github.com/coder/coder/v2/codersdk" @@ -3657,74 +3655,6 @@ func TestServer_inflightContext(t *testing.T) { } } -// TestPrepareManualTitleDebugRun_RouteFailureDerivesProviderFromConfig drives -// the fallback branch in prepareManualTitleDebugRun: AI-gateway route -// resolution fails (the BYOK key lookup returns a non-ErrNoRows error) while -// the linked provider stays enabled, so the debug run records the provider -// type derived from modelConfig.AIProviderID instead of an empty string. -func TestPrepareManualTitleDebugRun_RouteFailureDerivesProviderFromConfig(t *testing.T) { - t.Parallel() - - ctx := testutil.Context(t, testutil.WaitShort) - ctrl := gomock.NewController(t) - db := dbmock.NewMockStore(ctrl) - logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) - - ownerID := uuid.New() - providerID := uuid.New() - chat := database.Chat{ID: uuid.New(), OwnerID: ownerID} - modelConfig := database.ChatModelConfig{ - ID: uuid.New(), - Model: "claude-sonnet-4", - AIProviderID: uuid.NullUUID{UUID: providerID, Valid: true}, - } - provider := database.AIProvider{ - ID: providerID, - Type: database.AIProviderTypeAnthropic, - Name: "anthropic", - Enabled: true, - } - - // Resolved twice: once by gatewayProviderForConfig during route resolution, - // once by the fallback's own enabledAIProviderByID lookup. - db.EXPECT().GetAIProviderByID(gomock.Any(), providerID).Return(provider, nil).AnyTimes() - // A non-ErrNoRows BYOK error fails route resolution while the provider stays - // enabled, which is exactly the gap the fallback covers. - db.EXPECT().GetUserAIProviderKeyByProviderID(gomock.Any(), database.GetUserAIProviderKeyByProviderIDParams{ - UserID: ownerID, - AIProviderID: providerID, - }).Return(database.UserAIProviderKey{}, sql.ErrConnDone) - - var gotProvider sql.NullString - db.EXPECT().InsertChatDebugRun(gomock.Any(), gomock.Any()).DoAndReturn( - func(_ context.Context, params database.InsertChatDebugRunParams) (database.ChatDebugRun, error) { - gotProvider = params.Provider - return database.ChatDebugRun{ChatID: params.ChatID, Provider: params.Provider}, nil - }, - ) - - server := &Server{ - db: db, - logger: logger, - allowBYOK: true, - } - debugSvc := chatdebug.NewService(db, logger, nil) - fallbackModel := chatprovider.NewModel(&chattest.FakeModel{ProviderName: "stub", ModelName: "stub"}, nil) - - server.prepareManualTitleDebugRun( - ctx, - debugSvc, - chat, - modelConfig, - modelBuildOptions{}, - nil, - fallbackModel, - ) - - require.True(t, gotProvider.Valid, "debug run provider should be populated from the linked config") - require.Equal(t, "anthropic", gotProvider.String) -} - // TestResolveFallbackModelConfigID verifies that admission does not reuse // a disabled last model and rejects a disabled default. func TestResolveFallbackModelConfigID(t *testing.T) { diff --git a/coderd/x/chatd/compaction_override_internal_test.go b/coderd/x/chatd/compaction_override_internal_test.go index bc810bd2817..50a69adb15b 100644 --- a/coderd/x/chatd/compaction_override_internal_test.go +++ b/coderd/x/chatd/compaction_override_internal_test.go @@ -169,11 +169,13 @@ func TestCompactionOverride_SetUsable(t *testing.T) { require.NotNil(t, resolved) require.Equal(t, overrideConfig.ID, resolved.Config.ID) - override, err := server.resolveModelCall(ctx, compactionOverrideSpec( - chat, - resolved.Config, - modelBuildOptions{ActiveAPIKeyID: uuid.NewString()}, - )) + override, err := server.resolveModelCall(ctx, modelCallSpec{ + purpose: "compaction", + chat: chat, + explicitConfig: &resolved.Config, + chatdScopedRoute: true, + buildOptions: modelBuildOptions{ActiveAPIKeyID: uuid.NewString()}, + }) require.NoError(t, err) require.True(t, override.model.Valid()) require.Equal(t, overrideConfig.ID, override.dbConfig.ID) diff --git a/coderd/x/chatd/generation.go b/coderd/x/chatd/generation.go index 8ff9890159d..b7f03424d9a 100644 --- a/coderd/x/chatd/generation.go +++ b/coderd/x/chatd/generation.go @@ -918,7 +918,13 @@ func (s *taskStarter) generateCompaction( metricProvider, metricModel := compactionMetricIdentity(prepared.Compaction) if override := prepared.Compaction.Override; override != nil { // A usable override that fails to build is a hard generation failure. - overrideModel, err := s.server.resolveModelCall(ctx, compactionOverrideSpec(prepared.Chat, override.Config, prepared.ModelBuildOptions)) + overrideModel, err := s.server.resolveModelCall(ctx, modelCallSpec{ + purpose: "compaction", + chat: prepared.Chat, + explicitConfig: &override.Config, + chatdScopedRoute: true, + buildOptions: prepared.ModelBuildOptions, + }) if err != nil { return xerrors.Errorf("build compaction model override: %w", err) } @@ -930,7 +936,7 @@ func (s *taskStarter) generateCompaction( compactionOpts.ResolvedProvider = overrideModel.resolvedProvider compactionOpts.ResolvedModel = overrideModel.resolvedModel compactionOpts.ModelConfigID = overrideModel.dbConfig.ID - compactionOpts.SummaryCall = compactionSummaryCall(overrideModel.providerOptions) + compactionOpts.SummaryCall = compactionSummaryCall(overrideModel) compactionOpts.Messages = sanitizeCompactionPrompt( ctx, logger, diff --git a/coderd/x/chatd/generation_preparer.go b/coderd/x/chatd/generation_preparer.go index 60c2c69d5c1..47f117444c2 100644 --- a/coderd/x/chatd/generation_preparer.go +++ b/coderd/x/chatd/generation_preparer.go @@ -111,7 +111,13 @@ func (server *Server) prepareGeneration( } modelOpts := modelBuildOptions{ActiveAPIKeyID: apiKeyID} - resolved, err := server.resolveModelCall(ctx, standardTurnSpec(chat, modelOpts)) + requestedEffort := chatRequestedEffort(chat) + resolved, err := server.resolveModelCall(ctx, modelCallSpec{ + purpose: "standard_turn", + chat: chat, + requestedEffort: requestedEffort, + buildOptions: modelOpts, + }) if err != nil { return generationPrepared{}, err } @@ -131,13 +137,17 @@ func (server *Server) prepareGeneration( if err != nil { return generationPrepared{}, xerrors.Errorf("resolve computer use provider and model: %w", err) } - cuResolved, cuErr := server.resolveModelCall(ctx, computerUseSpec( - chat, - cuModelProvider, - cuModelName, - resolved.callConfig, - modelOpts, - )) + cuResolved, cuErr := server.resolveModelCall(ctx, modelCallSpec{ + purpose: "computer_use", + chat: chat, + fixedModel: &fixedModelCall{ + providerType: cuModelProvider, + modelName: cuModelName, + callConfig: resolved.callConfig, + }, + requestedEffort: requestedEffort, + buildOptions: modelOpts, + }) if cuErr != nil { return generationPrepared{}, xerrors.Errorf( "resolve computer use model for provider %q model %q: %w", @@ -632,8 +642,7 @@ func (server *Server) prepareGeneration( ResolvedModel: resolved.resolvedModel, ModelConfigID: modelConfig.ID, StepUsage: compactionStepUsage, - // The chat-model summary historically sends no provider options. - SummaryCall: compactionSummaryCall(nil), + SummaryCall: compactionSummaryCall(resolved), } // workspaceCtx.currentChatSnapshot may carry a freshly persisted @@ -783,7 +792,11 @@ func (server *Server) deriveFinalTurnRunResult( return runChatResult{FinalAssistantText: finalAssistantText, TriggerMessageID: triggerMessageID, HistoryTipMessageID: historyTipMessageID} } modelOpts := modelBuildOptions{ActiveAPIKeyID: apiKeyID} - resolved, err := server.resolveModelCall(ctx, chatModelSpec("turn_status_label", chat, modelOpts)) + resolved, err := server.resolveModelCall(ctx, modelCallSpec{ + purpose: "turn_status_label", + chat: chat, + buildOptions: modelOpts, + }) if err != nil { // Preserve the text and IDs for the generic-label fallback. logger.Warn(ctx, "derive final turn status label: resolve model", slog.Error(err)) @@ -797,7 +810,6 @@ func (server *Server) deriveFinalTurnRunResult( return runChatResult{ FinalAssistantText: finalAssistantText, StatusLabelCall: &resolved, - ModelBuildOptions: modelOpts, TriggerMessageID: triggerMessageID, HistoryTipMessageID: historyTipMessageID, } diff --git a/coderd/x/chatd/generation_preparer_internal_test.go b/coderd/x/chatd/generation_preparer_internal_test.go index 5a139ea5b7d..651ad17bbf4 100644 --- a/coderd/x/chatd/generation_preparer_internal_test.go +++ b/coderd/x/chatd/generation_preparer_internal_test.go @@ -171,9 +171,13 @@ func TestPrepareGenerationClampsRequestedReasoningEffortToMax(t *testing.T) { require.Equal(t, defaultChatMaxOutputTokens, *prepared.CallTemplate.MaxOutputTokens) require.NotNil(t, prepared.Compaction) - require.Nil(t, prepared.Compaction.Options.SummaryCall.ProviderOptions) - require.NotNil(t, prepared.Compaction.Options.SummaryCall.ToolChoice) - require.Equal(t, fantasy.ToolChoiceNone, *prepared.Compaction.Options.SummaryCall.ToolChoice) + summaryCall := prepared.Compaction.Options.SummaryCall + require.Equal(t, prepared.CallTemplate.ProviderOptions, summaryCall.ProviderOptions) + require.NotNil(t, summaryCall.ToolChoice) + require.Equal(t, fantasy.ToolChoiceNone, *summaryCall.ToolChoice) + // Non-streaming summaries must not inherit the default output cap the + // Anthropic SDK rejects. + require.Nil(t, summaryCall.MaxOutputTokens) } func TestPrepareGenerationComputerUseIgnoresChatTransportOverride(t *testing.T) { diff --git a/coderd/x/chatd/model_routing_internal_test.go b/coderd/x/chatd/model_routing_internal_test.go index a0b80011553..76c41f14b08 100644 --- a/coderd/x/chatd/model_routing_internal_test.go +++ b/coderd/x/chatd/model_routing_internal_test.go @@ -631,28 +631,37 @@ func TestAIBridgeGatewayProviderTypesPreserveSlashModelID(t *testing.T) { } } +func computerUseTestServer(t *testing.T, factory *aibridgeTestFactory) *Server { + t.Helper() + ctrl := gomock.NewController(t) + db := dbmock.NewMockStore(ctrl) + db.EXPECT().GetAIProviders(gomock.Any(), gomock.Any()).Return([]database.AIProvider{ + aibridgeTestAIProvider(uuid.New(), "primary-openai", database.AIProviderTypeOpenai), + }, nil).AnyTimes() + return &Server{db: db, aibridgeTransportFactory: aibridgeTestFactoryPointer(factory)} +} + func TestAIBridgeComputerUseModelUsesRoute(t *testing.T) { t.Parallel() - providerID := uuid.New() apiKeyID := uuid.NewString() factory := &aibridgeTestFactory{rt: roundTripFunc(func(*http.Request) (*http.Response, error) { t.Fatal("computer use model construction must not send a request") return nil, xerrors.New("unreachable") })} chat := database.Chat{ID: uuid.New(), OwnerID: uuid.New()} - server := &Server{ - aibridgeTransportFactory: aibridgeTestFactoryPointer(factory), - } + server := computerUseTestServer(t, factory) provider := codersdk.ChatComputerUseProviderOpenAI modelProvider, modelName, ok := chattool.DefaultComputerUseModel(provider) require.True(t, ok) ctx := aibridge.WithDelegatedAPIKeyID(t.Context(), "context-key-must-be-ignored") - spec := computerUseSpec(chat, modelProvider, modelName, codersdk.ChatModelCallConfig{}, modelBuildOptions{ActiveAPIKeyID: apiKeyID}) - route := aibridgeTestRoute(aibridgeTestAIProvider(providerID, "primary-openai", database.AIProviderTypeOpenai)) - spec.routeOverride = &route - resolved, err := server.resolveModelCall(ctx, spec) + resolved, err := server.resolveModelCall(ctx, modelCallSpec{ + purpose: "computer_use", + chat: chat, + fixedModel: &fixedModelCall{providerType: modelProvider, modelName: modelName}, + buildOptions: modelBuildOptions{ActiveAPIKeyID: apiKeyID}, + }) require.NoError(t, err) require.True(t, resolved.model.Valid()) require.False(t, resolved.debugEnabled) @@ -669,22 +678,23 @@ func TestAIBridgeComputerUseModelUsesRoute(t *testing.T) { func TestComputerUseModelCall_TransportIndependentOfChatConfig(t *testing.T) { t.Parallel() - providerID := uuid.New() factory := &aibridgeTestFactory{rt: roundTripFunc(func(*http.Request) (*http.Response, error) { t.Fatal("computer use model construction must not send a request") return nil, xerrors.New("unreachable") })} chat := database.Chat{ID: uuid.New(), OwnerID: uuid.New()} - server := &Server{aibridgeTransportFactory: aibridgeTestFactoryPointer(factory)} + server := computerUseTestServer(t, factory) provider := codersdk.ChatComputerUseProviderOpenAI modelProvider, modelName, ok := chattool.DefaultComputerUseModel(provider) require.True(t, ok) - spec := computerUseSpec(chat, modelProvider, modelName, codersdk.ChatModelCallConfig{}, modelBuildOptions{ActiveAPIKeyID: uuid.NewString()}) - route := aibridgeTestRoute(aibridgeTestAIProvider(providerID, "primary-openai", database.AIProviderTypeOpenai)) - spec.routeOverride = &route - resolved, err := server.resolveModelCall(t.Context(), spec) + resolved, err := server.resolveModelCall(t.Context(), modelCallSpec{ + purpose: "computer_use", + chat: chat, + fixedModel: &fixedModelCall{providerType: modelProvider, modelName: modelName}, + buildOptions: modelBuildOptions{ActiveAPIKeyID: uuid.NewString()}, + }) require.NoError(t, err) wantTransport := chatopenai.TransportFor(modelProvider, modelName, nil) @@ -699,23 +709,21 @@ func TestComputerUseModelCall_TransportIndependentOfChatConfig(t *testing.T) { func TestComputerUseModelCall_AIGatewayMissingAPIKeyID(t *testing.T) { t.Parallel() - providerID := uuid.New() factory := &aibridgeTestFactory{rt: roundTripFunc(func(*http.Request) (*http.Response, error) { t.Fatal("transport must not be used without an API key ID") return nil, xerrors.New("unreachable") })} chat := database.Chat{ID: uuid.New(), OwnerID: uuid.New()} - server := &Server{ - aibridgeTransportFactory: aibridgeTestFactoryPointer(factory), - } + server := computerUseTestServer(t, factory) provider := codersdk.ChatComputerUseProviderOpenAI modelProvider, modelName, ok := chattool.DefaultComputerUseModel(provider) require.True(t, ok) - spec := computerUseSpec(chat, modelProvider, modelName, codersdk.ChatModelCallConfig{}, modelBuildOptions{}) - route := aibridgeTestRoute(aibridgeTestAIProvider(providerID, "primary-openai", database.AIProviderTypeOpenai)) - spec.routeOverride = &route - resolved, err := server.resolveModelCall(t.Context(), spec) + resolved, err := server.resolveModelCall(t.Context(), modelCallSpec{ + purpose: "computer_use", + chat: chat, + fixedModel: &fixedModelCall{providerType: modelProvider, modelName: modelName}, + }) require.Error(t, err) require.False(t, resolved.model.Valid()) require.False(t, resolved.debugEnabled) diff --git a/coderd/x/chatd/modelcall.go b/coderd/x/chatd/modelcall.go index 6d7033729f1..ec9949d8db6 100644 --- a/coderd/x/chatd/modelcall.go +++ b/coderd/x/chatd/modelcall.go @@ -17,60 +17,27 @@ import ( const defaultChatMaxOutputTokens = int64(32_000) -type configSelectionMode int - -const ( - // configFromChat resolves the chat's last model config, falling back to - // the deployment default. The config must be enabled. - configFromChat configSelectionMode = iota - // configExplicit uses a config row the caller already selected (override - // and preferred-model flows own their selection and fallback policy). - configExplicit - // configFixedModel builds a client for a provider/model pair without a - // config row (computer use, debug transport rebuilds). - configFixedModel -) - -type configSelection struct { - mode configSelectionMode - config database.ChatModelConfig - providerType string - modelName string - configOptions []byte - callConfig codersdk.ChatModelCallConfig +// fixedModelCall selects a provider/model pair that has no config row of its +// own (computer use). +type fixedModelCall struct { + providerType string + modelName string + callConfig codersdk.ChatModelCallConfig } -type debugPolicy int - -const ( - debugPolicyOff debugPolicy = iota - // debugPolicyAware records only when chat debug is enabled. - debugPolicyAware - // debugPolicyForced records after the caller has enabled debugging. - debugPolicyForced -) - -// modelCallSpec declares config, routing, and construction policy for one LLM -// call. Build it with a purpose-specific constructor. type modelCallSpec struct { // purpose labels resolver logs only; it does not affect call behavior. - purpose string - chat database.Chat - config configSelection + purpose string + chat database.Chat + explicitConfig *database.ChatModelConfig + fixedModel *fixedModelCall + // requestedEffort overrides the config's default reasoning effort. requestedEffort *string - - omitProviderOptions bool - debug debugPolicy - debugSvc *chatdebug.Service - debugWrapProvider string - debugWrapModel string - routeOverride *aiGatewayModelRoute // chatdScopedRoute resolves the route with chatd scope. Deployment-wide // override models must route for user-owned chats regardless of the // caller's actor. - chatdScopedRoute bool - defaultMaxOutputTokens bool - buildOptions modelBuildOptions + chatdScopedRoute bool + buildOptions modelBuildOptions } func chatRequestedEffort(chat database.Chat) *string { @@ -80,139 +47,6 @@ func chatRequestedEffort(chat database.Chat) *string { return new(string(chat.LastReasoningEffort.ChatReasoningEffort)) } -func standardTurnSpec(chat database.Chat, buildOpts modelBuildOptions) modelCallSpec { - return modelCallSpec{ - purpose: "standard_turn", - chat: chat, - config: configSelection{mode: configFromChat}, - requestedEffort: chatRequestedEffort(chat), - debug: debugPolicyAware, - defaultMaxOutputTokens: true, - buildOptions: buildOpts, - } -} - -// chatModelSpec preserves summary and status-label behavior: no provider -// options and no standard-turn token default. -func chatModelSpec(purpose string, chat database.Chat, buildOpts modelBuildOptions) modelCallSpec { - return modelCallSpec{ - purpose: purpose, - chat: chat, - config: configSelection{mode: configFromChat}, - omitProviderOptions: true, - debug: debugPolicyAware, - buildOptions: buildOpts, - } -} - -// Background title generation uses the config's default reasoning effort, not -// the user's per-turn choice. -func titleChatSpec(chat database.Chat, buildOpts modelBuildOptions) modelCallSpec { - return modelCallSpec{ - purpose: "title", - chat: chat, - config: configSelection{mode: configFromChat}, - debug: debugPolicyAware, - buildOptions: buildOpts, - } -} - -// titleOverrideSpec uses chatd scope so owners need not have provider read -// access. -func titleOverrideSpec(chat database.Chat, config database.ChatModelConfig, buildOpts modelBuildOptions) modelCallSpec { - return modelCallSpec{ - purpose: "title", - chat: chat, - config: configSelection{mode: configExplicit, config: config}, - chatdScopedRoute: true, - buildOptions: buildOpts, - } -} - -// manualTitleSpec leaves debug instrumentation to a separate rebuild to -// preserve manual-title behavior. -func manualTitleSpec(chat database.Chat, config database.ChatModelConfig, buildOpts modelBuildOptions) modelCallSpec { - return modelCallSpec{ - purpose: "title", - chat: chat, - config: configSelection{mode: configExplicit, config: config}, - buildOptions: buildOpts, - } -} - -// compactionOverrideSpec receives a config with resolved reasoning effort and -// uses chatd scope so owners need not have provider read access. -func compactionOverrideSpec(chat database.Chat, config database.ChatModelConfig, buildOpts modelBuildOptions) modelCallSpec { - return modelCallSpec{ - purpose: "compaction", - chat: chat, - config: configSelection{mode: configExplicit, config: config}, - debug: debugPolicyAware, - chatdScopedRoute: true, - buildOptions: buildOpts, - } -} - -// advisorOverrideSpec omits provider options until the advisor pins its -// reasoning effort and output cap. -func advisorOverrideSpec(chat database.Chat, config database.ChatModelConfig, buildOpts modelBuildOptions) modelCallSpec { - return modelCallSpec{ - purpose: "advisor", - chat: chat, - config: configSelection{mode: configExplicit, config: config}, - omitProviderOptions: true, - buildOptions: buildOpts, - } -} - -// manualTitleDebugSpec preserves the resolved route and caller-selected -// attribution labels while enabling HTTP recording. -func manualTitleDebugSpec( - chat database.Chat, - config database.ChatModelConfig, - route aiGatewayModelRoute, - debugSvc *chatdebug.Service, - routeProvider string, - buildOpts modelBuildOptions, -) modelCallSpec { - return modelCallSpec{ - purpose: "debug_rebuild", - chat: chat, - config: configSelection{mode: configExplicit, config: config}, - omitProviderOptions: true, - debug: debugPolicyForced, - debugSvc: debugSvc, - debugWrapProvider: routeProvider, - debugWrapModel: config.Model, - routeOverride: &route, - buildOptions: buildOpts, - } -} - -// computerUseSpec uses the chat config only for per-call options because the -// fixed computer-use model has no config row. -func computerUseSpec( - chat database.Chat, - modelProvider string, - modelName string, - chatCallConfig codersdk.ChatModelCallConfig, - buildOpts modelBuildOptions, -) modelCallSpec { - return modelCallSpec{ - purpose: "computer_use", - chat: chat, - config: configSelection{ - mode: configFixedModel, - providerType: modelProvider, - modelName: modelName, - callConfig: chatCallConfig, - }, - requestedEffort: chatRequestedEffort(chat), - debug: debugPolicyAware, - buildOptions: buildOpts, - } -} - // modelCallConfigParseError lets the advisor distinguish malformed options // from route and client failures when deciding whether to fall back. type modelCallConfigParseError struct{ err error } @@ -241,8 +75,14 @@ func (p *Server) resolveModelCall(ctx context.Context, spec modelCallSpec) (reso var modelName string var configOptions []byte - switch spec.config.mode { - case configFromChat: + switch { + case spec.fixedModel != nil: + modelName = spec.fixedModel.modelName + case spec.explicitConfig != nil: + out.dbConfig = *spec.explicitConfig + modelName = out.dbConfig.Model + configOptions = out.dbConfig.Options + default: dbConfig, err := p.resolveModelConfig(ctx, spec.chat) if err != nil { return resolvedModelCall{}, xerrors.Errorf("resolve model config: %w", err) @@ -253,51 +93,41 @@ func (p *Server) resolveModelCall(ctx context.Context, spec modelCallSpec) (reso out.dbConfig = dbConfig modelName = dbConfig.Model configOptions = dbConfig.Options - case configExplicit: - out.dbConfig = spec.config.config - modelName = out.dbConfig.Model - configOptions = out.dbConfig.Options - case configFixedModel: - modelName = spec.config.modelName - configOptions = spec.config.configOptions } - // clientCallConfig always comes from configOptions: it drives client - // construction (beta headers, OpenAI transport override), while - // out.callConfig drives per-call option derivation and can differ for - // configFixedModel (computer use derives options from the chat model). + // clientCallConfig drives client construction; out.callConfig drives + // per-call option derivation and comes from the chat model for computer + // use, whose fixed model has no config of its own. clientCallConfig, err := parseModelConfigOptions(configOptions) if err != nil { return resolvedModelCall{}, modelCallConfigParseError{err: err} } - if spec.config.mode == configFixedModel { - out.callConfig = spec.config.callConfig + if spec.fixedModel != nil { + out.callConfig = spec.fixedModel.callConfig } else { out.callConfig = clientCallConfig } - if spec.defaultMaxOutputTokens && out.callConfig.MaxOutputTokens == nil { + if out.callConfig.MaxOutputTokens == nil { out.callConfig.MaxOutputTokens = ptr.Ref(defaultChatMaxOutputTokens) } - if spec.routeOverride != nil { - out.route = *spec.routeOverride + routeCtx := ctx + if spec.chatdScopedRoute { + //nolint:gocritic // Deployment-wide override models need chatd-scoped provider reads for user-owned chats. + routeCtx = dbauthz.AsChatd(ctx) + } + if spec.fixedModel != nil { + out.route, err = p.resolveModelRouteForProviderType(routeCtx, spec.chat.OwnerID, spec.fixedModel.providerType) } else { - routeCtx := ctx - if spec.chatdScopedRoute { - //nolint:gocritic // Deployment-wide override models need chatd-scoped provider reads for user-owned chats. - routeCtx = dbauthz.AsChatd(ctx) - } - var err error - if spec.config.mode == configFixedModel { - out.route, err = p.resolveModelRouteForProviderType(routeCtx, spec.chat.OwnerID, spec.config.providerType) - } else { - out.route, err = p.resolveModelRouteForConfig(routeCtx, spec.chat.OwnerID, out.dbConfig) - } - if err != nil { - return resolvedModelCall{}, err - } + out.route, err = p.resolveModelRouteForConfig(routeCtx, spec.chat.OwnerID, out.dbConfig) + } + if err != nil { + return resolvedModelCall{}, err } + // The resolved identity feeds metadata, logs, and debug labels. The + // client is constructed with the configured model string so gateway + // validation sees the name exactly as configured. out.resolvedProvider, out.resolvedModel, err = chatprovider.ResolveModelWithProviderHint( modelName, out.route.ModelProviderHint, @@ -306,59 +136,33 @@ func (p *Server) resolveModelCall(ctx context.Context, spec modelCallSpec) (reso return resolvedModelCall{}, xerrors.Errorf("resolve model metadata: %w", err) } - debugSvc := spec.debugSvc - switch spec.debug { - case debugPolicyAware: - if debugSvc == nil { - debugSvc = p.debugService() - } - out.debugEnabled = debugSvc != nil && debugSvc.IsEnabled(ctx, spec.chat.ID, spec.chat.OwnerID) - case debugPolicyForced: - out.debugEnabled = true - case debugPolicyOff: - } - - clientModelName := modelName - clientRoute := out.route - if spec.debug == debugPolicyAware { - // Debug-aware calls preserve their historical use of the resolved identity; - // other flows pass the configured model name. - clientRoute.ModelProviderHint = out.resolvedProvider - clientModelName = out.resolvedModel - } + debugSvc := p.debugService() + out.debugEnabled = debugSvc != nil && debugSvc.IsEnabled(ctx, spec.chat.ID, spec.chat.OwnerID) buildOpts := spec.buildOptions buildOpts.RecordHTTP = out.debugEnabled model, err := p.newModel(ctx, modelClientRequest{ Chat: spec.chat, - ModelName: clientModelName, + ModelName: modelName, UserAgent: chatprovider.UserAgent(), ExtraHeaders: chatprovider.CoderHeaders(spec.chat), CallConfig: clientCallConfig, - }, clientRoute, buildOpts) + }, out.route, buildOpts) if err != nil { return resolvedModelCall{}, xerrors.Errorf("create model: %w", err) } - if out.debugEnabled && debugSvc != nil { - wrapProvider := out.resolvedProvider - wrapModel := out.resolvedModel - if spec.debug == debugPolicyForced { - wrapProvider = spec.debugWrapProvider - wrapModel = spec.debugWrapModel - } + if out.debugEnabled { model = model.WithLanguageModel(chatdebug.WrapModel(model.LanguageModel(), debugSvc, chatdebug.RecorderOptions{ ChatID: spec.chat.ID, OwnerID: spec.chat.OwnerID, - Provider: wrapProvider, - Model: wrapModel, + Provider: out.resolvedProvider, + Model: out.resolvedModel, })) } out.model = model - if !spec.omitProviderOptions { - out.providerOptions = out.deriveProviderOptions(out.callConfig, spec.requestedEffort) - } + out.providerOptions = out.deriveProviderOptions(out.callConfig, spec.requestedEffort) p.logger.Debug(ctx, "resolved model call", slog.F("purpose", spec.purpose), @@ -382,15 +186,16 @@ func (r resolvedModelCall) newCall() fantasy.Call { } } -// Compaction summaries omit sampling and output-token options. The chat-model -// summary passes nil provider options; the override-model summary passes its -// resolved options. -func compactionSummaryCall(providerOptions fantasy.ProviderOptions) fantasy.Call { +// compactionSummaryCall follows the resolved call template, except summaries +// must not call tools and must not carry the default output cap: the summary +// request is non-streaming, and the Anthropic SDK rejects non-streaming +// requests whose max_tokens implies a completion longer than ten minutes. +func compactionSummaryCall(resolved resolvedModelCall) fantasy.Call { + call := resolved.newCall() toolChoiceNone := fantasy.ToolChoiceNone - return fantasy.Call{ - ToolChoice: &toolChoiceNone, - ProviderOptions: providerOptions, - } + call.ToolChoice = &toolChoiceNone + call.MaxOutputTokens = nil + return call } // deriveProviderOptions is the only production ProviderOptionsForCall call @@ -404,6 +209,11 @@ func (r resolvedModelCall) newObjectCall(schemaName, schemaDescription string, m SchemaName: schemaName, SchemaDescription: schemaDescription, MaxOutputTokens: ptr.Ref(maxOutputTokens), + Temperature: r.callConfig.Temperature, + TopP: r.callConfig.TopP, + TopK: r.callConfig.TopK, + PresencePenalty: r.callConfig.PresencePenalty, + FrequencyPenalty: r.callConfig.FrequencyPenalty, ProviderOptions: r.providerOptions, } } diff --git a/coderd/x/chatd/modelcall_internal_test.go b/coderd/x/chatd/modelcall_internal_test.go index e969e75338c..c233b613ddf 100644 --- a/coderd/x/chatd/modelcall_internal_test.go +++ b/coderd/x/chatd/modelcall_internal_test.go @@ -4,6 +4,8 @@ import ( "encoding/json" "testing" + "charm.land/fantasy" + fantasyopenai "charm.land/fantasy/providers/openai" "github.com/google/uuid" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" @@ -29,7 +31,23 @@ func modelCallSentinelOptions(t *testing.T, user string) json.RawMessage { return raw } -func TestChatModelSpecOmitsProviderOptions(t *testing.T) { +// The transport decides which of the two OpenAI option shapes derivation +// produces, so both are accepted. +func requireOpenAIUserOption(t *testing.T, options fantasy.ProviderOptions, user string) { + t.Helper() + switch opts := options[fantasyopenai.Name].(type) { + case *fantasyopenai.ResponsesProviderOptions: + require.NotNil(t, opts.User) + require.Equal(t, user, *opts.User) + case *fantasyopenai.ProviderOptions: + require.NotNil(t, opts.User) + require.Equal(t, user, *opts.User) + default: + t.Fatalf("unexpected openai provider options type %T", opts) + } +} + +func TestResolveModelCallDerivesProviderOptions(t *testing.T) { t.Parallel() ctx := testutil.Context(t, testutil.WaitShort) @@ -51,8 +69,12 @@ func TestChatModelSpecOmitsProviderOptions(t *testing.T) { }}, nil).AnyTimes() server := titleOverrideTestServer(db, logger) - resolved, err := server.resolveModelCall(ctx, chatModelSpec("chat_summary", chat, modelBuildOptions{ActiveAPIKeyID: uuid.NewString()})) + resolved, err := server.resolveModelCall(ctx, modelCallSpec{ + purpose: "chat_summary", + chat: chat, + buildOptions: modelBuildOptions{ActiveAPIKeyID: uuid.NewString()}, + }) require.NoError(t, err) - require.Nil(t, resolved.providerOptions) - require.Nil(t, summaryObjectCall(resolved).ProviderOptions) + requireOpenAIUserOption(t, resolved.providerOptions, "summary-options-sentinel") + requireOpenAIUserOption(t, summaryObjectCall(resolved).ProviderOptions, "summary-options-sentinel") } diff --git a/coderd/x/chatd/quickgen.go b/coderd/x/chatd/quickgen.go index e158817b884..9a0ab481e99 100644 --- a/coderd/x/chatd/quickgen.go +++ b/coderd/x/chatd/quickgen.go @@ -144,33 +144,6 @@ type shortTextCandidate struct { resolved resolvedModelCall } -// quickgenDebugSpec preserves the candidate's route, client options, and -// attribution labels while enabling HTTP recording. -func quickgenDebugSpec( - chat database.Chat, - candidate shortTextCandidate, - debugSvc *chatdebug.Service, - buildOpts modelBuildOptions, -) modelCallSpec { - route := candidate.resolved.route - return modelCallSpec{ - purpose: "debug_rebuild", - chat: chat, - config: configSelection{ - mode: configFixedModel, - modelName: candidate.model, - configOptions: candidate.resolved.dbConfig.Options, - }, - omitProviderOptions: true, - debug: debugPolicyForced, - debugSvc: debugSvc, - debugWrapProvider: candidate.provider, - debugWrapModel: candidate.model, - routeOverride: &route, - buildOptions: buildOpts, - } -} - func selectPreferredConfiguredShortTextModelConfig( configs []database.GetEnabledChatModelConfigsRow, ) (database.ChatModelConfig, bool) { @@ -256,7 +229,11 @@ func (p *Server) GenerateChatTitleAsync(ctx context.Context, chat database.Chat) } modelOpts := modelBuildOptions{ActiveAPIKeyID: apiKeyID} turnCtx := titleCtx - fallback, err := p.resolveModelCall(turnCtx, titleChatSpec(chat, modelOpts)) + fallback, err := p.resolveModelCall(turnCtx, modelCallSpec{ + purpose: "title", + chat: chat, + buildOptions: modelOpts, + }) if err != nil { logger.Debug(titleCtx, "failed to resolve model for automatic title generation", slog.Error(err), @@ -363,15 +340,13 @@ func (p *Server) maybeGenerateChatTitle( ) candidateCtx := titleCtx - candidateModel := candidate.resolved.model finishDebugRun := func(error) {} if debugEnabled { - candidateCtx, candidateModel, finishDebugRun = p.prepareQuickgenDebugCandidate( + candidateCtx, finishDebugRun = prepareQuickgenDebugCandidate( titleCtx, chat, debugSvc, candidate, - modelOpts, chatdebug.KindTitleGeneration, triggerMessageID, historyTipMessageID, @@ -380,7 +355,7 @@ func (p *Server) maybeGenerateChatTitle( ) } - title, err := generateTitle(candidateCtx, candidateModel.LanguageModel(), titleObjectCall(candidate.resolved), input) + title, err := generateTitle(candidateCtx, candidate.resolved.model.LanguageModel(), titleObjectCall(candidate.resolved), input) finishDebugRun(err) if err != nil { if overrideSet { @@ -425,35 +400,18 @@ func titleObjectCall(resolved resolvedModelCall) fantasy.ObjectCall { return resolved.newObjectCall("propose_title", "Propose a short chat title.", titleMaxOutputTokens) } -func (p *Server) prepareQuickgenDebugCandidate( +func prepareQuickgenDebugCandidate( ctx context.Context, chat database.Chat, debugSvc *chatdebug.Service, candidate shortTextCandidate, - modelOpts modelBuildOptions, kind chatdebug.RunKind, triggerMessageID int64, historyTipMessageID int64, seedSummary map[string]any, logger slog.Logger, -) (context.Context, chatprovider.Model, func(error)) { +) (context.Context, func(error)) { finishDebugRun := func(error) {} - if debugSvc == nil { - return ctx, candidate.resolved.model, finishDebugRun - } - - debugResolved, err := p.resolveModelCall(ctx, quickgenDebugSpec(chat, candidate, debugSvc, modelOpts)) - if err != nil { - logger.Warn(ctx, "failed to build short-text debug model", - slog.F("chat_id", chat.ID), - slog.F("run_kind", kind), - slog.F("provider", candidate.provider), - slog.F("model", candidate.model), - slog.Error(err), - ) - return ctx, candidate.resolved.model, finishDebugRun - } - debugModel := debugResolved.model // Debug instrumentation must not eat into the quickgen budget // (30s titleCtx / summaryCtx on the caller). Detach and bound @@ -482,7 +440,7 @@ func (p *Server) prepareQuickgenDebugCandidate( slog.F("model", candidate.model), slog.Error(err), ) - return ctx, candidate.resolved.model, finishDebugRun + return ctx, finishDebugRun } runContext := chatdebugRunContext(run) @@ -503,7 +461,7 @@ func (p *Server) prepareQuickgenDebugCandidate( ) } } - return runCtx, debugModel, finishDebugRun + return runCtx, finishDebugRun } func quickgenPrompt(systemPrompt, userInput string) fantasy.Prompt { @@ -1267,13 +1225,12 @@ func turnStatusLabelObjectCall(resolved resolvedModelCall) fantasy.ObjectCall { } // generateTurnStatusLabel returns an empty string if generation fails. -func (p *Server) generateTurnStatusLabel( +func generateTurnStatusLabel( ctx context.Context, chat database.Chat, status database.ChatStatus, assistantText string, resolved resolvedModelCall, - modelOpts modelBuildOptions, logger slog.Logger, debugSvc *chatdebug.Service, triggerMessageID int64, @@ -1298,15 +1255,13 @@ func (p *Server) generateTurnStatusLabel( statusSeedSummary := chatdebug.SeedSummary("Turn status label") candidateCtx := labelCtx - candidateModel := candidate.resolved.model finishDebugRun := func(error) {} if debugEnabled { - candidateCtx, candidateModel, finishDebugRun = p.prepareQuickgenDebugCandidate( + candidateCtx, finishDebugRun = prepareQuickgenDebugCandidate( labelCtx, chat, debugSvc, candidate, - modelOpts, chatdebug.KindQuickgen, triggerMessageID, historyTipMessageID, @@ -1317,7 +1272,7 @@ func (p *Server) generateTurnStatusLabel( generatedLabel, err := generateStructuredTurnStatusLabel( candidateCtx, - candidateModel.LanguageModel(), + candidate.resolved.model.LanguageModel(), turnStatusLabelObjectCall(resolved), turnStatusLabelPrompt, input, diff --git a/coderd/x/chatd/subagent_internal_test.go b/coderd/x/chatd/subagent_internal_test.go index 290c43fb34e..fca8c14296d 100644 --- a/coderd/x/chatd/subagent_internal_test.go +++ b/coderd/x/chatd/subagent_internal_test.go @@ -539,7 +539,10 @@ func TestResolveChatModel_AIProviderDisabled(t *testing.T) { LastModelConfigID: modelConfig.ID, }) - resolved, err := server.resolveModelCall(ctx, standardTurnSpec(chat, modelBuildOptions{})) + resolved, err := server.resolveModelCall(ctx, modelCallSpec{ + purpose: "standard_turn", + chat: chat, + }) require.ErrorContains(t, err, "is disabled") require.Equal(t, resolvedModelCall{}, resolved) } diff --git a/coderd/x/chatd/title_override.go b/coderd/x/chatd/title_override.go index 4f2dfb92c42..e5214c30b3b 100644 --- a/coderd/x/chatd/title_override.go +++ b/coderd/x/chatd/title_override.go @@ -88,7 +88,13 @@ func (p *Server) resolveTitleGenerationModelOverride( } modelConfig = withResolvedReasoningEffort(modelConfig, overrideEffort) - resolved, err := p.resolveModelCall(ctx, titleOverrideSpec(chat, modelConfig, modelOpts)) + resolved, err := p.resolveModelCall(ctx, modelCallSpec{ + purpose: "title", + chat: chat, + explicitConfig: &modelConfig, + chatdScopedRoute: true, + buildOptions: modelOpts, + }) if err != nil { return resolvedModelCall{}, true, xerrors.Errorf( "create title generation model override: %w", From 0989a010b0838fb878e143f052a26f69e754a2b1 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Fri, 14 Aug 2026 17:47:25 +0000 Subject: [PATCH 13/13] docs(coderd/x/chatd): drop architecture prose made stale by the model-call resolver --- coderd/x/chatd/ARCHITECTURE.md | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/coderd/x/chatd/ARCHITECTURE.md b/coderd/x/chatd/ARCHITECTURE.md index f5255dae622..409ca071a18 100644 --- a/coderd/x/chatd/ARCHITECTURE.md +++ b/coderd/x/chatd/ARCHITECTURE.md @@ -854,14 +854,6 @@ The generation goroutine supports: - turn limit after a user message (the LLM shouldn't be able to spin forever in loop) - and other things -##### Model call resolution - -TODO(PR author): Document the model-call resolver introduced in `modelcall.go`: - -- `Server.resolveModelCall` is the single pipeline from a `modelCallSpec` to a ready client plus call metadata (`resolvedModelCall`). It owns config selection, `chat_model_configs.options` parsing, route resolution, client construction, the debug-recording wrap, and provider-option derivation. -- Every call behaves the same: provider options are always derived, `MaxOutputTokens` always defaults, and debug recording is always on when chat debug is enabled. The spec carries only flow-specific inputs: the model source (chat config, explicit config row, or fixed provider/model pair for computer use), the requested reasoning effort, chatd-scoped route resolution for deployment-selected override models, and the active API key for AI Gateway transport. -- `resolvedModelCall.newCall` and `newObjectCall` are the only production constructors of `fantasy.Call` and `fantasy.ObjectCall`; flows pass prebuilt templates through options (`chatloop.GenerateAssistantOptions.CallTemplate`, the compaction `SummaryCall`, `chatadvisor.RuntimeConfig.CallTemplate`) and downstream packages copy the template and attach the prompt and tools they own. - ##### Reasoning effort Model configs may carry a `reasoning_effort` config (`{default, max}`) inside `chat_model_configs.options`. Users select a per-turn effort when sending or editing a message; the value is stored on `chat_messages.reasoning_effort` and on `chat_queued_messages.reasoning_effort` for queued messages. Queued messages carry the value through promotion, and `chats.last_reasoning_effort` tracks the most recent message that set one, mirroring `last_model_config_id`. @@ -884,9 +876,7 @@ Request preparation reads the transport from the model instead of recomputing it The first two happen together in `chatprovider.ProviderOptionsForCall`, the only entry point in `chatprovider` that builds provider options for a call; it delegates transport-aware OpenAI conversion to `chatopenai.ProviderOptionsFromChatConfig`. Config conversion and effort injection cannot pick different option types because one function owns both. -TODO(PR author): Update this paragraph for the model-call resolver. All client-building paths (compaction override, quick generation, advisor runtime) now go through `resolveModelCall`, and every path derives provider options through `ProviderOptionsForCall`; the previous exceptions for turn status labels and chat summaries are gone. Computer-use turns still substitute a hardcoded default model that has no config of its own; it carries its own transport, so the chat model's `openai_config` does not follow it. - -Paths that build their own clients get a `Model` from the same constructor, including the compaction override, quick generation (used by turn status labels and debug models), and the advisor runtime. Within quick generation, only title generation converts the model config through `ProviderOptionsForCall`; the turn status label and chat summary paths deliberately send no provider options, because they are short structured calls that set their own output bounds. Debug recording replaces the wrapped client and preserves the resolved transport. Computer-use turns substitute a hardcoded default model that has no config of its own; it carries its own transport, so the chat model's `openai_config` does not follow it. +Debug recording replaces the wrapped client and preserves the resolved transport. Computer-use turns substitute a hardcoded default model that has no config of its own; it carries its own transport, so the chat model's `openai_config` does not follow it. Azure is deliberately exempt: its provider always enables the Responses API for known models and exposes no equivalent per-model hook, so the transport keeps following the known-model list for Azure. Ignoring the override there is what keeps the decisions above in agreement with the Azure client. The exemption is narrower than it appears, because chatd never builds an azure-typed provider as a fantasy azure client: `fantasyConfigForAIBridge` folds every provider type other than anthropic, bedrock, and openai into openai-compat, which always speaks Chat Completions.