feat(drive): add wiki url/token support to +download and +preview - #2220
feat(drive): add wiki url/token support to +download and +preview#2220zzzchao123 wants to merge 1 commit into
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:
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDrive preview and download now accept file tokens, Drive URLs, and Wiki node references. Wiki nodes resolve to Drive file tokens before API calls. Validation, dry-run plans, output annotations, tests, and documentation cover the new inputs. ChangesDrive Wiki source support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant DriveCommand
participant WikiAPI
participant DriveAPI
CLI->>DriveCommand: provide file token, Drive URL, or Wiki reference
DriveCommand->>WikiAPI: get_node for Wiki source
WikiAPI-->>DriveCommand: resolved Drive file token and node metadata
DriveCommand->>DriveAPI: preview or download with resolved token
DriveAPI-->>CLI: result with Wiki metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 ❌ Your patch check has failed because the patch coverage (48.85%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2220 +/- ##
==========================================
+ Coverage 76.21% 76.29% +0.07%
==========================================
Files 986 994 +8
Lines 104268 106842 +2574
==========================================
+ Hits 79472 81513 +2041
- Misses 18752 19113 +361
- Partials 6044 6216 +172 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@262a04b39a46555644efb780065dafcb4168bd8c🧩 Skill updatenpx skills add larksuite/cli#feat/drive-download-preview-wiki -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: 3
🧹 Nitpick comments (6)
shortcuts/drive/drive_file_source.go (1)
176-186: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
annotateDriveFileWikiOutputmutates the caller map in place.The function writes into
outand also returns it. The dual style invites confusion at call sites, because callers may assume the input stays unchanged. The unit test atshortcuts/drive/drive_file_source_test.golines 100-105 works only because the unresolved branch returns early. Consider documenting the in-place mutation in the doc comment, or copying the map.♻️ Proposed doc clarification
// annotateDriveFileWikiOutput echoes the wiki resolution into the command -// output so callers can trace which node produced the file token. +// output so callers can trace which node produced the file token. It mutates +// out in place and returns it for call-site chaining. func annotateDriveFileWikiOutput(out map[string]interface{}, resolution driveFileWikiResolution) map[string]interface{} {🤖 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_file_source.go` around lines 176 - 186, Document in annotateDriveFileWikiOutput that the provided out map is mutated in place when resolution.Resolved is true, while preserving the existing unresolved early return and output behavior.shortcuts/drive/drive_file_source_test.go (1)
99-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit coverage for
resolveDriveFileWikiSourcerejection paths.The tests cover normalization and annotation. They do not cover the two new rejection branches in
shortcuts/drive/drive_file_source.go: incompleteget_nodedata (lines 147-154) and a non-fileobj_type(lines 155-163). A revert of either branch would not fail any test. Add table tests that feed a mockedget_noderesponse and assert the typed metadata, including the+exporthint for non-file nodes.Do you want me to draft these tests?
As per coding guidelines, "Every behavior change must have an accompanying test, and contract tests must assert the changed field or behavior directly so reverting the implementation causes failure".
🤖 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_file_source_test.go` around lines 99 - 123, Add table-driven unit tests for resolveDriveFileWikiSource covering get_node responses with incomplete metadata and non-file obj_type values. Mock the get_node response, assert the returned typed metadata and rejection behavior directly, and verify non-file nodes include the +export hint so reverting either rejection branch fails the tests.Source: Coding guidelines
tests/cli_e2e/drive/drive_download_dryrun_test.go (1)
118-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the dry-run step descriptions for the wiki flow.
shortcuts/drive/drive_download.golines 155-182 introduce dynamic step numbering. This test checks only URLs and the wiki token. If thestep++at line 164 were reverted, the assertions would still pass. Addapi.0.desc,api.1.desc, andapi.2.descassertions so the numbering contract fails on regression. The existing tests at lines 54 and 90 already follow this pattern.As per coding guidelines, "contract tests must assert the changed field or behavior directly so reverting the implementation causes failure".
🤖 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/drive/drive_download_dryrun_test.go` around lines 118 - 138, Extend the wiki dry-run assertions in the test around DryRunGet to validate api.0.desc, api.1.desc, and api.2.desc using the expected step descriptions defined by the drive download flow. Keep the existing URL and token assertions, and ensure the descriptions directly verify the dynamic step numbering introduced by the implementation.Source: Coding guidelines
shortcuts/drive/drive_preview.go (1)
63-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the dry-run step numbering with
drive +download.
planWikiResolutionlabels the wiki step[0], and the preview steps keep[1]and[2]. Two inconsistencies follow. First, without a wiki source the preview steps are[1]/[2]at plan indices 0/1, so the label does not track the plan position. Second,shortcuts/drive/drive_download.golines 155-182 use a dynamicstepcounter that starts at 1 and renumbers later steps. Use the same dynamic counter here so both commands present one numbering scheme.♻️ Proposed approach
- // planWikiResolution prepends the wiki get_node step (and echoes the - // wiki token) so the preview steps are numbered after it. - planWikiResolution := func(dry *common.DryRunAPI) { - if !wiki { - return - } - dry.GET("/open-apis/wiki/v2/spaces/get_node"). - Desc("[0] Resolve wiki node to the underlying Drive file token (obj_type must be file)"). - Params(map[string]interface{}{"token": source.WikiToken}) - dry.Set("wiki_token", source.WikiToken) - } + // planWikiResolution prepends the wiki get_node step (and echoes the + // wiki token) and returns the number of the next preview step. + step := 1 + planWikiResolution := func(dry *common.DryRunAPI) { + if !wiki { + return + } + dry.GET("/open-apis/wiki/v2/spaces/get_node"). + Desc(fmt.Sprintf("[%d] Resolve wiki node to the underlying Drive file token (obj_type must be file)", step)). + Params(map[string]interface{}{"token": source.WikiToken}) + dry.Set("wiki_token", source.WikiToken) + step++ + }Then build the preview descriptions with
fmt.Sprintf("[%d] ...", step)and incrementstepafter each planned request. Update the assertions intests/cli_e2e/drive/drive_preview_dryrun_test.goaccordingly.Also applies to: 99-101
🤖 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_preview.go` around lines 63 - 73, Align drive preview dry-run numbering with drive download by introducing a dynamic step counter in the preview planning flow, starting at 1 and incrementing after each planned request, including the wiki resolution in planWikiResolution. Replace hard-coded [0], [1], and [2] descriptions with fmt.Sprintf-based labels tied to that counter, and update the related dry-run assertions in drive_preview_dryrun_test.go.tests/cli_e2e/drive/drive_preview_dryrun_test.go (1)
186-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a rejection-path dry-run test for
drive +preview.The download suite covers mutually exclusive source flags in
tests/cli_e2e/drive/drive_download_dryrun_test.golines 175-195. The preview suite has no equivalent.shortcuts/drive/drive_preview.goline 36 shares the same validation, and preview also rejects non-file URLs such as/docx/. Add two tests: one that passes--urlplus--wiki-tokenand expects exit code 2 withinvalid_argument, and one that passes a/docx/URL and expects the same typed rejection with the+exporthint.Do you want me to draft these tests?
As per coding guidelines, "Every behavior change must have an accompanying test, and contract tests must assert the changed field or behavior directly so reverting the implementation causes failure".
🤖 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/drive/drive_preview_dryrun_test.go` around lines 186 - 217, Add rejection-path dry-run tests alongside TestDrivePreviewDryRun_WikiTokenSourceFile: cover mutually exclusive --url with --wiki-token and a non-file /docx/ URL. For both cases, assert exit code 2 and the typed invalid_argument response; for the /docx/ case also assert the message includes the +export hint, reusing the existing dry-run setup and command helpers.Source: Coding guidelines
shortcuts/drive/drive_download.go (1)
191-206: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove wiki resolution after the local overwrite pre-check.
resolveDriveFileWikiSourceperforms aget_nodeAPI call at line 199. The local pre-checks at lines 211-218 run after it. If--outputalready exists and--overwriteis absent, the command spends one API call and prints "Resolving wiki node" to stderr before it fails withoutput file already exists. Move the resolution block after the output-path and existence checks so local validation fails first.♻️ Proposed reordering
source, err := normalizeDriveFileSource(runtime.Str("file-token"), runtime.Str("url"), runtime.Str("wiki-token")) if err != nil { return err } - fileToken := source.FileToken - var wikiResolution driveFileWikiResolution - if source.NeedsWikiResolution() { - resolvedToken, resolution, resolveErr := resolveDriveFileWikiSource(ctx, runtime, source) - if resolveErr != nil { - return resolveErr - } - fileToken = resolvedToken - wikiResolution = resolution - } - outputPath := runtime.Str("output") overwrite := runtime.Bool("overwrite") // Early path validation + overwrite check if outputPath != "" { if _, resolveErr := runtime.ResolveSavePath(outputPath); resolveErr != nil { return errs.NewValidationError(errs.SubtypeInvalidArgument, "unsafe output path: %s", resolveErr).WithParam("--output") } if _, statErr := runtime.FileIO().Stat(outputPath); statErr == nil && !overwrite { return errs.NewValidationError(errs.SubtypeInvalidArgument, "output file already exists: %s (use --overwrite to replace)", outputPath).WithParam("--output") } } + + fileToken := source.FileToken + var wikiResolution driveFileWikiResolution + if source.NeedsWikiResolution() { + resolvedToken, resolution, resolveErr := resolveDriveFileWikiSource(ctx, runtime, source) + if resolveErr != nil { + return resolveErr + } + fileToken = resolvedToken + wikiResolution = resolution + }🤖 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_download.go` around lines 191 - 206, Move the wiki-resolution block that calls resolveDriveFileWikiSource out of its current position in the download flow and place it after the local output-path and existing-file checks. Keep source normalization and local overwrite validation ahead of any API resolution, while preserving the existing fileToken and wikiResolution assignments once validation succeeds.
🤖 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_file_source_test.go`:
- Around line 67-79: Update shortcuts/drive/drive_file_source_test.go lines
67-79 in the error-path table tests to add wantErrParam, assert problem.Category
and problem.Subtype from errs.ProblemOf, and use errors.As to verify the
*errs.ValidationError Param while preserving cause validation. Update
tests/cli_e2e/drive/drive_download_dryrun_test.go lines 191-194 to assert
Validate-stage failures exit with code 2, emit stderr containing error.type,
error.param, and error.message, and leave result.Stdout empty.
In `@skills/lark-drive/references/lark-drive-download.md`:
- Around line 29-34: 在参数表中补充已由 drive_download.go 的命令注册支持的 --overwrite
参数，注明其为可选布尔参数并用于覆盖已存在的输出文件；保持现有参数顺序和表格格式不变。
In `@tests/cli_e2e/drive/drive_download_dryrun_test.go`:
- Around line 191-194: Expand the rejection assertions in the dry-run test
around result.AssertExitCode to validate error.type, error.subtype, error.param,
and error.message from result.Stderr, plus assert that result.Stdout is empty.
Preserve the existing exit-code check and use the established DryRunGet parsing
flow for the typed validation envelope.
---
Nitpick comments:
In `@shortcuts/drive/drive_download.go`:
- Around line 191-206: Move the wiki-resolution block that calls
resolveDriveFileWikiSource out of its current position in the download flow and
place it after the local output-path and existing-file checks. Keep source
normalization and local overwrite validation ahead of any API resolution, while
preserving the existing fileToken and wikiResolution assignments once validation
succeeds.
In `@shortcuts/drive/drive_file_source_test.go`:
- Around line 99-123: Add table-driven unit tests for resolveDriveFileWikiSource
covering get_node responses with incomplete metadata and non-file obj_type
values. Mock the get_node response, assert the returned typed metadata and
rejection behavior directly, and verify non-file nodes include the +export hint
so reverting either rejection branch fails the tests.
In `@shortcuts/drive/drive_file_source.go`:
- Around line 176-186: Document in annotateDriveFileWikiOutput that the provided
out map is mutated in place when resolution.Resolved is true, while preserving
the existing unresolved early return and output behavior.
In `@shortcuts/drive/drive_preview.go`:
- Around line 63-73: Align drive preview dry-run numbering with drive download
by introducing a dynamic step counter in the preview planning flow, starting at
1 and incrementing after each planned request, including the wiki resolution in
planWikiResolution. Replace hard-coded [0], [1], and [2] descriptions with
fmt.Sprintf-based labels tied to that counter, and update the related dry-run
assertions in drive_preview_dryrun_test.go.
In `@tests/cli_e2e/drive/drive_download_dryrun_test.go`:
- Around line 118-138: Extend the wiki dry-run assertions in the test around
DryRunGet to validate api.0.desc, api.1.desc, and api.2.desc using the expected
step descriptions defined by the drive download flow. Keep the existing URL and
token assertions, and ensure the descriptions directly verify the dynamic step
numbering introduced by the implementation.
In `@tests/cli_e2e/drive/drive_preview_dryrun_test.go`:
- Around line 186-217: Add rejection-path dry-run tests alongside
TestDrivePreviewDryRun_WikiTokenSourceFile: cover mutually exclusive --url with
--wiki-token and a non-file /docx/ URL. For both cases, assert exit code 2 and
the typed invalid_argument response; for the /docx/ case also assert the message
includes the +export hint, reusing the existing dry-run setup and command
helpers.
🪄 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: ce4e5219-f5bb-41fd-b738-11e8da251ece
📒 Files selected for processing (9)
shortcuts/drive/drive_download.goshortcuts/drive/drive_file_source.goshortcuts/drive/drive_file_source_test.goshortcuts/drive/drive_preview.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-download.mdskills/lark-drive/references/lark-drive-preview.mdtests/cli_e2e/drive/drive_download_dryrun_test.gotests/cli_e2e/drive/drive_preview_dryrun_test.go
b65776e to
a3e0167
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: 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/drive/drive_file_source_test.go`:
- Around line 55-60: Extend the mutually exclusive input table in the drive file
source tests to cover both missing pairs: --file-token with --url and --url with
--wiki-token. Assert the same invalid-argument error subtype and the relevant
conflicting flag for each case, preserving the existing
--file-token/--wiki-token coverage.
- Around line 117-120: Update the unresolved assertion in the test around
annotateDriveFileWikiOutput to verify that the "wiki_token" key is absent, using
the two-value map lookup rather than checking whether its value is nil. Preserve
the existing failure message and ensure outputs containing the key with a nil
value fail the test.
🪄 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: 49972bae-4792-4262-b523-9ea6d5f11124
📒 Files selected for processing (9)
shortcuts/drive/drive_download.goshortcuts/drive/drive_file_source.goshortcuts/drive/drive_file_source_test.goshortcuts/drive/drive_preview.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-download.mdskills/lark-drive/references/lark-drive-preview.mdtests/cli_e2e/drive/drive_download_dryrun_test.gotests/cli_e2e/drive/drive_preview_dryrun_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/cli_e2e/drive/drive_preview_dryrun_test.go
- skills/lark-drive/SKILL.md
- tests/cli_e2e/drive/drive_download_dryrun_test.go
- skills/lark-drive/references/lark-drive-download.md
- skills/lark-drive/references/lark-drive-preview.md
- shortcuts/drive/drive_preview.go
- shortcuts/drive/drive_download.go
- shortcuts/drive/drive_file_source.go
wittam-01
left a comment
wittam-01
left a comment
There was a problem hiding this comment.
整体实现方向合理：共享 Wiki source resolver、按需声明 scope、仅允许 Wiki 下挂 Drive file，这些设计都符合目标。
合并前还需要处理：
- [P2] URL token 校验不一致：
shortcuts/drive/drive_file_source.go的--url分支解析出ref.Token后直接返回，没有像--file-token/--wiki-token一样调用validate.ResourceName。这会让..、非法字符或危险 Unicode 通过 URL 分支。建议 URL parse 后统一校验，并补 file/wiki URL 非法 token 用例。
非阻断建议：
- Skill 中“download/preview 直接接受 Wiki”与“遇到 Wiki 先
+inspect”的引导有些冲突。建议只有在尚未决定下游命令时才引导 inspect，并减少obj_token/obj_type等内部实现说明。 - 新 resolver 与 export resolver 有少量重复；可以只抽取最小的 Wiki 节点解析/注解能力，不建议为此扩大本 PR 重构范围。
另有一个非本 PR 引入的问题：流式 download/preview 遇到 HTTP 429 时，DoStream 会将其归为 network/transport，没有 rate-limit subtype、retryable 或退避提示。建议后续独立修复，不作为本 PR 的归责项。
| > **前置条件：** 先阅读 [`../lark-shared/SKILL.md`](../../lark-shared/SKILL.md) 了解认证、全局参数和安全规则。 | ||
|
|
||
| 从飞书云空间（云盘/云存储）下载文件到本地。 | ||
| 从飞书云空间（云盘/云存储）下载文件到本地。只处理 Drive **文件**（上传的 PDF/zip/图片/音视频等文件）。 |
There was a problem hiding this comment.
不要提drive，防止wiki的场景，直接被忽略了。 需要自然语言验证下，看wiki url的下载是否能正常处理，是否需要在wiki主SKILL引导
a3e0167 to
2268548
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. |
2268548 to
262a04b
Compare
Extend +download and +preview to accept --url and --wiki-token in addition to --file-token (mutually exclusive). Wiki nodes are resolved to their underlying object; only file-backed nodes are supported, and non-file documents (docx/sheet/bitable/slides) return a typed validation error hinting to use +export.
262a04b to
2a4d134
Compare
2 participants
Summary
Extend +download and +preview to accept --url and --wiki-token in addition to --file-token (mutually exclusive). Wiki nodes are resolved to their underlying object; only file-backed nodes are supported, and non-file documents (docx/sheet/bitable/slides) return a typed validation error hinting to use +export.
Changes
Test Plan
lark-cli <domain> <command>flow works as expectedRelated Issues
Summary by CodeRabbit
New Features
Bug Fixes
Documentation