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. | — |