feat(drive): add +pull shortcut for one-way Drive → local mirror#696
Open
fangshuyu-768 wants to merge 7 commits intomainfrom
Open
feat(drive): add +pull shortcut for one-way Drive → local mirror#696fangshuyu-768 wants to merge 7 commits intomainfrom
fangshuyu-768 wants to merge 7 commits 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 DiagramsequenceDiagram
participant User
participant CLI as CLI\n(+pull)
participant DriveAPI as Drive API
participant LocalFS as Local\nFilesystem
User->>CLI: +pull --local-dir <path> --folder-token <token> [flags]
CLI->>CLI: Validate flags, resolve safe local root
CLI->>DriveAPI: GET /open-apis/drive/v1/files (paginated) with folder token
DriveAPI-->>CLI: File & folder metadata (pages)
CLI->>CLI: Build rel_path → file_token map (type == "file"), sort keys
loop For each remote file (sorted)
CLI->>LocalFS: stat target path
alt missing or --if-exists=overwrite
CLI->>DriveAPI: GET /download?file_token=<token>
DriveAPI-->>CLI: File bytes stream
CLI->>LocalFS: Write file (create parents)
LocalFS-->>CLI: OK / Error
else exists & --if-exists=skip
CLI->>CLI: Record skipped
end
end
alt --delete-local && --yes
CLI->>LocalFS: Walk local root for regular files
loop For each local file
alt rel_path not in remote map
CLI->>LocalFS: Delete file
LocalFS-->>CLI: Deleted / Error
end
end
end
CLI->>User: Emit structured output (items array + summary counts)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
domain/ccm
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #696 +/- ##
==========================================
+ Coverage 63.55% 63.81% +0.25%
==========================================
Files 497 501 +4
Lines 42455 43700 +1245
==========================================
+ Hits 26984 27887 +903
- Misses 13129 13350 +221
- Partials 2342 2463 +121 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@c1b0bed3aa8ea163d3f07db770f7458ea45eed3e🧩 Skill updatenpx skills add larksuite/cli#feat/drive-pull -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
🤖 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_pull.go`:
- Around line 143-152: The delete-local pass treats any local path not in
remoteFiles as orphaned, but remoteFiles only contains downloadable entries;
modify the logic in the delete branch (when deleteLocal is true) and in the
subsequent deletion section around drivePullListRemote usage (see
drivePullWalkLocal, remoteFiles, drivePullListRemote) to maintain two sets:
remoteFiles (downloadable entries/type=file) and remotePaths (all remote
relative paths regardless of type). Use remotePaths to decide which local files
to keep (skip deletion if rel exists in remotePaths) and reserve remoteFiles for
download/overwrite logic; update the checks in both the initial delete pass and
the later deletion loop (lines ~185-233 area) to reference remotePaths for
existence testing.
- Around line 143-156: Replace direct os.Remove and filepath.WalkDir usage with
the vfs abstraction: in the deletion loop where os.Remove(target) is called,
call vfs.Remove(target) instead; and change the implementation that gathers
localPaths (drivePullWalkLocal) to recursively traverse using vfs.ReadDir()
(iterating entries and recursing into subdirs) rather than filepath.WalkDir(),
so all file deletions and directory reads go through the mockable vfs layer;
ensure drivePullWalkLocal and the removal loop reference the vfs package's
Remove and ReadDir functions and keep the same semantics for relative paths and
error handling.
In `@tests/cli_e2e/drive/drive_pull_dryrun_test.go`:
- Around line 73-100: Update the dry-run assertions in
TestDrive_PullDryRunRejectsAbsoluteLocalDir (and the nearby dry-run test block
around lines 105-138) to follow the suite's dry-run contract: assert
result.ExitCode == 0 instead of non-zero, and check gjson.Get(result.Stdout,
"error") for a populated error field rather than relying on process exit code;
then ensure the error payload mentions "--local-dir" (use the existing result
variable and its Stdout/ Stderr to compose the check).
🪄 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: 75cdde8c-1557-4dfa-8fcf-3e5f8ab89908
📒 Files selected for processing (7)
shortcuts/drive/drive_pull.goshortcuts/drive/drive_pull_test.goshortcuts/drive/shortcuts.goshortcuts/drive/shortcuts_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-pull.mdtests/cli_e2e/drive/drive_pull_dryrun_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
shortcuts/drive/drive_pull.go (2)
194-195: 🛠️ Refactor suggestion | 🟠 MajorRoute the delete path through
vfsas well.The new delete flow still uses
os.Removeandfilepath.WalkDiron user-facing paths, which bypasses the repo filesystem abstraction the rest of the shortcut relies on for test mocking. Please switch this helper tovfs.Removeplus a small recursive walk built onvfs.ReadDir.As per coding guidelines
**/*.go: Usevfs.*functions instead ofos.*for all filesystem access to enable test mocking.Also applies to: 304-324
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_pull.go` around lines 194 - 195, The delete path currently calls os.Remove and filepath.WalkDir on user paths (e.g., the branch where absPath is removed and drivePullItem is appended) which bypasses the repo VFS; change these to use vfs.Remove and replace the recursive filepath.WalkDir logic with a small recursive walk implemented with vfs.ReadDir so all filesystem operations go through the VFS abstraction. Specifically, update the code that uses os.Remove(absPath) to call vfs.Remove(ctx, absPath) (or vfs.Remove(absPath) depending on your vfs API) and rewrite any filepath.WalkDir-based recursion (the block that walks and deletes children and populates items, including the surrounding helper logic that uses rel, absPath, items, drivePullItem) to iterate directories via vfs.ReadDir and recurse, preserving the same error handling and drivePullItem creation.
134-135:⚠️ Potential issue | 🔴 CriticalUse the full remote entry set for
--delete-local.
drivePullListRemoteonly records downloadabletype=fileentries, but the delete pass treats that map as the source of truth. A remotedocx/sheet/shortcutat the same relative path still looks orphaned locally and can be deleted under--delete-local, even though it is present in Drive.Proposed fix
- remoteFiles, err := drivePullListRemote(ctx, runtime, folderToken, "") + remoteFiles, remoteEntries, err := drivePullListRemote(ctx, runtime, folderToken, "") if err != nil { return err } @@ - if _, ok := remoteFiles[rel]; ok { + if _, ok := remoteEntries[rel]; ok { continue } @@ -func drivePullListRemote(ctx context.Context, runtime *common.RuntimeContext, folderToken, relBase string) (map[string]string, error) { +func drivePullListRemote(ctx context.Context, runtime *common.RuntimeContext, folderToken, relBase string) (map[string]string, map[string]struct{}, error) { files := make(map[string]string) + entries := make(map[string]struct{}) pageToken := "" for { @@ result, err := runtime.CallAPI("GET", "/open-apis/drive/v1/files", params, nil) if err != nil { - return nil, err + return nil, nil, err } @@ - switch fType { + rel := drivePullJoinRel(relBase, fName) + entries[rel] = struct{}{} + switch fType { case drivePullFileType: - files[drivePullJoinRel(relBase, fName)] = fToken + files[rel] = fToken case drivePullFolderType: - subFiles, err := drivePullListRemote(ctx, runtime, fToken, drivePullJoinRel(relBase, fName)) + subFiles, subEntries, err := drivePullListRemote(ctx, runtime, fToken, rel) if err != nil { - return nil, err + return nil, nil, err } for k, v := range subFiles { files[k] = v } + for k := range subEntries { + entries[k] = struct{}{} + } } } @@ - return files, nil + return files, entries, nil }Please add a
--delete-localregression where Drive contains a non-file entry at the same relative path.Also applies to: 176-189, 223-270
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_pull.go` around lines 134 - 135, The delete-local logic is using the map returned by drivePullListRemote (assigned to remoteFiles) which currently only contains downloadable entries (type=file), causing non-file remote items (docx/sheet/shortcut) at the same relative path to be treated as missing and deleted; update the code so the delete-local pass consults a full remote-entry set instead of the filtered downloadable map: either change drivePullListRemote to return all entries (or add an option like includeNonFile) and use that full map for the delete-local check, or add a separate call (e.g. drivePullListRemoteAll) and replace usages of remoteFiles in the delete-local removal logic in drive_pull.go (and the other delete-local sections) so non-file remote entries prevent local deletion.
🤖 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_pull.go`:
- Around line 181-196: The two branches that append drivePullItem with Action
"delete_failed" (the relErr branch that appends drivePullItem{RelPath: absPath,
Action: "delete_failed", Error: relErr.Error()} and the os.Remove error branch
that appends drivePullItem{RelPath: rel, Action: "delete_failed", Error:
err.Error()}) should also increment the overall failure counter used for the
JSON summary (the variable that tracks summary.failed). Update both locations to
increment that failure counter (e.g., summary.failed++ or the appropriate field
on the summary struct) immediately after appending the delete_failed item so the
final summary.failed reflects delete failures. Ensure you reference the same
summary variable used when building the JSON summary.
---
Duplicate comments:
In `@shortcuts/drive/drive_pull.go`:
- Around line 194-195: The delete path currently calls os.Remove and
filepath.WalkDir on user paths (e.g., the branch where absPath is removed and
drivePullItem is appended) which bypasses the repo VFS; change these to use
vfs.Remove and replace the recursive filepath.WalkDir logic with a small
recursive walk implemented with vfs.ReadDir so all filesystem operations go
through the VFS abstraction. Specifically, update the code that uses
os.Remove(absPath) to call vfs.Remove(ctx, absPath) (or vfs.Remove(absPath)
depending on your vfs API) and rewrite any filepath.WalkDir-based recursion (the
block that walks and deletes children and populates items, including the
surrounding helper logic that uses rel, absPath, items, drivePullItem) to
iterate directories via vfs.ReadDir and recurse, preserving the same error
handling and drivePullItem creation.
- Around line 134-135: The delete-local logic is using the map returned by
drivePullListRemote (assigned to remoteFiles) which currently only contains
downloadable entries (type=file), causing non-file remote items
(docx/sheet/shortcut) at the same relative path to be treated as missing and
deleted; update the code so the delete-local pass consults a full remote-entry
set instead of the filtered downloadable map: either change drivePullListRemote
to return all entries (or add an option like includeNonFile) and use that full
map for the delete-local check, or add a separate call (e.g.
drivePullListRemoteAll) and replace usages of remoteFiles in the delete-local
removal logic in drive_pull.go (and the other delete-local sections) so non-file
remote entries prevent local deletion.
🪄 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: 64b476f6-b0f2-48db-b104-7a761301670e
📒 Files selected for processing (2)
shortcuts/drive/drive_pull.goshortcuts/drive/drive_pull_test.go
Adds `drive +pull`, a one-way Drive → local mirror command. It recursively lists --folder-token, downloads each type=file entry into --local-dir at the matching relative path, and optionally deletes local files absent from the remote (mirror semantics). Implementation notes: - Listing recurses through subfolders with the standard 200-page pagination loop. Online docs (docx, sheet, bitable, mindnote, slides) and shortcuts are skipped since there is no equivalent local binary to write back. Folder tree is reproduced under --local-dir, with parent directories auto-created by FileIO.Save. - Per-file --if-exists=overwrite (default) | skip controls how pre-existing local files are treated; the framework's enum guard rejects any other value. - --delete-local is the only destructive flag and is bound to --yes in Validate: --delete-local without --yes is rejected upfront so no listing or download even runs. --delete-local --yes performs downloads first, then walks --local-dir and removes regular files not present in the remote map. This matches the spec doc's "high-risk-write" intent for --delete-local without making the default pull path require confirmation. - --local-dir is funneled through validate.SafeLocalFlagPath so errors reference --local-dir instead of the framework default --file. FileIO().Stat then enforces existence and IsDir. - Scopes: drive:drive.metadata:readonly + drive:file:download. The broader drive:drive is disabled by enterprise policy in some tenants. - Listing helper (drivePullListRemote) is duplicated locally rather than reused from drive_status.go because that change is still in open PR #692; once it merges, both can be lifted into a shared drive package helper. TODO marker is left in the code. Tests cover six unit scenarios (happy-path with nested subfolder + docx skipping, --if-exists=skip, --delete-local rejection without --yes, --delete-local --yes deletes orphans, absolute-path rejection, bad enum) and four E2E dry-run scenarios (request shape, absolute path rejection, --delete-local --yes guard, missing required flag).
Adds references/lark-drive-pull.md covering parameters, output schema (summary + per-item action breakdown), the type=file scoping rule, the --if-exists policy matrix, and the --delete-local + --yes safety contract. Calls out the network-traffic caveat (pull is full-download, unlike +status which only fetches when both sides have the file) and the cwd boundary on --local-dir. Wires +pull into the Shortcuts table in SKILL.md.
… escape Same root cause as the +status fix: --local-dir was validated through SafeLocalFlagPath but the walk used the user-supplied raw string. SafeLocalFlagPath returns the original value (the canonical form is discarded), and SafeInputPath itself relies on filepath.Clean for normalization, which shrinks "link/.." to "." purely as string manipulation. The kernel then resolves "link/.." through the symlink target's parent at walk time, putting the traversal outside cwd. For +pull the bug is more dangerous than for +status because it travels through --delete-local --yes — a raw walk would let the delete pass land on files outside cwd. Fix: - In Execute, resolve --local-dir via validate.SafeInputPath to get a canonical absolute path, and resolve "." the same way for cwd. - Convert the resolved root back to a cwd-relative form (filepath.Rel) for download targets so FileIO.Save's existing SafeOutputPath check (which rejects absolute paths) still applies. - For --delete-local, walk the canonical absolute root, then delete via the absolute path. Both values come from the validated safeRoot, so kernel path resolution cannot redirect a delete to a file outside the canonical subtree. - drivePullWalkLocal now returns absolute paths instead of rel paths; the caller computes the rel_path via filepath.Rel against safeRoot for output / remote-set membership checks. Adds TestDrivePullDeleteLocalDoesNotEscapeViaSymlinkParentRef as a regression: it stages an "escape" sibling directory containing a sentinel file, adds a "link" symlink in cwd pointing into it, and runs +pull --delete-local --yes against an empty remote with --local-dir "link/..". The sentinel must survive (proving --delete did not escape) and the in-cwd file must be removed (proving the walk did run).
…ases Adds three regressions on top of the canonical-root walk fix: - TestDrivePullSkipsSymlinkInsideRoot: a child symlink inside the validated root pointing to a sibling temp dir. Under --delete-local --yes with an empty remote, the sentinel inside the target must survive (walker did not follow the child symlink) and the in-cwd file must be deleted (walker did run). - TestDrivePullSurvivesCircularSymlinkInsideRoot: a child symlink pointing at one of its ancestors. The walk must terminate so the test does not hang on the per-test timeout. - TestDrivePullDownloadDoesNotEscapeViaSymlinkParentRef: pins the download half of the fix. With --local-dir "link/.." the canonical root resolves to cwd, so the remote file must land in cwd, not inside the symlink target's parent. The preexisting sentinel inside the escape directory must remain untouched.
fangshuyu-768
force-pushed
the
feat/drive-pull
branch
from
bc19471 to
0d8fb90
Compare
… by online docs CodeRabbit (PR #696) flagged that the --delete-local pass treated any local path missing from `remoteFiles` as orphaned, but `remoteFiles` only records type=file entries. If Drive held a docx/sheet/shortcut at the same rel_path as a local file, the local file would be unlinked even though Drive still owned that path. drivePullListRemote now returns two views: - files: rel_path -> file_token, type=file only (download/skip set) - allPaths: every entry's rel_path regardless of type The download loop continues to consume `files`; the --delete-local pass consults `allPaths`, so an online-doc shadow of a local filename keeps the local file safe. Also routes the local walk and the delete through the vfs abstraction (vfs.ReadDir + vfs.Remove) instead of filepath.WalkDir + os.Remove. This drops the //nolint:forbidigo justifications and lines up with how internal/keychain and internal/registry already do filesystem I/O. The recursive vfs.ReadDir walker preserves the same "do not follow child symlinks" semantics that filepath.WalkDir gave us, so the canonical-root escape protections in 240b772 stay intact. Adds TestDrivePullDeleteLocalPreservesLocalFileShadowedByOnlineDoc as a direct regression: Drive serves keep.txt (file) plus notes.docx (docx), local has both keep.txt and a hand-edited notes.docx; --delete-local --yes must download keep.txt, leave notes.docx untouched, and report deleted_local=0.
CodeRabbit (PR #696) flagged that both delete_failed branches in the --delete-local pass appended an item but left the `failed` counter at zero, so the JSON summary could legitimately report `"failed": 0` after a partially-failed mirror. Increment failed in both branches (the filepath.Rel error path and the vfs.Remove error path) so summary.failed reflects every item flagged delete_failed in items[]. Adds TestDrivePullDeleteLocalCountsFailureInSummary, which forces vfs.Remove to fail by chmod-ing the local dir 0o555 right before the run and restoring 0o755 in t.Cleanup so t.TempDir teardown still works.
Collaborator
Author
|
Pushed two follow-up commits addressing the CodeRabbit review:
|
There was a problem hiding this comment.
♻️ Duplicate comments (3)
shortcuts/drive/drive_pull.go (3)
181-196:⚠️ Potential issue | 🟠 MajorCount
delete_failedinsummary.failed.Both delete-failure branches append an error item but leave the summary counter unchanged, so callers can see
"failed": 0after a partial cleanup failure.💡 Proposed fix
for _, absPath := range localAbsPaths { rel, relErr := filepath.Rel(safeRoot, absPath) if relErr != nil { items = append(items, drivePullItem{RelPath: absPath, Action: "delete_failed", Error: relErr.Error()}) + failed++ continue } rel = filepath.ToSlash(rel) @@ if err := os.Remove(absPath); err != nil { //nolint:forbidigo // see comment above items = append(items, drivePullItem{RelPath: rel, Action: "delete_failed", Error: err.Error()}) + failed++ continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_pull.go` around lines 181 - 196, The delete-failure branches add drivePullItem entries but don't increment the summary.failed counter, so update both error paths in drive_pull.go (the relErr branch that appends drivePullItem{RelPath: absPath, Action: "delete_failed", Error: relErr.Error()} and the os.Remove error branch that appends drivePullItem{RelPath: rel, Action: "delete_failed", Error: err.Error()}) to also increment the summary.failed field (e.g., summary.failed++ or appropriate exported field name) immediately after appending the item so the summary accurately reflects failures.
176-194: 🛠️ Refactor suggestion | 🟠 MajorRoute the destructive walk/delete path through the repo FS abstraction.
The download path already goes through
runtime.FileIO(), butfilepath.WalkDirandos.Removebypass the mockable filesystem layer for a user-supplied path. That makes the most dangerous branch harder to test and breaks the repo’s filesystem-access rule.As per coding guidelines, "Use
vfs.*functions instead ofos.*for all filesystem access to enable test mocking".Also applies to: 304-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_pull.go` around lines 176 - 194, The destructive local-walk and delete path currently uses filepath.WalkDir and os.Remove (in drivePullWalkLocal and the loop that removes absPath), bypassing the repo's mockable filesystem; replace those raw os/filepath usages with the repository FileIO abstraction obtained from runtime.FileIO() (e.g., use the FileIO's walk/ReadDir APIs and FileIO.Remove instead of filepath.WalkDir and os.Remove) so the delete branch is routable through the vfs mock; update both drivePullWalkLocal and the removal code paths (also the similar code around the 304-321 region) to call runtime.FileIO() and its methods.
134-150:⚠️ Potential issue | 🔴 CriticalDon’t treat skipped remote entries as orphaned during
--delete-local.
remoteFilesonly tracks downloadabletype=fileentries, but the delete pass also uses it as the source of truth. With--delete-local --yes, a local path that matches a remotedocx/sheet/shortcutis still treated as missing and gets deleted even though it still exists in Drive. Keep a separate remote-path set for skipped entries and use that for orphan detection; reserveremoteFilesfor download tokens. Please add a regression test with the fix.Also applies to: 176-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_pull.go` around lines 134 - 150, The delete-local logic is treating skipped remote entries (non-file types like docx/sheet/shortcut) as absent because remoteFiles only stores downloadable type=file tokens; create a separate set (e.g., skippedRemotePaths or remotePathSet) that records all remote paths encountered (including skipped types) while iterating remotePaths in the drive pull flow (functions/vars: drivePullListRemote, remoteFiles, remotePaths, drivePullItem), then use that full set for the orphan-detection / --delete-local pass and keep remoteFiles reserved solely for download tokens; add a regression test that simulates remote entries of non-file types and verifies --delete-local does not remove matching local files.
🤖 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_pull.go`:
- Around line 181-196: The delete-failure branches add drivePullItem entries but
don't increment the summary.failed counter, so update both error paths in
drive_pull.go (the relErr branch that appends drivePullItem{RelPath: absPath,
Action: "delete_failed", Error: relErr.Error()} and the os.Remove error branch
that appends drivePullItem{RelPath: rel, Action: "delete_failed", Error:
err.Error()}) to also increment the summary.failed field (e.g., summary.failed++
or appropriate exported field name) immediately after appending the item so the
summary accurately reflects failures.
- Around line 176-194: The destructive local-walk and delete path currently uses
filepath.WalkDir and os.Remove (in drivePullWalkLocal and the loop that removes
absPath), bypassing the repo's mockable filesystem; replace those raw
os/filepath usages with the repository FileIO abstraction obtained from
runtime.FileIO() (e.g., use the FileIO's walk/ReadDir APIs and FileIO.Remove
instead of filepath.WalkDir and os.Remove) so the delete branch is routable
through the vfs mock; update both drivePullWalkLocal and the removal code paths
(also the similar code around the 304-321 region) to call runtime.FileIO() and
its methods.
- Around line 134-150: The delete-local logic is treating skipped remote entries
(non-file types like docx/sheet/shortcut) as absent because remoteFiles only
stores downloadable type=file tokens; create a separate set (e.g.,
skippedRemotePaths or remotePathSet) that records all remote paths encountered
(including skipped types) while iterating remotePaths in the drive pull flow
(functions/vars: drivePullListRemote, remoteFiles, remotePaths, drivePullItem),
then use that full set for the orphan-detection / --delete-local pass and keep
remoteFiles reserved solely for download tokens; add a regression test that
simulates remote entries of non-file types and verifies --delete-local does not
remove matching local files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63cdb8e4-55c5-489c-94b7-c6972f88e3dd
📒 Files selected for processing (7)
shortcuts/drive/drive_pull.goshortcuts/drive/drive_pull_test.goshortcuts/drive/shortcuts.goshortcuts/drive/shortcuts_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-pull.mdtests/cli_e2e/drive/drive_pull_dryrun_test.go
✅ Files skipped from review due to trivial changes (3)
- shortcuts/drive/shortcuts.go
- shortcuts/drive/shortcuts_test.go
- skills/lark-drive/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/cli_e2e/drive/drive_pull_dryrun_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/drive/drive_pull.go`:
- Line 18: This file illegally imports internal/vfs; remove the direct import
from drive_pull.go and instead call an allowed abstraction: either (A) extend
the existing runtime/file I/O interface used by shortcuts with the minimal
methods you need (walk, delete, open, stat) and call those methods from
DrivePull-related functions, or (B) extract the vfs-backed helper into a new
non-internal package (e.g., vfshelper) that exposes the exact operations used
and is importable by shortcuts. Replace all direct references to package vfs
(including the walker/delete usage around the regions mentioned and any calls in
the functions handling pull logic at the blocks near lines 196-199 and 341-359)
with calls to the new interface/helper methods (preserve method names like
Walk/Delete/Open/Stat so callers remain obvious) and update constructors to
accept the interface/helper instead of importing internal/vfs. Ensure
tests/consumers pass and CI no longer imports internal packages.
- Around line 221-230: The folder rel_path must be added to the allPaths set
before recursing into its children: currently only the descendant paths from the
subfolder are merged, so add allPaths[rel_path] (or append rel_path) immediately
when you detect a remote folder entry (the same place where you currently
recurse/merge descendants into files/allPaths), then recurse and merge the
child's allPaths; update the logic that builds files and allPaths to treat
folders this way (referencing the variables allPaths and files in the
folder-handling branch). Also add a regression test that creates a local regular
file named "sub" and a remote folder "sub/..." and runs the pull with
--delete-local --yes to assert the local file is not deleted (i.e., remote
folder path prevents deletion).
🪄 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: 7ab76da6-dfce-4c6f-a899-ec0badb4a093
📒 Files selected for processing (2)
shortcuts/drive/drive_pull.goshortcuts/drive/drive_pull_test.go
…guard The previous fix-up commits used vfs.ReadDir + vfs.Remove inside the +pull shortcut, which depguard's "shortcuts-no-vfs" rule rejects: shortcuts cannot import internal/vfs directly. CI lint failed on the import line. Restore the same pattern used in drive_status.go and the prior +pull walker: - filepath.WalkDir to enumerate files under the canonical absolute root, gated by //nolint:forbidigo with a comment explaining why. - os.Remove for the actual delete, also gated by //nolint:forbidigo. The canonical-root safety still holds: validate.SafeInputPath bounds the walk root inside cwd before WalkDir runs, and WalkDir's default "do not follow child symlinks" policy is preserved. The two earlier fixes (drivePullListRemote returning allPaths so online-doc shadows do not look orphaned, and incrementing failed on delete_failed) stay in place. `go test ./shortcuts/drive/...` and `golangci-lint run --new-from-rev=origin/main` are both clean.
liangshuo-1
force-pushed
the
feat/drive-pull
branch
from
b8cfa76 to
c1b0bed
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
drive +pull, a one-way Drive → local mirror command. Recursively lists--folder-token, downloads eachtype=fileentry under the matching relative path in--local-dir, and optionally removes local files absent from the remote (mirror semantics).Output is split into a
summary(counts per action) plusitems[](per-file detail withrel_path,file_token,action, anderroron failure).This is the second of three P1 sync-disk commands in the design doc (4.1
+statusis in #692; 4.3+pushis the natural follow-up).Design notes
--folder-token; subfolders recurse. Subfolder structure is reproduced under--local-dir, with parent directories auto-created byFileIO.Save.type=fileonly. Online docs (docx/sheet/bitable/mindnote/slides) and shortcuts are skipped — there is no equivalent local binary to write back, so they would otherwise pollutesummary.failedor download empty placeholder bytes.--if-existscontrols per-file collision policy:overwrite(default) |skip. The framework's enum guard rejects any other value. Nokeep-both— callers who want it can rename and re-pull.--delete-localis the only destructive flag and is bound to--yesinValidate: without--yes, the command refuses to start (no listing, no download, nothing). With--yes, downloads run first, then--local-diris walked and any regular file not present in the remote map isos.Remove'd. This matches the spec doc's "high-risk-write" intent without making the default pull path require confirmation.--local-diris funneled throughvalidate.SafeLocalFlagPathso errors reference--local-dirrather than the framework's default--file.FileIO().Statthen enforces existence andIsDir.drive:drive.metadata:readonly+drive:file:download. The broaderdrive:driveis disabled by enterprise policy in some tenants; this narrower pair was verified end-to-end.drivePullListRemote) instead of being reused fromdrive_status.gobecause that change still sits in open PR feat(drive): add +status shortcut for content-hash diff #692. Once feat(drive): add +status shortcut for content-hash diff #692 merges, both copies should be lifted into a shared package-level helper. TODO marker left indrive_pull.go.Output shape
{ "summary": { "downloaded": 0, "skipped": 0, "failed": 0, "deleted_local": 0 }, "items": [ {"rel_path": "...", "file_token": "...", "action": "downloaded"}, {"rel_path": "...", "file_token": "...", "action": "skipped"}, {"rel_path": "...", "file_token": "...", "action": "failed", "error": "..."}, {"rel_path": "...", "action": "deleted_local"}, {"rel_path": "...", "action": "delete_failed", "error": "..."} ] }Test plan
Static checks
go build ./...cleango vet ./...cleangolangci-lint run --new-from-rev=origin/main— 0 issues (only the samenolint:forbidigojustifications used in+statusfor the walker and a singleos.Removein--delete-local, with comments)go test $(go list ./... | grep -v cli_e2e) -count=1— all packages greenUnit tests (
go test ./shortcuts/drive/... -run TestDrivePull)TestDrivePullDownloadsAndCreatesParents— happy path with a nested subfolder; verifiesdocxentries are skipped and parent directories are auto-createdTestDrivePullSkipsExistingWhenSkipPolicy— pre-existing local file is preserved verbatim,summary.skippedcounts itTestDrivePullDeleteLocalRequiresYes—--delete-localwithout--yesrejected upfrontTestDrivePullDeletesLocalOnlyFilesWhenYes—--delete-local --yesremoves both root-level orphan and orphan in a subdirectoryTestDrivePullRejectsAbsoluteLocalDir— error message references--local-dirTestDrivePullRejectsBadIfExistsEnum— framework enum guard kicks inTestShortcutsIncludesExpectedCommandsupdated to require+pullE2E dry-run tests (
tests/cli_e2e/drive/drive_pull_dryrun_test.go)TestDrive_PullDryRun— request shape: GET /open-apis/drive/v1/files + folder_token + description textTestDrive_PullDryRunRejectsAbsoluteLocalDir— Validate runs under --dry-run; --local-dir surfaced in errorTestDrive_PullDryRunRejectsDeleteLocalWithoutYes—--delete-localguard works under dry-run tooTestDrive_PullDryRunRejectsMissingFolderToken— cobra required-flag enforcementEnd-to-end against a real Drive folder (9 type=file entries across 4 levels)
downloaded=9 skipped=0 failed=0 deleted_local=0; full tree replicated locally includingdocs/中文文档.mdandimages/logo.bin(PNG header bytes intact)--if-exists=skipdownloaded=0 skipped=9 failed=0; existing local content preserved--delete-localwithout--yesvalidationerror:--delete-local requires --yes (high-risk: deletes local files absent from Drive)--delete-local --yesafter seedingstale-extra.txtandorphan-dir/file.txtdownloaded=9 deleted_local=2; both orphans removed, final tree exactly matches remoteSummary by CodeRabbit
New Features
Documentation
Tests