Repository navigation
fix(diff): keep Neovim alive when a handler raises during a failed openDiff setup - #538
Conversation
…enDiff setup Neovim 0.8.0 bundles a LuaJIT that has issue #1066 (fixed in September 2023). A Neovim API call raises inside the openDiff coroutine because an autocmd callback it ran raised. LuaJIT then keeps pointing at the main Lua thread, and the next JIT trace exit corrupts the stack of that thread. Neovim aborts (exit 134, "LuaJIT ASSERT lj_api.c:40: index2adr: bad stack slot -1"). plenary ignores the death by signal, so diff_failed_setup_guards_spec.lua printed no summary about 3 times in 100 runs on Neovim 0.8.0. Make one Neovim API call that returns normally before the failed setup rethrows. It points LuaJIT at the running coroutine again. The spec gets a case that forces the JIT to compile at once and repeats the three raising-handler cases. Without the change it aborts Neovim 0.8.0 in most runs.
|
@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 Review: Didn't find any major issues. Keep them coming! 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. |
covered.lock conflicted with #527. Took main's covered.lock and re-recorded it with check.sh --record after check.sh passed on the merged tree. Also names exit 139 (SIGSEGV) next to exit 134 in the comment of the workaround and in the spec comment. No code line changed.
|
@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. Nice work! 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". |
…#550) Comment-only follow-up of coder#538. No code line changes. ## Why The comments on the workaround for LuaJIT issue #1066 (the call before the rethrow in `_setup_blocking_diff`, and the eager-JIT case in `diff_failed_setup_guards_spec.lua`) named Neovim 0.8.0 as the only affected version. That is too narrow. Someone who moves the minimum version to 0.9 would read "0.8.0", conclude the call is dead, and delete it. The comments now say "a LuaJIT older than September 2023 (bundled by Neovim 0.8 and 0.9)". ## Evidence | Neovim | LuaJIT it bundles | Fix e86990f7f (21 September 2023) | Result | | --- | --- | --- | --- | | 0.8.0 (`v0.8.0-1210-gd367ed9b2`) | 633f265f (August 2022) | missing | the minimal repro aborts in most runs, see coder#538 | | 0.9.5 (official release, `LuaJIT 2.1.1692716794`) | 03c31124 | missing: 70 commits older than the fix (GitHub compare) | minimal repro `luajit1066b.lua`: 10 abort of 10 runs. This spec on the product code without the call, 4 parallel loops of 8: 29 bad of 32. With the call: 0 bad of 32 | | 0.10.0 to 0.10.4 | 75e92777 | present: 87 commits after the fix (GitHub compare) | not run | | 0.11.0 | 538a8213 | present: 172 commits after the fix (GitHub compare) | not run | | 0.12.3 | `LuaJIT 2.1.1774638290` | present | minimal repro survives 10 of 10 runs, see coder#538 | The 0.9.5 runs used the official `nvim-linux64.tar.gz` of v0.9.5. "Bad" means the spec printed no plenary summary. The 0.10 and 0.11 rows come from comparing the LuaJIT commit in `cmake.deps/deps.txt` at each tag with e86990f7f. I did not run those releases. ## Checks - Only comment lines differ from main in `lua/claudecode/diff.lua` and in the spec (a diff with comment lines removed is empty). - `covered.lock` is re-recorded. `check.sh` read STALE before the record (the `setup_blocking` block hash changed). Its model passes (main 89205 distinct states, live-DisconnectedCallsAnswered 5269) and no fact is FALSE. `--hash-only` matches after the record. - `luacheck lua/ tests/ --no-unused-args --no-max-line-length`: 0 warnings, 0 errors in 181 files. `stylua --check lua tests`: clean. - Unit suite without coverage (`busted --shuffle --seed=8888`): 1702 successes, 0 failures, 0 errors, 25 pending. - `scripts/run_integration_tests_individually.sh`: Neovim 0.12.3 Total Success 417, Failed 0, Errors 0. Neovim 0.8.0 Total Success 309, Failed 0, Errors 0. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
… I/O citations (coder#549) ## Why The READMEs name the buffer that holds Claude's proposed text in two ways. `README.md:1144` and `:1148`, `ARCHITECTURE.md` and 8 of the 16 uses in `lua/claudecode/README.md` say "proposal buffer". `lua/claudecode/README.md` and the new paragraph in `README.md:254` (from coder#546) also say "proposed buffer". The advisor of coder#546 asked for one name for one thing. The advisor of coder#547 named two small items in `ARCHITECTURE.md`. This PR takes them too, after coder#546 and coder#547 landed. - The Resource Cleanup sketch calls `M.stop()` with no condition. The real `VimLeavePre` callback runs `stop` only when `M.state.server` is set, and otherwise clears the @-mention queue (`lua/claudecode/init.lua:514-525`). - The I/O bullet says the TCP server "writes with libuv's asynchronous `write`" and cites nothing for it. ## What changed **1. The rename.** Nine words in two files. Each "proposed buffer" becomes "proposal buffer", and nothing else changes (a word-level diff shows 9 changes of "proposed" to "proposal" and no other change). | File | Lines | Count | |---|---|---| | `README.md` | 254 | 1 | | `lua/claudecode/README.md` | 181, 182, 183 (twice), 184, 187 (twice), 208 | 8 | I read each changed sentence in its new form. All nine read correctly, for example "A write of the proposal buffer (`:w`, `:wq`, `:x`, `ZZ` ...)" and "Both layouts keep the proposal buffer's `'bufhidden'` at `hide`". The other phrases in these two files already use "proposal" (`proposal window`, `proposal lines`, `proposal buffers`). `proposed changes` in `README.md` is a different thing and stays. **2. Resource Cleanup sketch** (`ARCHITECTURE.md`). The sketch now shows the `if M.state.server then M.stop() else clear_mention_queue() end` of the real callback, with its `ClaudeCodeShutdown` group. Every code line comes from `init.lua:514-525`, except two: the label `-- init.lua, M.setup (shortened)` and the trailing comment on `M.stop()`, which the old sketch had and which describes `M.stop` correctly. The other comment line is the real comment at `init.lua:520`. The lead-in "Automatic cleanup on shutdown:" had the same unconditional wording, so it now says that the autocmd that `setup` creates runs `stop` only when the server runs, and otherwise only clears the queue, with citations. This agrees with the Performance bullet ("runs `stop` when the server runs"). **3. I/O citation** (`ARCHITECTURE.md`). The write clause now cites `[lua/claudecode/server/client.lua:write]`. `write` is `local function write(client, data, callback)` at `client.lua:81`, and it calls `client.tcp_handle:write` at `:84` and `:90`. ## Scope The two README files above and the two `ARCHITECTURE.md` items. Left unchanged on purpose: - **Other bullets and sketches of `ARCHITECTURE.md`.** Not audited again here. - **Code comments and strings under `lua/`** (9 uses of "proposed buffer" in lowercase, plus `Proposed buffer ID` at `diff.lua:1187`). They sit in `diff.lua` (8) and `init.lua` (1). Both files are recorded in `covered.lock`, so a comment edit needs a new record, which is too much for a wording fix. - **`fixtures/issue-218/README.md` (2) and `fixtures/issue-238/README.md` (7).** They describe repro scripts. The sample output in `fixtures/issue-238/README.md` (`proposed buffer bufhidden = "hide"`) is what `scripts/repro_issue_238.lua` prints (`:260` and `:279`). A rename of the prose alone leaves two names in one README, and a rename of the output changes a script string. They stay as they are, because the fixture READMEs record what those scripts print. ## Blast Radius Three Markdown files. Nothing loads them. A wrong sentence here misleads a reader and breaks no behavior. ## Verification - Word-level diff of the two READMEs against `main`: exactly 9 changes of "proposed" to "proposal", no other change. - `grep` for "proposed buffer" in `README.md`, `lua/claudecode/README.md` and `ARCHITECTURE.md` finds 0 after the change. - The merge of `main` (coder#547 and coder#538) was clean and added no "proposed buffer" to a README. - `mise run format-check`: exit 0 (358 files, 0 changed). - Citation check over the seven READMEs and `ARCHITECTURE.md`: 809 entries (733 citations, 26 relative links and 50 test paths), 1 flagged. The flagged one is `[plugin/claudecode.lua:pcall(require, "claudecode")]`, an expression that `main` already cites. A negative control with three wrong symbols in the new citations flagged all three (bad 4, then 1 after the restore). The validator matches a plain name such as `write` only as a whole word anywhere in the file, so I also confirmed the definitions of `write` (`client.lua:81`) and `clear_mention_queue` (`init.lua:150`) with `grep`. - I did not run `mise run all`. The change is documentation only, and no test, script or CI file reads these files. CI runs the full suite on the pushed head. - `git merge-tree --write-tree` of the head and `origin/main` is clean. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
…split layouts (coder#551) ## Why When a user empties the proposal buffer (`ggdG` or `:%delete`) and saves it with `:w`, Claude must get what Neovim's own `:w` writes. Neovim writes no bytes for an emptied buffer. With `'eol'` on, the `vertical` and `horizontal` layouts answered `" "`, so Claude wrote a one-byte file where the user had asked for an empty one. The F14 verifier found this. PR coder#542 listed it as a known, pre-existing defect that this PR fixes. ## What changed `_resolve_diff_as_saved` in `lua/claudecode/diff.lua` saves the lines of the proposal buffer, joined with line breaks. It adds a final line break when the buffer's `'eol'` is on. A buffer with no lines at all (Neovim shows "--No lines in buffer--") still gives one empty string through `nvim_buf_get_lines`. So the save could not tell it from a buffer with one empty line, and it added the line break. The save now counts the bytes of the buffer with `vim.fn.wordcount().bytes`, run inside `nvim_buf_call`. The count is 0 for an empty buffer and at least 1 for a buffer with one empty line when `'eol'` is on. The line break is added only when the count is not 0 and `'eol'` is on. That replaces the check `#content_lines > 0`, which was always true. I tried `nvim_buf_get_offset` and `line2byte` first. Both gave wrong sizes for a buffer that `ggdG` had emptied (a stale line cache), so I did not use them. Other changes: - `tests/mocks/vim.lua` gets `wordcount()` and an `empty = true` flag for a mock buffer. `nvim_buf_call` in the mock now names the buffer that the callback acts on. - `tests/unit/diff_ui_cleanup_spec.lua` gets one fixture line. The mock's `nvim_buf_call` refuses a buffer that does not exist, and that spec saved a buffer that the mock never had. - `lua/claudecode/README.md`: one bullet replaces coder#542's bullet about the saved content of the split layouts. It states both rules, `'eol'` from the proposal and the empty buffer. It also says that a buffer with one empty line is saved as `"\n"` when `'eol'` is on and as `""` when it is off. - `.agents/skills/verify-diff-lifecycle/covered.lock` is re-recorded (see Blast Radius). ## Scope - The `vertical` and `horizontal` layouts share one code path, so one change fixes both. - The `unified` layout is unchanged. It answers with the proposal, not with the buffer, so what the user does to its buffer never reaches Claude. The new spec runs it as a control. - With `'eol'` on, a buffer with one empty line is still saved as `"\n"`. With `'eol'` off, it is saved as `""`, as before. A buffer with any line is saved as before. - On `main` (with coder#542), this PR changes the answer only for a buffer that the user emptied. It changes no answer for an unedited proposal: coder#542 already makes an empty proposal give `""`, and the new spec has that case as a control. ### Minor findings that I declined Two verifiers (Opus and GPT) found these. None of them changes an answer that a user reaches with the default settings. - **A buffer set to `bomb` (Opus F3).** With `ggdG`, then `:setlocal bomb`, then `:w`, Neovim writes the 3-byte BOM. Both `main` and this PR answer without the BOM (`main` answers `"\n"` when `'eol'` is on and `""` otherwise, this PR answers `""`). The plugin never sets `'bomb'` on its scratch proposal buffer, and no answer of the plugin ever holds a BOM, so a user has to set a non-default option on a scratch buffer first. I left it. - **The `nvim_buf_call` wrapper is covered by the mock spec only (Opus F4).** If I delete the wrapper, only the mock unit spec fails. In the paths that save (`BufWriteCmd` and `accept_current_diff`) the current buffer is always the proposal buffer, so the wrapper is defensive and no live answer changes. I kept the wrapper, because `wordcount()` reads the current buffer, and I added no live test for a case that no path reaches. - **Cost of `wordcount()` (Opus F5).** It adds a second pass over the buffer. For a 33 MB proposal that is about 0.1 to 0.2 s (0.12.3: +112 ms, 0.8.0: +216 ms), about the cost of `nvim_buf_get_lines` plus the join. I accept that for one save. ## Blast Radius - User-visible: a proposal buffer that the user emptied is accepted as an empty string. Every other buffer gives the same text as before. - `wordcount()` runs in the proposal buffer through `nvim_buf_call`. It does not change the user's current buffer or window. Both functions exist in Neovim 0.8. I ran the specs on Neovim 0.12.3 only. The CI job `integration-tests (v0.8.0)` runs the new spec on 0.8. - `covered.lock`: the change touches `_resolve_diff_as_saved`, a covered block. `check.sh` printed STALE for the `diff.lua` file hash and the `resolve_saved` block hash only. It printed no changed call site, and all 21 code facts stay TRUE. The guard that lets only a `pending` diff resolve is unchanged, and so are the status change to `saved` and the one call of the resolution callback. The change only decides whether the `text` string of the `FILE_SAVED` answer ends in a line break. The model stores which kind of answer a call got, not its text, so no modelled transition changes. The model passed (`main`: 89205 distinct states, `live-DisconnectedCallsAnswered`: 5269). `check.sh --record` wrote the new lock and `check.sh --hash-only` reports that it matches. ### Merge with coder#542, coder#539 and coder#535 coder#542 (F14) landed first, on 2026-11-12 at 04:36Z, and coder#539 (F12) at 04:46Z. coder#538 and coder#550 later changed `diff.lua` too, and coder#549 renamed "proposed buffer" to "proposal buffer" in the README. This PR merged `origin/main` (9bfc39c) with merge commits. It ran `check.sh --record` after each change to `diff.lua` and kept main's README wording in the last one. Only the `diff.lua` file hash and the `resolve_saved` block hash differ from the lock on `main`. The full `check.sh` passes and `check.sh --hash-only` matches. - One README bullet replaces this branch's bullet and coder#542's bullet, because the two said almost the same sentence and this branch's sentence about one empty line was false once `'eol'` can be off. - coder#535 (F11) is still open. It touches neither `diff.lua` nor this README bullet. Its current head (`a5dfc81b`, with `main` merged) merges with this PR without a conflict. ## Verification Red before the fix, green after (same specs, only `lua/claudecode/diff.lua` swapped, on the merged head, so the red column is `main` at 9bfc39c with coder#542): | Spec | `main` (with coder#542) | with the fix | | --- | --- | --- | | `tests/unit/diff_saved_text_spec.lua` (4 cases, mock) | 3 successes, 1 failure | 4 successes, 0 failures | | `tests/integration/diff_accept_emptied_buffer_spec.lua` (40 cases, real Neovim, `:write`) | 28 successes, 12 failures | 40 successes, 0 failures | Against the older merge base (5acc100, before coder#542), the 38-case spec of round a had 24 red cases. Today's 40-case spec has 26 there: the 24 emptied cases plus the new control for an unedited empty proposal in both layouts. coder#542 fixes the 12 cases from proposals without a final line break (it sets `'eol'` off), so 12 stay red on `main`. The integration spec opens a real `openDiff` and edits the proposal window. For the `vertical` and `horizontal` layouts it runs 6 proposals (two lines and a final line break, one line and a final line break, one blank line, one line with no final line break, two lines with no final line break, and no text) with 2 ways to empty the buffer (`ggdG` and `:%delete`). Each case compares the answer with what Neovim's own `:w` writes: the spec edits a real file buffer with the same text and commands, runs `:write`, and reads the file bytes, which are empty. Five controls per layout give the right text before and after (a blank line left in the buffer, no edit, an empty proposal without an edit, one line deleted, and text typed into the emptied buffer). In the `unified` layout, 6 controls check that the answer is the proposal. Gates on the final commit, which merges `origin/main` (9bfc39c) into the branch (private XDG folders): - `mise run all`: native exit 0. luacheck: 0 warnings / 0 errors in 183 files. busted: 1710 successes / 0 failures / 0 errors / 26 pending. treefmt: 0 files changed. - `mise run format-check`: native exit 0 (360 files, 0 changed). - Diff specs (`tests/unit/diff*_spec.lua`, `opendiff_blocking_spec`, `new_file_reject_then_reopen_spec`, and the `open_diff_mcp`, `close_tab` and `close_all_diff_tabs` specs): 226 successes, 0 failures. - `scripts/run_integration_tests_individually.sh`: 457 successes, 0 failed, 0 errors. The new spec gives 40 successes. - `.agents/skills/verify-diff-lifecycle/check.sh --hash-only`: covered.lock matches the code. Two independent verifiers (Opus and GPT) checked the code at `ef8be297`. Both found the code correct. Their blockers were the README sentence and the stale base, and this head fixes both. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
… 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 -->
diff_failed_setup_guards_spec.luaended Neovim 0.8.0 early with exit 0 and no plenary summary in a few of 100 runs. The cause is a LuaJIT bug in Neovim 0.8.0 that a user can hit too. This PR works around it indiff.luaand adds a case that fails without the workaround.Root cause
LuaJIT ASSERT lj_api.c:40: index2adr: bad stack slot -1. The same scenario can also end in SIGSEGV (exit 139): 16 of 125 failed runs of my eager-JIT loop did, and an independent verifier saw 4 of 4 on main with a stand-in terminal layout. I took no backtrace of a SIGSEGV. I assume it is the same bug, because it needs the same scenario and the workaround removes it. plenary's harness only checkscode ~= 0of the child, notsignal, so the outer Neovim exits 0 and the runner printsFAILED: ... no plenary summary (nvim exit 0).integration-tests (v0.8.0)) printsLuaJIT ASSERT lj_api.c:40: index2adr: bad stack slot -1right before the FAILED line. Run 43131434527 shows only the FAILED line. My local runs also lose that line in some aborts, because Neovim's buffered output dies with the process. The Neovim 0.8.0 release commit builds its LuaJIT withCFLAGS+=-DLUA_USE_APICHECK(cmake.deps/cmake/BuildLuajit.cmake), so a bad API call aborts. A LuaJIT without that flag takes the same path but, by the source oflib_aux.candlj_err.c, printsPANIC: unprotected error in call to Lua APIand exits 1. I did not run such a build.LD_PRELOAD, addresses resolved withaddr2line) reads:lua_errorcalled from the Lua VM, thenerr_unwindfinds no protecting frame, thenpaniccallslua_tostring(L, -1)on an empty stack, which trips the assert.G->cur_Lpointing at the main thread. The next JIT trace exit then restores a snapshot into the wrong stack, and a later nested call trips over it. I did not step through LuaJIT to watch each of these steps. The controls below and the upstream fix support them.tools.handle_invokerunsopenDiffin a coroutine (requires_coroutine). So the product hits the bug in production too, not only the spec under plenary. A user whose WinLeave, BufLeave or BufWinEnter handler raises can lose Neovim on Neovim 0.8.0. Neovim 0.12.3 (LuaJIT 2.1.1774638290) is not affected.Evidence
Controls on the minimal repro below (Neovim 0.8.0): JIT off survives, no coroutine survives, a callback that does not raise survives, no trace exit survives, one API call that returns between the raise and the trace exit survives. That last control is the workaround.
luajit1066b.luaselection_held_spec.lua, eager JIT, 3 runs eachdiff_failed_setup_guards_spec.luaabortsMinimal repro:
nvim --headless -u NONE -i NONE -c "luafile luajit1066b.lua"Fix
_setup_blocking_diffmakes onevim.api.nvim_get_current_tabpage()call before it rethrows "Diff setup failed". An API call that returns normally setscur_Lback to the running coroutine. One call before the rethrow was enough in every flow I measured. A call after the setuppcallalone was not (see the placement table below). I did not change any otherpcall.Counts
loop-spec.shstyle loop of the spec on mained1e0873, 6 parallel loops of 20no plenary summary, exit 0)diff.luaremoved, 4 parallel loops of 12tools.handle_invokefrom the main thread, no plenary, default JIT, 40 failed setups per run), 6 runsThe new case (
keeps Neovim running when handlers raise on repeated failed setups under an eager JIT) setsjit.opt.start("hotloop=1", "hotexit=1", "tryside=1"), runs the three raising-handler cases twice (6 passes) and restores LuaJIT's defaults. An eager JIT raises the abort rate from a few in 100 to most runs when the workaround is missing. It does not catch every missing workaround: 11 of 48 runs survived in the mutation measurement above.Placement of the call, measured on the original spec under an eager JIT (2 passes, 4 parallel loops of 8;
bad= no summary):pcallonlytabcloseand the lastset_current_tabpageGates
luacheck lua/ tests/ --no-unused-args --no-max-line-length: 0 warnings, 0 errors.stylua --check lua tests: clean.busted --shuffle --seed=7777, all*_spec.luaand*_test.lua): 1693 successes, 0 failures, 0 errors, 25 pending.scripts/run_integration_tests_individually.sh: Neovim 0.12.3 Total Success 417, Failed 0, Errors 0. Neovim 0.8.0 (the CI binary,v0.8.0-1210-gd367ed9b2) Total Success 309, Failed 0, Errors 0..agents/skills/verify-diff-lifecycle/check.sh: all facts TRUE and the model passes (main 89205 distinct states, live-DisconnectedCallsAnswered 5269). The change adds one API read before an error that already ends the setup, so no guard changes.covered.lockis re-recorded with--record, and--hash-onlymatches after mergingorigin/main(de309ae, with fix(terminal): keep the native start going when an autocmd closes its split #527). The numbers in this section are from head 3492410.Limits
openDiffthat ends in a rethrow from the coroutine. The accept and reject cleanup does not run in that coroutine:_resolve_diff_as_savedand_resolve_diff_as_rejectedrun from autocmds and commands on the main thread, andclose_tabruns underpcallintools.handle_invoke, also without a coroutine. The other code that runs in theopenDiffcoroutine is the replace cleanup, theClaudeCodeDiffOpenedevent and the float hide. I did not run those paths. An independent verifier ran them under an eager JIT on Neovim 0.8.0, and they survive on this head. Its replace scenario aborted 4 of 4 runs on main. The real fix is a LuaJIT from September 2023 or later.Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh