Address Copilot review comments on serverlist command

- Fix fmt.Fprintf spacing in ListLocalServers
- Sort instance names for deterministic output
- Add validation for missing ServerName in instances
- Add argument validation for helpCommand
- Add test cases for :SERVERLIST and :serverlist
- Add assertion for :serverlist in TestHelpCommand

Author: dlevy-msft-sql

pkg/sqlcmd/commands.go

@@ -608,6 +608,9 @@ func xmlCommand(s *Sqlcmd, args []string, line uint) error {
 
 // helpCommand displays the list of available sqlcmd commands
 func helpCommand(s *Sqlcmd, args []string, line uint) error {
+	if len(args) > 0 && strings.TrimSpace(args[0]) != "" {
+		return InvalidCommandError("HELP", line)
+	}
 	helpText := `:!! [<command>]
   - Executes a command in the operating system shell.
 :connect server[\instance] [-l timeout] [-U user [-P password]]

pkg/sqlcmd/commands_test.go

@@ -55,8 +55,8 @@ func TestCommandParsing(t *testing.T) {
 		{`:RESET`, "RESET", []string{""}},
 		{`RESET`, "RESET", []string{""}},
 		{`:HELP`, "HELP", []string{""}},
-		{`:help`, "HELP", []string{""}},
-	}
+		{`:help`, "HELP", []string{""}}, {`:SERVERLIST`, "SERVERLIST", []string{""}},
+		{`:serverlist`, "SERVERLIST", []string{""}}}
 
 	for _, test := range commands {
 		cmd, args := c.matchCommand(test.line)
@@ -479,5 +479,6 @@ func TestHelpCommand(t *testing.T) {
 	assert.Contains(t, output, ":out", "help should list :out")
 	assert.Contains(t, output, ":error", "help should list :error")
 	assert.Contains(t, output, ":r", "help should list :r")
+	assert.Contains(t, output, ":serverlist", "help should list :serverlist")
 	assert.Contains(t, output, "go", "help should list go")
 }

pkg/sqlcmd/serverlist.go

@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"io"
 	"net"
+	"sort"
 	"strings"
 	"time"
 
@@ -19,7 +20,7 @@ import (
 func ListLocalServers(w io.Writer) {
 	instances := GetLocalServerInstances()
 	for _, s := range instances {
-		fmt.Fprintln(w, "  ", s)
+		fmt.Fprintf(w, "  %s\n", s)
 	}
 }
 
@@ -52,11 +53,24 @@ func GetLocalServerInstances() []string {
 
 	data := parseInstances(resp[:read])
 	instances := make([]string, 0, len(data))
+
+	// Sort instance names for deterministic output
+	instanceNames := make([]string, 0, len(data))
 	for s := range data {
+		instanceNames = append(instanceNames, s)
+	}
+	sort.Strings(instanceNames)
+
+	for _, s := range instanceNames {
+		serverName := data[s]["ServerName"]
+		if serverName == "" {
+			// Skip instances without a ServerName
+			continue
+		}
 		if s == "MSSQLSERVER" {
-			instances = append(instances, "(local)", data[s]["ServerName"])
+			instances = append(instances, "(local)", serverName)
 		} else {
-			instances = append(instances, fmt.Sprintf(`%s\%s`, data[s]["ServerName"], s))
+			instances = append(instances, fmt.Sprintf(`%s\%s`, serverName, s))
 		}
 	}
 	return instances