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>
📝 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
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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@98c5edb4e602af00d29b3628ea2e7db70b102ed9🧩 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
| func interfaceSliceToStrings(v interface{}) []string { | ||
| items, _ := v.([]interface{}) | ||
| out := make([]string, 0, len(items)) | ||
| for _, item := range items { | ||
| out = append(out, fmt.Sprint(item)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject non-string rule_ids values in the test projection.
fmt.Sprint(item) silently converts invalid JSON types. For example, numeric 2 becomes "2". This can let a test accept a request body with the wrong rule_ids type.
Return an error for a non-array value or a non-string item. Capture that error in each OnMatch callback and assert it after cmd.execute() returns.
Proposed fix
-func interfaceSliceToStrings(v interface{}) []string {
- items, _ := v.([]interface{})
+func interfaceSliceToStrings(v interface{}) ([]string, error) {
+ items, ok := v.([]interface{})
+ if !ok {
+ return nil, fmt.Errorf("rule_ids = %T, want array", v)
+ }
out := make([]string, 0, len(items))
- for _, item := range items {
- out = append(out, fmt.Sprint(item))
+ for i, item := range items {
+ value, ok := item.(string)
+ if !ok {
+ return nil, fmt.Errorf("rule_ids[%d] = %T, want string", i, item)
+ }
+ out = append(out, value)
}
- return out
+ return out, nil
}As per coding guidelines, parse map[string]interface{} at the boundary and never silently coerce unsupported inputs.
📝 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 interfaceSliceToStrings(v interface{}) []string { | |
| items, _ := v.([]interface{}) | |
| out := make([]string, 0, len(items)) | |
| for _, item := range items { | |
| out = append(out, fmt.Sprint(item)) | |
| func interfaceSliceToStrings(v interface{}) ([]string, error) { | |
| items, ok := v.([]interface{}) | |
| if !ok { | |
| return nil, fmt.Errorf("rule_ids = %T, want array", v) | |
| } | |
| out := make([]string, 0, len(items)) | |
| for i, item := range items { | |
| value, ok := item.(string) | |
| if !ok { | |
| return nil, fmt.Errorf("rule_ids[%d] = %T, want string", i, item) | |
| } | |
| out = append(out, value) | |
| } | |
| return out, nil | |
| } |
🤖 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 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.
Source: Coding guidelines
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