Skip to content

fix(errors): name the failing API from one place - #38

Open
varjoranta wants to merge 3 commits into
mainfrom
fix/org-resolution-error-context
Open

fix(errors): name the failing API from one place#38
varjoranta wants to merge 3 commits into
mainfrom
fix/org-resolution-error-context

Conversation

@varjoranta

@varjoranta varjoranta commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Summary

The original fix improved one error message by hand-rolling a string that named the failing API. That left the other 123 HTTP error sites on the shared handle_http_error formatter, so the CLI would have grown a second error format. This moves the idea into the shared formatter instead.

  • handle_http_error now names the API when the host isn't the public default. The host is configurable through AVR_HOST or hosts.json, so against a self-hosted or staging endpoint a bare status never said which API answered. On the default host it stays quiet, where the URL is noise on every error.
  • Organization resolution now goes through the shared formatter. It is usually the first authenticated request a command makes, so it is where a stale token surfaces, and it was the one place that reported a raw HTTP 401 instead of the avr auth login hint and exit code 4 that scripts check for.
  • Repository resolution no longer renders httpx's own exception string for non-404 failures. That string is two lines long and ends in an MDN link.
  • The login flow keeps its own handling, because a 401 there means the browser session expired rather than "go run avr auth login". Its two handlers now name the host, matching what the rest of that file already did.

Behaviour

Default host, unchanged:

Error: Avrea is having trouble (HTTP 500). Try again shortly — `avr health` shows status.
  Detail: backend unavailable

Custom host:

Error: Avrea is having trouble (HTTP 500). Try again shortly — `avr health` shows status.
  Detail: backend unavailable
  API: https://avrea.internal.example.com/users/me/organizations

Expired token during organization resolution, previously exit 1 with a raw status:

Error: https://avrea.internal.example.com rejected your credentials (HTTP 401).
To get started with Avrea CLI, please run:  avr auth login

Exit code is now 4 at both migrated sites, matching every other API call.

Tests

Pin both halves of the host rule (named when custom, silent when default), the 401 exit code at both migrated call sites, and the absence of httpx internals in repository errors.

Verification

  • uv run --extra dev python -m pytest tests -q (798 passed)
  • make lint
  • git diff --check
  • Manual: every status against both a default and a custom host

@coderabbitai

coderabbitai Bot commented Aug 18, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your current included review allowance is based on your included PR review attempts over the past 7 days.

Next review available in: 18 minutes

Limit details: You’ve used all 2 included reviews currently available. Your 59 included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour.

You can run this review on demand instead of waiting.

On-demand reviews are free until September 18, 2026. After that, they cost $0.25 per reviewed file.

  • Run review for free
How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: caac3e6d-a056-4a03-bd11-a5b1e707139f

📥 Commits

Reviewing files that changed from the base of the PR and between 2278f99 and 64e6807.

📒 Files selected for processing (6)
  • avrea_cli/auth.py
  • avrea_cli/helpers.py
  • avrea_cli/repo_context.py
  • tests/test_auth_login.py
  • tests/test_helpers.py
  • tests/test_repo_context.py

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b85c6b18-ab2d-4422-9023-79216f588921

📥 Commits

Reviewing files that changed from the base of the PR and between 7e12a63 and 2278f99.

📒 Files selected for processing (2)
  • avrea_cli/helpers.py
  • tests/test_helpers.py

Included review availability: 1 review is currently available. Based on recent review activity, included reviews refill at 2 per hour.


📝 Walkthrough

Walkthrough

Organization lookup HTTP failures now raise click.ClickException messages that include the API URL and status code. A test verifies this behavior for an HTTP 500 response.

Changes

Organization lookup error reporting

Layer / File(s) Summary
HTTP error handling and validation
avrea_cli/helpers.py, tests/test_helpers.py
_fetch_user_orgs raises click.ClickException with the failing endpoint and status code. The test verifies the message for an HTTP 500 response.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 2278f

This localized change improves organization-discovery error messages and adds focused test coverage; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: leolannenmaki

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: identifying the failing API in organization discovery errors.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/org-resolution-error-context

Comment @coderabbitai help to get the list of available commands.

PR #38 improved the org-resolution error by hand-rolling a message that
named the failing API. That fixed one site and left the other 123 on the
shared formatter, so the CLI grew a second error format.

Move the idea into handle_http_error instead, and route the outliers
through it:

- handle_http_error names the API when the host isn't the public default.
  The host is configurable via AVR_HOST or hosts.json, so on a self-hosted
  or staging endpoint a bare status never said which API answered. On the
  default host it stays quiet, where the URL is noise on every error.
- _fetch_user_orgs and get_verified_org_slug now use the shared formatter.
  Org resolution is usually the first authenticated request a command
  makes, so it is where a stale token surfaces: it was the one place that
  reported a raw 401 instead of the auth hint and exit 4 that scripts
  check for.
- Repo resolution no longer renders httpx's own exception string (a
  two-line message ending in an MDN link) for non-404 failures.
- The login flow keeps its own handling, since a 401 there means the
  browser session expired rather than "go run avr auth login", but its
  two handlers now name the host like the rest of that file already did.

Tests pin both halves of the host rule, the 401 exit code at both
migrated sites, and the absence of httpx internals in repo errors.
@varjoranta varjoranta changed the title fix(org): identify API in resolution errors fix(errors): name the failing API from one place Aug 19, 2026
Mutation testing the previous commit found three changes that kept the
suite green when reverted, so they were not actually pinned:

- The 401 branch names the API in a sentence of its own, but the only 401
  test used the default host, where that sentence is suppressed. Deleting
  the line changed nothing observable. Now covered from both sides:
  named on a custom host, absent on the default one.
- get_verified_org_slug's move onto the shared formatter was never
  executed. Its existing tests cover a transport error and a non-JSON
  200, neither of which is an HTTPStatusError.
- The login flow's split between status and transport errors was never
  executed. Added a fixture that drives login() to the API-key exchange
  with no browser and no socket, following the pattern already used in
  test_auth_sso.

All twelve mutants are now caught, including full reverts of each
migrated call site.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant