feat: improve login scope validation and success output#317
Merged
JackZhao10086 merged 8 commits intomainfrom Apr 8, 2026
Merged
feat: improve login scope validation and success output#317JackZhao10086 merged 8 commits intomainfrom
JackZhao10086 merged 8 commits intomainfrom
Conversation
- Add scope validation to check for missing requested scopes - Implement detailed scope breakdown in login success output - Add new message strings for scope-related output - Refactor login success output to handle both JSON and text formats - Add tests for scope validation and output scenarios
|
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:
📝 WalkthroughWalkthroughAdds persistent caching for device-code requested scopes, computes and reports scope reconciliation (requested/newly-granted/already-granted/missing), enforces requested-scope validation before completing login, updates CLI messages for scope diagnostics, and expands tests for caching, reconciliation, and output behaviors. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI
participant DeviceAuth
participant TokenStore
participant ScopeCache
participant ScopeValidator
participant Output
User->>CLI: run login (maybe --no-wait)
CLI->>DeviceAuth: start device flow (get device_code)
alt --no-wait
CLI->>ScopeCache: save requested scope for device_code
CLI->>User: show device_code and exit
User->>DeviceAuth: complete auth via browser
end
CLI->>DeviceAuth: poll device flow (pollDeviceToken)
DeviceAuth->>TokenStore: return token (may be nil) with granted scopes
CLI->>ScopeCache: load requested scope (if cached)
CLI->>ScopeValidator: build scope summary (requested, newly, already, missing)
alt token nil
ScopeValidator->>CLI: error (auth succeeded but no token)
CLI->>User: print error
else missing scopes
ScopeValidator->>Output: emit warning + scope breakdown (JSON or text)
ScopeCache->>CLI: remove cache (per policy)
CLI->>User: success-with-warning
else all granted
ScopeValidator->>Output: emit success + scope breakdown
ScopeCache->>CLI: remove cache
CLI->>User: authorization_complete (JSON or text)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 unit tests (beta)
Comment |
github-actions
Bot
added
the
size/L
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@d6499273d99dffad0576323b484b35989d0ac226🧩 Skill updatenpx skills add larksuite/cli#feat/optimize_login_scope_print -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/auth/login_result.go (1)
178-186: Consider simplifying the redundant conditional blocks.When
loginSucceededis true, the code printsissue.Messagein two separate blocks (lines 180 and 185). While functionally correct, the structure could be clearer.♻️ Suggested simplification
fmt.Fprintln(f.IOStreams.ErrOut) if loginSucceeded { output.PrintSuccess(f.IOStreams.ErrOut, fmt.Sprintf(msg.LoginSuccess, userName, openId)) - } else { fmt.Fprintln(f.IOStreams.ErrOut, issue.Message) - } - if loginSucceeded { - fmt.Fprintln(f.IOStreams.ErrOut, issue.Message) + } else { + fmt.Fprintln(f.IOStreams.ErrOut, issue.Message) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login_result.go` around lines 178 - 186, The code prints issue.Message twice when loginSucceeded is true; remove the redundant second if block and print issue.Message only once. Concretely, keep the success/failure branch that calls output.PrintSuccess(f.IOStreams.ErrOut, fmt.Sprintf(msg.LoginSuccess, userName, openId)) when loginSucceeded is true (and the else branch that prints issue.Message on failure), then move a single fmt.Fprintln(f.IOStreams.ErrOut, issue.Message) outside/after those conditionals so issue.Message is emitted exactly once; update references to loginSucceeded, issue.Message, output.PrintSuccess and f.IOStreams.ErrOut accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/auth/login_result.go`:
- Around line 178-186: The code prints issue.Message twice when loginSucceeded
is true; remove the redundant second if block and print issue.Message only once.
Concretely, keep the success/failure branch that calls
output.PrintSuccess(f.IOStreams.ErrOut, fmt.Sprintf(msg.LoginSuccess, userName,
openId)) when loginSucceeded is true (and the else branch that prints
issue.Message on failure), then move a single fmt.Fprintln(f.IOStreams.ErrOut,
issue.Message) outside/after those conditionals so issue.Message is emitted
exactly once; update references to loginSucceeded, issue.Message,
output.PrintSuccess and f.IOStreams.ErrOut accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4c42938-e84f-4c74-9eb8-9cff5310550b
📒 Files selected for processing (4)
cmd/auth/login.gocmd/auth/login_messages.gocmd/auth/login_result.gocmd/auth/login_test.go
JackZhao10086
changed the title
Implement caching of requested scopes during device code login flow to ensure proper scope validation after authorization. The cache is stored in JSON files under config directory and automatically cleaned up after successful or failed authorization. Add tests for scope caching functionality and verify proper integration with existing login flow.
Greptile SummaryThis PR adds post-login scope validation and richer scope breakdown output to Confidence Score: 5/5Safe to merge; all remaining findings are P2 style and low-risk edge cases that do not affect the primary login flow. The P1 concerns from the previous review round (nil-token guard, emptyIfNil normalization, cache cleanup on error paths) are all addressed. The three remaining comments are P2: silent JSON write errors in success paths, a theoretical cache-key collision, and a dead result.OK arm. None affect correctness on the happy path. cmd/auth/login_result.go (JSON write error handling), cmd/auth/login_scope_cache.go (key sanitization and dead branch)
|
| Filename | Overview |
|---|---|
| cmd/auth/login.go | Adds scope cache persistence for --no-wait, nil-token guard, pollDeviceToken indirection for testability, and delegates success/issue output to new helpers; logic is sound. |
| cmd/auth/login_result.go | New file centralizing scope summary computation and login output; JSON success paths still use b, _ := json.Marshal + fmt.Fprintln instead of the encoder pattern introduced elsewhere in the PR. |
| cmd/auth/login_scope_cache.go | New file implementing file-based scope cache; sanitization via underscore replacement can cause key collisions, and shouldRemoveLoginRequestedScope contains a dead result.OK branch. |
| cmd/auth/login_messages.go | Adds new message fields in both zh and en locales; all fields are non-empty and verified by the new message tests. |
| cmd/auth/login_test.go | Adds unit tests for scope summary building, JSON output shape, scope-issue alignment, and empty-slice serialization; well-structured coverage. |
Sequence Diagram
sequenceDiagram
participant User
participant CLI as lark-cli auth login
participant Cache as Scope Cache (fs)
participant Lark as Lark Auth API
participant KR as Keyring
alt --no-wait flow
User->>CLI: auth login --scope X --no-wait
CLI->>Lark: RequestDeviceAuthorization(scope)
Lark-->>CLI: device_code, verification_url
CLI->>Cache: saveLoginRequestedScope(device_code, scope)
CLI-->>User: JSON {verification_url, device_code}
User->>CLI: auth login --device-code code
CLI->>Cache: loadLoginRequestedScope(device_code)
Cache-->>CLI: requestedScope
CLI->>Lark: PollDeviceToken(device_code)
Lark-->>CLI: token + granted_scope
CLI->>Cache: defer removeLoginRequestedScope
else inline flow
User->>CLI: auth login --scope X
CLI->>Lark: RequestDeviceAuthorization(scope)
CLI->>Lark: PollDeviceToken(device_code)
Lark-->>CLI: token + granted_scope
end
CLI->>Lark: getUserInfo(access_token)
Lark-->>CLI: openId, userName
CLI->>KR: GetStoredToken for previous scope
CLI->>CLI: buildLoginScopeSummary
CLI->>KR: SetStoredToken(new token)
alt all requested scopes granted
CLI-->>User: authorization_complete
else some scopes missing
CLI-->>User: authorization_complete + warning.missing_scope
end
Reviews (9): Last reviewed commit: "refactor(auth): improve scope handling a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Add detailed doc comments to all functions in login scope cache and result handling files to improve code documentation and maintainability.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/auth/login_scope_cache.go (1)
83-90: Avoid message-text coupling for cancellation detection.Cleanup logic currently depends on exact message text (
"Polling was cancelled"). Consider a stable machine field/code (e.g., explicit cancellation reason) instead of string matching to prevent silent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login_scope_cache.go` around lines 83 - 90, The cleanup currently relies on exact message text in shouldRemoveLoginRequestedScope(result *larkauth.DeviceFlowResult) to detect user cancellation; change it to check a stable machine-readable field (e.g., add/use result.Reason or result.Code with a specific cancellation enum/value) instead of matching result.Message == "Polling was cancelled", and fall back to the old message check only if the new field is absent to preserve compatibility; update callers/parsers that construct DeviceFlowResult so they populate the new field when cancellation occurs and adjust any serialization/deserialization to carry that code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/auth/login_scope_cache.go`:
- Around line 43-79: The functions saveLoginRequestedScope,
loadLoginRequestedScope and removeLoginRequestedScope must validate paths before
any file I/O: call validate.SafeInputPath on the resolved directory from
loginScopeCacheDir() before MkdirAll, and call validate.SafeInputPath on the
resolved file path from loginScopeCachePath(deviceCode) before ReadFile,
AtomicWrite and Remove; return the validation error if it fails so no file
operation proceeds on an unsafe path.
In `@cmd/auth/login_test.go`:
- Around line 723-790: Both tests
(TestAuthLoginRun_JSONWriteFailure_NoWaitIgnoresWriterError and
TestAuthLoginRun_JSONWriteFailure_DeviceAuthorizationStillPolls) call
authLoginRun but do not isolate config state; add
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the start of each test to
ensure LARKSUITE_CLI_CONFIG_DIR points to a temporary directory so tests don't
read/write real config and remain hermetic.
---
Nitpick comments:
In `@cmd/auth/login_scope_cache.go`:
- Around line 83-90: The cleanup currently relies on exact message text in
shouldRemoveLoginRequestedScope(result *larkauth.DeviceFlowResult) to detect
user cancellation; change it to check a stable machine-readable field (e.g.,
add/use result.Reason or result.Code with a specific cancellation enum/value)
instead of matching result.Message == "Polling was cancelled", and fall back to
the old message check only if the new field is absent to preserve compatibility;
update callers/parsers that construct DeviceFlowResult so they populate the new
field when cancellation occurs and adjust any serialization/deserialization to
carry that code.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 008b1348-6534-4ff8-93fc-f89fa130a44e
📒 Files selected for processing (5)
cmd/auth/login.gocmd/auth/login_result.gocmd/auth/login_scope_cache.gocmd/auth/login_scope_cache_test.gocmd/auth/login_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/auth/login.go
- cmd/auth/login_result.go
- Remove PendingScopes field and related logic as it's no longer needed - Add emptyIfNil helper to ensure nil slices are normalized to empty slices in JSON output - Update tests to verify JSON output stability and fix expected text outputs
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/auth/login_result.go (2)
136-151: Minor: Consider renaming parameter for clarity.The parameter
errOutis of type*cmdutil.IOStreamsbut the name suggests it's just the error output stream. Consider renaming tostreamsoriosfor clarity.✏️ Suggested rename
-func writeLoginScopeBreakdown(errOut *cmdutil.IOStreams, msg *loginMsg, summary *loginScopeSummary) { +func writeLoginScopeBreakdown(streams *cmdutil.IOStreams, msg *loginMsg, summary *loginScopeSummary) { if summary == nil { summary = &loginScopeSummary{} } - fmt.Fprintf(errOut.ErrOut, msg.RequestedScopes, formatScopeList(summary.Requested, msg.NoScopes)) - fmt.Fprintf(errOut.ErrOut, msg.NewlyGrantedScopes, formatScopeList(summary.NewlyGranted, msg.NoScopes)) + fmt.Fprintf(streams.ErrOut, msg.RequestedScopes, formatScopeList(summary.Requested, msg.NoScopes)) + fmt.Fprintf(streams.ErrOut, msg.NewlyGrantedScopes, formatScopeList(summary.NewlyGranted, msg.NoScopes)) if len(summary.AlreadyGranted) > 0 { - fmt.Fprintf(errOut.ErrOut, msg.AlreadyGrantedScopes, formatScopeList(summary.AlreadyGranted, msg.NoScopes)) + fmt.Fprintf(streams.ErrOut, msg.AlreadyGrantedScopes, formatScopeList(summary.AlreadyGranted, msg.NoScopes)) } if len(summary.Missing) > 0 { - fmt.Fprintf(errOut.ErrOut, msg.MissingScopes, formatScopeList(summary.Missing, msg.NoScopes)) + fmt.Fprintf(streams.ErrOut, msg.MissingScopes, formatScopeList(summary.Missing, msg.NoScopes)) } - fmt.Fprintf(errOut.ErrOut, msg.FinalGrantedScopes, formatScopeList(summary.Granted, msg.NoScopes)) + fmt.Fprintf(streams.ErrOut, msg.FinalGrantedScopes, formatScopeList(summary.Granted, msg.NoScopes)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login_result.go` around lines 136 - 151, The parameter name errOut in function writeLoginScopeBreakdown should be renamed to a clearer identifier like streams or ios; update the function signature (writeLoginScopeBreakdown(streams *cmdutil.IOStreams, msg *loginMsg, summary *loginScopeSummary)), replace all uses of errOut.ErrOut inside the function with streams.ErrOut (or ios.ErrOut), and update every call site invoking writeLoginScopeBreakdown to pass the same variable under the new name; ensure you update imports/usages consistently so compilation passes.
159-163: Consider logging or handling json.Marshal error.The error from
json.Marshalis discarded. While this is unlikely to fail for the basic types inauthorizationCompletePayload, silently ignoring errors can mask unexpected issues. At minimum, consider a debug log.✏️ Optional: handle marshal error
if opts.JSON { - b, _ := json.Marshal(authorizationCompletePayload(openId, userName, summary, nil)) + b, err := json.Marshal(authorizationCompletePayload(openId, userName, summary, nil)) + if err != nil { + // Should not happen with basic types, but log for debugging + fmt.Fprintf(f.IOStreams.ErrOut, "warning: failed to marshal JSON: %v\n", err) + } fmt.Fprintln(f.IOStreams.Out, string(b)) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login_result.go` around lines 159 - 163, The json.Marshal error returned in the opts.JSON branch is being ignored; update the block around json.Marshal(authorizationCompletePayload(...)) in login_result.go to handle the error (e.g., check err after calling json.Marshal and log or print a diagnostic via the existing logger/IOStreams before returning). Specifically, in the opts.JSON branch that currently uses b, _ := json.Marshal(...), replace the discard with err handling: capture (b, err), if err != nil write a clear error or debug message to f.IOStreams.Err (or the module logger) and return a non-success path, otherwise print the marshalled JSON to f.IOStreams.Out as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/auth/login_result.go`:
- Around line 136-151: The parameter name errOut in function
writeLoginScopeBreakdown should be renamed to a clearer identifier like streams
or ios; update the function signature (writeLoginScopeBreakdown(streams
*cmdutil.IOStreams, msg *loginMsg, summary *loginScopeSummary)), replace all
uses of errOut.ErrOut inside the function with streams.ErrOut (or ios.ErrOut),
and update every call site invoking writeLoginScopeBreakdown to pass the same
variable under the new name; ensure you update imports/usages consistently so
compilation passes.
- Around line 159-163: The json.Marshal error returned in the opts.JSON branch
is being ignored; update the block around
json.Marshal(authorizationCompletePayload(...)) in login_result.go to handle the
error (e.g., check err after calling json.Marshal and log or print a diagnostic
via the existing logger/IOStreams before returning). Specifically, in the
opts.JSON branch that currently uses b, _ := json.Marshal(...), replace the
discard with err handling: capture (b, err), if err != nil write a clear error
or debug message to f.IOStreams.Err (or the module logger) and return a
non-success path, otherwise print the marshalled JSON to f.IOStreams.Out as
before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8924beae-f97d-49fa-aa35-b52140e0a142
📒 Files selected for processing (3)
cmd/auth/login_messages.gocmd/auth/login_result.gocmd/auth/login_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/auth/login_messages.go
- cmd/auth/login_test.go
Move device token polling to a package-level variable to enable mocking in tests Add test case for scope cleanup when token is nil
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/auth/login.go (1)
281-294:⚠️ Potential issue | 🟠 MajorKeep
--jsonfailure results machine-readable in both polling paths.The direct flow already emits a JSON
authorization_failedenvelope for normal poll failures, but the new nil-token guard and the resumed--device-codefailure path return plain stderr errors.--jsoncallers lose their final structured result in those branches.📦 Suggested shape
if !result.OK { + if opts.JSON { + b, _ := json.Marshal(map[string]interface{}{ + "event": "authorization_failed", + "error": result.Message, + }) + fmt.Fprintln(f.IOStreams.Out, string(b)) + return output.ErrBare(output.ExitAuth) + } ... } if result.Token == nil { + if opts.JSON { + b, _ := json.Marshal(map[string]interface{}{ + "event": "authorization_failed", + "error": "authorization succeeded but no token returned", + }) + fmt.Fprintln(f.IOStreams.Out, string(b)) + return output.ErrBare(output.ExitAuth) + } return output.ErrAuth("authorization succeeded but no token returned") }As per coding guidelines:
**/*.go: Program output (JSON envelopes) must go to stdout; progress, warnings, and hints must go to stderr.Also applies to: 361-370
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 281 - 294, The JSON output path for --json is missing in the nil-token guard and the resumed device-code failure branch, so when opts.JSON is true those branches emit plain stderr errors; update the branches that check result.Token == nil and the resumed device-code failure path (the other poll failure at lines ~361-370) to emit the same machine-readable JSON envelope (e.g., {"event":"authorization_failed","error": ...}) to stdout when opts.JSON is true, and return the appropriate output.ErrBare(output.ExitAuth) afterward; keep progress/warnings on stderr but ensure all program JSON envelopes are printed to f.IOStreams.Out and use the same fields as the existing authorization_failed branch that checks result.OK.
♻️ Duplicate comments (1)
cmd/auth/login_test.go (1)
784-851:⚠️ Potential issue | 🟠 MajorIsolate config state in these auth-flow tests.
Both tests call
authLoginRunwithoutsetupLoginConfigDir(t)/t.Setenv, so they can still read or write real CLI config and the on-disk login-scope cache. The other new auth-flow tests in this file already isolate that state.🔒 Minimal fix
func TestAuthLoginRun_JSONWriteFailure_NoWaitIgnoresWriterError(t *testing.T) { + setupLoginConfigDir(t) f, _, _, reg := cmdutil.TestFactory(t, &core.CliConfig{ ProfileName: "default", @@ func TestAuthLoginRun_JSONWriteFailure_DeviceAuthorizationStillPolls(t *testing.T) { + setupLoginConfigDir(t) f, _, _, reg := cmdutil.TestFactory(t, &core.CliConfig{ ProfileName: "default",As per coding guidelines:
**/*_test.go: Isolate config state in Go tests by usingt.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login_test.go` around lines 784 - 851, These two tests (TestAuthLoginRun_JSONWriteFailure_NoWaitIgnoresWriterError and TestAuthLoginRun_JSONWriteFailure_DeviceAuthorizationStillPolls) call authLoginRun without isolating CLI config state; fix by invoking setupLoginConfigDir(t) (or directly t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())) at the start of each test before creating the Factory/calling authLoginRun so they use an isolated config dir and do not read/write the real on-disk login-scope cache.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/auth/login_test.go`:
- Around line 784-816: The test currently expects nil even if JSON output to
f.IOStreams.Out (failWriter) fails; update authLoginRun so that when emitting
JSON (JSON: true) — including the NoWait true path — it checks the error
returned by the JSON writer/encoder and returns that error instead of swallowing
it; locate the JSON emission logic in authLoginRun (where
LoginOptions.Factory.IOStreams.Out is written to, likely via
json.NewEncoder(...).Encode or fmt.Fprintln) and change it to capture and return
the writer error so callers receive the failure (the test should then assert an
error is returned when using failWriter).
In `@cmd/auth/login.go`:
- Around line 362-367: The early-return error path that checks
shouldRemoveLoginRequestedScope(result) lets many terminal failures (e.g.,
invalid_request, invalid_client) leave the device-code cache behind; change the
error handling so cleanupRequestedScope() is invoked for all terminal,
non-resumable failures before returning. Concretely, in the block that currently
does if shouldRemoveLoginRequestedScope(result) { cleanupRequestedScope() }
return output.ErrAuth(...), call cleanupRequestedScope() unconditionally (or
broaden the condition to include invalid_request/invalid_client and other
terminal error codes) so cleanupRequestedScope() runs whenever you return an
auth error from that handler.
- Around line 240-242: The write to the requested-scope cache must be treated as
mandatory when running with --no-wait: instead of logging a warning on
saveLoginRequestedScope(authResp.DeviceCode, finalScope) failure, detect the
no-wait option (e.g., opts.NoWait or the local noWait flag used in the login
flow) and return/propagate an error (or exit non-zero) so the login fails fast;
keep the warning-only behavior for normal interactive flows. Update the block
around saveLoginRequestedScope(authResp.DeviceCode, finalScope) to check the
no-wait flag and if true, surface the save error (return it from the enclosing
function or call the appropriate failure path) so ensureRequestedScopesGranted
in the resumed device-code path will have the cache available.
---
Outside diff comments:
In `@cmd/auth/login.go`:
- Around line 281-294: The JSON output path for --json is missing in the
nil-token guard and the resumed device-code failure branch, so when opts.JSON is
true those branches emit plain stderr errors; update the branches that check
result.Token == nil and the resumed device-code failure path (the other poll
failure at lines ~361-370) to emit the same machine-readable JSON envelope
(e.g., {"event":"authorization_failed","error": ...}) to stdout when opts.JSON
is true, and return the appropriate output.ErrBare(output.ExitAuth) afterward;
keep progress/warnings on stderr but ensure all program JSON envelopes are
printed to f.IOStreams.Out and use the same fields as the existing
authorization_failed branch that checks result.OK.
---
Duplicate comments:
In `@cmd/auth/login_test.go`:
- Around line 784-851: These two tests
(TestAuthLoginRun_JSONWriteFailure_NoWaitIgnoresWriterError and
TestAuthLoginRun_JSONWriteFailure_DeviceAuthorizationStillPolls) call
authLoginRun without isolating CLI config state; fix by invoking
setupLoginConfigDir(t) (or directly t.Setenv("LARKSUITE_CLI_CONFIG_DIR",
t.TempDir())) at the start of each test before creating the Factory/calling
authLoginRun so they use an isolated config dir and do not read/write the real
on-disk login-scope cache.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ae440c00-964b-4dba-a7e7-49112cadbe72
📒 Files selected for processing (2)
cmd/auth/login.gocmd/auth/login_test.go
Previously, JSON write errors were only logged to stderr but not returned, causing tests to pass when they should fail. Now properly propagate these errors to callers and update tests to verify error handling.
There was a problem hiding this comment.
♻️ Duplicate comments (4)
cmd/auth/login_test.go (2)
821-857:⚠️ Potential issue | 🟡 MinorIsolate config state in this test.
This test invokes
authLoginRunbut doesn't callsetupLoginConfigDir(t), so it can accidentally read/write non-test config state.Suggested fix
func TestAuthLoginRun_JSONWriteFailure_DeviceAuthorizationReturnsWriterError(t *testing.T) { + setupLoginConfigDir(t) f, _, _, reg := cmdutil.TestFactory(t, &core.CliConfig{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login_test.go` around lines 821 - 857, The test TestAuthLoginRun_JSONWriteFailure_DeviceAuthorizationReturnsWriterError currently can touch real user config because it doesn't isolate config state; call setupLoginConfigDir(t) at the start of the test (before creating the TestFactory or invoking authLoginRun) so the test uses a temporary config directory, then proceed with TestFactory, reg stubbing and the authLoginRun call; this ensures authLoginRun and TestFactory operate on isolated config state and prevents accidental reads/writes to non-test config.
784-819:⚠️ Potential issue | 🟡 MinorIsolate config state in this test.
This test invokes
authLoginRunbut doesn't callsetupLoginConfigDir(t), so it can accidentally read/write non-test config state.Suggested fix
func TestAuthLoginRun_JSONWriteFailure_NoWaitReturnsWriterError(t *testing.T) { + setupLoginConfigDir(t) f, _, _, reg := cmdutil.TestFactory(t, &core.CliConfig{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login_test.go` around lines 784 - 819, The test TestAuthLoginRun_JSONWriteFailure_NoWaitReturnsWriterError can leak or reuse real config because it doesn't isolate config state; call setupLoginConfigDir(t) at the start of this test (before creating the Factory via cmdutil.TestFactory and before calling authLoginRun) to ensure a temp test config dir is used and cleaned up; update the test to invoke setupLoginConfigDir(t) and then proceed with f, reg := cmdutil.TestFactory(...) and the existing assertions so authLoginRun and LoginOptions operate against isolated config state.cmd/auth/login.go (2)
240-242:⚠️ Potential issue | 🟠 MajorConsider making the scope cache write mandatory for
--no-wait.If this write fails, the subsequent
--device-codelogin won't have the requested scopes available, silently disabling the scope validation feature.Suggested fix
if opts.NoWait { - if err := saveLoginRequestedScope(authResp.DeviceCode, finalScope); err != nil { - fmt.Fprintf(f.IOStreams.ErrOut, "[lark-cli] [WARN] auth login: failed to cache requested scopes: %v\n", err) - } + if err := saveLoginRequestedScope(authResp.DeviceCode, finalScope); err != nil { + return output.Errorf( + output.ExitInternal, + "scope_cache_write_failed", + "failed to cache requested scopes for the follow-up --device-code login: %v", + err, + ) + }
364-368:⚠️ Potential issue | 🟡 MinorConsider cleaning up the scope cache for all terminal poll failures.
Currently,
shouldRemoveLoginRequestedScopeonly removes the cache forOK,access_denied, and mostexpired_tokenresults. Terminal failures likeinvalid_requestorinvalid_clientwill leave orphaned cache entries.Suggested fix
if !result.OK { - if shouldRemoveLoginRequestedScope(result) { - cleanupRequestedScope() - } + cleanupRequestedScope() // Clean up for all terminal failures return output.ErrAuth("authorization failed: %s", result.Message) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login.go` around lines 364 - 368, When handling a non-OK poll result in the login flow, ensure the login-requested scope cache is removed for all terminal failures (not just OK/access_denied/expired_token). Update the logic around the `if !result.OK` block so that `cleanupRequestedScope()` is invoked for all terminal failure outcomes (either by extending `shouldRemoveLoginRequestedScope` to include `invalid_request`, `invalid_client`, etc., or by calling `cleanupRequestedScope()` unconditionally on `!result.OK` before returning). Keep the existing `output.ErrAuth("authorization failed: %s", result.Message)` return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/auth/login_test.go`:
- Around line 821-857: The test
TestAuthLoginRun_JSONWriteFailure_DeviceAuthorizationReturnsWriterError
currently can touch real user config because it doesn't isolate config state;
call setupLoginConfigDir(t) at the start of the test (before creating the
TestFactory or invoking authLoginRun) so the test uses a temporary config
directory, then proceed with TestFactory, reg stubbing and the authLoginRun
call; this ensures authLoginRun and TestFactory operate on isolated config state
and prevents accidental reads/writes to non-test config.
- Around line 784-819: The test
TestAuthLoginRun_JSONWriteFailure_NoWaitReturnsWriterError can leak or reuse
real config because it doesn't isolate config state; call setupLoginConfigDir(t)
at the start of this test (before creating the Factory via cmdutil.TestFactory
and before calling authLoginRun) to ensure a temp test config dir is used and
cleaned up; update the test to invoke setupLoginConfigDir(t) and then proceed
with f, reg := cmdutil.TestFactory(...) and the existing assertions so
authLoginRun and LoginOptions operate against isolated config state.
In `@cmd/auth/login.go`:
- Around line 364-368: When handling a non-OK poll result in the login flow,
ensure the login-requested scope cache is removed for all terminal failures (not
just OK/access_denied/expired_token). Update the logic around the `if
!result.OK` block so that `cleanupRequestedScope()` is invoked for all terminal
failure outcomes (either by extending `shouldRemoveLoginRequestedScope` to
include `invalid_request`, `invalid_client`, etc., or by calling
`cleanupRequestedScope()` unconditionally on `!result.OK` before returning).
Keep the existing `output.ErrAuth("authorization failed: %s", result.Message)`
return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c0b707d5-41bc-4ad5-b86f-9e0b73e4d1dd
📒 Files selected for processing (2)
cmd/auth/login.gocmd/auth/login_test.go
JackZhao10086
force-pushed
the
feat/optimize_login_scope_print
branch
from
71d889f to
4ab384a
Compare
remove redundant scope display and consolidate hint messages to focus on actionable guidance
remove ShortHint field and simplify scope hint messages always display missing scopes section with consistent formatting add StatusHint for successful login with no missing scopes update tests to reflect new message structure and content
liangshuo-1
approved these changes
Apr 8, 2026
This was referenced Apr 11, 2026
This was referenced Apr 21, 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.
Summary
Improve scope handling and login result output in
auth login.This change validates whether requested scopes are actually granted after authorization and provides a clearer scope breakdown in both text and JSON output for users and AI agents.
Changes
Test Plan
go test ./cmd/authlark auth loginlocally, including successful authorization, missing-scope cases, and JSON outputRelated Issues
Summary by CodeRabbit
Bug Fixes
New Features
Tests