3 Commits

Author	SHA1	Message	Date
fangshuyu-768	f1aa7d8f42	feat(drive): add modified-time smart sync mode (#859)	2026-05-14 14:10:35 +08:00
fangshuyu-768	4ba39ef392	fix(drive): handle duplicate remote sync paths (#803)	2026-05-11 17:51:23 +08:00
fangshuyu-768	30ad38d4b6	feat(drive): add +pull shortcut for one-way Drive → local mirror (#696) ... * feat(drive): add +pull shortcut to mirror a Drive folder onto local 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). * docs(skills): document drive +pull in lark-drive skill 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. * fix(drive): walk +pull on canonical absolute root to close symlink/.. 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). * test(drive): pin walker / download behavior on +pull symlink corner cases 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. * fix(drive): +pull --delete-local must not unlink local files shadowed 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. * fix(drive): count +pull delete failures in summary.failed 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. * fix(drive): swap +pull walk/remove back to filepath/os to satisfy depguard 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. * fix(drive): record remote folder rel_path in +pull allPaths Follow-up to 45fe4e3. The folder branch in drivePullListRemote merged descendant rel_paths into allPaths but never recorded the folder's own rel_path, so a local regular file with the same name as a remote folder still looked orphaned and got unlinked under --delete-local. Adds the missing allPaths[rel] for the folder case and a regression: TestDrivePullDeleteLocalPreservesLocalFileShadowedByRemoteFolder stages a Drive containing a folder named shadow alongside a downloadable file, with the local side holding a regular file named shadow; --delete-local --yes must download keep.txt and leave the shadow file untouched. * fix(drive): +pull pagination + dir/file conflict + skill doc symlink claim Codex review on PR #696 surfaced three issues; addressed in one go: 1. drivePullListRemote only honored next_page_token. The shared common.PaginationMeta helper accepts both page_token and next_page_token; switched +pull over so a backend reply using page_token no longer makes the lister stop at page 1 (which would silently drop later remote files from both download and --delete-local). 2. --if-exists=skip swallowed mirror conflicts. The skip/overwrite branch only checked Stat success, so a local directory shadowing a remote regular file was reported as action=skipped. Now Stat's IsDir() is checked first; the conflict surfaces as action=failed with a message naming the directory, under both --if-exists=skip and --if-exists=overwrite, and increments summary.failed. 3. Skill doc told callers to soft-link the target into cwd if they wanted to pull from outside cwd. That is wrong: SafeInputPath evaluates symlinks before the cwd check, so a symlink pointing out-of-tree is rejected. Replaced the bogus shortcut with the actually viable options (switch the agent working directory, physically move/copy the target, or skip the comparison). Two new regressions: - TestDrivePullSurfacesDirectoryFileMirrorConflict — table test over both policies asserting failed=1, no skipped, action=failed, plus the 'is a directory' hint in the error message. - TestDrivePullPaginationHandlesPageTokenField — first page returns page_token (not next_page_token) with has_more=true; asserts both pages are fetched and both files land on disk. * fix(drive): +pull exits non-zero on item failures; gate --delete-local Two PR-696 review fixes: - Item-level failures (download error, dir/file conflict, delete error) now surface as a structured partial_failure ExitError instead of a success envelope with summary.failed > 0. Exit code becomes non-zero and error.detail still carries the {summary, items[]} payload, so AI / script callers can detect the failure via the exit code without reaching into the JSON body. - A failed download pass now skips the --delete-local walk entirely. Previously +pull would continue removing local-only files even when the download phase had partially failed, leaving the mirror in a half-synced state (some Drive files missing locally AND some local-only files unlinked). Re-runs after fixing the download error recover cleanly. Skill doc / shortcut description / flag desc updated to call the operation a one-way file-level mirror, since --delete-local only unlinks regular files and does not prune empty local directories left behind by remote folder deletes (true directory-level mirroring is explicitly out of scope). Tests: existing dir/file-conflict and delete-failure cases now assert the partial_failure ExitError shape; new test covers the "download fails => --delete-local skipped" gating contract. * refactor(drive): consolidate folder-listing helpers into listRemoteFolder Closes the post-#692 / post-#709 TODO that lived in drive_pull.go (and the matching note in drive_push.go): both #692 and #709 are now on main, so the three near-identical recursive Drive folder listers can collapse into one. New shared helper in shortcuts/drive/list_remote.go: driveRemoteEntry { FileToken, Type, RelPath } listRemoteFolder(ctx, runtime, folderToken, relBase) -> map[rel]entry Returns one entry per Drive item (every type), keyed by rel_path. Subfolders are descended into and the folder's own entry is recorded so callers can reason about "this rel_path is occupied by a folder" without re-listing. Pagination via common.PaginationMeta is unchanged. Each shortcut now derives its own per-shortcut view from the unified listing: - drive_status.go: collapses to remoteFiles (Type=="file" -> token) for the content-hash diff. - drive_pull.go: derives remoteFiles (Type=="file") for the download set, plus remotePaths (every rel_path) as the --delete-local guard. - drive_push.go: derives remoteFiles (Type=="file") for upload / overwrite / orphan-delete, plus remoteFolders (Type=="folder") for the create_folder cache. drivePushRemoteEntry was a duplicate of driveRemoteEntry's first two fields and is dropped; the few call sites that read .FileToken keep working unchanged. Per-shortcut copies removed: - drive_status.go: listRemoteForStatus, joinRelStatus, driveStatusListPageSize/FileType/FolderType - drive_pull.go: drivePullListRemote, drivePullJoinRel, drivePullListPageSize/FileType/FolderType - drive_push.go: drivePushListRemote, drivePushJoinRel, drivePushListPageSize/FileType/FolderType, drivePushRemoteEntry drive_push_test.go's TestDrivePushHelpersRelPath is retargeted at the shared joinRelDrive; the docstrings on the same-name-conflict tests were tweaked to refer to "the remoteFiles view" instead of the just-removed drivePushListRemote. Net diff: +1 new file, -207 net lines across the four touched files. All existing unit + e2e dry-run tests pass without behavioral change; the rel_path / pagination / type-filter contracts each shortcut depends on are preserved by construction.	2026-04-30 17:07:59 +08:00