-
Notifications
You must be signed in to change notification settings - Fork 4.9k
fix(tui): don't double-render finish_scan section headings #798
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,3 +1,4 @@ | ||||||||||||
| import re | ||||||||||||
| from typing import Any, ClassVar | ||||||||||||
|
|
||||||||||||
| from rich.text import Text | ||||||||||||
|
|
@@ -10,6 +11,23 @@ | |||||||||||
| FIELD_STYLE = "bold #4ade80" | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| def _strip_leading_heading(value: str, section: str) -> str: | ||||||||||||
| """Drop a leading markdown heading that just repeats the section label. | ||||||||||||
|
|
||||||||||||
| The finish_scan tool prompts the model to write markdown in every field, and | ||||||||||||
| the models routinely open each with a ``# <Section>`` heading (e.g. | ||||||||||||
| ``# Executive Summary``). This renderer also prints its own styled section | ||||||||||||
| label above the value, so that heading renders twice. Strip a leading | ||||||||||||
| ``#``-heading whose text matches this section (case-insensitively) so the | ||||||||||||
| label isn't duplicated; leave all other content — including headings that | ||||||||||||
| say something else — untouched. | ||||||||||||
| """ | ||||||||||||
| stripped = value.lstrip() | ||||||||||||
| pattern = rf"^#{{1,6}}\s+{re.escape(section)}\s*\n+" | ||||||||||||
| m = re.match(pattern, stripped, flags=re.IGNORECASE) | ||||||||||||
| return stripped[m.end() :] if m else value | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a field contains only
Suggested change
Prompt To Fix With AIThis is a comment left during a code review.
Path: strix/interface/tui/renderers/finish_renderer.py
Line: 27-28
Comment:
**Trailing Newline Erases Lone Heading**
When a field contains only `# Recommendations\n`, it passes the upstream non-empty check and matches this pattern, but the helper returns an empty string. The TUI then hides the field's only supplied content, contrary to the stated rule that a lone heading should be preserved.
```suggestion
m = re.match(pattern, stripped, flags=re.IGNORECASE)
remainder = stripped[m.end() :] if m else ""
return remainder if remainder.strip() else value
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||||||||||||
|
|
||||||||||||
|
|
||||||||||||
| @register_tool_renderer | ||||||||||||
| class FinishScanRenderer(BaseToolRenderer): | ||||||||||||
| tool_name: ClassVar[str] = "finish_scan" | ||||||||||||
|
|
@@ -32,25 +50,25 @@ def render(cls, tool_data: dict[str, Any]) -> Static: | |||||||||||
| text.append("\n\n") | ||||||||||||
| text.append("Executive Summary", style=FIELD_STYLE) | ||||||||||||
| text.append("\n") | ||||||||||||
| text.append(executive_summary) | ||||||||||||
| text.append(_strip_leading_heading(executive_summary, "Executive Summary")) | ||||||||||||
|
|
||||||||||||
| if methodology: | ||||||||||||
| text.append("\n\n") | ||||||||||||
| text.append("Methodology", style=FIELD_STYLE) | ||||||||||||
| text.append("\n") | ||||||||||||
| text.append(methodology) | ||||||||||||
| text.append(_strip_leading_heading(methodology, "Methodology")) | ||||||||||||
|
|
||||||||||||
| if technical_analysis: | ||||||||||||
| text.append("\n\n") | ||||||||||||
| text.append("Technical Analysis", style=FIELD_STYLE) | ||||||||||||
| text.append("\n") | ||||||||||||
| text.append(technical_analysis) | ||||||||||||
| text.append(_strip_leading_heading(technical_analysis, "Technical Analysis")) | ||||||||||||
|
|
||||||||||||
| if recommendations: | ||||||||||||
| text.append("\n\n") | ||||||||||||
| text.append("Recommendations", style=FIELD_STYLE) | ||||||||||||
| text.append("\n") | ||||||||||||
| text.append(recommendations) | ||||||||||||
| text.append(_strip_leading_heading(recommendations, "Recommendations")) | ||||||||||||
|
|
||||||||||||
| if not (executive_summary or methodology or technical_analysis or recommendations): | ||||||||||||
| text.append("\n ") | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| """Tests for the finish_scan TUI renderer (section-heading de-duplication).""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| from rich.text import Text | ||
|
|
||
| from strix.interface.tui.renderers.finish_renderer import ( | ||
| FinishScanRenderer, | ||
| _strip_leading_heading, | ||
| ) | ||
|
|
||
|
|
||
| def _plain(static: object) -> str: | ||
| content = static.content # type: ignore[attr-defined] | ||
| return content.plain if isinstance(content, Text) else str(content) | ||
|
|
||
|
|
||
| def _render(**args: str) -> str: | ||
| return _plain(FinishScanRenderer.render({"status": "completed", "args": args})) | ||
|
|
||
|
|
||
| # --- _strip_leading_heading ------------------------------------------------- | ||
|
|
||
|
|
||
| def test_strips_matching_leading_heading() -> None: | ||
| assert _strip_leading_heading("# Executive Summary\n\nBody", "Executive Summary") == "Body" | ||
| assert _strip_leading_heading("## Methodology\nSteps", "Methodology") == "Steps" | ||
|
|
||
|
|
||
| def test_strip_is_case_and_whitespace_insensitive() -> None: | ||
| assert _strip_leading_heading("# technical analysis \n\nX", "Technical Analysis") == "X" | ||
|
|
||
|
|
||
| def test_keeps_a_different_leading_heading() -> None: | ||
| # A heading that isn't the section label is real content — leave it. | ||
| val = "# Findings Overview\nY" | ||
| assert _strip_leading_heading(val, "Executive Summary") == val | ||
|
|
||
|
|
||
| def test_keeps_body_when_no_heading() -> None: | ||
| assert _strip_leading_heading("No heading here", "Methodology") == "No heading here" | ||
|
|
||
|
|
||
| def test_keeps_lone_heading_with_no_body() -> None: | ||
| # No trailing newline => not a section split; don't strip to empty. | ||
| assert _strip_leading_heading("# Recommendations", "Recommendations") == "# Recommendations" | ||
|
|
||
|
|
||
| # --- end-to-end render ------------------------------------------------------ | ||
|
|
||
|
|
||
| def test_section_heading_rendered_once_not_twice() -> None: | ||
| # The model is prompted to write markdown and routinely opens each field | ||
| # with a `# <Section>` heading; the renderer also prints a styled label. | ||
| # Regression: both showed, doubling the heading. Now the field's leading | ||
| # heading is stripped so "Executive Summary" appears exactly once. | ||
| out = _render(executive_summary="# Executive Summary\n\nThe app is sound.") | ||
| assert out.count("Executive Summary") == 1 | ||
| assert "The app is sound." in out | ||
|
|
||
|
|
||
| def test_render_preserves_body_without_heading() -> None: | ||
| out = _render(methodology="White-box review of the diff.") | ||
| assert "Methodology" in out | ||
| assert "White-box review of the diff." in out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A valid Markdown heading such as
# Executive Summary #\n\nBodydoes not match because of its closing#. The original heading remains below the styled section label, so the duplicate heading this change targets is still displayed for that common Markdown form.Prompt To Fix With AI