From 2926f0c141f7ed888a65370b3da02feba65fdaab Mon Sep 17 00:00:00 2001 From: Turgay Nalbant <182372382+tnalbant@users.noreply.github.com> Date: Sat, 17 Oct 2026 12:51:09 +0300 Subject: [PATCH] fix(translate): retry a sentence batch whose reply cannot be read --- backend/ai_logic.py | 72 ++++++++++--- tests/test_provider_request_retry.py | 154 +++++++++++++++++++++++++++ 2 files changed, 211 insertions(+), 15 deletions(-) diff --git a/backend/ai_logic.py b/backend/ai_logic.py index 4fb4999b..0b328c3b 100644 --- a/backend/ai_logic.py +++ b/backend/ai_logic.py @@ -1417,28 +1417,55 @@ def _first_char_after(line: str, index: int) -> str | None: return line[index] if index < len(line) else None -def _strip_json_code_fence(text: str) -> str: - stripped = text.strip() - if not stripped.startswith("```"): - return stripped - stripped = re.sub(r"^```(?:json)?\s*", "", stripped, flags=re.IGNORECASE) - stripped = re.sub(r"\s*```$", "", stripped) - return stripped.strip() +class _UnusableSentenceReplyError(RuntimeError): + """A sentence-translation reply that cannot be read as translation pairs. + + Raised for a reply that is not JSON, is JSON without pairs, or is missing a + translation for one of the requested units. The caller retries the batch in + smaller pieces before giving up on it. + """ + + +_FENCED_BLOCK_PATTERN = re.compile( + r"```(?:json)?\s*(.*?)\s*```", re.DOTALL | re.IGNORECASE +) + + +def _extract_json_object(text: str) -> str: + """Return the JSON object inside a provider reply, ignoring surrounding text. + + Models — free ones especially — wrap the JSON in a fenced block, prefix it + with a sentence, or append a closing remark. Only the object itself parses, + so a fenced block wins; otherwise the outermost ``{...}`` span is used. Text + holding neither is returned unchanged, so the caller still reports the raw + reply as unusable instead of silently accepting it. + """ + stripped = (text or "").strip() + fenced = _FENCED_BLOCK_PATTERN.search(stripped) + if fenced: + stripped = fenced.group(1).strip() + start = stripped.find("{") + end = stripped.rfind("}") + if start != -1 and end > start: + return stripped[start : end + 1] + return stripped def _coerce_translation_pairs( response_text: str, source_units: list[str] ) -> list[dict[str, str]]: try: - data = json.loads(_strip_json_code_fence(response_text)) + data = json.loads(_extract_json_object(response_text)) except json.JSONDecodeError as exc: - raise RuntimeError( + raise _UnusableSentenceReplyError( "AI provider returned invalid sentence translation JSON." ) from exc raw_pairs = data.get("pairs") if isinstance(data, dict) else data if not isinstance(raw_pairs, list): - raise RuntimeError("AI provider did not return sentence translation pairs.") + raise _UnusableSentenceReplyError( + "AI provider did not return sentence translation pairs." + ) pairs: list[dict[str, str]] = [] for index, source in enumerate(source_units): @@ -1452,7 +1479,9 @@ def _coerce_translation_pairs( pairs.append({"source": source, "translated": translated.strip()}) if any(not pair["translated"] for pair in pairs): - raise RuntimeError("AI provider returned incomplete sentence translations.") + raise _UnusableSentenceReplyError( + "AI provider returned incomplete sentence translations." + ) return pairs @@ -1535,7 +1564,9 @@ def _request_sentence_translation_batch_once( response_text = _extract_openai_compatible_text(response.json()).strip() if not response_text: - raise RuntimeError("AI provider returned an empty sentence translation.") + raise _UnusableSentenceReplyError( + "AI provider returned an empty sentence translation." + ) return _coerce_translation_pairs(response_text, source_units) @@ -1547,14 +1578,18 @@ def _request_sentence_translation_batch_from_provider( source_language: str, target_language: str, ) -> list[dict[str, str]]: - """Translate a batch, shrinking it when the provider runs out of context. + """Translate a batch, shrinking it when the reply cannot be used. A single sentence far below the batch limit can still trip a provider's context limit once the system and user prompts are included. When the provider reports ``context_length_exceeded`` (or an equivalent message), the batch is split in half and each half is retried recursively so only - the offending units keep shrinking. A single unit that still fails is - surfaced as a fatal error, since there is nothing left to shrink. + the offending units keep shrinking. A reply that cannot be read as + translation pairs — invalid JSON, no pairs, or a missing translation, which + free and smaller models produce intermittently — is treated the same way, + because a smaller prompt is usually answered correctly. A single unit that + still fails is surfaced as a provider error with a message the user can act + on, since there is nothing left to shrink. """ try: return _request_sentence_translation_batch_once( @@ -1565,6 +1600,13 @@ def _request_sentence_translation_batch_from_provider( source_language, target_language, ) + except _UnusableSentenceReplyError as exc: + if len(source_units) <= 1: + raise ProviderRequestError( + 502, + "The AI provider returned a sentence translation that could not " + "be read. Please try again, or choose a different model.", + ) from exc except ProviderRequestError as exc: if not _is_context_length_error(exc.detail) or len(source_units) <= 1: raise diff --git a/tests/test_provider_request_retry.py b/tests/test_provider_request_retry.py index c494ec4b..1ec6749e 100644 --- a/tests/test_provider_request_retry.py +++ b/tests/test_provider_request_retry.py @@ -402,3 +402,157 @@ def translate_markdown_with_ai(self, *args, **kwargs): ) self.assertEqual(response.status_code, 429) self.assertIn("quota", response.json()["detail"]) + + +def _raw_content_payload(text: str) -> dict: + """A provider reply whose content is raw text, not necessarily clean JSON.""" + return {"choices": [{"message": {"content": text}}]} + + +def _json_batch_reply(*units: str) -> str: + return json.dumps( + {"pairs": [{"source": unit, "translated": unit + "!"} for unit in units]} + ) + + +class TestUnusableSentenceReplyRecovery(unittest.TestCase): + """A reply that cannot be read as sentence pairs is retried, then reported. + + Free and smaller models occasionally answer with prose around the JSON, with + the JSON cut short, or with something that is not JSON at all. Such a reply + used to abort the whole translation with an unhandled error (HTTP 500), so a + single bad reply lost every batch of the document. + """ + + def setUp(self) -> None: + self.patcher = patch.object(logic.time, "sleep") + self.patcher.start() + self.addCleanup(self.patcher.stop) + + def _run_batch(self, responses, units): + """Run one sentence batch and return ``(pairs, mock_post)``.""" + with tempfile.TemporaryDirectory() as tmp_dir: + settings_file = Path(tmp_dir) / "settings.json" + with ( + patch.object(logic, "APP_SETTINGS_FILE_PATH", settings_file), + patch.object(logic, "keyring", None), + patch.dict( + os.environ, + { + "AI_PROVIDER": "openrouter", + "OPENROUTER_API_KEY": "test-key", + "OPENROUTER_MODEL": "nvidia/nemotron-3.5-lightning:free", + }, + ), + patch.object( + logic.requests, "post", side_effect=responses + ) as mock_post, + ): + pairs = logic.translate_markdown_sentence_batch_with_ai( + units, "English", "German" + ) + return pairs, mock_post + + def test_prose_wrapped_json_reply_is_accepted(self) -> None: + content = ( + "Here is the translation:\n" + "```json\n" + _json_batch_reply("A", "B") + "\n```\n" + "Let me know if you need anything else." + ) + responses = [_FakeResponse(200, _raw_content_payload(content))] + pairs, mock_post = self._run_batch(responses, ["A", "B"]) + self.assertEqual([pair["translated"] for pair in pairs], ["A!", "B!"]) + self.assertEqual(mock_post.call_count, 1) + + def test_unreadable_reply_splits_the_batch(self) -> None: + responses = [ + _FakeResponse(200, _raw_content_payload("I cannot translate this.")), + _FakeResponse(200, _raw_content_payload(_json_batch_reply("A", "B"))), + _FakeResponse(200, _raw_content_payload(_json_batch_reply("C", "D"))), + ] + pairs, mock_post = self._run_batch(responses, ["A", "B", "C", "D"]) + self.assertEqual([pair["source"] for pair in pairs], ["A", "B", "C", "D"]) + self.assertEqual(mock_post.call_count, 3) + + def test_truncated_reply_splits_the_batch(self) -> None: + truncated = _json_batch_reply("A", "B", "C", "D")[:-12] + responses = [ + _FakeResponse(200, _raw_content_payload(truncated)), + _FakeResponse(200, _raw_content_payload(_json_batch_reply("A", "B"))), + _FakeResponse(200, _raw_content_payload(_json_batch_reply("C", "D"))), + ] + pairs, mock_post = self._run_batch(responses, ["A", "B", "C", "D"]) + self.assertEqual(len(pairs), 4) + self.assertEqual(mock_post.call_count, 3) + + def test_incomplete_pair_list_splits_the_batch(self) -> None: + responses = [ + _FakeResponse(200, _raw_content_payload(_json_batch_reply("A"))), + _FakeResponse(200, _raw_content_payload(_json_batch_reply("A", "B"))), + _FakeResponse(200, _raw_content_payload(_json_batch_reply("C", "D"))), + ] + pairs, mock_post = self._run_batch(responses, ["A", "B", "C", "D"]) + self.assertEqual(len(pairs), 4) + self.assertEqual(mock_post.call_count, 3) + + def test_single_unit_unreadable_reply_is_a_provider_error(self) -> None: + responses = [ + _FakeResponse(200, _raw_content_payload("I cannot translate this.")) + ] + with tempfile.TemporaryDirectory() as tmp_dir: + settings_file = Path(tmp_dir) / "settings.json" + with ( + patch.object(logic, "APP_SETTINGS_FILE_PATH", settings_file), + patch.object(logic, "keyring", None), + patch.dict( + os.environ, + { + "AI_PROVIDER": "openrouter", + "OPENROUTER_API_KEY": "test-key", + "OPENROUTER_MODEL": "nvidia/nemotron-3.5-lightning:free", + }, + ), + patch.object( + logic.requests, "post", side_effect=responses + ) as mock_post, + ): + with self.assertRaises(ProviderRequestError) as ctx: + logic.translate_markdown_sentence_batch_with_ai( + ["A"], "English", "German" + ) + self.assertEqual(ctx.exception.status_code, 502) + self.assertIn("could not be read", ctx.exception.detail) + self.assertEqual(mock_post.call_count, 1) + + def test_batch_endpoint_reports_a_readable_error(self) -> None: + app = FastAPI() + app.include_router(ai_router.router) + responses = [ + _FakeResponse(200, _raw_content_payload("I cannot translate this.")) + ] + with tempfile.TemporaryDirectory() as tmp_dir: + settings_file = Path(tmp_dir) / "settings.json" + with ( + patch.object(logic, "APP_SETTINGS_FILE_PATH", settings_file), + patch.object(logic, "keyring", None), + patch.dict( + os.environ, + { + "AI_PROVIDER": "openrouter", + "OPENROUTER_API_KEY": "test-key", + "OPENROUTER_MODEL": "nvidia/nemotron-3.5-lightning:free", + }, + ), + patch.object(logic.requests, "post", side_effect=responses), + ): + client = TestClient(app) + response = client.post( + "/translate/sentences/batch", + json={ + "items": ["A"], + "source_language": "English", + "target_language": "German", + }, + ) + self.assertEqual(response.status_code, 502) + self.assertIn("could not be read", response.json()["detail"])