feat: implement :serverlist command

[David Levy (dlevy-msft-sql)]

Problem

Go sqlcmd lacks the interactive :serverlist command available in ODBC sqlcmd, and interactive users do not have a :help command that lists supported commands.

Root Cause

Server discovery was implemented only in the legacy -L path, so the interactive command framework could not reuse it. The interactive command list also had no help handler.

Solution

Move SQL Browser discovery into pkg/sqlcmd, use it from both -L and :serverlist, and add :help with the supported interactive command syntax. Discovery is best effort, uses a five-second timeout, treats unavailable Browser responses as no results, and routes other failures through the configured error stream.

Changes

File	Change
pkg/sqlcmd/serverlist.go	Shared SQL Browser discovery, parsing, sorting, error handling, and platform-specific output line endings
pkg/sqlcmd/serverlist_test.go	Deterministic tests for output, malformed responses, unavailable Browser errors, and command error routing
pkg/sqlcmd/commands.go	Register and implement :serverlist and :help
pkg/sqlcmd/commands_test.go	Cover command recognition and help output
cmd/sqlcmd/sqlcmd.go	Reuse the shared implementation for -L
README.md	Document :serverlist, output, and batch usage

Usage

# From the command line
sqlcmd -L

# In interactive mode
1> :serverlist
1> :help

Testing

• go test ./pkg/sqlcmd -run '^TestHelpCommand$'
• go test ./cmd/sqlcmd -run '^$'
• Required GitHub checks pass on the current head

Related Issues

N/A

[Copilot]

Pull request overview

This PR implements the :serverlist interactive command to discover SQL Server instances via the SQL Browser service, bringing go-sqlcmd to feature parity with ODBC sqlcmd.

Changes:

• Added :serverlist command that queries the SQL Browser service on UDP port 1434
• Refactored server listing logic from cmd/sqlcmd/sqlcmd.go to pkg/sqlcmd/serverlist.go for code reuse
• Both the -L command-line flag and :serverlist command now share the ListLocalServers() function

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File	Description
pkg/sqlcmd/serverlist.go	New file containing refactored server listing functions (ListLocalServers, GetLocalServerInstances, parseInstances)
pkg/sqlcmd/serverlist_test.go	New test file with comprehensive tests for server listing and parsing functionality
pkg/sqlcmd/commands.go	Added SERVERLIST command registration and serverlistCommand handler function
cmd/sqlcmd/sqlcmd.go	Removed listLocalServers and parseInstances functions (moved to pkg/sqlcmd), updated -L flag to use sqlcmd.ListLocalServers()

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

[Copilot]

🔵 Needs a closer look

Browser-unavailable detection likely misses Windows’ common UDP ECONNRESET case (and related tests/error-output consistency should be adjusted), which can cause unintended stderr output instead of an empty result.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

pkg/sqlcmd/serverlist.go:78

• isBrowserUnavailableError only treats ECONNREFUSED as “browser unavailable”. On Windows, UDP reads to a closed port commonly surface as ECONNRESET (WSAECONNRESET), which would currently be returned as an error instead of being treated as “no instances” as intended.
  pkg/sqlcmd/commands.go:666
• serverlistCommand prints errors via fmt.Fprintln, which uses \n line endings and bypasses the existing Sqlcmd.WriteError helper that consistently appends SqlcmdEol and handles routing. Using WriteError keeps error output consistent with the rest of the interactive command framework.
  pkg/sqlcmd/serverlist_test.go:95
• If isBrowserUnavailableError is extended to treat ECONNRESET as “browser unavailable” (common on Windows for UDP to a closed port), the tests should assert that behavior to prevent regressions.

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

:serverlist discovery errors can be written to stdout by default, conflicting with the documented stderr/error-stream behavior and breaking 2>-based scripting expectations.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The SQL Browser response parser currently drops the last instance if the response doesn’t end with the expected terminator, and the added tests don’t cover that edge case.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

pkg/sqlcmd/serverlist.go:134

• parseInstances only emits an instance when it sees an empty token delimiter (the expected "two semicolons" terminator). If the SQL Browser response is truncated or doesn't end with the terminator, the last fully-parsed instance will be silently dropped.
  pkg/sqlcmd/serverlist_test.go:79
• TestParseInstances only covers responses where each instance ends with the expected ";;" terminator. Add a test case for a response that omits the final terminator to ensure the parser is resilient to truncated/malformed SQL Browser responses.

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The -L output path now mixes \n (fmt.Println/Fprintln) with SqlcmdEol (\r\n on Windows), creating inconsistent line endings when redirecting output.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

cmd/sqlcmd/sqlcmd.go:242

• On Windows, ListLocalServers uses SqlcmdEol ("\r\n"), but the -L path prints the blank line and "Servers:" header via fmt.Println/Fprintln ("\n"). This can produce mixed line endings when output is redirected, which is user-visible and inconsistent with the rest of the server list output. Prefer writing these lines with sqlcmd.SqlcmdEol as well.

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The -L error printing path uses a %s format verb with an error, which will produce incorrect output (%!s(...)) instead of the actual error message.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 6/6 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The new :help output contains inaccurate/misleading command syntax details (notably for :connect and :setvar) that should be corrected before release.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

pkg/sqlcmd/commands.go:616

• The :connect usage string in :help is missing supported flags (-D and -G), so the help output is inaccurate relative to connectCommand (which defines -D and -G). Update the help text so users can discover these options.

This issue also appears on line 648 of the same file.

pkg/sqlcmd/commands.go:648

• In the :help output, :setvar {variable} can be read as requiring literal braces, but setVarCommand expects :setvar <variable> (no braces). Use the same placeholder style as the next line to avoid confusing users.

:setvar {variable}

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

:serverlist error routing currently makes :error stdout ineffective due to WriteError’s SqlcmdError special-casing, causing inconsistent/incorrect error redirection behavior.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

pkg/sqlcmd/commands.go:666

• serverlistCommand wraps discovery failures in CommonSqlcmdErr, but WriteError forces all SqlcmdError output to stderr when the configured error stream is os.Stdout. This makes :error stdout ineffective for :serverlist, contradicting the :error help text and producing inconsistent redirection behavior.

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The PR introduces a new :help command in addition to :serverlist, so the PR description/scope should be updated (or the change split) to avoid an undocumented feature addition.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

pkg/sqlcmd/commands.go:656

• The PR description focuses on implementing :serverlist, but this change also introduces a new :help interactive command (command registration plus implementation and tests). Please either update the PR description to include :help as part of the scope, or split :help into a separate PR to keep the change focused and reviewable.

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite