feat(apps): add role management shortcuts#1881
Open
linchao5102 wants to merge 3 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds nine Apps CLI shortcuts for role CRUD, role-member management, and role matching, with shared validation, pagination, normalization, rendering, documentation, unit tests, registry tests, and fixture-gated E2E workflows. Department identifier fixtures now use dash syntax. ChangesApps role management
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@e1f0bdd06a8e6d296b53cd19de260dba72928397🧩 Skill updatenpx skills add larksuite/cli#feat/apps-role-management-framework -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@shortcuts/apps/apps_role_common.go`:
- Around line 275-278: Remove the single-flag param metadata from the multi-flag
validation errors: in shortcuts/apps/apps_role_common.go lines 275-278, update
the validation error for the total users/departments/chats check; in
shortcuts/apps/apps_role.go lines 193-195, update the validation error for the
alternative update fields; and in shortcuts/apps/apps_role_member.go lines
370-372, update the validation error for member flags or --all. Each error
should omit param while preserving its message and hint.
In `@shortcuts/apps/apps_role_test.go`:
- Around line 898-907: Extend the error assertions after AppsRoleList.Execute in
shortcuts/apps/apps_role_test.go (898-907) to verify the typed problem’s
category and subtype in addition to roleAppHint. Apply the same category and
subtype assertions after role-match hint decoration in
shortcuts/apps/apps_role_member_test.go (700-709), preserving the existing hint
checks.
In `@shortcuts/apps/apps_role.go`:
- Around line 431-448: Make role-list parsing fail closed in parseRoleListPage
by validating every item and requiring a usable role_id or id when processing
the unfiltered path; return an invalid-response error for malformed entries
rather than accepting them. In shortcuts/apps/apps_role_member.go lines 432-443,
reject present non-array roles fields and malformed role entries instead of
treating them as no matches. Add regression tests covering each malformed
response shape in both affected files.
In `@tests/cli_e2e/apps/coverage.md`:
- Line 46: Update the role-member-list row in the coverage table so the
member_type alternatives use the existing slash-separated convention instead of
literal pipe characters, preserving the meaning while keeping the row at the
expected column count.
🪄 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: 1cb84bc1-0ca8-461e-a529-4fbebb09e76d
📒 Files selected for processing (14)
shortcuts/apps/apps_access_scope_get_test.goshortcuts/apps/apps_role.goshortcuts/apps/apps_role_common.goshortcuts/apps/apps_role_common_test.goshortcuts/apps/apps_role_member.goshortcuts/apps/apps_role_member_test.goshortcuts/apps/apps_role_test.goshortcuts/apps/shortcuts.goshortcuts/apps/shortcuts_test.goskills/lark-apps/SKILL.mdskills/lark-apps/references/lark-apps-access-scope-set.mdskills/lark-apps/references/lark-apps-role.mdtests/cli_e2e/apps/apps_role_management_test.gotests/cli_e2e/apps/coverage.md
Comment on lines
+275
to
+278
| total := len(groups.Users) + len(groups.Departments) + len(groups.Chats) | ||
| if total == 0 { | ||
| return groups, appsValidationParamError("--users", "at least one of --users, --departments, or --chats is required"). | ||
| WithHint("resolve names to IDs first, then pass --users open_id, --departments open_department_id, or --chats open_chat_id") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Avoid misleading single-flag metadata for multi-flag constraints.
shortcuts/apps/apps_role_common.go#L275-L278: omitparam; any member flag can satisfy the constraint.shortcuts/apps/apps_role.go#L193-L195: omitparam; either update field is valid.shortcuts/apps/apps_role_member.go#L370-L372: omitparam; member flags or--allare valid alternatives.
As per coding guidelines, the param field must only name the user input that actually failed.
📍 Affects 3 files
shortcuts/apps/apps_role_common.go#L275-L278(this comment)shortcuts/apps/apps_role.go#L193-L195shortcuts/apps/apps_role_member.go#L370-L372
🤖 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 `@shortcuts/apps/apps_role_common.go` around lines 275 - 278, Remove the
single-flag param metadata from the multi-flag validation errors: in
shortcuts/apps/apps_role_common.go lines 275-278, update the validation error
for the total users/departments/chats check; in shortcuts/apps/apps_role.go
lines 193-195, update the validation error for the alternative update fields;
and in shortcuts/apps/apps_role_member.go lines 370-372, update the validation
error for member flags or --all. Each error should omit param while preserving
its message and hint.
Source: Coding guidelines
Comment on lines
+431
to
+448
| func parseRoleListPage(data map[string]interface{}) ([]interface{}, bool, error) { | ||
| rawItems, hasItems := data["items"] | ||
| items, ok := rawItems.([]interface{}) | ||
| if !hasItems || !ok { | ||
| return nil, false, errs.NewInternalError( | ||
| errs.SubtypeInvalidResponse, | ||
| "role list response field items must be an array", | ||
| ).WithHint("retry the read; do not treat a missing or malformed role list as empty") | ||
| } | ||
| rawHasMore, hasHasMore := data["has_more"] | ||
| hasMore, ok := rawHasMore.(bool) | ||
| if !hasHasMore || !ok { | ||
| return nil, false, errs.NewInternalError( | ||
| errs.SubtypeInvalidResponse, | ||
| "role list response field has_more must be a boolean", | ||
| ).WithHint("retry the read; pagination is incomplete without a valid has_more value") | ||
| } | ||
| return items, hasMore, nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail closed on malformed backend role collections.
shortcuts/apps/apps_role.go#L431-L448: validate every role item and require a usablerole_id/idon the unfiltered path.shortcuts/apps/apps_role_member.go#L432-L443: reject present non-arrayrolesfields and malformed role entries instead of normalizing them to no matches.
Add regression tests for each malformed shape. As per coding guidelines, every behavior change needs a test alongside it.
📍 Affects 2 files
shortcuts/apps/apps_role.go#L431-L448(this comment)shortcuts/apps/apps_role_member.go#L432-L443
🤖 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 `@shortcuts/apps/apps_role.go` around lines 431 - 448, Make role-list parsing
fail closed in parseRoleListPage by validating every item and requiring a usable
role_id or id when processing the unfiltered path; return an invalid-response
error for malformed entries rather than accepting them. In
shortcuts/apps/apps_role_member.go lines 432-443, reject present non-array roles
fields and malformed role entries instead of treating them as no matches. Add
regression tests covering each malformed response shape in both affected files.
Source: Coding guidelines
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1881 +/- ##
==========================================
+ Coverage 74.58% 74.78% +0.19%
==========================================
Files 863 881 +18
Lines 90123 92690 +2567
==========================================
+ Hits 67220 69318 +2098
- Misses 17696 18018 +322
- Partials 5207 5354 +147 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shortcuts/apps/apps_role_common.go (1)
452-456: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMagic number "100" duplicated instead of referencing
maxRoleMembers.
roleMemberLimitParams'sreasonstring (L475) hardcodes"...exceeds 100", and the adjacent.WithHint(...)text (L455) hardcodes"at most 100 members". IfmaxRoleMembersever changes, both strings silently go stale while the primary error message (which correctly uses%d, maxRoleMembers) stays accurate.♻️ Proposed fix
- WithHint("reduce the atomic request to at most 100 members; the CLI does not split member writes automatically") + WithHint(fmt.Sprintf("reduce the atomic request to at most %d members; the CLI does not split member writes automatically", maxRoleMembers))func roleMemberLimitParams(groups roleMemberGroups) []errs.InvalidParam { - reason := "combined role member count exceeds 100" + reason := fmt.Sprintf("combined role member count exceeds %d", maxRoleMembers)Also applies to: 474-487
🤖 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 `@shortcuts/apps/apps_role_common.go` around lines 452 - 456, Replace the hardcoded “100” values in the WithHint text near the maxRoleMembers check and the reason string returned by roleMemberLimitParams with formatting that uses maxRoleMembers, so all member-limit messages remain consistent when the constant changes.
🤖 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.
Inline comments:
In `@shortcuts/apps/apps_role_common.go`:
- Around line 288-313: The authorization branch in withRoleErrorHint currently
overwrites server-provided Problem.Hint details; preserve existing lifted detail
by appending the role-specific hint instead. Update the hint-selection logic so
CategoryAuthorization follows the same append behavior as other noncanonical
existing hints, while retaining the current handling for empty or canonical API
hints.
---
Nitpick comments:
In `@shortcuts/apps/apps_role_common.go`:
- Around line 452-456: Replace the hardcoded “100” values in the WithHint text
near the maxRoleMembers check and the reason string returned by
roleMemberLimitParams with formatting that uses maxRoleMembers, so all
member-limit messages remain consistent when the constant changes.
🪄 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: 93faca35-fe72-42ce-84cb-7ecac62e1859
📒 Files selected for processing (13)
internal/errclass/codemeta_spark.gointernal/errclass/codemeta_spark_test.gointernal/qualitygate/rules/dryrun.gointernal/qualitygate/rules/dryrun_test.goshortcuts/apps/apps_role.goshortcuts/apps/apps_role_common.goshortcuts/apps/apps_role_common_test.goshortcuts/apps/apps_role_member.goshortcuts/apps/apps_role_member_test.goshortcuts/apps/apps_role_test.goskills/lark-apps/references/lark-apps-role.mdtests/cli_e2e/apps/apps_role_management_test.gotests/cli_e2e/apps/coverage.md
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/cli_e2e/apps/coverage.md
- shortcuts/apps/apps_role_test.go
- shortcuts/apps/apps_role_member.go
- skills/lark-apps/references/lark-apps-role.md
- tests/cli_e2e/apps/apps_role_management_test.go
- shortcuts/apps/apps_role_member_test.go
Comment on lines
+288
to
+313
| // withRoleErrorHint refines documented Spark role errors with command-specific | ||
| // recovery while preserving the typed error, numeric code, log_id, and any | ||
| // server-provided detail. Unknown codes retain the existing Apps fallback. | ||
| func withRoleErrorHint(err error, operation roleErrorOperation) error { | ||
| if err == nil { | ||
| return nil | ||
| } | ||
| problem, ok := errs.ProblemOf(err) | ||
| if !ok { | ||
| return err | ||
| } | ||
| hint := roleErrorHint(problem.Code, operation) | ||
| if hint == "" { | ||
| return withAppsHint(err, roleFallbackHint(operation)) | ||
| } | ||
|
|
||
| existing := strings.TrimSpace(problem.Hint) | ||
| canonicalAPIHint := strings.TrimSpace(errclass.APIHint(problem.Subtype)) | ||
| switch { | ||
| case existing == "", existing == canonicalAPIHint, problem.Category == errs.CategoryAuthorization: | ||
| problem.Hint = hint | ||
| case !strings.Contains(existing, hint): | ||
| problem.Hint = existing + "; " + hint | ||
| } | ||
| return err | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check classification of role auth-related error codes and whether buildPermissionError can surface server detail.
rg -n -B2 -A10 '3344030|3344031' internal/errclass/codemeta_spark.go
rg -n -A30 'func buildPermissionError' internal/errclass/classify.goRepository: larksuite/cli
Length of output: 3327
🏁 Script executed:
#!/bin/bash
sed -n '288,330p' shortcuts/apps/apps_role_common.go
printf '\n--- tests ---\n'
rg -n -A8 -B8 'withRoleErrorHint|PreservesServerDetail|roleErrAdminRequired|roleErrManagerRequired' shortcuts/apps -g '*test.go'Repository: larksuite/cli
Length of output: 7140
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect BuildAPIError detail lifting and any auth-code detail tests/fixtures.
rg -n -A40 -B20 'func BuildAPIError|liftErrorDetailValues|details' internal/errclass -g '*.go'
printf '\n--- role auth detail fixtures/tests ---\n'
rg -n -A8 -B8 'roleErrAdminRequired|roleErrManagerRequired|3344030|3344031|PermissionDenied|CategoryAuthorization' shortcuts/apps internal/errclass -g '*test.go' -g '*.go'Repository: larksuite/cli
Length of output: 50369
Authorization role errors overwrite server detail. BuildAPIError already lifts resp.error.details[].value into Problem.Hint, but this errs.CategoryAuthorization branch replaces that hint with the role-specific text for roleErrAdminRequired/roleErrManagerRequired. Preserve the lifted detail here and append the role hint instead.
🤖 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 `@shortcuts/apps/apps_role_common.go` around lines 288 - 313, The authorization
branch in withRoleErrorHint currently overwrites server-provided Problem.Hint
details; preserve existing lifted detail by appending the role-specific hint
instead. Update the hint-selection logic so CategoryAuthorization follows the
same append behavior as other noncanonical existing hints, while retaining the
current handling for empty or canonical API hints.
5 tasks
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.
Summary
Add first-class app role management to the
appsshortcut domain, including role CRUD, typed member operations, and user-role matching. The commands preserve structured error contracts, require explicit confirmation for destructive operations, and keep machine-readable output suitable for AI-agent consumers.Changes
+role-list,+role-get,+role-create,+role-update,+role-delete,+role-member-list,+role-member-add,+role-member-remove, and+role-match-list.lark-appsSkill and role reference with role routing, high-impact confirmation, readback, and member-resolution guidance.od_tood-and refresh the selected Apps E2E coverage accounting.Test Plan
make unit-test)make build)go vet ./...gofmt -l .produces no outputgo mod tidyleavesgo.modandgo.sumunchangedgo test ./tests/cli_e2e/apps -count=1lark-cli apps +role-*lifecycle and member-management flows work as expectedmain(pending)Related Issues
Summary by CodeRabbit
od-form.