Fix mail rule reorder with partial IDs - #2175
Conversation
Fetch the current mailbox rules with the same service command context before calling reorder, validate the requested IDs locally, and submit the completed rule ID order. Document partial reorder input behavior for the mail skill reference. Test: go test ./cmd/service Co-authored-by: TRAE CLI <noreply@bytedance.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe CLI preprocesses mail-rule reorder requests. It validates ChangesMail-rule reorder flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant serviceMethodRun
participant MailRulesListAPI
participant MailRulesReorderAPI
CLI->>serviceMethodRun: submit reorder request
serviceMethodRun->>MailRulesListAPI: retrieve existing rule IDs
MailRulesListAPI-->>serviceMethodRun: return paginated rule IDs
serviceMethodRun->>MailRulesReorderAPI: submit completed rule order
MailRulesReorderAPI-->>CLI: return reorder response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 3
🧹 Nitpick comments (5)
cmd/service/mail_rules_reorder.go (2)
149-158: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider restricting the parameters that are copied to the list request.
copyRequestParamsWithoutPageTokenforwards every undeclared query parameter from the reorder request to the list request. The service runner passes undeclared--paramskeys through verbatim (buildServiceRequest, Line 614). Such a key can be meaningful for reorder and rejected by the list endpoint. An allowlist (for examplepage_sizeonly) keeps the list call stable.🤖 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 `@cmd/service/mail_rules_reorder.go` around lines 149 - 158, Update copyRequestParamsWithoutPageToken to copy only query parameters supported by the list request, such as page_size, instead of forwarding every key; continue excluding page_token and preserve the existing output map behavior.
160-189: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEmpty
rule_idwith a non-stringidproduces a confusing state.At Line 179, if
rule["rule_id"]is an empty string,okistrueandidis"". Line 181 then reassigns both fromrule["id"]. This path is correct, but the double-fallback is hard to follow. A single helper that returns the first non-empty string field is clearer.♻️ Proposed simplification
- id, ok := rule["rule_id"].(string) - if !ok || id == "" { - id, ok = rule["id"].(string) - } - if !ok || id == "" { + id := firstNonEmptyString(rule, "rule_id", "id") + if id == "" { return nil, errs.NewInternalError(errs.SubtypeInvalidResponse, "mail rules list item %d missing rule_id", i) }func firstNonEmptyString(m map[string]any, keys ...string) string { for _, k := range keys { if s, ok := m[k].(string); ok && s != "" { return s } } return "" }🤖 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 `@cmd/service/mail_rules_reorder.go` around lines 160 - 189, In extractMailRuleIDs, replace the current rule_id/id type-and-empty checks with a shared firstNonEmptyString helper that checks those fields in order and returns the first non-empty string. Preserve the existing invalid-response error when neither field yields a valid ID, including the item index.cmd/service/mail_rules_reorder_test.go (2)
54-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the
cobraCommandShimindirection.
newMailRulesReorderCommandwrapsSetArgsandExecutein a struct of function fields.TestMailRulesReorder_ListUsesRulesBaseWhenReorderHasSubpathat Lines 249-267 uses the*cobra.Commanddirectly. Return*cobra.Commandfrom the helper and delete the shim for one consistent pattern.🤖 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 `@cmd/service/mail_rules_reorder_test.go` around lines 54 - 64, Remove the cobraCommandShim type and update newMailRulesReorderCommand to return the underlying *cobra.Command directly alongside the existing values. Adjust callers, including TestMailRulesReorder_ListUsesRulesBaseWhenReorderHasSubpath, to invoke SetArgs and Execute on that command without the wrapper.
149-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that no API call occurs.
The test name states that validation errors do not call APIs. The test registers no stubs, so the claim rests on the mock registry rejecting unmatched requests. Make the contract explicit: register a reusable list stub and a reusable reorder stub, then assert that neither was hit. The test then fails if preprocessing calls the list endpoint before validation.
🤖 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 `@cmd/service/mail_rules_reorder_test.go` around lines 149 - 167, Update TestMailRulesReorder_ValidationErrorsDoNotCallAPIs to register reusable list and reorder API stubs, retain references to both stubs, and assert neither was hit after each validation case. Ensure the assertions cover preprocessing as well as the reorder request, while preserving the existing validation-message checks.skills/lark-mail/references/lark-mail-rules.md (1)
5-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win建议补充所需权限说明。
reorder 现在会先调用
user_mailbox.rules list。因此该命令除 reorder 权限外，还需要规则列表读取权限。仅具备 reorder 权限的凭证现在会收到 permission error。请在此处说明该新增权限要求，以便用户提前配置。（说明：静态检查将 Line 7 的“补齐”标记为疑似笔误，属误报，无需修改。）
🤖 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 `@skills/lark-mail/references/lark-mail-rules.md` around lines 5 - 16, 在“重排序”说明中补充权限要求：由于 CLI 会先调用 user_mailbox.rules list，执行 reorder 的凭证除 reorder 权限外还必须具备规则列表读取权限；仅有 reorder 权限时应提示会收到 permission error。保留现有“补齐”表述不变。Source: Linters/SAST tools
🤖 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 `@cmd/service/mail_rules_reorder_test.go`:
- Around line 275-284: Update cmd/service/mail_rules_reorder_test.go:275-284 in
assertServiceValidationError to assert validationErr.Param equals "rule_ids" and
verify Category and Subtype via errs.ProblemOf, while retaining the type check.
At cmd/service/mail_rules_reorder_test.go:193-196 and :211-214, extend the list
and reorder failure assertions for the *errs.APIError to validate Category and
Subtype through errs.ProblemOf and confirm the original cause is preserved; do
not rely only on message substrings.
- Around line 107-112: Update the OnMatch callback in the reorder tests,
including the cases around the shared mailbox assertions, to capture decoded
rule IDs in a variable instead of calling t.Fatalf there. After cmd.execute()
returns, assert the captured IDs on the test goroutine; apply the same change to
the corresponding callbacks around the later test cases.
In `@cmd/service/service.go`:
- Around line 433-436: Document in lark-mail-rules.md that
mail.user_mailbox.rules.reorder --dry-run displays the user-supplied,
potentially incomplete rule_ids, while real execution completes the list via
maybeCompleteMailRulesReorderIDs before sending; clarify that the dry-run body
is not the final payload.
---
Nitpick comments:
In `@cmd/service/mail_rules_reorder_test.go`:
- Around line 54-64: Remove the cobraCommandShim type and update
newMailRulesReorderCommand to return the underlying *cobra.Command directly
alongside the existing values. Adjust callers, including
TestMailRulesReorder_ListUsesRulesBaseWhenReorderHasSubpath, to invoke SetArgs
and Execute on that command without the wrapper.
- Around line 149-167: Update TestMailRulesReorder_ValidationErrorsDoNotCallAPIs
to register reusable list and reorder API stubs, retain references to both
stubs, and assert neither was hit after each validation case. Ensure the
assertions cover preprocessing as well as the reorder request, while preserving
the existing validation-message checks.
In `@cmd/service/mail_rules_reorder.go`:
- Around line 149-158: Update copyRequestParamsWithoutPageToken to copy only
query parameters supported by the list request, such as page_size, instead of
forwarding every key; continue excluding page_token and preserve the existing
output map behavior.
- Around line 160-189: In extractMailRuleIDs, replace the current rule_id/id
type-and-empty checks with a shared firstNonEmptyString helper that checks those
fields in order and returns the first non-empty string. Preserve the existing
invalid-response error when neither field yields a valid ID, including the item
index.
In `@skills/lark-mail/references/lark-mail-rules.md`:
- Around line 5-16: 在“重排序”说明中补充权限要求：由于 CLI 会先调用 user_mailbox.rules list，执行
reorder 的凭证除 reorder 权限外还必须具备规则列表读取权限；仅有 reorder 权限时应提示会收到 permission
error。保留现有“补齐”表述不变。
🪄 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 Plus
Run ID: 8b4ee069-5daa-4ca2-b02a-e7ff9537a432
📒 Files selected for processing (4)
cmd/service/mail_rules_reorder.gocmd/service/mail_rules_reorder_test.gocmd/service/service.goskills/lark-mail/references/lark-mail-rules.md
| if err := maybeCompleteMailRulesReorderIDs(opts.Ctx, ac, opts, &request, checkErr); err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--dry-run prints the uncompleted rule_ids.
The dry-run branch returns at Line 408, before this hook runs. For mail.user_mailbox.rules.reorder, --dry-run therefore prints the partial rule_ids that the user supplied, while a real run sends the completed list. Completion requires a list API call, so skipping it in dry-run is defensible. Document this difference in skills/lark-mail/references/lark-mail-rules.md so users do not treat the dry-run body as the final payload.
🤖 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 `@cmd/service/service.go` around lines 433 - 436, Document in
lark-mail-rules.md that mail.user_mailbox.rules.reorder --dry-run displays the
user-supplied, potentially incomplete rule_ids, while real execution completes
the list via maybeCompleteMailRulesReorderIDs before sending; clarify that the
dry-run body is not the final payload.
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@92eea87bf53b2f4c2febe93d850d87ada68c3f8c🧩 Skill updatenpx skills add yangr-happy/cli#feat/ebe347c -y -g |
Complete mail rule reorder IDs before dry-run output so printed requests match executed requests. Move HTTP mock assertions out of OnMatch callbacks and assert typed error metadata for reorder failures. Change-Type: ci-fix Co-authored-by: TRAE CLI <noreply@bytedance.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/service/mail_rules_reorder_test.go`:
- Around line 333-337: Update interfaceSliceToStrings to return an error
alongside the string slice, rejecting non-array inputs and any item that is not
a string instead of using fmt.Sprint. In every OnMatch callback that invokes
this helper, capture the returned error and assert it after cmd.execute()
completes, while preserving valid string-array handling.
🪄 Autofix
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 Plus
Run ID: 52b3c6da-2f4e-4d37-b4b3-985edd99ff1e
📒 Files selected for processing (2)
cmd/service/mail_rules_reorder_test.gocmd/service/service.go
Reject non-string rule_ids in reorder test helpers so mocked request assertions fail on invalid JSON element types. Change-Type: ci-fix Co-authored-by: TRAE CLI <noreply@bytedance.com>
Read the reordered request body from the dry-run API call envelope so the test validates the completed rule_ids actually sent by the command. Change-Type: ci-fix Co-authored-by: TRAE CLI <noreply@bytedance.com>
Change-Type: ci-fix Co-authored-by: TRAE CLI <noreply@bytedance.com>
yangr-happy
left a comment
yangr-happy
left a comment
There was a problem hiding this comment.
🤖 AI Review | CR 汇总 | 有风险（1 个 P1，5 个 P2/P3）
本次针对 feat/ebe347c（4 个文件，+600/-3）做了安全、业务、对抗三视角审查。没有发现 P0 安全问题：list 与 reorder 复用同一 As 身份和同一份 params（copyRequestParamsWithoutPageToken 只剔除 page_token），mailbox 上下文一致且有单测断言；未引入日志泄露、越权或路径拼接风险。
| 级别 | 数量 | 说明 |
|---|---|---|
| P0 | 0 | — |
| P1 | 1 | 分页中途失败被静默吞掉，可能用不完整的规则列表去 reorder |
| P2 | 2 | --dry-run 变成会发真实请求；空规则列表报 internal error |
| P3 | 3 | service.go 残留恒 no-op 的第二次补齐调用；远端 meta 假设无保护；错误分支缺单测 |
需要重点确认的是 P1：PaginateAll 在第 2 页及以后失败时返回 nil error，且合并结果的顶层 code 取自第一页，因此 checkErr 看不到中途失败。技术方案「若接口分页，CLI 必须迭代到 exhausted」这条约束在分页成功时已满足（TestMailRulesReorder_ListPaginationFetchesAllRules 有覆盖），但分页中途失败时会退化为静默截断——恰好落在本 PR 要解决的问题域里，建议合入前补一个 has_more 完整性校验。
其余 P2/P3 不阻断合入，建议一并处理。已有的 CodeRabbit 评论（t.Fatalf in OnMatch、断言依赖消息子串、测试投影类型）本次未重复提出。
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if apiErr := checkErr(result, reorderRequest.As); apiErr != nil { |
There was a problem hiding this comment.
🤖 AI Review | [P1 稳定性] 分页中途失败会被静默吞掉，导致用不完整的规则列表去 reorder
PaginateAll 内部的 paginateLoop（internal/client/client.go:376-397）在第 2 页及以后失败时并不返回 error：网络错误走 break，业务错误（code != 0）也是 append 后 break，两种情况最终都返回 allResults, nil。而 mergePagedResults 的顶层字段（含 code）是从第一页复制的，所以第 139 行的 checkErr(result, ...) 只能看到第一页的 code: 0，中途失败完全感知不到。listAllMailRuleIDs 也没有检查合并结果里的 data.has_more。
具体失败场景：某邮箱规则有 2 页，第 2 页请求超时。此时 PaginateAll 只返回第一页原始响应（len(results)==1 直接返回 results[0]，其 has_more 仍为 true），existingIDs 只含第一页 ID。接着分两种走向——(a) 用户要移动的规则正好在第 2 页：completeMailRuleIDs 报 unknown rule_id: xxx，误导用户以为该规则不存在；(b) 用户要移动的规则在第 1 页：补齐出的 rule_ids 缺少第 2 页规则，reorder 仍会触发后端“缺少规则 ID”的报错，本 PR 想解决的问题在分页场景下并未解决。
修复建议: PaginateAll 返回后先校验完整性再继续——取 data["has_more"]，为 true 时直接返回明确错误（例如 failed to fetch the complete rule list, please retry）而不是继续调用 reorder；同时补一条“第 2 页返回 code != 0 / 网络失败”的 httpmock 单测，断言不发出 reorder 请求。
如有疑问或认为判断不准确，欢迎直接回复讨论。
| if err != nil { | ||
| return err | ||
| } | ||
| if err := maybeCompleteMailRulesReorderIDs(opts.Ctx, ac, opts, &request, ac.CheckResponse); err != nil { |
There was a problem hiding this comment.
🤖 AI Review | [P2 正确性] --dry-run 现在会发起真实网络请求并要求有效凭据
补齐逻辑被放在 if opts.DryRun 分支之前，因此 reorder 这一条命令的 dry-run 语义发生了变化：改动前 dry-run 在 f.NewAPIClientWithConfig 之前就返回，全程不建 client、不发请求；改动后 dry-run 会构造 API client 并真实发出一次 list 请求（还会往 stderr 打 [page N] fetching...）。凭据缺失或过期、list 接口报错时，lark-cli mail user_mailbox.rules reorder --dry-run 会直接失败退出。
这与仓库自己的说明不一致：skills/lark-shared/SKILL.md:206 写的是「用 --dry-run 预览危险请求……它不触发门禁，会打印完整请求详情」，使用者据此认为 dry-run 是纯本地预览。同理，这次 list 调用也排在下方 RiskHighRiskWrite 确认门禁之前，即用户还没确认就已经打过一次接口。
修复建议: 保留当前行为（dry-run 展示补齐后的 body 确实更有用），但把代价写清楚——在本 PR 已经改动的 skills/lark-mail/references/lark-mail-rules.md 「重排序」小节补一句：reorder 的 --dry-run 会先发起一次只读 list 请求，需要有效凭据，list 失败时 dry-run 也会失败。
如有疑问或认为判断不准确，欢迎直接回复讨论。
| // with MissingScopes / Identity / ConsoleURL populated from the response. | ||
| checkErr := ac.CheckResponse | ||
|
|
||
| if !mailRulesReorderCompleted { |
There was a problem hiding this comment.
🤖 AI Review | [P3 过程痕迹] 第二处补齐调用恒为 no-op，是上一版实现的残留
mailRulesReorderCompleted 只可能在 opts.SchemaPath == mailRulesReorderSchemaPath 时为 true。因此走到第 448 行、条件成立（!completed）时，必然有 opts.SchemaPath != mailRulesReorderSchemaPath，而 maybeCompleteMailRulesReorderIDs 的第一行就是 if opts.SchemaPath != mailRulesReorderSchemaPath { return nil } —— 这个分支在任何输入下都不做事。
它连同 mailRulesReorderCompleted 标志位，是「补齐发生在 client 创建之后」那一版实现留下的痕迹；补齐上移到 dry-run 之前后就失去了作用，只剩下让读者误以为存在第二条补齐路径。
修复建议: 删除第 448-452 行整块和第 405、414 行的 mailRulesReorderCompleted 变量；第 430 行的 if ac == nil { ac, err = ... } 延迟创建保留即可，语义已经完整。
如有疑问或认为判断不准确，欢迎直接回复讨论。
| rules, ok = data["rules"].([]any) | ||
| } | ||
| if !ok { | ||
| return nil, errs.NewInternalError(errs.SubtypeInvalidResponse, "mail rules list response missing items") |
There was a problem hiding this comment.
🤖 AI Review | [P2 正确性] 规则列表为空时报的是 internal error，而不是可操作的校验错误
data 里既没有 items 也没有 rules 时，这里直接返回 InternalError(invalid_response)。但 Lark OAPI 在列表为空时通常会省略数组字段（或返回 null），这属于正常响应，被当成了响应格式非法。
具体失败场景：新邮箱一条收信规则都没有，用户执行 reorder --data '{"rule_ids":["r1"]}'，拿到的是 mail rules list response missing items 这种内部错误，既看不出问题在哪、也不知道该怎么办；而按设计本应落到 completeMailRuleIDs 的 unknown rule_id: r1，那是一条明确可操作的提示。另外 PaginateAll 在 len(results)==0 时返回空 map，同样会先在第 162 行撞上 missing data。
修复建议: items / rules 缺失或为 null 时按空列表处理（ids := []string{} 后正常返回），只有字段存在但类型不是数组时才报 invalid_response；补一条空列表响应的单测，断言最终报的是 unknown rule_id。
如有疑问或认为判断不准确，欢迎直接回复讨论。
| } | ||
|
|
||
| func mailRulesListURL(reorderURL string) string { | ||
| return strings.TrimSuffix(reorderURL, "/reorder") |
There was a problem hiding this comment.
🤖 AI Review | [P3 可维护性] 对远端 meta 形状的三处硬编码假设没有任何断言或告警保护
补齐能否生效同时依赖三个硬编码假设：schema path 常量 mail.user_mailbox.rules.reorder（第 15 行）、list URL = reorder URL 去掉 /reorder 后缀（第 146 行）、响应数组字段名是 items 或 rules（第 165-167 行）。但仓内 internal/registry/meta_data_default.json 并不包含 mail rules 的定义，meta 是运行时从远端注册表拉取的，这三个假设在编译期和单测里都无法被验证——单测用的是自己造的 mailSpec() / mailRulesReorderMethod()。
具体失败场景：远端 meta 把 schema path 改名（例如 rules → rule），或把 reorder 端点从 /rules/reorder 换成 /rules/order。前者会让 maybeCompleteMailRulesReorderIDs 直接 return nil，用户悄无声息地退回到「后端报缺少规则 ID」的旧行为；后者会让 TrimSuffix 原样返回，list 请求打到 reorder 端点上拿到 404/405。两种情况都没有任何日志或告警提示补齐已失效。
修复建议: 在 mailRulesListURL 里判断，当 reorder URL 既不以 /reorder 结尾、派生 URL 又与原 URL 相同时，往 stderr 打一条 warning 说明正按同路径 GET 取列表；数组字段名也可以复用 internal/client/pagination.go 已有的 output.FindArrayField，而不是再写一份 items / rules 白名单。
如有疑问或认为判断不准确，欢迎直接回复讨论。
| out = append(out, s) | ||
| } | ||
| return out | ||
| } |
There was a problem hiding this comment.
🤖 AI Review | [P3 可测试性] 错误分支基本没有单测覆盖
367 行测试集中在 happy path、分页、dry-run 和三条 validation 分支上，但 mail_rules_reorder.go 里这几条错误路径一条都没被执行到：maybeCompleteMailRulesReorderIDs 第 22 行的 mail rules reorder requires a JSON object body（--data 不是 JSON 对象）、mailRuleIDsFromBody 第 61/67 行的 rule_ids 非数组 / 元素非字符串、以及 extractMailRuleIDs 第 162/170/176/181 行的四条 invalid_response。
具体失败场景：这些分支返回的错误消息、errs 分类和 WithParam("rule_ids") 参数名都没有断言保护，后续重构（比如把校验挪进公共 helper、或调整错误 subtype）会静默改变用户看到的错误契约与 JSON stderr envelope，CI 不会有任何反应。
修复建议: 参照已有的 TestMailRulesReorder_ValidationErrorsDoNotCallAPIs 表驱动写法，补一组用例覆盖上述分支，并复用 assertServiceValidationError / requireProblem 断言错误类别与 param。
如有疑问或认为判断不准确，欢迎直接回复讨论。
1 participant
This completes mail receive-rule reorder requests before they are sent to the service.
Tested with
go test ./cmd/service.Summary by CodeRabbit
New Features
Bug Fixes
Documentation