fix(cli): robust terminal size detection and diagnostic window_too_small widget - #57453
fix(cli): robust terminal size detection and diagnostic window_too_small widget#57453Sahil-SS9 wants to merge 2 commits into
Conversation
alt-glitch
added
type/bug
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused diagnostic work. The current cli.py:13343 startup scroll path is still present on main, so the issue is not superseded.
Problems
cli.py:3201returns on any positive stdout ioctl. Python'sshutil.get_terminal_size()also queriessys.__stdout__whenCOLUMNS/LINESare unset, so a stale-but-positive80x24result bypasses every later strategy in this PR.cli.py:13063changes startup scroll padding, whilecli.py:14852only replaces HSplit's fallback display. prompt_toolkit documentswindow_too_smallas the container displayed after insufficient layout space is determined; this does not alter the output-size path that triggers it.tests/cli/test_cli_window_too_small.py:58-68builds a local expected string rather than invoking the widget's text callable, and does not cover a stale-positive size.
Suggested changes
- Trace and correct the prompt_toolkit output-size boundary; compare alternate size sources before accepting an ioctl value.
- Add a regression with
80x24from stdout and a larger verified pane source, plus a test of the real diagnostic widget text.
Automated hermes-sweeper review.
| try: | ||
| size = os.get_terminal_size(fd) | ||
| if size.columns > 0 and size.lines > 0: | ||
| return size.columns, size.lines |
There was a problem hiding this comment.
This early return prevents the advertised fallback strategies from running for the reported stale-but-positive case. When COLUMNS/LINES are absent, shutil.get_terminal_size() also reads stdout, so a stale 80x24 here reproduces the existing result rather than reaching a tmux-aware source.
There was a problem hiding this comment.
Fixed. _detect_terminal_size() no longer returns early on a stale-but-positive 80x24 ioctl value — it now treats that exact (cols, rows) == (80, 24) marker as a candidate and keeps going, so the tmux-aware tput source can win when the real pane is larger. A regression test (test_stale_positive_80x24_reaches_alternate_source) covers this path. Pushed to the branch.
| # scroll the cursor to the last row before any content is rendered. | ||
| try: | ||
| _term_lines = shutil.get_terminal_size().lines | ||
| _term_lines = _detect_terminal_size()[1] |
There was a problem hiding this comment.
This only changes the pre-Application scroll padding. The Window too small decision is made later by prompt_toolkit layout from its output geometry; please address that size source as well if the goal is to prevent the fallback rather than only diagnose it.
There was a problem hiding this comment.
Addressed. The Window too small decision is made by prompt_toolkit from its output geometry (output.get_size()), so I routed the output's term-size callable in _build_cpr_disabled_output through _detect_terminal_size(). The layout now sees the real pane size (falling back to the direct ioctl on any detector error) instead of a stale 80x24, so the fallback is prevented at the size source rather than only pre-Application scroll padding. Pushed to the branch.
| """The text callable should include 'detected' and dimensions.""" | ||
| fake_size = os.terminal_size((199, 48)) | ||
| with patch("os.get_terminal_size", return_value=fake_size): | ||
| cols, rows = cli_mod._detect_terminal_size() |
There was a problem hiding this comment.
This test calls the detector and reconstructs the expected literal, but never invokes the FormattedTextControl from _make_window_too_small_widget(). Exercise the widget's actual callable, and add a stale-positive 80x24 regression that verifies the selected alternate source.
There was a problem hiding this comment.
Fixed. The tests now invoke the widget's actual FormattedTextControl callable (_widget_text() calls control.text()), exercising the same path the renderer uses rather than reconstructing the expected literal. Added a stale-positive 80x24 regression that verifies the detector reaches the alternate source, and a widget-level test asserting the callable renders non-stale dimensions in that case. Pushed to the branch.
teknium1
added
sweeper:risk-compatibility
- _detect_terminal_size: do not short-circuit on a stale 80x24 ioctl value; treat it as a candidate and let the tmux-aware tput source win so a stale-but-positive winsize no longer reproduces the misdetection. - _build_cpr_disabled_output: route prompt_toolkit's output geometry through _detect_terminal_size so the 'Window too small' decision made from output.get_size() sees the real pane size. - tests: exercise the widget's actual FormattedTextControl callable, and add a stale-positive 80x24 regression covering the alternate source.
2db8c03 to
af24f0a
Compare
3 participants
Fixes #57393
Description
shutil.get_terminal_size()can fall back to 80×24 inside tmux when the PTY winsize is stale orCOLUMNS/LINESenv vars are absent, causing a spurious "Window too small" error even in a full-size pane.This PR adds:
_detect_terminal_size()— a robust helper that tries multiple strategies in order: ioctl on stdout/stdin/stderr,shutil.get_terminal_size(), andtputsubprocess fallback. Returns the largest plausible result._make_window_too_small_widget()— a diagnosticwindow_too_smallcontainer for the rootHSplitthat shows the detected terminal dimensions (e.g.Window too small — detected 199×48) instead of prompt_toolkit's bare "Window too small..." message, making size-misdetection bugs immediately diagnosable.shutil.get_terminal_size().linescall in the initial blank-line scroll with_detect_terminal_size()so the scroll height matches the real pane.Verification
scripts/run_tests.sh tests/cli/test_cli_window_too_small.py— 6 tests pass (CI-parity env)python -c "compile(open('cli.py').read(), 'cli.py', 'exec')"— clean compile