On branch edburns/dd-2984436-make-copilot-cli-versions-consistent

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.

Author: edburns

CONTRIBUTING.md

@@ -40,39 +40,9 @@ If you have ideas for entirely new features, please post an issue or start a dis
 
 ### Running locally, including tests and linters
 
-```bash
-# Obtain the pinned version of Copilot CLI to the local workarea.
-# This clones the reference implementation at the commit pinned in
-# .lastmerge, but does NOT run `npm ci` inside its nodejs/ subdir.
-mvn generate-test-resources
-
-# Install the pinned Copilot CLI into target/copilot-sdk/nodejs/node_modules
-# so the SDK tests use the version declared in
-# target/copilot-sdk/nodejs/package.json (the SDK-test pin), NOT the version
-# in target/copilot-sdk/test/harness/package.json (the replay-harness pin,
-# which is incidental and may be older).
-
-## POSIX
-
-(cd target/copilot-sdk/nodejs && npm ci --ignore-scripts)
-
-## PowerShell
-
-Push-Location target\copilot-sdk\nodejs; if ($?) { npm ci --ignore-scripts }; Pop-Location
-
-# Make it so the pinned Copilot CLI is used for the tests. The patterns
-# below are scoped to the `nodejs/node_modules/` subtree so they cannot
-# accidentally pick up the older harness copy under
-# target/copilot-sdk/test/harness/node_modules/.
-
-## POSIX
-
-export COPILOT_CLI_PATH="$(find "$PWD/target" -type f -path '*/nodejs/node_modules/@github/copilot/index.js' | head -n 1)"
-
-## PowerShell
-
-$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)
+The POM has logic to ensure a known correct installation of Copilot CLI is used when executing the tests. If you want to test against a different installation of Copilot CLI, set the value of the `copilot.cli.path` maven property to the fully qualified path to the Copilot CLI.
 
+```bash
 # Build and run all tests
 mvn clean verify
 
@@ -86,17 +56,19 @@ mvn spotless:apply
 mvn spotless:check
 ```
 
-Assuming you are in the same shell you used to run the above commands, to run this exact Copilot CLI locally you can do the following.
+## Running the known correct Copilot CLI installation
 
 ### POSIX
 
 ```bash
+export COPILOT_CLI_PATH="$(find "$PWD/target" -type f -path '*/nodejs/node_modules/@github/copilot/index.js' | head -n 1)"
 node ${COPILOT_CLI_PATH}
 ```
 
 ### PowerShell
 
 ```PowerShell
+$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
 ```
 

pom.xml

@@ -49,6 +49,17 @@
         <!-- Directory where the copilot-sdk repo will be cloned for tests -->
         <copilot.sdk.clone.dir>${project.build.directory}/copilot-sdk</copilot.sdk.clone.dir>
         <copilot.tests.dir>${copilot.sdk.clone.dir}/test</copilot.tests.dir>
+        <!--
+            Path to the Copilot CLI entry point used by the SDK tests. Defaults
+            to the CLI installed under target/copilot-sdk/nodejs by the
+            install-nodejs-cli-dependencies execution. Surefire injects this
+            into the test JVM as the COPILOT_CLI_PATH environment variable, so
+            `mvn verify` is self-contained and the developer never has to set
+            it manually. Override on the command line to point at a different
+            CLI build, e.g.:
+              mvn verify -Dcopilot.cli.path=/some/other/copilot/index.js
+        -->
+        <copilot.cli.path>${copilot.sdk.clone.dir}/nodejs/node_modules/@github/copilot/index.js</copilot.cli.path>
         <!-- Set to true (via -Pskip-test-harness) to skip git-clone + npm install of test harness -->
         <skip.test.harness>false</skip.test.harness>
         <!-- Extra JVM args for Surefire; overridden by the jdk21+ profile -->
@@ -261,6 +272,35 @@
                             </arguments>
                         </configuration>
                     </execution>
+                    <!--
+                        Install the @github/copilot CLI declared by
+                        target/copilot-sdk/nodejs/package.json. This is the CLI
+                        version the SDK tests must run against (independent of
+                        the older pin in test/harness/package.json which is
+                        incidental to harness internals). Mirrors what
+                        .github/workflows/build-test.yml does manually so that
+                        `mvn clean verify` is self-contained: the prior
+                        target/copilot-sdk/nodejs/node_modules is wiped by
+                        clean, but this re-creates it before tests run.
+                        Uses npm ci with the ignore-scripts flag, matching
+                        build-test.yml.
+                    -->
+                    <execution>
+                        <id>install-nodejs-cli-dependencies</id>
+                        <phase>generate-test-resources</phase>
+                        <goals>
+                            <goal>exec</goal>
+                        </goals>
+                        <configuration>
+                            <skip>${skip.test.harness}</skip>
+                            <executable>npm</executable>
+                            <workingDirectory>${copilot.sdk.clone.dir}/nodejs</workingDirectory>
+                            <arguments>
+                                <argument>ci</argument>
+                                <argument>--ignore-scripts</argument>
+                            </arguments>
+                        </configuration>
+                    </execution>
                 </executions>
             </plugin>
             <plugin>
@@ -274,6 +314,15 @@
                         <copilot.tests.dir>${copilot.tests.dir}</copilot.tests.dir>
                         <copilot.sdk.dir>${copilot.sdk.clone.dir}</copilot.sdk.dir>
                     </systemPropertyVariables>
+                    <!--
+                        Set COPILOT_CLI_PATH for the forked test JVM so the SDK
+                        tests transparently use the pinned CLI under
+                        target/copilot-sdk/nodejs/. See the copilot.cli.path
+                        property above for the override mechanism.
+                    -->
+                    <environmentVariables>
+                        <COPILOT_CLI_PATH>${copilot.cli.path}</COPILOT_CLI_PATH>
+                    </environmentVariables>
                 </configuration>
             </plugin>
             <!-- Add src/generated/java as an additional source root -->