diff --git a/.golangci.yml b/.golangci.yml index 1c47846272..fcd98d633d 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/drive/|shortcuts/im/|shortcuts/mail/|shortcuts/minutes/|shortcuts/okr/|shortcuts/task/|shortcuts/vc/|shortcuts/whiteboard/) + - 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/drive/|shortcuts/im/|shortcuts/mail/|shortcuts/minutes/|shortcuts/okr/|shortcuts/task/|shortcuts/vc/|shortcuts/whiteboard/) 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/drive/|shortcuts/im/|shortcuts/mail/|shortcuts/minutes/|shortcuts/okr/|shortcuts/task/|shortcuts/vc/|shortcuts/whiteboard/|shortcuts/common/mcp_client\.go) + - path-except: (shortcuts/base/|shortcuts/calendar/|shortcuts/contact/|shortcuts/drive/|shortcuts/im/|shortcuts/mail/|shortcuts/minutes/|shortcuts/okr/|shortcuts/task/|shortcuts/vc/|shortcuts/whiteboard/|shortcuts/common/mcp_client\.go) 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/drive/|shortcuts/im/|shortcuts/mail/|shortcuts/minutes/|shortcuts/okr/|shortcuts/task/|shortcuts/vc/|shortcuts/whiteboard/) + - path-except: (shortcuts/base/|shortcuts/calendar/|shortcuts/contact/|shortcuts/drive/|shortcuts/im/|shortcuts/mail/|shortcuts/minutes/|shortcuts/okr/|shortcuts/task/|shortcuts/vc/|shortcuts/whiteboard/) 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 8b21a289db..ee84462624 100644 --- a/lint/errscontract/rule_no_legacy_common_helper_call.go +++ b/lint/errscontract/rule_no_legacy_common_helper_call.go @@ -17,6 +17,7 @@ import ( var migratedCommonHelperPaths = []string{ "shortcuts/base/", "shortcuts/calendar/", + "shortcuts/contact/", "shortcuts/drive/", "shortcuts/mail/", "shortcuts/minutes/", diff --git a/lint/errscontract/rule_no_legacy_envelope_literal.go b/lint/errscontract/rule_no_legacy_envelope_literal.go index b85bee3b8d..35677f231f 100644 --- a/lint/errscontract/rule_no_legacy_envelope_literal.go +++ b/lint/errscontract/rule_no_legacy_envelope_literal.go @@ -18,6 +18,7 @@ import ( var migratedEnvelopePaths = []string{ "shortcuts/base/", "shortcuts/calendar/", + "shortcuts/contact/", "shortcuts/drive/", "shortcuts/mail/", "shortcuts/minutes/", diff --git a/lint/errscontract/rules_test.go b/lint/errscontract/rules_test.go index 2e222ab3b0..b3d199edfa 100644 --- a/lint/errscontract/rules_test.go +++ b/lint/errscontract/rules_test.go @@ -691,7 +691,7 @@ func boom() error { return &output.ExitError{Code: 1} } ` - v := CheckNoLegacyEnvelopeLiteral("shortcuts/contact/foo.go", src) + v := CheckNoLegacyEnvelopeLiteral("shortcuts/unmigrated/foo.go", src) if len(v) != 0 { t.Errorf("non-migrated path should pass, got: %+v", v) } @@ -907,7 +907,7 @@ func boom(runtime *common.RuntimeContext) error { return err } ` - v := CheckNoLegacyRuntimeAPICall("shortcuts/contact/contact_get.go", src) + v := CheckNoLegacyRuntimeAPICall("shortcuts/unmigrated/sample.go", src) if len(v) != 0 { t.Errorf("non-migrated path must not fire, got: %+v", v) } @@ -1006,7 +1006,7 @@ func boom() { common.FlagErrorf("legacy allowed until domain migrates") } ` - v := CheckNoLegacyCommonHelperCall("shortcuts/contact/contact_get.go", src) + v := CheckNoLegacyCommonHelperCall("shortcuts/unmigrated/sample.go", src) if len(v) != 0 { t.Errorf("non-migrated path must pass, got: %+v", v) } diff --git a/shortcuts/common/userids.go b/shortcuts/common/userids.go index 4ce8829e3c..b06121996a 100644 --- a/shortcuts/common/userids.go +++ b/shortcuts/common/userids.go @@ -6,24 +6,8 @@ package common import ( "fmt" "strings" - - "github.com/larksuite/cli/internal/output" ) -// ResolveOpenIDs expands the special identifier "me" to the current user's -// open_id, removes duplicates case-insensitively while preserving the -// first-occurrence form, and returns nil for an empty input. flagName is -// used in error messages to point the user at the offending CLI flag. -// -// Deprecated: use ResolveOpenIDsTyped for typed error envelopes. -func ResolveOpenIDs(flagName string, ids []string, runtime *RuntimeContext) ([]string, error) { - out, msg := resolveOpenIDs(flagName, ids, runtime) - if msg != "" { - return nil, output.ErrValidation("%s", msg) - } - return out, nil -} - // ResolveOpenIDsTyped expands the special identifier "me" to the current // user's open_id, removes duplicates case-insensitively while preserving the // first-occurrence form, and returns nil for an empty input. flagName names diff --git a/shortcuts/common/userids_test.go b/shortcuts/common/userids_test.go index 7f60c29a3d..7d5e2f76a9 100644 --- a/shortcuts/common/userids_test.go +++ b/shortcuts/common/userids_test.go @@ -17,9 +17,9 @@ func resolveOpenIDsTestRuntime(userOpenID string) *RuntimeContext { return TestNewRuntimeContext(cmd, cfg) } -func TestResolveOpenIDs_Empty(t *testing.T) { +func TestResolveOpenIDsTyped_Empty(t *testing.T) { rt := resolveOpenIDsTestRuntime("ou_self") - out, err := ResolveOpenIDs("--user-ids", nil, rt) + out, err := ResolveOpenIDsTyped("--user-ids", nil, rt) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -28,21 +28,9 @@ func TestResolveOpenIDs_Empty(t *testing.T) { } } -func TestResolveOpenIDs_ExpandsMeAndDedups(t *testing.T) { +func TestResolveOpenIDsTyped_MeIsCaseInsensitive(t *testing.T) { rt := resolveOpenIDsTestRuntime("ou_self") - out, err := ResolveOpenIDs("--user-ids", []string{"me", "ou_a", "me", "ou_a"}, rt) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - want := []string{"ou_self", "ou_a"} - if len(out) != len(want) || out[0] != want[0] || out[1] != want[1] { - t.Fatalf("got %v, want %v", out, want) - } -} - -func TestResolveOpenIDs_MeIsCaseInsensitive(t *testing.T) { - rt := resolveOpenIDsTestRuntime("ou_self") - out, err := ResolveOpenIDs("--user-ids", []string{"ou_other", "me", "Me", "ME"}, rt) + out, err := ResolveOpenIDsTyped("--user-ids", []string{"ou_other", "me", "Me", "ME"}, rt) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -52,22 +40,11 @@ func TestResolveOpenIDs_MeIsCaseInsensitive(t *testing.T) { } } -func TestResolveOpenIDs_MeWithoutLogin(t *testing.T) { - rt := resolveOpenIDsTestRuntime("") - _, err := ResolveOpenIDs("--user-ids", []string{"me"}, rt) - if err == nil { - t.Fatal("expected validation error") - } - if !strings.Contains(err.Error(), "--user-ids") { - t.Fatalf("error should mention the offending flag name; got: %v", err) - } -} - -func TestResolveOpenIDs_DedupIsCaseInsensitive(t *testing.T) { +func TestResolveOpenIDsTyped_DedupIsCaseInsensitive(t *testing.T) { rt := resolveOpenIDsTestRuntime("ou_self") // Same underlying open_id with three case variants — should collapse to // one entry, preserving the first-occurrence form. - out, err := ResolveOpenIDs("--user-ids", []string{"ou_abc123", "OU_ABC123", "Ou_Abc123"}, rt) + out, err := ResolveOpenIDsTyped("--user-ids", []string{"ou_abc123", "OU_ABC123", "Ou_Abc123"}, rt) if err != nil { t.Fatalf("unexpected error: %v", err) } diff --git a/shortcuts/common/validate_ids.go b/shortcuts/common/validate_ids.go index efc6b210ef..acc0566459 100644 --- a/shortcuts/common/validate_ids.go +++ b/shortcuts/common/validate_ids.go @@ -5,8 +5,6 @@ package common import ( "strings" - - "github.com/larksuite/cli/internal/output" ) // ValidateChatIDTyped checks if a chat ID has valid format (oc_ prefix). @@ -42,17 +40,6 @@ func normalizeChatID(input string) (string, string) { return input, "" } -// ValidateUserID checks if a user ID has valid format (ou_ prefix). -// -// Deprecated: use ValidateUserIDTyped for typed error envelopes. -func ValidateUserID(input string) (string, error) { - userID, msg := normalizeUserID(input) - if msg != "" { - return "", output.ErrValidation("%s", msg) - } - return userID, nil -} - // ValidateUserIDTyped checks if a user ID has valid format (ou_ prefix). // param names the flag being validated (e.g. "--creator-ids") and is // recorded on the typed error. diff --git a/shortcuts/contact/contact_errors.go b/shortcuts/contact/contact_errors.go new file mode 100644 index 0000000000..6edcb40664 --- /dev/null +++ b/shortcuts/contact/contact_errors.go @@ -0,0 +1,78 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package contact + +import ( + "errors" + "fmt" + "net/http" + "strings" + + "github.com/larksuite/cli/errs" +) + +const contactFanoutRetryHint = "retry the command; if it persists, narrow --queries to a single term to isolate the failing input" + +func contactInvalidResponseError(format string, args ...any) *errs.InternalError { + return errs.NewInternalError(errs.SubtypeInvalidResponse, format, args...) +} + +func contactFanoutErrorSummary(err error) string { + if p, ok := errs.ProblemOf(err); ok { + if p.Code >= 100 && p.Code < 600 { + prefix := fmt.Sprintf("HTTP %d:", p.Code) + body := strings.TrimSpace(strings.TrimPrefix(p.Message, prefix)) + msg := fmt.Sprintf("HTTP %d %s", p.Code, http.StatusText(p.Code)) + if body != "" { + msg = fmt.Sprintf("%s: %s", msg, contactTruncateError(body, 200)) + } + return msg + } + if p.Code != 0 { + return fmt.Sprintf("API %d: %s", p.Code, p.Message) + } + return p.Message + } + return err.Error() +} + +// contactFanoutAllFailedError builds the top-level error returned when every +// fanout query fails. It mirrors the representative (first) failure's +// classification — category, subtype, code, log_id, retryable, hint — so the +// exit-code classifier still sees the real signal, while carrying the aggregate +// message. The representative error is copied (never mutated) and kept as the +// cause, so a single-query problem object is not rewritten into an aggregate one. +func contactFanoutAllFailedError(err error, msg string) error { + var ( + apiErr *errs.APIError + netErr *errs.NetworkError + intErr *errs.InternalError + ) + switch { + case errors.As(err, &apiErr): + c := *apiErr + c.Message = msg + c.Cause = err + return &c + case errors.As(err, &netErr): + c := *netErr + c.Message = msg + c.Cause = err + return &c + case errors.As(err, &intErr): + c := *intErr + c.Message = msg + c.Cause = err + return &c + } + return errs.NewInternalError(errs.SubtypeUnknown, "%s", msg).WithHint(contactFanoutRetryHint).WithCause(err) +} + +func contactTruncateError(s string, maxRunes int) string { + r := []rune(s) + if len(r) <= maxRunes { + return s + } + return string(r[:maxRunes]) + "..." +} diff --git a/shortcuts/contact/contact_errors_test.go b/shortcuts/contact/contact_errors_test.go new file mode 100644 index 0000000000..28a278681c --- /dev/null +++ b/shortcuts/contact/contact_errors_test.go @@ -0,0 +1,81 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package contact + +import ( + "errors" + "strings" + "testing" + + "github.com/larksuite/cli/errs" +) + +func TestContactFanoutErrorSummary_HTTPStatus(t *testing.T) { + err := errs.NewNetworkError(errs.SubtypeNetworkServer, `HTTP 503: {"reason":"upstream_unavailable"}`). + WithCode(503). + WithRetryable() + + got := contactFanoutErrorSummary(err) + if !strings.HasPrefix(got, "HTTP 503 Service Unavailable: ") { + t.Fatalf("summary: got %q", got) + } + if !strings.Contains(got, "upstream_unavailable") { + t.Fatalf("summary should include truncated body details, got %q", got) + } +} + +func TestContactInvalidResponseError_TypedInternal(t *testing.T) { + got := contactInvalidResponseError("decode contact response failed") + p, ok := errs.ProblemOf(got) + if !ok { + t.Fatalf("expected typed problem, got %T", got) + } + if p.Category != errs.CategoryInternal || p.Subtype != errs.SubtypeInvalidResponse { + t.Fatalf("problem type: got %s/%s", p.Category, p.Subtype) + } +} + +func TestContactFanoutAllFailedError_PreservesTypedProblem(t *testing.T) { + err := errs.NewAPIError(errs.SubtypeRateLimit, "rate limit"). + WithCode(99991663). + WithLogID("log-contact-1"). + WithRetryable() + + got := contactFanoutAllFailedError(err, "all 2 queries failed; first: API 99991663: rate limit (query=\"alice\")") + p, ok := errs.ProblemOf(got) + if !ok { + t.Fatalf("expected typed problem, got %T", got) + } + if p.Category != errs.CategoryAPI || p.Subtype != errs.SubtypeRateLimit { + t.Fatalf("problem type: got %s/%s", p.Category, p.Subtype) + } + if p.Code != 99991663 || p.LogID != "log-contact-1" || !p.Retryable { + t.Fatalf("problem metadata not preserved: %+v", p) + } + if !strings.Contains(p.Message, "all 2 queries failed") { + t.Fatalf("problem message not decorated: %q", p.Message) + } + // The representative error must not be mutated: it stays a single-query + // failure, while the aggregate is a distinct value carrying it as cause. + if err.Message != "rate limit" { + t.Fatalf("representative error message was mutated: %q", err.Message) + } + if !errors.Is(got, err) { + t.Fatalf("aggregate error should keep the representative failure as its cause") + } +} + +func TestContactFanoutAllFailedError_UntypedGetsActionableHint(t *testing.T) { + got := contactFanoutAllFailedError(nil, "all 2 queries failed; first: internal error (query=\"alice\")") + p, ok := errs.ProblemOf(got) + if !ok { + t.Fatalf("expected typed problem, got %T", got) + } + if p.Category != errs.CategoryInternal || p.Subtype != errs.SubtypeUnknown { + t.Fatalf("problem type: got %s/%s", p.Category, p.Subtype) + } + if !strings.Contains(p.Hint, "narrow --queries") { + t.Fatalf("hint should guide recovery, got %q", p.Hint) + } +} diff --git a/shortcuts/contact/contact_get_user.go b/shortcuts/contact/contact_get_user.go index a056a637f5..16747f112c 100644 --- a/shortcuts/contact/contact_get_user.go +++ b/shortcuts/contact/contact_get_user.go @@ -28,7 +28,8 @@ var ContactGetUser = common.Shortcut{ }, Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { if runtime.Str("user-id") == "" && runtime.IsBot() { - return common.FlagErrorf("bot identity cannot get current user info, specify --user-id") + return common.ValidationErrorf("bot identity cannot get current user info, specify --user-id"). + WithParam("--user-id") } return nil }, @@ -63,7 +64,7 @@ var ContactGetUser = common.Shortcut{ if userId == "" { // Current user - data, err := runtime.CallAPI("GET", "/open-apis/authen/v1/user_info", nil, nil) + data, err := runtime.CallAPITyped("GET", "/open-apis/authen/v1/user_info", nil, nil) if err != nil { return err } @@ -87,7 +88,7 @@ var ContactGetUser = common.Shortcut{ if runtime.IsBot() { // Bot identity: GET /contact/v3/users/:user_id (full profile) - data, err := runtime.CallAPI("GET", "/open-apis/contact/v3/users/"+url.PathEscape(userId), + data, err := runtime.CallAPITyped("GET", "/open-apis/contact/v3/users/"+url.PathEscape(userId), map[string]interface{}{"user_id_type": userIdType}, nil) if err != nil { return err @@ -110,7 +111,7 @@ var ContactGetUser = common.Shortcut{ } // User identity: POST /contact/v3/users/basic_batch (lightweight) - data, err := runtime.CallAPI("POST", "/open-apis/contact/v3/users/basic_batch", + data, err := runtime.CallAPITyped("POST", "/open-apis/contact/v3/users/basic_batch", map[string]interface{}{"user_id_type": userIdType}, map[string]interface{}{"user_ids": []string{userId}}) if err != nil { diff --git a/shortcuts/contact/contact_get_user_test.go b/shortcuts/contact/contact_get_user_test.go new file mode 100644 index 0000000000..a669492021 --- /dev/null +++ b/shortcuts/contact/contact_get_user_test.go @@ -0,0 +1,125 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package contact + +import ( + "bytes" + "errors" + "testing" + + "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/internal/cmdutil" + "github.com/larksuite/cli/internal/httpmock" +) + +func TestGetUser_BotCurrentUserValidationTyped(t *testing.T) { + f, stdout, _, _ := cmdutil.TestFactory(t, searchUserDefaultConfig()) + + err := mountAndRun(t, ContactGetUser, []string{"+get-user", "--as", "bot"}, f, stdout) + if err == nil { + t.Fatalf("expected validation error") + } + var validation *errs.ValidationError + if !errors.As(err, &validation) { + t.Fatalf("expected validation error, got %T: %v", err, err) + } + if validation.Param != "--user-id" { + t.Fatalf("param: got %q, want --user-id", validation.Param) + } +} + +func TestGetUser_DryRunShapes(t *testing.T) { + cases := []struct { + name string + args []string + want []string + }{ + { + name: "current user", + args: []string{"+get-user", "--dry-run", "--as", "user"}, + want: []string{"GET", "/authen/v1/user_info", "current_user"}, + }, + { + name: "bot specific user", + args: []string{"+get-user", "--user-id", "ou_a", "--dry-run", "--as", "bot"}, + want: []string{"GET", "/contact/v3/users/ou_a", "ou_a", "open_id"}, + }, + { + name: "user basic batch", + args: []string{"+get-user", "--user-id", "ou_a", "--dry-run", "--as", "user"}, + want: []string{"POST", "/contact/v3/users/basic_batch", "ou_a", "open_id"}, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + f, stdout, _, _ := cmdutil.TestFactory(t, searchUserDefaultConfig()) + if err := mountAndRun(t, ContactGetUser, tc.args, f, stdout); err != nil { + t.Fatalf("dry-run: %v", err) + } + out := stdout.String() + for _, want := range tc.want { + if !bytes.Contains(stdout.Bytes(), []byte(want)) { + t.Fatalf("dry-run output missing %q: %s", want, out) + } + } + }) + } +} + +func TestGetUser_CurrentUserAPIFailureTyped(t *testing.T) { + f, stdout, _, reg := cmdutil.TestFactory(t, searchUserDefaultConfig()) + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "/open-apis/authen/v1/user_info", + Body: map[string]interface{}{"code": 123456, "msg": "upstream rejected contact request"}, + }) + + err := mountAndRun(t, ContactGetUser, []string{"+get-user", "--as", "user"}, f, stdout) + if err == nil { + t.Fatalf("expected API error") + } + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected typed problem, got %T: %v", err, err) + } + if p.Code != 123456 { + t.Fatalf("code: got %d, want 123456", p.Code) + } + if p.Category != errs.CategoryAPI { + t.Fatalf("category: got %q, want %q", p.Category, errs.CategoryAPI) + } + if stdout.Len() != 0 { + t.Fatalf("stdout should stay empty on API failure, got %q", stdout.String()) + } +} + +func TestGetUser_UserBasicBatchUsesTypedAPI(t *testing.T) { + f, stdout, _, reg := cmdutil.TestFactory(t, searchUserDefaultConfig()) + stub := &httpmock.Stub{ + Method: "POST", + URL: "/open-apis/contact/v3/users/basic_batch?user_id_type=open_id", + Body: map[string]interface{}{ + "code": 0, + "msg": "ok", + "data": map[string]interface{}{ + "users": []interface{}{ + map[string]interface{}{"user_id": "ou_a", "name": "Alice"}, + }, + }, + }, + } + reg.Register(stub) + + err := mountAndRun(t, ContactGetUser, []string{"+get-user", "--user-id", "ou_a", "--as", "user", "--format", "json"}, f, stdout) + if err != nil { + t.Fatalf("execute: %v", err) + } + if !bytes.Contains(stub.CapturedBody, []byte(`"ou_a"`)) { + t.Fatalf("request body should include user id, got %s", string(stub.CapturedBody)) + } + if !bytes.Contains(stdout.Bytes(), []byte(`"user"`)) { + t.Fatalf("stdout should include user object, got %s", stdout.String()) + } +} diff --git a/shortcuts/contact/contact_search_user.go b/shortcuts/contact/contact_search_user.go index 3aacce1491..3187d0834d 100644 --- a/shortcuts/contact/contact_search_user.go +++ b/shortcuts/contact/contact_search_user.go @@ -15,6 +15,7 @@ import ( "strings" "unicode/utf8" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/output" "github.com/larksuite/cli/shortcuts/common" @@ -80,12 +81,6 @@ type searchUserAPIFilter struct { HasEnterpriseEmail bool `json:"has_enterprise_email,omitempty"` } -type searchUserAPIEnvelope struct { - Code int `json:"code"` - Msg string `json:"msg"` - Data *searchUserAPIData `json:"data"` -} - type searchUserAPIData struct { Items []searchUserAPIItem `json:"items"` HasMore bool `json:"has_more"` @@ -216,19 +211,17 @@ func executeSearchUserSingle(ctx context.Context, runtime *common.RuntimeContext if err != nil { return err } - if apiResp.StatusCode != http.StatusOK { - return output.ErrAPI(apiResp.StatusCode, http.StatusText(apiResp.StatusCode), string(apiResp.RawBody)) - } - var resp searchUserAPIEnvelope - if err := json.Unmarshal(apiResp.RawBody, &resp); err != nil { - return output.ErrWithHint(output.ExitInternal, "validation", "unmarshal response failed", err.Error()) + data, err := runtime.ClassifyAPIResponse(apiResp) + if err != nil { + return err } - if resp.Code != 0 { - return output.ErrAPI(resp.Code, resp.Msg, string(apiResp.RawBody)) + respData, err := decodeSearchUserAPIData(data) + if err != nil { + return err } - users, hasMore := projectUsers(resp.Data, runtime.Str("lang"), runtime.Config.Brand) + users, hasMore := projectUsers(respData, runtime.Str("lang"), runtime.Config.Brand) out := searchUserResponse{Users: users, HasMore: hasMore} runtime.OutFormat(out, &output.Meta{Count: len(users)}, func(w io.Writer) { @@ -245,6 +238,20 @@ func executeSearchUserSingle(ctx context.Context, runtime *common.RuntimeContext return nil } +func decodeSearchUserAPIData(data map[string]interface{}) (*searchUserAPIData, error) { + raw, err := json.Marshal(data) + if err != nil { + return nil, contactInvalidResponseError("marshal search user response data failed"). + WithCause(err) + } + var out searchUserAPIData + if err := json.Unmarshal(raw, &out); err != nil { + return nil, contactInvalidResponseError("decode search user response data failed"). + WithCause(err) + } + return &out, nil +} + func isHumanReadableFormat(format string) bool { return format == "pretty" || format == "table" } @@ -373,52 +380,74 @@ func rowFromItem(item *searchUserAPIItem, lang string, brand core.LarkBrand) sea func validateSearchUser(runtime *common.RuntimeContext) error { if !hasAnySearchInput(runtime) { - return common.FlagErrorf( + return common.ValidationErrorf( "specify at least one of --query, --queries, --user-ids, --has-chatted, --has-enterprise-email, --exclude-external-users, --left-organization", + ).WithParams( + errs.InvalidParam{Name: "--query", Reason: "required; specify at least one search input"}, + errs.InvalidParam{Name: "--queries", Reason: "required; specify at least one search input"}, + errs.InvalidParam{Name: "--user-ids", Reason: "required; specify at least one search input"}, + errs.InvalidParam{Name: "--has-chatted", Reason: "required; specify at least one search input"}, + errs.InvalidParam{Name: "--has-enterprise-email", Reason: "required; specify at least one search input"}, + errs.InvalidParam{Name: "--exclude-external-users", Reason: "required; specify at least one search input"}, + errs.InvalidParam{Name: "--left-organization", Reason: "required; specify at least one search input"}, ) } queriesRaw := strings.TrimSpace(runtime.Str("queries")) if queriesRaw != "" { if strings.TrimSpace(runtime.Str("query")) != "" { - return common.FlagErrorf("--query and --queries are mutually exclusive") + return common.ValidationErrorf("--query and --queries are mutually exclusive"). + WithParams( + errs.InvalidParam{Name: "--query", Reason: "mutually exclusive with --queries"}, + errs.InvalidParam{Name: "--queries", Reason: "mutually exclusive with --query"}, + ) } if strings.TrimSpace(runtime.Str("user-ids")) != "" { - return common.FlagErrorf("--user-ids and --queries are mutually exclusive") + return common.ValidationErrorf("--user-ids and --queries are mutually exclusive"). + WithParams( + errs.InvalidParam{Name: "--user-ids", Reason: "mutually exclusive with --queries"}, + errs.InvalidParam{Name: "--queries", Reason: "mutually exclusive with --user-ids"}, + ) } queries := parseAndDedupQueries(queriesRaw) if len(queries) == 0 { - return common.FlagErrorf("--queries: no valid query parsed from %q (separate entries with ',')", queriesRaw) + return common.ValidationErrorf("--queries: no valid query parsed from %q (separate entries with ',')", queriesRaw). + WithParam("--queries") } if len(queries) > maxFanoutQueries { - return common.FlagErrorf("--queries: must be at most %d entries (got %d)", maxFanoutQueries, len(queries)) + return common.ValidationErrorf("--queries: must be at most %d entries (got %d)", maxFanoutQueries, len(queries)). + WithParam("--queries") } for _, q := range queries { if utf8.RuneCountInString(q) > maxSearchUserQueryChars { - return common.FlagErrorf("--queries: entry %q exceeds %d characters", q, maxSearchUserQueryChars) + return common.ValidationErrorf("--queries: entry %q exceeds %d characters", q, maxSearchUserQueryChars). + WithParam("--queries") } } } if q := strings.TrimSpace(runtime.Str("query")); q != "" { if utf8.RuneCountInString(q) > maxSearchUserQueryChars { - return common.FlagErrorf("--query: length must be between 1 and %d characters", maxSearchUserQueryChars) + return common.ValidationErrorf("--query: length must be between 1 and %d characters", maxSearchUserQueryChars). + WithParam("--query") } } if raw := strings.TrimSpace(runtime.Str("user-ids")); raw != "" { - ids, err := common.ResolveOpenIDs("--user-ids", common.SplitCSV(raw), runtime) + ids, err := common.ResolveOpenIDsTyped("--user-ids", common.SplitCSV(raw), runtime) if err != nil { return err } if len(ids) == 0 { - return common.FlagErrorf("--user-ids: no valid open_id parsed from %q (separate entries with ',')", raw) + return common.ValidationErrorf("--user-ids: no valid open_id parsed from %q (separate entries with ',')", raw). + WithParam("--user-ids") } if len(ids) > maxSearchUserUserIDs { - return common.FlagErrorf("--user-ids: must be at most %d entries", maxSearchUserUserIDs) + return common.ValidationErrorf("--user-ids: must be at most %d entries", maxSearchUserUserIDs). + WithParam("--user-ids") } for _, id := range ids { - if _, err := common.ValidateUserID(id); err != nil { + if _, err := common.ValidateUserIDTyped("--user-ids", id); err != nil { return err } } @@ -429,15 +458,16 @@ func validateSearchUser(runtime *common.RuntimeContext) error { // silent wrong-result bugs. for _, bf := range searchUserBoolFilters { if runtime.Cmd.Flags().Changed(bf.Flag) && !runtime.Bool(bf.Flag) { - return common.FlagErrorf( + return common.ValidationErrorf( "--%s: pass the flag to enable the filter; omit it to disable filtering (=false is rejected to prevent silent wrong results)", bf.Flag, - ) + ).WithParam("--" + bf.Flag) } } if n := runtime.Int("page-size"); n < 1 || n > maxSearchUserPageSize { - return common.FlagErrorf("--page-size: must be between 1 and %d", maxSearchUserPageSize) + return common.ValidationErrorf("--page-size: must be between 1 and %d", maxSearchUserPageSize). + WithParam("--page-size") } return nil } @@ -473,7 +503,7 @@ func buildSearchUserBody(runtime *common.RuntimeContext) (*searchUserAPIRequest, hasFilter := false if raw := strings.TrimSpace(runtime.Str("user-ids")); raw != "" { - ids, err := common.ResolveOpenIDs("--user-ids", common.SplitCSV(raw), runtime) + ids, err := common.ResolveOpenIDsTyped("--user-ids", common.SplitCSV(raw), runtime) if err != nil { return nil, err } diff --git a/shortcuts/contact/contact_search_user_fanout.go b/shortcuts/contact/contact_search_user_fanout.go index e3dd8d40fc..b9342ee140 100644 --- a/shortcuts/contact/contact_search_user_fanout.go +++ b/shortcuts/contact/contact_search_user_fanout.go @@ -5,7 +5,6 @@ package contact import ( "context" - "encoding/json" "fmt" "io" "net/http" @@ -47,7 +46,7 @@ type fanoutResult struct { Users []searchUser HasMore bool ErrMsg string // empty = success - ErrCode int // 0 = success or unknown; otherwise an HTTP status or Lark API code corresponding to the first error + Err error // original failure, kept for typed all-failed propagation } // isFanoutSummaryFormat gates the per-fanout stderr summary line. Includes csv @@ -67,7 +66,7 @@ func runOneQuery(ctx context.Context, runtime *common.RuntimeContext, index int, // Pre-check ctx so queued workers see cancellation before issuing a // request; in-flight workers continue until DoAPI returns. if err := ctx.Err(); err != nil { - return fanoutResult{Index: index, Query: query, ErrMsg: err.Error()} + return fanoutErrorResult(index, query, err) } body := &searchUserAPIRequest{Query: query} @@ -82,38 +81,29 @@ func runOneQuery(ctx context.Context, runtime *common.RuntimeContext, index int, QueryParams: larkcore.QueryParams{"page_size": []string{strconv.Itoa(runtime.Int("page-size"))}}, }) if err != nil { - return fanoutResult{Index: index, Query: query, ErrMsg: err.Error()} - } - if apiResp.StatusCode != http.StatusOK { - body := strings.TrimSpace(string(apiResp.RawBody)) - const maxBody = 200 - if len(body) > maxBody { - body = body[:maxBody] + "..." - } - msg := fmt.Sprintf("HTTP %d %s", apiResp.StatusCode, http.StatusText(apiResp.StatusCode)) - if body != "" { - msg = fmt.Sprintf("%s: %s", msg, body) - } - return fanoutResult{Index: index, Query: query, - ErrMsg: msg, - ErrCode: apiResp.StatusCode} + return fanoutErrorResult(index, query, err) } - var resp searchUserAPIEnvelope - if err := json.Unmarshal(apiResp.RawBody, &resp); err != nil { - return fanoutResult{Index: index, Query: query, - ErrMsg: fmt.Sprintf("parse response failed: %v", err)} + data, err := runtime.ClassifyAPIResponse(apiResp) + if err != nil { + return fanoutErrorResult(index, query, err) } - if resp.Code != 0 { - return fanoutResult{Index: index, Query: query, - ErrMsg: fmt.Sprintf("API %d: %s", resp.Code, resp.Msg), - ErrCode: resp.Code} + respData, err := decodeSearchUserAPIData(data) + if err != nil { + return fanoutErrorResult(index, query, err) } - users, hasMore := projectUsers(resp.Data, runtime.Str("lang"), runtime.Config.Brand) + users, hasMore := projectUsers(respData, runtime.Str("lang"), runtime.Config.Brand) return fanoutResult{Index: index, Query: query, Users: users, HasMore: hasMore} } +func fanoutErrorResult(index int, query string, err error) fanoutResult { + if err == nil { + return fanoutResult{Index: index, Query: query} + } + return fanoutResult{Index: index, Query: query, ErrMsg: contactFanoutErrorSummary(err), Err: err} +} + type fanoutUser struct { searchUser MatchedQuery string `json:"matched_query"` @@ -146,7 +136,7 @@ func buildFanoutResponse(queries []string, results []fanoutResult) (*fanoutRespo } failed := 0 var firstErrMsg, firstErrQuery string - var firstErrCode int + var firstErr error for i, r := range indexed { out.Queries = append(out.Queries, querySummary{ Query: queries[i], @@ -158,7 +148,7 @@ func buildFanoutResponse(queries []string, results []fanoutResult) (*fanoutRespo if firstErrMsg == "" { firstErrMsg = r.ErrMsg firstErrQuery = queries[i] - firstErrCode = r.ErrCode + firstErr = r.Err } continue } @@ -169,18 +159,7 @@ func buildFanoutResponse(queries []string, results []fanoutResult) (*fanoutRespo if failed == len(queries) && len(queries) > 0 { msg := fmt.Sprintf("all %d queries failed; first: %s (query=%q)", len(queries), firstErrMsg, firstErrQuery) - // Only the HTTP-status / Lark-API-code branches in runOneQuery populate - // ErrCode; transport, parse, panic, and ctx-canceled stay at 0. Code 0 - // means success in the Lark protocol, so don't pretend it's an API error - // when we have nothing structured to report. - if firstErrCode != 0 { - return nil, output.ErrAPI(firstErrCode, msg, "") - } - // No structured API code — the failure was transport, parse, panic, or - // cancellation. Suggest the actionable next step rather than shipping - // an empty hint that would leave the calling agent with nothing to do. - return nil, output.ErrWithHint(output.ExitInternal, "fanout", msg, - "retry the command; if it persists, narrow --queries to a single term to isolate the failing input") + return nil, contactFanoutAllFailedError(firstErr, msg) } return out, nil } diff --git a/shortcuts/contact/contact_search_user_test.go b/shortcuts/contact/contact_search_user_test.go index 5e14f1974a..fee87466e8 100644 --- a/shortcuts/contact/contact_search_user_test.go +++ b/shortcuts/contact/contact_search_user_test.go @@ -7,7 +7,6 @@ import ( "bytes" "context" "encoding/json" - "errors" "fmt" "net/http" "strings" @@ -16,10 +15,10 @@ import ( "time" "unicode/utf8" + "github.com/larksuite/cli/errs" "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" ) @@ -254,6 +253,16 @@ func TestRowFromItem_CrossTenantEmptyEmailNoPanic(t *testing.T) { } } +func TestProjectUsers_NilData(t *testing.T) { + users, hasMore := projectUsers(nil, "", core.BrandFeishu) + if users == nil { + t.Fatalf("users should be an empty slice, not nil") + } + if len(users) != 0 || hasMore { + t.Fatalf("projectUsers(nil): got users=%v hasMore=%v", users, hasMore) + } +} + func TestValidateSearchUser_AllEmpty_Errors(t *testing.T) { cmd := newSearchUserTestCommand() rt := common.TestNewRuntimeContext(cmd, searchUserDefaultConfig()) @@ -479,6 +488,26 @@ func TestBuildBody_UserIDsResolveAndDedup(t *testing.T) { } } +func TestBuildBody_UserIDsMeWithoutLoginReturnsTypedError(t *testing.T) { + cmd := newSearchUserTestCommand() + _ = cmd.Flags().Set("user-ids", "me") + cfg := searchUserDefaultConfig() + cfg.UserOpenId = "" + rt := common.TestNewRuntimeContext(cmd, cfg) + + body, err := buildSearchUserBody(rt) + if err == nil { + t.Fatalf("expected error, got body %+v", body) + } + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected typed problem, got %T: %v", err, err) + } + if p.Category != errs.CategoryValidation { + t.Fatalf("category: got %q, want %q", p.Category, errs.CategoryValidation) + } +} + func TestValidateSearchUser_PageSizeOutOfRange_Errors(t *testing.T) { for _, n := range []int{0, 31} { cmd := newSearchUserTestCommand() @@ -504,6 +533,20 @@ func TestValidateSearchUser_PageSizeBoundaries_OK(t *testing.T) { } } +func TestDecodeSearchUserAPIData_MarshalFailureTyped(t *testing.T) { + _, err := decodeSearchUserAPIData(map[string]interface{}{"bad": func() {}}) + if err == nil { + t.Fatalf("expected marshal failure") + } + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected typed problem, got %T: %v", err, err) + } + if p.Category != errs.CategoryInternal || p.Subtype != errs.SubtypeInvalidResponse { + t.Fatalf("problem type: got %s/%s", p.Category, p.Subtype) + } +} + // mountAndRun mounts the shortcut under a parent cobra command and runs it // with the given args. Mirrors the pattern used in other shortcut packages. func mountAndRun(t *testing.T, s common.Shortcut, args []string, f *cmdutil.Factory, stdout *bytes.Buffer) error { @@ -1011,6 +1054,13 @@ func TestRunOneQuery_APINonZeroCode(t *testing.T) { if got.ErrMsg != "API 99991663: rate limited" { t.Errorf("ErrMsg = %q, want 'API 99991663: rate limited'", got.ErrMsg) } + p, ok := errs.ProblemOf(got.Err) + if !ok { + t.Fatalf("expected typed problem on fanout result, got %T", got.Err) + } + if p.Code != 99991663 { + t.Errorf("problem code: got %d, want 99991663", p.Code) + } if got.Users != nil || got.HasMore { t.Errorf("on error, Users/HasMore must be zero values; got %+v", got) } @@ -1032,8 +1082,15 @@ func TestRunOneQuery_HTTPNon200(t *testing.T) { if !strings.Contains(got.ErrMsg, "upstream_unavailable") { t.Errorf("ErrMsg should include response body for diagnosis; got %q", got.ErrMsg) } - if got.ErrCode != 503 { - t.Errorf("ErrCode = %d, want 503", got.ErrCode) + p, ok := errs.ProblemOf(got.Err) + if !ok { + t.Fatalf("expected typed problem on fanout result, got %T", got.Err) + } + if p.Code != 503 { + t.Errorf("problem code: got %d, want 503", p.Code) + } + if p.Category != errs.CategoryNetwork { + t.Errorf("problem category: got %q, want %q", p.Category, errs.CategoryNetwork) } } @@ -1080,6 +1137,16 @@ func TestRunOneQuery_TransportError(t *testing.T) { } } +func TestFanoutErrorResult_NilErrorIsSuccess(t *testing.T) { + got := fanoutErrorResult(4, "alice", nil) + if got.Index != 4 || got.Query != "alice" { + t.Fatalf("Index/Query mismatch: %+v", got) + } + if got.ErrMsg != "" || got.Err != nil { + t.Fatalf("nil error should produce a success result, got %+v", got) + } +} + func TestFanoutAssemble_OrderAndShape(t *testing.T) { results := []fanoutResult{ {Index: 1, Query: "bob", Users: []searchUser{{OpenID: "ou_b"}}, HasMore: true}, @@ -1136,7 +1203,7 @@ func TestFanoutAssemble_AllFailed_ReturnsError(t *testing.T) { } // When all queries fail with no structured Lark API code (transport, parse, -// panic, ctx-canceled), the returned ExitError must carry an actionable +// panic, ctx-canceled), the returned typed error must carry an actionable // hint so the calling agent has a next step to try instead of giving up. func TestFanoutAssemble_AllFailed_NoCode_HasActionableHint(t *testing.T) { results := []fanoutResult{ @@ -1147,28 +1214,38 @@ func TestFanoutAssemble_AllFailed_NoCode_HasActionableHint(t *testing.T) { if err == nil { t.Fatalf("expected error when all queries failed") } - var exitErr *output.ExitError - if !errors.As(err, &exitErr) { - t.Fatalf("expected *output.ExitError, got %T", err) + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected typed problem, got %T", err) } - if exitErr.Detail == nil { - t.Fatalf("expected Detail, got nil") + if p.Category != errs.CategoryInternal { + t.Fatalf("category: got %q, want %q", p.Category, errs.CategoryInternal) } - if exitErr.Detail.Hint == "" { + if p.Hint == "" { t.Errorf("expected non-empty Hint so agents have a next step; got empty") } - if !strings.Contains(exitErr.Detail.Hint, "retry") { - t.Errorf("hint should suggest retry as the first action; got %q", exitErr.Detail.Hint) + if !strings.Contains(p.Hint, "retry") { + t.Errorf("hint should suggest retry as the first action; got %q", p.Hint) } } -// Codes from the first failure must propagate through output.ErrAPI so the -// CLI's exit-code classifier sees the real signal (e.g., 99991663 rate limit) +// Codes from the first failure must propagate through typed problem fields so +// the CLI's exit-code classifier sees the real signal (e.g., 99991663 rate limit) // instead of 0, which would mean "success" in the Lark protocol. func TestFanoutAssemble_AllFailed_PropagatesFirstCode(t *testing.T) { results := []fanoutResult{ - {Index: 0, Query: "alice", ErrMsg: "API 99991663: rate limit", ErrCode: 99991663}, - {Index: 1, Query: "bob", ErrMsg: "HTTP 500", ErrCode: 500}, + { + Index: 0, + Query: "alice", + ErrMsg: "API 99991663: rate limit", + Err: errs.NewAPIError(errs.SubtypeRateLimit, "rate limit").WithCode(99991663), + }, + { + Index: 1, + Query: "bob", + ErrMsg: "HTTP 500", + Err: errs.NewNetworkError(errs.SubtypeNetworkServer, "HTTP 500").WithCode(500), + }, } _, err := buildFanoutResponse([]string{"alice", "bob"}, results) if err == nil { @@ -1177,6 +1254,16 @@ func TestFanoutAssemble_AllFailed_PropagatesFirstCode(t *testing.T) { if !strings.Contains(err.Error(), "rate limit") { t.Errorf("error should contain first ErrMsg; got %v", err) } + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected typed problem, got %T", err) + } + if p.Code != 99991663 { + t.Errorf("problem code: got %d, want 99991663", p.Code) + } + if p.Subtype != errs.SubtypeRateLimit { + t.Errorf("problem subtype: got %q, want %q", p.Subtype, errs.SubtypeRateLimit) + } } func TestFanoutAssemble_PartialFailureOK(t *testing.T) { @@ -1220,6 +1307,37 @@ func TestFanoutAssemble_NoTopLevelHasMore(t *testing.T) { } } +func TestPrettyFanoutUserRows(t *testing.T) { + rows := prettyFanoutUserRows([]fanoutUser{ + { + searchUser: searchUser{ + OpenID: "ou_a", + LocalizedName: "Alice", + Department: strings.Repeat("d", 80), + EnterpriseEmail: "alice@example.com", + HasChatted: true, + ChatRecencyHint: "Contacted yesterday", + }, + MatchedQuery: "alice", + }, + }) + if len(rows) != 1 { + t.Fatalf("rows: got %d, want 1", len(rows)) + } + row := rows[0] + for _, key := range []string{"matched_query", "localized_name", "department", "enterprise_email", "has_chatted", "chat_recency_hint", "open_id"} { + if _, ok := row[key]; !ok { + t.Fatalf("row missing key %q: %+v", key, row) + } + } + if row["matched_query"] != "alice" || row["open_id"] != "ou_a" { + t.Fatalf("row identity fields: %+v", row) + } + if len(row["department"].(string)) >= 80 { + t.Fatalf("department should be truncated for table display, got %q", row["department"]) + } +} + // Verifies that with the auto-pagination flags removed, --page-all / --page-limit // are no longer accepted. cobra must reject the unknown flag at parse time — // no stub is registered because the command should never reach the API.