feat(drive): add +apply-permission to request doc access#588
Open
caojie0621 wants to merge 4 commits intomainfrom
Open
feat(drive): add +apply-permission to request doc access#588caojie0621 wants to merge 4 commits intomainfrom
caojie0621 wants to merge 4 commits intomainfrom
Conversation
Wrap the POST /drive/v1/permissions/:token/members/apply endpoint as a user-only shortcut. --token accepts either a bare token or a document URL, with type auto-inferred from the URL path (/docx/, /sheets/, /base/, /bitable/, /file/, /wiki/, /doc/, /mindnote/, /minutes/, /slides/); an explicit --type always wins. --perm is limited to view or edit; full_access is rejected client-side to match the spec. Classifier gains two domain-specific hints for the endpoint's newly documented error codes: 1063006 (per-user-per-document quota of 5/day reached) and 1063007 (document does not accept apply requests — covers disallow-external-apply, already-has-access, and unsupported-type).
github-actions
Bot
added
domain/ccm
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new Drive shortcut command Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as "CLI\n(command)"
participant Validator as "Validator\n(resolvePermApplyTarget)"
participant APIClient as "API Client"
participant Server as "Drive API"
User->>CLI: drive +apply-permission --token <token|url> --perm <view|edit> [--type] [--remark]
CLI->>Validator: resolvePermApplyTarget(rawTokenOrURL, explicitType)
Validator-->>CLI: (token, docType) or error
alt validation succeeds
CLI->>CLI: build body {perm, optional remark}
CLI->>APIClient: POST /open-apis/drive/v1/permissions/:token/members/apply?type=docType
APIClient->>Server: Send request
Server-->>APIClient: 200 OK or error (1063006 / 1063007 ...)
APIClient-->>CLI: response
CLI-->>User: print status or classified error (rate_limit / invalid_params)
else validation fails
CLI-->>User: print validation error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@ff735712b208265cba0bfafd82c7a31416e1887d🧩 Skill updatenpx skills add larksuite/cli#feat/perm_apply -y -g |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #588 +/- ##
==========================================
+ Coverage 60.92% 61.12% +0.19%
==========================================
Files 399 401 +2
Lines 34089 34199 +110
==========================================
+ Hits 20770 20903 +133
+ Misses 11395 11370 -25
- Partials 1924 1926 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shortcuts/drive/drive_apply_permission_test.go (1)
218-239: Rate-limit test asserts onlyerr != nil; consider mirroringNotApplicableHint.The sibling
TestDriveApplyPermission_ExecuteNotApplicableHintverifies the surfaced server/hint text, but this test stops at non-nil error. Since the function name includesHint, asserting on the hint substring (e.g.,"5 times per day") would lock in that the classifier wiring forLarkErrDrivePermApplyRateLimitreaches the user.♻️ Suggested assertion
if err == nil { t.Fatal("expected error for 1063006") } + if !strings.Contains(err.Error(), "5 times per day") { + t.Fatalf("expected rate-limit hint, got: %v", err) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_apply_permission_test.go` around lines 218 - 239, TestDriveApplyPermission_ExecuteRateLimitHint currently only checks err != nil; update it to assert the returned error contains the rate-limit hint text (mirror the approach in TestDriveApplyPermission_ExecuteNotApplicableHint). After calling mountAndRunDrive with DriveApplyPermission, check that the error string includes the expected rate-limit hint substring (e.g., "5 times per day" or the canonical hint produced by the LarkErrDrivePermApplyRateLimit classifier) so the test verifies the classifier wiring surfaces the hint to the user.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/drive/drive_apply_permission_test.go`:
- Around line 218-239: TestDriveApplyPermission_ExecuteRateLimitHint currently
only checks err != nil; update it to assert the returned error contains the
rate-limit hint text (mirror the approach in
TestDriveApplyPermission_ExecuteNotApplicableHint). After calling
mountAndRunDrive with DriveApplyPermission, check that the error string includes
the expected rate-limit hint substring (e.g., "5 times per day" or the canonical
hint produced by the LarkErrDrivePermApplyRateLimit classifier) so the test
verifies the classifier wiring surfaces the hint to the user.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19903b7d-5e47-4c48-a516-02b4d7437f75
📒 Files selected for processing (8)
internal/output/lark_errors.gointernal/output/lark_errors_test.goshortcuts/drive/drive_apply_permission.goshortcuts/drive/drive_apply_permission_test.goshortcuts/drive/shortcuts.goshortcuts/drive/shortcuts_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-apply-permission.md
Invoke the real CLI binary via clie2e.RunCmd under --dry-run and parse the rendered request JSON with gjson to lock in method, URL path (including the token segment), type query parameter (auto-inferred for docx / sheet / slides URLs, taken from explicit --type for bare tokens), perm body field, and remark presence/omission. A separate test asserts --perm full_access is rejected by the enum validator before reaching the server. Fake LARKSUITE_CLI_APP_ID / APP_SECRET / BRAND are enough because dry-run short-circuits before any API call. Update drive coverage.md to add a row and refresh metrics.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cli_e2e/drive/drive_apply_permission_dryrun_test.go`:
- Around line 68-78: The test case labeled "bare token with explicit type wins
over inference" doesn't actually exercise URL-derived type override because the
args use a bare token; update the test row in
drive_apply_permission_dryrun_test.go so it either (A) supplies a URL-like token
(e.g., include a "--url" or a token that contains a host/path) alongside the
explicit "--type" to force an inference path to be present and ensure wantType
"bitable" overrides it, or (B) rename the test and its description to remove the
claim about URL-inference; modify the args array and/or the name string
accordingly and keep the expected fields wantURL, wantType, and wantPerm
consistent with the change.
- Around line 28-32: Add t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) to
isolate the CLI config for subprocesses: in the TestDrive_ApplyPermissionDryRun
test (and the other E2E test in this file that spawns the real CLI subprocess)
set the LARKSUITE_CLI_CONFIG_DIR environment variable to t.TempDir() at the
start of the test so the subprocess cannot read/write the developer's real CLI
config.
🪄 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: 59dfe81c-e8fe-4a38-9b50-298045c81df5
📒 Files selected for processing (2)
tests/cli_e2e/drive/coverage.mdtests/cli_e2e/drive/drive_apply_permission_dryrun_test.go
Set LARKSUITE_CLI_CONFIG_DIR to t.TempDir() in both +apply-permission dry-run tests so the subprocess can't read a developer's real credentials/profile instead of the fake env vars the tests inject.
Previous "bare token with explicit type wins over inference" row used a bare token, which has no URL-derived type to override. Replace it with a /docx/ URL + --type wiki combo that actually forces the explicit flag to win over URL inference, and add a separate bare-token row to keep the simpler path covered. Refresh coverage.md wording to match.
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
Wrap the
POST /open-apis/drive/v1/permissions/:token/members/applyendpoint as a new
drive +apply-permissionshortcut. The endpoint lets auser request
vieworeditaccess on a doc/sheet/file/wiki/bitable/docx(plus mindnote / minutes / slides) from its owner; the CLI previously
exposed no first-class path for it, forcing bare
lark-cli apicalls.Shortcut
drive +apply-permissiondocs:permission.member:apply["user"]— the endpoint rejectstenant_access_token, so bot identity is refused up-frontwrite(sends a push notification to the document owner)Flags
--token(required): accepts either a bare token or a document URL.URL path markers auto-infer
type:/docx/,/sheets/,/base/,/bitable/,/file/,/wiki/,/doc/,/mindnote/,/minutes/,/slides/.--type: optional when the URL shape is recognized; required with abare token. Explicit
--typealways wins over URL inference.--perm(required):vieworedit.full_accessis refusedclient-side to match the spec (the server also rejects it but catching
it locally avoids a wasted request and an opaque server error).
--remark: optional note attached to the request card shown to theowner; omitted from the body when empty.
Test plan
go test ./shortcuts/drive/ ./internal/output/Skill docs
skills/lark-drive/references/lark-drive-apply-permission.mdwithusage, parameter table, identity/scope requirements, rate-limit
callout, and full error-code table.
skills/lark-drive/SKILL.mdshortcut table gains a row for the newcommand.
Summary by CodeRabbit
New Features
drive +apply-permissionshortcut to request view/edit access from document owners, with optional remark and--dry-runpreview.Bug Fixes
Documentation
Tests