Upstream sync: Add clone() methods to config classes (6 commits)#138
Upstream sync: Add clone() methods to config classes (6 commits)#138brunoborges merged 12 commits intomainfrom
Conversation
Port upstream commit ce54e43: Add copy constructors and Clone methods to the various .NET config option bags (#422) Added clone() methods to: - CopilotClientOptions - SessionConfig - ResumeSessionConfig - MessageOptions These methods create shallow clones where mutable collection properties (lists, maps, arrays) are copied into new instances to ensure independence, while other reference-type properties are shared. Also added comprehensive tests in ConfigCloneTest to verify the clone behavior. Co-authored-by: brunoborges <129743+brunoborges@users.noreply.github.com>
Co-authored-by: brunoborges <129743+brunoborges@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Ports upstream changes to add shallow clone() helpers for SDK configuration “option bag” classes, improving ergonomics when deriving modified configs without mutating the original, and syncs upstream merge tracking.
Changes:
- Added shallow
clone()methods toCopilotClientOptions,SessionConfig,ResumeSessionConfig, andMessageOptions. - Added
ConfigCloneTestto validate cloning semantics. - Clarified Windows console window behavior comment in
CliServerManagerand updated.lastmergeto the new upstream commit.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/com/github/copilot/sdk/json/CopilotClientOptions.java | Adds clone() for client options (arrays copied; other fields currently shared). |
| src/main/java/com/github/copilot/sdk/json/SessionConfig.java | Adds clone() for new-session configuration, copying list/map fields. |
| src/main/java/com/github/copilot/sdk/json/ResumeSessionConfig.java | Adds clone() for resume-session configuration, copying list/map fields. |
| src/main/java/com/github/copilot/sdk/json/MessageOptions.java | Adds clone() for message options, copying attachments list. |
| src/test/java/com/github/copilot/sdk/ConfigCloneTest.java | Introduces unit tests for clone behavior. |
| src/main/java/com/github/copilot/sdk/CliServerManager.java | Adds explanatory comment about Windows console window behavior in Java. |
| .lastmerge | Updates tracked upstream commit hash to 5016587.... |
| original.setCliPath("/usr/local/bin/copilot"); | ||
| original.setLogLevel("debug"); | ||
| original.setPort(9000); | ||
|
|
||
| CopilotClientOptions cloned = original.clone(); | ||
|
|
||
| assertEquals(original.getCliPath(), cloned.getCliPath()); |
There was a problem hiding this comment.
ConfigCloneTest currently includes trailing whitespace on otherwise blank lines (e.g., around this section). Spotless is configured with trimTrailingWhitespace, so this will fail formatting checks in CI; please run mvn spotless:apply or remove the trailing spaces.
There was a problem hiding this comment.
Applied. Spotless formatting has been run to remove trailing whitespace and fix all formatting issues. Commit eb573ef
| void sessionConfigListIndependence() { | ||
| SessionConfig original = new SessionConfig(); | ||
| List<String> toolList = new ArrayList<>(); | ||
| toolList.add("grep"); | ||
| toolList.add("bash"); | ||
| original.setAvailableTools(toolList); | ||
|
|
||
| SessionConfig cloned = original.clone(); | ||
|
|
||
| List<String> clonedTools = new ArrayList<>(cloned.getAvailableTools()); | ||
| clonedTools.add("web"); | ||
| cloned.setAvailableTools(clonedTools); | ||
|
|
||
| assertEquals(2, original.getAvailableTools().size()); | ||
| assertEquals(3, cloned.getAvailableTools().size()); | ||
| } |
There was a problem hiding this comment.
sessionConfigListIndependence() doesn’t currently validate that SessionConfig.clone() actually copies the underlying availableTools list: it builds a new list from the clone and sets it back, which would still pass even if the clone shared the original list. To test clone semantics, mutate the original list instance (toolList) after cloning (or otherwise observe independence) and assert the cloned config is unaffected.
There was a problem hiding this comment.
Fixed test to properly validate list independence by mutating the original list after cloning and verifying the cloned config is unaffected. Commit eb573ef
There was a problem hiding this comment.
Fixed test to properly validate list independence by mutating the original list after cloning and verifying the cloned config is unaffected. Commit 6e2e991
src/main/java/com/github/copilot/sdk/json/ResumeSessionConfig.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * Creates a shallow clone of this {@code CopilotClientOptions} instance. | ||
| * <p> | ||
| * Array properties (like {@code cliArgs}) are copied into new arrays so that | ||
| * modifications to the clone do not affect the original. Other reference-type | ||
| * properties (like {@code environment}) are shared between the original and | ||
| * clone. | ||
| * | ||
| * @return a clone of this options instance | ||
| */ | ||
| public CopilotClientOptions clone() { | ||
| CopilotClientOptions copy = new CopilotClientOptions(); | ||
| copy.cliPath = this.cliPath; | ||
| copy.cliArgs = this.cliArgs != null ? this.cliArgs.clone() : null; | ||
| copy.cwd = this.cwd; | ||
| copy.port = this.port; | ||
| copy.useStdio = this.useStdio; | ||
| copy.cliUrl = this.cliUrl; | ||
| copy.logLevel = this.logLevel; | ||
| copy.autoStart = this.autoStart; | ||
| copy.autoRestart = this.autoRestart; | ||
| copy.environment = this.environment; | ||
| copy.githubToken = this.githubToken; | ||
| copy.useLoggedInUser = this.useLoggedInUser; | ||
| return copy; |
There was a problem hiding this comment.
CopilotClientOptions.clone() copies cliArgs but shares the mutable environment Map. Since getEnvironment()/setEnvironment() expose a modifiable Map and other config clone() methods copy their Map/List fields, sharing this Map makes it easy for changes on the clone to affect the original (and vice versa). Consider cloning the environment map as well (or, if sharing is intentional, document that as an exception and add a test to lock in the behavior).
There was a problem hiding this comment.
Fixed to clone the environment map for collection independence. Updated Javadoc to reflect that the environment map is now copied. Added test copilotClientOptionsEnvironmentIndependence to validate this behavior. Commit eb573ef
There was a problem hiding this comment.
Fixed to clone the environment map for collection independence. Updated Javadoc to reflect that the environment map is now copied. Added test copilotClientOptionsEnvironmentIndependence to validate this behavior. Commit 6e2e991
…java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
….java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…java Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <223894421+github-code-quality[bot]@users.noreply.github.com>
- Fix sessionConfigListIndependence test to properly validate list independence by mutating original list after cloning - Clone environment map in CopilotClientOptions for collection independence - Add copilotClientOptionsEnvironmentIndependence test to validate environment map cloning - Update CopilotClientOptions Javadoc to reflect environment map is now copied - Apply spotless formatting Co-authored-by: brunoborges <129743+brunoborges@users.noreply.github.com>
Upstream Sync: 6 commits from github/copilot-sdk (304d812...5016587)
Summary
Successfully ported relevant changes from 6 upstream commits. The main change was adding clone() methods to configuration classes, following Java idioms for shallow cloning.
Commits Analyzed
5016587- Fix Available SDKs table Location links to point to SDK subfolders (#486)46a1239- Change Node engine requirement to 20+. Use 22 for CI.f1038d7- Hide CLI console window on Windows across all SDKs (#329)273c425- Allow custom npm registry URL for downloading Copilot from dotnet SDK (#463)f460b5d- Fix maccatalyst RID detection in SDK targets (#485)ce54e43- Add copy constructors and Clone methods to the various .NET config option bags (#422)Changes Ported
✅ Clone methods (
ce54e43) - Added clone() methods to config classes:✅ Windows console hiding (
f1038d7) - Added clarifying comment in CliServerManager explaining Java's behavior✅ Tests - Added ConfigCloneTest with 8 comprehensive test cases covering:
Changes NOT Ported (Intentional)
❌ README.md table fix (
5016587) - Not applicable, Java SDK is in separate repository❌ Node.js engine requirement (
46a1239) - Node.js CI-specific, not applicable to Java SDK❌ Custom npm registry URL (
273c425) - .NET build-time feature for bundled CLI download. Java SDK expects users to install CLI separately via PATH❌ maccatalyst RID detection (
f460b5d) - .NET build-time feature for platform detection. Java SDK doesn't bundle CLI executablesVerification
mvn clean verify5016587a62652f3d184b3c6958dfc63359921aa8Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.