From 3c8587fa231eac13864fd9fd2182bc07b5cc9989 Mon Sep 17 00:00:00 2001 From: Bartosz Blizniak Date: Wed, 12 Aug 2026 23:38:12 +0100 Subject: [PATCH 1/4] fix(no-ticket): stop 401 hint from asserting a permissions problem A 401 means authentication failed, not that authorization was denied, so the hint for a credentialed 401 shouldn't claim it's specifically a permissions issue -- that contradicts the "Unauthorized" status line it's paired with and sends users chasing the wrong kind of bug (issue #77). Reworded to point at the credential itself (invalid, expired, or lacking access) without asserting which of those it is. --- cloudsmith_cli/cli/exceptions.py | 5 +++-- cloudsmith_cli/cli/tests/test_exceptions.py | 11 +++++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) 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_exceptions.py b/cloudsmith_cli/cli/tests/test_exceptions.py index c9ddce13..4961085a 100644 --- a/cloudsmith_cli/cli/tests/test_exceptions.py +++ b/cloudsmith_cli/cli/tests/test_exceptions.py @@ -30,10 +30,17 @@ 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 means authentication failed, not that authorization was + denied -- the hint shouldn't claim it's specifically a permissions + issue, since that contradicts the 401 status line it's paired with. + """ credential = CredentialResult(api_key="csa_abc123", source_name="oidc") - assert "permission" in hint_for(credential) + hint = hint_for(credential) + + assert "permission" not in hint + assert "credentials" in hint def test_no_credential_suggests_authenticating(self): assert "cloudsmith token" in hint_for(None) From b14e0ac521b8ba041c51dd0fbe38cea5a348b2d5 Mon Sep 17 00:00:00 2001 From: Bartosz Blizniak Date: Thu, 13 Aug 2026 09:20:17 +0100 Subject: [PATCH 2/4] test(no-ticket): cover credentialed 401 hint rendering --- cloudsmith_cli/cli/tests/test_exceptions.py | 57 ++++++++++++++++++--- 1 file changed, 50 insertions(+), 7 deletions(-) diff --git a/cloudsmith_cli/cli/tests/test_exceptions.py b/cloudsmith_cli/cli/tests/test_exceptions.py index 4961085a..b9d5cb8f 100644 --- a/cloudsmith_cli/cli/tests/test_exceptions.py +++ b/cloudsmith_cli/cli/tests/test_exceptions.py @@ -1,11 +1,22 @@ """Tests for CLI error hints.""" -from unittest.mock import Mock +import json +from unittest.mock import Mock, patch +import click.testing + +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. @@ -31,19 +42,51 @@ def test_bearer_credential_suggests_reauthenticating(self): assert "cloudsmith auth" in hint_for(credential) def test_api_key_credential_does_not_assert_a_permissions_problem(self): - """A 401 means authentication failed, not that authorization was - denied -- the hint shouldn't claim it's specifically a permissions - issue, since that contradicts the 401 status line it's paired with. - """ + """A 401 alone cannot establish a specific permissions problem.""" credential = CredentialResult(api_key="csa_abc123", source_name="oidc") hint = hint_for(credential) - assert "permission" not in hint - assert "credentials" in hint + assert hint == API_KEY_HINT + assert "permission" not in hint.lower() 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(output_format="pretty"): + """Raise a translated 401 through the registered command tree.""" + args = [ + "whoami", + "--config-file", + "/dev/null", + "--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 click.testing.CliRunner().invoke(main, args) + + +def test_credentialed_401_renders_actionable_hint_in_text_output(): + result = invoke_credentialed_401() + + assert "status: 401 - Unauthorized" in result.output + assert f"Hint: {API_KEY_HINT}" in result.output + + +def test_credentialed_401_renders_actionable_hint_in_json_output(): + result = invoke_credentialed_401("json") + + error = json.loads(result.stdout) + assert error["meta"] == {"code": 401, "description": "Unauthorized"} + assert error["help"]["hint"] == API_KEY_HINT From 194bcc71101246cac585031ab4db1f9483da03f2 Mon Sep 17 00:00:00 2001 From: Bartosz Blizniak Date: Thu, 13 Aug 2026 16:31:06 +0100 Subject: [PATCH 3/4] test(no-ticket): isolate 401 hint tests from on-disk config The command-tree tests pinned --config-file at /dev/null, which is POSIX-only and left credentials.ini discovery untouched, so a real credentials file on the machine could feed the run. Both paths now point at pytest's tmp_path, and the tests use the shared runner fixture and class layout that cli/tests/commands/test_whoami.py already uses. CHANGELOG gains the Unreleased entry the wording change should have carried. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 5 +++ cloudsmith_cli/cli/tests/test_exceptions.py | 41 +++++++++++---------- 2 files changed, 27 insertions(+), 19 deletions(-) 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/cli/tests/test_exceptions.py b/cloudsmith_cli/cli/tests/test_exceptions.py index b9d5cb8f..588e479a 100644 --- a/cloudsmith_cli/cli/tests/test_exceptions.py +++ b/cloudsmith_cli/cli/tests/test_exceptions.py @@ -3,8 +3,6 @@ import json from unittest.mock import Mock, patch -import click.testing - 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 @@ -45,10 +43,7 @@ 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") - hint = hint_for(credential) - - assert hint == API_KEY_HINT - assert "permission" not in hint.lower() + assert hint_for(credential) == API_KEY_HINT def test_no_credential_suggests_authenticating(self): assert "cloudsmith token" in hint_for(None) @@ -57,12 +52,18 @@ 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(output_format="pretty"): - """Raise a translated 401 through the registered command tree.""" +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", - "/dev/null", + str(config_dir), + "--credentials-file", + str(config_dir), "--api-host", "https://api.example.invalid", "--api-key", @@ -74,19 +75,21 @@ def invoke_credentialed_401(output_format="pretty"): "cloudsmith_cli.cli.commands.whoami.get_user_brief", side_effect=ApiException(status=401, detail="Invalid API key"), ): - return click.testing.CliRunner().invoke(main, args) + return runner.invoke(main, args) -def test_credentialed_401_renders_actionable_hint_in_text_output(): - result = invoke_credentialed_401() +class TestCredentialed401Rendering: + """The hint has to survive the renderers, not just the hint function.""" - assert "status: 401 - Unauthorized" in result.output - assert f"Hint: {API_KEY_HINT}" in result.output + 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_credentialed_401_renders_actionable_hint_in_json_output(): - result = invoke_credentialed_401("json") + 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 + error = json.loads(result.stdout) + assert error["meta"] == {"code": 401, "description": "Unauthorized"} + assert error["help"]["hint"] == API_KEY_HINT From 6256e20fe39e46485a7968132e2982b8f8d5d3cc Mon Sep 17 00:00:00 2001 From: Bartosz Blizniak Date: Thu, 13 Aug 2026 16:31:15 +0100 Subject: [PATCH 4/4] fix(no-ticket): make `python -m cloudsmith_cli` exit non-zero on failure 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. The module entrypoint called main() bare and discarded that return value, so a failed push or an unauthorised request exited 0 -- CI and scripts saw a failure as success. The console script and the PyInstaller entry point already wrapped main() in sys.exit(); the module now does too. Verified end to end against a local server returning 401: both `python -m cloudsmith_cli whoami` and the console script exit 145 (401 & 0xFF). No HTTP status the CLI can raise truncates to 0. Co-Authored-By: Claude Opus 5 --- cloudsmith_cli/__main__.py | 9 ++- .../cli/tests/test_entrypoint_exit_codes.py | 73 +++++++++++++++++++ 2 files changed, 81 insertions(+), 1 deletion(-) create mode 100644 cloudsmith_cli/cli/tests/test_entrypoint_exit_codes.py 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/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()