Skip to content

Commit 9fe28a8

Browse files
edburnsCopilot
andcommitted
Fix Java in-process test lifecycle and parity
Prevent the Java in-process test profile from corrupting Surefire control streams or poisoning later tests through the runtime's process-global LLM provider registration. Preserve explicit subprocess and TCP transport choices, sanitize process-only options before constructing in-process clients, and run request-handler tests over their required isolated stdio runtime. Fix the remaining test-contract issues by making fake socket RPC handler registration atomic with reader startup, honoring the configured CLI entrypoint when runtime.node is in a prebuilds directory, and isolating the streaming model-cache scenario. Remove in-process skip annotations from tests that already exercise an explicit subprocess transport. The complete `mvn clean verify -Pinprocess` run now finishes successfully without hangs, transport timeouts, provider-ownership failures, or Surefire stream corruption. File-by-file manifest: - `java/sdk/pom.xml`: use Surefire's TCP fork channel for unit and integration tests so native runtime output cannot corrupt Maven's process-pipe protocol. - `java/sdk/src/main/java/com/github/copilot/CopilotClient.java`: preserve explicitly selected TCP options when the default connection environment requests in-process transport. - `java/sdk/src/main/java/com/github/copilot/JsonRpcClient.java`: add a socket construction hook that registers handlers before the reader thread starts. - `java/sdk/src/main/java/com/github/copilot/ffi/NativeRuntimeLoader.java`: resolve the configured Copilot executable separately from runtime.node when the native library uses the package's prebuilds layout. - `java/sdk/src/main/java/com/github/copilot/rpc/CopilotClientOptions.java`: allow `setCwd(null)` to clear a previously configured working directory. - `java/sdk/src/test/java/com/github/copilot/ClientOptionsE2ETest.java`: run explicit fake-stdio option forwarding tests under the in-process profile. - `java/sdk/src/test/java/com/github/copilot/ConfigCloneTest.java`: cover clearing a configured working directory. - `java/sdk/src/test/java/com/github/copilot/CopilotClientTest.java`: remove obsolete in-process skips from explicit subprocess and TCP lifecycle tests. - `java/sdk/src/test/java/com/github/copilot/CopilotClientTransportTest.java`: test explicit transport precedence, in-process option sanitization, and TCP token selection under the profile default. - `java/sdk/src/test/java/com/github/copilot/CopilotRequestTestSupport.java`: explicitly select stdio for request-handler tests that register the process-global LLM inference provider. - `java/sdk/src/test/java/com/github/copilot/E2ETestContext.java`: honor explicit transports, route request-handler clients to subprocess isolation, and clear environment, cwd, and CLI arguments before in-process client construction. - `java/sdk/src/test/java/com/github/copilot/GitHubTelemetryTest.java`: register fake runtime RPC handlers before socket message processing begins. - `java/sdk/src/test/java/com/github/copilot/MetadataApiTest.java`: run explicit stdio metadata tests instead of skipping them under the profile. - `java/sdk/src/test/java/com/github/copilot/PerSessionAuthTest.java`: run the explicit subprocess unauthenticated case under the profile. - `java/sdk/src/test/java/com/github/copilot/RpcServerMiscE2ETest.java`: run the explicit subprocess account lifecycle case under the profile. - `java/sdk/src/test/java/com/github/copilot/StreamingFidelityTest.java`: give the gpt-5.4 reasoning/streaming scenario an isolated proxy and runtime model cache. - `java/sdk/src/test/java/com/github/copilot/ffi/FfiRuntimeHostTest.java`: cover failed connection-open cleanup followed by successful sequential startup. - `java/sdk/src/test/java/com/github/copilot/ffi/NativeRuntimeLoaderTest.java`: cover resolving a configured CLI beside a prebuilds runtime. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e03c4e94-97b0-41ad-9f4e-c01633dc0bf7
1 parent b78e74e commit 9fe28a8

19 files changed

Lines changed: 339 additions & 90 deletions
Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
# Prompt: make the Java InProcess test run clean
2+
3+
You are working in `/home/edburns/workareas/copilot-sdk-01`, branch
4+
`edburns/review-copilot-pr-2272`. Read these files first:
5+
6+
- `1917-java-embed-rust-cli-runtime-remove-before-merge/post-agentic-01-test-parity-fix-remaining-tests.md`
7+
- `java/20260807-0145-job-logs.txt`
8+
- the current git diff and the Java test/runtime/harness sources
9+
10+
The target command is:
11+
12+
```bash
13+
cd java
14+
mvn clean verify -Pinprocess
15+
```
16+
17+
Make the implementation and test changes necessary for a genuinely clean,
18+
non-hanging run. Do not solve this by broadly skipping tests, increasing
19+
timeouts, weakening assertions, or hiding errors. Preserve the negative-test
20+
assertions; expected negative cases may be logged, but they must not be
21+
reported as test errors.
22+
23+
## What the interrupted log establishes
24+
25+
The run was interrupted after more than an hour; it has no `BUILD SUCCESS`.
26+
There are 88 errors in 20 suites. The failures are highly clustered:
27+
28+
- `std/in stream corrupted` appears during `AskUserTest`.
29+
- `ByokBearerTokenProviderE2ETest` has the expected fake 404 in one negative
30+
case, but the other two tests fail because
31+
`llmInference.setProvider` says “Another client is already the LLM inference
32+
provider.”
33+
- The same provider-ownership error breaks
34+
`CopilotRequestCancelErrorE2ETest`, `CopilotRequestHandlerE2ETest`,
35+
`SessionConfigE2ETest`, and other provider/handler tests.
36+
- `CompactionTest`, `CopilotSessionTest`, `ErrorHandlingTest`,
37+
`EventFidelityTest`, `ExecutorWiringTest`, `HooksTest`, `McpAndAgentsTest`,
38+
`ModeHandlersTest`, `MultiProviderRegistryE2ETest`, `PermissionsTest`,
39+
`PreMcpToolCallHookTest`, `RpcSessionStateExtrasE2ETest`,
40+
`SessionConfigE2ETest`, and `SessionEventsE2ETest` contain repeated
41+
approximately 60-second `sendAndWait`/future timeouts.
42+
- `GitHubTelemetryTest` fails immediately because an InProcess connection
43+
receives `Method not found: connect` and `Method not found: ping`; determine
44+
whether this test must explicitly use the subprocess/socket transport or
45+
whether the InProcess endpoint is missing required handlers.
46+
- `RpcServerE2ETest` has a 30-second RPC timeout and
47+
`RpcSessionStateExtrasE2ETest` has a 60-second timeout.
48+
- `PerSessionAuthTest` has one skipped test and a negative 401 “Bad
49+
credentials” trace. The test itself is not an error.
50+
- `ClientOptionsE2ETest` skips all three tests. Other suites also report
51+
intentional-looking skips: `CopilotClientTest` (14),
52+
`CopilotClientTransportTest` (4), `MetadataApiTest` (3),
53+
`RpcServerMiscE2ETest` (1), and `CompactionTest` (1).
54+
- Many stack traces in `CreateSessionReKeyEntryTest`, `JsonRpcClientTest`,
55+
`LifecycleEventManagerTest`, `RpcHandlerDispatcherTest`, and
56+
`SessionHandlerTest` are deliberately generated negative-test traces and
57+
are followed by passing summaries. Do not misclassify them as failures.
58+
59+
## Priority 1: stop stream corruption and fix InProcess ownership/lifecycle
60+
61+
Investigate `std/in stream corrupted` first. Trace every process and stream
62+
created by the InProcess FFI path, `host_start`, the bundled `copilot`
63+
entrypoint, `NativeRuntimeLoader`, `InProcessRuntimeConnection`, `CapiProxy`,
64+
and Surefire. Identify which native/child process is writing bytes to the
65+
Surefire-controlled stdout/stdin protocol. Ensure child stdout/stderr are
66+
consumed or redirected in the same way as the supported transport and that
67+
the FFI receive/send streams are not closed or reused by another client.
68+
Do not merely suppress Surefire output.
69+
70+
Then fix the “Another client is already the LLM inference provider” root
71+
cause. Determine whether clients, native hosts, provider registrations, or
72+
`InProcessEnvGuard` instances survive test teardown. Verify the close path on
73+
both successful and failed `start()`, failed `createSession()`, and failed
74+
requests. Ensure a failed startup cannot leave a provider registered and that
75+
each test context closes its client/proxy/runtime deterministically. If the
76+
InProcess runtime is process-global, serialize or otherwise coordinate provider
77+
ownership rather than allowing overlapping providers. Add focused regression
78+
coverage for failed-start cleanup and sequential client startup.
79+
80+
The earlier context notes that `E2ETestContext.applyContextOptions()` must
81+
clear InProcess-incompatible `cwd` and `cliArgs` in addition to `environment`.
82+
Implement that carefully, and verify the actual setter semantics:
83+
`setEnvironment(null)` clears to an empty map, while `setCwd(null)` and
84+
`setCliArgs(null)` must be checked rather than assumed. Add or update tests so
85+
the options are truly absent according to constructor validation.
86+
87+
## Priority 2: isolate and repair the common timeout
88+
89+
After Priority 1, run small, serial selectors, not the full suite:
90+
91+
```bash
92+
cd java
93+
COPILOT_SDK_DEFAULT_CONNECTION=inprocess mvn test -pl sdk \
94+
-Dtest="AskUserTest,ByokBearerTokenProviderE2ETest,CopilotSessionTest" \
95+
-DfailIfNoTests=false
96+
```
97+
98+
Use a bounded shell timeout while debugging so a regression cannot consume an
99+
hour. For any remaining timeout, capture a thread dump and inspect the
100+
corresponding Surefire report plus replay-proxy output. Follow one request
101+
from Java JSON-RPC send, through the FFI callback/`QueueInputStream`, into the
102+
replay proxy, and back to the Java reader. Confirm that:
103+
104+
1. `host_start` returns a valid handle and the child `copilot` entrypoint is
105+
reachable.
106+
2. The request reaches the proxy with the expected snapshot.
107+
3. Every response/event is framed correctly and enqueued to the receive
108+
stream.
109+
4. stream completion/EOF and client close wake blocked readers.
110+
5. callbacks do not depend on a thread or executor that has already shut down.
111+
112+
Use `StreamingFidelityTest.testShouldEmitStreamingDeltasWithReasoningEffortConfigured`
113+
as the minimal streaming reproducer, but also test one ordinary
114+
`CopilotSessionTest` request. Do not patch each timed-out suite individually;
115+
the repeated 60-second failures indicate a shared transport or lifecycle
116+
defect. Once the common path works, rerun representative handler, hook,
117+
permission, event, session-config, MCP, and RPC-server selectors and only
118+
then the complete profile.
119+
120+
`GitHubTelemetryTest` is a separate transport-contract issue: inspect its
121+
test setup and the supported connection mode. If it intentionally uses a
122+
minimal fake RPC peer that only supports telemetry, make it explicitly select
123+
that transport so the global InProcess profile cannot route it to a runtime
124+
without `connect`/`ping`. If InProcess is intended, implement the missing
125+
protocol surface and add focused coverage.
126+
127+
## Priority 3: remove unjustified skips
128+
129+
Audit every skipped test in the log and the associated assumptions. For each:
130+
131+
- make it run under InProcess when the behavior is transport-independent;
132+
- explicitly force subprocess/socket transport when the test is specifically
133+
validating subprocess-only options or protocol behavior; or
134+
- change the test setup so the same public behavior is exercised through
135+
InProcess.
136+
137+
Do not add a profile-wide exclusion and do not convert skipped tests to
138+
passing assertions. In particular, investigate all three
139+
`ClientOptionsE2ETest` skips, the `PerSessionAuthTest` skip, and the skips in
140+
`CopilotClientTest`, `CopilotClientTransportTest`, `MetadataApiTest`,
141+
`RpcServerMiscE2ETest`, and `CompactionTest`. The final profile run should
142+
have zero skips unless a test is demonstrably impossible on the platform and
143+
the repository’s existing policy explicitly permits it; document any
144+
remaining exception in the test source.
145+
146+
## Priority 4: make expected negative output intentional
147+
148+
Do not alter assertions for negative tests. After all tests pass, reduce noisy
149+
expected stack-trace logging only where the repository’s logging conventions
150+
support it: distinguish expected test-triggered failures from unexpected
151+
transport failures, and avoid logging full stack traces at warning/error for
152+
the expected path if that can be done without hiding real failures. The
153+
`fake byok endpoint`, `401 Bad credentials`, `session.resume` not-found,
154+
handler exceptions, malformed JSON, socket-close, and re-key traces must
155+
remain asserted and diagnosable.
156+
157+
## Validation and completion criteria
158+
159+
Use the repository’s normal Java bootstrap and Maven logging conventions.
160+
Format Java changes with `mvn spotless:apply` from `java`. Run focused tests
161+
after each root-cause fix, then:
162+
163+
```bash
164+
cd java
165+
mvn clean verify -Pinprocess
166+
```
167+
168+
The task is complete only when this command terminates normally with
169+
`BUILD SUCCESS`, all test suites report zero failures and zero errors, no
170+
test hangs or 60-second transport timeouts occur, no Surefire stream
171+
corruption occurs, and the skip count is zero or each explicitly justified
172+
platform exception is documented and approved by the existing test policy.

java/sdk/pom.xml

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -243,6 +243,7 @@
243243
</execution>
244244
</executions>
245245
<configuration>
246+
<forkNode implementation="org.apache.maven.plugin.surefire.extensions.SurefireForkNodeFactory" />
246247
<systemPropertyVariables>
247248
<project.build.directory>${project.build.directory}</project.build.directory>
248249
<project.build.finalName>${project.build.finalName}</project.build.finalName>
@@ -264,6 +265,7 @@
264265
<artifactId>maven-surefire-plugin</artifactId>
265266
<configuration>
266267
<runOrder>alphabetical</runOrder>
268+
<forkNode implementation="org.apache.maven.plugin.surefire.extensions.SurefireForkNodeFactory" />
267269
<!-- Inject JaCoCo agent + any JDK-version-specific flags -->
268270
<argLine>${testExecutionAgentArgs} ${surefire.jvm.args} --add-opens com.github.copilot.java/com.github.copilot.e2e=ALL-UNNAMED</argLine>
269271
<!--

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,9 @@ static RuntimeConnection resolveDefaultConnection(CopilotClientOptions options,
269269
if (options.getPort() != 0) {
270270
return inferConnectionFromOptions(options);
271271
}
272+
if (!options.isUseStdio() || options.getTcpConnectionToken() != null) {
273+
return inferConnectionFromOptions(options);
274+
}
272275
return RuntimeConnection.forInProcess();
273276
}
274277
if (!"stdio".equalsIgnoreCase(envValue)) {

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818
import java.util.concurrent.Executors;
1919
import java.util.concurrent.atomic.AtomicLong;
2020
import java.util.function.BiConsumer;
21+
import java.util.function.Consumer;
2122
import java.util.logging.Level;
2223
import java.util.logging.Logger;
2324

@@ -59,6 +60,11 @@ private JsonRpcClient(InputStream inputStream, OutputStream outputStream, Socket
5960

6061
private JsonRpcClient(InputStream inputStream, OutputStream outputStream, Socket socket, Process process,
6162
boolean ownsStreams) {
63+
this(inputStream, outputStream, socket, process, ownsStreams, null);
64+
}
65+
66+
private JsonRpcClient(InputStream inputStream, OutputStream outputStream, Socket socket, Process process,
67+
boolean ownsStreams, Consumer<JsonRpcClient> initializer) {
6268
this.inputStream = inputStream;
6369
this.outputStream = outputStream;
6470
this.socket = socket;
@@ -69,6 +75,9 @@ private JsonRpcClient(InputStream inputStream, OutputStream outputStream, Socket
6975
t.setDaemon(true);
7076
return t;
7177
});
78+
if (initializer != null) {
79+
initializer.accept(this);
80+
}
7281
startReader();
7382
}
7483

@@ -100,6 +109,10 @@ public static JsonRpcClient fromSocket(Socket socket) throws IOException {
100109
return new JsonRpcClient(socket.getInputStream(), socket.getOutputStream(), socket, null);
101110
}
102111

112+
static JsonRpcClient fromSocket(Socket socket, Consumer<JsonRpcClient> initializer) throws IOException {
113+
return new JsonRpcClient(socket.getInputStream(), socket.getOutputStream(), socket, null, false, initializer);
114+
}
115+
103116
/**
104117
* Creates a JSON-RPC client over arbitrary input/output streams. The client
105118
* takes ownership of the streams and closes them when {@link #close()} is

java/sdk/src/main/java/com/github/copilot/ffi/NativeRuntimeLoader.java

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,19 @@ public static Path resolve() throws IOException {
135135
* if the CLI executable cannot be located
136136
*/
137137
public static Path resolveEntrypoint() throws IOException {
138-
Path runtimePath = resolve();
138+
String configuredCli = System.getenv(COPILOT_CLI_PATH_ENV);
139+
return resolveEntrypoint(configuredCli, resolve());
140+
}
141+
142+
static Path resolveEntrypoint(String configuredCli, Path runtimePath) throws IOException {
143+
if (configuredCli != null && !configuredCli.isBlank()) {
144+
Path configuredPath = Path.of(configuredCli).toAbsolutePath().normalize();
145+
if (resolveFromCliPath(configuredCli) != null && Files.isRegularFile(configuredPath)
146+
&& Files.size(configuredPath) > 0) {
147+
return configuredPath;
148+
}
149+
}
150+
139151
Path parent = runtimePath.getParent();
140152
String cliName = isWindows() ? CLI_FILENAME_WINDOWS : CLI_FILENAME;
141153
Path cliPath = parent.resolve(cliName);

java/sdk/src/main/java/com/github/copilot/rpc/CopilotClientOptions.java

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -282,13 +282,11 @@ public String getCwd() {
282282
* Sets the working directory for the CLI process.
283283
*
284284
* @param cwd
285-
* the working directory path (must not be {@code null} or empty)
285+
* the working directory path, or {@code null} to clear
286286
* @return this options instance for method chaining
287-
* @throws IllegalArgumentException
288-
* if {@code cwd} is {@code null} or empty
289287
*/
290288
public CopilotClientOptions setCwd(String cwd) {
291-
this.cwd = Objects.requireNonNull(cwd, "cwd must not be null");
289+
this.cwd = cwd;
292290
return this;
293291
}
294292

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

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,6 @@
1515

1616
import org.junit.jupiter.api.Test;
1717

18-
import com.github.copilot.e2e.SkipInProcess;
19-
2018
import com.fasterxml.jackson.databind.JsonNode;
2119
import com.fasterxml.jackson.databind.ObjectMapper;
2220
import com.github.copilot.generated.rpc.SessionLimitsConfig;
@@ -31,7 +29,6 @@ class ClientOptionsE2ETest {
3129
private static final ObjectMapper MAPPER = new ObjectMapper();
3230

3331
@Test
34-
@SkipInProcess("Exercises direct CLI argument and working-directory forwarding to a spawned stdio subprocess")
3532
void testShouldForwardAdvancedSessionCreationOptionsToTheCli() throws Exception {
3633
try (var fake = FakeStdioCli.create()) {
3734
var workDir = fake.path("create-work");
@@ -98,7 +95,6 @@ void testShouldForwardAdvancedSessionCreationOptionsToTheCli() throws Exception
9895
}
9996

10097
@Test
101-
@SkipInProcess("Exercises direct CLI argument and working-directory forwarding to a spawned stdio subprocess")
10298
void testShouldForwardSingularProviderConfigurationOnSessionCreation() throws Exception {
10399
try (var fake = FakeStdioCli.create()) {
104100
try (var client = fake.createClient()) {
@@ -127,7 +123,6 @@ void testShouldForwardSingularProviderConfigurationOnSessionCreation() throws Ex
127123
}
128124

129125
@Test
130-
@SkipInProcess("Exercises direct CLI argument and working-directory forwarding to a spawned stdio subprocess")
131126
void testShouldForwardAdvancedSessionResumeOptionsToTheCli() throws Exception {
132127
try (var fake = FakeStdioCli.create()) {
133128
var workDir = fake.path("resume-work");

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -377,6 +377,15 @@ void copilotClientOptionsSetEnvironmentNullClearsExisting() {
377377
assertTrue(env == null || env.isEmpty());
378378
}
379379

380+
@Test
381+
void copilotClientOptionsSetCwdNullClearsExisting() {
382+
CopilotClientOptions opts = new CopilotClientOptions().setCwd("/tmp");
383+
384+
opts.setCwd(null);
385+
386+
assertNull(opts.getCwd());
387+
}
388+
380389
@Test
381390
@SuppressWarnings("deprecation")
382391
void copilotClientOptionsDeprecatedGithubToken() {

0 commit comments

Comments
 (0)