From 40622d6f6b387fc1fecf23fa06683ff1c79ab605 Mon Sep 17 00:00:00 2001 From: Dan LaManna Date: Tue, 29 Sep 2026 11:17:12 -0400 Subject: [PATCH 1/3] Include the environment and user in bug reports click pops its context before an unexpected exception propagates out of cli(), so get_current_context() in main always returned None and every bug report listed the env and user as "-". Report unexpected errors from a resource registered on the root click context instead, which runs while ctx.obj is still available. click only passes exception info to context resources as of 8.3.0. --- isic_cli/cli/__init__.py | 36 +++++++++++++++++++++++++++--------- pyproject.toml | 2 +- tests/test_cli_base.py | 38 +++++++++++++++++++++++++++++++++----- uv.lock | 2 +- 4 files changed, 62 insertions(+), 16 deletions(-) diff --git a/isic_cli/cli/__init__.py b/isic_cli/cli/__init__.py index 25ca652..4af0136 100644 --- a/isic_cli/cli/__init__.py +++ b/isic_cli/cli/__init__.py @@ -1,5 +1,6 @@ from __future__ import annotations +from contextlib import contextmanager import datetime from http.client import HTTPConnection import logging @@ -7,10 +8,12 @@ import platform import sys import traceback +from typing import TYPE_CHECKING from authlib.integrations.base_client.errors import OAuthError import click -from click import UsageError, get_current_context +from click import Abort, ClickException, UsageError +from click.exceptions import Exit from requests.exceptions import ConnectionError as RequestsConnectionError from requests.exceptions import HTTPError import sentry_sdk @@ -34,6 +37,9 @@ from isic_cli.session import get_session from isic_cli.utils.version import check_for_newer_version, get_version, is_dev_install +if TYPE_CHECKING: + from collections.abc import Iterator + DOMAINS = { "dev": "http://127.0.0.1:8000", "sandbox": "https://api-sandbox.isic-archive.com", @@ -108,7 +114,9 @@ def _sentry_setup(): @click.option("-v", "--verbose", is_flag=True, help="Enable verbose mode.") @click.version_option() @click.pass_context -def cli(ctx, verbose: bool, guest: bool, sandbox: bool, dev: bool, no_version_check: bool): # noqa: FBT001, C901, PLR0913 +def cli(ctx, verbose: bool, guest: bool, sandbox: bool, dev: bool, no_version_check: bool) -> None: # noqa: FBT001, C901, PLR0913 + ctx.with_resource(_report_unexpected_errors(ctx)) + logger.addHandler(logging.StreamHandler(sys.stderr)) logger.setLevel(logging.WARNING) @@ -179,9 +187,15 @@ def cli(ctx, verbose: bool, guest: bool, sandbox: bool, dev: bool, no_version_ch cli.add_command(user_group, name="user") -def main(): +# registered on the root context so it runs while ctx.obj is still available. click tears +# the context down before an exception could reach main. +@contextmanager +def _report_unexpected_errors(ctx: click.Context) -> Iterator[None]: try: - cli() + yield + # click's standalone mode handles these once the context has closed + except (ClickException, Exit, Abort, EOFError, BrokenPipeError): + raise except RequestsConnectionError as e: click.secho( "Unable to connect to the ISIC Archive. Check your network connection and try again.", @@ -201,15 +215,15 @@ def main(): click.echo(traceback.format_exc(), err=True) - ctx = get_current_context(silent=True) env = "-" user = "-" - if ctx and ctx.obj: - env = ctx.obj["env"] + isic_context: IsicContext | None = ctx.obj + if isic_context: + env = isic_context.env - if ctx.obj.user: - user = ctx.obj.user["id"] + if isic_context.user: + user = isic_context.user["id"] set_tag("platform", platform.system()) set_tag("isic-env", env) @@ -246,3 +260,7 @@ def main(): else: click.secho("Alternatively you can open an issue below: \n", fg="yellow", err=True) click.echo("https://github.com/ImageMarkup/isic-cli/issues/new", err=True) + + +def main(): + cli() diff --git a/pyproject.toml b/pyproject.toml index 5714454..c2fcb4b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -21,7 +21,7 @@ classifiers = [ "Programming Language :: Python :: 3.14", ] dependencies = [ - "click>=8.2.0", + "click>=8.3.0", "django-s3-file-field-client>=1.0.0", # We expect girder-cli-oauth-client to drop oob support in the future "girder-cli-oauth-client<1.0.0", diff --git a/tests/test_cli_base.py b/tests/test_cli_base.py index b1aea84..5fabb30 100644 --- a/tests/test_cli_base.py +++ b/tests/test_cli_base.py @@ -1,5 +1,7 @@ from __future__ import annotations +import sys + from packaging.version import Version import pytest @@ -48,10 +50,10 @@ def test_sentry_error_capture(mocker, send_bug_report, capture_exception_sent): from isic_cli import cli from isic_cli.cli import main - def _exception(): - raise Exception("foo") # noqa: TRY002 - - mocker.patch("isic_cli.cli.cli", side_effect=_exception) + mocker.patch("isic_cli.cli.collection.get_collections", side_effect=RuntimeError("foo")) + mocker.patch.object( + sys, "argv", ["isic", "--guest", "--no-version-check", "collection", "list"] + ) mocker.patch("isic_cli.cli.click.prompt", return_value=send_bug_report) mocker.patch("isic_cli.cli.is_dev_install", return_value=False) @@ -60,13 +62,39 @@ def _exception(): assert spy.call_count == capture_exception_sent +@pytest.mark.usefixtures("_mock_user") +def test_bug_report_describes_env_and_user(mocker, capsys): + from isic_cli.cli import main + + user = {"id": 1, "email": "fakeuser@email.test"} + mocker.patch("isic_cli.cli.get_users_me", return_value=user) + mocker.patch("isic_cli.cli.collection.get_collections", side_effect=RuntimeError("foo")) + mocker.patch("isic_cli.cli.is_dev_install", return_value=True) + mocker.patch.object( + sys, "argv", ["isic", "--sandbox", "--no-version-check", "collection", "list"] + ) + + main() + + err = capsys.readouterr().err + assert "RuntimeError: foo" in err + assert "env: sandbox" in err + assert f"user: {user['id']}" in err + + def test_connection_error_is_not_reported_as_bug(mocker, capsys): from requests.exceptions import SSLError from isic_cli import cli from isic_cli.cli import main - mocker.patch("isic_cli.cli.cli", side_effect=SSLError("EOF occurred in violation of protocol")) + mocker.patch( + "isic_cli.cli.collection.get_collections", + side_effect=SSLError("EOF occurred in violation of protocol"), + ) + mocker.patch.object( + sys, "argv", ["isic", "--guest", "--no-version-check", "collection", "list"] + ) mocker.patch("isic_cli.cli.is_dev_install", return_value=False) prompt = mocker.patch("isic_cli.cli.click.prompt") spy = mocker.spy(cli, "capture_exception") diff --git a/uv.lock b/uv.lock index 0af7255..e89ef38 100644 --- a/uv.lock +++ b/uv.lock @@ -497,7 +497,7 @@ type = [ [package.metadata] requires-dist = [ - { name = "click", specifier = ">=8.2.0" }, + { name = "click", specifier = ">=8.3.0" }, { name = "django-s3-file-field-client", specifier = ">=1.0.0" }, { name = "girder-cli-oauth-client", specifier = "<1.0.0" }, { name = "humanize" }, From 36a43d36531187128197899a67292d10553b7fb0 Mon Sep 17 00:00:00 2001 From: Dan LaManna Date: Tue, 29 Sep 2026 11:17:51 -0400 Subject: [PATCH 2/3] Exit with a non-zero status after unexpected errors The error handler returned normally after printing the traceback, so the process exited 0 whether or not a bug report was sent. Scripts had no way to tell the command failed. --- isic_cli/cli/__init__.py | 4 +++- tests/test_cli_base.py | 9 +++++++-- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/isic_cli/cli/__init__.py b/isic_cli/cli/__init__.py index 4af0136..f235ecb 100644 --- a/isic_cli/cli/__init__.py +++ b/isic_cli/cli/__init__.py @@ -242,7 +242,7 @@ def _report_unexpected_errors(ctx: click.Context) -> Iterator[None]: click.echo(f'command: isic {" ".join(sys.argv[1:])}\n', err=True) if is_dev_install(): - return + sys.exit(1) send_bug_report = click.prompt( click.style( @@ -261,6 +261,8 @@ def _report_unexpected_errors(ctx: click.Context) -> Iterator[None]: click.secho("Alternatively you can open an issue below: \n", fg="yellow", err=True) click.echo("https://github.com/ImageMarkup/isic-cli/issues/new", err=True) + sys.exit(1) + def main(): cli() diff --git a/tests/test_cli_base.py b/tests/test_cli_base.py index 5fabb30..e777881 100644 --- a/tests/test_cli_base.py +++ b/tests/test_cli_base.py @@ -58,7 +58,10 @@ def test_sentry_error_capture(mocker, send_bug_report, capture_exception_sent): mocker.patch("isic_cli.cli.is_dev_install", return_value=False) spy = mocker.spy(cli, "capture_exception") - main() + with pytest.raises(SystemExit) as exc_info: + main() + + assert exc_info.value.code == 1 assert spy.call_count == capture_exception_sent @@ -74,8 +77,10 @@ def test_bug_report_describes_env_and_user(mocker, capsys): sys, "argv", ["isic", "--sandbox", "--no-version-check", "collection", "list"] ) - main() + with pytest.raises(SystemExit) as exc_info: + main() + assert exc_info.value.code == 1 err = capsys.readouterr().err assert "RuntimeError: foo" in err assert "env: sandbox" in err From 41fd85e1cb1645b131501d169a8ae8cb9fce7ca9 Mon Sep 17 00:00:00 2001 From: Dan LaManna Date: Tue, 29 Sep 2026 11:18:51 -0400 Subject: [PATCH 3/3] Only prompt to send a bug report from an interactive terminal When stdin wasn't a terminal (cron, CI, piped input), the prompt either consumed whatever was piped in as the answer or hit EOF and raised Abort from inside the error handler, printing a second traceback. Skip the prompt in that case and point to the issue tracker instead. --- isic_cli/cli/__init__.py | 26 ++++++++++++++++---------- tests/test_cli_base.py | 18 +++++++++++++----- 2 files changed, 29 insertions(+), 15 deletions(-) diff --git a/isic_cli/cli/__init__.py b/isic_cli/cli/__init__.py index f235ecb..9755024 100644 --- a/isic_cli/cli/__init__.py +++ b/isic_cli/cli/__init__.py @@ -244,21 +244,27 @@ def _report_unexpected_errors(ctx: click.Context) -> Iterator[None]: if is_dev_install(): sys.exit(1) - send_bug_report = click.prompt( - click.style( - "This is a bug in isic-cli, would you like to send a bug report?", fg="yellow" - ), - type=click.Choice(choices=["y", "n"]), - default="y", - err=True, - show_choices=True, - ) + # the prompt can't be answered without an interactive terminal (e.g. cron, CI, or piped + # input), so only point to the issue tracker. + send_bug_report = "n" + if sys.stdin.isatty(): + send_bug_report = click.prompt( + click.style( + "This is a bug in isic-cli, would you like to send a bug report?", fg="yellow" + ), + type=click.Choice(choices=["y", "n"]), + default="y", + err=True, + show_choices=True, + ) # this is the only code that actually sends data to sentry, so it's guarded with an opt-in if send_bug_report == "y": capture_exception(e) else: - click.secho("Alternatively you can open an issue below: \n", fg="yellow", err=True) + click.secho( + "You can report this bug by opening an issue below: \n", fg="yellow", err=True + ) click.echo("https://github.com/ImageMarkup/isic-cli/issues/new", err=True) sys.exit(1) diff --git a/tests/test_cli_base.py b/tests/test_cli_base.py index e777881..40b8949 100644 --- a/tests/test_cli_base.py +++ b/tests/test_cli_base.py @@ -39,13 +39,14 @@ def test_new_version( @pytest.mark.parametrize( - ("send_bug_report", "capture_exception_sent"), + ("interactive", "send_bug_report", "capture_exception_sent"), [ - ("y", 1), - ("n", 0), + (True, "y", 1), + (True, "n", 0), + (False, None, 0), ], ) -def test_sentry_error_capture(mocker, send_bug_report, capture_exception_sent): +def test_sentry_error_capture(mocker, capsys, interactive, send_bug_report, capture_exception_sent): # Note: _sentry_setup is always mocked from isic_cli import cli from isic_cli.cli import main @@ -54,15 +55,22 @@ def test_sentry_error_capture(mocker, send_bug_report, capture_exception_sent): mocker.patch.object( sys, "argv", ["isic", "--guest", "--no-version-check", "collection", "list"] ) - mocker.patch("isic_cli.cli.click.prompt", return_value=send_bug_report) + prompt = mocker.patch("isic_cli.cli.click.prompt", return_value=send_bug_report) mocker.patch("isic_cli.cli.is_dev_install", return_value=False) + stdin = mocker.patch.object(sys, "stdin") + stdin.isatty.return_value = interactive spy = mocker.spy(cli, "capture_exception") with pytest.raises(SystemExit) as exc_info: main() assert exc_info.value.code == 1 + assert prompt.called == interactive assert spy.call_count == capture_exception_sent + issue_link_shown = ( + "https://github.com/ImageMarkup/isic-cli/issues/new" in capsys.readouterr().err + ) + assert issue_link_shown == (capture_exception_sent == 0) @pytest.mark.usefixtures("_mock_user")