feat(im): return typed error envelopes across the im domain#1230
feat(im): return typed error envelopes across the im domain#1230evandance wants to merge 1 commit into
Conversation
Migrate the produced errors in shortcuts/im from legacy output.Err* and fmt.Errorf to typed errs.* builders, so consumers get stable category and subtype fields (plus param and missing_scopes where applicable) for classification and recovery instead of free-form message text. Previously untyped download/upload failures are now classified as validation or network instead of the generic internal fallback. Genuine intermediate %w wraps are left untouched. Ban legacy output.Err* on shortcuts/im via forbidigo to prevent reintroduction.
📝 WalkthroughWalkthroughThis PR migrates the IM shortcuts package from legacy ChangesIM Shortcuts Error Handling Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1230 +/- ##
=======================================
Coverage 69.22% 69.23%
=======================================
Files 635 636 +1
Lines 59687 59702 +15
=======================================
+ Hits 41320 41333 +13
- Misses 15033 15035 +2
Partials 3334 3334 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@a8c75353da153c2ea81181310ad75654935cbfda🧩 Skill updatenpx skills add larksuite/cli#feat/errs-migrate-im -y -g |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
shortcuts/im/im_messages_send.go (1)
218-224:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTreat missing local media files as invalid input.
Ignoring
os.IsNotExist(err)lets--image/--file/--video/--audiopoint at a missing local path without producing the typed validation envelope this migration is adding.runtime.FileIO().Stat()already handles path-safety for these flags, so the remainingStaterrors should be surfaced here too.Suggested fix
func validateMediaFlagPath(fio fileio.FileIO, flagName, value string) error { if value == "" || strings.HasPrefix(value, "http://") || strings.HasPrefix(value, "https://") || isMediaKey(value) { return nil } - if _, err := fio.Stat(value); err != nil && !os.IsNotExist(err) { + if _, err := fio.Stat(value); err != nil { return errs.NewValidationError(errs.SubtypeInvalidArgument, "%s: %v", flagName, err).WithParam(flagName) } return nil }Based on learnings,
runtime.FileIO().Stat()already enforces path-safety for user-supplied local file paths, so suppressingStatfailures here only hides missing-file input errors.🤖 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/im/im_messages_send.go` around lines 218 - 224, The validateMediaFlagPath function currently ignores os.IsNotExist errors from fio.Stat, allowing missing local media paths to pass validation; update the error handling in validateMediaFlagPath so that any Stat error (including os.IsNotExist) is returned as a typed validation error using errs.NewValidationError with flagName context (i.e., replace the conditional that skips os.IsNotExist with a straight check for err != nil and return the validation error), preserving the initial checks for empty/HTTP/media-key values.shortcuts/im/im_messages_resources_download.go (1)
400-419:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap the final transport error before returning it.
After the retry loop,
lastErris returned verbatim. IfDoAPIStreamkeeps failing with a raw timeout/DNS/socket error, callers lose the typed IM envelope on the exact path this PR is migrating.Suggested fix
if lastErr != nil { - return nil, lastErr + return nil, wrapIMNetworkErr(lastErr) } return nil, errs.NewNetworkError(errs.SubtypeNetworkTransport, "download request 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/im/im_messages_resources_download.go` around lines 400 - 419, The loop currently returns lastErr verbatim which loses the IM-specific network envelope; instead wrap lastErr in the IM network error before returning so callers keep the typed envelope. Replace the final "return lastErr" with a wrapped error (e.g. return errs.NewNetworkError(errs.SubtypeNetworkTransport, "download request failed", lastErr) or otherwise attach lastErr as the cause), leaving the retry logic using DoAPIStream, imDownloadRequestRetries and sleepIMDownloadRetry unchanged; ensure the final return uses errs.NewNetworkError with lastErr included.shortcuts/im/im_messages_search.go (1)
360-366:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject oversized
--page-sizevalues instead of silently clamping.This now returns a typed invalid-argument error for
page-size < 1, but--page-size > 50still succeeds and is quietly rewritten to 50. That makes the new validation contract asymmetric and hides caller mistakes.Suggested fix
pageSize := runtime.Int("page-size") - if pageSize < 1 { + if pageSize < 1 || pageSize > messagesSearchMaxPageSize { return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, "--page-size must be an integer between 1 and 50").WithParam("--page-size") } - if pageSize > messagesSearchMaxPageSize { - pageSize = messagesSearchMaxPageSize - }🤖 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/im/im_messages_search.go` around lines 360 - 366, The code currently clamps pageSize > messagesSearchMaxPageSize to 50; change this to return a validation error instead so the contract is symmetric. In the function that reads pageSize (the runtime.Int("page-size") branch), replace the clamp block that references messagesSearchMaxPageSize with a check that if pageSize > messagesSearchMaxPageSize you return errs.NewValidationError(errs.SubtypeInvalidArgument, "--page-size must be an integer between 1 and 50").WithParam("--page-size") (matching the existing pageSize < 1 error) so oversized values are rejected rather than silently rewritten.
🤖 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/im/helpers.go`:
- Around line 1052-1055: The validation errors for oversize uploads lose
structured attribution because upload helpers (e.g., uploadImageToIM) don't
include the originating flag; update the upload helper signatures to accept a
flagName string (e.g., add flagName param to uploadImageToIM and the other
upload helpers referenced around lines 1089–1092), pass s.flagName from
resolveLocalMedia into those helpers, and include that flagName as the param on
errs.NewValidationError calls so the error includes the source flag (use
s.flagName when calling the updated upload functions and set the param field in
the validation error construction).
- Around line 1371-1384: Change the error types returned from the API lookup:
when items is missing or empty (the items := data["items"].([]any) check) return
errs.NewAPIError(errs.SubtypeNotFound, ...) instead of errs.NewValidationError;
for malformed payload shapes — the msg type assertion
(items[0].(map[string]any)) and missing chat_id (msg["chat_id"].(string)) —
return errs.NewInternalError(errs.SubtypeInvalidResponse, ...) instead of
errs.NewValidationError so downstream callers get NotFound for lookups and
Internal/InvalidResponse for bad API payloads.
In `@shortcuts/im/im_errors.go`:
- Around line 32-33: The ValidationError returned when errors.Is(err,
fileio.ErrPathValidation) currently drops the param field; update the return in
im_errors.go for the fileio.ErrPathValidation case so it preserves the flag by
chaining .WithParam("--output") on the error (i.e., return
errs.NewValidationError(errs.SubtypeInvalidArgument, "unsafe output path: %s",
err).WithParam("--output").WithCause(err)), referencing the existing
errors.Is(err, fileio.ErrPathValidation) branch and the NewValidationError call.
In `@shortcuts/im/im_flag_create.go`:
- Around line 140-145: The current code around getMessageChatID (and likewise
resolveThreadFeedItemType at lines ~151-155) indiscriminately wraps all errors
into errs.NewValidationError, which hides upstream categories; change the logic
to return upstream errors unchanged unless the failure truly indicates invalid
caller input—i.e., detect the specific condition that means the caller must
change input and only then synthesize a ValidationError (using
errs.NewValidationError). For all other errors from getMessageChatID and
resolveThreadFeedItemType, propagate the original error (or map through the IM
network wrapper) instead of wrapping it into a ValidationError so
transport/auth/network failures keep their original types.
---
Outside diff comments:
In `@shortcuts/im/im_messages_resources_download.go`:
- Around line 400-419: The loop currently returns lastErr verbatim which loses
the IM-specific network envelope; instead wrap lastErr in the IM network error
before returning so callers keep the typed envelope. Replace the final "return
lastErr" with a wrapped error (e.g. return
errs.NewNetworkError(errs.SubtypeNetworkTransport, "download request failed",
lastErr) or otherwise attach lastErr as the cause), leaving the retry logic
using DoAPIStream, imDownloadRequestRetries and sleepIMDownloadRetry unchanged;
ensure the final return uses errs.NewNetworkError with lastErr included.
In `@shortcuts/im/im_messages_search.go`:
- Around line 360-366: The code currently clamps pageSize >
messagesSearchMaxPageSize to 50; change this to return a validation error
instead so the contract is symmetric. In the function that reads pageSize (the
runtime.Int("page-size") branch), replace the clamp block that references
messagesSearchMaxPageSize with a check that if pageSize >
messagesSearchMaxPageSize you return
errs.NewValidationError(errs.SubtypeInvalidArgument, "--page-size must be an
integer between 1 and 50").WithParam("--page-size") (matching the existing
pageSize < 1 error) so oversized values are rejected rather than silently
rewritten.
In `@shortcuts/im/im_messages_send.go`:
- Around line 218-224: The validateMediaFlagPath function currently ignores
os.IsNotExist errors from fio.Stat, allowing missing local media paths to pass
validation; update the error handling in validateMediaFlagPath so that any Stat
error (including os.IsNotExist) is returned as a typed validation error using
errs.NewValidationError with flagName context (i.e., replace the conditional
that skips os.IsNotExist with a straight check for err != nil and return the
validation error), preserving the initial checks for empty/HTTP/media-key
values.
🪄 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: 955ef48f-2fa5-4305-bc24-030443534482
📒 Files selected for processing (20)
.golangci.ymlshortcuts/im/helpers.goshortcuts/im/im_chat_create.goshortcuts/im/im_chat_list.goshortcuts/im/im_chat_messages_list.goshortcuts/im/im_chat_search.goshortcuts/im/im_chat_update.goshortcuts/im/im_errors.goshortcuts/im/im_errors_test.goshortcuts/im/im_flag_cancel.goshortcuts/im/im_flag_create.goshortcuts/im/im_flag_list.goshortcuts/im/im_flag_test.goshortcuts/im/im_messages_mget.goshortcuts/im/im_messages_reply.goshortcuts/im/im_messages_resources_download.goshortcuts/im/im_messages_search.goshortcuts/im/im_messages_send.goshortcuts/im/im_threads_messages_list.goshortcuts/im/mute_filter.go
| func uploadImageToIM(ctx context.Context, runtime *common.RuntimeContext, filePath, imageType string) (string, error) { | ||
| if info, err := runtime.FileIO().Stat(filePath); err == nil && info.Size() > maxImageUploadSize { | ||
| return "", fmt.Errorf("image size %s exceeds limit (max 5MB)", common.FormatSize(info.Size())) | ||
| return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "image size %s exceeds limit (max 5MB)", common.FormatSize(info.Size())) | ||
| } |
There was a problem hiding this comment.
Keep the source flag on upload size validation errors.
Lines 1054 and 1091 emit typed validation errors without param, even though the failing input came from a specific flag. resolveLocalMedia already has s.flagName, so oversize --image, --video-cover, --file, --audio, or --video inputs currently lose structured attribution.
Suggested direction
-func uploadImageToIM(ctx context.Context, runtime *common.RuntimeContext, filePath, imageType string) (string, error) {
+func uploadImageToIM(ctx context.Context, runtime *common.RuntimeContext, filePath, imageType, param string) (string, error) {
if info, err := runtime.FileIO().Stat(filePath); err == nil && info.Size() > maxImageUploadSize {
- return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "image size %s exceeds limit (max 5MB)", common.FormatSize(info.Size()))
+ return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "image size %s exceeds limit (max 5MB)", common.FormatSize(info.Size())).
+ WithParam(param)
}
...
}
-func uploadFileToIM(ctx context.Context, runtime *common.RuntimeContext, filePath, fileType, duration string) (string, error) {
+func uploadFileToIM(ctx context.Context, runtime *common.RuntimeContext, filePath, fileType, duration, param string) (string, error) {
if info, err := runtime.FileIO().Stat(filePath); err == nil && info.Size() > maxFileUploadSize {
- return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "file size %s exceeds limit (max 100MB)", common.FormatSize(info.Size()))
+ return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "file size %s exceeds limit (max 100MB)", common.FormatSize(info.Size())).
+ WithParam(param)
}
...
}Then pass s.flagName from resolveLocalMedia.
Also applies to: 1089-1092
🤖 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/im/helpers.go` around lines 1052 - 1055, The validation errors for
oversize uploads lose structured attribution because upload helpers (e.g.,
uploadImageToIM) don't include the originating flag; update the upload helper
signatures to accept a flagName string (e.g., add flagName param to
uploadImageToIM and the other upload helpers referenced around lines 1089–1092),
pass s.flagName from resolveLocalMedia into those helpers, and include that
flagName as the param on errs.NewValidationError calls so the error includes the
source flag (use s.flagName when calling the updated upload functions and set
the param field in the validation error construction).
| items, ok := data["items"].([]any) | ||
| if !ok || len(items) == 0 { | ||
| return "", output.ErrValidation("message not found or unexpected API response format") | ||
| return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "message not found or unexpected API response format") | ||
| } | ||
|
|
||
| msg, ok := items[0].(map[string]any) | ||
| if !ok { | ||
| return "", output.ErrValidation("unexpected message format in API response") | ||
| return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "unexpected message format in API response") | ||
| } | ||
|
|
||
| chatID, ok := msg["chat_id"].(string) | ||
| if !ok { | ||
| return "", output.ErrValidation("message response missing chat_id field") | ||
| return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "message response missing chat_id field") | ||
| } |
There was a problem hiding this comment.
Don’t classify lookup/API-shape failures as invalid arguments.
Lines 1372-1384 turn both “message not found” and malformed message payloads into ValidationError. This helper is doing a derived API lookup from an already-validated message ID, so an empty items result should be errs.NewAPIError(errs.SubtypeNotFound, ...), while bad payload shapes or missing chat_id should be errs.NewInternalError(errs.SubtypeInvalidResponse, ...). Keeping them as validation changes the typed category and exit-code behavior for downstream callers.
Suggested fix
items, ok := data["items"].([]any)
if !ok || len(items) == 0 {
- return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "message not found or unexpected API response format")
+ return "", errs.NewAPIError(errs.SubtypeNotFound, "message not found")
}
msg, ok := items[0].(map[string]any)
if !ok {
- return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "unexpected message format in API response")
+ return "", errs.NewInternalError(errs.SubtypeInvalidResponse, "unexpected message format in API response")
}
chatID, ok := msg["chat_id"].(string)
if !ok {
- return "", errs.NewValidationError(errs.SubtypeInvalidArgument, "message response missing chat_id field")
+ return "", errs.NewInternalError(errs.SubtypeInvalidResponse, "message response missing chat_id field")
}🤖 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/im/helpers.go` around lines 1371 - 1384, Change the error types
returned from the API lookup: when items is missing or empty (the items :=
data["items"].([]any) check) return errs.NewAPIError(errs.SubtypeNotFound, ...)
instead of errs.NewValidationError; for malformed payload shapes — the msg type
assertion (items[0].(map[string]any)) and missing chat_id
(msg["chat_id"].(string)) — return
errs.NewInternalError(errs.SubtypeInvalidResponse, ...) instead of
errs.NewValidationError so downstream callers get NotFound for lookups and
Internal/InvalidResponse for bad API payloads.
| case errors.Is(err, fileio.ErrPathValidation): | ||
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "unsafe output path: %s", err).WithCause(err) |
There was a problem hiding this comment.
Preserve --output on path-validation save errors.
Line 32 drops the structured param field when runtime.FileIO().Save(...) rejects an unsafe output path. im_messages_resources_download.go routes save failures through this helper, and internal/client/response.go already classifies the same condition as ValidationError.WithParam("--output"), so callers would lose flag attribution here.
Suggested fix
case errors.Is(err, fileio.ErrPathValidation):
- return errs.NewValidationError(errs.SubtypeInvalidArgument, "unsafe output path: %s", err).WithCause(err)
+ return errs.NewValidationError(errs.SubtypeInvalidArgument, "unsafe output path: %s", err).
+ WithParam("--output").
+ WithCause(err)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case errors.Is(err, fileio.ErrPathValidation): | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "unsafe output path: %s", err).WithCause(err) | |
| case errors.Is(err, fileio.ErrPathValidation): | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, "unsafe output path: %s", err). | |
| WithParam("--output"). | |
| 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 `@shortcuts/im/im_errors.go` around lines 32 - 33, The ValidationError returned
when errors.Is(err, fileio.ErrPathValidation) currently drops the param field;
update the return in im_errors.go for the fileio.ErrPathValidation case so it
preserves the flag by chaining .WithParam("--output") on the error (i.e., return
errs.NewValidationError(errs.SubtypeInvalidArgument, "unsafe output path: %s",
err).WithParam("--output").WithCause(err)), referencing the existing
errors.Is(err, fileio.ErrPathValidation) branch and the NewValidationError call.
| chatID, err := getMessageChatID(rt, id) | ||
| if err != nil { | ||
| return flagItem{}, output.ErrValidation( | ||
| "failed to query message for feed-layer flag: %v; if you know the chat type, specify --item-type explicitly", err) | ||
| // TODO(errs): this is failed_precondition semantics (ambiguous mapping, caller must change input); upgrade to errs.SubtypeFailedPrecondition once it lands on main | ||
| return flagItem{}, errs.NewValidationError(errs.SubtypeInvalidArgument, | ||
| "failed to query message for feed-layer flag: %v", err).WithHint("specify --item-type explicitly").WithCause(err) | ||
| } |
There was a problem hiding this comment.
Preserve upstream lookup error categories here.
getMessageChatID and resolveThreadFeedItemType are prerequisite IM lookups, so their failures are not always invalid user input. Rewrapping every error as ValidationError turns network/API/auth failures into exit-code-2 validation envelopes and hides the real category from callers. Please return existing typed errors as-is (or route raw transport failures through the new IM network wrapper) and only synthesize a validation/precondition error for the genuine “caller must change input” cases.
Also applies to: 151-155
🤖 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/im/im_flag_create.go` around lines 140 - 145, The current code
around getMessageChatID (and likewise resolveThreadFeedItemType at lines
~151-155) indiscriminately wraps all errors into errs.NewValidationError, which
hides upstream categories; change the logic to return upstream errors unchanged
unless the failure truly indicates invalid caller input—i.e., detect the
specific condition that means the caller must change input and only then
synthesize a ValidationError (using errs.NewValidationError). For all other
errors from getMessageChatID and resolveThreadFeedItemType, propagate the
original error (or map through the IM network wrapper) instead of wrapping it
into a ValidationError so transport/auth/network failures keep their original
types.
Summary
shortcuts/imreturned errors through legacyoutput.Err*/fmt.Errorf, so consumers saw free-form messages with no machine-readable classification and AI agents had to regex error text to recover. This migrates the im domain to typederrs.*envelopes, giving callers stabletype,subtype,param, andmissing_scopesfields, and corrects error paths that previously fell into the generic internal exit code.Changes
shortcuts/im/*.go(chat, message, flag, thread, mute, resource-download) fromoutput.Err*/output.Errorf/ finalfmt.Errorfto typederrs.*buildersshortcuts/im/im_errors.gowithwrapIMNetworkErr(pass typed errors through, wrap raw ones as transport-level network errors) andimSaveError(path-validation → validation, mkdir/write → internal file-I/O)*errs.PermissionErrorwith a structuredmissing_scopesarray; classify derived "not found" lookups as*errs.APIError{not_found}validationornetworkas appropriate--file-keyvs--output) vianormalizeDownloadOutputPathshortcuts/imvia forbidigo (.golangci.yml) to prevent reintroduction of legacy constructors// TODO(errs)markers to adopterrs.WithParams/errs.SubtypeFailedPreconditiononce those primitives land onmain; until then they useinvalid_argumentTest Plan
make unit-testpassed (shortcuts/im,shortcuts/im/convert_lib,internal/errclass)--new-from-rev=origin/main: 0 issuesim +messages-resources-download --message-id om_x --type file --file-key a/b→ exit 2,error.type=validation,error.param=--file-key; absolute--output→error.param=--outputRelated Issues
N/A
Summary by CodeRabbit
Bug Fixes & Improvements
Tests