diff --git a/CHANGELOG.md b/CHANGELOG.md index 03f87991..e54a469d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [Unreleased] +### Fixed + +- `python -m cloudsmith_cli` now exits non-zero when a command fails. `AliasGroup.main` runs click with `standalone_mode=False` so click returns the exit code from `ctx.exit()` rather than raising `SystemExit`, and the module entrypoint discarded that return value — so a failed push, or an unauthorised request, exited 0. The `cloudsmith` console script and the standalone binaries already wrapped `main()` in `sys.exit()` and were unaffected. +- The hint shown for a 401 when a credential is set no longer claims the cause is a missing permission. A 401 does not tell the CLI whether the credential is invalid, expired, or simply has no access to the resource, so the hint now names those possibilities and asks the user to check their credentials, instead of contradicting the `401 - Unauthorized` status it accompanies. + ## [1.22.0] - 2026-08-11 ### Added diff --git a/cloudsmith_cli/__main__.py b/cloudsmith_cli/__main__.py index a1eca464..ba087115 100644 --- a/cloudsmith_cli/__main__.py +++ b/cloudsmith_cli/__main__.py @@ -1,8 +1,15 @@ """Cloudsmith CLI - Main script.""" +import sys + from .cli.commands.main import main if __name__ == "__main__": + # sys.exit() is required: AliasGroup.main runs click with + # standalone_mode=False, so click returns the exit code (e.g. from + # ctx.exit()) instead of raising SystemExit. The console script and the + # PyInstaller entry point wrap main() in sys.exit() too; a bare main() + # call would discard the code and always exit 0. # Disable false positive for parameters handled by click. # pylint: disable=no-value-for-parameter - main() + sys.exit(main()) diff --git a/cloudsmith_cli/cli/exceptions.py b/cloudsmith_cli/cli/exceptions.py index ae7a72c8..7fe1b51e 100644 --- a/cloudsmith_cli/cli/exceptions.py +++ b/cloudsmith_cli/cli/exceptions.py @@ -171,8 +171,9 @@ def get_401_error_hint(ctx, opts, exc): if credential: return ( - "Since you have an API key set, this probably means " - "you don't have the permission to perform this action." + "This usually means your API key is invalid, expired, or " + "lacks access to this resource - check your credentials and " + "try again." ) if ctx.info_name == "token": diff --git a/cloudsmith_cli/cli/tests/test_entrypoint_exit_codes.py b/cloudsmith_cli/cli/tests/test_entrypoint_exit_codes.py new file mode 100644 index 00000000..20d409f5 --- /dev/null +++ b/cloudsmith_cli/cli/tests/test_entrypoint_exit_codes.py @@ -0,0 +1,73 @@ +"""Tests that every shipped entrypoint reports API failures as an exit code. + +``AliasGroup.main`` runs click with ``standalone_mode=False`` so it can render +click's own errors as JSON, which means click *returns* the code passed to +``ctx.exit()`` instead of raising ``SystemExit``. Every entrypoint therefore has +to wrap ``main()`` in ``sys.exit()``; one that calls it bare exits 0 on a failed +command, and a caller (CI, a script) sees a push or an auth failure as success. +""" + +import pathlib +import runpy +import sys +from unittest.mock import patch + +import pytest + +from cloudsmith_cli.cli.commands.main import main +from cloudsmith_cli.core.api.exceptions import ApiException + +REPO_ROOT = pathlib.Path(__file__).resolve().parents[3] + +# The entrypoints that are not importable as modules: the console script is +# generated by the installer from [project.scripts], and the PyInstaller entry +# only runs under __main__ in the frozen bundle. +UNIMPORTABLE_ENTRYPOINTS = [ + REPO_ROOT / "packaging" / "pyinstaller" / "entry.py", +] + + +def unauthorised_args(config_dir): + """Return args for a whoami that will raise a 401, isolated from real config.""" + return [ + "whoami", + "--config-file", + str(config_dir), + "--credentials-file", + str(config_dir), + "--api-host", + "https://api.example.invalid", + "--api-key", + "fake-api-key", + ] + + +def unauthorised_api(): + """Patch the whoami API call to raise a 401.""" + return patch( + "cloudsmith_cli.cli.commands.whoami.get_user_brief", + side_effect=ApiException(status=401, detail="Invalid API key"), + ) + + +def test_main_returns_the_status_as_an_exit_code(tmp_path): + """The value entrypoints must pass to sys.exit() - not a raised SystemExit.""" + with unauthorised_api(): + assert main(unauthorised_args(tmp_path), standalone_mode=True) == 401 + + +def test_python_m_exits_non_zero(tmp_path, monkeypatch): + monkeypatch.setattr(sys, "argv", ["cloudsmith", *unauthorised_args(tmp_path)]) + + with unauthorised_api(), pytest.raises(SystemExit) as exc_info: + runpy.run_module("cloudsmith_cli", run_name="__main__") + + assert exc_info.value.code == 401 + + +@pytest.mark.parametrize("path", UNIMPORTABLE_ENTRYPOINTS, ids=lambda p: p.name) +def test_unimportable_entrypoint_wraps_main_in_sys_exit(path): + if not path.is_file(): + pytest.skip(f"{path} is not present (not shipped in the distribution)") + + assert "sys.exit(main())" in path.read_text() diff --git a/cloudsmith_cli/cli/tests/test_exceptions.py b/cloudsmith_cli/cli/tests/test_exceptions.py index c9ddce13..588e479a 100644 --- a/cloudsmith_cli/cli/tests/test_exceptions.py +++ b/cloudsmith_cli/cli/tests/test_exceptions.py @@ -1,11 +1,20 @@ """Tests for CLI error hints.""" -from unittest.mock import Mock +import json +from unittest.mock import Mock, patch +from cloudsmith_cli.cli.commands.main import main from cloudsmith_cli.cli.exceptions import get_401_error_hint +from cloudsmith_cli.core.api.exceptions import ApiException from cloudsmith_cli.core.credentials.models import CredentialResult +API_KEY_HINT = ( + "This usually means your API key is invalid, expired, or lacks access to this " + "resource - check your credentials and try again." +) + + def hint_for(credential, info_name="push"): """Return the 401 hint for a session holding the given credential. @@ -30,13 +39,57 @@ def test_bearer_credential_suggests_reauthenticating(self): assert "cloudsmith auth" in hint_for(credential) - def test_api_key_credential_suggests_a_permissions_problem(self): + def test_api_key_credential_does_not_assert_a_permissions_problem(self): + """A 401 alone cannot establish a specific permissions problem.""" credential = CredentialResult(api_key="csa_abc123", source_name="oidc") - assert "permission" in hint_for(credential) + assert hint_for(credential) == API_KEY_HINT def test_no_credential_suggests_authenticating(self): assert "cloudsmith token" in hint_for(None) def test_no_credential_on_token_command_reports_a_failed_login(self): assert "login failed" in hint_for(None, info_name="token") + + +def invoke_credentialed_401(runner, config_dir, output_format="pretty"): + """Raise a translated 401 through the registered command tree. + + The config and credentials paths point at an empty directory so a real + config.ini or credentials.ini on the machine cannot change the hint. + """ + args = [ + "whoami", + "--config-file", + str(config_dir), + "--credentials-file", + str(config_dir), + "--api-host", + "https://api.example.invalid", + "--api-key", + "fake-api-key", + "--output-format", + output_format, + ] + with patch( + "cloudsmith_cli.cli.commands.whoami.get_user_brief", + side_effect=ApiException(status=401, detail="Invalid API key"), + ): + return runner.invoke(main, args) + + +class TestCredentialed401Rendering: + """The hint has to survive the renderers, not just the hint function.""" + + def test_text_output_renders_the_hint(self, runner, tmp_path): + result = invoke_credentialed_401(runner, tmp_path) + + assert "status: 401 - Unauthorized" in result.output + assert f"Hint: {API_KEY_HINT}" in result.output + + def test_json_output_renders_the_hint(self, runner, tmp_path): + result = invoke_credentialed_401(runner, tmp_path, output_format="json") + + error = json.loads(result.stdout) + assert error["meta"] == {"code": 401, "description": "Unauthorized"} + assert error["help"]["hint"] == API_KEY_HINT