feat(drive): add +push shortcut for one-way local → Drive mirror#709
feat(drive): add +push shortcut for one-way local → Drive mirror#709fangshuyu-768 wants to merge 1 commit intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
participant User as CLI User
participant Validator as Input Validator
participant LocalFS as Local Filesystem
participant DriveAPI as Drive API
participant Cache as Token Cache
participant Output as Result Output
User->>Validator: run `drive +push` with flags
Validator->>Validator: validate & canonicalize inputs
Validator->>LocalFS: walk local dir -> collect files & dirs
Validator->>DriveAPI: list remote folder recursively
DriveAPI-->>Validator: remote files & folders
Validator->>Cache: resolve/create parent folder tokens as needed
loop per local file (sorted rel_path)
Validator->>DriveAPI: decide upload or skip/overwrite
alt upload single-part
Validator->>DriveAPI: `upload_all` (new or with `file_token`)
else multipart
Validator->>DriveAPI: `upload_prepare`
loop upload parts
Validator->>DriveAPI: `upload_part`
end
Validator->>DriveAPI: `upload_finish`
end
DriveAPI-->>Validator: confirm upload (may return version)
end
alt --delete-remote enabled
Validator->>DriveAPI: delete orphan remote files (DELETE)
DriveAPI-->>Validator: deletion responses
end
Validator->>Output: emit summary and items[]
Output-->>User: display structured result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
github-actions
Bot
added
domain/ccm
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #709 +/- ##
==========================================
+ Coverage 64.14% 64.15% +0.01%
==========================================
Files 504 505 +1
Lines 44285 44636 +351
==========================================
+ Hits 28406 28637 +231
- Misses 13411 13485 +74
- Partials 2468 2514 +46 ☔ 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@74caa7fe5f0a183f8534772753953c038426f961🧩 Skill updatenpx skills add larksuite/cli#feat/drive-push -y -g |
|
Addressed the three review findings in 2e77ce2: 1. Missing Added it to the default 2. Removed it from default 3. Empty directories silently dropped (drive_push.go:274, :205) Walker now also returns All checks rerun locally:
|
|
Round 2 of review addressed in 80681b2: 1. Multipart re-opened the file per block (
2. Failed overwrite dropped the returned Comment added explaining the overwrite-token-stability invariant (in-place overwrite preserves the existing token, so 3. Multipart had no unit test Added
Local checks rerun: build, vet, gofmt, golangci-lint (0 issues), |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/drive/drive_push.go (1)
417-472: Consider adding a depth limit for recursive folder listing.The recursive
drivePushListRemotehas no explicit depth bound. While Drive likely has practical limits, extremely deep folder structures could cause stack exhaustion. This mirrors existing behavior in+pull/+status, so it's not a blocker, but worth noting for future hardening.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_push.go` around lines 417 - 472, The recursive function drivePushListRemote currently has no depth limit; add a depth parameter (e.g., depth int) and a constant max depth (e.g., drivePushMaxDepth) and check at the start of drivePushListRemote to stop recursion when depth >= max (either return no children or a specific error); when recursing into subfolders, pass depth+1; update all call sites (including initial callers like the top-level drive push handler) to pass 0 as the initial depth and ensure the new max depth constant is defined near related symbols (drivePushListRemote, drivePushListPageSize, drivePushJoinRel).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/drive/drive_push.go`:
- Around line 417-472: The recursive function drivePushListRemote currently has
no depth limit; add a depth parameter (e.g., depth int) and a constant max depth
(e.g., drivePushMaxDepth) and check at the start of drivePushListRemote to stop
recursion when depth >= max (either return no children or a specific error);
when recursing into subfolders, pass depth+1; update all call sites (including
initial callers like the top-level drive push handler) to pass 0 as the initial
depth and ensure the new max depth constant is defined near related symbols
(drivePushListRemote, drivePushListPageSize, drivePushJoinRel).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d0f3f0e-acb8-40f5-bce5-614f4c6e6d23
📒 Files selected for processing (3)
shortcuts/drive/drive_push.goshortcuts/drive/drive_push_test.goskills/lark-drive/references/lark-drive-push.md
fangshuyu-768
force-pushed
the
feat/drive-push
branch
from
881b877 to
f88f16e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shortcuts/drive/drive_push_test.go (1)
720-817: This test doesn't actually pin the single-open optimization.The current assertions prove the multipart request sequence, but a regression back to reopen-per-block would still pass because nothing here counts or constrains
runtime.FileIO().Opencalls. If the shared-fd reuse is important, please instrumentOpenand assert it happens exactly once.As per coding guidelines, "Every behavior change must have an accompanying test".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_push_test.go` around lines 720 - 817, TestDrivePushUploadsLargeFileViaMultipart currently verifies multipart flow but doesn't assert reuse of the shared fd; wrap or mock runtime.FileIO().Open to count invocations before calling mountAndRunDrive and assert the counter equals 1 after the run to ensure drivePushUploadMultipart reused the open file descriptor; restore the original implementation afterwards to avoid test pollution and fail the test if Open was called more than once.
🤖 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_push_test.go`:
- Around line 337-339: The test is checking for the wrong JSON key; update the
assertion in drive_push_test.go to search for the serialized field name used by
drivePushItem: replace the check for `"token": "tok_link"` with a check for
`"file_token": "tok_link"` so the test will catch regressions that leak the
shortcut token into items[]. Locate the assertion that inspects the output
string (the existing strings.Contains(...) call) and change the substring to
`"file_token": "tok_link"`, keeping the same failure message and behavior.
---
Nitpick comments:
In `@shortcuts/drive/drive_push_test.go`:
- Around line 720-817: TestDrivePushUploadsLargeFileViaMultipart currently
verifies multipart flow but doesn't assert reuse of the shared fd; wrap or mock
runtime.FileIO().Open to count invocations before calling mountAndRunDrive and
assert the counter equals 1 after the run to ensure drivePushUploadMultipart
reused the open file descriptor; restore the original implementation afterwards
to avoid test pollution and fail the test if Open was called more than once.
🪄 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: d2eaada4-2bc4-4b15-bb8f-dc28d4872a3d
📒 Files selected for processing (3)
shortcuts/drive/drive_push.goshortcuts/drive/drive_push_test.goskills/lark-drive/references/lark-drive-push.md
✅ Files skipped from review due to trivial changes (1)
- skills/lark-drive/references/lark-drive-push.md
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
shortcuts/drive/drive_push_test.go (1)
337-339:⚠️ Potential issue | 🟡 MinorFix the shortcut-token leak assertion key.
This check looks for
"token": "tok_link", butitems[]serialize tokens underfile_token. As written, the test can miss the regression it intends to catch.💡 Suggested change
- if strings.Contains(out, `"token": "tok_link"`) { + if strings.Contains(out, `"file_token": "tok_link"`) { t.Errorf("shortcut tok_link must not appear in items: %s", out) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_push_test.go` around lines 337 - 339, The assertion is checking for the wrong JSON key: change the contains check that looks for `"token": "tok_link"` to instead search for the serialized key used in items, `"file_token": "tok_link"` (i.e., update the string in the if-condition that inspects the test output variable `out` so the test asserts that `"file_token": "tok_link"` does not appear).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cli_e2e/drive/drive_push_dryrun_test.go`:
- Around line 96-98: The test currently only asserts result.ExitCode != 0 for
Validate-stage rejects; change these to assert result.ExitCode == 2 (the
Validate failure code) and on failure include the combined stdout/stderr in the
failure message; locate the checks using the result variable in
drive_push_dryrun_test.go (the blocks that currently do `if result.ExitCode == 0
{ ... }` around the Validate path) and replace them with an explicit `if
result.ExitCode != 2 { t.Fatalf("expected exit=2 (Validate), got
exit=%d\noutput:\n%s", result.ExitCode, result.Stdout+result.Stderr) }` style
assertion so the test locks in Validate-classification and surfaces logs.
---
Duplicate comments:
In `@shortcuts/drive/drive_push_test.go`:
- Around line 337-339: The assertion is checking for the wrong JSON key: change
the contains check that looks for `"token": "tok_link"` to instead search for
the serialized key used in items, `"file_token": "tok_link"` (i.e., update the
string in the if-condition that inspects the test output variable `out` so the
test asserts that `"file_token": "tok_link"` does not appear).
🪄 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: e44d7529-b096-45b9-9dde-f97137ab341b
📒 Files selected for processing (7)
shortcuts/drive/drive_push.goshortcuts/drive/drive_push_test.goshortcuts/drive/shortcuts.goshortcuts/drive/shortcuts_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-push.mdtests/cli_e2e/drive/drive_push_dryrun_test.go
✅ Files skipped from review due to trivial changes (3)
- shortcuts/drive/shortcuts_test.go
- shortcuts/drive/shortcuts.go
- skills/lark-drive/references/lark-drive-push.md
🚧 Files skipped from review as they are similar to previous changes (1)
- skills/lark-drive/SKILL.md
| if result.ExitCode == 0 { | ||
| t.Fatalf("absolute --local-dir must be rejected, got exit=0\nstdout:\n%s", result.Stdout) | ||
| } |
There was a problem hiding this comment.
Assert the exact Validate failure exit code (2).
Line 96, Line 135, and Line 170 only check exit != 0. These cases exercise Validate-stage rejection and should lock in exit == 2, otherwise regressions in failure classification can slip through.
💡 Suggested change
- if result.ExitCode == 0 {
- t.Fatalf("absolute --local-dir must be rejected, got exit=0\nstdout:\n%s", result.Stdout)
- }
+ if result.ExitCode != 2 {
+ t.Fatalf("absolute --local-dir must be rejected with exit=2, got exit=%d\nstdout:\n%s\nstderr:\n%s", result.ExitCode, result.Stdout, result.Stderr)
+ }
...
- if result.ExitCode == 0 {
- t.Fatalf("--delete-remote without --yes must be rejected, got exit=0\nstdout:\n%s", result.Stdout)
- }
+ if result.ExitCode != 2 {
+ t.Fatalf("--delete-remote without --yes must be rejected with exit=2, got exit=%d\nstdout:\n%s\nstderr:\n%s", result.ExitCode, result.Stdout, result.Stderr)
+ }
...
- if result.ExitCode == 0 {
- t.Fatalf("missing --folder-token must be rejected, got exit=0\nstdout:\n%s", result.Stdout)
- }
+ if result.ExitCode != 2 {
+ t.Fatalf("missing --folder-token must be rejected with exit=2, got exit=%d\nstdout:\n%s\nstderr:\n%s", result.ExitCode, result.Stdout, result.Stderr)
+ }Based on learnings: In larksuite/cli E2E dry-run tests, Validate-callback failures should assert specific exit code 2 and inspect combined stdout/stderr for the message.
Also applies to: 135-137, 170-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/cli_e2e/drive/drive_push_dryrun_test.go` around lines 96 - 98, The test
currently only asserts result.ExitCode != 0 for Validate-stage rejects; change
these to assert result.ExitCode == 2 (the Validate failure code) and on failure
include the combined stdout/stderr in the failure message; locate the checks
using the result variable in drive_push_dryrun_test.go (the blocks that
currently do `if result.ExitCode == 0 { ... }` around the Validate path) and
replace them with an explicit `if result.ExitCode != 2 { t.Fatalf("expected
exit=2 (Validate), got exit=%d\noutput:\n%s", result.ExitCode,
result.Stdout+result.Stderr) }` style assertion so the test locks in
Validate-classification and surfaces logs.
f88f16e to
e7d3ed7
Compare
|
Round 3 of review addressed in e7d3ed7 (force-pushed; same single commit 1. Inline: shortcut-token assertion key was checking the wrong JSON name ( 2. Multipart fd-reuse pin: added a 3. Recursive folder listing depth limit (nitpick): intentionally not changing here. The bot's own note says it's "not a blocker, mirrors existing behavior in +pull/+status" — divergence would just leave +push out of step with #692/#696, and Drive's practical folder nesting depth is far below any stack-exhaustion threshold. If we want to harden this, it should be a single follow-up PR that lifts the listing helper into a shared package and adds the bound consistently across all three commands. Local checks rerun: build, vet, gofmt, golangci-lint (0 issues), full non-e2e suite green, |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/cli_e2e/drive/drive_push_dryrun_test.go (1)
96-98:⚠️ Potential issue | 🟡 MinorAssert
exit == 2for these Validate failures.These are Validate-stage rejects, so
exit != 0is too loose. A regression that routes them through a different failure path would still pass.Based on learnings: In
larksuite/cliE2E dry-run tests, Validate-callback failures should assert specific exit code 2 and inspect combined stdout/stderr for the message.Also applies to: 135-137, 170-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/drive/drive_push_dryrun_test.go` around lines 96 - 98, This test currently only asserts non-zero exit for Validate-stage rejects; change the assertion to require exit == 2 and check the combined stdout+stderr contains the expected validation message. Update the three occurrences that inspect result.ExitCode (the one using result.Stdout at lines ~96-98 and the similar checks at ~135-137 and ~170-172) to assert result.ExitCode == 2, and when failing print both result.Stdout and result.Stderr (or their concatenation) so the failure message and validation error are visible; keep references to the existing variables result.ExitCode, result.Stdout, and result.Stderr when making the change.
🤖 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_push_test.go`:
- Around line 468-496: The test currently stubs the upload_all response with
"file_token": "tok_keep" which matches the original token and leaves the test
blind to regressions; change the stubbed token to a distinct value (e.g.
"tok_new") in the POST /open-apis/drive/v1/files/upload_all stub and add an
assertion that the CLI output contains the returned token in items[].file_token
(check stdout for the new token string) so the test pins the returned token and
fails if the code wrongly re-uses entry.FileToken on failure. Ensure you update
the stub value (currently "tok_keep") and add the corresponding stdout contains
check for that token.
In `@shortcuts/drive/drive_push.go`:
- Around line 572-592: The code currently aborts on non-zero larkCode and
returns empty tokens, dropping any returned data.file_token; extract data :=
result["data"].(map[string]interface{}) and token := common.GetString(data,
"file_token") before returning the ErrAPI in the larkCode != 0 branch so you
preserve and return token (e.g. return token, "", output.ErrAPI(...,
result["error"])); this keeps overwrite-token stability used by Execute and
avoids falling back to entry.FileToken when upload_all reports an error but
still provides a file_token.
---
Duplicate comments:
In `@tests/cli_e2e/drive/drive_push_dryrun_test.go`:
- Around line 96-98: This test currently only asserts non-zero exit for
Validate-stage rejects; change the assertion to require exit == 2 and check the
combined stdout+stderr contains the expected validation message. Update the
three occurrences that inspect result.ExitCode (the one using result.Stdout at
lines ~96-98 and the similar checks at ~135-137 and ~170-172) to assert
result.ExitCode == 2, and when failing print both result.Stdout and
result.Stderr (or their concatenation) so the failure message and validation
error are visible; keep references to the existing variables result.ExitCode,
result.Stdout, and result.Stderr when making the change.
🪄 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: 045af397-01c7-4fe0-91a8-ddaeada86346
📒 Files selected for processing (7)
shortcuts/drive/drive_push.goshortcuts/drive/drive_push_test.goshortcuts/drive/shortcuts.goshortcuts/drive/shortcuts_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-push.mdtests/cli_e2e/drive/drive_push_dryrun_test.go
✅ Files skipped from review due to trivial changes (2)
- shortcuts/drive/shortcuts.go
- skills/lark-drive/references/lark-drive-push.md
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/drive/shortcuts_test.go
- skills/lark-drive/SKILL.md
| if larkCode := int(common.GetFloat(result, "code")); larkCode != 0 { | ||
| msg, _ := result["msg"].(string) | ||
| return "", "", output.ErrAPI(larkCode, fmt.Sprintf("upload failed: [%d] %s", larkCode, msg), result["error"]) | ||
| } | ||
| data, _ := result["data"].(map[string]interface{}) | ||
| token := common.GetString(data, "file_token") | ||
| if token == "" { | ||
| return "", "", output.Errorf(output.ExitAPI, "api_error", "upload failed: no file_token returned") | ||
| } | ||
| version := common.GetString(data, "version") | ||
| if version == "" { | ||
| // Some backends return the version under data_version; accept either | ||
| // per the markdown +overwrite contract. | ||
| version = common.GetString(data, "data_version") | ||
| } | ||
| if existingToken != "" && version == "" { | ||
| // The protocol guarantees a non-empty version on overwrite. If the | ||
| // deployed backend hasn't shipped the field yet we surface the gap | ||
| // rather than report a phantom success — callers can downgrade to | ||
| // --if-exists=skip in the meantime. | ||
| return token, "", output.Errorf(output.ExitAPI, "api_error", "overwrite for %q succeeded but no version was returned by upload_all", file.RelPath) |
There was a problem hiding this comment.
Preserve file_token on non-zero upload_all responses.
If the backend returns code != 0 and data.file_token, this path drops the token and the caller falls back to the stale entry.FileToken. That defeats the overwrite-error token-stability handling added in Execute.
Suggested fix
var result map[string]interface{}
if err := json.Unmarshal(apiResp.RawBody, &result); err != nil {
return "", "", output.Errorf(output.ExitAPI, "api_error", "upload failed: invalid response JSON: %v", err)
}
+ data, _ := result["data"].(map[string]interface{})
+ token := common.GetString(data, "file_token")
if larkCode := int(common.GetFloat(result, "code")); larkCode != 0 {
msg, _ := result["msg"].(string)
- return "", "", output.ErrAPI(larkCode, fmt.Sprintf("upload failed: [%d] %s", larkCode, msg), result["error"])
+ return token, "", output.ErrAPI(larkCode, fmt.Sprintf("upload failed: [%d] %s", larkCode, msg), result["error"])
}
- data, _ := result["data"].(map[string]interface{})
- token := common.GetString(data, "file_token")
if token == "" {
return "", "", output.Errorf(output.ExitAPI, "api_error", "upload failed: no file_token returned")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if larkCode := int(common.GetFloat(result, "code")); larkCode != 0 { | |
| msg, _ := result["msg"].(string) | |
| return "", "", output.ErrAPI(larkCode, fmt.Sprintf("upload failed: [%d] %s", larkCode, msg), result["error"]) | |
| } | |
| data, _ := result["data"].(map[string]interface{}) | |
| token := common.GetString(data, "file_token") | |
| if token == "" { | |
| return "", "", output.Errorf(output.ExitAPI, "api_error", "upload failed: no file_token returned") | |
| } | |
| version := common.GetString(data, "version") | |
| if version == "" { | |
| // Some backends return the version under data_version; accept either | |
| // per the markdown +overwrite contract. | |
| version = common.GetString(data, "data_version") | |
| } | |
| if existingToken != "" && version == "" { | |
| // The protocol guarantees a non-empty version on overwrite. If the | |
| // deployed backend hasn't shipped the field yet we surface the gap | |
| // rather than report a phantom success — callers can downgrade to | |
| // --if-exists=skip in the meantime. | |
| return token, "", output.Errorf(output.ExitAPI, "api_error", "overwrite for %q succeeded but no version was returned by upload_all", file.RelPath) | |
| data, _ := result["data"].(map[string]interface{}) | |
| token := common.GetString(data, "file_token") | |
| if larkCode := int(common.GetFloat(result, "code")); larkCode != 0 { | |
| msg, _ := result["msg"].(string) | |
| return token, "", output.ErrAPI(larkCode, fmt.Sprintf("upload failed: [%d] %s", larkCode, msg), result["error"]) | |
| } | |
| if token == "" { | |
| return "", "", output.Errorf(output.ExitAPI, "api_error", "upload failed: no file_token returned") | |
| } | |
| version := common.GetString(data, "version") | |
| if version == "" { | |
| // Some backends return the version under data_version; accept either | |
| // per the markdown +overwrite contract. | |
| version = common.GetString(data, "data_version") | |
| } | |
| if existingToken != "" && version == "" { | |
| // The protocol guarantees a non-empty version on overwrite. If the | |
| // deployed backend hasn't shipped the field yet we surface the gap | |
| // rather than report a phantom success — callers can downgrade to | |
| // --if-exists=skip in the meantime. | |
| return token, "", output.Errorf(output.ExitAPI, "api_error", "overwrite for %q succeeded but no version was returned by upload_all", file.RelPath) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/drive/drive_push.go` around lines 572 - 592, The code currently
aborts on non-zero larkCode and returns empty tokens, dropping any returned
data.file_token; extract data := result["data"].(map[string]interface{}) and
token := common.GetString(data, "file_token") before returning the ErrAPI in the
larkCode != 0 branch so you preserve and return token (e.g. return token, "",
output.ErrAPI(..., result["error"])); this keeps overwrite-token stability used
by Execute and avoids falling back to entry.FileToken when upload_all reports an
error but still provides a file_token.
Mirrors a local directory onto a Drive folder: walks --local-dir,
recursively lists --folder-token, mirrors local subdirectory structure
(including empty dirs) onto Drive via create_folder, and for each
rel_path uploads new files, overwrites already-present files, or skips
them per --if-exists. With --delete-remote --yes, any Drive type=file
entry absent locally is removed; Lark native cloud docs (docx/sheet/
bitable/mindnote/slides) and shortcuts are never overwritten or deleted.
Overwrite hits POST /open-apis/drive/v1/files/upload_all with the
existing file_token in the form body and the response's `version` is
propagated to items[].version, mirroring the markdown +overwrite
contract. Files >20MB fall back to the 3-step
upload_prepare/upload_part/upload_finish path with a single shared fd
reused via io.NewSectionReader per block.
Output is a {summary, items[]} envelope; items[].action is one of
uploaded / overwritten / skipped / folder_created / deleted_remote /
failed / delete_failed.
--delete-remote is bound to --yes upfront in Validate, same pattern as
+pull's --delete-local: a stray flag never silently deletes anything.
Path safety reuses the canonical-root walk + SafeInputPath mechanics
from the sibling +status / +pull commands.
Scopes: drive:drive.metadata:readonly + drive:file:upload +
space:folder:create. space:document:delete is intentionally NOT in the
default set — the framework's pre-flight scope check would otherwise
block plain pushes and dry-runs for callers that haven't granted delete;
--delete-remote --yes relies on the runtime DELETE call to surface
missing_scope. The skill ref calls out the scope so users running
mirror sync can grant it upfront.
13 unit tests cover the upload/overwrite/skip/delete matrix, online-doc
protection, same-name conflict between local file and native cloud doc,
empty-directory mirroring, multipart, scope/path validation, and helper
correctness. 4 dry-run e2e tests pin the request shape.
fangshuyu-768
force-pushed
the
feat/drive-push
branch
from
e7d3ed7 to
74caa7f
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
shortcuts/drive/drive_push.go (1)
568-576:⚠️ Potential issue | 🟠 MajorPreserve
data.file_tokenon non-zeroupload_allresponses.The
larkCode != 0branch still returns empty strings, so a partial overwrite that returnsdata.file_tokenloses the newer token and Line 244 falls back to the staleentry.FileToken. That breaks the token-stability handling this command is trying to preserve after partial-success overwrite errors.Suggested fix
var result map[string]interface{} if err := json.Unmarshal(apiResp.RawBody, &result); err != nil { return "", "", output.Errorf(output.ExitAPI, "api_error", "upload failed: invalid response JSON: %v", err) } + data, _ := result["data"].(map[string]interface{}) + token := common.GetString(data, "file_token") if larkCode := int(common.GetFloat(result, "code")); larkCode != 0 { msg, _ := result["msg"].(string) - return "", "", output.ErrAPI(larkCode, fmt.Sprintf("upload failed: [%d] %s", larkCode, msg), result["error"]) + return token, "", output.ErrAPI(larkCode, fmt.Sprintf("upload failed: [%d] %s", larkCode, msg), result["error"]) } - data, _ := result["data"].(map[string]interface{}) - token := common.GetString(data, "file_token") if token == "" { return "", "", output.Errorf(output.ExitAPI, "api_error", "upload failed: no file_token returned") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_push.go` around lines 568 - 576, When handling non-zero larkCode in the upload response, preserve and return any newer data.file_token instead of returning empty strings; before calling output.ErrAPI in the larkCode != 0 branch, extract data := result["data"].(map[string]interface{}) (or use common.GetString/common helper) and read fileToken := data["file_token"].(string) and return that token in the function's file-token return position rather than "" so the caller (which currently falls back to entry.FileToken) receives the updated token after partial-success overwrite errors; update the larkCode != 0 return to include that extracted fileToken alongside the error return.tests/cli_e2e/drive/drive_push_dryrun_test.go (1)
96-98:⚠️ Potential issue | 🟡 MinorAssert
Validate-stage failures with exit code 2.These checks are too loose right now. All three cases are expected to fail before DryRun completes, so
exit != 0won't catch regressions where the failure starts surfacing through a different path or exit classification.Suggested fix
- if result.ExitCode == 0 { - t.Fatalf("absolute --local-dir must be rejected, got exit=0\nstdout:\n%s", result.Stdout) - } + if result.ExitCode != 2 { + t.Fatalf("absolute --local-dir must be rejected with exit=2, got exit=%d\nstdout:\n%s\nstderr:\n%s", result.ExitCode, result.Stdout, result.Stderr) + } ... - if result.ExitCode == 0 { - t.Fatalf("--delete-remote without --yes must be rejected, got exit=0\nstdout:\n%s", result.Stdout) - } + if result.ExitCode != 2 { + t.Fatalf("--delete-remote without --yes must be rejected with exit=2, got exit=%d\nstdout:\n%s\nstderr:\n%s", result.ExitCode, result.Stdout, result.Stderr) + } ... - if result.ExitCode == 0 { - t.Fatalf("missing --folder-token must be rejected, got exit=0\nstdout:\n%s", result.Stdout) - } + if result.ExitCode != 2 { + t.Fatalf("missing --folder-token must be rejected with exit=2, got exit=%d\nstdout:\n%s\nstderr:\n%s", result.ExitCode, result.Stdout, result.Stderr) + }Based on learnings: In
larksuite/cliE2E dry-run tests, if a shortcut’s validation fails inside the Validate callback, the CLI must exit with code 2 and tests should assert that exact code while checking combined stdout/stderr for the message.Also applies to: 135-137, 170-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/drive/drive_push_dryrun_test.go` around lines 96 - 98, Update the assertions that currently check only result.ExitCode != 0 to require the specific Validate-stage exit code 2: replace the loose checks around result.ExitCode (in drive_push_dryrun_test.go where result is examined at the three spots) with an assertion that result.ExitCode == 2, and also assert that the combined output (result.Stdout+result.Stderr) contains the expected validation failure message reported by the Validate callback for the DryRun path; ensure you adjust the messages/assert helpers that reference result to look at both Stdout and Stderr together and to expect exit code 2 for these Validate-stage failures.
🤖 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_push.go`:
- Around line 568-576: When handling non-zero larkCode in the upload response,
preserve and return any newer data.file_token instead of returning empty
strings; before calling output.ErrAPI in the larkCode != 0 branch, extract data
:= result["data"].(map[string]interface{}) (or use common.GetString/common
helper) and read fileToken := data["file_token"].(string) and return that token
in the function's file-token return position rather than "" so the caller (which
currently falls back to entry.FileToken) receives the updated token after
partial-success overwrite errors; update the larkCode != 0 return to include
that extracted fileToken alongside the error return.
In `@tests/cli_e2e/drive/drive_push_dryrun_test.go`:
- Around line 96-98: Update the assertions that currently check only
result.ExitCode != 0 to require the specific Validate-stage exit code 2: replace
the loose checks around result.ExitCode (in drive_push_dryrun_test.go where
result is examined at the three spots) with an assertion that result.ExitCode ==
2, and also assert that the combined output (result.Stdout+result.Stderr)
contains the expected validation failure message reported by the Validate
callback for the DryRun path; ensure you adjust the messages/assert helpers that
reference result to look at both Stdout and Stderr together and to expect exit
code 2 for these Validate-stage failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b077c1e-4614-4a0d-a37a-5fa1d9f93c11
📒 Files selected for processing (7)
shortcuts/drive/drive_push.goshortcuts/drive/drive_push_test.goshortcuts/drive/shortcuts.goshortcuts/drive/shortcuts_test.goskills/lark-drive/SKILL.mdskills/lark-drive/references/lark-drive-push.mdtests/cli_e2e/drive/drive_push_dryrun_test.go
✅ Files skipped from review due to trivial changes (4)
- shortcuts/drive/shortcuts_test.go
- shortcuts/drive/shortcuts.go
- skills/lark-drive/references/lark-drive-push.md
- skills/lark-drive/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/drive/drive_push_test.go
1 participant
Summary
Adds
drive +push, a one-way local → Drive mirror command. Walks--local-dir, recursively lists--folder-token, mirrors local subdirectory structure (including empty dirs) onto Drive, and for each rel_path uploads new files, overwrites already-present files (carrying the existingfile_tokenonupload_allfor protocol-level versioning), or skips them per--if-exists. With--delete-remote --yes, any Drivetype=fileentry absent locally is removed; online docs and shortcuts are never touched.This is the third of the three P1 sync-disk commands (4.1
+statusis in #692; 4.2+pullis in #696; 4.3+pushis this PR).Output shape
{ "summary": { "uploaded": 0, "skipped": 0, "failed": 0, "deleted_remote": 0 }, "items": [ {"rel_path": "...", "file_token": "...", "action": "folder_created"}, {"rel_path": "...", "file_token": "...", "action": "uploaded", "size_bytes": 0}, {"rel_path": "...", "file_token": "...", "action": "overwritten", "version": "...", "size_bytes": 0}, {"rel_path": "...", "file_token": "...", "action": "skipped", "size_bytes": 0}, {"rel_path": "...", "action": "failed", "size_bytes": 0, "error": "..."}, {"rel_path": "...", "file_token": "...", "action": "deleted_remote"}, {"rel_path": "...", "file_token": "...", "action": "delete_failed", "error": "..."} ] }folder_createditems do not bumpsummary.uploaded— that field counts files only. Pre-existing remote folders cache-hit and stay silent (noitems[]entry).Design notes
file_tokenonupload_all. The overwrite path passes the existingfile_tokenas a form field onPOST /open-apis/drive/v1/files/upload_all, and the response is expected to carry a non-emptyversion(ordata_version) which is surfaced asitems[].version. This mirrors themarkdown +overwritecontract.file_token-on-upload_alloverwrite parameter and the matchingversionfield on the response may not be exposed on every Drive tenant yet. The implementation is written against the protocol; on tenants where the field has not shipped,--if-exists=overwritesurfaces a structuredapi_errorper file (overwrite for "..." succeeded but no version was returned by upload_all) and the rest of the push (new uploads, folder creation, deletes) keeps running. Callers can downgrade to--if-exists=skipuntil the backend catches up. The unit testTestDrivePushOverwriteWithoutVersionFailspins this behavior.--delete-remoteis bound to--yesinValidate, same pattern as+pull's--delete-local: without--yes, the command refuses upfront — no listing, no upload, no delete — so a stray flag never silently deletes anything. With--yes, uploads run first, then any remotetype=fileabsent locally isDELETEd.type=fileonly. Lark native cloud docs (docx/sheet/bitable/mindnote/slides) and shortcuts are never uploaded against, overwritten, or deleted by--delete-remote.drivePushListRemotefilters them out ofremoteFiles, so they are unreachable from both the upload loop (no overwrite) and the delete loop (not iterated). When a local regular file shares a rel_path with a native cloud doc, the local file is uploaded as a sibling type=file and the native doc is left untouched — Drive permits same-name entries of different types in one folder. Pinned byTestDrivePushDeleteRemoteSkipsOnlineDocsandTestDrivePushUploadsSiblingWhenRemoteSameNameIsNativeDoc, and verified live against a folder pre-populated with a native sheet and docx.POST /open-apis/drive/v1/files/create_folder. A per-executionfolderCacheis seeded from the remote listing so existing sub-folders are reused; newly created folders surface asitems[].action="folder_created".Executeprefers thefile_tokenreturned by the API and falls back toentry.FileTokenonly when the response carried none — this matters for the missing-version partial-success case where bytes already landed and the response carries a non-empty token alongside the error.--local-diris funneled throughvalidate.SafeLocalFlagPaththen walked from a canonical absolute root, same threat model as feat(drive): add +status shortcut for content-hash diff #692/feat(drive): add +pull shortcut for one-way Drive → local mirror #696. WalkDir's default child-symlink policy keeps traversal inside the validated subtree.drive:drive.metadata:readonly+drive:file:upload+space:folder:create. The broaderdrive:driveis disabled by enterprise policy in some tenants; this narrower set follows the precedent set by feat(drive): add +status shortcut for content-hash diff #692/feat(drive): add +pull shortcut for one-way Drive → local mirror #696.space:document:deleteis intentionally NOT in the default set even though--delete-remoteneeds it: the framework's pre-flight scope check (runner.go: checkShortcutScopes) runs unconditionally before Validate / dry-run, so declaring it would block every plain push and dry-run for callers that haven't granted delete. The skill ref calls out the scope so users running mirror sync can grant it upfront and avoid a half-synced state.upload_prepare/upload_part/upload_finishpath. The fd is opened once before the chunking loop and reused viaio.NewSectionReaderper block (saves oneOpen+ path validation per chunk vs. the legacy reopen-per-block pattern indrive_upload.go). The current multipartupload_finishcontract does not returnversion, soitems[].versionis empty for large-file overwrites; thefile_tokenandaction="overwritten"are still produced correctly.drivePushListRemote) instead of being lifted fromdrive_status.go/drive_pull.gobecause both feat(drive): add +status shortcut for content-hash diff #692 and feat(drive): add +pull shortcut for one-way Drive → local mirror #696 are still open. Once they merge, all three copies should be consolidated. TODO marker left indrive_push.go.Test plan
Static checks
go build ./...cleango vet ./...cleangofmtcleangolangci-lint run --new-from-rev=origin/main ./shortcuts/drive/...— 0 issues (only the samenolint:forbidigopattern used in+status/+pullfor the local walker, with comment)node scripts/skill-format-check/index.js skillspassesgo test $(go list ./... | grep -v cli_e2e) -count=1— all packages greenUnit tests (
go test ./shortcuts/drive/... -run TestDrivePush)TestDrivePushUploadsAndCreatesParents— happy path with a nested subfolder; verifiescreate_folderruns once and both files reach DriveTestDrivePushOverwritesWhenIfExistsOverwrite— overwrite path sendsfile_tokenin the form body and propagatesversiontoitems[].versionTestDrivePushSkipsWhenIfExistsSkip— pre-existing remote file is preserved without anupload_allcall;summary.skippedcounts itTestDrivePushDeleteRemoteRequiresYes—--delete-remotewithout--yesrejected upfrontTestDrivePushDeleteRemoteSkipsOnlineDocs—docx/shortcutentries that share a rel_path with a missing local file are NOT deleted; onlytype=fileorphans areTestDrivePushRejectsAbsoluteLocalDir— error message references--local-dirTestDrivePushRejectsBadIfExistsEnum— framework enum guard kicks inTestDrivePushOverwriteWithoutVersionFails— pins the behavior on tenants where the overwrite-version field hasn't shipped yet (structuredapi_errorper item,summary.failed=1)TestDrivePushReusesExistingRemoteFolder— when a remote folder exists at the target rel_path, uploadparent_nodereuses its token instead of callingcreate_folderTestDrivePushUploadsSiblingWhenRemoteSameNameIsNativeDoc— a remote Lark native cloud doc (sheet/docx/...) at the same rel_path as a local regular file is never overwritten and never reached by--delete-remote; the local file is uploaded as a sibling type=fileTestDrivePushMirrorsEmptyDirectories— empty local dirs surface asfolder_createdinstead of being silently droppedTestDrivePushUploadsLargeFileViaMultipart— exercises the 3-stepupload_prepare/ twoupload_part/upload_finishflow on a file one byte over the single-part threshold; pins the shared-fd refactor and assertsupload_preparebody omitsfile_tokenfor fresh uploadsTestDrivePushHelpersRelPath— pins the path utilities (drivePushParentRel/drivePushSplitRel/drivePushJoinRel)TestShortcutsIncludesExpectedCommandsupdated to require+pushE2E dry-run tests (
tests/cli_e2e/drive/drive_push_dryrun_test.go)TestDrive_PushDryRun— request shape:GET /open-apis/drive/v1/files+folder_token+ description includes both list and upload phrasingTestDrive_PushDryRunRejectsAbsoluteLocalDir— Validate runs under--dry-run;--local-dirsurfaced in errorTestDrive_PushDryRunRejectsDeleteRemoteWithoutYes—--delete-remoteguard works under dry-run tooTestDrive_PushDryRunRejectsMissingFolderToken— cobra required-flag enforcementManual end-to-end against a real Drive folder
Six-phase walkthrough on a freshly created Drive folder with a fixture of 6 files across 2 nested directories + 2 empty directories (one of which is itself a nested empty dir), including a multilingual filename
中文.txt, a binary file with PNG header bytes, and a 0-byte file.folder_createdsurfaces every newly minted directory including the nested emptyempty-parent/empty-child. The 0-byte file is rejected by the Drive backend with1061002 params error;drive +uploadagainst the same backend returns the identical error, so this is a backend constraint rather than a+pushregression. The failure surfaces initems[]asfailedwith the full structured error andlog_id; other files are unaffected andsummary.failed=1.--if-exists=skip). No local changes. Result:skipped=5, uploaded=0. Empty folders cache-hit silently (no re-create, noitems[]entry), confirming the folderCache contract holds across runs.docs/changelog.mdand re-push without--if-exists. Result: all 5 type=file entries withaction="overwritten"; each surfaces a non-empty distinctversionfield, and everyfile_tokenis preserved (the overwrite-token-stability invariant is verified live, not just unit-tested).drive +downloadof the modified file round-trips the new content byte-for-byte.--delete-remotewithout--yes. Result: rejected upfront in Validate, zero API calls issued.--delete-remote --yes). Locally removereadme.txt, adddocs/new-doc.md, re-push. Result:summary.uploaded=5(1 new + 4 overwrites),summary.deleted_remote=1(readme.txt),summary.failed=0. A follow-updrive files listconfirmsreadme.txtis gone from remote anddocs/new-doc.mdis present, alongside the 4 overwritten files and the 4 mirrored folders.GET /filesplan envelope; zero side-effecting requests issued.Manual end-to-end with native cloud documents present in the target folder
Separate live walkthrough on a folder pre-populated with a Lark native sheet (titled
report, type=sheet) and a Lark native docx (titledminutes, type=docx), both imported viadrive +import. Confirms the unit-test mock string values (type: "sheet"/type: "docx") match whatGET /open-apis/drive/v1/filesactually returns, and pins the same-name-conflict path against a real backend.data.csv,notes.md(no remote conflict) +reportandminutes(regular files whose names collide with the native sheet and docx). Result:summary.uploaded=4, failed=0; all four files emerge asaction="uploaded"with brand-newfile_tokens, none of which match the sheet's or docx's tokens. A follow-updrive files listshows the folder now holds 6 entries — the original sheet (token unchanged) + the original docx (token unchanged) + 4 newly uploaded type=file siblings, including two same-named pairs (report-sheet alongsidereport-file,minutes-docx alongsideminutes-file). Drive permits same-name siblings of different types.--delete-remote --yeswith same-name conflict. Locally remove the regularreportfile and re-push with--delete-remote --yes. Result:summary.deleted_remote=1deletes the type=filereport(Zlrbb…) by token, the native sheetreport(XiH2s…) is left strictly alone. Thedrive files listafterward confirms the sheet still has its original token, and the docx is also intact. Pins that the orphan-deletion loop iteratesremoteFiles(type=file only) by token rather than by name.Related
drive +status(open)drive +pull(open)Summary by CodeRabbit
New Features
Bug Fixes / Safety
Documentation
Tests