Refactor error messages for clarity and consistency in token handling - #36
Conversation
…n-related error handling in cliSession and token source implementations for improved clarity and consistency.
WalkthroughError messages were standardized across CLI commands, CLI session storage, and OAuth2 token sources. cliSession adds a fallback from chunked storage to single-key retrieval in GetKey. Tests were updated to expect new error prefixes. Minor prompt formatting changes were made in NewCliSession. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant CS as CliSession
participant Store as Storage
Client->>CS: GetKey(ctx, key)
CS->>Store: Read chunk count
alt Chunk count found and valid
loop For each chunk i
CS->>Store: Read chunk i
alt Read fails
CS-->>Client: error "chunked storage: …"
end
end
CS-->>Client: Concatenated key
else Chunk count missing/parse error
note right of CS: New/changed flow: fallback
CS->>Store: Read single key
alt Fallback succeeds
CS-->>Client: Key
else Fallback fails
CS-->>Client: error "chunked storage (chunked: …, fallback: …)"
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests
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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
examples/cli/pkg/cmd/loginCommand.go (1)
33-52: Standardize error prefixing and context usage in runLoginWrap earlier failures with the same "run login failed" prefix and use the passed-in cmd.Context() consistently.
deviceFlow, err := config.NewDeviceAuthorizationFlow(c.kindeDomain) if err != nil { - return err + return fmt.Errorf("run login failed: %w", err) } -deviceAuth, err := deviceFlow.StartDeviceAuth(c.cmd.Context()) +deviceAuth, err := deviceFlow.StartDeviceAuth(cmd.Context()) if err != nil { - return err + return fmt.Errorf("run login failed: %w", err) } ... -err = deviceFlow.ExchangeDeviceAccessToken(c.cmd.Context(), deviceAuth) +err = deviceFlow.ExchangeDeviceAccessToken(cmd.Context(), deviceAuth) if err != nil { - return err + return fmt.Errorf("run login failed: %w", err) } token, err := deviceFlow.GetToken(cmd.Context()) if err != nil { return fmt.Errorf("run login failed: %w", err) }examples/cli/pkg/cmd/whoAmICommand.go (2)
32-35: Prefix the constructor error to match the new schemeAlign with the PR’s error style.
deviceFlow, err := config.NewDeviceAuthorizationFlow(c.kindeDomain) if err != nil { - return err + return fmt.Errorf("run whoami failed: %w", err) }
37-40: Avoid logging a nil error in the unauthenticated branchGuard Err(err) to prevent logging “error=null”.
- if isAuthenticated, err := deviceFlow.IsAuthenticated(cmd.Context()); !isAuthenticated { - log.Error().Err(err).Msg("You are not authenticated") + if isAuthenticated, err := deviceFlow.IsAuthenticated(cmd.Context()); !isAuthenticated { + if err != nil { + log.Error().Err(err).Msg("You are not authenticated") + } else { + log.Error().Msg("You are not authenticated") + } return fmt.Errorf("you are not logged in. Please run 'login' command first") }oauth2/client_credentials/token_source.go (1)
51-52: Propagate SetRawToken failure (currently ignored)If persisting the token fails, callers lose that signal. Consider returning it (mirrors authorization_code.Token()).
- t.flow.sessionHooks.SetRawToken(possiblyNewToken) - return possiblyNewToken, nil + if err := t.flow.sessionHooks.SetRawToken(possiblyNewToken); err != nil { + return possiblyNewToken, fmt.Errorf("session hook: %w", err) + } + return possiblyNewToken, niloauth2/authorization_code/token_source.go (1)
17-21: Avoid double-wrapping Token() errors hereToken() already prefixes with “session hook:”/“token source:”. Re-wrapping as “failed to get token” is redundant.
token, err := t.Token() if err != nil { - return nil, fmt.Errorf("failed to get token: %w", err) + return nil, err }frameworks/cli/cliSession.go (2)
84-89: Join chunked+fallback errors while keeping the stable prefixUse errors.Join so callers can unwrap both causes; retain the “chunked storage:” prefix for test stability.
- ringItem, fallbackErr := c.keyring.Get(key) - if fallbackErr != nil { - return nil, fmt.Errorf("chunked storage (chunked: %v, fallback: %w)", err, fallbackErr) - } + ringItem, fallbackErr := c.keyring.Get(key) + if fallbackErr != nil { + return nil, fmt.Errorf("chunked storage: %w", errors.Join(err, fallbackErr)) + }
263-267: Minor UX polish: write prompts/errors to stderr and use lowercase error startsKeeps CLI stdout clean for piping and follows Go error style.
- fmt.Printf("%s\n", prompt) + fmt.Fprintln(os.Stderr, prompt) ... - fmt.Printf("\nError reading password: %v\n", err) + fmt.Fprintf(os.Stderr, "\nerror reading password: %v\n", err)frameworks/cli/cliSession_test.go (1)
1-12: Add a test for GetCodeVerifier error pathGiven the new “code_verifier storage:” prefix, add a small test asserting the error substring when the key is missing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
examples/cli/pkg/cmd/loginCommand.go(1 hunks)examples/cli/pkg/cmd/whoAmICommand.go(1 hunks)frameworks/cli/cliSession.go(9 hunks)frameworks/cli/cliSession_test.go(7 hunks)oauth2/authorization_code/token_source.go(1 hunks)oauth2/client_credentials/token_source.go(1 hunks)
🔇 Additional comments (6)
oauth2/client_credentials/token_source.go (1)
42-45: LGTM on error prefix change“token source: %w” matches the new, consistent taxonomy.
oauth2/authorization_code/token_source.go (1)
37-46: LGTM on standardized prefixes“session hook: %w” and “token source: %w” improve traceability without over-specifying steps.
frameworks/cli/cliSession.go (3)
146-152: LGTM on “raw token storage” prefixingClear, consistent, and wraps both read and unmarshal failures.
200-220: LGTM on chunk save + count save prefixesThe “chunked storage:” and “chunk count storage:” labels make failures actionable.
185-205: Ensure Go version ≥1.22 and implementmin()helper
Nogo.modwas found to declare Go 1.22 (required forfor i := range chunks), and there’s nomin()function in the codebase. Add a module file withgo 1.22(or bump CI) and define or import amin()helper before using it.frameworks/cli/cliSession_test.go (1)
108-111: Tests updated to new prefixes — looks goodAssertions target stable substrings (“raw token storage: …”, “chunked storage: …”) and shouldn’t be brittle.
1 participant
Improve error messages in login and whoami commands, and enhance token-related error handling in cliSession and token source implementations for better clarity and consistency.