diff --git a/server/pkg/agent/copilot_invocation.go b/server/pkg/agent/copilot_invocation.go index 026af73da3..50f4beeb07 100644 --- a/server/pkg/agent/copilot_invocation.go +++ b/server/pkg/agent/copilot_invocation.go @@ -1,22 +1,106 @@ package agent -import "log/slog" +import ( + "log/slog" + "os" + "path/filepath" + "runtime" + "strings" +) // chooseCopilotInvocation selects the actual program (argv[0]) and the full // argv to spawn a Copilot CLI run. // -// On macOS/Linux the npm binstub is a shebang script that execs node directly, -// so argv passes through unchanged. On Windows the npm installer ships -// copilot.cmd, which routes through cmd.exe; cmd.exe re-tokenises the raw -// command line via %*, mangling arguments that contain newlines or whitespace -// (e.g. the multi-line -p prompt). To avoid that, we find the sibling -// copilot.ps1 and invoke PowerShell with -File directly, so Go passes -// each argument as a discrete token. +// On macOS/Linux the npm binstub is a symlink to npm-loader.js with a +// `#!/usr/bin/env node` shebang, so execve hands argv to node unchanged and +// node's spawnSync forwards it to the bundled native binary unchanged. // -// The Windows rewrite lives in copilot_invocation_windows.go. +// On Windows there is no shebang, so npm ships copilot.cmd/copilot.ps1 +// launchers and every layer in between re-serialises argv onto a new command +// line. Both launchers are lossy for the multi-line, quote-bearing -p prompt +// built by buildCopilotArgs; the symptom is Copilot's own +// "It looks like your prompt was not quoted, so the extra words were treated +// as separate arguments" error. The Windows rewrite in +// copilot_invocation_windows.go therefore skips the launchers and spawns the +// bundled copilot.exe directly. func chooseCopilotInvocation(execName, lookedUp string, args []string, logger *slog.Logger) (string, []string) { if argv0, full, ok := platformCopilotInvocation(lookedUp, args, logger); ok { return argv0, full } return execName, args } + +// resolveCopilotNativeFromShim returns the path to the native Copilot +// executable bundled inside the npm package, given the path to the npm +// `copilot.cmd` shim that PATH lookup found on Windows. Returns "" if the +// shim doesn't end in `.cmd` or no candidate platform package ships a binary +// at either known location, in which case the caller falls back to the +// PowerShell launcher. +// +// Why bypass the launchers instead of quoting harder: nothing on the Go side +// can make them lossless. Both hops re-parse a command line with rules that +// differ from the CRT rules Go's os/exec escapes for. +// +// - copilot.cmd forwards with `%*`, which cmd.exe expands by re-tokenising +// the raw command line — newlines and quotes in the -p prompt do not +// survive. +// - copilot.ps1 ends in `& node.exe npm-loader.js $args`, and PowerShell +// re-serialises $args onto node's command line. Under Windows PowerShell +// 5.1 (and pwsh <= 7.2, which default to Legacy native argument passing) +// embedded double quotes are not re-escaped, so a prompt containing them +// is re-tokenised — the same defect already documented for cursor-agent +// in buildCursorArgs (#5649). +// +// Copilot has no stdin prompt channel (the escape hatch cursor-agent offers), +// so the prompt must stay on the command line and the launchers have to go. +// Spawning copilot.exe directly leaves exactly one hop, Go -> native binary, +// and Go's syscall.EscapeArg is the exact inverse of the CRT parsing the +// binary uses. +// +// Layout when installed via `npm install -g @github/copilot` (verified +// against @github/copilot 1.0.77): +// +// \copilot.cmd (shim) +// \node_modules\@github\copilot\npm-loader.js (JS entry) +// \node_modules\@github\copilot\node_modules\@github\copilot-win32-x64\copilot.exe (native) +// +// npm keeps the platform package nested under the parent package; older npm +// versions and other package managers hoist it to the top-level +// node_modules instead, so both locations are probed. +// +// statFn is injected so this is testable on non-Windows hosts. +func resolveCopilotNativeFromShim(shimPath string, statFn func(string) (os.FileInfo, error)) string { + if !strings.EqualFold(filepath.Ext(shimPath), ".cmd") { + return "" + } + prefix := filepath.Dir(shimPath) + for _, pkg := range copilotWindowsPackageCandidates(runtime.GOARCH) { + candidates := []string{ + // npm's current layout: optional platform dep nested under the parent. + filepath.Join(prefix, "node_modules", "@github", "copilot", "node_modules", "@github", pkg, "copilot.exe"), + // Hoisted layout used by older npm versions and other installers. + filepath.Join(prefix, "node_modules", "@github", pkg, "copilot.exe"), + } + for _, candidate := range candidates { + if _, err := statFn(candidate); err == nil { + return candidate + } + } + } + return "" +} + +// copilotWindowsPackageCandidates returns the npm platform package names that +// may host the bundled copilot.exe, ordered so the most likely match for the +// given GOARCH comes first. ARM64 hosts try the arm64 build first; everything +// else tries x64 first, because an x64 Node running under emulation on an +// ARM64 host installs the x64 platform package. Cost is one extra statFn call +// per miss when the GOARCH-preferred package isn't installed. +func copilotWindowsPackageCandidates(goarch string) []string { + switch goarch { + case "arm64": + return []string{"copilot-win32-arm64", "copilot-win32-x64"} + default: + return []string{"copilot-win32-x64", "copilot-win32-arm64"} + } +} diff --git a/server/pkg/agent/copilot_invocation_test.go b/server/pkg/agent/copilot_invocation_test.go index 6e6fef0686..21b4c885e4 100644 --- a/server/pkg/agent/copilot_invocation_test.go +++ b/server/pkg/agent/copilot_invocation_test.go @@ -34,3 +34,121 @@ func TestChooseCopilotInvocation_PassthroughForNonLauncher(t *testing.T) { t.Errorf("argv changed unexpectedly:\n got %#v\n want %#v", gotArgs, args) } } + +// ── Windows native-binary resolution tests ── +// +// These run on every platform: resolveCopilotNativeFromShim takes statFn as a +// parameter precisely so the Windows layout can be asserted from macOS/Linux CI. + +// TestResolveCopilotNativeFromShim_NestedNpmLayout covers the layout npm +// actually produces today (verified against @github/copilot 1.0.77): the +// platform package stays nested under the parent package rather than being +// hoisted. +func TestResolveCopilotNativeFromShim_NestedNpmLayout(t *testing.T) { + t.Parallel() + + prefix := filepath.Join("C:\\Users", "dev", "AppData", "Roaming", "npm") + shim := filepath.Join(prefix, "copilot.cmd") + native := filepath.Join(prefix, "node_modules", "@github", "copilot", "node_modules", "@github", "copilot-win32-x64", "copilot.exe") + + if got := resolveCopilotNativeFromShim(shim, fakeStat(native)); got != native { + t.Errorf("got %q, want %q", got, native) + } +} + +// TestResolveCopilotNativeFromShim_HoistedNpmLayout covers older npm versions +// and other installers that hoist the optional platform dep to the top-level +// node_modules. +func TestResolveCopilotNativeFromShim_HoistedNpmLayout(t *testing.T) { + t.Parallel() + + prefix := filepath.Join("C:\\Users", "dev", "AppData", "Roaming", "npm") + shim := filepath.Join(prefix, "copilot.cmd") + native := filepath.Join(prefix, "node_modules", "@github", "copilot-win32-x64", "copilot.exe") + + if got := resolveCopilotNativeFromShim(shim, fakeStat(native)); got != native { + t.Errorf("got %q, want %q", got, native) + } +} + +// TestResolveCopilotNativeFromShim_FindsArm64Package covers Windows-on-ARM +// hosts, where npm installs @github/copilot-win32-arm64 instead. The resolver +// must find it regardless of which arch this test binary was built for, since +// the arch list only controls probe order, not membership. +func TestResolveCopilotNativeFromShim_FindsArm64Package(t *testing.T) { + t.Parallel() + + prefix := filepath.Join("C:\\Users", "dev", "AppData", "Roaming", "npm") + shim := filepath.Join(prefix, "copilot.cmd") + native := filepath.Join(prefix, "node_modules", "@github", "copilot", "node_modules", "@github", "copilot-win32-arm64", "copilot.exe") + + if got := resolveCopilotNativeFromShim(shim, fakeStat(native)); got != native { + t.Errorf("got %q, want %q", got, native) + } +} + +// TestResolveCopilotNativeFromShim_ReturnsEmptyWhenNativeMissing covers a +// partial install or a Copilot build predating the platform packages. The +// caller must fall back to the PowerShell launcher rather than spawn a path +// that doesn't exist. +func TestResolveCopilotNativeFromShim_ReturnsEmptyWhenNativeMissing(t *testing.T) { + t.Parallel() + + shim := filepath.Join("C:\\Users", "dev", "AppData", "Roaming", "npm", "copilot.cmd") + + if got := resolveCopilotNativeFromShim(shim, fakeStat()); got != "" { + t.Errorf("got %q, want empty (missing native binary)", got) + } +} + +// TestResolveCopilotNativeFromShim_SkipsNonCmdPath keeps macOS/Linux and +// direct-binary Windows launches on the untouched passthrough path. +func TestResolveCopilotNativeFromShim_SkipsNonCmdPath(t *testing.T) { + t.Parallel() + + for _, p := range []string{ + "/usr/local/bin/copilot", + "C:\\Users\\dev\\AppData\\Roaming\\npm\\copilot.exe", + "C:\\Users\\dev\\AppData\\Roaming\\npm\\copilot.ps1", + "", + } { + if got := resolveCopilotNativeFromShim(p, fakeStat("anything")); got != "" { + t.Errorf("path %q: got %q, want empty", p, got) + } + } +} + +// TestResolveCopilotNativeFromShim_AcceptsUppercaseExtension guards the +// PATHEXT case: Windows filesystems are case-insensitive and exec.LookPath +// can return either case. +func TestResolveCopilotNativeFromShim_AcceptsUppercaseExtension(t *testing.T) { + t.Parallel() + + prefix := filepath.Join("C:\\Users", "dev", "AppData", "Roaming", "npm") + shim := filepath.Join(prefix, "copilot.CMD") + native := filepath.Join(prefix, "node_modules", "@github", "copilot", "node_modules", "@github", "copilot-win32-x64", "copilot.exe") + + if got := resolveCopilotNativeFromShim(shim, fakeStat(native)); got != native { + t.Errorf("got %q, want %q", got, native) + } +} + +// TestCopilotWindowsPackageCandidates_ArchOrdering pins the probe order: the +// host's own arch first, but never at the cost of dropping the other one — an +// x64 Node running under emulation on an ARM64 host installs the x64 package. +func TestCopilotWindowsPackageCandidates_ArchOrdering(t *testing.T) { + t.Parallel() + + cases := []struct { + goarch string + want []string + }{ + {"arm64", []string{"copilot-win32-arm64", "copilot-win32-x64"}}, + {"amd64", []string{"copilot-win32-x64", "copilot-win32-arm64"}}, + } + for _, tc := range cases { + if got := copilotWindowsPackageCandidates(tc.goarch); !reflect.DeepEqual(got, tc.want) { + t.Errorf("goarch %q: got %#v, want %#v", tc.goarch, got, tc.want) + } + } +} diff --git a/server/pkg/agent/copilot_invocation_windows.go b/server/pkg/agent/copilot_invocation_windows.go index 70ca1a4d9c..37e88351aa 100644 --- a/server/pkg/agent/copilot_invocation_windows.go +++ b/server/pkg/agent/copilot_invocation_windows.go @@ -2,12 +2,31 @@ package agent -import "log/slog" +import ( + "log/slog" + "os" +) -// platformCopilotInvocation rewrites copilot.cmd → PowerShell -File -// copilot.ps1 on Windows to avoid cmd.exe %* re-tokenisation mangling -// the multi-line -p prompt built by buildCopilotArgs. -// powerShellLookup and rewriteCmdToPS1 are defined in cursor_invocation_windows.go. +// platformCopilotInvocation keeps the multi-line -p prompt built by +// buildCopilotArgs intact on Windows by spawning the native copilot.exe +// bundled in the npm package, so neither cmd.exe's %* expansion nor +// PowerShell's $args re-serialisation can re-tokenise it (see +// resolveCopilotNativeFromShim for why neither launcher can be made safe). +// +// When the native binary can't be located — a partial install, or a Copilot +// build old enough to predate the platform packages — fall back to +// `powershell -File copilot.ps1`, which still avoids the strictly worse +// cmd.exe layer. powerShellLookup and rewriteCmdToPS1 are defined in +// cursor_invocation_windows.go. func platformCopilotInvocation(lookedUp string, args []string, logger *slog.Logger) (string, []string, bool) { + if native := resolveCopilotNativeFromShim(lookedUp, os.Stat); native != "" { + if logger != nil { + logger.Info("copilot: spawning the bundled native binary to keep argv intact", + "shim", lookedUp, + "native", native, + ) + } + return native, args, true + } return rewriteCmdToPS1("copilot", lookedUp, args, logger) } diff --git a/server/pkg/agent/copilot_invocation_windows_test.go b/server/pkg/agent/copilot_invocation_windows_test.go index 03c355802d..19dd689569 100644 --- a/server/pkg/agent/copilot_invocation_windows_test.go +++ b/server/pkg/agent/copilot_invocation_windows_test.go @@ -5,17 +5,66 @@ package agent import ( "io" "log/slog" + "os" "path/filepath" "reflect" + "runtime" "testing" ) -// TestPlatformCopilotInvocation_RewritesCmdLauncherToPowerShellFile is the -// core Windows test: when LookPath resolves copilot to the npm-installed .cmd -// launcher and a sibling copilot.ps1 exists, we should invoke PowerShell with -// -File and forward every original arg unchanged — including the -// multi-line -p prompt that would otherwise be mangled by cmd.exe's %* -// re-expansion inside copilot.cmd. +// TestPlatformCopilotInvocation_PrefersBundledNativeBinary is the core Windows +// test: when LookPath resolves copilot to the npm .cmd launcher and the npm +// package ships copilot.exe, we must spawn that binary directly and forward +// every original arg unchanged. Both launchers re-serialise argv onto a new +// command line and mangle the multi-line, quote-bearing -p prompt, which +// Copilot then rejects with "your prompt was not quoted". +func TestPlatformCopilotInvocation_PrefersBundledNativeBinary(t *testing.T) { + dir := t.TempDir() + cmdPath := filepath.Join(dir, "copilot.cmd") + ps1Path := filepath.Join(dir, "copilot.ps1") + writeFile(t, cmdPath, "@echo off\r\npowershell -NoProfile -ExecutionPolicy Bypass -File \"%~dp0copilot.ps1\" %*\r\n") + // A usable .ps1 launcher must NOT win over the native binary. + writeFile(t, ps1Path, "# fake copilot.ps1\r\n") + stubPowerShell(t, filepath.Join(dir, "powershell.exe"), true) + + pkg := "copilot-win32-x64" + if runtime.GOARCH == "arm64" { + pkg = "copilot-win32-arm64" + } + nativeDir := filepath.Join(dir, "node_modules", "@github", "copilot", "node_modules", "@github", pkg) + if err := os.MkdirAll(nativeDir, 0o755); err != nil { + t.Fatalf("mkdir native dir: %v", err) + } + nativePath := filepath.Join(nativeDir, "copilot.exe") + writeFile(t, nativePath, "") + + prompt := "You are running as a local coding agent.\n\n# Context\nRun `go build -ldflags \"-X main.version=x\"`.\n" + args := []string{ + "-p", prompt, + "--output-format", "json", + "--allow-all", + "--no-ask-user", + } + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + + gotExec, gotArgs, ok := platformCopilotInvocation(cmdPath, args, logger) + if !ok { + t.Fatalf("expected native rewrite to be applied, got ok=false") + } + if gotExec != nativePath { + t.Errorf("argv0: got %q want %q", gotExec, nativePath) + } + if !reflect.DeepEqual(gotArgs, args) { + t.Errorf("argv must pass through untouched:\n got %#v\n want %#v", gotArgs, args) + } +} + +// TestPlatformCopilotInvocation_RewritesCmdLauncherToPowerShellFile covers the +// fallback: when the bundled native binary can't be located (partial install, +// or a Copilot build predating the platform packages) but a sibling +// copilot.ps1 exists, we should invoke PowerShell with -File and forward +// every original arg unchanged — still better than cmd.exe's %* re-expansion +// inside copilot.cmd. func TestPlatformCopilotInvocation_RewritesCmdLauncherToPowerShellFile(t *testing.T) { dir := t.TempDir() cmdPath := filepath.Join(dir, "copilot.cmd")