Skip to content

Commit beb427c

Browse files
Improve Go SDK error propagation and enhance CLI error tests
Go SDK: - Change processDone from buffered error channel to closed signal channel - Store processError separately and use mutex for thread-safe access - Check for process exit before sending requests (fail fast) - Subsequent requests after process exit now get the same stderr error All SDKs: - Enhanced CLI error tests to verify subsequent calls also fail properly - Tests now call createSession/send instead of just ping Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 393ef10 commit beb427c

File tree

6 files changed

+107
-14
lines changed

6 files changed

+107
-14
lines changed

dotnet/test/ClientTests.cs

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -244,6 +244,19 @@ public async Task Should_Report_Error_With_Stderr_When_CLI_Fails_To_Start()
244244
Assert.Contains("stderr", errorMessage, StringComparison.OrdinalIgnoreCase);
245245
Assert.Contains("nonexistent", errorMessage, StringComparison.OrdinalIgnoreCase);
246246

247+
// Verify subsequent calls also fail with the same error (containing stderr info)
248+
var ex2 = await Assert.ThrowsAnyAsync<Exception>(async () =>
249+
{
250+
var session = await client.CreateSessionAsync();
251+
await session.SendAsync(new MessageOptions { Prompt = "test" });
252+
});
253+
var errorMessage2 = ex2.Message;
254+
Assert.True(
255+
errorMessage2.Contains("stderr", StringComparison.OrdinalIgnoreCase) ||
256+
errorMessage2.Contains("nonexistent", StringComparison.OrdinalIgnoreCase) ||
257+
errorMessage2.Contains("not connected", StringComparison.OrdinalIgnoreCase),
258+
$"Expected subsequent error to reference CLI failure, got: {errorMessage2}");
259+
247260
// Cleanup - ForceStop should handle the disconnected state gracefully
248261
try { await client.ForceStopAsync(); } catch { /* Expected */ }
249262
}

go/client.go

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,8 @@ type Client struct {
8888
typedLifecycleHandlers map[SessionLifecycleEventType][]SessionLifecycleHandler
8989
lifecycleHandlersMux sync.Mutex
9090
stderrBuf bytes.Buffer // captures CLI stderr for error messages
91-
processDone chan error // signals when CLI process exits
91+
processDone chan struct{} // closed when CLI process exits
92+
processError error // set before processDone is closed
9293

9394
// RPC provides typed server-scoped RPC methods.
9495
// This field is nil until the client is connected via Start().
@@ -1107,22 +1108,23 @@ func (c *Client) startCLIServer(ctx context.Context) error {
11071108
}
11081109

11091110
// Monitor process exit to signal pending requests
1110-
c.processDone = make(chan error, 1)
1111+
c.processDone = make(chan struct{})
11111112
go func() {
11121113
err := c.process.Wait()
11131114
stderrOutput := strings.TrimSpace(c.stderrBuf.String())
11141115
if stderrOutput != "" {
1115-
c.processDone <- fmt.Errorf("CLI process exited: %v\nstderr: %s", err, stderrOutput)
1116+
c.processError = fmt.Errorf("CLI process exited: %v\nstderr: %s", err, stderrOutput)
11161117
} else if err != nil {
1117-
c.processDone <- fmt.Errorf("CLI process exited: %v", err)
1118+
c.processError = fmt.Errorf("CLI process exited: %v", err)
11181119
} else {
1119-
c.processDone <- fmt.Errorf("CLI process exited unexpectedly")
1120+
c.processError = fmt.Errorf("CLI process exited unexpectedly")
11201121
}
1122+
close(c.processDone)
11211123
}()
11221124

11231125
// Create JSON-RPC client immediately
11241126
c.client = jsonrpc2.NewClient(stdin, stdout)
1125-
c.client.SetProcessDone(c.processDone)
1127+
c.client.SetProcessDone(c.processDone, &c.processError)
11261128
c.RPC = rpc.NewServerRpc(c.client)
11271129
c.setupNotificationHandler()
11281130
c.client.Start()

go/internal/e2e/client_test.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,5 +245,18 @@ func TestClient(t *testing.T) {
245245
if !strings.Contains(errStr, "stderr") || !strings.Contains(errStr, "nonexistent") {
246246
t.Errorf("Expected error to contain stderr output about invalid flag, got: %v", err)
247247
}
248+
249+
// Verify subsequent calls also fail with the same error
250+
session, err := client.CreateSession(t.Context(), nil)
251+
if err == nil {
252+
_, err = session.Send(t.Context(), copilot.MessageOptions{Prompt: "test"})
253+
}
254+
if err == nil {
255+
t.Fatal("Expected CreateSession/Send to fail after CLI exit")
256+
}
257+
errStr = err.Error()
258+
if !strings.Contains(errStr, "stderr") && !strings.Contains(errStr, "nonexistent") {
259+
t.Errorf("Expected subsequent error to reference CLI failure, got: %v", err)
260+
}
248261
})
249262
}

go/internal/jsonrpc2/jsonrpc2.go

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,9 @@ type Client struct {
5757
running bool
5858
stopChan chan struct{}
5959
wg sync.WaitGroup
60-
processDone <-chan error // signals when the underlying process exits
60+
processDone chan struct{} // closed when the underlying process exits
61+
processError error // set before processDone is closed
62+
processErrorMu sync.RWMutex // protects processError
6163
}
6264

6365
// NewClient creates a new JSON-RPC client
@@ -71,9 +73,26 @@ func NewClient(stdin io.WriteCloser, stdout io.ReadCloser) *Client {
7173
}
7274
}
7375

74-
// SetProcessDone sets a channel that signals when the underlying process exits
75-
func (c *Client) SetProcessDone(ch <-chan error) {
76-
c.processDone = ch
76+
// SetProcessDone sets a channel that will be closed when the process exits,
77+
// and stores the error that should be returned to pending/future requests.
78+
func (c *Client) SetProcessDone(done chan struct{}, errPtr *error) {
79+
c.processDone = done
80+
// Monitor the channel and copy the error when it closes
81+
go func() {
82+
<-done
83+
if errPtr != nil {
84+
c.processErrorMu.Lock()
85+
c.processError = *errPtr
86+
c.processErrorMu.Unlock()
87+
}
88+
}()
89+
}
90+
91+
// getProcessError returns the process exit error if the process has exited
92+
func (c *Client) getProcessError() error {
93+
c.processErrorMu.RLock()
94+
defer c.processErrorMu.RUnlock()
95+
return c.processError
7796
}
7897

7998
// Start begins listening for messages in a background goroutine
@@ -178,6 +197,19 @@ func (c *Client) Request(method string, params any) (json.RawMessage, error) {
178197
c.mu.Unlock()
179198
}()
180199

200+
// Check if process already exited before sending
201+
if c.processDone != nil {
202+
select {
203+
case <-c.processDone:
204+
if err := c.getProcessError(); err != nil {
205+
return nil, err
206+
}
207+
return nil, fmt.Errorf("process exited unexpectedly")
208+
default:
209+
// Process still running, continue
210+
}
211+
}
212+
181213
paramsData, err := json.Marshal(params)
182214
if err != nil {
183215
return nil, fmt.Errorf("failed to marshal params: %w", err)
@@ -203,8 +235,8 @@ func (c *Client) Request(method string, params any) (json.RawMessage, error) {
203235
return nil, response.Error
204236
}
205237
return response.Result, nil
206-
case err := <-c.processDone:
207-
if err != nil {
238+
case <-c.processDone:
239+
if err := c.getProcessError(); err != nil {
208240
return nil, err
209241
}
210242
return nil, fmt.Errorf("process exited unexpectedly")

nodejs/test/e2e/client.test.ts

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -140,13 +140,31 @@ describe("Client", () => {
140140
});
141141
onTestFinishedForceStop(client);
142142

143+
let initialError: Error | undefined;
143144
try {
144145
await client.start();
145146
expect.fail("Expected start() to throw an error");
147+
} catch (error) {
148+
initialError = error as Error;
149+
expect(initialError.message).toContain("stderr");
150+
expect(initialError.message).toContain("nonexistent");
151+
}
152+
153+
// Verify subsequent calls also fail with the same error
154+
try {
155+
const session = await client.createSession();
156+
await session.send("test");
157+
expect.fail("Expected send() to throw an error after CLI exit");
146158
} catch (error) {
147159
const message = (error as Error).message;
148-
expect(message).toContain("stderr");
149-
expect(message).toContain("nonexistent");
160+
// Accept either stderr info or a "not connected" style error
161+
expect(
162+
message.includes("stderr") ||
163+
message.includes("nonexistent") ||
164+
message.includes("not connected") ||
165+
message.includes("Connection") ||
166+
message.includes("closed")
167+
).toBe(true);
150168
}
151169
});
152170
});

python/e2e/test_client.py

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,5 +201,20 @@ async def test_should_report_error_with_stderr_when_cli_fails_to_start(self):
201201
assert "nonexistent" in error_message, (
202202
f"Expected error to contain 'nonexistent', got: {error_message}"
203203
)
204+
205+
# Verify subsequent calls also fail with the same error
206+
with pytest.raises(Exception) as exc_info2:
207+
session = await client.create_session()
208+
await session.send("test")
209+
210+
error_message2 = str(exc_info2.value)
211+
# Accept either stderr info or a generic connection error
212+
assert (
213+
"stderr" in error_message2
214+
or "nonexistent" in error_message2
215+
or "Invalid argument" in error_message2
216+
or "not connected" in error_message2.lower()
217+
or "closed" in error_message2.lower()
218+
), f"Expected subsequent error to reference CLI failure, got: {error_message2}"
204219
finally:
205220
await client.force_stop()

0 commit comments

Comments
 (0)