From e11ddcfb2965f12590bc0a184c9b4b11617b7730 Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Tue, 8 Sep 2026 15:35:47 +0200 Subject: [PATCH 1/4] feat(printer): Hold output while a widget owns the terminal Output produced while a prompt is up now waits for the answer instead of landing in the middle of the question. A tool result arriving from a parallel tool, a title generator finishing, an MCP server logging to stderr: each used to print straight over the widget's frame, on a terminal in raw mode where a line feed no longer returns to column zero. The same holds while an external `$EDITOR` has the screen. A handover is bracketed by the guard `suspend_status` and the prompt writers already take, so the printer now knows exactly when the terminal is not its own. For that window the worker keeps ordinary output in a queue and writes it, in the order it was produced, once the terminal comes back. Handovers nest, and only the outermost hand-back releases anything: a permission prompt opening the inline reply widget on top of itself never gives the screen away mid-question. A print task says who it belongs to, and only a prompt session's own writes reach a terminal someone else owns. Nothing is dropped: if a run ends with a widget still holding the terminal, whatever it was holding back is written on the way out, because that output is the answer the user asked for and an untidy final frame is the smaller loss. Signed-off-by: Jean Mertz --- crates/jp_printer/src/printer.rs | 123 ++++++++++++++++++++++--- crates/jp_printer/src/printer_tests.rs | 80 ++++++++++++++++ crates/jp_printer/src/region.rs | 10 ++ 3 files changed, 200 insertions(+), 13 deletions(-) diff --git a/crates/jp_printer/src/printer.rs b/crates/jp_printer/src/printer.rs index 8f8bb725d..0474366ed 100644 --- a/crates/jp_printer/src/printer.rs +++ b/crates/jp_printer/src/printer.rs @@ -1,6 +1,7 @@ //! The printer module. use std::{ + collections::VecDeque, fmt::{self, Write}, io, sync::{ @@ -158,6 +159,7 @@ impl Printer { rx, delay_control, regions: RegionStack::new(), + held: VecDeque::new(), }; worker.run(); }) @@ -292,13 +294,17 @@ impl Printer { StatusRegion::new(id, self.tx.clone(), buffer, refresh) } - /// Suspend status-region rendering until the returned guard drops. + /// Hand the terminal to a writer outside the printer until the returned + /// guard drops. /// - /// Blocks until the worker has erased any drawn region and entered the - /// suspended state, so a caller handing the terminal to a writer outside - /// the printer — an external `$EDITOR`, say — knows the rows are gone + /// Blocks until the worker has erased any drawn status region, so a caller + /// handing the terminal to an external `$EDITOR` knows the rows are gone /// before the child paints. - /// Returns immediately when regions are disabled. + /// For the guard's lifetime the worker also holds back ordinary output, + /// writing it once the terminal comes back: whoever holds the terminal owns + /// the cursor, and JP keeps producing while they have it. + /// + /// Handovers nest; the terminal comes back when the outermost guard drops. #[must_use] pub fn suspend_status(&self) -> SuspendGuard { self.suspend_regions() @@ -306,10 +312,6 @@ impl Printer { /// Enqueue a suspension and wait for the worker to apply it. fn suspend_regions(&self) -> SuspendGuard { - if !self.chrome_repaints() || !self.terminal.permits_regions(self.format) { - return SuspendGuard::inert(); - } - let (ack, rx) = mpsc::channel(); if self @@ -493,6 +495,7 @@ impl Printer { PrinterWriter { printer: self, target: PrintTarget::Out, + origin: PrintOrigin::Content, } } @@ -502,6 +505,7 @@ impl Printer { PrinterWriter { printer: self, target: PrintTarget::Err, + origin: PrintOrigin::Content, } } @@ -552,6 +556,7 @@ impl Printer { writer: PrinterWriter { printer: self, target: self.prompt_target(), + origin: PrintOrigin::Prompt, }, _suspension: self.begin_prompt_session(), _trace: PromptTrace::open(), @@ -573,6 +578,7 @@ impl Printer { let mut task = p.into_task(); task.content.push('\n'); task.target = self.prompt_target(); + task.origin = PrintOrigin::Prompt; self.send(Command::Print(task)); } @@ -769,6 +775,9 @@ pub struct PrinterWriter<'a> { /// The target output stream. target: PrintTarget, + + /// Who the writes belong to. + origin: PrintOrigin, } impl fmt::Write for PrinterWriter<'_> { @@ -784,6 +793,7 @@ impl fmt::Write for PrinterWriter<'_> { content: s.to_owned(), mode: PrintMode::Instant, target: self.target, + origin: self.origin, }; self.printer @@ -914,6 +924,7 @@ impl io::Write for OwnedPrinterWriter { content: s.to_owned(), mode: PrintMode::Instant, target: self.target, + origin: PrintOrigin::Prompt, }; self.tx .send(Command::Print(task)) @@ -950,6 +961,11 @@ struct Worker { /// The claimed status regions and the rows currently painted for them. regions: RegionStack, + + /// Ordinary output produced while the terminal is handed to someone else. + /// + /// Written, in the order it was produced, once the terminal comes back. + held: VecDeque, } impl Worker { @@ -977,8 +993,18 @@ impl Worker { }; match cmd { - Command::Print(task) => self.process_task(&task), - Command::Region(cmd) => self.regions.apply(cmd, &mut self.err), + Command::Print(task) => { + if let Some(task) = self.admit(task) { + self.process_task(&task); + } + } + Command::Region(cmd) => { + let resuming = matches!(cmd, RegionCommand::Resume); + self.regions.apply(cmd, &mut self.err); + if resuming { + self.release_held(); + } + } Command::Flush(tx) => { // We don't need to do anything specific to flush out/err // because we flush after every write in `process_task`. We @@ -995,9 +1021,47 @@ impl Worker { } } + // A run can end with a widget still holding the terminal — a `Ctrl+C` + // at a prompt, an error unwinding past one. Nothing is going to give it + // back, and what is held is the answer the user asked for, so it goes + // out anyway: a last frame the widget left untidy is a smaller loss + // than the output. + self.drain_held(); self.regions.erase(&mut self.err); } + /// Decide whether `task` may be written now, holding it back if not. + /// + /// Only the prompt session's own writes reach a terminal someone else owns; + /// everything else waits for it to come back. + fn admit(&mut self, task: PrintTask) -> Option { + if !self.regions.is_suspended() || task.origin == PrintOrigin::Prompt { + return Some(task); + } + + self.held.push_back(task); + None + } + + /// Write everything held back, now that the terminal has come back. + /// + /// A no-op while it is still away, so the inner close of a nested handover + /// releases nothing. + fn release_held(&mut self) { + if self.regions.is_suspended() { + return; + } + + self.drain_held(); + } + + /// Write everything held back, in the order it was produced. + fn drain_held(&mut self) { + while let Some(task) = self.held.pop_front() { + self.process_task(&task); + } + } + /// Drain all pending commands, printing instantly. /// /// Processes queued `Print` tasks as `Instant` (ignoring typewriter @@ -1007,8 +1071,18 @@ impl Worker { fn drain_instant(&mut self) { while let Ok(cmd) = self.rx.try_recv() { match cmd { - Command::Print(task) => self.process_task_instant(&task), - Command::Region(cmd) => self.regions.apply(cmd, &mut self.err), + Command::Print(task) => { + if let Some(task) = self.admit(task) { + self.process_task_instant(&task); + } + } + Command::Region(cmd) => { + let resuming = matches!(cmd, RegionCommand::Resume); + self.regions.apply(cmd, &mut self.err); + if resuming { + self.release_held(); + } + } Command::Flush(tx) => { let _ = tx.send(()); } @@ -1092,6 +1166,7 @@ impl Worker { content, mode, target, + .. } = task; let writer: &mut dyn io::Write = match target { @@ -1293,6 +1368,23 @@ pub enum PrintTarget { Tty, } +/// Who a print task belongs to. +/// +/// While the terminal is handed to a widget, ordinary output waits and the +/// widget's own writes go straight through: it is drawing the screen the user +/// is answering on, and content landing in the middle of that is what the wait +/// exists to prevent. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub enum PrintOrigin { + /// Ordinary output: assistant responses, chrome, structured records. + #[default] + Content, + + /// Part of a prompt session — the widget's own drawing, or a line written + /// to give its question the context it needs to be answerable. + Prompt, +} + #[derive(Debug, Clone)] /// A task to be printed. pub struct PrintTask { @@ -1304,6 +1396,10 @@ pub struct PrintTask { /// The target output stream. pub target: PrintTarget, + + /// Who the task belongs to, which decides whether it may land while a + /// widget owns the terminal. + pub origin: PrintOrigin, } impl Default for PrintTask { @@ -1312,6 +1408,7 @@ impl Default for PrintTask { content: String::new(), mode: PrintMode::Instant, target: PrintTarget::Out, + origin: PrintOrigin::Content, } } } diff --git a/crates/jp_printer/src/printer_tests.rs b/crates/jp_printer/src/printer_tests.rs index f407d5272..3d380688f 100644 --- a/crates/jp_printer/src/printer_tests.rs +++ b/crates/jp_printer/src/printer_tests.rs @@ -873,6 +873,86 @@ fn a_prompt_that_ends_mid_line_does_not_hold_the_region_hostage() { ); } +#[test] +fn output_produced_while_a_prompt_is_open_waits_for_it() { + let (printer, out, err) = Printer::memory(OutputFormat::TextPretty); + + { + let mut prompt = printer.prompt_writer(); + write!(prompt, "Run local shell tool? [y/n] ").unwrap(); + + // A streaming response, an MCP server's stderr, a title generator: all + // of them keep producing while the user thinks about the question. + printer.println("assistant content"); + printer.eprintln("chrome"); + printer.flush(); + + assert_eq!( + *out.lock(), + "Run local shell tool? [y/n] ", + "only the widget writes while it owns the terminal" + ); + assert_eq!(*err.lock(), ""); + } + + printer.flush(); + assert_eq!( + *out.lock(), + "Run local shell tool? [y/n] assistant content\n" + ); + assert_eq!(*err.lock(), "chrome\n"); +} + +#[test] +fn a_nested_prompt_does_not_release_the_outer_hold() { + // A permission prompt holds its writer and opens the inline reply widget + // on top of it, so sessions nest. The terminal comes back when the + // outermost one gives it back, not the first. + let (printer, out, _err) = Printer::memory(OutputFormat::TextPretty); + + let outer = printer.prompt_writer(); + { + let _inner = printer.owned_prompt_writer(); + printer.println("assistant content"); + } + + printer.flush(); + assert_eq!(*out.lock(), "", "the outer session still owns the terminal"); + + drop(outer); + printer.flush(); + assert_eq!(*out.lock(), "assistant content\n"); +} + +#[test] +fn a_nested_acquisition_does_not_flush_held_output() { + let (printer, out, _err) = Printer::memory(OutputFormat::TextPretty); + + let _outer = printer.prompt_writer(); + printer.println("assistant content"); + + // Acquisition drains the queue instantly, which must not mean pushing held + // output onto the screen a widget is already drawing on. + let _inner = printer.owned_prompt_writer(); + + assert_eq!(*out.lock(), ""); +} + +#[test] +fn output_held_by_a_prompt_is_not_lost_at_shutdown() { + let (printer, out, _err) = Printer::memory(OutputFormat::TextPretty); + + let _prompt = printer.prompt_writer(); + printer.println("assistant content"); + printer.shutdown(); + + assert_eq!( + *out.lock(), + "assistant content\n", + "held output is deferred, not discarded" + ); +} + #[test] fn suspend_status_erases_before_it_returns() { let (printer, _out, err) = region_printer(); diff --git a/crates/jp_printer/src/region.rs b/crates/jp_printer/src/region.rs index 9b2a773a1..609b1fa65 100644 --- a/crates/jp_printer/src/region.rs +++ b/crates/jp_printer/src/region.rs @@ -838,6 +838,16 @@ impl RegionStack { } } + /// Whether the terminal is currently handed to a writer outside the + /// printer. + /// + /// A suspension is taken for exactly as long as someone else — a prompt + /// widget, an external `$EDITOR` — owns the cursor, so this answers both + /// "are the rows hidden" and "may ordinary output land". + pub const fn is_suspended(&self) -> bool { + self.suspensions > 0 + } + /// Record whether the persistent write that just landed left the cursor /// part-way along a row. /// From 4045f8573e699800bce098eb27b4be368a628f6b Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Tue, 8 Sep 2026 15:44:42 +0200 Subject: [PATCH 2/4] refactor(md, term, cli): Move ANSI stream machinery into `jp_term` `ansi` and `shade` move from `jp_md` to `jp_term`, along with the region-background types they interpret, which land in a new `jp_term::background`. Nothing about what they do is markdown: `segments` tokenizes an escape stream, `AnsiState` tracks what a stream left active, and `ShadedWriter` keeps a background showing across cursor rewrites and split escapes. `jp_md` gains a `jp_term` dependency and imports them like any other consumer. The move puts them where the printer can reach them. `jp_printer` depends on `jp_term` and deliberately not on `jp_md`, so shading a prompt meant either pulling a markdown renderer into the output layer or writing the state machine a second time. `DefaultBackground`, `BackgroundFill`, and `line_fill` leave `jp_md::format` for `jp_term::background`, so the types and the one place that interprets them travel together. `AnsiState`'s methods widen from `pub(crate)` to `pub` now that its callers are in another crate. Signed-off-by: Jean Mertz --- Cargo.lock | 1 + crates/jp_cli/src/cmd/query/tool/prompter.rs | 2 +- .../src/cmd/query/tool/prompter_tests.rs | 2 +- .../jp_cli/src/cmd/query/turn/coordinator.rs | 2 +- crates/jp_cli/src/render/chat.rs | 6 +- crates/jp_cli/src/render/tool.rs | 7 +-- crates/jp_cli/src/render/tool_tests.rs | 2 +- crates/jp_cli/src/render/turn_view.rs | 2 +- crates/jp_md/Cargo.toml | 2 + crates/jp_md/src/format.rs | 53 ++--------------- crates/jp_md/src/format_tests.rs | 2 + crates/jp_md/src/lib.rs | 2 - crates/jp_md/src/render.rs | 11 ++-- crates/jp_md/src/table.rs | 6 +- crates/jp_md/src/writer.rs | 12 ++-- crates/jp_md/src/writer_tests.rs | 1 - crates/{jp_md => jp_term}/src/ansi.rs | 40 ++++++++----- crates/{jp_md => jp_term}/src/ansi_tests.rs | 0 crates/jp_term/src/background.rs | 58 +++++++++++++++++++ crates/jp_term/src/lib.rs | 3 + crates/{jp_md => jp_term}/src/shade.rs | 13 ++--- crates/{jp_md => jp_term}/src/shade_tests.rs | 0 ...-configurable-markdown-element-coloring.md | 4 +- docs/rfd/091-printer-owned-status-region.md | 8 +-- 24 files changed, 132 insertions(+), 107 deletions(-) rename crates/{jp_md => jp_term}/src/ansi.rs (90%) rename crates/{jp_md => jp_term}/src/ansi_tests.rs (100%) create mode 100644 crates/jp_term/src/background.rs rename crates/{jp_md => jp_term}/src/shade.rs (96%) rename crates/{jp_md => jp_term}/src/shade_tests.rs (100%) diff --git a/Cargo.lock b/Cargo.lock index 7e9644be7..5d5d71440 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2572,6 +2572,7 @@ dependencies = [ "comrak", "glob", "insta", + "jp_term", "proptest", "syntect", "two-face", diff --git a/crates/jp_cli/src/cmd/query/tool/prompter.rs b/crates/jp_cli/src/cmd/query/tool/prompter.rs index b0f30f77e..229c17b50 100644 --- a/crates/jp_cli/src/cmd/query/tool/prompter.rs +++ b/crates/jp_cli/src/cmd/query/tool/prompter.rs @@ -22,8 +22,8 @@ use jp_conversation::event::SelectOption; use jp_editor::{EditOutcome, EditorBackend}; use jp_inquire::{InlineOption, ReplyEditMode, ReplyOutcome, prompt::PromptBackend}; use jp_llm::tool::executor::PermissionInfo; -use jp_md::{format::DefaultBackground, shade::ShadedWriter}; use jp_printer::{Printer, PromptWriter}; +use jp_term::{background::DefaultBackground, shade::ShadedWriter}; use jp_tool::AnswerType; use serde_json::Value; diff --git a/crates/jp_cli/src/cmd/query/tool/prompter_tests.rs b/crates/jp_cli/src/cmd/query/tool/prompter_tests.rs index b0a0d1117..70577d498 100644 --- a/crates/jp_cli/src/cmd/query/tool/prompter_tests.rs +++ b/crates/jp_cli/src/cmd/query/tool/prompter_tests.rs @@ -2,8 +2,8 @@ use std::{sync::Arc, time::Duration}; use jp_editor::MockEditorBackend; use jp_inquire::{ReplyOutcome, prompt::MockPromptBackend}; -use jp_md::format::BackgroundFill; use jp_printer::{OutputFormat, PrintableExt as _, SharedBuffer}; +use jp_term::background::BackgroundFill; use serde_json::json; use super::*; diff --git a/crates/jp_cli/src/cmd/query/turn/coordinator.rs b/crates/jp_cli/src/cmd/query/turn/coordinator.rs index 1c6051476..2f657ac4e 100644 --- a/crates/jp_cli/src/cmd/query/turn/coordinator.rs +++ b/crates/jp_cli/src/cmd/query/turn/coordinator.rs @@ -9,8 +9,8 @@ use jp_llm::{ event::{Event, EventPart, FinishReason}, event_builder::EventBuilder, }; -use jp_md::format::DefaultBackground; use jp_printer::Printer; +use jp_term::background::DefaultBackground; use crate::{ cmd::query::{interrupt::InterruptAction, stream::TurnView}, diff --git a/crates/jp_cli/src/render/chat.rs b/crates/jp_cli/src/render/chat.rs index 878d5001d..e302adc2a 100644 --- a/crates/jp_cli/src/render/chat.rs +++ b/crates/jp_cli/src/render/chat.rs @@ -38,13 +38,11 @@ use jp_config::style::{ use jp_conversation::event::ChatResponse; use jp_md::{ buffer::{Buffer, Event, Fixups}, - format::{ - BackgroundFill, CodeBlockState, DefaultBackground, Formatter, TerminalOptions, - render_separator, - }, + format::{CodeBlockState, Formatter, TerminalOptions, render_separator}, theme, }; use jp_printer::{OutputWidth, PrintableExt as _, Printer, RegionStyle, StatusRegion}; +use jp_term::background::{BackgroundFill, DefaultBackground}; use tracing::warn; /// The kind of content last pushed into the renderer. diff --git a/crates/jp_cli/src/render/tool.rs b/crates/jp_cli/src/render/tool.rs index e499ec5a2..1c71dd8d9 100644 --- a/crates/jp_cli/src/render/tool.rs +++ b/crates/jp_cli/src/render/tool.rs @@ -22,12 +22,9 @@ use jp_config::{ }; use jp_conversation::event::ToolCallResponse; use jp_llm::{CommandResult, run_tool_command, tool::InvocationContext}; -use jp_md::{ - format::{DefaultBackground, Formatter}, - shade::ShadedWriter, -}; +use jp_md::format::Formatter; use jp_printer::{ErrChannel, LineSink, OutputLines, RegionStyle, StatusRegion}; -use jp_term::osc::hyperlink; +use jp_term::{background::DefaultBackground, osc::hyperlink, shade::ShadedWriter}; use serde_json::{Map, Value}; use tokio_util::sync::CancellationToken; use tracing::warn; diff --git a/crates/jp_cli/src/render/tool_tests.rs b/crates/jp_cli/src/render/tool_tests.rs index d3651d50a..36e7c678d 100644 --- a/crates/jp_cli/src/render/tool_tests.rs +++ b/crates/jp_cli/src/render/tool_tests.rs @@ -7,8 +7,8 @@ use jp_config::{ style::stderr_rows::{RowCount, StderrRows}, }; use jp_conversation::event::ToolCallResponse; -use jp_md::format::{BackgroundFill, DefaultBackground}; use jp_printer::{ErrChannel, OutputFormat, Printer, SharedBuffer, TerminalCapability}; +use jp_term::background::{BackgroundFill, DefaultBackground}; use serde_json::{Map, Value}; use super::*; diff --git a/crates/jp_cli/src/render/turn_view.rs b/crates/jp_cli/src/render/turn_view.rs index b47c88b43..64dfbc1de 100644 --- a/crates/jp_cli/src/render/turn_view.rs +++ b/crates/jp_cli/src/render/turn_view.rs @@ -20,8 +20,8 @@ use std::sync::{ use jp_config::style::StyleConfig; use jp_conversation::event::{ChatRequest, ChatResponse}; -use jp_md::format::DefaultBackground; use jp_printer::Printer; +use jp_term::background::DefaultBackground; use super::{ChatRenderer, RenderFlow, StructuredRenderer}; diff --git a/crates/jp_md/Cargo.toml b/crates/jp_md/Cargo.toml index d5ed2805b..633a8ece9 100644 --- a/crates/jp_md/Cargo.toml +++ b/crates/jp_md/Cargo.toml @@ -13,6 +13,8 @@ repository.workspace = true version.workspace = true [dependencies] +jp_term = { workspace = true } + comrak = { workspace = true } syntect = { workspace = true, features = ["regex-fancy", "default-themes", "default-syntaxes"] } two-face = { workspace = true, features = ["syntect-fancy"] } diff --git a/crates/jp_md/src/format.rs b/crates/jp_md/src/format.rs index 01b9ee02e..87bde5260 100644 --- a/crates/jp_md/src/format.rs +++ b/crates/jp_md/src/format.rs @@ -1,17 +1,20 @@ //! Markdown formatting utilities. -use std::{borrow::Cow, fmt, sync::LazyLock}; +use std::{fmt, sync::LazyLock}; use comrak::{ Arena, nodes::{NodeList, NodeValue}, options::{Extension, ListStyleType, Render}, }; +use jp_term::{ + ansi::{self, AnsiState, Segment}, + background::{DefaultBackground, line_fill}, +}; use syntect::{highlighting::Theme, parsing::SyntaxSet}; use two_face::syntax; use crate::{ - ansi::{self, AnsiState, Segment}, render::{self, HrOptions, RenderOptions}, table::TableOptions, theme, @@ -33,19 +36,6 @@ const DEFAULT_WIDTH: usize = 80; /// Default maximum column width for tables. const DEFAULT_TABLE_MAX_COL_WIDTH: usize = 40; -/// How a default background color fills each line. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub enum BackgroundFill { - /// Fill to the last visible character on the line. - Content, - - /// Fill to a fixed column width (padding with spaces if needed). - Column(usize), - - /// Fill to the end of the terminal window via `\x1b[K`. - Terminal, -} - /// Controls how horizontal rules (`---`) are rendered in terminal output. #[derive(Debug, Clone, Copy, Default)] pub enum HrStyle { @@ -58,17 +48,6 @@ pub enum HrStyle { Line, } -/// A default background color applied to all content, with a fill mode -/// controlling how far it extends on each line. -#[derive(Debug, Clone, PartialEq, Eq)] -pub struct DefaultBackground { - /// SGR background parameter, e.g. `"48;5;236"` or `"48;2;80;73;69"`. - pub param: String, - - /// How far the background extends on each line. - pub fill: BackgroundFill, -} - /// Per-call options for [`Formatter::format_terminal_with`]. #[derive(Debug, Default, Clone, PartialEq, Eq)] pub struct TerminalOptions { @@ -484,28 +463,6 @@ fn ends_with_tight_list<'a>(root: &'a comrak::nodes::AstNode<'a>) -> bool { ) } -/// The text that extends a background from `column` to the end of the line. -/// -/// The one place [`BackgroundFill`] is interpreted. -/// Every writer that maintains a region background consults this so the three -/// modes cannot drift apart: `Content` adds nothing, `Terminal` defers to the -/// terminal's erase-to-end-of- line, and `Column` pads with real spaces — the -/// only form a host that lays out its own sub-window (an `fzf` preview pane) -/// renders, since it does not implement the erase. -/// -/// The caller is responsible for having the background active before writing -/// the result. -pub(crate) fn line_fill(fill: BackgroundFill, column: usize) -> Cow<'static, str> { - match fill { - BackgroundFill::Content => Cow::Borrowed(""), - BackgroundFill::Terminal => Cow::Borrowed("\x1b[K"), - BackgroundFill::Column(target) => match target.saturating_sub(column) { - 0 => Cow::Borrowed(""), - pad => Cow::Owned(" ".repeat(pad)), - }, - } -} - /// Render an inter-block separator (blank line) with optional background fill. /// Used between blocks and after closing code fences. /// diff --git a/crates/jp_md/src/format_tests.rs b/crates/jp_md/src/format_tests.rs index 3c15df71d..59460d306 100644 --- a/crates/jp_md/src/format_tests.rs +++ b/crates/jp_md/src/format_tests.rs @@ -1,3 +1,5 @@ +use jp_term::background::BackgroundFill; + use super::*; struct TestCase { diff --git a/crates/jp_md/src/lib.rs b/crates/jp_md/src/lib.rs index d611d0641..471599ea6 100644 --- a/crates/jp_md/src/lib.rs +++ b/crates/jp_md/src/lib.rs @@ -27,12 +27,10 @@ reason = "we don't host the docs, and use them mainly for LSP integration" )] -mod ansi; pub mod buffer; pub mod format; pub mod heading; mod render; -pub mod shade; mod table; pub mod theme; mod writer; diff --git a/crates/jp_md/src/render.rs b/crates/jp_md/src/render.rs index 39b500b0b..3935d9916 100644 --- a/crates/jp_md/src/render.rs +++ b/crates/jp_md/src/render.rs @@ -34,14 +34,17 @@ use comrak::{ NodeTaskItem, NodeValue, }, }; -use syntect::highlighting::Theme; - -use crate::{ +use jp_term::{ ansi::{ BG_END, BOLD_END, BOLD_START, FG_END, ITALIC_END, ITALIC_START, STRIKETHROUGH_END, STRIKETHROUGH_START, UNDERLINE_END, UNDERLINE_START, }, - format::{DefaultBackground, HrStyle, SYNTAXES}, + background::DefaultBackground, +}; +use syntect::highlighting::Theme; + +use crate::{ + format::{HrStyle, SYNTAXES}, table, writer::TerminalWriter, }; diff --git a/crates/jp_md/src/table.rs b/crates/jp_md/src/table.rs index 920662174..3d07616c6 100644 --- a/crates/jp_md/src/table.rs +++ b/crates/jp_md/src/table.rs @@ -22,13 +22,11 @@ use std::{cmp::min, fmt::Write as _}; use comrak::nodes::{NodeValue, TableAlignment}; +use jp_term::ansi::{self, AnsiState, RESET, Segment}; use unicode_segmentation::UnicodeSegmentation as _; use unicode_width::UnicodeWidthStr as _; -use crate::{ - ansi::{self, AnsiState, RESET, Segment}, - render::{RenderOptions, TerminalFormatter}, -}; +use crate::render::{RenderOptions, TerminalFormatter}; /// Type alias for comrak AST node references. type Node<'a> = &'a comrak::nodes::AstNode<'a>; diff --git a/crates/jp_md/src/writer.rs b/crates/jp_md/src/writer.rs index 70e15cc6c..9c0a01328 100644 --- a/crates/jp_md/src/writer.rs +++ b/crates/jp_md/src/writer.rs @@ -17,11 +17,13 @@ use std::{ fmt::{self, Write}, }; -use crate::{ +use jp_term::{ ansi::{self, AnsiState, RESET, Segment}, - format::{self, BackgroundFill, DefaultBackground}, + background::{BackgroundFill, DefaultBackground, line_fill}, }; +use crate::format; + /// ANSI-aware terminal writer with word-wrapping support. #[expect(clippy::struct_excessive_bools)] pub struct TerminalWriter<'w> { @@ -432,8 +434,8 @@ impl<'w> TerminalWriter<'w> { /// Extend the active background to the end of the current line. /// - /// The fill itself comes from [`format::line_fill`], the single - /// interpretation of [`BackgroundFill`]. + /// The fill itself comes from [`line_fill`], the single interpretation of + /// [`BackgroundFill`]. /// The background escape always precedes it so a temporary background /// (inline code, say) can't bleed into the fill. /// @@ -444,7 +446,7 @@ impl<'w> TerminalWriter<'w> { return Ok(()); }; - let fill = format::line_fill(bg.fill, self.column); + let fill = line_fill(bg.fill, self.column); if fill.is_empty() { return Ok(()); } diff --git a/crates/jp_md/src/writer_tests.rs b/crates/jp_md/src/writer_tests.rs index 27a65046c..070f9ac53 100644 --- a/crates/jp_md/src/writer_tests.rs +++ b/crates/jp_md/src/writer_tests.rs @@ -1,5 +1,4 @@ use super::*; -use crate::format::{BackgroundFill, DefaultBackground}; /// Regression test: a 1-byte prefix used to underflow in `write_prefix` because /// `prefix.len() - 2` wraps to `usize::MAX`. diff --git a/crates/jp_md/src/ansi.rs b/crates/jp_term/src/ansi.rs similarity index 90% rename from crates/jp_md/src/ansi.rs rename to crates/jp_term/src/ansi.rs index 224185f29..16b1ba4e6 100644 --- a/crates/jp_md/src/ansi.rs +++ b/crates/jp_term/src/ansi.rs @@ -1,8 +1,11 @@ -//! Shared ANSI SGR escape constants and state tracking. +//! ANSI SGR escape constants, state tracking, and escape-aware measurement. //! -//! This module provides the escape sequences, state tracking, and visual width -//! computation used by both the terminal renderer (`render.rs`) and the table -//! formatter (`table.rs`). +//! [`segments`] is the tokenizer everything else here is built on: it splits a +//! byte stream into visible text and complete escape sequences. +//! [`AnsiState`] tracks which attributes a stream has left active, so a writer +//! can close them at a line break and re-open them on the next line. +//! [`visual_width`] and [`advance_column`] measure text as the display lays it +//! out, skipping escapes and advancing a tab to its stop. use unicode_width::UnicodeWidthStr as _; @@ -47,9 +50,10 @@ pub const RESET: &str = "\x1b[0m"; /// Tracks which ANSI SGR attributes are currently active. /// -/// Used to close formatting at line breaks and re-open it on the next line, -/// both for the terminal renderer's incremental wrapping and the table -/// formatter's batch wrapping. +/// Feed it every escape a stream emits, and it answers what a line break has to +/// close and what the next line has to re-open. +/// Attributes combined into one escape are tracked individually, so +/// `\x1b[1;48;5;236m` registers as both bold and a background. #[derive(Debug, Clone, Default)] #[expect(clippy::struct_excessive_bools)] pub struct AnsiState { @@ -80,7 +84,8 @@ pub struct AnsiState { impl AnsiState { /// Returns `true` if any attribute is currently active. - pub(crate) const fn is_active(&self) -> bool { + #[must_use] + pub const fn is_active(&self) -> bool { self.bold || self.italic || self.underline @@ -102,7 +107,7 @@ impl AnsiState { /// Returns `true` when the escape resets all attributes or sets/clears the /// background — the signal a default-background overlay uses to know it /// must re-assert its fill after the escape is forwarded. - pub(crate) fn update(&mut self, esc: &str) -> bool { + pub fn update(&mut self, esc: &str) -> bool { let Some(params) = esc.strip_prefix("\x1b[").and_then(|s| s.strip_suffix('m')) else { return false; }; @@ -165,7 +170,7 @@ impl AnsiState { } /// Update state by scanning all ANSI escape sequences in `s`. - pub(crate) fn update_from_str(&mut self, s: &str) { + pub fn update_from_str(&mut self, s: &str) { for segment in segments(s) { if let Segment::Escape(esc) = segment { let _affects_background = self.update(esc); @@ -174,7 +179,8 @@ impl AnsiState { } /// Builds a string that re-activates all currently active attributes. - pub(crate) fn restore_sequence(&self) -> String { + #[must_use] + pub fn restore_sequence(&self) -> String { let mut s = String::new(); if self.bold { s.push_str(BOLD_START); @@ -232,6 +238,7 @@ fn consume_color<'a, I: Iterator>(prefix: &str, tokens: &mut I) /// SGR is the only family [`AnsiState`] tracks, so this doubles as the test for /// whether an escape's effect can be closed with [`RESET`] or re-opened after a /// line break. +#[must_use] pub fn is_sgr(esc: &str) -> bool { esc.starts_with("\x1b[") && esc.ends_with('m') } @@ -250,10 +257,10 @@ pub enum Segment<'a> { /// Split `s` into visible-text runs and ANSI escape sequences. /// /// An escape sequence runs from `\x1b` through the first ASCII letter or `~` — -/// sufficient for the SGR/CSI sequences this crate emits and consumes. -/// This is the single tokenizer for every escape-aware routine in the crate -/// (width computation, state tracking, table wrapping), so the termination rule -/// cannot drift between call sites. +/// sufficient for the SGR/CSI sequences JP emits and consumes. +/// This is the single tokenizer every escape-aware routine goes through, so the +/// termination rule cannot drift between call sites. +#[must_use] pub const fn segments(s: &str) -> Segments<'_> { Segments { rest: s } } @@ -328,6 +335,7 @@ fn osc_terminator_end(body: &str) -> Option { /// Grapheme cluster boundaries are a property of this text, not of the escape /// separated runs it was built from, so anything measuring or cutting on /// cluster boundaries has to work from here. +#[must_use] pub fn visible_text(s: &str) -> String { let mut plain = String::new(); for segment in segments(s) { @@ -343,6 +351,7 @@ pub fn visible_text(s: &str) -> String { /// A tab counts as a single column. /// Use [`advance_column`] where the resulting cursor position matters, since a /// tab moves the cursor to the next tab stop instead. +#[must_use] pub fn visual_width(s: &str) -> usize { visible_text(s).width() } @@ -353,6 +362,7 @@ pub fn visual_width(s: &str) -> usize { /// of [`TAB_STOP`], and a carriage return returns to column 0 — the positions /// the display actually arrives at, so text padded to a fixed column lands /// there instead of overshooting. +#[must_use] pub fn advance_column(column: usize, s: &str) -> usize { let plain = visible_text(s); let mut column = column; diff --git a/crates/jp_md/src/ansi_tests.rs b/crates/jp_term/src/ansi_tests.rs similarity index 100% rename from crates/jp_md/src/ansi_tests.rs rename to crates/jp_term/src/ansi_tests.rs diff --git a/crates/jp_term/src/background.rs b/crates/jp_term/src/background.rs new file mode 100644 index 000000000..be8b03fba --- /dev/null +++ b/crates/jp_term/src/background.rs @@ -0,0 +1,58 @@ +//! A background colour applied to a region of terminal output. +//! +//! A region background is the fill behind a run of rows — the shading a +//! reasoning block sits in, the tint a status row is drawn against. +//! It is described by two things: the SGR parameter that sets the colour, and +//! how far along each row the colour extends. +//! +//! [`line_fill`] is the one place that second question is answered, so every +//! writer that maintains a background agrees on what a filled row looks like. + +use std::borrow::Cow; + +/// How a default background colour fills each line. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum BackgroundFill { + /// Fill to the last visible character on the line. + Content, + + /// Fill to a fixed column width (padding with spaces if needed). + Column(usize), + + /// Fill to the end of the terminal window via `\x1b[K`. + Terminal, +} + +/// A default background colour applied to all content, with a fill mode +/// controlling how far it extends on each line. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct DefaultBackground { + /// SGR background parameter, e.g. `"48;5;236"` or `"48;2;80;73;69"`. + pub param: String, + + /// How far the background extends on each line. + pub fill: BackgroundFill, +} + +/// The text that extends a background from `column` to the end of the line. +/// +/// The one place [`BackgroundFill`] is interpreted. +/// Every writer that maintains a region background consults this so the three +/// modes cannot drift apart: `Content` adds nothing, `Terminal` defers to the +/// terminal's erase-to-end-of-line, and `Column` pads with real spaces — the +/// only form a host that lays out its own sub-window (an `fzf` preview pane) +/// renders, since it does not implement the erase. +/// +/// The caller is responsible for having the background active before writing +/// the result. +#[must_use] +pub fn line_fill(fill: BackgroundFill, column: usize) -> Cow<'static, str> { + match fill { + BackgroundFill::Content => Cow::Borrowed(""), + BackgroundFill::Terminal => Cow::Borrowed("\x1b[K"), + BackgroundFill::Column(target) => match target.saturating_sub(column) { + 0 => Cow::Borrowed(""), + pad => Cow::Owned(" ".repeat(pad)), + }, + } +} diff --git a/crates/jp_term/src/lib.rs b/crates/jp_term/src/lib.rs index 4f79c0fb8..a7c90f05e 100644 --- a/crates/jp_term/src/lib.rs +++ b/crates/jp_term/src/lib.rs @@ -1,5 +1,8 @@ #![allow(clippy::print_stdout, clippy::print_stderr)] +pub mod ansi; +pub mod background; pub mod osc; +pub mod shade; pub mod table; pub mod width; diff --git a/crates/jp_md/src/shade.rs b/crates/jp_term/src/shade.rs similarity index 96% rename from crates/jp_md/src/shade.rs rename to crates/jp_term/src/shade.rs index e97b7e73f..1ec26fa57 100644 --- a/crates/jp_md/src/shade.rs +++ b/crates/jp_term/src/shade.rs @@ -9,18 +9,15 @@ //! no row past the region is painted. //! [`shade`] is the buffer-at-a-time convenience built on the same core. //! -//! Unlike the line-oriented [`apply_line_background`], the writer holds its -//! state across `write_str` calls, so it can keep `B` active across cursor -//! rewrites and escape sequences split between writes — the cases a pure -//! per-line transform cannot express. -//! -//! [`apply_line_background`]: crate::format::apply_line_background +//! The writer holds its state across `write_str` calls, so `B` survives a +//! cursor rewrite or an escape sequence split between two writes — the cases a +//! per-line transform over a finished string cannot express. use std::fmt::{self, Write}; use crate::{ ansi::{self, AnsiState, Segment, is_sgr}, - format::{self, BackgroundFill, DefaultBackground}, + background::{BackgroundFill, DefaultBackground, line_fill}, }; /// Wraps a writer and maintains a default-background invariant across the byte @@ -199,7 +196,7 @@ impl ShadedWriter { /// Fill the rest of the current line with the active background, ahead of a /// newline. fn fill_line(&mut self) -> fmt::Result { - let fill = format::line_fill(self.fill, self.column); + let fill = line_fill(self.fill, self.column); if fill.is_empty() { return Ok(()); } diff --git a/crates/jp_md/src/shade_tests.rs b/crates/jp_term/src/shade_tests.rs similarity index 100% rename from crates/jp_md/src/shade_tests.rs rename to crates/jp_term/src/shade_tests.rs diff --git a/docs/rfd/084-configurable-markdown-element-coloring.md b/docs/rfd/084-configurable-markdown-element-coloring.md index d8cbbecad..0dbc98723 100644 --- a/docs/rfd/084-configurable-markdown-element-coloring.md +++ b/docs/rfd/084-configurable-markdown-element-coloring.md @@ -369,7 +369,7 @@ build themes against it. ### Renderer changes -`AnsiState` (`crates/jp_md/src/ansi.rs`) grows an `intensity: Intensity` field +`AnsiState` (`crates/jp_term/src/ansi.rs`) grows an `intensity: Intensity` field in place of its existing `bold: bool`. `restore_sequence` re-emits SGR 1 or SGR 2 as appropriate after any SGR 22 transition, so wrap-break and pop-style restoration handle dim with the same @@ -778,7 +778,7 @@ Depends on Phase 2. This RFD extends that renderer's styling surface. - `jp_md::render::TerminalFormatter` — the AST walker whose hard-coded SGR calls this RFD replaces. -- `jp_md::ansi::AnsiState` — the existing state-tracking primitive that the +- `jp_term::ansi::AnsiState` — the existing state-tracking primitive that the proposed writer stack builds on. - `termimad`'s `MadSkin` — referenced as schema inspiration; not adopted. diff --git a/docs/rfd/091-printer-owned-status-region.md b/docs/rfd/091-printer-owned-status-region.md index 4ac4a4d13..2fa545b67 100644 --- a/docs/rfd/091-printer-owned-status-region.md +++ b/docs/rfd/091-printer-owned-status-region.md @@ -273,7 +273,7 @@ A line that leaves an attribute open is terminated with a reset, so child state cannot bleed into JP's own chrome below it. This is the policy `jp_md::table` already applies when truncating cells (retain -SGR, drop the rest, close with a reset), and `jp_md::ansi::is_sgr` is the same +SGR, drop the rest, close with a reset), and `jp_term::ansi::is_sgr` is the same predicate. In `jp_printer` it is a second policy over the existing `vte` parser that backs `AnsiStripper` — same crate, same parser, no new dependency. @@ -347,7 +347,7 @@ temp and progress rows as part of it: while a reasoning region with background produced by cursor-relative rewrites, and including the `\x1b[K` that erases them, which fills with whatever background is active when it runs. `ToolRenderer` holds that invariant today by routing its writes through -`jp_md::shade::ShadedWriter`. +`jp_term::shade::ShadedWriter`. A worker that draws and erases those rows itself, knowing nothing about the reasoning region, would punch an unshaded hole in the middle of a shaded one — the exact gap RFD 095 closed. @@ -1068,12 +1068,12 @@ contracts commits to. replaces. - `crates/jp_cli/src/cmd/query/stream/retry.rs` — `notify` and `clear_line`, the ninth hand-rolled mechanism. -- `crates/jp_md/src/shade.rs` — `ShadedWriter`, which holds the background +- `crates/jp_term/src/shade.rs` — `ShadedWriter`, which holds the background invariant for tool chrome today. - `crates/jp_printer/src/printer.rs` — the worker loop this RFD extends. - `crates/jp_printer/src/ansi.rs` — the `vte`-based `AnsiStripper` the SGR allowlist extends. -- `crates/jp_md/src/ansi.rs` — `is_sgr`, the predicate the allowlist reuses, +- `crates/jp_term/src/ansi.rs` — `is_sgr`, the predicate the allowlist reuses, and the retain-SGR-drop-the-rest precedent in `jp_md/src/table.rs`. - `crates/jp_mcp/src/client.rs` — `spawn_stderr_forwarder`, the stderr ring buffer, and `StartupSet`. From c7783efde736d668760b58f39d1ba7b24a40cf3c Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Tue, 8 Sep 2026 15:57:11 +0200 Subject: [PATCH 3/4] fix(printer, cli): Shade every prompt with the reasoning background A tool call made from a reasoning block sits inside that block's shading, and its prompts now do too. The approval prompt and the tool's questions were already shaded; the inline reply widget, reached by `e` to edit arguments or `r` to skip with a reason, was not, and cut an unshaded hole in the middle of the block. The printer knows which background prompts are drawn against, and applies it to every prompt writer it hands out. Whoever owns the region names it once and no prompt site has to remember: `Printer::set_prompt_background` replaces the copy `ToolPrompter` was keeping, and the shaded-writer plumbing that lived beside it in `jp_cli` is gone. The owned prompt writer, which is the one the reply widget takes, is covered by the same code as the borrowed one rather than needing its own. Closes: T-0fg8cjs Signed-off-by: Jean Mertz --- .../jp_cli/src/cmd/query/tool/coordinator.rs | 13 +- .../src/cmd/query/tool/coordinator_tests.rs | 9 +- crates/jp_cli/src/cmd/query/tool/prompter.rs | 101 +-------- .../src/cmd/query/tool/prompter_tests.rs | 128 +++--------- crates/jp_cli/src/cmd/query/turn_loop.rs | 2 + crates/jp_inquire/src/prompt.rs | 12 +- crates/jp_printer/src/printer.rs | 191 +++++++++++++++--- crates/jp_printer/src/printer_tests.rs | 119 +++++++++++ 8 files changed, 340 insertions(+), 235 deletions(-) diff --git a/crates/jp_cli/src/cmd/query/tool/coordinator.rs b/crates/jp_cli/src/cmd/query/tool/coordinator.rs index 7c6908fde..a5ba55b28 100644 --- a/crates/jp_cli/src/cmd/query/tool/coordinator.rs +++ b/crates/jp_cli/src/cmd/query/tool/coordinator.rs @@ -553,13 +553,14 @@ impl ToolCoordinator { interactive: bool, turn_state: &mut TurnState, tool_renderer: &ToolRenderer, + printer: &Printer, ) -> ToolCallDecision { // A tool call reached from a reasoning block sits inside that block's - // shading, and a prompt is a visual row like any other (RFD 095). The - // prompter writes through the printer rather than through the - // renderer, so it has to be handed the background rather than finding - // it. - prompter.set_background(tool_renderer.current_region()); + // shading, and a prompt is a visual row like any other (RFD 095). This + // is the one place holding both the renderer that owns the region and + // the printer that draws the prompts, and the region is per tool, so + // the read happens here rather than at the tool-call boundary. + printer.set_prompt_background(tool_renderer.current_region()); // Step 1: decide. let decision = self.decide_permission(executor, interactive, turn_state); @@ -886,6 +887,7 @@ impl ToolCoordinator { interactive: bool, turn_state: &mut TurnState, tool_renderer: &ToolRenderer, + printer: &Printer, ) -> ( Vec<(usize, Box)>, Vec<(usize, ToolCallResponse)>, @@ -905,6 +907,7 @@ impl ToolCoordinator { interactive, turn_state, tool_renderer, + printer, ) .await; diff --git a/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs b/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs index b0565e273..f0650f3c6 100644 --- a/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs +++ b/crates/jp_cli/src/cmd/query/tool/coordinator_tests.rs @@ -493,7 +493,14 @@ async fn test_resolve_tool_call_decision_invalidates_prerender_on_edit() { let mut turn_state = TurnState::default(); let decision = coordinator - .resolve_tool_call_decision(executor, &prompter, true, &mut turn_state, &tool_renderer) + .resolve_tool_call_decision( + executor, + &prompter, + true, + &mut turn_state, + &tool_renderer, + &printer, + ) .await; match decision { diff --git a/crates/jp_cli/src/cmd/query/tool/prompter.rs b/crates/jp_cli/src/cmd/query/tool/prompter.rs index 229c17b50..b316ae88a 100644 --- a/crates/jp_cli/src/cmd/query/tool/prompter.rs +++ b/crates/jp_cli/src/cmd/query/tool/prompter.rs @@ -10,11 +10,7 @@ //! imperative shell" principle. //! The `jp_llm` crate remains pure. -use std::{ - fmt::Write as _, - io::{self, Write as _}, - sync::{Arc, Mutex, PoisonError}, -}; +use std::{io::Write as _, sync::Arc}; use crossterm::style::Stylize as _; use jp_config::conversation::tool::{RunMode, ToolSource}; @@ -23,7 +19,6 @@ use jp_editor::{EditOutcome, EditorBackend}; use jp_inquire::{InlineOption, ReplyEditMode, ReplyOutcome, prompt::PromptBackend}; use jp_llm::tool::executor::PermissionInfo; use jp_printer::{Printer, PromptWriter}; -use jp_term::{background::DefaultBackground, shade::ShadedWriter}; use jp_tool::AnswerType; use serde_json::Value; @@ -91,63 +86,6 @@ pub struct ToolPrompter { edit_mode: ReplyEditMode, printer: Arc, - - /// The reasoning-region background prompts are drawn against. - /// - /// Set per tool call by the coordinator, which is the only place that knows - /// both this prompter and the renderer holding the region. - background: Mutex>, -} - -/// A prompt writer carrying the reasoning background, when one is open. -/// -/// A prompt drawn inside a reasoning block shows that block's background like -/// every other row (RFD 095), and a widget owns its own cursor — it rewrites -/// its line with `\r\x1b[K` on each keystroke, and that erase fills with -/// whatever background is active. -/// [`ShadedWriter`] is what keeps the fill right across the widget's own -/// escapes, including the resets it emits mid-line. -enum PromptCanvas<'a> { - /// No reasoning block is open; writes pass straight through. - Plain(PromptWriter<'a>), - - /// Writes are shaded with the open block's background. - Shaded(Box>>), -} - -impl io::Write for PromptCanvas<'_> { - fn write(&mut self, buf: &[u8]) -> io::Result { - match self { - Self::Plain(writer) => io::Write::write(writer, buf), - Self::Shaded(writer) => { - let text = str::from_utf8(buf).map_err(io::Error::other)?; - writer.write_str(text).map_err(io::Error::other)?; - Ok(buf.len()) - } - } - } - - /// A widget flushes before it reads a key, and on this path a flush is a - /// barrier rather than a buffer drain: it blocks until the printer's worker - /// has written everything queued ahead of it. - /// Shading decorates the writer and must not swallow that. - fn flush(&mut self) -> io::Result<()> { - match self { - Self::Plain(writer) => io::Write::flush(writer), - Self::Shaded(writer) => io::Write::flush(writer.get_mut()), - } - } -} - -impl Drop for PromptCanvas<'_> { - /// Close the background here rather than at the end of each prompt: a - /// prompt can end by cancellation or by an error, and a background left - /// open paints everything printed after it. - fn drop(&mut self) { - if let Self::Shaded(writer) = self { - let _err = writer.finish(); - } - } } impl ToolPrompter { @@ -166,34 +104,6 @@ impl ToolPrompter { prompt_backend, edit_mode, printer, - background: Mutex::new(None), - } - } - - /// Draw prompts against `background` until it is replaced. - /// - /// `None` while no reasoning block is open, which is the common case. - pub(crate) fn set_background(&self, background: Option) { - *self - .background - .lock() - .unwrap_or_else(PoisonError::into_inner) = background; - } - - /// A prompt writer shaded with the open reasoning block, if there is one. - fn canvas(&self) -> PromptCanvas<'_> { - let writer = self.printer.prompt_writer(); - - match self - .background - .lock() - .unwrap_or_else(PoisonError::into_inner) - .as_ref() - { - Some(background) => { - PromptCanvas::Shaded(Box::new(ShadedWriter::new(writer, background))) - } - None => PromptCanvas::Plain(writer), } } @@ -205,7 +115,6 @@ impl ToolPrompter { prompt_backend: Arc, ) -> Self { Self { - background: Mutex::new(None), editor, prompt_backend, edit_mode: ReplyEditMode::Emacs, @@ -280,7 +189,7 @@ impl ToolPrompter { let inline_options = select_options_to_inline(&Self::permission_options()); - let mut writer = self.canvas(); + let mut writer = self.printer.prompt_writer(); match self .prompt_backend @@ -498,7 +407,7 @@ impl ToolPrompter { /// - `Ok(true)` if user confirms delivery /// - `Ok(false)` if user skips delivery pub fn prompt_result_confirmation(&self, tool_name: &str) -> Result { - let mut writer = self.canvas(); + let mut writer = self.printer.prompt_writer(); let question = format!("Deliver {} result to assistant?", tool_name.yellow().bold()); @@ -539,7 +448,7 @@ impl ToolPrompter { /// A `QuestionResult` containing the answer and `persist_level` which /// indicates whether the answer should be remembered for this turn. pub fn prompt_question(&self, question: &jp_tool::Question) -> Result { - let mut writer = self.canvas(); + let mut writer = self.printer.prompt_writer(); if let Some(pre_amble) = &question.pre_amble { writeln!(writer, "{pre_amble}")?; @@ -600,7 +509,7 @@ impl ToolPrompter { fn prompt_boolean_git_style( &self, question: &jp_tool::Question, - writer: &mut PromptCanvas<'_>, + writer: &mut PromptWriter<'_>, ) -> Result { let options = vec![ InlineOption::new('y', "yes, just this once"), diff --git a/crates/jp_cli/src/cmd/query/tool/prompter_tests.rs b/crates/jp_cli/src/cmd/query/tool/prompter_tests.rs index 70577d498..50d3c66a7 100644 --- a/crates/jp_cli/src/cmd/query/tool/prompter_tests.rs +++ b/crates/jp_cli/src/cmd/query/tool/prompter_tests.rs @@ -1,9 +1,9 @@ -use std::{sync::Arc, time::Duration}; +use std::sync::Arc; use jp_editor::MockEditorBackend; use jp_inquire::{ReplyOutcome, prompt::MockPromptBackend}; -use jp_printer::{OutputFormat, PrintableExt as _, SharedBuffer}; -use jp_term::background::BackgroundFill; +use jp_printer::{OutputFormat, SharedBuffer}; +use jp_term::background::{BackgroundFill, DefaultBackground}; use serde_json::json; use super::*; @@ -32,65 +32,16 @@ fn terminal_region() -> DefaultBackground { } #[test] -fn a_prompt_inside_a_reasoning_block_carries_its_background() { - let (prompter, out) = prompter_with_output(MockPromptBackend::new()); - prompter.set_background(Some(terminal_region())); - - { - let mut canvas = prompter.canvas(); - write!(canvas, "Run local shell tool?").unwrap(); - } - prompter.printer.flush(); - - // The background is asserted before the text and closed once the widget is - // done with the terminal, so the row is shaded and nothing after it is. - assert_eq!(*out.lock(), "\x1b[48;5;236mRun local shell tool?\x1b[49m"); -} - -#[test] -fn a_prompt_outside_a_reasoning_block_is_unshaded() { - let (prompter, out) = prompter_with_output(MockPromptBackend::new()); - - { - let mut canvas = prompter.canvas(); - write!(canvas, "Run local shell tool?").unwrap(); - } - prompter.printer.flush(); - - assert_eq!(*out.lock(), "Run local shell tool?"); -} - -#[test] -fn a_cancelled_prompt_still_closes_its_background() { - // A widget can end by `Ctrl+C` or by an error, neither of which returns - // through the normal path. The close lives in `Drop` so the background - // cannot outlive the prompt and paint whatever is printed next. - let (prompter, out) = prompter_with_output(MockPromptBackend::new()); - prompter.set_background(Some(terminal_region())); - - { - let mut canvas = prompter.canvas(); - write!(canvas, "Deliver result?").unwrap(); - // No further writes: the prompt is abandoned mid-session. - } - prompter.printer.flush(); - - let rendered = out.lock().clone(); - assert!( - rendered.ends_with("\x1b[49m"), - "an abandoned prompt must still close its background, got {rendered:?}" - ); -} - -#[test] -fn prompting_a_question_shades_through_the_canvas() { - // The tests above drive the canvas directly, so they hold even if a prompt - // method still reached for a bare prompt writer. This one goes through - // `prompt_question`, whose pre-amble is the one part of a prompt written by - // the prompter rather than by the widget. +fn prompting_a_question_carries_the_reasoning_background() { + // `jp_printer` owns the shading and pins its escapes; what this covers is + // that a real prompt method reaches it, rather than building a writer of + // its own. The pre-amble is the one part of a prompt written by the + // prompter rather than by the widget. let (prompter, out) = prompter_with_output(MockPromptBackend::new().with_inline_responses(['y'])); - prompter.set_background(Some(terminal_region())); + prompter + .printer + .set_prompt_background(Some(terminal_region())); let mut question = jp_tool::Question::boolean("confirm", "Proceed?").expect("valid question"); question.pre_amble = Some("About to run a shell command".to_owned()); @@ -110,53 +61,30 @@ fn prompting_a_question_shades_through_the_canvas() { } #[test] -fn flushing_a_shaded_canvas_waits_for_the_printer() { - // A widget flushes its writer before it reads a key, and on this path a - // flush is a barrier rather than a buffer drain: it is what makes the bytes - // have landed before the widget takes the cursor and the terminal's mode. - // Shading is a decoration over that writer and has no business swallowing - // it. - let (prompter, out) = prompter_with_output(MockPromptBackend::new()); - prompter.set_background(Some(terminal_region())); - - let mut canvas = prompter.canvas(); - - // Queued after acquisition drained the printer, and slow enough that the - // worker is certainly still inside it: without a real barrier the prompt's - // own text cannot have reached the terminal yet. +fn the_inline_reply_widget_carries_the_reasoning_background() { + // `edit_result` runs the inline reply widget, which owns its output stream + // and so takes `owned_prompt_writer` rather than the borrowed one. It is + // reached by `e` from a permission prompt, inside the same block. + let (prompter, out) = prompter_with_output( + MockPromptBackend::new().with_reply_outcomes([ReplyOutcome::Submit("edited".into())]), + ); prompter .printer - .print("slow".typewriter(Duration::from_millis(100))); + .set_prompt_background(Some(terminal_region())); - write!(canvas, "Run local shell tool?").unwrap(); - canvas.flush().unwrap(); + prompter + .edit_result("original") + .expect("the mock submits the edit"); + prompter.printer.flush(); - // Deliberately no `printer.flush()`, which is the assertion. + let rendered = out.lock().clone(); assert!( - out.lock().contains("Run local shell tool?"), - "flushing the canvas must drain the printer, got {:?}", - *out.lock() + rendered.starts_with("\x1b[48;5;236m"), + "the widget's writes must open under the background, got {rendered:?}" ); -} - -#[test] -fn clearing_the_background_unshades_later_prompts() { - let (prompter, out) = prompter_with_output(MockPromptBackend::new()); - - prompter.set_background(Some(terminal_region())); - drop(prompter.canvas()); - prompter.set_background(None); - - { - let mut canvas = prompter.canvas(); - write!(canvas, "after").unwrap(); - } - prompter.printer.flush(); - assert!( - out.lock().ends_with("after"), - "a prompt after the block closed carries no background: {:?}", - *out.lock() + rendered.ends_with("\x1b[49m"), + "and close it when the widget gives the terminal back, got {rendered:?}" ); } diff --git a/crates/jp_cli/src/cmd/query/turn_loop.rs b/crates/jp_cli/src/cmd/query/turn_loop.rs index 85c0072f3..37068c68e 100644 --- a/crates/jp_cli/src/cmd/query/turn_loop.rs +++ b/crates/jp_cli/src/cmd/query/turn_loop.rs @@ -637,6 +637,7 @@ pub(super) async fn run_turn_loop( interactive, &mut turn_state, &tool_renderer, + &printer, ) .await; @@ -729,6 +730,7 @@ pub(super) async fn run_turn_loop( interactive, &mut turn_state, &tool_renderer, + &printer, ) .await; diff --git a/crates/jp_inquire/src/prompt.rs b/crates/jp_inquire/src/prompt.rs index b6be20e20..c41d65281 100644 --- a/crates/jp_inquire/src/prompt.rs +++ b/crates/jp_inquire/src/prompt.rs @@ -287,15 +287,23 @@ impl PromptBackend for MockPromptBackend { .ok_or(InquireError::OperationCanceled) } + /// Writes `message` to `output` before answering. + /// + /// The real widget draws on the stream it is handed, so a mock that never + /// touched it could not tell a caller's writer apart from any other — a + /// test asserting on what a prompt renders would pass against no prompt at + /// all. fn inline_reply( &self, - _message: &str, + message: &str, _initial_text: &str, _edit_mode: ReplyEditMode, _editor_escape: bool, _help: Option<&str>, - _output: Box, + mut output: Box, ) -> Result { + write!(output, "{message}")?; + self.reply_outcomes .lock() .pop_front() diff --git a/crates/jp_printer/src/printer.rs b/crates/jp_printer/src/printer.rs index 0474366ed..5cad8d2a7 100644 --- a/crates/jp_printer/src/printer.rs +++ b/crates/jp_printer/src/printer.rs @@ -13,6 +13,7 @@ use std::{ time::{Duration, Instant}, }; +use jp_term::{background::DefaultBackground, shade::ShadedWriter}; use parking_lot::{Condvar, Mutex}; use tracing::{debug, error}; @@ -110,6 +111,12 @@ pub struct Printer { /// even a currently-running typewriter task wakes up immediately. /// Cleared by the worker after processing `FlushInstant`. delay_control: Arc, + + /// The region background prompts are drawn against, when one is open. + /// + /// Shared across clones: whoever owns the region sets it once, and every + /// prompt taken from any handle picks it up. + prompt_background: Arc>>, } impl Clone for Printer { @@ -123,6 +130,7 @@ impl Clone for Printer { terminal: self.terminal, worker_handle: self.worker_handle.clone(), delay_control: self.delay_control.clone(), + prompt_background: self.prompt_background.clone(), } } } @@ -174,6 +182,7 @@ impl Printer { terminal: TerminalCapability::default(), worker_handle: Arc::new(Mutex::new(Some(handle))), delay_control, + prompt_background: Arc::new(Mutex::new(None)), } } @@ -550,19 +559,36 @@ impl Printer { /// erased, and no region redraws until the writer drops: a prompt session /// is a run of small writes with the widget owning the cursor in between, /// and anything landing between them corrupts it. + /// + /// Writes carry whatever background [`Self::set_prompt_background`] last + /// named. #[must_use] pub fn prompt_writer(&self) -> PromptWriter<'_> { + let writer = PrinterWriter { + printer: self, + target: self.prompt_target(), + origin: PrintOrigin::Prompt, + }; + let suspension = self.begin_prompt_session(); + PromptWriter { - writer: PrinterWriter { - printer: self, - target: self.prompt_target(), - origin: PrintOrigin::Prompt, - }, - _suspension: self.begin_prompt_session(), + writer: Canvas::new(writer, self.prompt_background.lock().as_ref()), + _suspension: suspension, _trace: PromptTrace::open(), } } + /// Draw prompts against `background` until it is replaced. + /// + /// `None` while no shaded region is open, which is the common case. + /// A prompt drawn inside one is a visual row like any other and shows the + /// same background to the right edge (RFD 095); whoever owns the region + /// names it here, and every prompt taken afterwards picks it up without its + /// call site knowing a region exists. + pub fn set_prompt_background(&self, background: Option) { + *self.prompt_background.lock() = background; + } + /// Print a line on the prompt stream. /// /// For the context a question needs to be answerable — the identity of a @@ -594,10 +620,15 @@ impl Printer { /// ordered with the printer's other output. #[must_use] pub fn owned_prompt_writer(&self) -> Box { - Box::new(OwnedPrinterWriter { + let writer = OwnedPrinterWriter { tx: self.tx.clone(), target: self.prompt_target(), - _suspension: self.begin_prompt_session(), + }; + let suspension = self.begin_prompt_session(); + + Box::new(OwnedPromptWriter { + writer: Canvas::new(writer, self.prompt_background.lock().as_ref()), + _suspension: suspension, _trace: PromptTrace::open(), }) } @@ -864,23 +895,85 @@ impl Drop for PromptTrace { } } -/// A writer for interactive prompt output that suspends status regions. +/// A prompt's writes, shaded with the open region background if there is one. /// -/// Region rows are erased when the writer is acquired and no redraw lands until -/// it drops, so a widget owning the cursor between writes is never interrupted. +/// A prompt drawn inside a shaded region shows that region's background like +/// every other row (RFD 095), and a widget owns its own cursor — it rewrites +/// its line with `\r\x1b[K` on each keystroke, and that erase fills with +/// whatever background is active. +/// [`ShadedWriter`] is what keeps the fill right across the widget's own +/// escapes, including the resets it emits mid-line. +enum Canvas { + /// No region is open; writes pass straight through. + Plain(W), + + /// Writes are shaded with the open region's background. + Shaded(Box>), +} + +impl Canvas { + /// Wrap `writer`, shading it when a region is open. + fn new(writer: W, background: Option<&DefaultBackground>) -> Self { + match background { + Some(background) => Self::Shaded(Box::new(ShadedWriter::new(writer, background))), + None => Self::Plain(writer), + } + } + + /// The writer underneath the shading. + fn get_mut(&mut self) -> &mut W { + match self { + Self::Plain(writer) => writer, + Self::Shaded(writer) => writer.get_mut(), + } + } + + /// Close the background, if one was opened. + /// + /// Called from the prompt writer's `Drop` rather than at the end of each + /// prompt: a prompt can end by cancellation or by an error, and a + /// background left open paints everything printed after it. + fn finish(&mut self) { + if let Self::Shaded(writer) = self { + let _err = writer.finish(); + } + } +} + +impl fmt::Write for Canvas { + fn write_str(&mut self, s: &str) -> fmt::Result { + match self { + Self::Plain(writer) => writer.write_str(s), + Self::Shaded(writer) => writer.write_str(s), + } + } +} + +/// A writer for interactive prompt output. +/// +/// The terminal is quiet and clean when the writer is acquired, and stays that +/// way until it drops: status rows are erased, ordinary output waits, and no +/// redraw lands between the widget's own writes. +/// Writes carry the open region background, when there is one. /// Returned by [`Printer::prompt_writer`]. -#[derive(Debug)] pub struct PromptWriter<'a> { - /// The underlying writer, targeting the TTY or `out`. - writer: PrinterWriter<'a>, + /// The underlying writer, targeting the TTY or `out`, shaded when a region + /// is open. + writer: Canvas>, - /// Holds the suspension for the writer's lifetime. + /// Holds the terminal for the writer's lifetime. _suspension: SuspendGuard, /// Records the session's lifetime for the trace log. _trace: PromptTrace, } +impl Drop for PromptWriter<'_> { + fn drop(&mut self) { + self.writer.finish(); + } +} + impl fmt::Write for PromptWriter<'_> { fn write_str(&mut self, s: &str) -> fmt::Result { self.writer.write_str(s) @@ -889,17 +982,21 @@ impl fmt::Write for PromptWriter<'_> { impl io::Write for PromptWriter<'_> { fn write(&mut self, buf: &[u8]) -> io::Result { - io::Write::write(&mut self.writer, buf) + let s = str::from_utf8(buf).map_err(io::Error::other)?; + self.writer.write_str(s).map_err(io::Error::other)?; + Ok(s.len()) } + /// A widget flushes before it reads a key, and on this path a flush is a + /// barrier rather than a buffer drain: it blocks until the printer's worker + /// has written everything queued ahead of it. fn flush(&mut self) -> io::Result<()> { - io::Write::flush(&mut self.writer) + io::Write::flush(self.writer.get_mut()) } } -/// An owned writer targeting one of the [`Printer`]'s streams. +/// An owned channel into one of the [`Printer`]'s streams. /// -/// Returned (boxed) by [`Printer::owned_prompt_writer`]. /// Owns a clone of the printer's command channel, so it is `'static` and /// `Send`; writes flow through the printer's serialized worker just like /// [`PrinterWriter`]. @@ -909,26 +1006,24 @@ struct OwnedPrinterWriter { /// The target output stream. target: PrintTarget, - - /// Holds the status-region suspension for the writer's lifetime. - _suspension: SuspendGuard, - - /// Records the session's lifetime for the trace log. - _trace: PromptTrace, } -impl io::Write for OwnedPrinterWriter { - fn write(&mut self, buf: &[u8]) -> io::Result { - let s = str::from_utf8(buf).map_err(io::Error::other)?; +impl fmt::Write for OwnedPrinterWriter { + fn write_str(&mut self, s: &str) -> fmt::Result { let task = PrintTask { content: s.to_owned(), mode: PrintMode::Instant, target: self.target, origin: PrintOrigin::Prompt, }; - self.tx - .send(Command::Print(task)) - .map_err(|_| io::Error::new(io::ErrorKind::BrokenPipe, "printer shutdown"))?; + self.tx.send(Command::Print(task)).map_err(|_| fmt::Error) + } +} + +impl io::Write for OwnedPrinterWriter { + fn write(&mut self, buf: &[u8]) -> io::Result { + let s = str::from_utf8(buf).map_err(io::Error::other)?; + self.write_str(s).map_err(io::Error::other)?; Ok(s.len()) } @@ -942,6 +1037,40 @@ impl io::Write for OwnedPrinterWriter { } } +/// The owned counterpart of [`PromptWriter`], for components that need to own +/// their output stream. +/// +/// Returned (boxed) by [`Printer::owned_prompt_writer`], and carries the same +/// guarantees. +struct OwnedPromptWriter { + /// The underlying channel, shaded when a region is open. + writer: Canvas, + + /// Holds the terminal for the writer's lifetime. + _suspension: SuspendGuard, + + /// Records the session's lifetime for the trace log. + _trace: PromptTrace, +} + +impl Drop for OwnedPromptWriter { + fn drop(&mut self) { + self.writer.finish(); + } +} + +impl io::Write for OwnedPromptWriter { + fn write(&mut self, buf: &[u8]) -> io::Result { + let s = str::from_utf8(buf).map_err(io::Error::other)?; + self.writer.write_str(s).map_err(io::Error::other)?; + Ok(s.len()) + } + + fn flush(&mut self) -> io::Result<()> { + io::Write::flush(self.writer.get_mut()) + } +} + /// The worker thread that processes print tasks. struct Worker { /// The `out` writer; strips ANSI escapes for non-pretty formats. diff --git a/crates/jp_printer/src/printer_tests.rs b/crates/jp_printer/src/printer_tests.rs index 3d380688f..e70ec9a05 100644 --- a/crates/jp_printer/src/printer_tests.rs +++ b/crates/jp_printer/src/printer_tests.rs @@ -1,3 +1,5 @@ +use jp_term::background::BackgroundFill; + use super::*; use crate::region::OutputLines; @@ -953,6 +955,123 @@ fn output_held_by_a_prompt_is_not_lost_at_shutdown() { ); } +/// A full-width reasoning-region background. +fn shaded_region() -> DefaultBackground { + DefaultBackground { + param: "48;5;236".into(), + fill: BackgroundFill::Terminal, + } +} + +#[test] +fn a_prompt_inside_a_shaded_region_carries_its_background() { + let (printer, out, _err) = Printer::memory(OutputFormat::TextPretty); + printer.set_prompt_background(Some(shaded_region())); + + { + let mut prompt = printer.prompt_writer(); + write!(prompt, "Run local shell tool?").unwrap(); + } + printer.flush(); + + // The background is asserted before the text and closed once the widget is + // done with the terminal, so the row is shaded and nothing after it is. + assert_eq!(*out.lock(), "\x1b[48;5;236mRun local shell tool?\x1b[49m"); +} + +#[test] +fn an_owned_prompt_writer_carries_the_background_too() { + // The inline reply widget owns its output stream, so it takes this writer + // rather than the borrowed one. Both are prompts and both are inside the + // region. + let (printer, out, _err) = Printer::memory(OutputFormat::TextPretty); + printer.set_prompt_background(Some(shaded_region())); + + { + let mut prompt = printer.owned_prompt_writer(); + write!(prompt, "Edit arguments").unwrap(); + } + printer.flush(); + + assert_eq!(*out.lock(), "\x1b[48;5;236mEdit arguments\x1b[49m"); +} + +#[test] +fn a_prompt_outside_a_shaded_region_is_unshaded() { + let (printer, out, _err) = Printer::memory(OutputFormat::TextPretty); + + { + let mut prompt = printer.prompt_writer(); + write!(prompt, "Run local shell tool?").unwrap(); + } + printer.flush(); + + assert_eq!(*out.lock(), "Run local shell tool?"); +} + +#[test] +fn clearing_the_background_unshades_later_prompts() { + let (printer, out, _err) = Printer::memory(OutputFormat::TextPretty); + + printer.set_prompt_background(Some(shaded_region())); + drop(printer.prompt_writer()); + printer.set_prompt_background(None); + + { + let mut prompt = printer.prompt_writer(); + write!(prompt, "after").unwrap(); + } + printer.flush(); + + assert_eq!(*out.lock(), "after"); +} + +#[test] +fn an_abandoned_prompt_still_closes_its_background() { + // A widget can end by `Ctrl+C` or by an error, neither of which returns + // through the normal path. The close lives in `Drop` so the background + // cannot outlive the prompt and paint whatever is printed next. + let (printer, out, _err) = Printer::memory(OutputFormat::TextPretty); + printer.set_prompt_background(Some(shaded_region())); + + { + let mut prompt = printer.prompt_writer(); + write!(prompt, "Deliver result?").unwrap(); + // No further writes: the prompt is abandoned mid-session. + } + printer.set_prompt_background(None); + printer.println("after"); + printer.flush(); + + assert_eq!(*out.lock(), "\x1b[48;5;236mDeliver result?\x1b[49mafter\n"); +} + +#[test] +fn flushing_a_shaded_prompt_waits_for_the_printer() { + // A widget flushes its writer before it reads a key, and on this path a + // flush is a barrier rather than a buffer drain: the bytes have to have + // landed before the widget takes the cursor and the terminal's mode. + // Shading decorates that writer and must not swallow it. + let (printer, out, _err) = Printer::memory(OutputFormat::TextPretty); + printer.set_prompt_background(Some(shaded_region())); + + let mut prompt = printer.prompt_writer(); + + // Queued after acquisition drained the printer, and slow enough that the + // worker is certainly still inside it. + printer.print("slow".typewriter(Duration::from_millis(100))); + + write!(prompt, "Run local shell tool?").unwrap(); + io::Write::flush(&mut prompt).unwrap(); + + // Deliberately no `printer.flush()`, which is the assertion. + assert!( + out.lock().contains("Run local shell tool?"), + "flushing the prompt must drain the printer, got {:?}", + *out.lock() + ); +} + #[test] fn suspend_status_erases_before_it_returns() { let (printer, _out, err) = region_printer(); From 00a9cd52a8469cde39996bc8b6ca0fdf95cc2e8f Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Tue, 8 Sep 2026 15:58:02 +0200 Subject: [PATCH 4/4] docs(tickets): Close the prompt-shading ticket, file the width overlap `T-0fg8cjs` records where the shading landed, why `ansi` and `shade` had to move crates to get there, and the two things found on the way: the swallowed flush that made prompt output staircase inside a reasoning block, and a mock reply widget that wrote nothing to the stream it was handed. `T-0fk64t3` records what the move exposed. `jp_term` holds two implementations of display width that arrived from different directions and have never been compared, one of which knows about tabs and one of which does not. Signed-off-by: Jean Mertz --- ...ckground-drops-out-while-a-prompt-is-up.md | 60 ++++++++++++++++++- ...jp_term-measures-display-width-two-ways.md | 43 +++++++++++++ 2 files changed, 102 insertions(+), 1 deletion(-) create mode 100644 docs/ticket/0fk64t3-jp_term-measures-display-width-two-ways.md diff --git a/docs/ticket/0fg8cjs-the-reasoning-background-drops-out-while-a-prompt-is-up.md b/docs/ticket/0fg8cjs-the-reasoning-background-drops-out-while-a-prompt-is-up.md index 8026a17e1..e0c4e13f6 100644 --- a/docs/ticket/0fg8cjs-the-reasoning-background-drops-out-while-a-prompt-is-up.md +++ b/docs/ticket/0fg8cjs-the-reasoning-background-drops-out-while-a-prompt-is-up.md @@ -1,6 +1,6 @@ # The reasoning background drops out while a prompt is up -- **Status**: Todo +- **Status**: Done - **Kind**: Bug - **Authors**: jp - **Date**: 2026-09-08 @@ -126,3 +126,61 @@ painted the row rather than clearing it, and the shell prompt inherited it after `prompter_tests.rs` has a test that abandons a prompt mid-session and asserts the close still lands; worth extending to the editor path rather than writing a new one. + +----- + +- **From**: jp +- **Date**: 2026-09-08T13:57:29Z + +Done, including the `Ctrl+X` / inline-reply path this ticket left open. + +## Where it landed + +The background moved into `jp_printer`, which is where this ticket's "shape of a +fix" pointed: `Printer::set_prompt_background` holds it, and both +`prompt_writer` and `owned_prompt_writer` wrap their writer in a `ShadedWriter` +when one is set. +Every prompt taken from the printer is shaded without its call site knowing a +region exists, so the reply widget is covered by the same code as the approval +prompt rather than needing its own. + +That required `ShadedWriter` to be reachable from `jp_printer`, which depends on +`jp_term` and deliberately not on `jp_md`. +`ansi` and `shade` moved to `jp_term` along with `DefaultBackground`, +`BackgroundFill` and `line_fill` (now `jp_term::background`). +None of them were markdown: `segments` tokenizes an escape stream, `AnsiState` +tracks what a stream left active. + +The `owned_prompt_writer` trait problem this ticket recorded resolved itself in +the move. +`OwnedPrinterWriter` gained a `fmt::Write` impl and became the inner writer of a +shared `Canvas`, so the adapter is a trait impl on the type that needs it +rather than a wrapper at the call site. + +`ToolPrompter::set_background`, `canvas()`, and `PromptCanvas` are deleted. +The coordinator still names the background at the same point in +`resolve_tool_call_decision` — the region is per tool call, so the read has to +happen there — but it now hands it to the printer. + +## Two things found on the way + +The `PromptCanvas::flush` this ticket's earlier comment described was swallowing +the flush for the shaded variant, on the grounds that the printer holds nothing +back a flush could release. +True of buffering, false of ordering: on the prompt path a flush is a barrier, +and `inquire` flushes before it reads a key. +Inside a reasoning block that was the only difference between the two variants, +which is why prompt output staircased there and nowhere else. +Fixed separately, along with making prompt-writer acquisition quiesce the +terminal. + +`MockPromptBackend::inline_reply` wrote nothing to the stream it was handed, so +a test asserting on what the reply widget renders passed against no prompt at +all. +It now writes its message, like the real widget does. + +## Not covered + +The eight prompts in `T-0ffsv2r` still build their own `io::stderr()` writer, so +they are still unshaded. +They pick this up for free once they go through the printer. diff --git a/docs/ticket/0fk64t3-jp_term-measures-display-width-two-ways.md b/docs/ticket/0fk64t3-jp_term-measures-display-width-two-ways.md new file mode 100644 index 000000000..b4b8078a8 --- /dev/null +++ b/docs/ticket/0fk64t3-jp_term-measures-display-width-two-ways.md @@ -0,0 +1,43 @@ +# `jp_term` measures display width two ways + +- **Status**: Todo +- **Kind**: Chore +- **Authors**: jp +- **Date**: 2026-09-08 +- **Label**: package=jp_term +- **Label**: type=task + +`jp_term` now holds two independent implementations of "how wide is this text on +screen", which arrived from different directions and have never been compared. + +- `jp_term::width::display_width` strips ANSI with `strip-ansi-escapes`, then + measures with `unicode-width`. + Its neighbours (`truncate_to_width`, `wrap_ranges`, `prefix_end_for_width`) + walk grapheme clusters and probe for ligatures that collapse. +- `jp_term::ansi::visual_width` tokenizes with `ansi::segments`, joins the + visible runs, and measures the result. + `advance_column` builds on it and adds tab stops and carriage returns. + +They disagree in at least one way that matters: `visual_width` counts a tab as +one column and `advance_column` exists because of it, while the `width` module +has no notion of a tab at all. +Whether they disagree on OSC 8 hyperlinks, partial escapes, or ligature collapse +is unknown — nothing tests them against each other. + +## Why it is worth doing + +The duplication was invisible while the two lived in different crates. +Now a reader of `jp_term` has to pick one, and the names give no help: +`display_width` and `visual_width` are the same phrase. + +## Shape of a fix + +Start with a test that runs both over the same inputs — plain ASCII, CJK, VS16 +emoji, ZWJ sequences, tabs, OSC 8 links, escapes split mid-sequence — and +records where they differ. +That answers whether this is one function with two names or two functions with +one job each. + +If they agree except on tabs, collapse to one and keep `advance_column` as the +cursor-position variant. +If they genuinely differ, the names have to say how.