Repository navigation
fix: add actionable hints and doc links to Atmos Pro API errors - #2264
Merged
Merged
Conversation
Wrap all Pro API errors in the error builder pattern so users get status-specific guidance instead of opaque HTTP status codes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
March 27, 2026 14:08
Dependency Review✅ No vulnerabilities or license issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
Erik Osterman (Cloud Posse) (osterman)
added
the
patch
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughReworked Atmos Pro API client error handling to replace string-wrapped errors with structured, enriched errors using Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/pro/api_client_test.go`:
- Around line 567-640: Add a regression row to
TestBuildProAPIError_HintsPerStatusCode that simulates the real bug: call
buildProAPIError with a non-zero statusCode (e.g., http.StatusUnauthorized or
http.StatusForbidden) but pass an apiResponse whose Status field is zero
(omitted), then assert the produced error still contains the expected
status-specific hints; locate the test table in pkg/pro/api_client_test.go and
the buildProAPIError helper to add the case (e.g., name "401 with missing
response status" with statusCode http.StatusUnauthorized and apiResponse.Status
left at 0) and include the same expectedHints as the normal 401/403/404/500
rows.
In `@pkg/pro/api_client.go`:
- Around line 302-315: The LockStackResponse/UnlockStackResponse handling
currently only calls buildProAPIError after a successful JSON decode and thus
returns raw decode errors for non-JSON (HTML/plain) responses; update the switch
in the function handling params.Out to mirror handleAPIResponse: after calling
logProAPIResponse(params.Op, responseData.AtmosApiResponse), if JSON decoding
failed or response body wasn't JSON then check resp.StatusCode (e.g.,
401/403/404/5xx) and return a user-facing error that includes the
troubleshooting/guidance text (same guidance used by handleAPIResponse) combined
with detailed debug info (wrap in params.WrapErr and include buildProAPIError
when appropriate) so non-JSON failures on LockStack/UnlockStack produce the same
helpful messages as other API paths. Ensure you reference params.Op,
params.WrapErr, resp.StatusCode, logProAPIResponse, buildProAPIError,
dtos.LockStackResponse and dtos.UnlockStackResponse when implementing.
- Around line 414-449: buildProAPIError currently lets the human-facing/fallback
message come from apiResponse.Status which can be 0 and hide the transport
status; update buildProAPIError so the transport statusCode is the canonical
source for status: if apiResponse.Status == 0 set apiResponse.Status =
statusCode (or pass statusCode into logAndReturnProAPIError) before calling
logAndReturnProAPIError, and keep using the statusCode variable for hint
selection (function buildProAPIError and helper logAndReturnProAPIError are the
relevant symbols to change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fabec5df-cfca-4fb9-961c-05f585a76007
📒 Files selected for processing (2)
pkg/pro/api_client.gopkg/pro/api_client_test.go
- Show ui.Success/ui.Error messages when --upload completes or fails - Return upload errors instead of silently swallowing them - Merge 5xx hint fragments into one self-contained hint with doc link - Remove duplicate quickstart link from 404 hints Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2264 +/- ##
==========================================
+ Coverage 77.23% 77.27% +0.04%
==========================================
Files 1020 1020
Lines 96481 96530 +49
==========================================
+ Hits 74515 74597 +82
+ Misses 17766 17734 -32
+ Partials 4200 4199 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
…hints Address CodeRabbit review findings where error hints could be bypassed: - Normalize statusCode in buildProAPIError so hints work even when JSON body omits the status field (status=0) - Pass resp.StatusCode from doStackLockAction and exchangeOIDCTokenForAtmosToken instead of the response body status - Add enriched error handling for non-JSON responses in lock/unlock and OIDC token exchange paths (matching handleAPIResponse pattern) - Add regression tests for status=0 in response body Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/pro/api_client.go`:
- Around line 423-468: buildProAPIError currently unconditionally adds a 5xx
hint that says "will be retried automatically", which is inaccurate for
operations that are not wrapped by doWithRetry (e.g., LockStack/UnlockStack via
doStackLockAction and ExchangeOIDCToken) while others (UploadAffectedStacks,
UploadInstances, UploadInstanceStatus) do retry; fix by making the hint
conditional: add a boolean parameter (e.g., isRetried) to buildProAPIError or
derive retryability via an explicit operation-name whitelist, update all callers
(including doStackLockAction, ExchangeOIDCToken, UploadAffectedStacks,
UploadInstances, UploadInstanceStatus) to pass the correct value, and only
append the "will be retried automatically" 5xx hint when isRetried is true
(otherwise use a neutral server-error hint).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4753229f-b4a0-48d8-888d-004186a462c5
📒 Files selected for processing (3)
internal/exec/describe_affected.gopkg/pro/api_client.gopkg/pro/api_client_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/pro/api_client_test.go
- Remove "will be retried automatically" from 5xx error hint — not all callers use doWithRetry (lock/unlock, OIDC exchange don't retry) - Add TestHandleAPIResponse_NonJSONErrorResponse: covers non-JSON body with error status returning enriched error with troubleshooting link - Add TestHandleAPIResponse_NonJSONSuccessResponse: covers non-JSON body with success status returning nil - Add TestHandleAPIResponse_SuccessHTTPStatusRange: covers 201 Created trusting HTTP status over missing Success field - Add TestBuildProAPIError_WithTraceID: covers trace_id context in hints Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Andriy Knysh (aknysh)
approved these changes
Mar 28, 2026
|
These changes were released in v1.214.0-rc.1. |
This was referenced May 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
what
ui.Success/ui.Errormessages when--uploadcompletes or fails (previously silent on success, swallowed on failure)fmt.Errorferror wrapping witherrors.Join+buildProAPIErroracross all Pro API paths (uploads, lock/unlock, OIDC exchange)why
API request failed with status 403). The most common cause is per-repo permissions not being configured.log.Warnonly) — now they surface as proper errors with hintsreferences
atmos describe affected --uploadwith no actionable guidanceSummary by CodeRabbit
Bug Fixes
New Features
Tests