From 6d0bea10b5e0e0abb86426ba485da87877870aa7 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sat, 3 Oct 2026 16:20:23 -0400 Subject: [PATCH 01/12] Add OpenAI-compatible llm.Provider adapter internal/ai/llm/openai implements Provider over plain net/http against POST {base}/chat/completions and GET {base}/models, with Bearer auth and optional extra headers. Structured output uses response_format json_schema (strict), json_object, or prompt-only with first-JSON-object extraction, per the configured mode. Usage tokens, model and finish_reason are parsed. Errors map 401/403 to ErrAuth, 429 to ErrRateLimited with Retry-After (seconds or HTTP-date), 408 and transport timeouts to ErrTimeout, and other non-2xx statuses (and OpenRouter error bodies in a 2xx) to ErrProvider with a key-redacted excerpt. Contract tests replay OpenAI, OpenRouter and Ollama /v1 payloads from httptest servers and assert the key and Authorization value never appear in an error. Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/openai/openai.go | 415 +++++++++++++ internal/ai/llm/openai/openai_test.go | 579 ++++++++++++++++++ .../testdata/ollama_chat_prompt_only.json | 18 + .../llm/openai/testdata/ollama_chat_text.json | 11 + .../testdata/ollama_chat_truncated.json | 15 + .../testdata/ollama_error_not_found.json | 1 + .../ai/llm/openai/testdata/ollama_models.json | 7 + .../testdata/openai_chat_json_schema.json | 28 + .../openai/testdata/openai_chat_refusal.json | 16 + .../testdata/openai_error_invalid_key.json | 8 + .../openai/testdata/openai_error_server.json | 1 + .../ai/llm/openai/testdata/openai_models.json | 7 + .../testdata/openrouter_chat_json_object.json | 22 + .../openrouter_chat_upstream_error.json | 5 + .../openrouter_error_rate_limited.json | 1 + .../openai/testdata/openrouter_models.json | 22 + 16 files changed, 1156 insertions(+) create mode 100644 internal/ai/llm/openai/openai.go create mode 100644 internal/ai/llm/openai/openai_test.go create mode 100644 internal/ai/llm/openai/testdata/ollama_chat_prompt_only.json create mode 100644 internal/ai/llm/openai/testdata/ollama_chat_text.json create mode 100644 internal/ai/llm/openai/testdata/ollama_chat_truncated.json create mode 100644 internal/ai/llm/openai/testdata/ollama_error_not_found.json create mode 100644 internal/ai/llm/openai/testdata/ollama_models.json create mode 100644 internal/ai/llm/openai/testdata/openai_chat_json_schema.json create mode 100644 internal/ai/llm/openai/testdata/openai_chat_refusal.json create mode 100644 internal/ai/llm/openai/testdata/openai_error_invalid_key.json create mode 100644 internal/ai/llm/openai/testdata/openai_error_server.json create mode 100644 internal/ai/llm/openai/testdata/openai_models.json create mode 100644 internal/ai/llm/openai/testdata/openrouter_chat_json_object.json create mode 100644 internal/ai/llm/openai/testdata/openrouter_chat_upstream_error.json create mode 100644 internal/ai/llm/openai/testdata/openrouter_error_rate_limited.json create mode 100644 internal/ai/llm/openai/testdata/openrouter_models.json diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go new file mode 100644 index 0000000..3e7c4c8 --- /dev/null +++ b/internal/ai/llm/openai/openai.go @@ -0,0 +1,415 @@ +// Package openai is the llm.Provider adapter for OpenAI-compatible chat +// APIs: OpenAI itself, OpenRouter and Ollama's /v1 endpoint. It calls +// POST {base}/chat/completions and GET {base}/models over plain net/http. +// +// The adapter never logs, and no error it returns contains the API key or +// the Authorization header value. +package openai + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "io" + "net" + "net/http" + "net/textproto" + "net/url" + "strconv" + "strings" + "time" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/llm" +) + +// StructuredOutput is how the adapter asks the vendor for JSON when a +// Request carries a Schema. Which mode a vendor and model support comes +// from its preset. +type StructuredOutput string + +const ( + // JSONSchema sends response_format {type: json_schema, strict: true} + // with the Request's schema. + JSONSchema StructuredOutput = "json_schema" + // JSONObject sends response_format {type: json_object} and puts the + // schema in the system prompt. + JSONObject StructuredOutput = "json_object" + // PromptOnly sends no response_format, puts the schema in the system + // prompt and takes the first JSON object found in the reply. + PromptOnly StructuredOutput = "prompt_only" +) + +// Config configures a Provider. +type Config struct { + // BaseURL is the API root the endpoint paths are appended to, e.g. + // "https://api.openai.com/v1", "https://openrouter.ai/api/v1" or + // "http://ollama:11434/v1". + BaseURL string + // APIKey is sent as "Authorization: Bearer ". Empty sends no + // Authorization header, for endpoints such as Ollama that need none. + APIKey string + // Model is the model Generate requests. ListModels and Verify do not + // need it. + Model string + // Headers are extra request headers, e.g. OpenRouter's HTTP-Referer and + // X-Title. They must not set Authorization or Content-Type. + Headers map[string]string + // StructuredOutput is how JSON is requested when a Request has a Schema. + StructuredOutput StructuredOutput +} + +// Provider is an OpenAI-compatible llm.Provider. +type Provider struct { + base string + apiKey string + model string + headers http.Header + mode StructuredOutput + client *http.Client +} + +var _ llm.Provider = (*Provider)(nil) + +// New validates cfg and returns a Provider. +func New(cfg Config) (*Provider, error) { + u, err := url.Parse(cfg.BaseURL) + if err != nil { + return nil, errors.New("openai: base URL does not parse") + } + if (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" { + return nil, fmt.Errorf("openai: base URL %q must be an absolute http or https URL", u.Redacted()) + } + if u.User != nil || u.RawQuery != "" || u.Fragment != "" { + return nil, fmt.Errorf("openai: base URL %q must not carry credentials, a query or a fragment", u.Redacted()) + } + switch cfg.StructuredOutput { + case JSONSchema, JSONObject, PromptOnly: + default: + return nil, fmt.Errorf("openai: unknown structured output mode %q", cfg.StructuredOutput) + } + headers := make(http.Header, len(cfg.Headers)) + for name, value := range cfg.Headers { + canonical := textproto.CanonicalMIMEHeaderKey(name) + if canonical == "Authorization" || canonical == "Content-Type" { + return nil, fmt.Errorf("openai: extra header %q is set by the adapter", canonical) + } + if name == "" || strings.ContainsAny(name, " \t\r\n:") || strings.ContainsAny(value, "\r\n") { + return nil, fmt.Errorf("openai: extra header %q is malformed", name) + } + headers.Set(canonical, value) + } + return &Provider{ + base: strings.TrimRight(cfg.BaseURL, "/"), + apiKey: cfg.APIKey, + model: cfg.Model, + headers: headers, + mode: cfg.StructuredOutput, + client: &http.Client{}, + }, nil +} + +// maxResponseBody caps a successful response body; a larger one is an error. +const maxResponseBody = 16 << 20 + +// maxErrorBody caps how much of an error response body is read. It is far +// above llm.MaxBodyExcerpt, so redaction still sees every secret that could +// reach the excerpt. +const maxErrorBody = 64 << 10 + +type chatMessage struct { + Role string `json:"role"` + Content string `json:"content"` +} + +type jsonSchemaFormat struct { + Name string `json:"name"` + Schema json.RawMessage `json:"schema"` + Strict bool `json:"strict"` +} + +type responseFormat struct { + Type string `json:"type"` + JSONSchema *jsonSchemaFormat `json:"json_schema,omitempty"` +} + +type chatRequest struct { + Model string `json:"model"` + Messages []chatMessage `json:"messages"` + Temperature *float64 `json:"temperature,omitempty"` + MaxTokens int `json:"max_tokens,omitempty"` + ResponseFormat *responseFormat `json:"response_format,omitempty"` +} + +type chatResponse struct { + Model string `json:"model"` + Choices []struct { + Message struct { + Content *string `json:"content"` + Refusal *string `json:"refusal"` + } `json:"message"` + FinishReason string `json:"finish_reason"` + } `json:"choices"` + Usage struct { + PromptTokens int `json:"prompt_tokens"` + CompletionTokens int `json:"completion_tokens"` + } `json:"usage"` + Error json.RawMessage `json:"error"` +} + +// schemaInstruction is appended to the system prompt in the json_object and +// prompt_only modes, where the schema is not sent in response_format. +const schemaInstruction = "Reply with only a JSON object, and no other text, that matches this JSON Schema:\n" + +// Generate sends req to {base}/chat/completions. +func (p *Provider) Generate(ctx context.Context, req llm.Request) (llm.Response, error) { + if err := req.Validate(); err != nil { + return llm.Response{}, fmt.Errorf("openai: generate: %w", err) + } + if p.model == "" { + return llm.Response{}, errors.New("openai: generate: no model configured") + } + body := chatRequest{ + Model: p.model, + Temperature: req.Temperature, + MaxTokens: req.MaxTokens, + } + system := req.System + if req.Schema != nil { + switch p.mode { + case JSONSchema: + body.ResponseFormat = &responseFormat{ + Type: "json_schema", + JSONSchema: &jsonSchemaFormat{Name: req.Schema.Name, Schema: req.Schema.Schema, Strict: true}, + } + case JSONObject: + body.ResponseFormat = &responseFormat{Type: "json_object"} + system = appendInstruction(system, req.Schema.Schema) + case PromptOnly: + system = appendInstruction(system, req.Schema.Schema) + } + } + if system != "" { + body.Messages = append(body.Messages, chatMessage{Role: "system", Content: system}) + } + for _, m := range req.Messages { + body.Messages = append(body.Messages, chatMessage{Role: string(m.Role), Content: m.Content}) + } + payload, err := json.Marshal(body) + if err != nil { + return llm.Response{}, fmt.Errorf("openai: generate: encode request: %w", err) + } + + if req.Timeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, req.Timeout) + defer cancel() + } + raw, status, err := p.do(ctx, http.MethodPost, "/chat/completions", payload) + if err != nil { + return llm.Response{}, fmt.Errorf("openai: generate: %w", err) + } + + var cr chatResponse + if err := json.Unmarshal(raw, &cr); err != nil { + return llm.Response{}, fmt.Errorf("openai: generate: decode response: %w", err) + } + if len(cr.Error) > 0 && string(cr.Error) != "null" { + // OpenRouter can report an upstream failure in a 2xx body. + return llm.Response{}, fmt.Errorf("openai: generate: %w", llm.NewProviderError(status, raw, p.secrets())) + } + if len(cr.Choices) == 0 { + return llm.Response{}, errors.New("openai: generate: response has no choices") + } + choice := cr.Choices[0] + resp := llm.Response{ + Usage: llm.Usage{InputTokens: cr.Usage.PromptTokens, OutputTokens: cr.Usage.CompletionTokens}, + Model: cr.Model, + FinishReason: choice.FinishReason, + } + if choice.Message.Content != nil { + resp.Text = *choice.Message.Content + } + if req.Schema == nil { + return resp, nil + } + if choice.Message.Refusal != nil && *choice.Message.Refusal != "" { + return resp, fmt.Errorf("openai: generate: model refused the structured request: %w", llm.ErrBadOutput) + } + if p.mode == PromptOnly { + resp.JSON = firstJSONObject(resp.Text) + } else if text := strings.TrimSpace(resp.Text); isJSONObject(text) { + resp.JSON = json.RawMessage(text) + } + if resp.JSON == nil { + return resp, fmt.Errorf("openai: generate: reply (finish reason %q) holds no JSON object: %w", resp.FinishReason, llm.ErrBadOutput) + } + return resp, nil +} + +func appendInstruction(system string, schema json.RawMessage) string { + instruction := schemaInstruction + string(schema) + if system == "" { + return instruction + } + return system + "\n\n" + instruction +} + +// isJSONObject reports whether s is exactly one JSON object. +func isJSONObject(s string) bool { + return strings.HasPrefix(s, "{") && json.Valid([]byte(s)) +} + +// firstJSONObject returns the first complete JSON object in s, trying each +// "{" in order, or nil when there is none. Text around it, such as prose or +// a Markdown code fence, is ignored. +func firstJSONObject(s string) json.RawMessage { + for i := strings.IndexByte(s, '{'); i >= 0; { + var obj json.RawMessage + if err := json.NewDecoder(strings.NewReader(s[i:])).Decode(&obj); err == nil { + return obj + } + next := strings.IndexByte(s[i+1:], '{') + if next < 0 { + break + } + i += 1 + next + } + return nil +} + +type modelsResponse struct { + Data []struct { + ID string `json:"id"` + Name string `json:"name"` + } `json:"data"` +} + +// ListModels returns the models {base}/models lists. +func (p *Provider) ListModels(ctx context.Context) ([]llm.Model, error) { + raw, _, err := p.do(ctx, http.MethodGet, "/models", nil) + if err != nil { + return nil, fmt.Errorf("openai: list models: %w", err) + } + var mr modelsResponse + if err := json.Unmarshal(raw, &mr); err != nil { + return nil, fmt.Errorf("openai: list models: decode response: %w", err) + } + if mr.Data == nil { + return nil, errors.New("openai: list models: response has no data list") + } + models := make([]llm.Model, 0, len(mr.Data)) + for i, m := range mr.Data { + if m.ID == "" { + return nil, fmt.Errorf("openai: list models: model %d has no id", i) + } + name := m.Name + if name == "" { + name = m.ID + } + models = append(models, llm.Model{ID: m.ID, DisplayName: name}) + } + return models, nil +} + +// Verify checks the credentials with an authenticated ListModels call. +func (p *Provider) Verify(ctx context.Context) error { + _, err := p.ListModels(ctx) + return err +} + +func (p *Provider) secrets() []string { return []string{p.apiKey} } + +// do sends one request and returns the body and status of a 2xx response. +// Any other status becomes the matching llm typed error. +func (p *Provider) do(ctx context.Context, method, path string, payload []byte) ([]byte, int, error) { + var reqBody io.Reader + if payload != nil { + reqBody = bytes.NewReader(payload) + } + httpReq, err := http.NewRequestWithContext(ctx, method, p.base+path, reqBody) + if err != nil { + return nil, 0, fmt.Errorf("build request: %w", err) + } + for name, values := range p.headers { + httpReq.Header[name] = values + } + if payload != nil { + httpReq.Header.Set("Content-Type", "application/json") + } + httpReq.Header.Set("Accept", "application/json") + if p.apiKey != "" { + httpReq.Header.Set("Authorization", "Bearer "+p.apiKey) + } + + httpResp, err := p.client.Do(httpReq) + if err != nil { + return nil, 0, transportError(err) + } + defer httpResp.Body.Close() + + if httpResp.StatusCode < 200 || httpResp.StatusCode > 299 { + body, err := io.ReadAll(io.LimitReader(httpResp.Body, maxErrorBody)) + if err != nil { + return nil, httpResp.StatusCode, transportError(err) + } + return nil, httpResp.StatusCode, p.statusError(httpResp, body) + } + body, err := io.ReadAll(io.LimitReader(httpResp.Body, maxResponseBody+1)) + if err != nil { + return nil, httpResp.StatusCode, transportError(err) + } + if len(body) > maxResponseBody { + return nil, httpResp.StatusCode, fmt.Errorf("response body exceeds %d bytes", maxResponseBody) + } + return body, httpResp.StatusCode, nil +} + +// statusError maps a non-2xx response to an llm typed error. +func (p *Provider) statusError(resp *http.Response, body []byte) error { + switch resp.StatusCode { + case http.StatusUnauthorized, http.StatusForbidden: + return fmt.Errorf("HTTP %d: %w", resp.StatusCode, llm.ErrAuth) + case http.StatusTooManyRequests: + return &llm.RateLimitedError{RetryAfter: retryAfter(resp.Header.Get("Retry-After"), time.Now())} + case http.StatusRequestTimeout: + return fmt.Errorf("HTTP %d: %w", resp.StatusCode, llm.ErrTimeout) + default: + return llm.NewProviderError(resp.StatusCode, body, p.secrets()) + } +} + +// retryAfter parses a Retry-After header, in delay-seconds or HTTP-date +// form. A missing, malformed or past value means the vendor gave no usable +// wait, reported as 0. +func retryAfter(value string, now time.Time) time.Duration { + value = strings.TrimSpace(value) + if value == "" { + return 0 + } + if secs, err := strconv.Atoi(value); err == nil { + if secs < 0 { + return 0 + } + return time.Duration(secs) * time.Second + } + if at, err := http.ParseTime(value); err == nil && at.After(now) { + return at.Sub(now).Round(time.Second) + } + return 0 +} + +func isTimeout(err error) bool { + var ne net.Error + return errors.Is(err, context.DeadlineExceeded) || (errors.As(err, &ne) && ne.Timeout()) +} + +// transportError reports a failure to complete the HTTP exchange. The +// underlying error names the URL, never the request headers. +func transportError(err error) error { + if isTimeout(err) { + return fmt.Errorf("%w: %w", llm.ErrTimeout, err) + } + return err +} diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go new file mode 100644 index 0000000..2f13e5c --- /dev/null +++ b/internal/ai/llm/openai/openai_test.go @@ -0,0 +1,579 @@ +package openai + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync" + "testing" + "time" + + "code.cubecraftlabs.com/Maniacs_Mansion/Meal-Quest/internal/ai/llm" +) + +const testKey = "sk-test-9f8e7d6c5b4a39281706f5e4d3c2b1a0" + +// recorded is one request the test server received. +type recorded struct { + method string + path string + header http.Header + body []byte +} + +// reply is the response the test server replays. +type reply struct { + status int + header map[string]string + fixture string // testdata file; {{KEY}} is replaced with testKey + delay time.Duration +} + +// vendorServer replays one recorded payload and records the request. +type vendorServer struct { + *httptest.Server + mu sync.Mutex + got []recorded +} + +func newVendorServer(t *testing.T, r reply) *vendorServer { + t.Helper() + var payload []byte + if r.fixture != "" { + raw, err := os.ReadFile(filepath.Join("testdata", r.fixture)) + if err != nil { + t.Fatal(err) + } + payload = bytes.ReplaceAll(raw, []byte("{{KEY}}"), []byte(testKey)) + } + vs := &vendorServer{} + vs.Server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + body, _ := io.ReadAll(req.Body) + vs.mu.Lock() + vs.got = append(vs.got, recorded{req.Method, req.URL.Path, req.Header.Clone(), body}) + vs.mu.Unlock() + if r.delay > 0 { + select { + case <-time.After(r.delay): + case <-req.Context().Done(): + return + } + } + for k, v := range r.header { + w.Header().Set(k, v) + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(r.status) + w.Write(payload) + })) + t.Cleanup(vs.Close) + return vs +} + +func (vs *vendorServer) only(t *testing.T) recorded { + t.Helper() + vs.mu.Lock() + defer vs.mu.Unlock() + if len(vs.got) != 1 { + t.Fatalf("server got %d requests, want 1", len(vs.got)) + } + return vs.got[0] +} + +func newProvider(t *testing.T, base string, mode StructuredOutput, headers map[string]string) *Provider { + t.Helper() + p, err := New(Config{BaseURL: base, APIKey: testKey, Model: "test-model", Headers: headers, StructuredOutput: mode}) + if err != nil { + t.Fatal(err) + } + return p +} + +var dinnersSchema = json.RawMessage(`{"type":"object","properties":{"dinners":{"type":"array"}},"required":["dinners"],"additionalProperties":false}`) + +func schemaRequest() llm.Request { + temp := 0.0 + return llm.Request{ + System: "You plan dinners for Our kitchen.", + Messages: []llm.Message{{Role: llm.RoleUser, Content: "Suggest a Side quest."}}, + Temperature: &temp, + MaxTokens: 400, + Schema: &llm.JSONSchema{Name: "side_quests", Schema: dinnersSchema}, + } +} + +// sentBody decodes the chat request body into a generic map. +func sentBody(t *testing.T, r recorded) map[string]any { + t.Helper() + var m map[string]any + if err := json.Unmarshal(r.body, &m); err != nil { + t.Fatalf("request body %q: %v", r.body, err) + } + return m +} + +func assertJSON(t *testing.T, got json.RawMessage, want string) { + t.Helper() + var g, w any + if err := json.Unmarshal(got, &g); err != nil { + t.Fatalf("Response.JSON %q: %v", got, err) + } + if err := json.Unmarshal([]byte(want), &w); err != nil { + t.Fatal(err) + } + gb, _ := json.Marshal(g) + wb, _ := json.Marshal(w) + if !bytes.Equal(gb, wb) { + t.Fatalf("Response.JSON = %s, want %s", gb, wb) + } +} + +func TestGenerateJSONSchemaOpenAI(t *testing.T) { + vs := newVendorServer(t, reply{status: 200, fixture: "openai_chat_json_schema.json"}) + p := newProvider(t, vs.URL+"/v1/", JSONSchema, nil) + + resp, err := p.Generate(context.Background(), schemaRequest()) + if err != nil { + t.Fatal(err) + } + assertJSON(t, resp.JSON, `{"dinners":[{"name":"Sheet-pan gnocchi","why":"Like your Regular tray bake, but quicker."}]}`) + if resp.Text != string(resp.JSON) { + t.Errorf("Text = %q, want the raw reply", resp.Text) + } + if resp.Usage != (llm.Usage{InputTokens: 1117, OutputTokens: 46}) || resp.Model != "gpt-4o-2024-08-06" || resp.FinishReason != "stop" { + t.Errorf("Usage, Model, FinishReason = %+v, %q, %q", resp.Usage, resp.Model, resp.FinishReason) + } + + got := vs.only(t) + if got.method != http.MethodPost || got.path != "/v1/chat/completions" { + t.Errorf("request = %s %s", got.method, got.path) + } + if h := got.header.Get("Authorization"); h != "Bearer "+testKey { + t.Errorf("Authorization = %q", h) + } + if h := got.header.Get("Content-Type"); h != "application/json" { + t.Errorf("Content-Type = %q", h) + } + body := sentBody(t, got) + if body["model"] != "test-model" || body["temperature"] != 0.0 || body["max_tokens"] != 400.0 { + t.Errorf("model, temperature, max_tokens = %v, %v, %v", body["model"], body["temperature"], body["max_tokens"]) + } + msgs := body["messages"].([]any) + if len(msgs) != 2 { + t.Fatalf("messages = %v", msgs) + } + system := msgs[0].(map[string]any) + if system["role"] != "system" || system["content"] != "You plan dinners for Our kitchen." { + t.Errorf("system message = %v (the schema must not be in the prompt in json_schema mode)", system) + } + if user := msgs[1].(map[string]any); user["role"] != "user" || user["content"] != "Suggest a Side quest." { + t.Errorf("user message = %v", user) + } + rf := body["response_format"].(map[string]any) + js := rf["json_schema"].(map[string]any) + if rf["type"] != "json_schema" || js["name"] != "side_quests" || js["strict"] != true { + t.Errorf("response_format = %v", rf) + } + schema, _ := json.Marshal(js["schema"]) + var want any + json.Unmarshal(dinnersSchema, &want) + wantB, _ := json.Marshal(want) + if !bytes.Equal(schema, wantB) { + t.Errorf("response_format schema = %s", schema) + } +} + +func TestGenerateJSONObjectOpenRouter(t *testing.T) { + vs := newVendorServer(t, reply{status: 200, fixture: "openrouter_chat_json_object.json"}) + p := newProvider(t, vs.URL+"/api/v1", JSONObject, map[string]string{"http-referer": "https://mealquest.example", "X-Title": "MealQuest"}) + + req := schemaRequest() + req.Temperature = nil + req.MaxTokens = 0 + resp, err := p.Generate(context.Background(), req) + if err != nil { + t.Fatal(err) + } + assertJSON(t, resp.JSON, `{"dinners":[{"name":"Chicken tinga tacos","why":"Close to your taco-night Regular."}]}`) + if resp.Usage != (llm.Usage{InputTokens: 842, OutputTokens: 31}) || resp.Model != "openai/gpt-4o-mini" || resp.FinishReason != "stop" { + t.Errorf("Usage, Model, FinishReason = %+v, %q, %q", resp.Usage, resp.Model, resp.FinishReason) + } + + got := vs.only(t) + if got.path != "/api/v1/chat/completions" { + t.Errorf("path = %s", got.path) + } + if got.header.Get("HTTP-Referer") != "https://mealquest.example" || got.header.Get("X-Title") != "MealQuest" { + t.Errorf("extra headers = %v", got.header) + } + body := sentBody(t, got) + if _, ok := body["temperature"]; ok { + t.Error("temperature sent although Request.Temperature is nil") + } + if _, ok := body["max_tokens"]; ok { + t.Error("max_tokens sent although Request.MaxTokens is 0") + } + if rf := body["response_format"].(map[string]any); rf["type"] != "json_object" || rf["json_schema"] != nil { + t.Errorf("response_format = %v", rf) + } + system := body["messages"].([]any)[0].(map[string]any)["content"].(string) + if !strings.HasPrefix(system, "You plan dinners for Our kitchen.\n\n"+schemaInstruction) || !strings.Contains(system, `"required":["dinners"]`) { + t.Errorf("system prompt lacks the schema instruction: %q", system) + } +} + +func TestGeneratePromptOnlyOllama(t *testing.T) { + vs := newVendorServer(t, reply{status: 200, fixture: "ollama_chat_prompt_only.json"}) + p, err := New(Config{BaseURL: vs.URL + "/v1", Model: "llama3.2", StructuredOutput: PromptOnly}) + if err != nil { + t.Fatal(err) + } + req := schemaRequest() + req.System = "" + resp, err := p.Generate(context.Background(), req) + if err != nil { + t.Fatal(err) + } + // The reply has a non-JSON "{idea}" first and a second object after the + // fenced one; only the first complete object is taken. + assertJSON(t, resp.JSON, `{"dinners":[{"name":"Crock pot chili","why":"A Quick dinner like your {Regular} stew."}]}`) + if !strings.HasPrefix(resp.Text, "Sure! Here is") { + t.Errorf("Text = %q, want the whole reply", resp.Text) + } + if resp.Usage != (llm.Usage{InputTokens: 512, OutputTokens: 58}) || resp.Model != "llama3.2" || resp.FinishReason != "stop" { + t.Errorf("Usage, Model, FinishReason = %+v, %q, %q", resp.Usage, resp.Model, resp.FinishReason) + } + + got := vs.only(t) + if _, ok := got.header["Authorization"]; ok { + t.Error("Authorization sent without an API key") + } + body := sentBody(t, got) + if _, ok := body["response_format"]; ok { + t.Error("response_format sent in prompt_only mode") + } + system := body["messages"].([]any)[0].(map[string]any) + if system["role"] != "system" || !strings.HasPrefix(system["content"].(string), schemaInstruction) { + t.Errorf("system message = %v", system) + } +} + +func TestGenerateWithoutSchemaReturnsText(t *testing.T) { + vs := newVendorServer(t, reply{status: 200, fixture: "ollama_chat_text.json"}) + p := newProvider(t, vs.URL+"/v1", PromptOnly, nil) + resp, err := p.Generate(context.Background(), llm.Request{Messages: []llm.Message{{Role: llm.RoleUser, Content: "Dinner?"}}}) + if err != nil { + t.Fatal(err) + } + if resp.Text != "Try a lemon orzo bake tonight." || resp.JSON != nil { + t.Errorf("Text, JSON = %q, %q", resp.Text, resp.JSON) + } + body := sentBody(t, vs.only(t)) + if msgs := body["messages"].([]any); len(msgs) != 1 { + t.Errorf("messages = %v, want only the user message", msgs) + } + if _, ok := body["response_format"]; ok { + t.Error("response_format sent without a schema") + } +} + +func TestGenerateBadOutput(t *testing.T) { + cases := []struct { + name string + fixture string + mode StructuredOutput + }{ + {"refusal", "openai_chat_refusal.json", JSONSchema}, + {"truncated prompt_only", "ollama_chat_truncated.json", PromptOnly}, + {"truncated json_object", "ollama_chat_truncated.json", JSONObject}, + {"prose in json_object", "ollama_chat_text.json", JSONObject}, + {"prose in prompt_only", "ollama_chat_text.json", PromptOnly}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + vs := newVendorServer(t, reply{status: 200, fixture: c.fixture}) + p := newProvider(t, vs.URL, c.mode, nil) + _, err := p.Generate(context.Background(), schemaRequest()) + if !errors.Is(err, llm.ErrBadOutput) { + t.Fatalf("err = %v, want ErrBadOutput", err) + } + }) + } +} + +func TestListModels(t *testing.T) { + cases := []struct { + fixture string + base string + want []llm.Model + }{ + {"openai_models.json", "/v1", []llm.Model{ + {ID: "gpt-4o-2024-08-06", DisplayName: "gpt-4o-2024-08-06"}, + {ID: "gpt-4o-mini", DisplayName: "gpt-4o-mini"}, + }}, + {"openrouter_models.json", "/api/v1", []llm.Model{ + {ID: "openai/gpt-4o-mini", DisplayName: "OpenAI: GPT-4o-mini"}, + {ID: "anthropic/claude-sonnet-5", DisplayName: "Anthropic: Claude Sonnet 5"}, + }}, + {"ollama_models.json", "/v1", []llm.Model{ + {ID: "llama3.2:latest", DisplayName: "llama3.2:latest"}, + {ID: "qwen2.5:7b", DisplayName: "qwen2.5:7b"}, + }}, + } + for _, c := range cases { + t.Run(c.fixture, func(t *testing.T) { + vs := newVendorServer(t, reply{status: 200, fixture: c.fixture}) + p := newProvider(t, vs.URL+c.base, JSONSchema, map[string]string{"X-Title": "MealQuest"}) + models, err := p.ListModels(context.Background()) + if err != nil { + t.Fatal(err) + } + if len(models) != len(c.want) { + t.Fatalf("models = %+v", models) + } + for i := range c.want { + if models[i] != c.want[i] { + t.Errorf("model %d = %+v, want %+v", i, models[i], c.want[i]) + } + } + got := vs.only(t) + if got.method != http.MethodGet || got.path != c.base+"/models" { + t.Errorf("request = %s %s", got.method, got.path) + } + if got.header.Get("Authorization") != "Bearer "+testKey || got.header.Get("X-Title") != "MealQuest" { + t.Errorf("headers = %v", got.header) + } + }) + } +} + +func TestVerify(t *testing.T) { + ok := newVendorServer(t, reply{status: 200, fixture: "openai_models.json"}) + if err := newProvider(t, ok.URL, JSONSchema, nil).Verify(context.Background()); err != nil { + t.Fatalf("Verify() = %v", err) + } + if got := ok.only(t); got.path != "/models" { + t.Errorf("Verify called %s", got.path) + } + bad := newVendorServer(t, reply{status: 401, fixture: "openai_error_invalid_key.json"}) + if err := newProvider(t, bad.URL, JSONSchema, nil).Verify(context.Background()); !errors.Is(err, llm.ErrAuth) { + t.Fatalf("Verify() = %v, want ErrAuth", err) + } +} + +// assertNoSecret fails if err's text holds the key or Authorization value. +func assertNoSecret(t *testing.T, err error) { + t.Helper() + for _, secret := range []string{testKey, "Bearer " + testKey, testKey[:12]} { + if strings.Contains(err.Error(), secret) { + t.Fatalf("error leaks %q: %v", secret, err) + } + } +} + +func TestErrorMapping(t *testing.T) { + cases := []struct { + name string + reply reply + want error + check func(t *testing.T, err error) + }{ + {name: "401 OpenAI", reply: reply{status: 401, fixture: "openai_error_invalid_key.json"}, want: llm.ErrAuth}, + {name: "403", reply: reply{status: 403, fixture: "openai_error_invalid_key.json"}, want: llm.ErrAuth}, + {name: "429 seconds OpenRouter", reply: reply{status: 429, header: map[string]string{"Retry-After": "17"}, fixture: "openrouter_error_rate_limited.json"}, want: llm.ErrRateLimited, + check: func(t *testing.T, err error) { + var rl *llm.RateLimitedError + if !errors.As(err, &rl) || rl.RetryAfter != 17*time.Second { + t.Fatalf("RetryAfter from %v", err) + } + }}, + {name: "429 HTTP-date", reply: reply{status: 429, header: map[string]string{"Retry-After": time.Now().Add(90 * time.Second).UTC().Format(http.TimeFormat)}, fixture: "openrouter_error_rate_limited.json"}, want: llm.ErrRateLimited, + check: func(t *testing.T, err error) { + var rl *llm.RateLimitedError + if !errors.As(err, &rl) || rl.RetryAfter < 85*time.Second || rl.RetryAfter > 91*time.Second { + t.Fatalf("RetryAfter from %v", err) + } + }}, + {name: "429 no Retry-After", reply: reply{status: 429, fixture: "openrouter_error_rate_limited.json"}, want: llm.ErrRateLimited, + check: func(t *testing.T, err error) { + var rl *llm.RateLimitedError + if !errors.As(err, &rl) || rl.RetryAfter != 0 { + t.Fatalf("RetryAfter from %v", err) + } + }}, + {name: "408", reply: reply{status: 408}, want: llm.ErrTimeout}, + {name: "404 Ollama", reply: reply{status: 404, fixture: "ollama_error_not_found.json"}, want: llm.ErrProvider, + check: func(t *testing.T, err error) { + var pe *llm.ProviderError + if !errors.As(err, &pe) || pe.Status() != 404 || !strings.Contains(pe.Body(), `model \"llama9\" not found`) { + t.Fatalf("ProviderError from %v", err) + } + }}, + {name: "500 echoing the key", reply: reply{status: 500, fixture: "openai_error_server.json"}, want: llm.ErrProvider, + check: func(t *testing.T, err error) { + var pe *llm.ProviderError + if !errors.As(err, &pe) || pe.Status() != 500 || !strings.Contains(pe.Body(), "Authorization: Bearer [REDACTED]") { + t.Fatalf("ProviderError from %v", err) + } + }}, + {name: "400 echoing the key", reply: reply{status: 400, fixture: "openai_error_invalid_key.json"}, want: llm.ErrProvider}, + {name: "200 OpenRouter upstream error", reply: reply{status: 200, fixture: "openrouter_chat_upstream_error.json"}, want: llm.ErrProvider, + check: func(t *testing.T, err error) { + var pe *llm.ProviderError + if !errors.As(err, &pe) || !strings.Contains(pe.Body(), "Upstream error from provider") { + t.Fatalf("ProviderError from %v", err) + } + }}, + } + all := []error{llm.ErrAuth, llm.ErrRateLimited, llm.ErrTimeout, llm.ErrBadOutput, llm.ErrProvider} + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + vs := newVendorServer(t, c.reply) + p := newProvider(t, vs.URL+"/v1", JSONSchema, nil) + _, genErr := p.Generate(context.Background(), schemaRequest()) + errs := []error{genErr} + if c.reply.status != 200 { + _, listErr := p.ListModels(context.Background()) + errs = append(errs, listErr) + } + for _, err := range errs { + for _, sentinel := range all { + if errors.Is(err, sentinel) != (sentinel == c.want) { + t.Fatalf("errors.Is(%v, %v) = %v", err, sentinel, !(sentinel == c.want)) + } + } + assertNoSecret(t, err) + if c.check != nil { + c.check(t, err) + } + } + }) + } +} + +func TestTimeouts(t *testing.T) { + vs := newVendorServer(t, reply{status: 200, fixture: "openai_chat_json_schema.json", delay: 5 * time.Second}) + p := newProvider(t, vs.URL, JSONSchema, nil) + + req := schemaRequest() + req.Timeout = 50 * time.Millisecond + start := time.Now() + _, err := p.Generate(context.Background(), req) + if !errors.Is(err, llm.ErrTimeout) { + t.Fatalf("Request.Timeout: err = %v, want ErrTimeout", err) + } + if elapsed := time.Since(start); elapsed > 2*time.Second { + t.Fatalf("Request.Timeout not honoured: took %v", elapsed) + } + assertNoSecret(t, err) + + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + if _, err := p.ListModels(ctx); !errors.Is(err, llm.ErrTimeout) { + t.Fatalf("context deadline: err = %v, want ErrTimeout", err) + } + + ctx, cancel = context.WithCancel(context.Background()) + cancel() + if _, err := p.Generate(ctx, schemaRequest()); !errors.Is(err, context.Canceled) || errors.Is(err, llm.ErrTimeout) { + t.Fatalf("cancelled context: err = %v, want context.Canceled only", err) + } +} + +func TestTransportErrorHasNoSecret(t *testing.T) { + vs := newVendorServer(t, reply{status: 200}) + base := vs.URL + vs.Close() + _, err := newProvider(t, base, JSONSchema, nil).Generate(context.Background(), schemaRequest()) + if err == nil { + t.Fatal("Generate against a closed server succeeded") + } + assertNoSecret(t, err) +} + +func TestGenerateRejectsInvalidRequests(t *testing.T) { + vs := newVendorServer(t, reply{status: 200, fixture: "ollama_chat_text.json"}) + p := newProvider(t, vs.URL, JSONSchema, nil) + if _, err := p.Generate(context.Background(), llm.Request{}); err == nil { + t.Error("Generate accepted a request with no messages") + } + noModel, err := New(Config{BaseURL: vs.URL, APIKey: testKey, StructuredOutput: JSONSchema}) + if err != nil { + t.Fatal(err) + } + if _, err := noModel.Generate(context.Background(), schemaRequest()); err == nil { + t.Error("Generate succeeded with no model configured") + } + vs.mu.Lock() + defer vs.mu.Unlock() + if len(vs.got) != 0 { + t.Errorf("invalid requests reached the vendor %d times", len(vs.got)) + } +} + +func TestNewRejectsBadConfig(t *testing.T) { + cases := map[string]Config{ + "empty base": {StructuredOutput: JSONSchema}, + "relative base": {BaseURL: "api.openai.com/v1", StructuredOutput: JSONSchema}, + "ftp base": {BaseURL: "ftp://example.com", StructuredOutput: JSONSchema}, + "credentials in URL": {BaseURL: "https://user:" + testKey + "@example.com/v1", StructuredOutput: JSONSchema}, + "query in URL": {BaseURL: "https://example.com/v1?key=x", StructuredOutput: JSONSchema}, + "unknown mode": {BaseURL: "https://example.com/v1", StructuredOutput: "json"}, + "no mode": {BaseURL: "https://example.com/v1"}, + "authorization hdr": {BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: map[string]string{"authorization": "Bearer x"}}, + "content-type hdr": {BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: map[string]string{"Content-Type": "text/plain"}}, + "header newline": {BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: map[string]string{"X-Title": "a\r\nb"}}, + "header bad name": {BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: map[string]string{"X Title": "a"}}, + } + for name, cfg := range cases { + cfg.APIKey = testKey + _, err := New(cfg) + if err == nil { + t.Errorf("%s: New() = nil error", name) + continue + } + assertNoSecret(t, err) + } +} + +func TestFirstJSONObject(t *testing.T) { + cases := map[string]string{ + `{"a":1}`: `{"a":1}`, + "text {not json} then {\"b\":2}": `{"b":2}`, + "```json\n{\"c\":{\"d\":3}}\n```": `{"c":{"d":3}}`, + `{"a":1} {"b":2}`: `{"a":1}`, + "no object": ``, + `{"open":`: ``, + `[{"in":"array"}]`: `{"in":"array"}`, + } + for in, want := range cases { + if got := string(firstJSONObject(in)); got != want { + t.Errorf("firstJSONObject(%q) = %q, want %q", in, got, want) + } + } +} + +func TestRetryAfter(t *testing.T) { + now := time.Date(2026, 10, 3, 12, 0, 0, 0, time.UTC) + cases := map[string]time.Duration{ + "": 0, + "30": 30 * time.Second, + " 5 ": 5 * time.Second, + "-3": 0, + "soon": 0, + "Sat, 03 Oct 2026 12:02:00 GMT": 2 * time.Minute, + "Sat, 03 Oct 2026 11:59:00 GMT": 0, + } + for in, want := range cases { + if got := retryAfter(in, now); got != want { + t.Errorf("retryAfter(%q) = %v, want %v", in, got, want) + } + } +} diff --git a/internal/ai/llm/openai/testdata/ollama_chat_prompt_only.json b/internal/ai/llm/openai/testdata/ollama_chat_prompt_only.json new file mode 100644 index 0000000..f235cd4 --- /dev/null +++ b/internal/ai/llm/openai/testdata/ollama_chat_prompt_only.json @@ -0,0 +1,18 @@ +{ + "id": "chatcmpl-417", + "object": "chat.completion", + "created": 1741570322, + "model": "llama3.2", + "system_fingerprint": "fp_ollama", + "choices": [ + { + "index": 0, + "message": { + "role": "assistant", + "content": "Sure! Here is a dinner {idea} for you:\n```json\n{\"dinners\": [{\"name\": \"Crock pot chili\", \"why\": \"A Quick dinner like your {Regular} stew.\"}]}\n```\nEnjoy {\"not\": \"this one\"}!" + }, + "finish_reason": "stop" + } + ], + "usage": {"prompt_tokens": 512, "completion_tokens": 58, "total_tokens": 570} +} diff --git a/internal/ai/llm/openai/testdata/ollama_chat_text.json b/internal/ai/llm/openai/testdata/ollama_chat_text.json new file mode 100644 index 0000000..cebe0de --- /dev/null +++ b/internal/ai/llm/openai/testdata/ollama_chat_text.json @@ -0,0 +1,11 @@ +{ + "id": "chatcmpl-419", + "object": "chat.completion", + "created": 1741570340, + "model": "llama3.2", + "system_fingerprint": "fp_ollama", + "choices": [ + {"index": 0, "message": {"role": "assistant", "content": "Try a lemon orzo bake tonight."}, "finish_reason": "stop"} + ], + "usage": {"prompt_tokens": 40, "completion_tokens": 9, "total_tokens": 49} +} diff --git a/internal/ai/llm/openai/testdata/ollama_chat_truncated.json b/internal/ai/llm/openai/testdata/ollama_chat_truncated.json new file mode 100644 index 0000000..ff036f8 --- /dev/null +++ b/internal/ai/llm/openai/testdata/ollama_chat_truncated.json @@ -0,0 +1,15 @@ +{ + "id": "chatcmpl-418", + "object": "chat.completion", + "created": 1741570330, + "model": "llama3.2", + "system_fingerprint": "fp_ollama", + "choices": [ + { + "index": 0, + "message": {"role": "assistant", "content": "{\"dinners\": [{\"name\": \"Crock pot ch"}, + "finish_reason": "length" + } + ], + "usage": {"prompt_tokens": 512, "completion_tokens": 16, "total_tokens": 528} +} diff --git a/internal/ai/llm/openai/testdata/ollama_error_not_found.json b/internal/ai/llm/openai/testdata/ollama_error_not_found.json new file mode 100644 index 0000000..137e3ba --- /dev/null +++ b/internal/ai/llm/openai/testdata/ollama_error_not_found.json @@ -0,0 +1 @@ +{"error": {"message": "model \"llama9\" not found, try pulling it first", "type": "api_error", "param": null, "code": null}} diff --git a/internal/ai/llm/openai/testdata/ollama_models.json b/internal/ai/llm/openai/testdata/ollama_models.json new file mode 100644 index 0000000..2421a5e --- /dev/null +++ b/internal/ai/llm/openai/testdata/ollama_models.json @@ -0,0 +1,7 @@ +{ + "object": "list", + "data": [ + {"id": "llama3.2:latest", "object": "model", "created": 1741000000, "owned_by": "library"}, + {"id": "qwen2.5:7b", "object": "model", "created": 1740000000, "owned_by": "library"} + ] +} diff --git a/internal/ai/llm/openai/testdata/openai_chat_json_schema.json b/internal/ai/llm/openai/testdata/openai_chat_json_schema.json new file mode 100644 index 0000000..3d596d1 --- /dev/null +++ b/internal/ai/llm/openai/testdata/openai_chat_json_schema.json @@ -0,0 +1,28 @@ +{ + "id": "chatcmpl-B9MHDbslfkBeAs8l4bebGdFOJ6PeG", + "object": "chat.completion", + "created": 1741570283, + "model": "gpt-4o-2024-08-06", + "choices": [ + { + "index": 0, + "message": { + "role": "assistant", + "content": "{\"dinners\":[{\"name\":\"Sheet-pan gnocchi\",\"why\":\"Like your Regular tray bake, but quicker.\"}]}", + "refusal": null, + "annotations": [] + }, + "logprobs": null, + "finish_reason": "stop" + } + ], + "usage": { + "prompt_tokens": 1117, + "completion_tokens": 46, + "total_tokens": 1163, + "prompt_tokens_details": {"cached_tokens": 0, "audio_tokens": 0}, + "completion_tokens_details": {"reasoning_tokens": 0, "audio_tokens": 0, "accepted_prediction_tokens": 0, "rejected_prediction_tokens": 0} + }, + "service_tier": "default", + "system_fingerprint": "fp_fc9f1d7035" +} diff --git a/internal/ai/llm/openai/testdata/openai_chat_refusal.json b/internal/ai/llm/openai/testdata/openai_chat_refusal.json new file mode 100644 index 0000000..d00903b --- /dev/null +++ b/internal/ai/llm/openai/testdata/openai_chat_refusal.json @@ -0,0 +1,16 @@ +{ + "id": "chatcmpl-B9MHDbslfkBeAs8l4bebGdFOJ6PeH", + "object": "chat.completion", + "created": 1741570290, + "model": "gpt-4o-2024-08-06", + "choices": [ + { + "index": 0, + "message": {"role": "assistant", "content": null, "refusal": "I'm sorry, I can't help with that request.", "annotations": []}, + "logprobs": null, + "finish_reason": "stop" + } + ], + "usage": {"prompt_tokens": 210, "completion_tokens": 11, "total_tokens": 221}, + "system_fingerprint": "fp_fc9f1d7035" +} diff --git a/internal/ai/llm/openai/testdata/openai_error_invalid_key.json b/internal/ai/llm/openai/testdata/openai_error_invalid_key.json new file mode 100644 index 0000000..f82a0ff --- /dev/null +++ b/internal/ai/llm/openai/testdata/openai_error_invalid_key.json @@ -0,0 +1,8 @@ +{ + "error": { + "message": "Incorrect API key provided: {{KEY}}. You can find your API key at https://platform.openai.com/account/api-keys.", + "type": "invalid_request_error", + "param": null, + "code": "invalid_api_key" + } +} diff --git a/internal/ai/llm/openai/testdata/openai_error_server.json b/internal/ai/llm/openai/testdata/openai_error_server.json new file mode 100644 index 0000000..006fc65 --- /dev/null +++ b/internal/ai/llm/openai/testdata/openai_error_server.json @@ -0,0 +1 @@ +{"error": {"message": "The server had an error while processing your request. Authorization: Bearer {{KEY}}", "type": "server_error", "param": null, "code": null}} diff --git a/internal/ai/llm/openai/testdata/openai_models.json b/internal/ai/llm/openai/testdata/openai_models.json new file mode 100644 index 0000000..c5dda95 --- /dev/null +++ b/internal/ai/llm/openai/testdata/openai_models.json @@ -0,0 +1,7 @@ +{ + "object": "list", + "data": [ + {"id": "gpt-4o-2024-08-06", "object": "model", "created": 1722814719, "owned_by": "system"}, + {"id": "gpt-4o-mini", "object": "model", "created": 1721172741, "owned_by": "system"} + ] +} diff --git a/internal/ai/llm/openai/testdata/openrouter_chat_json_object.json b/internal/ai/llm/openai/testdata/openrouter_chat_json_object.json new file mode 100644 index 0000000..a1fddac --- /dev/null +++ b/internal/ai/llm/openai/testdata/openrouter_chat_json_object.json @@ -0,0 +1,22 @@ +{ + "id": "gen-1741570300-xQmT9vWb4kR2nP7sL3aE", + "provider": "OpenAI", + "model": "openai/gpt-4o-mini", + "object": "chat.completion", + "created": 1741570300, + "choices": [ + { + "logprobs": null, + "finish_reason": "stop", + "native_finish_reason": "stop", + "index": 0, + "message": { + "role": "assistant", + "content": "{\n \"dinners\": [{\"name\": \"Chicken tinga tacos\", \"why\": \"Close to your taco-night Regular.\"}]\n}", + "refusal": null, + "reasoning": null + } + } + ], + "usage": {"prompt_tokens": 842, "completion_tokens": 31, "total_tokens": 873} +} diff --git a/internal/ai/llm/openai/testdata/openrouter_chat_upstream_error.json b/internal/ai/llm/openai/testdata/openrouter_chat_upstream_error.json new file mode 100644 index 0000000..6167272 --- /dev/null +++ b/internal/ai/llm/openai/testdata/openrouter_chat_upstream_error.json @@ -0,0 +1,5 @@ +{ + "id": "gen-1741570311-aB3dE5fG7hJ9kL1mN2pQ", + "error": {"code": 502, "message": "Upstream error from provider", "metadata": {"provider_name": "Lambda", "raw": "upstream returned 502"}}, + "user_id": "user_2abcDEF" +} diff --git a/internal/ai/llm/openai/testdata/openrouter_error_rate_limited.json b/internal/ai/llm/openai/testdata/openrouter_error_rate_limited.json new file mode 100644 index 0000000..bc12a1e --- /dev/null +++ b/internal/ai/llm/openai/testdata/openrouter_error_rate_limited.json @@ -0,0 +1 @@ +{"error": {"code": 429, "message": "Rate limit exceeded: free-models-per-min. Key {{KEY}}", "metadata": {"headers": {"X-RateLimit-Limit": "20", "X-RateLimit-Remaining": "0"}}}} diff --git a/internal/ai/llm/openai/testdata/openrouter_models.json b/internal/ai/llm/openai/testdata/openrouter_models.json new file mode 100644 index 0000000..741effd --- /dev/null +++ b/internal/ai/llm/openai/testdata/openrouter_models.json @@ -0,0 +1,22 @@ +{ + "data": [ + { + "id": "openai/gpt-4o-mini", + "canonical_slug": "openai/gpt-4o-mini", + "name": "OpenAI: GPT-4o-mini", + "created": 1721260800, + "description": "GPT-4o mini is OpenAI's newest model after GPT-4 Omni.", + "context_length": 128000, + "architecture": {"modality": "text+image->text", "input_modalities": ["text", "image"], "output_modalities": ["text"], "tokenizer": "GPT"}, + "pricing": {"prompt": "0.00000015", "completion": "0.0000006"}, + "supported_parameters": ["max_tokens", "temperature", "response_format", "structured_outputs"] + }, + { + "id": "anthropic/claude-sonnet-5", + "name": "Anthropic: Claude Sonnet 5", + "created": 1767225600, + "context_length": 200000, + "supported_parameters": ["max_tokens", "temperature"] + } + ] +} -- 2.55.0 From 1f4356cd177f50a56c43a35b42002a303ce854fa Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sat, 3 Oct 2026 16:28:21 -0400 Subject: [PATCH 02/12] Keep base URL out of config errors and always keep a bounded error excerpt New no longer echoes the supplied base URL in any configuration error, so a credential in a rejected URL's query or fragment cannot leak. llm.NewProviderError now takes the single credential sent with the request ("" when none) and always keeps a body excerpt bounded to MaxBodyExcerpt bytes, redacting the credential first. Keyless Ollama errors therefore keep the vendor's explanation, e.g. model not found. Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/errors.go | 26 ++++------- internal/ai/llm/errors_test.go | 36 +++++++-------- internal/ai/llm/openai/openai.go | 12 ++--- internal/ai/llm/openai/openai_test.go | 64 ++++++++++++++++++++++++++- 4 files changed, 95 insertions(+), 43 deletions(-) diff --git a/internal/ai/llm/errors.go b/internal/ai/llm/errors.go index be8e922..555b99a 100644 --- a/internal/ai/llm/errors.go +++ b/internal/ai/llm/errors.go @@ -56,23 +56,15 @@ type ProviderError struct { body string } -// NewProviderError returns the error for an unexpected HTTP status. -// secrets must hold the API key (and any other credential sent with the -// request). Every occurrence of each non-empty secret in body is replaced -// before the body is cut to MaxBodyExcerpt bytes, so no part of a secret -// survives. If secrets holds no non-empty value the body cannot be -// redacted, so no excerpt is kept at all. -func NewProviderError(status int, body []byte, secrets []string) *ProviderError { +// NewProviderError returns the error for an unexpected HTTP status. It +// always keeps a body excerpt of at most MaxBodyExcerpt bytes. secret is the +// credential sent with the request, or "" when the request sent none; every +// occurrence of a non-empty secret in body is replaced before the body is +// cut, so no part of it survives. +func NewProviderError(status int, body []byte, secret string) *ProviderError { s := string(body) - redactedAny := false - for _, secret := range secrets { - if secret != "" { - s = strings.ReplaceAll(s, secret, redacted) - redactedAny = true - } - } - if !redactedAny { - return &ProviderError{status: status} + if secret != "" { + s = strings.ReplaceAll(s, secret, redacted) } s = strings.TrimSpace(s) if len(s) > MaxBodyExcerpt { @@ -89,7 +81,7 @@ func NewProviderError(status int, body []byte, secrets []string) *ProviderError func (e *ProviderError) Status() int { return e.status } // Body returns the redacted, truncated response-body excerpt; it is empty -// when the vendor sent no body or no secret was given to redact it with. +// when the vendor sent no body. func (e *ProviderError) Body() string { return e.body } func (e *ProviderError) Error() string { diff --git a/internal/ai/llm/errors_test.go b/internal/ai/llm/errors_test.go index 321f887..8911611 100644 --- a/internal/ai/llm/errors_test.go +++ b/internal/ai/llm/errors_test.go @@ -46,7 +46,7 @@ func TestRateLimitedError(t *testing.T) { } func TestProviderError(t *testing.T) { - err := fmt.Errorf("gemini: %w", NewProviderError(503, []byte(` {"error":"overloaded"} `), []string{"sk-test"})) + err := fmt.Errorf("gemini: %w", NewProviderError(503, []byte(` {"error":"overloaded"} `), "sk-test")) if !errors.Is(err, ErrProvider) { t.Fatal("errors.Is(err, ErrProvider) = false") } @@ -63,7 +63,7 @@ func TestProviderError(t *testing.T) { if got := err.Error(); got != `gemini: llm: provider error: HTTP 503: {"error":"overloaded"}` { t.Fatalf("Error() = %q", got) } - if got := NewProviderError(500, nil, []string{"sk-test"}).Error(); got != "llm: provider error: HTTP 500" { + if got := NewProviderError(500, nil, "sk-test").Error(); got != "llm: provider error: HTTP 500" { t.Fatalf("Error() without body = %q", got) } } @@ -71,7 +71,7 @@ func TestProviderError(t *testing.T) { func TestProviderErrorRedactsSecrets(t *testing.T) { const key = "sk-live-0123456789abcdef" body := []byte(`{"error":"invalid key sk-live-0123456789abcdef","echo":"Bearer sk-live-0123456789abcdef"}`) - pe := NewProviderError(400, body, []string{key, ""}) + pe := NewProviderError(400, body, key) for _, s := range []string{pe.Error(), pe.Body()} { if strings.Contains(s, key) { t.Fatalf("secret leaked: %q", s) @@ -88,7 +88,7 @@ func TestProviderErrorRedactsBeforeTruncating(t *testing.T) { // The key starts just before the excerpt limit, so truncating first // would leave its prefix in the excerpt. body := strings.Repeat("x", MaxBodyExcerpt-5) + key - pe := NewProviderError(500, []byte(body), []string{key}) + pe := NewProviderError(500, []byte(body), key) if strings.Contains(pe.Body(), "sk-se") { t.Fatalf("secret prefix leaked: %q", pe.Body()) } @@ -96,26 +96,24 @@ func TestProviderErrorRedactsBeforeTruncating(t *testing.T) { func TestProviderErrorTruncatesOnRuneBoundary(t *testing.T) { body := strings.Repeat("a", MaxBodyExcerpt-1) + "é" + strings.Repeat("b", 100) - pe := NewProviderError(500, []byte(body), []string{"sk-test"}) + pe := NewProviderError(500, []byte(body), "sk-test") want := strings.Repeat("a", MaxBodyExcerpt-1) + "…" if pe.Body() != want { t.Fatalf("Body() has %d bytes, want %d", len(pe.Body()), len(want)) } } -func TestProviderErrorOmitsBodyWithoutSecret(t *testing.T) { - const key = "sk-live-0123456789abcdef" - body := []byte(`{"error":"invalid key sk-live-0123456789abcdef"}`) - for _, secrets := range [][]string{nil, {}, {""}} { - pe := NewProviderError(401, body, secrets) - if pe.Body() != "" { - t.Fatalf("secrets %q: Body() = %q, want empty", secrets, pe.Body()) - } - if got := pe.Error(); got != "llm: provider error: HTTP 401" { - t.Fatalf("secrets %q: Error() = %q", secrets, got) - } - if strings.Contains(pe.Error(), key) { - t.Fatalf("secrets %q: secret leaked: %q", secrets, pe.Error()) - } +func TestProviderErrorKeepsBoundedBodyWithoutSecret(t *testing.T) { + body := []byte(` {"error":"model \"llama9\" not found"} `) + pe := NewProviderError(404, body, "") + if pe.Body() != `{"error":"model \"llama9\" not found"}` { + t.Fatalf("Body() = %q", pe.Body()) + } + if got := pe.Error(); got != `llm: provider error: HTTP 404: {"error":"model \"llama9\" not found"}` { + t.Fatalf("Error() = %q", got) + } + long := NewProviderError(500, []byte(strings.Repeat("x", 4*MaxBodyExcerpt)), "") + if want := strings.Repeat("x", MaxBodyExcerpt) + "…"; long.Body() != want { + t.Fatalf("Body() has %d bytes, want %d", len(long.Body()), len(want)) } } diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index 3e7c4c8..0d8c38d 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -74,15 +74,17 @@ var _ llm.Provider = (*Provider)(nil) // New validates cfg and returns a Provider. func New(cfg Config) (*Provider, error) { + // Base URL errors never echo the URL: a rejected one may carry a + // credential. u, err := url.Parse(cfg.BaseURL) if err != nil { return nil, errors.New("openai: base URL does not parse") } if (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" { - return nil, fmt.Errorf("openai: base URL %q must be an absolute http or https URL", u.Redacted()) + return nil, errors.New("openai: base URL must be an absolute http or https URL") } if u.User != nil || u.RawQuery != "" || u.Fragment != "" { - return nil, fmt.Errorf("openai: base URL %q must not carry credentials, a query or a fragment", u.Redacted()) + return nil, errors.New("openai: base URL must not carry credentials, a query or a fragment") } switch cfg.StructuredOutput { case JSONSchema, JSONObject, PromptOnly: @@ -217,7 +219,7 @@ func (p *Provider) Generate(ctx context.Context, req llm.Request) (llm.Response, } if len(cr.Error) > 0 && string(cr.Error) != "null" { // OpenRouter can report an upstream failure in a 2xx body. - return llm.Response{}, fmt.Errorf("openai: generate: %w", llm.NewProviderError(status, raw, p.secrets())) + return llm.Response{}, fmt.Errorf("openai: generate: %w", llm.NewProviderError(status, raw, p.apiKey)) } if len(cr.Choices) == 0 { return llm.Response{}, errors.New("openai: generate: response has no choices") @@ -319,8 +321,6 @@ func (p *Provider) Verify(ctx context.Context) error { return err } -func (p *Provider) secrets() []string { return []string{p.apiKey} } - // do sends one request and returns the body and status of a 2xx response. // Any other status becomes the matching llm typed error. func (p *Provider) do(ctx context.Context, method, path string, payload []byte) ([]byte, int, error) { @@ -376,7 +376,7 @@ func (p *Provider) statusError(resp *http.Response, body []byte) error { case http.StatusRequestTimeout: return fmt.Errorf("HTTP %d: %w", resp.StatusCode, llm.ErrTimeout) default: - return llm.NewProviderError(resp.StatusCode, body, p.secrets()) + return llm.NewProviderError(resp.StatusCode, body, p.apiKey) } } diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index 2f13e5c..a7bd6c6 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -498,6 +498,64 @@ func TestTransportErrorHasNoSecret(t *testing.T) { assertNoSecret(t, err) } +// TestKeyedErrorsNeverLeakKey drives every error path of the keyed OpenAI +// and OpenRouter shapes with vendor bodies that echo the key. +func TestKeyedErrorsNeverLeakKey(t *testing.T) { + shapes := map[string]struct { + path string + headers map[string]string + }{ + "OpenAI": {path: "/v1"}, + "OpenRouter": {path: "/api/v1", headers: map[string]string{"HTTP-Referer": "https://mealquest.example", "X-Title": "MealQuest"}}, + } + fixtures := []string{"openai_error_invalid_key.json", "openai_error_server.json", "openrouter_error_rate_limited.json"} + statuses := []int{400, 401, 403, 404, 408, 429, 500, 502, 503} + for name, shape := range shapes { + for _, fixture := range fixtures { + for _, status := range statuses { + vs := newVendorServer(t, reply{status: status, fixture: fixture}) + p := newProvider(t, vs.URL+shape.path, JSONSchema, shape.headers) + _, genErr := p.Generate(context.Background(), schemaRequest()) + _, listErr := p.ListModels(context.Background()) + verifyErr := p.Verify(context.Background()) + for _, err := range []error{genErr, listErr, verifyErr} { + if err == nil { + t.Fatalf("%s %s HTTP %d: nil error", name, fixture, status) + } + assertNoSecret(t, err) + var pe *llm.ProviderError + if errors.As(err, &pe) { + assertNoSecret(t, errors.New(pe.Body())) + } + } + } + } + } +} + +func TestKeylessOllamaErrorKeepsExcerpt(t *testing.T) { + vs := newVendorServer(t, reply{status: 404, fixture: "ollama_error_not_found.json"}) + p, err := New(Config{BaseURL: vs.URL + "/v1", Model: "llama9", StructuredOutput: PromptOnly}) + if err != nil { + t.Fatal(err) + } + _, err = p.Generate(context.Background(), schemaRequest()) + var pe *llm.ProviderError + if !errors.As(err, &pe) || pe.Status() != 404 { + t.Fatalf("err = %v, want a 404 ProviderError", err) + } + if !strings.Contains(pe.Body(), `model \"llama9\" not found`) || len(pe.Body()) > llm.MaxBodyExcerpt { + t.Fatalf("Body() = %q, want the bounded not-found explanation", pe.Body()) + } + if !strings.Contains(err.Error(), pe.Body()) { + t.Fatalf("Error() = %q, want it to carry the excerpt", err) + } + assertNoSecret(t, err) + if got := vs.only(t).header.Get("Authorization"); got != "" { + t.Fatalf("keyless request sent Authorization %q", got) + } +} + func TestGenerateRejectsInvalidRequests(t *testing.T) { vs := newVendorServer(t, reply{status: 200, fixture: "ollama_chat_text.json"}) p := newProvider(t, vs.URL, JSONSchema, nil) @@ -524,7 +582,11 @@ func TestNewRejectsBadConfig(t *testing.T) { "relative base": {BaseURL: "api.openai.com/v1", StructuredOutput: JSONSchema}, "ftp base": {BaseURL: "ftp://example.com", StructuredOutput: JSONSchema}, "credentials in URL": {BaseURL: "https://user:" + testKey + "@example.com/v1", StructuredOutput: JSONSchema}, - "query in URL": {BaseURL: "https://example.com/v1?key=x", StructuredOutput: JSONSchema}, + "key in query": {BaseURL: "https://example.com/v1?key=" + testKey, StructuredOutput: JSONSchema}, + "key in fragment": {BaseURL: "https://example.com/v1#" + testKey, StructuredOutput: JSONSchema}, + "key, relative base": {BaseURL: "example.com/v1?key=" + testKey, StructuredOutput: JSONSchema}, + "key, ftp base": {BaseURL: "ftp://example.com/v1?key=" + testKey, StructuredOutput: JSONSchema}, + "key, unparseable": {BaseURL: "https://example.com/%zz?key=" + testKey, StructuredOutput: JSONSchema}, "unknown mode": {BaseURL: "https://example.com/v1", StructuredOutput: "json"}, "no mode": {BaseURL: "https://example.com/v1"}, "authorization hdr": {BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: map[string]string{"authorization": "Bearer x"}}, -- 2.55.0 From 80d91eba70c2142e7177bd76937e9816210888e3 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sat, 3 Oct 2026 16:33:41 -0400 Subject: [PATCH 03/12] Keep the request URL out of transport errors and document fixture provenance A base URL path may hold the API key, and net/http's *url.Error names the full request URL. Transport failures now keep only the wrapped cause, with the key redacted, and unwrap only to llm.ErrTimeout or context.Canceled. The request-build error no longer echoes the URL either. A new test fails the exchange (connection refused and connection dropped) against a base URL whose path holds the key. testdata/README.md records each fixture's source in the vendor's public API documentation (Ollama: its openai middleware source), how it was sanitized, and which fields are not in the public example. Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/openai/openai.go | 51 ++++++++++++---- internal/ai/llm/openai/openai_test.go | 43 ++++++++++++++ internal/ai/llm/openai/testdata/README.md | 71 +++++++++++++++++++++++ 3 files changed, 155 insertions(+), 10 deletions(-) create mode 100644 internal/ai/llm/openai/testdata/README.md diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index 0d8c38d..0258c01 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -330,7 +330,8 @@ func (p *Provider) do(ctx context.Context, method, path string, payload []byte) } httpReq, err := http.NewRequestWithContext(ctx, method, p.base+path, reqBody) if err != nil { - return nil, 0, fmt.Errorf("build request: %w", err) + // The parse error names the URL, whose path may hold the API key. + return nil, 0, errors.New("build request: request URL is invalid") } for name, values := range p.headers { httpReq.Header[name] = values @@ -345,20 +346,20 @@ func (p *Provider) do(ctx context.Context, method, path string, payload []byte) httpResp, err := p.client.Do(httpReq) if err != nil { - return nil, 0, transportError(err) + return nil, 0, p.transportError(err) } defer httpResp.Body.Close() if httpResp.StatusCode < 200 || httpResp.StatusCode > 299 { body, err := io.ReadAll(io.LimitReader(httpResp.Body, maxErrorBody)) if err != nil { - return nil, httpResp.StatusCode, transportError(err) + return nil, httpResp.StatusCode, p.transportError(err) } return nil, httpResp.StatusCode, p.statusError(httpResp, body) } body, err := io.ReadAll(io.LimitReader(httpResp.Body, maxResponseBody+1)) if err != nil { - return nil, httpResp.StatusCode, transportError(err) + return nil, httpResp.StatusCode, p.transportError(err) } if len(body) > maxResponseBody { return nil, httpResp.StatusCode, fmt.Errorf("response body exceeds %d bytes", maxResponseBody) @@ -405,11 +406,41 @@ func isTimeout(err error) bool { return errors.Is(err, context.DeadlineExceeded) || (errors.As(err, &ne) && ne.Timeout()) } -// transportError reports a failure to complete the HTTP exchange. The -// underlying error names the URL, never the request headers. -func transportError(err error) error { - if isTimeout(err) { - return fmt.Errorf("%w: %w", llm.ErrTimeout, err) +// transportFailure is a failed HTTP exchange. It carries the cause's text +// with the request URL removed and the API key redacted, and unwraps only to +// the cause's category: llm.ErrTimeout, context.Canceled or nothing. +type transportFailure struct { + detail string + kind error +} + +func (e *transportFailure) Error() string { + if e.kind == llm.ErrTimeout { + return fmt.Sprintf("%s: transport: %s", llm.ErrTimeout, e.detail) } - return err + return "transport: " + e.detail +} + +func (e *transportFailure) Unwrap() error { return e.kind } + +// transportError reports a failure to complete the HTTP exchange. A +// *url.Error names the request URL, and a base URL path may hold the API +// key, so only the wrapped cause is kept, rendered with the key redacted. +func (p *Provider) transportError(err error) error { + var kind error + switch { + case isTimeout(err): + kind = llm.ErrTimeout + case errors.Is(err, context.Canceled): + kind = context.Canceled + } + var ue *url.Error + if errors.As(err, &ue) { + err = ue.Err + } + detail := err.Error() + if p.apiKey != "" { + detail = strings.ReplaceAll(detail, p.apiKey, "[REDACTED]") + } + return &transportFailure{detail: detail, kind: kind} } diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index a7bd6c6..8548d4b 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -8,6 +8,7 @@ import ( "io" "net/http" "net/http/httptest" + "net/url" "os" "path/filepath" "strings" @@ -498,6 +499,48 @@ func TestTransportErrorHasNoSecret(t *testing.T) { assertNoSecret(t, err) } +// TestTransportErrorWithKeyInBaseURLPath fails the exchange against a base +// URL whose path holds the key, which New accepts; net/http names the full +// request URL in its transport errors. +func TestTransportErrorWithKeyInBaseURLPath(t *testing.T) { + refused := newVendorServer(t, reply{status: 200}) + refusedBase := refused.URL + refused.Close() + + dropped := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + conn, _, err := w.(http.Hijacker).Hijack() + if err != nil { + t.Error(err) + return + } + conn.Close() + })) + t.Cleanup(dropped.Close) + + for name, base := range map[string]string{ + "connection refused": refusedBase + "/v1/" + testKey, + "connection dropped": dropped.URL + "/v1/" + testKey, + } { + p := newProvider(t, base, JSONSchema, nil) + _, genErr := p.Generate(context.Background(), schemaRequest()) + _, listErr := p.ListModels(context.Background()) + verifyErr := p.Verify(context.Background()) + for _, err := range []error{genErr, listErr, verifyErr} { + if err == nil { + t.Fatalf("%s: nil error", name) + } + assertNoSecret(t, err) + var ue *url.Error + if errors.As(err, &ue) { + t.Fatalf("%s: error chain exposes the request URL: %v", name, ue) + } + if errors.Is(err, llm.ErrTimeout) || errors.Is(err, llm.ErrProvider) { + t.Fatalf("%s: err = %v, want an untyped transport failure", name, err) + } + } + } +} + // TestKeyedErrorsNeverLeakKey drives every error path of the keyed OpenAI // and OpenRouter shapes with vendor bodies that echo the key. func TestKeyedErrorsNeverLeakKey(t *testing.T) { diff --git a/internal/ai/llm/openai/testdata/README.md b/internal/ai/llm/openai/testdata/README.md new file mode 100644 index 0000000..a058c82 --- /dev/null +++ b/internal/ai/llm/openai/testdata/README.md @@ -0,0 +1,71 @@ +# OpenAI-compatible adapter fixtures + +These are the vendor response bodies the contract tests in `openai_test.go` +replay. They are **not live captures**. Product AI is tested with fakes, and +live vendor calls are opt-in and never use credentials, so no request was +sent to a vendor to make them. Each fixture is adapted from the response +shape in that vendor's public API documentation or open source, then +sanitized: + +- IDs, timestamps, fingerprints and token counts are illustrative values in + the documented format, not values from a real account. +- Message content is replaced with MealQuest dinner text. +- No real credential appears anywhere. `{{KEY}}` is a placeholder the tests + replace with a fake key, to prove the adapter redacts a key echoed back in + a vendor body. + +The tests check the adapter against these bodies with independent +assertions on both the request sent (path, headers, `response_format`, +messages) and the response decoded. The request side does not depend on the +fixtures. + +Below, *not in the public example* marks fields or values that were added +for a test case and do not come from the vendor's documented example. + +## OpenAI + +Sources: API reference, "The chat completion object" and "List models" +(, +); the +Structured Outputs guide, for `refusal` +(); and the error +codes guide (). + +| Fixture | Shape | Not in the public example | +| --- | --- | --- | +| `openai_chat_json_schema.json` | Chat completion object with `usage` details, `service_tier`, `system_fingerprint`. | The JSON reply content. | +| `openai_chat_refusal.json` | Structured Outputs refusal: `content: null`, `refusal` set. | — | +| `openai_models.json` | `{object: list, data: [model]}`. | — | +| `openai_error_invalid_key.json` | Error object, `code: invalid_api_key`, message echoing the key. | The full key in the message; OpenAI masks most of it. Kept whole to test redaction. | +| `openai_error_server.json` | Error object, `type: server_error`. | The `Authorization: Bearer {{KEY}}` text; added to test redaction. | + +## OpenRouter + +Sources: API reference, chat completion response and errors +(, +), and "List available +models" (). + +| Fixture | Shape | Not in the public example | +| --- | --- | --- | +| `openrouter_chat_json_object.json` | Chat completion with `provider`, `native_finish_reason` and `message.reasoning`. | The JSON reply content. | +| `openrouter_chat_upstream_error.json` | An `error` object (`code`, `message`, `metadata.provider_name`, `metadata.raw`) in an HTTP 200 body, which OpenRouter documents for upstream failures. | `user_id`. | +| `openrouter_error_rate_limited.json` | Error object, `code: 429`, rate-limit `metadata`. | The `Key {{KEY}}` text; added to test redaction. | +| `openrouter_models.json` | `{data: [model]}` with `name`, `context_length`, `architecture`, `pricing`, `supported_parameters`. | The second model is a minimal entry, to test optional fields. | + +## Ollama + +Ollama's OpenAI compatibility page +() documents +requests but shows no response bodies. The shapes come from its `openai` +middleware source (): +`chatcmpl-` IDs, `system_fingerprint: fp_ollama`, `owned_by: library` on +models, and errors as `{error: {message, type: api_error, param, code}}`. + +| Fixture | Shape | Not in the public example | +| --- | --- | --- | +| `ollama_chat_text.json` | Plain chat completion. | — | +| `ollama_chat_prompt_only.json` | Chat completion. | The reply text: prose and stray braces around a fenced JSON object, to test prompt-only extraction. | +| `ollama_chat_truncated.json` | Chat completion with `finish_reason: length`. | The cut-off JSON reply. | +| `ollama_models.json` | `{object: list, data: [model]}`. | — | +| `ollama_error_not_found.json` | Middleware error object for a missing model. | — | -- 2.55.0 From adfd84810e51694eb1d5aeadf828480d43c170ce Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sat, 3 Oct 2026 16:40:29 -0400 Subject: [PATCH 04/12] Validate extra headers in New and honour a configured Accept header Config.Headers names must be RFC 9110 tokens and values must hold no control characters other than horizontal tab, so bad configuration fails at construction instead of on the first request. Headers the adapter or net/http manage (Authorization, Content-Type, Content-Length, Transfer-Encoding, Host, Connection) are rejected. A configured Accept header is sent as given; without one the adapter sends application/json. Header values are never echoed in errors. Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/openai/openai.go | 60 ++++++++++++++++++++++++--- internal/ai/llm/openai/openai_test.go | 43 +++++++++++++++++++ 2 files changed, 97 insertions(+), 6 deletions(-) diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index 0258c01..5a1b4e3 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -54,7 +54,9 @@ type Config struct { // need it. Model string // Headers are extra request headers, e.g. OpenRouter's HTTP-Referer and - // X-Title. They must not set Authorization or Content-Type. + // X-Title. An Accept header replaces the default "application/json". + // Headers the adapter or net/http set themselves (see managedHeaders) + // are rejected. Headers map[string]string // StructuredOutput is how JSON is requested when a Request has a Schema. StructuredOutput StructuredOutput @@ -91,17 +93,24 @@ func New(cfg Config) (*Provider, error) { default: return nil, fmt.Errorf("openai: unknown structured output mode %q", cfg.StructuredOutput) } - headers := make(http.Header, len(cfg.Headers)) + headers := make(http.Header, len(cfg.Headers)+1) for name, value := range cfg.Headers { + // Values are never echoed: one may carry a credential. + if !validHeaderName(name) { + return nil, fmt.Errorf("openai: extra header name %q is not a valid HTTP token", name) + } canonical := textproto.CanonicalMIMEHeaderKey(name) - if canonical == "Authorization" || canonical == "Content-Type" { + if managedHeaders[canonical] { return nil, fmt.Errorf("openai: extra header %q is set by the adapter", canonical) } - if name == "" || strings.ContainsAny(name, " \t\r\n:") || strings.ContainsAny(value, "\r\n") { - return nil, fmt.Errorf("openai: extra header %q is malformed", name) + if !validHeaderValue(value) { + return nil, fmt.Errorf("openai: extra header %q has a control character in its value", canonical) } headers.Set(canonical, value) } + if headers.Get("Accept") == "" { + headers.Set("Accept", "application/json") + } return &Provider{ base: strings.TrimRight(cfg.BaseURL, "/"), apiKey: cfg.APIKey, @@ -112,6 +121,46 @@ func New(cfg Config) (*Provider, error) { }, nil } +// managedHeaders are the request headers the adapter or net/http set; a +// configured value for one would be overwritten or ignored. +var managedHeaders = map[string]bool{ + "Authorization": true, + "Content-Type": true, + "Content-Length": true, + "Transfer-Encoding": true, + "Host": true, + "Connection": true, +} + +// validHeaderName reports whether name is an RFC 9110 field-name: a +// non-empty token of letters, digits and !#$%&'*+-.^_`|~. +func validHeaderName(name string) bool { + if name == "" { + return false + } + for i := 0; i < len(name); i++ { + c := name[i] + switch { + case 'a' <= c && c <= 'z', 'A' <= c && c <= 'Z', '0' <= c && c <= '9': + case strings.IndexByte("!#$%&'*+-.^_`|~", c) >= 0: + default: + return false + } + } + return true +} + +// validHeaderValue reports whether value is an RFC 9110 field-value: no +// control characters other than horizontal tab, so no CR, LF or DEL. +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 +} + // maxResponseBody caps a successful response body; a larger one is an error. const maxResponseBody = 16 << 20 @@ -339,7 +388,6 @@ func (p *Provider) do(ctx context.Context, method, path string, payload []byte) if payload != nil { httpReq.Header.Set("Content-Type", "application/json") } - httpReq.Header.Set("Accept", "application/json") if p.apiKey != "" { httpReq.Header.Set("Authorization", "Bearer "+p.apiKey) } diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index 8548d4b..30d2dfd 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -619,6 +619,40 @@ func TestGenerateRejectsInvalidRequests(t *testing.T) { } } +func TestNewRejectsBadHeaderWithoutEchoingValue(t *testing.T) { + _, err := New(Config{BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: map[string]string{"X-Title": testKey + "\u0019"}}) + if err == nil { + t.Fatal("New() = nil error") + } + assertNoSecret(t, err) +} + +func TestAcceptHeader(t *testing.T) { + for name, tc := range map[string]struct { + headers map[string]string + want string + }{ + "default": {nil, "application/json"}, + "configured": {map[string]string{"accept": "application/json, text/event-stream"}, "application/json, text/event-stream"}, + } { + vs := newVendorServer(t, reply{status: 200, fixture: "openai_models.json"}) + p := newProvider(t, vs.URL+"/v1", JSONSchema, tc.headers) + if _, err := p.ListModels(context.Background()); err != nil { + t.Fatalf("%s: %v", name, err) + } + if got := vs.only(t).header.Values("Accept"); len(got) != 1 || got[0] != tc.want { + t.Errorf("%s: Accept = %q, want %q", name, got, tc.want) + } + } +} + +func TestNewAcceptsValidHeaders(t *testing.T) { + headers := map[string]string{"X-Trace_ID.v2!#$%&'*+^`|~": "tab\tand obs-text \xe2\x9c\x93", "X-Empty": ""} + if _, err := New(Config{BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: headers}); err != nil { + t.Fatal(err) + } +} + func TestNewRejectsBadConfig(t *testing.T) { cases := map[string]Config{ "empty base": {StructuredOutput: JSONSchema}, @@ -636,6 +670,15 @@ func TestNewRejectsBadConfig(t *testing.T) { "content-type hdr": {BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: map[string]string{"Content-Type": "text/plain"}}, "header newline": {BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: map[string]string{"X-Title": "a\r\nb"}}, "header bad name": {BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: map[string]string{"X Title": "a"}}, + "header empty name": {BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: map[string]string{"": "a"}}, + "header bracket": {BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: map[string]string{"X[Trace]": "a"}}, + "header colon": {BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: map[string]string{"X-Title:": "a"}}, + "header non-ASCII": {BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: map[string]string{"X-Tïtle": "a"}}, + "header U+0019": {BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: map[string]string{"X-Title": "a\u0019b"}}, + "header NUL": {BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: map[string]string{"X-Title": "a\x00b"}}, + "header DEL": {BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: map[string]string{"X-Title": "a\x7fb"}}, + "host hdr": {BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: map[string]string{"host": "other.example"}}, + "content-length hdr": {BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: map[string]string{"Content-Length": "1"}}, } for name, cfg := range cases { cfg.APIKey = testKey -- 2.55.0 From 3ddc18fa0de5825f7fe8a8ed13dade4c1edf6308 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sat, 3 Oct 2026 16:45:44 -0400 Subject: [PATCH 05/12] Redact the API key from all config errors and reject empty URL delimiters An invalid extra-header name is no longer echoed, and every other New error that quotes caller-supplied text (structured output mode, header names) has the API key redacted first via a shared redactKey helper, also used for transport errors. A base URL containing "?" or "#" is rejected even when the query or fragment is empty, so endpoint paths can no longer land in either. Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/openai/openai.go | 27 +++++++++++++++++--------- internal/ai/llm/openai/openai_test.go | 28 +++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 9 deletions(-) diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index 5a1b4e3..f897689 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -85,26 +85,31 @@ func New(cfg Config) (*Provider, error) { if (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" { return nil, errors.New("openai: base URL must be an absolute http or https URL") } - if u.User != nil || u.RawQuery != "" || u.Fragment != "" { + // A trailing "?" or "#" parses to an empty query or fragment, yet the + // endpoint path appended to it would land in the query or fragment. + if u.User != nil || strings.ContainsAny(cfg.BaseURL, "?#") { return nil, errors.New("openai: base URL must not carry credentials, a query or a fragment") } + // Every other error that names caller-supplied text has the API key + // redacted from that text before it is quoted. switch cfg.StructuredOutput { case JSONSchema, JSONObject, PromptOnly: default: - return nil, fmt.Errorf("openai: unknown structured output mode %q", cfg.StructuredOutput) + return nil, fmt.Errorf("openai: unknown structured output mode %q", redactKey(string(cfg.StructuredOutput), cfg.APIKey)) } headers := make(http.Header, len(cfg.Headers)+1) for name, value := range cfg.Headers { - // Values are never echoed: one may carry a credential. + // Invalid names and all values are never echoed: either may carry a + // credential. if !validHeaderName(name) { - return nil, fmt.Errorf("openai: extra header name %q is not a valid HTTP token", name) + return nil, errors.New("openai: an extra header name is not a valid HTTP token") } canonical := textproto.CanonicalMIMEHeaderKey(name) if managedHeaders[canonical] { return nil, fmt.Errorf("openai: extra header %q is set by the adapter", canonical) } if !validHeaderValue(value) { - return nil, fmt.Errorf("openai: extra header %q has a control character in its value", canonical) + return nil, fmt.Errorf("openai: extra header %q has a control character in its value", redactKey(canonical, cfg.APIKey)) } headers.Set(canonical, value) } @@ -486,9 +491,13 @@ func (p *Provider) transportError(err error) error { if errors.As(err, &ue) { err = ue.Err } - detail := err.Error() - if p.apiKey != "" { - detail = strings.ReplaceAll(detail, p.apiKey, "[REDACTED]") + return &transportFailure{detail: redactKey(err.Error(), p.apiKey), kind: kind} +} + +// redactKey replaces every occurrence of a non-empty key in s. +func redactKey(s, key string) string { + if key == "" { + return s } - return &transportFailure{detail: detail, kind: kind} + return strings.ReplaceAll(s, key, "[REDACTED]") } diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index 30d2dfd..dc55844 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -627,6 +627,31 @@ func TestNewRejectsBadHeaderWithoutEchoingValue(t *testing.T) { assertNoSecret(t, 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) { + cases := map[string]Config{ + "key in invalid header name": {Headers: map[string]string{"Bearer " + testKey: "a"}}, + "key as invalid header name": {Headers: map[string]string{testKey + ":": "a"}}, + "key in name, bad value": {Headers: map[string]string{"X-" + testKey: "a\nb"}}, + "key in structured output": {StructuredOutput: StructuredOutput("Bearer " + testKey)}, + "key with quote in mode": {StructuredOutput: StructuredOutput(testKey + "\"\x00")}, + } + for name, cfg := range cases { + cfg.BaseURL = "https://example.com/v1" + cfg.APIKey = testKey + if cfg.StructuredOutput == "" { + cfg.StructuredOutput = JSONSchema + } + _, err := New(cfg) + if err == nil { + t.Errorf("%s: New() = nil error", name) + continue + } + assertNoSecret(t, err) + } +} + func TestAcceptHeader(t *testing.T) { for name, tc := range map[string]struct { headers map[string]string @@ -664,6 +689,9 @@ func TestNewRejectsBadConfig(t *testing.T) { "key, relative base": {BaseURL: "example.com/v1?key=" + testKey, StructuredOutput: JSONSchema}, "key, ftp base": {BaseURL: "ftp://example.com/v1?key=" + testKey, StructuredOutput: JSONSchema}, "key, unparseable": {BaseURL: "https://example.com/%zz?key=" + testKey, StructuredOutput: JSONSchema}, + "empty query": {BaseURL: "https://example.com/v1?", StructuredOutput: JSONSchema}, + "empty fragment": {BaseURL: "https://example.com/v1#", StructuredOutput: JSONSchema}, + "empty both": {BaseURL: "https://example.com/v1?#", StructuredOutput: JSONSchema}, "unknown mode": {BaseURL: "https://example.com/v1", StructuredOutput: "json"}, "no mode": {BaseURL: "https://example.com/v1"}, "authorization hdr": {BaseURL: "https://example.com/v1", StructuredOutput: JSONSchema, Headers: map[string]string{"authorization": "Bearer x"}}, -- 2.55.0 From 095c03f112344768c4988c7250e02ca9399c20f9 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sat, 3 Oct 2026 16:46:52 -0400 Subject: [PATCH 06/12] Stop echoing header names in header value errors Canonicalising a header name changes its case, so a key embedded in the name survived exact-match redaction. A bad header value now gets a fixed message; only managed header names, which are fixed known strings, are named. assertNoSecret now matches case-insensitively. Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/openai/openai.go | 12 +++++++----- internal/ai/llm/openai/openai_test.go | 4 ++-- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index f897689..cdd4265 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -90,8 +90,8 @@ func New(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") } - // Every other error that names caller-supplied text has the API key - // redacted from that text before it is quoted. + // An error that names caller-supplied text has the API key redacted from + // it before it is quoted. switch cfg.StructuredOutput { case JSONSchema, JSONObject, PromptOnly: default: @@ -99,8 +99,10 @@ func New(cfg Config) (*Provider, error) { } headers := make(http.Header, len(cfg.Headers)+1) for name, value := range cfg.Headers { - // Invalid names and all values are never echoed: either may carry a - // credential. + // Caller-supplied names and values are never echoed: either may carry + // a credential, and canonicalising a name changes its case, which + // defeats redaction. Only a managed name, which is a fixed known + // string, is named. if !validHeaderName(name) { return nil, errors.New("openai: an extra header name is not a valid HTTP token") } @@ -109,7 +111,7 @@ func New(cfg Config) (*Provider, error) { return nil, fmt.Errorf("openai: extra header %q is set by the adapter", canonical) } if !validHeaderValue(value) { - return nil, fmt.Errorf("openai: extra header %q has a control character in its value", redactKey(canonical, cfg.APIKey)) + return nil, errors.New("openai: an extra header value has a control character") } headers.Set(canonical, value) } diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index dc55844..6feff02 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -373,7 +373,7 @@ func TestVerify(t *testing.T) { func assertNoSecret(t *testing.T, err error) { t.Helper() for _, secret := range []string{testKey, "Bearer " + testKey, testKey[:12]} { - if strings.Contains(err.Error(), secret) { + if strings.Contains(strings.ToLower(err.Error()), strings.ToLower(secret)) { t.Fatalf("error leaks %q: %v", secret, err) } } @@ -633,7 +633,7 @@ func TestNewConfigErrorsNeverLeakKey(t *testing.T) { cases := map[string]Config{ "key in invalid header name": {Headers: map[string]string{"Bearer " + testKey: "a"}}, "key as invalid header name": {Headers: map[string]string{testKey + ":": "a"}}, - "key in name, bad value": {Headers: map[string]string{"X-" + testKey: "a\nb"}}, + "key in name, bad value": {Headers: map[string]string{"x-" + testKey: "a\nb"}}, "key in structured output": {StructuredOutput: StructuredOutput("Bearer " + testKey)}, "key with quote in mode": {StructuredOutput: StructuredOutput(testKey + "\"\x00")}, } -- 2.55.0 From f33344025614542b780ed0f97ea8c2f705787710 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sat, 3 Oct 2026 18:07:31 -0400 Subject: [PATCH 07/12] Never quote caller-supplied text in New errors A managed-header error named the canonical header, which equals the API key when the key is itself a managed header name such as Authorization. The unknown structured-output error quoted the mode after redacting the whole key, which still leaked any fragment of the key. Both now return fixed messages, so no New error interpolates caller input. Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/openai/openai.go | 13 +++++------ internal/ai/llm/openai/openai_test.go | 32 +++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 8 deletions(-) diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index cdd4265..44828d7 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -90,25 +90,22 @@ func New(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") } - // An error that names caller-supplied text has the API key redacted from - // it before it is quoted. + // 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. switch cfg.StructuredOutput { case JSONSchema, JSONObject, PromptOnly: default: - return nil, fmt.Errorf("openai: unknown structured output mode %q", redactKey(string(cfg.StructuredOutput), cfg.APIKey)) + return nil, errors.New("openai: unknown structured output mode") } headers := make(http.Header, len(cfg.Headers)+1) for name, value := range cfg.Headers { - // Caller-supplied names and values are never echoed: either may carry - // a credential, and canonicalising a name changes its case, which - // defeats redaction. Only a managed name, which is a fixed known - // string, is named. if !validHeaderName(name) { return nil, errors.New("openai: an extra header name is not a valid HTTP token") } canonical := textproto.CanonicalMIMEHeaderKey(name) if managedHeaders[canonical] { - return nil, fmt.Errorf("openai: extra header %q is set by the adapter", canonical) + return nil, errors.New("openai: an extra header is set by the adapter") } if !validHeaderValue(value) { return nil, errors.New("openai: an extra header value has a control character") diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index 6feff02..745da45 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -652,6 +652,38 @@ func TestNewConfigErrorsNeverLeakKey(t *testing.T) { } } +// TestNewManagedHeaderErrorNeverLeaksKey uses an API key that is itself a +// managed header name, so naming the rejected header would print the key. +func TestNewManagedHeaderErrorNeverLeaksKey(t *testing.T) { + for _, key := range []string{"Authorization", "Content-Type", "Host"} { + _, err := New(Config{ + BaseURL: "https://example.com/v1", + APIKey: key, + StructuredOutput: JSONSchema, + Headers: map[string]string{key: "x"}, + }) + if err == nil { + t.Errorf("key %q: New() = nil error", key) + continue + } + for _, secret := range []string{key, "Bearer " + key} { + if strings.Contains(strings.ToLower(err.Error()), strings.ToLower(secret)) { + t.Errorf("key %q: error leaks %q: %v", key, secret, err) + } + } + } +} + +// TestNewModeErrorNeverLeaksKeyFragment uses a structured output mode that is +// a fragment of the API key, which redacting the whole key would not catch. +func TestNewModeErrorNeverLeaksKeyFragment(t *testing.T) { + _, err := New(Config{BaseURL: "https://example.com/v1", APIKey: testKey, StructuredOutput: StructuredOutput(testKey[:20])}) + if err == nil { + t.Fatal("New() = nil error") + } + assertNoSecret(t, err) +} + func TestAcceptHeader(t *testing.T) { for name, tc := range map[string]struct { headers map[string]string -- 2.55.0 From 2afd6a9338860285e03c0330d0bb376093e6b233 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 4 Oct 2026 07:24:37 -0400 Subject: [PATCH 08/12] Never quote request or reply text in errors and redact the key as a backstop Request.Validate no longer quotes the role or schema name, and the bad-output error no longer quotes finish_reason: any of that text may equal the API key. New, Generate, ListModels and Verify now pass every error through redact, which removes the key (and so "Bearer "+key) from the error text, wrapped errors included, while Unwrap keeps the chain. Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/llm.go | 7 +- internal/ai/llm/openai/openai.go | 64 +++++++++++++++-- internal/ai/llm/openai/openai_test.go | 99 +++++++++++++++++++++++++++ 3 files changed, 162 insertions(+), 8 deletions(-) diff --git a/internal/ai/llm/llm.go b/internal/ai/llm/llm.go index ce2aaad..23e7b9a 100644 --- a/internal/ai/llm/llm.go +++ b/internal/ai/llm/llm.go @@ -67,14 +67,15 @@ type Request struct { } // Validate reports whether r is well formed. Adapters call it before -// contacting the vendor. +// contacting the vendor. Its errors never quote the request's text, such as +// a role or schema name: any of it may equal the API key. func (r Request) Validate() error { if len(r.Messages) == 0 { return errors.New("llm: request has no messages") } for i, m := range r.Messages { if m.Role != RoleUser && m.Role != RoleAssistant { - return fmt.Errorf("llm: message %d has unknown role %q", i, m.Role) + return fmt.Errorf("llm: message %d has an unknown role", i) } } if r.Temperature != nil && *r.Temperature < 0 { @@ -92,7 +93,7 @@ func (r Request) Validate() error { } var obj map[string]json.RawMessage if err := json.Unmarshal(r.Schema.Schema, &obj); err != nil || obj == nil { - return fmt.Errorf("llm: schema %q is not a JSON object", r.Schema.Name) + return errors.New("llm: schema is not a JSON object") } } return nil diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index 44828d7..cc4c13f 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -3,7 +3,9 @@ // POST {base}/chat/completions and GET {base}/models over plain net/http. // // The adapter never logs, and no error it returns contains the API key or -// the Authorization header value. +// the Authorization header value: errors do not quote caller- or +// vendor-supplied text, and every exported method passes its error through +// redact as a backstop. package openai import ( @@ -76,6 +78,14 @@ var _ llm.Provider = (*Provider)(nil) // New validates cfg and returns a Provider. func New(cfg Config) (*Provider, error) { + p, err := newFromConfig(cfg) + if err != nil { + return nil, redact(err, cfg.APIKey) + } + return p, nil +} + +func newFromConfig(cfg Config) (*Provider, error) { // Base URL errors never echo the URL: a rejected one may carry a // credential. u, err := url.Parse(cfg.BaseURL) @@ -219,6 +229,11 @@ const schemaInstruction = "Reply with only a JSON object, and no other text, tha // Generate sends req to {base}/chat/completions. func (p *Provider) Generate(ctx context.Context, req llm.Request) (llm.Response, error) { + resp, err := p.generate(ctx, req) + return resp, redact(err, p.apiKey) +} + +func (p *Provider) generate(ctx context.Context, req llm.Request) (llm.Response, error) { if err := req.Validate(); err != nil { return llm.Response{}, fmt.Errorf("openai: generate: %w", err) } @@ -298,7 +313,7 @@ func (p *Provider) Generate(ctx context.Context, req llm.Request) (llm.Response, resp.JSON = json.RawMessage(text) } if resp.JSON == nil { - return resp, fmt.Errorf("openai: generate: reply (finish reason %q) holds no JSON object: %w", resp.FinishReason, llm.ErrBadOutput) + return resp, fmt.Errorf("openai: generate: reply holds no JSON object: %w", llm.ErrBadOutput) } return resp, nil } @@ -343,6 +358,11 @@ type modelsResponse struct { // ListModels returns the models {base}/models lists. func (p *Provider) ListModels(ctx context.Context) ([]llm.Model, error) { + models, err := p.listModels(ctx) + return models, redact(err, p.apiKey) +} + +func (p *Provider) listModels(ctx context.Context) ([]llm.Model, error) { raw, _, err := p.do(ctx, http.MethodGet, "/models", nil) if err != nil { return nil, fmt.Errorf("openai: list models: %w", err) @@ -370,8 +390,8 @@ func (p *Provider) ListModels(ctx context.Context) ([]llm.Model, error) { // Verify checks the credentials with an authenticated ListModels call. func (p *Provider) Verify(ctx context.Context) error { - _, err := p.ListModels(ctx) - return err + _, err := p.listModels(ctx) + return redact(err, p.apiKey) } // do sends one request and returns the body and status of a 2xx response. @@ -493,10 +513,44 @@ func (p *Provider) transportError(err error) error { return &transportFailure{detail: redactKey(err.Error(), p.apiKey), kind: kind} } -// redactKey replaces every occurrence of a non-empty key in s. +// redactKey replaces every occurrence of a non-empty key in s. Replacing +// the key also replaces the "Bearer "+key header value that contains it. func redactKey(s, key string) string { if key == "" { return s } return strings.ReplaceAll(s, key, "[REDACTED]") } + +// withheldText replaces an error's text when redactKey cannot remove the +// key, because the key is part of the replacement marker. +const withheldText = "openai: error text withheld: it contains the API key" + +// redactedError is an error whose text had the API key removed. It unwraps +// to the original error, so errors.Is and errors.As still see the chain. +type redactedError struct { + text string + err error +} + +func (e *redactedError) Error() string { return e.text } + +func (e *redactedError) Unwrap() error { return e.err } + +// redact is the backstop every exported method applies to its error: when +// err's text, wrapped errors included, holds a non-empty key, it returns err +// with that text redacted. Any other err is returned unchanged. +func redact(err error, key string) error { + if err == nil || key == "" { + return err + } + text := err.Error() + if !strings.Contains(text, key) { + return err + } + text = redactKey(text, key) + if strings.Contains(text, key) { + text = withheldText + } + return &redactedError{text: text, err: err} +} diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index 745da45..cc45f32 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "io" "net/http" "net/http/httptest" @@ -785,3 +786,101 @@ func TestRetryAfter(t *testing.T) { } } } + +// headerNameKey is an API key that is also a word callers and vendors put in +// text: an error that quotes such text prints the key. +const headerNameKey = "Authorization" + +// assertNoKey fails if err's text holds key or "Bearer "+key in any case. +func assertNoKey(t *testing.T, err error, key string) { + t.Helper() + for _, secret := range []string{key, "Bearer " + key} { + if strings.Contains(err.Error(), secret) || strings.Contains(strings.ToLower(err.Error()), strings.ToLower(secret)) { + t.Fatalf("error leaks %q: %v", secret, err) + } + } +} + +// keyServer replies to every request with status and body. +func keyServer(t *testing.T, status int, body string) *Provider { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + io.WriteString(w, body) + })) + t.Cleanup(srv.Close) + p, err := New(Config{BaseURL: srv.URL + "/v1", APIKey: headerNameKey, Model: "test-model", StructuredOutput: PromptOnly}) + if err != nil { + t.Fatal(err) + } + return p +} + +func TestGenerateInvalidRoleNeverLeaksKey(t *testing.T) { + p := keyServer(t, 200, `{}`) + _, err := p.Generate(context.Background(), llm.Request{Messages: []llm.Message{{Role: llm.Role(headerNameKey), Content: "hi"}}}) + if err == nil { + t.Fatal("Generate accepted an unknown role") + } + assertNoKey(t, err, headerNameKey) +} + +func TestGenerateInvalidSchemaNeverLeaksKey(t *testing.T) { + p := keyServer(t, 200, `{}`) + req := schemaRequest() + req.Schema = &llm.JSONSchema{Name: headerNameKey, Schema: json.RawMessage(`[]`)} + _, err := p.Generate(context.Background(), req) + if err == nil { + t.Fatal("Generate accepted a schema that is not a JSON object") + } + assertNoKey(t, err, headerNameKey) +} + +func TestGenerateBadOutputNeverLeaksKey(t *testing.T) { + p := keyServer(t, 200, `{"model":"m","choices":[{"message":{"content":"no JSON here"},"finish_reason":"`+headerNameKey+`"}]}`) + resp, err := p.Generate(context.Background(), schemaRequest()) + if !errors.Is(err, llm.ErrBadOutput) { + t.Fatalf("err = %v, want ErrBadOutput", err) + } + assertNoKey(t, err, headerNameKey) + if resp.FinishReason != headerNameKey { + t.Fatalf("FinishReason = %q, want it kept on the response", resp.FinishReason) + } +} + +func TestProviderErrorBodyNeverLeaksKey(t *testing.T) { + p := keyServer(t, 500, `{"error":{"message":"rejected `+headerNameKey+`: Bearer `+headerNameKey+`"}}`) + _, genErr := p.Generate(context.Background(), schemaRequest()) + _, listErr := p.ListModels(context.Background()) + verifyErr := p.Verify(context.Background()) + for _, err := range []error{genErr, listErr, verifyErr} { + var pe *llm.ProviderError + if !errors.Is(err, llm.ErrProvider) || !errors.As(err, &pe) || pe.Status() != 500 { + t.Fatalf("err = %v, want a 500 ProviderError", err) + } + assertNoKey(t, err, headerNameKey) + } +} + +// TestRedactKeepsChainAndHidesKey checks the backstop every exported method +// applies: the key and its Bearer value leave the text, the chain stays. +func TestRedactKeepsChainAndHidesKey(t *testing.T) { + inner := fmt.Errorf("decode: body %q: %w", "Bearer "+testKey, llm.ErrBadOutput) + err := redact(inner, testKey) + assertNoSecret(t, err) + if !errors.Is(err, llm.ErrBadOutput) || errors.Unwrap(err) != inner { + t.Fatalf("redact broke the chain of %v", err) + } + if redact(inner, "") != inner { + t.Fatal("redact changed an error for the empty key") + } + clean := errors.New("openai: generate: no model configured") + if redact(clean, testKey) != clean { + t.Fatal("redact wrapped an error that holds no key") + } + // A key inside the redaction marker would survive replacement. + if got := redact(errors.New("x E y"), "E").Error(); strings.Contains(got, "E") { + t.Fatalf("redact kept the key: %q", got) + } +} -- 2.55.0 From cceeafd20076cdd45e294385e3f9a351c148506d Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 4 Oct 2026 07:26:37 -0400 Subject: [PATCH 09/12] Redact every layer of a returned error chain, not just the top redactedError.Unwrap returned the original error, so errors.Unwrap could reach an inner error whose text still held the API key. Unwrap now passes each cause through redact, so every error reached by unwrapping has the key removed, and errors with several causes are handled the same way. redact also checks the whole chain, catching a layer that hides the key from its own text but wraps an error that shows it. Is and As consult the original chain, so sentinels and *llm.ProviderError still match. Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/openai/openai.go | 70 +++++++++++++++++---- internal/ai/llm/openai/openai_test.go | 90 ++++++++++++++++++++++++++- 2 files changed, 146 insertions(+), 14 deletions(-) diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index cc4c13f..785d1ad 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -526,31 +526,77 @@ func redactKey(s, key string) string { // key, because the key is part of the replacement marker. const withheldText = "openai: error text withheld: it contains the API key" -// redactedError is an error whose text had the API key removed. It unwraps -// to the original error, so errors.Is and errors.As still see the chain. +// redactedError stands in for one layer of an error chain that holds the +// API key. Its text is the layer's text with the key redacted, and Unwrap +// returns the layer's causes passed through redact, so no error reached by +// unwrapping has text holding the key. Is and As consult the original +// layer and its chain, so sentinels and typed errors still match. type redactedError struct { text string err error + key string } func (e *redactedError) Error() string { return e.text } -func (e *redactedError) Unwrap() error { return e.err } +func (e *redactedError) Unwrap() error { return redact(errors.Unwrap(e.err), e.key) } + +func (e *redactedError) Is(target error) bool { return errors.Is(e.err, target) } + +func (e *redactedError) As(target any) bool { return errors.As(e.err, target) } + +// redactedJoin is redactedError for a layer with several causes, such as +// one built by errors.Join or by fmt.Errorf with more than one %w. +type redactedJoin struct { + redactedError +} + +func (e *redactedJoin) Unwrap() []error { + causes := e.err.(interface{ Unwrap() []error }).Unwrap() + safe := make([]error, len(causes)) + for i, c := range causes { + safe[i] = redact(c, e.key) + } + return safe +} // redact is the backstop every exported method applies to its error: when -// err's text, wrapped errors included, holds a non-empty key, it returns err -// with that text redacted. Any other err is returned unchanged. +// the text of err or of any error in its chain holds a non-empty key, it +// returns a stand-in whose text, and the text of every error it unwraps +// to, has the key redacted. Any other err is returned unchanged. func redact(err error, key string) error { - if err == nil || key == "" { + if err == nil || key == "" || !holdsKey(err, key) { return err } - text := err.Error() - if !strings.Contains(text, key) { - return err - } - text = redactKey(text, key) + text := redactKey(err.Error(), key) if strings.Contains(text, key) { text = withheldText } - return &redactedError{text: text, err: err} + re := redactedError{text: text, err: err, key: key} + if _, ok := err.(interface{ Unwrap() []error }); ok { + return &redactedJoin{re} + } + return &re +} + +// holdsKey reports whether the text of err or of any error in its chain +// contains key. +func holdsKey(err error, key string) bool { + if err == nil { + return false + } + if strings.Contains(err.Error(), key) { + return true + } + switch u := err.(type) { + case interface{ Unwrap() error }: + return holdsKey(u.Unwrap(), key) + case interface{ Unwrap() []error }: + for _, c := range u.Unwrap() { + if holdsKey(c, key) { + return true + } + } + } + return false } diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index cc45f32..dea2b44 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -868,8 +868,8 @@ func TestProviderErrorBodyNeverLeaksKey(t *testing.T) { func TestRedactKeepsChainAndHidesKey(t *testing.T) { inner := fmt.Errorf("decode: body %q: %w", "Bearer "+testKey, llm.ErrBadOutput) err := redact(inner, testKey) - assertNoSecret(t, err) - if !errors.Is(err, llm.ErrBadOutput) || errors.Unwrap(err) != inner { + assertChainNoKey(t, err, testKey) + if !errors.Is(err, llm.ErrBadOutput) { t.Fatalf("redact broke the chain of %v", err) } if redact(inner, "") != inner { @@ -884,3 +884,89 @@ func TestRedactKeepsChainAndHidesKey(t *testing.T) { t.Fatalf("redact kept the key: %q", got) } } + +// assertChainNoKey fails if err, or any error reached from it by unwrapping, +// has text holding key or "Bearer "+key. +func assertChainNoKey(t *testing.T, err error, key string) { + t.Helper() + if err == nil { + return + } + assertNoKey(t, err, key) + switch u := err.(type) { + case interface{ Unwrap() error }: + assertChainNoKey(t, u.Unwrap(), key) + case interface{ Unwrap() []error }: + for _, c := range u.Unwrap() { + assertChainNoKey(t, c, key) + } + } +} + +// TestGenerateInvalidJSONWithKeyNeverLeaksKeyByUnwrapping sends an invalid +// JSON body holding the key and walks the returned chain with errors.Unwrap. +func TestGenerateInvalidJSONWithKeyNeverLeaksKeyByUnwrapping(t *testing.T) { + p := keyServer(t, 200, `{"choices": Bearer `+headerNameKey+` `+headerNameKey+`}`) + _, err := p.Generate(context.Background(), schemaRequest()) + if err == nil { + t.Fatal("Generate accepted an invalid JSON body") + } + var syntax *json.SyntaxError + if !errors.As(err, &syntax) { + t.Fatalf("err = %v, want a *json.SyntaxError in the chain", err) + } + for layer := err; layer != nil; layer = errors.Unwrap(layer) { + assertNoKey(t, layer, headerNameKey) + } +} + +// TestRedactMakesEveryUnwrappedLayerSafe covers chains whose inner layers +// hold the key, including a layer whose own text does not show it and a +// layer with several causes, and checks Is and As still match. +func TestRedactMakesEveryUnwrappedLayerSafe(t *testing.T) { + pe := llm.NewProviderError(500, []byte(`{"error":"down"}`), testKey) + hidden := &opaqueError{err: fmt.Errorf("body Bearer %s: %w", testKey, llm.ErrAuth)} + inner := fmt.Errorf("decode %s: %w", testKey, llm.ErrBadOutput) + joined := errors.Join(inner, pe, context.Canceled, fmt.Errorf("limit: %w", llm.ErrRateLimited)) + cases := []error{ + fmt.Errorf("openai: generate: %w", inner), + fmt.Errorf("openai: generate: %w", hidden), + fmt.Errorf("openai: generate: %w", joined), + fmt.Errorf("openai: %w and %w", inner, &transportFailure{detail: "slow", kind: llm.ErrTimeout}), + } + for _, c := range cases { + err := redact(c, testKey) + if err == c { + t.Fatalf("redact left %v unchanged", c) + } + assertChainNoKey(t, err, testKey) + for layer := err; layer != nil; layer = errors.Unwrap(layer) { + assertNoKey(t, layer, testKey) + } + } + err := redact(fmt.Errorf("openai: %w", joined), testKey) + var got *llm.ProviderError + if !errors.As(err, &got) || got != pe { + t.Fatalf("errors.As lost the ProviderError in %v", err) + } + for _, target := range []error{llm.ErrBadOutput, llm.ErrProvider, llm.ErrRateLimited, context.Canceled} { + if !errors.Is(err, target) { + t.Fatalf("errors.Is(%v, %v) = false", err, target) + } + } + err = redact(fmt.Errorf("openai: %w", hidden), testKey) + if !errors.Is(err, llm.ErrAuth) { + t.Fatalf("errors.Is lost ErrAuth in %v", err) + } + err = redact(fmt.Errorf("openai %s: %w", testKey, &transportFailure{detail: "slow", kind: llm.ErrTimeout}), testKey) + if !errors.Is(err, llm.ErrTimeout) { + t.Fatalf("errors.Is lost ErrTimeout in %v", err) + } +} + +// opaqueError hides its cause's text but unwraps to it. +type opaqueError struct{ err error } + +func (e *opaqueError) Error() string { return "opaque" } + +func (e *opaqueError) Unwrap() error { return e.err } -- 2.55.0 From 68391d14c8f2c9ce8601621a485cde7928c6ae07 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 4 Oct 2026 07:32:34 -0400 Subject: [PATCH 10/12] Never insert a redaction marker that itself holds the API key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit llm.Redact is now the single redaction rule for ProviderError bodies and the openai backstop. Each occurrence of the key becomes "[REDACTED]" only when the result then holds no key; otherwise occurrences are deleted until none remains, which also covers a marker that joins with its neighbours to form the key. NewProviderError redacts again after truncation so the "…" mark cannot complete the key. The openai adapter drops its own redactKey and the withheld-text fallback, which itself could hold a short key. redactedError.As now refuses a match whose text holds the key, so errors.As cannot reach an unredacted error. Tests cover the key "REDACTED" in a 500 body (ProviderError.Body and Error after errors.As) and the key "A" in a malformed JSON body (every unwrapped layer and every errors.As result). Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/errors.go | 36 +++++++++---- internal/ai/llm/errors_test.go | 42 +++++++++++++++ internal/ai/llm/openai/openai.go | 39 ++++++-------- internal/ai/llm/openai/openai_test.go | 74 ++++++++++++++++++++++++++- 4 files changed, 158 insertions(+), 33 deletions(-) diff --git a/internal/ai/llm/errors.go b/internal/ai/llm/errors.go index 555b99a..f826dfc 100644 --- a/internal/ai/llm/errors.go +++ b/internal/ai/llm/errors.go @@ -46,8 +46,27 @@ func (e *RateLimitedError) Is(target error) bool { return target == ErrRateLimit // ProviderError keeps. const MaxBodyExcerpt = 512 -// redacted replaces each secret in a ProviderError body. -const redacted = "[REDACTED]" +// redactionMarker replaces each secret in redacted text, unless the marker +// would itself hold the secret. +const redactionMarker = "[REDACTED]" + +// Redact returns s with every occurrence of a non-empty secret removed. Each +// occurrence becomes "[REDACTED]" when the result then holds no secret; +// otherwise, because the marker contains the secret or joins with its +// neighbours to form it, occurrences are deleted until none remains. The +// result never contains secret. +func Redact(s, secret string) string { + if secret == "" { + return s + } + if out := strings.ReplaceAll(s, secret, redactionMarker); !strings.Contains(out, secret) { + return out + } + for strings.Contains(s, secret) { + s = strings.ReplaceAll(s, secret, "") + } + return s +} // ProviderError is an unexpected HTTP status from the vendor. It matches // ErrProvider. Build it with NewProviderError, which redacts the body. @@ -59,20 +78,17 @@ type ProviderError struct { // NewProviderError returns the error for an unexpected HTTP status. It // always keeps a body excerpt of at most MaxBodyExcerpt bytes. secret is the // credential sent with the request, or "" when the request sent none; every -// occurrence of a non-empty secret in body is replaced before the body is -// cut, so no part of it survives. +// occurrence of a non-empty secret in body is removed by Redact before the +// body is cut, so no part of it survives, and again after the cut, so the +// truncation mark cannot complete it. func NewProviderError(status int, body []byte, secret string) *ProviderError { - s := string(body) - if secret != "" { - s = strings.ReplaceAll(s, secret, redacted) - } - s = strings.TrimSpace(s) + s := strings.TrimSpace(Redact(string(body), secret)) if len(s) > MaxBodyExcerpt { cut := MaxBodyExcerpt for cut > 0 && !utf8.RuneStart(s[cut]) { cut-- } - s = s[:cut] + "…" + s = Redact(s[:cut]+"…", secret) } return &ProviderError{status: status, body: s} } diff --git a/internal/ai/llm/errors_test.go b/internal/ai/llm/errors_test.go index 8911611..b08d632 100644 --- a/internal/ai/llm/errors_test.go +++ b/internal/ai/llm/errors_test.go @@ -117,3 +117,45 @@ func TestProviderErrorKeepsBoundedBodyWithoutSecret(t *testing.T) { t.Fatalf("Body() has %d bytes, want %d", len(long.Body()), len(want)) } } + +func TestRedactNeverLeavesSecret(t *testing.T) { + cases := []struct{ s, secret, want string }{ + {"key sk-1 and Bearer sk-1", "sk-1", "key [REDACTED] and Bearer [REDACTED]"}, + {"no secret here", "sk-1", "no secret here"}, + {"x", "", "x"}, + // The marker holds the secret, so occurrences are deleted. + {"rejected REDACTED", "REDACTED", "rejected "}, + {"A then A", "A", " then "}, + // The marker joins with a neighbour to form the secret. + {"]]aa", "]a", ""}, + // The marker holds the secret and deleting once leaves a new one. + {"DDAA", "DA", ""}, + {"aabb", "ab", "a[REDACTED]b"}, + } + for _, c := range cases { + if got := Redact(c.s, c.secret); got != c.want { + t.Errorf("Redact(%q, %q) = %q, want %q", c.s, c.secret, got, c.want) + } + } +} + +func TestProviderErrorWithMarkerSecretNeverLeaksSecret(t *testing.T) { + const key = "REDACTED" + pe := NewProviderError(500, []byte(`{"error":"bad key REDACTED","echo":"Bearer REDACTED"}`), key) + for _, s := range []string{pe.Error(), pe.Body()} { + if strings.Contains(s, key) { + t.Fatalf("secret leaked: %q", s) + } + } + if want := `{"error":"bad key ","echo":"Bearer "}`; pe.Body() != want { + t.Fatalf("Body() = %q, want %q", pe.Body(), want) + } +} + +func TestProviderErrorTruncationMarkCannotCompleteSecret(t *testing.T) { + const key = "z…" + body := strings.Repeat("a", MaxBodyExcerpt-1) + "z" + strings.Repeat("b", 10) + if pe := NewProviderError(500, []byte(body), key); strings.Contains(pe.Body(), key) { + t.Fatalf("secret leaked: %q", pe.Body()) + } +} diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index 785d1ad..52fd89d 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -19,6 +19,7 @@ import ( "net/http" "net/textproto" "net/url" + "reflect" "strconv" "strings" "time" @@ -510,27 +511,15 @@ func (p *Provider) transportError(err error) error { if errors.As(err, &ue) { err = ue.Err } - return &transportFailure{detail: redactKey(err.Error(), p.apiKey), kind: kind} + return &transportFailure{detail: llm.Redact(err.Error(), p.apiKey), kind: kind} } -// redactKey replaces every occurrence of a non-empty key in s. Replacing -// the key also replaces the "Bearer "+key header value that contains it. -func redactKey(s, key string) string { - if key == "" { - return s - } - return strings.ReplaceAll(s, key, "[REDACTED]") -} - -// withheldText replaces an error's text when redactKey cannot remove the -// key, because the key is part of the replacement marker. -const withheldText = "openai: error text withheld: it contains the API key" - // redactedError stands in for one layer of an error chain that holds the // API key. Its text is the layer's text with the key redacted, and Unwrap // returns the layer's causes passed through redact, so no error reached by -// unwrapping has text holding the key. Is and As consult the original -// layer and its chain, so sentinels and typed errors still match. +// unwrapping has text holding the key. Is consults the original layer and +// its chain, so sentinels still match. As does too, but refuses a match +// whose text holds the key, so it never hands back an unredacted error. type redactedError struct { text string err error @@ -543,7 +532,17 @@ func (e *redactedError) Unwrap() error { return redact(errors.Unwrap(e.err), e.k func (e *redactedError) Is(target error) bool { return errors.Is(e.err, target) } -func (e *redactedError) As(target any) bool { return errors.As(e.err, target) } +func (e *redactedError) As(target any) bool { + if !errors.As(e.err, target) { + return false + } + found := reflect.ValueOf(target).Elem() + if matched, ok := found.Interface().(error); ok && holdsKey(matched, e.key) { + found.SetZero() + return false + } + return true +} // redactedJoin is redactedError for a layer with several causes, such as // one built by errors.Join or by fmt.Errorf with more than one %w. @@ -568,11 +567,7 @@ func redact(err error, key string) error { if err == nil || key == "" || !holdsKey(err, key) { return err } - text := redactKey(err.Error(), key) - if strings.Contains(text, key) { - text = withheldText - } - re := redactedError{text: text, err: err, key: key} + re := redactedError{text: llm.Redact(err.Error(), key), err: err, key: key} if _, ok := err.(interface{ Unwrap() []error }); ok { return &redactedJoin{re} } diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index dea2b44..ca734ad 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -803,6 +803,12 @@ func assertNoKey(t *testing.T, err error, key string) { // keyServer replies to every request with status and body. func keyServer(t *testing.T, status int, body string) *Provider { + t.Helper() + return keyServerWithKey(t, headerNameKey, status, body) +} + +// keyServerWithKey is keyServer for a provider configured with key. +func keyServerWithKey(t *testing.T, key string, status int, body string) *Provider { t.Helper() srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") @@ -810,7 +816,7 @@ func keyServer(t *testing.T, status int, body string) *Provider { io.WriteString(w, body) })) t.Cleanup(srv.Close) - p, err := New(Config{BaseURL: srv.URL + "/v1", APIKey: headerNameKey, Model: "test-model", StructuredOutput: PromptOnly}) + p, err := New(Config{BaseURL: srv.URL + "/v1", APIKey: key, Model: "test-model", StructuredOutput: PromptOnly}) if err != nil { t.Fatal(err) } @@ -970,3 +976,69 @@ type opaqueError struct{ err error } func (e *opaqueError) Error() string { return "opaque" } func (e *opaqueError) Unwrap() error { return e.err } + +// assertChainNoExactKey fails if the text of err, of any error reached from +// it by unwrapping, or of a *llm.ProviderError found by errors.As holds key. +// It matches case-sensitively, so it suits keys too short for assertNoKey. +func assertChainNoExactKey(t *testing.T, err error, key string) { + t.Helper() + var walk func(error) + walk = func(e error) { + if e == nil { + return + } + if strings.Contains(e.Error(), key) { + t.Fatalf("error leaks %q: %q", key, e.Error()) + } + switch u := e.(type) { + case interface{ Unwrap() error }: + walk(u.Unwrap()) + case interface{ Unwrap() []error }: + for _, c := range u.Unwrap() { + walk(c) + } + } + } + walk(err) + var pe *llm.ProviderError + if errors.As(err, &pe) && (strings.Contains(pe.Body(), key) || strings.Contains(pe.Error(), key)) { + t.Fatalf("ProviderError leaks %q: body %q", key, pe.Body()) + } + var syntax *json.SyntaxError + if errors.As(err, &syntax) && strings.Contains(syntax.Error(), key) { + t.Fatalf("errors.As reached a *json.SyntaxError leaking %q: %q", key, syntax.Error()) + } +} + +// TestProviderErrorBodyWithMarkerKeyNeverLeaksKey uses the redaction marker +// text as the API key, so replacing the key with the marker would keep it. +func TestProviderErrorBodyWithMarkerKeyNeverLeaksKey(t *testing.T) { + const key = "REDACTED" + p := keyServerWithKey(t, key, 500, `{"error":{"message":"rejected `+key+`: Bearer `+key+`"}}`) + _, err := p.Generate(context.Background(), schemaRequest()) + var pe *llm.ProviderError + if !errors.Is(err, llm.ErrProvider) || !errors.As(err, &pe) || pe.Status() != 500 { + t.Fatalf("err = %v, want a 500 ProviderError", err) + } + if strings.Contains(pe.Body(), key) || strings.Contains(pe.Error(), key) { + t.Fatalf("ProviderError leaks the key: %q", pe.Error()) + } + assertChainNoExactKey(t, err, key) +} + +// TestGenerateMalformedJSONWithShortKeyNeverLeaksKey uses a one-letter key +// found in the redaction marker and in ordinary error prose. +func TestGenerateMalformedJSONWithShortKeyNeverLeaksKey(t *testing.T) { + const key = "A" + p := keyServerWithKey(t, key, 200, `{"choices": A Bearer A}`) + _, err := p.Generate(context.Background(), schemaRequest()) + if err == nil { + t.Fatal("Generate accepted a malformed JSON body") + } + assertChainNoExactKey(t, err, key) + for layer := err; layer != nil; layer = errors.Unwrap(layer) { + if strings.Contains(layer.Error(), key) { + t.Fatalf("layer leaks %q: %q", key, layer.Error()) + } + } +} -- 2.55.0 From 893e00d14e059d5b9aea2bd7c0d8c5c80364704a Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 4 Oct 2026 07:36:56 -0400 Subject: [PATCH 11/12] Strip the API key from fixed error text and keep typed matches ProviderError and the new NewRateLimitedError constructor now keep the credential and remove it from their complete Error() text, including fixed words such as "HTTP" and "rate", so errors.As to them succeeds through the adapter's redaction stand-in instead of being refused. The openai adapter builds RateLimitedError with the key. Tests cover the keys "HTTP" (500), "rate" (429) and "openai" (401 and no model), checking Error(), Body(), every unwrap layer, errors.Is and errors.As. Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/errors.go | 25 ++++++--- internal/ai/llm/errors_test.go | 25 +++++++++ internal/ai/llm/openai/openai.go | 8 +-- internal/ai/llm/openai/openai_test.go | 77 +++++++++++++++++++++++++++ 4 files changed, 126 insertions(+), 9 deletions(-) diff --git a/internal/ai/llm/errors.go b/internal/ai/llm/errors.go index f826dfc..50183e6 100644 --- a/internal/ai/llm/errors.go +++ b/internal/ai/llm/errors.go @@ -26,17 +26,28 @@ var ( ) // RateLimitedError is a throttled request. It matches ErrRateLimited. +// Adapters build it with NewRateLimitedError so its text never holds their +// credential. type RateLimitedError struct { // RetryAfter is how long the vendor asked callers to wait; 0 means it // did not say. RetryAfter time.Duration + secret string +} + +// NewRateLimitedError returns the error for a throttled request. secret is +// the credential sent with the request, or "" when the request sent none; +// Error removes every occurrence of a non-empty secret, even where the +// fixed text happens to spell it. +func NewRateLimitedError(retryAfter time.Duration, secret string) *RateLimitedError { + return &RateLimitedError{RetryAfter: retryAfter, secret: secret} } func (e *RateLimitedError) Error() string { if e.RetryAfter == 0 { - return ErrRateLimited.Error() + return Redact(ErrRateLimited.Error(), e.secret) } - return fmt.Sprintf("%s: retry after %v", ErrRateLimited, e.RetryAfter) + return Redact(fmt.Sprintf("%s: retry after %v", ErrRateLimited, e.RetryAfter), e.secret) } // Is reports whether target is ErrRateLimited. @@ -73,6 +84,7 @@ func Redact(s, secret string) string { type ProviderError struct { status int body string + secret string } // NewProviderError returns the error for an unexpected HTTP status. It @@ -80,7 +92,8 @@ type ProviderError struct { // credential sent with the request, or "" when the request sent none; every // occurrence of a non-empty secret in body is removed by Redact before the // body is cut, so no part of it survives, and again after the cut, so the -// truncation mark cannot complete it. +// truncation mark cannot complete it. Error removes it from the complete +// text too, even where the fixed text happens to spell it. func NewProviderError(status int, body []byte, secret string) *ProviderError { s := strings.TrimSpace(Redact(string(body), secret)) if len(s) > MaxBodyExcerpt { @@ -90,7 +103,7 @@ func NewProviderError(status int, body []byte, secret string) *ProviderError { } s = Redact(s[:cut]+"…", secret) } - return &ProviderError{status: status, body: s} + return &ProviderError{status: status, body: s, secret: secret} } // Status returns the HTTP status code. @@ -102,9 +115,9 @@ func (e *ProviderError) Body() string { return e.body } func (e *ProviderError) Error() string { if e.body == "" { - return fmt.Sprintf("%s: HTTP %d", ErrProvider, e.status) + return Redact(fmt.Sprintf("%s: HTTP %d", ErrProvider, e.status), e.secret) } - return fmt.Sprintf("%s: HTTP %d: %s", ErrProvider, e.status, e.body) + return Redact(fmt.Sprintf("%s: HTTP %d: %s", ErrProvider, e.status, e.body), e.secret) } // Is reports whether target is ErrProvider. diff --git a/internal/ai/llm/errors_test.go b/internal/ai/llm/errors_test.go index b08d632..b5784ad 100644 --- a/internal/ai/llm/errors_test.go +++ b/internal/ai/llm/errors_test.go @@ -159,3 +159,28 @@ func TestProviderErrorTruncationMarkCannotCompleteSecret(t *testing.T) { t.Fatalf("secret leaked: %q", pe.Body()) } } + +func TestProviderErrorWithFixedWordSecretNeverLeaksSecret(t *testing.T) { + const secret = "HTTP" + pe := NewProviderError(500, []byte(`{"error":"HTTP down"}`), secret) + if strings.Contains(pe.Error(), secret) || strings.Contains(pe.Body(), secret) { + t.Fatalf("secret leaked: Error %q, Body %q", pe.Error(), pe.Body()) + } + if got := NewProviderError(500, nil, secret).Error(); got != "llm: provider error: [REDACTED] 500" { + t.Fatalf("Error() without body = %q", got) + } +} + +func TestRateLimitedErrorWithFixedWordSecretNeverLeaksSecret(t *testing.T) { + for _, secret := range []string{"rate", "retry after"} { + for _, wait := range []time.Duration{0, 30 * time.Second} { + rl := NewRateLimitedError(wait, secret) + if strings.Contains(rl.Error(), secret) { + t.Fatalf("secret %q leaked: %q", secret, rl.Error()) + } + if rl.RetryAfter != wait || !errors.Is(rl, ErrRateLimited) { + t.Fatalf("NewRateLimitedError(%v) = %+v", wait, rl) + } + } + } +} diff --git a/internal/ai/llm/openai/openai.go b/internal/ai/llm/openai/openai.go index 52fd89d..e6493de 100644 --- a/internal/ai/llm/openai/openai.go +++ b/internal/ai/llm/openai/openai.go @@ -446,7 +446,7 @@ func (p *Provider) statusError(resp *http.Response, body []byte) error { case http.StatusUnauthorized, http.StatusForbidden: return fmt.Errorf("HTTP %d: %w", resp.StatusCode, llm.ErrAuth) case http.StatusTooManyRequests: - return &llm.RateLimitedError{RetryAfter: retryAfter(resp.Header.Get("Retry-After"), time.Now())} + return llm.NewRateLimitedError(retryAfter(resp.Header.Get("Retry-After"), time.Now()), p.apiKey) case http.StatusRequestTimeout: return fmt.Errorf("HTTP %d: %w", resp.StatusCode, llm.ErrTimeout) default: @@ -518,8 +518,10 @@ func (p *Provider) transportError(err error) error { // API key. Its text is the layer's text with the key redacted, and Unwrap // returns the layer's causes passed through redact, so no error reached by // unwrapping has text holding the key. Is consults the original layer and -// its chain, so sentinels still match. As does too, but refuses a match -// whose text holds the key, so it never hands back an unredacted error. +// its chain, so sentinels still match. As does too. The llm typed errors +// remove the key from their own complete text, fixed words included, so +// they always match; As refuses only a match from outside llm whose text +// holds the key, so it never hands back an unredacted error. type redactedError struct { text string err error diff --git a/internal/ai/llm/openai/openai_test.go b/internal/ai/llm/openai/openai_test.go index ca734ad..8f0a248 100644 --- a/internal/ai/llm/openai/openai_test.go +++ b/internal/ai/llm/openai/openai_test.go @@ -1042,3 +1042,80 @@ func TestGenerateMalformedJSONWithShortKeyNeverLeaksKey(t *testing.T) { } } } + +// assertLayersNoExactKey fails if the text of err or of any error reached +// from it by errors.Unwrap holds key, matched case-sensitively. +func assertLayersNoExactKey(t *testing.T, err error, key string) { + t.Helper() + for layer := err; layer != nil; layer = errors.Unwrap(layer) { + if strings.Contains(layer.Error(), key) { + t.Fatalf("layer leaks %q: %q", key, layer.Error()) + } + } +} + +// TestProviderErrorWithFixedWordKeyKeepsTypedMatch uses the fixed word +// "HTTP" of ProviderError's text as the API key: the text must drop it and +// errors.As must still reach the ProviderError. +func TestProviderErrorWithFixedWordKeyKeepsTypedMatch(t *testing.T) { + const key = "HTTP" + p := keyServerWithKey(t, key, 500, `{"error":"down"}`) + _, err := p.Generate(context.Background(), schemaRequest()) + var pe *llm.ProviderError + if !errors.Is(err, llm.ErrProvider) || !errors.As(err, &pe) || pe.Status() != 500 { + t.Fatalf("err = %v, want a 500 ProviderError", err) + } + if strings.Contains(pe.Error(), key) || strings.Contains(pe.Body(), key) { + t.Fatalf("ProviderError leaks the key: Error %q, Body %q", pe.Error(), pe.Body()) + } + assertLayersNoExactKey(t, err, key) + assertChainNoExactKey(t, err, key) +} + +// TestRateLimitedErrorWithFixedWordKeyKeepsTypedMatch uses the fixed word +// "rate" of RateLimitedError's text as the API key. +func TestRateLimitedErrorWithFixedWordKeyKeepsTypedMatch(t *testing.T) { + const key = "rate" + srv := newVendorServer(t, reply{status: 429, header: map[string]string{"Retry-After": "17"}}) + p, err := New(Config{BaseURL: srv.URL + "/v1", APIKey: key, Model: "test-model", StructuredOutput: PromptOnly}) + if err != nil { + t.Fatal(err) + } + _, err = p.Generate(context.Background(), schemaRequest()) + var rl *llm.RateLimitedError + if !errors.Is(err, llm.ErrRateLimited) || !errors.As(err, &rl) || rl.RetryAfter != 17*time.Second { + t.Fatalf("err = %v, want a RateLimitedError with RetryAfter 17s", err) + } + if strings.Contains(rl.Error(), key) { + t.Fatalf("RateLimitedError leaks the key: %q", rl.Error()) + } + assertLayersNoExactKey(t, err, key) + assertChainNoExactKey(t, err, key) +} + +// TestGenerateWithAdapterNameKeyNeverLeaksKey uses the fixed prefix +// "openai" of the adapter's errors as the API key. +func TestGenerateWithAdapterNameKeyNeverLeaksKey(t *testing.T) { + const key = "openai" + t.Run("401", func(t *testing.T) { + p := keyServerWithKey(t, key, 401, `{"error":{"message":"bad key"}}`) + _, err := p.Generate(context.Background(), schemaRequest()) + if !errors.Is(err, llm.ErrAuth) { + t.Fatalf("err = %v, want ErrAuth", err) + } + assertLayersNoExactKey(t, err, key) + assertChainNoExactKey(t, err, key) + }) + t.Run("no model", func(t *testing.T) { + p, err := New(Config{BaseURL: "http://127.0.0.1:1/v1", APIKey: key, StructuredOutput: PromptOnly}) + if err != nil { + t.Fatal(err) + } + _, err = p.Generate(context.Background(), schemaRequest()) + if err == nil { + t.Fatal("Generate succeeded without a model") + } + assertLayersNoExactKey(t, err, key) + assertChainNoExactKey(t, err, key) + }) +} -- 2.55.0 From fba766794905039ec4a3ea0334f892b4209062e5 Mon Sep 17 00:00:00 2001 From: dev_agent Date: Sun, 4 Oct 2026 07:38:03 -0400 Subject: [PATCH 12/12] Keep only redacted text in typed errors, not the credential ProviderError and RateLimitedError now build their redacted Error text when constructed and do not store the secret, so formatting the value with %#v or %+v cannot print it. Co-Authored-By: Claude Opus 5.5 --- internal/ai/llm/errors.go | 44 ++++++++++++++++++++-------------- internal/ai/llm/errors_test.go | 12 ++++++++++ 2 files changed, 38 insertions(+), 18 deletions(-) diff --git a/internal/ai/llm/errors.go b/internal/ai/llm/errors.go index 50183e6..07fddef 100644 --- a/internal/ai/llm/errors.go +++ b/internal/ai/llm/errors.go @@ -32,22 +32,30 @@ type RateLimitedError struct { // RetryAfter is how long the vendor asked callers to wait; 0 means it // did not say. RetryAfter time.Duration - secret string + // text is the redacted Error text set by NewRateLimitedError; "" means + // the error was built without a credential. + text string } // NewRateLimitedError returns the error for a throttled request. secret is -// the credential sent with the request, or "" when the request sent none; -// Error removes every occurrence of a non-empty secret, even where the -// fixed text happens to spell it. +// the credential sent with the request, or "" when the request sent none. +// Every occurrence of a non-empty secret is removed from the error text +// when it is built, even where the fixed text happens to spell it, and the +// secret itself is not kept. func NewRateLimitedError(retryAfter time.Duration, secret string) *RateLimitedError { - return &RateLimitedError{RetryAfter: retryAfter, secret: secret} + e := &RateLimitedError{RetryAfter: retryAfter} + e.text = Redact(e.Error(), secret) + return e } func (e *RateLimitedError) Error() string { - if e.RetryAfter == 0 { - return Redact(ErrRateLimited.Error(), e.secret) + if e.text != "" { + return e.text } - return Redact(fmt.Sprintf("%s: retry after %v", ErrRateLimited, e.RetryAfter), e.secret) + if e.RetryAfter == 0 { + return ErrRateLimited.Error() + } + return fmt.Sprintf("%s: retry after %v", ErrRateLimited, e.RetryAfter) } // Is reports whether target is ErrRateLimited. @@ -84,7 +92,7 @@ func Redact(s, secret string) string { type ProviderError struct { status int body string - secret string + text string } // NewProviderError returns the error for an unexpected HTTP status. It @@ -92,8 +100,9 @@ type ProviderError struct { // credential sent with the request, or "" when the request sent none; every // occurrence of a non-empty secret in body is removed by Redact before the // body is cut, so no part of it survives, and again after the cut, so the -// truncation mark cannot complete it. Error removes it from the complete -// text too, even where the fixed text happens to spell it. +// truncation mark cannot complete it. It is removed from the complete error +// text too, even where the fixed text happens to spell it, and the secret +// itself is not kept. func NewProviderError(status int, body []byte, secret string) *ProviderError { s := strings.TrimSpace(Redact(string(body), secret)) if len(s) > MaxBodyExcerpt { @@ -103,7 +112,11 @@ func NewProviderError(status int, body []byte, secret string) *ProviderError { } s = Redact(s[:cut]+"…", secret) } - return &ProviderError{status: status, body: s, secret: secret} + text := fmt.Sprintf("%s: HTTP %d", ErrProvider, status) + if s != "" { + text += ": " + s + } + return &ProviderError{status: status, body: s, text: Redact(text, secret)} } // Status returns the HTTP status code. @@ -113,12 +126,7 @@ func (e *ProviderError) Status() int { return e.status } // when the vendor sent no body. func (e *ProviderError) Body() string { return e.body } -func (e *ProviderError) Error() string { - if e.body == "" { - return Redact(fmt.Sprintf("%s: HTTP %d", ErrProvider, e.status), e.secret) - } - return Redact(fmt.Sprintf("%s: HTTP %d: %s", ErrProvider, e.status, e.body), e.secret) -} +func (e *ProviderError) Error() string { return e.text } // Is reports whether target is ErrProvider. func (e *ProviderError) Is(target error) bool { return target == ErrProvider } diff --git a/internal/ai/llm/errors_test.go b/internal/ai/llm/errors_test.go index b5784ad..5ac5fa7 100644 --- a/internal/ai/llm/errors_test.go +++ b/internal/ai/llm/errors_test.go @@ -184,3 +184,15 @@ func TestRateLimitedErrorWithFixedWordSecretNeverLeaksSecret(t *testing.T) { } } } + +func TestTypedErrorsDoNotKeepSecret(t *testing.T) { + const secret = "sk-live-0123456789abcdef" + for _, err := range []error{ + NewProviderError(500, []byte("down "+secret), secret), + NewRateLimitedError(30*time.Second, secret), + } { + if got := fmt.Sprintf("%#v %+v", err, err); strings.Contains(got, secret) { + t.Fatalf("formatting %T leaks the secret: %q", err, got) + } + } +} -- 2.55.0