fix(go): capture CLI stderr and fix SetProcessDone race

go/client.go

@@ -30,6 +30,7 @@ package copilot
 
 import (
 	"bufio"
+	"bytes"
 	"context"
 	"encoding/json"
 	"errors"
@@ -1252,6 +1253,8 @@ func (c *Client) startCLIServer(ctx context.Context) error {
 			return fmt.Errorf("failed to create stdout pipe: %w", err)
 		}
 
+		c.process.Stderr = &bytes.Buffer{}
+
 		if err := c.process.Start(); err != nil {
 			return fmt.Errorf("failed to start CLI server: %w", err)
 		}
@@ -1282,6 +1285,8 @@ func (c *Client) startCLIServer(ctx context.Context) error {
 			return fmt.Errorf("failed to create stdout pipe: %w", err)
 		}
 
+		c.process.Stderr = &bytes.Buffer{}
+
 		if err := c.process.Start(); err != nil {
 			return fmt.Errorf("failed to start CLI server: %w", err)
 		}
@@ -1342,10 +1347,22 @@ func (c *Client) monitorProcess() {
 	c.processErrorPtr = &processError
 	go func() {
 		waitErr := proc.Wait()
+		var stderrOutput string
+		if buf, ok := proc.Stderr.(*bytes.Buffer); ok {
+			stderrOutput = strings.TrimSpace(buf.String())
+		}
 		if waitErr != nil {
-			processError = fmt.Errorf("CLI process exited: %w", waitErr)
+			if stderrOutput != "" {
+				processError = fmt.Errorf("CLI process exited: %w\nstderr: %s", waitErr, stderrOutput)
+			} else {
+				processError = fmt.Errorf("CLI process exited: %w", waitErr)
+			}
 		} else {
-			processError = errors.New("CLI process exited unexpectedly")
+			if stderrOutput != "" {
+				processError = fmt.Errorf("CLI process exited unexpectedly\nstderr: %s", stderrOutput)
+			} else {
+				processError = errors.New("CLI process exited unexpectedly")
+			}
 		}
 		close(done)
 	}()

go/client_test.go

@@ -1,12 +1,15 @@
 package copilot
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"reflect"
 	"regexp"
+	"strings"
 	"sync"
 	"testing"
 )
@@ -648,3 +651,108 @@ func TestClient_StartStopRace(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+// TestHelperProcess is a helper used by tests that need to spawn a process
+// which writes to stderr and exits with a non-zero status. It is invoked
+// via "go test" by running the test binary itself with -test.run.
+func TestHelperProcess(t *testing.T) {
+	if os.Getenv("GO_WANT_HELPER_PROCESS") != "1" {
+		// Not in helper process mode; let the test run normally.
+		return
+	}
+
+	// Find the "--" separator and treat the argument after it as the stderr message.
+	args := os.Args
+	i := 0
+	for i < len(args) && args[i] != "--" {
+		i++
+	}
+	var msg string
+	if i+1 < len(args) {
+		msg = args[i+1]
+	} else {
+		msg = "no stderr message provided"
+	}
+
+	_, _ = os.Stderr.WriteString(msg + "\n")
+	os.Exit(1)
+}
+
+// TestMonitorProcess_StderrCaptured validates that when the CLI process
+// writes an error to stderr and exits, the stderr content IS included
+// in the process error (now that startCLIServer sets Stderr).
+func TestMonitorProcess_StderrCaptured(t *testing.T) {
+	client := &Client{
+		sessions: make(map[string]*Session),
+	}
+
+	stderrMsg := "error: authentication failed: invalid token"
+	client.process = exec.Command(os.Args[0], "-test.run=TestHelperProcess", "--", stderrMsg)
+	client.process.Env = append(os.Environ(), "GO_WANT_HELPER_PROCESS=1")
+
+	// Replicate what startCLIServer now does: capture stderr.
+	client.process.Stderr = &bytes.Buffer{}
+
+	if err := client.process.Start(); err != nil {
+		t.Fatalf("failed to start test process: %v", err)
+	}
+
+	client.monitorProcess()
+
+	// Wait for the process to exit.
+	<-client.processDone
+
+	processError := *client.processErrorPtr
+	if processError == nil {
+		t.Fatal("expected a process error after non-zero exit, got nil")
+	}
+
+	if !strings.Contains(processError.Error(), stderrMsg) {
+		t.Errorf("stderr output not included in process error.\n"+
+			"  got:  %q\n"+
+			"  want: error containing %q", processError.Error(), stderrMsg)
+	}
+}
+
+// TestMonitorProcess_StderrCapturedOnZeroExit validates that even when the
+// CLI process exits with code 0, stderr content is included in the error.
+func TestMonitorProcess_StderrCapturedOnZeroExit(t *testing.T) {
+	client := &Client{
+		sessions: make(map[string]*Session),
+	}
+
+	stderrMsg := "warning: version mismatch, shutting down"
+	client.process = exec.Command("sh", "-c", "echo '"+stderrMsg+"' >&2; exit 0")
+	client.process.Stderr = &bytes.Buffer{}
+
+	if err := client.process.Start(); err != nil {
+		t.Fatalf("failed to start test process: %v", err)
+	}
+
+	client.monitorProcess()
+	<-client.processDone
+
+	processError := *client.processErrorPtr
+	if processError == nil {
+		t.Fatal("expected a process error for unexpected exit, got nil")
+	}
+
+	if !strings.Contains(processError.Error(), stderrMsg) {
+		t.Errorf("stderr output not included in process error for exit code 0.\n"+
+			"  got:  %q\n"+
+			"  want: error containing %q", processError.Error(), stderrMsg)
+	}
+}
+
+// TestStartCLIServer_StderrFieldSet verifies that startCLIServer sets
+// exec.Cmd.Stderr to a bytes.Buffer so CLI diagnostic output is captured.
+func TestStartCLIServer_StderrFieldSet(t *testing.T) {
+	// Verify that a bytes.Buffer assigned to Stderr is recognized by
+	// monitorProcess (type assertion to *bytes.Buffer).
+	cmd := exec.Command("true")
+	buf := &bytes.Buffer{}
+	cmd.Stderr = buf
+	if _, ok := cmd.Stderr.(*bytes.Buffer); !ok {
+		t.Error("expected Stderr to be *bytes.Buffer after assignment")
+	}
+}

go/internal/jsonrpc2/jsonrpc2.go

@@ -59,8 +59,8 @@ type Client struct {
 	stopChan        chan struct{}
 	wg              sync.WaitGroup
 	processDone     chan struct{} // closed when the underlying process exits
-	processError    error         // set before processDone is closed
-	processErrorMu  sync.RWMutex  // protects processError
+	processErrorPtr *error        // points to the process error
+	processErrorMu  sync.RWMutex  // protects processErrorPtr
 	onClose         func()        // called when the read loop exits unexpectedly
 }
 
@@ -76,25 +76,26 @@ func NewClient(stdin io.WriteCloser, stdout io.ReadCloser) *Client {
 }
 
 // SetProcessDone sets a channel that will be closed when the process exits,
-// and stores the error that should be returned to pending/future requests.
+// and stores the error pointer that should be returned to pending/future requests.
+// The error is read directly from the pointer after the channel closes, avoiding
+// a race between an async goroutine and callers checking the error.
 func (c *Client) SetProcessDone(done chan struct{}, errPtr *error) {
 	c.processDone = done
-	// Monitor the channel and copy the error when it closes
-	go func() {
-		<-done
-		if errPtr != nil {
-			c.processErrorMu.Lock()
-			c.processError = *errPtr
-			c.processErrorMu.Unlock()
-		}
-	}()
+	c.processErrorMu.Lock()
+	c.processErrorPtr = errPtr
+	c.processErrorMu.Unlock()
 }
 
-// getProcessError returns the process exit error if the process has exited
+// getProcessError returns the process exit error if the process has exited.
+// It reads directly from the stored error pointer, which is guaranteed to be
+// set before the processDone channel is closed.
 func (c *Client) getProcessError() error {
 	c.processErrorMu.RLock()
 	defer c.processErrorMu.RUnlock()
-	return c.processError
+	if c.processErrorPtr != nil {
+		return *c.processErrorPtr
+	}
+	return nil
 }
 
 // Start begins listening for messages in a background goroutine

go/internal/jsonrpc2/jsonrpc2_test.go

@@ -1,7 +1,9 @@
 package jsonrpc2
 
 import (
+	"errors"
 	"io"
+	"runtime"
 	"sync"
 	"testing"
 	"time"
@@ -67,3 +69,121 @@ func TestOnCloseNotCalledOnIntentionalStop(t *testing.T) {
 		t.Error("onClose should not be called on intentional Stop()")
 	}
 }
+
+// TestSetProcessDone_ErrorAvailableImmediately validates that getProcessError()
+// returns the correct error immediately after processDone is closed.
+// The current implementation copies the error in an async goroutine, which
+// creates a race: the channel close is visible to callers before the error
+// is stored, so getProcessError() can return nil.
+func TestSetProcessDone_ErrorAvailableImmediately(t *testing.T) {
+	misses := 0
+	const iterations = 1000
+
+	for i := 0; i < iterations; i++ {
+		stdinR, stdinW := io.Pipe()
+		stdoutR, stdoutW := io.Pipe()
+
+		client := NewClient(stdinW, stdoutR)
+
+		done := make(chan struct{})
+		processErr := errors.New("CLI process exited: exit status 1")
+
+		client.SetProcessDone(done, &processErr)
+
+		// Simulate process exit: error is already set, close the channel.
+		close(done)
+
+		// Do NOT yield to the scheduler — check immediately.
+		// In the current code the goroutine inside SetProcessDone may not
+		// have copied the error to client.processError yet.
+		if err := client.getProcessError(); err == nil {
+			misses++
+		}
+
+		stdinR.Close()
+		stdinW.Close()
+		stdoutR.Close()
+		stdoutW.Close()
+	}
+
+	if misses > 0 {
+		t.Errorf("SetProcessDone race: getProcessError() returned nil %d/%d times "+
+			"immediately after processDone was closed. The async goroutine had not "+
+			"yet copied the error.", misses, iterations)
+	}
+}
+
+// TestSetProcessDone_RequestMissesProcessError validates that the Request()
+// method can fall through to the generic "process exited unexpectedly" message
+// when the SetProcessDone goroutine hasn't copied the error in time.
+func TestSetProcessDone_RequestMissesProcessError(t *testing.T) {
+	misses := 0
+	const iterations = 100
+
+	for i := 0; i < iterations; i++ {
+		stdinR, stdinW := io.Pipe()
+		stdoutR, stdoutW := io.Pipe()
+
+		client := NewClient(stdinW, stdoutR)
+		client.Start()
+
+		done := make(chan struct{})
+		processErr := errors.New("CLI process exited: authentication failed")
+
+		client.SetProcessDone(done, &processErr)
+
+		// Simulate process exit.
+		close(done)
+		// Close the writer so the readLoop can exit.
+		stdoutW.Close()
+
+		// Make a request — should get the specific process error.
+		_, err := client.Request("test.method", nil)
+		if err != nil && err.Error() == "process exited unexpectedly" {
+			misses++
+		}
+
+		client.Stop()
+		stdinR.Close()
+		stdinW.Close()
+		stdoutR.Close()
+	}
+
+	if misses > 0 {
+		t.Errorf("Request() race: returned generic 'process exited unexpectedly' %d/%d times "+
+			"instead of the actual process error. The error was lost because "+
+			"SetProcessDone copies it asynchronously.", misses, iterations)
+	}
+}
+
+// TestSetProcessDone_ErrorCopiedEventually verifies that the error IS eventually
+// available if we give the goroutine time to run — confirming the issue is
+// purely a timing race, not a logic error.
+func TestSetProcessDone_ErrorCopiedEventually(t *testing.T) {
+	stdinR, stdinW := io.Pipe()
+	stdoutR, stdoutW := io.Pipe()
+	defer stdinR.Close()
+	defer stdinW.Close()
+	defer stdoutR.Close()
+	defer stdoutW.Close()
+
+	client := NewClient(stdinW, stdoutR)
+
+	done := make(chan struct{})
+	processErr := errors.New("CLI process exited: version mismatch")
+
+	client.SetProcessDone(done, &processErr)
+
+	// Close the channel and yield to let the goroutine run.
+	close(done)
+	runtime.Gosched()
+	time.Sleep(10 * time.Millisecond)
+
+	err := client.getProcessError()
+	if err == nil {
+		t.Fatal("expected process error to be available after yielding, got nil")
+	}
+	if err.Error() != processErr.Error() {
+		t.Errorf("expected %q, got %q", processErr.Error(), err.Error())
+	}
+}