diff --git a/.agents/skills/verify-diff-lifecycle/covered.lock b/.agents/skills/verify-diff-lifecycle/covered.lock index 560ea3859..17d8c35c4 100644 --- a/.agents/skills/verify-diff-lifecycle/covered.lock +++ b/.agents/skills/verify-diff-lifecycle/covered.lock @@ -3,11 +3,11 @@ file lua/claudecode/config.lua 3afb1bdeff27bfb6 == lua/claudecode/cwd.lua file lua/claudecode/cwd.lua b034e82c76e8dee4 == lua/claudecode/diff.lua -file lua/claudecode/diff.lua 56ed24d0d5b77e5c -block resolve_saved 4d34b0c7e34d561f +file lua/claudecode/diff.lua 9a2b9b3ec002bf64 +block resolve_saved 36fd89d5b97f0173 block resolve_rejected c7a620816abff9a1 block open_blocking 13e23b610cab9f40 -block setup_blocking b45a7bfd5203a085 +block setup_blocking 2dc7835cec40f258 block close_by_name cc40c0d2ffdf7946 block close_pending 31bc827d1065da70 block close_for_client 330cc91666bc7884 diff --git a/lua/claudecode/README.md b/lua/claudecode/README.md index 6b46a55d3..61d39fed7 100644 --- a/lua/claudecode/README.md +++ b/lua/claudecode/README.md @@ -181,7 +181,7 @@ Module map (direct files): - When the target buffer is dirty, diff setup raises `-32000` with message "Diff setup failed". Its `data` holds the inner error, which contains "Cannot create diff: file has unsaved changes" [diff.lua:M._setup_blocking_diff] - A diff resolves only from status `pending`. Save returns `FILE_SAVED` and the content. In the `unified` layout the content is the proposal's lines with a final line break exactly when the proposal ends with one, so every blank line at its end stays. Reject returns `DIFF_REJECTED` and the tab name [diff.lua:M._resolve_diff_as_saved] [diff_inline.lua:M.resolve_inline_as_saved] - A write of the proposal buffer (`:w`, `:wq`, `:x`, `ZZ`, which run `BufWriteCmd`) never writes the file. It accepts a pending diff and does nothing else. `:w` leaves the windows open until `close_tab`, and `:wq`, `:x` and `ZZ` also close the proposal window. A write after the diff has its answer sends nothing and does not fail (the handler never returns `true`, which would delete the autocmd and make the next `:w` fail with E676). It warns when the buffer holds edits that Claude never gets [diff.lua:M._write_proposal] -- In the `vertical` and `horizontal` layouts the saved content is the lines of the proposal buffer, joined with line breaks, plus a final line break only when the buffer's `'eol'` is on and the buffer has a line. The diff sets `'eol'` from the proposal when it creates the buffer, so a proposal without a final line break is saved without one, and the user's edits keep that ending. A buffer with no lines at all (an empty proposal, or a buffer the user emptied with `ggdG` or `:%delete`) is saved as an empty string, because `:w` writes no bytes for it whatever `'eol'` says. A buffer with one empty line is saved as `"\n"` when `'eol'` is on and as `""` when it is off. The save tells the two apart with `wordcount().bytes`, because `nvim_buf_get_lines` returns one empty string for both [diff.lua:M._setup_blocking_diff] [diff.lua:M._resolve_diff_as_saved] +- In the `vertical` and `horizontal` layouts the saved content is the lines of the proposal buffer, joined with the line break of the buffer's `'fileformat'` (LF, CRLF for `dos`, CR for `mac`), plus a final line break only when the buffer's `'eol'` is on and the buffer has a line. When `'fileformats'` includes `dos`, a proposal that has a line break and only CRLF line breaks gets `'fileformat'` `dos` and lines without their CRs, as the same text read from a file does, so a line that the user types or joins is saved with CRLF, as `:w` writes it. Any other proposal gets `'fileformat'` `unix` whatever the first entry of `'fileformats'` is, with its CRs inside its lines, which `:w` writes back unchanged. The `unified` layout's buffer is read-only, so its text is the proposal itself. The diff sets `'eol'` from the proposal when it creates the buffer, so a proposal without a final line break is saved without one, and the user's edits keep that ending. A buffer with no lines at all (an empty proposal, or a buffer the user emptied with `ggdG` or `:%delete`) is saved as an empty string, because `:w` writes no bytes for it whatever `'eol'` says. A buffer with one empty line is saved as one line break of the buffer's `'fileformat'` when `'eol'` is on and as `""` when it is off. The save tells the two apart with `wordcount().bytes`, because `nvim_buf_get_lines` returns one empty string for both [diff.lua:M._setup_blocking_diff] [diff.lua:M._resolve_diff_as_saved] - A pending diff rejects at once when its proposal buffer is shown in no window, in the split and the unified layout: its last window closes (`:q`, `:close`, `c`, `:tabclose`), or that window switches to another buffer (`:edit`, `:buffer`, `:enew`, which only hide it). A copy of the proposal in another window or tab defers the reject until that copy leaves too. The switch path rejects on the next loop turn, and only when the diff is still the same one and still hidden. Both layouts keep the proposal buffer's `'bufhidden'` at `hide`: the buffer is modified, and `wipe` would make `:q` fail with E37 [diff.lua:M._register_reject_when_hidden] [diff_inline.lua:M.setup_inline_diff] - A diff that has its answer, by either one, clears `'modified'` on its proposal buffer. While the flag stayed set, `:wq` left the window open, and `:qa` stopped with E37 or E162 and moved a window onto the hidden buffer until `close_tab` deleted it [diff.lua:mark_proposal_unmodified] - At exit, Neovim fires `BufUnload` for each loaded buffer before `VimLeavePre`, and a proposal's `BufUnload` autocmd rejects its diff, but that loop stops as soon as an autocmd deletes the buffer it fired for. The reject of a new-file diff with `on_new_file_reject = "keep_empty"`, in a tab that the diff did not open, deletes its proposal buffer, and another plugin's buffer can delete itself in its own `BufUnload`. The diffs after either get no `BufUnload` (probed on Neovim 0.8.0, 0.9.5, 0.10.4, 0.11.5 and 0.12.3). SIGTERM and SIGHUP run `VimLeavePre` without that loop. A quit from inside an autocmd that is not `nested`, SIGKILL and a crash run no `VimLeavePre`. The `VimLeavePre` autocmd of `ClaudeCodeMCPDiff` therefore runs `cleanup_at_exit`, which first rejects each diff with the status `pending`, oldest first, each under `pcall`, and skips a diff with another status. A reject can remove its own diff (the `keep_empty` case above), so a diff can be gone before the next one is rejected. The reject of a diff whose `openDiff` setup has not finished stays in `setup_result`, because the call has no response sender yet, so the server's `stop` sends that call's response. The README at the repository root, under Global Invariants, lists what Claude receives for each kind of call [diff.lua:cleanup_at_exit] [diff.lua:M.open_diff_blocking] [server/init.lua:M.stop] diff --git a/lua/claudecode/diff.lua b/lua/claudecode/diff.lua index 3bdc491a3..14388d3b1 100644 --- a/lua/claudecode/diff.lua +++ b/lua/claudecode/diff.lua @@ -1110,7 +1110,9 @@ function M._resolve_diff_as_saved(tab_name, buffer_id) -- Get content from buffer local content_lines = vim.api.nvim_buf_get_lines(buffer_id, 0, -1, false) - local final_content = table.concat(content_lines, "\n") + -- :w ends each line with the line break of the buffer's 'fileformat': LF, CRLF for dos, CR for mac. + local line_break = ({ dos = "\r\n", mac = "\r" })[vim.api.nvim_buf_get_option(buffer_id, "fileformat")] or "\n" + local final_content = table.concat(content_lines, line_break) -- Add trailing newline if the buffer has one, as :w does. :w writes nothing for an empty buffer, whatever 'eol' is. -- Such a buffer has no lines, but nvim_buf_get_lines still returns one empty string for it, so the count of bytes tells -- it from a buffer with one empty line. @@ -1118,7 +1120,7 @@ function M._resolve_diff_as_saved(tab_name, buffer_id) return vim.fn.wordcount().bytes end) if byte_count > 0 and vim.api.nvim_buf_get_option(buffer_id, "eol") then - final_content = final_content .. "\n" + final_content = final_content .. line_break end -- Do not modify windows/tabs here; wait for explicit close_tab tool call to clean up UI @@ -1835,12 +1837,22 @@ function M._setup_blocking_diff(params, resolution_callback) local new_unique_name = is_new_file and (tab_name .. " (NEW FILE - proposed)") or (tab_name .. " (proposed)") vim.api.nvim_buf_set_name(new_buffer, new_unique_name) - local lines = vim.split(params.new_file_contents, "\n") + -- A proposal that has a line break, and only CRLF line breaks, takes 'fileformat' dos, as the same text read from + -- a file does, when the user's 'fileformats' includes dos. The buffer then holds the lines without their CRs, as + -- the file's own buffer does, and the save gives a line that the user types a CRLF, as :w does. Any other text is + -- unix, with its CRs inside the lines, which :w writes back unchanged. 'fileformat' is set in both cases, because + -- a new buffer takes the first entry of 'fileformats', and that entry can be dos. + local proposal = params.new_file_contents + local is_dos = vim.o.fileformats:find("dos", 1, true) ~= nil + and proposal:find("\n", 1, true) ~= nil + and not proposal:gsub("\r\n", ""):find("\n", 1, true) + local lines = vim.split(proposal, is_dos and "\r\n" or "\n", { plain = true }) -- Remove trailing empty line if content ended with \n if #lines > 0 and lines[#lines] == "" then table.remove(lines, #lines) end vim.api.nvim_buf_set_lines(new_buffer, 0, -1, false, lines) + vim.api.nvim_buf_set_option(new_buffer, "fileformat", is_dos and "dos" or "unix") -- A new buffer has 'eol' on, and the saved text ends with a line break exactly when 'eol' is on. So the buffer takes -- the ending of the proposal, and a proposal without a final line break is saved without one. vim.api.nvim_buf_set_option(new_buffer, "eol", params.new_file_contents:sub(-1) == "\n") diff --git a/tests/integration/diff_accept_crlf_spec.lua b/tests/integration/diff_accept_crlf_spec.lua new file mode 100644 index 000000000..c18111699 --- /dev/null +++ b/tests/integration/diff_accept_crlf_spec.lua @@ -0,0 +1,300 @@ +if vim == nil or vim.fn.has("nvim") ~= 1 then + describe("the text a diff accepts when the proposal has CRLF line breaks", function() + pending("needs a real Neovim") + end) + return +end + +-- The accepted text goes back to Claude as the file's new contents, so it must be what Neovim's own `:w` writes for the +-- proposal. A proposal whose line breaks are all CRLF reads as 'fileformat' dos when Neovim reads it from a file, and +-- `:w` then ends every line with CRLF, a line that the user typed too. The split layouts kept each CR inside its line +-- and joined the lines with LF, so a line that the user typed, or the join of two lines, came back with an LF or with +-- a stray CR in the middle of a line. The unified layout's buffer is read-only, so its answer is the proposal itself. +-- The diff gives the buffer the 'fileformat' of the proposal, whatever the first entry of 'fileformats' is, and takes +-- dos only when 'fileformats' includes it, as Neovim does when it reads a file. +describe("the text a diff accepts when the proposal has CRLF line breaks", function() + local diff = require("claudecode.diff") + local tools = require("claudecode.tools.init") + local tab = "✻ [Claude Code] crlf.txt ⧉" + local dir, answers + + --- Opens the diff of a proposal. `old_text` nil means the file does not exist yet. + local function open_diff(layout, old_text, new_text) + local file = dir .. "/crlf.txt" + if old_text then + local handle = assert(io.open(file, "wb")) + handle:write(old_text) + handle:close() + vim.cmd("edit " .. vim.fn.fnameescape(file)) + end + diff.setup({ terminal = { provider = "none" }, diff_opts = { layout = layout } }) + local call = tools.handle_invoke({ id = "crlf_spec" }, { + name = "openDiff", + arguments = { old_file_path = file, new_file_path = file, new_file_contents = new_text, tab_name = tab }, + }) + assert.is_true(call._deferred, "openDiff waits for the user") + _G.claude_deferred_responses[tostring(call.coroutine)] = function(response) + answers[#answers + 1] = response + end + return diff._get_active_diffs()[tab] + end + + --- What Neovim's own `:w` writes for the file `text` after the same commands. Neovim reads the text as a file, so it + --- detects 'fileformat' as it does for any file. + local function neovim_writes(text, commands) + local file = dir .. "/oracle.txt" + local handle = assert(io.open(file, "wb")) + handle:write(text) + handle:close() + vim.cmd("edit " .. vim.fn.fnameescape(file)) + for _, command in ipairs(commands) do + vim.cmd(command) + end + vim.cmd("write") + local written = assert(io.open(file, "rb")) + local bytes = written:read("*a") + written:close() + vim.cmd("bwipeout!") + return bytes + end + + --- Runs `commands` in the proposal window, then saves with `:write`, and returns the text that Claude gets. + local function edit_and_accept(state, commands) + vim.api.nvim_set_current_win(state.new_window) + for _, command in ipairs(commands) do + vim.cmd(command) + end + vim.cmd("write") + assert.equals(1, #answers, "one answer") + assert.equals("FILE_SAVED", answers[1].content[1].text) + return answers[1].content[2].text + end + + --- The lines of the proposal window that diff mode marks as changed. + local function changed_lines(state) + return vim.api.nvim_win_call(state.new_window, function() + local lines = {} + for line = 1, vim.api.nvim_buf_line_count(0) do + if vim.fn.diff_hlID(line, 1) ~= 0 then + lines[#lines + 1] = line + end + end + return lines + end) + end + + before_each(function() + vim.o.columns, vim.o.lines = 160, 40 + dir = vim.fn.tempname() + vim.fn.mkdir(dir, "p") + tools.register_all() + _G.claude_deferred_responses = {} + answers = {} + end) + + after_each(function() + diff._cleanup_all_active_diffs("test cleanup") + vim.cmd("silent! tabonly!") + vim.cmd("silent! only!") + vim.cmd("silent! %bwipeout!") + vim.fn.delete(dir, "rf") + _G.claude_deferred_responses = nil + end) + + local split_layouts = { "vertical", "horizontal" } + local all_layouts = { "vertical", "horizontal", "unified" } + + -- A proposal without a final line break stays without one (#542), where a default `:w` adds one. `nofixeol` makes + -- Neovim's `:w` keep it too, so the rest of the bytes can be compared. + local proposals = { + { name = "CRLF line breaks and a final one", text = "a\r\nb\r\n", commands = {} }, + { name = "a blank line inside", text = "a\r\n\r\nb\r\n", commands = {} }, + { name = "CRLF line breaks and no final one", text = "a\r\nb", commands = { "setlocal nofixeol" } }, + } + local old_files = { + { name = "a CRLF file", text = "old\r\n" }, + { name = "a file that does not exist yet", text = nil }, + } + + -- The edits change what the buffer holds, so the split layouts take the text from the buffer. The unified layout's + -- buffer is read-only. + local edits = { + { name = "a typed line after the last line", commands = { "normal! Gox" } }, + { name = "a typed line above the first line", commands = { "normal! ggOx" } }, + { name = "all text replaced by a typed line", commands = { "normal! ggdG", "normal! ix" } }, + { name = "the first two lines joined", commands = { "normal! ggJ" } }, + { name = "the first line deleted", commands = { "normal! ggdd" } }, + { name = "a line break typed at the end of the first line", commands = { 'execute "normal! ggA\\x"' } }, + } + + for _, layout in ipairs(split_layouts) do + describe("in the " .. layout .. " layout", function() + for _, proposal in ipairs(proposals) do + for _, edit in ipairs(edits) do + local commands = vim.list_extend(vim.list_slice(proposal.commands), edit.commands) + it(("accepts what :w writes after %s, from a proposal with %s"):format(edit.name, proposal.name), function() + local expected = neovim_writes(proposal.text, commands) + local state = open_diff(layout, "old\r\n", proposal.text) + + assert.equal(vim.inspect(expected), vim.inspect(edit_and_accept(state, commands))) + end) + end + end + + for _, old_file in ipairs(old_files) do + it("accepts what :w writes after a typed line, against " .. old_file.name, function() + local commands = { "normal! Gox" } + local expected = neovim_writes("a\r\nb\r\n", commands) + local state = open_diff(layout, old_file.text, "a\r\nb\r\n") + + assert.equal(vim.inspect(expected), vim.inspect(edit_and_accept(state, commands))) + end) + end + + it("ends a line that the user types with CRLF", function() + local state = open_diff(layout, "old\r\n", "a\r\nb\r\n") + + assert.equal(vim.inspect("a\r\nb\r\nx\r\n"), vim.inspect(edit_and_accept(state, { "normal! Gox" }))) + end) + + it("shows a CRLF proposal as 'fileformat' dos, without a CR in its lines", function() + local state = open_diff(layout, "old\r\n", "a\r\nb\r\n") + local buffer = vim.api.nvim_win_get_buf(state.new_window) + + assert.equal("dos", vim.api.nvim_buf_get_option(buffer, "fileformat")) + assert.same({ "a", "b" }, vim.api.nvim_buf_get_lines(buffer, 0, -1, false)) + end) + + it("shows no difference between a CRLF proposal and the CRLF file that holds the same text", function() + local state = open_diff(layout, "a\r\nb\r\nc\r\n", "a\r\nb\r\nc\r\n") + + assert.same({}, changed_lines(state)) + end) + + -- Controls: these gave the text that :w writes before the fix too. A proposal that is not all CRLF reads as + -- unix, so it keeps the text it has, CRs included. + local controls = { + { name = "an LF proposal", text = "a\nb\n", commands = { "normal! Gox" }, format = "unix" }, + { + name = "a proposal with CRLF and LF line breaks", + text = "a\r\nb\n", + commands = { "normal! Gox" }, + format = "unix", + }, + { name = "a proposal with a lone CR", text = "a\rb\n", commands = { "normal! Gox" }, format = "unix" }, + { name = "a proposal with no line break", text = "a\r", commands = { "setlocal nofixeol" }, format = "unix" }, + { name = "a CRLF proposal that is not edited", text = "a\r\nb\r\n", commands = {} }, + { name = "an emptied CRLF proposal", text = "a\r\nb\r\n", commands = { "normal! ggdG" } }, + } + for _, control in ipairs(controls) do + it("still accepts what :w writes for " .. control.name, function() + local expected = neovim_writes(control.text, control.commands) + local state = open_diff(layout, "old\n", control.text) + local buffer = vim.api.nvim_win_get_buf(state.new_window) + + if control.format then + assert.equal(control.format, vim.api.nvim_buf_get_option(buffer, "fileformat")) + end + assert.equal(vim.inspect(expected), vim.inspect(edit_and_accept(state, control.commands))) + end) + end + + -- The user can change 'fileformat' of the proposal, and `:w` then writes that format. + local formats = { "dos", "mac", "unix" } + for _, format in ipairs(formats) do + it("accepts what :w writes after :setlocal fileformat=" .. format .. " on an LF proposal", function() + local commands = { "setlocal fileformat=" .. format } + local expected = neovim_writes("a\nb\n", commands) + local state = open_diff(layout, "old\n", "a\nb\n") + + assert.equal(vim.inspect(expected), vim.inspect(edit_and_accept(state, commands))) + end) + end + end) + end + + -- 'fileformats' decides how Neovim reads a text, and its first entry is the 'fileformat' of a new buffer. So the diff + -- must not take the 'fileformat' of the proposal buffer from the option, and it takes dos only for a value that + -- includes dos, whatever else the value holds. The oracle reads the text under the same value. + local lf_text = { name = "an LF proposal", text = "a\nb\n" } + local mixed_text = { name = "a proposal with CRLF and LF line breaks", text = "a\r\nb\n" } + local crlf_text = { name = "a CRLF proposal", text = "a\r\nb\r\n" } + local all_texts = { lf_text, mixed_text, crlf_text } + local fileformats_values = { + { value = "unix,dos", typed = all_texts }, + { value = "dos,unix", typed = all_texts }, + { value = "unix", typed = all_texts }, + -- With the one entry dos, Neovim reads every file as dos, an LF file too, and `:w` writes CRLF for a line that + -- the user types into it. The diff keeps an LF proposal unix, so only the CRLF proposal is compared with `:w`. + { value = "dos", typed = { crlf_text } }, + -- A value without dos must not give dos. It holds mac, which Neovim also detects in some texts with CRs, so only + -- the CRLF proposal is compared with `:w`. + { value = "unix,mac", typed = { crlf_text } }, + } + for _, setting in ipairs(fileformats_values) do + describe("with 'fileformats' set to " .. setting.value, function() + local saved_fileformats + + before_each(function() + saved_fileformats = vim.o.fileformats + vim.o.fileformats = setting.value + end) + + after_each(function() + vim.o.fileformats = saved_fileformats + end) + + for _, layout in ipairs(all_layouts) do + for _, proposal in ipairs(all_texts) do + it(("the %s layout accepts %s unedited"):format(layout, proposal.name), function() + local state = open_diff(layout, "old\n", proposal.text) + + assert.equal(vim.inspect(proposal.text), vim.inspect(edit_and_accept(state, {}))) + end) + end + end + + for _, layout in ipairs(split_layouts) do + for _, proposal in ipairs(setting.typed) do + it( + ("the %s layout accepts what :w writes after a typed line, from %s"):format(layout, proposal.name), + function() + local commands = { "normal! Gox" } + local expected = neovim_writes(proposal.text, commands) + local state = open_diff(layout, "old\n", proposal.text) + + assert.equal(vim.inspect(expected), vim.inspect(edit_and_accept(state, commands))) + end + ) + end + + it( + "the " .. layout .. " layout shows no difference between a CRLF proposal and the CRLF file with its text", + function() + local state = open_diff(layout, "a\r\nb\r\nc\r\n", "a\r\nb\r\nc\r\n") + + assert.same({}, changed_lines(state)) + end + ) + end + end) + end + + -- Every layout accepts an unedited proposal as it is. The unified layout has no other way: its buffer is read-only. + for _, layout in ipairs(all_layouts) do + for _, old_file in ipairs(old_files) do + for _, proposal in ipairs(proposals) do + local title = ("the %s layout accepts a proposal with %s unedited, against %s"):format( + layout, + proposal.name, + old_file.name + ) + it(title, function() + local state = open_diff(layout, old_file.text, proposal.text) + + assert.equal(vim.inspect(proposal.text), vim.inspect(edit_and_accept(state, {}))) + end) + end + end + end +end) diff --git a/tests/mocks/vim.lua b/tests/mocks/vim.lua index 574f40ca6..d9c7192e5 100644 --- a/tests/mocks/vim.lua +++ b/tests/mocks/vim.lua @@ -1460,7 +1460,7 @@ vim._tab_windows[1] = { 1000 } vim._current_window = 1000 -- Global options table (minimal) -vim.o = setmetatable({ columns = 120, lines = 40, eventignore = "" }, { +vim.o = setmetatable({ columns = 120, lines = 40, eventignore = "", fileformats = "unix,dos" }, { __index = function(_, k) return vim._options[k] end,