From f76e416008ca55f5e81c01a1d01531322453a671 Mon Sep 17 00:00:00 2001 From: Christian-Sidak <61099993+Christian-Sidak@users.noreply.github.com> Date: Fri, 24 Jul 2026 20:12:09 -0700 Subject: [PATCH] fix(rewrite): surface provider auth errors instead of generic message When a quick-rewrite action (e.g. "Rewrite shorter") hits a provider auth failure, stream_llm emits an SSE error event whose data payload uses {"status": 401, "text": "...", "raw": "..."} -- no "error" key. The rewriteWith() SSE parser in chat.js skipped all "event:" lines without setting a flag, and its error check only tested data.error. This caused 401/auth-failure payloads to be silently ignored, leaving newText empty and showing the generic "model returned no rewritten text" message instead of the actual provider error. Fix: track the SSE event type ("event: error") and check data.text alongside data.error, matching the pattern the main chat stream handler already uses. Fixes #5738 Signed-off-by: Christian Sidak Signed-off-by: Christian-Sidak <61099993+Christian-Sidak@users.noreply.github.com> --- static/js/chat.js | 14 +++- ...st_rewrite_provider_auth_error_surfaced.py | 69 +++++++++++++++++++ 2 files changed, 81 insertions(+), 2 deletions(-) create mode 100644 tests/test_rewrite_provider_auth_error_surfaced.py diff --git a/static/js/chat.js b/static/js/chat.js index ea2d8c1bb..e7d4945d8 100644 --- a/static/js/chat.js +++ b/static/js/chat.js @@ -5738,6 +5738,7 @@ import { wireArrowUpRecall, getUserMessagesFromChatHistory } from './composerArr const decoder = new TextDecoder(); let buffer = ''; let newText = ''; + let _rwNextIsError = false; while (true) { const { done, value } = await reader.read(); @@ -5747,6 +5748,10 @@ import { wireArrowUpRecall, getUserMessagesFromChatHistory } from './composerArr buffer = lines.pop() || ''; for (const line of lines) { + if (line.startsWith('event: ')) { + if (line.slice(7).trim() === 'error') _rwNextIsError = true; + continue; + } if (!line.startsWith('data: ')) continue; const payload = line.slice(6).trim(); if (payload === '[DONE]') continue; @@ -5754,9 +5759,14 @@ import { wireArrowUpRecall, getUserMessagesFromChatHistory } from './composerArr const data = JSON.parse(payload); // The endpoint streams `event: error\ndata: {error,status}` on // failure — surface it instead of silently hanging on "Rewriting…". - if (data.error) { - throw new Error(data.error || ('HTTP ' + (data.status || 500))); + // Provider HTTP errors (e.g. 401 auth failures) arrive with a "text" + // field rather than "error" -- check both, and also honour the SSE + // event type flag set above. + if (_rwNextIsError || data.error || data.text) { + _rwNextIsError = false; + throw new Error(data.error || data.text || ('HTTP ' + (data.status || 500))); } + _rwNextIsError = false; // Reasoning tokens (vLLM --reasoning-parser: Qwen3 / DeepSeek-R1) // arrive as separate {delta, thinking:true} chunks. They are NOT // the rewrite — fold them away so they don't pollute the result. diff --git a/tests/test_rewrite_provider_auth_error_surfaced.py b/tests/test_rewrite_provider_auth_error_surfaced.py new file mode 100644 index 000000000..a9cae8a12 --- /dev/null +++ b/tests/test_rewrite_provider_auth_error_surfaced.py @@ -0,0 +1,69 @@ +"""Regression guard for issue #5738. + +When a quick rewrite action (e.g. "Rewrite shorter") triggers a provider +auth failure (HTTP 401), stream_llm emits an SSE error event whose data +payload uses a "text" field rather than "error": + + event: error + data: {"status": 401, "text": "...", "raw": "..."} + +The rewriteWith() function in chat.js previously only checked `data.error`, +so the auth-failure payload was silently ignored, `newText` stayed empty, +and the UI showed the generic "model returned no rewritten text" message +instead of the actual provider error. + +The fix must: +1. Track `event: error` SSE lines (not skip them entirely). +2. Check `data.text` alongside `data.error` when deciding whether the + chunk represents a provider-side failure. +""" +import re +from pathlib import Path + +CHAT_JS = Path(__file__).resolve().parent.parent / "static/js/chat.js" + + +def _rewrite_with_body() -> str: + """Return the source of the rewriteWith export function.""" + text = CHAT_JS.read_text(encoding="utf-8") + start = text.index("export async function rewriteWith(") + rest = text[start:] + # Stop at the next top-level export/function so we only look at rewriteWith. + m = re.search(r"\n(export |function )", rest[1:]) + return rest[: m.start() + 1] if m else rest + + +def test_rewrite_tracks_event_error_sse_type(): + """rewriteWith must parse 'event: error' lines, not skip them.""" + body = _rewrite_with_body() + assert re.search( + r"""line\.startsWith\s*\(\s*['"]event:\s*['"]\s*\)""", body + ), ( + "rewriteWith must handle 'event: ' lines so the SSE error type can be " + "tracked; previously these were skipped entirely" + ) + + +def test_rewrite_error_check_includes_data_text(): + """rewriteWith error detection must cover data.text, not only data.error. + + Provider HTTP errors (e.g. 401 auth failures from ChatGPT Subscription) + arrive as {"status": N, "text": "...", "raw": "..."} -- no "error" key. + Without checking data.text the failure is silently swallowed. + """ + body = _rewrite_with_body() + # The condition that throws must reference data.text. + assert "data.text" in body, ( + "rewriteWith error condition must check data.text to catch provider " + "HTTP errors whose payload uses 'text' instead of 'error'" + ) + + +def test_rewrite_error_message_prefers_human_readable_text(): + """The thrown Error must use data.error || data.text as the message.""" + body = _rewrite_with_body() + # Allow either order; both are acceptable. + assert re.search(r"data\.error\s*\|\|\s*data\.text|data\.text\s*\|\|\s*data\.error", body), ( + "rewriteWith must surface data.error || data.text as the error message " + "so the user sees the provider's explanation rather than a generic fallback" + )