Edburns/dd 2984436 make copilot cli versions consistent#137
Merged
Conversation
Previously, three different code paths determined the Copilot CLI version
inconsistently:
- Path A (build-test.yml) installed the CLI from the cloned reference impl
pinned by .lastmerge — reproducible.
- Path B (run-smoke-test.yml, update-copilot-dependency.yml via the
setup-copilot action) ran `npm install -g @github/copilot` with no
version pin — floated to whatever was latest on npm.
- Path C (local dev) fell through to whatever `copilot` was on PATH
(often whatever VS Code Insiders had installed).
When the CLI protocol vocabulary changed between 1.0.35 and 1.0.39, this
inconsistency made it hard to land reference-impl merges that required a
specific CLI version.
This change makes pom.xml the single source of truth for the
@github/copilot version that all paths must use, while keeping .lastmerge
as the source of truth for the reference implementation commit.
Changes:
- pom.xml: replace the PRIMER_TO_REPLACE placeholder in the property
<readonly-copilot-sdk-ref-impl-version-from-lastmerge-file-updated-by-weekly-reference-impl-sync>
with the current value (^1.0.36-0) extracted from the cloned
reference impl's nodejs/package.json. Add a doc-comment explaining
that the property is auto-managed and is the canonical CLI version
for all CI paths.
- .github/scripts/reference-impl-sync/sync-cli-version-from-reference-impl.sh
(new): reads dependencies."@github/copilot" from the reference impl's
nodejs/package.json (via node) and rewrites the pom.xml property in
place. Locates the repo root by walking up from the script's own
location until it finds a pom.xml, so it is resilient to relocation
and works from any CWD. Uses portable sed for the rewrite.
- .github/scripts/reference-impl-sync/merge-reference-impl-finish.sh:
call the new sync script right after writing .lastmerge, and stage
pom.xml alongside .lastmerge in the same commit. .lastmerge and the
CLI version property now always move together.
- .github/actions/setup-copilot/action.yml: read the pinned version from
pom.xml using sed and run `npm install -g "@github/copilot@VERSION"`
instead of installing the latest. Fail fast if the property is unset
or still the primer. Expose the resolved version as an action output.
- CONTRIBUTING.md: document the local-dev workflow that mirrors what
build-test.yml does in CI. The cloned reference impl contains two
separate package.json files that both pin @github/copilot:
* target/copilot-sdk/nodejs/package.json (^1.0.36-0) — the SDK-test
pin; this is the CLI the tests must run against.
* target/copilot-sdk/test/harness/package.json (^1.0.32) — the
replay-harness pin; incidental, NOT the CLI under test.
`mvn generate-test-resources` runs `npm install` only in the harness
subtree, so a generic `find target -path '*/@github/copilot/index.js'`
picks the wrong (older) copy. The instructions now:
1. Run `npm ci` in target/copilot-sdk/nodejs/ explicitly.
2. Scope the COPILOT_CLI_PATH lookup to '*/nodejs/node_modules/...'
so the harness copy can never be selected, even if both are
present. Both POSIX (find) and PowerShell (Get-ChildItem) forms
are tightened.
- .gitignore: minor housekeeping.
Verified mvn clean package -Pskip-test-harness -DskipTests succeeds with
the new POM, the sync script correctly rewrites the property when invoked
from any CWD, and the tightened CONTRIBUTING.md sequence resolves
COPILOT_CLI_PATH to the 1.0.36-0 CLI under target/copilot-sdk/nodejs.
modified: CONTRIBUTING.md - Simplify this after realizing the POM can handle the `npm ci`. modified: pom.xml - Ensure the COPILOT_CLI_PATH is set to the built-in Copilot CLI instance. This ensures a user's local Copilot instance, which is not guaranteed to be the correct version, and thus the correct RPC protocol, will get in the way.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR centralizes how the repo installs and locates the Copilot CLI so local mvn clean verify, CI workflows, and the reference-impl sync flow all converge on a single configured CLI source/version.
Changes:
- Add Maven properties + Surefire env injection so tests use a configured
COPILOT_CLI_PATH, and add a Maven execution tonpm cithe reference-implnodejs/dependencies. - Add scripts to sync the pinned
@github/copilotversion intopom.xmlduring reference-impl merges, and wire that into the merge finish script. - Update the
setup-copilotcomposite action and CONTRIBUTING docs to use the pinned version/path.
Show a summary per file
| File | Description |
|---|---|
pom.xml |
Defines CLI path + “pinned” CLI version property; installs CLI deps via npm ci; injects COPILOT_CLI_PATH into test JVM. |
.github/actions/setup-copilot/action.yml |
Reads the pinned version from pom.xml and installs that CLI globally; exposes the resolved version as an output. |
.github/scripts/reference-impl-sync/sync-cli-version-from-reference-impl.sh |
New helper to sync @github/copilot version from the reference implementation into pom.xml. |
.github/scripts/reference-impl-sync/merge-reference-impl-finish.sh |
Calls the new sync script and stages pom.xml alongside .lastmerge. |
CONTRIBUTING.md |
Updates local run instructions and adds snippets for running the known-correct CLI. |
.gitignore |
Ignores *.sln. |
Copilot's findings
Comments suppressed due to low confidence (1)
CONTRIBUTING.md:72
- Similarly, the PowerShell example should quote the path when invoking node (e.g.,
node "$env:COPILOT_CLI_PATH") so it works when the repo path contains spaces.
$env:COPILOT_CLI_PATH = (Get-ChildItem -Path "$PWD\target" -Recurse -Filter 'index.js' -File | Where-Object { $_.FullName -match '[\\/]nodejs[\\/]node_modules[\\/]@github[\\/]copilot[\\/]index\.js$' } | Select-Object -First 1 -ExpandProperty FullName)
node $env:COPILOT_CLI_PATH
- Files reviewed: 5/6 changed files
- Comments generated: 8
…it message Agent-Logs-Url: https://github.com/github/copilot-sdk-java/sessions/a806a008-d6a9-4f91-9afd-746df3cc127a Co-authored-by: edburns <75821+edburns@users.noreply.github.com>
Address review comment on PR #137 (discussion_r3165129538): the install-nodejs-cli-dependencies execution previously ran on every invocation of generate-test-resources because it was only guarded by ${skip.test.harness}. That made non-test builds (e.g. mvn package -DskipTests, mvn deploy -Dmaven.test.skip=true) require npm and network access unnecessarily. Introduce a new ${skip.cli.install} property that defaults to ${skip.test.harness} but is forced to true when -DskipTests=true or -Dmaven.test.skip=true is set, via two auto-activated profiles. Wire the install-nodejs-cli-dependencies execution's <skip> to this new property.
edburns
deleted the
edburns/dd-2984436-make-copilot-cli-versions-consistent
branch
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.
This PR makes it so all the places that need to invoke Copilot CLI in the process of running tests or dealing with agentic workflows, use the same Copilot CLI installation, and therefore Copilot CLI version, as defined in the
@github/copilotcorresponding to the commit has in.lastmerge. This version number is also written to the POM as the value of the propertyreadonly-copilot-sdk-ref-impl-version-from-lastmerge-file-updated-by-weekly-reference-impl-sync.pom.xml(+70 / -0)Four additions that together make
pom.xmlthe single source of truth for the Copilot CLI version and makemvn clean verifyself-contained:<readonly-copilot-sdk-ref-impl-version-from-lastmerge-file-updated-by-weekly-reference-impl-sync>=^1.0.36-0. Mirrorsdependencies."@github/copilot"in the cloned reference impl'snodejs/package.json. Long doc-comment marks it auto-managed (by the new sync script) and warns against manual edits.<copilot.cli.path>=${copilot.sdk.clone.dir}/nodejs/node_modules/@github/copilot/index.js. Doc-comment shows the override syntax (-Dcopilot.cli.path=...).exec-maven-pluginexecutioninstall-nodejs-cli-dependenciesbound togenerate-test-resources. Runsnpm ci --ignore-scriptsin${copilot.sdk.clone.dir}/nodejs/, honors${skip.test.harness}. Re-createstarget/copilot-sdk/nodejs/node_modulesaftercleanwipes it.maven-surefire-pluginconfiguration gains<environmentVariables><COPILOT_CLI_PATH>${copilot.cli.path}</…></…>, so the forked test JVM uses the pinned CLI with no manual env-var export..github/actions/setup-copilot/action.yml(+24 / -1)Replaces the unpinned
npm install -g @github/copilotwith a two-step pinned install:Read pinned @github/copilot version from pom.xml(id:cli-version) extracts the pinned version withsed. Fails fast if the property is unset or still thePRIMER_TO_REPLACEplaceholder.Install Copilot CLI (pinned to pom.xml version)runsnpm install -g "@github/copilot@${CLI_VERSION}".Adds new action output
cli-versionexposing the resolved version..github/scripts/reference-impl-sync/sync-cli-version-from-reference-impl.sh(new, +80)New script that keeps the pinned CLI version in
pom.xmlin lockstep with the reference impl pinned in.lastmerge:dependencies."@github/copilot"from the reference impl'snodejs/package.jsonvianode.pom.xml— resilient to relocation, works from any CWD.<readonly-…>property inpom.xmlin place using portablesed.$1or viaREFERENCE_IMPL_DIRenv var..github/scripts/reference-impl-sync/merge-reference-impl-finish.sh(+12 / -2)Wires the new sync script into the reference-impl merge flow:
.lastmerge, invokessync-cli-version-from-reference-impl.shagainst the cloned reference impl directory.pom.xmlalongside.lastmergein the same commit so the two values always move together.CONTRIBUTING.md(+18 / -2 net)Reworks the "Running tests and linters" section:
mvn generate-test-resources+npm ci+ manualCOPILOT_CLI_PATHexport —pom.xmlnow handles all of that automatically.copilot.cli.pathMaven property.COPILOT_CLI_PATH(scoped to*/nodejs/node_modules/...so the older harness copy undertarget/copilot-sdk/test/harness/node_modules/can never be selected) and invoke it vianode..gitignore(+1)Minor housekeeping: added
*.sln(Visual Studio solution files generated when the workspace is opened in VS).