feat: add mail sender allow/block list shortcuts#1599
Conversation
Add user-level mail sender allow/block shortcuts for listing, querying, setting, and deleting entries. The commands mirror the published user_mailbox allow_sender and blocked_sender APIs and surface retry guidance for warming search caches. sprint: S7
sprint: S7
sprint: S7
📝 WalkthroughWalkthroughAdded four mail sender allow/block shortcuts, wired them into the mail shortcut registry, and updated the related mail docs and references. The implementation covers list, query, set, and delete flows, with validation, pagination, dry-run behavior, structured outputs, and tests. ChangesMail sender allow/block shortcuts
Sequence Diagram(s)sequenceDiagram
participant MailSenderList
participant MailSenderQuery
participant "sender list endpoint"
participant outputSenderList
MailSenderList->>sender list endpoint: GET allow/block pages
sender list endpoint-->>MailSenderList: items, total, has_more, next_page_token
MailSenderList->>outputSenderList: print table
MailSenderQuery->>sender list endpoint: GET keyword search
sender list endpoint-->>MailSenderQuery: matching items
MailSenderQuery->>outputSenderList: print filtered results
sequenceDiagram
participant MailSenderSet
participant MailSenderDelete
participant "net/mail"
participant "batch_create endpoint"
participant "batch_remove endpoint"
MailSenderSet->>net/mail: parse and validate addresses
MailSenderSet->>batch_create endpoint: POST batch_create
batch_create endpoint-->>MailSenderSet: failed_items
MailSenderDelete->>net/mail: parse and validate addresses
MailSenderDelete->>batch_remove endpoint: POST batch_remove
batch_remove endpoint-->>MailSenderDelete: deleted_count
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
domain/mail
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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_sender_allow_block_test.go`:
- Around line 16-31: The helper assertValidationError only checks
output.ExitError and text, so it can miss regressions in typed metadata and
cause preservation. Update the mail shortcut error-path tests to assert
errs.ProblemOf(err) for category/subtype fields, use errors.As to verify any
Param on *errs.ValidationError separately, and keep a check that the 456 path
still unwraps to the original cause.
In `@shortcuts/mail/mail_sender_allow_block.go`:
- Around line 436-445: The error handling in decorateSenderAPIError is
rebuilding lower-layer failures into a new generic output error, which loses the
original API classification and cause. Update this helper to pass through
already-typed errors from runtime.DoAPIJSON unchanged, switch callers in
shortcuts/mail/mail_sender_allow_block.go to use runtime.CallAPITyped for Lark
API failures, and when enriching the 456 case preserve the original error chain
by attaching the cause with .WithCause(err) instead of creating a fresh
replacement error.
- Around line 333-338: `--type all` pagination is only storing one cursor, but
`senderReadQuery`/the `NextPageToken` handling in `mail_sender_allow_block.go`
needs to support separate cursors for allow and blocked lists. Update the read
path and command surface so `NextPageTokens` are preserved per list and replayed
independently, and make the `senderReadQuery` logic consume the correct token
for each of `allow_senders` and `blocked_senders` instead of applying a single
`--page-token` to both.
- Around line 261-269: `validateBotMailboxNotMe` and `mailValidationParamError`
are using untyped `output.ErrValidation`, which breaks the shortcut error
contract and loses the failing input metadata. Update both helpers to return
typed `errs.ValidationError` values instead, and populate the `param` field with
the relevant user-facing flag/input so callers can identify the invalid
argument. Keep the existing logic in `validateBotMailboxNotMe` and
`mailValidationParamError`, but route the error creation through the typed
`errs` package used by other shortcut validation paths.
🪄 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: 951ae50c-a1c2-4eb9-a902-a5d383ba7933
📒 Files selected for processing (9)
shortcuts/mail/mail_sender_allow_block.goshortcuts/mail/mail_sender_allow_block_test.goshortcuts/mail/shortcuts.goskill-template/domains/mail.mdskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-sender-delete.mdskills/lark-mail/references/lark-mail-sender-list.mdskills/lark-mail/references/lark-mail-sender-query.mdskills/lark-mail/references/lark-mail-sender-set.md
| func assertValidationError(t *testing.T, err error, wantSubstr string) { | ||
| t.Helper() | ||
| if err == nil { | ||
| t.Fatal("expected validation error, got nil") | ||
| } | ||
| var exitErr *output.ExitError | ||
| if !errors.As(err, &exitErr) { | ||
| t.Fatalf("expected output.ExitError, got %T: %v", err, err) | ||
| } | ||
| if exitErr.Code != output.ExitValidation { | ||
| t.Fatalf("exit code = %d, want ExitValidation", exitErr.Code) | ||
| } | ||
| if wantSubstr != "" && !strings.Contains(exitErr.Error(), wantSubstr) { | ||
| t.Fatalf("error = %q, want substring %q", exitErr.Error(), wantSubstr) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Make the new error-path tests assert typed metadata.
These assertions only check output.ExitError and message text, so they will still pass if category, subtype, param, or cause preservation regresses. Once the shortcut returns typed errs.*, assert problem fields via errs.ProblemOf(err), use errors.As(...*errs.ValidationError) for Param, and verify the 456 path still unwraps the original cause.
As per coding guidelines, **/*_test.go: error-path tests must assert typed metadata and cause preservation. Based on learnings, errs.ProblemOf does not expose Param, so that field needs a separate errors.As assertion on *errs.ValidationError.
Also applies to: 294-303
🤖 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_sender_allow_block_test.go` around lines 16 - 31, The
helper assertValidationError only checks output.ExitError and text, so it can
miss regressions in typed metadata and cause preservation. Update the mail
shortcut error-path tests to assert errs.ProblemOf(err) for category/subtype
fields, use errors.As to verify any Param on *errs.ValidationError separately,
and keep a check that the 456 path still unwraps to the original cause.
Sources: Coding guidelines, Learnings
| func validateBotMailboxNotMe(runtime *common.RuntimeContext) error { | ||
| if runtime.IsBot() && strings.EqualFold(strings.TrimSpace(runtime.Str("mailbox")), "me") { | ||
| return output.ErrValidation("bot identity does not support --mailbox me; pass an explicit mailbox email address") | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func mailValidationParamError(flag, format string, args ...interface{}) error { | ||
| return output.ErrValidation("%s: %s", flag, fmt.Sprintf(format, args...)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Switch these validation helpers to typed errs.ValidationErrors.
validateBotMailboxNotMe and mailValidationParamError currently return output.ErrValidation, which violates the shortcut error contract and drops structured param metadata for the failing flag. That also matches the repo’s errs-typed-only lint failure pattern for changed shortcuts/ code.
Suggested direction
+import "github.com/larksuite/cli/internal/errs"
+
func validateBotMailboxNotMe(runtime *common.RuntimeContext) error {
if runtime.IsBot() && strings.EqualFold(strings.TrimSpace(runtime.Str("mailbox")), "me") {
- return output.ErrValidation("bot identity does not support --mailbox me; pass an explicit mailbox email address")
+ return errs.NewValidationError(
+ errs.SubtypeInvalidArgument,
+ "bot identity does not support --mailbox me",
+ ).WithParam("--mailbox").WithHint("pass an explicit mailbox email address")
}
return nil
}
func mailValidationParamError(flag, format string, args ...interface{}) error {
- return output.ErrValidation("%s: %s", flag, fmt.Sprintf(format, args...))
+ return errs.NewValidationError(
+ errs.SubtypeInvalidArgument,
+ fmt.Sprintf(format, args...),
+ ).WithParam(flag)
}As per coding guidelines, **/*.go: command-facing failures must use typed errs.* errors and the param field should name the user input that failed. Based on learnings, changed shortcuts/ code using output.Err* hits the forbidigo [errs-typed-only] rule.
📝 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.
| func validateBotMailboxNotMe(runtime *common.RuntimeContext) error { | |
| if runtime.IsBot() && strings.EqualFold(strings.TrimSpace(runtime.Str("mailbox")), "me") { | |
| return output.ErrValidation("bot identity does not support --mailbox me; pass an explicit mailbox email address") | |
| } | |
| return nil | |
| } | |
| func mailValidationParamError(flag, format string, args ...interface{}) error { | |
| return output.ErrValidation("%s: %s", flag, fmt.Sprintf(format, args...)) | |
| func validateBotMailboxNotMe(runtime *common.RuntimeContext) error { | |
| if runtime.IsBot() && strings.EqualFold(strings.TrimSpace(runtime.Str("mailbox")), "me") { | |
| return errs.NewValidationError( | |
| errs.SubtypeInvalidArgument, | |
| "bot identity does not support --mailbox me", | |
| ).WithParam("--mailbox").WithHint("pass an explicit mailbox email address") | |
| } | |
| return nil | |
| } | |
| func mailValidationParamError(flag, format string, args ...interface{}) error { | |
| return errs.NewValidationError( | |
| errs.SubtypeInvalidArgument, | |
| fmt.Sprintf(format, args...), | |
| ).WithParam(flag) | |
| } |
🤖 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_sender_allow_block.go` around lines 261 - 269,
`validateBotMailboxNotMe` and `mailValidationParamError` are using untyped
`output.ErrValidation`, which breaks the shortcut error contract and loses the
failing input metadata. Update both helpers to return typed
`errs.ValidationError` values instead, and populate the `param` field with the
relevant user-facing flag/input so callers can identify the invalid argument.
Keep the existing logic in `validateBotMailboxNotMe` and
`mailValidationParamError`, but route the error creation through the typed
`errs` package used by other shortcut validation paths.
Sources: Coding guidelines, Learnings
| token := strVal(data["page_token"]) | ||
| if token != "" { | ||
| out.NextPageTokens[currentType] = token | ||
| if listType != senderListAll { | ||
| out.NextPageToken = token | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
--type all pagination is not resumable.
The read path returns per-list cursors in next_page_tokens, but the command surface only accepts one --page-token, and senderReadQuery replays that single token to both allow_senders and blocked_senders. As soon as the two lists diverge, page 2 for +sender-list/+sender-query --type all cannot be fetched correctly.
Also applies to: 361-370
🤖 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_sender_allow_block.go` around lines 333 - 338, `--type
all` pagination is only storing one cursor, but `senderReadQuery`/the
`NextPageToken` handling in `mail_sender_allow_block.go` needs to support
separate cursors for allow and blocked lists. Update the read path and command
surface so `NextPageTokens` are preserved per list and replayed independently,
and make the `senderReadQuery` logic consume the correct token for each of
`allow_senders` and `blocked_senders` instead of applying a single
`--page-token` to both.
| func decorateSenderAPIError(err error, action string) error { | ||
| var exitErr *output.ExitError | ||
| if errors.As(err, &exitErr) && exitErr.Detail != nil && exitErr.Detail.Code == 456 { | ||
| withHint := output.ErrWithHint(exitErr.Code, exitErr.Detail.Type, exitErr.Detail.Message, "search cache warming; retry later") | ||
| withHint.Detail.Code = exitErr.Detail.Code | ||
| withHint.Detail.Detail = exitErr.Detail.Detail | ||
| return withHint | ||
| } | ||
| return output.Errorf(output.ExitAPI, "api_error", "%s failed: %v", action, err) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve typed lower-layer API errors instead of rebuilding them here.
This helper rewrites every runtime.DoAPIJSON failure into a fresh output error, so network/auth/API classifications get collapsed to a generic API failure. The 456 branch also drops Unwrap() semantics by returning a new hinted error without the original cause.
As per coding guidelines, shortcuts/**/*.go should use runtime.CallAPITyped for Lark API failures, pass through already-typed lower-layer errors unchanged, and preserve causes with .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_sender_allow_block.go` around lines 436 - 445, The error
handling in decorateSenderAPIError is rebuilding lower-layer failures into a new
generic output error, which loses the original API classification and cause.
Update this helper to pass through already-typed errors from runtime.DoAPIJSON
unchanged, switch callers in shortcuts/mail/mail_sender_allow_block.go to use
runtime.CallAPITyped for Lark API failures, and when enriching the 456 case
preserve the original error chain by attaching the cause with .WithCause(err)
instead of creating a fresh replacement error.
Source: Coding guidelines
infeng
changed the title
1 participant
Add mail sender allow/block list shortcuts to the lark-cli
maildomain: list, search, add, and remove allow-list and block-list sender entries, with combined or filtered views, paging, and exact-match search.Changes
shortcuts/mail/mail_sender_allow_block.go— new shortcuts to list / search / add / remove allow- and block-list sender entries (with tests)shortcuts/mail/shortcuts.go— register the new shortcutsskills/lark-mail/references +skill-template/domains/mail.md— usage docs and examples for the new commandsSummary by CodeRabbit
New Features
Bug Fixes