Integrate Bruno's PR 1478 with Ed's desired CI/CD changes#1483
Merged
Conversation
This was referenced May 28, 2026
Collaborator
Author
edburns
added a commit
that referenced
this pull request
May 28, 2026
modified: .github/workflows/docs-validation.yml - Use Java 25 per @brunoborges. modified: .github/workflows/java-sdk-tests.yml - Update labels. modified: .vscode/settings.json - Additional Java settings. modified: scripts/docs-validation/package.json The `--lang` parsing in validate.ts uses `args.find((a) => a.startsWith("--lang="))`, which won't match `--lang typescript` (space-separated). So `targetLang` is always `undefined`, and every job validates **all** languages. This is a pre-existing bug — but it was invisible before because `mvn install` succeeded with the pre-installed JDK 17 on all runners. However, the reason it matters **now** is: after fixing the `validate-java` job to use JDK 25, the other 4 jobs (TypeScript, Go, Python, C#) still don't have JDK 25. Since they also accidentally validate Java (due to the broken `--lang` filter), they'd continue to fail. So you actually have two options: **Option A:** Only fix JDK in `validate-java` AND fix `--lang` parsing so other jobs stop accidentally validating Java. **Option B:** Only fix JDK in `validate-java` AND add JDK 25 setup to all 4 other jobs too (ugly but works without touching the script). The `--lang` fix is the cleaner path, but it's a separate pre-existing bug, not something introduced by PR #1483. If you'd prefer to keep the changes minimal and just address the PR's breakage, I can revert the package.json change and instead add `setup-java` with JDK 25 to every job. What's your preference?
Contributor
There was a problem hiding this comment.
Pull request overview
Integrates PR #1478 (JDK 25 multi-release JAR with virtual-thread default executor for the Java SDK) together with CI/CD restructuring: builds always run on JDK 25, but tests are also exercised against JDK 17 via a matrix using the already-compiled JAR. Adds executor lifecycle ownership/shutdown logic in CopilotClient, a Failsafe integration test running against the packaged JAR, and Maven enforcer/antrun guards to ensure the MR overlay is present.
Changes:
- Introduce
InternalExecutorProvider(base +java25overlay) and wireCopilotClientto own and shut down its executor when applicable. - Add a Failsafe IT (with a child-JVM probe) and Maven enforcer/antrun checks to validate the packaged MR-JAR end-to-end; bump version to
1.0.0-beta-10-java.0-SNAPSHOT. - Rework
java-sdk-tests.ymlto use JDK 25 for build + a matrix that re-runssurefire:teston JDK 17 against the JDK 25-built classes; bump publish workflows and docs-validation to JDK 25.
Show a summary per file
| File | Description |
|---|---|
| java/src/main/java/com/github/copilot/InternalExecutorProvider.java | New baseline provider (common pool, not owned). |
| java/src/main/java25/com/github/copilot/InternalExecutorProvider.java | MR-25 overlay using virtual-thread executor (owned). |
| java/src/main/java/com/github/copilot/CopilotClient.java | Cache resolved executor, shut it down on close/forceStop, dedicated shutdown dispatcher thread. |
| java/src/main/java/com/github/copilot/rpc/CopilotClientOptions.java | Javadoc updates documenting default-executor policy. |
| java/src/test/java/com/github/copilot/InternalExecutorProviderTest.java | Unit tests for provider + user-executor non-ownership. |
| java/src/test/java/com/github/copilot/InternalExecutorProviderProbe.java | Child-JVM probe printing executor/runtime facts. |
| java/src/test/java/com/github/copilot/InternalExecutorProviderIT.java | Failsafe IT running probe against packaged JAR. |
| java/pom.xml | Version bump, Failsafe plugin, JaCoCo MR exclusion, JDK25 enforcer, java25-multi-release profile and overlay verifier. |
| java/README.md | Document MR-JAR/JDK 25 behavior, dev setup, update versions. |
| .github/workflows/java-sdk-tests.yml | JDK 25 build + JDK 17 test matrix; new CLI/Javadoc steps; per-matrix artifacts. |
| .github/workflows/java-publish-snapshot.yml | Use JDK 25. |
| .github/workflows/java-publish-maven.yml | Use JDK 25. |
| .github/workflows/docs-validation.yml | Use JDK 25 for Java docs validation. |
| .github/actions/java-test-report/action.yml | Configurable summary title. |
| .vscode/settings.json | Enable Java null-analysis. |
| scripts/docs-validation/package.json | Use --lang=… form for validate scripts. |
Copilot's findings
- Files reviewed: 16/16 changed files
- Comments generated: 6
Use a multi-release JAR to select virtual threads as the default internal executor on JDK 25+, while retaining the common pool on older JDKs. Keep user-provided executors caller-owned and only shut down SDK-owned defaults. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…nership and shutdown capability
…executor management
…pdate documentation
…rtion tests Addresses PR #1478 review (Copilot AI): the existing baseProviderUsesCommonPoolWithoutOwnership method bundled three unrelated assertions (commonPool identity, user-executor ownership, package-private visibility). Split into baseProviderReturnsCommonPool, userProvidedExecutorIsNotOwned, and providerIsPackagePrivate so failures point at a single condition each.
…Stop() Addresses PR #1478 review (Copilot AI, discussion r3314987809): CopilotClient#forceStop() chains shutdownOwnedExecutor() onto cleanupConnection() via whenComplete(...), but cleanupConnection() is itself built on async work scheduled on the SDK-owned executor (e.g. CompletableFuture.supplyAsync(..., executor) in connection setup). On JDK 25+ this means the whenComplete lambda can land on one of the owned executor's threads; awaitTermination(...) then blocks waiting for the very thread it is running on, forcing the full AUTOCLOSEABLE_TIMEOUT_SECONDS timeout followed by shutdownNow(). Fix: dispatch the shutdown continuation via whenCompleteAsync(...) onto a private one-shot SHUTDOWN_DISPATCHER that spawns a fresh daemon thread named "copilot-client-shutdown". This guarantees the awaitTermination call is never made from inside the executor it is draining. close() is unaffected: it calls stop().get(...) synchronously and runs shutdownOwnedExecutor() in its finally block on the caller's thread.
Addresses PR #1478 review (Copilot AI, discussion r3314987870): close() and forceStop() can each invoke shutdownOwnedExecutor() (e.g. user calls forceStop() and then close() in try-with-resources). A second call would redundantly invoke shutdown() and awaitTermination() on an already- terminated ExecutorService. While the JDK handles this gracefully (awaitTermination returns immediately after a prior shutdownNow), the redundant call obscures diagnostics. Short-circuit at the top of shutdownOwnedExecutor() when isShutdown() is already true and log at FINE so the second invocation is visible without spamming normal output.
…DK tests across JDK 17 and 25
The java25-multi-release Maven profile is activated only on JDK 25+
(<jdk>[25,)</jdk>). Without it, the build skips the compile-java25 execution,
the packaged JAR has no META-INF/versions/25/InternalExecutorProvider.class,
and the manifest lacks Multi-Release: true. The InternalExecutorProvider
JDK 25 overlay (Executors.newVirtualThreadPerTaskExecutor()) is then
effectively dead in CI and in published Maven Central artifacts -- consumers
on JDK 25+ silently fall back to ForkJoinPool.commonPool().
Changes:
- java-publish-snapshot.yml: set up JDK 25 (was 17). The pom keeps
<maven.compiler.release>17</maven.compiler.release> so baseline bytecode
remains JDK 17 compatible; --release 17 is supported by the JDK 25
compiler.
- java-publish-maven.yml: same JDK bump for release:perform.
- java-sdk-tests.yml: matrix on java-version: [17, 25]. JDK 25 entry
exercises the MR-JAR overlay end-to-end via InternalExecutorProviderIT
(asserts feature >= 25 => canBeShutdown=true, virtual=true) and runs the
new verify-java25-overlay antrun structural guard. Side-effects (site
artifact upload, JaCoCo badge generation, badge-update PR) remain gated to
the JDK 17 entry so the badge source-of-truth stays a single baseline.
Failure artifact name suffixed with -jdk${matrix.java-version} to avoid
collisions.
Branch protection note: the job's check name changes from "Java SDK Tests"
to "Java SDK Tests (JDK 17)" + "Java SDK Tests (JDK 25)". Update branch
protection rules accordingly after merge if required-checks reference the
old name.
modified: .github/workflows/java-sdk-tests.yml - Have `test-jdk17` be a parameter, set to true by default. - Do not use matrix. It is vital that the tests verify that the MR-JAR facility works when compiled using JDK 25 with `maven.compiler.release` set to 17. To that end, this workflow does not re-compile the jar or the tests, but uses a the JDK 17 just to run the tests. - Use a separate banner for each variant. - Use separate summary for 17 and 25 tests. modified: java/pom.xml - Print banners for inspection during tests. - Use Maven enforcer plugin to require using 25 when compiling. modified: java/README.md - Add content for running the tests with 17.
modified: .github/workflows/docs-validation.yml - Use Java 25 per @brunoborges. modified: .github/workflows/java-sdk-tests.yml - Update labels. modified: .vscode/settings.json - Additional Java settings. modified: scripts/docs-validation/package.json The `--lang` parsing in validate.ts uses `args.find((a) => a.startsWith("--lang="))`, which won't match `--lang typescript` (space-separated). So `targetLang` is always `undefined`, and every job validates **all** languages. This is a pre-existing bug — but it was invisible before because `mvn install` succeeded with the pre-installed JDK 17 on all runners. However, the reason it matters **now** is: after fixing the `validate-java` job to use JDK 25, the other 4 jobs (TypeScript, Go, Python, C#) still don't have JDK 25. Since they also accidentally validate Java (due to the broken `--lang` filter), they'd continue to fail. So you actually have two options: **Option A:** Only fix JDK in `validate-java` AND fix `--lang` parsing so other jobs stop accidentally validating Java. **Option B:** Only fix JDK in `validate-java` AND add JDK 25 setup to all 4 other jobs too (ugly but works without touching the script). The `--lang` fix is the cleaner path, but it's a separate pre-existing bug, not something introduced by PR #1483. If you'd prefer to keep the changes minimal and just address the PR's breakage, I can revert the package.json change and instead add `setup-java` with JDK 25 to every job. What's your preference?
modified: .github/actions/java-test-report/action.yml modified: .github/workflows/java-sdk-tests.yml - Ensure the correct Copilot CLI is used for tests.
modified: .github/workflows/java-sdk-tests.yml Now the JDK 17 run: 1. `jacoco:prepare-agent@wire-up-coverage-instrumentation` — generates the JaCoCo agent arg compatible with JDK 17 and sets `testExecutionAgentArgs` 2. `antrun:run@print-test-jdk-banner` — prints the JDK banner 3. `surefire:test` — runs pre-compiled tests with the JaCoCo agent attached 4. `jacoco:report@build-coverage-report-from-tests` — generates the HTML/XML/CSV reports The `-DtestExecutionAgentArgs=` override is removed so JaCoCo's prepared value flows through to Surefire's `<argLine>`. The test report action already reads from the default jacoco.xml path, so the coverage section will appear in both reports automatically.
…omments. modified: .github/actions/java-test-report/action.yml - Add failsafe tests to the report. modified: .github/workflows/java-sdk-tests.yml - Ensure failsafe tests are invoked in the 17 case. modified: java/README.md - Fix spelling error. - Fix artifact version error. - Fix 17 invocation.
edburns
force-pushed
the
edburns/review-brunoborges-pr-1478
branch
from
91cee47 to
159e8c9
Compare
edburns
added a commit
to github/copilot-sdk-java
that referenced
this pull request
May 28, 2026
edburns
added a commit
to edburns/copilot-sdk-java
that referenced
this pull request
May 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #1478 .