Repository navigation
fix(selection): keep the selection when the demotion fires in a Claude terminal - #562
Merged
Merged
Conversation
…nal keeps the selection A new terminal is not registered when its BufEnter fires, so nothing cancels the demotion. With the default 50 ms it fires while the terminal is the current buffer, and Condition 1 of handle_selection_demotion cleared last_active_visual_selection. When you came back, update_selection sent the cursor. These cases fail on main: an integration spec for a registered terminal (delay 50) and for a terminal that only its name says is Claude's, and two unit tests.
…e terminal Condition 1 of handle_selection_demotion cleared last_active_visual_selection when the demotion fired while the Claude terminal was the current buffer. A new terminal is not registered at its BufEnter, so nothing cancels the demotion, and with the default 50 ms it fires first. When you came back, update_selection found no recent visual exit and sent the cursor, so Claude lost the selection although the cursor had not moved. The demotion now returns at once in any buffer that is_claude_terminal_buffer accepts (the registered terminal or one that only its name marks) and leaves the records alone, so the return keeps the selection as it does when you never left. The model drops holdLoose, which allowed either outcome there, and gets a fact, DemotionIgnoresTerminal, that reads the new Condition 1 and breaks HeldUntilMoved when the clear comes back. Both locks are re-recorded.
… still ends the hold
…bounce can cancel it
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 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". |
🛡️ 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. |
…n-in-terminal # Conflicts: # .agents/skills/verify-diff-lifecycle/covered.lock # .agents/skills/verify-selection-tracking/covered.lock
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
|
Codex Review: Didn't find any major issues. 🚀 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. |
4 tasks
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 a user sees
You select text and press Esc. Within the next 100 to 150 ms you enter the Claude terminal (a toggle mapping, or
:ClaudeCodethat opens it). You come back to the file and the cursor has not moved. Claude holds the cursor and not your selection. A slower switch, or a terminal that Neovim already knows, keeps the selection. With this change Claude keeps it in both cases.I found this while tracing the
selection_held_specflake (#559): a stall of the main loop moved the demotion into that window, and the plugin did what a person can also trigger.Cause
At the default
visual_demotion_delay_msof 50, the debounce (100 ms) reads the release and arms the demotion timer, and the timer fires 50 ms later. When you enter a terminal that the plugin already knows,reconcile_held_selectioncancels the demotion atBufEnter. A new terminal has no name and is not registered atBufEnter, so nothing cancels it, and the demotion fires while the terminal is the current buffer, before the debounce of thatBufEntercan cancel it.Condition 1 of
handle_selection_demotionhandled that case. It returned, but first it setlast_active_visual_selection = nil. When you came back,update_selectionfound no recent visual exit, took the "genuinely in normal mode" branch and sent the cursor.Why Condition 1 cleared it:
git log -Lon the block shows one commit, f1cad48 (May 2025, the commit that addedvisual_demotion_delay_ms). Its comment says the selection is preserved and the "pending" status oflast_active_visual_selectionis resolved. That was written before #246. At that commit Condition 3 demoted without the cursor-unmoved test, and a return to the file reported the cursor. #246 (#267,0b245051) added the test and made Condition 3 leave the record intact, so that a later cursor move re-arms the demotion. Condition 1 was the one branch that still cleared it. ARCHITECTURE.md already said the timer "does nothing in the Claude terminal", and the model had a step for Condition 1 and aholdLooseflag that allowed either outcome on the return (SKILL.md lists it as a case where the model allows both).The fix
lua/claudecode/selection.lua: Condition 1 is nowif M.is_claude_terminal_buffer(current_buf) then return end. The demotion does nothing in a Claude terminal and leaves the records alone, so the return keeps the selection while the cursor stands where you released it, as it does when you never left.It uses
is_claude_terminal_buffer, the check thatupdate_selectionand the flush already use. Before, Condition 1 asked only for the registered terminal. For a terminal that only its name marks (terminal.provider = "none"with a:terminal ... claudebuffer), the demotion fell through to the last block of the function, which cleared the same records, so that user lost the selection the same way. SKILL.md described that as "clears the held-state records and sends nothing". The narrow change (only the registered terminal) leaves that case broken, and the new name-only cases fail with it.Each case that Condition 1 served still behaves
last_broadcast == nil, the integration spec assertsalphain the terminalsettle()after the returnCursorMovedmainearlier, at the assertion that the selection was kept)BufEnter: the demotion is cancelled theredemote_if_selection_text_changedpassProof, test first
tests/integration/selection_demotion_in_terminal_spec.lua(new, 3 cases): releasealpha, arm the demotion, open a terminal, let the demotion fire there, come back, and check that Claude holdsalpha. It fails onmain'sselection.luafor delay 50 with a registered terminal and for the name-only terminal (2 of 3, on Neovim 0.8.0 and 0.12.3). Delay 200 passes on both, because the debounce cancels the demotion. With the fix it passes 3 of 3 on 0.8.0, 0.9.5, 0.10.4, 0.11.4 and 0.12.3. It arms the demotion by calling what the debounce timer calls (update_selection), so that the demotion is pending when the terminal opens and a stall cannot end it first.tests/selection_test.lua(3 new cases, 161 in the file on the final head): the new terminal, the name-only terminal, and the cursor move after the return. All 3 fail onmain'sselection.lua. With only the registered terminal in Condition 1 (the narrow change), the name-only case fails and nothing else does.holdLooseis gone fromSelectionTracking.tla, soHeldUntilMovednow applies to the return from the terminal. A new fact,DemotionIgnoresTerminal, reads the whole body of Condition 1 (theifand onereturn, nothing else), andDemoteusesclearViswhen it isFALSE. On this treecheck.shpasses (95,938 distinct states, 129,986 before: the either-outcome branch is gone). With the old clear put back,DemotionIgnoresTerminalreadsFALSEandcheck.shfails withmain: BROKEN: HeldUntilMoved.check.sh --mutantson the merged tree (head7ba852fa5e): exit 0 in 3,774 s, 27 rows (26 before, one for the new fact), "every guard is caught". The new fact's row readsDemotionIgnoresTerminal=FALSE caught by: HeldUntilMoved HeldInTerminal.verify-selection-tracking, andverify-diff-lifecyclefor the file hash ofselection.lua: the diff model reads no block or site of it).--hash-onlymatches for all fourverify-*skills.Gates
busted --shuffle, seed 1794487328, before the last test was added): 1739 successes, 0 failures, 29 pending.selection_test.luaalone 161 of 161 on the final head. A verifier reran the full suite on the head (same seed): 1770 successes, 0 failures, 29 pending.HOME, normal andodd tmp 50%TMPDIR): 530 tests in both runs on 0.12.3 and 393 in both runs on v0.8.0, 0 failures.selection_held_spec.lua14 of 14 and the new spec 3 of 3 on all five Neovim versions, on the final head.luacheck0 warnings in 186 files,treefmt --ci0 changes,shfmtandshellcheckclean oncheck.sh.Notes for the reviewer
mainafter it. Only the twocovered.lockfiles conflicted, and I re-recorded both (--hash-onlymatches for all fourverify-*skills). The skill checks pass on the merged tree:check.sh95,938 states for the selection skill, 89,205 (and 5,269 live) for the diff skill, 16 of 16 replays on Neovim 0.8.0 and 0.12.3,selection_test.lua161 of 161.selection_held_spec.lua. This PR does not touch that file, so the two do not conflict.update_selectionreturns in the terminal and only checks the held text, as before.Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh