feat(errs): typed envelope contract for auth-domain errors#1135
Open
evandance wants to merge 1 commit into
Open
feat(errs): typed envelope contract for auth-domain errors#1135evandance wants to merge 1 commit into
evandance wants to merge 1 commit into
Conversation
evandance
added
domain/auth
|
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:
📝 WalkthroughWalkthroughMigrates the codebase from legacy output errors to builder-based typed ChangesTyped Error Migration & Classification
Estimated code review effort Possibly related PRs
Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
github-actions
Bot
added
domain/calendar
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@74c145ffc05b4db235e83876ce241147315827fd🧩 Skill updatenpx skills add larksuite/cli#feat/error-contract-auth-migration -y -g |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1135 +/- ##
==========================================
+ Coverage 68.63% 68.81% +0.18%
==========================================
Files 625 626 +1
Lines 58386 58842 +456
==========================================
+ Hits 40071 40491 +420
- Misses 15027 15059 +32
- Partials 3288 3292 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/client/response.go (1)
127-138:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve invalid-argument classification for unsafe output paths.
Both call sites convert all save failures into
InternalError, so an unsafe user--outputpath gets misreported as internal instead of validation/configurable user input failure.💡 Suggested direction
- return nil, fmt.Errorf("unsafe output path: %s", err) + return nil, fmt.Errorf("unsafe output path: %w", err)- return errs.NewInternalError(errs.SubtypeSDKError, "save response: %v", err).WithCause(err) + if errors.Is(err, fileio.ErrPathValidation) { + return errs.NewValidationError(errs.SubtypeInvalidArgument, "unsafe output path: %v", err). + WithParam("--output"). + WithCause(err) + } + return errs.NewInternalError(errs.SubtypeSDKError, "save response: %v", err).WithCause(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 `@internal/client/response.go` around lines 127 - 138, The save failures around SaveResponse and saveAndPrint are always wrapped as InternalError, which hides validation errors (e.g. unsafe user --output paths); change both error-return sites to detect and preserve invalid-argument/validation errors from SaveResponse instead of unconditionally wrapping them: if the returned err has the invalid-argument/validation subtype (use your errs utility—e.g., errs.Is(err, errs.SubtypeInvalidArgument) or equivalent), return that err (optionally .WithCause(err)) unchanged, otherwise wrap as errs.NewInternalError(...). Update the error handling in both the caller that logs "save response: %v" and in saveAndPrint to follow this branching so unsafe output paths are reported as invalid-argument.
🧹 Nitpick comments (3)
lint/errscontract/rule_nil_safe_error.go (1)
103-118: ⚡ Quick winFilter to canonical
Error() stringmethods before validation.
collectMethodsNamedcurrently accepts any method namedError, which can let non-interface-compatible signatures pass this rule’s presence check.Proposed fix
func collectMethodsNamed(file *ast.File, methodName string) map[string]*ast.FuncDecl { out := map[string]*ast.FuncDecl{} for _, decl := range file.Decls { fn, ok := decl.(*ast.FuncDecl) @@ if fn.Name == nil || fn.Name.Name != methodName { continue } + if !isCanonicalErrorMethodSig(fn.Type) { + continue + } recv := receiverTypeName(fn.Recv.List[0].Type) if recv == "" { continue } out[recv] = fn } return out } + +func isCanonicalErrorMethodSig(ft *ast.FuncType) bool { + if ft == nil { + return false + } + if ft.Params != nil && len(ft.Params.List) != 0 { + return false + } + if ft.Results == nil || len(ft.Results.List) != 1 { + return false + } + id, ok := ft.Results.List[0].Type.(*ast.Ident) + return ok && id.Name == "string" +}🤖 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 `@lint/errscontract/rule_nil_safe_error.go` around lines 103 - 118, collectMethodsNamed is currently accepting any method named "Error", which lets non-canonical signatures slip through; update it to only record methods whose signature exactly matches Error() string by checking fn.Type.Params has zero fields and fn.Type.Results has exactly one field whose type is the predeclared string (e.g., a *ast.Ident with Name == "string" or the corresponding AST node), then only add that fn to out; keep using receiverTypeName(fn.Recv.List[0].Type) to obtain the receiver key.cmd/config/config_test.go (2)
128-133: ⚡ Quick winStrengthen this test to assert structured error shape, not only message text.
This currently passes on any auth-coded error containing the phrase. Asserting
*core.ConfigErrorkeeps the contract check stable.Proposed test assertion tightening
- if gotCode := output.ExitCodeOf(err); gotCode != output.ExitAuth { - t.Errorf("exit code = %d, want %d", gotCode, output.ExitAuth) - } - if !strings.Contains(err.Error(), "no active profile") { - t.Fatalf("error = %v, want to contain 'no active profile'", err) - } + if gotCode := output.ExitCodeOf(err); gotCode != output.ExitAuth { + t.Errorf("exit code = %d, want %d", gotCode, output.ExitAuth) + } + var cfgErr *core.ConfigError + if !errors.As(err, &cfgErr) { + t.Fatalf("error type = %T, want *core.ConfigError", err) + } + if cfgErr.Type != "config" { + t.Fatalf("config error type = %q, want %q", cfgErr.Type, "config") + } + if !strings.Contains(cfgErr.Message, "no active profile") { + t.Fatalf("message = %q, want to contain 'no active profile'", cfgErr.Message) + }🤖 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/config/config_test.go` around lines 128 - 133, Replace the loose message check with a structured type assertion: use errors.As to assert err is a *core.ConfigError (declare var cfgErr *core.ConfigError) and then assert the specific error code/field for "no active profile" (e.g., cfgErr.Code == core.ErrNoActiveProfile or cfgErr.Kind == core.NoActiveProfile depending on the ConfigError shape) in addition to the existing exit-code check; add the "errors" import if missing and keep the message containment assertion only as a fallback.
398-400: ⚡ Quick winAvoid false positives by asserting the external-provider reason too.
Exit code alone can pass on unrelated validation failures.
Proposed assertion add-on
+ if err == nil { + t.Fatal("expected external-provider guard error") + } if gotCode := output.ExitCodeOf(err); gotCode != output.ExitValidation { t.Errorf("exit code = %d, want %d", gotCode, output.ExitValidation) } + if !strings.Contains(strings.ToLower(err.Error()), "external") { + t.Errorf("error = %v, want external-provider hint", 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/config/config_test.go` around lines 398 - 400, The test currently only checks exit code via output.ExitCodeOf(err) against output.ExitValidation which can hide unrelated validation failures; add an additional assertion that inspects the validation reason for this error (e.g., call output.ReasonOf(err) or the equivalent helper) and assert it equals the external-provider reason (use the project's constant or string for "external-provider") alongside the exit code check so the test only passes for the intended failure; locate this change around the existing err/exit code assertion that uses output.ExitCodeOf and output.ExitValidation.
🤖 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/login_interactive.go`:
- Around line 157-160: The code maps huh.ErrUserAborted from form1.Run() to
errs.NewValidationError with errs.SubtypeInvalidArgument; instead, treat this as
a user cancellation by returning a confirmation/cancellation-style error.
Replace the errs.NewValidationError(...) call in the form1.Run() error branch
that checks for huh.ErrUserAborted with the appropriate
confirmation/cancellation constructor (e.g., errs.NewConfirmationError or
errs.NewCancelledError) and use a cancellation subtype (e.g.,
errs.SubtypeCancelled or errs.SubtypeUserCancelled) so the abort path preserves
confirmation-style semantics for CLI/agent consumers.
In `@cmd/auth/login_result_test.go`:
- Around line 20-22: The test
TestHandleLoginScopeIssue_FailedJSON_PreservesScopeTriple currently creates the
factory before isolating config state; update the test to call
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) before invoking
cmdutil.TestFactory so the factory uses a temp config dir and cannot read/write
shared config state.
In `@cmd/config/binder.go`:
- Line 200: The error messages that embed "hint: ..." into the Message should
instead set that text via the error envelope's Hint field; locate the
errs.NewInternalError(...) calls in binder.go (the call that returns "keychain
unavailable: %v\nhint: use file: reference in config to bypass keychain" and the
similar calls around the other two sites flagged) and remove the inline "hint:
..." text from the message argument, then chain the appropriate .WithHint("use
file: reference in config to bypass keychain") on each error (and preserve
.WithCause(err) where present) so all three call sites produce a consistent
Message + Hint envelope.
In `@cmd/config/default_as.go`:
- Around line 44-50: The RunE branch returns
errs.NewInternalError/errs.NewValidationError directly instead of wrapping them
with command-layer output helpers; update the code in the RunE handler so that
validation and save failures use output.Errorf or output.ErrWithHint (instead of
returning errs.* directly) when returning errors from this command: replace the
validation error return and the core.SaveMultiAppConfig error return with
corresponding output.Errorf/output.ErrWithHint calls that include the original
error message (and chain the cause where applicable) while still setting
app.DefaultAs before saving; reference symbols: RunE, app.DefaultAs,
core.SaveMultiAppConfig, errs.NewValidationError, errs.NewInternalError.
In `@cmd/config/init_interactive.go`:
- Around line 129-132: The validation currently treats appID and appSecret
together and always attaches WithParam("--app-id"); change the check in
init_interactive.go to detect which field is missing (appID vs appSecret vs
both) and return a specific errs.NewValidationError with an appropriate message
and WithParam value (e.g., "--app-id" for appID, "--app-secret" for appSecret,
or both/neutral when both are empty) so the remediation hint correctly
identifies the missing credential; update the branch that calls
errs.NewValidationError and WithParam to use the correct flag name based on the
empty variable(s).
In `@cmd/config/keychain_downgrade_other.go`:
- Around line 23-25: In the RunE closure in keychain_downgrade_other.go replace
the direct return of errs.NewValidationError(...) with the command-layer wrapper
(e.g. use output.Errorf or output.ErrWithHint) so the error is emitted via the
command stderr contract; locate the anonymous RunE func(cmd *cobra.Command, args
[]string) and convert the call that currently constructs
errs.NewValidationError(errs.SubtypeInvalidArgument, "keychain-downgrade is only
supported on macOS") into an equivalent output.Errorf/output.ErrWithHint
invocation that preserves the same validation subtype and message.
In `@cmd/config/keychain_downgrade.go`:
- Around line 57-60: The returned error from the keychain downgrade flow is a
raw errs.NewInternalError from a RunE-reachable path; change the return to use
output.ErrWithHint (or output.Errorf when appropriate) so command handlers emit
command-contract-compatible stderr. Replace the direct
errs.NewInternalError(...) return in keychain_downgrade.go with
output.ErrWithHint(output.NewError(...)) or
output.ErrWithHint(output.Errorf(...)) that preserves the original message and
hint and wraps the original err (the error produced in this block), referencing
the same descriptive message "keychain downgrade failed" and the WithHint text
so parsers still receive the hint and underlying cause. Ensure the final
returned value is an output.* error type, not errs.*.
In `@cmd/config/remove.go`:
- Line 53: The error uses errs.SubtypeSDKError but this is a local config
persistence failure; update the NewInternalError call in cmd/config/remove.go
(the return that currently calls errs.NewInternalError(errs.SubtypeSDKError,
"...").WithCause(err)) to use an I/O/config-specific subtype such as
errs.SubtypeLocalIO or errs.SubtypeConfigPersist so the failure is not
classified as an SDK error; if that subtype doesn't exist, add a clear subtype
(e.g., SubtypeLocalIO) to the errs subtype definitions and use it in the
NewInternalError call.
In `@cmd/config/strict_mode.go`:
- Around line 83-84: The error returned from the core.SaveMultiAppConfig failure
is wrongly classified as errs.SubtypeSDKError; change the subtype used in the
errs.NewInternalError(...) call(s) (the lines that currently call
errs.NewInternalError(errs.SubtypeSDKError, "failed to save config: %v",
err).WithCause(err)) to a non-SDK subtype that reflects local
persistence/config-save failures (e.g., a local/config/persistence subtype
constant available in errs), and update both occurrences that wrap the
SaveMultiAppConfig error so the subtype accurately represents a local
persistence error rather than an SDK transport/runtime error.
In `@internal/cmdutil/factory.go`:
- Around line 131-136: The code builds a ValidationError and unconditionally
calls WithHint("use --as %s", supported[0]) which will panic if the supported
slice is empty; modify the branch that returns errs.NewValidationError(...) so
it checks len(supported) > 0 before accessing supported[0] and only appends the
WithHint(...) when there is at least one supported element (otherwise omit the
hint or provide a safe generic hint), referencing the existing variables
supported, as, list and the returned errs.NewValidationError call.
In `@internal/errclass/classify.go`:
- Around line 114-117: The code currently sets action := meta.Action then falls
back to cc.LarkCmd but can still be empty; change this so action is guaranteed
non-empty by adding a final fallback or error path: after the existing fallback
check, if action == "" then set action = a constant like
DefaultConfirmationAction (e.g., "confirm") or return an error indicating a
missing confirmation action. Update the block around meta.Action and cc.LarkCmd
(the action variable assignment) to implement this deterministic fallback or
explicit error return so confirmation envelopes never have an empty Action.
In `@internal/errcompat/promote_auth_test.go`:
- Around line 48-52: The test currently calls errors.As(got, &authErr) but
ignores the boolean result and then dereferences authErr; change the test to
assert the result of errors.As returns true before accessing authErr (e.g., if
ok := errors.As(got, &authErr); !ok { t.Fatalf("expected AuthenticationError via
errors.As, got: %T %v", got, got) }) so that promotion is verified and you avoid
a panic when using authErr.UserOpenID.
In `@lint/errscontract/rule_build_api_error_arms.go`:
- Around line 51-71: The current scanner silently returns no violations if the
BuildAPIError function is absent; modify rule_build_api_error_arms.go to detect
a missing BuildAPIError and emit a fail-closed Violation: add a found flag
(e.g., foundBuildAPIError) while iterating file.Decls (where you already match
fn.Name.Name == "BuildAPIError"), set it true when found, and after the loop if
false append a Violation (same structure/message as the existing "has no
Category switch" case) so absence of BuildAPIError triggers a rejection;
reference the existing helpers findCategorySwitch and checkSwitchArms when the
function is present.
In `@shortcuts/common/mcp_client.go`:
- Around line 106-108: The json.Unmarshal error path in mcp_client.go currently
drops the original unmarshal error; update the return so the created
*errs.InternalError preserves the original err as its cause (either by passing
err into errs.NewInternalError if it supports a cause parameter, or by wrapping
the message with fmt.Errorf("%w", err) and supplying that to NewInternalError),
keeping the same message that includes TruncateStr(string(respBody),
mcpErrorBodyLimit); reference the json.Unmarshal call and the respBody/data
variables when making the change.
---
Outside diff comments:
In `@internal/client/response.go`:
- Around line 127-138: The save failures around SaveResponse and saveAndPrint
are always wrapped as InternalError, which hides validation errors (e.g. unsafe
user --output paths); change both error-return sites to detect and preserve
invalid-argument/validation errors from SaveResponse instead of unconditionally
wrapping them: if the returned err has the invalid-argument/validation subtype
(use your errs utility—e.g., errs.Is(err, errs.SubtypeInvalidArgument) or
equivalent), return that err (optionally .WithCause(err)) unchanged, otherwise
wrap as errs.NewInternalError(...). Update the error handling in both the caller
that logs "save response: %v" and in saveAndPrint to follow this branching so
unsafe output paths are reported as invalid-argument.
---
Nitpick comments:
In `@cmd/config/config_test.go`:
- Around line 128-133: Replace the loose message check with a structured type
assertion: use errors.As to assert err is a *core.ConfigError (declare var
cfgErr *core.ConfigError) and then assert the specific error code/field for "no
active profile" (e.g., cfgErr.Code == core.ErrNoActiveProfile or cfgErr.Kind ==
core.NoActiveProfile depending on the ConfigError shape) in addition to the
existing exit-code check; add the "errors" import if missing and keep the
message containment assertion only as a fallback.
- Around line 398-400: The test currently only checks exit code via
output.ExitCodeOf(err) against output.ExitValidation which can hide unrelated
validation failures; add an additional assertion that inspects the validation
reason for this error (e.g., call output.ReasonOf(err) or the equivalent helper)
and assert it equals the external-provider reason (use the project's constant or
string for "external-provider") alongside the exit code check so the test only
passes for the intended failure; locate this change around the existing err/exit
code assertion that uses output.ExitCodeOf and output.ExitValidation.
In `@lint/errscontract/rule_nil_safe_error.go`:
- Around line 103-118: collectMethodsNamed is currently accepting any method
named "Error", which lets non-canonical signatures slip through; update it to
only record methods whose signature exactly matches Error() string by checking
fn.Type.Params has zero fields and fn.Type.Results has exactly one field whose
type is the predeclared string (e.g., a *ast.Ident with Name == "string" or the
corresponding AST node), then only add that fn to out; keep using
receiverTypeName(fn.Recv.List[0].Type) to obtain the receiver key.
🪄 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: 0daec0fd-18e8-4fdc-9eda-f924b4b684ce
📒 Files selected for processing (92)
.golangci.ymlcmd/api/api.gocmd/api/api_test.gocmd/auth/auth.gocmd/auth/auth_test.gocmd/auth/check.gocmd/auth/check_test.gocmd/auth/login.gocmd/auth/login_interactive.gocmd/auth/login_result.gocmd/auth/login_result_test.gocmd/auth/login_test.gocmd/auth/logout.gocmd/auth/qrcode.gocmd/auth/qrcode_test.gocmd/auth/scopes.gocmd/auth/scopes_test.gocmd/config/bind.gocmd/config/bind_test.gocmd/config/binder.gocmd/config/binder_test.gocmd/config/config_test.gocmd/config/default_as.gocmd/config/init.gocmd/config/init_interactive.gocmd/config/init_test.gocmd/config/keychain_downgrade.gocmd/config/keychain_downgrade_other.gocmd/config/remove.gocmd/config/show.gocmd/config/strict_mode.gocmd/error_auth_hint.gocmd/root.gocmd/root_integration_test.gocmd/root_test.gocmd/service/service.goerrs/ERROR_CONTRACT.mderrs/marshal_test.goerrs/predicates.goerrs/subtypes.goerrs/subtypes_service_task.goerrs/types.goerrs/types_test.gointernal/client/api_errors.gointernal/client/api_errors_test.gointernal/client/client.gointernal/client/client_test.gointernal/client/response.gointernal/client/response_test.gointernal/cmdutil/factory.gointernal/cmdutil/factory_test.gointernal/credential/default_provider.gointernal/errclass/classify.gointernal/errclass/classify_internal_test.gointernal/errclass/classify_test.gointernal/errclass/codemeta.gointernal/errclass/codemeta_task.gointernal/errclass/codemeta_test.gointernal/errcompat/promote.gointernal/errcompat/promote_auth.gointernal/errcompat/promote_auth_test.gointernal/errcompat/promote_test.gointernal/output/errors.gointernal/output/errors_test.gointernal/output/lark_errors.gointernal/registry/helpers.golint/errscontract/rule_build_api_error_arms.golint/errscontract/rule_builder_immutable.golint/errscontract/rule_new_invariants_test.golint/errscontract/rule_nil_safe_error.golint/errscontract/rule_unwrap_symmetry.golint/errscontract/runner.golint/errscontract/scan.golint/errscontract/scan_test.goscripts/check-skill-wire-vocab.shshortcuts/calendar/helpers.goshortcuts/common/mcp_client.goshortcuts/common/mcp_client_test.goshortcuts/common/runner.goshortcuts/common/runner_scope_test.goshortcuts/mail/helpers.goshortcuts/mail/mail_confirm_send_scope_test.goshortcuts/vc/vc_notes.goskills/lark-apps/SKILL.mdskills/lark-apps/references/lark-apps-access-scope-get.mdskills/lark-apps/references/lark-apps-access-scope-set.mdskills/lark-apps/references/lark-apps-create.mdskills/lark-apps/references/lark-apps-html-publish.mdskills/lark-apps/references/lark-apps-list.mdskills/lark-apps/references/lark-apps-update.mdskills/lark-slides/references/examples.mdtests/cli_e2e/config/bind_test.go
💤 Files with no reviewable changes (1)
- errs/subtypes_service_task.go
evandance
force-pushed
the
feat/error-contract-auth-migration
branch
2 times, most recently
from
0446c4f to
ab66d43
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/login_result.go`:
- Around line 175-179: The PermissionError created by errs.NewPermissionError is
missing the Identity field for the local "missing_scope" envelope; update the
error builder chain in the returned value to include the identity (e.g., call
the builder's identity setter such as .WithIdentity(issue.Identity) or
equivalent) so the returned PermissionError matches the authorization envelope
shape produced elsewhere—add the .WithIdentity(...) call to the chain after
NewPermissionError and before returning, keeping the existing WithHint,
WithRequestedScopes, WithGrantedScopes, and WithMissingScopes calls.
In `@cmd/config/bind.go`:
- Around line 386-387: The failure returned from vfs.MkdirAll is a local
filesystem I/O error but is currently wrapped with errs.NewInternalError using
errs.SubtypeSDKError; change the classification to the file I/O subtype (e.g.,
replace errs.SubtypeSDKError with errs.SubtypeFileIO) so the envelope correctly
represents a filesystem error, leaving the existing message and WithCause(err)
intact; locate the call to vfs.MkdirAll and the error return that uses
errs.NewInternalError(errs.SubtypeSDKError, ...) to make this change.
- Around line 336-339: The branch that enforces the --force escalation gate
currently returns a validation error via
errs.NewValidationError(errs.SubtypeInvalidArgument, ...) which removes
confirmation semantics; change this to emit a confirmation-style error (e.g.,
use errs.NewConfirmationError or the project's confirmation error constructor)
so the CLI/agent treats it as a user-confirmation requirement, and keep the same
message and hint (IdentityEscalationMessage and IdentityEscalationHint) in the
new error call so the text remains identical.
In `@errs/types.go`:
- Around line 698-714: NewConfirmationRequiredError currently propagates empty
strings for risk/action when callers pass "", so update the constructor
(NewConfirmationRequiredError) to normalize those params to a non-empty default
before building the *ConfirmationRequiredError (set Problem.Category/Subtype
as-is, but ensure Risk and Action fields are replaced with a sensible default
like "unspecified" or "unknown" when the incoming risk or action is ""), e.g.,
compute local vars normalizedRisk and normalizedAction and use those for the
returned ConfirmationRequiredError.Risk and .Action to guarantee the envelope
never contains empty values.
In `@internal/client/api_errors.go`:
- Around line 123-126: The JSON-decode fallback is too broad: the current check
in WrapDoAPIError (the block that inspects err.Error()) uses
strings.Contains(msg, "invalid character") which matches non-JSON
transport/construct errors; narrow it by requiring the fuller JSON-specific
phrase or pattern (e.g., require that "invalid character" appears together with
the JSON parser context like "looking for beginning of value" or match a regexp
such as `invalid character .* looking for beginning of value`) so only real
JSON-unmarshal errors trigger the internal/invalid_response path; update the
conditional in the function that builds msg to use this stricter check (or a
regexp) alongside the existing "unexpected end of JSON input" and "cannot
unmarshal" checks.
In `@internal/errclass/classify.go`:
- Around line 317-320: The permission-denied branch under
errs.SubtypePermissionDenied always returns "this user", ignoring the identity
parameter; update the branch in internal/errclass/classify.go (the
errs.SubtypePermissionDenied case) to use the identity parameter when composing
the hint (e.g., substitute "this user" with the identity value or a mapped
phrase like "this bot"/"this user"), and keep a sensible fallback when identity
is empty so behavior remains unchanged for callers that don't pass identity.
🪄 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: 5e390133-6b3e-4a78-b3bb-6c5d4bde49ed
📒 Files selected for processing (92)
.golangci.ymlcmd/api/api.gocmd/api/api_test.gocmd/auth/auth.gocmd/auth/auth_test.gocmd/auth/check.gocmd/auth/check_test.gocmd/auth/login.gocmd/auth/login_interactive.gocmd/auth/login_result.gocmd/auth/login_result_test.gocmd/auth/login_test.gocmd/auth/logout.gocmd/auth/qrcode.gocmd/auth/qrcode_test.gocmd/auth/scopes.gocmd/auth/scopes_test.gocmd/config/bind.gocmd/config/bind_test.gocmd/config/binder.gocmd/config/binder_test.gocmd/config/config_test.gocmd/config/default_as.gocmd/config/init.gocmd/config/init_interactive.gocmd/config/init_test.gocmd/config/keychain_downgrade.gocmd/config/keychain_downgrade_other.gocmd/config/remove.gocmd/config/show.gocmd/config/strict_mode.gocmd/error_auth_hint.gocmd/root.gocmd/root_integration_test.gocmd/root_test.gocmd/service/service.goerrs/ERROR_CONTRACT.mderrs/marshal_test.goerrs/predicates.goerrs/subtypes.goerrs/subtypes_service_task.goerrs/types.goerrs/types_test.gointernal/client/api_errors.gointernal/client/api_errors_test.gointernal/client/client.gointernal/client/client_test.gointernal/client/response.gointernal/client/response_test.gointernal/cmdutil/factory.gointernal/cmdutil/factory_test.gointernal/credential/default_provider.gointernal/errclass/classify.gointernal/errclass/classify_internal_test.gointernal/errclass/classify_test.gointernal/errclass/codemeta.gointernal/errclass/codemeta_task.gointernal/errclass/codemeta_test.gointernal/errcompat/promote.gointernal/errcompat/promote_auth.gointernal/errcompat/promote_auth_test.gointernal/errcompat/promote_test.gointernal/output/errors.gointernal/output/errors_test.gointernal/output/lark_errors.gointernal/registry/helpers.golint/errscontract/rule_build_api_error_arms.golint/errscontract/rule_builder_immutable.golint/errscontract/rule_new_invariants_test.golint/errscontract/rule_nil_safe_error.golint/errscontract/rule_unwrap_symmetry.golint/errscontract/runner.golint/errscontract/scan.golint/errscontract/scan_test.goscripts/check-skill-wire-vocab.shshortcuts/calendar/helpers.goshortcuts/common/mcp_client.goshortcuts/common/mcp_client_test.goshortcuts/common/runner.goshortcuts/common/runner_scope_test.goshortcuts/mail/helpers.goshortcuts/mail/mail_confirm_send_scope_test.goshortcuts/vc/vc_notes.goskills/lark-apps/SKILL.mdskills/lark-apps/references/lark-apps-access-scope-get.mdskills/lark-apps/references/lark-apps-access-scope-set.mdskills/lark-apps/references/lark-apps-create.mdskills/lark-apps/references/lark-apps-html-publish.mdskills/lark-apps/references/lark-apps-list.mdskills/lark-apps/references/lark-apps-update.mdskills/lark-slides/references/examples.mdtests/cli_e2e/config/bind_test.go
💤 Files with no reviewable changes (1)
- errs/subtypes_service_task.go
✅ Files skipped from review due to trivial changes (8)
- cmd/auth/logout.go
- skills/lark-apps/references/lark-apps-update.md
- skills/lark-apps/references/lark-apps-list.md
- cmd/api/api.go
- skills/lark-apps/references/lark-apps-create.md
- skills/lark-apps/references/lark-apps-access-scope-set.md
- skills/lark-slides/references/examples.md
- errs/ERROR_CONTRACT.md
🚧 Files skipped from review as they are similar to previous changes (66)
- cmd/config/remove.go
- skills/lark-apps/references/lark-apps-access-scope-get.md
- skills/lark-apps/SKILL.md
- internal/client/response_test.go
- internal/errcompat/promote_auth_test.go
- shortcuts/calendar/helpers.go
- shortcuts/mail/mail_confirm_send_scope_test.go
- cmd/config/keychain_downgrade.go
- internal/errcompat/promote_auth.go
- internal/errclass/codemeta_task.go
- shortcuts/common/runner.go
- errs/predicates.go
- scripts/check-skill-wire-vocab.sh
- cmd/config/keychain_downgrade_other.go
- cmd/api/api_test.go
- lint/errscontract/scan.go
- cmd/auth/login_result_test.go
- skills/lark-apps/references/lark-apps-html-publish.md
- cmd/root_integration_test.go
- cmd/auth/scopes.go
- shortcuts/common/mcp_client_test.go
- shortcuts/vc/vc_notes.go
- cmd/config/default_as.go
- lint/errscontract/runner.go
- cmd/auth/scopes_test.go
- lint/errscontract/rule_unwrap_symmetry.go
- tests/cli_e2e/config/bind_test.go
- internal/credential/default_provider.go
- cmd/auth/check_test.go
- cmd/config/strict_mode.go
- shortcuts/mail/helpers.go
- internal/cmdutil/factory_test.go
- cmd/config/binder_test.go
- internal/errclass/codemeta_test.go
- shortcuts/common/runner_scope_test.go
- internal/errclass/classify_internal_test.go
- lint/errscontract/rule_build_api_error_arms.go
- lint/errscontract/scan_test.go
- internal/registry/helpers.go
- internal/client/client.go
- cmd/auth/qrcode_test.go
- internal/errcompat/promote.go
- internal/output/errors.go
- internal/cmdutil/factory.go
- cmd/config/show.go
- cmd/root.go
- internal/client/response.go
- internal/output/errors_test.go
- cmd/error_auth_hint.go
- lint/errscontract/rule_nil_safe_error.go
- .golangci.yml
- cmd/service/service.go
- internal/client/client_test.go
- cmd/auth/login_test.go
- cmd/config/init.go
- internal/output/lark_errors.go
- lint/errscontract/rule_builder_immutable.go
- shortcuts/common/mcp_client.go
- cmd/config/init_test.go
- cmd/root_test.go
- lint/errscontract/rule_new_invariants_test.go
- internal/errcompat/promote_test.go
- errs/types_test.go
- cmd/auth/login.go
- cmd/config/bind_test.go
- cmd/auth/qrcode.go
evandance
force-pushed
the
feat/error-contract-auth-migration
branch
from
ab66d43 to
faef7e1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/auth/logout.go (1)
64-64: ⚡ Quick winUse
SubtypeStoragefor config-save failures, notSubtypeSDKError.
core.SaveMultiAppConfigwrites to the local filesystem.SubtypeSDKErrorimplies an external SDK/service failure, weakening the typed envelope taxonomy.Suggested fix
- return errs.NewInternalError(errs.SubtypeSDKError, "failed to save config: %v", err).WithCause(err) + return errs.NewInternalError(errs.SubtypeStorage, "failed to save config: %v", err).WithCause(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/logout.go` at line 64, The error subtype used when config save fails is incorrect: in cmd/auth/logout.go replace the errs.SubtypeSDKError passed to errs.NewInternalError with errs.SubtypeStorage for failures from core.SaveMultiAppConfig (local filesystem/storage), i.e., update the error construction that currently reads NewInternalError(errs.SubtypeSDKError, "failed to save config: %v", err).WithCause(err) to use errs.SubtypeStorage so the error envelope correctly reflects a storage/config write failure.
🤖 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.
Nitpick comments:
In `@cmd/auth/logout.go`:
- Line 64: The error subtype used when config save fails is incorrect: in
cmd/auth/logout.go replace the errs.SubtypeSDKError passed to
errs.NewInternalError with errs.SubtypeStorage for failures from
core.SaveMultiAppConfig (local filesystem/storage), i.e., update the error
construction that currently reads NewInternalError(errs.SubtypeSDKError, "failed
to save config: %v", err).WithCause(err) to use errs.SubtypeStorage so the error
envelope correctly reflects a storage/config write failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5da103d9-94bb-4ee1-9264-1877a58cd3b4
📒 Files selected for processing (92)
.golangci.ymlcmd/api/api.gocmd/api/api_test.gocmd/auth/auth.gocmd/auth/auth_test.gocmd/auth/check.gocmd/auth/check_test.gocmd/auth/login.gocmd/auth/login_interactive.gocmd/auth/login_result.gocmd/auth/login_result_test.gocmd/auth/login_test.gocmd/auth/logout.gocmd/auth/qrcode.gocmd/auth/qrcode_test.gocmd/auth/scopes.gocmd/auth/scopes_test.gocmd/config/bind.gocmd/config/bind_test.gocmd/config/binder.gocmd/config/binder_test.gocmd/config/config_test.gocmd/config/default_as.gocmd/config/init.gocmd/config/init_interactive.gocmd/config/init_test.gocmd/config/keychain_downgrade.gocmd/config/keychain_downgrade_other.gocmd/config/remove.gocmd/config/show.gocmd/config/strict_mode.gocmd/error_auth_hint.gocmd/root.gocmd/root_integration_test.gocmd/root_test.gocmd/service/service.goerrs/ERROR_CONTRACT.mderrs/marshal_test.goerrs/predicates.goerrs/subtypes.goerrs/subtypes_service_task.goerrs/types.goerrs/types_test.gointernal/client/api_errors.gointernal/client/api_errors_test.gointernal/client/client.gointernal/client/client_test.gointernal/client/response.gointernal/client/response_test.gointernal/cmdutil/factory.gointernal/cmdutil/factory_test.gointernal/credential/default_provider.gointernal/errclass/classify.gointernal/errclass/classify_internal_test.gointernal/errclass/classify_test.gointernal/errclass/codemeta.gointernal/errclass/codemeta_task.gointernal/errclass/codemeta_test.gointernal/errcompat/promote.gointernal/errcompat/promote_auth.gointernal/errcompat/promote_auth_test.gointernal/errcompat/promote_test.gointernal/output/errors.gointernal/output/errors_test.gointernal/output/lark_errors.gointernal/registry/helpers.golint/errscontract/rule_build_api_error_arms.golint/errscontract/rule_builder_immutable.golint/errscontract/rule_new_invariants_test.golint/errscontract/rule_nil_safe_error.golint/errscontract/rule_unwrap_symmetry.golint/errscontract/runner.golint/errscontract/scan.golint/errscontract/scan_test.goscripts/check-skill-wire-vocab.shshortcuts/calendar/helpers.goshortcuts/common/mcp_client.goshortcuts/common/mcp_client_test.goshortcuts/common/runner.goshortcuts/common/runner_scope_test.goshortcuts/mail/helpers.goshortcuts/mail/mail_confirm_send_scope_test.goshortcuts/vc/vc_notes.goskills/lark-apps/SKILL.mdskills/lark-apps/references/lark-apps-access-scope-get.mdskills/lark-apps/references/lark-apps-access-scope-set.mdskills/lark-apps/references/lark-apps-create.mdskills/lark-apps/references/lark-apps-html-publish.mdskills/lark-apps/references/lark-apps-list.mdskills/lark-apps/references/lark-apps-update.mdskills/lark-slides/references/examples.mdtests/cli_e2e/config/bind_test.go
💤 Files with no reviewable changes (1)
- errs/subtypes_service_task.go
✅ Files skipped from review due to trivial changes (8)
- skills/lark-apps/references/lark-apps-create.md
- skills/lark-apps/references/lark-apps-access-scope-get.md
- skills/lark-apps/references/lark-apps-access-scope-set.md
- skills/lark-apps/references/lark-apps-list.md
- skills/lark-slides/references/examples.md
- cmd/api/api_test.go
- cmd/api/api.go
- .golangci.yml
🚧 Files skipped from review as they are similar to previous changes (75)
- internal/credential/default_provider.go
- cmd/config/remove.go
- shortcuts/mail/mail_confirm_send_scope_test.go
- cmd/config/keychain_downgrade_other.go
- cmd/config/keychain_downgrade.go
- cmd/root_integration_test.go
- internal/errcompat/promote_auth.go
- shortcuts/common/mcp_client_test.go
- cmd/auth/login_result_test.go
- skills/lark-apps/SKILL.md
- cmd/auth/check.go
- lint/errscontract/scan.go
- cmd/config/default_as.go
- cmd/config/config_test.go
- internal/errcompat/promote_auth_test.go
- lint/errscontract/runner.go
- skills/lark-apps/references/lark-apps-update.md
- shortcuts/mail/helpers.go
- errs/predicates.go
- skills/lark-apps/references/lark-apps-html-publish.md
- cmd/config/show.go
- internal/registry/helpers.go
- cmd/error_auth_hint.go
- internal/client/response_test.go
- internal/errcompat/promote_test.go
- shortcuts/common/runner_scope_test.go
- internal/errclass/codemeta_test.go
- scripts/check-skill-wire-vocab.sh
- cmd/auth/qrcode_test.go
- lint/errscontract/rule_build_api_error_arms.go
- internal/output/errors_test.go
- cmd/auth/auth.go
- shortcuts/common/runner.go
- shortcuts/calendar/helpers.go
- lint/errscontract/rule_unwrap_symmetry.go
- cmd/auth/qrcode.go
- internal/errclass/classify_internal_test.go
- cmd/auth/scopes.go
- shortcuts/vc/vc_notes.go
- cmd/auth/auth_test.go
- internal/cmdutil/factory_test.go
- cmd/config/strict_mode.go
- cmd/auth/scopes_test.go
- cmd/config/init_test.go
- cmd/auth/login_result.go
- tests/cli_e2e/config/bind_test.go
- internal/errcompat/promote.go
- internal/output/errors.go
- internal/cmdutil/factory.go
- errs/marshal_test.go
- internal/errclass/codemeta.go
- errs/types_test.go
- errs/ERROR_CONTRACT.md
- errs/subtypes.go
- cmd/config/init_interactive.go
- internal/output/lark_errors.go
- internal/client/response.go
- internal/client/api_errors.go
- cmd/auth/login_test.go
- internal/client/client_test.go
- lint/errscontract/rule_nil_safe_error.go
- lint/errscontract/rule_new_invariants_test.go
- cmd/auth/check_test.go
- lint/errscontract/rule_builder_immutable.go
- cmd/root_test.go
- cmd/service/service.go
- cmd/config/binder_test.go
- internal/errclass/classify.go
- cmd/root.go
- shortcuts/common/mcp_client.go
- cmd/config/init.go
- internal/errclass/classify_test.go
- cmd/config/binder.go
- cmd/auth/login.go
- internal/client/api_errors_test.go
evandance
force-pushed
the
feat/error-contract-auth-migration
branch
2 times, most recently
from
d930ef8 to
0c66901
Compare
Every failure on the authentication, authorization, and configuration
path now surfaces as a typed structured error instead of an ad-hoc
envelope. Users and scripts that consume CLI output get:
- a fixed nine-category taxonomy on the wire, each mapped to a
stable shell exit code (authentication/authorization/config = 3,
network = 4, internal = 5, policy = 6, confirmation = 10)
- identity-aware detail fields (missing_scopes, requested_scopes,
granted_scopes, console_url, log_id, retryable, hint) carried
uniformly on the envelope
- a single canonical policy envelope at exit 6; the legacy
auth_error carve-out is retired
- per-subtype canonical message + hint that preserves Lark's
diagnostic phrasing and routes recovery to the right actor:
app developer (app_scope_not_applied), user (missing_scope,
token_scope_insufficient, user_unauthorized), or tenant admin
(app_unavailable, app_disabled)
- wrong app credentials classify as config/invalid_client whether
surfaced by the Open API endpoint (99991543) or the tenant
access-token mint endpoint (10003 / 10014), instead of
collapsing to a transport error or api/unknown
- local shortcut scope preflight emits the same
authorization/missing_scope envelope (identity + deterministic
missing-scope set) used by the post-call permission path, so AI
consumers read the same structured shape from precheck and from
server-returned permission denial
- streaming download/upload failures keep the same network subtype
split (timeout / TLS / DNS / transport) as the non-stream path
instead of collapsing every cause to a generic transport failure
- console_url is carried only on the bot-perspective
app_scope_not_applied envelope (where the recovery action is
"developer applies the scope at the developer console"); the
user-perspective missing_scope envelope drops the field, since
the only actionable user recovery is `lark-cli auth login --scope`
and pointing an end user at a console they cannot modify is
misleading
- bind workflows (Hermes / OpenClaw / lark-channel) flatten dynamic
Type tags to wire 'config' with the original module name kept
as a metric label
All 10 typed errors are cause-bearing, nil-safe on .Error() and
.Unwrap(), and defensively clone slice setter inputs. Four lint
rules (CheckNilSafeError / CheckBuilderImmutable / CheckUnwrapSymmetry
/ CheckBuildAPIErrorArms) lock these invariants on migrated paths.
evandance
force-pushed
the
feat/error-contract-auth-migration
branch
from
0c66901 to
74c145f
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
Every failure on the authentication, authorization, and configuration path now surfaces as a typed structured error on stderr. Scripts and agents that consume CLI output get a fixed taxonomy, stable shell exit codes, and per-subtype recovery hints — instead of having to regex over an ad-hoc envelope whose wording could shift between releases.
Changes
missing_scopes,requested_scopes,granted_scopes,identity,console_url, pluslog_id/retryable/hint— so consumers no longer reach into an opaque detail blob.config bindworkflows (Hermes / OpenClaw / lark-channel) flatten dynamic Type tags onto the wire asconfig, keeping the original module name as a metric label.auth_errorcarve-out..Error()and.Unwrap(), and defensively clone slice setter inputs. Four lint rules lock these invariants on migrated paths.Test Plan
go build ./...go vet ./...gofmt -l errs/ internal/ cmd/ shortcuts/ lint/errs,internal/errclass,internal/errcompat,internal/client,internal/registry,internal/cmdutil,internal/output,cmd/auth,cmd/api,cmd/config,cmd/service,shortcuts/commonlint/errscontract(CheckNilSafeError,CheckBuilderImmutable,CheckUnwrapSymmetry,CheckBuildAPIErrorArms)--scopehint round-trip correctlyRelated Issues
None.
Summary by CodeRabbit
New Features
Bug Fixes
Chores