Improve GitHub authentication errors (#24502)

* Improve GitHub authentication errors

* Add ddev authentication error changelog

* Separate exception handler initialization

* Use public MRO API for exception handlers

* Centralize GitHub authentication error handling

* Preserve GitHub rate limit and recovery handling

* Route GitHub auth failures through central handling

* Preserve GitHub auth compatibility and recovery context

* Use public CLI exception helper name

Author: AAraKKe

ddev/changelog.d/24502.fixed

@@ -0,0 +1 @@
+Show actionable guidance when GitHub rejects ddev's configured token.

ddev/src/ddev/cli/__init__.py

@@ -9,7 +9,7 @@
 
 from ddev._version import __version__
 from ddev.cli import upgrade_check
-from ddev.cli.application import Application
+from ddev.cli.application import Application, DdevGroup
 from ddev.cli.ci import ci
 from ddev.cli.clean import clean
 from ddev.cli.config import config
@@ -27,9 +27,14 @@
 from ddev.plugin import specs
 from ddev.utils.ci import running_in_ci
 from ddev.utils.fs import Path
+from ddev.utils.github_errors import GitHubAuthenticationError
 
 
-@click.group(context_settings={'help_option_names': ['-h', '--help']}, invoke_without_command=True)
+def display_registered_exception(app: Application, error: Exception) -> None:
+    app.display_error(str(error))
+
+
+@click.group(cls=DdevGroup, context_settings={'help_option_names': ['-h', '--help']}, invoke_without_command=True)
 @click.option('--core', '-c', is_flag=True, help='Work on `integrations-core`.')
 @click.option('--extras', '-e', is_flag=True, help='Work on `integrations-extras`.')
 @click.option('--marketplace', '-m', is_flag=True, help='Work on `marketplace`.')
@@ -93,6 +98,7 @@ def ddev(
         interactive = not running_in_ci()
 
     app = Application(ctx.exit, verbose - quiet, color, interactive)
+    app.register_exception_handler(GitHubAuthenticationError, display_registered_exception)
 
     if config_file:
         app.config_file.path = Path(config_file).resolve()

ddev/src/ddev/cli/application.py

@@ -6,10 +6,12 @@
 import logging
 import os
 from collections import defaultdict
-from collections.abc import Iterable
+from collections.abc import Callable, Iterable
 from functools import cached_property
 from typing import TYPE_CHECKING, cast
 
+import click
+
 from ddev.cli.terminal import Terminal
 from ddev.config.constants import AppEnvVars, ConfigEnvVars, VerbosityLevels
 from ddev.config.file import ConfigFileWithOverrides, RootConfig
@@ -20,7 +22,23 @@
 from ddev.utils.platform import Platform
 
 if TYPE_CHECKING:
-    from typing import Any, Callable, NoReturn
+    from typing import Any, NoReturn
+
+
+type ExceptionHandler[E: Exception] = Callable[[Application, E], None]
+
+
+class DdevGroup(click.Group):
+    """Root command group that renders registered exceptions through the application."""
+
+    def invoke(self, ctx: click.Context) -> Any:
+        try:
+            return super().invoke(ctx)
+        except Exception as error:
+            app = ctx.obj
+            if isinstance(app, Application) and app.handle_exception(error):
+                ctx.exit(1)
+            raise
 
 
 class AppLoggingHandler(logging.Handler):
@@ -57,6 +75,8 @@ def __init__(self, exit_func: Callable[[int], NoReturn], *args, **kwargs):
         # TODO: remove this when the old CLI is gone
         self.__config: dict[str, Any] = {}
 
+        self.__exception_handlers: dict[type[Exception], ExceptionHandler[Exception]] = {}
+
     @property
     def config(self) -> RootConfig:
         return self.config_file.combined_model
@@ -116,6 +136,22 @@ def abort(self, text: str = '', code: int = 1, **kwargs: Any) -> NoReturn:
             self.display_error(text, **kwargs)
         self.__exit_func(code)
 
+    def register_exception_handler[E: Exception](
+        self,
+        exception_type: type[E],
+        handler: ExceptionHandler[E],
+    ) -> None:
+        """Register an internal CLI handler for an exception type."""
+        self.__exception_handlers[exception_type] = cast(ExceptionHandler[Exception], handler)
+
+    def handle_exception(self, error: Exception) -> bool:
+        """Render an exception with its nearest registered type."""
+        for exception_type in type(error).mro():
+            if handler := self.__exception_handlers.get(exception_type):
+                handler(self, error)
+                return True
+        return False
+
     def annotate_error(self, file: str, message: str, line: int = 1) -> None:
         """Emit a GitHub Actions ``error`` workflow annotation; no-op outside CI."""
         self._emit_github_annotation(AnnotationLevel.ERROR, file, message, line)

ddev/src/ddev/cli/release/branch/create.py

@@ -12,6 +12,8 @@
 from httpx import HTTPError, HTTPStatusError
 from packaging.version import Version
 
+from ddev.utils.github_errors import GitHubAuthenticationError
+
 from .build_agent import BUILD_AGENT_YAML_PATH, ensure_build_agent_yaml_updated
 
 if TYPE_CHECKING:
@@ -137,6 +139,11 @@ def bump_milestone(app: Application, branch_name: str) -> None:
             f'after cutting the `{branch_name}` release branch.',
         )
         app.display_success(f'Pull request created: {pr_url}')
+    except GitHubAuthenticationError:
+        app.display_warning(
+            f'Failed to create the pull request. Please create one manually from `{bump_branch}` to `master`.'
+        )
+        raise
     except HTTPError as e:
         app.display_warning(
             f'Failed to create the pull request ({e}). Please create one manually from `{bump_branch}` to `master`.'

ddev/src/ddev/cli/release/branch/tag.py

@@ -9,6 +9,7 @@
 from packaging.version import Version
 
 from ddev.utils.git import GitRepository
+from ddev.utils.github_errors import GitHubAuthenticationError
 
 from .build_agent import BUILD_AGENT_YAML_PATH, find_build_agent_template_main_branch_matches
 from .create import BRANCH_NAME_REGEX
@@ -328,6 +329,8 @@ def _check_open_prs(app: Application, target_branch: str, skip_open_pr_check: bo
     httpx_logger.setLevel(logging.WARNING)
     try:
         prs = app.github.list_open_pull_requests_targeting_base(target_branch)
+    except GitHubAuthenticationError:
+        raise
     except Exception as e:
         click.secho(f'Warning: unable to check for open PRs: {e}', fg='yellow')
         return []
@@ -366,6 +369,12 @@ def _trigger_build_agent_yaml_update_workflow(app: Application, branch_name: str
             UPDATE_BUILD_AGENT_YAML_WORKFLOW_REF,
             {'branch': branch_name},
         )
+    except GitHubAuthenticationError:
+        app.display_warning(
+            f'The tag was pushed, but `{UPDATE_BUILD_AGENT_YAML_WORKFLOW}` could not be triggered.\n'
+            f'To trigger it manually: gh workflow run {UPDATE_BUILD_AGENT_YAML_WORKFLOW} -f branch={branch_name}'
+        )
+        raise
     except HTTPStatusError as e:
         app.display_warning(
             f'Warning: unable to trigger `{UPDATE_BUILD_AGENT_YAML_WORKFLOW}`: {e}\n'

ddev/src/ddev/cli/release/port_commit_workflow.py

@@ -323,8 +323,16 @@ def execute(self) -> None:
         import httpx
         from pydantic import ValidationError
 
+        from ddev.utils.github_errors import GitHubAuthenticationError
+
         try:
             asyncio.run(self._create_pr())
+        except GitHubAuthenticationError:
+            if self.pr_url:
+                self.app.display_warning(
+                    f'Pull request created at {self.pr_url} but labeling failed. Add the labels manually on the PR.'
+                )
+            raise
         except (httpx.HTTPError, ValidationError) as e:
             if self.pr_url:
                 raise PortStepError(
@@ -410,6 +418,8 @@ def _resolve_pr_to_commit(app: Application, pr_number: int, *, dry_run: bool) ->
     import httpx
     from pydantic import ValidationError
 
+    from ddev.utils.github_errors import GitHubAuthenticationError
+
     if not app.config.github.token:
         app.abort(
             'GitHub token required to resolve a PR reference. Set `github.token`, or pass the '
@@ -420,15 +430,12 @@ def _resolve_pr_to_commit(app: Application, pr_number: int, *, dry_run: bool) ->
     app.display_info(f'Resolving PR #{pr_number} via GitHub...')
     try:
         pr = asyncio.run(_fetch_pr(app.config.github.token, owner, repo, pr_number))
+    except GitHubAuthenticationError:
+        raise
     except httpx.HTTPStatusError as exc:
         status = exc.response.status_code
         if status == 404:
             raise _PRNotFound(str(pr_number)) from exc
-        if status in (401, 403):
-            app.abort(
-                f'GitHub denied the request for PR #{pr_number} (HTTP {status}). '
-                'Check that `github.token` is set and has `repo` scope.'
-            )
         app.abort(f'Failed to fetch PR #{pr_number} from GitHub: {exc}.')
     except (httpx.HTTPError, ValidationError) as exc:
         app.abort(f'Failed to fetch PR #{pr_number} from GitHub: {exc}.')

ddev/src/ddev/cli/release/test_agent/monitoring.py

@@ -20,6 +20,7 @@
 
 from ddev.cli.release.test_agent.validation import REPO_NAME, REPO_OWNER
 from ddev.utils.github_async import async_github_client
+from ddev.utils.github_errors import GitHubAuthenticationError
 
 if TYPE_CHECKING:
     from ddev.cli.release.test_agent.dispatch import DispatchedWorkflow
@@ -161,6 +162,8 @@ async def poll() -> MonitorState:
         nonlocal state
         try:
             state = await collect_monitor_state(client, workflows)
+        except GitHubAuthenticationError:
+            raise
         except httpx.HTTPError:
             pass
         return state

ddev/src/ddev/utils/github.py

@@ -8,6 +8,15 @@
 from time import time
 from typing import TYPE_CHECKING, overload
 
+from ddev.utils.github_errors import (
+    GITHUB_AUTHENTICATION_STATUS_CODES,
+    GitHubAuthenticationError,
+    github_secondary_rate_limit_wait,
+)
+
+MAX_SECONDARY_RATE_LIMIT_RETRIES = 2
+MAX_SECONDARY_RATE_LIMIT_WAIT_SECONDS = 3600
+
 if TYPE_CHECKING:
     from typing import Any, Literal
 
@@ -199,6 +208,8 @@ def get_changed_files_by_commit_sha(self, sha: str) -> list[str] | None:
 
         try:
             response = self.__api_get(self.COMMIT_API.format(repo_id=self.repo_id, sha=sha))
+        except GitHubAuthenticationError:
+            raise
         except HTTPStatusError:
             return None
         return [file_data['filename'] for file_data in response.json().get('files', [])]
@@ -215,6 +226,8 @@ def get_pull_request_labels(self, pr_number: int) -> list[str] | None:
 
         try:
             response = self.__api_get(self.PULL_REQUEST_API.format(repo_id=self.repo_id, pr_number=pr_number))
+        except GitHubAuthenticationError:
+            raise
         except HTTPStatusError:
             return None
         return [label['name'] for label in response.json().get('labels', [])]
@@ -305,16 +318,29 @@ def __api_get(self, *args, **kwargs):
         return self.__api_call('get', *args, **kwargs)
 
     def __api_call(self, method, *args, **kwargs):
-        from httpx import HTTPError
+        from httpx import HTTPError, HTTPStatusError
 
         retry_wait = 2
+        secondary_rate_limit_retries = 0
         while True:
             try:
                 response = getattr(self.client, method)(*args, auth=self.__auth, **kwargs)
 
+                secondary_rate_limit_wait = github_secondary_rate_limit_wait(response)
+                if secondary_rate_limit_wait is not None:
+                    if (
+                        secondary_rate_limit_retries < MAX_SECONDARY_RATE_LIMIT_RETRIES
+                        and secondary_rate_limit_wait <= MAX_SECONDARY_RATE_LIMIT_WAIT_SECONDS
+                    ):
+                        secondary_rate_limit_retries += 1
+                        self.__status.wait_for(
+                            secondary_rate_limit_wait + 1,
+                            context='GitHub API secondary rate limit reached',
+                        )
+                        continue
                 # https://docs.github.com/en/rest/overview/resources-in-the-rest-api?apiVersion=2022-11-28#rate-limiting
                 # https://docs.github.com/en/rest/guides/best-practices-for-integrators?apiVersion=2022-11-28#dealing-with-rate-limits
-                if response.status_code == 403 and response.headers['X-RateLimit-Remaining'] == '0':  # noqa: PLR2004
+                elif response.status_code == 403 and response.headers.get('X-RateLimit-Remaining') == '0':  # noqa: PLR2004
                     self.__status.wait_for(
                         float(response.headers['X-RateLimit-Reset']) - time() + 1,
                         context='GitHub API rate limit reached',
@@ -325,5 +351,12 @@ def __api_call(self, method, *args, **kwargs):
                 retry_wait *= 2
                 continue
 
-            response.raise_for_status()
+            try:
+                response.raise_for_status()
+            except HTTPStatusError as e:
+                if github_secondary_rate_limit_wait(e.response) is not None:
+                    raise
+                if e.response.status_code in GITHUB_AUTHENTICATION_STATUS_CODES:
+                    raise GitHubAuthenticationError.from_http_status_error(e) from e
+                raise
             return response

ddev/src/ddev/utils/github_async/client.py

@@ -10,13 +10,18 @@
 import zipfile
 from collections.abc import AsyncIterator, Callable
 from contextlib import asynccontextmanager, suppress
-from dataclasses import dataclass
+from dataclasses import dataclass, replace
 from pathlib import Path
 from typing import Any, Literal, Self, overload
 
 import httpx
 from pydantic import BaseModel, ConfigDict, Field
 
+from ddev.utils.github_errors import (
+    GITHUB_AUTHENTICATION_STATUS_CODES,
+    GitHubAuthenticationError,
+    github_secondary_rate_limit_wait,
+)
 from ddev.utils.rate_limiting import NULL_SNAPSHOT, BudgetSnapshot, InstrumentedAsyncLimiter
 
 from .defaults import default_github_rate_limiter
@@ -180,12 +185,15 @@ def _is_rate_limit_response(response: httpx.Response) -> bool:
         """Whether *response* is a retryable rate-limit rejection, by GitHub's own discrimination rule.
 
         A 403 is also used for plain permission denials, which waiting cannot fix; retrying one would
-        sleep out a pause (up to a full window) and then fail identically. Only header-confirmed
-        rate-limit responses are retryable.
+        sleep out a pause (up to a full window) and then fail identically. Only responses confirmed
+        by rate-limit headers or GitHub's secondary-limit message are retryable.
         """
         if response.status_code not in (403, 429):
             return False
-        return "retry-after" in response.headers or response.headers.get("x-ratelimit-remaining") == "0"
+        return (
+            github_secondary_rate_limit_wait(response) is not None
+            or response.headers.get("x-ratelimit-remaining") == "0"
+        )
 
     async def _execute_request(
         self,
@@ -203,6 +211,9 @@ async def _execute_request(
         # exception, so one request's 403 protects every other in-flight and future request in this
         # process.
         snapshot = github_rate_limit_snapshot(response.headers)
+        secondary_rate_limit_wait = github_secondary_rate_limit_wait(response)
+        if secondary_rate_limit_wait is not None:
+            snapshot = replace(snapshot or NULL_SNAPSHOT, retry_after=secondary_rate_limit_wait)
         if snapshot is not None:
             self._rate_limiter.observe(snapshot)
         response.raise_for_status()
@@ -236,8 +247,14 @@ async def _request(
                     # the action. (Transport errors are never retried, and are not caught here: after
                     # one we cannot know whether the action executed.) Give up on the last attempt or
                     # on a non-rate-limit status, which waiting cannot fix.
-                    if attempt == self._max_rate_limit_retries or not self._is_rate_limit_response(exc.response):
-                        raise
+                    is_rate_limit_response = self._is_rate_limit_response(exc.response)
+                    if is_rate_limit_response:
+                        if attempt == self._max_rate_limit_retries:
+                            raise
+                        continue
+                    if exc.response.status_code in GITHUB_AUTHENTICATION_STATUS_CODES:
+                        raise GitHubAuthenticationError.from_http_status_error(exc) from exc
+                    raise
         raise RuntimeError("unreachable: the retry loop always returns or raises")  # pragma: no cover
 
     async def _paginated_request(
@@ -711,6 +728,8 @@ async def _resolve_artifact_redirect(
             redirect_response = await self._request(
                 "GET", archive_download_url, timeout=timeout, follow_redirects=False
             )
+        except GitHubAuthenticationError:
+            raise
         except httpx.HTTPStatusError as exc:
             # httpx.raise_for_status() treats the expected 302 as an error since it isn't a 2xx;
             # recover the response from the exception so the redirect can still be inspected below.