Address AI review findings: content-first parsing, response helper, tests

P1: Relax status check — extract content first via _extract_response_text(),
only fail when no usable content is found (not on status mismatch alone).
P2: Extract response parsing into dedicated helper with unit tests covering
multiple payload shapes (missing status, null status, SDK output_text,
multiple output blocks). Add reasoning-model timeout hint to stderr.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: igerber

.claude/scripts/openai_review.py

@@ -1111,6 +1111,25 @@ def estimate_tokens(text: str) -> int:
     return len(text) // 4
 
 
+def _extract_response_text(result: dict) -> str:
+    """Extract review text from a Responses API JSON payload.
+
+    Tries the top-level ``output_text`` convenience field first (populated by
+    the Python SDK but typically null in raw HTTP responses), then walks
+    ``output[].content[]`` items.  Returns an empty string when no text is
+    found so the caller can decide how to handle it.
+    """
+    text = result.get("output_text") or ""
+    if text:
+        return text
+    for item in result.get("output", []):
+        if item.get("type") == "message":
+            for block in item.get("content", []):
+                if block.get("type") == "output_text":
+                    text += block.get("text", "")
+    return text
+
+
 def call_openai(
     prompt: str,
     model: str,
@@ -1188,31 +1207,23 @@ def call_openai(
         print(f"Error: Network error — {e.reason}", file=sys.stderr)
         sys.exit(1)
 
-    status = result.get("status")
-    if status != "completed":
+    content = _extract_response_text(result)
+
+    if not content.strip():
+        # No usable content — report the best diagnostic we have.
+        status = result.get("status", "<missing>")
         detail = result.get("incomplete_details") or result.get("error") or ""
-        print(
-            f"Error: OpenAI response status is '{status}' (expected 'completed').",
-            file=sys.stderr,
-        )
+        if status not in ("completed", "<missing>"):
+            print(
+                f"Error: OpenAI response status is '{status}' with no review content.",
+                file=sys.stderr,
+            )
+        else:
+            print("Error: Empty review content from OpenAI API.", file=sys.stderr)
         if detail:
             print(f"Detail: {detail}", file=sys.stderr)
         sys.exit(1)
 
-    # Extract text from output items (output_text is null in raw HTTP responses;
-    # the convenience property only exists in the Python SDK).
-    content = result.get("output_text") or ""
-    if not content:
-        for item in result.get("output", []):
-            if item.get("type") == "message":
-                for block in item.get("content", []):
-                    if block.get("type") == "output_text":
-                        content += block.get("text", "")
-
-    if not content.strip():
-        print("Error: Empty review content from OpenAI API.", file=sys.stderr)
-        sys.exit(1)
-
     usage = result.get("usage", {})
     return (content, usage)
 
@@ -1585,6 +1596,12 @@ def main() -> None:
         sys.exit(0)
 
     # Call OpenAI API
+    if _is_reasoning_model(args.model) and args.timeout == DEFAULT_TIMEOUT:
+        print(
+            f"Note: {args.model} is a reasoning model. Consider --timeout 900 "
+            "for large reviews.",
+            file=sys.stderr,
+        )
     print(f"Sending review to {args.model}...", file=sys.stderr)
     print(f"Estimated input tokens: ~{est_tokens:,}", file=sys.stderr)
     if cost_str:

tests/test_openai_review.py

@@ -1572,6 +1572,37 @@ def test_pro_snapshot_matches_pro(self, review_mod):
         assert snapshot == base
 
 
+class TestExtractResponseText:
+    def test_prefers_output_text_field(self, review_mod):
+        result = {"output_text": "Direct text.", "output": []}
+        assert review_mod._extract_response_text(result) == "Direct text."
+
+    def test_walks_output_items_when_output_text_null(self, review_mod):
+        result = {
+            "output_text": None,
+            "output": [{"type": "message", "content": [
+                {"type": "output_text", "text": "Walked text."},
+            ]}],
+        }
+        assert review_mod._extract_response_text(result) == "Walked text."
+
+    def test_concatenates_multiple_blocks(self, review_mod):
+        result = {
+            "output_text": None,
+            "output": [{"type": "message", "content": [
+                {"type": "output_text", "text": "A"},
+                {"type": "output_text", "text": "B"},
+            ]}],
+        }
+        assert review_mod._extract_response_text(result) == "AB"
+
+    def test_empty_when_no_output(self, review_mod):
+        assert review_mod._extract_response_text({"output_text": None, "output": []}) == ""
+
+    def test_empty_when_missing_keys(self, review_mod):
+        assert review_mod._extract_response_text({}) == ""
+
+
 class TestResponsesAPIConstants:
     def test_endpoint_is_responses(self, review_mod):
         assert "responses" in review_mod.ENDPOINT
@@ -1652,8 +1683,63 @@ def test_timeout_passed_through(self, review_mod, mock_urlopen):
         review_mod.call_openai("test", "gpt-5.4", "fake-key", timeout=900)
         assert mock_urlopen["timeout"] == 900
 
-    def test_incomplete_status_exits(self, review_mod, mock_urlopen):
-        """Non-completed status should cause sys.exit."""
+    def test_missing_status_with_valid_output_succeeds(self, review_mod, mock_urlopen):
+        """Valid content should be accepted even when status field is absent."""
+        mock_urlopen["response_data"] = {
+            "output_text": None,
+            "output": [{
+                "type": "message",
+                "content": [{"type": "output_text", "text": "Good review."}],
+            }],
+            "usage": {"input_tokens": 10, "output_tokens": 5},
+        }
+        content, _ = review_mod.call_openai("test", "gpt-5.4", "fake-key")
+        assert content == "Good review."
+
+    def test_status_none_with_valid_output_succeeds(self, review_mod, mock_urlopen):
+        """status=None should not prevent content extraction."""
+        mock_urlopen["response_data"] = {
+            "status": None,
+            "output_text": None,
+            "output": [{
+                "type": "message",
+                "content": [{"type": "output_text", "text": "Good review."}],
+            }],
+            "usage": {"input_tokens": 10, "output_tokens": 5},
+        }
+        content, _ = review_mod.call_openai("test", "gpt-5.4", "fake-key")
+        assert content == "Good review."
+
+    def test_output_text_convenience_field_used(self, review_mod, mock_urlopen):
+        """When output_text is populated (SDK-style), use it directly."""
+        mock_urlopen["response_data"] = {
+            "status": "completed",
+            "output_text": "SDK-provided text.",
+            "output": [],
+            "usage": {"input_tokens": 10, "output_tokens": 5},
+        }
+        content, _ = review_mod.call_openai("test", "gpt-5.4", "fake-key")
+        assert content == "SDK-provided text."
+
+    def test_multiple_output_text_blocks_concatenated(self, review_mod, mock_urlopen):
+        """Multiple output_text blocks should be concatenated in order."""
+        mock_urlopen["response_data"] = {
+            "status": "completed",
+            "output_text": None,
+            "output": [{
+                "type": "message",
+                "content": [
+                    {"type": "output_text", "text": "Part 1. "},
+                    {"type": "output_text", "text": "Part 2."},
+                ],
+            }],
+            "usage": {"input_tokens": 10, "output_tokens": 5},
+        }
+        content, _ = review_mod.call_openai("test", "gpt-5.4", "fake-key")
+        assert content == "Part 1. Part 2."
+
+    def test_failed_status_no_content_exits(self, review_mod, mock_urlopen):
+        """Failed status with no usable content should exit."""
         mock_urlopen["response_data"] = {
             "status": "failed",
             "output_text": None,
@@ -1664,7 +1750,7 @@ def test_incomplete_status_exits(self, review_mod, mock_urlopen):
             review_mod.call_openai("test", "gpt-5.4", "fake-key")
 
     def test_empty_output_exits(self, review_mod, mock_urlopen):
-        """Empty output items should cause sys.exit."""
+        """Empty output items with completed status should exit."""
         mock_urlopen["response_data"] = {
             "status": "completed",
             "output_text": None,