Repository navigation
fix(diff): split before diffthis so the proposal window saves no diff mode - #573
Conversation
…lone when a split diff closes The plugin split the window after it ran diffthis, so the proposal window saved diff mode for the file and brought it back with :edit. Both split open paths now split first. The cleanup of a split diff: - holds BufEnter and BufWinEnter back while a window falls back to the file, ends the leftover there, and then runs them, so a handler of the user's sets its options on a clean window - looks at the windows of every tab page that show the file, and leaves alone a window that was in diff mode before the diff opened or where a command turned 'diff' on while it was open (OptionSet fires for a command and not for a split copy) - leaves the original-side window of another open diff alone _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
…re diffthis The change touches register_state, cleanup_state and the setup of a split diff. It adds no call that resolves, closes or answers a diff, and it guards none. check.sh passes with TLC (89205 states). _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
…tched back to the file The plugin splits before diffthis, so Neovim saved no diff mode for the file in the proposal window. _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
One child per case took more than the runner's 40 s per file when the host was busy. _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
…-diffthis # Conflicts: # .agents/skills/verify-diff-lifecycle/covered.lock # lua/claudecode/README.md
|
@codex review |
|
@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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ae8ceca08
ℹ️ 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".
…-diffthis # Conflicts: # .agents/skills/verify-diff-lifecycle/covered.lock
Restore main's text for the sweep, the delete block of the cleanup and the spec cases that belong to follow-up work: the OptionSet autocmd and the snapshot of diff windows, the held and replayed events, the check for a window that another open diff uses, the tab-page sweep, and their spec cases. Keep the split before `diffthis` in both open paths. The original window gets its `diffthis` in `nvim_win_call`, so no WinEnter or BufEnter fires for a user handler that moves the focus between the two `diffthis` calls. Move the `:tabnew`, `:tabnew` + `:buffer` and `:tab sbuffer` cases into diff_left_window_spec, where they need no OptionSet. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
…he split before diffthis Only the hash of diff.lua changed. The blocks that the model cites are the same as on main. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
… for a user's handler _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
…ch open path runs _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
The legacy diff path calls nvim_win_call for the window it read with nvim_get_current_win, and the specs that drive it never register that window. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
The legacy diff path and the specs that drive it use window ids that no spec registers, and nvim_set_current_win in the mock already accepts every id. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
…-diffthis # Conflicts: # .agents/skills/verify-diff-lifecycle/covered.lock
Only the hash of diff.lua changed. The blocks that the model cites are the same as on main. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
…-diffthis # Conflicts: # .agents/skills/verify-diff-lifecycle/covered.lock
…gain Only the hash of diff.lua changed. The blocks that the model cites are the same as on main. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
|
@codex review |
|
@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. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs 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". |
… exit answers docs (coder#572) ## Why Keep the exit tests and the "Exit answers" docs true after coder#558 and coder#564. 1. coder#564 made the unified layout work on Neovim 0.8.0 and removed the "needs Neovim 0.9" skips from eight specs. `diff_exit_reject_spec.lua` came in with coder#558 and still skipped the unified layout below 0.9 in three places. So 14 unified exit scenarios stayed pending in the v0.8.0 job, and the file kept a comment that is no longer true. 2. The advisor and the verifier of coder#558 found README sentences that say more than the code does. They concern the exits that run no `VimLeavePre`. ## What changed - `tests/integration/diff_exit_reject_spec.lua`: - The three skips and the comment "The unified layout needs Neovim 0.9" are gone. - Three new cases (each in the vertical and unified layouts) start the exit inside an autocmd, with two waiting calls. A direct `:qa!` in an autocmd that is not `nested`, and SIGTERM while such an autocmd waits in `vim.wait`: no call is answered. A direct `:qa!` in a `nested` autocmd: both calls get `DIFF_REJECTED`. Each case first checks that both calls were waiting. The two no-answer cases also check that the child ended by itself. - The spec has 34 passes on Neovim 0.8.0 and 34 on 0.12.3. On main it has 14 passes and 14 pending on 0.8.0, and 28 passes on 0.12.3. - `README.md`, Global Invariants, "Exit answers": - The `"close_window"` sentence now says "waiting calls". A call that a nested `:qa!` cuts off during its `openDiff` setup has no response sender yet. The server stop answers it with -32603 after the `ClaudeCodeDiffClosed` events. Every call still gets one answer before close code 1001. - The sub-bullet for exits without `VimLeavePre` keeps SIGKILL, a crash and a `:qa!` from inside an autocmd that is not `nested`, as on main, and adds `:noautocmd qa!` and a Lua `os.exit()`, which also run no `VimLeavePre`. It adds one rule: a `:qa!`, SIGTERM or SIGHUP that Neovim handles while an autocmd that is not `nested` is still running can end Neovim without `VimLeavePre` too. A synchronous LSP format on save, which waits in `vim.wait`, is the common example. It says "can" because Neovim looks only at the autocmd that runs when it handles the exit, so a `nested = true` handler that such an autocmd started (with `:doautocmd` or `nvim_exec_autocmds`) still gets `VimLeavePre`. It promises nothing for a signal during a plain Lua loop, because Neovim handles a signal only at its next event check. - `ARCHITECTURE.md`, `lua/claudecode/README.md` and the comment of `cleanup_at_exit` in `lua/claudecode/diff.lua` say the same. ## Scope Tests and docs. The only change under `lua/` is that comment in `diff.lua`. `verify-diff-lifecycle` passes (93,477 distinct states, and 5,557 for the live case), and its `covered.lock` records the new file hash. The plugin does not answer calls in the exits that the README lists as running no `VimLeavePre` (SIGKILL, a crash, `:noautocmd qa!`, `os.exit()`, and `:qa!` in an autocmd that is not `nested`), because none of its exit code runs in them. The docs say so. ## Blast Radius None at runtime. The v0.8.0 CI job runs 20 more tests of this spec, and the stable job runs 6 more. ## Verification - Spec on Neovim 0.8.0 and 0.12.3: 34 passes, 0 failures, in 5 of 5 runs on each version, and in 3 of 3 runs again after the last commits, which change comments, docs and the lock only. Mutants: - The autocmd's `:qa!` replaced by a no-op: the first no-answer case is red on both versions (the file then runs into the runner's 40 s limit, because each affected case waits 20 s). - `cleanup_at_exit` returns at once: 8 cases are red on both versions, among them the nested `:qa!` case in the vertical layout. The no-answer cases stay green, because they pin what Neovim does. A Neovim that changes it turns them red, and the README has to change. - Probes on 0.8.0 and 0.12.3 (vertical and unified, two waiting calls, 6 runs per row, a `VimLeavePre` marker and the answers in one log). In an autocmd that is not `nested`, each of these runs no `VimLeavePre` and answers no call, in all 54 runs per version: a direct `:qa!`, a timer `:qa!` during `vim.wait`, SIGTERM and SIGHUP during `vim.wait`, SIGTERM and SIGHUP in the Lua part of a handler that waits later, SIGTERM in the first of two handlers when the second waits, SIGTERM in a Vim script loop, and SIGTERM in a `:%s` loop. In a `nested = true` autocmd the same cases answer both calls. With no autocmd, a timer `:qa!`, SIGTERM and SIGHUP answer both calls (18 of 18). `:noautocmd qa!` and a Lua `os.exit()` from a timer run no `VimLeavePre` and answer no call (9 of 9 on each version, three layouts), and a plain `:qa!` and `:cq` run it and answer both calls (9 of 9). A non-nested autocmd that starts a `nested = true` handler which waits in `vim.wait`, with SIGTERM during the wait, runs `VimLeavePre` and answers both calls (6 of 6 on each version). - Waiting calls with `close_window`, `:qa!` and SIGTERM, three layouts: both answers come before the first `ClaudeCodeDiffClosed` (6 of 6 on each version). A nested `:qa!` that cuts a call off in its setup: the -32603 comes after every `ClaudeCodeDiffClosed` event (12 of 12 on each version), and no id is answered twice. - Independent reads with a real WebSocket client on Neovim 0.8.0, 0.9.5, 0.10.4, 0.11.5 and 0.12.3: a verifier (330 runs), an advisor (225 runs) and two claim checks (375 runs in the first, and 140 runs for the nested-handler case in the second). They found that the first wording dropped the direct `:qa!` case, that the second promised `VimLeavePre` for a signal during a Lua loop, and that "ends" is false when the running autocmd started a `nested` handler. The sub-bullet above says only what survives all three, with "can". A last narrow check found only that "Without `VimLeavePre`" was too general, because `eventignore` can skip `VimLeavePre` and a `:qa!` still rejects diffs while buffers unload. The README now says "In these exits". - At the head after the merge of main (coder#573): `mise run format-check` and `mise run all` pass (1892 successes, 0 failures, 0 errors, 35 pending), and the integration runner passes on 0.12.3 (844) and on 0.8.0 (840). The commits of this PR change comments, docs, the spec header and the lock only. `check.sh --hash-only` passes for the four verify skills, and the full `verify-diff-lifecycle` check passes (93,477 and 5,557 states). --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
…#593) ## Why `tests/integration/diff_window_sizes_spec.lua` died once under host load with `Nvim: Caught deadly signal 'SIGHUP'` (seen on a coder#573 head, green on the rerun, and main has the same exposure). The runner then reported "no plenary summary (nvim exit 1)". A test must not kill the Neovim that runs it, and a retry hides the cause. ## Cause The cause is a race inside Neovim and libuv. It is not a signal mask or disposition that stays wrong after exec. 1. `pty_process_spawn` calls `forkpty()`. In the child, `login_tty()` gives it a new session on the pty. 2. `init_child()` then runs `setsid()`, resets SIGCHLD, SIGHUP, SIGINT, SIGQUIT, SIGTERM and SIGALRM to their defaults, changes directory, builds the environment and calls `execvp`. Until exec the child is a copy of Neovim and has Neovim's libuv signal handlers. 3. When the pty master closes in that gap (`jobstop`, a buffer wipe), the kernel sends SIGHUP to the child. libuv's inherited handler writes a message into the signal pipe that the child shares with Neovim. Neovim reads it as its own SIGHUP and exits. `strace -f` on a held child shows it. The child gets `SIGHUP {si_code=SI_KERNEL}` and runs the handler, which writes to fd 11 on Neovim 0.8.0 (fd 8 on 0.12.3) and then one byte to a second descriptor. The parent then prints `Caught deadly signal 'SIGHUP'`. The 0.8.0 and 0.12.3 sources have the same order in `init_child()`. The spec reaches it because several tests start a terminal job (`terminal.open()` or `termopen({ "sleep", "60" })`), end within milliseconds, and `after_each` wipes the buffer. A child that the host scheduler delays inside the gap is enough. coder#336 had already measured the same race on a plain `termopen` followed by `jobstop`. ## Fix `tests/helpers/termopen_wait.lua` wraps `vim.fn.termopen`, and `tests/minimal_init.lua` installs it. The wrapper returns only after `/proc/<pid>/exe` or `/proc/<pid>/cmdline` of the child differs from Neovim's own, so the child has run `exec` and holds none of Neovim's handlers. It compares the command line too, because a job that runs Neovim itself keeps the same executable. The wait ends with an assertion after 10 s. Every integration spec and every Neovim that loads `minimal_init.lua` gets it. The wrapper reads `/proc`, so it works on Linux only (CI runs on Linux). On another system it does nothing. - `tests/integration/pty_child_exec_spec.lua` has 10 cases. Five drive the wrapper with stand-ins for `/proc` (a child that runs Neovim's code for N reads, a child that has run exec, a job that runs Neovim with another command line, a child that never runs exec, a job with no pid). One checks that `vim.fn.termopen` is the wrapper. Four use real jobs: a plain job, a job that a `TermOpen` autocmd stops, a job whose buffer a `TermOpen` autocmd wipes, and a job that runs Neovim. - `.agents/skills/verify-claudecode-nvim/scripts/proof-pty-child-hangup.sh` reproduces the race with an `LD_PRELOAD` shim that holds the child inside the `signal(SIGCHLD, SIG_DFL)` call that follows `setsid()`, and it checks the wrapper against it. It saves the PID and start time of each case's child and kills only that process. `SKILL.md` and `features/terminal.md` point to it. - `DEVELOPMENT.md` has a paragraph about the wrapper and what it does not cover. No plugin code changes. The merge of `origin/main` brings in main's own `diff.lua` change. ## Evidence All runs use the official `v0.8.0` release binary (`NVIM v0.8.0-1210-gd367ed9b2`, as in CI) or Neovim 0.12.3, with a scratch HOME and XDG dirs, on a shared host with a load average of 20 to 100. The injected rates depend on the host load. | Run | main | this branch | |---|---|---| | `diff_window_sizes_spec` with the child held 100 ms in the gap, 0.8.0 | 13 of 20 die by SIGHUP | 0 of 20 | | same, 0.12.3 | 18 of 20 die by SIGHUP | 0 of 20 | | `diff_window_sizes_spec`, no shim | 0 of 200 fail (0.8.0) | 0 of 20 fail on 0.8.0 and on 0.12.3 | | plain `termopen`, spin 50 to 1200 µs, `jobstop`, 40 runs per delay (0.8.0) | 2 of 200 die by SIGHUP | not applicable, it uses no `minimal_init.lua` | | `proof-pty-child-hangup.sh` | the `wrapper` case fails on 0.8.0 and 0.12.3 | 3 of 3 cases pass on both | The wrapper spec against 12 mutants of the wrapper (wrapper not installed, no wait, never waits because `/proc/self/exe` is unreadable, wait while the executable differs, wait while the command line differs, executable check only, a single 1 ms poll, no deadline assertion, a 1 ms deadline, no `pcall` around `jobpid`, no sleep, polling Neovim's own pid) and a control: the control is red 0 of 3 and every mutant is red 3 of 3, on 0.8.0 and on 0.12.3. The first version of the spec caught a single 1 ms poll only 2 of 6 times on 0.8.0 and 4 of 6 on 0.12.3. Two cases that the first version got wrong now hold (the nested-Neovim case was wrong in the wrapper, the slow-parent case in the spec): - A job that runs Neovim itself. The old wait took the whole job (2 s for `sleep 2`) or failed after 10 s (for `sleep 15`). Now `termopen` returns after 7 to 14 ms. - A `TermOpen` autocmd that takes 50 ms. With main's `minimal_init.lua` plus that autocmd, the old spec was red 0 of 5 and the new spec is red 5 of 5 on both builds. With this branch's `minimal_init.lua` plus the same autocmd, the new spec is red 0 of 5. The child's gap after the fork is only 2 to 6 ms, even with an environment of 20000 to 200000 entries, so no setting of the spec can keep the child in Neovim's code through such an autocmd. That is why the wait is pinned with stand-ins. The proof script left one `sleep 60` per run before this fix (3 of 3 runs), and leaves none now. ## Not proven or not covered - Natural runs of `diff_window_sizes_spec` on main did not die in 200 tries on this host. The original death is explained by the mechanism and by the injected runs, but I did not reproduce that exact occurrence naturally. - The wrapper covers `vim.fn.termopen` only. A job that a spec starts with `jobstart` and `term = true` is not covered. No spec does that today. - A stop that a `TermOpen` autocmd issues inside `termopen` runs before the wait, so it is outside the wrapper. For a job that runs Neovim with the same command line as the test Neovim, `termopen` blocks until that job exits and raises if it lives 10 s. A poll that lands inside `exec` can return early. No spec starts such a job. - The plugin has the same exposure outside the tests. `:ClaudeCodeClose` of the native terminal waits for the exec of a young job (coder#544). A direct `jobstop` or `:bwipeout` of the terminal buffer right after `termopen`, and the Snacks close, do not wait. This PR changes no plugin code. - Neovim and libuv have the root fix to make (block SIGHUP across `forkpty()` or reset the handlers before the session becomes the pty's). I did not report it upstream. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
…iff is replaced (coder#536) ## Why Claude waits for an answer to every `openDiff`. When the plugin never answers a call, Claude waits forever. One order on `main` leaves a call without an answer. A handler of the user runs the event loop (`vim.wait`), so that Claude's queued second call for the same tab name runs inside it. The verifiers of coder#512 found this order while they checked a candidate. That candidate newly reaches it, but `main` has it without any of its code, so I fix it here first. **A close handler.** An `openDiff` A for a tab name replaces the open diff X. A `ClaudeCodeDiffClosed` handler of the user runs the event loop while X closes, so a queued call B registers a diff for the tab name. When X is a new file, A then cleans up the diff that is registered for the tab name, and that is B's. Nothing answers B, and its tab or windows can stay. When X is an existing file, B can instead stay open with no answer, or A fails with E95. When X is an existing file and B shares A's proposal name, A fails with E95 as well, and A's failure cleanup removes B the same way. ## What changed 1. The replace step of `open_diff_blocking` is a loop. It rejects the diff that is registered for the tab name (if it is pending) and cleans it up, as before, and repeats while another diff is registered. It cleans up only the diff that it handled, so a diff that registered meanwhile is never removed without an answer. Each pending call that A replaces gets one `DIFF_REJECTED` answer, B included, and A sets up on a free tab name. The change is 11 added and 5 removed lines in `lua/claudecode/diff.lua`. 2. `tests/integration/diff_answer_every_call_spec.lua` has 9 cases. Four of them run the close handler order in both layouts, with X a new file and with X an existing file. Five pin what `main` does today after a plugin reload inside a setup (see "A reload" below). 3. `lua/claudecode/README.md` and the `verify-diff-lifecycle` skill describe the loop. The skill lists the order as not modeled, its `ReplaceRejects` and `ReplaceKeepsOthersSaved` facts read `while existing_diff do`, and `covered.lock` has the new hashes. **A reload.** An earlier version of this PR also registered a diff in the new module after a plugin reload inside a setup. I removed that step. A reload inside a setup behaves as on `main`, with and without the event loop in the reload: the diff finishes its setup in the old module, so `:ClaudeCodeDiffAccept`, `:ClaudeCodeDiffDeny` and `:ClaudeCodeCloseAllDiffs` of the new module do not reach it. `:w`, `:q` and `:bwipeout!` in its window still answer it, and the user can end it from there. This PR does not promise one `ClaudeCodeDiffClosed` per diff after a reload, because `main` does not either. Every order that the scenarios below cover, except the close handler order, ends as it does on `main`. ## Evidence Real headless Neovim, one instance per scenario, `tools.handle_invoke` and the real commands, a stub in place of the `claude` CLI. The rows below come from the head before the last main merge (`c7d4f61b`) and `origin/main` at `48ca93be`. On the final head `b001b741`, which merges `origin/main` at `f28eba7d`, an independent verifier reran the close handler probe (0 of 224 runs with a problem, 220 of 224 on `main`) and a probe of the newly merged split and CRLF paths. No row is worse than `main`. Neovim 0.12.3 and v0.8.0 give the same rows. | Order | `main` | this head | | --- | --- | --- | | Close handler, split layout, X a new file | X rejected, B never answered, A opens | X and B rejected, A opens and is answered at accept | | Close handler, split layout, X an existing file | A fails with E95, B never answered, 2 tabs | X and B rejected, A opens, 1 tab | | Close handler, unified layout, X new or existing, with or without a leftover buffer | B never answered | B rejected, A opens | | Close handler, split layout, with a leftover buffer, B an existing file that shares A's proposal name | A and B fail with E95 | same as `main` | | Close handler, split layout, with a leftover buffer, B a new file | A fails with E95, B never answered, 2 tabs | A fails with E95, B rejected, 1 tab | | Reload inside A's setup (every reload scenario) | unchanged | same as `main` | **Spec.** 9 cases. On `main`, 4 fail and 5 pass, on both Neovim versions. The 4 that fail are the close handler order: `answers a call that a ClaudeCodeDiffClosed handler lets register while the old new-file diff is replaced` and the same title with `existing-file`, each in the vertical and the unified layout. The 5 that pass are the controls: `:w` in A's window after a reload answers A once, and `:q` or `:bwipeout!` on a new-file proposal after a reload, with and without the event loop in the reload, fires one open and one close event, also after `close_tab` and close-all. This head passes all 9. **Mutation.** 6 mutants of the loop, all red on both Neovim versions: the replace step runs once, it cleans up what it did not handle, it does not read the registry again, it does not reject a pending diff, it does not clean up, and it rejects only on the first pass. Removing the `assert` after the cleanup changes no result, because it only states what the cleanup already did. **Reload equals `main`.** The verifier's N1 spec (a reload inside A's setup, a queued call B in the new module, and a close handler that closes A's window) gives the same rows on `main` and on this head, on both versions (2 cases, both pass). The verifier's D1 probe (a reload inside A's setup, then `:q`, `:bwipeout!`, deny or write in A's window, then `close_tab`; 16 runs per tree) gives the same 16 rows on both trees. **Other orders.** I ran 776 scenarios on `main` and on this head, on both versions (1552 runs per tree): the 657 scenarios of the first verifier of coder#512 (the core set, the event matrices with and without notifications, three-deep, three queued calls, the window orders, the V3 orders and the reload-after-action orders), the reload, I1, V2, W1, N1, three-deep and three-queued scripts of the second verifier and its 90-scenario harness, and my own close handler orders. 1538 runs give the same output on both trees. 14 differ, which are 7 scenarios on each version, and all 7 are the close handler order: the four split and unified orders without a leftover buffer, the two unified orders with one, and `f3_diffclosed_pump`. In each, `main` leaves a call unanswered and this head answers every call once with its own text and leaves no extra window or tab. **Gate** on the final head (`b001b741`, which contains `origin/main` at `f28eba7d`, coder#558 and coder#573 included): `mise run clean && mise run all` exit 0 (1904 successes, 0 failures, 37 pending, luacheck 0 warnings in 201 files). The spec runs 9 of 9 on Neovim 0.12.3 and on v0.8.0. The coder#558 specs pass too: `tests/unit/diff_exit_reject_spec.lua` 6 of 6, and `tests/integration/diff_exit_reject_spec.lua` 34 of 34 on both versions. The coder#573 specs `diff_close_save_spec` (77 of 77) and `diff_left_window_spec` (40 of 40) pass on both versions. The integration runner passes with 865 successes and 0 failures on Neovim 0.12.3 and 861 and 0 on v0.8.0, each also under a `TMPDIR` with a space and a `%`. `check.sh` PASS (93477 and 5557 states) and `--hash-only` is fresh for all five verify skills. The 6 mutants of the loop are red on both versions. On `main` the spec fails 4 of 9 cases and passes 5. ## Not in this PR - A reload inside a setup, and a reload after a setup has finished: the old module keeps the diff, as the module README says. A fix needs a design for what the new module may take over. The leftover-buffer case is coder#512's. - A `WinClosed`, `BufWipeout` or `BufHidden` handler that runs the event loop while the diff X is cleaned up (not in the `ClaudeCodeDiffClosed` event) also lets B register. The last step of `_cleanup_diff_state` then removes B without an answer in some variants (a `BufWipeout` or `BufHidden` handler on a new file still answers B), and X's `ClaudeCodeDiffClosed` fires twice, on `main` and on this head. A guard on that step reaches every cleanup, not only a replace, so it changes more orders than the one here. - A second `openDiff` that registers while the first one is still inside its setup in the same module, without a reload: B is never answered and an extra tab can stay, on `main` and on this head. coder#512 handles it with a refusal. - The four leftovers that the last verifier found (the `accept_other` order, one SKILL.md line, a `FileType` handler that closes the proposal window during setup, and a call that registers during exit) are tracked in coder#603. The last two are on main too. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
## Why Neovim refuses window changes in two states: while the user is in the command-line window (`q:`, E11) and under textlock (E565, for example while an `<expr>` mapping waits for a key). coder#499 holds the disconnect cleanup in those states. Five other paths still tear a diff down at that moment and raise: - the server stops (`:ClaudeCodeStop`, or the plugin stops itself), - Claude calls `close_tab`, - Claude calls `closeAllDiffTabs`, - a second Claude client sends `openDiff` with the `tab_name` of an open diff (a replacing openDiff), - a bulk close of many diffs. What the user sees on `main` at `d8e767c` (coder#499): `stop` raises E11 or E565 before the TCP teardown, a pending openDiff call is never answered, the server keeps running, and the next start says "Already running". A replacing openDiff raises in `diffoff`, and the old rejected diff stays on screen after the user leaves the window. The unified layout is worse: nothing raises, but the registry entry is dropped while its windows stay, so nothing can close them. `main` has moved since (`700209ee`, with coder#485 and coder#536), and a replacing openDiff in the command-line window now behaves differently there. On Neovim 0.12.3 it no longer raises in `diffoff`. It drops the registry entry and leaves the old windows, and the new call gets "Diff setup failed". On v0.8.0 the cleanup still raises E11, and the old rejected diff stays registered and on screen. This PR's `diff_cleanup_locked` spec fails 15 of 24 cases on `700209ee` with 0.12.3 and 14 of 24 with v0.8.0. ## What changed - `_cleanup_diff_state` now hands the teardown to `utils.when_windows_allowed`, the gate from coder#499. The old body is `teardown_diff`, and only the gate calls it. When Neovim allows window changes, it runs at once, as before. Otherwise the diff stays registered and the gate's one 50 ms timer runs the teardown later. - The held teardown runs only while `active_diffs[tab_name]` still holds the entry that waited, because a new diff can take the tab name before the timer fires. - The answer to each waiting call goes out before the teardown, so the server stop, `close_tab`, `closeAllDiffTabs` and the bulk closes answer every call at once. - Many held cleanups share the gate's one timer. A spec pins this with 20 held diffs. - A replacing openDiff that finds its old diff held by the gate fails at once with error `-32000` ("Diff setup failed") and runs no setup. A waiting old call gets `DIFF_REJECTED` (an old diff the user already accepted keeps its `FILE_SAVED` answer), and the held teardown closes the old diff after the user leaves the locked state. - `.agents/skills/verify-diff-lifecycle`: the model has a lock on window changes. Three facts (`CleanupHoldsWhenLocked`, `CleanupRetries`, `RetryChecksEntry`) read the gate and `teardown_diff`, and the liveness rule `DeferredCleanupDone` says a held cleanup runs once windows can change. After the merge the model also holds coder#536's reject pass and exit cleanup, about 256,000 states. - `.agents/skills/verify-claudecode-nvim`: two journeys, `journey-diff-disconnect-cmdwin.sh` and `journey-diff-replace-cmdwin.sh`, drive the paths in a real Neovim terminal with the scripted Claude client. ## Merge of main This head merges `origin/main` at `700209ee` (coder#485 is in) into the branch with a normal merge commit (`9baedc48`). No rebase, no force push. Eight files conflicted. I kept main's code in each one and put this PR's gate around it. | File | Resolution | | --- | --- | | `lua/claudecode/diff.lua` | Kept main's `end_diff_mode_if_alone`, `end_diff_mode_in_windows_that_show_the_file`, `release_surviving_diff_tab` and the `diffoff` changes. `teardown_diff` now holds main's whole body, including the new-tab release from coder#485. `_cleanup_diff_state` still calls the gate. The replace loop of coder#536 asserted that the old entry is gone after `_cleanup_diff_state`. With the gate the entry can stay while windows are locked. The merge turned the assert into a `break`, and a verifier found that this was wrong (see "Finding F1" below). `ebff0dc2` replaced the `break` with the call that fails at once. | | `lua/claudecode/README.md` | Kept main's bullets and this PR's gate bullet. The bullet on a refused `:diffoff` now says the cleanup waits for the gate. | | `verify-diff-lifecycle/model/DiffLifecycle.tla` | Kept both sides: main's `RejectEach`, `DiffCleanupAtExit`, `ExitInSetup` and the exit constants, and this PR's `DeferredCleanupRuns`. `Next` lists all of them. | | `verify-diff-lifecycle/check.sh` | The rules end with main's `ExitedCallsAnswered`. The live properties are `DisconnectedCallsAnswered` and `DeferredCleanupDone`. | | `verify-diff-lifecycle/model/statechart-view.ts` and `SKILL.md` | Both sides kept. The notes state the merged model's size and the replace loop's exit. | | `verify-claudecode-nvim/SKILL.md` | Main's journey list plus this PR's two journeys. | `covered.lock` of `verify-diff-lifecycle` came from main and I re-recorded it in its own commit. It was the only STALE lock of the five. Two more re-records followed, one for a comment-only edit of `diff.lua` and one for the replace-loop fix. The model's facts still match the code, and the model never lets a replacing openDiff find windows locked (its `SKILL.md` says what the code does there). Every teardown path that main added since `ef610b7d`: | Path | Gate | | --- | --- | | coder#485 new-tab release (`release_surviving_diff_tab`) | Behind the gate. `teardown_diff` calls it, directly in the split layout and through `cleanup_inline_diff` in the unified layout, and nothing else calls it. | | coder#573 and the split-close helpers (`_close_proposal_window`, `end_diff_mode_in_windows_that_show_the_file`) | Behind the gate through `teardown_diff`. | | coder#536 exit cleanup (`cleanup_at_exit`) | The reject pass resolves statuses, and its one window change (`keep_empty`) calls `_cleanup_diff_state`. The cleanup pass calls `_cleanup_all_active_diffs`. Both go through the gate. | | coder#536 replace loop in `open_diff_blocking` | Calls `_cleanup_diff_state`, so it is gated. When the gate holds the old diff, the loop fails the call before any setup. | | Setup-failure cleanup and `remove_failed_proposal` | No gate for the setup's own leftovers (the branch for a diff that the setup already registered goes through the gate). They run only for a setup that failed, and they tell a diff that the setup registered from one that was already registered only by `active_diffs[tab_name]`. The merge's `break` broke that assumption (F1). The replace loop now fails the call before its setup runs, so these handlers see only an entry that the setup registered, or none. | ## Finding F1 (found by a verifier on `0b34b13c`, fixed in `ebff0dc2`) The merge turned coder#536's assert into a `break`. A replacing openDiff B then ran its setup while the old diff A was still registered under the same `tab_name`. When that setup failed in a locked state, its failure handlers saw A's entry. They held A's teardown a second time and skipped B's own cleanup. What B's setup had made stayed behind: - on Neovim 0.8, a window that shows the `[Command Line]` buffer after the user leaves `q:`, - a hidden proposal buffer that is marked modified, so `:qa` stops with E37 on v0.8.0, - a buffer name that made every later openDiff of that kind and tab name fail with E95 ("Failed to rename buffer" on v0.8.0). It needs a second openDiff with the `tab_name` of an open diff while the user is in `q:` or under textlock. PROTOCOL.md says Claude Code sends a new random `tab_name` per call, so only other clients that reuse a `tab_name` reach it. Fix: when the gate kept A's entry, the replace loop fails B's call with `-32000` ("Diff setup failed") before any setup. I used this form and not the verifier's alternative (the failure handlers compare the entry with the one registered before the setup). Every setup fails in those states anyway, so the first form runs nothing that would need undoing. It changes one place, where the alternative changes the failure handlers in `diff.lua` and in `diff_inline.lua` and still runs a setup that cannot work. The loop holds A's teardown once. B's setup used to hold it a second time. ## Known limit `close_tab` of a diff the user already saved still raises E11 in those states. The file reload that runs 100 ms after the close (`reload_file_buffers_manual` from `close_diff_by_tab_name`) does not go through the gate yet. I re-ran it on this head on 0.12.3 and on v0.8.0, in the command-line window (E11) and under textlock (E565). The reload raises from `edit`, and the file's buffer keeps its old text, because the reload never runs. The diff itself closes after the lock. `main` is no better: its `close_tab` itself fails in those states, the diff stays registered, and the reload raises the same way. A follow-up PR routes the reload through the gate. A replacing openDiff in those states holds the old diff, and the new call fails with error `-32000` ("Diff setup failed") before it sets anything up, so it leaves no window or buffer. The old diff closes once the user leaves the window. Holding the new openDiff itself is not part of this PR. ## Tests I used the `test-audit` authoring gate, `principle-test-behavior-not-implementation` and `tdd` (failing check before the fix, then pass after) for every test change after the merge: - New `diff_replace_locked_spec.lua` (24 cases on each build) drives a replacing openDiff through `tools.handle_invoke` in the command-line window and under textlock, in the split and unified layouts, with the old and the new file of different kinds (existing and new, both orders), while the user is in another tab. Per combination it checks three behaviors. The old call gets `DIFF_REJECTED` once, the new call gets one `-32000` answer, and the old diff stays registered while locked. After the lock the old diff closes, no window shows `[Command Line]`, no proposal buffer is left and no window is left over. A later openDiff with the same tab name and the new call's kind opens and is answered once. On `0b34b13c` 10 of the 24 cases fail on both builds (the command-line window, the leftover proposal buffer, and the later openDiff). With the fix all 24 pass on 0.12.3 and on v0.8.0. - `diff_left_window_spec.lua`, case "answers close_tab from the command-line window...": it pinned the pre-gate behavior (the copy keeps its diff mode). It now expects the diff to stay registered while the window is open, to close after the window closes, and to end diff mode in the copy afterwards. The `no gate` mutant turns it red. - `diff_locked_paths_spec.lua`, replacing openDiff cases: the old assertion looked for `"error"` in the reply. The mutant that restores coder#536's "entry is gone" assert in the replace loop survived it (0 red). The assertion now looks for "Diff setup failed", and the same mutant turns 2 cases red (command-line window and textlock). - `diff_cleanup_locked_spec.lua`: I removed its skip of the unified layout below Neovim 0.9. Main dropped that claim in coder#564, so all 24 cases now run on v0.8.0. - `diff_locked_paths_spec.lua` counts `vim.defer_fn` calls and uses `nvim_replace_termcodes` through `tests/helpers/window_lock.lua`, because v0.8.0 has no `vim.keycode` and `uv.walk` crashes there. - `journey-diff-replace-cmdwin.sh` no longer sends Enter at a "Press ENTER" prompt. The refused call now logs nothing, and Enter would run the command line and close the window. ## Verification On `d8e767c` I replayed each path over a real socket in headless Neovim 0.12.3 (the textlock cases use `getchar()` in a typed `<expr>` mapping). Result after the change: | Path | `main` | this branch | | --- | --- | --- | | stop in the command-line window | `stop` raises E11, server keeps running, restart says "Already running" | stop returns true, both calls answered, both diffs close after the window, restart works | | stop under textlock | raises E565, same leftovers | same as above | | replacing openDiff, command-line window | error names `diffoff`, old rejected diff stays | new call gets -32000 before its setup, old diff closes after the window | | replacing openDiff, textlock | same | same | | 30 held cleanups | `close_pending_diffs` raises E11 after the first | 30 diffs close, 1 timer armed | Replacing openDiff, all combinations (a verifier's matrix, rerun by me on this fix and on `700209ee`, headless, one fresh Neovim per row): 288 scenarios on each of 0.12.3 and v0.8.0, 576 rows. Scenario axes are layout (vertical, horizontal, unified), new-tab diff or not, lock (`q:`, textlock, none), user in the same or another tab, old and new file existing or new, and the old diff pending or saved. Each row also opens a later openDiff, accepts it and closes its tab. - No row is worse than `700209ee`, and 376 rows are strictly better. The 384 rows with a lock have no leftover. The other 192 rows have no lock and give the same result line as `700209ee` in every row. - `0b34b13c` is worse than `700209ee` in 40 rows (a row can count in more than one: 24 with a later openDiff that fails, 16 with a leftover buffer, 8 with an extra window), and none of them is worse on this head. - With the new call arriving through `vim.schedule` (the way a socket message arrives), the result is the same: 0 worse rows, 376 better. - coder#536's close-handler probe (224 runs) and split-path probe (320 runs) give identical output on `700209ee` and on this head in every run. The locked close-handler orders (448 runs): 0 problems on this head, 358 on `700209ee`, none worse. - The verifier's standalone repro (user in another tab, textlock, new file, then two later new-file openDiffs) answers both later calls with E95 on `0b34b13c` and opens both on this head, as `700209ee` does, on 0.12.3 and on v0.8.0. Gates on the merged head `74bbd458`, one run each, none passed only on retry: - `mise run clean` and `mise run all`: exit 0, 2026 successes, 0 failures, 0 errors, 54 pending, luacheck 0 warnings in 227 files, and all 5 static proofs ok. The new spec is one more pending case under busted, because it needs a real Neovim. - Twelve specs, each run in real Neovim on 0.12.3 and on v0.8.0, native exit 0 each time: `diff_cleanup_locked` 24 cases, `diff_locked_paths` 7, `diff_disconnect_window_lock` 2, `diff_new_tab_autocmds` 14, `diff_new_tab_close_mode` 10, `diff_new_tab_last_tab` 12, `diff_new_tab_mode` 12, `utils_without_autocmds` 4, `diff_answer_every_call` 9, `diff_close_save` 77, `diff_left_window` 40 and `diff_replace_locked` 24. All 0 failures and 0 errors. The first two and the last are this PR's specs. The other nine cover the new-tab, answer-every-call, close-save and left-window teardown code that main changed, and the disconnect window lock from coder#499. - `scripts/run_integration_tests_individually.sh` with the stub `claude` first on `PATH`: 1161 successes on 0.12.3 and 1157 on v0.8.0, 0 failures, 0 errors, exit 0. Each ran twice, once with a plain `TMPDIR` and once with a `TMPDIR` that holds a space and a percent sign. - `check.sh` of all five skills: PASS. States: `verify-diff-lifecycle` 255,881 (the two live properties 13,509 each), `verify-mention-queue` 540,240, `verify-selection-tracking` 95,938, `verify-terminal-lifecycle` 279,492, `verify-ws-connection` 262,285. The three fact mutants of the merged model fail as expected (run before the F1 fix, which does not touch the model): `CleanupHoldsWhenLocked=FALSE` breaks `NoRejectedLeftover`, `CleanupRetries=FALSE` breaks `DeferredCleanupDone`, and `RetryChecksEntry=FALSE` breaks `NoStrandedCall`. - `journey-diff-disconnect-cmdwin.sh` and `journey-diff-replace-cmdwin.sh` both print PASS on this head in a real Neovim terminal, and `vcn.sh cleanup` leaves no process. They read `OUT` and `WORK` through `vcn.sh get`, because main's `no-source-run-env` static proof refused the `source` of `run.env`. - Eight mutants, each run on the five spec files that cover the gate and the replace loop (`diff_cleanup_locked`, `diff_locked_paths`, `diff_disconnect_window_lock`, `diff_left_window`, `diff_replace_locked`). The count is red cases on 0.12.3 and on v0.8.0, and a no-op control is green on both: - no gate (`_cleanup_diff_state` calls `teardown_diff` directly): 38 and 35, - no identity check (the held function drops the `active_diffs[tab_name] == diff_data` test): 12 and 12, - own timer per cleanup (`when_windows_allowed` arms a `vim.defer_fn` on every call): 3 and 3, - held function never runs (the retry loop walks an empty list): 39 and 39, - entry dropped while held (the entry is removed before the gate holds the teardown): 39 and 37, - probe always allows (`windows_allowed` returns true): 40 and 37, - replace loop breaks (the merge's form, the cause of F1): 10 and 10, all in `diff_replace_locked`, - replace loop asserts the entry is gone (coder#536's assert): 2 and 2, both in `diff_locked_paths`. The counts depend on the exact edit, so they are not comparable with the counts of an earlier head. - Earlier on `d8e767c`: 6 of the 7 `diff_locked_paths_spec.lua` cases and 16 of the 24 `diff_cleanup_locked_spec.lua` cases fail there. `journey-diff-replace-cmdwin.sh` fails there at the `diffoff` step. On `700209ee` this PR's specs fail 15 and 14 of the 24 `diff_cleanup_locked` cases (0.12.3 and v0.8.0), 6 of the 7 `diff_locked_paths` cases on both, and 16 and 14 of the 24 `diff_replace_locked` cases. Refs coder#483, coder#485, coder#499. coder#485 is merged, and this PR resolves the overlap in `_cleanup_diff_state`. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
Why
#494 ended diff mode in windows that show the file after a split diff closes. A window can have diff mode again after the diff because the plugin ran
diffthisand then split. This PR removes that cause in both open paths: the split comes first.Refs #494.
Cause
Neovim saves the options of a window, 'diff' included, for each buffer that leaves it, and sets them again when the buffer enters the window. A split copies the options of its source window.
Both open paths ran
diffthisin the original window and then split it. The proposal window copied diff mode and saved it for the file when it showed the proposal. So:edit <file>back in that window turned diff mode on after the diff had ended. A window in a new tab page (:tabnew <file>,:tabnewand:buffer <file>,:tab sbuffer) took that saved set too, while the diff was open.What changed
All code is in
lua/claudecode/diff.lua.setup_new_bufferandshow_native_diff(the legacy native path,diff.open_diff): split first, rundiffthisin the proposal window, then in the original window. The proposal window saves no diff mode for the file.diffthisinnvim_win_call. A switch to that window and back fired 8 moreWinLeave,WinEnter,BufLeaveandBufEnterevents than on main (12 against 4 in a probe). ABufEnterhandler that moved the focus then put diff mode in the user's other window (0.12.3) or madeopenDifffail (0.8.0). Withnvim_win_callthe events are the same as on main (probe below).lua/claudecode/README.mdand.agents/skills/verify-claudecode-nvim/features/open-diff.md: describe the split order.tests/mocks/vim.lua: thenvim_win_callmock accepts every window id, asnvim_set_current_windoes there. With main's stricter mock, one unit spec fails (tests/unit/new_file_reject_then_reopen_spec.lua, the blocking path). The mock now checks less than real Neovim, which rejects an invalid window id. The integration specs call the realnvim_win_callon every open.tools.setup()instead oftools.register_all().setupstores the server and then callsregister_all, so the tests behave the same.WinNew,WinEnter,BufWinEnterorFileTypehandler closes the original window while the diff opens, this head answersopenDiffwith an "Invalid window id" error, restores the layout and fires noClaudeCodeDiffOpened. Main opens a diff that has only the proposal window, and in two of those handler kinds leaves the file window in diff mode after close.covered.lockis re-recorded in its own commit. Only the hash ofdiff.luachanged. No block that the model cites changed, andcheck.shpasses with TLC.git diff origin/main...HEAD -- lua/touchessetup_new_bufferandshow_native_diffand one bullet of the README. Nothing else inlua/changes. The sweep (end_diff_mode_in_windows_that_show_the_file,end_diff_mode_if_alone) and the delete block of_cleanup_diff_statestay as on main.What stays as on main
These are not part of this PR. An earlier head of this PR changed them. Their fixes will ship in follow-up PRs in this run.
:b <proposal>andclose_tab): the sweep resets it, as on main. Follow-up: handler state during proposal delete.:diffoff | diffthisin a:splitcopy, and a:tab splitcopy in another tab page: main's sweep ends the first and leaves the second. Follow-up: user diff mode in split copies.These stay as on main, because Neovim saves or copies the window's options. No PR in this run changes them. The
close_taberror in the last item belongs in main's close path (#511).:edit third.txtin the original window, then:edit #: Neovim saved diff mode for the file in that window, so it comes back.:split <file>from a window that left the file: the new window takes the saved set.ClaudeCodeDiffClosedhandler that brings the file back into a window.:diffoffin the command-line window, the window keeps its diff mode.close_tabfrom the command-line window answers an error on both Neovim versions (single plain diff, probed on main).Before and after
Real Neovim, the plugin's own
openDiffandclose_tabhandlers, on 0.12.3 and v0.8.0, main against this branch::edit other.txt,close_tab,:edit <file>diff=truediff=false:edit other.txt, then:edit <file>, in the legacydiff.open_diffdiff=truediff=false:tabnew <file>,:tabnew+:buffer,:tab sbufferwhile the diff is opendiff=truediff=falseValidation
Head c67c231 (the fix, eight merges of main, the last one of
c7e2eb4e, the lock re-recorded in its own commit, and the spec and mock commits).mise run clean && mise run all: exit 0. luacheck 0 warnings, 0 errors in 197 files. busted 1892 successes, 0 failures, 0 errors, 35 pending.diff_left_window_spec.lua40 cases,diff_close_save_spec.lua77 cases): 0 fail on 0.12.3 and v0.8.0. Against main'sdiff.lua, 6 cases fail on both versions: the proposal-window:editcase (1), the legacy case (1b), the three new-tab cases (1c), and the:edit #row ofdiff_close_save_spec.lua. 32 of fix(diff): end diff mode in windows that show the file after a split diff closes #494's 33 rows are unchanged and pass. The 33rd, the proposal-window:editrow, expectsdiff=falsenow. Three cases are parity cases (green on main too): aBufEnterhandler that moves the focus while the diff opens, and noWinEnterfor the original window while each open path runs (blocking and legacy).claudefirst on PATH, normal and under aTMPDIRwith a space and a%: 0.12.3 838 successes and v0.8.0 820, 0 failed, 0 errors, 37 files each. All four runs (two versions, twoTMPDIRshapes) passed on the first attempt. The cache folder stays untouched.diffthisin each open path, the original window'sdiffthisin each path, andnvim_win_callinstead of a window switch in each path). All red on both versions, and the unmutated baseline is green. They ran on24468969, whosediff.luais the same as this head's.check.sh(TLC): pass, 93477 states for the main model and 5557 for the live model. I did not runcheck.sh --mutants.OptionSetfires), main against this branch, on 0.12.3 and v0.8.0:BufEnterorBufWinEnterhandler that raises while the proposal buffer is deleted (9 scenarios): 126 checks, 0 fail here and pass on main. The answers are the same as on main.close_tabthat arrives while the command-line window is open does not close a diff on main either: in these sequences main answersTAB_CLOSEDbut leaves the diff's tab on screen, untracked, and a single plain diff fails on both versions. Withclose_tabdelivered the way the server delivers it (fromvim.schedule), this head answersTAB_CLOSEDin the author's sequence on both versions. In a shorter sequence it answers anE11error and keeps the diff on screen and registered. The fix belongs in main's close path, which open PR fix(diff): hold diff teardowns behind the shared window gate #511 ("hold every diff teardown behind the shared window gate") changes.:splitcopy in diff mode that this branch does not.Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh