Vim compat 2/9: make contested key defaults derive from editor_mode — "Vim mode" currently does not mean Vim

[JDonaghy]

Part of the Vim-compatibility chain (issue 2 of 9). Chained after the oracle-harness issue.

Decision this implements

EditorMode { Vim, Vscode } already exists (src/core/settings.rs:20), and editor_mode already defaults to EditorMode::Vim (settings.rs:761).

But editor_mode is referenced zero times in src/core/engine/keys.rs. The contested defaults are unconditional constants:

Setting	Current default	settings.rs
ctrl_f_action	"find" — opens find/replace, even in Normal mode	:335
auto_pairs	true — typing ( in insert yields ()	:355
completion_keys.accept	"Tab" — the popup eats <Tab>	:658

So "Vim mode" is already the shipped default and already does not mean Vim. A user pressing <C-f> to page down gets a find dialog.

Agreed design: mode-derived defaults. Each default_*() becomes a function of EditorMode. Vim mode yields strict Vim values; Vscode mode yields today's IDE values. Every setting stays independently overridable, so :set mode=vim followed by :set auto_pairs=true is a valid, supported combination.

Agreed ship default: EditorMode::Vim — unchanged from today, but now honest.

fn default_ctrl_f_action(mode: EditorMode) -> String {
    match mode {
        EditorMode::Vim    => "page".to_string(),  // <C-f> pages down
        EditorMode::Vscode => "find".to_string(),  // opens find/replace
    }
}

What to do

1. Introduce a mode-aware defaults layer in src/core/settings.rs. Serde's #[serde(default = "...")] cannot see sibling fields, so resolve in two steps: deserialize contested fields as Option<T>, then fill None from the mode in a post-deserialize pass (Settings::apply_mode_defaults(&mut self)), called after load and whenever editor_mode changes at runtime.
2. An explicit user setting must always win over the mode default, including when the mode is changed later. Distinguish "unset, inherit from mode" from "explicitly set to the value that happens to equal a mode default" — otherwise :set mode=vscode silently discards a user's explicit auto_pairs=false.
3. Convert these three to mode-derived: ctrl_f_action, auto_pairs, completion_keys.accept. Later issues in this chain will add more; this issue establishes the mechanism and is deliberately small.
4. :set mode=vim / :set mode=vscode must re-resolve unset fields live, without a restart.
5. Document the mechanism and the full contested-defaults table in docs/PATTERNS.md (it already covers "adding new settings").

Migration

This changes behaviour on upgrade for existing users on the default config: <C-f> stops opening find, ( stops auto-pairing, <Tab> stops accepting the completion popup. Call it out in the release notes with the one-line escape hatch (:set mode=vscode).

Acceptance

• With no config file: mode=vim, ctrl_f_action=page, auto_pairs=false, completion_keys.accept does not capture <Tab>.
• With mode=vscode and nothing else set: all three revert to today's IDE values.
• :set mode=vim + :set auto_pairs=true keeps autopairs on and everything else strict; a later :set mode=vscode does not clobber that explicit true.
• Unit tests in settings.rs for each of the three fields × two modes × (unset, explicitly set).
• A TuiDriver black-box test in src/tui_main/shell_app.rs: in default (Vim) mode, <C-f> scrolls the rendered viewport rather than opening the find overlay. Assert on rendered output, not on a state field.
• docs/PATTERNS.md updated.

[JDonaghy]

Coordinator Assignment

Machine: dell64
Repo: vimcode
Files: (unspecified)
Do not touch: (nothing else in flight)

Briefing

Issue #800: Vim compat 2/9: make contested key defaults derive from editor_mode — "Vim mode" currently does not mean Vim

Part of the Vim-compatibility chain (issue 2 of 9). Chained after the oracle-harness issue.

Decision this implements

EditorMode { Vim, Vscode } already exists (src/core/settings.rs:20), and editor_mode already defaults to EditorMode::Vim (settings.rs:761).

But editor_mode is referenced zero times in src/core/engine/keys.rs. The contested defaults are unconditional constants:

Setting	Current default	settings.rs
ctrl_f_action	"find" — opens find/replace, even in Normal mode	:335
auto_pairs	true — typing ( in insert yields ()	:355
completion_keys.accept	"Tab" — the popup eats <Tab>	:658

So "Vim mode" is already the shipped default and already does not mean Vim. A user pressing <C-f> to page down gets a find dialog.

Agreed design: mode-derived defaults. Each default_*() becomes a function of EditorMode. Vim mode yields strict Vim values; Vscode mode yields today's IDE values. Every setting stays independently overridable, so :set mode=vim followed by :set auto_pairs=true is a valid, supported combination.

Agreed ship default: EditorMode::Vim — unchanged from today, but now honest.

fn default_ctrl_f_action(mode: EditorMode) -> String {
    match mode {
        EditorMode::Vim    => "page".to_string(),  // <C-f> pages down
        EditorMode::Vscode => "find".to_string(),  // opens find/replace
    }
}

What to do

1. Introduce a mode-aware defaults layer in src/core/settings.rs. Serde's #[serde(default = "...")] cannot see sibling fields, so resolve in two steps: deserialize contested fields as Option<T>, then fill None from the mode in a post-deserialize pass (Settings::apply_mode_defaults(&mut self)), called after load and whenever editor_mode changes at runtime.
2. An explicit user setting must always win over the mode default, including when the mode is changed later. Distinguish "unset, inherit from mode" from "explicitly set to the value that happens to equal a mode default" — otherwise :set mode=vscode silently discards a user's explicit auto_pairs=false.
3. Convert these three to mode-derived: ctrl_f_action, auto_pairs, completion_keys.accept. Later issues in this chain will add more; this issue establishes the mechanism and is deliberately small.
4. :set mode=vim / :set mode=vscode must re-resolve unset fields live, without a restart.
5. Document the mechanism and the full contested-defaults table in docs/PATTERNS.md (it already covers "adding new settings").

Migration

This changes behaviour on upgrade for existing users on the default config: <C-f> stops opening find, ( stops auto-pairing, <Tab> stops accepting the completion popup. Call it out in the release notes with the one-line escape hatch (:set mode=vscode).

Acceptance

• With no config file: mode=vim, ctrl_f_action=page, auto_pairs=false, completion_keys.accept does not capture <Tab>.
• With mode=vscode and nothing else set: all three revert to today's IDE values.
• :set mode=vim + :set auto_pairs=true keeps autopairs on and everything else strict; a later :set mode=vscode does not clobber that explicit true.
• Unit tests in settings.rs for each of the three fields × two modes × (unset, explicitly set).
• A TuiDriver black-box test in src/tui_main/shell_app.rs: in default (Vim) mode, <C-f> scrolls the rendered viewport rather than opening the find overlay. Assert on rendered output, not on a state field.
• docs/PATTERNS.md updated.

[JDonaghy]

Coordinator: Assignment Complete

Machine: dell64
Status: done
Exit code: 0
Duration: 13m 27s
Log: /home/john/.coord/logs/4c3765665a63.log

[JDonaghy]

Coordinator Assignment

Machine: dell64
Repo: vimcode
Files: (unspecified)
Do not touch: (nothing else in flight)

Briefing

You are fixing a failed smoke test for issue #800: Vim compat 2/9: make contested key defaults derive from editor_mode — "Vim mode" currently does not mean Vim

The previous worker created branch issue-800-vim-compat-2-9-make-contested-key-defaul. You are already on that branch.
Do NOT start over — work from the existing code.

What was done

The previous worker's changes are already committed on this branch.
Run git fetch origin && git log --oneline origin/develop..HEAD to see what was done.
Run git diff origin/develop...HEAD to see the full diff.

Test failure

cargo test fails to compile: tests/ide_polish.rs uses e.settings.auto_pairs as bool but branch changed it to Option (E0308/E0600 x4)

Guidance

Fix the failing tests and push.

Rules

• Do NOT start over or rewrite from scratch
• Fix the specific test failures
• Commit your fixes and push with git push origin HEAD

[JDonaghy]

Coordinator: Assignment Complete

Machine: dell64
Status: done
Exit code: 0
Duration: 2m 52s
Log: /home/john/.coord/logs/8ed3cd1c2ce3.log

[JDonaghy]

Coordinator: Assignment Complete

Machine: dell64
Status: done
Exit code: 0
Duration: 6m 36s
Log: /home/john/.coord/logs/e9063ca1b5b8.log

[JDonaghy]

Coordinator: Assignment Complete

Machine: dell64
Status: done
Exit code: 0
Duration: 3m 46s
Log: /home/john/.coord/logs/143b475da6ed.log

[JDonaghy]

Coordinator: Assignment Complete

Machine: precision
Status: done
Exit code: 0
Duration: —

Summary

Verdict: request-changes

Review findings

Blocking findings

• completion_keys.accept is fully "mode-derived" at the settings layer (default flips between <C-y> in Vim and Tab in Vscode, with correct Option<T>/accessor/override-survives-mode-switch semantics, and thorough unit tests), but the actual key-dispatch code never reads this setting, so the behavior change does not exist. src/core/engine/keys.rs:4737-4744 (handle_insert_key, the "Tab: accept display-only popup" block) gates acceptance purely on !ctrl && key_name == "Tab" && self.completion_display_only — it does not call self.settings.completion_keys.accept(self.settings.editor_mode) anywhere. I confirmed via git grep on the PR branch tip (FETCH_HEAD 4d4a334) that completion_keys.accept( is called nowhere in src/ outside settings.rs itself and its own unit tests. So in the shipped default Vim mode, pressing <Tab> still accepts the completion popup exactly as before this PR — the issue's own stated defect ("the popup eats <Tab>") and acceptance criterion ("completion_keys.accept does not capture <Tab>") are unmet. Worse, the Vim-mode default this PR introduces, <C-y>, is already bound to an unrelated, pre-existing action — "insert character from line above cursor" (keys.rs:5028-5029, does not check completion_display_only) — so setting completion_keys.accept to <C-y> doesn't even give users a working accept-key substitute; it collides silently with an existing binding.
• Because this behavior is unimplemented, there is (correctly, since there is nothing to test) no TuiDriver/black-box test asserting on rendered output that <Tab> no longer accepts the completion popup in default Vim mode, unlike the ctrl_f_action change which does get one (ctrl_f_pages_down_the_viewport_in_default_vim_mode_via_shell_app in src/tui_main/shell_app.rs). Per this repo's CLAUDE.md ("Black-box coverage is the acceptance bar … MANDATORY … rejects behaviour-changing PRs that lack one"), a PR that presents a setting as changing user-visible key behavior (see the PATTERNS.md table and the issue's Migration section: "<Tab> stops accepting the completion popup") must ship a rendered-output test proving it — and here it can't, because the wiring doesn't exist. This needs either (a) wiring handle_insert_key's Tab/accept-key check to consult self.settings.completion_keys.accept(...) (and presumably add the same check for the configured accept key when it isn't Tab, e.g. recognizing <C-y> for accept while not clobbering the existing Ctrl-Y line-above binding — worth a design discussion), plus a driver test proving it, or (b) descoping completion_keys.accept out of this PR/issue's "done" set and being explicit that only the settings-resolution mechanism landed for it, not the behavior.

Non-blocking concerns

• The issue's Migration section asks to "call it out in the release notes with the one-line escape hatch (:set mode=vscode)" — there is no CHANGELOG.md/release-notes file in this repo to update, so this presumably needs to happen in the PR description or wherever this project tracks release notes (not visible to me in the diff). Worth confirming it was done somewhere, since three defaults are changing for every upgrading user on this PR (well, two of three — see blocking finding above).
• ctrl_f_action and auto_pairs are correctly wired end-to-end: every read site in src/core/engine/keys.rs and src/core/engine/vscode.rs was updated to call the new accessor methods, the :set/:set ...? read and write paths in settings.rs correctly box into Some(...), and the acceptance-criteria scenarios (no config file → strict Vim; mode=vscode with nothing else set → IDE values; explicit override survives a later mode switch) all have direct unit-test coverage. The new EditorMode: Copy derive is a reasonable, low-risk addition to support passing the enum by value into the parameterized default functions.
• The ctrl_f_action TuiDriver test (shell_app.rs) looks solid: it asserts on rendered content (screen_contains, find_bounds("Aa")) rather than engine state, and the doc comment explicitly states it was verified RED against unfixed develop, matching the CLAUDE.md black-box rules.
• Pinned context for this review flagged a compile failure in tests/ide_polish.rs (auto_pairs used as bool vs. the branch's new Option<bool>, E0308/E0600 x4). I verified this was already fixed by a follow-up commit already on the PR branch (4d4a334 #800: fix tests/ide_polish.rs for the Option<bool> auto_pairs field, on top of 8709045), and confirmed via git grep across the branch tip that no other file references .auto_pairs, .ctrl_f_action, or .completion_keys.accept in a way incompatible with the new Option<T> types. This concern appears resolved in the diff under review.

Nits

• docs/PATTERNS.md's new "Mode-Derived Contested Defaults" table and the Settings::completion_keys.accept doc comment both describe the Vim-mode <C-y> default as though it actively "leaves <Tab> alone" / is a working accept key — given the blocking finding above, this documentation currently overstates what the shipped code does for this one field. Worth a follow-up correction once the wiring lands (or a caveat now).

[JDonaghy]

Review Complete — ⚠️ Changes Requested

Reviewer findings could not be posted directly to PR #809 (gh pr review was rejected — likely a self-review restriction). Findings are reproduced here.

Note: a completion comment was posted earlier but findings could not be extracted at that time. These are the retroactive findings.

Blocking findings

• completion_keys.accept is fully "mode-derived" at the settings layer (default flips between <C-y> in Vim and Tab in Vscode, with correct Option<T>/accessor/override-survives-mode-switch semantics, and thorough unit tests), but the actual key-dispatch code never reads this setting, so the behavior change does not exist. src/core/engine/keys.rs:4737-4744 (handle_insert_key, the "Tab: accept display-only popup" block) gates acceptance purely on !ctrl && key_name == "Tab" && self.completion_display_only — it does not call self.settings.completion_keys.accept(self.settings.editor_mode) anywhere. I confirmed via git grep on the PR branch tip (FETCH_HEAD 4d4a334) that completion_keys.accept( is called nowhere in src/ outside settings.rs itself and its own unit tests. So in the shipped default Vim mode, pressing <Tab> still accepts the completion popup exactly as before this PR — the issue's own stated defect ("the popup eats <Tab>") and acceptance criterion ("completion_keys.accept does not capture <Tab>") are unmet. Worse, the Vim-mode default this PR introduces, <C-y>, is already bound to an unrelated, pre-existing action — "insert character from line above cursor" (keys.rs:5028-5029, does not check completion_display_only) — so setting completion_keys.accept to <C-y> doesn't even give users a working accept-key substitute; it collides silently with an existing binding.
• Because this behavior is unimplemented, there is (correctly, since there is nothing to test) no TuiDriver/black-box test asserting on rendered output that <Tab> no longer accepts the completion popup in default Vim mode, unlike the ctrl_f_action change which does get one (ctrl_f_pages_down_the_viewport_in_default_vim_mode_via_shell_app in src/tui_main/shell_app.rs). Per this repo's CLAUDE.md ("Black-box coverage is the acceptance bar … MANDATORY … rejects behaviour-changing PRs that lack one"), a PR that presents a setting as changing user-visible key behavior (see the PATTERNS.md table and the issue's Migration section: "<Tab> stops accepting the completion popup") must ship a rendered-output test proving it — and here it can't, because the wiring doesn't exist. This needs either (a) wiring handle_insert_key's Tab/accept-key check to consult self.settings.completion_keys.accept(...) (and presumably add the same check for the configured accept key when it isn't Tab, e.g. recognizing <C-y> for accept while not clobbering the existing Ctrl-Y line-above binding — worth a design discussion), plus a driver test proving it, or (b) descoping completion_keys.accept out of this PR/issue's "done" set and being explicit that only the settings-resolution mechanism landed for it, not the behavior.

Non-blocking concerns

• The issue's Migration section asks to "call it out in the release notes with the one-line escape hatch (:set mode=vscode)" — there is no CHANGELOG.md/release-notes file in this repo to update, so this presumably needs to happen in the PR description or wherever this project tracks release notes (not visible to me in the diff). Worth confirming it was done somewhere, since three defaults are changing for every upgrading user on this PR (well, two of three — see blocking finding above).
• ctrl_f_action and auto_pairs are correctly wired end-to-end: every read site in src/core/engine/keys.rs and src/core/engine/vscode.rs was updated to call the new accessor methods, the :set/:set ...? read and write paths in settings.rs correctly box into Some(...), and the acceptance-criteria scenarios (no config file → strict Vim; mode=vscode with nothing else set → IDE values; explicit override survives a later mode switch) all have direct unit-test coverage. The new EditorMode: Copy derive is a reasonable, low-risk addition to support passing the enum by value into the parameterized default functions.
• The ctrl_f_action TuiDriver test (shell_app.rs) looks solid: it asserts on rendered content (screen_contains, find_bounds("Aa")) rather than engine state, and the doc comment explicitly states it was verified RED against unfixed develop, matching the CLAUDE.md black-box rules.
• Pinned context for this review flagged a compile failure in tests/ide_polish.rs (auto_pairs used as bool vs. the branch's new Option<bool>, E0308/E0600 x4). I verified this was already fixed by a follow-up commit already on the PR branch (4d4a334 #800: fix tests/ide_polish.rs for the Option<bool> auto_pairs field, on top of 8709045), and confirmed via git grep across the branch tip that no other file references .auto_pairs, .ctrl_f_action, or .completion_keys.accept in a way incompatible with the new Option<T> types. This concern appears resolved in the diff under review.

Nits

• docs/PATTERNS.md's new "Mode-Derived Contested Defaults" table and the Settings::completion_keys.accept doc comment both describe the Vim-mode <C-y> default as though it actively "leaves <Tab> alone" / is a working accept key — given the blocking finding above, this documentation currently overstates what the shipped code does for this one field. Worth a follow-up correction once the wiring lands (or a caveat now).