feat(mail): add +draft-send shortcut for batch draft sending - #1017
Merged
Conversation
|
|
|
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:
📝 WalkthroughWalkthroughAdds a new ChangesDraft-Send Shortcut Implementation
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Runtime
participant MailAPI
participant LarkBackend
CLI->>Runtime: run +draft-send (draft IDs, mailbox, flags)
Runtime->>MailAPI: POST /user_mailboxes/:mb/drafts/:id/send (one per draft)
MailAPI->>LarkBackend: forward send request
LarkBackend-->>MailAPI: response {message_id, thread_id} or {detail, automation_send_disable}
MailAPI-->>Runtime: HTTP response
Runtime->>Runtime: isFatalSendErr / extractAutomationDisabledReason
Runtime-->>CLI: aggregated envelope (sent[], failed[]) or abort with ExitAPI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 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 `@shortcuts/mail/mail_draft_send.go`:
- Around line 123-127: The loop over draftIDs trims id for validation but later
uses the original untrimmed id when building API paths; update the code so you
trim once and use the trimmed value for both validation and path construction
(e.g., assign strings.TrimSpace(id) to a local trimmedID and use trimmedID in
the validation check and when building the request path). Apply the same change
to the other loop/block referenced (lines 131-135) so all uses of draftIDs use
the trimmed value 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: 69681751-04ca-4e55-8af3-2a76eddeae86
📒 Files selected for processing (4)
internal/output/lark_errors.goshortcuts/mail/mail_draft_send.goshortcuts/mail/mail_draft_send_test.goshortcuts/mail/shortcuts.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
shortcuts/mail/mail_draft_send.go (1)
123-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
--draft-idvalues before using them in API paths.Line 124 trims only for emptiness validation, but Line 135 and Line 186 still use raw IDs. Inputs like
--draft-id "d1, d2"can produce malformed draft paths due to leading spaces.🔧 Proposed fix
- draftIDs := rt.StrSlice("draft-id") + rawDraftIDs := rt.StrSlice("draft-id") - if len(draftIDs) == 0 { + if len(rawDraftIDs) == 0 { return output.ErrValidation("--draft-id is required") } - if len(draftIDs) > MaxBatchSendDrafts { + if len(rawDraftIDs) > MaxBatchSendDrafts { return output.ErrValidation( "too many drafts: %d > %d (split into multiple batches)", - len(draftIDs), MaxBatchSendDrafts) + len(rawDraftIDs), MaxBatchSendDrafts) } - for _, id := range draftIDs { - if strings.TrimSpace(id) == "" { + draftIDs := make([]string, 0, len(rawDraftIDs)) + for _, rawID := range rawDraftIDs { + id := strings.TrimSpace(rawID) + if id == "" { return output.ErrValidation("--draft-id contains empty value") } + draftIDs = append(draftIDs, id) } @@ - draftIDs := rt.StrSlice("draft-id") + rawDraftIDs := rt.StrSlice("draft-id") @@ - for _, id := range draftIDs { + for _, rawID := range rawDraftIDs { + id := strings.TrimSpace(rawID) api = api.POST(mailboxPath(mailboxID, "drafts", id, "send")) }Also applies to: 131-135, 185-187
🤖 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_draft_send.go` around lines 123 - 127, The code trims draft IDs only for emptiness but later uses the original raw values (draftIDs) when building API paths, which allows leading/trailing spaces to create malformed paths; update the draftIDs slice by normalizing each entry (e.g., strings.TrimSpace) immediately after splitting/receiving it so that all subsequent uses (the loop using id, any path construction around where draftIDs are iterated/used) operate on the trimmed values; modify the spot where draftIDs is produced to overwrite each element with its trimmed version (or create a new normalized slice) and keep the existing empty-value validation against the trimmed entries.
🧹 Nitpick comments (1)
shortcuts/mail/mail_draft_send_test.go (1)
46-79: ⚡ Quick winPin the exact flag count in metadata contract assertions.
You currently verify required flags exist, but accidental extra user-declared flags would still pass. Add an exact count assertion to keep this test a strict public-surface guard.
Proposed patch
flagByName := map[string]common.Flag{} for _, fl := range MailDraftSend.Flags { flagByName[fl.Name] = fl } + if len(MailDraftSend.Flags) != 3 { + t.Fatalf("Flags count = %d, want 3", len(MailDraftSend.Flags)) + } mailbox, ok := flagByName["mailbox"]🤖 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_draft_send_test.go` around lines 46 - 79, The test currently checks presence and properties of individual flags but doesn't fail if extra flags are added; update the test to assert the exact flag count to lock the public surface by adding a single assertion that the number of flags on MailDraftSend (e.g., len(MailDraftSend.Flags) or len(flagByName)) equals 3 (the expected flags "mailbox", "draft-id", "stop-on-error"), so any accidental additional flags cause the test to fail.
🤖 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.
Duplicate comments:
In `@shortcuts/mail/mail_draft_send.go`:
- Around line 123-127: The code trims draft IDs only for emptiness but later
uses the original raw values (draftIDs) when building API paths, which allows
leading/trailing spaces to create malformed paths; update the draftIDs slice by
normalizing each entry (e.g., strings.TrimSpace) immediately after
splitting/receiving it so that all subsequent uses (the loop using id, any path
construction around where draftIDs are iterated/used) operate on the trimmed
values; modify the spot where draftIDs is produced to overwrite each element
with its trimmed version (or create a new normalized slice) and keep the
existing empty-value validation against the trimmed entries.
---
Nitpick comments:
In `@shortcuts/mail/mail_draft_send_test.go`:
- Around line 46-79: The test currently checks presence and properties of
individual flags but doesn't fail if extra flags are added; update the test to
assert the exact flag count to lock the public surface by adding a single
assertion that the number of flags on MailDraftSend (e.g.,
len(MailDraftSend.Flags) or len(flagByName)) equals 3 (the expected flags
"mailbox", "draft-id", "stop-on-error"), so any accidental additional flags
cause the test to fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c0d8f7e3-ed2f-4ea5-adac-20f4de9a3976
📒 Files selected for processing (4)
internal/output/lark_errors.goshortcuts/mail/mail_draft_send.goshortcuts/mail/mail_draft_send_test.goshortcuts/mail/shortcuts.go
✅ Files skipped from review due to trivial changes (2)
- shortcuts/mail/shortcuts.go
- internal/output/lark_errors.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1017 +/- ##
==========================================
+ Coverage 67.85% 67.97% +0.12%
==========================================
Files 592 604 +12
Lines 55373 55905 +532
==========================================
+ Hits 37574 38004 +430
- Misses 14685 14761 +76
- Partials 3114 3140 +26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@6db414fa0d301418c106456ae5953d544426b4e8🧩 Skill updatenpx skills add xukuncx/cli#feat/cf467ea -y -g |
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 `@shortcuts/mail/mail_draft_send.go`:
- Around line 136-155: When a fatal send error is detected inside the loop
(isFatalSendErr(err)), preserve and emit the accumulated batch result before
returning: append the current draft to out.Failed (using failedDraft{DraftID:
id, Error: err.Error()}), marshal/print the out batch result to stdout (JSON
envelope) and then return the fatal error; do the same for the
automation-disabled early-return (the branch that returns output.Errorf after
calling extractAutomationDisabledReason(data))—emit the current out to stdout
first so callers receive the partial sent/failed state before the error is
returned. Ensure these changes touch the isFatalSendErr(err) branch, the
failedDraft append logic, and the extractAutomationDisabledReason(...) /
output.Errorf(...) branch.
🪄 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: f7a994b3-3d4b-4815-896e-f1e20c827a31
📒 Files selected for processing (4)
internal/output/lark_errors.goshortcuts/mail/mail_draft_send.goshortcuts/mail/mail_draft_send_test.goshortcuts/mail/shortcuts.go
✅ Files skipped from review due to trivial changes (1)
- shortcuts/mail/shortcuts.go
github-actions
Bot
added
size/L
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
shortcuts/mail/mail_draft_send.go (1)
127-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlways emit the batch envelope before aborting.
The current
hadProgress/out.hasProgress()guards still skip stdout entirely when the first draft hits a fatal orautomation_send_disabledfailure. That leaves callers with only stderr and no structuredfailed[]ledger for the attempted draft. As per coding guidelines**/*.go: stdout is data (JSON envelopes), stderr is everything else (progress, warnings, hints). Never mix them as it corrupts pipe chains.Proposed fix
if err != nil { if isFatalSendErr(err) { - hadProgress := out.hasProgress() out.Failed = append(out.Failed, failedDraft{DraftID: id, Error: err.Error()}) - if hadProgress { - emitDraftSendOutput(rt, &out) - } + emitDraftSendOutput(rt, &out) // Account- / mailbox-level failures (auth, permission, network, // quota) will repeat identically for every remaining draft — // abort immediately so the caller sees a single clear error // instead of 100 redundant failed[] entries. @@ if reason := extractAutomationDisabledReason(data); reason != "" { err := output.Errorf(output.ExitAPI, "automation_send_disabled", "automation send is disabled for this mailbox: %s", reason) - if out.hasProgress() { - out.Failed = append(out.Failed, failedDraft{DraftID: id, Error: err.Error()}) - emitDraftSendOutput(rt, &out) - } + out.Failed = append(out.Failed, failedDraft{DraftID: id, Error: err.Error()}) + emitDraftSendOutput(rt, &out) // HTTP success (code: 0) but the backend signaled automation send // is disabled — every subsequent send will fail the same way, so // abort the batch with a single descriptive error. return errAlso applies to: 145-150
🤖 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_draft_send.go` around lines 127 - 131, When a fatal send error (isFatalSendErr) is encountered we must always emit the JSON batch envelope before aborting; remove the hadProgress / out.hasProgress() guard and call emitDraftSendOutput(rt, &out) unconditionally after appending the failedDraft to out.Failed (same change in both places around emitDraftSendOutput usage). In short: inside the branches that detect isFatalSendErr (and the similar branch at 145-150), append the failedDraft to out.Failed, then always call emitDraftSendOutput(rt, &out) before returning/aborting, ensuring stdout always contains the structured envelope even for first-draft failures.
🤖 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_draft_send.go`:
- Around line 197-215: normalizeDraftSendIDs currently trims and checks empties
but allows duplicate draft IDs which can cause double-sends; update
normalizeDraftSendIDs to reject duplicate IDs by tracking seen values (e.g.,
with a map[string]struct{}) while iterating trimmed IDs, returning a validation
error when a duplicate is encountered (include the duplicate value in the
message), append only unique IDs to normalized, and ensure the
MaxBatchSendDrafts check uses the length of the unique normalized slice;
reference normalizeDraftSendIDs and MaxBatchSendDrafts when making the change.
---
Duplicate comments:
In `@shortcuts/mail/mail_draft_send.go`:
- Around line 127-131: When a fatal send error (isFatalSendErr) is encountered
we must always emit the JSON batch envelope before aborting; remove the
hadProgress / out.hasProgress() guard and call emitDraftSendOutput(rt, &out)
unconditionally after appending the failedDraft to out.Failed (same change in
both places around emitDraftSendOutput usage). In short: inside the branches
that detect isFatalSendErr (and the similar branch at 145-150), append the
failedDraft to out.Failed, then always call emitDraftSendOutput(rt, &out) before
returning/aborting, ensuring stdout always contains the structured envelope even
for first-draft failures.
🪄 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: 72771519-c511-4128-ba42-ca3e5fd79cf0
📒 Files selected for processing (5)
internal/output/lark_errors.goshortcuts/mail/mail_draft_send.goshortcuts/mail/mail_draft_send_test.goshortcuts/mail/shortcuts.gotests/cli_e2e/mail/coverage.md
✅ Files skipped from review due to trivial changes (3)
- shortcuts/mail/shortcuts.go
- internal/output/lark_errors.go
- tests/cli_e2e/mail/coverage.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shortcuts/mail/mail_draft_send_test.go (1)
548-573: ⚡ Quick winAssert dry-run step order explicitly, not just presence.
These checks currently validate only containment. A reordered dry-run plan would still pass even though the test intent says “in input order”.
Proposed test hardening
func TestMailDraftSend_DryRun(t *testing.T) { @@ s := stdout.String() - for _, want := range []string{ + assertContainsInOrder(t, s, []string{ `/user_mailboxes/me/drafts/d1/send`, `/user_mailboxes/me/drafts/d2/send`, `/user_mailboxes/me/drafts/d3/send`, `"method"`, `"POST"`, - } { - if !strings.Contains(s, want) { - t.Errorf("dry-run output missing %q; got %s", want, s) - } - } + }) } @@ func TestMailDraftSend_DryRunDirectInvocation(t *testing.T) { @@ s := string(raw) - for _, want := range []string{ + assertContainsInOrder(t, s, []string{ `/user_mailboxes/alice@example.com/drafts/d1/send`, `/user_mailboxes/alice@example.com/drafts/d2/send`, `"method":"POST"`, - } { - if !strings.Contains(s, want) { - t.Errorf("dry-run JSON missing %q; got %s", want, s) - } - } + }) } + +func assertContainsInOrder(t *testing.T, s string, wants []string) { + t.Helper() + pos := 0 + for _, want := range wants { + idx := strings.Index(s[pos:], want) + if idx < 0 { + t.Fatalf("missing %q in %s", want, s) + } + pos += idx + len(want) + } +}Also applies to: 620-627
🤖 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_draft_send_test.go` around lines 548 - 573, The test TestMailDraftSend_DryRun only checks for presence of expected lines in stdout, so a reordering would still pass; modify the test (around TestMailDraftSend_DryRun and similarly the other block at 620-627) to assert the dry-run output preserves input order by checking the index positions in the output string rather than simple containment—e.g., build the expected sequence ["/user_mailboxes/me/drafts/d1/send", "/user_mailboxes/me/drafts/d2/send", "/user_mailboxes/me/drafts/d3/send", `"method"`, `"POST"`] and verify strings.Index(stdout.String(), itemN) returns non-negative and strictly increasing for each successive item (use runMountedMailShortcut and stdout as now) so the test fails on reordered output.
🤖 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 `@tests/cli_e2e/mail/mail_draft_send_dryrun_test.go`:
- Line 91: Test currently checks only result.Stderr for the expected error
message, which is fragile because validate-stage dry-run can emit errors in
stdout; update the assertion to first assert a non-zero exit (e.g.,
assert.NotEqual(t, 0, result.ExitCode)) and then assert.Contains using the
concatenated output (e.g., assert.Contains(t, result.Stdout+result.Stderr,
tt.wantMsg, ...)) replacing the existing assert.Contains(t, result.Stderr,
tt.wantMsg, ...). Use the same result and tt.wantMsg symbols so the change is
localized to that assertion.
---
Nitpick comments:
In `@shortcuts/mail/mail_draft_send_test.go`:
- Around line 548-573: The test TestMailDraftSend_DryRun only checks for
presence of expected lines in stdout, so a reordering would still pass; modify
the test (around TestMailDraftSend_DryRun and similarly the other block at
620-627) to assert the dry-run output preserves input order by checking the
index positions in the output string rather than simple containment—e.g., build
the expected sequence ["/user_mailboxes/me/drafts/d1/send",
"/user_mailboxes/me/drafts/d2/send", "/user_mailboxes/me/drafts/d3/send",
`"method"`, `"POST"`] and verify strings.Index(stdout.String(), itemN) returns
non-negative and strictly increasing for each successive item (use
runMountedMailShortcut and stdout as now) so the test fails on reordered output.
🪄 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: 3f0cc50a-4bb2-4968-8eea-a534e8945637
📒 Files selected for processing (7)
internal/output/lark_errors.goshortcuts/mail/mail_draft_send.goshortcuts/mail/mail_draft_send_test.goshortcuts/mail/shortcuts.gotests/cli_e2e/mail/coverage.mdtests/cli_e2e/mail/mail_draft_send_dryrun_test.gotests/cli_e2e/mail/mail_draft_send_workflow_test.go
✅ Files skipped from review due to trivial changes (1)
- tests/cli_e2e/mail/coverage.md
xukuncx
force-pushed
the
feat/cf467ea
branch
2 times, most recently
from
May 26, 2026 06:53
36e2a8e to
9f62679
Compare
Add `lark-cli mail +draft-send` shortcut that takes one or more existing
draft IDs and sends each via POST /drafts/:draft_id/send sequentially.
Per-draft failures are isolated and aggregated into a structured output;
fatal failures (auth, permission, network, mailbox quota) abort the
entire batch immediately while recoverable failures honor --stop-on-error.
Also extend internal/output with six mail-send-specific errno constants
(LarkErrMailboxNotFound=4013, LarkErrMailSendQuota{User,UserExt,TenantExt},
LarkErrMailQuota, LarkErrTenantStorageLimit) consumed by isFatalSendErr.
Risk is "high-risk-write" so the framework's --yes gate applies; the
shortcut declares only the minimal mail:user_mailbox.message:send scope
to avoid asking users for permissions it does not need.
chanthuang
approved these changes
May 27, 2026
6 tasks
tuxedomm
pushed a commit
to zhumiaoxin/cli
that referenced
this pull request
Jun 6, 2026
…te#1017) Add `lark-cli mail +draft-send` shortcut that takes one or more existing draft IDs and sends each via POST /drafts/:draft_id/send sequentially. Per-draft failures are isolated and aggregated into a structured output; fatal failures (auth, permission, network, mailbox quota) abort the entire batch immediately while recoverable failures honor --stop-on-error. Also extend internal/output with six mail-send-specific errno constants (LarkErrMailboxNotFound=4013, LarkErrMailSendQuota{User,UserExt,TenantExt}, LarkErrMailQuota, LarkErrTenantStorageLimit) consumed by isFatalSendErr. Risk is "high-risk-write" so the framework's --yes gate applies; the shortcut declares only the minimal mail:user_mailbox.message:send scope to avoid asking users for permissions it does not need.
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
3 participants
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.
Generated by the harness-coding skill.
Sprints
mail +draft-sendshortcut + 6 mail send error codes + unit testsSummary
Adds
lark-cli mail +draft-send, a high-risk-write shortcut that takes one or more existing draft IDs and sends each viaPOST /drafts/:draft_id/sendsequentially. Per-draft failures are isolated and aggregated into a structured{sent[], failed[]}output; fatal failures (auth, permission, network, mailbox quota) abort the entire batch while recoverable failures honor--stop-on-error. Also extendsinternal/outputwith six new mail-send errno constants used by the new shortcut's fatal-error classifier.shortcuts/mail/mail_draft_send.go,shortcuts/mail/mail_draft_send_test.go,internal/output/lark_errors.goconstantsshortcuts/mail/shortcuts.go(registerMailDraftSend)mail:user_mailbox.message:sendonly (minimal-permission)user;Risk="high-risk-write"triggers framework--yesgateThis MR was created autonomously. Quality gates were enforced by the repo's own pre-commit hooks.
Summary by CodeRabbit
+draft-sendshortcut to batch-send existing drafts (max 50), with aggregated JSON ledger of sent/failed items, dry-run preview, confirmation gating, and--stop-on-error.