fix(pro): friendlier 4xx error rendering and propagate hints to users#2386
Merged
Andriy Knysh (aknysh) merged 5 commits intomainfrom May 6, 2026
Merged
fix(pro): friendlier 4xx error rendering and propagate hints to users#2386Andriy Knysh (aknysh) merged 5 commits intomainfrom
Andriy Knysh (aknysh) merged 5 commits intomainfrom
Conversation
Erik Osterman (CEO @ Cloud Posse) (osterman)
requested a review
from a team
as a code owner
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Erik Osterman (CEO @ Cloud Posse) (osterman)
added
the
patch
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConverted Pro client multi-error joins to a sentinel-aware wrapper ChangesPro API client error & DTO overhaul
sequenceDiagram
participant Client as Atmos CLI
participant OIDC as GitHub OIDC
participant Pro as Atmos Pro API
participant HTTP as net/http Client
Client->>OIDC: getGitHubOIDCToken()
OIDC-->>Client: OIDC token / error
Client->>HTTP: exchangeOIDCTokenForAtmosToken (POST)
HTTP-->>Pro: request
Pro-->>HTTP: response (200/400/5xx)
HTTP-->>Client: response body
Client->>Client: handleAPIResponse -> buildProAPIError / renderValidationErrors
alt retryable (5xx)
Client->>Client: doWithRetry / token refresh flows
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 4
🤖 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_commit.go`:
- Around line 61-64: The early return after calling client.Do(req) can leak
connections because resp may be non-nil when doErr is set; update the Do error
path in the block where resp, doErr := client.Do(req) is handled so that if resp
!= nil you close resp.Body before returning (e.g., call resp.Body.Close() or
ensure a defer is set immediately after verifying resp) and then return
wrapErr(errUtils.ErrFailedToMakeRequest, doErr); reference the resp, doErr
variables and the wrapErr/errUtils.ErrFailedToMakeRequest usage to locate the
change.
In `@pkg/pro/api_client_instances.go`:
- Around line 93-96: The request path calls client.Do(req) and returns on error
but doesn't close a non-nil resp body, leaking connections; update the error
branch around client.Do(req) so that if doErr != nil and resp != nil you fully
drain and close resp.Body (e.g., io.Copy(io.Discard, resp.Body);
resp.Body.Close()) before returning wrapErr(errUtils.ErrFailedToMakeRequest,
doErr), and keep the existing defer resp.Body.Close() for the successful path;
ensure io is imported if using io.Discard.
In `@pkg/pro/api_client_test.go`:
- Around line 764-766: The comment lines in the test
TestBuildProAPIError_400DriftDetection contain multi-line doc comments that do
not end with periods; update those comment blocks (and the other mentioned
blocks at ranges 793-794, 806-808, 841-842, 869-870, 913-914 in the same file)
so each sentence/line ends with a period and reflow wrapped lines as needed to
follow the repo rule; locate the comments adjacent to the
TestBuildProAPIError_400DriftDetection function name and edit the comment text
to add trailing periods and adjust line breaks so every line ends cleanly with a
period.
In `@pkg/pro/api_client.go`:
- Around line 106-112: Deduplicate the entries in data.validationErrors before
building the bullet list: create a seen map[string]struct{} and iterate the
existing validationErrors slice, skipping any value already in seen and adding
new values to seen (or alternatively build a unique slice preserving
first-occurrence order), then use that deduplicated slice in the block that
writes to the strings.Builder (the code around the variables validationErrors
and b). Replace the current direct loop over validationErrors with a loop over
the deduplicated collection so duplicate bullets are not emitted.
🪄 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: b20590fb-cffa-4d52-bc56-1ddaa0c2033e
📒 Files selected for processing (13)
pkg/pro/api_client.gopkg/pro/api_client_commit.gopkg/pro/api_client_exchange_oidc_token_test.gopkg/pro/api_client_instance_status.gopkg/pro/api_client_instances.gopkg/pro/api_client_instances_test.gopkg/pro/api_client_test.gopkg/pro/api_error.gopkg/pro/api_error_test.gopkg/pro/dtos/atmos_api_response.gopkg/pro/dtos/atmos_api_response_test.gopkg/pro/retry.gopkg/pro/wrap.go
![atmos-pro[bot]](https://avatars.githubusercontent.com/in/927493?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2386 +/- ##
==========================================
+ Coverage 78.11% 78.15% +0.04%
==========================================
Files 1092 1094 +2
Lines 103308 103377 +69
==========================================
+ Hits 80697 80793 +96
+ Misses 18183 18152 -31
- Partials 4428 4432 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/pro/api_client.go (1)
320-323:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClose non-nil response bodies on the remaining
Doerror paths.These branches still return early without closing
resp.Bodywhenhttp.Client.Do(...)yields both a response and an error. That is the same leak pattern you already fixed in the commit and instances paths.Suggested fix pattern.
resp, err := client.Do(req) if err != nil { + if resp != nil && resp.Body != nil { + _ = resp.Body.Close() + } return wrapErr(errUtils.ErrFailedToMakeRequest, err) }In Go's net/http package, can http.Client.Do return both a non-nil *http.Response and a non-nil error, and should callers close Response.Body in that case?Also applies to: 349-352, 601-605, 664-667
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/pro/api_client.go` around lines 320 - 323, The early returns after calling c.HTTPClient.Do(req) can leak when Do returns both a non-nil resp and a non-nil error; update each site that calls c.HTTPClient.Do(req) so that on doErr != nil you check if resp != nil, drain (io.Copy to io.Discard) and close resp.Body before returning wrapErr(errUtils.ErrFailedToMakeRequest, doErr), and add an import for io if needed; apply the same change to the other occurrences of c.HTTPClient.Do(req) in this file so every error path closes resp.Body.
🤖 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 99-120: The code trims and dedupes validationErrors only when
emitting the list, but containsAllValidationErrors(headlineTail,
validationErrors) is called on the raw slice so blanks and whitespace-only
duplicates can cause the inline ": A" tail to remain and duplicate entries to
render; normalize the slice first (trim each entry, skip empty strings, dedupe
into a new []string or map) and use that normalized slice both when calling
containsAllValidationErrors and when iterating to build the output (replace the
current loop and seen map logic), ensuring headline mutation and the builder
loop operate off the same cleaned set.
- Around line 80-84: The trace_id suffix is being appended to errorMsg after
calling renderValidationErrors, which makes it appear as part of the last
validation bullet; update the logic in the function that builds the error
message (the code using renderValidationErrors, errorMsg, validationErrors and
traceID) so trace_id is either attached before calling renderValidationErrors or
appended as its own separate line (e.g., add a "\n" or separate bullet) rather
than directly concatenating to errorMsg after renderValidationErrors returns;
ensure you modify the code paths that reference traceID and
renderValidationErrors to preserve existing formatting and keep trace_id
visually separate from the validation bullets.
---
Duplicate comments:
In `@pkg/pro/api_client.go`:
- Around line 320-323: The early returns after calling c.HTTPClient.Do(req) can
leak when Do returns both a non-nil resp and a non-nil error; update each site
that calls c.HTTPClient.Do(req) so that on doErr != nil you check if resp !=
nil, drain (io.Copy to io.Discard) and close resp.Body before returning
wrapErr(errUtils.ErrFailedToMakeRequest, doErr), and add an import for io if
needed; apply the same change to the other occurrences of c.HTTPClient.Do(req)
in this file so every error path closes resp.Body.
🪄 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: 4853b58a-1031-404c-abdf-c05dc1006e8c
📒 Files selected for processing (4)
pkg/pro/api_client.gopkg/pro/api_client_commit.gopkg/pro/api_client_instances.gopkg/pro/api_client_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/pro/api_client_test.go
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/pro/wrap_test.go (1)
16-43: ⚡ Quick winPlease consolidate these three scenarios into a table-driven test.
Nice coverage here, but this block is testing one behavior family with three variants. Converting to table-driven form will align with repo test conventions and reduce duplication.
As per coding guidelines "Use table-driven tests for testing multiple scenarios in Go."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/pro/wrap_test.go` around lines 16 - 43, Replace the three separate tests with a single table-driven test (e.g., TestWrapErr_TableDriven) that defines cases with fields: name, sentinel, cause, expectError, expectSame, expectIsSentinel, expectIsCause, expectHintsContains; iterate cases with t.Run and for each call wrapErr(sentinel, cause), assert require.Error(t, got) when expectError is true, use assert.Same(t, ...) only when expectSame is true (for the nil sentinel case), use assert.True(t, errors.Is(got, errUtils.ErrFailedToMakeRequest)) and assert.True(t, errors.Is(got, cause)) according to expectIsSentinel/expectIsCause, and check cockroachErrors.GetAllHints(got) contains the expected hint when expectHintsContains is set (for the WithHint case); keep references to wrapErr, errUtils.ErrFailedToMakeRequest, cockroachErrors.WithHint and cockroachErrors.GetAllHints to locate code.pkg/pro/api_client_commit_test.go (1)
163-186: ⚡ Quick win
TestSendCommitRequest_RedirectErrorClosesBodycurrently validates error wrapping, not body-close behavior.If the
resp.Body.Close()guard regresses, this test would still pass. Please either assert the close side effect (preferred) or rename the test to match what it verifies today.As per coding guidelines, "
**/*_test.go: Test behavior, not implementation; never test stub functions; avoid tautological tests."
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/pro/wrap_test.go`:
- Around line 16-43: Replace the three separate tests with a single table-driven
test (e.g., TestWrapErr_TableDriven) that defines cases with fields: name,
sentinel, cause, expectError, expectSame, expectIsSentinel, expectIsCause,
expectHintsContains; iterate cases with t.Run and for each call
wrapErr(sentinel, cause), assert require.Error(t, got) when expectError is true,
use assert.Same(t, ...) only when expectSame is true (for the nil sentinel
case), use assert.True(t, errors.Is(got, errUtils.ErrFailedToMakeRequest)) and
assert.True(t, errors.Is(got, cause)) according to
expectIsSentinel/expectIsCause, and check cockroachErrors.GetAllHints(got)
contains the expected hint when expectHintsContains is set (for the WithHint
case); keep references to wrapErr, errUtils.ErrFailedToMakeRequest,
cockroachErrors.WithHint and cockroachErrors.GetAllHints to locate code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5def6968-7731-4bd2-be7d-5991a88581a2
📒 Files selected for processing (4)
pkg/pro/api_client.gopkg/pro/api_client_commit_test.gopkg/pro/api_client_instances_test.gopkg/pro/wrap_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/pro/api_client_instances_test.go
- pkg/pro/api_client.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/pro/retry.go (1)
82-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the 401 refresh-failure path on
wrapErr(...)too.This block now uses
wrapErr(...)for retry exhaustion, but the earlierrefreshErrbranch still returnserrors.Join(err, refreshErr). That means a failed token refresh can fall back to the old multi-error shape and lose the hint/cause annotations this PR is trying to preserve in CLI output. Please route that branch through the same wrapper path as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/pro/retry.go` around lines 82 - 97, The refreshErr branch currently returns errors.Join(err, refreshErr) which strips the wrapper annotations; change that branch to return the same wrapped error form as the retry-exhausted path by calling wrapErr with the refresh sentinel as the wrapper and the original err as the cause (i.e. replace errors.Join(err, refreshErr) with wrapErr(refreshErr, err) in the block that checks refreshErr), keeping the existing logging and other behavior around attempt, cfg.maxRetries, refreshErr, and err.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/pro/api_client.go`:
- Around line 101-123: The headline truncation logic should only run when there
are non-empty normalized validation errors; update the conditional around the
tail-stripping block that uses headline, normalized, and
containsAllValidationErrors so it first checks len(normalized) > 0, and also
consider using strings.LastIndex(headline, ": ") (instead of strings.Index) to
locate the final colon so earlier colon-delimited context in headline is not
lost. Modify the code that computes idx and the if that calls
containsAllValidationErrors(tail, normalized) accordingly (variables/functions:
headline, normalized, containsAllValidationErrors, strings.Index/LastIndex).
---
Outside diff comments:
In `@pkg/pro/retry.go`:
- Around line 82-97: The refreshErr branch currently returns errors.Join(err,
refreshErr) which strips the wrapper annotations; change that branch to return
the same wrapped error form as the retry-exhausted path by calling wrapErr with
the refresh sentinel as the wrapper and the original err as the cause (i.e.
replace errors.Join(err, refreshErr) with wrapErr(refreshErr, err) in the block
that checks refreshErr), keeping the existing logging and other behavior around
attempt, cfg.maxRetries, refreshErr, and err.
🪄 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: acb2e036-9987-4d33-8045-73f8b1f068af
📒 Files selected for processing (3)
pkg/pro/api_client.gopkg/pro/api_client_instances.gopkg/pro/retry.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/pro/api_client_instances.go
Improve developer experience when the Atmos Pro API returns a 4xx by
surfacing the server's structured `errorMessage`, `errorTag`, and
`data.validationErrors[]` in a way users can actually act on.
- DTO: tolerate both `errorMessage` and legacy `error`; capture
`errorTag` and `data.validationErrors`; add `EffectiveErrorMessage()`.
- Render `data.validationErrors` as a bullet list under the headline
(with dedupe of trailing concatenations).
- Add a 400 hint pointing at `settings.pro.*` configuration plus a
drift-detection-specific hint when the server's `errorTag` or
message identifies that subsystem.
- Drop the redundant "HTTP <code>:" prefix in `APIError.Error()` for
4xx responses — the server's message is already user-facing, so
output reads `UploadInstances: <message>` instead of
`UploadInstances: HTTP 400: API response error: <message>`.
- Preserve `trace_id` on all statuses (including 4xx) so support can
still correlate user-reported issues server-side.
- Replace `errors.Join(sentinel, cause)` with a small `wrapErr` helper
using the existing `errUtils.Build(...).WithCause(...)` pattern.
`errors.Join` (and `fmt.Errorf("%w: %w", ...)`) produce multi-errors
that hide cockroach hint annotations from `GetAllHints`, so users
never saw the lightbulb hints. `wrapErr` re-attaches hints on the
outer layer so the CLI renderer surfaces them.
- Retry behavior unchanged: 4xx still non-retryable, 401 still
refreshes the OIDC token, 5xx still backs off.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address CodeRabbit review feedback: collapse duplicate or whitespace-only validation errors into a single bullet, and close any non-nil response body returned alongside an error from http.Client.Do (e.g., redirect failures) in the commit and instances upload paths. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address CodeRabbit review on the prior commit: - Put trace_id on its own line so it can't be misread as the last validation bullet when both are rendered. - Normalize the validation-error slice once (trim, drop empties, dedupe) and reuse it for both the headline tail-match and the bullet rendering, so inputs like ["A", " A "] strip the inline tail consistently. - Add direct tests for wrapErr and the defensive resp.Body close path triggered by CheckRedirect failures, lifting the patch coverage above the 80% target. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
CI's pre-commit go-fumpt hook (gofumpt v0.10.0) reformats long log.Error/log.Warn/log.Debug calls to put the format string on its own line. Apply the same formatting locally so the hook passes. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Erik Osterman (CEO @ Cloud Posse) (osterman)
force-pushed
the
osterman/pro-error-msgs
branch
from
92f57ae to
27c6b24
Compare
Address CodeRabbit feedback on renderValidationErrors: - Guard the tail-stripping path with len(normalized) > 0 so a server reply containing only blank/whitespace validationErrors no longer truncates the headline despite rendering no bullets. - Switch the colon search from strings.Index to strings.LastIndex so earlier colon-delimited context (e.g. "Component vpc: validation failed: A; B") is preserved when stripping the duplicated tail. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Andriy Knysh (aknysh)
approved these changes
May 6, 2026
Contributor
|
These changes were released in v1.218.0-rc.1. |
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
errorMessage,errorTag,data.validationErrors[]) directly to the CLI user instead of opaqueHTTP 500: API response error: API request failed with status Nnoise.data.validationErrors[]as a bullet list under the headline (with dedupe of trailing server-side concatenations like"failed: A; B").settings.pro.*docs and a drift-detection-specific hint (selected byerrorTag == DriftDetectionValidationErroror substring match on the message).HTTP <code>:prefix inAPIError.Error()for 4xx so output readsUploadInstances: <server message>instead ofUploadInstances: HTTP 400: API response error: <server message>.trace_idon all statuses (including 4xx) so support can correlate user-reported issues.errorMessage(current) and legacyerrorfield on responses; newEffectiveErrorMessage()prefers the former.errors.Join(sentinel, cause)with a smallwrapErrhelper using the existingerrUtils.Build(...).WithCause(...)pattern. Stdliberrors.Joinandfmt.Errorf("%w: %w", ...)both produce multi-errors that hide cockroach hint annotations fromGetAllHints, which is why the existing 401/403/404/5xx hints never actually rendered to users.why
atmos list instances --uploadagainst a misconfigured stack producedUploadInstances: HTTP 500: API response error: API request failed with status 500 (trace_id: ...)four times in a row. The server actually returned a clean validation message describing the missing drift-detection workflows, but the CLI couldn't surface it (DTO field-name mismatch + lost hints).errorMessageanddata.validationErrors[]for user-error conditions; this PR is the CLI side of that contract.errUtils.Build(...).WithHint(...)were silently dropped at the outermost wrap layer becauseerrors.Joindoesn't expose its children tocockroachErrors.GetAllHints. Users never saw the lightbulb hints the 401/403/404/5xx code paths were already emitting; this PR makes them visible.references
N/A
Summary by CodeRabbit
New Features
Bug Fixes
Tests