Complete mail rule reorder IDs#1729
Open
bubbmon233 wants to merge 1 commit into
Open
Conversation
github-actions
Bot
added
the
size/L
📝 WalkthroughWalkthroughThis PR adds a new mail rule reorder operation. It detects the reorder method, completes requests by merging user-supplied ChangesMail Rule Reorder Feature
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as Service CLI
participant Service as serviceMethodRun
participant Reorder as MailRuleReorder logic
participant API as Mail API
CLI->>Service: run reorder command
alt dry-run
Service->>Reorder: mailRuleReorderDryRun(request)
Reorder->>Reorder: extractRuleIDs & validate
Reorder->>API: GET current rules (described)
Reorder-->>Service: two-step dry-run plan output
else execute
Service->>Reorder: completeMailRuleReorder(request)
Reorder->>Reorder: extractRuleIDs & validate
Reorder->>API: GET current rules
API-->>Reorder: current rule IDs
Reorder->>Reorder: mergeRuleIDs(input, current)
Reorder-->>Service: updated request
Service->>API: POST reorder with merged rule_ids
API-->>Service: response
end
Service-->>CLI: result/output
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@705d2bc3066e02f87caf0cd34a41517cad59ca24🧩 Skill updatenpx skills add bubbmon233/cli#feat/275cd33 -y -g |
PR Quality SummaryCI did not complete successfully. Use the failed check links below to decide whether this PR needs a code change or a rerun. Failed checksdeterministic-gate
|
![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
🧹 Nitpick comments (2)
cmd/service/mail_rules_reorder.go (2)
27-44: 🗄️ Data Integrity & Integration | 🔵 TrivialTOCTOU window between listing current order and posting the reorder.
completeMailRuleReorderreads the current rule order via GET, computes the merged order, then a separate POST commits it (inservice.go). If another session/process changes the rule set between these two calls, the completedrule_idssent could be stale (missing a newly added rule, or containing a since-deleted one), and the reorder could fail or silently drop new rules from the order.Is there a conditional-write mechanism (e.g.,
If-Match/version token) on the reorder endpoint that this could leverage, or is this window an accepted limitation for now?Also applies to: 121-141
🤖 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 27 - 44, The reorder flow in completeMailRuleReorder has a TOCTOU gap because it fetches current IDs with listMailRuleIDs and then later submits the merged rule_ids, so the final POST can be based on stale state. Update the reorder path to use any available conditional-write/version mechanism on the APIClient request (for example, an If-Match or revision token captured alongside the GET) or, if none exists, make the limitation explicit in the service.go reorder handling and guard against stale/missing IDs as much as possible in completeMailRuleReorder, mergeRuleIDs, and the caller that submits the request.
143-159: 📐 Maintainability & Code Quality | 🔵 TrivialMalformed/unexpected list response shape is silently treated as "no rules".
If the response lacks a
data/itemsobject or the shapes don't match expectations,extractCurrentRuleIDsquietly returns an empty slice. Downstream, this surfaces as a generic "unknown rule ID" validation error for every input ID rather than signaling a response-shape problem, which could confuse debugging of an actual API/schema change.🤖 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 143 - 159, extractCurrentRuleIDs is treating malformed list responses as an empty result, which hides API/schema issues and later shows up as misleading unknown rule ID errors. Update extractCurrentRuleIDs to validate the expected result -> data -> items shape and distinguish “no rules” from “unexpected response shape”; if any required map/slice cast fails, return an explicit error (or propagate one) instead of silently returning an empty slice. Then adjust the caller in mail_rules_reorder.go to handle that error path and surface a clear response-shape failure before validation proceeds.
🤖 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 88-117: The validation test only checks the error type and message
substring, but it should assert the typed metadata too. Update
TestExtractRuleIDsValidation to verify the ValidationError from extractRuleIDs
carries the expected subtype and param values, using errs.ProblemOf or direct
field checks on validationErr, and keep the existing type assertion while
removing reliance on strings.Contains for the main validation.
- Around line 41-86: The unknown-id test in TestMergeRuleIDs should assert the
typed ValidationError metadata returned by mergeRuleIDs, not just a substring of
err.Error(). Update the "unknown id" case to verify the error is an
errs.ProblemOf with the expected category/subtype and Param("--data.rule_ids"),
while still checking the cause/content as needed; keep the successful merge
cases unchanged.
In `@cmd/service/mail_rules_reorder.go`:
- Around line 91-119: `mergeRuleIDs` can panic if `currentIDs` contains
duplicate rule IDs because it indexes `inputIDs[nextInput]` without checking
bounds. Update the logic in `mergeRuleIDs` to guard the `nextInput` access when
rebuilding `out`, and fail safely if the reordered traversal would consume more
selected IDs than are available. Keep the existing validation/error style in
this function so the fix is localized and easy to trace.
- Around line 170-202: The dry-run output currently prints the “=== Dry Run ===”
banner to the same stream as JSON, which breaks stdout-only machine parsing in
json mode. Update mailRuleReorderDryRun and printDryRunAPI so the banner is only
emitted for pretty output or sent to stderr, and make the same stdout-only
adjustment in internal/cmdutil.PrintDryRun and PrintDryRunWithFile while
preserving the JSON body behavior.
---
Nitpick comments:
In `@cmd/service/mail_rules_reorder.go`:
- Around line 27-44: The reorder flow in completeMailRuleReorder has a TOCTOU
gap because it fetches current IDs with listMailRuleIDs and then later submits
the merged rule_ids, so the final POST can be based on stale state. Update the
reorder path to use any available conditional-write/version mechanism on the
APIClient request (for example, an If-Match or revision token captured alongside
the GET) or, if none exists, make the limitation explicit in the service.go
reorder handling and guard against stale/missing IDs as much as possible in
completeMailRuleReorder, mergeRuleIDs, and the caller that submits the request.
- Around line 143-159: extractCurrentRuleIDs is treating malformed list
responses as an empty result, which hides API/schema issues and later shows up
as misleading unknown rule ID errors. Update extractCurrentRuleIDs to validate
the expected result -> data -> items shape and distinguish “no rules” from
“unexpected response shape”; if any required map/slice cast fails, return an
explicit error (or propagate one) instead of silently returning an empty slice.
Then adjust the caller in mail_rules_reorder.go to handle that error path and
surface a clear response-shape failure before validation proceeds.
🪄 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: 3a51e2db-0195-44ba-bfcb-930f06e8f1a3
📒 Files selected for processing (3)
cmd/service/mail_rules_reorder.gocmd/service/mail_rules_reorder_test.gocmd/service/service.go
Comment on lines
+41
to
+86
| func TestMergeRuleIDs(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| input []string | ||
| current []string | ||
| want []string | ||
| wantErr string | ||
| }{ | ||
| { | ||
| name: "complete input stays unchanged", | ||
| input: []string{"r3", "r2", "r1"}, | ||
| current: []string{"r1", "r2", "r3"}, | ||
| want: []string{"r3", "r2", "r1"}, | ||
| }, | ||
| { | ||
| name: "partial input fills original selected slots", | ||
| input: []string{"r3", "r1"}, | ||
| current: []string{"r1", "r2", "r3", "r4"}, | ||
| want: []string{"r3", "r2", "r1", "r4"}, | ||
| }, | ||
| { | ||
| name: "unknown id", | ||
| input: []string{"r9"}, | ||
| current: []string{"r1", "r2"}, | ||
| wantErr: "unknown rule ID", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| got, err := mergeRuleIDs(tt.input, tt.current) | ||
| if tt.wantErr != "" { | ||
| if err == nil || !strings.Contains(err.Error(), tt.wantErr) { | ||
| t.Fatalf("mergeRuleIDs error = %v, want containing %q", err, tt.wantErr) | ||
| } | ||
| return | ||
| } | ||
| if err != nil { | ||
| t.Fatalf("mergeRuleIDs unexpected error: %v", err) | ||
| } | ||
| if strings.Join(got, ",") != strings.Join(tt.want, ",") { | ||
| t.Fatalf("mergeRuleIDs = %v, want %v", got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Assert typed error metadata, not just message substrings.
The "unknown id" case only checks err.Error() contains "unknown rule ID". mergeRuleIDs returns a typed errs.ValidationError with SubtypeFailedPrecondition and Param("--data.rule_ids") (per mergeRuleIDs in mail_rules_reorder.go); asserting only the message text won't catch regressions in classification/param, and won't survive wording changes.
🔧 Proposed fix
{
name: "unknown id",
input: []string{"r9"},
current: []string{"r1", "r2"},
- wantErr: "unknown rule ID",
},
...
got, err := mergeRuleIDs(tt.input, tt.current)
- if tt.wantErr != "" {
- if err == nil || !strings.Contains(err.Error(), tt.wantErr) {
- t.Fatalf("mergeRuleIDs error = %v, want containing %q", err, tt.wantErr)
- }
+ if tt.name == "unknown id" {
+ requireProblem(t, err, errs.CategoryValidation, errs.SubtypeFailedPrecondition, 0)
return
}As per path instructions, **/*_test.go: "Error-path tests must assert typed metadata via errs.ProblemOf (category / subtype / param) and cause preservation, not message substrings alone".
📝 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
| func TestMergeRuleIDs(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| input []string | |
| current []string | |
| want []string | |
| wantErr string | |
| }{ | |
| { | |
| name: "complete input stays unchanged", | |
| input: []string{"r3", "r2", "r1"}, | |
| current: []string{"r1", "r2", "r3"}, | |
| want: []string{"r3", "r2", "r1"}, | |
| }, | |
| { | |
| name: "partial input fills original selected slots", | |
| input: []string{"r3", "r1"}, | |
| current: []string{"r1", "r2", "r3", "r4"}, | |
| want: []string{"r3", "r2", "r1", "r4"}, | |
| }, | |
| { | |
| name: "unknown id", | |
| input: []string{"r9"}, | |
| current: []string{"r1", "r2"}, | |
| wantErr: "unknown rule ID", | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| got, err := mergeRuleIDs(tt.input, tt.current) | |
| if tt.wantErr != "" { | |
| if err == nil || !strings.Contains(err.Error(), tt.wantErr) { | |
| t.Fatalf("mergeRuleIDs error = %v, want containing %q", err, tt.wantErr) | |
| } | |
| return | |
| } | |
| if err != nil { | |
| t.Fatalf("mergeRuleIDs unexpected error: %v", err) | |
| } | |
| if strings.Join(got, ",") != strings.Join(tt.want, ",") { | |
| t.Fatalf("mergeRuleIDs = %v, want %v", got, tt.want) | |
| } | |
| }) | |
| } | |
| } | |
| func TestMergeRuleIDs(t *testing.T) { | |
| tests := []struct { | |
| name string | |
| input []string | |
| current []string | |
| want []string | |
| wantErr string | |
| }{ | |
| { | |
| name: "complete input stays unchanged", | |
| input: []string{"r3", "r2", "r1"}, | |
| current: []string{"r1", "r2", "r3"}, | |
| want: []string{"r3", "r2", "r1"}, | |
| }, | |
| { | |
| name: "partial input fills original selected slots", | |
| input: []string{"r3", "r1"}, | |
| current: []string{"r1", "r2", "r3", "r4"}, | |
| want: []string{"r3", "r2", "r1", "r4"}, | |
| }, | |
| { | |
| name: "unknown id", | |
| input: []string{"r9"}, | |
| current: []string{"r1", "r2"}, | |
| }, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| got, err := mergeRuleIDs(tt.input, tt.current) | |
| if tt.name == "unknown id" { | |
| requireProblem(t, err, errs.CategoryValidation, errs.SubtypeFailedPrecondition, 0) | |
| return | |
| } | |
| if err != nil { | |
| t.Fatalf("mergeRuleIDs unexpected error: %v", err) | |
| } | |
| if strings.Join(got, ",") != strings.Join(tt.want, ",") { | |
| t.Fatalf("mergeRuleIDs = %v, want %v", got, tt.want) | |
| } | |
| }) | |
| } | |
| } |
🤖 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 41 - 86, The unknown-id
test in TestMergeRuleIDs should assert the typed ValidationError metadata
returned by mergeRuleIDs, not just a substring of err.Error(). Update the
"unknown id" case to verify the error is an errs.ProblemOf with the expected
category/subtype and Param("--data.rule_ids"), while still checking the
cause/content as needed; keep the successful merge cases unchanged.
Source: Path instructions
Comment on lines
+88
to
+117
| func TestExtractRuleIDsValidation(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| data any | ||
| wantErr string | ||
| }{ | ||
| {name: "missing", data: map[string]any{}, wantErr: "string array"}, | ||
| {name: "not array", data: map[string]any{"rule_ids": "r1"}, wantErr: "string array"}, | ||
| {name: "non string", data: map[string]any{"rule_ids": []any{"r1", 2}}, wantErr: "string array"}, | ||
| {name: "empty array", data: map[string]any{"rule_ids": []any{}}, wantErr: "at least one"}, | ||
| {name: "empty id", data: map[string]any{"rule_ids": []any{""}}, wantErr: "must not contain empty"}, | ||
| {name: "duplicate id", data: map[string]any{"rule_ids": []any{"r1", "r1"}}, wantErr: "duplicate"}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| _, err := extractRuleIDs(tt.data) | ||
| if err == nil { | ||
| t.Fatal("expected validation error") | ||
| } | ||
| var validationErr *errs.ValidationError | ||
| if !errors.As(err, &validationErr) { | ||
| t.Fatalf("expected ValidationError, got %T: %v", err, err) | ||
| } | ||
| if !strings.Contains(err.Error(), tt.wantErr) { | ||
| t.Fatalf("error = %v, want containing %q", err, tt.wantErr) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Also assert Subtype/Param here, not just error type + message.
errors.As confirms the error is a *errs.ValidationError, but the test never reads validationErr.Subtype/validationErr.Param — it falls back to strings.Contains for the actual assertion. All these cases map to SubtypeInvalidArgument with Param("--data.rule_ids") per extractRuleIDs; asserting those fields directly would satisfy the guideline and be more robust than string matching.
As per path instructions, **/*_test.go: "Error-path tests must assert typed metadata via errs.ProblemOf (category / subtype / param) and cause preservation, not message substrings alone".
🤖 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 88 - 117, The validation
test only checks the error type and message substring, but it should assert the
typed metadata too. Update TestExtractRuleIDsValidation to verify the
ValidationError from extractRuleIDs carries the expected subtype and param
values, using errs.ProblemOf or direct field checks on validationErr, and keep
the existing type assertion while removing reliance on strings.Contains for the
main validation.
Source: Path instructions
Comment on lines
+91
to
+119
| func mergeRuleIDs(inputIDs, currentIDs []string) ([]string, error) { | ||
| inputSet := map[string]struct{}{} | ||
| for _, id := range inputIDs { | ||
| inputSet[id] = struct{}{} | ||
| } | ||
| currentSet := map[string]struct{}{} | ||
| for _, id := range currentIDs { | ||
| currentSet[id] = struct{}{} | ||
| } | ||
| for _, id := range inputIDs { | ||
| if _, ok := currentSet[id]; !ok { | ||
| return nil, errs.NewValidationError(errs.SubtypeFailedPrecondition, "unknown rule ID in --data.rule_ids: %s", id). | ||
| WithParam("--data.rule_ids"). | ||
| WithHint("run: lark-cli mail user_mailbox.rules list --params '{\"user_mailbox_id\":\"<mailbox>\"}' --as user") | ||
| } | ||
| } | ||
|
|
||
| nextInput := 0 | ||
| out := make([]string, 0, len(currentIDs)) | ||
| for _, id := range currentIDs { | ||
| if _, selected := inputSet[id]; selected { | ||
| out = append(out, inputIDs[nextInput]) | ||
| nextInput++ | ||
| continue | ||
| } | ||
| out = append(out, id) | ||
| } | ||
| return out, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard against index-out-of-range if currentIDs contains a duplicate.
mergeRuleIDs only validates that each input ID exists in currentSet (a set), not that currentIDs itself is duplicate-free. If the list endpoint ever returns the same rule ID twice, inputIDs[nextInput] can be indexed past len(inputIDs)-1 for the second occurrence, panicking. Unlikely from a well-behaved API, but there's no fallback here — it's a hard crash, not a safe degradation.
🛡️ Proposed bounds guard
nextInput := 0
out := make([]string, 0, len(currentIDs))
for _, id := range currentIDs {
if _, selected := inputSet[id]; selected {
+ if nextInput >= len(inputIDs) {
+ return nil, errs.NewInternalError(errs.SubtypeUnknown, "unexpected duplicate rule ID in current order: %s", id)
+ }
out = append(out, inputIDs[nextInput])
nextInput++
continue
}
out = append(out, id)
}📝 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
| func mergeRuleIDs(inputIDs, currentIDs []string) ([]string, error) { | |
| inputSet := map[string]struct{}{} | |
| for _, id := range inputIDs { | |
| inputSet[id] = struct{}{} | |
| } | |
| currentSet := map[string]struct{}{} | |
| for _, id := range currentIDs { | |
| currentSet[id] = struct{}{} | |
| } | |
| for _, id := range inputIDs { | |
| if _, ok := currentSet[id]; !ok { | |
| return nil, errs.NewValidationError(errs.SubtypeFailedPrecondition, "unknown rule ID in --data.rule_ids: %s", id). | |
| WithParam("--data.rule_ids"). | |
| WithHint("run: lark-cli mail user_mailbox.rules list --params '{\"user_mailbox_id\":\"<mailbox>\"}' --as user") | |
| } | |
| } | |
| nextInput := 0 | |
| out := make([]string, 0, len(currentIDs)) | |
| for _, id := range currentIDs { | |
| if _, selected := inputSet[id]; selected { | |
| out = append(out, inputIDs[nextInput]) | |
| nextInput++ | |
| continue | |
| } | |
| out = append(out, id) | |
| } | |
| return out, nil | |
| } | |
| func mergeRuleIDs(inputIDs, currentIDs []string) ([]string, error) { | |
| inputSet := map[string]struct{}{} | |
| for _, id := range inputIDs { | |
| inputSet[id] = struct{}{} | |
| } | |
| currentSet := map[string]struct{}{} | |
| for _, id := range currentIDs { | |
| currentSet[id] = struct{}{} | |
| } | |
| for _, id := range inputIDs { | |
| if _, ok := currentSet[id]; !ok { | |
| return nil, errs.NewValidationError(errs.SubtypeFailedPrecondition, "unknown rule ID in --data.rule_ids: %s", id). | |
| WithParam("--data.rule_ids"). | |
| WithHint("run: lark-cli mail user_mailbox.rules list --params '{\"user_mailbox_id\":\"<mailbox>\"}' --as user") | |
| } | |
| } | |
| nextInput := 0 | |
| out := make([]string, 0, len(currentIDs)) | |
| for _, id := range currentIDs { | |
| if _, selected := inputSet[id]; selected { | |
| if nextInput >= len(inputIDs) { | |
| return nil, errs.NewInternalError(errs.SubtypeUnknown, "unexpected duplicate rule ID in current order: %s", id) | |
| } | |
| out = append(out, inputIDs[nextInput]) | |
| nextInput++ | |
| continue | |
| } | |
| out = append(out, id) | |
| } | |
| 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.go` around lines 91 - 119, `mergeRuleIDs` can
panic if `currentIDs` contains duplicate rule IDs because it indexes
`inputIDs[nextInput]` without checking bounds. Update the logic in
`mergeRuleIDs` to guard the `nextInput` access when rebuilding `out`, and fail
safely if the reordered traversal would consume more selected IDs than are
available. Keep the existing validation/error style in this function so the fix
is localized and easy to trace.
Comment on lines
+170
to
+202
| func mailRuleReorderDryRun(f *cmdutil.Factory, request client.RawApiRequest, config *core.CliConfig, format string) error { | ||
| if _, err := extractRuleIDs(request.Data); err != nil { | ||
| return err | ||
| } | ||
| dr := cmdutil.NewDryRunAPI(). | ||
| Desc("mail rule reorder completes partial rule_ids at execution time"). | ||
| GET(strings.TrimSuffix(request.URL, "/reorder")). | ||
| Desc("Step 1: list current mail rules to read the full rule ID order"). | ||
| POST(request.URL). | ||
| Desc("Step 2: replace --data.rule_ids with the completed full order, then reorder") | ||
| if !util.IsNil(request.Data) { | ||
| dr.Body(map[string]any{ | ||
| "rule_ids": "<completed from current list; selected slots keep current positions and use input order>", | ||
| "input": request.Data, | ||
| }) | ||
| } | ||
| dr.Set("as", string(request.As)) | ||
| dr.Set("appId", config.AppID) | ||
| if config.UserOpenId != "" { | ||
| dr.Set("userOpenId", config.UserOpenId) | ||
| } | ||
| return printDryRunAPI(f.IOStreams.Out, dr, format) | ||
| } | ||
|
|
||
| func printDryRunAPI(w io.Writer, dr *cmdutil.DryRunAPI, format string) error { | ||
| fmt.Fprintln(w, "=== Dry Run ===") | ||
| if format == "pretty" { | ||
| fmt.Fprint(w, dr.Format()) | ||
| return nil | ||
| } | ||
| output.PrintJson(w, dr) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether other dry-run paths mix a header with JSON on stdout.
rg -n "Dry Run" -C3 --type=go
rg -n "func PrintJson" -C5 internal/output
rg -n "func serviceDryRun|func.*PrintDryRunWithFile" -C10 --type=goRepository: larksuite/cli
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant dry-run helpers and JSON printer implementations.
git ls-files | rg '(^|/)(cmd/service/mail_rules_reorder\.go|internal/output|cmdutil|serviceDryRun|PrintDryRunWithFile|dry.?run).*\.go$' || true
# Find all dry-run related stdout/header strings and helper names.
rg -n "Dry Run|PrintDryRunWithFile|serviceDryRun|PrintJson\(" . --type=go
# Inspect the target file and the likely shared helpers.
for f in cmd/service/mail_rules_reorder.go; do
echo "### $f"
wc -l "$f"
cat -n "$f" | sed -n '150,230p'
done
# Inspect JSON printing behavior.
for f in $(git ls-files | rg '^internal/output/.*\.go$'); do
echo "### $f"
wc -l "$f"
cat -n "$f" | sed -n '1,220p'
done
# Inspect any shared dry-run helpers that might already handle headers separately.
for f in $(git ls-files | rg '(^|/)cmdutil/.*\.go$|(^|/)cmd/.*\.go$'); do
if rg -n "PrintDryRunWithFile|serviceDryRun|Dry Run|DryRun" "$f" >/dev/null 2>&1; then
echo "### $f"
wc -l "$f"
cat -n "$f" | sed -n '1,260p'
fi
doneRepository: larksuite/cli
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the shared dry-run helper and the service branch that calls it.
cat -n internal/cmdutil/dryrun.go | sed -n '200,320p'
cat -n cmd/service/service.go | sed -n '620,690p'
# Check the dry-run tests and any contract assertions around stdout/JSON.
cat -n internal/cmdutil/dryrun_test.go | sed -n '1,220p'
cat -n cmd/service/service_test.go | sed -n '280,340p'
cat -n cmd/service/service_test.go | sed -n '1060,1105p'Repository: larksuite/cli
Length of output: 16358
Dry-run JSON should stay stdout-only
printDryRunAPI writes === Dry Run === before the JSON body, and internal/cmdutil.PrintDryRun / PrintDryRunWithFile do the same. In --format json mode this breaks machine parsing; move the banner to stderr or gate it to pretty output.
🤖 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 170 - 202, The dry-run output
currently prints the “=== Dry Run ===” banner to the same stream as JSON, which
breaks stdout-only machine parsing in json mode. Update mailRuleReorderDryRun
and printDryRunAPI so the banner is only emitted for pretty output or sent to
stderr, and make the same stdout-only adjustment in internal/cmdutil.PrintDryRun
and PrintDryRunWithFile while preserving the JSON body behavior.
Source: Path instructions
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.
Completes the mail rule reorder command by expanding partial rule ID input into the full current rule order before calling the reorder endpoint.
Changes:
Summary by CodeRabbit
New Features
Bug Fixes