From 1b9745c3119762d6b87deae14253afa833eb6730 Mon Sep 17 00:00:00 2001 From: Ethan <39577870+ethanndickson@users.noreply.github.com> Date: Tue, 16 Jun 2026 00:21:13 +1000 Subject: [PATCH] fix: surface chat error diagnostics (#26367) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes CODAGT-223 ## What's already on `main` (via #25803) #25803 fixed how `detail` is *rendered* when present: `ChatStatusCallout` shows `status.detail` in a monospace `` block for `kind === "generic"`, `AgentChatPage` reads `error.response?.data?.detail` inline, and the auth message was tightened. It did not fix `detail` being absent in the first place. ## The gap `chaterror.Classify` only populates `Detail` from `*fantasy.ProviderError` (OpenAI-shaped JSON envelope). Every other realistic failure shape produces blank `Detail`: `context.DeadlineExceeded`, `Post "…": connection refused`, `stream error: stream ID …; INTERNAL_ERROR`, `Post "https://api.openai.com/…": 400 invalid model: gpt-9000`, `fantasy.Error` from the stream decoder, `xerrors.New("status 401 from upstream")`, HTTP/2 peer resets. Users still see the dead-end alert: "Request failed / The chat request failed unexpectedly." with no third line. ## The fix A new `chaterror.FormatDiagnosticDetail` entry point shares diagnostic-detail logic with `classify.go`: non-auth rule-table branches now fall back to a bounded raw error string when structured detail is absent, while auth-classified failures keep only structured provider detail. Curated branches (canceled, interrupted, Responses-API, stream-incomplete, chain-broken) are left alone. The `exp_chats.go` POST catch-all uses the exported helper, so the backend consistently emits a bounded diagnostic string instead of leaving `Detail` blank. Fallback diagnostics redact URLs preserved in typed transport errors by stripping userinfo, query strings, and fragments before display, which keeps provider error text useful while reducing credential exposure from standard request URL wrappers. ## Security This change surfaces upstream error text in the chat UI, where it is also persisted in `chats.last_error`, so it crosses a trust boundary. Codex brought this up as an issue through reviews. Mindful of cases like #20968, where a sensitive field leaked into agent logs, the design deliberately narrows what can reach a user: - Auth-classified failures keep only structured provider detail and never fall back to the raw error string. - Fallback diagnostics redact any URL preserved in a typed `*url.Error` by removing userinfo, query strings, and fragments, so credentials in standard transport URL wrappers do not leak. - Request-side credentials are not exposed: providers authenticate via headers, and `fantasy.ProviderError.Error()` does not print the URL or request dump. Dumped response headers are stripped before parsing, and detail is length-capped. The remaining channels are structured provider detail (`error.message` from the provider's response body), which is surfaced verbatim because it is the useful diagnostic this PR exists to deliver, and already-flattened fallback text where typed transport context has been lost. A well-behaved provider returns a description of the failure here, not a secret; OpenAI, for example, masks the middle of the submitted key and returns only a short fragment alongside a docs link. For a real secret to appear, the upstream API, or a proxy an admin points `base_url` at, would have to echo a plaintext credential into its own error body or flattened error prose. I judge that any secret leakage as a result of this PR would require a misbehaving API or middleware, and that the usefulness of real diagnostics outweighs that bounded risk. --- coderd/exp_chats.go | 3 +- coderd/x/chatd/chaterror/classify.go | 8 ++- coderd/x/chatd/chaterror/classify_test.go | 46 ++++++++++++++ coderd/x/chatd/chaterror/diagnostic.go | 58 ++++++++++++++++++ coderd/x/chatd/chaterror/diagnostic_test.go | 67 +++++++++++++++++++++ coderd/x/chatd/chaterror/payload_test.go | 1 + coderd/x/chatd/chatretry/chatretry_test.go | 1 + 7 files changed, 181 insertions(+), 3 deletions(-) create mode 100644 coderd/x/chatd/chaterror/diagnostic.go create mode 100644 coderd/x/chatd/chaterror/diagnostic_test.go diff --git a/coderd/exp_chats.go b/coderd/exp_chats.go index bca949f531..49967ada83 100644 --- a/coderd/exp_chats.go +++ b/coderd/exp_chats.go @@ -49,6 +49,7 @@ import ( "github.com/coder/coder/v2/coderd/workspaceapps" "github.com/coder/coder/v2/coderd/wsbuilder" "github.com/coder/coder/v2/coderd/x/chatd" + "github.com/coder/coder/v2/coderd/x/chatd/chaterror" "github.com/coder/coder/v2/coderd/x/chatd/chatprovider" "github.com/coder/coder/v2/coderd/x/chatd/chatstate" "github.com/coder/coder/v2/coderd/x/chatd/chattool" @@ -3123,7 +3124,7 @@ func (api *API) postChatMessages(rw http.ResponseWriter, r *http.Request) { } httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ Message: "Failed to create chat message.", - Detail: sendErr.Error(), + Detail: chaterror.FormatDiagnosticDetail(sendErr), }) return } diff --git a/coderd/x/chatd/chaterror/classify.go b/coderd/x/chatd/chaterror/classify.go index 6bfe3d101c..0147248e2f 100644 --- a/coderd/x/chatd/chaterror/classify.go +++ b/coderd/x/chatd/chaterror/classify.go @@ -292,8 +292,12 @@ func Classify(err error) ClassifiedError { if !rule.match { continue } + detail := structured.detail + if rule.kind != codersdk.ChatErrorKindAuth { + detail = resolveDiagnosticDetail(structured.detail, err) + } return normalizeClassification(ClassifiedError{ - Detail: structured.detail, + Detail: detail, Kind: rule.kind, Provider: provider, Retryable: rule.retryable, @@ -314,7 +318,7 @@ func Classify(err error) ClassifiedError { } return normalizeClassification(ClassifiedError{ - Detail: structured.detail, + Detail: resolveDiagnosticDetail(structured.detail, err), Kind: codersdk.ChatErrorKindGeneric, Provider: provider, StatusCode: statusCode, diff --git a/coderd/x/chatd/chaterror/classify_test.go b/coderd/x/chatd/chaterror/classify_test.go index 4bedc07874..1f8c2cabf4 100644 --- a/coderd/x/chatd/chaterror/classify_test.go +++ b/coderd/x/chatd/chaterror/classify_test.go @@ -32,6 +32,7 @@ func TestClassify(t *testing.T) { err: xerrors.New("status 529 from upstream"), want: chaterror.ClassifiedError{ Message: "The AI provider is temporarily overloaded.", + Detail: "status 529 from upstream", Kind: codersdk.ChatErrorKindOverloaded, Provider: "", Retryable: true, @@ -43,6 +44,7 @@ func TestClassify(t *testing.T) { err: xerrors.New("anthropic overloaded_error"), want: chaterror.ClassifiedError{ Message: "Anthropic is temporarily overloaded.", + Detail: "anthropic overloaded_error", Kind: codersdk.ChatErrorKindOverloaded, Provider: "anthropic", Retryable: true, @@ -93,6 +95,7 @@ func TestClassify(t *testing.T) { err: xerrors.New("invalid model"), want: chaterror.ClassifiedError{ Message: "The AI provider rejected the model configuration. Check the selected model and provider settings.", + Detail: "invalid model", Kind: codersdk.ChatErrorKindConfig, Provider: "", Retryable: false, @@ -137,6 +140,7 @@ func TestClassify(t *testing.T) { err: xerrors.New("forbidden: context length exceeded"), want: chaterror.ClassifiedError{ Message: "The AI provider rejected the model configuration. Check the selected model and provider settings.", + Detail: "forbidden: context length exceeded", Kind: codersdk.ChatErrorKindConfig, Provider: "", Retryable: false, @@ -148,6 +152,7 @@ func TestClassify(t *testing.T) { err: xerrors.New("status 429 from upstream"), want: chaterror.ClassifiedError{ Message: "The AI provider is rate limiting requests.", + Detail: "status 429 from upstream", Kind: codersdk.ChatErrorKindRateLimit, Provider: "", Retryable: true, @@ -159,6 +164,7 @@ func TestClassify(t *testing.T) { err: xerrors.New("status 429: invalid model"), want: chaterror.ClassifiedError{ Message: "The AI provider rejected the model configuration. Check the selected model and provider settings.", + Detail: "status 429: invalid model", Kind: codersdk.ChatErrorKindConfig, Provider: "", Retryable: false, @@ -170,6 +176,7 @@ func TestClassify(t *testing.T) { err: xerrors.New("status 429: invalid model quota"), want: chaterror.ClassifiedError{ Message: "The AI provider rejected the model configuration. Check the selected model and provider settings.", + Detail: "status 429: invalid model quota", Kind: codersdk.ChatErrorKindConfig, Provider: "", Retryable: false, @@ -181,6 +188,7 @@ func TestClassify(t *testing.T) { err: xerrors.New("service unavailable"), want: chaterror.ClassifiedError{ Message: "The AI provider is temporarily unavailable.", + Detail: "service unavailable", Kind: codersdk.ChatErrorKindTimeout, Provider: "", Retryable: true, @@ -192,6 +200,7 @@ func TestClassify(t *testing.T) { err: xerrors.New("status 503: invalid model"), want: chaterror.ClassifiedError{ Message: "The AI provider rejected the model configuration. Check the selected model and provider settings.", + Detail: "status 503: invalid model", Kind: codersdk.ChatErrorKindConfig, Provider: "", Retryable: false, @@ -203,6 +212,7 @@ func TestClassify(t *testing.T) { err: xerrors.New("service unavailable: model not found"), want: chaterror.ClassifiedError{ Message: "The AI provider rejected the model configuration. Check the selected model and provider settings.", + Detail: "service unavailable: model not found", Kind: codersdk.ChatErrorKindConfig, Provider: "", Retryable: false, @@ -214,6 +224,7 @@ func TestClassify(t *testing.T) { err: xerrors.New("connection refused: unsupported model"), want: chaterror.ClassifiedError{ Message: "The AI provider rejected the model configuration. Check the selected model and provider settings.", + Detail: "connection refused: unsupported model", Kind: codersdk.ChatErrorKindConfig, Provider: "", Retryable: false, @@ -225,6 +236,7 @@ func TestClassify(t *testing.T) { err: context.DeadlineExceeded, want: chaterror.ClassifiedError{ Message: "The request timed out before it completed.", + Detail: "context deadline exceeded", Kind: codersdk.ChatErrorKindTimeout, Provider: "", Retryable: false, @@ -236,6 +248,7 @@ func TestClassify(t *testing.T) { err: errors.Join(chaterror.ErrProviderTransportReset, context.Canceled), want: chaterror.ClassifiedError{ Message: "The AI provider is temporarily unavailable.", + Detail: "provider transport reset context canceled", Kind: codersdk.ChatErrorKindTimeout, Provider: "", Retryable: true, @@ -258,6 +271,7 @@ func TestClassify(t *testing.T) { err: xerrors.Errorf("received status 500 from upstream: %w", context.Canceled), want: chaterror.ClassifiedError{ Message: "The AI provider returned an unexpected error.", + Detail: "received status 500 from upstream: context canceled", Kind: codersdk.ChatErrorKindGeneric, Provider: "", Retryable: true, @@ -324,6 +338,7 @@ func TestClassify(t *testing.T) { err: xerrors.New(fmt.Sprintf("%s: AI provider %q is disabled", codersdk.ChatErrorKindProviderDisabled, "anthropic")), want: chaterror.ClassifiedError{ Message: "The Anthropic provider has been disabled. Contact your Coder administrator.", + Detail: fmt.Sprintf("%s: AI provider %q is disabled", codersdk.ChatErrorKindProviderDisabled, "anthropic"), Kind: codersdk.ChatErrorKindProviderDisabled, Provider: "anthropic", Retryable: false, @@ -663,6 +678,7 @@ func TestClassify_HTTP2TransportErrors(t *testing.T) { require.Equal(t, codersdk.ChatErrorKindTimeout, classified.Kind, "Kind") require.True(t, classified.Retryable, "Retryable") require.Equal(t, tt.provider, classified.Provider, "Provider") + require.Equal(t, tt.err, classified.Detail, "Detail") require.Equal(t, tt.wantMessage, classified.Message, "Message") }) } @@ -689,6 +705,7 @@ func TestClassify_HTTP2StreamErrorValues(t *testing.T) { err: peerReset(http2.ErrCodeInternal), want: chaterror.ClassifiedError{ Message: "The AI provider is temporarily unavailable.", + Detail: "stream error: stream ID 455; INTERNAL_ERROR; received from peer", Kind: codersdk.ChatErrorKindTimeout, Retryable: true, }, @@ -698,6 +715,7 @@ func TestClassify_HTTP2StreamErrorValues(t *testing.T) { err: peerReset(http2.ErrCodeRefusedStream), want: chaterror.ClassifiedError{ Message: "The AI provider is temporarily unavailable.", + Detail: "stream error: stream ID 455; REFUSED_STREAM; received from peer", Kind: codersdk.ChatErrorKindTimeout, Retryable: true, }, @@ -711,6 +729,7 @@ func TestClassify_HTTP2StreamErrorValues(t *testing.T) { }, want: chaterror.ClassifiedError{ Message: "The AI provider is temporarily unavailable.", + Detail: "stream error: stream ID 455; CANCEL; received from peer", Kind: codersdk.ChatErrorKindTimeout, Retryable: true, }, @@ -720,6 +739,7 @@ func TestClassify_HTTP2StreamErrorValues(t *testing.T) { err: peerReset(http2.ErrCodeEnhanceYourCalm), want: chaterror.ClassifiedError{ Message: "The AI provider is temporarily unavailable.", + Detail: "stream error: stream ID 455; ENHANCE_YOUR_CALM; received from peer", Kind: codersdk.ChatErrorKindTimeout, Retryable: true, }, @@ -729,6 +749,7 @@ func TestClassify_HTTP2StreamErrorValues(t *testing.T) { err: peerReset(http2.ErrCodeNo), want: chaterror.ClassifiedError{ Message: "The AI provider is temporarily unavailable.", + Detail: "stream error: stream ID 455; NO_ERROR; received from peer", Kind: codersdk.ChatErrorKindTimeout, Retryable: true, }, @@ -807,6 +828,7 @@ func TestClassify_HTTP2StreamIDDoesNotBecomeStatusCode(t *testing.T) { }, want: chaterror.ClassifiedError{ Message: "The AI provider is temporarily unavailable.", + Detail: "stream error: stream ID 401; INTERNAL_ERROR; received from peer", Kind: codersdk.ChatErrorKindTimeout, Retryable: true, }, @@ -820,6 +842,7 @@ func TestClassify_HTTP2StreamIDDoesNotBecomeStatusCode(t *testing.T) { }, want: chaterror.ClassifiedError{ Message: "The chat request failed unexpectedly.", + Detail: "stream error: stream ID 503; PROTOCOL_ERROR; received from peer", Kind: codersdk.ChatErrorKindGeneric, }, }, @@ -828,6 +851,7 @@ func TestClassify_HTTP2StreamIDDoesNotBecomeStatusCode(t *testing.T) { err: xerrors.New("stream error: stream ID 401; INTERNAL_ERROR; received from peer"), want: chaterror.ClassifiedError{ Message: "The AI provider is temporarily unavailable.", + Detail: "stream error: stream ID 401; INTERNAL_ERROR; received from peer", Kind: codersdk.ChatErrorKindTimeout, Retryable: true, }, @@ -837,6 +861,7 @@ func TestClassify_HTTP2StreamIDDoesNotBecomeStatusCode(t *testing.T) { err: xerrors.New("stream error: stream ID 503; PROTOCOL_ERROR; received from peer"), want: chaterror.ClassifiedError{ Message: "The chat request failed unexpectedly.", + Detail: "stream error: stream ID 503; PROTOCOL_ERROR; received from peer", Kind: codersdk.ChatErrorKindGeneric, }, }, @@ -1106,6 +1131,7 @@ func TestWithProviderUsesExplicitHint(t *testing.T) { enriched := classified.WithProvider("azure openai") require.Equal(t, chaterror.ClassifiedError{ Message: "Azure OpenAI is rate limiting requests.", + Detail: "openai received status 429 from upstream", Kind: codersdk.ChatErrorKindRateLimit, Provider: "azure", Retryable: true, @@ -1122,6 +1148,7 @@ func TestWithProviderAddsProviderWhenUnknown(t *testing.T) { enriched := classified.WithProvider("openai") require.Equal(t, chaterror.ClassifiedError{ Message: "OpenAI is rate limiting requests.", + Detail: "received status 429 from upstream", Kind: codersdk.ChatErrorKindRateLimit, Provider: "openai", Retryable: true, @@ -1241,6 +1268,25 @@ func TestClassify_UsesStructuredProviderDetailFromResponseDump(t *testing.T) { }, classified) } +func TestClassify_AuthKeepsStructuredProviderDetail(t *testing.T) { + t.Parallel() + + classified := chaterror.Classify(testProviderError( + "invalid api key test-key", + 401, + nil, + testProviderResponseDump(`{"error":{"message":"Incorrect API key provided."}}`), + )) + + require.Equal(t, chaterror.ClassifiedError{ + Message: "Authentication with the AI provider failed. Check the API key and permissions.", + Detail: "Incorrect API key provided.", + Kind: codersdk.ChatErrorKindAuth, + Retryable: false, + StatusCode: 401, + }, classified) +} + func TestClassify_FallsBackToProviderMessageForDetail(t *testing.T) { t.Parallel() diff --git a/coderd/x/chatd/chaterror/diagnostic.go b/coderd/x/chatd/chaterror/diagnostic.go new file mode 100644 index 0000000000..9d5df96a25 --- /dev/null +++ b/coderd/x/chatd/chaterror/diagnostic.go @@ -0,0 +1,58 @@ +package chaterror + +import ( + "errors" + "net/url" + "strings" +) + +// FormatDiagnosticDetail returns a bounded, single-line diagnostic string from +// err, suitable for surfacing to a user. +func FormatDiagnosticDetail(err error) string { + return resolveDiagnosticDetail("", err) +} + +// resolveDiagnosticDetail picks the detail string to surface: structured +// provider detail always wins, otherwise the raw error string is used as a +// fallback after redacting any URL preserved in a typed *url.Error and bounding +// its length. +func resolveDiagnosticDetail(structured string, err error) string { + if strings.TrimSpace(structured) != "" { + return structured + } + if err == nil { + return "" + } + detail := strings.TrimSpace(err.Error()) + if detail == "" { + return "" + } + detail = redactTypedTransportURL(detail, err) + return normalizeClassificationDetail(strings.Join(strings.Fields(detail), " ")) +} + +func redactTypedTransportURL(message string, err error) string { + var urlErr *url.Error + if !errors.As(err, &urlErr) || urlErr == nil || urlErr.URL == "" { + return message + } + redactedURL, changed := redactDiagnosticURL(urlErr.URL) + if !changed { + return message + } + return strings.ReplaceAll(message, urlErr.URL, redactedURL) +} + +func redactDiagnosticURL(rawURL string) (string, bool) { + parsed, err := url.Parse(rawURL) + if err != nil { + return "[REDACTED_URL]", true + } + redacted := *parsed + redacted.User = nil + redacted.RawQuery = "" + redacted.ForceQuery = false + redacted.Fragment = "" + redactedURL := redacted.String() + return redactedURL, redactedURL != rawURL +} diff --git a/coderd/x/chatd/chaterror/diagnostic_test.go b/coderd/x/chatd/chaterror/diagnostic_test.go new file mode 100644 index 0000000000..32ec097985 --- /dev/null +++ b/coderd/x/chatd/chaterror/diagnostic_test.go @@ -0,0 +1,67 @@ +package chaterror_test + +import ( + "net/url" + "strings" + "testing" + + "github.com/stretchr/testify/require" + "golang.org/x/xerrors" + + "github.com/coder/coder/v2/coderd/x/chatd/chaterror" +) + +func TestFormatDiagnosticDetail(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + err error + want string + }{ + { + name: "Nil", + err: nil, + }, + { + name: "CollapsesWhitespace", + err: xerrors.New("stream response:\n\tconnection reset by peer"), + want: "stream response: connection reset by peer", + }, + { + name: "RedactsURLUserinfoQueryAndFragment", + err: &url.Error{ + Op: "Post", + URL: "https://test-user:test-password@gateway.internal/v1/chat?test_token=test-value#fragment", + Err: xerrors.New("unexpected EOF"), + }, + want: `Post "https://gateway.internal/v1/chat": unexpected EOF`, + }, + { + name: "RedactsWrappedURLError", + err: xerrors.Errorf("stream failed: %w", &url.Error{ + Op: "Get", + URL: "https://test-key@gateway.internal/v1/chat?test_token=test-value", + Err: xerrors.New("connection refused"), + }), + want: `stream failed: Get "https://gateway.internal/v1/chat": connection refused`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := chaterror.FormatDiagnosticDetail(tt.err) + require.Equal(t, tt.want, got) + }) + } +} + +func TestFormatDiagnosticDetail_TruncatesLongDiagnostic(t *testing.T) { + t.Parallel() + + got := chaterror.FormatDiagnosticDetail(xerrors.New(strings.Repeat("x", 510))) + + require.Len(t, []rune(got), 500) + require.True(t, strings.HasSuffix(got, "…")) +} diff --git a/coderd/x/chatd/chaterror/payload_test.go b/coderd/x/chatd/chaterror/payload_test.go index 2843e37430..a03d5ad197 100644 --- a/coderd/x/chatd/chaterror/payload_test.go +++ b/coderd/x/chatd/chaterror/payload_test.go @@ -22,6 +22,7 @@ func TestTerminalErrorPayloadUsesNormalizedClassification(t *testing.T) { require.Equal(t, &codersdk.ChatError{ Message: "Azure OpenAI is rate limiting requests.", + Detail: "azure openai received status 429 from upstream", Kind: codersdk.ChatErrorKindRateLimit, Provider: "azure", Retryable: true, diff --git a/coderd/x/chatd/chatretry/chatretry_test.go b/coderd/x/chatd/chatretry/chatretry_test.go index b750548d0a..4e4cc74b26 100644 --- a/coderd/x/chatd/chatretry/chatretry_test.go +++ b/coderd/x/chatd/chatretry/chatretry_test.go @@ -173,6 +173,7 @@ func TestRetry_ContextCanceledFromAttemptWithHealthyParentRetries(t *testing.T) require.ErrorIs(t, retryErr, context.Canceled) require.Equal(t, chaterror.ClassifiedError{ Message: "The AI provider is temporarily unavailable.", + Detail: "provider transport reset context canceled", Kind: codersdk.ChatErrorKindTimeout, Retryable: true, StatusCode: 0,