diff --git a/internal/errclass/codemeta_drive.go b/internal/errclass/codemeta_drive.go index fe231633b..9722d7e42 100644 --- a/internal/errclass/codemeta_drive.go +++ b/internal/errclass/codemeta_drive.go @@ -10,8 +10,15 @@ import "github.com/larksuite/cli/errs" // ambiguous codes fall back to CategoryAPI via BuildAPIError. // BuildAPIError consumes this map via mergeCodeMeta + LookupCodeMeta. var driveCodeMeta = map[int]CodeMeta{ - 1061044: {Category: errs.CategoryAPI, Subtype: errs.SubtypeNotFound}, // parent folder does not exist (upload) - 1069302: {Category: errs.CategoryAPI, Subtype: errs.SubtypeInvalidParameters}, // comment endpoint "Invalid or missing parameters" + 1061001: {Category: errs.CategoryAPI, Subtype: errs.SubtypeServerError, Retryable: true}, // Drive "unknown error" + 1061002: {Category: errs.CategoryAPI, Subtype: errs.SubtypeInvalidParameters}, // params error + 1061004: {Category: errs.CategoryAuthorization, Subtype: errs.SubtypePermissionDenied}, // forbidden + 1061007: {Category: errs.CategoryAPI, Subtype: errs.SubtypeNotFound}, // file has been deleted + 1061043: {Category: errs.CategoryAPI, Subtype: errs.SubtypeQuotaExceeded}, // file size beyond limit + 1061044: {Category: errs.CategoryAPI, Subtype: errs.SubtypeNotFound}, // parent folder does not exist (upload) + 1062009: {Category: errs.CategoryAPI, Subtype: errs.SubtypeInvalidParameters}, // actual size inconsistent with declared size + 1069302: {Category: errs.CategoryAPI, Subtype: errs.SubtypeInvalidParameters}, // comment endpoint "Invalid or missing parameters" + 2200: {Category: errs.CategoryAPI, Subtype: errs.SubtypeServerError, Retryable: true}, // Drive tenant/internal errors } func init() { mergeCodeMeta(driveCodeMeta, "drive") } diff --git a/internal/errclass/codemeta_test.go b/internal/errclass/codemeta_test.go index bde3d0887..521850e65 100644 --- a/internal/errclass/codemeta_test.go +++ b/internal/errclass/codemeta_test.go @@ -102,6 +102,35 @@ func TestLookupCodeMeta_RetryableRateLimit(t *testing.T) { } } +func TestLookupCodeMeta_DrivePushCodes(t *testing.T) { + cases := []struct { + code int + wantCat errs.Category + wantSubtype errs.Subtype + wantRetry bool + }{ + {1061001, errs.CategoryAPI, errs.SubtypeServerError, true}, + {1061002, errs.CategoryAPI, errs.SubtypeInvalidParameters, false}, + {1061004, errs.CategoryAuthorization, errs.SubtypePermissionDenied, false}, + {1061007, errs.CategoryAPI, errs.SubtypeNotFound, false}, + {1061043, errs.CategoryAPI, errs.SubtypeQuotaExceeded, false}, + {1062009, errs.CategoryAPI, errs.SubtypeInvalidParameters, false}, + {2200, errs.CategoryAPI, errs.SubtypeServerError, true}, + } + for _, tc := range cases { + t.Run(fmt.Sprintf("%d", tc.code), func(t *testing.T) { + got, ok := LookupCodeMeta(tc.code) + if !ok { + t.Fatalf("LookupCodeMeta(%d) ok=false, want true", tc.code) + } + if got.Category != tc.wantCat || got.Subtype != tc.wantSubtype || got.Retryable != tc.wantRetry { + t.Fatalf("LookupCodeMeta(%d) = %+v, want Category=%v Subtype=%v Retryable=%v", + tc.code, got, tc.wantCat, tc.wantSubtype, tc.wantRetry) + } + }) + } +} + func TestLookupCodeMeta_Unknown(t *testing.T) { _, ok := LookupCodeMeta(999999) if ok { diff --git a/shortcuts/drive/drive_pull.go b/shortcuts/drive/drive_pull.go index 3fa27eda2..bcdce7612 100644 --- a/shortcuts/drive/drive_pull.go +++ b/shortcuts/drive/drive_pull.go @@ -37,11 +37,16 @@ const ( ) type drivePullItem struct { - RelPath string `json:"rel_path"` - FileToken string `json:"file_token,omitempty"` - SourceID string `json:"source_id,omitempty"` - Action string `json:"action"` - Error string `json:"error,omitempty"` + RelPath string `json:"rel_path"` + FileToken string `json:"file_token,omitempty"` + SourceID string `json:"source_id,omitempty"` + Action string `json:"action"` + Error string `json:"error,omitempty"` + Phase string `json:"phase,omitempty"` + ErrorClass string `json:"error_class,omitempty"` + Code int `json:"code,omitempty"` + Subtype string `json:"subtype,omitempty"` + Retryable *bool `json:"retryable,omitempty"` } type drivePullTarget struct { @@ -189,6 +194,9 @@ var DrivePull = common.Shortcut{ sort.Strings(downloadablePaths) for _, rel := range downloadablePaths { + if drivePullHasTerminalFailure(items) { + break + } targetFile := remoteFiles[rel] downloadToken := targetFile.DownloadToken itemFileToken := targetFile.ItemFileToken @@ -204,13 +212,9 @@ var DrivePull = common.Shortcut{ // pre-existing file under --if-exists=skip silently // hides the conflict. Surface as a failure. if info.IsDir() { - items = append(items, drivePullItem{ - RelPath: rel, - FileToken: itemFileToken, - SourceID: itemSourceID, - Action: "failed", - Error: fmt.Sprintf("local path is a directory, remote is a regular file: %s", target), - }) + conflictErr := errs.NewValidationError(errs.SubtypeFailedPrecondition, "local path is a directory, remote is a regular file: %s", target) + item, _ := drivePullFailedItem(rel, itemFileToken, itemSourceID, "failed", "local", conflictErr) + items = append(items, item) failed++ downloadFailed++ continue @@ -223,9 +227,14 @@ var DrivePull = common.Shortcut{ } if err := drivePullDownload(ctx, runtime, downloadToken, target, targetFile.ModifiedTime); err != nil { - items = append(items, drivePullItem{RelPath: rel, FileToken: itemFileToken, SourceID: itemSourceID, Action: "failed", Error: err.Error()}) + item, terminal := drivePullFailedItem(rel, itemFileToken, itemSourceID, "failed", "download", err) + items = append(items, item) failed++ downloadFailed++ + if terminal { + fmt.Fprintf(runtime.IO().ErrOut, "Aborting +pull after terminal %s failure: %v\n", item.Phase, err) + break + } continue } items = append(items, drivePullItem{RelPath: rel, FileToken: itemFileToken, SourceID: itemSourceID, Action: "downloaded"}) @@ -251,7 +260,8 @@ var DrivePull = common.Shortcut{ for _, absPath := range localAbsPaths { rel, relErr := filepath.Rel(safeRoot, absPath) if relErr != nil { - items = append(items, drivePullItem{RelPath: absPath, Action: "delete_failed", Error: relErr.Error()}) + item, _ := drivePullFailedItem(absPath, "", "", "delete_failed", "delete_local", relErr) + items = append(items, item) failed++ continue } @@ -271,7 +281,9 @@ var DrivePull = common.Shortcut{ // acceptable here. Shortcuts cannot import internal/vfs // directly (depguard rule shortcuts-no-vfs). if err := os.Remove(absPath); err != nil { //nolint:forbidigo // see comment above - items = append(items, drivePullItem{RelPath: rel, Action: "delete_failed", Error: err.Error()}) + deleteErr := errs.NewInternalError(errs.SubtypeFileIO, "delete local %q: %s", rel, err).WithCause(err) + item, _ := drivePullFailedItem(rel, "", "", "delete_failed", "delete_local", deleteErr) + items = append(items, item) failed++ continue } @@ -286,6 +298,7 @@ var DrivePull = common.Shortcut{ "skipped": skipped, "failed": failed, "deleted_local": deletedLocal, + "aborted": drivePullHasTerminalFailure(items), }, "items": items, } @@ -317,6 +330,32 @@ var DrivePull = common.Shortcut{ }, } +func drivePullFailedItem(relPath, fileToken, sourceID, action, phase string, err error) (drivePullItem, bool) { + decision := driveClassifyBatchFailure(err) + item := drivePullItem{ + RelPath: relPath, + FileToken: fileToken, + SourceID: sourceID, + Action: action, + Error: err.Error(), + Phase: phase, + ErrorClass: decision.Class, + Code: decision.Code, + Subtype: decision.Subtype, + Retryable: driveBoolPtr(decision.Retryable), + } + return item, decision.Terminal +} + +func drivePullHasTerminalFailure(items []drivePullItem) bool { + for _, item := range items { + if driveTerminalBatchErrorClass(item.ErrorClass) { + return true + } + } + return false +} + // drivePullDownload streams one Drive file into the local mirror target and // then best-effort aligns the local mtime to Drive's modified_time. func drivePullDownload(ctx context.Context, runtime *common.RuntimeContext, fileToken, target, remoteModifiedTime string) error { diff --git a/shortcuts/drive/drive_pull_test.go b/shortcuts/drive/drive_pull_test.go index 5a6d6caa3..5502291b0 100644 --- a/shortcuts/drive/drive_pull_test.go +++ b/shortcuts/drive/drive_pull_test.go @@ -1032,6 +1032,66 @@ func TestDrivePullDownloadFailureSkipsDeleteLocalAndExitsNonZero(t *testing.T) { } } +func TestDrivePullAbortsAfterDownloadForbidden(t *testing.T) { + f, stdout, _, reg := cmdutil.TestFactory(t, driveTestConfig()) + + tmpDir := t.TempDir() + withDriveWorkingDir(t, tmpDir) + if err := os.MkdirAll("local", 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "folder_token=folder_root", + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "files": []interface{}{ + map[string]interface{}{"token": "tok_a", "name": "a.txt", "type": "file"}, + map[string]interface{}{"token": "tok_b", "name": "b.txt", "type": "file"}, + }, + "has_more": false, + }, + }, + }) + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "/open-apis/drive/v1/files/tok_a/download", + Status: http.StatusForbidden, + RawBody: []byte("forbidden"), + }) + + err := mountAndRunDrive(t, DrivePull, []string{ + "+pull", + "--local-dir", "local", + "--folder-token", "folder_root", + "--as", "bot", + }, f, stdout) + assertDrivePullPartialFailure(t, err) + + summary, items := splitDrivePullStdout(t, stdout.Bytes()) + if got := summary["aborted"]; got != true { + t.Fatalf("summary.aborted = %v, want true", got) + } + if got := summary["failed"]; got != float64(1) { + t.Fatalf("summary.failed = %v, want 1", got) + } + if len(items) != 1 { + t.Fatalf("items len = %d, want 1; items=%#v", len(items), items) + } + item := items[0] + if item["rel_path"] != "a.txt" || item["phase"] != "download" || item["error_class"] != "permission_denied" { + t.Fatalf("unexpected failed item: %#v", item) + } + if item["code"] != float64(http.StatusForbidden) || item["retryable"] != false { + t.Fatalf("unexpected failure classification: %#v", item) + } + if _, statErr := os.Stat(filepath.Join("local", "b.txt")); !os.IsNotExist(statErr) { + t.Fatalf("b.txt should not be downloaded after terminal permission failure; stat err=%v", statErr) + } +} + // TestDrivePullDeleteLocalDoesNotEscapeViaSymlinkParentRef is the // regression for the "link/.." escape applied to --delete-local — the // most dangerous variant, since the bug would otherwise let the kernel diff --git a/shortcuts/drive/drive_push.go b/shortcuts/drive/drive_push.go index 351ef5601..e0ff5c122 100644 --- a/shortcuts/drive/drive_push.go +++ b/shortcuts/drive/drive_push.go @@ -29,12 +29,25 @@ const ( ) type drivePushItem struct { - RelPath string `json:"rel_path"` - FileToken string `json:"file_token,omitempty"` - Action string `json:"action"` - Version string `json:"version,omitempty"` - SizeBytes int64 `json:"size_bytes,omitempty"` - Error string `json:"error,omitempty"` + RelPath string `json:"rel_path"` + FileToken string `json:"file_token,omitempty"` + Action string `json:"action"` + Version string `json:"version,omitempty"` + SizeBytes int64 `json:"size_bytes,omitempty"` + Error string `json:"error,omitempty"` + Phase string `json:"phase,omitempty"` + ErrorClass string `json:"error_class,omitempty"` + Code int `json:"code,omitempty"` + Subtype string `json:"subtype,omitempty"` + Retryable *bool `json:"retryable,omitempty"` +} + +type driveBatchFailureDecision struct { + Class string + Code int + Subtype string + Retryable bool + Terminal bool } // DrivePush is a one-way, file-level mirror from a local directory onto a @@ -248,9 +261,14 @@ var DrivePush = common.Shortcut{ continue } if _, ensureErr := drivePushEnsureFolder(ctx, runtime, folderToken, relDir, folderCache); ensureErr != nil { - items = append(items, drivePushItem{RelPath: relDir, Action: "failed", Error: ensureErr.Error()}) + item, terminal := drivePushFailedItem(relDir, "", "failed", "create_folder", 0, ensureErr) + items = append(items, item) failed++ uploadFailed = true + if terminal { + fmt.Fprintf(runtime.IO().ErrOut, "Aborting +push after terminal %s failure: %v\n", item.Phase, ensureErr) + break + } continue } items = append(items, drivePushItem{RelPath: relDir, FileToken: folderCache[relDir], Action: "folder_created"}) @@ -266,6 +284,9 @@ var DrivePush = common.Shortcut{ for _, rel := range localPaths { localFile := localFiles[rel] + if uploadFailed && drivePushHasTerminalFailure(items) { + break + } if entry, ok := remoteFiles[rel]; ok { if drivePushShouldSkipExisting(localFile, entry, ifExists) { @@ -275,9 +296,14 @@ var DrivePush = common.Shortcut{ } parentToken, parentErr := drivePushEnsureParentToken(ctx, runtime, folderToken, rel, folderCache) if parentErr != nil { - items = append(items, drivePushItem{RelPath: rel, FileToken: entry.FileToken, Action: "failed", SizeBytes: localFile.Size, Error: parentErr.Error()}) + item, terminal := drivePushFailedItem(rel, entry.FileToken, "failed", "create_folder", localFile.Size, parentErr) + items = append(items, item) failed++ uploadFailed = true + if terminal { + fmt.Fprintf(runtime.IO().ErrOut, "Aborting +push after terminal %s failure: %v\n", item.Phase, parentErr) + break + } continue } token, version, upErr := drivePushUploadFile(ctx, runtime, localFile, entry.FileToken, parentToken) @@ -301,9 +327,14 @@ var DrivePush = common.Shortcut{ if failedToken == "" { failedToken = entry.FileToken } - items = append(items, drivePushItem{RelPath: rel, FileToken: failedToken, Action: "failed", SizeBytes: localFile.Size, Error: upErr.Error()}) + item, terminal := drivePushFailedItem(rel, failedToken, "failed", "upload", localFile.Size, upErr) + items = append(items, item) failed++ uploadFailed = true + if terminal { + fmt.Fprintf(runtime.IO().ErrOut, "Aborting +push after terminal %s failure: %v\n", item.Phase, upErr) + break + } continue } items = append(items, drivePushItem{RelPath: rel, FileToken: token, Action: "overwritten", Version: version, SizeBytes: localFile.Size}) @@ -314,16 +345,26 @@ var DrivePush = common.Shortcut{ parentRel := drivePushParentRel(rel) parentToken, ensureErr := drivePushEnsureFolder(ctx, runtime, folderToken, parentRel, folderCache) if ensureErr != nil { - items = append(items, drivePushItem{RelPath: rel, Action: "failed", SizeBytes: localFile.Size, Error: ensureErr.Error()}) + item, terminal := drivePushFailedItem(rel, "", "failed", "create_folder", localFile.Size, ensureErr) + items = append(items, item) failed++ uploadFailed = true + if terminal { + fmt.Fprintf(runtime.IO().ErrOut, "Aborting +push after terminal %s failure: %v\n", item.Phase, ensureErr) + break + } continue } token, _, upErr := drivePushUploadFile(ctx, runtime, localFile, "", parentToken) if upErr != nil { - items = append(items, drivePushItem{RelPath: rel, Action: "failed", SizeBytes: localFile.Size, Error: upErr.Error()}) + item, terminal := drivePushFailedItem(rel, "", "failed", "upload", localFile.Size, upErr) + items = append(items, item) failed++ uploadFailed = true + if terminal { + fmt.Fprintf(runtime.IO().ErrOut, "Aborting +push after terminal %s failure: %v\n", item.Phase, upErr) + break + } continue } items = append(items, drivePushItem{RelPath: rel, FileToken: token, Action: "uploaded", SizeBytes: localFile.Size}) @@ -350,7 +391,11 @@ var DrivePush = common.Shortcut{ } sort.Strings(remoteRelPaths) + abortDelete := false for _, rel := range remoteRelPaths { + if abortDelete { + break + } keepToken := "" if _, ok := localFiles[rel]; ok { if chosen, ok := remoteFiles[rel]; ok { @@ -362,8 +407,14 @@ var DrivePush = common.Shortcut{ continue } if err := drivePushDeleteFile(ctx, runtime, entry.FileToken); err != nil { - items = append(items, drivePushItem{RelPath: rel, FileToken: entry.FileToken, Action: "delete_failed", Error: err.Error()}) + item, terminal := drivePushFailedItem(rel, entry.FileToken, "delete_failed", "delete", 0, err) + items = append(items, item) failed++ + if terminal { + fmt.Fprintf(runtime.IO().ErrOut, "Aborting +push after terminal %s failure: %v\n", item.Phase, err) + abortDelete = true + break + } continue } items = append(items, drivePushItem{RelPath: rel, FileToken: entry.FileToken, Action: "deleted_remote"}) @@ -378,6 +429,7 @@ var DrivePush = common.Shortcut{ "skipped": skipped, "failed": failed, "deleted_remote": deletedRemote, + "aborted": drivePushHasTerminalFailure(items), }, "items": items, } @@ -507,6 +559,91 @@ func drivePushShouldSkipSmart(localFile drivePushLocalFile, remoteFile driveRemo return cmp >= 0 } +func drivePushFailedItem(relPath, fileToken, action, phase string, sizeBytes int64, err error) (drivePushItem, bool) { + decision := driveClassifyBatchFailure(err) + item := drivePushItem{ + RelPath: relPath, + FileToken: fileToken, + Action: action, + SizeBytes: sizeBytes, + Error: err.Error(), + Phase: phase, + ErrorClass: decision.Class, + Code: decision.Code, + Subtype: decision.Subtype, + Retryable: driveBoolPtr(decision.Retryable), + } + return item, decision.Terminal +} + +func driveBoolPtr(v bool) *bool { + return &v +} + +func driveClassifyBatchFailure(err error) driveBatchFailureDecision { + decision := driveBatchFailureDecision{Class: "unknown", Retryable: errs.IsRetryable(err)} + problem, ok := errs.ProblemOf(err) + if !ok { + return decision + } + decision.Code = problem.Code + decision.Subtype = string(problem.Subtype) + decision.Retryable = problem.Retryable + + switch { + case problem.Category == errs.CategoryAuthorization && problem.Code == 99991672: + decision.Class = "app_scope_missing" + decision.Terminal = true + case problem.Category == errs.CategoryAuthorization && problem.Code == 99991679: + decision.Class = "user_scope_missing" + decision.Terminal = true + case problem.Category == errs.CategoryAuthorization && problem.Subtype == errs.SubtypePermissionDenied: + decision.Class = "permission_denied" + decision.Terminal = true + case problem.Category == errs.CategoryNetwork && problem.Code == http.StatusForbidden: + decision.Class = "permission_denied" + decision.Terminal = true + case problem.Subtype == errs.SubtypeInvalidParameters || problem.Code == 1061002: + decision.Class = "invalid_api_parameters" + decision.Terminal = true + case problem.Subtype == errs.SubtypeRateLimit || problem.Code == 99991400: + decision.Class = "rate_limited" + decision.Terminal = true + case problem.Subtype == errs.SubtypeQuotaExceeded || problem.Code == 1061043: + decision.Class = "file_size_limit" + case problem.Code == 1062009: + decision.Class = "upload_size_mismatch" + case problem.Subtype == errs.SubtypeNotFound || problem.Code == 1061007: + decision.Class = "remote_not_found" + case problem.Subtype == errs.SubtypeServerError || problem.Code == 1061001 || problem.Code == 2200: + decision.Class = "server_error" + decision.Terminal = true + case problem.Subtype == errs.SubtypeFailedPrecondition: + decision.Class = "local_file_changed" + default: + decision.Class = string(problem.Subtype) + } + return decision +} + +func drivePushHasTerminalFailure(items []drivePushItem) bool { + for _, item := range items { + if driveTerminalBatchErrorClass(item.ErrorClass) { + return true + } + } + return false +} + +func driveTerminalBatchErrorClass(errorClass string) bool { + switch errorClass { + case "app_scope_missing", "user_scope_missing", "permission_denied", "invalid_api_parameters", "rate_limited", "server_error": + return true + default: + return false + } +} + func drivePushRemoteViews(entries []driveRemoteEntry, duplicateRemote string) (map[string]driveRemoteEntry, map[string]driveRemoteEntry, map[string][]driveRemoteEntry, error) { remoteFiles := make(map[string]driveRemoteEntry, len(entries)) remoteFolders := make(map[string]driveRemoteEntry, len(entries)) @@ -600,6 +737,12 @@ func drivePushEnsureParentToken(ctx context.Context, runtime *common.RuntimeCont // the three-step prepare/part/finish flow, which mirrors drive +upload's // existing multipart logic. func drivePushUploadFile(ctx context.Context, runtime *common.RuntimeContext, file drivePushLocalFile, existingToken, parentToken string) (string, string, error) { + if err := drivePushValidateUploadRequest(file, existingToken, parentToken); err != nil { + return "", "", err + } + if err := drivePushVerifyLocalSnapshot(runtime, file); err != nil { + return "", "", err + } if file.Size > common.MaxDriveMediaUploadSinglePartSize { token, err := drivePushUploadMultipart(ctx, runtime, file, existingToken, parentToken) // Multipart finish does not return version on the existing @@ -612,6 +755,44 @@ func drivePushUploadFile(ctx context.Context, runtime *common.RuntimeContext, fi return drivePushUploadAll(ctx, runtime, file, existingToken, parentToken) } +func drivePushValidateUploadRequest(file drivePushLocalFile, existingToken, parentToken string) error { + if strings.TrimSpace(file.FileName) == "" { + return errs.NewValidationError(errs.SubtypeInvalidArgument, "cannot upload %q: file name is empty", file.RelPath) + } + if file.Size < 0 { + return errs.NewValidationError(errs.SubtypeInvalidArgument, "cannot upload %q: file size is negative", file.RelPath) + } + if strings.TrimSpace(parentToken) == "" { + return errs.NewValidationError(errs.SubtypeInvalidArgument, "cannot upload %q: parent folder token is empty", file.RelPath) + } + if err := validate.ResourceName(parentToken, "parent_node"); err != nil { + return errs.NewValidationError(errs.SubtypeInvalidArgument, "cannot upload %q: %s", file.RelPath, err) + } + if existingToken != "" { + if err := validate.ResourceName(existingToken, "file_token"); err != nil { + return errs.NewValidationError(errs.SubtypeInvalidArgument, "cannot overwrite %q: %s", file.RelPath, err) + } + } + return nil +} + +func drivePushVerifyLocalSnapshot(runtime *common.RuntimeContext, file drivePushLocalFile) error { + info, err := runtime.FileIO().Stat(file.OpenPath) + if err != nil { + return errs.NewValidationError(errs.SubtypeFailedPrecondition, "local file changed during push: %s is no longer readable: %v", file.RelPath, err).WithCause(err) + } + if !info.Mode().IsRegular() { + return errs.NewValidationError(errs.SubtypeFailedPrecondition, "local file changed during push: %s is no longer a regular file", file.RelPath) + } + if info.Size() != file.Size { + return errs.NewValidationError(errs.SubtypeFailedPrecondition, "local file changed during push: %s snapshot size no longer matches", file.RelPath) + } + if modTimer, ok := info.(interface{ ModTime() time.Time }); ok && !modTimer.ModTime().Equal(file.ModTime) { + return errs.NewValidationError(errs.SubtypeFailedPrecondition, "local file changed during push: %s snapshot modtime no longer matches", file.RelPath) + } + return nil +} + func drivePushUploadAll(_ context.Context, runtime *common.RuntimeContext, file drivePushLocalFile, existingToken, parentToken string) (string, string, error) { f, err := runtime.FileIO().Open(file.OpenPath) if err != nil { diff --git a/shortcuts/drive/drive_push_test.go b/shortcuts/drive/drive_push_test.go index fff10eebe..dfdd60738 100644 --- a/shortcuts/drive/drive_push_test.go +++ b/shortcuts/drive/drive_push_test.go @@ -5,8 +5,10 @@ package drive import ( "context" + "encoding/json" "errors" "io" + "net/http" "os" "path/filepath" "strings" @@ -14,12 +16,14 @@ import ( "testing" "time" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/extension/fileio" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/httpmock" "github.com/larksuite/cli/internal/output" "github.com/larksuite/cli/shortcuts/common" + "github.com/spf13/cobra" ) // countingOpenProvider wraps a fileio.Provider and counts FileIO.Open @@ -652,6 +656,82 @@ func TestDrivePushDeleteRemoteSkipsOnlineDocs(t *testing.T) { } } +func TestDrivePushDeleteRemoteAbortsAfterTerminalFailure(t *testing.T) { + f, stdout, _, reg := cmdutil.TestFactory(t, driveTestConfig()) + + tmpDir := t.TempDir() + withDriveWorkingDir(t, tmpDir) + if err := os.MkdirAll("local", 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "folder_token=folder_root", + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "files": []interface{}{ + map[string]interface{}{"token": "tok_a", "name": "a.txt", "type": "file"}, + map[string]interface{}{"token": "tok_b", "name": "b.txt", "type": "file"}, + }, + "has_more": false, + }, + }, + }) + reg.Register(&httpmock.Stub{ + Method: "DELETE", + URL: "/open-apis/drive/v1/files/tok_a", + Body: map[string]interface{}{ + "code": 1061004, + "msg": "forbidden", + }, + }) + // No DELETE stub for tok_b: terminal delete failure must stop before it. + + err := mountAndRunDrive(t, DrivePush, []string{ + "+push", + "--local-dir", "local", + "--folder-token", "folder_root", + "--delete-remote", + "--yes", + "--as", "bot", + }, f, stdout) + if err == nil { + t.Fatalf("expected partial failure, got nil\nstdout: %s", stdout.String()) + } + var pfErr *output.PartialFailureError + if !errors.As(err, &pfErr) || pfErr.Code != output.ExitAPI { + t.Fatalf("expected ExitAPI *output.PartialFailureError, got %T %v", err, err) + } + + summary, items := splitDrivePushStdout(t, stdout.Bytes()) + if got := summary["failed"]; got != float64(1) { + t.Fatalf("summary.failed = %v, want 1", got) + } + if got := summary["aborted"]; got != true { + t.Fatalf("summary.aborted = %v, want true", got) + } + if got := summary["deleted_remote"]; got != float64(0) { + t.Fatalf("summary.deleted_remote = %v, want 0", got) + } + if len(items) != 1 { + t.Fatalf("items len = %d, want 1; items=%#v", len(items), items) + } + item := items[0] + if item["action"] != "delete_failed" || item["phase"] != "delete" || item["error_class"] != "permission_denied" { + t.Fatalf("unexpected failed item: %#v", item) + } + if item["code"] != float64(1061004) || item["retryable"] != false { + t.Fatalf("unexpected failure metadata: %#v", item) + } + for _, item := range items { + if item["file_token"] == "tok_b" { + t.Fatalf("terminal delete failure must abort before tok_b, got items=%#v", items) + } + } +} + func TestDrivePushNewestOverwritesChosenDuplicateAndDeletesSibling(t *testing.T) { f, stdout, _, reg := cmdutil.TestFactory(t, driveTestConfig()) @@ -886,21 +966,22 @@ func TestDrivePushOverwriteWithoutVersionFails(t *testing.T) { t.Errorf("expected ExitAPI (%d), got code=%d", output.ExitAPI, pfErr.Code) } - out := stdout.String() - // summary.failed should reflect the missing version; summary.uploaded - // should not pretend the overwrite succeeded. - if !strings.Contains(out, `"failed": 1`) { - t.Errorf("expected failed=1, got: %s", out) + summary, items := splitDrivePushStdout(t, stdout.Bytes()) + if got := summary["failed"]; got != float64(1) { + t.Errorf("summary.failed = %v, want 1", got) } - if !strings.Contains(out, "no version") { - t.Errorf("expected error about missing version in items[].error, got: %s", out) + if len(items) != 1 { + t.Fatalf("items len = %d, want 1; items=%#v", len(items), items) + } + if got, _ := items[0]["error"].(string); !strings.Contains(got, "no version") { + t.Errorf("items[0].error = %q, want missing-version message", got) } // Pin the token-stability contract: the failed item must surface the // token returned by upload_all (tok_keep_new), NOT the fallback // entry.FileToken (tok_keep). Without this, a regression that always // uses entry.FileToken on failure would slip through. - if !strings.Contains(out, `"file_token": "tok_keep_new"`) { - t.Errorf("expected failed item to surface upload_all's returned file_token (tok_keep_new), got: %s", out) + if got := items[0]["file_token"]; got != "tok_keep_new" { + t.Errorf("items[0].file_token = %v, want tok_keep_new", got) } } @@ -962,24 +1043,313 @@ func TestDrivePushOverwritePartialSuccessSurfacesReturnedToken(t *testing.T) { t.Fatalf("expected ExitAPI from *output.PartialFailureError, got %T %v", err, err) } - out := stdout.String() // Partial failure reports an ok:false result envelope on stdout (not a // misleading ok:true) while still carrying BOTH the succeeded and failed // items — consistent with the pre-change payload. The failed side is // asserted via "failed": 1 and the succeeded side via tok_keep_partial. - if !strings.Contains(out, `"ok": false`) { - t.Errorf("partial failure must emit an ok:false result envelope, got: %s", out) + envelope := decodeDrivePushStdout(t, stdout.Bytes()) + if envelope.OK { + t.Fatalf("partial failure must emit ok=false; stdout=%s", stdout.String()) } - if !strings.Contains(out, `"failed": 1`) { - t.Errorf("expected failed=1, got: %s", out) + summary, items := splitDrivePushStdout(t, stdout.Bytes()) + if got := summary["failed"]; got != float64(1) { + t.Errorf("summary.failed = %v, want 1", got) + } + if len(items) != 1 { + t.Fatalf("items len = %d, want 1; items=%#v", len(items), items) } // The freshly returned token must be the one in items[].file_token, // not the stale entry.FileToken (tok_keep_old). - if !strings.Contains(out, `"file_token": "tok_keep_partial"`) { - t.Errorf("expected items[].file_token to surface upload_all's returned token (tok_keep_partial), got: %s", out) + if got := items[0]["file_token"]; got != "tok_keep_partial" { + t.Errorf("items[0].file_token = %v, want tok_keep_partial", got) } - if strings.Contains(out, `"file_token": "tok_keep_old"`) { - t.Errorf("must NOT fall back to entry.FileToken when upload_all returned a token; got: %s", out) + if got := items[0]["file_token"]; got == "tok_keep_old" { + t.Errorf("must NOT fall back to entry.FileToken when upload_all returned a token; item=%#v", items[0]) + } +} + +func TestDrivePushAbortsAfterUploadParamsError(t *testing.T) { + f, stdout, _, reg := cmdutil.TestFactory(t, driveTestConfig()) + + tmpDir := t.TempDir() + withDriveWorkingDir(t, tmpDir) + if err := os.MkdirAll("local", 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + if err := os.WriteFile(filepath.Join("local", "a.txt"), []byte("A"), 0o644); err != nil { + t.Fatalf("WriteFile a: %v", err) + } + if err := os.WriteFile(filepath.Join("local", "b.txt"), []byte("B"), 0o644); err != nil { + t.Fatalf("WriteFile b: %v", err) + } + + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "folder_token=folder_root", + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{"files": []interface{}{}, "has_more": false}, + }, + }) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/drive/v1/files/upload_all", + Body: map[string]interface{}{ + "code": 1061002, + "msg": "params error.", + }, + }) + + err := mountAndRunDrive(t, DrivePush, []string{ + "+push", + "--local-dir", "local", + "--folder-token", "folder_root", + "--as", "bot", + }, f, stdout) + if err == nil { + t.Fatalf("expected partial failure, got nil\nstdout: %s", stdout.String()) + } + var pfErr *output.PartialFailureError + if !errors.As(err, &pfErr) { + t.Fatalf("expected *output.PartialFailureError, got %T: %v", err, err) + } + summary, items := splitDrivePushStdout(t, stdout.Bytes()) + if got := summary["failed"]; got != float64(1) { + t.Fatalf("summary.failed = %v, want 1", got) + } + if got := summary["aborted"]; got != true { + t.Fatalf("summary.aborted = %v, want true", got) + } + if len(items) != 1 { + t.Fatalf("items len = %d, want 1; items=%#v", len(items), items) + } + item := items[0] + if item["rel_path"] != "a.txt" || item["phase"] != "upload" || item["error_class"] != "invalid_api_parameters" { + t.Fatalf("unexpected failed item: %#v", item) + } + if item["code"] != float64(1061002) || item["subtype"] != "invalid_parameters" || item["retryable"] != false { + t.Fatalf("unexpected failure metadata: %#v", item) + } + for _, item := range items { + if item["rel_path"] == "b.txt" { + t.Fatalf("terminal upload params error must abort before b.txt, got items=%#v", items) + } + } +} + +func TestDrivePushAbortsAfterCreateFolderMissingScope(t *testing.T) { + f, stdout, _, reg := cmdutil.TestFactory(t, driveTestConfig()) + + tmpDir := t.TempDir() + withDriveWorkingDir(t, tmpDir) + if err := os.MkdirAll(filepath.Join("local", "a"), 0o755); err != nil { + t.Fatalf("MkdirAll a: %v", err) + } + if err := os.MkdirAll(filepath.Join("local", "b"), 0o755); err != nil { + t.Fatalf("MkdirAll b: %v", err) + } + + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "folder_token=folder_root", + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{"files": []interface{}{}, "has_more": false}, + }, + }) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/drive/v1/files/create_folder", + Body: map[string]interface{}{ + "code": 99991672, + "msg": "Access denied. One of the following scopes is required: [drive:drive, space:folder:create].", + }, + }) + + err := mountAndRunDrive(t, DrivePush, []string{ + "+push", + "--local-dir", "local", + "--folder-token", "folder_root", + "--as", "bot", + }, f, stdout) + if err == nil { + t.Fatalf("expected partial failure, got nil\nstdout: %s", stdout.String()) + } + var pfErr *output.PartialFailureError + if !errors.As(err, &pfErr) { + t.Fatalf("expected *output.PartialFailureError, got %T: %v", err, err) + } + summary, items := splitDrivePushStdout(t, stdout.Bytes()) + if got := summary["failed"]; got != float64(1) { + t.Fatalf("summary.failed = %v, want 1", got) + } + if got := summary["aborted"]; got != true { + t.Fatalf("summary.aborted = %v, want true", got) + } + if len(items) != 1 { + t.Fatalf("items len = %d, want 1; items=%#v", len(items), items) + } + item := items[0] + if item["rel_path"] != "a" || item["phase"] != "create_folder" || item["error_class"] != "app_scope_missing" { + t.Fatalf("unexpected failed item: %#v", item) + } + if item["code"] != float64(99991672) || item["retryable"] != false { + t.Fatalf("unexpected failure metadata: %#v", item) + } + for _, item := range items { + if item["rel_path"] == "b" { + t.Fatalf("missing folder-create scope must abort before b, got items=%#v", items) + } + } +} + +func TestDrivePushDetectsLocalFileChangedBeforeUpload(t *testing.T) { + f, stdout, _, reg := cmdutil.TestFactory(t, driveTestConfig()) + + tmpDir := t.TempDir() + withDriveWorkingDir(t, tmpDir) + if err := os.MkdirAll("local", 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + localPath := filepath.Join("local", "changing.txt") + if err := os.WriteFile(localPath, []byte("before"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "folder_token=folder_root", + OnMatch: func(req *http.Request) { + if err := os.WriteFile(localPath, []byte("after-change"), 0o644); err != nil { + t.Fatalf("mutate local file: %v", err) + } + }, + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{"files": []interface{}{}, "has_more": false}, + }, + }) + err := mountAndRunDrive(t, DrivePush, []string{ + "+push", + "--local-dir", "local", + "--folder-token", "folder_root", + "--as", "bot", + }, f, stdout) + if err == nil { + t.Fatalf("expected partial failure, got nil\nstdout: %s", stdout.String()) + } + var pfErr *output.PartialFailureError + if !errors.As(err, &pfErr) { + t.Fatalf("expected *output.PartialFailureError, got %T: %v", err, err) + } + summary, items := splitDrivePushStdout(t, stdout.Bytes()) + if got := summary["failed"]; got != float64(1) { + t.Fatalf("summary.failed = %v, want 1", got) + } + if got := summary["aborted"]; got != false { + t.Fatalf("summary.aborted = %v, want false", got) + } + if len(items) != 1 { + t.Fatalf("items len = %d, want 1; items=%#v", len(items), items) + } + item := items[0] + if item["error_class"] != "local_file_changed" || item["subtype"] != "failed_precondition" || item["retryable"] != false { + t.Fatalf("unexpected failure metadata: %#v", item) + } + if got, _ := item["error"].(string); !strings.Contains(got, "local file changed during push") { + t.Fatalf("items[0].error = %q, want local-change message", got) + } + if strings.Contains(stdout.String(), "httpmock: no stub") { + t.Fatalf("upload_all was called after local snapshot changed:\n%s", stdout.String()) + } + + problemErr := drivePushVerifyLocalSnapshot(common.TestNewRuntimeContext(&cobra.Command{Use: "drive +push"}, &core.CliConfig{}), drivePushLocalFile{ + RelPath: "missing.txt", + OpenPath: filepath.Join("local", "missing.txt"), + FileName: "missing.txt", + Size: 1, + ModTime: time.Now(), + }) + problem, ok := errs.ProblemOf(problemErr) + if !ok { + t.Fatalf("ProblemOf(snapshot error) ok=false, err=%T %v", problemErr, problemErr) + } + if problem.Subtype != errs.SubtypeFailedPrecondition { + t.Fatalf("snapshot error subtype = %q, want %q", problem.Subtype, errs.SubtypeFailedPrecondition) + } + if problem.Category != errs.CategoryValidation { + t.Fatalf("snapshot error category = %q, want %q", problem.Category, errs.CategoryValidation) + } + if errors.Unwrap(problemErr) == nil { + t.Fatalf("snapshot error cause was not preserved") + } +} + +func TestDrivePushDetectsSameSizeLocalFileChangedBeforeUpload(t *testing.T) { + f, stdout, _, reg := cmdutil.TestFactory(t, driveTestConfig()) + + tmpDir := t.TempDir() + withDriveWorkingDir(t, tmpDir) + if err := os.MkdirAll("local", 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + localPath := filepath.Join("local", "changing.txt") + if err := os.WriteFile(localPath, []byte("before"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + originalModTime := time.Unix(100, 0) + changedModTime := time.Unix(200, 0) + if err := os.Chtimes(localPath, originalModTime, originalModTime); err != nil { + t.Fatalf("Chtimes original: %v", err) + } + + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "folder_token=folder_root", + OnMatch: func(req *http.Request) { + if err := os.WriteFile(localPath, []byte("AFTER!"), 0o644); err != nil { + t.Fatalf("mutate local file: %v", err) + } + if err := os.Chtimes(localPath, changedModTime, changedModTime); err != nil { + t.Fatalf("Chtimes changed: %v", err) + } + }, + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{"files": []interface{}{}, "has_more": false}, + }, + }) + + err := mountAndRunDrive(t, DrivePush, []string{ + "+push", + "--local-dir", "local", + "--folder-token", "folder_root", + "--as", "bot", + }, f, stdout) + if err == nil { + t.Fatalf("expected partial failure, got nil\nstdout: %s", stdout.String()) + } + var pfErr *output.PartialFailureError + if !errors.As(err, &pfErr) { + t.Fatalf("expected *output.PartialFailureError, got %T: %v", err, err) + } + + summary, items := splitDrivePushStdout(t, stdout.Bytes()) + if got := summary["failed"]; got != float64(1) { + t.Fatalf("summary.failed = %v, want 1", got) + } + if len(items) != 1 { + t.Fatalf("items len = %d, want 1; items=%#v", len(items), items) + } + item := items[0] + if item["rel_path"] != "changing.txt" || item["error_class"] != "local_file_changed" || item["subtype"] != "failed_precondition" { + t.Fatalf("unexpected failure metadata: %#v", item) + } + if got, _ := item["error"].(string); !strings.Contains(got, "snapshot modtime no longer matches") { + t.Fatalf("items[0].error = %q, want modtime mismatch", got) + } + if strings.Contains(stdout.String(), "httpmock: no stub") { + t.Fatalf("upload_all was called after local snapshot changed:\n%s", stdout.String()) } } @@ -1113,6 +1483,32 @@ func TestDrivePushExitsZeroOnCleanRun(t *testing.T) { } } +type drivePushStdoutEnvelope struct { + OK bool `json:"ok"` + Data struct { + Summary map[string]interface{} `json:"summary"` + Items []map[string]interface{} `json:"items"` + } `json:"data"` +} + +func decodeDrivePushStdout(t *testing.T, stdout []byte) drivePushStdoutEnvelope { + t.Helper() + var envelope drivePushStdoutEnvelope + if err := json.Unmarshal(stdout, &envelope); err != nil { + t.Fatalf("unmarshal stdout: %v\nraw=%s", err, string(stdout)) + } + return envelope +} + +func splitDrivePushStdout(t *testing.T, stdout []byte) (map[string]interface{}, []map[string]interface{}) { + t.Helper() + envelope := decodeDrivePushStdout(t, stdout) + if envelope.Data.Summary == nil { + t.Fatalf("stdout missing data.summary; raw=%s", string(stdout)) + } + return envelope.Data.Summary, envelope.Data.Items +} + // TestDrivePushUploadsSiblingWhenRemoteSameNameIsNativeDoc pins the // behavior when a local regular file shares its rel_path with a Lark // native cloud document on Drive (sheet/docx/bitable/...). diff --git a/shortcuts/drive/drive_sync.go b/shortcuts/drive/drive_sync.go index ecb97e52d..75658e351 100644 --- a/shortcuts/drive/drive_sync.go +++ b/shortcuts/drive/drive_sync.go @@ -25,12 +25,21 @@ const ( driveSyncOnConflictAsk = "ask" ) +func driveSyncActionScopes() []string { + return []string{"drive:file:download", "drive:file:upload", "space:folder:create"} +} + type driveSyncItem struct { - RelPath string `json:"rel_path"` - FileToken string `json:"file_token,omitempty"` - Action string `json:"action"` - Direction string `json:"direction,omitempty"` // "pull" or "push" - Error string `json:"error,omitempty"` + RelPath string `json:"rel_path"` + FileToken string `json:"file_token,omitempty"` + Action string `json:"action"` + Direction string `json:"direction,omitempty"` // "pull" or "push" + Error string `json:"error,omitempty"` + Phase string `json:"phase,omitempty"` + ErrorClass string `json:"error_class,omitempty"` + Code int `json:"code,omitempty"` + Subtype string `json:"subtype,omitempty"` + Retryable *bool `json:"retryable,omitempty"` } // DriveSync performs a two-way sync between a local directory and a Drive @@ -66,6 +75,7 @@ var DriveSync = common.Shortcut{ "Default --on-conflict=remote-wins pulls the remote version when both sides changed a file. Use local-wins to push instead, keep-both to rename and keep both copies, or ask for interactive resolution.", "Pass --quick for faster best-effort diff detection using modified_time instead of SHA-256 hash (no remote file downloads needed during diffing).", "Because +sync acts on the diff, --quick can still pull, overwrite, or rename files when timestamps differ even if file contents are actually unchanged.", + "Actual sync execution pre-flights download, upload, and folder-create scopes before listing or walking, so missing grants fail before any partial sync can start.", "Only entries with type=file are synced; online docs (docx, sheet, bitable, mindnote, slides) and shortcuts are skipped.", }, Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { @@ -110,10 +120,8 @@ var DriveSync = common.Shortcut{ duplicateRemote = driveDuplicateRemoteFail } quick := runtime.Bool("quick") - if !quick { - if err := runtime.EnsureScopes([]string{"drive:file:download"}); err != nil { - return err - } + if err := runtime.EnsureScopes(driveSyncActionScopes()); err != nil { + return err } safeRoot, err := validate.SafeInputPath(localDir) @@ -262,18 +270,6 @@ var DriveSync = common.Shortcut{ var pulled, pushed, skipped, failed int items := make([]driveSyncItem, 0) - if quick && driveSyncNeedsDownloadScope(newRemote, modified, conflictResolutions) { - if err := runtime.EnsureScopes([]string{"drive:file:download"}); err != nil { - return err - } - } - plannedUploads := driveSyncPlannedUploadPaths(newLocal, modified, conflictResolutions) - if len(plannedUploads) > 0 { - if err := runtime.EnsureScopes([]string{"drive:file:upload"}); err != nil { - return err - } - } - // Build push infrastructure: local walk for push + remote views + folder cache. folderCache := map[string]string{"": folderToken} for relDir, entry := range remoteFolders { @@ -287,20 +283,18 @@ var DriveSync = common.Shortcut{ return err } - if driveSyncNeedsCreateScope(plannedUploads, localDirs, folderCache) { - if err := runtime.EnsureScopes([]string{"space:folder:create"}); err != nil { - return err - } - } - // Mirror local directory structure first (same as +push), so // empty local directories are not silently dropped. for _, relDir := range localDirs { + if driveSyncHasTerminalFailure(items) { + break + } if _, alreadyRemote := folderCache[relDir]; alreadyRemote { continue } if _, ensureErr := drivePushEnsureFolder(ctx, runtime, folderToken, relDir, folderCache); ensureErr != nil { - items = append(items, driveSyncItem{RelPath: relDir, Action: "failed", Direction: "push", Error: ensureErr.Error()}) + item, _ := driveSyncFailedItem(relDir, "", "failed", "push", "create_folder", ensureErr) + items = append(items, item) failed++ continue } @@ -310,6 +304,9 @@ var DriveSync = common.Shortcut{ // 2a. Pull new_remote files. for _, entry := range newRemote { + if driveSyncHasTerminalFailure(items) { + break + } targetFile, ok := pullRemoteFiles[entry.RelPath] if !ok { // Non-file type (doc, shortcut, etc.) — skip. @@ -317,8 +314,13 @@ var DriveSync = common.Shortcut{ } target := filepath.Join(rootRelToCwd, entry.RelPath) if err := drivePullDownload(ctx, runtime, targetFile.DownloadToken, target, targetFile.ModifiedTime); err != nil { - items = append(items, driveSyncItem{RelPath: entry.RelPath, FileToken: entry.FileToken, Action: "failed", Direction: "pull", Error: err.Error()}) + item, terminal := driveSyncFailedItem(entry.RelPath, entry.FileToken, "failed", "pull", "download", err) + items = append(items, item) failed++ + if terminal { + fmt.Fprintf(runtime.IO().ErrOut, "Aborting +sync after terminal %s failure: %v\n", item.Phase, err) + break + } continue } items = append(items, driveSyncItem{RelPath: entry.RelPath, FileToken: entry.FileToken, Action: "downloaded", Direction: "pull"}) @@ -327,6 +329,9 @@ var DriveSync = common.Shortcut{ // 2b. Push new_local files. for _, entry := range newLocal { + if driveSyncHasTerminalFailure(items) { + break + } localFile, ok := pushLocalFiles[entry.RelPath] if !ok { items = append(items, driveSyncItem{RelPath: entry.RelPath, Action: "skipped", Direction: "push", Error: "local file disappeared during sync"}) @@ -336,14 +341,20 @@ var DriveSync = common.Shortcut{ parentRel := drivePushParentRel(entry.RelPath) parentToken, ensureErr := drivePushEnsureFolder(ctx, runtime, folderToken, parentRel, folderCache) if ensureErr != nil { - items = append(items, driveSyncItem{RelPath: entry.RelPath, Action: "failed", Direction: "push", Error: ensureErr.Error()}) + item, _ := driveSyncFailedItem(entry.RelPath, "", "failed", "push", "create_folder", ensureErr) + items = append(items, item) failed++ continue } token, _, upErr := drivePushUploadFile(ctx, runtime, localFile, "", parentToken) if upErr != nil { - items = append(items, driveSyncItem{RelPath: entry.RelPath, Action: "failed", Direction: "push", Error: upErr.Error()}) + item, terminal := driveSyncFailedItem(entry.RelPath, token, "failed", "push", "upload", upErr) + items = append(items, item) failed++ + if terminal { + fmt.Fprintf(runtime.IO().ErrOut, "Aborting +sync after terminal %s failure: %v\n", item.Phase, upErr) + break + } continue } items = append(items, driveSyncItem{RelPath: entry.RelPath, FileToken: token, Action: "uploaded", Direction: "push"}) @@ -352,6 +363,9 @@ var DriveSync = common.Shortcut{ // 2c. Resolve modified files by --on-conflict strategy. for _, entry := range modified { + if driveSyncHasTerminalFailure(items) { + break + } remoteFile := remoteFiles[entry.RelPath] localFile, hasLocal := pushLocalFiles[entry.RelPath] if !hasLocal { @@ -379,8 +393,13 @@ var DriveSync = common.Shortcut{ } target := filepath.Join(rootRelToCwd, entry.RelPath) if err := drivePullDownload(ctx, runtime, targetFile.DownloadToken, target, targetFile.ModifiedTime); err != nil { - items = append(items, driveSyncItem{RelPath: entry.RelPath, FileToken: entry.FileToken, Action: "failed", Direction: "pull", Error: err.Error()}) + item, terminal := driveSyncFailedItem(entry.RelPath, entry.FileToken, "failed", "pull", "download", err) + items = append(items, item) failed++ + if terminal { + fmt.Fprintf(runtime.IO().ErrOut, "Aborting +sync after terminal %s failure: %v\n", item.Phase, err) + break + } continue } items = append(items, driveSyncItem{RelPath: entry.RelPath, FileToken: entry.FileToken, Action: "downloaded", Direction: "pull"}) @@ -396,7 +415,8 @@ var DriveSync = common.Shortcut{ } parentToken, parentErr := drivePushEnsureFolder(ctx, runtime, folderToken, drivePushParentRel(entry.RelPath), folderCache) if parentErr != nil { - items = append(items, driveSyncItem{RelPath: entry.RelPath, FileToken: existingToken, Action: "failed", Direction: "push", Error: parentErr.Error()}) + item, _ := driveSyncFailedItem(entry.RelPath, existingToken, "failed", "push", "create_folder", parentErr) + items = append(items, item) failed++ continue } @@ -411,8 +431,13 @@ var DriveSync = common.Shortcut{ if failedToken == "" { failedToken = existingToken } - items = append(items, driveSyncItem{RelPath: entry.RelPath, FileToken: failedToken, Action: "failed", Direction: "push", Error: upErr.Error()}) + item, terminal := driveSyncFailedItem(entry.RelPath, failedToken, "failed", "push", "upload", upErr) + items = append(items, item) failed++ + if terminal { + fmt.Fprintf(runtime.IO().ErrOut, "Aborting +sync after terminal %s failure: %v\n", item.Phase, upErr) + break + } continue } items = append(items, driveSyncItem{RelPath: entry.RelPath, FileToken: token, Action: "overwritten", Direction: "push"}) @@ -433,7 +458,8 @@ var DriveSync = common.Shortcut{ } suffixedRel, err := relPathWithUniqueFileTokenSuffix(entry.RelPath, remoteFile.FileToken, occupied) if err != nil { - items = append(items, driveSyncItem{RelPath: entry.RelPath, Action: "failed", Direction: "conflict", Error: err.Error()}) + item, _ := driveSyncFailedItem(entry.RelPath, "", "failed", "conflict", "conflict", err) + items = append(items, item) failed++ continue } @@ -441,7 +467,9 @@ var DriveSync = common.Shortcut{ oldAbsPath := filepath.Join(safeRoot, filepath.FromSlash(entry.RelPath)) newAbsPath := filepath.Join(safeRoot, filepath.FromSlash(suffixedRel)) if err := os.Rename(oldAbsPath, newAbsPath); err != nil { //nolint:forbidigo // shortcuts cannot import internal/vfs (depguard rule shortcuts-no-vfs); safeRoot is validated. - items = append(items, driveSyncItem{RelPath: entry.RelPath, Action: "failed", Direction: "conflict", Error: fmt.Sprintf("rename local: %s", err)}) + renameErr := errs.NewInternalError(errs.SubtypeFileIO, "rename local: %s", err).WithCause(err) + item, _ := driveSyncFailedItem(entry.RelPath, "", "failed", "conflict", "conflict", renameErr) + items = append(items, item) failed++ continue } @@ -454,19 +482,30 @@ var DriveSync = common.Shortcut{ if rollbackErr != nil { errMsg += "; rollback failed: " + rollbackErr.Error() } - items = append(items, driveSyncItem{RelPath: entry.RelPath, Action: "failed", Direction: "pull", Error: errMsg}) + notFoundErr := errs.NewAPIError(errs.SubtypeNotFound, "%s", errMsg) + item, _ := driveSyncFailedItem(entry.RelPath, "", "failed", "pull", "download", notFoundErr) + items = append(items, item) failed++ continue } target := filepath.Join(rootRelToCwd, entry.RelPath) if err := drivePullDownload(ctx, runtime, targetFile.DownloadToken, target, targetFile.ModifiedTime); err != nil { + downloadErr := err rollbackErr := driveSyncRollbackRenamedLocal(oldAbsPath, newAbsPath) errMsg := err.Error() if rollbackErr != nil { errMsg += "; rollback failed: " + rollbackErr.Error() } - items = append(items, driveSyncItem{RelPath: entry.RelPath, FileToken: entry.FileToken, Action: "failed", Direction: "pull", Error: errMsg}) + item, terminal := driveSyncFailedItem(entry.RelPath, entry.FileToken, "failed", "pull", "download", downloadErr) + if rollbackErr != nil { + item.Error = errMsg + } + items = append(items, item) failed++ + if terminal { + fmt.Fprintf(runtime.IO().ErrOut, "Aborting +sync after terminal %s failure: %v\n", item.Phase, downloadErr) + break + } continue } items = append(items, driveSyncItem{RelPath: entry.RelPath, Action: "renamed_local", Direction: "conflict"}) @@ -492,6 +531,7 @@ var DriveSync = common.Shortcut{ "pushed": pushed, "skipped": skipped, "failed": failed, + "aborted": driveSyncHasTerminalFailure(items), }, "items": items, } @@ -520,6 +560,32 @@ func driveSyncStatusRemoteFiles(pullRemoteFiles map[string]drivePullTarget) map[ return remoteFiles } +func driveSyncFailedItem(relPath, fileToken, action, direction, phase string, err error) (driveSyncItem, bool) { + decision := driveClassifyBatchFailure(err) + item := driveSyncItem{ + RelPath: relPath, + FileToken: fileToken, + Action: action, + Direction: direction, + Error: err.Error(), + Phase: phase, + ErrorClass: decision.Class, + Code: decision.Code, + Subtype: decision.Subtype, + Retryable: driveBoolPtr(decision.Retryable), + } + return item, decision.Terminal +} + +func driveSyncHasTerminalFailure(items []driveSyncItem) bool { + for _, item := range items { + if driveTerminalBatchErrorClass(item.ErrorClass) { + return true + } + } + return false +} + // driveSyncAskConflict prompts the user for a conflict resolution strategy // for a single file. Returns the strategy string, or empty string if the // user chose to skip. @@ -558,51 +624,6 @@ func driveSyncAskConflict(relPath string, runtime *common.RuntimeContext) (strin } } -func driveSyncNeedsDownloadScope(newRemote, modified []driveStatusEntry, conflictResolutions map[string]string) bool { - if len(newRemote) > 0 { - return true - } - for _, entry := range modified { - switch conflictResolutions[entry.RelPath] { - case driveSyncOnConflictRemoteWins, driveSyncOnConflictKeepBoth: - return true - } - } - return false -} - -func driveSyncPlannedUploadPaths(newLocal, modified []driveStatusEntry, conflictResolutions map[string]string) []string { - planned := make([]string, 0, len(newLocal)+len(modified)) - for _, entry := range newLocal { - planned = append(planned, entry.RelPath) - } - for _, entry := range modified { - if conflictResolutions[entry.RelPath] == driveSyncOnConflictLocalWins { - planned = append(planned, entry.RelPath) - } - } - return planned -} - -func driveSyncNeedsCreateScope(uploadPaths []string, localDirs []string, folderCache map[string]string) bool { - for _, relPath := range uploadPaths { - parentRel := drivePushParentRel(relPath) - if parentRel == "" { - continue - } - if _, ok := folderCache[parentRel]; !ok { - return true - } - } - // Empty local directories also need create_folder if not already on Drive. - for _, relDir := range localDirs { - if _, ok := folderCache[relDir]; !ok { - return true - } - } - return false -} - func driveSyncRollbackRenamedLocal(oldAbsPath, newAbsPath string) error { if info, err := os.Stat(oldAbsPath); err == nil { //nolint:forbidigo // shortcuts cannot import internal/vfs (depguard rule shortcuts-no-vfs); safeRoot is validated. if info.IsDir() { diff --git a/shortcuts/drive/drive_sync_test.go b/shortcuts/drive/drive_sync_test.go index 1e1ea5afc..70104a2f3 100644 --- a/shortcuts/drive/drive_sync_test.go +++ b/shortcuts/drive/drive_sync_test.go @@ -311,6 +311,71 @@ func TestDriveSyncRemoteWinsPullsNewRemoteAndPushesNewLocal(t *testing.T) { } } +func TestDriveSyncAbortsAfterNewRemoteDownloadForbidden(t *testing.T) { + syncTestConfig := &core.CliConfig{ + AppID: "drive-sync-forbidden", AppSecret: "test-secret", Brand: core.BrandFeishu, + } + f, stdout, _, reg := cmdutil.TestFactory(t, syncTestConfig) + + tmpDir := t.TempDir() + withDriveWorkingDir(t, tmpDir) + if err := os.MkdirAll("local", 0o755); err != nil { + t.Fatalf("MkdirAll: %v", err) + } + + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "folder_token=folder_root", + Body: map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "files": []interface{}{ + map[string]interface{}{"token": "tok_a", "name": "a.txt", "type": "file", "modified_time": "100"}, + map[string]interface{}{"token": "tok_b", "name": "b.txt", "type": "file", "modified_time": "100"}, + }, + "has_more": false, + }, + }, + }) + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "/open-apis/drive/v1/files/tok_a/download", + Status: http.StatusForbidden, + RawBody: []byte("forbidden"), + }) + + err := mountAndRunDrive(t, DriveSync, []string{ + "+sync", + "--local-dir", "local", + "--folder-token", "folder_root", + "--quick", + "--as", "bot", + }, f, stdout) + assertDriveSyncPartialFailure(t, err) + + summary := driveSyncStdoutSummary(t, stdout.Bytes()) + if got := summary["aborted"]; got != true { + t.Fatalf("summary.aborted = %v, want true", got) + } + if got := summary["failed"]; got != float64(1) { + t.Fatalf("summary.failed = %v, want 1", got) + } + items := driveSyncStdoutItems(t, stdout.Bytes()) + if len(items) != 1 { + t.Fatalf("items len = %d, want 1; items=%#v", len(items), items) + } + item := items[0] + if item.RelPath != "a.txt" || item.Direction != "pull" || item.Phase != "download" || item.ErrorClass != "permission_denied" { + t.Fatalf("unexpected failed item: %#v", item) + } + if item.Code != http.StatusForbidden || item.Retryable == nil || *item.Retryable { + t.Fatalf("unexpected failure classification: %#v", item) + } + if _, statErr := os.Stat(filepath.Join("local", "b.txt")); !os.IsNotExist(statErr) { + t.Fatalf("b.txt should not be downloaded after terminal permission failure; stat err=%v", statErr) + } +} + // TestDriveSyncLocalWinsPushesOverRemote verifies that --on-conflict=local-wins // pushes the local version over the remote file. func TestDriveSyncLocalWinsPushesOverRemote(t *testing.T) { @@ -1552,11 +1617,11 @@ func TestDriveSyncDryRunQuickAcceptsMetadataOnlyScope(t *testing.T) { } } -func TestDriveSyncExactRemoteWinsAcceptsDownloadOnlyScope(t *testing.T) { +func TestDriveSyncPreflightsActionScopesBeforeListing(t *testing.T) { syncTestConfig := &core.CliConfig{ AppID: "drive-sync-download-scope-only", AppSecret: "test-secret", Brand: core.BrandFeishu, } - f, stdout, _, reg := cmdutil.TestFactory(t, syncTestConfig) + f, stdout, _, _ := cmdutil.TestFactory(t, syncTestConfig) f.Credential = credential.NewCredentialProvider(nil, nil, &driveStatusScopedTokenResolver{scopes: "drive:drive.metadata:readonly drive:file:download"}, nil) tmpDir := t.TempDir() @@ -1568,34 +1633,6 @@ func TestDriveSyncExactRemoteWinsAcceptsDownloadOnlyScope(t *testing.T) { t.Fatalf("WriteFile a.txt: %v", err) } - reg.Register(&httpmock.Stub{ - Method: "GET", - URL: "folder_token=folder_root", - Body: map[string]interface{}{ - "code": 0, "msg": "ok", - "data": map[string]interface{}{ - "files": []interface{}{ - map[string]interface{}{"token": "tok_a", "name": "a.txt", "type": "file"}, - }, - "has_more": false, - }, - }, - }) - reg.Register(&httpmock.Stub{ - Method: "GET", - URL: "/open-apis/drive/v1/files/tok_a/download", - Status: 200, - Body: []byte("remote-a"), - Headers: http.Header{"Content-Type": []string{"application/octet-stream"}}, - }) - reg.Register(&httpmock.Stub{ - Method: "GET", - URL: "/open-apis/drive/v1/files/tok_a/download", - Status: 200, - Body: []byte("remote-a"), - Headers: http.Header{"Content-Type": []string{"application/octet-stream"}}, - }) - err := mountAndRunDrive(t, DriveSync, []string{ "+sync", "--local-dir", "local", @@ -1603,11 +1640,30 @@ func TestDriveSyncExactRemoteWinsAcceptsDownloadOnlyScope(t *testing.T) { "--on-conflict", "remote-wins", "--as", "bot", }, f, stdout) - if err != nil { - t.Fatalf("expected exact remote-wins to succeed with download-only scope, got: %v\nstdout: %s", err, stdout.String()) + if err == nil { + t.Fatalf("expected action-scope preflight to reject download-only scope\nstdout: %s", stdout.String()) } - if strings.Contains(strings.ToLower(stdout.String()), "missing_scope") { - t.Fatalf("should not surface missing_scope, got: %s", stdout.String()) + var permErr *errs.PermissionError + if !errors.As(err, &permErr) { + t.Fatalf("expected *errs.PermissionError, got %T: %v", err, err) + } + if permErr.Subtype != errs.SubtypeMissingScope { + t.Fatalf("Subtype = %q, want %q", permErr.Subtype, errs.SubtypeMissingScope) + } + for _, scope := range []string{"drive:file:upload", "space:folder:create"} { + found := false + for _, missing := range permErr.MissingScopes { + if missing == scope { + found = true + break + } + } + if !found { + t.Fatalf("MissingScopes = %v, want %s", permErr.MissingScopes, scope) + } + } + if strings.Contains(stdout.String(), "folder_root") { + t.Fatalf("preflight should fail before remote listing, got stdout: %s", stdout.String()) } } @@ -2552,30 +2608,6 @@ func TestDriveSyncAskConflictRemoteShortForms(t *testing.T) { } } -// TestDriveSyncNeedsDownloadScopeReturnsFalseForLocalWinsOnly verifies -// that driveSyncNeedsDownloadScope returns false when there are no -// new_remote entries and all modified entries resolve to local-wins. -func TestDriveSyncNeedsDownloadScopeReturnsFalseForLocalWinsOnly(t *testing.T) { - modified := []driveStatusEntry{{RelPath: "a.txt"}, {RelPath: "b.txt"}} - resolutions := map[string]string{"a.txt": driveSyncOnConflictLocalWins, "b.txt": driveSyncOnConflictLocalWins} - - if driveSyncNeedsDownloadScope(nil, modified, resolutions) { - t.Fatal("expected false when no new_remote and all conflicts are local-wins") - } -} - -// TestDriveSyncNeedsDownloadScopeReturnsTrueForKeepBoth verifies that -// driveSyncNeedsDownloadScope returns true when a modified entry resolves -// to keep-both (which requires pulling the remote version). -func TestDriveSyncNeedsDownloadScopeReturnsTrueForKeepBoth(t *testing.T) { - modified := []driveStatusEntry{{RelPath: "a.txt"}} - resolutions := map[string]string{"a.txt": driveSyncOnConflictKeepBoth} - - if !driveSyncNeedsDownloadScope(nil, modified, resolutions) { - t.Fatal("expected true when a conflict resolves to keep-both") - } -} - // TestDriveSyncRemoteWinsReportsMissingPullView verifies that when a // modified file's rel_path is not in pullRemoteFiles during the // remote-wins branch, a failed item is reported instead of a panic. @@ -3083,3 +3115,19 @@ func driveSyncStdoutItems(t *testing.T, stdout []byte) []driveSyncItem { } return envelope.Data.Items } + +func driveSyncStdoutSummary(t *testing.T, stdout []byte) map[string]interface{} { + t.Helper() + var envelope struct { + Data struct { + Summary map[string]interface{} `json:"summary"` + } `json:"data"` + } + if err := json.Unmarshal(stdout, &envelope); err != nil { + t.Fatalf("unmarshal stdout: %v\nraw=%s", err, string(stdout)) + } + if envelope.Data.Summary == nil { + t.Fatalf("stdout missing data.summary; raw=%s", string(stdout)) + } + return envelope.Data.Summary +}