fix: resolve high-severity terminal correctness bugs - #61
Conversation
Ground-state input is UTF-8, where 0x80..0x9f only occur as continuation bytes. Honouring a lone 0x9b/0x9d/0x90/0x9f there as an 8-bit CSI/OSC/DCS/ APC introducer let one stray byte from a damaged multi-byte sequence open a control string and swallow the visible text that followed until an ST happened to arrive. Such bytes now fall through to the UTF-8 decoder and print U+FFFD; C1 controls are still recognised once a sequence is already in progress (ST terminating a string, for example).
…ibutes `38:2:r:g:b;1` is one colon-delimited group followed by a separate `;1`. The extended-colour decoder read its slots by position, so an incomplete colon group (`38:2:255;1`, `38:5;1`) consumed the next attribute as a colour component and dropped it, and the colorspace-less colon truecolor spelling swallowed the bold that followed it. The group now ends at the first semicolon; a group too short for its colour kind yields no colour rather than a garbage one. The lone-truecolor fast path is limited to the semicolon spelling because it cannot see where a colon group ends.
DECOM (?6) was tracked but never applied: CUP/CHA/VPA, the homing done by DECSTBM/DECSLRM/DECOM itself, and the CPR report all used absolute coordinates, so full-screen programs that address a scroll region in origin mode painted outside it. Origin mode is saved and restored with the cursor (DECSC/DECRC, ?1048, ?1049) as in xterm, so a restore reinstates the addressing mode the position was saved under. The synthetic seed replays that state so a restored terminal homes and reports identically. The scroll_region_origin parity fixture is re-blessed; its previous expectation recorded this gap as known-divergent.
IRM (CSI 4 h) was accepted as a mode but printing always overwrote, so programs that rely on insert mode to open space in a line rather than repaint it lost the characters under the cursor. The ASCII batch fast paths write cells in place, so they are bypassed while the mode is on; REP takes the same per-cell path. The mode is also answered by DECRQM and replayed by the synthetic seed.
With left/right margins set, insert/delete character shifted the row all the way to its last column, line insert/delete and scroll moved whole rows, and TAB/CBT ran past the margins, so a program using DECSLRM for a side pane saw the other pane's content pushed around. Row moves inside narrowed margins copy the margin columns cell by cell instead of rotating rows, leaving the columns outside untouched. Cursor addressing still clamps to the margins, so the cursor cannot currently sit left of the left margin or on a row outside the region; a marker in insert_blank_chars notes the guard needed if that changes.
Shrinking the row count removed rows from the bottom whenever they were below the cursor, regardless of content, so text under the cursor was discarded instead of the top rows moving into scrollback. Only blank, unwrapped rows not covered by a Kitty image placement are disposable now; the rest of the shrink pushes from the top.
getText may return MAX_TEXT_MAX_BYTES of text, and JSON escaping can inflate that (a NUL becomes six bytes) past tungstenite's max_write_buffer_size, which the server sets to the attach output capacity. Exceeding it fails the write and disconnects the control client, so one large read tore down the session. The response text is trimmed until the serialised message fits, and flagged truncated. The payload budget is derived from the same constant as the buffer size so the two cannot drift apart.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bb116c6374
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let y = usize::from(old_rows - 1 - offset); | ||
| let row = &self.grid[y]; | ||
| let abs_row = live_top + y; | ||
| row.occupied() == 0 |
There was a problem hiding this comment.
When a bottom row previously held text and is then cleared with default-background EL/ED, Row::occupied() can remain nonzero because it is deliberately only an upper bound and those erase paths do not lower it. This predicate therefore treats a visually empty row as populated; a height-only shrink then drains rows from the top instead of discarding the empty rows below the cursor, unnecessarily moving visible content into scrollback. Check the cells for actual defaultness or reset occupancy when a row is fully cleared.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8bdd245: the shrink now checks the cells within the occupancy watermark (Row::is_blank), so rows cleared by EL/ED count as disposable. Regression: resize_shrink_rows_treats_erased_rows_below_cursor_as_disposable.
| } | ||
| if self.has_narrow_horizontal_margins() { | ||
| self.scroll_rectangle_up(top, bottom, n); | ||
| self.remove_placements_intersecting_grid_rows(top, bottom); |
There was a problem hiding this comment.
When DECSLRM defines a narrow horizontal region, a Kitty placement wholly to the left or right of that region should remain unchanged during a scroll. This row-only removal call deletes every placement overlapping the vertical band regardless of its anchor_col/cols, so an unrelated image can disappear after LF/IND or SU; the corresponding down/IL/DL branches have the same issue. Limit removal to placements that also intersect the active horizontal margins.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a34a566: rectangle scrolls (LF/IND, SU/SD, IL/DL under narrow DECSLRM) now remove only placements that also overlap the horizontal margins. Regression: kitty_rectangle_scroll_keeps_placements_outside_the_margins.
Row::occupied() is a monotonic upper bound that EL/ED never lower, so a row that once held text and was then cleared still read as populated. Shrinking the row count then drained rows from the top into scrollback instead of dropping the visually blank rows below the cursor. The shrink now inspects the cells within the watermark, so erased rows count as blank.
Scrolling inside narrowed DECSLRM margins removed every Kitty placement overlapping the scrolled rows, ignoring columns, so an image in a side pane vanished whenever the other pane scrolled (LF/IND, SU/SD, IL/DL). Rectangle scrolls now remove only placements that also overlap the horizontal margins; full-width scrolls are unchanged.
1 participant
Summary
noa.getTextresponses are trimmed to fit tungstenite's write buffer so a large read cannot disconnect the control client.Why
Each of these could lose visible content, corrupt display state, or drop a control connection under ordinary program output (full-screen apps using scroll regions and margins, editors relying on IRM, a stray byte in damaged UTF-8, a large
getTexton a screen full of escapable characters).Review notes
crates/noa-grid/src/screen/edit.rsfirst for the DECOM and DECSLRM changes.tests/parity/fixtures/scroll_region_origin.txtis re-blessed: its old expectation recorded the DECOM gap as known-divergent.#TODO(agent):marker is added ininsert_blank_chars(DEFERRED): IL/DL outside the horizontal margins and ICH/DCH left of the left margin are unreachable while cursor addressing clamps to DECSLRM; the guard is needed if that clamping is ever relaxed.Test plan