From cd9b956269bd905a6a177ce3d92cb00c86b68a87 Mon Sep 17 00:00:00 2001 From: Bohan Jiang <52446949+Bohan-J@users.noreply.github.com> Date: Fri, 31 Jul 2026 16:09:36 +0800 Subject: [PATCH] fix(agent): spawn Copilot's native binary on Windows so the prompt survives (MUL-5586) (#6236) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Windows the daemon passes the full multi-line prompt as `-p ` but spawns npm's `copilot.cmd`, which we already rewrite to `powershell -File copilot.ps1`. Neither launcher can carry that argument: - `copilot.cmd` forwards with `%*`, which cmd.exe expands by re-tokenising the raw command line. - `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 the prompt is re-tokenised. Copilot then sees several argv tokens where one was intended and refuses the run with "It looks like your prompt was not quoted, so the extra words were treated as separate arguments" — the same defect class already fixed for cursor-agent in #5649, except Copilot has no stdin prompt channel to escape through, so the prompt must stay on the command line and the launchers have to go. Copilot CLI ships a native per-platform binary and `npm-loader.js` does nothing but `spawnSync` it with argv untouched, so resolve `copilot-win32-{x64,arm64}\copilot.exe` out of the npm layout and spawn it directly. That leaves exactly one hop, Go -> native binary, and Go's syscall.EscapeArg is the exact inverse of the CRT parsing that binary uses. This mirrors resolveOpenCodeNativeFromShim / resolveDevecoNativeFromShim. Both the nested (current npm) and hoisted (older npm) platform-package locations are probed; when neither resolves, we keep falling back to the PowerShell launcher, which is still better than cmd.exe. Co-authored-by: Bohan-J Co-authored-by: multica-agent --- server/pkg/agent/copilot_invocation.go | 102 +++++++++++++-- server/pkg/agent/copilot_invocation_test.go | 118 ++++++++++++++++++ .../pkg/agent/copilot_invocation_windows.go | 29 ++++- .../agent/copilot_invocation_windows_test.go | 61 ++++++++- 4 files changed, 290 insertions(+), 20 deletions(-) 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")