vi: cursor position semantics are not mode-aware (root cause of multiple off-by-one bugs)

[sim590]

Problem

Reedline's LineBuffer::insertion_point is a plain byte offset with no mode-aware bounds checking. In Vi, the two modes have fundamentally different cursor semantics:

• Insert mode — cursor is between characters; valid range is 0..=len()
• Normal mode — cursor is on a character; valid range is 0..last_grapheme_start

Reedline makes no such distinction. The cursor can always reach len() regardless of mode, causing a cascade of off-by-one bugs.

Affected issues

All of the following share this single root cause:

• vi mode cursor position isn't vi-like #694 — Escape doesn't move cursor back; cursor can go past last character in normal mode
• Move(Big)WordRightEnd is off-by-1 in insert mode #766 — MoveWordRightEnd is off-by-1 in insert mode
• vi keybinds are wrong and/or inconsistent with readline #788 — Multiple Vi keybinding inconsistencies (pts 1, 4, 5, 6, 7)
• Line editor selects two characters instead of one #893 — Shift+arrow selects two characters instead of one
• Prefix history search broken in vi-mode #772 — Prefix history search broken after Move left when exiting insert mode #699 (which was a partial fix for the same issue)

Affected code paths

Every function that can place the cursor at len() is incorrect when in Vi normal mode:

Function	File	Problem
set_insertion_point	line_buffer.rs:71	No bounds check
move_to_end / move_to_line_end	line_buffer.rs:132-139	Always goes to len()
move_right	line_buffer.rs:453	l can move past last character
set_buffer	line_buffer.rs:81	After history load, cursor lands at len()
All *_word_right_*	line_buffer.rs	Fallback to len() instead of last grapheme
previous_history	engine.rs:1548	Calls move_to_end() after loading history

Proposed approaches

Approach A — Centralized clamping (less invasive)

Add a clamp_cursor_for_vi_normal() method in Editor and call it from run_edit_command() whenever the mode is Vi normal, and after history navigation / buffer changes in the engine.

fn clamp_cursor_for_vi_normal(&mut self) {
    let len = self.get_buffer().len();
    if !self.get_buffer().is_empty() && self.insertion_point() >= len {
        let last = self.line_buffer.grapheme_left_index_from_pos(len);
        self.line_buffer.set_insertion_point(last);
    }
}

Pros: minimal API changes, single enforcement point, fixes all symptoms at once
Cons: requires careful placement at every entry point (history load, buffer set, etc.) — risk of missing a spot

Approach B — Mode-aware movement functions (more invasive)

Pass the current PromptEditMode to each movement function so they bound themselves:

pub fn move_right(&mut self, mode: PromptEditMode) {
    let next = self.grapheme_right_index();
    let limit = match mode {
        PromptEditMode::Vi(PromptViMode::Normal) =>
            self.grapheme_left_index_from_pos(self.lines.len()),
        _ => self.lines.len(),
    };
    self.insertion_point = next.min(limit);
}

Pros: impossible to miss a case — correctness is enforced at the source
Cons: large API change across LineBuffer, Editor, and all callers

Question for maintainers

@blindFS @ysthakur @fdncred — which approach aligns better with the direction you have in mind for reedline? Or is there a third path you'd prefer? We'd like to implement this properly rather than continuing with piecemeal fixes.

[fdncred]

Seems like mode-aware might be better so then, vi can do one things, emacs another, and helix yet another.

[sim590]

@fdncred Quick question about your preference for the mode-aware approach: would a centralized clamp_cursor() method in Editor — matching on PromptEditMode after each operation — be sufficient, or do you want the mode passed directly to each LineBuffer movement function signature?

[fdncred]

You might want to chat with @schlich and @kronberger-droid since they're working on the Helix mode to come up with an equitable solution since Helix mode will need something too.

[sim590]

@fdncred Thanks for the pointer. Here's what I've been working on. Would appreciate feedback from @kronberger-droid and @schlich as well.

I've introduced a CursorMode enum at the LineBuffer level:

pub enum CursorMode {
    Insert,  // cursor between characters (Emacs, Vi insert, Helix insert)
    Normal,  // cursor on a character (Vi normal, Helix normal)
}

Movement functions in LineBuffer now take this parameter and bound the cursor accordingly:

pub fn move_right(&mut self, mode: CursorMode) { ... }
pub fn move_to_end(&mut self, mode: CursorMode) { ... }
pub fn move_to_line_end(&mut self, mode: CursorMode) { ... }
// etc.

Editor converts from PromptEditMode to CursorMode via a cursor_mode() method, keeping LineBuffer decoupled from prompt/mode concerns.

Relationship with #1059 (WordFlavor / motion-operator decoupling)

CursorMode and the WordFlavor parameterization discussed in #1059 are orthogonal: they answer different questions:

• WordFlavor: which word boundary algorithm to use (Emacs UAX#29, Vi three-class, Helix three-class+Eol)
• CursorMode: where the cursor may land after a movement (can it go past the last character?)

These operate at different layers and can coexist without conflict. A future call might look like:

move_word_right(flavor, cursor_mode)

Does this approach work for the Helix mode needs? Happy to adjust.

[kronberger-droid]

Hey @sim590, your root-cause framing is right, but I'd push it one layer further.

The cursor invariant isn't a buffer concern.
The buffer doesn't know what mode it's in, and shouldn't have to.

So Approach B feels wrong to me, since passing PromptEditMode into LineBuffer::move_right pushes mode-awareness deeper into the buffer.

Approach A is the right idea in the wrong location.
A ClampCursorToGrapheme primitive on EditCommand is fine, but what matters is who emits it.
That should be the mode, at its own boundary points. The buffer stays dumb, the mode owns the invariant.

Worth flagging long-term: LineBuffer::insertion_point only models a cursor between graphemes, which fits insert mode and emacs but not vi normal's "cursor on a grapheme" or helix's range model.

[sim590]

@kronberger-droid Thanks for the pushback, that's a helpful reframe.

Based on your comment, here is what I'd call Approach C: keep LineBuffer fully mode-agnostic and introduce EditCommand::ClampCursorToNormalMode as a primitive. Editor implements it as "if the current mode is Vi normal and the cursor is past the last grapheme, move left one grapheme." The Vi mode owns its own invariant and decides when to emit it.

Two sub-options for emission:

C1 -- Per-command: The Vi normal mode parser emits ClampCursorToNormalMode explicitly after each relevant command:

// x in normal mode
vec![
    ReedlineOption::Edit(EditCommand::CutChar),
    ReedlineOption::Edit(EditCommand::ClampCursorToNormalMode),
]

Simple and transparent, but requires discipline: any future command added to normal mode must remember to include it.

C2 -- Automatic wrapping at the mode boundary: A helper on Vi appends ClampCursorToNormalMode to any ReedlineEvent that contains edit commands:

fn with_normal_mode_clamp(event: ReedlineEvent) -> ReedlineEvent {
    match event {
        ReedlineEvent::Edit(mut cmds) => {
            cmds.push(EditCommand::ClampCursorToNormalMode);
            ReedlineEvent::Edit(cmds)
        }
        ReedlineEvent::Multiple(mut events) => {
            events.push(ReedlineEvent::Edit(vec![
                EditCommand::ClampCursorToNormalMode,
            ]));
            ReedlineEvent::Multiple(events)
        }
        other => other,
    }
}

All normal-mode dispatch paths in parse_event pass their result through this helper. New commands get the behavior automatically. Transitions to insert mode are safe: since ClampCursorToNormalMode checks editor.edit_mode at execution time, it is a no-op once the mode has already changed.

C2 seems preferable since it centralizes the invariant at the mode boundary and eliminates the risk of forgetting.

Does this align with what you had in mind?

[kronberger-droid]

@sim590 Approach C is the right shape i think.
But C1 vs C2 feels like a maintainer call (@fdncred), so I won't push either way.

One concern i still have about the wrapping pattern:

Both variants are post-hoc clamps, since cursor lands invalid and gets corrected after.
That covers keystroke paths because everything flows through parse_event, but not history load, completion accept, paste, or external set_buffer.

The structural fix is the thing I flagged already:
insertion_point: usize can't model vi-normal's "on a grapheme" or helix's range.
A Cursor { anchor, head } on grapheme boundaries generalizes all three modes (helix is the free case, vi-normal is width-1, insert is anchor == head).
The vi-normal constructor caps head at the last grapheme start, so len() is unrepresentable rather than something to clamp.

It's a real refactor, since LineBuffer loses ownership of the cursor, every insertion_point() callsite migrates, and it's a public API break.
I don't want to propose it as a blocker for the PR. But helix mode is going to force this question anyways.

[fdncred]

I'd vote for whichever is the easiest to maintain. I'm guessing that may be C1?

[sim590]

@fdncred C1 it is.

One thing worth addressing alongside it, raised by @kronberger-droid: C1 only covers the keystroke path through parse_event. History load, completion accept, paste, and external set_buffer calls can also leave the cursor past the last grapheme in normal mode. Approach A had targeted guards at those entry points (set_buffer, move_to_end, move_to_line_end). I'd combine C1 with those guards so all paths are covered.

Does that sound reasonable?

[fdncred]

ok, sounds reasonable.