Conversation
… and caching (task 4.3) Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
edburns
left a comment
There was a problem hiding this comment.
@copilot Please fix the following issue-requirement gaps:
Resolution order does not match the task and plan
NativeRuntimeLoader.resolve() currently reads copilot-runtime.properties before checking COPILOT_CLI_PATH, and resolveFromCliPath() interprets that variable only as a CLI executable whose sibling may be runtime.node. The task's authoritative plan distinguishes three ordered sources: (1) COPILOT_CLI_PATH as the explicit runtime override, (2) native/<classifier>/runtime.node from the classpath, and (3) runtime.node alongside the bundled CLI. The current implementation collapses sources 1 and 3 and has no separate bundled-CLI fallback.
Fix: Implement and test all three distinct resolution steps in the stated order. Honor a valid explicit override before reading the version properties or performing platform/classpath work, so an override remains usable even when those resources are unavailable. Add tests proving the classpath source wins over the bundled-CLI sibling and that the bundled-CLI sibling is used only when the classpath resource is absent. Surface clear errors for invalid explicit paths rather than silently changing the requested source unless the plan explicitly requires fallback.
Atomic rename behavior is not directly tested
The issue explicitly requires an atomic rename behavior test. The concurrent extraction test exercises a race but does not prove the ATOMIC_MOVE option is used or that an unsupported atomic move produces the required clear failure and cleans up the caller's temp file.
Fix: Add a deterministic test seam around publication and tests for atomic publication/failure cleanup, while preserving Files.move(temp, cached, ATOMIC_MOVE) in production.
There was a problem hiding this comment.
Pull request overview
Adds Java native runtime discovery and atomic extraction for the in-process FFI transport.
Changes:
- Adds versioned
runtime.noderesolution and caching. - Configures Maven-filtered runtime version metadata.
- Adds unit coverage for resolution, caching, and concurrency.
Show a summary per file
| File | Description |
|---|---|
java/sdk/pom.xml |
Enables resource filtering. |
copilot-runtime.properties |
Provides the SDK version cache key. |
NativeRuntimeLoader.java |
Implements native runtime resolution and extraction. |
NativeRuntimeLoaderTest.java |
Tests loader behavior and concurrency. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 6
- Review effort level: Balanced
…rder and add AtomicPublisher test seam Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
Fixed in commit Resolution order — The three distinct sources are now implemented in the required order:
Tests added: Atomic rename test seam — Added
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-SDK Consistency Review ✅This PR adds Finding: No cross-SDK consistency issues. This is purely Java-internal infrastructure. All other SDKs already have equivalent native runtime loading mechanisms:
This PR brings Java to parity with the other SDKs on native runtime loading. No changes are needed in other language implementations.
|
Implements task 4.3 of the Java in-process FFI transport:
NativeRuntimeLoader, which locatesruntime.nodeon the classpath, extracts it atomically to a versioned cache directory, and returns the filesystem path for JNA to load.Changes
java/sdk/pom.xml<resources>block with<filtering>true</filtering>so Maven substitutes${project.version}into the new properties resource at build time.java/sdk/src/main/resources/copilot-runtime.properties(new)version=${project.version}. Maven resource filtering writes the real version;NativeRuntimeLoaderreads it at runtime, giving a stable cache key that works from a JAR and from IDE-runtarget/classes/.java/sdk/src/main/java/com/github/copilot/ffi/NativeRuntimeLoader.java(new)Resolution order:
COPILOT_CLI_PATH— if set, checks forruntime.nodealongside the CLI binary (same directory).native/<classifier>/runtime.node— extracted atomically to~/.copilot/runtime-cache/<version>/<classifier>/runtime.node.Extraction sequence follows the plan's resolved decisions:
Files.createTempFilein the same directory (guarantees same filesystem) →Files.copy→FileChannel.force(true)→Files.move(ATOMIC_MOVE). If another process wins the race, accept the winner after the same regular/non-empty check. If the filesystem rejectsATOMIC_MOVE, fail with a clearIllegalStateException. Temp file is deleted infinally.native/<classifier>/runtime.node, making the loader safe in future uber-jar scenarios where all 8 platform resources coexist on the classpath.java/sdk/src/test/java/com/github/copilot/ffi/NativeRuntimeLoaderTest.java(new)17 unit tests using
@TempDirandURLClassLoaderfor classpath resource injection — no realruntime.nodeneeded. Covers: version resource reading,COPILOT_CLI_PATHpriority, extraction to correct versioned path, cache hit (no re-extraction), missing resource errors, content fidelity, classifier filtering, and concurrent extraction safety (8-thread race withCountDownLatch).