diff --git a/pkg/llm/llm.go b/pkg/llm/llm.go index f53c99d93..5dc761534 100644 --- a/pkg/llm/llm.go +++ b/pkg/llm/llm.go @@ -17,6 +17,7 @@ package llm import ( "context" "io" + "strings" "time" "go.gearno.de/kit/log" @@ -72,6 +73,8 @@ func NewClient(provider Provider, system string, opts ...Option) *Client { } func (c *Client) ChatCompletion(ctx context.Context, req *ChatCompletionRequest) (*ChatCompletionResponse, error) { + req = sanitizeRequest(ctx, c.logger, req) + ctx, span := startChatSpan(ctx, c.tracer, c.system, req) c.logger.InfoCtx( @@ -115,6 +118,8 @@ func (c *Client) ChatCompletion(ctx context.Context, req *ChatCompletionRequest) } func (c *Client) ChatCompletionStream(ctx context.Context, req *ChatCompletionRequest) (ChatCompletionStream, error) { + req = sanitizeRequest(ctx, c.logger, req) + ctx, span := startChatSpan(ctx, c.tracer, c.system, req) c.logger.InfoCtx( @@ -140,3 +145,69 @@ func (c *Client) ChatCompletionStream(ctx context.Context, req *ChatCompletionRe return newTracedStream(stream, span), nil } + +// sanitizeRequest drops sampling parameters the target model does not +// accept, using the default model registry. Reasoning models (e.g. the +// GPT-5 family) reject an explicit temperature, top_p, or penalties and +// fail the whole request with a 400, so the safest behavior is to omit +// the unsupported knobs rather than propagate a hard error. When the +// model is unknown to the registry the request is left untouched. +func sanitizeRequest(ctx context.Context, logger *log.Logger, req *ChatCompletionRequest) *ChatCompletionRequest { + if req == nil { + return req + } + + model, ok := DefaultRegistry().Lookup(req.Model) + if !ok { + return req + } + + supports := model.Supports + + dropTemperature := req.Temperature != nil && !supports.Temperature + dropTopP := req.TopP != nil && !supports.TopP + dropFrequencyPenalty := req.FrequencyPenalty != nil && !supports.FrequencyPenalty + dropPresencePenalty := req.PresencePenalty != nil && !supports.PresencePenalty + dropStop := len(req.StopSequences) > 0 && !supports.Stop + + if !dropTemperature && !dropTopP && !dropFrequencyPenalty && !dropPresencePenalty && !dropStop { + return req + } + + sanitized := *req + + dropped := make([]string, 0, 5) + if dropTemperature { + sanitized.Temperature = nil + dropped = append(dropped, "temperature") + } + + if dropTopP { + sanitized.TopP = nil + dropped = append(dropped, "top_p") + } + + if dropFrequencyPenalty { + sanitized.FrequencyPenalty = nil + dropped = append(dropped, "frequency_penalty") + } + + if dropPresencePenalty { + sanitized.PresencePenalty = nil + dropped = append(dropped, "presence_penalty") + } + + if dropStop { + sanitized.StopSequences = nil + dropped = append(dropped, "stop") + } + + logger.WarnCtx( + ctx, + "dropping unsupported sampling parameters for model", + log.String("model", req.Model), + log.String("dropped", strings.Join(dropped, ",")), + ) + + return &sanitized +} diff --git a/pkg/llm/llm_test.go b/pkg/llm/llm_test.go index e2861b051..d58accb1c 100644 --- a/pkg/llm/llm_test.go +++ b/pkg/llm/llm_test.go @@ -47,6 +47,24 @@ func (m *mockProvider) ChatCompletionStream(_ context.Context, _ *llm.ChatComple return m.streamResp, m.streamErr } +// capturingProvider records the request it received so tests can assert +// on the parameters the client forwarded to the provider. +type capturingProvider struct { + lastReq *llm.ChatCompletionRequest + chatResp *llm.ChatCompletionResponse + streamResp llm.ChatCompletionStream +} + +func (p *capturingProvider) ChatCompletion(_ context.Context, req *llm.ChatCompletionRequest) (*llm.ChatCompletionResponse, error) { + p.lastReq = req + return p.chatResp, nil +} + +func (p *capturingProvider) ChatCompletionStream(_ context.Context, req *llm.ChatCompletionRequest) (llm.ChatCompletionStream, error) { + p.lastReq = req + return p.streamResp, nil +} + type mockStream struct { events []llm.ChatCompletionStreamEvent idx int @@ -358,6 +376,105 @@ func TestChatCompletion(t *testing.T) { }) } +// --------------------------------------------------------------------------- +// Client — request sanitization +// --------------------------------------------------------------------------- + +func TestChatCompletionSanitizesUnsupportedParameters(t *testing.T) { + t.Parallel() + + temp := 0.1 + topP := 0.9 + freq := 0.5 + pres := 0.5 + + t.Run("drops temperature for reasoning model snapshot", func(t *testing.T) { + t.Parallel() + + provider := &capturingProvider{ + chatResp: &llm.ChatCompletionResponse{ + Model: "gpt-5-nano", + FinishReason: llm.FinishReasonStop, + Message: llm.Message{ + Role: llm.RoleAssistant, + Parts: []llm.Part{llm.TextPart{Text: "ok"}}, + }, + }, + } + + client, _ := newTestClient(provider) + _, err := client.ChatCompletion(context.Background(), &llm.ChatCompletionRequest{ + Model: "gpt-5-nano-2025-08-07", + Messages: []llm.Message{{Role: llm.RoleUser, Parts: []llm.Part{llm.TextPart{Text: "Hi"}}}}, + Temperature: &temp, + TopP: &topP, + FrequencyPenalty: &freq, + PresencePenalty: &pres, + }) + + require.NoError(t, err) + require.NotNil(t, provider.lastReq) + assert.Nil(t, provider.lastReq.Temperature) + assert.Nil(t, provider.lastReq.TopP) + assert.Nil(t, provider.lastReq.FrequencyPenalty) + assert.Nil(t, provider.lastReq.PresencePenalty) + }) + + t.Run("keeps temperature for chat model", func(t *testing.T) { + t.Parallel() + + provider := &capturingProvider{ + chatResp: &llm.ChatCompletionResponse{ + Model: "gpt-4o", + FinishReason: llm.FinishReasonStop, + Message: llm.Message{ + Role: llm.RoleAssistant, + Parts: []llm.Part{llm.TextPart{Text: "ok"}}, + }, + }, + } + + client, _ := newTestClient(provider) + _, err := client.ChatCompletion(context.Background(), &llm.ChatCompletionRequest{ + Model: "gpt-4o", + Messages: []llm.Message{{Role: llm.RoleUser, Parts: []llm.Part{llm.TextPart{Text: "Hi"}}}}, + Temperature: &temp, + }) + + require.NoError(t, err) + require.NotNil(t, provider.lastReq) + require.NotNil(t, provider.lastReq.Temperature) + assert.InEpsilon(t, 0.1, *provider.lastReq.Temperature, 1e-9) + }) + + t.Run("leaves unknown model untouched", func(t *testing.T) { + t.Parallel() + + provider := &capturingProvider{ + chatResp: &llm.ChatCompletionResponse{ + Model: "mystery-model", + FinishReason: llm.FinishReasonStop, + Message: llm.Message{ + Role: llm.RoleAssistant, + Parts: []llm.Part{llm.TextPart{Text: "ok"}}, + }, + }, + } + + client, _ := newTestClient(provider) + _, err := client.ChatCompletion(context.Background(), &llm.ChatCompletionRequest{ + Model: "mystery-model", + Messages: []llm.Message{{Role: llm.RoleUser, Parts: []llm.Part{llm.TextPart{Text: "Hi"}}}}, + Temperature: &temp, + }) + + require.NoError(t, err) + require.NotNil(t, provider.lastReq) + require.NotNil(t, provider.lastReq.Temperature) + assert.InEpsilon(t, 0.1, *provider.lastReq.Temperature, 1e-9) + }) +} + // --------------------------------------------------------------------------- // Client — ChatCompletionStream // --------------------------------------------------------------------------- diff --git a/pkg/llm/registry.go b/pkg/llm/registry.go index 1cc0f23b2..130b8b3b4 100644 --- a/pkg/llm/registry.go +++ b/pkg/llm/registry.go @@ -17,6 +17,7 @@ package llm //go:generate go run go.probo.inc/probo/internal/cmd/genmodels import ( + "regexp" "strings" "sync" ) @@ -81,7 +82,9 @@ func DefaultRegistry() *Registry { // Lookup finds a model by ID. It accepts both provider-prefixed IDs // ("anthropic/claude-opus-4.6") and bare provider IDs ("claude-opus-4-6", -// "gpt-5.4"). Returns false if the model is not in the registry. +// "gpt-5.4"). Dated provider snapshots ("gpt-5-nano-2025-08-07") fall +// back to their undated base model ("gpt-5-nano"). Returns false if the +// model is not in the registry. func (r *Registry) Lookup(modelID string) (ModelDefinition, bool) { if m, ok := r.byID[modelID]; ok { return *m, true @@ -91,6 +94,16 @@ func (r *Registry) Lookup(modelID string) (ModelDefinition, bool) { return *m, true } + if base := stripModelDateSuffix(modelID); base != modelID { + if m, ok := r.byID[base]; ok { + return *m, true + } + + if m, ok := r.byID[normalizeModelID(base)]; ok { + return *m, true + } + } + return ModelDefinition{}, false } @@ -121,3 +134,13 @@ func normalizeModelID(id string) string { return strings.ReplaceAll(id, ".", "-") } + +// modelDateSuffix matches a trailing provider snapshot date such as the +// "-2025-08-07" in "gpt-5-nano-2025-08-07". +var modelDateSuffix = regexp.MustCompile(`-\d{4}-\d{2}-\d{2}$`) + +// stripModelDateSuffix removes a trailing dated-snapshot suffix from a +// model ID, leaving the base model ID unchanged when none is present. +func stripModelDateSuffix(id string) string { + return modelDateSuffix.ReplaceAllString(id, "") +} diff --git a/pkg/llm/registry_test.go b/pkg/llm/registry_test.go index f9a09000c..25049b2bd 100644 --- a/pkg/llm/registry_test.go +++ b/pkg/llm/registry_test.go @@ -73,6 +73,23 @@ func TestRegistry_Lookup(t *testing.T) { }, ) + t.Run( + "dated snapshot falls back to base model", + func(t *testing.T) { + t.Parallel() + + r := llm.NewRegistry(map[string]llm.ModelDefinition{ + "openai/gpt-5-nano": { + Name: "OpenAI: GPT-5 Nano", + }, + }) + + m, ok := r.Lookup("gpt-5-nano-2025-08-07") + require.True(t, ok) + assert.Equal(t, "openai/gpt-5-nano", m.ID) + }, + ) + t.Run( "unknown model returns false", func(t *testing.T) {