diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 2f8ad424..1c93b28b 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -149,3 +149,9 @@ jobs: - name: Run integration tests run: ./scripts/run_integration_tests_individually.sh + + # Real-Neovim regression gate for issue #238 (reject-with-:q). The unit spec + # only exercises the WinClosed handler logic against a mock; this drives a + # genuine `:q` and self-reports via cquit (exit 0 = fixed, 1 = regressed). + - name: Reject-on-quit regression gate (#238) + run: nvim --headless -u NONE -l scripts/repro_issue_238.lua diff --git a/CHANGELOG.md b/CHANGELOG.md index 91ddc750..0dfd9393 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,7 @@ ### Bug Fixes - `focus_after_send = true` no longer fails silently with `terminal.provider = "none"`/`"external"`: those providers run Claude outside Neovim, so focus cannot move there. A one-time warning is now emitted at setup pointing to the new `User ClaudeCodeSendComplete` autocmd, which you can hook to focus your own terminal. (`focus_after_send` still only auto-focuses the in-editor providers.) ([#228](https://github.com/coder/claudecode.nvim/issues/228)) +- Rejecting a Claude diff with `:q` (or `:close` / `c` / closing the tab) now resolves it as rejected, matching the documented behavior. The proposed buffer is a scratch buffer that `:q` only hides, so the existing `BufDelete`/`BufUnload`/`BufWipeout` autocmds never fired; a `WinClosed` autocmd now handles window-close rejection. ([#238](https://github.com/coder/claudecode.nvim/issues/238)) - Diffs opened via `openDiff` no longer linger forever when they are resolved outside this Neovim or their Claude session goes away. Pending diffs are now automatically closed when the client that opened them disconnects or the integration is stopped, and `closeAllDiffTabs` now also resolves/cleans the diff module's tracked state instead of only closing windows. ([#248](https://github.com/coder/claudecode.nvim/issues/248)) - Show diffs when the Claude Code terminal is the only window (no other splits). Previously `openDiff` failed with "No suitable editor window found"; now a split is created to host the diff, matching the behavior of the `openFile` tool. ([#231](https://github.com/coder/claudecode.nvim/issues/231)) - Work around a Neovim core bug (< 0.12.2) that fragmented large bracketed pastes into the terminal across `vim.paste` phases, making Cmd+V appear to truncate content. Added a scoped, version-gated `vim.paste` shim controlled by `terminal.fix_streamed_paste` (`"auto"` by default; no-op on Neovim >= 0.12.2). ([#161](https://github.com/coder/claudecode.nvim/issues/161)) diff --git a/fixtures/issue-238/README.md b/fixtures/issue-238/README.md new file mode 100644 index 00000000..a9368224 --- /dev/null +++ b/fixtures/issue-238/README.md @@ -0,0 +1,91 @@ +# `issue-238` fixture — repro for issue #238 + +Reproduces [#238 "[BUG] Rejecting with `:q` does not work"](https://github.com/coder/claudecode.nvim/issues/238): +the README documents two ways to reject a Claude diff — `:q` **or** `ad` +(`:ClaudeCodeDiffDeny`). The keymap works; **`:q` does not reject**. The proposed +window closes but Claude is never told `DIFF_REJECTED`, and (with +`open_in_new_tab = true`) the diff tab lingers. + +This fixture uses the reporter's exact config: + +- `terminal.provider = "none"` (Claude runs in an external terminal, e.g. + sidekick.nvim, so claudecode manages no terminal of its own), and +- `diff_opts = { layout = "vertical", open_in_new_tab = true }`. + +It mirrors [`remote-diff`](../remote-diff) but adds a JSON `:DiffStateFile` +inspector that records window/tab counts, each diff's status, **and the proposed +buffer's `bufhidden`** — the smoking gun for this bug. + +> Set `REPRO238_NEW_TAB=0` to launch in the **default** same-tab layout and +> confirm the bug is not tab-specific (it reproduces there too — the split just +> silently collapses to the original file). + +## Files + +- `init.lua` — claudecode.nvim config matching the issue + `:DiffState`/`:DiffStateFile`. +- `example/target.txt` — a sample file to diff against. + +## Root cause (verified) + +The proposed buffer is created with `vim.api.nvim_create_buf(false, true)` +(a scratch buffer ⇒ `bufhidden = "hide"`) and rejection is wired **only** through +buffer-destruction autocmds (`BufDelete` / `BufUnload` / `BufWipeout` → +`_resolve_diff_as_rejected`). Because `bufhidden = "hide"`, `:q` merely **hides** +the still-loaded buffer instead of destroying it, so none of those autocmds fire +and the diff is never resolved. (`:ClaudeCodeDiffDeny` works because it calls +`_resolve_diff_as_rejected` directly, bypassing the autocmds.) + +## Quick start — headless one-liner (no WebSocket needed) + +The fastest way to see the bug. Drives the real `diff.lua` and performs a genuine +`:q`, for both `open_in_new_tab` layouts: + +```sh +nvim --headless -u NONE -l scripts/repro_issue_238.lua; echo "exit: $?" +``` + +Exit code **1** = bug reproduced (current code), **0** = fixed. On current code it prints: + +``` +[default config (open_in_new_tab=false)] + proposed buffer bufhidden = "hide" + after :q -> rejected=false status=pending proposed_buf_still_loaded=true tabpages 1->1 + => BUG: `:q` did NOT reject the diff (Claude never receives DIFF_REJECTED) +[reporter config (open_in_new_tab=true)] + proposed buffer bufhidden = "hide" + after :q -> rejected=false status=pending proposed_buf_still_loaded=true tabpages 2->2 + => BUG: ... +``` + +## Quick start — live, playing the role of Claude over MCP + +```sh +# Terminal 1 — the editor under test: +source fixtures/nvim-aliases.sh +vv issue-238 example/target.txt +# (equivalently: NVIM_APPNAME=issue-238 XDG_CONFIG_HOME=fixtures nvim fixtures/issue-238/example/target.txt) +# The server auto-starts; check the lock file: ls ~/.claude/ide/*.lock + +# Terminal 2 — open a diff and HOLD the socket open while you reject in Neovim: +scripts/repro_issue_238.sh --file "$PWD/fixtures/issue-238/example/target.txt" --hold 30 +``` + +A diff opens in a new tab. In Neovim, try to reject it with **`:q`**. Then: + +- Run `:DiffState` in Neovim → it still shows the diff as `[pending]` + (`bufhidden=hide`), and the tab is still open. +- The Terminal-2 script reports `no DIFF_REJECTED / FILE_SAVED in window` — Claude + never learned the diff was rejected. + +Contrast: reject with `:ClaudeCodeDiffDeny` (or `ad`) instead → the script +prints `DIFF_REJECTED was received` and `:DiffState` shows the diff `[rejected]`. + +> If `websocat` is a `mise` shim that refuses to run in this directory, pass the +> real binary: `WEBSOCAT="$(mise which websocat)" scripts/repro_issue_238.sh ...`. + +## Inspector commands (added by this fixture) + +- `:DiffState` — notify window/tab count + each active diff's status, `created_new_tab`, and proposed `bufhidden`. +- `:DiffStateFile [path]` — write the same info as JSON (for automation; defaults to `stdpath('cache')/diff_state.json`). +- `as` — run `:DiffState`. +- `aa` / `ad` — accept / deny the focused diff. diff --git a/fixtures/issue-238/example/target.txt b/fixtures/issue-238/example/target.txt new file mode 100644 index 00000000..9864d229 --- /dev/null +++ b/fixtures/issue-238/example/target.txt @@ -0,0 +1,5 @@ +line one +line two +line three +line four +line five diff --git a/fixtures/issue-238/init.lua b/fixtures/issue-238/init.lua new file mode 100644 index 00000000..b42f1b22 --- /dev/null +++ b/fixtures/issue-238/init.lua @@ -0,0 +1,143 @@ +-- Repro fixture for issue #238: "[BUG] Rejecting with `:q` does not work". +-- +-- Scenario this fixture is built to demonstrate: +-- 1. claudecode.nvim is configured exactly like the reporter: +-- - terminal.provider = "none" (Claude runs in an *external* terminal, +-- e.g. sidekick.nvim — Neovim manages no terminal of its own) +-- - diff_opts.open_in_new_tab = true +-- - diff_opts.layout = "vertical" +-- 2. Claude opens a diff via the `openDiff` MCP tool. It lands in a NEW tab +-- with the original file on the left and the proposed buffer on the right. +-- 3. The user tries to REJECT the change with `:q` (as the README documents: +-- "Reject: `:q` or ad"). +-- 4. EXPECTED: the diff is rejected (Claude is told DIFF_REJECTED) and the +-- tab closes. +-- ACTUAL (the bug): `:q` only closes the proposed window; the buffer is +-- merely *hidden* (it is a scratch buffer => bufhidden=hide), so none of +-- the BufDelete/BufUnload/BufWipeout autocmds that drive rejection fire. +-- The diff stays "pending" forever and the tab lingers. +-- +-- This fixture mirrors `remote-diff` but uses the reporter's exact config and +-- exposes a `:DiffStateFile` command that writes a machine-readable JSON +-- snapshot (window/tab counts, per-diff status, and the proposed buffer's +-- bufhidden) so automation can assert on the bug without scraping the screen. +-- +-- Usage (from repo root): +-- source fixtures/nvim-aliases.sh +-- vv issue-238 example/target.txt +-- # or: NVIM_APPNAME=issue-238 XDG_CONFIG_HOME=fixtures nvim fixtures/issue-238/example/target.txt +-- +-- Then drive the MCP side (play the role of Claude) with: +-- scripts/repro_issue_238.sh + +local config_dir = vim.fn.stdpath("config") +local repo_root = vim.fn.fnamemodify(config_dir, ":h:h") +vim.opt.rtp:prepend(repo_root) + +vim.g.mapleader = " " +vim.g.maplocalleader = "\\" + +local ok, claudecode = pcall(require, "claudecode") +assert(ok, "Failed to load claudecode.nvim from repo root: " .. tostring(claudecode)) + +-- The reporter's exact config uses open_in_new_tab = true, but the underlying +-- bug is not tab-specific. Set REPRO238_NEW_TAB=0 to probe the default +-- (same-tab) layout and confirm `:q` rejection is broken there too. +local open_in_new_tab = os.getenv("REPRO238_NEW_TAB") ~= "0" + +claudecode.setup({ + auto_start = false, + -- Quiet logging keeps the diff UI clean for screenshots / automation and + -- avoids the hit-enter prompt that long :messages can trigger. + log_level = "warn", + terminal = { + -- The reporter uses sidekick.nvim to run Claude in an external terminal, + -- so claudecode itself manages no terminal: provider = "none". + provider = "none", + }, + diff_opts = { + layout = "vertical", + open_in_new_tab = open_in_new_tab, + keep_terminal_focus = false, + }, +}) + +local function ensure_started() + local ok_start, started_or_err, port_or_err = pcall(function() + return claudecode.start(false) + end) + if not ok_start then + vim.notify("ClaudeCode start crashed: " .. tostring(started_or_err), vim.log.levels.ERROR) + return false + end + if started_or_err or port_or_err == "Already running" then + return true + end + vim.notify("ClaudeCode failed to start: " .. tostring(port_or_err), vim.log.levels.ERROR) + return false +end + +ensure_started() + +-- Build a snapshot of everything that matters for this bug. +local function diff_state() + local diff = require("claudecode.diff") + local active = diff._get_active_diffs() + + local diffs = {} + for tab_name, data in pairs(active) do + local proposed_bufhidden = nil + if data.new_buffer and vim.api.nvim_buf_is_valid(data.new_buffer) then + proposed_bufhidden = vim.api.nvim_buf_get_option(data.new_buffer, "bufhidden") + end + diffs[#diffs + 1] = { + tab_name = tab_name, + status = data.status or "?", + created_new_tab = data.created_new_tab or false, + new_buffer = data.new_buffer, + new_buffer_valid = data.new_buffer and vim.api.nvim_buf_is_valid(data.new_buffer) or false, + new_buffer_loaded = data.new_buffer and vim.api.nvim_buf_is_loaded(data.new_buffer) or false, + proposed_bufhidden = proposed_bufhidden, + } + end + table.sort(diffs, function(a, b) + return tostring(a.tab_name) < tostring(b.tab_name) + end) + + return { + windows = #vim.api.nvim_list_wins(), + tabpages = #vim.api.nvim_list_tabpages(), + active_diffs = #diffs, + diffs = diffs, + } +end + +-- Human-readable variant. +vim.api.nvim_create_user_command("DiffState", function() + local s = diff_state() + local lines = { + ("windows=%d tabpages=%d active_diffs=%d"):format(s.windows, s.tabpages, s.active_diffs), + } + for _, d in ipairs(s.diffs) do + lines[#lines + 1] = (" [%s] new_tab=%s bufhidden=%s loaded=%s %s"):format( + d.status, + tostring(d.created_new_tab), + tostring(d.proposed_bufhidden), + tostring(d.new_buffer_loaded), + d.tab_name + ) + end + vim.notify(table.concat(lines, "\n"), vim.log.levels.INFO) +end, { desc = "Show window/tab count + active claudecode diffs" }) + +-- Scriptable variant: writes the state as JSON to a file so external automation +-- can assert on it without scraping the message area. +vim.api.nvim_create_user_command("DiffStateFile", function(opts) + local path = opts.args ~= "" and opts.args or (vim.fn.stdpath("cache") .. "/diff_state.json") + local s = diff_state() + vim.fn.writefile({ vim.json.encode(s) }, path) +end, { nargs = "?", desc = "Write window/diff state as JSON to a file" }) + +vim.keymap.set("n", "aa", "ClaudeCodeDiffAccept", { desc = "Accept diff" }) +vim.keymap.set("n", "ad", "ClaudeCodeDiffDeny", { desc = "Deny diff" }) +vim.keymap.set("n", "as", "DiffState", { desc = "Show diff state" }) diff --git a/lua/claudecode/diff.lua b/lua/claudecode/diff.lua index 4e234e8e..51d91bcd 100644 --- a/lua/claudecode/diff.lua +++ b/lua/claudecode/diff.lua @@ -867,6 +867,39 @@ local function register_diff_autocmds(tab_name, new_buffer) end, }) + -- WinClosed: reject when the proposed window is closed (`:q`, `:close`, `c`, `:tabclose`). + -- The proposed buffer is scratch (bufhidden="hide"), so closing its window merely HIDES the + -- still-loaded buffer and none of BufDelete/BufUnload/BufWipeout below fire -> `:q` would never + -- resolve the diff and Claude would never receive DIFF_REJECTED (issue #238). + -- + -- We do NOT bind to a single window-id pattern: the proposed buffer may be split into several + -- windows (v), and rejecting just because the *tracked* window closed would prematurely + -- reject while the user still views a clone (and closing a clone would never match the pattern). + -- Instead we reject only once the proposed buffer is displayed in NO window (across all tabs; + -- a copy the user split into another tab defers rejection until that copy is closed too). + -- WinClosed fires BEFORE the closing window leaves the layout, so we exclude it (args.match). + -- _resolve_diff_as_rejected no-ops once status != "pending", so this is harmless after :w + -- (accept) or during _cleanup_diff_state (which deletes these autocmds before closing windows). + autocmd_ids[#autocmd_ids + 1] = vim.api.nvim_create_autocmd("WinClosed", { + group = get_autocmd_group(), + callback = function(args) + if not vim.api.nvim_buf_is_valid(new_buffer) then + return + end + local closing_win = tonumber(args.match) + local still_visible = false + for _, win in ipairs(vim.fn.win_findbuf(new_buffer)) do + if win ~= closing_win then + still_visible = true + break + end + end + if not still_visible then + M._resolve_diff_as_rejected(tab_name) + end + end, + }) + -- Buffer deletion monitoring for rejection (multiple events to catch all deletion methods) -- BufDelete: When buffer is deleted with :bdelete, :bwipeout, etc. @@ -902,6 +935,9 @@ local function register_diff_autocmds(tab_name, new_buffer) return autocmd_ids end +-- Exposed for testing the reject-on-window-close (WinClosed) behavior. +M._register_diff_autocmds = register_diff_autocmds + ---Create diff view from a specific window ---@param target_window NvimWin|nil The window to use as base for the diff ---@param old_file_path string Path to the original file diff --git a/scripts/repro_issue_238.lua b/scripts/repro_issue_238.lua new file mode 100644 index 00000000..8df5e4c6 --- /dev/null +++ b/scripts/repro_issue_238.lua @@ -0,0 +1,307 @@ +-- Reproduction / verification for issue #238: +-- "[BUG] Rejecting with `:q` does not work" +-- https://github.com/coder/claudecode.nvim/issues/238 +-- +-- The README documents two ways to reject a Claude diff: `:q` or ad +-- (:ClaudeCodeDiffDeny). The keymap works; `:q` does NOT. +-- +-- Root cause: the proposed ("new") buffer is created with +-- vim.api.nvim_create_buf(false, true) -- scratch => bufhidden = "hide" +-- and rejection is wired ONLY through buffer-destruction autocmds +-- BufDelete / BufUnload / BufWipeout -> _resolve_diff_as_rejected +-- Because bufhidden = "hide", running `:q` on the proposed window merely HIDES +-- the buffer (the window closes, the buffer stays loaded), so none of those +-- autocmds fire and the diff is never resolved as rejected. Claude is never +-- told DIFF_REJECTED and (with open_in_new_tab=true) the tab lingers. +-- +-- This script drives the REAL diff.lua (the exact path the openDiff MCP tool +-- uses, M._setup_blocking_diff) and performs a genuine `:q` on the proposed +-- window — no WebSocket / Claude CLI needed. It both reproduces the bug (on +-- unfixed code) and verifies a fix. +-- +-- Run from the repo root: +-- nvim --headless -u NONE -l scripts/repro_issue_238.lua +-- +-- Exit code: 1 if the bug is reproduced (any scenario fails to reject on `:q`), +-- 0 if `:q` rejects in every scenario (fixed). The verdict is printed either way. + +local script_path = debug.getinfo(1, "S").source:sub(2) +local repo_root = vim.fn.fnamemodify(script_path, ":h:h") +vim.opt.rtp:prepend(repo_root) + +local function out(msg) + io.stdout:write(msg .. "\n") +end + +local diff = require("claudecode.diff") + +-- Match the reporter's terminal config (Claude runs in an external terminal, so +-- claudecode manages no terminal of its own). Best-effort: the bug does not +-- depend on the provider, but this keeps the repro faithful. +pcall(function() + require("claudecode.terminal").setup({ provider = "none" }, nil) +end) + +-- A single temp file under Neovim's own tempdir (auto-removed on exit, even on a +-- crash) -- so the gate never leaves an untracked file in the repo root. +local target_path = nil + +---Write a real on-disk file so old_file_exists = true (editing an existing file). +---@return string path +local function make_target() + target_path = target_path or vim.fn.tempname() + vim.fn.writefile({ "line one", "line two", "line three", "line four", "line five" }, target_path) + return target_path +end + +---Reset to a single empty window/tab between scenarios. +local function reset_editor() + pcall(function() + diff._cleanup_all_active_diffs("repro reset") + end) + pcall(vim.cmd, "silent! tabonly") + pcall(vim.cmd, "silent! only") + pcall(vim.cmd, "silent! enew!") +end + +---Open a diff for an existing file, then reject it with a genuine `:q`. +---@param open_in_new_tab boolean +---@return table report +local function run_scenario(open_in_new_tab) + reset_editor() + diff.setup({ + diff_opts = { layout = "vertical", open_in_new_tab = open_in_new_tab, keep_terminal_focus = false }, + terminal = { provider = "none" }, + }) + + local target = make_target() + local tab_name = ("REPRO238 target.txt (new_tab=%s)"):format(tostring(open_in_new_tab)) + + -- The resolution callback is what the deferred-response system uses to send + -- the result back to Claude. If `:q` rejects correctly it fires with + -- DIFF_REJECTED; if the bug is present it never fires. + local captured = nil + diff._setup_blocking_diff({ + old_file_path = target, + new_file_path = target, + new_file_contents = "line one\nline two\nline three (EDITED BY CLAUDE)\nline four\nline five\n", + tab_name = tab_name, + }, function(result) + captured = result + end) + + -- Inspect the freshly-created diff state. + local active = diff._get_active_diffs() + local data = active[tab_name] + assert(data, "diff state was not registered for " .. tab_name) + local proposed_buf = data.new_buffer + local bufhidden = vim.api.nvim_buf_get_option(proposed_buf, "bufhidden") + local tabs_before = #vim.api.nvim_list_tabpages() + + -- Make the proposed window current and reject with a genuine `:q`, exactly as + -- the user would. setup_new_buffer leaves the proposed window focused, but be + -- explicit so the test does not depend on that. + if data.new_window and vim.api.nvim_win_is_valid(data.new_window) then + vim.api.nvim_set_current_win(data.new_window) + end + pcall(vim.cmd, "quit") + + -- Measure BEFORE any cleanup so we see exactly what `:q` alone did. + local status_after = (diff._get_active_diffs()[tab_name] or {}).status + local buf_loaded_after = vim.api.nvim_buf_is_loaded(proposed_buf) + local tabs_after = #vim.api.nvim_list_tabpages() + local rejected = captured ~= nil + and captured.content ~= nil + and captured.content[1] ~= nil + and captured.content[1].text == "DIFF_REJECTED" + + return { + open_in_new_tab = open_in_new_tab, + proposed_bufhidden = bufhidden, + rejected = rejected, + status_after = status_after, + buf_loaded_after = buf_loaded_after, + tabs_before = tabs_before, + tabs_after = tabs_after, + } +end + +---Split the proposed window so the buffer is shown twice, then close the clones one at a time. +---Closing one clone must NOT reject (still visible elsewhere); closing the last must reject ONCE. +---This exercises the load-bearing part of the pattern-less WinClosed handler that the +---single-window scenarios above do not: the "exclude the closing window from the count" arithmetic. +---@return boolean ok, string detail +local function run_multiwindow_scenario() + reset_editor() + diff.setup({ + diff_opts = { layout = "vertical", open_in_new_tab = false, keep_terminal_focus = false }, + terminal = { provider = "none" }, + }) + local target = make_target() + local tab_name = "REPRO238 multiwindow" + + local reject_count = 0 + diff._setup_blocking_diff({ + old_file_path = target, + new_file_path = target, + new_file_contents = "line one\nline two\nline three (EDITED BY CLAUDE)\nline four\nline five\n", + tab_name = tab_name, + }, function(result) + if result and result.content and result.content[1] and result.content[1].text == "DIFF_REJECTED" then + reject_count = reject_count + 1 + end + end) + + local data = diff._get_active_diffs()[tab_name] + if not data then + return false, "diff state was not registered" + end + local proposed = data.new_buffer + + -- Split so the proposed buffer is shown in two windows. + vim.api.nvim_set_current_win(data.new_window) + vim.cmd("vsplit") + local wins = vim.fn.win_findbuf(proposed) + if #wins < 2 then + return false, ("expected proposed buffer in >=2 windows, got %d"):format(#wins) + end + + -- Close one clone: must NOT reject (still visible in the other). + vim.api.nvim_win_close(wins[1], false) + if reject_count ~= 0 or (diff._get_active_diffs()[tab_name] or {}).status ~= "pending" then + return false, + ("closing one split prematurely rejected (count=%d, status=%s)"):format( + reject_count, + tostring((diff._get_active_diffs()[tab_name] or {}).status) + ) + end + + -- Close the last window showing it: must reject exactly once. + local remaining = vim.fn.win_findbuf(proposed) + if #remaining > 0 then + vim.api.nvim_win_close(remaining[1], false) + end + if reject_count ~= 1 or (diff._get_active_diffs()[tab_name] or {}).status ~= "rejected" then + return false, + ("closing the last split did not reject exactly once (count=%d, status=%s)"):format( + reject_count, + tostring((diff._get_active_diffs()[tab_name] or {}).status) + ) + end + + return true, "split: one close kept pending, last close rejected exactly once" +end + +---Reject a NEW-FILE diff (is_new_file=true) with `:q`. With on_new_file_reject="keep_empty" +---(the default), _resolve_diff_as_rejected eagerly runs _cleanup_diff_state, which closes the +---diff window from INSIDE the WinClosed callback -- a re-entrant window close the existing-file +---scenarios never exercise (historically an E1159 "cannot change window layout" risk). +---@return boolean ok, string detail +local function run_newfile_scenario() + reset_editor() + diff.setup({ + diff_opts = { + layout = "vertical", + open_in_new_tab = false, + keep_terminal_focus = false, + on_new_file_reject = "keep_empty", + }, + terminal = { provider = "none" }, + }) + + local newpath = vim.fn.tempname() + pcall(os.remove, newpath) -- ensure it does NOT exist -> is_new_file = true + local tab_name = "REPRO238 newfile" + + local rejected = false + diff._setup_blocking_diff({ + old_file_path = newpath, + new_file_path = newpath, + new_file_contents = "brand new line 1\nbrand new line 2\n", + tab_name = tab_name, + }, function(result) + if result and result.content and result.content[1] and result.content[1].text == "DIFF_REJECTED" then + rejected = true + end + end) + + local data = diff._get_active_diffs()[tab_name] + if not data then + return false, "new-file diff state was not registered" + end + if data.new_window and vim.api.nvim_win_is_valid(data.new_window) then + vim.api.nvim_set_current_win(data.new_window) + end + -- The reject signal fires before the eager cleanup, so `rejected` is set even if the + -- nested window close is a no-op; a hard error here would surface as quit_ok=false. + local quit_ok = pcall(vim.cmd, "quit") + if not rejected then + return false, ("new-file `:q` did not reject (quit_ok=%s)"):format(tostring(quit_ok)) + end + return true, "new-file `:q` rejected (eager keep_empty cleanup ran without a hard error)" +end + +out("== issue #238 reproduction: reject-with-:q ==") +out(("Neovim: %s"):format(vim.version and tostring(vim.version()) or "?")) + +local scenarios = { + { label = "default config (open_in_new_tab=false)", new_tab = false }, + { label = "reporter config (open_in_new_tab=true)", new_tab = true }, +} + +local any_bug = false +for _, sc in ipairs(scenarios) do + local r = run_scenario(sc.new_tab) + out("") + out(("[%s]"):format(sc.label)) + out((" proposed buffer bufhidden = %q"):format(tostring(r.proposed_bufhidden))) + out( + (" after :q -> rejected=%s status=%s proposed_buf_still_loaded=%s tabpages %d->%d"):format( + tostring(r.rejected), + tostring(r.status_after), + tostring(r.buf_loaded_after), + r.tabs_before, + r.tabs_after + ) + ) + if r.rejected then + out(" => OK: `:q` resolved the diff as DIFF_REJECTED") + else + out(" => BUG: `:q` did NOT reject the diff (Claude never receives DIFF_REJECTED)") + any_bug = true + end +end + +out("") +out("[multi-window: split the proposed buffer, close clones one at a time]") +local mw_ok, mw_detail = run_multiwindow_scenario() +if mw_ok then + out(" => OK: " .. mw_detail) +else + out(" => BUG: " .. mw_detail) + any_bug = true +end + +out("") +out("[new file: reject a not-yet-existing file's diff with :q (re-entrant keep_empty cleanup)]") +local nf_ok, nf_detail = run_newfile_scenario() +if nf_ok then + out(" => OK: " .. nf_detail) +else + out(" => BUG: " .. nf_detail) + any_bug = true +end + +reset_editor() +pcall(os.remove, target_path) + +out("") +out("== verdict ==") +if any_bug then + out("BUG REPRODUCED: `:q` fails to reject the Claude diff (issue #238).") +else + out("FIXED: `:q` rejects the Claude diff in every scenario.") +end + +io.stdout:flush() +vim.cmd("cquit " .. (any_bug and 1 or 0)) diff --git a/scripts/repro_issue_238.sh b/scripts/repro_issue_238.sh new file mode 100755 index 00000000..2204bb3b --- /dev/null +++ b/scripts/repro_issue_238.sh @@ -0,0 +1,149 @@ +#!/usr/bin/env bash +# +# repro_issue_238.sh — Reproduce GitHub issue #238 +# "[BUG] Rejecting with `:q` does not work" +# +# Symptom: with terminal.provider = "none" and diff_opts.open_in_new_tab = true, +# rejecting a Claude diff with `:q` closes the proposed window but does NOT +# reject the change — Claude is never told DIFF_REJECTED and the tab lingers. +# +# This script acts as the MCP client (i.e. it plays the role of Claude). It: +# 1. discovers the running claudecode.nvim server from its lock file, +# 2. sends the MCP initialize handshake, +# 3. opens ONE diff via `openDiff` (blocks server-side — deferred response), +# 4. KEEPS THE SOCKET OPEN for --hold seconds, logging every server->client +# frame to --out, so you can `:q` in Neovim and observe whether a +# DIFF_REJECTED response ever comes back. +# +# Unlike repro_issue_248.sh (which disconnects to test on_disconnect cleanup), +# this script stays connected so the rejection signal — if any — is captured. +# +# Usage: +# scripts/repro_issue_238.sh --file /abs/path/to/target.txt \ +# --lock-dir "$CLAUDE_CONFIG_DIR/ide" --out /tmp/frames.jsonl --hold 30 +# +# Env: +# CLAUDE_CONFIG_DIR if set, lock files are read from "$CLAUDE_CONFIG_DIR/ide" +# (matches lua/claudecode/lockfile.lua get_lock_dir()). +# +# Requirements: websocat, jq. + +set -euo pipefail + +FILE="" +LOCK_DIR="${CLAUDE_LOCKFILE_DIR:-${CLAUDE_CONFIG_DIR:+$CLAUDE_CONFIG_DIR/ide}}" +LOCK_DIR="${LOCK_DIR:-$HOME/.claude/ide}" +OUT="" +HOLD=30 +TAB="✻ [Claude Code] target.txt (issue238) ⧉" + +while [[ $# -gt 0 ]]; do + case "$1" in + --file) + FILE="$2" + shift 2 + ;; + --lock-dir) + LOCK_DIR="$2" + shift 2 + ;; + --out) + OUT="$2" + shift 2 + ;; + --hold) + HOLD="$2" + shift 2 + ;; + --tab) + TAB="$2" + shift 2 + ;; + -h | --help) + sed -n '2,40p' "$0" + exit 0 + ;; + *) + echo "Unknown arg: $1" >&2 + exit 2 + ;; + esac +done + +# Allow overriding the websocat binary (e.g. to bypass a mise shim that refuses +# to run in an untrusted directory): WEBSOCAT=/abs/path/to/websocat scripts/... +WEBSOCAT="${WEBSOCAT:-websocat}" +command -v "$WEBSOCAT" >/dev/null || { + echo "ERROR: websocat not found (looked for: $WEBSOCAT)" >&2 + exit 1 +} +command -v jq >/dev/null || { + echo "ERROR: jq not found" >&2 + exit 1 +} +[[ -n "$FILE" ]] || { + echo "ERROR: --file is required" >&2 + exit 2 +} +[[ -f "$FILE" ]] || { + echo "ERROR: file not found: $FILE" >&2 + exit 2 +} +OUT="${OUT:-$(mktemp -t repro238.frames.XXXXXX)}" + +# --- discover the running server ------------------------------------------- +LOCK_FILE=$(find "$LOCK_DIR" -maxdepth 1 -name '*.lock' -type f 2>/dev/null | head -1 || true) +if [[ -z "$LOCK_FILE" ]]; then + echo "ERROR: no lock file in $LOCK_DIR — is the fixture's server running?" >&2 + exit 1 +fi +PORT=$(basename "$LOCK_FILE" .lock) +TOKEN=$(jq -r '.authToken // empty' "$LOCK_FILE") +[[ -n "$TOKEN" ]] || { + echo "ERROR: lock file missing authToken: $LOCK_FILE" >&2 + exit 1 +} + +echo "server : ws://127.0.0.1:$PORT" +echo "lock file : $LOCK_FILE" +echo "file : $FILE" +echo "frames out : $OUT" +echo "hold : ${HOLD}s" +echo + +# --- build the MCP message stream ------------------------------------------ +REQ=$(mktemp -t repro238.req.XXXXXX) +trap 'rm -f "$REQ"' EXIT +emit() { printf '%s\n' "$1" >>"$REQ"; } + +emit '{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2025-03-26","capabilities":{"roots":{"listChanged":true}},"clientInfo":{"name":"repro-issue-238","version":"1.0.0"}}}' +emit '{"jsonrpc":"2.0","method":"notifications/initialized","params":{}}' + +# Proposed contents: original file with one line changed, so the diff is real. +contents="$(sed 's/line three/line three (EDITED BY CLAUDE)/' "$FILE")" +msg=$(jq -nc \ + --arg old "$FILE" --arg new "$FILE" --arg contents "$contents" --arg tab "$TAB" \ + '{jsonrpc:"2.0",id:101,method:"tools/call",params:{name:"openDiff",arguments:{old_file_path:$old,new_file_path:$new,new_file_contents:$contents,tab_name:$tab}}}') +emit "$msg" +echo " -> openDiff: $TAB" + +# --- run the connection, holding it open while we capture server frames ----- +# URL must come BEFORE --header (websocat --header is variadic). +: >"$OUT" +( + tail -n +1 -f "$REQ" & + TAIL_PID=$! + sleep "$HOLD" + kill "$TAIL_PID" 2>/dev/null || true +) | "$WEBSOCAT" -t "ws://127.0.0.1:$PORT" --header "x-claude-code-ide-authorization: $TOKEN" 2>/dev/null | + tee "$OUT" || true + +echo +echo "=== server -> client frames captured in $OUT ===" +if grep -q DIFF_REJECTED "$OUT" 2>/dev/null; then + echo "RESULT: DIFF_REJECTED was received — rejection WORKED." +elif grep -q FILE_SAVED "$OUT" 2>/dev/null; then + echo "RESULT: FILE_SAVED was received — diff was accepted." +else + echo "RESULT: no DIFF_REJECTED / FILE_SAVED in window — diff was NOT resolved (the #238 bug)." +fi diff --git a/tests/unit/diff_reject_on_quit_spec.lua b/tests/unit/diff_reject_on_quit_spec.lua new file mode 100644 index 00000000..9a9b9b98 --- /dev/null +++ b/tests/unit/diff_reject_on_quit_spec.lua @@ -0,0 +1,243 @@ +-- luacheck: globals expect +-- Regression test for issue #238: rejecting a Claude diff with `:q` must resolve +-- the diff as rejected. The proposed buffer is a scratch buffer (bufhidden="hide"), +-- so `:q` only HIDES it and never fires BufDelete/BufUnload/BufWipeout -- rejection +-- therefore relies on a WinClosed autocmd, gated on the proposed buffer no longer +-- being visible in any window (so splitting it and closing one clone does not +-- prematurely reject). +-- +-- This is a unit test of the WinClosed handler logic. The end-to-end behavior +-- (that a real `:q` actually triggers it) is covered by the headless gate +-- scripts/repro_issue_238.lua and the fixtures/issue-238 fixture. +require("tests.busted_setup") + +describe("diff reject on window close (issue #238)", function() + local diff + local saved_win_findbuf + local saved_buf_is_valid + + -- All opts the diff registered for a given event in the "ClaudeCodeMCPDiff" augroup. + local function autocmd_entries(event_name) + local group = _G.vim._autocmds and _G.vim._autocmds["ClaudeCodeMCPDiff"] + assert(group, "ClaudeCodeMCPDiff augroup was not created") + local found = {} + for _, ev in ipairs(group.events) do + if ev.events == event_name then + found[#found + 1] = ev.opts + end + end + return found + end + + -- Opts for the first registration of an event (callers below register a single diff). + local function autocmd_opts(event_name) + return autocmd_entries(event_name)[1] + end + + -- The WinClosed callback, asserting EXACTLY ONE is registered. autocmd lookup is + -- by event name, so a stale/duplicate registration would otherwise hand back the + -- wrong diff's callback (and mask a leaked, never-cleaned-up handler) -- fail loudly. + local function winclosed_callback() + local found = autocmd_entries("WinClosed") + assert(#found == 1, "expected exactly one WinClosed autocmd, got " .. #found) + return found[1].callback + end + + -- Every registered WinClosed callback, in registration order (for multi-diff tests: + -- a real window close fires them all). + local function winclosed_callbacks() + local cbs = {} + for _, opts in ipairs(autocmd_entries("WinClosed")) do + cbs[#cbs + 1] = opts.callback + end + return cbs + end + + before_each(function() + package.loaded["claudecode.diff"] = nil + package.loaded["claudecode.logger"] = nil + package.loaded["claudecode.logger"] = { + debug = function() end, + warn = function() end, + error = function() end, + info = function() end, + } + + saved_win_findbuf = _G.vim.fn.win_findbuf + saved_buf_is_valid = _G.vim.api.nvim_buf_is_valid + _G.vim.api.nvim_buf_is_valid = function() + return true + end + + diff = require("claudecode.diff") + diff.setup({ diff_opts = {} }) + end) + + after_each(function() + -- _G.vim is shared across the whole busted run; restore what we stubbed. + _G.vim.fn.win_findbuf = saved_win_findbuf + _G.vim.api.nvim_buf_is_valid = saved_buf_is_valid + end) + + -- Register the reject autocmds and a pending diff state that captures its + -- resolution. autocmd_ids is captured from _register_diff_autocmds (as the real + -- _setup_blocking_diff does) so _cleanup_diff_state can tear the handlers down. + local function register_pending(tab_name, new_buffer, status) + local captured = { result = nil } + local autocmd_ids = diff._register_diff_autocmds(tab_name, new_buffer) + diff._register_diff_state(tab_name, { + status = status or "pending", + resolution_callback = function(result) + captured.result = result + end, + new_buffer = new_buffer, + is_new_file = false, + autocmd_ids = autocmd_ids, + }) + return captured + end + + -- Single-diff convenience: register one diff and return its captured-result table + -- plus its (sole) WinClosed callback. + local function setup_pending_diff(tab_name, new_buffer, status) + local captured = register_pending(tab_name, new_buffer, status) + return captured, winclosed_callback() + end + + it("rejects the diff when the proposed window closes and the buffer is no longer visible", function() + local captured, cb = setup_pending_diff("issue238-a", 42) + expect(cb).to_be_function() + + _G.vim.fn.win_findbuf = function() + return {} -- buffer shown in no window after the close + end + cb({ match = "1001" }) + + expect(captured.result).to_be_table() + expect(captured.result.content[1].text).to_be("DIFF_REJECTED") + expect(diff._get_active_diffs()["issue238-a"].status).to_be("rejected") + end) + + it("rejects when only the closing window showed the buffer (the last window)", function() + local captured, cb = setup_pending_diff("issue238-b", 43) + + -- WinClosed fires BEFORE the window leaves the layout, so win_findbuf still + -- lists the closing window; it must be excluded from the visibility check. + _G.vim.fn.win_findbuf = function() + return { 1001 } + end + cb({ match = "1001" }) + + expect(captured.result).to_be_table() + expect(captured.result.content[1].text).to_be("DIFF_REJECTED") + expect(diff._get_active_diffs()["issue238-b"].status).to_be("rejected") + end) + + it("does NOT reject when the buffer is still visible in another window (split)", function() + local captured, cb = setup_pending_diff("issue238-c", 44) + + -- Closing window 1001, but the buffer is also shown in window 2002. + _G.vim.fn.win_findbuf = function() + return { 1001, 2002 } + end + cb({ match = "1001" }) + + expect(captured.result).to_be_nil() + expect(diff._get_active_diffs()["issue238-c"].status).to_be("pending") + end) + + it("is a no-op when the diff was already accepted (:w)", function() + -- Accept sets status="saved"; a later window close must not flip it to rejected. + local captured, cb = setup_pending_diff("issue238-d", 45, "saved") + + _G.vim.fn.win_findbuf = function() + return {} + end + cb({ match = "1001" }) + + expect(captured.result).to_be_nil() + expect(diff._get_active_diffs()["issue238-d"].status).to_be("saved") + end) + + it("is a no-op when the proposed buffer is already invalid", function() + local captured, cb = setup_pending_diff("issue238-e", 46) + + -- If the buffer is already gone, the handler must bail before touching win_findbuf. + _G.vim.api.nvim_buf_is_valid = function() + return false + end + _G.vim.fn.win_findbuf = function() + error("win_findbuf must not be called when the buffer is invalid") + end + cb({ match = "1001" }) + + expect(captured.result).to_be_nil() + expect(diff._get_active_diffs()["issue238-e"].status).to_be("pending") + end) + + it("registers WinClosed window-wide (no buffer/pattern) while Buf* events stay buffer-scoped", function() + -- The pattern-less registration is load-bearing: WinClosed must observe *any* window's + -- close so it can re-check visibility (a buffer= or pattern= scope would reintroduce the + -- multi-window premature-reject bug). The Buf* deletion events stay buffer-scoped. + setup_pending_diff("issue238-f", 47) + + local winclosed = autocmd_opts("WinClosed") + expect(winclosed).to_be_table() + expect(winclosed.buffer).to_be_nil() + expect(winclosed.pattern).to_be_nil() + + expect(autocmd_opts("BufDelete").buffer).to_be(47) + expect(autocmd_opts("BufUnload").buffer).to_be(47) + expect(autocmd_opts("BufWipeout").buffer).to_be(47) + end) + + it("does NOT reject when an unrelated window (not showing the buffer) closes", function() + -- WinClosed is window-wide, so it fires for every window close. Closing a window + -- that never showed the proposed buffer must not touch this diff. + local captured, cb = setup_pending_diff("issue238-g", 48) + + _G.vim.fn.win_findbuf = function() + return { 1001 } -- proposed buffer still in its own untouched window + end + cb({ match = "9999" }) -- an unrelated window closed + + expect(captured.result).to_be_nil() + expect(diff._get_active_diffs()["issue238-g"].status).to_be("pending") + end) + + it("with two pending diffs, a window close rejects only the one no longer visible", function() + -- The window-wide registration exists precisely so concurrent diffs stay isolated: + -- a real close fires EVERY diff's WinClosed, each re-checking its own buffer. + local cap_a = register_pending("issue238-A", 60) + local cap_b = register_pending("issue238-B", 61) + local cbs = winclosed_callbacks() + expect(#cbs).to_be(2) + + _G.vim.fn.win_findbuf = function(buf) + if buf == 60 then + return {} -- A's buffer no longer shown + end + return { 2002 } -- B's buffer still shown elsewhere + end + for _, cb in ipairs(cbs) do + cb({ match = "1001" }) + end + + expect(cap_a.result).to_be_table() + expect(cap_a.result.content[1].text).to_be("DIFF_REJECTED") + expect(diff._get_active_diffs()["issue238-A"].status).to_be("rejected") + expect(cap_b.result).to_be_nil() + expect(diff._get_active_diffs()["issue238-B"].status).to_be("pending") + end) + + it("captures the WinClosed autocmd id so _cleanup_diff_state tears it down", function() + -- If the WinClosed id were dropped from autocmd_ids, cleanup would leak the + -- window-wide handler and it would keep firing against later diffs. + register_pending("issue238-h", 62) + expect(#autocmd_entries("WinClosed")).to_be(1) + + diff._cleanup_diff_state("issue238-h", "test cleanup") + + expect(#autocmd_entries("WinClosed")).to_be(0) + end) +end)