Skip to content

fix(vt): handle SCO restore cursor (CSI u) - #921

Open
PeterSR wants to merge 1 commit into
charmbracelet:mainfrom
PeterSR:fix/sco-restore-cursor
Open

fix(vt): handle SCO restore cursor (CSI u)#921
PeterSR wants to merge 1 commit into
charmbracelet:mainfrom
PeterSR:fix/sco-restore-cursor

Conversation

@PeterSR

@PeterSR PeterSR commented Jul 22, 2026

Copy link
Copy Markdown

What's broken

vt implements the SCO save-cursor sequence (CSI s, ansi.SCOSC) but not the
matching restore (CSI u, ansi.SCORC). In vt/handlers.go, registerDefaultCsiHandlers
registers a handler for 's' that either sets DECSLRM margins (when
ansi.ModeLeftRightMargin is set) or calls e.scr.SaveCursor() for the SCO case.
There is no corresponding e.RegisterCsiHandler('u', ...) anywhere in the package, so
CSI u is parsed correctly by the sequence parser but matches no handler and is
silently dropped.

The visible effect: after ESC[s, a following ESC[u does not move the cursor. The
cursor stays wherever it happened to end up, so the next write lands at the wrong
column and gets appended instead of overwriting the saved position.

Minimal repro

Write the following to the emulator and inspect row 0:

ESC[2J ESC[1;1H ESC[s AAAA ESC[u BB
Step Sequence Effect
1 ESC[2J clear screen
2 ESC[1;1H move to row 1, col 1
3 ESC[s save cursor position (SCOSC)
4 AAAA write, cursor now at col 5
5 ESC[u restore cursor position (SCORC), should return to col 1
6 BB write, should overwrite the first two cells

Expected row 0: "BBAA" (steps 5-6 return to the saved position and overwrite).

Actual row 0 (before this patch): "AAAABB" (step 5 is a no-op, step 6 appends after
the existing text instead of overwriting it).

Every other repositioning mechanism already works

Only the SCO pair is affected. Every other way of getting the cursor back to the start
of the line correctly produces "BBAA":

Mechanism Sequence(s) Result
Carriage return \r "BBAA" (correct)
CHA (cursor horizontal absolute) ESC[1G "BBAA" (correct)
CUB (cursor backward) ESC[4D "BBAA" (correct)
CUP (cursor position) ESC[1;1H "BBAA" (correct)
DEC save/restore ESC7 / ESC8 "BBAA" (correct)
SCO save/restore ESC[s / ESC[u "AAAABB" (bug: restore is dropped)

xterm and vte both implement CSI u as SCO restore-cursor; this emulator should match.

The fix

Register a CSI u handler that calls e.scr.RestoreCursor(), mirroring the existing
DECRC handler (ESC 8) already registered in the same file:

e.RegisterCsiHandler('u', func(params ansi.Params) bool {
	// Restore Current Cursor Position [ansi.SCORC]
	e.scr.RestoreCursor()
	return true
})

Placed immediately after the 's' handler registration in
registerDefaultCsiHandlers, with a comment mirroring the neighbouring
// Save Current Cursor Position [ansi.SCOSC] style. ansi.SCORC already exists as a
named constant (ansi/cursor.go), same as ansi.SCOSC.

Added a table-driven case to vt/emulator_test.go asserting SCOSC/SCORC repositions
the cursor, next to an equivalent DECSC/DECRC (ESC7/ESC8) case so the symmetry
between the two save/restore pairs is explicit.

Kitty keyboard protocol is unaffected

The kitty keyboard protocol also ends sequences in u (CSI > flags u,
CSI = flags ; mode u, CSI < number u), which raised the question of whether a plain
'u' handler could swallow them. It cannot: those sequences carry a prefix byte
(>, =, <), and ansi.Command(prefix, inter, final) packs the prefix into higher
bits of the command int, separate from the final byte. handleCsi dispatches on that
full packed int (h.csiHandlers[int(cmd)]), so a handler registered for bare 'u'
(prefix 0) occupies a different map key than one registered for
ansi.Command('>', 0, 'u'), ansi.Command('=', 0, 'u'), or ansi.Command('<', 0, 'u').
The new handler only ever fires for unprefixed CSI u, i.e. SCORC.

Testing

gofmt -l vt
go vet ./vt/...
go test ./vt/...

All clean. Confirmed the new SCOSC/SCORC test fails without the handler (and that the
DECSC/DECRC case passes either way), then passes with the one-hunk fix applied.

Where this turned up

Found downstream in pupptyeer, which renders PTY output with this emulator: PeterSR/pupptyeer#5 currently carries a byte-rewrite workaround (ESC[u -> ESC8 on the way into the emulator) that this patch makes unnecessary.

vt registers a CSI 's' handler that saves the cursor for SCO console
mode (ansi.SCOSC) when DECSLRM is not active, but there was no
matching CSI 'u' handler for the SCO restore (ansi.SCORC). The
sequence parsed fine and simply matched no handler, so ESC[u was
silently dropped: the cursor never moved back and subsequent writes
landed at the wrong column instead of overwriting.

Register a CSI 'u' handler that calls scr.RestoreCursor(), mirroring
the existing DECSC/DECRC pair (ESC 7 / ESC 8) already wired up in the
same file. This does not collide with the kitty keyboard protocol
sequences on the same final byte (CSI > u, CSI = ... u, CSI < u):
those carry a prefix byte, which is packed into the command int
separately from a bare final byte, so they resolve to distinct
handler keys.

Adds a table-driven test asserting that SCOSC/SCORC repositions the
cursor, next to an equivalent DECSC/DECRC case for symmetry.
PeterSR added a commit to PeterSR/pupptyeer that referenced this pull request Jul 22, 2026
…-close race (#5)

Two independent daemon-side fixes in pkg/ptysession, no wire-protocol change.

SCO restore-cursor (fixes #4): the vt emulator handles ESC[s (SCOSC) but
has no ESC[u (SCORC) handler, so restore was silently dropped and later
writes landed at the wrong column. Rewrite ESC[u into ESC8 (DECRC) on the
way into the emulator only; ring and OnOutput keep the child's bytes
verbatim. Upstream fix filed at charmbracelet/x#921; scorc.go carries a
TODO to delete once it lands.

Data race: Emulator.Close writes an unsynchronised closed bool that
Emulator.Read checks while drainTerm is parked in it. closeTerm closes the
reply pipe directly via InputPipe instead, so the flag is never written.
Go tests now run under -race in CI and make test.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant