feat: add --json flag support to auth subcommands - #1431
Conversation
📝 WalkthroughWalkthroughAdds a ChangesJSON Flag Support for Auth Commands
🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
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 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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1431 +/- ##
==========================================
+ Coverage 72.83% 72.86% +0.03%
==========================================
Files 732 732
Lines 69140 69181 +41
==========================================
+ Hits 50356 50411 +55
+ Misses 15003 14987 -16
- Partials 3781 3783 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@f0b42920c07b18cdc034fe26641896a9adfbc5b1🧩 Skill updatenpx skills add larksuite/cli#feat/auth_cmd_json -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/logout_test.go (1)
1-147: ⚡ Quick winConsider adding error-path test for
SaveMultiAppConfigfailure.The logout implementation has an error return path when
core.SaveMultiAppConfig(multi)fails (seecmd/auth/logout.goLine 88), but there's no test exercising this case. As per coding guidelines, error-path tests should assert typed metadata (category,subtype,cause) viaerrs.ProblemOf.💡 Example error-path test structure
func TestAuthLogoutRun_SaveConfigError_ReturnsTypedError(t *testing.T) { // Setup: create a scenario where SaveMultiAppConfig will fail // (e.g., read-only filesystem, permission error, etc.) // Assert the error is typed: // - Use errs.ProblemOf to check category=internal, subtype=storage // - Verify .WithCause(err) preserved the underlying error }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/auth/logout_test.go` around lines 1 - 147, Add a test that forces core.SaveMultiAppConfig to fail and asserts authLogoutRun returns a typed error: arrange the environment (set LARKSUITE_CLI_CONFIG_DIR to a temp dir), make SaveMultiAppConfig fail by making the config path unwritable (e.g., create the config dir and set its permissions readonly or create a file that blocks writing), call authLogoutRun(&LogoutOptions{Factory: f, JSON: true}) and capture the error, then use errs.ProblemOf(err) to assert Category == "internal", Subtype == "storage" and that Problem.Cause wraps the underlying filesystem error (preserved via .WithCause); reference the functions core.SaveMultiAppConfig, authLogoutRun, LogoutOptions and errs.ProblemOf when locating code to change or tests to add.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/auth/logout_test.go`:
- Around line 1-147: Add a test that forces core.SaveMultiAppConfig to fail and
asserts authLogoutRun returns a typed error: arrange the environment (set
LARKSUITE_CLI_CONFIG_DIR to a temp dir), make SaveMultiAppConfig fail by making
the config path unwritable (e.g., create the config dir and set its permissions
readonly or create a file that blocks writing), call
authLogoutRun(&LogoutOptions{Factory: f, JSON: true}) and capture the error,
then use errs.ProblemOf(err) to assert Category == "internal", Subtype ==
"storage" and that Problem.Cause wraps the underlying filesystem error
(preserved via .WithCause); reference the functions core.SaveMultiAppConfig,
authLogoutRun, LogoutOptions and errs.ProblemOf when locating code to change or
tests to add.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7f1132a-b646-4c0c-b0a0-da13f332932f
📒 Files selected for processing (2)
cmd/auth/logout.gocmd/auth/logout_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/auth/list.go (1)
49-81: ⚖️ Poor tradeoffConsider unifying the response format in a future breaking-change release.
When
--jsonis used with empty states, the output is an object withok,users, andreasonfields. However, when users exist (line 100), the output is a bare array. Clients must detect and handle both shapes:
- Empty:
{"ok": true, "users": [], "reason": "not_configured"}- Populated:
[{...}, {...}]A uniform response shape would simplify client parsing. For example, always wrapping in
{"ok": true, "users": [...]}with an optionalreasonfield when empty.However, changing line 100 now would break existing callers, and the PR explicitly preserves backward compatibility. Consider documenting this inconsistency and addressing it in a coordinated breaking-change release.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/auth/list.go` around lines 49 - 81, The JSON output shape is inconsistent: empty states use output.PrintJson with {"ok", "users", "reason"} while the populated branch (where users exist, invoked after multi.CurrentAppConfig) returns a bare array; to fix, change the populated branch to emit the same wrapped shape (call output.PrintJson(f.IOStreams.Out, map[string]interface{}{"ok": true, "users": users}) and include "reason" only when appropriate), update any tests/examples that expect the bare array, and add a short note in CLI docs; touch the code paths around opts.JSON handling and the populated-users response in list.go so both empty and non-empty branches use the unified response object.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/auth/list.go`:
- Around line 49-81: The JSON output shape is inconsistent: empty states use
output.PrintJson with {"ok", "users", "reason"} while the populated branch
(where users exist, invoked after multi.CurrentAppConfig) returns a bare array;
to fix, change the populated branch to emit the same wrapped shape (call
output.PrintJson(f.IOStreams.Out, map[string]interface{}{"ok": true, "users":
users}) and include "reason" only when appropriate), update any tests/examples
that expect the bare array, and add a short note in CLI docs; touch the code
paths around opts.JSON handling and the populated-users response in list.go so
both empty and non-empty branches use the unified response object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c3a70fd-f18f-4cc5-869f-4698e5be24ff
📒 Files selected for processing (2)
cmd/auth/list.gocmd/auth/list_test.go
2 participants
Summary
This PR makes
authsubcommands more consistent for machine-readable usage by adding--jsoncompatibility where it previously causedunknown flagerrors. It keeps existing output contracts unchanged forstatus,list,check, andlogout, while makingauth scopes --jsonexplicitly map to JSON output.Changes
--jsonflags toauth status,auth list,auth check, andauth logoutwithout changing their existing output behavior--jsonsupport toauth scopesand map it to--format json--jsoncompatibility behavior across the updatedauthsubcommandsTest Plan
lark-cli <domain> <command>flow works as expectedRelated Issues
Summary by CodeRabbit
New Features
Bug Fixes / Behavior
Tests