Implement the Kitty text sizing protocol (OSC 66)

[Copilot]

OSC 66 ; key=value : … ; text ST was not implemented — a run of text drawn at a multiple of the cell size, the modern successor to DECDWL/DECDHL, and the mechanism by which a client states how many cells a string takes instead of both sides guessing at Unicode.

The protocol has two halves and they are worth keeping apart. The width half is the emulator's own and is implemented completely: a run really claims s * w columns. The scale half needs a renderer, so it is recorded per line for one to read.

terminal.Write("\^[]66;s=2;Heading\^[\\");

// The block is stored the way a double-width character is.
var line = terminal.Buffer.Lines[terminal.Buffer.YBase]!;
line[0].Content;   // "H"
line[0].Width;     // 2 — the columns it took
line[1].Width;     // 0 — a continuation
terminal.Buffer.X; // 14

if (line.TryGetSizedRunAt(col, out LineSizedRun run))
{
    // run.Cols, run.Rows (the scale, growing downwards), run.Sizing
}

Changes

• Common/TextSizing.cs (new) — parses s, w, n, d, v, h and validates each against the protocol's ranges. Anything out of range leaves the sequence unhandled and reported as unrecognised through OscReceived, rather than clamped into a size nobody asked for: a client sending s=99 has a bug, and drawing its heading at some other size hides the bug while still producing wrong output.
• InputHandler — writes the payload as multicell blocks. w=0 gives each grapheme its own block s times its normal width; a non-zero w puts the whole payload in one block of s * w columns. Text in the first cell with Width equal to the columns taken, zero-width continuations after it — so search, selection, reflow and the cursor understand a scaled run without being told about this protocol. Blocks wrap whole under DECAWM, move back to fit when wrapping is off, and are discarded when wider than the screen.
• Buffer/LineSizedRun.cs (new) — scale, fraction and alignments held in a per-line side table, the same shape as the OSC 8 link table and for the same reason: BufferCell stays 24 bytes with no GC reference in it.
• Invalidation — overwriting, erasing (ED/EL/ECH) or shifting (ICH/DCH) any cell of a block erases the whole block and blanks its remaining cells, per the protocol — and the rule is REGION-based: an erase or line splice touching any row a tall block covers (ED through its lower rows, IL/DL, and a partial-region scroll that would tear it) erases the block via its anchor. Line reuse drops runs. A partly overwritten block would otherwise leave a first cell claiming columns that now hold something else.
• README — a renderer-facing section that is explicit about the division of labour, including that the rows a tall block covers are held for it (the print path skips covered cells, and writing on the topmost row replaces the block with spaces, per the spec's multicell rules) and that a renderer which cannot scale should draw the text at base size in the first cell.

On the issue's warning about partial support

The issue argues that advertising the capability and drawing unscaled text is worse than nothing. The protocol's probe measures cursor advance, not glyph size, and cursor advance is exactly what this change makes correct — CR CPR, w=2, CPR, s=2, CPR now reports columns 1 → 3 → 5. A host that ignores LineSizedRun gets base-size text in a correctly sized block, which is the degradation the protocol itself anticipates, not a broken layout.

A line holding a sized run is exempt from re-wrap on resize, the same arrangement double-width lines have always had: the run keeps its shape and its columns, and only ordinary lines reflow around it.

Tests in src/XTerm.NET.Tests/TextSizingTests.cs cover parsing ranges, cursor advance, the capability probe, wrapping with and without DECAWM, wide characters inside a scale, overwrite/erase/shift invalidation, and line reuse.

• Fixes Kitty text sizing protocol (OSC 66) not implemented #59

[JohnCampionJr]

The implementation is spec-faithful on parsing (ranges, d>n, probe-visible cursor advance, wrap-whole at the right edge, move-back with DECAWM off, discard wider-than-screen) and safe against payload injection: the parser drops C0 controls inside OSC strings and ESC/BEL terminate, so embedded sequences can neither execute nor reach cell content. The buffer model (first cell Width=s*w, zero-width continuations, LineSizedRun side table mirroring LineHyperlink, whole-block erase on partial overwrite/EL/ED/ECH/ICH/DCH, cleared on line recycle) is coherent and the tests assert concrete buffer state — low no-op-pass risk. The two real gaps are the spec's vertical occupation rules (nothing models the s rows a block is tall) and reflow/resize (run metadata neither moves with cells nor survives, and the CopyCellsFrom hook actively blanks block cells mid-reflow), which is also exactly the 'partial support' outcome issue #59 warned about, since the capability probe now reports support while the companion renderer repo draws nothing scaled.

1 high, 2 medium, 5 low — inline above. Width-half is well built and genuinely tested, but the vertical half of the spec and reflow/resize are unimplemented, so scaled text is overdrawn by following lines and garbled by resize.

[JohnCampionJr]

High — Vertical occupation (spec rules 3-4) is not modeled: rows below an s>1 block are unprotected.

The spec's multicell rules say text written on the rows below the first row of an s>1 block must interact with the block (cursor skips past it; overwriting the topmost row replaces it with spaces). This PR deliberately models zero vertical extent ('nothing here reserves those rows', DECDHL-style). Concrete failure: ESC]66;s=2;Big ESC\ then CR LF and normal output — the next line's text claims the columns directly under 'Big', and a renderer honoring run.Rows=2 draws the 2-cell-tall glyphs over that text. Because the protocol's own capability probe (CPR around ESC]66;s=2; BEL) now reports support, kitty-targeting clients will rely on rule 4 and emit output on the following rows expecting the terminal to skip the block. The DECDHL precedent doesn't transfer: DECDHL clients leave the room by convention, kitty clients rely on the spec'd cursor-skip. This is presumably the WIP part, but the PR body's checklist doesn't mention it — it should either be implemented or called out as an explicit scope decision (issue #59 asked for exactly that: 'partial support is not obviously better than none').

[Copilot]

Implemented in efce390. The rows a tall block covers are now held for it: TerminalBuffer.TryGetSizedRunCovering(absoluteRow, column, …) answers which block covers a cell, and the print path steps past any cell covered from above (SkipCellsCoveredFromAbove), so a client that follows the spec and writes on the rows below a heading lands after the block rather than under it. Rather than track the occupied rows forward through scrolling, the lookup walks up at most MaxScale - 1 rows and asks whether a run reaches down that far — nothing to keep in step. A sticky per-buffer HasMultiRowSizedRuns flag guards it, so a session that never sends s>1 pays one field read; the ASCII fast paths fall back to per-character printing only when it is set. Tests: Text_under_a_tall_block_is_pushed_past_it, The_row_below_a_one_row_block_is_ordinary, The_rows_a_block_occupies_are_answerable.

[JohnCampionJr]

Medium — Reflow/resize neither moves nor preserves sized runs; the CopyCellsFrom hook blanks block cells mid-reflow.

TerminalBuffer.ReflowLarger/ReflowSmaller redistribute cells between lines via CopyCellsFrom, and run metadata never travels with the cells. Worse, the hook added here (EraseSizedRunsOver(destCol, length, blankAll: true)) fires on those reflow copies too. Verified trace, reflow-larger: wrapped group where line1 = "ab" + 4-col block at cols 2-5; widen the terminal; the compaction copies line1[0..4) up to line0 (splitting the block: first cell lands near the end of line0 with continuations left behind), then compacts line1 in place — CopyCellsFrom(dest=line1, destCol=0) removes line1's run and blanks cells 2-5 BEFORE cells 4-5 are read as copy source, so the moved content arrives as blanks. Reflow-smaller (applyInReverse copies into fresh lines) instead leaves the run on the original line describing columns whose cells moved away — a renderer then scales unrelated text and TryGetSizedRunAt can report a run wider than the line. BufferLine.Resize likewise truncates cells without touching _sizedRuns. The README added by this PR explicitly claims 'the cursor, selection, search and reflow already agree with the client' — for reflow that claim is false. Minimum fix: treat lines with HasSizedRuns like HasNonNormalLineAttribute in BufferReflow (skip reflowing those groups), and clear runs in BufferLine.Resize.

[Copilot]

Fixed in efce390. The CopyCellsFrom hook is gone — you were right that reflow moves cells through it, so it could blank a copy source. Invalidation now happens at the call sites that actually overwrite (Print, PrintAsciiRun, ICH/DCH, Fill).

Reflow follows your suggestion: BufferReflow.IsUnreflowable(lines) tests HasNonNormalLineAttribute or HasSizedRuns, and both call sites use it, so a group holding a block is left alone.

BufferLine.Resize needed one adjustment to your minimum fix: TerminalBuffer.Resize resizes every line before ReflowLarger on a widening, so clearing runs there hid them from the guard and the group got reflowed anyway. It now prunes only runs cut by a narrowing (EraseSizedRunsOver past the new width, blanking their cells), which leaves runs visible to the guard on a widening and leaves no cell claiming columns that no longer exist. Tests: A_block_survives_the_line_growing, A_block_cut_by_a_narrowing_is_dropped, Reflow_does_not_garble_a_wrapped_group_holding_a_block (which also asserts no cell runs off the end of its line). README corrected — reflow no longer appears in the "already agree" list; instead it states that a group holding a run is not re-wrapped, exactly as a double-width line is not, and what a narrowing costs.

[JohnCampionJr]

Medium — Probe now advertises support while no shipped renderer honors scale — the outcome issue #59 warned against.

With this PR, the detection handshake (cursor advances for w=2 and s=2) reports full support, and ucs-detect will say Yes. But the drawing half lives in Iciclecreek.Avalonia.Terminal (separate repo, not part of this PR), so until that lands, every probing client gets 'supported' and renders base-size text in the first cell of each block — the 'broken heading' scenario the issue explicitly said should be a deliberate, stated decision rather than a side effect. The width half genuinely works standalone (that is what the probe measures), so this may be acceptable — but the PR should say so, and coordinated renderer work should exist before merge.

[Copilot]

Stated explicitly in the PR body now (efce390): the width half is complete and is what the probe measures; the scale half is recorded, not drawn, because this repository has no renderer. A host ignoring LineSizedRun gets base-size text in a correctly sized block. With vertical occupation now implemented, the layout a client builds around a heading is also correct at base size — the text below it lands below the block rather than inside it — so the degradation is "unscaled" rather than "broken". Renderer work in Iciclecreek.Avalonia.Terminal is a separate change; happy to hold merge for it if you would rather they land together.

[JohnCampionJr]

Low — Any unknown metadata key silently deletes the payload text.

TryParse returns false for any key outside {s,w,n,d,v,h} (and for any out-of-range value), and HandleTextSizing then prints nothing — the user-visible text is dropped entirely. The spec is silent on error handling, so rejecting out-of-range values is defensible, but rejecting UNKNOWN KEYS means any future protocol revision (kitty has extended its protocols before) makes text vanish on this terminal while degrading gracefully elsewhere; ignoring unknown keys, or falling back to printing the payload unscaled on a failed parse, loses less. Side note undercutting the strictness rationale: int.TryParse's default NumberStyles.Integer accepts 's=+2' and 's= 2', which the spec's digit-only grammar does not.

[Copilot]

Fixed in efce390. Unknown keys (and keys longer than one letter) are now ignored instead of failing the parse, so a future revision of the protocol degrades to unscaled text rather than to no text. And when the metadata genuinely cannot be parsed — an out-of-range value — the payload is printed unscaled (PrintUnsized) rather than dropped, while the sequence is still reported unrecognised through OscReceived. Also fixed the grammar hole you spotted: values go through a digits-only TryParseDigits, so s=+2 and s= 2 are rejected. Tests: Text_of_an_unhandled_sequence_is_still_printed, An_unknown_key_is_ignored_and_the_rest_honoured, A_key_longer_than_one_letter_is_ignored_too, plus the two new parse rows.

[JohnCampionJr]

Low — 64-UTF-16-unit truncation can drop text that would fit in the block.

MaxSizedRunText is measured in UTF-16 code units (text.Length), not the 4096 bytes the docstring cites from the spec. A maximal block is sw = 49 columns; 49 astral-plane characters (e.g. mathematical alphanumerics, common in fancy headings) are 98 UTF-16 units, so Truncate keeps only ~32 of the 49 characters the client legitimately sized the block for. If a cap is wanted, cap at a value that cannot bite content that fits — e.g. 4096 bytes per the spec, or at least 492 units.

[Copilot]

Fixed in efce390 — the cap is MaxSizedRunBytes = 4096 and Truncate measures UTF-8 bytes (cutting on a whole code point), matching the spec. Astral-plane characters that fit in the block are no longer dropped.

[JohnCampionJr]

Low — PrintSizedBlock arms REP, so CSI b replays OSC 66 text unscaled.

HandleOsc calls CancelRepeat() on dispatch — the documented invariant is that any OSC forgets the preceding graphic character — but PrintSizedBlock then calls RememberForRepeat with the block's cell. Verified consequence: ESC]66;s=2;X ESC\ followed by ESC[3b prints three normal-width, unscaled X cells (and for a w>0 multi-grapheme block, REP replays the whole interned cluster as single cells). The OSC payload is not a graphic character in the data stream; dropping this call (letting the dispatcher's CancelRepeat stand) matches the design described at RememberForRepeat.

[Copilot]

Fixed in efce390 — the RememberForRepeat call is gone, so the dispatcher's CancelRepeat() stands and CSI b after a block repeats nothing. Test: A_sized_block_is_not_repeated_by_rep.

[JohnCampionJr]

Low — Insert-mode path replicates the repo's pre-existing overlapping forward-copy corruption.

CopyCellsFrom(line, column, column+cols, Cols-column-cols, false) does a forward element-by-element copy on the same backing array with destCol > srcCol, so once i reaches cols it re-reads cells it has already overwritten — the shifted region degenerates into the first cols cells repeated. Verified against CopyCellsFrom's implementation (naive loop, no temp buffer; applyInReverse=true is the correct direction for a right shift). This is not a regression — main's Print insert-mode and InsertChars pass false identically, so IRM/ICH right-shifts are already broken repo-wide — but the PR adds another instance of the pattern and its Shifting_cells test only passes because the sized run is blanked wholesale first. Worth fixing repo-wide in a separate change; this PR should at least pass true.

[Copilot]

Changed to applyInReverse: true in efce390. Agreed the pre-existing Print/InsertChars instances are the same bug and are worth a separate change; left them alone here. Test: Insert_mode_shifts_the_rest_of_the_line_intact asserts the shifted tail rather than relying on the block being blanked first.

[JohnCampionJr]

Low — PR body checklist is stale; remaining unfinished surface.

The PR description still shows 'Tests covering parsing, cursor advance, wrapping, overwrite and reset' unchecked, but the tip commit added 24 tests that do cover those (with real buffer-state assertions — no tautological passes found; the parse tests double-check both TryParse and the OscReceived.Recognized signal). What the tests do NOT cover, matching the real gaps: resize/reflow with a sized run present, insert mode, REP after a sized block, and alt-buffer behavior. Size (998 added lines incl. 346 test lines) is proportionate for this protocol; the v/h alignment enums are stored-but-unused by the emulator, which is correct since they are renderer contract.

[Copilot]

PR body refreshed in efce390 — the test line is checked, the newly covered ground is listed, and the scope decision is stated rather than implied. The gaps you named are now covered: A_resize_drops_the_run_and_keeps_the_text became A_block_survives_the_line_growing / A_block_cut_by_a_narrowing_is_dropped, plus Reflow_does_not_garble_a_wrapped_group_holding_a_block, Insert_mode_shifts_the_rest_of_the_line_intact, Insert_mode_over_a_block_erases_it and A_sized_block_is_not_repeated_by_rep. Alt-buffer needs nothing specific — the runs live on BufferLine, so switching buffers carries them with the lines and ResetInPlace clears them on reuse (A_recycled_line_keeps_no_runs).

[JohnCampionJr]

@copilot Please address the review feedback in my review above (8 inline comments). Priorities: either implement the spec's vertical occupation rules for s>1 blocks or state the scope decision explicitly in the PR body; stop reflow from corrupting sized runs (skip reflowing lines with HasSizedRuns like HasNonNormalLineAttribute, clear runs in BufferLine.Resize) and correct the README claim; drop the RememberForRepeat call in PrintSizedBlock; don't reject unknown metadata keys by deleting the text; fix the truncation cap to bytes per the spec; and add tests for resize/reflow, insert mode, and REP.

[Copilot]

@copilot Please address the review feedback in my review above (8 inline comments). Priorities: either implement the spec's vertical occupation rules for s>1 blocks or state the scope decision explici...

All eight addressed in efce390 (full suite green, 1263 tests):

• Vertical occupation — implemented rather than deferred. TerminalBuffer.TryGetSizedRunCovering answers which block covers a cell, and printing steps past cells covered from above, so a client writing below a heading lands after the block. The scope decision that remains (scale is recorded, not drawn — no renderer lives here) is now stated in the PR body.
• Reflow/resize — IsUnreflowable now covers HasSizedRuns alongside HasNonNormalLineAttribute; the CopyCellsFrom hook that blanked copy sources mid-reflow is gone. BufferLine.Resize prunes only runs cut by a narrowing rather than clearing all of them, because TerminalBuffer.Resize resizes lines before reflowing on a widening and clearing there defeated the guard. README claim corrected.
• REP — RememberForRepeat call dropped.
• Unknown keys — ignored; unparseable metadata prints the payload unscaled; values parse digits-only, so s=+2/s= 2 are rejected.
• Truncation — 4096 bytes per the spec, measured in UTF-8.
• Insert-mode shift — applyInReverse: true.
• Tests — resize (grow and shrink), reflow, insert mode and REP added.

[JohnCampionJr]

Re-reviewed the new commits: every finding from the first review was addressed on its own terms, including the hard one — vertical occupation is now genuinely implemented (TryGetSizedRunCovering walks up adjacently, Print skips covered cells per rule 4, rule 3's whole-block-to-spaces on topmost overwrite holds, all pinned by tests), reflow no longer corrupts sized runs (IsUnreflowable covers both directions, BufferLine.Resize clears, the removed hook is compensated at every remaining caller, README corrected), the probe statement is argued in the PR body, and all four lows are fixed and tested. Genuinely impressive delta.

What remains, inline above: the spec's REGION-based erase rule (a block intersected by ED/EL/ECH on its lower rows must die; currently invalidation only fires via the anchor line) — the one Medium — plus the IL/DL split-erase rule, two bounded edges in the skip logic (guard exhaustion on legal input, cursor-column-only checking), a permanently-sticky HasMultiRowSizedRuns flag that quietly retires the ASCII fast path for the session, and stale v1 prose in LineSizedRun.cs and the PR body.

@copilot please address the six inline comments above.

[JohnCampionJr]

Medium — ED/EL/ECH intersecting a block's lower rows do not erase the block.

Run invalidation on erase lives in BufferLine.Fill, so it only fires when the erased range touches the ANCHOR line. The kitty spec's erase rule is region-based: 'Any multicell character that intersects with the erased region of the screen must be erased' — and a tall block's lower rows are part of the region they sit in. Verified trace: ESC]66;s=3;H ESC\ (block rows 0-2), CSI 2;1H, CSI J (erase below). Rows 1..end are filled; row 0's run is untouched, so the heading survives a clear that intersected two of its three rows — a renderer redraws the heading's lower rows over the freshly cleared screen, and because TryGetSizedRunCovering still reports rows 1-2 covered, text a client writes at column 0 of the 'cleared' row 1 lands displaced past phantom columns. Same for CSI K/CSI X on a covered row. In kitty the block would be gone. Fix: the erase handlers should consult TryGetSizedRunCovering for the region's rows and erase intersecting blocks via their anchor line — the machinery already exists.

[Copilot]

Implemented in 1ae38ca. The erase rule is now region-based: TerminalBuffer.EraseSizedRunsCovering(absoluteRow, column, count) walks up to the anchor lines and erases every block reaching down into the erased columns of that row (blanking what is left of it), and ED, EL and ECH call it for each row and column range they touch. BufferLine.EraseSizedRunsOver grew a reachingRows filter so only blocks tall enough to reach the row in question die — an erase beside or above a block leaves it alone. Tests: Erasing_below_takes_a_block_hanging_into_it, Erasing_a_covered_row_takes_the_block (EL 0, EL 2, ECH), Erasing_beside_a_block_leaves_it_alone.

[JohnCampionJr]

Low — IL/DL (and region scrolls) splitting a tall block do not erase it.

The spec: 'When inserting n lines at cursor position y any multi-line characters that are split at the line y must be erased', and for DL 'any multicell character that intersects the deleted lines must be erased'. InsertLines/DeleteLines are pure Lines.Splice operations with no multicell handling. Verified trace: ESC]66;s=2;Hi ESC\ (rows 0-1), CSI 2;1H, CSI L — a blank line is spliced between anchor and lower half; the run survives on row 0, so adjacency-based TryGetSizedRunCovering now reports the INSERTED blank row as covered, while the block's original second row, now two rows down, is stranded as ordinary cells. DL of a covered row similarly re-targets coverage at whatever scrolled up. The look-up-not-mark design is sound for whole-screen scrolling, but these line-splice operations break the adjacency invariant it depends on; per spec they should erase the intersected block.

[Copilot]

Fixed in 1ae38ca. InsertLines erases blocks hanging over the cursor's row before the splice, and DeleteLines does the same for every row it removes; blocks anchored on deleted rows leave with the lines that describe them. So the adjacency invariant the look-up design depends on cannot be broken by a splice — it is restored by killing the block, which is what the spec asks. Both then call RefreshMultiRowSizedRuns. Tests: Inserting_a_line_through_a_block_erases_it, Deleting_a_covered_line_erases_the_block.

[JohnCampionJr]

Low — Skip-loop guard exhaustion prints INTO covered cells after 15 consecutive runs.

SkipCellsCoveredFromAbove's guard allows MaxScale*2+1 = 15 iterations and on exhaustion returns TRUE — proceeding to write into a still-covered cell. Adjacent runs with DIFFERENT sizings do not merge, and one iteration is also consumed per wrap, so this is constructible with legal input: alternate s=2 and s=2:n=1:d=2 blocks across a full 80-column row (40 unmergeable runs), then CR LF and print — after ~30 columns the guard exhausts and characters land under blocks, where the renderer overdraws them. The bound is right in spirit (hostile input must not hang), but since every iteration strictly advances the cursor, a bound of Cols * MaxScale (or looping while progress is made) is equally hang-proof and cannot exhaust on a legal screen.

[Copilot]

Fixed in 1ae38ca — the bound is (Cols + 1) * MaxScale, which a legal screen cannot reach (a row holds at most Cols blocks, and only a wrap changes rows), and exhaustion now returns false so a pathological case discards the character rather than writing into a covered cell. Test A_row_of_many_blocks_is_skipped_completely builds your case: 40 unmergeable alternating s=2 / s=2:n=1:d=2 blocks filling an 80-column row, then CR LF and a print — the character lands on the row after the covered one, with the covered row untouched.

[JohnCampionJr]

Low — Skip check tests only the cursor column, so a wide character can overlap a covered region from the left.

SkipCellsCoveredFromAbove asks TryGetSizedRunCovering for _buffer.X only, but Print then writes width columns and PrintSizedBlock writes cols columns; the spec's rule is about the cells the text 'will overwrite', not the cursor cell. Verified trace: x then ESC]66;s=2;A ESC\ (block at columns 1-2, rows 0-1), CR LF, print a CJK ideograph — column 0 is not covered, so the width-2 cell is written over columns 0-1, and column 1 is inside the block's footprint: the buffer now holds a wide character whose right half a spec-honoring renderer will overdraw. Same for a new sized block whose span crosses into a covered region to its right. The skip should consider the full [X, X+width) span.

[Copilot]

Fixed in 1ae38ca. SkipCellsCoveredFromAbove(int width) now tests every column in [X, X + width) and moves past the first covering block it finds. Print computes the character's width before the skip (the line is fetched after, since a skip can wrap), and PrintSizedBlock passes its cols — and because a skip can leave too little room while making room can land under another block, those two are now settled against each other in a bounded loop, with a block that cannot be settled dropped rather than written into cells that belong to something else. Test: A_wide_character_may_not_overlap_a_block_from_the_left uses exactly your trace.

[JohnCampionJr]

Low — HasMultiRowSizedRuns is sticky for the Terminal's lifetime — one tall block permanently disables the ASCII fast path.

The flag is only ever set true; nothing clears it — not ED 2, not the block's line recycling, and not RIS (Terminal.Reset reuses the same TerminalBuffer objects and only calls ClearBuffer). The doc comment says a stale true 'costs a few field reads', but both PrintAsciiRun overloads check it and permanently fall back to per-character Print, with every printed character additionally paying the up-to-6-line covering walk. Concrete scenario: a shell greeting prints one scaled heading, then the user runs a throughput-heavy program — the span fast path is gone for the rest of the session, and the CI perf gate won't see it because the benchmark stream contains no OSC 66. Cheap improvement: clear the flag in ClearBuffer/Reset and optionally re-derive after a full-screen erase.

[Copilot]

Fixed in 1ae38ca. TerminalBuffer.RefreshMultiRowSizedRuns() re-derives the flag by scanning the ring, and returns immediately when the flag is already false — so a session that never draws a tall block never pays for it. It is called from EraseInDisplay, InsertLines, DeleteLines and Terminal.ClearBuffer (which covers Clear() and RIS). Test: Clearing_the_screen_stops_the_search_for_blocks asserts the flag goes false after ED 2, so the ASCII fast path comes back.

[JohnCampionJr]

Low — PR body and LineSizedRun remarks still describe the de-scoped v1 design.

LineSizedRun.cs's class remarks still say 'nothing here reserves those rows — the same arrangement DECDHL has always had' and that reflow agrees via continuation cells — but this push implements vertical occupation, and the README now documents the opposite ('The rows a tall block covers are held for it'). The PR description likewise still states 'nothing reserves the rows below a scaled run' and 'Reflow drops sized runs... scale is lost across a resize' — both false since this push (reflow skips run-holding groups and KEEPS the runs). Whoever reads the PR to understand the merge gets the abandoned design; the stale doc comment ships in the code. Update both to match.

[Copilot]

Updated in 1ae38ca. LineSizedRun's remarks now say that the rows are claimed too (pointing at TryGetSizedRunCovering), that erasing or splicing any row a block touches erases the block, and that a line holding a run is not re-wrapped by a resize; reflow is out of the "already agree" list. The PR body is rewritten to match — the scope statement is now "width and position are complete, drawing is recorded", and the stale claims about reserved rows and dropped runs are gone. README gained the erase/splice sentence.

[Copilot]

Re-reviewed the new commits: every finding from the first review was addressed on its own terms, including the hard one — vertical occupation is now genuinely implemented (TryGetSizedRunCovering wal...

All six addressed in 1ae38ca (full suite green, 1273 tests):

• Region erase (Medium) — TerminalBuffer.EraseSizedRunsCovering erases blocks reaching down into an erased region; ED, EL and ECH all call it. EraseSizedRunsOver grew a reachingRows filter so an erase beside or above a block leaves it alone.
• IL/DL — both erase blocks hanging over the rows they splice, so a splice cannot break the adjacency the look-up design depends on.
• Guard exhaustion — bound is now (Cols + 1) * MaxScale, unreachable on a legal screen, and exhaustion returns false rather than printing into a covered cell.
• Span check — the skip tests [X, X + width); Print computes width first and PrintSizedBlock passes cols, with skipping and fitting settled against each other.
• Sticky flag — RefreshMultiRowSizedRuns re-derives it on ED, IL, DL and ClearBuffer (so Clear() and RIS too), and costs nothing when the flag is already false.
• Stale prose — LineSizedRun remarks, README and the PR body all rewritten to the shipped design.

New tests: Erasing_below_takes_a_block_hanging_into_it, Erasing_a_covered_row_takes_the_block (×3), Erasing_beside_a_block_leaves_it_alone, Inserting_a_line_through_a_block_erases_it, Deleting_a_covered_line_erases_the_block, A_wide_character_may_not_overlap_a_block_from_the_left, Clearing_the_screen_stops_the_search_for_blocks, A_row_of_many_blocks_is_skipped_completely.

[github-actions]

Perf comparison

3 run(s) of each side, alternating on one machine. Allocation is a count and is gated exactly. Time is a measurement, so its gate is derived from the spread this job just observed in itself rather than fixed in advance.

corpus	bytes/char	gen0/Mchar	ns/char	Δ time	noise	gate
scroll-ascii	0.00 → 0.00	0.00 → 0.00	3.35 → 3.38	+0.8%	±5%	14%
sgr-churn	0.00 → 0.00	0.00 → 0.00	8.94 → 8.91	-0.4%	±3%	8%
truecolor	0.00 → 0.00	0.00 → 0.00	9.71 → 9.76	+0.6%	±1%	5%
alt-redraw	0.00 → 0.00	0.00 → 0.00	13.61 → 14.16	+4.1%	±1%	5%
unicode	7.66 → 7.66	0.45 → 0.45	34.18 → 35.77	+4.7%	±3%	8%
flood	0.00 → 0.00	0.00 → 0.00	96.06 → 94.65	-1.5%	±2%	7%

Each corpus is gated at max(5%, 3 × its own noise). A wide noise column means this runner was busy and the timing half of the table should be read as advisory; the allocation half is exact either way.

assemblies measured

• base: XTerm.NET 2.0.0.0 mvid:584dcd9e-3fa1-4806-8e73-b38f3fa31a2a
• head: XTerm.NET 2.0.0.0 mvid:297dcc4e-3f16-4cd4-a210-448cfbf64fd4

[JohnCampionJr]

Re-reviewed the erase/splice commit: five of the six previous items are resolved and verified by trace — ED/EL/ECH erase blocks hanging over the affected region via EraseSizedRunsCovering with a correct reach test and no reentrancy; IL/DL erase intersected blocks before splicing; the skip loop is bounded at (Cols+1)*MaxScale with strictly-forward progress and fails safe by dropping rather than printing into covered cells (the 40-block adversarial row now passes); the skip tests the full [X, X+width) span so the wide-char and new-block overlaps are closed; and HasMultiRowSizedRuns is re-derived on ED/IL/DL/Clear/Reset with no false negatives, every new hook early-outing on one flag read so the perf-gate path is untouched.

Two lows remain, inline above: the partial-region SCROLL splices (CSI S/T, LF at a DECSTBM bottom, RI at the top) do the same line-splice IL/DL now invalidate but got no hook — DL kills a boundary-straddling block while CSI S leaves it alive — and the PR body still carries three v1 claims the fix commits made false.

@copilot please address the two inline comments above (for the scroll paths, either hook them or explicitly narrow the documented claim).

[JohnCampionJr]

Low — Region scrolls (SU/SD, LF at the bottom of a DECSTBM region, RI at the top) still splice through blocks with no invalidation.

IL and DL got EraseBlocksHangingOver + RefreshMultiRowSizedRuns, but the same buffer transformation reached through TerminalBuffer.ScrollUp/ScrollDown did not: CSI S / CSI T call _buffer.ScrollUp/ScrollDown directly, and LineFeed/IndexDown/NextLine at ScrollBottom and ReverseIndex at ScrollTop do the same. With a partial scroll region, ScrollUp splices the region-top line out and ScrollDown the region-bottom line — exactly what DL/IL do — so a tall block straddling the region boundary survives with its coverage retargeted at the row that scrolled into place: the skip path displaces text written there and a renderer draws the scaled glyph over unrelated content. Verified inconsistency: with region 2..10 and a block anchored at row 1 reaching row 2, CSI 2;1H CSI M (DL) erases the block, while CSI S — the identical splice — leaves it alive. The full-screen scroll path is genuinely consistent, so only the partial-region splice paths need the hook; alternatively narrow the gap explicitly in the remarks and qualify the README's "Erasing or splicing any row a block touches ... erases the whole block", which currently claims coverage the scroll paths do not have.

[Copilot]

Hooked rather than narrowed, in 0da811a. TerminalBuffer.EraseSizedRunsSplitBy(regionStart, regionEnd) erases any block torn by a splice at either boundary, and both partial-region paths in ScrollUp/ScrollDown call it before splicing — which covers CSI S/CSI T, LF at a DECSTBM bottom and RI at its top, since they all reach the buffer through the same two methods. Your inconsistency is gone: with region 2..10 and a block anchored at row 1 reaching row 2, CSI S now erases it exactly as CSI M does. The full-screen path is untouched, so it stays hang-free and allocation-free. Tests: Scrolling_a_region_up_through_a_block_erases_it, Scrolling_a_region_down_through_a_block_erases_it, A_full_screen_scroll_keeps_a_block.

The merge with main added a fourth cell-moving path — ScrollMarginColumns, the box scroll inside DECSLRM margins — which moves cell ranges between lines while runs stay behind. That one is handled inside ScrollMarginColumns itself rather than at its four call sites, so no future caller can forget it, and it early-outs on the same single flag read.

[JohnCampionJr]

Low — PR body still describes v1 semantics that the last two fix commits made false.

The LineSizedRun.cs remarks are properly rewritten and the README now matches the code, but the PR description was not updated and contradicts both in three places: (a) it still says "nothing reserves the rows below a scaled run (the arrangement DECDHL has always had)" — v2 reserves those rows; (b) "Reflow drops sized runs ... scale is lost across a resize" — current behavior exempts run-holding lines from re-wrap, like double-width lines; (c) the Invalidation bullet lists only overwrite/ED/EL/ECH/ICH/DCH, omitting the region rule, IL/DL, and coverage-row erasure that are now the headline behavior. The body is the reviewer- and archaeology-facing summary; the README section at this line is an accurate template to crib from.

[Copilot]

Rewritten. The body now says that a block occupies s rows and text aimed below one is placed after it; that a wrap group holding a run is exempt from re-wrap like a double-width line, so scale survives a widening, and only a narrowing that cuts a block erases it; and the invalidation bullet lists the region rule for ED/EL/ECH including coverage rows, IL/DL, partial-region scrolls, margin box scrolls, ICH/DCH, overwrite and line reuse. The scope statement is now "width and position complete, drawing recorded", with the probe argument attached to it. Cribbed from the README section as you suggested.

[JohnCampionJr]

Description brought in line with the shipped v2 semantics directly (the three v1 claims — unreserved rows, reflow dropping runs, and the pre-region invalidation list — are replaced with what the code now does). That was the last open item from my reviews: with the conflict-resolution merge verified against the full suite, everything I raised across three rounds is closed. Ready.

[JohnCampionJr]

All review findings across every round are resolved and verified, the conflict-resolution merge is confirmed against the full test suite, and the implementation matches the protocol spec. Approved.

[JohnCampionJr]

Perf gate diagnosed and fixed in fd731da. The failure (alt-redraw +11.1% against a 5% gate, unicode flagged at +5.7%) was ResolveAutowrap(): extracting Print's wrap logic into a method put a non-inlined call on the once-per-printed-character path where main had an inline compare — the exact lesson NoteLinkRun's guard already records in this file, measured at 12% on the same corpus the first time around. The test is back inline at the call site and the method runs only when a wrap is actually due. Measured locally over three alternating runs per side: alt-redraw +9.0% → +4.0% (under its gate), unicode settles at +2.2% (the +5.7% was runner noise), allocation identical everywhere. Suite 1605/1605. The remaining cost is the guarded flag reads the feature needs, all already at call sites per the house rule.

[JohnCampionJr]

Perf gate green after the inline-wrap-test fix; final state verified across three review rounds plus the perf diagnosis. Safe to merge.

[JohnCampionJr]

Squeezed the residual: alt-redraw is now +1.3% at ±1% noise (was +4.0% after the first fix, +11.1% originally), unicode +1.0%, allocation identical everywhere. The second find was the same disease in another limb — EraseBlocksHangingOver held its flag test inside the method while ED calls it once per erased line, so a full-screen redraw paid a method call per line to read a false flag. The guard now lives at every call site, hoisted to one read outside the ED loops. What remains is the HasMultiRowSizedRuns/HasSizedRuns reads themselves — the feature's honest price, at the measurement floor. Suite 1605/1605.

[JohnCampionJr]

One payload-limit issue remains before merge.

[JohnCampionJr]

Enforce the protocol's 4096-byte payload limit for w=0 too. Truncate() is only called in the fixed-width branch, so the default w=0 path processes an arbitrarily large OSC payload and interns every grapheme in the process-wide cluster table. That both violates OSC 66's 4096-byte limit and leaves a memory-exhaustion path despite the comment above MaxSizedRunBytes. Validate/truncate the UTF-8 payload once before branching (at a grapheme boundary), then use the bounded text in both modes; add a w=0 oversized-payload test. Spec: https://sw.kovidgoyal.net/kitty/text-sizing-protocol/

[Copilot]

Fixed in 223afdb. Truncate() now runs once at the top of PrintSized, before the branch, so the bounded text is what both modes work from — the w=0 walk can no longer intern an unbounded number of clusters. It stays a grapheme-boundary cut measured in UTF-8 bytes, and the cheap length pre-check means a real payload still costs no encoding pass.

Test A_payload_over_the_limit_is_cut_when_each_grapheme_is_its_own_block writes a 5000-character w=0 payload and counts the cells that landed: 4096. Verified it fails against the previous commit. Suite 1606/1606.

[JohnCampionJr]

Verified — and already delivered on the tip: Copilot's last two commits apply Truncate() before the mode branch with exactly this rationale in the comment ('the payload limit applies to the sequence, not to one of its two modes: with w=0 an oversized payload would otherwise be walked grapheme by grapheme, interning every one in the process-wide cluster table'), and the renamed test pins 4096 printed cells for the w=0 path. I merged current main (through #80) on top; suite 1755/1755.

[JohnCampionJr]

Codex's payload-limit finding was already delivered on the tip and is verified (truncate before the branch, w=0 oversize test); merged with main through #80; suite 1755/1755. Perf gates were re-validated at the noise floor in the earlier rounds and these two commits touch only the cold OSC path. Supersedes my change-request.