diff --git a/.golangci.yml b/.golangci.yml index 9428d7a2b..4e3b6c42f 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -73,20 +73,20 @@ linters: - forbidigo # errs-typed-only enforced on paths already migrated to errs.NewXxxError. # Add a path when its migration is complete. - - path-except: (internal/auth/|internal/errcompat/|internal/errclass/|internal/client/|internal/cmdutil/factory\.go|cmd/auth/|cmd/config/|cmd/service/|shortcuts/common/mcp_client\.go|shortcuts/base/|shortcuts/calendar/|shortcuts/contact/|shortcuts/doc/|shortcuts/drive/|shortcuts/im/|shortcuts/mail/|shortcuts/minutes/|shortcuts/okr/|shortcuts/sheets/|shortcuts/task/|shortcuts/vc/|shortcuts/whiteboard/|internal/event/consume/|cmd/event/|events/|shortcuts/event/) + - path-except: (internal/auth/|internal/errcompat/|internal/errclass/|internal/client/|internal/cmdutil/factory\.go|cmd/auth/|cmd/config/|cmd/service/|shortcuts/common/mcp_client\.go|shortcuts/base/|shortcuts/calendar/|shortcuts/contact/|shortcuts/doc/|shortcuts/drive/|shortcuts/im/|shortcuts/mail/|shortcuts/minutes/|shortcuts/okr/|shortcuts/sheets/|shortcuts/slides/|shortcuts/task/|shortcuts/vc/|shortcuts/whiteboard/|internal/event/consume/|cmd/event/|events/|shortcuts/event/) text: errs-typed-only linters: - forbidigo # errs-no-bare-wrap enforced on paths fully migrated to typed final # errors. Scoped separately from errs-typed-only because cmd/auth/, # cmd/config/ still have residual fmt.Errorf and must not be caught. - - path-except: (shortcuts/base/|shortcuts/calendar/|shortcuts/contact/|shortcuts/doc/|shortcuts/drive/|shortcuts/im/|shortcuts/mail/|shortcuts/minutes/|shortcuts/okr/|shortcuts/sheets/|shortcuts/task/|shortcuts/vc/|shortcuts/whiteboard/|shortcuts/common/mcp_client\.go|cmd/event/|events/|shortcuts/event/) + - path-except: (shortcuts/base/|shortcuts/calendar/|shortcuts/contact/|shortcuts/doc/|shortcuts/drive/|shortcuts/im/|shortcuts/mail/|shortcuts/minutes/|shortcuts/okr/|shortcuts/sheets/|shortcuts/slides/|shortcuts/task/|shortcuts/vc/|shortcuts/whiteboard/|shortcuts/common/mcp_client\.go|cmd/event/|events/|shortcuts/event/) text: errs-no-bare-wrap linters: - forbidigo # errs-no-legacy-helper enforced on domains whose shared validation/save # helpers have migrated to typed final errors. - - path-except: (shortcuts/base/|shortcuts/calendar/|shortcuts/contact/|shortcuts/doc/|shortcuts/drive/|shortcuts/im/|shortcuts/mail/|shortcuts/minutes/|shortcuts/okr/|shortcuts/sheets/|shortcuts/task/|shortcuts/vc/|shortcuts/whiteboard/|cmd/event/|events/|shortcuts/event/) + - path-except: (shortcuts/base/|shortcuts/calendar/|shortcuts/contact/|shortcuts/doc/|shortcuts/drive/|shortcuts/im/|shortcuts/mail/|shortcuts/minutes/|shortcuts/okr/|shortcuts/sheets/|shortcuts/slides/|shortcuts/task/|shortcuts/vc/|shortcuts/whiteboard/|cmd/event/|events/|shortcuts/event/) text: errs-no-legacy-helper linters: - forbidigo diff --git a/lint/errscontract/rule_no_legacy_common_helper_call.go b/lint/errscontract/rule_no_legacy_common_helper_call.go index 742b506dd..5f096bc56 100644 --- a/lint/errscontract/rule_no_legacy_common_helper_call.go +++ b/lint/errscontract/rule_no_legacy_common_helper_call.go @@ -28,6 +28,7 @@ var migratedCommonHelperPaths = []string{ "shortcuts/minutes/", "shortcuts/okr/", "shortcuts/sheets/", + "shortcuts/slides/", "shortcuts/task/", "shortcuts/vc/", "shortcuts/whiteboard/", diff --git a/lint/errscontract/rule_no_legacy_envelope_literal.go b/lint/errscontract/rule_no_legacy_envelope_literal.go index e7bba084c..ef2a3e167 100644 --- a/lint/errscontract/rule_no_legacy_envelope_literal.go +++ b/lint/errscontract/rule_no_legacy_envelope_literal.go @@ -29,6 +29,7 @@ var migratedEnvelopePaths = []string{ "shortcuts/minutes/", "shortcuts/okr/", "shortcuts/sheets/", + "shortcuts/slides/", "shortcuts/task/", "shortcuts/vc/", "shortcuts/whiteboard/", diff --git a/lint/errscontract/rules_test.go b/lint/errscontract/rules_test.go index 22a7b396c..94cf58113 100644 --- a/lint/errscontract/rules_test.go +++ b/lint/errscontract/rules_test.go @@ -955,6 +955,7 @@ func TestCheckNoLegacyCommonHelperCall_RejectsLegacyHelpersOnMigratedPath(t *tes "shortcuts/mail/mail_send.go", "shortcuts/okr/okr_progress_create.go", "shortcuts/sheets/helpers.go", + "shortcuts/slides/slides_create.go", "shortcuts/task/task_update.go", "shortcuts/whiteboard/whiteboard_query.go", } @@ -1039,6 +1040,23 @@ func boom() { } } +func TestCheckNoLegacyCommonHelperCall_CoversSlidesPathWithAliasAndFunctionValue(t *testing.T) { + src := `package migrated + +import c "github.com/larksuite/cli/shortcuts/common" + +func boom() { + f := c.FlagErrorf + _ = f + c.WrapInputStatError(nil) +} +` + v := CheckNoLegacyCommonHelperCall("shortcuts/slides/slides_create.go", src) + if len(v) != 2 { + t.Fatalf("expected 2 violations for aliased/function-value legacy helpers on slides path, got %d: %+v", len(v), v) + } +} + func TestCheckNoLegacyCommonHelperCall_AllowsNonMigratedPath(t *testing.T) { src := `package contact diff --git a/shortcuts/slides/helpers.go b/shortcuts/slides/helpers.go index c8722b209..2c2212264 100644 --- a/shortcuts/slides/helpers.go +++ b/shortcuts/slides/helpers.go @@ -9,7 +9,7 @@ import ( "regexp" "strings" - "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/shortcuts/common" ) @@ -30,7 +30,7 @@ type presentationRef struct { func parsePresentationRef(input string) (presentationRef, error) { raw := strings.TrimSpace(input) if raw == "" { - return presentationRef{}, output.ErrValidation("--presentation cannot be empty") + return presentationRef{}, errs.NewValidationError(errs.SubtypeInvalidArgument, "--presentation cannot be empty").WithParam("--presentation") } // URL inputs: parse properly and only honor /slides/ or /wiki/ when they // appear as a prefix of the URL path. Substring matching previously let @@ -38,7 +38,7 @@ func parsePresentationRef(input string) (presentationRef, error) { if strings.Contains(raw, "://") { u, err := url.Parse(raw) if err != nil || u.Path == "" { - return presentationRef{}, output.ErrValidation("unsupported --presentation input %q: use an xml_presentation_id, a /slides/ URL, or a /wiki/ URL", raw) + return presentationRef{}, errs.NewValidationError(errs.SubtypeInvalidArgument, "unsupported --presentation input %q: use an xml_presentation_id, a /slides/ URL, or a /wiki/ URL", raw).WithParam("--presentation") } if token, ok := tokenAfterPathPrefix(u.Path, "/slides/"); ok { return presentationRef{Kind: "slides", Token: token}, nil @@ -46,13 +46,13 @@ func parsePresentationRef(input string) (presentationRef, error) { if token, ok := tokenAfterPathPrefix(u.Path, "/wiki/"); ok { return presentationRef{Kind: "wiki", Token: token}, nil } - return presentationRef{}, output.ErrValidation("unsupported --presentation input %q: use an xml_presentation_id, a /slides/ URL, or a /wiki/ URL", raw) + return presentationRef{}, errs.NewValidationError(errs.SubtypeInvalidArgument, "unsupported --presentation input %q: use an xml_presentation_id, a /slides/ URL, or a /wiki/ URL", raw).WithParam("--presentation") } // Non-URL input must be a bare token — anything with path/query/fragment // chars is rejected so partial-path inputs like `tmp/wiki/wikcn123` don't // get silently accepted. if strings.ContainsAny(raw, "/?#") { - return presentationRef{}, output.ErrValidation("unsupported --presentation input %q: use an xml_presentation_id, a /slides/ URL, or a /wiki/ URL", raw) + return presentationRef{}, errs.NewValidationError(errs.SubtypeInvalidArgument, "unsupported --presentation input %q: use an xml_presentation_id, a /slides/ URL, or a /wiki/ URL", raw).WithParam("--presentation") } return presentationRef{Kind: "slides", Token: raw}, nil } @@ -82,7 +82,7 @@ func resolvePresentationID(runtime *common.RuntimeContext, ref presentationRef) case "slides": return ref.Token, nil case "wiki": - data, err := runtime.CallAPI( + data, err := runtime.CallAPITyped( "GET", "/open-apis/wiki/v2/spaces/get_node", map[string]interface{}{"token": ref.Token}, @@ -95,14 +95,18 @@ func resolvePresentationID(runtime *common.RuntimeContext, ref presentationRef) objType := common.GetString(node, "obj_type") objToken := common.GetString(node, "obj_token") if objType == "" || objToken == "" { - return "", output.Errorf(output.ExitAPI, "api_error", "wiki get_node returned incomplete node data") + return "", errs.NewInternalError(errs.SubtypeInvalidResponse, "wiki get_node returned incomplete node data") } if objType != "slides" { - return "", output.ErrValidation("wiki resolved to %q, but slides shortcuts require a slides presentation", objType) + return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "wiki resolved to %q, but slides shortcuts require a slides presentation", objType).WithParam("--presentation") } return objToken, nil default: - return "", output.ErrValidation("unsupported presentation ref kind %q", ref.Kind) + // Unreachable: ref.Kind is set only by parsePresentationRef, which + // emits exclusively "slides" or "wiki". A hit here means an internal + // invariant broke (e.g. a new kind added without updating this switch), + // not bad user input — classify as internal, not validation. + return "", errs.NewInternalError(errs.SubtypeUnknown, "unsupported presentation ref kind %q", ref.Kind) } } @@ -191,7 +195,7 @@ var xmlIdAttrRegex = regexp.MustCompile(`(?s)(?:^|\s)id\s*=\s*(["'])(.*?)(["'])` func ensureXMLRootID(xmlFragment, want string) (string, error) { m := xmlRootOpenTagRegex.FindStringSubmatchIndex(xmlFragment) if m == nil { - return "", fmt.Errorf("no root element found in XML fragment") + return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "no root element found in XML fragment") } prefix := xmlFragment[m[2]:m[3]] tagName := xmlFragment[m[4]:m[5]] diff --git a/shortcuts/slides/slides_create.go b/shortcuts/slides/slides_create.go index b54deb02d..115f04cd1 100644 --- a/shortcuts/slides/slides_create.go +++ b/shortcuts/slides/slides_create.go @@ -10,7 +10,7 @@ import ( "path/filepath" "strings" - "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/validate" "github.com/larksuite/cli/shortcuts/common" ) @@ -45,24 +45,24 @@ var SlidesCreate = common.Shortcut{ if slidesStr := runtime.Str("slides"); slidesStr != "" { var slides []string if err := json.Unmarshal([]byte(slidesStr), &slides); err != nil { - return common.FlagErrorf("--slides invalid JSON, must be an array of XML strings") + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--slides invalid JSON, must be an array of XML strings").WithParam("--slides") } if len(slides) > maxSlidesPerCreate { - return common.FlagErrorf("--slides array exceeds maximum of %d slides; create the presentation first, then add slides via xml_presentation.slide.create", maxSlidesPerCreate) + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--slides array exceeds maximum of %d slides; create the presentation first, then add slides via xml_presentation.slide.create", maxSlidesPerCreate).WithParam("--slides") } // Validate placeholder paths up front so we don't create a presentation // only to fail mid-way on a missing local file. for _, path := range extractImagePlaceholderPaths(slides) { stat, err := runtime.FileIO().Stat(path) if err != nil { - return common.WrapInputStatError(err, fmt.Sprintf("--slides @%s: file not found", path)) + return slidesInputStatError(err, "--slides", fmt.Sprintf("--slides @%s: file not found", path)) } if !stat.Mode().IsRegular() { - return common.FlagErrorf("--slides @%s: must be a regular file", path) + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--slides @%s: must be a regular file", path).WithParam("--slides") } if stat.Size() > common.MaxDriveMediaUploadSinglePartSize { - return common.FlagErrorf("--slides @%s: file size %s exceeds 20 MB limit for slides image upload", - path, common.FormatSize(stat.Size())) + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--slides @%s: file size %s exceeds 20 MB limit for slides image upload", + path, common.FormatSize(stat.Size())).WithParam("--slides") } } } @@ -128,7 +128,7 @@ var SlidesCreate = common.Shortcut{ slidesStr := runtime.Str("slides") // Step 1: Create presentation - data, err := runtime.CallAPI( + data, err := runtime.CallAPITyped( "POST", "/open-apis/slides_ai/v1/xml_presentations", nil, @@ -144,7 +144,7 @@ var SlidesCreate = common.Shortcut{ presentationID := common.GetString(data, "xml_presentation_id") if presentationID == "" { - return output.Errorf(output.ExitAPI, "api_error", "slides create returned no xml_presentation_id") + return errs.NewInternalError(errs.SubtypeInvalidResponse, "slides create returned no xml_presentation_id") } result := map[string]interface{}{ @@ -168,9 +168,7 @@ var SlidesCreate = common.Shortcut{ if len(placeholders) > 0 { tokens, uploaded, err := uploadSlidesPlaceholders(runtime, presentationID, placeholders) if err != nil { - return output.Errorf(output.ExitAPI, "api_error", - "image upload failed: %v (presentation %s was created; %d image(s) uploaded before failure)", - err, presentationID, uploaded) + return appendSlidesProgressHint(err, fmt.Sprintf("presentation %s was created; %d image(s) uploaded before failure", presentationID, uploaded)) } for i := range slides { slides[i] = replaceImagePlaceholders(slides[i], tokens) @@ -185,7 +183,7 @@ var SlidesCreate = common.Shortcut{ var slideIDs []string for i, slideXML := range slides { - slideData, err := runtime.CallAPI( + slideData, err := runtime.CallAPITyped( "POST", slideURL, map[string]interface{}{"revision_id": -1}, @@ -194,9 +192,7 @@ var SlidesCreate = common.Shortcut{ }, ) if err != nil { - return output.Errorf(output.ExitAPI, "api_error", - "slide %d/%d failed: %v (presentation %s was created; %d slide(s) added before failure)", - i+1, len(slides), err, presentationID, i) + return appendSlidesProgressHint(err, fmt.Sprintf("adding slide %d/%d failed; presentation %s was created, %d slide(s) added before failure", i+1, len(slides), presentationID, i)) } if sid := common.GetString(slideData, "slide_id"); sid != "" { slideIDs = append(slideIDs, sid) @@ -256,10 +252,10 @@ func uploadSlidesPlaceholders(runtime *common.RuntimeContext, presentationID str for i, path := range paths { stat, err := runtime.FileIO().Stat(path) if err != nil { - return tokens, i, common.WrapInputStatError(err, fmt.Sprintf("@%s: file not found", path)) + return tokens, i, slidesInputStatError(err, "--slides", fmt.Sprintf("@%s: file not found", path)) } if !stat.Mode().IsRegular() { - return tokens, i, output.ErrValidation("@%s: must be a regular file", path) + return tokens, i, errs.NewValidationError(errs.SubtypeInvalidArgument, "@%s: must be a regular file", path).WithParam("--slides") } fileName := filepath.Base(path) fmt.Fprintf(runtime.IO().ErrOut, "Uploading image %d/%d: %s (%s)\n", @@ -267,7 +263,7 @@ func uploadSlidesPlaceholders(runtime *common.RuntimeContext, presentationID str token, err := uploadSlidesMedia(runtime, path, fileName, stat.Size(), presentationID) if err != nil { - return tokens, i, fmt.Errorf("@%s: %w", path, err) + return tokens, i, fmt.Errorf("@%s: %w", path, err) //nolint:forbidigo // intermediate; preserves typed cause via %w, reclassified by appendSlidesProgressHint at the call site } tokens[path] = token } diff --git a/shortcuts/slides/slides_create_test.go b/shortcuts/slides/slides_create_test.go index 2470fdfcb..36b964a07 100644 --- a/shortcuts/slides/slides_create_test.go +++ b/shortcuts/slides/slides_create_test.go @@ -6,12 +6,14 @@ package slides import ( "bytes" "encoding/json" + "errors" "os" "strings" "testing" "github.com/spf13/cobra" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/httpmock" @@ -400,15 +402,21 @@ func TestSlidesCreateWithSlidesPartialFailure(t *testing.T) { if err == nil { t.Fatal("expected error for partial failure, got nil") } - errMsg := err.Error() - if !strings.Contains(errMsg, "pres_partial") { - t.Fatalf("error should contain presentation ID, got: %s", errMsg) + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected a typed errs.* error, got %v", err) } - if !strings.Contains(errMsg, "slide 2/2") { - t.Fatalf("error should indicate slide 2/2 failed, got: %s", errMsg) + // The presentation was created but a slide add failed; the recovery hint + // carries the partial-progress context (which presentation exists, how many + // slides landed) so the caller can resume without recreating. + if !strings.Contains(p.Hint, "pres_partial") { + t.Fatalf("hint should contain presentation ID, got: %s", p.Hint) } - if !strings.Contains(errMsg, "1 slide(s) added") { - t.Fatalf("error should report 1 slide added before failure, got: %s", errMsg) + if !strings.Contains(p.Hint, "slide 2/2") { + t.Fatalf("hint should indicate slide 2/2 failed, got: %s", p.Hint) + } + if !strings.Contains(p.Hint, "1 slide(s) added") { + t.Fatalf("hint should report 1 slide added before failure, got: %s", p.Hint) } } @@ -457,6 +465,71 @@ func TestSlidesCreateWithSlidesExceedsMax(t *testing.T) { } } +// TestSlidesCreateValidationParam locks Param=="--slides" on the pure +// validation rejections, so callers route on the typed field rather than the +// message. +func TestSlidesCreateValidationParam(t *testing.T) { + t.Parallel() + + elems := make([]string, 11) + for i := range elems { + elems[i] = `""` + } + exceedsMax := "[" + strings.Join(elems, ",") + "]" + + tests := []struct { + name string + slides string + }{ + {"invalid JSON", "not json"}, + {"exceeds max", exceedsMax}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + f, stdout, _, _ := cmdutil.TestFactory(t, slidesTestConfig(t, "")) + err := runSlidesCreateShortcut(t, f, stdout, []string{ + "+create", + "--slides", tt.slides, + "--as", "user", + }) + + var ve *errs.ValidationError + if !errors.As(err, &ve) { + t.Fatalf("err = %v, want *errs.ValidationError", err) + } + if ve.Param != "--slides" { + t.Fatalf("Param = %q, want --slides", ve.Param) + } + }) + } +} + +// TestSlidesCreatePlaceholderMissingParam guards the create.go caller wiring: +// a missing @-placeholder file must surface a --slides-tagged validation error +// through the shared slidesInputStatError helper. +func TestSlidesCreatePlaceholderMissingParam(t *testing.T) { + dir := t.TempDir() + withSlidesTestWorkingDir(t, dir) + + f, stdout, _, _ := cmdutil.TestFactory(t, slidesTestConfig(t, "")) + slidesJSON := `[""]` + err := runSlidesCreateShortcut(t, f, stdout, []string{ + "+create", + "--slides", slidesJSON, + "--as", "user", + }) + + var ve *errs.ValidationError + if !errors.As(err, &ve) { + t.Fatalf("err = %v, want *errs.ValidationError", err) + } + if ve.Param != "--slides" { + t.Fatalf("Param = %q, want --slides", ve.Param) + } +} + // TestSlidesCreateWithSlidesEmptyArray verifies that --slides '[]' behaves like no --slides. func TestSlidesCreateWithSlidesEmptyArray(t *testing.T) { t.Parallel() diff --git a/shortcuts/slides/slides_errors.go b/shortcuts/slides/slides_errors.go new file mode 100644 index 000000000..9856cf1a1 --- /dev/null +++ b/shortcuts/slides/slides_errors.go @@ -0,0 +1,50 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package slides + +import ( + "errors" + + "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/extension/fileio" +) + +// slidesInputStatError maps a FileIO.Stat error for an input image path to a +// typed validation error, prefixing the caller's context message and tagging +// the offending flag via param so callers route on the typed Param rather than +// parsing the message. Both path validation failures and other stat errors are +// user-actionable input problems (exit code 2). Already-typed errors are not +// expected here (Stat returns raw fs errors), so this always classifies as +// validation. +func slidesInputStatError(err error, param, msg string) error { + if err == nil { + return nil + } + if errors.Is(err, fileio.ErrPathValidation) { + return errs.NewValidationError(errs.SubtypeInvalidArgument, "%s: unsafe file path: %s", msg, err).WithParam(param).WithCause(err) + } + return errs.NewValidationError(errs.SubtypeInvalidArgument, "%s: %s", msg, err).WithParam(param).WithCause(err) +} + +// appendSlidesProgressHint preserves err's typed classification (per +// ERROR_CONTRACT.md "propagate typed errors unchanged") and appends an +// orchestration-progress hint — e.g. "presentation was created; N image(s) +// uploaded before failure" — so a failure mid-sequence still tells the caller +// what partial state exists. An unclassified error (e.g. surfaced from a shared +// helper boundary before it can be classified) falls back to a typed internal +// error carrying the hint. +func appendSlidesProgressHint(err error, hint string) error { + if err == nil { + return nil + } + if p, ok := errs.ProblemOf(err); ok { + if p.Hint != "" { + p.Hint = p.Hint + "\n" + hint + } else { + p.Hint = hint + } + return err + } + return errs.NewInternalError(errs.SubtypeUnknown, "%s", err.Error()).WithHint(hint).WithCause(err) +} diff --git a/shortcuts/slides/slides_errors_test.go b/shortcuts/slides/slides_errors_test.go new file mode 100644 index 000000000..f175402e0 --- /dev/null +++ b/shortcuts/slides/slides_errors_test.go @@ -0,0 +1,111 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package slides + +import ( + "errors" + "testing" + + "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/extension/fileio" +) + +// TestSlidesInputStatError verifies the shared stat-error helper tags the +// offending flag via the typed Param — so callers route on the structured +// field rather than parsing the message — and always classifies as a +// validation error while preserving the underlying cause. +func TestSlidesInputStatError(t *testing.T) { + t.Parallel() + + if err := slidesInputStatError(nil, "--slides", "ctx"); err != nil { + t.Fatalf("nil input should return nil, got %v", err) + } + + tests := []struct { + name string + in error + }{ + {"path validation", fileio.ErrPathValidation}, + {"generic stat error", errors.New("permission denied")}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + err := slidesInputStatError(tt.in, "--file", "file not found") + + var ve *errs.ValidationError + if !errors.As(err, &ve) { + t.Fatalf("err = %v, want *errs.ValidationError", err) + } + if ve.Param != "--file" { + t.Fatalf("Param = %q, want --file", ve.Param) + } + if !errors.Is(err, tt.in) { + t.Fatalf("err must wrap the underlying cause %v", tt.in) + } + }) + } +} + +// TestAppendSlidesProgressHint covers both branches of the orchestration-hint +// helper: a typed error keeps its classification and gains (or extends) the +// progress hint, while an unclassified error surfaced from a shared-helper +// boundary falls back to a typed internal error that still carries the hint +// and the original cause. +func TestAppendSlidesProgressHint(t *testing.T) { + t.Parallel() + + if err := appendSlidesProgressHint(nil, "hint"); err != nil { + t.Fatalf("nil input should return nil, got %v", err) + } + + t.Run("typed error preserves classification and sets hint", func(t *testing.T) { + t.Parallel() + base := errs.NewValidationError(errs.SubtypeInvalidArgument, "bad input") + err := appendSlidesProgressHint(base, "2 image(s) uploaded before failure") + + var ve *errs.ValidationError + if !errors.As(err, &ve) { + t.Fatalf("err = %v, want classification preserved as *errs.ValidationError", err) + } + p, _ := errs.ProblemOf(err) + if p.Hint != "2 image(s) uploaded before failure" { + t.Fatalf("Hint = %q, want the progress hint", p.Hint) + } + }) + + t.Run("typed error appends to an existing hint", func(t *testing.T) { + t.Parallel() + base := errs.NewValidationError(errs.SubtypeInvalidArgument, "bad input").WithHint("first") + err := appendSlidesProgressHint(base, "second") + + p, _ := errs.ProblemOf(err) + if p.Hint != "first\nsecond" { + t.Fatalf("Hint = %q, want %q", p.Hint, "first\nsecond") + } + }) + + t.Run("unclassified error falls back to typed internal error", func(t *testing.T) { + t.Parallel() + cause := errors.New("raw boundary error") + err := appendSlidesProgressHint(cause, "presentation was created") + + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("err = %v, want a typed errs.* error", err) + } + if p.Category != errs.CategoryInternal { + t.Fatalf("Category = %v, want CategoryInternal", p.Category) + } + if p.Subtype != errs.SubtypeUnknown { + t.Fatalf("Subtype = %v, want SubtypeUnknown", p.Subtype) + } + if p.Hint != "presentation was created" { + t.Fatalf("Hint = %q, want the progress hint", p.Hint) + } + if !errors.Is(err, cause) { + t.Fatalf("fallback must preserve the original cause via WithCause") + } + }) +} diff --git a/shortcuts/slides/slides_media_upload.go b/shortcuts/slides/slides_media_upload.go index ebf086527..8834cd0a9 100644 --- a/shortcuts/slides/slides_media_upload.go +++ b/shortcuts/slides/slides_media_upload.go @@ -8,7 +8,7 @@ import ( "fmt" "path/filepath" - "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/shortcuts/common" ) @@ -86,15 +86,15 @@ var SlidesMediaUpload = common.Shortcut{ stat, err := runtime.FileIO().Stat(filePath) if err != nil { - return common.WrapInputStatError(err, "file not found") + return slidesInputStatError(err, "--file", "file not found") } if !stat.Mode().IsRegular() { - return output.ErrValidation("file must be a regular file: %s", filePath) + return errs.NewValidationError(errs.SubtypeInvalidArgument, "file must be a regular file: %s", filePath).WithParam("--file") } if stat.Size() > common.MaxDriveMediaUploadSinglePartSize { - return output.ErrValidation("file %s is %s, exceeds 20 MB limit for slides image upload", - filepath.Base(filePath), common.FormatSize(stat.Size())) + return errs.NewValidationError(errs.SubtypeInvalidArgument, "file %s is %s, exceeds 20 MB limit for slides image upload", + filepath.Base(filePath), common.FormatSize(stat.Size())).WithParam("--file") } fileName := filepath.Base(filePath) @@ -124,7 +124,7 @@ var SlidesMediaUpload = common.Shortcut{ // because the multipart upload API does not accept parent_type=slide_file. func uploadSlidesMedia(runtime *common.RuntimeContext, filePath, fileName string, fileSize int64, presentationID string) (string, error) { if fileSize > common.MaxDriveMediaUploadSinglePartSize { - return "", output.ErrValidation("file %s is %s, exceeds 20 MB limit for slides image upload", + return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "file %s is %s, exceeds 20 MB limit for slides image upload", fileName, common.FormatSize(fileSize)) } parent := presentationID diff --git a/shortcuts/slides/slides_media_upload_test.go b/shortcuts/slides/slides_media_upload_test.go index c78d9ea14..a46205bb4 100644 --- a/shortcuts/slides/slides_media_upload_test.go +++ b/shortcuts/slides/slides_media_upload_test.go @@ -6,6 +6,7 @@ package slides import ( "bytes" "encoding/json" + "errors" "mime" "mime/multipart" "os" @@ -14,6 +15,7 @@ import ( "github.com/spf13/cobra" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/httpmock" "github.com/larksuite/cli/shortcuts/common" @@ -215,6 +217,14 @@ func TestSlidesMediaUploadFileNotFound(t *testing.T) { if !strings.Contains(err.Error(), "file not found") && !strings.Contains(err.Error(), "no such file") { t.Fatalf("err = %v, want file-not-found error", err) } + + var ve *errs.ValidationError + if !errors.As(err, &ve) { + t.Fatalf("err = %v, want *errs.ValidationError", err) + } + if ve.Param != "--file" { + t.Fatalf("Param = %q, want --file", ve.Param) + } } // TestSlidesMediaUploadInvalidPresentation verifies validation rejects a bad ref. diff --git a/shortcuts/slides/slides_replace_slide.go b/shortcuts/slides/slides_replace_slide.go index c576ce987..a69107288 100644 --- a/shortcuts/slides/slides_replace_slide.go +++ b/shortcuts/slides/slides_replace_slide.go @@ -6,11 +6,10 @@ package slides import ( "context" "encoding/json" - "errors" "fmt" "strings" - "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/validate" "github.com/larksuite/cli/shortcuts/common" ) @@ -58,7 +57,7 @@ var SlidesReplaceSlide = common.Shortcut{ return err } if strings.TrimSpace(runtime.Str("slide-id")) == "" { - return common.FlagErrorf("--slide-id cannot be empty") + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--slide-id cannot be empty").WithParam("--slide-id") } parts, err := parseReplaceParts(runtime.Str("parts")) if err != nil { @@ -153,7 +152,7 @@ var SlidesReplaceSlide = common.Shortcut{ "/open-apis/slides_ai/v1/xml_presentations/%s/slide/replace", validate.EncodePathSegment(presentationID), ) - data, err := runtime.CallAPI("POST", url, query, body) + data, err := runtime.CallAPITyped("POST", url, query, body) if err != nil { return enrichSlidesReplaceError(err) } @@ -201,11 +200,11 @@ type replacePart struct { func parseReplaceParts(raw string) ([]replacePart, error) { s := strings.TrimSpace(raw) if s == "" { - return nil, common.FlagErrorf("--parts cannot be empty") + return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts cannot be empty").WithParam("--parts") } var decoded []map[string]interface{} if err := json.Unmarshal([]byte(s), &decoded); err != nil { - return nil, common.FlagErrorf("--parts invalid JSON, must be an array of objects: %v", err) + return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts invalid JSON, must be an array of objects: %v", err).WithParam("--parts").WithCause(err) } out := make([]replacePart, 0, len(decoded)) for i, m := range decoded { @@ -213,35 +212,35 @@ func parseReplaceParts(raw string) ([]replacePart, error) { if v, ok := m["action"]; ok { s, ok := v.(string) if !ok { - return nil, common.FlagErrorf("--parts[%d].action must be a string", i) + return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d].action must be a string", i).WithParam("--parts") } p.Action = s } if v, ok := m["replacement"]; ok { s, ok := v.(string) if !ok { - return nil, common.FlagErrorf("--parts[%d].replacement must be a string", i) + return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d].replacement must be a string", i).WithParam("--parts") } p.Replacement = &s } if v, ok := m["block_id"]; ok { s, ok := v.(string) if !ok { - return nil, common.FlagErrorf("--parts[%d].block_id must be a string", i) + return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d].block_id must be a string", i).WithParam("--parts") } p.BlockID = &s } if v, ok := m["insertion"]; ok { s, ok := v.(string) if !ok { - return nil, common.FlagErrorf("--parts[%d].insertion must be a string", i) + return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d].insertion must be a string", i).WithParam("--parts") } p.Insertion = &s } if v, ok := m["insert_before_block_id"]; ok { s, ok := v.(string) if !ok { - return nil, common.FlagErrorf("--parts[%d].insert_before_block_id must be a string", i) + return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d].insert_before_block_id must be a string", i).WithParam("--parts") } p.InsertBeforeBlockID = &s } @@ -261,17 +260,18 @@ const slides3350001Hint = "common causes: (1) block_id not found in current slid // enrichSlidesReplaceError attaches slides3350001Hint when the API returns // 3350001 (invalid param). Other error codes pass through untouched. func enrichSlidesReplaceError(err error) error { - var exitErr *output.ExitError - if !errors.As(err, &exitErr) || exitErr.Detail == nil || exitErr.Detail.Code != larkCodeSlidesInvalidParam { + p, ok := errs.ProblemOf(err) + if !ok || p.Code != larkCodeSlidesInvalidParam { return err } // Only fall back to the generic checklist when no upstream hint is // already attached — don't clobber a more specific hint set by the - // backend or an earlier wrapper. - if exitErr.Detail.Hint == "" { - exitErr.Detail.Hint = slides3350001Hint + // backend or an earlier wrapper. p points at the embedded Problem, so + // the mutation is reflected in the returned err. + if p.Hint == "" { + p.Hint = slides3350001Hint } - return exitErr + return err } // validateReplaceParts enforces CLI-level invariants: @@ -280,33 +280,33 @@ func enrichSlidesReplaceError(err error) error { // - per-action required fields are present func validateReplaceParts(parts []replacePart) error { if len(parts) == 0 { - return common.FlagErrorf("--parts must contain at least 1 item") + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts must contain at least 1 item").WithParam("--parts") } if len(parts) > maxReplaceParts { - return common.FlagErrorf("--parts contains %d items, exceeds maximum of %d", len(parts), maxReplaceParts) + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts contains %d items, exceeds maximum of %d", len(parts), maxReplaceParts).WithParam("--parts") } for i, p := range parts { switch p.Action { case "block_replace": if p.BlockID == nil || strings.TrimSpace(*p.BlockID) == "" { - return common.FlagErrorf("--parts[%d] (block_replace) requires non-empty block_id", i) + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d] (block_replace) requires non-empty block_id", i).WithParam("--parts") } if p.Replacement == nil || strings.TrimSpace(*p.Replacement) == "" { - return common.FlagErrorf("--parts[%d] (block_replace) requires non-empty replacement", i) + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d] (block_replace) requires non-empty replacement", i).WithParam("--parts") } case "block_insert": if p.Insertion == nil || strings.TrimSpace(*p.Insertion) == "" { - return common.FlagErrorf("--parts[%d] (block_insert) requires non-empty insertion", i) + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d] (block_insert) requires non-empty insertion", i).WithParam("--parts") } case "str_replace": // Backend still accepts str_replace, but product decision is to // force structural edits through the CLI. Block it up-front so // users don't build tooling around an option we won't keep. - return common.FlagErrorf("--parts[%d] action %q is not supported by this shortcut; use block_replace or block_insert", i, p.Action) + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d] action %q is not supported by this shortcut; use block_replace or block_insert", i, p.Action).WithParam("--parts") case "": - return common.FlagErrorf("--parts[%d].action is required", i) + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d].action is required", i).WithParam("--parts") default: - return common.FlagErrorf("--parts[%d] unknown action %q, supported: block_replace, block_insert", i, p.Action) + return errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d] unknown action %q, supported: block_replace, block_insert", i, p.Action).WithParam("--parts") } } return nil @@ -327,7 +327,7 @@ func injectBlockReplaceIDs(parts []replacePart) ([]map[string]interface{}, error case "block_replace": fixed, err := ensureXMLRootID(*p.Replacement, *p.BlockID) if err != nil { - return nil, output.ErrValidation("--parts[%d].replacement: %v", i, err) + return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "--parts[%d].replacement: %v", i, err).WithParam("--parts").WithCause(err) } fixed = ensureShapeHasContent(fixed) m["block_id"] = *p.BlockID diff --git a/shortcuts/slides/slides_replace_slide_test.go b/shortcuts/slides/slides_replace_slide_test.go index 7ed4d9155..a4c37db39 100644 --- a/shortcuts/slides/slides_replace_slide_test.go +++ b/shortcuts/slides/slides_replace_slide_test.go @@ -10,9 +10,9 @@ import ( "strings" "testing" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/httpmock" - "github.com/larksuite/cli/internal/output" ) // TestReplaceSlideBlockReplaceInjectsID is the core regression: users write @@ -631,15 +631,15 @@ func TestReplaceSlide3350001ErrorEnrichment(t *testing.T) { if err == nil { t.Fatal("expected error for 3350001") } - var exitErr *output.ExitError - if !errors.As(err, &exitErr) || exitErr.Detail == nil { - t.Fatalf("expected ExitError with Detail, got %v", err) + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected a typed errs.* error, got %v", err) } - if exitErr.Detail.Code != 3350001 { - t.Fatalf("expected code 3350001, got %d", exitErr.Detail.Code) + if p.Code != 3350001 { + t.Fatalf("expected code 3350001, got %d", p.Code) } - if !strings.Contains(exitErr.Detail.Hint, tt.wantHint) { - t.Fatalf("hint = %q, want substring %q", exitErr.Detail.Hint, tt.wantHint) + if !strings.Contains(p.Hint, tt.wantHint) { + t.Fatalf("hint = %q, want substring %q", p.Hint, tt.wantHint) } }) } @@ -670,17 +670,64 @@ func TestReplaceSlideNon3350001ErrorNotEnriched(t *testing.T) { if err == nil { t.Fatal("expected error") } - var exitErr *output.ExitError - if !errors.As(err, &exitErr) || exitErr.Detail == nil { - t.Fatalf("expected ExitError, got %v", err) + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected a typed errs.* error, got %v", err) } - if exitErr.Detail.Code != 99991672 { - t.Fatalf("expected code 99991672, got %d", exitErr.Detail.Code) + if p.Code != 99991672 { + t.Fatalf("expected code 99991672, got %d", p.Code) } // Non-3350001 errors must not have the slides-specific hint attached. // Assert the actual hint is not our 3350001 checklist, rather than a // string the hint never emits. - if strings.Contains(exitErr.Detail.Hint, "common causes") { - t.Fatalf("non-3350001 error should not get slides-specific hint, got %q", exitErr.Detail.Hint) + if strings.Contains(p.Hint, "common causes") { + t.Fatalf("non-3350001 error should not get slides-specific hint, got %q", p.Hint) + } +} + +// TestReplaceSlideValidationParam locks the structured Param on every +// +replace-slide validation error, so callers route on the typed field +// instead of parsing the message. Guards against a regression where the flag +// tag is dropped from any of the --slide-id / --parts validation branches. +func TestReplaceSlideValidationParam(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + slideID string + parts string + wantParam string + }{ + {"slide-id empty", "", `[{"action":"block_insert","insertion":""}]`, "--slide-id"}, + {"parts whitespace-only", "s", " ", "--parts"}, + {"parts invalid JSON", "s", "not-json", "--parts"}, + {"parts non-string field", "s", `[{"action":123}]`, "--parts"}, + {"parts empty array", "s", `[]`, "--parts"}, + {"parts missing required field", "s", `[{"action":"block_insert"}]`, "--parts"}, + {"parts str_replace rejected", "s", `[{"action":"str_replace","pattern":"a","replacement":"b"}]`, "--parts"}, + {"parts unknown action", "s", `[{"action":"nuke","block_id":"b"}]`, "--parts"}, + {"parts replacement without root", "s", `[{"action":"block_replace","block_id":"b","replacement":"plain text"}]`, "--parts"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + f, stdout, _, _ := cmdutil.TestFactory(t, slidesTestConfig(t, "")) + err := runSlidesShortcut(t, f, stdout, SlidesReplaceSlide, []string{ + "+replace-slide", + "--presentation", "pres_abc", + "--slide-id", tt.slideID, + "--parts", tt.parts, + "--as", "user", + }) + + var ve *errs.ValidationError + if !errors.As(err, &ve) { + t.Fatalf("err = %v, want *errs.ValidationError", err) + } + if ve.Param != tt.wantParam { + t.Fatalf("Param = %q, want %q", ve.Param, tt.wantParam) + } + }) } }