From 6e69fa2984dcc20469b63e9ef96520b82cea6ea0 Mon Sep 17 00:00:00 2001 From: Jake Savin Date: Mon, 29 Jun 2026 22:00:51 -0700 Subject: [PATCH 1/2] feat(boxen): C M6 #805 -- input_decoder cutover in tb2 backend Replace termbox2's input layer with the custom input_decoder built in M1-M5. The decoder now owns the TTY fd read loop end-to-end: escape-sequence parsing, modifier param decoding, mouse, bracketed paste, Kitty protocol, double-click synthesis. backend_tb2.c changes: - include input_decoder.h; add static input_decoder_t *g_decoder - tb2_init creates the decoder after tb_init; obtains TTY fd via tb_get_fds with /dev/tty fallback (risk R1) - tb2_shutdown destroys the decoder BEFORE tb_shutdown so any decoder writes land while raw mode is still active - tb2_poll_event forwards directly to input_decoder_poll; no translation - Removed translate_key, translate_mod, translate_mouse, and the g_tb2_last_press double-click state struct -- all subsumed by the decoder - Kept translate_attr and translate_color (rendering-side; unaffected) - DOES NOT call tb_set_input_mode(TB_INPUT_MOUSE). Mouse starts disabled per plan section 5.1; M7 /mouse toggle wires the enable input_decoder.c changes: - Add the real read(2) + select(2) path in input_decoder_poll. When tty_fd >= 0, block up to timeout_ms on select(2), then drain the pipe with non-blocking reads (collapses multi-byte escape sequences that arrive as one OS burst into one parse cycle). Test seam (tty_fd == -1) preserved -- inject_bytes path unchanged - Split poll body into dec_drain_buffer + orchestrator so the same parser loop runs both before and after the TTY fill - EINTR/EAGAIN handled correctly; EOF on the TTY surfaces as BOXEN_ERR_IO so the REPL can shut down rather than spin Test results at HEAD: - PTY-replay: 138/138 passed (no regression from M5 baseline) - Unit suite: 964/964 passed - Integration: 2186/2207 (21 pre-existing failures match develop) Closes #814 Closes #805 --- frontier-cli/boxen/backend_tb2.c | 374 +++++++++++------------------ frontier-cli/boxen/input_decoder.c | 278 +++++++++++++++++++-- 2 files changed, 408 insertions(+), 244 deletions(-) diff --git a/frontier-cli/boxen/backend_tb2.c b/frontier-cli/boxen/backend_tb2.c index d20bfff8c..6356facc4 100644 --- a/frontier-cli/boxen/backend_tb2.c +++ b/frontier-cli/boxen/backend_tb2.c @@ -5,157 +5,100 @@ * or include termbox2.h. All other boxen source files work through the * boxen_backend_t vtable and must never use termbox2 directly. * - * Responsibilities: - * - Translate TB_KEY_* -> BOXEN_KEY_* for special keys - * - Translate TB_MOD_* -> BOXEN_MOD_* modifiers - * - Translate TB_EVENT_* -> BOXEN_EV_* event types - * - Translate TB_KEY_MOUSE_* -> BOXEN_EV_MOUSE with button field - * - Synthesize double-click using tracked timestamp + position state - * - Translate BOXEN_ATTR_* -> TB_BOLD / TB_UNDERLINE / TB_REVERSE / TB_ITALIC + * Responsibilities (post-M6): + * - Bring up termbox2 for the rendering side (alternate screen, raw mode, + * SIGWINCH, cell buffer, color, cursor positioning). + * - Bring up the input_decoder for the read side -- it owns the TTY fd + * read loop, escape-sequence parsing, mouse-mode writes, bracketed + * paste, Kitty protocol enable, and double-click synthesis. All key + * and mouse translation that used to live in this file is now inside + * the decoder; this file just forwards bytes between the two layers. + * - Translate BOXEN_ATTR_* -> termbox2 attribute bits (rendering-side). + * - Translate BOXEN_COLOR_* -> termbox2 color values (rendering-side). * - * Key translation notes (per EXECUTION_PLAN.md Section 6): - * Tab/Enter/Esc/Backspace are translated BEFORE the generic CTRL_* range. - * TB_KEY_TAB (0x09) -> BOXEN_KEY_TAB (not BOXEN_KEY_CTRL_I) - * TB_KEY_ENTER (0x0d) -> BOXEN_KEY_ENTER (not BOXEN_KEY_CTRL_M) - * TB_KEY_ESC (0x1b) -> BOXEN_KEY_ESCAPE (not BOXEN_KEY_CTRL_BRACKET) - * TB_KEY_BACKSPACE (0x08) -> BOXEN_KEY_BACKSPACE (not BOXEN_KEY_CTRL_H) + * 2026-06-29 JES #805 M6: Cutover -- replaced tb_peek_event with + * input_decoder_poll. Removed translate_key, translate_mod, + * translate_mouse, and the g_tb2_last_press double-click state struct -- + * the input decoder subsumes all of that. The rendering path + * (translate_attr / translate_color / tb_set_cell / tb_present / etc.) + * is untouched. Mouse mode is NOT enabled in tb2_init; the decoder + * keeps it off by default per plan section 5.1, and the M7 /mouse + * slash command will toggle it. See planning/phase_c/INPUT_DECODER_PLAN.md + * sections 2.3, 5, 7 (M6 spec), and 8 (R1, R3, R4). */ #include "termbox2.h" #include "boxen_internal.h" +#include "input_decoder.h" +#include #include #include #include -#include +#include /* ------------------------------------------------------------------------- - * Clock for double-click synthesis - * ---------------------------------------------------------------------- */ - -static uint64_t tb2_now_ms(void) { - struct timespec ts; - clock_gettime(CLOCK_MONOTONIC, &ts); - return (uint64_t)ts.tv_sec * 1000 + (uint64_t)ts.tv_nsec / 1000000; -} - -/* ------------------------------------------------------------------------- - * Double-click state + * 2026-06-29 JES #805 M6: input decoder lifetime. + * + * Created in tb2_init after tb_init() (which sets up raw mode and the + * alternate screen on the TTY). Destroyed in tb2_shutdown BEFORE + * tb_shutdown() so the decoder's writes (mouse-disable / paste-disable on + * destroy paths in future milestones) land while the TTY is still in raw + * mode and the fd is still valid. + * + * Single instance per process -- the boxen_backend_t vtable is global and + * boxen_repl owns the only init/shutdown pair. This matches the single- + * owner threading invariant documented in input_decoder.h. * ---------------------------------------------------------------------- */ - -static struct { - uint64_t time_ms; - int x; - int y; - uint8_t button; - bool valid; -} g_tb2_last_press; - -static void tb2_maybe_set_double_click(boxen_event_t *ev) { - if (!ev->mouse.pressed) { - return; - } - - uint64_t now = tb2_now_ms(); - - if (g_tb2_last_press.valid) { - uint64_t elapsed = now - g_tb2_last_press.time_ms; - int dx = ev->mouse.x - g_tb2_last_press.x; - int dy = ev->mouse.y - g_tb2_last_press.y; - if (dx < 0) dx = -dx; - if (dy < 0) dy = -dy; - int dist = dx > dy ? dx : dy; - - if (elapsed <= BOXEN_DOUBLE_CLICK_MS - && dist <= BOXEN_DOUBLE_CLICK_RADIUS - && ev->mouse.button == g_tb2_last_press.button) { - ev->mouse.flags |= BOXEN_MOUSE_DOUBLE_CLICK; - g_tb2_last_press.valid = false; - return; - } - } - - g_tb2_last_press.time_ms = now; - g_tb2_last_press.x = ev->mouse.x; - g_tb2_last_press.y = ev->mouse.y; - g_tb2_last_press.button = ev->mouse.button; - g_tb2_last_press.valid = true; -} +static input_decoder_t *g_decoder = NULL; /* ------------------------------------------------------------------------- - * TB_KEY_* -> BOXEN_KEY_* translation + * 2026-06-29 JES #805 M6 / risk R1: obtain TTY fd from termbox2 if possible, + * otherwise open /dev/tty directly. * - * Order matters: check explicit aliases (Tab, Enter, Esc, Backspace) FIRST, - * before the generic CTRL_* numeric range check, because several TB_KEY_* - * values are assigned the same numeric value (e.g., TB_KEY_TAB == TB_KEY_CTRL_I). + * The vendored termbox2 (frontier-cli/third_party/termbox2/termbox2.h) does + * export tb_get_fds() -- verified at M6 implementation time. If a future + * vendored update drops or renames the symbol, the fallback opens /dev/tty + * which is what termbox2 itself opens internally when no fd is passed to + * tb_init. Risk R1 disposition: LOW; both paths are well-trodden. + * + * Returns -1 on failure -- caller must surface as BOXEN_ERR_INIT so the + * REPL can refuse to start cleanly instead of running with a wedged input + * path. fd_was_opened_out is set true when we opened /dev/tty ourselves + * (caller closes on shutdown) and false when we borrowed termbox2's fd + * (caller leaves alone -- tb_shutdown() closes it). * ---------------------------------------------------------------------- */ +static int tb2_obtain_tty_fd(bool *fd_was_opened_out) { + *fd_was_opened_out = false; -static boxen_key_t translate_key(uint16_t tb_key, uint32_t tb_ch) { - /* Printable character: key field is 0, ch carries the codepoint. */ - if (tb_ch != 0) { - return BOXEN_KEY_NONE; + int ttyfd = -1; + int resizefd = -1; + if (tb_get_fds(&ttyfd, &resizefd) == TB_OK && ttyfd >= 0) { + return ttyfd; } - /* Named aliases FIRST -- must precede the generic CTRL_* range */ - switch (tb_key) { - case TB_KEY_TAB: return BOXEN_KEY_TAB; - case TB_KEY_ENTER: return BOXEN_KEY_ENTER; - case TB_KEY_ESC: return BOXEN_KEY_ESCAPE; - case TB_KEY_BACKSPACE: return BOXEN_KEY_BACKSPACE; - case TB_KEY_BACKSPACE2:return BOXEN_KEY_BACKSPACE; - case TB_KEY_SPACE: return BOXEN_KEY_NONE; /* space is printable via ch */ - - /* Function keys */ - case TB_KEY_F1: return BOXEN_KEY_F1; - case TB_KEY_F2: return BOXEN_KEY_F2; - case TB_KEY_F3: return BOXEN_KEY_F3; - case TB_KEY_F4: return BOXEN_KEY_F4; - case TB_KEY_F5: return BOXEN_KEY_F5; - case TB_KEY_F6: return BOXEN_KEY_F6; - case TB_KEY_F7: return BOXEN_KEY_F7; - case TB_KEY_F8: return BOXEN_KEY_F8; - case TB_KEY_F9: return BOXEN_KEY_F9; - case TB_KEY_F10: return BOXEN_KEY_F10; - case TB_KEY_F11: return BOXEN_KEY_F11; - case TB_KEY_F12: return BOXEN_KEY_F12; - - /* Navigation */ - case TB_KEY_ARROW_UP: return BOXEN_KEY_UP; - case TB_KEY_ARROW_DOWN: return BOXEN_KEY_DOWN; - case TB_KEY_ARROW_LEFT: return BOXEN_KEY_LEFT; - case TB_KEY_ARROW_RIGHT: return BOXEN_KEY_RIGHT; - case TB_KEY_HOME: return BOXEN_KEY_HOME; - case TB_KEY_END: return BOXEN_KEY_END; - case TB_KEY_PGUP: return BOXEN_KEY_PGUP; - case TB_KEY_PGDN: return BOXEN_KEY_PGDN; - case TB_KEY_INSERT: return BOXEN_KEY_INSERT; - case TB_KEY_DELETE: return BOXEN_KEY_DELETE; - - default: - break; + /* Fallback: open /dev/tty directly. O_NONBLOCK is NOT set because + * the decoder uses select(2) + read(2) with explicit timeouts; a + * non-blocking fd would cause spurious EAGAIN returns inside the + * decoder's blocking-poll path. */ + int fd = open("/dev/tty", O_RDONLY | O_CLOEXEC); + if (fd < 0) { + return -1; } - - /* Generic CTRL_* range (1-26) -- AFTER the aliases above */ - if (tb_key >= 0x01 && tb_key <= 0x1a) { - /* Map 0x01 -> CTRL_A (index 0), 0x1a -> CTRL_Z (index 25) */ - return (boxen_key_t)(BOXEN_KEY_CTRL_A + (tb_key - 0x01)); - } - if (tb_key == TB_KEY_CTRL_BACKSLASH) { - return BOXEN_KEY_CTRL_BACKSLASH; - } - if (tb_key == 0x1b) { - /* ESC / CTRL_LSQ_BRACKET -- handled above as BOXEN_KEY_ESCAPE */ - return BOXEN_KEY_ESCAPE; - } - if (tb_key == TB_KEY_CTRL_TILDE) { - return BOXEN_KEY_CTRL_SPACE; - } - - return BOXEN_KEY_NONE; + *fd_was_opened_out = true; + return fd; } +/* Track whether we own (and must close) the TTY fd. False when borrowed + * from termbox2 (tb_shutdown closes it); true when we opened /dev/tty. */ +static bool g_tb2_owns_tty_fd = false; +static int g_tb2_tty_fd = -1; + /* ------------------------------------------------------------------------- * Attribute translation: BOXEN_ATTR_* -> termbox2 uintattr_t bits + * + * Rendering-side translator -- NOT subsumed by the input decoder. Kept + * intact from pre-M6 backend_tb2.c. * ---------------------------------------------------------------------- */ static uintattr_t translate_attr(uint16_t boxen_attr) { @@ -174,6 +117,9 @@ static uintattr_t translate_attr(uint16_t boxen_attr) { * have the same numeric values as TB_DEFAULT and TB_BLACK..TB_WHITE. This * function exists to make that coincidence explicit and local: if termbox2 * ever renumbers its color constants, only this function needs to change. + * + * Rendering-side translator -- NOT subsumed by the input decoder. Kept + * intact from pre-M6 backend_tb2.c. * ---------------------------------------------------------------------- */ static uintattr_t translate_color(boxen_color_t c) { @@ -181,76 +127,44 @@ static uintattr_t translate_color(boxen_color_t c) { } /* ------------------------------------------------------------------------- - * Modifier translation: TB_MOD_* -> BOXEN_MOD_* + * Backend vtable implementation * ---------------------------------------------------------------------- */ -static uint16_t translate_mod(uint8_t tb_mod) { - uint16_t m = 0; - if (tb_mod & TB_MOD_ALT) m |= BOXEN_MOD_ALT; - if (tb_mod & TB_MOD_CTRL) m |= BOXEN_MOD_CTRL; - if (tb_mod & TB_MOD_SHIFT) m |= BOXEN_MOD_SHIFT; - return m; -} - -/* ------------------------------------------------------------------------- - * Mouse event translation - * - * termbox2 encodes mouse events as TB_EVENT_MOUSE with a key field set to - * one of the TB_KEY_MOUSE_* constants. Release has no button identity. - * ---------------------------------------------------------------------- */ +static int tb2_init(void *config) { + (void)config; -static void translate_mouse(const struct tb_event *te, boxen_event_t *out) { - out->type = BOXEN_EV_MOUSE; - out->mouse.x = te->x; - out->mouse.y = te->y; - out->mouse.mod = translate_mod(te->mod); - out->mouse.flags = 0; - - switch (te->key) { - case TB_KEY_MOUSE_LEFT: - out->mouse.button = 1; - out->mouse.pressed = true; - break; - case TB_KEY_MOUSE_RIGHT: - out->mouse.button = 3; - out->mouse.pressed = true; - break; - case TB_KEY_MOUSE_MIDDLE: - out->mouse.button = 2; - out->mouse.pressed = true; - break; - case TB_KEY_MOUSE_RELEASE: - out->mouse.button = 0; - out->mouse.pressed = false; - break; - case TB_KEY_MOUSE_WHEEL_UP: - out->mouse.button = 4; - out->mouse.pressed = true; - break; - case TB_KEY_MOUSE_WHEEL_DOWN: - out->mouse.button = 5; - out->mouse.pressed = true; - break; - default: - out->mouse.button = 0; - out->mouse.pressed = false; - break; + int r = tb_init(); + if (r != TB_OK) { + return BOXEN_ERR_INIT; } - if (out->mouse.pressed) { - tb2_maybe_set_double_click(out); + /* 2026-06-29 JES #805 M6: mouse mode is NOT enabled here. Per plan + * section 5.1, the decoder keeps mouse off by default so native + * terminal selection works on REPL startup. The M7 /mouse slash + * command and palette-open auto-enable will flip it on demand via + * input_decoder_set_mouse(g_decoder, true). Do NOT add a + * tb_set_input_mode(TB_INPUT_MOUSE) call here -- that was the PR + * #808 regression that broke selection for everyone. */ + + bool opened_tty = false; + int tty_fd = tb2_obtain_tty_fd(&opened_tty); + if (tty_fd < 0) { + tb_shutdown(); + return BOXEN_ERR_INIT; } -} -/* ------------------------------------------------------------------------- - * Backend vtable implementation - * ---------------------------------------------------------------------- */ + g_decoder = input_decoder_create(tty_fd); + if (g_decoder == NULL) { + if (opened_tty) { + close(tty_fd); + } + tb_shutdown(); + return BOXEN_ERR_INIT; + } + g_tb2_tty_fd = tty_fd; + g_tb2_owns_tty_fd = opened_tty; -static int tb2_init(void *config) { - (void)config; - memset(&g_tb2_last_press, 0, sizeof(g_tb2_last_press)); - int r = tb_init(); - return (r == TB_OK) ? BOXEN_OK : BOXEN_ERR_INIT; + return BOXEN_OK; } /* Last cursor position passed to tb_set_cursor(). Used by @@ -261,9 +175,20 @@ static int g_tb2_cursor_x = 0; static int g_tb2_cursor_y = 0; static void tb2_shutdown(void) { - /* Symmetric with init: clear double-click state so a re-init starts fresh - * even if init's memset is ever removed or the cycle is reused. */ - memset(&g_tb2_last_press, 0, sizeof(g_tb2_last_press)); + /* 2026-06-29 JES #805 M6: destroy decoder BEFORE tb_shutdown. Any + * decoder-side writes (e.g., a mouse-disable sequence the decoder + * may emit on destroy in a future milestone) must land while the + * TTY is still in raw mode and the fd is still valid. Order + * matters; do not reorder these two calls. */ + input_decoder_destroy(g_decoder); + g_decoder = NULL; + + if (g_tb2_owns_tty_fd && g_tb2_tty_fd >= 0) { + close(g_tb2_tty_fd); + } + g_tb2_tty_fd = -1; + g_tb2_owns_tty_fd = false; + g_tb2_cursor_x = 0; g_tb2_cursor_y = 0; tb_shutdown(); @@ -313,47 +238,38 @@ static void tb2_present(void) { tb_present(); } +/* ------------------------------------------------------------------------- + * 2026-06-29 JES #805 M6: poll event via the input decoder. + * + * The decoder emits boxen_event_t directly -- no translation layer needed. + * timeout_ms flows straight through to input_decoder_poll, which uses + * select(2) for the blocking wait and read(2) to drain the TTY pipe. + * + * Threading: caller (boxen_poll_event -> usertalk dispatch) is GIL-held + * on entry. The decoder may block inside select(2) for timeout_ms; that + * block happens with the GIL still held -- the GIL hand-off for cross- + * thread progress during the wait is the REPL event loop's responsibility, + * not the backend's. M6 preserves the prior behavior in this respect: + * tb_peek_event was also called with the GIL held. + * + * Risk R3 (mouse-enable timing window) is mitigated inside the decoder + * (mouse writes are synchronous + the decoder accepts both SGR and X10 + * formats during the handshake window). Risk R4 (silent breakage) is + * mitigated by the PTY-replay test suite (138 tests at M5; M6 adds none + * because inject_bytes still bypasses tty_fd in the test seam). + * ---------------------------------------------------------------------- */ static int tb2_poll_event(boxen_event_t *out, int timeout_ms) { - struct tb_event te; - int r; - - memset(out, 0, sizeof(*out)); - - if (timeout_ms == 0) { - /* Non-blocking peek */ - r = tb_peek_event(&te, 0); - } else { - r = tb_peek_event(&te, timeout_ms); + if (out == NULL) { + return BOXEN_ERR_INVALID; } - - if (r == TB_ERR_NO_EVENT || r == TB_ERR_POLL) { + if (g_decoder == NULL) { + /* Defensive: tb2_init was not called or failed. Return TIMEOUT + * rather than IO so an over-eager poll during teardown is not + * mistaken for a hard error. */ + memset(out, 0, sizeof(*out)); return BOXEN_ERR_TIMEOUT; } - if (r != TB_OK) { - return BOXEN_ERR_IO; - } - - switch (te.type) { - case TB_EVENT_KEY: - out->type = BOXEN_EV_KEY; - out->key.key = translate_key(te.key, te.ch); - out->key.ch = te.ch; - out->key.mod = translate_mod(te.mod); - break; - case TB_EVENT_MOUSE: - translate_mouse(&te, out); - break; - case TB_EVENT_RESIZE: - out->type = BOXEN_EV_RESIZE; - out->resize.w = te.w; - out->resize.h = te.h; - break; - default: - out->type = BOXEN_EV_NONE; - break; - } - - return BOXEN_OK; + return input_decoder_poll(g_decoder, out, timeout_ms); } static void tb2_clear(void) { diff --git a/frontier-cli/boxen/input_decoder.c b/frontier-cli/boxen/input_decoder.c index dce7f5284..60e0e1b08 100644 --- a/frontier-cli/boxen/input_decoder.c +++ b/frontier-cli/boxen/input_decoder.c @@ -57,11 +57,13 @@ #include "boxen_internal.h" /* BOXEN_DOUBLE_CLICK_MS, BOXEN_DOUBLE_CLICK_RADIUS */ #include +#include /* 2026-06-29 JES #805 M6: EINTR / EAGAIN handling on read(2) */ #include #include #include +#include /* 2026-06-29 JES #805 M6: select(2) for blocking poll wait */ #include /* clock_gettime for double-click synthesis */ -#include /* write(2) for mouse-enable sequences (M3) */ +#include /* write(2) for mouse-enable sequences (M3); read(2) for M6 */ /* 2026-06-29 JES #809 M1: ring-buffer sizing. * @@ -1355,27 +1357,174 @@ static bool utf8_codepoint_valid(uint32_t cp, int lead_len) { } /* ------------------------------------------------------------------------- - * 2026-06-30 JES #810 M2: poll -- run the state machine against buffered - * bytes until one event emits or we run out of bytes. + * 2026-06-29 JES #805 M6: TTY fill helpers. + * + * The M2 poll body consumed only bytes already in the ring buffer (deposited + * by inject_bytes in the test seam). M6 wires the real read(2) path: when + * tty_fd >= 0 and the parser exhausts the buffer without emitting an event, + * we block (via select(2)) up to timeout_ms for more bytes, drain the pipe + * with one or more non-blocking reads, then re-run the parser. + * + * Plan reference: section 4.4 (ESC disambiguation strategy). The contract: + * - Initial wait uses select(2) with timeout_ms. + * - When the first read arrives, do a non-blocking follow-up read to drain + * the pipe. This collapses a multi-byte escape sequence that arrived + * as one OS burst into a single parse cycle, and gives sub-millisecond + * ESC-alone disambiguation (no extra delay when ESC really is alone). + * - Bytes that don't fit are counted in dropped_bytes (same back-pressure + * contract as inject_bytes overflow). + * + * Threading: the parent (tb2_poll_event) holds the GIL on entry; it must + * drop the GIL around this call before invoking input_decoder_poll if the + * REPL needs other threads to make progress during the blocking wait. + * That GIL hand-off is the tb2 backend's responsibility, not the decoder's. + * The single-owner invariant from input_decoder.h means no other thread + * touches *dec during the GIL-dropped window. + * ---------------------------------------------------------------------- */ + +/* Append bytes from the TTY into the decoder ring buffer. Returns the + * number of bytes appended. On short fills (ring near capacity) the + * excess is counted in dropped_bytes. Called only when tty_fd >= 0. */ +static size_t dec_append_from_fd(input_decoder_t *dec, const uint8_t *src, size_t len) { + compact_buffer(dec); + size_t avail = INPUT_DECODER_BUFFER_SIZE - dec->fill_len; + size_t to_copy = len < avail ? len : avail; + if (to_copy > 0) { + memcpy(dec->buf + dec->fill_len, src, to_copy); + dec->fill_len += to_copy; + } + size_t dropped = len - to_copy; + if (dropped > 0) { + dec->dropped_bytes += dropped; + } + return to_copy; +} + +/* Read whatever bytes are immediately available on tty_fd without blocking. + * Uses a select(2) with zero timeout to probe readability, then read(2) once + * to drain. Returns the count of bytes deposited into the ring buffer (may + * be 0 if nothing is pending or the read got EAGAIN). Returns SIZE_MAX on + * a hard read error (closed fd or unexpected errno) -- caller surfaces as + * BOXEN_ERR_IO. EINTR is treated as "no data this time" (caller may retry). + * + * Bytes that don't fit in the ring are counted in dropped_bytes via + * dec_append_from_fd. We cap the read size at the ring's remaining space + * so we never read more bytes than we can store -- the kernel will buffer + * the rest for the next call. Slightly suboptimal in pathological bursts + * (a paste of 8 KB arrives in two select rounds instead of one) but + * preserves the back-pressure invariant and avoids stack staging. */ +static size_t dec_read_nonblocking(input_decoder_t *dec) { + compact_buffer(dec); + size_t avail = INPUT_DECODER_BUFFER_SIZE - dec->fill_len; + if (avail == 0) { + /* Buffer full; do not read -- caller must drain via the parser + * before more bytes can arrive. Returning 0 lets the parser + * consume what's already buffered on the next iteration. */ + return 0; + } + + fd_set rfds; + FD_ZERO(&rfds); + FD_SET(dec->tty_fd, &rfds); + struct timeval tv = {0, 0}; /* non-blocking probe */ + int r = select(dec->tty_fd + 1, &rfds, NULL, NULL, &tv); + if (r < 0) { + if (errno == EINTR) { + return 0; + } + return SIZE_MAX; + } + if (r == 0) { + return 0; + } + + uint8_t stage[INPUT_DECODER_BUFFER_SIZE]; + size_t to_read = avail < sizeof(stage) ? avail : sizeof(stage); + ssize_t got = read(dec->tty_fd, stage, to_read); + if (got < 0) { + if (errno == EINTR || errno == EAGAIN) { + return 0; + } + return SIZE_MAX; + } + if (got == 0) { + /* EOF on the TTY -- closed or detached. Treat as IO error so + * the caller can surface and shut down rather than spin. */ + return SIZE_MAX; + } + (void)dec_append_from_fd(dec, stage, (size_t)got); + return (size_t)got; +} + +/* Block until at least one byte is readable on tty_fd or timeout_ms elapses. + * Returns 1 if readable, 0 on timeout, -1 on hard error. EINTR is treated + * as "retry" -- the loop re-enters select with the remaining time deducted. + * timeout_ms of -1 blocks indefinitely; 0 returns immediately. */ +static int dec_select_readable(int fd, int timeout_ms) { + struct timespec start; + if (timeout_ms > 0) { + clock_gettime(CLOCK_MONOTONIC, &start); + } + + for (;;) { + fd_set rfds; + FD_ZERO(&rfds); + FD_SET(fd, &rfds); + + struct timeval tv; + struct timeval *ptv; + int remaining_ms = timeout_ms; + if (timeout_ms < 0) { + ptv = NULL; + } else { + tv.tv_sec = remaining_ms / 1000; + tv.tv_usec = (remaining_ms % 1000) * 1000; + ptv = &tv; + } + + int r = select(fd + 1, &rfds, NULL, NULL, ptv); + if (r > 0) { + return 1; + } + if (r == 0) { + return 0; + } + /* r < 0 */ + if (errno != EINTR) { + return -1; + } + /* EINTR: recompute remaining timeout and retry. */ + if (timeout_ms > 0) { + struct timespec now; + clock_gettime(CLOCK_MONOTONIC, &now); + long elapsed_ms = (long)(now.tv_sec - start.tv_sec) * 1000 + + (long)(now.tv_nsec - start.tv_nsec) / 1000000; + if (elapsed_ms >= timeout_ms) { + return 0; + } + timeout_ms -= (int)elapsed_ms; + clock_gettime(CLOCK_MONOTONIC, &start); + } + /* timeout_ms == 0 was handled by initial select returning 0; + * timeout_ms < 0 just loops back into another blocking wait. */ + } +} + +/* ------------------------------------------------------------------------- + * 2026-06-30 JES #810 M2: drain helper -- run the state machine against + * buffered bytes until one event emits or we run out of bytes. + * 2026-06-29 JES #805 M6: split from input_decoder_poll so the orchestrator + * can call us, then fill from TTY, then call us again. * * Returns BOXEN_OK + writes *out on a successful emit. * Returns BOXEN_ERR_TIMEOUT (with *out zeroed) when no event can be * produced from the currently-buffered bytes (empty buffer OR partial * sequence still waiting for more bytes). - * Returns BOXEN_ERR_INVALID for NULL args. * - * timeout_ms is currently unused (M6 will use it for the select(2) wait - * on tty_fd). In the test seam the caller drives bytes via inject_bytes - * before each poll, so a timeout has no observable effect. + * Pre-condition: dec and out are non-NULL. Caller is input_decoder_poll + * (which validates) -- not called directly from outside this file. * ---------------------------------------------------------------------- */ -int input_decoder_poll(input_decoder_t *dec, boxen_event_t *out, int timeout_ms) { - (void)timeout_ms; /* M6 wires the select(2) timeout. */ - - if (dec == NULL || out == NULL) { - return BOXEN_ERR_INVALID; - } - memset(out, 0, sizeof(*out)); - +static int dec_drain_buffer(input_decoder_t *dec, boxen_event_t *out) { while (dec->head < dec->fill_len) { /* 2026-06-29 JES #812 M4: PASTE_ACTIVE branch. * @@ -1826,6 +1975,105 @@ int input_decoder_poll(input_decoder_t *dec, boxen_event_t *out, int timeout_ms) return BOXEN_ERR_TIMEOUT; } +/* ------------------------------------------------------------------------- + * 2026-06-29 JES #805 M6: public poll orchestrator. + * + * Contract (extends the M2 contract): + * - Always try to emit from the existing buffer first (handles a burst + * where the prior poll left additional complete events behind). + * - If the buffer is exhausted (or only a partial sequence remains) AND + * tty_fd >= 0, block on select(2) for up to timeout_ms. When data + * arrives, drain the pipe with non-blocking reads (collapse a multi- + * byte escape sequence that arrived in one OS burst into one parse + * cycle). Then re-run the parser. + * - If tty_fd == -1 (test seam): no read attempted; behavior matches the + * M2 contract -- caller drives bytes via input_decoder_inject_bytes + * before each poll. + * - timeout_ms semantics: 0 = non-blocking probe; positive = block up to + * that many milliseconds; negative = block indefinitely. The tb2 + * backend passes timeout_ms straight through from boxen_poll_event. + * + * Returns BOXEN_OK + writes *out on a successful emit. + * Returns BOXEN_ERR_TIMEOUT (with *out zeroed) when no event materialized + * within the timeout (empty buffer, no TTY data, or partial sequence + * awaiting more bytes). + * Returns BOXEN_ERR_IO on a hard read(2) error or EOF on tty_fd. + * Returns BOXEN_ERR_INVALID for NULL args. + * + * Plan reference: section 2.3 (integration), section 4.4 (ESC disambig), + * section 3.12 (burst-read robustness). + * ---------------------------------------------------------------------- */ +int input_decoder_poll(input_decoder_t *dec, boxen_event_t *out, int timeout_ms) { + if (dec == NULL || out == NULL) { + return BOXEN_ERR_INVALID; + } + memset(out, 0, sizeof(*out)); + + /* First: drain anything already buffered. This handles bursts where + * the prior poll consumed only one of several queued events, and the + * test-seam path where inject_bytes deposited bytes before the call. */ + int r = dec_drain_buffer(dec, out); + if (r == BOXEN_OK) { + return BOXEN_OK; + } + + /* Test seam: no TTY fd, so nothing more to do -- caller will inject + * before the next poll. Preserves M1..M5 test contract. */ + if (dec->tty_fd < 0) { + return BOXEN_ERR_TIMEOUT; + } + + /* Block (up to timeout_ms) for the first byte to arrive on the TTY. + * Special case: timeout_ms == 0 is a non-blocking probe; skip the + * blocking select and go straight to the non-blocking drain so we + * still pick up bytes already queued at the OS level. */ + if (timeout_ms != 0) { + int sr = dec_select_readable(dec->tty_fd, timeout_ms); + if (sr < 0) { + return BOXEN_ERR_IO; + } + if (sr == 0) { + memset(out, 0, sizeof(*out)); + return BOXEN_ERR_TIMEOUT; + } + } + + /* Drain the pipe non-blocking: collapses a multi-byte escape sequence + * that arrived as one OS burst into one parse cycle, and gives sub- + * millisecond ESC-alone disambiguation (a single ESC byte returns no + * follow-up bytes from the second read, so the parser's stored + * ESC_RECEIVED state will be flushed as KEY_ESCAPE on the next poll + * via the existing flush-on-no-more-bytes path in dec_drain_buffer). + * + * Loop the non-blocking reads up to a small bound to soak up any + * pipe bytes that arrived between our select(2) and our first read. + * Bound prevents starvation -- after a few rounds we surrender and + * let the parser run so partial sequences in the buffer get a chance + * to complete on the next poll. */ + for (int i = 0; i < 4; i++) { + size_t got = dec_read_nonblocking(dec); + if (got == SIZE_MAX) { + return BOXEN_ERR_IO; + } + if (got == 0) { + break; + } + } + + /* Re-run the parser against the freshly populated buffer. */ + r = dec_drain_buffer(dec, out); + if (r == BOXEN_OK) { + return BOXEN_OK; + } + + /* Nothing parsed yet (partial sequence in flight, or only a lone ESC + * sitting in ESC_RECEIVED waiting for disambiguation). Caller will + * poll again; ESC_RECEIVED will flush on the next call if no more + * bytes arrive within the next timeout window. */ + memset(out, 0, sizeof(*out)); + return BOXEN_ERR_TIMEOUT; +} + /* ------------------------------------------------------------------------- * Test seam (compile-guarded) * ---------------------------------------------------------------------- */ From 48112cb9e5182a8c555f9d40fb76f380b6362eab Mon Sep 17 00:00:00 2001 From: Jake Savin Date: Mon, 29 Jun 2026 22:05:57 -0700 Subject: [PATCH 2/2] docs(boxen): C M6 #805 -- correct GIL contract comments per gate review Gate review (PR #821 P1, concurrency reviewer): the M6 cutover comments in backend_tb2.c and input_decoder.c mis-stated the GIL contract, saying the decoder runs with the GIL held during select(2). Actual behavior (boxen_repl.c:2147-2152, mirroring debugger_tui_main per ADR-014): the REPL drops the GIL via pthread_mutex_unlock BEFORE calling boxen_poll_event, so the decoder's blocking select(2) runs with the GIL NOT held. The single-owner invariant from input_decoder.h still holds (the REPL thread is the only caller). Comment-only fix. No behavioral change. PTY-replay 138/138 still green; unit suite still 964/964 green. --- frontier-cli/boxen/backend_tb2.c | 19 +++++++++++++------ frontier-cli/boxen/input_decoder.c | 15 +++++++++------ 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/frontier-cli/boxen/backend_tb2.c b/frontier-cli/boxen/backend_tb2.c index 6356facc4..da451d264 100644 --- a/frontier-cli/boxen/backend_tb2.c +++ b/frontier-cli/boxen/backend_tb2.c @@ -245,12 +245,19 @@ static void tb2_present(void) { * timeout_ms flows straight through to input_decoder_poll, which uses * select(2) for the blocking wait and read(2) to drain the TTY pipe. * - * Threading: caller (boxen_poll_event -> usertalk dispatch) is GIL-held - * on entry. The decoder may block inside select(2) for timeout_ms; that - * block happens with the GIL still held -- the GIL hand-off for cross- - * thread progress during the wait is the REPL event loop's responsibility, - * not the backend's. M6 preserves the prior behavior in this respect: - * tb_peek_event was also called with the GIL held. + * Threading: the caller (`boxen_repl_event_loop` at boxen_repl.c:2147-2152, + * mirroring `debugger_tui_main` per ADR-014) DROPS the Frontier GIL via + * `pthread_mutex_unlock(&frontier_gil)` BEFORE calling boxen_poll_event, + * and reacquires it after. So this function -- and `input_decoder_poll` + * underneath -- runs with the GIL NOT held during the blocking wait. + * That's by design: it lets other Frontier threads make progress while + * the REPL is parked in select(2). M6 preserves the prior behavior in + * this respect (tb_peek_event also ran with the GIL released). + * + * The single-owner invariant from input_decoder.h still holds because + * the REPL thread is the only one that touches *g_decoder; the other + * GIL-acquiring threads run UserTalk code that never reaches the + * decoder. No locks or atomics needed inside the decoder. * * Risk R3 (mouse-enable timing window) is mitigated inside the decoder * (mouse writes are synchronous + the decoder accepts both SGR and X10 diff --git a/frontier-cli/boxen/input_decoder.c b/frontier-cli/boxen/input_decoder.c index 60e0e1b08..b5da0b577 100644 --- a/frontier-cli/boxen/input_decoder.c +++ b/frontier-cli/boxen/input_decoder.c @@ -1374,12 +1374,15 @@ static bool utf8_codepoint_valid(uint32_t cp, int lead_len) { * - Bytes that don't fit are counted in dropped_bytes (same back-pressure * contract as inject_bytes overflow). * - * Threading: the parent (tb2_poll_event) holds the GIL on entry; it must - * drop the GIL around this call before invoking input_decoder_poll if the - * REPL needs other threads to make progress during the blocking wait. - * That GIL hand-off is the tb2 backend's responsibility, not the decoder's. - * The single-owner invariant from input_decoder.h means no other thread - * touches *dec during the GIL-dropped window. + * Threading: input_decoder_poll runs with the Frontier GIL NOT held. + * The REPL event loop (boxen_repl.c:2147-2152) releases the GIL via + * `pthread_mutex_unlock(&frontier_gil)` BEFORE calling boxen_poll_event + * (which forwards through tb2_poll_event into this function), and + * reacquires it after. That's by design -- it lets other Frontier + * threads run while the REPL is parked in select(2). The single-owner + * invariant from input_decoder.h still holds because the REPL thread + * is the only caller; other GIL-acquiring threads run UserTalk code + * that never touches *dec. No locks or atomics needed. * ---------------------------------------------------------------------- */ /* Append bytes from the TTY into the decoder ring buffer. Returns the