From 61c767ea32d9cbdfe73fdf43388d2cd3066596df Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 4 Oct 2026 08:51:19 -0400 Subject: [PATCH 1/5] Add provider presets and FromSettings factory internal/ai/llm/presets defines the anthropic, openai, openrouter, ollama and openai_compatible presets (base URL, adapter, key policy, headers, structured output modes and model quirks) and FromSettings, which turns llm.Settings plus the decrypted key into a ready llm.Provider or a typed configuration error from package llm. Temperature is omitted for model ids matching ^o1 or ^o3, listed once in TemperatureRejectingModels. Maestro task: 4d90a725-5a7c-4bee-bbe4-d036cfb69c5b Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/presets/presets.go | 257 +++++++++++++++++ internal/ai/llm/presets/presets_test.go | 357 ++++++++++++++++++++++++ internal/ai/llm/settings.go | 65 +++++ 3 files changed, 679 insertions(+) create mode 100644 internal/ai/llm/presets/presets.go create mode 100644 internal/ai/llm/presets/presets_test.go create mode 100644 internal/ai/llm/settings.go diff --git a/internal/ai/llm/presets/presets.go b/internal/ai/llm/presets/presets.go new file mode 100644 index 0000000..f05b936 --- /dev/null +++ b/internal/ai/llm/presets/presets.go @@ -0,0 +1,257 @@ +// Package presets holds what MealQuest knows about each language-model +// vendor — base URL, adapter, credentials, headers, structured output mode +// and model quirks — and FromSettings, which turns the household's +// llm.Settings into a ready llm.Provider. +// +// FromSettings lives here rather than in package llm because it builds the +// adapters, and every adapter imports llm. +// +// Nothing in this package logs, and no error it returns contains the API +// key or quotes a setting's value. +package presets + +import ( + "context" + "fmt" + "math" + "regexp" + "slices" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/llm" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/llm/anthropic" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/llm/ollama" + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/llm/openai" +) + +// Adapter names the llm adapter package a preset builds. +type Adapter string + +// The adapters. +const ( + AdapterAnthropic Adapter = "anthropic" + AdapterOpenAI Adapter = "openai" + AdapterOllama Adapter = "ollama" +) + +// KeyPolicy is whether a preset sends an API key. +type KeyPolicy int + +// The key policies. +const ( + // KeyRequired presets fail without a key. + KeyRequired KeyPolicy = iota + // KeyOptional presets send a key when one is given. + KeyOptional + // KeyNone presets send no key and refuse one. + KeyNone +) + +// TemperatureRejectingModels is the one list of model-id patterns that +// reject the temperature field: OpenAI's o1 and o3 reasoning models. A +// preset whose OmitTemperature holds it never sends a temperature to a +// matching model. +var TemperatureRejectingModels = []*regexp.Regexp{ + regexp.MustCompile(`^o1`), + regexp.MustCompile(`^o3`), +} + +// Preset is one vendor's fixed configuration. +type Preset struct { + // Name is the Settings.Provider value that selects the preset. + Name string + // Adapter is the adapter the preset builds. + Adapter Adapter + // BaseURL is the API root the adapter appends its endpoint paths to. + // Empty means the household must supply it in Settings.BaseURL. + BaseURL string + // Key is whether the preset sends an API key. + Key KeyPolicy + // RequiresPublicURL means the preset needs Settings.PublicURL for its + // headers. + RequiresPublicURL bool + // Headers returns the extra request headers an openai-adapter preset + // sends; nil means none. + Headers func(llm.Settings) map[string]string + // StructuredOutput is the default mode of an openai-adapter preset. + StructuredOutput openai.StructuredOutput + // FallbackStructuredOutput is the mode the household can select when + // the default fails for its model; empty means none. FromSettings + // builds one mode and never retries a request in another. + FallbackStructuredOutput openai.StructuredOutput + // StructuredOutputs are the modes Settings.StructuredOutput may select. + StructuredOutputs []openai.StructuredOutput + // MaxTokens is the max_tokens the anthropic adapter sends when a + // Request sets none. + MaxTokens int + // OmitTemperature are model-id patterns the preset never sends a + // temperature to. + OmitTemperature []*regexp.Regexp +} + +// openRouterHeaders identify MealQuest to OpenRouter: HTTP-Referer is +// MEALQUEST_PUBLIC_URL and X-Title is the app name. +func openRouterHeaders(s llm.Settings) map[string]string { + return map[string]string{ + "HTTP-Referer": s.PublicURL, + "X-Title": "MealQuest", + } +} + +// omitsTemperature reports whether model matches one of the preset's +// OmitTemperature patterns. +func (p Preset) omitsTemperature(model string) bool { + return slices.ContainsFunc(p.OmitTemperature, func(re *regexp.Regexp) bool { + return re.MatchString(model) + }) +} + +// Lookup returns the preset Settings.Provider name selects. +func Lookup(name string) (Preset, bool) { + p, ok := all[name] + return p, ok +} + +// all is every preset, by Settings.Provider name. +var all = map[string]Preset{ + "anthropic": { + Name: "anthropic", + Adapter: AdapterAnthropic, + BaseURL: "https://api.anthropic.com", + Key: KeyRequired, + MaxTokens: 4096, + }, + "openai": { + Name: "openai", + Adapter: AdapterOpenAI, + BaseURL: "https://api.openai.com/v1", + Key: KeyRequired, + StructuredOutput: openai.JSONSchema, + StructuredOutputs: []openai.StructuredOutput{openai.JSONSchema}, + OmitTemperature: TemperatureRejectingModels, + }, + "openrouter": { + Name: "openrouter", + Adapter: AdapterOpenAI, + BaseURL: "https://openrouter.ai/api/v1", + Key: KeyRequired, + RequiresPublicURL: true, + Headers: openRouterHeaders, + StructuredOutput: openai.JSONSchema, + FallbackStructuredOutput: openai.JSONObject, + StructuredOutputs: []openai.StructuredOutput{openai.JSONSchema, openai.JSONObject}, + }, + "ollama": { + Name: "ollama", + Adapter: AdapterOllama, + Key: KeyNone, + }, + "openai_compatible": { + Name: "openai_compatible", + Adapter: AdapterOpenAI, + Key: KeyOptional, + StructuredOutput: openai.JSONSchema, + StructuredOutputs: []openai.StructuredOutput{openai.JSONSchema, openai.JSONObject, openai.PromptOnly}, + OmitTemperature: TemperatureRejectingModels, + }, +} + +// FromSettings returns the Provider s describes, using key as its API key. +// It fails with a configuration error from package llm — ErrUnknownProvider, +// ErrMissingKey, ErrUnexpectedKey, ErrMissingBaseURL, ErrMissingModel, +// ErrMissingPublicURL or ErrInvalidSetting — before contacting any vendor. +// +// The Provider sends s.Temperature with every request whose Temperature is +// nil; a request that sets one keeps it. Neither is sent to a model the +// preset's OmitTemperature matches. +func FromSettings(s llm.Settings, key string) (llm.Provider, error) { + p, ok := Lookup(s.Provider) + if !ok { + return nil, llm.ErrUnknownProvider + } + if err := check(p, s, key); err != nil { + return nil, fmt.Errorf("%s: %w", p.Name, err) + } + base := p.BaseURL + if base == "" { + base = s.BaseURL + } + + var inner llm.Provider + var err error + switch p.Adapter { + case AdapterAnthropic: + inner, err = anthropic.New(anthropic.Config{BaseURL: base, APIKey: key, Model: s.Model, MaxTokens: p.MaxTokens}) + case AdapterOllama: + inner, err = ollama.New(ollama.Config{BaseURL: base, Model: s.Model}) + case AdapterOpenAI: + mode := p.StructuredOutput + if s.StructuredOutput != "" { + mode = openai.StructuredOutput(s.StructuredOutput) + } + cfg := openai.Config{BaseURL: base, APIKey: key, Model: s.Model, StructuredOutput: mode} + if p.Headers != nil { + cfg.Headers = p.Headers(s) + } + inner, err = openai.New(cfg) + default: + panic("presets: preset " + p.Name + " has unknown adapter " + string(p.Adapter)) + } + if err != nil { + return nil, fmt.Errorf("%s: %w: %w", p.Name, llm.ErrInvalidSetting, err) + } + + cp := &configured{Provider: inner} + if !p.omitsTemperature(s.Model) { + t := s.Temperature + cp.temperature = &t + } + return cp, nil +} + +// check validates s and key against p. Its errors never quote a value. +func check(p Preset, s llm.Settings, key string) error { + switch { + case p.Key == KeyRequired && key == "": + return llm.ErrMissingKey + case p.Key == KeyNone && key != "": + return llm.ErrUnexpectedKey + } + switch { + case p.BaseURL == "" && s.BaseURL == "": + return llm.ErrMissingBaseURL + case p.BaseURL != "" && s.BaseURL != "" && s.BaseURL != p.BaseURL: + return fmt.Errorf("%w: base URL is fixed by the preset", llm.ErrInvalidSetting) + } + if s.Model == "" { + return llm.ErrMissingModel + } + if p.RequiresPublicURL && s.PublicURL == "" { + return llm.ErrMissingPublicURL + } + if s.StructuredOutput != "" && !slices.Contains(p.StructuredOutputs, openai.StructuredOutput(s.StructuredOutput)) { + return fmt.Errorf("%w: structured output mode is not supported by the preset", llm.ErrInvalidSetting) + } + if math.IsNaN(s.Temperature) || math.IsInf(s.Temperature, 0) || s.Temperature < 0 { + return fmt.Errorf("%w: temperature must be a non-negative number", llm.ErrInvalidSetting) + } + return nil +} + +// configured applies the household temperature and the preset's +// temperature quirk to every Generate. +type configured struct { + llm.Provider + // temperature is sent when a request sets none; nil means the model + // rejects temperature, so none is ever sent. + temperature *float64 +} + +func (c *configured) Generate(ctx context.Context, req llm.Request) (llm.Response, error) { + if c.temperature == nil { + req.Temperature = nil + } else if req.Temperature == nil { + t := *c.temperature + req.Temperature = &t + } + return c.Provider.Generate(ctx, req) +} diff --git a/internal/ai/llm/presets/presets_test.go b/internal/ai/llm/presets/presets_test.go new file mode 100644 index 0000000..bda8346 --- /dev/null +++ b/internal/ai/llm/presets/presets_test.go @@ -0,0 +1,357 @@ +package presets + +import ( + "context" + "encoding/json" + "errors" + "io" + "net/http" + "strings" + "testing" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/llm" +) + +const testKey = "sk-test-0123456789" + +// captured is the HTTP request an adapter sent. +type captured struct { + url string + header http.Header + body map[string]json.RawMessage +} + +var errStop = errors.New("recorded") + +// recordingTransport records each request and fails it, so no request +// leaves the process. +type recordingTransport struct{ reqs []captured } + +func (rt *recordingTransport) RoundTrip(r *http.Request) (*http.Response, error) { + c := captured{url: r.URL.String(), header: r.Header.Clone()} + if r.Body != nil { + raw, err := io.ReadAll(r.Body) + if err != nil { + return nil, err + } + if err := json.Unmarshal(raw, &c.body); err != nil { + return nil, err + } + } + rt.reqs = append(rt.reqs, c) + return nil, errStop +} + +// generate builds the provider s describes and returns the request its +// Generate sends for req. The adapters use http.DefaultTransport, which the +// test replaces, so the real preset base URLs are never contacted. +func generate(t *testing.T, s llm.Settings, key string, req llm.Request) captured { + t.Helper() + rt := &recordingTransport{} + orig := http.DefaultTransport + http.DefaultTransport = rt + t.Cleanup(func() { http.DefaultTransport = orig }) + + p, err := FromSettings(s, key) + if err != nil { + t.Fatalf("FromSettings: %v", err) + } + if _, err := p.Generate(context.Background(), req); err == nil { + t.Fatal("Generate succeeded through the recording transport") + } + if len(rt.reqs) != 1 { + t.Fatalf("sent %d requests, want 1", len(rt.reqs)) + } + return rt.reqs[0] +} + +func plainRequest() llm.Request { + return llm.Request{Messages: []llm.Message{{Role: llm.RoleUser, Content: "dinner?"}}} +} + +func schemaRequest() llm.Request { + r := plainRequest() + r.Schema = &llm.JSONSchema{Name: "side_quests", Schema: json.RawMessage(`{"type":"object"}`)} + return r +} + +func ptr(f float64) *float64 { return &f } + +func responseFormatType(t *testing.T, c captured) string { + t.Helper() + raw, ok := c.body["response_format"] + if !ok { + return "" + } + var rf struct{ Type string } + if err := json.Unmarshal(raw, &rf); err != nil { + t.Fatalf("response_format: %v", err) + } + return rf.Type +} + +func temperature(t *testing.T, c captured) (float64, bool) { + t.Helper() + raw, ok := c.body["temperature"] + if !ok { + return 0, false + } + var f float64 + if err := json.Unmarshal(raw, &f); err != nil { + t.Fatalf("temperature: %v", err) + } + return f, true +} + +func TestAnthropicPreset(t *testing.T) { + c := generate(t, llm.Settings{Provider: "anthropic", Model: "claude-sonnet-5", Temperature: 0.7}, testKey, plainRequest()) + if c.url != "https://api.anthropic.com/v1/messages" { + t.Fatalf("URL = %q", c.url) + } + if got := c.header.Get("X-Api-Key"); got != testKey { + t.Fatalf("x-api-key = %q", got) + } + if got := c.header.Get("Authorization"); got != "" { + t.Fatalf("Authorization = %q, want none", got) + } + if got, ok := temperature(t, c); !ok || got != 0.7 { + t.Fatalf("temperature = %v, %v; want 0.7", got, ok) + } + if string(c.body["max_tokens"]) != "4096" { + t.Fatalf("max_tokens = %s", c.body["max_tokens"]) + } +} + +func TestAnthropicPresetAcceptsItsOwnBaseURL(t *testing.T) { + c := generate(t, llm.Settings{Provider: "anthropic", BaseURL: "https://api.anthropic.com", Model: "claude-sonnet-5"}, testKey, plainRequest()) + if c.url != "https://api.anthropic.com/v1/messages" { + t.Fatalf("URL = %q", c.url) + } +} + +func TestOpenAIPreset(t *testing.T) { + c := generate(t, llm.Settings{Provider: "openai", Model: "gpt-4o", Temperature: 0.4}, testKey, schemaRequest()) + if c.url != "https://api.openai.com/v1/chat/completions" { + t.Fatalf("URL = %q", c.url) + } + if got := c.header.Get("Authorization"); got != "Bearer "+testKey { + t.Fatalf("Authorization = %q", got) + } + if got := responseFormatType(t, c); got != "json_schema" { + t.Fatalf("response_format type = %q, want json_schema", got) + } + if c.header.Get("HTTP-Referer") != "" || c.header.Get("X-Title") != "" { + t.Fatal("openai sends OpenRouter headers") + } +} + +func TestOpenRouterPreset(t *testing.T) { + s := llm.Settings{Provider: "openrouter", Model: "openai/gpt-4o", PublicURL: "https://meals.example.com/"} + c := generate(t, s, testKey, schemaRequest()) + if c.url != "https://openrouter.ai/api/v1/chat/completions" { + t.Fatalf("URL = %q", c.url) + } + if got := c.header.Get("Authorization"); got != "Bearer "+testKey { + t.Fatalf("Authorization = %q", got) + } + if got := c.header.Get("HTTP-Referer"); got != "https://meals.example.com/" { + t.Fatalf("HTTP-Referer = %q", got) + } + if got := c.header.Get("X-Title"); got != "MealQuest" { + t.Fatalf("X-Title = %q", got) + } + if got := responseFormatType(t, c); got != "json_schema" { + t.Fatalf("default response_format type = %q, want json_schema", got) + } +} + +func TestOpenRouterFallbackModeIsJSONObject(t *testing.T) { + p, _ := Lookup("openrouter") + s := llm.Settings{ + Provider: "openrouter", + Model: "meta-llama/llama-3.3-70b-instruct", + PublicURL: "https://meals.example.com", + StructuredOutput: string(p.FallbackStructuredOutput), + } + c := generate(t, s, testKey, schemaRequest()) + if got := responseFormatType(t, c); got != "json_object" { + t.Fatalf("response_format type = %q, want json_object", got) + } +} + +func TestOllamaPreset(t *testing.T) { + s := llm.Settings{Provider: "ollama", BaseURL: "http://ollama.lan:11434", Model: "llama3.2", Temperature: 0.5} + c := generate(t, s, "", plainRequest()) + if c.url != "http://ollama.lan:11434/api/chat" { + t.Fatalf("URL = %q", c.url) + } + if got := c.header.Get("Authorization"); got != "" { + t.Fatalf("Authorization = %q, want none", got) + } + var opts struct{ Temperature *float64 } + if err := json.Unmarshal(c.body["options"], &opts); err != nil || opts.Temperature == nil || *opts.Temperature != 0.5 { + t.Fatalf("options = %s", c.body["options"]) + } +} + +func TestOpenAICompatiblePreset(t *testing.T) { + base := llm.Settings{Provider: "openai_compatible", BaseURL: "http://llm.lan:8000/v1", Model: "qwen2.5"} + + c := generate(t, base, "", schemaRequest()) + if c.url != "http://llm.lan:8000/v1/chat/completions" { + t.Fatalf("URL = %q", c.url) + } + if got := c.header.Get("Authorization"); got != "" { + t.Fatalf("Authorization without key = %q, want none", got) + } + if got := responseFormatType(t, c); got != "json_schema" { + t.Fatalf("default response_format type = %q, want json_schema", got) + } + + c = generate(t, base, testKey, plainRequest()) + if got := c.header.Get("Authorization"); got != "Bearer "+testKey { + t.Fatalf("Authorization with key = %q", got) + } + + for mode, want := range map[string]string{"json_schema": "json_schema", "json_object": "json_object", "prompt_only": ""} { + s := base + s.StructuredOutput = mode + if got := responseFormatType(t, generate(t, s, "", schemaRequest())); got != want { + t.Errorf("mode %s: response_format type = %q, want %q", mode, got, want) + } + } +} + +func TestTemperatureOmittedForRejectingModels(t *testing.T) { + for _, provider := range []string{"openai", "openai_compatible"} { + for _, model := range []string{"o1", "o1-mini", "o3-mini"} { + s := llm.Settings{Provider: provider, BaseURL: "http://llm.lan/v1", Model: model, Temperature: 0.8} + if provider == "openai" { + s.BaseURL = "" + } + for _, req := range []llm.Request{plainRequest(), func() llm.Request { r := plainRequest(); r.Temperature = ptr(0.2); return r }()} { + if got, ok := temperature(t, generate(t, s, testKey, req)); ok { + t.Errorf("%s %s: temperature %v sent, want omitted", provider, model, got) + } + } + } + } +} + +func TestTemperatureSentForOtherModels(t *testing.T) { + for _, model := range []string{"gpt-4o", "gpt-4o-mini", "chatgpt-o1-like"} { + c := generate(t, llm.Settings{Provider: "openai", Model: model, Temperature: 0.8}, testKey, plainRequest()) + if got, ok := temperature(t, c); !ok || got != 0.8 { + t.Errorf("%s: temperature = %v, %v; want 0.8", model, got, ok) + } + } + // A request's own temperature wins over the household's. + req := plainRequest() + req.Temperature = ptr(0.1) + c := generate(t, llm.Settings{Provider: "openai", Model: "gpt-4o", Temperature: 0.8}, testKey, req) + if got, ok := temperature(t, c); !ok || got != 0.1 { + t.Fatalf("temperature = %v, %v; want 0.1", got, ok) + } +} + +func TestTemperatureQuirkPresets(t *testing.T) { + want := map[string]bool{"openai": true, "openai_compatible": true} + for name, p := range all { + if got := len(p.OmitTemperature) > 0; got != want[name] { + t.Errorf("%s: lists temperature quirk = %v, want %v", name, got, want[name]) + } + } +} + +func TestPresetTable(t *testing.T) { + cases := []struct { + name string + adapter Adapter + baseURL string + key KeyPolicy + }{ + {"anthropic", AdapterAnthropic, "https://api.anthropic.com", KeyRequired}, + {"openai", AdapterOpenAI, "https://api.openai.com/v1", KeyRequired}, + {"openrouter", AdapterOpenAI, "https://openrouter.ai/api/v1", KeyRequired}, + {"ollama", AdapterOllama, "", KeyNone}, + {"openai_compatible", AdapterOpenAI, "", KeyOptional}, + } + if len(all) != len(cases) { + t.Fatalf("%d presets, want %d", len(all), len(cases)) + } + for _, c := range cases { + p, ok := Lookup(c.name) + if !ok { + t.Fatalf("no preset %q", c.name) + } + if p.Name != c.name || p.Adapter != c.adapter || p.BaseURL != c.baseURL || p.Key != c.key { + t.Errorf("%s = {%s %s %q %d}", c.name, p.Name, p.Adapter, p.BaseURL, p.Key) + } + } +} + +func TestConfigErrors(t *testing.T) { + sentinels := []error{ + llm.ErrUnknownProvider, llm.ErrMissingKey, llm.ErrUnexpectedKey, llm.ErrMissingBaseURL, + llm.ErrMissingModel, llm.ErrMissingPublicURL, llm.ErrInvalidSetting, + } + cases := []struct { + name string + s llm.Settings + key string + want error + }{ + {"unknown provider", llm.Settings{Provider: "gemini", Model: "m"}, testKey, llm.ErrUnknownProvider}, + {"anthropic no key", llm.Settings{Provider: "anthropic", Model: "m"}, "", llm.ErrMissingKey}, + {"openai no key", llm.Settings{Provider: "openai", Model: "m"}, "", llm.ErrMissingKey}, + {"openrouter no key", llm.Settings{Provider: "openrouter", Model: "m", PublicURL: "https://x.example"}, "", llm.ErrMissingKey}, + {"ollama with key", llm.Settings{Provider: "ollama", BaseURL: "http://o:11434", Model: "m"}, testKey, llm.ErrUnexpectedKey}, + {"ollama no base URL", llm.Settings{Provider: "ollama", Model: "m"}, "", llm.ErrMissingBaseURL}, + {"openai_compatible no base URL", llm.Settings{Provider: "openai_compatible", Model: "m"}, "", llm.ErrMissingBaseURL}, + {"anthropic no model", llm.Settings{Provider: "anthropic"}, testKey, llm.ErrMissingModel}, + {"openai no model", llm.Settings{Provider: "openai"}, testKey, llm.ErrMissingModel}, + {"openrouter no model", llm.Settings{Provider: "openrouter", PublicURL: "https://x.example"}, testKey, llm.ErrMissingModel}, + {"ollama no model", llm.Settings{Provider: "ollama", BaseURL: "http://o:11434"}, "", llm.ErrMissingModel}, + {"openai_compatible no model", llm.Settings{Provider: "openai_compatible", BaseURL: "http://l/v1"}, "", llm.ErrMissingModel}, + {"openrouter no public URL", llm.Settings{Provider: "openrouter", Model: "m"}, testKey, llm.ErrMissingPublicURL}, + {"fixed base URL overridden", llm.Settings{Provider: "openai", BaseURL: "https://evil.example/v1", Model: "m"}, testKey, llm.ErrInvalidSetting}, + {"openai json_object", llm.Settings{Provider: "openai", Model: "m", StructuredOutput: "json_object"}, testKey, llm.ErrInvalidSetting}, + {"anthropic mode", llm.Settings{Provider: "anthropic", Model: "m", StructuredOutput: "json_schema"}, testKey, llm.ErrInvalidSetting}, + {"unknown mode", llm.Settings{Provider: "openai_compatible", BaseURL: "http://l/v1", Model: "m", StructuredOutput: "xml"}, "", llm.ErrInvalidSetting}, + {"negative temperature", llm.Settings{Provider: "openai", Model: "m", Temperature: -0.1}, testKey, llm.ErrInvalidSetting}, + {"bad base URL", llm.Settings{Provider: "ollama", BaseURL: "ftp://o", Model: "m"}, "", llm.ErrInvalidSetting}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + p, err := FromSettings(c.s, c.key) + if p != nil || !errors.Is(err, c.want) { + t.Fatalf("FromSettings = %v, %v; want %v", p, err, c.want) + } + for _, other := range sentinels { + if other != c.want && errors.Is(err, other) { + t.Fatalf("error %v also matches %v", err, other) + } + } + if c.key != "" && strings.Contains(err.Error(), c.key) { + t.Fatalf("error leaks the key: %v", err) + } + }) + } +} + +func TestReadyProviderForEveryPreset(t *testing.T) { + for _, s := range []struct { + s llm.Settings + key string + }{ + {llm.Settings{Provider: "anthropic", Model: "m"}, testKey}, + {llm.Settings{Provider: "openai", Model: "m"}, testKey}, + {llm.Settings{Provider: "openrouter", Model: "m", PublicURL: "https://x.example"}, testKey}, + {llm.Settings{Provider: "ollama", BaseURL: "http://o:11434", Model: "m"}, ""}, + {llm.Settings{Provider: "openai_compatible", BaseURL: "http://l/v1", Model: "m"}, ""}, + } { + if p, err := FromSettings(s.s, s.key); err != nil || p == nil { + t.Errorf("%s: FromSettings = %v, %v", s.s.Provider, p, err) + } + } +} diff --git a/internal/ai/llm/settings.go b/internal/ai/llm/settings.go new file mode 100644 index 0000000..a9333a7 --- /dev/null +++ b/internal/ai/llm/settings.go @@ -0,0 +1,65 @@ +package llm + +import "errors" + +// Settings is the household's AI configuration, the input to +// presets.FromSettings. The caller fills it from the db.AiSetting row and +// the service config; this package does not import internal/db. +// +// llm.Settings{ +// Provider: row.Provider, +// BaseURL: row.BaseUrl, +// Model: row.Model, +// Temperature: row.Temperature, +// PublicURL: cfg.PublicURL.String(), // MEALQUEST_PUBLIC_URL +// } +// +// Settings never holds the API key: the caller decrypts it and passes it +// to presets.FromSettings separately. +type Settings struct { + // Provider names the preset: "anthropic", "openai", "openrouter", + // "ollama" or "openai_compatible". + Provider string + // BaseURL is the API root. It is required for ollama and + // openai_compatible; the other presets fix their own, and a non-empty + // value must equal it. + BaseURL string + // Model is the model id Generate requests. It is required. + Model string + // Temperature is the household's sampling temperature. It is sent with + // every request that sets none, except to models that reject it. + Temperature float64 + // PublicURL is MEALQUEST_PUBLIC_URL. OpenRouter requires it and + // receives it as the HTTP-Referer header; other presets ignore it. + PublicURL string + // StructuredOutput selects how an OpenAI-compatible preset asks for + // JSON: "json_schema", "json_object" or "prompt_only". Empty selects + // the preset's default, json_schema. Each preset allows only the modes + // it lists; anthropic and ollama allow none, so it must be empty. + StructuredOutput string +} + +// Configuration errors from presets.FromSettings. Each is distinct and +// matches with errors.Is. +var ( + // ErrUnknownProvider means Settings.Provider names no preset. + ErrUnknownProvider = errors.New("llm: unknown provider") + // ErrMissingKey means the preset requires an API key and none was + // given. + ErrMissingKey = errors.New("llm: API key is required") + // ErrUnexpectedKey means an API key was given to a preset that sends + // none. + ErrUnexpectedKey = errors.New("llm: provider takes no API key") + // ErrMissingBaseURL means the preset requires a base URL and none was + // given. + ErrMissingBaseURL = errors.New("llm: base URL is required") + // ErrMissingModel means Settings.Model is empty. + ErrMissingModel = errors.New("llm: model is required") + // ErrMissingPublicURL means the preset requires Settings.PublicURL and + // it is empty. + ErrMissingPublicURL = errors.New("llm: public URL is required") + // ErrInvalidSetting means a setting has a value the preset does not + // accept, such as a base URL other than the preset's, an unsupported + // structured output mode or a negative temperature. + ErrInvalidSetting = errors.New("llm: invalid setting") +) -- 2.55.0 From b6e433cc98a395f1b0e89b951ed847c402c8aeac Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 4 Oct 2026 08:54:03 -0400 Subject: [PATCH 2/5] Allow openai and openrouter AI providers in the schema 0001 limited ai_settings.provider and ai_runs.provider to openai_compatible, anthropic and ollama, so the openai and openrouter presets could not be stored. Migration 0004 rebuilds both tables with the wider CHECK, keeping the three existing values and every row. ai_runs is referenced by suggestion_batches and week_plans, so it is rebuilt by copying rows aside and recreating the table under its own name with foreign keys deferred; the create-then-rename rebuild from 0003 leaves the dropped table's references counted as violations at commit. Tests cover upgrading a database with ai_settings, ai_runs and referencing rows, and storing every preset provider in both tables. Co-Authored-By: Claude Opus 5.5 --- internal/store/migrate_test.go | 95 +++++++++++++++++++++++-- internal/store/schema_test.go | 21 +++++- migrations/0004_ai_provider_presets.sql | 68 ++++++++++++++++++ 3 files changed, 178 insertions(+), 6 deletions(-) create mode 100644 migrations/0004_ai_provider_presets.sql diff --git a/internal/store/migrate_test.go b/internal/store/migrate_test.go index e104996..444399a 100644 --- a/internal/store/migrate_test.go +++ b/internal/store/migrate_test.go @@ -175,8 +175,8 @@ func TestMigrateEmbeddedMigrations(t *testing.T) { if err := Migrate(context.Background(), s.Writer, migrations.FS); err != nil { t.Fatal(err) } - if got := versions(migrationRows(t, s)); got != "0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status" { - t.Errorf("versions = %q, want 0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status", got) + if got := versions(migrationRows(t, s)); got != "0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets" { + t.Errorf("versions = %q, want 0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets", got) } } @@ -196,8 +196,8 @@ func TestMigrateBootstrapThenEmbeddedInitApplies(t *testing.T) { if err := Migrate(ctx, s.Writer, migrations.FS); err != nil { t.Fatal(err) } - if got := versions(migrationRows(t, s)); got != "0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status" { - t.Errorf("versions = %q, want 0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status", got) + if got := versions(migrationRows(t, s)); got != "0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets" { + t.Errorf("versions = %q, want 0000_bootstrap,0001_init,0002_household_settings_updated_by,0003_jobs_status,0004_ai_provider_presets", got) } if !tableExists(t, s, "dinners") { t.Error("table from 0001_init was not created") @@ -309,3 +309,90 @@ func TestMigrateJobsStatusMapsLegacyRows(t *testing.T) { t.Error("jobs_new left behind") } } + +// TestMigrateAIProviderPresetsKeepsRowsAndReferences upgrades a database +// holding ai_settings and ai_runs rows, with suggestion_batches and +// week_plans referencing the run, and checks 0004 carries every row over, +// keeps the references enforced and accepts the new providers. +func TestMigrateAIProviderPresetsKeepsRowsAndReferences(t *testing.T) { + ctx := context.Background() + s := openTestStore(t) + before := fstest.MapFS{} + for _, name := range []string{"0000_bootstrap.sql", "0001_init.sql", "0002_household_settings_updated_by.sql", "0003_jobs_status.sql"} { + body, err := fs.ReadFile(migrations.FS, name) + if err != nil { + t.Fatal(err) + } + before[name] = &fstest.MapFile{Data: body} + } + if err := Migrate(ctx, s.Writer, before); err != nil { + t.Fatal(err) + } + for _, stmt := range []string{ + `INSERT INTO ai_settings (id, provider, base_url, model, temperature, system_prompt, api_key_ct, api_key_last4, key_verified_at, created_at, updated_at) + VALUES (1, 'anthropic', 'https://api.anthropic.com', 'claude', 0.7, 'sp', x'0102', 'abcd', 'v', 'c', 'u')`, + `INSERT INTO ai_runs (id, skill, skill_version, provider, model, temperature, status, latency_ms, tokens_in, tokens_out, error, request_redacted, response_redacted, created_at, updated_at) + VALUES ('run-1', 'suggest', '1', 'ollama', 'llama', 0.5, 'ok', 10, 20, 30, 'e', 'req', 'resp', 'c', 'u')`, + `INSERT INTO suggestion_batches (id, trigger, status, ai_run_id, created_at, updated_at) + VALUES ('batch-1', 'manual', 'done', 'run-1', 'c', 'u')`, + `INSERT INTO week_plans (id, week_start, status, ai_run_id, created_at, updated_at) + VALUES ('plan-1', '2026-10-04', 'draft', 'run-1', 'c', 'u')`, + } { + if _, err := s.Writer.Exec(stmt); err != nil { + t.Fatalf("seed: %v", err) + } + } + + if err := Migrate(ctx, s.Writer, migrations.FS); err != nil { + t.Fatal(err) + } + + var provider, baseURL, model, systemPrompt, last4, verifiedAt, createdAt, updatedAt string + var temperature float64 + var keyCT []byte + if err := s.Reader.QueryRow(`SELECT provider, base_url, model, temperature, system_prompt, api_key_ct, api_key_last4, key_verified_at, created_at, updated_at FROM ai_settings WHERE id = 1`). + Scan(&provider, &baseURL, &model, &temperature, &systemPrompt, &keyCT, &last4, &verifiedAt, &createdAt, &updatedAt); err != nil { + t.Fatalf("read ai_settings: %v", err) + } + settings := []any{provider, baseURL, model, temperature, systemPrompt, string(keyCT), last4, verifiedAt, createdAt, updatedAt} + if want := []any{"anthropic", "https://api.anthropic.com", "claude", 0.7, "sp", "\x01\x02", "abcd", "v", "c", "u"}; !slices.Equal(settings, want) { + t.Errorf("ai_settings = %v, want %v", settings, want) + } + + var skill, skillVersion, status, errText, request, response string + var latency, tokensIn, tokensOut int + if err := s.Reader.QueryRow(`SELECT skill, skill_version, provider, model, temperature, status, latency_ms, tokens_in, tokens_out, error, request_redacted, response_redacted, created_at, updated_at FROM ai_runs WHERE id = 'run-1'`). + Scan(&skill, &skillVersion, &provider, &model, &temperature, &status, &latency, &tokensIn, &tokensOut, &errText, &request, &response, &createdAt, &updatedAt); err != nil { + t.Fatalf("read ai_runs: %v", err) + } + run := []any{skill, skillVersion, provider, model, temperature, status, latency, tokensIn, tokensOut, errText, request, response, createdAt, updatedAt} + if want := []any{"suggest", "1", "ollama", "llama", 0.5, "ok", 10, 20, 30, "e", "req", "resp", "c", "u"}; !slices.Equal(run, want) { + t.Errorf("ai_runs = %v, want %v", run, want) + } + + rows, err := s.Reader.Query("PRAGMA foreign_key_check") + if err != nil { + t.Fatal(err) + } + if rows.Next() { + t.Error("foreign_key_check reported violations after 0004") + } + if err := rows.Close(); err != nil { + t.Fatal(err) + } + if _, err := s.Writer.Exec(`DELETE FROM ai_runs WHERE id = 'run-1'`); err == nil || !strings.Contains(err.Error(), "FOREIGN KEY constraint failed") { + t.Errorf("deleting a referenced ai_run: error %v, want FOREIGN KEY constraint failure", err) + } + + if _, err := s.Writer.Exec(`UPDATE ai_settings SET provider = 'openrouter' WHERE id = 1`); err != nil { + t.Errorf("ai_settings.provider openrouter rejected: %v", err) + } + if _, err := s.Writer.Exec(`UPDATE ai_runs SET provider = 'openai' WHERE id = 'run-1'`); err != nil { + t.Errorf("ai_runs.provider openai rejected: %v", err) + } + for _, table := range []string{"ai_settings_copy", "ai_runs_copy"} { + if tableExists(t, s, table) { + t.Errorf("%s left behind", table) + } + } +} diff --git a/internal/store/schema_test.go b/internal/store/schema_test.go index 2b36ad1..1ce39a4 100644 --- a/internal/store/schema_test.go +++ b/internal/store/schema_test.go @@ -305,7 +305,7 @@ func TestSchemaEnumChecksRejectInvalidValues(t *testing.T) { {"household_settings", "plan_draft_dow", 5.5}, {"household_settings", "suggestions_dow", 0.5}, {"household_settings", "chat_autosend_dow", 3.5}, - {"ai_runs", "provider", "openai"}, + {"ai_runs", "provider", "gemini"}, {"ai_runs", "status", "done"}, {"dinners", "status", "deck"}, {"dinners", "origin", "ai"}, @@ -322,7 +322,7 @@ func TestSchemaEnumChecksRejectInvalidValues(t *testing.T) { {"rotation_reviews", "decision", "promote"}, {"shopping_lists", "status", "frozen"}, {"shopping_items", "needed", 2}, - {"ai_settings", "provider", "openai"}, + {"ai_settings", "provider", "gemini"}, {"recipe_search_settings", "provider", "google"}, {"chat_settings", "last_test_status", "pending"}, {"notification_prefs", "event", "weekly_digest"}, @@ -360,6 +360,23 @@ func TestSchemaRotationReviewEventAllowed(t *testing.T) { } } +// TestSchemaAIProvidersAllowed checks every provider preset can be stored in +// both ai_settings and ai_runs. +func TestSchemaAIProvidersAllowed(t *testing.T) { + s := seededStore(t) + for _, provider := range []string{"openai_compatible", "anthropic", "ollama", "openai", "openrouter"} { + for _, table := range []string{"ai_settings", "ai_runs"} { + inRolledBackTx(t, s, func(tx *sql.Tx) { + row := testRow(table, 1) + row["provider"] = provider + if err := insertSchemaRow(tx, table, row); err != nil { + t.Errorf("%s.provider %s rejected: %v", table, provider, err) + } + }) + } + } +} + func TestSchemaDinnerStatuses(t *testing.T) { s := seededStore(t) for _, status := range []string{"suggested", "quest_log", "tried", "regular", "archived"} { diff --git a/migrations/0004_ai_provider_presets.sql b/migrations/0004_ai_provider_presets.sql new file mode 100644 index 0000000..965ca79 --- /dev/null +++ b/migrations/0004_ai_provider_presets.sql @@ -0,0 +1,68 @@ +-- 0004_ai_provider_presets: ai_settings.provider and ai_runs.provider become +-- openai_compatible, anthropic, ollama, openai, openrouter, the presets in +-- internal/ai/llm/presets. Every 0001 value is kept, so existing rows copy +-- unchanged. SQLite cannot change a CHECK in place, so both tables are +-- rebuilt: copy the rows aside, drop the table, create it again with the +-- wider CHECK, copy the rows back and drop the copy. Neither table has +-- indexes or triggers. +-- +-- suggestion_batches.ai_run_id and week_plans.ai_run_id reference ai_runs, +-- so the rebuild cannot use 0003's create-new-then-rename: dropping ai_runs +-- counts every referencing row as a foreign key violation, and only inserting +-- the matching ai_runs rows under the name ai_runs clears them. Deferring +-- foreign keys holds the check until commit, after the rows are back. The +-- pragma lasts only for this migration's transaction. + +PRAGMA defer_foreign_keys = ON; + +CREATE TABLE ai_settings_copy AS SELECT * FROM ai_settings; + +DROP TABLE ai_settings; + +CREATE TABLE ai_settings ( + id INTEGER PRIMARY KEY CHECK (id = 1), + provider TEXT NOT NULL CHECK (provider IN ('openai_compatible', 'anthropic', 'ollama', 'openai', 'openrouter')), + base_url TEXT NOT NULL, + model TEXT NOT NULL, + temperature REAL NOT NULL, + system_prompt TEXT, + api_key_ct BLOB, + api_key_last4 TEXT, + key_verified_at TEXT, + created_at TEXT NOT NULL, + updated_at TEXT NOT NULL +); + +INSERT INTO ai_settings (id, provider, base_url, model, temperature, system_prompt, api_key_ct, api_key_last4, key_verified_at, created_at, updated_at) +SELECT id, provider, base_url, model, temperature, system_prompt, api_key_ct, api_key_last4, key_verified_at, created_at, updated_at +FROM ai_settings_copy; + +DROP TABLE ai_settings_copy; + +CREATE TABLE ai_runs_copy AS SELECT * FROM ai_runs; + +DROP TABLE ai_runs; + +CREATE TABLE ai_runs ( + id TEXT NOT NULL PRIMARY KEY, + skill TEXT NOT NULL, + skill_version TEXT NOT NULL, + provider TEXT NOT NULL CHECK (provider IN ('openai_compatible', 'anthropic', 'ollama', 'openai', 'openrouter')), + model TEXT NOT NULL, + temperature REAL NOT NULL, + status TEXT NOT NULL CHECK (status IN ('running', 'ok', 'error')), + latency_ms INTEGER, + tokens_in INTEGER, + tokens_out INTEGER, + error TEXT, + request_redacted TEXT NOT NULL, + response_redacted TEXT, + created_at TEXT NOT NULL, + updated_at TEXT NOT NULL +); + +INSERT INTO ai_runs (id, skill, skill_version, provider, model, temperature, status, latency_ms, tokens_in, tokens_out, error, request_redacted, response_redacted, created_at, updated_at) +SELECT id, skill, skill_version, provider, model, temperature, status, latency_ms, tokens_in, tokens_out, error, request_redacted, response_redacted, created_at, updated_at +FROM ai_runs_copy; + +DROP TABLE ai_runs_copy; -- 2.55.0 From cb93134f3efa77e252e0e1a2e5d3bbc7e5ebc394 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 4 Oct 2026 08:58:43 -0400 Subject: [PATCH 3/5] Fix reasoning-model temperature quirk matching and validation - Omit temperature for OpenRouter's namespaced OpenAI reasoning models (openai/o1, openai/o1-mini, openai/o3-mini, openai/o1:online). - Match o1/o3 only as the bare family name or with a "-" variant, so o123 and o3custom keep their temperature. - Validate the caller's request before clearing temperature, so a negative temperature is still a validation error, not a sent request. Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/presets/presets.go | 31 ++++++++---- internal/ai/llm/presets/presets_test.go | 64 +++++++++++++++++++++---- 2 files changed, 75 insertions(+), 20 deletions(-) diff --git a/internal/ai/llm/presets/presets.go b/internal/ai/llm/presets/presets.go index f05b936..ac80e42 100644 --- a/internal/ai/llm/presets/presets.go +++ b/internal/ai/llm/presets/presets.go @@ -46,14 +46,16 @@ const ( KeyNone ) -// TemperatureRejectingModels is the one list of model-id patterns that -// reject the temperature field: OpenAI's o1 and o3 reasoning models. A -// preset whose OmitTemperature holds it never sends a temperature to a -// matching model. -var TemperatureRejectingModels = []*regexp.Regexp{ - regexp.MustCompile(`^o1`), - regexp.MustCompile(`^o3`), -} +// The reasoning models that reject the temperature field are OpenAI's o1 +// and o3 families: the bare family name or the name followed by a "-" +// variant (o1, o1-mini, o3-2025-04-16), never a longer word such as o123. +// OpenRouter names them with an "openai/" prefix and may add a ":" variant +// (openai/o1:online). A preset whose OmitTemperature holds a pattern never +// sends a temperature to a matching model. +var ( + openAIReasoningModels = regexp.MustCompile(`^o[13](-.*)?$`) + openRouterReasoningModels = regexp.MustCompile(`^openai/o[13]([-:].*)?$`) +) // Preset is one vendor's fixed configuration. type Preset struct { @@ -127,7 +129,7 @@ var all = map[string]Preset{ Key: KeyRequired, StructuredOutput: openai.JSONSchema, StructuredOutputs: []openai.StructuredOutput{openai.JSONSchema}, - OmitTemperature: TemperatureRejectingModels, + OmitTemperature: []*regexp.Regexp{openAIReasoningModels}, }, "openrouter": { Name: "openrouter", @@ -139,6 +141,7 @@ var all = map[string]Preset{ StructuredOutput: openai.JSONSchema, FallbackStructuredOutput: openai.JSONObject, StructuredOutputs: []openai.StructuredOutput{openai.JSONSchema, openai.JSONObject}, + OmitTemperature: []*regexp.Regexp{openRouterReasoningModels}, }, "ollama": { Name: "ollama", @@ -151,7 +154,9 @@ var all = map[string]Preset{ Key: KeyOptional, StructuredOutput: openai.JSONSchema, StructuredOutputs: []openai.StructuredOutput{openai.JSONSchema, openai.JSONObject, openai.PromptOnly}, - OmitTemperature: TemperatureRejectingModels, + // The server may be OpenAI itself or a gateway that names models + // the OpenRouter way. + OmitTemperature: []*regexp.Regexp{openAIReasoningModels, openRouterReasoningModels}, }, } @@ -246,7 +251,13 @@ type configured struct { temperature *float64 } +// Generate validates req as sent by the caller before changing its +// temperature, so clearing it for a model that rejects temperature never +// hides an invalid one. func (c *configured) Generate(ctx context.Context, req llm.Request) (llm.Response, error) { + if err := req.Validate(); err != nil { + return llm.Response{}, fmt.Errorf("presets: generate: %w", err) + } if c.temperature == nil { req.Temperature = nil } else if req.Temperature == nil { diff --git a/internal/ai/llm/presets/presets_test.go b/internal/ai/llm/presets/presets_test.go index bda8346..512a47b 100644 --- a/internal/ai/llm/presets/presets_test.go +++ b/internal/ai/llm/presets/presets_test.go @@ -223,11 +223,19 @@ func TestOpenAICompatiblePreset(t *testing.T) { } func TestTemperatureOmittedForRejectingModels(t *testing.T) { - for _, provider := range []string{"openai", "openai_compatible"} { - for _, model := range []string{"o1", "o1-mini", "o3-mini"} { - s := llm.Settings{Provider: provider, BaseURL: "http://llm.lan/v1", Model: model, Temperature: 0.8} - if provider == "openai" { - s.BaseURL = "" + cases := map[string][]string{ + "openai": {"o1", "o1-mini", "o1-2024-12-17", "o3", "o3-mini"}, + "openai_compatible": {"o1", "o1-mini", "o3-mini", "openai/o1"}, + "openrouter": {"openai/o1", "openai/o1-mini", "openai/o3", "openai/o3-mini", "openai/o1:online"}, + } + for provider, models := range cases { + for _, model := range models { + s := llm.Settings{Provider: provider, Model: model, Temperature: 0.8} + switch provider { + case "openai_compatible": + s.BaseURL = "http://llm.lan/v1" + case "openrouter": + s.PublicURL = "https://mealquest.example" } for _, req := range []llm.Request{plainRequest(), func() llm.Request { r := plainRequest(); r.Temperature = ptr(0.2); return r }()} { if got, ok := temperature(t, generate(t, s, testKey, req)); ok { @@ -239,10 +247,19 @@ func TestTemperatureOmittedForRejectingModels(t *testing.T) { } func TestTemperatureSentForOtherModels(t *testing.T) { - for _, model := range []string{"gpt-4o", "gpt-4o-mini", "chatgpt-o1-like"} { - c := generate(t, llm.Settings{Provider: "openai", Model: model, Temperature: 0.8}, testKey, plainRequest()) - if got, ok := temperature(t, c); !ok || got != 0.8 { - t.Errorf("%s: temperature = %v, %v; want 0.8", model, got, ok) + cases := map[string][]string{ + "openai": {"gpt-4o", "gpt-4o-mini", "chatgpt-o1-like", "o123", "o3custom", "o1mini", "openai/o1"}, + "openrouter": {"openai/gpt-4o", "openai/o123", "openai/o3custom", "acme/o1", "o1-mini"}, + } + for provider, models := range cases { + for _, model := range models { + s := llm.Settings{Provider: provider, Model: model, Temperature: 0.8} + if provider == "openrouter" { + s.PublicURL = "https://mealquest.example" + } + if got, ok := temperature(t, generate(t, s, testKey, plainRequest())); !ok || got != 0.8 { + t.Errorf("%s %s: temperature = %v, %v; want 0.8", provider, model, got, ok) + } } } // A request's own temperature wins over the household's. @@ -254,8 +271,35 @@ func TestTemperatureSentForOtherModels(t *testing.T) { } } +// A request is validated before the temperature quirk clears its +// temperature, so a negative one is an error and nothing is sent. +func TestNegativeTemperatureRejectedForRejectingModels(t *testing.T) { + rt := &recordingTransport{} + orig := http.DefaultTransport + http.DefaultTransport = rt + t.Cleanup(func() { http.DefaultTransport = orig }) + + for _, s := range []llm.Settings{ + {Provider: "openai", Model: "o1"}, + {Provider: "openrouter", Model: "openai/o1", PublicURL: "https://mealquest.example"}, + } { + p, err := FromSettings(s, testKey) + if err != nil { + t.Fatalf("%s: FromSettings: %v", s.Model, err) + } + req := plainRequest() + req.Temperature = ptr(-0.5) + if _, err := p.Generate(context.Background(), req); err == nil || errors.Is(err, errStop) || !strings.Contains(err.Error(), "temperature") { + t.Errorf("%s: Generate = %v, want a temperature validation error", s.Model, err) + } + } + if len(rt.reqs) != 0 { + t.Fatalf("sent %d requests, want 0", len(rt.reqs)) + } +} + func TestTemperatureQuirkPresets(t *testing.T) { - want := map[string]bool{"openai": true, "openai_compatible": true} + want := map[string]bool{"openai": true, "openrouter": true, "openai_compatible": true} for name, p := range all { if got := len(p.OmitTemperature) > 0; got != want[name] { t.Errorf("%s: lists temperature quirk = %v, want %v", name, got, want[name]) -- 2.55.0 From 31322f239ee6a95612ed3f4cf6054e48a6761059 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 4 Oct 2026 09:03:12 -0400 Subject: [PATCH 4/5] Redact the API key from preset configuration and validation errors FromSettings errors and the configured provider's request-validation errors now pass through a presets redactedError: its text has the key removed with llm.Redact, it does not unwrap to the unredacted layers, and Is still matches the llm sentinels. A key that equals a request value (temperature -0.5) or a setting no longer appears in the error. Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/presets/presets.go | 46 +++++++++++++++-- internal/ai/llm/presets/presets_test.go | 65 +++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 4 deletions(-) diff --git a/internal/ai/llm/presets/presets.go b/internal/ai/llm/presets/presets.go index ac80e42..a05c675 100644 --- a/internal/ai/llm/presets/presets.go +++ b/internal/ai/llm/presets/presets.go @@ -6,12 +6,14 @@ // FromSettings lives here rather than in package llm because it builds the // adapters, and every adapter imports llm. // -// Nothing in this package logs, and no error it returns contains the API -// key or quotes a setting's value. +// Nothing in this package logs, and no error it returns or unwraps to +// contains the API key, even where the key equals a setting's or request's +// value or text the error spells anyway. package presets import ( "context" + "errors" "fmt" "math" "regexp" @@ -169,6 +171,14 @@ var all = map[string]Preset{ // nil; a request that sets one keeps it. Neither is sent to a model the // preset's OmitTemperature matches. func FromSettings(s llm.Settings, key string) (llm.Provider, error) { + cp, err := fromSettings(s, key) + if err != nil { + return nil, redact(err, key) + } + return cp, nil +} + +func fromSettings(s llm.Settings, key string) (*configured, error) { p, ok := Lookup(s.Provider) if !ok { return nil, llm.ErrUnknownProvider @@ -205,7 +215,7 @@ func FromSettings(s llm.Settings, key string) (llm.Provider, error) { return nil, fmt.Errorf("%s: %w: %w", p.Name, llm.ErrInvalidSetting, err) } - cp := &configured{Provider: inner} + cp := &configured{Provider: inner, key: key} if !p.omitsTemperature(s.Model) { t := s.Temperature cp.temperature = &t @@ -246,6 +256,9 @@ func check(p Preset, s llm.Settings, key string) error { // temperature quirk to every Generate. type configured struct { llm.Provider + // key is the API key, redacted from validation errors; the adapter + // redacts its own. + key string // temperature is sent when a request sets none; nil means the model // rejects temperature, so none is ever sent. temperature *float64 @@ -256,7 +269,7 @@ type configured struct { // hides an invalid one. func (c *configured) Generate(ctx context.Context, req llm.Request) (llm.Response, error) { if err := req.Validate(); err != nil { - return llm.Response{}, fmt.Errorf("presets: generate: %w", err) + return llm.Response{}, redact(fmt.Errorf("presets: generate: %w", err), c.key) } if c.temperature == nil { req.Temperature = nil @@ -266,3 +279,28 @@ func (c *configured) Generate(ctx context.Context, req llm.Request) (llm.Respons } return c.Provider.Generate(ctx, req) } + +// redactedError is an error with the API key removed from its text. It +// does not unwrap, because every layer below may hold the key; Is still +// matches the sentinels the original error wraps. +type redactedError struct { + text string + err error +} + +func (e *redactedError) Error() string { return e.text } + +// GoString keeps %#v to the redacted Error text instead of the fields. +func (e *redactedError) GoString() string { return e.text } + +// Is reports whether the original error matches target. +func (e *redactedError) Is(target error) bool { return errors.Is(e.err, target) } + +// redact returns err with key removed from its text; a nil err or an empty +// key returns err unchanged. +func redact(err error, key string) error { + if err == nil || key == "" { + return err + } + return &redactedError{text: llm.Redact(err.Error(), key), err: err} +} diff --git a/internal/ai/llm/presets/presets_test.go b/internal/ai/llm/presets/presets_test.go index 512a47b..78c6b2c 100644 --- a/internal/ai/llm/presets/presets_test.go +++ b/internal/ai/llm/presets/presets_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "io" "net/http" "strings" @@ -399,3 +400,67 @@ func TestReadyProviderForEveryPreset(t *testing.T) { } } } + +// A key that matches a value the request carries, or text the error spells +// anyway, never appears in a validation error, and nothing is sent. +func TestGenerateValidationErrorsNeverLeakKey(t *testing.T) { + rt := &recordingTransport{} + orig := http.DefaultTransport + http.DefaultTransport = rt + t.Cleanup(func() { http.DefaultTransport = orig }) + + for _, key := range []string{"-0.5", "temperature", "llm", "negative"} { + p, err := FromSettings(llm.Settings{Provider: "openai_compatible", BaseURL: "http://l/v1", Model: "m"}, key) + if err != nil { + t.Fatalf("%s: FromSettings: %v", key, err) + } + req := plainRequest() + req.Temperature = ptr(-0.5) + _, err = p.Generate(context.Background(), req) + if err == nil || errors.Is(err, errStop) { + t.Fatalf("%s: Generate = %v, want a validation error", key, err) + } + for e := error(err); e != nil; e = errors.Unwrap(e) { + if s := e.Error() + fmt.Sprintf("%#v", e); strings.Contains(s, key) { + t.Fatalf("%s: error leaks the key: %s", key, s) + } + } + } + if len(rt.reqs) != 0 { + t.Fatalf("sent %d requests, want 0", len(rt.reqs)) + } +} + +// A configuration error keeps its sentinel but never holds the key, even +// when the key is also a setting's value or text the error spells anyway. +func TestConfigErrorsNeverLeakKey(t *testing.T) { + cases := []struct { + name string + s llm.Settings + key string + want error + }{ + {"unknown provider named by key", llm.Settings{Provider: "llm", Model: "m"}, "llm", llm.ErrUnknownProvider}, + {"no base URL", llm.Settings{Provider: "openai_compatible", Model: "m"}, "base URL", llm.ErrMissingBaseURL}, + {"no model", llm.Settings{Provider: "openai_compatible", BaseURL: "http://l/v1"}, "model", llm.ErrMissingModel}, + {"key as model, no public URL", llm.Settings{Provider: "openrouter", Model: "missing"}, "missing", llm.ErrMissingPublicURL}, + {"key as base URL", llm.Settings{Provider: "openai", BaseURL: "llm", Model: "m"}, "llm", llm.ErrInvalidSetting}, + {"key as mode", llm.Settings{Provider: "openai_compatible", BaseURL: "http://l/v1", Model: "m", StructuredOutput: "xml"}, "xml", llm.ErrInvalidSetting}, + {"key as temperature", llm.Settings{Provider: "openai", Model: "m", Temperature: -0.1}, "-0.1", llm.ErrInvalidSetting}, + {"key in bad base URL", llm.Settings{Provider: "openai_compatible", BaseURL: "ftp://k3y", Model: "m"}, "k3y", llm.ErrInvalidSetting}, + {"ollama with key", llm.Settings{Provider: "ollama", BaseURL: "http://o:11434", Model: "m"}, "key", llm.ErrUnexpectedKey}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + p, err := FromSettings(c.s, c.key) + if p != nil || !errors.Is(err, c.want) { + t.Fatalf("FromSettings = %v, %v; want %v", p, err, c.want) + } + for e := error(err); e != nil; e = errors.Unwrap(e) { + if s := e.Error() + fmt.Sprintf("%#v", e); strings.Contains(s, c.key) { + t.Fatalf("error leaks the key: %s", s) + } + } + }) + } +} -- 2.55.0 From 08be8891da9ea2201a414ce4b09e9a9c443af2bf Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 4 Oct 2026 09:08:46 -0400 Subject: [PATCH 5/5] Reject API keys that are not valid header values at construction FromSettings now fails with llm.ErrInvalidKey for a key with a control character such as CR or LF, for every preset that sends a key, and openai.New rejects such a key too, so neither defers the failure to the first request. anthropic.New already rejected it. No error quotes the key. Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/openai/openai.go | 4 ++++ internal/ai/llm/openai/openai_test.go | 14 ++++++++++++++ internal/ai/llm/presets/presets.go | 15 ++++++++++++++- internal/ai/llm/presets/presets_test.go | 8 +++++++- internal/ai/llm/settings.go | 3 +++ 5 files changed, 42 insertions(+), 2 deletions(-) diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index 4f21be7..70c94e9 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -53,6 +53,7 @@ type Config struct { BaseURL string // APIKey is sent as "Authorization: Bearer ". Empty sends no // Authorization header, for endpoints such as Ollama that need none. + // It must not have a control character other than tab. APIKey string // Model is the model Generate requests. ListModels and Verify do not // need it. @@ -102,6 +103,9 @@ func newFromConfig(cfg Config) (*Provider, error) { if u.User != nil || strings.ContainsAny(cfg.BaseURL, "?#") { return nil, errors.New("openai: base URL must not carry credentials, a query or a fragment") } + if !validHeaderValue(cfg.APIKey) { + return nil, errors.New("openai: API key has a control character") + } // No error from New quotes caller-supplied text or a name derived from // it: any such text may equal or contain the API key, and redaction // cannot catch a fragment or a case-changed copy of it. diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index c953fe1..12f2c09 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -628,6 +628,20 @@ func TestNewRejectsBadHeaderWithoutEchoingValue(t *testing.T) { assertNoSecret(t, err) } +func TestNewRejectsKeyWithControlCharacter(t *testing.T) { + for _, key := range []string{testKey + "\n", testKey + "\r\nX-Injected: 1", "\x00" + testKey, testKey + "\x7f"} { + _, err := New(Config{BaseURL: "https://example.com/v1", APIKey: key, StructuredOutput: JSONSchema}) + if err == nil { + t.Errorf("New(%q) = nil error", key) + continue + } + assertNoSecret(t, err) + if strings.Contains(err.Error()+fmt.Sprintf("%#v", err), key) { + t.Errorf("error leaks the key: %v", err) + } + } +} + // TestNewConfigErrorsNeverLeakKey covers every New error that names // caller-supplied text, each with the API key embedded in that text. func TestNewConfigErrorsNeverLeakKey(t *testing.T) { diff --git a/internal/ai/llm/presets/presets.go b/internal/ai/llm/presets/presets.go index a05c675..d33d613 100644 --- a/internal/ai/llm/presets/presets.go +++ b/internal/ai/llm/presets/presets.go @@ -164,7 +164,7 @@ var all = map[string]Preset{ // FromSettings returns the Provider s describes, using key as its API key. // It fails with a configuration error from package llm — ErrUnknownProvider, -// ErrMissingKey, ErrUnexpectedKey, ErrMissingBaseURL, ErrMissingModel, +// ErrMissingKey, ErrUnexpectedKey, ErrInvalidKey, ErrMissingBaseURL, ErrMissingModel, // ErrMissingPublicURL or ErrInvalidSetting — before contacting any vendor. // // The Provider sends s.Temperature with every request whose Temperature is @@ -230,6 +230,8 @@ func check(p Preset, s llm.Settings, key string) error { return llm.ErrMissingKey case p.Key == KeyNone && key != "": return llm.ErrUnexpectedKey + case !validHeaderValue(key): + return llm.ErrInvalidKey } switch { case p.BaseURL == "" && s.BaseURL == "": @@ -252,6 +254,17 @@ func check(p Preset, s llm.Settings, key string) error { return nil } +// validHeaderValue reports whether value has no control character other +// than tab, so it can be sent as an HTTP header value. +func validHeaderValue(value string) bool { + for i := 0; i < len(value); i++ { + if c := value[i]; (c < 0x20 && c != '\t') || c == 0x7f { + return false + } + } + return true +} + // configured applies the household temperature and the preset's // temperature quirk to every Generate. type configured struct { diff --git a/internal/ai/llm/presets/presets_test.go b/internal/ai/llm/presets/presets_test.go index 78c6b2c..47f98c5 100644 --- a/internal/ai/llm/presets/presets_test.go +++ b/internal/ai/llm/presets/presets_test.go @@ -337,7 +337,7 @@ func TestPresetTable(t *testing.T) { func TestConfigErrors(t *testing.T) { sentinels := []error{ - llm.ErrUnknownProvider, llm.ErrMissingKey, llm.ErrUnexpectedKey, llm.ErrMissingBaseURL, + llm.ErrUnknownProvider, llm.ErrMissingKey, llm.ErrUnexpectedKey, llm.ErrInvalidKey, llm.ErrMissingBaseURL, llm.ErrMissingModel, llm.ErrMissingPublicURL, llm.ErrInvalidSetting, } cases := []struct { @@ -351,6 +351,10 @@ func TestConfigErrors(t *testing.T) { {"openai no key", llm.Settings{Provider: "openai", Model: "m"}, "", llm.ErrMissingKey}, {"openrouter no key", llm.Settings{Provider: "openrouter", Model: "m", PublicURL: "https://x.example"}, "", llm.ErrMissingKey}, {"ollama with key", llm.Settings{Provider: "ollama", BaseURL: "http://o:11434", Model: "m"}, testKey, llm.ErrUnexpectedKey}, + {"anthropic key with newline", llm.Settings{Provider: "anthropic", Model: "m"}, testKey + "\n", llm.ErrInvalidKey}, + {"openai key with newline", llm.Settings{Provider: "openai", Model: "m"}, testKey + "\n", llm.ErrInvalidKey}, + {"openrouter key with CR LF", llm.Settings{Provider: "openrouter", Model: "m", PublicURL: "https://x.example"}, testKey + "\r\nX-Injected: 1", llm.ErrInvalidKey}, + {"openai_compatible key with NUL", llm.Settings{Provider: "openai_compatible", BaseURL: "http://l/v1", Model: "m"}, "\x00" + testKey, llm.ErrInvalidKey}, {"ollama no base URL", llm.Settings{Provider: "ollama", Model: "m"}, "", llm.ErrMissingBaseURL}, {"openai_compatible no base URL", llm.Settings{Provider: "openai_compatible", Model: "m"}, "", llm.ErrMissingBaseURL}, {"anthropic no model", llm.Settings{Provider: "anthropic"}, testKey, llm.ErrMissingModel}, @@ -449,6 +453,8 @@ func TestConfigErrorsNeverLeakKey(t *testing.T) { {"key as temperature", llm.Settings{Provider: "openai", Model: "m", Temperature: -0.1}, "-0.1", llm.ErrInvalidSetting}, {"key in bad base URL", llm.Settings{Provider: "openai_compatible", BaseURL: "ftp://k3y", Model: "m"}, "k3y", llm.ErrInvalidSetting}, {"ollama with key", llm.Settings{Provider: "ollama", BaseURL: "http://o:11434", Model: "m"}, "key", llm.ErrUnexpectedKey}, + {"key with newline", llm.Settings{Provider: "openai", Model: "m"}, "sk-test\n", llm.ErrInvalidKey}, + {"key with newline as text", llm.Settings{Provider: "openai", Model: "m"}, "header\n", llm.ErrInvalidKey}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { diff --git a/internal/ai/llm/settings.go b/internal/ai/llm/settings.go index a9333a7..adde36c 100644 --- a/internal/ai/llm/settings.go +++ b/internal/ai/llm/settings.go @@ -50,6 +50,9 @@ var ( // ErrUnexpectedKey means an API key was given to a preset that sends // none. ErrUnexpectedKey = errors.New("llm: provider takes no API key") + // ErrInvalidKey means the API key is not a valid HTTP header value: + // it has a control character such as CR or LF. + ErrInvalidKey = errors.New("llm: API key is not a valid header value") // ErrMissingBaseURL means the preset requires a base URL and none was // given. ErrMissingBaseURL = errors.New("llm: base URL is required") -- 2.55.0