feat(auth): improve login scope handling and messages#523
feat(auth): improve login scope handling and messages#523JackZhao10086 merged 3 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded an AuthorizedUser field to login messages; revised localized login and scope-mismatch strings; changed non-JSON scope-issue printing to emit a single issue message and optionally the authorized-account line when present; updated tests to match revised outputs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 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 |
github-actions
bot
added
the
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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_test.go (1)
621-639:⚠️ Potential issue | 🟠 MajorTest expectation inconsistent with updated
ScopeMismatchmessage - test will fail.Line 623 expects the old message
"授权完成，但以下请求 scopes 未被授予: im:message:send", butlogin_messages.goline 65 changedScopeMismatchto"授权结果异常：以下请求 scopes 未被授予: %s". This test calls the fullauthLoginRunflow which usesensureRequestedScopesGranted, which formatsmsg.ScopeMismatchwith the missing scopes.🐛 Proposed fix to align test expectation with new message
for _, want := range []string{ "OK: 登录成功! 用户: tester (ou_user)", - "授权完成，但以下请求 scopes 未被授予: im:message:send", + "授权结果异常：以下请求 scopes 未被授予: im:message:send", "本次请求 scopes: im:message:send", "本次未授予 scopes: im:message:send", "以上结果是本次授权请求用户最终确认后的结果，请勿持续重试",🤖 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 621 - 639, Update the test expectations to match the new ScopeMismatch message format used by ensureRequestedScopesGranted/authLoginRun: replace the old literal "授权完成，但以下请求 scopes 未被授予: im:message:send" with the updated formatted message "授权结果异常：以下请求 scopes 未被授予: im:message:send" (i.e., match login_messages.go's ScopeMismatch string); ensure other expected lines still appear in the output and keep the negative assertions for "最终已授权 scopes:" and "ERROR:" unchanged.
🧹 Nitpick comments (1)
cmd/auth/login_result.go (1)
194-196: Simplify nested format calls.The nested
fmt.Sprintfinsidefmt.Fprintfis redundant. You can format directly withFprintf.♻️ Suggested simplification
if msg.AuthorizedUser != "" { - fmt.Fprintf(f.IOStreams.ErrOut, "%s\n", fmt.Sprintf(msg.AuthorizedUser, userName, openId)) + fmt.Fprintf(f.IOStreams.ErrOut, msg.AuthorizedUser+"\n", userName, openId) }🤖 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 194 - 196, The code currently calls fmt.Fprintf(f.IOStreams.ErrOut, "%s\n", fmt.Sprintf(msg.AuthorizedUser, userName, openId)), which nests fmt.Sprintf unnecessarily; replace the nested call by writing directly to the error stream using fmt.Fprintf(f.IOStreams.ErrOut, msg.AuthorizedUser+"\n", userName, openId) (keeping the existing check for msg.AuthorizedUser != "" and the same arguments userName and openId) so the formatting is done in a single call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/auth/login_test.go`:
- Around line 621-639: Update the test expectations to match the new
ScopeMismatch message format used by ensureRequestedScopesGranted/authLoginRun:
replace the old literal "授权完成，但以下请求 scopes 未被授予: im:message:send" with the
updated formatted message "授权结果异常：以下请求 scopes 未被授予: im:message:send" (i.e.,
match login_messages.go's ScopeMismatch string); ensure other expected lines
still appear in the output and keep the negative assertions for "最终已授权 scopes:"
and "ERROR:" unchanged.
---
Nitpick comments:
In `@cmd/auth/login_result.go`:
- Around line 194-196: The code currently calls fmt.Fprintf(f.IOStreams.ErrOut,
"%s\n", fmt.Sprintf(msg.AuthorizedUser, userName, openId)), which nests
fmt.Sprintf unnecessarily; replace the nested call by writing directly to the
error stream using fmt.Fprintf(f.IOStreams.ErrOut, msg.AuthorizedUser+"\n",
userName, openId) (keeping the existing check for msg.AuthorizedUser != "" and
the same arguments userName and openId) so the formatting is done in a single
call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a1e7d61-811c-428e-ab38-0ef5bc71f6cd
📒 Files selected for processing (4)
cmd/auth/login_messages.gocmd/auth/login_messages_test.gocmd/auth/login_result.gocmd/auth/login_test.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@d424367394803da577f8828af48f0894efc80ef8🧩 Skill updatenpx skills add larksuite/cli#feat/login_response_content_opt -y -g |
- Add AuthorizedUser message to display current authorized account - Update scope mismatch message wording to be more accurate - Reorganize login success output to show scope issues first - Remove redundant success message when scope issues exist
…orization" Update both Chinese and English login success messages to use "authorization" instead of "login" for consistency with the authentication flow. Also update corresponding test cases to match the new wording.
JackZhao10086
force-pushed
the
feat/login_response_content_opt
branch
from
4ff6a6f to
2e04c14
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/auth/login_messages.go (1)
98-101: Nit: EnglishScopeMismatchphrasing reads a bit awkward.
"authorization result is abnormal: these requested scopes were not granted: %s"is understandable but stilted. Since every error message is parsed by humans and AI agents per coding guidelines, a slightly tighter phrasing reads better, e.g.:✏️ Suggested wording
- ScopeMismatch: "authorization result is abnormal: these requested scopes were not granted: %s", + ScopeMismatch: "abnormal authorization result: the following requested scopes were not granted: %s",Feel free to ignore if the current wording is intentional.
As per coding guidelines: "Design CLI flags, help text, and error messages with AI agent consumption in mind; every error message will be parsed by AI agents".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/auth/login_messages.go` around lines 98 - 101, The ScopeMismatch message is worded awkwardly; update the ScopeMismatch constant in the login_messages.go diff to a tighter, clearer sentence such as "The following requested scopes were not granted: %s" (or similar concise wording) so it reads naturally for humans and AI agents; change the value for the ScopeMismatch key to the new phrasing in the AuthSuccess/LoginSuccess/AuthorizedUser block.cmd/auth/login_result.go (1)
191-199: Minor refactor: de-duplicate theissue.Messageprint and drop the double-format.Both branches of the
if loginSucceeded { … } else { … }printissue.Messageidentically, so the branch only gates the extraAuthorizedUserline. You can hoist the common print. Additionally, line 195 wraps afmt.Sprintfinside afmt.Fprintf("%s\n", …), which is an unnecessary double format pass —Fprintlnor a singleFprintfhandles it directly.♻️ Suggested simplification
fmt.Fprintln(f.IOStreams.ErrOut) - if loginSucceeded { - fmt.Fprintln(f.IOStreams.ErrOut, issue.Message) - if msg.AuthorizedUser != "" { - fmt.Fprintf(f.IOStreams.ErrOut, "%s\n", fmt.Sprintf(msg.AuthorizedUser, userName, openId)) - } - } else { - fmt.Fprintln(f.IOStreams.ErrOut, issue.Message) - } + fmt.Fprintln(f.IOStreams.ErrOut, issue.Message) + if loginSucceeded && msg.AuthorizedUser != "" { + fmt.Fprintln(f.IOStreams.ErrOut, fmt.Sprintf(msg.AuthorizedUser, userName, openId)) + }Note: since both
loginMsgZhandloginMsgEnalways initializeAuthorizedUser, the!= ""guard is effectively defensive — fine to keep, just flagging for awareness.🤖 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 191 - 199, Hoist the common fmt.Fprintln(f.IOStreams.ErrOut, issue.Message) out of the if/else so it is printed once regardless of loginSucceeded, then only conditionally emit the extra AuthorizedUser line when loginSucceeded and msg.AuthorizedUser != ""; replace the double-format pattern fmt.Fprintf(f.IOStreams.ErrOut, "%s\n", fmt.Sprintf(msg.AuthorizedUser, userName, openId)) with a single formatting call such as fmt.Fprintf(f.IOStreams.ErrOut, msg.AuthorizedUser+"\n", userName, openId) (or fmt.Fprintln with a single fmt.Sprintf) so you remove the unnecessary nested formatting and keep the same outputs for f.IOStreams.ErrOut, loginSucceeded, issue.Message, msg.AuthorizedUser, userName and openId.
🤖 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_messages.go`:
- Around line 98-101: The ScopeMismatch message is worded awkwardly; update the
ScopeMismatch constant in the login_messages.go diff to a tighter, clearer
sentence such as "The following requested scopes were not granted: %s" (or
similar concise wording) so it reads naturally for humans and AI agents; change
the value for the ScopeMismatch key to the new phrasing in the
AuthSuccess/LoginSuccess/AuthorizedUser block.
In `@cmd/auth/login_result.go`:
- Around line 191-199: Hoist the common fmt.Fprintln(f.IOStreams.ErrOut,
issue.Message) out of the if/else so it is printed once regardless of
loginSucceeded, then only conditionally emit the extra AuthorizedUser line when
loginSucceeded and msg.AuthorizedUser != ""; replace the double-format pattern
fmt.Fprintf(f.IOStreams.ErrOut, "%s\n", fmt.Sprintf(msg.AuthorizedUser,
userName, openId)) with a single formatting call such as
fmt.Fprintf(f.IOStreams.ErrOut, msg.AuthorizedUser+"\n", userName, openId) (or
fmt.Fprintln with a single fmt.Sprintf) so you remove the unnecessary nested
formatting and keep the same outputs for f.IOStreams.ErrOut, loginSucceeded,
issue.Message, msg.AuthorizedUser, userName and openId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9d6d3f97-2ce3-433d-afed-9beee74cf505
📒 Files selected for processing (4)
cmd/auth/login_messages.gocmd/auth/login_messages_test.gocmd/auth/login_result.gocmd/auth/login_test.go
✅ Files skipped from review due to trivial changes (1)
- cmd/auth/login_messages_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/auth/login_test.go
Update test assertions to verify correct error messages when requested scopes are not granted. Remove checks for success message in this scenario.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #523 +/- ##
=======================================
Coverage 59.05% 59.05%
=======================================
Files 384 384
Lines 32636 32636
=======================================
Hits 19274 19274
- Misses 11553 11554 +1
+ Partials 1809 1808 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Summary
Improve the login experience by making scope-related authorization results clearer and easier to interpret.
This change adds the authorized account display, refines scope mismatch wording, and reorganizes output to better reflect partially successful authorization flows.
Changes
Test Plan
lark auth logincommands work locallyRelated Issues
Summary by CodeRabbit