From 68fb9752c49e81a2bf5706e1b33cd0cea4fcc2ff Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Sat, 16 May 2026 22:24:57 -0400 Subject: [PATCH 1/2] Honor preinstalled CLI path in .NET MSBuild targets (#921) The `CopilotSkipCliDownload=true` escape hatch also gated the copy and register targets, so consumers behind authenticated npm mirrors had no way to point the build at a pre-downloaded copilot CLI. `_CopilotCliBinaryPath` was also unconditionally reassigned inside each target, so even setting it via a global property had no effect. Add a public `CopilotCliBinaryPath` property that: * suppresses `_DownloadCopilotCli` * still runs `_CopyCopilotCliToOutput` and `_RegisterCopilotCliForCopy` so the supplied binary is placed at the canonical `runtimes//native/copilot[.exe]` path that `Client.GetBundledCliPath` searches at runtime * fails the build with an actionable `` if the path is missing Switch the `` task from `DestinationFolder` to `DestinationFiles` so off-spec source filenames are normalized to `copilot[.exe]` on copy. Add `dotnet/test/Unit/MSBuildTargetsTests.cs` -- the first MSBuild-target test in the repo -- with 5 integration tests that import the real targets file into a throwaway csproj and shell out to `dotnet build` to validate the four scenarios (preinstalled, preinstalled + skip, skip only, missing path) end-to-end. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- dotnet/src/build/GitHub.Copilot.SDK.targets | 49 +++- dotnet/test/Unit/MSBuildTargetsTests.cs | 290 ++++++++++++++++++++ 2 files changed, 327 insertions(+), 12 deletions(-) create mode 100644 dotnet/test/Unit/MSBuildTargetsTests.cs diff --git a/dotnet/src/build/GitHub.Copilot.SDK.targets b/dotnet/src/build/GitHub.Copilot.SDK.targets index 9bc98f0f7..d03a8deaa 100644 --- a/dotnet/src/build/GitHub.Copilot.SDK.targets +++ b/dotnet/src/build/GitHub.Copilot.SDK.targets @@ -55,14 +55,33 @@ 600 - - + + + <_CopilotCliBinaryPath Condition="'$(CopilotCliBinaryPath)' != ''">$(CopilotCliBinaryPath) + + + + <_CopilotCacheDir>$(IntermediateOutputPath)copilot-cli\$(CopilotCliVersion)\$(_CopilotPlatform) - <_CopilotCliBinaryPath>$(_CopilotCacheDir)\$(_CopilotBinary) + <_CopilotCliBinaryPath Condition="'$(_CopilotCliBinaryPath)' == ''">$(_CopilotCacheDir)\$(_CopilotBinary) <_CopilotArchivePath>$(_CopilotCacheDir)\copilot.tgz <_CopilotNormalizedRegistryUrl>$([System.String]::Copy('$(CopilotNpmRegistryUrl)').TrimEnd('/')) <_CopilotDownloadUrl>$(_CopilotNormalizedRegistryUrl)/@github/copilot-$(_CopilotPlatform)/-/copilot-$(_CopilotPlatform)-$(CopilotCliVersion).tgz @@ -91,23 +110,29 @@ - - + + - <_CopilotCacheDir>$(IntermediateOutputPath)copilot-cli\$(CopilotCliVersion)\$(_CopilotPlatform) - <_CopilotCliBinaryPath>$(_CopilotCacheDir)\$(_CopilotBinary) + <_CopilotCacheDir Condition="'$(_CopilotCacheDir)' == ''">$(IntermediateOutputPath)copilot-cli\$(CopilotCliVersion)\$(_CopilotPlatform) + <_CopilotCliBinaryPath Condition="'$(_CopilotCliBinaryPath)' == ''">$(_CopilotCacheDir)\$(_CopilotBinary) <_CopilotOutputDir>$(OutDir)runtimes\$(_CopilotRid)\native + - + - - + + - <_CopilotCacheDir>$(IntermediateOutputPath)copilot-cli\$(CopilotCliVersion)\$(_CopilotPlatform) - <_CopilotCliBinaryPath>$(_CopilotCacheDir)\$(_CopilotBinary) + <_CopilotCacheDir Condition="'$(_CopilotCacheDir)' == ''">$(IntermediateOutputPath)copilot-cli\$(CopilotCliVersion)\$(_CopilotPlatform) + <_CopilotCliBinaryPath Condition="'$(_CopilotCliBinaryPath)' == ''">$(_CopilotCacheDir)\$(_CopilotBinary) + +/// Integration tests for the MSBuild targets shipped in +/// dotnet/src/build/GitHub.Copilot.SDK.targets. Each test creates a throwaway +/// project that imports the targets file directly and invokes dotnet build in +/// a subprocess so we exercise real MSBuild evaluation. +/// +/// +/// These tests deliberately do not exercise the network-bound default download path; they +/// pin a fake CopilotCliVersion and supply a fake CLI binary via +/// CopilotCliBinaryPath. That is sufficient to cover the regression in issue +/// #921 ("preinstalled CLI is ignored and copy/register are skipped when +/// CopilotSkipCliDownload=true"). +/// +public class MSBuildTargetsTests +{ + private static readonly string TargetsFilePath = FindTargetsFile(); + + private static readonly string BinaryName = OperatingSystem.IsWindows() ? "copilot.exe" : "copilot"; + + [Fact] + public async Task PreinstalledCliBinaryPath_IsHonored_DownloadSkipped_AndCopiedToOutput() + { + using var sandbox = MSBuildSandbox.Create(); + var preinstalled = sandbox.WritePreinstalledBinary("fake-cli-contents"); + + var result = await sandbox.BuildAsync(new Dictionary + { + ["CopilotCliBinaryPath"] = preinstalled, + }); + + Assert.True(result.Succeeded, result.FailureMessage()); + + // Download message must be absent because the download target was skipped. + Assert.DoesNotContain("Downloading Copilot CLI", result.StandardOutput, StringComparison.Ordinal); + + // Binary must be placed at the canonical runtimes path so Client.cs can locate it. + var outputPath = sandbox.ExpectedOutputBinary(); + Assert.True(File.Exists(outputPath), $"Expected CLI to be copied to '{outputPath}'.\n{result.FailureMessage()}"); + Assert.Equal(File.ReadAllText(preinstalled), File.ReadAllText(outputPath)); + } + + [Fact] + public async Task PreinstalledCliBinaryPath_NormalizesNonStandardFileNameToCanonical() + { + using var sandbox = MSBuildSandbox.Create(); + // Use an off-spec source filename to confirm the copy task renames it to copilot[.exe]. + var preinstalled = sandbox.WritePreinstalledBinary("custom-named", fileName: "my-copilot-binary-v1.bin"); + + var result = await sandbox.BuildAsync(new Dictionary + { + ["CopilotCliBinaryPath"] = preinstalled, + }); + + Assert.True(result.Succeeded, result.FailureMessage()); + + var outputPath = sandbox.ExpectedOutputBinary(); + Assert.True(File.Exists(outputPath), $"Expected canonical binary at '{outputPath}'.\n{result.FailureMessage()}"); + } + + [Fact] + public async Task SkipCliDownload_WithoutBinaryPath_ProducesNoBinaryAndSucceeds() + { + using var sandbox = MSBuildSandbox.Create(); + + var result = await sandbox.BuildAsync(new Dictionary + { + ["CopilotSkipCliDownload"] = "true", + }); + + Assert.True(result.Succeeded, result.FailureMessage()); + + // The runtimes folder may or may not be created by something else, but the binary + // itself must not exist. + Assert.False(File.Exists(sandbox.ExpectedOutputBinary()), + $"Expected no CLI binary in output when CopilotSkipCliDownload=true and no path supplied.\n{result.FailureMessage()}"); + + // Download must also have been skipped. + Assert.DoesNotContain("Downloading Copilot CLI", result.StandardOutput, StringComparison.Ordinal); + } + + [Fact] + public async Task PreinstalledCliBinaryPath_WithSkipCliDownload_StillCopiesToOutput() + { + using var sandbox = MSBuildSandbox.Create(); + var preinstalled = sandbox.WritePreinstalledBinary("fake-cli-contents"); + + var result = await sandbox.BuildAsync(new Dictionary + { + ["CopilotCliBinaryPath"] = preinstalled, + ["CopilotSkipCliDownload"] = "true", + }); + + Assert.True(result.Succeeded, result.FailureMessage()); + Assert.True(File.Exists(sandbox.ExpectedOutputBinary()), result.FailureMessage()); + } + + [Fact] + public async Task PreinstalledCliBinaryPath_NonExistentFile_FailsWithActionableError() + { + using var sandbox = MSBuildSandbox.Create(); + var nonexistent = Path.Combine(sandbox.ProjectDir, "does-not-exist", BinaryName); + + var result = await sandbox.BuildAsync(new Dictionary + { + ["CopilotCliBinaryPath"] = nonexistent, + }); + + Assert.False(result.Succeeded, "Build should have failed when CopilotCliBinaryPath points at a missing file."); + Assert.Contains("Copilot CLI binary not found", result.StandardOutput, StringComparison.Ordinal); + Assert.Contains(nonexistent, result.StandardOutput, StringComparison.Ordinal); + } + + private static string FindTargetsFile([CallerFilePath] string? thisFile = null) + { + // thisFile == /dotnet/test/Unit/MSBuildTargetsTests.cs + if (thisFile is not null && File.Exists(thisFile)) + { + var candidate = Path.GetFullPath(Path.Combine( + Path.GetDirectoryName(thisFile)!, "..", "..", "src", "build", "GitHub.Copilot.SDK.targets")); + if (File.Exists(candidate)) + { + return candidate; + } + } + + // Fall back to walking up from the test assembly location. + var dir = AppContext.BaseDirectory; + for (var i = 0; i < 8 && dir is not null; i++) + { + var candidate = Path.Combine(dir, "src", "build", "GitHub.Copilot.SDK.targets"); + if (File.Exists(candidate)) + { + return candidate; + } + dir = Path.GetDirectoryName(dir); + } + + throw new InvalidOperationException( + "Could not locate GitHub.Copilot.SDK.targets relative to test assembly or source file."); + } + + /// + /// A throwaway directory containing a minimal csproj that imports the SDK targets + /// file. Disposing removes the directory tree. + /// + private sealed class MSBuildSandbox : IDisposable + { + public string ProjectDir { get; } + + private MSBuildSandbox(string projectDir) + { + ProjectDir = projectDir; + } + + public static MSBuildSandbox Create() + { + var dir = Path.Combine(Path.GetTempPath(), "copilot-sdk-targets-test-" + Guid.NewGuid().ToString("N")); + Directory.CreateDirectory(dir); + + // Minimal class library that imports the SDK targets with a pinned fake + // CopilotCliVersion so the targets do not need the generated props file. + var csproj = $""" + + + net8.0 + 0.0.0-test + true + + + + """; + File.WriteAllText(Path.Combine(dir, "App.csproj"), csproj); + File.WriteAllText(Path.Combine(dir, "Stub.cs"), "namespace CopilotSdkTargetsTest { internal static class Stub { } }\n"); + + return new MSBuildSandbox(dir); + } + + public string WritePreinstalledBinary(string contents, string? fileName = null) + { + var preinstallDir = Path.Combine(ProjectDir, "preinstall"); + Directory.CreateDirectory(preinstallDir); + var path = Path.Combine(preinstallDir, fileName ?? BinaryName); + File.WriteAllText(path, contents); + return path; + } + + public string ExpectedOutputBinary() + { + var rid = GetPortableRid(); + return Path.Combine(ProjectDir, "bin", "Debug", "net8.0", "runtimes", rid, "native", BinaryName); + } + + public async Task BuildAsync(IDictionary properties) + { + var args = new StringBuilder("build --nologo -clp:NoSummary"); + foreach (var (key, value) in properties) + { + // Quote the value so paths with spaces are preserved. + args.Append(" /p:").Append(key).Append('=').Append('"').Append(value).Append('"'); + } + + var psi = new ProcessStartInfo("dotnet", args.ToString()) + { + WorkingDirectory = ProjectDir, + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + CreateNoWindow = true, + }; + // Avoid inheriting the parent's MSBuildSDKsPath/RuntimeIdentifier from the + // running test host; the subprocess should resolve its own SDK. + psi.Environment.Remove("MSBuildSDKsPath"); + + using var process = Process.Start(psi) ?? throw new InvalidOperationException("Failed to start dotnet build subprocess."); + + // Drain both streams concurrently to avoid deadlocks on full pipe buffers. + var stdoutTask = process.StandardOutput.ReadToEndAsync(); + var stderrTask = process.StandardError.ReadToEndAsync(); + + // Generous timeout: dotnet restore + build of an empty project on a slow CI + // worker can take ~60s the first time. We keep individual tests short by + // using minimal projects. + using var cts = new CancellationTokenSource(TimeSpan.FromMinutes(5)); + try + { + await process.WaitForExitAsync(cts.Token); + } + catch (OperationCanceledException) + { + try { process.Kill(entireProcessTree: true); } catch { /* best effort */ } + throw new TimeoutException($"dotnet build did not complete within the timeout for args: {args}"); + } + + return new BuildResult( + ExitCode: process.ExitCode, + StandardOutput: await stdoutTask, + StandardError: await stderrTask, + CommandLine: $"dotnet {args}"); + } + + public void Dispose() + { + try { Directory.Delete(ProjectDir, recursive: true); } catch { /* best effort */ } + } + + private static string GetPortableRid() + { + if (OperatingSystem.IsWindows()) + { + return System.Runtime.InteropServices.RuntimeInformation.OSArchitecture switch + { + System.Runtime.InteropServices.Architecture.Arm64 => "win-arm64", + _ => "win-x64", + }; + } + if (OperatingSystem.IsMacOS()) + { + return System.Runtime.InteropServices.RuntimeInformation.OSArchitecture switch + { + System.Runtime.InteropServices.Architecture.Arm64 => "osx-arm64", + _ => "osx-x64", + }; + } + return System.Runtime.InteropServices.RuntimeInformation.OSArchitecture switch + { + System.Runtime.InteropServices.Architecture.Arm64 => "linux-arm64", + _ => "linux-x64", + }; + } + } + + private sealed record BuildResult(int ExitCode, string StandardOutput, string StandardError, string CommandLine) + { + public bool Succeeded => ExitCode == 0; + + public string FailureMessage() => + $"{CommandLine}\nExitCode: {ExitCode}\n--- STDOUT ---\n{StandardOutput}\n--- STDERR ---\n{StandardError}"; + } +} From 8b0a5e1b2f53218c827bc71a5564451e06b6866b Mon Sep 17 00:00:00 2001 From: Copilot <223556219+Copilot@users.noreply.github.com> Date: Sat, 16 May 2026 22:39:02 -0400 Subject: [PATCH 2/2] Address review feedback on MSBuildTargetsTests - Also drop RuntimeIdentifier from the subprocess env (the comment said it but only MSBuildSDKsPath was actually removed), so CI workers that set a RID don't pull the build into a different runtimes folder than ExpectedOutputBinary() expects. - Narrow the two best-effort catch clauses (process.Kill on timeout, and Directory.Delete in Dispose) to the specific exception types those operations actually throw, while keeping the same swallow-and- continue semantics. - Sanitize the caller-supplied fileName in WritePreinstalledBinary via Path.GetFileName so a rooted or directory-containing value can't silently escape preinstallDir via Path.Combine. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- dotnet/test/Unit/MSBuildTargetsTests.cs | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/dotnet/test/Unit/MSBuildTargetsTests.cs b/dotnet/test/Unit/MSBuildTargetsTests.cs index 887e8271b..745069ca4 100644 --- a/dotnet/test/Unit/MSBuildTargetsTests.cs +++ b/dotnet/test/Unit/MSBuildTargetsTests.cs @@ -190,7 +190,9 @@ public string WritePreinstalledBinary(string contents, string? fileName = null) { var preinstallDir = Path.Combine(ProjectDir, "preinstall"); Directory.CreateDirectory(preinstallDir); - var path = Path.Combine(preinstallDir, fileName ?? BinaryName); + // Strip any path information from fileName so it cannot escape preinstallDir. + var safeFileName = string.IsNullOrEmpty(fileName) ? BinaryName : Path.GetFileName(fileName); + var path = Path.Combine(preinstallDir, safeFileName); File.WriteAllText(path, contents); return path; } @@ -219,8 +221,10 @@ public async Task BuildAsync(IDictionary properties CreateNoWindow = true, }; // Avoid inheriting the parent's MSBuildSDKsPath/RuntimeIdentifier from the - // running test host; the subprocess should resolve its own SDK. + // running test host; the subprocess should resolve its own SDK and pick the + // RID that matches ExpectedOutputBinary(). psi.Environment.Remove("MSBuildSDKsPath"); + psi.Environment.Remove("RuntimeIdentifier"); using var process = Process.Start(psi) ?? throw new InvalidOperationException("Failed to start dotnet build subprocess."); @@ -238,7 +242,10 @@ public async Task BuildAsync(IDictionary properties } catch (OperationCanceledException) { - try { process.Kill(entireProcessTree: true); } catch { /* best effort */ } + try { process.Kill(entireProcessTree: true); } + catch (InvalidOperationException) { /* process already exited */ } + catch (NotSupportedException) { /* not supported on this platform */ } + catch (System.ComponentModel.Win32Exception) { /* kill failed; best effort */ } throw new TimeoutException($"dotnet build did not complete within the timeout for args: {args}"); } @@ -251,7 +258,9 @@ public async Task BuildAsync(IDictionary properties public void Dispose() { - try { Directory.Delete(ProjectDir, recursive: true); } catch { /* best effort */ } + try { Directory.Delete(ProjectDir, recursive: true); } + catch (IOException) { /* cleanup is best effort */ } + catch (UnauthorizedAccessException) { /* cleanup is best effort */ } } private static string GetPortableRid()