feat(drive): add +status shortcut for content-hash diff#692
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 Changes
Sequence DiagramsequenceDiagram
participant User
participant Cmd as "drive +status"
participant LocalFS as "Local Filesystem"
participant DriveAPI as "Drive API"
participant Hasher as "SHA-256 Hasher"
User->>Cmd: run with --local-dir, --folder-token, --files-from
Cmd->>Cmd: validate inputs
Cmd->>LocalFS: read manifest lines -> open files
LocalFS-->>Hasher: stream file bytes
Hasher-->>Cmd: local hashes by rel_path
Cmd->>DriveAPI: list folder contents (paginated, recursive)
DriveAPI-->>Cmd: file entries (only type=="file")
Cmd->>Cmd: map remote rel_path -> file_token
loop for each rel_path in intersection
Cmd->>DriveAPI: download remote file (file_token)
DriveAPI-->>Hasher: stream remote bytes
Hasher-->>Cmd: remote hash
Cmd->>Cmd: compare -> modified / unchanged
end
Cmd->>Cmd: mark new_local and new_remote
Cmd-->>User: output JSON {new_local,new_remote,modified,unchanged}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #692 +/- ##
==========================================
+ Coverage 63.55% 63.83% +0.27%
==========================================
Files 497 501 +4
Lines 42455 43695 +1240
==========================================
+ Hits 26984 27893 +909
- Misses 13129 13342 +213
- Partials 2342 2460 +118 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
shortcuts/drive/shortcuts_test.go (1)
8-47: Consider asserting the exact shortcut order.This membership-only check will miss regressions where
Shortcuts()returns the right set in the wrong order. If the registry order is user-visible, a slice-by-slice assertion would be safer.🤖 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 8 - 47, The test only checks membership and misses order regressions; change TestShortcutsIncludesExpectedCommands to assert the exact ordering by extracting commands from Shortcuts() (use the existing got variable: iterate over got to build a []string of commands) and compare that slice to want (e.g., with reflect.DeepEqual or cmp.Diff) instead of only checking lengths and a map of seen; keep the duplicate-check but replace the final membership loop with an ordered slice equality assertion so the test fails if Shortcuts() returns the correct items in the wrong order.
🤖 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_status_test.go`:
- Around line 135-173: Add tests that exercise the folder-token validation path
for the DriveStatus command: create one test that omits --folder-token and
asserts mountAndRunDrive(DriveStatus, ["+status","--local-dir", tmpDir,
"--as","bot"], ...) returns a validation error, and another test that supplies
an invalid/ malformed --folder-token (e.g. empty string or bad format) and
asserts the error message mentions folder-token format; follow the pattern and
helpers used by TestDriveStatusRejectsMissingLocalDir and
TestDriveStatusRejectsLocalFile (use withDriveWorkingDir, mountAndRunDrive,
DriveStatus and the same argument style) so the new tests cover required and
format validation for --folder-token.
In `@shortcuts/drive/drive_status.go`:
- Around line 76-81: The DryRun endpoint for the Drive shortcut (the DryRun func
in drive_status.go) is missing a required E2E dry-run test; add a new dry-run
E2E test under tests/cli_e2e/dryrun/ (or the drive domain test dir) that
exercises the CLI in dry-run mode for this shortcut: invoke the CLI command with
--dry-run plus the same flags used by the DryRun implementation (e.g.,
--local-dir and --folder-token), assert the tool performs the expected dry-run
HTTP request to GET /open-apis/drive/v1/files with the folder_token query param,
and validate the dry-run output contains the descriptive text from Desc; follow
the existing dry-run test harness patterns in the repo for setup/teardown and
request assertions.
---
Nitpick comments:
In `@shortcuts/drive/shortcuts_test.go`:
- Around line 8-47: The test only checks membership and misses order
regressions; change TestShortcutsIncludesExpectedCommands to assert the exact
ordering by extracting commands from Shortcuts() (use the existing got variable:
iterate over got to build a []string of commands) and compare that slice to want
(e.g., with reflect.DeepEqual or cmp.Diff) instead of only checking lengths and
a map of seen; keep the duplicate-check but replace the final membership loop
with an ordered slice equality assertion so the test fails if Shortcuts()
returns the correct items in the wrong order.
🪄 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: 84a2de7a-6eff-4fe6-b431-b1a41e75310a
📒 Files selected for processing (4)
shortcuts/drive/drive_status.goshortcuts/drive/drive_status_test.goshortcuts/drive/shortcuts.goshortcuts/drive/shortcuts_test.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@ac67edf9adada80c1d26d5fbd08e0a171e7b2b8c🧩 Skill updatenpx skills add larksuite/cli#feat/drive-status -y -g |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shortcuts/drive/drive_status.go (1)
75-80:⚠️ Potential issue | 🟠 MajorPlease confirm required dry-run E2E coverage for
drive +status.This shortcut adds new behavior and DryRun shape, but I can’t verify an accompanying dry-run E2E test from the provided diff context. Please confirm it exists under
tests/cli_e2e/dryrun/(or drive domain equivalent).#!/bin/bash # Check for dry-run E2E coverage of drive +status fd -i '_test.go' tests/cli_e2e rg -n --type=go --iglob '*_test.go' 'drive\s+\+status|--local-dir|--folder-token|--dry-run' tests/cli_e2eBased on learnings "Dry-run E2E tests are required for every shortcut change; place in
tests/cli_e2e/dryrun/or corresponding domain directory".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_status.go` around lines 75 - 80, Add or confirm a dry-run E2E test that covers the new DryRun shape for the drive +status shortcut: create/verify a test under tests/cli_e2e/dryrun (or a drive domain subdir) that invokes the CLI with drive +status and the flags --local-dir and --folder-token and asserts the DryRun HTTP GET to /open-apis/drive/v1/files with folder_token is produced; reference the DryRun function and its Desc text in shortcuts/drive/drive_status.go to ensure the test exercises the exact dry-run behavior.
🤖 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_status.go`:
- Around line 75-80: Add or confirm a dry-run E2E test that covers the new
DryRun shape for the drive +status shortcut: create/verify a test under
tests/cli_e2e/dryrun (or a drive domain subdir) that invokes the CLI with drive
+status and the flags --local-dir and --folder-token and asserts the DryRun HTTP
GET to /open-apis/drive/v1/files with folder_token is produced; reference the
DryRun function and its Desc text in shortcuts/drive/drive_status.go to ensure
the test exercises the exact dry-run behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e8d7c01-362d-45b0-8ed1-68034e2f4bc1
📒 Files selected for processing (1)
shortcuts/drive/drive_status.go
ea6cd37 to
d23598f
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_status.go`:
- Around line 206-209: The branch that now only checks strings.HasPrefix(p,
"../") should also reject a bare ".." when cleanRoot is non-empty: add a guard
that returns "" when p == ".." in the same conditional block so that callers
(e.g., where filepath.Join(cleanRoot, p) is used) don't get a stray ".."
returned; update the logic around the p variable in drive_status.go to mirror
the earlier check for p == ".." when cleanRoot == "." or "" so both cases
consistently reject directory traversal.
🪄 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: 2bdde10a-c4b2-459c-b78a-eeb886e9046f
📒 Files selected for processing (6)
shortcuts/drive/drive_status.goshortcuts/drive/drive_status_test.goshortcuts/drive/shortcuts.goshortcuts/drive/shortcuts_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-status.md
✅ 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 (2)
- shortcuts/drive/drive_status_test.go
- skills/lark-drive/references/lark-drive-status.md
37f6fb3 to
482485b
Compare
Addresses two CodeRabbit review comments on PR #692: - Adds TestDriveStatusRejectsEmptyFolderToken and TestDriveStatusRejectsMalformedFolderToken so the Validate-stage required-check and the ResourceName format guard for --folder-token are exercised, not just --local-dir. - Adds tests/cli_e2e/drive/drive_status_dryrun_test.go which drives the real binary in dry-run mode and asserts: * the request shape (GET /open-apis/drive/v1/files with folder_token in the dry-run envelope), plus the description text, * --local-dir absolute paths are rejected by Validate (which still runs under --dry-run) with --local-dir surfaced in the message, * cobra's required-flag enforcement rejects a missing --folder-token before any custom validation.
Adds `drive +status`, a read-only diff primitive that walks --local-dir, recursively lists --folder-token, and reports four buckets — new_local, new_remote, modified, unchanged — by SHA-256 content hash. Implementation notes: - Drive's list/metas APIs do not expose a content hash, so files present on both sides are downloaded via DoAPIStream and hashed in memory (sha256 + io.Copy, no disk write). Files only on one side are not fetched. The command stays Risk: "read". - Only Drive entries with type=file participate. Online docs (docx, sheet, bitable, mindnote, slides) and shortcuts are skipped — there is no equivalent local binary to hash against. - --local-dir is funneled through the framework's validate.SafeLocalFlagPath helper so that absolute paths and any .. that escapes cwd are rejected with --local-dir in the error message (rather than the internal default --file). FileIO().Stat() then enforces existence and the IsDir check. - Local walk uses filepath.WalkDir behind a //nolint:forbidigo comment. The runtime FileIO interface has no walker today and shortcuts can't import internal/vfs; SafeInputPath has already bounded the walk root inside cwd, so the bare walk is acceptable until a runtime-level walker lands. - Scopes: drive:drive.metadata:readonly (list folders) + drive:file:download (fetch files for hashing). The broader drive:drive scope is disabled by enterprise policy in some tenants; this narrower pair was verified end-to-end. Tests cover the four-bucket categorization with a nested subfolder and docx/shortcut filtering, plus validation errors for missing local-dir, non-directory local-dir, and absolute-path local-dir.
Adds references/lark-drive-status.md covering parameters, output schema, the type=file scoping rule, and the network-traffic caveat (hash is streamed in memory, but bytes still cross the wire). Notes that --local-dir is bounded to cwd by the CLI's path validation, and that when a user wants to compare a directory outside cwd the agent should ask the user to relocate or to switch the agent's working directory rather than `cd`-ing on its own. Wires +status into the Shortcuts table in SKILL.md.
Addresses two CodeRabbit review comments on PR #692: - Adds TestDriveStatusRejectsEmptyFolderToken and TestDriveStatusRejectsMalformedFolderToken so the Validate-stage required-check and the ResourceName format guard for --folder-token are exercised, not just --local-dir. - Adds tests/cli_e2e/drive/drive_status_dryrun_test.go which drives the real binary in dry-run mode and asserts: * the request shape (GET /open-apis/drive/v1/files with folder_token in the dry-run envelope), plus the description text, * --local-dir absolute paths are rejected by Validate (which still runs under --dry-run) with --local-dir surfaced in the message, * cobra's required-flag enforcement rejects a missing --folder-token before any custom validation.
….. escape
Reported in PR review: --local-dir was validated through
SafeLocalFlagPath, but the actual walk used the user-supplied raw
string. SafeLocalFlagPath returns the original value (it only checks
the path through SafeInputPath and discards the canonical form), and
SafeInputPath itself relies on filepath.Clean for path normalization.
filepath.Clean shrinks "link/.." to "." purely as string manipulation,
so the validator sees a path inside cwd. The kernel, however, resolves
"link/.." through the symlink target's parent — which is outside cwd
and is what filepath.WalkDir actually traverses.
Fix: in Execute, resolve --local-dir via validate.SafeInputPath to
get the canonical absolute path (this one fully evaluates symlinks
across the entire path), and walk that path. Each absolute walk hit
is converted to a cwd-relative form via filepath.Rel against
validate.SafeInputPath(".") so FileIO.Open's existing SafeInputPath
guard (which rejects absolute paths) still applies.
Adds TestDriveStatusDoesNotEscapeViaSymlinkParentRef as a regression:
it stages an "escape" sibling directory containing a sentinel file,
adds a "link" symlink in cwd pointing into the escape directory, and
runs +status with --local-dir "link/..". Without this fix, the raw
walk visits the sentinel and leaks it into new_local; with the fix,
the walk stays inside the canonical cwd.
A standalone repro confirms the underlying behavior: raw
filepath.WalkDir("link/..", ...) traversed dozens of unrelated files
in the kernel-resolved parent directory; walking the canonical root
visits only the legitimate cwd contents.
…atus Adds two corner-case regressions to back up the canonical-root walk fix: - TestDriveStatusSkipsSymlinkInsideRoot: a child symlink under --local-dir that points to a sibling temp dir outside cwd. WalkDir's default policy must report it as a non-regular entry so the callback skips it, and the sentinel inside the target must not surface in new_local. This pins the contract our caller relies on (walk declines to follow child symlinks even when the canonical root resolves cleanly). - TestDriveStatusSurvivesCircularSymlinkInsideRoot: a child symlink pointing back at one of its ancestors. The walk must terminate and surface the legitimate sibling file; if WalkDir ever followed the loop, the per-test timeout would catch it.
bb4d909 to
32eaf8a
Compare
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).
…ve E2E) Three independent fixes flagged on PR #692: 1. Route the recursive Drive folder listing through common.PaginationMeta instead of reading next_page_token directly. The shared helper accepts both page_token and next_page_token, matching what okr/im already do and keeping +status safe against a backend field rename. Adds TestDriveStatusPaginatesRemoteListing, which serves a 2-page response where page 1 advertises the cursor as next_page_token and page 2 as page_token; either spelling alone would silently drop one page. 2. The skill doc previously suggested "or symlink the target into cwd" as a workaround for cwd-relative --local-dir. SafeInputPath calls filepath.EvalSymlinks before checking isUnderDir(canonicalCwd), so any symlink whose final target sits outside cwd still gets rejected as `unsafe file path`. Rewrite the section so agents stop steering users into a path that always errors out. 3. Add tests/cli_e2e/drive/drive_status_workflow_test.go — the live E2E that AGENTS.md requires for new shortcuts. Seeds a real Drive folder with three uploaded files (unchanged.txt, modified.txt, remote-only.txt), seeds a local tree with matching/diverging content plus a local-only.txt, runs +status, and asserts each of the four buckets contains exactly the file we expect with the right file_token. Cleanup of every uploaded file plus the parent folder is registered through the existing best-effort cleanup helpers. Coverage table bumped: drive +status moves to ✓ and the denominator goes from 28→29 to account for the new shortcut. Codex also flagged the local-side filepath.WalkDir as a vfs-bypass. Investigated: the depguard rule shortcuts-no-vfs explicitly forbids shortcuts from importing internal/vfs (see commit c1b0bed on the +pull branch where the same migration was rejected by CI). The filepath.WalkDir + nolint:forbidigo pattern in walkLocalForStatus is the lint-required convention until FileIO grows a walker, so leaving it as-is.
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).
…lder 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.
* 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.
Summary
Adds a new
drive +statusshortcut: a read-only diff primitive that walks--local-dir, recursively lists--folder-token, and reports four buckets by SHA-256 content hash.new_local— only in--local-dirnew_remote— only under--folder-tokenmodified— both sides exist, hashes differunchanged— both sides exist, hashes matchDesigned as the foundation any future
+push/+pull/+syncworkflow can build on.Design notes
GET /open-apis/drive/v1/files/:file_token/downloadand hashed viaio.Copy(sha256.New(), resp.Body)— never written to disk. Files only on one side are not fetched. Command staysRisk: "read".type=fileonly. Online docs (docx/sheet/bitable/mindnote/slides) and shortcuts are skipped because there is no equivalent local binary to hash against. Subfolders recurse via the standard list pagination loop.--local-diris bounded by cwd through three layers: (1) the framework'svalidate.SafeLocalFlagPath("--local-dir", ...)runs first so the error message references the correct flag name; (2)runtime.FileIO().Statre-appliesSafeInputPathfor existence + IsDir checks; (3) per-fileruntime.FileIO().Openduring walk. Absolute paths and any..that escapes cwd are rejected with structuredvalidationerrors.filepath.WalkDiris annotated with//nolint:forbidigo. Shortcuts can't importinternal/vfsand the runtimeFileIOinterface has no walker today; the bare walk is fine here becauseSafeInputPathhas already bounded the root. Worth replacing once a runtime-level walker lands.drive:drive.metadata:readonly(list folders) +drive:file:download(fetch files for hashing). The broaderdrive:driveis disabled by enterprise policy in some tenants; the narrower pair was verified end-to-end.Output shape
{ "new_local": [{"rel_path": "..."}], "new_remote": [{"rel_path": "...", "file_token": "..."}], "modified": [{"rel_path": "...", "file_token": "..."}], "unchanged": [{"rel_path": "...", "file_token": "..."}] }Test plan
A. Static checks
go build ./...go vet ./...golangci-lint run --new-from-rev=origin/main ./shortcuts/drive/...nolint:forbidigoon the walker, with comment)node scripts/skill-format-check/index.js skillsB. Unit tests (
go test ./shortcuts/drive/... -run TestDriveStatus)TestDriveStatusCategorizesByHashdocxandshortcutentries are skippedTestDriveStatusRejectsMissingLocalDir--local-dirpoints at a path that doesn't exist → validation errorTestDriveStatusRejectsLocalFile--local-dirpoints at a regular file → "is not a directory" errorTestDriveStatusRejectsAbsoluteLocalDir--local-dir /etc→ SafeInputPath rejectionTestShortcutsIncludesExpectedCommands+statusC. End-to-end against a real Drive folder (happy path)
Local tree (10 files, 7 directories) ↔ remote tree (9 files, 4 directories):
modifiedconflict.txt,docs/changelog.md,docs/中文文档.mdunchangeddocs/readme.md,images/logo.bin(binary),stub.txt(1 byte)new_localdeeply/nested/path/deep.txt(4-level nesting),empty-local-only.txt(0 bytes),images/new-icon.bin,shared/notes.txtnew_remotearchive/2025-summary.md(remote-only subdir),docs/deprecated.md,images/old-icon.binrel_path(lexicographic)docs/中文文档.md) round-trip without mojibakeimages/logo.bin) hashes identically on both sides →unchangedempty-local-only.txt) doesn't crash the hasher; correctly placed innew_localdeeply/nested/path/deep.txt) recurses correctlyarchive/) surfaces all its files innew_remoteshared/,deeply/) don't make remote API callsD.
--local-dirpath variants (all real binary)local/(trailing slash)./local(dot-slash prefix).(cwd itself)E. Path validation errors (real binary)
All return structured
validationerrors with the--local-dirflag name in the message (not the framework default--file):--local-dir /etc--local-dir: --file must be a relative path within the current directory, got "/etc"--local-dir ../escape--local-dir: --file "../escape" resolves outside the current working directory--local-dir does-not-existcannot read file: stat .../does-not-exist: no such file or directory--local-dir notadir.txt(regular file)--local-dir is not a directory: notadir.txt--local-dir $'lo\ncal'(newline char in name)cannot read file: stat ...: no such file or directory(newline is a legal Unix filename char, so the path itself is well-formed and the error comes from the missing target)F. Required / token validation
--local-dirError: required flag(s) "local-dir" not set--folder-tokenError: required flag(s) "folder-token" not set--local-dir ""--local-dir is required--folder-token ""--folder-token is required--folder-token "tok\nwithnewline"--folder-token contains invalid characters--folder-token "BOGUS_TOKEN_xxxxx"(passes format, fails API)log_idand troubleshooter URLdrive:driveattempt before scope narrowing)missing_scopewithauth loginhint — confirms framework checks token scopes before issuing the API callG. Dry-run
--dry-rundescription+GET /open-apis/drive/v1/files+folder_tokensummary; no real API callsH. Output schema
{ok, identity, data}{new_local, new_remote, modified, unchanged}new_local[*]schema{rel_path}(nofile_token, since the file isn't in Drive)new_remote / modified / unchanged[*]schema{rel_path, file_token}rel_pathseparator