refactor(agent-team): apply /code-review findings on the App dispatch seams
- 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.
This commit is contained in:
parent
183789a239
commit
78104e8934
3 changed files with 14 additions and 9 deletions
|
|
@ -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(
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Reference in a new issue