fix(go): capture CLI stderr and fix SetProcessDone race by claudiogodoy99 · Pull Request #863 · github/copilot-sdk

claudiogodoy99 · 2026-03-15T05:59:23Z

Fix: Capture CLI stderr and resolve SetProcessDone race

Problem

When the CLI process exits during startup, the SDK reports an opaque error with no indication of why the process failed:

panic: process exited unexpectedly
        failed to kill CLI process: os: process already finished

The actual cause (e.g. missing module, auth failure, version mismatch) is written to stderr, which the SDK silently discards.

Root Causes

Stderr not captured — client.go never sets exec.Cmd.Stderr, so all CLI diagnostic output is lost.
Race in SetProcessDone — jsonrpc2.go copies the process error inside an async goroutine. When Request() detects processDone is closed and immediately calls getProcessError(), the goroutine hasn't run yet, so the error is nil and callers get the generic "process exited unexpectedly" message.

Changes

File	Change
`client.go`	Set `c.process.Stderr = &bytes.Buffer{}` before `Start()` in both stdio and TCP branches
`client.go`	Read stderr buffer in `monitorProcess` after `Wait()` and include contents in error
`internal/jsonrpc2/jsonrpc2.go`	Replace async goroutine in `SetProcessDone` with direct error pointer storage
`internal/jsonrpc2/jsonrpc2.go`	`getProcessError()` now dereferences the stored pointer, which is set before channel close
`client_test.go`	Tests verifying stderr is captured on both zero and non-zero exits
`internal/jsonrpc2/jsonrpc2_test.go`	Tests verifying error is available immediately after `processDone` closes

After

panic: CLI process exited: exit status 1
        stderr: Error: Cannot find module '.../copilot/index.js'
            at Function._resolveFilename (node:internal/modules/cjs/loader:1383:15)
            ...

Testing

TestMonitorProcess_StderrCaptured — stderr included in error on non-zero exit
TestMonitorProcess_StderrCapturedOnZeroExit — stderr included on exit code 0
TestStartCLIServer_StderrFieldSet — confirms Stderr is assigned
TestSetProcessDone_ErrorAvailableImmediately — no race: error accessible right after channel close
TestSetProcessDone_RequestMissesProcessError — Request() returns actual error, not generic message
TestSetProcessDone_ErrorCopiedEventually — sanity check that error pointer works after yield

Manual Test Evidence

Before the fix — stderr is discarded, error is opaque:

panic: process exited unexpectedly
        failed to kill CLI process: os: process already finished

After the fix — stderr is captured and included in the error:

$ go run samples/chat.go
panic: CLI process exited: exit status 1
        stderr: node:internal/modules/cjs/loader:1386
          throw err;
          ^

        Error: Cannot find module '/home/claudiogodoy/repos/fork/nodejs/node_modules/@github/copilot/index.js'
            at Function._resolveFilename (node:internal/modules/cjs/loader:1383:15)
            at defaultResolveImpl (node:internal/modules/cjs/loader:1025:19)
            at resolveForCJSWithHooks (node:internal/modules/cjs/loader:1030:22)
            at Function._load (node:internal/modules/cjs/loader:1192:37)
            at TracingChannel.traceSync (node:diagnostics_channel:328:14)
            at wrapModuleLoad (node:internal/modules/cjs/loader:237:24)
            at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:171:5)
            at node:internal/main/run_main_module:36:49 {
          code: 'MODULE_NOT_FOUND',
          requireStack: []
        }

        Node.js v22.21.0
        failed to kill CLI process: os: process already finished

goroutine 1 [running]:
main.main()
        /home/claudiogodoy/repos/fork/copilot-sdk/go/samples/chat.go:22 +0x4a7
exit status 2

The error now clearly shows the CLI failed because of a missing Node.js module, making the root cause immediately actionable.

…cess exit

claudiogodoy99 · 2026-03-15T06:02:13Z

pr for issue: #862

Copilot

Pull request overview

This PR improves the Go SDK’s error reporting and process-exit handling by removing a race in how the JSON-RPC client observes CLI process exit errors, and by capturing CLI stderr to enrich exit diagnostics.

Changes:

Replace async copying of the CLI process error in jsonrpc2.Client with a stored error pointer read after processDone closes.
Capture CLI stderr into a buffer and include it in monitorProcess() exit errors.
Add unit tests covering the process error visibility and stderr capture behavior.

Reviewed changes

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

File	Description
go/internal/jsonrpc2/jsonrpc2.go	Switches to pointer-based process error retrieval to avoid timing races after `processDone` closes.
go/internal/jsonrpc2/jsonrpc2_test.go	Adds tests for the process error visibility behavior (but some comments/names still describe the pre-change implementation).
go/client.go	Captures CLI stderr and appends it to process-exit errors.
go/client_test.go	Adds tests asserting stderr is included in the process error (but currently uses non-portable shell commands).

go/client_test.go

+
+	stderrMsg := "error: authentication failed: invalid token"
+	client.process = exec.Command("sh", "-c", "echo '"+stderrMsg+"' >&2; exit 1")
+
+	// Replicate what startCLIServer now does: capture stderr.


go/client_test.go

+
+	stderrMsg := "warning: version mismatch, shutting down"
+	client.process = exec.Command("sh", "-c", "echo '"+stderrMsg+"' >&2; exit 0")
+	client.process.Stderr = &bytes.Buffer{}
+


go/client_test.go

+	// 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_test.go

+// 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.


go/internal/jsonrpc2/jsonrpc2_test.go

+// 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) {


go/internal/jsonrpc2/jsonrpc2_test.go

+// 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) {


go/client.go

 			return fmt.Errorf("failed to create stdout pipe: %w", err)
 		}

+		c.process.Stderr = &bytes.Buffer{}


go/client.go

 		// For TCP mode, capture stdout to get port number
 		stdout, err := c.process.StdoutPipe()
 		if err != nil {
 			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)
 		}


claudiogodoy99 added 2 commits March 15, 2026 02:37

test(go): validate stderr not captured and SetProcessDone race on pro…

f395846

…cess exit

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

971e234

claudiogodoy99 requested a review from a team as a code owner March 15, 2026 05:59

Copilot AI review requested due to automatic review settings March 15, 2026 05:59

Copilot started reviewing on behalf of claudiogodoy99 March 15, 2026 06:00 View session

claudiogodoy99 mentioned this pull request Mar 15, 2026

[go] - Client does not capture CLI stderr, making startup failures opaque #862

Open

Copilot AI reviewed Mar 15, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

fix(go): capture CLI stderr and fix SetProcessDone race#863
claudiogodoy99 wants to merge 2 commits intogithub:mainfrom
claudiogodoy99:fix/capture-cli-stderr

claudiogodoy99 commented Mar 15, 2026

Uh oh!

claudiogodoy99 commented Mar 15, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

claudiogodoy99 commented Mar 15, 2026

Fix: Capture CLI stderr and resolve SetProcessDone race

Problem

Root Causes

Changes

After

Testing

Manual Test Evidence

Uh oh!

claudiogodoy99 commented Mar 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants