diff --git a/codex-rs/tui/src/bottom_pane/chat_composer/status_surface_tests.rs b/codex-rs/tui/src/bottom_pane/chat_composer/status_surface_tests.rs index 3d429c11fa..49be9b4971 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer/status_surface_tests.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer/status_surface_tests.rs @@ -25,17 +25,18 @@ fn composer() -> ChatComposer { #[test] fn extension_status_line_clips_links_to_footer_layout() { let mut composer = composer(); - let tree = serde_json::from_value(serde_json::json!({ - "type": "row", "props": {}, "children": [ - { "type": "text", "props": { "text": "plain" } }, - { "type": "row", "props": {}, "children": [ - { "type": "text", "props": { "text": " · " } }, + let tree = r#"{ + "type": "row", "props": {"gap": 0}, "children": [ + "plain", + { "type": "row", "props": {"gap": 0}, "children": [ + " · ", { "type": "link", "props": { "href": "https://example.com/pr/42" }, "children": [ - { "type": "text", "props": { "text": "PR #42" } } + "PR #42" ] } ] } ] - })) + }"# + .parse() .unwrap(); composer.set_status_line(Some(FooterContent::Extension { content: tree })); diff --git a/codex-rs/tui/src/bottom_pane/footer/footer_content.rs b/codex-rs/tui/src/bottom_pane/footer/footer_content.rs index 32d56b518c..05ce7ddd51 100644 --- a/codex-rs/tui/src/bottom_pane/footer/footer_content.rs +++ b/codex-rs/tui/src/bottom_pane/footer/footer_content.rs @@ -8,14 +8,14 @@ use ratatui::widgets::Widget; use crate::line_truncation::truncate_line_with_ellipsis_if_overflow; use crate::terminal_hyperlinks::HyperlinkLine; -use crate::ui_tree::UiNode; +use crate::ui_tree::UiTree; /// Owned content for the footer's negotiated left-hand area. #[derive(Clone, Debug, PartialEq)] pub(crate) enum FooterContent { Line(Line<'static>), HyperlinkLine(HyperlinkLine), - Extension { content: UiNode }, + Extension { content: UiTree }, } impl FooterContent { diff --git a/codex-rs/tui/src/bottom_pane/status_line_content.rs b/codex-rs/tui/src/bottom_pane/status_line_content.rs index 47c52f5d5d..faf4929d32 100644 --- a/codex-rs/tui/src/bottom_pane/status_line_content.rs +++ b/codex-rs/tui/src/bottom_pane/status_line_content.rs @@ -4,11 +4,11 @@ use super::STATUS_LINE_SEPARATOR; use super::footer_content::FooterContent; use super::status_line_from_segments; use super::status_line_setup::StatusLineItem; -use crate::ui_tree::Key; use crate::ui_tree::LinkHref; use crate::ui_tree::LinkProps; -use crate::ui_tree::StackProps; +use crate::ui_tree::RowProps; use crate::ui_tree::UiNode; +use crate::ui_tree::UiTree; use codex_protocol::ThreadId; #[derive(Clone, Copy, Debug, PartialEq, Eq)] @@ -38,27 +38,27 @@ impl StatusLineRenderer { let mut children = Vec::new(); for (item, text) in segments { if !children.is_empty() { - children.push(Some(UiNode::text(/*key*/ None, STATUS_LINE_SEPARATOR))); + children.push(Some(UiNode::String(STATUS_LINE_SEPARATOR.to_owned()))); } - let key = Some(Key::String(item.to_string())); + let text = UiNode::String(text); let node = if item == StatusLineItem::PullRequestNumber && let Some(href) = pull_request_url.and_then(|url| LinkHref::try_from(url).ok()) { UiNode::Link { - key, props: LinkProps { href, title: None }, - children: vec![Some(UiNode::text(/*key*/ None, text))], + children: vec![Some(text)], } } else { - UiNode::text(key, text) + text }; children.push(Some(node)); } let tree = UiNode::Row { - key: Some(Key::String("status-line".to_string())), - props: StackProps::default(), + props: RowProps { gap: 0 }, children, }; - Some(FooterContent::Extension { content: tree }) + Some(FooterContent::Extension { + content: UiTree(Some(tree)), + }) } } diff --git a/codex-rs/tui/src/ui_tree.rs b/codex-rs/tui/src/ui_tree.rs index e5b9225081..45de0fc8ba 100644 --- a/codex-rs/tui/src/ui_tree.rs +++ b/codex-rs/tui/src/ui_tree.rs @@ -1,10 +1,6 @@ -//! Bounded, display-only subset of the `codex-tui/1` tree contract. -//! -//! The wire tree is independent of its producer. Producers normalize text into leaf nodes and -//! arrays into composite nodes; unsupported primitives and properties fail deserialization. This -//! first slice renders one horizontal line of plain text, links, rows, spacers, dividers, and -//! numeric gaps. Links underline their text. Keys are preserved for future reconciliation. +//! Display elements for the terminal UI and their JSON representation. +mod decode; mod render; #[cfg(test)] @@ -14,47 +10,36 @@ mod tests; use serde::Deserialize; use serde::Serialize; -/// A single display node, or no content. -pub(crate) type UiTree = Option; +const MAX_GAP: u64 = (1 << 53) - 1; +const MAX_TEXT_BYTES: usize = 16 * 1024; + +/// A nullable root with bounded depth, node count, and total UTF-8 leaf bytes. +#[derive(Clone, Debug, PartialEq, Serialize)] +pub(crate) struct UiTree(pub(crate) Option); #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] #[serde(tag = "type", rename_all = "kebab-case", deny_unknown_fields)] pub(crate) enum UiNode { Text { - #[serde(skip_serializing_if = "Option::is_none")] - key: Option, + #[serde(default, skip_serializing)] props: TextProps, + children: Vec>, }, Link { - #[serde(skip_serializing_if = "Option::is_none")] - key: Option, props: LinkProps, - children: Vec, + children: Vec>, }, Row { - #[serde(skip_serializing_if = "Option::is_none")] - key: Option, - props: StackProps, - children: Vec, - }, - Spacer { - #[serde(skip_serializing_if = "Option::is_none")] - key: Option, - }, - Divider { - #[serde(skip_serializing_if = "Option::is_none")] - key: Option, + props: RowProps, + children: Vec>, }, + #[serde(untagged)] + String(String), } -impl UiNode { - pub(crate) fn text(key: Option, text: impl Into) -> Self { - Self::Text { - key, - props: TextProps { text: text.into() }, - } - } -} +#[derive(Clone, Debug, Default, PartialEq, Deserialize)] +#[serde(deny_unknown_fields)] +pub(crate) struct TextProps {} #[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] #[serde(deny_unknown_fields)] @@ -64,7 +49,7 @@ pub(crate) struct LinkProps { pub(crate) title: Option, } -/// An absolute HTTP(S) destination safe to embed in a terminal hyperlink. +/// An absolute HTTP(S) URL without control characters, at most 8 KiB in UTF-8. #[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] #[serde(try_from = "String")] pub(crate) struct LinkHref(String); @@ -84,27 +69,14 @@ impl TryFrom<&str> for LinkHref { crate::terminal_hyperlinks::web_destination(value) .filter(|destination| destination == value) .map(Self) - .ok_or("link href must be an absolute HTTP(S) URL without control characters") + .ok_or("link href must be an absolute HTTP(S) URL of at most 8 KiB without control characters") } } -#[derive(Clone, Debug, Eq, Hash, PartialEq, Serialize, Deserialize)] -#[serde(untagged)] -pub(crate) enum Key { - String(String), - Number(serde_json::Number), -} - -#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)] -#[serde(deny_unknown_fields)] -pub(crate) struct TextProps { - pub(crate) text: String, -} - #[derive(Clone, Debug, Default, PartialEq, Serialize, Deserialize)] #[serde(deny_unknown_fields)] -pub(crate) struct StackProps { - /// Numeric spacing uses the profile's four-point scale. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub(crate) gap: Option, +pub(crate) struct RowProps { + /// Horizontal spacing in terminal columns, expressed as a non-negative JavaScript-safe integer. + #[serde(deserialize_with = "decode::gap")] + pub(crate) gap: u64, } diff --git a/codex-rs/tui/src/ui_tree/decode.rs b/codex-rs/tui/src/ui_tree/decode.rs new file mode 100644 index 0000000000..0377563bcf --- /dev/null +++ b/codex-rs/tui/src/ui_tree/decode.rs @@ -0,0 +1,54 @@ +//! Enforce the input byte limit before parsing and the UI tree limits after deserialization. + +use super::MAX_GAP; +use super::MAX_TEXT_BYTES; +use super::UiNode; +use super::UiTree; +use serde::Deserialize; +use serde::Deserializer; +use serde::de::Error; + +impl std::str::FromStr for UiTree { + type Err = serde_json::Error; + + fn from_str(text: &str) -> Result { + if text.len() > 64 * 1024 { + return Err(Self::Err::custom("UI tree JSON exceeds 64 KiB")); + } + let root: Option = serde_json::from_str(text)?; + let mut pending: Vec<_> = root.iter().map(|node| (node, 0)).collect(); + let mut nodes = 0; + let mut text_bytes = 0; + while let Some((node, depth)) = pending.pop() { + if depth > 16 { + return Err(Self::Err::custom("UI tree exceeds maximum depth 16")); + } + nodes += 1; + if nodes > 256 { + return Err(Self::Err::custom("UI tree exceeds 256 non-null nodes")); + } + match node { + UiNode::String(text) => { + text_bytes += text.len(); + if text_bytes > MAX_TEXT_BYTES { + return Err(Self::Err::custom("UI tree exceeds 16 KiB of text")); + } + } + UiNode::Text { children, .. } + | UiNode::Link { children, .. } + | UiNode::Row { children, .. } => { + pending.extend(children.iter().flatten().map(|child| (child, depth + 1))); + } + } + } + Ok(Self(root)) + } +} + +pub(super) fn gap<'de, D: Deserializer<'de>>(de: D) -> Result { + let gap = u64::deserialize(de)?; + if gap > MAX_GAP { + return Err(D::Error::custom("gap exceeds the maximum safe integer")); + } + Ok(gap) +} diff --git a/codex-rs/tui/src/ui_tree/render.rs b/codex-rs/tui/src/ui_tree/render.rs index a232478ef8..a2553d035a 100644 --- a/codex-rs/tui/src/ui_tree/render.rs +++ b/codex-rs/tui/src/ui_tree/render.rs @@ -1,5 +1,6 @@ //! Render the visible prefix of a horizontal display tree into one terminal line. +use super::MAX_TEXT_BYTES; use super::UiNode; use super::UiTree; use crate::history_cell::sanitize_user_text; @@ -8,27 +9,14 @@ use crate::terminal_hyperlinks::HyperlinkLine; use crate::terminal_hyperlinks::TerminalHyperlink; use ratatui::style::Modifier; -const MAX_DEPTH: usize = 16; -const MAX_NODES: usize = 256; -const MAX_TEXT_BYTES: usize = 16 * 1024; - -struct Budget { - nodes: usize, - text_bytes: usize, -} - -impl UiNode { +impl UiTree { /// Render one row. On overflow, fit an unlinked ellipsis inside the supplied width. pub(crate) fn render_line(&self, width: u16) -> HyperlinkLine { let width = usize::from(width); - if width == 0 { + let Some(root) = self.0.as_ref().filter(|_| width > 0) else { return HyperlinkLine::default(); - } - let mut budget = Budget { - nodes: MAX_NODES, - text_bytes: MAX_TEXT_BYTES, }; - let mut line = node_line(Some(self), /*depth*/ 0, &mut budget).unwrap_or_default(); + let mut line = node_line(root, width); if line.width() > width { line.line = truncate_line_to_width(line.line, width - 1); line.clip_hyperlinks_to_width(line.width()); @@ -38,30 +26,18 @@ impl UiNode { } } -fn node_line(node: Option<&UiNode>, depth: usize, budget: &mut Budget) -> Option { - if budget.nodes == 0 { - return None; - } - budget.nodes -= 1; - if depth > MAX_DEPTH { - return None; - } - match node? { - UiNode::Text { props, .. } => { - let raw = &props.text; - if budget.text_bytes == 0 && !raw.is_empty() { - return None; - } - let end = raw.floor_char_boundary(raw.len().min(budget.text_bytes)); - let first = raw[..end].split('\n').next().unwrap_or_default(); - budget.text_bytes -= first.len(); - let visible = sanitize_user_text(first.into()).replace('\t', " "); - Some(visible.into()) +fn node_line(node: &UiNode, width: usize) -> HyperlinkLine { + match node { + UiNode::String(raw) => { + // Built-in values are constructed directly, bypassing the wire decoder. + let raw = &raw[..raw.floor_char_boundary(MAX_TEXT_BYTES)]; + let first = raw.split('\n').next().unwrap_or_default(); + sanitize_user_text(first.into()) + .replace('\t', " ") + .into() } - UiNode::Link { - props, children, .. - } => { - let mut line = row_line(children, /*gap*/ 0, depth, budget)?; + UiNode::Link { props, children } => { + let mut line = row_line(children, /*gap*/ 0, width); for span in &mut line.line.spans { span.style = span.style.add_modifier(Modifier::UNDERLINED); } @@ -69,48 +45,40 @@ fn node_line(node: Option<&UiNode>, depth: usize, budget: &mut Budget) -> Option if width > 0 { line.hyperlinks = vec![TerminalHyperlink::web(0..width, props.href.0.clone())]; } - Some(line) + line } - UiNode::Row { - props, children, .. - } => { - // A spacing unit is four points; a terminal cell is eight points across. - let gap = (props.gap.unwrap_or(0.0).clamp(0.0, 32.0) / 2.0).ceil() as usize; - row_line(children, gap, depth, budget) - } - UiNode::Spacer { .. } => Some(" ".into()), - UiNode::Divider { .. } => Some("│".into()), + UiNode::Row { props, children } => row_line(children, props.gap, width), + UiNode::Text { children, .. } => row_line(children, /*gap*/ 0, width), } } -fn row_line( - children: &[UiTree], - gap: usize, - depth: usize, - budget: &mut Budget, -) -> Option { - let mut result: Option = None; +fn row_line(children: &[Option], gap: u64, width: usize) -> HyperlinkLine { + let mut result = HyperlinkLine::default(); let mut used = 0; - for child in children { - if budget.nodes == 0 { - break; - } - let Some(child) = node_line(child.as_ref(), depth + 1, budget) else { + for child in children.iter().flatten() { + let child = node_line(child, width); + let child_width = child.width(); + if child_width == 0 { continue; - }; - let spacing = if result.is_some() { gap } else { 0 }; - let line = result.get_or_insert_default(); - if spacing > 0 { - line.line.push_span(" ".repeat(spacing)); } - let offset = used + spacing; - used = offset + child.width(); - line.line.spans.extend(child.line.spans); - line.hyperlinks + if used > 0 && gap > 0 { + // One column past the viewport is sufficient to detect overflow. + let spacing = gap.min((width + 1 - used) as u64) as usize; + result.line.push_span(" ".repeat(spacing)); + used += spacing; + } + let offset = used; + result.line.spans.extend(child.line.spans); + result + .hyperlinks .extend(child.hyperlinks.into_iter().map(|mut link| { link.columns = link.columns.start + offset..link.columns.end + offset; link })); + used += child_width; + if used > width { + break; + } } result } diff --git a/codex-rs/tui/src/ui_tree_tests.rs b/codex-rs/tui/src/ui_tree_tests.rs index 4380046c78..9aa2574033 100644 --- a/codex-rs/tui/src/ui_tree_tests.rs +++ b/codex-rs/tui/src/ui_tree_tests.rs @@ -4,45 +4,83 @@ use super::*; use pretty_assertions::assert_eq; use ratatui::buffer::Buffer; use ratatui::layout::Rect; +use serde_json::Value; use serde_json::json; -fn tree(value: serde_json::Value) -> UiNode { - serde_json::from_value::(value).unwrap().unwrap() +fn parse(value: Value) -> serde_json::Result { + value.to_string().parse() } -fn text(key: &str, value: &str) -> serde_json::Value { - json!({"type":"text","key":key,"props":{"text":value}}) +fn tree(value: Value) -> UiTree { + parse(value).unwrap() } #[test] -fn normalized_tree_round_trips_and_enforces_leaf_shape() { - let value = json!({"type":"row","key":7,"props":{"gap":2.0},"children":[{"type":"link","props":{"href":"https://example.com/pr/42"},"children":[text("usage", "12%")]}, null, {"type":"spacer"}]}); +fn elements_round_trip_and_require_explicit_row_spacing() { + let value = json!({"type":"row","props":{"gap":1},"children":[{"type":"link","props":{"href":"https://example.com/pr/42","title":"PR details"},"children":["12%"]}, null, " "]}); assert_eq!(serde_json::to_value(tree(value.clone())).unwrap(), value); - assert!( - serde_json::from_value::( - json!({"type":"text","props":{"text":"hello"},"children":[]}) - ) - .is_err() - ); - assert_eq!(serde_json::from_value::(json!(null)).unwrap(), None); for href in [ "relative/path", "file:///tmp/example", "https://example.com/\u{001b}", ] { - assert!( - serde_json::from_value::( - json!({"type":"link","props":{"href":href},"children":[]}) - ) - .is_err() - ); + assert!(parse(json!({"type":"link","props":{"href":href},"children":[]})).is_err()); } + for (gap, expected) in [(0, "AB"), (1, "A B"), (2, "A B"), (MAX_GAP, "A …")] { + let row = tree(json!({"type":"row","props":{"gap":gap},"children":["A","B"]})); + assert_eq!(row.render_line(/*width*/ 4).line.to_string(), expected); + } +} + +#[test] +fn rejects_invalid_gaps_and_elements() { + for gap in ["-1", "1.5", "9007199254740992", r#""1""#, "[1]"] { + let row = format!(r#"{{"children":[],"props":{{"gap":{gap}}},"type":"row"}}"#); + assert!(row.parse::().is_err(), "{row}"); + } + for input in [ + "[]", + "null true", + r#"{"type":{"text":null},"children":[]}"#, + r#"{"type":"text","children":[["Ready"]]}"#, + r#"{"type":"text","props":null,"children":[]}"#, + r#"{"type":"row","props":{},"children":[]}"#, + r#"{"type":"row","props":{"gap":0,"extra":true},"children":[]}"#, + r#"{"props":{"gap":0,"gap":1},"children":[],"type":"row"}"#, + ] { + assert!(input.parse::().is_err(), "{input}"); + } +} + +#[test] +fn deserialization_checks_limits_across_the_whole_tree() { + let parent = |children: Vec| json!({"type":"text","children":children}); + assert!(parse(parent(vec![Value::Null; 257])).is_ok()); + let nested = (0..16).fold(json!("X"), |child, _| parent(vec![child])); + assert!(parse(nested.clone()).is_ok()); + assert!(parse(parent(vec![nested])).is_err()); + let mut leaves = vec![json!(""); 255]; + assert!(parse(parent(leaves.clone())).is_ok()); + leaves.push(json!("")); + assert!(parse(parent(leaves)).is_err()); + let text = "界".repeat(5461); // 16,383 UTF-8 bytes. + for (suffix, ok) in [("x", true), ("xx", false)] { + assert_eq!(parse(parent(vec![json!(text), json!(suffix)])).is_ok(), ok); + } + let at_limit = format!("null{}", " ".repeat(64 * 1024 - 4)); + assert_eq!(at_limit.parse::().unwrap(), UiTree(None)); + assert!(format!("{at_limit} ").parse::().is_err()); + let prefix = "https://example.com/"; + let href = format!("{prefix}{}", "x".repeat(8192 - prefix.len())); + let link = |url: &str| json!({"type":"link","props":{"href":url},"children":["ok"]}); + assert!(parse(link(&href)).is_ok()); + assert!(parse(link(&format!("{href}x"))).is_err()); } #[test] fn links_only_annotate_their_own_cells() { let tree = tree( - json!({"type":"row","props":{"gap":2},"children":[text("prefix", "界"),{"type":"link","props":{"href":"https://example.com/pr/42"},"children":[{"type":"row","props":{},"children":[text("pr", "PR #42")]}]}]}), + json!({"type":"row","props":{"gap":1},"children":["界",{"type":"link","props":{"href":"https://example.com/pr/42"},"children":[{"type":"text","children":["PR #42"]}]}]}), ); let area = Rect::new( /*x*/ 0, /*y*/ 0, /*width*/ 16, /*height*/ 1, @@ -75,32 +113,30 @@ fn links_only_annotate_their_own_cells() { } #[test] -fn extension_status_line_keeps_exactly_fitting_halfwidth_voiced_katakana() { - let status = tree(text("name", "ｶﾞ")); +fn render_line_keeps_exactly_fitting_halfwidth_voiced_katakana() { + let text = tree(json!("ｶﾞ")); let area = Rect::new( /*x*/ 0, /*y*/ 0, /*width*/ 2, /*height*/ 1, ); let mut buffer = Buffer::empty(area); - status - .render_line(area.width) + text.render_line(area.width) .render_fitted(area, &mut buffer); assert_eq!(buffer[(0, 0)].symbol(), "ｶﾞ"); } #[test] fn ellipsis_counts_toward_width_only_when_there_is_overflow() { - let status = - tree(json!({"type":"row","props":{"gap":2},"children":[text("label","abcde"), null]})); + let row = tree(json!({"type":"row","props":{"gap":1},"children":["abcde", null]})); for (width, expected) in [(0, ""), (1, "…"), (4, "abc…"), (5, "abcde")] { - let line = status.render_line(width); + let line = row.render_line(width); assert_eq!(line.line.to_string(), expected); assert!(line.width() <= usize::from(width)); } - let wide = tree(text("label", "a界界")); + let wide = tree(json!("a界界")); assert_eq!(wide.render_line(/*width*/ 4).line.to_string(), "a界…"); let long = "x".repeat(513); assert_eq!( - tree(text("label", &long)) + tree(json!(&long)) .render_line(/*width*/ 513) .line .to_string(), @@ -110,9 +146,16 @@ fn ellipsis_counts_toward_width_only_when_there_is_overflow() { #[test] fn first_line_skips_hidden_text_before_rendering_its_sibling() { - let multiline = format!("\u{1b}[2Jok\n{}", "x".repeat(16 * 1024)); - let tree = tree(json!({"type":"row","props":{},"children":[ - text("first", &multiline), text("sibling", "!") + let multiline = format!("\u{1b}[2Jok\n{}", "x".repeat(1024)); + let tree = tree(json!({"type":"row","props":{"gap":0},"children":[ + &multiline, "!" ]})); assert_eq!(tree.render_line(/*width*/ 4).line.to_string(), "ok!"); } + +#[test] +fn directly_constructed_text_keeps_a_bounded_utf8_prefix() { + let prefix = "x".repeat(MAX_TEXT_BYTES - 1); + let text = UiTree(Some(UiNode::String(format!("{prefix}界tail")))); + assert_eq!(text.render_line(u16::MAX).line.to_string(), prefix); +}