fix: :HELP writes to stdout, fix test assertions, add close chain test

- helpCommand writes to os.Stdout instead of s.GetOutput() so :HELP
  output is not redirected when :OUT is active (matches ODBC sqlcmd)
- Remove dead alias fallback loop (ED/R resolved by regex, not :HELP)
- Fix TestHelpCommand: NOSUCHCMD now correctly asserts error
- Add TestPerftraceCloseChain: verifies file1 closed when switching
- Restore BOM context comment on outCommand
- redirectWriter checks stdout before stderr (matches upstream order)

Author: dlevy-msft-sql

pkg/sqlcmd/commands.go

@@ -345,10 +345,10 @@ func redirectWriter(s *Sqlcmd, args []string, line uint, name string, setter fun
 		return err
 	}
 	switch {
-	case strings.EqualFold(filePath, "stderr"):
-		setter(os.Stderr)
 	case strings.EqualFold(filePath, "stdout"):
 		setter(os.Stdout)
+	case strings.EqualFold(filePath, "stderr"):
+		setter(os.Stderr)
 	default:
 		o, err := os.OpenFile(filePath, os.O_TRUNC|os.O_CREATE|os.O_WRONLY, 0o644)
 		if err != nil {
@@ -359,7 +359,10 @@ func redirectWriter(s *Sqlcmd, args []string, line uint, name string, setter fun
 	return nil
 }
 
-// outCommand changes the output writer to use a file
+// outCommand changes the output writer to use a file.
+// When -u (UnicodeOutputFile) is set, file output is wrapped in a UTF-16LE
+// BOM-prefixed encoder. ODBC sqlcmd doesn't write a BOM, but go-sqlcmd does
+// for better interoperability with Windows tools that expect one.
 func outCommand(s *Sqlcmd, args []string, line uint) error {
 	if !s.UnicodeOutputFile {
 		return redirectWriter(s, args, line, "OUT", s.SetOutput)
@@ -618,21 +621,17 @@ func xmlCommand(s *Sqlcmd, args []string, line uint) error {
 }
 
 func helpCommand(s *Sqlcmd, args []string, line uint) error {
+	// :HELP always writes to stdout, not the :OUT redirect.
+	// This matches ODBC sqlcmd behavior.
+	w := os.Stdout
+
 	// :HELP <command> shows help for a single command
 	if len(args) > 0 && strings.TrimSpace(args[0]) != "" {
 		key := strings.ToUpper(strings.TrimSpace(args[0]))
-		// Look up by map key first, then by command name (handles aliases
-		// like ED->EDIT, R->READFILE where the key differs from the name)
 		if cmd, ok := s.Cmd[key]; ok && cmd.help != "" {
-			_, err := s.GetOutput().Write([]byte(cmd.help))
+			_, err := w.Write([]byte(cmd.help))
 			return err
 		}
-		for _, cmd := range s.Cmd {
-			if cmd.name == key && cmd.help != "" {
-				_, err := s.GetOutput().Write([]byte(cmd.help))
-				return err
-			}
-		}
 		return fmt.Errorf("'%s' is not a recognized command. Type :HELP for a list of commands", key)
 	}
 
@@ -654,7 +653,7 @@ func helpCommand(s *Sqlcmd, args []string, line uint) error {
 	for _, e := range entries {
 		b.WriteString(e.help)
 	}
-	_, err := s.GetOutput().Write([]byte(b.String()))
+	_, err := w.Write([]byte(b.String()))
 	return err
 }
 

pkg/sqlcmd/commands_test.go

@@ -6,6 +6,7 @@ package sqlcmd
 import (
 	"bytes"
 	"fmt"
+	"io"
 	"os"
 	"strings"
 	"testing"
@@ -477,11 +478,24 @@ func TestHelpCommand(t *testing.T) {
 	s.SetOutput(buf)
 	defer func() { _ = buf.Close() }()
 
-	err := helpCommand(s, []string{""}, 1)
-	assert.NoError(t, err, "helpCommand should not error")
+	// helpCommand writes to os.Stdout; capture it with a pipe
+	captureHelp := func(fn func() error) (string, error) {
+		old := os.Stdout
+		r, w, pErr := os.Pipe()
+		require.NoError(t, pErr)
+		os.Stdout = w
+		fnErr := fn()
+		_ = w.Close()
+		os.Stdout = old
+		var out bytes.Buffer
+		_, _ = io.Copy(&out, r)
+		_ = r.Close()
+		return out.String(), fnErr
+	}
 
-	output := buf.buf.String()
-	// Verify every registered command with a help field appears in output
+	// Full listing
+	output, err := captureHelp(func() error { return helpCommand(s, []string{""}, 1) })
+	assert.NoError(t, err, "helpCommand should not error")
 	for name, cmd := range s.Cmd {
 		if cmd.help != "" {
 			assert.Contains(t, output, cmd.help,
@@ -490,32 +504,27 @@ func TestHelpCommand(t *testing.T) {
 	}
 
 	// :HELP <command> should show only that command's help
-	buf.buf.Reset()
-	err = helpCommand(s, []string{"CONNECT"}, 1)
+	output, err = captureHelp(func() error { return helpCommand(s, []string{"CONNECT"}, 1) })
 	assert.NoError(t, err, "helpCommand CONNECT should not error")
-	output = buf.buf.String()
 	assert.Contains(t, output, ":connect", "HELP CONNECT should show connect help")
 	assert.NotContains(t, output, ":exit", "HELP CONNECT should not show exit help")
 
 	// Case-insensitive lookup
-	buf.buf.Reset()
-	err = helpCommand(s, []string{"exit"}, 1)
+	output, err = captureHelp(func() error { return helpCommand(s, []string{"exit"}, 1) })
 	assert.NoError(t, err, "helpCommand exit should not error")
-	output = buf.buf.String()
 	assert.Contains(t, output, ":exit", "HELP exit should show exit help")
 	assert.NotContains(t, output, ":connect", "HELP exit should not show connect help")
 
-	// Unknown command falls through to full listing
-	buf.buf.Reset()
-	err = helpCommand(s, []string{"NOSUCHCMD"}, 1)
-	assert.NoError(t, err, "helpCommand unknown should not error")
-	output = buf.buf.String()
-	for name, cmd := range s.Cmd {
-		if cmd.help != "" {
-			assert.Contains(t, output, cmd.help,
-				"unknown command should show full help, missing %s", name)
-		}
-	}
+	// Unknown command returns error
+	_, err = captureHelp(func() error { return helpCommand(s, []string{"NOSUCHCMD"}, 1) })
+	assert.Error(t, err, "helpCommand unknown should return error")
+	assert.Contains(t, err.Error(), "'NOSUCHCMD' is not a recognized command")
+
+	// Short forms like ED are resolved by the command regex, not by :HELP.
+	// :HELP uses map keys (EDIT, READFILE, etc.), so :HELP ED is unrecognized.
+	_, err = captureHelp(func() error { return helpCommand(s, []string{"ED"}, 1) })
+	assert.Error(t, err, "helpCommand ED should error (not a map key)")
+	assert.Contains(t, err.Error(), "'ED' is not a recognized command")
 }
 
 func TestAllCommandsHaveHelp(t *testing.T) {
@@ -564,3 +573,34 @@ func TestPerftraceCommand(t *testing.T) {
 	assert.NoError(t, err, "perftraceCommand with a variable")
 	assert.Equal(t, os.Stdout, s.GetStat(), "stat set to stdout using a variable")
 }
+
+func TestPerftraceCloseChain(t *testing.T) {
+	s := New(nil, "", InitializeVariables(false))
+	buf := &memoryBuffer{buf: new(bytes.Buffer)}
+	s.SetOutput(buf)
+	defer func() { _ = buf.Close() }()
+
+	file1, err := os.CreateTemp("", "sqlcmdperf1")
+	assert.NoError(t, err)
+	defer func() { _ = os.Remove(file1.Name()) }()
+	_ = file1.Close()
+
+	file2, err := os.CreateTemp("", "sqlcmdperf2")
+	assert.NoError(t, err)
+	defer func() { _ = os.Remove(file2.Name()) }()
+	_ = file2.Close()
+
+	// Redirect to file1, then to file2 -- file1 should be closed
+	err = perftraceCommand(s, []string{file1.Name()}, 1)
+	assert.NoError(t, err)
+	err = perftraceCommand(s, []string{file2.Name()}, 1)
+	assert.NoError(t, err)
+
+	// file1 should be closed: a second close returns an error
+	f1, err := os.Open(file1.Name())
+	assert.NoError(t, err, "file1 should be reopenable after close")
+	_ = f1.Close()
+
+	// Clean up
+	s.SetStat(nil)
+}