Conversation
ThomasK33
left a comment
There was a problem hiding this comment.
Thanks for this. The binding is thin in the right way. It delegates to libghostty-vt's own ghostty_mouse_encoder_* / ghostty_mouse_event_* API instead of reimplementing the protocols, it reads the negotiated mode from the terminal, and the native lifetime handling is careful. I found one blocking issue: extreme (but finite) inputs reach unchecked integer conversions inside Ghostty. Details and verification are below.
Blocking
1. Out-of-range coordinates or geometry trigger illegal behavior inside Ghostty's encoder. The package builds Ghostty with ReleaseFast, so this is undefined behavior.
The binding accepts any finite float for x/y and any u32 for geometry. Ghostty then converts these with unchecked casts:
posToCell:@intFromFloat(clamped_x / cell_width)into au16(vendor/ghostty/src/renderer/size.zig:144-145). For release events this runs no matter where the pointer is (src/input/mouse_encode.zig:90-104).GridSize.update:@intFromFloat(screen_width / cell_width)into au16(size.zig:258-259).subPadding:padding.left + padding.righton u32 values (size.zig:201).posToPixels: the@intFromFloattoi32for SGR-pixels has the same problem (mouse_encode.zig:279-280).
scripts/build-libghostty-vt.sh defaults to -Doptimize=ReleaseFast, which removes these checks. What I observed on the PR head (x86_64 Linux, Node 25.9, pinned Ghostty 48ccec1):
| Input (80x24 terminal, unit geometry unless noted) | ReleaseFast output (shipped config) | ReleaseSafe build of the same commit |
|---|---|---|
SGR, release left x=1e30 y=3 |
ESC[<0;1;4m (column 1; the clamp says it should be 80) |
panic: integer part of floating point value out of bounds, process aborts (exit 134) |
SGR-pixels, release left x=1e12 y=3 |
ESC[<0;-2147483648;3m |
same panic |
SGR, press x=99999, screenWidth: 100000 |
ESC[<0;34464;4M (wrapped mod 65536) |
same panic |
SGR, paddingLeft: 2**32-1, paddingRight: 2 |
ESC[<0;1;4M |
panic: integer overflow |
Mouse events often come from a remote client, for example a browser that relays pointer events over a websocket. So one bad or malicious value can send garbage bytes to the child PTY today. With a safety-checked libghostty build, it aborts the Node process. Ghostty's own docs say out-of-range positions are allowed (mouse_encode.zig:65-68), so this is also worth reporting upstream. The binding still needs to guard, because it ships ReleaseFast. A possible fix, in both the TS normalizer and the native parser:
- Reject geometry where
screenWidth / cellWidthorscreenHeight / cellHeightexceeds 65535 (the same u16 limitresize()uses), and wherepaddingLeft + paddingRightorpaddingTop + paddingBottomexceeds the screen size. Compute the sum in 64-bit. - Clamp
x/yso that(x - paddingLeft) / cellWidthstays within the 16-bit cell range and the SGR-pixels value fits ini32. Keep a margin outside the viewport, so out-of-viewport and drag semantics are unchanged. - Add regression tests for these inputs. The current invalid-input test would not catch this, because the calls return without throwing.
Non-blocking suggestions
-
Motion dedup does not survive
feed(). Every non-emptyfeed()setsmouse_modes_dirty_(native/terminal.cc:258), andghostty_mouse_encoder_setopt_from_terminalalways clearslast_cell(vendor/ghostty/src/terminal/c/mouse_encode.zig:195). I confirmed that in any-event mode withtrackLastCell: true, motion in the same cell is emitted again afterterm.feed("output from child"). Child output is continuous in practice, sotrackLastCellrarely helps. The PR text says dedup state is kept while geometry is unchanged, which is only true between feeds. Option: read modes withghostty_terminal_mode_getand applyGHOSTTY_MOUSE_ENCODER_OPT_EVENT/OPT_FORMAT, which only resetlast_cellwhen the value changes (mouse_encode.zig:146,158). Otherwise, document the limitation. -
Geometry is not reconciled with
cols/rows. With an 80-column terminal andscreenWidth: 100, a press atx=95encodes column 96. At minimum, document that geometry must describe the same grid asresize(). Consider validating it. -
Buttons
tenandelevenare accepted but never encoded. Ghostty'sbuttonCodereturns null for them (src/input/mouse_encode.zig:225), although xterm defines 130/131. The test asserts empty output. The README should say so, so callers don't assume buttons 1–11 all work. -
Wheel "release". Legacy formats encode every release as button 3, so a wheel release looks like a normal button release. Suggest a README line saying wheel buttons should only be sent as
press. -
supportsMouseInput. This is a new style of export. The existing capability surface isgetNativeInfo(), and the constant is not documented in the README's public-contract list. Please either document it there or drop it, sincetypeof term.encodeMouse === "function"already works without native allocation on a terminal the caller already has. -
Test coverage gaps. I verified all of the following locally and they behave correctly, but none have tests:
- button-event tracking (
?1002): motion with a button is reported and motion without one is suppressed - legacy release encoding (X10
ESC[M#…, urxvtESC[35;…M) - X10 releases and wheel presses are dropped under
?9 - the X10 223-column limit (column 223 is encoded, 224 is dropped)
- UTF-8 multibyte coordinates (column 100 gives
0xC2 0x85)
Also,
expect(invalidCall).toThrow()without a message pattern would pass for any error. The existing tests match on/cols/and similar. - button-event tracking (
-
CHANGELOG. Release notes are generated by Communique in the
release/v*flow. Please check with the maintainers whether hand-written[Unreleased]entries are wanted, so the entry isn't duplicated.
What I verified by running it
- Checked out the PR head
fae62e2locally, with nothing pushed. The PR does not changepackage.json,package-lock.json,binding.gypor scripts, so I ran the standard pipeline:npm ci,npm run build:libghostty(Ghostty48ccec1, Zig 0.15.2, ReleaseFast),npm run build:native(no compiler warnings),npm run build, andnpm run verify(typecheck, then vitest with 2 files and 13 tests passing, then smoke). All passed. - Checked the tests' byte expectations against the protocols:
- SGR button codes 0/1/2, wheel 64–67, and extended buttons 128/129
- modifier bits: shift +4, alt +8, ctrl +16, and motion +32
- X10 mode drops modifiers
- SGR
mrelease keeps the button code - urxvt adds 32
- SGR-pixels uses raw pixel coordinates
- Native code: event allocation and free are balanced on every path (
try/catchrethrow). Output is sized with a two-passOUT_OF_SPACEquery, then aBuffer::Copyofwritten. Encoder and terminal are freed and nulled inDisposeNative, and the constructor's failure path cleans up.GhosttyMouseEncoderSize.sizeis set. The raw native object is only reached through the TS wrapper, which passes freshly built plain objects, so user getters cannot re-enterdispose()duringEncodeMouse. - Ran the ReleaseSafe build and probes described above in a separate scratch copy.
CI: this commit has no CI results. The ci and release-changelog runs from 2026-08-19 finished as failure with zero jobs, and GitHub updated them exactly 30 days later. That matches a first-time-contributor approval request that expired. A maintainer needs to re-run or approve the workflows after the next push.
Generated with xum • Model: anthropic:claude-opus-5-5 • Thinking: high
| size.screen_width = PositiveUint32(env, geometry.Get("screenWidth"), "screenWidth"); | ||
| size.screen_height = PositiveUint32(env, geometry.Get("screenHeight"), "screenHeight"); | ||
| size.cell_width = PositiveUint32(env, geometry.Get("cellWidth"), "cellWidth"); | ||
| size.cell_height = PositiveUint32(env, geometry.Get("cellHeight"), "cellHeight"); | ||
| size.padding_top = NonNegativeUint32(env, geometry.Get("paddingTop"), "paddingTop"); | ||
| size.padding_bottom = NonNegativeUint32(env, geometry.Get("paddingBottom"), "paddingBottom"); | ||
| size.padding_right = NonNegativeUint32(env, geometry.Get("paddingRight"), "paddingRight"); | ||
| size.padding_left = NonNegativeUint32(env, geometry.Get("paddingLeft"), "paddingLeft"); |
There was a problem hiding this comment.
Blocking (see review item 1). Each field is bounded only to u32. Ghostty then computes screen_width / cell_width into a u16 (vendor/ghostty/src/renderer/size.zig:258-259) and padding.left + padding.right in u32 (size.zig:201). Both are unchecked in the shipped ReleaseFast build. screenWidth: 100000 produced column 34464 for x=99999, and paddingLeft: 2**32-1, paddingRight: 2 hit panic: integer overflow in a ReleaseSafe build. Please reject grids wider or taller than 65535 cells (the same limit as resize()) and padding sums larger than the screen size. Compute the sums in 64-bit.
| FiniteFloat(env, input.Get("x"), "mouse x"), | ||
| FiniteFloat(env, input.Get("y"), "mouse y"), |
There was a problem hiding this comment.
Blocking (see review item 1). Any finite f32 passes. Ghostty's posToCell then does @intFromFloat(x / cell_width) into a u16 (size.zig:144-145), and for releases it does this wherever the pointer is. release left x=1e30 encodes column 1 instead of the clamped 80 in the shipped build and aborts the process in a ReleaseSafe build. SGR-pixels x=1e12 sends -2147483648 to the child. Please clamp x/y to a range whose cell index fits in 16 bits and whose pixel value fits in i32, with a small margin outside the viewport so out-of-viewport and drag semantics stay the same.
| size.padding_left = NonNegativeUint32(env, geometry.Get("paddingLeft"), "paddingLeft"); | ||
|
|
||
| if (mouse_modes_dirty_) { | ||
| ghostty_mouse_encoder_setopt_from_terminal(encoder, terminal); |
There was a problem hiding this comment.
Non-blocking: setopt_from_terminal always clears last_cell (vendor/ghostty/src/terminal/c/mouse_encode.zig:195), and every non-empty feed() marks the modes dirty. So trackLastCell dedup is lost whenever the child prints anything. I confirmed a duplicate same-cell motion after term.feed("output from child"). You could read the modes with ghostty_terminal_mode_get and set OPT_EVENT/OPT_FORMAT, which only reset on change (lines 146/158). Otherwise, document the limitation.
|
|
||
| // Fill stable defaults while preserving explicit renderer geometry for both | ||
| // cell-based and pixel-based terminal protocols. | ||
| function normalizeMouseOptions(options: MouseEncoderOptions): MouseEncoderOptions { |
There was a problem hiding this comment.
Once the native bounds from review item 1 exist, it would help to mirror them here (grid size of at most 65535 cells, padding sums no larger than the screen, clamped or bounded x/y). Then callers get the same TypeError style as the other validators, before any native call.
| } from "./types.js"; | ||
|
|
||
| /** Import-time capability marker for consumers which must avoid native allocation. */ | ||
| export const supportsMouseInput = true; |
There was a problem hiding this comment.
Non-blocking: the existing capability surface is getNativeInfo(), and this constant isn't listed in the README's public-contract section. Please document it there or drop it. typeof term.encodeMouse === "function" already works without extra native allocation.
| ]; | ||
|
|
||
| for (const invalidCall of invalidCalls) { | ||
| expect(invalidCall).toThrow(); |
There was a problem hiding this comment.
Non-blocking: toThrow() without a pattern passes for any error. The existing tests match on messages (/cols/, /disposed/). Please add regression cases for the extreme inputs in review item 1 once they are bounded, plus the missing tests for ?1002, legacy release encoding, the X10 223-column limit, and UTF-8 multibyte coordinates.
| Buttons `four`, `five`, `six`, and `seven` conventionally represent wheel up, | ||
| wheel down, wheel left, and wheel right. The binding keeps Ghostty's names at | ||
| this low-level API boundary so consumers can provide their own user-facing | ||
| aliases. |
There was a problem hiding this comment.
Non-blocking: two things worth documenting here. First, ten and eleven are accepted but always return an empty buffer, because Ghostty's buttonCode has no mapping for them (src/input/mouse_encode.zig:225). Second, wheel buttons should only be sent as press, because legacy formats encode any release as button 3.
Summary
encodeMouse(event, options)on each terminal instanceBufferbytes suitable for writing to the child PTYCoverage
Tests cover X10 tracking, normal tracking with X10 wire format, UTF-8, URXVT, SGR, SGR-pixels, press/release/motion, modifiers, buttons 1 through 11, mode transitions, disabled and suppressed events, out-of-viewport drags, same-cell motion deduplication, invalid input sets, and disposal.
A deliberate mutation that stopped resynchronization after
feed()made the negotiated-mode regression test fail. An earlier implementation that reapplied unchanged geometry made the motion-dedup test fail, which is why the binding caches geometry.Verification
npm run build:nativenpm run buildnpm run verify