Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions .squad/agents/rachael/history.md
Original file line number Diff line number Diff line change
Expand Up @@ -133,3 +133,13 @@ Scribe note: the CLI dev-tool charter and prioritized backlog from the merged CL
## 2026-07-28T11:20:06+0000 — Independent #75 lockout revision
- Took ownership of PR #346's revision after Holden requested changes and Bryant was locked out.
- `c20ec211` corrected StringNormalizer/TfIdfVectorizer default-domain registration and LabelEncoder-1 default-attribute dtype selection; Holden re-approved.

## 2026-07-28T10:10:00-07:00 — REPL newline and stats polish

- Fixed TTY turn finalization so generated text that lacks its own trailing newline is followed by exactly one CLI-owned separator before the next prompt/status/error path; piped REPL output keeps the legacy byte-stable separator behavior.
- Expanded compact stats/profile reporting with explicit prefix-cache hit tokens plus percent of prompt, KV page activity on the compact line, and matching JSON/profile fields.

## 2026-07-28T10:33:00-07:00 — REPL stats block two-line follow-up

- Revised TTY stats rendering from a squeezed one-line string to a deliberate two-line block: performance/termination first, cache/context/scheduler/pages/RSS second.
- Restored compact finish reason and end-to-end throughput to the always-on TTY block now that Justin allowed two lines; non-TTY opt-in stats keep the single-line formatter for byte stability.
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

9 changes: 7 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -160,12 +160,17 @@ most detailed section.
without the full `--profile` report. `generate` uses the same compact line for
terminal text generation unless `--no-stats` is passed. While a REPL reply
streams, the numbers update live beneath it; when the turn ends they settle into
one line:
a deliberate two-line block: performance and termination first, cache/scheduler/
memory behavior second.

```text
[ 613 in · 64 out · 41.2 tok/s · ttft 116 ms · 598 reused · encoder 1/1 · rss 2.5 GiB ]
[ 613 in · 64 out · backend native · 41.7 tok/s · e2e 39.3 tok/s · ttft 116 ms · finish stop-seq · cap 3.6k->128 ]
[ cache 598/613 98% · ctx 677/8.2k · mm 120 · enc 1/2 · pg +5/-2 hot 1 pref 3 fail 1 · rss 2.5 GiB ]
```

If the scheduler had to admit a smaller decode budget, or the KV page pool
allocated/freed/evicted pages during the turn, those appear in this block.

The live view is drawn with [ratatui](https://ratatui.rs) into an *inline*
viewport rather than an alternate screen, so finished lines spill into the
terminal's own scrollback and the conversation stays selectable, copyable, and
Expand Down
7 changes: 7 additions & 0 deletions crates/onnx-genai-cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,13 @@ image = { workspace = true, features = ["png"] }
onnx-genai = { workspace = true }
serde_json = { workspace = true }

[target.'cfg(unix)'.dev-dependencies]
# PTY-driven e2e tests: openpty/dup to verify TTY-only rendering branches.
# nix is already in the dependency tree via ctrlc; no version bump needed.
# Features: "term" for nix::pty::openpty, "fs" for nix::unistd::dup,
# "poll" for nix::poll::poll (used for read timeouts in drain_pty_master).
nix = { version = "0.31", default-features = false, features = ["term", "fs", "poll"] }

[dependencies.pyo3]
version = "0.29"
# `abi3-py310` builds against the limited API (a single cp310-abi3 module usable
Expand Down
6 changes: 2 additions & 4 deletions crates/onnx-genai-cli/src/generate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -130,9 +130,7 @@ fn generate_text(
None,
) {
Ok(output) => {
if args.stream {
println!();
} else {
if !args.stream {
println!("{output}");
}
Comment on lines 132 to 135

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The premise here is incorrect. Before this change, generate.rs emitted println!() unconditionally for piped+stream (preserving the byte-stable trailing newline), and run_generation_turn emitted it conditionally for TTY+stream. Piped output was never changed; the TTY newline was always conditional on whether the output already ended with one.

What was genuinely wrong was the shape: one concern split across two files with opposite conditions. A reviewer as capable as Copilot misread it in both directions, which is evidence the shape was confusing. The fix consolidates the whole decision into run_generation_turn: piped still gets an unconditional newline (explicit comment explains why), TTY gets the conditional one. Both callers no longer emit newlines themselves. Two new e2e tests pin the piped byte-stable guarantee.

if let (Some(before), Some(after)) = (pages_before, backend.page_stats()) {
Expand All @@ -144,7 +142,7 @@ fn generate_text(
}
Err(error) if is_interrupt_error(&error) => {
// A Ctrl-C during a one-shot generation aborts and exits non-zero.
eprintln!("\n^C interrupted");
eprintln!("^C interrupted");
std::process::exit(EXIT_INTERRUPTED);
}
Err(error) => Err(error),
Expand Down
23 changes: 14 additions & 9 deletions crates/onnx-genai-cli/src/interactive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,6 @@ pub(super) fn initial_repl_show_stats(mode: ReplInputMode, no_stats: bool) -> bo
matches!(mode, ReplInputMode::Tty) && !no_stats
}

pub(super) fn plain_stream_needs_trailing_newline(used_live_this_turn: bool) -> bool {
!used_live_this_turn
}

struct ReplPrompt;

impl Prompt for ReplPrompt {
Expand Down Expand Up @@ -975,6 +971,7 @@ pub(super) fn run_repl(args: RunArgs, profiling: &ProfileArgs) -> anyhow::Result
None => println!("timeline off"),
}
}

Ok(ProfileSetting::Toggle(on)) => {
show_profile = on;
if on && onnx_genai::ort::profile::trace_destination().is_none() {
Expand Down Expand Up @@ -1294,7 +1291,6 @@ pub(super) fn run_repl(args: RunArgs, profiling: &ProfileArgs) -> anyhow::Result
profile.memory = memory;
}
let pages_before = backend.page_stats();
let used_live_this_turn = show_stats && live.is_active();
match run_generation_turn(
&mut backend,
turn,
Expand All @@ -1307,9 +1303,6 @@ pub(super) fn run_repl(args: RunArgs, profiling: &ProfileArgs) -> anyhow::Result
show_stats.then_some(&mut live),
) {
Ok(output) => {
if plain_stream_needs_trailing_newline(used_live_this_turn) {
println!();
}
// Reasoning models are trained with earlier turns' thinking
// removed, so replaying it degrades quality and inflates the
// context. Only the answer becomes history.
Expand Down Expand Up @@ -1352,7 +1345,7 @@ pub(super) fn run_repl(args: RunArgs, profiling: &ProfileArgs) -> anyhow::Result
// Drop the interrupted turn from history so a partial/aborted
// reply never pollutes the conversation context, then return to
// the prompt instead of exiting.
eprintln!("\n^C interrupted (press Ctrl-C again to exit)");
eprintln!("^C interrupted (press Ctrl-C again to exit)");
history.pop();
}
Err(error) => {
Expand Down Expand Up @@ -1396,3 +1389,15 @@ pub(super) fn stage_attachment(
}
Ok(Some(path))
}
#[cfg(test)]
mod tests {
use super::*;

#[test]
fn tty_mode_enables_live_stats_by_default() {
assert!(matches!(repl_input_mode(true, true), ReplInputMode::Tty));
assert!(initial_repl_show_stats(ReplInputMode::Tty, false));
assert!(!initial_repl_show_stats(ReplInputMode::Tty, true));
assert!(!initial_repl_show_stats(ReplInputMode::Plain, false));
}
}
16 changes: 2 additions & 14 deletions crates/onnx-genai-cli/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ use interactive::run_repl;
use interactive::{
InterruptAction, Interrupted, ReplInputMode, apply_context_sized_max_new_tokens,
context_exhaustion_error, context_window_is_full, drop_exhausted_repl_turn,
initial_repl_show_stats, interrupt_action, is_interrupt_error,
plain_stream_needs_trailing_newline, repl_input_mode, stage_attachment,
initial_repl_show_stats, interrupt_action, is_interrupt_error, repl_input_mode,
stage_attachment,
};
use model_inspection::{list, show, version};
use onnx_genai::engine::EngineDecodeBackend;
Expand Down Expand Up @@ -1161,18 +1161,6 @@ mod tests {
assert!(!initial_repl_show_stats(ReplInputMode::Plain, false));
}

#[test]
fn plain_stream_newline_depends_on_this_turns_renderer_usage() {
let pending_live_renderer = live_turn::LiveTurn::Pending;
assert!(pending_live_renderer.is_active());
assert!(plain_stream_needs_trailing_newline(false));
assert!(!plain_stream_needs_trailing_newline(true));
assert!(
plain_stream_needs_trailing_newline(false),
"a pending reusable live renderer must not suppress the newline when this turn did not use it"
);
}

#[test]
fn one_ctrl_c_stops_the_generation_and_two_exit() {
// Idle prompt: the first press only warns, the second leaves.
Expand Down
15 changes: 10 additions & 5 deletions crates/onnx-genai-cli/src/live_turn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,12 +151,14 @@ impl LiveTurn {
}

/// End the turn: spill the whole reply into scrollback and clear the area.
pub(crate) fn finish(&mut self) -> anyhow::Result<()> {
pub(crate) fn finish(&mut self, output_needs_trailing_newline: bool) -> anyhow::Result<()> {
match self {
Self::Active(active) => active.finish(),
// Nothing was rendered, so the plain path still owes a newline.
Self::Active(active) => active.finish(output_needs_trailing_newline),
// Nothing was rendered, so any TTY separator still belongs to the plain path.
_ => {
println!();
if output_needs_trailing_newline {
println!();
}
Ok(())
}
}
Expand Down Expand Up @@ -244,7 +246,10 @@ impl Active {
Ok(())
}

fn finish(&mut self) -> anyhow::Result<()> {
fn finish(&mut self, output_needs_trailing_newline: bool) -> anyhow::Result<()> {
if output_needs_trailing_newline {
self.lines.push(Vec::new());
}
let width = self.width();
for line in std::mem::take(&mut self.lines) {
self.commit(line, width)?;
Expand Down
145 changes: 143 additions & 2 deletions crates/onnx-genai-cli/src/output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,25 @@ pub(super) fn emit_stats_line(show_stats: bool, show_profile: bool, profile: &mu
return;
}
profile.memory.sample_peak();
eprintln!("{}", profile.to_stats_line());
// Stats go to stderr — probe stderr, not stdout. Probing stdout inverts
// the decision when stdout is redirected (`> out.txt` gives a file on
// stdout but a terminal on stderr) or when stderr is redirected
// (`2> log` gives a file on stderr but a terminal on stdout).
let stats = stats_text(io::stderr().is_terminal(), profile);
eprintln!("{stats}");
}

/// Choose the stats rendering for the given stderr terminal state.
///
/// Extracted from `emit_stats_line` so the branching logic is unit-testable
/// without a real terminal. Pass `io::stderr().is_terminal()` at the call
/// site; pass `true`/`false` in tests.
fn stats_text(stderr_is_terminal: bool, profile: &RunProfile) -> String {
if stderr_is_terminal {
profile.to_stats_block()
} else {
profile.to_stats_line()
}
}

fn budget_cap_notice(cap: GenerationBudgetCap) -> String {
Expand All @@ -116,6 +134,18 @@ fn budget_cap_notice(cap: GenerationBudgetCap) -> String {
)
}

/// Whether a TTY streaming reply needs a trailing newline added by the caller.
///
/// Returns `true` when the reply is non-empty **and** its last character is
/// not `\n`. Extracted from `run_generation_turn` so the decision is directly
/// unit-testable without a full generate pipeline or a real terminal.
///
/// The piped path always emits an unconditional newline (preserving historical
/// byte-stable behaviour for scripts); this predicate is only for the TTY path.
fn needs_trailing_newline(output: &str) -> bool {
!output.is_empty() && !output.ends_with('\n')
}

fn should_emit_stats_line(show_stats: bool, show_profile: bool) -> bool {
show_stats && !show_profile
}
Expand Down Expand Up @@ -233,8 +263,29 @@ pub(super) fn run_generation_turn(
print!("\x1b[0m");
let _ = io::stdout().flush();
}
let output_needs_trailing_newline = needs_trailing_newline(&output);
if let Some(live) = live {
live.finish()?;
live.finish(output_needs_trailing_newline)?;
} else if stream {
if io::stdout().is_terminal() {
// TTY: conditional — skip when the reply already ended with a
// newline. A model that terminates with `\n` would otherwise
// produce a visible blank line before the next prompt.
if output_needs_trailing_newline {
println!();
}
} else {
// Piped: always emit a trailing newline, unconditionally.
//
// Piped consumers of `--stream` output have always received a
// terminating newline on every release. Making this conditional
// (like the TTY path above) would silently break any script or
// pipeline that depends on the line boundary — the extra newline
// is invisible to the consumer, but its absence is a byte-stable
// regression. The TTY path is conditional because a double blank
// line is a visible defect there; here it is not.
println!();
}
}
GENERATING.store(false, Ordering::SeqCst);
timings.finish();
Expand Down Expand Up @@ -300,4 +351,94 @@ mod tests {
assert!(!should_emit_stats_line(true, true));
assert!(!should_emit_stats_line(false, false));
}

/// `needs_trailing_newline` is the predicate that determines whether the
/// TTY streaming path adds a `\n` after the reply. It must return `true`
/// exactly when the output would leave the cursor in the middle of a line —
/// that is, when the output is non-empty **and** does not already end with
/// a newline. Every case is testable in isolation because the predicate
/// is a pure function with no I/O dependency.
///
/// The PTY integration test (`pty_tty_e2e.rs`) confirms that this predicate
/// is wired to a real terminal, but it cannot distinguish the conditional
/// from an unconditional `println!()` for the current tiny-llm fixture
/// (which never emits a trailing `\n`). These unit tests close that gap.
#[test]
fn tty_trailing_newline_predicate_covers_all_cases() {
// Normal reply with no trailing newline: cursor is mid-line → needs one.
assert!(needs_trailing_newline("hello world"));

// Reply that already ends with \n: cursor is at a new line → no extra.
// This is the case the conditional prevents a visible blank line for.
assert!(!needs_trailing_newline("hello world\n"));

// Empty output (e.g. --max-new-tokens 0): nothing to terminate.
assert!(!needs_trailing_newline(""));

// Multi-line reply ending with \n — still no extra newline needed.
assert!(!needs_trailing_newline("line one\nline two\n"));

// Reply ending with \n followed by trailing spaces: the last character
// is not \n, so the cursor is mid-line and a newline is required.
assert!(needs_trailing_newline("hello\n "));

// Newline is not the last character even if one appears inside.
assert!(needs_trailing_newline("line one\nno newline at end"));
}

/// The stats format is determined by stderr's terminal state, not stdout's.
///
/// `stats_text` takes only `stderr_is_terminal`; stdout's state is
/// deliberately absent from the signature. The two cases that motivated
/// the fix:
///
/// A) `> out.txt` in a terminal: stdout=file, stderr=terminal → block
/// B) `2> stats.log` in a terminal: stdout=terminal, stderr=file → line
///
/// A profile with both headline data (`prompt_tokens`) and resource data
/// (`context`) is used so the block form produces two `[ … ]` lines joined
/// by `\n`, making it structurally distinct from the single-line form.
#[test]
fn stats_format_follows_stderr_not_stdout() {
use crate::profile::{ContextUsage, RunProfile};

let profile = RunProfile {
prompt_tokens: Some(10),
context: Some(ContextUsage {
used_tokens: 10,
max_tokens: 4096,
}),
..Default::default()
};

// stderr=terminal → two-line block (headline \n resources)
let block = stats_text(true, &profile);
assert!(
block.contains('\n'),
"stderr=terminal should produce two-line block; got: {block:?}"
);

// stderr=plain → single line (no embedded newline)
let line = stats_text(false, &profile);
assert!(
!line.contains('\n'),
"stderr=plain should produce single line; got: {line:?}"
);

// Case A: stdout redirected (not terminal), stderr on terminal → block
// (the old stdout probe would have chosen line here)
let case_a = stats_text(/* stderr */ true, &profile);
assert!(
case_a.contains('\n'),
"case A: stderr=terminal must give block"
);

// Case B: stdout on terminal, stderr redirected (not terminal) → line
// (the old stdout probe would have chosen block here)
let case_b = stats_text(/* stderr */ false, &profile);
assert!(
!case_b.contains('\n'),
"case B: stderr=plain must give line"
);
}
}
Loading
Loading