From 78104e8934e98f98cc0dba0324736fd817f41fb4 Mon Sep 17 00:00:00 2001 From: Adam Moussa Date: Wed, 24 Jun 2026 17:19:58 -0400 Subject: [PATCH] refactor(agent-team): apply /code-review findings on the App dispatch seams MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - app_run_locator: default a missing status_code to None (fail closed -> raise) rather than 200, so a malformed response object can never be treated as a successful run list. - app_run_locator: drop the unused `_now` parameter (dead/misleading — the floor is derived solely from since_iso) and simplify the redundant two-step `_sleep` indirection to a single resolution. - github_app._parse_expires_at: normalise a naive parsed datetime to aware UTC so an offset-less expires_at cannot raise a bare TypeError in TokenProvider.token (bypassing the fail-closed GitHubAppError contract). - coordinator._app_dispatch_seams: use the module-level Path import instead of a redundant inline one. Follow-ups (left to respect the plan's "leave the gh _default_* seams untouched, additive only"): the floor/skew/poll scaffold is duplicated between app_run_locator and _default_run_locator, and the GitHub REST header dict is rebuilt in several places — both worth a later shared helper. Full suite 1526 passing; ruff clean. --- agent-team/agent_team/coordinator.py | 2 -- agent-team/agent_team/dispatcher.py | 12 ++++++------ agent-team/agent_team/github_app.py | 9 ++++++++- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/agent-team/agent_team/coordinator.py b/agent-team/agent_team/coordinator.py index c85959c..0fe4ba0 100644 --- a/agent-team/agent_team/coordinator.py +++ b/agent-team/agent_team/coordinator.py @@ -477,8 +477,6 @@ def _app_dispatch_seams() -> "tuple[Any, Any, Any] | None": return None # partial -> inert, NEVER raise, NEVER log key try: - from pathlib import Path - pem = Path(key_path).read_text(encoding="utf-8") except Exception: # noqa: BLE001 - unreadable key -> inert, never crash serve _LOG.warning( diff --git a/agent-team/agent_team/dispatcher.py b/agent-team/agent_team/dispatcher.py index 1e1b5ea..aa78b3a 100644 --- a/agent-team/agent_team/dispatcher.py +++ b/agent-team/agent_team/dispatcher.py @@ -747,7 +747,6 @@ def app_run_locator( *, _http: Any = None, _sleep: Any = None, - _now: Any = None, ) -> RunLocator: """App-token :class:`RunLocator`: match the triggered run via the REST API. @@ -767,16 +766,15 @@ def app_run_locator( treating the error body as "no runs" and silently exhausting the poll window. ``_http`` injects a ``requests``-like client (``.get(url, *, params=..., - headers=..., timeout=...)`` -> response exposing ``.json()``); ``_sleep`` / - ``_now`` are injected for tests. No token ever appears in a log or message. + headers=..., timeout=...)`` -> response exposing ``.json()``); ``_sleep`` is + injected for tests. No token ever appears in a log or message. """ - sleep = _sleep if _sleep is not None else None def _locate(*, owner: str, repo: str, task_id: str, since_iso: str) -> str | None: import time from datetime import timedelta - do_sleep = sleep if sleep is not None else time.sleep + do_sleep = _sleep if _sleep is not None else time.sleep try: floor_dt = datetime.strptime(since_iso, "%Y-%m-%dT%H:%M:%SZ").replace( @@ -818,7 +816,9 @@ def app_run_locator( # instead of treating a 401/403/404 error body as "no runs" and # silently exhausting the ~60s poll window. A genuine 200 with no # matching run still falls through to the None-on-no-match path below. - status = getattr(resp, "status_code", 200) + # Default a missing status_code to None (fail closed -> raise), never + # to 200 (which would treat a malformed response as success). + status = getattr(resp, "status_code", None) if status != 200: raise DispatcherError(f"run list failed: status={status}") body = resp.json() diff --git a/agent-team/agent_team/github_app.py b/agent-team/agent_team/github_app.py index 246a5e1..8e042f1 100644 --- a/agent-team/agent_team/github_app.py +++ b/agent-team/agent_team/github_app.py @@ -188,10 +188,17 @@ def _parse_expires_at(expires_at: str) -> datetime: fails closed rather than propagating a bare ``ValueError``. """ try: - return datetime.fromisoformat(expires_at.replace("Z", "+00:00")) + parsed = datetime.fromisoformat(expires_at.replace("Z", "+00:00")) except (ValueError, AttributeError) as exc: # Avoid even the literal substring "tok" so a naive secret scan / a test # asserting the token value is absent cannot false-positive on the word. raise GitHubAppError( f"could not parse installation-access expiry: {type(exc).__name__}" ) from None + # Normalise to aware UTC: a value lacking an offset would parse to a naive + # datetime, and comparing it against the aware ``now`` in TokenProvider.token + # would raise a bare TypeError (bypassing the fail-closed GitHubAppError + # contract). GitHub always sends ``Z``, so this is defensive. + if parsed.tzinfo is None: + parsed = parsed.replace(tzinfo=timezone.utc) + return parsed