feat(rich_output): stateful block markdown rendering (PR4) - #4513
feat(rich_output): stateful block markdown rendering (PR4)#4513KUSH42 wants to merge 54 commits into
Conversation
dd47908 to
dec9eb7
Compare
6d56392 to
c67eb9e
Compare
Introduces agent/rich_output.py — a self-contained Rich/Pygments rendering toolkit with no project-specific imports. Public API: - LanguageDetector: extension map + content-pattern heuristics - SyntaxHighlighter: Pygments → Rich markup → ANSI string - FilePathFormatter: per-filetype icons, compact relative paths - DiffRenderer: unified diff → Rich Text with line numbers - clean_command_output: strip venv/stacktrace noise from command output DiffRenderer replaces _render_inline_unified_diff in display.py: - Intra-line character-level highlighting via SequenceMatcher (threshold 0.5) - Per-run del/add pairing to avoid cross-hunk false matches - Summary header: ● filename.py Added N lines, removed M lines - Console width from shutil.get_terminal_size, not hardcoded Tests: 51 passing in tests/test_rich_output.py
Pygments emits Error tokens for content its markdown lexer cannot tokenize (emoji in headings, unknown syntax, etc.). Mapping Error to "bold red on red" matched the diff-deletion colour, causing spurious red backgrounds on unrelated text. Changed to "bold red" (text colour only), consistent with Generic.Error.
Adds _DIFF_MAX_LINES = 80 and a max_lines parameter to to_lines(). Outputs beyond the cap get a dim footer matching _highlight_block's style. Passes max_lines=0 in _render_inline_unified_diff so the section-level budget in _summarize_rendered_diff_sections is unaffected.
Apply Pygments token colours to diff line content so that keywords, strings, comments, numbers etc. are visually distinct within the red/green diff bands — matching the style shown in tools like delta. Changes: * Two new colour constants — _DIFF_BG_ADD_HL / _DIFF_BG_DEL_HL — a noticeably brighter shade of the base diff background, used to mark changed character ranges in intra-line diffs without touching the foreground (no conflict with syntax token colours). * _syntax_text(content, filename) — new helper that calls SyntaxHighlighter.to_markup(), converts to a Rich Text (foreground colours only), and strips the trailing newline Pygments always appends so line lengths stay accurate. * _flat_add / _flat_del — now accept an optional filename hint and apply syntax highlighting via _syntax_text, then overlay the base diff background with Text.stylize(). * _intra_diff — redesigned: syntax-highlight both lines first, apply the base diff background across the whole text, then apply the brighter highlight background (bold) only over changed character ranges via a second stylize() pass. Foreground colours come entirely from syntax; background colours entirely from diff state. * _style() — passes filename (explicit_filename or from_path) through to all three helpers above, and syntax-highlights context lines (dim style overlaid on syntax colours). * Tests updated to match the new interface: _intra_diff now returns ([Text], [Text]) with spans rather than lists of single-span segments; changed-region detection checks for bright bg spans instead of bright foreground colour names.
Line numbers and the -/+ prefix had no background set, so they rendered on the terminal default (black) while the content immediately to their right had the dark-red/dark-green diff background — creating a jarring visual break. Extend bgcolor to the line-number and sigil Text objects so the entire deleted/added row is uniformly on the diff background, matching the visual style of delta, GitHub, and VS Code's diff view.
97bfa4f to
d53b3e8
Compare
|
Minor test failure noticed while building on top of this PR:
diff = "--- a/foo.py\n+++ b/foo.py\n@@ -1 +1 @@\n-old\n+new\n"
lines = dr.to_lines(diff)
# Expected: non-empty list
# Actual: []All other 318 tests in |
kshitijk4poor
left a comment
There was a problem hiding this comment.
I tested this branch with python -m hermes_cli.main -w chat -q ... from the PR worktree and the stateful markdown direction is promising — setext-ish heading output, nested blockquotes, lists, and table rendering are all much closer to the intended UX in a real terminal than they were before.
That said, I don't think this is ready to merge yet. There are still blocking correctness issues in the incremental diff here:
- the
DiffRenderer.to_lines()regression from the stacked base review is still present on this branch; the default path still returns no output in the existing diff-renderer tests - the streaming markdown path is now gated on
_code_highlight_active, which makesdisplay.code_highlightcontrol whether plain-text markdown rendering happens at all during streaming - the
/code-highlightcommand surface is still being expanded even though the feature is already being treated as config-backed elsewhere
For reference, I ran:
python -m pytest tests/test_rich_output.py -qon this branch → 8 failurespython -m hermes_cli.main -w chat -q "..."on this branch to inspect the rendering directly
So I think this still needs another pass before merge.
| ``_summarize_rendered_diff_sections``). | ||
| """ | ||
| buf = StringIO() | ||
| Console(file=buf, highlight=False, force_terminal=True, width=width).print( |
There was a problem hiding this comment.
This regression is still here.
DiffRenderer.to_lines() is still constructing Console(..., width=width) with the default width=0. On this branch the existing DiffRenderer / DiffRendererTruncation tests still fail locally because the default call path can produce no rendered output at all. The stacked base review already called this out, so I don't think we should merge more rendering work on top until this is fixed.
There was a problem hiding this comment.
Fixed on the rebased branch head. The default DiffRenderer().to_lines(...) path is green again in the branch-local touched tests.
| if out2 is out: | ||
| # plain text — apply block + inline markdown (fires whenever | ||
| # _code_highlight_active is True, consistent with PR3) | ||
| if _display._code_highlight_active: |
There was a problem hiding this comment.
This makes streaming markdown rendering depend on _code_highlight_active.
At this point the branch is no longer just toggling code highlighting — it's toggling whether plain-text markdown features like headings / blockquotes / lists get rendered at all in the streaming path. That makes display.code_highlight semantically much broader than its name suggests, and it also reintroduces coupling that earlier review feedback was trying to unwind.
There was a problem hiding this comment.
Fixed on this branch. Plain streamed prose now always goes through the markdown rendering path; display.code_highlight no longer controls whether non-code markdown features render during streaming.
| self.console.print(f" Status bar {state}") | ||
| elif canonical == "verbose": | ||
| self._toggle_verbose() | ||
| elif canonical in ("code-highlight", "codehighlight", "code_highlight"): |
There was a problem hiding this comment.
This is still pushing code-highlight further as a slash-command surface by keeping the command and adding extra aliases.
Given the surrounding implementation and config naming, this feature still reads as config-backed rather than command-backed. Expanding the command handling here makes that product surface harder to unwind later, not easier.
There was a problem hiding this comment.
Addressed on this branch. I removed the remaining direct CLI dispatch path here as part of the same config-surface cleanup.
kshitijk4poor
left a comment
There was a problem hiding this comment.
A few more correctness notes after checking the stateful markdown path more closely.
| # _pending is intentionally left untouched (spec). | ||
| # If inside a blockquote, keep the gutter so the code block is visually | ||
| # contained within the quote; _bq_depth stays and exits on next blank line. | ||
| if "\x1b" in line: |
There was a problem hiding this comment.
Blockquoted prose can still be emitted after the following highlighted code block.
In batch mode, ANSI lines are emitted immediately here when _bq_depth is set, but the buffered quoted line is left in _pending. For input like `> quote
x=1
````, the code line is emitted first and the quote line second. I reproduced the plain output on this branch as:
`▌ 1 │ x=1
▌ quote`
So the quoted prose/code ordering is reversed.There was a problem hiding this comment.
Fixed on this branch. Pending blockquote prose is now flushed before an ANSI-highlighted code line is emitted, so quote/code order matches source order. Added regression coverage.
|
|
||
| # Loose table separator (no leading pipe, e.g. "---|---|---" or "--- --- ---"). | ||
| # Current line must look like a separator; pending line must be a loose header. | ||
| if _pending is not None and "|" in _pending and "-" in line and _TABLE_SEP_RE.match(line.strip()): |
There was a problem hiding this comment.
Loose-table detection still looks too permissive here.
This branch only checks that the delimiter row looks separator-like; it never validates that the separator has the same column structure as the pending header. That means ordinary pipe prose plus --- can still get promoted into a table instead of staying prose followed by a rule / setext marker.
I think this needs a column-count / shape check before treating the pair as a loose table.
There was a problem hiding this comment.
Fixed on this branch. Loose-table promotion now requires the separator row to match the pending header’s column shape instead of accepting any separator-looking line. Added regression coverage.
| # line has at least one ANSI escape — safe for pass-2 \x1b detection. | ||
| return _number_code_lines(highlighted) | ||
|
|
||
| # Pre-pass: collect reference link definitions for inline resolution |
There was a problem hiding this comment.
Reference definitions inside fenced code blocks leak into later prose link resolution.
ref_map is collected from the raw response before fenced-code replacement, so a code sample containing [ref]: https://example.com will unexpectedly make later prose Use [x][ref]. render as a real link. I reproduced that locally on this branch.
The streaming path has the same issue because StreamingBlockBuffer records _REF_DEF_RE matches before the code-block highlighter suppresses fence contents.
There was a problem hiding this comment.
Fixed on this branch. Reference definitions are now collected with fenced-code regions excluded in both batch and streaming paths, so defs inside code blocks no longer resolve later prose links. Added regression coverage.
aa51ce7 to
fc8b015
Compare
|
@lucaspirola Fixed on the current branch head. The |
_MAX_INLINE_DIFF_LINES (80), _MAX_INLINE_DIFF_FILES (6), and _PREVIEW_MAX_LINES (40) were hardcoded in agent/display.py with no user-facing knob. Wire them to config.yaml under display: diff_max_lines: 80 # lines shown per inline diff before "… omitted" summary diff_max_files: 6 # file sections shown per inline diff preview_max_lines: 40 # lines shown in read_file/execute_code/terminal previews Adds set_diff_limits() and set_preview_max_lines() setters in display.py; cli.py reads and applies all three at init alongside code_highlight.
…ext-in-blockquote, ref link resolution Five missing markdown features added to the terminal renderer: * Ordered lists — digits + '.' or ')' delimiter, dim numeral styling, continuation line handling in stateful paths, indent depth by 2-space scale matching UL * Task lists — '[ ]' renders as dim ○, '[x]'/'[X]' renders as bold green ✓; detected inside UL match branch, no new state required * Nested blockquotes — _bq_depth: int replaces _in_blockquote: bool throughout render_stateful_blocks and StreamingBlockBuffer; each depth level adds 2-space indent and one extra dim layer (capped at 3) * Setext headings inside blockquotes — stateful scan detects '==='/'---' inner content following a blockquote pending line, styles as h1/h2, re-wraps in gutter * Link reference definitions — pre-pass collects [label]: url defs into ref_map; apply_inline_markdown resolves [text][ref] and [text][] before the inline link step; StreamingBlockBuffer accumulates ref_map as defs arrive; batch path (format_response) does a full pre-pass 253 tests passing.
…kdown features Bug fix: _REF_DEF_RE only matched double-quoted titles; parenthesized (Title) and single-quoted 'Title' variants were silently dropped from ref_map, leaving [text][ref] unresolved. Extended the optional title group to cover all three CommonMark title forms. 49 new edge-case tests across ordered lists, task lists, nested blockquotes, setext-in-blockquote, and ref link resolution — covering delimiter variants, blank-line interaction, mixed list types, streaming path correctness, and depth/reset behaviour. 302 tests passing.
Two table rendering bugs fixed: 1. Strict tables (pipes at both ends of every row) now render with a full box frame using box-drawing characters (┌┬┐│├┼┤└┴┘─). A row separator is drawn between every pair of content rows. 2. Loose table separators without pipes (e.g. "--- --- ---") were silently dropped because detection required "|" in the separator line. Now uses _TABLE_SEP_RE (^[\s:\-|]+$) + "-" presence check, so pipe-free separators are handled correctly. Implementation: - Rename _TABLE_ROW_RE → _TABLE_STRICT_ROW_RE; add _TABLE_LOOSE_ROW_RE and _TABLE_SEP_RE regexes per spec - _render_table: add framed: bool param; framed path draws box chars with ANSI-aware column widths and inter-row dividers - Track _table_strict in render_stateful_blocks and StreamingBlockBuffer; set from first accumulated row (header); pass framed=_table_strict to _render_table on flush - Update loose separator detection in both state machines to use _TABLE_SEP_RE instead of "|" in line guard - Update tests: rename import, fix test_table_no_separator expectation (strict tables always have box chars now)
…tore outer style after inline spans List items (UL, OL, task lists) call apply_inline_markdown internally without reset_suffix. The returned string already contains \x1b, so the outer apply_inline_markdown call (which carries reset_suffix=_DIM) hits the early-exit and never restores the outer style. Fix: add reset_suffix parameter to apply_block_line and forward it to every inner apply_inline_markdown call — including the checkbox symbols in task lists and the dim numeral in ordered lists. Update all seven call sites in cli.py to pass reset_suffix to apply_block_line. Reproducer: any UL/OL/task-list line with a backtick span inside a reasoning/think block loses dim formatting for text after the code span.
… del block Commit 58ba8fe fixed non-monotonic del/add numbers on offset hunks (@@ -59 +58 @@) by storing ln_new+offset for deletions instead of ln_old. This broke when a context line appears between two deletion runs: both the preceding deletion and the context line end up with the same line number, and subsequent deletions pick up a shifted new-file scale that no longer matches their old-file positions. Fix: store ln_old in del_run at append time. At flush time, paired deletions (those with a matching addition) take the addition's new-file line number so paired del/add lines always show the same number. Unpaired deletions fall back to their saved ln_old, keeping numbers correct and monotonic even when context lines interrupt a deletion block.
…n cli.py _apply_inline_md and _apply_block_line were called throughout the streaming render path (_emit_stream_text, _flush_stream, reasoning box) but never imported. Every call raised NameError, silently swallowed by the try/except in _fire_stream_delta — so all streamed text was silently dropped. Add the two missing imports (apply_inline_markdown, apply_block_line) from agent.rich_output, and no-op fallbacks in the ImportError branch.
format_response() called apply_block_line / apply_inline_markdown without reset_suffix, so after any inline element (bold, italic, code span) the ANSI reset dropped to terminal default instead of the skin's banner_text colour — unlike the streaming path which passes reset_suffix=_tc throughout. - Add reset_suffix param to format_response(); thread into Pass 3 calls - cli.py Panel path: compute _text_reset from _resp_text hex (same logic as streaming's _stream_text_ansi) and pass as reset_suffix to _format_response - Also commit diff/preview line-limit config keys (diff_max_lines, diff_max_files, preview_max_lines) from prior working changes
TestFormatResponseResetSuffix: verifies that reset_suffix is threaded into inline-element ANSI resets (bold, italic, code spans) so the Panel path restores the caller's text colour instead of dropping to terminal default after each span — matching the streaming path's behaviour. Five cases: default empty string, suffix after bold, suffix after code, no suffix leak into fenced blocks, explicit empty == default.
_MAX_INLINE_DIFF_LINES (80), _MAX_INLINE_DIFF_FILES (6), and _PREVIEW_MAX_LINES (40) were hardcoded in agent/display.py with no user-facing knob. Wire them to config.yaml under display: diff_max_lines: 80 # lines shown per inline diff before "… omitted" summary diff_max_files: 6 # file sections shown per inline diff preview_max_lines: 40 # lines shown in read_file/execute_code/terminal previews Adds set_diff_limits() and set_preview_max_lines() setters in display.py; cli.py reads and applies all three at init alongside code_highlight.
TestSpinnerConfig: verifies _SPINNER_STYLES registry completeness, dot style frames, none-style empty frame, unknown-style fallback logic, per- style frame validity, and title_spinner/title_base instance attributes.
TestMonokaiIntraDiff and the monokai_skin fixture depend on SyntaxHighlighter.refresh() and the charizard skin's syntax_scheme, both of which are implemented in PR5 (theme integration). Having them here causes fixture-setup errors on PR4's branch where refresh() does not exist. Removing from this branch; PR5 re-adds them alongside the implementation.
adadefe to
8249345
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the substantial markdown-rendering work. I found one blocking streaming correctness issue in the current PR head.
Problems
cli.py:2059flushesStreamingBlockBufferonly insideif self._stream_buf:. A response ending in a newline leaves_stream_bufempty, whileStreamingBlockBuffer.process_line()may still retain the final line for lookahead. That retained output is never emitted.- This remains stacked on unmerged #4504, while current main's primary TUI renderer is now
ui-tui/src/components/markdown.tsx; the integration needs deliberate salvage rather than a direct merge.
Suggested changes
- Flush the stateful and code-block buffers unconditionally after any partial-line handling, and add a newline-terminated streaming regression test.
Automated hermes-sweeper review.
| for hl_line in out2.splitlines(): | ||
| _cprint(hl_line) | ||
| # Flush any buffered block-level state | ||
| buf_tail = self._stream_block_buf.flush() |
There was a problem hiding this comment.
flush() must not depend on _stream_buf being non-empty. A final newline empties _stream_buf, but StreamingBlockBuffer can still retain the last line in _pending for lookahead, so newline-terminated streamed output is lost. Move both stateful/code-buffer flushes outside this branch and add a regression test.
Adds a stateful second pass to
format_responsefor block elements requiring cross-line context: setext headings, multi-line blockquotes, GFM tables. Also completes the inline/block markdown surface with ordered lists, task lists, nested blockquotes, setext-in-blockquote, and ref link resolution.Stateful block rendering
render_stateful_blocks— pass 2 informat_response; single left-to-right scan after code-block highlightingStreamingBlockBuffer— state machine inserted beforeStreamingCodeBlockHighlighterin the streaming pipelineFeatures
Setext headings —
===/---marker consumed; preceding line rendered as h1/h2;---after blank line passes through as hrBlockquote lazy continuation — non-empty lines after
> textkeep the▌gutter until a blank line; ANSI lines keep the gutter; ```` fence exits blockquote mode for the code highlighterNested blockquotes — depth tracked as int; each level adds 2-space indent + one extra dim layer (capped at 3); depth resets on blank line
Pipe tables — strict and loose (GFM optional boundary pipes); column widths from data rows;
─separator after header;:---/---:/:---:alignment; numeric cells auto-right-aligned; ragged rows padded; emoji/wide-char-aware widthsOrdered lists —
1.and1)delimiter forms; dim numeral styling; continuation lines; nested indentTask lists —
[ ]→ dim○,[x]/[X]→ bold green✓; works inside nested UL; content passes through inline markdownSetext headings inside blockquotes —
> Heading\n> ===correctly detected and styled as h1/h2Link reference definitions — all three CommonMark title forms (double-quoted, single-quoted, parenthesized);
[text][ref]and[text][]resolved before inline link step; streaming accumulates defs as they arriveBare URL styling —
https?://,ftp(s)://,file://,www.auto-styled with link colour + underlineANSI corruption fix —
_MD_LINK_REuses(?<!\x1b)lookbehind preventing image reset codes from being matched as link bracketsCode block line numbers — dim right-justified
1 │,2 │, … on every highlighted fenced block in batch and streaming pathsNot included
Setext inside lists, table captions/multi-line cells, footnotes, definition lists, block-level HTML.
Tests
302 tests passing.