fix(terminal): clamp DCH delete count to cells after the cursor#12906
fix(terminal): clamp DCH delete count to cells after the cursor#12906obchain wants to merge 1 commit into
Conversation
The DCH handler (`CSI Pn P`, `delete_chars`) clamped the delete count to the full row width (`min(count, cols)`) instead of the cells remaining from the cursor to the end of the line. When `Pn` exceeded `cols - cursor_col`, the trailing clear `let end = cols - count` fell before the cursor column, so `row[end..]` wiped cells to the left of the cursor — corrupting characters that DCH must never touch. For a 5-column row holding `abcde`, `CSI 4 G` + `CSI 10 P` cleared the entire row instead of leaving `abc` and blanking only the two cells under and after the cursor. Clamp the count to `cols - start`, mirroring the bound `insert_blank` already uses. Add a regression test covering a delete count larger than the cells remaining to end-of-line. Closes warpdotdev#12820
github-actions
Bot
added
the
external-contributor
|
/oz-review |
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR clamps DCH (CSI Pn P) deletion to the cells from the cursor through the end of the row and adds a focused regression test for counts larger than the remaining line width. The implementation matches the intended terminal-grid behavior in the provided diff, and I did not find security issues or material spec drift.
Concerns
- For this user-facing terminal behavior change, please include screenshots or a short screen recording demonstrating the
printf 'abcde\r\033[3C\033[10P'repro working end to end in Warp. The PR description has the visual evidence checkbox unchecked, and textual/unit-test verification does not replace visual evidence under the repository review guidance.
Verdict
Found: 0 critical, 1 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
1 participant
Description
The DCH handler (
CSI Pn P,delete_charsinapp/src/terminal/model/grid/ansi_handler.rs) erased characters before the cursor whenPnwas larger than the number of cells remaining from the cursor to the end of the line.DCH must only delete the character under the cursor and those to its right (shifting the remainder left); cells before the cursor must never be touched. The handler clamped the count to the full row width (
min(count, cols)) instead ofcols - cursor_col, so the trailing clearlet end = cols - count;fell beforestart, androw[end..]wiped cells to the left of the cursor.Deterministic repro (5-column row, ASCII only): write
abcde, move the cursor to column 3 (CSI 4 G), emitCSI 10 P(10 > the 2 cells from the cursor to end-of-line). Expected: columns 0–2 keepabc, columns 3–4 blank. Actual: the entire row was cleared, destroyingabc. From a shell:printf 'abcde\r\033[3C\033[10P'.The fix clamps the count to
cols - start, the same bound the siblinginsert_blankalready uses.CHANGELOG-BUG-FIX: Fixed a terminal grid corruption where a Delete Character (DCH /
CSI Pn P) escape with a count larger than the cells remaining to end-of-line erased characters before the cursor instead of only those under and after it.Linked Issue
Closes #12820
ready-to-specorready-to-implement.Testing
Added
test_delete_chars_count_exceeding_end_of_line_preserves_cells_before_cursor, a deterministic unit test againstGridHandlerthat reproduces the issue's repro: it writesabcdeinto a 1×5 grid, moves the cursor to column 3, callsdelete_chars(10), and asserts columns 0–2 still holdabcwhile columns 3–4 are blanked. Before the fix the whole row was cleared (the first assertion got'\0'instead of'a').This is pure terminal grid logic with no rendering component, so the unit test fully exercises the fix end to end. I wasn't able to run a full
./script/runbuild on this machine (the client's Metal-shader build step requires the full Xcode toolchain, which isn't available in my environment), so the deterministic grid test is the verification here; CI runs the full suite../script/runAgent Mode