Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
32 changes: 26 additions & 6 deletions strix/interface/tui/renderers/proxy_renderer.py
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,9 +272,18 @@ def render(cls, tool_data: dict[str, Any]) -> Static: # noqa: PLR0912, PLR0915
elif modifications and isinstance(modifications, str):

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.

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")
elif not result.get("success", True):
# A replay that never reached DONE carries no `error`; report the
# replay status rather than rendering an empty response as if the
# request had come back.
replay_status = _sanitize(str(result.get("status") or "unknown"), 40)
text.append(
f"\n replay did not complete: {replay_status}",
style="#ef4444",
)
else:
elapsed_ms = result.get("elapsed_ms")
response = result.get("response") or {}
Expand Down Expand Up @@ -327,7 +347,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 +421,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 +512,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
88 changes: 87 additions & 1 deletion tests/test_proxy_renderer.py
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,80 @@ 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_incomplete_replay_says_so_instead_of_showing_an_empty_response() -> None:
# _format_replay_tool_result sets success=False with no `error` key when the
# replay never reached DONE; it must not render as a returned response.
tool_data = {
"args": {"request_id": "req-1"},
"result": {
"success": False,
"status": "TIMEDOUT",
"session_id": "s-1",
"elapsed_ms": 30000,
"response": None,
},
"status": "failed",
}
text = _plain(RepeatRequestRenderer.render(tool_data))

assert "replay did not complete: TIMEDOUT" in text
assert "<<" not in text, "rendered a response line for a replay that never returned"


def test_successful_replay_still_renders_its_response() -> None:
tool_data = {
"args": {"request_id": "req-1"},
"result": {
"success": True,
"status": "DONE",
"elapsed_ms": 12,
"response": {"status_code": 200, "body": "ok"},
},
"status": "completed",
}
text = _plain(RepeatRequestRenderer.render(tool_data))

assert "200" in text
assert "replay did not complete" not in text


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))