[codex] Harden Jules API transport and PR readiness

google-jules-control/scripts/jules_api.py

@@ -18,7 +18,8 @@
 import urllib.request
 from typing import Any
 
-DEFAULT_BASE_URL = os.environ.get("JULES_API_BASE_URL", "https://jules.googleapis.com/v1alpha")
+DEFAULT_BASE_URL = "https://jules.googleapis.com/v1alpha"
+DEFAULT_REQUEST_TIMEOUT_SECONDS = 60
 DEFAULT_TIMEOUT_SECONDS = 60 * 20
 ACTIVE_STATES = {"QUEUED", "PLANNING", "IN_PROGRESS", "PAUSED"}
 WAITING_STATES = {"AWAITING_PLAN_APPROVAL", "AWAITING_USER_FEEDBACK"}
@@ -186,7 +187,8 @@ def build_prompt_from_args(
 
 def build_url(path: str, query: dict[str, Any] | None = None) -> str:
     path = path if path.startswith("/") else f"/{path}"
-    url = f"{DEFAULT_BASE_URL}{path}"
+    base_url = os.environ.get("JULES_API_BASE_URL", DEFAULT_BASE_URL).strip() or DEFAULT_BASE_URL
+    url = f"{base_url.rstrip('/')}{path}"
     if query:
         filtered = {key: value for key, value in query.items() if value not in (None, "", False)}
         if filtered:
@@ -205,7 +207,7 @@ def api_request(method: str, path: str, *, payload: dict[str, Any] | None = None
         data = json.dumps(payload).encode("utf-8")
     request = urllib.request.Request(build_url(path, query), data=data, headers=headers, method=method.upper())
     try:
-        with urllib.request.urlopen(request) as response:
+        with urllib.request.urlopen(request, timeout=DEFAULT_REQUEST_TIMEOUT_SECONDS) as response:
             raw = response.read().decode("utf-8").strip()
     except urllib.error.HTTPError as exc:
         body = exc.read().decode("utf-8", errors="replace").strip()
@@ -215,7 +217,10 @@ def api_request(method: str, path: str, *, payload: dict[str, Any] | None = None
 
     if not raw:
         return {}
-    return json.loads(raw)
+    try:
+        return json.loads(raw)
+    except json.JSONDecodeError:
+        fail("Jules API request returned non-JSON data. Check the API endpoint, credentials, and network/proxy settings.")
 
 
 def build_api_error_message(status_code: int, raw_body: str) -> str:
@@ -543,17 +548,30 @@ def collect_status_check_blockers(checks: Any) -> list[str]:
         name = check.get("name") or check.get("context") or "status-check"
         status = normalize_gh_state(check.get("status"))
         conclusion = normalize_gh_state(check.get("conclusion"))
+        state = normalize_gh_state(check.get("state"))
+        signals = [signal for signal in [conclusion, state, status] if signal]
 
-        if conclusion in FAILED_CHECK_CONCLUSIONS:
-            blockers.append(f"{name}={conclusion}")
+        failed_signal = next((signal for signal in signals if signal in FAILED_CHECK_CONCLUSIONS), None)
+        if failed_signal:
+            blockers.append(f"{name}={failed_signal}")
             continue
 
-        if status in PENDING_CHECK_STATUSES:
-            blockers.append(f"{name}={status}")
+        pending_signal = next((signal for signal in signals if signal in PENDING_CHECK_STATUSES), None)
+        if pending_signal:
+            blockers.append(f"{name}={pending_signal}")
             continue
 
         if status == "COMPLETED" and conclusion not in SUCCESSFUL_CHECK_CONCLUSIONS:
             blockers.append(f"{name}={conclusion or 'UNKNOWN'}")
+            continue
+
+        if any(signal in SUCCESSFUL_CHECK_CONCLUSIONS for signal in signals):
+            continue
+
+        if signals:
+            blockers.append(f"{name}={signals[0]}")
+        else:
+            blockers.append(f"{name}=UNKNOWN")
     return blockers
 
 
@@ -1543,6 +1561,8 @@ def request_pr_rework(args: argparse.Namespace) -> None:
     readiness = [summarize_pr_merge_readiness(pr) for pr in report.get("pullRequests", [])]
     blocked = [item for item in readiness if not item["ready"]]
 
+    if not readiness:
+        fail("No pull request URL was found for this session. Wait for Jules to produce a PR, or inspect the session outputs first.")
     if not blocked:
         fail("All discovered PRs look merge-ready. No rework request message is needed.")
 

tests/test_jules_api.py

@@ -15,6 +15,47 @@
 
 
 class JulesApiTests(unittest.TestCase):
+    class _FakeResponse:
+        def __init__(self, body: str) -> None:
+            self.body = body
+
+        def __enter__(self):
+            return self
+
+        def __exit__(self, exc_type, exc, traceback) -> None:
+            return None
+
+        def read(self) -> bytes:
+            return self.body.encode("utf-8")
+
+    def test_build_url_uses_current_env_base_url(self) -> None:
+        with mock.patch.dict(jules_api.os.environ, {"JULES_API_BASE_URL": "https://example.test/v9"}):
+            url = jules_api.build_url("/sources", {"pageSize": 1})
+
+        self.assertEqual("https://example.test/v9/sources?pageSize=1", url)
+
+    def test_api_request_uses_request_timeout(self) -> None:
+        with mock.patch.dict(jules_api.os.environ, {"JULES_API_KEY": "test-key"}, clear=False):
+            with mock.patch.object(jules_api.urllib.request, "urlopen", return_value=self._FakeResponse("{}")) as urlopen:
+                jules_api.api_request("GET", "/sources")
+
+        self.assertEqual(60, urlopen.call_args.kwargs.get("timeout"))
+
+    def test_api_request_reports_invalid_json_without_traceback(self) -> None:
+        captured: BaseException | None = None
+        stderr = io.StringIO()
+
+        with mock.patch.dict(jules_api.os.environ, {"JULES_API_KEY": "test-key"}, clear=False):
+            with mock.patch.object(jules_api.urllib.request, "urlopen", return_value=self._FakeResponse("not json")):
+                with redirect_stderr(stderr):
+                    try:
+                        jules_api.api_request("GET", "/sources")
+                    except BaseException as exc:
+                        captured = exc
+
+        self.assertIsInstance(captured, SystemExit)
+        self.assertIn("non-JSON", stderr.getvalue())
+
     def test_build_strict_scope_prompt_includes_default_guards(self) -> None:
         prompt = jules_api.build_strict_scope_prompt("Fix the flaky login redirect test.")
 
@@ -124,6 +165,30 @@ def test_request_pr_rework_send_scopes_message_to_current_pr(self) -> None:
         self.assertIn("Only make the minimum changes required to make the current pull request merge-ready.", payload["prompt"])
         self.assertIn("Avoid dependency updates.", payload["prompt"])
 
+    def test_request_pr_rework_reports_missing_pull_request_output(self) -> None:
+        args = argparse.Namespace(
+            session="123",
+            extra_instruction=None,
+            send=False,
+            markdown=False,
+        )
+        session = {"name": "sessions/123", "title": "No PR", "state": "COMPLETED", "outputs": []}
+        report = {
+            "hasPullRequest": False,
+            "pullRequests": [],
+            "mergedPullRequests": [],
+            "allMerged": False,
+        }
+        stderr = io.StringIO()
+
+        with mock.patch.object(jules_api, "api_request", return_value=session):
+            with mock.patch.object(jules_api, "build_merge_report", return_value=report):
+                with redirect_stderr(stderr):
+                    with self.assertRaises(SystemExit):
+                        jules_api.request_pr_rework(args)
+
+        self.assertIn("No pull request URL", stderr.getvalue())
+
     def test_build_parser_supports_scope_control_flags(self) -> None:
         parser = jules_api.build_parser()
 
@@ -163,6 +228,36 @@ def test_in_progress_status_check_is_not_merge_ready(self) -> None:
         self.assertFalse(summary["ready"])
         self.assertIn("ci/test=IN_PROGRESS", summary["blockers"])
 
+    def test_state_only_failed_status_check_is_not_merge_ready(self) -> None:
+        pr = {
+            "status": "not_merged",
+            "mergeable": "MERGEABLE",
+            "mergeStateStatus": "CLEAN",
+            "reviewDecision": None,
+            "statusCheckRollup": [{"context": "ci/legacy", "state": "FAILURE"}],
+        }
+
+        self.assertFalse(jules_api.is_pr_merge_ready(pr))
+
+        summary = jules_api.summarize_pr_merge_readiness(pr)
+        self.assertFalse(summary["ready"])
+        self.assertIn("ci/legacy=FAILURE", summary["blockers"])
+
+    def test_unknown_status_check_shape_is_not_merge_ready(self) -> None:
+        pr = {
+            "status": "not_merged",
+            "mergeable": "MERGEABLE",
+            "mergeStateStatus": "CLEAN",
+            "reviewDecision": None,
+            "statusCheckRollup": [{"name": "ci/unknown"}],
+        }
+
+        self.assertFalse(jules_api.is_pr_merge_ready(pr))
+
+        summary = jules_api.summarize_pr_merge_readiness(pr)
+        self.assertFalse(summary["ready"])
+        self.assertIn("ci/unknown=UNKNOWN", summary["blockers"])
+
     def test_merged_pr_readiness_has_no_spurious_blockers(self) -> None:
         pr = {
             "status": "merged",