Repository navigation
fix(terminal): keep a native start single when an autocmd interrupts it - #544
Conversation
… split A BufEnter or TermOpen autocmd that calls focus_toggle while the native provider starts Claude made open() raise "Invalid window id" at nvim_win_get_buf: the toggle adopted the half-started terminal, found its window focused, and hid it, and the start then read the window that was gone. Claude kept running with no window, and the error reached the user. A second simple_toggle from such an autocmd, or a plain :close, did the same. open_terminal no longer assumes that its split survives the start. When the split is gone, Claude is tracked as a hidden terminal, and the next toggle shows it. The check on the buffer that an autocmd swapped in, and the focus step, take the window only while it exists. The snacks provider has no such gap: it tracks its terminal only after Snacks.terminal.open returns, and a nested open, toggle or close during the start does nothing.
When one autocmd of a native start hid the terminal and a later one showed it again, show_hidden_terminal tracked the replacement window, and the start then replaced it with nil because the split it created was gone. Claude was visible but tracked as hidden: a focused open returned to the original window, and the exit of the job did not close the replacement window with auto_close. open_terminal now tracks the split while it exists and then looks for the window that shows the terminal, as is_valid does for every other entry point.
The vsplit of a native start fires WinNew and WinEnter before the plugin has a buffer for Claude. An open, simple_toggle or focus_toggle that such an autocmd called found nothing to adopt and started a second Claude, which nothing tracked. An autocmd that closed the new split, or moved to another window, made the start take the current window, so your only window showed Claude. A close() that a BufEnter or TermOpen autocmd called waited for the job inside termopen, which aborts Neovim 0.8, and on other versions it cancelled the start with a "Failed to open" error. The start now: - ignores an entry that an autocmd calls while the vsplit creates its split (snacks does the same while its start runs), - takes the split from the windows that did not exist before the vsplit, and runs hidden when none is left, - runs a close() that an autocmd of the start called when the start returns, - clears its phase when the start raises, so a failed vsplit cannot make later entries return.
…can close An autocmd that closes the new split during a native start unloaded the buffer when 'nohidden' was set, because the buffer got bufhidden=hide only after termopen. On Neovim 0.8, a BufEnter autocmd made Neovim load the buffer again before termopen, and its term:// reader started a second terminal in it, which the plugin tracked: a bare claude with no arguments and no plugin env. A TermOpen autocmd ended Claude with a "Failed to open native terminal" error. open_terminal now sets bufhidden=hide before it shows the buffer. The README says what a start does when an autocmd closes the split and when another one shows Claude again, and the hide-and-show tests reject a window that is gone, so that dropping the rescan fails them.
…e start The early bufhidden=hide of the previous commit replaced the assignment after termopen. An autocmd of the start that sets another value (a terminal cleanup autocmd that sets wipe) then won, and a later toggle wiped the buffer and ended Claude. The start sets hide before it shows the buffer, for 'nohidden', and again after the autocmds, as it did before.
An autocmd of the user's can change bufhidden on the Claude terminal after the start (a TermEnter autocmd that sets wipe). The next hide closed the window, which then wiped the buffer and ended Claude. A hide now sets bufhidden=hide before it closes the window, so it keeps the job whatever an autocmd did since the start.
|
@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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb575a0900
ℹ️ 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".
🛡️ 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 found that picking the split as "the window that did not exist before the vsplit" proves nothing: vsplit returns no id and runs the autocmds of the user's, so an autocmd that makes a split of its own during the vsplit has its window picked, and Claude replaces that window's buffer (AGENTS.md, Code Shape: neither the current window nor "the one new window" is proof of ownership). The vsplit now runs under :noautocmd. No WinNew or WinEnter autocmd runs for the new split, so none can close it, replace it, make a split of its own or call an entry before Claude has a buffer, and the current window afterwards is the split that the call made. That replaces the window list, the split phase and the guards of the three entries. A close() that an autocmd of the start calls still waits for the start to return.
|
@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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 405be541f4
ℹ️ 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".
A close() that an autocmd of the start asked for was dropped when a later step of the start raised after termopen had launched the job: the wrapper cleared the request and rethrew. Claude then kept running against the request. The wrapper now runs the close first, then raises the error of the start.
|
@codex review |
|
@codex security review |
|
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 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". |
…ob that is young nvim_get_proc runs ps on Unix. It took 100 to 190 ms a call, close called it at least twice, and a host without a working ps -p made every native close raise after the close had cleared its state: Claude kept running with no tracking, and the next open started a second one. process_name now reads /proc/<pid>/comm where /proc exists (a read of a few bytes) and asks nvim_get_proc under pcall elsewhere. When no name can be read, the close goes on to jobstop. Only a close of a job that started less than 10 s ago looks at all, so a normal close on macOS pays no ps call. Tests: the exited job looks at no process (a strict nvim_get_proc raises for a non-number pid), the missing ps, the 1 s limit, the age rule, and both name sources. The replay gets a check with no /proc and no ps. The docs and numbers follow the verifier of a171a86.
|
Review-cap waiver (from the maintainer, 2026-11-12): one 7th review round is allowed on this PR. It covers one push that fixes the round-6 verifier finding (native close raises when Generated with |
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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. |
A clean merge of #544 (terminal files only). Every covered.lock matches the code, so no lock needs a new record.
…after the merge of #544 treefmt pads two rows of the table that the merge joined: the facts of the native start now read `utils.win_set_buf_in_split` and `start_terminal`. _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
…de terminal lifecycle (coder#525) ## What this adds `.agents/skills/verify-terminal-lifecycle/`: a TLA+ model of the Claude terminal in `claudecode.nvim`, checked by TLC against 34 guards that `check.sh` reads from the Lua code, on the shared harness from coder#450. It covers the `native` and `snacks` providers, the code that finds a Claude after a plugin reload (`:Lazy reload`), the wrapper that adopts a snacks Claude, and the provider choice. A change that removes or rewords a guard makes a rule break, or makes the covered code read STALE (exit 3) until someone confirms the model. The model describes `main` as it is now. The bugs that it found are fixed first: coder#451, coder#464, coder#482 and coder#527 are merged. coder#527 fixed a bug that a review comment on this PR found, and the model has a step for it now. ## The eight rules, as plain sentences | Rule | Plain sentence | | --- | --- | | `AtMostOneClaude` | At most one Claude runs, apart from the one that a close is stopping. | | `CloseStopsClaude` | When `:ClaudeCodeClose` returns, no Claude that ran when it began still runs. | | `CloseNeverReuses` | The plugin never tracks again a terminal that it began to close. | | `OpenShowsLiveClaude` | After an open, a show or a focus, you see a running Claude. A snacks terminal with `auto_close = false` is exempt, on purpose (it keeps an exited Claude). | | `HideKeepsClaude` | Hiding Claude's window keeps Claude running. | | `HideNoError` | Hiding Claude's window raises no error. | | `FailedStartLeavesNoWindow` | A failed start leaves no window, no buffer and no tracked terminal. | | `ExitClosesOnlyOwnWindow` | With `auto_close`, Claude's exit closes only Claude's own window, and never the last window. | ## Bugs this model found The first version of the model broke five rules on seven paths. I replayed each path on a real headless Neovim 0.12.3 (Claude is `sh -c 'sleep ...'`). Updates of the model after those fixes found an eighth and a ninth. Each fix is merged. | # | Rule | Bug | Fixed in | | --- | --- | --- | --- | | 1 | `AtMostOneClaude` | An autocmd that calls open while snacks starts Claude (`BufEnter`, `TermOpen`) started a second Claude. | coder#451 | | 2 | `AtMostOneClaude` | After a plugin reload, the next open started a second Claude (native adopted only a shown terminal, snacks adopted nothing). | coder#464 | | 3 | `CloseStopsClaude` | `:ClaudeCodeClose` after a reload left Claude running. | coder#464 | | 4 | `OpenShowsLiveClaude` | An open between Claude's exit and the scheduled cleanup (native) focused the dead terminal and started no Claude. | coder#451 | | 5 | `OpenShowsLiveClaude` | After `:e file` in Claude's window (native), `:ClaudeCodeOpen` focused your file and Claude stayed hidden. | coder#451 | | 6 | `HideNoError` | The native toggle raised `E444` when Claude's window was the only window. | coder#451 | | 7 | `ExitClosesOnlyOwnWindow` | After a wipe, the native exit cleanup closed the window that showed your file. | coder#451 | | 8 | `OpenShowsLiveClaude` | An exited snacks Claude stayed tracked until its scheduled close, and a Claude that started in that gap lost its Snacks key, so the next reload started a second Claude. | coder#482 | | 9 | `HideNoError` | A `BufEnter` or `TermOpen` autocmd that called `focus_toggle` while native started Claude made the open raise `Invalid window id`, and Claude kept running with no window. | coder#527 | Replays of each bug, against the code before its fix (`replays/`, exit 1 means findings): | Tree | Replays with a finding | | --- | --- | | before coder#451 (`8affa12`) | `snacks-reentrant-open` 2 of 2, `native-open-after-exit` 2 of 2, `native-open-after-swap` 2 of 2, `native-hide-only-window` 2 of 2, `native-wipe-closes-your-window` 1 of 1, plus the reload replays below | | before coder#464 (`14798d1`) | `native-reload-hidden` 1 of 1, `snacks-reload` 2 of 2, `auto-snacks-later` 1 of 2, `native-reload-close` 1 of 1 | | before coder#482 (`3498879`) | `snacks-reload-exit` 16 of 16, `snacks-exit-handlers` 1 of 4 | | this branch | 20 replays, 0 findings (`native-start-autocmd-hide` is new: 18 checks, 10 findings before coder#527 on Neovim 0.12.3 and on the 0.8-era build) | Eight of the 20 replays (`snacks-reload-adopted`, `snacks-reload-keys`, `-tabs`, `-backdrop`, `-older`, `-edit`, `-anchor`, `-focus`) guard the wrapper that coder#482 builds for an adopted snacks Claude: they are no model findings, because the model has no keys, tabs, backdrops or window sizes. Verification of the first drafts of coder#482 found each defect, and each fix landed before coder#482 did. SKILL.md lists them. ## Proof All on this branch (head `a639966`, `main` at `0d55c25` merged), Neovim 0.12.3, Java from mise: - `check.sh`: `main: pass (263060 distinct states found)`, `== RESULT: PASS`, all 34 facts `TRUE`. - `check.sh --mutants` (about 11 minutes on a loaded host): 34 mutant keys (26 single guards and 8 pairs), `== mutants: every guard is caught`, exit 0. Every rule breaks at least once (`AtMostOneClaude` 10, `CloseStopsClaude` 10, `OpenShowsLiveClaude` 9, `CloseNeverReuses` 5, `HideNoError` 3, `FailedStartLeavesNoWindow` 2, `ExitClosesOnlyOwnWindow` 2, `HideKeepsClaude` 1). I ran it on this head. - Scratch break: I removed the `owned.job_ended(terminal.buf)` check from `forget_ended_terminal` in `snacks.lua`. `check.sh` exits 1, `SnacksEntriesForgetEndedJob FALSE`, `main: BROKEN: OpenShowsLiveClaude`. After `git checkout lua`, `git diff --shortstat` is empty and `--hash-only` reads "covered.lock matches the code". - Scratch breaks for the two facts that Codex asked for in round 1: moving the native mark after `nvim_win_set_buf` makes `check.sh` exit 1 with `NativeMarkBeforeTermopen FALSE`, and removing the `jobstop` call of the stop helper in `owned.lua` makes it exit 1 with `OwnedStopOtherStops FALSE`. Both files were restored after each break. - Scratch breaks for the model step and the verifier fixes: with the native.lua from before coder#527, `check.sh` exits 1 (`NativeKeepsHiddenJob FALSE`, `NativeStartSurvivesClosedSplit FALSE`, `main: BROKEN: HideKeepsClaude`). With the `return` deleted from `hide_terminal`, it exits 1 (`NativeHideKeepsLastWindow FALSE`, `main: BROKEN: HideNoError`). The six rewrites that a verifier found to leave a fact TRUE with the guard gone (the adoption, `is_last_window`, the order of `forget_ended_terminal`, `job_ended`, the hide return, `if starting then`) read FALSE now. Files are restored after each break. - `scripts/tla/test-check-lib.sh`: `COUNT cases=44 failed=0 skipped=0`. - Replays: 20 scripts, 0 findings each, exit 0 (see the table above for the code before each fix). `run.sh` exits 2 for a missing Neovim, a replay that raises and one that hangs in `vim.wait` (a replay stuck in a CPU loop can outlive `timeout`'s SIGTERM), and it runs from a path with spaces, commas, pipes and backslashes (the repository and the snacks.nvim checkout). - `treefmt --ci` (stylua, prettier, shfmt -i 2, shellcheck, actionlint, zizmor): 383 files, 0 changed. `busted tests`: 1529 successes, 0 failures, 25 pending. `mise run test` (the CI file set, coverage, seed 1794460250): 1702 successes, 0 failures. luacheck: 0 warnings. - `covered.lock` is re-recorded on this head, which merges `main` at `0d55c25` (it had been recorded at `37eaa0f`). Changed since then in the covered files: `terminal.lua` (a warning message of coder#530, file line only), `native.lua` (coder#527, block `open_terminal`) and `snacks.lua` (coder#526 dead stores, block `snacks_open` and the wrapper blocks, and the comment of coder#534). I read each diff: none changes a guard that a fact reads, and every fact reads TRUE except for the one that coder#527 moved (`NativeKeepsHiddenJob`, updated). `--hash-only` reads "covered.lock matches the code" (about 10 s). ## Run it ```bash .agents/skills/verify-terminal-lifecycle/check.sh # about 1 min (30 to 50 s) .agents/skills/verify-terminal-lifecycle/check.sh --mutants # about 12 to 15 min, after you edit the model or a fact .agents/skills/verify-terminal-lifecycle/check.sh --hash-only # about 10 s, only compares covered.lock with the code SNACKS_NVIM=<snacks.nvim checkout> NVIM=<path to nvim> \ .agents/skills/verify-terminal-lifecycle/replays/run.sh snacks-reload-keys.lua ``` CI runs every `.agents/skills/verify-*/check.sh` in the `covered-lock` job, as for `verify-diff-lifecycle`. ## Notes for the reviewer - The diff is one new skill folder (SKILL.md, `check.sh`, `covered.lock`, `model/` with the TLA+ spec and a `/statechart` view, and `replays/`) and one sentence in `lua/claudecode/terminal/README.md`. The sentence says that a float of an older version has no backdrop "to hide or to show" until Snacks re-creates it (the text said only "to hide"). It touches no Lua code. - The skill does not model the items that SKILL.md lists under "Scope and bounds", each with what you would see if it broke. They include the `external`, `none` and custom table providers, a close that starts during another close's wait, more than one open or toggle per start from an autocmd, a `WinNew` or `WinEnter` autocmd that calls an entry while native's `vsplit` runs before the buffer has its mark (two Claudes run; open PR coder#544 proposes a fix), which window the plugin made (an exit with `auto_close` closes a window of yours that shows the terminal buffer), a cleanup that raises (`E444` in a failed start's window close), a `TermClose` autocmd of your own that closes the terminal, Snacks floats, tab pages, window sizes, a second reload, `:bdelete`, `:q` in Claude's window, and Neovim exit. It assumes Snacks' `fixbuf`, so `:e` in a snacks window is no step. - `DEVELOPMENT.md` is not edited here: the skill-listing change belongs to the PR that owns it. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
…#593) ## Why `tests/integration/diff_window_sizes_spec.lua` died once under host load with `Nvim: Caught deadly signal 'SIGHUP'` (seen on a coder#573 head, green on the rerun, and main has the same exposure). The runner then reported "no plenary summary (nvim exit 1)". A test must not kill the Neovim that runs it, and a retry hides the cause. ## Cause The cause is a race inside Neovim and libuv. It is not a signal mask or disposition that stays wrong after exec. 1. `pty_process_spawn` calls `forkpty()`. In the child, `login_tty()` gives it a new session on the pty. 2. `init_child()` then runs `setsid()`, resets SIGCHLD, SIGHUP, SIGINT, SIGQUIT, SIGTERM and SIGALRM to their defaults, changes directory, builds the environment and calls `execvp`. Until exec the child is a copy of Neovim and has Neovim's libuv signal handlers. 3. When the pty master closes in that gap (`jobstop`, a buffer wipe), the kernel sends SIGHUP to the child. libuv's inherited handler writes a message into the signal pipe that the child shares with Neovim. Neovim reads it as its own SIGHUP and exits. `strace -f` on a held child shows it. The child gets `SIGHUP {si_code=SI_KERNEL}` and runs the handler, which writes to fd 11 on Neovim 0.8.0 (fd 8 on 0.12.3) and then one byte to a second descriptor. The parent then prints `Caught deadly signal 'SIGHUP'`. The 0.8.0 and 0.12.3 sources have the same order in `init_child()`. The spec reaches it because several tests start a terminal job (`terminal.open()` or `termopen({ "sleep", "60" })`), end within milliseconds, and `after_each` wipes the buffer. A child that the host scheduler delays inside the gap is enough. coder#336 had already measured the same race on a plain `termopen` followed by `jobstop`. ## Fix `tests/helpers/termopen_wait.lua` wraps `vim.fn.termopen`, and `tests/minimal_init.lua` installs it. The wrapper returns only after `/proc/<pid>/exe` or `/proc/<pid>/cmdline` of the child differs from Neovim's own, so the child has run `exec` and holds none of Neovim's handlers. It compares the command line too, because a job that runs Neovim itself keeps the same executable. The wait ends with an assertion after 10 s. Every integration spec and every Neovim that loads `minimal_init.lua` gets it. The wrapper reads `/proc`, so it works on Linux only (CI runs on Linux). On another system it does nothing. - `tests/integration/pty_child_exec_spec.lua` has 10 cases. Five drive the wrapper with stand-ins for `/proc` (a child that runs Neovim's code for N reads, a child that has run exec, a job that runs Neovim with another command line, a child that never runs exec, a job with no pid). One checks that `vim.fn.termopen` is the wrapper. Four use real jobs: a plain job, a job that a `TermOpen` autocmd stops, a job whose buffer a `TermOpen` autocmd wipes, and a job that runs Neovim. - `.agents/skills/verify-claudecode-nvim/scripts/proof-pty-child-hangup.sh` reproduces the race with an `LD_PRELOAD` shim that holds the child inside the `signal(SIGCHLD, SIG_DFL)` call that follows `setsid()`, and it checks the wrapper against it. It saves the PID and start time of each case's child and kills only that process. `SKILL.md` and `features/terminal.md` point to it. - `DEVELOPMENT.md` has a paragraph about the wrapper and what it does not cover. No plugin code changes. The merge of `origin/main` brings in main's own `diff.lua` change. ## Evidence All runs use the official `v0.8.0` release binary (`NVIM v0.8.0-1210-gd367ed9b2`, as in CI) or Neovim 0.12.3, with a scratch HOME and XDG dirs, on a shared host with a load average of 20 to 100. The injected rates depend on the host load. | Run | main | this branch | |---|---|---| | `diff_window_sizes_spec` with the child held 100 ms in the gap, 0.8.0 | 13 of 20 die by SIGHUP | 0 of 20 | | same, 0.12.3 | 18 of 20 die by SIGHUP | 0 of 20 | | `diff_window_sizes_spec`, no shim | 0 of 200 fail (0.8.0) | 0 of 20 fail on 0.8.0 and on 0.12.3 | | plain `termopen`, spin 50 to 1200 µs, `jobstop`, 40 runs per delay (0.8.0) | 2 of 200 die by SIGHUP | not applicable, it uses no `minimal_init.lua` | | `proof-pty-child-hangup.sh` | the `wrapper` case fails on 0.8.0 and 0.12.3 | 3 of 3 cases pass on both | The wrapper spec against 12 mutants of the wrapper (wrapper not installed, no wait, never waits because `/proc/self/exe` is unreadable, wait while the executable differs, wait while the command line differs, executable check only, a single 1 ms poll, no deadline assertion, a 1 ms deadline, no `pcall` around `jobpid`, no sleep, polling Neovim's own pid) and a control: the control is red 0 of 3 and every mutant is red 3 of 3, on 0.8.0 and on 0.12.3. The first version of the spec caught a single 1 ms poll only 2 of 6 times on 0.8.0 and 4 of 6 on 0.12.3. Two cases that the first version got wrong now hold (the nested-Neovim case was wrong in the wrapper, the slow-parent case in the spec): - A job that runs Neovim itself. The old wait took the whole job (2 s for `sleep 2`) or failed after 10 s (for `sleep 15`). Now `termopen` returns after 7 to 14 ms. - A `TermOpen` autocmd that takes 50 ms. With main's `minimal_init.lua` plus that autocmd, the old spec was red 0 of 5 and the new spec is red 5 of 5 on both builds. With this branch's `minimal_init.lua` plus the same autocmd, the new spec is red 0 of 5. The child's gap after the fork is only 2 to 6 ms, even with an environment of 20000 to 200000 entries, so no setting of the spec can keep the child in Neovim's code through such an autocmd. That is why the wait is pinned with stand-ins. The proof script left one `sleep 60` per run before this fix (3 of 3 runs), and leaves none now. ## Not proven or not covered - Natural runs of `diff_window_sizes_spec` on main did not die in 200 tries on this host. The original death is explained by the mechanism and by the injected runs, but I did not reproduce that exact occurrence naturally. - The wrapper covers `vim.fn.termopen` only. A job that a spec starts with `jobstart` and `term = true` is not covered. No spec does that today. - A stop that a `TermOpen` autocmd issues inside `termopen` runs before the wait, so it is outside the wrapper. For a job that runs Neovim with the same command line as the test Neovim, `termopen` blocks until that job exits and raises if it lives 10 s. A poll that lands inside `exec` can return early. No spec starts such a job. - The plugin has the same exposure outside the tests. `:ClaudeCodeClose` of the native terminal waits for the exec of a young job (coder#544). A direct `jobstop` or `:bwipeout` of the terminal buffer right after `termopen`, and the Snacks close, do not wait. This PR changes no plugin code. - Neovim and libuv have the root fix to make (block SIGHUP across `forkpty()` or reset the handlers before the session becomes the pty's). I did not report it upstream. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
… connection lifecycle (coder#474) Adds `.agents/skills/verify-ws-connection`: a TLA+ model of one WebSocket connection between Claude and the plugin, checked with TLC against guards that `check.sh` reads from the Lua source. It is the second `verify-*` skill on the shared harness (`scripts/tla/check-lib.sh`, coder#450), after `verify-diff-lifecycle`. It describes the keepalive as coder#488 merged it (d63c9b3, with the fix for the fresh clock at the tick). On the code of coder#479 it reads STALE, and its facts for the new design read FALSE. ## What it checks Six rules, from the TCP accept through the handshake, requests, Close frames, keepalive pings and server stop: | Rule | Plain sentence | | --- | --- | | `NoRequestAfterEnd` | A request from Claude runs only after its 101 reply was written, and never after the plugin started to close the connection or saw it end. | | `DisconnectReportedOnce` | `on_disconnect` runs at most once per client. | | `StatusCloseDelivered` | A Close frame for a protocol error (1002, 1003, 1007, 1009) or a server stop (1001) is written before the plugin closes the socket, as long as Claude keeps reading. A Claude whose queue makes no progress for one keepalive window loses the frame to the deadline of `close_client`, which closes the socket (not modeled). | | `NoFalseTimeout` | A Claude that answers each ping before the next tick is never dropped by the ping timer, whatever blocks Neovim or sleeps the machine. A tick reads a fresh clock, only pings and stamps the oldest unanswered ping. The check that drops a client runs after a poll that read the sockets, judges nothing when its poll comes more than 1.5 intervals after its tick, and any bytes read from the client clear the stamp. A Claude that stopped once is not covered. A pong that comes later than one interval is not covered either: when a callback blocks inside the poll phase and refreshes the loop time, the poll can miss it, and the check closes that client once its ping is 2.5 intervals old (the verifier's probe on Neovim 0.12.3 dropped a peer that answered 2.4, 1.4 and 0.4 intervals late. Main does the same.) A real Claude answers a ping at once. The rule holds for any limit above `interval * 1.5`, the window in which a check still judges: a limit of 1.2 intervals passes the model, and real Neovim drops a Claude that answered within 100 ms when a read callback blocks about 1.3 intervals in the poll after the tick. The code keeps 2.5. | | `DeadClientDropped` | A client whose Claude hung up or stopped answering, or whose connection the plugin started to close, eventually leaves `server.clients` with its `on_disconnect`. | | `DroppedSocketCloses` | Once a client left `server.clients`, or the server stopped, its socket eventually closes, as long as Claude reads again. | `covered.lock` hashes the five files the model describes (`server/client.lua`, `tcp.lua`, `handshake.lua`, `server/init.lua`, `init.lua`) and the blocks inside them, so a change there reads `STALE` until someone confirms the model still fits. The model describes the keepalive judgement only. The stall rule of coder#467 (`judge_stall`) and the deadline of a Close frame are not modeled, and `SKILL.md` says what a user sees for each. ## Bugs this model found Its first run broke three rules on the code as it was. Each one was replayed with real sockets and fixed in coder#455. Reviews of this PR then found four more false timeouts that the first model could not see. coder#479 and coder#488 (three rounds of it) fixed them: - `NoFalseTimeout`: a main loop blocked during the handshake got a Claude that answers every ping dropped with 1011 before any ping reached it. Since coder#488 a client has no stamp until a tick pings it, so no block can time it out. - `DeadClientDropped`: after a protocol error while Claude's window was full, the client stayed in `server.clients` (and `is_claude_connected()` stayed true) until Claude read again. - `DroppedSocketCloses`: a stop between Claude's Close frame and the scheduled `on_close` left the plugin's socket in `CLOSE_WAIT`. - `NoFalseTimeout`, again (coder#479): a tick that Neovim ran late after a block of 1.0 to 1.5 intervals can run before the plugin reads Claude's pong, and found the last pong 2.3 intervals old. libuv runs timers before it reads sockets: with libuv 1.44, which Neovim 0.8 bundles, after a block in any callback, and with newer libuv after a block in a check or I/O callback, not after one in `vim.schedule` or a timer callback. - `NoFalseTimeout`, a third time (coder#488): libuv 1.44 runs the timers at the end of a pass of the main loop and again at the start of the next, with Neovim's `vim.schedule` callbacks between. Two ticks ran with no read between them, and coder#479 dropped a Claude that answered every ping. No counter of ticks fixes that, so a tick now only pings and a libuv check handle, which runs after the poll, does the dropping. - `NoFalseTimeout`, a fourth time (the review of coder#488): the check ran on a poll that did not reach Claude's answer. A pong waits behind more unread data than the socket holds (any bytes from the client now clear the stamp), or the machine sleeps between a tick and its poll (the check judges nothing after a block longer than 1.5 intervals). Each of the two guards alone leaves one path open. The model has both, and a replay for each. - `NoFalseTimeout`, a fifth time (Codex's review of coder#488): the tick read `vim.loop.now()` before it called `update_time()`. libuv runs all timers of a pass with the loop time of the start of the pass, so after a sleep or a block of more than 1.5 intervals in an earlier timer callback of the same pass the tick saw no gap, kept the old stamps, and the check dropped a Claude that had no time to answer. The tick now updates the loop time first. There is no replay for it (a replay cannot make a timer callback run before the ping timer inside one pass), so the unit test of coder#488 and the model pin it: with `ClockFreshAtTick` off, `NoFalseTimeout` breaks. The model in this PR describes the fixed code (18 guards, all read TRUE). ## Round 7 (the one extra review round that the maintainer allowed) This push carries the prepared fixes for the last findings of the two verifiers of `77bb6e2`, and merges of main. The model (`model/`) and the `EXPECT` map are the same as in `77bb6e2`. - The limit that the promise holds for. `NoFalseTimeout` holds for any limit above `interval * 1.5`, not for one interval or more. A limit of 1.2 intervals passes the model, and real Neovim drops a Claude that answered within 100 ms when a read callback blocks about 1.3 intervals in the poll after the tick. `SKILL.md` says so now. The code keeps 2.5. - Which test catches which clock mutation. `SKILL.md` named the wrong test for one of them. Deleting `update_time()`, or moving it after `local elapsed` without reading `now` again, turns red only "ages a ping from when it was sent...". Moving it after the wake test and reading `now` again turns red only "takes a tick for a wake...". I ran both mutations. - The README and the comment of `start_ping_timer` say that the first check that finds the oldest unanswered ping `interval * 2.5` old closes a silent Claude, and that a block inside the poll phase refreshes the loop time. - `check.sh`: the record pattern lists `write(client, ` and `handle:close()`, so a change to one of those lines reads STALE. The `DispatchGuard` fact needs the `end` of its `if` right after `on_message`. The `StopWaitsForCloseWrite` fact rejects `release_socket`, `abandon_close_frame` and `drop_client` in `stop_server`. Five wording fixes in `SKILL.md`, and a note that the `sleep` case of the bytes-only contrast on Neovim 0.8.3 depends on timing. - Merges of main (the last one is coder#577). The server README keeps this PR's keepalive paragraph and takes main's new bullets. After the first two merges three locks read STALE and are recorded again: `verify-diff-lifecycle` and `verify-mention-queue` (the hash of `tcp.lua`, which differs from main only in the comment of `start_ping_timer`), and `verify-ws-connection` (`tcp.lua`, and `M.start` and `M.stop` in `server/init.lua`, where main's coder#539 changed a log line in `on_open` and coder#558 answers in-flight requests before `stop_server`). The second merge made the ws lock stale once more: main's coder#541 changed `M._broadcast_at_mention` in `init.lua`, which the model does not cover, and the covered blocks have the same hashes. The model still fits: those answers are ordinary writes to a connected client, the case the model has. The last two merges (coder#544 and coder#577, terminal and diff files only) needed no new record. ## What changed since the first version of this PR - The model follows coder#488: the facts `DropsOnSecondOverdueTick`, `AliveTickClearsOverdue`, `PongResetsAge`, `WakeResetsPong` and `UpgradeStartsKeepalive` are gone. `JudgesAfterRead`, `ClockFreshAtTick`, `StampKeepsOldestPing`, `BytesClearStamp`, `GapSkip` and `WakeClearsStamp` replace them. The environment steps are `StaleLoopTime`, `PollMissesPong` and `SleepInWindow`. A `stale` tick (an earlier callback of the pass blocked or slept) is a wake that only a tick with a fresh clock sees. - **`NoFalseTimeout` no longer counts awake time.** The first model added one tick of awake time on a late tick, so its own sentence ("a block does not count as awake time") was false, and a drop at a late tick while a pong waited unread passed. The rule now asks whether a Claude that never stopped answering was dropped, and the ticks run at any point between reads. A variant that judges at the tick (`JudgesAfterRead` off) breaks it in 12 states. - The text of `SKILL.md` is fixed: the libuv wording, the timings, the exit codes of `--hash-only`, the numbering of the update steps, the order of the `/statechart` steps (the kept directory is deleted after the view is handed over), the sentence about `--mutants` (`RejectClosesSocket` is caught only together with `UnlistedDisconnectReleases`), and each item that is not modeled now says what a user sees. That includes a client that sends bytes and never reads (coder#467 handles that stall) and a client that writes data but never answers a ping, which coder#488 keeps by design. ## What this PR changes outside the skill - `server/README.md` and the comment of `start_ping_timer` described the silent close and the promise wrongly. A silent Claude is closed by the first check that finds its oldest unanswered ping `interval * 2.5` old: 90 s after the ping when ticks come on time. One late tick delays it, because a late tick moves the ticks after it back. Two late ticks in a row, or a poll that a block delays after a tick, bring it as early as 75 s (2.6 intervals in the probes, never earlier). A tick more than `interval * 1.5` after the tick before it is a wake and restarts the count, so a block of more than half an interval just before a tick is enough, not only one of 1.5 intervals. The promise of no false close is now for a Claude that answers each ping before the next tick, as the model checks it, and the README says what a pong later than one interval costs under a block in the poll phase. - A comment in `connection_lifecycle_spec.lua` still named the pong rule and `pong_overdue`, which coder#488 removed. It names the keepalive and the stall rule now. - The comment-only edits to `client.lua`, `tcp.lua` and the spec that this PR carried for the design of coder#479 are gone, because coder#488 replaced that code. - `DEVELOPMENT.md`: the two paragraphs about `covered.lock` name every `verify-*` lock, not only that of `verify-diff-lifecycle` (unchanged in this push). ## Proof - `check.sh` on this branch (checked on the head before the last merge): all 18 facts TRUE and `RESULT: PASS`. `main` 262,285 states, `live-NoFalseTimeout` 256,264, `live-DeadClientDropped` 11,701, `live-DroppedSocketCloses` 11,701. It took 256 s and 271 s in two runs at a host load average of 14 to 40, and 2 to 6 min in other runs. The `check.sh` of the other four skills (`verify-diff-lifecycle`, `verify-mention-queue`, `verify-selection-tracking`, `verify-terminal-lifecycle`) all pass too, and all five locks match the code (`--hash-only`). - `check.sh --mutants` (one TLC worker): I ran it uninterrupted on the head of this push, before the last three merges of main (they change no covered block of this skill): 18 rows, every guard caught, exit 0, 5,753 s at a load average of 14 to 40. Both verifiers of `77bb6e2` had run it uninterrupted too (Opus: 4761 s, gpt: 4814 s, 18 rows each). `git diff 77bb6e2 HEAD -- .agents/skills/verify-ws-connection/model` is empty and the `EXPECT` map is the same in all three runs. The new and changed guards are caught by `NoFalseTimeout` (`JudgesAfterRead`, `ClockFreshAtTick`, `BytesClearStamp`, `GapSkip`, `WakeClearsStamp`) and `DeadClientDropped` (`KeepaliveReaps`, `StampKeepsOldestPing`). `RejectClosesSocket` alone is caught by no rule, because `_disconnect_client` also closes the socket of a client that left the server, so it is removed together with `UnlistedDisconnectReleases`. - A design check outside `check.sh` for the fresh clock: with the `ClockFreshAtTick` fact forced FALSE, `main` passes (264,485 states) and `live-NoFalseTimeout` breaks. So the fact is needed, and the model shows why. - A design check outside `check.sh`: with a clear of the stamp on a PONG only, in place of the any-bytes clear (the design before the any-bytes rule of coder#488), `NoFalseTimeout` breaks through `PollAndCheckPartial`, the poll that reads only the first frame. So `BytesClearStamp` is needed, and the model shows why. The verifier's run of that variant found 83,746 states. The count depends on how the variant is cut, so I give no count of my own. - Fact fidelity: both verifiers removed each guard in the Lua and every one of the 18 facts flipped to FALSE (26 direct removals in Opus's run). Eight mutants that no fact saw now flip one: five in `tcp.lua` (no `table.insert(pinged, client)`, no ping sent, `judge_silence` called at the tick, `fn` run at once in `after_next_poll`, the `end` of the gap `if` moved above the call), and three guard moves from the last round (the `end` of the `DispatchGuard` if above `on_message`, and `abandon_close_frame` with `release_socket`, or `drop_client`, in the `stop_server` loop). I reran the verifier's 42 mutations on the new `check.sh`: none is blind. - `scripts/tla/test-check-lib.sh`: `COUNT cases=44 failed=0 skipped=0`. - Replays (real Neovim, real sockets), run again on the head before the last three merges of main, on Neovim 0.12.3 and on 0.8.3: `gap-during-handshake` 3 of 3, `short-block-after-ping` 3 of 3, `two-blocks-before-read` 3 of 3, `blocks-before-the-poll` 3 of 3, `protocol-close-window-full` 5 of 5, `stop-before-on-close` 4 of 4. `protocol-close-window-full.lua` changed for the merged code: it uses an interval of 500 ms and checks the open socket after half a keepalive window, because the deadline of `close_client` (merged on main since the replay was written) closes a socket whose Close frame makes no progress for one window, so it no longer stays open for ever while Claude does not read. The first three of the older replays keep `1 of 3`, `3 of 5` and `2 of 4` on main before coder#455 (3d4332c). `short-block-after-ping` keeps `2 of 3` on main before coder#479 (b65d266). `two-blocks-before-read` keeps `1 of 3` on the code of coder#479 on Neovim 0.8.3. `blocks-before-the-poll` keeps `0 of 3` on the first push of coder#488 (f3576ea) and with no clear of the stamp at all. With the gap skip and a clear on a PONG only it keeps `big-message` and `sleep` and breaks `two-blocks`. With the any-bytes clear and no gap skip it keeps `big-message` and `two-blocks` and breaks `sleep`. All on both Neovim versions. Both verifiers reproduced the contrast numbers of the other replays. The `sleep` run of the bytes-only build on Neovim 0.8.3 depends on timing: it kept all three once and two of three in five repeats. The last two replays run Claude in a second Neovim, because an in-process Claude answers only when the blocked loop runs again. - The full busted suite with CI's flags (`--coverage -v --shuffle`) on this branch: seeds 424242 and 9 on the head before the last two merges of main, each 1858 successes, 0 failures, 34 pending. Seed 424242 on `a929777` (merged with main at coder#544) 1887 successes, 0 failures, 34 pending. The last merge (coder#577, `diff.lua` and its spec) I did not run again as a whole suite. `luacheck` (with `--no-unused-args --no-max-line-length`) reports 0 warnings in 196 files. `treefmt` (stylua, prettier, shfmt, shellcheck) reports no change on the 16 files this PR touches. Every `covered.lock` matches the code (`--hash-only`) on the head of this push, and the full `check.sh` of `verify-diff-lifecycle` and `verify-terminal-lifecycle` pass on `a929777`. ## Run it ```bash .agents/skills/verify-ws-connection/check.sh # 2 to 6 min .agents/skills/verify-ws-connection/check.sh --mutants # about 80 min at a load average of 14, after you edit the model or a fact .agents/skills/verify-ws-connection/check.sh --hash-only # no TLC: compares covered.lock with the code NVIM=/path/to/nvim .agents/skills/verify-ws-connection/replays/run.sh blocks-before-the-poll.lua ``` CI's `covered-lock` job already runs every `.agents/skills/verify-*/check.sh`. `SKILL.md` has the rules, how to read a result, how to update the model, and its bounds. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
…the owned stop (coder#602) ## Why This is a follow-up to coder#593. It fixes the Snacks close and the stop in `owned.lua`, the two close paths that coder#593 left open. A job that was just started can still be a fork of Neovim until its child has run `exec`. A `jobstop` on it then ends Neovim with `Caught deadly signal 'SIGHUP'`, or the buffer delete after it hangs. coder#593 documents the race, and coder#544 made the native close wait for the exec of a young job. The Snacks close and the stop in `owned.lua` still called `jobstop` at once, so a close within milliseconds of the start could kill Neovim. ## Cause `pty_process_spawn` forks the child, and `init_child()` runs `setsid()` and resets the signal handlers only after that. Until `exec` the child has Neovim's libuv handlers. When the pty master closes in that gap (a `jobstop`), the kernel sends SIGHUP to the child, libuv's handler in the child writes into the signal pipe that it shares with Neovim, and Neovim reads a SIGHUP of its own and exits. With the child held in the gap (see Evidence), a close right after the start killed Neovim in every run for the Snacks provider and for the stop in `owned.lua`, on 0.8.0 and 0.12.3. The native provider survived in every run. ## Fix The wait moves out of `native.lua` into `owned.wait_for_exec(jobid)`, with `owned.is_young(started_at)` for the 10 s rule. The code is the same: it reads the names of Neovim and of the child from `/proc/<pid>/comm` (or `nvim_get_proc` where `/proc` is missing) and waits up to 1 s until the child has another name. - `native.close` calls it for a job younger than 10 s, as before. - `snacks.M.open` records `term._cc.job_started_at` where it already patches the instance. `snacks.close` reads it and calls the wait for a job younger than 10 s. A terminal that a reload adopted has no start time and is not waited for, as in native. Without the age rule, every Snacks close on macOS would pay about 200 ms for the two `ps` calls of the `nvim_get_proc` fallback. - `owned.lua` `stop_buffer` has no start time, so it calls it for every job. That path runs only after a reload or a provider switch, and the wait ends at the first look for a job that has run `exec`. The wait runs scheduled callbacks while the job still runs. Two Codex findings on `ea0fbfd6` were about that, and `12f1d45f` fixes both: - `snacks.close` now clears the owned mark before the wait for every terminal, not only for an adopted one. An `open()` among the callbacks can no longer adopt the terminal that is closing by its mark. - `stop_buffer` reads the channel before the wait and checks `nvim_buf_is_valid(buf)` before `jobstop`. A callback that deleted the buffer during the wait no longer makes `jobstop` raise `Invalid buffer id`. `lua/claudecode/terminal/README.md` and the `verify-terminal-lifecycle` skill describe the three places. The skill changed as follows: - The fact `OwnedStopOtherStops` reads `M.wait_for_exec(channel)` and the buffer check before `jobstop`. - The new fact `SnacksCloseClearsMark` reads the mark clear before the wait. The model has a switch of the same name in `CloseBegin`. The model counts a closing terminal as ended, so the fact breaks a rule only together with `OwnedLiveSkipsEndedJob`. `check.sh --mutants` has a row for the pair. - `OwnedLiveSkipsEndedJob` alone now breaks `OpenShowsLiveClaude`, not `CloseNeverReuses`, because the model clears the mark in every Snacks close. - The model still passes with 279492 distinct states. ## CI on `ea0fbfd6` and the fixes Two checks failed on `ea0fbfd6`. 1. The integration spec compiled its shim into a TMPDIR folder. CI's second pass runs with `TMPDIR="$RUNNER_TEMP/odd tmp 50%"`. `LD_PRELOAD` splits its value at a space or a colon, so the shim was ignored, the child was never held, and the probe timed out. `f910015b` puts the shim into `mktemp -d /tmp/claudecode.XXXXXX` when the TMPDIR path holds a space or a colon. 2. `verify-diff-lifecycle` also hashes `terminal/{native,owned,snacks}.lua`, so its `covered.lock` was stale. `3a1961a0` re-records the locks of `verify-terminal-lifecycle` and `verify-diff-lifecycle`. ## Tests - `tests/integration/terminal_close_young_job_spec.lua` runs one Neovim per case with `-u NONE` (so `minimal_init.lua` does not wait for the exec and hide the race) and `tests/helpers/hold_pty_child.c` preloaded. The shim holds the child inside `init_child()` and creates a file when it does. The probe (`tests/helpers/close_young_job_probe.lua`) starts a Claude terminal job, waits for that file, and closes the job. Cases: a plain `jobstop` as the control (it must end Neovim with SIGHUP, so the other cases can fail), the native close, the Snacks close (with a stand-in for Snacks) and the `owned.lua` stop. The spec is pending without Linux, a C compiler and `timeout`. It puts the shim into a `/tmp` folder when TMPDIR holds a space or a colon, and `tests/helpers/ld_preload_path.lua` makes that choice. - `tests/unit/ld_preload_path_spec.lua` has three cases: a plain path, a path with a space and a path with a colon. Before it, a spec pattern without the colon half passed in every CI configuration. - `tests/unit/terminal/owned_wait_for_exec_spec.lua` covers the wait (names from `/proc`, from `nvim_get_proc`, the 1 s limit, a job with no pid, unreadable names), `is_young`, the order of the stop in `owned.lua` (wait, then `jobstop`, then delete) and "leaves a buffer alone that a callback deleted during the wait". - `tests/unit/terminal/snacks_close_spec.lua` covers the Snacks close: it waits before `jobstop` for a job that just started, not for a job older than 10 s, and not for a job that exited. The case "does not let an open during the wait adopt the terminal that is closing" covers the mark clear. - Five Snacks unit specs gain `vim.loop.hrtime` in their vim mock, and one gains `vim.fn.getpid`, because the provider now reads a clock and the wait reads the pid. - `proof-pty-child-hangup.sh` compiles the same shim instead of a copy of it. Under a TMPDIR with a percent sign it failed all three cases, on `main` too. With only a space or only a colon it failed one of three, and its wrapper case passed only because the shim was not loaded. Now `d6019d13` puts its work folder into `/tmp` for such a path. - The header of `hold_pty_child.c` now says that the shim holds every fork of Neovim that calls `signal(SIGCHLD, SIG_DFL)`, including the `ps` children that `nvim_get_proc` starts, not only the pty child. ## Evidence Builds: the official `v0.8.0` release binary (`NVIM v0.8.0-1210-gd367ed9b2`, as in CI) and Neovim 0.12.3, scratch HOME and XDG dirs, on a shared host. | Run | main plus the new tests | this branch | |---|---|---| | integration spec, 10 runs per build | 10 of 10 fail on both builds (the Snacks and `owned.lua` cases fail, the control and native pass) | 0 of 10 fail on both builds | | integration spec, 20 runs per build, plain TMPDIR | not run | 0 of 20 fail on 0.8.0 and on 0.12.3 | | integration spec, 20 runs per build, `TMPDIR="odd tmp 50%"` | the spec of `ea0fbfd6` times out in this TMPDIR, as in CI | 0 of 20 fail on 0.8.0 and on 0.12.3 | | the unit specs of `tests/unit/terminal` plus `native_terminal_toggle_spec`, 20 runs | not run | 0 of 20 fail (374 successes each) | Mutants of the Lua files, one run each, on `89e74e31`. The merge of main changed no terminal file and no terminal spec, so I did not rerun them on the merge commit. "Unit" counts the failing tests among 374 in `tests/unit/terminal` plus `native_terminal_toggle_spec`. "Integration" is the new spec on 0.8.0 and 0.12.3, with the same result on both. "Fact" is the `verify-terminal-lifecycle` fact that turns FALSE in a facts-only run of `check.sh`. | Mutant | Unit | Integration | Fact | |---|---|---|---| | Snacks close does not wait | 2 failures | red | `SnacksCloseClearsMark` | | Snacks close always waits (no age rule) | 1 failure | green (the wait is harmless but slow) | none | | Snacks close never waits (age rule always false) | 2 failures | red | none | | Snacks does not record the start time | 2 failures | red | none | | Snacks clears the mark only for an adopted terminal (the form on `ea0fbfd6`) | 1 error (the open adopts the closing terminal) | green | `SnacksCloseClearsMark` | | Snacks clears the mark after the wait | 1 error (same case) | green | `SnacksCloseClearsMark` | | `stop_buffer` does not wait | 2 failures | red | `OwnedStopOtherStops` | | `stop_buffer` has no `nvim_buf_is_valid(buf)` check before `jobstop` | 1 failure | green | `OwnedStopOtherStops` | | `stop_buffer` re-reads the channel with no check (the form on `ea0fbfd6`) | 1 failure | green | `OwnedStopOtherStops` | | native close does not wait | 3 failures | red | none | | native close never counts a job as young | 3 failures | red | none | | `wait_for_exec` does not call `vim.wait` | 6 failures | red | none | | `wait_for_exec` stops while the names are equal | 4 failures | red | none | | `wait_for_exec` waits 0 ms | 4 failures | red | none | | `wait_for_exec` has no guard for an unreadable name of Neovim | 4 failures | green | none | | `wait_for_exec` calls `jobpid` without `pcall` (no guard for a job without a pid) | 1 failure and 2 errors | green | none | | `is_young` uses `<=` | 2 failures | green | none | | `is_young` reads a `nil` start time as young (no `nil` guard) | 1 failure | green | none | The control mutant (no change) is green in every column, with 374 successes. I ran the facts-only `check.sh` on every row, and "none" means that every fact stayed TRUE. A mutant of `tests/helpers/ld_preload_path.lua` that drops the colon, and one that drops the space, each fail one case of `ld_preload_path_spec.lua`. The model has its own mutants. `check.sh --mutants` removes each guard of the model in turn, and it ends with "every guard is caught" on the merge commit `191a3e24`. The pair `SnacksCloseClearsMark` with `OwnedLiveSkipsEndedJob` is caught by `CloseNeverReuses` and `OpenShowsLiveClaude`. - Gate: `MISE_LOCKFILE=false mise run clean && mise run all` with GNU timeout first on PATH exits 0 with 1925 successes, 0 failures, 0 errors and 38 pending, and luacheck reports 0 warnings in 206 files. The extra pending case is the new integration spec of coder#536, which needs a real Neovim. - `mise run format-check`: 464 files, 0 changed. - Integration runner, with a plain TMPDIR and with `TMPDIR="odd tmp 50%"`: 865 successes, 0 failures, 0 errors on 0.8.0, and 869, 0, 0 on 0.12.3. - `proof-pty-child-hangup.sh` passes 3 of 3 cases on 0.8.0 and on 0.12.3, with a plain TMPDIR, with `TMPDIR="odd tmp 50%"` and (on 0.12.3) with a TMPDIR that holds a colon. On `d6019d13`'s parent, with the odd TMPDIR, it failed 3 of 3 on both builds. - The five `check.sh` scripts (`verify-diff-lifecycle`, `verify-mention-queue`, `verify-selection-tracking`, `verify-terminal-lifecycle` and `verify-ws-connection`) all print PASS. The branch merges `origin/main` (coder#597, coder#594, coder#474, coder#596 and coder#536). coder#536 changed `diff.lua`, its README, `verify-diff-lifecycle` (SKILL.md, `check.sh` and `covered.lock`) and added `diff_answer_every_call_spec.lua`. Git merged the two edits of `verify-diff-lifecycle/covered.lock` with no conflict, and `check.sh` of all five skills prints PASS on the merge result, so no lock needed a new record after `3a1961a0`. - On the merge commit `191a3e24`, the specs this PR adds or changes pass 20 of 20 runs in a plain TMPDIR and in `TMPDIR="odd tmp 50%"` (24 successes for `owned_wait_for_exec_spec`, `snacks_close_spec` and `ld_preload_path_spec`, and 374 for the terminal unit specs plus `native_terminal_toggle_spec`). The integration spec of this PR passes 20 of 20 runs per build in both TMPDIRs. `diff_answer_every_call_spec` from coder#536 passes 10 of 10 runs per build in both TMPDIRs (9 successes each run). ## Not covered - The Snacks case uses a stand-in for Snacks, because CI installs no `snacks.nvim`. The provider code under test is the real one, and the stand-in starts a real pty job with `termopen`. - The wait reads `/proc` or `ps`. Where neither gives a name, the close goes on to `jobstop` and the race stays open, as in native before this PR. A `ps` that hangs would hang the close for as long as it hangs. I did not reproduce that. - I tested the `ps` fallback only on Linux, with `/proc` hidden from Lua. I did not run it on macOS or BSD. - A `jobstop` that other code issues, for example `:bwipeout` of the terminal buffer by the user right after the start, is not covered. - Another PR that edits `native.lua`, `snacks.lua`, `owned.lua` or a `covered.lock` must be merged one at a time with this one, and re-record the lock after its merge. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
Two ways an autocmd could interrupt a native start still broke it after #527, and a third broke a hide later. This PR fixes all three. I found the first two while verifying #525 and #527 (the verifiers of both PRs reproduced them on
main).This is round 7. The maintainer allowed one review round past the cap of six for this push (the waiver). Round 3 of this PR ran the
vsplitunder:noautocmd. Both verifiers of that head (a7b86897) found the same blocker::noautocmdskipsWinLeaveof the window you leave, so aWinLeaveautosave (autocmd WinLeave * silent! update) no longer saved the file before Claude started (7 of 8 cases, on both Neovim builds). Round 4 dropped:noautocmd, so no autocmd is skipped any more. Round 5 (fdb2b082) got two blockers from the verifier (the skill's runner could not run the new replay, and a close right aftertermopenended Neovim in a few runs in 100 under load) and three P2 findings from Codex. Round 6 (a171a86f) fixed both blockers and two of the three P2 findings, and Codex found no major issue.The verifier of
a171a86fthen returned ISSUES, all about the wait that round 6 added toclose_terminal(it askednvim_get_procfor the name of the child, andnvim_get_procrunspson Unix): (1) whereps -pdoes not work, every native close raised after it had cleared its state, so Claude kept running untracked and the next open started a second one. (2) A close from aTermCloseautocmd raisedInvalid 'pid'on a job that had exited, and no test covered the guard. (3) Every close cost 100 to 190 ms (1.1 to 1.3 s for a child namednvim). (4) No test covered the limit of the wait. (5) The docs gave a failure rate that the verifier could not reproduce, and none of them named thepscall or its cost. This push fixes all five (see "The fix" and "Tests").What breaks
vsplitof a start firesWinLeave,WinNewandWinEnter, and creating the buffer firesBufNewandBufAdd, all before the plugin has marked a buffer for Claude. Anopen,simple_toggleorfocus_togglethat such an autocmd calls finds nothing to adopt and starts a second Claude. Nothing tracks it, and a close returns with one still running. (A verifier found theBufNew/BufAddform: 60 of 64 cases onmain.)termopen. Aclose()from aBufEnterorTermOpenautocmd cancels the start with a "Failed to open native terminal" error, and aclose()from aTermOpenautocmd waits for the job insidetermopen. On the 0.8-era build, that wait aborts Neovim (corrupted double-linked list, segfault, exit 139).bufhiddenafter the start (aTermEnterautocmd that setswipe). The next hide closed the window, which wiped the buffer and ended Claude. (The verifier of fix(terminal): keep the native start going when an autocmd closes its split #527 found this; fix(terminal): keep the native start going when an autocmd closes its split #527 setshideagain aftertermopen, but not at the hide.)bufhiddenin a hide firesOptionSet, and an autocmd of that event can end Claude (close()), hide it or show it again. The hide then closedwinid, which wasnil(Invalid 'win') or the window that a nested toggle had shown. This came with the fix of 3 (Codex found it in round 4). Codex then found a second form in round 5: an autocmd shows a file of yours in Claude's window, and the hide closed your window.exec. Ajobstopon it ended Neovim withSIGHUP(exit 1), or left it hanging in the buffer delete. A close that an autocmd of a start asked for (item 2) stops the job right aftertermopen, and so doesopen()followed byclose()in one call, which also does it onmain. On my host it happened in 6 of 1260 runs at a load average of 50 to 100 (7 orders: the three autocmd closes andopen()thenclose(), on both builds). The verifier saw 62 of 1120 at a median load of 105, and 0 of 1120 with the wait.Not in this PR (each is a follow-up after this PR, with its own tracking issue):
WinNew,WinEnterorWinLeavethat closes the new split, or moves to another window, still makes the start take the window you are in (as onmain). The fix needs a proof of which window the plugin made, and it goes with the change that makes the exit close only a window that the plugin made.starting = falseand no marked buffer yet, so it can start a second Claude.mainhas the same gap. The fix needs a start flag that survives a reload, the pending close request, and a model with separate module generations.OptionSethide (on the 0.8-era build, and thatOptionSetruns with Claude's window current).The fix (
lua/claudecode/terminal/native.lua)open_terminalreturns while a start runs (if starting then return end). The start makes the one Claude, so an entry that one of its autocmds calls does nothing. Thevsplitkeeps every autocmd. This fixes 1.close_terminalwaits for the exec, only for a job that was just started.job_started_atrecords whentermopenreturned. A close of a job that this module started less than 10 s ago waits up to 1 s (a terminal adopted after a plugin reload has no start time and does not wait) (checked every 5 ms) until the child of the job has the name of its command, not the name of Neovim, and then callsjobstop. An older job callsjobstopat once, with no lookup. A job that already exited has no pid (jobpidraises), so the wait does not run. The names come from/proc/<pid>/comm(a read of a few bytes) where/procexists. Elsewhere (macOS, the BSDs)nvim_get_procgives them, which runsps, underpcall: when no name can be read (psis missing, or it fails), the close goes on tojobstopas before, with no wait and no raise. This fixes 5, and the five findings of the verifier (thepsraise, the guard, the cost, the limit).close()during a start setsclose_after_start, and the start runs it when it returns. It no longer waits insidetermopen. If the start raises after the job started, the close still runs first, and then the error of the start propagates. This fixes 2.open_terminalis now a small wrapper that callsstart_terminalunderpcall, so an error of the start cannot leavestartingset (which would defer every later close). The old body ofopen_terminalisstart_terminal.hide_terminalsetsbufhidden=hidebefore it closes the window, and only when it is nothidealready, because a write of the same value firesOptionSet. This fixes 3. After the write it looks at the window again, and it returns whenwinidchanged, when its window is gone, when it shows another buffer than Claude's, or when it became the last window. This fixes 4.Behavior effects: no autocmd is skipped. An entry that a start's early autocmd calls does nothing, where it started a second Claude. A
close()that an autocmd of the start calls returns before Claude stops, and Claude stops when the start returns.terminal/README.mdsays so.Tests
29 new tests in
tests/unit/native_terminal_toggle_spec.lua(101 before, 130 now). The mock gets a hook that runs an autocmd inside thevsplit, one that runs it insidenvim_create_buf(BufNew,BufAdd), one that runs anOptionSetautocmd inside a write ofbufhidden, a strictnvim_get_proc(it raises for a pid that is not a number, likeps -pwith a bad pid, and counts its lookups),getpid,vim.loop.hrtime, and a stub of/proc/<pid>/commthat a test can switch off (io.open, set withrawsetas the other specs do). The shared mocktests/mocks/vim.luagetsloop.hrtime.open,simple_toggle,focus_toggle) that aWinLeave/WinNew/WinEnterautocmd of the split or aBufNew/BufAddautocmd of the new buffer calls: one Claude runs, tracked, in one window, with no notice (6 tests).vsplitthat raises does not leavestartingset (1). A start that raises after the job started leaves a Claude that a close still stops (1). A close that an autocmd asked for during such a start still runs before the error propagates (1), and the error of the start is the one that propagates when the close raises too (1).close()from aBufEnterorTermOpenautocmd: Claude stops after the start, with no notice, no window and no buffer left (2). The next open starts Claude again after a close that waited for the start (1).jobstopcomes only after the exec, with the name from/proc(1) and fromnvim_get_proc(1). The wait ends at its limit when the child never runsexec(1). A close where no name can be read (no/proc,psmissing) goes on tojobstopwith no wait and no raise (1). A close of a job that exited (jobpidraises) makes no lookup (1). A close of a job older than 10 s makes no lookup (1). A close that an autocmd asked for during a good start raises the error of that close (1).bufhiddenwas changed towipe: Claude keeps running, and a toggle shows it again (1). A hide that findsbufhiddenalreadyhidewrites nothing (1).OptionSetautocmd ofbufhiddenthat runs inside the hide: it callsclose()(Claude stops, nothing raises), a nestedsimple_toggleorfocus_toggle(Claude stays hidden), hides and shows Claude again (the new window stays), closes Claude's window itself, closes every other window (the window stays), or swaps a file of yours into Claude's window (the window stays and shows your file). That is 7 tests.On
main(5c7a4db9), 20 of the 29 fail (19 failures and 1 error). The other 9 guard state that the fix introduces. I removed each guard in turn (thestartingreturn, settingstarting, resetting it, the deferral, running the deferred close, resettingclose_after_start, thepcall, the hide-timebufhidden, writing it only when it differs, and which error propagates (also when the start went well), the four clauses of the look at the window after the write, the wait for the exec, the condition of that wait, thejobpidguard, the 10 s rule, the limit of the wait (1000 ms to 1 ms), thepcallaroundnvim_get_proc, the read of/proc, and the own name): each removal fails a test.On real Neovim
I ran the verifiers' own probe scripts against this head, the old head
a7b86897andmain, on Neovim 0.12.3 and the 0.8-era build, withset hiddenandset nohidden. Claude is the repo's stubtests/bin/claude.WinLeaveorFocusLost,WinLeave(autosave-probe.lua, 8 cases per configuration): the file is saved before Claude starts in 8 of 8 on every configuration.mainalso saves 8 of 8. The old head left it stale in 7 of 8 (a control run on 0.12.3 withset hidden; the verifiers saw 7 of 8 on every configuration).design-probe.lua: event traces and 7 user setups): the output is byte-identical tomainon both builds (37 lines each). The old head differed in 15 setup rows.start-matrix-d.lua): 7 events (WinNew,WinEnter,WinLeave,BufEnter,BufWinEnter,TermOpen,TermEnter) × 8 actions (focus_toggleonce and twice,simple_toggleonce and twice,close(),ensure_visible(),open(),setlocal bufhidden=wipe) × 4 entries × 2 follow-ups, in 4 configurations: 0 cases with two Claudes, 0 untracked, 0 where a hide ended Claude. I left out the three actions that close or wipe the split (:close,:bwipeout,:bwipeout!): they show the item that is not in this PR (the start takes your window,USER-FILE-GONE), as onmain. On the 0.8-era build the groups of aTermEnterautocmd that hides or closes the window segfault (exit 139). The verifiers showed that onmain, and in plain Neovim 0.8 without this plugin. I see the same exit 139 on this head.BufNewandBufAdd(4 actions × 2 events): 60 cases with two Claudes onmain, 0 on this head.close()from aBufWinEnter,BufEnterorWinLeaveautocmd of the start, andopen()followed byclose()in one call, on both builds,hiddenandnohidden; 12 runs at a time, with the stubclaude): without the wait, 6 of 1260 runs ended withCaught deadly signal 'SIGHUP'(exit 1, at a load average of 50 to 100). With the wait of this head (/proc, young jobs only), 0 of 1750 runs did, and none hung or left a Claude running (36 CPU burners, load average 33 to 88).close-latency.lua, 1 start and 1 close of a settled Claude, real Neovim, load average 20 to 30):main1.7 ms,a171a86f180 to 400 ms, this head 1.0 to 3.8 ms.native-start-reentry.lua(new, inverify-terminal-lifecycle; 25 checks: the 15 entries, the autosave, bothclose()cases, thewipehide, 5OptionSethides that callclose,simple_toggle,focus_toggle, hide and show, or swap a file in, and a close when no process name can be read: no/proc, emptyPATH):NVIM=<nvim> .agents/skills/verify-terminal-lifecycle/replays/run.sh native-start-reentry.luaexits 0 with 0 findings on both builds. The replay shows 23 findings onmain, 21 on the:noautocmdhead (a7b86897), and 1 ona171a86f(the new check: the close raisesENOENTforpsand leaves Claude running). TheOptionSetchecks run after startup (lib.after_startup), because Neovim fires noOptionSetduring its startup.run.shnow waits for a replay that announces this, where it rancquit 2right after the file loaded.At times the host ran at a load average of 50 to 100, and a few matrix groups then died or showed a
DEAD-WINDOW-LEFTflag in a "then exit" follow-up. I reran each of those groups on its own, and none of them showed a flag.verify-terminal-lifecycleThe skill is on
mainnow (#525), so this PR updates it:begin(thevsplitand the new buffer exist, the buffer has no mark) and the stepSplitRuns.AutocmdCallscan run inbegin. A new stepCloseDuringStartrecords a close that an autocmd asked for, andStartReturnsruns it like a typed close. The new factNativeOpenSkipsDuringStartreadsif starting then returnand thestarting = true/starting = falsepair aroundpcall(start_terminal, ...). Without it, the model starts a second Claude and breaksAtMostOneClaude.NativeMarkBeforeTermopenandOwnedUnstartedIsNotEndednow pair with it, because the guard also covers the entry that they covered. The new factNativeRunsDeferredClosereads theclose_requestedblock ofopen_terminal. Without it the request is lost and the Claude that the start made runs on, which breaksCloseStopsClaude. The deferral inM.close(if starting then close_after_start = true) stays a documented limit inSKILL.md: the model has no wait nested in a start, so it cannot show the close that waits insidetermopen.native_terminal_toggle_spec.luaand the replay test that half.start_terminal, andclose_terminalis a forward-declaredfunction close_terminal().check.shpasses (279,492 states, was 263,060), and--mutantsshows every one of the 36 keys caught (26 single, 10 pairs).SKILL.mdhas bug 10 and the replay row, drops the not-modeled item that this change fixes, and lists the wait for the exec as not modeled (the model has no processes).run.shquits with 2, as before, for a replay that raises, one that returns without callinglib.after_startup, and one that hangs for 60 s (I ran all three).verify-terminal-lifecycleandverify-diff-lifecycle; for the diff skill only thefileline ofnative.luachanged).Validation
Run on
ec480c7e, the head of this push (it includesmainat0a4ebbbc, and all four skill locks match):busted tests: 1662 successes, 0 failures, 0 errors (33 pending).mise run check(syntax,luacheck, the static proofs ofverify-claudecode-nvim),stylua --check lua tests,treefmt --ci: clean (luacheck: 0 warnings in 193 files).verify-terminal-lifecycle/check.sh: PASS (279,492 states),--mutants: every guard is caught (36 keys, 26 single and 10 pairs; run onabdb9157, and the merge ofmainsince then changes no file that the skill reads).verify-diff-lifecycle,verify-mention-queueandverify-selection-trackinghash checks: pass.native-start-reentry.lua: 25 checks, 0 findings on Neovim 0.12.3 and on the 0.8-era build.Notes for the reviewer
mainsince30c57e3balso changed other skills, the diff code and its tests, scripts,fixtures/(the issue-218 configs),mise.toml, and docs includingAGENTS.md. fix(terminal): use a cwd folder with $ or \ in its name as written #569 changedlua/claudecode/terminal.lua(the cwd of a terminal) and its lock lines. None of the merges changeslua/claudecode/terminal/.:ClaudeCodeClosefrom such an autocmd returns before Claude stops. It stops when the start returns, in the same call stack, and also when the start raises after the job started.vsplit" proves nothing, a P2 about a close that is deferred past a raised start, then a clean round 3), then the two verifiers blocked on:noautocmd. Round 4 (29345666) got two Codex P2 findings, round 5 (fdb2b082) got two verifier blockers and three Codex P2 findings, round 6 (a171a86f) got a clean Codex round and the verifier ISSUES that this push (round 7) answers.Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh