feat(im): return typed error envelopes across the im domain#1230
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:
📝 WalkthroughWalkthroughReplaces legacy IM shortcut errors with typed ChangesIM Shortcuts Error Handling Migration
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 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 |
github-actions
Bot
added
domain/im
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1230 +/- ##
==========================================
+ Coverage 70.97% 71.00% +0.02%
==========================================
Files 680 681 +1
Lines 65318 65367 +49
==========================================
+ Hits 46362 46412 +50
+ Misses 15316 15314 -2
- Partials 3640 3641 +1 ☔ 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@9136393bda91e24064f0bff2508fe123dacd800c🧩 Skill updatenpx skills add larksuite/cli#feat/errs-migrate-im -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: 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
evandance
force-pushed
the
feat/errs-migrate-im
branch
2 times, most recently
from
bc092b6 to
a2b8d43
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)
shortcuts/im/helpers.go (1)
397-423:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve typed API failures before falling back to
not_found.These two helpers still use
runtime.DoAPI(...)and never inspect the response envelopecode. If the backend returns an API error in an HTTP 200 response, the emptydatapath now falls through toP2P chat not found/thread ID not found, masking permission/auth/rate-limit failures asnot_found.getMessageChatIDwas already moved toDoAPIJSONTypedfor exactly this reason; these lookups need the same classification before the final not-found branch.Also applies to: 435-457
🤖 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 397 - 423, The helper currently calls runtime.DoAPI and parses apiResp.RawBody directly, which can mask API errors returned in a 200 envelope and fall through to "not_found"; replace the runtime.DoAPI usage with runtime.DoAPIJSONTyped (or explicitly inspect the response envelope's "code" and "msg" from apiResp.RawBody/result before accessing data) so any API-level errors (permission/auth/rate-limit, etc.) are returned as typed errs.NewAPIError instead of falling through to the final not-found branch; apply the same change to the analogous block at lines 435-457 and ensure you return the API error as-is when the envelope indicates failure, only falling back to errs.SubtypeNotFound when the envelope is success but data/p2p_chats (or thread) is actually empty.
🤖 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 344-345: resolveLocalMedia currently sends local-path upload
failures through mediaFallbackOrError and then wraps them with wrapIMNetworkErr,
which misclassifies file I/O/validation errors (from runtime.FileIO().Stat/Open)
as network.transport; update the non-URL branch so that errors originating from
local file operations are not wrapped by wrapIMNetworkErr but are returned or
reclassified as file-IO/validation errors instead (i.e., preserve the original
error type and envelope from mediaFallbackOrError), and only apply
wrapIMNetworkErr to true network/URL upload failures (affecting
resolveLocalMedia, mediaFallbackOrError usage, and the call sites like
uploadImageToIM).
---
Outside diff comments:
In `@shortcuts/im/helpers.go`:
- Around line 397-423: The helper currently calls runtime.DoAPI and parses
apiResp.RawBody directly, which can mask API errors returned in a 200 envelope
and fall through to "not_found"; replace the runtime.DoAPI usage with
runtime.DoAPIJSONTyped (or explicitly inspect the response envelope's "code" and
"msg" from apiResp.RawBody/result before accessing data) so any API-level errors
(permission/auth/rate-limit, etc.) are returned as typed errs.NewAPIError
instead of falling through to the final not-found branch; apply the same change
to the analogous block at lines 435-457 and ensure you return the API error
as-is when the envelope indicates failure, only falling back to
errs.SubtypeNotFound when the envelope is success but data/p2p_chats (or thread)
is actually 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: 658c0a22-b574-4279-aedf-22af7996e44b
📒 Files selected for processing (28)
.golangci.ymllint/errscontract/rule_no_legacy_envelope_literal.goshortcuts/common/call_api_typed_test.goshortcuts/common/runner.goshortcuts/im/convert_lib/helpers.goshortcuts/im/convert_lib/merge.goshortcuts/im/convert_lib/reactions.goshortcuts/im/convert_lib/thread.goshortcuts/im/helpers.goshortcuts/im/helpers_network_test.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
✅ Files skipped from review due to trivial changes (1)
- lint/errscontract/rule_no_legacy_envelope_literal.go
🚧 Files skipped from review as they are similar to previous changes (7)
- shortcuts/im/im_messages_send.go
- shortcuts/im/im_flag_cancel.go
- shortcuts/im/im_flag_test.go
- shortcuts/im/im_chat_search.go
- shortcuts/im/im_chat_create.go
- shortcuts/im/im_chat_list.go
- shortcuts/im/im_messages_resources_download.go
evandance
force-pushed
the
feat/errs-migrate-im
branch
from
a2b8d43 to
76b0c22
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/im/convert_lib/merge.go (1)
203-207: 💤 Low valueStale doc references to
DoAPIJSON.The call switched to
runtime.DoAPIJSONTypedon Line 209, but the function godoc (Lines 203-207) and the inline comment on Line 216 still describeDoAPIJSON. Update the references to keep the docs accurate.🤖 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/convert_lib/merge.go` around lines 203 - 207, Update the stale documentation and inline comment that reference DoAPIJSON to instead reference the actual function in use, runtime.DoAPIJSONTyped: edit the function godoc and the inline comment near the call site so they describe runtime.DoAPIJSONTyped (behavior, why it’s used, and that it checks the response envelope's code/msg) and remove or replace mentions of DoAPIJSON to keep docs accurate relative to the call to runtime.DoAPIJSONTyped.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@shortcuts/im/convert_lib/merge.go`:
- Around line 203-207: Update the stale documentation and inline comment that
reference DoAPIJSON to instead reference the actual function in use,
runtime.DoAPIJSONTyped: edit the function godoc and the inline comment near the
call site so they describe runtime.DoAPIJSONTyped (behavior, why it’s used, and
that it checks the response envelope's code/msg) and remove or replace mentions
of DoAPIJSON to keep docs accurate relative to the call to
runtime.DoAPIJSONTyped.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec42a118-8d1b-4138-b879-c5d01a68eb1e
📒 Files selected for processing (29)
.golangci.ymllint/errscontract/rule_no_legacy_envelope_literal.goshortcuts/common/call_api_typed_test.goshortcuts/common/runner.goshortcuts/im/convert_lib/card_test.goshortcuts/im/convert_lib/helpers.goshortcuts/im/convert_lib/merge.goshortcuts/im/convert_lib/reactions.goshortcuts/im/convert_lib/thread.goshortcuts/im/helpers.goshortcuts/im/helpers_network_test.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
✅ Files skipped from review due to trivial changes (1)
- lint/errscontract/rule_no_legacy_envelope_literal.go
🚧 Files skipped from review as they are similar to previous changes (25)
- shortcuts/im/convert_lib/reactions.go
- shortcuts/im/im_messages_mget.go
- shortcuts/im/mute_filter.go
- shortcuts/im/im_chat_update.go
- shortcuts/im/convert_lib/thread.go
- .golangci.yml
- shortcuts/im/im_threads_messages_list.go
- shortcuts/im/convert_lib/card_test.go
- shortcuts/common/runner.go
- shortcuts/im/im_chat_search.go
- shortcuts/im/im_messages_search.go
- shortcuts/im/im_flag_create.go
- shortcuts/im/im_chat_messages_list.go
- shortcuts/im/convert_lib/helpers.go
- shortcuts/im/im_messages_send.go
- shortcuts/im/helpers_network_test.go
- shortcuts/im/im_flag_list.go
- shortcuts/im/im_errors_test.go
- shortcuts/common/call_api_typed_test.go
- shortcuts/im/im_flag_cancel.go
- shortcuts/im/im_chat_list.go
- shortcuts/im/im_flag_test.go
- shortcuts/im/im_messages_reply.go
- shortcuts/im/im_chat_create.go
- shortcuts/im/helpers.go
evandance
force-pushed
the
feat/errs-migrate-im
branch
13 times, most recently
from
ba0e4d2 to
5bb73f6
Compare
evandance
force-pushed
the
feat/errs-migrate-im
branch
from
5bb73f6 to
9136393
Compare
YangJunzhou-01
approved these changes
Jun 6, 2026
pluginmd
pushed a commit
to larkcowork/lark-mcp-cli
that referenced
this pull request
Jun 7, 2026
…ial rename Official larksuite#1230 renamed common.ValidateChatID → ValidateChatIDTyped; the MCP card-send command (added pre-rebase) still called the removed name. Restores go build ./... after rebasing the MCP/Cowork work onto larkcowork/main.
SunPeiYang996
pushed a commit
that referenced
this pull request
Jun 8, 2026
This was referenced Jun 9, 2026
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
shortcuts/impreviously returned many final errors through legacyoutput.Err*,output.Errorf, or untyped error shapes. That made agent/script consumers depend on free-form stderr text instead of stable fields. This PR migrates the IM domain to the typederrs.*contract so callers can reliably consumetype,subtype,param,missing_scopes,log_id, andhint.Changes
log_idand response subtype details.--image,--file,--audio,--video,--video-cover) and resource-download path failures (--file-key,--output).runtime.OutPartialFailure(...)for IM batch-style partial failures so stdout reportsok:falsewhile preserving per-item results.shortcuts/im/cannot regress to legacy error constructors or legacy envelope literals.Test Plan
git diff --check origin/main...HEADfast-gate,unit-test,lint,coverage,deadcode,e2e-dry-runpassed on the previous head before the final squash/rebaseLocal Go is
go1.20.14, while this repo requiresgo 1.23.0, so localgo testcannot run on this machine and CI is the authoritative test run.Related Issues
N/A