fix(input): handle ESC during CSI and SS3 parse states per ECMA-48#1053
Conversation
Properly check for ESC (`0x1B`) to prevent erroneously falling into the 'bad parse' branch of `intCsi` and `intSs3`.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe parser now treats an incoming ESC (0x1b) byte received while accumulating CSI or SS3 parameters as a reset: it transitions to the escape state ( Changes
Sequence Diagram(s)(Skipped — change is localized to parser state transitions without multi-component sequential flow requiring visualization.) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The drain loop in the ESC-during-CSI/SS3 tests was keeping the last EventKey, which could be overwritten by the escTimeout timer firing during the 100ms drain window. Capture the first EventKey instead.
|
I believe that this fix is good from a correctness... although unlikely to actually have any real world impact. I've never seen an aborted escape sequence in the wild, and I can think of no reason why a sender would ever start a sequence then abort it. (I also suspect that very few implementations actually would handle this case.) That said, the change is small and low risk, and I'm happy to include it. Thank you. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1053 +/- ##
==========================================
- Coverage 86.18% 86.16% -0.03%
==========================================
Files 46 46
Lines 5060 5066 +6
==========================================
+ Hits 4361 4365 +4
- Misses 541 542 +1
- Partials 158 159 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Disclosure: I stumbled across this while working on a personal project. AI tools were used to diagnose and resolve the issue as well as generate this potential upstream fix. Systems programming at this level is not my expertise, but the fix seemed small, contained, and testable so I thought it worthwhile to open a PR. AFAIK, the same issue is present in tcell v2.
The input parser's CSI and SS3 state handlers do not check for ESC (0x1B). Per ECMA-48 §5.3.1, ESC is "always effective" and should restart the escape sequence machine from any intermediate state. Instead, ESC falls into the "bad parse" branch in istCsi and is silently discarded, and in istSs3 it fails the key lookup and is similarly lost.
This causes the byte(s) following the discarded ESC to be misinterpreted. In practice, the user's first keystroke after Suspend/Resume can be swallowed: engage() sends escape sequences that may provoke terminal responses, and if those responses leave the parser mid-CSI, the next ESC-prefixed key (arrow keys, function keys) is consumed as the tail of the stale sequence.
The tty input flush added in #1048 reduces the likelihood of stale bytes reaching the parser, but does not eliminate it — terminal responses can arrive after the flush and before inputLoop starts, or bytes can be split across reads leaving a partial sequence in the parser's state machine.
This change adds an ESC check to both istCsi and istSs3 that transitions to istEsc and clears escChar, matching the behavior of istInit. Tests verify that a partial CSI or SS3 followed by a new escape sequence (e.g., arrow key) produces the correct key event.
Related: #194, #480, #1048
Summary by CodeRabbit
Bug Fixes
Tests