fix(coderd/x/chatd): show correct provider and clean detail for Bedrock errors (#26338)

## 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.
2026-09-24 15:04:27 +08:00 · 2026-06-16 11:14:37 +01:00
parent 12d7ad6100
commit 21a2652343
7 changed files with 293 additions and 34 deletions
@@ -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()

@@ -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": <status> <status text> ` 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 {
@@ -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)
 		}
@@ -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()

@@ -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",
@@ -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,
@@ -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,