Repository navigation
fix(diff): end diff mode in windows that show the file after a split diff closes - #494
Conversation
… comes back Neovim saves a window's options for a buffer when the buffer leaves the window, and sets them again when the buffer returns. A diff window that left the file's buffer while the diff had it in diff mode got diff mode back at that moment, after the diff ended: `:e third.txt` then `:e #` in the original window, `:e other.txt` in the proposal window then `:e <file>`, or `:b <proposal>` in the original window, where the deleted proposal buffer hands the window back to the file. The next openDiff then saw a window in diff mode and opened in a new tab. The cleanup now waits for the file in each window of the diff that does not show it, and ends diff mode there once. Refs #491, #483. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
… the diff journey The journey now runs three more steps for an existing file: `:e other.txt` then `:e hello.txt` in the proposal window, `:e other.txt` then `:e #` in the original window, and `:b proposed` in the original window. Each checks that diff mode stays off. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
…f check `diffoff` already does nothing in a window that is not in diff mode, so the check before it ended nothing. The spec also checks the options that diff mode changed in the proposal window, and the comments say less. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
|
@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 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: 05d647110f
ℹ️ 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".
…p one waiting autocmd per file The message of 05d6471 said that `diffoff` does nothing in a window that is not in diff mode. That is wrong: `diffoff` sets 'foldmethod' and 'scrollbind' back to their values from before the diff even when 'diff' is already off. The check on 'diff' is back, so options that the user set after turning diff mode off stay. - The autocmd runs `diffoff` only in the window the event fired for. A waiting window that got the file without a BufWinEnter event (`:noautocmd buffer`, a non-nested autocmd) is dropped without `diffoff`, because the user can have turned diff mode on there since, and a later `:tabnew <file>` no longer ends it. - One BufWinEnter autocmd serves a file's buffer however many diffs leave windows behind. Closed windows and wiped buffers are forgotten when the next diff arms. - README: one statement for what `:edit` does to diff mode, and what the cleanup resets. - Specs: the cases that failed on 05d6471 (own `:diffoff`, both orders of the entry without an event, one autocmd per file, the registry after a wipe) and the cases that keep the autocmd from running `diffoff!`, from ending a diff in another tab, and from acting in a window that is not the entered one. Refs #491, #483. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Bravo. 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. |
…ew windows a saved set without diff mode Round 2 said that a waiting window that got the file without a BufWinEnter event is dropped without `diffoff`. That holds only when the file next enters another window with an event. If the user starts a diff in that window and then leaves the file or reloads it there, the waiting autocmd still ended the user's diff. - The waiting autocmd listens to BufLeave and BufUnload of the file as well as BufWinEnter. Any of them shows that a waiting window already has the file, so the window stops waiting and its diff mode is the user's. `:edit` and `:edit!` raise BufUnload, leaving the window raises BufLeave. The autocmd deletes itself by id, because returning true from a callback deletes only the event that fired. - A window that never showed the file takes the newest set of options saved for it. After a diff window left the file, that set has diff mode on, so `:split <file>` opened a window in diff mode and the next openDiff opened in a new tab. The cleanup now shows the file once in a hidden float with 'eventignore' set to all, runs `diffoff` there while 'diff' is on, and closes the float, so the newest set has diff mode off. No window of the user changes. It skips a file buffer that is unloaded or has 'bufhidden' set. The waiting autocmd stays: a window that left the file takes its own saved set first, and the float does not change that set. - The registry of waiting windows and its test accessor are gone. Each autocmd now prunes closed windows at the next event of the file and deletes itself when no window is left, and every later diff of the file raises such events. - The comment in the cleanup that said `:edit` clears 'diff' only when the new buffer is in no other diff window is gone, and the README states the rule that Neovim follows. Refs #491, #483. _Generated with xum • Model: anthropic:claude-sonnet-5-5 • Thinking: xhigh_
|
@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. |
…ge when the file returns Neovim sets the options it saved for a buffer when the buffer enters a window, 'diff' included. After a split diff closed, a window that had left the file in diff mode got diff mode back when the file returned to it, and a new window for the file took it from the newest saved set. The cleanup now marks each diff window that shows another buffer, and runs :diffoff in a window of the original side's tab page that shows the file with no partner. One BufWinEnter autocmd per file buffer then runs :diffoff in the window that the file enters, only while 'diff' is on there and no other window of its tab page has it on, so a diff that has a partner stays. The autocmd goes when no window waits. The hidden float and the BufLeave/BufUnload handling of the previous push are gone. _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
The diff cleanup now keeps its marks in window variables and lists the windows that show the file with win_findbuf. The mocked unit specs that run the cleanup need both. _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
…ange The cleanup block of diff.lua gained one call that ends diff mode in windows. It resolves, closes and answers no diff, so the model is unchanged. check.sh passes with TLC (89205 states) and covered.lock matches the code. _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Chef's kiss. 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. |
|
@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: c219b15553
ℹ️ 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".
) ## Why `scripts/run_integration_tests_individually.sh` stops a file's Neovim after 30 s (`timeout -k 5s 30s`). `tests/integration/selection_held_spec.lua` is a real-timer spec: it sleeps 21.6 s itself, in 77 `vim.wait` calls (counted by wrapping `wait` in a copy of the spec: `WAITED_MS=21640 CALLS=77`). One run of coder#494 on a loaded host reached the 30 s limit and passed on rerun. I could not reproduce that run, but the arithmetic says why it can happen. The spec has 8.4 s of slack in 30 s, which is 0.11 s per `vim.wait` call. A loaded host that makes each call return 0.11 s late is enough. ## What changes The per-file limit is 40 s instead of 30 s. That leaves each of the 77 calls 0.24 s of slack (18.4 s / 77), twice the slack it had. - `scripts/run_integration_tests_individually.sh`: one variable, `FILE_TIMEOUT=40s`, sets both the `timeout` limit and the number in the `TIMEOUT:` line, so they cannot drift. - `scripts/README.md` line 22 names the new limit and the 50 s ceiling. - `tests/unit/integration_runner_spec.lua`: the two assertions that pin the limit take the new value. Two new tests: one fails when the limit plus the kill grace reaches 50 s, and one fails when the `TIMEOUT:` line names another limit than the one given to `timeout`. I did not shorten the spec's waits. They are the debounce (100 ms) plus a margin on each step. Cutting them would trade a slow runner for a flaky spec. ### Why the limit stays under 50 s plenary's `test_harness` applies its own per-file timeout of 50 s (`timeout = 50000` in `test_harness.lua`), and `test_file` does not pass on the runner's options. So with a limit of 50 s or more, the runner's `timeout` never decides. A hung file ends at about 51 s with exit 1, **no `TIMEOUT:` line**, and its nvim left running with parent pid 1. A first version of this PR used 60 s and had exactly that problem (found by the verifier). With 40 s, the limit plus the 5 s kill grace is 45 s, under 50 s. This is existing behavior on `main`, and this PR does not change plenary's 50 s. Passing a timeout in the harness options is a separate follow-up. A hung file now costs 40 s instead of 30 s before the runner reports it. ## Evidence | Hung spec (sleeps 120 s), real runner script | Result | |---|---| | `main` (30 s) | `TIMEOUT:` line at 30.8 s, no nvim left | | 60 s (first version of this PR) | exit 1 at 51.0 s, no `TIMEOUT:` line, nvim left running (ppid 1) | | this head (40 s) | exit 1 at 40.3 s, `TIMEOUT: hang_spec.lua was stopped (exit 124): nvim did not exit within 40s`, saved nvim pid not running | | `selection_held_spec.lua` | Result | |---|---| | through the runner, alone, 3 runs | 22.6 s, 22.5 s, 22.5 s, exit 0, no `TIMEOUT:` line | | two full runs at the same time, host load average 15 to 18 on 17 cores | 22.9 s and 22.8 s (verifier rerun; the spec's own waits add up to 21.6 s) | | every `vim.wait` 0.2 s longer (+15.4 s), limit 30 s | nvim exit 124 at 30.3 s (the runner exits 1), 10 `Success` rows, no `Fail` row, no summary | | every `vim.wait` 0.2 s longer, limit 40 s | exit 0 at 38.2 s, 14 passed, 0 failed | So the failure mode of a slow host is the runner's limit, not an assertion: the file is killed while its tests still pass. Mutants of the script, each run through `tests/unit/integration_runner_spec.lua` before coder#502 was merged in (15 tests, all pass unmutated): | Mutant | Result | |---|---| | limit 60 s | 3 fail: the two pins and the 50 s test | | limit 45 s (45 + 5 reaches 50) | 3 fail, including the 50 s test | | kill grace 10 s (40 + 10 reaches 50) | 3 fail, including the 50 s test | | `TIMEOUT:` line names 30 s | 1 fails: the new line test | Gate on the head with `main` `f932d7b` (coder#502) merged in: `mise run clean && mise run all` exit 0 (seed 1794412306, 1517 passed, 0 failed, 20 pending). `tests/unit/integration_runner_spec.lua` passes 40 of 40 on that merge, and the diff against `main` is these 3 files. ## The success count Before coder#502, the runner printed `Total Success: 311` where 290 cases ran: it counted each file's `Success:` summary line as one more passed test (290 `Success ||` rows plus 21 summary lines). coder#502 reads only the summary count, and its spec checks the totals. I ran coder#502's script on a tree with this change: `Total Success: 290`. That was before coder#501 merged; at this head, with 22 files, a local run and the CI stable job print `Total Success: 291` (the v0.8.0 job prints 204). This PR does not touch that code. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
…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`_
|
Maintainer decision (answer in the run's chat, recorded here before the push): round 8 is waived for one push. Main moved after the round-7 approval, and this branch now conflicts with main in Generated with |
…lose # Conflicts: # .agents/skills/verify-diff-lifecycle/covered.lock # lua/claudecode/README.md
|
@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: e80a5a3ded
ℹ️ 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".
|
Maintainer decision (answer in the run's chat, recorded here before the push): round 9 is waived for one bounded push. Main changed Generated with |
…lose # Conflicts: # .agents/skills/verify-diff-lifecycle/covered.lock
…a reason (coder#554) ## Why coder#532 left four follow-ups from its last review and the readiness advisor. This PR makes them, and it fixes what the review of this PR found. ## What changed 1. **A line with a reason for every non-zero exit, by stage.** A file whose Neovim exits non-zero gets at least one line: `FAILED: <file>: no plenary summary (nvim exit N)`, the `TIMEOUT:` line, or a new `FAILED: <file>: nvim exit N`. The new line prints when neither of the other two did, for example when Neovim crashes or exits 1 after a clean summary. A file whose specs fail (plenary exits 1) now also gets `FAILED: <file>: nvim exit 1`. A file with no summary whose status is 124, 137 or 143 gets two lines, the no-summary line and the `TIMEOUT:` line, as it does on main. A crash before the summary (for example 139) gets one line, the no-summary line. An exit status above 128 names its signal: `nvim exit 139, SIGSEGV`, also in the no-summary line. A failing `tee` gets its own line, `FAILED: <file>: tee exit N`, with the signal name above 128 (`tee exit 141, SIGPIPE`). On main, a `tee` failure after a clean summary printed no reason line, and the no-summary line showed the status of `tee` as the status of Neovim (a `tee` that exited 3 gave `nvim exit 3`). The file's subshell writes the exit statuses of Neovim and of `tee` to a file in the state folder, so the runner's cleanup deletes it with the folder. A subshell that ends after the cleanup (a signal stop) usually finds the folder gone, and its write fails without a message. When both fail, both lines print. Known limit: the subshell can still open its file while the cleanup runs. Under `strace -f`, this left the state folder with `exit_statuses` in `TMPDIR` and made `rm` print `Directory not empty` in 3 of 3 runs (2 scripted runs and 1 manual run), and main was clean in the 1 run under the same setup. At normal speed it happened in 0 of 150 runs on this PR, and in 0 of 60 runs on main. A follow-up PR in this run fixes it. 2. **The `TIMEOUT:` lines are true.** Exit 137 comes from `timeout -k` and also from an out-of-memory kill: `TIMEOUT: <file> was stopped (exit 137, SIGKILL): nvim did not exit within 40s, or another process killed it`. An outside SIGTERM that reaches `timeout` gives 124 with uutils `timeout` and 143 with GNU `timeout` (a child that dies of the signal), so the lines for 124 and 143 now end with `or an outside signal stopped it`. 3. **The file's process group is killed after any non-zero exit of Neovim (a `tee` failure alone does not kill it).** Before, only a timeout status killed it. When the Neovim that the runner starts died by a signal before its summary, the Neovim that plenary started for the spec ran on. This is on main too. The cleanup after a signal to the runner is unchanged. Known limit, not changed: the kill uses the process group id that `timeout` had. A process group id that another process reused in the few milliseconds after `timeout` ended would get SIGKILL. `pid_max` is 4194304 on my host, so a reuse needs a wrap-around in those milliseconds. Main has had the same risk after a timeout status since coder#445. I declined to change it, because the chance is very small (a wrap-around of 4194304 ids inside those milliseconds) and main already takes the same risk after a timeout status. The follow-up PR records this reason. 4. **README.** `scripts/README.md` says what the early window does for a runner that leads a pty session: SIGTERM, SIGHUP, SIGINT and SIGUSR1 leave nothing and start no Neovim, SIGKILL starts no Neovim but leaves all four temp items (without a terminal the Neovim starts as well), and bash ignores SIGQUIT so the file runs to its end. It also describes the new output lines and where the statuses go. 5. **Comments and tests.** The comment on the reason lines says what is true (see 1). The `-i NONE` test also checks `--headless` and `-u` followed by `tests/minimal_init.lua`. ## Tests New or changed runner spec tests (the names are the columns of the table below): - `clean_after`: five tests. A clean summary followed by exit 139, 134, 130, 129 or 1 prints the `FAILED:` line (with `, SIGSEGV`, `, SIGABRT`, `, SIGINT`, `, SIGHUP` or no name), exactly one `FAILED:` line, the counts in the totals, and the file in the final list. - `crash_before`: no summary and exit 139 names SIGSEGV in the no-summary line, with one `FAILED:` line and no `TIMEOUT` line. - `sigkill_timeout` and `kill_after`: a stub Neovim that kills itself with SIGKILL gets the `TIMEOUT:` text, exactly once, and no extra `FAILED:` line after a clean summary. - `stuck_137`: the test of a Neovim that ignores SIGTERM expects `(exit 137, SIGKILL)`. - `tee_stage`: `tee` exits 3 after a clean summary: `FAILED: <file>: tee exit 3`, no `nvim exit` text. - `both_stages`: Neovim exits 1 and `tee` exits 141: both lines, `tee exit 141, SIGPIPE`. - `outside_term`: an outside SIGTERM reaches `timeout` while the stub Neovim sleeps. The `TIMEOUT:` line has the new text with exit 124 (uutils) or 143 (GNU), exactly once. - `p3_orphan`: exit 139 before any summary with a process left in the group: the process is killed. - `cleanup`: after a file that failed, with a failing `tee`, nothing is left in `TMPDIR`, the status file included. - `late_tee`: a `tee` that ends a second after the SIGTERM, long after the runner cleaned up: nothing is left in `TMPDIR`, and the runner prints no `No such file` message. Each test ran against each mutant of the runner, with uutils `timeout` 0.10.0 and with GNU `timeout` 9.7. The two builds gave the same table. Red means the test fails. | Variant | clean_after | crash_before | sigkill_timeout | kill_after | stuck_137 | tee_stage | both_stages | outside_term | p3_orphan | cleanup | late_tee | |---|---|---|---|---|---|---|---|---|---|---|---| | this PR | green | green | green | green | green | green | green | green | green | green | green | | main's runner | **red** | **red** | **red** | **red** | **red** | **red** | **red** | **red** | **red** | **red** | green | | first push of this PR | green | green | green | green | green | **red** | **red** | **red** | **red** | **red** | green | | signal names only above 130 | **red** | green | green | green | green | green | green | green | green | green | green | | status file ignored | green | green | green | green | green | **red** | **red** | green | green | green | green | | no `tee` line | green | green | green | green | green | **red** | **red** | green | green | green | green | | a `tee` failure does not fail the file | green | green | green | green | green | **red** | green | green | green | green | green | | `tee` line without the signal name | green | green | green | green | green | green | **red** | green | green | green | green | | 124 and 143 lines as on main | green | green | green | green | green | green | green | **red** | green | green | green | | group kill only after a timeout status | green | green | green | green | green | green | green | green | **red** | green | green | | no group kill after a non-zero exit | green | green | green | green | green | green | green | green | **red** | green | green | | state folder not deleted | green | green | green | green | green | green | green | green | green | **red** | green | | nvim line uses the status of the subshell | green | green | green | green | green | **red** | green | green | green | green | green | | status file in the temp folder (previous push) | green | green | green | green | green | green | green | green | green | green | **red** | | status write without the guard for a folder that is gone | green | green | green | green | green | green | green | green | green | green | **red** | The existing test of what a timed-out file leaves running is green for this PR and for the variant that kills the group only after a timeout status. It is red when the group kill is removed. `late_tee` is green for main and for the first push because neither has a status file. Not tested: the fallback for a status file that the runner cannot read. The runner then uses the exit status of the subshell. A variant whose subshell exits with the status of Neovim only, and so loses the status of `tee` in that fallback, passes every test. A follow-up PR in this run adds a test for it. ### Real Neovim A tree with one spec, `timeout` 0.10.0 (uutils) and 9.7 (GNU), stdin `/dev/null`: | Case | main | this PR | |---|---|---| | the runner's own Neovim dies by SIGSEGV after the spec's Neovim started | `FAILED: … (nvim exit 139)`, the spec's Neovim still running 3 s after the runner ended | `FAILED: … (nvim exit 139, SIGSEGV)`, the spec's Neovim gone | | SIGTERM to the `timeout` that runs the runner's Neovim, before the summary, uutils | exit 124, `TIMEOUT: … (exit 124): nvim did not exit within 40s` | exit 124, the same line ending `, or an outside signal stopped it` | | the same, before the summary, GNU | real Neovim exits 1 on SIGTERM: `FAILED: … no plenary summary (nvim exit 1)`, no `TIMEOUT:` line | the same output | | the same, after the summary, uutils | the `TIMEOUT:` line only | the `TIMEOUT:` line only, with the new ending | | the same, after the summary, GNU | no line | `FAILED: … nvim exit 1` | | a `tee` that exits 3 after a clean summary | runner exit 1, no reason line | `FAILED: a_spec.lua: tee exit 3` | Early window, a stub `bash` that delays the process id file, a hanging spec, the runner leading a pty session: | Signal to the runner | Neovim started | Temp items left | |---|---|---| | SIGTERM, SIGHUP, SIGINT (Ctrl-C) | no | none | | SIGKILL | no | all four | | SIGQUIT | yes, the file runs to its end (bash ignores the signal) | not applicable | Without a terminal, SIGKILL to the runner leaves all four items and the Neovim starts. ### Whole runner, main versus this PR An independent verifier ran this on this exact head, with stdin `/dev/null` and `TMPDIR` unset. The rows are identical: 123 per-file rows each (30 files with a `Running` line and three count lines, plus three totals), no difference, and no `FAILED`, `TIMEOUT`, E886, `Killed` or `No such file` line. | Neovim | main | this PR | |---|---|---| | 0.12.3 | 527 passed, 0 failed, 0 errors | 527 passed, 0 failed, 0 errors | | v0.8.0 | 390 passed, 0 failed, 0 errors | 390 passed, 0 failed, 0 errors | With `TMPDIR` set to a folder with a space and a `%` (what the CI guard uses), this PR on 0.12.3 also gives 527 passed, 0 failed, 0 errors. ### Gate The verifier ran `mise run clean && mise run all` on this exact head. The formatter changed no file (0 of 426), luacheck reports 0 warnings and 0 errors in 185 files, and busted reports 1751 successes, 0 failures, 0 errors, 28 pending. `shellcheck` on `scripts/*.sh` and `actionlint` pass. A gate run of mine earlier in this round failed one test: the window test found the status file in `TMPDIR` after a signal stop, because it had been next to the output file. I moved it into the state folder and added the `late_tee` test. Three merges of main came after the commit that moved the status file (`51ed703b`). They brought coder#556 with coder#458 (a docs change and a `repro_issue_218.sh` change), then coder#494 (a diff change with its own integration spec), then coder#555 (a wire-probe cleanup change). My last gate run covered only the first of them. I ran the spec that coder#494 added through this runner 10 times alone just now: 10 passed, in 3.8 to 5.4 seconds. One earlier run of mine hit the 40 second limit on it. I did not find the cause. ## Follow-up A follow-up PR in this run has no number yet. It holds these changes: 1. Wording. The README line on the output lines and a runner comment say "a timeout or a signal" for the two-line case. It is status 124, 137 or 143, and a crash with 139 or 134 gets only the no-summary line. The README line and a comment say an outside SIGTERM gives 143 with GNU `timeout`. That holds only when the child dies of the signal, and real Neovim gives 1. The README line says "because nvim did not exit in time", which the hedged `TIMEOUT:` text contradicts. The README sentences on the cleanup ("deletes the four" and the status file) leave out the known limit in item 1 above. Optional: no signal name prints for a status from 193 to 255, because bash has no signal above 64. 2. A fix for the known limit in item 1: no file path is opened after the cleanup starts, with a regression test. Two ways are not tried yet. The runner can open the status file on a fixed file descriptor before it starts the subshell, or the cleanup can kill and reap the subshell before `rm -rf`. 3. A test that reaches the fallback for an unreadable status file, and fails for the variant that exits with the status of Neovim only. 4. A record of the group id limit from item 3 and the reason I declined it. No code change. ## Not in this PR A 40 second limit per file (`FILE_TIMEOUT`) was reached at a high host load while specs ran. The limit is the same on main. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
… other path characters (coder#561) ## Why `cwd.git_root` must find the repository of a folder whatever its name is. A repository in a folder with a space or a comma in its path was not found when git could not run: the fallback gave nil, or the repository of Neovim's current folder. A folder named `[x] {y}` or `$HOME-like` was lost or changed even when git answered. A verifier of F12 found the first case (finding F12-P1, a space or comma in the path makes the `.git` fallback return nil). ## What changed Four problems in `lua/claudecode/cwd.lua`. The first two took a path as a pattern. The third stepped one folder down at a trailing `..`. The fourth is a flaw in my first version of the second step. 1. **The `.git` fallback** called `vim.fn.finddir(".git", start_dir .. ";")`. `finddir` reads its path as a list in `'path'` option syntax. A space or a comma split the path, and a `;` ended it. The fallback now walks up from the full path of `start_dir` and tests each folder for a `.git` folder with `vim.fn.isdirectory`. A relative `start_dir` now reaches the folders above the current one too. Before, a relative start gave a wrong answer or nil in some cases (for example `git_root("sub/deeper")` in the repository root). 2. **`normalize_dir`** passed every path through `vim.fn.expand`. `expand` reads `[`, `{`, `*`, `?`, `$` and `\` in a name as patterns or variables. A folder named `[x] {y}` or `it's` came back with a trailing slash. A folder named `$HOME-like` or `back\slash` came back nil. This also happened when `git rev-parse` answered. `normalize_dir` now returns a directory that exists as written, and expands any other string (`~`, `$VAR`). See step 4 for text that starts with `~` or `$`. 3. **`..` before Neovim 0.11.** Before 0.11, `fnamemodify(path, ":p")` keeps a last `..`, and `:h` of `a/b/..` is `a/b`. So a walk from a start folder that ends in `..` stepped one folder down and returned the wrong root. The walk now calls `simplify()` on the full path first. A verifier found this. 4. **Text that starts with `~` or `$`.** The first version of step 2 took a path that names an existing folder as written, on the raw text. `vim.fn.isdirectory` does not expand, so with `cwd = "~"` or `"$HOME"` and a stray folder of that literal name in Neovim's current folder (for example one that an unexpanded `mkdir "~/x"` made), `git_root` and `resolve` returned the stray folder where main returned the home folder. `normalize_dir` now sends text that starts with `~` or `$` through `expand` first, as a shell and `:cd` do. A verifier of the terminal change found this. `lua/claudecode/README.md` gets one new bullet that states this, with the rule for text that starts with `~` or `$`. It sits after the "Terminal cwd precedence" bullet. It names `git_root` and `resolve` only, and it says that the terminal still passes a static `cwd` and a `cwd_provider` result through `vim.fn.expand` itself. ## Scope - Only `cwd.lua` changes in code. Steps 1 and 2 are one change of idea: a path is a literal, not a pattern. Step 3 is a quirk of `fnamemodify` before Neovim 0.11. Step 4 closes a hole that step 2 opened. - `normalize_dir` serves `cwd.git_root` and `cwd.resolve`. Nothing in `lua/` calls `cwd.resolve`. The terminal calls `cwd.git_root` when `git_repo_cwd` is set, so this PR fixes folder names with `$` and `\` on that path: when git does not answer, the terminal now gets `$HOME-like`, `back\slash`, `[x] {y}`, a space and a comma right, where main gave nil or the repository of Neovim's current folder. A static `cwd` and a `cwd_provider` result do not go through `normalize_dir`: `terminal.lua:build_config` passes them through `vim.fn.expand` itself, so this PR does not change what the terminal does with those. An absolute start path that ends in `..` is fixed here, through the `simplify()` call above. - Two gaps stay for separate follow-ups. Main already has both, and this PR does not reach them. First, the fallback finds a `.git` folder only, not a `.git` file in a worktree or submodule. Second, the walk is lexical, so a link into another repository answers with the repository of the link's own folder, where git answers with the link target's repository. The terminal's `expand` of a static `cwd` and of a provider result is a third follow-up, and it has its own change. - `~` and `$VAR` still expand in `cwd.resolve` (the spec checks both). The spec's cases for `cwd.resolve` are named as `cwd.resolve` cases, because they do not test the terminal. - `git rev-parse` still runs first. Its result for a plain path is the same string as before. ## Blast Radius - User-visible: a repository in a folder with a space, a comma, a quote or another path character is now found when git cannot run. With git, `git_root` returns a folder with `[`, `{`, `*`, `?`, `$` or `\` in its name exactly. It does not reach the terminal through a `cwd_provider`: `build_config` still expands the provider's result, so the root README's example still loses `$` and `\` there (follow-up). A plain path gives the same result as before. - The fallback result is an absolute path with no `..` part. Before it could be relative. The one exception is a relative start whose name begins with `~`: Vim reads `~` as the home folder and `~name` as a user's home folder, so `:p` leaves `~name` relative and the result can be the relative `~name`. That is a pathological case, and no caller in `lua/` passes a relative path (the terminal passes the file's folder from `expand("%:p")` or `getcwd()`). - A string that names an existing folder is taken as written, except text that starts with `~` or `$`. That text always goes through `expand` first, so a folder named `~` or `$HOME` in Neovim's current folder does not take the place of the home folder, as on main. The cost: a relative folder name that starts with `$` and reads as a variable (`$HOME-like`, `$odd/sub`) is expanded and not found, as on main. A lone relative `$odd` that names no variable is left unchanged by Vim and is still found. An absolute path never starts with `~` or `$`, so every folder name this PR fixes keeps working. The change applies to `git_root`'s `start_dir` and to `cwd.resolve`. The terminal's own `expand` of a static `cwd` and a provider result is a separate change. - Neovim 0.8: the code uses `vim.fn.isdirectory`, `vim.fn.fnamemodify` and `vim.fn.simplify`, which exist there. I ran the new spec on Neovim 0.8.0, 0.10.0 and 0.12.3. The `..` case fails on 0.8.0 and 0.10.0 without the `simplify()` call, and passes on 0.12.3 either way. - `covered.lock`: `cwd.lua` is the terminal's working-directory lookup. `check.sh` printed STALE for the file hash of `cwd.lua` only, with no changed block or call site. The change cannot reach diff state, an openDiff answer, a disconnect or a diff command. The model passed (`main`: 89205 distinct states, `live-DisconnectedCallsAnswered`: 5269). `check.sh --record` wrote the new lock and `check.sh --hash-only` reports that it matches. ### Merge with coder#539 (F12) coder#539 landed first. This PR's merge commit with `origin/main` resolves the interplay: it re-records `covered.lock` and changes the one stale test in coder#539's `cwd_spec.lua` (see Verification). F11 (coder#535) merges with this PR without a conflict. ## Verification Red before the fix, green after (same spec, only `lua/claudecode/cwd.lua` swapped): | Spec | origin/main | with the fix | | --- | --- | --- | | `tests/integration/cwd_path_characters_spec.lua` (37 cases, real Neovim, 0.8.0 and 0.12.3) | 20 successes, 17 failures | 37 successes, 0 failures | | the new case "goes up from a start folder that ends in .." at `ede2f733` (before the `simplify()` call) | fails on 0.8.0 and 0.10.0, passes on 0.12.3 | passes on 0.8.0, 0.10.0 and 0.12.3 (35 successes when added, 37 at the final head, 0 failures on each) | | the two cases "expands a leading ~ / $ even when Neovim's cwd holds a folder named ~ / $HOME", with `cwd.lua` from `ca9e61c0` (before the guard) | 35 successes, 2 failures on 0.8.0 and 0.12.3 | 37 successes, 0 failures on each | The spec makes real folders and runs `cwd.git_root` on them, in two modes. In the first mode git is off `PATH` (the shell keeps its absolute path), so `git` ends with "not found" and a list command raises E475, as on a host without git. In the second mode a real `git init` makes the repository. The folder names are: a space, a comma, a semicolon, single and double quotes, brackets and braces, parentheses, non-ASCII letters, a star and a question mark, a percent sign and a hash, a backslash, a dollar sign, a leading tilde, and a plain name. Each name runs from the repository folder and from a folder two levels below it, and the answer must equal the repository path with no trailing slash. A folder that is not a repository returns nil, whatever its name. Seven more cases cover a relative start folder, a start folder that ends in `..`, `cwd.resolve` with `git_repo_cwd`, a static `cwd` with brackets and a star in `cwd.resolve`, `~` and `$VAR` in a setting of `cwd.resolve`, and two cases where Neovim's current folder holds a folder named `~` or `$HOME` and the setting must still expand. The 17 red cases are 10 of the no-git names, 4 of the git names, the relative start, `resolve`, and the static `cwd`. Gates on the final head `f12368bd`, which contains a merge of `origin/main` (05b9785, with coder#551, coder#535, coder#494, coder#545, coder#553, coder#555 and coder#557) (private XDG folders). An independent verifier passed `c73ab044`. Since then the branch has three changes of its own and two merges with main. The changes are a docs commit (the README sentence about where the walk starts), the fix for text that starts with `~` or `$` (step 4, with two spec cases and the README clause), and the `covered.lock` record for it. For the lock I ran the full `check.sh`: the model passed (89205 distinct states, 5269 live states, every fact TRUE), and only the file hash of `cwd.lua` was stale, with no changed block or call site. - `mise run all`: native exit 0. luacheck: 0 warnings / 0 errors in 186 files. busted: 1756 successes / 0 failures / 0 errors / 29 pending. treefmt: 0 files changed. Since coder#545, `mise run all` also runs the static proofs of the `verify-claudecode-nvim` skill, and they pass. - `mise run format-check`: native exit 0 (428 files, 0 changed). - `scripts/run_integration_tests_individually.sh` on Neovim 0.12.3: 564 successes, 0 failed, 0 errors, native exit 0. On Neovim 0.8.0: 427 successes, 0 failed, 0 errors, native exit 0 (the 0.8.0 run skips the specs that need 0.9). The new spec gives 37 successes on both. It starts about 100 processes (git and a shell), so it takes about 6 s on a quiet host and can pass the runner's 40 s limit at a load average above 100. A TIMEOUT line there means the host was slow, not that the spec hung. - All four `.agents/skills/verify-*/check.sh --hash-only` match their `covered.lock`. The merge with `origin/main` had two parts to resolve in an earlier commit. `covered.lock` conflicted on the hash line of `cwd.lua`, so I ran the full `check.sh` and `check.sh --record`. coder#539's `tests/unit/cwd_spec.lua` had a test that stubbed `vim.fn.finddir`, which the fallback no longer calls, so that test now walks up real folders. It does not depend on where `TMPDIR` is: it passes with `TMPDIR` inside a git checkout too. `cwd.lua` itself merged without a text conflict. The two latest merges needed no `covered.lock` change: coder#551 conflicted only in `tests/mocks/vim.lua`, where the branch keeps both its `simplify` entry and coder#551's `wordcount`, and coder#535 and coder#494 merged without a conflict. An independent verifier found the `..` case, the README wording and the other points that this body now states. The `terminal.lua` expand and `.git` files in worktrees are gaps that main already has. They are follow-ups, not part of this PR. --- _Generated with [`xum`](https://github.com/coder/xum) • Model: `anthropic:claude-sonnet-5-5` • Thinking: `xhigh`_
… 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`_
Why
After a split diff closes, a window that still shows the file can stay in diff mode with no partner window. The next
openDiffthen sees a window in diff mode, opens in a new tab, and closes the user's window together with that tab. #491 ended diff mode in the windows that the cleanup knew about. This closes the windows that it did not.Refs #491, #483.
Cause
Neovim saves a window's options for a buffer when the buffer leaves the window ('diff', 'scrollbind', 'foldmethod' and the rest included), and sets them again when that buffer enters the window. A window that never showed the buffer can also take diff mode on, by copying the options of a window that shows the buffer (
:split <file>copies the window that it splits). While a split diff is open, the file's buffer is in diff mode in the original-side window. So when the cleanup closes the proposal window and deletes the proposal buffer, a window can end up showing the file in diff mode with no partner. Two ways, both reproduced on current main with real Neovim 0.12.3 and v0.8.0::b <proposal>in the original window, thenclose_tab. The cleanup deletes the proposal buffer, the window falls back to the file, and the saved options turn diff mode on. One window,diff=true.:split <file>from the original window during the diff, thenclose_tab. The copy took diff mode from its source, the cleanup closes the proposal window, and the copy keeps diff mode alone. It stays on whenclose_tabruns from a plain window of the same tab, or from another tab.What changed
The rule: when the cleanup is done, a window of the original side's tab page that shows the file leaves diff mode only if it is the only window with diff mode on in that tab page. A diff needs a partner window, so a lone diff window holds the leftover of the diff that ended, and a diff of the user's own over two or more windows stays.
lua/claudecode/diff.lua: two local functions and one call.end_diff_mode_if_alone(win)runsdiffoffin a window while'diff'is on there and no other window of its tab page has'diff'on. It catches the refusal that Neovim gives in the command-line window (E11).end_diff_mode_in_windows_that_show_the_file(diff_data)runs that check for each window of the original side's tab page that shows the file. The cleanup calls it after the buffer deletes and beforeClaudeCodeDiffClosedfires, so nothing that a handler of that event starts is looked at. Nothing is recorded and no autocmd is added.lua/claudecode/README.md: states the rule, and which cases stay as on main (below). The sentences on saved options and on the unified layout's'signcolumn'are rewritten from plain-Neovim probes.tests/integration/diff_left_window_spec.lua(real Neovim): 33 cases. They cover the two ways above, a window that the diff never left, a copy that has a partner and two copies that the user compares (kept), the options that the user set after ending diff mode, a lone diff window of another tab (not touched), the order of the look and the event (a diff that aClaudeCodeDiffClosedhandler starts stays),close_tabfrom the command-line window (it answers TAB_CLOSED and the copy keeps its diff mode, because Neovim refusesdiffoffthere), and the cases below that stay as on main.journey-diff-close-save.shshows the proposal in the original window with:band closes the diff for an existing file, andfeatures/open-diff.mdandSKILL.mddescribe it.covered.lockis re-recorded after merging main. Against main's lock only thediff.luafile hash and thecleanup_stateblock hash change.check.shpasses with TLC (89205 states).What stays as on main
A file that comes back to a window later gets the options that Neovim saved or copies, diff mode included, as on main:
:e other.txtin the proposal window,close_tab, then:e <file>there, or:e third.txtin the original window,close_tab, then:e #. Diff mode is on again. This is the common case. It gets its own follow-up PR that fixes it at the source: the plugin splits the window before it runsdiffthis, in both open paths, so the saved set never holds diff mode.:split <file>from a window that left the file after the diff.:tabnew <file>,:tab sbuffer,:tabnewthen:buffer <file>). The look covers only the tab page of the original side.ClaudeCodeDiffClosedhandler that brings the file back into a window.close_tabfrom the command-line window (q:) when the original-side window shows the file fails with -32000 and E11 in main's owndiffoff, as on main.Before and after
Real headless Neovim, the plugin's own
openDiffandclose_tabtool handlers, a stubclaudefirst on PATH::b <proposal>,close_tabdiff=truediff=false:split <file>from the original window during the diff,close_tabfrom a plain window of the same tabdiff=true, the plain window's'foldmethod','scrollbind'and'wrap'keptdiff=false, the plain window's options keptclose_tabfrom another tab that holds the user'sdiffthisover two windowsdiff=truediff=false, the other tab's diff keptopenDiff,:edit <file>in that window, and the secondclose_tab:e other.txtin the proposal window,close_tab,:e <file>diff=truediff=true(follow-up PR)diff=falsediff=falseThe same rows give the same result on 0.12.3 and v0.8.0.
Validation
Head 06bde3e (round 9: a merge-only push of main 884f7e2 that re-records
covered.lock, with no Lua change and no Codex review; the Lua patch-id equals rounds 7 and 8). On it:check.sh --hash-onlyandcheck.shpass, the #494 spec passes 33/33 on Neovim 0.12.3 and v0.8.0,mise run allpasses (1737 successes), the integration runner passes (527 on 0.12.3 after one rerun of two load timeouts, 390 on v0.8.0), and CI is green.Head e80a5a3, a merge of main 8b77e7e into c219b15 (round 8: a merge-only push, no code change). The PR's Lua diff is the same before and after the merge:
git diff c9e57d13..c219b155 -- '*.lua'andgit diff origin/main...HEAD -- '*.lua'give the same patch-id (76da21f7b53bfdadb02303aa6ef201c9f445de4e) and the same text without theindexlines and hunk numbers (main moveddiff.lua, so those differ). The merge conflicted incovered.lockand inlua/claudecode/README.md. The README conflict was main's edits around the cleanup bullet (#549, #542 and others) against the one bullet that this PR rewrites: I kept every line of main and put this PR's bullet in place of the old one, so the README differs from main by that one line. I re-recordedcovered.lockwithcheck.sh --recordon the merge result. Against main's lock only thediff.luafile hash and thecleanup_stateblock hash differ. Size against main: 7 files, 613 insertions(+), 7 deletions(-). Code and README: 2 files, 42 insertions(+), 3 deletions(-). The spec is most of the rest.mise run clean && mise run allon the merge result: exit 0. luacheck 0 warnings, 0 errors in 182 files. busted 1706 successes, 0 failures, 0 errors, 26 pending.close_tabfrom another tab). It also passes 33 of 33 on both versions on the merge of this head with the newest main (e4c56fd), which merges without a conflict, andcheck.sh --hash-onlypasses on that merge.claudefirst on PATH: 0.12.3 450 successes and v0.8.0 342, 0 failed, 0 errors (28 files). UnderTMPDIR="<dir>/odd tmp 50%"the same: 450 and 342, 0 failed, and the cache folder stays untouched.journey-diff-close-save.shin agent-tty with the scripted client (run on c219b15, before the merge): 4 of 4 runs pass (split and unified, existing and new file). Existing file: 24 checks. New file: 23.The 8 fixed checks of the round-4 set are the
:splitcopy cases (row 2 and 3), the 4 of the round-3 set are:b <proposal>(row 1) and the second diff (row 4), the 2 of the Opus set are the second diff.BufEnter,BufWinEnter,WinNew,WinEnter): gpt's 96 orders per version, Opus's 96 orders (48 per version) and my 40 orders (20 per version) give the same answer, in every field, on this head and on main. A focus probe with a userBufWinEnterandBufEnterhandler that moves the focus to another tab gives the same answer on both.:splitcopy with a lone user:diffthisthat gets the file back with:edit!,:e otherand:e #, and a window that got the file without aBufWinEnter): this head keeps the user's diff in all, as main does.diffoff, no'diff'check, no partner check, partner in any tab, look in any tab, skip the original-side window, no catch of the refusal, nonvim_win_call, the partner must show another buffer, the call after the event, the call before the deletes, no buffer and window guard,diffoff!). 13 are caught by the spec on both versions: 12 as failed cases and the one without the guard as an "Invalid window id" error. The survivor isdiffoff!fordiffoff. It is equivalent: the check runs first, so no other window in the tab has'diff'on, anddiffoff!acts only on windows with'diff'on.check.sh --hash-only: covered.lock matches the code.check.sh(TLC, 89205 states, and 5269 for the live model): pass.Round limit
The maintainer waived the review-round limit for one push (round 7): #494 (comment). A second waiver covers one merge-only push (round 8, this head): #494 (comment). The push replaces the earlier
BufWinEnterwaiter and its window records with the single look above and merges current main. There is no further push to this PR. A new security flaw, data loss or broken core behavior compared with main ends it as blocked. A third waiver covers one bounded merge-only push (round 9) with no Codex review, and declines the round-8 Codex P1 with a reason: #494 (comment).Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh