Skip to content

Commit 46a9546

Browse files
committed
Fix 6 LLM-friendliness inconsistencies in Phase 4 implementation steps
- Remove FfiTransportException from 4.3 (superseded by 3.10 resolution) - Rewrite 4.5 to use RuntimeConnection hierarchy (not rejected Transport enum) - Fix 4.7 file path to post-restructure java/sdk/src/ location - Add path note at 4.1 explaining src/ vs sdk/src/ relationship with 4.6a - Expand --no-auto-update invariant in 3.9 with ABI-skew rationale - Fix NativeSize to com.sun.jna.NativeLong in FfiOutputStream example
1 parent fbb2e9f commit 46a9546

1 file changed

Lines changed: 25 additions & 20 deletions

File tree

1917-java-embed-rust-cli-runtime-remove-before-merge/1917-embed-cli-runtime-ignorance-reduction-plan.md

Lines changed: 25 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -390,7 +390,7 @@ class FfiOutputStream extends OutputStream {
390390
@Override
391391
public void write(byte[] b, int off, int len) throws IOException {
392392
byte[] slice = (off == 0 && len == b.length) ? b : Arrays.copyOfRange(b, off, off + len);
393-
if (!lib.copilot_runtime_connection_write(connectionId, slice, new NativeSize(len))) {
393+
if (!lib.copilot_runtime_connection_write(connectionId, slice, new com.sun.jna.NativeLong(len))) {
394394
throw new IOException("copilot_runtime_connection_write failed");
395395
}
396396
}
@@ -631,7 +631,7 @@ Complete `env_json` key inventory (these are the **only** three keys used across
631631

632632
**Three key invariants:**
633633

634-
1. **`argv_json` must never be null.** It always contains at least `[entrypoint, "--embedded-host", "--no-auto-update"]`.
634+
1. **`argv_json` must never be null.** It always contains at least `[entrypoint, "--embedded-host", "--no-auto-update"]`. **`--no-auto-update` is mandatory** — it pins the worker to the bundled cdylib version, preventing ABI skew between the loaded library and the runtime worker. Omitting it allows the runtime to drift to a newer `~/.copilot/pkg` version whose ABI may be incompatible with the loaded cdylib.
635635
2. **`env_json` can be null** (with `env_json_len = 0`) when no environment overrides are needed.
636636
3. **All three metadata buffers (`ext_source`, `ext_name`, `conn_token`) are always null/0.** No current SDK uses them; they are reserved extension points.
637637

@@ -940,7 +940,7 @@ Every implementation step in this phase **must** follow this test-driven workflo
940940
1. **Write tests first.** Before writing or modifying production code for a step, write the unit tests (and integration tests where specified) that define the expected behavior. Tests should initially fail (red).
941941

942942
The test native library from `spike-3-4-jna-callback-and-threading/rust-dll/` is the test fixture for steps 4.3 and 4.4. Build it once with `cargo build --release` for the current OS and architecture and place the output at a known path before writing Java tests.
943-
943+
944944
2. **Implement until green.** Write the minimum production code to make all tests pass.
945945
3. **Refactor.** Clean up the implementation while keeping tests green. Run `mvn spotless:apply` to ensure formatting compliance.
946946
4. **Gate before proceeding.** All tests from the current step **and all prior steps** must pass (`mvn verify`) before moving to the next step. Do not proceed with a step if any prior step's tests are broken.
@@ -956,6 +956,8 @@ Every implementation step in this phase **must** follow this test-driven workflo
956956

957957
**What:** `PlatformDetector` class that determines `os`, `arch`, `libc` and produces the classifier string.
958958

959+
> **Path note:** Steps 4.1–4.5 list file paths as `java/src/...`. After step 4.6a (reactor restructure), these become `java/sdk/src/...`. If 4.6a is performed first, use the `java/sdk/src/...` paths. If 4.6a is performed after, the files will be moved during 4.6a.
960+
959961
**Files to create:**
960962

961963
- `java/src/main/java/com/github/copilot/ffi/PlatformDetector.java`
@@ -978,11 +980,11 @@ Every implementation step in this phase **must** follow this test-driven workflo
978980

979981
- `java/src/test/java/com/github/copilot/ffi/NativeRuntimeLoaderTest.java`
980982

981-
**Gating criteria:**
983+
**Gating criteria:**
982984

983985
- Extracts binary to `~/.copilot/runtime-cache/<version>/<classifier>/runtime.node`. Handles concurrent extraction safely.
984986

985-
- When *multiple* platform JARs are on the classpath (uber-jar scenario), it sorts candidates and picks the best match. The plan's `NativeRuntimeLoader` should handle this case — in the `copilot-native-all` uber-JAR, all 8 `native/<classifier>/runtime.node` resources exist on the classpath simultaneously. The loader must filter by the detected classifier, not just grab the first `runtime.node` it finds. ❌❌❌We are not doing the uber-jar approach now, but we want to do it in the future, so we must be ready for it.❌❌❌
987+
- When _multiple_ platform JARs are on the classpath (uber-jar scenario), it sorts candidates and picks the best match. The plan's `NativeRuntimeLoader` should handle this case — in the `copilot-native-all` uber-JAR, all 8 `native/<classifier>/runtime.node` resources exist on the classpath simultaneously. The loader must filter by the detected classifier, not just grab the first `runtime.node` it finds. ❌❌❌We are not doing the uber-jar approach now, but we want to do it in the future, so we must be ready for it.❌❌❌
986988

987989
### 4.3 — JNA binding interface and implementation
988990

@@ -993,17 +995,16 @@ Every implementation step in this phase **must** follow this test-driven workflo
993995
- `java/src/main/java/com/github/copilot/ffi/NativeBinding.java`
994996
- `java/src/main/java/com/github/copilot/ffi/JnaNativeBinding.java`
995997
- `java/src/main/java/com/github/copilot/ffi/OutboundCallback.java`
996-
- `java/src/main/java/com/github/copilot/ffi/FfiTransportException.java`
997998

998999
**Tests:** Unit tests using a test native library with minimal C ABI (or mock/spy on JNA calls).
9991000

10001001
- `java/src/test/java/com/github/copilot/ffi/JnaNativeBindingTest.java`
10011002

1002-
**Gating criteria:**
1003+
**Gating criteria:**
10031004

1004-
- Can load a native library, call functions, receive callbacks. Error cases wrapped in `FfiTransportException`.
1005+
- Can load a native library, call functions, receive callbacks. Error cases throw `IllegalStateException` (see 3.10 resolution — no dedicated `FfiTransportException`).
10051006

1006-
- **Library-never-unloads pattern** — the loaded native handle must be held in a `static` field and never released. JNA caches by library name, but the plan should make this explicit since native worker threads outlive any `FfiRuntimeHost` instance. See Rust `OnceLock<Mutex<HashMap<PathBuf, &'static Library>>>` + `Box::leak()` Missing this risks a crash if a second `FfiRuntimeHost` is created after the first is closed.
1007+
- **Library-never-unloads pattern** — the loaded native handle must be held in a `static` field and never released. JNA caches by library name, but the plan should make this explicit since native worker threads outlive any `FfiRuntimeHost` instance. See Rust `OnceLock<Mutex<HashMap<PathBuf, &'static Library>>>` + `Box::leak()` Missing this risks a crash if a second `FfiRuntimeHost` is created after the first is closed.
10071008

10081009
### 4.4 — FFI runtime host and transport streams
10091010

@@ -1023,34 +1024,38 @@ Every implementation step in this phase **must** follow this test-driven workflo
10231024

10241025
- **Callback `closing` flag early-exit** — the `on_outbound` callback must check a `closing` flag and return immediately without enqueuing data. Without this, the shutdown drain may never converge. Both .NET and Rust set this flag before `connection_close`. Failing to do this can caus a hang on shutdown.
10251026

1026-
- **Operation lock for concurrent write/close safety**`FfiOutputStream.write()` can race with `FfiRuntimeHost.close()`. See how the Rust SDK uses a `parking_lot::Mutex` (`operation_lock`). See the Rust SDK `FfiShared`. Failing to do this can cause a data race during shutdown.
1027+
- **Operation lock for concurrent write/close safety**`FfiOutputStream.write()` can race with `FfiRuntimeHost.close()`. See how the Rust SDK uses a `parking_lot::Mutex` (`operation_lock`). See the Rust SDK `FfiShared`. Failing to do this can cause a data race during shutdown.
10271028

1028-
- **`Connection` record needs `FfiRuntimeHost` field** — the current `CopilotClient.Connection` record has `(JsonRpcClient rpc, Process process, ServerRpc serverRpc)`. InProcess has no `Process`. Without an `ffiHost` field, `stop()` and `forceStop()` can't call `ffiHost.close()`. .NET's `Connection` record includes `FfiRuntimeHost? ffiHost`. Failure to do this can cause a leak of native resources on shutdown.
1029+
- **`Connection` record needs `FfiRuntimeHost` field** — the current `CopilotClient.Connection` record has `(JsonRpcClient rpc, Process process, ServerRpc serverRpc)`. InProcess has no `Process`. Without an `ffiHost` field, `stop()` and `forceStop()` can't call `ffiHost.close()`. .NET's `Connection` record includes `FfiRuntimeHost? ffiHost`. Failure to do this can cause a leak of native resources on shutdown.
10291030

10301031
### 4.5 — Transport integration with `CopilotClient`
10311032

1032-
**What:** `Transport` enum, `setTransport()` on `CopilotClientOptions`, InProcess code path in `CopilotClient` that uses `FfiRuntimeHost` instead of `CliServerManager`.
1033+
**What:** `RuntimeConnection` sealed class hierarchy (see 3.5.1 resolution), `setConnection()` on `CopilotClientOptions`, InProcess code path in `CopilotClient` that uses `FfiRuntimeHost` instead of `CliServerManager`. **Do NOT create a `Transport` enum or `setTransport()` method — that approach was explicitly rejected in the 3.5.1 resolution in favor of the `RuntimeConnection` type hierarchy.**
10331034

1034-
✅✅Remember to handle **`COPILOT_SDK_DEFAULT_CONNECTION` env var resolution in `CopilotClient` constructor**. `CopilotClient` must implement `resolveDefaultConnection()` when no `connection` is set. See NET Client.cs — search for `ResolveDefaultConnection` (private static method) and its caller `_options.Connection ?? ResolveDefaultConnection(_options)`; Rust lib.rs — search for `fn resolve_default_transport` and constant `DEFAULT_CONNECTION_ENV_VAR`.
1035+
✅✅Remember to handle **`COPILOT_SDK_DEFAULT_CONNECTION` env var resolution in `CopilotClient` constructor**. `CopilotClient` must implement `resolveDefaultConnection()` when no `connection` is set. See .NET `dotnet/src/Client.cs` — search for `ResolveDefaultConnection` (private static method) and its caller `_options.Connection ?? ResolveDefaultConnection(_options)`; Rust `rust/src/lib.rs` — search for `fn resolve_default_transport` and constant `DEFAULT_CONNECTION_ENV_VAR`.
10351036

1036-
✅✅Remember: **`ValidateEnvironmentOptions` — reject incompatible options for InProcess**`environment`, `telemetry`, `workingDirectory`, `extraArgs` must be rejected when InProcess is selected. Without this, users set options that silently do nothing in-process. See .NET Client.cs — search for `ValidateEnvironmentOptions` (private static method, called right after `ResolveDefaultConnection`); Rust lib.rs — search for `fn validate_inprocess_options`.
1037+
✅✅Remember: **`ValidateEnvironmentOptions` — reject incompatible options for InProcess**`environment`, `telemetry`, `workingDirectory`, `extraArgs` must be rejected when InProcess is selected. Without this, users set options that silently do nothing in-process. See .NET `dotnet/src/Client.cs` — search for `ValidateEnvironmentOptions` (private static method, called right after `ResolveDefaultConnection`); Rust `rust/src/lib.rs` — search for `fn validate_inprocess_options`.
10371038

10381039
**Files to modify:**
10391040

1040-
- `java/src/main/java/com/github/copilot/rpc/CopilotClientOptions.java` — add `transport` field
1041-
- `java/src/main/java/com/github/copilot/CopilotClient.java` — InProcess connection path
1041+
- `java/src/main/java/com/github/copilot/rpc/CopilotClientOptions.java` — add `connection` field (type `RuntimeConnection`, nullable, default `null`)
1042+
- `java/src/main/java/com/github/copilot/CopilotClient.java` — InProcess connection path via `RuntimeConnection` dispatch
10421043

10431044
**Files to create:**
10441045

1045-
- `java/src/main/java/com/github/copilot/ffi/Transport.java`
1046+
- `java/src/main/java/com/github/copilot/rpc/RuntimeConnection.java` — sealed class with factory methods (see 3.5.1 resolution)
1047+
- `java/src/main/java/com/github/copilot/rpc/StdioRuntimeConnection.java`
1048+
- `java/src/main/java/com/github/copilot/rpc/TcpRuntimeConnection.java`
1049+
- `java/src/main/java/com/github/copilot/rpc/UriRuntimeConnection.java`
1050+
- `java/src/main/java/com/github/copilot/rpc/InProcessRuntimeConnection.java`
10461051

10471052
**Tests:** Unit test that InProcess transport selection uses `FfiRuntimeHost`.
10481053

1049-
✅✅✅Test the backward-compatibility bridge (legacy fields → `RuntimeConnection` inference) and the `IllegalArgumentException` when both `connection` and legacy fields are set.✅✅✅
1054+
✅✅✅Test the backward-compatibility bridge (legacy fields → `RuntimeConnection` inference) and the `IllegalArgumentException` when both `connection` and legacy fields are set.✅✅✅
10501055

10511056
- `java/src/test/java/com/github/copilot/CopilotClientTransportTest.java`
10521057

1053-
**Gating criteria:** `new CopilotClientOptions().setTransport(Transport.IN_PROCESS)` routes through FFI host. `COPILOT_SDK_DEFAULT_CONNECTION=inprocess` env var works. CLI transport unchanged.
1058+
**Gating criteria:** `new CopilotClientOptions().setConnection(RuntimeConnection.forInProcess())` routes through FFI host. `COPILOT_SDK_DEFAULT_CONNECTION=inprocess` env var works. CLI transport unchanged.
10541059

10551060
### 4.6 — Multi-module reactor restructure and per-platform classifier JARs
10561061

@@ -1125,7 +1130,7 @@ version=${project.version}
11251130

11261131
**Files to create:**
11271132

1128-
- `java/src/test/java/com/github/copilot/e2e/InProcessTransportIT.java`
1133+
- `java/sdk/src/test/java/com/github/copilot/e2e/InProcessTransportIT.java` _(note: `sdk/` prefix — this path is post-4.6a restructure)_
11291134

11301135
**Snapshot files:** Reuse existing snapshots or create new ones as needed.
11311136

0 commit comments

Comments
 (0)