Skip to content

feat: add drive import, export, move, task result shortcuts#194

Merged
fangshuyu-768 merged 1 commit intomainfrom
feat/drive_dev
Apr 2, 2026
Merged

feat: add drive import, export, move, task result shortcuts#194
fangshuyu-768 merged 1 commit intomainfrom
feat/drive_dev

Conversation

@liujinkun2025
Copy link
Copy Markdown
Collaborator

@liujinkun2025 liujinkun2025 commented Apr 1, 2026

Summary

This PR adds a new set of Drive shortcuts to support document import, export,
move, and async task result queries in lark-cli. It also updates the related
skill documentation and registry metadata so the new capabilities are easier to
discover and use.

Changes

  • Add drive +import to upload local files and import them as docx, sheet, or bitable
  • Add drive +export to export doc, docx, sheet, and bitable files to local output with bounded polling
  • Add export download handling for completed export tasks
  • Add drive +move to move files and folders, including async polling for folder moves and root-folder fallback
  • Add drive +task_result to query async task status for import, export, and folder move scenarios
  • Update Drive and Wiki skill docs and refresh registry metadata for the new shortcuts and APIs

Test Plan

  • go test ./shortcuts/drive/...
  • Added or updated unit tests for import, export, move, and task result flows
  • Manual verification against live Drive APIs

Related Issues

  • None

Summary by CodeRabbit

  • New Features

    • Drive: export documents/sheets to local files (multiple formats; markdown direct flow; bounded polling with resumable timeout and follow-up command)
    • Drive: download exported files by token (file-name, output-dir, overwrite)
    • Drive: import local files to cloud docs (upload + async import with polling and resume)
    • Drive: move files/folders (sync file moves, async folder moves with polling and resume)
    • Drive: unified task-status query (+task_result) for import/export/move
  • Documentation

    • Added Drive command reference pages and updated Wiki API permissions/methods

@github-actions github-actions Bot added domain/ccm PR touches the ccm domain size/XL Architecture-level or global-impact change labels Apr 1, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 1, 2026

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds Drive CLI shortcuts: +export, +export-download, +import, +move, and +task_result. Implements validation, dry-run plans, multipart upload/download, async task creation with bounded polling and resume commands, filename sanitization/atomic save, tests, and documentation updates.

Changes

Cohort / File(s) Summary
Export & Download
shortcuts/drive/drive_export.go, shortcuts/drive/drive_export_common.go, shortcuts/drive/drive_export_common_test.go, shortcuts/drive/drive_export_download.go, shortcuts/drive/drive_export_test.go
Adds DriveExport and DriveExportDownload shortcuts. Implements markdown direct GET and async export task creation, polling with timeout and resume command generation, file-token download helper, filename sanitization/extension helpers, atomic write and overwrite enforcement, plus unit/integration tests.
Import
shortcuts/drive/drive_import.go, shortcuts/drive/drive_import_common.go, shortcuts/drive/drive_import_common_test.go, shortcuts/drive/drive_import_test.go
Adds DriveImport shortcut with local-file validation, multipart media upload, import task creation, polling with resume behavior, extension↔type validation, filename derivation helpers, DryRun scaffolding, and tests for validation, parsing, and timeout/resume flows.
Move
shortcuts/drive/drive_move.go, shortcuts/drive/drive_move_common.go, shortcuts/drive/drive_move_common_test.go, shortcuts/drive/drive_move_test.go
Adds DriveMove shortcut supporting synchronous file moves and asynchronous folder moves. Includes root-folder token resolution, move request builder, task-check polling utilities, validation, DryRun wiring, and tests for root resolution, success, and timeout/resume behavior.
Unified Task Result & Registry
shortcuts/drive/drive_task_result.go, shortcuts/drive/drive_task_result_test.go, shortcuts/drive/shortcuts.go, shortcuts/drive/shortcuts_test.go, shortcuts/drive/drive_io_test.go
Introduces DriveTaskResult (+task_result) to query import/export/task_check scenarios and normalize outputs; adds scenario-driven validation/DryRun/Execute. Registers new shortcuts in Shortcuts() and updates test infra (unique config IDs). Includes tests for scenario validation, DryRun, and execution paths.
Docs
skills/lark-drive/SKILL.md, skills/lark-drive/references/...
Adds documentation for new shortcuts and workflows (+export, +export-download, +import, +move, +task_result), examples, parameter tables, polling semantics, continuation flows, required scopes, and new API resource entries.
Wiki docs
skills/lark-wiki/SKILL.md
Updates wiki API resource index and permissions table (adds/adjusts spaces and nodes methods).

Sequence Diagram(s)

sequenceDiagram
    participant CLI as CLI (drive +export)
    participant API as Drive API (/open-apis/...)
    participant FS as Local Filesystem

    CLI->>API: POST /open-apis/drive/v1/export_tasks (create task)
    API-->>CLI: { ticket }
    CLI->>API: GET /open-apis/drive/v1/export_tasks/:ticket (poll)
    alt job not ready
        API-->>CLI: { job_status: processing }
        CLI->>CLI: wait (sleep) & retry
    else job ready
        API-->>CLI: { job_status: ready, file_token, filename }
        CLI->>API: GET /open-apis/drive/v1/export_tasks/file/:file_token/download
        API-->>CLI: [binary]
        CLI->>FS: saveContentToOutputDir(filename, overwrite?)
        FS-->>CLI: saved_path, size_bytes
    else job failed
        API-->>CLI: { job_status: failed, job_error_msg }
        CLI-->>CLI: return API error with ticket
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Suggested reviewers

  • liangshuo-1

Poem

🐇 I hopped through code to fetch and save the day,
Tickets and polls led the export on its way.
Filenames tidy, downloads snug and sweet,
Imports, moves, resume — my paws skip a beat.
A rabbit’s cheer: Drive shortcuts, hip hooray! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 48.39% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title clearly and concisely summarizes the main change: adding five new Drive shortcuts (import, export, move, task result) to the codebase.
Description check ✅ Passed The pull request description follows the template structure with Summary, Changes, and Test Plan sections, providing clear information about the new shortcuts and testing approach.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/drive_dev

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 1, 2026

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@b5c157a5c2a61de870a9e8692c99fbe217c3ef7b

🧩 Skill update

npx skills add larksuite/cli#feat/drive_dev -y -g

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented Apr 1, 2026

Greptile Summary

This PR adds five new Drive shortcuts (+import, +export, +export-download, +move, +task_result) with bounded async polling and resume-on-timeout flows for all three async scenarios. The implementation is well-structured, with shared helpers for status parsing, filename sanitization, and atomic file writes. All previous review concerns (misleading "success" label when all export polls fail, non-resilient pollDriveTaskCheck, commented-out scopes, undocumented empty mount_key behaviour) have been addressed.

Key findings from this pass:

  • Context cancellation not propagated to import/move polling loops (P1): pollDriveImportTask (drive_import_common.go) and pollDriveTaskCheck (drive_move_common.go) accept no context.Context and use bare time.Sleep. With 30 attempts × 2 s = 60 s maximum polling windows, a cancelled or timed-out parent context cannot interrupt these loops. The export polling loop in drive_export.go correctly uses select { case <-ctx.Done(): … case <-time.After(…): } — the same pattern should be applied here.
  • Inconsistent shell quoting in resume-command helpers (P2): driveExportTaskResultCommand, driveImportTaskResultCommand, and driveTaskCheckResultCommand embed tokens directly via %s without quoting, while driveExportDownloadCommand wraps every argument in strconv.Quote.

Confidence Score: 4/5

Safe to merge after the context-cancellation gap in import/move polling is resolved; all other issues are P2 style concerns.

One P1 remains: import and move polling loops ignore context cancellation, blocking for up to 60 s on a cancelled/timed-out operation. This is a real behavioral defect compared to the export loop which handles it correctly. No data-loss or security issues are present, and all prior P0/P1 concerns from earlier rounds have been fixed.

shortcuts/drive/drive_import_common.go (pollDriveImportTask — no ctx parameter, bare time.Sleep) and shortcuts/drive/drive_move_common.go (pollDriveTaskCheck — same problem)

Important Files Changed

Filename Overview
shortcuts/drive/drive_import_common.go Import polling and status model. Resilient to transient errors (continues on error). pollDriveImportTask lacks a ctx parameter and uses time.Sleep, preventing context-based cancellation during the 60 s polling window.
shortcuts/drive/drive_move_common.go Move polling and task-check status model. Resilient to transient errors. Uses bare time.Sleep without a ctx parameter; cannot respond to context cancellation during the 60 s polling window.
shortcuts/drive/drive_export_common.go Shared export helpers: status model, polling, filename sanitization, atomic file writes, and resume-command builders. Resume commands lack shell quoting unlike the download command helper.
shortcuts/drive/drive_export.go New export command with bounded polling, markdown fast-path, and recovery hints on timeout. Uses ctx-aware polling via select; previous issues are addressed.
shortcuts/drive/drive_import.go Import command: validates, uploads media, creates task, polls. Polling cannot be cancelled due to missing ctx propagation.
shortcuts/drive/drive_move.go Move command: root-folder fallback, sync file-move, async folder-move with bounded polling and resume command. Logic is clear and correct.
shortcuts/drive/drive_task_result.go Unified resume-from-timeout command for import, export, and task_check scenarios. Validation is scenario-specific and well-tested.
shortcuts/drive/drive_export_download.go Simple download-by-file-token shortcut delegating entirely to the shared downloadDriveExportFile helper; clean and correct.
shortcuts/drive/shortcuts.go Registry update adding the five new shortcuts in correct order.

Reviews (7): Last reviewed commit: "feat: add drive import, export, move, an..." | Re-trigger Greptile

Comment thread shortcuts/drive/drive_export.go
Comment thread shortcuts/drive/drive_move_common.go
Comment thread shortcuts/drive/drive_export.go
Comment thread shortcuts/drive/drive_import_common.go
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (5)
shortcuts/drive/shortcuts_test.go (1)

27-33: 建议补一条 Execute != nil 断言,防止“存在但不会挂载”的假阳性。

当前测试只校验命令名和唯一性;若某个 shortcut 的 Execute 为空,Mount 会静默跳过,测试仍会通过。

✅ Suggested test hardening
 	seen := make(map[string]bool, len(got))
 	for _, shortcut := range got {
+		if shortcut.Execute == nil {
+			t.Fatalf("shortcut %q has nil Execute and will not be mounted", shortcut.Command)
+		}
 		if seen[shortcut.Command] {
 			t.Fatalf("duplicate shortcut command: %s", shortcut.Command)
 		}
 		seen[shortcut.Command] = true
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/drive/shortcuts_test.go` around lines 27 - 33, The test currently
only checks for duplicate shortcut.Command values and can miss shortcuts whose
Execute is nil (which Mount will skip); update the loop over got (the slice
checked with the seen map) to also assert that each shortcut.Execute is non-nil
and fail the test (e.g., t.Fatalf) if any shortcut.Execute == nil so you catch
"exists but won't be mounted" false positives — reference the variables
shortcut, shortcut.Command, and shortcut.Execute in the existing loop and add
the nil-check before marking seen[shortcut.Command]=true.
shortcuts/drive/drive_move_test.go (2)

53-77: Consider adding t.Parallel() for consistency.

Same as above—add t.Parallel() for consistency with the rest of the test suite.

Proposed fix
 func TestDriveMoveRootFolderLookupRequiresToken(t *testing.T) {
+	t.Parallel()
+
 	f, _, _, reg := cmdutil.TestFactory(t, driveTestConfig())
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/drive/drive_move_test.go` around lines 53 - 77, In
TestDriveMoveRootFolderLookupRequiresToken add t.Parallel() at the start of the
test to run it in parallel with other tests; locate the test function
TestDriveMoveRootFolderLookupRequiresToken and insert t.Parallel() as the first
statement inside the function body to match the rest of the suite's concurrency
pattern.

14-51: Consider adding t.Parallel() for consistency with other test files in this PR.

Other test files (e.g., drive_export_common_test.go, drive_import_test.go) use t.Parallel() at the start of each test function. Adding it here would maintain consistency and potentially improve test execution time.

Proposed fix
 func TestDriveMoveUsesRootFolderWhenFolderTokenMissing(t *testing.T) {
+	t.Parallel()
+
 	f, stdout, _, reg := cmdutil.TestFactory(t, driveTestConfig())
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/drive/drive_move_test.go` around lines 14 - 51, The test function
TestDriveMoveUsesRootFolderWhenFolderTokenMissing should call t.Parallel() to
match other tests and allow parallel execution; update the function body in the
TestDriveMoveUsesRootFolderWhenFolderTokenMissing test to invoke t.Parallel() as
the first statement (immediately after the parameter t *testing.T) so it runs in
parallel with other tests.
shortcuts/drive/drive_move.go (1)

136-150: Unused ctx parameter in getRootFolderToken.

The ctx context.Context parameter is accepted but never used. Either pass it to runtime.CallAPI if that method supports context cancellation, or remove the parameter if it's not needed.

Option 1: Remove unused parameter
-func getRootFolderToken(ctx context.Context, runtime *common.RuntimeContext) (string, error) {
+func getRootFolderToken(runtime *common.RuntimeContext) (string, error) {

And update the call site at line 72:

-			rootToken, err := getRootFolderToken(ctx, runtime)
+			rootToken, err := getRootFolderToken(runtime)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/drive/drive_move.go` around lines 136 - 150, The getRootFolderToken
function declares an unused ctx parameter; either remove ctx from
getRootFolderToken and update its callers (e.g., where getRootFolderToken is
invoked) to match the new signature, or thread the context into the API call by
calling runtime.CallAPI with ctx (if CallAPI supports a context argument) so the
context is actually used; locate getRootFolderToken and runtime.CallAPI in
drive_move.go to implement the chosen option and update all call sites
accordingly.
shortcuts/drive/drive_task_result_test.go (1)

128-192: Consider adding t.Parallel() to integration tests for consistency.

The validation and DryRun tests use t.Parallel(), but the Execute tests (TestDriveTaskResultImportIncludesReadyFlags and TestDriveTaskResultTaskCheckIncludesReadyFlags) do not. Adding t.Parallel() would maintain consistency across the file, assuming there's no shared mutable state.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/drive/drive_task_result_test.go` around lines 128 - 192, The two
Execute tests TestDriveTaskResultImportIncludesReadyFlags and
TestDriveTaskResultTaskCheckIncludesReadyFlags are missing t.Parallel(); add a
call to t.Parallel() as the first statement inside each test function
(immediately after the function starts) to match the other tests and enable
running them in parallel; ensure the existing setup (registerDriveBotTokenStub,
httpmock stubs, mountAndRunDrive) remains unchanged and that no shared mutable
state is introduced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@shortcuts/drive/drive_export_common.go`:
- Around line 56-64: The zero-status export should be considered pending until a
file token exists; update the status-labeling to use the Ready() and Pending()
helpers from driveExportStatus instead of checking JobStatus alone.
Specifically, keep Ready() as FileToken != "" && JobStatus == 0 and Pending() as
JobStatus == 1 || JobStatus == 2 || (JobStatus == 0 && FileToken == ""), and in
the code that maps job status to labels (the logic in drive_task_result.go that
currently returns "success" for JobStatus == 0) change that branch to return
"success" only when s.Ready() is true and return "pending" when s.Pending() is
true so zero-status without FileToken stays pending.

In `@shortcuts/drive/drive_export.go`:
- Around line 111-114: The current code treats fetchDriveMetaTitle(runtime,
spec.Token, spec.DocType) failure as fatal, which should not be a hard
dependency because spec.Token is an acceptable fallback; change the logic so
title is initialized to spec.Token, then call fetchDriveMetaTitle and only
overwrite title if it succeeds, otherwise log the error (or a debug/warn) and
continue without returning the error; refer to fetchDriveMetaTitle, runtime,
spec.Token, spec.DocType and the title variable to locate and update the code.
- Around line 183-193: The timeout response in runtime.Out currently swaps the
numeric and string fields (setting "job_status" to the label and
"job_status_code" to the numeric), breaking the schema used by +task_result;
update the runtime.Out payload in drive_export.go so "job_status" contains
lastStatus.JobStatus (numeric) and add "job_status_label" containing
lastStatus.StatusLabel() (string), remove or avoid relying on "job_status_code"
for callers, and keep "timed_out": true and other fields unchanged to preserve
compatibility with drive_task_result.go's schema.
- Around line 24-29: The Scopes slice used for OAuth in drive_export.go is
missing required scopes for the API calls made in Execute(); update the Scopes
slice to include "docs:document.content:read" and
"drive:drive.metadata:readonly" alongside "docs:document:export" so the calls to
/open-apis/docs/v1/content and /open-apis/drive/v1/metas/batch_query have proper
authorization; locate the Scopes declaration and uncomment or add those two
scopes (keeping "docs:document:export") so Execute() can succeed at runtime.

In `@shortcuts/drive/drive_import.go`:
- Around line 198-203: The uploaded file_extension is not being normalized to
lowercase, causing a mismatch with validateDriveImportSpec() /
driveImportSpec.FileExtension(); change how ext is computed by lowercasing the
extension (e.g., set ext =
strings.ToLower(strings.TrimPrefix(filepath.Ext(filePath), "."))) before
inserting it into extraMap so extraMap["file_extension"] always uses a lowercase
value; update the code around ext and extraMap in drive_import.go accordingly.

In `@shortcuts/drive/drive_move_common.go`:
- Around line 142-145: The polling code that calls getDriveTaskCheckStatus
currently aborts on the first transient error; change it to treat read errors as
transient by retrying instead of returning immediately: inside the poll loop
(the code around getDriveTaskCheckStatus in the drive-move polling function),
catch non-fatal errors, log or record the error, wait (with a short backoff or
fixed sleep), and continue polling rather than returning driveTaskCheckStatus{},
false, err; optionally add a bounded retry counter or timeout to eventually fail
if errors persist. Ensure the behavior mirrors
pollDriveImportTask/pollDriveExport loops (i.e., transient network/5xx/timeouts
are retried while genuine terminal task states still break the loop).

---

Nitpick comments:
In `@shortcuts/drive/drive_move_test.go`:
- Around line 53-77: In TestDriveMoveRootFolderLookupRequiresToken add
t.Parallel() at the start of the test to run it in parallel with other tests;
locate the test function TestDriveMoveRootFolderLookupRequiresToken and insert
t.Parallel() as the first statement inside the function body to match the rest
of the suite's concurrency pattern.
- Around line 14-51: The test function
TestDriveMoveUsesRootFolderWhenFolderTokenMissing should call t.Parallel() to
match other tests and allow parallel execution; update the function body in the
TestDriveMoveUsesRootFolderWhenFolderTokenMissing test to invoke t.Parallel() as
the first statement (immediately after the parameter t *testing.T) so it runs in
parallel with other tests.

In `@shortcuts/drive/drive_move.go`:
- Around line 136-150: The getRootFolderToken function declares an unused ctx
parameter; either remove ctx from getRootFolderToken and update its callers
(e.g., where getRootFolderToken is invoked) to match the new signature, or
thread the context into the API call by calling runtime.CallAPI with ctx (if
CallAPI supports a context argument) so the context is actually used; locate
getRootFolderToken and runtime.CallAPI in drive_move.go to implement the chosen
option and update all call sites accordingly.

In `@shortcuts/drive/drive_task_result_test.go`:
- Around line 128-192: The two Execute tests
TestDriveTaskResultImportIncludesReadyFlags and
TestDriveTaskResultTaskCheckIncludesReadyFlags are missing t.Parallel(); add a
call to t.Parallel() as the first statement inside each test function
(immediately after the function starts) to match the other tests and enable
running them in parallel; ensure the existing setup (registerDriveBotTokenStub,
httpmock stubs, mountAndRunDrive) remains unchanged and that no shared mutable
state is introduced.

In `@shortcuts/drive/shortcuts_test.go`:
- Around line 27-33: The test currently only checks for duplicate
shortcut.Command values and can miss shortcuts whose Execute is nil (which Mount
will skip); update the loop over got (the slice checked with the seen map) to
also assert that each shortcut.Execute is non-nil and fail the test (e.g.,
t.Fatalf) if any shortcut.Execute == nil so you catch "exists but won't be
mounted" false positives — reference the variables shortcut, shortcut.Command,
and shortcut.Execute in the existing loop and add the nil-check before marking
seen[shortcut.Command]=true.
🪄 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: dfdbad4e-7d02-44c6-9550-ca67da2fb9f0

📥 Commits

Reviewing files that changed from the base of the PR and between eb8b542 and 8407a89.

📒 Files selected for processing (26)
  • internal/registry/meta_data.json
  • shortcuts/drive/drive_export.go
  • shortcuts/drive/drive_export_common.go
  • shortcuts/drive/drive_export_common_test.go
  • shortcuts/drive/drive_export_download.go
  • shortcuts/drive/drive_export_test.go
  • shortcuts/drive/drive_import.go
  • shortcuts/drive/drive_import_common.go
  • shortcuts/drive/drive_import_common_test.go
  • shortcuts/drive/drive_import_test.go
  • shortcuts/drive/drive_io_test.go
  • shortcuts/drive/drive_move.go
  • shortcuts/drive/drive_move_common.go
  • shortcuts/drive/drive_move_common_test.go
  • shortcuts/drive/drive_move_test.go
  • shortcuts/drive/drive_task_result.go
  • shortcuts/drive/drive_task_result_test.go
  • shortcuts/drive/shortcuts.go
  • shortcuts/drive/shortcuts_test.go
  • skills/lark-drive/SKILL.md
  • skills/lark-drive/references/lark-drive-export-download.md
  • skills/lark-drive/references/lark-drive-export.md
  • skills/lark-drive/references/lark-drive-import.md
  • skills/lark-drive/references/lark-drive-move.md
  • skills/lark-drive/references/lark-drive-task-result.md
  • skills/lark-wiki/SKILL.md

Comment thread shortcuts/drive/drive_export_common.go
Comment thread shortcuts/drive/drive_export.go
Comment thread shortcuts/drive/drive_export.go
Comment thread shortcuts/drive/drive_export.go
Comment thread shortcuts/drive/drive_import.go Outdated
Comment thread shortcuts/drive/drive_move_common.go
@liujinkun2025 liujinkun2025 force-pushed the feat/drive_dev branch 2 times, most recently from 1d33fd6 to 6c862c3 Compare April 1, 2026 13:34
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (3)
shortcuts/drive/drive_move_common.go (1)

57-59: Error message duplicates the allowed-types list.

The error message hardcodes the type names, which can drift from driveMoveAllowedTypes. Consider generating the list dynamically to avoid maintenance burden.

Suggested dynamic list generation
+func allowedMoveTypesString() string {
+	types := make([]string, 0, len(driveMoveAllowedTypes))
+	for t := range driveMoveAllowedTypes {
+		types = append(types, t)
+	}
+	return strings.Join(types, ", ")
+}
+
 func validateDriveMoveSpec(spec driveMoveSpec) error {
 	// ... existing validation ...
 	if !driveMoveAllowedTypes[spec.FileType] {
-		return output.ErrValidation("unsupported file type: %s. Supported types: file, docx, bitable, doc, sheet, mindnote, folder, slides", spec.FileType)
+		return output.ErrValidation("unsupported file type: %s. Supported types: %s", spec.FileType, allowedMoveTypesString())
 	}
 	return nil
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/drive/drive_move_common.go` around lines 57 - 59, The error text
currently hardcodes the supported types; instead build the allowed-types string
dynamically from the keys of driveMoveAllowedTypes and include that in the
output.ErrValidation call so it always reflects the map contents; locate the
validation branch that checks driveMoveAllowedTypes[spec.FileType] and replace
the static message with one that formats spec.FileType plus a joined list (e.g.,
comma-separated) derived from iterating the driveMoveAllowedTypes map before
calling output.ErrValidation.
shortcuts/drive/drive_export_test.go (1)

64-114: Add a regression case for the title-lookup fallback.

Lines 110-114 in shortcuts/drive/drive_export.go intentionally make fetchDriveMetaTitle() best-effort, but this suite only covers the success path. A markdown test where /open-apis/drive/v1/metas/batch_query fails and the command still writes docx123.md would keep that fallback from regressing.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/drive/drive_export_test.go` around lines 64 - 114, Add a regression
test that verifies the title-lookup fallback when fetchDriveMetaTitle() fails:
duplicate or extend TestDriveExportMarkdownWritesFile to register a failing stub
for "/open-apis/drive/v1/metas/batch_query" (e.g., return a non-zero code or
error body) while keeping the docs content stub returning "# hello\n", run
mountAndRunDrive with the same flags (including "--token", "docx123" and
"--file-extension", "markdown"), then assert that the written filename is
"docx123.md" (read file from tmpDir) and stdout still reports the fallback
filename; this ensures DriveExport and fetchDriveMetaTitle() remain best-effort.
shortcuts/drive/drive_export.go (1)

55-64: Keep markdown --dry-run aligned with the runtime path.

Execute() does a best-effort fetchDriveMetaTitle() before writing, and the markdown test stubs /open-apis/drive/v1/metas/batch_query to support that path. The dry-run branch only advertises /open-apis/docs/v1/content, so it hides a real API dependency and the reason drive:drive.metadata:readonly is needed. At minimum, mention the metadata lookup in Desc(); if DryRunAPI can express multiple calls, register that request too.

Also applies to: 108-114

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/drive/drive_export.go` around lines 55 - 64, The dry-run branch for
spec.FileExtension == "markdown" currently only registers the docs content call
via NewDryRunAPI().GET("/open-apis/docs/v1/content") but Execute() and
fetchDriveMetaTitle() also perform a metadata lookup (batch_query) so the
dry-run should reflect that dependency; update the NewDryRunAPI entry in the
markdown branch of drive_export.go (the block guarded by spec.FileExtension ==
"markdown") to either add a second DryRun request for GET
"/open-apis/drive/v1/metas/batch_query" with the appropriate Params or at
minimum append that fact to the Desc() string so the dry-run advertises the
metadata lookup and the need for drive:drive.metadata:readonly, matching the
runtime path used by Execute() and fetchDriveMetaTitle().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@shortcuts/drive/drive_export.go`:
- Around line 142-147: The polling loop in drive_export.go currently blocks with
time.Sleep(driveExportPollInterval) and ignores ctx cancellation; change the
sleep to a cancellable wait by replacing the direct time.Sleep with a select
that waits on either ctx.Done() or time.After(driveExportPollInterval), and if
ctx.Done() is closed return/exit immediately (propagate ctx.Err() or appropriate
cancellation handling) before calling getDriveExportStatus; ensure the same ctx
check runs before the first iteration as well so symbols involved are the
polling loop using driveExportPollAttempts, driveExportPollInterval,
getDriveExportStatus(runtime, spec.Token, ticket), and ctx.

In `@shortcuts/drive/drive_move_common.go`:
- Around line 41-46: RequestBody for driveMoveSpec always includes
"folder_token" even when empty; update the RequestBody method on driveMoveSpec
so it only adds the "folder_token" key when s.FolderToken is non-empty (e.g.,
check if s.FolderToken != "" before inserting into the map), keeping "type"
(s.FileType) always present; this aligns with other ops (sheets/sheet_create.go,
doc/docs_create.go, base/base_ops.go) and respects validateDriveMoveSpec
allowing an empty FolderToken.

---

Nitpick comments:
In `@shortcuts/drive/drive_export_test.go`:
- Around line 64-114: Add a regression test that verifies the title-lookup
fallback when fetchDriveMetaTitle() fails: duplicate or extend
TestDriveExportMarkdownWritesFile to register a failing stub for
"/open-apis/drive/v1/metas/batch_query" (e.g., return a non-zero code or error
body) while keeping the docs content stub returning "# hello\n", run
mountAndRunDrive with the same flags (including "--token", "docx123" and
"--file-extension", "markdown"), then assert that the written filename is
"docx123.md" (read file from tmpDir) and stdout still reports the fallback
filename; this ensures DriveExport and fetchDriveMetaTitle() remain best-effort.

In `@shortcuts/drive/drive_export.go`:
- Around line 55-64: The dry-run branch for spec.FileExtension == "markdown"
currently only registers the docs content call via
NewDryRunAPI().GET("/open-apis/docs/v1/content") but Execute() and
fetchDriveMetaTitle() also perform a metadata lookup (batch_query) so the
dry-run should reflect that dependency; update the NewDryRunAPI entry in the
markdown branch of drive_export.go (the block guarded by spec.FileExtension ==
"markdown") to either add a second DryRun request for GET
"/open-apis/drive/v1/metas/batch_query" with the appropriate Params or at
minimum append that fact to the Desc() string so the dry-run advertises the
metadata lookup and the need for drive:drive.metadata:readonly, matching the
runtime path used by Execute() and fetchDriveMetaTitle().

In `@shortcuts/drive/drive_move_common.go`:
- Around line 57-59: The error text currently hardcodes the supported types;
instead build the allowed-types string dynamically from the keys of
driveMoveAllowedTypes and include that in the output.ErrValidation call so it
always reflects the map contents; locate the validation branch that checks
driveMoveAllowedTypes[spec.FileType] and replace the static message with one
that formats spec.FileType plus a joined list (e.g., comma-separated) derived
from iterating the driveMoveAllowedTypes map before calling
output.ErrValidation.
🪄 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: afe01210-4285-4e22-9c06-6959e51e8fad

📥 Commits

Reviewing files that changed from the base of the PR and between 8407a89 and 1d33fd6.

📒 Files selected for processing (8)
  • shortcuts/drive/drive_export.go
  • shortcuts/drive/drive_export_download.go
  • shortcuts/drive/drive_export_test.go
  • shortcuts/drive/drive_import.go
  • shortcuts/drive/drive_import_common.go
  • shortcuts/drive/drive_import_test.go
  • shortcuts/drive/drive_move_common.go
  • skills/lark-drive/references/lark-drive-import.md
✅ Files skipped from review due to trivial changes (1)
  • skills/lark-drive/references/lark-drive-import.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • shortcuts/drive/drive_export_download.go
  • shortcuts/drive/drive_import_test.go

Comment thread shortcuts/drive/drive_export.go
Comment thread shortcuts/drive/drive_move_common.go
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (2)
shortcuts/drive/drive_move_common.go (1)

41-46: ⚠️ Potential issue | 🟡 Minor

Conditionally omit empty folder_token from move request body.

Line 41–46 still always sends "folder_token", while Line 52–56 treats it as optional. Please only include this key when non-empty.

Suggested fix
 func (s driveMoveSpec) RequestBody() map[string]interface{} {
-	return map[string]interface{}{
-		"type":         s.FileType,
-		"folder_token": s.FolderToken,
-	}
+	body := map[string]interface{}{
+		"type": s.FileType,
+	}
+	if s.FolderToken != "" {
+		body["folder_token"] = s.FolderToken
+	}
+	return body
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/drive/drive_move_common.go` around lines 41 - 46, The RequestBody
method in driveMoveSpec always includes "folder_token" even when empty; change
driveMoveSpec.RequestBody() so it only adds the "folder_token" key to the
returned map when s.FolderToken is non-empty (e.g., create the base map with
"type": s.FileType and then conditionally set "folder_token" when s.FolderToken
!= ""), ensuring consistency with the optional handling elsewhere.
shortcuts/drive/drive_export.go (1)

142-145: ⚠️ Potential issue | 🟠 Major

Polling loop does not honor context cancellation.

Line 144 blocks on time.Sleep(driveExportPollInterval) without checking ctx.Done(). With the current 10×5s defaults, a cancelled command can remain blocked for up to 50 seconds before the loop naturally exits. This prevents timely cancellation and cleanup.

Suggested fix
 		for attempt := 1; attempt <= driveExportPollAttempts; attempt++ {
 			if attempt > 1 {
-				time.Sleep(driveExportPollInterval)
+				select {
+				case <-ctx.Done():
+					return ctx.Err()
+				case <-time.After(driveExportPollInterval):
+				}
 			}
 
 			status, err := getDriveExportStatus(runtime, spec.Token, ticket)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/drive/drive_export.go` around lines 142 - 145, The polling loop
using driveExportPollAttempts and time.Sleep(driveExportPollInterval) blocks
cancellation; replace the unconditional time.Sleep with a context-aware wait
(use select { case <-time.After(driveExportPollInterval): /* continue */ case
<-ctx.Done(): return/ break }) and also check ctx.Err() at the top of the loop
so the loop exits promptly when ctx is cancelled; update the loop around the
existing attempts logic (the for loop that references driveExportPollAttempts
and driveExportPollInterval) to use this select-based sleep and return an
appropriate error when ctx is done.
🧹 Nitpick comments (4)
shortcuts/drive/drive_export_common.go (2)

276-278: HTTP 4xx/5xx errors should use ErrAPI rather than ErrNetwork.

HTTP status codes ≥400 indicate API-level failures (authentication, authorization, not found, etc.), not network transport issues. Using ErrNetwork here conflates the error types, which could mislead error handling logic and user-facing messages.

Suggested fix
 	if apiResp.StatusCode >= 400 {
-		return nil, output.ErrNetwork("download failed: HTTP %d: %s", apiResp.StatusCode, string(apiResp.RawBody))
+		return nil, output.Errorf(output.ExitAPI, "api_error", "download failed: HTTP %d: %s", apiResp.StatusCode, string(apiResp.RawBody))
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/drive/drive_export_common.go` around lines 276 - 278, The error
handling for HTTP responses treats 4xx/5xx as network errors; change the return
to use output.ErrAPI instead of output.ErrNetwork in the block that checks
apiResp.StatusCode >= 400 (the same place where apiResp.RawBody is used) so
API-level failures are categorized correctly—replace the ErrNetwork call with
output.ErrAPI("download failed: HTTP %d: %s", apiResp.StatusCode,
string(apiResp.RawBody)) while leaving the status check and message intact.

60-64: Operator precedence may cause unexpected behavior in Pending().

Line 63 relies on Go's operator precedence where && binds tighter than ||, so the expression evaluates as intended: s.JobStatus == 1 || s.JobStatus == 2 || (s.JobStatus == 0 && s.FileToken == ""). However, adding explicit parentheses would improve readability and prevent future maintenance errors.

Suggested clarification
 func (s driveExportStatus) Pending() bool {
 	// A zero status without a file token is still in progress because there is
 	// nothing downloadable yet.
-	return s.JobStatus == 1 || s.JobStatus == 2 || s.JobStatus == 0 && s.FileToken == ""
+	return s.JobStatus == 1 || s.JobStatus == 2 || (s.JobStatus == 0 && s.FileToken == "")
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/drive/drive_export_common.go` around lines 60 - 64, The Pending()
method on driveExportStatus uses a compound boolean expression that relies on
operator precedence (JobStatus == 1 || JobStatus == 2 || s.JobStatus == 0 &&
s.FileToken == "") which is correct but unclear; update the expression in the
Pending() function to add explicit parentheses so it reads like (s.JobStatus ==
1) || (s.JobStatus == 2) || ((s.JobStatus == 0) && (s.FileToken == "")) to
improve readability and prevent future precedence mistakes when maintaining the
JobStatus and FileToken checks.
shortcuts/drive/drive_import_common.go (1)

112-116: Whitespace-only FolderToken passes guard but propagates to API.

If FolderToken is whitespace-only (e.g., " "), TrimSpace produces an empty string (skipping validation), but CreateTaskBody sends the original whitespace to the API as mount_key. Consider normalizing or rejecting:

🛠️ Suggested fix
-	if strings.TrimSpace(spec.FolderToken) != "" {
+	trimmedToken := strings.TrimSpace(spec.FolderToken)
+	if trimmedToken != spec.FolderToken {
+		return output.ErrValidation("--folder-token must not have leading or trailing whitespace")
+	}
+	if trimmedToken != "" {
 		if err := validate.ResourceName(spec.FolderToken, "--folder-token"); err != nil {
 			return output.ErrValidation("%s", err)
 		}
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/drive/drive_import_common.go` around lines 112 - 116, Trim and
normalize spec.FolderToken before validating and sending it to the API: replace
the current strings.TrimSpace(...) guard with a trimmed variable (or assign back
to spec.FolderToken), then if the trimmed value is non-empty run
validate.ResourceName(trimmed, "--folder-token") and store the trimmed value in
spec.FolderToken so CreateTaskBody sends a normalized mount_key (or reject when
validation fails). Ensure references to spec.FolderToken, validate.ResourceName,
and CreateTaskBody/mount_key are updated accordingly.
shortcuts/drive/drive_import.go (1)

192-195: Wrap os.Open error for consistent classification.

If os.Open fails (e.g., permission denied), the raw error propagates without CLI exit-code classification. Consider wrapping it like the os.Stat error above.

🛠️ Suggested fix
 	f, err := os.Open(filePath)
 	if err != nil {
-		return "", err
+		return "", output.ErrValidation("cannot open file: %s", err)
 	}
 	defer f.Close()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/drive/drive_import.go` around lines 192 - 195, The raw error from
os.Open(filePath) should be wrapped with context so it can be consistently
classified (like the os.Stat error above); replace the bare return of err after
the os.Open call (the f, err := os.Open(filePath) block) with a wrapped error
such as returning fmt.Errorf("failed to open %s: %w", filePath, err) (ensure fmt
is imported) so the original error is preserved and classification/exit-code
handling works downstream.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@shortcuts/drive/drive_export.go`:
- Around line 142-145: The polling loop using driveExportPollAttempts and
time.Sleep(driveExportPollInterval) blocks cancellation; replace the
unconditional time.Sleep with a context-aware wait (use select { case
<-time.After(driveExportPollInterval): /* continue */ case <-ctx.Done(): return/
break }) and also check ctx.Err() at the top of the loop so the loop exits
promptly when ctx is cancelled; update the loop around the existing attempts
logic (the for loop that references driveExportPollAttempts and
driveExportPollInterval) to use this select-based sleep and return an
appropriate error when ctx is done.

In `@shortcuts/drive/drive_move_common.go`:
- Around line 41-46: The RequestBody method in driveMoveSpec always includes
"folder_token" even when empty; change driveMoveSpec.RequestBody() so it only
adds the "folder_token" key to the returned map when s.FolderToken is non-empty
(e.g., create the base map with "type": s.FileType and then conditionally set
"folder_token" when s.FolderToken != ""), ensuring consistency with the optional
handling elsewhere.

---

Nitpick comments:
In `@shortcuts/drive/drive_export_common.go`:
- Around line 276-278: The error handling for HTTP responses treats 4xx/5xx as
network errors; change the return to use output.ErrAPI instead of
output.ErrNetwork in the block that checks apiResp.StatusCode >= 400 (the same
place where apiResp.RawBody is used) so API-level failures are categorized
correctly—replace the ErrNetwork call with output.ErrAPI("download failed: HTTP
%d: %s", apiResp.StatusCode, string(apiResp.RawBody)) while leaving the status
check and message intact.
- Around line 60-64: The Pending() method on driveExportStatus uses a compound
boolean expression that relies on operator precedence (JobStatus == 1 ||
JobStatus == 2 || s.JobStatus == 0 && s.FileToken == "") which is correct but
unclear; update the expression in the Pending() function to add explicit
parentheses so it reads like (s.JobStatus == 1) || (s.JobStatus == 2) ||
((s.JobStatus == 0) && (s.FileToken == "")) to improve readability and prevent
future precedence mistakes when maintaining the JobStatus and FileToken checks.

In `@shortcuts/drive/drive_import_common.go`:
- Around line 112-116: Trim and normalize spec.FolderToken before validating and
sending it to the API: replace the current strings.TrimSpace(...) guard with a
trimmed variable (or assign back to spec.FolderToken), then if the trimmed value
is non-empty run validate.ResourceName(trimmed, "--folder-token") and store the
trimmed value in spec.FolderToken so CreateTaskBody sends a normalized mount_key
(or reject when validation fails). Ensure references to spec.FolderToken,
validate.ResourceName, and CreateTaskBody/mount_key are updated accordingly.

In `@shortcuts/drive/drive_import.go`:
- Around line 192-195: The raw error from os.Open(filePath) should be wrapped
with context so it can be consistently classified (like the os.Stat error
above); replace the bare return of err after the os.Open call (the f, err :=
os.Open(filePath) block) with a wrapped error such as returning
fmt.Errorf("failed to open %s: %w", filePath, err) (ensure fmt is imported) so
the original error is preserved and classification/exit-code handling works
downstream.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1143069a-a866-443f-a290-360112939df2

📥 Commits

Reviewing files that changed from the base of the PR and between 1d33fd6 and 6c862c3.

📒 Files selected for processing (9)
  • shortcuts/drive/drive_export.go
  • shortcuts/drive/drive_export_common.go
  • shortcuts/drive/drive_export_download.go
  • shortcuts/drive/drive_export_test.go
  • shortcuts/drive/drive_import.go
  • shortcuts/drive/drive_import_common.go
  • shortcuts/drive/drive_import_test.go
  • shortcuts/drive/drive_move_common.go
  • skills/lark-drive/references/lark-drive-import.md
✅ Files skipped from review due to trivial changes (1)
  • shortcuts/drive/drive_export_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • skills/lark-drive/references/lark-drive-import.md

@liujinkun2025 liujinkun2025 force-pushed the feat/drive_dev branch 2 times, most recently from 5691e33 to 29a091a Compare April 1, 2026 14:10
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

♻️ Duplicate comments (1)
shortcuts/drive/drive_move_common.go (1)

41-46: ⚠️ Potential issue | 🟡 Minor

Conditionally include folder_token only when provided.

On Line 41, RequestBody() always sends "folder_token" even when empty. Since --folder-token is optional (Line 52 logic), this should be omitted when blank to preserve root-target fallback behavior and API consistency.

Suggested fix
 func (s driveMoveSpec) RequestBody() map[string]interface{} {
-	return map[string]interface{}{
-		"type":         s.FileType,
-		"folder_token": s.FolderToken,
-	}
+	body := map[string]interface{}{
+		"type": s.FileType,
+	}
+	if strings.TrimSpace(s.FolderToken) != "" {
+		body["folder_token"] = s.FolderToken
+	}
+	return body
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/drive/drive_move_common.go` around lines 41 - 46, The RequestBody
currently always includes "folder_token" even when blank; update
driveMoveSpec.RequestBody to build the map with "type": s.FileType and only add
"folder_token": s.FolderToken when s.FolderToken != "" (or len>0) so the key is
omitted for empty values; locate the RequestBody method on type driveMoveSpec to
implement the conditional addition and return the resulting map.
🧹 Nitpick comments (1)
shortcuts/drive/drive_export_common.go (1)

39-40: Don't reuse --file-token for the source document token.

This helper injects the original document token into --file-token, while drive +export-download uses the same flag name for the exported artifact token. That semantic collision will make manual +task_result calls easy to get wrong. Renaming the export-status flag to --token or --doc-token now would avoid locking in the ambiguity.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/drive/drive_export_common.go` around lines 39 - 40, The helper
driveExportTaskResultCommand currently injects the original document token into
the flag --file-token, which collides with the exported artifact token used
elsewhere; update driveExportTaskResultCommand to use a distinct flag name
(e.g., --doc-token or --token) instead of --file-token, and ensure all callers
that expect the original document token are updated to pass/parse the new flag
name (reference driveExportTaskResultCommand and any callers of
+task_result/+export-download to avoid semantic collision).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@shortcuts/drive/drive_export.go`:
- Around line 154-159: The polling loop calling getDriveExportStatus currently
ignores persistent errors and emits a success-shaped timeout; modify the loop to
record the last non-nil error (e.g., lastPollErr) each time getDriveExportStatus
returns an error (where you currently print and continue), and after the retry
loop (where hasObservedStatus is checked) if hasObservedStatus is still false
attach/surface that lastPollErr in the timeout/final payload (or mark the export
as failed with that error) instead of returning only `"failed": false,
"timed_out": true`; update both occurrences around the
driveExportPollAttempts/attempt logic so callers/automation can distinguish
transient polling from persistent backend/auth failures.
- Around line 164-170: When downloadDriveExportFile(ctx, runtime,
status.FileToken, outputDir, fileName, overwrite) fails after the export is
Ready(), do not return the raw error; instead surface a recovery path including
the available status.FileToken (and optionally a recovery command like "drive
+export-download --file-token=<token> --output=<path>"). Update the error path
in the Ready() branch (around the downloadDriveExportFile call inside the export
loop) to wrap or augment the returned error with the token/recovery command so
callers can resume without scraping stderr — reference status.FileToken,
downloadDriveExportFile, ensureExportFileExtension and sanitizeExportFileName
when implementing the augmented error/log message.

In `@shortcuts/drive/drive_import_test.go`:
- Around line 133-135: The test currently uses got, _ :=
point["mount_key"].(string) which treats a missing key as the empty string and
can false-pass; update the root-case assertion to first check the key exists
(val, ok := point["mount_key"]; if !ok { t.Fatalf("mount_key missing for root
import") }) then perform the string type assertion on val (got, ok :=
val.(string)) and finally assert got == "" (or fail with a descriptive message)
so both presence and type are validated for mount_key in the test using the
point map.

In `@shortcuts/drive/drive_move_common.go`:
- Around line 70-79: driveTaskCheckStatus methods compare raw s.Status which can
have case/whitespace variants; normalize the status before terminal checks by
trimming whitespace and lowercasing (e.g., use strings.TrimSpace +
strings.ToLower or strings.EqualFold) and then compare to "success" and
"failed". Update Ready() and Failed() (or add an unexported helper like
normalizedStatus() used by Ready(), Failed(), Pending()) so comparisons use the
normalized value of s.Status rather than the raw string.

---

Duplicate comments:
In `@shortcuts/drive/drive_move_common.go`:
- Around line 41-46: The RequestBody currently always includes "folder_token"
even when blank; update driveMoveSpec.RequestBody to build the map with "type":
s.FileType and only add "folder_token": s.FolderToken when s.FolderToken != ""
(or len>0) so the key is omitted for empty values; locate the RequestBody method
on type driveMoveSpec to implement the conditional addition and return the
resulting map.

---

Nitpick comments:
In `@shortcuts/drive/drive_export_common.go`:
- Around line 39-40: The helper driveExportTaskResultCommand currently injects
the original document token into the flag --file-token, which collides with the
exported artifact token used elsewhere; update driveExportTaskResultCommand to
use a distinct flag name (e.g., --doc-token or --token) instead of --file-token,
and ensure all callers that expect the original document token are updated to
pass/parse the new flag name (reference driveExportTaskResultCommand and any
callers of +task_result/+export-download to avoid semantic collision).
🪄 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: 9621f196-71d4-40e3-8a12-a18c2bca43ae

📥 Commits

Reviewing files that changed from the base of the PR and between 6c862c3 and 5691e33.

📒 Files selected for processing (10)
  • shortcuts/drive/drive_export.go
  • shortcuts/drive/drive_export_common.go
  • shortcuts/drive/drive_export_download.go
  • shortcuts/drive/drive_export_test.go
  • shortcuts/drive/drive_import.go
  • shortcuts/drive/drive_import_common.go
  • shortcuts/drive/drive_import_test.go
  • shortcuts/drive/drive_move.go
  • shortcuts/drive/drive_move_common.go
  • skills/lark-drive/references/lark-drive-import.md
✅ Files skipped from review due to trivial changes (3)
  • shortcuts/drive/drive_export_download.go
  • skills/lark-drive/references/lark-drive-import.md
  • shortcuts/drive/drive_import.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • shortcuts/drive/drive_move.go
  • shortcuts/drive/drive_import_common.go

Comment thread shortcuts/drive/drive_export.go
Comment thread shortcuts/drive/drive_export.go
Comment thread shortcuts/drive/drive_import_test.go Outdated
Comment thread shortcuts/drive/drive_move_common.go
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (2)
shortcuts/drive/drive_move_common.go (1)

74-80: ⚠️ Potential issue | 🟠 Major

Normalize terminal status checks before comparing values.

Line 75 and Line 79 still compare raw status strings. Case/whitespace variants (e.g., " Success " / "FAILED") will be treated as pending and can incorrectly trigger timeout/resume behavior.

Suggested fix
 func (s driveTaskCheckStatus) Ready() bool {
-	return s.Status == "success"
+	return strings.EqualFold(strings.TrimSpace(s.Status), "success")
 }
 
 func (s driveTaskCheckStatus) Failed() bool {
-	return s.Status == "failed"
+	return strings.EqualFold(strings.TrimSpace(s.Status), "failed")
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/drive/drive_move_common.go` around lines 74 - 80,
driveTaskCheckStatus Ready() and Failed() currently compare raw s.Status
strings; normalize the status before comparing by trimming whitespace and
lowercasing (e.g., use strings.TrimSpace and strings.ToLower) so Ready() checks
against "success" and Failed() checks against "failed" robustly; update the
Ready() and Failed() methods to normalize s.Status first (keep method names
driveTaskCheckStatus.Ready and driveTaskCheckStatus.Failed as reference).
shortcuts/drive/drive_export.go (1)

164-176: ⚠️ Potential issue | 🟠 Major

Include recovery information when download fails after export completes.

When status.Ready() is true but downloadDriveExportFile fails (e.g., network error, disk full), the raw error is returned without the file_token that was successfully generated. Users cannot resume the download without re-querying the task status.

Consider wrapping the error with recovery information:

Proposed fix
 			if status.Ready() {
 				fmt.Fprintf(runtime.IO().ErrOut, "Export task completed: %s\n", common.MaskToken(status.FileToken))
 				fileName := ensureExportFileExtension(sanitizeExportFileName(status.FileName, spec.Token), spec.FileExtension)
 				out, err := downloadDriveExportFile(ctx, runtime, status.FileToken, outputDir, fileName, overwrite)
 				if err != nil {
-					return err
+					// Provide recovery path so users can retry the download without re-polling
+					recoveryCmd := fmt.Sprintf("lark-cli drive +export-download --file-token %s --output-dir %s", status.FileToken, outputDir)
+					return output.Errorf(output.ExitAPI, "download_failed", "download failed: %v\nExport completed successfully. Retry with: %s", err, recoveryCmd)
 				}
 				out["ticket"] = ticket
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@shortcuts/drive/drive_export.go` around lines 164 - 176, When status.Ready()
is true but downloadDriveExportFile(ctx, runtime, status.FileToken, ...) fails
you must return recovery info so callers can resume using the generated file
token instead of just the raw error; modify the Ready() branch (around
downloadDriveExportFile and variable status.FileToken) to include the file token
(and ticket/doc_type/file_extension) in the error path — e.g., create an output
map containing "file_token": status.FileToken (and existing ticket/spec fields)
and either call runtime.Out(out, wrappedErr) or wrap the returned error with
fmt.Errorf to include the file token string so the consumer can resume the
download without re-querying the export task. Ensure the change happens only in
the failure branch after downloadDriveExportFile returns an error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@shortcuts/drive/drive_import_common.go`:
- Around line 232-237: The loop calling getDriveImportStatus (using
driveImportPollAttempts and logging to runtime.IO().ErrOut) currently ignores
repeated failures and later returns nil (treated as a timeout); change it to
record the last non-nil error (e.g., lastErr) when getDriveImportStatus fails
and after the polling loop return that error if set instead of nil so genuine
auth/network/API failures are surfaced; keep the existing logging and only
return a timeout result (nil or a timeout-specific value) when at least one
status check succeeded but timed out.

In `@shortcuts/drive/drive_move_common.go`:
- Around line 45-47: The RequestBody() method currently checks s.FolderToken !=
"" and includes the raw value, which can send whitespace-only tokens; change it
to use strings.TrimSpace(s.FolderToken) when deciding and when setting
body["folder_token"] so the same trimmed value validateDriveMoveSpec expects is
what's sent. Locate the FolderToken field usage inside RequestBody() and replace
the non-trimmed check and assignment with a trimmedToken :=
strings.TrimSpace(s.FolderToken) guard and use trimmedToken for
body["folder_token"].

---

Duplicate comments:
In `@shortcuts/drive/drive_export.go`:
- Around line 164-176: When status.Ready() is true but
downloadDriveExportFile(ctx, runtime, status.FileToken, ...) fails you must
return recovery info so callers can resume using the generated file token
instead of just the raw error; modify the Ready() branch (around
downloadDriveExportFile and variable status.FileToken) to include the file token
(and ticket/doc_type/file_extension) in the error path — e.g., create an output
map containing "file_token": status.FileToken (and existing ticket/spec fields)
and either call runtime.Out(out, wrappedErr) or wrap the returned error with
fmt.Errorf to include the file token string so the consumer can resume the
download without re-querying the export task. Ensure the change happens only in
the failure branch after downloadDriveExportFile returns an error.

In `@shortcuts/drive/drive_move_common.go`:
- Around line 74-80: driveTaskCheckStatus Ready() and Failed() currently compare
raw s.Status strings; normalize the status before comparing by trimming
whitespace and lowercasing (e.g., use strings.TrimSpace and strings.ToLower) so
Ready() checks against "success" and Failed() checks against "failed" robustly;
update the Ready() and Failed() methods to normalize s.Status first (keep method
names driveTaskCheckStatus.Ready and driveTaskCheckStatus.Failed as reference).
🪄 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: 684865ba-2526-4b5b-b108-233aaf691977

📥 Commits

Reviewing files that changed from the base of the PR and between 5691e33 and 29a091a.

📒 Files selected for processing (10)
  • shortcuts/drive/drive_export.go
  • shortcuts/drive/drive_export_common.go
  • shortcuts/drive/drive_export_download.go
  • shortcuts/drive/drive_export_test.go
  • shortcuts/drive/drive_import.go
  • shortcuts/drive/drive_import_common.go
  • shortcuts/drive/drive_import_test.go
  • shortcuts/drive/drive_move.go
  • shortcuts/drive/drive_move_common.go
  • skills/lark-drive/references/lark-drive-import.md
✅ Files skipped from review due to trivial changes (2)
  • shortcuts/drive/drive_import_test.go
  • skills/lark-drive/references/lark-drive-import.md
🚧 Files skipped from review as they are similar to previous changes (3)
  • shortcuts/drive/drive_export_download.go
  • shortcuts/drive/drive_move.go
  • shortcuts/drive/drive_export_test.go

Comment thread shortcuts/drive/drive_import_common.go
Comment thread shortcuts/drive/drive_move_common.go Outdated
@liujinkun2025 liujinkun2025 force-pushed the feat/drive_dev branch 2 times, most recently from c9d7779 to c4d0d14 Compare April 1, 2026 14:55
Change-Id: I0938dcf587e377afc4ab7133f1e8ff1e2412e566
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

domain/ccm PR touches the ccm domain size/XL Architecture-level or global-impact change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants