feat: add miaoda app collaborator management - #2191
Conversation
|
|
github-actions
Bot
added
the
size/L
|
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:
📝 WalkthroughWalkthroughThe PR adds six apps collaborator-management shortcuts for listing, modifying, removing collaborators, and managing collaboration settings. It adds typed validation, response projection, error handling, command registration, CLI dry-run coverage, and skill documentation. ChangesApps collaborator management
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CLI
participant AppsMemberShortcut
participant MiaodaAPI
participant OutputRenderer
CLI->>AppsMemberShortcut: invoke collaborator or settings command
AppsMemberShortcut->>MiaodaAPI: send validated request
MiaodaAPI-->>AppsMemberShortcut: return collaborator or settings response
AppsMemberShortcut->>OutputRenderer: project and render response
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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@5e973905963bc21e5abd1a369b287501f597c25b🧩 Skill updatenpx skills add larksuite/cli#codex/miaoda-member-permissions -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: 2
🧹 Nitpick comments (6)
tests/cli_e2e/dryrun/apps_member_dryrun_test.go (1)
194-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that stdout stays empty on validation failures.
The validation cases check exit code 2 and read the typed envelope from
result.Stderr. No assertion coversresult.Stdout. If a regression wrote the validation envelope to stdout, these tests would still pass, and stdout would no longer be reserved for program data.Add
require.Empty(t, result.Stdout)to each validation case and to the empty settings-set case at lines 207-210.As per coding guidelines: "Send JSON program data to stdout and progress, warnings, and hints to stderr; never mix the two streams." Based on learnings: "Validate-stage failures must exit with code 2, write the typed JSON validation envelope to result.Stderr, and leave result.Stdout empty so stdout remains reserved for program data."♻️ Proposed fix
result := runAppsMemberCLI(t, tc.args...) result.AssertExitCode(t, 2) + require.Empty(t, result.Stdout, "stdout must stay reserved for program data, stderr:\n%s", result.Stderr) require.Equal(t, "validation", gjson.Get(result.Stderr, "error.type").String(), "stderr:\n%s", result.Stderr)🤖 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 `@tests/cli_e2e/dryrun/apps_member_dryrun_test.go` around lines 194 - 205, Add require.Empty(t, result.Stdout) to each validation subtest in the apps member CLI test loop and to the empty settings-set case, while preserving the existing stderr envelope and exit-code assertions.Sources: Coding guidelines, Learnings
shortcuts/apps/apps_member_common.go (1)
297-322: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueValidate
perminside the request builders for consistency.
buildMemberAddRequestandbuildMemberUpdateRequestvalidate the member identity throughbuildMemberIdentity, but they copy--permwithout checking it againstmemberRoles. Role validation exists only invalidateMemberMutation. TheDryRuncallbacks discard the builder error (body, _ := ...), so the builders are the last line of defense ifValidateis ever skipped or reordered.Move the role check into a shared helper so both the identity and the role fail closed at the same layer.
♻️ Proposed shared role validation
+func buildMemberRole(rctx *common.RuntimeContext) (string, error) { + role := strings.TrimSpace(rctx.Str("perm")) + if !memberStringAllowed(role, memberRoles) { + return "", appsValidationParamError("--perm", "--perm must be one of: view, edit, full_access"). + WithHint("choose the collaborator permission explicitly") + } + return role, nil +}🤖 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_member_common.go` around lines 297 - 322, Update buildMemberAddRequest and buildMemberUpdateRequest to validate the trimmed perm value against the existing memberRoles set before constructing or returning either request. Extract the shared role check into a helper alongside the request builders, return an error for missing or unsupported roles, and propagate that error from both builders so DryRun paths cannot bypass validation.shortcuts/apps/apps_member_common_test.go (2)
79-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the wrapped
validate.ResourceNamefailure.
requireAppsMemberValidationErrorchecksSubtype,Param, and a non-emptyHint. No test reachesvalidateMemberAppIDlines 205-209, which is the only--app-idbranch that attaches a cause withWithCause(err). Current cases stop at the prefix and character checks above it.Add one case with an
app_value that passes the prefix and character checks but failsvalidate.ResourceName, then assert the cause is preserved witherrors.Isorerrors.Unwrap.As per coding guidelines: "Error-path tests must assert typed metadata through
errs.ProblemOf(category,subtype, andparam) and verify cause preservation rather than relying only on message substrings."🤖 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_member_common_test.go` around lines 79 - 98, Add a test case for validateMemberAppID using an app_ identifier that passes prefix and character validation but fails validate.ResourceName, then verify the returned error’s typed metadata via errs.ProblemOf for category, subtype, and param and assert the original cause is preserved with errors.Is or errors.Unwrap. Extend requireAppsMemberValidationError only as needed while retaining its existing metadata checks.Source: Coding guidelines
61-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated int-flag default parsing in both member test helpers. Both runtime helpers derive an int flag default by comparing
flag.Defaultto the literal string"20". Any int flag added later with a different default silently becomes 0, so a test would exercise a default the shortcut never declares. The shared root cause is the hardcoded string comparison instead of parsing the declared default.
shortcuts/apps/apps_member_common_test.go#L61-L66: replace theflag.Default == "20"comparison innewAppsMemberRuntimewithstrconv.Atoi(flag.Default)and add thestrconvimport.shortcuts/apps/apps_member_response_test.go#L38-L43: apply the samestrconv.Atoi(flag.Default)change innewAppsMemberAPIRuntimeand add thestrconvimport.🤖 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_member_common_test.go` around lines 61 - 66, Replace the hardcoded integer default comparison with strconv.Atoi(flag.Default) in newAppsMemberRuntime at shortcuts/apps/apps_member_common_test.go:61-66 and add the strconv import. Apply the same change in newAppsMemberAPIRuntime at shortcuts/apps/apps_member_response_test.go:38-43, including its strconv import, so both helpers use each flag’s declared default.shortcuts/apps/apps_member_response_test.go (2)
116-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the fail-closed subtests instead of using letters.
t.Run(string(rune('a'+index)))produces subtest namesathroughg. A failure reports only a letter, so the reader must count table entries to find which malformed record failed. Each case maps to a distinct guard inprojectMemberRecord.♻️ Proposed named cases
- tests := []memberAPIRecord{ - {MemberType: "user", Role: "view"}, - {MemberType: "user", UserOpenID: &user, ChatID: &chat, Role: "view"}, - {MemberType: "user", ChatID: &chat, Role: "view"}, - {MemberType: "user", UserOpenID: &internal, Role: "view"}, - {MemberType: "chat", ChatID: &empty, Role: "view"}, - {MemberType: "unknown", UserOpenID: &user, Role: "view"}, - {MemberType: "user", UserOpenID: &user, Role: "owner"}, - } - for index, raw := range tests { - t.Run(string(rune('a'+index)), func(t *testing.T) { + tests := []struct { + name string + raw memberAPIRecord + }{ + {"no-typed-id", memberAPIRecord{MemberType: "user", Role: "view"}}, + {"two-typed-ids", memberAPIRecord{MemberType: "user", UserOpenID: &user, ChatID: &chat, Role: "view"}}, + {"type-id-mismatch", memberAPIRecord{MemberType: "user", ChatID: &chat, Role: "view"}}, + {"internal-numeric-id", memberAPIRecord{MemberType: "user", UserOpenID: &internal, Role: "view"}}, + {"empty-id", memberAPIRecord{MemberType: "chat", ChatID: &empty, Role: "view"}}, + {"unknown-member-type", memberAPIRecord{MemberType: "unknown", UserOpenID: &user, Role: "view"}}, + {"unsupported-role", memberAPIRecord{MemberType: "user", UserOpenID: &user, Role: "owner"}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) {Update the body to use
tc.raw.🤖 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_member_response_test.go` around lines 116 - 130, Name each table-driven case with a descriptive field and update the loop to use the named case’s raw record via tc.raw. Replace the letter-based t.Run name generation around projectMemberRecord so failures identify the specific malformed member record and its corresponding validation guard.
322-347: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCall
registry.Verify(t)in the settings execute test.
TestAppsMemberListExecuteUsesTypedProjectionWithoutLeakingRawFieldsandTestAppsMemberMutationExecuteProjectsResponsesboth callregistry.Verify(t). This test omits it. WithoutVerify, a stub that is never matched, or a request sent to a different URL or method, goes unreported.♻️ Proposed fix
if err := tc.shortcut.Execute(context.Background(), rctx); err != nil { t.Fatalf("%s Execute: %v", tc.shortcut.Command, err) } + registry.Verify(t) out := stdout.String()🤖 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_member_response_test.go` around lines 322 - 347, Call registry.Verify(t) at the end of each subtest in the settings execute test, after output assertions complete. Update the loop containing the shortcut Execute invocation so unmatched stubs and incorrect request method or URL are reported, consistent with the related execute tests.
🤖 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_member_common.go`:
- Around line 593-635: Consolidate the duplicated six-setting enum definitions
into one package-level memberSettingSpecs table containing each setting’s flag
name, JSON field name, and allowed values. Update
buildMemberSettingsUpdateRequest, validateMemberSettingsResponse, and
validateMemberSettingChanges to derive their field mappings and allowlists from
this table, and update AppsMemberSettingsSet.Flags to derive its flag enums from
the same source. Remove the duplicate allowlist declarations while preserving
current validation and flag behavior.
In `@shortcuts/apps/apps_member_response_test.go`:
- Around line 180-214: Update the stub response in
TestAppsMemberListExecuteNeverLeaksMetaTokenAcrossFormats to include meta_token
and sensitive-internal-token fields with their forbidden values in the member
record (and app data if applicable). Keep the existing cross-format output
assertions so the test verifies projection removes upstream-sensitive fields and
fails if projectMemberRecord is reverted.
---
Nitpick comments:
In `@shortcuts/apps/apps_member_common_test.go`:
- Around line 79-98: Add a test case for validateMemberAppID using an app_
identifier that passes prefix and character validation but fails
validate.ResourceName, then verify the returned error’s typed metadata via
errs.ProblemOf for category, subtype, and param and assert the original cause is
preserved with errors.Is or errors.Unwrap. Extend
requireAppsMemberValidationError only as needed while retaining its existing
metadata checks.
- Around line 61-66: Replace the hardcoded integer default comparison with
strconv.Atoi(flag.Default) in newAppsMemberRuntime at
shortcuts/apps/apps_member_common_test.go:61-66 and add the strconv import.
Apply the same change in newAppsMemberAPIRuntime at
shortcuts/apps/apps_member_response_test.go:38-43, including its strconv import,
so both helpers use each flag’s declared default.
In `@shortcuts/apps/apps_member_common.go`:
- Around line 297-322: Update buildMemberAddRequest and buildMemberUpdateRequest
to validate the trimmed perm value against the existing memberRoles set before
constructing or returning either request. Extract the shared role check into a
helper alongside the request builders, return an error for missing or
unsupported roles, and propagate that error from both builders so DryRun paths
cannot bypass validation.
In `@shortcuts/apps/apps_member_response_test.go`:
- Around line 116-130: Name each table-driven case with a descriptive field and
update the loop to use the named case’s raw record via tc.raw. Replace the
letter-based t.Run name generation around projectMemberRecord so failures
identify the specific malformed member record and its corresponding validation
guard.
- Around line 322-347: Call registry.Verify(t) at the end of each subtest in the
settings execute test, after output assertions complete. Update the loop
containing the shortcut Execute invocation so unmatched stubs and incorrect
request method or URL are reported, consistent with the related execute tests.
In `@tests/cli_e2e/dryrun/apps_member_dryrun_test.go`:
- Around line 194-205: Add require.Empty(t, result.Stdout) to each validation
subtest in the apps member CLI test loop and to the empty settings-set case,
while preserving the existing stderr envelope and exit-code assertions.
🪄 Autofix
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 Plus
Run ID: badaec36-8761-4de6-8235-36b5051f5b88
📒 Files selected for processing (10)
errs/subtypes.goshortcuts/apps/apps_member.goshortcuts/apps/apps_member_common.goshortcuts/apps/apps_member_common_test.goshortcuts/apps/apps_member_response_test.goshortcuts/apps/shortcuts.goshortcuts/apps/shortcuts_test.goskills/lark-apps/SKILL.mdtests/cli_e2e/apps/coverage.mdtests/cli_e2e/dryrun/apps_member_dryrun_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2191 +/- ##
==========================================
+ Coverage 75.85% 76.01% +0.15%
==========================================
Files 958 968 +10
Lines 101701 103167 +1466
==========================================
+ Hits 77150 78422 +1272
- Misses 18684 18777 +93
- Partials 5867 5968 +101 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1f46b44 to
cfeb5ed
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shortcuts/apps/apps_member_common.go (1)
423-432: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the invalid-parameter list from
memberSettingSpecs.The loop on Lines 417-422 reads the flag names from
memberSettingSpecs. TheWithParamscall on Lines 425-430 hardcodes the same six flag names. If a spec is added to the table, the new flag becomes usable, but this error omits it from the typed parameter list. An agent reading the error then cannot discover the flag.Build the parameter slice from the same table.
♻️ Proposed fix
+ params := make([]errs.InvalidParam, 0, len(memberSettingSpecs)) + for _, spec := range memberSettingSpecs { + params = append(params, appsInvalidParam("--"+spec.flag, "not provided")) + } return appsValidationError("at least one collaborator setting must be provided"). - WithParams( - appsInvalidParam("--external-access", "not provided"), - appsInvalidParam("--external-invite", "not provided"), - appsInvalidParam("--link-share", "not provided"), - appsInvalidParam("--manage-collaborators-by", "not provided"), - appsInvalidParam("--comment-by", "not provided"), - appsInvalidParam("--copy-download-by", "not provided"), - ). + WithParams(params...). WithHint("pass at least one setting flag; omitted settings remain unchanged")Confirm the element type accepted by
WithParamsbefore applying this change.🤖 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_member_common.go` around lines 423 - 432, Update the validation error path surrounding memberSettingSpecs so the WithParams argument is built by iterating over memberSettingSpecs and converting each spec’s flag name into the accepted invalid-parameter type. Remove the hardcoded six-name list, confirm the element type expected by WithParams, and preserve the existing validation message and hint.
🤖 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/SKILL.md`:
- Line 69: 更新包含 +member-add、+member-update、+member-remove 和 +member-settings-set
的高风险写命令说明，使其明确遵循全局预授权规则：仅在未预授权时要求逐步确认；已预授权流程可跳过该确认，但仍保留 --dry-run 核对要求及真实执行时使用
--yes 的约束。
---
Nitpick comments:
In `@shortcuts/apps/apps_member_common.go`:
- Around line 423-432: Update the validation error path surrounding
memberSettingSpecs so the WithParams argument is built by iterating over
memberSettingSpecs and converting each spec’s flag name into the accepted
invalid-parameter type. Remove the hardcoded six-name list, confirm the element
type expected by WithParams, and preserve the existing validation message and
hint.
🪄 Autofix
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 Plus
Run ID: 9a652eb3-7f1b-4b27-9a3f-7933d782f080
📒 Files selected for processing (10)
errs/subtypes.goshortcuts/apps/apps_member.goshortcuts/apps/apps_member_common.goshortcuts/apps/apps_member_common_test.goshortcuts/apps/apps_member_response_test.goshortcuts/apps/shortcuts.goshortcuts/apps/shortcuts_test.goskills/lark-apps/SKILL.mdtests/cli_e2e/apps/coverage.mdtests/cli_e2e/dryrun/apps_member_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- errs/subtypes.go
- shortcuts/apps/shortcuts.go
- shortcuts/apps/shortcuts_test.go
- tests/cli_e2e/dryrun/apps_member_dryrun_test.go
- shortcuts/apps/apps_member.go
- tests/cli_e2e/apps/coverage.md
- shortcuts/apps/apps_member_common_test.go
- shortcuts/apps/apps_member_response_test.go
cfeb5ed to
a51835d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
2 participants
Summary
Add AI-friendly collaborator management commands for Miaoda applications. The new shortcuts let agents list, add, update, and remove collaborators, as well as read and update collaborator permission settings, using structured external IDs and typed errors.
Changes
apps +member-list,+member-add,+member-update, and+member-removewith user, chat, and department collaborator support.apps +member-settings-getand+member-settings-setfor the supported sharing and collaborator policy fields.--dry-run.feature_not_availableerror for applications whose collaborators must be managed in the Miaoda console.lark-appsskill guidance and add unit, response-contract, and dry-run E2E coverage.Test Plan
go test ./errs -count=1go test ./shortcuts/apps -count=1go test ./tests/cli_e2e/dryrun -run '^TestAppsMember' -count=1go vet ./errs ./shortcuts/apps ./tests/cli_e2e/dryrunRelated Issues
Summary by CodeRabbit
New Features
Documentation
Tests