refactor: retire legacy error envelopes and enforce typed contract#1449
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRepo-wide migration replacing legacy internal/output envelope helpers with typed github.com/larksuite/cli/errs, adding raw/bare primitives, updating callers in commands and shortcuts, and aligning tests, docs, and lint rules. ChangesTyped error migration across CLI and internals
Possibly related PRs
Estimated code review effort: 🎯 4 (Complex) | ⏱️ ~45 minutes
✨ Finishing Touches🧪 Generate unit tests (beta)
|
github-actions
Bot
added
domain/base
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1449 +/- ##
==========================================
+ Coverage 73.52% 73.72% +0.19%
==========================================
Files 783 780 -3
Lines 74630 74140 -490
==========================================
- Hits 54874 54659 -215
+ Misses 15662 15420 -242
+ Partials 4094 4061 -33 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@c7674079ec32a7ce9bdf1f1c06dfb27707113d25🧩 Skill updatenpx skills add larksuite/cli#feat/errs-legacy-final-cleanup -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
ℹ️ 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
force-pushed
the
feat/errs-legacy-final-cleanup
branch
2 times, most recently
from
2091241 to
d03ae55
Compare
There was a problem hiding this comment.
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
ℹ️ 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
force-pushed
the
feat/errs-legacy-final-cleanup
branch
from
d03ae55 to
99ccd22
Compare
There was a problem hiding this comment.
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
ℹ️ 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
evandance
force-pushed
the
feat/errs-legacy-final-cleanup
branch
from
99ccd22 to
45923b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/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
ℹ️ 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
evandance
force-pushed
the
feat/errs-legacy-final-cleanup
branch
from
45923b8 to
97b7c9d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/profile/remove.go`:
- Around line 44-45: The validation error returned in the profile removal
function is missing parameter metadata that helps callers understand which user
input caused the error. Chain the `.WithParam()` method to the
errs.NewValidationError call in the return statement (where the error is raised
for an invalid profile name), passing the appropriate command-line flag name
(likely "--profile" or similar) as the parameter to identify which input
triggered the validation failure.
In `@errs/ERROR_CONTRACT.md`:
- Around line 121-133: The ERROR_CONTRACT.md document contains contradictory
statements about the runtime contract for error handling. The sections at lines
217-221 and 552-553 describe Cobra/legacy behavior and the *output.ExitError
type inconsistently with the anchor section (121-133) and the stated "migration
complete" objective. Review all three locations and align them to describe a
single, consistent runtime contract: either update lines 217-221 and 552-553 to
reflect the current dispatch behavior shown in the anchor (where untyped/Cobra
errors produce plain-text exit 1), or remove language suggesting ExitError is
"pending removal" if that contradicts the actual current contract. Ensure the
flow, shell, and contract sections all describe the same error-handling behavior
and migration state throughout the document.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6467ba7d-0396-4d20-b5aa-cc774399e587
📒 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_auth_test.go
- internal/core/errors.go
- internal/output/ownership_recovery_test.go
- internal/output/ownership_recovery.go
- internal/errcompat/promote_test.go
- internal/errcompat/promote_auth.go
- shortcuts/common/common.go
- internal/errcompat/promote.go
- internal/output/errors.go
- internal/output/envelope.go
- internal/output/lark_errors_test.go
- cmd/error_auth_hint.go
✅ Files skipped from review due to trivial changes (15)
- shortcuts/apps/apps_callapi_typed_test.go
- shortcuts/slides/slides_media_upload.go
- internal/cmdutil/lang.go
- shortcuts/task/tasklist_add_task_test.go
- internal/cmdpolicy/engine.go
- extension/platform/abort.go
- cmd/event/consume.go
- shortcuts/doc/doc_media_upload.go
- lint/errscontract/rule_typed_error_completeness.go
- shortcuts/drive/drive_add_comment_test.go
- shortcuts/doc/doc_media_test.go
- internal/core/config_test.go
- errs/types_test.go
- tests/cli_e2e/config/bind_test.go
- shortcuts/apps/apps_db_table_list_test.go
🚧 Files skipped from review as they are similar to previous changes (98)
- extension/platform/errors.go
- shortcuts/task/task_shortcut_test.go
- tests/cli_e2e/apps/apps_update_dryrun_test.go
- internal/cmdutil/factory_default_test.go
- cmd/profile/use.go
- tests/cli_e2e/apps/apps_access_scope_get_dryrun_test.go
- tests/cli_e2e/apps/apps_create_dryrun_test.go
- shortcuts/minutes/minutes_download_test.go
- tests/cli_e2e/drive/drive_push_dryrun_test.go
- internal/output/bare_test.go
- internal/cmdutil/json.go
- cmd/auth/list.go
- internal/output/emit_test.go
- cmd/event/status_fail_on_orphan_test.go
- cmd/platform_guards_test.go
- cmd/auth/check_test.go
- cmd/config/binder_test.go
- internal/auth/uat_client.go
- cmd/profile/rename.go
- shortcuts/register.go
- internal/keychain/keychain_darwin_test.go
- internal/cmdutil/confirm.go
- shortcuts/calendar/calendar_test.go
- cmd/prune.go
- cmd/platform_bootstrap_test.go
- internal/cmdpolicy/aggregation_test.go
- shortcuts/register_brand_guard_test.go
- internal/output/bare.go
- internal/auth/errors_test.go
- cmd/profile/list.go
- internal/output/jq.go
- cmd/config/init_test.go
- shortcuts/common/permission_grant_test.go
- cmd/config/init_messages.go
- internal/client/api_errors_test.go
- cmd/update/update.go
- shortcuts/vc/vc_notes_test.go
- shortcuts/base/record_upload_attachment.go
- cmd/config/init_guard_test.go
- internal/output/exitcode.go
- cmd/prune_test.go
- shortcuts/sheets/flag_schema_test.go
- internal/keychain/keychain.go
- internal/cmdutil/json_test.go
- cmd/doctor/doctor.go
- shortcuts/drive/drive_upload.go
- shortcuts/register_test.go
- internal/client/client_test.go
- errs/raw.go
- cmd/event/format_helpers_test.go
- cmd/schema/schema_test.go
- errs/raw_test.go
- internal/keychain/keychain_typed_error_test.go
- cmd/config/config_test.go
- cmd/config/init.go
- shortcuts/sheets/lark_sheet_write_cells.go
- cmd/completion/completion.go
- lint/errscontract/scan.go
- cmd/profile/add.go
- internal/core/notconfigured.go
- internal/cmdutil/confirm_test.go
- lint/errscontract/rule_no_legacy_runtime_api_call.go
- internal/hook/install.go
- internal/core/config.go
- tests/cli_e2e/apps/apps_html_publish_dryrun_test.go
- internal/hook/install_test.go
- shortcuts/drive/drive_push.go
- internal/client/client.go
- shortcuts/common/permission_grant.go
- shortcuts/sheets/backward/lark_sheets_float_images.go
- cmd/plugin_integration_test.go
- internal/output/emit.go
- internal/auth/errors.go
- internal/cmdutil/fileupload.go
- shortcuts/common/runner.go
- internal/core/notconfigured_test.go
- cmd/profile/profile_test.go
- cmd/flag_suggest_test.go
- cmd/platform_guards.go
- shortcuts/common/call_api_typed_test.go
- cmd/update/update_test.go
- cmd/unknown_subcommand_test.go
- shortcuts/drive/drive_errors.go
- shortcuts/common/drive_media_upload_test.go
- shortcuts/common/runner_scope_test.go
- cmd/root.go
- lint/errscontract/rule_no_legacy_envelope_literal.go
- cmd/api/api_test.go
- internal/output/lark_errors.go
- lint/errscontract/rule_no_legacy_common_helper_call.go
- cmd/api/api.go
- internal/cmdpolicy/source_label_test.go
- lint/errscontract/rules_test.go
- cmd/config/bind_test.go
- shortcuts/common/drive_media_upload.go
- shortcuts/common/validate_test.go
- cmd/root_integration_test.go
- cmd/root_test.go
evandance
force-pushed
the
feat/errs-legacy-final-cleanup
branch
5 times, most recently
from
c61c0d9 to
dd67317
Compare
evandance
force-pushed
the
feat/errs-legacy-final-cleanup
branch
from
dd67317 to
1943a83
Compare
evandance
force-pushed
the
feat/errs-legacy-final-cleanup
branch
from
1943a83 to
05aeefc
Compare
evandance
force-pushed
the
feat/errs-legacy-final-cleanup
branch
2 times, most recently
from
b66ae9c to
726111b
Compare
PR Quality SummaryThe semantic review system could not produce a fully trusted result. This is not reported as a code defect. System status
|
Consolidate all command error reporting onto the typed errs.* contract, remove the legacy error surface that predated it, and tighten the lint guards so the contract holds across the whole repository going forward. Every failure now reaches stderr as one envelope shape: a category, an optional subtype, a human- and agent-readable message, and a recovery hint, with invalid parameters listed under `params`. The legacy ExitError envelope, its constructors, and the boundary bridge that promoted untyped config and authorization errors are deleted, leaving a single path from error to wire. Predicate commands keep their silent-exit behavior through a dedicated signal that carries only an exit code. Infrastructure paths that still emitted ad-hoc envelopes — flag parsing, unknown commands and subcommands, plugin and policy guards, confirmation prompts, and auth/config failures — now classify into the same taxonomy. Business, API, auth, and config exit codes are preserved; the one behavioral change is that Cobra usage failures (missing required flag, unknown command, bad arguments) now emit the typed validation envelope and exit 2, matching the explicit flag and subcommand guards, instead of Cobra's plain-text exit 1. Enforcement is repo-wide rather than per-path: - The errscontract guards run by default everywhere instead of through a migration allowlist, so legacy envelopes cannot be reintroduced anywhere. - errorlint runs across the whole repository: every error wrap must use %w and every comparison must use errors.Is/errors.As, so interior wraps stay legal but can no longer break the chain the typed boundary relies on. - The errs-no-bare-wrap guard is keyed by structural prefix instead of an explicit per-domain allowlist, so new shortcut domains are covered without editing a list. It runs where forbidigo is enabled (the shortcut domains and the auth/config/service command groups); repo-wide chain integrity for the remaining command paths is carried by errorlint above.
evandance
force-pushed
the
feat/errs-legacy-final-cleanup
branch
from
726111b to
a0160fb
Compare
The api and service success path now emits the {"ok":true} envelope, so the
cli_e2e workflow assertions that still expected the old {"code":0} shape via
AssertStdoutStatus(t, 0) fail once they run with live credentials. Switch those
workflow assertions to AssertStdoutStatus(t, true); the fake-payload helper test
in core_test.go keeps its code-shape assertion.
liangshuo-1
approved these changes
Jun 17, 2026
Merged
xiongyuanwen-byted
added a commit
that referenced
this pull request
Jun 22, 2026
The merge from main brought in #1449 (retire legacy error envelopes), which removed output.ExitError / output.ErrDetail and forbids constructing them. Port tablePutPartial off the legacy envelope: - no sheets written -> typed errs.APIError (plain failure) - some sheets written -> ok:false result via runtime.OutPartialFailure carrying written_sheets, returning the partial-failure exit signal Also fix two drifts the same merge introduced: - regenerate flag_defs_gen.go to match the committed flag-defs.json - update the --max-chars flag test to assert visible (no longer hidden)
xiongyuanwen-byted
added a commit
that referenced
this pull request
Jun 22, 2026
The merge from main brought in #1449 (retire legacy error envelopes), which removed output.ExitError / output.ErrDetail and forbids constructing them. Port tablePutPartial off the legacy envelope: - no sheets written -> typed errs.APIError (plain failure) - some sheets written -> ok:false result via runtime.OutPartialFailure carrying written_sheets, returning the partial-failure exit signal Also fix two drifts the same merge introduced: - regenerate flag_defs_gen.go to match the committed flag-defs.json - update the --max-chars flag test to assert visible (no longer hidden)
xiongyuanwen-byted
added a commit
that referenced
this pull request
Jun 25, 2026
…t, skill refresh (#1355) * feat(sheets): add +sheet-show-gridline / +sheet-hide-gridline shortcuts * docs(sheets): strengthen lark-sheets references for common editing pitfalls Add targeted guidance to six lark-sheets references to reduce frequent mistakes when editing spreadsheets through the CLI: - write-cells: sanity-check units / dimension conversion / quantity factors before formula writes (formulas can run clean yet be off by a factor); keep derived output off original data columns to avoid clobbering source - core-operations: prefer live formulas for derived values even when "live update" is not explicitly requested; scope rewrite/transform precisely so rows/columns that should stay unchanged are kept 1:1; treat header-stated format rules as checklist items; confirm the artifact file actually exists before finishing; write back bare values from local scripts - visual-standards: apply border/header formatting on explicit request and identify the real header row; keep font size consistent with the source - range-operations: keep total column width within A4 for printing - read-data: dedup/compare long numbers via raw values, not csv formatted display (scientific notation collapses distinct numbers and causes false duplicates) - chart: format date/number axes via source-cell number_format; place charts outside the data area so they do not cover existing data * feat(sheets): implement table-put/table-get and sync skill specs - Add lark_sheet_table_io.go with +table-put / +table-get and tests - Refactor read-data; extend workbook; register new shortcuts - Sync generated flag defs/schemas (go:embed) from sheet-skill-spec - Sync skill references (write-cells numeric-column guidance, plus read-data / workbook / chart updates) * docs(sheets): surface typed-write path at the write-decision point Quick-ref table (SKILL.md, the first decision point) had no +table-put and gated typed writes on "DataFrame", so a model holding a Counter/list/dict would fall back to +csv-put and silently lose number/date fidelity. - split csv-put row to plain-text values (no numeric/date semantics) - add +table-put row for typed writes into an existing sheet - add +workbook-create --sheets row for create + typed write in one shot - add judgment note: number/amount/date/percent/count -> +table-put (or +workbook-create --sheets when the workbook does not exist yet); plain text -> +csv-put - reframe write-cells scenario row to lead with numeric semantics - point new-table writes at +workbook-create --sheets (one shot) instead of the create-empty-then-table-put two-step Synced from sheet-skill-spec canonical (generate:cli + sync:cli). * docs(sheets): sync SKILL.md (drop "not for local Excel" caveat) Mirror the upstream sheet-skill-spec change removing the "not applicable to local Excel files" tail from the sheets skill and reference descriptions. * docs(sheets): sync SKILL.md (drop "Feishu sheets only" caveat) Mirror the upstream sheet-skill-spec change removing the "applies to Feishu sheets only" tail from the 14 sheet reference descriptions. * feat(sheets): add +workbook-import wrapping the drive import core Import a local xlsx/xls/csv as a new spreadsheet by delegating to the shared drive import flow with the target type pinned to sheet. Refactor drive +import to expose ImportParams / ValidateImport / PlanImportDryRun / RunImport (behavior unchanged, existing drive tests still cover it); sheets reuses them. Regenerate flag_defs_gen.go and sync the spec mirror. * refactor(sheets): reuse the drive export core in +workbook-export Replace +workbook-export's parallel export-task implementation with the shared drive ExportParams/RunExport core (pinned to type=sheet). Drops ~90 lines of duplicated poll/download code; +workbook-export now inherits drive's ctx cancellation, resume-on-timeout, filename sanitize/overwrite, and the full set of export status labels. The output contract aligns with drive's (adds ready/downloaded/doc_type; saved_path preserved). Also normalize an empty drive --output-dir to "." so drive +export behavior is unchanged, and fix the sheets export e2e to call +workbook-export instead of a nonexistent +export. * docs(sheets): keep original column widths; align chart axis with requested metric - range-operations: only widen new / overflowing columns; never recompute or shrink the widths of existing columns (any blanket resize, even by 1px, breaks the original visual format) - chart: when the user asks for a share / percentage, the value axis should be a percentage (pie, or stack.percentage on bar/column) rather than raw counts * docs(sheets): reword guidance to avoid eval-specific phrasing Replace scoring-framework wording in the examples with plain functional consequences (e.g. "not delivered", "goes stale when the source changes", "breaks the original visual format"), so the references stay agent-facing. * docs: add lark sheets financial modeling guidance * docs(sheets): align write-cells reference with the generated output Bring the hand-applied write-cells example in line with the spec-generated reference so the CLI mirror is byte-identical to the canonical source. * docs(sheets): align +csv-put help with formula support Sync the formula-support wording from sheet-skill-spec (flag-defs, skill references) and update the hand-authored cobra Description and comment for +csv-put. +csv-put evaluates a leading-= cell as a formula via set_range_from_csv; descriptions only, no behavior change. * docs(sheets): fix invalid +dim-insert example in chart reference The chart reference's placement example used non-existent flags --dimension/--start/--end for +dim-insert. The real signature is --position (required) + --count (required); copying the example fails Validate with "--position is required". Replace it with +dim-insert --position V --count 6 (insert 6 columns before V, i.e. after U), aligning with the sheet-structure reference. * docs(sheets): chart coordinate base / quoting + filter condition enums Sync three reference-doc corrections from the spec source: 1. chart: label position.row as 0-based (first row = row:0), distinct from the 1-based row numbers used by A1 ranges and +dim-insert --position, removing the row-base ambiguity. 2. chart: convert the three runnable examples whose JSON contains a quoted sheet prefix ('Sheet1'!A1) from inline single-quoted --properties '{...}' to a stdin heredoc (--properties - <<'JSON'). Inside an inline single-quoted string bash strips the inner quotes around the sheet name (and splits names with spaces into words), corrupting the JSON; a quoted heredoc delimiter performs no shell substitution and preserves it. Adds a short note on the pitfall. 3. filter / filter-view: add the full conditions[].type x compare_type enum table (text / number / multiValue / color and their respective compare_type values and values shape), and call out the equals/notEquals (with s) vs equal/notEqual (no s) gotcha. The docs previously only showed two values via examples. * docs(sheets): label +sheet-create --index as 0-based The base flag description for +sheet-create's --index omitted the coordinate base, while its siblings +sheet-move ("Target position (0-based)") and +sheet-copy already state 0-based. Align the description so the index base is unambiguous. Synced from the spec source (flag-defs.json + workbook reference). * fix(sheets): regenerate flag defs and fix asasalint in table io * feat(sheets): add counta to chart aggregateType enum Add `counta` (count non-empty cells, incl. text) to manage_chart_object dim2.series[].aggregateType in the chart flag schema. `count` only counts numeric cells, so counting occurrences of a text/category column renders an empty chart; `counta` enables category frequency counts. Synced from the sheet-skill-spec canonical schema. * feat(sheets): make --target-position and --range mutually exclusive on +pivot-create Both flags map to the same wire field (properties.range), so passing non-default values for both is ambiguous. Mirror the --target-sheet-id / --target-sheet-name mutex pattern: --target-position takes priority over --range, and supplying both with non-default values is rejected up front with a typed FlagErrorf. --target-position=A1 is the documented default and is treated as "not set". Add a symmetric validateCreateInput hook on objectCRUDSpec (alongside the existing validateUpdateInput), wire it into objectCreateInput, and inject the pivot-specific check on pivotSpec. * feat(sheets): rework +workbook-create flags and --styles - --values builds a type-less typed payload, writing through --sheets' batched set_cell_range path (raw passthrough preserves auto-detect; large tables batch; big ints via json.Number) - drop --headers (subsumed by --values first row) and --header-style (typed header no longer auto-bold; use --styles instead) - styles: deep-merge overlapping cell_styles/border_styles fields (was wholesale-replace which dropped fields); add manual border_styles validation (style/weight enums + sides) since --styles is on parseJSONFlagSkip and bypasses the schema validator - regenerate flag-defs/flag-schemas/skills mirror from sheet-skill-spec (--styles flag + full per-side border schema) * fix(sheets): add mention_type enum to set_cell_range cells schema Constrain rich_text mention_type to the proto MENTION_FILE_TYPE set so a file @mention with an out-of-enum value (e.g. 6 = cloud shared folder) is rejected by the schema validator before it reaches the server and fails pb serialization ("mentionFileInfo.fileType: enum value expected"). - data/flag-schemas.json: mention_type gains enum + per-value description - lark_sheet_write_cells_test.go: cover reject (6) + allow (0 / 2 / 22) * feat(sheets): implement pandas-split --sheets protocol for +table-put/+table-get/+workbook-create Synced from sheet-skill-spec canonical (cli:table_put schema + references). +table-put/+workbook-create accept the new shape via a tableSheetIn -> tableSheetSpec normalize step (dtype string -> internal type/format mapping). +table-get emits the same shape so the writer's df_to_sheet and the reader's sheet_to_df round-trip cleanly. isoDateToSerial now accepts the full ISO datetime form (2024-01-15T00:00:00.000, including timezone suffixes) emitted by df.to_json(date_format="iso"), not just yyyy-mm-dd. End-to-end verified by the spec repo's contracts/python_helper_roundtrip script against a real Lark spreadsheet on pandas 2.2 and 3.0. * feat(sheets): add --dataframe Arrow IPC input for +table-put/+table-get/+workbook-create Introduce a binary-typed twin of --sheets: --dataframe accepts an Arrow IPC (Feather v2) payload that pandas' df.to_feather() writes, deriving dtypes and per-column number formats from the Arrow schema. The two producers are mutually exclusive and funnel through a shared resolver so +table-put and +workbook-create stay in lockstep; +table-get gains --dataframe-out for single-sheet reads. Also auto-grow a sub-sheet's row/column count before writing so blocks past the backend's default 200x20 bounds no longer fail with range-exceeds-sheet-bounds. * docs(lark-sheets): remove financial modeling standards reference Drop the lark-sheets-financial-modeling-standards.md reference doc and all pointers to it from SKILL.md, core-operations, and visual-standards. Bump skill version to 3.0.0. * docs(lark-sheets): clarify cell-image vs float-image routing and fix reference self-references Synced from sheet-skill-spec. - Add a binding-based decision (does the image belong to a record and move with its row?) to route +cells-set-image vs +float-image-create across the SKILL entry, float-image and write-cells references. - Add routing rows to the SKILL command cheat-sheet and warn against defaulting to float-image out of familiarity. - Replace mislabeled 本 skill / 子 skill / 跨 skill wording in references with 本文 / reference names, matching the existing convention. * feat(sheets): add --styles to +table-put for one-step typed write with styling +table-put now accepts --styles (same shape as +workbook-create's --styles): cell_styles merge into the set_cell_range matrix, while cell_merges / row_sizes / col_sizes apply as their own tool calls after the write. The styles payload is name-matched against the written sheets and validated up front, so a malformed or mismatched style fails before any write lands. Also points +sheet-create users to +table-put (auto-creates missing sheets) when they need data/styles, via a runtime Tip and the lark-sheets skill references. Flag is sourced from the upstream Base table and regenerated through sheet-skill-spec (flag-defs.json / flag-schemas.json / gen file). Adds unit tests (dry-run styles, name-mismatch reject, execute) and a dry-run E2E (tests/cli_e2e/sheets/sheets_table_put_dryrun_test.go). * docs(lark-sheets): point read-data to +sheet-info for hidden row/col identification skip-hidden defaults to false (lossless reads), but the read primitives don't mark which rows/cols are hidden. Cross-reference +sheet-info --include hidden_rows,hidden_cols + row_indices/col_indices so agents can identify hidden ranges when they need to filter or interpret hidden data. Synced from sheet-skill-spec. * feat(sheets): document link requirement for @document mentions in cells flag schema @document mentions (mention_type != 0) must pass link (doc URL) to render a clickable card; @user mentions (mention_type=0) don't need it. Synced from the upstream tools-schema. * fix(sheets): reject cond-format attrs whose shape mismatches rule_type A conditional-format rule created with --rule-type colorScale but cellIs-shaped attrs ({compare_type,value}, no color) was accepted by the CLI and written through to the server, producing a color-less color-scale segment. That dirty data crashes the frontend on snapshot deserialization, so the spreadsheet can no longer be opened (5005). The per-entry schema check can't catch this: properties.attrs.items is a oneOf over all nine attr shapes and passes as soon as any branch matches, blind to the sibling rule_type — {compare_type,value} matches the cellIs branch even when rule_type says colorScale. The tool side maps attrs blindly by rule_type and only validates dataBar count and iconSet ordering, so the gap reaches the data layer. Add a cross-field validator (validateCondFormatAttrs) wired into both create and update via the new objectCRUDSpec.validateCreateInput hook (twin of validateUpdateInput). It enforces, per rule_type, the keys every attrs entry must carry — mirroring the tool's converter contract — and treats an empty required string (notably color) as missing. Rule types that take no attrs (duplicateValues / uniqueValues / containsBlanks / notContainsBlanks) and updates that omit rule_type are left to the server. * test(sheets): guard condFormatAttrsRequired against flag-schemas drift Add TestCondFormatAttrsRequired_MatchesSchemaOneOf, comparing the hand-maintained condFormatAttrsRequired table against the embedded flag-schemas.json attrs oneOf (multiset of required-key sets, for both create and update). The cross-field validator only holds if its per-rule_type required keys mirror the schema branches, and the two share no compile-time link — this pins them together so a future schema sync that adds/drops a required key can't silently desync the table. * fix(sheets): default +table-get to full used range, not A1 current region +table-get without --range anchored its current_region probe at A1, so an internal blank row or column silently truncated everything past it — agents then treated the partial data as complete (the pro016 / pro025 incident). - Probe the used range over the full physical grid (row_count × column_count from the workbook structure) so it spans internal blank rows/columns; fall back to the legacy A1 anchor when dimensions are unknown. - Emit the actually-read `range` on every sheet so callers can detect truncation (get_cell_ranges has no has_more flag). - Fix the same A1-anchor bug in append mode's last-data-row probe, which could otherwise overwrite data past an internal blank row. - Add unit + dry-run/live E2E coverage; refresh synced skill docs. * docs(sheets): fix csv-get current_region guidance to cross-check row_count current_region is a blank-row/column-bounded block, not the true sheet extent: an internal blank row truncates it, so it can miss rows past the gap. The read-data reference previously called it the "真实数据边界" and told agents to prefer it over row_count — which drove the "read only to current_region's last row, miss the tail" failure. - current_region: warn it can be both smaller (internal blank rows truncate) and larger (trailing summary/signature rows) than the real data range. - csv-get output contract: clarify its row_count/col_count is the returned size (= actual_range), not the physical sheet size; has_more only reflects the current range, not whether the whole sheet was read. - "确定数据范围的正确流程": add a step to cross-check against +workbook-info's physical row_count and probe past current_region's last row for data beyond an internal blank row. * fix(sheets): collapse duplicate validateCreateInput from bad merge resolution A prior merge kept both branches' independently-added validateCreateInput fields on objectCRUDSpec with conflicting signatures (pivot's func(rt, input) and cond-format's func(input)), plus both call sites in objectCreateInput, which failed to compile (validateCreateInput redeclared). Collapse to the single richer func(rt flagView, input) signature and one call site. cond-format's validateCondFormatAttrs (func(input), still shared with validateUpdateInput) is wrapped in a closure that ignores rt. Both behaviors are preserved: pivot --target-position/--range mutex and cond-format attrs-shape-vs-rule_type validation. * refactor(sheets): migrate legacy error helpers to typed errs in sheets domain golangci-lint forbidigo (errs-no-legacy-helper / errs-no-bare-wrap) flagged the table I/O, workbook, and dataframe shortcuts that landed on this branch: 93 common.FlagErrorf and 48 fmt.Errorf calls. - Replace every common.FlagErrorf with common.ValidationErrorf (typed *errs.ValidationError, same signature) across workbook / table_io / dataframe / object_crud. - writeDataframeOut's two final --dataframe-out write failures become typed errs.NewInternalError(SubtypeFileIO, ...).WithCause(err). - applyWorkbookCreateVisualOps now passes the typed callTool error through unchanged (re-wrapping would downgrade classification) and attaches the failing op as a recovery hint only when none is set. - The remaining fmt.Errorf are genuine intermediate errors that the command layer re-wraps into typed validation errors (buildTypedCell / Arrow decode-encode) or surfaces as a partial_success message string (writeTypedSheets via tablePutPartial); each carries a //nolint:forbidigo with that reason, per the lint guidance. No behavior change: error messages and partial-success shapes are preserved; gofmt, go vet, golangci-lint (0 issues) and sheets tests all pass. * fix(shortcuts): clarify single-stdin constraint in flag help and error hint Input flags advertised '(supports @file, - for stdin)' per flag, leading AI agents to write '--a - <x --b - <y' where the second '<' silently clobbers the first and the first flag reads the wrong payload. A process has a single stdin, so at most one flag per call can use '-'. - Reword the generated help hint to '- reads stdin (one flag per call; use @file for others)'. - Add an actionable .WithHint to the stdin-conflict validation error pointing callers to @file for the extra flags. - Assert the new hint in TestResolveInputFlags_DuplicateStdin. * feat(sheets): +cells-get/+csv-get --max-chars 默认值 200000 → 500000 放宽默认防爆上限。flag_defs_gen.go 由 go generate 重生；flag_defs_test.go 的 expected default 同步；flag-schemas.json schema_version 2 → 3 是上游 spec-tables 架构调整带来的元数据 bump，与本业务改动无关、go:embed 不解析 该字段、无功能影响。 Synced from sheet-skill-spec@93f7a78. * docs(lark-sheets): sync from spec — +csv-put 含逗号公式正例 + 收敛警示标签 源同步自 sheet-skill-spec：write-cells 补含逗号公式 RFC 4180 转义正例与结构化写入优先指引；全 reference 收敛「高频致命错误」类标签。 * docs(lark-sheets): sync from spec — --max-chars 放出为可见 flag + 落盘优先指引 源同步自 sheet-skill-spec：--max-chars 放出（默认 500000，可调小避免大输出被 Bash/终端转存为文件、改 has_more 分页）；read-data 增「大数据优先落盘」指引。 * feat(sheets): 写操作报错增强 + --token 别名 - 复合 JSON shape 校验失败时报错附 --print-schema 提示，agent 可直接拿到精确结构（pro26 头号：+cells-set --cells 反复猜 shape） - JSON 解析失败且该 flag 支持 stdin 时提示改用 stdin（公式/引号/逗号内联到 shell 被转义弄坏 JSON） - --token 作为 --spreadsheet-token 的解析期别名：复用 sheets 已有 PostMount 钩子 + pflag normalize，仅 sheets 包，common 零改动 * docs(lark-sheets): sync from spec — set+H 改单引号 / 速查表补臆造命令名 / workbook-import 引导 * fix(sheets): migrate +table-put to typed error contract The merge from main brought in #1449 (retire legacy error envelopes), which removed output.ExitError / output.ErrDetail and forbids constructing them. Port tablePutPartial off the legacy envelope: - no sheets written -> typed errs.APIError (plain failure) - some sheets written -> ok:false result via runtime.OutPartialFailure carrying written_sheets, returning the partial-failure exit signal Also fix two drifts the same merge introduced: - regenerate flag_defs_gen.go to match the committed flag-defs.json - update the --max-chars flag test to assert visible (no longer hidden) * docs(lark-sheets): sync from spec — set+H 告诫通则化（移入 stdin 段） * feat(sheets): styles 接受 halign/valign 等对齐字段别名 把模型常幻觉的 horizontal_align / halign / vertical_align / valign 映射到 规范字段 horizontal_alignment / vertical_alignment，覆盖 --styles 与 typed --cells；与规范字段冲突时报错而非静默择一。同步 lark-sheets skill 文档补 对齐字段说明 + --print-schema --flag-name styles 提示。 * feat(sheets): resolve wiki URLs to the backing spreadsheet for --url Sheets shortcuts only accepted /sheets/ and /spreadsheets/ URLs via --url. A /wiki/<node_token> URL was rejected with "must be a spreadsheet URL" because the wiki node_token is not a spreadsheet token: resolving it to the backing spreadsheet needs a wiki get_node call, which Validate/DryRun (kept network-free) must not make. Mirror the existing slides/doc/drive two-stage pattern: - parseSpreadsheetRef classifies --url / --spreadsheet-token network-free into a sheet token or an (unresolved) wiki node_token. - resolveSpreadsheetTokenExec (Execute only) resolves a /wiki/ node_token via wiki get_node, verifies obj_type=sheet, and returns the obj_token. The wiki:node:read scope is enforced on this path only, so non-wiki invocations are unaffected. - resolveSpreadsheetToken stays network-free for Validate/DryRun, passing the node_token through unchanged. All 47 Execute paths (including +batch-update and +workbook-export) switch to the Exec resolver; Validate/DryRun keep the network-free one. No tool schema change: the CLI feeds the resolved spreadsheet token as excel_id, so this is a pure CLI-layer change. Tested: unit (parse classification + wiki get_node e2e via httpmock) and live end-to-end against a real wiki spreadsheet (read: +workbook-info, +cells-get, +csv-get; write: +sheet-create, +sheet-rename, +csv-put). * docs(sheets): note --url accepts wiki URLs (synced from spec) * fix(sheets): match --url path segment via url.Parse, not substring parseSpreadsheetRef classified /wiki/ with strings.Index over the whole URL, so a /sheets/ link whose query or fragment merely contained /wiki/ (e.g. .../sheets/sht?from=/wiki/x) was hijacked into a get_node call. Now parse the URL and match /sheets/, /spreadsheets/, /wiki/ only as a path prefix, mirroring slides parsePresentationRef which already fixed this class. Drop the substring helpers. Also align wiki resolution with slides: CallAPITyped (typed error + log_id) and classify an incomplete get_node response as InternalError instead of a --url validation error. Add regression tests for query/fragment /wiki/ and incomplete node. * fix(sheets): satisfy errorlint/copyloopvar + regen flag defs - helpers_test.go: drop the Go 1.22+ redundant `tc := tc` loop copy (copyloopvar). - lark_sheet_dataframe.go, lark_sheet_table_io.go: switch the intermediate-error fmt.Errorf calls from %v to %w so errorlint passes. Behavior unchanged — these errors are always rewrapped into typed validation errors at the command layer. - flag_defs_gen.go: regenerate from data/flag-defs.json (drift from the wiki-URL merge). * ci: allow Apache Arrow module in license check Arrow is Apache-2.0 overall, but it vendors c-ares (LicenseRef-C-Ares, ISC-like) inside the module which go-licenses classifies as Unknown and the strict disallowed_types=...,unknown gate rejects. Pass --ignore github.com/apache/arrow/go/v17 since Arrow is required by sheets +table-put / +table-get / +workbook-create --dataframe (Arrow IPC ingest) and the vendored c-ares is not redistributed by us. * fix(sheets): resolve wiki URL in +range-move/+range-copy Execute transformExecuteFn (the named Execute helper shared by +range-move and +range-copy) still called the network-free resolveSpreadsheetToken, so a /wiki/ URL reached transform_range as an unresolved node_token and failed. #1519's sweep over Execute hooks only rewrote inline closures; this is the only Execute backed by a named helper. Switch it to resolveSpreadsheetTokenExec (Validate/DryRun stay network-free) and add a +range-move wiki-URL regression test. * refactor(sheets): drop +table-put manual capacity grow; rely on set_cell_range auto-grow set_cell_range now auto-grows the sub-sheet to fit the write, so the ensureSheetCapacity helper (and its modify_sheet_structure dim-insert call before each write) is no longer needed. This also closes a data- safety hole flagged in review: inserting before the last existing row could push real data down into the area set_cell_range was about to write, and allow_overwrite=false could not protect against it because the structural insert had already mutated the sheet by the time the write-collision check ran. Verified end-to-end against a real spreadsheet: +table-put writing 300x25 into a fresh Sheet1 (default 200x20) succeeds in one write and the sheet ends up 301x25. * fix(sheets): close --dataframe stdin guard hole --dataframe is binary and bypasses the common Input resolver, which is where the existing single-stdin guard lives. Result: an invocation like +table-put --dataframe - --styles - was accepted, then one of the two consumers raced for stdin and the other silently saw an empty stream. Add a stdinConsumed marker on RuntimeContext that both consumers share: common.resolveInputFlags sets it when an Input flag uses '-', and readDataframeBytes both checks and sets it. A second consumer is rejected up front with an actionable hint pointing at @file. Flagged in code review (lark_sheet_dataframe.go:93). * fix(sheets): harden +table-put / +table-get input validation and round-trip safety Four review-flagged correctness gaps in table I/O, all bundled because they touch the same file: 1. --sheets accepted trailing data after the first JSON value (json.Decoder does not surface that, unlike json.Unmarshal). A new decoderExpectEOF helper rejects e.g. `--sheets '{...} oops'` with a typed validation error instead of letting the leading object pass through and surface as a confusing downstream failure. 2. +table-get with a duplicate header (e.g. `amount, amount`) used to read back successfully — the dtypes map silently collapsed to one entry — and only failed later on +table-put because the writer rejects duplicate column names. Fail fast at read time with an actionable hint to rename or pass --no-header. --no-header mode is exempt (fallback col<N> names are always unique). 3. +table-put dry-run rendered an invalid range like A1:C0 when header=false with rows=[]. tablePutFullRange returns "" for an empty matrix or zero columns instead of building a degenerate rectangle. 4. +table-get with --sheet-id and a get_workbook_structure miss (read failure or selector mismatch) used to return a target with name="", which then broke +table-get → +table-put round-trip (the writer requires a non-empty sheet name). Fall back to using the id as the name. End-to-end verified against a real spreadsheet: trailing data, duplicate header, and --no-header fallback all behave as advertised. * fix(sheets): apply +workbook-create style-only ops instead of silently dropping them A +workbook-create call carrying only cell_merges / row_sizes / col_sizes (no --values / --sheets and no cell_styles) used to create the workbook but silently drop the requested visual ops. Two reasons, both fixed: - workbookCreateStyleDimensions only counted cell_styles when computing the write extent, so cell_merges / row_sizes / col_sizes always contributed 0 → buildValuesPayload returned a nil payload → Execute skipped writeTypedSheets entirely → no visual ops ran. Extend the helper to fold the merge / resize ranges in. - Pure row_sizes / col_sizes payloads can never expand a cell rectangle (they are dimension ranges, not cell ranges), so even with the extent fix Execute would still skip the write path. Add a no-data branch: when payload == nil but a styles item is present, look up the default sheet and apply visual ops directly via applyWorkbookCreateVisualOps. The dry-run plan mirrors this so the preview shows the visual ops. Also picks up the --values trailing-JSON-data EOF check (mirror of the --sheets one in lark_sheet_table_io.go). End-to-end verified against a real spreadsheet: a cell_merges-only +workbook-create now produces a sheet with merged_cells_count: 1. * fix(sheets): preserve causes and render messages cleanly for typed validation errors common.ValidationErrorf goes through fmt.Sprintf, which does not support %w — the seven call sites that used `%w` were rendering the cause as literal `%!w(*fmt.wrapError=&{...})` and dropping the cause from the typed-error chain (so callers couldn't errors.As back to the underlying error). Switch each to `%v` for clean rendering and attach the cause via .WithCause(err) so the typed contract is preserved. Touched call sites: - lark_sheet_dataframe.go: --dataframe Arrow decode / stdin read / file read failures (3 call sites). - lark_sheet_table_io.go: --sheets invalid JSON, payload-validate per-cell coercion error, buildSheetMatrix per-cell error, --dataframe-out arrow encode failure (4 call sites). End-to-end verified against a real spreadsheet: both invalid-JSON and typed-cell errors now render readable messages instead of %!w(...). * sync(sheets): pick up +sheet-{show,hide}-gridline in +batch-update schema Mirror of the sheet-skill-spec change adding the two gridline shortcuts to cli-schemas.json batch_update.operations.shortcut enum. Synced from the upstream canonical via generate:cli + sync:cli. Verified end-to-end on a real spreadsheet — +batch-update with a +sheet-hide-gridline op passes schema validation and the backend run returns succeeded: 1. * sync(sheets): pick up +workbook-export UX clarification from spec Mirror of the sheet-skill-spec update that documents +workbook-export's default-no-download behavior and its relationship to drive +export --doc-type sheet. Synced from canonical via generate:cli + sync:cli + go generate. End-to-end verified against a real spreadsheet: - Omit --output-path → ok:true, downloaded:false, file_token returned - Pass --output-path ./crfix_test.xlsx → ok:true, file saved (17892 bytes), saved_path returned The --help output for +workbook-export now states the default behavior and points callers at `drive +export --doc-type sheet` when they need the --output-dir / --file-name / --overwrite split. * test(sheets): assert typed errs.Problem instead of err.Error() substrings Per the coding guideline "Error-path tests must assert typed metadata via errs.ProblemOf (category / subtype / param) and cause preservation, not message substrings alone." — sweep through every error-path assertion in the sheets domain and replace the `strings.Contains(stdout+stderr+err.Error(), ...)` pattern with two small helpers landed in helpers_test.go: requireProblem(t, err, wantCategory, wantSubtype, msgContains) -> *errs.Problem requireValidation(t, err, msgContains) -> *errs.ValidationError // shorthand for CategoryValidation + // SubtypeInvalidArgument; lets callers // also assert .Param / .Params / .Cause ~60 assertion sites across 18 test files now check the typed envelope shape, with message-substring checks moved onto the returned Problem (.Message / .Hint / .Param). The substring is preserved as a sanity check rather than the sole assertion, so a category drift like validation → internal would now fail loudly instead of slipping past. Cases intentionally left as substring (each with a one-line reason): - Errors that come straight from cobra's native flag parser (untyped *errors.errorString — e.g. "required flag(s) ... not set", mutually- exclusive groups). Re-typing these needs a custom FlagErrorFunc and is out of scope here. - Intermediate errors from decodeArrowToSheet that the caller wraps into a typed envelope (`//nolint:forbidigo` reason). Those unit tests assert the unwrapped intermediate directly. One production tweak: - shortcuts/sheets/flag_schema.go: printFlagSchemaFor returns typed *errs.ValidationError (with WithParam("--flag-name") on the unknown-flag branch) instead of raw fmt.Errorf. The framework already wraps this when called via --print-schema, so user-facing behaviour is unchanged; direct callers (and tests) now get the typed envelope. Verified: go test ./shortcuts/sheets/... passes; golangci-lint --new-from-rev=origin/main reports 0 issues. * test(common): assert typed errs.Problem instead of err.Error() substrings Mirror of the sweep just landed in shortcuts/sheets: replace error-path substring assertions with typed-envelope checks via two small helpers landed in a new shortcuts/common/typed_error_assertions_test.go: requireProblem(t, err, wantCategory, wantSubtype, msgContains) -> *errs.Problem requireValidation(t, err, msgContains) -> *errs.ValidationError // shorthand for CategoryValidation + // SubtypeInvalidArgument; lets callers // also assert .Param / .Params / .Cause 8 sites moved to typed assertions across runner_jq_test.go, mcp_client_test.go, drive_media_upload_typed_test.go, and runner_input_test.go (the input tests already used a typed-param helper; this just retargets the substring follow-up onto the typed Message). Sites intentionally left as substring + comment (production returns raw fmt.Errorf, not a typed envelope): - runner_botinfo_test.go (6 sites): BotInfo / fetchBotInfo wrap upstream errors with fmt.Errorf so the SDK-level message ([99991], 403, invalid character, etc.) shows through. - runner_args_test.go (4 sites in 2 tests): rejectPositionalArgs returns raw fmt.Errorf to satisfy cobra's PositionalArgs contract. - permission_grant_test.go (2 sites): assert on stderr / hint strings, not error messages — already out of the err.Error() substring class. No production code changes. Verified: go test ./shortcuts/common/... passes; golangci-lint --new-from-rev=origin/main ./shortcuts/common/... reports 0 issues. * fix(sheets): plug four +table-put / +table-get correctness gaps flagged in CR Four review-flagged bugs, all in lark_sheet_table_io.go (bundled because they touch the same file and the same +table-put / +table-get domain): 1. +table-get --dry-run dropped the --sheet-id / --sheet-name selector from the get_cell_ranges body, while Execute always passed it. Agents that validate the dry-run shape and then run live would see a request shape mismatch. The dry-run now calls sheetSelectorForToolInput so the body matches Execute. 2. isDateNumberFormat used a simple `strings.ContainsRune(_, 'y')` so number formats like "JPY #,##0" (a currency prefix that happens to contain a lone 'Y') were misread as date formats — round-tripping integer cells out as ISO dates. The detector is now token-aware: it skips quoted "...", `\\x`-escaped, and `[...]` bracket sections, and only fires on an unescaped `yy` (a real Excel year token). 3. sheetCreateDims sized new append-mode sheets by `headerOn(s)` only, but writeSheetData forces a header on empty append sheets when Header == nil. Near 50000 rows / 200 cols this created the sheet one row short and the follow-up set_cell_range bounced off the backend ceiling. Size now matches the forced-header logic exactly. 4. tableGetTargets fallback paths (read-failure / selector mismatch on --sheet-id) returned a target with name="" — already corrected for --sheet-id structure-success path in 086876d2, but the structure- failure fallback still left it empty. Use the id as the name there too so the +table-get → +table-put round-trip never breaks on a nameless sheet. End-to-end verified against a real spreadsheet: - table-get --dry-run with --sheet-name / --sheet-id both render the selector field in the get_cell_ranges body - A real round-trip (typed put → get) preserves dtypes + formats * fix(sheets): bound --dataframe memory use with byte / row / column caps readDataframeBytes used to read the whole Arrow file unbounded — a stdin / file > 1 GiB would OOM the CLI long before the backend per-sheet ceilings kicked in. decodeArrowToSheet then materialized every record into [][]interface{} regardless of size. Three caps now match the backend's per-sheet hard ceilings: - byte cap: 256 MiB (covers worst-case 200×50000 cells × ~25 B Arrow overhead). File path pre-Stat()s before opening; both file and stdin paths read through io.LimitReader so an oversized input is rejected without allocating the full payload. - column cap: 200, checked at schema-decode time before allocating any per-column slices. - row cap: 50000, checked during record-batch iteration so a 1M-row Arrow file is rejected mid-stream instead of fully decoding first. End-to-end verified against PPE — a 257 MiB file is rejected at file- Stat with a typed validation error before any read happens. * fix(drive): wrap +export ctx cancellation/deadline as typed errs.NetworkError The poll loop in RunExport returned ctx.Err() directly in two places — on the inter-attempt sleep cancel and on the pre-attempt deadline check. That let context.Canceled / context.DeadlineExceeded escape as untyped errors at the cobra layer, bypassing the typed-error contract every other failure path already honors. Add wrapExportContextErr that maps both into errs.NewNetworkError with SubtypeNetworkTransport / SubtypeNetworkTimeout respectively and preserves the cause via .WithCause(err), so callers can still errors.Is(err, context.Canceled) downstream. CR-flagged at drive_export.go:229 / :234. * ci(license): narrow Apache Arrow workaround with a follow-up assertion The dependency-license check still has to --ignore Apache Arrow wholesale because go-licenses' classifier parses its LICENSE.txt as a single license and mis-reports the module as LicenseRef-C-Ares / Unknown (Arrow inlines the c-ares 3rdparty notice alongside its own Apache-2.0). Re-classifying on our side isn't possible without changing go-licenses itself. The CR concern was that --ignore is too wide — a future Arrow re-license or new inlined dep would silently sail through. Add a follow-up step that re-checks Arrow's LICENSE.txt independently: it must still open with "Apache License" AND must still inline the c-ares 3rdparty notice (the two facts that make the --ignore safe today). If either invariant breaks, CI fails here and forces a human to re-evaluate the ignore. Verified locally — both assertions pass against the current pinned Arrow v17. * sync(sheets): pick up +table-put payload-shape doc corrections from spec Mirror of the sheet-skill-spec change that fixes three places teaching an invalid +table-put payload shape — the typed protocol only has columns / data / dtypes / formats (no formula field) and must always be wrapped in an outer {"sheets":[...]} envelope. write-cells and the SKILL.md decision table previously used the wrong field names (type / format) and pointed users at +table-put for formula writes, which the shortcut can't actually accept. Synced from upstream canonical via generate:cli + sync:cli. * test(sheets/e2e): add E2E coverage for new shortcuts + typed workbook-create AGENTS.md requires a dry-run E2E for every new shortcut and a live E2E for new flows. Three new files cover the four shortcuts this branch adds or materially changes: - sheets_gridline_dryrun_test.go — pins +sheet-show-gridline / +sheet-hide-gridline as a single modify_workbook_structure call with the right operation name (show_gridline / hide_gridline) and sheet_id, so an op-name typo would trip CI before any live run. - sheets_workbook_import_dryrun_test.go — pins +workbook-import as a two-step plan (drive media upload + drive import-task create) with the doc type hard-coded to "sheet" — the wrapper's whole reason for existing on top of generic drive +import. --name reaches file_name on the wire; file_extension is sniffed from the local file. - sheets_table_put_typed_workflow_test.go — two live workflows running against a freshly created spreadsheet. The first runs the full typed +table-put → +table-get round-trip (date / numeric / object columns with custom number_format) and asserts the dtype + format contract holds end-to-end. The second exercises the typed +workbook-create --sheets path: create + write in one shortcut, the payload sheet name adopts the workbook's default sheet (no empty "Sheet1" left behind), and the typed contract still survives the read-back. End-to-end verified locally (user identity): typed put round-trips preserve dtypes (date → datetime64[ns], numeric → float64, object → object) + formats verbatim; workbook-create adopts the named sheet as the first sheet with the same typed shape intact. * sync(sheets): pick up sheets_df.py — pandas ↔ JSON skill script from spec Mirror of the sheet-skill-spec change that adds a DataFrame ↔ JSON bridge as a skill-bundled Python script instead of inside the CLI binary. Per PR #1355 review (docx NcmxdRo2yoZ4OXxoMUZcxRZ7nHd, §4.2): keep the CLI a thin JSON/REST client; pandas / Arrow editing lives in the caller's Python process. Synced from canonical via generate:cli + sync:cli. - skills/lark-sheets/scripts/sheets_df.py (new): pandas DataFrame ↔ one sheet, .parquet / .feather / .arrow / .csv / .json. Shells out to `+table-put` / `+table-get` over typed JSON — no CLI changes. - SKILL.md decision tree + write-cells.md +table-put section: explicit pointers so pandas users land on the script instead of hand-rolling the `--sheets` payload. End-to-end verified against PPE: 3-row DataFrame (datetime / float / object) round-trips parquet → script put → real sheet → script get → parquet with dtypes preserved. * Revert "sync(sheets): pick up sheets_df.py — pandas ↔ JSON skill script from spec" This reverts commit 2964983b92fca3e6c92720e5977ee051dc97ff38. * sync(sheets): pick up sheets_df.py + doc DRY cleanup from spec Mirror of the sheet-skill-spec change that ships a 32-line helper-only sheets_df.py (df_to_sheet + sheet_to_df) and removes the corresponding inline `def` blocks from three reference docs. - skills/lark-sheets/scripts/sheets_df.py (new): pandas DataFrame ↔ one +table-put / +table-get sheet, importable as a library. Same helper pair the docs already taught, lifted out of the prose so callers can `from sheets_df import df_to_sheet, sheet_to_df`. - lark-sheets-write-cells.md / lark-sheets-read-data.md / lark-sheets-workbook.md: drop the inline helper definitions; keep the usage examples (single/multi-sheet, round-trip) and switch them to import-from-script. workbook reference's +workbook-create --sheets section now points pandas users at the helper directly (was previously a textual reference back to write-cells). End-to-end verified against PPE (--as user): - +workbook-create with df_to_sheet for three sheets (income / balance / cashflow): create ok, dtypes (datetime64[ns] / float64) + formats (#,##0 / 0.0% / yyyy-mm-dd) survive on read-back through sheet_to_df. - read → pandas mutate → write-back round-trip preserves both data and formats. * chore: drop accidentally-committed __pycache__/ and gitignore .pyc The previous commit (5fac9c39) shipped sheets_df.py and inadvertently included its `__pycache__/sheets_df.cpython-312.pyc` — local Python import created the bytecode cache during PPE round-trip verification and `git add skills/lark-sheets/` swept it in. Remove the pyc and add Python bytecode patterns to .gitignore so the skill-bundled helper scripts don't pull cache files into future commits. * refactor(sheets): drop --dataframe / --dataframe-out + apache/arrow dep Per the design review at NcmxdRo2yoZ4OXxoMUZcxRZ7nHd, the Arrow IPC binary input/output channel adds a heavy columnar runtime to the CLI for no new capability — the typed JSON --sheets path already covers everything, and the column-major / zero-copy advantages collapse the moment the CLI re- encodes into the row-oriented sheets OpenAPI JSON body. Removing it also lets us drop the `--ignore github.com/apache/arrow/go/v17` license-check escape hatch. Deleted: - shortcuts/sheets/lark_sheet_dataframe.go (+ test) - --dataframe branches in +table-put / +workbook-create - --dataframe-out branch in +table-get - StdinConsumed / MarkStdinConsumed exported methods (the binary stdin reader was the only out-of-band consumer); internal stdinConsumed guard against duplicate `-` input flags stays - apache/arrow/go/v17 + transitive deps via `go mod tidy` - CI go-licenses --ignore for arrow and the LICENSE.txt assertion step - --dataframe / --dataframe-out coverage in skill references Pandas users keep the round-trip via the existing skill script skills/lark-sheets/scripts/sheets_df.py over the JSON path. The full pre-removal state is preserved on branch feat/sheets-arrow-stash. Upstream sheet-skill-spec follow-up: the two flag rows in the canonical spec + base table tblV2F6fqIjyCFQW must also be dropped so the next sync does not re-add them. * sync(sheets): pick up --sheets one-liner fix from spec Mirrors sheet-skill-spec 5562f83. The +table-put / +workbook-create --sheets flag descriptions (and the --print-schema description on the sheets array) now point at the existing df_to_sheet helper instead of the previous misleading one-liner that produced a dict missing the outer {"sheets":[...]} envelope and the per-sheet `name`. Agents that copy-paste the description verbatim now build a valid payload. Auto-synced via spec's generate:cli + sync:consumers; go generate ./shortcuts/sheets/... regenerated flag_defs_gen.go so its embedded flagDefs stays byte-equal to data/flag-defs.json. * test(sheets/e2e): close E2E coverage gaps for newly added shortcuts AGENTS.md requires both dry-run and live E2E for every newly registered shortcut, and behavior-changing refactors need at least the matching half. Three gaps remained on feat/lark-sheets-develop: - +sheet-show-gridline / +sheet-hide-gridline (new): only dry-run E2E. Add sheets_gridline_workflow_test.go — create a real spreadsheet, toggle hide then show against a live sub-sheet, assert ok=true on both (gridline state is write-only — there is no read-back field on +sheet-info / +workbook-info — so a successful envelope is the meaningful signal; the dry-run E2E already pins the wire shape). - +workbook-import (new): only dry-run E2E. Add sheets_workbook_import_workflow_test.go — write a local CSV, run the full upload → create-task → poll, assert ready=true with a sheet token, +info confirms the imported workbook is reachable, cleanup deletes the spreadsheet. - +workbook-export refactor (no-download default changed): had live E2E but no dry-run E2E in tests/cli_e2e/. Add sheets_workbook_export_dryrun_test.go — pin the three sheet- specific differences vs drive +export: type=sheet hard-coded, csv mode routes --sheet-id onto sub_id (xlsx mode omits it), and --output-path maps onto the dry-run plan's top-level output_dir. Also pins the csv-without-sheet-id validation error. * refactor(sheets): unify workbookCreatedButFillFailed with OutPartialFailure Three "made it halfway and stopped" exits in the sheets domain previously disagreed on shape, which made the post-failure recovery flow hard for agents to predict from one command to another: - +table-put partial write → exit 1, stdout ok:false envelope - +table-put zero-sheet write → exit 1, stderr api/server_error - +workbook-create create-but-fill → exit 2, stderr validation/failed_precondition OutPartialFailure exists exactly for "the side effect landed but the follow-up didn't" — it stamps an ok:false result envelope on stdout (carrying the state the caller needs to recover) and returns the bare partial-failure exit signal. The workbook-create fill-failure path was the odd one out: it surfaced as a typed failed_precondition error on stderr, which agents couldn't tell apart from a plain validation refusal even though the spreadsheet really did exist and a retry / cleanup was possible. Migrate workbookCreatedButFillFailed onto OutPartialFailure so the four call sites in +workbook-create's Execute (sheet-resolve failure, initial fill failure, style-only resolve failure, style-only apply failure) emit the same envelope shape +table-put's partial write does: { "ok": false, "data": { "spreadsheet_token": "shtNEW", "reason": "spreadsheet shtNEW created but initial fill failed", "hint": "the spreadsheet exists; retry the fill … or delete it", "cause": {"category": "...", "subtype": "...", "message": "..."} } } The inner failure's typed problem (category / subtype / message) is flattened into the `cause` field so agents stay diagnosable from the JSON envelope alone, instead of having to errors.Unwrap a Go error. Updated TestExecute_WorkbookCreate_FillFailureKeepsToken to assert the new shape (ok:false envelope on stdout, *output.PartialFailureError exit signal, structured cause carrying the underlying invalid_response subtype) — preserving the original test intent (token must survive for recovery; inner cause must stay diagnosable) under the new contract. * chore(sheets): three review nits — WithCause + stale comment + unexport - shortcuts/sheets/flag_schema_validate.go:106 — composite-JSON shape validation was wrapping vErr's message into a typed sheets validation error without preserving vErr as the typed cause; add the missing .WithCause(vErr) so errors.Unwrap and ProblemOf still find the underlying validator error (matches every other typed-error chain helper in the file). - shortcuts/sheets/lark_sheet_batch_update.go:92 — comment claimed batchUpdateInput returns "FlagErrorf-typed errors", but FlagErrorf no longer exists (the typed-error migration replaced it with common.ValidationErrorf / errs.ValidationError); update the comment to reflect what is actually returned. - shortcuts/drive/drive_export.go:121 — drop the ValidateExport public alias and rename to validateExport. sheets +workbook-export reuses RunExport / PlanExportDryRun from this package but inlines its own (sheet-specific) Validate, so there is no cross-package call site — ValidateExport was a misleading sibling of the genuinely-shared ValidateImport. Comment added to record the asymmetry so future readers do not export it back. * chore(deps): drop stale indirect bumps left by the arrow removal The earlier --dataframe / --dataframe-out + apache/arrow/go/v17 removal deleted the arrow consumer but left two indirect lines in go.mod pinned to the versions arrow had pulled in: - github.com/kr/text v0.2.0 - golang.org/x/exp v0.0.0-20240222234643-814bf88cf225 With arrow gone, larksuite/cli was the only requirer of those exact versions; every real consumer needs lower ones (kr/pretty wants kr/text v0.1.0; charmbracelet/huh wants x/exp …20231006; xo/terminfo wants x/exp …20220909). Removing the two indirect lines and running `go mod tidy` lets MVS pick the real-consumer versions and drops the explicit indirect entries entirely — go.mod net-diff against main is now zero for this branch. Verified locally: go build ./...; go test ./shortcuts/sheets/... ./shortcuts/drive/... ./shortcuts/common/... ./internal/auth/... ./cmd/auth/... — all green. --------- Co-authored-by: zhengzhijie <zhengzhijie.j@bytedance.com> Co-authored-by: Chenweifeng-bd <chenweifeng.1534@bytedance.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
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