fix(cursor): clamp Windows legacy-console cursor moves at the buffer edge

[hdimer]

Closes #1094.

The bug

move_up, move_left and move_to_previous_line in src/cursor/sys/windows.rs
subtract the caller's count from the current row/column with a bare u16
subtraction and no floor:

move_to(column, row - count)?;      // move_up
move_to(column - count, row)?;      // move_left
move_to(0, row - count)?;           // move_to_previous_line

When count exceeds the current row/column that underflows: attempt to subtract with overflow in a debug build, or a wrap to a large u16 in release. This is the
legacy-console path only, reached through execute_winapi when supports_ansi is
false, so it does not affect the ANSI path.

MoveUp(n) with n greater than the current row is not really a caller error, it
is the ordinary "get back to the top of the block I just drew" idiom, and it does
the right thing everywhere except here.

Scope: three sites, not the two in the issue

#1094 names move_up and move_left and says move_to_previous_line is already
guarded. It isn't. MoveToPreviousLine::execute_winapi in src/cursor.rs only
checks if self.0 != 0, which skips the zero case, not the count > row case, so
sys::move_to_previous_line underflows the same way. All three are fixed here.

One correction to the issue while I'm at it: the release-mode outcome is not
uniformly the Argument Out of Range error it describes. That happens for counts
in 1..=32768, where the wrapped value casts to a negative i16 and trips the
guard in ScreenBufferCursor::move_to. A larger count wraps to a positive
i16, sails past the guard, and fails later against the real buffer bounds with a
different OS error. Either way it is wrong, but the error you get varies.

Why clamp

execute_winapi exists to emulate the ANSI sequence on legacy conhost, so the ANSI
branch is the spec. MoveUp/MoveLeft/MoveToPreviousLine emit CUU/CUB/CPL, which
terminals clamp at the top row and first column. Returning an error would make the
WinAPI path diverge from the branch it is imitating, and doing nothing would be
wrong on its own terms (moving up 5 from row 2 should land on row 0, not stay on
row 2). saturating_sub is also the primitive the crate already picked for this
exact bug class in src/event/sys/unix/parse.rs.

What is deliberately not in here

move_down, move_right and move_to_next_line add rather than subtract, so they
can overflow rather than underflow. I left them alone on purpose: saturating_add
would clamp at u16::MAX, which the as i16 cast turns straight back into an
error, so it converts a panic into a different failure rather than fixing anything.
A real fix there needs the screen buffer bounds and a decision about whether to
clamp to the buffer or the window, which is a separate question from this one.

Tests

The three existing tests moved back by the exact count (move_to(0, 2) then
move_up(2)), so they landed on zero without ever crossing it and never touched
the bug. Each now also overshoots. The overshoot deliberately starts from a
non-zero position rather than from the origin: starting at zero, "clamp at the
edge" and "do nothing" are indistinguishable, so such a test would still pass if
the fix were later replaced with checked_sub(count).unwrap_or(row). Starting two
rows down and asking for five pins the clamp itself.

Verification

This is cfg(windows) code needing a real console handle, and I am on macOS, so
the three tests here run on your Windows CI, not on my machine. What I did check
locally:

• cargo clippy --locked --target x86_64-pc-windows-msvc --all-targets --all-features -- -D warnings, clean. I confirmed --all-targets really does type-check the #[cfg(test)] module for that target by injecting a deliberate type error into one of the new test lines and watching it fail.
• rustup run 1.85.0 cargo check --locked --lib --all-features --target x86_64-pc-windows-msvc, clean, plus both host MSRV configurations.
• Host side: cargo fmt --check, host clippy, cargo test --locked --all-targets --all-features -- --test-threads 1 (121 passed), doc tests, cargo doc with RUSTDOCFLAGS=-D warnings, cargo package.
• To get an actual red/green cycle out of unrunnable code, I mirrored the three
  functions over a stub cursor and ran the new assertions against three
  implementations: the current bare subtraction fails all three (panic in debug,
  error in release), checked_sub(..).unwrap_or(..) fails all three, and
  saturating_sub passes in both profiles.

I did not run cargo deny, actionlint or zizmor; this touches no dependency
and no workflow file.

I used an AI assistant while working on this. The reasoning above, the scope
correction and the test design are mine and I have checked them; the diff is small
enough to read in a minute either way.

Disclosure: this change was prepared with AI assistance (Claude Code). The repro and tests described above were run before it was opened.

[joshka]

Thanks for the PR. I understand there's concerns about i16 casts and that's why you skipped the move_right/down fixes. I think it's worth getting this right in a single PR rather than two PRs or skipping the overflow condition here.

Would you mind taking a look and expanding on this?

[joshka]

This seems like it would similarly overflow - same move_down.

[hdimer]

Done in 4ebbaec — move_right, move_down and move_to_next_line clamp too now.

The clamp is at the last cell of the screen buffer, not the window. That is the bound SetConsoleCursorPosition actually validates against, and it means only calls that already failed change behaviour: MoveDown(1000) in a 9001-row buffer works today and still works. Clamping at the viewport would have capped that at row 29 and broken a working case, which felt like more than a bug fix. It is arguably the less faithful choice — CUD on a real terminal stops at the bottom of the screen, not the bottom of the scrollback — so if you would rather have the window bound, say so and I will switch it.

That also settles the i16 worry: buffer dimensions come out of dwSize as i16, so a clamped target can never reach move_to with a value that casts negative. saturating_add on its own would not have done it — u16::MAX casts to -1 and just trades the panic for an error — which is why I left the addition side out the first time rather than half-fixing it.

One behaviour change beyond the overflow, worth flagging: MoveRight(200) from column 10 of a 120-column buffer never overflowed anything, it was simply rejected by the OS call and returned Err. It now lands on column 119. That is what clamping means, but it is wider than "stop the panic", so I have said it in the changelog entry too.

Each of the three tests grew an overshoot case. They assert the cursor moved past where it started rather than a fixed index: CI's buffer size is not knowable from here, and conhost scrolls the window when the cursor goes below it, after which position() reports a window-relative row. Weaker fixes still fail them — no-op-on-overflow fails the >, plain saturating_add and an off-by-one ceiling both blow up in the OS call.

Verification as before: the tests run on your Windows CI, not here. Locally, cross-target clippy with --all-targets (again confirming the cfg(test) module really compiles for that target by injecting a type error and watching it fail), the 1.85 check, host fmt/clippy/suite (121 pass), cargo doc. And the stub mirror again, run against the unfixed adds this time: panic in debug, wrong position in release.

Two things next door that I found and left alone: parse_relative_y can hand back a window-relative row where move_to wants an absolute one, so a scrolled-back window can still produce Argument Out of Range on the untouched axis; and scroll_up/scroll_down in terminal/sys/windows.rs cast the row count to i16, so a large value goes negative and scrolls the wrong way. Different bugs, happy to open issues if useful.