Add mail allow/block shortcuts#1627
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds three mail shortcuts for listing, adding, and removing user-scoped allow/block sender entries, with validation, request/response handling, tests, and documentation updates. The new shortcuts are also registered in the mail shortcut list. ChangesMail allow/block sender shortcuts
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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: 3
🤖 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_allow_block_test.go`:
- Around line 281-379: The new error-path tests only verify Param or
message/hint text, but they should also lock the typed problem metadata returned
by the shortcut flow. Update TestMailAllowBlockValidation and
TestMailAllowBlockAPIHints to call errs.ProblemOf(err) and assert the expected
category/subtype for each case, while keeping errors.As for
*errs.ValidationError to check Param where applicable. In the mail shortcut test
helpers and assertions around runMountedMailShortcut and API error handling,
make sure the typed error contract is validated directly rather than relying on
substrings alone.
In `@shortcuts/mail/mail_allow_block.go`:
- Around line 389-400: The fallback logic in buildAllowBlockBatchOutput is
treating an explicit 0 from success_count, added_count, or deleted_count as if
the field were missing, which can overreport success. Update the count selection
in buildAllowBlockBatchOutput so it only falls back to requested when none of
those count fields are present in the data map, and preserve an explicit zero
when the API returns it. Use the existing helpers and symbols like
buildAllowBlockBatchOutput, intVal, and extractAllowBlockFailedItems to locate
the logic and adjust the presence check before assigning successCount.
- Around line 289-300: The delete path in executeAllowBlockDelete parses
batch_remove results with buildAllowBlockBatchOutput but never emits warnings
for failed_items, so partial deletes can appear successful. Update this branch
to mirror the set flow by checking the batch output for failed items and calling
emitAllowBlockFailedItemsWarning before returning. Use the existing
executeAllowBlockDelete, buildAllowBlockBatchOutput, and
emitAllowBlockFailedItemsWarning helpers so partial failures are surfaced
consistently.
🪄 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: 166febc5-192f-4724-9912-87ad13187305
📒 Files selected for processing (8)
shortcuts/mail/mail_allow_block.goshortcuts/mail/mail_allow_block_test.goshortcuts/mail/shortcuts.goskill-template/domains/mail.mdskills/lark-mail/SKILL.mdskills/lark-mail/references/lark-mail-allow-block-delete.mdskills/lark-mail/references/lark-mail-allow-block-list.mdskills/lark-mail/references/lark-mail-allow-block-set.md
Change-Type: ci-fix
oOvalm
left a comment
oOvalm
left a comment
There was a problem hiding this comment.
🤖 AI Review | CR 汇总 | 有风险（4 个 P2/P3）
本次新增 4 条评论：scene 契约不生效、部分成功计数、all 分页 cursor、删除大小写去重。
| items = append(items, map[string]interface{}{ | ||
| "sender": address, | ||
| "sender_type": allowBlockSenderType(address), | ||
| "scene": scene, |
There was a problem hiding this comment.
🤖 AI Review | [P2 正确性] --scene web_image 没有后端契约承接
CLI 第 337 行把 scene 放进 items，但本次 registry 的 user_sender_item 只有 sender / sender_type，open-access 的 UserSenderItem 也没有 scene 字段，服务端最终固定用 BLOCK_SENDER 写入。用户执行 +allow-block-set --scene web_image 会看到命令成功，但实际写入的不是网络图片信任来源，后续远程图片放行等依赖 SourceImage 的场景不会按用户预期生效。
修复建议: 如果 API 不支持 scene，就移除 CLI 的 --scene 参数并固定 sender 场景；如果需要支持 web_image，请先在 OpenAPI/registry/open-access 中补齐 scene 字段并映射到 PB 的 BLOCK_WEB_IMAGE。
如有疑问或认为判断不准确，欢迎直接回复讨论。
| if !ok { | ||
| successCount, ok = intValIfPresent(data, "deleted_count") | ||
| } | ||
| if !ok && len(failedItems) == 0 { |
There was a problem hiding this comment.
🤖 AI Review | [P2 正确性] 只返回 failed_items 时部分成功会被计为 0
当前逻辑只在没有任何失败项时才把缺失的 count 回退为 requested。但 registry 的 batch_create 响应只声明了 failed_items，不保证返回 success_count 或 added_count。例如请求 3 个地址、1 个失败时，服务端成功写入 2 个但只返回 failed_items，这里会输出 success_count=0，output.Meta.Count 也显示 0，用户会把部分成功误判为全失败。
修复建议: 当 count 字段不存在时，用 requested - len(failedItems) 作为默认成功数并 clamp 到 0；仍然保留已有逻辑对显式 0 count 的尊重。
如有疑问或认为判断不准确，欢迎直接回复讨论。
| func executeAllowBlockList(ctx context.Context, runtime *common.RuntimeContext) error { | ||
| mailboxID := resolveMailboxID(runtime) | ||
| typ := runtime.Str("type") | ||
| query := allowBlockListQuery(runtime) |
There was a problem hiding this comment.
🤖 AI Review | [P2 正确性] --type all 不能用一个 page_token 同时翻两张列表
第 249 行构造一份 query，随后第 260 和 264 行把同一个 page_token 传给 allow 和 block 两个独立资源；但输出里又分别返回 allow.next_page_token 和 block.next_page_token，说明两个 cursor 独立。用户下一页只能传一个 --page-token，这个 token 会被同时送到另一张表，可能 invalid cursor，或者导致 allow/block 某一侧跳页、重复。
修复建议: 对 --type all 禁止携带 --page-token，提示用户分别分页 allow/block；或新增独立的 allow/block page-token 参数，分别传给对应资源。
如有疑问或认为判断不准确，欢迎直接回复讨论。
| if addr == "" { | ||
| continue | ||
| } | ||
| key := strings.ToLower(addr) |
There was a problem hiding this comment.
🤖 AI Review | [P3 正确性] 删除去重会丢掉大小写不同的历史记录值
normalizeAllowBlockAddresses 第 197-202 行用 strings.ToLower 做去重，删除路径也复用了它。用户级名单删除按原始字符串 hash 匹配以兼容历史大写数据；如果用户一次传入 foo@example.com 和 Foo@Example.com，第二个值会被静默丢弃，取决于参数顺序，大小写精确的历史记录可能删不掉。
修复建议: 新增删除专用 normalize helper：只 trim 空白，并按原始值去重；保留 set 路径的大小写归一化去重。
如有疑问或认为判断不准确，欢迎直接回复讨论。
1 participant
Adds mail allow/block shortcut commands for managing sender rules from the CLI.
Summary by CodeRabbit
New Features
Bug Fixes
--type,--query,--page-size,--scene, and--address.Documentation