fix: drain escape sequences in cbreak listeners & disable mouse tracking leak#245
fix: drain escape sequences in cbreak listeners & disable mouse tracking leak#245lijunzh wants to merge 4 commits intompfaffenberger:mainfrom
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
d10c7fb to
b5b0726
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code_puppy/command_line/mcp/custom_server_form.py (1)
637-647:⚠️ Potential issue | 🟠 MajorGuard mouse-tracking cleanup so terminal restoration always runs.
If the import/call at Line [641]-Line [643] throws, Line [645]-Line [647] won’t run, which can leave the terminal state broken (alt screen/input flag not restored).
🔧 Proposed fix
finally: # Explicitly disable mouse tracking as a safety net — if any # previous prompt_toolkit Application left tracking enabled # (e.g., due to an exception or threading race), this ensures # the terminal stops sending mouse escape sequences. - from code_puppy.terminal_utils import disable_mouse_tracking - - disable_mouse_tracking() - # Exit alternate screen buffer - sys.stdout.write("\033[?1049l") - sys.stdout.flush() - set_awaiting_user_input(False) + try: + from code_puppy.terminal_utils import disable_mouse_tracking + disable_mouse_tracking() + except Exception: + pass + finally: + # Exit alternate screen buffer + sys.stdout.write("\033[?1049l") + sys.stdout.flush() + set_awaiting_user_input(False)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code_puppy/command_line/mcp/custom_server_form.py` around lines 637 - 647, The cleanup sequence can be interrupted if importing or calling disable_mouse_tracking raises, so wrap the import and call to disable_mouse_tracking in a try/except (or try/finally) and ensure the terminal restoration and flag reset always run in a finally block; specifically, protect the disable_mouse_tracking import/call and always execute sys.stdout.write("\033[?1049l"), sys.stdout.flush(), and set_awaiting_user_input(False) in the finally section so alternate screen buffer and awaiting-user flag are restored even on errors.
🧹 Nitpick comments (1)
code_puppy/terminal_utils.py (1)
154-179: Use this helper from listener call sites to remove duplicated drain loops.
drain_stdin_escape_sequence()now centralizes exactly the behavior duplicated incode_puppy/tools/command_runner.py(Line [399]-Line [401], Line [413]-Line [414]) andcode_puppy/agents/base_agent.py(Line [1779]-Line [1780], Line [1800]-Line [1801]). Wiring those call sites to this helper will reduce drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code_puppy/terminal_utils.py` around lines 154 - 179, Replace the duplicated stdin-draining loops in code_puppy/tools/command_runner.py and code_puppy/agents/base_agent.py with a single call to the new helper drain_stdin_escape_sequence() from code_puppy/terminal_utils; import the function where needed, remove the local select/read loop at the duplicate sites, and ensure the call is used in the same locations that previously invoked the manual draining logic so behavior (including the Windows early-return and exception-silencing) is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@code_puppy/command_line/mcp/custom_server_form.py`:
- Around line 637-647: The cleanup sequence can be interrupted if importing or
calling disable_mouse_tracking raises, so wrap the import and call to
disable_mouse_tracking in a try/except (or try/finally) and ensure the terminal
restoration and flag reset always run in a finally block; specifically, protect
the disable_mouse_tracking import/call and always execute
sys.stdout.write("\033[?1049l"), sys.stdout.flush(), and
set_awaiting_user_input(False) in the finally section so alternate screen buffer
and awaiting-user flag are restored even on errors.
---
Nitpick comments:
In `@code_puppy/terminal_utils.py`:
- Around line 154-179: Replace the duplicated stdin-draining loops in
code_puppy/tools/command_runner.py and code_puppy/agents/base_agent.py with a
single call to the new helper drain_stdin_escape_sequence() from
code_puppy/terminal_utils; import the function where needed, remove the local
select/read loop at the duplicate sites, and ensure the call is used in the same
locations that previously invoked the manual draining logic so behavior
(including the Windows early-return and exception-silencing) is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9f3eaaca-5d55-43a5-aca9-d5265a436fbf
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
code_puppy/agents/base_agent.pycode_puppy/command_line/mcp/custom_server_form.pycode_puppy/terminal_utils.pycode_puppy/tools/command_runner.py
…ing leak Fixes mpfaffenberger#244 — After prolonged use, mouse actions in the terminal send garbled escape sequence characters into the Code Puppy prompt. Root causes fixed: 1. _listen_for_ctrl_x_posix (base_agent.py, command_runner.py): Reads stdin byte-by-byte in cbreak mode but never drains multi-byte escape sequences (mouse events, arrow keys, etc.). When the listener stops mid-sequence, remaining bytes leak into the stdin buffer and appear as garbled input in prompt_toolkit. Fixed by detecting ESC (0x1b) and draining all subsequent bytes within a 10ms timeout, plus a final drain in the finally block before restoring terminal attrs. 2. custom_server_form.py: The only TUI component with mouse_support=True. If cleanup fails (threading race, exception), mouse tracking stays permanently enabled, flooding stdin with escape sequences on every mouse action. Changed to mouse_support=False (consistent with all other TUI components) and added explicit disable_mouse_tracking() call in the finally block as a safety net. 3. terminal_utils.py: Added disable_mouse_tracking() and drain_stdin_escape_sequence() helpers for reuse across cleanup paths.
b5b0726 to
80e6003
Compare
… stdin All 5 escape-sequence drain loops (2 in base_agent.py, 2 in command_runner.py, 1 in terminal_utils.py) used unbounded `while select.select(...)` which hangs when stdin is mocked to always return data (CI test) or when a terminal floods mouse events (production). Cap at 256 iterations — more than enough for any real escape sequence burst. Fixes CI hang on test_listen_for_ctrl_x_posix_ctrl_x_detection (31-minute timeout → exit code 137 on macos-latest). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
code_puppy/terminal_utils.py (1)
139-149: Only emit mouse-disable sequences when stdout is a TTY.At Line 139, this runs on all Unix stdout targets. If stdout is redirected, the escape bytes can leak into logs/pipes. Add a TTY guard before writing sequences.
Proposed patch
def disable_mouse_tracking() -> None: @@ - if platform.system() == "Windows": + if platform.system() == "Windows": + return + if not sys.stdout.isatty(): return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code_puppy/terminal_utils.py` around lines 139 - 149, The code unconditionally writes terminal mouse-disable escape sequences to stdout on non-Windows systems; add a TTY guard so these bytes are only emitted when stdout is a terminal. Update the block that checks platform.system() and the sys.stdout.write call to first check sys.stdout.isatty() (or os.isatty(sys.stdout.fileno())) and skip writing the escape sequences when stdout is not a TTY; preserve the existing Windows early return and keep flushing behavior when the TTY guard passes (referencing the sys.stdout.write/sys.stdout.flush usage in terminal_utils.py).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code_puppy/terminal_utils.py`:
- Around line 171-183: The draining loop is running on non-Windows even when
stdin is a redirected pipe; guard it with a TTY check so you only drain terminal
input. Update the early conditional in the function containing
platform.system(), select.select, sys.stdin, max_bytes and drained so it returns
unless sys.stdin.isatty() is true (e.g., if platform.system() == "Windows" or
not sys.stdin.isatty(): return), then proceed with the existing select/read
loop; this prevents consuming real piped data.
---
Nitpick comments:
In `@code_puppy/terminal_utils.py`:
- Around line 139-149: The code unconditionally writes terminal mouse-disable
escape sequences to stdout on non-Windows systems; add a TTY guard so these
bytes are only emitted when stdout is a terminal. Update the block that checks
platform.system() and the sys.stdout.write call to first check
sys.stdout.isatty() (or os.isatty(sys.stdout.fileno())) and skip writing the
escape sequences when stdout is not a TTY; preserve the existing Windows early
return and keep flushing behavior when the TTY guard passes (referencing the
sys.stdout.write/sys.stdout.flush usage in terminal_utils.py).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 62e343fd-d722-4e4d-9946-169546d44600
📒 Files selected for processing (6)
code_puppy/agents/base_agent.pycode_puppy/command_line/mcp/custom_server_form.pycode_puppy/terminal_utils.pycode_puppy/tools/command_runner.pytests/agents/test_base_agent_full_coverage.pytests/command_line/test_model_settings_menu_coverage.py
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/agents/test_base_agent_full_coverage.py
- code_puppy/agents/base_agent.py
- tests/command_line/test_model_settings_menu_coverage.py
- code_puppy/tools/command_runner.py
- code_puppy/command_line/mcp/custom_server_form.py
…loops 1. custom_server_form.py: wrap disable_mouse_tracking() in try/except with a finally block so alt screen exit + set_awaiting_user_input(False) always runs even if the import/call throws. 2. Deduplicate 4 inline drain loops in base_agent.py and command_runner.py by calling the centralized drain_stdin_escape_sequence() helper from terminal_utils.py. Add optional `stream` parameter to the helper so callers can pass a specific stdin reference instead of sys.stdin. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Fixes #244 — After prolonged use of Code Puppy, mouse actions in the terminal send garbled escape sequence characters into the prompt, rendering them meaningless.
Root Cause
Three interacting bugs compound over long sessions:
_listen_for_ctrl_x_posix(inbase_agent.pyandcommand_runner.py) reads stdin byte-by-byte in cbreak mode but never drains multi-byte escape sequences (mouse events, arrow keys, etc.). When the listener stops mid-sequence, the remaining bytes leak into the stdin buffer and appear as garbled characters when prompt_toolkit takes over.custom_server_form.pyis the only TUI component withmouse_support=True. If cleanup fails (threading race, exception duringrun(in_thread=True)), mouse tracking stays permanently enabled, flooding stdin with escape sequences on every mouse action.Concurrent stdin access between the cbreak listener thread and prompt_toolkit Applications fragments escape sequences across readers.
Changes
code_puppy/agents/base_agent.py\x1b) is read, drain all subsequent bytes within 10ms timeout instead of discarding just one byte (matching the pattern already used inask_user_question/tui_loop.py)finallyblock before restoring terminal attrscode_puppy/tools/command_runner.pybase_agent.py— drain escape sequences on ESC byte and in finally blockcode_puppy/command_line/mcp/custom_server_form.pymouse_support=True→mouse_support=False(consistent with all 17 other TUI components)disable_mouse_tracking()call in thefinallyblock as a safety netcode_puppy/terminal_utils.pydisable_mouse_tracking(): sends VT100 escape sequences to disable all mouse tracking modesdrain_stdin_escape_sequence(): drains pending bytes from stdin to prevent escape sequence fragment leaksTesting
Summary by CodeRabbit
Bug Fixes
Tests