fix: remove advisor reasoning configuration (#25030)

This commit is contained in:
Thomas Kosiewski
2026-05-07 15:19:19 +02:00
committed by GitHub
parent 8c08aa1f6c
commit 273e828442
12 changed files with 47 additions and 563 deletions
-8
View File
@@ -5104,14 +5104,6 @@ func (api *API) putChatAdvisorConfig(rw http.ResponseWriter, r *http.Request) {
})
return
}
switch req.ReasoningEffort {
case "", "low", "medium", "high":
default:
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
Message: fmt.Sprintf(`reasoning_effort %q is not valid; must be one of "", "low", "medium", or "high".`, req.ReasoningEffort),
})
return
}
if req.ModelConfigID != uuid.Nil {
// Use system context because GetChatModelConfigByID requires
// deployment-config read access, which can be broader than the
+24 -18
View File
@@ -11876,7 +11876,6 @@ func TestChatAdvisorConfig_Update(t *testing.T) {
Enabled: true,
MaxUsesPerRun: 5,
MaxOutputTokens: 1024,
ReasoningEffort: "high",
}
err := adminClient.UpdateChatAdvisorConfig(ctx, want)
@@ -11974,7 +11973,6 @@ func TestChatAdvisorConfig_RoundTripModelConfigID(t *testing.T) {
MaxUsesPerRun: 3,
MaxOutputTokens: 2048,
ModelConfigID: modelConfig.ID,
ReasoningEffort: "medium",
}
err := adminClient.UpdateChatAdvisorConfig(ctx, want)
@@ -11985,21 +11983,6 @@ func TestChatAdvisorConfig_RoundTripModelConfigID(t *testing.T) {
require.Equal(t, want, resp)
}
func TestChatAdvisorConfig_InvalidReasoningEffort(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitLong)
adminClient := newChatClient(t)
coderdtest.CreateFirstUser(t, adminClient.Client)
err := adminClient.UpdateChatAdvisorConfig(ctx, codersdk.UpdateAdvisorConfigRequest{
ReasoningEffort: "ultra",
})
sdkErr := requireSDKError(t, err, http.StatusBadRequest)
require.Contains(t, sdkErr.Message, `reasoning_effort "ultra"`)
require.Contains(t, sdkErr.Message, "not valid")
}
func TestChatAdvisorConfig_InvalidModelConfigID(t *testing.T) {
t.Parallel()
@@ -12055,7 +12038,6 @@ func TestChatAdvisorConfig_OverwriteClearsPreviousValues(t *testing.T) {
MaxUsesPerRun: 5,
MaxOutputTokens: 1024,
ModelConfigID: modelConfig.ID,
ReasoningEffort: "high",
}
err := adminClient.UpdateChatAdvisorConfig(ctx, rich)
require.NoError(t, err)
@@ -12124,6 +12106,30 @@ func TestChatAdvisorConfig_ClampsNegativeStoredValues(t *testing.T) {
require.JSONEq(t, stored, raw)
}
func TestChatAdvisorConfig_IgnoresLegacyReasoningEffort(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitLong)
adminClient, db := newChatClientWithDatabase(t)
coderdtest.CreateFirstUser(t, adminClient.Client)
stored := `{"enabled":true,"max_uses_per_run":3,"max_output_tokens":2048,"reasoning_effort":"high"}`
err := db.UpsertChatAdvisorConfig(dbauthz.AsSystemRestricted(ctx), stored)
require.NoError(t, err)
resp, err := adminClient.GetChatAdvisorConfig(ctx)
require.NoError(t, err)
require.Equal(t, codersdk.AdvisorConfig{
Enabled: true,
MaxUsesPerRun: 3,
MaxOutputTokens: 2048,
}, resp)
raw, err := db.GetChatAdvisorConfig(dbauthz.AsSystemRestricted(ctx))
require.NoError(t, err)
require.JSONEq(t, stored, raw)
}
// TestChatAdvisorConfig_CorruptStoredJSONReturnsError pins that the GET
// handler surfaces a 500 when the stored site_configs row contains bytes
// that are not valid JSON. Unlike the neighboring chat config endpoints,
-45
View File
@@ -8,7 +8,6 @@ import (
"time"
"charm.land/fantasy"
fantasyopenai "charm.land/fantasy/providers/openai"
"github.com/google/uuid"
"github.com/stretchr/testify/require"
"golang.org/x/xerrors"
@@ -412,48 +411,4 @@ func TestNewAdvisorRuntime(t *testing.T) {
require.Equal(t, int64(defaultAdvisorMaxOutputTokens), rt.MaxOutputTokens(),
"zero max output tokens must be replaced with defaultAdvisorMaxOutputTokens")
})
// Guards the wiring from AdvisorConfig.ReasoningEffort through
// newAdvisorRuntime to ApplyReasoningEffortToOptions. A field swap,
// typo, or accidental deletion of the apply call would otherwise
// ship silently because chatprovider_test only covers the helper in
// isolation.
t.Run("ReasoningEffortReachesProviderOptions", func(t *testing.T) {
t.Parallel()
ctx := testutil.Context(t, testutil.WaitShort)
store := &advisorOverrideStubStore{}
p := newAdvisorTestServer(ctx, t, store)
openAIModel := &chattest.FakeModel{
ProviderName: fantasyopenai.Name,
ModelName: "gpt-4",
}
rt := p.newAdvisorRuntime(
ctx,
database.Chat{},
codersdk.AdvisorConfig{
Enabled: true,
MaxUsesPerRun: 3,
MaxOutputTokens: 16384,
ReasoningEffort: "high",
},
openAIModel,
fallbackCallConfig,
chatprovider.ProviderAPIKeys{},
logger,
)
require.NotNil(t, rt)
providerOptions := rt.ProviderOptions()
require.NotNil(t, providerOptions,
"advisor runtime must seed provider options when reasoning effort is set")
opts, ok := providerOptions[fantasyopenai.Name].(*fantasyopenai.ResponsesProviderOptions)
require.True(t, ok,
"expected *ResponsesProviderOptions for Responses model, got %T",
providerOptions[fantasyopenai.Name])
require.NotNil(t, opts.ReasoningEffort,
"ReasoningEffort from AdvisorConfig must reach the provider options")
require.Equal(t, fantasyopenai.ReasoningEffortHigh, *opts.ReasoningEffort)
})
}
-10
View File
@@ -395,16 +395,6 @@ func (p *Server) newAdvisorRuntime(
advisorModel,
advisorCallConfig.ProviderOptions,
)
// ProviderOptionsFromChatModelConfig returns nil when the model config
// has no provider_options block, so the helper seeds a minimal entry
// for the advisor model's provider before applying reasoning_effort.
// This keeps the per-provider dispatch in chatprovider so adding a new
// provider there propagates here automatically.
providerOptions = chatprovider.ApplyReasoningEffortToOptions(
providerOptions,
advisorModel,
advisorCfg.ReasoningEffort,
)
rt, err := chatadvisor.NewRuntime(chatadvisor.RuntimeConfig{
Model: advisorModel,
-139
View File
@@ -701,145 +701,6 @@ func ReasoningEffortFromChat(provider string, value *string) *string {
}
}
// ApplyReasoningEffortToOptions applies the given reasoning_effort to every
// provider entry in providerOptions that understands it. When model is
// non-nil and the options map has no entry for the model's provider, this
// function seeds a minimal provider-specific options struct so the mutation
// still lands. Callers that produced providerOptions from a chat model
// config with no provider_options block would otherwise see
// reasoning_effort silently dropped.
//
// The returned map is the (possibly newly-allocated) providerOptions; the
// input is mutated in-place when non-nil.
func ApplyReasoningEffortToOptions(
providerOptions fantasy.ProviderOptions,
model fantasy.LanguageModel,
reasoningEffort string,
) fantasy.ProviderOptions {
reasoningEffort = strings.TrimSpace(reasoningEffort)
if reasoningEffort == "" {
return providerOptions
}
if model != nil {
providerOptions = seedProviderOptionsForModel(providerOptions, model)
}
if providerOptions == nil {
return nil
}
applyReasoningEffortDispatch(providerOptions, reasoningEffort)
return providerOptions
}
// seedProviderOptionsForModel ensures providerOptions has an entry for the
// given model's provider, allocating a minimal options struct when absent.
// Returns the possibly newly-allocated options map. Unknown providers are
// left untouched so callers get their input back unchanged.
func seedProviderOptionsForModel(
providerOptions fantasy.ProviderOptions,
model fantasy.LanguageModel,
) fantasy.ProviderOptions {
provider := model.Provider()
var seed fantasy.ProviderOptionsData
switch provider {
case fantasyopenai.Name:
if fantasyopenai.IsResponsesModel(model.Model()) {
seed = &fantasyopenai.ResponsesProviderOptions{}
} else {
seed = &fantasyopenai.ProviderOptions{}
}
case fantasyanthropic.Name:
seed = &fantasyanthropic.ProviderOptions{}
case fantasyopenaicompat.Name:
seed = &fantasyopenaicompat.ProviderOptions{}
case fantasyopenrouter.Name:
seed = &fantasyopenrouter.ProviderOptions{}
case fantasyvercel.Name:
seed = &fantasyvercel.ProviderOptions{}
default:
return providerOptions
}
if providerOptions == nil {
providerOptions = fantasy.ProviderOptions{}
}
if _, ok := providerOptions[provider]; !ok {
providerOptions[provider] = seed
}
return providerOptions
}
// applyReasoningEffortDispatch routes the normalized reasoning_effort to
// every provider entry present in providerOptions. Adding a new provider
// here (and only here) keeps chatd callers in sync automatically.
func applyReasoningEffortDispatch(
providerOptions fantasy.ProviderOptions,
reasoningEffort string,
) {
if normalized := ReasoningEffortFromChat(
fantasyopenai.Name,
&reasoningEffort,
); normalized != nil {
effort := fantasyopenai.ReasoningEffort(*normalized)
if raw, ok := providerOptions[fantasyopenai.Name]; ok {
switch opts := raw.(type) {
case *fantasyopenai.ProviderOptions:
opts.ReasoningEffort = &effort
case *fantasyopenai.ResponsesProviderOptions:
opts.ReasoningEffort = &effort
}
}
if raw, ok := providerOptions[fantasyopenaicompat.Name]; ok {
if opts, ok := raw.(*fantasyopenaicompat.ProviderOptions); ok {
opts.ReasoningEffort = &effort
}
}
}
if normalized := ReasoningEffortFromChat(
fantasyanthropic.Name,
&reasoningEffort,
); normalized != nil {
if raw, ok := providerOptions[fantasyanthropic.Name]; ok {
if opts, ok := raw.(*fantasyanthropic.ProviderOptions); ok {
effort := fantasyanthropic.Effort(*normalized)
opts.Effort = &effort
}
}
}
if normalized := ReasoningEffortFromChat(
fantasyopenrouter.Name,
&reasoningEffort,
); normalized != nil {
if raw, ok := providerOptions[fantasyopenrouter.Name]; ok {
if opts, ok := raw.(*fantasyopenrouter.ProviderOptions); ok {
if opts.Reasoning == nil {
opts.Reasoning = &fantasyopenrouter.ReasoningOptions{}
}
effort := fantasyopenrouter.ReasoningEffort(*normalized)
opts.Reasoning.Effort = &effort
}
}
}
if normalized := ReasoningEffortFromChat(
fantasyvercel.Name,
&reasoningEffort,
); normalized != nil {
if raw, ok := providerOptions[fantasyvercel.Name]; ok {
if opts, ok := raw.(*fantasyvercel.ProviderOptions); ok {
if opts.Reasoning == nil {
opts.Reasoning = &fantasyvercel.ReasoningOptions{}
}
effort := fantasyvercel.ReasoningEffort(*normalized)
opts.Reasoning.Effort = &effort
}
}
}
}
// MergeMissingModelCostConfig fills unset pricing metadata from defaults.
func MergeMissingModelCostConfig(
dst **codersdk.ModelCostConfig,
@@ -11,7 +11,6 @@ import (
fantasyanthropic "charm.land/fantasy/providers/anthropic"
fantasybedrock "charm.land/fantasy/providers/bedrock"
fantasyopenai "charm.land/fantasy/providers/openai"
fantasyopenaicompat "charm.land/fantasy/providers/openaicompat"
fantasyopenrouter "charm.land/fantasy/providers/openrouter"
fantasyvercel "charm.land/fantasy/providers/vercel"
"github.com/google/uuid"
@@ -1392,212 +1391,3 @@ func TestMergeMissingProviderOptions_OpenRouterNested(t *testing.T) {
require.Equal(t, []string{"int8"}, options.OpenRouter.Provider.Quantizations)
require.Equal(t, "latency", *options.OpenRouter.Provider.Sort)
}
// TestApplyReasoningEffortToOptions covers every provider's mutation branch
// plus the seeding path for missing provider entries. A typo or wrong type
// assertion in any branch fails a unit test here rather than silently
// dropping the admin-configured reasoning effort in chatd callers.
func TestApplyReasoningEffortToOptions(t *testing.T) {
t.Parallel()
t.Run("NilOptionsAndNilModelIsNoOp", func(t *testing.T) {
t.Parallel()
// Must not panic when options and model are both nil.
got := chatprovider.ApplyReasoningEffortToOptions(nil, nil, "medium")
require.Nil(t, got)
})
t.Run("EmptyEffortReturnsInputUnchanged", func(t *testing.T) {
t.Parallel()
model := &chattest.FakeModel{ProviderName: fantasyopenai.Name, ModelName: "gpt-4"}
got := chatprovider.ApplyReasoningEffortToOptions(nil, model, " ")
require.Nil(t, got)
})
t.Run("EmptyEffortPreservesExistingOptions", func(t *testing.T) {
t.Parallel()
effort := fantasyopenai.ReasoningEffortLow
opts := &fantasyopenai.ProviderOptions{ReasoningEffort: &effort}
providerOptions := fantasy.ProviderOptions{fantasyopenai.Name: opts}
got := chatprovider.ApplyReasoningEffortToOptions(providerOptions, nil, "")
require.NotNil(t, opts.ReasoningEffort)
require.Equal(t, fantasyopenai.ReasoningEffortLow, *opts.ReasoningEffort)
// The input map must be returned untouched rather than allocated anew.
require.Len(t, got, 1)
})
t.Run("UnrecognizedEffortLeavesOptionsUntouched", func(t *testing.T) {
t.Parallel()
opts := &fantasyopenai.ProviderOptions{}
providerOptions := fantasy.ProviderOptions{fantasyopenai.Name: opts}
chatprovider.ApplyReasoningEffortToOptions(providerOptions, nil, "not-a-real-effort")
require.Nil(t, opts.ReasoningEffort)
})
t.Run("OpenAIProviderOptions", func(t *testing.T) {
t.Parallel()
opts := &fantasyopenai.ProviderOptions{}
providerOptions := fantasy.ProviderOptions{fantasyopenai.Name: opts}
chatprovider.ApplyReasoningEffortToOptions(providerOptions, nil, "medium")
require.NotNil(t, opts.ReasoningEffort)
require.Equal(t, fantasyopenai.ReasoningEffortMedium, *opts.ReasoningEffort)
})
t.Run("OpenAIResponsesProviderOptions", func(t *testing.T) {
t.Parallel()
opts := &fantasyopenai.ResponsesProviderOptions{}
providerOptions := fantasy.ProviderOptions{fantasyopenai.Name: opts}
chatprovider.ApplyReasoningEffortToOptions(providerOptions, nil, "medium")
require.NotNil(t, opts.ReasoningEffort)
require.Equal(t, fantasyopenai.ReasoningEffortMedium, *opts.ReasoningEffort)
})
t.Run("OpenAICompatProviderOptions", func(t *testing.T) {
t.Parallel()
opts := &fantasyopenaicompat.ProviderOptions{}
providerOptions := fantasy.ProviderOptions{fantasyopenaicompat.Name: opts}
chatprovider.ApplyReasoningEffortToOptions(providerOptions, nil, "medium")
require.NotNil(t, opts.ReasoningEffort)
require.Equal(t, fantasyopenai.ReasoningEffortMedium, *opts.ReasoningEffort)
})
t.Run("AnthropicProviderOptions", func(t *testing.T) {
t.Parallel()
opts := &fantasyanthropic.ProviderOptions{}
providerOptions := fantasy.ProviderOptions{fantasyanthropic.Name: opts}
chatprovider.ApplyReasoningEffortToOptions(providerOptions, nil, "high")
require.NotNil(t, opts.Effort)
require.Equal(t, fantasyanthropic.EffortHigh, *opts.Effort)
})
t.Run("OpenRouterAllocatesReasoningOptions", func(t *testing.T) {
t.Parallel()
opts := &fantasyopenrouter.ProviderOptions{}
providerOptions := fantasy.ProviderOptions{fantasyopenrouter.Name: opts}
chatprovider.ApplyReasoningEffortToOptions(providerOptions, nil, "medium")
require.NotNil(t, opts.Reasoning, "Reasoning container must be allocated")
require.NotNil(t, opts.Reasoning.Effort)
require.Equal(t, fantasyopenrouter.ReasoningEffort("medium"), *opts.Reasoning.Effort)
})
t.Run("OpenRouterPreservesExistingReasoningContainer", func(t *testing.T) {
t.Parallel()
enabled := true
opts := &fantasyopenrouter.ProviderOptions{
Reasoning: &fantasyopenrouter.ReasoningOptions{Enabled: &enabled},
}
providerOptions := fantasy.ProviderOptions{fantasyopenrouter.Name: opts}
chatprovider.ApplyReasoningEffortToOptions(providerOptions, nil, "high")
require.NotNil(t, opts.Reasoning.Enabled)
require.True(t, *opts.Reasoning.Enabled)
require.NotNil(t, opts.Reasoning.Effort)
require.Equal(t, fantasyopenrouter.ReasoningEffort("high"), *opts.Reasoning.Effort)
})
t.Run("VercelAllocatesReasoningOptions", func(t *testing.T) {
t.Parallel()
opts := &fantasyvercel.ProviderOptions{}
providerOptions := fantasy.ProviderOptions{fantasyvercel.Name: opts}
chatprovider.ApplyReasoningEffortToOptions(providerOptions, nil, "minimal")
require.NotNil(t, opts.Reasoning)
require.NotNil(t, opts.Reasoning.Effort)
require.Equal(t, fantasyvercel.ReasoningEffortMinimal, *opts.Reasoning.Effort)
})
t.Run("MultipleProvidersReceiveMutations", func(t *testing.T) {
t.Parallel()
openaiOpts := &fantasyopenai.ProviderOptions{}
anthropicOpts := &fantasyanthropic.ProviderOptions{}
providerOptions := fantasy.ProviderOptions{
fantasyopenai.Name: openaiOpts,
fantasyanthropic.Name: anthropicOpts,
}
chatprovider.ApplyReasoningEffortToOptions(providerOptions, nil, "high")
require.NotNil(t, openaiOpts.ReasoningEffort)
require.Equal(t, fantasyopenai.ReasoningEffortHigh, *openaiOpts.ReasoningEffort)
require.NotNil(t, anthropicOpts.Effort)
require.Equal(t, fantasyanthropic.EffortHigh, *anthropicOpts.Effort)
})
t.Run("SeedsOpenAICompletionsWhenModelHasNoOptions", func(t *testing.T) {
t.Parallel()
// A model name absent from the Responses allowlist must seed
// the completions options struct so reasoning_effort lands.
model := &chattest.FakeModel{ProviderName: fantasyopenai.Name, ModelName: "not-a-real-openai-model"}
got := chatprovider.ApplyReasoningEffortToOptions(nil, model, "medium")
require.NotNil(t, got)
opts, ok := got[fantasyopenai.Name].(*fantasyopenai.ProviderOptions)
require.True(t, ok, "expected *ProviderOptions for non-Responses model, got %T", got[fantasyopenai.Name])
require.NotNil(t, opts.ReasoningEffort)
require.Equal(t, fantasyopenai.ReasoningEffortMedium, *opts.ReasoningEffort)
})
t.Run("SeedsOpenAIResponsesWhenModelIsResponsesModel", func(t *testing.T) {
t.Parallel()
// A model name in the Responses allowlist must seed the
// Responses-specific options struct so the provider routes to
// the Responses endpoint.
model := &chattest.FakeModel{ProviderName: fantasyopenai.Name, ModelName: "gpt-4"}
got := chatprovider.ApplyReasoningEffortToOptions(nil, model, "medium")
require.NotNil(t, got)
opts, ok := got[fantasyopenai.Name].(*fantasyopenai.ResponsesProviderOptions)
require.True(t, ok, "expected *ResponsesProviderOptions for Responses model, got %T", got[fantasyopenai.Name])
require.NotNil(t, opts.ReasoningEffort)
require.Equal(t, fantasyopenai.ReasoningEffortMedium, *opts.ReasoningEffort)
})
t.Run("SeedsAnthropicWhenModelHasNoOptions", func(t *testing.T) {
t.Parallel()
model := &chattest.FakeModel{ProviderName: fantasyanthropic.Name, ModelName: "claude-3-5"}
got := chatprovider.ApplyReasoningEffortToOptions(nil, model, "high")
require.NotNil(t, got)
opts, ok := got[fantasyanthropic.Name].(*fantasyanthropic.ProviderOptions)
require.True(t, ok)
require.NotNil(t, opts.Effort)
require.Equal(t, fantasyanthropic.EffortHigh, *opts.Effort)
})
t.Run("SeedsOpenRouterWhenModelHasNoOptions", func(t *testing.T) {
t.Parallel()
model := &chattest.FakeModel{ProviderName: fantasyopenrouter.Name, ModelName: "openrouter-x"}
got := chatprovider.ApplyReasoningEffortToOptions(nil, model, "low")
require.NotNil(t, got)
opts, ok := got[fantasyopenrouter.Name].(*fantasyopenrouter.ProviderOptions)
require.True(t, ok)
require.NotNil(t, opts.Reasoning)
require.NotNil(t, opts.Reasoning.Effort)
require.Equal(t, fantasyopenrouter.ReasoningEffort("low"), *opts.Reasoning.Effort)
})
t.Run("UnknownProviderReturnsInputUnchanged", func(t *testing.T) {
t.Parallel()
model := &chattest.FakeModel{ProviderName: "unknown", ModelName: "x"}
got := chatprovider.ApplyReasoningEffortToOptions(nil, model, "medium")
require.Nil(t, got)
})
t.Run("PreservesExistingProviderEntry", func(t *testing.T) {
t.Parallel()
existing := &fantasyopenai.ProviderOptions{}
existingEffort := fantasyopenai.ReasoningEffortLow
existing.ReasoningEffort = &existingEffort
providerOptions := fantasy.ProviderOptions{fantasyopenai.Name: existing}
model := &chattest.FakeModel{ProviderName: fantasyopenai.Name, ModelName: "gpt-4"}
got := chatprovider.ApplyReasoningEffortToOptions(providerOptions, model, "medium")
require.Same(t, existing, got[fantasyopenai.Name],
"existing provider entry must not be replaced")
// The reasoning effort on the existing entry is overwritten.
require.Equal(t, fantasyopenai.ReasoningEffortMedium, *existing.ReasoningEffort)
})
}