Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for streaming markdown rendering in the goose CLI. The implementation introduces a new MarkdownBuffer that intelligently buffers incoming markdown chunks and only flushes content when markdown constructs are complete, preventing broken rendering of partial bold text, code blocks, links, etc. during streaming.
Changes:
- Added a new
streaming_buffer.rsmodule with a markdown state machine parser that tracks open constructs - Modified the message rendering pipeline to use buffered streaming instead of immediate rendering
- Integrated buffer flushing at appropriate points in the streaming loop
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| crates/goose-cli/src/session/streaming_buffer.rs | New module implementing MarkdownBuffer with regex-based tokenization and state tracking for safe incremental markdown rendering, includes comprehensive test coverage |
| crates/goose-cli/src/session/output.rs | Added render_message_streaming function that uses MarkdownBuffer to accumulate text and flush helper functions |
| crates/goose-cli/src/session/mod.rs | Integrated MarkdownBuffer into the streaming loop, replacing immediate rendering with buffered rendering and ensuring flush on completion |
| pub fn render_message_streaming( | ||
| message: &Message, | ||
| buffer: &mut MarkdownBuffer, | ||
| debug: bool, | ||
| ) -> bool { |
There was a problem hiding this comment.
The function returns a bool indicating whether text content was present, but this return value is never used at the call site in mod.rs line 1038. Either use the return value for some purpose (e.g., tracking/logging) or change the function signature to return () to match render_message.
michaelneale
left a comment
michaelneale
left a comment
There was a problem hiding this comment.
why not - will make cli a little more modern. Not sure about failure modes if it misses a bit I guess it will finish it eventually in a chunk?
|
yeah, if you open a code block with ``` and it never closes, it will stop streaming and flush it at the end. but right now we often just break syntax highlighting for code etc. but yeah, we should think about the strategy when it comes to clients. I'm kinda thinking we keep the CLI, maybe slimming it down and then have N other clients that live outside our main repo? goose mobile, a TUI, etc |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
| let after = if after_lines.is_empty() { | ||
| "" | ||
| } else { | ||
| let table_end_line = lines[end]; | ||
| let content_ptr = content.as_ptr() as usize; | ||
| let line_ptr = table_end_line.as_ptr() as usize; | ||
| let table_end_pos = line_ptr.saturating_sub(content_ptr) + table_end_line.len(); | ||
| content | ||
| .get(table_end_pos..) | ||
| .and_then(|s| s.strip_prefix('\n')) | ||
| .unwrap_or("") |
There was a problem hiding this comment.
The after slice is reconstructed via pointer arithmetic on content.lines() output, which is brittle with CRLF input (since lines() strips \r) and makes it easy to miscompute the split point; consider computing the byte index using a direct search on content (e.g., tracking newline byte offsets) instead of relying on as_ptr math.
| let after = if after_lines.is_empty() { | |
| "" | |
| } else { | |
| let table_end_line = lines[end]; | |
| let content_ptr = content.as_ptr() as usize; | |
| let line_ptr = table_end_line.as_ptr() as usize; | |
| let table_end_pos = line_ptr.saturating_sub(content_ptr) + table_end_line.len(); | |
| content | |
| .get(table_end_pos..) | |
| .and_then(|s| s.strip_prefix('\n')) | |
| .unwrap_or("") | |
| // Precompute positions of newline bytes in the original content so we can | |
| // reliably slice the "after" portion without pointer arithmetic. | |
| let newline_indices: Vec<usize> = content | |
| .bytes() | |
| .enumerate() | |
| .filter_map(|(i, b)| if b == b'\n' { Some(i) } else { None }) | |
| .collect(); | |
| let after = if after_lines.is_empty() { | |
| "" | |
| } else if let Some(&newline_pos) = newline_indices.get(end) { | |
| // Slice after the newline that terminates the last table line. | |
| &content[newline_pos + 1..] | |
| } else { | |
| "" |
| pub fn push(&mut self, chunk: &str) -> Option<String> { | ||
| self.buffer.push_str(chunk); | ||
| let safe_end = self.find_safe_end(); | ||
|
|
There was a problem hiding this comment.
push() calls find_safe_end() which reparses the entire accumulated buffer from the beginning on every chunk; for long unflushed constructs (especially large fenced code blocks) this can devolve to O(n²) work over a stream, so consider keeping parsing state and the last scanned byte offset in MarkdownBuffer and only scanning newly appended text.
| fn extract_markdown_table(content: &str) -> Option<(String, Vec<&str>, &str)> { | ||
| let lines: Vec<&str> = content.lines().collect(); | ||
| let mut table_start = None; | ||
| let mut table_end = None; | ||
|
|
||
| for (i, line) in lines.iter().enumerate() { | ||
| let trimmed = line.trim(); | ||
| if trimmed.starts_with('|') && trimmed.ends_with('|') { | ||
| if table_start.is_none() { | ||
| table_start = Some(i); | ||
| } | ||
| table_end = Some(i); | ||
| } else if table_start.is_some() { | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| let start = table_start?; | ||
| let end = table_end?; | ||
| if end < start + 1 { | ||
| return None; | ||
| } | ||
|
|
||
| let has_separator = lines[start..=end] | ||
| .iter() | ||
| .any(|line| line.contains("---") || line.contains(":-") || line.contains("-:")); | ||
| if !has_separator { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
extract_markdown_table can mis-detect and re-render table-like text inside fenced code blocks (e.g., a code sample containing |---|---|), which changes the meaning of the markdown being rendered; consider skipping table extraction when the candidate table region is within a ```/~~~ fenced block (or otherwise validating it’s not in a code span/block).
| let has_separator = lines[start..=end] | ||
| .iter() | ||
| .any(|line| line.contains("---") || line.contains(":-") || line.contains("-:")); | ||
| if !has_separator { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
has_separator is currently a loose substring check ("---",":-","-:") across any line, which can produce false positives and render non-tables as tables; consider requiring the separator row to be the second row and matching the actual ---/:-:/-: cell pattern (similar to the is_separator logic in print_table).
…nto streaming-markdown
* main: fix: handle reasoning_content for Kimi/thinking models (#7252) feat: sandboxing for macos (#7197) fix(otel): use monotonic_counter prefix and support temporality env var (#7234) Streaming markdown (#7233) Improve compaction messages to enable better post-compaction agent behavior (#7259) fix: avoid shell-escaping special characters except quotes (#7242)
* origin/main: docs: playwright CLI skill tutorial (#7261) install node in goose dir (#7220) fix: relax test_basic_response assertion for providers returning reasoning_content (#7249) fix: handle reasoning_content for Kimi/thinking models (#7252) feat: sandboxing for macos (#7197) fix(otel): use monotonic_counter prefix and support temporality env var (#7234) Streaming markdown (#7233) Improve compaction messages to enable better post-compaction agent behavior (#7259) fix: avoid shell-escaping special characters except quotes (#7242) fix: use dynamic port for Tetrate auth callback server (#7228) docs: removing LLM Usage admonitions (#7227) feat(otel): respect standard OTel env vars for exporter selection (#7144) fix: fork session (#7219) Bump version numbers for 1.24.0 release (#7214) Move platform extensions into their own folder (#7210) fix: ignore deprecated skills extension (#7139) # Conflicts: # Cargo.lock # Cargo.toml
…led-extensions-cmd * 'main' of github.com:block/goose: (24 commits) Set up direnv and update flake inputs (#6526) fix: restore subagent tool call notifications after summon refactor (#7243) fix(ui): preserve server config values on partial provider config save (#7248) fix(claude-code): allow goose to run inside a Claude Code session (#7232) fix(openai): route gpt-5 codex via responses and map base paths (#7254) feat: add GoosePlatform to AgentConfig and MCP initialization (#6931) Fix copied over (#7270) feat(gemini-cli): add streaming support via stream-json events (#7244) fix: filter models without tool support from recommended list (#7198) fix(google): handle more thoughtSignature vagaries during streaming (#7204) docs: playwright CLI skill tutorial (#7261) install node in goose dir (#7220) fix: relax test_basic_response assertion for providers returning reasoning_content (#7249) fix: handle reasoning_content for Kimi/thinking models (#7252) feat: sandboxing for macos (#7197) fix(otel): use monotonic_counter prefix and support temporality env var (#7234) Streaming markdown (#7233) Improve compaction messages to enable better post-compaction agent behavior (#7259) fix: avoid shell-escaping special characters except quotes (#7242) fix: use dynamic port for Tetrate auth callback server (#7228) ...
2 participants
Summary
Supports streaming markdown, somewhat