Skip to content

Commit f36371a

Browse files
edburnsCopilot
edburns
and
Copilot
committed
Address Copilot review findings
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent a7880ac commit f36371a

7 files changed

Lines changed: 51 additions & 17 deletions

File tree

‎java/sdk/pom.xml‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -617,7 +617,7 @@ did not produce the multi-release output. Re-build on JDK 25+ and verify the
617617
<artifactId>maven-surefire-plugin</artifactId>
618618
<configuration>
619619
<skipTests>false</skipTests>
620-
<forkCount>0</forkCount>
620+
<forkCount>1</forkCount>
621621
<parallel>none</parallel>
622622
<environmentVariables>
623623
<COPILOT_SDK_DEFAULT_CONNECTION>inprocess</COPILOT_SDK_DEFAULT_CONNECTION>
@@ -628,7 +628,7 @@ did not produce the multi-release output. Re-build on JDK 25+ and verify the
628628
<groupId>org.apache.maven.plugins</groupId>
629629
<artifactId>maven-failsafe-plugin</artifactId>
630630
<configuration>
631-
<forkCount>0</forkCount>
631+
<forkCount>1</forkCount>
632632
<parallel>none</parallel>
633633
<environmentVariables>
634634
<COPILOT_CLI_PATH>${copilot.inprocess.cli.path}</COPILOT_CLI_PATH>

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,7 @@ public final class CopilotClient implements AutoCloseable {
123123
private final Integer optionsPort;
124124
private final RuntimeConnection runtimeConnection;
125125
private final String effectiveConnectionToken;
126+
private final Runnable closeHook;
126127
private volatile List<ModelInfo> modelsCache;
127128
private final Object modelsCacheLock = new Object();
128129

@@ -142,7 +143,12 @@ public CopilotClient() {
142143
* if mutually exclusive options are provided
143144
*/
144145
public CopilotClient(CopilotClientOptions options) {
146+
this(options, null);
147+
}
148+
149+
CopilotClient(CopilotClientOptions options, Runnable closeHook) {
145150
this.options = options != null ? options : new CopilotClientOptions();
151+
this.closeHook = closeHook;
146152

147153
// Resolve the transport: an explicit RuntimeConnection wins; otherwise the
148154
// COPILOT_SDK_DEFAULT_CONNECTION env var, or the individual transport options.
@@ -1663,6 +1669,9 @@ public void close() {
16631669
LOG.log(Level.FINE, "Error during close", e);
16641670
} finally {
16651671
shutdownOwnedExecutor();
1672+
if (closeHook != null) {
1673+
closeHook.run();
1674+
}
16661675
}
16671676
}
16681677

‎java/sdk/src/test/java/com/github/copilot/CopilotClientTest.java‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -131,6 +131,7 @@ void testClientConstructionWithOptions() {
131131
}
132132

133133
@Test
134+
@SkipInProcess("Validates external CLI URL transport options")
134135
void testCliUrlAutoCorrectsUseStdio() {
135136
var options = new CopilotClientOptions().setCliUrl("localhost:3000").setUseStdio(true);
136137

@@ -141,6 +142,7 @@ void testCliUrlAutoCorrectsUseStdio() {
141142
}
142143

143144
@Test
145+
@SkipInProcess("Validates external CLI URL transport options")
144146
void testCliUrlOnlyConstruction() {
145147
var options = new CopilotClientOptions().setCliUrl("localhost:4321");
146148

@@ -289,6 +291,7 @@ void testTcpConnectionTokenWithUseStdioThrows() {
289291
}
290292

291293
@Test
294+
@SkipInProcess("Validates TCP transport options")
292295
void testTcpConnectionTokenAcceptedInTcpMode() {
293296
var options = new CopilotClientOptions().setUseStdio(false).setTcpConnectionToken("my-token");
294297

@@ -409,6 +412,7 @@ void testOnLifecycleMultipleHandlers() throws Exception {
409412
// ===== getState() coverage =====
410413

411414
@Test
415+
@SkipInProcess("Validates subprocess CLI path failure handling")
412416
void testGetStateErrorAfterFailedStart() throws Exception {
413417
// Use a non-existent CLI path to trigger a startup failure
414418
var options = new CopilotClientOptions().setCliPath("/nonexistent/path/to/cli").setAutoStart(false);
@@ -429,6 +433,7 @@ void testGetStateErrorAfterFailedStart() throws Exception {
429433
}
430434

431435
@Test
436+
@SkipInProcess("Validates subprocess CLI path failure handling")
432437
void testGetStateConnectingDuringStart() throws Exception {
433438
// Use a non-existent CLI path; the future won't complete immediately
434439
var options = new CopilotClientOptions().setCliPath("/nonexistent/path/to/cli").setAutoStart(false);
@@ -469,6 +474,7 @@ void testCloseIsIdempotent() {
469474
}
470475

471476
@Test
477+
@SkipInProcess("Validates subprocess CLI path failure handling")
472478
void testCloseAfterFailedStart() throws Exception {
473479
var options = new CopilotClientOptions().setCliPath("/nonexistent/path/to/cli").setAutoStart(false);
474480
var client = new CopilotClient(options);
@@ -528,6 +534,7 @@ void testCloseSessionAfterStoppingClientDoesNotThrow() throws Exception {
528534
// ===== start() idempotency =====
529535

530536
@Test
537+
@SkipInProcess("Validates subprocess CLI path failure handling")
531538
void testStartIsIdempotentSingleConnectionAttempt() throws Exception {
532539
var options = new CopilotClientOptions().setCliPath("/nonexistent/path/to/cli").setAutoStart(false);
533540

‎java/sdk/src/test/java/com/github/copilot/E2ETestContext.java‎

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -327,8 +327,7 @@ public Map<String, String> getEnvironment() {
327327
*/
328328
public CopilotClient createClient() {
329329
CopilotClientOptions options = new CopilotClientOptions().setGitHubToken(DEFAULT_GITHUB_TOKEN);
330-
applyContextOptions(options);
331-
return new CopilotClient(options);
330+
return createClient(options);
332331
}
333332

334333
/**
@@ -341,18 +340,28 @@ public CopilotClient createClient() {
341340
* @return a new CopilotClient
342341
*/
343342
public CopilotClient createClient(CopilotClientOptions options) {
344-
applyContextOptions(options);
343+
CopilotClient client = applyContextOptions(options);
344+
if (client != null) {
345+
return client;
346+
}
345347
if (options.getGitHubToken() == null) {
346348
options.setGitHubToken(DEFAULT_GITHUB_TOKEN);
347349
}
348350

349351
return new CopilotClient(options);
350352
}
351353

352-
private void applyContextOptions(CopilotClientOptions options) {
354+
private CopilotClient applyContextOptions(CopilotClientOptions options) {
353355
if (isInProcessMode(options)) {
354-
inProcessEnvGuards.add(new InProcessEnvGuard(buildInProcessEnvironment(options)));
355-
return;
356+
InProcessEnvGuard guard = new InProcessEnvGuard(buildInProcessEnvironment(options));
357+
inProcessEnvGuards.add(guard);
358+
try {
359+
options.setEnvironment(null);
360+
return new CopilotClient(options, guard::close);
361+
} catch (RuntimeException e) {
362+
guard.close();
363+
throw e;
364+
}
356365
}
357366
if (options.getCliPath() == null) {
358367
options.setCliPath(cliPath);
@@ -363,6 +372,7 @@ private void applyContextOptions(CopilotClientOptions options) {
363372
if (options.getEnvironment() == null || options.getEnvironment().isEmpty()) {
364373
options.setEnvironment(getEnvironment());
365374
}
375+
return null;
366376
}
367377

368378
private boolean isInProcessMode(CopilotClientOptions options) {

‎java/sdk/src/test/java/com/github/copilot/ExecutorWiringTest.java‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -86,8 +86,7 @@ int getTaskCount() {
8686
}
8787

8888
private CopilotClientOptions createOptionsWithExecutor(TrackingExecutor executor) {
89-
CopilotClientOptions options = new CopilotClientOptions().setCliPath(ctx.getCliPath())
90-
.setCwd(ctx.getWorkDir().toString()).setEnvironment(ctx.getEnvironment()).setExecutor(executor)
89+
CopilotClientOptions options = new CopilotClientOptions().setExecutor(executor)
9190
.setGitHubToken("fake-token-for-e2e-tests");
9291
return options;
9392
}
@@ -111,7 +110,7 @@ void testClientStartUsesProvidedExecutor() throws Exception {
111110
TrackingExecutor trackingExecutor = new TrackingExecutor(ForkJoinPool.commonPool());
112111
int beforeStart = trackingExecutor.getTaskCount();
113112

114-
try (CopilotClient client = new CopilotClient(createOptionsWithExecutor(trackingExecutor))) {
113+
try (CopilotClient client = ctx.createClient(createOptionsWithExecutor(trackingExecutor))) {
115114
client.start().get(30, TimeUnit.SECONDS);
116115

117116
assertTrue(trackingExecutor.getTaskCount() > beforeStart,
@@ -156,7 +155,7 @@ void testToolCallDispatchUsesProvidedExecutor() throws Exception {
156155
});
157156

158157
// Reset count after client construction to isolate tool-call dispatch
159-
try (CopilotClient client = new CopilotClient(createOptionsWithExecutor(trackingExecutor))) {
158+
try (CopilotClient client = ctx.createClient(createOptionsWithExecutor(trackingExecutor))) {
160159
CopilotSession session = client.createSession(new SessionConfig().setTools(List.of(encryptTool))
161160
.setOnPermissionRequest(PermissionHandler.APPROVE_ALL)).get();
162161

@@ -198,7 +197,7 @@ void testPermissionDispatchUsesProvidedExecutor() throws Exception {
198197
var config = new SessionConfig().setOnPermissionRequest((request, invocation) -> CompletableFuture
199198
.completedFuture(new PermissionRequestResult().setKind(PermissionRequestResultKind.APPROVED)));
200199

201-
try (CopilotClient client = new CopilotClient(createOptionsWithExecutor(trackingExecutor))) {
200+
try (CopilotClient client = ctx.createClient(createOptionsWithExecutor(trackingExecutor))) {
202201
CopilotSession session = client.createSession(config).get();
203202

204203
Path testFile = ctx.getWorkDir().resolve("test.txt");
@@ -247,7 +246,7 @@ void testUserInputDispatchUsesProvidedExecutor() throws Exception {
247246
.completedFuture(new UserInputResponse().setAnswer(answer).setWasFreeform(wasFreeform));
248247
});
249248

250-
try (CopilotClient client = new CopilotClient(createOptionsWithExecutor(trackingExecutor))) {
249+
try (CopilotClient client = ctx.createClient(createOptionsWithExecutor(trackingExecutor))) {
251250
CopilotSession session = client.createSession(config).get();
252251

253252
int beforeSend = trackingExecutor.getTaskCount();
@@ -286,7 +285,7 @@ void testHooksDispatchUsesProvidedExecutor() throws Exception {
286285
.setHooks(new SessionHooks().setOnPreToolUse(
287286
(input, invocation) -> CompletableFuture.completedFuture(PreToolUseHookOutput.allow())));
288287

289-
try (CopilotClient client = new CopilotClient(createOptionsWithExecutor(trackingExecutor))) {
288+
try (CopilotClient client = ctx.createClient(createOptionsWithExecutor(trackingExecutor))) {
290289
CopilotSession session = client.createSession(config).get();
291290

292291
Path testFile = ctx.getWorkDir().resolve("hello.txt");
@@ -342,7 +341,7 @@ void testClientStopUsesProvidedExecutor() throws Exception {
342341
return CompletableFuture.completedFuture(input.toUpperCase());
343342
});
344343

345-
CopilotClient client = new CopilotClient(createOptionsWithExecutor(trackingExecutor));
344+
CopilotClient client = ctx.createClient(createOptionsWithExecutor(trackingExecutor));
346345
client.createSession(new SessionConfig().setTools(List.of(encryptTool))
347346
.setOnPermissionRequest(PermissionHandler.APPROVE_ALL)).get();
348347

‎java/sdk/src/test/java/com/github/copilot/MetadataApiTest.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import com.github.copilot.generated.rpc.ModelBillingTokenPrices;
1111
import com.github.copilot.generated.rpc.ModelBillingTokenPricesLongContext;
1212
import com.github.copilot.rpc.*;
13+
import com.github.copilot.e2e.SkipInProcess;
1314
import org.junit.jupiter.api.BeforeAll;
1415
import org.junit.jupiter.api.Test;
1516

@@ -271,6 +272,7 @@ void testGetModelsResponseDeserialization() throws Exception {
271272
// ===== Integration Tests (require CLI) =====
272273

273274
@Test
275+
@SkipInProcess("Uses explicit CLI stdio transport")
274276
void testGetStatus() throws Exception {
275277
assertNotNull(cliPath, "Copilot CLI not found in PATH or COPILOT_CLI_PATH");
276278

@@ -287,6 +289,7 @@ void testGetStatus() throws Exception {
287289
}
288290

289291
@Test
292+
@SkipInProcess("Uses explicit CLI stdio transport")
290293
void testGetAuthStatus() throws Exception {
291294
assertNotNull(cliPath, "Copilot CLI not found in PATH or COPILOT_CLI_PATH");
292295

@@ -302,6 +305,7 @@ void testGetAuthStatus() throws Exception {
302305
}
303306

304307
@Test
308+
@SkipInProcess("Uses explicit CLI stdio transport")
305309
void testListModels() throws Exception {
306310
assertNotNull(cliPath, "Copilot CLI not found in PATH or COPILOT_CLI_PATH");
307311

‎java/sdk/src/test/java/com/github/copilot/ffi/InProcessEnvGuard.java‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ private interface LibcEnv extends Library {
8383
* name -> previous value ({@code null} means the variable was not set before).
8484
*/
8585
private final List<Map.Entry<String, String>> saved = new ArrayList<>();
86+
private boolean closed;
8687

8788
/**
8889
* Applies {@code applyEnv} to the native process environment block, saving the
@@ -116,7 +117,11 @@ private void apply(String name, String value) {
116117
* before construction.
117118
*/
118119
@Override
119-
public void close() {
120+
public synchronized void close() {
121+
if (closed) {
122+
return;
123+
}
124+
closed = true;
120125
List<Map.Entry<String, String>> reversed = new ArrayList<>(saved);
121126
Collections.reverse(reversed);
122127
for (Map.Entry<String, String> entry : reversed) {

0 commit comments

Comments
 (0)