feat(drive): add +member-add shortcut for Drive collaborator permissions#1204
Open
zhaojiaxing-coding wants to merge 5 commits into
Open
feat(drive): add +member-add shortcut for Drive collaborator permissions#1204zhaojiaxing-coding wants to merge 5 commits into
zhaojiaxing-coding wants to merge 5 commits into
Conversation
Add drive +member-add shortcut that grants view/edit/full_access permissions to collaborators on Drive documents, files, folders, and wiki nodes. Supports single and batch member add (up to 10). Key features: - Auto-infer resource type from Feishu/Lark/Doubao URL or token prefix - Required --member-type with prefix conflict detection (appid aliased to userid) - Reject --perm-type for non-wiki resource types - Trusted host validation for URL inputs Also extends shortcuts/common/resource_url with minutes type support and InferResourceTypeFromToken for bare token prefix mapping.
|
|
|
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:
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a new drive +member-add shortcut with token-prefix resource-type inference, flag/spec parsing and validation, single and batch execution (with partial-failure detection), response shaping, comprehensive tests (unit/integration/e2e), and documentation updates. ChangesDrive +member-add shortcut and resource type inference
Sequence DiagramsequenceDiagram
participant User
participant CLI as "drive +member-add"
participant Parser as "readDriveMemberAddSpec"
participant Inference as "InferResourceTypeFromToken"
participant DriveAPI as "Drive API"
participant Output as "driveMemberAddOutput"
User->>CLI: invoke with --token, --member-id, flags
CLI->>Parser: parse flags
Parser->>Inference: infer resource type from token
Inference-->>Parser: resource type or not found
Parser->>Parser: validate member-type, perm, need-notification
Parser->>DriveAPI: POST single or POST /members/batch_create
DriveAPI-->>Output: API response (members / errors)
Output->>Output: align/backfill/mask internal fields
Output-->>User: flattened JSON + status or partial_failure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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
domain/ccm
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1204 +/- ##
==========================================
+ Coverage 68.61% 69.30% +0.69%
==========================================
Files 625 635 +10
Lines 58348 59823 +1475
==========================================
+ Hits 40035 41462 +1427
- Misses 15029 15034 +5
- Partials 3284 3327 +43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@9d54744759b713055132f9d4d2bd17fdf482e071🧩 Skill updatenpx skills add larksuite/cli#feat/drive-member-add-shortcut -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/drive/drive_member_add_test.go`:
- Around line 673-711: The test currently only asserts the POST body but must
also assert the bot request omitted the need_notification query param; update
the test that uses the httpmock.Stub (stub) in DriveMemberAdd/mountAndRunDrive
to capture the incoming request's URL.RawQuery (or use the stub's
CapturedRequest/URL field) after mountAndRunDrive runs, then assert that the raw
query string does not contain "need_notification" (or that parsed query has no
"need_notification" key); keep existing body assertions (captured["perm"] ==
"edit") and add a failing t.Fatalf message if the query contains
need_notification so the execute-path contract is enforced.
In `@shortcuts/drive/drive_member_add.go`:
- Around line 414-438: The loop currently only appends members when
rawMembers[i] is a valid map and falls back to backfilling only if results is
empty; change it to iterate over spec.MemberIDs (by index) and for each index
attempt to read rawMembers[i] as a map[string]interface{}—if present use
driveMemberAddOutput(spec, memberID, m), otherwise call
driveMemberAddOutput(spec, memberID, nil) so every requested member is
represented; ensure results is built in the same order as spec.MemberIDs and
that the final runtime.Out and the printed count reflect the full spec.MemberIDs
length.
In `@skills/lark-drive/SKILL.md`:
- Around line 259-261: The two adjacent blockquote paragraphs in SKILL.md need
to be merged into one contiguous blockquote to satisfy markdownlint MD028;
remove the blank line between the two blocks and combine their text into a
single blockquote so the note about member-type mapping and recommendation to
use bot open_id appears as one continuous block (look for the blockquote
starting with "注意：此方式仅适用于需要授权给**当前应用**的场景。" and the following "补充：" paragraph
and join them).
In `@tests/cli_e2e/drive/drive_member_add_dryrun_test.go`:
- Around line 319-320: The test currently asserts the dry-run validation error
only against result.Stderr; update the assertion so Validate-stage failures are
checked from the full CLI output by combining result.Stdout and result.Stderr.
Replace the require.Contains call that uses result.Stderr with one that checks
result.Stdout+result.Stderr (keeping the existing result.AssertExitCode(t, 2)
and tt.wantErr), so the test verifies the error message in the structured stdout
envelope as well as stderr.
🪄 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: e259d1a8-6b0f-4838-9192-0e4b19d3539b
📒 Files selected for processing (9)
shortcuts/common/resource_url.goshortcuts/common/resource_url_test.goshortcuts/drive/drive_member_add.goshortcuts/drive/drive_member_add_test.goshortcuts/drive/shortcuts.goshortcuts/drive/shortcuts_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-member-add.mdtests/cli_e2e/drive/drive_member_add_dryrun_test.go
Harden drive +member-add after reviewing the shortcut contract and skill docs. Keep the shortcut focused on Drive permission resources that are supported by the CLI, and make batch results conservative when the API response is incomplete. Key fixes: - Remove unsupported minutes/mincn URL and token inference from common resource parsing - Pass appid through as member_type=appid instead of aliasing it to userid - Reject duplicate batch member IDs and keep the batch limit at 10 - Return partial_failure for incomplete or mismatched batch member responses - Align lark-drive skill docs and dry-run E2E coverage with the implemented behavior
There was a problem hiding this comment.
♻️ Duplicate comments (2)
shortcuts/drive/drive_member_add_test.go (1)
867-910:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert
need_notificationis omitted for bot execution.This still only checks the captured body. Without inspecting
req.URL.RawQuery, a regression that starts sendingneed_notificationfor--as botwill slip through even though the comment says that contract matters.Suggested test update
func TestDriveMemberAdd_ExecuteSuccessAsBot(t *testing.T) { t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) f, stdout, _, reg := cmdutil.TestFactory(t, driveTestConfig()) + var capturedQuery string stub := &httpmock.Stub{ Method: "POST", URL: "/open-apis/drive/v1/permissions/wikcnBotTok/members", + OnMatch: func(req *http.Request) { + capturedQuery = req.URL.RawQuery + }, Body: map[string]interface{}{ "code": 0, "msg": "success", "data": map[string]interface{}{ @@ if captured["perm"] != "edit" { t.Fatalf("captured body perm = %v, want edit", captured["perm"]) } + if strings.Contains(capturedQuery, "need_notification=") { + t.Fatalf("captured query = %q, want need_notification omitted", capturedQuery) + } data := decodeDriveEnvelope(t, stdout)🤖 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/drive/drive_member_add_test.go` around lines 867 - 910, Add an assertion in TestDriveMemberAdd_ExecuteSuccessAsBot to verify the HTTP request query does NOT include need_notification for the "--as bot" case: after mountAndRunDrive and after decoding stub.CapturedBody keep the existing body checks, then inspect the request captured by the stub (e.g., stub.CapturedRequest.URL.RawQuery or parse stub.CapturedRequest.URL.Query()) and fail the test if the "need_notification" parameter is present or non-empty; reference the test function TestDriveMemberAdd_ExecuteSuccessAsBot and the stub variable to locate where to add this check.shortcuts/drive/drive_member_add.go (1)
443-496:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't validate batch success from a backfilled
member_id.This loop backfills
member_idbefore it decides whether a row succeeded, so a same-length response that omitsmember_idcan still be counted as a full success. It also buildsmembersonly fromrawMembers, which means shorter/malformed arrays still drop requested IDs from the partial-failure payload instead of returning one slot per requested member.💡 Suggested fix
func buildDriveMemberAddBatchResult(spec driveMemberAddSpec, data map[string]interface{}) map[string]interface{} { rawMembers, _ := data["members"].([]interface{}) - results := make([]map[string]interface{}, 0, len(rawMembers)) - succeededIDs := make(map[string]bool, len(rawMembers)) + results := make([]map[string]interface{}, 0, len(spec.MemberIDs)) + succeededIDs := make(map[string]bool, len(spec.MemberIDs)) var mismatched []map[string]interface{} - for i, raw := range rawMembers { - m, ok := raw.(map[string]interface{}) - if !ok { - continue - } - fallbackMemberID := "" - if len(rawMembers) == len(spec.MemberIDs) && i < len(spec.MemberIDs) { - fallbackMemberID = spec.MemberIDs[i] - } - out := driveMemberAddOutputWithOptions(spec, fallbackMemberID, m, len(rawMembers) == len(spec.MemberIDs)) - memberID := common.GetString(out, "member_id") - if i < len(spec.MemberIDs) { - requestedID := spec.MemberIDs[i] - rawMemberID := common.GetString(m, "member_id") - if rawMemberID != "" && rawMemberID != requestedID { - mismatched = append(mismatched, map[string]interface{}{ - "index": i + 1, - "requested": requestedID, - "returned": rawMemberID, - }) - } else if memberID == requestedID { - succeededIDs[requestedID] = true - } - } - results = append(results, out) + for i, requestedID := range spec.MemberIDs { + var rawMember map[string]interface{} + if i < len(rawMembers) { + rawMember, _ = rawMembers[i].(map[string]interface{}) + } + + if rawMemberID := common.GetString(rawMember, "member_id"); rawMemberID != "" { + if rawMemberID != requestedID { + mismatched = append(mismatched, map[string]interface{}{ + "index": i + 1, + "requested": requestedID, + "returned": rawMemberID, + }) + } else { + succeededIDs[requestedID] = true + } + } + + results = append(results, driveMemberAddOutputWithOptions(spec, requestedID, rawMember, true)) }🤖 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/drive/drive_member_add.go` around lines 443 - 496, The function buildDriveMemberAddBatchResult currently backfills member_id via driveMemberAddOutputWithOptions before deciding success and builds the members slice only from rawMembers, causing missing/short responses to be counted as successes and to drop slots for requested IDs; fix by: iterate over spec.MemberIDs as the primary loop, for each index obtain raw entry from rawMembers if present (raw := rawMembers[i] else nil), extract rawMemberID directly from the raw map (do not rely on the backfilled out member_id) to determine success and populate succeededIDs only when rawMemberID == requestedID, call driveMemberAddOutputWithOptions to build the output but ensure you create placeholder outputs for missing/raw-invalid entries so the resulting members slice has one entry per requested ID, and recompute partial/missing/mismatched accordingly (refer to buildDriveMemberAddBatchResult, spec.MemberIDs, rawMembers, driveMemberAddOutputWithOptions, and the "member_id" field).
🧹 Nitpick comments (1)
shortcuts/drive/drive_member_add_test.go (1)
674-728: ⚡ Quick winAssert the batch execute query contract.
This test pins the batch path and body, but it never inspects
req.URL.RawQuery, so a regression that stops sending the inferredtypequery param would still pass. Capture the request query in this happy-path execute test and assert it matches the inferred resource type forbascnTok.Suggested test update
func TestDriveMemberAdd_ExecuteBatchSuccess(t *testing.T) { t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) f, stdout, stderr, reg := cmdutil.TestFactory(t, driveTestConfig()) + var capturedQuery string stub := &httpmock.Stub{ Method: "POST", URL: "/open-apis/drive/v1/permissions/bascnTok/members/batch_create", + OnMatch: func(req *http.Request) { + capturedQuery = req.URL.RawQuery + }, Body: map[string]interface{}{ "code": 0, "msg": "success", "data": map[string]interface{}{ @@ if err != nil { t.Fatalf("unexpected error: %v", err) } + if !strings.Contains(capturedQuery, "type=") { + t.Fatalf("captured query = %q, want inferred type query param", capturedQuery) + } data := decodeDriveEnvelope(t, stdout)🤖 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/drive/drive_member_add_test.go` around lines 674 - 728, The test doesn't assert the outgoing request query so a regression removing the inferred type query param won't be caught; modify TestDriveMemberAdd_ExecuteBatchSuccess to capture the HTTP request's URL.RawQuery when the stub is invoked (e.g. register a handler/closure with reg or httpmock that saves the *http.Request into a local variable before returning the stub response) and after mountAndRunDrive assert the captured RawQuery contains the expected "type=<resource>" value for token "bascnTok" (keep the rest of the test assertions intact); reference TestDriveMemberAdd_ExecuteBatchSuccess, the existing stub registration, and the mountAndRunDrive call to locate where to add the request capture and subsequent assertion.
🤖 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.
Duplicate comments:
In `@shortcuts/drive/drive_member_add_test.go`:
- Around line 867-910: Add an assertion in
TestDriveMemberAdd_ExecuteSuccessAsBot to verify the HTTP request query does NOT
include need_notification for the "--as bot" case: after mountAndRunDrive and
after decoding stub.CapturedBody keep the existing body checks, then inspect the
request captured by the stub (e.g., stub.CapturedRequest.URL.RawQuery or parse
stub.CapturedRequest.URL.Query()) and fail the test if the "need_notification"
parameter is present or non-empty; reference the test function
TestDriveMemberAdd_ExecuteSuccessAsBot and the stub variable to locate where to
add this check.
In `@shortcuts/drive/drive_member_add.go`:
- Around line 443-496: The function buildDriveMemberAddBatchResult currently
backfills member_id via driveMemberAddOutputWithOptions before deciding success
and builds the members slice only from rawMembers, causing missing/short
responses to be counted as successes and to drop slots for requested IDs; fix
by: iterate over spec.MemberIDs as the primary loop, for each index obtain raw
entry from rawMembers if present (raw := rawMembers[i] else nil), extract
rawMemberID directly from the raw map (do not rely on the backfilled out
member_id) to determine success and populate succeededIDs only when rawMemberID
== requestedID, call driveMemberAddOutputWithOptions to build the output but
ensure you create placeholder outputs for missing/raw-invalid entries so the
resulting members slice has one entry per requested ID, and recompute
partial/missing/mismatched accordingly (refer to buildDriveMemberAddBatchResult,
spec.MemberIDs, rawMembers, driveMemberAddOutputWithOptions, and the "member_id"
field).
---
Nitpick comments:
In `@shortcuts/drive/drive_member_add_test.go`:
- Around line 674-728: The test doesn't assert the outgoing request query so a
regression removing the inferred type query param won't be caught; modify
TestDriveMemberAdd_ExecuteBatchSuccess to capture the HTTP request's
URL.RawQuery when the stub is invoked (e.g. register a handler/closure with reg
or httpmock that saves the *http.Request into a local variable before returning
the stub response) and after mountAndRunDrive assert the captured RawQuery
contains the expected "type=<resource>" value for token "bascnTok" (keep the
rest of the test assertions intact); reference
TestDriveMemberAdd_ExecuteBatchSuccess, the existing stub registration, and the
mountAndRunDrive call to locate where to add the request capture and subsequent
assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af292443-0256-4d73-88ac-18ece2f48ca5
📒 Files selected for processing (7)
shortcuts/common/resource_url.goshortcuts/common/resource_url_test.goshortcuts/drive/drive_member_add.goshortcuts/drive/drive_member_add_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-member-add.mdtests/cli_e2e/drive/drive_member_add_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- skills/lark-drive/SKILL.md
- tests/cli_e2e/drive/drive_member_add_dryrun_test.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/cli_e2e/drive/drive_member_add_dryrun_test.go`:
- Around line 354-359: The file contains duplicate trailing closing braces after
the test TestDrive_MemberAddDryRunRejectsInvalidInputs which cause a compile
parse error; remove the extra duplicated tokens — the stray "})", "}", "}" that
appear after the legitimate closing of t.Run/for/function so that only the
original three closers remain and the file ends immediately after the proper
function closing for TestDrive_MemberAddDryRunRejectsInvalidInputs.
🪄 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: 41aed911-639c-4f2c-a36a-f21c4d9163d8
📒 Files selected for processing (1)
tests/cli_e2e/drive/drive_member_add_dryrun_test.go
- Fix batch result validation: use rawMemberID instead of backfilled memberID to avoid miscounting partial failures - Add need_notification query assertion in bot execution test - Add batch query contract assertion (type=bitable for bascn token) - Check combined stdout+stderr in dry-run rejection tests - Merge adjacent blockquote paragraphs to fix markdownlint MD028
Comment on lines
+141
to
+142
| var tokenPrefixToType = []struct { | ||
| Prefix string |
Harden drive +member-add after reviewing bot identity constraints and batch result handling. Reject department collaborator grants before hitting the API when the shortcut runs with bot identity, and treat batch responses as unordered member_id sets so successful grants are not misreported as partial failures. Key fixes: - Reject --member-type=opendepartmentid with --as bot and point callers to --as user - Document that department collaborator authorization requires user identity - Match batch_create results by returned member_id instead of response array index - Keep partial_failure reporting for missing or unexpected returned member IDs - Add unit and dry-run E2E coverage for the validation and unordered response cases
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
3 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 drive +member-add shortcut to grant collaborator permissions on Drive documents, files, folders, and wiki nodes. Supports single and batch member add (up to 10).
Changes
• New shortcut shortcuts/drive/drive_member_add.go — implements +member-add command with --member-type, --perm, --perm-type flags; auto-infers resource type from Feishu/Lark/Doubao URL or token prefix
• Extended resource parsing shortcuts/common/resource_url.go — added InferResourceTypeFromToken and URL parsing support
• Shortcut registration shortcuts/drive/shortcuts.go — registered +member-add in the drive command group
• Unit tests shortcuts/drive/drive_member_add_test.go — covers single/batch add, bot execution, flag validation, error handling
• E2E tests tests/cli_e2e/drive/drive_member_add_dryrun_test.go — dry-run end-to-end tests covering various input combinations
• Documentation skills/lark-drive/SKILL.md and skills/lark-drive/references/lark-drive-member-add.md — updated skill docs and new command reference
• Review fixes — fixed batch result validation (use rawMemberID instead of backfilled memberID), added query contract assertions, merged adjacent blockquotes to fix markdownlint MD028
Test Plan
[✓] All unit tests pass
[✓] All E2E dry-run tests pass (16/16)
[✓] All CI checks pass (19/19)
Related Issues
None
Summary by CodeRabbit
drive +member-addshortcut for granting collaborator permissions to Drive resources (docs, folders, spaces, wiki nodes)