Repository navigation
fix(diff): remove the legacy diff's temp file and its buffer when a step fails - #516
Merged
Merged
Conversation
…n a step fails
The public legacy `require("claudecode.diff").open_diff` (through `_open_native_diff`) registers the autocmd that
deletes its temp file after every step that can raise. A failure after the temp file was written left it behind,
with a half-made diff:
- A second open_diff for a path whose proposal is still open raised E95 at the rename to "<path> (New)".
- A changed swap file for the original made `edit` raise E325.
- An error in a FileType autocmd for the filetype of the original raised while the proposal was made (from the edit
of the temp file, or on Neovim 0.8 from setting the filetype).
The steps after create_temp_file now run in `show_native_diff`, which records what it changes. When it raises,
`undo_native_diff` turns the diff mode of the original off, wipes the proposal's buffer, closes the windows that the
diff split off, goes back to the window that was current, and deletes the temp file. The error is raised again as
before, so callers see the same failure with nothing left behind.
Test: tests/integration/diff_legacy_cleanup_spec.lua has one case per path. All three fail without the change on
Neovim 0.12.3 and v0.8.0 and pass with it. The verify-diff-lifecycle lock is re-recorded: only the hash of diff.lua
changed, and `_open_native_diff` is outside the model.
---
_Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
…IR in the legacy diff specs Both legacy diff specs leave a swap file by killing a child Neovim with SIGKILL after :preserve. A killed Neovim cannot delete its socket folder, so each run left $TMPDIR/nvim.$USER/<random>/ with a dead socket behind. The child now starts with TMPDIR and XDG_RUNTIME_DIR set to a folder inside the spec's own directory, which after_each deletes. With TMPDIR pointing at an empty folder, both specs left 2 socket folders before and 0 after, on Neovim 0.12.3 and v0.8.0. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
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 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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7aee3dbeb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…ep fails Codex round 1 on the rollback of the legacy diff found that it closed every window of the starting tab that was not there before. A window that an autocmd opened while the diff set up (a BufRead, BufEnter or FileType handler) was closed too. The diff also remembered the tab it started in, although it can open in the editor window of another tab. show_native_diff now records the handle of each window that its own splits create, and undo_native_diff closes exactly those, in whatever tab they are. The window and tab the user started in are made current again. Each rollback step is protected, so an autocmd error in one of them cannot stop the next one or replace the error that made the diff fail. Test: a case where a FileType autocmd opens its own window and then fails is red on the previous commit on Neovim 0.12.3 and v0.8.0 (the autocmd's window was closed) and green now. A case where the diff opens in another tab passes on both, and guards the new bookkeeping. The verify-diff-lifecycle lock is re-recorded: only the hash of diff.lua changed. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
🛡️ 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf1d049b22
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…n diff alone when a step fails The legacy diff found its new window as the current window after create_split(), so a WinEnter handler that changed the window made the rollback close the wrong one. It now finds the new window by comparing the tab's windows before and after the split. It also turned diff mode off in the window it reused, even when that window was already in a diff of the user's own. It now turns diff off only in a window where its own diffthis enabled it.
…acy diff rolls back A verifier found four ways that the rollback of a failed legacy open_diff touched what was not the diff's own. 1. diffoff ran on the window of the original even when that window was in diff mode before the call. The window can be the one of an earlier diff or of the user's own :diffthis. It can also be a window that `edit` put in diff mode, because `edit` copies the window options of another window that shows the file. diff mode is now asked before the edit, and diffoff runs only where the diff turned it on. 2. split() took the current window after the split for the diff's own. A WinNew or WinEnter handler that opens or enters a window of its own made the rollback close that window by force, and wipe unsaved text in a buffer with bufhidden=wipe. split() now finds the new window by comparing the tab's windows before and after, enters it, so that the steps that follow work there, and records it also when a handler raises. A rollback closes windows without force. 3. A FileType autocmd that raises while `edit` loads the temp file left the proposal's buffer behind, under the temp file's name, and Neovim keeps an unlisted buffer for the old name after the rename. The rollback now wipes the buffer that has the temp file's name. The spec runs each of these, and its cases 3 to 5 now assert that the call raises.
Only the hash of diff.lua changed. No covered block or call site changed: the legacy open_diff path (_open_native_diff and what it calls) is outside the model. The model passes (89,205 distinct states).
…cache folder The merged CI step from #520 fails the integration job when a spec leaves a folder under the real cache path. The cleanup spec calls the legacy open_diff, which writes its temp files under stdpath("cache"), so it points XDG_CACHE_HOME at its own folder, as the swap spec does.
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
🛡️ 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1280864a8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…by its name Codex round 3 on 1280864 raised two cases that a handler can reach. 1. split() listed the windows of "the current tab" after the split. A WinNew or WinEnter handler that switches to another tab made it list that tab: a window that was already there counted as the diff's own, the steps that followed worked there, and a rollback could close it while the real split stayed. The tab is now taken before the split and listed by its handle. 2. The proposal's buffer was whatever buffer was current after the edit. A BufEnter or FileType handler can leave another buffer current without raising. The rename and the options then reached that buffer, and a rollback wiped it with force. The proposal is now found by the temp file's name, and the diff stops with an error when it is not current, so the rollback wipes the proposal and nothing else. Two spec cases fail on 1280864 and pass here, on Neovim 0.12.3 and 0.8.0.
Only the hash of diff.lua changed. No covered block or call site changed. The model passes (89,205 distinct states).
The unit specs mock vim without vim.fs, and the match only has to make the separators the same: Neovim can write backslashes on Windows.
Only the hash of diff.lua changed. No covered block or call site changed.
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
🛡️ 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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d6b24b6f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
…and leave the preview window first Codex round 4 on 1d6b24b raised three cases. 1. A BufEnter handler can wipe the proposal and leave a new buffer of the user's. The name lookup found nothing and the diff took the current buffer for the proposal, so a later failure wiped it with force. The diff now stops when no buffer has the temp file's name and the current buffer does not show the temp file. Neovim resolves a link in a buffer's name, so the lookup also matches the real path, and the current buffer is compared by device and inode, not by spelling. When Neovim cannot compare the files, the old behavior stays. 2. split() listed the windows before create_split() left the preview window, so a WinEnter handler that opened a window there looked like the diff's split. split() now leaves the preview window first. Neovim 0.8 raises when a handler moves the focus during that switch, so the call is protected. 3. The cleanup spec shared one Neovim between cases and left tab pages behind. after_each closes them. Three integration cases and three unit cases fail without these changes.
Only the hash of diff.lua changed. No covered block or call site changed.
…and its buffer Four review rounds found problems in the same place: a rollback that closed windows, turned diff mode off and moved focus after a failed legacy open_diff can reach a window, a buffer or a diff that is not the diff's own, and every case that a reviewer found was one more autocmd that changes the layout under it. The rollback is gone. When a step fails after the temp file was written, _open_native_diff now does two things and raises the same error: 1. It wipes the buffer that has the temp file's name (found by the name or the real path, because Neovim resolves links in buffer names), but not when a window shows it or it has unsaved changes. 2. It deletes the temp file and its folder. It closes no window, moves no focus and turns no diff mode off. The windows that the failed call had already split stay, as they did before. Nothing in the plugin calls this path. The cleanup spec is rewritten: each case checks that no temp file is left, and the cases for the sidebar, an open diff, the user's own diff, autocmd windows, a window with unsaved text and a modified proposal check that nothing of the user's is closed, turned off or wiped.
Only the hash of diff.lua changed. No covered block or call site changed: the legacy open_diff path is outside the model. The model passes (89,205 distinct states).
ThomasK33
changed the title
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
|
Codex Review: Didn't find any major issues. Hooray! 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. |
…window of another tab shows The window guard in remove_failed_proposal had no case for a window of another tab, so a mutant that looked at the current tab only passed all 17 cases. Add that case, and say in the README that a window which still shows the proposal sees E211 once on the next :checktime.
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c99ad57934
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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
|
Review-cap waiver (from the maintainer, 2026-11-12): this PR may merge at the six-round cap. Its last Opus verifier passed with minor findings only (README links, three test gaps, a note that Generated with |
…cleanup # Conflicts: # .agents/skills/verify-diff-lifecycle/covered.lock # lua/claudecode/README.md
This was referenced Nov 12, 2026
hareki
pushed a commit
to hareki/claudecode.nvim
that referenced
this pull request
Nov 13, 2026
… an openDiff setup (coder#574) ## Why The goal of this series: land the behavior fixes and a code-shape refactor of this plugin on main in small PRs, fixes first. This PR is one fix. On Neovim 0.8 and 0.9, a Lua autocmd handler of the user can make Neovim exit at once (exit 134 or 139) while the plugin opens or closes the windows of an `openDiff` diff. A user can lose the unsaved changes of open buffers when that happens. coder#538 fixed one case. Other cases still abort. The cause, in three sentences. Neovim 0.8 and 0.9 bundle a LuaJIT older than its commit e86990f7f of 2023-09-21, which fixes LuaJIT issue 1066: after a C function runs Lua on another thread and then raises into a `pcall`, `cur_L` still names that thread, and the next exit from a compiled trace crashes (the issue shows a crash in `lj_trace_exit`). A Neovim API call in a coroutine that runs a Lua callback and then raises behaves like that C function, and Neovim aborts (measured, below). The plugin runs `openDiff` in a coroutine, and Neovim runs the handlers of the user while the plugin opens and closes its windows. What a source shows: the issue text and the patch of e86990f7f, which sets `cur_L` again in `err_unwind` (`src/lj_err.c`, case `FRAME_PCALL`). We did not read the LuaJIT source for other code that uses `cur_L`. The proof below measures what happens instead. ## What changed - `utils.resume_tool_coroutine(co, ...)` is new. On an affected LuaJIT it turns the JIT off and flushes the compiled traces before it resumes the coroutine, and it gives the JIT back as it was when the resume returns. Everywhere else it is a plain `coroutine.resume`. It returns exactly what `coroutine.resume` returns, `nil` values and their count included. - "Affected" means: the global `jit` exists and `jit.version` is not a rolling `2.1.<n>` release with `n` of 1695261248 or more. `LuaJIT 2.1.0-beta3` (Neovim v0.8.0) and `LuaJIT 2.1.1692716794` (Neovim v0.9.5) are affected. A version that is no string, or that the check cannot read, counts as affected, which costs only a flush. - `handle_invoke` and the answer to a blocking diff (`open_diff_blocking`) resume the tool's coroutine through it. `git grep -n 'coroutine\.resume' -- lua plugin` shows one code line, inside the helper. - coder#538's call (`nvim_get_current_tabpage()` before the rethrow in `_setup_blocking_diff`) and its comment are gone. The cases of coder#538's spec stay, and their assertions are unchanged. Only the comment above the eager-JIT case changed. - `.agents/skills/verify-diff-lifecycle/check.sh`: the fact `SetupAnswerReturned` now needs the assignment line `local ... = utils.resume_tool_coroutine(co, result)`. A comment on a line of its own cannot satisfy it. A comment at the end of a plain-resume line leaves the fact TRUE, and `check.sh` then exits 3 (stale). The job `covered-lock` runs every `check.sh` and fails on any non-zero exit, exit 3 included (`.github/workflows/test.yml`, the loop with `status=1`). The model is unchanged. - Docs: `lua/claudecode/tools/README.md` names the helper, the LuaJIT condition and the cost, and `lua/claudecode/README.md` lists the helper. The comment above the helper in `utils.lua` names the LuaJIT builds and the Neovim versions that the bug affects. ## Why coder#538's call goes away coder#538's call is on main in every run below, and Neovim still died. The probe runs the tool path (`tools.handle_invoke`) with 8 raising handlers, 2 passes, and an eager JIT. Two verifiers measured it on two main commits. Each cell counts the runs that killed Neovim, of 30. The counts depend on the host and its load: the same probe on main gave 13 of 30 in one run and 8 of 30 twice on v0.9.5. | Tree | v0.8.0 | v0.9.5 | | --- | --- | --- | | main `884f7e22` (round 1 verifier) | 27 | 13 | | main `523ee200` (round 2 verifier) | 24 | 8 and 8 | | this PR (both verifiers) | 0 | 0 | Source: the two verifier reports (`verify-f55-r1-opus.md` and `verify-f55-r2-opus.md`), kept outside the repository. In the round 1 verifier's runs on main `884f7e22`, a handler on `WinEnter` makes every setup fail and killed Neovim in 17 of 20 runs on v0.8.0 and in 17 of 20 on v0.9.5. Handlers on `WinClosed`, `BufWinLeave`, `BufHidden`, `BufUnload`, `BufWipeout` and a mix of them, which the plugin's own `pcall` swallows while the setup goes on, killed Neovim in 0 of 20 runs on both versions. The new spec case for a setup that succeeds therefore guards that the answers of `openDiff` stay as they are. It does not show the bug. ## Proof Evidence folder: local files, not in the repository (`evidence/round1/`). File names below are relative to it. The facts below are the ones that a verifier reproduced, or that I measured on the head named in the file. 1. **The eager case shows the bug and the guard removes it.** With the helper forced to a plain `coroutine.resume`, the eager case of `diff_failed_setup_guards_spec.lua` alone ended without a plenary summary in 6 of 10 runs on v0.8.0 and 8 of 10 on v0.9.5 (round 2 verifier, all with exit 134 `bad stack slot`), and in 9 of 10 and 6 of 10 (round 1 verifier). The three other cases of that file kept their summary in 10 of 10 runs on each version (round 2 verifier). With the guard, the head had 0 of 10 without a summary in round 1. 2. **Each part of the fix has a test that turns red without it** (a scratch mutant per row, never committed). The two verifiers ran the mutants again. "Unit" is `utils_resume_tool_coroutine_spec.lua` and `tool_coroutine_resume_spec.lua`. "Spec" is `tests/integration/diff_tool_coroutine_jit_spec.lua` on v0.8.0 and v0.9.5. The unmutated tree is green everywhere. Source: `R1_mutants_matrix.txt` and `R1_mutants_failed_tests.txt`. | Removed part | Red in the unit suite | Red in the spec | | --- | --- | --- | | no `jit.off()` | "turns the JIT off, flushes its traces and turns it on again" and others | "runs the coroutine of a tool with the JIT off and no compiled trace, and turns the JIT on again" | | no `jit.flush()` | the same cases | the same case, and "keeps a JIT that was off off" | | no restore (`jit.on()`) | the same cases | the first case | | restore turns the JIT on although it was off | "flushes the traces but keeps the JIT off when the JIT was off before" | "keeps a JIT that was off off" | | first resume not on the helper | "goes through the helper when handle_invoke runs the tool" | both cases | | second resume not on the helper | "goes through the helper when a blocking diff is answered" | the first case (it records the JIT at the answer too) | | version check reversed | "leaves the JIT alone on ..." and others | both cases | | `tostring` removed from the version check | "treats no version / a number / a table as the version as affected" | none: real Neovim always sets a string | The verifiers also showed that with a plain `coroutine.resume(co, result)` at the diff answer, `SetupAnswerReturned` goes FALSE and `check.sh` exits 1. 3. **Nothing else changes for a user.** One `openDiff` accept (`:w`) and one reject (`:q`) through the WebSocket, with `.agents/skills/verify-claudecode-nvim`, on main and on this PR, on v0.8.0 and 0.12.3: the answers (`FILE_SAVED` plus the proposal text, `DIFF_REJECTED` plus the tab name, `TAB_CLOSED`) and the window and buffer states (names, diff mode, text) are byte-equal in 10 of 10 files on each version. The round 2 verifier reproduced this. Source: `R1_vcn_accept_reject_main_vs_head.txt`, `vcn/`. 4. **Stability under load.** Whole-file runs of `diff_tool_coroutine_jit_spec.lua` and `diff_failed_setup_guards_spec.lua`, one Neovim per run, pinned to one core with one busy loop on the same core. Runs that failed or ended without a summary: 0 of 100 on v0.8.0, 0 of 100 on 0.12.3 (both verifiers) and 0 of 30 on v0.9.5 (round 1 verifier). My own runs gave the same (`R1_load_counts.txt`). No run used a retry. 5. **Gates.** On this head (`579df505d800f11fa019607b99b219dbe3a52d6e`, which merges main `c43b6a308ea97f9dcfb0844ce6770e7ce7699ec0`): `covered.lock` was the only conflict of that merge. I took main's file and ran `check.sh --record`, and the merge commit holds the recorded file. `bash .agents/skills/verify-diff-lifecycle/check.sh` ends `== RESULT: PASS`. `mise run format-check`: 438 files, 0 changed. `mise run check`: exit 0, and luacheck reports 0 warnings and 0 errors in 196 files. `mise run test`: 1858 successes, 0 failures, 0 errors, 34 pending. Four integration specs, once on 0.12.3 and once on v0.8.0, with 0 failures and 0 errors each time: `diff_tool_coroutine_jit_spec.lua` 3 successes, `diff_failed_setup_guards_spec.lua` 4, and the two specs that coder#516 added or changed, `diff_legacy_cleanup_spec.lua` 18 and `diff_legacy_swap_spec.lua` 1. The whole integration runner did not run on this head (CI runs it). My change is the same as at `230be2d2` (the head that the round 2 verifier passed): the added and removed lines outside `covered.lock` are byte-equal, 469 lines in 10 files. On an earlier head (`661e4e52`, main `48ca93be` merged), the integration runner gave 0.12.3 `Total Success: 603`, `Failed: 0`, `Errors: 0`, and v0.8.0 `Total Success: 585`, `Failed: 0`, `Errors: 0`. A first 0.12.3 run under a host load above 100 lost the summary of three files to the runner's 40 s limit (`diff_close_save_spec.lua`, `diff_exit_reject_spec.lua`, `diff_left_window_spec.lua`). Each passed alone (77, 28 and 33 successes), and the whole runner passed on the second run. The two new specs ran 20 times per version on v0.8.0, v0.9.5 and 0.12.3 on `661e4e52` with 0 failed runs and 0 runs without a summary. The round 2 verifier ran the gates on `230be2d2`: `mise run test` 1758 successes, 0 failures, 0 errors, 29 pending, the integration runner 530/0/0 on 0.12.3 and 393/0/0 on v0.8.0, and `check.sh` PASS. Its counts are lower than mine because main had fewer tests then. Sources: `../round4/` for this head, `../round3/` for the head before it, and `../round2/R2_*.txt` and `../round2/m2_int_*.txt` for the older heads. 6. **Where the Neovim binaries come from** (`R1_binary_provenance.txt`, reproduced by the round 2 verifier). - v0.8.0: the asset `nvim-linux64.tar.gz` of the official GitHub release v0.8.0 (published 2022-09-30), fetched with `gh release download v0.8.0 -R neovim/neovim -p 'nvim-linux64.tar.gz*'`. Its sha256 matches the `.sha256sum` file and the binary that was run. `--version` prints `NVIM v0.8.0-1210-gd367ed9b2`. - v0.9.5: the asset `nvim-linux64.tar.gz` of the official GitHub release v0.9.5 (published 2023-12-30), fetched the same way, with the same sha256 checks. - 0.12.3 is the mise install of this repository. ## Cost on Neovim 0.8 and 0.9 On Neovim 0.8 and 0.9, each tool call that runs in a coroutine (`openDiff`) discards the compiled traces of the whole editor, and LuaJIT compiles them again when the code gets hot. The round 1 verifier measured it with about 740 compiled traces: a guarded resume took 18 to 41 µs (a plain resume took 1.1 µs or less), and the first run of a hot loop of 300 small functions after the flush took 3.6 to 10.3 ms against 0.3 to 0.6 ms when hot. Nobody measured a whole editing session. A LuaJIT that is not affected gets a plain `coroutine.resume`. ## What this PR does not cover - A coroutine of another plugin or of a test harness. The helper covers the coroutines of the plugin's tools only. Plenary runs each spec file in its own coroutine, so a spec that calls `_setup_blocking_diff` directly is not covered. Such a spec must go through `tools.handle_invoke`. - `scripts/manual_test_helper.lua` resumes its own coroutine. It is a manual script and not part of the plugin. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_ <!-- xum-attribution: 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.
The legacy
open_diffleft its temp file behind when a step failedrequire("claudecode.diff").open_diff(...)(the public legacy entry point, which wraps_open_native_diff) writes the proposal to a temp file, then opens the original and the proposal side by side. It registers the autocmd that deletes the temp file as its last step. A step that raises before that leaves the temp file instdpath("cache")/claudecode_diffs, and sometimes a proposal buffer.Four paths raise after the temp file exists. The first three were found by the verifier of #501:
open_difffor a path whose proposal is still open: the rename to<path> (New)raisesE95: Buffer with this name already exists.:preserve):editof the original raisesE325.FileTypeautocmd for lua. Neovim's filetype detection ignores the.newending of the temp file, soFileTypefires for it too. Whicheditraises depends on the layout:editreads it again andFileTypefires for the original. The error stops the diff at its first step.editdoes not read it again, soFileTypefires for the proposal's temp file, after the diff has split a window and turned diff mode on.FileTypethere reaches the caller on Neovim 0.8 and is only a message on 0.12.editof it raisesE37: No write since last change. I ran this by hand on 0.12.3 and v0.8.0: the call raisesE37, and no temp file, window or buffer is left over. The spec has no case for it.Any other error that one of these steps raises is cleaned up the same way, because the fix wraps the whole sequence.
Who is affected: only code that calls
open_diffitself. Nothing in the plugin calls this path: theopenDifftool usesopen_diff_blocking.The fix
lua/claudecode/diff.lua: the steps aftercreate_temp_filemove intoshow_native_diff, unchanged._open_native_diffcalls it withpcall. If it raises,remove_failed_proposaldoes two things, then_open_native_diffraises the same error again (the message has one position, as before):Every step is protected, so an error in one cannot stop the next or replace the error that made the diff fail.
It does nothing else. It closes no window, moves no focus or tab, and turns no diff mode off. A failed
open_diffcan still leave the windows that it had already split, and the diff mode it had turned on, asmaindoes. A proposal buffer that a window shows stays too, and so does a proposal that was already renamed to<path> (New). The window check looks at every tab. Because a window that shows the proposal stays, the temp file behind it is gone, and the next:checktimeprintsE211: File "..." no longer availableonce (I checked this on 0.12.3 and v0.8.0). No text is lost.lua/claudecode/README.mdsays so.Why this PR does not undo the windows
Earlier versions of this PR also undid the windows, the diff mode and the focus of a failed diff. In four review rounds, Codex and the verifier each found one more way for that undo to reach something that was not the diff's own: another diff's mode, a window that an autocmd opened, the split of another tab, a buffer of the user's with unsaved text. Each one was an autocmd that changes the layout while the diff runs. I removed the undo instead of fixing a fifth case. All Codex threads are resolved. Two of the three round-4 threads (4543276628, 4543276636) are about code that this PR no longer has. The third (4543276642) is about the spec's
after_each, which this PR keeps: it now runstabonly!first.Spec hygiene
diff_legacy_swap_spec.lua(#501) and the cleanup spec leave a swap file by killing a child Neovim with SIGKILL after:preserve. A killed Neovim cannot delete its socket, so each run of each spec left one folder behind:$TMPDIR/nvim.$USER/<random>/, ornvim.<pid>.0in$XDG_RUNTIME_DIRwhen that is set. The child now starts withTMPDIRandXDG_RUNTIME_DIRset to a folder inside the spec's own directory, whichafter_eachdeletes. I measured both specs with an empty privateTMPDIRandXDG_RUNTIME_DIR, on 0.12.3 and v0.8.0: one folder per run before the change, none after.The cleanup spec also points
XDG_CACHE_HOMEat its own folder, as the swap spec does since #520.open_diffwrites understdpath("cache"), and the CI step from #520 fails the job when a spec leaves a folder in the real cache. Run with a privateHOME, the integration runner leaves no~/.cache/nvim.Proof
tests/integration/diff_legacy_cleanup_spec.luahas 18 real-Neovim cases. Each asserts that the call raises and that no temp file is left, and each one that has a user's window, buffer or diff mode in it asserts that this stays.FileTypeerror, original currentFileTypeerror for the proposal, original hiddenBufFilePostraises):diffthisin two windowsWinNewhandler opens a window with unsaved textWinEnterhandler sends the cursor back to the editor windowBufEnterhandler wipes the proposal and leaves a new buffer with textBufEnterhandler changes the proposal and moves the window to another bufferBufEnterhandler shows the proposal in a new tab and then gives the split another buffer, so only a window of another tab shows itWith the
diff.luaofmain, 18 of 18 fail on Neovim 0.12.3 and on v0.8.0. With this head, 18 pass on both.Mutants: I changed
remove_failed_proposaland_open_native_diffin 16 ways, one at a time, and ran the spec on both versions. 13 are killed: no wipe, no temp file delete, no cleanup call, no re-raise,error(failure)forerror(failure, 0), wipe a buffer with unsaved changes, wipe a buffer that a window shows, no real-path match, delete before wipe, a cleanup that turns diff mode off or closes other windows, and two window checks that look at the current tab only (bufwinnrandbufwinidforwin_findbuf). Those last two passed the first 17 cases and fail only case 18, on both versions. Three survive. Nopcallaround the wipe, because an autocmd that raises duringnvim_buf_deleteis only a message on both versions. Nopcallaround the temp file delete, becausecleanup_temp_filereports a failed delete withvim.notifyand does not raise. Wiping the current buffer when no buffer has the name, because the unsaved-changes check and the window check each keep a buffer of the user's.The verifier's probe (
probe.shanddrive.lua, 22 scenarios) on this head and onmain, on both versions: the windows, the buffers, the diff modes and the focus after the call are the same in all 22 scenarios. The only differences are the temp files, which are gone (so a window that still shows the proposal seesE211once on the next:checktime), and, in one scenario, an unlistedfoo.lua.newbuffer that no window showed, which is wiped.Other gates:
mise run format-checkandmise run check(0 warnings in 177 files) are clean.mise run test: 1655 successes, 0 failures, 24 pending (the cleanup spec is pending under busted, because it needs a real Neovim).TMPDIRnamedodd tmp 50%(the two runs that the CI job makes since test: guard CI against odd TMPDIR and real-cache writes, plus follow-ups of #510 and #513 #520), in a privateHOMEwith noXDG_CACHE_HOME: 337 tests per run on 0.12.3 and 250 per run on v0.8.0. The real cache folder stays untouched. The 15diff_*_spec.luafiles (the two legacy specs and 13 others) ran in each run: 30 runs in each mode, 0 failures..agents/skills/verify-diff-lifecycle/covered.lockis re-recorded. Only the hash ofdiff.luachanged, and_open_native_diffis outside the model. The model passes (89,205 distinct states).Findings that
mainalready hasThe verifier of this PR found two defects that
mainhas too, and this PR does not change them:WinEnterautocmd that raises inside the diff's own split leaves that split open.Both are about windows. Nothing in the plugin calls this path, so they stay as they are.
Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh