From ee1405cc1ae6bd61f8bdd2469fdd4b9fa682d28b Mon Sep 17 00:00:00 2001 From: GreenEclipse Date: Sun, 23 Aug 2026 12:56:19 +0200 Subject: [PATCH] fix --- apps/api/internal/aiprovider/roles_test.go | 8 +- apps/api/internal/aiprovider/service.go | 30 ++++- apps/api/internal/processing/openai.go | 22 ++-- .../internal/processing/openai_api_error.go | 110 ++++++++++++++++++ .../processing/openai_api_error_test.go | 90 ++++++++++++++ apps/api/internal/processing/openai_test.go | 8 ++ apps/api/internal/processing/sanitize.go | 21 +++- 7 files changed, 260 insertions(+), 29 deletions(-) create mode 100644 apps/api/internal/processing/openai_api_error.go create mode 100644 apps/api/internal/processing/openai_api_error_test.go diff --git a/apps/api/internal/aiprovider/roles_test.go b/apps/api/internal/aiprovider/roles_test.go index 997c39e..66d89b3 100644 --- a/apps/api/internal/aiprovider/roles_test.go +++ b/apps/api/internal/aiprovider/roles_test.go @@ -35,8 +35,8 @@ func TestResolveCompleterForRole_unsetFallsBackToEnv(t *testing.T) { if !ok || oc == nil || !oc.Enabled() { t.Fatalf("expected enabled OpenAIClient, got %T", c) } - if label != ModeInternalLabel { - t.Fatalf("label=%q", label) + if label != ModeCustomLabel { + t.Fatalf("label=%q want %q (env bootstrap is custom)", label, ModeCustomLabel) } if byok { t.Fatal("env fallback must not be BYOK") @@ -97,8 +97,8 @@ func TestResolveCompleterForRole_platformProcessingRole(t *testing.T) { if oc.APIKey != "sk-plat-processing" || oc.Model != "plat-model" { t.Fatalf("key=%q model=%q", oc.APIKey, oc.Model) } - if label != ModeInternalLabel || byok { - t.Fatalf("label=%q byok=%v", label, byok) + if label != ModeCustomLabel || byok { + t.Fatalf("label=%q byok=%v want custom (loopback platform base)", label, byok) } } diff --git a/apps/api/internal/aiprovider/service.go b/apps/api/internal/aiprovider/service.go index e783194..c56e5de 100644 --- a/apps/api/internal/aiprovider/service.go +++ b/apps/api/internal/aiprovider/service.go @@ -389,9 +389,14 @@ func (s *Service) TestPlatformRole(ctx context.Context, role string) (map[string out["message"] = "AI role is not configured (or disabled) in admin platform settings" return out, nil } - if _, err := completer.Complete(ctx, "Reply with exactly: ok", "ping"); err != nil { + if err := probeCompleter(ctx, completer); err != nil { out["status"] = "failed" - out["message"] = "connection failed — check provider, key, base URL, and model" + // Prefer classified probe detail (no secrets / model-id dumps) over a generic blur. + msg := processing.TruncateError(err) + if msg == "" || msg == "processing_failed" || msg == "provider error (details redacted)" { + msg = "connection failed — check provider, key, base URL, and model" + } + out["message"] = msg return out, err } out["status"] = "ok" @@ -399,6 +404,21 @@ func (s *Service) TestPlatformRole(ctx context.Context, role string) (map[string return out, nil } +// probeCompleter runs a minimal chat completion with GPT-5.6-safe options +// (max_completion_tokens + reasoning_effort=none, no custom temperature). +func probeCompleter(ctx context.Context, completer processing.Completer) error { + if completer == nil { + return ErrNotConfigured + } + opts := processing.ProbeCompleteOptions() + if co, ok := completer.(processing.CompleterWithOptions); ok { + _, err := co.CompleteWithOptions(ctx, "Reply with exactly: ok", "ping", opts) + return err + } + _, err := completer.Complete(ctx, "Reply with exactly: ok", "ping") + return err +} + // TestConnection sends a minimal chat completion and records last_test_*. func (s *Service) TestConnection(ctx context.Context, companyID uuid.UUID) (map[string]any, error) { ctx, cancel := context.WithTimeout(ctx, aiProbeTimeout) @@ -422,12 +442,10 @@ func (s *Service) TestConnection(ctx context.Context, companyID uuid.UUID) (map[ WHERE company_id = $1`, companyID, status) return map[string]any{"status": status, "message": message, "mode": resolved.ModeLabel}, ErrNotConfigured } - _, err = resolved.Completer.Complete(ctx, "Reply with exactly: ok", "ping") - if err != nil { + if err := probeCompleter(ctx, resolved.Completer); err != nil { status = "failed" - // TruncateError classifies transport/auth failures without leaking secrets. message = processing.TruncateError(err) - if message == "" || message == "provider error (details redacted)" { + if message == "" || message == "processing_failed" || message == "provider error (details redacted)" { message = "connection failed — check provider, key, base URL, and model" } _, _ = s.Pool.Exec(ctx, ` diff --git a/apps/api/internal/processing/openai.go b/apps/api/internal/processing/openai.go index 4f44baf..6568fad 100644 --- a/apps/api/internal/processing/openai.go +++ b/apps/api/internal/processing/openai.go @@ -243,10 +243,7 @@ type chatResponse struct { CompletionTokens int `json:"completion_tokens"` TotalTokens int `json:"total_tokens"` } `json:"usage"` - Error *struct { - Message string `json:"message"` - Type string `json:"type"` - } `json:"error"` + Error *openAIErrorBody `json:"error"` } var ( @@ -466,8 +463,13 @@ func (c *OpenAIClient) doEmbed(ctx context.Context, texts []string) ([][]float32 } func (c *OpenAIClient) doComplete(ctx context.Context, system, user string, temperature float64, maxTokens int, reasoningEffort string) (Completion, bool, error) { + sysRole := "system" + if openAIReasoningChatModel(c.Model) { + // GPT-5 family prefers developer over system on Chat Completions. + sysRole = "developer" + } reqBody := buildChatCompletionBody(c.Model, []chatMessage{ - {Role: "system", Content: system}, + {Role: sysRole, Content: system}, {Role: "user", Content: user}, }, temperature, maxTokens, reasoningEffort) body, err := json.Marshal(reqBody) @@ -502,19 +504,13 @@ func (c *OpenAIClient) doComplete(ctx context.Context, system, user string, temp return Completion{}, false, decErr } if res.StatusCode == http.StatusTooManyRequests || res.StatusCode >= 500 { - msg := "rate limited or server error" - if parsed.Error != nil && parsed.Error.Message != "" { - msg = TruncateError(errors.New(parsed.Error.Message)) - } + msg := formatOpenAIHTTPError(res.StatusCode, parsed.Error) httpErr := errors.New(msg) stopWait(httpErr) return Completion{}, true, httpErr } if res.StatusCode >= 400 { - msg := fmt.Sprintf("openai http %d", res.StatusCode) - if parsed.Error != nil && parsed.Error.Message != "" { - msg = TruncateError(errors.New(parsed.Error.Message)) - } + msg := formatOpenAIHTTPError(res.StatusCode, parsed.Error) httpErr := errors.New(msg) stopWait(httpErr) return Completion{}, false, httpErr diff --git a/apps/api/internal/processing/openai_api_error.go b/apps/api/internal/processing/openai_api_error.go new file mode 100644 index 0000000..8e9da74 --- /dev/null +++ b/apps/api/internal/processing/openai_api_error.go @@ -0,0 +1,110 @@ +package processing + +import ( + "fmt" + "strings" +) + +// openAIErrorBody is the error object returned by OpenAI-compatible APIs. +type openAIErrorBody struct { + Message string `json:"message"` + Type string `json:"type"` + Code any `json:"code"` // string or number depending on provider + Param string `json:"param"` +} + +func (e *openAIErrorBody) codeString() string { + if e == nil { + return "" + } + switch v := e.Code.(type) { + case string: + return strings.TrimSpace(v) + case float64: + if v == float64(int64(v)) { + return fmt.Sprintf("%d", int64(v)) + } + return fmt.Sprintf("%g", v) + case int: + return fmt.Sprintf("%d", v) + default: + return "" + } +} + +// formatOpenAIHTTPError turns an upstream HTTP error into a short, secret-safe +// operator message. Intentionally avoids embedding model ids (gpt-…) so +// TruncateError / mapPublicErrorCode do not collapse useful validation text to +// "processing_failed". +func formatOpenAIHTTPError(status int, errObj *openAIErrorBody) string { + msg := "" + code := "" + param := "" + typ := "" + if errObj != nil { + msg = strings.TrimSpace(errObj.Message) + code = errObj.codeString() + param = strings.TrimSpace(errObj.Param) + typ = strings.TrimSpace(errObj.Type) + } + lower := strings.ToLower(msg) + codeLower := strings.ToLower(code) + paramLower := strings.ToLower(param) + + switch { + case status == 401 || codeLower == "invalid_api_key" || + strings.Contains(lower, "invalid api key") || + strings.Contains(lower, "incorrect api key"): + return "AI provider rejected the API key" + case status == 403: + return "AI provider forbidden the request" + case status == 429 || codeLower == "rate_limit_exceeded": + return "AI provider rate limited — retry later" + case codeLower == "insufficient_quota" || strings.Contains(lower, "insufficient_quota") || + strings.Contains(lower, "exceeded your current quota"): + return "OpenAI quota exceeded — check billing" + case strings.Contains(lower, "does not exist") || strings.Contains(lower, "do not have access") || + codeLower == "model_not_found": + return "model not found or API key lacks access — check model name and project permissions" + case strings.Contains(lower, "max_tokens") && strings.Contains(lower, "max_completion"): + return "model requires completion-token budget (legacy token cap unsupported)" + case paramLower == "temperature" || strings.Contains(lower, "temperature"): + return "model rejects custom temperature — omit temperature for reasoning chat models" + case paramLower == "max_tokens" || (strings.Contains(lower, "unsupported parameter") && strings.Contains(lower, "max_tokens")): + return "model rejects legacy token cap — use completion-token budget" + case paramLower == "reasoning_effort" || strings.Contains(lower, "reasoning_effort"): + return "model rejected reasoning effort — try none/low or omit" + case strings.Contains(lower, "unsupported parameter") || strings.Contains(lower, "unsupported value"): + if param != "" { + return fmt.Sprintf("unsupported chat parameter %q", param) + } + return "unsupported chat parameter for this model" + case status >= 500: + return "AI provider temporarily unavailable (server error)" + } + + // Prefer structured fields over raw message (raw often embeds model ids / urls). + if code != "" && param != "" { + return fmt.Sprintf("provider http %d (%s param=%s)", status, code, param) + } + if code != "" { + return fmt.Sprintf("provider http %d (%s)", status, code) + } + if typ != "" { + return fmt.Sprintf("provider http %d (%s)", status, typ) + } + if status > 0 { + return fmt.Sprintf("provider http %d", status) + } + return "provider request failed" +} + +// ProbeCompleteOptions is the Chat Completions budget for admin connection tests. +// Explicit max_completion_tokens + reasoning_effort=none avoids GPT-5.6 defaults +// spending the budget on hidden reasoning for a one-token "ok" reply. +func ProbeCompleteOptions() CompleteOptions { + return CompleteOptions{ + MaxTokens: 64, + ReasoningEffort: "none", + } +} diff --git a/apps/api/internal/processing/openai_api_error_test.go b/apps/api/internal/processing/openai_api_error_test.go new file mode 100644 index 0000000..c013698 --- /dev/null +++ b/apps/api/internal/processing/openai_api_error_test.go @@ -0,0 +1,90 @@ +package processing + +import ( + "strings" + "testing" +) + +func TestFormatOpenAIHTTPError_preservesUsefulHints(t *testing.T) { + t.Parallel() + cases := []struct { + status int + err *openAIErrorBody + want string + }{ + { + status: 400, + err: &openAIErrorBody{ + Message: "Unsupported parameter: 'temperature' does not support 0.2 with this model. Only the default (1) value is supported.", + Type: "invalid_request_error", + Code: "unsupported_value", + Param: "temperature", + }, + want: "model rejects custom temperature", + }, + { + status: 400, + err: &openAIErrorBody{ + Message: "Unsupported parameter: 'max_tokens' is not supported with this model. Use 'max_completion_tokens' instead.", + Param: "max_tokens", + }, + want: "model requires completion-token budget", + }, + { + status: 404, + err: &openAIErrorBody{ + Message: "The model `gpt-5.6-luna` does not exist or you do not have access to it.", + Code: "model_not_found", + }, + want: "model not found or API key lacks access", + }, + { + status: 400, + err: &openAIErrorBody{ + Message: "Unsupported parameter: 'max_tokens' is not supported with this model.", + Param: "max_tokens", + Code: "unsupported_parameter", + }, + want: "model rejects legacy token cap", + }, + { + status: 401, + err: &openAIErrorBody{ + Message: "Incorrect API key provided: sk-proj-REDACTED", + Code: "invalid_api_key", + }, + want: "AI provider rejected the API key", + }, + { + status: 400, + err: &openAIErrorBody{ + Message: "something obscure mentioning gpt-5.6-luna", + Type: "invalid_request_error", + Code: "invalid_request_error", + Param: "messages", + }, + want: "provider http 400 (invalid_request_error param=messages)", + }, + } + for _, tc := range cases { + got := formatOpenAIHTTPError(tc.status, tc.err) + if !strings.Contains(strings.ToLower(got), strings.ToLower(tc.want)) { + t.Fatalf("status=%d got=%q want contains %q", tc.status, got, tc.want) + } + if strings.Contains(got, "sk-") || strings.Contains(got, "gpt-5.6-luna") { + t.Fatalf("leaked secret or model id: %q", got) + } + safe := TruncateError(errString(got)) + if safe == "processing_failed" { + t.Fatalf("TruncateError wiped useful hint: in=%q", got) + } + } +} + +func TestProbeCompleteOptions(t *testing.T) { + t.Parallel() + opts := ProbeCompleteOptions() + if opts.MaxTokens != 64 || opts.ReasoningEffort != "none" { + t.Fatalf("%+v", opts) + } +} diff --git a/apps/api/internal/processing/openai_test.go b/apps/api/internal/processing/openai_test.go index 39529b9..a94ae8b 100644 --- a/apps/api/internal/processing/openai_test.go +++ b/apps/api/internal/processing/openai_test.go @@ -184,6 +184,14 @@ func TestOpenAIClient_doComplete_gpt56LunaUsesMaxCompletionTokens(t *testing.T) srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { var req map[string]any _ = json.NewDecoder(r.Body).Decode(&req) + msgs, _ := req["messages"].([]any) + if len(msgs) < 1 { + t.Errorf("expected messages") + } else if m0, ok := msgs[0].(map[string]any); ok { + if m0["role"] != "developer" { + t.Errorf("system role=%v want developer for gpt-5.6", m0["role"]) + } + } if _, ok := req["temperature"]; ok { t.Errorf("temperature must be omitted for gpt-5.6-luna") } diff --git a/apps/api/internal/processing/sanitize.go b/apps/api/internal/processing/sanitize.go index c9747a6..ca52b8f 100644 --- a/apps/api/internal/processing/sanitize.go +++ b/apps/api/internal/processing/sanitize.go @@ -232,17 +232,26 @@ func classifyProviderError(msg string) string { strings.Contains(lower, "http 401"), strings.Contains(lower, "invalid api key"), strings.Contains(lower, "incorrect api key"), - strings.Contains(lower, "invalid_api_key"): + strings.Contains(lower, "invalid_api_key"), + strings.Contains(lower, "rejected the api key"): return "AI provider rejected the API key" case strings.Contains(lower, "http 403"), - lower == "forbidden": + lower == "forbidden", + strings.Contains(lower, "forbidden the request"): return "AI provider forbidden the request" - case strings.Contains(lower, "http 429"), - strings.Contains(lower, "too many requests"), - lower == "rate limited": - return "AI provider rate limited — retry later" case lower == "rate limited or server error": return "AI provider temporarily unavailable (rate limited or server error)" + case strings.Contains(lower, "http 429"), + strings.Contains(lower, "too many requests"), + lower == "rate limited", + strings.Contains(lower, "rate limited —"), + strings.Contains(lower, "rate limited -"): + return "AI provider rate limited — retry later" + case strings.Contains(lower, "quota exceeded"): + return "OpenAI quota exceeded — check billing" + case strings.Contains(lower, "model not found"), + strings.Contains(lower, "lacks access"): + return "model not found or API key lacks access — check model name and project permissions" } return "" }