Repository navigation
test: guard CI against odd TMPDIR and real-cache writes, plus follow-ups of #510 and #513 - #520
Merged
Merged
Conversation
… of the real cache
- diff_hand_over_spec.lua parsed the plugin's `:split <path>` step with a
pattern that stops at the escaped space of a path in a TMPDIR with a space,
so 3 swap file step tests failed. It now reads up to the closing text.
- tmpdir_suite_spec.lua checked "1 success" with a plain find, which also
matches "11 successes". It compares the parsed count.
- diff_legacy_swap_spec.lua let the legacy diff write claudecode_diffs under
the real stdpath("cache"). It points XDG_CACHE_HOME at its temp folder.
- mise.toml comments named PUC Lua 5.1 for the pinned lua 5.5.0.
_Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
…d check the cache folder
No job ran with an unusual TMPDIR, so a test that gave a temp path to an Ex
command without fnameescape stayed green in CI. The integration job now runs the
runner a second time with TMPDIR set to a folder named "odd tmp 50%". A reverted
fnameescape turns it red.
A step after it fails when the runs wrote the real stdpath("cache") folder, which
a fresh runner does not have.
_Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
ThomasK33
marked this pull request as ready for review
November 11, 2026 17:58
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. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
ThomasK33
added a commit
that referenced
this pull request
Nov 11, 2026
…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.
This was referenced Nov 11, 2026
hareki
pushed a commit
to hareki/claudecode.nvim
that referenced
this pull request
Nov 13, 2026
…tep fails (coder#516) ## The legacy `open_diff` left its temp file behind when a step failed `require("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 in `stdpath("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 coder#501: 1. A second `open_diff` for a path whose proposal is still open: the rename to `<path> (New)` raises `E95: Buffer with this name already exists`. 2. A changed swap file for the real file (another Neovim changed it, or a killed one after `:preserve`): `edit` of the original raises `E325`. 3. An error in a `FileType` autocmd for lua. Neovim's filetype detection ignores the `.new` ending of the temp file, so `FileType` fires for it too. Which `edit` raises depends on the layout: - The original is the current buffer: `edit` reads it again and `FileType` fires for the original. The error stops the diff at its first step. - The original is loaded but hidden: `edit` does not read it again, so `FileType` fires for the proposal's temp file, after the diff has split a window and turned diff mode on. - After the rename the plugin sets the filetype again. An error in `FileType` there reaches the caller on Neovim 0.8 and is only a message on 0.12. 4. The original has unsaved changes and is the current buffer of the target window: `edit` of it raises `E37: No write since last change`. I ran this by hand on 0.12.3 and v0.8.0: the call raises `E37`, 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_diff` itself. Nothing in the plugin calls this path: the `openDiff` tool uses `open_diff_blocking`. ## The fix `lua/claudecode/diff.lua`: the steps after `create_temp_file` move into `show_native_diff`, unchanged. `_open_native_diff` calls it with `pcall`. If it raises, `remove_failed_proposal` does two things, then `_open_native_diff` raises the same error again (the message has one position, as before): 1. It wipes the buffer that has the temp file's name, unless a window shows that buffer or it has unsaved changes. It finds the buffer by the name or by the real path, because Neovim resolves a link in a buffer's name, so a cache folder that is a link changes the name. 2. It deletes the temp file and its folder. 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_diff` can still leave the windows that it had already split, and the diff mode it had turned on, as `main` does. 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 `:checktime` prints `E211: File "..." no longer available` once (I checked this on 0.12.3 and v0.8.0). No text is lost. `lua/claudecode/README.md` says 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 runs `tabonly!` first. ## Spec hygiene `diff_legacy_swap_spec.lua` (coder#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>/`, or `nvim.<pid>.0` in `$XDG_RUNTIME_DIR` when that is set. The child now starts with `TMPDIR` and `XDG_RUNTIME_DIR` set to a folder inside the spec's own directory, which `after_each` deletes. I measured both specs with an empty private `TMPDIR` and `XDG_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_HOME` at its own folder, as the swap spec does since coder#520. `open_diff` writes under `stdpath("cache")`, and the CI step from coder#520 fails the job when a spec leaves a folder in the real cache. Run with a private `HOME`, the integration runner leaves no `~/.cache/nvim`. ## Proof `tests/integration/diff_legacy_cleanup_spec.lua` has 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. | # | Case | What it checks | |---|---|---| | 1 | E95, a second diff of the same path | The first diff stays open and in diff mode. No window is closed. The error has one position, as before. | | 2 | E325 | A swap file for the original. | | 3 | `FileType` error, original current | The diff stops at its first step. | | 4 | `FileType` error for the proposal, original hidden | No window is closed. | | 5 | The cache folder is a link, and a step fails after the rename | The unlisted buffer under the temp file's name is found by its real path and wiped. | | 6 | Autocmds raise as that buffer is wiped | The cleanup finishes and the first error is raised. | | 7 | A step fails after the rename (`BufFilePost` raises) | No buffer has the temp file's name. | | 8 | A buffer has the proposal's name, original current | No window is closed. The buffer that was there stays. | | 9 | Sidebar on the left and a diff already open | That diff stays on. The sidebar stays. No window is closed. | | 10 | The user's own `:diffthis` in two windows | That diff stays on. No window is closed. | | 11 | A diff is open, and the call starts in a prompt window of another tab | The open diff stays on. | | 12 | The diff starts in a prompt window and picks the editor window of another tab | The prompt window stays. No window is closed. | | 13 | An autocmd opens a window, then raises | That window stays. | | 14 | A `WinNew` handler opens a window with unsaved text | The text stays. No window is closed. | | 15 | A `WinEnter` handler sends the cursor back to the editor window | The editor window stays. No window is closed. | | 16 | A `BufEnter` handler wipes the proposal and leaves a new buffer with text | The buffer of the user and its text stay. | | 17 | A `BufEnter` handler changes the proposal and moves the window to another buffer | The proposal stays, with its text. | | 18 | A `BufEnter` handler shows the proposal in a new tab and then gives the split another buffer, so only a window of another tab shows it | That window stays. No window is closed. | With the `diff.lua` of `main`, 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_proposal` and `_open_native_diff` in 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)` for `error(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 (`bufwinnr` and `bufwinid` for `win_findbuf`). Those last two passed the first 17 cases and fail only case 18, on both versions. Three survive. No `pcall` around the wipe, because an autocmd that raises during `nvim_buf_delete` is only a message on both versions. No `pcall` around the temp file delete, because `cleanup_temp_file` reports a failed delete with `vim.notify` and 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.sh` and `drive.lua`, 22 scenarios) on this head and on `main`, 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 sees `E211` once on the next `:checktime`), and, in one scenario, an unlisted `foo.lua.new` buffer that no window showed, which is wiped. Other gates: - `mise run format-check` and `mise 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). - The integration runner passes on both versions with 0 failures, once normally and once under a `TMPDIR` named `odd tmp 50%` (the two runs that the CI job makes since coder#520), in a private `HOME` with no `XDG_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 15 `diff_*_spec.lua` files (the two legacy specs and 13 others) ran in each run: 30 runs in each mode, 0 failures. - `.agents/skills/verify-diff-lifecycle/covered.lock` is re-recorded. Only the hash of `diff.lua` changed, and `_open_native_diff` is outside the model. The model passes (89,205 distinct states). ## Findings that `main` already has The verifier of this PR found two defects that `main` has too, and this PR does not change them: 1. A `WinEnter` autocmd that raises inside the diff's own split leaves that split open. 2. A failed diff does not give the target window back the buffer that it showed before. Both are about windows. Nothing in the plugin calls this path, so they stay as they are. --- _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.
What you see
TMPDIR. 3 tests oftests/integration/diff_hand_over_spec.lua("names a swap file step ...") fail. They fail onmaintoo. The plugin's message names the step:split <path>with the path escaped (a\ b), and the test's pattern did not match it at all, so the step was nil (Passed in: (nil)).TMPDIR. The%bug that test: make the suite pass with a % or a relative path in TMPDIR #510 fixed (an Ex command in a test that took a temp path unescaped) stayed green in CI. Nothing would catch it again.tests/integration/diff_legacy_swap_spec.lua(from fix(diff): open the legacy diff's temp file without a swap file #501), lets the legacy diff createclaudecode_diffsunder the realstdpath("cache"), so every integration run adds~/.cache/nvim/claudecode_diffs.What changes
diff_hand_over_spec.luareads the step with:(split .-) %(answer the swap file prompt, so an escaped path survives.tests/unit/tmpdir_suite_spec.luacompared the child's output withfind("1 success"), which also matches11 successes. It now compares the parsed count to 1.tests/unit/luacov_config_spec.lua: no change.load_runneronmainalready replaces theostable of the.luacovchunk through its environment (test: stop the luacov config spec reading the host's TMPDIR #513 as merged). It never swapsos.getenv.mise.toml: the two comments named "PUC Lua 5.1" for the pinlua = "5.5.0". They now say PUC Lua 5.5.0 is installed for its bundled LuaRocks, and that the 5.1 yield limit is that of stock PUC Lua 5.1 (LuaJIT is the test runtime). Comment-only. CI's mise and rocks caches key on this file, so this PR's own run builds cold once. They refill after the merge..github/workflows/test.yml: the integration job runs the runner a second time withTMPDIRset to a created folder namedodd tmp 50%. One more step, on both Neovim legs.diff_legacy_swap_spec.luapointsXDG_CACHE_HOMEat its temp folder for the test and restores it. A second new step fails the job when the runs left a folder under the real cache path. A fresh runner has none, so the check needs no allow list. It cannot run locally with a real home: a developer's own sessions fill that folder.Evidence
Integration runner, all files, plenary at CI's pinned commit, stub
claude:TMPDIRmaindiff_hand_over_spec)main%%, space, space and%%, space, space and%In each of those 8 runs on this branch,
XDG_CACHE_HOMEwas a fresh folder, and nonvimfolder appeared in it.The CI step turns red when a
fnameescapeof #510 is reverted: with the one indiff_new_tab_mode_spec.luareverted, the runner underodd tmp 50%gives 294 passed, 22 failed, exit 1. Without the revert it is green (rows above).The cache check, run with the real home swapped for an empty folder:
diff_legacy_swap_spec.luaas onmain.cache/nvim/claudecode_diffs(the spec passes)diff_legacy_swap_spec.luaof this branch.cache/nvimI ran each of the 24 integration specs alone with a fresh
XDG_CACHE_HOME. Onlydiff_legacy_swap_spec.luacreated anything (nvim/claudecode_diffs). The busted unit suite (1586 tests) with a freshXDG_CACHE_HOMEcreated nothing.CI time: on CI at this head the extra runner pass took 42.6 s on the stable leg and 37.1 s on the v0.8.0 leg. The two legs run in parallel, so the job gets under 1 minute longer. The cache check takes under a second.
Gate:
mise run clean && mise run allexit 0 (seed 1794418531, 1586 passed, 0 failed, 22 pending).actionlintpasses.Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh