Repository navigation
fix(diff): keep every trailing blank line in an accepted unified diff - #535
Merged
Merged
Conversation
The unified layout joined the accepted lines with "\n" and added a final "\n" only when the join did not already end in one. A proposal that ends in blank lines has a blank last line, so the join ended in "\n" and the final line break was never added back: "a\n\n" was accepted as "a\n". Add the final line break whenever the proposal ends with one. The vertical and horizontal layouts are unchanged.
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
main now has the split-layout fix (#542), so the two specs that waited for each other can run in full: - diff_accept_trailing_newlines_spec.lua: drop the pending clause, so the 2 vertical cases without a final line break run (28 cases). - diff_accept_missing_final_newline_spec.lua: drop the `blank_end` skip, so the 9 unified cases that end in blank lines run (94 cases).
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
This was referenced Nov 12, 2026
Merged
hareki
pushed a commit
to hareki/claudecode.nvim
that referenced
this pull request
Nov 13, 2026
…he split layouts (coder#542) ## Why The vertical and horizontal diff layouts must hand Claude back the exact text it proposed when the user accepts. A proposal without a final line break came back with one. `"a"` was accepted as `"a\n"`, `"a\nb"` as `"a\nb\n"`, and an empty proposal as `"\n"`. Claude then wrote a file with a line break the user never saw in the diff. ## What changed `_resolve_diff_as_saved` in `lua/claudecode/diff.lua` saves the lines of the proposal buffer, joined with line breaks. It adds a final line break when the buffer's `'eol'` option is on. A new Neovim buffer has `'eol'` on, so every saved proposal ended with a line break. The diff now sets `'eol'` on the proposal buffer from the proposal text, right after it sets the buffer's lines in `_setup_blocking_diff`. The saved text then ends with a line break exactly when the proposal does. The saving code is the same. The user's edits in the buffer still reach Claude and keep the proposal's ending. `lua/claudecode/README.md` gets one new bullet that states this. It sits after the bullet about writing the proposed buffer, so it does not touch the line that the unified fix (F11) edits. ## Scope - The `vertical` and `horizontal` layouts share one code path, so one change fixes both. - The `unified` layout is unchanged. Its no-final-line-break cases are controls in the new spec and pass before and after. - A proposal that ends in blank lines, one final line break, or no text before the first line break gave the right text before and still does. The new spec covers them in the split layouts as controls. - The blank-line-ending defect of the unified layout belongs to F11 (coder#535), not to this PR. - A pre-existing defect stays out of this PR. In the split layouts, a user who empties the proposal buffer after a proposal that ended in `"\n"` gets the answer `"\n"`, while Neovim writes an empty file for an empty buffer. It exists on main, and it is not changed here. The follow-up F16 fixes it. ## Blast Radius - User-visible: accepting a split-layout diff of a proposal without a final line break now sends that proposal as it is. A proposal with a final line break sends the same text as before. - With the default `'diffopt'` and `'fixeol'`, diff mode shows the same highlights, and the proposal buffer is modified as before. I probed both in a real Neovim. With `set nofixeol` and `diffopt-=internal`, one case highlights differently, which is a real difference between the endings. - `covered.lock`: the change touches `_setup_blocking_diff`, a covered block. `check.sh` printed STALE for the `diff.lua` file hash and the `setup_blocking` block hash only. It printed no changed call site, and all 21 code facts stay TRUE. The change adds one `nvim_buf_set_option` call on a buffer that the same function created a few lines before. It sits next to the other buffer option calls and runs in the same `pcall`. It does not change when the diff registers, when it answers, or how it resolves or closes. So I did not change the model (`model/DiffLifecycle.tla`). The model passed (`main`: 89205 distinct states, `live-DisconnectedCallsAnswered`: 5269). `check.sh --record` wrote the new lock and `check.sh --hash-only` reports that it matches. ### How this PR merges with F11 and F12 - F12 (`fix/server-auth-log-and-cwd-lookup`) merges with this branch without a conflict. I checked it with `git merge-tree --write-tree`. ### Merge rule for F11 (coder#535) and F14 F11 and F14 merge without a text conflict, but each leaves a test that the other makes stale. F11's spec lists 2 vertical cases as pending, because main still adds a final line break there. F14's spec skips 9 unified cases that end in blank lines, because main still drops one of those line breaks. Whichever of the two PRs lands second must do this in its own merge-from-main commit: 1. Apply the first diff below to F11's `tests/integration/diff_accept_trailing_newlines_spec.lua`. It replaces the 7-line `local case = it` pending clause with one `it(` line, so the 2 vertical no-final-line-break cases run. 2. Apply the second diff below to F14's `tests/integration/diff_accept_missing_final_newline_spec.lua`. It removes the `blank_end` skip (the `if not (layout == "unified" and proposal.blank_end) then` line and its `end`, the 3 `blank_end = true` fields and their comment), so the 9 unified blank-line cases run. 3. Show both specs green on the merged tree before its push. On a trial merge of the two branches with both diffs applied, F11's spec gives 28 successes, 0 failures and 0 pending. F14's spec gives 94 successes, 0 failures and 0 pending. <details> <summary>The two diffs</summary> ```diff diff --git a/tests/integration/diff_accept_trailing_newlines_spec.lua b/tests/integration/diff_accept_trailing_newlines_spec.lua index e619a02..53a1c73 100644 --- a/tests/integration/diff_accept_trailing_newlines_spec.lua +++ b/tests/integration/diff_accept_trailing_newlines_spec.lua @@ -78,13 +78,7 @@ describe("the text a proposed diff accepts", function() describe("in the " .. layout .. " layout", function() for _, old_file in ipairs(old_files) do for _, proposal in ipairs(proposals) do - -- The vertical layout writes a final line break after a proposal that has none. That is not what the - -- unified fix changes, so this case stays pending there. - local case = it - if layout == "vertical" and proposal.text:sub(-1) ~= "\n" then - case = pending - end - case(("keeps %s of a proposal against %s"):format(proposal.name, old_file.name), function() + it(("keeps %s of a proposal against %s"):format(proposal.name, old_file.name), function() local state = open_diff(layout, old_file.text, proposal.text) vim.api.nvim_set_current_win(state.new_window) ``` ```diff diff --git a/tests/integration/diff_accept_missing_final_newline_spec.lua b/tests/integration/diff_accept_missing_final_newline_spec.lua index dc501c6..d579a8f 100644 --- a/tests/integration/diff_accept_missing_final_newline_spec.lua +++ b/tests/integration/diff_accept_missing_final_newline_spec.lua @@ -71,8 +71,6 @@ describe("the text a proposed diff accepts, by how the proposal ends", function( end) end - -- `blank_end` marks a proposal that ends in a blank line. On main the unified layout drops one of those (F11 fixes - -- that), so this file runs them in the split layouts only. local proposals = { { name = "a final line without a line break", text = "a" }, { name = "two lines and no final line break", text = "a\nb" }, @@ -81,9 +79,9 @@ describe("the text a proposed diff accepts, by how the proposal ends", function( { name = "one final line break", text = "a\n" }, { name = "two lines and a final line break", text = "a\nb\n" }, { name = "a blank line only", text = "\n" }, - { name = "one blank line after the last line", text = "a\n\n", blank_end = true }, - { name = "two blank lines after the last line", text = "a\n\n\n", blank_end = true }, - { name = "two blank lines only", text = "\n\n", blank_end = true }, + { name = "one blank line after the last line", text = "a\n\n" }, + { name = "two blank lines after the last line", text = "a\n\n\n" }, + { name = "two blank lines only", text = "\n\n" }, } local old_files = { { name = "a file with other text", text = "old\n" }, @@ -95,13 +93,11 @@ describe("the text a proposed diff accepts, by how the proposal ends", function( describe("in the " .. layout .. " layout", function() for _, old_file in ipairs(old_files) do for _, proposal in ipairs(proposals) do - if not (layout == "unified" and proposal.blank_end) then - it(("accepts %s as it is, against %s"):format(proposal.name, old_file.name), function() - local state = open_diff(layout, old_file.text, proposal.text) + it(("accepts %s as it is, against %s"):format(proposal.name, old_file.name), function() + local state = open_diff(layout, old_file.text, proposal.text) - assert.equal(vim.inspect(proposal.text), vim.inspect(accept(state))) - end) - end + assert.equal(vim.inspect(proposal.text), vim.inspect(accept(state))) + end) end end end) ``` </details> ## Verification Red before the fix, green after (same spec, only `lua/claudecode/diff.lua` swapped): | Spec | origin/main | with the fix | | --- | --- | --- | | `tests/integration/diff_accept_missing_final_newline_spec.lua` (85 cases, real Neovim, `:write`) | 59 successes, 26 failures | 85 successes, 0 failures | The spec runs ten proposals against three old files (other text, no final line break, and no file yet) in the vertical and horizontal layouts. The proposals end in no line break (`"a"`, `"a\nb"`, an empty proposal, `"a\n\nb"`), in one line break (`"a\n"`, `"a\nb\n"`, `"\n"`) or in blank lines (`"a\n\n"`, `"a\n\n\n"`, `"\n\n"`). Each case compares the answer to the proposal byte for byte. The 26 red cases are the 24 no-line-break cases of the two split layouts and the 2 edit cases. The unified layout runs the 7 proposals that do not end in blank lines against the same three old files (21 cases, all green before and after). Four more cases append a line to the buffer after it opens, in both split layouts. They show that edits still reach Claude and keep the ending. Gates on the final commit, which merges `origin/main` (036427a) into the branch (private XDG folders): - `mise run all`: native exit 0. luacheck: 0 warnings / 0 errors in 179 files. busted: 1682 successes / 0 failures / 0 errors / 25 pending. treefmt: 0 files changed. - `mise run format-check`: native exit 0 (356 files, 0 changed). - Diff specs (`tests/unit/diff*_spec.lua`, `opendiff_blocking_spec`, `new_file_reject_then_reopen_spec`, and the `open_diff_mcp`, `close_tab` and `close_all_diff_tabs` specs): 222 successes, 0 failures. - `scripts/run_integration_tests_individually.sh`: 416 successes, 0 failed, 0 errors. The new spec gives 85 successes. - `.agents/skills/verify-diff-lifecycle/check.sh --hash-only`: covered.lock matches the code. Two independent verifiers (Opus and GPT) checked the code commit before the later spec-comment commit and the merge with `origin/main`. Both found no introduced defect. Their minor findings are in the branch (the spec comment and a case name) and in this body. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
hareki
pushed a commit
to hareki/claudecode.nvim
that referenced
this pull request
Nov 13, 2026
…split layouts (coder#551) ## Why When a user empties the proposal buffer (`ggdG` or `:%delete`) and saves it with `:w`, Claude must get what Neovim's own `:w` writes. Neovim writes no bytes for an emptied buffer. With `'eol'` on, the `vertical` and `horizontal` layouts answered `" "`, so Claude wrote a one-byte file where the user had asked for an empty one. The F14 verifier found this. PR coder#542 listed it as a known, pre-existing defect that this PR fixes. ## What changed `_resolve_diff_as_saved` in `lua/claudecode/diff.lua` saves the lines of the proposal buffer, joined with line breaks. It adds a final line break when the buffer's `'eol'` is on. A buffer with no lines at all (Neovim shows "--No lines in buffer--") still gives one empty string through `nvim_buf_get_lines`. So the save could not tell it from a buffer with one empty line, and it added the line break. The save now counts the bytes of the buffer with `vim.fn.wordcount().bytes`, run inside `nvim_buf_call`. The count is 0 for an empty buffer and at least 1 for a buffer with one empty line when `'eol'` is on. The line break is added only when the count is not 0 and `'eol'` is on. That replaces the check `#content_lines > 0`, which was always true. I tried `nvim_buf_get_offset` and `line2byte` first. Both gave wrong sizes for a buffer that `ggdG` had emptied (a stale line cache), so I did not use them. Other changes: - `tests/mocks/vim.lua` gets `wordcount()` and an `empty = true` flag for a mock buffer. `nvim_buf_call` in the mock now names the buffer that the callback acts on. - `tests/unit/diff_ui_cleanup_spec.lua` gets one fixture line. The mock's `nvim_buf_call` refuses a buffer that does not exist, and that spec saved a buffer that the mock never had. - `lua/claudecode/README.md`: one bullet replaces coder#542's bullet about the saved content of the split layouts. It states both rules, `'eol'` from the proposal and the empty buffer. It also says that a buffer with one empty line is saved as `"\n"` when `'eol'` is on and as `""` when it is off. - `.agents/skills/verify-diff-lifecycle/covered.lock` is re-recorded (see Blast Radius). ## Scope - The `vertical` and `horizontal` layouts share one code path, so one change fixes both. - The `unified` layout is unchanged. It answers with the proposal, not with the buffer, so what the user does to its buffer never reaches Claude. The new spec runs it as a control. - With `'eol'` on, a buffer with one empty line is still saved as `"\n"`. With `'eol'` off, it is saved as `""`, as before. A buffer with any line is saved as before. - On `main` (with coder#542), this PR changes the answer only for a buffer that the user emptied. It changes no answer for an unedited proposal: coder#542 already makes an empty proposal give `""`, and the new spec has that case as a control. ### Minor findings that I declined Two verifiers (Opus and GPT) found these. None of them changes an answer that a user reaches with the default settings. - **A buffer set to `bomb` (Opus F3).** With `ggdG`, then `:setlocal bomb`, then `:w`, Neovim writes the 3-byte BOM. Both `main` and this PR answer without the BOM (`main` answers `"\n"` when `'eol'` is on and `""` otherwise, this PR answers `""`). The plugin never sets `'bomb'` on its scratch proposal buffer, and no answer of the plugin ever holds a BOM, so a user has to set a non-default option on a scratch buffer first. I left it. - **The `nvim_buf_call` wrapper is covered by the mock spec only (Opus F4).** If I delete the wrapper, only the mock unit spec fails. In the paths that save (`BufWriteCmd` and `accept_current_diff`) the current buffer is always the proposal buffer, so the wrapper is defensive and no live answer changes. I kept the wrapper, because `wordcount()` reads the current buffer, and I added no live test for a case that no path reaches. - **Cost of `wordcount()` (Opus F5).** It adds a second pass over the buffer. For a 33 MB proposal that is about 0.1 to 0.2 s (0.12.3: +112 ms, 0.8.0: +216 ms), about the cost of `nvim_buf_get_lines` plus the join. I accept that for one save. ## Blast Radius - User-visible: a proposal buffer that the user emptied is accepted as an empty string. Every other buffer gives the same text as before. - `wordcount()` runs in the proposal buffer through `nvim_buf_call`. It does not change the user's current buffer or window. Both functions exist in Neovim 0.8. I ran the specs on Neovim 0.12.3 only. The CI job `integration-tests (v0.8.0)` runs the new spec on 0.8. - `covered.lock`: the change touches `_resolve_diff_as_saved`, a covered block. `check.sh` printed STALE for the `diff.lua` file hash and the `resolve_saved` block hash only. It printed no changed call site, and all 21 code facts stay TRUE. The guard that lets only a `pending` diff resolve is unchanged, and so are the status change to `saved` and the one call of the resolution callback. The change only decides whether the `text` string of the `FILE_SAVED` answer ends in a line break. The model stores which kind of answer a call got, not its text, so no modelled transition changes. The model passed (`main`: 89205 distinct states, `live-DisconnectedCallsAnswered`: 5269). `check.sh --record` wrote the new lock and `check.sh --hash-only` reports that it matches. ### Merge with coder#542, coder#539 and coder#535 coder#542 (F14) landed first, on 2026-11-12 at 04:36Z, and coder#539 (F12) at 04:46Z. coder#538 and coder#550 later changed `diff.lua` too, and coder#549 renamed "proposed buffer" to "proposal buffer" in the README. This PR merged `origin/main` (9bfc39c) with merge commits. It ran `check.sh --record` after each change to `diff.lua` and kept main's README wording in the last one. Only the `diff.lua` file hash and the `resolve_saved` block hash differ from the lock on `main`. The full `check.sh` passes and `check.sh --hash-only` matches. - One README bullet replaces this branch's bullet and coder#542's bullet, because the two said almost the same sentence and this branch's sentence about one empty line was false once `'eol'` can be off. - coder#535 (F11) is still open. It touches neither `diff.lua` nor this README bullet. Its current head (`a5dfc81b`, with `main` merged) merges with this PR without a conflict. ## Verification Red before the fix, green after (same specs, only `lua/claudecode/diff.lua` swapped, on the merged head, so the red column is `main` at 9bfc39c with coder#542): | Spec | `main` (with coder#542) | with the fix | | --- | --- | --- | | `tests/unit/diff_saved_text_spec.lua` (4 cases, mock) | 3 successes, 1 failure | 4 successes, 0 failures | | `tests/integration/diff_accept_emptied_buffer_spec.lua` (40 cases, real Neovim, `:write`) | 28 successes, 12 failures | 40 successes, 0 failures | Against the older merge base (5acc100, before coder#542), the 38-case spec of round a had 24 red cases. Today's 40-case spec has 26 there: the 24 emptied cases plus the new control for an unedited empty proposal in both layouts. coder#542 fixes the 12 cases from proposals without a final line break (it sets `'eol'` off), so 12 stay red on `main`. The integration spec opens a real `openDiff` and edits the proposal window. For the `vertical` and `horizontal` layouts it runs 6 proposals (two lines and a final line break, one line and a final line break, one blank line, one line with no final line break, two lines with no final line break, and no text) with 2 ways to empty the buffer (`ggdG` and `:%delete`). Each case compares the answer with what Neovim's own `:w` writes: the spec edits a real file buffer with the same text and commands, runs `:write`, and reads the file bytes, which are empty. Five controls per layout give the right text before and after (a blank line left in the buffer, no edit, an empty proposal without an edit, one line deleted, and text typed into the emptied buffer). In the `unified` layout, 6 controls check that the answer is the proposal. Gates on the final commit, which merges `origin/main` (9bfc39c) into the branch (private XDG folders): - `mise run all`: native exit 0. luacheck: 0 warnings / 0 errors in 183 files. busted: 1710 successes / 0 failures / 0 errors / 26 pending. treefmt: 0 files changed. - `mise run format-check`: native exit 0 (360 files, 0 changed). - Diff specs (`tests/unit/diff*_spec.lua`, `opendiff_blocking_spec`, `new_file_reject_then_reopen_spec`, and the `open_diff_mcp`, `close_tab` and `close_all_diff_tabs` specs): 226 successes, 0 failures. - `scripts/run_integration_tests_individually.sh`: 457 successes, 0 failed, 0 errors. The new spec gives 40 successes. - `.agents/skills/verify-diff-lifecycle/check.sh --hash-only`: covered.lock matches the code. Two independent verifiers (Opus and GPT) checked the code at `ef8be297`. Both found the code correct. Their blockers were the README sentence and the stale base, and this head fixes both. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
hareki
pushed a commit
to hareki/claudecode.nvim
that referenced
this pull request
Nov 13, 2026
… other path characters (coder#561) ## Why `cwd.git_root` must find the repository of a folder whatever its name is. A repository in a folder with a space or a comma in its path was not found when git could not run: the fallback gave nil, or the repository of Neovim's current folder. A folder named `[x] {y}` or `$HOME-like` was lost or changed even when git answered. A verifier of F12 found the first case (finding F12-P1, a space or comma in the path makes the `.git` fallback return nil). ## What changed Four problems in `lua/claudecode/cwd.lua`. The first two took a path as a pattern. The third stepped one folder down at a trailing `..`. The fourth is a flaw in my first version of the second step. 1. **The `.git` fallback** called `vim.fn.finddir(".git", start_dir .. ";")`. `finddir` reads its path as a list in `'path'` option syntax. A space or a comma split the path, and a `;` ended it. The fallback now walks up from the full path of `start_dir` and tests each folder for a `.git` folder with `vim.fn.isdirectory`. A relative `start_dir` now reaches the folders above the current one too. Before, a relative start gave a wrong answer or nil in some cases (for example `git_root("sub/deeper")` in the repository root). 2. **`normalize_dir`** passed every path through `vim.fn.expand`. `expand` reads `[`, `{`, `*`, `?`, `$` and `\` in a name as patterns or variables. A folder named `[x] {y}` or `it's` came back with a trailing slash. A folder named `$HOME-like` or `back\slash` came back nil. This also happened when `git rev-parse` answered. `normalize_dir` now returns a directory that exists as written, and expands any other string (`~`, `$VAR`). See step 4 for text that starts with `~` or `$`. 3. **`..` before Neovim 0.11.** Before 0.11, `fnamemodify(path, ":p")` keeps a last `..`, and `:h` of `a/b/..` is `a/b`. So a walk from a start folder that ends in `..` stepped one folder down and returned the wrong root. The walk now calls `simplify()` on the full path first. A verifier found this. 4. **Text that starts with `~` or `$`.** The first version of step 2 took a path that names an existing folder as written, on the raw text. `vim.fn.isdirectory` does not expand, so with `cwd = "~"` or `"$HOME"` and a stray folder of that literal name in Neovim's current folder (for example one that an unexpanded `mkdir "~/x"` made), `git_root` and `resolve` returned the stray folder where main returned the home folder. `normalize_dir` now sends text that starts with `~` or `$` through `expand` first, as a shell and `:cd` do. A verifier of the terminal change found this. `lua/claudecode/README.md` gets one new bullet that states this, with the rule for text that starts with `~` or `$`. It sits after the "Terminal cwd precedence" bullet. It names `git_root` and `resolve` only, and it says that the terminal still passes a static `cwd` and a `cwd_provider` result through `vim.fn.expand` itself. ## Scope - Only `cwd.lua` changes in code. Steps 1 and 2 are one change of idea: a path is a literal, not a pattern. Step 3 is a quirk of `fnamemodify` before Neovim 0.11. Step 4 closes a hole that step 2 opened. - `normalize_dir` serves `cwd.git_root` and `cwd.resolve`. Nothing in `lua/` calls `cwd.resolve`. The terminal calls `cwd.git_root` when `git_repo_cwd` is set, so this PR fixes folder names with `$` and `\` on that path: when git does not answer, the terminal now gets `$HOME-like`, `back\slash`, `[x] {y}`, a space and a comma right, where main gave nil or the repository of Neovim's current folder. A static `cwd` and a `cwd_provider` result do not go through `normalize_dir`: `terminal.lua:build_config` passes them through `vim.fn.expand` itself, so this PR does not change what the terminal does with those. An absolute start path that ends in `..` is fixed here, through the `simplify()` call above. - Two gaps stay for separate follow-ups. Main already has both, and this PR does not reach them. First, the fallback finds a `.git` folder only, not a `.git` file in a worktree or submodule. Second, the walk is lexical, so a link into another repository answers with the repository of the link's own folder, where git answers with the link target's repository. The terminal's `expand` of a static `cwd` and of a provider result is a third follow-up, and it has its own change. - `~` and `$VAR` still expand in `cwd.resolve` (the spec checks both). The spec's cases for `cwd.resolve` are named as `cwd.resolve` cases, because they do not test the terminal. - `git rev-parse` still runs first. Its result for a plain path is the same string as before. ## Blast Radius - User-visible: a repository in a folder with a space, a comma, a quote or another path character is now found when git cannot run. With git, `git_root` returns a folder with `[`, `{`, `*`, `?`, `$` or `\` in its name exactly. It does not reach the terminal through a `cwd_provider`: `build_config` still expands the provider's result, so the root README's example still loses `$` and `\` there (follow-up). A plain path gives the same result as before. - The fallback result is an absolute path with no `..` part. Before it could be relative. The one exception is a relative start whose name begins with `~`: Vim reads `~` as the home folder and `~name` as a user's home folder, so `:p` leaves `~name` relative and the result can be the relative `~name`. That is a pathological case, and no caller in `lua/` passes a relative path (the terminal passes the file's folder from `expand("%:p")` or `getcwd()`). - A string that names an existing folder is taken as written, except text that starts with `~` or `$`. That text always goes through `expand` first, so a folder named `~` or `$HOME` in Neovim's current folder does not take the place of the home folder, as on main. The cost: a relative folder name that starts with `$` and reads as a variable (`$HOME-like`, `$odd/sub`) is expanded and not found, as on main. A lone relative `$odd` that names no variable is left unchanged by Vim and is still found. An absolute path never starts with `~` or `$`, so every folder name this PR fixes keeps working. The change applies to `git_root`'s `start_dir` and to `cwd.resolve`. The terminal's own `expand` of a static `cwd` and a provider result is a separate change. - Neovim 0.8: the code uses `vim.fn.isdirectory`, `vim.fn.fnamemodify` and `vim.fn.simplify`, which exist there. I ran the new spec on Neovim 0.8.0, 0.10.0 and 0.12.3. The `..` case fails on 0.8.0 and 0.10.0 without the `simplify()` call, and passes on 0.12.3 either way. - `covered.lock`: `cwd.lua` is the terminal's working-directory lookup. `check.sh` printed STALE for the file hash of `cwd.lua` only, with no changed block or call site. The change cannot reach diff state, an openDiff answer, a disconnect or a diff command. The model passed (`main`: 89205 distinct states, `live-DisconnectedCallsAnswered`: 5269). `check.sh --record` wrote the new lock and `check.sh --hash-only` reports that it matches. ### Merge with coder#539 (F12) coder#539 landed first. This PR's merge commit with `origin/main` resolves the interplay: it re-records `covered.lock` and changes the one stale test in coder#539's `cwd_spec.lua` (see Verification). F11 (coder#535) merges with this PR without a conflict. ## Verification Red before the fix, green after (same spec, only `lua/claudecode/cwd.lua` swapped): | Spec | origin/main | with the fix | | --- | --- | --- | | `tests/integration/cwd_path_characters_spec.lua` (37 cases, real Neovim, 0.8.0 and 0.12.3) | 20 successes, 17 failures | 37 successes, 0 failures | | the new case "goes up from a start folder that ends in .." at `ede2f733` (before the `simplify()` call) | fails on 0.8.0 and 0.10.0, passes on 0.12.3 | passes on 0.8.0, 0.10.0 and 0.12.3 (35 successes when added, 37 at the final head, 0 failures on each) | | the two cases "expands a leading ~ / $ even when Neovim's cwd holds a folder named ~ / $HOME", with `cwd.lua` from `ca9e61c0` (before the guard) | 35 successes, 2 failures on 0.8.0 and 0.12.3 | 37 successes, 0 failures on each | The spec makes real folders and runs `cwd.git_root` on them, in two modes. In the first mode git is off `PATH` (the shell keeps its absolute path), so `git` ends with "not found" and a list command raises E475, as on a host without git. In the second mode a real `git init` makes the repository. The folder names are: a space, a comma, a semicolon, single and double quotes, brackets and braces, parentheses, non-ASCII letters, a star and a question mark, a percent sign and a hash, a backslash, a dollar sign, a leading tilde, and a plain name. Each name runs from the repository folder and from a folder two levels below it, and the answer must equal the repository path with no trailing slash. A folder that is not a repository returns nil, whatever its name. Seven more cases cover a relative start folder, a start folder that ends in `..`, `cwd.resolve` with `git_repo_cwd`, a static `cwd` with brackets and a star in `cwd.resolve`, `~` and `$VAR` in a setting of `cwd.resolve`, and two cases where Neovim's current folder holds a folder named `~` or `$HOME` and the setting must still expand. The 17 red cases are 10 of the no-git names, 4 of the git names, the relative start, `resolve`, and the static `cwd`. Gates on the final head `f12368bd`, which contains a merge of `origin/main` (05b9785, with coder#551, coder#535, coder#494, coder#545, coder#553, coder#555 and coder#557) (private XDG folders). An independent verifier passed `c73ab044`. Since then the branch has three changes of its own and two merges with main. The changes are a docs commit (the README sentence about where the walk starts), the fix for text that starts with `~` or `$` (step 4, with two spec cases and the README clause), and the `covered.lock` record for it. For the lock I ran the full `check.sh`: the model passed (89205 distinct states, 5269 live states, every fact TRUE), and only the file hash of `cwd.lua` was stale, with no changed block or call site. - `mise run all`: native exit 0. luacheck: 0 warnings / 0 errors in 186 files. busted: 1756 successes / 0 failures / 0 errors / 29 pending. treefmt: 0 files changed. Since coder#545, `mise run all` also runs the static proofs of the `verify-claudecode-nvim` skill, and they pass. - `mise run format-check`: native exit 0 (428 files, 0 changed). - `scripts/run_integration_tests_individually.sh` on Neovim 0.12.3: 564 successes, 0 failed, 0 errors, native exit 0. On Neovim 0.8.0: 427 successes, 0 failed, 0 errors, native exit 0 (the 0.8.0 run skips the specs that need 0.9). The new spec gives 37 successes on both. It starts about 100 processes (git and a shell), so it takes about 6 s on a quiet host and can pass the runner's 40 s limit at a load average above 100. A TIMEOUT line there means the host was slow, not that the spec hung. - All four `.agents/skills/verify-*/check.sh --hash-only` match their `covered.lock`. The merge with `origin/main` had two parts to resolve in an earlier commit. `covered.lock` conflicted on the hash line of `cwd.lua`, so I ran the full `check.sh` and `check.sh --record`. coder#539's `tests/unit/cwd_spec.lua` had a test that stubbed `vim.fn.finddir`, which the fallback no longer calls, so that test now walks up real folders. It does not depend on where `TMPDIR` is: it passes with `TMPDIR` inside a git checkout too. `cwd.lua` itself merged without a text conflict. The two latest merges needed no `covered.lock` change: coder#551 conflicted only in `tests/mocks/vim.lua`, where the branch keeps both its `simplify` entry and coder#551's `wordcount`, and coder#535 and coder#494 merged without a conflict. An independent verifier found the `..` case, the README wording and the other points that this body now states. The `terminal.lua` expand and `.git` files in worktrees are gaps that main already has. They are follow-ups, not part of this PR. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
hareki
pushed a commit
to hareki/claudecode.nvim
that referenced
this pull request
Nov 13, 2026
…ak spec (coder#571) ## Why Two integration specs ran the same test for the accepted text of a diff. `diff_accept_trailing_newlines_spec.lua` (88 lines, from coder#535) opens a diff, accepts it unedited, and compares the answer with the proposal. `diff_accept_missing_final_newline_spec.lua` (from coder#542) does the same over a larger table of layouts, old files and proposals. Both had the same setup and the same four assertions. Twelve of the 28 cases of the trailing spec were also rows of the other one. A second copy of the loop means two places to fix when the accept path changes. ## What changed Only two files change, both under `tests/integration/`. No file under `lua/` or `plugin/` changes. 1. `diff_accept_missing_final_newline_spec.lua` gets two rows. These were the only cases that the trailing spec ran and the table did not: - the proposal `"a\n\nb\n\n"` ("a blank line inside and one after the last line"), and - the old file `"a\n"` ("a file that is a line shorter"). 2. `diff_accept_trailing_newlines_spec.lua` is deleted. 3. The header comment of the table now states which proposals the unified layout lost a line break for. Only a proposal that ends in `"\n\n"` lost one (`"a\n\n"` came back as `"a\n"`). A proposal that ends in a single `"\n"`, such as `"a\n"` or `"\n"`, kept it. The merged spec has 136 tests: 3 layouts × 4 old files × 11 proposals, plus the 4 edit tests that were already there. Before, the two specs had 94 + 28 = 122 tests. ## Nothing is lost 1. All 28 cases of the deleted spec are in the merged spec. All 90 matrix cases of the table before this PR are still in it. 2. The four assertions of the deleted spec are in the table's own helpers: openDiff waits for the user (`call._deferred`), exactly one answer, the answer is `FILE_SAVED`, and the accepted text equals the proposal. 3. Nothing else in the tree names the deleted file (`git grep` for its name, its describe title and `trailing_newlines_spec`: 0 hits at the head and on `origin/main`). ## Verification: the merged spec still catches the bug of coder#535 I reverted coder#535 (the reverse patch of `884f7e22` on `lua/claudecode/diff_inline.lua`) and ran each spec alone on a copy of the head. | Spec | head | coder#535 reverted | |---|---|---| | old trailing spec | 28 ok | 20 ok, 8 failed | | old table spec | 94 ok | 85 ok, 9 failed | | merged spec (this PR) | 136 ok | 120 ok, 16 failed | (The first two rows ran the two specs of the base of this PR, `37ee1e35`, on Neovim 0.12.3 and 0.8.0, with the same counts on both. The merged spec's row ran on Neovim 0.12.3, 0.11.5, 0.10.4, 0.9.5 and 0.8.0, with the same counts on all five.) 1. The 16 failures are the unified layout × 4 old files × the four proposals that end in `"\n\n"`: `"a\n\n"`, `"a\n\n\n"`, `"\n\n"` and `"a\n\nb\n\n"`. Proposals that end in a single `"\n"` pass with coder#535 reverted, as they did before coder#535. 2. Every case that failed in either old spec with coder#535 reverted also fails in the merged spec (the 14 failures of the two old specs, 3 of them shared, are all among the 16). 3. Two of the 16 are new: `"a\n\nb\n\n"` against `"old"` (a file without a final line break) and against a file that does not exist. They come from the new proposal row. ## Gates The head is `1f736d0d95154574376390c92f55bbcf3ad308a3`. It merges `origin/main` at `5c7a4db9`. The full gates ran at `76b50be9`, the merge before it. The commit after it brings only `terminal.lua`, `scripts/`, docs and the new `tests/integration/terminal_cwd_spec.lua` from main, and nothing in the two specs of this PR. At the head I ran `format-check`, `check.sh --hash-only` of all four skills, and the merged spec on Neovim 0.12.3 and 0.8.0 (136 ok each). 1. `mise run all` (`BUSTED_SEED=97531`) exits 0: 1832 successes, 0 failures, 0 errors, 29 pending. luacheck: 0 warnings, 0 errors in 189 files. 2. `mise run format-check` exits 0 at both heads (432 files, 0 changed at the last one). 3. `scripts/run_integration_tests_individually.sh` exits 0 on Neovim 0.12.3 (Total Success 614, Failed 0, Errors 0) and on Neovim 0.8.0 (Total Success 596, Failed 0, Errors 0). 4. `check.sh --hash-only` of `verify-diff-lifecycle`, `verify-mention-queue`, `verify-selection-tracking` and `verify-terminal-lifecycle` exits 0, at both heads. The full `check.sh` of `verify-diff-lifecycle` exits 0 (`main` 93477 distinct states, `live-DisconnectedCallsAnswered` 5557). The PR changes no covered file. One comment line of the table spec is 123 columns wide. It comes from coder#542 and I left it as it was. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A unified diff must hand Claude back the exact text it proposed when the user accepts it. A proposal that ends in a blank line lost one line break. A proposed file ending
"a\n\n"was accepted as"a\n", so Claude wrote a file one blank line shorter than the one the user approved.What changed
resolve_inline_as_savedinlua/claudecode/diff_inline.luajoined the accepted lines with"\n". It added the final"\n"only when the join did not already end in one. A proposal that ends in blank lines has a blank last line, so the join already ended in"\n"and the final line break was never added back.The function now adds the final
"\n"whenever the proposal ends with one. The lines never carry a line break after the last one, so this is always one line break short.Other changes:
lua/claudecode/README.mdstates theunifiedcontract on the line that describes theFILE_SAVEDcontent..agents/skills/verify-diff-lifecycle/covered.lockis re-recorded (see Blast Radius).Scope
Only the
unifiedlayout changes. Theverticalandhorizontallayouts keep their code and their results.A separate defect used to stay out of this PR. In both the
verticaland thehorizontallayout, a proposal without a final line break was accepted with one. F14 (#542) fixed it and is now inmain. The new integration spec runs theverticallayout, and its 2 cases without a final line break run as active tests (they were pending before F14 landed).Blast Radius
covered.lock: the change touchesresolve_inline_as_saved, a covered block. It changes only thetextstring in theFILE_SAVEDanswer. It does not change the saved status, the answer count, the resolution callback order or disconnect handling. Diff commands make the same decisions.:ClaudeCodeCloseAllDiffsputs the answer text into the file's buffer, so for a saved unified diff it now keeps the trailing blank lines too. The model does not represent answer text: it stores which kind of answer a call got, so no modelled transition changes.check.shprinted STALE for exactly two entries (the file hash and theinline_savedblock hash ofdiff_inline.lua) and no changed call site. The model passed (main: 89205 distinct states,live-DisconnectedCallsAnswered: 5269).check.sh --recordwrote the new lock andcheck.sh --hash-onlythen reports that it matches.Merge with F14 (#542)
F14 (#542) landed first, as 49df75a. The two PRs left a test that each other made stale, so this PR owed the merge rule, and its merge commit with
origin/main(0b0de5b) applies it:tests/integration/diff_accept_trailing_newlines_spec.lua: the 7-linelocal case = itpending clause is replaced with oneit(line, so the 2 vertical no-final-line-break cases run.tests/integration/diff_accept_missing_final_newline_spec.lua: theblank_endskip is removed (theif not (layout == "unified" and proposal.blank_end) thenline and itsend, the 3blank_end = truefields and their comment), so the 9 unified blank-line cases run.On the merged head, F11's spec gives 28 successes, 0 failures and 0 pending. F14's spec gives 94 successes, 0 failures and 0 pending. Without this PR's fix (
diff_inline.luafrommain), F11's spec gives 20 successes and 8 failures, and F14's spec gives 85 successes and 9 failures. All of those failures are unified blank-line cases.Verification
Red before the fix, green after (same specs, only
diff_inline.luaswapped, on the merged head, somainalready has F14):tests/unit/diff_inline_spec.lua(24 new cases)tests/integration/diff_accept_trailing_newlines_spec.lua(28 cases, real Neovim)unified)tests/integration/diff_accept_missing_final_newline_spec.lua(F14's spec, 94 cases, real Neovim)unified)The unit cases run
compute_inline_diffandresolve_inline_as_savedfor each proposal and compare the accepted text byte for byte. The integration spec opens a realopenDiffin both layouts, runs:writein the proposal window and compares the answer byte for byte. It uses proposals ending in"\n\n","\n\n\n", no line break, a single"\n","\n"only,"\n\n"only and a blank line inside plus one at the end, each against a file with other text and against a file one line shorter.The audit's own probe in a real Neovim shows the same change: proposal
"a\n\n"was accepted as"a\n"and is now accepted as"a\n\n".Gates on the final commit, which merges
origin/main(9bfc39c) into the branch (the merge rule was applied in the earlier merge commit 0b0de5b) (private XDG folders):mise run all: native exit 0. luacheck: 0 warnings / 0 errors in 182 files. busted: 1730 successes / 0 failures / 0 errors / 26 pending. treefmt: 0 files changed.mise run format-check: native exit 0 (359 files, 0 changed).diff_inline_spec,diff_inline_new_tab_spec,diff_inline_quit_spec,diff_spec,opendiff_blocking_spec): 83 successes, 0 failures.scripts/run_integration_tests_individually.sh: 454 successes, 0 failed, 0 errors..agents/skills/verify-diff-lifecycle/check.sh --hash-only: covered.lock matches the code.Rounds 1 and 2 (heads 7e6f819 and 0b0de5b): CI green and both Codex lanes clean. Round 3 is this head, after a merge with
mainfor a README conflict (#549). Two independent verifiers (Opus and GPT) checked the code commit before the first merge withorigin/main. Both found no introduced defect. They reran the red and green specs, reverted the code change alone to see the new specs fail, and compared the accepted bytes through a real WebSocket call.Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh