diff --git a/CONTEXT.md b/CONTEXT.md index 4f0de1f..49a850b 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -77,7 +77,7 @@ The script the agent invokes directly when it's about to (or has just) used an e Job: take the agent's native hook payload, optionally fast-path-filter noisy tools (a backend-keyed branch inside the shim; the [normalisers](#core-handler) tool map is the source of truth), discover the running Neovim, splice the payload + backend name, and make one [RPC](#rpc) into the [core handler](#core-handler). -The hook entry is **per-OS** because that is a language boundary: a `.sh` shim on Unix, a PowerShell `.ps1` shim on Windows (issue #46). PowerShell is the single Windows logic language — the only stock-Windows-11 tool that parses JSON natively, enumerates named pipes, and probes the RPC socket; Windows PowerShell 5.1 (`powershell.exe`) is the floor, not pwsh 7. The installer writes the interpreter explicitly into the agent's `command` field (`powershell -NoProfile -ExecutionPolicy Bypass -File \hook-entry.ps1 `) via [`platform.hook_command`](docs/adr/0008-one-hook-entry-per-os.md). Copilot is the exception: its config uses a `bash` field, so it always invokes `hook-entry.sh` (Copilot-on-Windows would need git-bash, deferred). +The hook entry is **per-OS** because that is a language boundary: a `.sh` shim on Unix, a PowerShell `.ps1` shim on Windows (issue #46). PowerShell is the single Windows logic language — the only stock-Windows-11 tool that parses JSON natively, enumerates named pipes, and probes the RPC socket; Windows PowerShell 5.1 (`powershell.exe`) is the floor, not pwsh 7. The installer writes the interpreter explicitly into the agent's `command` field (`powershell -NoProfile -ExecutionPolicy Bypass -File \hook-entry.ps1 `) via [`platform.hook_command`](docs/adr/0008-one-hook-entry-per-os.md). Copilot is the exception: its hook entry carries *both* a `bash` field (runs `hook-entry.sh`, macOS/Linux) and a `powershell` field (runs `hook-entry.ps1`, Windows). Copilot runs the `powershell` field's string itself under **pwsh 7+** (not the 5.1 floor the other agents target), so the installer emits a bare `& '\hook-entry.ps1' ` invocation rather than reusing `platform.hook_command`. ## Core handler diff --git a/README.md b/README.md index 1b7ef0e..3e5c806 100644 --- a/README.md +++ b/README.md @@ -452,7 +452,8 @@ vim.api.nvim_create_autocmd({ "FocusGained", "BufEnter", "CursorHold" }, { **Copilot CLI hooks not firing** - Run `:CodePreviewInstallCopilotCliHooks` in the project root - Verify `.github/hooks/code-preview.json` exists -- Ensure `jq` is in PATH +- Ensure `jq` is in PATH (macOS/Linux only; the Windows hook uses PowerShell's native JSON parsing) +- On Windows, Copilot runs the hook under **pwsh 7+** (its own requirement). If hooks silently don't fire, check your PowerShell execution policy with `Get-ExecutionPolicy` — pwsh's default `RemoteSigned` runs the local hook script, but `Restricted`/`AllSigned` blocks it (e.g. `Set-ExecutionPolicy -Scope CurrentUser RemoteSigned`) - Restart Copilot CLI (hooks are loaded at session start) **Diff doesn't close after rejecting** diff --git a/lua/code-preview/apply/patch.lua b/lua/code-preview/apply/patch.lua index 20a600c..9387b6e 100644 --- a/lua/code-preview/apply/patch.lua +++ b/lua/code-preview/apply/patch.lua @@ -18,19 +18,19 @@ -- to files (the bin/apply-patch.lua shim does, pre_tool may use them -- differently in future). +local platform = require("code-preview.platform") + local M = {} --- Detect already-absolute paths before joining with cwd. Besides a Unix "/", --- Codex emits Windows-absolute paths in apply_patch directives (a drive-letter --- `D:\proj\file` / `D:/proj/file`, or a UNC `\\server\share`). Without these --- checks such a path is treated as relative and doubled onto cwd +-- Detect already-absolute paths before joining with cwd. Codex emits +-- Windows-absolute paths in apply_patch directives (drive-letter +-- `D:\proj\file` / `D:/proj/file`, or a UNC `\\server\share`); without this +-- check such a path is treated as relative and doubled onto cwd -- (`D:\proj\D:\proj\file`), which then fs_stats as missing (so the file is -- mis-marked "created"), opens the diff at a bogus path, and injects a junk --- neo-tree node. +-- neo-tree node. Detection is the shared platform.is_absolute (issue #46). local function resolve_path(path, cwd) - if path:sub(1, 1) == "/" -- Unix absolute - or path:match("^%a:[/\\]") -- Windows drive-letter absolute - or path:sub(1, 2) == "\\\\" then -- Windows UNC + if platform.is_absolute(path) then return path end return cwd .. "/" .. path diff --git a/lua/code-preview/backends/copilot.lua b/lua/code-preview/backends/copilot.lua index dafe09b..b0b07a6 100644 --- a/lua/code-preview/backends/copilot.lua +++ b/lua/code-preview/backends/copilot.lua @@ -12,20 +12,31 @@ end local platform = require("code-preview.platform") local function bin_dir() return plugin_root() .. "/bin" end --- Copilot's hook field is `bash` (the value runs under a bash shell), so it --- always invokes the .sh shim — the PowerShell-wrapped command form doesn't --- apply to this field shape. Copilot-on-Windows (which would need git-bash) --- is deferred (issue #46). -local function hook_script() return bin_dir() .. "/hook-entry.sh" end +-- Copilot's hook entry carries BOTH a `bash` and a `powershell` field (issue +-- #46). Copilot picks the one matching the OS: `bash` runs hook-entry.sh on +-- macOS/Linux, `powershell` runs hook-entry.ps1 on Windows. Unlike Claude +-- Code/Codex — where our installer writes the interpreter into the command and +-- gets Windows PowerShell 5.1 — Copilot runs the `powershell` field's string +-- itself under pwsh 7+, so we emit a bare `& '' …` invocation rather than +-- reusing platform.hook_command's 5.1 `powershell -File` form. +local function sh_hook_script() return bin_dir() .. "/hook-entry.sh" end +local function ps_hook_script() return bin_dir() .. "/hook-entry.ps1" end local function hooks_dir() return vim.fn.getcwd() .. "/.github/hooks" end local function config_path() return hooks_dir() .. "/code-preview.json" end --- Shell-quote a path for use inside the `bash` field of hooks.json. +-- Quote a path for the `bash` field (POSIX single-quote escaping). local function shquote(s) return "'" .. s:gsub("'", "'\\''") .. "'" end +-- Quote a path for the `powershell` field (PowerShell single-quote escaping: +-- a literal ' is doubled). Paired with the call operator (`& ''`) so +-- paths containing spaces invoke correctly. +local function psquote(s) + return "'" .. s:gsub("'", "''") .. "'" +end + -- True iff `path` looks like a code-preview.json our installer produced. We -- match on the hook-entry shim stem ("hook-entry"), with "code-preview-diff" -- kept so older per-backend installs are still recognised for uninstall after @@ -53,17 +64,32 @@ local function ensure_executable(path) end function M.install() - local hook = hook_script() - if not ensure_executable(hook) then return end + local sh_hook = sh_hook_script() + local ps_hook = ps_hook_script() + -- Verify (and, on Unix, chmod) only the OS-native shim — matches the + -- claudecode/codex installers. The config references both shims so it works + -- across OSes, but the non-native one is never executed here and always ships + -- with the plugin, so we don't fail the install on its account. + if not ensure_executable(bin_dir() .. "/hook-entry" .. platform.script_ext()) then return end vim.fn.mkdir(hooks_dir(), "p") - -- The bash field runs the shim under bash with the backend + event args. + -- Each entry carries both fields so the hook fires on every OS: Copilot runs + -- `bash` on macOS/Linux and `powershell` (under pwsh 7+) on Windows. + local function entry(event) + return { + type = "command", + bash = shquote(sh_hook) .. " copilot " .. event, + powershell = "& " .. psquote(ps_hook) .. " copilot " .. event, + timeoutSec = 30, + } + end + local data = { version = 1, hooks = { - preToolUse = { { type = "command", bash = shquote(hook) .. " copilot pre", timeoutSec = 30 } }, - postToolUse = { { type = "command", bash = shquote(hook) .. " copilot post", timeoutSec = 30 } }, + preToolUse = { entry("pre") }, + postToolUse = { entry("post") }, }, } diff --git a/lua/code-preview/health.lua b/lua/code-preview/health.lua index df72010..8d681bc 100644 --- a/lua/code-preview/health.lua +++ b/lua/code-preview/health.lua @@ -147,12 +147,11 @@ function M.check() warn("copilot not found in PATH (install from https://github.com/github/copilot-cli)") end - -- Copilot uses the shared bin/hook-entry.sh (checked above) through its `bash` - -- hook field. On Windows that field needs git-bash, so Copilot-on-Windows is - -- deferred (issue #46). - if is_win then - warn("Copilot CLI on Windows is not yet supported (issue #46); use Claude Code on Windows") - end + -- Copilot's hook entry carries both a `bash` field (hook-entry.sh, macOS/Linux) + -- and a `powershell` field (hook-entry.ps1, Windows). On Windows Copilot runs + -- the powershell field under pwsh 7+; ensuring that interpreter is present is + -- Copilot CLI's own documented prerequisite, so — like Codex and Claude Code — + -- we add no Windows-specific warning here. -- hooks.json installed local copilot_hooks = vim.fn.getcwd() .. "/.github/hooks/code-preview.json" diff --git a/lua/code-preview/platform.lua b/lua/code-preview/platform.lua index 7a01bfd..3de7dc8 100644 --- a/lua/code-preview/platform.lua +++ b/lua/code-preview/platform.lua @@ -43,6 +43,23 @@ function M.make_executable(path) end end +--- True if `p` is an already-absolute path. Recognises Unix-rooted (`/…`), +--- Windows drive-letter (`C:\` or `C:/`), and Windows UNC (`\\server\share`) +--- forms. Deliberately OS-INDEPENDENT — it inspects the string's shape, so it +--- recognises Windows-absolute paths even on Unix. Callers use it to avoid +--- joining an already-absolute path onto cwd, which would double it +--- (`D:\proj\D:\proj\file` → fs_stat miss → file mis-marked "created", no diff). +--- Single source of truth shared by the path resolvers in pre_tool/normalisers, +--- pre_tool/shell_detect, and apply/patch. +--- @param p string +--- @return boolean +function M.is_absolute(p) + if not p or p == "" then return false end + return p:sub(1, 1) == "/" -- Unix absolute + or p:match("^%a:[/\\]") ~= nil -- Windows drive-letter (C:\ or C:/) + or p:sub(1, 2) == "\\\\" -- Windows UNC (\\server\share) +end + --- The external dependency each OS's shim relies on, for health reporting: --- the Unix shims parse JSON with jq; the Windows shims use PowerShell's native --- ConvertFrom-Json (so jq is irrelevant there). diff --git a/lua/code-preview/pre_tool/normalisers.lua b/lua/code-preview/pre_tool/normalisers.lua index 26ce41c..4ee4083 100644 --- a/lua/code-preview/pre_tool/normalisers.lua +++ b/lua/code-preview/pre_tool/normalisers.lua @@ -13,6 +13,8 @@ -- so the opencode normaliser maps both into the canonical shape. New backends -- slot in by adding a function to the table. +local platform = require("code-preview.platform") + local M = {} local function identity(raw) @@ -57,10 +59,15 @@ local OPENCODE_TOOL_MAP = { -- Matches Node's path.resolve semantics the old TS plugin used; without it -- opencode keys could be raw "/proj/../escape.txt" strings that don't -- compare equal to claudecode-shaped keys for the same logical file. +-- An already-absolute path must NOT be joined onto cwd, or it doubles +-- (`D:\proj\D:\proj\file`): Gemini-class models route edits through Copilot's +-- `edit` tool with an absolute Windows `path`. Absolute-path detection lives in +-- the shared platform.is_absolute, so normalisers, shell_detect, and apply/patch +-- agree (issue #46). local function resolve_path(p, cwd) if not p or p == "" then return p end local abs = p - if p:sub(1, 1) ~= "/" and cwd and cwd ~= "" then + if not platform.is_absolute(p) and cwd and cwd ~= "" then abs = cwd .. "/" .. p end return vim.fs.normalize(abs) @@ -109,6 +116,13 @@ end -- before invoking us). `str_replace` and `edit` carry the same {path, -- old_str, new_str} shape; both alias to Edit. `create` and `write` both -- alias to Write (file_text vs content). +-- +-- Shell ops: on macOS/Linux Copilot uses `bash`; on Windows it uses +-- `powershell` (issue #46, observed with Gemini-class models — Remove-Item +-- deletes, git status, …). Both carry the same {command, description} shape and +-- alias to Bash, so shell_detect's PowerShell/POSIX grammar tells them apart. +-- Without the `powershell` entry, Windows shell deletes/writes arrive as +-- tool_name=nil and are silently dropped (no neo-tree indicator). local COPILOT_TOOL_MAP = { apply_patch = "ApplyPatch", edit = "Edit", @@ -116,11 +130,14 @@ local COPILOT_TOOL_MAP = { create = "Write", write = "Write", bash = "Bash", + powershell = "Bash", } --- Copilot delivers `toolArgs` as a JSON-encoded string in preToolUse and as --- an object in postToolUse. For apply_patch the string IS the raw patch text --- (not JSON). For every other tool the string contains a JSON object with +-- Copilot delivers `toolArgs` as a JSON-encoded string in preToolUse. In +-- postToolUse the shape varies across OS / model / Copilot version — it may be +-- the same JSON string or a decoded object — so the branches below accept BOTH +-- rather than assume either. For apply_patch the string IS the raw patch text +-- (not JSON); for every other tool the string contains a JSON object with -- snake_case keys (path, old_str, new_str, file_text, command, ...). -- -- Note: file paths are run through the shared `resolve_path`, which collapses diff --git a/lua/code-preview/pre_tool/shell_detect.lua b/lua/code-preview/pre_tool/shell_detect.lua index 940e940..a5ff542 100644 --- a/lua/code-preview/pre_tool/shell_detect.lua +++ b/lua/code-preview/pre_tool/shell_detect.lua @@ -28,6 +28,8 @@ -- pre-tool detection logic; resist "obvious simplifications" without first -- reading shell_detect_spec.lua. +local platform = require("code-preview.platform") + local M = {} local function is_windows() @@ -121,10 +123,11 @@ end local win_paths = {} +-- Absolute on Windows: drive-letter (C:\/C:/), UNC (\\server), or unix-rooted +-- (`/…`, git-bash). Delegates to the shared platform.is_absolute so every path +-- resolver agrees on what counts as absolute (issue #46). local function win_is_absolute(p) - return p:match("^%a:[\\/]") ~= nil -- drive-letter: C:\ or C:/ - or p:match("^\\\\") ~= nil -- UNC: \\server\share - or p:sub(1, 1) == "/" -- unix-rooted (git-bash) + return platform.is_absolute(p) end -- Canonicalise to backslash separators with ./.. collapsed. diff --git a/tests/backends/copilot/test_install.sh b/tests/backends/copilot/test_install.sh index 754ccc1..7083c11 100644 --- a/tests/backends/copilot/test_install.sh +++ b/tests/backends/copilot/test_install.sh @@ -36,9 +36,11 @@ test_install_copilot_hooks() { # Both hook events are registered with the right adapter scripts assert_contains "$content" "preToolUse" "should have preToolUse hook" || return 1 assert_contains "$content" "postToolUse" "should have postToolUse hook" || return 1 - assert_contains "$content" "hook-entry.sh" "should reference the generic hook-entry shim" || return 1 - assert_contains "$content" "copilot pre" "preToolUse should pass the pre event" || return 1 - assert_contains "$content" "copilot post" "postToolUse should pass the post event" || return 1 + assert_contains "$content" "hook-entry.sh" "should reference the Unix hook-entry shim" || return 1 + assert_contains "$content" "hook-entry.ps1" "should reference the Windows hook-entry shim" || return 1 + assert_contains "$content" "powershell" "should include a powershell field for Windows" || return 1 + assert_contains "$content" "copilot pre" "preToolUse should pass the pre event" || return 1 + assert_contains "$content" "copilot post" "postToolUse should pass the post event" || return 1 # Each event should have exactly one entry (no accidental duplication) local pre_count post_count diff --git a/tests/plugin/pre_tool_normaliser_spec.lua b/tests/plugin/pre_tool_normaliser_spec.lua index e728a8d..23cb6fb 100644 --- a/tests/plugin/pre_tool_normaliser_spec.lua +++ b/tests/plugin/pre_tool_normaliser_spec.lua @@ -181,6 +181,18 @@ describe("normalisers.normalise (copilot)", function() assert.equals("ls", out.tool_input.command) end) + -- Regression (issue #46): on Windows Copilot's shell tool is `powershell`, + -- not `bash` (observed with Gemini-class models). Same {command, description} + -- shape; must alias to Bash so shell_detect runs and marks neo-tree. Without + -- it, Remove-Item deletes arrive as tool_name=nil and are silently dropped. + it("powershell (Windows shell tool) maps to Bash with command", function() + local cmd = 'Remove-Item -Path "D:\\a\\x.txt", "D:\\b\\y.txt" -ErrorAction Stop' + local out = normalisers.normalise( + copilot_pre("powershell", { command = cmd, description = "delete temp files" }), "copilot") + assert.equals("Bash", out.tool_name) + assert.equals(cmd, out.tool_input.command) + end) + it("apply_patch treats toolArgs string as raw patch text (not JSON)", function() local out = normalisers.normalise({ toolName = "apply_patch", @@ -208,6 +220,27 @@ describe("normalisers.normalise (copilot)", function() assert.equals("/proj/src/rel.lua", out.tool_input.file_path) end) + -- Regression (issue #46): Gemini-class models route edits through Copilot's + -- `edit`/`create` tools with an ABSOLUTE Windows path. resolve_path must treat + -- a drive-letter / UNC path as already-absolute; otherwise it is joined onto + -- cwd (`D:/proj/D:/proj/file`), which fs_stats as missing → the file is + -- mis-marked "created", no diff renders, and a junk neo-tree node is injected. + it("does not double an absolute Windows drive-letter path", function() + local raw = copilot_pre("edit", { path = "D:/proj/sub/foo.lua", old_str = "a", new_str = "b" }) + raw.cwd = "D:/proj" + local out = normalisers.normalise(raw, "copilot") + assert.equals("Edit", out.tool_name) + assert.equals("D:/proj/sub/foo.lua", out.tool_input.file_path) + end) + + it("does not double an absolute UNC path", function() + local out = normalisers.normalise( + copilot_pre("create", { path = "\\\\srv\\share\\new.lua", file_text = "x" }), "copilot") + assert.equals("Write", out.tool_name) + -- Key regression assertion: cwd ("/proj") is NOT prepended. + assert.is_nil(out.tool_input.file_path:find("proj", 1, true)) + end) + it("noise / unknown tool yields nil tool_name", function() local out = normalisers.normalise( copilot_pre("view", { path = "/tmp/whatever" }), "copilot")