diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml new file mode 100644 index 00000000..fe33f79a --- /dev/null +++ b/.github/workflows/codeql.yml @@ -0,0 +1,44 @@ +name: "CodeQL" + +on: + push: + branches: [main] + pull_request: + branches: [main] + schedule: + - cron: "27 4 * * 1" # weekly, catches new queries on unchanged code + +jobs: + analyze: + name: Analyze (${{ matrix.language }}) + runs-on: ubuntu-latest + permissions: + security-events: write # upload results to code scanning + packages: read # fetch CodeQL query packs + actions: read + contents: read + strategy: + fail-fast: false + matrix: + include: + - language: actions + build-mode: none + - language: rust + build-mode: none + - language: javascript-typescript + build-mode: none + steps: + - uses: actions/checkout@v7 + + - name: Initialize CodeQL + uses: github/codeql-action/init@v4 + with: + languages: ${{ matrix.language }} + build-mode: ${{ matrix.build-mode }} + # Uncomment for deeper analysis (more queries, higher false-positive rate): + # queries: security-extended + + - name: Perform CodeQL Analysis + uses: github/codeql-action/analyze@v4 + with: + category: "/language:${{ matrix.language }}" diff --git a/CHANGELOG.md b/CHANGELOG.md index 841b9c37..79e3ff24 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ All notable changes to this project will be documented in this file. - **Mention file autocomplete** (#226, @srothgan): Move `@` file matching off the UI path, support raw indexed file and folder mentions with spaces, preserve whole-mention replacement boundaries, and rank shallow project paths ahead of deep dependency matches unless the query explicitly targets the deep path. - **Atomic input placeholders** (#227, @srothgan): Treat image badges and pasted-text placeholders as shared textarea atoms, keeping cursor movement, deletion, undo/redo, image attachment state, and paste expansion aligned through `tui-textarea-2` `0.12.0` atomic range support. +- **Question and list rendering** (#228, @srothgan): Render markdown lists with indentation instead of injected blank gaps, and show `AskUserQuestion` answers as structured results with selected options, descriptions, previews, and notes. ### Documentation @@ -20,10 +21,12 @@ All notable changes to this project will be documented in this file. ### Maintenance +- **Dead test helper cleanup** (#228, @srothgan): Remove stale test-only helpers that kept old production paths alive only through tests. - **Social discoverability** (#219, @srothgan): Add Open Graph and Twitter Card meta tags with a social preview image to the docs site, and expand the npm package keywords to mirror the repo topics. ### CI and Dependencies +- **CodeQL scanning** (#228, @srothgan): Add a CodeQL workflow for repository code scanning. - **quinn-proto advisory fix** (#220, @srothgan): Bump `quinn-proto` to `0.11.15` for `RUSTSEC-2026-0185` (remote memory exhaustion from unbounded out-of-order stream reassembly). - **Security Audit workflow resilience** (#220, @srothgan): Mark the `cargo audit` step `continue-on-error` so newly published advisories still report and file a tracking issue without failing the scheduled Security Audit run. - **Dependency updates** (#221, #222, #224, #223): Bump `uuid` to `1.23.4`, `anyhow` to `1.0.103`, `tui-markdown` to `0.3.8`, and `knip` to `6.23.0` in `agent-sdk`. diff --git a/agent-sdk/src/bridge.test.ts b/agent-sdk/src/bridge.test.ts index b53fd92f..52291b69 100644 --- a/agent-sdk/src/bridge.test.ts +++ b/agent-sdk/src/bridge.test.ts @@ -3681,6 +3681,71 @@ test("requestAskUserQuestionAnswers preserves previews and annotations in update question_index: 0, total_questions: 1, }); + + const completedQuestionUpdate = events + .map((event) => (event.event === "session_update" ? (event.update as Record) : undefined)) + .find((update) => { + const toolCallUpdate = update?.tool_call_update as Record | undefined; + const fields = toolCallUpdate?.fields as Record | undefined; + return toolCallUpdate?.tool_call_id === "tool-question" && fields?.status === "completed"; + })?.tool_call_update as Record | undefined; + const completedFields = completedQuestionUpdate?.fields as Record | undefined; + assert.deepEqual(completedFields?.raw_input, { + questions: [ + { + question: "Pick deployment target", + header: "Target", + multiSelect: true, + options: [ + { + label: "Staging", + description: "Low-risk validation", + preview: "Deploy to staging first.", + }, + { + label: "Production", + description: "Customer-facing rollout", + preview: "Deploy to production after approval.", + }, + ], + }, + ], + answers: { + "Pick deployment target": "Staging, Production", + }, + annotations: { + "Pick deployment target": { + preview: "Deploy to staging first.\n\nDeploy to production after approval.", + notes: "Roll out in both environments", + }, + }, + question_results: [ + { + question: "Pick deployment target", + header: "Target", + question_index: 0, + total_questions: 1, + selected_options: [ + { + option_id: "question_0", + label: "Staging", + description: "Low-risk validation", + preview: "Deploy to staging first.", + }, + { + option_id: "question_1", + label: "Production", + description: "Customer-facing rollout", + preview: "Deploy to production after approval.", + }, + ], + annotation: { + preview: "Deploy to staging first.\n\nDeploy to production after approval.", + notes: "Roll out in both environments", + }, + }, + ], + }); }); test("normalizeToolKind maps known tool names", () => { diff --git a/agent-sdk/src/bridge/user_interaction.ts b/agent-sdk/src/bridge/user_interaction.ts index afc40332..21d57675 100644 --- a/agent-sdk/src/bridge/user_interaction.ts +++ b/agent-sdk/src/bridge/user_interaction.ts @@ -210,6 +210,41 @@ function askUserQuestionTranscript( return answers.map((entry) => `${entry.header}: ${entry.answer}\n ${entry.question}`).join("\n"); } +function askUserQuestionCompletedRawInput( + prompts: AskUserQuestionPrompt[], + answers: Record, + annotations: Record, + questionResults: Json[], +): Json { + return { + questions: prompts.map((prompt) => ({ + question: prompt.question, + header: prompt.header, + multiSelect: prompt.multiSelect, + options: prompt.options.map((option) => ({ + label: option.label, + description: option.description, + ...(option.preview ? { preview: option.preview } : {}), + })), + })), + answers, + ...(Object.keys(annotations).length > 0 ? { annotations: questionAnnotationsJson(annotations) } : {}), + question_results: questionResults, + }; +} + +function questionAnnotationsJson(annotations: Record): { [key: string]: Json } { + return Object.fromEntries( + Object.entries(annotations).map(([question, annotation]) => [ + question, + { + ...(annotation.preview ? { preview: annotation.preview } : {}), + ...(annotation.notes ? { notes: annotation.notes } : {}), + }, + ]), + ); +} + function deriveAnnotation( selectedOptions: QuestionOption[], annotation?: QuestionAnnotation, @@ -244,6 +279,7 @@ export async function requestAskUserQuestionAnswers( const answers: Record = {}; const annotations: Record = {}; const transcript: Array<{ header: string; question: string; answer: string }> = []; + const questionResults: Json[] = []; for (const [index, prompt] of prompts.entries()) { const promptToolCall = askUserQuestionPromptToolCall(baseToolCall, prompt, index, prompts.length); @@ -301,12 +337,36 @@ export async function requestAskUserQuestionAnswers( annotations[prompt.question] = annotation; } transcript.push({ header: prompt.header, question: prompt.question, answer }); + questionResults.push({ + question: prompt.question, + header: prompt.header, + question_index: index, + total_questions: prompts.length, + selected_options: selectedOptions.map((option) => ({ + option_id: option.option_id, + label: option.label, + ...(option.description ? { description: option.description } : {}), + ...(option.preview ? { preview: option.preview } : {}), + })), + ...(annotation + ? { + annotation: { + ...(annotation.preview ? { preview: annotation.preview } : {}), + ...(annotation.notes ? { notes: annotation.notes } : {}), + }, + } + : {}), + }); const summary = askUserQuestionTranscript(transcript); + const completed = index + 1 >= prompts.length; const progressFields: ToolCallUpdateFields = { - status: index + 1 >= prompts.length ? "completed" : "in_progress", + status: completed ? "completed" : "in_progress", raw_output: summary, content: [{ type: "content", content: { type: "text", text: summary } }], + ...(completed + ? { raw_input: askUserQuestionCompletedRawInput(prompts, answers, annotations, questionResults) } + : {}), }; emitToolCallUpdate(session, toolUseId, progressFields, "summary"); } diff --git a/src/app/events/tests.rs b/src/app/events/tests.rs index 02064bf9..aea52cd3 100644 --- a/src/app/events/tests.rs +++ b/src/app/events/tests.rs @@ -4066,6 +4066,39 @@ fn typing_reclaims_input_from_auto_focused_question() { assert_eq!(question_focus_state(&app, "question-auto"), Some(false)); } +#[test] +fn space_toggles_focused_question_without_reclaiming_input() { + let mut app = make_test_app(); + let _response_rx = attach_pending_question( + &mut app, + "question-space", + model::QuestionPrompt::new( + "Choose drinks", + "Drinks", + true, + vec![ + model::QuestionOption::new("coffee", "Coffee"), + model::QuestionOption::new("tea", "Tea"), + ], + ), + true, + ); + + handle_terminal_event( + &mut app, + Event::Key(KeyEvent::new(KeyCode::Char(' '), KeyModifiers::NONE)), + ); + + assert_eq!(app.focus_owner(), FocusOwner::Permission); + assert_eq!(app.input.text(), ""); + let (mi, bi) = app.lookup_tool_call("question-space").expect("question tool call"); + let MessageBlock::ToolCall(tc) = app.messages.get(mi).unwrap().blocks.get(bi).unwrap() else { + panic!("expected tool call block"); + }; + let question = tc.pending_question.as_ref().expect("pending question"); + assert!(question.selected_option_indices.contains(&0)); +} + #[test] fn stale_inline_interaction_queue_head_is_pruned_before_enter_response() { let mut app = make_test_app(); diff --git a/src/app/input.rs b/src/app/input.rs index 14f0734a..cb8d27fe 100644 --- a/src/app/input.rs +++ b/src/app/input.rs @@ -571,30 +571,6 @@ fn normalize_line_endings(text: &str) -> String { out } -/// Count logical lines for text containing mixed `\n`, `\r`, and `\r\n` endings. -#[cfg(test)] -#[must_use] -fn count_text_lines(text: &str) -> usize { - // Count universal newlines (\n, \r, and \r\n as a single break). - let mut lines = 1; - let bytes = text.as_bytes(); - let mut i = 0; - while i < bytes.len() { - match bytes[i] { - b'\n' => lines += 1, - b'\r' => { - lines += 1; - if i + 1 < bytes.len() && bytes[i + 1] == b'\n' { - i += 1; - } - } - _ => {} - } - i += 1; - } - lines -} - /// Count Unicode scalar characters in a text payload. #[must_use] pub fn count_text_chars(text: &str) -> usize { @@ -1684,13 +1660,6 @@ mod tests { assert!(!input.append_to_active_paste_block("x")); } - #[test] - fn count_text_lines_handles_mixed_line_endings() { - assert_eq!(count_text_lines("a\r\nb\nc\rd"), 4); - assert_eq!(count_text_lines("single"), 1); - assert_eq!(count_text_lines("x\r\n"), 2); - } - #[test] fn count_text_chars_counts_unicode_scalars() { assert_eq!(count_text_chars("abc"), 3); diff --git a/src/app/keys.rs b/src/app/keys.rs index c63197f3..ae3f0d7a 100644 --- a/src/app/keys.rs +++ b/src/app/keys.rs @@ -213,6 +213,13 @@ fn should_reclaim_input_focus_before_inline_interaction(app: &App, key: KeyEvent let question_notes_editing = questions::focused_question_is_editing_notes(app); match key.code { KeyCode::Backspace | KeyCode::Delete => !question_notes_editing, + KeyCode::Char(' ') + if questions::has_focused_question(app) + && is_printable_text_modifiers(key.modifiers) + && !question_notes_editing => + { + false + } KeyCode::Char(_) if is_printable_text_modifiers(key.modifiers) => !question_notes_editing, _ => false, } diff --git a/src/app/terminal_runtime/mod.rs b/src/app/terminal_runtime/mod.rs index 306ab506..37cb0dfa 100644 --- a/src/app/terminal_runtime/mod.rs +++ b/src/app/terminal_runtime/mod.rs @@ -23,13 +23,6 @@ use std::sync::atomic::{AtomicBool, Ordering}; pub(crate) use release_guard::TerminalReleaseGuard; -#[cfg(test)] -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum SurfaceSessionKind { - Chat, - Fullscreen, -} - enum SurfaceTerminalSession { Chat(ChatTerminalSession), Fullscreen(FullscreenTerminalSession), @@ -280,14 +273,6 @@ fn restore_terminal_modes(alternate_screen_active: &AtomicBool) -> std::io::Resu } } -#[cfg(test)] -fn session_kind_for_surface(surface: SurfaceMode) -> SurfaceSessionKind { - match surface { - SurfaceMode::Chat => SurfaceSessionKind::Chat, - SurfaceMode::Fullscreen(_) => SurfaceSessionKind::Fullscreen, - } -} - fn plan_surface_transition(from: SurfaceMode, to: SurfaceMode) -> SurfaceTransitionPlan { match (from, to) { (SurfaceMode::Chat, SurfaceMode::Fullscreen(view)) => { diff --git a/src/app/terminal_runtime/tests.rs b/src/app/terminal_runtime/tests.rs index e1c5d7cb..add99e30 100644 --- a/src/app/terminal_runtime/tests.rs +++ b/src/app/terminal_runtime/tests.rs @@ -16,15 +16,6 @@ fn draw_fullscreen_surface_frame_supports_fullscreen_retained_views() { draw_fullscreen_surface_frame(&mut terminal, &mut app).expect("draw fullscreen view"); } -#[test] -fn session_kind_matches_surface_mode() { - assert_eq!(session_kind_for_surface(SurfaceMode::Chat), SurfaceSessionKind::Chat); - assert_eq!( - session_kind_for_surface(SurfaceMode::Fullscreen(FullscreenView::Trusted)), - SurfaceSessionKind::Fullscreen - ); -} - #[test] fn surface_transition_plan_is_noop_for_chat_to_chat() { assert_eq!( diff --git a/src/ui/config.rs b/src/ui/config.rs index 68411f4e..999c8b76 100644 --- a/src/ui/config.rs +++ b/src/ui/config.rs @@ -1009,20 +1009,6 @@ struct CapabilityBadge { fg: Color, } -#[cfg(test)] -fn model_overlay_title_text( - option: &crate::app::config::OverlayModelOption, - marker: &str, -) -> String { - let badges = model_capability_badges(option); - let mut title = format!("{marker} {}", option.display_name); - if !badges.is_empty() { - title.push_str(" "); - title.push_str(&badges.into_iter().map(|badge| badge.label).collect::>().join(" ")); - } - title -} - fn model_overlay_title_line( option: &crate::app::config::OverlayModelOption, marker: &str, @@ -1120,7 +1106,7 @@ fn render_tab_header(frame: &mut Frame, area: Rect, active_tab: ConfigTab) { mod tests { use super::{ SETTINGS_LIMITATION_HINT, effort_overlay_scroll, model_overlay_lines, model_overlay_scroll, - model_overlay_title_line, model_overlay_title_text, + model_overlay_title_line, }; use crate::agent::model::{AvailableModel, EffortLevel}; use crate::app::App; @@ -1420,8 +1406,8 @@ mod tests { } #[test] - fn model_overlay_title_text_uses_human_labels_without_divider() { - let title = model_overlay_title_text( + fn model_overlay_title_line_uses_human_badge_labels_without_divider() { + let line = model_overlay_title_line( &crate::app::config::OverlayModelOption { id: "sonnet".to_owned(), display_name: "Sonnet".to_owned(), @@ -1433,12 +1419,21 @@ mod tests { supports_auto_mode: Some(false), }, ">", + false, + false, ); + let title = line.spans.iter().map(|span| span.content.as_ref()).collect::(); + let badge_labels = line + .spans + .iter() + .map(|span| span.content.trim()) + .filter(|content| !content.is_empty()) + .collect::>(); assert!(title.starts_with("> Sonnet")); - assert!(title.contains("Effort")); - assert!(title.contains("Adaptive thinking")); - assert!(title.contains("Fast mode")); + assert!(badge_labels.contains(&"Effort")); + assert!(badge_labels.contains(&"Adaptive thinking")); + assert!(badge_labels.contains(&"Fast mode")); assert!(!title.contains("Auto mode")); assert!(!title.contains('[')); assert!(!title.contains('|')); diff --git a/src/ui/inline_chat_rows.rs b/src/ui/inline_chat_rows.rs index f515e35b..a257d36f 100644 --- a/src/ui/inline_chat_rows.rs +++ b/src/ui/inline_chat_rows.rs @@ -933,7 +933,8 @@ fn render_assistant_rows(mut request: AssistantRowsRequest<'_>) -> RenderedMessa has_visible_content: request.has_prior_assistant_content, }; - for item in request.items { + let mut items = request.items.into_iter().peekable(); + while let Some(item) = items.next() { let boundary = AssistantBoundaryMeta { ids: item.ids, msg_idx: item.msg_idx, @@ -944,7 +945,7 @@ fn render_assistant_rows(mut request: AssistantRowsRequest<'_>) -> RenderedMessa let item_leading_blank_lines = item.leading_blank_lines; match item.item { AssistantRenderItem::Text(block) => { - let trailing_gap = block.trailing_blank_lines(); + let trailing_gap = assistant_text_trailing_gap(&block, items.peek()); let rendered = render_assistant_text_block(block, request.width, !state.has_visible_content); if !rendered.is_empty() { @@ -960,7 +961,7 @@ fn render_assistant_rows(mut request: AssistantRowsRequest<'_>) -> RenderedMessa } } AssistantRenderItem::Notice(block) => { - let trailing_gap = block.trailing_blank_lines(); + let trailing_gap = assistant_text_trailing_gap(&block.text, items.peek()); let rendered = render_assistant_notice_block(block, request.width, !state.has_visible_content); if !rendered.is_empty() { @@ -1017,6 +1018,86 @@ fn render_assistant_rows(mut request: AssistantRowsRequest<'_>) -> RenderedMessa RenderedMessageRows::rendered(trim_trailing_blank_rows(rows), boundaries) } +fn assistant_text_trailing_gap( + block: &TextBlock, + next_item: Option<&AssistantRenderItemSpec>, +) -> usize { + let gap = block.trailing_blank_lines(); + if gap == 0 { + return 0; + } + + let next_text = next_item.and_then(assistant_item_text); + if text_boundary_touches_markdown_list(&block.text, next_text) { 0 } else { gap } +} + +fn assistant_item_text(item: &AssistantRenderItemSpec) -> Option<&TextBlock> { + match &item.item { + AssistantRenderItem::Text(block) => Some(block), + AssistantRenderItem::Notice(block) => Some(&block.text), + AssistantRenderItem::CanonicalTool { .. } => None, + } +} + +fn text_boundary_touches_markdown_list(current: &str, next: Option<&TextBlock>) -> bool { + markdown_source_ends_with_list_item(current) + || next.is_some_and(|block| markdown_source_starts_with_list_item(&block.text)) +} + +fn markdown_source_starts_with_list_item(text: &str) -> bool { + markdown_source_boundary_line(text, BoundaryLine::First) + .is_some_and(markdown_source_line_is_list_item) +} + +fn markdown_source_ends_with_list_item(text: &str) -> bool { + markdown_source_boundary_line(text, BoundaryLine::Last) + .is_some_and(markdown_source_line_is_list_item) +} + +#[derive(Clone, Copy)] +enum BoundaryLine { + First, + Last, +} + +fn markdown_source_boundary_line(text: &str, boundary: BoundaryLine) -> Option<&str> { + let mut in_fenced_code = false; + let mut first = None; + let mut last = None; + + for line in text.lines() { + let trimmed = line.trim(); + let is_fence = trimmed.starts_with("```") || trimmed.starts_with("~~~"); + let in_code_line = in_fenced_code || is_fence; + if is_fence { + in_fenced_code = !in_fenced_code; + } + if in_code_line || trimmed.is_empty() { + continue; + } + first.get_or_insert(line); + last = Some(line); + } + + match boundary { + BoundaryLine::First => first, + BoundaryLine::Last => last, + } +} + +fn markdown_source_line_is_list_item(line: &str) -> bool { + let trimmed = line.trim_start(); + trimmed.starts_with("- ") + || trimmed.starts_with("* ") + || trimmed.starts_with("+ ") + || markdown_source_starts_with_ordered_list_marker(trimmed) +} + +fn markdown_source_starts_with_ordered_list_marker(text: &str) -> bool { + let digit_count = text.bytes().take_while(u8::is_ascii_digit).count(); + digit_count > 0 && text[digit_count..].starts_with(". ") +} + fn append_assistant_label_rows( rows: &mut Vec>, boundaries: &mut Vec, @@ -1698,6 +1779,38 @@ mod tests { assert_eq!(line_texts(&rows), vec!["Claude", "line 1: ready", "", "line 2: ready"]); } + #[test] + fn live_assistant_text_suppresses_paragraph_gap_before_list_block() { + let mut app = App::test_default(); + app.messages.push(assistant_blocks_message(vec![ + MessageBlock::Text( + TextBlock::from_complete("Intro\n\n") + .with_trailing_spacing(TextBlockSpacing::ParagraphBreak), + ), + MessageBlock::Text(TextBlock::from_complete("- One\n- Two")), + ])); + + let rows = serialize_live_rows(&mut app, 120); + + assert_eq!(line_texts(&rows), vec!["Claude", "Intro", " - One", " - Two"]); + } + + #[test] + fn live_assistant_text_suppresses_paragraph_gap_after_list_block() { + let mut app = App::test_default(); + app.messages.push(assistant_blocks_message(vec![ + MessageBlock::Text( + TextBlock::from_complete("- One\n- Two\n\n") + .with_trailing_spacing(TextBlockSpacing::ParagraphBreak), + ), + MessageBlock::Text(TextBlock::from_complete("Outro")), + ])); + + let rows = serialize_live_rows(&mut app, 120); + + assert_eq!(line_texts(&rows), vec!["Claude", " - One", " - Two", "Outro"]); + } + #[test] fn live_assistant_text_preserves_single_newline_rows() { let mut app = App::test_default(); diff --git a/src/ui/markdown.rs b/src/ui/markdown.rs index 08460dc3..5c8754f4 100644 --- a/src/ui/markdown.rs +++ b/src/ui/markdown.rs @@ -28,7 +28,7 @@ where fn render_with_tui_markdown(text: &str, bg: Option) -> Vec> { let rendered = tui_markdown::from_str(text); - rendered + let lines = rendered .lines .into_iter() .map(|line| { @@ -45,7 +45,71 @@ fn render_with_tui_markdown(text: &str, bg: Option) -> Vec> if let Some(bg_color) = bg { line.style.bg(bg_color) } else { line.style }; Line::from(owned_spans).style(line_style) }) - .collect() + .collect(); + normalize_list_spacing(lines) +} + +fn normalize_list_spacing(lines: Vec>) -> Vec> { + let list_lines = rendered_list_line_flags(&lines); + let mut normalized = Vec::with_capacity(lines.len()); + + for (idx, line) in lines.into_iter().enumerate() { + if line_is_blank(&line) { + let before_list = list_lines.get(idx + 1).copied().unwrap_or(false); + let after_list = idx > 0 && list_lines.get(idx - 1).copied().unwrap_or(false); + if before_list || after_list { + continue; + } + } + + if list_lines[idx] { + normalized.push(indent_line(line, " ")); + } else { + normalized.push(line); + } + } + + normalized +} + +fn rendered_list_line_flags(lines: &[Line<'_>]) -> Vec { + let mut in_fenced_code = false; + let mut flags = Vec::with_capacity(lines.len()); + + for line in lines { + let text = line_text(line); + let starts_fence = text.trim_start().starts_with("```"); + let in_code_line = in_fenced_code || starts_fence; + flags.push(!in_code_line && rendered_line_is_list_item(&text)); + if starts_fence { + in_fenced_code = !in_fenced_code; + } + } + + flags +} + +fn rendered_line_is_list_item(text: &str) -> bool { + let trimmed = text.trim_start(); + trimmed.starts_with("- ") || starts_with_ordered_list_marker(trimmed) +} + +fn starts_with_ordered_list_marker(text: &str) -> bool { + let digit_count = text.bytes().take_while(u8::is_ascii_digit).count(); + digit_count > 0 && text[digit_count..].starts_with(". ") +} + +fn indent_line(mut line: Line<'static>, indent: &'static str) -> Line<'static> { + line.spans.insert(0, Span::styled(indent, line.style)); + line +} + +fn line_is_blank(line: &Line<'_>) -> bool { + line.spans.iter().all(|span| span.content.trim().is_empty()) +} + +fn line_text(line: &Line<'_>) -> String { + line.spans.iter().map(|span| span.content.as_ref()).collect() } fn plain_text_fallback(text: &str, bg: Option) -> Vec> { @@ -60,6 +124,10 @@ mod tests { use super::*; use std::panic::catch_unwind; + fn rendered_text(lines: &[Line<'_>]) -> Vec { + lines.iter().map(line_text).collect() + } + #[test] fn render_markdown_safe_handles_common_and_edge_case_inputs_without_panicking() { let inputs = [ @@ -94,4 +162,48 @@ mod tests { assert_eq!(lines[0].spans[0].style.bg, Some(Color::Blue)); assert_eq!(lines[1].spans[0].style.bg, Some(Color::Blue)); } + + #[test] + fn unordered_lists_use_indentation_instead_of_boundary_blank_lines() { + let lines = render_markdown_safe("Intro\n\n- One\n- Two\n\nOutro", None); + + assert_eq!(rendered_text(&lines), vec!["Intro", " - One", " - Two", "Outro"]); + } + + #[test] + fn ordered_lists_use_indentation_instead_of_boundary_blank_lines() { + let lines = render_markdown_safe("Intro\n\n1. One\n2. Two\n\nOutro", None); + + assert_eq!(rendered_text(&lines), vec!["Intro", " 1. One", " 2. Two", "Outro"]); + } + + #[test] + fn nested_lists_keep_relative_indentation() { + let lines = render_markdown_safe("- Parent\n - Child", None); + + assert_eq!(rendered_text(&lines), vec![" - Parent", " - Child"]); + } + + #[test] + fn task_lists_keep_markers_after_indentation() { + let lines = render_markdown_safe("- [ ] Todo\n- [x] Done", None); + + assert_eq!(rendered_text(&lines), vec![" - [ ] Todo", " - [x] Done"]); + } + + #[test] + fn fenced_code_keeps_list_like_lines_unchanged() { + let lines = render_markdown_safe("```md\n- Not a rendered list\n```", None); + + assert_eq!(rendered_text(&lines), vec!["```md", "- Not a rendered list", "```"]); + } + + #[test] + fn list_indent_preserves_requested_background() { + let lines = render_markdown_safe("- One", Some(Color::Blue)); + + assert_eq!(rendered_text(&lines), vec![" - One"]); + assert_eq!(lines[0].spans[0].style.bg, Some(Color::Blue)); + assert!(lines[0].spans.iter().all(|span| span.style.bg == Some(Color::Blue))); + } } diff --git a/src/ui/tool_call/interactions.rs b/src/ui/tool_call/interactions.rs index c5a01e46..90782891 100644 --- a/src/ui/tool_call/interactions.rs +++ b/src/ui/tool_call/interactions.rs @@ -338,17 +338,15 @@ pub(super) fn render_question_lines(question: &InlineQuestion) -> Vec Vec Vec Vec &'static str { + if question.prompt.multi_select { " " } else { " " } +} + +fn option_description_suffix(description: Option<&str>) -> Span<'static> { + description.map(str::trim).filter(|desc| !desc.is_empty()).map_or_else( + || Span::raw(""), + |desc| { + Span::styled(format!(" - {}", single_line_text(desc)), Style::default().fg(theme::DIM)) + }, + ) +} + +fn single_line_text(text: &str) -> String { + text.split_whitespace().collect::>().join(" ") +} + #[cfg(test)] mod tests { use super::{render_permission_lines, render_plan_approval_lines, render_question_lines}; @@ -507,6 +525,48 @@ mod tests { assert_eq!(lines[2].spans[0].style.fg, Some(Color::Gray)); } + #[test] + fn focused_question_option_description_stays_on_option_line() { + let mut question = test_question(); + question.prompt.options[0] = + QuestionOption::new("safe", "Safer path").description(Some("Avoids churn.".to_owned())); + + let rendered = render_question_lines(&question) + .into_iter() + .map(|line| line.spans.into_iter().map(|span| span.content.into_owned()).collect()) + .collect::>(); + + assert!(rendered.iter().any(|line| line.contains("Safer path - Avoids churn."))); + assert!(!rendered.iter().any(|line| line.trim() == "Avoids churn.")); + let first_option_idx = rendered + .iter() + .position(|line| line.contains("Safer path - Avoids churn.")) + .expect("first option line"); + assert_eq!(rendered.get(first_option_idx + 1).map(String::as_str), Some("")); + assert!( + rendered.get(first_option_idx + 2).is_some_and(|line| line.contains("Faster path")) + ); + } + + #[test] + fn focused_question_preview_and_notes_align_with_option_text() { + let mut question = test_question(); + question.prompt.multi_select = true; + question.prompt.options[0] = QuestionOption::new("safe", "Safer path") + .description(Some("Avoids churn.".to_owned())) + .preview(Some("Preview text".to_owned())); + question.notes = "note text".to_owned(); + + let rendered = render_question_lines(&question) + .into_iter() + .map(|line| line.spans.into_iter().map(|span| span.content.into_owned()).collect()) + .collect::>(); + + assert!(rendered.iter().any(|line| line.contains(" Preview"))); + assert!(rendered.iter().any(|line| line.contains(" Preview text"))); + assert!(rendered.iter().any(|line| line.contains(" Notes: note text"))); + } + #[test] fn selected_permission_option_uses_orange_label() { let tc = test_tool_call("Bash"); diff --git a/src/ui/tool_call/mod.rs b/src/ui/tool_call/mod.rs index 0871c8ef..3938294f 100644 --- a/src/ui/tool_call/mod.rs +++ b/src/ui/tool_call/mod.rs @@ -239,6 +239,10 @@ fn tool_display_title<'a>( tc: &'a ToolCallInfo, render_context: ToolCallRenderContext<'_>, ) -> Cow<'a, str> { + if tc.is_ask_question_tool() { + return ask_user_question_display_title(tc); + } + if render_context.current_mode_id == Some("plan") { match tc.sdk_tool_name.as_str() { "Write" => return Cow::Borrowed("Create Plan"), @@ -250,6 +254,29 @@ fn tool_display_title<'a>( tool_display::tool_title(&tc.sdk_tool_name, &tc.title) } +fn ask_user_question_display_title(tc: &ToolCallInfo) -> Cow<'static, str> { + if let Some(question) = &tc.pending_question { + return if question.total_questions > 1 { + Cow::Owned(format!("Questions ({})", question.total_questions)) + } else { + Cow::Borrowed("Question") + }; + } + + let result_count = tc + .raw_input + .as_ref() + .and_then(|input| input.get("question_results")) + .and_then(serde_json::Value::as_array) + .map_or(0, Vec::len); + + match result_count { + 0 => Cow::Borrowed("Question"), + 1 => Cow::Borrowed("Answered question"), + count => Cow::Owned(format!("Answered questions ({count})")), + } +} + // --------------------------------------------------------------------------- // Tests // --------------------------------------------------------------------------- diff --git a/src/ui/tool_call/standard.rs b/src/ui/tool_call/standard.rs index e8c253cd..2fb15619 100644 --- a/src/ui/tool_call/standard.rs +++ b/src/ui/tool_call/standard.rs @@ -13,6 +13,7 @@ use crate::ui::tool_display; use crate::ui::wrap::wrap_lines_to_physical_rows; use ratatui::style::{Modifier, Style}; use ratatui::text::{Line, Span}; +use serde_json::Value; use std::path::Path; use two_face::theme::EmbeddedThemeName; @@ -38,6 +39,7 @@ const DIFF_BODY_INDENT_WIDTH: u16 = 2; const STANDARD_BODY_PREFIX_WIDTH: u16 = 5; const EXECUTE_BODY_INDENT: &str = " "; const EXECUTE_BODY_INDENT_WIDTH: u16 = 6; +const ASK_USER_QUESTION_RESULT_TEXT_INDENT: &str = " "; const READ_SYNTAX_THEME: EmbeddedThemeName = EmbeddedThemeName::MonokaiExtendedBright; /// Render just the title line for a tool call (the line containing the spinner icon). @@ -164,6 +166,7 @@ pub(super) fn tool_call_has_body(tc: &ToolCallInfo) -> bool { !tc.content.is_empty() || tc.pending_permission.is_some() || tc.pending_question.is_some() + || renders_structured_ask_user_question_result(tc) || (tc.is_execute_tool() && (tc.terminal_output.is_some() || matches!(tc.status, model::ToolCallStatus::InProgress))) @@ -317,7 +320,9 @@ fn truncate_summary_line(line: &str, max_chars: usize) -> String { /// Render the full content of a tool call as lines. fn render_tool_content(tc: &ToolCallInfo, width: u16) -> Vec> { - let mut lines: Vec> = Vec::new(); + if hides_pending_ask_user_question_transcript(tc) { + return Vec::new(); + } if tasks::is_state_tool(tc) && let Some(task_lines) = tasks::render_tool_content(tc) @@ -376,9 +381,11 @@ fn render_tool_content(tc: &ToolCallInfo, width: u16) -> Vec> { } if tc.is_execute_tool() { - lines.extend(execute::render_execute_content(tc)); - debug_failed_tool_render(tc); - return lines; + return render_execute_tool_content(tc); + } + + if let Some(question_lines) = render_completed_ask_user_question_content(tc) { + return question_lines; } if tool_body_uses_summary_only(tc) { @@ -390,6 +397,7 @@ fn render_tool_content(tc: &ToolCallInfo, width: u16) -> Vec> { }; } + let mut lines: Vec> = Vec::new(); for content in &tc.content { match content { model::ToolCallContent::Diff(diff) => { @@ -420,6 +428,153 @@ fn render_tool_content(tc: &ToolCallInfo, width: u16) -> Vec> { lines } +fn render_execute_tool_content(tc: &ToolCallInfo) -> Vec> { + let lines = execute::render_execute_content(tc); + debug_failed_tool_render(tc); + lines +} + +fn render_completed_ask_user_question_content(tc: &ToolCallInfo) -> Option>> { + if tc.is_ask_question_tool() + && tc.pending_question.is_none() + && matches!(tc.status, model::ToolCallStatus::Completed) + { + render_ask_user_question_result(tc) + } else { + None + } +} + +fn hides_pending_ask_user_question_transcript(tc: &ToolCallInfo) -> bool { + tc.is_ask_question_tool() && tc.pending_question.is_some() +} + +fn render_ask_user_question_result(tc: &ToolCallInfo) -> Option>> { + let results = tc.raw_input.as_ref()?.get("question_results")?.as_array()?; + if results.is_empty() { + return None; + } + + let mut lines = Vec::new(); + for (idx, result) in results.iter().enumerate() { + if idx > 0 { + lines.push(Line::default()); + } + render_ask_user_question_result_entry(result, &mut lines); + } + + (!lines.is_empty()).then_some(lines) +} + +fn render_ask_user_question_result_entry(result: &Value, lines: &mut Vec>) { + let header = json_str(result, "header").unwrap_or("Question"); + let question = json_str(result, "question"); + let title = ask_user_question_result_title( + header, + json_usize(result, "question_index"), + json_usize(result, "total_questions"), + ); + + lines.push(Line::from(vec![ + Span::styled("? ", Style::default().fg(theme::RUST_ORANGE)), + Span::styled(title, Style::default().add_modifier(Modifier::BOLD)), + ])); + + if let Some(question) = question { + for row in question.lines() { + lines.push(Line::from(Span::styled( + format!(" {row}"), + Style::default().fg(theme::DIM), + ))); + } + } + + let selected_options: &[Value] = + result.get("selected_options").and_then(Value::as_array).map_or(&[], Vec::as_slice); + if !selected_options.is_empty() { + lines.push(Line::default()); + for option in selected_options { + render_selected_question_option(option, lines); + } + } + + render_question_annotation_section(result, "preview", "Preview", lines); + render_question_annotation_section(result, "notes", "Notes", lines); +} + +fn ask_user_question_result_title( + header: &str, + question_index: Option, + total_questions: Option, +) -> String { + match (question_index, total_questions) { + (Some(index), Some(total)) if total > 1 => format!("{header} ({}/{total})", index + 1), + _ => header.to_owned(), + } +} + +fn render_selected_question_option(option: &Value, lines: &mut Vec>) { + let Some(label) = json_str(option, "label") else { + return; + }; + let mut spans = vec![ + Span::styled(" [x] ", Style::default().fg(theme::DIM)), + Span::styled(label.to_owned(), Style::default().add_modifier(Modifier::BOLD)), + ]; + + if let Some(description) = json_str(option, "description").map(single_line_text) { + spans.push(Span::styled(" - ", Style::default().fg(theme::DIM))); + spans.push(Span::styled(description, Style::default().fg(theme::DIM))); + } + lines.push(Line::from(spans)); +} + +fn render_question_annotation_section( + result: &Value, + key: &str, + title: &str, + lines: &mut Vec>, +) { + let Some(text) = result.get("annotation").and_then(|annotation| json_str(annotation, key)) + else { + return; + }; + + lines.push(Line::default()); + if key == "notes" { + lines.push(Line::from(vec![ + Span::styled( + format!("{ASK_USER_QUESTION_RESULT_TEXT_INDENT}{title}: "), + Style::default().add_modifier(Modifier::BOLD), + ), + Span::styled(single_line_text(text), Style::default().fg(theme::DIM)), + ])); + } else { + lines.push(Line::from(Span::styled( + format!("{ASK_USER_QUESTION_RESULT_TEXT_INDENT}{title}"), + Style::default().add_modifier(Modifier::BOLD), + ))); + for row in text.lines() { + lines.push(Line::from(Span::styled( + format!("{ASK_USER_QUESTION_RESULT_TEXT_INDENT} {row}"), + Style::default().fg(theme::DIM), + ))); + } + } +} + +fn single_line_text(text: &str) -> String { + text.split_whitespace().collect::>().join(" ") +} + +fn json_str<'a>(value: &'a Value, key: &str) -> Option<&'a str> { + value.get(key)?.as_str().map(str::trim).filter(|text| !text.is_empty()) +} + +fn json_usize(value: &Value, key: &str) -> Option { + value.get(key)?.as_u64().and_then(|number| usize::try_from(number).ok()) +} + fn tool_body_uses_summary_only(tc: &ToolCallInfo) -> bool { tc.is_exit_plan_mode_tool() || matches!(tc.sdk_tool_name.as_str(), "Agent" | "Task" | "WebSearch" | "WebFetch") @@ -432,13 +587,25 @@ enum ToolContentHeightPolicy { } fn tool_content_height_policy(tc: &ToolCallInfo) -> ToolContentHeightPolicy { - if renders_only_plan_file_content(tc) { + if renders_only_plan_file_content(tc) || renders_structured_ask_user_question_result(tc) { ToolContentHeightPolicy::Unbounded } else { ToolContentHeightPolicy::Bounded } } +fn renders_structured_ask_user_question_result(tc: &ToolCallInfo) -> bool { + tc.is_ask_question_tool() + && tc.pending_question.is_none() + && matches!(tc.status, model::ToolCallStatus::Completed) + && tc + .raw_input + .as_ref() + .and_then(|input| input.get("question_results")) + .and_then(Value::as_array) + .is_some_and(|results| !results.is_empty()) +} + fn renders_only_plan_file_content(tc: &ToolCallInfo) -> bool { let mut saw_plan_file = false; diff --git a/src/ui/tool_call/tests.rs b/src/ui/tool_call/tests.rs index 77c35de6..28bd131d 100644 --- a/src/ui/tool_call/tests.rs +++ b/src/ui/tool_call/tests.rs @@ -227,6 +227,49 @@ fn tool_display_title_uses_plan_aliases() { assert_eq!(tool_display_title(&read, plan), "tc-plan-read"); } +#[test] +fn tool_display_title_uses_stable_question_title_for_pending_ask_user_question() { + let mut tc = test_tool_call( + "What is your favorite language?", + "AskUserQuestion", + model::ToolCallStatus::InProgress, + ); + let (response_tx, _response_rx) = tokio::sync::oneshot::channel(); + tc.pending_question = Some(crate::app::InlineQuestion { + prompt: model::QuestionPrompt::new( + "What is your favorite language?", + "Language", + false, + vec![model::QuestionOption::new("rust", "Rust")], + ), + response_tx, + focused_option_index: 0, + selected_option_indices: std::collections::BTreeSet::new(), + notes: String::new(), + notes_cursor: 0, + editing_notes: false, + focused: true, + question_index: 0, + total_questions: 4, + }); + + assert_eq!(tool_display_title(&tc, ToolCallRenderContext::default()), "Questions (4)"); +} + +#[test] +fn tool_display_title_uses_answered_questions_title_for_completed_ask_user_question() { + let mut tc = + test_tool_call("Which config format?", "AskUserQuestion", model::ToolCallStatus::Completed); + tc.raw_input = Some(serde_json::json!({ + "question_results": [ + { "question": "First?", "header": "First", "selected_options": [] }, + { "question": "Second?", "header": "Second", "selected_options": [] } + ] + })); + + assert_eq!(tool_display_title(&tc, ToolCallRenderContext::default()), "Answered questions (2)"); +} + #[test] fn tool_display_title_formats_raw_mcp_titles() { let tc = test_tool_call( @@ -392,6 +435,153 @@ fn markdown_read_body_uses_markdown_renderer() { assert!(!rendered.contains("**")); } +#[test] +fn ask_user_question_completed_body_renders_structured_answers() { + let mut tc = + test_tool_call("AskUserQuestion", "AskUserQuestion", model::ToolCallStatus::Completed); + tc.raw_input = Some(serde_json::json!({ + "question_results": [ + { + "question": "Pick deployment target", + "header": "Target", + "question_index": 0, + "total_questions": 2, + "selected_options": [ + { + "option_id": "question_0", + "label": "Staging", + "description": "Low-risk validation", + "preview": "Deploy to staging first." + } + ], + "annotation": { + "preview": "Deploy to staging first.", + "notes": "Roll out here before production." + } + }, + { + "question": "When should this run?", + "header": "Timing", + "question_index": 1, + "total_questions": 2, + "selected_options": [ + { + "option_id": "question_1", + "label": "After tests pass", + "description": "" + } + ] + } + ] + })); + tc.content = + vec![model::ToolCallContent::from("Target: Staging\n Pick deployment target".to_owned())]; + + let body = standard::render_tool_call_body(&tc, 120); + let rendered = rendered_line_texts_trimmed(&body); + let joined = rendered.join("\n"); + + assert!(joined.contains("? Target (1/2)")); + assert!(joined.contains("Pick deployment target")); + assert!(joined.contains("[x] Staging - Low-risk validation")); + assert!(joined.contains("Preview")); + assert!(joined.contains("Deploy to staging first.")); + assert!(joined.contains("Notes: Roll out here before production.")); + assert!(joined.contains("? Timing (2/2)")); + assert!(joined.contains("[x] After tests pass")); + assert!(!joined.contains("Target: Staging")); +} + +#[test] +fn ask_user_question_completed_body_indents_preview_and_notes_with_answers() { + let mut tc = + test_tool_call("AskUserQuestion", "AskUserQuestion", model::ToolCallStatus::Completed); + tc.raw_input = Some(serde_json::json!({ + "question_results": [ + { + "question": "Which config format do you prefer?", + "header": "Config fmt", + "question_index": 0, + "total_questions": 1, + "selected_options": [ + { + "option_id": "question_0", + "label": "JSON", + "description": "Ubiquitous, no comments." + } + ], + "annotation": { + "preview": "{\n \"server\": { \"host\": \"127.0.0.1\" }\n}", + "notes": "Use this for generated config." + } + } + ] + })); + + let body = standard::render_tool_call_body(&tc, 120); + let rendered = rendered_line_texts_trimmed(&body); + + assert!(rendered.iter().any(|line| line.contains(" [x] JSON - Ubiquitous, no comments."))); + assert!(rendered.iter().any(|line| line.contains(" Preview"))); + assert!(rendered.iter().any(|line| line.contains(" {"))); + assert!( + rendered.iter().any(|line| line.contains(" Notes: Use this for generated config.")) + ); +} + +#[test] +fn ask_user_question_pending_body_hides_answer_transcript() { + let mut tc = + test_tool_call("AskUserQuestion", "AskUserQuestion", model::ToolCallStatus::InProgress); + tc.content = vec![model::ToolCallContent::from( + "Log level: info\n What log level should ship as the default?".to_owned(), + )]; + let (response_tx, _response_rx) = tokio::sync::oneshot::channel(); + tc.pending_question = Some(crate::app::InlineQuestion { + prompt: model::QuestionPrompt::new( + "Which features should be enabled by default?", + "Features", + true, + vec![ + model::QuestionOption::new("question_0", "Streaming") + .description(Some("Stream responses as they generate.".to_owned())), + ], + ), + response_tx, + focused_option_index: 0, + selected_option_indices: std::collections::BTreeSet::from([0]), + notes: String::new(), + notes_cursor: 0, + editing_notes: false, + focused: true, + question_index: 1, + total_questions: 4, + }); + + let body = standard::render_tool_call_body(&tc, 120); + let joined = rendered_line_texts_trimmed(&body).join("\n"); + + assert!(joined.contains("? Features (2/4)")); + assert!(joined.contains("Which features should be enabled by default?")); + assert!(joined.contains("[x] Streaming")); + assert!(!joined.contains("Log level: info")); + assert!(!joined.contains("What log level should ship as the default?")); +} + +#[test] +fn ask_user_question_completed_body_falls_back_to_transcript_without_structured_answers() { + let mut tc = + test_tool_call("AskUserQuestion", "AskUserQuestion", model::ToolCallStatus::Completed); + tc.content = + vec![model::ToolCallContent::from("Target: Staging\n Pick deployment target".to_owned())]; + + let body = standard::render_tool_call_body(&tc, 120); + let joined = rendered_line_texts_trimmed(&body).join("\n"); + + assert!(joined.contains("Target: Staging")); + assert!(joined.contains("Pick deployment target")); +} + #[test] fn bash_title_does_not_wrap_for_long_title() { let tc = ToolCallInfo {