Skip to content

Commit 0b6c28b

Browse files
Copilotedburns
andauthored
Port reference implementation changes: process cleanup, connect fallback, and E2E test coverage
Source changes from c063458 (Expand SDK E2E runtime coverage): - Add waitFor() after destroyForcibly() in process cleanup - Expand connect method fallback to match 'Unhandled method connect' message - Extract formatCliExitedMessage helper for consistent error formatting - Wait for stderr reader before throwing in port announcement New E2E tests ported: - EventFidelityTest: assistant.usage and session.usage_info event tests - ToolResultsTest: rejected and denied resultType handling - StreamingFidelityTest: streaming disabled on resume, reasoning effort - ToolsTest: parallel tools, availableTools/excludedTools combined - PermissionsTest: noResult kind, setApproveAll, slow handler Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
1 parent 7b80d72 commit 0b6c28b

7 files changed

Lines changed: 601 additions & 10 deletions

File tree

src/main/java/com/github/copilot/sdk/CliServerManager.java

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ final class CliServerManager {
3333

3434
private final CopilotClientOptions options;
3535
private final StringBuilder stderrBuffer = new StringBuilder();
36+
private volatile Thread stderrThread;
3637
private String connectionToken;
3738

3839
CliServerManager(CopilotClientOptions options) {
@@ -199,7 +200,7 @@ JsonRpcClient connectToServer(Process process, String tcpHost, Integer tcpPort)
199200
}
200201

201202
private void startStderrReader(Process process) {
202-
var stderrThread = new Thread(() -> {
203+
var thread = new Thread(() -> {
203204
try (BufferedReader reader = new BufferedReader(
204205
new InputStreamReader(process.getErrorStream(), StandardCharsets.UTF_8))) {
205206
String line;
@@ -213,8 +214,9 @@ private void startStderrReader(Process process) {
213214
LOG.log(Level.FINE, "Error reading stderr", e);
214215
}
215216
}, "cli-stderr-reader");
216-
stderrThread.setDaemon(true);
217-
stderrThread.start();
217+
thread.setDaemon(true);
218+
thread.start();
219+
this.stderrThread = thread;
218220
}
219221

220222
private Integer waitForPortAnnouncement(Process process) throws IOException {
@@ -226,11 +228,10 @@ private Integer waitForPortAnnouncement(Process process) throws IOException {
226228
while (System.currentTimeMillis() < deadline) {
227229
String line = reader.readLine();
228230
if (line == null) {
231+
awaitStderrReader();
229232
String stderr = getStderrOutput();
230-
if (!stderr.isEmpty()) {
231-
throw new IOException("CLI process exited unexpectedly. stderr: " + stderr);
232-
}
233-
throw new IOException("CLI process exited unexpectedly");
233+
throw new IOException(
234+
CopilotClient.formatCliExitedMessage("CLI process exited unexpectedly.", stderr));
234235
}
235236

236237
Matcher matcher = portPattern.matcher(line);
@@ -250,6 +251,17 @@ String getStderrOutput() {
250251
}
251252
}
252253

254+
private void awaitStderrReader() {
255+
Thread t = this.stderrThread;
256+
if (t != null) {
257+
try {
258+
t.join(5000);
259+
} catch (InterruptedException e) {
260+
Thread.currentThread().interrupt();
261+
}
262+
}
263+
}
264+
253265
private void clearStderrBuffer() {
254266
synchronized (stderrBuffer) {
255267
stderrBuffer.setLength(0);

src/main/java/com/github/copilot/sdk/CopilotClient.java

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -216,12 +216,20 @@ private Connection startCoreBody() {
216216
} catch (Exception e) {
217217
String stderr = serverManager.getStderrOutput();
218218
if (!stderr.isEmpty()) {
219-
throw new CompletionException(new IOException("CLI process exited unexpectedly. stderr: " + stderr, e));
219+
throw new CompletionException(
220+
new IOException(formatCliExitedMessage("CLI process exited unexpectedly.", stderr), e));
220221
}
221222
throw new CompletionException(e);
222223
}
223224
}
224225

226+
static String formatCliExitedMessage(String message, String stderrOutput) {
227+
if (stderrOutput == null || stderrOutput.isEmpty()) {
228+
return message;
229+
}
230+
return message + "\nstderr: " + stderrOutput;
231+
}
232+
225233
private static final int MIN_PROTOCOL_VERSION = 2;
226234
private static final int METHOD_NOT_FOUND_ERROR_CODE = -32601;
227235

@@ -244,7 +252,7 @@ private void verifyProtocolVersion(Connection connection) throws Exception {
244252
while (cause instanceof java.util.concurrent.ExecutionException || cause instanceof CompletionException) {
245253
cause = cause.getCause();
246254
}
247-
if (cause instanceof JsonRpcException rpcEx && rpcEx.getCode() == METHOD_NOT_FOUND_ERROR_CODE) {
255+
if (cause instanceof JsonRpcException rpcEx && isUnsupportedConnectMethod(rpcEx)) {
248256
// Legacy server without 'connect'; fall back to 'ping'.
249257
// A token, if any, is silently dropped — the legacy server can't enforce one.
250258
var params = new HashMap<String, Object>();
@@ -270,6 +278,10 @@ private void verifyProtocolVersion(Connection connection) throws Exception {
270278
}
271279
}
272280

281+
private static boolean isUnsupportedConnectMethod(JsonRpcException ex) {
282+
return ex.getCode() == METHOD_NOT_FOUND_ERROR_CODE || "Unhandled method connect".equals(ex.getMessage());
283+
}
284+
273285
/**
274286
* Disconnects from the Copilot server and closes all active sessions.
275287
* <p>
@@ -348,7 +360,7 @@ private CompletableFuture<Void> cleanupConnection() {
348360
if (connection.process != null) {
349361
try {
350362
if (connection.process.isAlive()) {
351-
connection.process.destroyForcibly();
363+
connection.process.destroyForcibly().waitFor();
352364
}
353365
} catch (Exception e) {
354366
LOG.log(Level.FINE, "Error killing process", e);
Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
/*---------------------------------------------------------------------------------------------
2+
* Copyright (c) Microsoft Corporation. All rights reserved.
3+
*--------------------------------------------------------------------------------------------*/
4+
5+
package com.github.copilot.sdk;
6+
7+
import static org.junit.jupiter.api.Assertions.*;
8+
9+
import java.util.ArrayList;
10+
import java.util.List;
11+
import java.util.concurrent.TimeUnit;
12+
13+
import org.junit.jupiter.api.AfterAll;
14+
import org.junit.jupiter.api.BeforeAll;
15+
import org.junit.jupiter.api.Test;
16+
17+
import com.github.copilot.sdk.generated.AssistantUsageEvent;
18+
import com.github.copilot.sdk.generated.SessionEvent;
19+
import com.github.copilot.sdk.generated.SessionUsageInfoEvent;
20+
import com.github.copilot.sdk.json.MessageOptions;
21+
import com.github.copilot.sdk.json.PermissionHandler;
22+
import com.github.copilot.sdk.json.SessionConfig;
23+
24+
/**
25+
* E2E tests for event fidelity — verifying the shape, ordering, and presence of
26+
* key events emitted from the runtime.
27+
*
28+
* <p>
29+
* Snapshots are stored in {@code test/snapshots/event_fidelity/}.
30+
* </p>
31+
*/
32+
public class EventFidelityTest {
33+
34+
private static E2ETestContext ctx;
35+
36+
@BeforeAll
37+
static void setup() throws Exception {
38+
ctx = E2ETestContext.create();
39+
}
40+
41+
@AfterAll
42+
static void teardown() throws Exception {
43+
if (ctx != null) {
44+
ctx.close();
45+
}
46+
}
47+
48+
/**
49+
* Verifies that an {@code assistant.usage} event is emitted after the model
50+
* processes a prompt.
51+
*
52+
* @see Snapshot:
53+
* event_fidelity/should_emit_assistant_usage_event_after_model_call
54+
*/
55+
@Test
56+
void testShouldEmitAssistantUsageEventAfterModelCall() throws Exception {
57+
ctx.configureForTest("event_fidelity", "should_emit_assistant_usage_event_after_model_call");
58+
59+
try (CopilotClient client = ctx.createClient()) {
60+
CopilotSession session = client
61+
.createSession(new SessionConfig().setOnPermissionRequest(PermissionHandler.APPROVE_ALL)).get();
62+
63+
List<SessionEvent> events = new ArrayList<>();
64+
session.on(events::add);
65+
66+
session.sendAndWait(new MessageOptions().setPrompt("What is 5+5? Reply with just the number.")).get(60,
67+
TimeUnit.SECONDS);
68+
69+
List<AssistantUsageEvent> usageEvents = events.stream().filter(e -> e instanceof AssistantUsageEvent)
70+
.map(e -> (AssistantUsageEvent) e).toList();
71+
72+
assertFalse(usageEvents.isEmpty(), "Should have received an assistant.usage event after model call");
73+
74+
AssistantUsageEvent lastUsage = usageEvents.get(usageEvents.size() - 1);
75+
assertNotNull(lastUsage.getData().model(), "Usage event should have a model field");
76+
assertFalse(lastUsage.getData().model().isEmpty(), "Model field should not be empty");
77+
78+
session.close();
79+
}
80+
}
81+
82+
/**
83+
* Verifies that a {@code session.usage_info} event is emitted after the model
84+
* processes a prompt.
85+
*
86+
* @see Snapshot:
87+
* event_fidelity/should_emit_session_usage_info_event_after_model_call
88+
*/
89+
@Test
90+
void testShouldEmitSessionUsageInfoEventAfterModelCall() throws Exception {
91+
ctx.configureForTest("event_fidelity", "should_emit_session_usage_info_event_after_model_call");
92+
93+
try (CopilotClient client = ctx.createClient()) {
94+
CopilotSession session = client
95+
.createSession(new SessionConfig().setOnPermissionRequest(PermissionHandler.APPROVE_ALL)).get();
96+
97+
List<SessionEvent> events = new ArrayList<>();
98+
session.on(events::add);
99+
100+
session.sendAndWait(new MessageOptions().setPrompt("What is 5+5? Reply with just the number.")).get(60,
101+
TimeUnit.SECONDS);
102+
103+
List<SessionUsageInfoEvent> usageInfoEvents = events.stream()
104+
.filter(e -> e instanceof SessionUsageInfoEvent).map(e -> (SessionUsageInfoEvent) e).toList();
105+
106+
assertFalse(usageInfoEvents.isEmpty(), "Should have received a session.usage_info event after model call");
107+
108+
session.close();
109+
}
110+
}
111+
}

src/test/java/com/github/copilot/sdk/PermissionsTest.java

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -363,4 +363,114 @@ void testShouldDenyToolOperationsWhenHandlerExplicitlyDeniesAfterResume(TestInfo
363363
session2.close();
364364
}
365365
}
366+
367+
/**
368+
* Verifies that a permission handler returning {@code noResult} is handled
369+
* correctly — the handler is called, and the session can be aborted afterward.
370+
*
371+
* @see Snapshot: permissions/should_deny_permission_with_noresult_kind
372+
*/
373+
@Test
374+
void testShouldDenyPermissionWithNoResultKind() throws Exception {
375+
ctx.configureForTest("permissions", "should_deny_permission_with_noresult_kind");
376+
377+
var permissionCalled = new CompletableFuture<Boolean>();
378+
379+
try (CopilotClient client = ctx.createClient()) {
380+
CopilotSession session = client
381+
.createSession(new SessionConfig().setOnPermissionRequest((request, invocation) -> {
382+
permissionCalled.complete(true);
383+
return CompletableFuture.completedFuture(
384+
new PermissionRequestResult().setKind(PermissionRequestResultKind.NO_RESULT));
385+
})).get();
386+
387+
session.send(new MessageOptions().setPrompt("Run 'node --version'"));
388+
389+
assertTrue(permissionCalled.get(30, TimeUnit.SECONDS),
390+
"Expected the no-result permission handler to be called.");
391+
392+
session.abort().get(10, TimeUnit.SECONDS);
393+
session.close();
394+
}
395+
}
396+
397+
/**
398+
* Verifies that the runtime short-circuits the permission handler when
399+
* {@code session.permissions.setApproveAll(true)} has been called.
400+
*
401+
* @see Snapshot:
402+
* permissions/should_short_circuit_permission_handler_when_set_approve_all_enabled
403+
*/
404+
@Test
405+
void testShouldShortCircuitPermissionHandlerWhenSetApproveAllEnabled() throws Exception {
406+
ctx.configureForTest("permissions", "should_short_circuit_permission_handler_when_set_approve_all_enabled");
407+
408+
var handlerCallCount = new int[]{0};
409+
410+
try (CopilotClient client = ctx.createClient()) {
411+
CopilotSession session = client
412+
.createSession(new SessionConfig().setOnPermissionRequest((request, invocation) -> {
413+
handlerCallCount[0]++;
414+
return CompletableFuture.completedFuture(
415+
new PermissionRequestResult().setKind(PermissionRequestResultKind.APPROVED));
416+
})).get();
417+
418+
// Set approve-all so the runtime short-circuits
419+
var setResult = session.getRpc().permissions
420+
.setApproveAll(new com.github.copilot.sdk.generated.rpc.SessionPermissionsSetApproveAllParams(
421+
session.getSessionId(), true))
422+
.get(10, TimeUnit.SECONDS);
423+
assertTrue(setResult.success(), "setApproveAll should succeed");
424+
425+
AssistantMessageEvent response = session
426+
.sendAndWait(new MessageOptions().setPrompt("Run 'echo test' and tell me what happens"))
427+
.get(60, TimeUnit.SECONDS);
428+
assertNotNull(response);
429+
430+
// Handler should not have been called since runtime approves all
431+
assertEquals(0, handlerCallCount[0],
432+
"Permission handler should not be called when setApproveAll is enabled");
433+
434+
session.close();
435+
}
436+
}
437+
438+
/**
439+
* Verifies that the SDK correctly waits for a slow permission handler before
440+
* completing tool execution.
441+
*
442+
* @see Snapshot: permissions/should_wait_for_slow_permission_handler
443+
*/
444+
@Test
445+
void testShouldWaitForSlowPermissionHandler() throws Exception {
446+
ctx.configureForTest("permissions", "should_wait_for_slow_permission_handler");
447+
448+
var handlerEntered = new CompletableFuture<Void>();
449+
var releaseHandler = new CompletableFuture<Void>();
450+
451+
try (CopilotClient client = ctx.createClient()) {
452+
CopilotSession session = client
453+
.createSession(new SessionConfig().setOnPermissionRequest((request, invocation) -> {
454+
handlerEntered.complete(null);
455+
return releaseHandler.thenApply(
456+
v -> new PermissionRequestResult().setKind(PermissionRequestResultKind.APPROVED));
457+
})).get();
458+
459+
// Use send (non-blocking) so we can interact with the handler
460+
CompletableFuture<AssistantMessageEvent> responseFuture = session
461+
.sendAndWait(new MessageOptions().setPrompt("Run 'echo slow_handler_test'"));
462+
463+
// Wait for permission handler to be entered
464+
handlerEntered.get(30, TimeUnit.SECONDS);
465+
466+
// Release the handler
467+
releaseHandler.complete(null);
468+
469+
// Session should complete successfully
470+
AssistantMessageEvent message = responseFuture.get(60, TimeUnit.SECONDS);
471+
assertNotNull(message);
472+
473+
session.close();
474+
}
475+
}
366476
}

0 commit comments

Comments
 (0)