Address Copilot review comments for codepage support

- Use localizer.Errorf for all user-facing error messages
- Fix UTF-16 BOM handling using ExpectBOM for input decoding
- Add transformWriteCloser to properly close underlying file handles
- Use transformWriteCloser in outCommand and errorCommand for both
  UnicodeOutputFile and CodePage transforms to prevent file handle leaks
- Add integration tests for output/error codepage encoding

Author: dlevy-msft-sql

pkg/sqlcmd/codepage.go

@@ -4,10 +4,10 @@
 package sqlcmd
 
 import (
-	"fmt"
 	"strconv"
 	"strings"
 
+	"github.com/microsoft/go-sqlcmd/internal/localizer"
 	"golang.org/x/text/encoding"
 	"golang.org/x/text/encoding/charmap"
 	"golang.org/x/text/encoding/japanese"
@@ -43,21 +43,21 @@ func ParseCodePage(arg string) (*CodePageSettings, error) {
 			// Input codepage
 			cp, err := strconv.Atoi(strings.TrimPrefix(strings.ToLower(part), "i:"))
 			if err != nil {
-				return nil, fmt.Errorf("invalid input codepage: %s", part)
+				return nil, localizer.Errorf("invalid input codepage: %s", part)
 			}
 			settings.InputCodePage = cp
 		} else if strings.HasPrefix(strings.ToLower(part), "o:") {
 			// Output codepage
 			cp, err := strconv.Atoi(strings.TrimPrefix(strings.ToLower(part), "o:"))
 			if err != nil {
-				return nil, fmt.Errorf("invalid output codepage: %s", part)
+				return nil, localizer.Errorf("invalid output codepage: %s", part)
 			}
 			settings.OutputCodePage = cp
 		} else {
 			// Both input and output
 			cp, err := strconv.Atoi(part)
 			if err != nil {
-				return nil, fmt.Errorf("invalid codepage: %s", part)
+				return nil, localizer.Errorf("invalid codepage: %s", part)
 			}
 			settings.InputCodePage = cp
 			settings.OutputCodePage = cp
@@ -88,11 +88,11 @@ func GetEncoding(codepage int) (encoding.Encoding, error) {
 		// UTF-8 - Go's native encoding, return nil to indicate no transformation needed
 		return nil, nil
 	case 1200:
-		// UTF-16LE
-		return unicode.UTF16(unicode.LittleEndian, unicode.IgnoreBOM), nil
+		// UTF-16LE - Use ExpectBOM to strip BOM if present during input
+		return unicode.UTF16(unicode.LittleEndian, unicode.ExpectBOM), nil
 	case 1201:
-		// UTF-16BE
-		return unicode.UTF16(unicode.BigEndian, unicode.IgnoreBOM), nil
+		// UTF-16BE - Use ExpectBOM to strip BOM if present during input
+		return unicode.UTF16(unicode.BigEndian, unicode.ExpectBOM), nil
 
 	// OEM/DOS codepages
 	case 437:
@@ -224,7 +224,7 @@ func GetEncoding(codepage int) (encoding.Encoding, error) {
 		return traditionalchinese.Big5, nil
 
 	default:
-		return nil, fmt.Errorf("unsupported codepage %d", codepage)
+		return nil, localizer.Errorf("unsupported codepage %d", codepage)
 	}
 }
 

pkg/sqlcmd/commands.go

@@ -6,17 +6,36 @@ package sqlcmd
 import (
 	"flag"
 	"fmt"
+	"io"
 	"os"
 	"regexp"
 	"sort"
 	"strconv"
 	"strings"
 
 	"github.com/microsoft/go-sqlcmd/internal/color"
+	"github.com/microsoft/go-sqlcmd/internal/localizer"
 	"golang.org/x/text/encoding/unicode"
 	"golang.org/x/text/transform"
 )
 
+// transformWriteCloser wraps a transform.Writer and ensures the underlying
+// file is closed when Close() is called.
+type transformWriteCloser struct {
+	*transform.Writer
+	underlying io.Closer
+}
+
+// Close flushes the transform writer and closes the underlying file.
+func (t *transformWriteCloser) Close() error {
+	// Close the transform writer (flushes pending data)
+	if err := t.Writer.Close(); err != nil {
+		_ = t.underlying.Close()
+		return err
+	}
+	return t.underlying.Close()
+}
+
 // Command defines a sqlcmd action which can be intermixed with the SQL batch
 // Commands for sqlcmd are defined at https://docs.microsoft.com/sql/tools/sqlcmd-utility#sqlcmd-commands
 type Command struct {
@@ -324,7 +343,10 @@ func outCommand(s *Sqlcmd, args []string, line uint) error {
 			// ODBC sqlcmd doesn't write a BOM but we will.
 			// Maybe the endian-ness should be configurable.
 			win16le := unicode.UTF16(unicode.LittleEndian, unicode.UseBOM)
-			encoder := transform.NewWriter(o, win16le.NewEncoder())
+			encoder := &transformWriteCloser{
+				Writer:     transform.NewWriter(o, win16le.NewEncoder()),
+				underlying: o,
+			}
 			s.SetOutput(encoder)
 		} else if s.CodePage != nil && s.CodePage.OutputCodePage != 0 {
 			// Use specified output codepage
@@ -335,7 +357,10 @@ func outCommand(s *Sqlcmd, args []string, line uint) error {
 			}
 			if enc != nil {
 				// Transform from UTF-8 to specified encoding
-				encoder := transform.NewWriter(o, enc.NewEncoder())
+				encoder := &transformWriteCloser{
+					Writer:     transform.NewWriter(o, enc.NewEncoder()),
+					underlying: o,
+				}
 				s.SetOutput(encoder)
 			} else {
 				// UTF-8, no transformation needed
@@ -372,15 +397,18 @@ func errorCommand(s *Sqlcmd, args []string, line uint) error {
 			enc, err := GetEncoding(s.CodePage.OutputCodePage)
 			if err != nil {
 				if cerr := o.Close(); cerr != nil {
-					return fmt.Errorf("%v; additionally, closing error file %q failed: %w", err, args[0], cerr)
+					return localizer.Errorf("%v; additionally, closing error file %q failed: %v", err, args[0], cerr)
 				}
 				return err
 			}
 			if enc == nil {
 				// UTF-8 (or default) encoding: write directly without transform
 				s.SetError(o)
 			} else {
-				encoder := transform.NewWriter(o, enc.NewEncoder())
+				encoder := &transformWriteCloser{
+					Writer:     transform.NewWriter(o, enc.NewEncoder()),
+					underlying: o,
+				}
 				s.SetError(encoder)
 			}
 		} else {

pkg/sqlcmd/commands_test.go

@@ -458,3 +458,100 @@ func TestExitCommandAppendsParameterToCurrentBatch(t *testing.T) {
 	}
 
 }
+
+func TestOutputCodePageCommand(t *testing.T) {
+	tests := []struct {
+		name           string
+		codepage       int
+		expectedBytes  []byte
+		inputText      string
+		skipOnEncError bool
+	}{
+		{
+			name:          "UTF-8 output",
+			codepage:      65001,
+			inputText:     "café",
+			expectedBytes: []byte("café"),
+		},
+		{
+			name:          "Windows-1252 output",
+			codepage:      1252,
+			inputText:     "café",
+			expectedBytes: []byte{0x63, 0x61, 0x66, 0xe9}, // "café" in Windows-1252
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			s, buf := setupSqlCmdWithMemoryOutput(t)
+			defer buf.Close()
+
+			// Set up codepage
+			s.CodePage = &CodePageSettings{
+				OutputCodePage: tt.codepage,
+			}
+
+			// Create temp file for output
+			file, err := os.CreateTemp("", "sqlcmdout")
+			require.NoError(t, err, "os.CreateTemp")
+			defer os.Remove(file.Name())
+			fileName := file.Name()
+			_ = file.Close()
+
+			// Run the OUT command
+			err = outCommand(s, []string{fileName}, 1)
+			require.NoError(t, err, "outCommand")
+
+			// Write some text
+			_, err = s.GetOutput().Write([]byte(tt.inputText))
+			require.NoError(t, err, "Write")
+
+			// Close to flush
+			if closer, ok := s.GetOutput().(interface{ Close() error }); ok {
+				require.NoError(t, closer.Close(), "Close output")
+			}
+
+			// Read the file and check encoding
+			content, err := os.ReadFile(fileName)
+			require.NoError(t, err, "ReadFile")
+			assert.Equal(t, tt.expectedBytes, content, "Output encoding mismatch")
+		})
+	}
+}
+
+func TestErrorCodePageCommand(t *testing.T) {
+	s, buf := setupSqlCmdWithMemoryOutput(t)
+	defer buf.Close()
+
+	// Set up codepage for Windows-1252
+	s.CodePage = &CodePageSettings{
+		OutputCodePage: 1252,
+	}
+
+	// Create temp file for error output
+	file, err := os.CreateTemp("", "sqlcmderr")
+	require.NoError(t, err, "os.CreateTemp")
+	defer os.Remove(file.Name())
+	fileName := file.Name()
+	_ = file.Close()
+
+	// Run the ERROR command
+	err = errorCommand(s, []string{fileName}, 1)
+	require.NoError(t, err, "errorCommand")
+
+	// Write some text with special characters
+	_, err = s.err.Write([]byte("Error: café"))
+	require.NoError(t, err, "Write")
+
+	// Close to flush
+	if closer, ok := s.err.(interface{ Close() error }); ok {
+		require.NoError(t, closer.Close(), "Close error")
+	}
+
+	// Read the file and check encoding
+	content, err := os.ReadFile(fileName)
+	require.NoError(t, err, "ReadFile")
+	// "Error: café" in Windows-1252
+	expected := []byte{0x45, 0x72, 0x72, 0x6f, 0x72, 0x3a, 0x20, 0x63, 0x61, 0x66, 0xe9}
+	assert.Equal(t, expected, content, "Error output encoding mismatch")
+}