From 8251aba83974292538db72ca46c2feb72a9c807e Mon Sep 17 00:00:00 2001 From: Song Seung Hu Date: Fri, 24 Apr 2026 22:02:58 +0900 Subject: [PATCH] Harden Jules API helper edge cases --- google-jules-control/scripts/jules_api.py | 36 +++++++-- tests/test_jules_api.py | 95 +++++++++++++++++++++++ 2 files changed, 123 insertions(+), 8 deletions(-) diff --git a/google-jules-control/scripts/jules_api.py b/google-jules-control/scripts/jules_api.py index 6e1ebcc..015de58 100644 --- a/google-jules-control/scripts/jules_api.py +++ b/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.") diff --git a/tests/test_jules_api.py b/tests/test_jules_api.py index af9ac34..e92ecda 100644 --- a/tests/test_jules_api.py +++ b/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",