feat(drive): emit typed error envelopes across the drive domain#1205
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds per-parameter diagnostics to errs.ValidationError, registers drive API code metadata, tightens forbidigo linting, adds drive I/O/error helpers and errclass classification, migrates many Drive shortcuts to typed errs (validation/internal/network/API), updates partial-failure stdout behavior, and updates tests. ChangesTyped error infrastructure and drive shortcuts
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@1ee4cb2b5bb22bf834290c0f989dbfc6957761a2🧩 Skill updatenpx skills add larksuite/cli#feat/errs-migrate-drive -y -g |
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)
shortcuts/drive/drive_import_common.go (1)
225-240:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBind
--file/--typeinvalid-param metadata onfile type mismatch
ValidationError.WithParamstakes...errs.InvalidParam(not strings). This error is currently the only one invalidateDriveImportSpecthat omits param binding; since the failure is the--typevs file-extension pairing, attach both parameters to keep the invalid-params envelope consistent with the sibling cases.♻️ Proposed change
- return errs.NewValidationError(errs.SubtypeInvalidArgument, "file type mismatch: %s", hint) + return errs.NewValidationError(errs.SubtypeInvalidArgument, "file type mismatch: %s", hint). + WithParams( + errs.InvalidParam{Name: "--file", Reason: hint}, + errs.InvalidParam{Name: "--type", Reason: hint}, + )🤖 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/drive/drive_import_common.go` around lines 225 - 240, The file-type mismatch branch in validateDriveImportSpec returns a ValidationError without binding InvalidParam metadata; change the error construction to use ValidationError.WithParams (or the equivalent helper used elsewhere) and attach two errs.InvalidParam entries for the offending flags (e.g. name="--file" with value spec.Filename or ext, and name="--type" with value spec.DocType) so the returned error matches the other cases that call errs.WithParams and provides structured invalid-param info.
🧹 Nitpick comments (1)
shortcuts/drive/drive_search.go (1)
444-448: 💤 Low valueOptional: attribute the hard-cap span error to both
--opened-*flags viaWithParams.This validation spans
--opened-sinceand--opened-until, so a singleWithParamdoesn't fit, but the new multi-parameterParamssupport added in this stack would let agents branch on the offending flags here too. LeavingParamempty (as with the mutual-exclusion cases) stays consistent.🤖 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/drive/drive_search.go` around lines 444 - 448, The validation error for the opened-window span should attribute the offending flags so agents can branch on them: modify the returned error from errs.NewValidationError(...) in drive_search.go to include the multi-parameter attribution (use the new Params/WithParams API) listing both "--opened-since" and "--opened-until" (keep the same error message and driveSearchMaxOpenedSpanDays reference).
🤖 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/drive/drive_download.go`:
- Around line 58-60: The validation error returned when an existing output file
is detected should include the parameter binding like the others; update the
error construction in the FileIO stat check (the branch that uses
runtime.FileIO().Stat and the overwrite variable) to call .WithParam("--output")
on the errs.NewValidationError result so the returned error is consistent with
the validation errors earlier in this function.
---
Outside diff comments:
In `@shortcuts/drive/drive_import_common.go`:
- Around line 225-240: The file-type mismatch branch in validateDriveImportSpec
returns a ValidationError without binding InvalidParam metadata; change the
error construction to use ValidationError.WithParams (or the equivalent helper
used elsewhere) and attach two errs.InvalidParam entries for the offending flags
(e.g. name="--file" with value spec.Filename or ext, and name="--type" with
value spec.DocType) so the returned error matches the other cases that call
errs.WithParams and provides structured invalid-param info.
---
Nitpick comments:
In `@shortcuts/drive/drive_search.go`:
- Around line 444-448: The validation error for the opened-window span should
attribute the offending flags so agents can branch on them: modify the returned
error from errs.NewValidationError(...) in drive_search.go to include the
multi-parameter attribution (use the new Params/WithParams API) listing both
"--opened-since" and "--opened-until" (keep the same error message and
driveSearchMaxOpenedSpanDays reference).
🪄 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: a0714d18-2c47-4e61-a1f5-615e7f537e1e
📒 Files selected for processing (39)
.golangci.ymlerrs/types.goerrs/types_test.gointernal/errclass/codemeta_drive.gointernal/errclass/codemeta_drive_test.goshortcuts/drive/drive_add_comment.goshortcuts/drive/drive_add_comment_test.goshortcuts/drive/drive_apply_permission.goshortcuts/drive/drive_create_folder.goshortcuts/drive/drive_create_shortcut.goshortcuts/drive/drive_delete.goshortcuts/drive/drive_download.goshortcuts/drive/drive_duplicate_remote_test.goshortcuts/drive/drive_export.goshortcuts/drive/drive_export_common.goshortcuts/drive/drive_export_download.goshortcuts/drive/drive_export_test.goshortcuts/drive/drive_import.goshortcuts/drive/drive_import_common.goshortcuts/drive/drive_inspect.goshortcuts/drive/drive_move.goshortcuts/drive/drive_move_common.goshortcuts/drive/drive_pull.goshortcuts/drive/drive_pull_test.goshortcuts/drive/drive_push.goshortcuts/drive/drive_search.goshortcuts/drive/drive_search_test.goshortcuts/drive/drive_secure_label.goshortcuts/drive/drive_status.goshortcuts/drive/drive_status_test.goshortcuts/drive/drive_sync.goshortcuts/drive/drive_sync_test.goshortcuts/drive/drive_task_result.goshortcuts/drive/drive_task_result_test.goshortcuts/drive/drive_upload.goshortcuts/drive/drive_upload_classify_test.goshortcuts/drive/drive_version.goshortcuts/drive/drive_version_test.goshortcuts/drive/list_remote.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1205 +/- ##
==========================================
- Coverage 69.19% 69.16% -0.04%
==========================================
Files 634 636 +2
Lines 59482 59546 +64
==========================================
+ Hits 41161 41184 +23
- Misses 15007 15041 +34
- Partials 3314 3321 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a3361c0 to
37d031e
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/drive/drive_download.go (1)
88-88:⚠️ Potential issue | 🟠 MajorMigrate this save-error wrapper to typed
errs.*errors
common.WrapSaveErrorByCategoryreturns legacy*output.ExitErrorviaoutput.ErrValidation/output.Errorf(not typed*errs.*), soreturn common.WrapSaveErrorByCategory(err, "io")leaves a non-typed error path inshortcuts/drive/drive_download.go.Replace with typed errors matching the same branches:
fileio.ErrPathValidation→errs.NewValidationError(errs.SubtypeInvalidArgument, "unsafe output path: %s", err)*fileio.MkdirError→errs.NewInternalError(errs.SubtypeFileIO, "cannot create parent directory: %s", err)- default →
errs.NewInternalError(errs.SubtypeFileIO, "cannot create file: %s", 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 `@shortcuts/drive/drive_download.go` at line 88, Replace the legacy common.WrapSaveErrorByCategory(err, "io") return in drive_download.go with typed errs-based branches: detect fileio.ErrPathValidation and return errs.NewValidationError(errs.SubtypeInvalidArgument, "unsafe output path: %s", err); detect a *fileio.MkdirError and return errs.NewInternalError(errs.SubtypeFileIO, "cannot create parent directory: %s", err); otherwise return errs.NewInternalError(errs.SubtypeFileIO, "cannot create file: %s", err); keep the original error variable name and ensure you import/reference fileio and errs symbols used above.
🧹 Nitpick comments (1)
shortcuts/drive/drive_download.go (1)
79-79: ⚡ Quick winRemove redundant error text from the message format.
The call
wrapDriveNetworkErr(err, "download failed: %s", err)includes the error both in the formatted message and as the cause, resulting in duplicate error text. SinceWithCause(err)already attaches the underlying error, the message should be a plain description.♻️ Proposed fix
- return wrapDriveNetworkErr(err, "download failed: %s", err) + return wrapDriveNetworkErr(err, "download 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 `@shortcuts/drive/drive_download.go` at line 79, The wrapDriveNetworkErr call duplicates the error by formatting it into the message and also passing it as the cause; update the call to use a plain descriptive message instead of including "%s" and the error. Locate the call to wrapDriveNetworkErr(err, "download failed: %s", err) in drive_download.go and change it to something like wrapDriveNetworkErr(err, "download failed") so the underlying err is only attached via the cause, not interpolated into the message.
🤖 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.
Outside diff comments:
In `@shortcuts/drive/drive_download.go`:
- Line 88: Replace the legacy common.WrapSaveErrorByCategory(err, "io") return
in drive_download.go with typed errs-based branches: detect
fileio.ErrPathValidation and return
errs.NewValidationError(errs.SubtypeInvalidArgument, "unsafe output path: %s",
err); detect a *fileio.MkdirError and return
errs.NewInternalError(errs.SubtypeFileIO, "cannot create parent directory: %s",
err); otherwise return errs.NewInternalError(errs.SubtypeFileIO, "cannot create
file: %s", err); keep the original error variable name and ensure you
import/reference fileio and errs symbols used above.
---
Nitpick comments:
In `@shortcuts/drive/drive_download.go`:
- Line 79: The wrapDriveNetworkErr call duplicates the error by formatting it
into the message and also passing it as the cause; update the call to use a
plain descriptive message instead of including "%s" and the error. Locate the
call to wrapDriveNetworkErr(err, "download failed: %s", err) in
drive_download.go and change it to something like wrapDriveNetworkErr(err,
"download failed") so the underlying err is only attached via the cause, not
interpolated into the message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c5d7533-116d-4f0a-873d-cc176fed60fa
📒 Files selected for processing (40)
.golangci.ymlerrs/types.goerrs/types_test.gointernal/errclass/codemeta_drive.gointernal/errclass/codemeta_drive_test.goshortcuts/drive/drive_add_comment.goshortcuts/drive/drive_add_comment_test.goshortcuts/drive/drive_apply_permission.goshortcuts/drive/drive_create_folder.goshortcuts/drive/drive_create_shortcut.goshortcuts/drive/drive_delete.goshortcuts/drive/drive_download.goshortcuts/drive/drive_duplicate_remote_test.goshortcuts/drive/drive_export.goshortcuts/drive/drive_export_common.goshortcuts/drive/drive_export_download.goshortcuts/drive/drive_export_test.goshortcuts/drive/drive_import.goshortcuts/drive/drive_import_common.goshortcuts/drive/drive_inspect.goshortcuts/drive/drive_move.goshortcuts/drive/drive_move_common.goshortcuts/drive/drive_pull.goshortcuts/drive/drive_pull_test.goshortcuts/drive/drive_push.goshortcuts/drive/drive_search.goshortcuts/drive/drive_search_test.goshortcuts/drive/drive_secure_label.goshortcuts/drive/drive_status.goshortcuts/drive/drive_status_test.goshortcuts/drive/drive_sync.goshortcuts/drive/drive_sync_test.goshortcuts/drive/drive_task_result.goshortcuts/drive/drive_task_result_test.goshortcuts/drive/drive_upload.goshortcuts/drive/drive_upload_classify_test.goshortcuts/drive/drive_version.goshortcuts/drive/drive_version_test.goshortcuts/drive/list_remote.gotests/cli_e2e/drive/drive_duplicate_sync_workflow_test.go
🚧 Files skipped from review as they are similar to previous changes (39)
- tests/cli_e2e/drive/drive_duplicate_sync_workflow_test.go
- shortcuts/drive/drive_export_download.go
- shortcuts/drive/drive_secure_label.go
- internal/errclass/codemeta_drive_test.go
- shortcuts/drive/drive_task_result_test.go
- shortcuts/drive/drive_import.go
- shortcuts/drive/drive_version_test.go
- shortcuts/drive/drive_search_test.go
- shortcuts/drive/drive_create_shortcut.go
- internal/errclass/codemeta_drive.go
- shortcuts/drive/drive_delete.go
- shortcuts/drive/drive_move.go
- .golangci.yml
- shortcuts/drive/drive_status_test.go
- errs/types_test.go
- errs/types.go
- shortcuts/drive/drive_export_test.go
- shortcuts/drive/drive_move_common.go
- shortcuts/drive/drive_create_folder.go
- shortcuts/drive/drive_inspect.go
- shortcuts/drive/drive_duplicate_remote_test.go
- shortcuts/drive/drive_add_comment_test.go
- shortcuts/drive/drive_pull.go
- shortcuts/drive/drive_apply_permission.go
- shortcuts/drive/drive_version.go
- shortcuts/drive/list_remote.go
- shortcuts/drive/drive_task_result.go
- shortcuts/drive/drive_status.go
- shortcuts/drive/drive_export_common.go
- shortcuts/drive/drive_upload_classify_test.go
- shortcuts/drive/drive_export.go
- shortcuts/drive/drive_pull_test.go
- shortcuts/drive/drive_sync_test.go
- shortcuts/drive/drive_search.go
- shortcuts/drive/drive_import_common.go
- shortcuts/drive/drive_push.go
- shortcuts/drive/drive_upload.go
- shortcuts/drive/drive_add_comment.go
- shortcuts/drive/drive_sync.go
5cf7e04 to
0f40284
Compare
6d2f155 to
57072bc
Compare
Drive-domain errors now leave the CLI as typed, machine-branchable envelopes — a stable `type` plus `subtype` and named fields (param, params, retryable, log_id, hint) — so scripts and AI agents can branch on structure and act on a recovery hint instead of parsing prose. Changes: - Every error produced in the drive domain — validation, file I/O, and the failures returned from its Lark API calls — is emitted as a typed errs.* error; the exit code is derived from the error category. Drive's API calls now go through a shared typed classifier, so failures carry subtype, troubleshooter, a recovery hint, and the request's log_id whether the server returns it in the response body or the x-tt-logid header; an already-typed network/auth error is never downgraded into a generic API error. - Known API conditions (resource conflict, cross-tenant, cross-brand, ...) carry a recovery hint keyed by their error class; a command can refine that hint with command-specific guidance. - Batch partial failures (+push / +pull / +sync, where some items succeed and some fail) now report an honest ok:false multi-status result on stdout — the summary and every per-item outcome stay machine-readable — and exit non-zero, instead of a misleading ok:true success envelope. - Duplicate rel_path conflicts report each colliding path as a structured params entry (RFC 7807 invalid-params style). - Static guards lock the drive path so legacy error construction — direct envelopes or the auto-classifying API helpers — cannot be reintroduced, making drive the template for the remaining domains. Output changes worth noting for consumers: - Error envelopes now carry typed type/subtype and named fields; exit codes follow the error category (malformed or incomplete API responses are reported as internal errors rather than generic API errors). - Batch partial failures (+push / +pull / +sync) emit an ok:false result envelope on stdout (summary + per-item items[]) and exit non-zero; the per-item results stay on stdout rather than in a stderr error envelope. Errors surfaced through shared cross-domain helpers (scope precheck, media import upload, metadata lookup, save-path resolution) are not yet typed; they migrate with the shared layer in a follow-up change.
57072bc to
1ee4cb2
Compare
Summary
Drive errors previously left the CLI as a free-form legacy envelope that callers had to parse by reading prose. This PR makes the drive domain's own error producers — validation, file I/O, and every Lark API call it makes — emit typed
errs.*errors: a stabletype+subtypeplus named fields (param,params,retryable,hint,log_id), so scripts and AI agents can branch on structure and know the next action without string-matching. Two static guards (forbidigo + errscontract AST rules) lock the drive path so legacy shapes — direct envelopes and the auto-classifying API helpers — cannot be reintroduced. This is the reusable template for migrating the remaining business domains.Errors that drive surfaces from shared cross-domain
commonhelpers (scope precheck, media import upload, metadata lookup, save-path resolution) are explicitly out of scope here — they migrate with the shared layer in a follow-up (see "Out of scope / deferred").What to focus on (reviewer guide)
runtime.CallAPITyped(shortcuts/common/runner.go; drive calls it directly). It performs the same SDK request asruntime.CallAPI(identical transport + query model) but classifies failures viaerrclass.BuildAPIErrorinto typed errors, and liftslog_idfrom both the response body and thex-tt-logidheader (the body-only parse used to drop the headerlog_id— see review P1). Typed boundary errors are propagated unchanged — an already-typed network/auth error keeps its class and is never downgraded into a blanketapi_error. This replaces 34CallAPI+ 1DoAPIJSONsites that previously surfaced legacyapi_errorenvelopes via the sharedHandleApiResult.CallAPITypedis the reusable primitive the remaining domains adopt (so they need no per-domain wrapper). The classification lives in a sharedClassifyAPIResponsereused by bothCallAPITypedand the file-uploadDoAPIpaths (so uploads also lift headerlog_id), and a non-JSON HTTP 5xx (e.g. a gateway 502 text/html) is classified as a retryablenetwork/server_errorinstead of a mis-parsed internal error.+push/+pull/+syncpartial failures now emit an honestok:falsemulti-status result on stdout (summary + per-itemitems[], both succeeded and failed) plus a typed*output.PartialFailureErrorexit signal — replacing the priorok:truestdout envelope +ErrBare, which both lied about theokfield and used the predicate-onlyErrBarefor non-predicate commands. Modeled as a first-class third outcome inerrs/ERROR_CONTRACT.md;ErrBarestays predicate-only.validation/failed_precondition(gRPC FAILED_PRECONDITION: state not ready, fix state — not a bad arg, not a retry) + a recoveryhint+ structuredparams.internal/invalid_response(was a generic API error).conflict,cross_tenant,cross_brand) live inerrclass.APIHint(subtype)— sibling of the existingConfigHint/PermissionHint. A command may refine the hint with command-specific guidance (e.g.+searchnames the offending--creator-ids/--sharer-ids); that command-level enrichment overrides the default.shortcuts/drive/:output.Err*,fmt.Errorf,errors.New, and the shared legacy helpers (common.FlagErrorf/WrapInputStatError/WrapSaveErrorByCategory).&output.ExitError{}/&output.ErrDetail{}literals and (b) the auto-classifying runtime helpersruntime.CallAPI/DoAPIJSON/DoAPIJSONWithLogIDon migrated paths.output.ErrBare(predicate exit signal) andruntime.DoAPI(raw response, caller classifies) remain allowed.errs/,internal/errclass/,internal/output/,shortcuts/common/, all additive / non-breaking):errs.InvalidParam+ValidationError.Params(RFC 7807 invalid-params),errs.SubtypeFailedPrecondition,errclass.APIHint,output.PartialFailureError+runtime.OutPartialFailure, andruntime.CallAPITyped(the shared typed API primitive other domains will reuse).Changes
runtime.CallAPITyped(shortcuts/common/runner.go): same SDK request asCallAPI, typed classification viaerrclass.BuildAPIError, andlog_idlifted from the response body orx-tt-logidheader. Drive calls it directly; migrate all 34runtime.CallAPI+ theDoAPIJSONWithLogIDsite across the domain to it.runtime.OutPartialFailurewrites anok:falsemulti-status result to stdout and returns the typedoutput.PartialFailureErrorexit signal; migrate+push/+pull/+syncto it (no moreOut(...)+ErrBare). Document it as the third outcome inerrs/ERROR_CONTRACT.md.output.Err*,fmt.Errorf) to typederrs.*; exit codes derive from the error category —shortcuts/drive/*.go.shortcuts/drive/drive_errors.go(wrapDriveNetworkErr,driveInputStatError,driveSaveError,appendDriveExportRecoveryHint) — all propagate already-typed errors and preserve the cause; replace drive's calls to shared legacy helpers.enrichDriveSearchErrorto decorate the typedProblemin place (was*output.ExitError).ValidationError(failed_precondition) withparams+ recovery hint —shortcuts/drive/list_remote.go.errs.InvalidParam+ValidationError.Params+SubtypeFailedPrecondition(errs/),errclass.APIHint(subtype)(internal/errclass/classify.go),output.PartialFailureError(internal/output/).internal/errclass/codemeta_drive.go..golangci.yml+ errscontract rulesrule_no_legacy_envelope_literal.goandrule_no_legacy_runtime_api_call.go.Test Plan
gofmt -l,go vet,go build ./..., golangci-lint (--new-from-rev, 0 issues), errscontract scan, unit tests (88 pkgs + lint module) — all greenCallAPITyped(code≠0 → typedAPIError; success → data; typed network/auth boundary passthrough not downgraded;log_idlifted from header-only and body; non-JSON 5xx → retryablenetwork/server_errorincl. empty Content-Type; non-object JSON[]→invalid_response);OutPartialFailure(ok:false stdout envelope carrying both succeeded+failed items + typedPartialFailureError); the reclassified API-error / partial-failure command tests now assert the typed envelopelark drive +search --mine --creator-ids ou_x→ typedvalidation(mutually-exclusive flags), exit 2e2e-live)Output changes for consumers (CHANGELOG-worthy)
type/subtype+ named fields (param/params/hint/retryable/log_id); exit codes follow the error category.internal(exit 5) instead of a generic API error (exit 1).+push/+pull/+sync) emit anok:falseresult envelope on stdout (summary + per-itemitems[], both succeeded and failed) and exit non-zero — replacing the prior misleadingok:truestdout envelope. The per-item results stay on stdout rather than in a stderr error envelope.Out of scope / deferred
items[].erroris still a string (pre-existing on+push/+pull/+sync). The overall outcome is typed (ok:false multi-status), but a structured per-item problem object (so an agent can branch per item on subtype/retryable/missing_scopes) is a follow-up — it changes the item output shape and touches all item-collection sites.commonhelpers that drive still surfaces legacy through — migrated in the shared-layer phase, not drive-local (cross-domain; would otherwise be duplicated or lossily re-wrapped):EnsureScopes→checkShortcutScopes(scope precheck),common.UploadDriveMediaAll/Multipart(+import),common.FetchDriveMetaTitle(+inspect),common.ResolveSavePath(barefmt.Errorf).contextcancellation/timeout (return ctx.Err()) at a few pre-existing sites still degrades to a plain-text root error; this is the cross-cutting untyped-passthrough class, fixed systematically by the planned root catch-all (not drive-local).FlagErrorf/Wrap*Error) stay for not-yet-migrated domains; typed/removed globally in the shared-layer phase. Drive no longer calls them.go/typesto also catch alias /new()bypasses; converge the migrated-path list (.golangci.yml+ errscontract) into a single manifest.Related Issues
N/A