feat(mail): return typed error envelopes across the mail domain#1250
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:
📝 WalkthroughWalkthroughMigrates mail shortcuts to typed error helpers (validation/failed-precondition/invalid-response), replaces legacy fmt.Errorf/output.ErrValidation with mail-specific errors, switches many runtime.CallAPI calls to CallAPITyped, adds mail error metadata, extends linter scopes, and updates tests for typed error contracts. ChangesMail typed error migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
github-actions
Bot
added
domain/mail
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@3924114502b02eb3f627bec8e4f18848b0725d67🧩 Skill updatenpx skills add larksuite/cli#feat/errs-migrate-mail -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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/mail/mail_triage.go (1)
1093-1110:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDo not retry every
DoAPIfailure.The
runtime.DoAPIerror branch bypassesshouldRetryTriageAPIError, so auth/config/request-construction failures are retried three times exactly like network/rate-limit failures. That adds unnecessary delay and obscures the real error; gate this branch with the same retry predicate after wrapping/decorating the 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 `@shortcuts/mail/mail_triage.go` around lines 1093 - 1110, The DoAPI error branch is currently always retried; modify the block handling runtime.DoAPI errors so it wraps the error with client.WrapDoAPIError and mailDecorateProblemMessage into lastErr, then consult shouldRetryTriageAPIError(lastErr) before deciding to retry—i.e., if !shouldRetryTriageAPIError(lastErr) or attempt == triageAPIRetries return nil, lastErr, otherwise proceed to the sleep/retry; keep runtime.ClassifyAPIResponse handling unchanged. Use the same symbols shown: runtime.DoAPI, client.WrapDoAPIError, mailDecorateProblemMessage, shouldRetryTriageAPIError, triageAPIRetries to locate and update the logic.
🧹 Nitpick comments (1)
shortcuts/mail/draft/large_attachment_parse.go (1)
4-4: ⚡ Quick winNarrow
nolint:forbidigoscope to the specific intermediate wraps.A file-level suppression can hide unrelated future violations in this parser. Prefer line-level
//nolint:forbidigoon the intentional intermediatefmt.Errorfsites instead.🤖 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/mail/draft/large_attachment_parse.go` at line 4, Remove the file-level "//nolint:forbidigo" suppression and instead place line-level "//nolint:forbidigo" comments directly on the specific intermediate fmt.Errorf calls that perform the intentional error wraps in this parser; locate each fmt.Errorf usage in this file (the intermediate large-attachment parser error wrap sites) and add the nolint right after those lines so other forbidigo violations elsewhere in the file remain linted.
🤖 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 @.golangci.yml:
- Around line 79-82: Update the comment above the errs-no-legacy-helper rule to
reflect that it now applies to both shortcuts/drive and shortcuts/mail;
specifically edit the comment that currently reads "errs-no-legacy-helper is
drive-only" to mention drive and mail (or remove the "drive-only" qualifier) so
it matches the path-except pattern (shortcuts/drive/|shortcuts/mail/) used for
errs-no-legacy-helper.
In `@shortcuts/mail/large_attachment_test.go`:
- Around line 977-979: Update the downstream tests to match the new error
wording: replace their expectations that look for "stat" with checks for the
updated phrase "cannot read file" (or ensure they use
strings.Contains(err.Error(), "cannot read file")). Specifically adjust
TestProcessLargeAttachments_FileStatError and
TestPreprocessLargeAttachments_StatError so they assert against the same error
contract produced by statAttachmentFiles(...) (use the same contains check as
the changed test).
In `@shortcuts/mail/mail_draft_edit.go`:
- Around line 520-525: The patch validation branch drops the `--patch-file`
attribution; change the error returned from patch.Validate() to use
mailValidationParamError("--patch-file", ...) (instead of mailValidationError)
so validation failures retain the invalid_parameters metadata; specifically
update the return in the patch.Validate() error branch (the call to
patch.Validate()) to return mailValidationParamError("--patch-file", "validate
patch file: %v", err).WithCause(err) (preserving the existing err and message
style) while leaving the json.Unmarshal branch unchanged.
In `@shortcuts/mail/mail_draft_send.go`:
- Around line 277-292: The typed internal/SDK errors from CallAPITyped should be
treated as fatal like the untyped fallback case: update the fatal-check to
include errs.CategoryInternal (add it to the existing category switch alongside
errs.CategoryAuthentication/Authorization/Config/Network) so that when
errs.ProblemOf(err) yields a Problem with Category == errs.CategoryInternal the
function returns true; also ensure any subtype-based checks still apply and
update the corresponding test that expected internal errors to be recoverable.
In `@shortcuts/mail/mail_reply.go`:
- Around line 274-276: The error message passed to mailValidationError is
redundantly formatting the error and also attaching it as the cause; in the
ResolveLocalImagePaths error branch (resolved, refs, resolveErr :=
draftpkg.ResolveLocalImagePaths(...)) replace mailValidationError("%v",
resolveErr).WithCause(resolveErr) with a descriptive message (e.g. "failed to
resolve local image paths") and call .WithCause(resolveErr) so the error text
appears only as the cause; keep draftpkg.ResolveLocalImagePaths, resolveErr,
mailValidationError and WithCause references intact.
- Around line 338-339: The error message duplicates the error because the
formatted string includes "%v" while the same err is also attached via
WithCause(err); update the return in the mailReply build path to remove the "%v"
placeholder so it reads something like mailValidationError("failed to build
EML").WithCause(err), keeping the mailValidationError call and the
WithCause(err) chaining intact (look for the return statement that calls
mailValidationError(...).WithCause(err) in this function).
In `@shortcuts/mail/mail_watch.go`:
- Around line 210-212: The call inside the vfs.MkdirAll error path uses
mailFileIOError("cannot create output directory %q: %v", err, outputDir) with a
mismatched format argument order; update the mailFileIOError invocation in that
block so the format verbs and arguments align (either pass outputDir then err to
match "%q: %v", or drop the "%v" if the function attaches the cause itself) —
locate the vfs.MkdirAll error branch and fix the mailFileIOError call
accordingly.
---
Outside diff comments:
In `@shortcuts/mail/mail_triage.go`:
- Around line 1093-1110: The DoAPI error branch is currently always retried;
modify the block handling runtime.DoAPI errors so it wraps the error with
client.WrapDoAPIError and mailDecorateProblemMessage into lastErr, then consult
shouldRetryTriageAPIError(lastErr) before deciding to retry—i.e., if
!shouldRetryTriageAPIError(lastErr) or attempt == triageAPIRetries return nil,
lastErr, otherwise proceed to the sleep/retry; keep runtime.ClassifyAPIResponse
handling unchanged. Use the same symbols shown: runtime.DoAPI,
client.WrapDoAPIError, mailDecorateProblemMessage, shouldRetryTriageAPIError,
triageAPIRetries to locate and update the logic.
---
Nitpick comments:
In `@shortcuts/mail/draft/large_attachment_parse.go`:
- Line 4: Remove the file-level "//nolint:forbidigo" suppression and instead
place line-level "//nolint:forbidigo" comments directly on the specific
intermediate fmt.Errorf calls that perform the intentional error wraps in this
parser; locate each fmt.Errorf usage in this file (the intermediate
large-attachment parser error wrap sites) and add the nolint right after those
lines so other forbidigo violations elsewhere in the file remain linted.
🪄 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: f8009d50-c6dc-4810-a483-f13bf7f7e15f
📒 Files selected for processing (49)
.golangci.ymlinternal/errclass/classify.gointernal/errclass/codemeta_mail.golint/errscontract/rule_no_legacy_envelope_literal.goshortcuts/mail/body_file.goshortcuts/mail/draft/charset.goshortcuts/mail/draft/large_attachment_parse.goshortcuts/mail/draft/limits.goshortcuts/mail/draft/model.goshortcuts/mail/draft/parse.goshortcuts/mail/draft/patch.goshortcuts/mail/draft/patch_calendar.goshortcuts/mail/draft/serialize.goshortcuts/mail/draft/service.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/filecheck/filecheck.goshortcuts/mail/flag_suggest.goshortcuts/mail/flag_suggest_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/large_attachment.goshortcuts/mail/large_attachment_test.goshortcuts/mail/lint/linter.goshortcuts/mail/mail_decline_receipt.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_draft_send.goshortcuts/mail/mail_draft_send_test.goshortcuts/mail/mail_errors.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_lint_html.goshortcuts/mail/mail_message.goshortcuts/mail/mail_messages.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goshortcuts/mail/mail_send_receipt.goshortcuts/mail/mail_share_to_chat.goshortcuts/mail/mail_shortcut_validation_test.goshortcuts/mail/mail_signature.goshortcuts/mail/mail_template_create.goshortcuts/mail/mail_template_update.goshortcuts/mail/mail_thread.goshortcuts/mail/mail_triage.goshortcuts/mail/mail_watch.goshortcuts/mail/mail_watch_test.goshortcuts/mail/signature/provider.goshortcuts/mail/signature_compose.goshortcuts/mail/template_compose.go
evandance
force-pushed
the
feat/errs-migrate-mail
branch
from
d176e3c to
a137598
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1250 +/- ##
==========================================
+ Coverage 69.84% 70.06% +0.21%
==========================================
Files 666 668 +2
Lines 64451 64724 +273
==========================================
+ Hits 45019 45351 +332
+ Misses 15812 15735 -77
- Partials 3620 3638 +18 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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 `@shortcuts/mail/emlbuilder/builder.go`:
- Around line 63-66: The helper readFile opens an untrusted CLI-supplied path
without validating it; before calling fio.Open(path) in function readFile (and
keeping the existing nolint comment), call validate.SafeInputPath(path) and
handle/return any validation error (wrap it similar to the existing fmt.Errorf
message) so the package-level guard is explicit; reference
validate.SafeInputPath, the readFile function, and the path variable when making
this change.
In `@shortcuts/mail/helpers.go`:
- Around line 924-927: The current by-name helper loop (using idFn and returning
mailValidationError when id==value) incorrectly rejects exact IDs and routes
*Name resolvers through ID-returning helpers; update callers (resolveFolderName,
resolveLabelName, resolveFolderQueryName, resolveLabelQueryName) to first detect
an exact-ID input using idFn and, if it matches, call a new or existing
resolveNameValueByID (or equivalent) path that returns the name directly for
that ID, and only invoke the existing by-name helper for true name lookups;
remove or change the branch that returns mailValidationError when id==value so
exact IDs are accepted and only non-matching name lookups fall through to name
resolution.
🪄 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: efe5b45b-3dda-4c66-983f-42de09be8c3c
📒 Files selected for processing (50)
.golangci.ymlinternal/errclass/classify.gointernal/errclass/codemeta_mail.golint/errscontract/rule_no_legacy_envelope_literal.goshortcuts/mail/body_file.goshortcuts/mail/draft/charset.goshortcuts/mail/draft/large_attachment_parse.goshortcuts/mail/draft/limits.goshortcuts/mail/draft/model.goshortcuts/mail/draft/parse.goshortcuts/mail/draft/patch.goshortcuts/mail/draft/patch_calendar.goshortcuts/mail/draft/serialize.goshortcuts/mail/draft/service.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/filecheck/filecheck.goshortcuts/mail/flag_suggest.goshortcuts/mail/flag_suggest_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/large_attachment.goshortcuts/mail/large_attachment_test.goshortcuts/mail/lint/linter.goshortcuts/mail/mail_decline_receipt.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_draft_send.goshortcuts/mail/mail_draft_send_test.goshortcuts/mail/mail_errors.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_lint_html.goshortcuts/mail/mail_message.goshortcuts/mail/mail_messages.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goshortcuts/mail/mail_send_receipt.goshortcuts/mail/mail_share_to_chat.goshortcuts/mail/mail_shortcut_validation_test.goshortcuts/mail/mail_signature.goshortcuts/mail/mail_template_create.goshortcuts/mail/mail_template_update.goshortcuts/mail/mail_thread.goshortcuts/mail/mail_triage.goshortcuts/mail/mail_watch.goshortcuts/mail/mail_watch_test.goshortcuts/mail/signature/provider.goshortcuts/mail/signature_compose.goshortcuts/mail/template_compose.goshortcuts/register_test.go
✅ Files skipped from review due to trivial changes (10)
- shortcuts/mail/draft/patch_calendar.go
- shortcuts/mail/draft/serialize.go
- shortcuts/mail/draft/model.go
- shortcuts/mail/draft/patch.go
- shortcuts/mail/draft/large_attachment_parse.go
- shortcuts/mail/draft/limits.go
- shortcuts/mail/mail_shortcut_validation_test.go
- shortcuts/mail/draft/parse.go
- shortcuts/mail/lint/linter.go
- shortcuts/mail/filecheck/filecheck.go
🚧 Files skipped from review as they are similar to previous changes (34)
- shortcuts/mail/large_attachment_test.go
- shortcuts/mail/mail_message.go
- shortcuts/mail/mail_signature.go
- lint/errscontract/rule_no_legacy_envelope_literal.go
- shortcuts/mail/helpers_test.go
- shortcuts/mail/flag_suggest.go
- internal/errclass/codemeta_mail.go
- shortcuts/mail/draft/charset.go
- shortcuts/mail/mail_lint_html.go
- shortcuts/mail/mail_watch_test.go
- shortcuts/mail/mail_thread.go
- shortcuts/mail/mail_send_receipt.go
- shortcuts/mail/mail_decline_receipt.go
- shortcuts/mail/mail_reply.go
- shortcuts/mail/signature/provider.go
- shortcuts/mail/body_file.go
- .golangci.yml
- shortcuts/mail/mail_draft_create.go
- shortcuts/mail/mail_send.go
- shortcuts/mail/mail_template_create.go
- shortcuts/mail/mail_reply_all.go
- shortcuts/mail/mail_watch.go
- shortcuts/mail/template_compose.go
- shortcuts/mail/mail_triage.go
- shortcuts/mail/signature_compose.go
- shortcuts/mail/mail_errors.go
- shortcuts/mail/mail_draft_send_test.go
- shortcuts/mail/mail_draft_edit.go
- shortcuts/mail/draft/service.go
- shortcuts/mail/large_attachment.go
- shortcuts/mail/mail_draft_send.go
- shortcuts/mail/mail_share_to_chat.go
- shortcuts/mail/flag_suggest_test.go
- shortcuts/mail/mail_forward.go
evandance
force-pushed
the
feat/errs-migrate-mail
branch
2 times, most recently
from
ab6deb0 to
06b6197
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
shortcuts/mail/draft/serialize.go (1)
4-4: ⚡ Quick winAvoid file-wide
forbidigosuppression; scope it to specific lines.Line 4 suppresses
forbidigofor the whole file, which can mask unrelated violations later. Prefer local//nolint:forbidigoonly on the exactfmt.Errorfsites that need migration bridging.🤖 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/mail/draft/serialize.go` at line 4, Remove the file-wide "//nolint:forbidigo" at the top of serialize.go and instead add a line-scoped "//nolint:forbidigo" comment immediately next to each usage of fmt.Errorf in this file (e.g., inside functions that construct intermediate serializer errors such as SerializeDraft, DraftSerializer.Serialize, or any function returning fmt.Errorf) so the suppression only covers those specific error-bridge sites; keep the surrounding comment noting that the mail command layer wraps these into a typed ValidationError.
🤖 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/mail/mail_signature.go`:
- Line 113: The validation error uses the wrong flag name; replace the hardcoded
"--signature-id" in the mailValidationParamError call with the actual flag
exposed by this command ("--detail") so the message points users to the correct
flag; update the call at mailValidationParamError("--signature-id", "signature
not found: %s", sigID) to use "--detail" (keeping the "signature not found: %s"
format and sigID variable) in the mail signature handling function.
In `@shortcuts/mail/mail_triage.go`:
- Around line 927-929: The current check in the if uses !paramWantsSearch but
the inner predicates require a search token scenario, so the branch is
unreachable; change the condition to use paramWantsSearch (remove the negation)
so the check reads: if paramWantsSearch && (strings.TrimSpace(query) != "" ||
len(triageQueryFilterFields(filter)) > 0) { return false,
mailValidationParamError("--page-token", "...") } ensuring you keep the same
error message and references to query, filter, triageQueryFilterFields,
paramWantsSearch, and mailValidationParamError.
---
Nitpick comments:
In `@shortcuts/mail/draft/serialize.go`:
- Line 4: Remove the file-wide "//nolint:forbidigo" at the top of serialize.go
and instead add a line-scoped "//nolint:forbidigo" comment immediately next to
each usage of fmt.Errorf in this file (e.g., inside functions that construct
intermediate serializer errors such as SerializeDraft,
DraftSerializer.Serialize, or any function returning fmt.Errorf) so the
suppression only covers those specific error-bridge sites; keep the surrounding
comment noting that the mail command layer wraps these into a typed
ValidationError.
🪄 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: 657227bd-7e35-4ddd-b476-c0195f748554
📒 Files selected for processing (53)
.golangci.ymlinternal/errclass/classify.gointernal/errclass/codemeta_mail.golint/errscontract/rule_no_legacy_envelope_literal.goshortcuts/mail/body_file.goshortcuts/mail/draft/charset.goshortcuts/mail/draft/large_attachment_parse.goshortcuts/mail/draft/limits.goshortcuts/mail/draft/model.goshortcuts/mail/draft/parse.goshortcuts/mail/draft/patch.goshortcuts/mail/draft/patch_calendar.goshortcuts/mail/draft/serialize.goshortcuts/mail/draft/service.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/filecheck/filecheck.goshortcuts/mail/flag_suggest.goshortcuts/mail/flag_suggest_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/large_attachment.goshortcuts/mail/large_attachment_test.goshortcuts/mail/lint/linter.goshortcuts/mail/mail_decline_receipt.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_draft_send.goshortcuts/mail/mail_draft_send_test.goshortcuts/mail/mail_errors.goshortcuts/mail/mail_errors_test.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_lint_html.goshortcuts/mail/mail_message.goshortcuts/mail/mail_messages.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goshortcuts/mail/mail_send_receipt.goshortcuts/mail/mail_share_to_chat.goshortcuts/mail/mail_shortcut_validation_test.goshortcuts/mail/mail_signature.goshortcuts/mail/mail_template_create.goshortcuts/mail/mail_template_update.goshortcuts/mail/mail_thread.goshortcuts/mail/mail_triage.goshortcuts/mail/mail_triage_test.goshortcuts/mail/mail_watch.goshortcuts/mail/mail_watch_test.goshortcuts/mail/signature/provider.goshortcuts/mail/signature_compose.goshortcuts/mail/signature_compose_test.goshortcuts/mail/template_compose.goshortcuts/register_test.go
✅ Files skipped from review due to trivial changes (11)
- shortcuts/mail/lint/linter.go
- shortcuts/mail/draft/patch.go
- shortcuts/mail/draft/limits.go
- shortcuts/mail/draft/patch_calendar.go
- shortcuts/mail/large_attachment_test.go
- shortcuts/mail/draft/large_attachment_parse.go
- shortcuts/mail/draft/charset.go
- shortcuts/mail/draft/parse.go
- shortcuts/mail/large_attachment.go
- shortcuts/mail/draft/model.go
- shortcuts/mail/mail_shortcut_validation_test.go
evandance
changed the title
evandance
force-pushed
the
feat/errs-migrate-mail
branch
from
06b6197 to
f5c6aa7
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/mail/mail_draft_edit.go (1)
509-514:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate
--patch-filebefore opening it.Line 511 reads a CLI-supplied path directly. Add an explicit safe-path validation gate before
Openso untrusted path input is rejected early and consistently.Proposed fix
func loadPatchFile(runtime *common.RuntimeContext, path string) (draftpkg.Patch, error) { var patch draftpkg.Patch + if err := runtime.ValidatePath(path); err != nil { + return patch, mailValidationParamError("--patch-file", "--patch-file %q: %v", path, err).WithCause(err) + } f, err := runtime.FileIO().Open(path) if err != nil { return patch, mailValidationParamError("--patch-file", "--patch-file %q: %v", path, err).WithCause(mailInputStatError(err)) }As per coding guidelines
**/*.go: “Validate paths before reading withvalidate.SafeInputPathbecause CLI arguments are untrusted (they come from AI agents)”.🤖 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/mail/mail_draft_edit.go` around lines 509 - 514, In loadPatchFile, validate the CLI-supplied path with validate.SafeInputPath before calling runtime.FileIO().Open so untrusted input is rejected early; if validate.SafeInputPath(path) returns an error, return the same mailValidationParamError used for Open (e.g., mailValidationParamError("--patch-file", "--patch-file %q: %v", path, err).WithCause(mailInputStatError(err))) so the caller sees a consistent validation error; keep the subsequent Open only after the SafeInputPath check.
♻️ Duplicate comments (1)
shortcuts/mail/mail_watch.go (1)
210-212:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the
mailFileIOErrorformat placeholder mismatch.The format string has two verbs (
%q: %v), but only one formatting argument is supplied after the cause, which can produce malformed user-facing text.Proposed fix
- if err := vfs.MkdirAll(outputDir, 0700); err != nil { - return mailFileIOError("cannot create output directory %q: %v", err, outputDir) - } + if err := vfs.MkdirAll(outputDir, 0700); err != nil { + return mailFileIOError("cannot create output directory %q", err, outputDir) + }🤖 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/mail/mail_watch.go` around lines 210 - 212, The format string passed to mailFileIOError in the vfs.MkdirAll error branch uses two verbs ("%q: %v") but supplies only the error plus outputDir in the wrong order, causing incorrect formatting; fix this by calling mailFileIOError with arguments matching the format verbs (e.g., pass outputDir first then err) or simplify the format to a single verb and supply the corresponding value—update the call near vfs.MkdirAll so the format string and arguments (outputDir, err) align with mailFileIOError.
🤖 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/mail/helpers.go`:
- Around line 2711-2714: The validator currently uses
base64.URLEncoding.DecodeString which rejects valid unpadded base64url IDs;
update the decode logic in the function around the DecodeString call (the block
referencing decoded, err, id, index and mailValidationParamError) to first try
base64.RawURLEncoding.DecodeString(id) and if that returns an error, fall back
to base64.URLEncoding.DecodeString(id); retain the existing
mailValidationParamError return when both attempts fail or decoded length is
zero so both unpadded (raw) and padded values are accepted.
---
Outside diff comments:
In `@shortcuts/mail/mail_draft_edit.go`:
- Around line 509-514: In loadPatchFile, validate the CLI-supplied path with
validate.SafeInputPath before calling runtime.FileIO().Open so untrusted input
is rejected early; if validate.SafeInputPath(path) returns an error, return the
same mailValidationParamError used for Open (e.g.,
mailValidationParamError("--patch-file", "--patch-file %q: %v", path,
err).WithCause(mailInputStatError(err))) so the caller sees a consistent
validation error; keep the subsequent Open only after the SafeInputPath check.
---
Duplicate comments:
In `@shortcuts/mail/mail_watch.go`:
- Around line 210-212: The format string passed to mailFileIOError in the
vfs.MkdirAll error branch uses two verbs ("%q: %v") but supplies only the error
plus outputDir in the wrong order, causing incorrect formatting; fix this by
calling mailFileIOError with arguments matching the format verbs (e.g., pass
outputDir first then err) or simplify the format to a single verb and supply the
corresponding value—update the call near vfs.MkdirAll so the format string and
arguments (outputDir, err) align with mailFileIOError.
🪄 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: 148f70a3-6b0b-4d70-bc80-38fd40a693b3
📒 Files selected for processing (52)
.golangci.ymlinternal/errclass/classify.gointernal/errclass/codemeta_mail.golint/errscontract/rule_no_legacy_envelope_literal.goshortcuts/mail/body_file.goshortcuts/mail/draft/charset.goshortcuts/mail/draft/large_attachment_parse.goshortcuts/mail/draft/limits.goshortcuts/mail/draft/model.goshortcuts/mail/draft/parse.goshortcuts/mail/draft/patch.goshortcuts/mail/draft/patch_calendar.goshortcuts/mail/draft/serialize.goshortcuts/mail/draft/service.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/filecheck/filecheck.goshortcuts/mail/flag_suggest.goshortcuts/mail/flag_suggest_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/large_attachment.goshortcuts/mail/large_attachment_test.goshortcuts/mail/lint/linter.goshortcuts/mail/mail_decline_receipt.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_draft_send.goshortcuts/mail/mail_draft_send_test.goshortcuts/mail/mail_errors.goshortcuts/mail/mail_errors_test.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_lint_html.goshortcuts/mail/mail_message.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goshortcuts/mail/mail_send_receipt.goshortcuts/mail/mail_share_to_chat.goshortcuts/mail/mail_shortcut_validation_test.goshortcuts/mail/mail_signature.goshortcuts/mail/mail_template_create.goshortcuts/mail/mail_template_update.goshortcuts/mail/mail_thread.goshortcuts/mail/mail_triage.goshortcuts/mail/mail_triage_test.goshortcuts/mail/mail_watch.goshortcuts/mail/mail_watch_test.goshortcuts/mail/signature/provider.goshortcuts/mail/signature_compose.goshortcuts/mail/signature_compose_test.goshortcuts/mail/template_compose.goshortcuts/register_test.go
✅ Files skipped from review due to trivial changes (13)
- shortcuts/mail/draft/parse.go
- shortcuts/mail/draft/serialize.go
- lint/errscontract/rule_no_legacy_envelope_literal.go
- shortcuts/mail/draft/patch.go
- shortcuts/mail/mail_shortcut_validation_test.go
- shortcuts/mail/draft/limits.go
- shortcuts/mail/draft/large_attachment_parse.go
- shortcuts/mail/signature/provider.go
- shortcuts/mail/lint/linter.go
- shortcuts/mail/draft/charset.go
- shortcuts/mail/draft/model.go
- shortcuts/mail/filecheck/filecheck.go
- shortcuts/mail/emlbuilder/builder.go
evandance
force-pushed
the
feat/errs-migrate-mail
branch
from
f5c6aa7 to
381c14e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/mail/draft/charset.go`:
- Line 4: Remove the file-scoped "//nolint:forbidigo" in
shortcuts/mail/draft/charset.go and instead apply a targeted inline nolint on
the specific fmt.Errorf call inside encodeTextCharset (the "unsupported charset
%q" error) so only that call suppresses forbidigo; keep encodeTextCharset's
behavior unchanged (do not propagate the error) because encodedLeafBody in
shortcuts/mail/draft/serialize.go swallows/resets charset, but explicitly
narrowing the nolint makes the intent clearer and avoids silencing other
forbidigo uses in the file.
🪄 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: dc8b8c40-1f72-44de-912f-ad162455327e
📒 Files selected for processing (52)
.golangci.ymlinternal/errclass/classify.gointernal/errclass/codemeta_mail.golint/errscontract/rule_no_legacy_envelope_literal.goshortcuts/mail/body_file.goshortcuts/mail/draft/charset.goshortcuts/mail/draft/large_attachment_parse.goshortcuts/mail/draft/limits.goshortcuts/mail/draft/model.goshortcuts/mail/draft/parse.goshortcuts/mail/draft/patch.goshortcuts/mail/draft/patch_calendar.goshortcuts/mail/draft/serialize.goshortcuts/mail/draft/service.goshortcuts/mail/emlbuilder/builder.goshortcuts/mail/filecheck/filecheck.goshortcuts/mail/flag_suggest.goshortcuts/mail/flag_suggest_test.goshortcuts/mail/helpers.goshortcuts/mail/helpers_test.goshortcuts/mail/large_attachment.goshortcuts/mail/large_attachment_test.goshortcuts/mail/lint/linter.goshortcuts/mail/mail_decline_receipt.goshortcuts/mail/mail_draft_create.goshortcuts/mail/mail_draft_edit.goshortcuts/mail/mail_draft_send.goshortcuts/mail/mail_draft_send_test.goshortcuts/mail/mail_errors.goshortcuts/mail/mail_errors_test.goshortcuts/mail/mail_forward.goshortcuts/mail/mail_lint_html.goshortcuts/mail/mail_message.goshortcuts/mail/mail_reply.goshortcuts/mail/mail_reply_all.goshortcuts/mail/mail_send.goshortcuts/mail/mail_send_receipt.goshortcuts/mail/mail_share_to_chat.goshortcuts/mail/mail_shortcut_validation_test.goshortcuts/mail/mail_signature.goshortcuts/mail/mail_template_create.goshortcuts/mail/mail_template_update.goshortcuts/mail/mail_thread.goshortcuts/mail/mail_triage.goshortcuts/mail/mail_triage_test.goshortcuts/mail/mail_watch.goshortcuts/mail/mail_watch_test.goshortcuts/mail/signature/provider.goshortcuts/mail/signature_compose.goshortcuts/mail/signature_compose_test.goshortcuts/mail/template_compose.goshortcuts/register_test.go
✅ Files skipped from review due to trivial changes (11)
- shortcuts/mail/draft/limits.go
- shortcuts/mail/draft/large_attachment_parse.go
- shortcuts/mail/draft/parse.go
- shortcuts/mail/draft/patch.go
- shortcuts/mail/draft/patch_calendar.go
- shortcuts/mail/large_attachment_test.go
- shortcuts/mail/mail_shortcut_validation_test.go
- shortcuts/mail/draft/serialize.go
- shortcuts/mail/draft/model.go
- shortcuts/mail/filecheck/filecheck.go
- shortcuts/mail/emlbuilder/builder.go
🚧 Files skipped from review as they are similar to previous changes (31)
- shortcuts/mail/lint/linter.go
- lint/errscontract/rule_no_legacy_envelope_literal.go
- shortcuts/mail/mail_thread.go
- shortcuts/mail/mail_signature.go
- shortcuts/mail/mail_lint_html.go
- shortcuts/register_test.go
- shortcuts/mail/mail_share_to_chat.go
- internal/errclass/classify.go
- shortcuts/mail/mail_message.go
- shortcuts/mail/helpers_test.go
- shortcuts/mail/mail_reply.go
- shortcuts/mail/mail_send.go
- shortcuts/mail/mail_watch_test.go
- internal/errclass/codemeta_mail.go
- shortcuts/mail/mail_reply_all.go
- shortcuts/mail/mail_decline_receipt.go
- shortcuts/mail/mail_send_receipt.go
- shortcuts/mail/flag_suggest.go
- shortcuts/mail/mail_template_create.go
- shortcuts/mail/mail_template_update.go
- shortcuts/mail/mail_draft_send.go
- shortcuts/mail/signature/provider.go
- shortcuts/mail/mail_errors_test.go
- shortcuts/mail/signature_compose.go
- shortcuts/mail/template_compose.go
- shortcuts/mail/signature_compose_test.go
- shortcuts/mail/large_attachment.go
- shortcuts/mail/mail_triage.go
- shortcuts/mail/mail_watch.go
- shortcuts/mail/mail_draft_edit.go
- shortcuts/mail/helpers.go
evandance
force-pushed
the
feat/errs-migrate-mail
branch
4 times, most recently
from
f7cb07a to
0d9a104
Compare
Replace every produced error path in shortcuts/mail with typed errs.* envelopes, so consumers get stable category, subtype, param/params, hint, retryable, and log_id metadata for classification and recovery instead of free-form message text. - Locally constructed mail errors move from output.Err* / output.Errorf / final fmt.Errorf / common legacy helpers to errs.* builders, with structured params on multi-flag validation and failed-precondition states kept non-retryable. - API-call failures move from runtime.CallAPI / DoAPIJSON legacy boundaries to runtime.CallAPITyped or runtime.ClassifyAPIResponse, and mail-specific enrichers read errs.ProblemOf so typed code, subtype, hint, and log_id metadata are preserved. - Batch draft-send partial failures now use runtime.OutPartialFailure so successful and failed draft sends stay in stdout while the command exits through a typed multi-status signal. - Add mail-domain typed helpers, mail API code metadata, and guard wiring to keep shortcuts/mail from reintroducing legacy envelopes or legacy API calls. - Keep genuine intermediate fmt.Errorf wraps in parser/builder layers annotated with nolint comments; command-facing paths wrap them into typed validation, API, network, or internal errors.
evandance
force-pushed
the
feat/errs-migrate-mail
branch
from
0d9a104 to
45af782
Compare
github-actions
Bot
added
size/XL
chanthuang
reviewed
Jun 4, 2026
When an account-level failure interrupts a batch send after some drafts already went out, the command previously produced two machine-readable failure results: the partial-failure ledger on stdout and a second error envelope on stderr. Consumers could not tell which one to recover from. The batch ledger is now the only failure result for that case: it gains aborted and abort_error fields carrying the typed cause, so callers can see which drafts were sent, which failed, why the batch stopped, and how to recover — all from stdout. A --stop-on-error stop keeps these fields unset because stopping early there is the caller's own choice.
chanthuang
approved these changes
Jun 4, 2026
chanthuang
left a comment
chanthuang
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. The aborted +draft-send path now emits the batch ledger as the single failure result and returns the envelope-less partial-failure signal, so the previous double-envelope concern is resolved.\n\nVerified locally: ok github.com/larksuite/cli/shortcuts/mail (cached) and ok github.com/larksuite/cli/shortcuts/mail 1.669s
ok github.com/larksuite/cli/shortcuts/common 0.595s.\n\n
chanthuang
approved these changes
Jun 4, 2026
This was referenced Jun 5, 2026
tuxedomm
pushed a commit
to zhumiaoxin/cli
that referenced
this pull request
Jun 6, 2026
…suite#1250) * feat(mail): return typed error envelopes across the mail domain Replace every produced error path in shortcuts/mail with typed errs.* envelopes, so consumers get stable category, subtype, param/params, hint, retryable, and log_id metadata for classification and recovery instead of free-form message text. - Locally constructed mail errors move from output.Err* / output.Errorf / final fmt.Errorf / common legacy helpers to errs.* builders, with structured params on multi-flag validation and failed-precondition states kept non-retryable. - API-call failures move from runtime.CallAPI / DoAPIJSON legacy boundaries to runtime.CallAPITyped or runtime.ClassifyAPIResponse, and mail-specific enrichers read errs.ProblemOf so typed code, subtype, hint, and log_id metadata are preserved. - Batch draft-send partial failures now use runtime.OutPartialFailure so successful and failed draft sends stay in stdout while the command exits through a typed multi-status signal. - Add mail-domain typed helpers, mail API code metadata, and guard wiring to keep shortcuts/mail from reintroducing legacy envelopes or legacy API calls. - Keep genuine intermediate fmt.Errorf wraps in parser/builder layers annotated with nolint comments; command-facing paths wrap them into typed validation, API, network, or internal errors. * fix(mail): report aborted draft-send batches as a single failure result When an account-level failure interrupts a batch send after some drafts already went out, the command previously produced two machine-readable failure results: the partial-failure ledger on stdout and a second error envelope on stderr. Consumers could not tell which one to recover from. The batch ledger is now the only failure result for that case: it gains aborted and abort_error fields carrying the typed cause, so callers can see which drafts were sent, which failed, why the batch stopped, and how to recover — all from stdout. A --stop-on-error stop keeps these fields unset because stopping early there is the caller's own choice.
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/mailreturned many command-facing errors through legacyoutput.Err*/output.Errorf/ finalfmt.Errorfpaths and legacy API-call envelopes, so consumers had to recover classification from free-form message text. This migrates the mail domain to typederrs.*envelopes, giving callers stabletype,subtype,param/params,hint,retryable, andlog_idmetadata across validation, API, network, file-I/O, and partial-failure flows. The shared drive-media upload helpers gain additive typed counterparts so mail's attachment uploads emit typed envelopes as well.Changes
shortcuts/mailfrom legacy output constructors, common legacy wrappers, and final barefmt.Errorfreturns to typederrs.*buildersruntime.CallAPI/ JSON helpers to typed calls orruntime.ClassifyAPIResponse, and update mail-specific enrichers to read and preserveerrs.ProblemOfmetadatashortcuts/mail/mail_errors.gowith mail-local typed helpers for validation, file I/O, invalid-response, problem-message decoration, and hint enrichmentUploadDriveMediaAllTyped/UploadDriveMediaMultipartTypedtoshortcuts/commonand deprecate the legacy pair (legacy implementations unchanged); mail's large-attachment and template-attachment uploads consume the typed variants so upload failures carry typed subtype / code / log_idruntime.OutPartialFailureso successful and failed draft sends stay structured in stdout while the command exits with a typed multi-status signal; account-level fatal classification reads typed category/subtype/code (unregistered not_found codes stay per-draft recoverable)--message-idsaccepts unpadded base64url IDs; attachment and patch-file paths are validated withSafeInputPath/ValidatePathbefore reading; unknown-flag errors emit typed validation envelopes with candidate suggestions inparamsshortcuts/commonTest Plan
gofmt -l .go vet ./...go test -race ./shortcuts/mail/... ./shortcuts/ ./shortcuts/common/ ./internal/errclass/ ./internal/output/ ./errs/go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/main ./shortcuts/mail/... ./shortcuts/common/... ./internal/errclass/...— 0 issuesgo run -C lint . ..go build ./...Related Issues
N/A