Open
Conversation
WalkthroughAdds a device-code web login flow selectable via a new 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 3
🧹 Nitpick comments (1)
cli/auth.go (1)
174-183: Handleslow_downresponses during device token polling.
pollDeviceTokentreats unknown errors as fatal; for device flow,slow_downshould increase polling interval and continue instead of aborting login.♻️ Proposed refactor
+var errDeviceFlowSlowDown = errors.New("device flow requested slower polling") @@ - token, err := pollDeviceToken(codeResp.DeviceCode) - if err != nil { - return err - } + token, err := pollDeviceToken(codeResp.DeviceCode) + if errors.Is(err, errDeviceFlowSlowDown) { + interval += 5 * time.Second + continue + } + if err != nil { + return err + } @@ switch tokenResp.Error { case "authorization_pending": return "", nil + case "slow_down": + return "", errDeviceFlowSlowDown case "expired_token": return "", fmt.Errorf("device code expired")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/auth.go` around lines 174 - 183, pollDeviceToken currently treats unknown tokenResp.Error values as fatal; specifically handle the "slow_down" response by not returning an error but increasing the polling interval and continuing to poll. In the switch over tokenResp.Error (and in/around the pollDeviceToken function), add a case for "slow_down" that increments the delay (or adds a backoff) before the next request and then continue the loop, rather than returning an error for that value; keep existing behavior for "authorization_pending", "expired_token", empty string, and the default error case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/auth.go`:
- Line 102: In runAuthLoginWeb and pollDeviceToken replace the unchecked defer
resp.Body.Close() with a pattern that captures and logs/returns the Close()
error: assign resp.Body.Close() to a variable inside a deferred func (e.g.,
defer func() { if err := resp.Body.Close(); err != nil { /* handle: log with the
existing logger or wrap/return the error */ } }()), so the close error is not
ignored and satisfies errcheck; locate the resp variable usages in
runAuthLoginWeb and pollDeviceToken and apply this deferred closure pattern
consistently.
- Line 413: The masking code for variable token (masked := token[:4] +
strings.Repeat("*", 40)) can panic for tokens shorter than 4; update the logic
in the function that builds masked to first compute a safe prefix length (e.g.,
n := len(token); if n > 4 { n = 4 }) or use a min helper, then slice token[:n];
decide how many stars to append (fixed 40 or max(0, 40-(len(token)-n))), and
assign masked using that safe prefix and the stars; change the single expression
that creates masked to use these guarded values so short tokens do not cause a
panic.
- Around line 408-411: The current RunE handler in cli/auth.go calls os.Exit
when err != nil || token == "", which prevents Cobra from handling errors and
makes tests brittle; change this to return errors instead and distinguish
keyring.ErrNotFound from other keyring errors: inside the function (the RunE
callback that obtains token and err), if err == keyring.ErrNotFound or token ==
"" return a clear user-facing error (e.g., fmt.Errorf("not logged into
mobilenexthq.com")), if err != nil (and not ErrNotFound) return that underlying
keyring error wrapped with context, and remove any os.Exit calls so Cobra can
handle/report errors and tests can assert on returned errors.
---
Nitpick comments:
In `@cli/auth.go`:
- Around line 174-183: pollDeviceToken currently treats unknown tokenResp.Error
values as fatal; specifically handle the "slow_down" response by not returning
an error but increasing the polling interval and continuing to poll. In the
switch over tokenResp.Error (and in/around the pollDeviceToken function), add a
case for "slow_down" that increments the delay (or adds a backoff) before the
next request and then continue the loop, rather than returning an error for that
value; keep existing behavior for "authorization_pending", "expired_token",
empty string, and the default error case.
| if err != nil { | ||
| return fmt.Errorf("failed to request device code: %w", err) | ||
| } | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
Handle resp.Body.Close() errors (currently failing lint).
Line 102 and Line 162 defer resp.Body.Close() without checking the returned error, which is currently failing errcheck.
🔧 Proposed fix
- defer resp.Body.Close()
+ defer func() {
+ if closeErr := resp.Body.Close(); closeErr != nil {
+ fmt.Fprintf(os.Stderr, "warning: failed to close response body: %v\n", closeErr)
+ }
+ }()Apply this pattern in both runAuthLoginWeb and pollDeviceToken.
Also applies to: 162-162
🧰 Tools
🪛 GitHub Check: lint
[failure] 102-102:
Error return value of resp.Body.Close is not checked (errcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/auth.go` at line 102, In runAuthLoginWeb and pollDeviceToken replace the
unchecked defer resp.Body.Close() with a pattern that captures and logs/returns
the Close() error: assign resp.Body.Close() to a variable inside a deferred func
(e.g., defer func() { if err := resp.Body.Close(); err != nil { /* handle: log
with the existing logger or wrap/return the error */ } }()), so the close error
is not ignored and satisfies errcheck; locate the resp variable usages in
runAuthLoginWeb and pollDeviceToken and apply this deferred closure pattern
consistently.
| os.Exit(1) | ||
| } | ||
|
|
||
| masked := token[:4] + strings.Repeat("*", 40) |
There was a problem hiding this comment.
Guard token masking against short tokens to prevent slice panic.
token[:4] panics when token length is less than 4.
🔧 Proposed fix
- masked := token[:4] + strings.Repeat("*", 40)
+ visible := 4
+ if len(token) < visible {
+ visible = len(token)
+ }
+ masked := token[:visible] + strings.Repeat("*", len(token)-visible)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| masked := token[:4] + strings.Repeat("*", 40) | |
| visible := 4 | |
| if len(token) < visible { | |
| visible = len(token) | |
| } | |
| masked := token[:visible] + strings.Repeat("*", len(token)-visible) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/auth.go` at line 413, The masking code for variable token (masked :=
token[:4] + strings.Repeat("*", 40)) can panic for tokens shorter than 4; update
the logic in the function that builds masked to first compute a safe prefix
length (e.g., n := len(token); if n > 4 { n = 4 }) or use a min helper, then
slice token[:n]; decide how many stars to append (fixed 40 or max(0,
40-(len(token)-n))), and assign masked using that safe prefix and the stars;
change the single expression that creates masked to use these guarded values so
short tokens do not cause a panic.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
cli/auth.go (3)
406-411:⚠️ Potential issue | 🟠 MajorCheck
errbefore treating this as logged-out.With the current ordering, a real keyring backend failure can still be flattened into "not logged in" whenever
token == "". HandleErrNotFoundfirst, then other errors, then the empty-token case.🔧 Suggested fix
token, err := keyring.Get(keyringService, keyringUser) - if errors.Is(err, keyring.ErrNotFound) || token == "" { + if errors.Is(err, keyring.ErrNotFound) { return fmt.Errorf("not logged into mobilenexthq.com") } if err != nil { return fmt.Errorf("failed to read keyring: %w", err) } + if token == "" { + return fmt.Errorf("not logged into mobilenexthq.com") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/auth.go` around lines 406 - 411, The current logic calls keyring.Get and treats empty token as "not logged in" before handling actual errors; update the ordering in the code that uses keyring.Get (the token, err := keyring.Get(...) block) to first check if errors.Is(err, keyring.ErrNotFound) and return "not logged into mobilenexthq.com", then check if err != nil and return fmt.Errorf("failed to read keyring: %w", err), and only after those error checks validate if token == "" and return "not logged into mobilenexthq.com" if empty; reference keyring.Get, keyring.ErrNotFound, token and err when making the changes.
414-414:⚠️ Potential issue | 🟠 MajorGuard the token slice before masking.
token[:4]still panics when the stored token is shorter than four characters.🔧 Suggested fix
- masked := token[:4] + strings.Repeat("*", 40) + visible := 4 + if len(token) < visible { + visible = len(token) + } + masked := token[:visible] + strings.Repeat("*", 40)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/auth.go` at line 414, The masking line that builds masked := token[:4] + strings.Repeat("*", 40) can panic if token is shorter than 4 chars; update the code that produces the mask (where `token` is used) to guard the slice by checking len(token) and slicing safely (e.g., use the full token when len(token) < 4 or compute prefix := token[:min(4,len(token))]) before concatenating the repeated asterisks so `masked` never slices out of bounds in the function handling authentication/token display.
101-101:⚠️ Potential issue | 🟠 MajorHandle both
resp.Body.Close()errors.
errcheckis still failing on these two defers, so the PR stays red until the close error is consumed explicitly.🔧 Minimal fix
- defer resp.Body.Close() + defer func() { + if closeErr := resp.Body.Close(); closeErr != nil { + fmt.Printf("warning: failed to close response body: %v\n", closeErr) + } + }()Also applies to: 161-161
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/auth.go` at line 101, Replace the plain defer resp.Body.Close() calls with an explicit close-error consumer: wrap the Close() in a deferred func that captures the returned error (e.g., if cerr := resp.Body.Close(); cerr != nil { /* log or handle cerr */ }) so the Close error is not ignored; locate both occurrences where resp.Body.Close() is deferred and update them to use this pattern, referencing the resp variable and its Body.Close() method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/auth.go`:
- Around line 128-138: The device-flow path currently stores the raw token
returned by pollDeviceToken (deviceTokenResponse.AccessToken) into keyring via
keyring.Set, while the browser/normal flow stores apiTokenResponse.Token after
exchanging an ID token; confirm these are equivalent session tokens and if not,
implement a consistent exchange so both flows persist the same token type.
Specifically, inspect pollDeviceToken and the device endpoint response, compare
with the API exchange that produces apiTokenResponse.Token, and either (a)
perform the same token exchange in the device flow to produce
apiTokenResponse.Token before calling keyring.Set(keyringService, keyringUser,
token), or (b) normalize both responses to a common session token structure and
persist that normalized token; update any consumers expecting
apiTokenResponse.Token to accept the normalized form. Ensure function names
referenced (pollDeviceToken, keyring.Set, apiTokenResponse.Token,
deviceTokenResponse.AccessToken) are where the change is made so auth login
--web writes credentials compatible with existing consumers.
---
Duplicate comments:
In `@cli/auth.go`:
- Around line 406-411: The current logic calls keyring.Get and treats empty
token as "not logged in" before handling actual errors; update the ordering in
the code that uses keyring.Get (the token, err := keyring.Get(...) block) to
first check if errors.Is(err, keyring.ErrNotFound) and return "not logged into
mobilenexthq.com", then check if err != nil and return fmt.Errorf("failed to
read keyring: %w", err), and only after those error checks validate if token ==
"" and return "not logged into mobilenexthq.com" if empty; reference
keyring.Get, keyring.ErrNotFound, token and err when making the changes.
- Line 414: The masking line that builds masked := token[:4] +
strings.Repeat("*", 40) can panic if token is shorter than 4 chars; update the
code that produces the mask (where `token` is used) to guard the slice by
checking len(token) and slicing safely (e.g., use the full token when len(token)
< 4 or compute prefix := token[:min(4,len(token))]) before concatenating the
repeated asterisks so `masked` never slices out of bounds in the function
handling authentication/token display.
- Line 101: Replace the plain defer resp.Body.Close() calls with an explicit
close-error consumer: wrap the Close() in a deferred func that captures the
returned error (e.g., if cerr := resp.Body.Close(); cerr != nil { /* log or
handle cerr */ }) so the Close error is not ignored; locate both occurrences
where resp.Body.Close() is deferred and update them to use this pattern,
referencing the resp variable and its Body.Close() method.
Comment on lines
+128
to
+138
| token, err := pollDeviceToken(codeResp.DeviceCode) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if token == "" { | ||
| continue | ||
| } | ||
|
|
||
| if err := keyring.Set(keyringService, keyringUser, token); err != nil { | ||
| return fmt.Errorf("failed to store session token: %w", err) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare how the auth token is produced and consumed across the repo.
rg -n -C3 --type=go 'keyring\.(Set|Get)\(keyringService,\s*keyringUser'
rg -n -C3 --type=go 'exchangeIDTokenForSession|apiTokenURL|deviceTokenResponse|access_token'Repository: mobile-next/mobilecli
Length of output: 3990
🏁 Script executed:
cat -n cli/auth.go | head -n 200 | tail -n 100Repository: mobile-next/mobilecli
Length of output: 3714
🏁 Script executed:
sed -n '140,200p' cli/auth.goRepository: mobile-next/mobilecli
Length of output: 1919
🏁 Script executed:
sed -n '371,390p' cli/auth.goRepository: mobile-next/mobilecli
Length of output: 697
🏁 Script executed:
sed -n '371,410p' cli/auth.goRepository: mobile-next/mobilecli
Length of output: 1337
🏁 Script executed:
# Also check what deviceTokenURL constant is
rg -n 'deviceTokenURL\s*=' cli/auth.goRepository: mobile-next/mobilecli
Length of output: 140
🏁 Script executed:
rg -n 'type apiTokenResponse' cli/auth.goRepository: mobile-next/mobilecli
Length of output: 98
🏁 Script executed:
sed -n '51,60p' cli/auth.goRepository: mobile-next/mobilecli
Length of output: 346
Device flow stores a different token type than the browser flow.
The --web flag stores deviceTokenResponse.AccessToken from the device endpoint, but the normal login path stores apiTokenResponse.Token from the API endpoint (after exchanging an IDToken). These are different response structures from different endpoints. Confirm that both token types are interchangeable session tokens, or implement a consistent token exchange flow for both login paths to ensure auth login --web produces credentials compatible with existing consumers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/auth.go` around lines 128 - 138, The device-flow path currently stores
the raw token returned by pollDeviceToken (deviceTokenResponse.AccessToken) into
keyring via keyring.Set, while the browser/normal flow stores
apiTokenResponse.Token after exchanging an ID token; confirm these are
equivalent session tokens and if not, implement a consistent exchange so both
flows persist the same token type. Specifically, inspect pollDeviceToken and the
device endpoint response, compare with the API exchange that produces
apiTokenResponse.Token, and either (a) perform the same token exchange in the
device flow to produce apiTokenResponse.Token before calling
keyring.Set(keyringService, keyringUser, token), or (b) normalize both responses
to a common session token structure and persist that normalized token; update
any consumers expecting apiTokenResponse.Token to accept the normalized form.
Ensure function names referenced (pollDeviceToken, keyring.Set,
apiTokenResponse.Token, deviceTokenResponse.AccessToken) are where the change is
made so auth login --web writes credentials compatible with existing consumers.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cli/auth.go (1)
406-411:⚠️ Potential issue | 🟠 MajorCheck
errbefore treating an empty token as logged-out.With the current order, any non-
ErrNotFoundkeyring failure that also returns an empty string is reported as "not logged into mobilenexthq.com" instead of surfacing the real read error.🔧 Suggested fix
token, err := keyring.Get(keyringService, keyringUser) - if errors.Is(err, keyring.ErrNotFound) || token == "" { - return fmt.Errorf("not logged into mobilenexthq.com") - } if err != nil { + if errors.Is(err, keyring.ErrNotFound) { + return fmt.Errorf("not logged into mobilenexthq.com") + } return fmt.Errorf("failed to read keyring: %w", err) } + if token == "" { + return fmt.Errorf("not logged into mobilenexthq.com") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cli/auth.go` around lines 406 - 411, The current logic calls keyring.Get and treats an empty token as "not logged into mobilenexthq.com" before checking for a non-ErrNotFound error; change the order in the keyring.Get handling so you first inspect err: if err != nil then if errors.Is(err, keyring.ErrNotFound) return the "not logged into mobilenexthq.com" message, otherwise return fmt.Errorf("failed to read keyring: %w", err); only after err is nil check if token == "" and then return the "not logged into mobilenexthq.com" error. This touches the keyring.Get call and the token/err handling in the same function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cli/auth.go`:
- Around line 125-130: The poll loop treating any err from
pollDeviceToken(codeResp.DeviceCode) as terminal is wrong for the "slow_down"
case — instead detect when the error indicates "slow_down" (inspect the error
type/message returned by pollDeviceToken) and, rather than returning err,
increase the poll interval (e.g., back off by multiplying interval up to a cap)
and continue the loop; leave other errors unchanged (still return them). Apply
the same change to the second polling block referenced (the similar loop around
the other pollDeviceToken usage) so both loops handle "slow_down" by backing off
and continuing rather than aborting.
---
Duplicate comments:
In `@cli/auth.go`:
- Around line 406-411: The current logic calls keyring.Get and treats an empty
token as "not logged into mobilenexthq.com" before checking for a
non-ErrNotFound error; change the order in the keyring.Get handling so you first
inspect err: if err != nil then if errors.Is(err, keyring.ErrNotFound) return
the "not logged into mobilenexthq.com" message, otherwise return
fmt.Errorf("failed to read keyring: %w", err); only after err is nil check if
token == "" and then return the "not logged into mobilenexthq.com" error. This
touches the keyring.Get call and the token/err handling in the same function.
Comment on lines
+125
to
+130
| for time.Now().Before(deadline) { | ||
| time.Sleep(interval) | ||
|
|
||
| token, err := pollDeviceToken(codeResp.DeviceCode) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Treat slow_down as backoff, not a terminal login failure.
Right now any unrecognized device-token error aborts the flow. If the endpoint responds with slow_down, this will fail login instead of increasing the poll interval and continuing.
🔧 Suggested fix
+var errDeviceFlowSlowDown = errors.New("device flow slow_down")
+
func pollDeviceToken(deviceCode string) (string, error) {
...
switch tokenResp.Error {
case "authorization_pending":
return "", nil
+ case "slow_down":
+ return "", errDeviceFlowSlowDown
case "expired_token":
return "", fmt.Errorf("device code expired")
case "":
return tokenResp.AccessToken, nil
default:
return "", fmt.Errorf("device token error: %s", tokenResp.Error)
}
} for time.Now().Before(deadline) {
time.Sleep(interval)
token, err := pollDeviceToken(codeResp.DeviceCode)
if err != nil {
+ if errors.Is(err, errDeviceFlowSlowDown) {
+ interval += 5 * time.Second
+ continue
+ }
return err
}Also applies to: 173-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cli/auth.go` around lines 125 - 130, The poll loop treating any err from
pollDeviceToken(codeResp.DeviceCode) as terminal is wrong for the "slow_down"
case — instead detect when the error indicates "slow_down" (inspect the error
type/message returned by pollDeviceToken) and, rather than returning err,
increase the poll interval (e.g., back off by multiplying interval up to a cap)
and continue the loop; leave other errors unchanged (still return them). Apply
the same change to the second polling block referenced (the similar loop around
the other pollDeviceToken usage) so both loops handle "slow_down" by backing off
and continuing rather than aborting.
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
1 participant
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.
No description provided.