Skip to content

Commit 7951af7

Browse files
authored
Fix cliUrl to auto-correct useStdio for backend service scenarios (#97)
When cliUrl is set (connecting to an external CLI server), useStdio is now automatically set to false since TCP is inherently required. Previously, users had to explicitly call setUseStdio(false) alongside setCliUrl(), which contradicted the setup.md documentation. Also simplified CliServerManager.connectToServer to be parameter-driven rather than flag-driven, and updated tests accordingly.
1 parent a652202 commit 7951af7

3 files changed

Lines changed: 34 additions & 20 deletions

File tree

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

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -127,17 +127,15 @@ ProcessInfo startCliServer() throws IOException, InterruptedException {
127127
* if connection fails
128128
*/
129129
JsonRpcClient connectToServer(Process process, String tcpHost, Integer tcpPort) throws IOException {
130-
if (options.isUseStdio()) {
131-
if (process == null) {
132-
throw new IllegalStateException("CLI process not started");
133-
}
134-
return JsonRpcClient.fromProcess(process);
135-
} else {
136-
if (tcpHost == null || tcpPort == null) {
137-
throw new IllegalStateException("Cannot connect because TCP host or port are not available");
138-
}
130+
if (tcpHost != null && tcpPort != null) {
131+
// TCP mode: external server or child process with explicit port
139132
Socket socket = new Socket(tcpHost, tcpPort);
140133
return JsonRpcClient.fromSocket(socket);
134+
} else if (process != null) {
135+
// Stdio mode: child process
136+
return JsonRpcClient.fromProcess(process);
137+
} else {
138+
throw new IllegalStateException("Cannot connect: no process for stdio and no host:port for TCP");
141139
}
142140
}
143141

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

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -90,10 +90,15 @@ public CopilotClient() {
9090
public CopilotClient(CopilotClientOptions options) {
9191
this.options = options != null ? options : new CopilotClientOptions();
9292

93-
// Validate mutually exclusive options
93+
// When cliUrl is set, auto-correct useStdio since we're connecting via TCP
94+
if (this.options.getCliUrl() != null && !this.options.getCliUrl().isEmpty()) {
95+
this.options.setUseStdio(false);
96+
}
97+
98+
// Validate mutually exclusive options: cliUrl and cliPath cannot both be set
9499
if (this.options.getCliUrl() != null && !this.options.getCliUrl().isEmpty()
95-
&& (this.options.isUseStdio() || this.options.getCliPath() != null)) {
96-
throw new IllegalArgumentException("CliUrl is mutually exclusive with UseStdio and CliPath");
100+
&& this.options.getCliPath() != null) {
101+
throw new IllegalArgumentException("CliUrl is mutually exclusive with CliPath");
97102
}
98103

99104
// Validate auth options with external server

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

Lines changed: 19 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -96,16 +96,29 @@ void testClientConstructionWithOptions() {
9696
}
9797

9898
@Test
99-
void testCliUrlMutualExclusion() {
99+
void testCliUrlAutoCorrectsUseStdio() {
100100
var options = new CopilotClientOptions().setCliUrl("localhost:3000").setUseStdio(true);
101101

102-
assertThrows(IllegalArgumentException.class, () -> new CopilotClient(options));
102+
// Should NOT throw - useStdio is auto-corrected to false when cliUrl is set
103+
var client = new CopilotClient(options);
104+
assertFalse(options.isUseStdio(), "useStdio should be auto-corrected to false when cliUrl is set");
105+
client.close();
106+
}
107+
108+
@Test
109+
void testCliUrlOnlyConstruction() {
110+
var options = new CopilotClientOptions().setCliUrl("localhost:4321");
111+
112+
// Should work without explicitly setting useStdio to false
113+
var client = new CopilotClient(options);
114+
assertEquals(ConnectionState.DISCONNECTED, client.getState());
115+
assertFalse(options.isUseStdio(), "useStdio should be auto-corrected to false when cliUrl is set");
116+
client.close();
103117
}
104118

105119
@Test
106120
void testCliUrlMutualExclusionWithCliPath() {
107-
var options = new CopilotClientOptions().setCliUrl("localhost:3000").setCliPath("/path/to/cli")
108-
.setUseStdio(false);
121+
var options = new CopilotClientOptions().setCliUrl("localhost:3000").setCliPath("/path/to/cli");
109122

110123
assertThrows(IllegalArgumentException.class, () -> new CopilotClient(options));
111124
}
@@ -194,16 +207,14 @@ void testExplicitUseLoggedInUserTrueWithGithubToken() {
194207

195208
@Test
196209
void testGithubTokenWithCliUrlThrows() {
197-
var options = new CopilotClientOptions().setCliUrl("localhost:8080").setGithubToken("gho_test_token")
198-
.setUseStdio(false);
210+
var options = new CopilotClientOptions().setCliUrl("localhost:8080").setGithubToken("gho_test_token");
199211

200212
assertThrows(IllegalArgumentException.class, () -> new CopilotClient(options));
201213
}
202214

203215
@Test
204216
void testUseLoggedInUserWithCliUrlThrows() {
205-
var options = new CopilotClientOptions().setCliUrl("localhost:8080").setUseLoggedInUser(false)
206-
.setUseStdio(false);
217+
var options = new CopilotClientOptions().setCliUrl("localhost:8080").setUseLoggedInUser(false);
207218

208219
assertThrows(IllegalArgumentException.class, () -> new CopilotClient(options));
209220
}

0 commit comments

Comments
 (0)