-
Notifications
You must be signed in to change notification settings - Fork 961
refactor: retire legacy error envelopes and enforce typed contract#1449
Open
evandance wants to merge 1 commit into
Open
refactor: retire legacy error envelopes and enforce typed contract #1449evandance wants to merge 1 commit into
evandance wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughRepo-wide migration replacing legacy internal/output envelope helpers with typed github.com/larksuite/cli/errs, adding raw/bare primitives, updating callers (API/upload/runtime), tests, docs, and repo lint rules. ChangesTyped error migration across CLI and internals
Estimated code review effort: 🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested reviewers
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
@github-actions
github-actions
Bot
added
domain/base
PR touches the base domain
domain/calendar
PR touches the calendar domain
domain/ccm
PR touches the ccm domain
domain/task
PR touches the task domain
domain/vc
PR touches the vc domain
size/XL
Architecture-level or global-impact change
labels
Jun 13, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@ ## main #1449 +/- ## ========================================== + Coverage 73.32% 73.50% +0.17% ========================================== Files 750 747 -3 Lines 69240 68799 -441 ========================================== - Hits 50769 50569 -200 + Misses 14733 14523 -210 + Partials 3738 3707 -31 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide
🧰 CLI update
npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@45923b8f49cc5fd0cc88f39d3aefb12dae268b72
🧩 Skill update
npx skills add larksuite/cli#feat/errs-legacy-final-cleanup -y -g
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 15
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
internal/client/api_errors_test.go (1)
266-282:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert typed metadata on the untyped fallback path.
This test verifies type routing, but it doesn’t assert the typed problem metadata (
category/subtype) for the fallback branch. Adderrs.ProblemOf(got)checks to pinnetwork+network_transport.Suggested test hardening
func TestWrapDoAPIError_UntypedErrorRoutesToNetwork(t *testing.T) { got := WrapDoAPIError(errors.New("no access token available for user")) var ne *errs.NetworkError if !errors.As(got, &ne) { t.Fatalf("expected *errs.NetworkError for an untyped error, got %T (%v)", got, got) } + p, ok := errs.ProblemOf(got) + if !ok { + t.Fatalf("expected typed problem metadata, got %T (%v)", got, got) + } + if p.Category != errs.CategoryNetwork || p.Subtype != errs.SubtypeNetworkTransport { + t.Fatalf("category/subtype = %q/%q, want network/network_transport", p.Category, p.Subtype) + } // Sanity: not silently re-classified as JSON-decode. var ie *errs.InternalError if errors.As(got, &ie) { t.Fatalf("expected NetworkError, got InternalError %v", ie) } }As per coding guidelines:
**/*_test.go: Error-path tests must assert typed metadata viaerrs.ProblemOf(category/subtype/param) and cause preservation, not message substrings alone.🤖 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_test.go` around lines 266 - 282, Update the TestWrapDoAPIError_UntypedErrorRoutesToNetwork test to assert the typed problem metadata on the fallback NetworkError: after asserting errors.As to *errs.NetworkError, call p := errs.ProblemOf(got) and add assertions that p.Category == "network" and p.Subtype == "network_transport" (and any expected params if applicable) to pin the fallback branch; keep the existing checks that it is not an InternalError and preserve the original error as the cause.Source: Coding guidelines
internal/core/notconfigured_test.go (1)
167-200:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
TestLoadOrNotConfigured_CorruptFile_PreservesCausedoes not verify cause-chain preservation.The test name and comment promise cause preservation, but no assertion currently checks that the wrapped underlying error is retained. Add an explicit
errors.Unwrap/errors.Ascause check.Suggested assertion add-on
if strings.Contains(cfgErr.Hint, "config init") || strings.Contains(cfgErr.Hint, "config bind") { t.Errorf("corrupt-file hint must not redirect to init/bind; got %q", cfgErr.Hint) } + if errors.Unwrap(err) == nil { + t.Errorf("expected corrupt-config error to preserve underlying cause") + } }As per coding guidelines:
**/*_test.go: Error-path tests must assert typed metadata viaerrs.ProblemOf(category/subtype/param) and cause preservation, not message substrings alone.🤖 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/core/notconfigured_test.go` around lines 167 - 200, The test TestLoadOrNotConfigured_CorruptFile_PreservesCause should assert that the original parsing error is preserved and that typed metadata is exposed via errs.ProblemOf: after calling LoadOrNotConfigured() and asserting the returned error is a *errs.ConfigError, add an errors.As or errors.Unwrap check to capture the underlying error (e.g. var parseErr *json.SyntaxError or var cause error and use errors.As/unpack) and assert it's non-nil and matches the expected parse/syntax error; also call errs.ProblemOf(err) to assert category/subtype/params (or at least that Subtype == errs.SubtypeInvalidConfig) instead of relying solely on message substrings so the test verifies cause-chain preservation and typed metadata.Source: Coding guidelines
cmd/config/bind_test.go (1)
32-63:⚠️ Potential issue | 🟠 MajorStrengthen
assertExitErrorto assert typed metadata and preserve causes
cmd/config/bind_test.go’sassertExitError(andwantErrDetail) only comparesCategory/Message/Hintfrom*errs.ValidationError/*errs.ConfigError; it doesn’t asserterrs.ProblemOftyped fields (subtype/param) and doesn’t check cause preservation (errors.Unwrap/.WithCausechain). Extend the shared helper to validate those typed metadata and unwrapping/cause assertions so regressions are caught across all error-path tests using it.🤖 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/bind_test.go` around lines 32 - 63, Update assertExitError and the wantErrDetail type to also assert ProblemOf typed metadata (subtype and param) and to verify cause preservation: after detecting a *errs.ValidationError or *errs.ConfigError, extract and compare the ProblemOf fields (e.g., ve.ProblemOf.Subtype / ve.ProblemOf.Param and ce.ProblemOf.Subtype / ce.ProblemOf.Param) against wantErrDetail.Subtype and wantErrDetail.Param, and assert the error's cause chain preserves the original cause by checking errors.Unwrap/errors.Is (or walking errors.Unwrap until nil) matches wantErrDetail.Cause (or its message) — apply the same checks in both the ValidationError and ConfigError branches and update any tests to populate the new wantErrDetail fields.Source: Coding guidelines
lint/errscontract/rule_no_legacy_runtime_api_call.go (1)
42-44:⚠️ Potential issue | 🟠 MajorFix receiver matching in
no_legacy_runtime_api_callto avoid false positivesIn
lint/errscontract/rule_no_legacy_runtime_api_call.go, the rule gates only on whether the file importsgithub.com/larksuite/cli/shortcuts/common(L42-44), then rejects any selector call whose method name isCallAPI/DoAPIJSON/DoAPIJSONWithLogIDwith no receiver-type check (L55-63;matchLegacyRuntimeAPIMethodis name-only). This can still reject valid code that importsshortcuts/commonfor unrelated reasons but calls a same-named method on a non-common.RuntimeContextreceiver. Existing tests cover*common.RuntimeContextreceivers and exceptions (typed wrapper / RawAPI), but not this non-runtime-context receiver case.Add a regression test for "imports common + non-
common.RuntimeContextreceiver withCallAPI=> no violation" and narrow the matcher to only fire when the selector receiver is proven to becommon.RuntimeContextor*common.RuntimeContext.🤖 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_no_legacy_runtime_api_call.go` around lines 42 - 44, The rule currently only checks importsPath(...) and uses matchLegacyRuntimeAPIMethod (name-only) which causes false positives; update the matcher to also verify the selector's receiver type is exactly common.RuntimeContext or *common.RuntimeContext (i.e., inspect the selector expression's type info / AST to confirm the named/ptr type refers to the common package's RuntimeContext) and only then flag method names CallAPI, DoAPIJSON, DoAPIJSONWithLogID; add a regression test that imports github.com/larksuite/cli/shortcuts/common but calls CallAPI on a different receiver type (non-common.RuntimeContext) and assert no violation; keep existing exceptions (typed wrapper / RawAPI) unchanged.
🟡 Minor comments (9)
internal/core/notconfigured.go-45-49 (1)
45-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
.WithCause(err)— underlying error not preserved forerrors.Is/errors.Unwrap.The original
erris formatted into the message but not attached as a cause. Per coding guidelines, underlying errors should be preserved with.WithCause(err)so callers can useerrors.Is/errors.Unwrapfor programmatic error inspection.Proposed fix
- return nil, errs.NewConfigError(subtype, "failed to load config: %v", err) + return nil, errs.NewConfigError(subtype, "failed to load 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 `@internal/core/notconfigured.go` around lines 45 - 49, The returned config error currently formats the original err into the message but does not attach it as a cause; update the return in the function that sets subtype (using isMalformedConfigError) so that the Errs value produced by errs.NewConfigError(subtype, "failed to load config: %v", err) is followed by .WithCause(err) before returning (i.e., return nil, errs.NewConfigError(...).WithCause(err)) so callers can use errors.Is/errors.Unwrap to inspect the underlying error.Source: Coding guidelines
internal/keychain/keychain.go-52-54 (1)
52-54:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider
errs.NewInternalErrorinstead oferrs.NewAPIErrorfor keychain failures.Keychain operations are local infrastructure, not Lark API calls. Per the error contract,
errs.NewAPIErroris intended for Lark API failures, whileerrs.NewInternalError(errs.SubtypeUnknown, ...)is the canonical choice for unclassified lower-layer errors. Both produce exit code 1, but the semantic category differs —internalbetter describes local credential-store failures.Proposed fix
- return errs.NewAPIError(errs.SubtypeUnknown, "%s", msg). + return errs.NewInternalError(errs.SubtypeUnknown, "%s", msg). WithHint("%s", hint). 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/keychain/keychain.go` around lines 52 - 54, Replace the API error construction in the keychain failure path: instead of calling errs.NewAPIError(...) use errs.NewInternalError(errs.SubtypeUnknown, "%s", msg) and then chain .WithHint("%s", hint).WithCause(err) so the returned error uses the internal error category for local keychain/credential-store failures (replace the existing errs.NewAPIError(...) expression in internal/keychain/keychain.go).Source: Coding guidelines
cmd/completion/completion.go-34-36 (1)
34-36:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
WithParamto the unsupported shell validation error.This path validates user input but does not set
param, so the typed envelope loses which argument failed.Suggested patch
- return errs.NewValidationError(errs.SubtypeInvalidArgument, - "unsupported shell: %s", args[0]). - WithHint("supported shells: bash, zsh, fish, powershell") + return errs.NewValidationError(errs.SubtypeInvalidArgument, + "unsupported shell: %s", args[0]). + WithParam("<shell>"). + WithHint("supported shells: bash, zsh, fish, powershell")As per coding guidelines: "Use
errs.NewValidationError(errs.SubtypeInvalidArgument, ...)with.WithParam(...)for user flag/argument validation failures", and "paramfield in errors should only name the user input that actually failed."🤖 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/completion/completion.go` around lines 34 - 36, The validation error created by errs.NewValidationError for unsupported shells (the call that currently uses errs.NewValidationError(..., "unsupported shell: %s", args[0]).WithHint(...)) must include the parameter name of the failing user input; update that error chain to call .WithParam("shell") (e.g., errs.NewValidationError(..., "unsupported shell: %s", args[0]).WithParam("shell").WithHint(...)) so the error envelope records which argument failed.Source: Coding guidelines
cmd/config/config_test.go-418-427 (1)
418-427:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert cause preservation in the wrapper regression test
At Line 425, this test validates subtype conversion but not cause chaining; it could still pass if
.WithCause(err)is accidentally removed later.Suggested patch
func TestWrapSaveConfigError_PassesTypedValidationThrough(t *testing.T) { conflict := errs.NewValidationError(errs.SubtypeInvalidArgument, "name conflict").WithParam("--name") var verr *errs.ValidationError if !errors.As(wrapSaveConfigError(conflict), &verr) { t.Fatalf("typed validation must pass through unchanged, got %T", wrapSaveConfigError(conflict)) } + base := errors.New("disk full") var ierr *errs.InternalError - if !errors.As(wrapSaveConfigError(errors.New("disk full")), &ierr) || ierr.Subtype != errs.SubtypeStorage { + got := wrapSaveConfigError(base) + if !errors.As(got, &ierr) || ierr.Subtype != errs.SubtypeStorage { t.Fatalf("untyped failure must become internal/storage") } + if !errors.Is(got, base) { + t.Fatalf("wrapped error must preserve cause chain") + } }As per coding guidelines, error-path tests should assert typed metadata and cause preservation rather than relying on surface behavior only.
🤖 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 418 - 427, The test TestWrapSaveConfigError_PassesTypedValidationThrough currently only checks type/subtype but not that the original cause is preserved; update the test to capture the wrapped error (e.g., wrapped := wrapSaveConfigError(conflict)) and assert that the original error is preserved via errors.Is or errors.Unwrap (for example: if !errors.Is(wrapped, conflict) { t.Fatalf("expected cause to be preserved") }); do the same for the untyped case by creating the original err (e.g., orig := errors.New("disk full")), wrapping it, and asserting errors.Is(wrapped, orig) or that errors.Unwrap(wrapped) == orig while keeping the existing type/subtype assertions for wrapSaveConfigError.Source: Coding guidelines
errs/ERROR_CONTRACT.md-556-568 (1)
556-568:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMigration status text is internally inconsistent with earlier sections.
This section states migration is complete, but earlier parts of the same document still describe staged migration / unmigrated legacy paths. Please align the intro and invariant text so the contract has one authoritative status.
🤖 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/ERROR_CONTRACT.md` around lines 556 - 568, The "Migration to the typed contract is complete" statement conflicts with earlier sections that describe a staged migration and unmigrated legacy paths; choose a single authoritative status and make the text consistent: either change the intro/invariant paragraph to describe a staged migration and list remaining legacy paths (and keep references to legacy constructors like Errorf, ErrAPI, ErrAuth, ErrWithHint, ClassifyLarkError, ErrDetail, ExitError, ErrorEnvelope as "removed only in migrated modules") or update the earlier sections to state migration is complete and remove any references to staged/unmigrated paths. Also ensure the note about output.ErrBare (and the new *output.BareError type) is worded consistently with the chosen status and references the typed contract builders (errs.NewXxxError(...).WithX(...)) so the document presents one clear, non-contradictory migration status.cmd/root.go-271-275 (1)
271-275:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve the original Cobra error as cause when creating
depErr.This path converts an untyped Cobra error into a typed validation error, but it currently drops the original
errobject. That breakserrors.Is/Astraversal for downstream handlers and tests.Suggested fix
- depErr := errs.NewValidationError(errs.SubtypeInvalidArgument, "%s", err.Error()) + depErr := errs.NewValidationError(errs.SubtypeInvalidArgument, "%s", err.Error()). + WithCause(err)As per coding guidelines, preserve underlying errors with
.WithCause(err)soerrors.Is/errors.Unwrapcontinue working.🤖 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 271 - 275, The code builds a typed validation error but drops the original Cobra error, breaking errors.Is/As; update the creation of depErr so it preserves the original err as its cause (use errs.NewValidationError(...).WithCause(err) or the library's equivalent) before passing depErr to output.WriteTypedErrorEnvelope and output.ExitCodeOf so error unwrapping still works for downstream handlers and tests.Source: Coding guidelines
shortcuts/common/validate_test.go-86-89 (1)
86-89:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert typed metadata on the negative flag-group branches.
The error branches here only check
err != nil, so typed-envelope regressions (category/subtype/param) can slip through undetected.✅ Suggested test-shape update
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { rt := newTestRuntime(tt.flags) err := MutuallyExclusiveTyped(rt, tt.check...) - if (err != nil) != tt.wantErr { - t.Errorf("MutuallyExclusiveTyped() error = %v, wantErr %v", err, tt.wantErr) - } + if tt.wantErr { + ve := assertValidationParam(t, err, "") + if p, ok := errs.ProblemOf(ve); !ok || p.Category != errs.CategoryValidation { + t.Fatalf("expected typed validation category, got %#v (ok=%v)", p, ok) + } + return + } + if err != nil { + t.Errorf("MutuallyExclusiveTyped() unexpected error = %v", err) + } }) }As per coding guidelines:
**/*_test.gorequires error-path tests to assert typed metadata, not just error presence.Also applies to: 241-244, 278-281
🤖 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/common/validate_test.go` around lines 86 - 89, The test only asserts presence/absence of err for MutuallyExclusiveTyped table cases, which lets regressions in the typed error envelope pass; update the negative branches to assert the error's typed metadata (e.g., its category/subtype/param) rather than just non-nil. Concretely, in the MutuallyExclusiveTyped test cases where tt.wantErr is true, extract and assert the concrete typed error (using errors.As or the project's helper for unwrapping typed errors) and compare its category/subtype/param fields to the expected values from the test case; apply the same pattern to the other failing test sites that follow the same (err != nil) checks so they assert the typed metadata as well.Source: Coding guidelines
shortcuts/register_brand_guard_test.go-74-83 (1)
74-83:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStrengthen these error-path assertions with
errs.ProblemOfmetadata checks.Line 74 and Line 116 now assert typed class correctly, but the changed checks still lean on message substrings. Please also assert
category/subtypeviaerrs.ProblemOfto lock the typed contract shape.Suggested test assertion update
- var validationErr *errs.ValidationError - if !errors.As(err, &validationErr) { - t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err) - } - if validationErr.Subtype != errs.SubtypeFailedPrecondition { - t.Errorf("expected subtype %q, got %q", errs.SubtypeFailedPrecondition, validationErr.Subtype) - } + var validationErr *errs.ValidationError + if !errors.As(err, &validationErr) { + t.Fatalf("expected *errs.ValidationError, got %T: %v", err, err) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem == nil { + t.Fatalf("expected typed problem metadata, got %T: %v", err, err) + } + if problem.Category != errs.CategoryValidation { + t.Errorf("expected category %q, got %q", errs.CategoryValidation, problem.Category) + } + if problem.Subtype != errs.SubtypeFailedPrecondition { + t.Errorf("expected subtype %q, got %q", errs.SubtypeFailedPrecondition, problem.Subtype) + } @@ - var validationErr *errs.ValidationError - if !errors.As(err, &validationErr) { - t.Fatalf("expected *errs.ValidationError from cobra dispatch, got %T: %v", err, err) - } + var validationErr *errs.ValidationError + if !errors.As(err, &validationErr) { + t.Fatalf("expected *errs.ValidationError from cobra dispatch, got %T: %v", err, err) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem == nil { + t.Fatalf("expected typed problem metadata, got %T: %v", err, err) + } + if problem.Category != errs.CategoryValidation || problem.Subtype != errs.SubtypeFailedPrecondition { + t.Errorf("expected validation/failed_precondition, got %s/%s", problem.Category, problem.Subtype) + }As per coding guidelines,
**/*_test.goerror-path tests should assert typed metadata viaerrs.ProblemOf(category/subtype/param) instead of message substrings alone.Also applies to: 116-122
🤖 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/register_brand_guard_test.go` around lines 74 - 83, The test currently asserts the error is an *errs.ValidationError and checks substrings in validationErr.Error(); instead, use errs.ProblemOf to assert the structured metadata (category, subtype, and any param keys) so the contract is locked. Update the assertions around validationErr (the variable created by errors.As) to call errs.ProblemOf(validationErr) and assert Problem.Category (or Category string), Problem.Subtype equals errs.SubtypeFailedPrecondition, and that the Problem.Params include the expected keys/values (e.g., "apps" and "lark") rather than relying on message substring checks; apply the same replacement for the similar checks at the second site (the assertions currently at lines ~116-122).Source: Coding guidelines
shortcuts/sheets/flag_schema_test.go-194-197 (1)
194-197:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert typed metadata, not just concrete error type.
Line 194 currently checks only
*errs.ValidationError. Please also assert typed metadata (errs.ProblemOffor subtype/category) andve.Paramso this error-path test locks the full contract.Suggested test hardening
var ve *errs.ValidationError if !errors.As(err, &ve) { t.Fatalf("error type = %T, want a typed *errs.ValidationError", err) } +pb, ok := errs.ProblemOf(err) +if !ok { + t.Fatalf("expected errs.ProblemOf(err) to succeed") +} +if pb.Subtype != errs.SubtypeInvalidArgument { + t.Fatalf("Subtype = %q, want %q", pb.Subtype, errs.SubtypeInvalidArgument) +} +if pb.Category == "" { + t.Fatalf("Category must be set for typed validation errors") +} +if ve.Param != "--flag-name" { + t.Fatalf("Param = %q, want --flag-name", ve.Param) +}As per coding guidelines, "Error-path tests must assert typed metadata via
errs.ProblemOf(category/subtype/param) ... not message substrings alone." Based on learnings,Parammust be read from*errs.ValidationErrorrather thanerrs.ProblemOf.🤖 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/sheets/flag_schema_test.go` around lines 194 - 197, After the errors.As check that captures ve, assert the typed problem metadata and the concrete Param: call errs.ProblemOf(ve) and assert its category/subtype/param fields equal the test's expected values, and also assert ve.Param (the ValidationError.Param field) equals the expectedParam; place these assertions immediately after the errors.As branch so the test locks both ProblemOf metadata and the ValidationError.Param contract.Sources: Coding guidelines, Learnings
🧹 Nitpick comments (3)
internal/cmdutil/fileupload.go (2)
134-136: 💤 Low valueFile open failure subtype may not always fit
invalid_argument.When
fileIO.Open(filePath)fails, the cause might be file not found, permission denied, or other I/O issues—not necessarily that the user's argument was invalid. Per guidelines, local file I/O failures should useerrs.NewInternalError(errs.SubtypeFileIO, ...). However, if the file doesn't exist because the user mistyped the path,SubtypeInvalidArgumentcould be appropriate.Consider inspecting the error to choose the subtype (e.g.,
os.IsNotExist→SubtypeInvalidArgument; permission/other →SubtypeFileIO), or document the intentional choice if all file-open failures are attributed to user error.🤖 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/cmdutil/fileupload.go` around lines 134 - 136, Change the error construction after fileIO.Open(filePath) to pick the subtype based on the underlying error: if os.IsNotExist(err) then return errs.NewValidationError(errs.SubtypeInvalidArgument, "cannot open file: %s", filePath).WithParam("--file").WithCause(err); otherwise return errs.NewInternalError(errs.SubtypeFileIO, "cannot open file: %s", filePath).WithParam("--file").WithCause(err). Preserve the existing WithParam("--file") and WithCause(err) chaining and perform the os.IsNotExist check on the original error returned by fileIO.Open(filePath).Source: Coding guidelines
121-123: 💤 Low valueSubtype mismatch: stdin read failure is not an invalid argument.
A failure to read from stdin is an I/O issue, not a user providing an invalid argument. Per coding guidelines,
SubtypeInvalidArgumentis for user flag/argument validation failures. Consider usingerrs.NewInternalError(errs.SubtypeFileIO, ...)for the read failure itself.Same concern applies to lines 141-143 where file read fails after successfully opening.
🤖 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/cmdutil/fileupload.go` around lines 121 - 123, The error construction for stdin and file read failures in internal/cmdutil/fileupload.go incorrectly uses errs.NewValidationError with errs.SubtypeInvalidArgument; change both sites (the stdin-read return around the current block returning errs.NewValidationError(... "--file: failed to read stdin") and the subsequent file-read failure return around lines ~141-143) to return an internal I/O error using errs.NewInternalError with errs.SubtypeFileIO (and preserve the existing message, param via WithParam("--file") and WithCause(err)); this makes the error subtype reflect a file I/O failure rather than an invalid argument.Source: Coding guidelines
internal/core/config_test.go (1)
107-113: 💤 Low valueConsider asserting subtype via
errs.ProblemOfper testing guidelines.The test correctly verifies
*errs.ConfigErrortype, but per coding guidelines for*_test.go, error-path tests should assert typed metadata viaerrs.ProblemOf(category/subtype) rather than just type and hint presence. This would make the test more robust against future changes.prob, ok := errs.ProblemOf(err) if !ok || prob.Subtype != "not_configured" { t.Errorf("expected not_configured subtype, got %v", prob) }🤖 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/core/config_test.go` around lines 107 - 113, Replace the loose type check with a subtype assertion using errs.ProblemOf and keep the hint assertion: call prob, ok := errs.ProblemOf(err) and fail the test if !ok or prob.Subtype != "not_configured", then still extract the concrete error for the hint (e.g. var cfgErr *errs.ConfigError; if !errors.As(err, &cfgErr) { t.Fatalf(...) } ) and assert cfgErr.Hint != "" so the test verifies both the problem subtype ("not_configured") and that the ConfigError Hint is non-empty.Source: Coding guidelines
🤖 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/config/init.go`:
- Around line 128-131: Replace the call that currently returns
errs.NewConfigError(...) with
errs.NewValidationError(errs.SubtypeFailedPrecondition, ...) so this
valid-but-disallowed-by-state error uses a failed-precondition validation error;
preserve the existing error message format (the formatted ws.Display() strings)
and keep the .WithHint(...) unchanged so the hint text remains the same.
In `@cmd/profile/add.go`:
- Around line 150-151: The error wrap for core.SaveMultiAppConfig should use the
file-I/O subtype: change the call that currently constructs
errs.NewInternalError(errs.SubtypeStorage, "failed to save config: %v",
err).WithCause(err) to use errs.SubtypeFileIO instead (i.e.,
errs.NewInternalError(errs.SubtypeFileIO, "failed to save config: %v",
err).WithCause(err)) so the local config file write failure preserves the
correct subtype and the underlying cause.
In `@cmd/profile/list.go`:
- Line 49: The current code converts every non-os.ErrNotExist config load
failure into errs.NewValidationError which misclassifies file I/O and can strip
existing typed errors; update the error handling where the config load error is
returned so that: if err == nil return as before; if errors.Is(err,
os.ErrNotExist) keep the existing behavior; if the incoming err already is a
typed/errs.* error (or implements the package's typed error interface) re‐return
it unchanged; otherwise wrap file I/O failures with
errs.NewInternalError(errs.SubtypeFileIO, "failed to load config: %v",
err).WithCause(err) instead of NewValidationError; locate and change the call
site that currently calls errs.NewValidationError(...) to implement this
branching logic.
In `@cmd/profile/profile_test.go`:
- Around line 440-460: Update assertValidationError to use errs.ProblemOf(err)
for the problem-level assertions (category, subtype, hint, retryable) instead of
relying on the concrete ValidationError.Message, and keep errors.As(err,
&valErr) only to assert the typed cause/Param preservation; specifically, call p
:= errs.ProblemOf(err) and check p.Category and p.Subtype (and p.Hint /
p.Retryable if expected) and remove/replace the message substring check with
problem-level checks, while still using errors.As(..., *errs.ValidationError) to
inspect Param-specific fields and keep the exit code assertion against
output.ExitValidation.
In `@cmd/profile/remove.go`:
- Around line 69-70: Save the typed error returned by core.SaveMultiAppConfig
instead of always re-wrapping it: after calling core.SaveMultiAppConfig(multi),
if err != nil use errors.As (or a type assertion) to check whether err is
already an errs.* typed error and if so return it directly; only call
errs.NewInternalError(...).WithCause(err) when err is not already an errs error
so you preserve lower-layer typed classifications (reference
core.SaveMultiAppConfig, errs.NewInternalError and .WithCause).
In `@cmd/profile/rename.go`:
- Around line 70-71: Change the error classification for the local config save
failure: in the SaveMultiAppConfig error branch (the call to
core.SaveMultiAppConfig and the errs.NewInternalError invocation), replace
errs.SubtypeStorage with errs.SubtypeFileIO so the failure is classified as file
I/O, keeping the same message and the .WithCause(err) call to preserve the
original error.
In `@cmd/profile/use.go`:
- Around line 70-71: The error classification is wrong: in the call that wraps
core.SaveMultiAppConfig's error, replace errs.SubtypeStorage with
errs.SubtypeFileIO so the code reads errs.NewInternalError(errs.SubtypeFileIO,
"failed to save config: %v", err).WithCause(err) and keep the existing
WithCause(err) to preserve the original error.
In `@cmd/schema/schema_test.go`:
- Around line 212-224: Replace the current substring and concrete-type
assertions on err with checks against errs.ProblemOf(err): call p :=
errs.ProblemOf(err) and assert p != nil, then assert p.Category, p.Subtype
(expect errs.SubtypeInvalidArgument) and any p.Param values you expect; also
assert the original cause is preserved using errors.Is/errors.As against the
underlying sentinel or cause if applicable. Keep the existing message/hint
substring checks (e.g., ve.Hint) only as optional secondary assertions, but drop
the concrete *errs.ValidationError type assertion and the primary reliance on
err.Error() substrings and instead use errs.ProblemOf(err) as the authoritative
metadata surface.
In `@cmd/update/update_test.go`:
- Around line 338-347: Replace the concrete-type assertions on errs.NetworkError
with the typed-test contract: call errs.ProblemOf(err) and assert the returned
Problem's Category/Subtype/Param values (e.g., Subtype ==
errs.SubtypeNetworkTransport) instead of checking netErr.Subtype directly;
additionally verify cause-preservation by asserting the underlying cause
(errors.Unwrap / errors.As / errors.Is) matches the original lower-level error;
keep the exit-code assertion using output.ExitCodeOf(err) == output.ExitNetwork.
Ensure these changes are applied to the same test sites that previously checked
concrete types (the blocks around the existing use of errs.NetworkError and
output.ExitCodeOf) so the test now validates errs.ProblemOf metadata plus
preserved cause rather than relying solely on concrete type checks.
In `@cmd/update/update.go`:
- Around line 237-239: The failure from PrepareSelfReplace is misclassified as
an API error; change the error creation passed to reportError so it uses
errs.NewInternalError with subtype errs.SubtypeFileIO (preserving the original
error as the cause) instead of errs.NewAPIError; specifically replace the
errs.NewAPIError(...) call in the reportError invocation that handles the
PrepareSelfReplace error with errs.NewInternalError(errs.SubtypeFileIO, "failed
to prepare update: %s", err).WithCause(err) while keeping the same
reportError(opts, io, "update_error", ...) form and message text.
In `@internal/cmdutil/fileupload_test.go`:
- Around line 24-49: Update the shared test helper requireFileValidationError to
also assert problem-level metadata using errs.ProblemOf(err): call prob :=
errs.ProblemOf(err) (or similar API) and check prob.Category ==
errs.CategoryValidation and prob.Subtype == wantSubtype (or compare prob.Subtype
to wantSubtype) so callers enforce category+subtype; keep the existing
errors.As(&valErr) checks for Param/Params and preserve returning valErr so the
cause/typed error assertions remain valid. Ensure the new ProblemOf check
happens before/alongside the existing exit-code and Param assertions and report
failures via t.Errorf/t.Fatalf as appropriate.
In `@internal/hook/install.go`:
- Around line 258-262: The current panic handling wraps the recovered value `r`
with fmt.Errorf inside the WithCause call, which loses the original error
identity; change the WithCause argument at the returned assignment so that if
the recovered `r` is already an error (type assert or type switch) you pass that
error directly to WithCause(err), otherwise construct a new error (e.g.,
fmt.Errorf("hook %q panic: %v", fullName, r)) and pass that; update the
WithCause(...) call in the same returned =
errs.NewValidationError(...).WithCause(...) expression so errors.Is/As can match
the original panic error type.
In `@internal/output/jq.go`:
- Line 55: The jq execution error is currently wrapped as an API error via
errs.NewAPIError which misclassifies a local --jq validation failure; replace
the call to errs.NewAPIError(errs.SubtypeUnknown, "jq error: %s",
err).WithCause(err) with a validation-style error (e.g.,
errs.NewValidationError(errs.SubtypeUnknown, "jq error: %s",
err).WithCause(err)) so jq iterator failures are reported as local validation
errors rather than API errors, preserving the original message and the
WithCause(err) chaining.
In `@shortcuts/common/call_api_typed_test.go`:
- Around line 146-155: The test currently only asserts that err is a typed
error; extend it to assert the typed metadata and cause: after calling
rt.DoAPIJSON("POST", "/open-apis/x/y", ...) and verifying data==nil and err
non-nil, extract p := errs.ProblemOf(err) and assert p.Category (or p.Code)
equals the expected category/code string and, if a deterministic subtype exists
for this HTTP-400 case, assert p.Subtype equals that subtype; also assert that
p.Cause (or errs.Cause(err)) preserves the original lower-level error (not just
message) or is non-nil as appropriate. Use the existing variables (rt, err,
data, errs.ProblemOf) and add clear t.Fatalf/t.Errorf checks for mismatch so the
test fails when metadata drifts.
In `@shortcuts/common/runner.go`:
- Around line 167-169: In AccessToken, avoid reclassifying every resolver error
to errs.SubtypeTokenInvalid: after calling ResolveToken, check whether err is
already a typed/structured error (e.g., via a type assertion to the package's
typed error type or interface) and if so return it unchanged; only call
errs.NewAuthenticationError(..., errs.SubtypeTokenInvalid, ...).WithCause(err)
when the error is not already a typed error. Preserve existing symbols:
AccessToken, ResolveToken, errs.NewAuthenticationError,
errs.SubtypeTokenInvalid, and WithCause.
---
Outside diff comments:
In `@cmd/config/bind_test.go`:
- Around line 32-63: Update assertExitError and the wantErrDetail type to also
assert ProblemOf typed metadata (subtype and param) and to verify cause
preservation: after detecting a *errs.ValidationError or *errs.ConfigError,
extract and compare the ProblemOf fields (e.g., ve.ProblemOf.Subtype /
ve.ProblemOf.Param and ce.ProblemOf.Subtype / ce.ProblemOf.Param) against
wantErrDetail.Subtype and wantErrDetail.Param, and assert the error's cause
chain preserves the original cause by checking errors.Unwrap/errors.Is (or
walking errors.Unwrap until nil) matches wantErrDetail.Cause (or its message) —
apply the same checks in both the ValidationError and ConfigError branches and
update any tests to populate the new wantErrDetail fields.
In `@internal/client/api_errors_test.go`:
- Around line 266-282: Update the TestWrapDoAPIError_UntypedErrorRoutesToNetwork
test to assert the typed problem metadata on the fallback NetworkError: after
asserting errors.As to *errs.NetworkError, call p := errs.ProblemOf(got) and add
assertions that p.Category == "network" and p.Subtype == "network_transport"
(and any expected params if applicable) to pin the fallback branch; keep the
existing checks that it is not an InternalError and preserve the original error
as the cause.
In `@internal/core/notconfigured_test.go`:
- Around line 167-200: The test
TestLoadOrNotConfigured_CorruptFile_PreservesCause should assert that the
original parsing error is preserved and that typed metadata is exposed via
errs.ProblemOf: after calling LoadOrNotConfigured() and asserting the returned
error is a *errs.ConfigError, add an errors.As or errors.Unwrap check to capture
the underlying error (e.g. var parseErr *json.SyntaxError or var cause error and
use errors.As/unpack) and assert it's non-nil and matches the expected
parse/syntax error; also call errs.ProblemOf(err) to assert
category/subtype/params (or at least that Subtype == errs.SubtypeInvalidConfig)
instead of relying solely on message substrings so the test verifies cause-chain
preservation and typed metadata.
In `@lint/errscontract/rule_no_legacy_runtime_api_call.go`:
- Around line 42-44: The rule currently only checks importsPath(...) and uses
matchLegacyRuntimeAPIMethod (name-only) which causes false positives; update the
matcher to also verify the selector's receiver type is exactly
common.RuntimeContext or *common.RuntimeContext (i.e., inspect the selector
expression's type info / AST to confirm the named/ptr type refers to the common
package's RuntimeContext) and only then flag method names CallAPI, DoAPIJSON,
DoAPIJSONWithLogID; add a regression test that imports
github.com/larksuite/cli/shortcuts/common but calls CallAPI on a different
receiver type (non-common.RuntimeContext) and assert no violation; keep existing
exceptions (typed wrapper / RawAPI) unchanged.
---
Minor comments:
In `@cmd/completion/completion.go`:
- Around line 34-36: The validation error created by errs.NewValidationError for
unsupported shells (the call that currently uses errs.NewValidationError(...,
"unsupported shell: %s", args[0]).WithHint(...)) must include the parameter name
of the failing user input; update that error chain to call .WithParam("shell")
(e.g., errs.NewValidationError(..., "unsupported shell: %s",
args[0]).WithParam("shell").WithHint(...)) so the error envelope records which
argument failed.
In `@cmd/config/config_test.go`:
- Around line 418-427: The test
TestWrapSaveConfigError_PassesTypedValidationThrough currently only checks
type/subtype but not that the original cause is preserved; update the test to
capture the wrapped error (e.g., wrapped := wrapSaveConfigError(conflict)) and
assert that the original error is preserved via errors.Is or errors.Unwrap (for
example: if !errors.Is(wrapped, conflict) { t.Fatalf("expected cause to be
preserved") }); do the same for the untyped case by creating the original err
(e.g., orig := errors.New("disk full")), wrapping it, and asserting
errors.Is(wrapped, orig) or that errors.Unwrap(wrapped) == orig while keeping
the existing type/subtype assertions for wrapSaveConfigError.
In `@cmd/root.go`:
- Around line 271-275: The code builds a typed validation error but drops the
original Cobra error, breaking errors.Is/As; update the creation of depErr so it
preserves the original err as its cause (use
errs.NewValidationError(...).WithCause(err) or the library's equivalent) before
passing depErr to output.WriteTypedErrorEnvelope and output.ExitCodeOf so error
unwrapping still works for downstream handlers and tests.
In `@errs/ERROR_CONTRACT.md`:
- Around line 556-568: The "Migration to the typed contract is complete"
statement conflicts with earlier sections that describe a staged migration and
unmigrated legacy paths; choose a single authoritative status and make the text
consistent: either change the intro/invariant paragraph to describe a staged
migration and list remaining legacy paths (and keep references to legacy
constructors like Errorf, ErrAPI, ErrAuth, ErrWithHint, ClassifyLarkError,
ErrDetail, ExitError, ErrorEnvelope as "removed only in migrated modules") or
update the earlier sections to state migration is complete and remove any
references to staged/unmigrated paths. Also ensure the note about output.ErrBare
(and the new *output.BareError type) is worded consistently with the chosen
status and references the typed contract builders
(errs.NewXxxError(...).WithX(...)) so the document presents one clear,
non-contradictory migration status.
In `@internal/core/notconfigured.go`:
- Around line 45-49: The returned config error currently formats the original
err into the message but does not attach it as a cause; update the return in the
function that sets subtype (using isMalformedConfigError) so that the Errs value
produced by errs.NewConfigError(subtype, "failed to load config: %v", err) is
followed by .WithCause(err) before returning (i.e., return nil,
errs.NewConfigError(...).WithCause(err)) so callers can use
errors.Is/errors.Unwrap to inspect the underlying error.
In `@internal/keychain/keychain.go`:
- Around line 52-54: Replace the API error construction in the keychain failure
path: instead of calling errs.NewAPIError(...) use
errs.NewInternalError(errs.SubtypeUnknown, "%s", msg) and then chain
.WithHint("%s", hint).WithCause(err) so the returned error uses the internal
error category for local keychain/credential-store failures (replace the
existing errs.NewAPIError(...) expression in internal/keychain/keychain.go).
In `@shortcuts/common/validate_test.go`:
- Around line 86-89: The test only asserts presence/absence of err for
MutuallyExclusiveTyped table cases, which lets regressions in the typed error
envelope pass; update the negative branches to assert the error's typed metadata
(e.g., its category/subtype/param) rather than just non-nil. Concretely, in the
MutuallyExclusiveTyped test cases where tt.wantErr is true, extract and assert
the concrete typed error (using errors.As or the project's helper for unwrapping
typed errors) and compare its category/subtype/param fields to the expected
values from the test case; apply the same pattern to the other failing test
sites that follow the same (err != nil) checks so they assert the typed metadata
as well.
In `@shortcuts/register_brand_guard_test.go`:
- Around line 74-83: The test currently asserts the error is an
*errs.ValidationError and checks substrings in validationErr.Error(); instead,
use errs.ProblemOf to assert the structured metadata (category, subtype, and any
param keys) so the contract is locked. Update the assertions around
validationErr (the variable created by errors.As) to call
errs.ProblemOf(validationErr) and assert Problem.Category (or Category string),
Problem.Subtype equals errs.SubtypeFailedPrecondition, and that the
Problem.Params include the expected keys/values (e.g., "apps" and "lark") rather
than relying on message substring checks; apply the same replacement for the
similar checks at the second site (the assertions currently at lines ~116-122).
In `@shortcuts/sheets/flag_schema_test.go`:
- Around line 194-197: After the errors.As check that captures ve, assert the
typed problem metadata and the concrete Param: call errs.ProblemOf(ve) and
assert its category/subtype/param fields equal the test's expected values, and
also assert ve.Param (the ValidationError.Param field) equals the expectedParam;
place these assertions immediately after the errors.As branch so the test locks
both ProblemOf metadata and the ValidationError.Param contract.
---
Nitpick comments:
In `@internal/cmdutil/fileupload.go`:
- Around line 134-136: Change the error construction after fileIO.Open(filePath)
to pick the subtype based on the underlying error: if os.IsNotExist(err) then
return errs.NewValidationError(errs.SubtypeInvalidArgument, "cannot open file:
%s", filePath).WithParam("--file").WithCause(err); otherwise return
errs.NewInternalError(errs.SubtypeFileIO, "cannot open file: %s",
filePath).WithParam("--file").WithCause(err). Preserve the existing
WithParam("--file") and WithCause(err) chaining and perform the os.IsNotExist
check on the original error returned by fileIO.Open(filePath).
- Around line 121-123: The error construction for stdin and file read failures
in internal/cmdutil/fileupload.go incorrectly uses errs.NewValidationError with
errs.SubtypeInvalidArgument; change both sites (the stdin-read return around the
current block returning errs.NewValidationError(... "--file: failed to read
stdin") and the subsequent file-read failure return around lines ~141-143) to
return an internal I/O error using errs.NewInternalError with errs.SubtypeFileIO
(and preserve the existing message, param via WithParam("--file") and
WithCause(err)); this makes the error subtype reflect a file I/O failure rather
than an invalid argument.
In `@internal/core/config_test.go`:
- Around line 107-113: Replace the loose type check with a subtype assertion
using errs.ProblemOf and keep the hint assertion: call prob, ok :=
errs.ProblemOf(err) and fail the test if !ok or prob.Subtype !=
"not_configured", then still extract the concrete error for the hint (e.g. var
cfgErr *errs.ConfigError; if !errors.As(err, &cfgErr) { t.Fatalf(...) } ) and
assert cfgErr.Hint != "" so the test verifies both the problem subtype
("not_configured") and that the ConfigError Hint is non-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
i️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3a0d617a-b89d-4b30-bfa9-d7118f873644
📒 Files selected for processing (131)
.golangci.ymlcmd/api/api.gocmd/api/api_test.gocmd/auth/check_test.gocmd/auth/list.gocmd/auth/login_test.gocmd/completion/completion.gocmd/config/bind.gocmd/config/bind_test.gocmd/config/binder_test.gocmd/config/config_test.gocmd/config/init.gocmd/config/init_guard_test.gocmd/config/init_messages.gocmd/config/init_test.gocmd/doctor/doctor.gocmd/error_auth_hint.gocmd/event/format_helpers_test.gocmd/event/status_fail_on_orphan_test.gocmd/flag_suggest_test.gocmd/platform_bootstrap_test.gocmd/platform_guards.gocmd/platform_guards_test.gocmd/plugin_integration_test.gocmd/profile/add.gocmd/profile/list.gocmd/profile/profile_test.gocmd/profile/remove.gocmd/profile/rename.gocmd/profile/use.gocmd/prune.gocmd/prune_test.gocmd/root.gocmd/root_integration_test.gocmd/root_test.gocmd/schema/schema.gocmd/schema/schema_test.gocmd/unknown_subcommand_test.gocmd/update/update.gocmd/update/update_test.goerrs/ERROR_CONTRACT.mderrs/raw.goerrs/raw_test.goerrs/types_test.goextension/platform/abort.goextension/platform/errors.gointernal/auth/errors.gointernal/auth/errors_test.gointernal/auth/uat_client.gointernal/client/api_errors_test.gointernal/client/client.gointernal/client/client_test.gointernal/cmdpolicy/aggregation_test.gointernal/cmdpolicy/apply.gointernal/cmdpolicy/engine.gointernal/cmdpolicy/source_label_test.gointernal/cmdutil/confirm.gointernal/cmdutil/confirm_test.gointernal/cmdutil/factory_default_test.gointernal/cmdutil/fileupload.gointernal/cmdutil/fileupload_test.gointernal/cmdutil/json.gointernal/cmdutil/json_test.gointernal/cmdutil/lang.gointernal/core/config.gointernal/core/config_test.gointernal/core/errors.gointernal/core/notconfigured.gointernal/core/notconfigured_test.gointernal/errcompat/promote.gointernal/errcompat/promote_auth.gointernal/errcompat/promote_auth_test.gointernal/errcompat/promote_test.gointernal/hook/install.gointernal/hook/install_test.gointernal/keychain/keychain.gointernal/keychain/keychain_darwin_test.gointernal/keychain/keychain_typed_error_test.gointernal/output/bare.gointernal/output/bare_test.gointernal/output/emit.gointernal/output/emit_test.gointernal/output/envelope.gointernal/output/errors.gointernal/output/errors_test.gointernal/output/exitcode.gointernal/output/jq.gointernal/output/lark_errors.gointernal/output/lark_errors_test.gointernal/output/ownership_recovery.gointernal/output/ownership_recovery_test.golint/errscontract/rule_no_legacy_common_helper_call.golint/errscontract/rule_no_legacy_envelope_literal.golint/errscontract/rule_no_legacy_runtime_api_call.golint/errscontract/rule_typed_error_completeness.golint/errscontract/rules_test.goshortcuts/apps/apps_callapi_typed_test.goshortcuts/apps/apps_db_table_list_test.goshortcuts/base/record_upload_attachment.goshortcuts/calendar/calendar_test.goshortcuts/common/call_api_typed_test.goshortcuts/common/common.goshortcuts/common/drive_media_upload.goshortcuts/common/drive_media_upload_test.goshortcuts/common/drive_media_upload_typed_test.goshortcuts/common/permission_grant.goshortcuts/common/permission_grant_test.goshortcuts/common/runner.goshortcuts/common/runner_scope_test.goshortcuts/common/validate.goshortcuts/common/validate_test.goshortcuts/doc/doc_media_test.goshortcuts/doc/doc_media_upload.goshortcuts/drive/drive_add_comment_test.goshortcuts/drive/drive_errors.goshortcuts/drive/drive_import_common.goshortcuts/drive/drive_push.goshortcuts/drive/drive_upload.goshortcuts/minutes/minutes_download_test.goshortcuts/register.goshortcuts/register_brand_guard_test.goshortcuts/register_test.goshortcuts/sheets/backward/lark_sheets_float_images.goshortcuts/sheets/flag_schema_test.goshortcuts/sheets/lark_sheet_object_crud.goshortcuts/sheets/lark_sheet_write_cells.goshortcuts/slides/slides_media_upload.goshortcuts/task/task_shortcut_test.goshortcuts/task/tasklist_add_task_test.goshortcuts/vc/vc_notes_test.gotests/cli_e2e/config/bind_test.go
💤 Files with no reviewable changes (12)
- internal/errcompat/promote_auth.go
- internal/errcompat/promote_test.go
- internal/output/ownership_recovery.go
- internal/errcompat/promote.go
- internal/core/errors.go
- cmd/error_auth_hint.go
- internal/output/ownership_recovery_test.go
- shortcuts/common/common.go
- internal/output/errors.go
- internal/errcompat/promote_auth_test.go
- internal/output/envelope.go
- internal/output/lark_errors_test.go
@evandance
evandance
force-pushed
the
feat/errs-legacy-final-cleanup
branch
2 times, most recently
from
June 13, 2026 10:39
2091241 to
d03ae55
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/profile/add.go (1)
151-151:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
SubtypeFileIOfor config save failures.Line 151 wraps
core.SaveMultiAppConfigaserrs.SubtypeStorage, but this is a local config file write path and should keep the file-I/O subtype contract.Suggested fix
- return errs.NewInternalError(errs.SubtypeStorage, "failed to save config: %v", err).WithCause(err) + return errs.NewInternalError(errs.SubtypeFileIO, "failed to save config: %v", err).WithCause(err)As per coding guidelines, local file I/O failures must use
errs.NewInternalError(errs.SubtypeFileIO, ...)and preserve underlying causes via.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/profile/add.go` at line 151, The error returned when core.SaveMultiAppConfig is failing uses errs.SubtypeStorage but should use the file-I/O subtype; change the error construction at the return site to use errs.NewInternalError(errs.SubtypeFileIO, "failed to save config: %v", err).WithCause(err) so the failure is classified as SubtypeFileIO and the underlying err is preserved via .WithCause(err).Source: Coding guidelines
🤖 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 `@shortcuts/common/drive_media_upload_test.go`:
- Around line 235-241: The test currently only checks errs.ProblemOf returned
p.Category and p.Code; extend the assertions to also validate the typed metadata
and cause preservation: after p, ok := errs.ProblemOf(err) assert ok, then
assert p.Subtype equals the expected subtype string and any expected p.Param
entries (or p.Param == nil if none), and verify the original cause is preserved
either by checking p.Cause is the original error or using errors.Is/As against
the original underlying error; update the same pattern in the sibling test that
also uses errs.ProblemOf to cover subtype/param and cause.
---
Duplicate comments:
In `@cmd/profile/add.go`:
- Line 151: The error returned when core.SaveMultiAppConfig is failing uses
errs.SubtypeStorage but should use the file-I/O subtype; change the error
construction at the return site to use errs.NewInternalError(errs.SubtypeFileIO,
"failed to save config: %v", err).WithCause(err) so the failure is classified as
SubtypeFileIO and the underlying err is preserved via .WithCause(err).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
i️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 98cbff77-6c1a-4159-8e5d-568e1b67b6e3
📒 Files selected for processing (136)
.golangci.ymlcmd/api/api.gocmd/api/api_test.gocmd/auth/check_test.gocmd/auth/list.gocmd/auth/login_test.gocmd/completion/completion.gocmd/config/bind.gocmd/config/bind_test.gocmd/config/binder_test.gocmd/config/config_test.gocmd/config/init.gocmd/config/init_guard_test.gocmd/config/init_messages.gocmd/config/init_test.gocmd/doctor/doctor.gocmd/error_auth_hint.gocmd/event/consume.gocmd/event/format_helpers_test.gocmd/event/status_fail_on_orphan_test.gocmd/flag_suggest_test.gocmd/platform_bootstrap_test.gocmd/platform_guards.gocmd/platform_guards_test.gocmd/plugin_integration_test.gocmd/profile/add.gocmd/profile/list.gocmd/profile/profile_test.gocmd/profile/remove.gocmd/profile/rename.gocmd/profile/use.gocmd/prune.gocmd/prune_test.gocmd/root.gocmd/root_integration_test.gocmd/root_test.gocmd/schema/schema_test.gocmd/unknown_subcommand_test.gocmd/update/update.gocmd/update/update_test.goerrs/ERROR_CONTRACT.mderrs/raw.goerrs/raw_test.goerrs/types_test.goextension/platform/abort.goextension/platform/errors.gointernal/auth/errors.gointernal/auth/errors_test.gointernal/auth/uat_client.gointernal/client/api_errors_test.gointernal/client/client.gointernal/client/client_test.gointernal/cmdpolicy/aggregation_test.gointernal/cmdpolicy/apply.gointernal/cmdpolicy/engine.gointernal/cmdpolicy/source_label_test.gointernal/cmdutil/confirm.gointernal/cmdutil/confirm_test.gointernal/cmdutil/factory_default_test.gointernal/cmdutil/fileupload.gointernal/cmdutil/fileupload_test.gointernal/cmdutil/json.gointernal/cmdutil/json_test.gointernal/cmdutil/lang.gointernal/core/config.gointernal/core/config_test.gointernal/core/errors.gointernal/core/notconfigured.gointernal/core/notconfigured_test.gointernal/errcompat/promote.gointernal/errcompat/promote_auth.gointernal/errcompat/promote_auth_test.gointernal/errcompat/promote_test.gointernal/hook/install.gointernal/hook/install_test.gointernal/keychain/keychain.gointernal/keychain/keychain_darwin_test.gointernal/keychain/keychain_typed_error_test.gointernal/output/bare.gointernal/output/bare_test.gointernal/output/emit.gointernal/output/emit_test.gointernal/output/envelope.gointernal/output/errors.gointernal/output/errors_test.gointernal/output/exitcode.gointernal/output/jq.gointernal/output/lark_errors.gointernal/output/lark_errors_test.gointernal/output/ownership_recovery.gointernal/output/ownership_recovery_test.golint/errscontract/rule_no_legacy_common_helper_call.golint/errscontract/rule_no_legacy_envelope_literal.golint/errscontract/rule_no_legacy_runtime_api_call.golint/errscontract/rule_typed_error_completeness.golint/errscontract/rules_test.golint/errscontract/scan.goshortcuts/apps/apps_callapi_typed_test.goshortcuts/apps/apps_db_table_list_test.goshortcuts/base/record_upload_attachment.goshortcuts/calendar/calendar_test.goshortcuts/common/call_api_typed_test.goshortcuts/common/common.goshortcuts/common/drive_media_upload.goshortcuts/common/drive_media_upload_test.goshortcuts/common/drive_media_upload_typed_test.goshortcuts/common/permission_grant.goshortcuts/common/permission_grant_test.goshortcuts/common/runner.goshortcuts/common/runner_scope_test.goshortcuts/common/validate.goshortcuts/common/validate_test.goshortcuts/doc/doc_media_test.goshortcuts/doc/doc_media_upload.goshortcuts/drive/drive_add_comment_test.goshortcuts/drive/drive_errors.goshortcuts/drive/drive_import_common.goshortcuts/drive/drive_push.goshortcuts/drive/drive_upload.goshortcuts/minutes/minutes_download_test.goshortcuts/register.goshortcuts/register_brand_guard_test.goshortcuts/register_test.goshortcuts/sheets/backward/lark_sheets_float_images.goshortcuts/sheets/flag_schema_test.goshortcuts/sheets/lark_sheet_object_crud.goshortcuts/sheets/lark_sheet_write_cells.goshortcuts/slides/slides_media_upload.goshortcuts/task/task_shortcut_test.goshortcuts/task/tasklist_add_task_test.goshortcuts/vc/vc_notes_test.gotests/cli_e2e/apps/apps_access_scope_get_dryrun_test.gotests/cli_e2e/apps/apps_create_dryrun_test.gotests/cli_e2e/apps/apps_html_publish_dryrun_test.gotests/cli_e2e/apps/apps_update_dryrun_test.gotests/cli_e2e/config/bind_test.go
💤 Files with no reviewable changes (11)
- internal/errcompat/promote_test.go
- internal/errcompat/promote_auth.go
- internal/output/ownership_recovery.go
- internal/output/ownership_recovery_test.go
- internal/errcompat/promote_auth_test.go
- shortcuts/common/common.go
- internal/output/lark_errors_test.go
- internal/errcompat/promote.go
- internal/core/errors.go
- internal/output/envelope.go
- internal/output/errors.go
✅ Files skipped from review due to trivial changes (10)
- cmd/auth/list.go
- cmd/event/consume.go
- lint/errscontract/rule_typed_error_completeness.go
- shortcuts/doc/doc_media_test.go
- shortcuts/apps/apps_callapi_typed_test.go
- shortcuts/apps/apps_db_table_list_test.go
- internal/cmdpolicy/engine.go
- extension/platform/abort.go
- errs/types_test.go
- tests/cli_e2e/config/bind_test.go
🚧 Files skipped from review as they are similar to previous changes (101)
- internal/output/bare_test.go
- shortcuts/vc/vc_notes_test.go
- shortcuts/task/tasklist_add_task_test.go
- cmd/profile/list.go
- shortcuts/task/task_shortcut_test.go
- cmd/auth/check_test.go
- extension/platform/errors.go
- shortcuts/sheets/lark_sheet_write_cells.go
- shortcuts/minutes/minutes_download_test.go
- cmd/auth/login_test.go
- cmd/event/format_helpers_test.go
- internal/output/bare.go
- shortcuts/drive/drive_import_common.go
- shortcuts/sheets/backward/lark_sheets_float_images.go
- cmd/config/init_guard_test.go
- internal/client/client_test.go
- cmd/completion/completion.go
- internal/cmdutil/confirm_test.go
- cmd/config/init_test.go
- shortcuts/doc/doc_media_upload.go
- shortcuts/register_brand_guard_test.go
- cmd/event/status_fail_on_orphan_test.go
- internal/auth/errors_test.go
- cmd/platform_guards_test.go
- shortcuts/common/call_api_typed_test.go
- cmd/config/binder_test.go
- cmd/profile/remove.go
- cmd/profile/rename.go
- shortcuts/base/record_upload_attachment.go
- internal/cmdutil/confirm.go
- lint/errscontract/rule_no_legacy_runtime_api_call.go
- shortcuts/calendar/calendar_test.go
- internal/cmdutil/factory_default_test.go
- cmd/profile/use.go
- cmd/platform_bootstrap_test.go
- internal/core/config_test.go
- cmd/prune.go
- cmd/config/init_messages.go
- shortcuts/drive/drive_errors.go
- internal/hook/install_test.go
- internal/core/config.go
- shortcuts/common/drive_media_upload_typed_test.go
- cmd/prune_test.go
- internal/keychain/keychain_darwin_test.go
- errs/raw.go
- cmd/flag_suggest_test.go
- internal/keychain/keychain.go
- cmd/api/api_test.go
- shortcuts/drive/drive_add_comment_test.go
- internal/hook/install.go
- internal/cmdutil/json.go
- shortcuts/sheets/lark_sheet_object_crud.go
- internal/client/api_errors_test.go
- cmd/update/update.go
- internal/keychain/keychain_typed_error_test.go
- internal/cmdutil/lang.go
- cmd/schema/schema_test.go
- cmd/platform_guards.go
- lint/errscontract/rule_no_legacy_envelope_literal.go
- shortcuts/register.go
- internal/core/notconfigured_test.go
- errs/raw_test.go
- shortcuts/common/permission_grant.go
- shortcuts/sheets/flag_schema_test.go
- shortcuts/register_test.go
- shortcuts/slides/slides_media_upload.go
- cmd/plugin_integration_test.go
- internal/output/emit.go
- internal/output/errors_test.go
- cmd/update/update_test.go
- internal/auth/errors.go
- shortcuts/common/runner.go
- shortcuts/drive/drive_push.go
- cmd/root_test.go
- cmd/api/api.go
- internal/core/notconfigured.go
- cmd/config/init.go
- cmd/doctor/doctor.go
- internal/output/lark_errors.go
- internal/cmdutil/json_test.go
- cmd/unknown_subcommand_test.go
- internal/output/exitcode.go
- cmd/root_integration_test.go
- cmd/config/config_test.go
- shortcuts/drive/drive_upload.go
- cmd/error_auth_hint.go
- internal/output/jq.go
- lint/errscontract/rules_test.go
- internal/auth/uat_client.go
- internal/client/client.go
- internal/cmdutil/fileupload_test.go
- .golangci.yml
- internal/cmdpolicy/apply.go
- lint/errscontract/rule_no_legacy_common_helper_call.go
- shortcuts/common/validate.go
- shortcuts/common/drive_media_upload.go
- shortcuts/common/validate_test.go
- shortcuts/common/permission_grant_test.go
- cmd/config/bind_test.go
- shortcuts/common/runner_scope_test.go
- cmd/profile/profile_test.go
@evandance
evandance
force-pushed
the
feat/errs-legacy-final-cleanup
branch
from
June 13, 2026 11:16
d03ae55 to
99ccd22
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/config/bind_test.go (1)
23-63:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftStrengthen
assertExitErrorto enforce typed metadata contract, not just type/message/hint.This helper currently verifies only category/message/hint, so subtype/param/cause regressions can pass silently across many bind error-path tests. Please assert
Subtypeviaerrs.ProblemOf, and assertParamviaerrors.As(*errs.ValidationError)where relevant.Suggested direction
type wantErrDetail struct { - Type string - Message string - Hint string + Type string + Subtype string + Message string + Hint string + Param string } func assertExitError(t *testing.T, err error, wantCode int, wantDetail wantErrDetail) { t.Helper() if err == nil { t.Fatal("expected error, got nil") } + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("error is not typed: %T: %v", err, err) + } + gotDetail := wantErrDetail{ + Type: string(p.Category), + Subtype: string(p.Subtype), + Message: p.Message, + Hint: p.Hint, + } + var ve *errs.ValidationError + if errors.As(err, &ve) { + gotDetail.Param = ve.Param + } // compare gotDetail vs wantDetail... }As per coding guidelines, error-path tests should assert typed metadata via
errs.ProblemOf(with cause preservation where applicable); based on learnings,Parammust be read from*errs.ValidationErrorinstead oferrs.ProblemOf.🤖 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/bind_test.go` around lines 23 - 63, The helper assertExitError currently only compares Category/Message/Hint so subtype/param/cause regressions slip by; update assertExitError to also assert the error's Subtype by calling errs.ProblemOf(err).Subtype (for both ValidationError and ConfigError branches) and to assert Param by extracting the concrete *errs.ValidationError via errors.As and comparing its Param field to the expected value; ensure the ExitCodeOf checks remain, and preserve existing failure messages/calls to t.Errorf/t.Fatalf so tests fail with clear diagnostics if Subtype or Param differ (use the same got/want formatting used for Message/Hint).Sources: Coding guidelines, Learnings
🤖 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/ERROR_CONTRACT.md`:
- Around line 417-421: The guidance text is internally contradictory: update the
paragraph so it consistently says the legacy output.ExitError envelope surface
(output.ErrValidation, output.ErrAuth, output.ErrNetwork, output.Errorf,
output.ErrWithHint, output.ErrAPI, output.WriteErrorEnvelope) has been removed
as legacy constructors, but clarify that the typed builder is the preferred
approach while explicitly permitting direct struct literals as an allowed (not
prohibited) migration path; ensure wording replaces "the builder is the only
way" with phrasing that the builder is preferred (recommended) and that
struct-literal construction remains allowed where previously documented.
- Around line 130-132: Summary: The flow docs incorrectly state Cobra usage
failures print plain text and exit 1, but the dispatcher classifies them into
the typed validation envelope and exits 2. Fix: update the ERRORS_CONTRACT.md
wording to reflect actual runtime behavior—state that Cobra
usage/flag/subcommand errors are intercepted by the dispatcher, emitted as the
typed validation JSON envelope on stderr, and terminate with exit code 2 (not
exit 1); reference the dispatcher classification and the "typed validation
envelope" terminology so consumers know to expect JSON+exit 2 for these cases.
---
Outside diff comments:
In `@cmd/config/bind_test.go`:
- Around line 23-63: The helper assertExitError currently only compares
Category/Message/Hint so subtype/param/cause regressions slip by; update
assertExitError to also assert the error's Subtype by calling
errs.ProblemOf(err).Subtype (for both ValidationError and ConfigError branches)
and to assert Param by extracting the concrete *errs.ValidationError via
errors.As and comparing its Param field to the expected value; ensure the
ExitCodeOf checks remain, and preserve existing failure messages/calls to
t.Errorf/t.Fatalf so tests fail with clear diagnostics if Subtype or Param
differ (use the same got/want formatting used for Message/Hint).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
i️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a2dfc4d-b1ec-44eb-b38a-6d1cd3c58d2e
📒 Files selected for processing (136)
.golangci.ymlcmd/api/api.gocmd/api/api_test.gocmd/auth/check_test.gocmd/auth/list.gocmd/auth/login_test.gocmd/completion/completion.gocmd/config/bind.gocmd/config/bind_test.gocmd/config/binder_test.gocmd/config/config_test.gocmd/config/init.gocmd/config/init_guard_test.gocmd/config/init_messages.gocmd/config/init_test.gocmd/doctor/doctor.gocmd/error_auth_hint.gocmd/event/consume.gocmd/event/format_helpers_test.gocmd/event/status_fail_on_orphan_test.gocmd/flag_suggest_test.gocmd/platform_bootstrap_test.gocmd/platform_guards.gocmd/platform_guards_test.gocmd/plugin_integration_test.gocmd/profile/add.gocmd/profile/list.gocmd/profile/profile_test.gocmd/profile/remove.gocmd/profile/rename.gocmd/profile/use.gocmd/prune.gocmd/prune_test.gocmd/root.gocmd/root_integration_test.gocmd/root_test.gocmd/schema/schema_test.gocmd/unknown_subcommand_test.gocmd/update/update.gocmd/update/update_test.goerrs/ERROR_CONTRACT.mderrs/raw.goerrs/raw_test.goerrs/types_test.goextension/platform/abort.goextension/platform/errors.gointernal/auth/errors.gointernal/auth/errors_test.gointernal/auth/uat_client.gointernal/client/api_errors_test.gointernal/client/client.gointernal/client/client_test.gointernal/cmdpolicy/aggregation_test.gointernal/cmdpolicy/apply.gointernal/cmdpolicy/engine.gointernal/cmdpolicy/source_label_test.gointernal/cmdutil/confirm.gointernal/cmdutil/confirm_test.gointernal/cmdutil/factory_default_test.gointernal/cmdutil/fileupload.gointernal/cmdutil/fileupload_test.gointernal/cmdutil/json.gointernal/cmdutil/json_test.gointernal/cmdutil/lang.gointernal/core/config.gointernal/core/config_test.gointernal/core/errors.gointernal/core/notconfigured.gointernal/core/notconfigured_test.gointernal/errcompat/promote.gointernal/errcompat/promote_auth.gointernal/errcompat/promote_auth_test.gointernal/errcompat/promote_test.gointernal/hook/install.gointernal/hook/install_test.gointernal/keychain/keychain.gointernal/keychain/keychain_darwin_test.gointernal/keychain/keychain_typed_error_test.gointernal/output/bare.gointernal/output/bare_test.gointernal/output/emit.gointernal/output/emit_test.gointernal/output/envelope.gointernal/output/errors.gointernal/output/errors_test.gointernal/output/exitcode.gointernal/output/jq.gointernal/output/lark_errors.gointernal/output/lark_errors_test.gointernal/output/ownership_recovery.gointernal/output/ownership_recovery_test.golint/errscontract/rule_no_legacy_common_helper_call.golint/errscontract/rule_no_legacy_envelope_literal.golint/errscontract/rule_no_legacy_runtime_api_call.golint/errscontract/rule_typed_error_completeness.golint/errscontract/rules_test.golint/errscontract/scan.goshortcuts/apps/apps_callapi_typed_test.goshortcuts/apps/apps_db_table_list_test.goshortcuts/base/record_upload_attachment.goshortcuts/calendar/calendar_test.goshortcuts/common/call_api_typed_test.goshortcuts/common/common.goshortcuts/common/drive_media_upload.goshortcuts/common/drive_media_upload_test.goshortcuts/common/drive_media_upload_typed_test.goshortcuts/common/permission_grant.goshortcuts/common/permission_grant_test.goshortcuts/common/runner.goshortcuts/common/runner_scope_test.goshortcuts/common/validate.goshortcuts/common/validate_test.goshortcuts/doc/doc_media_test.goshortcuts/doc/doc_media_upload.goshortcuts/drive/drive_add_comment_test.goshortcuts/drive/drive_errors.goshortcuts/drive/drive_import_common.goshortcuts/drive/drive_push.goshortcuts/drive/drive_upload.goshortcuts/minutes/minutes_download_test.goshortcuts/register.goshortcuts/register_brand_guard_test.goshortcuts/register_test.goshortcuts/sheets/backward/lark_sheets_float_images.goshortcuts/sheets/flag_schema_test.goshortcuts/sheets/lark_sheet_object_crud.goshortcuts/sheets/lark_sheet_write_cells.goshortcuts/slides/slides_media_upload.goshortcuts/task/task_shortcut_test.goshortcuts/task/tasklist_add_task_test.goshortcuts/vc/vc_notes_test.gotests/cli_e2e/apps/apps_access_scope_get_dryrun_test.gotests/cli_e2e/apps/apps_create_dryrun_test.gotests/cli_e2e/apps/apps_html_publish_dryrun_test.gotests/cli_e2e/apps/apps_update_dryrun_test.gotests/cli_e2e/config/bind_test.go
💤 Files with no reviewable changes (12)
- internal/core/errors.go
- internal/errcompat/promote_test.go
- internal/errcompat/promote_auth.go
- internal/errcompat/promote.go
- shortcuts/common/common.go
- internal/output/ownership_recovery.go
- internal/output/ownership_recovery_test.go
- internal/errcompat/promote_auth_test.go
- internal/output/lark_errors_test.go
- cmd/error_auth_hint.go
- internal/output/envelope.go
- internal/output/errors.go
✅ Files skipped from review due to trivial changes (11)
- shortcuts/minutes/minutes_download_test.go
- shortcuts/task/tasklist_add_task_test.go
- shortcuts/apps/apps_db_table_list_test.go
- shortcuts/drive/drive_add_comment_test.go
- lint/errscontract/rule_typed_error_completeness.go
- cmd/event/consume.go
- internal/cmdpolicy/engine.go
- errs/types_test.go
- shortcuts/doc/doc_media_test.go
- extension/platform/abort.go
- tests/cli_e2e/config/bind_test.go
🚧 Files skipped from review as they are similar to previous changes (104)
- tests/cli_e2e/apps/apps_update_dryrun_test.go
- shortcuts/apps/apps_callapi_typed_test.go
- shortcuts/vc/vc_notes_test.go
- shortcuts/drive/drive_import_common.go
- shortcuts/sheets/lark_sheet_write_cells.go
- cmd/profile/list.go
- cmd/event/status_fail_on_orphan_test.go
- internal/output/bare.go
- shortcuts/slides/slides_media_upload.go
- shortcuts/base/record_upload_attachment.go
- internal/output/bare_test.go
- cmd/profile/remove.go
- cmd/config/init_messages.go
- shortcuts/sheets/backward/lark_sheets_float_images.go
- shortcuts/calendar/calendar_test.go
- cmd/completion/completion.go
- shortcuts/common/drive_media_upload_typed_test.go
- lint/errscontract/scan.go
- internal/keychain/keychain_typed_error_test.go
- shortcuts/task/task_shortcut_test.go
- internal/output/emit_test.go
- tests/cli_e2e/apps/apps_create_dryrun_test.go
- tests/cli_e2e/apps/apps_html_publish_dryrun_test.go
- internal/client/client_test.go
- cmd/platform_guards_test.go
- cmd/auth/login_test.go
- cmd/event/format_helpers_test.go
- internal/auth/uat_client.go
- cmd/auth/list.go
- internal/cmdutil/factory_default_test.go
- shortcuts/drive/drive_push.go
- cmd/config/binder_test.go
- tests/cli_e2e/apps/apps_access_scope_get_dryrun_test.go
- cmd/flag_suggest_test.go
- shortcuts/doc/doc_media_upload.go
- internal/cmdutil/fileupload_test.go
- internal/output/lark_errors.go
- internal/cmdutil/fileupload.go
- internal/cmdutil/json.go
- internal/keychain/keychain.go
- cmd/profile/use.go
- shortcuts/register_test.go
- internal/auth/errors_test.go
- shortcuts/register.go
- cmd/profile/rename.go
- shortcuts/drive/drive_errors.go
- shortcuts/drive/drive_upload.go
- internal/client/api_errors_test.go
- cmd/config/init_test.go
- shortcuts/sheets/lark_sheet_object_crud.go
- cmd/config/bind.go
- lint/errscontract/rule_no_legacy_runtime_api_call.go
- cmd/platform_bootstrap_test.go
- internal/core/notconfigured.go
- cmd/plugin_integration_test.go
- internal/hook/install_test.go
- cmd/doctor/doctor.go
- internal/cmdpolicy/apply.go
- errs/raw_test.go
- internal/cmdpolicy/source_label_test.go
- internal/cmdutil/confirm.go
- cmd/profile/add.go
- shortcuts/common/permission_grant.go
- shortcuts/sheets/flag_schema_test.go
- cmd/prune.go
- cmd/update/update.go
- cmd/api/api.go
- shortcuts/common/call_api_typed_test.go
- internal/cmdutil/json_test.go
- cmd/prune_test.go
- cmd/config/config_test.go
- internal/core/config_test.go
- cmd/api/api_test.go
- shortcuts/register_brand_guard_test.go
- shortcuts/common/runner_scope_test.go
- internal/output/emit.go
- errs/raw.go
- internal/keychain/keychain_darwin_test.go
- internal/cmdpolicy/aggregation_test.go
- cmd/config/init_guard_test.go
- cmd/unknown_subcommand_test.go
- cmd/platform_guards.go
- internal/output/jq.go
- lint/errscontract/rule_no_legacy_envelope_literal.go
- cmd/profile/profile_test.go
- internal/core/notconfigured_test.go
- internal/cmdutil/confirm_test.go
- cmd/config/init.go
- cmd/root_integration_test.go
- lint/errscontract/rule_no_legacy_common_helper_call.go
- cmd/schema/schema_test.go
- internal/auth/errors.go
- shortcuts/common/drive_media_upload_test.go
- internal/output/errors_test.go
- lint/errscontract/rules_test.go
- internal/client/client.go
- .golangci.yml
- internal/core/config.go
- shortcuts/common/runner.go
- shortcuts/common/validate.go
- shortcuts/common/drive_media_upload.go
- cmd/update/update_test.go
- cmd/root.go
- shortcuts/common/validate_test.go
Comment on lines
+130
to
+132
Only the typed branch emits a JSON envelope on stderr. Untyped errors
(including Cobra's "required flag missing" / unknown subcommand messages)
print plain text and exit `1` — consumers must tolerate that fallback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flow docs currently misstate Cobra usage failures.
Line 130-132 says Cobra usage errors stay untyped/plain text with exit 1, but the dispatcher path classifies them into a typed validation envelope and exits 2. This mismatch makes the contract unreliable for script/agent consumers.
🤖 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/ERROR_CONTRACT.md` around lines 130 - 132, Summary: The flow docs
incorrectly state Cobra usage failures print plain text and exit 1, but the
dispatcher classifies them into the typed validation envelope and exits 2. Fix:
update the ERRORS_CONTRACT.md wording to reflect actual runtime behavior—state
that Cobra usage/flag/subcommand errors are intercepted by the dispatcher,
emitted as the typed validation JSON envelope on stderr, and terminate with exit
code 2 (not exit 1); reference the dispatcher classification and the "typed
validation envelope" terminology so consumers know to expect JSON+exit 2 for
these cases.
Consolidate all command error reporting onto the typed errs.* contract, remove the legacy error surface that predated it, and tighten the lint guards so the contract holds across the whole repository going forward. Every failure now reaches stderr as one envelope shape: a category, an optional subtype, a human- and agent-readable message, and a recovery hint, with invalid parameters listed under `params`. The legacy ExitError envelope, its constructors, and the boundary bridge that promoted untyped config and authorization errors are deleted, leaving a single path from error to wire. Predicate commands keep their silent-exit behavior through a dedicated signal that carries only an exit code. Infrastructure paths that still emitted ad-hoc envelopes — flag parsing, unknown commands and subcommands, plugin and policy guards, confirmation prompts, and auth/config failures — now classify into the same taxonomy. Exit codes are unchanged at every call site; the only user-visible change is the richer envelope on those paths. Enforcement is repo-wide rather than per-path: - The errscontract guards run by default everywhere instead of through a migration allowlist, so legacy envelopes cannot be reintroduced anywhere. - errorlint runs across the whole repository: every error wrap must use %w and every comparison must use errors.Is/errors.As, so interior wraps stay legal but can no longer break the chain the typed boundary relies on. - The errs-no-bare-wrap guard is keyed by structural prefix instead of an explicit per-domain allowlist, so new shortcut domains are covered without editing a list. It runs where forbidigo is enabled (the shortcut domains and the auth/config/service command groups); repo-wide chain integrity for the remaining command paths is carried by errorlint above.
@evandance
evandance
force-pushed
the
feat/errs-legacy-final-cleanup
branch
from
June 13, 2026 11:27
99ccd22 to
45923b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/cmdutil/factory_default_test.go (1)
111-117:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
errs.ProblemOfassertions to lock category/subtype contract.Line 111 currently validates concrete type and message, but the typed contract in tests should also assert problem metadata (
category/subtype) explicitly.Suggested patch
var cfgErr *errs.ConfigError if !errors.As(err, &cfgErr) { t.Fatalf("Config() error type = %T, want *errs.ConfigError", err) } + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("Config() should expose errs.Problem") + } + if p.Category != errs.CategoryConfig { + t.Fatalf("Config() category = %q, want %q", p.Category, errs.CategoryConfig) + } + if p.Subtype != errs.SubtypeNotConfigured { + t.Fatalf("Config() subtype = %q, want %q", p.Subtype, errs.SubtypeNotConfigured) + } if cfgErr.Message != `profile "missing" not found` { t.Fatalf("Config() error message = %q, want %q", cfgErr.Message, `profile "missing" not found`) }As per coding guidelines, error-path tests must assert typed metadata via
errs.ProblemOf(category/subtype/param) rather than relying on message text alone.🤖 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/cmdutil/factory_default_test.go` around lines 111 - 117, The test currently checks the concrete error type cfgErr (*errs.ConfigError) and message text; update it to also assert typed problem metadata using errs.ProblemOf: call p := errs.ProblemOf(cfgErr) and assert p.Category == "config" (or the expected category) and p.Subtype == "profile-not-found" (and any expected Param values) to lock the category/subtype contract, keeping the existing type check and message assertion in place.Source: Coding guidelines
🤖 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/core/notconfigured.go`:
- Line 49: The returned error currently stringifies the underlying error via
errs.NewConfigError(subtype, "failed to load config: %v", err) but drops the
unwrap chain; update the return to preserve the original error by calling
.WithCause(err) on the created error (i.e., attach the original err using
WithCause after errs.NewConfigError) so errors.Is / errors.Unwrap continue to
work.
---
Outside diff comments:
In `@internal/cmdutil/factory_default_test.go`:
- Around line 111-117: The test currently checks the concrete error type cfgErr
(*errs.ConfigError) and message text; update it to also assert typed problem
metadata using errs.ProblemOf: call p := errs.ProblemOf(cfgErr) and assert
p.Category == "config" (or the expected category) and p.Subtype ==
"profile-not-found" (and any expected Param values) to lock the category/subtype
contract, keeping the existing type check and message assertion in place.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
i️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b71e6287-a4d0-49aa-8f2d-34146f01e554
📒 Files selected for processing (137)
.golangci.ymlcmd/api/api.gocmd/api/api_test.gocmd/auth/check_test.gocmd/auth/list.gocmd/auth/login_test.gocmd/completion/completion.gocmd/config/bind.gocmd/config/bind_test.gocmd/config/binder_test.gocmd/config/config_test.gocmd/config/init.gocmd/config/init_guard_test.gocmd/config/init_messages.gocmd/config/init_test.gocmd/doctor/doctor.gocmd/error_auth_hint.gocmd/event/consume.gocmd/event/format_helpers_test.gocmd/event/status_fail_on_orphan_test.gocmd/flag_suggest_test.gocmd/platform_bootstrap_test.gocmd/platform_guards.gocmd/platform_guards_test.gocmd/plugin_integration_test.gocmd/profile/add.gocmd/profile/list.gocmd/profile/profile_test.gocmd/profile/remove.gocmd/profile/rename.gocmd/profile/use.gocmd/prune.gocmd/prune_test.gocmd/root.gocmd/root_integration_test.gocmd/root_test.gocmd/schema/schema_test.gocmd/unknown_subcommand_test.gocmd/update/update.gocmd/update/update_test.goerrs/ERROR_CONTRACT.mderrs/raw.goerrs/raw_test.goerrs/types_test.goextension/platform/abort.goextension/platform/errors.gointernal/auth/errors.gointernal/auth/errors_test.gointernal/auth/uat_client.gointernal/client/api_errors_test.gointernal/client/client.gointernal/client/client_test.gointernal/cmdpolicy/aggregation_test.gointernal/cmdpolicy/apply.gointernal/cmdpolicy/engine.gointernal/cmdpolicy/source_label_test.gointernal/cmdutil/confirm.gointernal/cmdutil/confirm_test.gointernal/cmdutil/factory_default_test.gointernal/cmdutil/fileupload.gointernal/cmdutil/fileupload_test.gointernal/cmdutil/json.gointernal/cmdutil/json_test.gointernal/cmdutil/lang.gointernal/core/config.gointernal/core/config_test.gointernal/core/errors.gointernal/core/notconfigured.gointernal/core/notconfigured_test.gointernal/errcompat/promote.gointernal/errcompat/promote_auth.gointernal/errcompat/promote_auth_test.gointernal/errcompat/promote_test.gointernal/hook/install.gointernal/hook/install_test.gointernal/keychain/keychain.gointernal/keychain/keychain_darwin_test.gointernal/keychain/keychain_typed_error_test.gointernal/output/bare.gointernal/output/bare_test.gointernal/output/emit.gointernal/output/emit_test.gointernal/output/envelope.gointernal/output/errors.gointernal/output/errors_test.gointernal/output/exitcode.gointernal/output/jq.gointernal/output/lark_errors.gointernal/output/lark_errors_test.gointernal/output/ownership_recovery.gointernal/output/ownership_recovery_test.golint/errscontract/rule_no_legacy_common_helper_call.golint/errscontract/rule_no_legacy_envelope_literal.golint/errscontract/rule_no_legacy_runtime_api_call.golint/errscontract/rule_typed_error_completeness.golint/errscontract/rules_test.golint/errscontract/scan.goshortcuts/apps/apps_callapi_typed_test.goshortcuts/apps/apps_db_table_list_test.goshortcuts/base/record_upload_attachment.goshortcuts/calendar/calendar_test.goshortcuts/common/call_api_typed_test.goshortcuts/common/common.goshortcuts/common/drive_media_upload.goshortcuts/common/drive_media_upload_test.goshortcuts/common/drive_media_upload_typed_test.goshortcuts/common/permission_grant.goshortcuts/common/permission_grant_test.goshortcuts/common/runner.goshortcuts/common/runner_scope_test.goshortcuts/common/validate.goshortcuts/common/validate_test.goshortcuts/doc/doc_media_test.goshortcuts/doc/doc_media_upload.goshortcuts/drive/drive_add_comment_test.goshortcuts/drive/drive_errors.goshortcuts/drive/drive_import_common.goshortcuts/drive/drive_push.goshortcuts/drive/drive_upload.goshortcuts/minutes/minutes_download_test.goshortcuts/register.goshortcuts/register_brand_guard_test.goshortcuts/register_test.goshortcuts/sheets/backward/lark_sheets_float_images.goshortcuts/sheets/flag_schema_test.goshortcuts/sheets/lark_sheet_object_crud.goshortcuts/sheets/lark_sheet_write_cells.goshortcuts/slides/slides_media_upload.goshortcuts/task/task_shortcut_test.goshortcuts/task/tasklist_add_task_test.goshortcuts/vc/vc_notes_test.gotests/cli_e2e/apps/apps_access_scope_get_dryrun_test.gotests/cli_e2e/apps/apps_create_dryrun_test.gotests/cli_e2e/apps/apps_html_publish_dryrun_test.gotests/cli_e2e/apps/apps_update_dryrun_test.gotests/cli_e2e/config/bind_test.gotests/cli_e2e/drive/drive_push_dryrun_test.go
💤 Files with no reviewable changes (12)
- internal/errcompat/promote.go
- internal/core/errors.go
- internal/errcompat/promote_test.go
- internal/output/ownership_recovery.go
- internal/errcompat/promote_auth.go
- internal/errcompat/promote_auth_test.go
- internal/output/ownership_recovery_test.go
- shortcuts/common/common.go
- internal/output/lark_errors_test.go
- internal/output/envelope.go
- internal/output/errors.go
- cmd/error_auth_hint.go
✅ Files skipped from review due to trivial changes (13)
- extension/platform/errors.go
- cmd/event/consume.go
- shortcuts/doc/doc_media_test.go
- shortcuts/apps/apps_db_table_list_test.go
- extension/platform/abort.go
- internal/cmdpolicy/engine.go
- cmd/config/binder_test.go
- shortcuts/drive/drive_add_comment_test.go
- tests/cli_e2e/config/bind_test.go
- errs/raw.go
- lint/errscontract/rule_typed_error_completeness.go
- errs/types_test.go
- shortcuts/apps/apps_callapi_typed_test.go
🚧 Files skipped from review as they are similar to previous changes (105)
- lint/errscontract/scan.go
- shortcuts/register.go
- internal/cmdutil/lang.go
- shortcuts/slides/slides_media_upload.go
- tests/cli_e2e/apps/apps_html_publish_dryrun_test.go
- cmd/completion/completion.go
- shortcuts/task/tasklist_add_task_test.go
- internal/output/bare.go
- shortcuts/drive/drive_import_common.go
- shortcuts/common/drive_media_upload_typed_test.go
- shortcuts/doc/doc_media_upload.go
- internal/keychain/keychain_darwin_test.go
- cmd/platform_bootstrap_test.go
- cmd/auth/check_test.go
- shortcuts/minutes/minutes_download_test.go
- cmd/auth/list.go
- shortcuts/drive/drive_upload.go
- cmd/profile/list.go
- shortcuts/task/task_shortcut_test.go
- internal/output/bare_test.go
- shortcuts/sheets/lark_sheet_object_crud.go
- cmd/prune_test.go
- shortcuts/drive/drive_errors.go
- shortcuts/vc/vc_notes_test.go
- internal/core/config_test.go
- internal/cmdpolicy/source_label_test.go
- internal/output/exitcode.go
- cmd/event/status_fail_on_orphan_test.go
- internal/cmdutil/fileupload.go
- tests/cli_e2e/apps/apps_update_dryrun_test.go
- internal/output/jq.go
- lint/errscontract/rule_no_legacy_runtime_api_call.go
- internal/cmdutil/json.go
- shortcuts/common/call_api_typed_test.go
- cmd/profile/rename.go
- cmd/auth/login_test.go
- shortcuts/sheets/flag_schema_test.go
- internal/cmdutil/confirm_test.go
- internal/auth/uat_client.go
- shortcuts/calendar/calendar_test.go
- shortcuts/register_test.go
- cmd/prune.go
- cmd/profile/use.go
- tests/cli_e2e/apps/apps_create_dryrun_test.go
- internal/core/notconfigured_test.go
- internal/hook/install_test.go
- internal/cmdutil/json_test.go
- shortcuts/register_brand_guard_test.go
- cmd/profile/remove.go
- shortcuts/common/permission_grant.go
- cmd/platform_guards_test.go
- internal/output/emit_test.go
- cmd/config/init_messages.go
- internal/cmdpolicy/aggregation_test.go
- shortcuts/base/record_upload_attachment.go
- lint/errscontract/rule_no_legacy_envelope_literal.go
- cmd/config/init_guard_test.go
- internal/core/config.go
- internal/keychain/keychain_typed_error_test.go
- shortcuts/common/runner_scope_test.go
- internal/output/errors_test.go
- cmd/api/api.go
- cmd/schema/schema_test.go
- shortcuts/sheets/lark_sheet_write_cells.go
- cmd/profile/add.go
- shortcuts/drive/drive_push.go
- internal/auth/errors.go
- internal/auth/errors_test.go
- cmd/config/init_test.go
- shortcuts/sheets/backward/lark_sheets_float_images.go
- cmd/flag_suggest_test.go
- cmd/config/config_test.go
- internal/client/api_errors_test.go
- cmd/update/update.go
- internal/client/client_test.go
- shortcuts/common/permission_grant_test.go
- internal/output/emit.go
- cmd/event/format_helpers_test.go
- internal/cmdutil/fileupload_test.go
- internal/keychain/keychain.go
- errs/raw_test.go
- cmd/config/bind.go
- internal/cmdutil/confirm.go
- internal/client/client.go
- cmd/doctor/doctor.go
- shortcuts/common/runner.go
- cmd/plugin_integration_test.go
- internal/cmdpolicy/apply.go
- cmd/root_integration_test.go
- cmd/update/update_test.go
- lint/errscontract/rules_test.go
- lint/errscontract/rule_no_legacy_common_helper_call.go
- internal/hook/install.go
- .golangci.yml
- shortcuts/common/validate.go
- cmd/unknown_subcommand_test.go
- shortcuts/common/drive_media_upload_test.go
- shortcuts/common/drive_media_upload.go
- cmd/platform_guards.go
- cmd/root_test.go
- cmd/profile/profile_test.go
- shortcuts/common/validate_test.go
- cmd/root.go
- cmd/config/bind_test.go
- internal/output/lark_errors.go
if isMalformedConfigError(err) {
subtype = errs.SubtypeInvalidConfig
}
return nil, errs.NewConfigError(subtype, "failed to load config: %v", err)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preserve the original load failure with .WithCause(err).
Line 49 currently stringifies the underlying error into Message but drops the unwrap chain, which weakens downstream errors.Is / errors.Unwrap behavior.
Suggested patch
- return nil, errs.NewConfigError(subtype, "failed to load config: %v", err) + return nil, errs.NewConfigError(subtype, "failed to load config: %v", err).WithCause(err)
As per coding guidelines, preserve underlying errors with .WithCause(err) so errors.Is / errors.Unwrap continue working.
📝 Committable suggestion
!!️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
return nil, errs.NewConfigError(subtype, "failed to load config: %v", err)
return nil, errs.NewConfigError(subtype, "failed to load 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 `@internal/core/notconfigured.go` at line 49, The returned error currently
stringifies the underlying error via errs.NewConfigError(subtype, "failed to
load config: %v", err) but drops the unwrap chain; update the return to preserve
the original error by calling .WithCause(err) on the created error (i.e., attach
the original err using WithCause after errs.NewConfigError) so errors.Is /
errors.Unwrap continue to work.
Source: Coding guidelines
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
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.
Uh oh!
There was an error while loading. Please reload this page.
Summary
Completes the migration to the typed error contract: every command now reports failures through a single
errs.*envelope shape (category, optional subtype, human- and agent-readable message, recovery hint, andparamsfor invalid arguments). The legacyoutput.ExitErrorsurface and the boundary bridge that promoted untyped config/auth errors are removed, so there is exactly one path from an error to the wire. Exit codes are unchanged for business, API, auth, and config paths; cobra usage failures (missing required flag, unknown command, bad arguments) now emit the typed validation envelope at exit 2 — consistent with the existing unknown-flag and unknown-subcommand guards — instead of cobra's plain-text exit 1. The richer envelope also reaches plugin/policy guards and confirmation prompts.Changes
output.ExitError/ErrDetail/ErrorEnvelopetypes, their constructors,WriteErrorEnvelope, andClassifyLarkError; carry predicate silent-exits on a dedicatedoutput.BareError.errs.*at their origin ininternal/authandinternal/core, and remove theinternal/errcompatpromotion bridge.code:0body is never swallowed (shortcuts/common/runner.go).cmd/root.go).config init --nameconflicts as typed validation errors instead of internal storage failures (cmd/config/init.go).errorlintrequires%wwraps anderrors.Is/As; and the typed-final-error guard spans every command group, shortcut, and event boundary by structural prefix so new domains are covered automatically (.golangci.yml,lint/errscontract).paramsfield and the typed-only contract inerrs/ERROR_CONTRACT.md.Test Plan
make unit-testpassedlark-cli api GET /open-apis/nonexistent/v1/routeand an invalid-tokenlark-cli apicall confirmed typedapi/authenticationenvelopes with correct exit codes; missing-required-flag and unknown-command calls confirmed typedvalidationenvelopes at exit 2Related Issues
N/A
Summary by CodeRabbit
New Features
Bug Fixes
Chores