From 21a2652343ea67650ee5ccdb2113dca15e458880 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 16 Jun 2026 11:14:37 +0100 Subject: [PATCH] fix(coderd/x/chatd): show correct provider and clean detail for Bedrock errors (#26338) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem Bedrock errors (routed through aibridge) showed the wrong provider ("Anthropic ...") and a doubly-wrapped detail string instead of the clean message. ## Fix (chatd only) - **Provider label:** thread the configured provider to error classification via `GenerateAssistantOptions.ErrorProvider`. Transport provider still drives prompt prep, sanitization, and metric labels (unchanged). - **Detail:** unwrap the SDK transport wrapper (`METHOD "URL": NNN {body}`) to surface the inner message; handles top-level `{"message":...}` and nested `{"error":{"message":...}}`. ## Notes - Surfacing a top-level `message` now applies to all providers (intentional; nested wins when both present). - The advisor path keeps the transport label; accepted as-is (it returns `err.Error()`, not the classification). - Fixes display in chatd only; the wrapper originates in aibridge (out of scope here). 🤖 Generated by Coder Agents. --- coderd/x/chatd/chaterror/classify_test.go | 150 ++++++++++++++++++ coderd/x/chatd/chaterror/provider_error.go | 78 +++++++-- coderd/x/chatd/chatloop/chatloop.go | 22 ++- .../chatloop/chatloop_run_internal_test.go | 27 ++++ coderd/x/chatd/chatloop/metrics_test.go | 10 +- coderd/x/chatd/generation.go | 5 + coderd/x/chatd/generation_preparer.go | 35 ++-- 7 files changed, 293 insertions(+), 34 deletions(-) diff --git a/coderd/x/chatd/chaterror/classify_test.go b/coderd/x/chatd/chaterror/classify_test.go index 1f8c2cabf4..1fb0a46e5b 100644 --- a/coderd/x/chatd/chaterror/classify_test.go +++ b/coderd/x/chatd/chaterror/classify_test.go @@ -1268,6 +1268,141 @@ func TestClassify_UsesStructuredProviderDetailFromResponseDump(t *testing.T) { }, classified) } +func TestClassify_UsesTopLevelProviderMessage(t *testing.T) { + t.Parallel() + + // Many providers return a bare top-level message rather than the + // nested error envelope. Surface that message directly instead of the + // raw provider error string. + classified := chaterror.Classify(testProviderError( + "", + 400, + nil, + testProviderResponseDump(`{"message":"The provided request is not valid"}`), + )) + + require.Equal(t, "The provided request is not valid", classified.Detail) +} + +func TestClassify_UnwrapsBedrockTransportWrapper(t *testing.T) { + t.Parallel() + + // AWS Bedrock errors reach chatd wrapped twice: aibridge returns the + // nested Anthropic envelope, but error.message is itself the Anthropic + // SDK transport string that embeds the raw Bedrock body. + wrapped := `POST \"https://bedrock-runtime.eu-north-1.amazonaws.com/v1/messages\": 400 Bad Request {\"message\":\"The provided request is not valid\"}` + classified := chaterror.Classify(testProviderError( + "", + 400, + nil, + testProviderResponseDump(`{"error":{"message":"`+wrapped+`","type":"api_error"}}`), + )).WithProvider("bedrock") + + require.Equal(t, chaterror.ClassifiedError{ + Message: "AWS Bedrock returned an unexpected error.", + Detail: "The provided request is not valid", + Kind: codersdk.ChatErrorKindGeneric, + Provider: "bedrock", + Retryable: false, + StatusCode: 400, + }, classified) +} + +func TestClassify_DoesNotUnwrapNonTransportMessage(t *testing.T) { + t.Parallel() + + // A plain nested message that does not match the transport wrapper + // prefix must pass through unchanged, braces and all. + classified := chaterror.Classify(testProviderError( + "", + 400, + nil, + testProviderResponseDump(`{"error":{"message":"Value {x} is not allowed."}}`), + )) + + require.Equal(t, "Value {x} is not allowed.", classified.Detail) +} + +func TestClassify_UnwrapsTransportWrapperWithBraceInURL(t *testing.T) { + t.Parallel() + + // A templated URL containing a brace must not be mistaken for the JSON + // body; the inner message is still extracted. + wrapped := `POST \"https://example.com/{resource}/invoke\": 400 Bad Request {\"message\":\"real error\"}` + classified := chaterror.Classify(testProviderError( + "", + 400, + nil, + testProviderResponseDump(`{"error":{"message":"`+wrapped+`"}}`), + )) + + require.Equal(t, "real error", classified.Detail) +} + +func TestClassify_KeepsTransportWrapperWhenNoBody(t *testing.T) { + t.Parallel() + + // When the message matches the transport prefix but has no JSON body at + // all after it, the wrapper is surfaced unchanged. + classified := chaterror.Classify(testProviderError( + `POST "https://example.com/api": 500 Internal Server Error`, + 500, + nil, + )) + + require.Equal(t, `POST "https://example.com/api": 500 Internal Server Error`, classified.Detail) +} + +func TestClassify_KeepsTransportWrapperWhenInnerBodyNotJSON(t *testing.T) { + t.Parallel() + + // When the message matches the transport prefix but the trailing body + // has no extractable message, the wrapper is surfaced unchanged rather + // than dropped. + wrapped := `POST \"https://bedrock-runtime.eu-north-1.amazonaws.com/v1/messages\": 400 Bad Request {\"foo\":\"bar\"}` + classified := chaterror.Classify(testProviderError( + "", + 400, + nil, + testProviderResponseDump(`{"error":{"message":"`+wrapped+`"}}`), + )) + + require.Equal(t, + `POST "https://bedrock-runtime.eu-north-1.amazonaws.com/v1/messages": 400 Bad Request {"foo":"bar"}`, + classified.Detail) +} + +func TestClassify_PrefersNestedMessageOverTopLevel(t *testing.T) { + t.Parallel() + + // When both shapes are present, the nested error.message wins. + classified := chaterror.Classify(testProviderError( + "", + 400, + nil, + testProviderResponseDump(`{"message":"top level","error":{"message":"nested wins"}}`), + )) + + require.Equal(t, "nested wins", classified.Detail) +} + +// TestClassify_KeepsTopLevelMessageWhenErrorIsNonObject guards against a +// regression where a single decode into a combined struct would fail (and +// silently drop a usable top-level message) whenever "error" is present as a +// non-object value such as a string code. +func TestClassify_KeepsTopLevelMessageWhenErrorIsNonObject(t *testing.T) { + t.Parallel() + + classified := chaterror.Classify(testProviderError( + "", + 429, + nil, + testProviderResponseDump(`{"message":"rate limited","error":"rate_limit"}`), + )) + + require.Equal(t, "rate limited", classified.Detail) +} + func TestClassify_AuthKeepsStructuredProviderDetail(t *testing.T) { t.Parallel() @@ -1300,6 +1435,21 @@ func TestClassify_FallsBackToProviderMessageForDetail(t *testing.T) { require.Equal(t, "image exceeds 5 MB maximum", classified.Detail) } +func TestClassify_UnwrapsTransportWrapperInMessageFallback(t *testing.T) { + t.Parallel() + + // When the response dump is unavailable, the detail falls back to + // providerErr.Message, which for Bedrock via aibridge is itself the SDK + // transport wrapper. It must be unwrapped to the clean inner message. + classified := chaterror.Classify(testProviderError( + `POST "https://bedrock-runtime.eu-north-1.amazonaws.com/v1/messages": 400 Bad Request {"message":"The provided request is not valid"}`, + 400, + nil, + )) + + require.Equal(t, "The provided request is not valid", classified.Detail) +} + func TestClassify_TruncatesProviderDetail(t *testing.T) { t.Parallel() diff --git a/coderd/x/chatd/chaterror/provider_error.go b/coderd/x/chatd/chaterror/provider_error.go index d588d0f401..0b3b440639 100644 --- a/coderd/x/chatd/chaterror/provider_error.go +++ b/coderd/x/chatd/chaterror/provider_error.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "net/http" + "regexp" "strconv" "strings" "time" @@ -12,6 +13,13 @@ import ( "charm.land/fantasy" ) +// transportErrorPrefix matches the Anthropic SDK transport error format +// `METHOD "URL": ` that wraps the real provider +// response body. The SDK emits this when it cannot parse a non-Anthropic +// response, so AWS Bedrock errors arrive through aibridge with this +// wrapper, hiding the useful message inside a trailing JSON body. +var transportErrorPrefix = regexp.MustCompile(`^[A-Z]+ "[^"]+": \d{3} `) + type providerErrorDetails struct { detail string statusCode int @@ -35,26 +43,76 @@ func providerErrorDetail(providerErr *fantasy.ProviderError) string { if detail := providerErrorResponseMessage(providerErr.ResponseBody); detail != "" { return detail } - return strings.TrimSpace(providerErr.Message) + // The Message fallback can also be the SDK transport wrapper (e.g. for + // Bedrock via aibridge), so unwrap it for the same clean detail. + return unwrapTransportErrorMessage(strings.TrimSpace(providerErr.Message)) } -// providerErrorResponseMessage extracts error.message from the common -// provider error JSON envelope after stripping any dumped HTTP status -// line and headers. +// providerErrorResponseMessage extracts the human-readable message from a +// provider error response body after stripping any dumped HTTP status line +// and headers. It understands both the top-level `{"message":...}` shape +// used by many providers and the nested `{"error":{"message":...}}` +// envelope. When the extracted message is itself an SDK-formatted transport +// error wrapper, the clean inner provider message is returned. func providerErrorResponseMessage(responseDump []byte) string { if len(responseDump) == 0 || len(responseDump) > 64*1024 { return "" } body := providerErrorResponseBody(responseDump) - var envelope struct { - Error struct { - Message string `json:"message"` - } `json:"error"` + return unwrapTransportErrorMessage(jsonErrorMessage(body)) +} + +// unwrapTransportErrorMessage extracts the clean provider message from an +// SDK-formatted wrapper such as: +// +// POST "https://bedrock-runtime...": 400 Bad Request {"message":"..."} +// +// When the trailing body is JSON with a top-level "message" or a nested +// "error.message", that inner message is returned. Otherwise msg is +// returned unchanged. +func unwrapTransportErrorMessage(msg string) string { + loc := transportErrorPrefix.FindStringIndex(msg) + if loc == nil { + return msg } - if err := json.Unmarshal(body, &envelope); err != nil { + // Search for the JSON body after the matched prefix so a brace inside + // the URL or status text cannot be mistaken for the body. + rest := msg[loc[1]:] + start := strings.IndexByte(rest, '{') + if start < 0 { + return msg + } + if inner := jsonErrorMessage([]byte(rest[start:])); inner != "" { + return inner + } + return msg +} + +// jsonErrorMessage parses both the nested `{"error":{"message":...}}` +// envelope and the top-level `{"message":...}` shape used by many +// providers, preferring the nested form when present. +func jsonErrorMessage(body []byte) string { + var env struct { + Message string `json:"message"` + Error json.RawMessage `json:"error"` + } + if err := json.Unmarshal(body, &env); err != nil { return "" } - return strings.TrimSpace(envelope.Error.Message) + // Prefer the nested error.message when error is an object carrying one. + // error may also be a non-object (string, array, number); tolerate that + // and fall back to the top-level message instead of dropping it. + if len(env.Error) > 0 { + var inner struct { + Message string `json:"message"` + } + if err := json.Unmarshal(env.Error, &inner); err == nil { + if m := strings.TrimSpace(inner.Message); m != "" { + return m + } + } + } + return strings.TrimSpace(env.Message) } func providerErrorResponseBody(responseDump []byte) []byte { diff --git a/coderd/x/chatd/chatloop/chatloop.go b/coderd/x/chatd/chatloop/chatloop.go index f2aa7dab19..64c010ab10 100644 --- a/coderd/x/chatd/chatloop/chatloop.go +++ b/coderd/x/chatd/chatloop/chatloop.go @@ -1,6 +1,7 @@ package chatloop import ( + "cmp" "context" "database/sql" "encoding/base64" @@ -210,7 +211,13 @@ type RunOptions struct { // GenerateAssistantOptions configures one assistant model call. type GenerateAssistantOptions struct { - Model fantasy.LanguageModel + Model fantasy.LanguageModel + // ErrorProvider labels user-facing errors with the configured provider + // identity (e.g. "bedrock"). It differs from Model.Provider(), which + // reflects the fantasy transport client and is "anthropic" for Bedrock + // routed through aibridge. Metrics and prompt preparation keep using + // Model.Provider(). When empty, Model.Provider() is used. + ErrorProvider string Messages []fantasy.Message Tools []fantasy.AgentTool ActiveTools []string @@ -344,6 +351,11 @@ func GenerateAssistant(ctx context.Context, opts GenerateAssistantOptions) (Assi provider := opts.Model.Provider() modelName := opts.Model.Model() + // errorProvider labels user-facing errors with the configured provider; + // see GenerateAssistantOptions.ErrorProvider. The transport provider is + // kept for prompt preparation, Anthropic history sanitization, and the + // metric labels below. + errorProvider := cmp.Or(opts.ErrorProvider, provider) runOpts := RunOptions{ Model: opts.Model, Logger: opts.Logger, @@ -382,8 +394,8 @@ func GenerateAssistant(ctx context.Context, opts GenerateAssistantOptions) (Assi opts.Metrics, ) if streamErr != nil { - wrappedErr := wrapProviderStreamError(provider, streamErr) - classified := chaterror.Classify(wrappedErr).WithProvider(provider) + wrappedErr := wrapProviderStreamError(errorProvider, streamErr) + classified := chaterror.Classify(wrappedErr).WithProvider(errorProvider) if classified.Retryable { opts.Metrics.RecordStreamRetry(provider, modelName, classified) } @@ -396,8 +408,8 @@ func GenerateAssistant(ctx context.Context, opts GenerateAssistantOptions) (Assi if errors.Is(err, ErrInterrupted) { return AssistantOutcome{}, ErrInterrupted } - wrappedErr := wrapProviderStreamError(provider, err) - classified := chaterror.Classify(wrappedErr).WithProvider(provider) + wrappedErr := wrapProviderStreamError(errorProvider, err) + classified := chaterror.Classify(wrappedErr).WithProvider(errorProvider) if classified.Retryable { opts.Metrics.RecordStreamRetry(provider, modelName, classified) } diff --git a/coderd/x/chatd/chatloop/chatloop_run_internal_test.go b/coderd/x/chatd/chatloop/chatloop_run_internal_test.go index 4e0c6bf55a..80e77b0aa8 100644 --- a/coderd/x/chatd/chatloop/chatloop_run_internal_test.go +++ b/coderd/x/chatd/chatloop/chatloop_run_internal_test.go @@ -171,6 +171,33 @@ func TestGenerateAssistant_ProviderContextSurvivesStreamError(t *testing.T) { require.Equal(t, "OpenAI returned an unexpected error.", classified.Message) } +func TestGenerateAssistant_ErrorProviderOverridesTransportLabel(t *testing.T) { + t.Parallel() + + // Bedrock routed through aibridge uses the Anthropic transport, so + // Model.Provider() reports "anthropic". The user-facing error must use + // the configured provider from ErrorProvider instead. + model := &chattest.FakeModel{ + ProviderName: "anthropic", + ModelName: "qwen3-coder-next", + StreamFn: func(_ context.Context, _ fantasy.Call) (fantasy.StreamResponse, error) { + return nil, xerrors.New("upstream returned status 400") + }, + } + + _, err := GenerateAssistant(context.Background(), GenerateAssistantOptions{ + Model: model, + ErrorProvider: "bedrock", + Messages: []fantasy.Message{ + textMessage(fantasy.MessageRoleUser, "hello"), + }, + }) + require.Error(t, err) + classified := chaterror.Classify(err) + require.Equal(t, "bedrock", classified.Provider) + require.Equal(t, "AWS Bedrock returned an unexpected error.", classified.Message) +} + func TestGenerateAssistant_HTTP2TransportErrorClassifiedAsRetryableTimeout(t *testing.T) { t.Parallel() diff --git a/coderd/x/chatd/chatloop/metrics_test.go b/coderd/x/chatd/chatloop/metrics_test.go index e414e91fab..1f48fa4de0 100644 --- a/coderd/x/chatd/chatloop/metrics_test.go +++ b/coderd/x/chatd/chatloop/metrics_test.go @@ -441,11 +441,17 @@ func TestGenerateAssistant_StreamRetryRecordsMetric(t *testing.T) { } _, err := chatloop.GenerateAssistant(context.Background(), chatloop.GenerateAssistantOptions{ - Model: model, - Metrics: metrics, + Model: model, + // ErrorProvider diverges from the transport provider. Error copy must + // use it while the retry metric stays on the transport provider. + ErrorProvider: "bedrock", + Metrics: metrics, }) require.Error(t, err) require.Equal(t, 1, calls) + // Error classification uses the configured provider. + require.Equal(t, "bedrock", chaterror.Classify(err).Provider) + // Retry metric keeps the transport provider label, not "bedrock". requireCounter(t, reg, "coderd_chatd_stream_retries_total", 1, map[string]string{ "provider": "test-provider", "model": "test-model", diff --git a/coderd/x/chatd/generation.go b/coderd/x/chatd/generation.go index dbc276aabf..afca036c09 100644 --- a/coderd/x/chatd/generation.go +++ b/coderd/x/chatd/generation.go @@ -48,6 +48,10 @@ type generationPrepared struct { ModelRoute resolvedModelRoute ModelBuildOptions modelBuildOptions + // ResolvedProvider is the configured provider identity used to label + // user-facing errors. See chatloop.GenerateAssistantOptions.ErrorProvider. + ResolvedProvider string + ModelConfigID uuid.UUID ModelConfig codersdk.ChatModelCallConfig ProviderOptions fantasy.ProviderOptions @@ -626,6 +630,7 @@ func (s *taskStarter) generateAssistant( runCtx := input.DebugTurn.Ensure(ctx, prepared.Chat, prepared.Debug) outcome, err := chatloop.GenerateAssistant(runCtx, chatloop.GenerateAssistantOptions{ Model: prepared.Model, + ErrorProvider: prepared.ResolvedProvider, Messages: prepared.Prompt, Tools: prepared.Tools, ActiveTools: prepared.ActiveTools, diff --git a/coderd/x/chatd/generation_preparer.go b/coderd/x/chatd/generation_preparer.go index 5a8e4fd598..a0aec403ba 100644 --- a/coderd/x/chatd/generation_preparer.go +++ b/coderd/x/chatd/generation_preparer.go @@ -37,18 +37,18 @@ func (server *Server) prepareGeneration( ) var ( - model fantasy.LanguageModel - modelConfig database.ChatModelConfig - providerKeys chatprovider.ProviderAPIKeys - modelRoute resolvedModelRoute - modelOpts modelBuildOptions - callConfig codersdk.ChatModelCallConfig - promptRows []database.ChatMessage - mcpConfigs []database.MCPServerConfig - mcpTokens []database.MCPServerUserToken - debugEnabled bool - debugProvider string - debugModel string + model fantasy.LanguageModel + modelConfig database.ChatModelConfig + providerKeys chatprovider.ProviderAPIKeys + modelRoute resolvedModelRoute + modelOpts modelBuildOptions + callConfig codersdk.ChatModelCallConfig + promptRows []database.ChatMessage + mcpConfigs []database.MCPServerConfig + mcpTokens []database.MCPServerUserToken + debugEnabled bool + resolvedProvider string + debugModel string ) var g errgroup.Group @@ -86,7 +86,7 @@ func (server *Server) prepareGeneration( ctx = withActiveTurnAPIKeyID(ctx, modelOpts) var err error - model, modelConfig, providerKeys, modelRoute, debugEnabled, debugProvider, debugModel, err = server.resolveChatModel(ctx, chat, modelOpts) + model, modelConfig, providerKeys, modelRoute, debugEnabled, resolvedProvider, debugModel, err = server.resolveChatModel(ctx, chat, modelOpts) if err != nil { return generationPrepared{}, err } @@ -459,7 +459,7 @@ func (server *Server) prepareGeneration( } modelRoute = computerUseRoute providerKeys = computerUseRoute.directProviderKeys() - cuModel, cuDebugEnabled, resolvedProvider, resolvedModel, cuErr := server.resolveComputerUseModel( + cuModel, cuDebugEnabled, cuResolvedProvider, cuResolvedModel, cuErr := server.resolveComputerUseModel( ctx, chat, computerUseRoute, @@ -474,8 +474,8 @@ func (server *Server) prepareGeneration( } model = cuModel debugEnabled = cuDebugEnabled - debugProvider = resolvedProvider - debugModel = resolvedModel + resolvedProvider = cuResolvedProvider + debugModel = cuResolvedModel providerTools, err = appendComputerUseProviderTool(providerTools, computerUseProviderToolOptions{ provider: computerUseProvider, isPlanModeTurn: isPlanModeTurn, @@ -535,7 +535,7 @@ func (server *Server) prepareGeneration( debug = &generationDebug{ Enabled: true, Service: debugSvc, - Provider: debugProvider, + Provider: resolvedProvider, Model: debugModel, TriggerMessageID: triggerMessageID, HistoryTipMessageID: historyTipMessageID, @@ -587,6 +587,7 @@ func (server *Server) prepareGeneration( ProviderKeys: providerKeys, ModelRoute: modelRoute, ModelBuildOptions: modelOpts, + ResolvedProvider: resolvedProvider, ModelConfigID: modelConfig.ID, ModelConfig: callConfig, ProviderOptions: providerOptions,