|
| 1 | +# Fix remaining InProcess test parity failures |
| 2 | + |
| 3 | +## Context |
| 4 | + |
| 5 | +Branch: `edburns/review-copilot-pr-2272` (local worktree at `copilot-sdk-01`) |
| 6 | +Push target: `git push upstream HEAD:copilot/edburns1917-java-embed-rust-cli-runtime-post-agent` |
| 7 | + |
| 8 | +The `-Pinprocess` Maven profile sets `COPILOT_SDK_DEFAULT_CONNECTION=inprocess`, which forces all E2E tests to use the InProcess FFI transport instead of subprocess. Most tests now pass. 24 tests still fail in two categories. |
| 9 | + |
| 10 | +## Category 1: Tests that set `cwd` or `cliArgs` on options |
| 11 | + |
| 12 | +These tests go through `ctx.createClient(options)` → `E2ETestContext.applyContextOptions()`. The InProcess branch absorbs `environment` into `InProcessEnvGuard` and nulls it, but does NOT do the same for `cwd` or `cliArgs`. The `CopilotClient` constructor then calls `validateEnvironmentOptions()` which rejects non-null `cwd`/`cliArgs` for InProcess. |
| 13 | + |
| 14 | +**Fix:** In `E2ETestContext.applyContextOptions()`, when InProcess mode is detected, also null out `cwd` and `cliArgs` before constructing the client. For `cwd`, it's meaningless in InProcess (host process cwd is already set). For `cliArgs`, they're subprocess-specific flags. |
| 15 | + |
| 16 | +Location: `java/sdk/src/test/java/com/github/copilot/E2ETestContext.java` lines 354-376 |
| 17 | + |
| 18 | +Current InProcess branch in `applyContextOptions`: |
| 19 | +```java |
| 20 | +if (isInProcessMode(options)) { |
| 21 | + InProcessEnvGuard guard = new InProcessEnvGuard(buildInProcessEnvironment(options)); |
| 22 | + inProcessEnvGuards.add(guard); |
| 23 | + try { |
| 24 | + options.setEnvironment(null); |
| 25 | + return new CopilotClient(options, guard::close); |
| 26 | + } catch (RuntimeException e) { |
| 27 | + guard.close(); |
| 28 | + throw e; |
| 29 | + } |
| 30 | +} |
| 31 | +``` |
| 32 | + |
| 33 | +Needs to also null `cwd` and `cliArgs`: |
| 34 | +```java |
| 35 | +options.setEnvironment(null); |
| 36 | +options.setCwd(null); |
| 37 | +options.setCliArgs(null); |
| 38 | +``` |
| 39 | + |
| 40 | +Affected tests: `PerSessionAuthTest` (sets cwd+environment), possibly others. |
| 41 | + |
| 42 | +## Category 2: StreamingFidelityTest hang |
| 43 | + |
| 44 | +`StreamingFidelityTest.testShouldEmitStreamingDeltasWithReasoningEffortConfigured` hangs indefinitely in InProcess mode. The main thread is blocked on `CompletableFuture.get()` at line 258. The JSON-RPC reader thread is reading from `QueueInputStream` (the InProcess FFI receive stream) but never receives the expected response. |
| 45 | + |
| 46 | +This is a functional issue, not a validation issue. The replay proxy is running (CapiProxy thread is active), but the InProcess transport isn't completing the streaming interaction. |
| 47 | + |
| 48 | +Diagnosis approach: |
| 49 | +1. Check if the test's replay snapshot exists and is correct for streaming |
| 50 | +2. Check if `host_start` succeeds for this test (serverHandle != 0) |
| 51 | +3. jstack showed the reader thread blocked in `QueueInputStream.read()` — no data arriving via the FFI callback |
| 52 | +4. Possible causes: the replay proxy response format doesn't match what the InProcess runtime expects for streaming, or the connection isn't routing correctly through the replay proxy |
| 53 | + |
| 54 | +## Key architectural facts |
| 55 | + |
| 56 | +- `runtime.node` is loaded via JNA. `copilot` CLI binary is spawned as child by `host_start` via `argv[0]`. |
| 57 | +- Both are now bundled in the classifier JAR at `native/<classifier>/runtime.node` and `native/<classifier>/copilot`. |
| 58 | +- `NativeRuntimeLoader.resolve()` extracts both to `~/.copilot/runtime-cache/<version>/<classifier>/`. |
| 59 | +- `NativeRuntimeLoader.resolveEntrypoint()` finds `copilot` alongside `runtime.node`. |
| 60 | +- `CopilotClient.resolveInProcessEntrypoint()` simply calls `NativeRuntimeLoader.resolveEntrypoint().toString()`. |
| 61 | +- `InProcessEnvGuard` uses JNA `libc.setenv()` to mutate the native process env (not visible to `System.getenv()`). |
| 62 | +- The replay proxy (CapiProxy) runs as a Node.js subprocess serving YAML snapshot responses. |
| 63 | + |
| 64 | +## CopilotClientOptions.setEnvironment(null) quirk |
| 65 | + |
| 66 | +`setEnvironment(null)` does NOT set the field to null — it calls `this.environment.clear()`, leaving an empty HashMap. `getEnvironment()` then returns a non-null empty map. The validation now checks `!isEmpty()` too (already fixed). |
| 67 | + |
| 68 | +Similarly, check if `setCwd(null)` / `setCliArgs(null)` have similar behavior. If `setCwd(null)` doesn't actually null the field, the validation might still fire. |
| 69 | + |
| 70 | +## Validation in CopilotClient constructor |
| 71 | + |
| 72 | +```java |
| 73 | +private static void validateEnvironmentOptions(CopilotClientOptions options, RuntimeConnection connection) { |
| 74 | + if (!(connection instanceof InProcessRuntimeConnection)) return; |
| 75 | + rejectInProcessOption("Environment", options.getEnvironment() != null && !options.getEnvironment().isEmpty(), ...); |
| 76 | + rejectInProcessOption("Telemetry", options.getTelemetry() != null, ...); |
| 77 | + rejectInProcessOption("Cwd", options.getCwd() != null, ...); |
| 78 | + rejectInProcessOption("CliArgs", options.getCliArgs() != null && options.getCliArgs().length > 0, ...); |
| 79 | +} |
| 80 | +``` |
| 81 | + |
| 82 | +## resolveDefaultConnection precedence (already fixed) |
| 83 | + |
| 84 | +When `COPILOT_SDK_DEFAULT_CONNECTION=inprocess` but `cliUrl`/`cliPath`/`port` are explicitly set, the explicit options win and subprocess transport is used. Tests like `McpAuthInterestRegistrationTest` that create `new CopilotClient(options.setCliUrl(...))` directly now correctly bypass InProcess. |
| 85 | + |
| 86 | +## Full list of 24 failing test methods |
| 87 | + |
| 88 | +``` |
| 89 | +ByokBearerTokenProviderE2ETest (3 methods) |
| 90 | +CopilotRequestCancelErrorE2ETest (2) |
| 91 | +CopilotRequestHandlerE2ETest (2) |
| 92 | +CopilotRequestSessionIdE2ETest (1) |
| 93 | +GitHubTelemetryTest (2) |
| 94 | +McpAuthInterestRegistrationTest (3) |
| 95 | +ModeHandlersTest (2) |
| 96 | +PerSessionAuthTest (3) |
| 97 | +ProviderEndpointE2ETest (2) |
| 98 | +RpcServerE2ETest (1 - testShouldAddSecretFilterValues — NOW PASSES) |
| 99 | +SessionConfigE2ETest (2) |
| 100 | +StreamingFidelityTest (1 - hangs) |
| 101 | +SubagentHooksE2ETest (1) |
| 102 | +``` |
| 103 | + |
| 104 | +## Commands |
| 105 | + |
| 106 | +```bash |
| 107 | +# Run all tests with InProcess |
| 108 | +cd java && mvn clean verify -Pinprocess |
| 109 | + |
| 110 | +# Run specific failing tests |
| 111 | +COPILOT_SDK_DEFAULT_CONNECTION=inprocess mvn test -pl sdk -Dtest="PerSessionAuthTest,StreamingFidelityTest" -DfailIfNoTests=false |
| 112 | + |
| 113 | +# Format before commit |
| 114 | +mvn spotless:apply |
| 115 | + |
| 116 | +# Push |
| 117 | +git push upstream HEAD:copilot/edburns1917-java-embed-rust-cli-runtime-post-agent |
| 118 | +``` |
| 119 | + |
| 120 | +## Java env bootstrap (required before any mvn/java command) |
| 121 | +```bash |
| 122 | +export JAVA_HOME="/usr/lib/jvm/msopenjdk-25-amd64" |
| 123 | +export M2_HOME="${HOME}/Downloads/apache-maven-3.9.8" |
| 124 | +export PATH="${M2_HOME}/bin:${JAVA_HOME}/bin:${PATH}" |
| 125 | +``` |
0 commit comments