WSL port: land checkpoint 7 on main (quit chord + its test) - #15
Merged
Conversation
Ctrl+Shift+Q did nothing on WSL. Nothing automated could have found it: suite_quit.sh writes ESC[113;6u straight onto the pty, so it passes on a terminal where the chord could never arrive, and demo.sh --check cannot send CSI-u over --remote-send at all. It took pressing the key. Windows Terminal 1.24 is new enough to speak CSI-u and does not negotiate it with nvim. So the chord arrives as the legacy 0x11 -- plain <C-q> -- and the <C-S-q> mapping is never reached. The documented way out of the editor did not exist on WSL, leaving `:qa!`: the one thing that whole section was written to stop being necessary. So bind what actually arrives. This is not a second key to learn -- you press Ctrl+Shift+Q, exactly as documented, and <C-q> is what nvim is handed. Insert only, and gated on WSL, because both halves cost something. Normal mode keeps Ctrl+Q as visual block: it is the only way to ask for one, and drill's premise is that you are in insert anyway. In insert, vanilla <C-q> is literal-insert, the twin of <C-v> -- in a Python scratchpad where <C-v> is already paste that is close to unreachable, and a quit prompt defaulting to Cancel is the better use of the key. But it is a real vim behaviour, so nowhere but WSL loses it. Verified both branches by loading the config with the detection forced each way. On WSL insert <C-q> is quit_drill. Forced non-WSL -- what a Mac sees -- insert <C-q> is unmapped, normal <C-q> is <C-V>, insert <C-v> is still paste and <C-S-q> is still quit: byte-for-byte the old behaviour. The WSL probe is hoisted to one IS_WSL near the top, since the clipboard fallback asks the same question. Suite: 556/556, ALL SUITES PASSED. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
Review, and correct. <C-S-q> is bound in n/i/t precisely so you can quit from inside the interpreter without pressing <C-e> first -- KEYS.md advertises it -- and a fallback covering only insert silently drops that on the one platform that needs the fallback at all. Normal mode stays out: there <C-q> is visual block. python has no use for <C-q>, and drill.sh's `stty -ixon` is what frees it from XON to begin with. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
Review on the base PR: the gates named one condition each while the checkpoints named several, so "gate passed" did not mean the checkpoint was done. Spell them out -- including the one doing the least work: "CI green" says nothing about whether xvfb and xclip made the clipboard cases real or whether they were silently skipped, so require that evidence from the log instead. Landed at the tip of the stack rather than on the base branch: TODO.md is edited by every checkpoint, so the same change down there collides with all five checkbox ticks above it for no benefit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
Review: the checkpoint 4 entry still carried the socket/CSI-u explanation that commit "Retract the socket-mode quit skip" explicitly withdrew, so the final diff documented behaviour the final code does not have -- and documented a false cause for it. Rewritten to say what was actually wrong and what the measurement really showed, because "I got this wrong and here is why" is worth more in a checklist than a tidy sentence that is false. Checkpoint 3 was stale the same way: it credited `ps -o comm=`, which was replaced by the source marker after that check turned out to accept `python3 train.py --tag drill-timer`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
Codex review of this branch, three findings, and all three are the docs claiming more than the code does. The P1 is real and I am fixing it in the documentation rather than the binding, because the binding is the lesser evil. On a terminal with no CSI-u, Ctrl+Shift+Q arrives as 0x11 from every mode; the fallback covers insert and terminal but not normal, so from normal mode you get a visual block and no prompt -- verified on a pty, mode goes n -> \x16 and the screen says VISUAL BLOCK. KEYS.md guaranteed normal mode anyway. Binding normal-mode <C-q> to the prompt would fix that and cost more than it buys: <C-q> is the ONLY route to a visual block, because <C-v> in normal mode is paste. Quitting from normal mode still has :qa!; a block would have nothing. So the trade stands and the docs now say so plainly, including what to press instead. Second: docs/wsl.md said a WSL session that does speak CSI-u is unaffected. It is not -- the gate is IS_WSL, not the protocol, because there is no runtime signal for CSI-u (vim.g.termfeatures is nil even in a real TUI on 0.12). Such a session gets the insert binding too, and loses literal-insert with it. Said outright instead of implied away. Third: the page still carried the "--remote-send collapses <C-S-q>" claim that the checkpoint-4 commit in this same stack retracted. Corrected, and the retraction is named so the next reader does not have to rediscover it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
Review: this entry still said demo.sh --check "cannot send CSI-u over --remote-send at all", which is the claim the checkpoint-4 commit in this same stack retracted -- and it contradicted docs/wsl.md two files away. Fourth place that retraction had to be chased down, after the code, the workflow docs and this file's own checkpoint 4. It also said "you press the documented chord and it works" without qualifying it. That is true in insert and terminal mode and false in normal mode, where <C-q> is visual block and stays that way on purpose -- it is the only route to a block, while quitting still has :qa!. Recorded, with what to press instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
Review: the WSL page listed insert and normal mode and left out terminal,
though nvimrc.lua binds <C-q> in {i,t} and KEYS.md advertises quitting from
inside the REPL. Quitting without pressing Ctrl+E first is the reason terminal
mode is in that mode list at all, so leaving it out of the page undersold the
fix. The "two consequences" line had the same omission.
TODO.md also quoted the old "(CSI-u)" check label, which the previous commit
renamed for overstating what --remote-send can prove.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
Two review findings on the last branch in the stack. The fallback had no coverage at all: the 556-case gate never launched the config with WSL detection either way, so binding <C-q> in insert and terminal could have regressed silently. Two assertions now check it from whichever side the machine is on -- a WSL box proves the bindings exist, and CI on ubuntu-latest, where /proc/version has no "microsoft", proves they do not leak onto a normal Linux desktop. Neither machine can prove both halves; between the two runs both are covered. And the comment claimed "python has no use for <C-q>". It does: readline in emacs mode binds ^Q to quoted-insert, so the rebind takes a live feature away. It stays -- ^V is bound to the same command in readline, so quoted-insert survives -- but it is a fair trade, not a free one, and the comment said free. suite_options 86/86, up from 84. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
Bind the quit chord that WSL actually sends
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Review, twice, on the same point I had already fixed in docs/wsl.md and then left standing in two other places. The nvimrc comment said the block is skipped on "any Linux terminal that does negotiate CSI-u", and KEYS.md framed the rebind as a consequence of Windows Terminal not negotiating it. Neither matches the code: the guard is IS_WSL, because there is no runtime signal for whether CSI-u was negotiated. A WSL session whose terminal DOES speak it still gets the binding, and bare Ctrl+Q still quits from insert there. That is a real difference for anyone changing this later -- the old wording invites the assumption that adding a protocol check would be a no-op. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
📝 WalkthroughWalkthroughThe change adds WSL-only ChangesWSL quit-key fallback
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant WindowsTerminal
participant Neovim
participant quit_drill
WindowsTerminal->>Neovim: sends legacy Ctrl+Q
Neovim->>quit_drill: invokes quit confirmation in insert or terminal mode
quit_drill-->>Neovim: returns quit or save decision
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nvimrc.lua`:
- Around line 1274-1288: Update the mode-scope comment above the WSL-gated <C-q>
fallback mappings to describe both insert and terminal modes instead of saying
“INSERT ONLY”; preserve the existing explanation of normal-mode behavior and
ensure it explicitly matches the implementation’s insert/terminal coverage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b2bb22a4-1a9a-4590-a229-24d99340bd17
📒 Files selected for processing (5)
KEYS.mdTODO.mddocs/wsl.mdnvimrc.luatests/suite_options.sh
Review: the block maps <C-q> in {i,t} and the comment above it still said
INSERT ONLY -- left over from the first version, before the terminal-mode gap
was found and fixed. A maintainer trusting the comment would delete the
terminal mapping and silently break quitting from inside the REPL on WSL, which
is exactly the bug that mapping was added to fix.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
All reported issues were addressed across 1 file (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Review: my terminal-mode bullet restated the paragraph directly below it, so the same claim and rationale appeared twice and a future change would have had to edit both or let them drift. Reviewing this block also showed a third paragraph repeating the WSL gating. One block now, three bullets for the three mode choices, and the gating stated once at the end. Verified unchanged after: i=quit_drill t=quit_drill n=<C-V>. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
There was a problem hiding this comment.
No application code in the PR — skipped Code Health checks.
See analysis details in CodeScene
Quality Gate Profile: Clean Code Collective
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
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.
Mechanical follow-up, no new code — this moves already-reviewed, already-merged
work onto
main.What happened
#14 was merged, but into
wsl/06-install-in-place, notmain. GitHubretargets a stacked PR to
mainonly when its base branch is merged anddeleted; #12 merged but left
wsl/06-install-in-placein place, so #14 keptits original base and landed there.
Result: all 8 PRs show MERGED, but
mainis 9 commits short of the finishedport.
Contents
Exactly checkpoint 7 plus its merge commit — the WSL quit chord, its test, and
the documentation corrections:
Every one of these was reviewed on #14 by CodeRabbit, Greptile, cubic, CodeScene
and Codex, with all threads resolved. Nothing is new here.
Verification
./tests/run.shpasses on this branch, including the two newwsl_quit_fallback_*assertions that cover the binding this checkpoint adds.🤖 Generated with Claude Code
https://claude.ai/code/session_01WveBqAyhoM49p9gWfwBDBx
Summary by cubic
Ports checkpoint 7 to
main: makes the quit chord work on WSL by binding<C-q>to the quit prompt in insert and terminal mode, and clarifies the fallback is WSL‑gated. No behavior change off WSL.Bug Fixes
<C-q>to the quit prompt in insert and terminal mode; normal mode stays visual block.<C-q>binding;:qa!remains for normal mode.Tests and Docs
wsl_quit_fallback_*assertions to ensure the binding exists only on WSL.nvimrc.luainto one clear block; documents WSL gating, terminal‑mode coverage, and the normal‑mode trade‑off.Written for commit f276a8e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Ctrl+Qto trigger quit confirmation in insert and terminal modes.Ctrl+Qvisual-block selection behavior in normal mode.Documentation
Tests
Greptile Summary
This PR ports the previously reviewed WSL quit fallback onto
main.<C-q>to the existing quit confirmation in insert and terminal modes on WSL.<C-q>for visual-block selection.Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains.
Important Files Changed
Reviews (4): Last reviewed commit: "Fold three overlapping paragraphs into o..." | Re-trigger Greptile