Repository navigation
fix(diff): hold diff teardowns behind the shared window gate - #511
Conversation
…hen the editor cannot change windows A cleanup that runs in the command-line window (E11) or under text lock (E565) cannot close windows. In the split layout the diffoff raised and stopped the bulk close, so the older openDiff call kept waiting for an answer and the newer diff kept its windows. In the unified layout nothing raised, but the entry left active_diffs while its windows stayed. Hold the whole cleanup, entry registered, until windows can change. The answer to each call is sent before the cleanup, so every call is answered exactly once. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking:
…mmand-line window journey The verify-diff-lifecycle model treated one cleanup as a step that cannot fail halfway, so it could not see a disconnect cleanup that raises in the command-line window. Any teardown in a step that Claude or a scheduled callback starts can now find windows locked: the diff stays registered, the call is answered, and a retry tears it down later. Three code facts (CleanupHoldsWhenLocked, CleanupRetries, RetryChecksEntry) and the liveness rule DeferredCleanupDone keep that guard checked. journey-diff-disconnect-cmdwin.sh drives the same path in a real Neovim with vcn.sh. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking:
# Conflicts: # .agents/skills/verify-claudecode-nvim/SKILL.md # .agents/skills/verify-diff-lifecycle/SKILL.md # lua/claudecode/diff.lua
…he locked-window cleanup _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking:
# Conflicts: # .agents/skills/verify-claudecode-nvim/SKILL.md # .agents/skills/verify-diff-lifecycle/SKILL.md # .agents/skills/verify-diff-lifecycle/covered.lock
_cleanup_diff_state now hands the teardown to utils.when_windows_allowed, the gate the disconnect cleanup already uses, in place of its own probe and a timer per waiting cleanup. The server stop, close_tab, closeAllDiffTabs and a replacing openDiff no longer raise in the command-line window or under textlock: the diff stays registered, its call is answered, and one shared timer closes it once Neovim allows window changes. Many held cleanups share that one timer. A replacing openDiff in those states holds the old diff. Its own setup fails with an error answer, as every openDiff does there. The verify-diff-lifecycle facts now read the gate and the held teardown, and a second journey drives the replacing openDiff in a real Neovim. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking:
…he gated teardown _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking:
|
@codex review |
|
@codex security review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. 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. |
…vim skill (coder#517) ## Why The operator asked for a code-shape refactor of the plugin, landed on main in small PRs, fixes first. His rule for the verification skill: a scratch harness that catches a blocker goes into the repo's skill, and memory points at repo paths. The wire probe that caught the blockers of coder#500 over four review rounds lived only in a scratch folder, so the next agent could not run it. This PR puts it in `.agents/skills/verify-claudecode-nvim`. ## What changed Three new files under `.agents/skills/verify-claudecode-nvim`. No existing file, no `lua/`, `plugin/`, `tests/`, `scripts/` or `.github/` file changes (425 added lines, above the 300-line aim: the launcher, the checker and the page are all in it). - `scripts/proof-tools-call-wire.sh [tree]` is the one command. It starts the plugin server of `tree` (default: this checkout) in one headless nvim and runs the checker. Exit 0 pass, 1 a case failed or a frame leaked, 2 the probe could not run or judged fewer cases than the table holds. - `scripts/proof-tools-call-wire.mjs` holds the case table, the WebSocket client and the checker. It prints one `ok:` or `FAIL:` line per case and ends with `COUNT tools-call-wire cases=<n> failed=<n> leaks=<n>`. - `features/tools-call-wire.md` is the feature page: the command, a pass and a fail, the red proof, the normalization. Rules in the script: - Each case is a row of one table: name, `params`, expected answer. A refusal names the parameter (`-32602`, and the data names it). A normal answer is written out whole. The tools, argument shapes and wrong values are expanded from short lists into rows, and the checker has one path for every row. - Normalization sits next to the table. The leak rule and the token rule read the text of every frame that the server sends as the wire sent it, and no parse or rebuild comes first. One step comes before them: every escaped slash, a run of backslashes and a slash at any depth of nesting, is read as a slash. Then the path rewrite applies to that text: the folder nvim runs in becomes `$CWD`, the plugin tree `$TREE`, the real home folder `$HOME`, in that order. A frame leaks when its text then holds `.lua:` and a digit, `$TREE` or `$HOME`, or 8 or more characters of the auth token in a row. So a path in a sibling field, an object key, a nested list, a duplicate key or a JSON fragment inside a plain string leaks. It also reads a frame that comes before the answer, or within 200 ms after the last answer, and counts a frame for one case only. A frame that arrives after its own answer is judged with the case in flight. The comparison with the table reads a rebuilt copy of the answer that rewrites strings but not keys, so two keys that the rewrite would make equal never pass as a correct answer. The leak rule finds each of the three paths in a frame unchanged, so the probe refuses to run when the tree path, the work folder path or the real home folder path holds a double quote, a backslash or a control character (the characters that JSON writes escaped). It exits 2 with a message that names the path, before it sends a case. A backslash in the tree path can make nvim fail to load the plugin first: the script then exits 2 with `nvim wrote no lock file in 10 s`. The workspace folder is not a leak: `getWorkspaceFolders` returns it on purpose, and its expected answer is `{"folders":[{"name":"work","path":"$CWD","uri":"file://$CWD"}],"rootPath":"$CWD","success":true}`. - A pass needs at least one case and every row judged: an empty table, or fewer judged cases than rows, exits 2 and prints no `COUNT` line. The `cases=` count is the number of cases that the checker judged. - Safe launch: nvim and the checker start with an empty environment. When nvim and node are the binaries themselves, nvim gets only `PATH=/usr/bin:/bin`, `WIRE_PARENT`, `WIRE_TREE`, `WIRE_CWD`, and `HOME`, `TMPDIR`, `XDG_CONFIG_HOME`, `XDG_DATA_HOME`, `XDG_STATE_HOME`, `XDG_CACHE_HOME` and `CLAUDE_CONFIG_DIR` in `/tmp/wire-probe.XXXXXX` (from `mktemp -d`, mode 0700), and the checker gets no variable. A version-manager shim that does start adds variables of its own. nvim runs with `terminal.provider = "none"`. The script exits 2 when a `claude` program is on `PATH=/usr/bin:/bin`, when nvim or node does not start with an empty environment (a version-manager shim does not), and when the temp dir cannot be made. nvim must be a direct child of the script: it checks its parent before the plugin starts, and when the parent is not the script (a wrapper that runs Neovim without `exec`) it quits with an error, so no server starts and no lock file is written. The script then exits 2 with a message that names the wrapper and says to set `VCN_NVIM` to the Neovim binary itself. A wrapper that uses `exec` passes. When the server does not come up for another reason, the script prints the last lines of nvim's output. - The token: it lives in the memory of nvim and of the checker, and in the `0600` lock file in that folder. The checker sends it only in the upgrade header. The checker prints only text that it wrote itself (`stop`) and the text of a frame that passed `show`, which withholds a text that holds 8 or more characters of the token in a row (the output says `<holds the auth token>`). It never prints the message or the stack of an exception that Node or a library raised, because such a message can quote its input (the WebSocket constructor quotes the header value): a frame that is not JSON and a lock file that does not parse end the run with exit 2 and a fixed message, and any other exception ends it with exit 2 and one line that holds only the exception's class name. When the server does not come up, the script prints the last lines of nvim's output. That happens only while no lock file exists, and the plugin logs only the length of its token. On a normal exit, a failed case, TERM, INT and HUP, the script stops nvim and the checker by PID (it polls for their end and sends SIGKILL, and does not `wait` for them) and removes the folder. After SIGKILL no cleanup runs: nvim, a direct child of the script, checks its parent every half second and quits, its server stop removes the lock file and closes the socket, and the checker exits on the closed socket. Both processes and the lock file are gone within 5 seconds (the bound the script holds to; 0.41 s at most in the 10 runs below). The folder stays then, with no token in it. - Why not `vcn.sh launch`: it starts a TUI in agent-tty (agent-tty, Chromium) and puts this checkout on the runtimepath, so it takes no tree argument, which the red proof needs. The probe keeps the skill's conventions: `VCN_NVIM`, node from `mise which node`, and a client that takes the token from the `0600` lock file as `ws-client.mjs` does. It does not call `vcn.sh`, so coder#506 does not change how it runs. ## Cases in this PR, and the cases left out 44 cases: `params` absent, `false`, `null`, `true`, a number, a string, `[]`, `[1]` (all name `params`) and `{}` (names `name`). A non-string `name` (8 values). For `getWorkspaceFolders` and `openFile`: `arguments` absent, `{}` and each of the 7 non-object values, and for `openFile` each wrong type of `filePath` (8 values) plus `""`. Left out, all for size: the other nine tools (`getOpenEditors`, `getCurrentSelection`, `getLatestSelection`, `closeAllDiffTabs`, `getDiagnostics`, `checkDocumentDirty`, `saveDocument`, `close_tab`, `openDiff`) with their string parameters and the `getDiagnostics` `uri`, the `ctl_*` valid-call controls, and the two notification cases of the scratch `wire3.lua`. They are the next PR, which adds table rows and the silent-answer check. The index lines (`SKILL.md` Helpers and `features/README.md`) are not in this PR either, because coder#506 and coder#508 are open and change those files. They go in the next PR. ## How I proved it Neovim 0.12.3 and the official v0.8.0 release, each three runs in a row, from the head of this branch (merged with main `4c1df79`. Main has moved since to `c9e57d13`, which changes nothing in the skill folder or in these three files, so I did not merge it, and a run on a tree of `c9e57d13` printed the same line on both versions): every run printed `COUNT tools-call-wire cases=44 failed=0 leaks=0` and exit 0, and the six outputs are byte-identical. Red proof (`git archive f932d7b`, main before coder#500), exit 1 on both versions: ```text COUNT tools-call-wire cases=44 failed=30 leaks=15 ``` Round 3 (findings from the third review round, my proofs on this head): - A tree path with a double quote (`vim.json.encode` writes `"` as `\"`, so the rewrite to `$TREE` misses it). The mutant that puts the tree path into an object key of an answer, in the folder `/tmp/wp517-q"t/`: the previous head exits 0 with `cases=44 failed=0 leaks=0` on both versions, and this head exits 2 with the message above and no `ok:` line on both versions. The same mutant in a plain path stays red (`failed=2 leaks=2`), and the mutant with the tree path in every refusal gives `failed=31 leaks=31`. The checker alone refuses a quote in the tree, in the work folder and in the home folder, on Node 22.23.4 and 26.12.1. A tree path under the real home folder is read as `$HOME`, so the old gap shows only outside it. A backslash in the tree path makes nvim fail first, and the script exits 2 with `nvim wrote no lock file in 10 s` on both versions. A newline in the tree path does not: the checker refuses it with its own message (exit 2, both versions). - The token in an exception of Node: a lock file that is valid JSON whose token holds a line break makes the WebSocket constructor throw a message that quotes the header value. The previous head printed 26 eight-character runs of such a value on stderr (my mutant's value has 33 characters, one more than a real token of 32 characters, which has 25 such runs). This head exits 2 with `stopped by an unexpected TypeError`, 0 runs on stdout and stderr, on both versions. - A `cd` into the new temp dir that fails (a stand-in `mktemp` that prints a dir that does not exist, or a dir with mode 000): this head names the dir that `mktemp` printed in both cases, and leaves 0 dirs. The previous head named an empty path in both, and left 1 dir in the mode 000 case. - A HUP, TERM or INT that came just after the checker exited could hang the launcher in its cleanup, with nvim and its lock file alive until the launcher was killed (`wait <pid>` blocked on a child that was already gone: 3 hangs in 118 signals sent right after the `COUNT` line, all three HUP on 0.12.3). The cleanup no longer calls `wait`. My 100 TERM trials on 0.8.0, each right after the `COUNT` line, hung 0 times (my log keeps no exit code, so it does not show that the trap ran in each one, and I count none of my HUP or INT trials after the change). Two independent verifier runs on this head: 0 hangs in 130 and in 120 signals. With `wait` back in the cleanup: 1 hang in 130 and 1 in 60. I did not find the cause inside bash. - A server that died in the middle of a run could print `WebSocket handshake failed`. The checker now drops that handler when the connection opens, and prints `the server closed the connection` on both versions (SIGKILL of nvim, exit 2, no process left, no temp dir). - The same runs on this head as before: green three times on each version with `cases=44 failed=0 leaks=0` and byte-identical outputs, red proof `failed=30 leaks=15` on both, wrapper without `exec` exit 2 and with `exec` pass (3 runs each, both versions), SIGKILL of the script (5 runs on each version, 0.41 s at most), 12 signal trials (TERM, INT and HUP, each between the lock file and the end of the run and right after the `COUNT` line, both versions: exit 143, 130, 129 or 0, no process, no temp dir left). Findings of rounds 1 and 2 and their proofs (copies, not committed): - Round 2, the leak rule reads the text of each frame as the wire sent it. A leaking value behind a key collision (mutant `mE`: one key is the work folder path and holds `/virtual/probe.lua:42`, one key is `$CWD` and holds `safe`): `failed=41 leaks=41` on both versions, and the previous head printed `failed=0 leaks=0`. A duplicate key in the text item of a correct result (the real home path in the first, so the parser keeps the last): `failed=2 leaks=2`, and the previous head passed on both versions. A path in a JSON fragment inside a plain string (the slashes are `\/` on Neovim 0.8): `failed=31 leaks=31` on both versions, and the previous head passed on 0.8. The sibling-field, object-key, nested-array and late-frame mutants of round 1 stay red (`failed=2 leaks=2` for the first three, `failed=1 leaks=2` for the late frame). - Round 2, a server frame that is `x` followed by the token (not JSON): exit 2 and one fixed message, and 0 matches of any 8-character run of the token on stdout and stderr, on both versions. The previous head printed 2 such matches on stderr. A lock file whose token value is not valid JSON, with a made-up value (not a token): 0 matches of any 8-character run of the value on stdout and stderr, and 3 on the previous head. The token in an error answer (mutant `mT`: the token as a field `t` of every `-32602` error object): `failed=41 leaks=41` and 0 matches. - Round 2, a wrapper that runs Neovim without `exec` (`VCN_NVIM` is a script that runs the real binary as a child): three runs on each version exit 2 with the message, print no `ok:` line, and leave no process and no temp dir. A wrapper with `exec`: three runs on each version pass with `cases=44 failed=0 leaks=0`. - Round 2, a leak in the answer of the last case: `failed=1 leaks=1` and one `FAIL` line, where the previous head printed `failed=2` and a second line for the same frame. A failing `mktemp` or `mkdir`: exit 2 and a message, where the previous head exited 1. Round 1: - A path in a sibling field of a correct result (`result.debugPath = "/virtual/probe.lua:42"`): `failed=2 leaks=2` on both versions. Before, the probe printed `leaks=0` and exit 0. The tree path as an object key of an answer: `failed=2 leaks=2`, and the same mutant passes when the checker does not rewrite keys. An extra server frame 100 ms after each valid answer, which only the read after the last case sees: `failed=1 leaks=2`. - The auth token in an error answer: `failed=31 leaks=31`, no `FAIL` line printed the token, and the match count of the token in stdout, stderr and nvim's output file is 0. - SIGKILL of the script after the checker connected: five runs on each Neovim version, both processes (recorded by PID) gone after 0.2 to 0.4 s, lock file gone after 0.2 to 0.4 s, 0 files with the token in the folder. The launcher of round 1, same test: both processes still up after 5 s, the lock file still there, 1 file with the token. A server killed in the middle of a run ends the checker with exit 2 and one message. - An empty table: exit 2, one message, no `COUNT` line. A loop that judges no case, or skips one row: exit 2, no `COUNT` line. - A version-manager shim as `VCN_NVIM`: exit 2 and a message that names it and says to set `VCN_NVIM` to a real binary. A node that does not start: exit 2 and a message that names it. A tree whose `require` fails: the last lines of nvim's output and exit 2. In a fresh clone of the branch on this host, which mise does not trust, the page's commands failed with exit 2 and that message until `VCN_NVIM` named a real binary. With it, the whole proof exits 0, the red proof exits 1 with `failed=30 leaks=15`, and the Neovim 0.8 run prints the same lines as the 0.12.3 run. - The real home path in an answer, with the caller's `HOME=/tmp`: `failed=31 leaks=31`. The previous head printed `failed=0 leaks=0` there. A module named in the caller's `NODE_OPTIONS` ran inside the checker of the previous head (1 marker line) and does not run now (0). - nvim's own temp root: in a fresh `/tmp` (a private tmpfs), the previous head left `/tmp/nvim.coder` and this head leaves nothing, on both versions. Controls that must go red (copies, not committed): main with the entry error holding the tree path, or the real home path: `failed=31 leaks=31` each. The checker without the `$CWD` rewrite: `failed=2 leaks=0`. A checker whose home folder is `/tmp`, so that the run dir lies under it: with the `$CWD` rewrite first it prints `failed=0 leaks=0`, and with the `$CWD` rewrite after the `$HOME` one `failed=2 leaks=2`. A wrong expected `rootPath`: `failed=2`. A wrong named parameter for `filePath`: `failed=8`. Other checks: - Token: match counts of the token in stdout, stderr and nvim's output file, and in the command line and environment of the child processes: 0 each. The token has 32 characters, and none of it was printed. - A `claude` on the stripped PATH (a stand-in that writes a marker file): exit 2, the marker was never written, no temp dir was made. - TERM, INT and HUP, each sent between the lock file and the end of the run and right after the `COUNT` line, left no process and no temp dir (12 trials, see round 3). The cleanup ignores TERM, INT and HUP while it runs. - Missing tree, a tree without `lua/claudecode`, a missing nvim, an nvim that exits at once, and a tree whose `require` fails: exit 2 with a message each. - Node 22.23.4 and 26.12.1 both ran the whole probe on this head (pass on both Neovim versions, and `failed=30 leaks=15` on the red tree with Node 22). - Local merges of this head with coder#523 (`eb16b3aa`), coder#508 (`17252700`), coder#506 (`ab9d431c`) and coder#494 (`c219b155`), never pushed: no file of this PR changes, and each ran `cases=44 failed=0 leaks=0` on both versions. coder#511 (`ef610b7d`) and coder#485 (`d00e1be2`) conflict with current main in `covered.lock` on their own, and their trees do not hold coder#500 (the probe prints `failed=30 leaks=15` on them), so a merge check says nothing about these three files. Neither changes a path of this PR. - A fresh clone of this head, the page's commands word for word: the whole proof exits 0 with `failed=0 leaks=0`, the red proof exits 1 with `failed=30 leaks=15`, and Neovim 0.8 prints the same line. Without `VCN_NVIM` the clone (which mise does not trust) exits 2 and names the shim. Gates on this head: `mise run format-check` 351 files 0 changed, `mise run check` 0 warnings / 0 errors in 176 files, `mise run test` native exit 0 with 1586 successes / 0 failures / 0 errors / 22 pending (seed 1794431103), and `bash .agents/skills/verify-diff-lifecycle/check.sh --hash-only` prints `covered.lock matches the code`. ## Limits - Only `getWorkspaceFolders` and `openFile` are in the table. The feature page says so. - The run dir is `/tmp/wire-probe.XXXXXX`. The probe ignores `TMPDIR`. - `vcn.sh` has the same `bin()` and then `env -i` start for nvim, so a version-manager shim fails there too. This PR does not change `vcn.sh`, because coder#506 rewrites it. - A frame that comes more than 200 ms after the last answer is not read. - The probe has no CI job. It needs nvim and Node, and CI runs neither the harness nor this probe. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
… mode (coder#573) ## Why coder#494 ended diff mode in windows that show the file after a split diff closes. A window can have diff mode again after the diff because the plugin ran `diffthis` and then split. This PR removes that cause in both open paths: the split comes first. Refs coder#494. ## Cause Neovim saves the options of a window, 'diff' included, for each buffer that leaves it, and sets them again when the buffer enters the window. A split copies the options of its source window. Both open paths ran `diffthis` in the original window and then split it. The proposal window copied diff mode and saved it for the file when it showed the proposal. So `:edit <file>` back in that window turned diff mode on after the diff had ended. A window in a new tab page (`:tabnew <file>`, `:tabnew` and `:buffer <file>`, `:tab sbuffer`) took that saved set too, while the diff was open. ## What changed All code is in `lua/claudecode/diff.lua`. - `setup_new_buffer` and `show_native_diff` (the legacy native path, `diff.open_diff`): split first, run `diffthis` in the proposal window, then in the original window. The proposal window saves no diff mode for the file. - The original window gets its `diffthis` in `nvim_win_call`. A switch to that window and back fired 8 more `WinLeave`, `WinEnter`, `BufLeave` and `BufEnter` events than on main (12 against 4 in a probe). A `BufEnter` handler that moved the focus then put diff mode in the user's other window (0.12.3) or made `openDiff` fail (0.8.0). With `nvim_win_call` the events are the same as on main (probe below). - `lua/claudecode/README.md` and `.agents/skills/verify-claudecode-nvim/features/open-diff.md`: describe the split order. - `tests/mocks/vim.lua`: the `nvim_win_call` mock accepts every window id, as `nvim_set_current_win` does there. With main's stricter mock, one unit spec fails (`tests/unit/new_file_reject_then_reopen_spec.lua`, the blocking path). The mock now checks less than real Neovim, which rejects an invalid window id. The integration specs call the real `nvim_win_call` on every open. - Two specs call `tools.setup()` instead of `tools.register_all()`. `setup` stores the server and then calls `register_all`, so the tests behave the same. - One behavior differs from main on purpose: when a user's `WinNew`, `WinEnter`, `BufWinEnter` or `FileType` handler closes the original window while the diff opens, this head answers `openDiff` with an "Invalid window id" error, restores the layout and fires no `ClaudeCodeDiffOpened`. Main opens a diff that has only the proposal window, and in two of those handler kinds leaves the file window in diff mode after close. - `covered.lock` is re-recorded in its own commit. Only the hash of `diff.lua` changed. No block that the model cites changed, and `check.sh` passes with TLC. `git diff origin/main...HEAD -- lua/` touches `setup_new_buffer` and `show_native_diff` and one bullet of the README. Nothing else in `lua/` changes. The sweep (`end_diff_mode_in_windows_that_show_the_file`, `end_diff_mode_if_alone`) and the delete block of `_cleanup_diff_state` stay as on main. ## What stays as on main These are not part of this PR. An earlier head of this PR changed them. Their fixes will ship in follow-up PRs in this run. - A handler of the user that sets diff state while the proposal buffer is deleted (after `:b <proposal>` and `close_tab`): the sweep resets it, as on main. Follow-up: handler state during proposal delete. - A user's `:diffoff | diffthis` in a `:split` copy, and a `:tab split` copy in another tab page: main's sweep ends the first and leaves the second. Follow-up: user diff mode in split copies. - Two diffs of one file: closing the first can end the second diff's diff mode or close its window. Follow-up: two diffs of one file. These stay as on main, because Neovim saves or copies the window's options. No PR in this run changes them. The `close_tab` error in the last item belongs in main's close path (coder#511). - `:edit third.txt` in the original window, then `:edit #`: Neovim saved diff mode for the file in that window, so it comes back. - `:split <file>` from a window that left the file: the new window takes the saved set. - A `ClaudeCodeDiffClosed` handler that brings the file back into a window. - When Neovim refuses `:diffoff` in the command-line window, the window keeps its diff mode. `close_tab` from the command-line window answers an error on both Neovim versions (single plain diff, probed on main). ## Before and after Real Neovim, the plugin's own `openDiff` and `close_tab` handlers, on 0.12.3 and v0.8.0, main against this branch: | Scenario | main | this branch | | --- | --- | --- | | 1. proposal window `:edit other.txt`, `close_tab`, `:edit <file>` | `diff=true` | `diff=false` | | 1b. proposal window `:edit other.txt`, then `:edit <file>`, in the legacy `diff.open_diff` | `diff=true` | `diff=false` | | 1c. `:tabnew <file>`, `:tabnew` + `:buffer`, `:tab sbuffer` while the diff is open | new tab window `diff=true` | new tab window `diff=false` | ## Validation Head c67c231 (the fix, eight merges of main, the last one of `c7e2eb4e`, the lock re-recorded in its own commit, and the spec and mock commits). - `mise run clean && mise run all`: exit 0. luacheck 0 warnings, 0 errors in 197 files. busted 1892 successes, 0 failures, 0 errors, 35 pending. - Specs (`diff_left_window_spec.lua` 40 cases, `diff_close_save_spec.lua` 77 cases): 0 fail on 0.12.3 and v0.8.0. Against main's `diff.lua`, 6 cases fail on both versions: the proposal-window `:edit` case (1), the legacy case (1b), the three new-tab cases (1c), and the `:edit #` row of `diff_close_save_spec.lua`. 32 of coder#494's 33 rows are unchanged and pass. The 33rd, the proposal-window `:edit` row, expects `diff=false` now. Three cases are parity cases (green on main too): a `BufEnter` handler that moves the focus while the diff opens, and no `WinEnter` for the original window while each open path runs (blocking and legacy). - Integration runner with a stub `claude` first on PATH, normal and under a `TMPDIR` with a space and a `%`: 0.12.3 838 successes and v0.8.0 820, 0 failed, 0 errors, 37 files each. All four runs (two versions, two `TMPDIR` shapes) passed on the first attempt. The cache folder stays untouched. - Mutants: 6 that revert each kept step (the split before `diffthis` in each open path, the original window's `diffthis` in each path, and `nvim_win_call` instead of a window switch in each path). All red on both versions, and the unmutated baseline is green. They ran on `24468969`, whose `diff.lua` is the same as this head's. - `check.sh` (TLC): pass, 93477 states for the main model and 5557 for the live model. I did not run `check.sh --mutants`. - Probes of the verifier of an earlier head, run live (`OptionSet` fires), main against this branch, on 0.12.3 and v0.8.0: - A `BufEnter` or `BufWinEnter` handler that raises while the proposal buffer is deleted (9 scenarios): 126 checks, 0 fail here and pass on main. The answers are the same as on main. - The command-line window set (13 scenarios): 80 checks, 0 fail here and pass on main, 3 that fail on main pass here. A `close_tab` that arrives while the command-line window is open does not close a diff on main either: in these sequences main answers `TAB_CLOSED` but leaves the diff's tab on screen, untracked, and a single plain diff fails on both versions. With `close_tab` delivered the way the server delivers it (from `vim.schedule`), this head answers `TAB_CLOSED` in the author's sequence on both versions. In a shorter sequence it answers an `E11` error and keeps the diff on screen and registered. The fix belongs in main's close path, which open PR coder#511 ("hold every diff teardown behind the shared window gate") changes. - A user handler that moves the focus while the diff opens (20 rows, both open paths, four events): events fired are the same as on main (4 or 5, against 12 with a window switch). 6 rows end differently from main, none worse: in each the handler moves the focus at the first window entry, and main leaves a stray `:split` copy in diff mode that this branch does not. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
Conflicts: lua/claudecode/diff.lua (main's partner-window and new-tab release helpers kept, the gate around teardown_diff kept), lua/claudecode/README.md, the verify-diff-lifecycle model, check.sh and SKILL.md files, the verify-claudecode-nvim SKILL.md, and covered.lock (main's copy, re-recorded in the next commit). The replace loop of open_diff_blocking no longer asserts that the replaced entry is gone: while Neovim refuses window changes the gate holds that teardown, so the loop leaves instead. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking:
…it in the verify-diff-lifecycle notes _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking:
… merging main _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking:
…e that window is closed The case pinned what main did before the window gate: close_tab in the command-line window dropped the entry and left the split copy in diff mode, because Neovim refused diffoff. The cleanup now waits for the gate. The case keeps its assertions that close_tab does not raise and answers TAB_CLOSED, and now expects the diff to stay registered while the window is open, then to close with diff mode ended in the copy. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking:
…he edited diff.lua comments _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking:
…line window journeys Main's static proof no-source-run-env refuses a journey that sources run.env. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking:
The guard said the unified layout needs Neovim 0.9. Main dropped that claim in #564, and the 24 cases pass on v0.8.0. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking:
|
@codex review |
|
@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. |
… for the window gate While Neovim refuses window changes, the gate keeps the old diff registered. The new call used to run its setup then, and the setup's failure handlers took the old entry for their own diff. They held the old teardown a second time and left what the setup made: a window that shows the command-line window on Neovim 0.8, a hidden proposal buffer, and a buffer name that made every later openDiff of that tab name fail with E95. The call now fails with -32000 before any setup, and the held teardown closes the old diff once windows can change. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
…he replace loop's error The replace loop of open_diff_blocking now fails the call where it used to break. The model's facts (ReplaceRejects, ReplaceKeepsOthersSaved) still match the code. The model never lets a replacing openDiff find windows locked, and SKILL.md now says what the code does there. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
…that no longer appears The replacing openDiff in the command-line window now fails before its setup and logs nothing, so Neovim shows no Press ENTER prompt. The old Enter keystroke ran the command line and closed the window, so the journey failed at its next check. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ 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. |
Why
Neovim refuses window changes in two states: while the user is in the command-line window (
q:, E11) and under textlock (E565, for example while an<expr>mapping waits for a key). #499 holds the disconnect cleanup in those states. Five other paths still tear a diff down at that moment and raise::ClaudeCodeStop, or the plugin stops itself),close_tab,closeAllDiffTabs,openDiffwith thetab_nameof an open diff (a replacing openDiff),What the user sees on
mainatd8e767c(#499):stopraises E11 or E565 before the TCP teardown, a pending openDiff call is never answered, the server keeps running, and the next start says "Already running". A replacing openDiff raises indiffoff, and the old rejected diff stays on screen after the user leaves the window. The unified layout is worse: nothing raises, but the registry entry is dropped while its windows stay, so nothing can close them.mainhas moved since (700209ee, with #485 and #536), and a replacing openDiff in the command-line window now behaves differently there. On Neovim 0.12.3 it no longer raises indiffoff. It drops the registry entry and leaves the old windows, and the new call gets "Diff setup failed". On v0.8.0 the cleanup still raises E11, and the old rejected diff stays registered and on screen. This PR'sdiff_cleanup_lockedspec fails 15 of 24 cases on700209eewith 0.12.3 and 14 of 24 with v0.8.0.What changed
_cleanup_diff_statenow hands the teardown toutils.when_windows_allowed, the gate from fix(server): hold the disconnect cleanup until Neovim allows window changes #499. The old body isteardown_diff, and only the gate calls it. When Neovim allows window changes, it runs at once, as before. Otherwise the diff stays registered and the gate's one 50 ms timer runs the teardown later.active_diffs[tab_name]still holds the entry that waited, because a new diff can take the tab name before the timer fires.close_tab,closeAllDiffTabsand the bulk closes answer every call at once.-32000("Diff setup failed") and runs no setup. A waiting old call getsDIFF_REJECTED(an old diff the user already accepted keeps itsFILE_SAVEDanswer), and the held teardown closes the old diff after the user leaves the locked state..agents/skills/verify-diff-lifecycle: the model has a lock on window changes. Three facts (CleanupHoldsWhenLocked,CleanupRetries,RetryChecksEntry) read the gate andteardown_diff, and the liveness ruleDeferredCleanupDonesays a held cleanup runs once windows can change. After the merge the model also holds fix(diff): answer a call that a close handler lets register while a diff is replaced #536's reject pass and exit cleanup, about 256,000 states..agents/skills/verify-claudecode-nvim: two journeys,journey-diff-disconnect-cmdwin.shandjourney-diff-replace-cmdwin.sh, drive the paths in a real Neovim terminal with the scripted Claude client.Merge of main
This head merges
origin/mainat700209ee(#485 is in) into the branch with a normal merge commit (9baedc48). No rebase, no force push. Eight files conflicted. I kept main's code in each one and put this PR's gate around it.lua/claudecode/diff.luaend_diff_mode_if_alone,end_diff_mode_in_windows_that_show_the_file,release_surviving_diff_taband thediffoffchanges.teardown_diffnow holds main's whole body, including the new-tab release from #485._cleanup_diff_statestill calls the gate. The replace loop of #536 asserted that the old entry is gone after_cleanup_diff_state. With the gate the entry can stay while windows are locked. The merge turned the assert into abreak, and a verifier found that this was wrong (see "Finding F1" below).ebff0dc2replaced thebreakwith the call that fails at once.lua/claudecode/README.md:diffoffnow says the cleanup waits for the gate.verify-diff-lifecycle/model/DiffLifecycle.tlaRejectEach,DiffCleanupAtExit,ExitInSetupand the exit constants, and this PR'sDeferredCleanupRuns.Nextlists all of them.verify-diff-lifecycle/check.shExitedCallsAnswered. The live properties areDisconnectedCallsAnsweredandDeferredCleanupDone.verify-diff-lifecycle/model/statechart-view.tsandSKILL.mdverify-claudecode-nvim/SKILL.mdcovered.lockofverify-diff-lifecyclecame from main and I re-recorded it in its own commit. It was the only STALE lock of the five. Two more re-records followed, one for a comment-only edit ofdiff.luaand one for the replace-loop fix. The model's facts still match the code, and the model never lets a replacing openDiff find windows locked (itsSKILL.mdsays what the code does there).Every teardown path that main added since
ef610b7d:release_surviving_diff_tab)teardown_diffcalls it, directly in the split layout and throughcleanup_inline_diffin the unified layout, and nothing else calls it._close_proposal_window,end_diff_mode_in_windows_that_show_the_file)teardown_diff.cleanup_at_exit)keep_empty) calls_cleanup_diff_state. The cleanup pass calls_cleanup_all_active_diffs. Both go through the gate.open_diff_blocking_cleanup_diff_state, so it is gated. When the gate holds the old diff, the loop fails the call before any setup.remove_failed_proposalactive_diffs[tab_name]. The merge'sbreakbroke that assumption (F1). The replace loop now fails the call before its setup runs, so these handlers see only an entry that the setup registered, or none.Finding F1 (found by a verifier on
0b34b13c, fixed inebff0dc2)The merge turned #536's assert into a
break. A replacing openDiff B then ran its setup while the old diff A was still registered under the sametab_name. When that setup failed in a locked state, its failure handlers saw A's entry. They held A's teardown a second time and skipped B's own cleanup. What B's setup had made stayed behind:[Command Line]buffer after the user leavesq:,:qastops with E37 on v0.8.0,It needs a second openDiff with the
tab_nameof an open diff while the user is inq:or under textlock. PROTOCOL.md says Claude Code sends a new randomtab_nameper call, so only other clients that reuse atab_namereach it.Fix: when the gate kept A's entry, the replace loop fails B's call with
-32000("Diff setup failed") before any setup. I used this form and not the verifier's alternative (the failure handlers compare the entry with the one registered before the setup). Every setup fails in those states anyway, so the first form runs nothing that would need undoing. It changes one place, where the alternative changes the failure handlers indiff.luaand indiff_inline.luaand still runs a setup that cannot work. The loop holds A's teardown once. B's setup used to hold it a second time.Known limit
close_tabof a diff the user already saved still raises E11 in those states. The file reload that runs 100 ms after the close (reload_file_buffers_manualfromclose_diff_by_tab_name) does not go through the gate yet. I re-ran it on this head on 0.12.3 and on v0.8.0, in the command-line window (E11) and under textlock (E565). The reload raises fromedit, and the file's buffer keeps its old text, because the reload never runs. The diff itself closes after the lock.mainis no better: itsclose_tabitself fails in those states, the diff stays registered, and the reload raises the same way. A follow-up PR routes the reload through the gate.A replacing openDiff in those states holds the old diff, and the new call fails with error
-32000("Diff setup failed") before it sets anything up, so it leaves no window or buffer. The old diff closes once the user leaves the window. Holding the new openDiff itself is not part of this PR.Tests
I used the
test-auditauthoring gate,principle-test-behavior-not-implementationandtdd(failing check before the fix, then pass after) for every test change after the merge:diff_replace_locked_spec.lua(24 cases on each build) drives a replacing openDiff throughtools.handle_invokein the command-line window and under textlock, in the split and unified layouts, with the old and the new file of different kinds (existing and new, both orders), while the user is in another tab. Per combination it checks three behaviors. The old call getsDIFF_REJECTEDonce, the new call gets one-32000answer, and the old diff stays registered while locked. After the lock the old diff closes, no window shows[Command Line], no proposal buffer is left and no window is left over. A later openDiff with the same tab name and the new call's kind opens and is answered once. On0b34b13c10 of the 24 cases fail on both builds (the command-line window, the leftover proposal buffer, and the later openDiff). With the fix all 24 pass on 0.12.3 and on v0.8.0.diff_left_window_spec.lua, case "answers close_tab from the command-line window...": it pinned the pre-gate behavior (the copy keeps its diff mode). It now expects the diff to stay registered while the window is open, to close after the window closes, and to end diff mode in the copy afterwards. Theno gatemutant turns it red.diff_locked_paths_spec.lua, replacing openDiff cases: the old assertion looked for"error"in the reply. The mutant that restores fix(diff): answer a call that a close handler lets register while a diff is replaced #536's "entry is gone" assert in the replace loop survived it (0 red). The assertion now looks for "Diff setup failed", and the same mutant turns 2 cases red (command-line window and textlock).diff_cleanup_locked_spec.lua: I removed its skip of the unified layout below Neovim 0.9. Main dropped that claim in fix(diff): say the unified layout runs on Neovim 0.8.0, and run its specs there #564, so all 24 cases now run on v0.8.0.diff_locked_paths_spec.luacountsvim.defer_fncalls and usesnvim_replace_termcodesthroughtests/helpers/window_lock.lua, because v0.8.0 has novim.keycodeanduv.walkcrashes there.journey-diff-replace-cmdwin.shno longer sends Enter at a "Press ENTER" prompt. The refused call now logs nothing, and Enter would run the command line and close the window.Verification
On
d8e767cI replayed each path over a real socket in headless Neovim 0.12.3 (the textlock cases usegetchar()in a typed<expr>mapping). Result after the change:mainstopraises E11, server keeps running, restart says "Already running"diffoff, old rejected diff staysclose_pending_diffsraises E11 after the firstReplacing openDiff, all combinations (a verifier's matrix, rerun by me on this fix and on
700209ee, headless, one fresh Neovim per row): 288 scenarios on each of 0.12.3 and v0.8.0, 576 rows. Scenario axes are layout (vertical, horizontal, unified), new-tab diff or not, lock (q:, textlock, none), user in the same or another tab, old and new file existing or new, and the old diff pending or saved. Each row also opens a later openDiff, accepts it and closes its tab.700209ee, and 376 rows are strictly better. The 384 rows with a lock have no leftover. The other 192 rows have no lock and give the same result line as700209eein every row.0b34b13cis worse than700209eein 40 rows (a row can count in more than one: 24 with a later openDiff that fails, 16 with a leftover buffer, 8 with an extra window), and none of them is worse on this head.vim.schedule(the way a socket message arrives), the result is the same: 0 worse rows, 376 better.700209eeand on this head in every run. The locked close-handler orders (448 runs): 0 problems on this head, 358 on700209ee, none worse.0b34b13cand opens both on this head, as700209eedoes, on 0.12.3 and on v0.8.0.Gates on the merged head
74bbd458, one run each, none passed only on retry:mise run cleanandmise run all: exit 0, 2026 successes, 0 failures, 0 errors, 54 pending, luacheck 0 warnings in 227 files, and all 5 static proofs ok. The new spec is one more pending case under busted, because it needs a real Neovim.diff_cleanup_locked24 cases,diff_locked_paths7,diff_disconnect_window_lock2,diff_new_tab_autocmds14,diff_new_tab_close_mode10,diff_new_tab_last_tab12,diff_new_tab_mode12,utils_without_autocmds4,diff_answer_every_call9,diff_close_save77,diff_left_window40 anddiff_replace_locked24. All 0 failures and 0 errors. The first two and the last are this PR's specs. The other nine cover the new-tab, answer-every-call, close-save and left-window teardown code that main changed, and the disconnect window lock from fix(server): hold the disconnect cleanup until Neovim allows window changes #499.scripts/run_integration_tests_individually.shwith the stubclaudefirst onPATH: 1161 successes on 0.12.3 and 1157 on v0.8.0, 0 failures, 0 errors, exit 0. Each ran twice, once with a plainTMPDIRand once with aTMPDIRthat holds a space and a percent sign.check.shof all five skills: PASS. States:verify-diff-lifecycle255,881 (the two live properties 13,509 each),verify-mention-queue540,240,verify-selection-tracking95,938,verify-terminal-lifecycle279,492,verify-ws-connection262,285. The three fact mutants of the merged model fail as expected (run before the F1 fix, which does not touch the model):CleanupHoldsWhenLocked=FALSEbreaksNoRejectedLeftover,CleanupRetries=FALSEbreaksDeferredCleanupDone, andRetryChecksEntry=FALSEbreaksNoStrandedCall.journey-diff-disconnect-cmdwin.shandjourney-diff-replace-cmdwin.shboth print PASS on this head in a real Neovim terminal, andvcn.sh cleanupleaves no process. They readOUTandWORKthroughvcn.sh get, because main'sno-source-run-envstatic proof refused thesourceofrun.env.diff_cleanup_locked,diff_locked_paths,diff_disconnect_window_lock,diff_left_window,diff_replace_locked). The count is red cases on 0.12.3 and on v0.8.0, and a no-op control is green on both:_cleanup_diff_statecallsteardown_diffdirectly): 38 and 35,active_diffs[tab_name] == diff_datatest): 12 and 12,when_windows_allowedarms avim.defer_fnon every call): 3 and 3,windows_allowedreturns true): 40 and 37,diff_replace_locked,diff_locked_paths.The counts depend on the exact edit, so they are not comparable with the counts of an earlier head.
d8e767c: 6 of the 7diff_locked_paths_spec.luacases and 16 of the 24diff_cleanup_locked_spec.luacases fail there.journey-diff-replace-cmdwin.shfails there at thediffoffstep. On700209eethis PR's specs fail 15 and 14 of the 24diff_cleanup_lockedcases (0.12.3 and v0.8.0), 6 of the 7diff_locked_pathscases on both, and 16 and 14 of the 24diff_replace_lockedcases.Refs #483, #485, #499. #485 is merged, and this PR resolves the overlap in
_cleanup_diff_state.Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh