feat(errs): add structured CLI error contract#984
Open
evandance wants to merge 1 commit into
Open
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 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@569e208a1e0055c3c0d601c804be5af2696177a6🧩 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.72% 67.76% +0.03%
==========================================
Files 575 609 +34
Lines 54360 56133 +1773
==========================================
+ Hits 36817 38040 +1223
- Misses 14496 14912 +416
- Partials 3047 3181 +134 ☔ 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
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/lintcheck/scan_test.go (1)
278-283: 💤 Low valueMisleading comment contradicts fixture content.
The comment states "the const block is empty so no values/names are declared," but the fixture at line 282 actually declares
const SubtypeKnown Subtype = "known". The test likely still passes becauseLoadTypedScopefails for another reason (missing internal/errclass/codemeta.go), but the comment should accurately reflect the test's setup.♻️ Suggested comment fix
- // subtypes.go is present so LoadSubtypeAllowlists succeeds, but the - // const block is empty so no values/names are declared. + // subtypes.go is present so LoadSubtypeAllowlists succeeds. LoadTypedScope + // fails because internal/errclass/codemeta.go is missing from this fixture.🤖 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/scan_test.go` around lines 278 - 283, Update the misleading test comment that claims "the const block is empty so no values/names are declared" to accurately describe the fixture in the scan_test.go snippet: note that the errs/subtypes.go fixture declares a const SubtypeKnown Subtype = "known", so adjust the comment near the errs/subtypes.go entry (in scan_test.go around the LoadTypedScope/LoadSubtypeAllowlists setup) to reflect that SubtypeKnown is declared rather than claiming the const block is empty.
🤖 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_test.go`:
- Around line 150-166: In TestHandleRootError_SecurityPolicyKeepsLegacyEnvelope
before calling cmdutil.TestFactory, isolate the CLI config by calling
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()); this ensures TestFactory (and
its f.IOStreams) uses a per-test config directory and prevents cross-test state
bleed; update the test function
(TestHandleRootError_SecurityPolicyKeepsLegacyEnvelope) to set the env var
immediately at the start of the t.Run body before creating f via
cmdutil.TestFactory.
In `@errs/CONTRACT.md`:
- Around line 168-169: The quick-reference example for errs.InternalError
incorrectly places Subtype at the top level; update the example to show Subtype
as a field inside the Problem object (e.g., errs.InternalError{Problem: ...,
where Problem contains Subtype: SubtypeSDKFailure}) so the shape matches the
contract; locate the example string using the symbol errs.InternalError and
adjust the structure to nest Subtype under Problem accordingly.
---
Nitpick comments:
In `@internal/lintcheck/scan_test.go`:
- Around line 278-283: Update the misleading test comment that claims "the const
block is empty so no values/names are declared" to accurately describe the
fixture in the scan_test.go snippet: note that the errs/subtypes.go fixture
declares a const SubtypeKnown Subtype = "known", so adjust the comment near the
errs/subtypes.go entry (in scan_test.go around the
LoadTypedScope/LoadSubtypeAllowlists setup) to reflect that SubtypeKnown is
declared rather than claiming the const block is empty.
🪄 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: 195e3ed9-43ec-49b4-9d71-ee4650faf445
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (86)
.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/CONTRACT.mderrs/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/envelope.gointernal/output/errors.gointernal/output/errors_test.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_search.goshortcuts/drive/drive_status_test.goshortcuts/mail/mail_shortcut_validation_test.goshortcuts/markdown/helpers.goshortcuts/markdown/markdown_diff_test.goshortcuts/sheets/lark_sheets_sheet_management.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 (3)
- shortcuts/sheets/lark_sheets_sheet_management.go
- internal/output/errors_test.go
- shortcuts/drive/drive_search.go
✅ Files skipped from review due to trivial changes (7)
- go.mod
- errs/internal_carrier.go
- internal/output/envelope.go
- internal/core/errors.go
- errs/doc.go
- internal/core/notconfigured.go
- shortcuts/base/base_execute_test.go
evandance
force-pushed
the
feat/error-contract-framework
branch
from
002d1d2 to
80cce5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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 retry response before deciding to clear token.
After retry, the code updates
code/errStrbut keeps the originalmeta. A retry that returns a policy code is currently treated as generic failure (token cleared,nilreturned), so the caller loses the structuredSecurityPolicyErrorchallenge details.💡 Suggested fix
code = getInt(data, "code", -1) errStr = getStr(data, "error") + meta, metaOK = errclass.LookupCodeMeta(code) + if metaOK && meta.Category == errs.CategoryPolicy { + challengeUrl := getStr(data, "challenge_url") + cliHint := getStr(data, "cli_hint") + msg := getStr(data, "error_description") + return nil, &errs.SecurityPolicyError{ + Problem: errs.Problem{ + Category: errs.CategoryPolicy, + Subtype: meta.Subtype, + Code: code, + Message: msg, + Hint: cliHint, + }, + ChallengeURL: challengeUrl, + } + } if (code != -1 && code != 0) || errStr != "" { 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 {🤖 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 performing the retry (after callEndpoint() and after updating code and errStr via getInt/getStr), recompute the response metadata the same way the original call did (i.e. re-run the logic that set meta and metaOK originally) and use that updated meta to decide whether to clear the token; specifically, if the new meta indicates a SecurityPolicyError (meta.Category == errs.CategoryAuthentication and the structured policy challenge case) then return the structured SecurityPolicyError (don't clear the token), otherwise proceed to clear via RemoveStoredToken(opts.AppId, opts.UserOpenId) and return nil,nil as before. Ensure you reference the same meta/metaOK variables and the callEndpoint()/getInt/getStr results when rebuilding meta so the post-retry decision uses the fresh classification.
🧹 Nitpick comments (2)
internal/lintcheck/typecheck.go (1)
88-91: 💤 Low valueConsider logging package-level errors from
packages.Load.
packages.Loadmay return a non-nilpkgsslice even when individual packages have errors (stored inpkg.Errors). These errors are silently ignored, which could cause subtle issues if the errs package fails to type-check (e.g., syntax error in a new commit). The scope would then be "disabled" with no diagnostic.Consider iterating
pkgsand logging anypkg.Errorsat debug level, or at least checking that the errs package loaded without errors before marking the scope enabled.🤖 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/typecheck.go` around lines 88 - 91, packages.Load can return pkgs with per-package errors in pkg.Errors that are currently ignored; iterate the returned pkgs slice after packages.Load (the pkgs variable) and for each pkg check pkg.Errors and log them (at debug level) via your logger or return an error if the special "errs" package failed to load/type-check before you mark the scope enabled; specifically update the code around packages.Load to loop through pkgs, inspect pkg.Errors, emit debug logs for each error (or bail if the errs package is among the failed packages), and only enable the scope once the errs package is confirmed present with no pkg.Errors.errs/marshal_test.go (1)
63-64: ⚡ Quick winCheck
json.Marshalerrors in tests instead of discarding them.Several cases use
b, _ := json.Marshal(...). If marshal fails, assertions can become misleading rather than fail at the root cause. Please asserterr == nilin each case.Proposed pattern
- b, _ := json.Marshal(ve) + b, err := json.Marshal(ve) + if err != nil { + t.Fatalf("json.Marshal failed: %v", err) + } s := string(b)Also applies to: 78-79, 89-90, 108-109, 122-123, 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, Tests call json.Marshal and discard the error (e.g., b, _ := json.Marshal(ve) followed by s := string(b)); change each occurrence to capture the error (b, err := json.Marshal(ve)) and assert err == nil using the test helper in scope (e.g., require.NoError(t, err) or t.Fatalf) before using s, updating every listed site (lines shown and the other occurrences at 78-79, 89-90, 108-109, 122-123, 147-148, 164-165, 175-176, 193-194, 209-210, 224-225) so that marshal failures fail the test immediately rather than producing misleading assertions.
🤖 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 `@internal/lintcheck/scan.go`:
- Around line 196-199: LoadSubtypeAllowlists currently returns a generic
fmt.Errorf when no subtypes*.go files are found, which prevents ScanRepo from
recognizing the "missing allowlist → skip Rule E" path (it only checks
os.IsNotExist). Change LoadSubtypeAllowlists so that when found == 0 it returns
os.ErrNotExist (or an error wrapped with errors.Is(os.ErrNotExist) true) instead
of fmt.Errorf("no subtypes*.go found under %s", errsDir), so callers like
ScanRepo can detect and skip Rule E; update the return values of
LoadSubtypeAllowlists accordingly and adjust any tests if they expect the old
error string.
---
Outside diff comments:
In `@internal/auth/uat_client.go`:
- Around line 250-267: After performing the retry (after callEndpoint() and
after updating code and errStr via getInt/getStr), recompute the response
metadata the same way the original call did (i.e. re-run the logic that set meta
and metaOK originally) and use that updated meta to decide whether to clear the
token; specifically, if the new meta indicates a SecurityPolicyError
(meta.Category == errs.CategoryAuthentication and the structured policy
challenge case) then return the structured SecurityPolicyError (don't clear the
token), otherwise proceed to clear via RemoveStoredToken(opts.AppId,
opts.UserOpenId) and return nil,nil as before. Ensure you reference the same
meta/metaOK variables and the callEndpoint()/getInt/getStr results when
rebuilding meta so the post-retry decision uses the fresh classification.
---
Nitpick comments:
In `@errs/marshal_test.go`:
- Around line 63-64: Tests call json.Marshal and discard the error (e.g., b, _
:= json.Marshal(ve) followed by s := string(b)); change each occurrence to
capture the error (b, err := json.Marshal(ve)) and assert err == nil using the
test helper in scope (e.g., require.NoError(t, err) or t.Fatalf) before using s,
updating every listed site (lines shown and the other occurrences at 78-79,
89-90, 108-109, 122-123, 147-148, 164-165, 175-176, 193-194, 209-210, 224-225)
so that marshal failures fail the test immediately rather than producing
misleading assertions.
In `@internal/lintcheck/typecheck.go`:
- Around line 88-91: packages.Load can return pkgs with per-package errors in
pkg.Errors that are currently ignored; iterate the returned pkgs slice after
packages.Load (the pkgs variable) and for each pkg check pkg.Errors and log them
(at debug level) via your logger or return an error if the special "errs"
package failed to load/type-check before you mark the scope enabled;
specifically update the code around packages.Load to loop through pkgs, inspect
pkg.Errors, emit debug logs for each error (or bail if the errs package is among
the failed packages), and only enable the scope once the errs package is
confirmed present with no pkg.Errors.
🪄 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: 077bc9f9-fd67-46c8-9f1d-894561916833
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (86)
.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/CONTRACT.mderrs/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/envelope.gointernal/output/errors.gointernal/output/errors_test.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_search.goshortcuts/drive/drive_status_test.goshortcuts/mail/mail_shortcut_validation_test.goshortcuts/markdown/helpers.goshortcuts/markdown/markdown_diff_test.goshortcuts/sheets/lark_sheets_sheet_management.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 (3)
- shortcuts/drive/drive_search.go
- shortcuts/sheets/lark_sheets_sheet_management.go
- internal/output/errors_test.go
✅ Files skipped from review due to trivial changes (5)
- internal/output/envelope.go
- shortcuts/task/task_body_test.go
- errs/internal_carrier.go
- internal/core/errors.go
- errs/doc.go
evandance
force-pushed
the
feat/error-contract-framework
branch
from
80cce5f to
799a12b
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
internal/lintcheck/scan.go (1)
196-199:⚠️ Potential issue | 🟠 Major | ⚡ Quick win"No subtypes found" error is not caught by
os.IsNotExistcheck.
LoadSubtypeAllowlistsreturnsfmt.Errorf("no subtypes*.go found...")when the directory exists but contains no matching files. However,ScanRepo(line 35) only skips Rule E foros.IsNotExist(err). This means a valid errs/ directory without subtypes files will causeScanRepoto fail instead of gracefully skipping Rule E.🐛 Proposed fix: wrap with os.ErrNotExist
if found == 0 { // Treat absence like a missing file — caller silently skips Rule E. - return nil, nil, fmt.Errorf("no subtypes*.go found under %s", errsDir) + return nil, nil, fmt.Errorf("%w: no subtypes*.go found under %s", os.ErrNotExist, errsDir) }This aligns with the comment's intent ("Treat absence like a missing file") and allows
ScanRepoto correctly skip Rule E via itsos.IsNotExist(err)check.🤖 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/scan.go` around lines 196 - 199, LoadSubtypeAllowlists currently returns a plain fmt.Errorf when no subtypes*.go files are found, which ScanRepo only treats as missing when os.IsNotExist(err) is true; modify LoadSubtypeAllowlists so that the "no subtypes*.go found under %s" error wraps os.ErrNotExist (use fmt.Errorf("...: %w", os.ErrNotExist) or errors.Wrap) so callers like ScanRepo and its os.IsNotExist(err) check will recognize and skip Rule E; update the return path in LoadSubtypeAllowlists (referencing errsDir and the existing "no subtypes*.go found" message) to wrap with os.ErrNotExist.
🧹 Nitpick comments (3)
cmd/root_test.go (1)
29-51: ⚡ Quick winAdd
t.Setenvfor config isolation in these tests.These tests use
cmdutil.TestFactorybut don't isolate the config directory. While they may not touch config state in practice, adding isolation prevents future regressions and aligns with the coding guidelines.♻️ Proposed fix
func TestPersistentPreRunE_AuthCheckDisabledAnnotations(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) f, _, _, _ := cmdutil.TestFactory(t, nil)func TestPersistentPreRunE_AuthSubcommands(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) f, _, _, _ := cmdutil.TestFactory(t, nil)func TestPersistentPreRunE_ConfigSubcommands(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) f, _, _, _ := cmdutil.TestFactory(t, nil)As per coding guidelines
**/*_test.go:Use t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) to isolate config state in tests.Also applies to: 53-73
🤖 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_test.go` around lines 29 - 51, Add test-local config isolation by calling t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the start of the tests that call cmdutil.TestFactory; specifically, in TestPersistentPreRunE_AuthCheckDisabledAnnotations (and the similar test covering lines 53-73) set that env var before creating the factory and commands (auth.NewCmdAuth, cmdconfig.NewCmdConfig, schema.NewCmdSchema, api.NewCmdApi) so each test uses an isolated config directory.internal/lintcheck/rules.go (2)
264-264: 💤 Low valueConsider compiling the regex once at package level.
The
adHocregex is compiled on every call toscanSubtype. For large repos with many files, this adds unnecessary overhead.♻️ Proposed optimization
+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_]+$`) var out []Violation emit := func(pos token.Pos, c subtypeClassification) {Then update references from
adHoctoadHocRe.🤖 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 264, The local regexp adHoc := regexp.MustCompile(`^ad_hoc_[a-z0-9_]+$`) in scanSubtype is being compiled on every call; move it to a package-level var (e.g., var adHocRe = regexp.MustCompile(...)) so it is compiled once at init, then replace uses of adHoc in scanSubtype with adHocRe and remove the local compilation; ensure the new var name (adHocRe) is referenced wherever the current adHoc local variable was used.
525-530: 💤 Low value
unquoteSimpledoesn't handle escape sequences.This strips quotes but doesn't process escapes like
\"or\\. If a subtype literal contains escapes, the value won't match the allowlist. Given that subtypes are simple snake_case identifiers, this is likely fine in practice, but worth documenting the limitation.📝 Optional: add a clarifying comment
+// unquoteSimple strips the outer quotes from a string literal without +// processing escape sequences. This is intentionally simple since Subtype +// values are snake_case identifiers that never contain escapes. func unquoteSimple(quoted string) string { if len(quoted) >= 2 && (quoted[0] == '"' || quoted[0] == '`') { return quoted[1 : len(quoted)-1] } return quoted }🤖 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` around lines 525 - 530, The unquoteSimple function currently strips surrounding quotes but ignores escape sequences (e.g., \" or \\), which can cause mismatches; update unquoteSimple to handle common Go string escapes (at least backslash and escaped quote) or, if you prefer to keep it simple, add a clear comment above unquoteSimple documenting this limitation and stating that it does not unescape escape sequences and is only intended for simple snake_case identifiers; reference the function name unquoteSimple in the comment so future readers know where the limitation applies.
🤖 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 `@errs/CONTRACT.md`:
- Line 232: Update the grammar in the line that currently reads "Add a MCP/OAuth
projection adapter if the type maps to one of those" by changing "a MCP" to "an
MCP" so it reads "Add an MCP/OAuth projection adapter if the type maps to one of
those"; locate this exact phrase in the CONTRACT.md content and make the
single-word article replacement.
In `@errs/types.go`:
- Line 16: Each Unwrap() method currently dereferences its receiver and can
panic if the typed pointer is nil; update every Unwrap method (for types
ValidationError, AuthenticationError, NotFoundError, ConflictError,
PermissionError, InternalError) to guard the receiver at the top (if e == nil)
and return nil in that case, otherwise return e.Cause—this prevents panics when
errors.Unwrap calls these methods on a nil typed pointer.
In `@internal/client/api_errors.go`:
- Around line 75-82: The transport detection is too broad: change the url.Error
and syscall.Errno checks to only classify true network transport failures—when
encountering a *url.Error (variable urlErr) unwrap its underlying error
(urlErr.Err or errors.Unwrap(urlErr)) and return true only if that cause is
context.DeadlineExceeded or context.Canceled or implements net.Error (and
indicates a network error); for syscall.Errno (variable syscallErr) replace the
blanket match with an explicit whitelist comparing syscallErr to only
syscall.ECONNREFUSED, syscall.ECONNRESET, syscall.ETIMEDOUT,
syscall.EHOSTUNREACH, and syscall.ENETUNREACH so only those errno values result
in CategoryNetwork.
In `@internal/output/lark_errors.go`:
- Around line 73-97: The legacyHints map is missing entries for refresh-token
error codes used by ClassifyLarkError, causing those cases to return empty
hints; add map entries for the refresh-token codes (e.g., 20026, 20037, 20064,
20073 and retryable 20050) in legacyHints with the same re-auth guidance string
("run: lark-cli auth login to re-authorize" or the canonical re-auth message
used elsewhere) so ClassifyLarkError will return the expected legacy hint for
those error paths.
In `@shortcuts/mail/mail_shortcut_validation_test.go`:
- Around line 14-27: The helper assertValidationError currently rejects legacy
*output.ExitError because it requires errs.IsValidation(err); change the
validation check to accept either errs.IsValidation(err) OR
output.ExitCodeOf(err) == output.ExitValidation (i.e., treat legacy
exit-code-based validation errors as valid). Update the conditional and its
failure message in assertValidationError so it only fails when neither condition
holds, preserving the subsequent exit-code assertion and message checks.
---
Duplicate comments:
In `@internal/lintcheck/scan.go`:
- Around line 196-199: LoadSubtypeAllowlists currently returns a plain
fmt.Errorf when no subtypes*.go files are found, which ScanRepo only treats as
missing when os.IsNotExist(err) is true; modify LoadSubtypeAllowlists so that
the "no subtypes*.go found under %s" error wraps os.ErrNotExist (use
fmt.Errorf("...: %w", os.ErrNotExist) or errors.Wrap) so callers like ScanRepo
and its os.IsNotExist(err) check will recognize and skip Rule E; update the
return path in LoadSubtypeAllowlists (referencing errsDir and the existing "no
subtypes*.go found" message) to wrap with os.ErrNotExist.
---
Nitpick comments:
In `@cmd/root_test.go`:
- Around line 29-51: Add test-local config isolation by calling
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the start of the tests that
call cmdutil.TestFactory; specifically, in
TestPersistentPreRunE_AuthCheckDisabledAnnotations (and the similar test
covering lines 53-73) set that env var before creating the factory and commands
(auth.NewCmdAuth, cmdconfig.NewCmdConfig, schema.NewCmdSchema, api.NewCmdApi) so
each test uses an isolated config directory.
In `@internal/lintcheck/rules.go`:
- Line 264: The local regexp adHoc := regexp.MustCompile(`^ad_hoc_[a-z0-9_]+$`)
in scanSubtype is being compiled on every call; move it to a package-level var
(e.g., var adHocRe = regexp.MustCompile(...)) so it is compiled once at init,
then replace uses of adHoc in scanSubtype with adHocRe and remove the local
compilation; ensure the new var name (adHocRe) is referenced wherever the
current adHoc local variable was used.
- Around line 525-530: The unquoteSimple function currently strips surrounding
quotes but ignores escape sequences (e.g., \" or \\), which can cause
mismatches; update unquoteSimple to handle common Go string escapes (at least
backslash and escaped quote) or, if you prefer to keep it simple, add a clear
comment above unquoteSimple documenting this limitation and stating that it does
not unescape escape sequences and is only intended for simple snake_case
identifiers; reference the function name unquoteSimple in the comment so future
readers know where the limitation applies.
🪄 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: b8b64581-d109-43a6-8f54-aba00cea974f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (94)
.github/workflows/ci.yml.golangci.ymlcmd/api/api.gocmd/api/api_test.gocmd/auth/login_result.gocmd/auth/scopes.gocmd/config/bind_test.gocmd/config/config_test.gocmd/error_auth_hint.gocmd/event/runtime.gocmd/lintcheck/main.gocmd/platform_guards.gocmd/prune.gocmd/root.gocmd/root_integration_test.gocmd/root_test.gocmd/service/service.gocmd/service/service_test.goerrs/CONTRACT.mderrs/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/cmdpolicy/apply.gointernal/cmdutil/confirm.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/hook/install.gointernal/lintcheck/rules.gointernal/lintcheck/rules_test.gointernal/lintcheck/scan.gointernal/lintcheck/scan_test.gointernal/lintcheck/typecheck.gointernal/lintcheck/typecheck_test.gointernal/output/envelope.gointernal/output/errors.gointernal/output/errors_test.gointernal/output/exitcode.gointernal/output/exitcode_test.gointernal/output/lark_errors.goshortcuts/base/base_execute_test.goshortcuts/calendar/calendar_test.goshortcuts/calendar/errors.goshortcuts/common/drive_media_upload.goshortcuts/common/drive_media_upload_test.goshortcuts/common/mcp_client_test.goshortcuts/drive/drive_search.goshortcuts/drive/drive_status_test.goshortcuts/drive/list_remote.goshortcuts/mail/mail_shortcut_validation_test.goshortcuts/markdown/helpers.goshortcuts/markdown/markdown_diff_test.goshortcuts/sheets/lark_sheets_sheet_management.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 (3)
- shortcuts/drive/drive_search.go
- shortcuts/sheets/lark_sheets_sheet_management.go
- internal/output/errors_test.go
✅ Files skipped from review due to trivial changes (14)
- internal/cmdutil/confirm.go
- internal/hook/install.go
- shortcuts/calendar/errors.go
- cmd/prune.go
- internal/core/errors.go
- errs/internal_carrier.go
- shortcuts/drive/list_remote.go
- cmd/auth/login_result.go
- cmd/platform_guards.go
- internal/core/notconfigured.go
- errs/doc.go
- .golangci.yml
- internal/cmdpolicy/apply.go
- shortcuts/base/base_execute_test.go
evandance
force-pushed
the
feat/error-contract-framework
branch
2 times, most recently
from
e1c4e7c to
a789959
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/client/api_errors.go (1)
71-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
*url.Errorcan still be misclassified as transport due to the earliernet.Errormatch.At Line 71,
errors.As(err, &netErr)runs before the Line 79*url.Errorguard. Since*url.Errorsatisfiesnet.Error, non-transport cases (for example invalid/unsupported URL scheme) can still be tagged asCategoryNetwork.Proposed fix
func isTransportError(err error) bool { if err == nil { return false } if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) { return true } - var netErr net.Error - if errors.As(err, &netErr) { - return true - } // *url.Error is too broad on its own — it also wraps things like // "unsupported protocol scheme" (a programmer/config bug, not a // network failure). Treat it as transport only when its wrapped cause // is a context cancel/deadline or itself a net.Error. var urlErr *url.Error if errors.As(err, &urlErr) && urlErr != nil { if errors.Is(urlErr.Err, context.DeadlineExceeded) || errors.Is(urlErr.Err, context.Canceled) { return true } var innerNetErr net.Error if errors.As(urlErr.Err, &innerNetErr) { return true } + return false + } + var netErr net.Error + if errors.As(err, &netErr) { + return true }In Go standard library, does *net/url.Error implement net.Error, and can it wrap non-transport errors like "unsupported protocol scheme"?🤖 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/client/api_errors.go` around lines 71 - 87, The current ordering misclassifies some *url.Error instances as net.Error because errors.As(err, &netErr) runs first; reorder the checks so you first try errors.As(err, &urlErr) (the urlErr handling block that inspects urlErr.Err for context.DeadlineExceeded, context.Canceled, or an inner net.Error) and only if that fails fall back to errors.As(err, &netErr); alternatively, explicitly skip treating url.Error as a plain net.Error by checking for *url.Error (urlErr) before the net.Error branch so invalid/unsupported URL-scheme errors are not labeled CategoryNetwork.
🧹 Nitpick comments (1)
errs/projection/oauth_test.go (1)
80-95: ⚡ Quick winAlso assert empty mapped value when
ok == falseThis case should also verify
got == ""to pin the full fallback contract (consistent with the other negative tests in this file).Proposed patch
func TestOAuthErrorFor_OtherCategoriesNotMapped(t *testing.T) { for _, cat := range []errs.Category{ errs.CategoryConfig, errs.CategoryNetwork, errs.CategoryAPI, errs.CategoryPolicy, errs.CategoryInternal, errs.CategoryConfirmation, } { t.Run(string(cat), func(t *testing.T) { got, ok := OAuthErrorFor(cat, errs.Subtype("anything")) if ok { t.Errorf("ok = true, want false for %q (got %q)", cat, got) } + if got != "" { + t.Errorf("oauthError = %q, want empty for %q", got, cat) + } }) } }🤖 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/projection/oauth_test.go` around lines 80 - 95, Update the test TestOAuthErrorFor_OtherCategoriesNotMapped to also assert that the returned mapped string is empty when ok is false: after calling OAuthErrorFor(cat, errs.Subtype("anything")) and checking ok is false, add an assertion that got == "" (or use t.Errorf/t.Fatalf to report if got is non-empty) so the test fully verifies the fallback contract of OAuthErrorFor returning an empty string when no mapping exists.
🤖 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 `@internal/lintcheck/typecheck.go`:
- Around line 151-157: The doc for ResolveSubtypeIdent is incorrect about the
meaning of the two return booleans; update the comment to match the
implementation of ResolveSubtypeIdent(resolved bool, ok bool): state that
ok=true means the typed scope is available and the function made a definitive
resolution (even if the identifier resolves to a non-errs package or a
non-Subtype constant, in which case resolved=false indicates a definitive
rejection), and ok=false only when the typed scope is disabled (caller should
defer to AST-only resolution). Mention the exact return tuple (resolved, ok) and
the three cases the function distinguishes so callers won't mis-handle the
definitive-rejection branch.
---
Duplicate comments:
In `@internal/client/api_errors.go`:
- Around line 71-87: The current ordering misclassifies some *url.Error
instances as net.Error because errors.As(err, &netErr) runs first; reorder the
checks so you first try errors.As(err, &urlErr) (the urlErr handling block that
inspects urlErr.Err for context.DeadlineExceeded, context.Canceled, or an inner
net.Error) and only if that fails fall back to errors.As(err, &netErr);
alternatively, explicitly skip treating url.Error as a plain net.Error by
checking for *url.Error (urlErr) before the net.Error branch so
invalid/unsupported URL-scheme errors are not labeled CategoryNetwork.
---
Nitpick comments:
In `@errs/projection/oauth_test.go`:
- Around line 80-95: Update the test TestOAuthErrorFor_OtherCategoriesNotMapped
to also assert that the returned mapped string is empty when ok is false: after
calling OAuthErrorFor(cat, errs.Subtype("anything")) and checking ok is false,
add an assertion that got == "" (or use t.Errorf/t.Fatalf to report if got is
non-empty) so the test fully verifies the fallback contract of OAuthErrorFor
returning an empty string when no mapping exists.
🪄 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: 7c5c37f9-2848-4521-a49e-2941e0eee498
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (94)
.github/workflows/ci.yml.golangci.ymlcmd/api/api.gocmd/api/api_test.gocmd/auth/login_result.gocmd/auth/scopes.gocmd/config/bind_test.gocmd/config/config_test.gocmd/error_auth_hint.gocmd/event/runtime.gocmd/lintcheck/main.gocmd/platform_guards.gocmd/prune.gocmd/root.gocmd/root_integration_test.gocmd/root_test.gocmd/service/service.gocmd/service/service_test.goerrs/CONTRACT.mderrs/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/cmdpolicy/apply.gointernal/cmdutil/confirm.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/hook/install.gointernal/lintcheck/rules.gointernal/lintcheck/rules_test.gointernal/lintcheck/scan.gointernal/lintcheck/scan_test.gointernal/lintcheck/typecheck.gointernal/lintcheck/typecheck_test.gointernal/output/envelope.gointernal/output/errors.gointernal/output/errors_test.gointernal/output/exitcode.gointernal/output/exitcode_test.gointernal/output/lark_errors.goshortcuts/base/base_execute_test.goshortcuts/calendar/calendar_test.goshortcuts/calendar/errors.goshortcuts/common/drive_media_upload.goshortcuts/common/drive_media_upload_test.goshortcuts/common/mcp_client_test.goshortcuts/drive/drive_search.goshortcuts/drive/drive_status_test.goshortcuts/drive/list_remote.goshortcuts/mail/mail_shortcut_validation_test.goshortcuts/markdown/helpers.goshortcuts/markdown/markdown_diff_test.goshortcuts/sheets/lark_sheets_sheet_management.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 (3)
- shortcuts/drive/drive_search.go
- internal/output/errors_test.go
- shortcuts/sheets/lark_sheets_sheet_management.go
✅ Files skipped from review due to trivial changes (13)
- errs/category_test.go
- internal/cmdutil/confirm.go
- cmd/auth/login_result.go
- shortcuts/base/base_execute_test.go
- shortcuts/drive/list_remote.go
- internal/core/errors.go
- cmd/prune.go
- errs/internal_carrier.go
- cmd/platform_guards.go
- shortcuts/calendar/errors.go
- internal/hook/install.go
- errs/doc.go
- internal/output/envelope.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@internal/output/errors.go`:
- Around line 241-242: WriteTypedErrorEnvelope is returning true regardless of
errors from buf.WriteTo(w); change it to check the error returned by
buf.WriteTo(w) (using the existing buf and w variables) and return false when
buf.WriteTo returns a non-nil error so callers don’t assume the envelope was
written successfully; ensure the function still returns true only when
buf.WriteTo succeeds and any write error is propagated to the caller via the
boolean result.
🪄 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: ec481368-d2f8-45b1-91bb-9729e633c240
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (94)
.github/workflows/ci.yml.golangci.ymlcmd/api/api.gocmd/api/api_test.gocmd/auth/login_result.gocmd/auth/scopes.gocmd/config/bind_test.gocmd/config/config_test.gocmd/error_auth_hint.gocmd/event/runtime.gocmd/lintcheck/main.gocmd/platform_guards.gocmd/prune.gocmd/root.gocmd/root_integration_test.gocmd/root_test.gocmd/service/service.gocmd/service/service_test.goerrs/ERROR_CONTRACT.mderrs/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/cmdpolicy/apply.gointernal/cmdutil/confirm.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/hook/install.gointernal/lintcheck/rules.gointernal/lintcheck/rules_test.gointernal/lintcheck/scan.gointernal/lintcheck/scan_test.gointernal/lintcheck/typecheck.gointernal/lintcheck/typecheck_test.gointernal/output/envelope.gointernal/output/errors.gointernal/output/errors_test.gointernal/output/exitcode.gointernal/output/exitcode_test.gointernal/output/lark_errors.goshortcuts/base/base_execute_test.goshortcuts/calendar/calendar_test.goshortcuts/calendar/errors.goshortcuts/common/drive_media_upload.goshortcuts/common/drive_media_upload_test.goshortcuts/common/mcp_client_test.goshortcuts/drive/drive_search.goshortcuts/drive/drive_status_test.goshortcuts/drive/list_remote.goshortcuts/mail/mail_shortcut_validation_test.goshortcuts/markdown/helpers.goshortcuts/markdown/markdown_diff_test.goshortcuts/sheets/lark_sheets_sheet_management.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 (3)
- shortcuts/sheets/lark_sheets_sheet_management.go
- shortcuts/drive/drive_search.go
- internal/output/errors_test.go
✅ Files skipped from review due to trivial changes (14)
- shortcuts/calendar/errors.go
- cmd/auth/login_result.go
- errs/category.go
- cmd/prune.go
- internal/cmdpolicy/apply.go
- internal/core/errors.go
- go.mod
- shortcuts/base/base_execute_test.go
- internal/hook/install.go
- shortcuts/drive/list_remote.go
- cmd/platform_guards.go
- internal/output/envelope.go
- shortcuts/task/task_body_test.go
- errs/doc.go
evandance
force-pushed
the
feat/error-contract-framework
branch
8 times, most recently
from
42c94f9 to
750d291
Compare
evandance
force-pushed
the
feat/error-contract-framework
branch
10 times, most recently
from
8476062 to
136aad7
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
136aad7 to
569e208
Compare
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
1 participant
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 so future PRs can replace
output.ErrAPI(code, ...)call sites with&errs.PermissionError{...}etc. Go callers will branch viaerrors.As(&errs.XxxError{}); shell scripts, AI agents, and protocol adapters will 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.
SecurityPolicyErrortaxonomy migration is PR 2; per-domain shortcut migration (task / drive / calendar / im / mail / whiteboard / contact) is PRs 3+.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.*.internal/lintcheck/,.golangci.yml): A — forbidoutput.ErrAPI/output.Errorfinshortcuts/*andcmd/service/*; B — every exported*Errorstruct embedserrs.Problem; C — no service-sidemergeCodeMeta/ registrar pattern (codemeta is centrally owned); D —Subtype: "ad_hoc_*"literals get a governance label (not rejected); E —Subtypevalue must be a declared constant or matchad_hoc_*.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 — per-domain migration in PRs 3+ flipsCheckResponseand call sites to use it.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 documented "config errors share the auth exit code" contract.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; taxonomy migration is PR 2. Guardrail test:TestHandleRootError_SecurityPolicyKeepsLegacyEnvelope.output.ErrAPI(...)call sites keep emitting the legacy envelope; per-domain migration is PRs 3+.Test Plan
make unit-test(77 packages, race + gcflags, 0 FAIL)go build ./...,go vet ./...,gofmt -l .empty,go mod tidy,go run ./cmd/lintcheck .exit 0,golangci-lint run --new-from-rev=origin/main0 issues,go-licenses check--yesflag gap + test-account permission gap; not regressions)docs/superpowers/verify_results/acceptance_review_v2.md)TestBuildAPIError_ExitCodeMatrix,TestConsoleURL_EscapesDangerousChars,TestHandleRootError_SecurityPolicyKeepsLegacyEnvelope,TestPromoteConfigError_*,TestDoSDKRequest_AuthFailurePreservesAuthCategory,TestTryHandleMCPResponse_*,TestWrapDoAPIError_LegacyExitErrorPassesThroughRelated Issues
N/A — first PR in the
feat/error-contract-*series.