Skip to content

test(cli): give the PTY harness a real window size - #393

Merged
justinchuby merged 2 commits into
mainfrom
fix/repl-swallows-typeahead
Jul 29, 2026
Merged

justinchuby merged 2 commits into
mainfrom
fix/repl-swallows-typeahead

Conversation

@justinchuby

Copy link
Copy Markdown
Owner

A hazard in merged code

PR #372's PTY harness calls openpty(None, None) — a 0x0 window. ratatui's insert_before_no_scrolling_regions infinite-loops at zero rows, so the harness as merged is one type-ahead test away from hanging a CI job, and the next person to extend it would have had no idea why.

The window is now a real 24x80, with the reason at the call site — a bare 24, 80 reads as arbitrary and someone would eventually "simplify" it back. The CR-not-LF gotcha is cross-referenced there too: terminals send \r, and feeding \n means the line never completes, which presents as a hang that looks exactly like swallowed input.

Both artifacts were found the expensive way, so they now live in the code a harness author will read, not only in a decision record.

Confirmed neutral rather than assumed: test 1 passes at both 0x0 and 24x80; test 2 behaves identically at both. Test 2's empty read under WSL is not a window-size effect — it is the harness's own 30s drain_pty_master timeout against a ~48s debug model load over the slow /mnt/c mount. A byte-for-byte replica with a 90s window returns the expected tok22tok22tok20\r\n. Native CI Linux loads fast enough that both pass.

Context: three negatives on the type-ahead report

This branch began as an investigation into run swallowing typed-ahead input. Input is not being lost. Each hypothesis was refuted with evidence rather than argument:

  • The cursor-query theory is dead on both platforms. crossterm 0.29's cursor::position() routes user keystrokes to skipped_events and drains them back into the queue; ratatui 0.30's inline viewport never reads stdin outside init/resize. Verified by withholding the DSR reply 250ms and injecting /help into the open window — it survived.
  • Windows cannot lose it either. ReadConsoleInputW reads raw records unaffected by ENABLE_LINE_INPUT; SetConsoleMode does not flush the queue; there is no FlushConsoleInputBuffer anywhere. Instrumented GetConsoleMode (0x01f7) and pending INPUT_RECORD counts across prompt boundaries confirm it, including for a partial line with no Enter.
  • The timing gap is closed. Re-run against qwen2.5-0.5b streaming ~19s, with /help injected pasted, delayed, and character-by-character at human speed. Type-ahead survived every time.

Raw bytes argue against the erase hypothesis: /help appears at the banner and at the next prompt — never mid-stream. ConPTY does not echo type-ahead during generation, so there is nothing for a repaint to overwrite.

No speculative fix shipped. The remaining untested path is native-conhost immediate echo, which no automated harness on a headless box can produce — plausibly why it is visible in an ordinary session and not here. Fixing an unobserved mechanism would look like it worked and would not.

…ght hang)

PR #372's `pty_tty_e2e.rs` opened its PTY with `openpty(None, None)`, i.e. a
0x0 window. Its own two tests drive one-shot `generate` and are unaffected, but
the `run` REPL renders through ratatui's inline viewport, whose
`insert_before_no_scrolling_regions` infinite-loops at 0 rows (`to_draw =
min(h, 0) = 0`, never makes forward progress). The next author to add a
type-ahead / streaming test on top would hang the harness for the 30s
`drain_pty_master` timeout and then time out CI, with no obvious cause.

Set a real 24x80 `Winsize` in `open_pty()` and document at the call site both
why a non-zero window is mandatory and the companion CR-not-LF gotcha (a
terminal sends `\r` for Enter; feeding `\n` never completes the line and hangs
`read_line`). Both are "false swallow" traps for anyone extending this harness.

Confirmed neutral: `stats_single_line_when_stderr_is_piped_and_stdout_is_tty`
passes at both 0x0 and 24x80; `tty_stdout_gets_exactly_one_trailing_newline_
after_streaming` behaves identically at both sizes (its outcome depends on the
drain timeout vs model-load time, not the window size).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@codecov

codecov Bot commented Jul 29, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.38%. Comparing base (eb5b5d4) to head (c2f7ca6).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #393      +/-   ##
==========================================
+ Coverage   80.59%   81.38%   +0.78%     
==========================================
  Files         311      311              
  Lines      121012   121100      +88     
  Branches   121012   121100      +88     
==========================================
+ Hits        97530    98555    +1025     
+ Misses      19504    18558     -946     
- Partials     3978     3987       +9     
Flag Coverage Δ
cli-ort-linux 83.16% <ø> (+0.16%) ⬆️
cli-ort-windows 82.55% <ø> (+0.17%) ⬆️
mlas 77.91% <ø> (ø)
offline 81.35% <ø> (+0.82%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 10 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

github-actions Bot commented Jul 29, 2026 •

Copy link
Copy Markdown

🔴 Benchmark Regression Detected

Comparison of criterion micro-benchmarks: PR head vs merge-base, measured on the same runner in the same job (base first → PR second).

ℹ️ Absolute times are informational only — they vary with runner load. The % change column is the reliable signal because both sides ran under identical conditions.

Status Scenario Base PR Change
🔴 matmul/medium_generic_f16_threads=8/32x512x512 31.79 µs 47.53 µs +49.5%
🔴 gather/small_bf16_threads=1-internal/4096 563.1 ns 833.3 ns +48.0%
🔴 gather/medium_f32_threads=1-internal/32768 4.71 µs 6.29 µs +33.5%
⚠️ matmul/large_generic_f16_threads=8/32x1024x1024 96.69 µs 123.43 µs +27.7%
⚠️ matmul/large_generic_bf16_threads=8/32x1024x1024 2.10 ms 2.63 ms +24.9%
⚠️ matmul/medium_generic_f16_threads=1/32x512x512 33.96 µs 42.24 µs +24.4%
⚠️ matmul/large_generic_bf16_threads=1/32x1024x1024 2.35 ms 2.73 ms +15.8%
⚠️ add/large_f16_threads=1-internal/4194304 46.95 ms 54.14 ms +15.3%
✅ gather/small_f16_threads=1-internal/4096 553.5 ns 621.7 ns +12.3%
✅ gather/small_f32_threads=1-internal/4096 801.9 ns 868.1 ns +8.3%
✅ tokenization/encode_tokens_per_second 400.72 µs 414.88 µs +3.5%
✅ gather/medium_f16_threads=1-internal/32768 2.85 µs 2.89 µs +1.2%
✅ sampling_latency/top_p_per_token 1.06 ms 1.08 ms +1.1%
✅ gather/medium_bf16_threads=1-internal/32768 2.93 µs 2.93 µs -0.1%
✅ tokenization/decode_tokens_per_second 6.56 ms 6.54 ms -0.3%
✅ matmul/large_generic_f16_threads=1/32x1024x1024 89.64 µs 88.20 µs -1.6%
✅ matmul/medium_generic_bf16_threads=1/32x512x512 673.42 µs 656.42 µs -2.5%
✅ reduce_mean/medium_f32_threads=1-internal/65536 287.95 µs 278.08 µs -3.4%
✅ reduce_mean/large_f32_threads=1-internal/262144 1.14 ms 1.09 ms -4.3%
✅ add/large_bf16_threads=1-internal/4194304 49.79 ms 46.49 ms -6.6%
✅ sampling_latency/min_p_per_token 402.28 µs 375.29 µs -6.7%
✅ matmul/medium_generic_f32_threads=1/32x512x512 2.75 ms 2.57 ms -6.9%
✅ matmul/medium_generic_f32_threads=8/32x512x512 1.61 ms 1.49 ms -7.5%
✅ matmul/small_generic_bf16_threads=8/1x256x256 41.53 µs 38.14 µs -8.2%
✅ matmul/medium_generic_bf16_threads=8/32x512x512 703.63 µs 645.72 µs -8.2%
✅ sampling_latency/top_k_per_token 558.74 µs 512.03 µs -8.4%
✅ add/small_f16_threads=1-internal/1024 15.41 µs 14.10 µs -8.5%
✅ reduce_mean/small_f32_threads=1-internal/4096 18.42 µs 16.64 µs -9.6%
✅ add/large_f32_threads=1-internal/4194304 52.20 ms 46.86 ms -10.2%
✅ matmul/small_generic_f16_threads=8/1x256x256 40.85 µs 36.37 µs -11.0%
✅ gather/large_f16_threads=1-internal/131072 17.52 µs 15.35 µs -12.4%
✅ sampling_latency/greedy_per_token 4.05 µs 3.53 µs -12.8%
✅ matmul/small_generic_f32_threads=8/1x256x256 63.58 µs 55.12 µs -13.3%
✅ add/small_bf16_threads=1-internal/1024 16.25 µs 13.88 µs -14.6%
✅ matmul/small_generic_bf16_threads=1/1x256x256 41.19 µs 35.06 µs -14.9%
🟢 matmul/small_generic_f32_threads=1/1x256x256 47.74 µs 40.45 µs -15.3%
🟢 kv_cache/alloc_dealloc_pages 45.81 µs 38.50 µs -16.0%
🟢 add/medium_f16_threads=1-internal/262144 3.35 ms 2.81 ms -16.2%
🟢 matmul/small_generic_f16_threads=1/1x256x256 40.28 µs 33.54 µs -16.7%
🟢 grammar_masking/llguidance_compute_mask/32 105.06 µs 87.17 µs -17.0%
🟢 gather/large_f32_threads=1-internal/131072 42.72 µs 35.21 µs -17.6%
🟢 add/medium_bf16_threads=1-internal/262144 3.52 ms 2.88 ms -18.2%
🟢 logit_processing/seven_processor_chain_per_step 1.76 ms 1.41 ms -20.1%
🟢 matmul/large_generic_f32_threads=1/32x1024x1024 14.67 ms 10.85 ms -26.1%
🟢 add/small_f32_threads=1-internal/1024 343.5 ns 242.5 ns -29.4%
🟢 gather/large_bf16_threads=1-internal/131072 16.97 µs 11.07 µs -34.8%
🟢 matmul/large_generic_f32_threads=8/32x1024x1024 6.81 ms 4.27 ms -37.3%
🟢 add/medium_f32_threads=1-internal/262144 5.37 ms 2.73 ms -49.2%

Visual flags: ⚠️ ≥ 15% slower, 🔴 ≥ 30% slower — calibrated against measured runner noise (~27% worst-case on multi-threaded matmul)

Host info
CPU: Apple M1 (Virtual)
Cores: 3
OS: Darwin 25.4.0 arm64
Rust: rustc 1.97.1 (8bab26f4f 2026-07-14)
Load avg: { 3.94 4.08 7.32 }
What this cannot catch
  • Regressions in code paths not covered by these benchmarks (e.g., end-to-end decode with a real model)
  • Sub-threshold regressions that compound over multiple PRs
  • Performance changes that only manifest under GPU execution
  • Latency changes in the ORT integration path (these benchmarks exercise the native Rust kernels)

`drain_pty_master` had a bare 30s total budget. The child is silent on the PTY
until its first token (model load logs go to stderr), so on a slow machine the
budget can expire during load and return an empty read — which then trips the
trailing-newline assertion and masquerades as a rendering defect. On the slowest
machine measured (a debug build over WSL 2 `/mnt/c` under load), cold
load-to-first-token was ~48s, i.e. the old 30s budget already lost by 18s; it
was a coin flip that had been landing the right way. A magic constant like that
is exactly what RULES.md rule 2 exists to catch.

Make it an idle timeout instead: each poll() waits up to DRAIN_IDLE_TIMEOUT for
the *next* byte, so the clock resets on every read. A slow start or a long
stream is tolerated; only a genuinely stuck child (no byte for the whole window)
fails, and it fails promptly rather than hanging to the job limit. The budget is
120s, justified in a comment against the measured ~48s worst case (~2.5x
headroom for a cold CI runner), not picked to feel safe.

Draining now returns `DrainOutcome { bytes, timed_out }`. Test 2 checks
`timed_out` before asserting on content and reports a timeout distinctly — "this
is a stuck or too-slow child, NOT a trailing-newline defect" with an ordered
checklist — so a timeout can never wear the content assertion's clothes
(RULES.md rule 1).

Validated on WSL 2 (the machine that exposed the flake): both pty_tty_e2e tests
now pass with the 120s budget where 30s returned empty; clippy -D warnings is
clean there (the file is #[cfg(unix)], never linted on Windows).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@justinchuby
justinchuby merged commit ca3c39d into main Jul 29, 2026
14 checks passed
@justinchuby
justinchuby deleted the fix/repl-swallows-typeahead branch July 29, 2026 08:02
justinchuby pushed a commit that referenced this pull request Jul 29, 2026
Recovered decision records from agents' per-worktree inbox files,
which were deleted before Scribe ran. Substance survived in history.md
and PR descriptions; durable rules now appended to decisions.md:

From Rachael (PR #372):
- Probe the stream you are writing to, not a convenient nearby one
- Run the gate command CI runs, verbatim — local subset validation fails
- Terminal behaviour requires PTY-driven tests; piped I/O cannot cover it

From Zhora (PR #393):
- Type-ahead is not lost during generation on Unix or Windows
- PTY-harness hazards that present as swallowed input
- Prefer an idle timeout to a total one when child is silent before first output

From Leon (PRs #367/#385/#392/#395):
- DeepSeek "repeats thinking" is not a backend bug, but greedy-decoding degeneration
- Model-declared generation defaults are canonical; our constants are fallback, not override
- CUDA driver API ships with display driver, not toolkit; inferred bad facts must be verified

From Coordinator:
- Standing Operational Rule: worktree lifecycle and decision merging — do not delete
  a worktree before Scribe has merged its decision inbox

Before: 27,360 bytes | After: 34,662 bytes | Added: 7,302 bytes

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants