Surface typed CLIErrors from auth.Login for known failures#985
Surface typed CLIErrors from auth.Login for known failures#985nimeshk03 wants to merge 3 commits into
Conversation
|
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)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughClassifies OAuth token-exchange and PKCE cancellation errors into typed clierr.CLIError values and surfaces those typed errors through the login command rendering instead of wrapping all failures as Transport. ChangesOAuth Error Classification and Command Rendering
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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 |
| var ce clierr.CLIError | ||
| if errors.As(err, &ce) { | ||
| return render.Error(opts.IO, scope, ce) | ||
| } |
There was a problem hiding this comment.
render.Error already does this. asCLIError (render.go:107-120) and textError (render.go:86-89) both run errors.As(err, &cliErr) — typed CLIErrors pass through with their code, everything else falls back to Transport. This block plus the new errors import collapses to return render.Error(opts.IO, scope, err) with identical behavior.
There was a problem hiding this comment.
Addressed in the latest commit. Removed the explicit CLIError handling from runLogin and delegated error rendering directly to render.Error(...).
| if ce := classifyTokenError(err); ce != nil { | ||
| return nil, ce | ||
| } | ||
| return nil, fmt.Errorf("client_credentials token exchange: %w", err) |
There was a problem hiding this comment.
The classify-then-fmt.Errorf pattern is duplicated in loginPKCE (line 160). Could fold into one wrapTokenError(err, ctx) helper so each site becomes a single return nil, wrapTokenError(err, "client_credentials").
There was a problem hiding this comment.
Addressed in the latest commit. Extracted the classification-and-wrapping logic into wrapTokenError() and updated both login flows to use the helper. The PKCE path retains its existing fallback error context.
| "github.com/wso2/agent-manager/cli/pkg/iostreams" | ||
| ) | ||
|
|
||
| func TestRunLogin_TypedErrorPassthrough(t *testing.T) { |
There was a problem hiding this comment.
These 4 tests each repeat ~13 lines of setup + envelope-decode + code assertion. auth/login_test.go in this same PR is already table-driven — a table here would cut the duplication and match that style.
There was a problem hiding this comment.
Addressed in the latest commit. I consolidated the repeated login error test cases into a single table-driven test to reduce duplication and keep the assertions in one place.
|
Hi @jhivandb, Thank you for the review and feedback. I've addressed all comments in the latest commit. Let me know if you have any further feedback or if there are any additional changes required. |
|
Hi @nimeshk03, I don't think we should merge this right now. The existing token exchange paths already preserve the OAuth error text: return nil, fmt.Errorf("client_credentials token exchange: %w", err) So this mainly changes the structured JSON code from I also don't see a real browser-cancel path that reaches Given that, I don't see enough value in adding new stable error codes here yet. I think we should defer this until the authz/client-credentials story is settled, then add the error contract |
2 participants
Purpose
auth.Loginreturned plainfmt.Errorf-wrapped errors on all failure paths, socmd/login.gocould not distinguish a known auth failure from a transport error — both becameCLI_TRANSPORT.Resolves #786
Goals
invalid_granttoken-endpoint errors toclierr.Unauthorizedclierr.AuthLoginCancelledcodeclierr.CLIErrorvalues throughcmd/login.gounchanged instead of always wrapping asCLI_TRANSPORTApproach
classifyTokenErrorinauth/login.gothat inspects*oauth2.RetrieveErrorand returns a typedCLIErrorfor known cases; returnsnilfor anything else so the existingfmt.Errorfwrap still appliesloginPKCE, check forcontext.Canceled,context.DeadlineExceeded, andio.EOFbefore callingclassifyTokenErrorcmd/login.go, useerrors.Asto pass through aclierr.CLIErrorfromauth.Loginbefore falling back to theCLI_TRANSPORTwrapAuthLoginCancelled = "AUTH_LOGIN_CANCELLED"constant toclierrUser stories
As a script author using
amctl login, I want the JSON errorcodefield to distinguish auth rejections from transport failures so I can handle each case without parsing message strings.Release note
amctl loginnow emitsUNAUTHORIZEDfor 401/invalid_granterrors andAUTH_LOGIN_CANCELLEDwhen the browser login is cancelled, instead of always emittingCLI_TRANSPORT.Documentation
N/A — internal CLI error code change, no user-facing doc impact.
Training
N/A
Certification
N/A
Marketing
N/A
Automation tests
pkg/auth/login_test.go(new): 8 table-driven cases forclassifyTokenErrorcovering 401,invalid_grant, wrapped errors, 403, unknown codes, plain errors, nil responsepkg/cmd/login_test.go(new): 4 cases verifying typedCLIErrorpassthrough, plain errors becomeCLI_TRANSPORT, cancelled login passesAuthLoginCancelled, missing--urlreturnsINVALID_FLAGSecurity checks
Samples
N/A
Related PRs
#753 — original
amctlCLI PR where this fix was deferredMigrations (if applicable)
N/A
Test environment
Ubuntu 26.04, Go 1.26.0
Learning
N/A
Summary by CodeRabbit
Bug Fixes
Tests