feat: preflight export permission for downloads - #2218
Conversation
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 Plus Run ID: 📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughDrive and document downloads now check export permission with the current runtime identity. Dry-run plans include the authorization request. Permission, HTTP 403, and rate-limit errors receive typed handling and recovery hints. Whiteboard downloads skip the permission check. ChangesDrive permission authorization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DownloadShortcut
participant DrivePermissionAPI
participant MetadataAPI
participant DownloadAPI
DownloadShortcut->>DrivePermissionAPI: Check export permission
DrivePermissionAPI-->>DownloadShortcut: Return auth_result
DownloadShortcut->>MetadataAPI: Fetch metadata when required
DownloadShortcut->>DownloadAPI: Download file or media
Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
skills/lark-drive/references/lark-drive-download.md (1)
8-9: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the internal metadata-ordering detail from the caller guidance.
The sentence describes when the CLI queries metadata and that it happens only after authorization passes. That is internal resolution logic. Keep the caller-facing facts: the export preflight with the current
--asidentity, and the required scopesdrive:file:downloadanddocs:permission.member:auth.♻️ Proposed wording
-命令会先用当前 `--as` 身份校验目标文件的 `export` 权限；只有鉴权通过后才查询 metadata（省略 `--output` 时）并下载。所需权限包括 `drive:file:download` 和 `docs:permission.member:auth`。 +命令会先用当前 `--as` 身份校验目标文件的 `export` 权限，鉴权通过后才下载。所需权限包括 `drive:file:download` 和 `docs:permission.member:auth`。Based on learnings: "avoid exposing internal implementation/compatibility details—e.g., how
drive +downloadfilename-resolution precedence is handled or any conditional metadata-scope behavior."🤖 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 `@skills/lark-drive/references/lark-drive-download.md` around lines 8 - 9, Update the command guidance sentence to describe only the export-permission preflight using the current --as identity and the required scopes drive:file:download and docs:permission.member:auth. Remove the details about when metadata is queried and its dependency on authorization, while preserving the download behavior and caller-facing permission requirements.Source: Learnings
tests/cli_e2e/docs/docs_media_download_dryrun_test.go (1)
15-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLive E2E coverage is missing for the new export preflight on both download flows. The PR adds an export-permission request before
docs +media-downloadanddrive +download, but the new E2E coverage asserts dry-run plans only. No test runs the preflight against the real API.
tests/cli_e2e/docs/docs_media_download_dryrun_test.go#L15-L75: add a live test that downloads a real media token with bot credentials, and a live case that confirms--type whiteboardstill succeeds without thedocs:permission.member:authscope.tests/cli_e2e/drive/drive_download_dryrun_test.go#L33-L67: add a live test that downloads a real Drive file token with bot credentials, covering both the explicit--outputpath and the omitted--outputpath.As per path instructions: "New flows or behavior changes require live E2E coverage with a self-contained create/use/cleanup workflow and bot credentials where applicable."
🤖 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/docs/docs_media_download_dryrun_test.go` around lines 15 - 75, Extend tests/cli_e2e/docs/docs_media_download_dryrun_test.go:15-75 with live, self-contained bot-credential coverage that creates and cleans up a real media download, plus a whiteboard download proving it succeeds without the docs:permission.member:auth scope; retain the existing dry-run tests. Also extend tests/cli_e2e/drive/drive_download_dryrun_test.go:33-67 with live create/use/cleanup coverage for a real Drive file token, testing both explicit --output and omitted --output paths.Source: Path instructions
🤖 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.
Nitpick comments:
In `@skills/lark-drive/references/lark-drive-download.md`:
- Around line 8-9: Update the command guidance sentence to describe only the
export-permission preflight using the current --as identity and the required
scopes drive:file:download and docs:permission.member:auth. Remove the details
about when metadata is queried and its dependency on authorization, while
preserving the download behavior and caller-facing permission requirements.
In `@tests/cli_e2e/docs/docs_media_download_dryrun_test.go`:
- Around line 15-75: Extend
tests/cli_e2e/docs/docs_media_download_dryrun_test.go:15-75 with live,
self-contained bot-credential coverage that creates and cleans up a real media
download, plus a whiteboard download proving it succeeds without the
docs:permission.member:auth scope; retain the existing dry-run tests. Also
extend tests/cli_e2e/drive/drive_download_dryrun_test.go:33-67 with live
create/use/cleanup coverage for a real Drive file token, testing both explicit
--output and omitted --output paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 724f3e38-b56e-4025-a416-242ffee6d52c
📒 Files selected for processing (12)
shortcuts/common/drive_permission_auth.goshortcuts/common/drive_permission_auth_test.goshortcuts/doc/doc_errors.goshortcuts/doc/doc_media_download.goshortcuts/doc/doc_media_test.goshortcuts/drive/drive_download.goshortcuts/drive/drive_errors.goshortcuts/drive/drive_io_test.goskills/lark-doc/references/lark-doc-media-download.mdskills/lark-drive/references/lark-drive-download.mdtests/cli_e2e/docs/docs_media_download_dryrun_test.gotests/cli_e2e/drive/drive_download_dryrun_test.go
wittam-01
left a comment
wittam-01
left a comment
There was a problem hiding this comment.
Review summary
The main implementation path looks sound: the preflight uses the current runtime identity, validates auth_result strictly, preserves typed error metadata, runs before Drive metadata/download calls, and skips the new check for whiteboards.
I recommend addressing the P1 before merge, plus the two P2 contract/guidance issues below.
P1: add live E2E coverage for docs +media-download
tests/cli_e2e/docs/docs_media_download_dryrun_test.go:15 only verifies the dry-run plan. The actual flow now makes a real permission request before downloading, while the repository AGENTS.md requires self-contained live E2E for behavior changes. Dry-run coverage cannot prove that the current identity, auth_result, and subsequent media download work together against the backend.
Action: add an opt-in, self-contained create/use/cleanup live workflow that at least covers the allowed export → successful download path. If the denied branch cannot be self-contained, gate it behind an explicit fixture. Update tests/cli_e2e/docs/coverage.md at the same time.
This overlaps CodeRabbits live-E2E note; I agree with that finding and treat the Docs gap as merge-blocking.
P2: make the Drive fallback command executable
skills/lark-drive/references/lark-drive-download.md:32 shows drive +preview --type source_file --output <path>, but --file-token is required.
Action: use the complete command:
lark-cli drive +preview --file-token "<FILE_TOKEN>" --type source_file --output <path>P2: prevent silent resourceType / action swaps
shortcuts/common/drive_permission_auth.go:25 declares both values as string; the dry-run helper repeats the same contract. Swapping them still compiles and sends an invalid permission query, contrary to the repository distinct-types convention.
Action: either introduce separate named types for resource type and action, or simplify these helpers to the only currently supported file/export operation.
Verified against head 5799b852; git diff --check and gofmt -d were clean. No local test/build rerun was performed as part of this review.
5799b85 to
3c2bb93
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
🤖 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/common/drive_permission_auth.go`:
- Around line 39-45: Replace the direct data["auth_result"] assertion in the
Drive permission auth response handling with a typed response struct and a
single projection function for this API shape. Have the projection validate that
auth_result exists and is boolean, while preserving the existing
errs.SubtypeInvalidResponse error for invalid responses.
🪄 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: 05d1e4ce-47aa-48ee-9f59-7add46606878
📒 Files selected for processing (14)
shortcuts/common/drive_permission_auth.goshortcuts/common/drive_permission_auth_test.goshortcuts/doc/doc_errors.goshortcuts/doc/doc_media_download.goshortcuts/doc/doc_media_test.goshortcuts/drive/drive_download.goshortcuts/drive/drive_errors.goshortcuts/drive/drive_io_test.goskills/lark-doc/references/lark-doc-media-download.mdskills/lark-drive/references/lark-drive-download.mdtests/cli_e2e/docs/coverage.mdtests/cli_e2e/docs/docs_media_download_dryrun_test.gotests/cli_e2e/docs/docs_media_download_workflow_test.gotests/cli_e2e/drive/drive_download_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- tests/cli_e2e/drive/drive_download_dryrun_test.go
- shortcuts/doc/doc_errors.go
- shortcuts/common/drive_permission_auth_test.go
- shortcuts/drive/drive_errors.go
- shortcuts/doc/doc_media_download.go
- shortcuts/doc/doc_media_test.go
- tests/cli_e2e/docs/docs_media_download_dryrun_test.go
- shortcuts/drive/drive_io_test.go
- shortcuts/drive/drive_download.go
3c2bb93 to
e9fe398
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.
🧹 Nitpick comments (1)
shortcuts/drive/drive_io_test.go (1)
1832-1854: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert that the file token does not leak into the hint.
The other recovery-hint tests verify that the hint contains a placeholder instead of the real token. This test passes
"file_secret"but never checks for its absence. Add the negative assertion so a regression that interpolates the token fails here too.🛡️ Proposed added assertion
if strings.Contains(problem.Hint, "1 minute") { t.Fatalf("hint=%q, want no fixed retry duration", problem.Hint) } + if strings.Contains(problem.Hint, "file_secret") { + t.Fatalf("hint=%q, want no raw file token", problem.Hint) + } }🤖 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_io_test.go` around lines 1832 - 1854, Update TestDriveDownloadTypedRateLimitSuggestsBackoff to assert that problem.Hint does not contain the input token "file_secret", alongside the existing hint-content checks, so the recovery hint preserves the placeholder behavior.
🤖 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.
Nitpick comments:
In `@shortcuts/drive/drive_io_test.go`:
- Around line 1832-1854: Update TestDriveDownloadTypedRateLimitSuggestsBackoff
to assert that problem.Hint does not contain the input token "file_secret",
alongside the existing hint-content checks, so the recovery hint preserves the
placeholder behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d24b25c-ba5f-4d8f-b389-dcdcfd7d1e5c
📒 Files selected for processing (14)
shortcuts/common/drive_permission_auth.goshortcuts/common/drive_permission_auth_test.goshortcuts/doc/doc_errors.goshortcuts/doc/doc_media_download.goshortcuts/doc/doc_media_test.goshortcuts/drive/drive_download.goshortcuts/drive/drive_errors.goshortcuts/drive/drive_io_test.goskills/lark-doc/references/lark-doc-media-download.mdskills/lark-drive/references/lark-drive-download.mdtests/cli_e2e/docs/coverage.mdtests/cli_e2e/docs/docs_media_download_dryrun_test.gotests/cli_e2e/docs/docs_media_download_workflow_test.gotests/cli_e2e/drive/drive_download_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/cli_e2e/drive/drive_download_dryrun_test.go
- skills/lark-drive/references/lark-drive-download.md
- shortcuts/drive/drive_errors.go
- tests/cli_e2e/docs/docs_media_download_dryrun_test.go
- shortcuts/common/drive_permission_auth.go
- tests/cli_e2e/docs/docs_media_download_workflow_test.go
- skills/lark-doc/references/lark-doc-media-download.md
- shortcuts/drive/drive_download.go
- shortcuts/doc/doc_media_test.go
- shortcuts/doc/doc_errors.go
- shortcuts/common/drive_permission_auth_test.go
- shortcuts/doc/doc_media_download.go
fangshuyu-768
left a comment
fangshuyu-768
left a comment
There was a problem hiding this comment.
Reviewed the latest head e9fe398. No actionable findings; targeted unit tests, vet, build, dry-run E2E, and lint passed.
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@e89777c1279a8a283978c31df41ddecfd39f046c🧩 Skill updatenpx skills add wittam-01/cli#feat/download-export-permission-preflight -y -g |
Check Drive export permission before file and document media downloads, while preserving whiteboard behavior and adding actionable preview and rate-limit recovery hints. Spec source: active@ddbecbafcf0d68cc115faf3fcf0214fb6edc30b284147d33e5c9934e7bad82b4
e89777c
e9fe398 to
e89777c
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. |
Detect rate-limited Drive/Doc responses (SubtypeRateLimit, business code 99991400, or HTTP 429; excluding the 1063006 daily quota) and append exponential-backoff-with-jitter guidance to the typed error hint, so AI agents stop hammering and retry sensibly. Apply the recovery hint explicitly inside each drive shortcut's Execute via a defer, mirroring PR #2218's call-site style, instead of an implicit blanket decorator over the shortcut registry. Also cover the docs +resource-download cover path, which streams from the same Drive media download endpoint.
Detect rate-limited Drive/Doc responses (SubtypeRateLimit, business code 99991400, or HTTP 429; excluding the 1063006 daily quota) and append exponential-backoff-with-jitter guidance to the typed error hint, so AI agents stop hammering and retry sensibly. Apply the recovery hint explicitly inside each drive shortcut's Execute via a defer, mirroring PR #2218's call-site style, instead of an implicit blanket decorator over the shortcut registry. Also cover the docs +resource-download cover path, which streams from the same Drive media download endpoint.
Detect rate-limited Drive/Doc responses (SubtypeRateLimit, business code 99991400, or HTTP 429; excluding the 1063006 daily quota) and append exponential-backoff-with-jitter guidance to the typed error hint, so AI agents stop hammering and retry sensibly. Apply the recovery hint explicitly inside each drive shortcut's Execute via a defer, mirroring PR #2218's call-site style, instead of an implicit blanket decorator over the shortcut registry. Also cover the docs +resource-download cover path, which streams from the same Drive media download endpoint.
Detect rate-limited Drive/Doc responses (SubtypeRateLimit, business code 99991400, or HTTP 429; excluding the 1063006 daily quota) and append exponential-backoff-with-jitter guidance to the typed error hint, so AI agents stop hammering and retry sensibly. Apply the recovery hint explicitly inside each drive shortcut's Execute via a defer, mirroring PR #2218's call-site style, instead of an implicit blanket decorator over the shortcut registry. Also cover the docs +resource-download cover path, which streams from the same Drive media download endpoint.
3 participants
Summary
Preflight Drive export permission before direct file and document-media downloads so callers get an actionable preview fallback before attempting a download that cannot succeed.
Changes
docs:permission.member:authexport-permission preflight fordrive +downloadand non-whiteboarddocs +media-download.drive +preview --type source_fileordocs +media-previewrecovery hints when export is unavailable.Test Plan
shortcuts/common,shortcuts/drive, andshortcuts/doc.go vetpasses for the affected shortcut packages.permission_deniederrors with the expected preview hints and creates no output files.Related Issues
Summary by CodeRabbit
New Features
Documentation