Repository navigation
Land raw-fd keystroke reading on main (follow-up to #15) - #18
Merged
Merged
Conversation
The menu read single keys through buffered sys.stdin.read(1) while _read_escape_sequence drains bytes straight from the descriptor. When two keystrokes arrive in one packet (fast arrow taps), the buffered layer pulls the whole packet into its internal buffer, the fd-level reader finds nothing ready, and both taps are silently dropped; the leftover bytes then masquerade as ordinary keys and can swallow Enter, leaving the menu stuck. Read every key with os.read(fd, 1) so both consumers share one byte stream, and exit cleanly on EOF or Ctrl-D instead of spinning the redraw loop forever on a closed stdin.
Diffly verdict: QUARANTINE#18 · 2 files · 60 lines changed · checks: PENDING Risk flags: Risk flags and reasoning
Blast-radius summary
Deterministic triage is authoritative; any optional LLM explanation cannot change the verdict. |
Three defects surfaced once the pty tests actually exercised the menu loop (their stdout-isatty guard previously short-circuited to the fallback report): - tty.setcbreak defaults to TCSAFLUSH on newer Pythons and silently discarded keys typed while the menu rendered; pin TCSANOW. - _read_escape_sequence drained up to 16 bytes per call, swallowing neighboring keystrokes in a burst; read one byte at a time and stop at the CSI final byte so trailing keys stay queued. - Restoring termios with TCSADRAIN can block forever when nothing reads our echoed output (CI ptys); restore with TCSANOW instead. Tests now patch both isatty guards, drive real cursor movement over a pty, and assert exact sequence consumption.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Follow-up to #15. That PR was stacked on #14, so when it was merged, GitHub landed its commits into the stacked base branch (
fix/interactive-arrow-nameerror) instead ofmain—mainstill reads menu keys through bufferedsys.stdin.read(1)(cli.py:423), so the bug this fix addresses is live onmaintoday:The menu read single keys through the buffered text layer while
_read_escape_sequence()drains bytes straight from the file descriptor. When two keystrokes arrive in one packet — exactly what fast arrow tapping produces — the buffered layer pulls the entire packet into its internal buffer,select()on the descriptor then reports nothing ready, and both taps are dropped; leftover bytes ([,B) are later misread as ordinary keys and can swallow Enter, leaving the menu stuck. A closed stdin also spins the clear-and-redraw loop forever.What users will see / Surface area
src/diffly_cli/cli.py::interactive_view: every keystroke is read withos.read(fd, 1)so both consumers share a single raw byte stream.q/Qquit, Space toggles, arrows move, Enter renders.Validation
main.test_interactive_menu_keeps_up_with_rapid_arrow_taps(↓↓↑ delivered in one packet + Enter) andtest_menu_exits_when_stdin_reaches_eof; both hang/fail without the fix.pytest -q→ 58 passed.Follow-up hardening (second commit)
Once the pty tests were made to exercise the real loop (see below), three more defects surfaced:
tty.setcbreakdiscards typed keys on newer Pythons — it defaults toTCSAFLUSH, flushing everything queued while the menu rendered. Now pinned toTCSANOW.test_escape_sequence_reader_consumes_exactly_one_sequenceproves[A\x1b[Bconsumes exactly one sequence and leaves the next Escape untouched.TCSADRAINcan wait forever when nothing drains echoed output (CI harnesses); restore now usesTCSANOW.The pty regression tests previously passed vacuously: pytest's captured stdout failed the
isattyguard, so they fell back to the plain report without ever running the menu. They now patch both guards and drive real cursor movement (visible in-soutput), and the suite runs them for real.