diff --git a/cmd/api/api_paginate_test.go b/cmd/api/api_paginate_test.go index 11e576bfee..72375a1a03 100644 --- a/cmd/api/api_paginate_test.go +++ b/cmd/api/api_paginate_test.go @@ -245,8 +245,15 @@ func TestAPIPaginate_StreamingWriteFailureStopsFurtherPages(t *testing.T) { t.Fatalf("apiPaginate() error = %v, want preserved writer cause", err) } problem, ok := errs.ProblemOf(err) - if !ok || problem.Category != errs.CategoryInternal { - t.Fatalf("apiPaginate() problem = %#v, %v; want internal typed error", problem, ok) + if !ok || problem.Category != errs.CategoryInternal || problem.Subtype != errs.SubtypeUnknown { + t.Fatalf("apiPaginate() problem = %#v, %v; want internal/unknown", problem, ok) + } + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) { + t.Fatalf("apiPaginate() error = %T (%v), want PaginationError", err, err) + } + if paginationErr.CompletedPages != 1 || paginationErr.NextPageToken != "next-1" { + t.Fatalf("progress = %d/%q, want 1/next-1", paginationErr.CompletedPages, paginationErr.NextPageToken) } if calls != 2 { t.Fatalf("pagination requests = %d, want 2", calls) @@ -290,7 +297,7 @@ func TestAPIPaginate_StreamingFormatFallsBackToJSONWithoutList(t *testing.T) { } } -func TestAPIPaginate_BusinessErrorsWriteRawAndAreMarkedRaw(t *testing.T) { +func TestAPIPaginate_BusinessErrorsReturnProgressWithoutStdout(t *testing.T) { businessResponse := map[string]interface{}{ "code": 123456, "msg": "fixture business error", @@ -322,9 +329,19 @@ func TestAPIPaginate_BusinessErrorsWriteRawAndAreMarkedRaw(t *testing.T) { if !errs.IsRaw(err) { t.Fatalf("errs.IsRaw(error) = false, want true; error = %T: %v", err, err) } - assertAPIPaginateJSONBytes(t, out.Bytes(), businessResponse) - if bytes.Contains(out.Bytes(), []byte(`"ok": true`)) { - t.Fatalf("business-error stdout contains a success envelope:\n%s", out.Bytes()) + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) { + t.Fatalf("error = %T (%v), want PaginationError", err, err) + } + if paginationErr.CompletedPages != 0 || paginationErr.NextPageToken != "" { + t.Fatalf("progress = %d/%q, want 0/empty", paginationErr.CompletedPages, paginationErr.NextPageToken) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryAPI || problem.Subtype != errs.SubtypeUnknown || problem.Code != 123456 { + t.Fatalf("ProblemOf = %#v, %v; want api/unknown code 123456", problem, ok) + } + if got := out.String(); got != "" { + t.Fatalf("stdout bytes = %q, want empty", got) } if got := errOut.String(); got != "" { t.Fatalf("stderr bytes = %q, want empty", got) @@ -357,6 +374,17 @@ func TestAPIPaginate_TransportErrorsAreMarkedRaw(t *testing.T) { if !errs.IsRaw(err) { t.Fatalf("errs.IsRaw(error) = false, want true; error = %T: %v", err, err) } + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) { + t.Fatalf("error = %T (%v), want PaginationError", err, err) + } + if paginationErr.CompletedPages != 0 || paginationErr.NextPageToken != "" { + t.Fatalf("progress = %d/%q, want 0/empty", paginationErr.CompletedPages, paginationErr.NextPageToken) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryNetwork { + t.Fatalf("ProblemOf = %#v, %v; want original network classification", problem, ok) + } if got := out.String(); got != "" { t.Fatalf("stdout bytes = %q, want empty", got) } @@ -367,6 +395,37 @@ func TestAPIPaginate_TransportErrorsAreMarkedRaw(t *testing.T) { } } +func TestAPIPaginate_DecodeErrorReturnsProgressWithoutStdout(t *testing.T) { + ac, out, errOut, reg := newAPIPaginateTestHarness(t) + reg.Register(&httpmock.Stub{ + URL: "/open-apis/test/v1/items", + RawBody: []byte(`{"code":`), + }) + + err := apiPaginate(context.Background(), ac, apiPaginateRequest(), + output.FormatJSON, "", out, errOut, "lark-cli api GET", client.PaginationOptions{PageDelay: -1}) + if err == nil { + t.Fatal("apiPaginate() error = nil, want decode error") + } + if !errs.IsRaw(err) { + t.Fatalf("errs.IsRaw(error) = false, want true; error = %T: %v", err, err) + } + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) { + t.Fatalf("error = %T (%v), want PaginationError", err, err) + } + if paginationErr.CompletedPages != 0 || paginationErr.NextPageToken != "" { + t.Fatalf("progress = %d/%q, want 0/empty", paginationErr.CompletedPages, paginationErr.NextPageToken) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryInternal || problem.Subtype != errs.SubtypeInvalidResponse { + t.Fatalf("ProblemOf = %#v, %v; want internal/invalid_response", problem, ok) + } + if out.Len() != 0 || errOut.Len() != 0 { + t.Fatalf("decode error wrote stdout/stderr: %q / %q", out.String(), errOut.String()) + } +} + func TestAPIPaginate_StreamBusinessErrorIsMarkedRaw(t *testing.T) { ac, out, errOut, reg := newAPIPaginateTestHarness(t) reg.Register(&httpmock.Stub{ @@ -387,6 +446,10 @@ func TestAPIPaginate_StreamBusinessErrorIsMarkedRaw(t *testing.T) { if !errs.IsRaw(err) { t.Fatalf("errs.IsRaw(error) = false, want true; error = %T: %v", err, err) } + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) || paginationErr.CompletedPages != 0 || paginationErr.NextPageToken != "" { + t.Fatalf("pagination progress = %#v, want 0 completed pages and no token", paginationErr) + } if got := out.String(); got != "" { t.Fatalf("stdout bytes = %q, want empty", got) } diff --git a/cmd/api/api_test.go b/cmd/api/api_test.go index c269bb9bb4..9d56239df4 100644 --- a/cmd/api/api_test.go +++ b/cmd/api/api_test.go @@ -448,7 +448,7 @@ func TestApiCmd_PageAll_NonBatchAPI_FallbackToJSON(t *testing.T) { } } -func TestApiCmd_PageAll_NonBatchAPI_ErrorStillOutputsJSON(t *testing.T) { +func TestApiCmd_PageAll_NonBatchAPI_ErrorReturnsPaginationProgress(t *testing.T) { f, stdout, _, reg := cmdutil.TestFactory(t, &core.CliConfig{ AppID: "test-app-pageall-err", AppSecret: "test-secret-pageall-err", Brand: core.BrandFeishu, }) @@ -468,17 +468,17 @@ func TestApiCmd_PageAll_NonBatchAPI_ErrorStillOutputsJSON(t *testing.T) { if err == nil { t.Fatal("expected an error for non-zero code") } - // Should still output the response body so user can see the error details - if !strings.Contains(stdout.String(), "230027") { - t.Errorf("expected error response in stdout, got: %s", stdout.String()) + if stdout.Len() != 0 { + t.Fatalf("pagination error wrote stdout: %s", stdout.String()) } - if !strings.Contains(stdout.String(), "user not authorized") { - t.Errorf("expected error message in stdout, got: %s", stdout.String()) + requireProblem(t, err, errs.CategoryAuthorization, errs.SubtypeUserUnauthorized, 230027) + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) { + t.Fatalf("expected PaginationError, got %T: %v", err, err) } - if strings.Contains(stdout.String(), `"ok": true`) || strings.Contains(stdout.String(), `"ok":true`) { - t.Fatalf("unexpected success envelope on error path: %s", stdout.String()) + if paginationErr.CompletedPages != 0 || paginationErr.NextPageToken != "" { + t.Fatalf("progress = %d/%q, want 0/empty", paginationErr.CompletedPages, paginationErr.NextPageToken) } - requireProblem(t, err, errs.CategoryAuthorization, errs.SubtypeUserUnauthorized, 230027) var permErr *errs.PermissionError if !errors.As(err, &permErr) { t.Fatalf("expected PermissionError, got %T: %v", err, err) diff --git a/cmd/auth/auth.go b/cmd/auth/auth.go index 9d5c8204a1..077351c85b 100644 --- a/cmd/auth/auth.go +++ b/cmd/auth/auth.go @@ -6,7 +6,6 @@ package auth import ( "context" "encoding/json" - "fmt" "net/http" "slices" @@ -14,7 +13,9 @@ import ( larkcore "github.com/larksuite/oapi-sdk-go/v3/core" "github.com/spf13/cobra" + "github.com/larksuite/cli/errs" larkauth "github.com/larksuite/cli/internal/auth" + "github.com/larksuite/cli/internal/client" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/errclass" @@ -80,15 +81,22 @@ func getUserInfo(ctx context.Context, sdk *lark.Client, accessToken string) (ope return "", "", err } + _, err = client.ClassifyAPIResponse(apiResp, func(result interface{}) error { + raw, _ := result.(map[string]interface{}) + return errclass.BuildAPIError(raw, errclass.ClassifyContext{Identity: string(core.AsUser)}) + }) + if err != nil { + return "", "", err + } + var resp userInfoResponse if err := json.Unmarshal(apiResp.RawBody, &resp); err != nil { - return "", "", fmt.Errorf("failed to parse user info: %w", err) - } - if resp.Code != 0 { - return "", "", fmt.Errorf("failed to get user info [%d]: %s", resp.Code, resp.Msg) + return "", "", errs.NewInternalError(errs.SubtypeInvalidResponse, + "failed to parse user info: %v", err).WithCause(err) } if resp.Data.OpenID == "" { - return "", "", fmt.Errorf("failed to get user info: missing open_id in response") + return "", "", errs.NewInternalError(errs.SubtypeInvalidResponse, + "failed to get user info: missing open_id in response") } name = resp.Data.Name @@ -146,12 +154,23 @@ func getAppInfo(ctx context.Context, f *cmdutil.Factory, appId string) (*appInfo return nil, err } + cc := errclass.ClassifyContext{Identity: string(core.AsBot)} + if cfg, _ := f.Config(); cfg != nil { + cc.Brand = string(cfg.Brand) + cc.AppID = appId + } + _, err = client.ClassifyAPIResponse(apiResp, func(result interface{}) error { + raw, _ := result.(map[string]interface{}) + return errclass.BuildAPIError(raw, cc) + }) + if err != nil { + return nil, err + } + var resp appInfoResponse if err := json.Unmarshal(apiResp.RawBody, &resp); err != nil { - return nil, fmt.Errorf("failed to parse response: %w", err) - } - if resp.Code != 0 { - return nil, classifyAppInfoErr(apiResp.RawBody, resp.Code, resp.Msg, f, appId) + return nil, errs.NewInternalError(errs.SubtypeInvalidResponse, + "failed to parse response: %v", err).WithCause(err) } app := resp.Data.App @@ -170,21 +189,3 @@ func getAppInfo(ctx context.Context, f *cmdutil.Factory, appId string) (*appInfo return &appInfo{OwnerOpenId: ownerOpenId, UserScopes: userScopes}, nil } - -// classifyAppInfoErr re-decodes the raw body so BuildAPIError sees the -// upstream `error` block — the typed appInfoResponse shape drops it. -func classifyAppInfoErr(rawBody []byte, code int, msg string, f *cmdutil.Factory, appId string) error { - var raw map[string]any - _ = json.Unmarshal(rawBody, &raw) - if raw == nil { - raw = map[string]any{} - } - raw["code"] = code - raw["msg"] = msg - cc := errclass.ClassifyContext{Identity: string(core.AsBot)} - if cfg, _ := f.Config(); cfg != nil { - cc.Brand = string(cfg.Brand) - cc.AppID = appId - } - return errclass.BuildAPIError(raw, cc) -} diff --git a/cmd/auth/auth_test.go b/cmd/auth/auth_test.go index f633a61433..8389939783 100644 --- a/cmd/auth/auth_test.go +++ b/cmd/auth/auth_test.go @@ -5,6 +5,7 @@ package auth import ( "context" + "encoding/json" "errors" "io" "net/http" @@ -481,6 +482,188 @@ func TestAuthScopesRun_LarkPermissionError_TypedAsPermissionError(t *testing.T) } } +func TestGetAppInfo_RateLimitRecoveryMetadata(t *testing.T) { + for _, tt := range []struct { + name string + status int + body map[string]interface{} + wantCode int + }{ + {name: "HTTP 429 code zero", status: http.StatusTooManyRequests, body: map[string]interface{}{"code": 0, "msg": "slow"}, wantCode: 429}, + {name: "business rate limit", status: http.StatusOK, body: map[string]interface{}{"code": 99991400, "msg": "slow"}, wantCode: 99991400}, + } { + t.Run(tt.name, func(t *testing.T) { + f, _, _, reg := cmdutil.TestFactory(t, &core.CliConfig{AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu}) + f.Credential = credential.NewCredentialProvider(nil, nil, &authScopesTokenResolver{}, nil) + reg.Register(&httpmock.Stub{ + Method: http.MethodGet, + URL: "/open-apis/application/v6/applications/test-app", + Status: tt.status, + Headers: http.Header{"Content-Type": []string{"application/json"}, "Retry-After": []string{"17"}}, + Body: tt.body, + }) + + _, err := getAppInfo(context.Background(), f, "test-app") + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("getAppInfo() error = %T (%v), want *errs.APIError", err, err) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryAPI || problem.Subtype != errs.SubtypeRateLimit { + t.Fatalf("problem = %#v, want api/rate_limit", problem) + } + if apiErr.Subtype != errs.SubtypeRateLimit || apiErr.Code != tt.wantCode { + t.Fatalf("rate limit problem = %#v, want code %d", apiErr.Problem, tt.wantCode) + } + if apiErr.RetryAfterSeconds == nil || *apiErr.RetryAfterSeconds != 17 || apiErr.RetryAfterSource != "retry-after" { + t.Fatalf("retry metadata = (%v, %q), want (17, retry-after)", apiErr.RetryAfterSeconds, apiErr.RetryAfterSource) + } + }) + } +} + +func TestGetAppInfo_InvalidJSONReturnsTypedInvalidResponse(t *testing.T) { + f, _, _, reg := cmdutil.TestFactory(t, &core.CliConfig{AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu}) + f.Credential = credential.NewCredentialProvider(nil, nil, &authScopesTokenResolver{}, nil) + reg.Register(&httpmock.Stub{ + Method: http.MethodGet, + URL: "/open-apis/application/v6/applications/test-app", + RawBody: []byte("not-json"), + }) + + _, err := getAppInfo(context.Background(), f, "test-app") + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryInternal || problem.Subtype != errs.SubtypeInvalidResponse { + t.Fatalf("error = %T (%v), problem = %#v; want internal/invalid_response", err, err, problem) + } + var syntaxErr *json.SyntaxError + if !errors.As(err, &syntaxErr) { + t.Fatalf("error chain does not preserve JSON syntax error: %v", err) + } +} + +func TestGetUserInfo_RateLimitRecoveryMetadata(t *testing.T) { + for _, tt := range []struct { + name string + status int + body map[string]interface{} + wantCode int + }{ + {name: "HTTP 429 code zero", status: http.StatusTooManyRequests, body: map[string]interface{}{"code": 0, "msg": "slow"}, wantCode: 429}, + {name: "business rate limit", status: http.StatusOK, body: map[string]interface{}{"code": 99991400, "msg": "slow"}, wantCode: 99991400}, + } { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) + f, _, _, reg := cmdutil.TestFactory(t, &core.CliConfig{ + AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu, + }) + reg.Register(&httpmock.Stub{ + Method: http.MethodGet, + URL: "/open-apis/authen/v1/user_info", + Status: tt.status, + Headers: http.Header{"Content-Type": []string{"application/json"}, "Retry-After": []string{"23"}}, + Body: tt.body, + }) + sdk, err := f.LarkClient() + if err != nil { + t.Fatalf("LarkClient() error = %v", err) + } + + _, _, err = getUserInfo(context.Background(), sdk, "user-token") + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("getUserInfo() error = %T (%v), want *errs.APIError", err, err) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryAPI || problem.Subtype != errs.SubtypeRateLimit { + t.Fatalf("problem = %#v, want api/rate_limit", problem) + } + if apiErr.Code != tt.wantCode || apiErr.Subtype != errs.SubtypeRateLimit { + t.Fatalf("rate limit problem = %#v, want code %d", apiErr.Problem, tt.wantCode) + } + if apiErr.RetryAfterSeconds == nil || *apiErr.RetryAfterSeconds != 23 { + t.Fatalf("RetryAfterSeconds = %v, want 23", apiErr.RetryAfterSeconds) + } + }) + } +} + +func TestGetUserInfo_RateLimitLogIDUsesCompleteStrictBody(t *testing.T) { + tests := []struct { + name string + body string + wantLog string + wantCode int + }{ + {name: "top before nested and header", body: `{"code":99991400,"log_id":"top-log","error":{"log_id":"nested-log"}}`, wantLog: "top-log", wantCode: 99991400}, + {name: "nested before header", body: `{"code":99991400,"error":{"log_id":"nested-log"}}`, wantLog: "nested-log", wantCode: 99991400}, + {name: "header fallback", body: `{"code":99991400}`, wantLog: "header-log", wantCode: 99991400}, + {name: "invalid body log falls back to header", body: `{"code":99991400,"log_id":"bad/log"}`, wantLog: "header-log", wantCode: 99991400}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) + f, _, _, reg := cmdutil.TestFactory(t, &core.CliConfig{ + AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu, + }) + reg.Register(&httpmock.Stub{ + Method: http.MethodGet, + URL: "/open-apis/authen/v1/user_info", + Status: http.StatusTooManyRequests, + RawBody: []byte(tt.body), + Headers: http.Header{"Content-Type": []string{"application/json"}, "X-Tt-Logid": []string{"header-log"}, "X-Request-Id": []string{"request-log"}}, + }) + sdk, err := f.LarkClient() + if err != nil { + t.Fatalf("LarkClient() error = %v", err) + } + _, _, err = getUserInfo(context.Background(), sdk, "user-token") + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("getUserInfo() error = %T (%v), want APIError", err, err) + } + if apiErr.LogID != tt.wantLog || apiErr.Code != tt.wantCode { + t.Fatalf("rate limit metadata = code %d/log %q, want %d/%q", apiErr.Code, apiErr.LogID, tt.wantCode, tt.wantLog) + } + }) + } +} + +func TestGetAppInfo_RateLimitLogIDUsesCompleteStrictBody(t *testing.T) { + tests := []struct { + name string + body string + wantLog string + wantCode int + }{ + {name: "top before nested and header", body: `{"code":99991400,"log_id":"top-log","error":{"log_id":"nested-log"}}`, wantLog: "top-log", wantCode: 99991400}, + {name: "nested before header", body: `{"code":99991400,"error":{"log_id":"nested-log"}}`, wantLog: "nested-log", wantCode: 99991400}, + {name: "header fallback", body: `{"code":99991400}`, wantLog: "header-log", wantCode: 99991400}, + {name: "invalid body log falls back to header", body: `{"code":99991400,"log_id":"bad/log"}`, wantLog: "header-log", wantCode: 99991400}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f, _, _, reg := cmdutil.TestFactory(t, &core.CliConfig{AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu}) + f.Credential = credential.NewCredentialProvider(nil, nil, &authScopesTokenResolver{}, nil) + reg.Register(&httpmock.Stub{ + Method: http.MethodGet, + URL: "/open-apis/application/v6/applications/test-app", + Status: http.StatusTooManyRequests, + RawBody: []byte(tt.body), + Headers: http.Header{"Content-Type": []string{"application/json"}, "X-Tt-Logid": []string{"header-log"}, "X-Request-Id": []string{"request-log"}}, + }) + _, err := getAppInfo(context.Background(), f, "test-app") + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("getAppInfo() error = %T (%v), want APIError", err, err) + } + if apiErr.LogID != tt.wantLog || apiErr.Code != tt.wantCode { + t.Fatalf("rate limit metadata = code %d/log %q, want %d/%q", apiErr.Code, apiErr.LogID, tt.wantCode, tt.wantLog) + } + }) + } +} + type authScopesTokenResolver struct { requests []credential.TokenSpec } diff --git a/cmd/auth/login.go b/cmd/auth/login.go index 6cbbce2811..b9ace01182 100644 --- a/cmd/auth/login.go +++ b/cmd/auth/login.go @@ -350,7 +350,7 @@ func authLoginRun(opts *LoginOptions) error { } openId, userName, err := getUserInfo(opts.Ctx, sdk, result.Token.AccessToken) if err != nil { - return errs.NewAuthenticationError(errs.SubtypeUnknown, "failed to get user info: %v", err).WithCause(err) + return wrapLoginUserInfoError(err) } scopeSummary := loadLoginScopeSummary(config.AppID, openId, finalScope, result.Token.Scope) @@ -433,7 +433,7 @@ func authLoginPollDeviceCode(opts *LoginOptions, config *core.CliConfig, msg *lo } openId, userName, err := getUserInfo(opts.Ctx, sdk, result.Token.AccessToken) if err != nil { - return errs.NewAuthenticationError(errs.SubtypeUnknown, "failed to get user info: %v", err).WithCause(err) + return wrapLoginUserInfoError(err) } scopeSummary := loadLoginScopeSummary(config.AppID, openId, requestedScope, result.Token.Scope) @@ -468,6 +468,13 @@ func authLoginPollDeviceCode(opts *LoginOptions, config *core.CliConfig, msg *lo return nil } +func wrapLoginUserInfoError(err error) error { + if _, ok := errs.ProblemOf(err); ok { + return err + } + return errs.NewAuthenticationError(errs.SubtypeUnknown, "failed to get user info: %v", err).WithCause(err) +} + // syncLoginUserToProfile persists the logged-in user info into the named profile. func syncLoginUserToProfile(profileName, appID, openID, userName string) error { multi, err := core.LoadMultiAppConfig() diff --git a/cmd/auth/login_test.go b/cmd/auth/login_test.go index 50b7f3fc27..b4203c580f 100644 --- a/cmd/auth/login_test.go +++ b/cmd/auth/login_test.go @@ -16,6 +16,9 @@ import ( "strings" "testing" + "github.com/zalando/go-keyring" + + "github.com/larksuite/cli/errs" larkauth "github.com/larksuite/cli/internal/auth" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/core" @@ -25,11 +28,36 @@ import ( "github.com/larksuite/cli/internal/registry" "github.com/larksuite/cli/shortcuts" "github.com/larksuite/cli/shortcuts/common" - "github.com/zalando/go-keyring" ) type failWriter struct{} +func TestWrapLoginUserInfoErrorPassesTypedErrorsThrough(t *testing.T) { + typed := errs.NewAPIError(errs.SubtypeRateLimit, "rate limited"). + WithCode(99991400). + WithRetryable(). + WithRetryAfter(7, "header") + if got := wrapLoginUserInfoError(typed); got != typed { + t.Fatalf("wrapLoginUserInfoError() = %T (%v), want original typed error", got, got) + } + problem, _ := errs.ProblemOf(typed) + if !problem.Retryable || typed.RetryAfterSeconds == nil || *typed.RetryAfterSeconds != 7 { + t.Fatalf("typed recovery metadata changed: %#v", typed) + } +} + +func TestWrapLoginUserInfoErrorWrapsUntypedErrors(t *testing.T) { + cause := errors.New("transport failed") + got := wrapLoginUserInfoError(cause) + problem, ok := errs.ProblemOf(got) + if !ok || problem.Category != errs.CategoryAuthentication || problem.Subtype != errs.SubtypeUnknown { + t.Fatalf("problem = %#v, want authentication/unknown", problem) + } + if !errors.Is(got, cause) { + t.Fatal("wrapped error lost its cause") + } +} + func (failWriter) Write([]byte) (int, error) { return 0, errors.New("write failed") } diff --git a/cmd/config/init_probe.go b/cmd/config/init_probe.go index b6873ac21a..f49a55149c 100644 --- a/cmd/config/init_probe.go +++ b/cmd/config/init_probe.go @@ -38,11 +38,10 @@ const probeTimeout = 3 * time.Second // CategoryConfig / SubtypeInvalidClient, or whatever codemeta maps. That // typed error is propagated so the root dispatcher renders the canonical // envelope and `config init` exits non-zero — identical to how every other -// token-resolving command reports the same bad credentials. Ambiguous -// failures (transport errors, transient 5xx/server_error, JSON parse errors, -// timeouts) come back as raw untyped errors and are swallowed (return nil), -// so valid configurations are never disturbed by upstream noise. -// errs.IsTyped is the discriminator. +// token-resolving command reports the same bad credentials. A retryable +// api/rate_limit error and ambiguous untyped failures (transport errors, +// transient 5xx/server_error, JSON parse errors, timeouts) are swallowed so +// valid configurations are never disturbed by upstream noise. // // 2. If TAT succeeded, a POST to the probe endpoint is fired. The outcome of // that call (success, server error, timeout, parse failure) is always @@ -61,12 +60,12 @@ func runProbe(parent context.Context, factory *cmdutil.Factory, appID, appSecret token, err := credential.FetchTAT(ctx, httpClient, brand, appID, appSecret) if err != nil { - // A typed error from FetchTAT is a deterministic credential rejection - // (classifyTATResponseCode). Propagate it so config init exits with the - // same envelope the rest of the CLI uses for bad credentials. Untyped - // errors are ambiguous (transport / HTTP / parse / timeout) — stay - // silent and let the command succeed. - if errs.IsTyped(err) { + // A retryable API rate limit is expected probe noise. Other typed + // errors are deterministic and must remain visible to the caller. + if ignoreProbeTATError(err) { + return nil + } + if _, ok := errs.ProblemOf(err); ok { return err } return nil @@ -90,3 +89,8 @@ func runProbe(parent context.Context, factory *cmdutil.Factory, appID, appSecret _, _ = io.Copy(io.Discard, resp.Body) return nil } + +func ignoreProbeTATError(err error) bool { + problem, ok := errs.ProblemOf(err) + return ok && problem.Category == errs.CategoryAPI && problem.Subtype == errs.SubtypeRateLimit && problem.Retryable +} diff --git a/cmd/config/init_probe_test.go b/cmd/config/init_probe_test.go index f4156a73b9..74a06732be 100644 --- a/cmd/config/init_probe_test.go +++ b/cmd/config/init_probe_test.go @@ -170,6 +170,42 @@ func TestRunProbe_TATOtherClientError_Propagates(t *testing.T) { } } +func TestRunProbe_TATHTTP429RateLimit_Silent(t *testing.T) { + rt := &fakeRT{ + tatHandler: func(req *http.Request) (*http.Response, error) { + resp := jsonResp(429, `{"code":99991400,"error_description":"do not expose"}`) + resp.Header.Set("Retry-After", "5") + return resp, nil + }, + } + f, errBuf := fakeFactory(t, rt) + assertSilent(t, runProbe(context.Background(), f, "cli_x", "secret_y", core.BrandFeishu), errBuf) + if rt.tatCalls != 1 || rt.probeCalls != 0 { + t.Fatalf("calls = tat %d/probe %d, want 1/0", rt.tatCalls, rt.probeCalls) + } +} + +func TestIgnoreProbeTATError_ExactTypedMatch(t *testing.T) { + tests := []struct { + name string + err error + want bool + }{ + {name: "retryable api rate limit", err: errs.NewAPIError(errs.SubtypeRateLimit, "limited").WithRetryable(), want: true}, + {name: "non retryable rate limit", err: errs.NewAPIError(errs.SubtypeRateLimit, "limited")}, + {name: "wrong subtype", err: errs.NewAPIError(errs.SubtypeUnknown, "unknown").WithRetryable()}, + {name: "wrong category", err: errs.NewNetworkError(errs.SubtypeRateLimit, "limited").WithRetryable()}, + {name: "untyped", err: errors.New("network")}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ignoreProbeTATError(tt.err); got != tt.want { + t.Fatalf("ignoreProbeTATError() = %v, want %v", got, tt.want) + } + }) + } +} + // Non-200 HTTP at the TAT endpoint is ambiguous (not a payload credential // rejection) → silent, exit 0. func TestRunProbe_TATHTTPNon200_Silent(t *testing.T) { diff --git a/cmd/event/runtime.go b/cmd/event/runtime.go index 3f03f4073c..2b581b7515 100644 --- a/cmd/event/runtime.go +++ b/cmd/event/runtime.go @@ -6,6 +6,8 @@ package event import ( "context" "encoding/json" + "fmt" + "net/http" "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/client" @@ -27,36 +29,49 @@ func (r *consumeRuntime) CallAPI(ctx context.Context, method, path string, body }) if err != nil { if _, ok := errs.ProblemOf(err); ok { - return nil, err + return nil, withEventAPIContext(err, method, path) } return nil, errs.NewNetworkError(errs.SubtypeNetworkTransport, "api %s %s: %s", method, path, err).WithCause(err) } - // Non-JSON HTTP errors (gateway text/plain 404 etc.) skip OAPI envelope parsing. + // Event's non-JSON gateway contract includes method/path context and a + // tighter body bound. Preserve that domain-specific fallback, while routing + // HTTP 429 through the shared API classifier first. ct := resp.Header.Get("Content-Type") - if resp.StatusCode >= 400 && !client.IsJSONContentType(ct) && ct != "" { + if resp.StatusCode >= http.StatusBadRequest && !client.IsJSONContentType(ct) && ct != "" { + if resp.StatusCode == http.StatusTooManyRequests { + _, classified := client.ClassifyAPIResponse(resp, nil) + return nil, classified + } const maxBodyEcho = 256 body := string(resp.RawBody) if len(body) > maxBodyEcho { body = body[:maxBodyEcho] + "…(truncated)" } - if resp.StatusCode >= 500 { + if resp.StatusCode >= http.StatusInternalServerError { return nil, errs.NewNetworkError(errs.SubtypeNetworkServer, "api %s %s returned %d: %s", method, path, resp.StatusCode, body).WithRetryable() } return nil, errs.NewInternalError(errs.SubtypeInvalidResponse, "api %s %s returned %d: %s", method, path, resp.StatusCode, body) } - result, err := client.ParseJSONResponse(resp) - if err != nil { - if _, ok := errs.ProblemOf(err); ok { - return nil, err - } - return nil, errs.NewInternalError(errs.SubtypeInvalidResponse, - "api %s %s: %s", method, path, err).WithCause(err) - } - if apiErr := r.client.CheckResponse(result, r.accessIdentity); apiErr != nil { - return json.RawMessage(resp.RawBody), apiErr + _, classified := client.ClassifyAPIResponse(resp, func(result interface{}) error { + return r.client.CheckResponse(result, r.accessIdentity) + }) + if classified != nil { + return json.RawMessage(resp.RawBody), withEventAPIContext(classified, method, path) } return json.RawMessage(resp.RawBody), nil } + +func withEventAPIContext(err error, method, path string) error { + if problem, ok := errs.ProblemOf(err); ok { + if problem.Category == errs.CategoryInternal && problem.Subtype == errs.SubtypeInvalidResponse { + problem.Message = fmt.Sprintf("api %s %s: %s", method, path, problem.Message) + } + if problem.Category == errs.CategoryNetwork && problem.Subtype == errs.SubtypeNetworkServer { + problem.Retryable = true + } + } + return err +} diff --git a/cmd/event/runtime_test.go b/cmd/event/runtime_test.go index 67f2bea1c2..d891bdc728 100644 --- a/cmd/event/runtime_test.go +++ b/cmd/event/runtime_test.go @@ -101,8 +101,22 @@ func TestConsumeRuntimeCallAPI_NonJSONHTTPErrorTruncatesLongBody(t *testing.T) { func TestConsumeRuntimeCallAPI_UnparsableJSONBody(t *testing.T) { r := newTestConsumeRuntime(stubRoundTripper{respond: stubResponse(http.StatusOK, "application/json", "{not json")}) - _, err := r.CallAPI(context.Background(), "GET", "/open-apis/event/v1/connection", nil) + const path = "/open-apis/event/v1/connection" + _, err := r.CallAPI(context.Background(), "GET", path, nil) requireCallAPIProblem(t, err, errs.CategoryInternal, errs.SubtypeInvalidResponse) + if !strings.Contains(err.Error(), "api GET "+path) { + t.Fatalf("parse error lost method/path context: %v", err) + } +} + +func TestConsumeRuntimeCallAPI_JSON5xxMatchesEventRetryabilityContract(t *testing.T) { + r := newTestConsumeRuntime(stubRoundTripper{respond: stubResponse(http.StatusBadGateway, "application/json", `{"code":0}`)}) + _, err := r.CallAPI(context.Background(), "GET", "/open-apis/event/v1/connection", nil) + requireCallAPIProblem(t, err, errs.CategoryNetwork, errs.SubtypeNetworkServer) + problem, _ := errs.ProblemOf(err) + if !problem.Retryable { + t.Fatal("event JSON 5xx must match the existing retryable non-JSON 5xx contract") + } } func TestConsumeRuntimeCallAPI_TransportFailure(t *testing.T) { @@ -134,6 +148,58 @@ func TestConsumeRuntimeCallAPI_EnvelopeErrorIsTyped(t *testing.T) { } } +func TestConsumeRuntimeCallAPI_RateLimitRecoveryMetadata(t *testing.T) { + tests := []struct { + name string + status int + ct string + body string + wantCode int + }{ + {name: "HTTP 429 JSON code zero", status: http.StatusTooManyRequests, ct: "application/json", body: `{"code":0,"msg":"slow"}`, wantCode: 429}, + {name: "HTTP 429 JSON without code", status: http.StatusTooManyRequests, ct: "application/json", body: `{"msg":"slow"}`, wantCode: 429}, + {name: "HTTP 429 non JSON", status: http.StatusTooManyRequests, ct: "text/plain", body: "slow", wantCode: 429}, + {name: "HTTP 429 empty body", status: http.StatusTooManyRequests, body: "", wantCode: 429}, + {name: "business rate limit HTTP 200", status: http.StatusOK, ct: "application/json", body: `{"code":99991400,"msg":"slow"}`, wantCode: 99991400}, + {name: "business rate limit HTTP 400", status: http.StatusBadRequest, ct: "application/json", body: `{"code":99991400,"msg":"slow"}`, wantCode: 99991400}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + calls := 0 + r := newTestConsumeRuntime(stubRoundTripper{respond: func(req *http.Request) (*http.Response, error) { + calls++ + return &http.Response{ + StatusCode: tt.status, + Header: http.Header{ + "Content-Type": []string{tt.ct}, + "Retry-After": []string{"13"}, + }, + Body: io.NopCloser(strings.NewReader(tt.body)), + Request: req, + }, nil + }}) + + _, err := r.CallAPI(context.Background(), "POST", "/open-apis/event/v1/connection", map[string]any{"write": true}) + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("CallAPI() error = %T (%v), want *errs.APIError", err, err) + } + if apiErr.Subtype != errs.SubtypeRateLimit || apiErr.Code != tt.wantCode || !apiErr.Retryable { + t.Fatalf("rate limit problem = %#v, want code %d", apiErr.Problem, tt.wantCode) + } + if apiErr.RetryAfterSeconds == nil || *apiErr.RetryAfterSeconds != 13 || apiErr.RetryAfterSource != "retry-after" { + t.Fatalf("retry metadata = (%v, %q), want (13, retry-after)", apiErr.RetryAfterSeconds, apiErr.RetryAfterSource) + } + if !strings.Contains(apiErr.Hint, "safe to replay") { + t.Fatalf("hint does not warn against unsafe write replay: %q", apiErr.Hint) + } + if calls != 1 { + t.Fatalf("request count = %d, want 1", calls) + } + }) + } +} + func TestConsumeRuntimeCallAPI_Success(t *testing.T) { r := newTestConsumeRuntime(stubRoundTripper{respond: stubResponse(http.StatusOK, "application/json", `{"code":0,"data":{"ok":true}}`)}) diff --git a/cmd/root_test.go b/cmd/root_test.go index 096c404de0..b522af0199 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -707,3 +707,34 @@ func TestApplyNeedAuthorizationHint_AppendsExistingHint(t *testing.T) { t.Errorf("presenter mutated producer hint: %q", authErr.Hint) } } + +func TestRootErrorPresenter_EnrichesPaginationCauseAndRebuildsSnapshot(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) + f, _, _, _ := cmdutil.TestFactory(t, &core.CliConfig{ + AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu, + }) + f.ResolvedIdentity = core.AsUser + root := &cobra.Command{Use: "lark-cli"} + serviceCmd := &cobra.Command{Use: "docs"} + shortcutCmd := &cobra.Command{Use: "+create"} + root.AddCommand(serviceCmd) + serviceCmd.AddCommand(shortcutCmd) + f.CurrentCommand = shortcutCmd + + original := errs.NewPaginationError(newAuthErrorWithNeedAuthMarker(), 1, "resume-page-2") + rendered := presentRootError(f, original, recovery.NewProjector(nil)) + encoded, err := json.Marshal(rendered) + if err != nil { + t.Fatalf("json.Marshal(rendered): %v", err) + } + var wire map[string]interface{} + if err := json.Unmarshal(encoded, &wire); err != nil { + t.Fatalf("json.Unmarshal(rendered): %v", err) + } + if hint, _ := wire["hint"].(string); !strings.Contains(hint, recovery.UserAuthorization("docx:document:create").String()) { + t.Fatalf("pagination error lost authorization hint enhancement: %#v", wire) + } + if wire["completed_pages"] != float64(1) || wire["next_page_token"] != "resume-page-2" { + t.Fatalf("pagination progress = %#v", wire) + } +} diff --git a/cmd/service/service_paginate_test.go b/cmd/service/service_paginate_test.go index 62f77b7c42..e0914855b5 100644 --- a/cmd/service/service_paginate_test.go +++ b/cmd/service/service_paginate_test.go @@ -245,8 +245,15 @@ func TestServicePaginate_StreamingWriteFailureStopsFurtherPages(t *testing.T) { t.Fatalf("servicePaginate() error = %v, want preserved writer cause", err) } problem, ok := errs.ProblemOf(err) - if !ok || problem.Category != errs.CategoryInternal { - t.Fatalf("servicePaginate() problem = %#v, %v; want internal typed error", problem, ok) + if !ok || problem.Category != errs.CategoryInternal || problem.Subtype != errs.SubtypeUnknown { + t.Fatalf("servicePaginate() problem = %#v, %v; want internal/unknown", problem, ok) + } + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) { + t.Fatalf("servicePaginate() error = %T (%v), want PaginationError", err, err) + } + if paginationErr.CompletedPages != 1 || paginationErr.NextPageToken != "next-1" { + t.Fatalf("progress = %d/%q, want 1/next-1", paginationErr.CompletedPages, paginationErr.NextPageToken) } if calls != 2 { t.Fatalf("pagination requests = %d, want 2", calls) @@ -291,7 +298,7 @@ func TestServicePaginate_StreamingFormatFallsBackToJSONWithoutList(t *testing.T) } } -func TestServicePaginate_BusinessErrorsWriteRawAndRemainUnmarked(t *testing.T) { +func TestServicePaginate_BusinessErrorsReturnProgressWithoutStdout(t *testing.T) { businessResponse := map[string]interface{}{ "code": 123456, "msg": "fixture business error", @@ -324,9 +331,19 @@ func TestServicePaginate_BusinessErrorsWriteRawAndRemainUnmarked(t *testing.T) { if errs.IsRaw(err) { t.Fatalf("errs.IsRaw(error) = true, want current servicePaginate pass-through behavior") } - assertServicePaginateJSONBytes(t, out.Bytes(), businessResponse) - if bytes.Contains(out.Bytes(), []byte(`"ok": true`)) { - t.Fatalf("business-error stdout contains a success envelope:\n%s", out.Bytes()) + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) { + t.Fatalf("error = %T (%v), want PaginationError", err, err) + } + if paginationErr.CompletedPages != 0 || paginationErr.NextPageToken != "" { + t.Fatalf("progress = %d/%q, want 0/empty", paginationErr.CompletedPages, paginationErr.NextPageToken) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryAPI || problem.Subtype != errs.SubtypeUnknown || problem.Code != 123456 { + t.Fatalf("ProblemOf = %#v, %v; want api/unknown code 123456", problem, ok) + } + if got := out.String(); got != "" { + t.Fatalf("stdout bytes = %q, want empty", got) } if got := errOut.String(); got != "" { t.Fatalf("stderr bytes = %q, want empty", got) @@ -360,6 +377,17 @@ func TestServicePaginate_TransportErrorsRemainUnmarked(t *testing.T) { if errs.IsRaw(err) { t.Fatalf("errs.IsRaw(error) = true, want current servicePaginate pass-through behavior") } + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) { + t.Fatalf("error = %T (%v), want PaginationError", err, err) + } + if paginationErr.CompletedPages != 0 || paginationErr.NextPageToken != "" { + t.Fatalf("progress = %d/%q, want 0/empty", paginationErr.CompletedPages, paginationErr.NextPageToken) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryNetwork { + t.Fatalf("ProblemOf = %#v, %v; want original network classification", problem, ok) + } if got := out.String(); got != "" { t.Fatalf("stdout bytes = %q, want empty", got) } @@ -370,6 +398,38 @@ func TestServicePaginate_TransportErrorsRemainUnmarked(t *testing.T) { } } +func TestServicePaginate_DecodeErrorReturnsProgressWithoutStdout(t *testing.T) { + ac, out, errOut, reg := newServicePaginateTestHarness(t) + reg.Register(&httpmock.Stub{ + URL: "/open-apis/test/v1/items", + RawBody: []byte(`{"code":`), + }) + + err := servicePaginate(context.Background(), ac, servicePaginateRequest(), + output.FormatJSON, "", out, errOut, "lark-cli test items list", + client.PaginationOptions{PageDelay: -1}, ac.CheckResponse) + if err == nil { + t.Fatal("servicePaginate() error = nil, want decode error") + } + if errs.IsRaw(err) { + t.Fatalf("errs.IsRaw(error) = true, want service pass-through behavior") + } + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) { + t.Fatalf("error = %T (%v), want PaginationError", err, err) + } + if paginationErr.CompletedPages != 0 || paginationErr.NextPageToken != "" { + t.Fatalf("progress = %d/%q, want 0/empty", paginationErr.CompletedPages, paginationErr.NextPageToken) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryInternal || problem.Subtype != errs.SubtypeInvalidResponse { + t.Fatalf("ProblemOf = %#v, %v; want internal/invalid_response", problem, ok) + } + if out.Len() != 0 || errOut.Len() != 0 { + t.Fatalf("decode error wrote stdout/stderr: %q / %q", out.String(), errOut.String()) + } +} + func TestServicePaginate_StreamBusinessErrorRemainsUnmarked(t *testing.T) { ac, out, errOut, reg := newServicePaginateTestHarness(t) reg.Register(&httpmock.Stub{ @@ -391,6 +451,10 @@ func TestServicePaginate_StreamBusinessErrorRemainsUnmarked(t *testing.T) { if errs.IsRaw(err) { t.Fatalf("errs.IsRaw(error) = true, want current servicePaginate pass-through behavior") } + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) || paginationErr.CompletedPages != 0 || paginationErr.NextPageToken != "" { + t.Fatalf("pagination progress = %#v, want 0 completed pages and no token", paginationErr) + } if got := out.String(); got != "" { t.Fatalf("stdout bytes = %q, want empty", got) } diff --git a/cmd/service/service_test.go b/cmd/service/service_test.go index fceae019df..658136ec2c 100644 --- a/cmd/service/service_test.go +++ b/cmd/service/service_test.go @@ -743,7 +743,7 @@ func TestServiceMethod_BusinessErrorReturnsTypedErrorWithoutSuccessEnvelope(t *t } } -func TestServiceMethod_PageAll_DefaultBusinessErrorOutputsRawResponse(t *testing.T) { +func TestServiceMethod_PageAll_DefaultBusinessErrorReturnsPaginationProgress(t *testing.T) { f, stdout, _, reg := cmdutil.TestFactory(t, &core.CliConfig{ AppID: "test-app-service-pageall-err", AppSecret: "test-secret-service-pageall-err", Brand: core.BrandFeishu, }) @@ -765,11 +765,15 @@ func TestServiceMethod_PageAll_DefaultBusinessErrorOutputsRawResponse(t *testing t.Fatal("expected error for non-zero code") } requireProblem(t, err, errs.CategoryAuthorization, errs.SubtypeUserUnauthorized, 230027) - if !strings.Contains(stdout.String(), "230027") || !strings.Contains(stdout.String(), "user not authorized") { - t.Fatalf("expected raw error response on stdout, got: %s", stdout.String()) + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) { + t.Fatalf("expected PaginationError, got %T: %v", err, err) } - if strings.Contains(stdout.String(), `"ok": true`) || strings.Contains(stdout.String(), `"ok":true`) { - t.Fatalf("unexpected success envelope on error path: %s", stdout.String()) + if paginationErr.CompletedPages != 0 || paginationErr.NextPageToken != "" { + t.Fatalf("progress = %d/%q, want 0/empty", paginationErr.CompletedPages, paginationErr.NextPageToken) + } + if stdout.Len() != 0 { + t.Fatalf("pagination error wrote stdout: %s", stdout.String()) } } @@ -1007,7 +1011,7 @@ func TestServiceMethod_PageAll_WithJq(t *testing.T) { } } -func TestServiceMethod_PageAll_WithJqBusinessErrorOutputsRawResponse(t *testing.T) { +func TestServiceMethod_PageAll_WithJqBusinessErrorReturnsPaginationProgress(t *testing.T) { f, stdout, _, reg := cmdutil.TestFactory(t, &core.CliConfig{ AppID: "test-app-spjq-err", AppSecret: "test-secret-spjq-err", Brand: core.BrandFeishu, }) @@ -1033,11 +1037,15 @@ func TestServiceMethod_PageAll_WithJqBusinessErrorOutputsRawResponse(t *testing. if !errors.As(err, &permErr) { t.Fatalf("expected PermissionError, got %T: %v", err, err) } - if !strings.Contains(stdout.String(), "230027") || !strings.Contains(stdout.String(), "user not authorized") { - t.Fatalf("expected raw error response on stdout, got: %s", stdout.String()) + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) { + t.Fatalf("expected PaginationError, got %T: %v", err, err) } - if strings.Contains(stdout.String(), `"ok": true`) || strings.Contains(stdout.String(), `"ok":true`) { - t.Fatalf("unexpected success envelope on error path: %s", stdout.String()) + if paginationErr.CompletedPages != 0 || paginationErr.NextPageToken != "" { + t.Fatalf("progress = %d/%q, want 0/empty", paginationErr.CompletedPages, paginationErr.NextPageToken) + } + if stdout.Len() != 0 { + t.Fatalf("pagination error wrote stdout: %s", stdout.String()) } } diff --git a/errs/ERROR_CONTRACT.md b/errs/ERROR_CONTRACT.md index 196063db29..c0e0937014 100644 --- a/errs/ERROR_CONTRACT.md +++ b/errs/ERROR_CONTRACT.md @@ -67,16 +67,6 @@ Typed errors render to **stderr** as one JSON object per process exit: | `error.params` | per-Subtype-stable | per-parameter validation detail array (`ValidationError`); see **Validation parameters** | | per-Subtype extension fields | per-Subtype-stable | e.g. `missing_scopes`, `console_url`, `challenge_url`; `console_url` is emitted for developer/admin recovery such as `app_scope_not_applied`, not user `missing_scope` | -For retryable `type=api, subtype=rate_limit`, the CLI may emit -`retry_after_seconds` when the upstream response supplies a precise delay. For -TAT HTTP 429 responses, the delay uses Lark's `x-ogw-ratelimit-reset` header, -then a numeric `Retry-After` value. If neither header contains a valid delay, -the field is omitted and the hint recommends exponential backoff with jitter. -The envelope intentionally does not expose an implementation detail such as -`retry_after_source`, and the CLI does not automatically replay the request. -HTTP 429 classification is currently added only to TAT fetching; other API -transports retain their existing behavior. - `SecurityPolicyError` renders through the same typed envelope as every other category. `error.type` is `"policy"`, `error.subtype` is one of `challenge_required` / `access_denied`, and process exit is `6` via @@ -456,6 +446,73 @@ A `params` wire example (multiple parameters each carrying a reason): } ``` +#### Unsupported execution identity + +`validation/identity_not_supported` means the CLI knows from command or +Shortcut metadata that the selected `user` or `bot` identity cannot execute +the command. Consumers may branch on this subtype; they must not parse the +message to detect the condition. + +When the user explicitly selected an incompatible identity, the error carries +`param: "--as"`. When the identity came from auto-detection, `default-as`, or +another implicit policy, `param` is omitted because `--as` was not the failing +user input. The hint lists the identities the command supports and recommends +`--as` only conditionally: the corresponding authorization must already be +configured. If no supported identity is known, the producer omits the hint +rather than suggesting an unusable value. + +This subtype is produced only from a known local identity boundary. Raw API +responses are not reclassified from message text or broad numeric codes: +`230027` remains `authorization/user_unauthorized`, `99991663` remains +`authentication/token_invalid`, and `20008` retains its existing API +classification. + +#### Short-term rate limits + +HTTP `429` and Lark business code `99991400` produce +`api/rate_limit` with `retryable: true`. They also carry +`retry_after_seconds` and `retry_after_source`. Lark's bounded numeric +`X-Ogw-Ratelimit-Reset` takes precedence, followed by a valid standard +`Retry-After` delta or HTTP date. If neither is usable, the source is `default` +and the suggested delay is one second. Values that are negative, expired, +malformed, larger than 86400 seconds, or overflow an integer are ignored. + +`retryable: true` describes a short-lived server response. It does **not** +mean the original request—especially a write—is safe to replay. The CLI never +sleeps or automatically retries these requests. A caller should wait for the +suggested interval, reevaluate the operation, and retry only after verifying +its result or idempotency guarantee. `retry_after_seconds` is recovery advice, +not authorization to repeat an operation. + +An HTTP 429 without business code uses `code: 429`. If the response contains +business code `99991400`, that code is preserved. + +#### Pagination progress on failure + +When a paginated request fails, the final typed error preserves the original +category, subtype, code, recovery extensions, and cause chain. It adds: + +- `completed_pages`: the number of pages successfully checked and accepted + before the failure; always present and possibly zero. +- `next_page_token`: the cursor used to request the failed page; omitted when + the failed first page had no starting cursor. + +The failed response is never added to the accumulated result. Aggregate output +therefore cannot turn partial data into a success envelope. Streaming formats +cannot retract pages already written, but the command returns the typed error +and a non-zero exit code so callers can detect that the stream is incomplete. +If a page callback fails, that page is not counted as completed and its request +cursor is the resume token. If the context is canceled during the delay between +successful pages, the next page token already returned by the API is the resume +token. Callback and cancellation causes are always normalized to typed errors +and remain reachable through `errors.Is` / `errors.As`. + +`next_page_token` is recovery data only. Producers must not copy it into +messages, hints, progress logs, or debug output. Pagination progress is added +by transparently marshaling the complete underlying typed error; a collision +with either reserved progress field fails closed as an internal serialization +error instead of overwriting data. + ### Constructing typed errors Prefer the **builder API**. The constructor pins `Category` + `Subtype` + diff --git a/errs/marshal_test.go b/errs/marshal_test.go index b970602e21..39882b0153 100644 --- a/errs/marshal_test.go +++ b/errs/marshal_test.go @@ -155,11 +155,11 @@ func TestNetworkError_MarshalJSON(t *testing.T) { } } -func TestAPIError_MarshalJSON(t *testing.T) { - ae := &APIError{ - Problem: Problem{Category: CategoryAPI, Subtype: SubtypeRateLimit, Code: 99991400, Message: "slow", Retryable: true}, - RetryAfterSeconds: 12, - } +func TestAPIError_RateLimitMarshalJSON(t *testing.T) { + ae := NewAPIError(SubtypeRateLimit, "slow"). + WithCode(99991400). + WithRetryable(). + WithRetryAfter(45, "retry-after") b, _ := json.Marshal(ae) s := string(b) for _, want := range []string{ @@ -167,20 +167,17 @@ func TestAPIError_MarshalJSON(t *testing.T) { `"subtype":"rate_limit"`, `"code":99991400`, `"retryable":true`, - `"retry_after_seconds":12`, + `"retry_after_seconds":45`, + `"retry_after_source":"retry-after"`, } { if !strings.Contains(s, want) { t.Errorf("missing %q in %s", want, s) } } - if strings.Contains(s, `"retry_after_source"`) { - t.Errorf("implementation detail retry_after_source must not be emitted: %s", s) - } - ae.RetryAfterSeconds = 0 - b, _ = json.Marshal(ae) - if strings.Contains(string(b), `"retry_after_seconds"`) { - t.Errorf("retry_after_seconds must be omitted when no precise delay was provided: %s", b) + bare, _ := json.Marshal(NewAPIError(SubtypeUnknown, "x")) + if strings.Contains(string(bare), "retry_after") { + t.Fatalf("unset retry-after fields must be omitted: %s", bare) } } diff --git a/errs/pagination.go b/errs/pagination.go new file mode 100644 index 0000000000..2c94086296 --- /dev/null +++ b/errs/pagination.go @@ -0,0 +1,139 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package errs + +import ( + "encoding/json" + "errors" +) + +const ( + paginationCompletedPagesField = "completed_pages" + paginationNextPageTokenField = "next_page_token" +) + +// PaginationError adds resumable pagination progress to an existing typed +// error without changing its category, subtype, extension fields, or cause +// chain. The failed page's response is never represented as completed work. +type PaginationError struct { + Problem + Cause error `json:"-"` + CompletedPages int `json:"-"` + NextPageToken string `json:"-"` + encoded []byte +} + +// NewPaginationError validates and snapshots the underlying typed error's +// complete JSON object before returning the wrapper. If the underlying object +// already owns a reserved progress field, it returns a serializable typed +// internal error instead of creating a wrapper whose MarshalJSON would fail at +// the final stderr boundary. +func NewPaginationError(cause error, completedPages int, nextPageToken string) error { + problem, ok := ProblemOf(cause) + if !ok { + return NewInternalError(SubtypeUnknown, + "cannot attach pagination progress to an untyped error").WithCause(cause) + } + paginationErr := &PaginationError{ + Problem: *problem, + Cause: cause, + CompletedPages: completedPages, + NextPageToken: nextPageToken, + } + encoded, err := paginationErr.buildJSON() + if err != nil { + return err + } + paginationErr.encoded = encoded + return paginationErr +} + +func (e *PaginationError) Error() string { + if e == nil { + return "" + } + if e.Cause == nil { + return "" + } + return e.Cause.Error() +} + +func (e *PaginationError) Unwrap() error { + if e == nil { + return nil + } + return e.Cause +} + +func (e *PaginationError) ProblemDetail() *Problem { + if e == nil { + return nil + } + return &e.Problem +} + +// IsPagination reports whether err contains pagination recovery progress. +func IsPagination(err error) bool { + var paginationErr *PaginationError + return errors.As(err, &paginationErr) +} + +func (e *PaginationError) MarshalJSON() ([]byte, error) { + if e != nil && e.encoded != nil { + return append([]byte(nil), e.encoded...), nil + } + return e.buildJSON() +} + +func (e *PaginationError) buildJSON() ([]byte, error) { + if e == nil || e.Cause == nil { + return nil, NewInternalError(SubtypeUnknown, "cannot serialize pagination progress without an underlying error") + } + typed, ok := UnwrapTypedError(e.Cause) + if !ok { + return nil, NewInternalError(SubtypeUnknown, + "cannot serialize pagination progress for untyped error").WithCause(e.Cause) + } + encoded, err := json.Marshal(typed) + if err != nil { + return nil, NewInternalError(SubtypeInvalidResponse, + "failed to serialize underlying pagination error: %v", err).WithCause(e.Cause) + } + var fields map[string]json.RawMessage + if err := json.Unmarshal(encoded, &fields); err != nil { + return nil, NewInternalError(SubtypeInvalidResponse, + "underlying pagination error did not serialize as a JSON object: %v", err).WithCause(e.Cause) + } + if fields == nil { + return nil, NewInternalError(SubtypeInvalidResponse, + "underlying pagination error did not serialize as a JSON object").WithCause(e.Cause) + } + for _, reserved := range []string{paginationCompletedPagesField, paginationNextPageTokenField} { + if _, exists := fields[reserved]; exists { + return nil, NewInternalError(SubtypeInvalidResponse, + "cannot add pagination progress: underlying error already contains reserved field %q", reserved).WithCause(e.Cause) + } + } + + completed, err := json.Marshal(e.CompletedPages) + if err != nil { + return nil, NewInternalError(SubtypeInvalidResponse, + "failed to serialize completed pagination page count: %v", err).WithCause(e.Cause) + } + fields[paginationCompletedPagesField] = completed + if e.NextPageToken != "" { + next, err := json.Marshal(e.NextPageToken) + if err != nil { + return nil, NewInternalError(SubtypeInvalidResponse, + "failed to serialize pagination resume token: %v", err).WithCause(e.Cause) + } + fields[paginationNextPageTokenField] = next + } + encoded, err = json.Marshal(fields) + if err != nil { + return nil, NewInternalError(SubtypeInvalidResponse, + "failed to serialize pagination progress: %v", err).WithCause(e.Cause) + } + return encoded, nil +} diff --git a/errs/pagination_test.go b/errs/pagination_test.go new file mode 100644 index 0000000000..a7ba3f31bf --- /dev/null +++ b/errs/pagination_test.go @@ -0,0 +1,158 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package errs_test + +import ( + "encoding/json" + "errors" + "testing" + + "github.com/larksuite/cli/errs" +) + +func TestPaginationError_PreservesTypedErrorAndAddsProgress(t *testing.T) { + sentinel := errors.New("permission cause") + inner := errs.NewPermissionError(errs.SubtypeMissingScope, "missing scope"). + WithCode(99991679). + WithHint("authorize the scope"). + WithLogID("log-1"). + WithMissingScopes("drive:drive:readonly"). + WithIdentity("user"). + WithConsoleURL("https://open.feishu.cn/app/cli_test"). + WithCause(sentinel) + wrapped := errs.NewPaginationError(inner, 3, "resume-page-4") + + if !errors.Is(wrapped, sentinel) { + t.Fatal("errors.Is did not preserve the inner cause chain") + } + var permissionErr *errs.PermissionError + if !errors.As(wrapped, &permissionErr) || permissionErr != inner { + t.Fatalf("errors.As = %#v, want original PermissionError", permissionErr) + } + problem, ok := errs.ProblemOf(wrapped) + if !ok || *problem != inner.Problem { + t.Fatalf("ProblemOf = %#v, %v; want preserved Problem", problem, ok) + } + if !errs.IsPagination(wrapped) { + t.Fatal("IsPagination = false, want true") + } + + encoded, err := json.Marshal(wrapped) + if err != nil { + t.Fatalf("json.Marshal: %v", err) + } + var got map[string]any + if err := json.Unmarshal(encoded, &got); err != nil { + t.Fatalf("json.Unmarshal: %v", err) + } + want := map[string]any{ + "type": "authorization", + "subtype": "missing_scope", + "code": float64(99991679), + "message": "missing scope", + "hint": "authorize the scope", + "log_id": "log-1", + "identity": "user", + "console_url": "https://open.feishu.cn/app/cli_test", + "completed_pages": float64(3), + "next_page_token": "resume-page-4", + "missing_scopes_size": 1, + } + for field, wantValue := range want { + if field == "missing_scopes_size" { + scopes, ok := got["missing_scopes"].([]any) + if !ok || len(scopes) != 1 || scopes[0] != "drive:drive:readonly" { + t.Errorf("missing_scopes = %#v", got["missing_scopes"]) + } + continue + } + if got[field] != wantValue { + t.Errorf("field %q = %#v, want %#v", field, got[field], wantValue) + } + } +} + +func TestPaginationError_PreservesRateLimitExtensions(t *testing.T) { + inner := errs.NewAPIError(errs.SubtypeRateLimit, "slow down"). + WithCode(99991400). + WithRetryable(). + WithRetryAfter(20, "retry-after") + wrapped := errs.NewPaginationError(inner, 1, "next") + + encoded, err := json.Marshal(wrapped) + if err != nil { + t.Fatalf("json.Marshal: %v", err) + } + var got map[string]any + if err := json.Unmarshal(encoded, &got); err != nil { + t.Fatalf("json.Unmarshal: %v", err) + } + if got["retryable"] != true || got["retry_after_seconds"] != float64(20) || got["retry_after_source"] != "retry-after" { + t.Fatalf("rate-limit extensions lost: %#v", got) + } +} + +func TestPaginationError_ZeroProgressAndOmittedToken(t *testing.T) { + inner := errs.NewNetworkError(errs.SubtypeNetworkTransport, "offline") + wrapped := errs.NewPaginationError(inner, 0, "") + + encoded, err := json.Marshal(wrapped) + if err != nil { + t.Fatalf("json.Marshal: %v", err) + } + var got map[string]any + if err := json.Unmarshal(encoded, &got); err != nil { + t.Fatalf("json.Unmarshal: %v", err) + } + if got["completed_pages"] != float64(0) { + t.Fatalf("completed_pages = %#v, want 0", got["completed_pages"]) + } + if _, ok := got["next_page_token"]; ok { + t.Fatalf("next_page_token unexpectedly present: %#v", got) + } +} + +type collidingPaginationError struct { + problem errs.Problem +} + +func (e *collidingPaginationError) Error() string { return e.problem.Message } +func (e *collidingPaginationError) ProblemDetail() *errs.Problem { return &e.problem } +func (e *collidingPaginationError) MarshalJSON() ([]byte, error) { + return []byte(`{"type":"api","subtype":"unknown","message":"collision","completed_pages":99}`), nil +} + +func TestPaginationError_FieldCollisionFailsClosed(t *testing.T) { + inner := &collidingPaginationError{problem: errs.Problem{ + Category: errs.CategoryAPI, + Subtype: errs.SubtypeUnknown, + Message: "collision", + }} + wrapped := errs.NewPaginationError(inner, 1, "next") + problem, ok := errs.ProblemOf(wrapped) + if !ok || problem.Category != errs.CategoryInternal || problem.Subtype != errs.SubtypeInvalidResponse { + t.Fatalf("ProblemOf = %#v, %v; want internal/invalid_response collision error", problem, ok) + } + if !errors.Is(wrapped, inner) { + t.Fatal("collision fallback did not preserve the original typed cause") + } + var recovered *collidingPaginationError + if !errors.As(wrapped, &recovered) || recovered != inner { + t.Fatalf("errors.As = %#v, want original colliding error", recovered) + } + encoded, err := json.Marshal(wrapped) + if err != nil { + t.Fatalf("collision fallback must remain JSON-serializable: %v", err) + } + var got map[string]any + if err := json.Unmarshal(encoded, &got); err != nil { + t.Fatalf("json.Unmarshal: %v", err) + } + if got["type"] != "internal" || got["subtype"] != "invalid_response" { + t.Fatalf("collision fallback JSON = %#v, want internal/invalid_response", got) + } + if _, exists := got["completed_pages"]; exists { + t.Fatalf("collision fallback must not overwrite reserved fields: %#v", got) + } +} diff --git a/errs/subtypes.go b/errs/subtypes.go index a3968ac2d2..847f465d3a 100644 --- a/errs/subtypes.go +++ b/errs/subtypes.go @@ -12,9 +12,10 @@ const ( // CategoryValidation subtypes const ( - SubtypeInvalidArgument Subtype = "invalid_argument" // user-supplied flag / arg failed validation (gRPC INVALID_ARGUMENT alignment) - SubtypeFailedPrecondition Subtype = "failed_precondition" // request is valid but the system/resource state is not in the state required to execute; caller must change state (not retry) — e.g. ambiguous remote mapping (gRPC FAILED_PRECONDITION alignment) - SubtypeCommandUnavailable Subtype = "command_unavailable" // command not included in this build (integrator-restricted distribution); absent, not gated + SubtypeInvalidArgument Subtype = "invalid_argument" // user-supplied flag / arg failed validation (gRPC INVALID_ARGUMENT alignment) + SubtypeFailedPrecondition Subtype = "failed_precondition" // request is valid but the system/resource state is not in the state required to execute; caller must change state (not retry) — e.g. ambiguous remote mapping (gRPC FAILED_PRECONDITION alignment) + SubtypeIdentityNotSupported Subtype = "identity_not_supported" // selected user/bot identity is outside the command's declared supported identities + SubtypeCommandUnavailable Subtype = "command_unavailable" // command not included in this build (integrator-restricted distribution); absent, not gated ) // CategoryAuthentication subtypes diff --git a/errs/types.go b/errs/types.go index 751042bf7a..a199e222f9 100644 --- a/errs/types.go +++ b/errs/types.go @@ -444,10 +444,13 @@ func (e *NetworkError) WithCause(cause error) *NetworkError { // errors.Is / errors.Unwrap; it is intentionally not serialized. type APIError struct { Problem - // RetryAfterSeconds is an upstream-provided minimum delay before another - // attempt. Zero means no precise delay was provided and omits the field. - RetryAfterSeconds int `json:"retry_after_seconds,omitempty"` - Cause error `json:"-"` + // RetryAfterSeconds is the bounded delay recommended before another + // attempt. A nil value omits recovery timing from the wire envelope. + RetryAfterSeconds *int `json:"retry_after_seconds,omitempty"` + // RetryAfterSource records whether the delay came from a response header or + // the classifier's conservative default. + RetryAfterSource string `json:"retry_after_source,omitempty"` + Cause error `json:"-"` } // Unwrap is nil-receiver safe; see ValidationError.Unwrap. @@ -496,6 +499,12 @@ func (e *APIError) WithRetryable() *APIError { return e } +func (e *APIError) WithRetryAfter(seconds int, source string) *APIError { + e.RetryAfterSeconds = &seconds + e.RetryAfterSource = source + return e +} + func (e *APIError) WithCause(cause error) *APIError { e.Cause = cause return e diff --git a/errs/types_test.go b/errs/types_test.go index 8279c2e46b..1c37ffe3b1 100644 --- a/errs/types_test.go +++ b/errs/types_test.go @@ -311,6 +311,12 @@ func TestWithHint_PrintfFormat(t *testing.T) { } } +func TestSubtypeIdentityNotSupported_WireValue(t *testing.T) { + if got, want := errs.SubtypeIdentityNotSupported, errs.Subtype("identity_not_supported"); got != want { + t.Errorf("SubtypeIdentityNotSupported = %q, want %q", got, want) + } +} + // TestPermissionError_FullChain verifies the most field-heavy typed error // constructs cleanly via the chain. func TestPermissionError_FullChain(t *testing.T) { diff --git a/internal/client/api_response.go b/internal/client/api_response.go new file mode 100644 index 0000000000..e8d6d760e7 --- /dev/null +++ b/internal/client/api_response.go @@ -0,0 +1,145 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package client + +import ( + "errors" + "net/http" + "strings" + "time" + + larkcore "github.com/larksuite/oapi-sdk-go/v3/core" + + "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/internal/errclass" + "github.com/larksuite/cli/internal/util" +) + +// APIResponseChecker classifies a successfully decoded Lark API envelope. +// It should return nil for a successful business response. +type APIResponseChecker func(result interface{}) error + +// ClassifyAPIResponse is the common response boundary for buffered Lark API +// calls. It decodes the JSON body once for normal responses, applies the +// caller's business-error classifier, gives explicit rate limits precedence, +// and finally rejects otherwise-unclassified HTTP errors. +// +// The decoded result is returned alongside a business error because some +// callers need response data from failed operations. +func ClassifyAPIResponse(resp *larkcore.ApiResp, check APIResponseChecker) (interface{}, error) { + if resp == nil { + return nil, errs.NewInternalError(errs.SubtypeInvalidResponse, "API returned a nil response") + } + + result, parseErr := ParseJSONResponse(resp) + if parseErr != nil { + if resp.StatusCode >= http.StatusBadRequest { + return nil, classifyAPIResponseError(resp, nil, nil) + } + return nil, WrapJSONResponseParseError(parseErr, resp.RawBody) + } + + var classified error + if check != nil { + classified = check(result) + } + if err := classifyAPIResponseError(resp, result, classified); err != nil { + return result, err + } + return result, nil +} + +// classifyAPIResponseError resolves the precedence between HTTP rate limits, +// business-envelope errors, and bare HTTP status failures. The raw body is +// strictly revalidated only when a rate-limit signal is present, preventing a +// malformed JSON prefix from forging trusted recovery metadata without adding +// a second decode to ordinary successful responses. +func classifyAPIResponseError(resp *larkcore.ApiResp, result interface{}, classified error) error { + if resp == nil { + return errs.NewInternalError(errs.SubtypeInvalidResponse, "API returned a nil response") + } + + businessRateLimit := errclass.IsBusinessRateLimit(result) + classifiedRateLimit := isClassifiedBusinessRateLimit(classified) + if resp.StatusCode == http.StatusTooManyRequests || businessRateLimit || classifiedRateLimit { + if len(resp.RawBody) > 0 { + parsed, parseErr := errclass.DecodeSingleJSON(resp.RawBody) + if parseErr != nil { + result = nil + classified = nil + if resp.StatusCode != http.StatusTooManyRequests { + return errs.NewInternalError(errs.SubtypeInvalidResponse, + "failed to validate candidate rate-limit response: %v", parseErr).WithCause(parseErr) + } + } else { + result = parsed + businessRateLimit = errclass.IsBusinessRateLimit(result) + if resp.StatusCode != http.StatusTooManyRequests && !businessRateLimit { + return errs.NewInternalError(errs.SubtypeInvalidResponse, + "candidate rate-limit classification does not match the response body") + } + } + } + + if resp.StatusCode == http.StatusTooManyRequests { + return errclass.ClassifyHTTPRateLimit(resp.StatusCode, resp.Header, result, classified, time.Now()) + } + + var apiErr *errs.APIError + var existing *errs.APIError + if errors.As(classified, &existing) { + apiErr = existing + } + if apiErr == nil { + apiErr = errs.NewAPIError(errs.SubtypeRateLimit, errclass.RateLimitMessage).WithCode(99991400) + } + apiErr.Subtype = errs.SubtypeRateLimit + apiErr.Code = 99991400 + apiErr.Message = errclass.RateLimitMessage + apiErr.LogID = errclass.RateLimitLogID(result, resp.Header) + + seconds, source := errclass.ParseRetryAfter(resp.Header, time.Now()) + apiErr.Hint = errclass.MergeRateLimitHint(apiErr.Hint, errclass.RateLimitGuidance(seconds)) + return apiErr.WithRetryable().WithRetryAfter(seconds, source) + } + + if classified != nil { + return classified + } + if resp.StatusCode >= http.StatusBadRequest { + return httpStatusError(resp.StatusCode, resp.RawBody, resp.Header) + } + return nil +} + +func isClassifiedBusinessRateLimit(classified error) bool { + problem, ok := errs.ProblemOf(classified) + return ok && problem.Code == 99991400 +} + +// httpStatusError classifies an HTTP error whose body carries no usable +// business error. Header request IDs are attached when they pass the same +// validation used for rate-limit metadata. +func httpStatusError(status int, rawBody []byte, header http.Header) error { + body := util.TruncateStrWithEllipsis(strings.TrimSpace(string(rawBody)), 500) + logID := errclass.RateLimitLogID(nil, header) + if status >= http.StatusInternalServerError { + err := errs.NewNetworkError(errs.SubtypeNetworkServer, + "HTTP %d: %s", status, body). + WithCode(status) + if logID != "" { + err = err.WithLogID(logID) + } + return err + } + subtype := errs.SubtypeUnknown + if status == http.StatusNotFound { + subtype = errs.SubtypeNotFound + } + err := errs.NewAPIError(subtype, "HTTP %d: %s", status, body).WithCode(status) + if logID != "" { + err = err.WithLogID(logID) + } + return err +} diff --git a/internal/client/api_response_test.go b/internal/client/api_response_test.go new file mode 100644 index 0000000000..be915820da --- /dev/null +++ b/internal/client/api_response_test.go @@ -0,0 +1,231 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package client + +import ( + "encoding/json" + "errors" + "net/http" + "strings" + "testing" + + larkcore "github.com/larksuite/oapi-sdk-go/v3/core" + + "github.com/larksuite/cli/errs" +) + +func TestClassifyAPIResponse_DecodesOnceAndReturnsBusinessErrorWithResult(t *testing.T) { + resp := &larkcore.ApiResp{ + StatusCode: http.StatusOK, + RawBody: []byte(`{"code":123,"msg":"failed","data":{"token":"keep"}}`), + } + wantErr := errs.NewAPIError(errs.SubtypeUnknown, "failed").WithCode(123) + checks := 0 + + result, err := ClassifyAPIResponse(resp, func(result interface{}) error { + checks++ + return wantErr + }) + if err != wantErr { + t.Fatalf("ClassifyAPIResponse() error = %T (%v), want original business error", err, err) + } + if checks != 1 { + t.Fatalf("business checker calls = %d, want 1", checks) + } + resultMap, ok := result.(map[string]interface{}) + if !ok || resultMap["data"] == nil { + t.Fatalf("ClassifyAPIResponse() result = %#v, want decoded failure envelope", result) + } +} + +func TestClassifyAPIResponse_BareHTTP429PrecedesJSONParseFailure(t *testing.T) { + resp := &larkcore.ApiResp{ + StatusCode: http.StatusTooManyRequests, + Header: http.Header{"Retry-After": []string{"7"}}, + RawBody: []byte("rate limit exceeded"), + } + + _, err := ClassifyAPIResponse(resp, nil) + problem, ok := errs.ProblemOf(err) + var apiErr *errs.APIError + if !ok || !errors.As(err, &apiErr) || problem.Category != errs.CategoryAPI || + problem.Subtype != errs.SubtypeRateLimit || !problem.Retryable || + apiErr.RetryAfterSeconds == nil || *apiErr.RetryAfterSeconds != 7 { + t.Fatalf("problem = %#v, want retryable api/rate_limit with retry_after_seconds=7", problem) + } +} + +func TestClassifyAPIResponse_BareHTTP5xxDoesNotChangeRetryabilityContract(t *testing.T) { + resp := &larkcore.ApiResp{ + StatusCode: http.StatusBadGateway, + RawBody: []byte("bad gateway"), + } + + _, err := ClassifyAPIResponse(resp, nil) + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryNetwork || problem.Subtype != errs.SubtypeNetworkServer { + t.Fatalf("problem = %#v, want network/server_error", problem) + } + if problem.Retryable { + t.Fatal("shared buffered 5xx classification must preserve the existing non-retryable contract") + } +} + +func TestClassifyAPIResponseError_HTTP429PreservesExplicitLongTermQuota(t *testing.T) { + original := errs.NewAPIError(errs.SubtypeRateLimit, "daily quota exceeded"). + WithCode(1063006). + WithHint("this operation is limited to 5 times per day") + resp := &larkcore.ApiResp{StatusCode: http.StatusTooManyRequests, Header: http.Header{"Retry-After": []string{"9"}}} + result := map[string]interface{}{"code": float64(1063006), "msg": "daily quota exceeded"} + + got := classifyAPIResponseError(resp, result, original) + if !errors.Is(got, original) || got != original { + t.Fatalf("classifyAPIResponseError() = %T %v, want original error unchanged", got, got) + } + if original.Retryable || original.RetryAfterSeconds != nil { + t.Fatalf("long-term quota was incorrectly decorated as short-term retryable: %#v", original) + } +} + +func TestClassifyAPIResponseError_MergesExistingHint(t *testing.T) { + original := errs.NewAPIError(errs.SubtypeRateLimit, "slow"). + WithCode(99991400). + WithHint("server says slow down") + resp := &larkcore.ApiResp{StatusCode: http.StatusTooManyRequests, Header: http.Header{"Retry-After": []string{"9"}}} + result := map[string]interface{}{"code": float64(99991400), "msg": "slow"} + + got := classifyAPIResponseError(resp, result, original) + if got != original { + t.Fatalf("classifyAPIResponseError() did not preserve the classified APIError pointer") + } + if !strings.HasPrefix(original.Hint, "server says slow down;") || !strings.Contains(original.Hint, "safe to replay") { + t.Fatalf("merged hint = %q, want server hint followed by replay guidance", original.Hint) + } +} + +func TestClassifyAPIResponseError_BackfillsExistingAPIErrorLogID(t *testing.T) { + tests := []struct { + name string + existingID string + result interface{} + want string + }{ + {name: "header fills empty", want: "header-log"}, + {name: "body nested before header", result: map[string]interface{}{"code": float64(99991400), "error": map[string]interface{}{"log_id": "nested-log"}}, want: "nested-log"}, + {name: "structured body has defined precedence", existingID: "existing-log", result: map[string]interface{}{"code": float64(99991400), "log_id": "body-log"}, want: "body-log"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + original := errs.NewAPIError(errs.SubtypeRateLimit, "slow").WithCode(99991400) + original.LogID = tt.existingID + result := tt.result + if result == nil { + result = map[string]interface{}{"code": float64(99991400)} + } + resp := &larkcore.ApiResp{StatusCode: http.StatusTooManyRequests, Header: http.Header{"X-Tt-Logid": []string{"header-log"}}} + if got := classifyAPIResponseError(resp, result, original); got != original { + t.Fatalf("classifyAPIResponseError() did not reuse existing APIError") + } + if original.LogID != tt.want { + t.Fatalf("LogID = %q, want %q", original.LogID, tt.want) + } + }) + } +} + +func TestClassifyAPIResponseError_BareHTTP429LogIDPrecedence(t *testing.T) { + tests := []struct { + name string + result interface{} + raw []byte + want string + }{ + {name: "top level before nested and header", result: map[string]interface{}{"log_id": "top", "error": map[string]interface{}{"log_id": "nested"}}, want: "top"}, + {name: "nested before header", result: map[string]interface{}{"error": map[string]interface{}{"log_id": "nested"}}, want: "nested"}, + {name: "raw body before header when result unavailable", raw: []byte(`{"error":{"log_id":"raw-nested"}}`), want: "raw-nested"}, + {name: "header fallback", result: map[string]interface{}{"msg": "slow"}, want: "header"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resp := &larkcore.ApiResp{StatusCode: http.StatusTooManyRequests, Header: http.Header{"X-Tt-Logid": []string{"header"}}, RawBody: tt.raw} + var apiErr *errs.APIError + err := classifyAPIResponseError(resp, tt.result, nil) + if !errors.As(err, &apiErr) { + t.Fatal("expected APIError") + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryAPI || problem.Subtype != errs.SubtypeRateLimit { + t.Fatalf("problem = %#v, want api/rate_limit", problem) + } + if apiErr.LogID != tt.want { + t.Fatalf("LogID = %q, want %q", apiErr.LogID, tt.want) + } + }) + } +} + +func TestClassifyAPIResponseError_BusinessCodeUsesCanonicalMessageAcrossHTTPStatuses(t *testing.T) { + for _, status := range []int{http.StatusBadRequest, http.StatusTooManyRequests} { + body := []byte(`{"code":99991400,"msg":"status-specific upstream text"}`) + result := map[string]interface{}{"code": float64(99991400), "msg": "status-specific upstream text"} + classified := errs.NewAPIError(errs.SubtypeRateLimit, "status-specific upstream text").WithCode(99991400) + resp := &larkcore.ApiResp{StatusCode: status, Header: http.Header{}, RawBody: body} + + var apiErr *errs.APIError + if !errors.As(classifyAPIResponseError(resp, result, classified), &apiErr) { + t.Fatalf("status %d: expected APIError", status) + } + if apiErr.Message != "request rate limit exceeded" { + t.Fatalf("status %d: message = %q, want canonical rate-limit message", status, apiErr.Message) + } + } +} + +func TestClassifyAPIResponseError_MalformedBusinessCandidateFailsClosed(t *testing.T) { + projected := map[string]interface{}{"code": float64(99991400)} + classified := errs.NewAPIError(errs.SubtypeRateLimit, "projected").WithCode(99991400) + resp := &larkcore.ApiResp{StatusCode: http.StatusBadRequest, RawBody: []byte(`{"code":99991400,]`)} + + err := classifyAPIResponseError(resp, projected, classified) + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryInternal || problem.Subtype != errs.SubtypeInvalidResponse { + t.Fatalf("problem = %#v, want internal/invalid_response", problem) + } + var syntaxErr *json.SyntaxError + if !errors.As(err, &syntaxErr) { + t.Fatalf("error chain does not preserve JSON syntax error: %v", err) + } +} + +func TestClassifyAPIResponseError_TrailingBusinessCandidateFailsClosed(t *testing.T) { + projected := map[string]interface{}{"code": float64(99991400)} + classified := errs.NewAPIError(errs.SubtypeRateLimit, "projected").WithCode(99991400) + resp := &larkcore.ApiResp{StatusCode: http.StatusBadRequest, RawBody: []byte(`{"code":99991400}trailing`)} + + err := classifyAPIResponseError(resp, projected, classified) + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryInternal || problem.Subtype != errs.SubtypeInvalidResponse { + t.Fatalf("problem = %#v, want internal/invalid_response", problem) + } + if errors.Unwrap(err) == nil { + t.Fatalf("error chain does not preserve trailing-content cause: %v", err) + } +} + +func TestClassifyAPIResponseError_MalformedRawBodyRejectsCallerProjection(t *testing.T) { + projected := map[string]interface{}{"code": float64(99991400), "log_id": "projected-log"} + classified := errs.NewAPIError(errs.SubtypeRateLimit, "projected").WithCode(99991400).WithLogID("projected-log") + resp := &larkcore.ApiResp{ + StatusCode: http.StatusTooManyRequests, + RawBody: []byte(`{"code":99991400,"log_id":"forged"}trailing`), + Header: http.Header{"X-Request-Id": []string{"header-log"}}, + } + var apiErr *errs.APIError + if !errors.As(classifyAPIResponseError(resp, projected, classified), &apiErr) { + t.Fatal("expected APIError") + } + if apiErr.Code != 429 || apiErr.LogID != "header-log" || apiErr == classified { + t.Fatalf("malformed raw body revived projection: %#v", apiErr) + } +} diff --git a/internal/client/client.go b/internal/client/client.go index f989c6069a..a5e5584c7e 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -157,7 +157,7 @@ func (c *APIClient) DoSDKRequest(ctx context.Context, req *larkcore.ApiReq, as c // Auth is resolved via Credential (same as DoSDKRequest). Security headers and // any extra headers from opts are applied automatically. // HTTP errors (status >= 400) are handled internally: the body is read (up to 4 KB), -// closed, and returned as a typed *errs.NetworkError — callers only receive successful responses. +// closed, and returned as a typed error — callers only receive successful responses. func (c *APIClient) DoStream(ctx context.Context, req *larkcore.ApiReq, as core.Identity, opts ...Option) (*http.Response, error) { cfg := buildConfig(opts) @@ -226,7 +226,35 @@ func (c *APIClient) DoStream(ctx context.Context, req *larkcore.ApiReq, as core. // Handle HTTP errors internally if resp.StatusCode >= 400 { defer resp.Body.Close() - errBody, _ := io.ReadAll(io.LimitReader(resp.Body, 4096)) + const maxStreamErrorBodyBytes = 4096 + body, readErr := io.ReadAll(io.LimitReader(resp.Body, maxStreamErrorBodyBytes+1)) + completeBody := readErr == nil && len(body) <= maxStreamErrorBodyBytes + var classificationBody []byte + if completeBody { + classificationBody = body + } + + var errBody []byte + if resp.StatusCode == http.StatusTooManyRequests { + // A bare 429 is classified from status and headers even when its body + // is incomplete. Never trust a truncated prefix for body metadata. + errBody = classificationBody + } else { + // Preserve the legacy non-429 message behavior: display at most the + // first 4 KiB and ignore a later body read error. Classification, + // however, only consumes a body proven complete within that bound. + errBody = body + if len(errBody) > maxStreamErrorBodyBytes { + errBody = errBody[:maxStreamErrorBodyBytes] + } + } + result := errclass.ParseRateLimitJSON(classificationBody) + if resp.StatusCode == http.StatusTooManyRequests || errclass.IsBusinessRateLimit(result) { + apiResp := &larkcore.ApiResp{StatusCode: resp.StatusCode, Header: resp.Header, RawBody: classificationBody} + if classified := classifyAPIResponseError(apiResp, result, c.CheckResponse(result, as)); classified != nil { + return nil, classified + } + } msg := strings.TrimSpace(string(errBody)) subtype := errs.SubtypeNetworkTransport if resp.StatusCode >= 500 { @@ -346,12 +374,46 @@ func (c *APIClient) CallAPI(ctx context.Context, request RawApiRequest) (interfa return result, nil } -// paginateLoop runs the core pagination loop. For each successful page (code == 0), -// it calls onResult if non-nil. It always accumulates and returns all raw page results. +// parsePaginationPage checks every response boundary before a page can reach +// the callback or accumulator: JSON decoding, business errors, explicit rate +// limits, and otherwise-unclassified HTTP failures. +func (c *APIClient) parsePaginationPage(resp *larkcore.ApiResp, identity core.Identity) (interface{}, error) { + return ClassifyAPIResponse(resp, func(result interface{}) error { + return c.CheckResponse(result, identity) + }) +} + +func normalizePaginationFailure(err error, operation string) error { + if _, ok := errs.ProblemOf(err); ok { + return err + } + if err == nil { + return errs.NewInternalError(errs.SubtypeUnknown, "%s failed without an error", operation) + } + return errs.NewInternalError(errs.SubtypeUnknown, "%s failed: %v", operation, err).WithCause(err) +} + +// paginateLoop runs the core pagination loop. Only pages that pass transport, +// HTTP, JSON, and business checks reach onResult and the accumulator. func (c *APIClient) paginateLoop(ctx context.Context, request RawApiRequest, opts PaginationOptions, onResult func(interface{}) error) ([]interface{}, error) { var allResults []interface{} var pageToken string + if rawPageToken, ok := request.Params["page_token"]; ok { + var valid bool + pageToken, valid = rawPageToken.(string) + if !valid { + return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, + "page_token in --params must be a string").WithParam("--params") + } + } page := 0 + classificationIdentity := opts.Identity + if classificationIdentity == "" { + classificationIdentity = request.As + } + if classificationIdentity == "" { + classificationIdentity = core.AsUser + } pageDelay := opts.PageDelay if pageDelay == 0 { pageDelay = 200 @@ -366,9 +428,10 @@ func (c *APIClient) paginateLoop(ctx context.Context, request RawApiRequest, opt if pageToken != "" { params["page_token"] = pageToken } + failedPageToken := pageToken fmt.Fprintf(c.ErrOut, "[page %d] fetching...\n", page) - result, err := c.CallAPI(ctx, RawApiRequest{ + resp, err := c.DoAPI(ctx, RawApiRequest{ Method: request.Method, URL: request.URL, Params: params, @@ -377,28 +440,17 @@ func (c *APIClient) paginateLoop(ctx context.Context, request RawApiRequest, opt ExtraOpts: request.ExtraOpts, }) if err != nil { - if page == 1 { - return nil, err - } - fmt.Fprintf(c.ErrOut, "[page %d] error, stopping pagination\n", page) - break + return allResults, errs.NewPaginationError(err, len(allResults), failedPageToken) } - - if resultMap, ok := result.(map[string]interface{}); ok { - code, _ := util.ToFloat64(resultMap["code"]) - if code != 0 { - allResults = append(allResults, result) - if page == 1 { - return allResults, nil - } - fmt.Fprintf(c.ErrOut, "[page %d] API error (code=%.0f), stopping pagination\n", page, code) - break - } + result, err := c.parsePaginationPage(resp, classificationIdentity) + if err != nil { + return allResults, errs.NewPaginationError(err, len(allResults), failedPageToken) } if onResult != nil { if err := onResult(result); err != nil { - return allResults, err + callbackErr := normalizePaginationFailure(err, "pagination page callback") + return allResults, errs.NewPaginationError(callbackErr, len(allResults), failedPageToken) } } allResults = append(allResults, result) @@ -427,7 +479,19 @@ func (c *APIClient) paginateLoop(ctx context.Context, request RawApiRequest, opt } if pageDelay > 0 { - time.Sleep(time.Duration(pageDelay) * time.Millisecond) + timer := time.NewTimer(time.Duration(pageDelay) * time.Millisecond) + select { + case <-timer.C: + case <-ctx.Done(): + if !timer.Stop() { + select { + case <-timer.C: + default: + } + } + cancelErr := normalizePaginationFailure(context.Cause(ctx), "pagination page delay") + return allResults, errs.NewPaginationError(cancelErr, len(allResults), pageToken) + } } } return allResults, nil diff --git a/internal/client/client_test.go b/internal/client/client_test.go index 23397761c7..c3b06bf3a0 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -13,6 +13,7 @@ import ( "net/http" "net/http/httptest" "strings" + "sync/atomic" "testing" "time" @@ -229,6 +230,17 @@ func TestStreamPages_OnItemsErrorStopsPagination(t *testing.T) { if !errors.Is(err, sentinel) { t.Fatalf("err = %v, want sentinel", err) } + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) { + t.Fatalf("err = %T (%v), want PaginationError", err, err) + } + if paginationErr.CompletedPages != 0 || paginationErr.NextPageToken != "" { + t.Fatalf("progress = %d/%q, want 0/empty for failed first-page callback", paginationErr.CompletedPages, paginationErr.NextPageToken) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryInternal || problem.Subtype != errs.SubtypeUnknown { + t.Fatalf("ProblemOf = %#v, %v; want internal/unknown", problem, ok) + } if result != nil { t.Fatalf("result = %#v, want nil when callback stops pagination", result) } @@ -243,6 +255,48 @@ func TestStreamPages_OnItemsErrorStopsPagination(t *testing.T) { } } +func TestPaginateLoop_TypedCallbackErrorPreservesTypeAndCurrentCursor(t *testing.T) { + rt := roundTripFunc(func(*http.Request) (*http.Response, error) { + return jsonResponse(map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "items": []interface{}{map[string]interface{}{"id": "current"}}, + "has_more": true, + "page_token": "future-page", + }, + }), nil + }) + ac, _ := newTestAPIClient(t, rt) + inner := errs.NewValidationError(errs.SubtypeFailedPrecondition, "typed callback stopped") + + results, err := ac.paginateLoop(context.Background(), RawApiRequest{ + Method: http.MethodGet, + URL: "/open-apis/test", + Params: map[string]interface{}{"page_token": "current-page"}, + As: core.AsBot, + }, PaginationOptions{PageDelay: -1}, func(interface{}) error { + return inner + }) + if len(results) != 0 { + t.Fatalf("results = %d pages, want callback-failed page excluded", len(results)) + } + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) { + t.Fatalf("err = %T (%v), want PaginationError", err, err) + } + if paginationErr.CompletedPages != 0 || paginationErr.NextPageToken != "current-page" { + t.Fatalf("progress = %d/%q, want 0/current-page", paginationErr.CompletedPages, paginationErr.NextPageToken) + } + var validationErr *errs.ValidationError + if !errors.As(err, &validationErr) || validationErr != inner { + t.Fatalf("errors.As = %#v, want original typed callback error", validationErr) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeFailedPrecondition { + t.Fatalf("ProblemOf = %#v, %v; want validation/failed_precondition", problem, ok) + } +} + func TestPaginateAll_PageLimitStopsPagination(t *testing.T) { apiCalls := 0 rt := roundTripFunc(func(req *http.Request) (*http.Response, error) { @@ -327,6 +381,352 @@ func TestPaginateAll_NaturalEndClearsPageToken(t *testing.T) { } } +func TestPaginateAll_SecondPageFailuresReturnProgress(t *testing.T) { + tests := []struct { + name string + secondPage func() (*http.Response, error) + category errs.Category + subtype errs.Subtype + }{ + { + name: "transport", + secondPage: func() (*http.Response, error) { + return nil, &net.DNSError{Err: "no such host", Name: "example.invalid"} + }, + category: errs.CategoryNetwork, + subtype: errs.SubtypeNetworkDNS, + }, + { + name: "malformed JSON", + secondPage: func() (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Header: http.Header{"Content-Type": []string{"application/json"}}, + Body: io.NopCloser(strings.NewReader(`{"code":`)), + }, nil + }, + category: errs.CategoryInternal, + subtype: errs.SubtypeInvalidResponse, + }, + { + name: "business error", + secondPage: func() (*http.Response, error) { + return jsonResponse(map[string]interface{}{"code": 230027, "msg": "user not authorized"}), nil + }, + category: errs.CategoryAuthorization, + subtype: errs.SubtypeUserUnauthorized, + }, + { + name: "HTTP error", + secondPage: func() (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusInternalServerError, + Header: http.Header{"Content-Type": []string{"text/plain"}}, + Body: io.NopCloser(strings.NewReader("gateway unavailable")), + }, nil + }, + category: errs.CategoryNetwork, + subtype: errs.SubtypeNetworkServer, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + calls := 0 + rt := roundTripFunc(func(*http.Request) (*http.Response, error) { + calls++ + if calls == 1 { + return jsonResponse(map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "items": []interface{}{map[string]interface{}{"id": "first"}}, + "has_more": true, + "page_token": "resume-secret", + }, + }), nil + } + return tt.secondPage() + }) + ac, errBuf := newTestAPIClient(t, rt) + + result, err := ac.PaginateAll(context.Background(), RawApiRequest{ + Method: http.MethodGet, + URL: "/open-apis/test", + As: core.AsUser, + }, PaginationOptions{PageDelay: -1}) + if err == nil { + t.Fatal("PaginateAll() error = nil") + } + if result != nil { + t.Fatalf("PaginateAll() result = %#v, want nil on partial failure", result) + } + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) { + t.Fatalf("error = %T (%v), want *errs.PaginationError", err, err) + } + if paginationErr.CompletedPages != 1 || paginationErr.NextPageToken != "resume-secret" { + t.Fatalf("progress = %d/%q, want 1/resume-secret", paginationErr.CompletedPages, paginationErr.NextPageToken) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != tt.category || problem.Subtype != tt.subtype { + t.Fatalf("ProblemOf = %#v, %v; want %s/%s", problem, ok, tt.category, tt.subtype) + } + if strings.Contains(errBuf.String(), "resume-secret") || strings.Contains(errBuf.String(), "error, stopping pagination") { + t.Fatalf("stderr leaked cursor or temporary error log: %q", errBuf.String()) + } + }) + } +} + +func TestPaginateAll_FirstPageFailureReportsStartingCursor(t *testing.T) { + rt := roundTripFunc(func(*http.Request) (*http.Response, error) { + return jsonResponse(map[string]interface{}{"code": 230027, "msg": "user not authorized"}), nil + }) + ac, errBuf := newTestAPIClient(t, rt) + + result, err := ac.PaginateAll(context.Background(), RawApiRequest{ + Method: http.MethodGet, + URL: "/open-apis/test", + Params: map[string]interface{}{"page_token": "starting-secret"}, + As: core.AsUser, + }, PaginationOptions{PageDelay: -1}) + if result != nil { + t.Fatalf("result = %#v, want nil", result) + } + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) { + t.Fatalf("error = %T (%v), want *errs.PaginationError", err, err) + } + if paginationErr.CompletedPages != 0 || paginationErr.NextPageToken != "starting-secret" { + t.Fatalf("progress = %d/%q, want 0/starting-secret", paginationErr.CompletedPages, paginationErr.NextPageToken) + } + if strings.Contains(errBuf.String(), "starting-secret") { + t.Fatalf("stderr leaked starting cursor: %q", errBuf.String()) + } +} + +func TestPaginateAll_RejectsNonStringStartingCursor(t *testing.T) { + var calls atomic.Int32 + rt := roundTripFunc(func(*http.Request) (*http.Response, error) { + calls.Add(1) + return jsonResponse(map[string]interface{}{"code": 0}), nil + }) + ac, _ := newTestAPIClient(t, rt) + + result, err := ac.PaginateAll(context.Background(), RawApiRequest{ + Method: http.MethodGet, + URL: "/open-apis/test", + Params: map[string]interface{}{"page_token": float64(42)}, + As: core.AsUser, + }, PaginationOptions{PageDelay: -1}) + if result != nil { + t.Fatalf("result = %#v, want nil", result) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeInvalidArgument { + t.Fatalf("problem = %#v, want validation/invalid_argument", problem) + } + var validationErr *errs.ValidationError + if !errors.As(err, &validationErr) || validationErr.Param != "--params" { + t.Fatalf("error = %#v, want --params validation error", err) + } + if got := calls.Load(); got != 0 { + t.Fatalf("HTTP calls = %d, want 0", got) + } +} + +func TestPaginateAll_BusinessErrorClassificationIdentityPrecedence(t *testing.T) { + tests := []struct { + name string + requestIdentity core.Identity + optionIdentity core.Identity + wantIdentity string + }{ + {name: "options override request", requestIdentity: core.AsBot, optionIdentity: core.AsUser, wantIdentity: "user"}, + {name: "request fallback", requestIdentity: core.AsBot, wantIdentity: "bot"}, + {name: "default fallback", wantIdentity: "user"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rt := roundTripFunc(func(*http.Request) (*http.Response, error) { + return jsonResponse(map[string]interface{}{ + "code": 99991679, + "msg": "missing scope", + }), nil + }) + ac, _ := newTestAPIClient(t, rt) + + result, err := ac.PaginateAll(context.Background(), RawApiRequest{ + Method: http.MethodGet, + URL: "/open-apis/test", + As: tt.requestIdentity, + }, PaginationOptions{Identity: tt.optionIdentity, PageDelay: -1}) + if result != nil { + t.Fatalf("result = %#v, want nil", result) + } + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) { + t.Fatalf("error = %T (%v), want PaginationError", err, err) + } + if paginationErr.CompletedPages != 0 || paginationErr.NextPageToken != "" { + t.Fatalf("progress = %d/%q, want 0/empty", paginationErr.CompletedPages, paginationErr.NextPageToken) + } + var permissionErr *errs.PermissionError + if !errors.As(err, &permissionErr) { + t.Fatalf("error = %T (%v), want PermissionError in wrapper", err, err) + } + if permissionErr.Identity != tt.wantIdentity { + t.Errorf("PermissionError.Identity = %q, want %q", permissionErr.Identity, tt.wantIdentity) + } + if tt.wantIdentity == "bot" { + if !strings.Contains(permissionErr.Hint, "developer console") { + t.Errorf("PermissionError.Hint = %q, want bot scope-application guidance", permissionErr.Hint) + } + } else if !strings.Contains(permissionErr.Hint, "lark-cli auth login") { + t.Errorf("PermissionError.Hint = %q, want user authorization recovery guidance", permissionErr.Hint) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryAuthorization || problem.Subtype != errs.SubtypeMissingScope { + t.Fatalf("ProblemOf = %#v, %v; want authorization/missing_scope", problem, ok) + } + }) + } +} + +func TestPaginateLoop_ThirdPageFailureExcludesFailedPage(t *testing.T) { + calls := 0 + rt := roundTripFunc(func(*http.Request) (*http.Response, error) { + calls++ + if calls == 3 { + return jsonResponse(map[string]interface{}{"code": 230027, "msg": "user not authorized"}), nil + } + return jsonResponse(map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "items": []interface{}{map[string]interface{}{"page": calls}}, + "has_more": true, + "page_token": map[int]string{1: "page-2", 2: "page-3"}[calls], + }, + }), nil + }) + ac, _ := newTestAPIClient(t, rt) + + results, err := ac.paginateLoop(context.Background(), RawApiRequest{ + Method: http.MethodGet, + URL: "/open-apis/test", + As: core.AsUser, + }, PaginationOptions{PageDelay: -1}, nil) + if len(results) != 2 { + t.Fatalf("accumulated pages = %d, want only two successful pages", len(results)) + } + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) { + t.Fatalf("error = %T (%v), want *errs.PaginationError", err, err) + } + if paginationErr.CompletedPages != 2 || paginationErr.NextPageToken != "page-3" { + t.Fatalf("progress = %d/%q, want 2/page-3", paginationErr.CompletedPages, paginationErr.NextPageToken) + } +} + +func TestStreamPages_PreservesWrittenPagesBeforeFailure(t *testing.T) { + calls := 0 + rt := roundTripFunc(func(*http.Request) (*http.Response, error) { + calls++ + if calls == 1 { + return jsonResponse(map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "items": []interface{}{map[string]interface{}{"id": "written"}}, + "has_more": true, + "page_token": "second-page", + }, + }), nil + } + return jsonResponse(map[string]interface{}{"code": 230027, "msg": "user not authorized"}), nil + }) + ac, _ := newTestAPIClient(t, rt) + var written []interface{} + + result, hasItems, err := ac.StreamPages(context.Background(), RawApiRequest{ + Method: http.MethodGet, + URL: "/open-apis/test", + As: core.AsUser, + }, func(items []interface{}) error { + written = append(written, items...) + return nil + }, PaginationOptions{PageDelay: -1}) + if err == nil { + t.Fatal("StreamPages() error = nil") + } + if result != nil || hasItems { + t.Fatalf("result/hasItems = %#v/%v, want nil/false on final error", result, hasItems) + } + if len(written) != 1 { + t.Fatalf("written items = %d, want successful first page preserved", len(written)) + } + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) || paginationErr.CompletedPages != 1 { + t.Fatalf("error progress = %#v, want one completed page", paginationErr) + } +} + +func TestPaginateAll_PageDelayHonorsContextCancellation(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + calls := 0 + rt := roundTripFunc(func(*http.Request) (*http.Response, error) { + calls++ + cancel() + return jsonResponse(map[string]interface{}{ + "code": 0, "msg": "ok", + "data": map[string]interface{}{ + "items": []interface{}{}, + "has_more": true, + "page_token": "not-requested", + }, + }), nil + }) + ac, _ := newTestAPIClient(t, rt) + + started := time.Now() + _, err := ac.PaginateAll(ctx, RawApiRequest{ + Method: http.MethodGet, + URL: "/open-apis/test", + As: core.AsBot, + }, PaginationOptions{PageDelay: 5_000}) + if !errors.Is(err, context.Canceled) { + t.Fatalf("error = %v, want context.Canceled", err) + } + var paginationErr *errs.PaginationError + if !errors.As(err, &paginationErr) { + t.Fatalf("error = %T (%v), want PaginationError", err, err) + } + if paginationErr.CompletedPages != 1 || paginationErr.NextPageToken != "not-requested" { + t.Fatalf("progress = %d/%q, want 1/not-requested", paginationErr.CompletedPages, paginationErr.NextPageToken) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryInternal || problem.Subtype != errs.SubtypeUnknown { + t.Fatalf("ProblemOf = %#v, %v; want internal/unknown", problem, ok) + } + encoded, marshalErr := json.Marshal(err) + if marshalErr != nil { + t.Fatalf("json.Marshal: %v", marshalErr) + } + var wire map[string]any + if unmarshalErr := json.Unmarshal(encoded, &wire); unmarshalErr != nil { + t.Fatalf("json.Unmarshal: %v", unmarshalErr) + } + if wire["completed_pages"] != float64(1) || wire["next_page_token"] != "not-requested" { + t.Fatalf("wire progress = %#v", wire) + } + if elapsed := time.Since(started); elapsed > time.Second { + t.Fatalf("cancellation took %s, want prompt return", elapsed) + } + if calls != 1 { + t.Fatalf("calls = %d, want no request after cancellation", calls) + } +} + func TestBuildApiReq_QueryParams(t *testing.T) { ac := &APIClient{} @@ -541,6 +941,230 @@ func TestDoStream_PreservesTypedTransportError(t *testing.T) { } } +func TestDoStream_RateLimitSendsOnce(t *testing.T) { + for _, method := range []string{http.MethodGet, http.MethodPost, http.MethodPatch, http.MethodDelete} { + t.Run(method, func(t *testing.T) { + calls := 0 + ac := &APIClient{ + HTTP: &http.Client{Transport: roundTripFunc(func(req *http.Request) (*http.Response, error) { + calls++ + return &http.Response{ + StatusCode: http.StatusTooManyRequests, + Header: http.Header{ + "Content-Type": []string{"application/json"}, + "Retry-After": []string{"7"}, + }, + Body: io.NopCloser(strings.NewReader(`{"code":99991400,"msg":"slow"}`)), + }, nil + })}, + Credential: credential.NewCredentialProvider(nil, nil, &staticTokenResolver{}, nil), + Config: &core.CliConfig{AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu}, + } + _, err := ac.DoStream(context.Background(), &larkcore.ApiReq{HttpMethod: method, ApiPath: "https://example.invalid/open-apis/test", Body: strings.NewReader("payload")}, core.AsBot) + var apiErr *errs.APIError + if !errors.As(err, &apiErr) || apiErr.Subtype != errs.SubtypeRateLimit { + t.Fatalf("DoStream() error = %T (%v), want api/rate_limit", err, err) + } + if calls != 1 { + t.Fatalf("request calls = %d, want exactly 1", calls) + } + }) + } +} + +func TestDoAPI_RateLimitSendsOnceForEveryMethod(t *testing.T) { + for _, method := range []string{http.MethodGet, http.MethodPost, http.MethodPatch, http.MethodDelete} { + t.Run(method, func(t *testing.T) { + calls := 0 + ac, _ := newTestAPIClient(t, roundTripFunc(func(*http.Request) (*http.Response, error) { + calls++ + return &http.Response{ + StatusCode: http.StatusTooManyRequests, + Header: http.Header{ + "Content-Type": []string{"application/json"}, + "Retry-After": []string{"2"}, + }, + Body: io.NopCloser(strings.NewReader(`{"code":99991400,"msg":"slow"}`)), + }, nil + })) + + resp, err := ac.DoAPI(context.Background(), RawApiRequest{ + Method: method, + URL: "/open-apis/test/v1/resources", + Data: map[string]interface{}{"value": "body must not be replayed"}, + As: core.AsBot, + }) + if err != nil { + t.Fatalf("DoAPI() error = %v", err) + } + err = HandleResponse(resp, ResponseOptions{Out: io.Discard, ErrOut: io.Discard}) + var apiErr *errs.APIError + if !errors.As(err, &apiErr) || apiErr.Subtype != errs.SubtypeRateLimit { + t.Fatalf("HandleResponse() error = %T (%v), want api/rate_limit", err, err) + } + if calls != 1 { + t.Fatalf("request calls = %d, want exactly 1", calls) + } + }) + } +} + +func TestDoStream_RateLimitErrorBodyIsBounded(t *testing.T) { + body := strings.Repeat("x", 8192) + ac := &APIClient{ + HTTP: &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + return &http.Response{StatusCode: http.StatusTooManyRequests, Header: http.Header{}, Body: io.NopCloser(strings.NewReader(body))}, nil + })}, + Credential: credential.NewCredentialProvider(nil, nil, &staticTokenResolver{}, nil), + Config: &core.CliConfig{AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu}, + } + _, err := ac.DoStream(context.Background(), &larkcore.ApiReq{HttpMethod: http.MethodGet, ApiPath: "https://example.invalid/open-apis/test"}, core.AsBot) + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("DoStream() error = %T (%v), want *errs.APIError", err, err) + } + if len(apiErr.Message) > 4600 { + t.Fatalf("rate-limit message unexpectedly contains unbounded body: len=%d", len(apiErr.Message)) + } +} + +type streamBoundaryErrorReader struct { + prefix *strings.Reader + err error +} + +func (r *streamBoundaryErrorReader) Read(p []byte) (int, error) { + if r.prefix.Len() > 0 { + return r.prefix.Read(p) + } + return 0, r.err +} + +func TestDoStream_HTTP429RejectsUntrustedFourKiBPrefixMetadata(t *testing.T) { + const maxBody = 4096 + prefix := `{"code":99991400,"log_id":"body-forged"}` + trustedPrefix := prefix + strings.Repeat(" ", maxBody-len(prefix)) + sentinel := errors.New("read beyond four KiB") + tests := []struct { + name string + body func() io.ReadCloser + }{ + {name: "trailing junk beyond limit", body: func() io.ReadCloser { return io.NopCloser(strings.NewReader(trustedPrefix + "junk")) }}, + {name: "second JSON beyond limit", body: func() io.ReadCloser { return io.NopCloser(strings.NewReader(trustedPrefix + `{"code":99991400}`)) }}, + {name: "boundary read error", body: func() io.ReadCloser { + return io.NopCloser(&streamBoundaryErrorReader{prefix: strings.NewReader(trustedPrefix), err: sentinel}) + }}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + calls := 0 + ac := &APIClient{ + HTTP: &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + calls++ + return &http.Response{ + StatusCode: http.StatusTooManyRequests, + Header: http.Header{ + "X-Request-Id": []string{"header-log"}, + "Retry-After": []string{"12"}, + }, + Body: tt.body(), + }, nil + })}, + Credential: credential.NewCredentialProvider(nil, nil, &staticTokenResolver{}, nil), + Config: &core.CliConfig{AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu}, + } + _, err := ac.DoStream(context.Background(), &larkcore.ApiReq{HttpMethod: http.MethodGet, ApiPath: "https://example.invalid/open-apis/test"}, core.AsBot) + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("DoStream() error = %T (%v), want APIError", err, err) + } + if apiErr.Code != 429 || apiErr.LogID != "header-log" || apiErr.RetryAfterSeconds == nil || *apiErr.RetryAfterSeconds != 12 { + t.Fatalf("untrusted 4 KiB prefix supplied metadata: %#v", apiErr) + } + if apiErr.Cause != nil || errors.Is(err, sentinel) { + t.Fatalf("body read error leaked into classification: %#v", apiErr) + } + if calls != 1 { + t.Fatalf("request calls = %d, want 1", calls) + } + }) + } +} + +func TestDoStream_BusinessRateLimitOnHTTP400(t *testing.T) { + ac := &APIClient{ + HTTP: &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusBadRequest, + Header: http.Header{"Content-Type": []string{"application/json"}}, + Body: io.NopCloser(strings.NewReader(`{"code":99991400,"msg":"slow"}`)), + }, nil + })}, + Credential: credential.NewCredentialProvider(nil, nil, &staticTokenResolver{}, nil), + Config: &core.CliConfig{AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu}, + } + _, err := ac.DoStream(context.Background(), &larkcore.ApiReq{HttpMethod: http.MethodGet, ApiPath: "https://example.invalid/open-apis/test"}, core.AsBot) + var apiErr *errs.APIError + if !errors.As(err, &apiErr) || apiErr.Code != 99991400 || apiErr.Subtype != errs.SubtypeRateLimit { + t.Fatalf("DoStream() error = %#v (%v), want api/rate_limit code 99991400", apiErr, err) + } +} + +func TestDoStream_HTTP400RejectsUntrustedFourKiBRateLimitPrefix(t *testing.T) { + const maxBody = 4096 + prefix := `{"code":99991400,"log_id":"body-forged"}` + trustedPrefix := prefix + strings.Repeat(" ", maxBody-len(prefix)) + for _, suffix := range []string{"junk", `{"code":99991400}`} { + t.Run(suffix, func(t *testing.T) { + ac := &APIClient{ + HTTP: &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusBadRequest, + Header: http.Header{"Content-Type": []string{"application/json"}}, + Body: io.NopCloser(strings.NewReader(trustedPrefix + suffix)), + }, nil + })}, + Credential: credential.NewCredentialProvider(nil, nil, &staticTokenResolver{}, nil), + Config: &core.CliConfig{AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu}, + } + + _, err := ac.DoStream(context.Background(), &larkcore.ApiReq{ + HttpMethod: http.MethodGet, + ApiPath: "https://example.invalid/open-apis/test", + }, core.AsBot) + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryNetwork || problem.Subtype != errs.SubtypeNetworkTransport || problem.Retryable { + t.Fatalf("problem = %#v, want non-retryable network/transport_error", problem) + } + if problem.Code != http.StatusBadRequest || problem.LogID != "" { + t.Fatalf("untrusted prefix supplied metadata: %#v", problem) + } + }) + } +} + +func TestDoStream_HTTP429PreservesLongTermQuotaClassification(t *testing.T) { + ac := &APIClient{ + HTTP: &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusTooManyRequests, + Header: http.Header{"Retry-After": []string{"4"}}, + Body: io.NopCloser(strings.NewReader(`{"code":1063006,"msg":"daily quota"}`)), + }, nil + })}, + Credential: credential.NewCredentialProvider(nil, nil, &staticTokenResolver{}, nil), + Config: &core.CliConfig{AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu}, + } + _, err := ac.DoStream(context.Background(), &larkcore.ApiReq{HttpMethod: http.MethodPost, ApiPath: "https://example.invalid/open-apis/test"}, core.AsUser) + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("DoStream() error = %T (%v), want *errs.APIError", err, err) + } + if apiErr.Code != 1063006 || apiErr.Retryable || apiErr.RetryAfterSeconds != nil { + t.Fatalf("long-term quota classification was changed: %#v", apiErr) + } +} + // failingTokenResolver always returns TokenUnavailableError, exercising the // auth/credential failure path through resolveAccessToken. type failingTokenResolver struct{} diff --git a/internal/client/response.go b/internal/client/response.go index 36ddc1ca38..316e9025da 100644 --- a/internal/client/response.go +++ b/internal/client/response.go @@ -41,26 +41,6 @@ type ResponseOptions struct { CheckError func(result interface{}, identity core.Identity) error } -// httpStatusError classifies an HTTP error response by status when the body -// carries no usable business error: 5xx → NetworkError (server tier), 404 → -// APIError/not_found, any other 4xx → APIError/unknown. Used wherever a -// status >= 400 must not be swallowed — a non-JSON body, an unparseable body, -// or a JSON body whose business code is 0. -func httpStatusError(status int, rawBody []byte) error { - body := util.TruncateStrWithEllipsis(strings.TrimSpace(string(rawBody)), 500) - if status >= 500 { - return errs.NewNetworkError(errs.SubtypeNetworkServer, - "HTTP %d: %s", status, body). - WithCode(status) - } - subtype := errs.SubtypeUnknown - if status == 404 { - subtype = errs.SubtypeNotFound - } - return errs.NewAPIError(subtype, "HTTP %d: %s", status, body). - WithCode(status) -} - // HandleResponse routes a raw *larkcore.ApiResp to the appropriate output: // 1. If Content-Type is JSON, check for business errors first (even with --output). // 2. If --output is set and response is not a JSON error, save to file. @@ -85,29 +65,17 @@ func HandleResponse(resp *larkcore.ApiResp, opts ResponseOptions) error { // Non-JSON error responses (e.g. 404 text/plain from gateway): return error // directly instead of falling through to the binary-save path. if resp.StatusCode >= 400 && !IsJSONContentType(ct) && ct != "" { - return httpStatusError(resp.StatusCode, resp.RawBody) + _, err := ClassifyAPIResponse(resp, nil) + return err } // JSON responses: always check for business errors before saving. if IsJSONContentType(ct) || ct == "" { - result, err := ParseJSONResponse(resp) + result, err := ClassifyAPIResponse(resp, func(result interface{}) error { + return check(result, identity) + }) if err != nil { - // An unparseable / empty body on an HTTP error (common with a - // missing Content-Type) must be classified by status, not reported - // as an internal decode failure, matching the non-JSON branch above. - if resp.StatusCode >= 400 { - return httpStatusError(resp.StatusCode, resp.RawBody) - } - return WrapJSONResponseParseError(err, resp.RawBody) - } - if apiErr := check(result, identity); apiErr != nil { - return apiErr - } - // CheckResponse treats business code 0 as success, so a 4xx/5xx whose - // JSON body omits a non-zero code would otherwise be served as a - // successful result. Classify by HTTP status so it is never swallowed. - if resp.StatusCode >= 400 { - return httpStatusError(resp.StatusCode, resp.RawBody) + return err } if opts.OutputPath != "" { // File downloads keep the existing raw-response scan path because the @@ -196,11 +164,19 @@ func ParseJSONResponse(resp *larkcore.ApiResp) (interface{}, error) { dec := json.NewDecoder(bytes.NewReader(resp.RawBody)) dec.UseNumber() if err := dec.Decode(&result); err != nil { - return nil, fmt.Errorf("response parse error: %w (body: %s)", err, util.TruncateStr(string(resp.RawBody), 500)) + return nil, fmt.Errorf("response parse error: %w (body: %s)", err, jsonResponseBodySummary(resp.RawBody)) } return result, nil } +func jsonResponseBodySummary(rawBody []byte) string { + const maxSummaryWindowBytes = 2048 + if len(rawBody) > maxSummaryWindowBytes { + rawBody = rawBody[:maxSummaryWindowBytes] + } + return util.TruncateStr(string(rawBody), 500) +} + // ── File saving ── // SaveResponse writes an API response body to the given outputPath and returns metadata. diff --git a/internal/client/response_test.go b/internal/client/response_test.go index 28ae282659..3f230cd8d8 100644 --- a/internal/client/response_test.go +++ b/internal/client/response_test.go @@ -81,6 +81,18 @@ func TestParseJSONResponse_Invalid(t *testing.T) { } } +func TestParseJSONResponse_LeavesTrailingContentValidationToSpecializedClassifiers(t *testing.T) { + resp := newApiResp([]byte(`{"code":0} trailing-gateway-content`), map[string]string{"Content-Type": "application/json"}) + result, err := ParseJSONResponse(resp) + if err != nil { + t.Fatalf("ParseJSONResponse() error = %v; generic parsing must retain its existing compatibility", err) + } + resultMap, ok := result.(map[string]interface{}) + if !ok || resultMap["code"] != json.Number("0") { + t.Fatalf("ParseJSONResponse() = %#v, want decoded first JSON value", result) + } +} + func TestParseJSONResponse_EmptyBody_WrapsEOF(t *testing.T) { resp := newApiResp([]byte{}, map[string]string{"Content-Type": "application/json"}) _, err := ParseJSONResponse(resp) @@ -364,6 +376,91 @@ func TestHandleResponse_JSONWithError(t *testing.T) { } } +func TestHandleResponse_RateLimitMetadata(t *testing.T) { + tests := []struct { + name string + status int + body []byte + headers map[string]string + wantCode int + wantWait int + wantSource string + }{ + {name: "HTTP 429 JSON without business code", status: 429, body: []byte(`{"msg":"too many requests"}`), headers: map[string]string{"Content-Type": "application/json", "Retry-After": "8"}, wantCode: 429, wantWait: 8, wantSource: "retry-after"}, + {name: "HTTP 429 non JSON", status: 429, body: []byte("slow down"), headers: map[string]string{"Content-Type": "text/plain"}, wantCode: 429, wantWait: 1, wantSource: "default"}, + {name: "HTTP 429 empty body", status: 429, headers: map[string]string{"Retry-After": "3"}, wantCode: 429, wantWait: 3, wantSource: "retry-after"}, + {name: "business rate limit on HTTP 400", status: 400, body: []byte(`{"code":99991400,"msg":"request rate limit exceeded"}`), headers: map[string]string{"Content-Type": "application/json", "Retry-After": "10"}, wantCode: 99991400, wantWait: 10, wantSource: "retry-after"}, + {name: "HTTP 429 preserves rate limit business code", status: 429, body: []byte(`{"code":99991400,"msg":"request rate limit exceeded"}`), headers: map[string]string{"Content-Type": "application/json", "Retry-After": "5"}, wantCode: 99991400, wantWait: 5, wantSource: "retry-after"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := HandleResponse(newApiRespWithStatus(tt.status, tt.body, tt.headers), ResponseOptions{Out: io.Discard, ErrOut: io.Discard, FileIO: &localfileio.LocalFileIO{}}) + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("HandleResponse() error = %T (%v), want *errs.APIError", err, err) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryAPI || problem.Subtype != errs.SubtypeRateLimit { + t.Fatalf("problem = %#v, want api/rate_limit", problem) + } + if apiErr.Subtype != errs.SubtypeRateLimit || apiErr.Code != tt.wantCode || !apiErr.Retryable { + t.Fatalf("rate limit problem = %#v, want subtype rate_limit, code %d, retryable", apiErr.Problem, tt.wantCode) + } + if apiErr.RetryAfterSeconds == nil || *apiErr.RetryAfterSeconds != tt.wantWait || apiErr.RetryAfterSource != tt.wantSource { + t.Fatalf("retry metadata = (%v, %q), want (%d, %q)", apiErr.RetryAfterSeconds, apiErr.RetryAfterSource, tt.wantWait, tt.wantSource) + } + if !strings.Contains(apiErr.Hint, "does not mean") || !strings.Contains(apiErr.Hint, "safe to replay") { + t.Fatalf("hint must distinguish retryable from safe replay: %q", apiErr.Hint) + } + }) + } +} + +func TestHandleResponse_BusinessRateLimitBackfillsHeaderLogID(t *testing.T) { + resp := newApiRespWithStatus(http.StatusBadRequest, + []byte(`{"code":99991400,"msg":"slow"}`), + map[string]string{"Content-Type": "application/json", "Retry-After": "2", "X-Tt-Logid": "header-log"}) + err := HandleResponse(resp, ResponseOptions{Out: io.Discard, ErrOut: io.Discard, FileIO: &localfileio.LocalFileIO{}}) + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("HandleResponse() error = %T (%v), want *errs.APIError", err, err) + } + if apiErr.LogID != "header-log" { + t.Fatalf("LogID = %q, want header-log", apiErr.LogID) + } +} + +func TestHandleResponse_HTTP429MalformedJSONCannotForgeMetadata(t *testing.T) { + for _, body := range [][]byte{ + []byte(`{"code":99991400,"log_id":"forged"}trailing-junk`), + []byte(`{"code":99991400,"log_id":"forged"}{"code":99991400}`), + } { + err := HandleResponse(newApiRespWithStatus(429, body, map[string]string{"Content-Type": "application/json"}), ResponseOptions{Out: io.Discard, ErrOut: io.Discard, FileIO: &localfileio.LocalFileIO{}}) + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("HandleResponse() error = %T (%v), want APIError", err, err) + } + if apiErr.Code != 429 || apiErr.LogID != "" || apiErr.Message != "request rate limit exceeded" { + t.Fatalf("malformed response forged metadata: %#v", apiErr) + } + } +} + +func TestParseJSONResponse_LargeInvalidBodyHasBoundedSummary(t *testing.T) { + body := []byte(`not-json{"blob":"` + strings.Repeat("界", 1<<20) + `tail-secret"}`) + _, err := ParseJSONResponse(newApiResp(body, map[string]string{"Content-Type": "application/json"})) + if err == nil { + t.Fatal("expected invalid JSON error") + } + message := err.Error() + if strings.Contains(message, "tail-secret") { + t.Fatalf("error summary contains far-tail content: len=%d", len(message)) + } + if len(message) > 2500 { + t.Fatalf("error summary is not bounded: len=%d", len(message)) + } +} + func TestHandleResponse_BinaryAutoSave(t *testing.T) { dir := t.TempDir() origWd, _ := os.Getwd() diff --git a/internal/cmdutil/error_presenter.go b/internal/cmdutil/error_presenter.go index 49873b55a0..aa1658038a 100644 --- a/internal/cmdutil/error_presenter.go +++ b/internal/cmdutil/error_presenter.go @@ -33,6 +33,13 @@ func (f *Factory) PresentError(err error, options ErrorPresentationOptions) erro projector = f.Recovery } rendered := projector.Render(err) + if typed, ok := errs.UnwrapTypedError(rendered); ok { + if paginationErr, ok := typed.(*errs.PaginationError); ok { //nolint:errorlint + completePermissionRecovery(f, paginationErr.Cause, projector, options.Identity, options.DeclaredScopes) + applyNeedAuthorizationHint(paginationErr.Cause, projector, options.DeclaredScopes) + return errs.NewPaginationError(paginationErr.Cause, paginationErr.CompletedPages, paginationErr.NextPageToken) + } + } completePermissionRecovery(f, rendered, projector, options.Identity, options.DeclaredScopes) applyNeedAuthorizationHint(rendered, projector, options.DeclaredScopes) return rendered diff --git a/internal/cmdutil/error_presenter_test.go b/internal/cmdutil/error_presenter_test.go index 29a7f8f617..238a361553 100644 --- a/internal/cmdutil/error_presenter_test.go +++ b/internal/cmdutil/error_presenter_test.go @@ -4,17 +4,46 @@ package cmdutil import ( + "encoding/json" "errors" "strings" "testing" "github.com/larksuite/cli/errs" + internalauth "github.com/larksuite/cli/internal/auth" "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/errclass" "github.com/larksuite/cli/internal/recovery" "github.com/larksuite/cli/internal/surface" ) +func TestFactoryPresentErrorEnrichesPaginationCauseAndRebuildsSnapshot(t *testing.T) { + f := &Factory{Recovery: recovery.NewProjector(nil)} + source := errs.NewPaginationError(internalauth.NewNeedUserAuthorizationError("ou_test"), 1, "resume-page-2") + + rendered := f.PresentError(source, ErrorPresentationOptions{ + DeclaredScopes: func() []string { return []string{"docx:document:create"} }, + }) + var paginationErr *errs.PaginationError + if !errors.As(rendered, &paginationErr) { + t.Fatalf("PresentError() = %T, want PaginationError", rendered) + } + encoded, err := json.Marshal(rendered) + if err != nil { + t.Fatalf("json.Marshal(rendered): %v", err) + } + var wire map[string]interface{} + if err := json.Unmarshal(encoded, &wire); err != nil { + t.Fatalf("json.Unmarshal(rendered): %v", err) + } + if got, want := wire["hint"], recovery.UserAuthorization("docx:document:create").String(); got != want { + t.Fatalf("hint = %#v, want %q", got, want) + } + if wire["completed_pages"] != float64(1) || wire["next_page_token"] != "resume-page-2" { + t.Fatalf("pagination progress = %#v", wire) + } +} + func TestFactoryPresentErrorClonesAndPreservesPermissionMachineFields(t *testing.T) { cause := errors.New("permission cause") source := errs.NewPermissionError(errs.SubtypeMissingScope, "missing scope"). diff --git a/internal/cmdutil/factory.go b/internal/cmdutil/factory.go index d1966209ff..fe747df546 100644 --- a/internal/cmdutil/factory.go +++ b/internal/cmdutil/factory.go @@ -49,6 +49,7 @@ type Factory struct { IdentityAutoDetected bool // set by ResolveAs when identity was auto-detected ResolvedIdentity core.Identity // identity resolved by the last ResolveAs call CurrentCommand *cobra.Command // last matched command being executed; set during PersistentPreRun + identityExplicit bool // set for any explicit non-auto --as value, including invalid values, so errors can attribute the flag Credential *credential.CredentialProvider @@ -116,9 +117,11 @@ func (f *Factory) ResolveFileIO(ctx context.Context) fileio.FileIO { // When the value is "auto" (or unset), auto-detect based on credential hints. func (f *Factory) ResolveAs(ctx context.Context, cmd *cobra.Command, flagAs core.Identity) core.Identity { f.IdentityAutoDetected = false + f.identityExplicit = false if cmd != nil && cmd.Flags().Changed("as") { if flagAs != core.AsAuto { + f.identityExplicit = true f.ResolvedIdentity = flagAs return flagAs } @@ -175,8 +178,17 @@ func (f *Factory) resolveIdentityHint(ctx context.Context) *credential.IdentityH // CheckIdentity verifies the resolved identity is in the supported list. // On success, sets f.ResolvedIdentity. On failure, returns an error -// tailored to whether the identity was explicit (--as) or auto-detected. +// tailored to whether the identity was explicitly selected with --as or was +// selected implicitly through auto-detection, default-as, or strict mode. func (f *Factory) CheckIdentity(as core.Identity, supported []string) error { + if as != core.AsUser && as != core.AsBot { + invalidErr := errs.NewValidationError(errs.SubtypeInvalidArgument, + "invalid identity %q, expected user or bot", as) + if f.identityExplicit { + return invalidErr.WithParam("--as") + } + return invalidErr + } for _, t := range supported { if string(as) == t { f.ResolvedIdentity = as @@ -184,19 +196,26 @@ func (f *Factory) CheckIdentity(as core.Identity, supported []string) error { } } list := strings.Join(supported, ", ") - if f.IdentityAutoDetected { - base := errs.NewValidationError(errs.SubtypeInvalidArgument, - "resolved identity %q (via auto-detect or default-as) is not supported, this command only supports: %s", - as, list). + var identityErr *errs.ValidationError + if f.identityExplicit { + identityErr = errs.NewValidationError(errs.SubtypeIdentityNotSupported, + "--as %s is not supported, this command only supports: %s", as, list). WithParam("--as") - if len(supported) > 0 { - return base.WithHint("use --as %s", supported[0]) - } - return base + } else { + identityErr = errs.NewValidationError(errs.SubtypeIdentityNotSupported, + "resolved identity %q is not supported, this command only supports: %s", as, list) } - return errs.NewValidationError(errs.SubtypeInvalidArgument, - "--as %s is not supported, this command only supports: %s", as, list). - WithParam("--as") + if len(supported) == 1 { + return identityErr.WithHint( + "this command supports %s identity; use --as %s only when %s authorization is configured", + supported[0], supported[0], supported[0]) + } + if len(supported) > 1 { + return identityErr.WithHint( + "this command supports these identities: %s; use --as with a supported identity only when its authorization is configured", + list) + } + return identityErr } // ResolveStrictMode returns the effective strict mode by reading diff --git a/internal/cmdutil/factory_test.go b/internal/cmdutil/factory_test.go index dfb5dae120..8f6c22943c 100644 --- a/internal/cmdutil/factory_test.go +++ b/internal/cmdutil/factory_test.go @@ -218,34 +218,120 @@ func TestCheckIdentity_Supported_UserOnly(t *testing.T) { func TestCheckIdentity_Unsupported_Explicit(t *testing.T) { f, _, _, _ := TestFactory(t, &core.CliConfig{AppID: "a", AppSecret: "s"}) - f.IdentityAutoDetected = false // explicit --as + cmd := newCmdWithAsFlag("user", true) + f.ResolveAs(context.Background(), cmd, core.AsUser) err := f.CheckIdentity(core.AsUser, []string{"bot"}) if err == nil { t.Fatal("expected error") } - if !strings.Contains(err.Error(), "--as user is not supported") { - t.Errorf("unexpected error message: %v", err) + problem, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected typed error, got %T: %v", err, err) + } + if problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeIdentityNotSupported { + t.Errorf("category/subtype = %s/%s, want validation/identity_not_supported", problem.Category, problem.Subtype) + } + var ve *errs.ValidationError + if !errors.As(err, &ve) { + t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err) + } + if ve.Param != "--as" { + t.Errorf("Param = %q, want --as", ve.Param) } - if !strings.Contains(err.Error(), "bot") { - t.Errorf("error should mention supported identity: %v", err) + if got, want := ve.Hint, "this command supports bot identity; use --as bot only when bot authorization is configured"; got != want { + t.Errorf("Hint = %q, want %q", got, want) } } func TestCheckIdentity_Unsupported_AutoDetected(t *testing.T) { f, _, _, _ := TestFactory(t, &core.CliConfig{AppID: "a", AppSecret: "s"}) - f.IdentityAutoDetected = true + cmd := newCmdWithAsFlag("auto", true) + f.ResolveAs(context.Background(), cmd, core.AsAuto) + + err := f.CheckIdentity(core.AsUser, []string{"bot"}) + var ve *errs.ValidationError + if !errors.As(err, &ve) { + t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err) + } + if ve.Subtype != errs.SubtypeIdentityNotSupported { + t.Errorf("Subtype = %q, want %q", ve.Subtype, errs.SubtypeIdentityNotSupported) + } + if ve.Param != "" { + t.Errorf("Param = %q, want empty for auto-selected identity", ve.Param) + } + if got, want := ve.Hint, "this command supports bot identity; use --as bot only when bot authorization is configured"; got != want { + t.Errorf("Hint = %q, want %q", got, want) + } +} + +func TestCheckIdentity_Unsupported_DefaultAs(t *testing.T) { + f, _, _, _ := TestFactory(t, &core.CliConfig{AppID: "a", AppSecret: "s", DefaultAs: "user"}) + cmd := newCmdWithAsFlag("auto", false) + f.ResolveAs(context.Background(), cmd, core.AsUser) err := f.CheckIdentity(core.AsUser, []string{"bot"}) var ve *errs.ValidationError if !errors.As(err, &ve) { t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err) } - if !strings.Contains(ve.Message, "resolved identity") { - t.Errorf("expected 'resolved identity' in message, got: %v", ve.Message) + if ve.Subtype != errs.SubtypeIdentityNotSupported { + t.Errorf("Subtype = %q, want %q", ve.Subtype, errs.SubtypeIdentityNotSupported) + } + if ve.Param != "" { + t.Errorf("Param = %q, want empty for default-as identity", ve.Param) } - if !strings.Contains(ve.Hint, "use --as bot") { - t.Errorf("expected hint to suggest --as bot, got: %v", ve.Hint) +} + +func TestCheckIdentity_Unsupported_NoSupportedIdentityHasNoHint(t *testing.T) { + f, _, _, _ := TestFactory(t, &core.CliConfig{AppID: "a", AppSecret: "s"}) + + err := f.CheckIdentity(core.AsUser, nil) + var ve *errs.ValidationError + if !errors.As(err, &ve) { + t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err) + } + if ve.Subtype != errs.SubtypeIdentityNotSupported { + t.Errorf("Subtype = %q, want %q", ve.Subtype, errs.SubtypeIdentityNotSupported) + } + if ve.Hint != "" { + t.Errorf("Hint = %q, want empty when no supported identity is known", ve.Hint) + } +} + +func TestCheckIdentity_InvalidIdentityRemainsInvalidArgument(t *testing.T) { + tests := []struct { + name string + identity core.Identity + explicit bool + wantParam string + }{ + {name: "explicit unknown", identity: "admin", explicit: true, wantParam: "--as"}, + {name: "explicit wrong case", identity: "USER", explicit: true, wantParam: "--as"}, + {name: "implicit unknown", identity: "bogus", explicit: false}, + {name: "implicit empty", identity: "", explicit: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f, _, _, _ := TestFactory(t, &core.CliConfig{AppID: "a", AppSecret: "s"}) + if tt.explicit { + cmd := newCmdWithAsFlag(string(tt.identity), true) + f.ResolveAs(context.Background(), cmd, tt.identity) + } + + err := f.CheckIdentity(tt.identity, []string{"user", "bot"}) + var ve *errs.ValidationError + if !errors.As(err, &ve) { + t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err) + } + if ve.Subtype != errs.SubtypeInvalidArgument { + t.Errorf("Subtype = %q, want %q", ve.Subtype, errs.SubtypeInvalidArgument) + } + if ve.Param != tt.wantParam { + t.Errorf("Param = %q, want %q", ve.Param, tt.wantParam) + } + }) } } diff --git a/internal/credential/default_provider.go b/internal/credential/default_provider.go index 66dee6f201..7e1a112ccd 100644 --- a/internal/credential/default_provider.go +++ b/internal/credential/default_provider.go @@ -5,6 +5,7 @@ package credential import ( "context" + "errors" "fmt" "io" "net/http" @@ -115,14 +116,29 @@ type DefaultTokenProvider struct { defaultAcct *DefaultAccountProvider httpClient func() (*http.Client, error) errOut io.Writer + tatResolver func(context.Context) (*TokenResult, error) - tatOnce sync.Once + tatMu sync.Mutex + tatFlight *tatResolution + tatCached bool tatResult *TokenResult tatErr error } +type tatResolution struct { + done chan struct{} + result *TokenResult + err error + followers int + leaderContextEnded bool + panicked bool + panicVal any +} + func NewDefaultTokenProvider(defaultAcct *DefaultAccountProvider, httpClient func() (*http.Client, error), errOut io.Writer) *DefaultTokenProvider { - return &DefaultTokenProvider{defaultAcct: defaultAcct, httpClient: httpClient, errOut: errOut} + p := &DefaultTokenProvider{defaultAcct: defaultAcct, httpClient: httpClient, errOut: errOut} + p.tatResolver = p.doResolveTAT + return p } func (p *DefaultTokenProvider) ResolveToken(ctx context.Context, req TokenSpec) (*TokenResult, error) { @@ -159,13 +175,73 @@ func (p *DefaultTokenProvider) resolveUAT(ctx context.Context) (*TokenResult, er return &TokenResult{Token: token, Scopes: scopes}, nil } -// resolveTAT resolves a tenant access token. The result is cached after the first -// call via sync.Once — only the context from the first call is used. +// resolveTAT resolves a tenant access token. Concurrent callers share an in-flight +// resolution. Successful and explicitly non-retryable typed results are cached; +// retryable or untyped errors allow a later call to try again. func (p *DefaultTokenProvider) resolveTAT(ctx context.Context) (*TokenResult, error) { - p.tatOnce.Do(func() { - p.tatResult, p.tatErr = p.doResolveTAT(ctx) - }) - return p.tatResult, p.tatErr + return p.resolveTATWithHandoff(ctx, true) +} + +func (p *DefaultTokenProvider) resolveTATWithHandoff(ctx context.Context, allowHandoff bool) (*TokenResult, error) { + p.tatMu.Lock() + if p.tatCached { + result, err := p.tatResult, p.tatErr + p.tatMu.Unlock() + return result, err + } + if flight := p.tatFlight; flight != nil { + flight.followers++ + p.tatMu.Unlock() + select { + case <-flight.done: + if flight.panicked { + panic(flight.panicVal) + } + if allowHandoff && flight.leaderContextEnded && ctx.Err() == nil { + return p.resolveTATWithHandoff(ctx, false) + } + return flight.result, flight.err + case <-ctx.Done(): + return nil, ctx.Err() + } + } + + var result *TokenResult + var err error + cacheResult := false + completed := false + + flight := &tatResolution{done: make(chan struct{})} + p.tatFlight = flight + p.tatMu.Unlock() + + defer func() { + panicVal := recover() + panicked := !completed + + p.tatMu.Lock() + flight.result, flight.err = result, err + leaderContextErr := ctx.Err() + flight.leaderContextEnded = leaderContextErr != nil && errors.Is(err, leaderContextErr) + flight.panicked, flight.panicVal = panicked, panicVal + if cacheResult { + p.tatResult, p.tatErr = result, err + p.tatCached = true + } + p.tatFlight = nil + close(flight.done) + p.tatMu.Unlock() + + if panicked { + panic(panicVal) + } + }() + + result, err = p.tatResolver(ctx) + _, typed := errs.ProblemOf(err) + cacheResult = err == nil || (typed && !errs.IsRetryable(err) && !errors.Is(err, context.Canceled) && !errors.Is(err, context.DeadlineExceeded)) + completed = true + return result, err } func (p *DefaultTokenProvider) doResolveTAT(ctx context.Context) (*TokenResult, error) { diff --git a/internal/credential/default_provider_test.go b/internal/credential/default_provider_test.go index 057f1dba80..f0c49be03d 100644 --- a/internal/credential/default_provider_test.go +++ b/internal/credential/default_provider_test.go @@ -4,8 +4,13 @@ package credential import ( + "context" "errors" + "runtime" + "sync" + "sync/atomic" "testing" + "time" "github.com/larksuite/cli/errs" ) @@ -19,6 +24,501 @@ func TestDefaultAccountProvider_Implements(t *testing.T) { var _ DefaultAccountResolver = &DefaultAccountProvider{} } +func requireTATProblem(t *testing.T, err error, category errs.Category, subtype errs.Subtype, retryable bool) { + t.Helper() + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != category || problem.Subtype != subtype || problem.Retryable != retryable { + t.Fatalf("problem = %#v, want category=%q subtype=%q retryable=%v", problem, category, subtype, retryable) + } +} + +func TestDefaultTokenProvider_RetryableTATErrorIsNotCached(t *testing.T) { + retryableErr := errs.NewAPIError(errs.SubtypeRateLimit, "rate limited").WithRetryable() + var calls atomic.Int32 + p := NewDefaultTokenProvider(nil, nil, nil) + p.tatResolver = func(context.Context) (*TokenResult, error) { + if calls.Add(1) == 1 { + return nil, retryableErr + } + return &TokenResult{Token: "recovered-token"}, nil + } + + if result, err := p.resolveTAT(context.Background()); result != nil || !errors.Is(err, retryableErr) { + t.Fatalf("first resolution = (%v, %v), want retryable error", result, err) + } else { + requireTATProblem(t, err, errs.CategoryAPI, errs.SubtypeRateLimit, true) + } + result, err := p.resolveTAT(context.Background()) + if err != nil { + t.Fatalf("second resolution returned error: %v", err) + } + if result == nil || result.Token != "recovered-token" { + t.Fatalf("second resolution = %#v, want recovered token", result) + } + if got := calls.Load(); got != 2 { + t.Fatalf("resolver calls = %d, want 2", got) + } +} + +func TestDefaultTokenProvider_UntypedTATErrorIsNotCached(t *testing.T) { + transientErr := errors.New("TAT endpoint transient failure") + var calls atomic.Int32 + p := NewDefaultTokenProvider(nil, nil, nil) + p.tatResolver = func(context.Context) (*TokenResult, error) { + if calls.Add(1) == 1 { + return nil, transientErr + } + return &TokenResult{Token: "recovered-token"}, nil + } + + if result, err := p.resolveTAT(context.Background()); result != nil || !errors.Is(err, transientErr) { + t.Fatalf("first resolution = (%v, %v), want transient error", result, err) + } + result, err := p.resolveTAT(context.Background()) + if err != nil || result == nil || result.Token != "recovered-token" { + t.Fatalf("second resolution = (%v, %v), want recovered token", result, err) + } + if got := calls.Load(); got != 2 { + t.Fatalf("resolver calls = %d, want 2", got) + } +} + +func TestDefaultTokenProvider_ConcurrentRetryableTATFlightIsSharedThenRetried(t *testing.T) { + const callers = 16 + retryableErr := errs.NewAPIError(errs.SubtypeRateLimit, "rate limited").WithRetryable() + started := make(chan struct{}) + release := make(chan struct{}) + var calls atomic.Int32 + p := NewDefaultTokenProvider(nil, nil, nil) + p.tatResolver = func(context.Context) (*TokenResult, error) { + if calls.Add(1) == 1 { + close(started) + <-release + return nil, retryableErr + } + return &TokenResult{Token: "recovered-token"}, nil + } + + type outcome struct { + result *TokenResult + err error + } + outcomes := make(chan outcome, callers) + go func() { + result, err := p.resolveTAT(context.Background()) + outcomes <- outcome{result: result, err: err} + }() + select { + case <-started: + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for the first TAT resolution to start") + } + + for i := 1; i < callers; i++ { + go func() { + result, err := p.resolveTAT(context.Background()) + outcomes <- outcome{result: result, err: err} + }() + } + deadline := time.Now().Add(2 * time.Second) + for { + p.tatMu.Lock() + followers := p.tatFlight.followers + p.tatMu.Unlock() + if followers == callers-1 { + break + } + if time.Now().After(deadline) { + t.Fatalf("in-flight followers = %d, want %d", followers, callers-1) + } + runtime.Gosched() + } + + close(release) + for i := 0; i < callers; i++ { + select { + case got := <-outcomes: + if got.result != nil || !errors.Is(got.err, retryableErr) { + t.Fatalf("shared retryable resolution = (%v, %v), want retryable error", got.result, got.err) + } + requireTATProblem(t, got.err, errs.CategoryAPI, errs.SubtypeRateLimit, true) + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for shared retryable resolution") + } + } + if got := calls.Load(); got != 1 { + t.Fatalf("resolver calls after shared flight = %d, want 1", got) + } + + result, err := p.resolveTAT(context.Background()) + if err != nil || result == nil || result.Token != "recovered-token" { + t.Fatalf("later resolution = (%v, %v), want recovered success", result, err) + } + if got := calls.Load(); got != 2 { + t.Fatalf("resolver calls after recovery = %d, want 2", got) + } +} + +func TestDefaultTokenProvider_ConcurrentTATResolverPanicUnblocksFollowersAndCleansFlight(t *testing.T) { + const callers = 16 + panicVal := &struct{ message string }{message: "resolver panic"} + started := make(chan struct{}) + release := make(chan struct{}) + var calls atomic.Int32 + p := NewDefaultTokenProvider(nil, nil, nil) + p.tatResolver = func(context.Context) (*TokenResult, error) { + if calls.Add(1) == 1 { + close(started) + <-release + panic(panicVal) + } + return &TokenResult{Token: "recovered-after-panic"}, nil + } + + recovered := make(chan any, callers) + resolveAndRecover := func() { + defer func() { + recovered <- recover() + }() + _, _ = p.resolveTAT(context.Background()) + } + go resolveAndRecover() + select { + case <-started: + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for the panicking TAT resolution to start") + } + + for i := 1; i < callers; i++ { + go resolveAndRecover() + } + deadline := time.Now().Add(2 * time.Second) + for { + p.tatMu.Lock() + followers := p.tatFlight.followers + p.tatMu.Unlock() + if followers == callers-1 { + break + } + if time.Now().After(deadline) { + t.Fatalf("in-flight followers = %d, want %d", followers, callers-1) + } + runtime.Gosched() + } + + close(release) + for i := 0; i < callers; i++ { + select { + case got := <-recovered: + if got != panicVal { + t.Fatalf("recovered panic = %#v, want %#v", got, panicVal) + } + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for caller to recover resolver panic") + } + } + if got := calls.Load(); got != 1 { + t.Fatalf("resolver calls after panicking flight = %d, want 1", got) + } + + result, err := p.resolveTAT(context.Background()) + if err != nil || result == nil || result.Token != "recovered-after-panic" { + t.Fatalf("later resolution = (%v, %v), want recovered success", result, err) + } + if got := calls.Load(); got != 2 { + t.Fatalf("resolver calls after panic recovery = %d, want 2", got) + } +} + +func TestDefaultTokenProvider_SuccessfulTATIsCached(t *testing.T) { + var calls atomic.Int32 + want := &TokenResult{Token: "cached-token"} + p := NewDefaultTokenProvider(nil, nil, nil) + p.tatResolver = func(context.Context) (*TokenResult, error) { + calls.Add(1) + return want, nil + } + + for i := 0; i < 2; i++ { + result, err := p.resolveTAT(context.Background()) + if err != nil || result != want { + t.Fatalf("resolution %d = (%v, %v), want cached success", i+1, result, err) + } + } + if got := calls.Load(); got != 1 { + t.Fatalf("resolver calls = %d, want 1", got) + } +} + +func TestDefaultTokenProvider_NonRetryableTATErrorIsCached(t *testing.T) { + wantErr := errs.NewConfigError(errs.SubtypeInvalidClient, "invalid credentials") + var calls atomic.Int32 + p := NewDefaultTokenProvider(nil, nil, nil) + p.tatResolver = func(context.Context) (*TokenResult, error) { + calls.Add(1) + return nil, wantErr + } + + for i := 0; i < 2; i++ { + result, err := p.resolveTAT(context.Background()) + if result != nil || !errors.Is(err, wantErr) { + t.Fatalf("resolution %d = (%v, %v), want cached error", i+1, result, err) + } + requireTATProblem(t, err, errs.CategoryConfig, errs.SubtypeInvalidClient, false) + } + if got := calls.Load(); got != 1 { + t.Fatalf("resolver calls = %d, want 1", got) + } +} + +func TestDefaultTokenProvider_ContextCancellationIsNotCached(t *testing.T) { + var calls atomic.Int32 + p := NewDefaultTokenProvider(nil, nil, nil) + p.tatResolver = func(ctx context.Context) (*TokenResult, error) { + if calls.Add(1) == 1 { + return nil, ctx.Err() + } + return &TokenResult{Token: "recovered-token"}, nil + } + + canceledCtx, cancel := context.WithCancel(context.Background()) + cancel() + if result, err := p.resolveTAT(canceledCtx); result != nil || !errors.Is(err, context.Canceled) { + t.Fatalf("canceled resolution = (%v, %v), want context.Canceled", result, err) + } + result, err := p.resolveTAT(context.Background()) + if err != nil || result == nil || result.Token != "recovered-token" { + t.Fatalf("resolution after cancellation = (%v, %v), want recovered token", result, err) + } + if got := calls.Load(); got != 2 { + t.Fatalf("resolver calls = %d, want 2", got) + } +} + +func TestDefaultTokenProvider_ContextDeadlineIsNotCached(t *testing.T) { + var calls atomic.Int32 + p := NewDefaultTokenProvider(nil, nil, nil) + p.tatResolver = func(context.Context) (*TokenResult, error) { + if calls.Add(1) == 1 { + return nil, context.DeadlineExceeded + } + return &TokenResult{Token: "recovered-token"}, nil + } + + if result, err := p.resolveTAT(context.Background()); result != nil || !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("deadline resolution = (%v, %v), want context.DeadlineExceeded", result, err) + } + result, err := p.resolveTAT(context.Background()) + if err != nil || result == nil || result.Token != "recovered-token" { + t.Fatalf("resolution after deadline = (%v, %v), want recovered token", result, err) + } + if got := calls.Load(); got != 2 { + t.Fatalf("resolver calls = %d, want 2", got) + } +} + +func TestDefaultTokenProvider_FollowerObservesOwnContextCancellation(t *testing.T) { + started := make(chan struct{}) + release := make(chan struct{}) + p := NewDefaultTokenProvider(nil, nil, nil) + p.tatResolver = func(context.Context) (*TokenResult, error) { + close(started) + <-release + return &TokenResult{Token: "leader-token"}, nil + } + + type outcome struct { + result *TokenResult + err error + } + leaderDone := make(chan outcome, 1) + go func() { + result, err := p.resolveTAT(context.Background()) + leaderDone <- outcome{result: result, err: err} + }() + select { + case <-started: + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for leader resolution") + } + + followerCtx, cancel := context.WithCancel(context.Background()) + followerDone := make(chan error, 1) + go func() { + _, err := p.resolveTAT(followerCtx) + followerDone <- err + }() + deadline := time.Now().Add(2 * time.Second) + for { + p.tatMu.Lock() + followers := p.tatFlight.followers + p.tatMu.Unlock() + if followers == 1 { + break + } + if time.Now().After(deadline) { + t.Fatalf("in-flight followers = %d, want 1", followers) + } + runtime.Gosched() + } + + cancel() + select { + case err := <-followerDone: + if !errors.Is(err, context.Canceled) { + t.Fatalf("follower error = %v, want context.Canceled", err) + } + case <-time.After(2 * time.Second): + t.Fatal("follower did not observe its context cancellation") + } + close(release) + select { + case got := <-leaderDone: + if got.err != nil || got.result == nil || got.result.Token != "leader-token" { + t.Fatalf("leader resolution = (%v, %v), want leader-token", got.result, got.err) + } + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for leader resolution") + } +} + +func TestDefaultTokenProvider_HealthyFollowerRetriesAfterLeaderCancellation(t *testing.T) { + started := make(chan struct{}) + var calls atomic.Int32 + p := NewDefaultTokenProvider(nil, nil, nil) + p.tatResolver = func(ctx context.Context) (*TokenResult, error) { + if calls.Add(1) == 1 { + close(started) + <-ctx.Done() + return nil, ctx.Err() + } + return &TokenResult{Token: "follower-token"}, nil + } + + type outcome struct { + result *TokenResult + err error + } + leaderCtx, cancelLeader := context.WithCancel(context.Background()) + leaderDone := make(chan outcome, 1) + go func() { + result, err := p.resolveTAT(leaderCtx) + leaderDone <- outcome{result: result, err: err} + }() + select { + case <-started: + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for leader resolution") + } + + followerDone := make(chan outcome, 1) + go func() { + result, err := p.resolveTAT(context.Background()) + followerDone <- outcome{result: result, err: err} + }() + deadline := time.Now().Add(2 * time.Second) + for { + p.tatMu.Lock() + followers := p.tatFlight.followers + p.tatMu.Unlock() + if followers == 1 { + break + } + if time.Now().After(deadline) { + t.Fatalf("in-flight followers = %d, want 1", followers) + } + runtime.Gosched() + } + + cancelLeader() + select { + case got := <-leaderDone: + if got.result != nil || !errors.Is(got.err, context.Canceled) { + t.Fatalf("leader resolution = (%v, %v), want context.Canceled", got.result, got.err) + } + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for canceled leader") + } + select { + case got := <-followerDone: + if got.err != nil || got.result == nil || got.result.Token != "follower-token" { + t.Fatalf("follower resolution = (%v, %v), want follower-token", got.result, got.err) + } + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for healthy follower recovery") + } + if got := calls.Load(); got != 2 { + t.Fatalf("resolver calls = %d, want 2", got) + } +} + +func TestDefaultTokenProvider_ConcurrentTATSuccessCoalesces(t *testing.T) { + const callers = 16 + started := make(chan struct{}) + release := make(chan struct{}) + var calls atomic.Int32 + p := NewDefaultTokenProvider(nil, nil, nil) + p.tatResolver = func(context.Context) (*TokenResult, error) { + if calls.Add(1) == 1 { + close(started) + } + <-release + return &TokenResult{Token: "shared-token"}, nil + } + + type outcome struct { + result *TokenResult + err error + } + outcomes := make(chan outcome, callers) + var ready sync.WaitGroup + ready.Add(callers) + begin := make(chan struct{}) + for i := 0; i < callers; i++ { + go func() { + ready.Done() + <-begin + result, err := p.resolveTAT(context.Background()) + outcomes <- outcome{result: result, err: err} + }() + } + ready.Wait() + close(begin) + select { + case <-started: + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for TAT resolution to start") + } + deadline := time.Now().Add(2 * time.Second) + for { + p.tatMu.Lock() + followers := p.tatFlight.followers + p.tatMu.Unlock() + if followers == callers-1 { + break + } + if time.Now().After(deadline) { + t.Fatalf("in-flight followers = %d, want %d", followers, callers-1) + } + runtime.Gosched() + } + close(release) + + for i := 0; i < callers; i++ { + select { + case got := <-outcomes: + if got.err != nil || got.result == nil || got.result.Token != "shared-token" { + t.Fatalf("concurrent resolution = (%v, %v), want shared success", got.result, got.err) + } + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for concurrent TAT resolution") + } + } + if got := calls.Load(); got != 1 { + t.Fatalf("resolver calls = %d, want 1", got) + } +} + // TestClassifyTATResponseCode_InvalidClient_MapsToInvalidClient pins that the // unified Token Endpoint's OAuth2 invalid_client error surfaces as // CategoryConfig/InvalidClient — the configured app_id/app_secret cannot mint a diff --git a/internal/credential/tat_fetch.go b/internal/credential/tat_fetch.go index 82173bcfd3..34cbde17e5 100644 --- a/internal/credential/tat_fetch.go +++ b/internal/credential/tat_fetch.go @@ -6,25 +6,18 @@ package credential import ( "context" "encoding/json" - "errors" "fmt" "io" "net/http" "net/url" - "strconv" "strings" + "time" - "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/core" + "github.com/larksuite/cli/internal/errclass" ) -type tatResponse struct { - Code int `json:"code"` - AccessToken string `json:"access_token"` - Error string `json:"error"` - ErrorDescription string `json:"error_description"` - Msg string `json:"msg"` -} +const maxTATResponseBodyBytes = 1 << 20 // FetchTAT performs a single HTTP POST to mint a tenant access token via the // unified OAuth 2.0 Token Endpoint ({accounts}/oauth/v3/token) using the @@ -38,9 +31,8 @@ type tatResponse struct { // doResolveTAT (and thus every token-resolving command) produces, so callers // see one consistent envelope. Transport failures, unreadable/unparseable // bodies, and transient server-side failures (5xx / server_error) are returned -// raw (untyped), leaving them ambiguous. HTTP 429 is the exception: it carries -// typed retry metadata so callers can back off instead of treating it as a -// credential rejection. +// raw (untyped), leaving them ambiguous. HTTP 429 is the exception: it returns +// a safe typed api/rate_limit error with bounded recovery metadata. // // The caller owns the context timeout. func FetchTAT(ctx context.Context, httpClient *http.Client, brand core.LarkBrand, appID, appSecret string) (string, error) { @@ -64,41 +56,27 @@ func FetchTAT(ctx context.Context, httpClient *http.Client, brand core.LarkBrand } defer resp.Body.Close() - body, err := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) + if resp.StatusCode == http.StatusTooManyRequests { + body, readErr := io.ReadAll(io.LimitReader(resp.Body, maxTATResponseBodyBytes+1)) + bodyOverflow := len(body) > maxTATResponseBodyBytes + var rateLimitResult any + if readErr == nil && !bodyOverflow { + rateLimitResult = errclass.ParseRateLimitJSON(body) + } + return "", errclass.ClassifyHTTPRateLimit(resp.StatusCode, resp.Header, rateLimitResult, nil, time.Now()) + } + body, err := io.ReadAll(io.LimitReader(resp.Body, maxTATResponseBodyBytes)) if err != nil { return "", fmt.Errorf("failed to read TAT response: %w", err) } - if resp.StatusCode == http.StatusTooManyRequests { - var rateLimitErr *errs.APIError - var result tatResponse - if json.Unmarshal(body, &result) == nil { - desc := result.ErrorDescription - if desc == "" { - desc = result.Msg - } - classified := classifyTATResponseCode(result.Code, result.Error, desc, string(brand), appID) - var apiErr *errs.APIError - if errors.As(classified, &apiErr) && - apiErr.Subtype == errs.SubtypeRateLimit && apiErr.Retryable { - rateLimitErr = apiErr - } - } - if rateLimitErr == nil { - rateLimitErr = errs.NewAPIError(errs.SubtypeRateLimit, "TAT endpoint rate limited (HTTP 429)"). - WithCode(http.StatusTooManyRequests). - WithRetryable() - } - if retryAfter := tatRetryAfterSeconds(resp.Header); retryAfter > 0 { - rateLimitErr.RetryAfterSeconds = retryAfter - rateLimitErr.Hint = fmt.Sprintf("wait at least %d seconds before retrying; if throttling continues, use exponential backoff with jitter", retryAfter) - } else { - rateLimitErr.Hint = "use exponential backoff with jitter when retrying" - } - return "", rateLimitErr + var result struct { + Code int `json:"code"` + AccessToken string `json:"access_token"` + Error string `json:"error"` + ErrorDescription string `json:"error_description"` + Msg string `json:"msg"` } - - var result tatResponse if err := json.Unmarshal(body, &result); err != nil { // An unparseable body is ambiguous (covers non-JSON error pages and // truncated payloads); stay untyped so probe callers treat it as noise. @@ -112,8 +90,8 @@ func FetchTAT(ctx context.Context, httpClient *http.Client, brand core.LarkBrand // Transient/server-side failures stay untyped so probe callers stay silent and // retryers can back off; only deterministic client rejections are typed. Covers // 5xx and the OAuth transient error strings (server_error, - // temporarily_unavailable, slow_down). HTTP 429 was already returned above - // as a typed rate-limit error with retry guidance and an upstream delay when available. + // temporarily_unavailable, slow_down) — matching the legacy "non-2xx is noise" + // behavior for OAuth transient errors. HTTP 429 was handled above. if resp.StatusCode >= 500 || result.Error == "server_error" || result.Error == "temporarily_unavailable" || result.Error == "slow_down" { @@ -135,13 +113,3 @@ func FetchTAT(ctx context.Context, httpClient *http.Client, brand core.LarkBrand } return "", classifyTATResponseCode(result.Code, result.Error, desc, string(brand), appID) } - -func tatRetryAfterSeconds(header http.Header) int { - for _, name := range []string{"X-Ogw-Ratelimit-Reset", "Retry-After"} { - seconds, err := strconv.Atoi(strings.TrimSpace(header.Get(name))) - if err == nil && seconds > 0 { - return seconds - } - } - return 0 -} diff --git a/internal/credential/tat_fetch_test.go b/internal/credential/tat_fetch_test.go index fc0b13768b..ca3cfddead 100644 --- a/internal/credential/tat_fetch_test.go +++ b/internal/credential/tat_fetch_test.go @@ -18,15 +18,18 @@ import ( // stubRoundTripper lets us assert request shape and return canned responses. type stubRoundTripper struct { - gotReq *http.Request - gotBody string - respCode int - respBody string - respHeader http.Header - err error + gotReq *http.Request + gotBody string + respCode int + respBody string + respRead io.ReadCloser + header http.Header + err error + calls int } func (s *stubRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + s.calls++ s.gotReq = req if req.Body != nil { b, _ := io.ReadAll(req.Body) @@ -35,17 +38,35 @@ func (s *stubRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) if s.err != nil { return nil, s.err } + body := s.respRead + if body == nil { + body = io.NopCloser(strings.NewReader(s.respBody)) + } header := make(http.Header) - if s.respHeader != nil { - header = s.respHeader.Clone() + if s.header != nil { + header = s.header.Clone() } return &http.Response{ StatusCode: s.respCode, - Body: io.NopCloser(strings.NewReader(s.respBody)), + Body: body, Header: header, }, nil } +type boundaryErrorReader struct { + prefix *strings.Reader + err error + boundaryReads int +} + +func (r *boundaryErrorReader) Read(p []byte) (int, error) { + if r.prefix.Len() > 0 { + return r.prefix.Read(p) + } + r.boundaryReads++ + return 0, r.err +} + func TestFetchTAT_Success(t *testing.T) { rt := &stubRoundTripper{ respCode: 200, @@ -179,80 +200,192 @@ func TestFetchTAT_ServerError_Untyped(t *testing.T) { } } -// HTTP 429 is actionable even while fetching TAT: surface a typed rate-limit -// error and carry the platform reset interval instead of misreporting a bad -// credential or returning an opaque transient error. -func TestFetchTAT_HTTP429_TypedRateLimit(t *testing.T) { +func TestFetchTAT_HTTP429TypedSafeRateLimit(t *testing.T) { tests := []struct { - name string - body string - header http.Header - wantCode int - wantDelay int + name string + body string + header http.Header + wantCode int + wantLog string + wantWait int + wantFrom string }{ - { - name: "platform envelope", - body: `{"code":99991400,"error":"too_many_requests","error_description":"rate limit exceeded"}`, - header: http.Header{"X-Ogw-Ratelimit-Reset": []string{"8"}, "Retry-After": []string{"4"}}, - wantCode: 99991400, - wantDelay: 8, - }, - { - name: "standard retry-after fallback", - body: `{"error":"too_many_requests"}`, - header: http.Header{"Retry-After": []string{"4"}}, - wantCode: http.StatusTooManyRequests, - wantDelay: 4, - }, - { - name: "non-JSON gateway response", - body: "rate limit exceeded", - wantCode: http.StatusTooManyRequests, - wantDelay: 0, - }, + {name: "business code", body: `{"code":99991400,"error_description":"secret\u0000oauth text","log_id":"body-log"}`, header: http.Header{"Retry-After": []string{"7"}, "X-Tt-Logid": []string{"header-log"}}, wantCode: 99991400, wantLog: "body-log", wantWait: 7, wantFrom: "retry-after"}, + {name: "platform reset header", body: `{"error":"too_many_requests"}`, header: http.Header{"X-Ogw-Ratelimit-Reset": []string{"8"}, "Retry-After": []string{"4"}}, wantCode: 429, wantWait: 8, wantFrom: "x-ogw-ratelimit-reset"}, + {name: "unrelated business code", body: `{"code":20002,"msg":"secret"}`, wantCode: 429, wantWait: 1}, + {name: "plain text", body: "secret plaintext", wantCode: 429, wantWait: 1}, + {name: "html", body: "secret", wantCode: 429, wantWait: 1}, + {name: "empty", wantCode: 429, wantWait: 1}, + {name: "malformed invalid utf8", body: "{\xffsecret", wantCode: 429, wantWait: 1}, + {name: "trailing junk cannot forge metadata", body: `{"code":99991400,"log_id":"forged"}trailing-junk`, wantCode: 429, wantWait: 1}, + {name: "concatenated values cannot forge metadata", body: `{"code":99991400,"log_id":"forged"}{"code":99991400}`, wantCode: 429, wantWait: 1}, + {name: "invalid log falls through", body: `{"log_id":"bad/value","error":{"log_id":"nested-ok"}}`, header: http.Header{"X-Tt-Logid": []string{"header-log"}}, wantCode: 429, wantLog: "nested-ok", wantWait: 1}, + {name: "multiple retry after defaults", header: http.Header{"Retry-After": []string{"3", "4"}}, wantCode: 429, wantWait: 1}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rt := &stubRoundTripper{respCode: 429, respBody: tt.body, header: tt.header} + _, err := FetchTAT(context.Background(), &http.Client{Transport: rt}, core.BrandFeishu, "cli_app", "secret_x") + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("error = %T (%v), want APIError", err, err) + } + if apiErr.Code != tt.wantCode || apiErr.Subtype != errs.SubtypeRateLimit || !apiErr.Retryable { + t.Fatalf("problem = %#v", apiErr.Problem) + } + if apiErr.Message != "request rate limit exceeded" || strings.Contains(apiErr.Hint, "secret") || apiErr.Cause != nil { + t.Fatalf("unsafe response detail leaked: %#v", apiErr) + } + if apiErr.LogID != tt.wantLog || apiErr.RetryAfterSeconds == nil || *apiErr.RetryAfterSeconds != tt.wantWait { + t.Fatalf("metadata = log %q retry %v, want %q/%d", apiErr.LogID, apiErr.RetryAfterSeconds, tt.wantLog, tt.wantWait) + } + if tt.wantFrom != "" && apiErr.RetryAfterSource != tt.wantFrom { + t.Fatalf("retry source = %q, want %q", apiErr.RetryAfterSource, tt.wantFrom) + } + if rt.calls != 1 { + t.Fatalf("request count = %d, want 1", rt.calls) + } + }) } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { +} + +func TestFetchTAT_HTTP429BoundedBodyDoesNotLeak(t *testing.T) { + prefix := `{"code":99991400,"log_id":"body-forged"}` + padding := strings.Repeat(" ", maxTATResponseBodyBytes-len(prefix)) + for _, suffix := range []string{ + "trailing-junk", + `{"code":99991400,"log_id":"second-forged"}`, + } { + t.Run(suffix, func(t *testing.T) { rt := &stubRoundTripper{ - respCode: http.StatusTooManyRequests, - respBody: tc.body, - respHeader: tc.header, + respCode: 429, + respBody: prefix + padding + suffix, + header: http.Header{"X-Request-Id": []string{"header-log"}, "Retry-After": []string{"11"}}, } - hc := &http.Client{Transport: rt} - - _, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") + _, err := FetchTAT(context.Background(), &http.Client{Transport: rt}, core.BrandFeishu, "cli_app", "secret_x") var apiErr *errs.APIError if !errors.As(err, &apiErr) { - t.Fatalf("HTTP 429 error = %T %v, want *errs.APIError", err, err) + t.Fatalf("error = %T (%v), want APIError", err, err) } - if apiErr.Subtype != errs.SubtypeRateLimit || !apiErr.Retryable { - t.Fatalf("problem = %+v, want retryable api/rate_limit", apiErr.Problem) + if apiErr.Code != 429 || apiErr.LogID != "header-log" { + t.Fatalf("overflow body supplied metadata: %#v", apiErr.Problem) } - if apiErr.Code != tc.wantCode { - t.Fatalf("code = %d, want %d", apiErr.Code, tc.wantCode) + if apiErr.RetryAfterSeconds == nil || *apiErr.RetryAfterSeconds != 11 || apiErr.RetryAfterSource != "retry-after" { + t.Fatalf("header retry metadata = (%v, %q), want (11, retry-after)", apiErr.RetryAfterSeconds, apiErr.RetryAfterSource) } - if apiErr.RetryAfterSeconds != tc.wantDelay { - t.Fatalf("retry_after_seconds = %v, want %d", apiErr.RetryAfterSeconds, tc.wantDelay) + if apiErr.Message != "request rate limit exceeded" || strings.Contains(apiErr.Error(), "forged") || apiErr.Cause != nil { + t.Fatalf("bounded response leaked into error: %#v", apiErr) } - if !strings.Contains(apiErr.Hint, "exponential backoff with jitter") { - t.Fatalf("hint = %q, want backoff guidance", apiErr.Hint) + if rt.calls != 1 { + t.Fatalf("request count = %d, want 1", rt.calls) } }) } } -// OAuth slow_down without HTTP 429 remains an ambiguous transient response; -// it has no OpenAPI reset header from which to produce precise retry metadata. -func TestFetchTAT_OAuthSlowDown_Untyped(t *testing.T) { - rt := &stubRoundTripper{respCode: 200, respBody: `{"error":"slow_down","error_description":"polling too fast"}`} - hc := &http.Client{Transport: rt} +func TestFetchTAT_Non429OverflowKeepsOneMiBPrefixSemantics(t *testing.T) { + prefix := `{"code":0,"access_token":"t-ok"}` + body := prefix + strings.Repeat(" ", maxTATResponseBodyBytes-len(prefix)) + "trailing-junk" + rt := &stubRoundTripper{respCode: http.StatusOK, respBody: body} + token, err := FetchTAT(context.Background(), &http.Client{Transport: rt}, core.BrandFeishu, "cli_app", "secret_x") + if err != nil || token != "t-ok" { + t.Fatalf("FetchTAT() = (%q, %v), want historical 1 MiB prefix success", token, err) + } + if rt.calls != 1 { + t.Fatalf("request count = %d, want 1", rt.calls) + } +} - _, err := FetchTAT(context.Background(), hc, core.BrandFeishu, "cli_app", "secret_x") - if err == nil { - t.Fatal("expected error for slow_down") +func TestFetchTAT_ReadBoundaryCompatibilityByHTTPStatus(t *testing.T) { + errorsAfterBoundary := []struct { + name string + err error + }{ + {name: "sentinel", err: errors.New("read beyond legacy boundary")}, + {name: "unexpected EOF", err: io.ErrUnexpectedEOF}, + } + for _, tt := range errorsAfterBoundary { + t.Run(tt.name, func(t *testing.T) { + successPrefix := `{"code":0,"access_token":"t-ok"}` + successBody := successPrefix + strings.Repeat(" ", maxTATResponseBodyBytes-len(successPrefix)) + successReader := &boundaryErrorReader{prefix: strings.NewReader(successBody), err: tt.err} + successRT := &stubRoundTripper{respCode: http.StatusOK, respRead: io.NopCloser(successReader)} + token, err := FetchTAT(context.Background(), &http.Client{Transport: successRT}, core.BrandFeishu, "cli_app", "secret_x") + if err != nil || token != "t-ok" { + t.Fatalf("HTTP 200 FetchTAT() = (%q, %v), want legacy prefix success", token, err) + } + if successReader.boundaryReads != 0 { + t.Fatalf("HTTP 200 read beyond 1 MiB %d times, want 0", successReader.boundaryReads) + } + + ratePrefix := `{"code":99991400,"log_id":"body-forged"}` + rateBody := ratePrefix + strings.Repeat(" ", maxTATResponseBodyBytes-len(ratePrefix)) + rateReader := &boundaryErrorReader{prefix: strings.NewReader(rateBody), err: tt.err} + rateRT := &stubRoundTripper{ + respCode: http.StatusTooManyRequests, + respRead: io.NopCloser(rateReader), + header: http.Header{"X-Tt-Logid": []string{"header-log"}, "Retry-After": []string{"6"}}, + } + _, err = FetchTAT(context.Background(), &http.Client{Transport: rateRT}, core.BrandFeishu, "cli_app", "secret_x") + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("HTTP 429 error = %T (%v), want safe APIError", err, err) + } + if apiErr.Code != 429 || apiErr.LogID != "header-log" || apiErr.RetryAfterSeconds == nil || *apiErr.RetryAfterSeconds != 6 || apiErr.Cause != nil { + t.Fatalf("HTTP 429 boundary classification = %#v", apiErr) + } + if rateReader.boundaryReads != 1 { + t.Fatalf("HTTP 429 boundary probes = %d, want 1", rateReader.boundaryReads) + } + }) } - if errs.IsTyped(err) { - t.Errorf("slow_down without HTTP 429 must stay untyped, got %T %v", err, err) +} + +func TestFetchTAT_EarlyBodyReadErrorByHTTPStatus(t *testing.T) { + errorsBeforeBoundary := []struct { + name string + err error + }{ + {name: "sentinel", err: errors.New("early body read failure")}, + {name: "unexpected EOF", err: io.ErrUnexpectedEOF}, + } + for _, tt := range errorsBeforeBoundary { + t.Run(tt.name, func(t *testing.T) { + partialBody := `{"code":99991400,"log_id":"body-forged"}` + + nonRateReader := &boundaryErrorReader{prefix: strings.NewReader(partialBody), err: tt.err} + nonRateRT := &stubRoundTripper{respCode: http.StatusOK, respRead: io.NopCloser(nonRateReader)} + _, err := FetchTAT(context.Background(), &http.Client{Transport: nonRateRT}, core.BrandFeishu, "cli_app", "secret_x") + if !errors.Is(err, tt.err) { + t.Fatalf("HTTP 200 error = %v, want wrapped read error %v", err, tt.err) + } + + rateReader := &boundaryErrorReader{prefix: strings.NewReader(partialBody), err: tt.err} + rateRT := &stubRoundTripper{ + respCode: http.StatusTooManyRequests, + respRead: io.NopCloser(rateReader), + header: http.Header{"X-Request-Id": []string{"header-log"}, "Retry-After": []string{"8"}}, + } + _, err = FetchTAT(context.Background(), &http.Client{Transport: rateRT}, core.BrandFeishu, "cli_app", "secret_x") + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("HTTP 429 error = %T (%v), want APIError", err, err) + } + if apiErr.Code != 429 || apiErr.LogID != "header-log" || apiErr.RetryAfterSeconds == nil || *apiErr.RetryAfterSeconds != 8 { + t.Fatalf("HTTP 429 early-read classification = %#v", apiErr) + } + if apiErr.Cause != nil || errors.Is(err, tt.err) || strings.Contains(apiErr.Message, tt.err.Error()) || strings.Contains(apiErr.Hint, tt.err.Error()) { + t.Fatalf("HTTP 429 leaked read error: %#v", apiErr) + } + }) + } +} + +func TestFetchTAT_OAuthSlowDownRemainsUntyped(t *testing.T) { + rt := &stubRoundTripper{respCode: 200, respBody: `{"error":"slow_down","error_description":"polling too fast"}`} + _, err := FetchTAT(context.Background(), &http.Client{Transport: rt}, core.BrandFeishu, "cli_app", "secret_x") + if err == nil || errs.IsTyped(err) { + t.Fatalf("slow_down error = %T (%v), want non-nil untyped", err, err) } } diff --git a/internal/errclass/http_rate_limit.go b/internal/errclass/http_rate_limit.go new file mode 100644 index 0000000000..77f86ba36f --- /dev/null +++ b/internal/errclass/http_rate_limit.go @@ -0,0 +1,279 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package errclass + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "math" + "math/big" + "net/http" + "strconv" + "strings" + "time" + + "github.com/larksuite/cli/errs" +) + +const ( + defaultRetryAfterSeconds = 1 + maxRetryAfterSeconds = 24 * 60 * 60 + retryAfterSourceOGW = "x-ogw-ratelimit-reset" + retryAfterSourceHeader = "retry-after" + retryAfterSourceDefault = "default" + // RateLimitMessage is the stable user-facing message for short-term limits. + RateLimitMessage = "request rate limit exceeded" +) + +// ClassifyHTTPRateLimit classifies only HTTP 429 responses. Business-code +// rate limits returned with another HTTP status remain the caller's concern. +func ClassifyHTTPRateLimit(status int, header http.Header, result any, classified error, now time.Time) error { + if status != http.StatusTooManyRequests { + return nil + } + + businessRateLimit := IsBusinessRateLimit(result) + if !businessRateLimit && classified != nil { + if problem, ok := errs.ProblemOf(classified); ok { + problem.LogID = RateLimitLogID(result, header) + } + return classified + } + + code := http.StatusTooManyRequests + if businessRateLimit { + code = 99991400 + } + + var apiErr *errs.APIError + if businessRateLimit { + var existing *errs.APIError + if errors.As(classified, &existing) { + apiErr = existing + } + } + if apiErr == nil { + apiErr = errs.NewAPIError(errs.SubtypeRateLimit, RateLimitMessage).WithCode(code) + } + if businessRateLimit { + apiErr.Subtype = errs.SubtypeRateLimit + apiErr.Code = code + apiErr.Message = RateLimitMessage + } + + // Derive the identifier exclusively from validated structured/header + // sources. This also clears an unsafe value set by an earlier classifier. + apiErr.LogID = RateLimitLogID(result, header) + seconds, source := ParseRetryAfter(header, now) + apiErr.Hint = MergeRateLimitHint(apiErr.Hint, RateLimitGuidance(seconds)) + return apiErr.WithRetryable().WithRetryAfter(seconds, source) +} + +// ParseRetryAfter prefers Lark's numeric reset header, then accepts exactly one +// bounded Retry-After value. Ambiguous or malformed values use the conservative +// default. +func ParseRetryAfter(header http.Header, now time.Time) (int, string) { + if seconds, ok := parsePositiveDeltaSeconds(header.Values("X-Ogw-Ratelimit-Reset")); ok { + return seconds, retryAfterSourceOGW + } + values := header.Values("Retry-After") + if len(values) != 1 { + return defaultRetryAfterSeconds, retryAfterSourceDefault + } + rawValue := values[0] + if len(rawValue) > 128 { + return defaultRetryAfterSeconds, retryAfterSourceDefault + } + value := strings.TrimSpace(rawValue) + if value == "" { + return defaultRetryAfterSeconds, retryAfterSourceDefault + } + + allDigits := true + for i := 0; i < len(value); i++ { + if value[i] < '0' || value[i] > '9' { + allDigits = false + break + } + } + if allDigits { + seconds, err := strconv.ParseInt(value, 10, 64) + if err == nil && seconds <= maxRetryAfterSeconds { + return int(seconds), retryAfterSourceHeader + } + return defaultRetryAfterSeconds, retryAfterSourceDefault + } + + retryAt, err := http.ParseTime(value) + if err != nil { + return defaultRetryAfterSeconds, retryAfterSourceDefault + } + delay := retryAt.Sub(now) + if delay <= 0 || delay > maxRetryAfterSeconds*time.Second { + return defaultRetryAfterSeconds, retryAfterSourceDefault + } + seconds := int((delay + time.Second - 1) / time.Second) + if seconds > maxRetryAfterSeconds { + return defaultRetryAfterSeconds, retryAfterSourceDefault + } + return seconds, retryAfterSourceHeader +} + +func parsePositiveDeltaSeconds(values []string) (int, bool) { + if len(values) != 1 || len(values[0]) > 128 { + return 0, false + } + value := strings.TrimSpace(values[0]) + if value == "" { + return 0, false + } + for i := 0; i < len(value); i++ { + if value[i] < '0' || value[i] > '9' { + return 0, false + } + } + seconds, err := strconv.ParseInt(value, 10, 64) + if err != nil || seconds <= 0 || seconds > maxRetryAfterSeconds { + return 0, false + } + return int(seconds), true +} + +var errTrailingJSONContent = errors.New("trailing content after JSON value") + +// DecodeSingleJSON decodes one JSON value and accepts only JSON whitespace +// after it. The remaining bytes are inspected directly so a trailing value is +// never decoded or allocated. +func DecodeSingleJSON(rawBody []byte) (any, error) { + decoder := json.NewDecoder(bytes.NewReader(rawBody)) + decoder.UseNumber() + var result any + if err := decoder.Decode(&result); err != nil { + return nil, err + } + for _, c := range rawBody[decoder.InputOffset():] { + if c != ' ' && c != '\t' && c != '\r' && c != '\n' { + return nil, errTrailingJSONContent + } + } + return result, nil +} + +// ParseRateLimitJSON decodes exactly one complete JSON value. A malformed +// value, trailing non-whitespace, or a second JSON value is rejected. +func ParseRateLimitJSON(rawBody []byte) any { + result, err := DecodeSingleJSON(rawBody) + if err != nil { + return nil + } + return result +} + +// RateLimitLogID returns the first valid identifier from the documented +// structured-body and response-header sources. +func RateLimitLogID(result any, header http.Header) string { + if resultMap, ok := result.(map[string]any); ok { + if logID := validRateLimitLogID(resultMap["log_id"]); logID != "" { + return logID + } + if errBlock, ok := resultMap["error"].(map[string]any); ok { + if logID := validRateLimitLogID(errBlock["log_id"]); logID != "" { + return logID + } + } + } + for _, name := range []string{"X-Tt-Logid", "X-Request-Id"} { + values := header.Values(name) + if len(values) == 1 { + if logID := validRateLimitLogID(values[0]); logID != "" { + return logID + } + } + } + return "" +} + +func validRateLimitLogID(value any) string { + logID, ok := value.(string) + if !ok { + return "" + } + logID = strings.TrimSpace(logID) + if len(logID) < 1 || len(logID) > 128 { + return "" + } + for i := 0; i < len(logID); i++ { + c := logID[i] + if (c >= 'a' && c <= 'z') || (c >= 'A' && c <= 'Z') || + (c >= '0' && c <= '9') || c == '.' || c == '_' || c == '-' { + continue + } + return "" + } + return logID +} + +// IsBusinessRateLimit reports whether result carries the exact integer Lark +// short-term rate-limit code. +func IsBusinessRateLimit(result any) bool { + resultMap, ok := result.(map[string]any) + if !ok { + return false + } + return exactBusinessRateLimitCode(resultMap["code"]) +} + +func exactBusinessRateLimitCode(value any) bool { + const target int64 = 99991400 + switch code := value.(type) { + case int: + return int64(code) == target + case int8: + return int64(code) == target + case int16: + return int64(code) == target + case int32: + return int64(code) == target + case int64: + return code == target + case uint: + return uint64(code) == uint64(target) + case uint8: + return uint64(code) == uint64(target) + case uint16: + return uint64(code) == uint64(target) + case uint32: + return uint64(code) == uint64(target) + case uint64: + return code == uint64(target) + case float32: + value := float64(code) + return !math.IsInf(value, 0) && !math.IsNaN(value) && math.Trunc(value) == value && value == float64(target) + case float64: + return !math.IsInf(code, 0) && !math.IsNaN(code) && math.Trunc(code) == code && code == float64(target) + case json.Number: + rational, ok := new(big.Rat).SetString(string(code)) + return ok && rational.IsInt() && rational.Num().IsInt64() && rational.Num().Int64() == target + default: + return false + } +} + +// RateLimitGuidance returns the canonical retry scheduling and replay-safety hint. +func RateLimitGuidance(seconds int) string { + return fmt.Sprintf("wait %d seconds before reevaluating; retryable does not mean a write request is safe to replay—verify the operation result or idempotency before retrying", seconds) +} + +// MergeRateLimitHint appends the canonical guidance without duplicating it. +func MergeRateLimitHint(existing, guidance string) string { + if existing == "" || existing == guidance { + return guidance + } + if strings.Contains(existing, guidance) { + return existing + } + return existing + "; " + guidance +} diff --git a/internal/errclass/http_rate_limit_test.go b/internal/errclass/http_rate_limit_test.go new file mode 100644 index 0000000000..ff20b66d7a --- /dev/null +++ b/internal/errclass/http_rate_limit_test.go @@ -0,0 +1,209 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package errclass + +import ( + "encoding/json" + "errors" + "math" + "net/http" + "strings" + "testing" + "time" + + "github.com/larksuite/cli/errs" +) + +func TestClassifyHTTPRateLimit_StatusIsolation(t *testing.T) { + if got := ClassifyHTTPRateLimit(http.StatusBadRequest, nil, map[string]any{"code": 99991400}, nil, time.Now()); got != nil { + t.Fatalf("non-429 classification = %v, want nil", got) + } +} + +func TestClassifyHTTPRateLimit_RetryAfterSafety(t *testing.T) { + now := time.Date(2026, 8, 5, 0, 0, 0, 250_000_000, time.UTC) + tests := []struct { + name string + values []string + want int + source string + }{ + {name: "zero", values: []string{"0"}, want: 0, source: "retry-after"}, + {name: "one day", values: []string{"86400"}, want: 86400, source: "retry-after"}, + {name: "date rounds up", values: []string{now.Add(2250 * time.Millisecond).Format(http.TimeFormat)}, want: 2, source: "retry-after"}, + {name: "multiple", values: []string{"2", "3"}, want: 1, source: "default"}, + {name: "negative", values: []string{"-1"}, want: 1, source: "default"}, + {name: "overflow", values: []string{"999999999999999999999999"}, want: 1, source: "default"}, + {name: "too large", values: []string{"86401"}, want: 1, source: "default"}, + {name: "expired date", values: []string{now.Add(-time.Second).Format(http.TimeFormat)}, want: 1, source: "default"}, + {name: "too long", values: []string{strings.Repeat("1", 129)}, want: 1, source: "default"}, + {name: "raw too long before trim", values: []string{strings.Repeat(" ", 128) + "0"}, want: 1, source: "default"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + header := make(http.Header) + if tt.values != nil { + header["Retry-After"] = tt.values + } + err := ClassifyHTTPRateLimit(http.StatusTooManyRequests, header, nil, nil, now) + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("error = %T (%v), want APIError", err, err) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryAPI || problem.Subtype != errs.SubtypeRateLimit { + t.Fatalf("problem = %#v, want api/rate_limit", problem) + } + if apiErr.RetryAfterSeconds == nil || *apiErr.RetryAfterSeconds != tt.want || apiErr.RetryAfterSource != tt.source { + t.Fatalf("retry metadata = (%v, %q), want (%d, %q)", apiErr.RetryAfterSeconds, apiErr.RetryAfterSource, tt.want, tt.source) + } + }) + } +} + +func TestClassifyHTTPRateLimit_OGWResetPrecedenceAndFallback(t *testing.T) { + tests := []struct { + name string + header http.Header + want int + source string + }{ + {name: "OGW reset preferred", header: http.Header{"X-Ogw-Ratelimit-Reset": []string{"8"}, "Retry-After": []string{"4"}}, want: 8, source: "x-ogw-ratelimit-reset"}, + {name: "invalid OGW falls back", header: http.Header{"X-Ogw-Ratelimit-Reset": []string{"invalid"}, "Retry-After": []string{"4"}}, want: 4, source: "retry-after"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + seconds, source := ParseRetryAfter(tt.header, time.Now()) + if seconds != tt.want || source != tt.source { + t.Fatalf("ParseRetryAfter() = (%d, %q), want (%d, %q)", seconds, source, tt.want, tt.source) + } + }) + } +} + +func TestClassifyHTTPRateLimit_BusinessCodeMustBeExactInteger(t *testing.T) { + tests := []struct { + name string + code any + want int + }{ + {name: "exact json number", code: json.Number("99991400"), want: 99991400}, + {name: "exact decimal json number", code: json.Number("99991400.0"), want: 99991400}, + {name: "exact exponent json number", code: json.Number("9.99914e7"), want: 99991400}, + {name: "fractional json number", code: json.Number("99991400.5"), want: 429}, + {name: "overflow json number", code: json.Number("999914000000000000000000000"), want: 429}, + {name: "fractional float", code: float64(99991400.5), want: 429}, + {name: "infinite float", code: math.Inf(1), want: 429}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var apiErr *errs.APIError + if !errors.As(ClassifyHTTPRateLimit(429, nil, map[string]any{"code": tt.code}, nil, time.Now()), &apiErr) { + t.Fatal("expected APIError") + } + if apiErr.Code != tt.want { + t.Fatalf("Code = %d, want %d", apiErr.Code, tt.want) + } + }) + } +} + +func TestParseRateLimitJSONRequiresSingleCompleteValue(t *testing.T) { + tests := []struct { + name string + body string + ok bool + }{ + {name: "single value", body: `{"code":99991400}`, ok: true}, + {name: "trailing whitespace", body: "{\"code\":99991400}\n\t", ok: true}, + {name: "trailing junk", body: `{"code":99991400,"log_id":"forged"}trailing-junk`}, + {name: "concatenated values", body: `{"code":99991400}{"log_id":"forged"}`}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := ParseRateLimitJSON([]byte(tt.body)); (got != nil) != tt.ok { + t.Fatalf("ParseRateLimitJSON() = %#v, want ok=%v", got, tt.ok) + } + }) + } +} + +func TestParseRateLimitJSONRejectsLargeTrailingObject(t *testing.T) { + body := []byte(`{"code":99991400}{"blob":"` + strings.Repeat("界", 1<<20) + `"}`) + if got := ParseRateLimitJSON(body); got != nil { + t.Fatalf("ParseRateLimitJSON() = %#v, want nil for large trailing value", got) + } +} + +func TestClassifyHTTPRateLimit_LogIDAllowlistAndPrecedence(t *testing.T) { + tests := []struct { + name string + result any + header http.Header + want string + }{ + {name: "top level", result: map[string]any{"log_id": " top.1_2-3 ", "error": map[string]any{"log_id": "nested"}}, header: http.Header{"X-Tt-Logid": []string{"header"}}, want: "top.1_2-3"}, + {name: "nested", result: map[string]any{"log_id": "bad/id", "error": map[string]any{"log_id": "nested"}}, header: http.Header{"X-Tt-Logid": []string{"header"}}, want: "nested"}, + {name: "tt header", result: map[string]any{"log_id": "bad\nvalue"}, header: http.Header{"X-Tt-Logid": []string{"tt-log"}, "X-Request-Id": []string{"request-log"}}, want: "tt-log"}, + {name: "request header", header: http.Header{"X-Tt-Logid": []string{"bad/value"}, "X-Request-Id": []string{"request-log"}}, want: "request-log"}, + {name: "invalid all", result: map[string]any{"log_id": strings.Repeat("a", 129)}, header: http.Header{"X-Tt-Logid": []string{"bad value"}}, want: ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var apiErr *errs.APIError + if !errors.As(ClassifyHTTPRateLimit(429, tt.header, tt.result, nil, time.Now()), &apiErr) { + t.Fatal("expected APIError") + } + if apiErr.LogID != tt.want { + t.Fatalf("LogID = %q, want %q", apiErr.LogID, tt.want) + } + }) + } +} + +func TestClassifyHTTPRateLimit_FixedSafeMessageAndBusinessCode(t *testing.T) { + err := ClassifyHTTPRateLimit(429, nil, map[string]any{ + "code": 99991400, + "msg": "secret\x00oauth description", + }, nil, time.Now()) + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("error = %T (%v), want APIError", err, err) + } + if apiErr.Code != 99991400 || apiErr.Message != "request rate limit exceeded" || !apiErr.Retryable { + t.Fatalf("API error = %#v", apiErr) + } + if apiErr.Cause != nil || strings.Contains(apiErr.Hint, "secret") { + t.Fatalf("unsafe payload leaked: %#v", apiErr) + } +} + +func TestClassifyHTTPRateLimit_ReclassifiesExistingAPIErrorAsBusinessRateLimit(t *testing.T) { + original := errs.NewAPIError(errs.SubtypeUnknown, "original classification").WithCode(12345) + err := ClassifyHTTPRateLimit(http.StatusTooManyRequests, nil, map[string]any{ + "code": 99991400, + }, original, time.Now()) + + if err != original { + t.Fatalf("classification = %T (%v), want original APIError pointer", err, err) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryAPI || problem.Subtype != errs.SubtypeRateLimit { + t.Fatalf("problem = %#v, want api/rate_limit", problem) + } + if original.Code != 99991400 || original.Message != RateLimitMessage { + t.Fatalf("reclassified API error = %#v, want api/rate_limit code 99991400", original) + } +} + +func TestClassifyHTTPRateLimit_PreservedClassificationStillSanitizesLogID(t *testing.T) { + original := errs.NewAPIError(errs.SubtypeRateLimit, "daily quota").WithCode(1063006).WithLogID("unsafe/log") + got := ClassifyHTTPRateLimit(429, http.Header{"X-Request-Id": []string{"request-id"}}, map[string]any{"code": 1063006}, original, time.Now()) + if got != original || original.Retryable || original.RetryAfterSeconds != nil { + t.Fatalf("classification changed: %#v", original) + } + if original.LogID != "request-id" { + t.Fatalf("LogID = %q, want sanitized header fallback", original.LogID) + } +} diff --git a/internal/recovery/render.go b/internal/recovery/render.go index 0b7d3ce2c4..013d5e912d 100644 --- a/internal/recovery/render.go +++ b/internal/recovery/render.go @@ -31,6 +31,10 @@ func renderWithContext(err error, plan *surface.Plan, context RenderContext) err if !ok { return err } + if paginationErr, ok := typed.(*errs.PaginationError); ok { //nolint:errorlint + renderedCause := renderWithContext(paginationErr.Cause, plan, context) + return errs.NewPaginationError(renderedCause, paginationErr.CompletedPages, paginationErr.NextPageToken) + } sourceProblem, ok := errs.ProblemOf(typed) if !ok { return err @@ -84,6 +88,15 @@ func CloneTyped(err error) (error, bool) { // producer. This switch must inspect that exact concrete value rather than // search through its Cause and accidentally clone a nested typed error. switch original := typed.(type) { //nolint:errorlint + case *errs.PaginationError: + if original == nil { + return nil, false + } + cause, ok := CloneTyped(original.Cause) + if !ok { + return nil, false + } + return errs.NewPaginationError(cause, original.CompletedPages, original.NextPageToken), true case *errs.Problem: if original == nil { return nil, false diff --git a/internal/recovery/render_test.go b/internal/recovery/render_test.go index 9bac54a6ef..84fa04bc78 100644 --- a/internal/recovery/render_test.go +++ b/internal/recovery/render_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "errors" "reflect" + "strings" "testing" "github.com/larksuite/cli/errs" @@ -31,6 +32,7 @@ func TestRenderClonesEveryConcreteTypedErrorAndPreservesWireExtensions(t *testin Retryable: true, } } + retryAfterSeconds := 7 tests := []struct { name string @@ -101,7 +103,7 @@ func TestRenderClonesEveryConcreteTypedErrorAndPreservesWireExtensions(t *testin name: "api", original: &errs.APIError{ Problem: problem(errs.CategoryAPI, errs.SubtypeRateLimit), - RetryAfterSeconds: 7, + RetryAfterSeconds: &retryAfterSeconds, Cause: sentinel, }, }, @@ -200,6 +202,66 @@ func TestRenderProjectsStructuredMessageWithoutMutatingSource(t *testing.T) { } } +func TestRenderProjectsPaginationCauseAndRebuildsSnapshot(t *testing.T) { + hint := Join("; ", + Command(TargetConfigInit, "run `lark-cli config init`"), + Text("inspect logs"), + ) + inner := Annotate( + errs.NewAPIError(errs.SubtypeRateLimit, "limited").WithHint("%s", hint.String()), + hint, + ) + original := errs.NewPaginationError(inner, 2, "resume-page-3") + plan := surface.NewPlan(map[surface.CommandID]surface.CommandState{ + surface.CommandConfigInit: surface.CommandConcealed, + }) + + rendered := Render(original, plan) + var paginationErr *errs.PaginationError + if !errors.As(rendered, &paginationErr) { + t.Fatalf("Render() = %T, want PaginationError", rendered) + } + encoded, err := json.Marshal(rendered) + if err != nil { + t.Fatalf("json.Marshal(rendered): %v", err) + } + var wire map[string]interface{} + if err := json.Unmarshal(encoded, &wire); err != nil { + t.Fatalf("json.Unmarshal(rendered): %v", err) + } + if wire["hint"] != "inspect logs" || wire["completed_pages"] != float64(2) || wire["next_page_token"] != "resume-page-3" { + t.Fatalf("rendered wire = %#v, want projected hint and pagination progress", wire) + } + originalProblem, _ := errs.ProblemOf(original) + if originalProblem.Hint != hint.String() { + t.Fatalf("Render mutated producer hint to %q", originalProblem.Hint) + } +} + +func TestProjectorPreservesRenderContextInsidePaginationError(t *testing.T) { + hint := UserAuthorization() + inner := Annotate( + errs.NewAuthenticationError(errs.SubtypeTokenMissing, "authorization required").WithHint("%s", hint.String()), + hint, + ) + original := errs.NewPaginationError(inner, 1, "resume-page-2") + projector := NewProjectorWithContext(nil, RenderContext{Profile: "team-beta"}) + + rendered := projector.Render(original) + encoded, err := json.Marshal(rendered) + if err != nil { + t.Fatalf("json.Marshal(rendered): %v", err) + } + var wire map[string]interface{} + if err := json.Unmarshal(encoded, &wire); err != nil { + t.Fatalf("json.Unmarshal(rendered): %v", err) + } + hintText, _ := wire["hint"].(string) + if !strings.Contains(hintText, "--profile='team-beta'") { + t.Fatalf("pagination hint lost render context: %#v", wire) + } +} + func TestRenderPreservesProducerEnrichmentAddedAfterAnnotation(t *testing.T) { hint := Join("; ", Command(TargetConfigInit, "run `lark-cli config init`"), diff --git a/shortcuts/common/call_api_typed_test.go b/shortcuts/common/call_api_typed_test.go index c8b7d3e1cd..55b957515d 100644 --- a/shortcuts/common/call_api_typed_test.go +++ b/shortcuts/common/call_api_typed_test.go @@ -7,6 +7,7 @@ import ( "context" "errors" "net/http" + "strings" "testing" "github.com/spf13/cobra" @@ -98,6 +99,69 @@ func TestCallAPITyped_Success(t *testing.T) { } } +func TestCallAPITyped_RateLimitMetadataFromResponseHeader(t *testing.T) { + for _, status := range []int{http.StatusOK, http.StatusBadRequest} { + t.Run(http.StatusText(status), func(t *testing.T) { + rt, reg := newCallAPITypedRuntime(t) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/x/y", + Status: status, + Headers: http.Header{ + "Content-Type": []string{"application/json"}, + "Retry-After": []string{"11"}, + }, + Body: map[string]interface{}{"code": float64(99991400), "msg": "slow"}, + }) + + _, err := rt.CallAPITyped("POST", "/open-apis/x/y", nil, map[string]any{"write": true}) + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("CallAPITyped() error = %T (%v), want *errs.APIError", err, err) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryAPI || problem.Subtype != errs.SubtypeRateLimit { + t.Fatalf("problem = %#v, want api/rate_limit", problem) + } + if apiErr.Subtype != errs.SubtypeRateLimit || apiErr.Code != 99991400 || !apiErr.Retryable { + t.Fatalf("rate limit problem = %#v", apiErr.Problem) + } + if apiErr.RetryAfterSeconds == nil || *apiErr.RetryAfterSeconds != 11 || apiErr.RetryAfterSource != "retry-after" { + t.Fatalf("retry metadata = (%v, %q), want (11, retry-after)", apiErr.RetryAfterSeconds, apiErr.RetryAfterSource) + } + if !strings.Contains(apiErr.Hint, "safe to replay") { + t.Fatalf("hint does not warn against unsafe write replay: %q", apiErr.Hint) + } + }) + } +} + +func TestCallAPITyped_BareHTTP429PrefersBodyLogID(t *testing.T) { + rt, reg := newCallAPITypedRuntime(t) + reg.Register(&httpmock.Stub{ + Method: "POST", + URL: "/open-apis/x/y", + Status: http.StatusTooManyRequests, + Headers: http.Header{ + "Content-Type": []string{"application/json"}, + "X-Tt-Logid": []string{"header-log"}, + }, + Body: map[string]interface{}{ + "msg": "slow", + "error": map[string]interface{}{"log_id": "nested-log"}, + }, + }) + + _, err := rt.CallAPITyped("POST", "/open-apis/x/y", nil, map[string]any{}) + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("CallAPITyped() error = %T (%v), want *errs.APIError", err, err) + } + if apiErr.LogID != "nested-log" { + t.Fatalf("LogID = %q, want nested body log before header fallback", apiErr.LogID) + } +} + // TestAPIClassifyContext verifies the classify context is built from the // runtime: Brand / AppID from config, Identity from the resolved caller, and // LarkCmd from the running command path. diff --git a/shortcuts/common/runner.go b/shortcuts/common/runner.go index e12224f0cd..37f7c0bc9d 100644 --- a/shortcuts/common/runner.go +++ b/shortcuts/common/runner.go @@ -139,6 +139,9 @@ func (ctx *RuntimeContext) fetchBotInfo() (*BotInfo, error) { return nil, fmt.Errorf("fetch bot info: %w", err) } if resp.StatusCode >= 400 { + if _, classified := ctx.ClassifyAPIResponse(resp); classified != nil { + return nil, classified + } return nil, fmt.Errorf("fetch bot info: HTTP %d", resp.StatusCode) } // /open-apis/bot/v3/info returns `{code, msg, bot: {...}}` — the bot @@ -155,6 +158,9 @@ func (ctx *RuntimeContext) fetchBotInfo() (*BotInfo, error) { return nil, fmt.Errorf("fetch bot info: unmarshal: %w", err) } if envelope.Code != 0 { + if _, classified := ctx.ClassifyAPIResponse(resp); classified != nil { + return nil, classified + } return nil, fmt.Errorf("fetch bot info: [%d] %s", envelope.Code, envelope.Msg) } if envelope.Data.OpenID == "" { @@ -326,58 +332,52 @@ func (ctx *RuntimeContext) ClassifyAPIResponse(resp *larkcore.ApiResp) (map[stri // classification context. func ClassifyAPIResponseWith(resp *larkcore.ApiResp, cc errclass.ClassifyContext) (map[string]interface{}, error) { logID, _ := logIDFromHeader(resp)["log_id"].(string) - - result, parseErr := client.ParseJSONResponse(resp) - if parseErr != nil { - if resp.StatusCode >= 400 { - return nil, httpStatusError(resp.StatusCode, resp.RawBody, logID) + result, classified := client.ClassifyAPIResponse(resp, func(result interface{}) error { + resultMap, ok := result.(map[string]interface{}) + if !ok { + return nil + } + if logID != "" && responseBodyLogID(resultMap) == "" { + if _, present := resultMap["log_id"]; !present { + resultMap["log_id"] = logID + } + } + return errclass.BuildAPIError(resultMap, cc) + }) + if resp != nil && resp.StatusCode >= http.StatusInternalServerError { + var networkErr *errs.NetworkError + if errors.As(classified, &networkErr) && networkErr.Subtype == errs.SubtypeNetworkServer { + networkErr.Retryable = true } - return nil, client.WrapJSONResponseParseError(parseErr, resp.RawBody) } resultMap, ok := result.(map[string]interface{}) if !ok { + if classified != nil { + return nil, classified + } e := errs.NewInternalError(errs.SubtypeInvalidResponse, "API returned a non-object JSON response") if logID != "" { e = e.WithLogID(logID) } return nil, e } - if logID != "" { - if _, present := resultMap["log_id"]; !present { - resultMap["log_id"] = logID - } - } out, _ := resultMap["data"].(map[string]interface{}) - if apiErr := errclass.BuildAPIError(resultMap, cc); apiErr != nil { - return out, apiErr - } - if resp.StatusCode >= 400 { - return out, httpStatusError(resp.StatusCode, resp.RawBody, logID) + if classified != nil { + return out, classified } return out, nil } -// httpStatusError classifies an HTTP error status whose body is not a usable -// API envelope: 5xx → retryable network/server_error, 404 → not_found, other -// 4xx → api error. The x-tt-logid (when present) is attached for diagnosis. -func httpStatusError(status int, rawBody []byte, logID string) error { - body := TruncateStr(strings.TrimSpace(string(rawBody)), 500) - if status >= 500 { - e := errs.NewNetworkError(errs.SubtypeNetworkServer, "HTTP %d: %s", status, body).WithCode(status).WithRetryable() - if logID != "" { - e = e.WithLogID(logID) - } - return e +func responseBodyLogID(result map[string]interface{}) string { + if logID, _ := result["log_id"].(string); strings.TrimSpace(logID) != "" { + return logID } - subtype := errs.SubtypeUnknown - if status == http.StatusNotFound { - subtype = errs.SubtypeNotFound - } - e := errs.NewAPIError(subtype, "HTTP %d: %s", status, body).WithCode(status) - if logID != "" { - e = e.WithLogID(logID) + if errBlock, ok := result["error"].(map[string]interface{}); ok { + if logID, _ := errBlock["log_id"].(string); strings.TrimSpace(logID) != "" { + return logID + } } - return e + return "" } // typedOrInternal passes an already-typed errs.* error through unchanged and diff --git a/shortcuts/common/runner_botinfo_test.go b/shortcuts/common/runner_botinfo_test.go index f8d77f747d..1020098c31 100644 --- a/shortcuts/common/runner_botinfo_test.go +++ b/shortcuts/common/runner_botinfo_test.go @@ -5,11 +5,14 @@ package common import ( "context" + "errors" + "net/http" "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" @@ -171,9 +174,50 @@ func TestFetchBotInfo_APICodeNonZero(t *testing.T) { if err == nil { t.Fatal("expected error for non-zero code") } - // fetchBotInfo returns a raw fmt.Errorf, not a typed envelope — message-substring assertion is intentional. - if !strings.Contains(err.Error(), "[99991]") { - t.Errorf("error = %q, want substring [99991]", err.Error()) + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryAPI || problem.Subtype == "" || problem.Code != 99991 { + t.Fatalf("problem = %#v, %v; want typed code 99991", problem, ok) + } +} + +func TestFetchBotInfo_RateLimitRecoveryMetadata(t *testing.T) { + for _, tt := range []struct { + name string + status int + body map[string]interface{} + wantCode int + }{ + {name: "HTTP 429 code zero", status: http.StatusTooManyRequests, body: map[string]interface{}{"code": 0, "msg": "slow"}, wantCode: 429}, + {name: "business rate limit", status: http.StatusOK, body: map[string]interface{}{"code": 99991400, "msg": "slow"}, wantCode: 99991400}, + } { + t.Run(tt.name, func(t *testing.T) { + f, _, _, reg := cmdutil.TestFactory(t, botInfoTestConfig(t)) + reg.Register(&httpmock.Stub{ + Method: "GET", + URL: "/open-apis/bot/v3/info", + Status: tt.status, + Headers: http.Header{"Content-Type": []string{"application/json"}, "Retry-After": []string{"19"}}, + Body: tt.body, + }) + + var info *BotInfo + var err error + runBotInfoShortcut(t, f, &info, &err) + var apiErr *errs.APIError + if !errors.As(err, &apiErr) { + t.Fatalf("fetchBotInfo error = %T (%v), want *errs.APIError", err, err) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryAPI || problem.Subtype != errs.SubtypeRateLimit { + t.Fatalf("problem = %#v, want api/rate_limit", problem) + } + if apiErr.Code != tt.wantCode || apiErr.Subtype != errs.SubtypeRateLimit { + t.Fatalf("rate limit problem = %#v, want code %d", apiErr.Problem, tt.wantCode) + } + if apiErr.RetryAfterSeconds == nil || *apiErr.RetryAfterSeconds != 19 { + t.Fatalf("RetryAfterSeconds = %v, want 19", apiErr.RetryAfterSeconds) + } + }) } } @@ -220,9 +264,9 @@ func TestFetchBotInfo_HTTP4xx(t *testing.T) { if err == nil { t.Fatal("expected error for HTTP 403") } - // fetchBotInfo returns a raw fmt.Errorf, not a typed envelope — message-substring assertion is intentional. - if !strings.Contains(err.Error(), "403") { - t.Errorf("error = %q, want substring '403'", err.Error()) + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryAPI || problem.Subtype == "" || problem.Code != 403 { + t.Fatalf("problem = %#v, %v; want typed code 403", problem, ok) } } diff --git a/shortcuts/contact/contact_get_user.go b/shortcuts/contact/contact_get_user.go index 16747f112c..39b4b4bedf 100644 --- a/shortcuts/contact/contact_get_user.go +++ b/shortcuts/contact/contact_get_user.go @@ -9,6 +9,7 @@ import ( "io" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/output" "github.com/larksuite/cli/shortcuts/common" ) @@ -28,8 +29,7 @@ var ContactGetUser = common.Shortcut{ }, Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { if runtime.Str("user-id") == "" && runtime.IsBot() { - return common.ValidationErrorf("bot identity cannot get current user info, specify --user-id"). - WithParam("--user-id") + return errs.NewValidationError(errs.SubtypeIdentityNotSupported, "bot identity cannot get current user info, specify --user-id") } return nil }, diff --git a/shortcuts/contact/contact_get_user_test.go b/shortcuts/contact/contact_get_user_test.go index a669492021..271162c1f8 100644 --- a/shortcuts/contact/contact_get_user_test.go +++ b/shortcuts/contact/contact_get_user_test.go @@ -24,8 +24,15 @@ func TestGetUser_BotCurrentUserValidationTyped(t *testing.T) { 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) + if validation.Param != "" { + t.Fatalf("param: got %q, want empty for omitted --user-id", validation.Param) + } + problem, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected typed problem, got %T: %v", err, err) + } + if problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeIdentityNotSupported { + t.Fatalf("problem = %s/%s, want validation/identity_not_supported", problem.Category, problem.Subtype) } } diff --git a/shortcuts/drive/drive_member_add.go b/shortcuts/drive/drive_member_add.go index 8f3540e9a6..4601432471 100644 --- a/shortcuts/drive/drive_member_add.go +++ b/shortcuts/drive/drive_member_add.go @@ -238,10 +238,10 @@ func readDriveMemberAddSpec(runtime *common.RuntimeContext) (driveMemberAddSpec, NotificationSet: runtime.Changed("need-notification"), } if runtime.As().IsBot() && spec.NotificationSet { - return driveMemberAddSpec{}, errs.NewValidationError(errs.SubtypeInvalidArgument, "--need-notification is only valid with --as user; omit it when using --as bot").WithParam("--need-notification") + return driveMemberAddSpec{}, errs.NewValidationError(errs.SubtypeIdentityNotSupported, "--need-notification is only valid with --as user; omit it when using --as bot").WithParam("--need-notification") } if runtime.As().IsBot() && spec.MemberType == "opendepartmentid" { - return driveMemberAddSpec{}, errs.NewValidationError(errs.SubtypeInvalidArgument, "--member-type=opendepartmentid requires --as user; bot identity does not support adding department collaborators").WithParam("--member-type") + return driveMemberAddSpec{}, errs.NewValidationError(errs.SubtypeIdentityNotSupported, "--member-type=opendepartmentid requires --as user; bot identity does not support adding department collaborators").WithParam("--member-type") } return spec, nil } diff --git a/shortcuts/drive/drive_member_add_test.go b/shortcuts/drive/drive_member_add_test.go index 007667e542..b8449d6005 100644 --- a/shortcuts/drive/drive_member_add_test.go +++ b/shortcuts/drive/drive_member_add_test.go @@ -552,6 +552,7 @@ func TestDriveMemberAdd_RejectsNotificationWithBot(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "--need-notification is only valid with --as user") { t.Fatalf("expected bot notification validation error, got: %v", err) } + assertDriveMemberIdentityProblem(t, err, "--need-notification") } func TestDriveMemberAdd_RejectsDepartmentWithBot(t *testing.T) { @@ -571,6 +572,19 @@ func TestDriveMemberAdd_RejectsDepartmentWithBot(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "--member-type=opendepartmentid requires --as user") { t.Fatalf("expected bot+opendepartmentid validation error, got: %v", err) } + assertDriveMemberIdentityProblem(t, err, "--member-type") +} + +func assertDriveMemberIdentityProblem(t *testing.T, err error, wantParam string) { + t.Helper() + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeIdentityNotSupported { + t.Fatalf("problem = %#v, want validation/identity_not_supported", problem) + } + var validationErr *errs.ValidationError + if !errors.As(err, &validationErr) || validationErr.Param != wantParam { + t.Fatalf("validation param = %q, want %q", validationErr.Param, wantParam) + } } func TestDriveMemberAdd_AcceptsAmbiguousIDWithExplicitType(t *testing.T) { diff --git a/shortcuts/im/builders_test.go b/shortcuts/im/builders_test.go index 3b32d736d5..da252db4bb 100644 --- a/shortcuts/im/builders_test.go +++ b/shortcuts/im/builders_test.go @@ -6,10 +6,12 @@ package im import ( "context" "encoding/json" + "errors" "reflect" "strings" "testing" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/shortcuts/common" "github.com/spf13/cobra" @@ -664,6 +666,27 @@ func TestShortcutValidateBranches(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "requires user identity") { t.Fatalf("ImChatMessageList.Validate() error = %v, want requires user identity", err) } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeIdentityNotSupported { + t.Fatalf("problem = %#v, want validation/identity_not_supported", problem) + } + var validationErr *errs.ValidationError + if !errors.As(err, &validationErr) || validationErr.Param != "--user-id" { + t.Fatalf("validation param = %q, want --user-id", validationErr.Param) + } + }) + + t.Run("ImChatCreate rejects bot-manager mode for user identity", func(t *testing.T) { + runtime := newTestRuntimeContext(t, map[string]string{}, map[string]bool{"set-bot-manager": true}) + err := ImChatCreate.Validate(context.Background(), runtime) + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeIdentityNotSupported { + t.Fatalf("problem = %#v, want validation/identity_not_supported", problem) + } + var validationErr *errs.ValidationError + if !errors.As(err, &validationErr) || validationErr.Param != "--set-bot-manager" { + t.Fatalf("validation param = %q, want --set-bot-manager", validationErr.Param) + } }) t.Run("ImMessagesMGet empty ids", func(t *testing.T) { diff --git a/shortcuts/im/coverage_additional_test.go b/shortcuts/im/coverage_additional_test.go index 26307b6cc5..75accb5bde 100644 --- a/shortcuts/im/coverage_additional_test.go +++ b/shortcuts/im/coverage_additional_test.go @@ -6,6 +6,7 @@ package im import ( "bytes" "context" + "errors" "fmt" "io" "net/http" @@ -17,6 +18,7 @@ import ( larkcore "github.com/larksuite/oapi-sdk-go/v3/core" "github.com/spf13/cobra" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/cmdutil" ) @@ -325,6 +327,18 @@ func TestResolveChatIDForMessagesList(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "requires user identity") { t.Fatalf("resolveChatIDForMessagesList() error = %v, want requires user identity", err) } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeIdentityNotSupported { + t.Fatalf("problem = %#v, want validation/identity_not_supported", problem) + } + var validationErr *errs.ValidationError + if !errors.As(err, &validationErr) || validationErr.Param != "--user-id" { + gotParam := "" + if validationErr != nil { + gotParam = validationErr.Param + } + t.Fatalf("validation param = %q, want --user-id", gotParam) + } }) } diff --git a/shortcuts/im/helpers.go b/shortcuts/im/helpers.go index 920dcb2094..2691be5d82 100644 --- a/shortcuts/im/helpers.go +++ b/shortcuts/im/helpers.go @@ -423,7 +423,7 @@ func mediaFallbackOrError(originalValue, mediaType string, uploadErr error) (str // resolveP2PChatID resolves user open_id to P2P chat_id. func resolveP2PChatID(runtime *common.RuntimeContext, openID string) (string, error) { if runtime.IsBot() { - return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "--user-id requires user identity (--as user); use --chat-id when calling with bot identity").WithParam("--user-id") + return "", errs.NewValidationError(errs.SubtypeIdentityNotSupported, "--user-id requires user identity (--as user); use --chat-id when calling with bot identity").WithParam("--user-id") } apiResp, err := runtime.DoAPI(&larkcore.ApiReq{ HttpMethod: http.MethodPost, diff --git a/shortcuts/im/im_chat_create.go b/shortcuts/im/im_chat_create.go index 0f8a35431e..a8eb7dd626 100644 --- a/shortcuts/im/im_chat_create.go +++ b/shortcuts/im/im_chat_create.go @@ -53,7 +53,7 @@ var ImChatCreate = common.Shortcut{ }, Validate: func(ctx context.Context, runtime *common.RuntimeContext) error { if runtime.Bool("set-bot-manager") && !runtime.IsBot() { - return errs.NewValidationError(errs.SubtypeInvalidArgument, "--set-bot-manager is only supported with bot identity (--as bot)").WithParam("--set-bot-manager") + return errs.NewValidationError(errs.SubtypeIdentityNotSupported, "--set-bot-manager is only supported with bot identity (--as bot)").WithParam("--set-bot-manager") } name := runtime.Str("name") diff --git a/shortcuts/im/im_chat_list.go b/shortcuts/im/im_chat_list.go index f4a07c66e9..53bb7efec5 100644 --- a/shortcuts/im/im_chat_list.go +++ b/shortcuts/im/im_chat_list.go @@ -94,7 +94,7 @@ var ImChatList = common.Shortcut{ return err } if len(parts) == 1 && parts[0] == "p2p" && runtime.IsBot() { - return errs.NewValidationError(errs.SubtypeInvalidArgument, + return errs.NewValidationError(errs.SubtypeIdentityNotSupported, `--types=p2p (single chats) is only supported with user identity (--as user). To protect user privacy, bot identity cannot list p2p chats. Use --as user, or include "group" in --types.`).WithParam("--types") } return nil diff --git a/shortcuts/im/im_chat_list_test.go b/shortcuts/im/im_chat_list_test.go index 8a2996b220..806b330f3e 100644 --- a/shortcuts/im/im_chat_list_test.go +++ b/shortcuts/im/im_chat_list_test.go @@ -7,12 +7,14 @@ import ( "bytes" "context" "encoding/json" + "errors" "io" "net/http" "strconv" "strings" "testing" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/shortcuts/common" @@ -295,6 +297,16 @@ func TestImChatList_Validate_Types(t *testing.T) { if !strings.Contains(err.Error(), c.wantErr) { t.Fatalf("Validate() err = %v; want substring %q", err, c.wantErr) } + if c.name == "bot single p2p rejected" { + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeIdentityNotSupported { + t.Fatalf("problem = %#v, want validation/identity_not_supported", problem) + } + var validationErr *errs.ValidationError + if !errors.As(err, &validationErr) || validationErr.Param != "--types" { + t.Fatalf("validation param = %q, want --types", validationErr.Param) + } + } }) } } diff --git a/shortcuts/im/im_chat_messages_list.go b/shortcuts/im/im_chat_messages_list.go index 8e3114216f..361ba7f501 100644 --- a/shortcuts/im/im_chat_messages_list.go +++ b/shortcuts/im/im_chat_messages_list.go @@ -80,7 +80,7 @@ var ImChatMessageList = common.Shortcut{ // Under bot identity, --user-id is not supported; require --chat-id only. if runtime.IsBot() { if runtime.Str("user-id") != "" { - return errs.NewValidationError(errs.SubtypeInvalidArgument, "--user-id requires user identity (--as user); use --chat-id when calling with bot identity").WithParam("--user-id") + return errs.NewValidationError(errs.SubtypeIdentityNotSupported, "--user-id requires user identity (--as user); use --chat-id when calling with bot identity").WithParam("--user-id") } if runtime.Str("chat-id") == "" { return errs.NewValidationError(errs.SubtypeInvalidArgument, "specify --chat-id (bot identity does not support --user-id)").WithParam("--chat-id") diff --git a/shortcuts/mail/helpers.go b/shortcuts/mail/helpers.go index e5a1dafeff..00b9eba38a 100644 --- a/shortcuts/mail/helpers.go +++ b/shortcuts/mail/helpers.go @@ -2654,13 +2654,35 @@ func buildCalendarBody(runtime *common.RuntimeContext, senderEmail string, toAdd // bot uses tenant access token; "me" cannot be resolved to a user mailbox under TAT. func validateBotMailboxNotMe(runtime *common.RuntimeContext) error { if runtime.IsBot() && runtime.Str("mailbox") == "me" { - return mailValidationParamError("--mailbox", + identityErr := errs.NewValidationError(errs.SubtypeIdentityNotSupported, "--as bot does not support --mailbox me: bot identity uses a tenant token and cannot resolve \"me\" to a user mailbox; "+ "pass an explicit email address, e.g. --mailbox alice@example.com") + if runtime.Changed("mailbox") { + return identityErr.WithParam("--mailbox") + } + return identityErr } return nil } +func mailUserIdentityRequiredError(runtime *common.RuntimeContext, format string, args ...any) *errs.ValidationError { + if runtime != nil && runtime.IsBot() { + return errs.NewValidationError(errs.SubtypeIdentityNotSupported, format, args...). + WithHint("use --as user only when user authorization is configured") + } + return mailFailedPreconditionError(format, args...) +} + +func mailRequireUserOpenID(runtime *common.RuntimeContext, format string, args ...any) (string, error) { + if runtime == nil || runtime.IsBot() { + return "", mailUserIdentityRequiredError(runtime, format, args...) + } + if runtime.Config == nil || runtime.UserOpenId() == "" { + return "", mailFailedPreconditionError(format, args...) + } + return runtime.UserOpenId(), nil +} + // validateMessageIDs parses and validates the existing +messages comma-separated // flag format. Unlike splitByComma, it keeps empty entries so "id1,,id2" fails // locally. It intentionally does not enforce the server-side single-call limit: diff --git a/shortcuts/mail/large_attachment.go b/shortcuts/mail/large_attachment.go index 97536fa6a2..0d5288316b 100644 --- a/shortcuts/mail/large_attachment.go +++ b/shortcuts/mail/large_attachment.go @@ -143,9 +143,9 @@ func uploadLargeAttachments(ctx context.Context, runtime *common.RuntimeContext, if len(files) == 0 { return nil, nil } - userOpenId := runtime.UserOpenId() - if userOpenId == "" { - return nil, mailFailedPreconditionError("large attachment upload requires user identity (user open_id not available)") + userOpenId, err := mailRequireUserOpenID(runtime, "large attachment upload requires user identity (user open_id not available)") + if err != nil { + return nil, err } results := make([]largeAttachmentResult, 0, len(files)) @@ -427,14 +427,14 @@ func processLargeAttachments( "empty messages cannot include the download link") } - if runtime.Config == nil || runtime.UserOpenId() == "" { - var totalBytes int64 - for _, f := range files { - totalBytes += f.Size - } - return bld, mailFailedPreconditionError("total attachment size %.1f MB exceeds the 25 MB EML limit; "+ - "large attachment upload requires user identity (--as user)", - float64(totalBytes)/1024/1024) + var totalBytes int64 + for _, f := range files { + totalBytes += f.Size + } + if _, err := mailRequireUserOpenID(runtime, "total attachment size %.1f MB exceeds the 25 MB EML limit; "+ + "large attachment upload requires user identity (--as user)", + float64(totalBytes)/1024/1024); err != nil { + return bld, err } results, err := uploadLargeAttachments(ctx, runtime, classified.Oversized) @@ -612,14 +612,14 @@ func preprocessLargeAttachmentsForDraftEdit( } // Guard: need user identity for upload. - if runtime.Config == nil || runtime.UserOpenId() == "" { - var totalBytes int64 - for _, f := range files { - totalBytes += f.Size - } - return patch, mailFailedPreconditionError("total attachment size %.1f MB exceeds the 25 MB EML limit; "+ - "large attachment upload requires user identity (--as user)", - float64(totalBytes)/1024/1024) + var totalBytes int64 + for _, f := range files { + totalBytes += f.Size + } + if _, err := mailRequireUserOpenID(runtime, "total attachment size %.1f MB exceeds the 25 MB EML limit; "+ + "large attachment upload requires user identity (--as user)", + float64(totalBytes)/1024/1024); err != nil { + return patch, err } // Upload oversized files. diff --git a/shortcuts/mail/large_attachment_test.go b/shortcuts/mail/large_attachment_test.go index a0aa34ada7..f6a5821059 100644 --- a/shortcuts/mail/large_attachment_test.go +++ b/shortcuts/mail/large_attachment_test.go @@ -12,6 +12,7 @@ import ( "github.com/spf13/cobra" + "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/vfs/localfileio" "github.com/larksuite/cli/shortcuts/common" @@ -1086,6 +1087,17 @@ func TestProcessLargeAttachments_OversizedNoIdentity(t *testing.T) { } } +func TestProcessLargeAttachments_OversizedBotIdentity(t *testing.T) { + chdirTemp(t) + if err := os.WriteFile("huge.zip", make([]byte, 100), 0o644); err != nil { + t.Fatal(err) + } + rt := common.TestNewRuntimeContextWithIdentity(&cobra.Command{}, &core.CliConfig{UserOpenId: "ou_stale_user"}, core.AsBot) + bld := emlbuilder.New().WithFileIO(rt.FileIO()) + _, err := processLargeAttachments(nil, rt, bld, "

body

", "", []string{"huge.zip"}, emlbuilder.MaxEMLSize, 0) + assertMailIdentityProblem(t, err, "") +} + func TestPreprocessLargeAttachments_NoAddAttachmentOps(t *testing.T) { rt := common.TestNewRuntimeContext(&cobra.Command{}, nil) snapshot := &draftpkg.DraftSnapshot{ @@ -1187,6 +1199,52 @@ func TestPreprocessLargeAttachments_OversizedNoIdentity(t *testing.T) { } } +func TestPreprocessLargeAttachments_OversizedBotWithConfiguredUserOpenID(t *testing.T) { + chdirTemp(t) + if err := os.WriteFile("big.zip", make([]byte, 100), 0o644); err != nil { + t.Fatal(err) + } + rt := common.TestNewRuntimeContextWithIdentity(&cobra.Command{}, &core.CliConfig{UserOpenId: "ou_stale_user"}, core.AsBot) + snapshot := &draftpkg.DraftSnapshot{Body: &draftpkg.Part{ + MediaType: "multipart/mixed", + Children: []*draftpkg.Part{ + {MediaType: "text/html", Body: make([]byte, emlbuilder.MaxEMLSize)}, + }, + }} + patch := draftpkg.Patch{Ops: []draftpkg.PatchOp{{Op: "add_attachment", Path: "big.zip"}}} + _, err := preprocessLargeAttachmentsForDraftEdit(nil, rt, snapshot, patch) + assertMailIdentityProblem(t, err, "") +} + +func TestUploadLargeAttachments_BotIdentity(t *testing.T) { + rt := common.TestNewRuntimeContextWithIdentity(&cobra.Command{}, &core.CliConfig{UserOpenId: "ou_stale_user"}, core.AsBot) + _, err := uploadLargeAttachments(nil, rt, []attachmentFile{{FileName: "huge.zip", Size: 100}}) + assertMailIdentityProblem(t, err, "") +} + +func TestMailRequireUserOpenIDChecksIdentityBeforeConfiguredUser(t *testing.T) { + botRuntime := common.TestNewRuntimeContextWithIdentity(&cobra.Command{}, &core.CliConfig{UserOpenId: "ou_stale_user"}, core.AsBot) + _, err := mailRequireUserOpenID(botRuntime, "user identity required") + assertMailIdentityProblem(t, err, "") + problem, _ := errs.ProblemOf(err) + if problem.Hint != "use --as user only when user authorization is configured" { + t.Fatalf("hint = %q, want user-authorization recovery hint", problem.Hint) + } + + userRuntime := common.TestNewRuntimeContextWithIdentity(&cobra.Command{}, &core.CliConfig{}, core.AsUser) + _, err = mailRequireUserOpenID(userRuntime, "user identity required") + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeFailedPrecondition { + t.Fatalf("problem = %#v, want validation/failed_precondition", problem) + } + + userRuntime = common.TestNewRuntimeContextWithIdentity(&cobra.Command{}, &core.CliConfig{UserOpenId: "ou_user"}, core.AsUser) + openID, err := mailRequireUserOpenID(userRuntime, "user identity required") + if err != nil || openID != "ou_user" { + t.Fatalf("mailRequireUserOpenID() = %q, %v; want ou_user, nil", openID, err) + } +} + func TestPreprocessLargeAttachments_NormalizesHeaders(t *testing.T) { rt := common.TestNewRuntimeContext(&cobra.Command{}, nil) serverVal := encodeServerHeader([]map[string]interface{}{ diff --git a/shortcuts/mail/mail_forward.go b/shortcuts/mail/mail_forward.go index 20a14577ae..88211e180a 100644 --- a/shortcuts/mail/mail_forward.go +++ b/shortcuts/mail/mail_forward.go @@ -432,14 +432,14 @@ var MailForward = common.Shortcut{ return mailFailedPreconditionError("large attachments require a body; " + "empty messages cannot include the download link") } - if runtime.Config == nil || runtime.UserOpenId() == "" { - var totalBytes int64 - for _, f := range classified.Oversized { - totalBytes += f.Size - } - return mailFailedPreconditionError("total attachment size %.1f MB exceeds the 25 MB EML limit; "+ - "large attachment upload requires user identity (--as user)", - float64(totalBytes)/1024/1024) + var totalBytes int64 + for _, f := range classified.Oversized { + totalBytes += f.Size + } + if _, err := mailRequireUserOpenID(runtime, "total attachment size %.1f MB exceeds the 25 MB EML limit; "+ + "large attachment upload requires user identity (--as user)", + float64(totalBytes)/1024/1024); err != nil { + return err } var allOversized []attachmentFile diff --git a/shortcuts/mail/mail_shortcut_validation_test.go b/shortcuts/mail/mail_shortcut_validation_test.go index 2eb56c0cdc..353fe4690a 100644 --- a/shortcuts/mail/mail_shortcut_validation_test.go +++ b/shortcuts/mail/mail_shortcut_validation_test.go @@ -5,6 +5,7 @@ package mail import ( "encoding/base64" + "errors" "os" "strings" "testing" @@ -35,6 +36,18 @@ func assertValidationError(t *testing.T, err error, wantSubstr string) { } } +func assertMailIdentityProblem(t *testing.T, err error, wantParam string) { + t.Helper() + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeIdentityNotSupported { + t.Fatalf("problem = %#v, want validation/identity_not_supported", problem) + } + var validationErr *errs.ValidationError + if !errors.As(err, &validationErr) || validationErr.Param != wantParam { + t.Fatalf("validation param = %q, want %q", validationErr.Param, wantParam) + } +} + // assertValidatePasses fails the test if err is a validation error; other // errors (e.g. API call failures from missing tokens) are acceptable because // we only care that the Validate callback passed. @@ -107,6 +120,7 @@ func TestMailMessageBotMailboxMeReturnsValidationError(t *testing.T) { "+message", "--as", "bot", "--mailbox", "me", "--message-id", "msg_xxx", }, f, stdout) assertValidationError(t, err, "does not support --mailbox me") + assertMailIdentityProblem(t, err, "--mailbox") } // TC-2: +message --as bot --mailbox explicit → Validate passes @@ -134,6 +148,7 @@ func TestMailMessagesBotDefaultMailboxMeReturnsValidationError(t *testing.T) { "+messages", "--as", "bot", "--message-ids", validMessageIDForTest("biz-x"), }, f, stdout) assertValidationError(t, err, "does not support --mailbox me") + assertMailIdentityProblem(t, err, "") } // TC-5: +messages --as bot --mailbox explicit → Validate passes diff --git a/shortcuts/mail/template_compose.go b/shortcuts/mail/template_compose.go index f1193ffd2a..1198b450bd 100644 --- a/shortcuts/mail/template_compose.go +++ b/shortcuts/mail/template_compose.go @@ -287,9 +287,9 @@ func uploadToDriveForTemplate(ctx context.Context, runtime *common.RuntimeContex if err := filecheck.CheckBlockedExtension(name); err != nil { return "", size, mailValidationError("%v", err).WithCause(err) } - userOpenId := runtime.UserOpenId() - if userOpenId == "" { - return "", size, mailFailedPreconditionError("template attachment upload requires user identity (--as user)") + userOpenId, err := mailRequireUserOpenID(runtime, "template attachment upload requires user identity (--as user)") + if err != nil { + return "", size, err } if size <= common.MaxDriveMediaUploadSinglePartSize { fileKey, err = common.UploadDriveMediaAllTyped(runtime, common.DriveMediaUploadAllConfig{ diff --git a/shortcuts/mail/template_compose_test.go b/shortcuts/mail/template_compose_test.go index 1b70afdd7b..95cc41b1d8 100644 --- a/shortcuts/mail/template_compose_test.go +++ b/shortcuts/mail/template_compose_test.go @@ -4,10 +4,16 @@ package mail import ( + "context" "encoding/base64" "encoding/json" "strings" "testing" + + "github.com/larksuite/cli/internal/core" + "github.com/larksuite/cli/internal/vfs" + "github.com/larksuite/cli/shortcuts/common" + "github.com/spf13/cobra" ) // --------------------------------------------------------------------------- @@ -336,6 +342,16 @@ func TestEncodeTemplateLargeAttachmentHeader(t *testing.T) { } } +func TestUploadToDriveForTemplate_BotIdentity(t *testing.T) { + chdirTemp(t) + if err := vfs.WriteFile("attachment.txt", []byte("hello"), 0o644); err != nil { + t.Fatal(err) + } + runtime := common.TestNewRuntimeContextWithIdentity(&cobra.Command{}, &core.CliConfig{UserOpenId: "ou_stale_user"}, core.AsBot) + _, _, err := uploadToDriveForTemplate(context.Background(), runtime, "attachment.txt") + assertMailIdentityProblem(t, err, "") +} + // --------------------------------------------------------------------------- // extractTemplatePayload // --------------------------------------------------------------------------- diff --git a/shortcuts/wiki/wiki_list_copy_test.go b/shortcuts/wiki/wiki_list_copy_test.go index 3d0cf983f5..4338c006c7 100644 --- a/shortcuts/wiki/wiki_list_copy_test.go +++ b/shortcuts/wiki/wiki_list_copy_test.go @@ -424,6 +424,7 @@ func TestWikiNodeListRejectsMyLibraryForBot(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "bot identity does not support --space-id my_library") { t.Fatalf("expected my_library bot rejection, got %v", err) } + requireWikiIdentityProblem(t, err, "--space-id") } func TestWikiNodeListResolvesMyLibraryForUser(t *testing.T) { diff --git a/shortcuts/wiki/wiki_member_add.go b/shortcuts/wiki/wiki_member_add.go index a7fc86394c..823f4c7a80 100644 --- a/shortcuts/wiki/wiki_member_add.go +++ b/shortcuts/wiki/wiki_member_add.go @@ -138,7 +138,7 @@ func readWikiMemberAddSpec(runtime *common.RuntimeContext) (wikiMemberAddSpec, e // not waste a network round-trip on a server-side 403. The escape hatch is // --as user, which is the only identity the API accepts for departments. if runtime.As().IsBot() && spec.MemberType == "opendepartmentid" { - return wikiMemberAddSpec{}, errs.NewValidationError(errs.SubtypeInvalidArgument, + return wikiMemberAddSpec{}, errs.NewValidationError(errs.SubtypeIdentityNotSupported, "--as bot does not support --member-type opendepartmentid; rerun with --as user", ).WithParam("--member-type") } diff --git a/shortcuts/wiki/wiki_member_helpers.go b/shortcuts/wiki/wiki_member_helpers.go index 8fd1e41289..bdb71d09cc 100644 --- a/shortcuts/wiki/wiki_member_helpers.go +++ b/shortcuts/wiki/wiki_member_helpers.go @@ -30,7 +30,7 @@ func validateWikiMemberSpaceID(runtime *common.RuntimeContext, spaceID string) e return errs.NewValidationError(errs.SubtypeInvalidArgument, "--space-id is required and cannot be blank").WithParam("--space-id") } if runtime.As().IsBot() && spaceID == wikiMyLibrarySpaceID { - return errs.NewValidationError(errs.SubtypeInvalidArgument, "bot identity does not support --space-id my_library; use an explicit --space-id").WithParam("--space-id") + return errs.NewValidationError(errs.SubtypeIdentityNotSupported, "bot identity does not support --space-id my_library; use an explicit --space-id").WithParam("--space-id") } return validateOptionalResourceName(spaceID, "--space-id") } diff --git a/shortcuts/wiki/wiki_member_test.go b/shortcuts/wiki/wiki_member_test.go index 63626ca276..ee68850f02 100644 --- a/shortcuts/wiki/wiki_member_test.go +++ b/shortcuts/wiki/wiki_member_test.go @@ -13,6 +13,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" @@ -214,6 +215,7 @@ func TestWikiMemberAddRejectsMyLibraryForBot(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "bot identity does not support --space-id my_library") { t.Fatalf("expected my_library bot rejection, got %v", err) } + requireWikiIdentityProblem(t, err, "--space-id") } func TestWikiMemberAddRejectsBotWithDepartment(t *testing.T) { @@ -231,6 +233,10 @@ func TestWikiMemberAddRejectsBotWithDepartment(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "--as bot does not support --member-type opendepartmentid") { t.Fatalf("expected bot+opendepartmentid rejection, got %v", err) } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeIdentityNotSupported { + t.Fatalf("problem = %#v, want validation/identity_not_supported", problem) + } } func TestWikiMemberAddAcceptsAppIDWithoutFormatValidation(t *testing.T) { @@ -507,6 +513,7 @@ func TestWikiMemberListRejectsMyLibraryForBot(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "bot identity does not support --space-id my_library") { t.Fatalf("expected my_library bot rejection, got %v", err) } + requireWikiIdentityProblem(t, err, "--space-id") } func TestWikiMemberListReturnsMembers(t *testing.T) { diff --git a/shortcuts/wiki/wiki_move.go b/shortcuts/wiki/wiki_move.go index 8ee798bcc5..52c812ce82 100644 --- a/shortcuts/wiki/wiki_move.go +++ b/shortcuts/wiki/wiki_move.go @@ -64,7 +64,7 @@ var WikiMove = common.Shortcut{ // for a tenant_access_token (--as bot), so reject early with a clear // hint instead of letting the API return a confusing error. if runtime.As().IsBot() && spec.TargetSpaceID == wikiMyLibrarySpaceID { - return errs.NewValidationError(errs.SubtypeInvalidArgument, "--target-space-id my_library is a per-user personal library alias and cannot be used with --as bot; resolve it to a real space_id first via `lark-cli wiki spaces get --params '{\"space_id\":\"my_library\"}' --as user`").WithParam("--target-space-id") + return errs.NewValidationError(errs.SubtypeIdentityNotSupported, "--target-space-id my_library is a per-user personal library alias and cannot be used with --as bot; resolve it to a real space_id first via `lark-cli wiki spaces get --params '{\"space_id\":\"my_library\"}' --as user`").WithParam("--target-space-id") } return validateWikiMoveSpec(spec) }, diff --git a/shortcuts/wiki/wiki_move_test.go b/shortcuts/wiki/wiki_move_test.go index 6d9a7e64f9..22520d137a 100644 --- a/shortcuts/wiki/wiki_move_test.go +++ b/shortcuts/wiki/wiki_move_test.go @@ -361,6 +361,10 @@ func TestWikiMoveValidateRejectsBotMyLibrary(t *testing.T) { if !strings.Contains(err.Error(), "my_library") || !strings.Contains(err.Error(), "--as bot") { t.Fatalf("unexpected error: %v", err) } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeIdentityNotSupported { + t.Fatalf("problem = %#v, want validation/identity_not_supported", problem) + } } func TestWikiMoveValidateAllowsUserMyLibrary(t *testing.T) { diff --git a/shortcuts/wiki/wiki_node_create.go b/shortcuts/wiki/wiki_node_create.go index a03d7e2017..aadc72591e 100644 --- a/shortcuts/wiki/wiki_node_create.go +++ b/shortcuts/wiki/wiki_node_create.go @@ -241,7 +241,7 @@ func validateWikiNodeCreateSpec(spec wikiNodeCreateSpec, identity core.Identity) // my_library must be rejected explicitly instead of deferring to API-time // resolution errors. if identity.IsBot() && spec.SpaceID == wikiMyLibrarySpaceID { - return errs.NewValidationError(errs.SubtypeInvalidArgument, "bot identity does not support --space-id my_library; use an explicit --space-id or --parent-node-token").WithParam("--space-id") + return errs.NewValidationError(errs.SubtypeIdentityNotSupported, "bot identity does not support --space-id my_library; use an explicit --space-id or --parent-node-token").WithParam("--space-id") } // Bot identity also cannot fall back implicitly, so it requires an explicit // target or a parent it can resolve from. diff --git a/shortcuts/wiki/wiki_node_create_test.go b/shortcuts/wiki/wiki_node_create_test.go index a42ea0e697..0e271d9a83 100644 --- a/shortcuts/wiki/wiki_node_create_test.go +++ b/shortcuts/wiki/wiki_node_create_test.go @@ -217,6 +217,19 @@ func TestValidateWikiNodeCreateSpecRejectsBotMyLibrarySpaceID(t *testing.T) { if err == nil || !strings.Contains(err.Error(), "bot identity does not support --space-id my_library") { t.Fatalf("expected bot my_library validation error, got %v", err) } + requireWikiIdentityProblem(t, err, "--space-id") +} + +func requireWikiIdentityProblem(t *testing.T, err error, wantParam string) { + t.Helper() + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeIdentityNotSupported { + t.Fatalf("problem = %#v, want validation/identity_not_supported", problem) + } + var validationErr *errs.ValidationError + if !errors.As(err, &validationErr) || validationErr.Param != wantParam { + t.Fatalf("validation param = %q, want %q", validationErr.Param, wantParam) + } } func TestResolveWikiNodeCreateSpaceUsesParentNode(t *testing.T) { diff --git a/shortcuts/wiki/wiki_node_list.go b/shortcuts/wiki/wiki_node_list.go index 494ca0fc3f..a9c14bca00 100644 --- a/shortcuts/wiki/wiki_node_list.go +++ b/shortcuts/wiki/wiki_node_list.go @@ -135,7 +135,7 @@ func readWikiNodeListSpec(runtime *common.RuntimeContext) (wikiNodeListSpec, err // hint instead of deferring to API-time errors. Matches the contract // used by +node-create and +move. if runtime.As().IsBot() && spaceID == wikiMyLibrarySpaceID { - return wikiNodeListSpec{}, errs.NewValidationError(errs.SubtypeInvalidArgument, "bot identity does not support --space-id my_library; use an explicit numeric --space-id").WithParam("--space-id") + return wikiNodeListSpec{}, errs.NewValidationError(errs.SubtypeIdentityNotSupported, "bot identity does not support --space-id my_library; use an explicit numeric --space-id").WithParam("--space-id") } if err := validateWikiNodeListSpaceID(spaceID); err != nil { return wikiNodeListSpec{}, err diff --git a/tests/cli_e2e/wiki/wiki_identity_validation_dryrun_test.go b/tests/cli_e2e/wiki/wiki_identity_validation_dryrun_test.go new file mode 100644 index 0000000000..31f760226f --- /dev/null +++ b/tests/cli_e2e/wiki/wiki_identity_validation_dryrun_test.go @@ -0,0 +1,66 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package wiki + +import ( + "context" + "testing" + "time" + + clie2e "github.com/larksuite/cli/tests/cli_e2e" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/tidwall/gjson" +) + +func TestWikiIdentityValidationDryRun(t *testing.T) { + setWikiNodeCreateDryRunEnv(t) + + tests := []struct { + name string + args []string + }{ + { + name: "member add rejects bot department member", + args: []string{ + "wiki", "+member-add", + "--space-id", "space_42", + "--member-id", "od_department", + "--member-type", "opendepartmentid", + "--member-role", "member", + "--dry-run", + }, + }, + { + name: "move rejects bot personal library", + args: []string{ + "wiki", "+move", + "--obj-type", "docx", + "--obj-token", "doccn_test", + "--target-space-id", "my_library", + "--dry-run", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + t.Cleanup(cancel) + + result, err := clie2e.RunCmd(ctx, clie2e.Request{Args: tt.args, DefaultAs: "bot"}) + require.NoError(t, err) + result.AssertExitCode(t, 2) + assert.Equal(t, "identity_not_supported", wikiErrorSubtype(result), + "identity incompatibility must be machine-readable; stdout=%s stderr=%s", result.Stdout, result.Stderr) + }) + } +} + +func wikiErrorSubtype(result *clie2e.Result) string { + if subtype := gjson.Get(result.Stdout, "error.subtype").String(); subtype != "" { + return subtype + } + return gjson.Get(result.Stderr, "error.subtype").String() +}