bito-code-review[bot] commented on code in PR #44439: URL: https://github.com/apache/superset/pull/44439#discussion_r4052373997
########## automation/orchestrator/api.py: ########## @@ -0,0 +1,342 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""HTTP clients for GitHub and the Devin API with a shared retry policy. + +Retry contract +-------------- +* ``429`` and rate-limited ``403`` (primary/secondary/abuse) are retried for + every method: the server rejected the request, nothing happened. +* ``5xx`` and transport errors (timeouts, resets) are retried for idempotent + methods (GET/HEAD/PUT/DELETE). For POST/PATCH they raise + :class:`AmbiguousWriteError` so the caller probes for the resource before + writing again -- this is what keeps retries idempotent. +* Other ``4xx`` raise :class:`PermanentError` immediately. +* Waits use exponential backoff with full jitter (tenacity), capped per + attempt and by a total budget, and honour ``Retry-After`` / + ``X-RateLimit-Reset`` when present. + +Secrets are only ever placed in request headers; they are never logged. +""" + +from __future__ import annotations + +import email.utils +import logging +import os +import time +from collections.abc import Callable, Iterator +from dataclasses import dataclass, field +from typing import Any + +import requests +from tenacity import ( + retry_if_exception_type, + RetryCallState, + Retrying, + stop_after_attempt, + stop_after_delay, + wait_exponential_jitter, +) + +log = logging.getLogger("automation.api") + +IDEMPOTENT_METHODS = frozenset({"GET", "PUT", "DELETE"}) +RATE_LIMIT_MARKERS = ("rate limit", "abuse detection", "too many requests") + + +class ApiError(Exception): + """Base class for API failures.""" + + def __init__(self, message: str, status: int | None = None) -> None: + super().__init__(message) + self.status = status + + +class TransientError(ApiError): + """Safe to retry: the request was rejected or the read failed.""" + + def __init__( + self, message: str, status: int | None = None, retry_after: float | None = None + ) -> None: + super().__init__(message, status) + self.retry_after = retry_after Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing docstrings on new __init__ methods</b></div> <div id="fix"> BITO rule 12147 requires docstrings on every new function; `ApiError.__init__` and `TransientError.__init__` have none, and `TransientError.retry_after`'s semantics (seconds, server hint consumed by `_wait`) are only inferable from `_retry_after_seconds`. Add brief docstrings documenting the `status`/`retry_after` parameters. </div> </div> <small><i>Code Review Run #8b0af2</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## automation/orchestrator/api.py: ########## @@ -0,0 +1,342 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""HTTP clients for GitHub and the Devin API with a shared retry policy. + +Retry contract +-------------- +* ``429`` and rate-limited ``403`` (primary/secondary/abuse) are retried for + every method: the server rejected the request, nothing happened. +* ``5xx`` and transport errors (timeouts, resets) are retried for idempotent + methods (GET/HEAD/PUT/DELETE). For POST/PATCH they raise + :class:`AmbiguousWriteError` so the caller probes for the resource before + writing again -- this is what keeps retries idempotent. +* Other ``4xx`` raise :class:`PermanentError` immediately. +* Waits use exponential backoff with full jitter (tenacity), capped per + attempt and by a total budget, and honour ``Retry-After`` / + ``X-RateLimit-Reset`` when present. + +Secrets are only ever placed in request headers; they are never logged. +""" + +from __future__ import annotations + +import email.utils +import logging +import os +import time +from collections.abc import Callable, Iterator +from dataclasses import dataclass, field +from typing import Any + +import requests +from tenacity import ( + retry_if_exception_type, + RetryCallState, + Retrying, + stop_after_attempt, + stop_after_delay, + wait_exponential_jitter, +) + +log = logging.getLogger("automation.api") + +IDEMPOTENT_METHODS = frozenset({"GET", "PUT", "DELETE"}) +RATE_LIMIT_MARKERS = ("rate limit", "abuse detection", "too many requests") + + +class ApiError(Exception): + """Base class for API failures.""" + + def __init__(self, message: str, status: int | None = None) -> None: + super().__init__(message) + self.status = status + + +class TransientError(ApiError): + """Safe to retry: the request was rejected or the read failed.""" + + def __init__( + self, message: str, status: int | None = None, retry_after: float | None = None + ) -> None: + super().__init__(message, status) + self.retry_after = retry_after + + +class AmbiguousWriteError(ApiError): + """A write may or may not have landed; probe before repeating it.""" + + +class PermanentError(ApiError): + """Client error that will not succeed on retry (401/404/422...).""" + + +@dataclass(frozen=True) +class RetryPolicy: + max_attempts: int = int(os.environ.get("RETRY_MAX_ATTEMPTS", "6")) + initial: float = float(os.environ.get("RETRY_BASE_SECONDS", "2")) + max_sleep: float = float(os.environ.get("RETRY_MAX_SLEEP", "120")) + budget_seconds: float = float(os.environ.get("RETRY_BUDGET_SECONDS", "900")) + timeout: float = float(os.environ.get("HTTP_TIMEOUT", "60")) + + +def _wait(policy: RetryPolicy) -> Callable[[RetryCallState], float]: + """Exponential backoff with full jitter, overridden by a server hint.""" + jitter = wait_exponential_jitter(initial=policy.initial, max=policy.max_sleep) + + def wait(state: RetryCallState) -> float: + exc = state.outcome.exception() if state.outcome else None + if isinstance(exc, TransientError) and exc.retry_after is not None: + return min(max(exc.retry_after, 0.0), policy.max_sleep) + return float(jitter(state)) + + return wait + + +def _retry_after_seconds(headers: Any) -> float | None: + """Parse Retry-After (seconds or HTTP-date) or X-RateLimit-Reset (epoch).""" + if raw := headers.get("Retry-After"): + try: + return float(raw) + except ValueError: Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Uncaught Retry-After parse error</b></div> <div id="fix"> `_retry_after_seconds` guards `float(raw)` but not `email.utils.parsedate_to_datetime(raw)`: a malformed `Retry-After` (e.g. "soon") raises `ValueError` inside `_classify`'s raise path, escaping the `TransientError` contract and surfacing as an unhandled `ValueError` to `request()` callers. Catch the parse failure and return `None`. </div> </div> <small><i>Code Review Run #8b0af2</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## automation/orchestrator/api.py: ########## @@ -0,0 +1,342 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""HTTP clients for GitHub and the Devin API with a shared retry policy. + +Retry contract +-------------- +* ``429`` and rate-limited ``403`` (primary/secondary/abuse) are retried for + every method: the server rejected the request, nothing happened. +* ``5xx`` and transport errors (timeouts, resets) are retried for idempotent + methods (GET/HEAD/PUT/DELETE). For POST/PATCH they raise + :class:`AmbiguousWriteError` so the caller probes for the resource before + writing again -- this is what keeps retries idempotent. +* Other ``4xx`` raise :class:`PermanentError` immediately. +* Waits use exponential backoff with full jitter (tenacity), capped per + attempt and by a total budget, and honour ``Retry-After`` / + ``X-RateLimit-Reset`` when present. + +Secrets are only ever placed in request headers; they are never logged. +""" + +from __future__ import annotations + +import email.utils +import logging +import os +import time +from collections.abc import Callable, Iterator +from dataclasses import dataclass, field +from typing import Any + +import requests +from tenacity import ( + retry_if_exception_type, + RetryCallState, + Retrying, + stop_after_attempt, + stop_after_delay, + wait_exponential_jitter, +) + +log = logging.getLogger("automation.api") + +IDEMPOTENT_METHODS = frozenset({"GET", "PUT", "DELETE"}) +RATE_LIMIT_MARKERS = ("rate limit", "abuse detection", "too many requests") + + +class ApiError(Exception): + """Base class for API failures.""" + + def __init__(self, message: str, status: int | None = None) -> None: + super().__init__(message) + self.status = status + + +class TransientError(ApiError): + """Safe to retry: the request was rejected or the read failed.""" + + def __init__( + self, message: str, status: int | None = None, retry_after: float | None = None + ) -> None: + super().__init__(message, status) + self.retry_after = retry_after + + +class AmbiguousWriteError(ApiError): + """A write may or may not have landed; probe before repeating it.""" + + +class PermanentError(ApiError): + """Client error that will not succeed on retry (401/404/422...).""" + + +@dataclass(frozen=True) +class RetryPolicy: + max_attempts: int = int(os.environ.get("RETRY_MAX_ATTEMPTS", "6")) + initial: float = float(os.environ.get("RETRY_BASE_SECONDS", "2")) + max_sleep: float = float(os.environ.get("RETRY_MAX_SLEEP", "120")) + budget_seconds: float = float(os.environ.get("RETRY_BUDGET_SECONDS", "900")) + timeout: float = float(os.environ.get("HTTP_TIMEOUT", "60")) + + +def _wait(policy: RetryPolicy) -> Callable[[RetryCallState], float]: + """Exponential backoff with full jitter, overridden by a server hint.""" + jitter = wait_exponential_jitter(initial=policy.initial, max=policy.max_sleep) + + def wait(state: RetryCallState) -> float: + exc = state.outcome.exception() if state.outcome else None + if isinstance(exc, TransientError) and exc.retry_after is not None: + return min(max(exc.retry_after, 0.0), policy.max_sleep) + return float(jitter(state)) + + return wait + + +def _retry_after_seconds(headers: Any) -> float | None: + """Parse Retry-After (seconds or HTTP-date) or X-RateLimit-Reset (epoch).""" + if raw := headers.get("Retry-After"): + try: + return float(raw) + except ValueError: + parsed = email.utils.parsedate_to_datetime(raw) + return max(parsed.timestamp() - time.time(), 0.0) + reset = headers.get("X-RateLimit-Reset") + remaining = headers.get("X-RateLimit-Remaining") + if reset and remaining == "0": + try: + return max(float(reset) - time.time(), 0.0) + except ValueError: + return None + return None + + +def _log_retry(policy: RetryPolicy) -> Callable[[RetryCallState], None]: + def before_sleep(state: RetryCallState) -> None: + log.warning( + "%s. Attempt %d/%d failed, retrying in %.1fs", + state.outcome.exception() if state.outcome else "", + state.attempt_number, + policy.max_attempts, + state.next_action.sleep if state.next_action else 0.0, + ) + + return before_sleep + + +@dataclass +class BaseClient: + """A ``requests`` session with the shared retry policy applied.""" + + base_url: str + token_env: str + policy: RetryPolicy = field(default_factory=RetryPolicy) + user_agent: str = "superset-automation-orchestrator" + session: requests.Session = field(default_factory=requests.Session) + + def _headers(self) -> dict[str, str]: + headers = {"User-Agent": self.user_agent, "Accept": "application/json"} + if token := os.environ.get(self.token_env): + headers["Authorization"] = f"Bearer {token}" + return headers + + def _classify(self, resp: requests.Response, method: str, context: str) -> None: + status = resp.status_code + if status < 400: + return + body = resp.text[:300] + snippet = f"HTTP {status} {resp.reason}: {body}" + if status == 429 or ( + status == 403 + and ( + resp.headers.get("X-RateLimit-Remaining") == "0" + or any(m in body.lower() for m in RATE_LIMIT_MARKERS) + ) + ): + raise TransientError( + f"{context} rate limited ({snippet})", + status, + _retry_after_seconds(resp.headers), + ) + if status >= 500: + if method in IDEMPOTENT_METHODS: + raise TransientError(f"{context} {snippet}", status) + raise AmbiguousWriteError(f"{context} {snippet}", status) + raise PermanentError(f"{context} {snippet}", status) + + def _once( + self, + method: str, + path: str, + context: str, + params: dict[str, Any] | None, + json_body: Any, + ) -> requests.Response: + url = path if path.startswith("http") else f"{self.base_url}/{path.lstrip('/')}" + try: + resp = self.session.request( + method, + url, + headers=self._headers(), + params=params, + json=json_body, + timeout=self.policy.timeout, + ) + except (requests.ConnectionError, requests.Timeout) as exc: + reason = f"{context} {type(exc).__name__}" + if method in IDEMPOTENT_METHODS: + raise TransientError(reason) from exc + raise AmbiguousWriteError(reason) from exc + self._classify(resp, method, context) + return resp + + def request( + self, + method: str, + path: str, + *, + context: str = "", + params: dict[str, Any] | None = None, + json_body: Any = None, + ) -> requests.Response: + """Perform one logical request under the retry contract.""" + method = method.upper() + context = context or f"{method} {path}" + retrying = Retrying( + retry=retry_if_exception_type(TransientError), + wait=_wait(self.policy), + stop=( + stop_after_attempt(self.policy.max_attempts) + | stop_after_delay(self.policy.budget_seconds) + ), + before_sleep=_log_retry(self.policy), + reraise=True, + ) + return retrying(self._once, method, path, context, params, json_body) + + def get_json(self, path: str, **kwargs: Any) -> Any: + return self.request("GET", path, **kwargs).json() + + +class GitHub(BaseClient): + """GitHub REST client bound to one repository.""" + + def __init__(self, repo: str, policy: RetryPolicy | None = None) -> None: + super().__init__( + base_url=os.environ.get("GITHUB_API", "https://api.github.com";), + token_env="GH_TOKEN", # noqa: S106 -- env var name, not a secret + policy=policy or RetryPolicy(), + ) + self.repo = repo + + def _headers(self) -> dict[str, str]: + headers = super()._headers() + headers["X-GitHub-Api-Version"] = "2022-11-28" + headers["Accept"] = "application/vnd.github+json" + return headers + + def paginate(self, path: str, **params: Any) -> Iterator[dict[str, Any]]: + params.setdefault("per_page", 100) + url: str | None = f"repos/{self.repo}/{path}" + while url: + resp = self.request("GET", url, params=params) + yield from resp.json() + url = resp.links.get("next", {}).get("url") + params = {} + + def open_issues(self, label: str) -> list[dict[str, Any]]: + return [ + i + for i in self.paginate("issues", state="open", labels=label) + if "pull_request" not in i + ] + + def pulls(self, state: str = "all") -> list[dict[str, Any]]: + return list(self.paginate("pulls", state=state)) + + def branch_commit_date(self, name: str) -> str: + data = self.get_json(f"repos/{self.repo}/branches/{name}") + return str(data["commit"]["commit"]["committer"]["date"]) + + def delete_branch(self, name: str) -> None: + self.request( + "DELETE", + f"repos/{self.repo}/git/refs/heads/{name}", + context=f"delete branch {name}", + ) + + def comments(self, issue: int) -> list[dict[str, Any]]: + return list(self.paginate(f"issues/{issue}/comments")) + + def create_comment(self, issue: int, body: str) -> dict[str, Any]: + return self.request( + "POST", + f"repos/{self.repo}/issues/{issue}/comments", + context=f"[Issue #{issue}] create comment", + json_body={"body": body}, + ).json() + + def update_comment(self, comment_id: int, body: str) -> dict[str, Any]: + return self.request( + "PATCH", + f"repos/{self.repo}/issues/comments/{comment_id}", + context=f"update comment {comment_id}", + json_body={"body": body}, + ).json() Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Empty-repo 404 aborts sweep</b></div> <div id="fix"> `paginate` treats every non-2xx as an error, but GitHub returns 404 for `/branches` on an empty repository, so `branch_for_issue`/`stale_branches` in `dispatch.py` abort with `PermanentError` instead of seeing zero branches. Consider treating a first-page 404 as an empty result, or handling it at those call sites. </div> </div> <div id="suggestion"> <div id="issue"><b>Missing method docstrings</b></div> <div id="fix"> Per BITO adaptive rule 12147, every newly introduced Python function needs a docstring; the eight methods added here (`paginate` through `update_comment`) have none. Brief docstrings would document pagination semantics (e.g. that `params` only applies to the first request) without reading callers. </div> </div> <small><i>Code Review Run #8b0af2</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## automation/orchestrator/api.py: ########## @@ -0,0 +1,342 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""HTTP clients for GitHub and the Devin API with a shared retry policy. + +Retry contract +-------------- +* ``429`` and rate-limited ``403`` (primary/secondary/abuse) are retried for + every method: the server rejected the request, nothing happened. +* ``5xx`` and transport errors (timeouts, resets) are retried for idempotent + methods (GET/HEAD/PUT/DELETE). For POST/PATCH they raise + :class:`AmbiguousWriteError` so the caller probes for the resource before + writing again -- this is what keeps retries idempotent. +* Other ``4xx`` raise :class:`PermanentError` immediately. +* Waits use exponential backoff with full jitter (tenacity), capped per + attempt and by a total budget, and honour ``Retry-After`` / + ``X-RateLimit-Reset`` when present. + +Secrets are only ever placed in request headers; they are never logged. +""" + +from __future__ import annotations + +import email.utils +import logging +import os +import time +from collections.abc import Callable, Iterator +from dataclasses import dataclass, field +from typing import Any + +import requests +from tenacity import ( + retry_if_exception_type, + RetryCallState, + Retrying, + stop_after_attempt, + stop_after_delay, + wait_exponential_jitter, +) + +log = logging.getLogger("automation.api") + +IDEMPOTENT_METHODS = frozenset({"GET", "PUT", "DELETE"}) +RATE_LIMIT_MARKERS = ("rate limit", "abuse detection", "too many requests") + + +class ApiError(Exception): + """Base class for API failures.""" + + def __init__(self, message: str, status: int | None = None) -> None: + super().__init__(message) + self.status = status + + +class TransientError(ApiError): + """Safe to retry: the request was rejected or the read failed.""" + + def __init__( + self, message: str, status: int | None = None, retry_after: float | None = None + ) -> None: + super().__init__(message, status) + self.retry_after = retry_after + + +class AmbiguousWriteError(ApiError): + """A write may or may not have landed; probe before repeating it.""" + + +class PermanentError(ApiError): + """Client error that will not succeed on retry (401/404/422...).""" + + +@dataclass(frozen=True) +class RetryPolicy: + max_attempts: int = int(os.environ.get("RETRY_MAX_ATTEMPTS", "6")) + initial: float = float(os.environ.get("RETRY_BASE_SECONDS", "2")) + max_sleep: float = float(os.environ.get("RETRY_MAX_SLEEP", "120")) + budget_seconds: float = float(os.environ.get("RETRY_BUDGET_SECONDS", "900")) + timeout: float = float(os.environ.get("HTTP_TIMEOUT", "60")) + + +def _wait(policy: RetryPolicy) -> Callable[[RetryCallState], float]: + """Exponential backoff with full jitter, overridden by a server hint.""" + jitter = wait_exponential_jitter(initial=policy.initial, max=policy.max_sleep) + + def wait(state: RetryCallState) -> float: + exc = state.outcome.exception() if state.outcome else None + if isinstance(exc, TransientError) and exc.retry_after is not None: + return min(max(exc.retry_after, 0.0), policy.max_sleep) + return float(jitter(state)) + + return wait + + +def _retry_after_seconds(headers: Any) -> float | None: + """Parse Retry-After (seconds or HTTP-date) or X-RateLimit-Reset (epoch).""" + if raw := headers.get("Retry-After"): + try: + return float(raw) + except ValueError: + parsed = email.utils.parsedate_to_datetime(raw) + return max(parsed.timestamp() - time.time(), 0.0) + reset = headers.get("X-RateLimit-Reset") + remaining = headers.get("X-RateLimit-Remaining") + if reset and remaining == "0": + try: + return max(float(reset) - time.time(), 0.0) + except ValueError: + return None + return None + + +def _log_retry(policy: RetryPolicy) -> Callable[[RetryCallState], None]: + def before_sleep(state: RetryCallState) -> None: + log.warning( + "%s. Attempt %d/%d failed, retrying in %.1fs", + state.outcome.exception() if state.outcome else "", + state.attempt_number, + policy.max_attempts, + state.next_action.sleep if state.next_action else 0.0, + ) + + return before_sleep + + +@dataclass +class BaseClient: + """A ``requests`` session with the shared retry policy applied.""" + + base_url: str + token_env: str + policy: RetryPolicy = field(default_factory=RetryPolicy) + user_agent: str = "superset-automation-orchestrator" + session: requests.Session = field(default_factory=requests.Session) + + def _headers(self) -> dict[str, str]: + headers = {"User-Agent": self.user_agent, "Accept": "application/json"} + if token := os.environ.get(self.token_env): + headers["Authorization"] = f"Bearer {token}" + return headers + + def _classify(self, resp: requests.Response, method: str, context: str) -> None: + status = resp.status_code + if status < 400: + return + body = resp.text[:300] + snippet = f"HTTP {status} {resp.reason}: {body}" + if status == 429 or ( + status == 403 + and ( + resp.headers.get("X-RateLimit-Remaining") == "0" + or any(m in body.lower() for m in RATE_LIMIT_MARKERS) + ) + ): + raise TransientError( + f"{context} rate limited ({snippet})", + status, + _retry_after_seconds(resp.headers), + ) + if status >= 500: + if method in IDEMPOTENT_METHODS: + raise TransientError(f"{context} {snippet}", status) + raise AmbiguousWriteError(f"{context} {snippet}", status) + raise PermanentError(f"{context} {snippet}", status) + + def _once( + self, + method: str, + path: str, + context: str, + params: dict[str, Any] | None, + json_body: Any, + ) -> requests.Response: + url = path if path.startswith("http") else f"{self.base_url}/{path.lstrip('/')}" + try: + resp = self.session.request( + method, + url, + headers=self._headers(), + params=params, + json=json_body, + timeout=self.policy.timeout, + ) + except (requests.ConnectionError, requests.Timeout) as exc: Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missed transport exceptions</b></div> <div id="fix"> `_once` catches only `requests.ConnectionError`/`Timeout`, but `Session.send` reads `r.content` for non-stream requests, so a truncated chunked body raises `requests.ChunkedEncodingError` — a direct `RequestException` subclass, not a `ConnectionError` (verified against requests 2.33.0). It escapes unclassified, bypassing the module's documented retry contract. Add the decoding-error types to the except tuple. </div> </div> <small><i>Code Review Run #8b0af2</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## automation/orchestrator/api.py: ########## @@ -0,0 +1,342 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""HTTP clients for GitHub and the Devin API with a shared retry policy. + +Retry contract +-------------- +* ``429`` and rate-limited ``403`` (primary/secondary/abuse) are retried for + every method: the server rejected the request, nothing happened. +* ``5xx`` and transport errors (timeouts, resets) are retried for idempotent + methods (GET/HEAD/PUT/DELETE). For POST/PATCH they raise + :class:`AmbiguousWriteError` so the caller probes for the resource before + writing again -- this is what keeps retries idempotent. +* Other ``4xx`` raise :class:`PermanentError` immediately. +* Waits use exponential backoff with full jitter (tenacity), capped per + attempt and by a total budget, and honour ``Retry-After`` / + ``X-RateLimit-Reset`` when present. + +Secrets are only ever placed in request headers; they are never logged. +""" + +from __future__ import annotations + +import email.utils +import logging +import os +import time +from collections.abc import Callable, Iterator +from dataclasses import dataclass, field +from typing import Any + +import requests +from tenacity import ( + retry_if_exception_type, + RetryCallState, + Retrying, + stop_after_attempt, + stop_after_delay, + wait_exponential_jitter, +) + +log = logging.getLogger("automation.api") + +IDEMPOTENT_METHODS = frozenset({"GET", "PUT", "DELETE"}) +RATE_LIMIT_MARKERS = ("rate limit", "abuse detection", "too many requests") + + +class ApiError(Exception): + """Base class for API failures.""" + + def __init__(self, message: str, status: int | None = None) -> None: + super().__init__(message) + self.status = status + + +class TransientError(ApiError): + """Safe to retry: the request was rejected or the read failed.""" + + def __init__( + self, message: str, status: int | None = None, retry_after: float | None = None + ) -> None: + super().__init__(message, status) + self.retry_after = retry_after + + +class AmbiguousWriteError(ApiError): + """A write may or may not have landed; probe before repeating it.""" + + +class PermanentError(ApiError): + """Client error that will not succeed on retry (401/404/422...).""" + + +@dataclass(frozen=True) +class RetryPolicy: + max_attempts: int = int(os.environ.get("RETRY_MAX_ATTEMPTS", "6")) + initial: float = float(os.environ.get("RETRY_BASE_SECONDS", "2")) + max_sleep: float = float(os.environ.get("RETRY_MAX_SLEEP", "120")) + budget_seconds: float = float(os.environ.get("RETRY_BUDGET_SECONDS", "900")) + timeout: float = float(os.environ.get("HTTP_TIMEOUT", "60")) Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Env parsed at import in dataclass defaults</b></div> <div id="fix"> `RetryPolicy` field defaults call `os.environ.get(...)` in the class body, so values are captured once at import: later env changes are ignored (every `RetryPolicy()` reuses the snapshot, e.g. `GitHub`/`Devin` defaults), and a malformed value (e.g. RETRY_MAX_ATTEMPTS=abc) raises ValueError at import, crashing the process before any logging. Parse env in a `from_env()` factory instead. </div> </div> <small><i>Code Review Run #8b0af2</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## automation/orchestrator/dispatch.py: ########## @@ -0,0 +1,290 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Dispatch one Devin fix session per open nightly-scan issue, safely. + +Every step is idempotent: a session is looked up by title before it is +created, a dashboard comment is looked up by a hidden marker before it is +posted, and a failure for one issue is recorded and the loop moves on. +""" + +from __future__ import annotations + +import logging +import re +import time +from collections.abc import Callable, Iterable +from dataclasses import dataclass +from datetime import datetime, timedelta, timezone +from typing import Any + +from .api import AmbiguousWriteError, ApiError, Devin, GitHub, session_url + +log = logging.getLogger("automation.dispatch") + +BRANCH_PREFIX = "devin/nightly-fix-" +BRANCH_ISSUE = re.compile(r"devin/nightly-fix-(?:issue-)?(\d+)") +BODY_REF = re.compile(r"\b(?:Fixes|Refs|Closes)\s+#(\d+)", re.I) +DEAD_STATUSES = frozenset({"error", "terminated", "cancelled", "canceled"}) +FINISHED_STATUSES = frozenset({"finished", "completed", "blocked", "suspended"}) + + +@dataclass +class Outcome: + issue: int + status: str # dispatched | skipped | failed + detail: str = "" + + +def marker(key: str) -> str: + return f"<!-- devin:{key} -->" + + +def attempted_issues( + prs: Iterable[dict[str, Any]], sessions: Iterable[dict[str, Any]] +) -> set[int]: + """Issue numbers that already have a PR, a fix branch, or a live session.""" + seen: set[int] = set() + for pr in prs: + seen.update(int(n) for n in BODY_REF.findall(pr.get("body") or "")) + if m := BRANCH_ISSUE.search(pr["head"]["ref"]): + seen.add(int(m.group(1))) + for s in sessions: + if str(s.get("status") or s.get("status_enum") or "").lower() in DEAD_STATUSES: Review Comment: <div> <div id="suggestion"> <div id="issue"><b>status logic duplicated</b></div> <div id="fix"> The expression `str(s.get("status") or s.get("status_enum") or "").lower()` is duplicated verbatim at lines 64, 82-83, and 130. A shared `session_status(s)` helper would centralize the status normalization and prevent the three sites from drifting (e.g. one adding a new status key). </div> </div> <small><i>Code Review Run #8b0af2</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## automation/tests/test_orchestrator.py: ########## @@ -0,0 +1,243 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Behavioural tests for the orchestrator against a fake GitHub/Devin API. + +Run with ``python -m pytest automation/tests``. +""" + +from __future__ import annotations + +import json # noqa: TID251 -- standalone tool, not part of the superset package +import threading +from collections.abc import Iterator +from datetime import timedelta +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer +from typing import Any + +import pytest + +from automation.orchestrator import dispatch as ops +from automation.orchestrator.api import ( + AmbiguousWriteError, + Devin, + GitHub, + PermanentError, + RetryPolicy, + TransientError, +) + +FAST = RetryPolicy( + max_attempts=4, initial=0.01, max_sleep=0.05, budget_seconds=5, timeout=1 +) + + +class Fake: + """Scriptable API: ``script[path]`` is a list of (status, body) replies.""" + + def __init__(self) -> None: + self.script: dict[str, list[tuple[int, Any]]] = {} + self.calls: list[tuple[str, str]] = [] + self.comments: list[dict[str, Any]] = [] + self.sessions: list[dict[str, Any]] = [] + + def reply(self, method: str, path: str) -> tuple[int, Any, dict[str, str]]: + self.calls.append((method, path)) + if path in self.script and self.script[path]: + status, body = self.script[path].pop(0) + headers = {"Retry-After": "0"} if status == 429 else {} + return status, body, headers + if path.endswith("/comments") and method == "GET": + return 200, self.comments, {} + if path.endswith("/comments") and method == "POST": + return 500, {"message": "boom"}, {} # landed, but reply lost + if path.startswith("/sessions") and method == "GET": + return 200, {"items": self.sessions, "has_next_page": False}, {} + if path == "/sessions" and method == "POST": + return 500, {"message": "boom"}, {} + return 200, [], {} + + [email protected] +def fake() -> Iterator[tuple[Fake, str]]: + state = Fake() + + class Handler(BaseHTTPRequestHandler): + def _handle(self) -> None: + length = int(self.headers.get("Content-Length") or 0) + payload = json.loads(self.rfile.read(length) or b"{}") if length else {} + path = self.path.split("?")[0] + if path.endswith("/comments") and self.command == "POST": + state.comments.append( + {"id": len(state.comments) + 1, "body": payload["body"]} + ) + if path == "/sessions" and self.command == "POST": + state.sessions.append( + {"session_id": "devin-1", "status": "running", **payload} + ) + status, body, headers = state.reply(self.command, path) + data = json.dumps(body).encode() + self.send_response(status) + self.send_header("Content-Type", "application/json") + for k, v in headers.items(): + self.send_header(k, v) + self.end_headers() + self.wfile.write(data) + + do_GET = do_POST = do_PATCH = do_DELETE = _handle # noqa: N815 + + def log_message(self, *_: Any) -> None: + pass + + server = ThreadingHTTPServer(("127.0.0.1", 0), Handler) + threading.Thread(target=server.serve_forever, daemon=True).start() + try: + yield state, f"http://127.0.0.1:{server.server_port}"; + finally: + server.shutdown() + + +def gh_client(base: str) -> GitHub: + gh = GitHub("o/r", FAST) + gh.base_url = base + return gh + + +def devin_client(base: str) -> Devin: + d = Devin("org", FAST) + d.base_url = base + return d + + +def test_get_retries_429_and_500_then_succeeds(fake: tuple[Fake, str]) -> None: + state, base = fake + state.script["/x"] = [(429, {}), (500, {}), (200, {"ok": True})] + assert gh_client(base).get_json("x") == {"ok": True} + assert [c for c in state.calls if c[1] == "/x"] == [("GET", "/x")] * 3 + + +def test_rate_limited_403_is_transient(fake: tuple[Fake, str]) -> None: + state, base = fake + state.script["/x"] = [(403, {"message": "API rate limit exceeded"}), (200, 1)] + assert gh_client(base).get_json("x") == 1 + + +def test_404_fails_fast(fake: tuple[Fake, str]) -> None: + state, base = fake + state.script["/x"] = [(404, {})] + with pytest.raises(PermanentError): + gh_client(base).get_json("x") + assert len([c for c in state.calls if c[1] == "/x"]) == 1 + + +def test_get_exhausts_after_max_attempts(fake: tuple[Fake, str]) -> None: + state, base = fake + state.script["/x"] = [(500, {})] * 10 + with pytest.raises(TransientError): + gh_client(base).get_json("x") + assert len([c for c in state.calls if c[1] == "/x"]) == FAST.max_attempts + + +def test_post_retries_429_but_not_500(fake: tuple[Fake, str]) -> None: + state, base = fake + state.script["/w"] = [(429, {}), (201, {"id": 1})] + assert gh_client(base).request("POST", "w", json_body={}).json() == {"id": 1} + state.script["/w"] = [(500, {})] + with pytest.raises(AmbiguousWriteError): + gh_client(base).request("POST", "w", json_body={}) + assert len([c for c in state.calls if c == ("POST", "/w")]) == 3 Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Comment POST assumes body key</b></div> <div id="fix"> `Handler._handle` indexes `payload["body"]` for POSTs to `*/comments`. Today the only producer is `GitHub.create_comment` (api.py:287), which always sends `body`; a future client posting without it crashes the handler thread (connection reset) instead of yielding a clean 4xx. `payload.get("body")` is the defensive form. </div> </div> <div id="suggestion"> <div id="issue"><b>Missing docstrings on new tests</b></div> <div id="fix"> The new fixture `fake`, helpers `gh_client`/`devin_client`, and the five tests have no docstrings. BITO.md adaptive rules [12148]/[12490] require docstrings on every new test function, fixture, and helper (type annotations are present; docstrings are not). Add one-liners stating scenario and expected outcome, e.g. on `test_get_retries_429_and_500_then_succeeds`. </div> </div> <small><i>Code Review Run #8b0af2</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## automation/tests/test_orchestrator.py: ########## @@ -0,0 +1,243 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Behavioural tests for the orchestrator against a fake GitHub/Devin API. + +Run with ``python -m pytest automation/tests``. +""" + +from __future__ import annotations + +import json # noqa: TID251 -- standalone tool, not part of the superset package Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Inline noqa vs per-file-ignores</b></div> <div id="fix"> The repo bans stdlib `json` via ruff `flake8-tidy-imports.banned-api` (msg: "Use superset.utils.json instead"), and standalone modules opt out through `per-file-ignores` entries in `pyproject.toml` (`scripts/*`, `setup.py`, `superset/config.py`, `superset/utils/json.py`, etc.) — never via inline `noqa`. Add an `automation/tests/*` per-file-ignores entry for TID251 and drop the inline suppression to match the established pattern. </div> </div> <small><i>Code Review Run #8b0af2</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## automation/scripts/status.sh: ########## @@ -0,0 +1,57 @@ +#!/usr/bin/env bash +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +# Shows whether the automation pipeline is healthy: which nightly-scan issues +# are open, which of them already have a fix PR (branch devin/nightly-fix-<N>-*), +# and, when DEVIN_API_KEY is set, the state of the Devin sessions behind them. +# +# Requires: gh (authenticated via GH_TOKEN or `gh auth login`), jq, curl. +# +set -euo pipefail + +REPO="${REPO:-moliyadhaval/superset}" +ORG_ID="${DEVIN_ORG_ID:-org-eef03b923043402190c037ffa8141840}" + +echo "== Open nightly-scan issues on $REPO" +gh issue list -R "$REPO" --state open --label nightly-scan --limit 300 \ + --json number,title,createdAt > /tmp/issues.json +gh pr list -R "$REPO" --state all --limit 500 --json number,state,headRefName,url > /tmp/prs.json + +jq -r --slurpfile prs /tmp/prs.json ' + ($prs[0] | map(select(.headRefName | test("^devin/nightly-fix-(issue-)?[0-9]+"))) + | map({key: (.headRefName | capture("nightly-fix-(issue-)?(?<n>[0-9]+)").n), value: "#\(.number) (\(.state))"}) + | from_entries) as $byIssue + | sort_by(-.number)[] + | "\(.number)\t\(.createdAt[:10])\t\($byIssue[(.number|tostring)] // "no PR yet")\t\(.title[:70])"' /tmp/issues.json \ + | column -t -s $'\t' + +echo +echo "== Counts" +jq -r 'length as $n | "open issues: \($n)"' /tmp/issues.json +jq -r 'map(select(.headRefName | startswith("devin/"))) | group_by(.state) | map("\(.[0].state): \(length)") | "automation PRs by state: " + join(", ")' /tmp/prs.json + +if [ -n "${DEVIN_API_KEY:-}" ]; then + echo + echo "== Devin auto-fix sessions (latest 100)" + curl -sf -H "Authorization: Bearer ${DEVIN_API_KEY}" \ + "https://api.devin.ai/v3/organizations/${ORG_ID}/sessions?tags=auto-fix&limit=100"; \ + | jq -r '(.items // .sessions // [])[] | "\(.status // .status_enum)\t\(.title)\thttps://app.devin.ai/sessions/\(.session_id | ltrimstr("devin-"))"' \ + | column -t -s $'\t' Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Optional section aborts script</b></div> <div id="fix"> With `set -euo pipefail`, a failed `curl -sf` (network/HTTP error) makes the pipeline fail and `set -e` aborts the script, discarding the issue/PR status already printed. This optional DEVIN_API_KEY section shouldn't kill the required output. Add `|| true` to the pipeline. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion | column -t -s $'\t' || true ```` </div> </details> </div> <small><i>Code Review Run #8b0af2</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## automation/tests/test_orchestrator.py: ########## @@ -0,0 +1,243 @@ +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Behavioural tests for the orchestrator against a fake GitHub/Devin API. + +Run with ``python -m pytest automation/tests``. +""" + +from __future__ import annotations + +import json # noqa: TID251 -- standalone tool, not part of the superset package +import threading +from collections.abc import Iterator +from datetime import timedelta +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer +from typing import Any + +import pytest + +from automation.orchestrator import dispatch as ops +from automation.orchestrator.api import ( + AmbiguousWriteError, + Devin, + GitHub, + PermanentError, + RetryPolicy, + TransientError, +) + +FAST = RetryPolicy( + max_attempts=4, initial=0.01, max_sleep=0.05, budget_seconds=5, timeout=1 +) + + +class Fake: + """Scriptable API: ``script[path]`` is a list of (status, body) replies.""" + + def __init__(self) -> None: + self.script: dict[str, list[tuple[int, Any]]] = {} + self.calls: list[tuple[str, str]] = [] + self.comments: list[dict[str, Any]] = [] + self.sessions: list[dict[str, Any]] = [] + + def reply(self, method: str, path: str) -> tuple[int, Any, dict[str, str]]: + self.calls.append((method, path)) + if path in self.script and self.script[path]: + status, body = self.script[path].pop(0) + headers = {"Retry-After": "0"} if status == 429 else {} + return status, body, headers + if path.endswith("/comments") and method == "GET": + return 200, self.comments, {} + if path.endswith("/comments") and method == "POST": + return 500, {"message": "boom"}, {} # landed, but reply lost + if path.startswith("/sessions") and method == "GET": + return 200, {"items": self.sessions, "has_next_page": False}, {} + if path == "/sessions" and method == "POST": + return 500, {"message": "boom"}, {} + return 200, [], {} + + [email protected] Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing fixture docstring</b></div> <div id="fix"> BITO adaptive rule 12148 requires every new test function and fixture to carry a docstring stating purpose and expected behavior. The `fake` fixture (line 72-73) spins up a `ThreadingHTTPServer` and yields a `(Fake, base_url)` tuple, none of which is self-evident from the name. Add a one-line docstring documenting what it yields and its server lifecycle. </div> </div> <small><i>Code Review Run #8b0af2</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
