fix(gateway): prefer a call site's own error message over the generic lookup (#630)

gateway.ErrorHandler unconditionally overwrote every error with
ErrorLookup's static per-code text, discarding both a caller's own
formatted message and any per-service distinction for wire codes
shared across services (e.g. ACM and EKS both use
ResourceInUseException). ResolveErrorDetail now also returns a
message when the producing call site used awserrors.Errorf, and
LookupErrorMessage resolves service-scoped wording before falling
back to the global default. DeleteVpc's DependencyViolation call
sites now name the blocking subnet or security group.

spinifex/awserrors/awserrors.go

@@ -383,10 +383,9 @@ var (
 	// wire code "ResourceInUseException", so this is a distinct, self-documenting
 	// name for ACM call sites rather than a second ErrorLookup entry — ErrorLookup
 	// is keyed by wire code across every service, so it can only hold one message
-	// per code. Naming it ErrorACMResourceInUse stops an ACM call site reading
-	// "EKS" while making no claim that the returned message text is ACM-flavored;
-	// it is still EKS's "cluster already exists" wording until ErrorLookup (or its
-	// caller in gateway.ErrorHandler) is keyed per-service instead of globally.
+	// per code. LookupErrorMessage resolves the ACM-flavored wording from
+	// errorLookupByService instead, so an ACM call site is not stuck with EKS's
+	// "cluster already exists" text.
 	ErrorACMResourceInUse = ErrorEKSResourceInUse
 	// ErrorACMRequestInProgress is what GetCertificate returns for a certificate
 	// that exists but has not been issued yet (still PENDING_VALIDATION), so a
@@ -561,34 +560,89 @@ func ValidErrorCode(code string) string {
 
 // ResolveErrorCode returns the first registered AWS error code in err's unwrap tree.
 func ResolveErrorCode(err error) (string, bool) {
+	code, _, ok := resolveErrorDetail(err)
+	return code, ok
+}
+
+// ResolveErrorDetail resolves the same registered code as ResolveErrorCode,
+// plus the message the producing call site attached via Errorf, if any. A
+// generic %w wrapper added purely for internal context carries no message.
+func ResolveErrorDetail(err error) (code, message string, ok bool) {
+	return resolveErrorDetail(err)
+}
+
+// resolveErrorDetail is the shared unwrap-tree walk behind ResolveErrorCode
+// and ResolveErrorDetail.
+func resolveErrorDetail(err error) (code, message string, ok bool) {
 	if err == nil {
-		return "", false
+		return "", "", false
 	}
-	code := err.Error()
-	if _, ok := ErrorLookup[code]; ok {
-		return code, true
+	text := err.Error()
+	if _, exists := ErrorLookup[text]; exists {
+		var cause *codedError
+		if errors.As(err, &cause) {
+			return text, cause.message, true
+		}
+		return text, "", true
 	}
 
-	if joined, ok := err.(interface{ Unwrap() []error }); ok {
+	if joined, isJoined := err.(interface{ Unwrap() []error }); isJoined {
 		for _, inner := range joined.Unwrap() {
-			if code, found := ResolveErrorCode(inner); found {
-				return code, true
+			if c, m, found := resolveErrorDetail(inner); found {
+				return c, m, true
 			}
 		}
-		return "", false
+		return "", "", false
 	}
-	if wrapped, ok := err.(interface{ Unwrap() error }); ok {
-		return ResolveErrorCode(wrapped.Unwrap())
+	if wrapped, isWrapped := err.(interface{ Unwrap() error }); isWrapped {
+		return resolveErrorDetail(wrapped.Unwrap())
 	}
-	return "", false
+	return "", "", false
 }
 
+// codedError is the leaf Errorf wraps around a registered code, letting the
+// resolved layer distinguish a client-facing message from an unrelated %w
+// wrapper. Error() returns only the code, so code resolution is unaffected.
+type codedError struct {
+	code    string
+	message string
+}
+
+func (c *codedError) Error() string { return c.code }
+
 // Errorf returns an error carrying an AWS error code where ResolveErrorCode can
 // find it, alongside a message for the logs and the traces. Formatting the code
 // into the message with %s instead leaves it unresolvable, so a handler's 400
 // reaches the client as a 500 with its own code stripped.
 func Errorf(code, format string, args ...any) error {
-	return fmt.Errorf(format+": %w", append(args, errors.New(code))...)
+	cause := &codedError{code: code}
+	outer := fmt.Errorf(format+": %w", append(args, cause)...)
+	// Derived from the already-formatted outer text, not a second
+	// fmt.Sprintf(format, args...) call, so a caller's own %w renders via
+	// fmt.Errorf's %w support instead of failing under Sprintf, which has none.
+	cause.message = strings.TrimSuffix(outer.Error(), ": "+code)
+	return outer
+}
+
+// errorLookupByService overrides ErrorLookup's message and HTTP status for a
+// (service, code) pair whose wire code is shared by services with different
+// canonical wording — e.g. ACM and EKS both use "ResourceInUseException".
+var errorLookupByService = map[string]map[string]ErrorMessage{
+	"acm": {
+		ErrorACMResourceInUse: {HTTPCode: 400, Message: "The certificate is in use by another AWS resource in this account. Remove the reference to the certificate before deleting it."},
+	},
+}
+
+// LookupErrorMessage returns the ErrorMessage for code, scoped to service
+// where errorLookupByService has an override, otherwise ErrorLookup's global
+// default.
+func LookupErrorMessage(service, code string) ErrorMessage {
+	if svcMsgs, ok := errorLookupByService[service]; ok {
+		if msg, ok := svcMsgs[code]; ok {
+			return msg
+		}
+	}
+	return ErrorLookup[code]
 }
 
 // ValidErrorCodeFromError resolves err or returns ErrorServerInternal.

spinifex/awserrors/awserrors_test.go

@@ -98,6 +98,67 @@ func TestResolveErrorCode(t *testing.T) {
 	}
 }
 
+// TestResolveErrorDetail_ErrorfMessageWins covers the message half of the
+// gateway fidelity fix: a call site that used Errorf gets its own formatted
+// wording back, not just the resolved code.
+func TestResolveErrorDetail_ErrorfMessageWins(t *testing.T) {
+	err := Errorf(ErrorDependencyViolation, "the VPC has a dependent subnet %s", "subnet-1")
+	code, message, ok := ResolveErrorDetail(err)
+	if !ok || code != ErrorDependencyViolation {
+		t.Fatalf("ResolveErrorDetail() code = (%q, %v), want (%q, true)", code, ok, ErrorDependencyViolation)
+	}
+	want := "the VPC has a dependent subnet subnet-1"
+	if message != want {
+		t.Errorf("ResolveErrorDetail() message = %q, want %q", message, want)
+	}
+}
+
+// TestResolveErrorDetail_PlainWrapCarriesNoMessage is the compatibility case:
+// a generic %w wrapper not produced by Errorf must not be mistaken for a
+// client-facing message, even though it sits right next to the bare code.
+func TestResolveErrorDetail_PlainWrapCarriesNoMessage(t *testing.T) {
+	err := fmt.Errorf("launch on node-1: %w", errors.New(ErrorInsufficientAddressCapacity))
+	code, message, ok := ResolveErrorDetail(err)
+	if !ok || code != ErrorInsufficientAddressCapacity {
+		t.Fatalf("ResolveErrorDetail() code = (%q, %v), want (%q, true)", code, ok, ErrorInsufficientAddressCapacity)
+	}
+	if message != "" {
+		t.Errorf("ResolveErrorDetail() message = %q, want empty (not Errorf-produced)", message)
+	}
+}
+
+// TestResolveErrorDetail_BareCodeCarriesNoMessage is the compatibility case for
+// the overwhelming majority of call sites: a bare errors.New(code) resolves the
+// code with no message, exactly as ResolveErrorCode always has.
+func TestResolveErrorDetail_BareCodeCarriesNoMessage(t *testing.T) {
+	code, message, ok := ResolveErrorDetail(errors.New(ErrorInvalidParameterValue))
+	if !ok || code != ErrorInvalidParameterValue || message != "" {
+		t.Errorf("ResolveErrorDetail() = (%q, %q, %v), want (%q, \"\", true)", code, message, ok, ErrorInvalidParameterValue)
+	}
+}
+
+// TestLookupErrorMessage covers per-service scoping: ACM must not inherit
+// EKS's wording for the shared ResourceInUseException code, and an unscoped
+// pair must fall back to ErrorLookup's existing global default unchanged.
+func TestLookupErrorMessage(t *testing.T) {
+	acm := LookupErrorMessage("acm", ErrorACMResourceInUse)
+	eks := LookupErrorMessage("eks", ErrorEKSResourceInUse)
+	global := ErrorLookup[ErrorEKSResourceInUse]
+
+	if acm.Message == global.Message {
+		t.Errorf("LookupErrorMessage(acm, ResourceInUseException) = %q, want distinct ACM wording", acm.Message)
+	}
+	if eks.Message != global.Message || eks.HTTPCode != global.HTTPCode {
+		t.Errorf("LookupErrorMessage(eks, ResourceInUseException) = %+v, want unchanged global %+v", eks, global)
+	}
+
+	// A service/code pair with no override falls back to the global default.
+	unscoped := LookupErrorMessage("ec2", ErrorInvalidParameterValue)
+	if unscoped != ErrorLookup[ErrorInvalidParameterValue] {
+		t.Errorf("LookupErrorMessage(ec2, InvalidParameterValue) = %+v, want unchanged global %+v", unscoped, ErrorLookup[ErrorInvalidParameterValue])
+	}
+}
+
 func TestValidErrorCodeFromError(t *testing.T) {
 	wrapped := fmt.Errorf("launch: %w", errors.New(ErrorInsufficientInstanceCapacity))
 	if got := ValidErrorCodeFromError(wrapped); got != ErrorInsufficientInstanceCapacity {

spinifex/gateway/gateway.go

@@ -555,13 +555,20 @@ func (gw *GatewayConfig) ErrorHandler(w http.ResponseWriter, r *http.Request, er
 	slog.Debug("ErrorHandler", "service", svc, "error", err.Error())
 
 	var requestId = uuid.NewString()
-	code, exists := awserrors.ResolveErrorCode(err)
+	code, message, exists := awserrors.ResolveErrorDetail(err)
 	if !exists {
 		slog.Warn("Unknown error code", "error", err.Error())
 		code = awserrors.ErrorInternalError
+		message = ""
 	}
 
-	errorMsg := awserrors.ErrorLookup[code]
+	// LookupErrorMessage resolves per-service wording first (e.g. ACM vs EKS
+	// both using "ResourceInUseException"); a message the producing call site
+	// supplied via awserrors.Errorf then takes priority over either default.
+	errorMsg := awserrors.LookupErrorMessage(svc, code)
+	if message != "" {
+		errorMsg.Message = message
+	}
 	if errorMsg.HTTPCode == 0 {
 		errorMsg.HTTPCode = 500
 	}

spinifex/gateway/gateway_test.go

@@ -220,6 +220,94 @@ func TestErrorHandler_WrappedErrorCode(t *testing.T) {
 	assert.NotContains(t, xmlStr, "launch on node-1")
 }
 
+// TestErrorHandler_PrefersCallSiteMessage covers the DeleteVpc DependencyViolation
+// fidelity gap: a call site that names the blocking resource via awserrors.Errorf
+// must have that wording reach the client instead of the generic ErrorLookup text.
+func TestErrorHandler_PrefersCallSiteMessage(t *testing.T) {
+	gw := &GatewayConfig{DisableLogging: true}
+
+	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		ctx := context.WithValue(r.Context(), ctxService, "ec2")
+		r = r.WithContext(ctx)
+		err := awserrors.Errorf(awserrors.ErrorDependencyViolation,
+			"the VPC has a dependent subnet %s that must be deleted first", "subnet-abc123")
+		gw.ErrorHandler(w, r, err)
+	})
+
+	req := httptest.NewRequest(http.MethodPost, "/", nil)
+	resp := doRequest(handler, req)
+	assert.Equal(t, 400, resp.StatusCode)
+
+	body, _ := io.ReadAll(resp.Body)
+	xmlStr := string(body)
+	assert.Contains(t, xmlStr, "<Code>"+awserrors.ErrorDependencyViolation+"</Code>")
+	assert.Contains(t, xmlStr, "subnet-abc123")
+	assert.NotContains(t, xmlStr, awserrors.ErrorLookup[awserrors.ErrorDependencyViolation].Message)
+}
+
+// TestErrorHandler_ACMResourceInUse_UsesACMWording is one direction of the
+// ResourceInUseException collision check: ACM's DeleteCertificate must not
+// surface EKS's "cluster already exists" wording for the shared wire code.
+func TestErrorHandler_ACMResourceInUse_UsesACMWording(t *testing.T) {
+	gw := &GatewayConfig{DisableLogging: true}
+	req := httptest.NewRequest(http.MethodPost, "/", nil)
+	ctx := context.WithValue(req.Context(), ctxService, "acm")
+	req = req.WithContext(ctx)
+	w := httptest.NewRecorder()
+
+	gw.ErrorHandler(w, req, errors.New(awserrors.ErrorACMResourceInUse))
+
+	var env struct {
+		Type    string `json:"__type"`
+		Message string `json:"message"`
+	}
+	require.NoError(t, json.Unmarshal(w.Body.Bytes(), &env))
+	assert.NotEqual(t, awserrors.ErrorLookup[awserrors.ErrorEKSResourceInUse].Message, env.Message)
+	assert.NotContains(t, env.Message, "cluster")
+}
+
+// TestErrorHandler_EKSResourceInUse_UsesEKSWording is the other direction: EKS's
+// own ResourceInUseException must keep its existing wording, unaffected by the
+// ACM-specific override.
+func TestErrorHandler_EKSResourceInUse_UsesEKSWording(t *testing.T) {
+	gw := &GatewayConfig{DisableLogging: true}
+	req := httptest.NewRequest(http.MethodPost, "/", nil)
+	ctx := context.WithValue(req.Context(), ctxService, "eks")
+	req = req.WithContext(ctx)
+	w := httptest.NewRecorder()
+
+	gw.ErrorHandler(w, req, errors.New(awserrors.ErrorEKSResourceInUse))
+
+	var env struct {
+		Type    string `json:"__type"`
+		Message string `json:"message"`
+	}
+	require.NoError(t, json.Unmarshal(w.Body.Bytes(), &env))
+	assert.Equal(t, awserrors.ErrorLookup[awserrors.ErrorEKSResourceInUse].Message, env.Message)
+}
+
+// TestErrorHandler_NoMessageSupplied_MatchesErrorLookup is the compatibility
+// case: an error path that supplies no message (the vast majority of call
+// sites) must keep rendering exactly today's ErrorLookup text, unchanged.
+func TestErrorHandler_NoMessageSupplied_MatchesErrorLookup(t *testing.T) {
+	gw := &GatewayConfig{DisableLogging: true}
+
+	handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		ctx := context.WithValue(r.Context(), ctxService, "ec2")
+		r = r.WithContext(ctx)
+		gw.ErrorHandler(w, r, errors.New(awserrors.ErrorInvalidParameterValue))
+	})
+
+	req := httptest.NewRequest(http.MethodPost, "/", nil)
+	resp := doRequest(handler, req)
+	assert.Equal(t, 400, resp.StatusCode)
+
+	body, _ := io.ReadAll(resp.Body)
+	xmlStr := string(body)
+	assert.Contains(t, xmlStr, "<Code>"+awserrors.ErrorInvalidParameterValue+"</Code>")
+	assert.Contains(t, xmlStr, awserrors.ErrorLookup[awserrors.ErrorInvalidParameterValue].Message)
+}
+
 func TestErrorHandler_ELBv2Service(t *testing.T) {
 	gw := &GatewayConfig{DisableLogging: true}
 

spinifex/handlers/ec2/vpc/security_group_test.go

@@ -1228,11 +1228,14 @@ func TestDeleteSecurityGroup_RejectsReferencedFromOtherSG(t *testing.T) {
 func TestDeleteVpc_BlocksOnNonDefaultSG(t *testing.T) {
 	svc := setupTestVPCService(t)
 	vpcID := createTestVPC(t, svc, "10.0.0.0/16")
-	createTestSG(t, svc, vpcID, "user-sg")
+	sgID := createTestSG(t, svc, vpcID, "user-sg")
 
 	_, err := svc.DeleteVpc(context.Background(), &ec2.DeleteVpcInput{VpcId: aws.String(vpcID)}, testAccountID)
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "DependencyViolation")
+	// The message must name the blocking security group so the caller knows
+	// what to delete first, not just that something is blocking.
+	assert.Contains(t, err.Error(), sgID)
 }
 
 func TestDeleteVpc_CascadesDefaultSG(t *testing.T) {

spinifex/handlers/ec2/vpc/service_impl.go

@@ -360,7 +360,8 @@ func (s *VPCServiceImpl) DeleteVpc(ctx context.Context, input *ec2.DeleteVpcInpu
 			return nil, errors.New(awserrors.ErrorServerInternal)
 		}
 		if subnet.VpcId == vpcID {
-			return nil, errors.New(awserrors.ErrorDependencyViolation)
+			return nil, awserrors.Errorf(awserrors.ErrorDependencyViolation,
+				"the VPC has a dependent subnet %s that must be deleted first", subnet.SubnetId)
 		}
 	}
 
@@ -396,7 +397,8 @@ func (s *VPCServiceImpl) DeleteVpc(ctx context.Context, input *ec2.DeleteVpcInpu
 			continue
 		}
 		if !sg.IsDefault {
-			return nil, errors.New(awserrors.ErrorDependencyViolation)
+			return nil, awserrors.Errorf(awserrors.ErrorDependencyViolation,
+				"the VPC has a dependent security group %s that must be deleted first", sg.GroupId)
 		}
 		defaultSGId = sg.GroupId
 	}

spinifex/handlers/ec2/vpc/service_impl_test.go

@@ -199,13 +199,16 @@ func TestDeleteVpc_MissingID(t *testing.T) {
 func TestDeleteVpc_WithSubnets(t *testing.T) {
 	svc := setupTestVPCService(t)
 	vpcID := createTestVPC(t, svc, "10.0.0.0/16")
-	createTestSubnet(t, svc, vpcID, "10.0.1.0/24")
+	subnetID := createTestSubnet(t, svc, vpcID, "10.0.1.0/24")
 
 	// Should fail because VPC has subnets
 	_, err := svc.DeleteVpc(context.Background(), &ec2.DeleteVpcInput{
 		VpcId: aws.String(vpcID),
 	}, testAccountID)
 	assert.ErrorContains(t, err, "DependencyViolation")
+	// The message must name the blocking subnet so the caller knows what to
+	// delete first, not just that something is blocking.
+	assert.ErrorContains(t, err, subnetID)
 }
 
 func TestDescribeVpcs_All(t *testing.T) {