From 91afeaebc4b1f02a89994f2feaf366e75a349378 Mon Sep 17 00:00:00 2001 From: Vasily Ilin Date: Sat, 12 Sep 2026 19:13:34 -0700 Subject: [PATCH] Retry malformed review JSON without repeating completed portions --- python/azure-review.md | 4 +- python/lean_pool/review.py | 41 ++++++++++++++-- python/tests/test_review_portions.py | 73 ++++++++++++++++++++++++++++ 3 files changed, 114 insertions(+), 4 deletions(-) diff --git a/python/azure-review.md b/python/azure-review.md index e7a4a11433..6334cf16bc 100644 --- a/python/azure-review.md +++ b/python/azure-review.md @@ -43,7 +43,9 @@ when later evidence changes its assessment. Missing/malformed portion results or integration evidence that cannot fit fail the run rather than silently reducing coverage. Small diffs still use one call per rubric. -A context-window rejection retries using smaller lossless portions. The token +A malformed JSON response retries the same model call at most twice, preserving +completed portions. Persistent format errors and unrelated worker failures remain +failures. A context-window rejection retries using smaller lossless portions. The token estimate is conservative, not an exact tokenizer measurement; the ceiling does not change the model's actual context window. Reported usage includes all source and integration calls, including successful work before a retry; unknown usage diff --git a/python/lean_pool/review.py b/python/lean_pool/review.py index 040fbb4b2d..5457069cf1 100644 --- a/python/lean_pool/review.py +++ b/python/lean_pool/review.py @@ -75,6 +75,7 @@ import difflib import hashlib import json +import logging import os import re import secrets @@ -94,6 +95,8 @@ from lean_pool import challenge, codex_review, prior_art, review_portions +logger = logging.getLogger(__name__) + REPO_ROOT = Path(__file__).resolve().parents[2] PROJECT_RULES_PATH = REPO_ROOT / ".github" / "REVIEW_RULES.md" REFACTOR_RULES_PATH = REPO_ROOT / ".github" / "REFACTOR_REVIEW_RULES.md" @@ -1303,9 +1306,19 @@ def __init__(self, model: str, effort: str | None) -> None: def send(self, messages: list[dict[str, str]]) -> ReviewResult: """Record attempted and completed calls independently.""" - with self.lock: - self.attempts += 1 - result = _send_review(self.model, messages, self.effort) + for attempt in range(3): + with self.lock: + self.attempts += 1 + try: + result = _send_review(self.model, messages, self.effort) + break + except Exception as error: + if attempt == 2 or not _response_format_failure(error): + raise + logger.warning( + "Malformed review JSON; retrying the same input (attempt %d of 3).", + attempt + 2, + ) with self.lock: self.completed.append(result) if destination := os.environ.get("REVIEW_EVIDENCE_PATH"): @@ -1325,6 +1338,28 @@ def accounted(self, result: ReviewResult) -> ReviewResult: ) +def _response_format_failure(error: Exception) -> bool: + """Recognize malformed model JSON without retrying unrelated worker errors.""" + if isinstance(error, json.JSONDecodeError): + return True + message = str(error).lower() + return ( + isinstance(error, RuntimeError) + and "azure codex review failed:" in message + and re.search(r"line \d+ column \d+ \(char \d+\)", message) is not None + and any( + marker in message + for marker in ( + "expecting ", + "unterminated string", + "invalid \\", + "invalid control character", + "extra data", + ) + ) + ) + + def _context_rejection(error: Exception) -> bool: """Recognize only context overflow, never quota or transport failures.""" return _is_token_overflow(error) or ( diff --git a/python/tests/test_review_portions.py b/python/tests/test_review_portions.py index b328fe83b0..5f2f81d743 100644 --- a/python/tests/test_review_portions.py +++ b/python/tests/test_review_portions.py @@ -718,3 +718,76 @@ def test_unknown_reopened_obligation_cannot_approve(): {"verdict": "pass", "reopened_obligations": ["invented"]}, {}, {} ) assert review_portions.enforce_resolutions(updated, {})["verdict"] == "discuss" + + +@pytest.mark.parametrize("azure", [False, True]) +def test_response_format_retry_preserves_accounting(monkeypatch, azure): + """Malformed JSON retries only the same request and discloses unmetered work.""" + calls = [] + + def send(model, messages, effort): + calls.append(messages) + if len(calls) < 3: + if azure: + raise RuntimeError( + "Azure Codex review failed: Expecting ',' delimiter: " + "line 1 column 2 (char 1)" + ) + raise json.JSONDecodeError("Expecting ',' delimiter", "{broken}", 1) + return result({"verdict": "pass"}) + + monkeypatch.setattr(review, "_send_review", send) + session = review._ReviewSession(review.DEFAULT_MODEL, "xhigh") + messages = [{"role": "user", "content": "the original input"}] + answer = session.accounted(session.send(messages)) + assert calls == [messages, messages, messages] + assert answer.calls == 3 + assert len(answer.requests) == 1 + assert answer.usage is None + + +def test_response_format_retry_is_bounded(monkeypatch): + """Persistent malformed output cannot retry indefinitely or approve.""" + + def send(*args): + raise json.JSONDecodeError("Expecting value", "", 0) + + monkeypatch.setattr(review, "_send_review", send) + session = review._ReviewSession(review.DEFAULT_MODEL, "xhigh") + with pytest.raises(json.JSONDecodeError): + session.send([]) + assert session.attempts == 3 + assert session.completed == [] + + +def test_format_failure_retries_one_portion_without_repeating_others(monkeypatch): + """A formatting error cannot restart all the source work for a rubric.""" + monkeypatch.setattr(review, "MAX_INPUT_TOKENS", 10_000) + calls = {} + + def send(model, messages, effort): + user = messages[1]["content"] + match = re.search(r"source portion (\d+)/", user) + if not match: + return result({"verdict": "pass"}) + number = int(match.group(1)) + calls[number] = calls.get(number, 0) + 1 + if number == 1 and calls[number] == 1: + raise json.JSONDecodeError("Expecting ',' delimiter", "{broken}", 1) + return result( + { + "verdict": "pass", + "evidence_summary": "Local evidence checked", + "open_questions": [], + } + ) + + monkeypatch.setattr(review, "_send_review", send) + answer = review.request_review( + review.DEFAULT_MODEL, "rules", source_diff(), "review" + ) + assert answer.payload["verdict"] == "pass" + assert calls[1] == 2 + assert all(count == 1 for number, count in calls.items() if number != 1) + assert len(calls) == answer.portions + assert answer.calls == answer.portions + 2