feat(apps): add role management shortcuts#1881
Merged
Merged
Conversation
|
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 nine Apps CLI shortcuts for role CRUD, role-member management, and role matching, with shared validation, pagination, response normalization, rendering, documentation, unit tests, registry tests, Spark error metadata, and fixture-gated E2E workflows. Department identifier fixtures now use dash syntax. ChangesApps role management
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant AppsAPI
participant RoleState
CLI->>AppsAPI: List, create, update, delete, or match role request
AppsAPI->>RoleState: Read or mutate role and member data
RoleState-->>AppsAPI: Role or member response
AppsAPI-->>CLI: Typed response or Spark error
CLI->>AppsAPI: Independent readback for verification
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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@5cdb7ee7de4e46f19aff7ccfc750c8f4fd53dbd2🧩 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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1881 +/- ##
==========================================
+ Coverage 74.73% 74.88% +0.14%
==========================================
Files 887 891 +4
Lines 92644 93730 +1086
==========================================
+ Hits 69235 70187 +952
- Misses 18050 18138 +88
- Partials 5359 5405 +46 ☔ 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
5 tasks
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@skills/lark-apps/references/lark-apps-role.md`:
- Around line 7-15: Remove the blank lines between the consecutive [!CAUTION]
blockquote paragraphs in the role documentation. Keep all caution text within
one contiguous blockquote so markdownlint MD028 passes.
- Around line 409-413: Update the destructive role-member removal procedure to
require confirmation before any write, following the batch-removal rule near
line 381. After identifying the affected app, roles, and target group, present
them and obtain confirmation, then execute the full command with --app-id,
--role-id, --as user, --chats, --yes, and --format json while preserving serial
writes and post-removal rereads.
🪄 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: 782e6372-29d6-442e-bd83-58aba6ffc618
📒 Files selected for processing (8)
shortcuts/apps/apps_role.goshortcuts/apps/apps_role_member.goshortcuts/apps/apps_role_member_test.goshortcuts/apps/apps_role_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.go
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/apps/apps_role_member.go
- shortcuts/apps/apps_role.go
- tests/cli_e2e/apps/apps_role_management_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.go`:
- Around line 624-658: Update roleResponseData and parseRoleResponseData to
project description into a typed Description field, validating any present value
as a supported string and rejecting unsupported non-string values instead of
retaining it in Fields; update shortcuts/apps/apps_role.go:709-712 to render the
projected Description, and add the non-string-description invalid-response case
in shortcuts/apps/apps_role_test.go:385-412.
In `@skills/lark-apps/references/lark-apps-role.md`:
- Line 286: Update the contact +search-user recipe and its duplicate rule to
match email inputs against the user email field rather than localized_name,
while continuing to match name inputs against localized_name. Preserve the
requirement that exactly one user matches and use that matched user’s open_id,
with has_more=false.
🪄 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: 66d3ea71-4715-4a03-a4ed-402ad50570c1
📒 Files selected for processing (8)
shortcuts/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.go
🚧 Files skipped from review as they are similar to previous changes (5)
- shortcuts/apps/apps_role_common_test.go
- shortcuts/apps/apps_role_common.go
- tests/cli_e2e/apps/apps_role_management_test.go
- shortcuts/apps/apps_role_member.go
- shortcuts/apps/apps_role_member_test.go
linchao5102
force-pushed
the
feat/apps-role-management-framework
branch
3 times, most recently
from
9a6801b to
6051903
Compare
linchao5102
force-pushed
the
feat/apps-role-management-framework
branch
from
6051903 to
5cdb7ee
Compare
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
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.This revision also hardens role-member response semantics: filtered reads expose only the selected member field, omitted fields remain unknown instead of being reported as empty, and malformed member mutation responses fail closed.
Changes
+role-list,+role-get,+role-create,+role-update,+role-delete,+role-member-list,+role-member-add,+role-member-remove, and+role-match-list.--member-typereturns only the selected field, pretty output omits unqueried groups, and malformed returned IDs or group shapes produce typedinvalid_responseerrors.lark-appsSkill and role reference with focused role routing, high-impact confirmation, readback, and member-resolution guidance.od_tood-and refresh Apps E2E coverage accounting to reflect 79 registered Apps shortcuts.Test Plan
make buildmake unit-testgo vet ./...make fmt-checkgo mod tidy -diffgo run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/mainQUALITY_GATE_CHANGED_FROM=origin/main make quality-gateLARK_CLI_BIN=./lark-cli go test ./tests/cli_e2e/apps -count=1with live role fixtures and tokens intentionally unset60519031, including unit, lint, deterministic-gate, dry-run E2E, live E2E job, coverage, security, and result aggregationLive role E2E remains fixture-gated and skipped by default to avoid mutating tenant role state; this verification did not perform live tenant mutations.
Related Issues
Summary by CodeRabbit
New Features
+role-list/get/create/update/delete), role member management (+role-member-list/add/remove), and role matching (+role-match-list), including output formatting and safer high-risk flows.+role-member-listcan warn and fall back when chat filtering isn’t supported.Bug Fixes
od-form (including simulated/dry-run placeholders).Documentation
Tests