Improve drive batch failure handling - #1703
Merged
Merged
Conversation
github-actions
Bot
added
domain/ccm
|
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 structured Drive failure metadata to push, pull, and sync item payloads, expands Drive code metadata mappings, adds terminal-failure abort handling, and updates tests to assert structured partial-failure output. ChangesDrive batch failure classification
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant drivePushUploadFile
participant drivePushFailedItem
participant driveClassifyBatchFailure
participant summary
drivePushUploadFile->>drivePushFailedItem: record upload/delete failure
drivePushFailedItem->>driveClassifyBatchFailure: classify(err)
driveClassifyBatchFailure-->>drivePushFailedItem: Class, Code, Subtype, Retryable, Terminal
drivePushFailedItem-->>drivePushUploadFile: structured drivePushItem
drivePushUploadFile->>drivePushUploadFile: break on terminal failure
drivePushUploadFile->>summary: aborted = hasTerminalFailure(items)
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1703 +/- ##
==========================================
- Coverage 74.48% 74.48% -0.01%
==========================================
Files 850 850
Lines 86703 86878 +175
==========================================
+ Hits 64583 64709 +126
- Misses 17175 17210 +35
- Partials 4945 4959 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@67f75c4f2d74ea31dc212b8166ce45aa1c38e449🧩 Skill updatenpx skills add larksuite/cli#codex/drive-push-p0-p1-reliability -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/drive/drive_push_test.go`:
- Around line 987-1048: The new drive push error tests in
TestDrivePushAbortsAfterUploadParamsError and the related upload failure cases
are asserting metadata by scanning raw stdout strings, which is brittle and
doesn’t prove the values are attached to the intended JSON fields. Update these
tests to parse stdout as JSON and assert the structured payload fields directly
(for example the item error metadata like error_class, code, subtype, aborted,
and phase), and use errs.ProblemOf for typed error metadata/cause checks instead
of message substring matching alone.
In `@shortcuts/drive/drive_push.go`:
- Line 12: The snapshot verification in drive_push.go is using direct os.Stat
access instead of the shortcut filesystem abstraction. Update the verifier and
related call sites to accept runtime and use runtime.FileIO().Stat on
file.OpenPath rather than file.AbsPath, so user-facing paths continue to flow
through the VFS/mocking layer. Keep the fix localized around the snapshot
verification helpers and any places that currently import os only for Stat.
- Line 43: The structured failure payload in the drive push flow is dropping
Retryable when it is false because the DriveItemFailure JSON tag uses omitempty.
Update the failure serialization in drive_push.go, including the related failure
items around the referenced block, so Retryable is always emitted and preserves
retryable:false for non-retryable failures. Keep the change focused on the
DriveItemFailure type and any constructors/emitters that build the failed item
JSON.
- Around line 407-408: The delete failure handling in drivePush currently
ignores the terminal flag returned by drivePushFailedItem, so non-retryable
auth/permission errors keep retrying delete calls while the summary still shows
aborted. Update the delete-error path in drivePush to capture and respect the
terminal value from drivePushFailedItem, and stop further delete attempts when
it indicates a terminal failure. Apply the same fix in both delete failure
branches referenced by drivePushFailedItem so terminal delete failures are
honored consistently.
🪄 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: 9cb6aa6e-769f-4757-9477-936d20b942e0
📒 Files selected for processing (4)
internal/errclass/codemeta_drive.gointernal/errclass/codemeta_test.goshortcuts/drive/drive_push.goshortcuts/drive/drive_push_test.go
github-actions
Bot
added
size/L
fangshuyu-768
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
internal/client/client.go (1)
247-277: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd test coverage for the HTTP 403 + unclassifiable-JSON fallback branch.
Lines 268-274 (the
status == http.StatusForbiddenfallback) only fire when the body parses as valid, non-empty JSON butBuildAPIErrorcan't classify it (missing/zerocode). Neither existing test exercises this:TestDoStream_HTTPErrorIncludesLogIDuses a non-JSON body (so classification bails out before reaching this branch), and the newTestDoStream_JSONHTTPErrorIsClassifieduses a classifiable code. This new branch is currently untested.As per coding guidelines, "Every behavior change needs a test alongside the change."
✅ Suggested additional test case
func TestDoStream_403UnclassifiedJSONFallsBackToPermissionDenied(t *testing.T) { t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) config := &core.CliConfig{AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu} factory, _, _, reg := cmdutil.TestFactory(t, config) reg.Register(&httpmock.Stub{ Method: http.MethodGet, URL: "/open-apis/drive/v1/files/file_token/download", Status: http.StatusForbidden, RawBody: []byte(`{"error":"forbidden"}`), }) client, err := factory.NewAPIClientWithConfig(config) if err != nil { t.Fatalf("NewAPIClientWithConfig() error = %v", err) } _, err = client.DoStream(context.Background(), &larkcore.ApiReq{ HttpMethod: http.MethodGet, ApiPath: "/open-apis/drive/v1/files/file_token/download", }, core.AsBot) var permErr *errs.PermissionError if !errors.As(err, &permErr) { t.Fatalf("expected *errs.PermissionError, got %T %v", err, err) } if permErr.Subtype != errs.SubtypePermissionDenied { t.Fatalf("Subtype = %q, want %q", permErr.Subtype, errs.SubtypePermissionDenied) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/client.go` around lines 247 - 277, Add a test for the unclassified JSON HTTP 403 fallback in classifyStreamAPIError. The current coverage misses the branch where rawBody is valid non-empty JSON, BuildAPIError returns nil, and status == http.StatusForbidden, so add a DoStream test in the client API suite that returns a 403 with a JSON body lacking a classifiable code and assert it falls back to errs.PermissionError with SubtypePermissionDenied. Use the existing helpers and symbols DoStream, classifyStreamAPIError, and errs.PermissionError so the new case is easy to locate and maintain.Source: Coding guidelines
internal/client/dostream_test.go (1)
53-93: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert
Categoryalongside Subtype/Code/Retryable/LogID.The test checks
apiErr.Code,.Subtype,.Retryable,.LogIDbut not.Category.As per path instructions for
**/*_test.go, "Error-path tests must assert typed metadata viaerrs.ProblemOf(category/subtype/param) and cause preservation, not message substrings alone."✅ Suggested addition
if apiErr.Subtype != errs.SubtypeRateLimit { t.Fatalf("Subtype = %q, want %q", apiErr.Subtype, errs.SubtypeRateLimit) } + if apiErr.Category != errs.CategoryAPI { + t.Fatalf("Category = %q, want %q", apiErr.Category, errs.CategoryAPI) + } if !apiErr.Retryable {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/client/dostream_test.go` around lines 53 - 93, The test for DoStream stream error classification is missing an assertion for the error category metadata. Update TestDoStream_JSONHTTPErrorIsClassified to verify apiErr.Category in addition to Code, Subtype, Retryable, and LogID, and keep using the typed APIError returned from client.DoStream so the error-path test validates the structured metadata expected by errs.ProblemOf.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/drive/drive_pull.go`:
- Line 49: The Drive pull result serialization is omitting retryable=false
because the bool field uses omitempty; update the result type in drive_pull.go
to use a pointer for Retryable and set it only on failed item payloads so
successful items still omit the field. Adjust the affected constructors/handlers
around the Drive pull flow so non-retryable failures serialize retryable:false
while retryable/unspecified cases remain absent.
- Around line 284-285: The delete fallback in drivePull currently passes the raw
os.Remove error into drivePullFailedItem, which drops the file_io subtype and
cause metadata. Wrap the remove failure as a typed internal file I/O error
before classification, using errs.NewInternalError with the file I/O subtype and
chaining the original error with .WithCause(err), then pass that wrapped error
into drivePullFailedItem in drivePull.
In `@shortcuts/drive/drive_sync.go`:
- Line 38: The drive sync failure payload currently omits retryable when it is
false because driveSyncItem.Retryable uses omitempty on a plain bool. Update
driveSyncItem and driveSyncFailedItem in drive_sync.go to represent Retryable as
a pointer and always set it for failed items so false is serialized explicitly,
then adjust any tests/helpers that read or decode driveSyncItem.Retryable to
handle the pointer-based field consistently.
- Around line 513-516: The rollback path in driveSync’s download handling is
overwriting the original drivePullDownload error with a new file_io error, which
can downgrade the error classification and corrupt retryable/terminal metadata.
In the rollback-failure branch, preserve the original download error value and
only record or attach the rollback failure separately, so driveSyncFailedItem
still receives the typed error returned by the lower layer. Use the existing
driveSyncFailedItem and rollbackErr handling in drive_sync.go to keep the
original err unchanged when rollback also fails.
---
Nitpick comments:
In `@internal/client/client.go`:
- Around line 247-277: Add a test for the unclassified JSON HTTP 403 fallback in
classifyStreamAPIError. The current coverage misses the branch where rawBody is
valid non-empty JSON, BuildAPIError returns nil, and status ==
http.StatusForbidden, so add a DoStream test in the client API suite that
returns a 403 with a JSON body lacking a classifiable code and assert it falls
back to errs.PermissionError with SubtypePermissionDenied. Use the existing
helpers and symbols DoStream, classifyStreamAPIError, and errs.PermissionError
so the new case is easy to locate and maintain.
In `@internal/client/dostream_test.go`:
- Around line 53-93: The test for DoStream stream error classification is
missing an assertion for the error category metadata. Update
TestDoStream_JSONHTTPErrorIsClassified to verify apiErr.Category in addition to
Code, Subtype, Retryable, and LogID, and keep using the typed APIError returned
from client.DoStream so the error-path test validates the structured metadata
expected by errs.ProblemOf.
🪄 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: db5297f2-c8e8-40c3-a88f-9bb3fd55eb25
📒 Files selected for processing (7)
internal/client/client.gointernal/client/dostream_test.goshortcuts/drive/drive_pull.goshortcuts/drive/drive_pull_test.goshortcuts/drive/drive_push.goshortcuts/drive/drive_sync.goshortcuts/drive/drive_sync_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- shortcuts/drive/drive_push.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/drive/drive_push_test.go`:
- Around line 1273-1278: The snapshot-error test in the drive push path only
checks the subtype, so it does not fully validate the typed error contract.
Update the test around ProblemOf/problemErr to also assert the expected
category, and verify cause preservation by checking that
errors.Unwrap(problemErr) is non-nil; keep using errs.ProblemOf to inspect the
typed metadata instead of relying on message text.
In `@shortcuts/drive/drive_push.go`:
- Around line 787-788: The snapshot verification in drivePushLocalFile only
compares size, so a same-size edit can slip through; update the push guard in
drive_push.go to also compare the captured ModTime from drivePushLocalFile
against the current file info before uploading. If either size or ModTime
differs, return the same failed-precondition validation error from the existing
local file change check. Add a regression test around the push/snapshot
verification path to cover a same-size mutation being rejected.
🪄 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: bb15a6c6-ed7f-4f6b-96c4-5e501b26a0e4
📒 Files selected for processing (5)
shortcuts/drive/drive_pull.goshortcuts/drive/drive_push.goshortcuts/drive/drive_push_test.goshortcuts/drive/drive_sync.goshortcuts/drive/drive_sync_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/drive/drive_pull.go
- shortcuts/drive/drive_sync.go
caojie0621
approved these changes
Jul 1, 2026
liangshuo-1
approved these changes
Jul 1, 2026
Merged
This was referenced Jul 2, 2026
7 tasks
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
3 participants
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
drive +push,drive +pull, anddrive +syncphase/error_class/code/subtype/retryablefields for push, pull, and sync partial failuresdrive +syncaction scopes (drive:file:download,drive:file:upload,space:folder:create) before listing/walking so missing grants surface as the standardmissing_scopepermission error before any partial sync can startTests
go test ./internal/client ./internal/errclass ./shortcuts/drivego build ./...go test ./...was also attempted and failed in unrelated environment/live/e2e areas: schema/manifest catalog fixtures missing services, temp git cleanup in publiccontent, missing local./lark-clifor e2e dry-runs, and live auth/timeout failures in calendar/im/task tests.Summary by CodeRabbit