Repository navigation
feat: input parser probe-response recognition → CapabilityEvent - #132
Conversation
Changeset suggestionThe current changeset no longer matches this PR. This review contains a corrected replacement. Why: Changeset package scope does not match the affected packages confidently. Changeset bump does not match the consumer-visible impact. View the proposed changeset---
'@bomb.sh/tty': minor
---
Adds `CapabilityEvent` to `InputEvent` and changes `InputOptions.terminfo` to accept a `TerminalInfo`.
`scan()` now parses terminal probe responses — OSC 10/11/12 theme colors, OSC 21 kitty color protocol, OSC 22 pointer shape, XTGETTCAP (`DCS`), kitty graphics (`APC`), kitty keyboard (`CSI ?…u`), synchronized output (`DECRPM`), and DA1 — and surfaces them as typed `CapabilityEvent` objects with keys `foreground-color`, `background-color`, `cursor-color`, `colordepth`, `sync-output`, `kitty-keyboard`, `kitty-graphics`, and `pointer-shape`.
`InputOptions.terminfo` now takes the `TerminalInfo` returned by `detectTerminal()` instead of raw compiled terminfo bytes. It seeds the key-sequence trie from `terminfo.keys` and uses `terminfo.capabilities.colors` to resolve colordepth denial events to the correct tier (`"16"` vs `"256"`). Raw bytes now go to `detectTerminal({ entry })`.
#### Migration
```diff
- import { createInput } from "@bomb.sh/tty";
+ import { createInput, detectTerminal } from "@bomb.sh/tty";
- const input = await createInput({ terminfo: myTerminfoBinary });
+ const terminfo = await detectTerminal({ env: process.env, entry: myTerminfoBinary });
+ const input = await createInput({ terminfo });
```
Omit `terminfo` entirely to keep the xterm default key sequences.Review this changeset manually If this draft is correct, react with 🚀 and Cooper will commit it to this branch.
|
commit: |
|
Size Increased — +7.7 KB 120.2 KB unpacked |
Merging this PR will degrade performance by 25.64%
Performance Changes
Comparing |
ff3f7c4 to
299df82
Compare
4b430b6 to
fb273f9
Compare
fb273f9 to
a2a08b1
Compare
a2a08b1 to
aeb96df
Compare
|
I consider the performance regression here acceptable for the capability we're gaining, but we may want to do a dedicated performance optimization pass once we're comfortable with the feature set. |
| @@ -0,0 +1,22 @@ | |||
| --- | |||
There was a problem hiding this comment.
Changeset needs revision.
Changeset package scope does not match the affected packages confidently. Changeset bump does not match the consumer-visible impact.
View the proposed replacement
---
'@bomb.sh/tty': minor
---
Adds `CapabilityEvent` to `InputEvent` and changes `InputOptions.terminfo` to accept a `TerminalInfo`.
`scan()` now parses terminal probe responses — OSC 10/11/12 theme colors, OSC 21 kitty color protocol, OSC 22 pointer shape, XTGETTCAP (`DCS`), kitty graphics (`APC`), kitty keyboard (`CSI ?…u`), synchronized output (`DECRPM`), and DA1 — and surfaces them as typed `CapabilityEvent` objects with keys `foreground-color`, `background-color`, `cursor-color`, `colordepth`, `sync-output`, `kitty-keyboard`, `kitty-graphics`, and `pointer-shape`.
`InputOptions.terminfo` now takes the `TerminalInfo` returned by `detectTerminal()` instead of raw compiled terminfo bytes. It seeds the key-sequence trie from `terminfo.keys` and uses `terminfo.capabilities.colors` to resolve colordepth denial events to the correct tier (`"16"` vs `"256"`). Raw bytes now go to `detectTerminal({ entry })`.
#### Migration
```diff
- import { createInput } from "@bomb.sh/tty";
+ import { createInput, detectTerminal } from "@bomb.sh/tty";
- const input = await createInput({ terminfo: myTerminfoBinary });
+ const terminfo = await detectTerminal({ env: process.env, entry: myTerminfoBinary });
+ const input = await createInput({ terminfo });
```
Omit `terminfo` entirely to keep the xterm default key sequences.Match createInput: the renderer option is `terminfo`, typed `TerminalInfo`. Adds the first test that seeds createTerm from a detected TerminalInfo (previously untested; test/caps.ts helpers were unused). The stack changeset now covers only what this PR and #131 add — detectTerminal/TerminalInfo, createTerm's option, term.capabilities, and the update() signature change — since #132 carries its own changeset for the input side.
Match createInput: the renderer option is `terminfo`, typed `TerminalInfo`. Adds the first test that seeds createTerm from a detected TerminalInfo (previously untested; test/caps.ts helpers were unused). The stack changeset now covers only what this PR and #131 add — detectTerminal/TerminalInfo, createTerm's option, term.capabilities, and the update() signature change — since #132 carries its own changeset for the input side.
Match createInput: the renderer option is `terminfo`, typed `TerminalInfo`. Adds the first test that seeds createTerm from a detected TerminalInfo (previously untested; test/caps.ts helpers were unused). The stack changeset now covers only what this PR and #131 add — detectTerminal/TerminalInfo, createTerm's option, term.capabilities, and the update() signature change — since #132 carries its own changeset for the input side.
0d1f019 to
660a987
Compare
There was a problem hiding this comment.
In addition to the memory question, I asked a robot to ponder the edge cases, and this is what it had to say:
The tests mostly cover valid, complete replies. They don’t sufficiently verify how the parser recovers from incomplete or invalid input, combines conflicting replies, or handles exhausted capacity. Those gaps can leave input stuck or produce incorrect events.
These are the tests I’d add, tied to what I observed:
| Test cases | Example and what it would catch |
|---|---|
resolves ambiguous response prefixes after a timeout |
Reproduced: feeding ESC P—also the encoding of Alt+P—produced no event and no pending delay. The parser waits for a DCS response without scheduling recovery. Include ESC ] and ESC _ as related cases. |
recovers when an incomplete response fills the input buffer |
Reproduced: ESC[? followed by 4,093 semicolons filled the 4,096-byte buffer. Feeding "a" afterward accepted zero bytes and produced zero events or pending delay. |
preserves truecolor when RGB succeeds and Tc fails, in either order |
Reproduced: ESC P1+r524742 ST followed by ESC P0+r5463 ST emitted "truecolor" followed by "256". One unsupported capability incorrectly undoes the positive evidence from the other. Test replies both together and across separate scans. |
emits the static color tier only when both RGB and Tc are denied |
Inspection: the denial branch immediately downgrades on any invalid XTGETTCAP reply. It doesn’t track whether both requested capabilities failed. Test both "16" and "256" baselines. |
rejects malformed payloads and preserves the following key |
Inspection: color parsing accepts a valid prefix without requiring the entire payload to be valid. For example, #fffjunk reaches the success path. Follow malformed replies with "a" and verify that no capability event is fabricated and the key survives recovery. |
recovers from oversized OSC, DCS, APC, and private CSI responses / handles numeric overflow |
Inspection: OSC/DCS/APC have a length check, but private CSI does not. Its decimal accumulator also multiplies an int without overflow checks. Exercise length boundaries and long digit strings, then verify subsequent input still works. |
preserves default key bindings or reports trie exhaustion explicitly |
Inspection: trie_add() returns -1 when its 1,024 nodes are exhausted, but initialization ignores that result. Custom sequences are inserted first, so they can leave no room for defaults. I did not reproduce exhaustion. |
does not fabricate events for unknown capability codes |
Inspection: mapCapEvent() defaults to { type: "capability", key: "pointer-shape", value: false }. An unknown code therefore becomes a plausible but unrelated event. |
recognizes every supported response split at every byte boundary |
Coverage gap: the new suite tests one split inside an OSC color payload. It should also split immediately after ESC, inside response prefixes, and between the two ST terminator bytes, while checking for duplicate events and leaked bytes. No failure demonstrated for this case. |
The timeout recovery policy and trie-exhaustion behavior should be explicit in the spec before their tests prescribe a particular outcome.
| if (keys && keys.byteLength > 0) { | ||
| top = (top + 7) & ~7; | ||
| keysPtr = top; | ||
| keysLen = keys.byteLength; | ||
| top += (keysLen + 7) & ~7; | ||
| let pages = Math.ceil(top / 65536); | ||
| let current = memory.buffer.byteLength / 65536; | ||
| if (pages > current) memory.grow(pages - current); | ||
| new Uint8Array(memory.buffer).set(keys, keysPtr); | ||
| } |
There was a problem hiding this comment.
When we (potentially) grow the memory, we should also make sure that we include the size of the parser state and the transfer buffer to make sure we have room for both. Do we? I can't tell, but it looks like perhaps not?
Perhaps a comment on how we layout the WASM memory would help keep everything in sync, because I'm definitely not sure where each region begins and ends, which we'd need to make sure we always have enough.
|
@cowboyd thanks for the robo-review! all valid 😅 addressed in 4fed036...9f88c97 with regression tests |
Revert the `terminfo` -> `detection` option rename. The option keeps its
name and changes format instead: it takes the `Detection` returned by
`detectTerminal()` rather than raw compiled terminfo bytes, which now go to
`detectTerminal({ terminfo })`. Specs updated for both `createInput` and
`createTerm` so the stack converges on one option name.
Changeset rewritten as a breaking-change note and fixed to target
`@bomb.sh/tty` (it referenced a nonexistent `@bombshell/input`).
`terminfo` now means one thing across the public API: the resolved `TerminalInfo` passed to `createInput`/`createTerm`. The raw compiled bytes move from `DetectOptions.terminfo` to `DetectOptions.entry` (ncurses' term for one compiled description), and `MAX_TERMINFO` becomes `MAX_TERMINFO_ENTRY` to match. `Detection` named how the value was made rather than what it holds. None of these have shipped yet.
The spec said detectTerminal() never rejects, but an `entry` over MAX_TERMINFO_ENTRY has always thrown a RangeError (and is tested). Keep that behavior and say why: it is caller error. Environmental conditions — missing, malformed, or oversized files on the search path — still resolve to the baseline. Adds the missing test for skipping an oversized file found on the search path.
createInputNative lays out linear memory as four consecutive 8-byte aligned regions above __heap_base: the terminfo key table, the InputState (input_size()), then the SCAN_BUFFER_SIZE transfer buffer. Growth was only computed up to the end of the key table, so the state and the transfer buffer could land past the end of memory. Today's numbers hide it (heap base ~72 KB + 32 KB max entry + 23 KB state + 4 KB buffer fits in the initial 4 pages), but TerminalInfo is a plain interface and a key table above ~180 KB trapped with "memory access out of bounds" inside input_init. Compute every region first, then grow once to cover the end of the transfer buffer. The code now reads top-down as the memory map.
parse_csi_private had no length limit: `ESC [ ?` followed by enough parameter bytes to fill the 4096-byte scan buffer kept returning PARSE_NEED_MORE, input_scan accepted zero bytes from then on, and the parser was wedged permanently. Before this branch the same bytes fell through to the Alt+[ fallback, so this was a regression. Apply the MAX_RESPONSE limit the OSC/DCS/APC paths already use; past it the sequence is rejected and falls back like any unrecognized ESC sequence. The decimal accumulator also multiplied a signed int without bounds, which is UB in C and wraps in wasm: `ESC [ ? 4294969322 ; 1 $ y` (2026 + 2^32) surfaced as a sync-output=true event. Parameters now saturate above MAX_CSI_PARAM, which no recognized mode reaches.
…denied terminfo-spec §6.3 says the static-tier denial is emitted when XTGETTCAP rejects both RGB and Tc, but every `DCS 0 + r` reply downgraded immediately. A terminal answering RGB=ok, Tc=invalid produced "truecolor" followed by "256", throwing away the positive evidence; the reverse order produced "256" then "truecolor". The parser now tallies the two replies in InputState. A valid reply for either name still emits "truecolor" and suppresses any later denial. A denial only emits the static tier once both names are denied. A `DCS 0 + r ST` that names neither capability (terminals that stop at the first unknown name) counts as denying both, which keeps the existing single-reply behaviour. The tally resets on the DA1 fence so a re-sent probe is evaluated from scratch. This is parser-private bookkeeping, not shared state, so TINV-6 holds.
parse_color_spec accepted any valid prefix: `#fffjunk` and `rgb:ff/ff/ffzz` both produced a white color event, and the `rgba:` form ignored whatever followed the blue channel. A malformed reply should be consumed without fabricating a CapabilityEvent. Both forms must now consume the whole payload. `rgba:` must carry a well-formed 1-4 digit alpha channel, which is still discarded. The surrounding OSC is still consumed through its terminator, so the key that follows a malformed reply is unaffected.
mapCapEvent's default branch turned any CAP_* code it did not know into
`{ key: "pointer-shape", value: false }`, a plausible but unrelated
event that would silently overwrite real pointer-shape state in
term.update(). Unknown codes can only appear if src/input.h and
input-native.ts drift apart, and dropping the event is the honest
outcome in that case.
No test: the C side only emits the eight known codes, and reaching the
default branch would mean exporting mapEvent through mod.ts's
`export * from "./input.ts"`, which would widen the public API.
The suite only split one OSC color payload. Feed each response from terminfo-spec §9.1, wrapped in an `x … y` key pair, split into two scans at every byte offset and also one byte per scan. That covers splits right after ESC, inside the OSC/DCS/APC/CSI introducers, inside payloads, and between the two bytes of an ST terminator, and asserts the exact event list so duplicated events or leaked response bytes fail. escLatency is set high so a split after a lone ESC never times out on a slow runner. No behaviour change; this passed before and after the recent parser fixes.
OSC/DCS/APC responses longer than MAX_RESPONSE are rejected and fall back to the unrecognized-ESC path. Pin the parts of that recovery that matter: no CapabilityEvent is fabricated from the oversized payload, and a key typed afterwards still comes through. The test does not pin how the rejected bytes themselves surface, which is a separate policy question.
… length cap Three parser behaviours that review showed were unspecified: - §4.3: ESC followed only by `[`, `O`, `]`, `P`, or `_` is pending like a lone ESC and resolves to the Alt-modified key after escLatency. Without this, Alt+P/]/_ (regressed by response recognition) and Alt+[/O (already broken on main) sat in the buffer until the next byte arrived. - §6.3: once an OSC/DCS/APC response header is recognized, BEL and ST terminate it, while any other C0 control, DEL, or an ESC not followed by `\` ends it early. The partial response is discarded and parsing resumes at the ending byte, so a reply cut off mid-payload cannot swallow the user's Enter or the next key sequence. The set is every byte a well-formed reply payload never contains; DEL is included because it is what Backspace sends. - §6.1: terminfo key sequences longer than 16 bytes are ignored. The default xterm tables use 307 of the trie's 1024 nodes, so capping the 23 terminfo keys at 16 bytes (368 nodes worst case) makes trie exhaustion impossible rather than something to report.
Implements input-spec §4.3. `ESC [`, `ESC O`, `ESC ]`, `ESC P` and `ESC _` are each a complete Alt+key press and also a prefix of a longer sequence. The trie, CSI and response parsers all answered PARSE_NEED_MORE for them without arming the ESC timer, so input_delay reported nothing and the bytes sat in the buffer until something else was typed. Alt+[ and Alt+O behaved that way on main; Alt+], Alt+P and Alt+_ started doing it when response recognition claimed their introducers. The two-byte case now goes through the same timer as a lone ESC: it reports `pending`, and a rescan after escLatency emits the introducer with MOD_ALT. When the ESC arrived alone in an earlier scan, the timer keeps running from that arrival.
Implements input-spec §6.3. After an OSC/DCS/APC response header was recognized, find_st scanned for BEL or ST and nothing else. A reply cut off mid-payload then swallowed whatever the user typed next, Enter and Ctrl+C included, until a terminator or MAX_RESPONSE bytes turned up. An ESC that was not part of an ST was rejected outright, so the parser fell back to Alt+introducer and replayed the partial reply as keystrokes. find_st now stops at any C0 control other than BEL and ESC, at DEL, and at an ESC not followed by `\`. It reports where the response ended; the parser discards the partial reply without an event and resumes at that byte. The key it starts comes through as if the response had never been there, and so does a following response that begins with ESC.
Implements input-spec §6.1. Terminfo keys go into the trie before the xterm defaults so that the entry wins on conflicts, and input_init ignored trie_add's -1 when the 1024 nodes ran out. One long key string was enough to use up the trie and silently drop the defaults: an entry with 1000-byte key strings left `ESC O A` parsing as Alt+O, A. Key strings longer than MAX_TERMINFO_KEY (16) are now skipped. Real key sequences are a handful of bytes, and the defaults use 307 nodes, so the worst case (23 keys x 16 nodes = 368) always fits and trie_add can no longer fail during init. The tests build legacy-format terminfo entries in memory so they can hit the 16/17-byte boundary and the exhaustion case without new binary fixtures.
Review asked for the layout to be written down next to the code that computes it, so the regions and their order can be checked at a glance when sizes change.
Match createInput: the renderer option is `terminfo`, typed `TerminalInfo`. Adds the first test that seeds createTerm from a detected TerminalInfo (previously untested; test/caps.ts helpers were unused). The stack changeset now covers only what this PR and #131 add — detectTerminal/TerminalInfo, createTerm's option, term.capabilities, and the update() signature change — since #132 carries its own changeset for the input side.
9f88c97 to
291910b
Compare
Match createInput: the renderer option is `terminfo`, typed `TerminalInfo`. Adds the first test that seeds createTerm from a detected TerminalInfo (previously untested; test/caps.ts helpers were unused). The stack changeset now covers only what this PR and #131 add — detectTerminal/TerminalInfo, createTerm's option, term.capabilities, and the update() signature change — since #132 carries its own changeset for the input side.
#101's bespoke parse_osc() drew review for unbounded digit accumulation on `ESC ] 9999...`. The rebuild drops that parser and relies on #132's parse_osc_response(), which already bails once the OSC number passes 22 and caps payloads at MAX_RESPONSE. These tests pin that behavior for OSC 22 so it cannot regress: BEL and ST terminators, the "0" empty-stack reply, byte-at-a-time delivery, a broken ST, a 64-digit OSC number, OSC 220, and an over-long payload (no event, parser recovers for the next reply).
… them (renderer-spec §7.9)
Elements declare open(id, { pointerShape }). While
term.capabilities.pointerShape is true (raised by #132/#133 from the
probe's OSC 22 query reply), each render resolves the shape under the
pointer and appends `OSC 22 ; <shape> ST` to result.output when it
changes. The host keeps writing one buffer (cowboyd on #101). The
capability is the only gate; there is no createTerm option.
Everything lives in TypeScript; the wasm module and the packed encoding
are untouched, so there is no size or startup cost (#101 regressed
createTerm ~20% and grew the bundle 8.3 KB). Without the capability the
render path pays one boolean check and never reads pointerShape (a test
pins this with a counting getter). With it, the directive walk runs only
when the pointer is over something, and only a frame whose shape
changed copies output to append the sequence.
Resolution takes the last declaring id in Clay's pointer-over order:
pre-order within the topmost layer the pointer reaches, so the innermost
element wins and capture-mode floats hide what is beneath while
passthrough floats defer to it. snapshot() records its declared shapes
beside the packed bytes so pre-packed subtrees still participate.
Values outside the kitty/CSS vocabulary are ignored (and rejected by
validate()), so arbitrary strings never reach an OSC payload.
Restore (dreyfus92 on #101): set-only OSC 22 cannot pop, so the emitted
shape is the one piece of cross-frame state. It resets to "default"
when the pointer leaves or is omitted, and update() returns the reset
when the pointer-shape capability is withdrawn. Resize keeps it, since
the terminal's pointer did not change.
Part 3/4 of the terminfo foundation stack. Requires #131.
src/input.{c,h}: the input parser seeds its escape-sequence trie from the terminfokeystable and recognizes probe responses (OSC 10/11/12/21/22, XTGETTCAP, DECRPM 2026, kitty keyboard/graphics APC, DA1 fence)input.ts:scan()surfaces those responses asCapabilityEventvalues alongside key and mouse events, ready to hand toterm.update()createInput({ terminfo })keeps its name but now takes theTerminalInfofromdetectTerminal()instead of raw bytesDetectionis renamedTerminalInfo; raw terminfo bytes move todetectTerminal({ entry })soterminfomeans one thing everywhere (MAX_TERMINFO→MAX_TERMINFO_ENTRY); none of these have shipped yet