Open
Conversation
github-actions
bot
added
domain/ccm
📝 WalkthroughWalkthroughAdds a shared Drive media uploader supporting single-part ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant LocalFile
participant DriveAPI
Client->>LocalFile: open file, stat size
alt size <= single-part limit
Client->>DriveAPI: POST /open-apis/drive/v1/medias/upload_all (multipart/form-data)
DriveAPI-->>Client: { code, data{ file_token }, ... }
else multipart
Client->>DriveAPI: POST /open-apis/drive/v1/medias/upload_prepare (json)
DriveAPI-->>Client: { upload_id, block_size, block_num }
loop seq = 1..block_num
Client->>LocalFile: read block (block_size)
Client->>DriveAPI: POST /open-apis/drive/v1/medias/upload_part (multipart: seq, upload_id, chunk)
DriveAPI-->>Client: ack
end
Client->>DriveAPI: POST /open-apis/drive/v1/medias/upload_finish (json upload_id, block_num)
DriveAPI-->>Client: { file_token }
end
Client->>LocalFile: close file
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
shortcuts/common/drive_media_upload_test.go (1)
412-426: Consider adding config directory isolation.Per coding guidelines, test config state should be isolated using
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()). While the atomic counter provides AppID uniqueness, adding config directory isolation would ensure complete test isolation.💡 Suggested improvement
func newDriveMediaUploadTestRuntime(t *testing.T) (*RuntimeContext, *httpmock.Registry) { t.Helper() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) cfg := &core.CliConfig{ AppID: fmt.Sprintf("common-drive-media-test-%d", commonDriveMediaUploadTestSeq.Add(1)), AppSecret: "test-secret", Brand: core.BrandFeishu, }Based on learnings: "Isolate config state in Go tests by using
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/common/drive_media_upload_test.go` around lines 412 - 426, The test helper newDriveMediaUploadTestRuntime should isolate CLI config state by setting a temporary config dir; call t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) near the top of newDriveMediaUploadTestRuntime (before creating cfg or calling cmdutil.TestFactory) so each test uses its own config directory and avoids shared state.shortcuts/drive/drive_import_common.go (1)
86-95: Consider usingvfs.Statinstead ofos.Statfor filesystem access.Per coding guidelines, filesystem operations should use
vfs.*instead ofos.*. However, since this line existed before this PR (it's not marked with~), this may be pre-existing technical debt rather than a new violation introduced by this change.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_import_common.go` around lines 86 - 95, Replace the direct os.Stat call in uploadMediaForImport with the virtual filesystem helper: call vfs.Stat (matching the project's vfs API) to get importInfo and err, update imports accordingly, and keep the same error handling/flow (i.e., return output.ErrValidation(...) on error and pass importInfo.Size() into validateDriveImportFileSize). Ensure the change is made in the uploadMediaForImport function and that validateDriveImportFileSize continues to be called with the same filePath, docType, and fileSize.
🤖 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/common/drive_media_upload.go`:
- Around line 52-85: The file open calls in UploadDriveMediaAll and
uploadDriveMediaMultipartParts must validate the file paths using
validate.SafeInputPath before calling os.Open; update UploadDriveMediaAll
(around the os.Open(cfg.FilePath) call) and uploadDriveMediaMultipartParts
(around the os.Open usage at the other site) to call
validate.SafeInputPath(cfg.FilePath) (or the equivalent variable) and return a
validation error if it fails, then proceed to os.Open only after the path is
validated to match the project's safe-path pattern.
---
Nitpick comments:
In `@shortcuts/common/drive_media_upload_test.go`:
- Around line 412-426: The test helper newDriveMediaUploadTestRuntime should
isolate CLI config state by setting a temporary config dir; call
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) near the top of
newDriveMediaUploadTestRuntime (before creating cfg or calling
cmdutil.TestFactory) so each test uses its own config directory and avoids
shared state.
In `@shortcuts/drive/drive_import_common.go`:
- Around line 86-95: Replace the direct os.Stat call in uploadMediaForImport
with the virtual filesystem helper: call vfs.Stat (matching the project's vfs
API) to get importInfo and err, update imports accordingly, and keep the same
error handling/flow (i.e., return output.ErrValidation(...) on error and pass
importInfo.Size() into validateDriveImportFileSize). Ensure the change is made
in the uploadMediaForImport function and that validateDriveImportFileSize
continues to be called with the same filePath, docType, and fileSize.
🪄 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: 32e66156-b312-4cb3-b63d-97fcecd1d735
📒 Files selected for processing (8)
shortcuts/common/drive_media_upload.goshortcuts/common/drive_media_upload_test.goshortcuts/doc/doc_media_insert.goshortcuts/doc/doc_media_test.goshortcuts/doc/doc_media_upload.goshortcuts/drive/drive_import_common.goshortcuts/drive/drive_import_common_test.goskills/lark-doc/references/lark-doc-media-insert.md
💤 Files with no reviewable changes (1)
- shortcuts/drive/drive_import_common_test.go
Greptile SummaryThis PR extracts a shared Confidence Score: 5/5Safe to merge; no blocking correctness or security issues found. Both findings are P2 style improvements (dead ctx parameter, minor path-passing inconsistency). Core logic — multipart session parsing, sequential sequential chunk reads, upload routing threshold, rollback on failure — is correct and well-covered by new tests. shortcuts/drive/drive_import_common.go has a dead ctx parameter worth cleaning up before it confuses future readers. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI
participant DriveAPI
Note over CLI: fileSize > 20 MB
CLI->>DriveAPI: POST /medias/upload_prepare<br/>(file_name, parent_type, parent_node, size)
DriveAPI-->>CLI: upload_id, block_size, block_num
loop seq = 0 .. block_num-1
CLI->>DriveAPI: POST /medias/upload_part<br/>(upload_id, seq, size, file_chunk)
DriveAPI-->>CLI: 200 OK
end
CLI->>DriveAPI: POST /medias/upload_finish<br/>(upload_id, block_num)
DriveAPI-->>CLI: file_token
Note over CLI: fileSize ≤ 20 MB → single POST /medias/upload_all
Reviews (3): Last reviewed commit: "support multipart doc media uploads" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@769ab5249ba5bdc48b8f6329a414ba17a761f534🧩 Skill updatenpx skills add larksuite/cli#feat/doc-media-chunked-upload -y -g |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
shortcuts/common/drive_media_upload.go (1)
52-57:⚠️ Potential issue | 🟠 MajorValidate and preflight the file path inside the shared uploader.
UploadDriveMediaAlland the multipart path still touch raw file paths, and multipart does not surface path/open failures until afterupload_preparehas already created a remote session. Please safe-validate once inside the common helper and pass the sanitized/opened file into the part loop so callers cannot bypass the repo’s path guard. As per coding guidelines,**/*.go: Validate paths usingvalidate.SafeInputPathbefore any file I/O operations.Also applies to: 87-116, 170-175
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/common/drive_media_upload.go` around lines 52 - 57, UploadDriveMediaAll currently opens cfg.FilePath directly; validate and sanitize the path first using validate.SafeInputPath before any file I/O, then open the returned safe path and pass the opened file handle (not the raw path) into the multipart/part loop so path/open failures surface before upload_prepare creates a remote session. Update UploadDriveMediaAll to call validate.SafeInputPath(cfg.FilePath), handle/return validation errors, open the sanitized path, defer close, and refactor the multipart upload code to accept the io.Reader/File (the opened file) instead of re-opening the raw path so callers cannot bypass the repo path guard.
🤖 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/common/drive_media_upload.go`:
- Around line 177-182: session.BlockSize must be bounded by the actual local
file size before allocating buffer to avoid OOM: retrieve the local file size
(e.g., via file.Stat().Size() or existing localFileSize variable), compute
allowed := min(session.BlockSize, localFileSize - currentOffset) and also clamp
to the existing maxInt check, then allocate buffer using make([]byte,
int(allowed)) instead of make([]byte, int(session.BlockSize)); ensure allowed is
>= 1 and fall back to a safe default or return an error if it's invalid.
In `@shortcuts/doc/doc_media_insert.go`:
- Around line 82-83: The dry-run helper appendDocMediaInsertUploadDryRun is
creating an upload preview that doesn't include the same payload fields used in
real execution (uploadDocMediaFile attaches the document "extra" field and the
single-part path includes "size"), so update appendDocMediaInsertUploadDryRun to
include the document extra metadata and the size field in its generated request
body for both multipart and single-part flows so the preview matches what
uploadDocMediaFile actually sends; ensure the same keys/naming are used as in
uploadDocMediaFile and mirror any conditional logic for multipart vs single-part
inserts.
In `@shortcuts/doc/doc_media_upload.go`:
- Around line 43-75: The dry-run for single-part uploads is missing the "size"
field so it doesn't match the real UploadDriveMediaAll request; update the
single-part branch (the code that sets body["file"] = "@" + filePath and returns
dry.Desc("multipart/form-data upload")...) to include the same "size" key as the
real request (set body["size"] to the file size or placeholder "<file_size>" so
the dry run mirrors UploadDriveMediaAll), ensuring the dry-run Body payload keys
match those used when not using docMediaShouldUseMultipart.
---
Duplicate comments:
In `@shortcuts/common/drive_media_upload.go`:
- Around line 52-57: UploadDriveMediaAll currently opens cfg.FilePath directly;
validate and sanitize the path first using validate.SafeInputPath before any
file I/O, then open the returned safe path and pass the opened file handle (not
the raw path) into the multipart/part loop so path/open failures surface before
upload_prepare creates a remote session. Update UploadDriveMediaAll to call
validate.SafeInputPath(cfg.FilePath), handle/return validation errors, open the
sanitized path, defer close, and refactor the multipart upload code to accept
the io.Reader/File (the opened file) instead of re-opening the raw path so
callers cannot bypass the repo path guard.
🪄 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: 3a9326a7-daf4-491f-9402-3df64057b744
📒 Files selected for processing (6)
shortcuts/common/drive_media_upload.goshortcuts/doc/doc_media_insert.goshortcuts/doc/doc_media_test.goshortcuts/doc/doc_media_upload.goshortcuts/drive/drive_import.goshortcuts/drive/drive_import_common.go
💤 Files with no reviewable changes (1)
- shortcuts/drive/drive_import.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/drive/drive_import_common.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/doc/doc_media_test.go
Comment on lines
+177
to
+182
| maxInt := int64(^uint(0) >> 1) | ||
| if session.BlockSize > maxInt { | ||
| return output.Errorf(output.ExitAPI, "api_error", "upload prepare failed: invalid block_size returned") | ||
| } | ||
|
|
||
| buffer := make([]byte, int(session.BlockSize)) |
There was a problem hiding this comment.
Bound the chunk buffer before allocating it.
session.BlockSize comes from upload_prepare, so make([]byte, int(session.BlockSize)) can reserve far more memory than the local file actually needs. A bad backend response here can OOM the CLI on a small upload; cap the allocation to the local file size before allocating.
💡 Suggested change
maxInt := int64(^uint(0) >> 1)
if session.BlockSize > maxInt {
return output.Errorf(output.ExitAPI, "api_error", "upload prepare failed: invalid block_size returned")
}
- buffer := make([]byte, int(session.BlockSize))
+ bufferSize := session.BlockSize
+ if fileSize > 0 && bufferSize > fileSize {
+ bufferSize = fileSize
+ }
+ if bufferSize <= 0 || bufferSize > maxInt {
+ return output.Errorf(output.ExitAPI, "api_error", "upload prepare failed: invalid block_size returned")
+ }
+ buffer := make([]byte, int(bufferSize))
remaining := fileSize📝 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.
Suggested change
| maxInt := int64(^uint(0) >> 1) | |
| if session.BlockSize > maxInt { | |
| return output.Errorf(output.ExitAPI, "api_error", "upload prepare failed: invalid block_size returned") | |
| } | |
| buffer := make([]byte, int(session.BlockSize)) | |
| maxInt := int64(^uint(0) >> 1) | |
| if session.BlockSize > maxInt { | |
| return output.Errorf(output.ExitAPI, "api_error", "upload prepare failed: invalid block_size returned") | |
| } | |
| bufferSize := session.BlockSize | |
| if fileSize > 0 && bufferSize > fileSize { | |
| bufferSize = fileSize | |
| } | |
| if bufferSize <= 0 || bufferSize > maxInt { | |
| return output.Errorf(output.ExitAPI, "api_error", "upload prepare failed: invalid block_size returned") | |
| } | |
| buffer := make([]byte, int(bufferSize)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/common/drive_media_upload.go` around lines 177 - 182,
session.BlockSize must be bounded by the actual local file size before
allocating buffer to avoid OOM: retrieve the local file size (e.g., via
file.Stat().Size() or existing localFileSize variable), compute allowed :=
min(session.BlockSize, localFileSize - currentOffset) and also clamp to the
existing maxInt check, then allocate buffer using make([]byte, int(allowed))
instead of make([]byte, int(session.BlockSize)); ensure allowed is >= 1 and fall
back to a safe default or return an error if it's invalid.
Comment on lines
+43
to
75
| dry := common.NewDryRunAPI() | ||
| if docMediaShouldUseMultipart(filePath) { | ||
| prepareBody := map[string]interface{}{ | ||
| "file_name": filepath.Base(filePath), | ||
| "parent_type": parentType, | ||
| "parent_node": parentNode, | ||
| "size": "<file_size>", | ||
| } | ||
| if extra, ok := body["extra"]; ok { | ||
| prepareBody["extra"] = extra | ||
| } | ||
| dry.Desc("chunked media upload (files > 20MB)"). | ||
| POST("/open-apis/drive/v1/medias/upload_prepare"). | ||
| Body(prepareBody). | ||
| POST("/open-apis/drive/v1/medias/upload_part"). | ||
| Body(map[string]interface{}{ | ||
| "upload_id": "<upload_id>", | ||
| "seq": "<chunk_index>", | ||
| "size": "<chunk_size>", | ||
| "file": "<chunk_binary>", | ||
| }). | ||
| POST("/open-apis/drive/v1/medias/upload_finish"). | ||
| Body(map[string]interface{}{ | ||
| "upload_id": "<upload_id>", | ||
| "block_num": "<block_num>", | ||
| }) | ||
| return dry | ||
| } | ||
|
|
||
| body["file"] = "@" + filePath | ||
| return dry.Desc("multipart/form-data upload"). | ||
| POST("/open-apis/drive/v1/medias/upload_all"). | ||
| Body(body) |
There was a problem hiding this comment.
Keep the upload_all dry-run body in sync with the real request.
The single-part preview omits the size field, but UploadDriveMediaAll always sends it. That makes the dry-run plan incomplete for small files.
📎 Suggested change
body := map[string]interface{}{
"file_name": filepath.Base(filePath),
"parent_type": parentType,
"parent_node": parentNode,
+ "size": "<file_size>",
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/doc/doc_media_upload.go` around lines 43 - 75, The dry-run for
single-part uploads is missing the "size" field so it doesn't match the real
UploadDriveMediaAll request; update the single-part branch (the code that sets
body["file"] = "@" + filePath and returns dry.Desc("multipart/form-data
upload")...) to include the same "size" key as the real request (set
body["size"] to the file size or placeholder "<file_size>" so the dry run
mirrors UploadDriveMediaAll), ensuring the dry-run Body payload keys match those
used when not using docMediaShouldUseMultipart.
Change-Id: I9d9fb00079dacfc96b5781e12e6ce79945baa2ed
liujinkun2025
force-pushed
the
feat/doc-media-chunked-upload
branch
from
2f072cf to
769ab52
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/drive/drive_upload.go (1)
184-184:⚠️ Potential issue | 🟡 MinorUse
vfs.Openinstead ofos.Openfor filesystem access.The multipart upload loop uses
os.Openwhile other file operations in this file correctly usevfs.Open(line 99) andvfs.Stat(line 70). As per coding guidelines, all filesystem access should usevfs.*for consistency and testability.🔧 Proposed fix
- partFile, err := os.Open(filePath) + partFile, err := vfs.Open(filePath)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/drive/drive_upload.go` at line 184, Replace the direct os.Open call with the VFS wrapper: open the part file using vfs.Open(filePath) instead of os.Open(filePath) (the variable partFile and error handling usage should remain the same), ensure the import set is updated (remove unused "os" if no longer needed), and keep the existing defer/Close logic unchanged so the multipart upload loop continues to use vfs for filesystem access and remains testable; update any error messages referencing os.Open to reflect vfs.Open if present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@shortcuts/drive/drive_upload.go`:
- Line 184: Replace the direct os.Open call with the VFS wrapper: open the part
file using vfs.Open(filePath) instead of os.Open(filePath) (the variable
partFile and error handling usage should remain the same), ensure the import set
is updated (remove unused "os" if no longer needed), and keep the existing
defer/Close logic unchanged so the multipart upload loop continues to use vfs
for filesystem access and remains testable; update any error messages
referencing os.Open to reflect vfs.Open if present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 518afbed-666a-4a55-9816-32a852516b52
📒 Files selected for processing (16)
shortcuts/common/drive_media_upload.goshortcuts/common/drive_media_upload_test.goshortcuts/doc/doc_media_insert.goshortcuts/doc/doc_media_test.goshortcuts/doc/doc_media_upload.goshortcuts/drive/drive_export_test.goshortcuts/drive/drive_import.goshortcuts/drive/drive_import_common.goshortcuts/drive/drive_import_common_test.goshortcuts/drive/drive_import_test.goshortcuts/drive/drive_io_test.goshortcuts/drive/drive_move_common_test.goshortcuts/drive/drive_move_test.goshortcuts/drive/drive_task_result_test.goshortcuts/drive/drive_upload.goskills/lark-doc/references/lark-doc-media-insert.md
💤 Files with no reviewable changes (5)
- shortcuts/drive/drive_task_result_test.go
- shortcuts/drive/drive_move_test.go
- shortcuts/drive/drive_export_test.go
- shortcuts/drive/drive_import_common_test.go
- shortcuts/drive/drive_move_common_test.go
✅ Files skipped from review due to trivial changes (2)
- shortcuts/drive/drive_import_test.go
- skills/lark-doc/references/lark-doc-media-insert.md
🚧 Files skipped from review as they are similar to previous changes (4)
- shortcuts/common/drive_media_upload_test.go
- shortcuts/doc/doc_media_upload.go
- shortcuts/doc/doc_media_test.go
- shortcuts/common/drive_media_upload.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add multipart upload support for doc media flows so
docs +media-uploadanddocs +media-insertcan handle files larger than 20MB. This also extracts the shared Drive media upload logic into a common helper and keeps the user-facingdocs +media- insertdry-run flow expressed as 4/5 high-level steps.Changes
upload_alland multipartupload_prepare -> upload_part -> upload_finishdocs +media-uploadto auto-switch to multipart upload for files larger than 20MBdocs +media-insertto auto-switch to multipart upload, keep rollback behavior, and keep dry-run output user-orientedshortcuts/commonlark-doc-media-insertreference to reflect automatic multipart upload for files larger than 20MBTest Plan
make unit-test)lark-cli docs +media-insertworks as expected for both a normal image upload and a file upload larger than 20MBRelated Issues
Summary by CodeRabbit
New Features
Documentation
Tests
Chores