Lifecycle, reset completeness, and a public class that shipped mojibake by JohnCampionJr · Pull Request #100 · tomlm/XTerm.NET

JohnCampionJr · 2026-08-29T15:30:11Z

Eighth and last of the audit PRs. Stacked on #98 → #97 → #94 → #93 → #92 → #89.

Fix	Why it mattered
`Terminal` declares `IDisposable`	it always had the method, but without the interface no `using`, DI container or analyzer could see it — handlers stayed subscribed for anyone who did not read the source
`Reset()` restores charset designations	`InputHandler.ResetCharsets` existed for exactly this and was called from nowhere, so a program that designated line drawing into G0 and died left the next one printing box characters for letters
`Reset()` restores the three Sixel modes	`DECRQM` went on reporting mode 80 as set after a reset
`ReverseWraparound` (DECSET 45) is honoured	stored and reported and never read, so a shell erasing a wrapped command line stopped at the wrap
OSC 99 no longer raises an empty notification	missing braces meant only the inner `if` was guarded by `TryBuild`, so a failed build raised the event anyway with null title and null body
Kitty image ids read unsigned	the signed reader saturates at `int.MaxValue`, so every id above 2³¹ collapsed onto one value — two images with distinct legal ids became one, and deleting either removed both. The unsigned reader was already there for the frame background

Public API

XTerm.Charset.Charsets is deleted. A public duplicate of XTerm.Common.Charsets whose line-drawing table shipped as literal ? characters. Nothing in the tree referenced it, but it was public, so this is a removal.
Terminal now implements IDisposable — additive.
Writes after Dispose are ignored, not thrown. I implemented ObjectDisposedException first and backed it out: two existing tests pin the no-op, and more importantly a host reads its pty on a background thread, so disposing mid-read is ordinary rather than a bug. Throwing there kills the read loop instead of the object being torn down. Documented in the method.

Deliberately not fixed

Terminal stores the caller's TerminalOptions by reference, so two terminals built from one options object mutate each other (DECSCUSR from one changes the other's cursor). Copying broke Constructor_WithOptions_UsesProvidedOptions, which asserts identity — and a host that mutates the options object it passed in expects that to reach the terminal. That is a contract decision rather than a bug fix; filed separately.

Validation

6 new tests. Suite green: 1938. No perf run — none of this is on the print path.

🤖 Generated with Claude Code

github-actions · 2026-08-29T15:33:18Z

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	2.71 → 2.71	+0.1%	±1%	5%
sgr-churn	0.00 → 0.00	0.00 → 0.00	7.08 → 7.46	+5.3% ⚠️	±1%	5%
truecolor	0.00 → 0.00	0.00 → 0.00	7.39 → 7.97	+7.9% ⚠️	±1%	5%
alt-redraw	0.00 → 0.00	0.00 → 0.00	10.84 → 10.95	+1.0%	±3%	8%
unicode	7.66 → 7.66	0.45 → 0.45	25.89 → 27.84	+7.6% ⚠️	±2%	6%
flood	0.00 → 0.00	0.00 → 0.00	81.49 → 81.74	+0.3%	±1%	5%

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:33c4e71d-0e35-4181-b8da-135b19204a82
head: XTerm.NET 2.0.0.0 mvid:60d6a452-74cc-4859-b8c0-68ce5b483b9a

Regressions

sgr-churn time 7.08 → 7.46 ns/char (5.3%, past its 5.0% gate, noise ±1.3%)
truecolor time 7.39 → 7.97 ns/char (7.9%, past its 5.0% gate, noise ±1.0%)
unicode time 25.89 → 27.84 ns/char (7.6%, past its 5.5% gate, noise ±1.8%)

Copilot

🟡 Changes recommended

The OSC parsing changes currently drop all non-ASCII payload characters and include a documentation/behavior mismatch around OSC truncation vs drop semantics.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR is the final audit pass focusing on terminal lifecycle correctness (dispose/reset), parser conformance, and protocol edge cases that previously produced incorrect state or unbounded work/memory. It also removes a public API duplicate (XTerm.Charset.Charsets) that shipped with mojibake.

Changes:

Make Terminal implement IDisposable, ignore writes after dispose, and restore more complete RIS/reset state (tab stops, charset designations, Sixel modes, reverse wraparound behavior).
Tighten VT parser correctness and safety (CAN/SUB abandon sequences, 8-bit ST ends OSC, saturating CSI params, OSC payload cap, DEL handling).
Fix protocol behaviors across input/output paths (tab stop semantics via HTS/TBC, mouse/keyboard encoding fixes, Kitty/Png bounds and unsigned IDs, buffer bookkeeping fixes like trim/splice).

File summaries

File	Description
src/XTerm.NET/Terminal.cs	Implements `IDisposable`, ignores writes after dispose, resets additional terminal state including tab stops and Sixel/charset state.
src/XTerm.NET/Selection/SelectionManager.cs	Fixes wrapped-line selection newline insertion and word selection expansion direction.
src/XTerm.NET/Parser/EscapeSequenceParser.cs	Improves state machine conformance and adds bounds/saturation logic for hostile inputs (OSC size, CSI params, control handling).
src/XTerm.NET/InputHandler.cs	Fixes multiple terminal semantics (wrap/erase/editing edge cases, charset save/restore, bounds, Kitty, etc.).
src/XTerm.NET/Input/MouseTracker.cs	Corrects coordinate clamping and modifier semantics for specific mouse protocols.
src/XTerm.NET/Input/KeyboardInputGenerator.cs	Aligns Escape/Home/End/keypad outputs with xterm behavior and application modes.
src/XTerm.NET/Graphics/PngDecoder.cs	Bounds decompression output to prevent allocation bombs.
src/XTerm.NET/Graphics/KittyCommand.cs	Parses Kitty image IDs as unsigned per protocol.
src/XTerm.NET/Common/CommandExtensions.cs	Adds ECMA-48 cursor motion aliases (HPA/HPR/VPR) to CSI mapping.
src/XTerm.NET/Common/ClusterTable.cs	Caps cluster interning and avoids expensive `ConcurrentDictionary.Count` on hot path.
src/XTerm.NET/Charset/Charsets.cs	Removes public duplicate charset table that shipped incorrect glyph mappings.
src/XTerm.NET/Buffer/TerminalBuffer.cs	Fixes nullable contract for `GetLine`, reports scrollback trims, and restores more cursor state.
src/XTerm.NET/Buffer/CircularList.cs	Corrects splice-at-capacity insertion semantics and releases trimmed references.
src/XTerm.NET/Buffer/BufferLine.cs	Adds wide-cell latch/repairs and extends erase/fill to preserve the wide-cell invariant.
src/XTerm.NET.Tests/*	Adds coverage for parser conformance, hostile bounds, lifecycle/reset, selection/bookkeeping, cursor/margins, and wide-cell invariants.

Review details

Files reviewed: 21/21 changed files
Comments generated: 4
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

    /// <summary>
    /// Sets a cell at a specific column.
    /// </summary>
+    /// <summary>
+    /// Whether this line has ever held a two-column character. A latch, not a count: it exists so
+    /// the print path can skip its orphan check with one field read, and the only cost of a stale
+    /// true is doing a check that finds nothing. Clearing it accurately would mean scanning the
+    /// row on every erase, which is the work it was added to avoid.
+    /// </summary>
+    public bool HasWideCells { get; private set; }


+                    if (code == 0x1B || code == 0x07 || code == 0x9C)
                    {
                        DispatchOsc();
                        Transition(code == 0x1B ? ParserState.Escape : ParserState.Ground);
                    }
-                    else if (code >= 0x20)
+                    else if (code >= 0x20 && code < 0x80)
                    {
                        OscPut(code);
                    }


+        // Past the cap the payload is dropped on the floor rather than truncated and dispatched:
+        // half a URL or half a base64 clipboard write is not something a handler should act on.
+        if (_osc.Length >= MaxOscPayloadChars)
+            return;


JohnCampionJr

Two lifecycle/reset state paths still retain or cross state that RIS/reverse-wrap should bound.

JohnCampionJr · 2026-08-29T15:48:14Z

+        // And the charset designations, with the SO/SI shift state. InputHandler.ResetCharsets
+        // existed for exactly this and was called from nowhere, so a program that designated line
+        // drawing into G0 and died left the next one printing box characters for letters.
+        _inputHandler.ResetCharsets();


Resetting only the live charset tables leaves _savedCharsetDesignations and SavedCursorState intact. If a program designates line drawing, executes DECSC, then RIS, a later DECRC restores the pre-reset line-drawing designation and undoes this fix. RIS should invalidate/reset the saved cursor context (including the saved designation snapshot), and the test should cover DECSC → RIS → DECRC before printing.

JohnCampionJr · 2026-08-29T15:48:14Z

                {
                    _buffer.SetCursor(_buffer.X - 1, _buffer.Y);
                }
+                else if (ReverseWraparound && _buffer.Y > 0)


This bounds reverse wrap at row 0 rather than at the scrolling region. With DECSTBM active, backspacing at X=0 on ScrollTop can move the cursor into the protected row above the region; it also lands at Cols-1 rather than the active right margin. Please use the same region/margin limits as the other cursor motions and add a test at ScrollTop with narrowed/full-width margins.

github-actions · 2026-08-29T16:42:38Z

Perf comparison — this change, against its base

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	2.70 → 2.70	+0.0%	±1%	4%
sgr-churn	0.00 → 0.00	0.00 → 0.00	7.22 → 7.20	-0.4%	±4%	12%
truecolor	0.00 → 0.00	0.00 → 0.00	7.59 → 7.57	-0.3%	±0%	4%
alt-redraw	0.00 → 0.00	0.00 → 0.00	10.78 → 10.80	+0.3%	±2%	6%
unicode	7.66 → 7.66	0.45 → 0.45	26.08 → 26.04	-0.2%	±3%	8%
flood	0.00 → 0.00	0.00 → 0.00	81.67 → 81.75	+0.1%	±1%	4%

Each corpus is gated at max(4%, 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:dbdef637-4e1d-439d-9363-4f00d8e39b55
head: XTerm.NET 2.0.0.0 mvid:09969b4d-b390-44d4-aeb7-8a14d7c42185

Perf comparison — cumulative, everything since 2.0.0-rc002

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	2.71 → 2.70	-0.1%	±1%	5%
sgr-churn	0.00 → 0.00	0.00 → 0.00	7.05 → 7.20	+2.0%	±4%	12%
truecolor	0.00 → 0.00	0.00 → 0.00	7.40 → 7.57	+2.3%	±2%	6%
alt-redraw	0.00 → 0.00	0.00 → 0.00	10.84 → 10.80	-0.3%	±5%	15%
unicode	7.66 → 7.66	0.45 → 0.45	25.66 → 26.04	+1.5%	±3%	8%
flood	0.00 → 0.00	0.00 → 0.00	81.78 → 81.75	-0.0%	±1%	5%

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:1b573f7c-2562-4a81-9dc9-0cdd9bd9fb1d
head: XTerm.NET 2.0.0.0 mvid:09969b4d-b390-44d4-aeb7-8a14d7c42185

- Terminal declares IDisposable. It always had the method, but without the interface no using statement, DI container or analyzer could see it, so handlers stayed subscribed for anyone who did not read the source. Dispose is idempotent now; writes after it are IGNORED rather than thrown, deliberately, because a host reads its pty on a background thread and disposing mid-read is ordinary -- throwing there would kill the read loop rather than the object being torn down. Two existing tests pin that. - Reset left the charset designations standing, so a program that designated line drawing into G0 and died left the next one printing box characters for letters. InputHandler.ResetCharsets existed for exactly this and was called from nowhere. - Reset also left the three Sixel modes, so DECRQM went on reporting mode 80 as set afterwards. - ReverseWraparound (DECSET 45) was stored and reported by DECRQM and nothing ever read it: a shell erasing a wrapped command line stopped at the wrap. - OSC 99 raised a notification with null title AND null body when the build FAILED -- missing braces meant only the inner if was guarded. - Kitty image ids were parsed through the signed reader, which saturates at int.MaxValue, so every id above 2^31 collapsed onto one value: two images with distinct legal ids became one, and deleting either removed both. The unsigned reader was already there. - Deleted XTerm.Charset.Charsets, a public duplicate of XTerm.Common.Charsets whose line-drawing table shipped as question marks. Nothing referenced it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…et RIS reach the state DECRC restores from Three review findings. RIS reset the live charset tables but left the SAVED cursor context alone, which put the reset one DECRC away from being undone: designate line drawing, DECSC, RIS, DECRC, and the dead program's designations are back. DECSC state is per-screen, so both buffers are cleared -- clearing only the active one leaves the other loaded and reachable the moment an application switches to it. Reverse wraparound was bounded at row 0 and landed at Cols - 1, so backspacing off the left edge on the first row of a DECSTBM region moved the cursor into the row the region exists to protect, and under DECSLRM it landed outside the pane. The motion moves next to the other cursor motions and reads the same limits they do: the left margin stops it, TopLimit bounds the row, and it lands on the right margin. The stray XML summary above _disposed is removed; it belonged to Dispose and was attaching itself to the field. Also folded in, as promised on #98: OscPut counts the width of what it is about to append, so a supplementary character cannot land a UTF-16 unit past the cap. It changed nothing observable -- DispatchOsc tests >= and dropped the payload either way -- but the branch below already knows the width, so holding the bound exactly costs nothing. The non-ASCII OSC test is a pin rather than a fix. Two reviewers have now read the annotated arm in the OSC control block as though it restricted the payload to ASCII; it cannot, because that block is entered only for C0 and C1, and everything from 0xA0 up reaches OscPut through the ordinary path. The test says so permanently.

JohnCampionJr · 2026-08-29T18:14:48Z

Rebased onto main (post-#98) — clean, no stacking on this one. 1956 tests green.

Both of your findings are real and fixed

RIS leaving the saved cursor context intact. You are right that resetting only the live tables leaves the reset one DECRC away from being undone. Since #93 the designations live in SavedCursorState rather than on the input handler, so the fix is to clear that — and to clear it on both buffers, because DECSC state is per-screen and clearing only the active one leaves the other loaded and reachable the moment an application switches to it. Two tests: DECSC → RIS → DECRC → print on the normal screen, and the same with the save made on the alternate screen.

Reverse wrap ignoring the region and the margins. Also right, and the fix is to stop having the logic in two places. The motion moved into InputHandler next to the other cursor motions, so it reads the limits they read: the left margin stops it (as CursorBackward does), TopLimit() bounds the row, and it lands on ScrollRight rather than Cols - 1. Tests cover the top of a DECSTBM region and a DECSLRM right margin.

Both regression tests fail against the previous commit.

One Copilot finding I checked and am not acting on

The claim that OSC payload accumulation "now only accepts characters < 0x80" is a misreading. The block that arm lives in is entered only for C0 and C1 — code < 0x20 || (0x80..0x9F) — so it never sees a payload character at all; everything from 0xA0 up reaches OscPut through the ordinary state-machine path (case ParserState.OscString: OscPut(code)). Non-ASCII titles and URLs were never affected.

That said, two reviewers have now read that arm the same wrong way, which is a fair signal the code invites it. So rather than only reply, there is now a test asserting a window title survives with its accents, CJK, an em dash and a check mark intact. If someone does break it later, they will be told.

The related "truncated payload is still dispatched" note is stale — DispatchOsc has tested _osc.Length >= MaxOscPayloadChars and returned without raising since #92.

Folded in from #98

As promised there: OscPut now counts the width of what it is about to append, so a supplementary character cannot land a UTF-16 unit past the cap. Nothing observable changes — DispatchOsc tests >= and dropped the payload either way — but the branch below already knows the width, so holding the bound exactly is free.

The stray <summary> above _disposed is removed. The HasWideCells one is #94's code, not this PR's.

JohnCampionJr requested a lite review from Copilot August 29, 2026 15:30

JohnCampionJr mentioned this pull request Aug 29, 2026

Terminal stores the caller's TerminalOptions by reference, so two terminals alias each other #101

Closed

Copilot started reviewing on behalf of JohnCampionJr August 29, 2026 15:30 View session

Copilot AI reviewed Aug 29, 2026

View reviewed changes

JohnCampionJr commented Aug 29, 2026

View reviewed changes

JohnCampionJr force-pushed the api-lifecycle branch from 05815a6 to 21eac6e Compare August 29, 2026 16:38

JohnCampionJr force-pushed the api-lifecycle branch from 21eac6e to 31cec01 Compare August 29, 2026 16:54

This was referenced Aug 29, 2026

Buffer bookkeeping and selection: six places a row index went wrong #97

Merged

Input encodings the application cannot ask us to clarify #98

Merged

JohnCampionJr and others added 2 commits August 29, 2026 14:08

JohnCampionJr force-pushed the api-lifecycle branch from 31cec01 to 4234fac Compare August 29, 2026 18:14

JohnCampionJr merged commit 0e93966 into main Aug 29, 2026
2 checks passed

JohnCampionJr mentioned this pull request Aug 29, 2026

Keep the wide-cell invariant on every path that writes cells #94

Merged

This was referenced Sep 2, 2026

Bump XTerm.NET from 1.2.0 to 2.0.1 ghostflyby/Maieutics#47

Merged

chore: Bump XTerm.NET from 1.1.0 to 2.0.1 kurrent-io/kcap-cli#798

Closed

dependabot Bot mentioned this pull request Sep 14, 2026

chore: Bump XTerm.NET from 1.2.0 to 2.0.2 kurrent-io/kcap-cli#927

Closed

dependabot Bot mentioned this pull request Sep 28, 2026

chore: Bump SvcSystems.UI.Terminal and XTerm.NET kurrent-io/kcap-cli#1188

Closed

Navigation Menu

Lifecycle, reset completeness, and a public class that shipped mojibake - #100

Lifecycle, reset completeness, and a public class that shipped mojibake#100
JohnCampionJr merged 2 commits into
mainfrom
api-lifecycle

JohnCampionJr commented Aug 29, 2026

Uh oh!

github-actions Bot commented Aug 29, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

JohnCampionJr left a comment

Uh oh!

JohnCampionJr Aug 29, 2026

Uh oh!

JohnCampionJr Aug 29, 2026

Uh oh!

github-actions Bot commented Aug 29, 2026 •

edited

Loading

Uh oh!

JohnCampionJr commented Aug 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

JohnCampionJr commented Aug 29, 2026

Public API

Deliberately not fixed

Validation

Uh oh!

github-actions Bot commented Aug 29, 2026

Perf comparison

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

JohnCampionJr left a comment

Choose a reason for hiding this comment

Uh oh!

JohnCampionJr Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

JohnCampionJr Aug 29, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Perf comparison — this change, against its base

Perf comparison — cumulative, everything since 2.0.0-rc002

Uh oh!

JohnCampionJr commented Aug 29, 2026

Both of your findings are real and fixed

One Copilot finding I checked and am not acting on

Folded in from #98

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

github-actions Bot commented Aug 29, 2026 •

edited

Loading