Repository navigation
fix(terminal): own the native terminal buffer and send one notice - #339
Merged
Merged
Conversation
Make the native spec's mock behave like Neovim: enew and nvim_win_set_buf fire a BufEnter hook, termopen turns the current buffer into the terminal, and nvim_buf_call rewraps a raised error. Add cases for one notice per failed start from open, simple_toggle, and focus_toggle, for an autocmd that swaps another plugin's empty buffer into the split, and for the listed, bufhidden=hide terminal buffer on success. The external jobstart failure test now asserts _get_terminal_for_test() is nil instead of get_active_bufnr(), which external always returns nil. These cases fail on this commit. The next commit fixes native.lua. Part of #333. _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
open_terminal ran enew in the new split and then treated whatever buffer the split showed as its own. A BufEnter autocmd that swaps another plugin's buffer into the split broke that: a failed start left the enew buffer listed and deleted the other plugin's buffer when it had no name and no changes, and a successful start turned that buffer into the terminal. The provider now creates its buffer with nvim_create_buf, shows it with nvim_win_set_buf, runs termopen in it through nvim_buf_call, and shows it in the split again if an autocmd swapped it out. A failed start deletes only that buffer. A normal start behaves as before: a listed terminal buffer with bufhidden=hide in the split, and the same on_exit handling. A failed start now sends one ERROR notice, from open_terminal, with the termopen error text. M.open, M.simple_toggle, and M.focus_toggle no longer add "Failed to open Claude terminal using native fallback.". Part of #333. _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
Codex Review SummaryThis 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. You're on a roll. 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. |
…termopen The nvim_buf_call mock now raises "Invalid buffer id" for a deleted buffer, as Neovim does. The new case swaps another buffer into the split and wipes the provider's buffer, then expects one failure notice, the split closed, and the other buffer kept. Part of #333. _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
A BufEnter or BufWinEnter autocmd can swap another buffer into the new split and wipe the provider's buffer before termopen runs. nvim_buf_call then raised "Invalid buffer id" as a raw Lua error and left the split open. open_terminal now checks the buffer first and runs the normal failure path: one ERROR notice, the split closed, and its state cleared. Part of #333. _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
The mock said every buffer was invalid, so the provider's new buffer looked deleted and the wiped-buffer check skipped termopen. Only buffers from earlier tests stay invalid now, so no stale terminal is adopted. Part of #333. _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-5-5` • Thinking: `xhigh`_
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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. |
This was referenced Oct 31, 2026
hareki
pushed a commit
to hareki/claudecode.nvim
that referenced
this pull request
Nov 1, 2026
…oder#336) ## Why The goal is to clear the follow-up bugs listed in coder#333 and coder#334, one small reviewed PR per bug. Part of coder#334. This PR handles item 1. The native terminal buffer has `bufhidden=hide`. On main, `close_terminal` only closed the window and then cleared its state. On real Neovim this had three effects: 1. A visible terminal: `:ClaudeCodeClose` closed the window, but the Claude job kept running in a hidden buffer that the plugin no longer tracked. The next `:ClaudeCode` started a second Claude process. 2. A hidden terminal (toggle open, toggle hidden, `:ClaudeCodeClose`): `winid` is `nil`, so `nvim_win_close(nil, true)` raised `Invalid 'win': Expected Lua number`. 3. The terminal is the only window: close raised `E444: Cannot close last window`. The unit test "should preserve process on toggle but kill on close" passed only because the mock's `nvim_win_close` ignored a `nil` window id. `close_terminal` now stops the job, waits for it to exit, and then deletes the terminal buffer. Deleting the buffer closes every window that shows it, whether the terminal is visible or hidden. The wait matters: deleting a terminal buffer while a write to its job is still pending hangs Neovim (neovim/neovim#42508, open upstream). ## What changed - `lua/claudecode/terminal/native.lua` (`close_terminal`): - For a job the plugin started, `close` calls `jobstop` and waits up to 1 s for `TermClose`. Then it calls `nvim_buf_delete(bufnr, { force = true })`. The wait also ends when a user's `TermClose` autocmd already deleted the buffer, and then `close` skips the delete. - For an adopted terminal (`find_existing_claude_terminal`, no `jobid`), `close` only closes its window, as on main. The adoption matcher accepts any visible terminal whose name contains `claude`, so the terminal can be an unrelated shell. - `tests/unit/native_terminal_toggle_spec.lua`: the mock now behaves like Neovim in these points: - `nvim_win_close` raises for a `nil` or unknown window id. - `chansend` leaves a pending write, and `nvim_buf_delete` raises while a write is pending. - `jobstop` queues the job's exit, and `vim.wait` runs it (pending writes end, then `TermClose` fires). - Global `TermClose` autocmds run too, and a buffer-local autocmd is dropped once its buffer is deleted. - New and rewritten tests cover close for a hidden terminal, a visible terminal, close right after a send, a user `TermClose` autocmd that deletes the buffer, and an adopted shell. - `lua/claudecode/terminal/README.md`: the `native.close` side-effect line now describes the fixed behavior. ## Scope Only the native provider's `close`. Hide, show, toggle, and the snacks and external providers do not change. The loose `claude` substring match in `find_existing_claude_terminal` is older than this PR and stays as it is. ## Blast Radius Users of the native terminal provider who run `:ClaudeCodeClose` or call `require("claudecode.terminal").close()`. Close now ends the Claude session, which the test name already promised. Hide (toggle) still keeps the process alive. A close normally takes about 2 ms (measured with a node process). For a process that ignores SIGHUP, close blocks for 1 s, and Neovim then kills the process itself. Known Neovim race, not fixed here: on Neovim 0.9.5 to 0.12.6, `jobstop` on a terminal job within a few milliseconds of `termopen` can kill Neovim with "Caught deadly signal 'SIGHUP'". A raw script without the plugin (`termopen` and then `jobstop` at once, 20 times per run) killed Neovim in 3 to 5 of 15 runs on each version. With a 50 ms pause it never did. `close` now calls `jobstop`, so a script that opens and closes in the same tick can hit this. A person typing `:ClaudeCodeClose` cannot. My guess at the cause, from strace: the forked child gets SIGHUP before it resets its signal handlers, and libuv's signal pipe, shared with the parent, passes it to Neovim. I did not verify this in Neovim's source. ## Verification I ran a headless script (native provider, `terminal_cmd = "cat"`) with 17 checks on Neovim 0.8.3, 0.9.5, 0.10.4, 0.11.6, 0.12.3, and 0.12.6. | Code | Result | | --- | --- | | main `b03b0bf` (0.12.6) | 7 of 17 pass. Visible close leaves the job running. Hidden close and only-window close raise. | | first fix `e93a0db` | Hangs at "send then close" on 0.8.3 and 0.12.6 (killed by timeout). | | `55692ea` | 17 of 17 pass on all six versions. A user `TermClose` autocmd that deletes the buffer makes close raise `Invalid buffer id`. | | `ca70b2d` | 17 of 17 pass in 8 of 8 runs on each of the six versions. Close with that autocmd succeeds in about 1 ms. | | head `67a3605` (`ca70b2d` merged with main, which brings coder#339) | 17 of 17 pass in 4 of 4 runs on each of the six versions. | The script waits 50 ms after each open because of the Neovim race above. The checks cover close from the terminal window and from the editor window, hidden close, close right after a send (5 bytes, and 24 KB with submit), close then reopen, the only-window case, a second tab that shows the terminal, an adopted `:terminal` shell (its process keeps running), and close with no terminal. Other runs: - A 64 KB send to a process that ignores SIGHUP and SIGTERM, then close: completes in about 1 s on 0.9.5 and 0.12.6, the buffer is gone, and Neovim kills the process within 6 s. - Raw Neovim, `chansend` and then `nvim_buf_delete`: hangs on 0.9.5 to 0.12.6. With `jobstop` and a delete after `TermClose`: completes. Unit tests: each test commit fails against the code before it, and the fix commits make them pass. `mise run check` (0 warnings), `mise run test` (743 successes, 0 failures), `stylua --check`, and `prettier --check` pass on the head commit. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-opus-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.
Why
The operator goal is to close issues #333 and #334 with small reviewed PRs. This PR covers the native terminal failure path from #333, items 1, 2, 3, 4, and 6.
open_terminalranenewin the new split and then treated whatever buffer the split showed as its own. When aBufEnterautocmd swaps another plugin's buffer into the split, a failed start leaves theenewbuffer listed. It also deletes the other plugin's buffer when that buffer has no name and no changes. A successful start in the same case turns the other plugin's buffer into the terminal. Every failed start also sent two ERROR notices.What changed
open_terminalcreates its buffer withnvim_create_buf, shows it withnvim_win_set_buf, and runstermopenin it throughnvim_buf_call. A failed start deletes only that buffer. After a start, the split shows that buffer again if an autocmd swapped it out.open_terminalsends the only notice, "Failed to open native terminal." plus thetermopenerror.M.open,M.simple_toggle, andM.focus_toggleno longer send "Failed to open Claude terminal using native fallback.".termopenturns the current buffer into the terminal). The test titles match their assertions. The externaljobstart failuretest asserts_get_terminal_for_test()is nil.lua/claudecode/terminal/README.mddescribes the new behavior.Scope
Blast Radius
Only the native provider's start path changes. A normal start still gives a listed terminal buffer with
bufhidden=hidein the split, with the sameon_exithandling. A real Neovim run on main and on this branch shows identical output for that case.Verification
origin/mainand this branch. With an autocmd that swaps in another plugin's scratch buffer and a missing command, main deletes that buffer and leaves an empty listed buffer behind. This branch keeps the plugin buffer and leaves no extra buffer. A working command gives the same terminal buffer, pty size, and exit cleanup on both.:ClaudeCodewithprovider = "native"and a missingterminal_cmd: main sends 2 ERROR notices, and this branch sends 1, which includes the E475 text.mise run checkandmise run test(724 tests) pass on the head.Part of #333
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:xhigh