Skip to content

fix(tui): show proxy tool errors instead of dropping them #837

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.

Sign up for GitHub

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

Open
thejesh23 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(tui): show proxy tool errors instead of dropping them #837

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd1f743
fix(tui): show proxy tool errors instead of dropping them
thejesh23 Jul 21, 2026
02899c9
fix(tui): report an incomplete replay instead of an empty response
thejesh23 Jul 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions strix/interface/tui/renderers/proxy_renderer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,17 @@ def _status_style(code: int | None) -> str:
return "dim"


def _has_result(status: str) -> bool:
"""True once the tool has returned, whether it succeeded or not.

``live_view._tool_status_from_result`` maps a ``{"success": false}``
payload to ``"failed"``, and every proxy-tool error payload sets that
flag, so gating result rendering on ``"completed"`` alone hides exactly
the payloads that carry an ``error``.
"""
return status in {"completed", "failed", "error"}


@register_tool_renderer
class ListRequestsRenderer(BaseToolRenderer):
tool_name: ClassVar[str] = "list_requests"
Expand Down Expand Up @@ -68,7 +79,7 @@ def render(cls, tool_data: dict[str, Any]) -> Static: # noqa: PLR0912, PLR0915
if meta_parts:
text.append(f" ({', '.join(meta_parts)})", style="dim")

if status == "completed" and isinstance(result, dict):
if _has_result(status) and isinstance(result, dict):
if "error" in result:
text.append(f" error: {_sanitize(str(result['error']), 150)}", style="#ef4444")
else:
Expand Down Expand Up @@ -139,7 +150,7 @@ def render(cls, tool_data: dict[str, Any]) -> Static: # noqa: PLR0912, PLR0915
if search_pattern:
text.append(f" /{_truncate(search_pattern, 100)}/", style="dim italic")

if status == "completed" and isinstance(result, dict):
if _has_result(status) and isinstance(result, dict):
if "error" in result:
text.append(f" error: {_sanitize(str(result['error']), 150)}", style="#ef4444")
elif "hits" in result:
Expand Down Expand Up @@ -261,7 +272,7 @@ def render(cls, tool_data: dict[str, Any]) -> Static: # noqa: PLR0912, PLR0915
elif modifications and isinstance(modifications, str):

@greptile-apps greptile-apps Bot Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Failed Replay Looks Like Response

When a replay ends with a non-DONE status but no error string, _format_replay_tool_result returns success: false without an error key. This widened gate now sends that payload through the normal response branch, so the TUI can show elapsed time and an empty response instead of explaining that the replay failed.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: strix/interface/tui/renderers/proxy_renderer.py
Line: 272

Comment:
**Failed Replay Looks Like Response**

When a replay ends with a non-`DONE` status but no error string, `_format_replay_tool_result` returns `success: false` without an `error` key. This widened gate now sends that payload through the normal response branch, so the TUI can show elapsed time and an empty response instead of explaining that the replay failed.

How can I resolve this? If you propose a fix, please make it concise.

@thejesh23 thejesh23 Jul 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, fixed in 02899c9. This was the one tradeoff I called out in the PR description, and rendering it as a response is the wrong call.

elif not result.get("success", True):
    replay_status = _sanitize(str(result.get("status") or "unknown"), 40)
    text.append(f"\n  replay did not complete: {replay_status}", style="#ef4444")

It reuses the status field _format_replay_tool_result already puts in the payload (tools.py:419), so the operator sees replay did not complete: TIMEDOUT rather than an empty response line. Error and success paths untouched.

Tests added for the incomplete-replay case (asserts no << response line is rendered) and for a DONE replay still showing its status code.

text.append(f"\n {_truncate(modifications, 200)}", style="dim italic")

if status == "completed" and isinstance(result, dict):
if _has_result(status) and isinstance(result, dict):
if not result.get("success", True) and result.get("error"):
text.append(f"\n error: {_sanitize(str(result['error']), 150)}", style="#ef4444")
else:
Expand Down Expand Up @@ -327,7 +338,7 @@ def render(cls, tool_data: dict[str, Any]) -> Static: # noqa: PLR0912, PLR0915
if meta_parts:
text.append(f" ({', '.join(meta_parts)})", style="dim")

if status == "completed" and isinstance(result, dict):
if _has_result(status) and isinstance(result, dict):
if "error" in result:
text.append(f" error: {_sanitize(str(result['error']), 150)}", style="#ef4444")
else:
Expand Down Expand Up @@ -401,7 +412,7 @@ def render(cls, tool_data: dict[str, Any]) -> Static: # noqa: PLR0912
if entry_id:
text.append(f" #{_truncate(str(entry_id), 20)}", style="dim")

if status == "completed" and isinstance(result, dict):
if _has_result(status) and isinstance(result, dict):
if "error" in result:
text.append(f" error: {_sanitize(str(result['error']), 150)}", style="#ef4444")
elif "entry" in result:
Expand Down Expand Up @@ -492,7 +503,7 @@ def render(cls, tool_data: dict[str, Any]) -> Static: # noqa: PLR0912, PLR0915
if len(denylist) > 4:
text.append(f" +{len(denylist) - 4}", style="dim italic")

if status == "completed" and isinstance(result, dict):
if _has_result(status) and isinstance(result, dict):
if "error" in result:
text.append(f" error: {_sanitize(str(result['error']), 150)}", style="#ef4444")
elif "scopes" in result:
Expand Down
51 changes: 50 additions & 1 deletion tests/test_proxy_renderer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,18 @@

from __future__ import annotations

import pytest
from rich.text import Text

from strix.interface.tui.renderers.proxy_renderer import ViewRequestRenderer
from strix.interface.tui.live_view import _tool_status_from_result
from strix.interface.tui.renderers.proxy_renderer import (
ListRequestsRenderer,
ListSitemapRenderer,
RepeatRequestRenderer,
ScopeRulesRenderer,
ViewRequestRenderer,
ViewSitemapEntryRenderer,
)


def _plain(static: object) -> str:
Expand Down Expand Up @@ -44,3 +53,43 @@ def test_more_content_hint_shown_when_has_more_flag_set() -> None:
content = "\n".join(f"line{i}" for i in range(3))

assert _MARKER in _render(content, has_more=True)


# Every proxy-tool error payload sets `success: false`, which
# `live_view._tool_status_from_result` maps to status "failed" -- so the
# renderers must show results for that status too, not just "completed".
@pytest.mark.parametrize(
("renderer", "args"),
[
(ListRequestsRenderer, {}),
(ViewRequestRenderer, {"request_id": "req-1"}),
(RepeatRequestRenderer, {"request_id": "req-1"}),
(ListSitemapRenderer, {}),
(ViewSitemapEntryRenderer, {"entry_id": "e-1"}),
(ScopeRulesRenderer, {"action": "list"}),
],
)
def test_error_is_rendered_for_failed_status(renderer: object, args: dict[str, object]) -> None:
tool_data = {
"args": args,
"result": {"success": False, "error": "proxy tool failed: boom"},
"status": _tool_status_from_result({"success": False, "error": "x"}),
}

assert "proxy tool failed: boom" in _plain(renderer.render(tool_data)) # type: ignore[attr-defined]


def test_tool_status_from_result_maps_failure_to_failed() -> None:
# Pins the mapping the renderers' gate has to agree with.
assert _tool_status_from_result({"success": False, "error": "x"}) == "failed"
assert _tool_status_from_result({"success": True}) == "completed"


def test_successful_result_still_renders() -> None:
tool_data = {
"args": {},
"result": {"success": True, "entries": [], "page_info": {"has_next_page": False}},
"status": "completed",
}

assert "[0 found]" in _plain(ListRequestsRenderer.render(tool_data))