chore(mail): sync mail shortcut updates#1406
Closed
bubbmon233 wants to merge 1 commit into
Closed
Conversation
github-actions
Bot
added
domain/mail
📝 WalkthroughWalkthroughThis PR modernizes error handling across all mail shortcuts by removing the custom ChangesMail Shortcuts Error Handler Modernization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@deb2bcf48e922c70029b58afc01e209573a4d4fa🧩 Skill updatenpx skills add bubbmon233/cli#chore/mail-updates -y -g |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (43.73%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #1406 +/- ##
==========================================
- Coverage 72.83% 72.48% -0.35%
==========================================
Files 731 730 -1
Lines 69111 68952 -159
==========================================
- Hits 50335 49978 -357
- Misses 14999 15216 +217
+ Partials 3777 3758 -19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
shortcuts/mail/draft/service.go (2)
34-50:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical: API call and error construction violate coding guidelines.
The changes in
GetRaw(and throughout this file) systematically violate the repository's documented error handling contract:
Line 34:
runtime.CallAPI(untyped) replacesruntime.CallAPITyped. Per coding guidelines: "Useruntime.CallAPITyped(shortcuts) orerrclass.BuildAPIError(raw responses) for Lark API errors — never hand-build error envelopes."Line 40:
fmt.Errorfreplaces typederrs.NewInternalError(..., errs.SubtypeInvalidResponse, ...). Per coding guidelines: "Command-facing failures must be typederrs.*errors — never use legacyoutput.Err*helpers or barefmt.Errorf."The same pattern repeats at lines 58/64 (
CreateWithRaw), 79 (UpdateWithRaw), and 102 (Send). This downgrade loses typed metadata (category, subtype,.WithCausechains) that downstream error handlers and tests rely on for classification and recovery logic.🤖 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/service.go` around lines 34 - 50, GetRaw (and similarly CreateWithRaw, UpdateWithRaw, Send) replaced typed API/error helpers with untyped calls and plain fmt errors; revert to using runtime.CallAPITyped (or errclass.BuildAPIError for raw responses) instead of runtime.CallAPI, and replace bare fmt.Errorf constructions with the repository's typed error builder (e.g., errs.NewInternalError(..., errs.SubtypeInvalidResponse, ...) and attach the underlying cause via .WithCause). Locate CallAPI usages in GetRaw/CreateWithRaw/UpdateWithRaw/Send and swap to CallAPITyped or BuildAPIError as appropriate, and change the empty-raw or missing-ID error creation (where extractRawEML or extractDraftID are validated) to construct the typed errs.NewInternalError with errs.SubtypeInvalidResponse and include the original response or error as the cause.Source: Coding guidelines
34-102:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical: PR systematically violates error handling and API call guidelines across all mail shortcuts.
This PR replaces typed
errs.*errors withoutput.ErrValidationand barefmt.Errorfacross every reviewed file, directly contradicting the repository's coding guidelines:Command-facing failures must be typed
errs.*errors — never use legacyoutput.Err*helpers or barefmt.ErrorfThe guidelines specify:
errs.NewValidationError(errs.SubtypeInvalidArgument, ...).WithParam("--flag")for user flag/arg validationerrs.NewInternalError(errs.SubtypeUnknown, ...).WithCause(err)for unclassified lower-layer errorsruntime.CallAPITyped(shortcuts) for Lark API errorsYet the PR:
- Replaces
CallAPITyped→CallAPI(draft/service.go lines 34, 58, 79, 102)- Replaces
errs.NewValidationError→output.ErrValidation(mail_draft_create.go L94/179/182, mail_draft_edit.go L91/107/425/451/etc., mail_send.go L86)- Replaces typed error wrapping → bare
fmt.Errorf(...: %w)(all execute paths in mail_reply.go, mail_reply_all.go, mail_send.go, mail_send_receipt.go)This downgrade strips typed metadata (category/subtype/param) that downstream error handlers, test assertions (
errs.ProblemOf), and recovery logic depend on. Before merging, either:
- Revert to typed
errs.*error construction per guidelines, or- Update the coding guidelines document if this represents a new architectural direction, and coordinate test/handler updates across the codebase.
🤖 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/service.go` around lines 34 - 102, The PR replaced typed errors and typed API calls with bare fmt.Errorf/output.ErrValidation and runtime.CallAPI; revert these in the mail shortcuts: use runtime.CallAPITyped instead of CallAPI in the functions shown (the draft fetch/write/send helpers including the GetDraftRaw flow, CreateWithRaw, UpdateWithRaw, and Send) and replace bare errors with the repository's typed errors (e.g., wrap lower-layer failures with errs.NewInternalError(errs.SubtypeUnknown, "...").WithCause(err) for API/IO failures and use errs.NewValidationError(errs.SubtypeInvalidArgument, "...").WithParam("--flag") for user/flag validation), ensuring extractDraftID/CallAPITyped error paths return properly typed errs.* values rather than fmt.Errorf or output.ErrValidation.Source: Coding guidelines
shortcuts/mail/mail_draft_edit.go (2)
91-218:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Execute path error handling violates coding guidelines.
The
Executefunction's error handling has been systematically downgraded from typederrs.*errors to untypedoutput.ErrValidationandfmt.Errorf:
- Lines 91, 107, 126-127, 150, 154, 169, 209, 214:
output.ErrValidation(...)violations- Lines 103, 218, 273, 277:
fmt.Errorf(...)violationsPer coding guidelines: "Command-facing failures must be typed
errs.*errors — never use legacyoutput.Err*helpers or barefmt.Errorf."Validation failures (lines 91, 107, etc.) should use
errs.NewValidationError(errs.SubtypeInvalidArgument, ...).WithParam(...). Execution failures (lines 103, 218) wrapping API/parse errors should useerrs.NewInternalError(errs.SubtypeUnknown, ...).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/mail/mail_draft_edit.go` around lines 91 - 218, The Execute path in mail_draft_edit.go downgrades typed errs.* errors to output.ErrValidation and fmt.Errorf; update each validation failure (e.g., the "--draft-id is required" check, parse draft raw EML failure, parseEventTimeRange/set_calendar validation branches, set_calendar/ICS build failure, serialize draft failed, apply draft patch failed, etc.) to return errs.NewValidationError(errs.SubtypeInvalidArgument, "<short message>").WithParam("<paramName>", "<value>") or a suitable validation subtype and message, and convert execution/internal failures that wrap underlying errors (e.g., draftpkg.GetRaw, draftpkg.Parse, draftpkg.Apply, draftpkg.Serialize, draftpkg.UpdateWithRaw and other API/IO errors) to return errs.NewInternalError(errs.SubtypeUnknown, "<short context message>").WithCause(err); locate these spots by searching in Execute for calls to output.ErrValidation and fmt.Errorf and replace them with the appropriate errs.* constructors and WithCause/WithParam chaining to preserve original error context.Source: Coding guidelines
425-486:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: buildDraftEditPatch validation errors violate coding guidelines.
Lines 425, 451, 454, 461, 464, and 478 use
output.ErrValidation(...)for mutual-exclusion checks and required-flag validation. Per coding guidelines, these should beerrs.NewValidationError(errs.SubtypeInvalidArgument, ...).WithParam(...)to preserve typed metadata for error classification.Additionally, line 486 returns
patch.Validate()directly. Verify thatpatch.Validate()returns a properly typederrs.*error, not a bare Go error, to maintain contract consistency.🤖 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 425 - 486, Replace the uses of output.ErrValidation(...) in buildDraftEditPatch with typed validation errors using errs.NewValidationError(errs.SubtypeInvalidArgument, "<message>").WithParam("<paramName>", "<paramValue>") (e.g., for mutual-exclusion/required-flag checks around set-event-* and body/patch-file logic replace output.ErrValidation calls with errs.NewValidationError and attach a param like "flag" or "combination" to preserve metadata); likewise for the other validation branches that currently return output.ErrValidation. Also ensure the final return uses a typed errs error: check patch.Validate() in draftpkg.Patch.Validate() and if it can return a plain error, wrap it with errs.NewValidationError (or convert it) before returning so the function always returns an errs.* typed validation error. Use function name buildDraftEditPatch and methods patch.Validate()/draftpkg.Patch to locate the spots.Source: Coding guidelines
shortcuts/mail/mail_send_receipt.go (1)
116-134:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Execute path error handling violates coding guidelines.
Lines 116, 119, 129, 134, 161, 166, and 170 use bare
fmt.Errorf(some with%wwrapping). Per coding guidelines: "Command-facing failures must be typederrs.*errors — never use legacyoutput.Err*helpers or barefmt.Errorf."
- Line 116: Fetch failure should use
errs.NewInternalError(...).WithCause(err)- Lines 119, 129, 134: Validation failures (missing label, missing sender) should use
errs.NewValidationError(errs.SubtypeFailedPrecondition, ...).WithHint(...)- Lines 161, 166, 170: EML build, draft creation, and send failures should use
errs.NewInternalError(...).WithCause(err)Also applies to: 161-170
🤖 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_send_receipt.go` around lines 116 - 134, Replace bare fmt.Errorf usages with typed errs.* errors per guidelines: for the fetch failure in the block that returns fmt.Errorf("failed to fetch original message: %w", err) wrap the cause using errs.NewInternalError("failed to fetch original message").WithCause(err); for validation checks (the branches that return fmt.Errorf about missing read receipt label and missing sender when hasReadReceiptRequestLabel(msg) or origFromEmail == "") return errs.NewValidationError(errs.SubtypeFailedPrecondition, "read receipt not requested" or "original message has no sender address").WithHint("provide the read-receipt label" or "specify --from explicitly"); for later EML build / draft creation / send failures (the returns around buildEML, createDraft, sendMessage) replace fmt.Errorf(...) with errs.NewInternalError("...").WithCause(err) preserving context in the message. Update the error messages to include the messageID or relevant context and keep the original error as the cause.Source: Coding guidelines
shortcuts/mail/mail_draft_send_test.go (1)
786-889:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical: Test input construction violates typed error contract.
Lines 786-889 changed test case inputs from
errs.*typed errors to*output.ExitError/output.Errorf(...). Per coding guidelines, the production code should return typederrs.*errors, and tests should validate them viaerrs.ProblemOf.The test cases should construct and verify typed errors:
{ - name: "auth → fatal", - err: &output.ExitError{ - Code: output.ExitAuth, - Detail: &output.ErrDetail{Type: "auth", Message: "token expired"}, - }, + name: "auth → fatal", + err: errs.NewAuthenticationError(errs.SubtypeTokenExpired, "token expired"), want: true, },Similar changes needed for app_status, config, permission, rate_limit, network, and API error cases. The production
isFatalSendErrshould classify viaerrs.ProblemOfinspection, not*output.ExitErrortype switches.🤖 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_send_test.go` around lines 786 - 889, Tests are constructing raw *output.ExitError / output.Errorf values but should use the typed error contract (errs.*) and isFatalSendErr should inspect via errs.ProblemOf; replace each test case that builds &output.ExitError{...} or output.Errorf(...) with an errs problem instance that represents the same condition (use errs.ProblemOf / the specific errs.* constructors to create auth, app_status, config, permission, rate_limit, network and API problems) and update isFatalSendErr to determine fatality by calling errs.ProblemOf(err) and examining the Problem.Type/Code instead of switching on *output.ExitError or concrete output errors so the tests and production behavior match the typed-error contract.Source: Coding guidelines
🧹 Nitpick comments (4)
shortcuts/mail/large_attachment.go (1)
124-128: 💤 Low valueError typing migration contradicts coding guidelines.
The coding guidelines state: "Command-facing failures must be typed
errs.*errors — never use legacyoutput.Err*helpers or barefmt.Errorf". This file migrates multiple error paths from typed errors to plainfmt.Errorf:
- Line 128:
fmt.Errorf("failed to stat attachment %s: %w", ...)- Line 147:
fmt.Errorf("large attachment upload requires user identity...")- Line 184:
fmt.Errorf("failed to upload large attachment %s: %w", ...)- Lines 400, 410, 425, 434, 458: Various validation/precondition errors
- Lines 591, 609, 619, 675: Same pattern in
preprocessLargeAttachmentsForDraftEditGiven the PR objectives indicate this is an intentional sync of error handling updates from an internal branch, I'm flagging this for awareness. If this migration is approved project-wide, consider updating the coding guidelines to reflect the new error convention for mail shortcuts.
Also applies to: 147-147, 159-184, 400-401, 409-412, 424-427, 429-437, 457-459, 590-593, 608-611, 618-622, 674-676
🤖 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/large_attachment.go` around lines 124 - 128, Several error returns in shortcuts/mail/large_attachment.go use plain fmt.Errorf but must use the project’s command-facing typed errors (errs.*) per guidelines; find the plain fmt.Errorf usages in the large attachment handling functions (e.g., the stat/upload paths and validation branches inside the functions that perform attachment processing and preprocessLargeAttachmentsForDraftEdit) and replace each fmt.Errorf call with the appropriate errs.* constructor or wrapper (e.g., errs.New, errs.InvalidArgument, errs.FailedPrecondition, or a wrapping helper used elsewhere in the repo) so command-facing failures are typed; ensure wrapped errors still preserve the underlying error (use the errs.* variant that supports wrapping) and update all occurrences referenced in the review (stat/upload error paths and the various validation/precondition errors) to follow the typed errs convention.Source: Coding guidelines
shortcuts/mail/mail_watch.go (1)
719-726: 💤 Low valueConsider using
json.DecoderwithUseNumberfor consistency.This differs from
doJSONAPIinmail_triage.gowhich usesjson.NewDecoderwithUseNumber()to preserve numeric precision. While the fields extracted here (code,msg,data) are unlikely to suffer precision loss, using the same pattern would be more consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/mail/mail_watch.go` around lines 719 - 726, Replace the json.Unmarshal usage on apiResp.RawBody with a json.NewDecoder(...).UseNumber() + Decode to preserve numeric precision and match the pattern used in doJSONAPI in mail_triage.go; specifically, decode into the existing result map[string]interface{} (the block handling apiResp.RawBody and extracting result["code"], result["msg"], result["data"]) so numeric fields are retained as json.Number rather than float64 and the later checks for code/msg remain unchanged.shortcuts/mail/mail_watch_test.go (1)
526-545: ⚡ Quick winTest asserts message strings instead of typed error metadata.
Per coding guidelines for test files:
"Error-path tests must assert typed metadata via
errs.ProblemOf(category/subtype/param) and cause preservation, not message substrings alone"This test checks
err.Error()string contents. However, since the production code now uses*output.ExitErrorinstead of typederrs.*errors, asserting viaerrs.ProblemOfis not applicable.If the error handling migration is confirmed, consider asserting the
*output.ExitErrorstructure (Code, Detail.Type, etc.) rather than just message substrings for stronger contract tests.🤖 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_test.go` around lines 526 - 545, TestWrapWatchSubscribeErrorExitError currently asserts on err.Error() substrings; update it to assert the typed *output.ExitError metadata instead: use errors.As or a type assertion on the returned error from wrapWatchSubscribeError to get an *output.ExitError and then assert ExitError.Code == output.ExitAPI and ExitError.Detail.Type == "api_error" and that ExitError.Detail.Message (and optionally Detail.Hint) contain the expected text; reference the test name TestWrapWatchSubscribeErrorExitError and the production helper wrapWatchSubscribeError to locate the change.Source: Coding guidelines
shortcuts/mail/mail_shortcut_validation_test.go (1)
19-28: ⚡ Quick winHelper accepts both error contracts during migration period.
The updated comment and dual-path assertion (
errs.IsValidation(err) && code != output.ExitValidation) allow the test suite to work with both typederrs.ValidationError(per guidelines) andoutput.ExitErrorduring the migration. However, per coding guidelines, the end state should be typederrs.*errors only—output.Err*helpers are explicitly labeled "legacy" in the guidelines and must not be used.Once all mail shortcuts return typed
errs.ValidationError, remove theoutput.ExitCodeOffallback path (lines 27, 30, 33) and simplify to asserterrs.IsValidation(err)only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/mail/mail_shortcut_validation_test.go` around lines 19 - 28, The test helper assertValidationError should stop accepting the legacy output.ExitError contract; remove the fallback checks that use output.ExitCodeOf and output.ExitValidation and simplify the assertions to only validate the typed error contract using errs.IsValidation(err). Locate the assertValidationError function and delete the branches/conditions that call output.ExitCodeOf or compare against output.ExitValidation, then replace them with a single assertion that errs.IsValidation(err) is true (and keep the existing nil check and t.Helper call).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shortcuts/mail/body_file.go`:
- Line 54: Replace uses of output.ErrValidation in shortcuts/mail/body_file.go
with typed errs.Validation errors per guidelines: for the mutual-exclusion check
(the return that now says "--body and --body-file are mutually exclusive; pass
exactly one") return errs.NewValidationError(errs.SubtypeInvalidArgument,
"...").WithParam("--body") or include both flags as context; for the body-file
path safety check replace output.ErrValidation with
errs.NewValidationError(errs.SubtypeInvalidArgument,
"<message>").WithParam("--body-file"); for the "required resolved body"
validation replace output.ErrValidation with
errs.NewValidationError(errs.SubtypeInvalidArgument,
"<message>").WithParam("--body"); and for the body-file I/O and size limit
failures (the three sites that currently return output.ErrValidation) return
errs.NewValidationError(errs.SubtypeInvalidArgument, "<io/size
message>").WithParam("--body-file"). Locate these replacements at the existing
return sites that reference output.ErrValidation in the file and keep the
original messages while using errs.NewValidationError(...).WithParam(...) to
attach the relevant flag name.
In `@shortcuts/mail/flag_suggest_test.go`:
- Around line 178-207: Update the tests that currently expect an
*output.ExitError from flagSuggestErrorFunc (e.g.,
TestFlagSuggestErrorFunc_LongUnknown_ReturnsExitError and the other affected
tests) to instead assert the error is a validation problem: use
errs.ProblemOf(err) to assert category == validation, use errors.As(err, &ve)
with ve as *errs.ValidationError and verify ve.Param contains the unknown flag
name (e.g., "--tos"), and check ve.Hint contains the candidate suggestions;
remove assertions specific to output.ExitError.Detail and ExitCode and replace
them with checks against the fields on errs.ValidationError and its
hint/candidate content returned by flagSuggestErrorFunc.
In `@shortcuts/mail/flag_suggest.go`:
- Around line 86-101: Replace the manual construction of &output.ExitError with
a typed validation error by returning
errs.NewValidationError(errs.SubtypeInvalidArgument,
err.Error()).WithParam(rawUnknownToken(token, isShorthand)).WithHint(hint or
strings.Join(matches, ", ")) so the unknown flag is provided as the param and
the candidate suggestions are attached as the hint; remove the output.ExitError
creation and any use of output.ExitAPI in this branch and ensure you import/use
errs.NewValidationError, errs.SubtypeInvalidArgument, WithParam and WithHint
(use the existing variables token, isShorthand, matches and hint).
In `@shortcuts/mail/mail_draft_create.go`:
- Line 94: Replace the plain output.ErrValidation return with a properly typed
validation error: use errs.NewValidationError(errs.SubtypeInvalidArgument,
"<message>").WithParam("<flag>") so the error carries validation metadata;
specifically update the subject check return in mail_draft_create.go (the return
currently using output.ErrValidation for the "--subject" case) to use
errs.NewValidationError(...).WithParam("--subject") and make the same
replacement for the other occurrences noted (the validation returns at the spots
corresponding to the lines referenced: the blocks handling required flags/args
around the checks at ~lines 179, 182, 288, and 368) to ensure all flag/arg
validation failures use errs.NewValidationError with the correct Subtype and
WithParam.
- Line 195: Replace bare fmt.Errorf returns in mail_draft_create.go with typed
errs errors: for the draft creation failure (the return at the "create draft
failed" site) replace fmt.Errorf("create draft failed: %w", err) with
errs.NewInternalError(errs.SubtypeUnknown, "create draft
failed").WithCause(err); for the sender-email determination error (the check
that should produce a validation error) return
errs.NewValidationError(errs.SubtypeInvalidInput, "invalid sender
email").WithCause(err) (or similar validation subtype) instead of fmt.Errorf;
and for the EML build failure (the return in the EML construction block) use
errs.NewInternalError(errs.SubtypeUnknown, "build EML failed").WithCause(err).
Ensure you update the specific return sites (the draft creation, sender-email
check, and EML build) to use these typed errs helpers and preserve the original
error as the cause.
In `@shortcuts/mail/mail_draft_edit.go`:
- Around line 493-508: The loadPatchFile function uses bare fmt.Errorf for file
open/read/parse failures and returns patch.Validate() directly; replace those
bare errors with errs.NewInternalError(errs.SubtypeFileIO, ...) that include the
operation and path and wrap the underlying err for the Open/Read/Unmarshal
failures (references: loadPatchFile, the Open call, io.ReadAll call,
json.Unmarshal call), and do not return patch.Validate() directly—call
patch.Validate(), and if it returns an error wrap that in an appropriate
user/input error (e.g., errs.NewInvalidInputError with a message like "invalid
patch file") and return that, otherwise return patch, nil (see same handling
pattern used in buildDraftEditPatch).
In `@shortcuts/mail/mail_draft_send_test.go`:
- Around line 228-237: Replace the direct *output.ExitError assertions in the
listed tests (TestMailDraftSend_PartialFailure, TestMailDraftSend_FatalAborts,
TestMailDraftSend_FatalAfterSuccessEmitsLedger,
TestMailDraftSend_AutomationDisabled,
TestMailDraftSend_AutomationDisabledAfterSuccessEmitsLedger,
TestMailDraftSend_MissingYes) with structured error checks using errs.ProblemOf
to assert the error category/subtype/params and verify the original cause is
preserved; in practice, for each occurrence that currently does errors.As(err,
&exitErr) and checks exitErr.Code/Detail, call errs.ProblemOf(err,
"<expected_category>", "<expected_subtype>", map[string]string{...}) to validate
metadata, and additionally ensure the cause contains the underlying
*output.ExitError (e.g., errors.As(err, &exitErr) or errs.Cause(err) pattern) so
the exit code is preserved — update assertions to use these symbols
(errs.ProblemOf, errs.Cause or errors.As for *output.ExitError) instead of
direct field comparisons on exitErr.Detail or Code.
In `@shortcuts/mail/mail_forward.go`:
- Around line 138-141: Replace fmt.Errorf wraps with the project's typed error
constructors: do not re-wrap lower-layer typed errors—propagate them unchanged
(e.g., if err already is a typed errs.* just return it), and for unclassified
errors use errs.NewInternalError(errs.SubtypeUnknown, "failed to fetch original
message").WithCause(err). For validation failures (e.g., the
validateForwardAttachmentURLs return and the specific case noted for "--inline")
return errs.NewValidationError(errs.SubtypeInvalidArgument, "invalid --inline
parameter").WithParam("--inline") instead of fmt.Errorf; update similar returns
referenced (validateForwardAttachmentURLs, fetch original message and the other
listed spots) to either pass through typed errs or construct the appropriate
errs.NewInternalError/errs.NewValidationError with WithCause/WithParam as
applicable.
- Around line 384-390: Replace the legacy output.ErrValidation usage in the
attachment checks with typed errs validation errors: where you currently return
output.ErrValidation("attachment %s (%.1f GB) exceeds the %.0f GB single file
limit", ...) for each file (references: f.FileName, f.Size,
MaxLargeAttachmentSize), return
errs.NewValidationError(errs.SubtypeInvalidArgument,
"...").WithParam("--attach") with the same formatted message and args; likewise
replace the total count check (totalCount computed from origAtts, largeAttIDs,
userFiles and compared against MaxAttachmentCount) to return
errs.NewValidationError(errs.SubtypeInvalidArgument, "attachment count %d
exceeds the limit of %d", totalCount, MaxAttachmentCount).WithParam("--attach").
- Around line 416-426: Replace the output.ErrValidation(...) calls in the
large-attachments validation block with errs.NewValidationError so the
command-facing validation follows guidelines: locate the section checking
runtime.Config and runtime.UserOpenId() and the earlier check that returns
output.ErrValidation("large attachments require a body...") (around the handling
of classified.Oversized and totalBytes calculation), and change both to return
errs.NewValidationError with an appropriate subtype and parameter annotation
(e.g., subtype "attachments" or similar and parameter "attachments" or "body" as
applicable), preserving the original human-readable message and the formatted
size value.
In `@shortcuts/mail/mail_lint_html.go`:
- Line 144: Replace the defensive/unreachable use of output.ErrValidation(...)
with the internal error helper: change the return from return "",
output.ErrValidation("internal: --body-file empty after Validate") to return "",
output.ErrInternal("internal: --body-file empty after Validate") so the code
uses the correct internal-error classification (adjust to ErrInternalf if
formatted message helper is preferred).
- Around line 57-68: Replace the output.ErrValidation(...) returns with typed
validation errors using errs.NewValidationError(errs.SubtypeInvalidArgument,
<message>).WithParam("<flag>"); specifically: in the empty-check that currently
returns output.ErrValidation("exactly one of --body or --body-file is required")
replace it with errs.NewValidationError(errs.SubtypeInvalidArgument, "exactly
one of --body or --body-file is required").WithParam("--body"); in the
mutual-exclusive check that returns output.ErrValidation("--body and --body-file
are mutually exclusive; pass exactly one") replace it with
errs.NewValidationError(errs.SubtypeInvalidArgument, "--body and --body-file are
mutually exclusive; pass exactly one").WithParam("--body"); and in the
runtime.ValidatePath(bodyFile) error branch replace
output.ErrValidation("--body-file: %v", err) with
errs.NewValidationError(errs.SubtypeInvalidArgument, fmt.Sprintf("--body-file:
%v", err)).WithParam("--body-file"). Ensure you import/alias errs and fmt if not
already present and update the return expressions accordingly in the same
function where bodyFile and runtime.ValidatePath are used.
In `@shortcuts/mail/mail_message.go`:
- Line 51: The return that currently uses fmt.Errorf("failed to fetch email:
%w", err) downgrades lower-layer typed errors; update the logic where that
return occurs (the error return in mail_message.go that wraps the fetch error)
to first check for a typed Problem error with if _, ok := errs.ProblemOf(err);
ok { return err } and only if not a typed Problem wrap/annotate the error (e.g.,
return fmt.Errorf("failed to fetch email: %w", err)). Ensure you reference the
same error variable `err` and replace the direct wrap at the location of the
existing fmt.Errorf call.
In `@shortcuts/mail/mail_reply.go`:
- Line 140: Replace the bare fmt.Errorf(...) wrappers at the indicated failure
sites in mail_reply.go with typed errs.* errors per guidelines: for the "failed
to fetch original message" return use errs.NewInternalError(errs.SubtypeUnknown,
"failed to fetch original message").WithCause(err); for the HTML mode
enforcement use errs.NewValidationError(errs.SubtypeInvalidArgument,
"...appropriate message..."); for EML build and draft creation failures replace
fmt.Errorf wraps with errs.NewInternalError(...).WithCause(err); for the send
failure wrap the underlying error with
errs.NewInternalError(...).WithCause(err); and for the value returned from
draftpkg.ResolveLocalImagePaths (currently returned as resolveErr) either ensure
ResolveLocalImagePaths returns a typed errs.* error or wrap resolveErr with an
appropriate errs.NewInternalError(...).WithCause(resolveErr). Reference these
exact symbols: errs.NewInternalError, errs.NewValidationError,
errs.SubtypeUnknown, errs.SubtypeInvalidArgument, .WithCause, and
draftpkg.ResolveLocalImagePaths when making the changes.
In `@shortcuts/mail/mail_template_update.go`:
- Around line 174-184: The error returns for file open/read/parse in the
patch-file handling (variables pf, f, buf, readErr, and json.Unmarshal into
templatePatchFile) use output.ErrValidation but should follow guidelines:
replace file I/O failures (open and io.ReadAll) with
errs.NewInternalError(errs.SubtypeFileIO, ...) including the original err, and
replace user-/flag-related validation failures (JSON parse, size or argument
checks) with errs.NewValidationError(errs.SubtypeInvalidArgument,
...).WithParam("--patch-file"); update the return sites around open
--patch-file, read --patch-file, and parse --patch-file accordingly, and apply
the same changes to the other similar blocks referenced (the blocks at the other
locations you noted: 201-203 and 315-320).
- Around line 96-102: Replace the current output.ErrValidation(...) calls in
mail_template_update.go with typed validation errors using
errs.NewValidationError(errs.SubtypeInvalidArgument, ...).WithParam(...): for
the missing template id case (where runtime.Str("template-id") is checked)
return errs.NewValidationError(errs.SubtypeInvalidArgument, "--template-id is
required (or use --print-patch-template to print the patch
skeleton)").WithParam("--template-id"); for the mutually exclusive content flags
(where runtime.Str("set-template-content") and
runtime.Str("set-template-content-file") are both non-empty) return
errs.NewValidationError(errs.SubtypeInvalidArgument, "--set-template-content and
--set-template-content-file are mutually
exclusive").WithParam("--set-template-content, --set-template-content-file");
and for the name length check (where name := runtime.Str("set-name")) return
errs.NewValidationError(errs.SubtypeInvalidArgument, "--set-name must be at most
100 characters").WithParam("--set-name"). Ensure you replace the exact return
sites that currently call output.ErrValidation(...) so callers get the typed
validation errors.
- Line 281: The current return statement uses wrapping ("return
fmt.Errorf(\"update template failed: %w\", err)") which can strip typed
classification; change it to preserve the original typed error by checking with
errors.As and returning the original typed error when it matches (e.g., var te
*MyTypedError; if errors.As(err, &te) { return te } ), and only wrap with
fmt.Errorf("%s: %w", "update template failed", err) for non-typed errors — keep
the exact message but ensure typed errors are returned unwrapped so callers can
use errors.Is/errors.As.
In `@shortcuts/mail/mail_thread.go`:
- Around line 91-93: Replace the raw runtime.CallAPI call and fmt.Errorf
wrapping with the typed shortcut: call runtime.CallAPITyped (for the same
mailboxPath(mailboxID, "threads", threadID) and params) so the Lark API error is
returned as a typed error, and do not re-wrap it — pass the error through
unchanged (i.e., return err) instead of using fmt.Errorf; ensure the code uses
the same result variable (listData) or the appropriate typed result from
CallAPITyped.
---
Outside diff comments:
In `@shortcuts/mail/draft/service.go`:
- Around line 34-50: GetRaw (and similarly CreateWithRaw, UpdateWithRaw, Send)
replaced typed API/error helpers with untyped calls and plain fmt errors; revert
to using runtime.CallAPITyped (or errclass.BuildAPIError for raw responses)
instead of runtime.CallAPI, and replace bare fmt.Errorf constructions with the
repository's typed error builder (e.g., errs.NewInternalError(...,
errs.SubtypeInvalidResponse, ...) and attach the underlying cause via
.WithCause). Locate CallAPI usages in GetRaw/CreateWithRaw/UpdateWithRaw/Send
and swap to CallAPITyped or BuildAPIError as appropriate, and change the
empty-raw or missing-ID error creation (where extractRawEML or extractDraftID
are validated) to construct the typed errs.NewInternalError with
errs.SubtypeInvalidResponse and include the original response or error as the
cause.
- Around line 34-102: The PR replaced typed errors and typed API calls with bare
fmt.Errorf/output.ErrValidation and runtime.CallAPI; revert these in the mail
shortcuts: use runtime.CallAPITyped instead of CallAPI in the functions shown
(the draft fetch/write/send helpers including the GetDraftRaw flow,
CreateWithRaw, UpdateWithRaw, and Send) and replace bare errors with the
repository's typed errors (e.g., wrap lower-layer failures with
errs.NewInternalError(errs.SubtypeUnknown, "...").WithCause(err) for API/IO
failures and use errs.NewValidationError(errs.SubtypeInvalidArgument,
"...").WithParam("--flag") for user/flag validation), ensuring
extractDraftID/CallAPITyped error paths return properly typed errs.* values
rather than fmt.Errorf or output.ErrValidation.
In `@shortcuts/mail/mail_draft_edit.go`:
- Around line 91-218: The Execute path in mail_draft_edit.go downgrades typed
errs.* errors to output.ErrValidation and fmt.Errorf; update each validation
failure (e.g., the "--draft-id is required" check, parse draft raw EML failure,
parseEventTimeRange/set_calendar validation branches, set_calendar/ICS build
failure, serialize draft failed, apply draft patch failed, etc.) to return
errs.NewValidationError(errs.SubtypeInvalidArgument, "<short
message>").WithParam("<paramName>", "<value>") or a suitable validation subtype
and message, and convert execution/internal failures that wrap underlying errors
(e.g., draftpkg.GetRaw, draftpkg.Parse, draftpkg.Apply, draftpkg.Serialize,
draftpkg.UpdateWithRaw and other API/IO errors) to return
errs.NewInternalError(errs.SubtypeUnknown, "<short context
message>").WithCause(err); locate these spots by searching in Execute for calls
to output.ErrValidation and fmt.Errorf and replace them with the appropriate
errs.* constructors and WithCause/WithParam chaining to preserve original error
context.
- Around line 425-486: Replace the uses of output.ErrValidation(...) in
buildDraftEditPatch with typed validation errors using
errs.NewValidationError(errs.SubtypeInvalidArgument,
"<message>").WithParam("<paramName>", "<paramValue>") (e.g., for
mutual-exclusion/required-flag checks around set-event-* and body/patch-file
logic replace output.ErrValidation calls with errs.NewValidationError and attach
a param like "flag" or "combination" to preserve metadata); likewise for the
other validation branches that currently return output.ErrValidation. Also
ensure the final return uses a typed errs error: check patch.Validate() in
draftpkg.Patch.Validate() and if it can return a plain error, wrap it with
errs.NewValidationError (or convert it) before returning so the function always
returns an errs.* typed validation error. Use function name buildDraftEditPatch
and methods patch.Validate()/draftpkg.Patch to locate the spots.
In `@shortcuts/mail/mail_draft_send_test.go`:
- Around line 786-889: Tests are constructing raw *output.ExitError /
output.Errorf values but should use the typed error contract (errs.*) and
isFatalSendErr should inspect via errs.ProblemOf; replace each test case that
builds &output.ExitError{...} or output.Errorf(...) with an errs problem
instance that represents the same condition (use errs.ProblemOf / the specific
errs.* constructors to create auth, app_status, config, permission, rate_limit,
network and API problems) and update isFatalSendErr to determine fatality by
calling errs.ProblemOf(err) and examining the Problem.Type/Code instead of
switching on *output.ExitError or concrete output errors so the tests and
production behavior match the typed-error contract.
In `@shortcuts/mail/mail_send_receipt.go`:
- Around line 116-134: Replace bare fmt.Errorf usages with typed errs.* errors
per guidelines: for the fetch failure in the block that returns
fmt.Errorf("failed to fetch original message: %w", err) wrap the cause using
errs.NewInternalError("failed to fetch original message").WithCause(err); for
validation checks (the branches that return fmt.Errorf about missing read
receipt label and missing sender when hasReadReceiptRequestLabel(msg) or
origFromEmail == "") return
errs.NewValidationError(errs.SubtypeFailedPrecondition, "read receipt not
requested" or "original message has no sender address").WithHint("provide the
read-receipt label" or "specify --from explicitly"); for later EML build / draft
creation / send failures (the returns around buildEML, createDraft, sendMessage)
replace fmt.Errorf(...) with errs.NewInternalError("...").WithCause(err)
preserving context in the message. Update the error messages to include the
messageID or relevant context and keep the original error as the cause.
---
Nitpick comments:
In `@shortcuts/mail/large_attachment.go`:
- Around line 124-128: Several error returns in
shortcuts/mail/large_attachment.go use plain fmt.Errorf but must use the
project’s command-facing typed errors (errs.*) per guidelines; find the plain
fmt.Errorf usages in the large attachment handling functions (e.g., the
stat/upload paths and validation branches inside the functions that perform
attachment processing and preprocessLargeAttachmentsForDraftEdit) and replace
each fmt.Errorf call with the appropriate errs.* constructor or wrapper (e.g.,
errs.New, errs.InvalidArgument, errs.FailedPrecondition, or a wrapping helper
used elsewhere in the repo) so command-facing failures are typed; ensure wrapped
errors still preserve the underlying error (use the errs.* variant that supports
wrapping) and update all occurrences referenced in the review (stat/upload error
paths and the various validation/precondition errors) to follow the typed errs
convention.
In `@shortcuts/mail/mail_shortcut_validation_test.go`:
- Around line 19-28: The test helper assertValidationError should stop accepting
the legacy output.ExitError contract; remove the fallback checks that use
output.ExitCodeOf and output.ExitValidation and simplify the assertions to only
validate the typed error contract using errs.IsValidation(err). Locate the
assertValidationError function and delete the branches/conditions that call
output.ExitCodeOf or compare against output.ExitValidation, then replace them
with a single assertion that errs.IsValidation(err) is true (and keep the
existing nil check and t.Helper call).
In `@shortcuts/mail/mail_watch_test.go`:
- Around line 526-545: TestWrapWatchSubscribeErrorExitError currently asserts on
err.Error() substrings; update it to assert the typed *output.ExitError metadata
instead: use errors.As or a type assertion on the returned error from
wrapWatchSubscribeError to get an *output.ExitError and then assert
ExitError.Code == output.ExitAPI and ExitError.Detail.Type == "api_error" and
that ExitError.Detail.Message (and optionally Detail.Hint) contain the expected
text; reference the test name TestWrapWatchSubscribeErrorExitError and the
production helper wrapWatchSubscribeError to locate the change.
In `@shortcuts/mail/mail_watch.go`:
- Around line 719-726: Replace the json.Unmarshal usage on apiResp.RawBody with
a json.NewDecoder(...).UseNumber() + Decode to preserve numeric precision and
match the pattern used in doJSONAPI in mail_triage.go; specifically, decode into
the existing result map[string]interface{} (the block handling apiResp.RawBody
and extracting result["code"], result["msg"], result["data"]) so numeric fields
are retained as json.Number rather than float64 and the later checks for
code/msg remain unchanged.
🪄 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: a61379a1-0df3-4d86-b9ea-51d67c1418bf
📒 Files selected for processing (51)
shortcuts/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_edit_test.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.goskills/lark-mail/references/lark-mail-triage.mdtests/cli_e2e/mail/mail_triage_dryrun_test.go
💤 Files with no reviewable changes (13)
- shortcuts/mail/draft/limits.go
- shortcuts/mail/draft/patch.go
- shortcuts/mail/draft/patch_calendar.go
- tests/cli_e2e/mail/mail_triage_dryrun_test.go
- shortcuts/mail/draft/large_attachment_parse.go
- shortcuts/mail/draft/serialize.go
- shortcuts/mail/draft/parse.go
- shortcuts/mail/signature_compose_test.go
- shortcuts/mail/draft/model.go
- shortcuts/mail/mail_errors_test.go
- shortcuts/mail/mail_errors.go
- shortcuts/mail/mail_draft_edit_test.go
- shortcuts/mail/mail_triage_test.go
👮 Files not reviewed due to content moderation or server errors (6)
- shortcuts/mail/draft/charset.go
- shortcuts/mail/emlbuilder/builder.go
- shortcuts/mail/filecheck/filecheck.go
- shortcuts/register_test.go
- shortcuts/mail/helpers.go
- shortcuts/mail/helpers_test.go
| mailInvalidParam("--body", "mutually exclusive with --body-file"), | ||
| mailInvalidParam("--body-file", "mutually exclusive with --body"), | ||
| ) | ||
| return output.ErrValidation("--body and --body-file are mutually exclusive; pass exactly one") |
There was a problem hiding this comment.
Migration contradicts coding guidelines for command-facing error handling.
The coding guidelines explicitly require typed errs.* errors for command-facing failures and specifically state "never use legacy output.Err* helpers". The guidelines direct: "Use errs.NewValidationError(errs.SubtypeInvalidArgument, ...).WithParam("--flag") for user flag/arg validation failures".
This file migrates six validation sites to output.ErrValidation:
- Line 54:
--body/--body-filemutual exclusion - Line 58:
--body-filepath safety - Line 82: required resolved body
- Lines 98, 103, 106: body-file I/O and size limits
All six should use errs.NewValidationError with .WithParam() to match the typed-error contract required by the guidelines.
Also applies to: 58-58, 82-82, 98-98, 103-103, 106-106
🤖 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/body_file.go` at line 54, Replace uses of output.ErrValidation
in shortcuts/mail/body_file.go with typed errs.Validation errors per guidelines:
for the mutual-exclusion check (the return that now says "--body and --body-file
are mutually exclusive; pass exactly one") return
errs.NewValidationError(errs.SubtypeInvalidArgument, "...").WithParam("--body")
or include both flags as context; for the body-file path safety check replace
output.ErrValidation with errs.NewValidationError(errs.SubtypeInvalidArgument,
"<message>").WithParam("--body-file"); for the "required resolved body"
validation replace output.ErrValidation with
errs.NewValidationError(errs.SubtypeInvalidArgument,
"<message>").WithParam("--body"); and for the body-file I/O and size limit
failures (the three sites that currently return output.ErrValidation) return
errs.NewValidationError(errs.SubtypeInvalidArgument, "<io/size
message>").WithParam("--body-file"). Locate these replacements at the existing
return sites that reference output.ErrValidation in the file and keep the
original messages while using errs.NewValidationError(...).WithParam(...) to
attach the relevant flag name.
Source: Coding guidelines
Comment on lines
+178
to
207
| func TestFlagSuggestErrorFunc_LongUnknown_ReturnsExitError(t *testing.T) { | ||
| cmd := newFakeMailCmd() | ||
| got := flagSuggestErrorFunc(cmd, errors.New("unknown flag: --tos")) | ||
|
|
||
| validationErr := requireFlagSuggestValidation(t, got) | ||
| assert.Equal(t, "unknown flag: --tos", validationErr.Message) | ||
| assert.Equal(t, "--tos", validationErr.Param) | ||
| assert.Contains(t, validationErr.Hint, "--to") | ||
|
|
||
| reason, ok := paramReason(validationErr.Params, "--tos") | ||
| require.True(t, ok, "unknown flag should be included in params") | ||
| assert.Equal(t, "unknown flag", reason) | ||
| reason, ok = paramReason(validationErr.Params, "--to") | ||
| require.True(t, ok, "expected --to in candidate params") | ||
| assert.Contains(t, reason, "candidate (prefix") | ||
| var exitErr *output.ExitError | ||
| require.True(t, errors.As(got, &exitErr), "expected *output.ExitError, got %T", got) | ||
| require.NotNil(t, exitErr.Detail) | ||
| assert.Equal(t, "unknown_flag", exitErr.Detail.Type) | ||
| assert.Equal(t, "unknown flag: --tos", exitErr.Detail.Message) | ||
| assert.Contains(t, exitErr.Detail.Hint, "--to") | ||
|
|
||
| detail, ok := exitErr.Detail.Detail.(map[string]any) | ||
| require.True(t, ok, "Detail.Detail should be map[string]any") | ||
| assert.Equal(t, "--tos", detail["unknown"]) | ||
| assert.Equal(t, cmd.CommandPath(), detail["command_path"]) | ||
|
|
||
| cands, ok := detail["candidates"].([]Candidate) | ||
| require.True(t, ok, "candidates should be []Candidate") | ||
| require.NotEmpty(t, cands) | ||
|
|
||
| var foundTo bool | ||
| for _, c := range cands { | ||
| if c.Flag == "--to" { | ||
| foundTo = true | ||
| assert.Equal(t, "prefix", c.Reason) | ||
| break | ||
| } | ||
| } | ||
| assert.True(t, foundTo, "expected --to in candidates") | ||
| } |
There was a problem hiding this comment.
Test assertions reflect implementation's guidelines violation.
These test updates assert *output.ExitError with Detail.Type = "unknown_flag" and ExitCode = output.ExitAPI. Per coding guidelines, unknown-flag errors should be typed errs.ValidationError with the validation category and appropriate subtype.
Once the implementation is corrected to use errs.NewValidationError, update these tests to:
- Assert
errs.ProblemOf(err)returns category=validation - Use
errors.As(err, &ve)whereveis*errs.ValidationErrorto check.Paramcontains the unknown flag name - Verify
.Hintcontains candidate suggestions
Also applies to: 217-225, 239-242, 252-271, 273-287, 293-296
🤖 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/flag_suggest_test.go` around lines 178 - 207, Update the tests
that currently expect an *output.ExitError from flagSuggestErrorFunc (e.g.,
TestFlagSuggestErrorFunc_LongUnknown_ReturnsExitError and the other affected
tests) to instead assert the error is a validation problem: use
errs.ProblemOf(err) to assert category == validation, use errors.As(err, &ve)
with ve as *errs.ValidationError and verify ve.Param contains the unknown flag
name (e.g., "--tos"), and check ve.Hint contains the candidate suggestions;
remove assertions specific to output.ExitError.Detail and ExitCode and replace
them with checks against the fields on errs.ValidationError and its
hint/candidate content returned by flagSuggestErrorFunc.
Source: Coding guidelines
Comment on lines
+86
to
101
| detail := map[string]any{ | ||
| "unknown": rawUnknownToken(token, isShorthand), | ||
| "command_path": c.CommandPath(), | ||
| "candidates": matches, | ||
| } | ||
| // Code is ExitAPI (=1), matching cobra's default unknown-flag exit | ||
| // code. The structured type discrimination lives in error.type. | ||
| return &output.ExitError{ | ||
| Code: output.ExitAPI, | ||
| Detail: &output.ErrDetail{ | ||
| Type: "unknown_flag", | ||
| Message: err.Error(), | ||
| Hint: hint, | ||
| Detail: detail, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Unknown-flag handling violates typed error guidelines.
Per coding guidelines: "Use errs.NewValidationError(errs.SubtypeInvalidArgument, ...).WithParam("--flag") for user flag/arg validation failures". Unknown flags are validation failures (invalid user input) and should return typed errs.ValidationError, not *output.ExitError.
The current implementation:
- Constructs
output.ExitErrormanually (lines 93-101) - Uses
output.ExitAPI(exit code 1) instead of the validation-error exit code - Bypasses the typed error metadata (category, subtype, param) required by guidelines
Refactor to use errs.NewValidationError with the unknown flag name in .WithParam() and the candidate suggestions in .WithHint().
🤖 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/flag_suggest.go` around lines 86 - 101, Replace the manual
construction of &output.ExitError with a typed validation error by returning
errs.NewValidationError(errs.SubtypeInvalidArgument,
err.Error()).WithParam(rawUnknownToken(token, isShorthand)).WithHint(hint or
strings.Join(matches, ", ")) so the unknown flag is provided as the param and
the candidate suggestions are attached as the hint; remove the output.ExitError
creation and any use of output.ExitAPI in this branch and ensure you import/use
errs.NewValidationError, errs.SubtypeInvalidArgument, WithParam and WithHint
(use the existing variables token, isShorthand, matches and hint).
Source: Coding guidelines
| } | ||
| if !hasTemplate && strings.TrimSpace(runtime.Str("subject")) == "" { | ||
| return mailValidationParamError("--subject", "--subject is required; pass the final email subject (or use --template-id)") | ||
| return output.ErrValidation("--subject is required; pass the final email subject (or use --template-id)") |
There was a problem hiding this comment.
Critical: Validation error violates coding guidelines.
output.ErrValidation(...) replaces the required typed error. Per coding guidelines: "Use errs.NewValidationError(errs.SubtypeInvalidArgument, ...).WithParam("--flag") for user flag/arg validation failures." The same issue appears at lines 179, 182, 288, and 368.
The correct form for this --subject validation failure is:
return errs.NewValidationError(errs.SubtypeInvalidArgument,
"--subject is required; pass the final email subject (or use --template-id)").
WithParam("--subject")This preserves the typed metadata that error classification and test assertions depend on.
🤖 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_create.go` at line 94, Replace the plain
output.ErrValidation return with a properly typed validation error: use
errs.NewValidationError(errs.SubtypeInvalidArgument,
"<message>").WithParam("<flag>") so the error carries validation metadata;
specifically update the subject check return in mail_draft_create.go (the return
currently using output.ErrValidation for the "--subject" case) to use
errs.NewValidationError(...).WithParam("--subject") and make the same
replacement for the other occurrences noted (the validation returns at the spots
corresponding to the lines referenced: the blocks handling required flags/args
around the checks at ~lines 179, 182, 288, and 368) to ensure all flag/arg
validation failures use errs.NewValidationError with the correct Subtype and
WithParam.
Source: Coding guidelines
| draftResult, err := draftpkg.CreateWithRaw(runtime, mailboxID, rawEML) | ||
| if err != nil { | ||
| return mailDecorateProblemMessage(err, "create draft failed") | ||
| return fmt.Errorf("create draft failed: %w", err) |
There was a problem hiding this comment.
Critical: Execution errors violate coding guidelines.
Lines 195, 253, and 368 use bare fmt.Errorf for command execution failures. Per coding guidelines: "Command-facing failures must be typed errs.* errors — never use legacy output.Err* helpers or bare fmt.Errorf."
For example, line 195's draft creation failure should use a typed error:
return errs.NewInternalError(errs.SubtypeUnknown,
"create draft failed").WithCause(err)Line 253 (determining sender email) should be errs.NewValidationError, and line 368 (EML build) should be errs.NewInternalError.
Also applies to: 253-253, 368-368
🤖 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_create.go` at line 195, Replace bare fmt.Errorf
returns in mail_draft_create.go with typed errs errors: for the draft creation
failure (the return at the "create draft failed" site) replace
fmt.Errorf("create draft failed: %w", err) with
errs.NewInternalError(errs.SubtypeUnknown, "create draft
failed").WithCause(err); for the sender-email determination error (the check
that should produce a validation error) return
errs.NewValidationError(errs.SubtypeInvalidInput, "invalid sender
email").WithCause(err) (or similar validation subtype) instead of fmt.Errorf;
and for the EML build failure (the return in the EML construction block) use
errs.NewInternalError(errs.SubtypeUnknown, "build EML failed").WithCause(err).
Ensure you update the specific return sites (the draft creation, sender-email
check, and EML build) to use these typed errs helpers and preserve the original
error as the cause.
Source: Coding guidelines
| } | ||
| if !hasTemplate && strings.TrimSpace(runtime.Str("subject")) == "" { | ||
| return mailValidationParamError("--subject", "--subject is required; pass the final email subject (or use --template-id)") | ||
| return output.ErrValidation("--subject is required; pass the final email subject (or use --template-id)") |
There was a problem hiding this comment.
Critical: Validation and execution errors violate coding guidelines.
-
Line 86:
output.ErrValidation(...)for--subjectvalidation. Per coding guidelines: "Useerrs.NewValidationError(errs.SubtypeInvalidArgument, ...).WithParam("--subject")for user flag/arg validation failures." -
Lines 311, 316, 329:
fmt.Errorf(...: %w, err)for EML build, draft creation, and send failures. Per guidelines, these should beerrs.NewInternalError(errs.SubtypeUnknown, ...).WithCause(err). -
Line 244: Returns
resolveErrdirectly—verify upstream returns typed errors.
Also applies to: 244-244, 311-329
Source: Coding guidelines
Comment on lines
+96
to
+102
| return output.ErrValidation("--template-id is required (or use --print-patch-template to print the patch skeleton)") | ||
| } | ||
| if runtime.Str("set-template-content") != "" && runtime.Str("set-template-content-file") != "" { | ||
| return mailValidationError("--set-template-content and --set-template-content-file are mutually exclusive"). | ||
| WithParams( | ||
| mailInvalidParam("--set-template-content", "mutually exclusive with --set-template-content-file"), | ||
| mailInvalidParam("--set-template-content-file", "mutually exclusive with --set-template-content"), | ||
| ) | ||
| return output.ErrValidation("--set-template-content and --set-template-content-file are mutually exclusive") | ||
| } | ||
| if name := runtime.Str("set-name"); name != "" && len([]rune(name)) > 100 { | ||
| return mailValidationParamError("--set-name", "--set-name must be at most 100 characters") | ||
| return output.ErrValidation("--set-name must be at most 100 characters") |
There was a problem hiding this comment.
Critical: Validate callback violates error guidelines.
Lines 96-102 use output.ErrValidation(...) for flag validation. Per coding guidelines:
Use
errs.NewValidationError(errs.SubtypeInvalidArgument, ...).WithParam("--flag")for user flag/arg validation failures
Replace with typed errors:
-return output.ErrValidation("--template-id is required (or use --print-patch-template to print the patch skeleton)")
+return errs.NewValidationError(errs.SubtypeInvalidArgument,
+ "--template-id is required (or use --print-patch-template to print the patch skeleton)").
+ WithParam("--template-id")
-return output.ErrValidation("--set-template-content and --set-template-content-file are mutually exclusive")
+return errs.NewValidationError(errs.SubtypeInvalidArgument,
+ "--set-template-content and --set-template-content-file are mutually exclusive").
+ WithParam("--set-template-content").WithParam("--set-template-content-file")
-return output.ErrValidation("--set-name must be at most 100 characters")
+return errs.NewValidationError(errs.SubtypeInvalidArgument,
+ "--set-name must be at most 100 characters").WithParam("--set-name")📝 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.
Suggested change
| return output.ErrValidation("--template-id is required (or use --print-patch-template to print the patch skeleton)") | |
| } | |
| if runtime.Str("set-template-content") != "" && runtime.Str("set-template-content-file") != "" { | |
| return mailValidationError("--set-template-content and --set-template-content-file are mutually exclusive"). | |
| WithParams( | |
| mailInvalidParam("--set-template-content", "mutually exclusive with --set-template-content-file"), | |
| mailInvalidParam("--set-template-content-file", "mutually exclusive with --set-template-content"), | |
| ) | |
| return output.ErrValidation("--set-template-content and --set-template-content-file are mutually exclusive") | |
| } | |
| if name := runtime.Str("set-name"); name != "" && len([]rune(name)) > 100 { | |
| return mailValidationParamError("--set-name", "--set-name must be at most 100 characters") | |
| return output.ErrValidation("--set-name must be at most 100 characters") | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, | |
| "--template-id is required (or use --print-patch-template to print the patch skeleton)"). | |
| WithParam("--template-id") | |
| } | |
| if runtime.Str("set-template-content") != "" && runtime.Str("set-template-content-file") != "" { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, | |
| "--set-template-content and --set-template-content-file are mutually exclusive"). | |
| WithParam("--set-template-content").WithParam("--set-template-content-file") | |
| } | |
| if name := runtime.Str("set-name"); name != "" && len([]rune(name)) > 100 { | |
| return errs.NewValidationError(errs.SubtypeInvalidArgument, | |
| "--set-name must be at most 100 characters").WithParam("--set-name") |
🤖 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_template_update.go` around lines 96 - 102, Replace the
current output.ErrValidation(...) calls in mail_template_update.go with typed
validation errors using errs.NewValidationError(errs.SubtypeInvalidArgument,
...).WithParam(...): for the missing template id case (where
runtime.Str("template-id") is checked) return
errs.NewValidationError(errs.SubtypeInvalidArgument, "--template-id is required
(or use --print-patch-template to print the patch
skeleton)").WithParam("--template-id"); for the mutually exclusive content flags
(where runtime.Str("set-template-content") and
runtime.Str("set-template-content-file") are both non-empty) return
errs.NewValidationError(errs.SubtypeInvalidArgument, "--set-template-content and
--set-template-content-file are mutually
exclusive").WithParam("--set-template-content, --set-template-content-file");
and for the name length check (where name := runtime.Str("set-name")) return
errs.NewValidationError(errs.SubtypeInvalidArgument, "--set-name must be at most
100 characters").WithParam("--set-name"). Ensure you replace the exact return
sites that currently call output.ErrValidation(...) so callers get the typed
validation errors.
Source: Coding guidelines
Comment on lines
+174
to
184
| return output.ErrValidation("open --patch-file %s: %v", pf, err) | ||
| } | ||
| buf, readErr := io.ReadAll(f) | ||
| f.Close() | ||
| if readErr != nil { | ||
| return mailValidationParamError("--patch-file", "read --patch-file %s: %v", pf, readErr).WithCause(readErr) | ||
| return output.ErrValidation("read --patch-file %s: %v", pf, readErr) | ||
| } | ||
| var patch templatePatchFile | ||
| if err := json.Unmarshal(buf, &patch); err != nil { | ||
| return mailValidationParamError("--patch-file", "parse --patch-file %s: %v", pf, err).WithCause(err) | ||
| return output.ErrValidation("parse --patch-file %s: %v", pf, err) | ||
| } |
There was a problem hiding this comment.
Critical: File I/O and size validation errors violate guidelines.
Multiple error returns use output.ErrValidation(...) for I/O and validation failures. Per coding guidelines:
Use
errs.NewInternalError(errs.SubtypeFileIO, ...)for local file I/O failures after path validation
Use
errs.NewValidationError(errs.SubtypeInvalidArgument, ...).WithParam("--flag")for user flag/arg validation failures
Example fixes:
-return output.ErrValidation("open --patch-file %s: %v", pf, err)
+return errs.NewInternalError(errs.SubtypeFileIO, "open --patch-file %s: %v", pf, err).WithParam("--patch-file")
-return output.ErrValidation("template content exceeds %d MB (got %.1f MB)", ...)
+return errs.NewValidationError(errs.SubtypeInvalidArgument,
+ "template content exceeds %d MB (got %.1f MB)", ...).WithParam("--set-template-content")Also applies to: 201-203, 315-320
🤖 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_template_update.go` around lines 174 - 184, The error
returns for file open/read/parse in the patch-file handling (variables pf, f,
buf, readErr, and json.Unmarshal into templatePatchFile) use
output.ErrValidation but should follow guidelines: replace file I/O failures
(open and io.ReadAll) with errs.NewInternalError(errs.SubtypeFileIO, ...)
including the original err, and replace user-/flag-related validation failures
(JSON parse, size or argument checks) with
errs.NewValidationError(errs.SubtypeInvalidArgument,
...).WithParam("--patch-file"); update the return sites around open
--patch-file, read --patch-file, and parse --patch-file accordingly, and apply
the same changes to the other similar blocks referenced (the blocks at the other
locations you noted: 201-203 and 315-320).
Source: Coding guidelines
| resp, err := updateTemplate(runtime, mailboxID, tid, tpl) | ||
| if err != nil { | ||
| return mailDecorateProblemMessage(err, "update template failed") | ||
| return fmt.Errorf("update template failed: %w", err) |
There was a problem hiding this comment.
Critical: Error wrapping downgrades typed classification.
Line 281 wraps the error with fmt.Errorf instead of preserving typed error classification:
-return fmt.Errorf("update template failed: %w", err)
+if _, ok := errs.ProblemOf(err); ok { return err }
+return errs.NewInternalError(errs.SubtypeUnknown, "update template failed").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.
Suggested change
| return fmt.Errorf("update template failed: %w", err) | |
| if _, ok := errs.ProblemOf(err); ok { return err } | |
| return errs.NewInternalError(errs.SubtypeUnknown, "update template failed").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/mail/mail_template_update.go` at line 281, The current return
statement uses wrapping ("return fmt.Errorf(\"update template failed: %w\",
err)") which can strip typed classification; change it to preserve the original
typed error by checking with errors.As and returning the original typed error
when it matches (e.g., var te *MyTypedError; if errors.As(err, &te) { return te
} ), and only wrap with fmt.Errorf("%s: %w", "update template failed", err) for
non-typed errors — keep the exact message but ensure typed errors are returned
unwrapped so callers can use errors.Is/errors.As.
Source: Coding guidelines
Comment on lines
+91
to
+93
| listData, err := runtime.CallAPI("GET", mailboxPath(mailboxID, "threads", threadID), params, nil) | ||
| if err != nil { | ||
| return mailDecorateProblemMessage(err, "failed to get thread") | ||
| return fmt.Errorf("failed to get thread: %w", err) |
There was a problem hiding this comment.
Critical: API call and error handling violate guidelines.
Lines 91-93 violate two coding guidelines:
-
Line 91 uses
runtime.CallAPIinstead ofruntime.CallAPITyped:Use
runtime.CallAPITyped(shortcuts) orerrclass.BuildAPIError(raw responses) for Lark API errors — never hand-build error envelopes -
Line 93 wraps the error with
fmt.Errorfinstead of preserving typed classification:Pass through lower-layer typed errors unchanged — re-wrapping downgrades their classification
Replace with:
-listData, err := runtime.CallAPI("GET", mailboxPath(mailboxID, "threads", threadID), params, nil)
-if err != nil {
- return fmt.Errorf("failed to get thread: %w", err)
-}
+var listData map[string]interface{}
+if err := runtime.CallAPITyped("GET", mailboxPath(mailboxID, "threads", threadID), params, nil, &listData); err != nil {
+ if _, ok := errs.ProblemOf(err); ok { return err }
+ return errs.NewInternalError(errs.SubtypeUnknown, "failed to get thread").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.
Suggested change
| listData, err := runtime.CallAPI("GET", mailboxPath(mailboxID, "threads", threadID), params, nil) | |
| if err != nil { | |
| return mailDecorateProblemMessage(err, "failed to get thread") | |
| return fmt.Errorf("failed to get thread: %w", err) | |
| var listData map[string]interface{} | |
| if err := runtime.CallAPITyped("GET", mailboxPath(mailboxID, "threads", threadID), params, nil, &listData); err != nil { | |
| if _, ok := errs.ProblemOf(err); ok { return err } | |
| return errs.NewInternalError(errs.SubtypeUnknown, "failed to get thread").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/mail/mail_thread.go` around lines 91 - 93, Replace the raw
runtime.CallAPI call and fmt.Errorf wrapping with the typed shortcut: call
runtime.CallAPITyped (for the same mailboxPath(mailboxID, "threads", threadID)
and params) so the Lark API error is returned as a typed error, and do not
re-wrap it — pass the error through unchanged (i.e., return err) instead of
using fmt.Errorf; ensure the code uses the same result variable (listData) or
the appropriate typed result from CallAPITyped.
Source: Coding guidelines
Collaborator
Author
|
Closing because this PR migrated a broader mail directory diff than intended. I will reopen a narrower PR with only the source MR diff. |
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
1 participant
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
Scope
This PR intentionally excludes the internal harness/eval files and only migrates mail-related source, tests, and skill docs that already exist in the public repository.
Tests
go test ./shortcuts/mail/... ./shortcutsSummary by CodeRabbit
Bug Fixes
Chores