Repository navigation
test(selection): count only what is sent after the operator in two visual-edit tests - #553
Merged
Merged
Conversation
…o timing-exposed visual-edit tests
Both tests press ve, wait 20 ms, edit another line or :write, wait 20 ms, and press d. When anything delays that past the 100 ms debounce (a slow :write on a busy host, a stalled loop), the debounce sends the selection that you are still making, with the mode still visual. That send is right, and the plugin sends the cursor as soon as d runs. sent_no_selection counted every message, so the test failed with {false, 'alpha'} on CI. It now looks at the messages after the operator, and checks that a selection sent before it is the selection you made.
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. Keep it up! 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". |
… range of an early selection After an operator the plugin sends the cursor once its flush or the debounce has run, and get_latest_selection() is nil until then. A host that stalls the loop past the fixed 400 ms wait made the five operator tests fail with 'attempt to index a nil value' (the stall repro fails them all on main). They wait until the plugin holds a cursor now, then give the debounce and the demotion time to run. The selection that the debounce may send before the operator is checked for its range too, not only its text.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43a128382b
ℹ️ 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".
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…rator is a cursor When a slow write or plugin edit lets the debounce send the selection before the operator, the tests passed even if the plugin only forgot the selection in its own state and never told Claude. The five operator tests now also assert that the last message sent is a cursor.
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
🛡️ Codex 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. Delightful! 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 12, 2026
…' into fix/selection-same-typeahead-edit #553 fixes two timing bugs of the operator tests of this spec. Its helpers move to the outer scope of the spec, where the cases of this branch use them, and the new mapping case waits for the cursor and checks the selections sent before the operator, as #553 does. The spec has one more case: an autocmd that edits as visual mode starts.
This was referenced Nov 12, 2026
hareki
pushed a commit
to hareki/claudecode.nvim
that referenced
this pull request
Nov 13, 2026
…oder#540) ## What a user sees You select text in visual mode, a plugin or a mapping edits another line, and your Esc is read before the plugin has checked that edit. The selection is untouched and its text is still there, but Claude gets the cursor and not the selection. coder#524 fixed the case in which the check ran before the key. Three ways to read the key first also failed on `main`, and this PR fixes all of them: 1. A mapping that edits and presses Esc in one key sequence, for example `<Cmd>lua edit_other_line()<CR><Esc>`. 2. A key that arrives while a callback runs, as a UI sends it (`nvim_input`): during the plugin's own timer callback, or during another one that was queued before the check. 3. An autocmd that edits when visual mode starts (a `ModeChanged` handler for `n:v`) with the Esc already in the typeahead (`ve<Esc>`). The replays of `verify-selection-tracking` print `BUG` on `main` for each of them, 3 of 3 runs on Neovim 0.8.0 and 0.12.3, and `OK` with this change. ## Cause The flush on visual exit skips a selection that has an edit nobody checked yet (`entry.consumed or entry.unchecked_edits > 0`). An operator (`d`, `c`, `>`) edits the text and then ends visual mode, and its flush runs before any check, so that skip is what stops a phantom selection. A plugin's edit leaves visual mode on. The check that tells the two apart (is visual mode still on?) ran only in `vim.schedule`, and a schedule runs after the command or callback that made the edit returns, and after any callback that was queued before it. When a key is read first, the flush takes the plugin's edit for an operator's. ## The fix `lua/claudecode/selection.lua`: - `check_visual_edits(entry)` is the old scheduled check as its own function. It judges every edit that is waiting at once, by the mode that they left (visual mode on: a plugin's edit, benign. Off: an operator consumed the selection). It does nothing for a stale entry or when nothing waits, so running it twice is safe. - `note_visual_edit` still counts the edit and schedules the check, and it now also sets a `vim.on_key` listener (one namespace, made on first use). Neovim calls the listener for the next key before it processes that key, also for a key that comes from a mapping or that a UI sends, so the Esc finds visual mode still on. Whichever of the schedule and the key comes first runs the check, and the check clears the listener. - An operator's flush still comes before both, so `d`, `c`, `>` and the other operators skip as before. A listener of a flushed selection does nothing at the next key and clears itself, so it can live until that key (README says "at the latest until the next key"). I checked the key listener on Neovim 0.8.0, 0.9.5, 0.10.4, 0.11.4 and 0.12.3 (a probe script, not the plugin): it fires for the `<Esc>` of a mapping before it is processed with the mode still `v`, a second `vim.on_key(fn, ns)` replaces the first, and `vim.on_key(nil, ns)` is safe inside the callback and for an unregistered namespace. I did not try the range of the edit (`on_lines` receives the changed lines) as the test for whether it touched the selected lines. By my reasoning, which I did not test, a plugin edit inside the selection would then count as an operator's although the selection is still on the screen, and the marks and the edit range can be in different coordinates once lines move. ## Proof - `tests/integration/selection_visual_edit_spec.lua` has eight new real-Neovim cases: a mapping edits another line and presses Esc, appends lines and presses Esc, edits twice and presses Esc, writes the file and presses Esc, edits and then `d` (nothing is sent after it), an operator followed by the next selection in one typeahead, an autocmd edit at a mode change with `ve<Esc>`, and a queued check that runs after visual mode ended and before the flush (a user `ModeChanged` handler for `*:n` that calls `vim.wait(0)`, defined before the plugin starts, and a mapping that edits and presses Esc). On `main`'s `selection.lua` five of them fail on Neovim 0.8.0 and 0.12.3 (the first three, the autocmd case and the new one), and the rest are guards. The spec passes 20 of 20 on 0.8.0, 0.9.5, 0.10.4, 0.11.4 and 0.12.3. The case for a key that arrives during a callback is a replay only: `vim.wait` gives the callback's key a main-loop turn first, so the check cannot lose the race in that spec. - `tests/selection_test.lua` has eleven new cases (the mock gets `vim.on_key` and one `nvim_create_namespace` id for each name, a new one for the empty name, as Neovim does): the key comes before the schedule, a check that runs after visual mode ended and before the flush judges nothing again, several edits are judged together, the edit of a mapping is not the operator that comes next, a stale check does not clear the listener of the next selection, the schedule clears the listener, several edits keep one listener, a stale listener clears itself, an operator's listener waits for a key, and (the last two) a detached listener counts as an operator on the visual entry and the detach of another buffer does not. I changed `selection.lua` in 14 ways, one at a time, and counted the failures in `tests/selection_test.lua` (158 cases): no key listener (9), a listener that only clears (4), one decrement for each check (1), never consumed (6), always consumed (6), no schedule (6), no `clear_visual_edit_keys()` in the key listener (2), no `clear_visual_edit_keys()` in `check_visual_edits` (1), a `clear_visual_edit_keys` that does nothing, which removes both (4), a namespace for each edit (3), no stale-entry guard (1), no `unchecked_edits == 0` guard (1), and no `entry.consumed = true` in `on_detach` (1). 13 are killed. One survives and I think it is equivalent, but I did not prove it: the key listener checks `M.state.visual_entry` instead of the entry it was set for. The listener always belongs to the last noted entry, and a different current entry has no unchecked edit, so the zero guard returns. The zero guard itself is not equivalent. A `ModeChanged` handler that runs before the plugin's and calls `vim.wait(0)` runs the queued check after visual mode ended and before the flush. Without the guard that check judges the edit a second time, takes it for an operator's, and Claude gets the cursor. The new unit case and the new integration case kill it (the integration case on Neovim 0.8.0 and 0.12.3). - `verify-selection-tracking` (merged in coder#477): `check.sh` passes (129,986 distinct states) and so does `verify-diff-lifecycle` (89,205), with both locks re-recorded: `selection.lua` changed, the selection skill has a new covered block (`check_visual_edits`), and the diff model reads no block or site of that file. All 16 replays print `OK` on Neovim 0.8.0 and 0.12.3 at the default delay and with `DELAY_MS=50`. The three that replay these bugs print `BUG` on `main`. - The skill is updated for this fix: `VisualCheckSparesPluginEdit` now reads `check_visual_edits` and both triggers in `note_visual_edit`, so removing the schedule, the key listener, the mode test or the `consumed` mark reads `FALSE` and nothing else (checked with five mutants of `selection.lua`; the model then breaks `ReleaseSent`). `ListensToSelectionBuffer` reads `check_soon()` in the body of `on_lines` (in the branch that holds the selection) and in the body of `on_reload`, each body on its own. So moving the call into the `elseif not entry` branch, removing it from `on_lines` or `on_reload`, or moving it from `on_reload` into `on_detach` reads `FALSE` and nothing else (five mutants). All 27 facts read `TRUE` on this tree. On the code before coder#524 exactly four facts read `FALSE` (checked at the previous push), and the old form of `on_lines` still reads `TRUE` for `ListensToSelectionBuffer` after this change. The replay header prints the Neovim version as numbers (it printed a table address on 0.8). - Gates on the final head: the full unit suite (`busted --shuffle`, seed 1794484841) 1748 successes, 0 failures, 28 pending. The integration runner (CI style, a private `HOME`, normal and `odd tmp 50%` `TMPDIR`): 535 tests in both runs on 0.12.3 and 398 in both runs on v0.8.0, 0 failures, and the real cache stays untouched. `scripts/tla/test-check-lib.sh` 44 of 44. The spec passes 20 of 20 on all five Neovim versions, `selection_test.lua` 158 of 158, `check.sh` of both skills passes, `luacheck` 0 warnings in 185 files, `treefmt --ci` 0 changes in 427 files, and `--hash-only` matches for all four `verify-*` skills (no re-record was needed after the merge of `main`). - `selection_held_spec.lua` is flaky under load on `main` (here 7 of 72 runs, on `origin/main` 14 of 72). This PR does not touch it. coder#559 fixes it with condition waits and shows the cause with traces. ## Notes for the reviewer - coder#553 is merged. This branch merges `main` and keeps the helpers of that PR at the outer scope of the spec, because the mapping cases use them. coder#559 also edits `selection_visual_edit_spec.lua` (`wait_for_cursor` waits 400 ms, and one new case at the end of the operator block), so whichever of the two lands second needs a small merge in that file. - Neovim 0.8 and 0.9 still need the scheduled check for a plugin edit whose callback ends before any key (their `on_lines` runs with visual mode off during the API call). The key listener adds the keys that come first. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
hareki
pushed a commit
to hareki/claudecode.nvim
that referenced
this pull request
Nov 13, 2026
… other path characters (coder#561) ## Why `cwd.git_root` must find the repository of a folder whatever its name is. A repository in a folder with a space or a comma in its path was not found when git could not run: the fallback gave nil, or the repository of Neovim's current folder. A folder named `[x] {y}` or `$HOME-like` was lost or changed even when git answered. A verifier of F12 found the first case (finding F12-P1, a space or comma in the path makes the `.git` fallback return nil). ## What changed Four problems in `lua/claudecode/cwd.lua`. The first two took a path as a pattern. The third stepped one folder down at a trailing `..`. The fourth is a flaw in my first version of the second step. 1. **The `.git` fallback** called `vim.fn.finddir(".git", start_dir .. ";")`. `finddir` reads its path as a list in `'path'` option syntax. A space or a comma split the path, and a `;` ended it. The fallback now walks up from the full path of `start_dir` and tests each folder for a `.git` folder with `vim.fn.isdirectory`. A relative `start_dir` now reaches the folders above the current one too. Before, a relative start gave a wrong answer or nil in some cases (for example `git_root("sub/deeper")` in the repository root). 2. **`normalize_dir`** passed every path through `vim.fn.expand`. `expand` reads `[`, `{`, `*`, `?`, `$` and `\` in a name as patterns or variables. A folder named `[x] {y}` or `it's` came back with a trailing slash. A folder named `$HOME-like` or `back\slash` came back nil. This also happened when `git rev-parse` answered. `normalize_dir` now returns a directory that exists as written, and expands any other string (`~`, `$VAR`). See step 4 for text that starts with `~` or `$`. 3. **`..` before Neovim 0.11.** Before 0.11, `fnamemodify(path, ":p")` keeps a last `..`, and `:h` of `a/b/..` is `a/b`. So a walk from a start folder that ends in `..` stepped one folder down and returned the wrong root. The walk now calls `simplify()` on the full path first. A verifier found this. 4. **Text that starts with `~` or `$`.** The first version of step 2 took a path that names an existing folder as written, on the raw text. `vim.fn.isdirectory` does not expand, so with `cwd = "~"` or `"$HOME"` and a stray folder of that literal name in Neovim's current folder (for example one that an unexpanded `mkdir "~/x"` made), `git_root` and `resolve` returned the stray folder where main returned the home folder. `normalize_dir` now sends text that starts with `~` or `$` through `expand` first, as a shell and `:cd` do. A verifier of the terminal change found this. `lua/claudecode/README.md` gets one new bullet that states this, with the rule for text that starts with `~` or `$`. It sits after the "Terminal cwd precedence" bullet. It names `git_root` and `resolve` only, and it says that the terminal still passes a static `cwd` and a `cwd_provider` result through `vim.fn.expand` itself. ## Scope - Only `cwd.lua` changes in code. Steps 1 and 2 are one change of idea: a path is a literal, not a pattern. Step 3 is a quirk of `fnamemodify` before Neovim 0.11. Step 4 closes a hole that step 2 opened. - `normalize_dir` serves `cwd.git_root` and `cwd.resolve`. Nothing in `lua/` calls `cwd.resolve`. The terminal calls `cwd.git_root` when `git_repo_cwd` is set, so this PR fixes folder names with `$` and `\` on that path: when git does not answer, the terminal now gets `$HOME-like`, `back\slash`, `[x] {y}`, a space and a comma right, where main gave nil or the repository of Neovim's current folder. A static `cwd` and a `cwd_provider` result do not go through `normalize_dir`: `terminal.lua:build_config` passes them through `vim.fn.expand` itself, so this PR does not change what the terminal does with those. An absolute start path that ends in `..` is fixed here, through the `simplify()` call above. - Two gaps stay for separate follow-ups. Main already has both, and this PR does not reach them. First, the fallback finds a `.git` folder only, not a `.git` file in a worktree or submodule. Second, the walk is lexical, so a link into another repository answers with the repository of the link's own folder, where git answers with the link target's repository. The terminal's `expand` of a static `cwd` and of a provider result is a third follow-up, and it has its own change. - `~` and `$VAR` still expand in `cwd.resolve` (the spec checks both). The spec's cases for `cwd.resolve` are named as `cwd.resolve` cases, because they do not test the terminal. - `git rev-parse` still runs first. Its result for a plain path is the same string as before. ## Blast Radius - User-visible: a repository in a folder with a space, a comma, a quote or another path character is now found when git cannot run. With git, `git_root` returns a folder with `[`, `{`, `*`, `?`, `$` or `\` in its name exactly. It does not reach the terminal through a `cwd_provider`: `build_config` still expands the provider's result, so the root README's example still loses `$` and `\` there (follow-up). A plain path gives the same result as before. - The fallback result is an absolute path with no `..` part. Before it could be relative. The one exception is a relative start whose name begins with `~`: Vim reads `~` as the home folder and `~name` as a user's home folder, so `:p` leaves `~name` relative and the result can be the relative `~name`. That is a pathological case, and no caller in `lua/` passes a relative path (the terminal passes the file's folder from `expand("%:p")` or `getcwd()`). - A string that names an existing folder is taken as written, except text that starts with `~` or `$`. That text always goes through `expand` first, so a folder named `~` or `$HOME` in Neovim's current folder does not take the place of the home folder, as on main. The cost: a relative folder name that starts with `$` and reads as a variable (`$HOME-like`, `$odd/sub`) is expanded and not found, as on main. A lone relative `$odd` that names no variable is left unchanged by Vim and is still found. An absolute path never starts with `~` or `$`, so every folder name this PR fixes keeps working. The change applies to `git_root`'s `start_dir` and to `cwd.resolve`. The terminal's own `expand` of a static `cwd` and a provider result is a separate change. - Neovim 0.8: the code uses `vim.fn.isdirectory`, `vim.fn.fnamemodify` and `vim.fn.simplify`, which exist there. I ran the new spec on Neovim 0.8.0, 0.10.0 and 0.12.3. The `..` case fails on 0.8.0 and 0.10.0 without the `simplify()` call, and passes on 0.12.3 either way. - `covered.lock`: `cwd.lua` is the terminal's working-directory lookup. `check.sh` printed STALE for the file hash of `cwd.lua` only, with no changed block or call site. The change cannot reach diff state, an openDiff answer, a disconnect or a diff command. The model passed (`main`: 89205 distinct states, `live-DisconnectedCallsAnswered`: 5269). `check.sh --record` wrote the new lock and `check.sh --hash-only` reports that it matches. ### Merge with coder#539 (F12) coder#539 landed first. This PR's merge commit with `origin/main` resolves the interplay: it re-records `covered.lock` and changes the one stale test in coder#539's `cwd_spec.lua` (see Verification). F11 (coder#535) merges with this PR without a conflict. ## Verification Red before the fix, green after (same spec, only `lua/claudecode/cwd.lua` swapped): | Spec | origin/main | with the fix | | --- | --- | --- | | `tests/integration/cwd_path_characters_spec.lua` (37 cases, real Neovim, 0.8.0 and 0.12.3) | 20 successes, 17 failures | 37 successes, 0 failures | | the new case "goes up from a start folder that ends in .." at `ede2f733` (before the `simplify()` call) | fails on 0.8.0 and 0.10.0, passes on 0.12.3 | passes on 0.8.0, 0.10.0 and 0.12.3 (35 successes when added, 37 at the final head, 0 failures on each) | | the two cases "expands a leading ~ / $ even when Neovim's cwd holds a folder named ~ / $HOME", with `cwd.lua` from `ca9e61c0` (before the guard) | 35 successes, 2 failures on 0.8.0 and 0.12.3 | 37 successes, 0 failures on each | The spec makes real folders and runs `cwd.git_root` on them, in two modes. In the first mode git is off `PATH` (the shell keeps its absolute path), so `git` ends with "not found" and a list command raises E475, as on a host without git. In the second mode a real `git init` makes the repository. The folder names are: a space, a comma, a semicolon, single and double quotes, brackets and braces, parentheses, non-ASCII letters, a star and a question mark, a percent sign and a hash, a backslash, a dollar sign, a leading tilde, and a plain name. Each name runs from the repository folder and from a folder two levels below it, and the answer must equal the repository path with no trailing slash. A folder that is not a repository returns nil, whatever its name. Seven more cases cover a relative start folder, a start folder that ends in `..`, `cwd.resolve` with `git_repo_cwd`, a static `cwd` with brackets and a star in `cwd.resolve`, `~` and `$VAR` in a setting of `cwd.resolve`, and two cases where Neovim's current folder holds a folder named `~` or `$HOME` and the setting must still expand. The 17 red cases are 10 of the no-git names, 4 of the git names, the relative start, `resolve`, and the static `cwd`. Gates on the final head `f12368bd`, which contains a merge of `origin/main` (05b9785, with coder#551, coder#535, coder#494, coder#545, coder#553, coder#555 and coder#557) (private XDG folders). An independent verifier passed `c73ab044`. Since then the branch has three changes of its own and two merges with main. The changes are a docs commit (the README sentence about where the walk starts), the fix for text that starts with `~` or `$` (step 4, with two spec cases and the README clause), and the `covered.lock` record for it. For the lock I ran the full `check.sh`: the model passed (89205 distinct states, 5269 live states, every fact TRUE), and only the file hash of `cwd.lua` was stale, with no changed block or call site. - `mise run all`: native exit 0. luacheck: 0 warnings / 0 errors in 186 files. busted: 1756 successes / 0 failures / 0 errors / 29 pending. treefmt: 0 files changed. Since coder#545, `mise run all` also runs the static proofs of the `verify-claudecode-nvim` skill, and they pass. - `mise run format-check`: native exit 0 (428 files, 0 changed). - `scripts/run_integration_tests_individually.sh` on Neovim 0.12.3: 564 successes, 0 failed, 0 errors, native exit 0. On Neovim 0.8.0: 427 successes, 0 failed, 0 errors, native exit 0 (the 0.8.0 run skips the specs that need 0.9). The new spec gives 37 successes on both. It starts about 100 processes (git and a shell), so it takes about 6 s on a quiet host and can pass the runner's 40 s limit at a load average above 100. A TIMEOUT line there means the host was slow, not that the spec hung. - All four `.agents/skills/verify-*/check.sh --hash-only` match their `covered.lock`. The merge with `origin/main` had two parts to resolve in an earlier commit. `covered.lock` conflicted on the hash line of `cwd.lua`, so I ran the full `check.sh` and `check.sh --record`. coder#539's `tests/unit/cwd_spec.lua` had a test that stubbed `vim.fn.finddir`, which the fallback no longer calls, so that test now walks up real folders. It does not depend on where `TMPDIR` is: it passes with `TMPDIR` inside a git checkout too. `cwd.lua` itself merged without a text conflict. The two latest merges needed no `covered.lock` change: coder#551 conflicted only in `tests/mocks/vim.lua`, where the branch keeps both its `simplify` entry and coder#551's `wordcount`, and coder#535 and coder#494 merged without a conflict. An independent verifier found the `..` case, the README wording and the other points that this body now states. The `terminal.lua` expand and `.git` files in worktrees are gaps that main already has. They are follow-ups, not part of this PR. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
hareki
pushed a commit
to hareki/claudecode.nvim
that referenced
this pull request
Nov 13, 2026
…coder#559) ## Result `selection_held_spec.lua` failed in 24 of 144 runs under load. With this change it failed in 0 of 144 runs, on the same tree, in the same load windows. The load was 16 spec runs at the same time, 8 disk writers (`dd conv=fsync` loops), Neovim 0.12.3, three rounds that alternate the old and the new spec (round 1: 18 of 48 against 0 of 48, round 2: 1 against 0, round 3: 5 against 0). Without load the spec takes 9 s instead of 23 s, because it no longer sleeps. This PR is only test code (two spec files). It does not change the plugin. ## Cause Every wait in the spec was a fixed `vim.wait(N)`. A wait ends at its deadline. When the main loop stalls for 230 to 390 ms (a loaded disk or CPU), the wait can end in the loop iteration that fires the timers. The `vim.schedule_wrap` callbacks of those timers have not run yet. And a late debounce arms the demotion timer too late for the deadline. The next step then reads a plugin that is not done. I traced this with a copy of the spec that logs the plugin's calls, adds a 5 ms heartbeat timer (it reports every stall of the loop), and dumps the log when a test fails. Each failure below comes from a wait that ended too early. The last row also exposes a plugin gap (see "Found while tracing"). The traces are in the details below. | Old test | What failed | What the trace shows | |---|---|---| | keeps one listener on the file through reloads (after `enew`) | `selection.isEmpty` was `false` | the loop stalled 334 ms, the wait ended in the iteration that fired the debounce, and `update_selection` sent the empty cursor 0 ms after the assertion | | ends the selection without an error when the buffer you visited is wiped (both kinds) | `selection.isEmpty` was `false` | a 270 ms or 389 ms stall, the debounce ran late and armed the 50 ms demotion, and the wait ended before it fired | | does not overflow the stack when the buffer of the held selection is taken for the Claude terminal | a `selection_changed` for `term://x//1:claude` was sent | the demotion timer fired in the stall, and its callback ran after the test had renamed the buffer and edited it (I reproduced this with an injected stall) | | keeps the selection when the Claude terminal first opens (delay 50) | held text was `""`, not `"alpha"` | the stall ended the wait with the demotion pending, the terminal opened (2.7 s on the loaded disk), and the demotion fired while the terminal was the current buffer | The old spec failed these in 24 of 144 runs: 18 times the overflow test, 7 times the first-opens test, 2 times a wiped-buffer test (27 failed tests in 24 runs). The listener test failed in other runs: 12 of 60, 5 of 60 and 15 of 48 traced runs, and 2 of 36 untraced runs. <details> <summary>Traces (times in ms, from a traced copy of the old spec)</summary> Listener test, a stall straddles the end of `wait(300)`: ``` 28502 on_cursor_moved(buf=24) :enew arms the 100 ms debounce 28853 STALL: loop did not run for 334 ms (28519..28853) 28853 shadow fast timer (100 ms from 28502) fired 28853 wait(300) took 349 ms 28853 cycle 1: isEmpty after wait(300) = false <- the assertion point 28853 shadow scheduled timer (100 ms from 28502) ran 28853 update_selection(buf=24) 28853 send_selection_update text= file= <- the cursor of the new buffer 28853 isEmpty became true 0 ms after the assertion point ``` Wiped-buffer test (`nvim_buf_delete`), the debounce runs late and its demotion misses the deadline: ``` 32138 on_cursor_moved(buf=35) the file buffer is current again, debounce armed 32488 STALL: loop did not run for 270 ms (32218..32488) 32493 update_selection(buf=35) arms the 50 ms demotion 32542 wait(400) took 404 ms the demotion would fire at about 32543 ``` Overflow test. I reproduced it by putting a 220 ms sleep between two loop iterations of the wait after Esc (the natural failure has the same assertion message: `selection_changed`, `filePath = 'term://x//1:claude'`, `text = ''`): ``` 12210 update_selection the debounce read the release, arms the demotion 12226 STALL injected between two loop iterations: the process sleeps 220 ms 12452 STALL: loop did not run for 226 ms (12226..12452) 12452 wait(300) took 347 ms 12452 handle_selection_demotion(buf=16) runs in the NEXT wait, after rename + edit 12452 send_selection_update text= file=term://x//1:claude stack: selection.lua:1170 send_selection_update < :711 handle_selection_demotion < :622 timer callback ``` First-opens test (delay 50): ``` 296 update_selection arms the 50 ms demotion 547 STALL: loop did not run for 231 ms (316..547) 547 wait(300) took 351 ms the wait ends with the demotion pending 748 on_cursor_moved(demotion_timer=true, buf=2) the terminal opens 3306 STALL: loop did not run for 2759 ms (547..3306) 3306 handle_selection_demotion(buf=2) fires while the terminal is the current buffer 3721 send_selection_update(buf=1) text= after you return: the cursor replaces the selection ``` </details> ## What changed `tests/integration/selection_held_spec.lua`: - `settle(keep_demotion)` waits until the plugin has no debounce timer and no demotion timer pending, and until every `vim.schedule` callback queued before the call has run (it queues a flag and waits for the flag, and the queue is first in, first out). Every `wait(N)` that waited for the plugin is now `settle()`. - `select_and_release(keep_demotion)` settles after `ve` (the debounce reads the selection) and after Esc (the debounce reads the release, and the demotion runs). The two specs that need a long demotion still pending (`visual_demotion_delay_ms` of 1000) pass `keep_demotion`, and the helper asserts that the demotion is pending, so a stall cannot silently end that premise. - The one `wait(20)` stays, with its comment: the popup is closed inside the debounce on purpose. `tests/integration/selection_visual_edit_spec.lua` (two follow-ups to coder#553, F1 and F2): - F1: `wait_for_cursor` waits 400 ms after the cursor (it was 200) and then drains the queued callbacks, so a phantom selection sent up to 400 ms after the operator shows again. - F2: a new case, "sends a cursor for d after the debounce sent the selection in visual mode". It waits until the debounce has sent the selection, before `d`. The two cases above it press `d` within the debounce, so their early-selection checks usually have nothing to look at. This one checks the text and the range of the early selection and that the last message is a cursor, on every run. ## The tests still test the same things One thing was lost in the first version and is back in round 2. The three tests that expect nothing to be sent (overflow, pending demotion, sends nothing in another terminal) lost the late-send window of their old fixed waits, because `settle()` returns as soon as no timer is pending. The verifier's mutant sends the cursor of the terminal buffer 200 ms late with `vim.defer_fn`. It failed 3 tests of the old spec and none of the first version of the new spec. Each of the three tests now calls `wait_for_late_send()`: it waits 300 ms after `settle()` and then settles again, because a timer that fires in the last iteration of that wait has not run its callback yet (Codex found this in round 2). The mutant fails the same 3 tests again, and the unchanged plugin passes all 14. I changed `selection.lua` ten ways (one at a time, in a scratch copy) and ran the old and the new spec. None of these ten is a late send. The set of failing tests is the same for nine of them, and the new spec also kills one that the old spec did not: | Mutant of `selection.lua` | Old spec fails | New spec fails | |---|---|---| | overflow: the terminal branch calls `update_selection` | 3 | 3 (same tests) | | no demotion cancel in the terminal branch | 1 | 1 | | `from_terminal` ignores the terminal name | 1 | 1 | | no `check_soon` in `on_reload` | 1 | 1 | | no `check_soon` in `on_lines` | 6 | 6 | | the listener forgets its buffer on reload | 1 | 1 | | the demotion ignores an unmoved cursor | 13 | 13 | | no `caf.left` mark | 2 | 2 | | the terminal update keeps the pending demotion | 0 | 1 (first opens, delay 1000) | | no re-watch in `on_detach` | 0 | 0 (survives both, as before) | For the coder#553 follow-ups, on coder#553's tree: removing the flush guard fails 5 tests of the old spec and 6 of the new one. A range off by one at the selection end (the text stays right) survives the old spec and fails the new F2 case. A phantom selection sent 350 ms after the operator fails 4 of the 12 tests of coder#553's spec with `wait(200)` in every run of the independent verifier (I saw 0 failures in one run, so the old margin depends on timing), and 6 tests with `wait(400)`. ## Found while tracing: a plugin bug that this PR does not fix At the default `visual_demotion_delay_ms` of 50, Claude loses a held selection when you enter the Claude terminal 100 to 150 ms after Esc and then come back. The demotion timer fires while the terminal is the current buffer, and Condition 1 of `handle_selection_demotion` sets `last_active_visual_selection = nil`. When you return, `update_selection` finds no recent visual exit and sends the cursor. A probe with no stall shows it: release `alpha`, wait until the debounce arms the demotion, open the terminal at once, wait for the demotion, `wincmd p`. At delay 50 the held text after the return is `""`. At delay 200 it is `"alpha"` (the terminal's own debounce cancels the demotion). Removing the `last_active_visual_selection = nil` block in Condition 1 keeps `"alpha"` at both delays, and this spec stays 14 of 14. That change touches a file that `verify-selection-tracking` covers, so it is its own PR: coder#562. The old test only failed because a stall moved the demotion into that window. ## Gates - `selection_held_spec.lua` 14 of 14 and `selection_visual_edit_spec.lua` 13 of 13 on Neovim 0.8.0, 0.9.5, 0.10.4, 0.11.4 and 0.12.3 before the coder#540 merge, and 21 of 21 on 0.8.0 and 0.12.3 after it (a verifier rerun). Under the load of the first table (16 runs at once, 8 disk writers) the spec with the added waits failed 0 of 48 runs. - The integration runner as CI runs it (a private `HOME`, normal and `odd tmp 50%` `TMPDIR`): 0.12.3 528 of 528 in both runs, v0.8.0 391 of 391 in both runs, 0 failures. - coder#553's spec under the same load, with F1 and F2: 0 of 96 runs failed, and 0 of 96 for coder#553's spec alone. - `luacheck` 0 warnings, `treefmt --ci` 0 changes. No unit test changes, so I did not run the unit suite. - I did not run the load comparison on Neovim 0.8.0. The comparison above is 0.12.3 only. ## Notes for the reviewer - coder#540 is merged. This branch merged `main` without a conflict, and F1 and F2 sit on the helpers that coder#540 moved to the outer scope of `selection_visual_edit_spec.lua`. --- _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 failed
CI run 43167645889 (job
integration-tests (stable), PR #523, head3ceb6ecc) failedtests/integration/selection_visual_edit_spec.lua:230, "sends no selection for d after a :write in visual mode", with{false, 'alpha'}.Cause: a timing bug in the test, not in the plugin
The test presses
ve, waits 20 ms, runs:write, waits 20 ms and pressesd. It then asserts that Claude was never sent a non-empty selection.That holds only while
dcomes within the 100 ms debounce ofve. When anything delays the keys past it (a slow:writeon a busy host, a stalled loop), the debounce sends the selection that you are still making, with the mode still visual. That send is right: you did selectalpha. The plugin then sends the cursor (an empty selection) the momentdruns.sent_no_selectioncounted every message, so it also counted the right one.I traced it with time stamps, on Neovim 0.12.3 and 0.8.0. In every failing run the sequence is the same:
A phantom after
dwould carrybeta(the text at the old marks) and a mode other thanv. None of the 30 traces has one. A deterministic repro makes:writeblock for 150 ms with aBufWritePostautocmd: the same sequence, the same failure, and the plugin ends with the cursor held andbeta gammaon line 1.The sibling test "sends no selection for d after a plugin edited another line in visual mode" has the same shape and failed too (
wait(20)stalled). Theved,vecandVj>tests press all their keys in onepress, so no timer can fire between them.The fix
Round 1 (
43a12838):sent_no_selection(from)looks only at the messages after numberfrom. The two tests record#sentbefore they pressd, and assert that a selection sent before the operator is the one the user made. With the flush made to never skip (if false and entry ...), the five operator tests fail on both versions, these two included.Round 2 (the Opus verifier's two findings):
main). After an operator the plugin sends the cursor when its flush or the debounce has run, andget_latest_selection()isniluntil then. A host that stalls the loop past the fixedwait(400)made the test fail withattempt to index a nil value. The five operator tests (ved,vec,Vj>, and the plugin-edit and:writetests) now callwait_for_cursor():vim.waitwith a condition (the plugin holds an empty selection) and a 5 s limit, thenwait(200)so that a selection sent late by the debounce would still show. The release tests keepwait(400): the flush on Esc sends the selection beforepressreturns, soget_latest_selection()is nevernilthere.assert_is_the_selectionchecks the text and the range (0:0to0:5) of every selection sent before the operator.Round 3 (Codex P2): when the debounce sends
alphabefored, the tests passed even if the plugin only forgot the selection locally and never sent the cursor. Each of the five operator tests now also asserts that the lastselection_changedsent is a cursor. A mutant whose flush setslatest_selectionto the cursor without sending it passes the round-2 spec 12 of 12, and fails this spec's plugin-edit and:writetests under the 150 ms stall on both versions (a verifier reran it: 2 of 12 fail, those two). Without a stall,tests/selection_test.luaalready catches that mutant.Proof
Deterministic stalls, on Neovim 0.12.3 and 0.8.0 (generated copies of the spec, not part of the PR):
main)d(:writeand plugin-edit tests){false,'alpha'})attempt to index a nil value)Wrong range, right text (the in-visual selection sent with its end line moved by 5), with the 150 ms stall: the round-1 spec passes 12 of 12 on both versions, and this spec fails the plugin-edit and
:writetests (both on 0.8.0 and in 3 of 4 runs on 0.12.3, one run failed only the:writetest).Same-window load (24 runners in parallel and 8
dd conv=fsyncloops, or 40 runners with 12 disk loops and 16 CPU burners):{false,'alpha'})attempt to index a nil value:Vj>15,vec7,:write1,ved1)A window with lower load had no failure for either spec (0 of 72 each), so the deterministic stalls above are the proof and the load windows the check.
Other gates: the spec passes 12 of 12 on both versions on an idle host. The CI-style integration runner (private
HOME, normal andodd tmp 50%TMPDIR) passes with 417 tests per run on 0.12.3 and 309 per run on v0.8.0, and leaves the real cache untouched.luacheckshows 0 warnings in 181 files andtreefmt --ci0 changes.Notes for the reviewer
#540 moves
sent_no_selectionto the outer scope of this spec and adds cases that use it. It resolves the conflict when it mergesmain.Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh