Repository navigation
fix(diff): run the unified exit specs on Neovim 0.8.0 and correct the exit answers docs - #572
Merged
Merged
Conversation
#564 made the unified layout work on Neovim 0.8.0 (it uses vim.diff there) and removed the "needs Neovim 0.9" skips from eight specs. diff_exit_reject_spec.lua came in with #558 and still skipped the unified layout below 0.9 in three places, so 14 unified exit scenarios stayed pending in the v0.8.0 job. The skips and the comment that said the layout needs Neovim 0.9 are gone. The spec has 28 passes on Neovim 0.8.0 and 28 on 0.12.3. Before: 14 passes and 14 pending on 0.8.0. No other claim in the repo says that the unified layout needs Neovim 0.9. The remaining nvim-0.9 checks are about 'statuscolumn', `:w ++p` and `nvim -l`.
…iting calls The sentence said that with on_new_file_reject = "close_window" both diffs have their answers before the first ClaudeCodeDiffClosed fires. That holds for calls that are waiting. A call that a nested `:qa!` cuts off during its openDiff setup has no response sender yet, and the server stop answers it with -32603 after the ClaudeCodeDiffClosed events. Probed on Neovim 0.8.0 and 0.12.3 in the vertical, horizontal and unified layouts with both values of on_new_file_reject. Waiting calls with close_window: the answers come before the first ClaudeCodeDiffClosed in 6 of 6 runs (:qa! and SIGTERM, three layouts) on each version. A nested `:qa!` in the handler of the second call, and in the handler of the only call: the -32603 comes after every ClaudeCodeDiffClosed event in 12 of 12 runs on each version, and no id is answered twice. The order of the ClaudeCodeDiffClosed events among themselves differs between the two versions, and the sentence does not depend on it.
…ts without VimLeavePre The "Exit answers" bullet said that SIGTERM and SIGHUP run VimLeavePre and give each waiting call a DIFF_REJECTED. When the signal arrives while an autocmd that is not `nested` runs and pumps the event loop (for example a handler that waits in `vim.wait`, such as a synchronous LSP format on save), Neovim runs no VimLeavePre. No waiting call gets an answer, the client sees 1006, and the lock file stays. The exits-without-VimLeavePre sub-bullet now says so. Probed on Neovim 0.8.0 and 0.12.3 (SIGTERM and SIGHUP, vertical and unified, two waiting calls): no answer line, and the handler never ends. The verifier of the previous commit found it with 24 of 24 runs on each version and a control: the same signal during `vim.wait` in a scheduled callback (no autocmd) answers both calls. The same commit makes the version note of the new sentence agree with the bullet: the new facts hold on Neovim 0.8.0, 0.9.5, 0.10.4, 0.11.5 and 0.12.3.
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🛡️ Codex 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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a9ea0a943
ℹ️ 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".
…ops the exit The sub-bullet for exits without VimLeavePre said that a SIGTERM, SIGHUP or `:qa!` that comes while an autocmd that is not `nested` runs gets no answer. That is true only for an autocmd that pumps the event loop, for example with `vim.wait`. An autocmd that does not pump it (a busy wait or an ordinary callback) lets the exit run after it returns, and the waiting calls get their DIFF_REJECTED. Probed on Neovim 0.8.0 and 0.12.3 (SIGTERM and SIGHUP, vertical and unified, two waiting calls): a non-nested autocmd that busy-waits 600 ms after the signal ends, and then both calls are answered, in 8 of 8 runs. With `vim.wait` in the same autocmd, no call is answered (8 of 8 runs before). Codex review round 1, finding P2 on README.md:1164.
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. 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. |
…cmd condition Commit 2df6683 put "runs and pumps the event loop" on the whole list, so a `:qa!` run directly in an autocmd that is not `nested` dropped out of the exits without VimLeavePre, and the new sentence said such an autocmd does not stop the exit. It does: a direct `:qa!` in a non-nested autocmd runs no VimLeavePre. The advisor of the PR found it. The sub-bullet now says: - SIGKILL, a crash and a `:qa!` from inside an autocmd that is not `nested` run no VimLeavePre (as on main). - A `:qa!`, SIGTERM or SIGHUP that comes while such an autocmd pumps the event loop (for example with `vim.wait`) runs none either. - A signal that comes while such an autocmd runs without pumping waits until it returns, and the exit then runs VimLeavePre. The note "The signal case was probed on Neovim 0.8.0 and 0.12.3" is gone. ARCHITECTURE.md, the module README and the comment of cleanup_at_exit say the same, and the comment no longer says that SIGTERM and SIGHUP always run VimLeavePre. Probed on Neovim 0.8.0 and 0.12.3, vertical and unified layouts, two waiting calls, 6 runs per cell: in a non-nested autocmd, a direct `:qa!`, a timer `:qa!` during vim.wait, SIGTERM and SIGHUP during vim.wait run no VimLeavePre and answer no call (24 of 24 on each version). SIGTERM and SIGHUP during a busy wait (no pump) end the autocmd, run VimLeavePre and answer both calls (12 of 12). In a `nested = true` autocmd every one of the six cases runs VimLeavePre and answers both calls (36 of 36).
… cleanup_at_exit comment Only the file hash of lua/claudecode/diff.lua changed, by a comment. The model passes (93,477 distinct states, and 5,557 for the live case).
The README says which exits run no VimLeavePre when the exit starts inside an autocmd. The spec now runs four of them with two waiting calls, in the vertical and unified layouts: - a direct `:qa!` in an autocmd that is not nested: no call is answered - SIGTERM while such an autocmd waits in vim.wait: no call is answered - SIGTERM while such an autocmd busy-waits (no pump): both calls get DIFF_REJECTED - a direct `:qa!` in a nested autocmd: both calls get DIFF_REJECTED Each case first checks that both calls were waiting. The child has no server, so the cases without VimLeavePre show only that no answer comes. The first two pin what Neovim does: a Neovim that changes it turns them red, and the README sentence has to change. The spec has 36 passes on Neovim 0.8.0 and 0.12.3, 20 of 20 runs on each. A mutant that makes cleanup_at_exit return at once turns the SIGTERM busy-wait case red in both layouts and the nested case red in the vertical layout.
# Conflicts: # .agents/skills/verify-diff-lifecycle/covered.lock
The last two commits tried to say when an exit that starts in an autocmd runs VimLeavePre, and each was wrong in a new case. The first dropped a direct `:qa!`. The second promised VimLeavePre for a signal that comes while an autocmd "runs without pumping the event loop". Neovim handles a signal at its next event check (vim.wait, a Vim script loop, `:%s`, idle), not during a Lua loop. When that check comes while any autocmd that is not `nested` still runs (the same handler waits later, the next handler for the event waits, a Vim script loop or `:%s`), no VimLeavePre runs. The sub-bullet now keeps only the rule that holds in every case: SIGKILL, a crash and a `:qa!` from inside an autocmd that is not `nested` run no VimLeavePre, and neither does a `:qa!`, SIGTERM or SIGHUP that Neovim handles while such an autocmd is still running. A synchronous LSP format on save, which waits in vim.wait, is the common example. Claude gets no answer, the socket closes with 1006 and the lock file stays. The text promises nothing for a signal during a plain Lua loop. ARCHITECTURE.md, the module README and the comment of cleanup_at_exit say the same. Probed on Neovim 0.8.0 and 0.12.3, vertical and unified layouts, two waiting calls, 6 runs per row. In an autocmd that is not nested, each of these runs no VimLeavePre and answers no call (all 54 runs per version): a direct `:qa!`, a timer `:qa!` during vim.wait, SIGTERM and SIGHUP during vim.wait, SIGTERM and SIGHUP in the Lua part of a handler that waits later, SIGTERM in the first of two handlers when the second waits, SIGTERM in a Vim script loop, and SIGTERM in a `:%s` loop. In a nested autocmd the same cases answer both calls and run VimLeavePre (the `:%s` case in the vertical layout answers both calls but a user VimLeavePre autocmd never logs, which is the hang that the verifier found on main). With no autocmd, a timer `:qa!`, SIGTERM and SIGHUP answer both calls and run VimLeavePre (18 of 18).
… comment of cleanup_at_exit Only the file hash of lua/claudecode/diff.lua changed, by a comment. The model passes.
…nd drop the busy-wait case The two "gives no answer" cases also passed when the child never exited: `run_child` stops a child that runs 20 s and reads the log at once, so a no-op instead of `:qa!`, or a SIGTERM that never came, left an empty answer list. Both cases now assert that the child ended by itself (`code ~= -1`). With the autocmd's `:qa!` replaced by a no-op, the first case is red on Neovim 0.8.0 and 0.12.3 (the file then runs into the 40 s limit of the runner, because each affected case waits 20 s). The busy-wait case (SIGTERM while a non-nested autocmd runs a Lua loop, both calls answered) is gone. The README no longer promises anything for it, and Neovim does not keep that outcome when an event check comes later in the same autocmd. The existing SIGTERM case without an autocmd already pins the VimLeavePre reject path. The spec has 34 passes on Neovim 0.8.0 and 34 on 0.12.3 (28, and 6 new cases: the two no-answer cases and the nested `:qa!` case, in two layouts).
# Conflicts: # .agents/skills/verify-diff-lifecycle/covered.lock
… two more exits without VimLeavePre The last claim check found that "a `:qa!`, SIGTERM or SIGHUP that Neovim handles while an autocmd that is not `nested` is still running ends Neovim without VimLeavePre" is false when that autocmd has started a `nested = true` handler (with `:doautocmd` or `nvim_exec_autocmds`, as the plugin does for its ClaudeCodeDiff* events) and the handler runs at that moment. Neovim looks only at the `nested` flag of the autocmd that runs when it handles the exit. Then VimLeavePre runs, the calls are answered, and the lock file goes. The rule now says such an exit "can end Neovim without VimLeavePre" in the README, the ARCHITECTURE.md paragraph, the module README and the comment of cleanup_at_exit. The list keeps "a `:qa!` from inside an autocmd that is not nested", the direct case that was probed. The list of exits that run no VimLeavePre gains `:noautocmd qa!` and a Lua `os.exit()`. Probed on Neovim 0.8.0 and 0.12.3 (vertical, horizontal and unified layouts, two waiting calls, 9 runs per row): both run no VimLeavePre and answer no call. A plain `:qa!` and `:cq` run VimLeavePre and answer both calls (9 of 9 each). A non-nested autocmd that starts a `nested = true` handler which waits in vim.wait, with SIGTERM during the wait, runs VimLeavePre and answers both calls (6 of 6 on each version).
… comment of cleanup_at_exit Only the file hash of lua/claudecode/diff.lua changed, by a comment. The model passes.
With eventignore skipping VimLeavePre, a :qa! still rejects diffs while buffers unload. So "Without VimLeavePre" was too general.
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
|
Codex Review: Didn't find any major issues. Keep it up! 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. |
Member
Author
|
@codex review |
Member
Author
|
@codex security review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Keep the exit tests and the "Exit answers" docs true after #558 and #564.
diff_exit_reject_spec.luacame in with fix(diff): answer every openDiff call that is open when Neovim exits #558 and still skipped the unified layout below 0.9 in three places. So 14 unified exit scenarios stayed pending in the v0.8.0 job, and the file kept a comment that is no longer true.VimLeavePre.What changed
tests/integration/diff_exit_reject_spec.lua::qa!in an autocmd that is notnested, and SIGTERM while such an autocmd waits invim.wait: no call is answered. A direct:qa!in anestedautocmd: both calls getDIFF_REJECTED. Each case first checks that both calls were waiting. The two no-answer cases also check that the child ended by itself.README.md, Global Invariants, "Exit answers":"close_window"sentence now says "waiting calls". A call that a nested:qa!cuts off during itsopenDiffsetup has no response sender yet. The server stop answers it with -32603 after theClaudeCodeDiffClosedevents. Every call still gets one answer before close code 1001.VimLeavePrekeeps SIGKILL, a crash and a:qa!from inside an autocmd that is notnested, as on main, and adds:noautocmd qa!and a Luaos.exit(), which also run noVimLeavePre. It adds one rule: a:qa!, SIGTERM or SIGHUP that Neovim handles while an autocmd that is notnestedis still running can end Neovim withoutVimLeavePretoo. A synchronous LSP format on save, which waits invim.wait, is the common example. It says "can" because Neovim looks only at the autocmd that runs when it handles the exit, so anested = truehandler that such an autocmd started (with:doautocmdornvim_exec_autocmds) still getsVimLeavePre. It promises nothing for a signal during a plain Lua loop, because Neovim handles a signal only at its next event check.ARCHITECTURE.md,lua/claudecode/README.mdand the comment ofcleanup_at_exitinlua/claudecode/diff.luasay the same.Scope
Tests and docs. The only change under
lua/is that comment indiff.lua.verify-diff-lifecyclepasses (93,477 distinct states, and 5,557 for the live case), and itscovered.lockrecords the new file hash. The plugin does not answer calls in the exits that the README lists as running noVimLeavePre(SIGKILL, a crash,:noautocmd qa!,os.exit(), and:qa!in an autocmd that is notnested), because none of its exit code runs in them. The docs say so.Blast Radius
None at runtime. The v0.8.0 CI job runs 20 more tests of this spec, and the stable job runs 6 more.
Verification
:qa!replaced by a no-op: the first no-answer case is red on both versions (the file then runs into the runner's 40 s limit, because each affected case waits 20 s).cleanup_at_exitreturns at once: 8 cases are red on both versions, among them the nested:qa!case in the vertical layout. The no-answer cases stay green, because they pin what Neovim does. A Neovim that changes it turns them red, and the README has to change.VimLeavePremarker and the answers in one log). In an autocmd that is notnested, each of these runs noVimLeavePreand answers no call, in all 54 runs per version: a direct:qa!, a timer:qa!duringvim.wait, SIGTERM and SIGHUP duringvim.wait, SIGTERM and SIGHUP in the Lua part of a handler that waits later, SIGTERM in the first of two handlers when the second waits, SIGTERM in a Vim script loop, and SIGTERM in a:%sloop. In anested = trueautocmd the same cases answer both calls. With no autocmd, a timer:qa!, SIGTERM and SIGHUP answer both calls (18 of 18).:noautocmd qa!and a Luaos.exit()from a timer run noVimLeavePreand answer no call (9 of 9 on each version, three layouts), and a plain:qa!and:cqrun it and answer both calls (9 of 9). A non-nested autocmd that starts anested = truehandler which waits invim.wait, with SIGTERM during the wait, runsVimLeavePreand answers both calls (6 of 6 on each version).close_window,:qa!and SIGTERM, three layouts: both answers come before the firstClaudeCodeDiffClosed(6 of 6 on each version). A nested:qa!that cuts a call off in its setup: the -32603 comes after everyClaudeCodeDiffClosedevent (12 of 12 on each version), and no id is answered twice.:qa!case, that the second promisedVimLeavePrefor a signal during a Lua loop, and that "ends" is false when the running autocmd started anestedhandler. The sub-bullet above says only what survives all three, with "can". A last narrow check found only that "WithoutVimLeavePre" was too general, becauseeventignorecan skipVimLeavePreand a:qa!still rejects diffs while buffers unload. The README now says "In these exits".mise run format-checkandmise run allpass (1892 successes, 0 failures, 0 errors, 35 pending), and the integration runner passes on 0.12.3 (844) and on 0.8.0 (840). The commits of this PR change comments, docs, the spec header and the lock only.check.sh --hash-onlypasses for the four verify skills, and the fullverify-diff-lifecyclecheck passes (93,477 and 5,557 states).Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh