mirror of
https://github.com/basicmachines-co/basic-memory
synced 2026-06-21 13:47:35 +00:00
fix(cli): handle non-subscription errors in cloud login
`bm cloud login` only caught SubscriptionRequiredError after the post-login `/proxy/health` subscription check. OAuth succeeds and tokens are saved, but if that check returns anything else — a 5xx while the tenant instance is still provisioning, a 403/401 whose body doesn't match the subscription_required shape, or a transport error — make_api_request raises a generic CloudAPIError that escaped uncaught, dumping a raw httpx.raise_for_status traceback. Users read this as "login failed" even though authentication actually worked. Add a CloudAPIError handler that prints a clean, actionable message and exits non-zero. make_api_request wraps every httpx error (status and transport) in CloudAPIError, so the single handler covers them all. Fixes #863. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: phernandez <paul@basicmachines.co>
This commit is contained in:
committed by
Paul Hernandez
parent
597b12fcd1
commit
8d21a38588
@@ -71,6 +71,29 @@ def login():
|
||||
)
|
||||
raise typer.Exit(1)
|
||||
|
||||
# Trigger: the subscription-check call (/proxy/health) returned any error
|
||||
# that is NOT a recognized subscription_required 403 — e.g. a 5xx while the
|
||||
# tenant instance is still provisioning, a 403/401 whose body doesn't match
|
||||
# the subscription_required shape, or a connection failure.
|
||||
# Why: OAuth already succeeded and tokens are saved at this point, so a raw
|
||||
# traceback (the old behavior) misleads users into thinking login itself
|
||||
# failed. See #863.
|
||||
# Outcome: surface a clean, actionable message and exit non-zero instead of
|
||||
# crashing. make_api_request wraps every httpx error (status + transport)
|
||||
# in CloudAPIError, so this single handler covers them all.
|
||||
except CloudAPIError as e:
|
||||
console.print("\n[yellow]Authenticated, but couldn't verify cloud access.[/yellow]\n")
|
||||
console.print(f"[dim]{e}[/dim]\n")
|
||||
console.print(
|
||||
"Your workspace may still be provisioning. Wait a moment, then check with "
|
||||
"[bold]bm cloud status[/bold] or retry [bold]bm cloud login[/bold].\n"
|
||||
)
|
||||
console.print(
|
||||
"[dim]If this persists, contact support at "
|
||||
"[blue underline]https://basicmemory.com[/blue underline].[/dim]"
|
||||
)
|
||||
raise typer.Exit(1)
|
||||
|
||||
run_with_cleanup(_login())
|
||||
|
||||
|
||||
|
||||
@@ -184,6 +184,43 @@ class TestLoginCommand:
|
||||
assert "Cloud authentication successful" in result.stdout
|
||||
assert "Cloud host ready: https://cloud.example.com" in result.stdout
|
||||
|
||||
def test_login_health_check_error_shows_clean_message(self, monkeypatch):
|
||||
"""Regression for #863: a non-subscription error from the post-login
|
||||
/proxy/health check must produce a clean message, not a raw traceback.
|
||||
|
||||
OAuth has already succeeded at this point; the tenant instance may still
|
||||
be provisioning (5xx) or return some other non-subscription_required error.
|
||||
"""
|
||||
runner = CliRunner()
|
||||
|
||||
monkeypatch.setattr(
|
||||
"basic_memory.cli.commands.cloud.core_commands.CLIAuth",
|
||||
lambda **_kwargs: _StubAuth(login_ok=True),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
"basic_memory.cli.commands.cloud.core_commands.get_cloud_config",
|
||||
lambda: ("client_id", "domain", "https://cloud.example.com"),
|
||||
)
|
||||
|
||||
async def fake_make_api_request(*_args, **_kwargs):
|
||||
# e.g. tenant instance not ready yet -> proxy returns 503
|
||||
raise CloudAPIError("API request failed: 503 Service Unavailable", status_code=503)
|
||||
|
||||
monkeypatch.setattr(
|
||||
"basic_memory.cli.commands.cloud.core_commands.make_api_request",
|
||||
fake_make_api_request,
|
||||
)
|
||||
|
||||
result = runner.invoke(app, ["cloud", "login"])
|
||||
# Clean exit, no traceback leaking the exception class.
|
||||
assert result.exit_code == 1
|
||||
assert result.exception is None or isinstance(result.exception, SystemExit)
|
||||
# Collapse Rich's line-wrapping before matching multi-word phrases.
|
||||
output = " ".join(result.stdout.split())
|
||||
assert "couldn't verify cloud access" in output
|
||||
assert "bm cloud status" in output
|
||||
assert "Traceback" not in output
|
||||
|
||||
def test_login_authentication_failure(self, monkeypatch):
|
||||
runner = CliRunner()
|
||||
|
||||
|
||||
Reference in New Issue
Block a user