feat: add drive folder delete shortcut with async task polling#415
feat: add drive folder delete shortcut with async task polling#415fangshuyu-768 merged 1 commit intomainfrom
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:
📝 WalkthroughWalkthroughAdds a new Drive shortcut command Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (drive +delete)
participant API as Drive API
participant Poll as TaskCheck Poller
CLI->>API: DELETE /open-apis/drive/v1/files/:file_token?type=...
alt non-folder
API-->>CLI: 200 OK (deleted)
CLI->>CLI: output { deleted: true, file_token, type }
else folder (async)
API-->>CLI: 200 OK (task_id)
CLI->>Poll: start polling GET /open-apis/drive/v1/files/task_check?task_id=...
Poll->>API: GET /open-apis/drive/v1/files/task_check?task_id=...
alt status=success
API-->>Poll: status=success
Poll-->>CLI: output { ready: true, deleted: true, task_id, status }
else status=fail
API-->>Poll: status=fail
Poll-->>CLI: output { ready: false, failed: true, task_id, status }
else no terminal status (timeout)
Poll-->>CLI: output { ready: false, timed_out: true, next_command }
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryAdds a new Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/test-coverage suggestions with no blocking correctness issues. The new delete shortcut correctly reuses the shared polling infrastructure, the Failed() fix and seenStatus guard are both correct, and test coverage is solid across all meaningful outcomes. Only P2 concerns remain: a missing comment on the mutex pattern in the test helper, and a missing failed terminal-status sub-case in the move test table. shortcuts/drive/drive_io_test.go (mutex comment), shortcuts/drive/drive_move_common_test.go (missing failed sub-case) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["lark-cli drive +delete\n--file-token X --type T --yes"] --> B{type == folder?}
B -- No --> C["DELETE /drive/v1/files/:token\n?type=file|docx|…"]
C --> D["Output: deleted=true, file_token, type"]
B -- Yes --> E["DELETE /drive/v1/files/:token\n?type=folder"]
E --> F{task_id in response?}
F -- No --> G["Error: no task_id"]
F -- Yes --> H["pollDriveTaskCheck\n(up to 30 × 2s)"]
H --> I{status?}
I -- success --> J["Output: deleted=true,\nready=true, task_id, status"]
I -- fail/failed --> K["Error: folder task failed"]
I -- pending/process --> L{attempts exhausted?}
L -- No --> H
L -- Yes --> M["Output: ready=false,\ntimed_out=true, next_command"]
M --> N["lark-cli drive +task_result\n--scenario task_check --task-id X --as user|bot"]
N --> O["GET /drive/v1/files/task_check\n?task_id=X"]
O --> P["Output: status, ready, failed"]
Reviews (5): Last reviewed commit: "feat: add drive folder delete shortcut w..." | Re-trigger Greptile |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@8236288f342ce66e9fa0418b0c64e95f8716c171🧩 Skill updatenpx skills add larksuite/cli#feat/file_delete_shortcut -y -g |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shortcuts/drive/drive_delete.go (1)
41-64: Reuse the shared task_check status type/parser here.
driveDeleteTaskCheckStatusandgetDriveDeleteTaskCheckStatusnow mirrordriveTaskCheckStatusinshortcuts/drive/drive_move_common.go, so the next task-state tweak has to be made in two places. Keeping the delete-specific log/error strings in the polling flow is enough; the status shape/parsing can stay shared.Also applies to: 181-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_delete.go` around lines 41 - 64, The delete-specific type driveDeleteTaskCheckStatus duplicates driveTaskCheckStatus; remove this duplicate and use the shared driveTaskCheckStatus from drive_move_common.go instead. Replace references/usages of driveDeleteTaskCheckStatus and getDriveDeleteTaskCheckStatus to return or accept driveTaskCheckStatus, leaving only delete-specific polling/error message strings in the delete flow; ensure Ready(), Failed(), and StatusLabel() behavior is provided by the shared driveTaskCheckStatus so parsing logic is centralized.
🤖 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_delete.go`:
- Around line 202-217: The polling loop calls getDriveDeleteTaskCheckStatus
repeatedly but drops the last error, causing Execute to report a timeout when
every poll failed; add a lastErr variable alongside lastStatus, set lastErr =
err whenever getDriveDeleteTaskCheckStatus returns an error, and after the loop
return that lastErr (wrapped appropriately) when no successful status was ever
observed (i.e., lastStatus is nil and lastErr != nil) instead of returning nil
and a timed-out result; update the function that returns (currently returning
lastStatus, false, nil) to return the stored error so backend/auth errors
surface.
---
Nitpick comments:
In `@shortcuts/drive/drive_delete.go`:
- Around line 41-64: The delete-specific type driveDeleteTaskCheckStatus
duplicates driveTaskCheckStatus; remove this duplicate and use the shared
driveTaskCheckStatus from drive_move_common.go instead. Replace
references/usages of driveDeleteTaskCheckStatus and
getDriveDeleteTaskCheckStatus to return or accept driveTaskCheckStatus, leaving
only delete-specific polling/error message strings in the delete flow; ensure
Ready(), Failed(), and StatusLabel() behavior is provided by the shared
driveTaskCheckStatus so parsing logic is centralized.
🪄 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: 11b84cc7-673f-4030-9676-734cb2056ec0
📒 Files selected for processing (8)
shortcuts/drive/drive_delete.goshortcuts/drive/drive_delete_test.goshortcuts/drive/drive_move_common.goshortcuts/drive/drive_task_result_test.goshortcuts/drive/shortcuts.goshortcuts/drive/shortcuts_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-delete.md
7582966 to
f1fdadf
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shortcuts/drive/drive_delete.go (1)
102-123:⚠️ Potential issue | 🟠 MajorAvoid mapping repeated polling failures to
timed_out=true.At Line 102, this path assumes
err == nil && ready == falsemeans “still running”. But the current shared helper can return that shape even when every poll attempt failed (backend/auth/network), which makes real failures look like resumable timeouts.Suggested fix (in shared helper)
--- a/shortcuts/drive/drive_move_common.go +++ b/shortcuts/drive/drive_move_common.go @@ func pollDriveTaskCheck(runtime *common.RuntimeContext, taskID string) (driveTaskCheckStatus, bool, error) { lastStatus := driveTaskCheckStatus{TaskID: taskID} + var ( + seenStatus bool + lastErr error + ) for attempt := 1; attempt <= driveTaskCheckPollAttempts; attempt++ { if attempt > 1 { time.Sleep(driveTaskCheckPollInterval) } status, err := getDriveTaskCheckStatus(runtime, taskID) if err != nil { + lastErr = err fmt.Fprintf(runtime.IO().ErrOut, "Error polling task %s: %s\n", taskID, err) continue } + seenStatus = true lastStatus = status @@ if status.Failed() { return status, false, output.Errorf(output.ExitAPI, "api_error", "folder task failed") } } + if !seenStatus && lastErr != nil { + return driveTaskCheckStatus{}, false, lastErr + } return lastStatus, false, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_delete.go` around lines 102 - 123, The current code treats any (err==nil, ready==false) result from pollDriveTaskCheck as a running task, but pollDriveTaskCheck can return that shape when all poll attempts failed; fix by changing pollDriveTaskCheck to return an additional boolean (e.g., pollFailed or allAttemptsFailed) or a distinct sentinel error so callers can distinguish "still running" vs "polling failed", then update this handler (which calls pollDriveTaskCheck and uses driveTaskCheckResultCommand and runtime.IO().ErrOut) to: if pollFailed is true, do not set timed_out=true or suggest the resume command—instead set out["poll_failed"]=true (or out["error"]=true) and write an error message to runtime.IO().ErrOut describing the polling failure; only when pollFailed==false and ready==false emit next_command and timed_out=true as before.
🤖 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_delete.go`:
- Around line 102-123: The current code treats any (err==nil, ready==false)
result from pollDriveTaskCheck as a running task, but pollDriveTaskCheck can
return that shape when all poll attempts failed; fix by changing
pollDriveTaskCheck to return an additional boolean (e.g., pollFailed or
allAttemptsFailed) or a distinct sentinel error so callers can distinguish
"still running" vs "polling failed", then update this handler (which calls
pollDriveTaskCheck and uses driveTaskCheckResultCommand and runtime.IO().ErrOut)
to: if pollFailed is true, do not set timed_out=true or suggest the resume
command—instead set out["poll_failed"]=true (or out["error"]=true) and write an
error message to runtime.IO().ErrOut describing the polling failure; only when
pollFailed==false and ready==false emit next_command and timed_out=true as
before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 949dd857-1273-4d3f-a613-12d3533b0d19
📒 Files selected for processing (9)
shortcuts/drive/drive_delete.goshortcuts/drive/drive_delete_test.goshortcuts/drive/drive_move_common.goshortcuts/drive/drive_move_common_test.goshortcuts/drive/drive_task_result_test.goshortcuts/drive/shortcuts.goshortcuts/drive/shortcuts_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-delete.md
✅ Files skipped from review due to trivial changes (3)
- skills/lark-drive/SKILL.md
- shortcuts/drive/shortcuts.go
- skills/lark-drive/references/lark-drive-delete.md
🚧 Files skipped from review as they are similar to previous changes (4)
- shortcuts/drive/shortcuts_test.go
- shortcuts/drive/drive_task_result_test.go
- shortcuts/drive/drive_delete_test.go
- shortcuts/drive/drive_move_common.go
f1fdadf to
875ea79
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/drive/drive_io_test.go (1)
40-47: Guard global poll overrides to avoid cross-test interference.
withSingleDriveTaskCheckPollmutates package-level globals. If two tests call this helper concurrently, poll settings can interleave and cause flaky behavior.Suggested hardening
import ( "bytes" "net/http" "os" "strings" + "sync" "testing" @@ ) +var driveTaskCheckPollMu sync.Mutex + func withSingleDriveTaskCheckPoll(t *testing.T) { t.Helper() + driveTaskCheckPollMu.Lock() prevAttempts, prevInterval := driveTaskCheckPollAttempts, driveTaskCheckPollInterval driveTaskCheckPollAttempts, driveTaskCheckPollInterval = 1, 0 t.Cleanup(func() { driveTaskCheckPollAttempts, driveTaskCheckPollInterval = prevAttempts, prevInterval + driveTaskCheckPollMu.Unlock() }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_io_test.go` around lines 40 - 47, The helper withSingleDriveTaskCheckPoll mutates package-level globals driveTaskCheckPollAttempts and driveTaskCheckPollInterval and can race when tests run concurrently; protect those mutations by introducing a package-level sync.Mutex (e.g., driveTaskPollMu) and lock it at the start of withSingleDriveTaskCheckPoll and release it in the t.Cleanup closure after restoring prev values so concurrent tests cannot interleave changes to the globals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/drive/drive_io_test.go`:
- Around line 40-47: The helper withSingleDriveTaskCheckPoll mutates
package-level globals driveTaskCheckPollAttempts and driveTaskCheckPollInterval
and can race when tests run concurrently; protect those mutations by introducing
a package-level sync.Mutex (e.g., driveTaskPollMu) and lock it at the start of
withSingleDriveTaskCheckPoll and release it in the t.Cleanup closure after
restoring prev values so concurrent tests cannot interleave changes to the
globals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08012e07-8460-4427-a6ac-ec91329b3a19
📒 Files selected for processing (10)
shortcuts/drive/drive_delete.goshortcuts/drive/drive_delete_test.goshortcuts/drive/drive_io_test.goshortcuts/drive/drive_move_common.goshortcuts/drive/drive_move_common_test.goshortcuts/drive/drive_task_result_test.goshortcuts/drive/shortcuts.goshortcuts/drive/shortcuts_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-delete.md
✅ Files skipped from review due to trivial changes (3)
- shortcuts/drive/shortcuts_test.go
- skills/lark-drive/SKILL.md
- skills/lark-drive/references/lark-drive-delete.md
🚧 Files skipped from review as they are similar to previous changes (3)
- shortcuts/drive/shortcuts.go
- shortcuts/drive/drive_task_result_test.go
- shortcuts/drive/drive_move_common_test.go
875ea79 to
a64fe7c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
shortcuts/drive/drive_delete_test.go (1)
128-215: Add a regression case for the all-error polling path.This table covers success, pending, and terminal failure statuses, but it never exercises
pollDriveTaskCheck’s!seenStatus && lastErr != nilbranch inshortcuts/drive/drive_move_common.go. A single-attempttask_checkAPI/transport error case here would pin the false-timeout fix from this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_delete_test.go` around lines 128 - 215, Add a regression case to TestDriveDeleteFolderTaskCheckOutcomes that simulates a single-attempt transport/API error for the task_check call so pollDriveTaskCheck's branch for "!seenStatus && lastErr != nil" is exercised: in the test table add a case (e.g., name "task_check error") whose reg.Register for URL "/open-apis/drive/v1/files/task_check" returns an HTTP error (non-200 status or httpmock transport error) instead of a normal body, and assert the command returns an error containing a short identifying message (e.g., "task_check" or "transport error") so the test fails when pollDriveTaskCheck in shortcuts/drive/drive_move_common.go doesn't handle the single-attempt error path correctly.shortcuts/drive/drive_delete.go (1)
42-43: Make the resume command self-contained.
DriveDeletesupports bothuserandbot, butnext_commandonly preservestask_id. Threading the selected--asthroughdriveTaskCheckResultCommandwould make the follow-up reproducible instead of depending on whatever auth default is active later.Also applies to: 117-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_delete.go` around lines 42 - 43, The follow-up command built in DriveDelete is not capturing the chosen auth ("--as" / auth type), so replaying the next_command only preserves task_id and may use a different default auth later; update the DriveDelete flow to include the selected auth type in next_command (e.g., add an auth field like "as" or "authType" alongside task_id) and thread that through to driveTaskCheckResultCommand so it reads and uses that auth value when reconstructing/executing the follow-up; ensure both places that build next_command (the DriveDelete response and any other builder referenced by driveTaskCheckResultCommand) include and consume this auth parameter so the follow-up is reproducible for both "user" and "bot".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/drive/drive_delete_test.go`:
- Around line 128-215: Add a regression case to
TestDriveDeleteFolderTaskCheckOutcomes that simulates a single-attempt
transport/API error for the task_check call so pollDriveTaskCheck's branch for
"!seenStatus && lastErr != nil" is exercised: in the test table add a case
(e.g., name "task_check error") whose reg.Register for URL
"/open-apis/drive/v1/files/task_check" returns an HTTP error (non-200 status or
httpmock transport error) instead of a normal body, and assert the command
returns an error containing a short identifying message (e.g., "task_check" or
"transport error") so the test fails when pollDriveTaskCheck in
shortcuts/drive/drive_move_common.go doesn't handle the single-attempt error
path correctly.
In `@shortcuts/drive/drive_delete.go`:
- Around line 42-43: The follow-up command built in DriveDelete is not capturing
the chosen auth ("--as" / auth type), so replaying the next_command only
preserves task_id and may use a different default auth later; update the
DriveDelete flow to include the selected auth type in next_command (e.g., add an
auth field like "as" or "authType" alongside task_id) and thread that through to
driveTaskCheckResultCommand so it reads and uses that auth value when
reconstructing/executing the follow-up; ensure both places that build
next_command (the DriveDelete response and any other builder referenced by
driveTaskCheckResultCommand) include and consume this auth parameter so the
follow-up is reproducible for both "user" and "bot".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: db57902c-7adb-4d1c-b9d7-68d6b2ee2f10
📒 Files selected for processing (10)
shortcuts/drive/drive_delete.goshortcuts/drive/drive_delete_test.goshortcuts/drive/drive_io_test.goshortcuts/drive/drive_move_common.goshortcuts/drive/drive_move_common_test.goshortcuts/drive/drive_task_result_test.goshortcuts/drive/shortcuts.goshortcuts/drive/shortcuts_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-delete.md
✅ Files skipped from review due to trivial changes (3)
- shortcuts/drive/shortcuts_test.go
- skills/lark-drive/references/lark-drive-delete.md
- shortcuts/drive/drive_move_common_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- skills/lark-drive/SKILL.md
- shortcuts/drive/drive_task_result_test.go
- shortcuts/drive/drive_io_test.go
- shortcuts/drive/shortcuts.go
Change-Id: Ifb34f67296b800501a1b4960e02d5fed3382b84a
a64fe7c to
8236288
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_delete.go`:
- Around line 16-26: driveDeleteAllowedTypes contains "shortcut" while
driveMoveAllowedTypes does not, causing an inconsistency; verify the Drive API
behavior for moving shortcuts and then update the code: if the API allows moving
shortcuts, add "shortcut": true to driveMoveAllowedTypes (symbol:
driveMoveAllowedTypes) so both allowlists match; if the API disallows moving
shortcuts, add a clear inline comment or documentation near
driveMoveAllowedTypes explaining that shortcuts cannot be moved and why
(referencing driveDeleteAllowedTypes and driveMoveAllowedTypes).
🪄 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: 01f3d620-0068-478e-aa7e-b77e364a7016
📒 Files selected for processing (11)
shortcuts/drive/drive_delete.goshortcuts/drive/drive_delete_test.goshortcuts/drive/drive_io_test.goshortcuts/drive/drive_move.goshortcuts/drive/drive_move_common.goshortcuts/drive/drive_move_common_test.goshortcuts/drive/drive_task_result_test.goshortcuts/drive/shortcuts.goshortcuts/drive/shortcuts_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-delete.md
✅ Files skipped from review due to trivial changes (5)
- skills/lark-drive/SKILL.md
- shortcuts/drive/shortcuts_test.go
- shortcuts/drive/shortcuts.go
- shortcuts/drive/drive_io_test.go
- skills/lark-drive/references/lark-drive-delete.md
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/drive/drive_task_result_test.go
- shortcuts/drive/drive_move_common_test.go
…larksuite#415) Change-Id: Ifb34f67296b800501a1b4960e02d5fed3382b84a
…larksuite#415) Change-Id: Ifb34f67296b800501a1b4960e02d5fed3382b84a
Summary
Add a new
lark-cli drive +deleteshortcut for deleting Drive files and folders. Folder deletes are handled as async operations with bounded polling and a resumable+task_resultfollow-up command so AI agents and humans get a clear, actionableresult shape.
Changes
drive +deleteshortcut with support for Drive files and folderstask_checkfollow-up flow and fix shared status parsing so"fail"is treated as a terminal failure stateTest Plan
lark xxxcommand works as expectedManual verification:
go test ./shortcuts/drivemake unit-testlark-cli drive +delete --as user --file-token <folder_token> --type folder --yesflowlark-cli drive +task_result --scenario task_check --task-id <task_id>returnedstatus=successRelated Issues
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests