fix(tui): show proxy tool errors instead of dropping them#837
fix(tui): show proxy tool errors instead of dropping them#837thejesh23 wants to merge 2 commits into
Conversation
All six proxy renderers gated result rendering on `status == "completed"`,
but `live_view._tool_status_from_result` maps any `{"success": false}`
payload to `"failed"` -- and every proxy-tool error payload sets that flag.
So the `if "error" in result` branch in each renderer was unreachable, and
a failed call rendered as a bare label indistinguishable from a call that
returned no rows.
Affected: Caido unreachable, malformed HTTPQL, an invalid regex in
view_request, a missing request id, and all four scope_rules argument
errors.
Add `_has_result(status)` (completed/failed/error) and use it for all six
gates. Each block already branches on the error key first, so success
rendering is unchanged.
One payload -- a repeat_request replay that did not reach DONE
(tools.py:420) -- sets success=false with no error key; it now renders its
replay response rather than nothing at all.
Adds a parametrised regression test over all six renderers plus a test
pinning the live_view status mapping the gate has to agree with; they fail
on main.
Greptile SummaryThis PR makes proxy tool failures visible in the TUI. The main changes are:
Confidence Score: 5/5This looks safe to merge after a small cleanup to failed replay output.
strix/interface/tui/renderers/proxy_renderer.py Important Files Changed
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
strix/interface/tui/renderers/proxy_renderer.py:272
**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.
Reviews (1): Last reviewed commit: "fix(tui): show proxy tool errors instead..." | Re-trigger Greptile |
| @@ -261,7 +272,7 @@ def render(cls, tool_data: dict[str, Any]) -> Static: # noqa: PLR0912, PLR0915 | |||
| elif modifications and isinstance(modifications, str): | |||
There was a problem hiding this 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.
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.There was a problem hiding this comment.
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.
_format_replay_tool_result sets success=False with no `error` key when a replay never reaches DONE, so the widened gate sent it down the normal response branch and rendered elapsed time with an empty response, as if the request had come back. Report the replay status instead (TIMEDOUT, ERROR, ...). Both the error and success paths are unchanged. Adds tests for the incomplete-replay and successful-replay cases.
|
Agreed — that's the tradeoff I flagged in the PR description, and you're right that rendering it as a response is the wrong call. Fixed in 02899c9. A replay that never reaches DONE now reports its replay status instead of an empty response line: It uses the Two tests added: one asserting the incomplete replay says so and renders no (Edited: this comment originally cited a wrong commit hash. The correct one is 02899c9.) |
Fixes #836
What
Widens the result-rendering gate in all six proxy renderers so a failed call shows its error, instead of rendering nothing.
Why
The gate was:
but
live_view._tool_status_from_resultmaps{"success": false}to"failed", and every proxy-tool error payload sets that flag —_no_client(tools.py:85),_err(:94), "Request not found" (:275,:408), "No raw {part}" (:288), "Invalid regex" (:316), and the fourscope_rulesargument errors (:572,:585,:604,:623).So the payloads that carry an
errorwere exactly the ones the gate excluded, making the error branch in all six renderers dead code. A failed proxy call rendered as a bare label — indistinguishable from a successful call that returned no rows.How
used for all six gates. The fix belongs here rather than in
live_view:"failed"is the correct status and is consumed elsewhere (get_css_classes,status_icon) — only the gate was too narrow.Success rendering is unaffected: every one of the six blocks already branches on the error key first, so an error payload takes the error branch and a success payload takes the same path it always did.
One behaviour change worth flagging:
_format_replay_tool_result(tools.py:420) sets"success": replay["status"] == "DONE", i.e. a replay that did not reach DONE is the only failure payload with noerrorkey. It previously rendered nothing at all; it now renders its replay response (elapsed, status code, body). That seemed strictly better than a blank line, but say the word if you'd rather it render an explicit "replay did not complete" instead.Tests
In
tests/test_proxy_renderer.py:test_error_is_rendered_for_failed_status— parametrised over all six renderers (ListRequests,ViewRequest,RepeatRequest,ListSitemap,ViewSitemapEntry,ScopeRules), deriving the status from the real_tool_status_from_resultrather than hard-coding"failed", so the test stays honest if that mapping changestest_tool_status_from_result_maps_failure_to_failed— pins the mapping the gate has to agree withtest_successful_result_still_renders— guards the success pathAll six parametrised cases fail on
main(6 failed, 5 passed), pass with this change (11 passed).Both failures are pre-existing on
main(tests/test_runner_root_prompt.py, stale settings stub) and unrelated — separate PR open.ruff format --check,ruff checkandmypy strix/interface/tui/renderers/proxy_renderer.pyare clean.