Skip to content

Commit 05aeefc

Browse files
committed
refactor: retire legacy error envelopes and enforce typed contract
Consolidate all command error reporting onto the typed errs.* contract, remove the legacy error surface that predated it, and tighten the lint guards so the contract holds across the whole repository going forward. Every failure now reaches stderr as one envelope shape: a category, an optional subtype, a human- and agent-readable message, and a recovery hint, with invalid parameters listed under `params`. The legacy ExitError envelope, its constructors, and the boundary bridge that promoted untyped config and authorization errors are deleted, leaving a single path from error to wire. Predicate commands keep their silent-exit behavior through a dedicated signal that carries only an exit code. Infrastructure paths that still emitted ad-hoc envelopes — flag parsing, unknown commands and subcommands, plugin and policy guards, confirmation prompts, and auth/config failures — now classify into the same taxonomy. Business, API, auth, and config exit codes are preserved; the one behavioral change is that Cobra usage failures (missing required flag, unknown command, bad arguments) now emit the typed validation envelope and exit 2, matching the explicit flag and subcommand guards, instead of Cobra's plain-text exit 1. Enforcement is repo-wide rather than per-path: - The errscontract guards run by default everywhere instead of through a migration allowlist, so legacy envelopes cannot be reintroduced anywhere. - errorlint runs across the whole repository: every error wrap must use %w and every comparison must use errors.Is/errors.As, so interior wraps stay legal but can no longer break the chain the typed boundary relies on. - The errs-no-bare-wrap guard is keyed by structural prefix instead of an explicit per-domain allowlist, so new shortcut domains are covered without editing a list. It runs where forbidigo is enabled (the shortcut domains and the auth/config/service command groups); repo-wide chain integrity for the remaining command paths is carried by errorlint above.
1 parent 7eeb111 commit 05aeefc

143 files changed

Lines changed: 2933 additions & 3893 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.golangci.yml

Lines changed: 15 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,11 @@ linters:
2929
- unused # checks for unused constants, variables, functions and types
3030
- depguard # blocks forbidden package imports
3131
- forbidigo # forbids specific function calls
32+
- errorlint # enforces error wrapping (%w) and errors.Is/As over == and type asserts
3233

3334
# To enable later after fixing existing issues:
3435
# - errcheck # checks for unchecked errors
3536
# - errname # checks that error types are named XxxError
36-
# - errorlint # checks error wrapping best practices
3737
# - gosec # security-oriented linter
3838
# - misspell # finds commonly misspelled English words
3939
# - staticcheck # comprehensive static analysis
@@ -49,9 +49,16 @@ linters:
4949
- gocritic
5050
- depguard
5151
- forbidigo
52-
# Paths that run forbidigo. Add an entry when a path joins one of
53-
# the rules below.
52+
- errorlint # tests legitimately do identity (==) and concrete type-assert checks
53+
# forbidigo runs repo-wide (minus the boundaries below) so errs-no-bare-wrap
54+
# has no gap. The framework bans (os/vfs, raw HTTP, fmt.Print, filepath,
55+
# log) stay scoped to shortcuts/ + internal/ + config/auth/service via the
56+
# next rule; elsewhere only errs-no-bare-wrap fires.
57+
- path-except: (shortcuts/|internal/|cmd/|events/)
58+
linters:
59+
- forbidigo
5460
- path-except: (shortcuts/|internal/|cmd/auth/|cmd/config/|cmd/service/)
61+
text: (vfs|IOStreams|ctx\.Out|shortcuts-no-raw-http|filepath functions|os\.Exit|structured error return)
5562
linters:
5663
- forbidigo
5764
- path: internal/vfs/
@@ -71,25 +78,14 @@ linters:
7178
text: shortcuts-no-raw-http
7279
linters:
7380
- forbidigo
74-
# errs-typed-only enforced on paths already migrated to errs.NewXxxError.
75-
# Add a path when its migration is complete.
76-
- path-except: (internal/auth/|internal/errcompat/|internal/errclass/|internal/client/|internal/cmdutil/factory\.go|cmd/auth/|cmd/config/|cmd/service/|shortcuts/common/mcp_client\.go|shortcuts/apps/|shortcuts/base/|shortcuts/calendar/|shortcuts/contact/|shortcuts/doc/|shortcuts/drive/|shortcuts/im/|shortcuts/mail/|shortcuts/markdown/|shortcuts/minutes/|shortcuts/okr/|shortcuts/sheets/|shortcuts/slides/|shortcuts/task/|shortcuts/vc/|shortcuts/whiteboard/|shortcuts/wiki/|internal/event/consume/|cmd/event/|events/|shortcuts/event/)
77-
text: errs-typed-only
78-
linters:
79-
- forbidigo
80-
# errs-no-bare-wrap enforced on paths fully migrated to typed final
81-
# errors. Scoped separately from errs-typed-only because cmd/auth/,
82-
# cmd/config/ still have residual fmt.Errorf and must not be caught.
83-
- path-except: (shortcuts/apps/|shortcuts/base/|shortcuts/calendar/|shortcuts/contact/|shortcuts/doc/|shortcuts/drive/|shortcuts/im/|shortcuts/mail/|shortcuts/markdown/|shortcuts/minutes/|shortcuts/okr/|shortcuts/sheets/|shortcuts/slides/|shortcuts/task/|shortcuts/vc/|shortcuts/whiteboard/|shortcuts/wiki/|shortcuts/common/mcp_client\.go|cmd/event/|events/|shortcuts/event/)
81+
# errs-no-bare-wrap enforced across every command/wire boundary by
82+
# structural prefix, so any future business domain or command is covered
83+
# without editing an allowlist. Genuine intermediate wraps inside these
84+
# paths use //nolint:forbidigo with a reason.
85+
- path-except: (cmd/|shortcuts/|events/)
8486
text: errs-no-bare-wrap
8587
linters:
8688
- forbidigo
87-
# errs-no-legacy-helper enforced on domains whose shared validation/save
88-
# helpers have migrated to typed final errors.
89-
- path-except: (shortcuts/apps/|shortcuts/base/|shortcuts/calendar/|shortcuts/contact/|shortcuts/doc/|shortcuts/drive/|shortcuts/im/|shortcuts/mail/|shortcuts/markdown/|shortcuts/minutes/|shortcuts/okr/|shortcuts/sheets/|shortcuts/slides/|shortcuts/task/|shortcuts/vc/|shortcuts/whiteboard/|shortcuts/wiki/|cmd/event/|events/|shortcuts/event/)
90-
text: errs-no-legacy-helper
91-
linters:
92-
- forbidigo
9389

9490
settings:
9591
depguard:
@@ -108,22 +104,6 @@ linters:
108104
Use runtime.FileIO() for file operations or runtime.ValidatePath() for path validation.
109105
forbidigo:
110106
forbid:
111-
# ── legacy output.Err* helpers banned on migrated paths ──
112-
# output.ErrBare is intentionally not listed — it is the predicate-
113-
# command silent-exit signal, outside the typed envelope contract.
114-
- pattern: output\.(ErrValidation|ErrAuth|ErrNetwork|ErrAPI|ErrWithHint|Errorf)\b
115-
msg: >-
116-
[errs-typed-only] use errs.NewXxxError(...) builder
117-
(see errs/types.go).
118-
# ── legacy shared error helpers banned on migrated domains ──
119-
# These helpers emit legacy output.Err* / bare error shapes or drop
120-
# typed metadata such as Param/Cause. Migrated domains must use typed
121-
# common replacements or local typed helpers instead.
122-
- pattern: (common\.FlagErrorf|common\.RejectDangerousChars|common\.WrapInputStatError|common\.WrapSaveErrorByCategory)\b
123-
msg: >-
124-
[errs-no-legacy-helper] these shared helpers emit legacy or
125-
metadata-poor error shapes. Use typed common replacements, typed
126-
errs.NewXxxError builders, or domain-local typed helpers.
127107
# ── bare error wraps banned on fully-typed paths ──
128108
- pattern: (fmt\.Errorf|errors\.New)\b
129109
msg: >-

cmd/api/api.go

Lines changed: 32 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"regexp"
1111
"strings"
1212

13+
"github.com/larksuite/cli/errs"
1314
"github.com/larksuite/cli/internal/client"
1415
"github.com/larksuite/cli/internal/cmdutil"
1516
"github.com/larksuite/cli/internal/core"
@@ -123,7 +124,13 @@ func buildAPIRequest(opts *APIOptions) (client.RawApiRequest, *cmdutil.FileUploa
123124

124125
// stdin conflict: --params and --data cannot both read from stdin, regardless of --file.
125126
if opts.Params == "-" && opts.Data == "-" {
126-
return client.RawApiRequest{}, nil, output.ErrValidation("--params and --data cannot both read from stdin (-)")
127+
return client.RawApiRequest{}, nil, errs.NewValidationError(errs.SubtypeInvalidArgument,
128+
"--params and --data cannot both read from stdin (-)").
129+
WithHint("pass at most one flag as '-'; give the other inline JSON or @file").
130+
WithParams(
131+
errs.InvalidParam{Name: "--params", Reason: "reads from stdin (-)"},
132+
errs.InvalidParam{Name: "--data", Reason: "reads from stdin (-)"},
133+
)
127134
}
128135

129136
params, err := cmdutil.ParseJSONMap(opts.Params, "--params", stdin, fileIO)
@@ -153,7 +160,10 @@ func buildAPIRequest(opts *APIOptions) (client.RawApiRequest, *cmdutil.FileUploa
153160
return client.RawApiRequest{}, nil, err
154161
}
155162
if _, ok := dataFields.(map[string]any); !ok {
156-
return client.RawApiRequest{}, nil, output.ErrValidation("--data must be a JSON object when used with --file")
163+
return client.RawApiRequest{}, nil, errs.NewValidationError(errs.SubtypeInvalidArgument,
164+
"--data must be a JSON object when used with --file").
165+
WithHint(`with --file, --data carries multipart form fields, e.g. --data '{"image_type":"message"}'`).
166+
WithParam("--data")
157167
}
158168
}
159169

@@ -196,7 +206,13 @@ func apiRun(opts *APIOptions) error {
196206
}
197207

198208
if opts.PageAll && opts.Output != "" {
199-
return output.ErrValidation("--output and --page-all are mutually exclusive")
209+
return errs.NewValidationError(errs.SubtypeInvalidArgument,
210+
"--output and --page-all are mutually exclusive").
211+
WithHint("drop --page-all to save a binary response, or drop --output to paginate JSON").
212+
WithParams(
213+
errs.InvalidParam{Name: "--output", Reason: "conflicts with --page-all"},
214+
errs.InvalidParam{Name: "--page-all", Reason: "conflicts with --output"},
215+
)
200216
}
201217
if err := output.ValidateJqFlags(opts.JqExpr, opts.Output, opts.Format); err != nil {
202218
return err
@@ -239,11 +255,11 @@ func apiRun(opts *APIOptions) error {
239255

240256
resp, err := ac.DoAPI(opts.Ctx, request)
241257
if err != nil {
242-
// MarkRaw tells the dispatcher to skip the legacy enrichPermissionError
243-
// pass on *output.ExitError values. Typed *errs.* errors that flow
244-
// through here keep their canonical message / hint from BuildAPIError;
245-
// MarkRaw is a no-op on those (it only flips a flag on *ExitError).
246-
return output.MarkRaw(err)
258+
// MarkRaw tells the dispatcher to skip hint enrichment so the typed
259+
// error keeps its canonical message / hint from BuildAPIError: the
260+
// `api` command is a passthrough surface and the caller wants the
261+
// original Lark response wording, not a locally enriched variant.
262+
return errs.MarkRaw(err)
247263
}
248264
err = client.HandleResponse(resp, client.ResponseOptions{
249265
OutputPath: opts.Output,
@@ -260,10 +276,10 @@ func apiRun(opts *APIOptions) error {
260276
// the client populate identity-aware fields (ConsoleURL etc.).
261277
CheckError: ac.CheckResponse,
262278
})
263-
// MarkRaw: see comment above on the DoAPI path. Skips legacy
264-
// *ExitError enrichment; typed errors flow through unchanged.
279+
// MarkRaw: see comment above on the DoAPI path. Skips dispatcher hint
280+
// enrichment; the typed error's own message / hint flow through unchanged.
265281
if err != nil {
266-
return output.MarkRaw(err)
282+
return errs.MarkRaw(err)
267283
}
268284
return nil
269285
}
@@ -279,7 +295,7 @@ func apiPaginate(ctx context.Context, ac *client.APIClient, request client.RawAp
279295
// When jq is set, always aggregate all pages then filter.
280296
if jqExpr != "" {
281297
if err := client.PaginateWithJq(ctx, ac, request, jqExpr, out, pagOpts, ac.CheckResponse); err != nil {
282-
return output.MarkRaw(err)
298+
return errs.MarkRaw(err)
283299
}
284300
return nil
285301
}
@@ -291,11 +307,11 @@ func apiPaginate(ctx context.Context, ac *client.APIClient, request client.RawAp
291307
pf.FormatPage(items)
292308
}, pagOpts)
293309
if err != nil {
294-
return output.MarkRaw(err)
310+
return errs.MarkRaw(err)
295311
}
296312
if apiErr := ac.CheckResponse(result, pagOpts.Identity); apiErr != nil {
297313
output.FormatValue(out, result, output.FormatJSON)
298-
return output.MarkRaw(apiErr)
314+
return errs.MarkRaw(apiErr)
299315
}
300316
if !hasItems {
301317
fmt.Fprintf(errOut, "warning: this API does not return a list, format %q is not supported, falling back to json\n", format)
@@ -305,11 +321,11 @@ func apiPaginate(ctx context.Context, ac *client.APIClient, request client.RawAp
305321
default:
306322
result, err := ac.PaginateAll(ctx, request, pagOpts)
307323
if err != nil {
308-
return output.MarkRaw(err)
324+
return errs.MarkRaw(err)
309325
}
310326
if apiErr := ac.CheckResponse(result, pagOpts.Identity); apiErr != nil {
311327
output.FormatValue(out, result, output.FormatJSON)
312-
return output.MarkRaw(apiErr)
328+
return errs.MarkRaw(apiErr)
313329
}
314330
output.FormatValue(out, result, format)
315331
return nil

cmd/api/api_test.go

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"github.com/larksuite/cli/errs"
1414
"github.com/larksuite/cli/internal/cmdutil"
1515
"github.com/larksuite/cli/internal/core"
16+
"github.com/larksuite/cli/internal/errclass"
1617
"github.com/larksuite/cli/internal/httpmock"
1718
"github.com/spf13/cobra"
1819
)
@@ -250,6 +251,28 @@ func TestApiCmd_ParamsAndDataBothStdinConflict(t *testing.T) {
250251
if !strings.Contains(err.Error(), "cannot both read from stdin") {
251252
t.Errorf("expected stdin conflict error, got: %v", err)
252253
}
254+
var ve *errs.ValidationError
255+
if !errors.As(err, &ve) {
256+
t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err)
257+
}
258+
if ve.Subtype != errs.SubtypeInvalidArgument {
259+
t.Errorf("Subtype = %q, want %q", ve.Subtype, errs.SubtypeInvalidArgument)
260+
}
261+
requireInvalidParamNames(t, ve, "--params", "--data")
262+
}
263+
264+
// requireInvalidParamNames asserts that ve.Params carries exactly the given
265+
// flag names (order-sensitive, mirroring declaration order at the call site).
266+
func requireInvalidParamNames(t *testing.T, ve *errs.ValidationError, names ...string) {
267+
t.Helper()
268+
if len(ve.Params) != len(names) {
269+
t.Fatalf("Params = %+v, want %d entries %v", ve.Params, len(names), names)
270+
}
271+
for i, name := range names {
272+
if ve.Params[i].Name != name {
273+
t.Errorf("Params[%d].Name = %q, want %q", i, ve.Params[i].Name, name)
274+
}
275+
}
253276
}
254277

255278
func TestApiCmd_OutputAndPageAllConflict(t *testing.T) {
@@ -270,6 +293,41 @@ func TestApiCmd_OutputAndPageAllConflict(t *testing.T) {
270293
if gotOpts != nil && !strings.Contains(err.Error(), "mutually exclusive") {
271294
t.Errorf("expected 'mutually exclusive' error, got: %v", err)
272295
}
296+
var ve *errs.ValidationError
297+
if !errors.As(err, &ve) {
298+
t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err)
299+
}
300+
if ve.Subtype != errs.SubtypeInvalidArgument {
301+
t.Errorf("Subtype = %q, want %q", ve.Subtype, errs.SubtypeInvalidArgument)
302+
}
303+
requireInvalidParamNames(t, ve, "--output", "--page-all")
304+
}
305+
306+
// TestApiCmd_FileDataNotObject_TypedValidation pins the typed envelope for
307+
// the --file + non-object --data rejection: *errs.ValidationError with
308+
// subtype invalid_argument and the offending flag on Param.
309+
func TestApiCmd_FileDataNotObject_TypedValidation(t *testing.T) {
310+
f, _, _, _ := cmdutil.TestFactory(t, &core.CliConfig{
311+
AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu,
312+
})
313+
cmd := NewCmdApi(f, func(opts *APIOptions) error {
314+
return apiRun(opts)
315+
})
316+
cmd.SetArgs([]string{"POST", "/open-apis/test", "--as", "bot", "--file", "photo.jpg", "--data", `["not-an-object"]`})
317+
err := cmd.Execute()
318+
if err == nil {
319+
t.Fatal("expected error for non-object --data with --file")
320+
}
321+
var ve *errs.ValidationError
322+
if !errors.As(err, &ve) {
323+
t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err)
324+
}
325+
if ve.Subtype != errs.SubtypeInvalidArgument {
326+
t.Errorf("Subtype = %q, want %q", ve.Subtype, errs.SubtypeInvalidArgument)
327+
}
328+
if ve.Param != "--data" {
329+
t.Errorf("Param = %q, want %q", ve.Param, "--data")
330+
}
273331
}
274332

275333
func TestApiCmd_BinaryResponse_AutoSave(t *testing.T) {
@@ -360,6 +418,11 @@ func TestApiCmd_PageAll_NonBatchAPI_ErrorStillOutputsJSON(t *testing.T) {
360418
if !strings.Contains(stdout.String(), "no permission") {
361419
t.Errorf("expected error message in stdout, got: %s", stdout.String())
362420
}
421+
// --page-all errors are raw passthrough: the dispatcher must not rewrite
422+
// the message / hint with local enrichment.
423+
if !errs.IsRaw(err) {
424+
t.Errorf("expected --page-all error to be marked raw, got %T: %v", err, err)
425+
}
363426
}
364427

365428
func TestApiCmd_PageAll_BatchAPI_StreamsItems(t *testing.T) {
@@ -737,6 +800,64 @@ func TestApiCmd_PermissionError_DerivesFirstClassFields(t *testing.T) {
737800
}
738801
}
739802

803+
// TestApiCmd_APIError_RawPassthrough pins the raw-passthrough contract of the
804+
// `api` command: errors returned to the dispatcher are marked raw via
805+
// errs.MarkRaw, so the dispatcher skips hint enrichment and the message /
806+
// hint stay exactly what errclass.BuildAPIError derived from the Lark
807+
// response at classification time — nothing in the api command path rewrites
808+
// them afterwards.
809+
func TestApiCmd_APIError_RawPassthrough(t *testing.T) {
810+
f, _, _, reg := cmdutil.TestFactory(t, &core.CliConfig{
811+
AppID: "cli_test_raw", AppSecret: "secret", Brand: core.BrandFeishu,
812+
})
813+
814+
respBody := map[string]interface{}{
815+
"code": 99991679,
816+
"msg": "scope missing",
817+
}
818+
reg.Register(&httpmock.Stub{
819+
URL: "/open-apis/docx/v1/documents/raw",
820+
Body: respBody,
821+
})
822+
823+
cmd := NewCmdApi(f, nil)
824+
cmd.SetArgs([]string{"GET", "/open-apis/docx/v1/documents/raw", "--as", "bot"})
825+
err := cmd.Execute()
826+
if err == nil {
827+
t.Fatal("expected error for non-zero code")
828+
}
829+
830+
if !errs.IsRaw(err) {
831+
t.Fatalf("expected error to be marked raw (errs.IsRaw), got %T: %v", err, err)
832+
}
833+
// The raw marker must not hide the typed error from the envelope writer.
834+
var pe *errs.PermissionError
835+
if !errors.As(err, &pe) {
836+
t.Fatalf("expected *errs.PermissionError through the raw marker, got %T: %v", err, err)
837+
}
838+
839+
// Canonical baseline: classify the same response body the same way
840+
// CheckResponse does. Message and hint surfaced by the command must equal
841+
// the classification-time values byte for byte.
842+
canonical := errclass.BuildAPIError(respBody, errclass.ClassifyContext{
843+
Brand: "feishu", AppID: "cli_test_raw", Identity: "bot",
844+
})
845+
var want *errs.PermissionError
846+
if !errors.As(canonical, &want) {
847+
t.Fatalf("expected canonical *errs.PermissionError, got %T: %v", canonical, canonical)
848+
}
849+
if pe.Message != want.Message {
850+
t.Errorf("Message = %q, want canonical %q (raw mode must not rewrite it)", pe.Message, want.Message)
851+
}
852+
if pe.Hint != want.Hint {
853+
t.Errorf("Hint = %q, want canonical %q (raw mode must not rewrite it)", pe.Hint, want.Hint)
854+
}
855+
// The dispatcher-side scope enrichment string must never appear in raw mode.
856+
if strings.Contains(pe.Hint, "current command requires scope(s)") {
857+
t.Errorf("hint was rewritten by enrichment in raw mode: %q", pe.Hint)
858+
}
859+
}
860+
740861
func TestApiCmd_JsonFlag_Accepted(t *testing.T) {
741862
f, _, _, _ := cmdutil.TestFactory(t, &core.CliConfig{
742863
AppID: "test-app", AppSecret: "test-secret", Brand: core.BrandFeishu,

cmd/auth/check_test.go

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,12 +33,9 @@ func TestAuthCheckRun_NotLoggedIn_ExitOneWithStdoutOnly(t *testing.T) {
3333
if got := output.ExitCodeOf(err); got != 1 {
3434
t.Errorf("exit code = %d, want 1 (predicate 'missing' signal)", got)
3535
}
36-
var bare *output.ExitError
36+
var bare *output.BareError
3737
if !errors.As(err, &bare) {
38-
t.Fatalf("expected *output.ExitError (ErrBare), got %T: %v", err, err)
39-
}
40-
if bare.Detail != nil {
41-
t.Errorf("ErrBare must carry no Detail (no envelope), got %+v", bare.Detail)
38+
t.Fatalf("expected *output.BareError (ErrBare), got %T: %v", err, err)
4239
}
4340

4441
if stderr.Len() != 0 {

cmd/auth/list.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99

1010
"github.com/spf13/cobra"
1111

12+
"github.com/larksuite/cli/errs"
1213
larkauth "github.com/larksuite/cli/internal/auth"
1314
"github.com/larksuite/cli/internal/cmdutil"
1415
"github.com/larksuite/cli/internal/core"
@@ -59,7 +60,7 @@ func authListRun(opts *ListOptions) error {
5960
// keep the same contract here. We still want the hint to be
6061
// workspace-aware, so we pull the message+hint out of
6162
// NotConfiguredError() instead of hard-coding it.
62-
var cfgErr *core.ConfigError
63+
var cfgErr *errs.ConfigError
6364
if errors.As(core.NotConfiguredError(), &cfgErr) {
6465
fmt.Fprintln(f.IOStreams.ErrOut, cfgErr.Message)
6566
if cfgErr.Hint != "" {

0 commit comments

Comments
 (0)