feat(errs): add structured CLI error contract#984
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds errs/ typed error taxonomy and projections; rewires client/auth/commands/output to produce and envelope typed errors; updates tests accordingly; introduces errclass code meta/classifier; adds lintcheck tool and CI step; adjusts exit code mapping and config-auth promotion; widespread deprecation notes. ChangesTyped error system and integration
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as CLI User
participant CLI as CLI Command
participant Client as APIClient
participant Class as errclass.Classify
participant Errs as errs.Typed
participant Out as output
User->>CLI: run command
CLI->>Client: DoSDKRequest/CallAPI
Client-->>Client: WrapDoAPIError/WrapJSONResponseParseError
Client->>Class: BuildAPIError(result, identity)
Class-->>Client: *errs.(API|Permission|... )Error
Client-->>CLI: return typed error
CLI->>Errs: ProblemOf / predicates
CLI->>Out: WriteTypedErrorEnvelope(err, identity)
Out-->>User: typed stderr JSON + mapped exit code
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
github-actions
Bot
added
domain/base
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (1)
errs/marshal_test.go (1)
63-64: ⚡ Quick winAvoid discarding
json.Marshalerrors in tests.These tests currently ignore marshal failures, which can create misleading assertions on empty/invalid payloads. Capture and assert
err == nilconsistently.Proposed cleanup
- b, _ := json.Marshal(ve) + b, err := json.Marshal(ve) + if err != nil { + t.Fatal(err) + }You can apply the same pattern to each marshal call in this file.
Also applies to: 78-79, 89-90, 108-109, 122-123, 136-137, 147-148, 164-165, 175-176, 193-194, 209-210, 224-225
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@errs/marshal_test.go` around lines 63 - 64, The tests call json.Marshal into variables like b and then ignore the error (e.g., b, _ := json.Marshal(ve); s := string(b)); update each marshal invocation (all occurrences around variables b and s) to capture the error (b, err := json.Marshal(...)) and add an assertion that err == nil (use the test helper being used in the file, e.g., require.NoError(t, err) or if that framework isn't used, t.Fatalf/if err != nil { t.Fatalf(...) }) before converting b to string; apply this pattern to every marshal call listed (lines near the examples: 63-64, 78-79, 89-90, 108-109, 122-123, 136-137, 147-148, 164-165, 175-176, 193-194, 209-210, 224-225).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/auth/scopes.go`:
- Around line 57-65: The current return in getAppInfo unconditionally wraps any
error as *errs.PermissionError (using errs.Problem/SubtypeAppScopeNotEnabled),
which hides the original error type and subtype; change the logic in getAppInfo
to detect and preserve non-permission errors (e.g., if err is already an
*errs.Error/*errs.Problem or not related to app-scope authorization) and only
construct and return a new *errs.PermissionError when the failure is truly an
authorization/app-scope issue; reference the existing symbols
errs.PermissionError, errs.Problem, errs.SubtypeAppScopeNotEnabled and the
getAppInfo call site to implement a type-check/inspect-and-branch so other
failure categories (network/auth/internal) are returned unchanged.
In `@cmd/lintcheck/main.go`:
- Around line 41-50: The CLI-provided root path is used without validation; call
validate.SafeInputPath on the resolved root (the local variable root) before
passing it to lintcheck.ScanRepo, handle the error returned (log or exit
appropriately) and only call lintcheck.ScanRepo with the validated/clean path;
update the code around the root variable and the lintcheck.ScanRepo call to use
the validated path and error handling to enforce path-safety.
In `@errs/subtypes_service_task.go`:
- Line 10: Update the header comment in errs/subtypes_service_task.go to point
to the correct consumer path: replace the incorrect reference to
internal/output/codemeta_task.go with internal/errclass/codemeta_task.go so the
comment accurately states where these task service subtypes are consumed; ensure
the comment text around "Task service subtypes" and any mention of
codemeta_task.go is updated accordingly.
In `@internal/client/pagination.go`:
- Around line 31-34: The fallback for identity currently sets identity :=
pagOpts.Identity and then defaults to core.AsUser if empty, which ignores
request.As; update the logic so identity is determined by checking
pagOpts.Identity first, then request.As, and only then defaulting to core.AsUser
(i.e., set identity = pagOpts.Identity if non-empty, else use request.As if
non-empty, else core.AsUser) so that checkErr and related metadata use the
correct request identity.
In `@internal/errcompat/promote.go`:
- Around line 27-29: PromoteConfigError currently dereferences cfgErr without
checking for nil; add a nil guard at the start of PromoteConfigError (the
function that accepts *core.ConfigError) and return nil (or a sensible error)
immediately if cfgErr is nil to avoid a panic, then proceed to inspect
cfgErr.Type and construct errs.AuthError / other error types as before.
In `@internal/lintcheck/rules.go`:
- Around line 125-126: The condition that tries to match RegisterServiceMap
variants misses names with infixes; update the callee check in the lint rule
(where the variable callee is compared alongside mergeCodeMeta) to use
strings.Contains(callee, "RegisterServiceMap") instead of
strings.HasPrefix/HasSuffix so any name containing "RegisterServiceMap" (e.g.,
FooRegisterServiceMapBar) is detected; keep the equality check for
"mergeCodeMeta" intact and replace only the RegisterServiceMap prefix/suffix
logic in the function in internal/lintcheck/rules.go.
- Around line 77-80: The rule currently flags any type whose name ends with
"Error" (using typeSpec.Name.Name); change it to only apply to exported Error
types by first ensuring the identifier is exported (e.g., check that the name is
non-empty and its first rune is uppercase or use a proper IsExported helper)
before applying the strings.HasSuffix(name, "Error") test, so unexported
"*Error" structs are ignored; update the condition around typeSpec.Name.Name to
return true for non-exported names and only run the suffix check for exported
identifiers.
In `@internal/lintcheck/scan.go`:
- Around line 28-29: The ScanRepo function currently consumes the CLI-controlled
root path directly (used with filepath.Join and directory walking); validate the
repository root by calling validate.SafeInputPath(root) at the start of ScanRepo
and return a clear error if validation fails before calling
LoadSubtypeAllowlists or performing any file system operations; apply the same
safe-path gate to any other functions in this file that accept a root/path (the
block referenced around the LoadSubtypeAllowlists use and the code region
covering the later scan logic) so all file reads/walks only occur after
validate.SafeInputPath has approved the input.
- Around line 41-43: Replace all direct os.* filesystem calls in scan.go with
the vfs equivalents to preserve test-mocking: change os.Stat -> vfs.Stat,
os.ReadDir -> vfs.ReadDir, os.ReadFile -> vfs.ReadFile and os.IsNotExist ->
vfs.IsNotExist, update the import to use the vfs package, and keep existing
error branches (e.g., the else-if that currently reads "!os.IsNotExist(err)")
but using vfs.IsNotExist(err) so behavior is identical while enabling vfs-based
tests; apply the same replacements for every occurrence noted (the blocks using
os.Stat, os.ReadDir, os.ReadFile and os.IsNotExist).
- Around line 311-315: CheckErrsContract is currently collecting any struct
whose name ends with "Error" (via the loop that inspects
d.Type.(*ast.StructType) and uses d.Name.Name) including unexported/internal
types; modify that logic to only register exported error types by adding an
exported-name check (e.g., use ast.IsExported(d.Name.Name) or test the first
rune is upper-case) before writing into typedErrors so only exported "*Error"
structs are added; keep the existing typedErrors map and fset.Position(d.Pos())
usage unchanged.
- Around line 29-34: The code currently treats any error from
LoadSubtypeAllowlists as "missing files" and silently disables Rule E; instead,
call LoadSubtypeAllowlists(root+"/errs") and if err is nil proceed, if
errors.Is(err, os.ErrNotExist) (or os.IsNotExist(err) / errors.Is(err,
fs.ErrNotExist) depending on imports) then set allowlist/nameset = nil to skip
Rule E, otherwise propagate or return the error (or log and fail CI) so genuine
parse/read failures are not swallowed; reference the LoadSubtypeAllowlists call
and the allowlist/nameset variables to locate where to add the is-not-exist
check and error propagation.
In `@internal/output/errors.go`:
- Around line 202-204: The code currently returns true when enc.Encode(env)
fails (encErr), which wrongly signals the envelope was written and suppresses
fallback; change the behavior so that on enc.Encode(env) error you do not report
"written" — e.g., handle or log encErr as appropriate and return false (or
propagate the error) instead of returning true; make this change where
enc.Encode(env) is called (variables enc, env, encErr).
---
Nitpick comments:
In `@errs/marshal_test.go`:
- Around line 63-64: The tests call json.Marshal into variables like b and then
ignore the error (e.g., b, _ := json.Marshal(ve); s := string(b)); update each
marshal invocation (all occurrences around variables b and s) to capture the
error (b, err := json.Marshal(...)) and add an assertion that err == nil (use
the test helper being used in the file, e.g., require.NoError(t, err) or if that
framework isn't used, t.Fatalf/if err != nil { t.Fatalf(...) }) before
converting b to string; apply this pattern to every marshal call listed (lines
near the examples: 63-64, 78-79, 89-90, 108-109, 122-123, 136-137, 147-148,
164-165, 175-176, 193-194, 209-210, 224-225).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 715761fd-2c37-462e-a0fb-fb08f53b8426
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (79)
.github/workflows/ci.yml.golangci.ymlcmd/api/api.gocmd/api/api_test.gocmd/auth/scopes.gocmd/config/bind_test.gocmd/config/config_test.gocmd/error_auth_hint.gocmd/event/runtime.gocmd/lintcheck/main.gocmd/root.gocmd/root_integration_test.gocmd/root_test.gocmd/service/service.gocmd/service/service_test.goerrs/category.goerrs/category_test.goerrs/doc.goerrs/internal_carrier.goerrs/marshal_test.goerrs/predicates.goerrs/predicates_test.goerrs/problem.goerrs/problem_test.goerrs/projection/mcp.goerrs/projection/mcp_test.goerrs/projection/oauth.goerrs/projection/oauth_test.goerrs/subtypes.goerrs/subtypes_service_task.goerrs/types.goerrs/types_test.goerrs/wrap.goerrs/wrap_test.gogo.modinternal/auth/errors.gointernal/auth/transport.gointernal/auth/uat_client.gointernal/client/api_errors.gointernal/client/api_errors_test.gointernal/client/client.gointernal/client/client_test.gointernal/client/pagination.gointernal/client/response.gointernal/client/response_test.gointernal/cmdutil/json.gointernal/core/config.gointernal/core/errors.gointernal/core/notconfigured.gointernal/errclass/classify.gointernal/errclass/classify_test.gointernal/errclass/codemeta.gointernal/errclass/codemeta_task.gointernal/errclass/codemeta_test.gointernal/errcompat/promote.gointernal/errcompat/promote_test.gointernal/lintcheck/rules.gointernal/lintcheck/rules_test.gointernal/lintcheck/scan.gointernal/lintcheck/scan_test.gointernal/lintcheck/typecheck.gointernal/lintcheck/typecheck_test.gointernal/output/errors.gointernal/output/exitcode.gointernal/output/exitcode_test.gointernal/output/lark_errors.goshortcuts/base/base_execute_test.goshortcuts/calendar/calendar_test.goshortcuts/common/drive_media_upload.goshortcuts/common/drive_media_upload_test.goshortcuts/common/mcp_client_test.goshortcuts/drive/drive_status_test.goshortcuts/mail/mail_shortcut_validation_test.goshortcuts/markdown/helpers.goshortcuts/markdown/markdown_diff_test.goshortcuts/task/task_body_test.goshortcuts/task/task_query_helpers_test.goshortcuts/task/task_upload_attachment_test.goshortcuts/task/task_util_test.go
💤 Files with no reviewable changes (1)
- cmd/error_auth_hint.go
evandance
force-pushed
the
feat/error-contract-framework
branch
from
255fc10 to
24b4a47
Compare
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@579b037c7a9a1ffd93a328022017774f38cf3750🧩 Skill updatenpx skills add larksuite/cli#feat/error-contract-framework -y -g |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #984 +/- ##
==========================================
+ Coverage 67.91% 67.92% +0.01%
==========================================
Files 592 601 +9
Lines 55410 55762 +352
==========================================
+ Hits 37631 37876 +245
- Misses 14669 14751 +82
- Partials 3110 3135 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/auth/uat_client.go (1)
250-267:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReclassify the second refresh response before fallback token-clear.
On Line 260-Line 267, after the retry call you only check “failed or not” and then clear token. If the second response is a policy block, the function drops
challenge_url/typed policy context and returnsnil, which breaks the policy carve-out behavior.Suggested patch
code = getInt(data, "code", -1) errStr = getStr(data, "error") if (code != -1 && code != 0) || errStr != "" { + if meta2, ok2 := errclass.LookupCodeMeta(code); ok2 && meta2.Category == errs.CategoryPolicy { + return nil, &errs.SecurityPolicyError{ + Problem: errs.Problem{ + Category: errs.CategoryPolicy, + Subtype: meta2.Subtype, + Code: code, + Message: getStr(data, "error_description"), + Hint: getStr(data, "cli_hint"), + }, + ChallengeURL: getStr(data, "challenge_url"), + } + } fmt.Fprintf(errOut, "[lark-cli] [WARN] uat-client: refresh failed after retry (code=%d) for %s, clearing token\n", code, opts.UserOpenId) if err := RemoveStoredToken(opts.AppId, opts.UserOpenId); err != nil { fmt.Fprintf(errOut, "[lark-cli] [WARN] uat-client: failed to remove token: %v\n", err) } return nil, nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/uat_client.go` around lines 250 - 267, After the retry call in the refresh path (inside the metaOK && meta.Category == errs.CategoryAuthentication && meta.Retryable block around callEndpoint and RemoveStoredToken), reclassify the second response before deciding to clear the token: parse the returned data into the same error/meta representation you used originally (i.e., recompute meta/category from data after callEndpoint), and if the reclassified meta indicates a policy block (errs.CategoryPolicy or a presence of challenge_url/typed policy context) return the data so the caller can handle the policy flow instead of clearing the token; only clear the stored token when the reclassified response is still an authentication failure/retryable auth error. Ensure you reference the existing variables/functions used there (callEndpoint, code/getInt, errStr/getStr, meta, RemoveStoredToken) so you reuse the same classification logic.
♻️ Duplicate comments (2)
cmd/auth/scopes.go (1)
57-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
getAppInfo's original typed failures instead of always converting toPermissionError.This branch rewrites every failure into
*errs.PermissionError, so non-permission failures (e.g., network errors, authentication failures, internal errors) lose their original subtype and produce incorrect exit codes/hints. For example, a network timeout would incorrectly tell the user to "ensure the app has enabled the application:application:self_manage scope."🐛 Proposed fix — preserve original error types
appInfo, err := getAppInfo(opts.Ctx, f, config.AppID) if err != nil { + // Preserve typed errors (network, auth, etc.) — only wrap unknown + // errors as permission failures when we're confident the cause is + // scope-related. + var perm *errs.PermissionError + if errors.As(err, &perm) { + if perm.ConsoleURL == "" { + perm.ConsoleURL = errclass.ConsoleURL(string(config.Brand), config.AppID, nil) + } + return perm + } + // If it's already a typed error, return as-is + if _, ok := errs.UnwrapTypedError(err); ok { + return err + } + // Only wrap truly unknown errors as permission failures return &errs.PermissionError{ Problem: errs.Problem{ Category: errs.CategoryAuthorization,Add the
"errors"import at the top of the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/auth/scopes.go` around lines 57 - 65, The code currently converts all failures from getAppInfo into a new *errs.PermissionError; instead, detect and preserve typed errors from getAppInfo (use errors.As or errors.Is) and only construct a PermissionError when the underlying error is not already an errs.* type that should be propagated; add the "errors" import, attempt errors.As(err, &var) to see if err is already an *errs.PermissionError (or other errs.* types) and return it directly, otherwise build and return the PermissionError with the same message/hint/ConsoleURL as before.internal/output/errors.go (1)
202-204:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not report "written" when typed envelope encoding fails.
Returning
truehere signals success to the caller, but encoding actually failed and nothing was written. This can suppress fallback handling while emitting nothing to the user.🐛 Proposed fix
if encErr := enc.Encode(env); encErr != nil { - return true // pretend written; we still consumed the typed error + return false }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/output/errors.go` around lines 202 - 204, The code currently returns true when enc.Encode(env) fails, incorrectly signaling success; change the branch handling enc.Encode(env) in the envelope write/emit function (the block that checks "if encErr := enc.Encode(env); encErr != nil") to return false (and/or propagate the actual error up if the surrounding function supports an error return) so failure to encode does not claim the envelope was written and callers can perform fallback/error handling.
🧹 Nitpick comments (2)
shortcuts/task/task_body_test.go (1)
71-80: ⚡ Quick win
wantSubstrexpectations are currently not asserted.Line 71 and Line 138 validate code/category, but the table’s
wantSubstris never checked, so message regressions won’t be caught.Proposed patch
@@ if string(p.Category) != tt.wantType { t.Errorf("error type = %q, want %q (err = %v)", p.Category, tt.wantType, err) } + if tt.wantSubstr != "" && !strings.Contains(err.Error(), tt.wantSubstr) { + t.Errorf("error = %q, want substring %q", err.Error(), tt.wantSubstr) + } }) } } @@ if string(p.Category) != tt.wantType { t.Errorf("error type = %q, want %q (err = %v)", p.Category, tt.wantType, err) } + if tt.wantSubstr != "" && !strings.Contains(err.Error(), tt.wantSubstr) { + t.Errorf("error = %q, want substring %q", err.Error(), tt.wantSubstr) + } }) } }Also applies to: 138-147
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/task/task_body_test.go` around lines 71 - 80, Tests currently check ExitCodeOf(err) and errs.ProblemOf(err)/p.Category but never assert the table's tt.wantSubstr, so add an assertion that the error message contains tt.wantSubstr after the existing checks: use err.Error() (or fmt.Sprintf("%v", err)) and t.Errorf when tt.wantSubstr is non-empty and not a substring. Apply the same change in the other test block that uses output.ExitCodeOf and errs.ProblemOf (the block around lines 138-147) so both places assert the message contains tt.wantSubstr.internal/lintcheck/rules.go (1)
259-259: 💤 Low valueConsider compiling the regex once at package level.
The
ad_hoc_*regex is recompiled on every call toscanSubtype. For a linting tool that processes many files, this adds unnecessary overhead.♻️ Proposed refactor
+var adHocRe = regexp.MustCompile(`^ad_hoc_[a-z0-9_]+$`) + func scanSubtype(path, src string, allowlist, nameset map[string]struct{}, scope *TypedScope, absPath string) ([]Violation, error) { fset := token.NewFileSet() file, err := parser.ParseFile(fset, path, src, parser.ParseComments) if err != nil { return nil, err } - adHoc := regexp.MustCompile(`^ad_hoc_[a-z0-9_]+$`) + adHoc := adHocRe var out []Violation🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/lintcheck/rules.go` at line 259, The regex `regexp.MustCompile(\`^ad_hoc_[a-z0-9_]+$\`)` is being compiled inside scanSubtype on every call; move it to a package-level variable (e.g., declare var adHoc = regexp.MustCompile(`^ad_hoc_[a-z0-9_]+$`) at top-level) and update scanSubtype to reuse that adHoc variable so the pattern is compiled once for the whole process.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/service/service.go`:
- Around line 288-291: The call to ac.DoAPI in serviceMethodRun (RunE) returns
raw errors from the credential/auth chain (e.g.,
DefaultTokenProvider.ResolveToken, auth.GetValidAccessToken) which must be
converted to typed output errors; modify the code around the ac.DoAPI call so
any non-nil err is wrapped using the same output.* error types used elsewhere
(for example reuse WrapDoAPIError-style wrapping or create an output-specific
wrapper) before returning from RunE, ensuring resolveAccessToken and any
DoAPI-originated errors never propagate as untyped errors.
In `@errs/predicates.go`:
- Around line 12-17: ProblemOf currently returns ok=true even when the
underlying Problem pointer is nil, causing callers (e.g., CategoryOf,
IsRetryable) to dereference nil; update ProblemOf to check the returned *Problem
from c.ProblemDetail() and only return (p, true) when p != nil, otherwise return
(nil, false). Apply the same defensive pattern to the other similar helper(s)
that call problemCarrier.ProblemDetail() (such as the functions handling
category/retry checks) so they never signal ok=true with a nil *Problem.
In `@errs/problem.go`:
- Around line 29-30: The Error method for type *Problem should guard against a
nil receiver to avoid panics when a nil *Problem is stored in an error
interface; modify (*Problem).Error() to check if p == nil and return an
appropriate string (e.g., "" or "<nil>") instead of dereferencing p.Message,
keeping the rest of the behavior unchanged and leaving ProblemDetail() as-is.
---
Outside diff comments:
In `@internal/auth/uat_client.go`:
- Around line 250-267: After the retry call in the refresh path (inside the
metaOK && meta.Category == errs.CategoryAuthentication && meta.Retryable block
around callEndpoint and RemoveStoredToken), reclassify the second response
before deciding to clear the token: parse the returned data into the same
error/meta representation you used originally (i.e., recompute meta/category
from data after callEndpoint), and if the reclassified meta indicates a policy
block (errs.CategoryPolicy or a presence of challenge_url/typed policy context)
return the data so the caller can handle the policy flow instead of clearing the
token; only clear the stored token when the reclassified response is still an
authentication failure/retryable auth error. Ensure you reference the existing
variables/functions used there (callEndpoint, code/getInt, errStr/getStr, meta,
RemoveStoredToken) so you reuse the same classification logic.
---
Duplicate comments:
In `@cmd/auth/scopes.go`:
- Around line 57-65: The code currently converts all failures from getAppInfo
into a new *errs.PermissionError; instead, detect and preserve typed errors from
getAppInfo (use errors.As or errors.Is) and only construct a PermissionError
when the underlying error is not already an errs.* type that should be
propagated; add the "errors" import, attempt errors.As(err, &var) to see if err
is already an *errs.PermissionError (or other errs.* types) and return it
directly, otherwise build and return the PermissionError with the same
message/hint/ConsoleURL as before.
In `@internal/output/errors.go`:
- Around line 202-204: The code currently returns true when enc.Encode(env)
fails, incorrectly signaling success; change the branch handling enc.Encode(env)
in the envelope write/emit function (the block that checks "if encErr :=
enc.Encode(env); encErr != nil") to return false (and/or propagate the actual
error up if the surrounding function supports an error return) so failure to
encode does not claim the envelope was written and callers can perform
fallback/error handling.
---
Nitpick comments:
In `@internal/lintcheck/rules.go`:
- Line 259: The regex `regexp.MustCompile(\`^ad_hoc_[a-z0-9_]+$\`)` is being
compiled inside scanSubtype on every call; move it to a package-level variable
(e.g., declare var adHoc = regexp.MustCompile(`^ad_hoc_[a-z0-9_]+$`) at
top-level) and update scanSubtype to reuse that adHoc variable so the pattern is
compiled once for the whole process.
In `@shortcuts/task/task_body_test.go`:
- Around line 71-80: Tests currently check ExitCodeOf(err) and
errs.ProblemOf(err)/p.Category but never assert the table's tt.wantSubstr, so
add an assertion that the error message contains tt.wantSubstr after the
existing checks: use err.Error() (or fmt.Sprintf("%v", err)) and t.Errorf when
tt.wantSubstr is non-empty and not a substring. Apply the same change in the
other test block that uses output.ExitCodeOf and errs.ProblemOf (the block
around lines 138-147) so both places assert the message contains tt.wantSubstr.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee39d5f2-560e-4ec5-a762-b57943afc3b9
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (80)
.github/workflows/ci.yml.golangci.ymlcmd/api/api.gocmd/api/api_test.gocmd/auth/scopes.gocmd/config/bind_test.gocmd/config/config_test.gocmd/error_auth_hint.gocmd/event/runtime.gocmd/lintcheck/main.gocmd/root.gocmd/root_integration_test.gocmd/root_test.gocmd/service/service.gocmd/service/service_test.goerrs/category.goerrs/category_test.goerrs/doc.goerrs/internal_carrier.goerrs/marshal_test.goerrs/predicates.goerrs/predicates_test.goerrs/problem.goerrs/problem_test.goerrs/projection/mcp.goerrs/projection/mcp_test.goerrs/projection/oauth.goerrs/projection/oauth_test.goerrs/subtypes.goerrs/subtypes_service_task.goerrs/types.goerrs/types_test.goerrs/wrap.goerrs/wrap_test.gogo.modinternal/auth/errors.gointernal/auth/transport.gointernal/auth/transport_test.gointernal/auth/uat_client.gointernal/client/api_errors.gointernal/client/api_errors_test.gointernal/client/client.gointernal/client/client_test.gointernal/client/pagination.gointernal/client/response.gointernal/client/response_test.gointernal/cmdutil/json.gointernal/core/config.gointernal/core/errors.gointernal/core/notconfigured.gointernal/errclass/classify.gointernal/errclass/classify_test.gointernal/errclass/codemeta.gointernal/errclass/codemeta_task.gointernal/errclass/codemeta_test.gointernal/errcompat/promote.gointernal/errcompat/promote_test.gointernal/lintcheck/rules.gointernal/lintcheck/rules_test.gointernal/lintcheck/scan.gointernal/lintcheck/scan_test.gointernal/lintcheck/typecheck.gointernal/lintcheck/typecheck_test.gointernal/output/errors.gointernal/output/exitcode.gointernal/output/exitcode_test.gointernal/output/lark_errors.goshortcuts/base/base_execute_test.goshortcuts/calendar/calendar_test.goshortcuts/common/drive_media_upload.goshortcuts/common/drive_media_upload_test.goshortcuts/common/mcp_client_test.goshortcuts/drive/drive_status_test.goshortcuts/mail/mail_shortcut_validation_test.goshortcuts/markdown/helpers.goshortcuts/markdown/markdown_diff_test.goshortcuts/task/task_body_test.goshortcuts/task/task_query_helpers_test.goshortcuts/task/task_upload_attachment_test.goshortcuts/task/task_util_test.go
💤 Files with no reviewable changes (1)
- cmd/error_auth_hint.go
✅ Files skipped from review due to trivial changes (6)
- errs/internal_carrier.go
- shortcuts/base/base_execute_test.go
- errs/wrap.go
- internal/core/errors.go
- errs/subtypes_service_task.go
- errs/doc.go
evandance
force-pushed
the
feat/error-contract-framework
branch
from
24b4a47 to
661c2a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
cmd/auth/scopes.go (1)
57-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve non-authorization failures from
getAppInfoinstead of always remapping.This branch rewrites every
getAppInfofailure into*errs.PermissionError, which masks original network/auth/internal error types and can break downstreamerrors.Asbranching and exit-code expectations. Only map true app-scope authorization failures; return other error types unchanged.Suggested minimal fix
+import "errors" @@ appInfo, err := getAppInfo(opts.Ctx, f, config.AppID) if err != nil { - return &errs.PermissionError{ - Problem: errs.Problem{ - Category: errs.CategoryAuthorization, - Subtype: errs.SubtypeAppScopeNotEnabled, - Message: fmt.Sprintf("failed to get app scope info: %v", err), - Hint: "ensure the app has enabled the application:application:self_manage scope.", - }, - ConsoleURL: errclass.ConsoleURL(string(config.Brand), config.AppID, nil), - } + var perm *errs.PermissionError + if errors.As(err, &perm) { + if perm.Subtype == errs.SubtypeAppScopeNotEnabled && perm.ConsoleURL == "" { + perm.ConsoleURL = errclass.ConsoleURL(string(config.Brand), config.AppID, nil) + } + return perm + } + return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/auth/scopes.go` around lines 57 - 65, The current code unconditionally wraps any error from getAppInfo into an errs.PermissionError (SubtypeAppScopeNotEnabled), which hides non-authorization failures; change the logic in the caller of getAppInfo (the scope-checking path that currently returns &errs.PermissionError{...}) to inspect the original error returned by getAppInfo and only construct and return an errs.PermissionError when the error clearly indicates the app-scope authorization issue (e.g., match or use an exported sentinel, error type, or check for a specific error string/condition that denotes app scope not enabled); for all other errors returned by getAppInfo, return the original error unchanged so callers can still errors.As/handle exit codes correctly.errs/problem.go (1)
29-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard nil receivers in
(*Problem).Error()to avoid panic.A nil
*Problemstored in anerrorinterface will panic whenError()dereferencesp.Message.Suggested minimal fix
-func (p *Problem) Error() string { return p.Message } +func (p *Problem) Error() string { + if p == nil { + return "" + } + return p.Message +}#!/bin/bash # Verify current implementation lacks a nil guard in errs/problem.go rg -n -A4 -B1 'func \(p \*Problem\) Error\(\) string' errs/problem.go rg -n 'if p == nil' errs/problem.go🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@errs/problem.go` at line 29, The Error method on type (*Problem) should guard against a nil receiver to avoid panics when a nil *Problem is stored in an error interface; update the method (func (p *Problem) Error()) to check if p == nil and return a safe string (e.g. an empty string or "<nil>") instead of dereferencing p.Message, otherwise return p.Message as before so callers never panic.cmd/lintcheck/main.go (1)
41-50:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate the CLI-supplied repo root before scanning.
rootis user-controlled input and is passed tolintcheck.ScanRepowithoutvalidate.SafeInputPath, which bypasses the repo’s path-safety guardrail for untrusted arguments.Suggested minimal fix
import ( "flag" "fmt" "os" + "github.com/larksuite/cli/internal/validate" "github.com/larksuite/cli/internal/lintcheck" ) @@ if flag.NArg() > 0 { root = flag.Arg(0) @@ } } + if err := validate.SafeInputPath(root); err != nil { + fmt.Fprintf(os.Stderr, "lintcheck: invalid repo root: %v\n", err) + os.Exit(2) + } violations, err := lintcheck.ScanRepo(root)As per coding guidelines:
Validate paths before reading with validate.SafeInputPath because CLI arguments are untrusted.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/lintcheck/main.go` around lines 41 - 50, The CLI-provided root variable is untrusted and is passed directly to lintcheck.ScanRepo which bypasses path-safety checks; validate the path using validate.SafeInputPath (call it on root after parsing/normalizing "./...") and handle its returned error (log and exit) before calling lintcheck.ScanRepo so only safe paths are scanned; update the main function to replace the direct use of root with the validated path and propagate any validation error to prevent calling lintcheck.ScanRepo with unsafe input.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/root.go`:
- Around line 203-212: Update the dispatch-order comment to reflect the actual
handler flow: state that errs.SecurityPolicyError is routed to
writeSecurityPolicyError before the typed-envelope path, and adjust the ordering
so that (1) core.ConfigError is promoted/handled as described, (2)
errs.SecurityPolicyError is handled by writeSecurityPolicyError, (3) other typed
errors from errs/ (e.g. *errs.PermissionError, *errs.APIError) are routed to the
typed envelope writer using errs.CategoryOf/ExitCodeOf, and (4) legacy
*output.ExitError is written as-is; mention the preservation of the original
core.ConfigError on the Cause chain as before.
In `@internal/lintcheck/rules.go`:
- Around line 573-576: The service-scope detection in rules.go currently treats
all internal/* packages as service scope except internal/errclass; add an
explicit exclusion for internal/output so it does not count as service scope and
avoids Rule C false positives. Update the branching that checks package path p
(the cases referencing "internal/errclass" and "internal/") to first check for
"internal/output" (e.g., strings.HasPrefix(p, "internal/output/") ||
strings.Contains(p, "/internal/output/")) and return false for that case before
the general internal/* case so internal/output is excluded from isServiceScope
checks.
In `@shortcuts/task/task_body_test.go`:
- Around line 71-80: The test removed assertions that verify the error message
branch (wantSubstr), allowing false positives; restore a check after obtaining p
from errs.ProblemOf(err) to assert that the produced problem message (e.g.,
p.Message or string(err) depending on how the test reads the message) contains
tt.wantSubstr, using the same pattern in both places (the block around
output.ExitCodeOf/errs.ProblemOf and the similar block at the later occurrence).
Ensure you reference output.ExitCodeOf(err), errs.ProblemOf(err), and p.Category
when adding the assertion so the test fails if the wrong validation branch is
returned even when category/code remains "validation".
In `@shortcuts/task/task_util_test.go`:
- Around line 47-56: The test removed the assertion that the returned error
message contains the expected substring (wantSubstr), so restore an assertion
after the typed error check that verifies the error text includes tt.wantSubstr
(e.g., using strings.Contains(err.Error(), tt.wantSubstr) or
strings.Contains(p.Message, tt.wantSubstr) depending on the Problem type); add
the strings import if needed and fail the test with t.Errorf or t.Fatalf when
the substring is missing so branch-level validation reasons are covered.
---
Duplicate comments:
In `@cmd/auth/scopes.go`:
- Around line 57-65: The current code unconditionally wraps any error from
getAppInfo into an errs.PermissionError (SubtypeAppScopeNotEnabled), which hides
non-authorization failures; change the logic in the caller of getAppInfo (the
scope-checking path that currently returns &errs.PermissionError{...}) to
inspect the original error returned by getAppInfo and only construct and return
an errs.PermissionError when the error clearly indicates the app-scope
authorization issue (e.g., match or use an exported sentinel, error type, or
check for a specific error string/condition that denotes app scope not enabled);
for all other errors returned by getAppInfo, return the original error unchanged
so callers can still errors.As/handle exit codes correctly.
In `@cmd/lintcheck/main.go`:
- Around line 41-50: The CLI-provided root variable is untrusted and is passed
directly to lintcheck.ScanRepo which bypasses path-safety checks; validate the
path using validate.SafeInputPath (call it on root after parsing/normalizing
"./...") and handle its returned error (log and exit) before calling
lintcheck.ScanRepo so only safe paths are scanned; update the main function to
replace the direct use of root with the validated path and propagate any
validation error to prevent calling lintcheck.ScanRepo with unsafe input.
In `@errs/problem.go`:
- Line 29: The Error method on type (*Problem) should guard against a nil
receiver to avoid panics when a nil *Problem is stored in an error interface;
update the method (func (p *Problem) Error()) to check if p == nil and return a
safe string (e.g. an empty string or "<nil>") instead of dereferencing
p.Message, otherwise return p.Message as before so callers never panic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e28fc312-4b4c-4fca-9c42-735cfc58ed34
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (81)
.github/workflows/ci.yml.golangci.ymlcmd/api/api.gocmd/api/api_test.gocmd/auth/scopes.gocmd/config/bind_test.gocmd/config/config_test.gocmd/error_auth_hint.gocmd/event/runtime.gocmd/lintcheck/main.gocmd/root.gocmd/root_integration_test.gocmd/root_test.gocmd/service/service.gocmd/service/service_test.goerrs/category.goerrs/category_test.goerrs/doc.goerrs/internal_carrier.goerrs/marshal_test.goerrs/predicates.goerrs/predicates_test.goerrs/problem.goerrs/problem_test.goerrs/projection/mcp.goerrs/projection/mcp_test.goerrs/projection/oauth.goerrs/projection/oauth_test.goerrs/subtypes.goerrs/subtypes_service_task.goerrs/types.goerrs/types_test.goerrs/wrap.goerrs/wrap_test.gogo.modinternal/auth/errors.gointernal/auth/transport.gointernal/auth/transport_test.gointernal/auth/uat_client.gointernal/client/api_errors.gointernal/client/api_errors_test.gointernal/client/client.gointernal/client/client_test.gointernal/client/pagination.gointernal/client/response.gointernal/client/response_test.gointernal/cmdutil/json.gointernal/core/config.gointernal/core/errors.gointernal/core/notconfigured.gointernal/errclass/classify.gointernal/errclass/classify_test.gointernal/errclass/codemeta.gointernal/errclass/codemeta_task.gointernal/errclass/codemeta_test.gointernal/errcompat/promote.gointernal/errcompat/promote_test.gointernal/lintcheck/rules.gointernal/lintcheck/rules_test.gointernal/lintcheck/scan.gointernal/lintcheck/scan_test.gointernal/lintcheck/typecheck.gointernal/lintcheck/typecheck_test.gointernal/output/errors.gointernal/output/exitcode.gointernal/output/exitcode_test.gointernal/output/lark_errors.goshortcuts/base/base_execute_test.goshortcuts/calendar/calendar_test.goshortcuts/common/drive_media_upload.goshortcuts/common/drive_media_upload_test.goshortcuts/common/mcp_client_test.goshortcuts/drive/drive_status_test.goshortcuts/mail/mail_shortcut_validation_test.goshortcuts/markdown/helpers.goshortcuts/markdown/markdown_diff_test.goshortcuts/task/task_body_test.goshortcuts/task/task_query_helpers_test.goshortcuts/task/task_upload_attachment_test.goshortcuts/task/task_util_test.gotests/cli_e2e/config/bind_test.go
💤 Files with no reviewable changes (1)
- cmd/error_auth_hint.go
✅ Files skipped from review due to trivial changes (3)
- errs/subtypes_service_task.go
- internal/core/errors.go
- shortcuts/base/base_execute_test.go
evandance
force-pushed
the
feat/error-contract-framework
branch
2 times, most recently
from
6fbf14c to
4dd7ea7
Compare
evandance
added
domain/core
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
internal/auth/transport.go (1)
179-180: ⚡ Quick winAdd focused tests for the new policy-category branch and typed return path.
These new lines are currently uncovered; add cases that verify (1) non-policy codes are ignored, and (2) policy codes produce
*errs.SecurityPolicyErrorwith expectedProblemfields andChallengeURL.Also applies to: 206-213
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/transport.go` around lines 179 - 180, Add unit tests covering the new branch in the error-to-transport translation that uses errclass.LookupCodeMeta: write one test where LookupCodeMeta returns ok but meta.Category != errs.CategoryPolicy and assert the code path ignores policy handling (original error or non-policy behavior); and a second test where LookupCodeMeta returns ok with meta.Category == errs.CategoryPolicy and the function returns a *errs.SecurityPolicyError with the expected Problem fields and ChallengeURL populated. Use the same function under test that calls errclass.LookupCodeMeta (referenced in the diff) and assert types and field values on the returned error to exercise the typed return path.cmd/root.go (1)
230-233: ⚡ Quick winAdd focused tests for the newly added root-dispatch branches.
The new config-promotion path and
asExitErrornil-return path are currently uncovered, and both sit on the root error contract boundary. A small regression test here would materially reduce dispatch drift risk.Also applies to: 292-297
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/root.go` around lines 230 - 233, Add focused unit tests for the new root dispatch branches: write one test that simulates an error that satisfies errors.As(..., *core.ConfigError) and verifies that root dispatch calls errcompat.PromoteConfigError (and the resulting behavior/exit code) so the config-promotion path is covered, and write another test where asExitError returns nil to exercise the nil-return path on the root error contract boundary and assert the dispatcher handles it as expected; target the code paths around the root dispatch logic (the errors.As(core.ConfigError) branch, the call to errcompat.PromoteConfigError, and the asExitError nil-return behavior) so these two branches (mentioned in root.go) have explicit assertions.internal/auth/uat_client.go (1)
228-244: ⚡ Quick winAdd focused tests for policy-code metadata mapping in refresh flow.
This new branch changes observable behavior (returns
*errs.SecurityPolicyErrorwithSubtype/ChallengeURL) but is currently uncovered. A dedicated unit test here would lock the contract and prevent regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/auth/uat_client.go` around lines 228 - 244, Add a focused unit test that exercises the refresh flow branch where errclass.LookupCodeMeta(code) returns metaOK and meta.Category == errs.CategoryPolicy and verifies the returned error is a *errs.SecurityPolicyError with Problem.Subtype set to the meta.Subtype and ChallengeURL set from the "challenge_url" field; to do this, arrange the test to inject an error response payload containing "challenge_url", "cli_hint", and "error_description", stub or ensure errclass.LookupCodeMeta(code) returns a meta with CategoryPolicy and a known Subtype, invoke the refresh method in the UAT client (the code path that contains the meta, metaOK := errclass.LookupCodeMeta(code) block), and assert the error type and that Problem.Subtype, Problem.Code, Problem.Message, Problem.Hint and ChallengeURL match the expected values to lock the contract and prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/service/service.go`:
- Line 520: The call to checkErr is using request.As which can mismatch the
computed identity; change checkErr to use the effective identity stored in
pagOpts.Identity (set by servicePaginate) instead of request.As so
classification/hints use the correct identity. Locate the calls to checkErr in
the servicePaginate flow (the one currently passing request.As around line with
servicePaginate and the other at the later call) and replace the request.As
argument with pagOpts.Identity (or the variable holding that value) so both
error checks use the initialized pagOpts.Identity.
In `@internal/lintcheck/scan_test.go`:
- Around line 291-299: The local variable rejectCount declared and incremented
inside the loop over v is never used and causes a compilation error; remove the
declaration "rejectCount := 0" and the increment "if vv.Action ==
lintcheck.ActionReject { rejectCount++ }" (or alternatively use rejectCount in
an assertion) so only used variables remain—look for the loop iterating over v
and the rejectCount symbol in scan_test.go and delete those two lines to fix the
test build.
---
Nitpick comments:
In `@cmd/root.go`:
- Around line 230-233: Add focused unit tests for the new root dispatch
branches: write one test that simulates an error that satisfies errors.As(...,
*core.ConfigError) and verifies that root dispatch calls
errcompat.PromoteConfigError (and the resulting behavior/exit code) so the
config-promotion path is covered, and write another test where asExitError
returns nil to exercise the nil-return path on the root error contract boundary
and assert the dispatcher handles it as expected; target the code paths around
the root dispatch logic (the errors.As(core.ConfigError) branch, the call to
errcompat.PromoteConfigError, and the asExitError nil-return behavior) so these
two branches (mentioned in root.go) have explicit assertions.
In `@internal/auth/transport.go`:
- Around line 179-180: Add unit tests covering the new branch in the
error-to-transport translation that uses errclass.LookupCodeMeta: write one test
where LookupCodeMeta returns ok but meta.Category != errs.CategoryPolicy and
assert the code path ignores policy handling (original error or non-policy
behavior); and a second test where LookupCodeMeta returns ok with meta.Category
== errs.CategoryPolicy and the function returns a *errs.SecurityPolicyError with
the expected Problem fields and ChallengeURL populated. Use the same function
under test that calls errclass.LookupCodeMeta (referenced in the diff) and
assert types and field values on the returned error to exercise the typed return
path.
In `@internal/auth/uat_client.go`:
- Around line 228-244: Add a focused unit test that exercises the refresh flow
branch where errclass.LookupCodeMeta(code) returns metaOK and meta.Category ==
errs.CategoryPolicy and verifies the returned error is a
*errs.SecurityPolicyError with Problem.Subtype set to the meta.Subtype and
ChallengeURL set from the "challenge_url" field; to do this, arrange the test to
inject an error response payload containing "challenge_url", "cli_hint", and
"error_description", stub or ensure errclass.LookupCodeMeta(code) returns a meta
with CategoryPolicy and a known Subtype, invoke the refresh method in the UAT
client (the code path that contains the meta, metaOK :=
errclass.LookupCodeMeta(code) block), and assert the error type and that
Problem.Subtype, Problem.Code, Problem.Message, Problem.Hint and ChallengeURL
match the expected values to lock the contract and prevent regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2cc6200f-1045-49c8-a66b-0f20f8c12625
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (81)
.github/workflows/ci.yml.golangci.ymlcmd/api/api.gocmd/api/api_test.gocmd/auth/scopes.gocmd/config/bind_test.gocmd/config/config_test.gocmd/error_auth_hint.gocmd/event/runtime.gocmd/lintcheck/main.gocmd/root.gocmd/root_integration_test.gocmd/root_test.gocmd/service/service.gocmd/service/service_test.goerrs/category.goerrs/category_test.goerrs/doc.goerrs/internal_carrier.goerrs/marshal_test.goerrs/predicates.goerrs/predicates_test.goerrs/problem.goerrs/problem_test.goerrs/projection/mcp.goerrs/projection/mcp_test.goerrs/projection/oauth.goerrs/projection/oauth_test.goerrs/subtypes.goerrs/subtypes_service_task.goerrs/types.goerrs/types_test.goerrs/wrap.goerrs/wrap_test.gogo.modinternal/auth/errors.gointernal/auth/transport.gointernal/auth/transport_test.gointernal/auth/uat_client.gointernal/client/api_errors.gointernal/client/api_errors_test.gointernal/client/client.gointernal/client/client_test.gointernal/client/pagination.gointernal/client/response.gointernal/client/response_test.gointernal/cmdutil/json.gointernal/core/config.gointernal/core/errors.gointernal/core/notconfigured.gointernal/errclass/classify.gointernal/errclass/classify_test.gointernal/errclass/codemeta.gointernal/errclass/codemeta_task.gointernal/errclass/codemeta_test.gointernal/errcompat/promote.gointernal/errcompat/promote_test.gointernal/lintcheck/rules.gointernal/lintcheck/rules_test.gointernal/lintcheck/scan.gointernal/lintcheck/scan_test.gointernal/lintcheck/typecheck.gointernal/lintcheck/typecheck_test.gointernal/output/errors.gointernal/output/exitcode.gointernal/output/exitcode_test.gointernal/output/lark_errors.goshortcuts/base/base_execute_test.goshortcuts/calendar/calendar_test.goshortcuts/common/drive_media_upload.goshortcuts/common/drive_media_upload_test.goshortcuts/common/mcp_client_test.goshortcuts/drive/drive_status_test.goshortcuts/mail/mail_shortcut_validation_test.goshortcuts/markdown/helpers.goshortcuts/markdown/markdown_diff_test.goshortcuts/task/task_body_test.goshortcuts/task/task_query_helpers_test.goshortcuts/task/task_upload_attachment_test.goshortcuts/task/task_util_test.gotests/cli_e2e/config/bind_test.go
💤 Files with no reviewable changes (1)
- cmd/error_auth_hint.go
✅ Files skipped from review due to trivial changes (3)
- errs/doc.go
- errs/internal_carrier.go
- shortcuts/base/base_execute_test.go
evandance
force-pushed
the
feat/error-contract-framework
branch
from
4dd7ea7 to
186e8da
Compare
github-actions
Bot
added
domain/base
evandance
force-pushed
the
feat/error-contract-framework
branch
from
42c94f9 to
750d291
Compare
evandance
force-pushed
the
feat/error-contract-framework
branch
12 times, most recently
from
569e208 to
4fa3bbe
Compare
evandance
force-pushed
the
feat/error-contract-framework
branch
8 times, most recently
from
f0f9074 to
d792dfe
Compare
Introduce a typed error contract framework for lark-cli so in-process
Go callers can branch via errors.As(&errs.XxxError{}) and shell scripts,
AI agents, and protocol adapters can branch on stable JSON type/subtype
fields instead of regex-parsing free-form messages.
Adds:
- Canonical taxonomy under errs/ (9 categories + typed Error structs
embedding a shared Problem, RFC 7807-aligned)
- Centralized Lark code metadata + identity-aware BuildAPIError dispatch
- Typed JSON envelope writer alongside the legacy envelope writer
- MCP / OAuth (RFC 6750 Bearer) projection adapters
- Five CI lint guards preventing ad-hoc taxonomy drift
Backward compatibility: legacy *output.ExitError producers (ErrAPI,
ErrWithHint, Errorf, ErrBare) and business shortcuts that use them
continue to render the legacy envelope unchanged. SecurityPolicyError
wire format and exit code are preserved via a carve-out; taxonomy
migration is deferred to PR 2. Domain-specific business migration is
staged across PR 3+.
Framework-direct paths now return typed *errs.*Error: ErrAuth /
ErrValidation / ErrNetwork emit category literals on the wire
(authentication / validation / network), *core.ConfigError is promoted
at the cmd/root boundary with exit code aligned from 2 to 3, and Lark
API permission denials classified by BuildAPIError exit 3.
At the SDK boundary, WrapDoAPIError preserves any already-classified
error (legacy *output.ExitError or typed *errs.*) so output.ErrAuth
from missing credentials surfaces with the auth category and exit 3
intact instead of being downgraded to a network error. Policy responses
classified by BuildAPIError (codes 21000 / 21001) extract challenge_url
and the canonical hint from the response body, matching what the
auth transport already surfaces at the HTTP layer; non-https
challenge URLs are dropped.
First PR in the feat/error-contract-* series.
evandance
force-pushed
the
feat/error-contract-framework
branch
from
d792dfe to
579b037
Compare
liangshuo-1
approved these changes
May 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add a typed error framework. Go callers branch via
errors.As(&errs.XxxError{}); shell scripts, AI agents, and protocol adapters branch on stable JSONtype/subtypefields instead of regex-parsing free-form messages.This PR is the framework slice — it adds the package and infrastructure but does not migrate any business call sites. Migration of existing producers to typed errors will be handled uniformly in a follow-up.
What's added
errs/package — 9 categories (validation,authentication,authorization,config,network,api,policy,internal,confirmation), one typed struct per category, all embedding a shared RFC 7807-alignedProblem.IsXxxpredicates for ergonomic matching.internal/errclass/codemeta.go) — single source of truth mapping Lark numeric codes to{Category, Subtype, Retryable}. Consumed byerrclass.BuildAPIErrorfor typed dispatch.internal/output/errors.goWriteTypedErrorEnvelope) — emits{ok, identity, error: {type, subtype, code?, message, hint?, log_id?, retryable?, …extension fields}}. Called bycmd/root.go handleRootErrorahead of the legacy writer; fires only when the error is a typed*errs.*.What stays legacy
Most production paths emit the legacy
*output.ExitErrorenvelope unchanged:output.ErrAPI/output.ErrAuth/output.ErrValidation/output.ErrNetwork/output.ErrWithHint/output.Errorf/output.ErrBare— all still return*output.ExitErrorand produce the legacy{ok, identity, error: {type, code, message, hint?, detail?}}envelope.errors.As(&output.ExitError{})still matches.APIClient.CheckResponse(used bycmd/api) — still routes Lark API responses throughClassifyLarkErrorand emits the legacy envelope.shortcuts/) hand-constructoutput.ErrAPI(...)— also legacy.BuildAPIErroris implemented and tested but has no production caller yet.User-visible wire / exit changes
Only these change in this PR:
*core.ConfigErrorexit code 2 → 3. Direct construction-site edit ininternal/core/config.go(twoCode: 2sites nowCode: 3). Aligns with the "config errors share the auth exit code" taxonomy.errors.As(&core.ConfigError{})still matches on the unwrap chain.APIClient.DoSDKRequest/DoStream/CallAPInow wrap untyped SDK errors at the boundary. Previously, a rawfmt.Errorfescaping the SDK surfaced as plainError: <msg>(exit 1) at the root dispatcher. It now goes throughWrapDoAPIError/WrapJSONResponseParseError, emitting a JSON envelope:type=network(exit 4) for transport-class failures,type=api_error(exit 1) for JSON-decode failures. Already-classified errors (*output.ExitError,*errs.*) pass through unchanged —output.ErrAuthfrom a missing token still surfaces astype=auth/ exit 3 instead of being downgraded tonetwork.No other production exit code or envelope shape changes in this PR.
Carve-outs
SecurityPolicyErrorkeeps its custom envelope and exit 1.handleRootErrorcheckserrors.As(err, &spErr)first and emits viawriteSecurityPolicyError, producing{type: "auth_error", code: "challenge_required"|"access_denied", message, retryable: false, challenge_url?, hint?}. OAuth / policy consumers depend on this shape. Guardrail test:TestHandleRootError_SecurityPolicyKeepsLegacyEnvelope.output.ErrAPI(...)call sites keep emitting the legacy envelope.Test Plan
make unit-test(77 packages, race + gcflags, 0 FAIL)go build ./...,go vet ./...,gofmt -l .empty,go mod tidy,golangci-lint run --new-from-rev=origin/main0 issues,go-licenses check--yesflag gap + test-account permission gap; not regressions)TestBuildAPIError_ExitCodeMatrix,TestConsoleURL_EscapesDangerousChars,TestHandleRootError_SecurityPolicyKeepsLegacyEnvelope,TestPromoteConfigError_*,TestDoSDKRequest_AuthFailurePreservesAuthCategory,TestTryHandleMCPResponse_*,TestWrapDoAPIError_LegacyExitErrorPassesThroughRelated Issues
N/A