Address code review feedback

- Fix stderr capture race condition by using synchronized list
- Remove unused StderrCapture class
- Improve ConnectionLostException error message with troubleshooting link
- Make process exit detection more reliable (50ms instead of 100ms)
- Fix platform-specific test to work on both Windows and Unix
- Remove placeholder version numbers from documentation
- Simplify connection error test assertions

Co-authored-by: patniko <26906478+patniko@users.noreply.github.com>

Author: Copilot

docs/troubleshooting-connection-errors.md

@@ -70,7 +70,7 @@ client = CopilotClient({
 **Symptom**: Error mentioning "protocol version mismatch"
 
 **Solution**: Update either the SDK or CLI to compatible versions
-- SDK version 0.1.x requires CLI version X.Y.Z or newer
+- Check the [release notes](https://github.com/github/copilot-sdk/releases) for compatibility information
 - Update CLI: Follow the installation guide to get the latest version
 - Update SDK: Install the latest SDK package
 
@@ -179,7 +179,7 @@ copilot --server --port 4321
 
 ### Examine Error Output
 
-Recent SDK versions (0.1.20+) include stderr output from the CLI in error messages. Look for:
+The latest versions of the SDK include stderr output from the CLI in error messages when processes fail. Look for:
 - Authentication errors
 - Missing file or permission errors
 - Node.js errors (if CLI is JS-based)

dotnet/src/Client.cs

@@ -598,7 +598,11 @@ internal static async Task<T> InvokeRpcAsync<T>(JsonRpc rpc, string method, obje
         }
         catch (StreamJsonRpc.ConnectionLostException ex)
         {
-            throw new IOException($"Communication error with Copilot CLI: The JSON-RPC connection with the remote party was lost before the request could complete. {ex.Message}", ex);
+            throw new IOException(
+                $"Communication error with Copilot CLI: The connection was lost. " +
+                $"This usually means the CLI process crashed or exited unexpectedly. " +
+                $"See the troubleshooting guide at https://github.com/github/copilot-sdk/blob/main/docs/troubleshooting-connection-errors.md for help. " +
+                $"Details: {ex.Message}", ex);
         }
         catch (StreamJsonRpc.RemoteRpcException ex)
         {
@@ -700,26 +704,10 @@ private async Task VerifyProtocolVersionAsync(Connection connection, Cancellatio
                 $"Error: {ex.Message}", ex);
         }
 
-        // Check if process exited immediately (indicates a startup failure)
-        await Task.Delay(100, cancellationToken); // Small delay to detect immediate crashes
-        if (cliProcess.HasExited)
-        {
-            var exitCode = cliProcess.ExitCode;
-            var errorOutput = string.Empty;
-            try
-            {
-                errorOutput = await cliProcess.StandardError.ReadToEndAsync(cancellationToken);
-            }
-            catch { /* ignore errors reading stderr */ }
-
-            throw new InvalidOperationException(
-                $"Copilot CLI process exited immediately with code {exitCode}. " +
-                $"This may indicate the CLI is not properly installed or configured. " +
-                (string.IsNullOrEmpty(errorOutput) ? "" : $"Error output:{Environment.NewLine}{errorOutput}"));
-        }
-
-        // Forward stderr to logger (don't await - let it run in background)
-        _ = Task.Run(async () =>
+        // Start capturing stderr immediately to avoid race conditions
+        var stderrLines = new List<string>();
+        var stderrLock = new object();
+        var stderrTask = Task.Run(async () =>
         {
             try
             {
@@ -728,6 +716,15 @@ private async Task VerifyProtocolVersionAsync(Connection connection, Cancellatio
                     var line = await cliProcess.StandardError.ReadLineAsync(cancellationToken);
                     if (line != null)
                     {
+                        lock (stderrLock)
+                        {
+                            stderrLines.Add(line);
+                            // Keep only last 50 lines to avoid unbounded growth
+                            if (stderrLines.Count > 50)
+                            {
+                                stderrLines.RemoveAt(0);
+                            }
+                        }
                         logger.LogDebug("[CLI] {Line}", line);
                     }
                 }
@@ -738,6 +735,28 @@ private async Task VerifyProtocolVersionAsync(Connection connection, Cancellatio
             }
         }, cancellationToken);
 
+        // Check if process exited immediately (indicates a startup failure)
+        // Use a shorter delay and check HasExited to minimize false positives
+        await Task.Delay(50, cancellationToken);
+        if (cliProcess.HasExited)
+        {
+            var exitCode = cliProcess.ExitCode;
+            
+            // Give a moment for stderr task to capture any error output
+            await Task.Delay(50, cancellationToken);
+            
+            string errorOutput;
+            lock (stderrLock)
+            {
+                errorOutput = stderrLines.Count > 0 ? string.Join(Environment.NewLine, stderrLines) : string.Empty;
+            }
+
+            throw new InvalidOperationException(
+                $"Copilot CLI process exited immediately with code {exitCode}. " +
+                $"This may indicate the CLI is not properly installed or configured. " +
+                (string.IsNullOrEmpty(errorOutput) ? "Run with debug logging enabled for more details." : $"Error output:{Environment.NewLine}{errorOutput}"));
+        }
+
         var detectedLocalhostTcpPort = (int?)null;
         if (!options.UseStdio)
         {
@@ -753,21 +772,21 @@ private async Task VerifyProtocolVersionAsync(Connection connection, Cancellatio
                     if (line == null)
                     {
                         var exitCode = cliProcess.HasExited ? cliProcess.ExitCode : -1;
-                        var errorOutput = string.Empty;
-                        try
+                        
+                        // Give a moment for stderr to be captured
+                        await Task.Delay(50, CancellationToken.None);
+                        
+                        string errorOutput;
+                        lock (stderrLock)
                         {
-                            if (cliProcess.HasExited)
-                            {
-                                errorOutput = await cliProcess.StandardError.ReadToEndAsync(CancellationToken.None);
-                            }
+                            errorOutput = stderrLines.Count > 0 ? string.Join(Environment.NewLine, stderrLines) : string.Empty;
                         }
-                        catch { /* ignore */ }
 
                         throw new InvalidOperationException(
                             $"CLI process exited unexpectedly" +
                             (cliProcess.HasExited ? $" with code {exitCode}" : "") +
                             ". " +
-                            (string.IsNullOrEmpty(errorOutput) ? "" : $"Error output:{Environment.NewLine}{errorOutput}"));
+                            (string.IsNullOrEmpty(errorOutput) ? "Check CLI logs for details." : $"Error output:{Environment.NewLine}{errorOutput}"));
                     }
 
                     var match = Regex.Match(line, @"listening on port (\d+)", RegexOptions.IgnoreCase);
@@ -813,7 +832,6 @@ private async Task<Connection> ConnectToServerAsync(Process? cliProcess, string?
         Stream inputStream, outputStream;
         TcpClient? tcpClient = null;
         NetworkStream? networkStream = null;
-        StderrCapture? stderrCapture = null;
 
         if (_options.UseStdio)
         {
@@ -822,17 +840,8 @@ private async Task<Connection> ConnectToServerAsync(Process? cliProcess, string?
             // Check if process has already exited
             if (cliProcess.HasExited)
             {
-                var exitCode = cliProcess.ExitCode;
-                var errorOutput = string.Empty;
-                try
-                {
-                    errorOutput = await cliProcess.StandardError.ReadToEndAsync(cancellationToken);
-                }
-                catch { /* ignore */ }
-
                 throw new InvalidOperationException(
-                    $"Cannot connect to CLI process - it has already exited with code {exitCode}. " +
-                    (string.IsNullOrEmpty(errorOutput) ? "" : $"Error output:{Environment.NewLine}{errorOutput}"));
+                    $"Cannot connect to CLI process - it has already exited with code {cliProcess.ExitCode}.");
             }
 
             inputStream = cliProcess.StandardOutput.BaseStream;
@@ -865,7 +874,7 @@ private async Task<Connection> ConnectToServerAsync(Process? cliProcess, string?
         rpc.AddLocalRpcMethod("tool.call", handler.OnToolCall);
         rpc.AddLocalRpcMethod("permission.request", handler.OnPermissionRequest);
         rpc.StartListening();
-        return new Connection(rpc, cliProcess, tcpClient, networkStream, stderrCapture);
+        return new Connection(rpc, cliProcess, tcpClient, networkStream);
     }
 
     [UnconditionalSuppressMessage("Trimming", "IL2026", Justification = "Using happy path from https://microsoft.github.io/vs-streamjsonrpc/docs/nativeAOT.html")]
@@ -1056,54 +1065,12 @@ private class Connection(
         JsonRpc rpc,
         Process? cliProcess, // Set if we created the child process
         TcpClient? tcpClient, // Set if using TCP
-        NetworkStream? networkStream, // Set if using TCP
-        StderrCapture? stderrCapture = null) // Set if we capture stderr
+        NetworkStream? networkStream) // Set if using TCP
     {
         public Process? CliProcess => cliProcess;
         public TcpClient? TcpClient => tcpClient;
         public JsonRpc Rpc => rpc;
         public NetworkStream? NetworkStream => networkStream;
-        public StderrCapture? StderrCapture => stderrCapture;
-    }
-
-    /// <summary>
-    /// Helper class to capture stderr output from the CLI process for better error diagnostics.
-    /// </summary>
-    private class StderrCapture
-    {
-        private readonly List<string> _lines = new();
-        private readonly object _lock = new();
-        private const int MaxLines = 100; // Keep last 100 lines
-
-        public void AddLine(string line)
-        {
-            lock (_lock)
-            {
-                _lines.Add(line);
-                if (_lines.Count > MaxLines)
-                {
-                    _lines.RemoveAt(0);
-                }
-            }
-        }
-
-        public string GetOutput()
-        {
-            lock (_lock)
-            {
-                return _lines.Count > 0 ? string.Join(Environment.NewLine, _lines) : string.Empty;
-            }
-        }
-
-        public string GetLastLines(int count)
-        {
-            lock (_lock)
-            {
-                var linesToTake = Math.Min(count, _lines.Count);
-                var startIndex = _lines.Count - linesToTake;
-                return linesToTake > 0 ? string.Join(Environment.NewLine, _lines.Skip(startIndex)) : string.Empty;
-            }
-        }
     }
 
     private static class ProcessArgumentEscaper

dotnet/test/ErrorHandlingTests.cs

@@ -35,12 +35,33 @@ public async Task Should_Provide_Helpful_Error_When_CLI_Not_Found()
     public async Task Should_Detect_Immediate_Process_Exit()
     {
         // Arrange: Use an executable that exits immediately
-        // Using 'false' command which always exits with code 1
-        var client = new CopilotClient(new CopilotClientOptions
+        // Platform-specific command that exits with non-zero code
+        string exitCommand;
+        if (OperatingSystem.IsWindows())
+        {
+            exitCommand = "cmd";
+        }
+        else
+        {
+            exitCommand = "false"; // Unix command that exits immediately with code 1
+        }
+
+        var clientOptions = new CopilotClientOptions
         {
-            CliPath = "false", // Unix command that exits immediately with code 1
             AutoStart = true
-        });
+        };
+
+        if (OperatingSystem.IsWindows())
+        {
+            clientOptions.CliPath = exitCommand;
+            clientOptions.CliArgs = ["/c", "exit", "1"]; // Exit with code 1
+        }
+        else
+        {
+            clientOptions.CliPath = exitCommand;
+        }
+
+        var client = new CopilotClient(clientOptions);
 
         // Act & Assert: Should detect the immediate exit
         var exception = await Assert.ThrowsAsync<InvalidOperationException>(
@@ -53,16 +74,15 @@ public async Task Should_Detect_Immediate_Process_Exit()
     [Fact]
     public async Task Should_Provide_Clear_Error_For_Connection_Issues()
     {
-        // This test verifies that connection lost exceptions are wrapped with helpful messages
-        // We can't easily simulate a real connection loss in a unit test,
-        // but we verify that the error handling code is in place
+        // Verify that ConnectionLostException is handled and wrapped properly
+        // by checking that the InvokeRpcAsync method has proper exception handling
 
-        // Just verify the method exists and has the right signature
+        // This is a minimal test that verifies the structure exists
+        // More comprehensive testing would require integration tests with a real CLI
         var method = typeof(CopilotClient).GetMethod(
             "InvokeRpcAsync",
             System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static);
 
         Assert.NotNull(method);
-        Assert.Equal("Task`1", method!.ReturnType.Name);
     }
 }